Repository navigation
Conversation
quad341
added a commit
that referenced
this pull request
May 31, 2026
…ga-gdc6) Addresses Deployer gate FAIL #4 on ga-gdc6: test/docsync.TestSchemaFreshness required city-schema.json / config.md to match source after ga-zol (config) and ga-zn8 (maintenance CLI) landed. Regenerated via: go run ./cmd/genschema No code changes; generated artifacts only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Jul 14, 2026
…reates (P0 #4 S2) (gastownhall#4150) ## What S2 of **audit P0 #4 — `Idempotency-Key` on mutating create endpoints**: wires the five batch-A creates through the `withIdempotency` helper from S1 (gastownhall#4147) so a timed-out client retry cannot mint a duplicate: - `create-agent` (POST /agents) - `create-provider` (POST /providers) - `create-rig` (POST /rigs) - `create-convoy` (POST /convoys) - `add-pack` (POST /packs) **Depends on gastownhall#4147 (S1).** Base=main per the pipeline convention — the diff shows both slices until S1 merges, then auto-cleans to S2 only. Left unlabeled until gastownhall#4147 merges. ## How each endpoint is wired Each input struct gains the optional `Idempotency-Key` header field, and **all fallible work moves into the `create()` closure** — validation, the pack SSRF fence, the config/store write, the agent visibility wait, the convoy link loop + rollback — so the helper's deferred release covers every error and panic path (no leaked reservations). Replay caches the response **body value** only: | endpoint | cached `T` | why | |---|---|---| | create-agent / provider / rig | `string` (the name) | output envelopes have anonymous inline bodies `{status, name}` — naming them would rename generated OpenAPI schema components; the body is rebuilt from the name | | create-convoy | `beads.Bead` | `IndexOutput` envelope; `X-GC-Index` recomputed fresh on replay (the create-bead pattern) | | add-pack | `importsvc.AddResult` | the body echoes all four fields | Cache paths are static (`/v0/agents`, …) — safe because the idempotency cache lives on the **per-city `Server`** (`getCityServer` caches one Server per city), the same property the beads/mail slices already rely on. ## Deliberate semantics (unchanged from an unkeyed retry) A create that fails **after** a durable write (agent visibility timeout 503/504, pack lockfile, convoy rollback failure) releases the reservation, so a same-key retry re-runs the create and surfaces the conflict (409 already-exists) rather than replaying success. Idempotency replays only completed successes; it cannot invent one. ## Contract changes - 5 ops gain the optional `Idempotency-Key` header parameter (spec + Go client + dashboard TS client regenerated and committed). - `create-convoy` now declares 409 (`idempotency-in-flight`); the other four already declared 409. - Guard lists (`TestCreateEndpointsAreTriagedForIdempotency`): the 5 opids moved `pending` → `require`. ## Tests / gates - New `idempotency_endpoints_test.go`: per-endpoint replay-without-re-running-create (config/store/import-call counting) + a wire-level 422 mismatch test. - Helper test now pins that the **key participates in the cache scope** (mutation-verified: dropping `+ key` from `scopedKey` fails two tests). - `gofmt`/`go vet` clean; full `internal/api` suite green; `TestOpenAPISpecInSync`, `TestGeneratedClientInSync`, `make dashboard-check` all pass. - Red-teamed pre-commit (5 lenses + adversarial verification, 13 agents): no code defects confirmed; the mutation-hardening test and explicit staging of the new test file came out of that review. ## Follow-ups S3 wires reply-mail, register-extmsg-adapter, emit-event, and supervisor create-city (needs the `withIdempotency` receiver generalized from `*Server` to the cache, since `SupervisorMux` has no Server), and moves `ensure-extmsg-group` → exempt (identity-idempotent by design). S4 (deferred): the session creates. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Jul 14, 2026
gastownhall#4155) ## What `isBdTransientWriteError` gets a deliberately conservative sqlite arm: bd write errors carrying an explicit `SQLITE_BUSY` / `SQLITE_LOCKED` result-code marker (e.g. `database is locked (5) (SQLITE_BUSY)`) are now classified transient and flow through the existing bounded-backoff write retry loop, exactly like Dolt serialization failures. Only the unambiguous code markers match — bd's sqlite driver (modernc.org/sqlite) always appends them, so no real coverage is lost, while bare "database is locked" phrasings stay excluded on purpose: Dolt's embedded mode emits `database is locked by another dolt process` for a persistent lock-file condition that a bounded retry cannot clear and must keep failing fast. Two hardening commits ride along in `cmd/gc` (see Why): a nil-`stderr` guard in `computeWorkSet`, and `t.TempDir()` instead of literal `/tmp` as the city dir in the `computeWorkSet` tests. ## Why The sqlite concurrency audit of the infra store (defect #4, CONFIRMED) found the write classifier matched only Dolt/MySQL needles, so a `SQLITE_BUSY` from a sqlite-backed bd was classified permanent and never retried: any write that lost a lock race failed on first contention. The audit's empirical hammer proved the trigger window is real and wide — because bd reads also take the single write lock, one long writer (a 26k-issue `bd import` holding the lock ~118s) failed 100% of concurrent operations with `SQLITE_BUSY` rc=1, each surfaced to gc as a permanent error even though a retry would have succeeded once the lock cleared. A BUSY write is the sqlite analog of a Dolt serialization failure: it lost the race without applying, so it is safe to retry. The red-team pass on the fix blocked the first push: the pre-push suite failed in the `computeWorkSet` tests with a nil-pointer panic inside `fmt.Fprintf`. Tracing it produced the two-part revision in this branch instead of a workaround: - `computeWorkSet` took `stderr io.Writer` and wrote probe-env diagnostics to it, but several callers (all reconciler unit tests, and any future fire-and-forget caller) pass nil — the probe-env error branch then panics instead of degrading to "skip this agent". The guard defaults nil to `io.Discard` at the function boundary. - The tests passed literal `/tmp` as `cityDir`, so they read whatever bead-store state the machine had at `/tmp/.beads` / `/tmp/.gc`. Hostile residue there (an authoritative-but-unresolvable scope config left by an unrelated process) made `controllerQueryRuntimeEnv("/tmp")` error for every agent, which is what reached the panic. Base-vs-branch was established explicitly: with the residue planted, these tests fail identically at the merge base — the hazard is latent at base and independent of the classifier change — but it deterministically blocked this push, so both halves are fixed here rather than papered over. Each test now uses `t.TempDir()`. ## Verification - `TestIsBdTransientWriteError` (new) pins the full needle set: sqlite BUSY/LOCKED markers retried; bare `database is locked` and Dolt's embedded lock-file phrasing NOT retried; sqlite constraint/syntax errors NOT retried; all pre-existing Dolt/MySQL needles unchanged. - `TestBdStoreDepAddRetriesSqliteBusyError` (new) proves end-to-end that a write failing once with `SQLITE_BUSY` is retried and succeeds on the second attempt. - New `computeWorkSet` regression test builds a real erroring probe-env fixture (authoritative scope config + postgres metadata with no resolvable password) and proves a nil-`stderr` caller skips the agent instead of panicking. - Red-team results: the nil-writer panic and the `/tmp` residue sensitivity reproduce identically at the merge base (pre-existing, now fixed); the full `TestComputeWorkSet` set passes with hostile `/tmp/.beads` residue deliberately planted; the conservative needle set was checked against Dolt embedded-mode lock errors to confirm no fail-fast regression. - Pre-push suite (`scripts/test-local-parallel fast`, 8 jobs: unit-core + 6 cmd/gc shards + darwin compile) passed in full on push. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Jul 14, 2026
… typed 400 on invalid (P1 #4 S1) (gastownhall#4157) ## What S1 of **audit P1 #4 — stable keyset cursors + one pagination vocabulary**: replaces the base64-offset pagination cursor on `GET /v0/beads` with a **versioned keyset token** and makes invalid cursors a **typed 400** instead of a silent page-1 restart. **Why**: offset cursors skip or duplicate rows whenever a concurrent write shifts the result set — a guarantee on a live work ledger — and any garbage cursor silently restarted the walk (duplicating rows). This is the audit's headline pagination correctness bug. ## The contract - `next_cursor` is now `v1:` + base64url(JSON) carrying the **(created_at, id) boundary** of the last row served. Opaque; clients change nothing (the envelope is unchanged). - Invalid cursor (garbage, a **legacy offset token**, wrong kind) → **400 problem+json `urn:gascity:error:invalid-cursor`** (new catalog code; route declares 400). Blast radius ≈ 0: the dashboard never sends cursors; the only in-repo pager (`gc events`, a different endpoint anyway) holds one in-memory for a 30s loop. - **Accept-what-you-mint**: a zero-CreatedAt boundary (degraded rows with NULL/unparseable `created_at` sort to the DESC tail) decodes fine — only the ID is required. Without this, a walk wedges in a 400 loop on a token the server itself issued (red-team major, pinned by an e2e walk over degraded rows). - Walk semantics: `Total` keeps full-set meaning and stays constant across a walk; a **degraded (partial) page always carries a resume cursor** — the client gets a boundary instead of a silently terminated walk, and following it ends cleanly on an empty page. ## Store layer `beads.ListQuery` grows `SeekAfter *SeekBoundary` — an exclusive compound boundary interpreted against the explicit `Sort`, applied in `Matches` (id tie-break mirrors `sortBeadsForQuery` exactly). **Boundary-before-limit on every backend** whose native query layer can't express the compound predicate (applying a native limit first silently drops page rows): | backend | mechanism | |---|---| | MemStore / CachingStore | Matches-before-limit by construction | | BdStore (both tiers) | `bdListRequiresClientLimit` + `canApplyWispsServerLimit` force unbounded fetch + Go-side filter/limit | | exec.Store | script-side `--limit` withheld when seeking | | doltlite (`gascity_native_beads`) | SQL bounded-top-N disqualified, per-table SQL LIMIT off, exact Go refilter before the limit cut; Count reports seek unsupported | Known trade-off: seeked doltlite pages are O(matching history) until the SQL boundary push-down lands (**ga-ylo6wr**; the whole-second sortKey truncation drift makes that a careful change). Handler: the bounded `all=true` path pushes the boundary into each store with `Limit=limit+1` (the extra row is the has-more signal); the full-scan path keeps its un-seeked fetch and suffix-slices after the boundary. ## Tests Token round-trip + rejection matrix; seek predicate both directions (ties, sub-second edges); **mid-walk no-skip/no-dup at three layers** (MemStore, CachingStore, API) with concurrent inserts between pages; tied-created_at walk (whole-second ties are the bd/doltlite norm); zero-CreatedAt tail walk; store-gate unit tests (bd, wisps, exec, tagged doltlite). The two pre-existing bounded tests updated to the new contract (limit+1 expectation; cursor-chain walks replacing offset jumps) — their original intents (O(limit) push-down, failed-rig Total exclusion) still guarded. Red-teamed pre-commit (5 lenses + adversarial verification, 18 agents, 12/13 findings upheld): the zero-CreatedAt wedge, two residual limit-push holes (bd wisps, exec), the degraded-walk termination, and the tie/coverage tests all came out of that review and are fixed here. ## Scope mail/convoys/sessions/events still speak offset cursors — they migrate in S2/S3 of this track (convoys also needs a deterministic order first; events unifies on seq DESC). S4 adds the spec-walking dialect guard + limit schema visibility. Owner-signed decisions: hard-400 old cursors, full S1–S4 program, grandfather legacy dialects. Gates: gofmt/vet clean; full `internal/beads` (default + native tag), `internal/api`, `exec`, genclient sync, dashboard-check all green. Tracking: `ga-q1rees`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Sep 25, 2026
…pond (gastownhall#6552) ## Summary `POST /v0/city/{cityName}/session/{id}/submit`, `/messages` and `/respond` did not accept an `Idempotency-Key`. A client whose request timed out could not safely retry, because a retry could deliver a second prompt or a second interaction response. These three were the last action endpoints listed as deferred (S4) in the audit P0 #4 idempotency guard. This PR finishes S4 for them. - `SessionSubmitInput`, `SessionMessageInput` and `SessionRespondInput` get an optional `Idempotency-Key` header. - Each handler now runs through `withIdempotency`. The scope is per target and per action (`/v0/session/<escaped id>/<action>`) because the session ID is in the URL, not the body. - **Same key, same body:** the original response is replayed. Submit and messages return the same 202 `{status, request_id, event_cursor}` and do not start a second delivery goroutine. Respond rebuilds the same body from the cached session ID and does not call `Respond` again. - **Same key, different body:** 422 `idempotency-mismatch`. For submit, `intent` is part of the body. - **Same key while the first request is still running:** 409 `idempotency-in-flight`. - **First attempt failed synchronously:** the key is released, so the retry runs. - **No key:** behavior is unchanged. - In `idempotency_guard_test.go`, `submit-session`, `send-session-message` and `respond-session` move from `pendingIdempotency` to `requireIdempotency`. `create-session` stays pending (out of scope). - The OpenAPI spec, Go client, TS client and embedded dashboard bundle are regenerated. Most dashboard asset changes are hash-only renames from the rebuild; the index chunk also grows by 99 bytes for the new zod header schemas. **Scope of the guarantee.** The idempotency cache is in memory, per city `Server`, with a 30-minute TTL and a 1024-entry cap shared by every idempotent endpoint. A key makes a retry safe within one server lifetime and for up to 30 minutes. Keys do not survive a city or supervisor restart (the per-city `Server` is rebuilt), so clients still need their own history-based recovery across restarts. For example, the "do not retry a prompt blindly" handling in the gascity-packs#455 bb plugin (thanks @csells) is still needed for that case. Submit and messages deliver asynchronously after the 202, so a failed async delivery stays replayed under the same key. To retry a failed delivery, send a new key. Stack: base `main`, no parent PR. A follow-up PR adding `client_request_id` correlation on session actions will build on this one. Related: - Follows gastownhall#4147 / gastownhall#4150 / gastownhall#4152 (audit P0 #4 idempotency S1 / S2 / S3). - CLI-side precedent: gastownhall#5831 (thanks @AlexBelous). - Part of the ACP support series (gastownhall#6541, gastownhall#6540, gastownhall#6542, gastownhall#6543, gastownhall#6544). This PR is independent of them. ## Testing - [x] Handler tests in `internal/api/idempotency_session_actions_test.go` (the original replay, mismatch, no-key and release tests were written first and seen failing; the scoping, intent and in-flight tests were added in review and checked by mutation, see below): - replay returns an identical body, and only one submit, message or `Respond` reaches the session fake - a different body returns 422, including a submit where only the intent changes - no key gives two deliveries - one key used on two sessions delivers to both, for submit, messages and respond (table-driven) - one key sent to `/submit` and then `/messages` on the same session delivers both - `sessionActionIdempotencyPath` escapes the target - in-flight: a same-key respond returns 409 `idempotency-in-flight` while the first request is blocked inside `Respond` (a channel-gated provider wrapper, no sleeps). After release, the replay returns the same 202 and there is exactly one `Respond` call. - a failed first attempt releases the key - [x] Mutation check (`go test -overlay`): dropping the target from the respond or messages scope, hashing only `Body.Message` on submit, sharing the submit scope with messages, and dropping `url.PathEscape` all fail the suite. - [x] `go test ./internal/api/...` (includes TestOpenAPISpecInSync, TestEveryKnownEventTypeHasRegisteredPayload, and the idempotency and write-auth route guards) - [x] Session action tests with `-race -count=3` - [x] `go vet ./...` clean - [x] `make spec-ci` (no drift), `npm run generate:client` (no drift), `make dashboard-check` (pre-commit hook) - [x] pre-push `make test-fast-parallel` on the current head - [ ] `make test-integration`: not run; no runtime, controller or workflow behavior changed ## Checklist - [x] Linked an issue, or explained why one is not needed (audit P0 #4 S4 follow-up; no separate issue) - [x] Added or updated tests for behavior changes - [x] Updated docs for user-facing changes (the OpenAPI reference schema is regenerated) - [x] Called out breaking changes or migration notes (none; the header is optional) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Oct 10, 2026
…les (item 3 W0) (gastownhall#7544) ## Summary This is efficiency item 3, slice W0, on the gascity side. **It changes no pool behaviour.** - **Pool workflows:** both (OSS and fork) pass the four `RBE_WARM_*` repository variables to the worker. They are unset, so no VM warms. - **Pin bump:** `gastownhall/rbe-worker` moves to `421eb59c`. - #3 adds `warm-cas` (dormant) and `rbe-first-exec:` measurement lines. - #4 removes the unused sticky-disk path. Enabling the warm set (W2: a fork-pool 50% A/B) comes later, once the core's warm-set builder (W1) has published and been verified. ## Test plan - [x] `go test ./scripts/... ./internal/testpolicy/... -count=1`, actionlint - [x] rbe-worker `product-check` at `421eb59c` against this tree: ok - [x] Opus review (two rounds): ship - [ ] After merge: pool runs log `rbe-worker: 421eb59c…` and `rbe-first-exec:`, and log no `rbe-warm` line (dormant) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
quad341
pushed a commit
that referenced
this pull request
Oct 12, 2026
…gastownhall#7592) ## Problem (staging v2 bug 1) At `stop`'s deadline the session effect executor cancels every effect context. If a launch's `leaf.Start` is still running when that happens, Start sees the cancel and tmux's `cleanupFailedStart` kills the runtime it just launched. After a supervisor restart (a shutdown that preserves sessions), the next controller then finds the row, can't adopt the runtime, and relaunches under a new token. A plain controller restart costs the start. ## Fix - **The cancel cause follows the shutdown mode.** At its deadline the executor cancels with `errEffectShutdown` only when the city's shutdown preserves sessions (the host reports `cr.preserveSessionsShutdown`, which the supervisor sets before canceling). Any other stop (`gc stop`) cancels with `errEffectStopped`, which ends a start as before: it cleans up what it launched, and nothing outlives the stop-all sweep. - **The start Call is detached before its lease watch.** `callWatched` runs a `callStart` Call on `outlivesShutdown(ctx)`, before `RuntimeLease.Watch`: - every cause relays, except a preserve-sessions shutdown that lands after the provider's Start began; - values pass through; - the effect's deadline is still reported and enforced; - the lease watch runs on the detached context, so a lost lease still ends a start that outlived the shutdown. - **A start never begins after the shutdown.** `startLaunch` takes `beginStart` right before `leaf.Start`. It refuses if the shutdown (or any end) already came, including during the pre-start steps (template resolution, `prepareRow`). Until then, the shutdown cause relays like any other. Every other effect still ends at the shutdown deadline, as before. ## Legacy This is a deliberate v2 difference, not parity. CONTRACT §12.2 #4: legacy rolls back a start in flight at shutdown, and v2 adopts it. Legacy's async starts run on the tick context, which `mc.cancel()` cancels at the start of shutdown, so tmux cleanup kills the runtime. v2 now adopts only in preserve mode, and ends the start in every other stop. ## Tests All new tests are in `cmd/gc/reconcile_effect_launch_test.go`, plus one in `city_runtime_v2_test.go`. They use `synctest` where they can. | Test | What it checks | |---|---| | `TestLaunchStartOutlivesTheShutdownCancel` | Through the real launch: a preserve-sessions shutdown during Start leaves its context alive; a lost lease or a provider swap ends it with their own causes. | | `TestOutlivesShutdownKeepsValuesAndOtherCauses` | Values pass through; a preserve shutdown after the Start began is ignored; a deadline or `errEffectStopped` relays. | | `TestNoStartBeginsAfterTheShutdown` | A shutdown before `beginStart` (before or after the detach) ends the start, and `beginStart` refuses. The pre-start steps see it at once. | | `TestStartOutlivesTheShutdownButNotALostLease` | Through `runTx` with a real lease: a started Call that saw a preserve shutdown still ends with `ErrRuntimeLeaseLost` when another holder takes the lease. | | `TestNonPreserveStopEndsALateStart` | The reviewer's `[late]` regression: a non-preserve stop, then the stop-all sweep, then pre_start finishing. Nothing runs after `gc stop`. | | `TestExecutorStopCancelsWithTheShutdownCause` | `errEffectShutdown` in preserve mode, `errEffectStopped` otherwise. | | `TestV2ExecutorSeesThePreserveSessionsShutdown` | The installed planner's executor reads the city's preserve flag. | Mutants, all killed: | Mutant | Killed by | |---|---| | Mode ignored (always the shutdown cause) | `TestNonPreserveStopEndsALateStart`, `TestExecutorStopCancelsWithTheShutdownCause` | | No `beginStart` gate | `TestLaunchStartOutlivesTheShutdownCancel` | | `beginStart` without the parent check | `TestNoStartBeginsAfterTheShutdown` | | Shutdown ignored before the Start began | `TestNoStartBeginsAfterTheShutdown` | | Detach after the lease watch (in `startLaunch`) | `TestStartOutlivesTheShutdownButNotALostLease` | | Deadline not reported | `TestTxCallRunsOnTheLeaseWatch` | | Preserve flag not wired to the host | `TestV2ExecutorSeesThePreserveSessionsShutdown` | | Relay no cause | `TestLaunchStartOutlivesTheShutdownCancel`, `TestOutlivesShutdownKeepsValuesAndOtherCauses` | | Values dropped | `TestOutlivesShutdownKeepsValuesAndOtherCauses` | **Residual (not blocking, CONTRACT S7):** a process that exits mid-Start skips the remaining launch steps (startup dialogs, session_setup, the startup nudge), and adopt only commits. SIGKILL does the same today; it belongs to C5a3's adoption side effects. Commands run: - `go vet ./cmd/gc` - `go test ./cmd/gc -run 'TestLaunch|TestOutlives|TestNoStartBegins|TestStartOutlives|TestNonPreserveStop|TestExecutor|TestTx|TestEffect|TestStart|TestPreWake|TestSim|TestRefusedStart|TestPlanner|Lease|TestV2|Shutdown|Swap'` - the pre-push gate 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
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
Tests