fix(maintenance): label JSONL escalation mails for wisp-compact retention - #3
Closed
austinborn wants to merge 2 commits into
Closed
fix(maintenance): label JSONL escalation mails for wisp-compact retention#3austinborn wants to merge 2 commits into
austinborn wants to merge 2 commits into
Conversation
…tion The mol-dog-jsonl order's ESCALATION mails (push-failure and spike) carry no labels today, so wisp-compact treats them as the 24h default class rather than the 7d wisp_type:escalation class its retention table defines. A factory that loses its archive remote can flood the mayor inbox with many of these escalations before the operator notices (real incident: 158-flood over 2.5 days in factory-main). This change introduces a small bash helper, send_labeled_escalation_mail, that wraps gc mail send, parses the new bead id from "Sent message <id> to <to>" on stdout, and calls bd label add <id> wisp_type:escalation. The label add is best-effort: if bd is missing or the bead id cannot be parsed, the escalation itself still reaches the mayor. Both ESCALATION send-sites in jsonl-export.sh now use the helper. Tests: - writeJsonlExportGCStubWithMailExitCode now mirrors real gc mail send's "Sent message <id> to <to>" stdout, with the id pinned by the GC_STUB_MAIL_ID env var so assertions can name it verbatim. - A default bd no-op stub is installed alongside the gc stub so existing tests don't accidentally invoke real bd from system PATH on every escalation. - Two new tests assert the label add happens on both push-failure and spike escalations. Generated by the operator's software factory. City: factory-main · Agent: local-core__builder-fm-6vo1db On behalf of: @austinborn Co-Authored-By: <operator-factory-bot> <factory-bot@actual-software.invalid>
CI's golangci-lint v2.9.0 misspell rule flagged "labelling"/"labelled" in the comments added by the previous commit. Normalize to US spellings to match the rest of the codebase. Generated by the operator's software factory. City: factory-main · Agent: local-core__builder-fm-6vo1db On behalf of: @austinborn Co-Authored-By: <operator-factory-bot> <factory-bot@actual-software.invalid>
austinborn
pushed a commit
that referenced
this pull request
May 26, 2026
…gastownhall#2082) (gastownhall#2559) Thanks to @mike-matchpoint for the clear repro and the structured options menu in gastownhall#2082 — this PR implements the smallest set of fail-closed mitigations (options #2 + #3 from the issue body) that stop the silent strand at the polecat boundary. ## Summary The polecat formula's `workspace-setup` step creates a per-bead branch `polecat/<bead-id>` and the refinery's standard scan only discovers those. Providers that skip `workspace-setup` (the reported codex case) commit on whatever branch happens to be checked out in the agent's home worktree, and the refinery silently never finds the work — beads end up "assigned to refinery, no merge target", requiring manual recovery. Two mitigations, both fail-closed: ### 1. Branch-shape gate in `mol-polecat-work.toml` (`submit-and-exit`) A new **step 1** runs before the push and before the refinery reassign: - Reads `git branch --show-current`. - Refuses to proceed if the current branch is not `polecat/{{issue}}` — prints recovery instructions, signals `gc runtime drain-ack`, and exits 1. - Also reconciles `metadata.branch` so the refinery's metadata view matches what is about to be pushed (in case `workspace-setup` recorded a divergent value). This stops the strand at the polecat boundary instead of after the bead has already advanced to "assigned to refinery". Existing step numbers shift by one (3→4 cleanup, 4→5 metadata, 5→6 reassign, 6→7 signal, 7→8 reconciler+exit); the existing `TestPolecatFormulaSignalsRefineryAfterReassign` assertions are updated to match. ### 2. `CRITICAL: Branch Convention` section in `prompt.template.md` The original prompt deferred all branch detail to the formula description. A provider that skips reading the formula now still sees the `polecat/<bead-id>` rule **inline** in the prompt, with a worked example table. ## Tests - **New** `TestPolecatFormulaSubmitHasBranchShapeGate` — asserts the gate body appears in order before the push and before the refinery reassign, AND that the `metadata.branch` reconciliation is present. - **New** `TestPolecatPromptInlinesBranchConvention` — asserts the CRITICAL section names the convention and references gastownhall#2082. - **Updated** `TestPolecatFormulaSignalsRefineryAfterReassign` — step renumber. ## Out of scope (deferred from the issue body) - **Option #1** (move per-bead worktree+branch creation into a pre-claim hook) — the more robust structural fix, but touches the supervisor/dispatch layer and warrants a maintainer design call. Belongs in a separate PR. - **Option #4** (codex-specific memory instructions) — provider-side and out of the formula+prompt surface this PR addresses. This change covers the prompt + formula layer where polecats already operate; the supervisor-side hook is its own PR. ## Files `examples/gastown/packs/gastown/formulas/mol-polecat-work.toml`, `examples/gastown/packs/gastown/agents/polecat/prompt.template.md`, `examples/gastown/gastown_test.go` (+141 / -11) Closes gastownhall#2082 --------- Co-authored-by: sjarmak <sjarmak@users.noreply.github.com>
Author
|
Superseded by #4, which adds the same |
austinborn
pushed a commit
that referenced
this pull request
Jul 24, 2026
…t read collapse (review-formulas wedge) (gastownhall#3626) ## What & why The **Review Formulas** workflow was ~85% red on `main` (last 30 runs: 22 fail / 4 pass / 4 cancelled). It is **not a required check** (required = `Check` + CodeQL `Analyze`, both green), but it runs on every push to `main` and on PRs, so it is the visible "main is broken" signal — and it represents a real production defect, not just a test flake. ### Root cause (RCA + workflow-based adversarial review) In `TestPersonalWorkFormulaCompileAndRun`, after the design-review `compose.expand` fan-out the single managed Dolt sql-server suffers a **read-side saturation collapse**: every `gc hook` ready-query times out (`rc=124`, `timeout 10`) for ~22 min until the 24-min deadline, so no agent can read the ready queue and the molecule never advances. Replacement polecats also time out — the **store itself is wedged**, not a stuck process. It is a cumulative-load threshold crossing, not a single-commit regression (`04eef3468` is a non-causal WARN→DEBUG log change). Two stacking amplifiers on the per-op-subprocess store model: 1. **(dominant)** A polecat `scale_check` makes pool demand non-event-backed, so `demandSnapshotsEnabled()` is false and `shouldRefreshDemandSnapshot` rebuilt the full desired state — including the `scale_check` subprocess `bd ready --metadata-field gc.routed_to=polecat --limit=0` — on **every** patrol tick. At `patrol_interval=100ms` that ran ~10×/s, and the metadata-filtered probe cannot use `COUNT(*)` (`doltliteCountSupported` bails on metadata filters) so it fell back to a full hydrated `List` each time. 2. **(permanence)** A query killed by `timeout 10` (SIGKILL, no clean `COM_QUIT`) orphans a server-side `Sleep` connection until `read_timeout` (was 30s), so under load orphans accumulated faster than they reaped. ## The fix (3 parts) - **`fix #1` (reconciler, the real fix):** floor patrol re-eval of a non-event-backed (`scale_check`) demand snapshot to `scaleCheckDemandMinInterval = 1s` via the new `demandSnapshotPatrolMaxAge()`. Non-patrol triggers (config reloads, sling pokes), config changes, session-fingerprint changes, and the no-event-provider case still rebuild immediately / every tick. **No-op at the 30s default `patrol_interval`** — it only bites pathologically fast (sub-second) cadences. - **`fix #3` (config hardening):** lower `DefaultDoltReadTimeoutMillis` 30000 → 15000 so orphaned per-call `Sleep` connections reap sooner. `read_timeout` is the listener idle / inter-row produce reaper (go-mysql-server `ErrRowTimeout` re-arms per row), **not** a live-query wall-clock timeout, so it cannot cut a long but steadily-producing query. Regenerated schema/docs. - **`fix #2` (test):** `review_formula_test` `patrol_interval` 100ms → 1s and bound the `scale_check` probe with `--limit=8` (pool ceiling is 3). ## Verification - New unit tests: `TestCityRuntimeDemandSnapshotThrottlesScaleCheckPatrolReeval` (throttle cadence + interval-elapse + poke-bypass + fingerprint-change rebuild) and the updated `…CachesCustomDemandCommands` table. - Full `go test ./cmd/gc/` (659s), `internal/config`, `internal/doctor`, `test/docsync` all green; `go build` + `go vet` clean; `genschema` idempotent. - **Workflow-based adversarial review** (16 agents, 10 candidate findings): 9 refuted as false alarms against the code/trace; the 1 confirmed finding was a comment-accuracy issue (fixed here). `read_timeout=15000` confirmed safe (idle/inter-row reaper, not a wall-clock query cut); `cfg==nil` throttle path confirmed unreachable in production. > "Wait for green" note: a single Review Formulas pass is not decisive (it was green ~15% even while broken). The deterministic unit test is the primary signal; CI on this PR is the integration confirmation. ## Follow-ups (out of scope; can file as beads) - Make `scaleCheckDemandMinInterval` configurable for cities needing sub-1s scale_check reaction. - Have the control-dispatcher poke the controller after creating routed pool work (mirroring sling), if sub-second fan-out scale-up is ever required. - Reduce the per-tick session-snapshot `store.List` fan-out. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
austinborn
pushed a commit
that referenced
this pull request
Jul 24, 2026
…astownhall#4146) ## What Closes the audit's cost-safety gap (P0 #3): the API had **no way to stop a running graph run** — a runaway burns LLM tokens for hours. Adds `POST /v0/city/{cityName}/runs/{run_id}/cancel` → **202**. ## How — synchronous soft-cancel Nothing in the dispatcher reads a cancel marker today (dispatch is driven purely by bead readiness), so a marker-only "record intent, let the controller converge" endpoint would be **inert**. Instead the endpoint stamps `gc.cancel_requested` on the run root and **synchronously winds the run down**: closes the root and its still-**open** member beads with a distinct `canceled` outcome (reusing the `DELETE /workflow` enumerate pattern). That **starves** the run — no ready work → no dispatch; in-flight sessions finish their current step and idle. `DELETE /workflow` stays teardown-only. - `beadmeta`: `OutcomeCanceled="canceled"` (distinct terminal outcome) + `CancelRequestedMetadataKey`. - `deriveRunStatus`: canceled outcome → `cancelled`; `cancel_requested` on an open root → `cancelling`. `deriveRunStepStatus` + `RunStepStatus` enum gain `cancelled`. - Root matching uses `sourceworkflow.IsWorkflowRoot` (kind=workflow OR graph.v2) so a graph.v2-only root the Run resource lists is cancellable, not a false 404. - Already-terminal run → 409; store failure → 503. ## Depends on gastownhall#4143 (S2) → gastownhall#4139 (S1) Third slice of the Run-resource track; branched on the S2 tip. Base is `main`, so the diff cleans to just S3 as the parents merge. ## Verification - 10 targeted tests incl. wire route (202/404), already-terminal 409, non-workflow 404, and **red-team regressions**: completed-member-untouched, store-failure→503, graph.v2-only-root cancellable. Full `internal/api` (204s) + `beadmeta` suites green; pre-push `make test-fast-parallel` passed. - **Fable red-team: two rounds.** Round 1 → DON'T-SHIP (4 findings): (1) `CloseAll` would rewrite already-completed steps' outcomes on the bd/Dolt store; (2) swallowed store errors → phantom 202; (3) canceled steps read `completed`; (4) false 404 on graph.v2-only roots. All fixed with regression tests → Round 2 **SHIP** (no new issues). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Stephanie Jarmak <36544495+sjarmak@users.noreply.github.com> Co-authored-by: sjarmak <sjarmak@users.noreply.github.com>
austinborn
pushed a commit
that referenced
this pull request
Jul 24, 2026
## Summary - classify Claude's interactive spend-limit modal as a high-confidence provider rate-limit screen - keep the match narrow by requiring the spend-limit action, usage-credit balance, and reset affordance together - add reconciler coverage proving this scrollback is quarantined as `rate_limit` before zombie healing/crash telemetry ## Root cause The reconciler already suppresses generic `session.crashed` telemetry when zombie scrollback is recognized as a provider rate-limit screen. Claude's spend-limit modal (`Adjust monthly spend limit` / `Usage credit balance` / `Wait for limit to reset`) was not in that classifier, so a provider billing ceiling could wedge sessions and surface as fleet-wide `zombie process` crashes instead of rate-limit quarantine. Refs gastownhall#4093 (this lands scoped-fix #1; suggested-fixes #2 auto-dismiss and #3 consolidated ops alert tracked separately). ## Validation - `go test ./internal/runtime -run 'TestContainsProviderRateLimitScreen|TestProviderTerminalErrorReason'` - `CGO_CPPFLAGS="-I$(brew --prefix icu4c@78)/include" CGO_LDFLAGS="-L$(brew --prefix icu4c@78)/lib" go test ./cmd/gc -run 'TestReconcileSessionBeads_(RateLimitScreenQuarantinesBeforeHeal|SpendLimitModalQuarantinesBeforeHeal|RateLimitScreenBeyondCrashCaptureSuppressesTelemetry)|TestCheckStability_RateLimitScreen_DoesNotCountAsCrash'` - `CGO_CPPFLAGS="-I$(brew --prefix icu4c@78)/include" CGO_LDFLAGS="-L$(brew --prefix icu4c@78)/lib" go test ./internal/runtime -count=1` - `CGO_CPPFLAGS="-I$(brew --prefix icu4c@78)/include" CGO_LDFLAGS="-L$(brew --prefix icu4c@78)/lib" go test ./cmd/gc -run 'TestReconcileSessionBeads_(RateLimitScreenQuarantinesBeforeHeal|SpendLimitModalQuarantinesBeforeHeal|RateLimitScreenBeyondCrashCaptureSuppressesTelemetry)|TestCheckStability_RateLimitScreen_DoesNotCountAsCrash' -count=1` --------- Co-authored-by: sjarmak <sjarmak@users.noreply.github.com>
austinborn
pushed a commit
that referenced
this pull request
Jul 24, 2026
…townhall#4245) (gastownhall#4248) Fixes gastownhall#4245 (partially — see Scope note). ### Bug `beads.OpenStoreAtForCity` already computes a `BeadsDiagnostic` naming the preflight gate and reason whenever native-store eligibility fails and store selection falls back to the fork-per-op `BdStore` (which execs a `bd` CLI process — and opens 3 SQL connections — per store operation). But `cmd/gc`'s doctor wiring (`openStoreAtForCity` in `main.go`) only ever kept the opened `Store` from that result and discarded the `Diagnostic`. `BeadsStoreCheck` (`gc doctor`'s `beads-store` check) never saw it, so it can only ever report `✓ store accessible` — a city silently running the dramatically more expensive fallback looks identical to a healthy native-store city. The reporter measured ~27 `bd` forks/second (~80 SQL connections/sec) sustaining 1-4 Dolt cores for hours before the fallback was found via manual process-spawn sampling. ### Fix Added `openStoreResultForCity` alongside the existing `openStoreForCity` in `cmd/gc/cmd_doctor.go` — same underlying `openStoreResultAtForCity`, but preserving the `Diagnostic` instead of discarding it. Changed `BeadsStoreCheck`'s factory field from `func(string) (beads.Store, error)` to `func(string) (beads.StoreOpenResult, error)` and wired the new factory into `gc doctor`'s registration. After a successful open+ping, `Run` now checks `Diagnostic.Store == beads.BeadsStoreNameBdStore` (set only on the actual fallback path, confirmed by reading `internal/beads/factory.go`) and reports `StatusWarning` naming the gate and reason instead of `StatusOK`. `StatusWarning` never contributes to `gc doctor`'s blocking exit code (only `StatusError` with `SeverityBlocking` does — confirmed in `doctor.go`), so this is purely additive visibility, matching the issue's own "Expected" behavior. ### Validation - New tests `TestBeadsStoreCheck_WarnsOnBdStoreFallback` (asserts `StatusWarning`, message names the gate and reason, non-empty `FixHint`) and `TestBeadsStoreCheck_NativeStoreDiagnosticStaysOK` (inverse — a native-store diagnostic stays `StatusOK`). TDD RED confirmed against the pre-fix `Run` (both new tests written, the warning test failed with `status = 0 (OK), want Warning` before the diagnostic branch was added) → GREEN after. - All 8 pre-existing `BeadsStoreCheck` tests updated for the factory signature change (mechanical: wrap returned stores in `beads.StoreOpenResult{Store: ...}`) and still pass unchanged in behavior. - Full `internal/doctor` suite green. `cmd/gc` doctor/store/status-focused tests (`Doctor|BeadsStore|OpenStoreResult|CityStatus` filter) green. Full `cmd/gc` sharded suite (`GC_FAST_UNIT=1 test-go-test-shard ./cmd/gc 1 6`) green except one confirmed pre-existing, unrelated timing flake (`TestStopManagedCityForcesCleanupAfterTimeout`, `cmd_supervisor_test.go` — reproduces intermittently in isolated re-runs regardless of this diff; touches only managed-city stop/cleanup timing, disjoint from the beads-store diagnostic path this PR changes). - `go build ./...` and `go vet ./...` clean across the full workspace; `gofmt -l` clean on all touched files. ### Scope note This PR implements only the issue's suggested fix #1 (a `gc doctor` warning). Deliberately left out of scope: - Suggested fix #2 (`gc status` human-output line showing `store: BdStore (fallback: <gate>)`) — a separate command's output formatting, not required to close the "silent" problem since `gc doctor` is the primary operator health-check surface. - Suggested fix #3 (periodic re-preflight so long-lived processes can upgrade off the fallback once its gate clears) — a bigger lifecycle change to a supervisor/session-scoped process, not a same-day fix. - Surfacing `PreflightResult.RepairSteps` in the warning message — that data lives on `contract.PreflightResult`, not on the `BeadsDiagnostic` struct that reaches the doctor check; threading it through would touch `diagnosticFromPreflight` and widen `BeadsDiagnostic`'s shape beyond what's needed to make the fallback visible. Gate + reason (already on `BeadsDiagnostic`) covers the issue's core ask. Happy to follow up if useful. - gastownhall#4246 (the fallback's per-tick polling cost / caching) is a distinct, larger issue about reducing `BdStore`'s cost via cursors/caching — correctly filed separately by the reporter; not touched here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gc mail sendescalation sites injsonl-export.shwith a small bash helper that captures the new bead id fromgc mail sendstdout and callsbd label add <id> wisp_type:escalationafterwards, so wisp-compact's retention policy applies (7d TTL instead of the 24h default).Context
The motivating incident was a 158-mail flood over 2.5 days that hit a single factory's mayor inbox when its archive repo lost its origin remote and every 15-minute cooldown of mol-dog-jsonl re-escalated. The root-cause fix (recognising no-origin as "local-only mode" rather than push-failure) is already in this script at `origin/main` HEAD (gascity gastownhall#1800 / gastownhall#2243). This PR adds the second half: even when escalation IS the right call, the resulting bead should carry the right retention label so it doesn't outlive its usefulness in the mayor inbox.
Test plan
Notes for reviewers