Wave 1 — stop the bleeding: three CRITICALs, the cost cap, and ADR-0073/0074 - #81
Open
cemililik wants to merge 47 commits into
Open
Wave 1 — stop the bleeding: three CRITICALs, the cost cap, and ADR-0073/0074#81cemililik wants to merge 47 commits into
cemililik wants to merge 47 commits into
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>
…ation The lane merges each landed prettier-clean in isolation, but the consolidated tree tripped `format:check` on five files (one from lane b, four from lanes c/d). `pnpm run ci` is exit 0 after this — verified by exit status, not by reading the reporter's summary line. Co-Authored-By: Claude <noreply@anthropic.com>
…mitted 0073 draft) Lane (d)'s code (G38/G39/G47/G49 — the reservation ledger and the re-armed budget warnings) merged into development with `Refs: G38, G39, G47, G49, 2.6.Q` and no ADR reference, because its decision record was still an untracked draft in that lane's worktree. CLAUDE.md rule 9 wants the record for exactly this kind of change: it amends ADR-0028's estimate-and-block pre-egress cap, plus 0036, 0045 and 0070. Renumbered 0073 → 0074: the draft claimed 0073, but 0073 was already taken by the history.db migration lock, which is Accepted and merged. ADR numbers are never reused, and assignment follows landing order. Only the number changed — the H1 and this index row; the body is the other lane's work verbatim, still Status: Proposed and awaiting the maintainer's ruling. Refs: ADR-0074, ADR-0028, G38 Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR hardens CLI terminal and output handling, separates approval classification from display redaction, adds failure reporting, introduces budget admission accounting, protects migrations with SQLite locking, expands retries, and makes session transcript writes atomic. ChangesCLI and runtime hardening
Budget and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Input
participant JobControl
participant Terminal
participant Process
Input->>JobControl: Ctrl-Z or SIGTSTP
JobControl->>Terminal: release alternate screen and mouse state
JobControl->>Process: suspendSelf()
Process-->>JobControl: SIGCONT
JobControl->>Terminal: reclaim alternate screen and input state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements Wave 1 fixes across four lanes: (a) secure tool-approval previews and terminal sanitization, (b) durable and conservative budget governance including async media and strict cost caps, (c) robust POSIX job-control and onboarding/Ink terminal handoff in the CLI, and (d) SQLite durability improvements including an ADR-backed cross‑process migration lock, async busy-retry, and atomic chat session turn writes — all with extensive regression tests and documentation updates (ADR‑0073/0074, roadmap/status docs). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
apps/cli/src/engine/model-catalog-view.ts (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unsound
Record<K, string>assertion.For a union
K,Record<K, string>promises every key exists, but this helper writes only the single runtimekey. ReturnPartial<Record<K, string>>and assign into a local result instead.Suggested refactor
-): Record<K, string> | Record<string, never> { - return value === undefined ? {} : ({ [key]: value } as Record<K, string>); +): Partial<Record<K, string>> { + const result: Partial<Record<K, string>> = {}; + if (value !== undefined) result[key] = value; + return result;As per coding guidelines: TypeScript must use strict typing and avoid unsafe type assertions.
🤖 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/engine/model-catalog-view.ts` around lines 71 - 76, Update spreadIfSet to avoid asserting the single computed property as Record<K, string>, which is unsound for union keys. Build the result in a local object typed as Partial<Record<K, string>> and return that for both undefined and defined values, preserving the existing empty-object behavior.Source: Coding guidelines
packages/db/src/client.test.ts (1)
466-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
instanceofguards instead of type assertions.
expect(...).toBeInstanceOf()does not narrowcaught; theas DbOpenErrorcasts violate the strict-TypeScript rule. Guard withif (!(caught instanceof DbOpenError)) throw ...before assigningcaughttoerror.As per coding guidelines, “Use TypeScript with strict mode; do not use
anyor unsafe type assertions, and prefer type guards.”Also applies to: 488-489
🤖 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/db/src/client.test.ts` around lines 466 - 467, Replace the unsafe `caught as DbOpenError` assertions in the affected test cases with explicit `instanceof DbOpenError` guards; throw or fail the test when the guard fails, then assign `caught` to `error` only after narrowing. Apply this to both locations around the existing `toBeInstanceOf` expectations.Source: Coding guidelines
packages/db/src/session-store.test.ts (1)
606-613: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis doc block belongs to the other suite (and describes a technique neither uses).
It talks about the three single-statement writers under contention, but sits on the
writeTurn is atomicdescribe; the writers suite at Line 682 already has its own accurate doc. Thebusy_timeout = 0+ held-write-lock reproduction it claims is also not what happens —failOnceinjects the fault. Retarget it to the atomicity claim.🤖 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/db/src/session-store.test.ts` around lines 606 - 613, The doc block above the “SessionStore — writeTurn is atomic (`#228`)” suite describes the wrong writer-contention scenario and reproduction method. Replace it with documentation focused on writeTurn’s atomicity guarantee, removing references to the three single-statement writers, busy_timeout, held locks, and failOnce unless directly relevant; leave the separate writers suite documentation unchanged.packages/core/src/tools/registry.ts (1)
366-383: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider stripping
unredactedPreviewbefore callingemitApprovalRequested.
emitApprovalRequestedis called with the fullrequestobject, including the in-process-onlyunredactedPreview. Per the field's own contract ("must not log, persist or forward it"), this boundary is currently guarded only by downstream discipline (confirmed today byagent-session.test.ts's assertion that the emitted event lacks the field), not by the type system at this call site. Passing an explicitly-stripped object here would make the boundary structurally impossible to violate.🔒 Proposed fix
try { - approval.emitApprovalRequested?.(request); + const { unredactedPreview: _unredactedPreview, ...observableRequest } = request; + approval.emitApprovalRequested?.(observableRequest); } catch {🤖 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/tools/registry.ts` around lines 366 - 383, Update the emitApprovalRequested call in the governed dispatch approval flow to pass an explicitly stripped request containing only the event-safe fields, excluding unredactedPreview. Preserve unredactedPreview on the internal ToolApprovalRequest for classification use, but ensure it cannot cross the observability boundary through this call.apps/cli/src/commands/chat.ts (1)
1804-1804: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
onListenerErrorgrouped with the other notice sinks. It currently sits betweenchatandnow, away fromonBudgetWarning/onEffortWithheld/onUnpricedat the bottom of the same literal — purely cosmetic, but the grouping is what makes a missing sink obvious in review.♻️ Proposed reordering
const wiringDeps: ReseatWiringDeps = { chat: params.chat, - onListenerError: (note: string) => emitLiveNotice(params.io, note), now: params.now, @@ onBudgetWarning: (warning) => emitLiveNotice(params.io, budgetWarningText(warning)), onEffortWithheld: onceEffortNotice((note) => emitLiveNotice(params.io, note)), + onListenerError: (note: string) => emitLiveNotice(params.io, note), onUnpriced: (note) => emitLiveNotice(params.io, note), };🤖 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/chat.ts` at line 1804, Reorder the onListenerError property in the relevant options literal so it is grouped with the other notice sinks—onBudgetWarning, onEffortWithheld, and onUnpriced—at the bottom. Do not change its callback implementation or behavior.apps/cli/src/render/tui/RunApp.test.tsx (1)
54-71: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider also pinning the newline collapse, not just control-byte removal. The payload embeds
\nin each field precisely to forge extra rows; the current assertions only prove ESC/C1/C0/bidi are gone. Asserting that a forged suffix stays on the same rendered row would cover the row-forging half of the threat.♻️ Suggested extra assertion
expect(frame).toContain('node visible'); + // sanitizeInline collapses \n → space, so a forged suffix can never own its own row. + expect(frame).toMatch(/tool visible.*forged tool row/);🤖 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/tui/RunApp.test.tsx` around lines 54 - 71, Extend the assertions in the RunApp rendering test to verify that newline-containing untrusted payloads are collapsed or sanitized rather than creating additional rendered rows. Keep the existing forbidden-character checks and visible-content assertions, and add an assertion against the rendered frame that confirms each forged suffix remains on the same row as its corresponding payload.apps/cli/src/render/suspend.test.ts (1)
545-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ctx.skip()over a bare early return for the POSIX-only case.An early
returnmakes the test report as passing on Windows even though nothing was asserted;ctx.skip()records it as skipped.♻️ Proposed change
- it('the production lifecycle adds and removes SIGTSTP/SIGCONT only on POSIX', () => { - if (process.platform === 'win32') return; + it('the production lifecycle adds and removes SIGTSTP/SIGCONT only on POSIX', (ctx) => { + if (process.platform === 'win32') ctx.skip();🤖 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/suspend.test.ts` around lines 545 - 546, Update the POSIX-only test “the production lifecycle adds and removes SIGTSTP/SIGCONT only on POSIX” to call the test context’s skip method when process.platform is win32 instead of returning early, so Windows records the test as skipped while POSIX continues through the existing assertions.Source: Linters/SAST tools
apps/cli/src/render/tui/home-app.test.tsx (1)
210-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also covering the
key.ctrl+zbranch.Only the literal SUB byte is exercised; the
key.ctrl === true && input === 'z'path added inapps/cli/src/render/tui/home-app.tsx(Lines 388-391) — the one real Ink terminals take — is untested, so a regression there would pass CI.🤖 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/tui/home-app.test.tsx` around lines 210 - 222, Extend the RootApp raw Ctrl-Z routing tests to cover the key event branch where key.ctrl is true and input is 'z', using the existing Home harness and onSuspend mock. Assert onSuspend is called once while the controller remains in home mode with empty input, matching the literal SUB-byte test.apps/cli/src/process/render-error.test.ts (1)
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared
captureIofromapps/cli/src/test-support.ts.
apps/cli/src/process/background-failure.test.tsimports the shared helper for the same purpose; this local copy duplicates theCliIoshape and will drift when the interface grows.🤖 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/process/render-error.test.ts` around lines 9 - 24, Remove the local captureIo helper from render-error.test.ts and import the shared captureIo from test-support.ts, matching the existing usage in background-failure.test.ts. Update references as needed while preserving the test behavior and shared helper’s stderr capture.
🤖 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/chat-mode-host.ts`:
- Line 1: Update the inline documentation in chat-mode-host.ts and chat-mode.ts
to reference ToolApprovalRequest.unredactedPreview instead of the nonexistent
target field, including the corresponding link target in chat-mode.ts. Preserve
the surrounding classification documentation unchanged.
- Around line 62-64: Update the comments near the approval-path logic to
reference the actual ToolApprovalRequest field, unredactedPreview, instead of
the nonexistent request.target; preserve the distinction between the unredacted
value and the redacted display preview and keep the existing precedence
explanation accurate.
In `@apps/cli/src/chat/chat-mode.ts`:
- Around line 166-169: The documentation in chat-mode.ts incorrectly links to
ToolApprovalRequest.target; update the reference to the actual
ToolApprovalRequest.unredactedPreview member, while preserving the explanation
that security classification must use the unredacted request data rather than
request.preview.
In `@apps/cli/src/config/load.test.ts`:
- Around line 145-162: Update the skipped test around loadConfigFile to mock
chmodSync so it throws an EACCES error, ensuring the test exercises the chmod
failure catch path rather than relying on directory permissions. Dynamically
import the module under test after installing the mock, then call its
loadConfigFile and preserve the expected successful config result and cleanup.
In `@apps/cli/src/home/drive-home.tsx`:
- Around line 183-189: Move the onboardingTerminal.onInput stdin subscription so
it is attached only after runOnboardingWizard starts, preventing stdin from
entering flowing mode during the gap before the first prompt. Keep the existing
Ctrl-Z handling and cleanup behavior intact.
In `@docs/project-structure.md`:
- Line 26: Update the shared-package count in the introduction of the
project-structure documentation from five to six, ensuring the prose, diagram,
and table consistently reflect the addition of packages/mcp.
In `@docs/reference/shared-core/tool-registry.md`:
- Around line 250-265: Update the canonical ToolApprovalRequest interface near
the documented toolId, action, and preview fields to include unredactedPreview,
with an explicit note that it is for in-process classification only and must
never be serialized or emitted across event, IPC, or --json boundaries. Keep the
existing preview contract and field-selection semantics unchanged.
- Around line 266-270: Update the command preview and approval flow described in
the tool registry documentation so detector-triggered redaction is explicitly
shown as “preview redacted—contents unavailable,” rather than appearing safe or
fully reviewed. Ensure approvers cannot mistake the placeholder for the actual
command, and require either denial or an explicit stronger confirmation before
approving a redacted process preview.
In `@docs/roadmap/current.md`:
- Around line 113-115: Update the ADR-number reservation text in the roadmap
entry to reflect that ADR-0073 and ADR-0074 are now used, changing the next
unused range from ADR-0073+ to ADR-0075+ while preserving the existing
description of the nine unwritten 2.6 ADRs.
In `@packages/db/src/client.ts`:
- Around line 102-108: Update the catch block that throws DbOpenError to use a
stable generic user-facing message without interpolating err or err.message.
Preserve the original driver or filesystem error only through the cause option,
and keep the existing path argument and open_failed classification unchanged.
---
Nitpick comments:
In `@apps/cli/src/commands/chat.ts`:
- Line 1804: Reorder the onListenerError property in the relevant options
literal so it is grouped with the other notice sinks—onBudgetWarning,
onEffortWithheld, and onUnpriced—at the bottom. Do not change its callback
implementation or behavior.
In `@apps/cli/src/engine/model-catalog-view.ts`:
- Around line 71-76: Update spreadIfSet to avoid asserting the single computed
property as Record<K, string>, which is unsound for union keys. Build the result
in a local object typed as Partial<Record<K, string>> and return that for both
undefined and defined values, preserving the existing empty-object behavior.
In `@apps/cli/src/process/render-error.test.ts`:
- Around line 9-24: Remove the local captureIo helper from render-error.test.ts
and import the shared captureIo from test-support.ts, matching the existing
usage in background-failure.test.ts. Update references as needed while
preserving the test behavior and shared helper’s stderr capture.
In `@apps/cli/src/render/suspend.test.ts`:
- Around line 545-546: Update the POSIX-only test “the production lifecycle adds
and removes SIGTSTP/SIGCONT only on POSIX” to call the test context’s skip
method when process.platform is win32 instead of returning early, so Windows
records the test as skipped while POSIX continues through the existing
assertions.
In `@apps/cli/src/render/tui/home-app.test.tsx`:
- Around line 210-222: Extend the RootApp raw Ctrl-Z routing tests to cover the
key event branch where key.ctrl is true and input is 'z', using the existing
Home harness and onSuspend mock. Assert onSuspend is called once while the
controller remains in home mode with empty input, matching the literal SUB-byte
test.
In `@apps/cli/src/render/tui/RunApp.test.tsx`:
- Around line 54-71: Extend the assertions in the RunApp rendering test to
verify that newline-containing untrusted payloads are collapsed or sanitized
rather than creating additional rendered rows. Keep the existing
forbidden-character checks and visible-content assertions, and add an assertion
against the rendered frame that confirms each forged suffix remains on the same
row as its corresponding payload.
In `@packages/core/src/tools/registry.ts`:
- Around line 366-383: Update the emitApprovalRequested call in the governed
dispatch approval flow to pass an explicitly stripped request containing only
the event-safe fields, excluding unredactedPreview. Preserve unredactedPreview
on the internal ToolApprovalRequest for classification use, but ensure it cannot
cross the observability boundary through this call.
In `@packages/db/src/client.test.ts`:
- Around line 466-467: Replace the unsafe `caught as DbOpenError` assertions in
the affected test cases with explicit `instanceof DbOpenError` guards; throw or
fail the test when the guard fails, then assign `caught` to `error` only after
narrowing. Apply this to both locations around the existing `toBeInstanceOf`
expectations.
In `@packages/db/src/session-store.test.ts`:
- Around line 606-613: The doc block above the “SessionStore — writeTurn is
atomic (`#228`)” suite describes the wrong writer-contention scenario and
reproduction method. Replace it with documentation focused on writeTurn’s
atomicity guarantee, removing references to the three single-statement writers,
busy_timeout, held locks, and failOnce unless directly relevant; leave the
separate writers suite documentation unchanged.
🪄 Autofix (Beta)
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: 2ff9acd2-781f-4f8c-9a73-6c6556218347
📒 Files selected for processing (82)
.claude/agents/relavium-reviewer.md.claude/skills/add-package/SKILL.md.claude/skills/security-review/SKILL.md.claude/skills/start-task/SKILL.md.claude/skills/write-architecture-doc/SKILL.mdCLAUDE.mdapps/cli/src/chat/chat-mode-host.test.tsapps/cli/src/chat/chat-mode-host.tsapps/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/session-host.test.tsapps/cli/src/chat/session-host.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat-alt-hoist.test.tsapps/cli/src/commands/chat.test.tsapps/cli/src/commands/chat.tsapps/cli/src/config/load.test.tsapps/cli/src/config/load.tsapps/cli/src/db/open.test.tsapps/cli/src/db/open.tsapps/cli/src/engine/model-catalog-view.test.tsapps/cli/src/engine/model-catalog-view.tsapps/cli/src/gate/clack-prompter.test.tsapps/cli/src/gate/clack-prompter.tsapps/cli/src/home/drive-home.test.tsapps/cli/src/home/drive-home.tsxapps/cli/src/index.tsapps/cli/src/process/background-failure.test.tsapps/cli/src/process/background-failure.tsapps/cli/src/process/render-error.test.tsapps/cli/src/process/render-error.tsapps/cli/src/render/alt-screen.test.tsapps/cli/src/render/alt-screen.tsapps/cli/src/render/suspend.test.tsapps/cli/src/render/suspend.tsapps/cli/src/render/tui/RunApp.test.tsxapps/cli/src/render/tui/RunApp.tsxapps/cli/src/render/tui/chat-app.test.tsxapps/cli/src/render/tui/chat-ink.tsxapps/cli/src/render/tui/final-summary.test.tsapps/cli/src/render/tui/final-summary.tsapps/cli/src/render/tui/home-app.test.tsxapps/cli/src/render/tui/home-app.tsxapps/cli/src/test-support.tsdocs/architecture/overview.mddocs/decisions/0073-history-db-migration-lock.mddocs/decisions/0074-durable-conservative-budget-commitments.mddocs/decisions/README.mddocs/project-structure.mddocs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/database-schema.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mdpackages/core/src/engine/agent-runner.e2e.test.tspackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/budget-governor.test.tspackages/core/src/engine/budget-governor.tspackages/core/src/engine/engine.tspackages/core/src/engine/m2-e2e-harness.e2e.test.tspackages/core/src/tools/registry.test.tspackages/core/src/tools/registry.tspackages/core/src/tools/types.tspackages/db/src/client.test.tspackages/db/src/client.tspackages/db/src/fixtures/migrate-racer.mjspackages/db/src/index.tspackages/db/src/migrate-lock.e2e.test.tspackages/db/src/migrate-lock.test.tspackages/db/src/migrate-lock.tspackages/db/src/retry.test.tspackages/db/src/retry.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/db/src/session-store.test.tspackages/db/src/session-store.ts
| @@ -1,6 +1,6 @@ | |||
| import { resolve } from 'node:path'; | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale request.target doc references — field is unredactedPreview. Both files describe the classification field as request.target / {@link ToolApprovalRequest.target}, but ToolApprovalRequest has no target member — it's unredactedPreview (packages/core/src/tools/types.ts:293-321). Same fix in both places.
apps/cli/src/chat/chat-mode-host.ts#L62-64: replacerequest.targetwithrequest.unredactedPreviewin the inline comment.apps/cli/src/chat/chat-mode.ts#L166-169: replacerequest.targetand{@linkToolApprovalRequest.target}withrequest.unredactedPreviewand{@linkToolApprovalRequest.unredactedPreview}.
🤖 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-host.ts` at line 1, Update the inline
documentation in chat-mode-host.ts and chat-mode.ts to reference
ToolApprovalRequest.unredactedPreview instead of the nonexistent target field,
including the corresponding link target in chat-mode.ts. Preserve the
surrounding classification documentation unchanged.
Comment on lines
+145
to
+162
| it.skipIf(!POSIX)( | ||
| 'NEVER throws when the mode cannot be changed — a config layer holds no secrets', | ||
| () => { | ||
| // A read-only parent directory blocks the chmod. Unlike `history.db`, the at-rest guarantee does not rest | ||
| // on this, so a permissive-but-readable config must still load rather than failing startup. | ||
| const readOnlyDir = join(dir, 'locked'); | ||
| mkdirSync(readOnlyDir); | ||
| const file = join(readOnlyDir, 'config.toml'); | ||
| writeFileSync(file, 'update_channel = "beta"\n'); | ||
| chmodSync(file, 0o444); | ||
| chmodSync(readOnlyDir, 0o500); | ||
| try { | ||
| expect(loadConfigFile(file, GlobalConfigSchema)).toEqual({ update_channel: 'beta' }); | ||
| } finally { | ||
| chmodSync(readOnlyDir, 0o700); // so the temp dir can be swept | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a real chmod failure.
Making the parent directory 0500 does not stop the file owner from calling chmod on config.toml; this test exercises a successful chmod rather than the catch path. Mock chmodSync to throw EACCES after importing the module under test dynamically.
🤖 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/config/load.test.ts` around lines 145 - 162, Update the skipped
test around loadConfigFile to mock chmodSync so it throws an EACCES error,
ensuring the test exercises the chmod failure catch path rather than relying on
directory permissions. Dynamically import the module under test after installing
the mock, then call its loadConfigFile and preserve the expected successful
config result and cleanup.
Comment on lines
+183
to
+189
| onInput: (listener) => { | ||
| const onData = (chunk: Buffer | string): void => { | ||
| listener(typeof chunk === 'string' ? chunk : chunk.toString('utf8')); | ||
| }; | ||
| process.stdin.on('data', onData); | ||
| return () => process.stdin.removeListener('data', onData); | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How does the wizard/prompter read stdin? Does anything pause/resume it?
fd -t f 'wizard.ts|prompter' apps/cli/src | xargs rg -n -C3 'stdin|setRawMode|readline|pause|resume'
rg -n -C3 "onInput|isRawMode|setRawMode" apps/cli/src/home/drive-home.test.tsRepository: HodeTech/Relavium
Length of output: 4819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and nearby tests.
sed -n '150,230p' apps/cli/src/home/drive-home.tsx
printf '\n--- clack-prompter ---\n'
sed -n '1,220p' apps/cli/src/gate/clack-prompter.ts
printf '\n--- drive-home tests around raw input ---\n'
sed -n '680,940p' apps/cli/src/home/drive-home.test.tsRepository: HodeTech/Relavium
Length of output: 17871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the onInput callsite and the onboarding flow around it.
rg -n -C4 "onInput\\(" apps/cli/src/home/drive-home.tsx
sed -n '820,880p' apps/cli/src/home/drive-home.tsx
# Inspect Clack onboarding prompt wiring/tests for input ownership assumptions.
rg -n -C3 "onboardingTerminalLifecycle|raw input|Clack owns stdin|raw mode" apps/cli/src/home/drive-home.test.ts apps/cli/src/gate/*.tsRepository: HodeTech/Relavium
Length of output: 8720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'JS'
const { Readable } = require('stream');
const r = new Readable({
read() {}
});
const events = [];
r.on('data', (chunk) => events.push(`a:${chunk.toString()}`));
r.push('x');
r.push(null);
console.log('single-listener:', JSON.stringify(events));
const r2 = new Readable({
read() {}
});
const events2 = [];
r2.on('data', (chunk) => events2.push(`a:${chunk.toString()}`));
setTimeout(() => {
r2.on('data', (chunk) => events2.push(`b:${chunk.toString()}`));
}, 0);
r2.push('y');
r2.push(null);
setTimeout(() => {
console.log('delayed-listener:', JSON.stringify(events2));
}, 10);
JSRepository: HodeTech/Relavium
Length of output: 201
Attach the stdin observer after the wizard starts. onboardingTerminal.onInput(...) puts stdin into flowing mode before runOnboardingWizard(...), so keystrokes typed in that gap are dropped instead of reaching the first prompt. If this is only for Ctrl-Z, move the subscription closer to Clack or document the early-input loss.
🤖 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/home/drive-home.tsx` around lines 183 - 189, Move the
onboardingTerminal.onInput stdin subscription so it is attached only after
runOnboardingWizard starts, preventing stdin from entering flowing mode during
the gap before the first prompt. Keep the existing Ctrl-Z handling and cleanup
behavior intact.
Comment on lines
+199
to
+224
| const claim: LockClaim = { pid: process.pid, startedAt: now(), nonce }; | ||
|
|
||
| const deadline = now() + MAX_WAIT_MS; | ||
| let vanished = 0; | ||
| for (;;) { | ||
| if (tryCreate(lockPath, claim)) { | ||
| try { | ||
| return run(); | ||
| } finally { | ||
| release(lockPath); | ||
| } | ||
| } | ||
| const held = readClaim(lockPath); | ||
| if (held === undefined) { | ||
| // Released between our create attempt and the read — go straight back for it. Bounded: if `tryCreate` | ||
| // keeps failing while the file keeps reading as absent, the filesystem is refusing the lock (a read-only | ||
| // directory, a mount without real `O_EXCL`) and no amount of retrying will change that. Spinning there | ||
| // would hang first-run startup, so a handful of attempts is enough to conclude it and reconcile instead. | ||
| vanished += 1; | ||
| if (vanished <= MAX_VANISHED_RETRIES) { | ||
| continue; | ||
| } | ||
| return runReconciling(run); | ||
| } | ||
| const stale = held.unreadable === true || now() - held.startedAt >= STALE_MS; | ||
| if (stale && tryTakeover(lockPath, claim)) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
startedAt is stamped before the wait, shrinking the holder's own staleness window.
claim is built once at Line 199, but acquisition can happen up to MAX_WAIT_MS (10 s) later at Line 204/224. The lock we publish then advertises a start time from before the wait, so a legitimately-held migration becomes "stale" after ~20 s of holding instead of 30 s — and a third process can take it over mid-migration. Stamp the time at each attempt; keep the nonce stable so takeover verification still works.
🛠️ Stamp startedAt per acquisition attempt
const lockPath = `${dbPath}.migrate.lock`;
- const claim: LockClaim = { pid: process.pid, startedAt: now(), nonce };
+ // Stamped per ATTEMPT, not once: a waiter may poll for up to MAX_WAIT_MS before it acquires, and a
+ // pre-wait `startedAt` would let a third process presume our live migration stale early.
+ const mkClaim = (): LockClaim => ({ pid: process.pid, startedAt: now(), nonce });
const deadline = now() + MAX_WAIT_MS;
let vanished = 0;
for (;;) {
- if (tryCreate(lockPath, claim)) {
+ if (tryCreate(lockPath, mkClaim())) {- if (stale && tryTakeover(lockPath, claim)) {
+ if (stale && tryTakeover(lockPath, mkClaim())) {📝 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
| const claim: LockClaim = { pid: process.pid, startedAt: now(), nonce }; | |
| const deadline = now() + MAX_WAIT_MS; | |
| let vanished = 0; | |
| for (;;) { | |
| if (tryCreate(lockPath, claim)) { | |
| try { | |
| return run(); | |
| } finally { | |
| release(lockPath); | |
| } | |
| } | |
| const held = readClaim(lockPath); | |
| if (held === undefined) { | |
| // Released between our create attempt and the read — go straight back for it. Bounded: if `tryCreate` | |
| // keeps failing while the file keeps reading as absent, the filesystem is refusing the lock (a read-only | |
| // directory, a mount without real `O_EXCL`) and no amount of retrying will change that. Spinning there | |
| // would hang first-run startup, so a handful of attempts is enough to conclude it and reconcile instead. | |
| vanished += 1; | |
| if (vanished <= MAX_VANISHED_RETRIES) { | |
| continue; | |
| } | |
| return runReconciling(run); | |
| } | |
| const stale = held.unreadable === true || now() - held.startedAt >= STALE_MS; | |
| if (stale && tryTakeover(lockPath, claim)) { | |
| const lockPath = `${dbPath}.migrate.lock`; | |
| // Stamped per ATTEMPT, not once: a waiter may poll for up to MAX_WAIT_MS before it acquires, and a | |
| // pre-wait `startedAt` would let a third process presume our live migration stale early. | |
| const mkClaim = (): LockClaim => ({ pid: process.pid, startedAt: now(), nonce }); | |
| const deadline = now() + MAX_WAIT_MS; | |
| let vanished = 0; | |
| for (;;) { | |
| if (tryCreate(lockPath, mkClaim())) { | |
| try { | |
| return run(); | |
| } finally { | |
| release(lockPath); | |
| } | |
| } | |
| const held = readClaim(lockPath); | |
| if (held === undefined) { | |
| // Released between our create attempt and the read — go straight back for it. Bounded: if `tryCreate` | |
| // keeps failing while the file keeps reading as absent, the filesystem is refusing the lock (a read-only | |
| // directory, a mount without real `O_EXCL`) and no amount of retrying will change that. Spinning there | |
| // would hang first-run startup, so a handful of attempts is enough to conclude it and reconcile instead. | |
| vanished += 1; | |
| if (vanished <= MAX_VANISHED_RETRIES) { | |
| continue; | |
| } | |
| return runReconciling(run); | |
| } | |
| const stale = held.unreadable === true || now() - held.startedAt >= STALE_MS; | |
| if (stale && tryTakeover(lockPath, mkClaim())) { |
…review The blocker was real and I could not patch around it. ADR-0073's lock-file protocol cannot provide mutual exclusion: the takeover's `rename` fixes only the file's final contents, so two takers each `rename` and then each read back their own nonce and BOTH enter the critical section — my "atomic swap" comment claimed the opposite. And `release()` unlinked unconditionally, so a superseded holder could delete the new owner's lock and admit a third process. Breaking a stale lock with plain file operations is not fixable; the ADR itself had already named the better mechanism and rejected it only for lacking a Node binding. So the lock is now a real advisory lock, taken through the driver we already have: `BEGIN EXCLUSIVE` on a dedicated `<db-path>.migrate.lock` SQLite database. The kernel releases an fcntl lock when the process dies, however it dies — which deletes the staleness threshold, the poll interval, the pid field and the entire takeover protocol rather than tuning them, and retires the negative about `finally` not surviving SIGKILL. Separate file, because taking EXCLUSIVE on the database drizzle is about to migrate would deadlock it against our own connection. ADR-0073 carries a dated append-only note; the decision to serialize at all, and why BEGIN IMMEDIATE cannot, are unchanged. Writing the new tests exposed a flaw in my own seam: the function overwrote the injected connection's `busy_timeout`, so the mutual-exclusion test sat through the real 10 s wait. The timeout is now injectable and the suite runs in 13 ms. `createClient` — both findings valid. The four PRAGMAs and the Drizzle bind sat OUTSIDE the try, so a failure there (a corrupt header surfaces on the first `journal_mode` write, not on open) leaked the connection and escaped as an untyped driver error, past both the typed-error contract and `openLocalDb`'s cleanup and at-rest hardening. And the message interpolated `err.message`, which for better-sqlite3 routinely contains the absolute path — putting back the OS username that moving the path onto `.path` was meant to remove. Now: everything inside the try, close on failure, a stable generic message, the driver error preserved via `cause`. Docs and naming, all verified stale before changing: `chat-mode.ts` / `chat-mode-host.ts` referenced a `target` field that the rename to `unredactedPreview` had left behind; `tool-registry.md`'s canonical interface omitted the field entirely (now documented, with the never-serialize contract); `current.md` still reserved ADR-0073+ when 0073 and 0074 are used; `project-structure.md` said five shared packages when its own diagram and table list six. Also: the two unsafe `as DbOpenError` casts became `instanceof` narrowing, the three single-code retry tests became one parameterized case with each code's reason written down, and `persister.ts`'s `session:turn_completed` arm — whose cognitive complexity my atomicity work pushed from 15 to 18 — extracts a named `commitTurn`. `pnpm run ci` → exit 0, verified by exit status: 4,830 tests, 23/23 + 7/7 tasks. Refs: ADR-0073, ADR-0050, #99, #91, #104 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/decisions/0073-history-db-migration-lock.md`:
- Around line 7-27: The ADR still presents the removed OS-lock protocol as
normative after the amendment. Add a clearly labeled current-decision section
immediately after the amendment describing the SQLite BEGIN EXCLUSIVE lock on
<db-path>.migrate.lock, then mark the later old Decision, constants, considered
options, and negative-consequence sections as superseded historical context
without deleting them.
🪄 Autofix (Beta)
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: e67728fe-15cf-4d89-823c-a5aa61c6482f
📒 Files selected for processing (13)
apps/cli/src/chat/chat-mode-host.tsapps/cli/src/chat/chat-mode.tsapps/cli/src/chat/persister.tsdocs/decisions/0073-history-db-migration-lock.mddocs/project-structure.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mdpackages/db/src/client.test.tspackages/db/src/client.tspackages/db/src/migrate-lock.e2e.test.tspackages/db/src/migrate-lock.test.tspackages/db/src/migrate-lock.tspackages/db/src/retry.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/project-structure.md
- docs/roadmap/current.md
- apps/cli/src/chat/chat-mode-host.ts
- packages/db/src/client.test.ts
- apps/cli/src/chat/chat-mode.ts
- docs/reference/shared-core/tool-registry.md
…weak tests The ADR's amendment note said the lock-file protocol was replaced, but the body still read as normative below it: the `'wx'` acquisition, the constants table, the staleness/pid handling and the `rename` takeover. A reader who skipped the note would implement the wrong mechanism. There is now a `## Decision — current (2026-07-30)` section immediately after the note stating the SQLite `BEGIN EXCLUSIVE` lock and why it is sound where the file protocol was not; the original section is retitled SUPERSEDED with a "do not implement from this" banner, and the negatives carry a note saying which of them (stale lock, threshold tuning, `finally` vs `SIGKILL`) no longer apply. Nothing deleted — the corpus is append-only, and the rejected-alternatives reasoning is still the record of why a lock was chosen at all. Two test smells, both verified and both real: - `suspend.test.ts` used a bare `return` to skip on Windows, which REPORTS A PASS — so the suite claimed to have verified SIGTSTP/SIGCONT registration on a platform where the assertions never ran. Now `ctx.skip()`, which records it honestly. - Two `on_exceed:warn` tests in `session-host.test.ts` had no assertions at all. They exercised "does not throw" implicitly, so they would have passed just as happily if admission stopped being granted altogether. Each now asserts the actual outcome — the egress is still ADMITTED, and `release()` does not throw. `pnpm run ci` → exit 0. Refs: ADR-0073 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/src/chat/session-host.test.ts (1)
992-996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
admissionbefore callingrelease()
toBeDefined()doesn’t narrow the next line, so both warning tests should use an explicit non-undefined guard and then calladmission.release()directly.🤖 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/session-host.test.ts` around lines 992 - 996, In both warning tests at apps/cli/src/chat/session-host.test.ts lines 992-996 and 1007-1011, explicitly guard the admission returned by wiring?.preEgress(OVER_CAP) before releasing it; retain the defined assertion, then conditionally call admission.release() directly rather than using optional chaining inside the no-throw assertion.
🤖 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 `@docs/decisions/0073-history-db-migration-lock.md`:
- Around line 33-36: Update the rationale near “Why it is sound where the lock
file was not” to describe BEGIN EXCLUSIVE as SQLite’s platform-neutral exclusive
locking mechanism; remove the fcntl-specific claim and, if implementation
details are needed, acknowledge that SQLite uses platform-specific primitives
such as fcntl on Unix and LockFileEx on Windows.
---
Nitpick comments:
In `@apps/cli/src/chat/session-host.test.ts`:
- Around line 992-996: In both warning tests at
apps/cli/src/chat/session-host.test.ts lines 992-996 and 1007-1011, explicitly
guard the admission returned by wiring?.preEgress(OVER_CAP) before releasing it;
retain the defined assertion, then conditionally call admission.release()
directly rather than using optional chaining inside the no-throw assertion.
🪄 Autofix (Beta)
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: 5289f177-a0e4-43d0-9ccc-609670d26a42
📒 Files selected for processing (3)
apps/cli/src/chat/session-host.test.tsapps/cli/src/render/suspend.test.tsdocs/decisions/0073-history-db-migration-lock.md
Comment on lines
+33
to
+36
| - **Why it is sound where the lock file was not.** `BEGIN EXCLUSIVE` takes a real `fcntl` write lock, and the | ||
| kernel releases it when the holder dies — however it dies. There is therefore **no staleness to detect and no | ||
| lock to break**, which is what made the original protocol unfixable: breaking a stale lock with plain file | ||
| operations cannot be mutually exclusive. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' docs/decisions/0073-history-db-migration-lock.mdRepository: HodeTech/Relavium
Length of output: 16349
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('docs/decisions/0073-history-db-migration-lock.md')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 28 <= i <= 40 or 140 <= i <= 190:
print(f"{i:>4}: {line}")
PYRepository: HodeTech/Relavium
Length of output: 5305
🌐 Web query:
SQLite locking uses fcntl on Unix and LockFileEx on Windows advisory lock BEGIN EXCLUSIVE better-sqlite3
💡 Result:
SQLite implements advisory file locking using native operating system mechanisms to coordinate access across processes [1][2]. On Unix-based systems, it uses POSIX advisory locking via the fcntl system call [1][3]. On Windows, it utilizes the LockFile or LockFileEx system calls [1][4]. These locks are advisory, meaning they rely on all participating processes to cooperate; if a process ignores these locks and accesses the file directly, database corruption can occur [1][5][2]. In the context of better-sqlite3 and general SQLite usage, the transaction modes determine when these locks are acquired [6][7]: 1. BEGIN DEFERRED (default): No locks are acquired at the start of the transaction [1][7]. A SHARED lock is acquired upon the first read, which may be upgraded to a RESERVED lock upon the first write attempt, and finally to an EXCLUSIVE lock during commit [1][7]. 2. BEGIN IMMEDIATE: Immediately acquires a RESERVED lock [7]. This allows other connections to continue reading, but prevents other connections from starting a write transaction, thereby avoiding deadlocks that might occur if a DEFERRED transaction attempts to upgrade to a write lock [6][7]. 3. BEGIN EXCLUSIVE: Immediately acquires an EXCLUSIVE lock, preventing any other connection from reading or writing to the database until the transaction finishes [6][8][7]. Using BEGIN EXCLUSIVE is generally more restrictive than BEGIN IMMEDIATE [8]. While better-sqlite3 provides helper methods for these modes (e.g.,.immediate and.exclusive) [9], the underlying locking behavior is controlled by SQLite itself, not the Node.js wrapper [10]. In environments where multiple connections write to the same database, using IMMEDIATE or EXCLUSIVE is a common strategy to handle concurrency and prevent SQLITE_BUSY errors [6][8].
Citations:
- 1: https://sqlite.org/lockingv3.html
- 2: https://sqldocs.org/sqlite-locks/
- 3: https://www.sambaiz.net/en/article/572/
- 4: https://github.com/sqlite/sqlite/blob/master/src/os.h
- 5: https://sqlite.org/draft/howtocorrupt.html
- 6: https://sqlite.org/lang_transaction.html
- 7: https://dev.to/lovestaco/explicit-locking-deadlocks-and-linux-lock-primitives-in-sqlite-1llj
- 8: https://sqlite.work/performance-impact-of-begin-exclusive-vs-begin-immediate-in-sqlite/
- 9: https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md
- 10: Throws immediately on locked database instead of waiting WiseLibs/better-sqlite3#836
🏁 Script executed:
rg -n "BEGIN EXCLUSIVE|LockFileEx|fcntl|SQLite|better-sqlite3|advisory lock" docs packages apps -SRepository: HodeTech/Relavium
Length of output: 50374
🏁 Script executed:
rg -n "LockFileEx|fcntl|BEGIN EXCLUSIVE|advisory lock|exclusive lock|SQLite" docs packages apps -SRepository: HodeTech/Relavium
Length of output: 50374
🌐 Web query:
SQLite locking Unix fcntl Windows LockFileEx BEGIN EXCLUSIVE advisory locking documentation
💡 Result:
SQLite manages database file locking through an internal abstraction layer known as the Virtual File System (VFS) [1][2]. The locking mechanism is designed to support concurrent access while ensuring data integrity [3][4]. Unix and Windows Implementation SQLite employs platform-specific system calls to manage advisory locks on database files [3][4]: - Unix: The default VFS utilizes POSIX advisory locks via the fcntl system call [3][5]. SQLite maintains internal state to manage these locks and only invokes fcntl when transitioning between lock states [5][6]. - Windows: SQLite utilizes the LockFile, LockFileEx, and UnlockFile system calls [7][3]. On Windows NT/2000/XP and later, LockFileEx is used to support reader/writer locking [7]. Locking Hierarchy SQLite uses five levels of locking, defined by integer values in the VFS interface, ranging from least to most restrictive [8]: 1. NONE (0): No locks are held [8]. 2. SHARED (1): Multiple connections may hold SHARED locks simultaneously for reading, but no connection may write [3][4][9]. 3. RESERVED (2): A process intends to write to the database. Only one RESERVED lock can exist, but it can coexist with SHARED locks [3][4][9]. 4. PENDING (3): The process holding this lock wants to write and is waiting for all existing SHARED locks to clear. It prevents new SHARED locks from being acquired [3][4][9]. 5. EXCLUSIVE (4): Required for writing to the database file. No other locks of any kind can coexist with an EXCLUSIVE lock [3][4][9]. Transaction Behavior and BEGIN EXCLUSIVE The command BEGIN EXCLUSIVE initiates a transaction that immediately attempts to acquire an EXCLUSIVE lock on the database [10]. Unlike the default (DEFERRED) transaction mode—which delays lock acquisition until the first read or write operation—BEGIN EXCLUSIVE prevents any other connection from reading or writing to the database from the moment the transaction starts [10]. Blocking and Concurrency By default, SQLite's locking is non-blocking at the OS level; if a lock cannot be acquired, SQLite typically returns SQLITE_BUSY [3][11]. However, on some systems, SQLite can be configured to use blocking locks (e.g., via the SQLITE_ENABLE_SETLK_TIMEOUT compile-time option), allowing processes to wait for a lock to become available [11][12]. Additionally, in WAL (Write-Ahead Logging) mode, concurrency is significantly improved as readers do not block writers, and writers do not block readers [3][11].
Citations:
- 1: https://www.sqlite.org/c3ref/io_methods.html
- 2: https://sqlite.org/vfs.html
- 3: https://a1.sqlite.org/lockingv3.html
- 4: https://sqlite.org/lockingv3.html
- 5: https://github.com/sqlite/sqlite/blob/d46beb06aab941bf165a9d216d08207ab6c1440d/src/os_unix.c
- 6: https://github.com/sqlite/sqlite/blob/master/src/os_unix.c
- 7: https://github.com/sqlite/sqlite/blob/master/src/os.h
- 8: https://www.sqlite.org/c3ref/c_lock_exclusive.html
- 9: https://sqlite.org/atomiccommit.html
- 10: https://www.sqlite.org/lang_transaction.html
- 11: https://sqlite.org/src/doc/tip/doc/wal-lock.md
- 12: https://chromium.googlesource.com/external/github.com/sqlite/sqlite.git/+show/HEAD/doc/wal-lock.md
Use platform-neutral SQLite locking terminology. BEGIN EXCLUSIVE is the cross-platform SQLite exclusive lock; fcntl is only the Unix implementation detail, while Windows uses LockFileEx.
🤖 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/0073-history-db-migration-lock.md` around lines 33 - 36,
Update the rationale near “Why it is sound where the lock file was not” to
describe BEGIN EXCLUSIVE as SQLite’s platform-neutral exclusive locking
mechanism; remove the fcntl-specific claim and, if implementation details are
needed, acknowledge that SQLite uses platform-specific primitives such as fcntl
on Unix and LockFileEx on Windows.
…our missing amendment notes
Two required actions the ADR was missing, both verified against the code before changing anything.
**A false premise in its own Negative.** It claimed "current readers already require
forward-compatible handling of unknown event types". They do not: `RunEventUnionSchema` is a
`z.discriminatedUnion('type', …)` and `loadRunEvents` parses EVERY row through it, so an unknown
`type` THROWS rather than being skipped — one `budget:estimate_committed` in the log makes the
whole run unreadable to an older binary, not merely partially understood. That turns a mild
deploy-ordering caveat into a one-way door, so implementing §2 now also requires deciding the
reader posture (tolerant read that drops unknown types, versus an explicit refusal to downgrade).
The Negative says so, and says the event must not be emitted until that is settled.
**The amendment notes.** The title claims it amends ADR-0028, 0036, 0045 and 0070, but none of
the four carried a note pointing back — which `docs/decisions/README.md` step 5 and
`documentation-style.md` §7 require, and which is how the corpus stays navigable in both
directions. Each now has a dated note naming precisely what 0074 changes for it and what it
leaves intact, and stating that 0074 is still Proposed so the note records the amendment rather
than a settled behaviour change.
Not changed: the decision itself, which reads sound — three separate quantities, an estimate that
never masquerades as realized spend, a frozen money basis for a submitted media job, and the same
guarantee on both surfaces. `Status: Proposed` stands; it is the maintainer's ruling.
Refs: ADR-0074, ADR-0028, ADR-0036, ADR-0045, ADR-0070
Co-Authored-By: Claude <noreply@anthropic.com>
The `maxRetries` option was forwarded only when a caller supplied it
(`...(deps.maxRetries === undefined ? {} : …)`), and production never does —
`providers.ts` constructs all four adapters bare. So the Anthropic and OpenAI SDKs' built-in
retry (2 more attempts) was live inside the adapter, which pre-empts `FallbackChain`: the chain
never saw the first 429, failover was delayed by the SDK's own backoff, and a rate limit looked
like a slow call rather than a reason to move to the next provider. ADR-0011 is explicit that the
runner, not the adapter, owns retry policy.
Now always passed, `?? 0`, so production is explicit rather than implicit. A caller that wants
the SDK's behaviour can still opt in.
The reason this survived so long is worth recording: every existing adapter test passes
`maxRetries: 0` itself, so none of them ever exercised the production default. The two new tests
deliberately omit it and assert EXACTLY ONE network attempt on a 429. Break-verified — restoring
the conditional makes the Anthropic case fail, and it takes 1356 ms doing it, which is the SDK's
backoff being measured.
Refs: ADR-0011, ADR-0040, #276
Co-Authored-By: Claude <noreply@anthropic.com>
…6 regression) The Opus round on 67f982d found a real regression I introduced, and I verified its premise directly: with the vendor SDKs' internal retry correctly disabled, the shipping chat and one-shot `agent run` surfaces had NO retry left at all. `agent-session.ts` built the primary entry with `maxAttempts: 1`, the default chat agent declares no `fallback_chain`, and at a budget of 1 the chain's own guard (`attempt < budget`) is false — so `#backoff` is unreachable and the chain never even waits. A transient 429 ended the user's turn instantly. The SDK's retry had been quietly absorbing that the whole time, which is why nothing caught it. The reviewer's sharper point, also verified: #279 alone would NOT have fixed this. `Retry-After` and jitter live in `backoffDelayMs`, reached only from `#backoff`, reached only when `attempt < budget` — dead code on a single-attempt plan. The required trio is #276 + #279 + a primary budget. Reading ADR-0040 made the fix more surgical than the review proposed. Raising the budget on BOTH sites would multiply the authored node-retry budget on the workflow path, which is precisely what that ADR exists to prevent — and the workflow path does not need it, because `WorkflowEngine` retries above the chain there. Only the SESSION path lacks that loop, which ADR-0040's own comment already said. So `SESSION_PRIMARY_MAX_ATTEMPTS = 2` applies to `agent-session.ts` alone; `agent-runner.ts` stays at 1. ADR-0040 carries a dated append-only note: its above-chain-budget decision holds for workflows and did not hold for sessions, why that was invisible until #276, and that A.2's deferred authored primary `max_attempts` field still stands. One regression test, break-verified: two scripted turns where the first fails retryable and the second succeeds must be consumed by ONE session turn. At budget 1 it closes `stopReason: 'error'`; at 2 it closes `'stop'`. Refs: ADR-0040, ADR-0011, #276 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/decisions/0040-node-retry-budget-above-the-chain.md`:
- Line 19: Fix the amendment blockquote in the decision document by removing the
blank line or prefixing it with `>` so the blockquote remains valid and passes
Markdownlint MD028.
In `@docs/decisions/0045-async-media-job-loop-poll-checkpoint-resume-cancel.md`:
- Around line 7-8: Normalize blank-line separators within the amendment
blockquotes to satisfy Markdownlint MD028: update
docs/decisions/0045-async-media-job-loop-poll-checkpoint-resume-cancel.md lines
7-8 after the ADR-0074 note,
docs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.md lines 7-8
before ADR-0042, docs/decisions/0028-workflow-resource-governance.md lines 7-8
before ADR-0044, and docs/decisions/0040-node-retry-budget-above-the-chain.md
lines 7-19 closing the ADR-0074 note. Prefix each separator with “>” or remove
the blank line; no content changes are needed.
In `@docs/decisions/0070-durable-per-model-session-cost-attribution.md`:
- Around line 10-13: Move the ADR-0074 amendment note out of the Related
(secondary) metadata list in the document header. Keep ADR-0024 and all
subsequent related links within the complete metadata field, placing the
amendment after the metadata block or before the Related entry begins.
In `@packages/core/src/engine/agent-session.ts`:
- Around line 1214-1224: Update the `#resolvePlan`() JSDoc above the
FallbackPlanEntry definitions to document that the session primary entry uses
SESSION_PRIMARY_MAX_ATTEMPTS, currently 2, instead of stating maxAttempts is 1.
Keep the existing workflow retry-budget documentation unchanged.
🪄 Autofix (Beta)
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: 81a54f0d-11d9-4b5d-b379-34d8cba1958b
📒 Files selected for processing (13)
apps/cli/src/render/suspend.tsdocs/decisions/0028-workflow-resource-governance.mddocs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.mddocs/decisions/0040-node-retry-budget-above-the-chain.mddocs/decisions/0045-async-media-job-loop-poll-checkpoint-resume-cancel.mddocs/decisions/0070-durable-per-model-session-cost-attribution.mddocs/decisions/0074-durable-conservative-budget-commitments.mdpackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.ts
💤 Files with no reviewable changes (1)
- apps/cli/src/render/suspend.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/decisions/0074-durable-conservative-budget-commitments.md
Comment on lines
+1214
to
+1224
| // ADR-0040 makes node retry the engine's ABOVE-chain budget, and the primary entry does not consume it. | ||
| // But a SESSION has no such loop (this comment used to end there, with `maxAttempts: 1`), so on the chat | ||
| // and one-shot `agent run` surfaces the chain was the ONLY place a retry could happen — and with a budget | ||
| // of 1 the guard `attempt < budget` is false, so `#backoff` is unreachable and nothing retried at all. | ||
| // That was masked until #276: the vendor SDK's own retry was silently absorbing transient 429/5xx here. | ||
| // With the SDK's retry correctly off, a bare 429 would fail the turn outright with no wait, so the primary | ||
| // carries a minimal budget of 2 on this path. The WORKFLOW path deliberately stays at 1 (see | ||
| // `agent-runner.ts`) because the engine retries above it there — raising both would multiply the authored | ||
| // budget. Recorded as a dated amendment on ADR-0040. | ||
| const entries: FallbackPlanEntry[] = [ | ||
| { provider: primary, model: agent.model, maxAttempts: 1 }, | ||
| { provider: primary, model: agent.model, maxAttempts: SESSION_PRIMARY_MAX_ATTEMPTS }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale retry-budget documentation.
The #resolvePlan() JSDoc immediately above this block still says the primary entry uses maxAttempts 1, but this implementation now uses SESSION_PRIMARY_MAX_ATTEMPTS (2). Update that comment so the documented session contract matches the code.
🤖 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 1214 - 1224, Update
the `#resolvePlan`() JSDoc above the FallbackPlanEntry definitions to document
that the session primary entry uses SESSION_PRIMARY_MAX_ATTEMPTS, currently 2,
instead of stating maxAttempts is 1. Keep the existing workflow retry-budget
documentation unchanged.
…(lane c, G0) Found by the lane-(c) audit. `wireJobControl` installs a SIGTSTP listener at construction, but `dispose` removed only the SIGCONT one — so every rebuild of the session left another suspend handler behind. A `/clear`, a `/model` reseat and the onboarding→Home handoff all rebuild, so one Ctrl-Z eventually ran several handlers and Node warned about a listener leak. The audit agent left this one-line fix uncommitted in the working tree. I did not take it on faith: I read the original `dispose`, confirmed `removeSuspendListener` was only reachable from the suspend flow, and pinned it with a test using the harness's existing `removeSuspend` spy — dispose must call it exactly once, and a Ctrl-Z after disposal must reach nothing. Break-verified: removing the line turns that test red. Refs: G0, #50 Co-Authored-By: Claude <noreply@anthropic.com>
…st of the #276 review `maxRetries: 0` was applied to ONE client shared by every surface, but ADR-0011's "the runner owns retry policy" only reaches calls the runner actually wraps. Two do not: - the async media-job poll — a transient fault on ONE status poll of a multi-minute, ALREADY-BILLED video job throws straight to the engine, which settles the node `provider_unavailable` (the authored `retry` block has no default, so `#shouldRetry` returns false). Highest-cost loss of the original change. - `listModels` — `boundedListModels` has a timeout and no retry, so one 500 fails a provider's live discovery (`/models refresh`, provider onboarding). `createClient` now takes a budget and those two pass `OFF_CHAIN_MAX_RETRIES = 2`. The chain-governed `generate`/`stream` stay at 0. The constant's doc says why the ADR-0011 rule does not reach them, so this reads as a principle rather than an exception. Also folded, all verified before changing: - The value is floored (`Math.max(0, Math.trunc(…))`). `AnthropicAdapterDeps`/`OpenAiAdapterDeps` are public exports and a negative made the SDK's loop UNBOUNDED — `retriesRemaining - 1` stays truthy at -1, so it recursed forever at the 8 s backoff cap. - `llm-provider-seam.md` still said the SDKs are used "for typed event parsing and retry plumbing". That is the canonical home, and a future reader would have concluded the SDKs handle retry and reintroduced #276 — exactly the failure mode this workstream has a task about. - The four conformance factories hard-coded `maxRetries: 0`, which is *the reason #276 survived*: the only cross-provider parity gate never ran the production configuration. Dropped, so it does now. - Gemini's compliance is real but ACCIDENTAL, so it is now written down at the transport: `@google/genai` only retries if `httpOptions.retryOptions` is supplied, which this adapter never does — plus the two traps (never pass that option; `client.interactions`/`.webhooks`/`.agents` default to `maxRetries: 2`). Refs: ADR-0011, ADR-0045, ADR-0064, #276 Co-Authored-By: Claude <noreply@anthropic.com>
…#279) A rate limit is the one case where the provider knows better than we do — it says when its window reopens — and `FallbackChain` was ignoring that and retrying on its own curve, either hammering early or waiting longer than needed. `LlmError` gains an additive optional `retryAfterMs`: a plain number, so no vendor header type crosses the seam. Both adapters populate it from either header shape (`retry-after-ms`, and `retry-after` in both delta-seconds and RFC 7231 HTTP-date form). `#backoff` prefers it over `backoffDelayMs`. Carried through OpenAI's key-redacting re-wrap too — dropping it there would have silently discarded the header on exactly the custom-endpoint path that most needs a polite retry. **No jitter, per your ruling and ADR-0040.** That ADR pins this backoff deterministic — no jitter, never `Math.random` — so a replay reproduces the same schedule, and `retry.ts` follows the same convention. #279 asked for jitter as well; the two cannot both hold, so the thundering-herd risk is accepted and DOCUMENTED at `#backoff` rather than silently traded away: parallel branches that hit one provider's limit together will retry together. Revisiting it is an ADR-0040 amendment. The header is untrusted input, so the normalizer is defensive rather than trusting: a non-numeric, negative, past-dated or absurd value is dropped, not honoured — a hostile `retry-after: 999999999` would otherwise park a turn for ~31 years. `undefined` strictly means "no instruction", never "wait zero", which would have turned the backoff into a tight retry loop. Eight tests. Break-verified: making `#backoff` ignore the requested value turns the preference test red, and the fallback test pins that an absent header still produces the 100/200 curve. Refs: ADR-0040, ADR-0011, #279 Co-Authored-By: Claude <noreply@anthropic.com>
…y call site (#194, #198) Two money-path defects, and both had the same shape: a real failure looking like a benign one. **#194 + 2.6.Q's realized-cost half.** `#emitSuccess`'s catch was bare, so it swallowed every exception from `costTracker.record` — not just the `UnknownModelError` its own comment documented. A genuine defect in the money path (a bad `Usage`, a broken overlay, a throwing custom tracker) silently produced "no cost" and was indistinguishable from an unpriced model. It now rethrows anything that is not `UnknownModelError`, because a money bug must be loud. That exposed the second half: "no cost" was itself ambiguous between *could not price* and *genuinely free*. A strict cap cannot be enforced over spend it cannot see, so `AttemptRecord` gains `priced?: false` — set only on the could-not-price path, absent otherwise so nothing changes shape for existing consumers. It is the in-flight twin of ADR-0070's durable `unpriced_calls` counter and ADR-0071 §K7's degrade-and-notify posture. **#198.** `CostTracker.record` folded whatever it was handed straight into the cumulative total. A `NaN` there is not merely wrong, it is ABSORBING: once folded in, every later `cumulative > cap` comparison is false and the cost cap silently stops being a cap. The seam schema is the first line of defence, but the tracker is also handed to custom hosts, so the invariant is asserted where the arithmetic happens. `Number.isSafeInteger` rather than `>= 0`, because past 2^53 the total is already inexact before any cap reads it. Ten tests. Both break-verified: removing the `instanceof` guard turns the non-pricing-failure test red, and removing the assertion turns the bounds table red — including the case that documents the poisoning, which asserts the total is untouched by a rejected value. Also removed three scratch test files a review agent left in `packages/llm/src/adapters/` — one of them was breaking typecheck. Refs: ADR-0070, ADR-0071, #194, #198, 2.6.Q Co-Authored-By: Claude <noreply@anthropic.com>
…nitize two command sinks Folding the two review passes. Both confirmed findings are things I introduced or missed. **BLOCKER — my own #276 fold opened a worse hole than it closed.** Giving `listModels` and the media-job poll `OFF_CHAIN_MAX_RETRIES = 2` looked right (nothing else retries them), but the SDK's retry sleep is neither abort-aware nor ceiling-bounded, and the SDK parses `Retry-After` ITSELF — so the seam's `RETRY_AFTER_CEILING_MS`, written for exactly this, never runs. A gateway answering one status poll with `retry-after-ms: 999999999` parked the poll for ~11.6 days, ignoring the user's cancel and holding a run slot and an event-loop timer; `base_url` is user-configurable (ADR-0065), so that is reachable. `/models refresh` had the milder version: a cancel at 50 ms hung 15 s and was then misreported as a timeout. Both reverted to no SDK retry, with the residual written down rather than hidden: these paths genuinely have no retry now, and the fix is a retry WE own — abort-aware and ceiling-bounded — which is the follow-up. An unbounded retry is worse than none. **Retry-After now CLAMPS instead of dropping.** Dropping an over-ceiling value looked safer and was strictly worse: a provider legitimately asking for 120 s fell back to our own curve and retried after ~250 ms — hammering the endpoint that had just asked us to back off, far harder than honouring a capped 60 s. Only genuinely uninterpretable values (non-finite, negative) still mean "no instruction". **Two unguarded sinks from the lane-(c) security review** — the one Wave 1 required and that had never been done. `status.ts` and `gate-list.ts` print `human_gate:paused.message` with zero sanitization. That is `resolveTemplate(node.message_template, …)` — interpolated upstream node output, the most model-controlled string in the product — and it is the SAME field `clack-prompter.ts` sanitizes. The review confirmed by EXECUTING the real commands against a real DB that OSC 52 clipboard writes, OSC 8 `file://` hyperlinks, CSI 2J and bidi overrides all reached the terminal intact. Lane (c) hardened four files; the class needed the shared boundary, so it stopped one call-site short. The review also certified the helper itself: 44 hand-built payloads (OSC 8/52/0/2, DCS/APC/PM/SOS, 8-bit C1 introducers, RIS, DECALN, bare CR, backspace, the full Trojan-Source bidi family, and three strip-then-reassemble constructions) — zero survivors, all idempotent, because pass 2 deletes 0x1B unconditionally so no ESC can survive under any input. Truncation ordering is correct too: every bound sanitizes before bounding. Break-verified: reverting the sanitize call turns the new gate-list test red. Refs: ADR-0011, ADR-0045, ADR-0065, #276, #279, G34, #57 Co-Authored-By: Claude <noreply@anthropic.com>
…; pin the Retry-After wire Two confirmed findings from the second review pass. **My justification for leaving `agent-runner.ts` at `maxAttempts: 1` did not hold.** I reasoned the workflow path was covered because the engine retries above the chain — but that budget only exists when a node AUTHORS `retry:`: `#shouldRetry` returns false for `retry === undefined` and `RetrySchema.max` has no default. So every workflow node without a `retry:` block had no retry above the chain AND none inside it (at a chain budget of 1 the guard `attempt < budget` is false, so even the backoff is unreachable). The #276 regression was live there too. The primary is now 2 when unauthored and 1 when authored — never both, which would multiply the author's budget (`retry.max: 3` becoming up to 6 real calls), exactly what ADR-0040 prevents. My first cut of that check was itself wrong and two existing e2e tests caught it: "authored" is `node.retry ?? agent.retry`, matching `Engine#retryConfig` (ADR-0040 A.8) — an AGENT-level `retry:` also gives the engine a budget, so checking only the node double-counted it. That is what those tests are for, and they did their job. ADR-0040's note is extended rather than rewritten, and says plainly that its first reading was wrong to leave the workflow path wholly at 1. **The `retryAfterMs` adapter→chain wire was completely unpinned** — the review severed it on BOTH adapters and all 718 llm tests stayed green, even though the feature demonstrably works in production. Now pinned with real SDK 429s carrying real headers, on both adapters, plus the absent-header case (absent must mean "no instruction", never 0). Break-verified: severing it turns exactly those two tests red. Refs: ADR-0040, ADR-0011, #276, #279 Co-Authored-By: Claude <noreply@anthropic.com>
…tize the third render leaf
**ADR-0074 is Accepted.** The review was right that it was an incomplete decision, and the
substance of the fix is that its open question answers itself.
§5 (new) settles it: **the read boundary tolerates an unknown event `type` and drops it, while a
KNOWN type with an invalid body still fails loud.** That is not a new choice — it is the contract
`sse-event-schema.md` §Forward-compatibility already states ("provided consumers ignore unknown
`type`s and unknown fields"). The code is the deviation, and it predates this ADR. So the work §2
depends on is *fixing a pre-existing doc↔code contradiction*, not weakening a guarantee to make
room for a new event, and the one distinction that keeps ADR-0050's durability posture intact is
unknown-discriminator versus unparseable-body.
The review also caught that I had only named `RunEventUnionSchema`. Verified: `SessionEventSchema`
is a `discriminatedUnion` too and `RunOrSessionEventSchema` is a `z.union` of both, so **all three
throw** — and the event is dual-envelope, so all three need the tolerant read. §5 says so, and says
the seam is a `safeParse` at the read boundary, never a `.catch()` at the call site.
Two more findings folded. A conservative commitment was described as cap-consuming "until a future
provider protocol" can reconcile it — an indefinite block, which is a worse failure than the
overspend it prevents. It is now bounded twice: it dies with its owning run/session, and within a
live owner the user can release it deliberately, which is an explicit decision about their own money
like raising the cap. What stays forbidden is the system silently deciding the estimate was wrong.
And the event's shape now points at its canonical home rather than being half-specified here.
Dropped the "remains seam-pure" consequence: no new dependency was never in question, so the line
asserted nothing.
Separately, the third render leaf: `createPlainRenderer` (the no-TTY/CI half of "one seam, three
renderers") now sanitizes at its SINGLE write point rather than per field, so an arm added later
inherits the guard. Lane (c) hardened four files; this class needed the boundary. Break-verified.
Refs: ADR-0074, ADR-0028, ADR-0036, ADR-0045, ADR-0050, ADR-0070, G34, #57
Co-Authored-By: Claude <noreply@anthropic.com>
…the run/gate path) The last open code item in Wave 1's lane (c). `driveRun` registered only SIGINT, ink hides the cursor while it renders, and `signal-exit`'s signal list contains no SIGTSTP — so a Ctrl-Z during `relavium run` or `relavium gate` returned the user to a shell with a permanently invisible cursor, for the rest of that terminal's life rather than just the run's. Deliberately NOT `wireJobControl`. That primitive serves the full-screen chat/Home surfaces: it releases and reclaims the alternate screen and mouse reporting and owns a suspend hatch. This path enters none of that (`RunApp` uses no `useInput`, so the terminal stays cooked and Ctrl-Z is a real signal), so reusing it would release state that was never entered. The security review's audit narrowed the harm to exactly this: no mouse-reporting leak, no retained alt screen — a hidden cursor, plus a clack gate prompt `fg` cannot revive. So the fix is the narrow pair: show the cursor before we stop, hide it again when foregrounded and ink resumes drawing. It re-raises rather than swallowing the signal — a Ctrl-Z that silently did nothing would be worse than the bug. `dispose` removes BOTH listeners, which is the leak shape `suspend.ts` itself had, and is idempotent because the caller's `finally` also runs on the throw path. A no-op where job control does not exist, rather than half-wired. Four tests, injectable lifecycle so no real signal is raised. Break-verified: dropping the show-cursor write turns two of them red. Refs: G0, #50 Co-Authored-By: Claude <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. Four file-disjoint lanes, developed in parallel and consolidated here as four
--no-ffmerges so each lane's history stays a readable unit.Closes M2.5.5-1.
registry.tscf3a5d0packages/dbchaincf3a5d09668afa,0a2323c642b7d6Decisions 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.
landed with no ADR reference because its record was still an untracked draft; it amends ADR-0028's
estimate-and-block cap, so rule 9 wants it. Renumbered from 0073, which was already taken.
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.
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.
verified they merge clean and the gate is green, but I did not review their diffs against their
acceptance paragraphs.
current.md's Wave 1 items and thephase-2.5.5.mdbullets are still open, pending lane (c)/(d) sign-off.
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
history.dband config file permissions (self-heal to0600).