Wave 1 — stop the bleeding: three CRITICALs, the cost cap, and ADR-0073/0074 - #81
Merged
Conversation
…phase tasks done PR #80 landed the CI-truth batch; this finishes Wave 0's remaining two items and records the result where the phase documents can be read on their own. * `#162` — three skills embedded `/Users/dev/Documents/Projects/Agent-Organizer/`, a path from an unrelated project. Filed as documentation, but `add-package`'s `mkdir -p` would have created a directory tree OUTSIDE the repository, and the other two would simply fail. All three now anchor on `$(git rev-parse --show-toplevel)`, matching `write-adr` and `commit-and-pr`. * `#128`/`#129`/`#153`/`#163`/`#254` — `packages/mcp` is a shipped, tested package that appeared in none of the five inventory sites. Added to CLAUDE.md's package table, project-structure.md's table and diagram, overview.md's diagram and package list, and the reviewer agent — the last of which mattered most: its checklist never looked at the package, so an ADR-0052 boundary break had no reviewer signal. * `#164` — the reviewer agent's secrets item described only the desktop threat model (0% built) and said nothing about the surface that actually ships. Now leads with the CLI floor: stdin-not-argv, `0600` on `history.db`/`config.toml`, and redaction before anything reaches an approval preview, a run summary, or a `--json` payload. * `#167` — the security-review skill's SSRF step enumerated the ranges to block but never required REUSING the shared guard. A hand-rolled second range check is how the two drift and one silently stops covering a range (rule 3). Phase-document bookkeeping: 2.5.5.H is now 8/14 and 2.5.5.F 3/20, each task marked at its bullet so the phase file stands alone without cross-reading current.md. Wave 0 is closed; Wave 1 (the three CRITICALs and the cost-cap gaps) is next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Label the universal error-boundary and human-gate regressions with their binding G34 and G44 identifiers. Refs: G34, G44, #57 Co-Authored-By: Claude <noreply@anthropic.com>
Collapse untrusted error-message line breaks so a failure cannot forge a node-status row in persistent terminal scrollback. Refs: #57 Co-Authored-By: Claude <noreply@anthropic.com>
Apply the repository formatter to the gate and fatal-error terminal projection changes. Refs: G34, G44 Co-Authored-By: Claude <noreply@anthropic.com>
Apply the shared inline terminal projection to a forged timeout action and lock the display boundary with a runtime-regression test. Refs: G44, #57 Co-Authored-By: Claude <noreply@anthropic.com>
Handle G0 and #50 with a reversible SIGTSTP/SIGCONT lifecycle, register Home safety nets before onboarding, and pin the raw Ctrl-Z and terminal-state regressions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ady does (#91) `previewFor()` built the approval preview by echoing `target.path` / `target.command` verbatim, never calling the `redactSecretShapedText` scrub its sibling `sanitizeInput()` applies to `toolInput` moments later in the same dispatch. A model-placed credential in a `run_command` arg or a `write_file` path therefore rode the preview onto the approval prompt and the `agent:approval_requested` / `--json` observability stream unredacted — the ADR-0029(c) taint gate only covers KNOWN `secret`-typed args, so it does not cover this. All three preview fields now go through the same detector. `host` is scrubbed too: it is provably an author-allowlisted FQDN by then (`enforceHttpEgress` ran in the floor above), so that arm is defence in depth — it keeps every field on one rule so a future action class or a reordered floor cannot reopen the leak on one branch alone. Display-only in both directions: `enforcePolicy` has already resolved and checked the REAL target, so the scrub can never change which side effect runs. The one consumer that reads a preview field back is the CLI's auto-mode `isProtectedTarget`; the scrub is shape-targeted, so an ordinary protected path passes through byte-identical (pinned by a test), and the fs layer hard-denies protected paths regardless. Four regression tests; the three redaction assertions were verified RED against the unfixed `previewFor` before the change (break-verify), and the fourth — the no-over-redaction guard on `./config/.env` — passes on both sides, which is what makes it a guard. Refs: ADR-0029, ADR-0050, #91 Co-Authored-By: Claude <noreply@anthropic.com>
#99's originally-proposed fix — wrap the migration batch in BEGIN IMMEDIATE — is not implementable. drizzle's synchronous SQLite migrator runs the SELECT that DECIDES which migrations are pending outside its own transaction, and its raw `BEGIN` throws if we hoist the call into ours. So the read that decides and the write that applies cannot be made atomic by choosing a different BEGIN mode; two processes on a fresh history.db both decide the full set is pending regardless. Records the OS lock file (`<db-path>.migrate.lock`, `openSync(..., 'wx')`) with a run-then-reconcile fallback, and writes down why the two rejected options lost — including BEGIN IMMEDIATE explicitly, so it is not re-proposed. Filed as Proposed pending the maintainer's ruling; it flips to Accepted before the implementation lands. Refs: ADR-0073, ADR-0064, #99 Co-Authored-By: Claude <noreply@anthropic.com>
…0, #226) #100 — `RETRYABLE_CODES` listed only SQLITE_BUSY/SQLITE_LOCKED while this module's own header says it exists specifically to retry the SQLITE_BUSY_SNAPSHOT a stale DEFERRED writer hits. better-sqlite3 reports the EXTENDED result code, so that fault arrives as a distinct string, not as a SQLITE_BUSY prefix — the set never matched it. Matched exactly, so an unrelated future SQLITE_BUSY_* code is not swept in. #226 — the finding's premise does not hold as written: it says "the retry loop's callers are already async", but nine of the ten call sites sit inside SYNCHRONOUS store methods (recordSessionCost, metadata-store.upsert, model-catalog-store.upsert/clearUserPricing/ replaceProviderModels, provider-store.upsert, the three media-reference GC writes). Only persistEvent is async. So rather than an API-wide async conversion — five port interfaces and every CLI consumer, for no gain, since better-sqlite3 blocks synchronously inside each attempt regardless — this adds `withBusyRetryAsync` as a twin and uses it where the caller really is async. Both twins share the retryable-code set, the budget, the linear schedule and the fail-loud exhaustion via one `throwUnlessRetryable`, so the policy cannot drift between them. Scope stated honestly in the code and in the canonical doc: this removes the sub-300 ms term of the ~25 s worst case database-schema.md already documents, not the dominant `busy_timeout` one. The async twin's yield between attempts is observable — a sibling writer may commit during the backoff — so the "one self-contained, re-runnable transaction" requirement on `fn` becomes load-bearing; persistEvent satisfies it (one IMMEDIATE transaction, already rolled back before the sleep) and the engine awaits a branch's events sequentially. Nine tests: the SNAPSHOT regression was verified RED before the fix, the twin's schedule assertion is byte-identical to the sync twin's so they cannot drift, and the yield is proven with a macrotask armed inside attempt 1 — the thing `Atomics.wait` structurally cannot allow. Refs: ADR-0064, ADR-0040, ADR-0050, #100, #226 Co-Authored-By: Claude <noreply@anthropic.com>
…session (#228) CRITICAL. The chat persister writes `history.db` from inside a `RunEventBus` subscriber. `deliver()` isolates the throw, so the turn always survived — but with no `onListenerError` sink the bus re-threw it out-of-band, and Node's default for the resulting unhandled rejection is to KILL THE PROCESS. Asynchronously, after the user had already seen the reply. The turn they were billed for was lost and the session died with it. Three layers, none of which is sufficient alone: 1. `session-store.ts` — `createSession` / `updateSession` / `appendMessage` were the only session writers with nothing behind them but `busy_timeout`, which waits 5 s and then FAILS. All three now route through `withBusyRetry`, so the transient lock never reaches layer 2. Deliberately NOT wrapped in `BEGIN IMMEDIATE`: a lone INSERT/UPDATE already takes the write lock immediately, which is exactly why database-schema.md exempts single-statement writes — the retry is the orthogonal half, and the missing one. 2. `session-host.ts` — the bus's `onListenerError` sink existed and was simply never wired. It now always is, routing to a new optional `onListenerError` surface channel (same shape as `onBudgetWarning`/`onUnpriced`). With no surface sink it deliberately RETHROWS rather than degrading to a no-op: `RunEventBus.#reportListenerError` catches a throwing sink and routes it out-of-band, so the fallback is explicit rather than a silent catch. 3. `index.ts` — a process-level `unhandledRejection` net: report loudly, keep the process alive, and upgrade a clean `0` exit code to `1` so a lost write cannot be reported as success. Only a clean `0` is upgraded — a `chatEnded` 4 or `gatePaused` 3 still means what it says. Scoped to rejections; an `uncaughtException` is a different class and continuing past one would be unsafe. Seven tests. Break-verified in both halves: reverting the layer-2 wiring turns the sink test red AND produces five unhandled rejections (the crash chain itself), and stripping the three layer-1 wrappers turns exactly the three retry tests red while the fail-loud guard stays green. The first store test I wrote passed vacuously — it released the lock before the write — and was replaced with an injected fault that actually enters the retry. Still open and unchanged by this commit: per-turn write atomicity, which database-schema.md already names as a tracked follow-up. A turn is still three auto-committed statements, so a failure between them can leave a trailing unanswered `user` row — which `resumableMessageSequences` already rolls back on resume, so it degrades rather than corrupts. Refs: ADR-0036, ADR-0050, ADR-0064, #228 Co-Authored-By: Claude <noreply@anthropic.com>
The previous commit asserted that "an ordinary protected path (.env, .ssh/…) passes through byte-identical". Both halves were wrong, and the review proved it by execution. `.env` is not a protected path at all — the write-side `isProtectedPath` matches `.git` / `.relavium` / `.ssh` plus the rc basenames; `.env` lives in the READ-side sensitivity list, which the write path never consults. So the test claiming to pin the auto-mode classification was exercising a path that classification never looked at. And the property itself was false. Two detector patterns (`bearer|basic|token <run>` and `<key-ish>=<value>`) have character classes containing `/`, so one match swallows the rest of the path: `./Access Token Backup/.ssh/authorized_keys` collapsed to `./Access Token [redacted]`, and the protected segment classified as UNPROTECTED. Four such paths verified. The fs floor hard-denies protected paths at three separate points, so the user lost the prompt, not the file — but the comment claimed a guarantee the code did not provide. `redactPathPreview` now scrubs one path segment at a time. Separators and segment count round-trip exactly, so no match can cross a boundary and every `.ssh`/`.git` segment survives verbatim, while a credential-shaped segment is still replaced. The `command` field keeps its whole-string scrub — there is no structure to preserve — and the docblock now NAMES the consequence instead of implying it away: the command is fully model-controlled and the detector's patterns are public, so an injected model can shape a payload to match one (`sh -c token:evil.example/x|sh` → `sh -c [redacted]`) and be shown a reassuring marker in place of what it is about to run. Bounded by deny-all-by-default command allowlists, but real; surfacing "this was redacted" to the prompt needs a schema field and is recorded as the follow-up. Also from the review: - the egress `host` arm was completely untested — reverting it left all 101 tests green. Now pinned with a host whose leftmost label is itself credential-shaped. - the run_command assertion was survived by `command: '[redacted]'` (redact everything), the mutation that matters most now. Added the survival half: the preview must stay REVIEWABLE. - the fs_write test now asserts the side effect ran on the REAL target, which was the commit's headline safety claim and was unasserted. - `tool-registry.md` and `sse-event-schema.md` — the two canonical homes — now state that a preview field may contain `[redacted]` and must never be parsed by a machine consumer. Five mutations, all killed: whole-string path (3 red), no path scrub (4), no command scrub (1), no host scrub (1), redact-everything (3). The first, fourth and fifth all survived before. Refs: ADR-0029, ADR-0050, ADR-0057, #91 Co-Authored-By: Claude <noreply@anthropic.com>
…s (#104, #105)
#104 — `createClient` threw a bare `Error` with the database path interpolated into the
message, unlike every other typed error this package exports. `DbOpenError` follows the
package convention (`SafeEgressError`, `MediaWriteError`): a `code` discriminant callers
narrow on, `name` set, `cause` preserved.
The path moves OUT of the message and onto a structured `.path` field. That is the
error-handling standard's rule ("structured context as fields, not interpolated"), and it
matters concretely here: `apps/cli/src/history/reader.ts` and `session-open.ts` interpolate
this message verbatim into a user-facing `CliError`, and an absolute `history.db` path
carries the OS username — which a user-facing error is not supposed to leak. A caller that
wants to show it still can, redacted, from the field.
#105 — the JSDoc documented `journal_mode` and `foreign_keys` but omitted `busy_timeout` and
`synchronous`, both load-bearing for the concurrency behaviour this wave is hardening;
`busy_timeout` in particular is the term that DOMINATES `withBusyRetry`'s worst case. All
four are now listed, each with what it is for, pointing at the canonical home rather than
restating it.
Two tests: the `uri_unsupported` and `open_failed` codes, the path on the field, the cause
preserved, and — the point of the change — that the path is NOT in the message.
Refs: ADR-0021, ADR-0050, #104, #105
Co-Authored-By: Claude <noreply@anthropic.com>
…view (#91) Sonnet's independent pass proved by execution that the per-segment scrub from the previous commit REOPENS part of the leak. A credential whose value spans a separator escapes both segments, because the visible part falls under the pattern's own length floor: ./api_key=AAAAA/BBBBBB.txt whole-string: [redacted] per-segment: unchanged And whole-string cannot simply be restored — that is what swallowed `.ssh` and flipped the auto-mode protected-path classification, which is why per-segment existed. Both directions come from the same regex's `/`-bearing value class; no single pattern gives both properties. So the two USES are split instead of the trade being taken. `previewFor` goes back to a whole-string scrub, and `ToolApprovalRequest` gains `unredactedPreview` for classification. The CLI's auto-mode `isProtectedTarget` reads that, so a swallowed `.ssh` segment cannot change a security decision, and the display copy can be scrubbed as hard as secrecy needs. Two things make the unredacted copy safe to hand out at all: - It is the PREVIEW's field selection, not the raw `PolicyTarget`. An existing test caught the first attempt, which passed the whole target: `target.url` carries the query string, so `?token=abc` rode the request. Egress now carries the host only, like the display copy. `rawPreviewFor` owns the field selection and `previewFor` is its scrub, so the two copies cannot drift apart. - It never reaches the event. `agent:approval_requested` is built from `preview` alone, pinned by an assertion that the field is absent from the emitted body. Optional, not required, so a hand-built fixture stays constructible — every one of the ~15 sites TypeScript flagged is a test, several under `render/tui/` which this branch must not touch. The classifier prefers it and falls back to `preview`; the fallback can only ever UNDER-classify, never over-, because the scrub only loses information — and the fs floor still hard-denies the write. Refs: ADR-0029, ADR-0050, ADR-0057, #91 Co-Authored-By: Claude <noreply@anthropic.com>
Release and reclaim Clack-owned terminal state around onboarding stops, replay a stop issued during Ink reclaim, and disarm reversible handoffs before permanent terminal restoration. Refs: G0, #50 Co-Authored-By: Claude <noreply@anthropic.com>
The previous commit's stated residual was wrong, and Opus reproduced the real one against
the live persister and store: a failure between the `user` append and the `assistant` append
leaves an unanswered `user` row, and `resumableMessageSequences` rolls back only a TRAILING
one. Because the session now SURVIVES the failure and keeps chatting, that orphan gets buried
mid-transcript — `0:user 2:user 3:assistant`, with seq 1 never written. A resume then replays
two consecutive `user` messages, which a provider rejects. This commit created that residual;
it is not the pre-existing follow-up.
`SessionStore.writeTurn` appends the turn's messages and flushes the session row in one
`withBusyRetry(db.transaction(…, { behavior: 'immediate' }))`. All of it, or none of it. It
shares `mutableSessionColumns` with `updateSession` so a second write path cannot reintroduce
the ADR-0070 violation of SETting `total_cost_microcents` (pinned by a test).
The persister now STAGES a turn against provisional sequence numbers and adopts them only
after the write returns, so a failure advances neither `sequenceNumber`, `realMessageSeqs`,
the token totals, nor the derived title — no phantom sequence survives in-process either.
`record()` takes the staged values so the row written inside the transaction is exactly what
the process commits on success.
Also discharges the per-turn-atomicity follow-up `database-schema.md` has tracked since 2.5.I,
and cuts the worst-case contended block roughly threefold by collapsing three retryable
statements into one.
Six tests. My first attempt at break-verifying was itself flawed — mocking `writeTurn` proves
the persister's staging but never enters the transaction — so the store-level proof uses a
real UNIQUE-index violation instead: mutating `writeTurn` back to three auto-committed
statements leaves the first row behind and turns it red.
Refs: ADR-0062, ADR-0064, ADR-0070, #228
Co-Authored-By: Claude <noreply@anthropic.com>
Retry incomplete Clack cursor reclamation before a later stop request and preserve terminal safety across partial control-sequence writes. Refs: G0, #50 Co-Authored-By: Claude <noreply@anthropic.com>
Reserve each true provider attempt against the shared cap, reconcile it with realized cost, and release it on zero-cost failures. Re-arm warn-mode advisories after actual spend and report projected threshold percentages. Refs: G38, G39, G47, G49, 2.6.Q Co-Authored-By: Claude <noreply@anthropic.com>
Keep concurrent pre-egress reservations through provider ownership, conservatively retain uncertain charges, and preserve async media admissions across the in-process lifecycle. Refs: G38, G39, G47, G49, 2.6.Q
…ess net (#228 B1, H1-H3) B1 — layer 2 was inert in production. The sink was wired on the BUS but no call site supplied `onListenerError`, so it always took the rethrow branch, which is byte-for-byte identical to not wiring a sink at all. The only suppliers were my own tests. The binding acceptance clause requires the failure to surface THROUGH the sink rather than as an unhandled rejection; it was surfacing as one. Now wired at all seven sites: `chat`, `chat-resume`, the `/clear` and reseat rebuilds, `agent run`, and both Home paths. H3 — `BuildResumedChatSessionOptions` omitted the field entirely, so `chat-resume` could never be given a sink even after B1: `buildSessionRuntime` reads it through a `Pick`, and because it is optional the call compiled silently with `undefined` forever. That is the HIGHEST-contention surface for this bug (#227: two resumes on one session write the same rows). H2 — the process net wrote a rejection's message and stack to the terminal unsanitized. A rejection reason is arbitrary `unknown`: realistically an MCP server's error text, a provider body, or a tool result — precisely the untrusted sources the ANSI/OSC/Trojan-Source guard exists for, and the class the sibling boundary in `process/render-error.ts` already handles. Now `sanitizeInline` + `stripTerminalControls`, matching that boundary. H1 — the net had zero coverage: removing the handler entirely, and making the escalation overwrite ANY exit code, both left all 2248 tests green. Nothing imports the bin entry, and it cannot be imported (top-level `await run(...)`). Extracted to `process/background-failure.ts` with seven tests; both of those mutations now go red. Also from the review: the sink's wording is neutral rather than blaming `history.db` (M3) — the renderer, the Home store and the NDJSON printer all subscribe to the same bus, so a render fault must not be reported as a database problem; `describeReason` carries the error's `code` discriminant and handles the driver-shaped plain object and `AggregateError` instead of collapsing them to "unknown reason" (L5); and reports are bounded at five so a repeating fault cannot scroll an interactive session away (M6). Refs: ADR-0036, ADR-0050, ADR-0057, #228 Co-Authored-By: Claude <noreply@anthropic.com>
…he async twin Folding the #100/#226 Opus review. Both premises I relied on were checked independently: the "9 of 10 sync call sites" count was wrong (it is 12 of 13 — three were missed, which only strengthens the conclusion, so the decision stands but the docstring did not), and the "busy_timeout dominates" claim measured true for SQLITE_BUSY (~5.2 s) but FALSE for SQLITE_BUSY_SNAPSHOT, which returns in 0 ms. Both now stated precisely instead of broadly. M3 — `SQLITE_BUSY_RECOVERY` was missing, and the exact argument for adding SNAPSHOT applies to it verbatim: SQLite returns it while another process rebuilds the WAL index (the first open after a crash) and `busy_timeout`'s handler loop exits with it still set rather than downgrading. Verified producible in this build. Without it a first-run-after-crash write fails loud on a condition that clears in milliseconds. The deliberate EXCLUSIONS are now recorded too, so the set is auditable: shared-cache is compiled out and SETLK_TIMEOUT is undefined. M6 — `throwUnlessRetryable` moved the loop's termination condition outside the loop, where neither a reader nor TypeScript's control-flow analysis could see it. Breaking the predicate did not fail an assertion, it hung the test worker to an OOM — in production a wedged, CPU-burning process. Now `shouldRetry` returns a boolean with the bound back in the `for`, so the same mutation degrades to a fail-loud rethrow. M8 — the central behavioural change of #226 had no guard at all: reverting `persistEvent` to the sync twin left all 257 db tests green. Now pinned by a test that arms a macrotask before the backoff and asserts it runs between the attempts — something `Atomics.wait` structurally cannot allow. Verified red on that revert. M4 — the "demoted to a SAVEPOINT" justification for the SNAPSHOT entry was wrong and measured so: with the outer transaction still open a retry cannot refresh the read snapshot, so every attempt fails identically and the budget burns for nothing. That case is unretryable by construction; the docstring now says so and points at the real fix. Also: `resolveBudget` shared by both twins with a `baseDelayMs` floor (a stray 0 made the async twin drain only microtasks — never yielding, the exact opposite of its purpose); the root cause preserved on `persistEvent`'s wrap; an identity rather than message assertion where the claim is "the ORIGINAL error"; and a comment corrected that claimed spy leakage was prevented when a fresh client per test makes it impossible. Refs: ADR-0040, ADR-0050, ADR-0064, #100, #226 Co-Authored-By: Claude <noreply@anthropic.com>
… Proposed) All eight points folded. Three changed the mechanism rather than the prose: - **Takeover was hand-waved and would have recreated the race one level up.** "Unlink the stale lock, then create ours" lets two waiters both observe the same stale lock, both unlink, and both create. Takeover is now an atomic `rename` of a uniquely-named temp claim over the lock path, with the taker re-reading and proceeding only if the claim it reads back is its own. - **PID was load-bearing and should not be.** Pids recycle, so an is-it-alive check can be confidently wrong in either direction. Staleness is now decided by the recorded start time alone; the pid is explicitly diagnostic-only. - **`:memory:` would have been a real bug.** A lock file named `:memory:.migrate.lock` is nonsense and cross-process contention is impossible there by construction. The lock is taken only for a real filesystem path — which also keeps hundreds of test migrations off it. The constants are pinned in the ADR rather than deferred, with reasoning: 30 s stale threshold, 10 s max wait, 50 ms poll. They are this decision's whole tuning surface, so leaving them to the implementation would have left the decision half-recorded. `flock`/`LockFileEx` is now named as the considered-and-rejected alternative it deserved to be — mechanically superior, because the kernel releases on process death and the entire stale-lock protocol becomes unnecessary; rejected only because Node exposes no dependency-free advisory lock, which §9 gates. Recorded as the upgrade path rather than omitted. Two negatives sharpened: `finally` does NOT cover SIGKILL/OOM/power-loss, so a crashed holder WILL leave a lock file and the stale threshold is the only recovery — stated as a design assumption, not left implied. And Windows is named on its own terms instead of lumped in with "an exotic mount": `'wx'` is atomic on NTFS, but scanners and SMB redirectors make the fallback load-bearing there rather than decorative. Refs: ADR-0073, ADR-0064, #99 Co-Authored-By: Claude <noreply@anthropic.com>
…kill /models (G1) `new Date(x).toISOString()` throws `RangeError: Invalid time value` on NaN or any value outside ±8.64e15, and the conversion runs inside a `.map()` over EVERY catalog row — so a single corrupt `deprecation_date` aborted the entire `/models` projection rather than degrading that one row. SQLite is dynamically typed, so an `INTEGER` column really can hold these. `isoDateOrUndefined` applies the same guarded-parse discipline `statedLimit` already gives the limit columns a few lines away, at both conversion sites (the live-listing mapper and the merged-entry mapper — the finding named one; both had it). A deprecation date is decoration and the catalog is not, so the degradation is: drop the date, keep the row, keep the picker. Four cases pinned (NaN, both out-of-range directions, Infinity), each asserting the corrupt row is still PRESENT and that a healthy sibling keeps its date — so the guard cannot silently become a blanket drop. Verified red against the unguarded conversion. Refs: ADR-0064, ADR-0072, G1 Co-Authored-By: Claude <noreply@anthropic.com>
…0600 (#99, #28, #33) ADR-0073 accepted, so this lands it together with the 0600 self-heal the roadmap requires it to ship with. #99 — drizzle's migrator decides what to apply in a `SELECT` OUTSIDE its own transaction, so two processes on a fresh `history.db` both conclude the full set is pending, one commits, and the other dies on a duplicate `CREATE TABLE`. `BEGIN IMMEDIATE` cannot fix that (the ADR records why), so `withMigrationLock` serializes the batch with a `'wx'` lock file: staleness by recorded start time (never by pid — pids recycle), takeover by atomic `rename` (never unlink-then-create, which recreates the race one level up), and a run-then-reconcile fallback when the lock cannot be taken at all. A `:memory:` path skips it entirely, which also keeps hundreds of test migrations off the lock path. Writing the tests found two real bugs in my own first cut, both of which HUNG the suite: `readClaim` conflated "the file vanished" with "the file is garbage", so an unparseable lock spun the wait loop forever; and an unreadable claim was only stale by arithmetic against the clock, so under an injected clock it never expired. Unreadability is now a flag, decided independently of time, and a create-fails-but-reads-absent cycle is bounded rather than infinite (that combination means the filesystem is refusing the lock, and no retry count changes it). #28 — the ADR-0050 `0600` ran only AFTER the migrations, so a migration that threw left the file at the process umask (typically 644) for the lifetime of the install. `createClient` has already created the file by then, so an at-rest guarantee conditional on migration success is not a guarantee. Now applied on both sides of the batch, and idempotent. #33 — `config/write.ts` applies `0600` at write time only, so a layer predating that guard, or one an editor/`cp`/restore left permissive, stayed world-readable forever. `loadConfigFile` now re-asserts it on read, reusing the `stat` it already does and skipping the `chmod` when the mode is right. Deliberately never throws: unlike `history.db` a config file holds no secret VALUES, so this is defence in depth and must not turn a readable config into a startup failure. Also: a reviewer's probe file, left in the worktree, caught the suppression line in the process net promising "set RELAVIUM_DEBUG to see them" while the code ignored the flag. A diagnostic flag that does not reveal the diagnostics is worse than none, so the bound is now lifted under it, and `captureIo` takes an env so that is testable. Tests: 10 deterministic protocol cases (wait, no-steal-inside-threshold, nonce-verified stale takeover, garbage lock, reconcile, fail-loud-with-the-FIRST-error) plus the real two-process race the acceptance clause asks for — two `node` children against one fresh file, mirroring `concurrency.e2e.test.ts`'s dist gate and visibly skipping rather than silently passing without a build. #28 and #33 are both break-verified. Refs: ADR-0073, ADR-0050, ADR-0064, #99, #28, #33 Co-Authored-By: Claude <noreply@anthropic.com>
Measured rather than assumed: with the lock it passes every run; without it, it fails roughly one run in three, because the collision depends on how closely the two `spawn`s land. That is the same flakiness that let #99 ship, so the doc now says plainly that this is a coexistence SMOKE and that the deterministic branch guards are the injected-clock unit tests — the same division of labour `concurrency.e2e.test.ts` documents for `withBusyRetry`. I tried a READY-handshake barrier to make the collision deterministic, as that harness does. The child parked and printed READY, but the pair deadlocked in the parent and I did not find it within a reasonable budget; pausing the child's stdin (a resumed stdin keeps its event loop alive, so it would print OK and never exit) was necessary but not sufficient. Shipping a hanging test, or spending more of this change's budget on child-process plumbing, is worse than shipping the working form with its detection rate written down. Recorded as a follow-up. Refs: ADR-0073, #99 Co-Authored-By: Claude <noreply@anthropic.com>
Commit 02cb65f accidentally captured a transient review mutation (the `withBusyRetry` wrapper on `writeTurn` replaced with a bare IIFE) via `git commit -a` while a reviewer probe was in flight. This restores the file byte-for-byte to its pre-probe state; no intentional change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… BEGIN IMMEDIATE
Two real defects in my own #228 work, both found by reviewer probe files left in the worktree
and both independently reproduced before fixing.
**The token leak.** `totalInputTokens`/`totalOutputTokens` were assigned BEFORE `writeTurn`, so
a failed write left them advanced and the next turn's row claimed two turns' tokens for one
visible exchange — while that turn's MESSAGES were correctly absent. That contradicts the rule
stated two comments above it (a turn whose messages do not persist accumulates no tokens), and
it defeated the point of staging. Every in-memory mutation for a turn now happens after the
durable write returns, with a comment saying why the ordering is load-bearing rather than
incidental.
Pinned without magic numbers: a control run of one clean turn establishes what a turn is worth,
and failed-then-clean must match it exactly. Verified red by moving the assignment back.
**The unpinned lock mode.** A mutation that dropped `{ behavior: 'immediate' }` from `writeTurn`
left the entire suite green — atomicity was pinned, the LOCK MODE was not. `BEGIN IMMEDIATE` is
what closes the read→write upgrade race (ADR-0064's 2.5.I convention); DEFERRED would still be
atomic and still be wrong. Now spied, the same way `loadFull` already guards its read
transaction, with the describe restoring its own mocks so the spy cannot leak.
Also restores the `else` branch a leftover mutation had dropped: an errored or aborted turn
persists no messages and accumulates no tokens, but its row is still flushed — `updatedAt` moves
and the session cost, already folded per `cost:updated`, is real.
Refs: ADR-0062, ADR-0064, ADR-0070, #228
Co-Authored-By: Claude <noreply@anthropic.com>
A blocker I shipped and then failed to notice twice. `persister.test.ts` and `background-failure.test.ts` provoke throws that travel out of a `RunEventBus` subscriber. With no `onListenerError` wired the bus deliberately re-throws out-of-band — by design — which vitest counts as a file-level error. Both files reported every test PASSED and exited 1, so `turbo run test` and therefore `pnpm run ci` were red against the phase's own Definition of Done. I missed it because I was grepping the reporter's "Tests N passed" line instead of checking the exit status, and read turbo's "12 successful, 14 total" as caching rather than as two failed tasks. That summary-says-passed/exit-nonzero shape is exactly why it reads as a flake. The persister fix is not mere suppression: the sink is threaded through both test helpers and the atomicity test now ASSERTS the failure was reported, which pins the #228 fail-loud path the atomicity work depends on. The dispose test — the one case that legitimately has no net installed — gets a scoped absorber instead. Also closes a surviving mutation from the same review: moving `realMessageSeqs.push` before `writeTurn` left the whole suite green, and that array is the ADR-0062 boundary seed, so a phantom entry silently shifts every later /compact and /trim boundary — the "step-3 data-loss trap" the file's own comment warns about. The phantom-seq test now drives a real compaction, which is the only way anything observes that array; the mutation now fails three tests. Refs: ADR-0062, #228 Co-Authored-By: Claude <noreply@anthropic.com>
`node:failed`, `run:failed` and `run:cancelled` all carry a run-wide cumulative cost snapshot, and three consumers ignored them (#W15-6). `cost:updated` is streamed and never persisted, so those snapshots are the ONLY durable carriers of money spent after the last `node:completed`. Store — `node:failed` now gets the same telescoping delta fold as `node:completed` (it has a real nodeId, so the attribution is real), and both run terminals fold their residual. The root-cause node's `node:failed` snapshots the cumulative as of THAT node, but a sibling's paid media job abandoned by the failure is folded only just BEFORE the terminal (ADR-0045 §5) — after that `node:failed` was already emitted — so the terminal is the only carrier of that last addend. The residual is written as a `run_costs` row with an EMPTY `node_id`, not just a bump to `runs.total_cost_microcents`, so ADR-0070's `SUM(run_costs) == runs.total_cost_microcents` stays exactly true. The money is real but the event does not say which node it belongs to, and inventing an attribution would be worse than admitting there is none; every event-carried `nodeId` is `nonEmptyString`, so an empty one is PROVABLY not an authored node — unlike a plausible-looking sentinel. A zero delta writes nothing, so the common no-spend failure does not litter the table. The store's `run:failed` comment claimed these events "carry no total-cost field in the run-event schema". They do; that note is gone. Checkpoint — `node:failed` now restores the cumulative like `node:completed`. A node that failed can still have spent, and a resume that skipped it restored a total that had forgotten real spend, handing the cap headroom it does not have. Run TUI — `run:failed` was the one terminal arm not folding its own snapshot (`node:failed` and `run:cancelled` already did), so the summary showed the pre-cleanup figure. Break-verified at all three layers with each mutation confirmed applied. Refs: #W15-6, ADR-0045 §5, ADR-0070 Co-Authored-By: Claude <noreply@anthropic.com>
#W15-17 — that `restoreConservativeCost` is actually wired through
`buildSessionRuntime` → `GovernorWiring` → `AgentSession.resume`. The
note in this file recorded a first attempt that was vacuous; two more
were needed before the test earned its keep, and both failures were
found by verifying the mutation rather than trusting a green run:
1. cloning the realized test left `totalCostMicrocents` high, so the
cap tripped on the realized seeding either way;
2. with a 1µ¢ cap the next-worst-case ESTIMATE alone exceeds it, so
the turn tripped whatever the seeding did.
The isolating shape is a cap large enough that the estimate alone
leaves headroom, ZERO realized cost, and a conservative total that
consumes all but 1µ¢ of it. Deleting the wiring spread now reddens it.
#W15-21 — `agent-run.ts`'s no-op conservative writer. `agent run` opens
no session store, so a commitment has nowhere durable to go; without
the explicit no-op every commitment REJECTS, the governor marks the
session durability-broken, and §2's barrier fails a turn for a session
nobody was ever going to resume. Deleting that one line silently broke
every capped `agent run` and left the whole suite green.
Same trap here: a plain `textTurn` carries usage, so no commitment is
made and the test is vacuous. A stream that ends WITHOUT a stop chunk
carries none — the "may already have billed, returned nothing
accountable" case the mechanism exists for — and that version dies when
the line is removed.
Refs: #W15-17, #W15-21, ADR-0074 §4
Co-Authored-By: Claude <noreply@anthropic.com>
`releaseConservativeCommitments` and `conservativeState` existed, were tested, and no command, REPL action or Home affordance called either (#W15-3). §1 requires the surface that RENDERS the committed amount to also expose clearing it — and this PR removed the accidental escape a restart used to provide, so shipping without it was an indefinite block §1 explicitly rejects. `/cost --release` is that surface. It clears the DURABLE columns first, then the live governor. That order matters: §4 seeds a resumed governor from `agent_sessions.total_conservative_microcents`, so clearing only the in-memory total let a released commitment come straight back on the next `chat-resume` — the "deliberate user decision" surviving exactly as long as the process did. `releaseSessionConservativeCommitments` zeroes the per-model `session_costs` holds AND the session aggregate in one `BEGIN IMMEDIATE`, and reads the released amount INSIDE the transaction so the number reported is the number actually cleared. Zeroing one without the other would leave `/cost` printing holds that no longer sum to the session total. Realized spend is untouched: a release clears an estimate, never an invoice. `/cost` also renders the durability state, which nothing did. The wording says only what is true — the cap still holds in this session, what is broken is that a resume would not see it — and names the escape hatch where the amount is rendered, as §1 asks. On the reserved `budget:estimate_released`: it stays reserved and unemitted, deliberately. The session path restores its conservative total from COLUMNS, not by replaying events, so zeroing them IS what makes the release survive a resume there. The event is needed only by a workflow-run release surface, which does not exist yet — saying so beats emitting an event nothing reads. The resume guarantee is proven in two mutation-verified halves rather than one hollow end-to-end: the store test pins that the columns go to zero, and #W15-17's test pins that those columns are what seed the resumed cap. ADR-0028's amendment note said ADR-0074 §2–§5 had not landed. Appended (never rewritten — ADRs are append-only) with what is true now, and with the two gaps that remain open under their own ADRs. Break-verified with each mutation confirmed applied: dropping the per-model zeroing, and dropping the `--release` flag binding. Refs: #W15-3, ADR-0074 §1/§4 Co-Authored-By: Claude <noreply@anthropic.com>
`RunEventBus` isolates listener errors by design, so a persister that could not write still let the turn report success (#W15-4). Two things followed from that, and both were silent: - the running cost total advanced BEFORE `recordSessionCost`, so a failed write left this process believing spend it had not recorded — and `mutableSessionColumns` makes that write the SINGLE writer of the durable total, so no later flush could repair it. A resume then restored a cap that had forgotten real money; - a failed `writeTurn` left the staged turn in place only by accident (the throw skipped the reset), and the next `beginUserTurn` overwrote it — the durable transcript losing an exchange the in-memory session still showed. The persister now LATCHES its first durable-write failure and re-throws it. Re-throwing is load-bearing twice: it skips every in-memory mutation the arm had queued after the write, and it keeps #228's other half — the bus routes the throw to the listener-error sink, so the user is still told. Swallowing it would have latched the state and silently removed the notice; that regression showed up on the first attempt and is why it is written this way. The host gates pre-egress on that latch, cap or no cap, and it runs before the budget governor: a session whose record is already lost must not spend more. `beginUserTurn` refuses to overwrite a staged turn once the latch is set, as the backstop behind the gate. This CHANGES a previously-tested decision. #228 pinned that the session survives a failed write and the next turn is clean; the session now stops instead. #228's guarantees — atomicity, no phantom sequence, and the user being told — all still hold and are still tested; what is no longer true is that it silently carries on. The phantom-sequence test was rewritten because the compaction it observed the leak through is unreachable on a stopped session, and it says so rather than quietly asserting something weaker. Break-verified with the mutation confirmed applied — and the first version of the gate test was vacuous (the second turn wrote nothing either way), so it now asserts the turn terminal the gate uniquely produces. Refs: #W15-4 Co-Authored-By: Claude <noreply@anthropic.com>
Each ✅ item is fixed, break-verified with the mutation confirmed applied, and committed. The status block says plainly what remains and why, rather than letting the marked list imply the register is done: - `#W15-1` / `#W15-2` are §A, unchanged — each needs an append-only ADR before any code and earns its own PR; - four of §E's six coverage gaps need engine- or e2e-level scaffolding that does not exist yet, and a hollow test in place of the real one is the mistake this register already caught three times. Also surfaces two decisions that would otherwise only live in a commit message: `#W15-4` changed a behaviour #228 had pinned, and `#W15-23` is scoped to one store while the same convention remains open in the rest of `packages/db`. Refs: PR #81 Co-Authored-By: Claude <noreply@anthropic.com>
The last of §E's gaps in this package (#W15-18). The earlier note said the test needed a `model_catalog` FK row this block did not seed — the fixture was `seedModelCatalog`, in this same file; the block simply never called it. A model can be CATALOGUED mid-session (2.6.Q), so the upsert has to backfill a NULL id once one resolves, while never letting a later unresolved lookup wipe an id that IS known: "we don't know" is not "there is none". Three cases, and each kills a different mutation: - backfill — dropping the whole conditional reddens it; - never wipe with NULL — the guard that omits the column from the SET; - never REPLACE a known id, even with another resolved one — replacing `coalesce` with a plain assignment reddens it. The third is asserted as the behaviour it IS rather than an intent the code states (the docblock only promises "never overwrite with NULL"), so a future change to `coalesce` has to be deliberate. A second catalog row makes that case real: `model_catalog` is refreshed from models.dev, so the same model string can resolve to a new UUID. Break-verified with each mutation confirmed applied. Refs: #W15-18, ADR-0074 §4 Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0074 §2's turn-boundary half (#W15-19). Before it, a chat reported a turn complete — `sendMessage` returned, the process could exit — while the commitment for a possibly-billed call had not reached the database. The production change is one awaited call; what was unproven is the ORDERING it buys. The earlier attempt counted microtask ticks before asserting, which is fragile and vacuous in the failure direction: drain too few and the turn has not started, so "no terminal yet" passes for the wrong reason. The barrier itself is the signal instead — the hook records that it was called, and the assertion runs only once the turn is provably parked on it. That holds whether or not the `await` is there, so the assertion is what decides, not the loop. Break-verified with the mutation confirmed applied: `await` → `void` reddens it. Refs: #W15-19, ADR-0074 §2 Co-Authored-By: Claude <noreply@anthropic.com>
Reverting the omission left the whole suite green (#W15-20). `0` means "the gate RAN and reserved nothing" — an unpriced model's allow-degrade path. Under H3's approved bypass NO hook runs at all (`#runAttempt` passes `preEgress: undefined`), so there is no priced basis to freeze and emitting `0` would claim one. On resume the frozen branch would then call `reserveAcceptedCost(model, 0)`, reserve NOTHING, and skip `registerLegacyMediaJob` — so a job deliberately submitted OVER the cap comes back holding no reservation and no hold, letting a sibling spend headroom that is still owed. The scaffolding turned out far smaller than planned: the assertion depends only on the `budgetApproved` flag, so no budget block, no governor and no LLM are needed. A stub handler returns the budget gate in the shape the governor's `BudgetPauseError` is converted into (the governor raising it is already pinned in `budget-governor.test.ts`), and the approved re-dispatch returns a `media_job` outcome — which the `NodeOutcome` union supports directly. Two rows, because one would not have been enough: the omission, and the contrast where the hook DID run and the field is `0`. Without the contrast the first assertion would also pass on a build that never emitted the field at all. `mediaJobOutcome` uses a FRACTIONAL `units` on purpose — `duration_seconds` is fractional by contract, and an integer bound there once made a 12.5-second job unwritable after the provider had accepted it. Break-verified with the mutation confirmed applied. Refs: #W15-20, ADR-0074 §3 Co-Authored-By: Claude <noreply@anthropic.com>
The last of §E (#W15-16). Every part was unit-correct and nothing exercised them together: `budget-governor.test.ts` proves the governor releases its holds, and nothing proved the engine ever calls that on abort. Deleting the listener compiled and left the whole suite green. A budgeted resume with a LEGACY parked media job (no `units`, no `acceptedCostMicrocents` — which is what makes the resume register the hold instead of restoring a frozen reservation) plus a concurrent sibling that suspends in `checkPreEgress`, cancelled mid-hold. The failure mode is the point: without the listener the test TIMES OUT rather than failing an assertion. A `checkPreEgress` suspended in the hold keeps its node counted as `running`, so `#step` never reaches `#countRunning() === 0` and the run emits no terminal — the unkillable run, reproduced exactly. Break-verified that way. Two things the test guards against being vacuous: it asserts the sibling is provably inside the hook AND that no terminal has been emitted before the cancel, so what follows tests the release rather than a run that had already finished. `seedStarted` gained an optional workflow id — a `resumeFromCheckpoint` test must pass the id `resolveWorkflowId` mints for its slug or the resume is refused with `workflow_mismatch`. The default keeps the reconcile callers unchanged. The in-source note claiming this was uncovered is replaced with what now covers it. Refs: #W15-16, ADR-0074 §3 Co-Authored-By: Claude <noreply@anthropic.com>
All six of §E's coverage gaps are now closed, so everything outside §A is done. `#W15-16`'s composition test fails by TIMING OUT when the abort listener is removed — the unkillable run reproduced exactly rather than an assertion standing in for it. Records the maintainer decisions taken for §A on 2026-08-09 (fail closed on any skipped row on the resume path; the ledger as a new durable run event folded into a `run_costs` row) and the ordering they imply: `#W15-2` first, because `#W15-1`'s new event type falls straight into §5's skip path on an older binary. Refs: PR #81 Co-Authored-By: Claude <noreply@anthropic.com>
Seven findings, all valid. Two mattered. **A shipped defect the review surfaced.** `onLegacyMediaJobHold` is declared on `WorkflowEngineDeps`, built into a real user-facing sentence by `gate.ts`, forwarded by `build-engine.ts` — and the `WorkflowEngine` constructor never read it. So the sink was dead: on a `relavium gate` resume of a LEGACY parked media job the hold engaged and the user got a silent stall, which is exactly what ADR-0074 §3's observability clause exists to prevent. This is the THIRD occurrence of the bug class the comment above `#resolveEndpoint`/`#onUnpriced` already records; wired the same way, at the same three sites. **My #W15-16 guard did not hold.** It flagged handler ENTRY, and `expect(terminalsIn(events)).toHaveLength(0)` is satisfied by `run:paused` (not a terminal). Deleting `registerLegacyMediaJob` — the one call that CREATES the hold — left it green, and the whole core suite green with it: the test was quietly asserting "a media-parked resume can be cancelled" while its name claimed otherwise. Rebuilt on two real edges: the governor's own hold notice (now that it is wired) as the signal, and a `released` flag proving the sibling is PARKED before the cancel and through after it. Three mutations now redden it — the hold never registered (12 ms), the abort listener gone (5 s timeout), and the notice sink dead again. Also folded: - #W15-20's `MEDIA_GATED` gained a real `budget:` block. Without a governor `ctx.preEgress` was `undefined` on EVERY dispatch, so the bypass assertion was vacuous — removing the `budgetApproved ? undefined :` guard left it green. It now asserts the contrast (`[true, false]`) and reddens under that mutation. - the contrast row's title and comment claimed a gate ran when no governor existed at all. It pins PRESENCE, not the amount — a `StubExecutor` cannot attach an admission, so the value is necessarily the `?? 0` fallback. Said outright, with a pointer to the fixture that could assert the amount. - the in-handler `expect` moved out: `#runAttempt`'s catch-all turns a handler throw into a generic `internal` failure, so a failing assertion surfaced as a confusing `run:failed` rather than itself. - #W15-19's assertion wrapped in `try/finally` so a failure cannot strand `sent` pending for the life of the worker. - the roadmap's §E preamble, which still spoke of the gaps in the present tense. Refs: #W15-16, #W15-19, #W15-20, ADR-0074 §3 Co-Authored-By: Claude <noreply@anthropic.com>
…live
Sonnet round on the same diff. Four findings, all downstream of the
sink `b0af9f3` brought to life rather than in the wiring itself — which
is the right place for a second pass to land.
Reading the newly-live sentence turned up one the round did not name:
`nodeIds.join(', ')` skipped BOTH rules this repo applies everywhere
else a node id reaches a terminal. `renderer.ts` runs `sanitizeInline`
over one — a node id is authored, not model-controlled, but a workflow
YAML can arrive from anywhere — and `logs.ts` bounds its id list
because the COUNT is the signal and the first few ids are the lead.
This line had neither, and an embedded newline in a node id would have
forged a whole extra stderr row. It was the one such line in the CLI
that never got the treatment, for the simple reason that it had never
rendered.
The sentence is now `legacyMediaJobHoldNotice`, exported and pinned:
number agreement for one and for several, sanitization, the bound, and
exactly one trailing newline. Break-verified — dropping the
sanitize+bound reddens two of the five. `gateCommand` still writes it
to stderr, so `--json` stays a pure event stream.
Also folded:
- the `onLegacyMediaJobHold` docblock claimed `run` routes the notice.
Only `gate` wires it AND only `gate` can reach it: the hold is
registered from `#restoreParkedMediaJob`, reachable only through
`resumeFromCheckpoint`, and a fresh `relavium run` has no checkpoint
to restore a legacy job from. Narrowed, with the reason.
- `engineWith`'s third parameter is now
`Partial<Omit<WorkflowEngineDeps, 'host' | 'executor'>>` — the spread
runs after those two, so leaving them assignable would let a future
caller override the second argument with no compiler error.
- the notice is NOT deduped, unlike `onUnpriced`, and the asymmetry is
now stated where it lives: unpriced is a standing condition of a
model, a hold is an event against one attempted egress. Dropping the
second one would leave a sibling stalled with no explanation, which
is what §3 exists to prevent. Behaviour unchanged; the intent is no
longer inferable only from a one-clause comment.
Refs: #W15-16, ADR-0074 §3
Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0075, amending ADR-0074 §5 (#W15-2). §5's tolerant read — drop an unknown event `type`, still fail loud on a known type with a bad body — was right, and it fixed a real doc↔code contradiction. One read is not like the others: `checkpointer.ts` builds resumable state from what the read returns, and a resume does not display a log, it decides what work still has to happen. An older binary that drops a row cannot know whether it was a node terminal, a job submission, a gate decision or a cost commitment — so it may re-run completed work, re-submit an already-billed media job, or re-ask an answered gate, silently. The mitigation that shipped with §5 was narrower than it looked. Folding the skipped rows' `seq` back in prevents a `UNIQUE(run_id, seq)` collision; it does not restore lost state. That repair is now unreachable and removed, because refusing covers both halves. `loadRunEventLogForReplay` is the strict entry point; every display read stays tolerant. `UnreadableRunEventLogError` is deliberately distinct from `CorruptRunEventError` — the data is fine and this binary is too old, so the remedy is real and the user can perform it, and the message says so plus what is still readable. The ADR answers §5's three stated reasons for rejecting this rather than reversing it silently. The first has become false and that is the fact that unlocked the decision: it needs no durable version marker, because `skipped.length > 0` is derived at read time. `checkpointer.test.ts`'s counterpart test asserted the OPPOSITE and is rewritten, keeping the reasoning for the behaviour it replaces so the history is not lost. `load` is now `async`, so the strict read's synchronous throw reaches the port as a rejection — the port types it as a Promise, and that is the same guarantee `persistEvent` leans on. Folds an external ADR review: - ADR-0074's §1 note still said the release half did not exist; it shipped, and the note now says so and what stayed reserved. - §5's heading carries `(amended by ADR-0075)`, so a reader meets the narrowing before the original text. - ADR-0075 gained a section on why the SESSION path needs no counterpart — no session resume reads a stored event log at all, its durable state is typed rows — because "it was forgotten" is the reading that section exists to rule out. Break-verified: removing the refusal reddens the checkpointer test. Refs: #W15-2, ADR-0075, ADR-0074 §5 Co-Authored-By: Claude <noreply@anthropic.com>
Opus round on `f1b7773`. Nine findings; one was a blocker and it
invalidated four sentences I had written.
**The refusal never reached the user.** `relavium gate` is the only
consumer of the durable checkpointer, and its `catch` — which pre-dates
this work — folded ANY throw into a generic
`CliError('invalid_invocation')`. So the typed error lost its count,
its `seq` values, the upgrade remedy and the "your history is still
readable" half one frame above the `toUserFacing` mapper written for
it; the user saw "the persisted event log could not be read", which is
the corruption sentence, and got exit 2 — semantically wrong, since the
invocation was valid. `errors.ts`'s new branch was unreachable code.
That made four ADR/commit sentences overclaim, which is the exact
defect class this workstream exists to correct. The typed error now
passes through untouched, and a `gate.test.ts` case pins the rendered
outcome end to end — it reddens when the re-wrap comes back.
**Six mutations survived the rewritten checkpointer test**, and
`packages/db` had no test at all for its own new public method, against
this repo's engine-first testing order. `loadRunEventLogForReplay` now
has its own block covering what the CLI layer cannot: a MID-LOG skip
(the case that distinguishes ADR-0075's "any row was skipped" from the
strictly narrower tail-only rule the removed high-water repair needed),
the >8 elision, the run id in the message, `CorruptRunEventError`
keeping precedence when a damaged row precedes a skipped one, and
`streamingIncluded: true` being load-bearing — a replay read that
inherited the display read's firehose exclusion would fold a log
missing rows it never even counted as skipped.
Also folded:
- `pending.ts` and `gate-list.ts` still claimed the discovery surfaces
and the resume path "can never disagree". As of ADR-0075 they can, by
design: same fold, deliberately different read. Corrected in both
homes rather than left as a true-sounding sentence.
- `run-history-store.test.ts`'s rationale still described resume seeding
from a fold that can contain skipped rows, and pointed at a test that
was rewritten to assert the opposite. Kept the assertion, rewrote the
reasoning — the standard applied to `checkpointer.test.ts` and missed
here.
- `docs/roadmap/current.md` asserted the ADR-0074 §1 release was still
unreachable while the ADR asserted it shipped, and listed
`budget:estimate_released` as a closing criterion that deliberately
did not ship. One canonical home, so the roadmap moves with it.
`#W15-2` marked closed: 23 of 24.
- `return await Promise.resolve(...)` reduced to the plain return — the
rejection guarantee comes from `async`, not the wrapper.
- "re-exposes the first three" was off by two.
Refs: #W15-2, #W15-3, ADR-0075, ADR-0074 §1/§5
Co-Authored-By: Claude <noreply@anthropic.com>
Four findings, all valid, plus one observation worth acting on. **The exit-code asymmetry.** The Opus fold's own reasoning — "exit 2 is wrong when the invocation was valid" — applies just as well to `CorruptRunEventError`, which `gate.ts` was still re-wrapping. So `relavium logs` on a corrupt log exited 1 while `relavium gate` on the IDENTICAL log exited 2, for the same error type. It now passes through typed too: `toUserFacing`'s corrupt branch already composes the better sentence (the run, the `seq`, the `event_type`, and what is still listable), so re-wrapping made `gate` both less diagnosable and inconsistent with every other single-run surface. Its case moves out of the exit-2 table into its own, which is where the behaviour change is visible rather than buried in a fixture list. **Two message assertions were substring-only and missed a mutation each.** Hardcoding the plural suffix reads "1 events" and stayed green because only the 12-skip case asserted wording; and four independent `toContain`s pass on any shuffle of the same words, so the reading order — what is wrong, then the remedy, then what is NOT lost — was unpinned. Both now assert what they claim. **The `streamingIncluded: true` test proved less than its comment.** It showed the two DB reads return different lists; it did not show the CONSEQUENCE. `reconstructCheckpointState` takes `Math.max` over every event it is handed, so a replay read that inherited the display read's `agent:*` exclusion would reconstruct a mark BELOW the true stored max whenever the tail is a token row — reintroducing the `UNIQUE(run_id, seq)` collision the removed high-water repair guarded, and NOT through the skip/refuse path, since an excluded row is a SQL `WHERE` filter and is never recorded as skipped. A checkpointer case now closes that loop and reddens when the flag is flipped. **One gap deliberately left open, and named.** The refusal is pinned at `gate.ts`'s own catch — where the blocker was — and at `toUserFacing`, but not through `dispatch.ts` / `specs.ts` to the process exit. A re-wrapping `try/catch` in either layer reproduces the same blocker and leaves both tests green. Closing it needs a `run(argv, io)` drive with a redirected HOME and a seeded on-disk `history.db`; no such harness exists for `gate`, and a partial one would read as coverage it is not. Marked at the test it concerns and in the roadmap. Refs: #W15-2, ADR-0075 Co-Authored-By: Claude <noreply@anthropic.com>
…edger ADR-0074 made the CONSERVATIVE commitment durable and deliberately left the other half. A call that returned trustworthy usage has no equivalent barrier: `cost:updated` is streamed and never persisted, and the durable record is reconstructed from a LATER `node:completed` snapshot. Between a paid call and that terminal, the charge exists only in memory. That window is where the engine does its riskiest work. A paid call succeeds, the model asks for a tool, the process dies during the tool — and the resumed node does not merely forget the money, it SPENDS IT AGAIN, because the cap it re-evaluates against is understated by exactly the amount already charged. An agent turn is a loop, so a node can lose many calls this way. The decision: an additive durable run event recording one settled attempt, emitted through the same barrier ADR-0074 §2 uses, folded into a `run_costs` row at persist time. Three properties make it a ledger rather than another observation — written before the next thing that can spend or mutate; idempotent by `UNIQUE(run_id, seq)` rather than by a new key, because a second key is a second thing that can disagree; and non-double-counting by arithmetic, since the node terminal's delta is `max(0, cumulative − sum so far)` and therefore zero once the attempt rows have advanced that sum. Four alternatives are weighed inline, including the obvious one — making `cost:updated` itself durable, which needs no new discriminant. It loses because it silently changes what every existing reader sees for an event documented as streamed-only, and because `cost:updated` is dual-envelope while `persistEvent` refuses session events: a durable-on-one-envelope-only event is a contract that cannot be stated in one sentence. ADR-0070 gains an amendment note — `run_costs` gains a second writer, and its SUM invariant is unchanged and continues to hold by arithmetic. Implementation is NOT in this commit. The ADR lands first because it is self-contained and because ADR-0075 had to precede it: a new durable event type is exactly the input that decision governs, and without it an older binary would drop the row and resume against a cap missing the charges this ADR exists to record. Refs: #W15-1, ADR-0076, ADR-0074, ADR-0075, ADR-0070 Co-Authored-By: Claude <noreply@anthropic.com>
The decision for `#W15-1` is made and committed; the code is not. This says so plainly and names the five steps in dependency order, so the next session starts from a stated position rather than re-deriving it. Also names the break-verify that matters most — step 4's awaited emit. Without it the event still records the charge, but not the guarantee that makes it a ledger rather than another observation, and a test that does not redden when the `await` is deleted would not notice. Refs: #W15-1, ADR-0076 Co-Authored-By: Claude <noreply@anthropic.com>
Four findings on the ADR, all valid. These are clarifications that ADD precision; none reverses the decision, and nothing depends on the ADR yet (it landed hours ago and its implementation has not started), so they are folded in rather than filed as a superseding ADR — which for "we did not name the event" would be corpus noise, not history. **The event now has a name: `cost:attempt_settled`.** ADR-0074 names `budget:estimate_committed` inline and this one said only "the event", which makes the pair hard to follow across the chain. The name states the pairing — one records an amount that MIGHT have been billed, the other one that WAS — and reuses the admission vocabulary, since a settled admission is exactly the moment it is emitted. **The session path is answered rather than left open**, and the answer inverts the finding: the session path already HAS this ledger. `persister.ts` writes `recordSessionCost` on every `cost:updated` with the PER-ATTEMPT increment, not a cumulative snapshot, and since `#W15-4` that write is latched and a failure gates further egress. So this ADR is run-only by reason, not by omission — it removes the asymmetry ADR-0074 §4 removed in the other direction. Verified in code, not assumed. **Tool-effect cost is now an explicit non-goal.** A resumed run can still re-execute an `http_request` POST or an MCP mutation; the duplicate EFFECT is the harm and no cost bookkeeping prevents it. Named so nobody reads "realized cost is durable" as "a resume cannot repeat work", with the effect journal it needs called out as the next ADR. Recorded in the roadmap too. **The barrier mechanism is stated precisely**, because the reviewer reasonably expected a §2-style queue-and-flush and building one would be wrong. §2 needs `flushBudgetCommitments` because a commitment is emitted fire-and-forget from inside the governor; a settled attempt is emitted by the engine on the path about to continue, so awaiting `#emitDurable` there IS the barrier — and it is total for store faults, so the await cannot hang. A second queue would add back the very concurrency the await removes. Refs: #W15-1, ADR-0076 Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/reference/contracts/sse-event-schema.md (1)
175-183: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd the frozen media pricing fields to
MediaJobSubmittedEvent.Line 175 documents
units?andacceptedCostMicrocents?, and Line 87 lists both fields. However, the illustrative interface at Lines 174-183 stops atdeadlineAt.Add both fields and document that
acceptedCostMicrocentsrequiresunits. Preserve0as a valid accepted cost.Proposed fix
export interface MediaJobSubmittedEvent extends BaseEvent { type: 'media_job:submitted'; nodeId: string; jobId: string; provider: 'anthropic' | 'openai' | 'gemini' | 'deepseek'; model: string; modality: 'image' | 'audio' | 'video'; startedAt: string; deadlineAt: string; + units?: number; // required when acceptedCostMicrocents is present + acceptedCostMicrocents?: number; // frozen accepted reservation; 0 is meaningful }The event table and frozen-basis section already define these fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/contracts/sse-event-schema.md` around lines 175 - 183, Update the illustrative MediaJobSubmittedEvent interface to include optional units and acceptedCostMicrocents fields after deadlineAt, documenting that acceptedCostMicrocents is valid only when units is present while preserving 0 as a valid value. Keep the field types and frozen-pricing semantics consistent with the definitions already used in the event table and frozen-basis section.apps/cli/src/chat/persister.ts (2)
377-404: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute the compaction and trim writes through
persistDurably.The
cost:updatedarm and bothsession:turn_completedarms wrap their store calls inpersistDurably. Thesession:compactedandsession:trimmedarms calldeps.store.writeTurndirectly. A failure in these two arms therefore does not latchdurabilityFailure.The consequence is the state
durabilityFailureexists to prevent. The boundary marker never reaches the transcript,durabilityFailurestaysundefined, thepreEgressgate inapps/cli/src/chat/session-host.tskeeps reporting healthy, and the session keeps sending against a durable transcript that no longer matches the compacted in-memory history. On resume,resumableMessageSequencesthen reads a transcript with no compaction boundary.The in-memory totals are already safe, because a throw skips lines 389-391 and 402. Only the latch is missing.
🛡️ Proposed fix
{ const marker = stageMarker(event.summary, event.keptMessageCount); const nextInput = totalInputTokens + event.tokensUsed.input; const nextOutput = totalOutputTokens + event.tokensUsed.output; - deps.store.writeTurn({ - messages: marker === undefined ? [] : [{ message: marker }], - session: record('active', { input: nextInput, output: nextOutput }), - }); + persistDurably(() => { + deps.store.writeTurn({ + messages: marker === undefined ? [] : [{ message: marker }], + session: record('active', { input: nextInput, output: nextOutput }), + }); + }); if (marker !== undefined) sequenceNumber += 1; // the marker's seq is NOT a real-message seq totalInputTokens = nextInput; totalOutputTokens = nextOutput; } return; case 'session:trimmed': // A deterministic /trim — a summary-less boundary marker, no cost. Flush the row (updatedAt) after. { const marker = stageMarker('', event.keptMessageCount); - deps.store.writeTurn({ - messages: marker === undefined ? [] : [{ message: marker }], - session: record('active'), - }); + persistDurably(() => { + deps.store.writeTurn({ + messages: marker === undefined ? [] : [{ message: marker }], + session: record('active'), + }); + }); if (marker !== undefined) sequenceNumber += 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/chat/persister.ts` around lines 377 - 404, Route the store.writeTurn calls in both session:compacted and session:trimmed arms through persistDurably, preserving their existing marker and totals updates. Ensure failures from either boundary-marker write latch durabilityFailure, while retaining the current in-memory state update ordering and return behavior.
460-485: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
recordConservativeCommitmentthrows synchronously instead of rejecting.
deps.store.recordSessionConservativeCommitmentis synchronous underbetter-sqlite3. If it throws, the throw escapes beforePromise.resolve()is reached, so the method throws instead of returning a rejected promise. The doc comment on lines 119-121 andGovernorWiring.attachConservativeWriterinapps/cli/src/chat/session-host.tsboth describe this seam as a promise the governor's barrier awaits and classifies on rejection.A caller that stores the returned promise before awaiting it, or that does not call the writer inside a
try, receives an unclassified synchronous throw rather than theCommitmentDurabilityErrorthe barrier produces. Make the rejection path explicit so the contract holds for every caller shape.🛡️ Proposed fix
const catalogId = deps.resolveModelCatalogId?.(commitment.model); - deps.store.recordSessionConservativeCommitment({ - id: deps.uuid(), - sessionId: deps.sessionId, - model: commitment.model, // the RAW provider string — the attribution key, never the catalog UUID - ...(catalogId === undefined ? {} : { modelCatalogId: catalogId }), - estimateMicrocents: commitment.estimateMicrocents, - ts: deps.now(), - }); - return Promise.resolve(); + try { + deps.store.recordSessionConservativeCommitment({ + id: deps.uuid(), + sessionId: deps.sessionId, + model: commitment.model, // the RAW provider string — the attribution key, never the catalog UUID + ...(catalogId === undefined ? {} : { modelCatalogId: catalogId }), + estimateMicrocents: commitment.estimateMicrocents, + ts: deps.now(), + }); + } catch (err) { + // The seam is a promise by contract (§4): the governor's barrier classifies a REJECTION, so a + // synchronous driver throw must not escape as one. + return Promise.reject(err instanceof Error ? err : new Error(String(err), { cause: err })); + } + return Promise.resolve();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/chat/persister.ts` around lines 460 - 485, Update recordConservativeCommitment so the synchronous deps.store.recordSessionConservativeCommitment call is captured and converted into a rejected Promise, while successful writes still return a resolved Promise. Preserve the existing catalog resolution and payload, ensuring every caller observes promise-based rejection for store failures.apps/cli/src/chat/chat-mode.ts (1)
203-213: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the docblock: "absent" is no longer the blank condition.
Line 206 states "Every field must be absent for the preview to count as blank." Line 212 now tests
!isInformative(value), so a field that is present but reduced entirely to redaction markers also counts as blank. The comment describes the pre-change rule and contradicts the code it introduces.📝 Proposed fix
- // Keyed by `keyof ToolActionPreview` so a NEW reviewable field breaks the build HERE (it must be added below) - // rather than silently making a preview that carries it look "blank" — which would re-open the `always` blank - // check this closes. Every field must be absent for the preview to count as blank. + // Keyed by `keyof ToolActionPreview` so a NEW reviewable field breaks the build HERE (it must be added below) + // rather than silently making a preview that carries it look "blank" — which would re-open the `always` blank + // check this closes. Every field must be NON-INFORMATIVE (absent, or reduced entirely to redaction markers) + // for the preview to count as blank.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/chat/chat-mode.ts` around lines 203 - 213, Update the comments in isBlankPreview to describe the actual blank condition: every field must be non-informative according to isInformative, including values reduced entirely to redaction markers, rather than strictly absent. Keep the exhaustive keyof ToolActionPreview behavior unchanged.
🧹 Nitpick comments (3)
docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName one canonical event specification.
Line 29 identifies both
sse-event-schema.mdandrun-event.tsas the canonical home. This creates two authorities for the same event contract.Keep
docs/reference/contracts/sse-event-schema.mdas the canonical specification. Describepackages/shared/src/run-event.tsas its validation or implementation source.Proposed correction
- Its exact Zod shape and envelope rules have one canonical home in [sse-event-schema.md](../reference/contracts/sse-event-schema.md) and [run-event.ts](../../packages/shared/src/run-event.ts); + Its exact shape and envelope rules have one canonical specification in [sse-event-schema.md](../reference/contracts/sse-event-schema.md). [run-event.ts](../../packages/shared/src/run-event.ts) validates and implements that contract;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md` at line 29, Update the event-contract wording in this ADR so docs/reference/contracts/sse-event-schema.md is the sole canonical specification; describe packages/shared/src/run-event.ts only as the corresponding validation or implementation source, not as another canonical home.Source: Coding guidelines
apps/cli/src/chat/chat-mode.ts (1)
215-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the shared global regex is safe here.
REDACTION_MARKERis module-level and carries thegflag.String.prototype.replaceresetslastIndexfor a global regex, soisInformativeis not order-dependent. This is correct as written. Do not reuseREDACTION_MARKERwith.test()or.exec()later; those advancelastIndexand would make the result depend on call order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/chat/chat-mode.ts` around lines 215 - 234, Keep REDACTION_MARKER as a replacement-only global regex within isInformative. Do not use it with test() or exec(), or otherwise introduce stateful reuse that advances lastIndex and makes results order-dependent.apps/cli/src/commands/gate-list.ts (1)
90-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sanitizing the run ids before writing them to stderr.
legacyMediaJobHoldNoticeinapps/cli/src/commands/gate.tsappliessanitizeInlineto each node id before it composes the same kind of bounded stderr note.renderUnreadableRunsinterpolatesrunIdsdirectly. Run ids are machine-minted UUIDs today, so there is no current defect. Applying the same treatment keeps the two stderr notices consistent and removes the dependency on that assumption.♻️ Proposed refactor
function renderUnreadableRuns(io: CliIo, runIds: readonly string[]): void { if (runIds.length === 0) return; - const shown = runIds.slice(0, MAX_REPORTED_UNREADABLE); + const shown = runIds.slice(0, MAX_REPORTED_UNREADABLE).map((id) => sanitizeInline(id)); const ids = runIds.length > shown.length ? `${shown.join(', ')}, …` : shown.join(', ');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/gate-list.ts` around lines 90 - 99, Update renderUnreadableRuns to sanitize each run ID with the existing sanitizeInline helper before joining and interpolating IDs into the stderr warning, matching legacyMediaJobHoldNotice while preserving the existing truncation and message behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/chat/persister.test.ts`:
- Line 354: Update the test title for the FAILED-turn token totals case in the
persister test suite to be unique, adding a distinguishing detail such as the
scenario identifier or failed-then-clean context while preserving its existing
meaning.
- Around line 253-273: Update the test around persister.start and the mocked
store.recordSessionCost to retain the spy and assert that recordSessionCost was
called after the user turn completes. Keep the existing durable total invariant
assertion, ensuring the test verifies a failed cost write occurred rather than
passing because no cost update was emitted.
In `@apps/cli/src/chat/persister.ts`:
- Around line 359-365: In the turn persistence flow around persistDurably and
commitTurn, capture the already-validated pendingUserText value in a const
before creating the callback, then pass that narrowed local to commitTurn.
Remove the unsafe as string assertion while preserving the existing undefined
guard and callback behavior.
In `@apps/cli/src/commands/gate-list.ts`:
- Around line 65-75: Update the gate list JSON documentation in the CLI command
reference to describe the unavailable record shape { runId, unavailable:
'corrupt_event_log' } for unreadable runs, alongside the existing pending-gate
record shape. Keep the documented one-record-per-line output contract consistent
with the writeRecordLines behavior in gate list.
In `@apps/cli/src/commands/gate.test.ts`:
- Around line 811-822: Move the ADR-0074 §3 hold-notice docblock from above the
ADR-0075 describe to immediately above the `legacyMediaJobHoldNotice (ADR-0074
§3)` describe. Keep the ADR-0075 refusal docblock directly associated with its
existing describe and preserve both docblock contents unchanged.
In `@apps/cli/src/config/load.ts`:
- Around line 154-162: Update the permission-healing logic around
lstatSync/chmodSync to open the file with O_NOFOLLOW, validate the opened
descriptor using fstatSync(), and apply mode 0o600 via fchmodSync() on that
descriptor. Preserve the existing symlink/non-regular-file safety behavior and
best-effort failure handling, ensuring the descriptor is closed on every path.
In `@apps/cli/src/home/home-store.ts`:
- Around line 138-153: Update the recentRuns filtering logic near readGates and
unreadableRunIds to exclude any run whose ID appears in unreadableRunIds, in
addition to the existing failed and human-gated exclusions. Preserve the
attention-required handling for degraded runs, and add or update the relevant
assertion so bad-01 is absent from recentRuns.
In `@apps/cli/src/render/sanitize.ts`:
- Around line 84-90: Update sanitizeUntrusted and sanitizeUntrustedInline to
strip terminal controls before calling scrubSecrets, ensuring both functions
redact the normalized text. For sanitizeUntrustedInline, apply sanitizeInline
only after terminal-control stripping and secret redaction so inline collapsing
cannot reconstruct an unredacted credential.
In `@docs/decisions/0028-workflow-resource-governance.md`:
- Around line 9-11: In the appended status follow-up and the “Amended 2026-06-18
by ADR-0044” blockquotes, replace the bare blank line separating the two quote
blocks with an empty HTML comment so markdownlint MD028 is satisfied without
merging the amendments.
In `@docs/decisions/0074-durable-conservative-budget-commitments.md`:
- Around line 69-75: Keep accepted ADRs append-only: in
docs/decisions/0074-durable-conservative-budget-commitments.md lines 69-75, move
the /cost --release implementation-status update to release or roadmap
documentation, or create a new ADR only if it changes the decision; remove the
embedded ADR-0075 amendment summary at lines 98-107 because ADR-0075 already
records it; and update
docs/decisions/0070-durable-per-model-session-cost-attribution.md lines 21-25 so
the ADR-0076 relationship is documented in ADR-0076 rather than by amending
ADR-0070.
In `@docs/roadmap/current.md`:
- Around line 146-154: Update the Wave 1 lane (e) status in the headline and
corresponding lane bullet to indicate §1 is complete, consistent with the
durable `/cost --release` behavior documented later and the ✅ `#W15-3` status.
Mark the 2026-07-30 retraction as superseded or remove it so the document no
longer claims the release escape hatch is unreachable.
In `@packages/core/src/engine/agent-session.ts`:
- Around line 640-649: Move the successful-turn await of flushBudgetCommitments
ahead of the `#turnCount` increment and assistant-message append, preserving the
turn-completion durability barrier while ensuring a rejection is rolled back by
the existing catch. Update `#settleTurnError` to classify durability-flush
failures as an expected turn error with the appropriate user-facing
code/message, rather than settling as internal and rethrowing.
---
Outside diff comments:
In `@apps/cli/src/chat/chat-mode.ts`:
- Around line 203-213: Update the comments in isBlankPreview to describe the
actual blank condition: every field must be non-informative according to
isInformative, including values reduced entirely to redaction markers, rather
than strictly absent. Keep the exhaustive keyof ToolActionPreview behavior
unchanged.
In `@apps/cli/src/chat/persister.ts`:
- Around line 377-404: Route the store.writeTurn calls in both session:compacted
and session:trimmed arms through persistDurably, preserving their existing
marker and totals updates. Ensure failures from either boundary-marker write
latch durabilityFailure, while retaining the current in-memory state update
ordering and return behavior.
- Around line 460-485: Update recordConservativeCommitment so the synchronous
deps.store.recordSessionConservativeCommitment call is captured and converted
into a rejected Promise, while successful writes still return a resolved
Promise. Preserve the existing catalog resolution and payload, ensuring every
caller observes promise-based rejection for store failures.
In `@docs/reference/contracts/sse-event-schema.md`:
- Around line 175-183: Update the illustrative MediaJobSubmittedEvent interface
to include optional units and acceptedCostMicrocents fields after deadlineAt,
documenting that acceptedCostMicrocents is valid only when units is present
while preserving 0 as a valid value. Keep the field types and frozen-pricing
semantics consistent with the definitions already used in the event table and
frozen-basis section.
---
Nitpick comments:
In `@apps/cli/src/chat/chat-mode.ts`:
- Around line 215-234: Keep REDACTION_MARKER as a replacement-only global regex
within isInformative. Do not use it with test() or exec(), or otherwise
introduce stateful reuse that advances lastIndex and makes results
order-dependent.
In `@apps/cli/src/commands/gate-list.ts`:
- Around line 90-99: Update renderUnreadableRuns to sanitize each run ID with
the existing sanitizeInline helper before joining and interpolating IDs into the
stderr warning, matching legacyMediaJobHoldNotice while preserving the existing
truncation and message behavior.
In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md`:
- Line 29: Update the event-contract wording in this ADR so
docs/reference/contracts/sse-event-schema.md is the sole canonical
specification; describe packages/shared/src/run-event.ts only as the
corresponding validation or implementation source, not as another canonical
home.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e488a2-84bc-4b4f-8cc0-344277452479
📒 Files selected for processing (79)
apps/cli/src/chat/chat-mode.test.tsapps/cli/src/chat/chat-mode.tsapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/chat/repl-info.test.tsapps/cli/src/chat/repl-info.tsapps/cli/src/chat/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/agent-run.test.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/gate-list.test.tsapps/cli/src/commands/gate-list.tsapps/cli/src/commands/gate.test.tsapps/cli/src/commands/gate.tsapps/cli/src/commands/repl-commands.test.tsapps/cli/src/commands/repl-commands.tsapps/cli/src/commands/status.test.tsapps/cli/src/commands/status.tsapps/cli/src/config/load.test.tsapps/cli/src/config/load.tsapps/cli/src/engine/build-engine.tsapps/cli/src/engine/checkpointer.test.tsapps/cli/src/engine/checkpointer.tsapps/cli/src/gate/pending.tsapps/cli/src/history/per-run-read.test.tsapps/cli/src/history/per-run-read.tsapps/cli/src/home/home-store.test.tsapps/cli/src/home/home-store.tsapps/cli/src/index.tsapps/cli/src/process/background-failure.tsapps/cli/src/process/errors.test.tsapps/cli/src/process/errors.tsapps/cli/src/process/render-error.tsapps/cli/src/process/sleep.test.tsapps/cli/src/process/sleep.tsapps/cli/src/render/records.test.tsapps/cli/src/render/records.tsapps/cli/src/render/renderer.test.tsapps/cli/src/render/renderer.tsapps/cli/src/render/sanitize.test.tsapps/cli/src/render/sanitize.tsapps/cli/src/render/tui/home-app.test.tsxapps/cli/src/render/tui/home-controller.test.tsapps/cli/src/render/tui/home-projection.tsapps/cli/src/render/tui/home-view.tsxapps/cli/src/render/tui/run-view-model.test.tsapps/cli/src/render/tui/run-view-model.tsdocs/decisions/0028-workflow-resource-governance.mddocs/decisions/0070-durable-per-model-session-cost-attribution.mddocs/decisions/0074-durable-conservative-budget-commitments.mddocs/decisions/0075-fail-closed-resume-on-an-unreadable-event-log.mddocs/decisions/0076-durable-per-attempt-realized-cost-ledger.mddocs/decisions/README.mddocs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/llm-provider-seam.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mdpackages/core/src/engine/agent-session.tspackages/core/src/engine/budget-governor.tspackages/core/src/engine/checkpoint.test.tspackages/core/src/engine/checkpoint.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/session-resume.test.tspackages/db/src/client.tspackages/db/src/index.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/db/src/session-store.test.tspackages/db/src/session-store.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.test.tspackages/llm/src/adapters/shared.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tspackages/llm/src/index.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- docs/decisions/README.md
- apps/cli/src/process/errors.ts
- apps/cli/src/chat/repl-info.test.ts
- apps/cli/src/engine/build-engine.ts
- packages/db/src/index.ts
- apps/cli/src/index.ts
- apps/cli/src/history/per-run-read.test.ts
- apps/cli/src/process/background-failure.ts
- apps/cli/src/render/tui/home-app.test.tsx
- packages/llm/src/adapters/openai.ts
- packages/llm/src/adapters/shared.ts
- packages/shared/src/run-event.test.ts
- packages/core/src/engine/checkpoint.test.ts
- apps/cli/src/commands/chat.ts
- packages/core/src/engine/budget-governor.ts
- packages/core/src/engine/engine.ts
- docs/roadmap/phases/phase-2.5.5-hardening-and-remediation.md
- docs/reference/shared-core/tool-registry.md
Comment on lines
65
to
75
| if (deps.global.json) { | ||
| writeRecordLines(deps.io, rows); | ||
| // One extra record per unreadable run, on the SAME one-record-per-line stream (ADR-0049). It carries no | ||
| // `gateId`, so a consumer selecting gates is unaffected while a careful one can tell "damaged" from | ||
| // "none". A stderr-only note (the `logs --json` precedent) would have kept stdout pure but left the | ||
| // machine contract silently short of gates, which is the defect itself. | ||
| writeRecordLines(deps.io, [ | ||
| ...rows, | ||
| ...unreadableRunIds.map((runId) => ({ runId, unavailable: 'corrupt_event_log' })), | ||
| ]); | ||
| return EXIT_CODES.success; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the canonical reference for CLI JSON output and check whether the new marker is recorded.
set -uo pipefail
fd -e md . docs/reference | head -50
rg -n -C3 'corrupt_event_log|unavailable' docs/ || echo "NOT DOCUMENTED in docs/"
# Other consumers of the gate-list JSON stream
rg -n -C3 "gate list --json|gateListCommand" --glob '!**/*.test.ts'Repository: HodeTech/Relavium
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- gate-list documentation references ---'
rg -n -C8 -i 'gate list|gate-list|pending gates|NDJSON|machine output contract' docs/reference/cli/commands.md docs/reference/cli/README.md docs/reference/cli/home.md
printf '%s\n' '--- implementation and tests ---'
rg -n -C5 'corrupt_event_log|unreadableRunIds|gate list|writeRecordLines' apps/cli/src apps/cli/test apps/cli --glob '*.{ts,tsx}' --glob '*.md' 2>/dev/null | head -300Repository: HodeTech/Relavium
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- canonical gate-list reference ---'
sed -n '272,285p' docs/reference/cli/commands.md
printf '%s\n' '--- implementation and tests ---'
rg -n -C4 'corrupt_event_log|unreadableRunIds|writeRecordLines' apps/cli/src/commands apps/cli --glob '*.{ts,tsx}' --glob '*.{test,spec}.{ts,tsx}' | head -240Repository: HodeTech/Relavium
Length of output: 19217
Document the gate list --json unavailable record.
Update docs/reference/cli/commands.md to include { runId, unavailable: 'corrupt_event_log' } for unreadable runs alongside the pending-gate record shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/commands/gate-list.ts` around lines 65 - 75, Update the gate
list JSON documentation in the CLI command reference to describe the unavailable
record shape { runId, unavailable: 'corrupt_event_log' } for unreadable runs,
alongside the existing pending-gate record shape. Keep the documented
one-record-per-line output contract consistent with the writeRecordLines
behavior in gate list.
Source: Coding guidelines
Comment on lines
+811
to
+822
| /** | ||
| * ADR-0074 §3's hold notice. The engine sink that feeds it was DEAD — `WorkflowEngine` never read | ||
| * `onLegacyMediaJobHold` — so this sentence had never rendered in production and nothing pinned it. It is | ||
| * what stops a resumed run that is correctly holding from looking like an unexplained stall. | ||
| */ | ||
| /** | ||
| * ADR-0075's refusal, at the surface a user actually meets. The typed error was introduced with a mapper in | ||
| * `toUserFacing` and this catch — which pre-dates it — folded it into a generic `invalid_invocation`, so the | ||
| * count, the `seq` values, the upgrade remedy and the exit code were all discarded one frame above the mapper | ||
| * written for them. Nothing pinned the rendered outcome, which is why that was possible. | ||
| */ | ||
| describe('gateCommand — a run written by a NEWER binary (ADR-0075)', () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the hold-notice docblock to the describe it documents.
The docblock at Lines 811-815 describes ADR-0074 §3's legacyMediaJobHoldNotice and its dead onLegacyMediaJobHold sink. It sits directly above the ADR-0075 describe at Line 822, which is about an unreadable event log. The describe it belongs to is legacyMediaJobHoldNotice (ADR-0074 §3) at Line 896. Two stacked, unrelated docblocks also make the ADR-0075 block harder to attribute.
📝 Proposed fix
-/**
- * ADR-0074 §3's hold notice. The engine sink that feeds it was DEAD — `WorkflowEngine` never read
- * `onLegacyMediaJobHold` — so this sentence had never rendered in production and nothing pinned it. It is
- * what stops a resumed run that is correctly holding from looking like an unexplained stall.
- */
/**
* ADR-0075's refusal, at the surface a user actually meets. …
*/
describe('gateCommand — a run written by a NEWER binary (ADR-0075)', () => {Then place the removed docblock immediately above Line 896:
+/**
+ * ADR-0074 §3's hold notice. The engine sink that feeds it was DEAD — `WorkflowEngine` never read
+ * `onLegacyMediaJobHold` — so this sentence had never rendered in production and nothing pinned it. It is
+ * what stops a resumed run that is correctly holding from looking like an unexplained stall.
+ */
describe('legacyMediaJobHoldNotice (ADR-0074 §3)', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/commands/gate.test.ts` around lines 811 - 822, Move the ADR-0074
§3 hold-notice docblock from above the ADR-0075 describe to immediately above
the `legacyMediaJobHoldNotice (ADR-0074 §3)` describe. Keep the ADR-0075 refusal
docblock directly associated with its existing describe and preserve both
docblock contents unchanged.
Comment on lines
+69
to
+75
| > **Closed 2026-08-09.** The release half shipped with the persistence, as this note required: `/cost --release` | ||
| > on the chat surface — the one that renders the amount, which is what §1 ties it to — clearing the durable | ||
| > per-model and aggregate columns first and the live governor second, so a released commitment does not return | ||
| > on the next `chat-resume`. `/cost` also renders the durability state. The reserved `budget:estimate_released` | ||
| > event stays reserved and unemitted: the session path restores its conservative total from COLUMNS rather than | ||
| > by replaying events, so zeroing them is what makes the release durable there. That event is needed only by a | ||
| > workflow-run release surface, which does not exist — the workflow escapes named above still stand. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep accepted ADRs append-only.
These amendments modify prior ADR bodies. Record new decisions in a new superseding ADR instead. Keep implementation-status updates in release or roadmap documentation.
docs/decisions/0074-durable-conservative-budget-commitments.md#L69-L75: move the/cost --releaseimplementation-status update to release or roadmap documentation, or to a new ADR if it changes the decision.docs/decisions/0074-durable-conservative-budget-commitments.md#L98-L107: remove the embedded ADR-0075 amendment summary. ADR-0075 already records the superseding decision.docs/decisions/0070-durable-per-model-session-cost-attribution.md#L21-L25: keep the ADR-0076 relationship in ADR-0076 rather than amending ADR-0070.
As per coding guidelines, “ADRs are append-only, and changes must be documented in a new superseding ADR.”
📍 Affects 2 files
docs/decisions/0074-durable-conservative-budget-commitments.md#L69-L75(this comment)docs/decisions/0074-durable-conservative-budget-commitments.md#L98-L107docs/decisions/0070-durable-per-model-session-cost-attribution.md#L21-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/decisions/0074-durable-conservative-budget-commitments.md` around lines
69 - 75, Keep accepted ADRs append-only: in
docs/decisions/0074-durable-conservative-budget-commitments.md lines 69-75, move
the /cost --release implementation-status update to release or roadmap
documentation, or create a new ADR only if it changes the decision; remove the
embedded ADR-0075 amendment summary at lines 98-107 because ADR-0075 already
records it; and update
docs/decisions/0070-durable-per-model-session-cost-attribution.md lines 21-25 so
the ADR-0076 relationship is documented in ADR-0076 rather than by amending
ADR-0070.
Source: Coding guidelines
Comment on lines
+640
to
+649
| // ADR-0074 §2: the enclosing turn completion WAITS for the commitment's durability acknowledgement. | ||
| // Before this, a chat reported a turn complete — `sendMessage` returned, the process could exit — while | ||
| // the commitment for a possibly-billed call had not reached the database. A failure here fails THIS turn, | ||
| // which is the point: §2 says a durability failure fails the active owner loudly, and surfacing it on some | ||
| // later unrelated turn is exactly the misattribution the barrier exists to prevent. | ||
| // | ||
| // Only the SUCCESS path awaits it this way. An abort or a classified turn error already carries its own | ||
| // terminal, and replacing a `provider_auth` failure with a durability failure would hide the cause the | ||
| // user needs; the governor keeps the debit and its sticky `conservativeDurabilityBroken` regardless. | ||
| await this.#deps.flushBudgetCommitments?.(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
The flush await breaks the rollback invariant the catch depends on.
The catch at line 659 pops one message and documents why that is safe on lines 662-664: "Nothing is pushed after the user message on a throw (the assistant append is past the await), so the last element is always that message."
The new await on line 649 runs after the assistant append on line 638. When result.text.length > 0 and flushBudgetCommitments rejects, the pop removes the ASSISTANT message and leaves the user message in #messages.
The result is the exact state the rollback exists to prevent. The transcript keeps a trailing unanswered user message. The next sendMessage appends a second user message, and the provider rejects two consecutive user messages. #turnCount on line 628 has also already been incremented for a turn that now settles as a failure.
A second effect follows from the same placement. A durability rejection is neither an AgentTurnError nor a BudgetPauseError, so #settleTurnError takes the unclassified branch. The turn completes with code internal and the message "the session turn failed with an unexpected error", and the error is re-thrown out of sendMessage. A commitment durability failure is an expected failure mode, so it should not surface as an unclassified internal bug.
Move the barrier ahead of the turn-count increment and the assistant append. Nothing is emitted between those points, so §2's "the enclosing turn completion waits for the acknowledgement" still holds.
🐛 Proposed fix
if (this.#statusIs('cancelled')) {
this.#messages.pop();
return;
}
+ // ADR-0074 §2: the enclosing turn completion WAITS for the commitment's durability acknowledgement.
+ // Before this, a chat reported a turn complete — `sendMessage` returned, the process could exit — while
+ // the commitment for a possibly-billed call had not reached the database.
+ //
+ // It is awaited HERE, before the turn count and the assistant append, and NOT just before the emit: the
+ // `catch` below pops exactly one message on the documented assumption that a throw can only ever leave
+ // the user message last. Awaiting after the assistant append would break that assumption and leave a
+ // dangling user message that the next turn turns into two consecutive `user` messages.
+ //
+ // Only the SUCCESS path awaits it this way. An abort or a classified turn error already carries its own
+ // terminal; the governor keeps the debit and its sticky `conservativeDurabilityBroken` regardless.
+ await this.#deps.flushBudgetCommitments?.();
// EA7 note: an `abort()` that lands AFTER the turn fully resolved (a late `Esc`, past the turn core's
@@
if (result.text.length > 0) {
this.#messages.push({ role: 'assistant', content: [{ type: 'text', text: result.text }] });
}
- // ADR-0074 §2: the enclosing turn completion WAITS for the commitment's durability acknowledgement.
- // Before this, a chat reported a turn complete — `sendMessage` returned, the process could exit — while
- // the commitment for a possibly-billed call had not reached the database. A failure here fails THIS turn,
- // which is the point: §2 says a durability failure fails the active owner loudly, and surfacing it on some
- // later unrelated turn is exactly the misattribution the barrier exists to prevent.
- //
- // Only the SUCCESS path awaits it this way. An abort or a classified turn error already carries its own
- // terminal, and replacing a `provider_auth` failure with a durability failure would hide the cause the
- // user needs; the governor keeps the debit and its sticky `conservativeDurabilityBroken` regardless.
- await this.#deps.flushBudgetCommitments?.();
this.#emitTurnCompleted(result.stopReason, {Classify the durability failure as well, so it does not settle as internal and re-throw. Add a branch to #settleTurnError, or wrap the flush:
+ try {
+ await this.#deps.flushBudgetCommitments?.();
+ } catch (err) {
+ throw new AgentTurnError(
+ 'internal',
+ err instanceof Error ? err.message : String(err),
+ false,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/engine/agent-session.ts` around lines 640 - 649, Move the
successful-turn await of flushBudgetCommitments ahead of the `#turnCount`
increment and assistant-message append, preserving the turn-completion
durability barrier while ensuring a rejection is rolled back by the existing
catch. Update `#settleTurnError` to classify durability-flush failures as an
expected turn error with the appropriate user-facing code/message, rather than
settling as internal and rethrowing.
Verified each finding against current code. The one that mattered contradicted my own written reasoning, and it was right. **`sanitizeUntrusted` leaked a credential.** The order was redact-then-normalize, and this file's docblock admitted it could not construct a case where the order mattered. There is one: a key split by a byte `stripTerminalControls` REMOVES (`U+0001`) defeats the pattern while the byte is present, and the strip then REJOINS the halves — printing the whole key on exactly the failure paths #W15-8 hardened. Normalize-then-redact sees the contiguous key and replaces it. The reviewer's stated example — a newline rejoining after the inline collapse — does NOT demonstrate it: a newline collapses to a SPACE, so the halves never become contiguous. That near-miss is why the old order looked safe, and both cases are now pinned so the distinction is not lost. Break-verified: reversing the order reddens the leak test. **`agent-session` could leave a dangling user turn.** The rollback catch pops exactly ONE message and its comment relies on "nothing is pushed after the user message on a throw" — which stopped being true when ADR-0074 §2's `await` landed between the user push and the assistant push. A flush rejection popped the ASSISTANT message and left the user turn dangling, the exact shape that rollback prevents. The await now runs before the append, restoring the invariant. **A #W15-4 test was vacuous**, as its own comment half-admitted: it passed when no `cost:updated` was emitted at all. It now asserts the spy fired and the failure latched, before the restore. Also folded: a degraded run rendered in BOTH Home strips (attention and Continue); `gate list`'s stderr warning interpolated run ids without `sanitizeInline` while its sibling notice sanitizes; the compaction and trim boundary writes bypassed `persistDurably`, so a failure there did not latch; `recordConservativeCommitment` threw synchronously into a barrier documented as needing "a real rejection"; the unsafe `as string` in the turn arm; the duplicate test title, `String.raw` and `codePointAt` (Sonar); and four documentation corrections — including the roadmap headline still calling lane (e) incomplete after `#W15-3` closed it. SKIPPED, with reasons: the ADR append-only objections — in-place dated amendment notes are this corpus's established convention (ADR-0028 carries one from ADR-0044, ADR-0074 added one to ADR-0028), so following it is not a rewrite; and `REDACTION_MARKER` is already replacement-only. DEFERRED, recorded rather than half-done: `config/load.ts`'s lstat→chmod TOCTOU (the `O_NOFOLLOW` + `fchmod` form), and two Sonar cognitive-complexity refactors in `gate.ts` and `run-view-model.ts`. Refs: #W15-4, #W15-8, #W15-12, #W15-15, ADR-0074 §2 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md (2)
33-35: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the awaited durability barrier fail closed.
#emitDurablecatchespersistEvent, sets#failure, aborts the run, and resolves. Callers do not receive a failure. For example,#stepdispatches the node immediately after awaitingnode:started, without checking#failure. The abort signal is cooperative, so the engine can still invoke the executor after persistence fails.Return or reject a failure and stop before dispatch. Add a regression test that rejects
persistEventand asserts that no later executor, provider, or tool effect occurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md` around lines 33 - 35, The awaited durability barrier must propagate persistence failures instead of resolving silently. Update `#emitDurable` to return or reject the persistEvent failure, and make callers such as `#step` check that result before dispatching the node or proceeding to any executor, provider, or tool effect. Add a regression test covering rejected persistEvent and asserting no subsequent side effect occurs.
29-36: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine stable idempotency for each settled provider attempt.
UNIQUE(run_id, seq)rejects only an exact sequence replay. It does not deduplicate the same attempt when a retry receives a new sequence. The in-memoryattemptNumberalso resets for a newChainRun. Define a stable attempt identifier or a deterministic sequence rule, and test recovery after commit but before acknowledgement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md` around lines 29 - 36, Revise the idempotency section of the ADR to define a stable identifier or deterministic sequence rule for each settled provider attempt that remains consistent across retries and new ChainRun instances. Update the persistence/recovery description to use that identity for deduplication, and require a test covering recovery after the event commits but before acknowledgement.apps/cli/src/render/sanitize.test.ts (1)
67-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the non-null assertion from the test. Replace
hostile.codePointAt(0)!with a checked value or fixed expected code point. The test cases otherwise cover all requested behaviors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/sanitize.test.ts` around lines 67 - 103, Remove the non-null assertion in the hostile-character test within stringifyJsonLine by storing or validating the result of codePointAt(0) before converting it to hexadecimal. Preserve the existing assertions and test cases while ensuring the code handles the possibly undefined return safely.Source: Coding guidelines
🧹 Nitpick comments (1)
apps/cli/src/render/sanitize.test.ts (1)
41-41: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove scanner-detectable credential literals from the test.
Betterleaks reports the fixtures on Lines 41 and 51 as generic API keys. These are deterministic test values, not live credentials. They can still fail a secret-scanning gate or hide a real finding in alert noise.
Construct the suffix at runtime from deterministic non-secret characters, or use the repository's approved test-secret marker. Keep the runtime value compatible with
scrubSecrets. Verify the configured security scan after the change.As per coding guidelines, “When a change touches keys, cryptography, the keychain, custom provider base URLs, or the JavaScript sandbox, flag it for explicit security review.”
Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/render/sanitize.test.ts` at line 41, Replace the scanner-detectable credential literals in the test fixtures around key and scrubSecrets coverage with runtime-constructed deterministic non-secret values or the repository-approved test-secret marker. Preserve compatibility with scrubSecrets, update both affected fixtures, and verify the configured security scan passes.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/render/sanitize.test.ts`:
- Around line 47-53: Strengthen the newline-split credential test around
sanitizeUntrustedInline by asserting that the credential suffix is not exposed
and the redaction marker is present, rather than only checking the original
contiguous key is absent. Update the sanitizer’s normalization/redaction flow so
keys split by a newline are detected and redacted across the normalized
separator.
---
Outside diff comments:
In `@apps/cli/src/render/sanitize.test.ts`:
- Around line 67-103: Remove the non-null assertion in the hostile-character
test within stringifyJsonLine by storing or validating the result of
codePointAt(0) before converting it to hexadecimal. Preserve the existing
assertions and test cases while ensuring the code handles the possibly undefined
return safely.
In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md`:
- Around line 33-35: The awaited durability barrier must propagate persistence
failures instead of resolving silently. Update `#emitDurable` to return or reject
the persistEvent failure, and make callers such as `#step` check that result
before dispatching the node or proceeding to any executor, provider, or tool
effect. Add a regression test covering rejected persistEvent and asserting no
subsequent side effect occurs.
- Around line 29-36: Revise the idempotency section of the ADR to define a
stable identifier or deterministic sequence rule for each settled provider
attempt that remains consistent across retries and new ChainRun instances.
Update the persistence/recovery description to use that identity for
deduplication, and require a test covering recovery after the event commits but
before acknowledgement.
---
Nitpick comments:
In `@apps/cli/src/render/sanitize.test.ts`:
- Line 41: Replace the scanner-detectable credential literals in the test
fixtures around key and scrubSecrets coverage with runtime-constructed
deterministic non-secret values or the repository-approved test-secret marker.
Preserve compatibility with scrubSecrets, update both affected fixtures, and
verify the configured security scan passes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8457e63e-75a1-468d-aebe-8123ecc32a6c
📒 Files selected for processing (11)
apps/cli/src/chat/chat-mode.tsapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/commands/gate-list.tsapps/cli/src/home/home-store.tsapps/cli/src/render/sanitize.test.tsapps/cli/src/render/sanitize.tsdocs/decisions/0028-workflow-resource-governance.mddocs/decisions/0076-durable-per-attempt-realized-cost-ledger.mddocs/roadmap/current.mdpackages/core/src/engine/agent-session.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/decisions/0028-workflow-resource-governance.md
- packages/core/src/engine/agent-session.ts
- apps/cli/src/home/home-store.ts
- apps/cli/src/chat/chat-mode.ts
- apps/cli/src/render/sanitize.ts
- apps/cli/src/commands/gate-list.ts
- apps/cli/src/chat/persister.ts
- docs/roadmap/current.md
Comment on lines
+47
to
+53
| it('does not leak when a NEWLINE splits it either — the near-miss that made the old order look safe', () => { | ||
| // Recorded because it is the case that misled the earlier reasoning: a newline is COLLAPSED to a space, | ||
| // not removed, so the halves never become contiguous and neither order redacts a usable key. That is why | ||
| // "I could not construct a case" was not evidence — only the REMOVED bytes rejoin. | ||
| const key = join('sk-', 'ant-', 'api03-', 'AbCdEf0123456789xyz'); | ||
| expect(sanitizeUntrustedInline(`${key.slice(0, 12)}\n${key.slice(12)}`)).not.toContain(key); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strengthen the newline test.
Line 52 checks only that the exact contiguous key is absent. Because sanitizeInline converts the newline to a space, sanitizeUntrustedInline can return sk-ant-api03 AbCdEf0123456789xyz and still pass. The credential material remains visible.
If this path must prevent credential disclosure, assert that the suffix is absent and [REDACTED] is present. Update the sanitizer to redact across the normalized separator. If the split form is intentionally allowed, rename the test and assert the exact allowed output.
Suggested assertion for the redaction contract
- expect(sanitizeUntrustedInline(`${key.slice(0, 12)}\n${key.slice(12)}`)).not.toContain(key);
+ const sanitized = sanitizeUntrustedInline(`${key.slice(0, 12)}\n${key.slice(12)}`);
+ expect(sanitized).not.toContain('0123456789xyz');
+ expect(sanitized).toContain('[REDACTED]');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('does not leak when a NEWLINE splits it either — the near-miss that made the old order look safe', () => { | |
| // Recorded because it is the case that misled the earlier reasoning: a newline is COLLAPSED to a space, | |
| // not removed, so the halves never become contiguous and neither order redacts a usable key. That is why | |
| // "I could not construct a case" was not evidence — only the REMOVED bytes rejoin. | |
| const key = join('sk-', 'ant-', 'api03-', 'AbCdEf0123456789xyz'); | |
| expect(sanitizeUntrustedInline(`${key.slice(0, 12)}\n${key.slice(12)}`)).not.toContain(key); | |
| }); | |
| it('does not leak when a NEWLINE splits it either — the near-miss that made the old order look safe', () => { | |
| // Recorded because it is the case that misled the earlier reasoning: a newline is COLLAPSED to a space, | |
| // not removed, so the halves never become contiguous and neither order redacts a usable key. That is why | |
| // "I could not construct a case" was not evidence — only the REMOVED bytes rejoin. | |
| const key = join('sk-', 'ant-', 'api03-', 'AbCdEf0123456789xyz'); | |
| const sanitized = sanitizeUntrustedInline(`${key.slice(0, 12)}\n${key.slice(12)}`); | |
| expect(sanitized).not.toContain('0123456789xyz'); | |
| expect(sanitized).toContain('[REDACTED]'); | |
| }); |
🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/render/sanitize.test.ts` around lines 47 - 53, Strengthen the
newline-split credential test around sanitizeUntrustedInline by asserting that
the credential suffix is not exposed and the redaction marker is present, rather
than only checking the original contiguous key is absent. Update the sanitizer’s
normalization/redaction flow so keys split by a newline are detected and
redacted across the normalized separator.
…ells **ADR-0076 said the awaited emit IS the barrier. It is not, on its own.** Verified against `engine.ts`: `#emitDurable` is *total for store faults* — a `persistEvent` rejection is absorbed into the run's failure state and the promise RESOLVES. So a caller that only awaits it walks straight into the next tool effect or egress on a run whose ledger write did not land, which is the exact thing the property exists to forbid. The ADR now requires await AND an explicit check of the run's failure/abort state, and names the regression test that has to plant a rejecting `persistEvent`. It also records why making `#emitDurable` reject is NOT the fix: its totality is what keeps the exactly-one-terminal-event invariant and stops an unhandled rejection escaping the fire-and-forget loop. **The idempotency claim overreached.** "Re-persisting an attempt is impossible" holds for one emitted event, not for a crash after the row commits: the resumed run re-dispatches the node and makes a NEW provider call, which is a real second charge and correctly gets its own row. The ledger is not double-counting one charge — it is recording two. The duplicate the user actually experiences is the re-run, which no cost key prevents; that is the effect-journal decision this ADR already scopes out. Stated rather than left to be discovered. **The newline-split credential test was asserting the wrong thing.** It checked only that the CONTIGUOUS key was absent, which is trivially true of a split string. The secret suffix IS still printed, so this is an exposure. Pinned as current behaviour with the suffix asserted present, because closing it means teaching the shared `scrubSecrets` to match across a separator — broadening a redactor every surface depends on, at real risk to ordinary diagnostics. That is a change to `@relavium/llm`, not a local edit, and a test that implied it was already safe was worse than one that says it is not. Also: the non-null assertion in the hostile-codepoint loop, and the two cognitive-complexity smells deferred last round — `gate.ts`'s checkpoint-load fault mapping is now `throwGateLoadFault`, and `run-view-model.ts`'s three terminal arms are `reduceTerminal`. Both behaviour-preserving; the suites pass unchanged. SKIPPED: the test-fixture credential literals are already built at runtime via `join(...)` precisely so no contiguous key-shaped literal sits in source, which is what the finding asks for. Refs: ADR-0076, #W15-8 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md (1)
94-95: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the replay guarantee.
Lines 55-59 state that a crash can cause the resumed run to make a new provider call and incur a new charge. Therefore, the statement that a resumed run “no longer re-spends it” is incorrect. State that the ledger preserves the charge for accounting and cap decisions, but does not prevent provider-call replay.
Proposed wording
- A crash mid-agent-loop no longer discards realized spend, and a resumed run no longer re-spends it. The cap survives the boundary it was previously blind to, which is what makes it a control rather than a heuristic. + A crash mid-agent-loop no longer discards realized spend. A resumed run includes that spend in its cap and accounting state, but it may still re-dispatch the provider call after a crash. Preventing that duplicate effect remains out of scope for this ADR.This finding uses the ADR’s own replay caveat in Lines 55-59 and effect-journal scope in Lines 72-77.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md` around lines 94 - 95, Correct the replay claim in the listed durable-ledger benefits: state that the ledger preserves realized charges for accounting and cap decisions after a crash, while explicitly retaining that a resumed run may replay the provider call and incur a new charge. Update the sentence beginning “A crash mid-agent-loop” without changing the separate per-attempt and per-model attribution statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/render/sanitize.test.ts`:
- Around line 47-59: Fix the newline-split credential leak in
sanitizeUntrustedInline by updating the shared scrubSecrets redaction or
normalization contract to recognize credentials separated by newline whitespace.
Preserve normal diagnostic text where possible, then update the test to assert
the credential suffix is absent and the output contains [REDACTED] instead of
documenting the limitation.
---
Outside diff comments:
In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md`:
- Around line 94-95: Correct the replay claim in the listed durable-ledger
benefits: state that the ledger preserves realized charges for accounting and
cap decisions after a crash, while explicitly retaining that a resumed run may
replay the provider call and incur a new charge. Update the sentence beginning
“A crash mid-agent-loop” without changing the separate per-attempt and per-model
attribution statement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed9a926e-e075-4855-b72c-f6032a4097db
📒 Files selected for processing (4)
apps/cli/src/commands/gate.tsapps/cli/src/render/sanitize.test.tsapps/cli/src/render/tui/run-view-model.tsdocs/decisions/0076-durable-per-attempt-realized-cost-ledger.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/render/tui/run-view-model.ts
…umenting it Last round I pinned this as a known limitation. That was the wrong call: the exposure is the secret SUFFIX, and "documented" does not make a printed key safer. Unlike the control bytes `stripTerminalControls` removes, a newline is display-significant, so normalization COLLAPSES it to a space rather than deleting it — the halves never become contiguous and the in-place pass sees nothing. `redactIncludingAcrossSeparators` now asks a second question when the first pass finds nothing: is there a credential once the separators are gone? If so it redacts the WHOLE text. Coarse on purpose. A boolean cannot locate the span, and this path's stated posture is already that a false positive costs a `[REDACTED]` in a diagnostic while a miss costs a key. A test pins the shapes that must NOT trip it — ENOENT, a locked database with a retry line, a rate-limit notice, a two-line node failure — so the fallback cannot quietly start eating ordinary output. Deliberately NOT in the shared `scrubSecrets`, which the reviewer offered as the alternative: every surface depends on it, and the same broadening there would put ordinary output at risk far outside the failure paths this hardening is scoped to. Break-verified: removing the cross-separator check reddens the leak test. Also corrects ADR-0076's first Positive bullet, which claimed "a resumed run no longer re-spends it". It does re-spend — it replays the provider call and incurs a new charge. What changes is that the cap remembers the first one instead of being understated by exactly that amount. The idempotency section already said this; the benefit list still said the stronger thing. Refs: #W15-8, ADR-0076 Co-Authored-By: Claude <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Aug 10, 2026
Verified every ✅ in PR #81's closing register against the code rather than trusting the marks: `#W15-3`'s `/cost --release`, `#W15-4`'s durability latch, `#W15-5`'s projection check, `#W15-6`'s folds, `#W15-7`'s refinement, `#W15-9`'s fold guard, `#W15-10`'s escaper, `#W15-13`'s opt-in heal, `#W15-14`'s `hostSleep`, `#W15-15`'s degraded flag, `#W15-22`'s `Reflect.apply`, `#W15-23`'s `TxDb`. All present. ADR-0074/0075/0076 are Accepted and both new ones are indexed. Two markings were wrong, both in the same milestone note: - it said ADR-0074 §1 "remains open". It closed on 2026-08-09 with `/cost --release` (`commands/chat.ts` calls `releaseConservativeCommitments`; `repl-info.ts` renders the durability state and points at the flag). - it called that work "Wave 1.5". That framing was retracted — the work was PR #81's own closing list, not a later wave — and this was the last place still carrying it. Also: the register said the fixes were "committed on `development`". They are merged to `main`. `#W15-1` stays OPEN and is NOT marked. Its decision is made (ADR-0076, Accepted) but no implementation has landed, so the count remains 23 of 24. Marking it now is exactly the false-completion error this register was rebuilt to correct. Refs: PR #81, ADR-0074 §1, ADR-0076 Co-Authored-By: Claude <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Aug 11, 2026
… its run Verified each finding against the tree; fixed the ones that still hold. **Correctness.** `checkDurableTruth` reported foreign events and then compared against them anyway: `history` read ANOTHER run's terminal and `durableTerminalCount` counted it, so the verdict named the right cause while every downstream number described the wrong run. Every view now reads the runId-filtered log. Break-verified: reverting the filter reddens the foreign-log test at `durableTerminalCount`. `ledger B3`'s ordering assertion was vacuous — `indexOf` returns `-1` for a missing entry, which is less than everything, so a run that never settled a charge satisfied the ordering it exists to prove. Presence is asserted first. `tools/test-isolation` counted `...REPO_LOCAL_CHECKOUTS` repo-wide and required 2 — which is also what two spreads in `test.exclude` and none in `coverage.exclude` looks like, the exact half-failure assertion 4 is for. Now counted per property, exactly once each. Break-verified with that mutation. `persister.test.ts` restored its SQLITE_BUSY spies at the END of a test, so one failing assertion leaked a throwing `updateSession` into every later test in the file. Restore is suite-level now. **Docs — the contract contradicted itself.** `sse-event-schema.md` called `cost:attempt_settled.cumulativeCostMicrocents` "NOT the restore source" and `costMicrocents` "the delta a reader SUMS", while the fold rule three lines below says restore is `Math.max` over the absolutes and must never sum the deltas. The fold rule is what `checkpoint.ts` implements; both call-outs now agree with it. Roadmap: PR #81 (23 items, merged) and `#W15-1`/PR #82 (open) were recorded as one closure; separated. `45 CR items` → 46, the count the phase doc states. CR-03's Fix/Acceptance said three paths where five shipped. **Skipped, with reason.** The `run_costs` node-retry attribution identity needs a schema column + migration + snapshot regen — the caveat is recorded in `nodeSettledCost` and the run-level SUM stays exact either way; it is already a `#W15-1` follow-up. Condensing ADR-0077 conflicts with append-only, and at 214 lines it sits between ADR-0070 (204) and ADR-0073 (213), so it is not an outlier. The README architecture SVG is a curated asset, not a diagram that drifted. `settle()` stays fixed-turn: it drains to quiescence to prove an action did NOT run, which no predicate can express. Also folded: codepoint sort in `canonical()` (`localeCompare` is locale-dependent, so the instrument could manufacture a disagreement), `#flushBudgetCommitments` → `#joinMoneyDurability` (it fronts both money chains since ADR-0077; the host-supplied `AgentSessionDeps.flushBudgetCommitments` keeps its name and is correct), cognitive complexity 39 → five named helpers, the sync-throw recovery test now reuses the instance that took the throw, and two `expect:'repaired'` branches plus the derived fixture types the `as never` was hiding. `pnpm run ci` and `pnpm coverage` green — 96.97% lines / 93.34% branches. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Aug 11, 2026
Phase 2.6.5 status moves to in progress with a Progress block naming the six closed items, their closure dates, and the three gaps carried forward rather than implied: ADR-0077's unbuilt required regression (without it `#runAttempt`'s money-durability arm is unreached), `CR-10`'s property being inexpressible from the durable log alone, and the oracle's three remaining debts to `CR-92`. `current.md`: the Wave 1 closure is now merged on both PRs (#81 on 2026-08-09, `#W15-1` + the first 2.6.5 batch via #82 on 2026-08-11), the ledger node is marked complete in the graph, and the 2.6.5 section carries a live 6-of-46 count pointing at `CR-10` as next. Co-Authored-By: Claude Opus 5 <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
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.
Wave 1 — Stop the bleeding
Closes all three CRITICALs of Phase 2.5.5's Wave 1, plus the states where
max_cost_microcentswassilently not a cap, plus the money hole those four lanes left open (ADR-0074). Five lanes; the first
four were developed in parallel and consolidated as
--no-ffmerges so each lane's history stays areadable unit, and lane (e) landed step-by-step on top.
Closes M2.5.5-1.
registry.tsrawPreviewFor+unredactedPreviewinregistry.ts/tools/types.tspackages/dbchainSQLITE_BUSY_SNAPSHOT,withBusyRetryAsync,writeTurn+background-failure.ts,migrate-lock.ts,DbOpenError,deprecationDateguardRunApp9,final-summary6,render-error4,clack-prompter4;run-job-control.ts; #50 by ordering (drive-home.tsxsubscribes signals at:808, beforerunOnboardingWizardat:855)maxRetries,Retry-After,CostTrackerbounds, narrowed#emitSuccesscatchparseStoredRunEvent; §2 event + barrier + checkpoint fold + resume; §3reserveAcceptedCost; §4 two columns +recordSessionConservativeCommitmentEvery mark above was re-verified against the shipped source before it went in, and the check is
stated rather than the task text restated — #50 in particular is a sequence claim that no grep
count could have proved.
Two gaps recorded rather than closed
Neither is a shipped defect; both have mutation-verified coverage one layer down, and both are marked
in the source at the line they concern.
state machine is mutation-verified; its composition with
#countRunning's termination gate is not.That composition gap is exactly what let a hang ship mid-review (see below).
restoreConservativeCostis wired throughbuildSessionRuntime.AgentSession.resumecalling the hook IS mutation-verified inpackages/core; only the spread thatsupplies it is unproven. I wrote that test, mutation-tested it, found it green against a deleted
wiring line, and removed it rather than keep false assurance.
§1's release: exposed, not reachable
ADR-0074 §1 ties a commitment's release to the surface that renders it. Rendering is done on all
three surfaces (run TUI,
relavium logs, and/cost, which reads estimated, possibly billed with(no completed call)for a commitment-only row). Release is exposed and tested onGovernorWiringbut nothing calls it, so a user cannot yet clear a commitment. The workflow surfaceis unaffected — a run is not long-lived and
on_exceed's escapes are §1's own analogy — but a chat'stotal is now durable across resume, so the accidental escape a restart used to provide is gone. The
remaining step needs the reserved
budget:estimate_releasedevent, or a release would simply returnon the next
chat-resume.Decisions recorded
runMigrationsis serialized across processes by an OS lock file, notby a transaction.
BEGIN IMMEDIATE, the originally-proposed fix, is not implementable:drizzle's migrator runs the
SELECTthat decides what to apply outside its own transaction, andits raw
BEGINthrows if the call is hoisted into ours. The ADR records that explicitly so it isnot re-proposed, and names
flock/LockFileExas the mechanically superior option rejected onlybecause Node exposes no dependency-free advisory lock.
amends ADR-0028's estimate-and-block cap. Its §2 carries a dated note recording what implementation
settled that the Decision left to the spec: a reader restores the conservative total by summing
estimateMicrocents, never last-wins over the cumulative snapshot (the engine assignssequenceNumberafter anawaitand disclaims any canonical order), and §1's release isrepresentable as a reserved
budget:estimate_released.What the review rounds changed
Each lane-(a)/(b) step got an Opus round and a Sonnet second pass with adversarial verification of
every finding. Three of them overturned work I had already committed:
.envis a protected path — it is not (the write-side rulecovers
.git/.relavium/.ssh+ rc files). Then Opus proved a whole-string scrub swallows./Access Token Backup/.ssh/authorized_keysdown to./Access Token [redacted], flipping theauto-mode protected-path classification. I moved to per-segment scrubbing; Sonnet then proved that
reopens the leak for a credential spanning a separator (
./api_key=AAAAA/BBBBBB.txtpassesthrough untouched). Both directions come from the same regex's
/-bearing value class, so nosingle pattern gives both properties. Resolved by splitting the two uses:
previewis thescrubbed display copy,
unredactedPreviewis the in-process classification copy that neverreaches the event.
unanswered
userrow that resume rolls back. Opus reproduced the truth: because the session nowsurvives, the orphan is buried mid-transcript (
0:user 2:user 3:assistant) and resume rolls backonly trailing ones — so a resume replays two consecutive user messages, which providers reject.
Fixed with an atomic
writeTurn, which also discharges the per-turn-transaction follow-updatabase-schema.mdhas tracked since 2.5.I.pnpm run ciwas RED and I missed it twice. Two of my test files provoke throws out of aRunEventBussubscriber; with no sink wired the bus re-throws out-of-band by design, which vitestcounts as a file-level error. Both files reported every test passed and exited 1. I had been
grepping the reporter's summary line instead of the exit status, and read turbo's
"12 successful, 14 total" as caching rather than two failed tasks.
What lane (e)'s review rounds overturned
Each of ADR-0074's four sections got an Opus round and a Sonnet second pass. Five of them overturned
code I had already committed, and two of those were money-losing:
units: positiveIntrejected a legal workflow after the provider had billed.duration_secondsis fractional by contract, so
units = duration × countis too. A 12.5-second video job failedvalidation at the bus, the
media_job:submittedrow was never written, and a job the provider hadaccepted and billed became unpollable and unresumable. Every test in the repo used integer
durations, which is why nothing caught it.
retryable: truethat was a no-op. §3's hold mapped tobudget_exceededwithretryable: true— but#shouldRetrygates onRETRYABLE_ERROR_CODESmembership even whenretry_onisabsent, and
retry_onis schema-restricted to the same set. The label changed; the behaviour didnot. A resumed run with a legacy media job and a ready sibling still died within milliseconds,
abandoning the very job it was waiting for.
checkPreEgresskept its node counted asrunning, so#stepnever reached#countRunning() === 0and the run emitted no terminal at all — while the awaited job's poll returned silently on abort
without releasing the hold. Now one abort listener registered at construction releases every hold,
so no future abort site can forget it.
acceptedCostMicrocents: 0on the approved-bypass path. Under H3's one-shot cap bypass nopre-egress hook runs, so
?? 0froze "priced at zero" for a job that had never been priced. Onresume that reserved nothing AND skipped the fail-closed hold, letting a sibling spend headroom
still owed to a job deliberately submitted over the cap. The field is now omitted, routing the
resume through the legacy branch.
/clear,chat-resume,the Home's inline chat and
agent runall built a governor and attached nothing, so everycommitment rejected and marked the session durability-broken. The fix is not "remember to call it":
the persister attaches itself, and
governoris a required dep so the compiler demands a decisionat every site. That change immediately surfaced two sites I had not looked at.
Two lessons about my own process are worth recording, since they explain otherwise-odd commits in the
history. My mutation harness silently no-oped twice — the edit script matched a source string a
prettier reflow had already changed, so I read an unmutated run as proof a test was vacuous and
deleted two good tests. Every mutation claim in this PR was subsequently re-verified by checking the
mutation actually applied. And a review agent left
zz-review-scratch.test.tsin the tree, whichthe wiring fix removed.
Verification
pnpm run ci→ exit 0 (checked by exit status): 23/23 + 7/7 turbo tasks, 4,834 tests,format:check, the seam fence, engine-dep allowlists, bundle closure, and the compiled-binary smoke.against the unfixed source before the fix landed.
were rewritten — the egress-host arm of #91 was untested (reverting it left all 101 tests green),
command: '[redacted]'(redact everything) passed, revertingpersistEventto the sync retry twinleft all 257 db tests green, and moving
realMessageSeqs.pushbefore the write left the wholesuite green while silently corrupting every later ADR-0062 compaction boundary.
Known limitations, stated rather than buried
passes every run; without it, it fails roughly one run in three, because the collision depends
on
spawntiming. That is the same flakiness that let #99 ship. The deterministic per-branchguards are the injected-clock unit tests. A READY-handshake barrier would fix the detection rate;
I attempted it, hit a deadlock I could not resolve within this change's budget, and recorded it as
a follow-up rather than ship a hanging test.
commandpreview can blind the approver. The command is fully model-controlled andthe detector's patterns are public, so an injected model can deliberately match one and be shown
sh -c [redacted]— which reads as "we protected you" rather than "you cannot see this". Boundedby deny-all-by-default command allowlists and the fs/egress floors. Surfacing "this was redacted"
needs a schema field; recorded as the follow-up.
checked each finding's claim against the shipped source — the four sanitizer boundaries by call
count, fix(cli,ci): correct relavium --version + release-smoke install path; mark 2.L Done #50 by the signal-vs-wizard ordering, the governor ledger and the llm-side retry/pricing
guards by their named symbols — and all of them hold. What I did not do is read those diffs line
by line against their acceptance paragraphs. Artifact-level verification is weaker than a review and
is named as such.
Conformance
any, no unsafeas, no@ts-ignore.packages/corepurity intact — no platform import added.@relavium/llmseam.node:fs's atomic'wx'create, the idiom already used inconfig/write.tsandmedia-write.ts.--json;unredactedPreviewis in-process only and asserted absent from the emitted body.Note on authorship
Two commits (
77028b7, anda59800f's subject) originate from automated agents rather than adeliberate authored change:
77028b7was committed by a verification agent and carried a leftover[...messages].reverse()mutation, which114aa28removes. Flagging it because the message is notmine.
🤖 Generated with Claude Code
Summary by Sourcery
Strengthen CLI safety and resilience by adding POSIX job control handling, conservative budget admissions and async media cost accounting, cross-process migration locking, atomic session turn writes under contention, secret-safe tool approval previews, and comprehensive sanitization and error-handling across the TUI and CLI.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
/cost --release, improved budget controls, retry handling, and async media recovery.