feat(2.6.5): W1 — the eight P0 blockers, closing with consent before a local MCP spawn (ADR-0084) - #83
Merged
Conversation
Phase 2.6.5 status moves to in progress with a Progress block naming the six closed items, their closure dates, and the three gaps carried forward rather than implied: ADR-0077's unbuilt required regression (without it `#runAttempt`'s money-durability arm is unreached), `CR-10`'s property being inexpressible from the durable log alone, and the oracle's three remaining debts to `CR-92`. `current.md`: the Wave 1 closure is now merged on both PRs (#81 on 2026-08-09, `#W15-1` + the first 2.6.5 batch via #82 on 2026-08-11), the ledger node is marked complete in the graph, and the 2.6.5 section carries a live 6-of-46 count pointing at `CR-10` as next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fore the ADRs A seven-agent audit re-verified every W1 Evidence claim against the tree before any ADR is written. Most held; seven did not, and two would have produced an ADR that was wrong on its first pass. **CR-14's Failure paragraph is FALSE and inverts where the defect is.** All three shipped adapters already detect a no-terminal EOF and emit a classified `transport` error — anthropic.ts:843, openai.ts:1301, gemini.ts:935. The gap is a missing trust boundary at `FallbackChain` for FOREIGN providers (cassette, scripted, Phase-2's gateway) plus the chain's own `usage === undefined => succeeded`. The enforcement point moves to the chain; the adapters become defence in depth. **CR-12's key cannot work as written.** `nodeAttempt` + `toolCallId` in the identity means every retry and resume mints a new key, so tier 1's "safe retry under the same key" is unreachable — and it contradicts action-guard-seam.md:109, which already says the node-retry attempt is NOT the idempotency key. Two identities are needed. Three of the five components are also unobtainable today (`runId` is absent from NodeExecContext and from AgentSession by design; the attemptNumber that reaches dispatch is the within-chain counter). Two scope claims were wrong: the `tool` NODE type is unimplemented (three dispatch sites, not four), and an MCP tool cannot be assigned a tier at all — no annotations are parsed, and they would be attacker-controlled if they were. **CR-13: half of ADR-0062 §1's rejection ground was already false when written.** Anthropic's `mergeAdjacentSameRole` landed 2026-06-14, three weeks before ADR-0062, and closes the "two consecutive user messages" hazard at the seam. The superseding ADR does not merely offer a better alternative. **CR-16's lazy connect is structurally blocked** by ADR-0052 §3 — the registry has no mutation API and MCP ToolDefs exist only as a product of connect-time `listTools()`, so deferring the spawn deletes the tool grant. Split out with its blocker named; consent-before-spawn alone satisfies the written Acceptance. **CR-17's "key reference plus version" is not implementable** — ADR-0006 defines no version concept and `maskInputs` emits a self-reference, not a keychain ref. `sse-event-schema.md:247` already states the false version. **CR-10**: the quoted code comment does not exist verbatim, and out-of-order commit is NOT reachable on the CLI's synchronous store on the happy path — it needs a SQLITE_BUSY yield or an async store. Also recorded: this item does NOT retire ADR-0074's sum-vs-max fold, which is driven by seq-ASSIGNMENT order. **CR-92**: "handle resolution" does not locate on the terminal path; only the media reclaim does. `#emitDurable`'s totality must survive for non-terminal events or ADR-0077's barrier argument rots. `reconcile()` is a second write path that bypasses the choke point entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n helper `scripts[call] ?? []` handed an overrun call a SILENT EMPTY stream, which the chain reads today as a successful zero-usage attempt — so a test that invoked the provider more times than it scripted passed for a reason it never stated. This lands before `CR-14` deliberately. Under that item's rules 5 and 6 the same shape becomes a classified error, so leaving the fallback in place would have shown a wall of unrelated red inside CR-14's own PR with real regressions hidden in it. Matches the existing precedent verbatim — `m2-e2e-harness.e2e.test.ts:102` and `m5-chat-harness.e2e.test.ts:77` already index directly and throw. All 46 tests still pass, so nothing was relying on the fallback. Refs: CR-14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…al outbox
Closes CR-10 and CR-92 of phase 2.6.5, and establishes the durable-write seam
CR-11 and CR-12 extend rather than re-break.
Eight sections. The ones that carry weight:
The engine serializes each run's appends into one tail, and the mechanism is a
one-line move — `await prior` goes ABOVE the persist, so the single existing tail
serializes ask, write and delivery in that order. A second `#persistTail` is
rejected: two chains over one ordering property is how they drift.
The media de-inline stays OUTSIDE that region (an unbounded host round-trip
inside it would block every later write including the terminal); the media
reclaim moves AFTER a successful terminal persist (a terminal whose write failed
must not have already released the run's references). Two calls twenty lines
apart, stated separately so a reviewer does not conflate them.
`persistEvent` takes one extensible `DurableWriteContext`. The store's guard is a
`SELECT max(seq)` inside the existing IMMEDIATE transaction through the `tx`
handle — no migration, no snapshot regen.
The terminal outbox lives outside the store behind a required host port, per the
maintainer's ruling: a row in the same history.db is unavailable for most of the
fault class it exists to survive, and no outbox at all means reconcile() writes
`run:failed{internal}` for a run that completed. It is drained BEFORE
reconciliation — across processes that order is what stops the same divergence
being reintroduced — and a drained entry whose run already has a terminal is
dropped, not appended.
`durability: 'durable' | 'uncertain'` is minted once, at the handle and never on
the RunEvent (the store persists the delivered event verbatim, so a live-only
field either lands on disk or forces the two forms to diverge). CR-11 and CR-14
reuse it.
Totality is preserved for non-terminal events — ADR-0077's B1/B2/B3 argument
rests on it and both money events are non-terminal.
Two limits stated rather than papered over: "durable" here means process-crash,
not power-loss (`synchronous = NORMAL`); and CR-10's headline property is not
provable from the log alone, because streamed events consume sequence numbers and
are never persisted — it needs the store harness that lands before the
implementation.
And what this does NOT change: ADR-0074's sum-vs-last-wins and the Math.max
checkpoint fold survive untouched. They are driven by seq-ASSIGNMENT order among
concurrent emitters, which an ordered append tail cannot change.
Refs: ADR-0078
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oven with Lands BEFORE CR-10's implementation, exactly as CR-91's durable-truth oracle preceded the spine. Asserting the spine with an instrument the reviews already found insufficient is the failure this ordering exists to prevent. **Why a log assertion cannot do this.** CR-10's property is "no persisted event is missing from the middle", and that is not expressible from the durable log alone: the run's sequence numbers are shared with streamed events (`agent:token`, `cost:updated`, …) which take a number and are never persisted, so a healthy completed run reads [0,1,2,3,5,10,…] and a lost event is byte-identical to one that was never meant to land. The witness has to come from the ask side, which only a store decorator holds. `createAppendAudit` wraps a RunStore and records, per run, the ask order, the commit order and each ask's outcome. Three predicates, deliberately separate: - **PREFIX** — committed events in sequence order must be an unbroken leading run of the asked events. A cut tail is legitimate (a crash truncates); a hole is the defect. - **ASK ORDER**, independently — a synchronous store commits in order even when the engine issued its writes concurrently, which is exactly the pre-CR-10 state. Without this predicate the whole item would look green on `better-sqlite3`. - **COMMIT ORDER** — what an async store actually did with the asks it accepted. Vacuity checked as ADR-0078 requires: mutating the prefix predicate to a SET comparison turns exactly the three hole-detecting tests red, so the prefix is what does the work. Restored after measuring. Exported from packages/core on the same terms as `checkDurableTruth` — CR-92's acceptance has to be certified in apps/cli against the real history.db store. Scoped per run SEGMENT, not per runId for all time: a resume re-seeds the bus from the durable maximum, so a resumed leg legitimately starts far above 0 and a caller wraps the store again for the second leg. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-10 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…defect it exists for 41 agents, six lenses, every finding adversarially verified before it was acted on; 17 stood, the rest were refuted. Two mattered. **The harness had no predicate for the property ADR-0078 §1 establishes — and its docblock claimed one did.** §1's property is "ask N+1 is not ISSUED until N has SETTLED". `askOrderViolations` cannot express that: the engine assigns the sequence number and starts the persist with no await between them, so its asks go out in sequence order whether or not they overlap. Measured: run the pre-CR-10 emit shape against a synchronous store — which is what `better-sqlite3` is, and which this ADR itself established commits in order — and the prefix, ask-order and commit-order predicates ALL verdict HOLDS. CR-10's own acceptance clause, "break-verify by restoring the concurrent start", would have gone green against the instrument built to prove it. That is the exact failure the oracle-before-the-spine ordering exists to prevent, reproduced one level up. `overlapViolations` now states the property directly: an ask issued for a run while a prior ask for that run is still in flight, captured at ask time because a settled record looks identical either way. Break-verified — disabling it reddens the concurrent-start test and nothing else. Three tests added, including the negative one (two runs writing concurrently is legitimate; the append is ordered per RUN, and scoping the check globally would fail every parallel run). **A test was hollow and a second asserted nothing about its own title.** The dual-event test survived deleting the guard it names — its assertions observed the per-run filter, not the guard. The inner-rejection test asserted only `holds`/`holes`, which are byte-identical whether the rejection was recorded or dropped. Both now assert the thing they claim. Docs: ADR-0036 gained the dated `> Amended` back-pointer every prior amender of it carries. ADR-0078's Related said "Closes CR-10 and CR-92" on an ADR whose every mechanism is unimplemented — the precise false-completion shape the phase document warns about twice; it now says "Decides … implementation staged". §1's "the awaited total is unchanged" was false for a concurrent emitter and contradicted the ADR's own first Negative; scoped. §8 now names the `run-history-store.ts` comment CR-10 must rewrite — it justifies the telescoping delta by out-of-order commit, the very thing §1 removes, so landing the tail would leave the money write's stated reason contradicted by the code beside it. Also: `durable-truth.ts` claimed CR-10 for itself and carried no pointer to the harness the append-audit docblock says it points to; corrected to the ORDER half only. `records`/`mutable` were byte-identical duplicate arrays whose hand-copied type literal was the only reason two widening `as` existed — one array now, with the mutable view derived by a mapped type. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-10 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… own predicate
Second pass over the same step, aimed at what the first round missed and at the
fold itself. Five findings stood, one refuted. The first round of 41 agents did
not find the one that mattered.
**A fault hook that THROWS orphaned its record at `pending` forever.**
`AppendFault` is typed to RETURN an Error, but a store double that throws is the
obvious way to write one — and `better-sqlite3` throws synchronously for real.
The hook sat outside the try/catch, so such a throw escaped before `outcome` was
set. Every LATER ask on that run then read the orphan as still-in-flight and
reported a false overlap: the predicate this harness exists for would have fired
on a correctly ordered engine, failing the very implementation it is meant to
certify. One try/catch now covers the hook and the write. Break-verified —
restoring the old shape reddens exactly the new regression and nothing else.
**The docblock overclaimed, and this round measured it.** It said the pre-CR-10
engine overlaps "on every event". It does not: every `#emitDurable` call site
awaits and `#emitDurable` awaits its own region, so an ordinary sequential run
overlaps NOTHING — asked and committed both `[0,1,2,3,6,7,8,9,10]`, zero
overlaps. It fires under genuine concurrency: a `max_parallel: 2` fan-out
produced exactly one, "sequence 10 (node:completed) was asked while [9] was still
in flight". That is clause 4 — record rather than assert — and the corrected
paragraph now states what was measured.
**The measurement is now a test, not a claim in a comment.** Two e2e cases drive
a live WorkflowEngine over an audited store: the sequential baseline (zero
overlaps, `committed === asked`) and the fan-out baseline. The fan-out assertion
is written to FLIP when CR-10 lands, and says so — flipping it from
`toBeGreaterThan(0)` to `toEqual([])` IS the acceptance the phase document asks
for ("break-verify by restoring the concurrent start"), and deleting it instead
would remove the only end-to-end evidence the ordered tail changed anything.
ADR-0042 gets the dated back-pointer its §4 was owed — ADR-0078 §1 re-times the
terminal media sweep to after a successful terminal persist, and the previous
fold gave that note to ADR-0036 only. ADR-0078's title and index row now name
both. Also renumbered two predicate comment blocks that both read "3." after the
overlap predicate was inserted between them.
`pnpm run ci` and `pnpm coverage` green.
Refs: ADR-0078, CR-10
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements ADR-0078 §1–§3. The engine's asks for one run are now serialized, the store refuses an append that would leave a hole, and `reconcile()` — the second write path — carries the same guard. **§1 is one line, as the ADR said.** `await prior` moves ABOVE the persist inside `#emitDurable`, so the single existing tail serializes the ask, the write and the delivery instead of only the delivery. Below it the previous event's write had been STARTED but not joined, so two events for one run overlapped in flight and nothing but the store's timing kept the log a prefix. **§2's guard is `DurableWriteContext`, one extensible object.** It lives in `@relavium/shared` rather than beside the port, because `@relavium/db` implements that port and depends only on shared — the dependency runs one way. `CR-11`'s fencing token and `CR-12`'s journal correlation extend the same object rather than breaking `persistEvent` twice more. `expectedLastSequenceNumber` is the last sequence ASKED, not the last committed, and that difference is the guard. After a lost write the engine keeps running (§6 totality), so reporting the last SUCCESSFUL sequence would make an append that skips the lost one match — creating exactly the hole. The store evaluates it with `max(seq)` INSIDE the existing IMMEDIATE transaction through `tx`: no migration, no snapshot regen, and — verified — a refusal rolls the derived rows back with it. `InMemoryRunStore` enforces the identical guard, because a reference that accepts what the real store rejects makes every core test prove nothing. The terminal is exempt, and that is a named hole, not an oversight: exactly-one-terminal (ADR-0036) outranks the guard, and a terminal the store will not take is CR-92's outbox to own. Resume seeds the guard from the checkpoint's `lastSequenceNumber` — left at `-1` a resumed leg's first append would claim the log was empty and be refused, breaking resume outright. **The acceptance flipped a test rather than adding one.** The fan-out e2e case landed one commit ago asserting `overlapViolations.length > 0` — the measured pre-CR-10 baseline — and said in its own comment that flipping it IS the acceptance. It went red on this change and is now `toEqual([])`. Break-verified the way the phase document words it: putting `await prior` back below the persist reddens it again. Exactly one test moved; the other 1154 in `packages/core` stayed green, which is the evidence that serializing the append perturbed no interleaving a test legitimately pins. Also folds ADR-0078 §8's doc obligation: the telescoping `run_costs` comment justified itself by out-of-order COMMIT — the thing this commit removes — so it would have read as obsolete the moment the tail landed. The real reason is stamp-time capture (ADR-0077), which the ordered tail does not touch. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-10 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uard switched off 41 agents, six lenses, every finding adversarially verified; 18 stood. The reorder itself survived — measured end to end on a max_parallel:3 fan-out, asks came out 0..15 with the belief exactly the previous durable seq every time, no deadlock on any path, and ADR-0077's totality argument intact. What did not survive is the scaffolding around it. **The append-audit harness dropped `ctx`.** Its decorator re-spelled `persistEvent`'s signature by hand, so when ADR-0078 §2 added the parameter it silently kept compiling and forwarded only the event — every store driven through the instrument built one commit earlier to certify CR-10 ran with CR-10's guard OFF. Now typed as the port's own member, so CR-11's fencing token and CR-12's journal correlation cannot be dropped the same way. The belief is recorded on `AppendAskRecord` too: a WRONG belief is now visible from the ask side rather than only as a store rejection. **Nothing observed the engine side of the guard.** Both mutations passed the whole suite: dropping the ctx entirely (the guard disconnected from the durable write path) and guarding the terminal too (ADR-0036's exemption removed). A recording-double test now pins that every non-terminal ask carries the previous ask's sequence, the first believes `-1`, and the terminal is unguarded. Break-verified. **CR-10's acceptance clause was undischarged.** "A crash injected between the two must leave a prefix with no hole" now has an end-to-end test: one `node:completed` write is dropped mid-fan-out and the sibling's append is REFUSED rather than landing past the gap. Also added: `reconcile()`'s guard — the second write path this item exists to close — had no test at all, and the store's per-run scoping was untested (dropping `where(runId)` passed all 307 db tests, while its in-memory twin had exactly that case). **Two claims of mine were wrong and are corrected in place.** The db test said the guard is "unreachable through the engine once the ordered tail is in place"; measurement says the opposite — after any lost write the engine's very next guarded ask is a holed one, because §6 keeps it emitting. And the port comment presented the optional `ctx` as ADR-0078 §2, which specifies a required one; it now records the deviation, why it differs from §4's required outbox port, and the residual risk it leaves. The `#emitDurable` docblock still said "Persists stay concurrent; only delivery is serialized" — the exact sentence ADR-0078's Context quotes as the defect. ADR-0078 §8's two §6 re-derivations landed. `database-schema.md` §"Concurrency & transaction behavior" — the canonical home the ADR names — now describes the compare-and-append, including that it is deliberately NOT in the retryable set. Also recorded rather than left implied: the ordered tail made ADR-0074 §2's ordering assertion hold with or without the money barrier, so that test no longer pins the barrier. It is kept for the tail, and the note says where the barrier's own half is pinned instead — which matters, because this commit's predecessor offered "the other 1154 tests stayed green" as evidence. CR-10 is marked closed in the phase document with the code that closes it, and with the two things it does NOT close: the terminal exemption (CR-92's outbox), and end-to-end certification against the real history.db (rides with CR-92). `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-10 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t test what I said Second pass over CR-10 including the Opus fold. The reorder and the store guard survived a sixth independent attempt to break them — six properties mutated, each caught by an existing test. What did not survive is two tests the previous commit message specifically credited, and a claim about the type system. **The `reconcile()` guard test was hollow, and its own comment named a scenario it did not build.** It called `reconcile()` twice and asserted the second produced nothing — which it does, but because `listInterruptedRuns` already excludes a run carrying a terminal, so the second call never reached the guarded write at all. Two reviewers independently verified by mutation: removing ADR-0078 §3's guard from `reconcile()` entirely left all 1165 `packages/core` tests green, this one included. It now drives two CONCURRENT reconciles against one still-interrupted run — both read the same belief, only one can be right — and asserts exactly one repair landed. Break-verified: the mutation reddens it. **The acceptance test asserted something the injected fault satisfied by itself.** "A lost non-terminal write makes the engine's next ask fail closed" checked `rejected.length >= 1`, and the deliberately-dropped write IS a rejection — so it passed whether or not the guard refused anything downstream. It now requires at least two, and asserts the prefix property directly over the segment CR-10 covers: every non-terminal committed before the first miss, none after it. The terminal landing past the miss is asserted too, rather than hidden — that is precisely the residual CR-92's outbox closes. **And a claim about TypeScript that is not true.** The previous commit said typing the harness decorator as the port's own member means CR-11's and CR-12's context fields "cannot be dropped the same way". Structural assignability lets a fewer-parameter function satisfy a wider signature, so a decorator that ignores them would still compile — contextually typed or not. What actually closes the hole is the forwarding regression that asserts on what the inner store RECEIVED. Corrected in place under clause 4. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-10 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… says so
Implements ADR-0078 §4 and §5, and closes the two things CR-10 deliberately left
open.
**The defect.** `#emitDurable` is total for store faults, so a terminal whose
write failed was still delivered — a caller drained `run:completed` with outputs
while the durable log had no terminal at all, and nothing in the API could tell
the two apart. Reconciliation would later write `run:failed{internal}` for that
run, relabelling the divergence rather than closing it and losing the outputs.
**The outbox lives outside the store, in a separate file**, per the maintainer's
ruling. The store that must hold a refused terminal is the store that just
refused it: a full disk, a corrupt history.db or an exhausted busy-retry fails an
outbox row for exactly the reason it failed the terminal. `apps/cli` writes NDJSON
beside history.db at 0600, appending rather than rewriting — the process writing
there has already demonstrated a write can fail, and a rewrite that dies
mid-truncate would lose every other run's entry. Compaction happens on `remove`,
when the process is healthy again by definition.
The port is REQUIRED on ExecutionHost, and it broke the CLI wiring on the first
typecheck — which is the behaviour that makes it worth requiring. The optional
precedent there (`mediaStore?`) is absent-tolerant because a text-only host has
no media; there is no legitimate host with no terminal durability.
**`RunHandle.durability`** is `'pending' | 'durable' | 'uncertain'`, at the
handle and never on the RunEvent — the store persists the delivered event
verbatim, so a live-only field would either land on disk (self-contradictory, the
row existing IS the durability) or force delivered and persisted to diverge.
CR-11 and CR-14 reuse this vocabulary rather than minting their own.
**The drain runs BEFORE reconciliation**, and the order is the point: a crashed
process leaves its terminal in the outbox, and reconciling first would see a run
with no durable terminal, conclude it needs repair, and write `run:failed` for a
run that completed. A drained entry whose run is no longer in
`listInterruptedRuns` is DROPPED, never appended — the original may have
committed with only its acknowledgement lost, and replaying would break
exactly-one-terminal from the path that exists to restore it. The drain uses only
the three methods `RunStore` already declares, so a Phase-2 cloud store can
implement it.
**The media reclaim moved after a successful terminal persist** (re-timing
ADR-0042 §4). It ran at the emit, so a terminal whose write then failed had
already released the run's media references — the outbox could retry the terminal
into a log whose media was gone.
Four tests, break-verified: mutating `'uncertain'` to `'durable'` reddens two of
them. The negative control is there too — a terminal that lands reports `durable`
and holds nothing — because without it the headline assertion passes for a handle
that reports `uncertain` unconditionally.
`pnpm run ci` and `pnpm coverage` green.
Refs: ADR-0078, CR-92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…chable on the shipping surface 51 agents. The engine wiring survived every attempt to break it — measured, not read: the reclaim runs exactly once on success and never on the refused path; `#emitDurable` stays TOTAL even with BOTH the store and the outbox throwing, with zero unhandled rejections; a cancelled run whose terminal fails reports `uncertain` and is held exactly like a completed one; and the drain's already-terminal DROP is what stops a blind replay. The gate-parked question I flagged has a reassuring answer, and it is worth recording: an outbox entry only exists because a terminal was EMITTED, so the run demonstrably settled — `resumable` describes only what the durable log looks like, and after a failed terminal write it lies. Skipping such a run the way `reconcile()` skips resumable ones would leave it resumable forever, and a later `gate approve` would re-execute it into a second, DIFFERENT terminal plus duplicate effects. Appending is correct, and the drain running before the resumable skip is what makes it hold. **What the review actually found is that none of it was reachable.** `createFileTerminalOutbox` had ZERO production call sites — every real CLI run got the in-memory reference that, by its own docblock, survives nothing, in a one-shot process. The maintainer chose a separate file over an in-database row precisely for fault isolation, and that choice was unrealized. Both `run` and `gate` now wire the real path (`~/.relavium/terminal-outbox.ndjson`, exposed from the history opener so the two commands cannot drift onto different files). **And the file implementation had no test — the half specifically chosen.** Eleven now, and the first one written found a real bug: a process killed mid-append leaves a partial line with no terminator, and appending straight onto it CONCATENATES — one corrupt line that swallows the NEXT entry as well as the truncated one. So a crash during the very write this outbox exists to survive would have cost the following terminal. Each entry is now written with a leading newline as well as a trailing one; the reader already skips blank lines, so it costs a byte and needs no read of the existing file, which matters on a path that has just seen I/O fail. The other cases pin what the implementation claims: newest-wins is last-wins on READ (both lines verifiably on disk), a valid-JSON-but-invalid-RunEvent line is REJECTED rather than becoming a fabricated terminal, `remove` compacts without touching other runs, the file is created 0600 like history.db, nothing throws when the directory is gone or the file unreadable, and a C1/bidi payload round-trips — the line is re-read by a later process that then persists what it finds. Still open from this review and carried to the Sonnet round: nothing reads `handle.durability()` yet, so `relavium run` still exits 0 on an uncertain terminal; the terminal remains exempt from the append guard while a comment says that hole closes here; and the end-to-end certification against the real history.db is not written. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-92 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…exemption is decided Closes the three items the Opus review left open on CR-92. **`relavium run` no longer exits 0 on a terminal that was not recorded.** Nothing read `handle.durability()`, so the headline sentence of CR-92's acceptance — "if durability is uncertain the API must not say completed" — was unmet on the only shipping surface. `outcomeToExitCode` now takes the disposition and lets `'uncertain'` OUTRANK the outcome, on both the `run` and `gate` paths. Exit code 5, and deliberately neither 0 nor 1: the run may well have COMPLETED — the outputs are in the delivered terminal — and only its durable record is missing, so reporting failure would be as wrong as reporting success. A script seeing 5 should treat the run as done-but-unrecorded and re-check `relavium status` after a later invocation, by which point the outbox drain has retried it. The taxonomy extension landed in `commands.md`'s exit-code table, which an existing test pins byte-for-byte — that test caught the omission before I did. **The terminal exemption is now DECIDED, not deferred.** The code said the hole "closes when CR-92 lands"; CR-92 has landed and the exemption stays, so that sentence had become false rather than pending. The original reason is genuinely gone — §4's outbox gives a refused terminal a home — but guarding it would convert the COMMON case (a non-terminal write was lost, so the belief no longer matches the log) into a run whose terminal is refused and only lands at the next `reconcile()`. That trades a run that ends correctly-but-with-a-hole for one that does not durably end at all, on the failure path. The residual is stated rather than hidden, including that `checkDurableTruth` and `createAppendAudit` deliberately disagree about such a log — which is the honest pair, since the run really did end and really did lose an event. Five tests on the exit mapping, including the negative control and the absent/pending cases — without them the headline assertions pass for an implementation that returns 5 unconditionally. Still open and carried to the Sonnet round: the end-to-end certification against the real history.db. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, ADR-0049, CR-92 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last item the CR-92 Opus review left open. Both were proven in
`packages/core` against `InMemoryRunStore` — which the engine ships as a
reference precisely so a core test needs no filesystem — but the guard that
matters is the one inside the SQLite store's IMMEDIATE transaction, and the
outbox that matters is the one that writes a real file beside a real database. A
reference implementation that agrees with the real one is a claim until someone
runs both. The phase document says in terms that this certification belongs here.
Four cases, all over an on-disk `better-sqlite3` database and an on-disk outbox
file — the same two objects `relavium run` wires:
- A full run commits a clean prefix with no overlapping asks, measured through
`createAppendAudit` wrapped around the real store.
- A stale append is refused with a typed `AppendConflictError` and writes
NOTHING — the rollback half matters as much as the refusal, since a partial
write would leave derived `runs`/`step_executions` rows with no event.
- A refused terminal is held in the real FILE outbox, the handle reports
`uncertain`, and the database really does lack the terminal.
- The drain retries it into the real store — read back through a FRESH outbox
object over the same file, so the handoff is proven to be the file rather than
in-process state — and the run ends `run:completed`, NOT relabelled `failed` by
reconciliation. Exactly one terminal, and the entry is forgotten once it lands.
That last one is CR-92's acceptance sentence ("live, history, resume and reconcile
agree") across a process-like boundary.
`pnpm run ci` and `pnpm coverage` green.
Refs: ADR-0078, CR-10, CR-92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aller, and remove raced a concurrent put Second pass over CR-92 and its three folds. The in-process half held up under adversarial reading and measurement. Two real defects, and the first was mine. **The whole retry path was unreachable from the shipping binary.** `reconcile()` — the only thing that drained the outbox — has zero production call sites anywhere in the monorepo. So the outbox, the drain, and the real-history.db certification I had just written were all dead code from `relavium`'s point of view: a user who saw exit code 5 had no command that would ever move their run to `durable`, while that exit code's documentation — added in this same range — asserted one would. That sentence was false as shipped, and it is exactly the class of claim this phase exists to remove. `drainTerminalOutbox()` is now public and both `run` and `gate` call it at start. Draining rather than wiring `reconcile()`: the drain writes only terminals the engine itself already produced, for runs whose log still lacks one, and switches on nothing else — whereas `reconcile()` also repairs every interrupted run, a far larger behaviour to turn on inside a CR-92 fold. That it remains unwired is now a known, separable thing rather than an accident hiding this one. **`remove` truncated the file and destroyed a concurrent `put`.** Reproduced against the shipped code: an append landing inside another process's truncate-then-write window vanished silently, with no error on either side — precisely the unrecoverable loss §4 exists to close, reintroduced by the compaction. Concurrent processes over one `~/.relavium` are designed for here (ADR-0073, ADR-0064 §5), not an edge case. Removal now appends a tombstone, so the file is append-only in BOTH operations — which is what its docblock always claimed; I had only broken it in `remove`. Three tests, including that a run put AGAIN after its tombstone is held again, which is what a second failed retry must produce. Also: the drain now runs the D11 media sweep its sibling `reconcile()` arm already did — the crashed process never ran its in-process reclaim, which is why the terminal is in the outbox at all, so without it the run's media references survive forever. And `gate` repeated the outbox path literal instead of consuming the opener's, contradicting an earlier commit message of mine that claimed the two commands "cannot drift"; there is now one exported `terminalOutboxPath`. Two gate tests caught the new wiring through a stub that lacked the method — the wiring did not find them. The review also settled the question I had flagged: a gate-parked run can never have an outbox entry, because an entry exists only if a terminal draft was built, which never happens for a currently-parked run. Better than the answer the first round gave. `pnpm run ci` and `pnpm coverage` green. Refs: ADR-0078, CR-92 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se and fencing token Decides CR-11, the third link of phase 2.6.5's durability spine. Implementation staged behind it. The gap: `resumeFromCheckpoint`'s cross-process check is an in-memory `this.#runs.has(runId)`, which is per-process by construction. Two `relavium` processes can resume the same paused run and become two independent side-effect producers. ADR-0078's compare-and-append stops the second one WRITING the same row; it does not stop either DOING the work, because the effect happens before anything is written. That is why CR-12's effect journal cannot mean anything until this closes — an idempotency key is worthless when two owners each believe they are alone. A `run_leases` row with a monotonic `generation`, not a column on `runs`: that row is a derived projection the event fold rewrites, so authoritative ownership state in it invites a fold to clobber it. The fence rides `DurableWriteContext` — which ADR-0078 §2 introduced as one extensible object precisely so the two items after it would extend rather than re-break `persistEvent`. Checked inside the same IMMEDIATE transaction as the append guard, through the same `tx` handle. A fresh run's lease is created in the same transaction as `run:started`, which sidesteps `start()` being synchronous: a fresh runId comes from `ids.newId()`, so the start-side lease is uncontended by construction and all contention is on the already-async resume path. **The maintainer's three rulings, recorded with their reasoning.** A process fenced out MID-RUN emits no terminal at all — writing `run:failed` would be a durable lie about a run another process may be finishing, and the fence would reject it anyway; it closes its local stream and reports `uncertain`, reusing exactly the vocabulary ADR-0078 §5 said CR-11 would. TTL 60s with a 20s heartbeat: three missed beats permit takeover, wide enough not to mistake a long provider call for death, narrow enough not to lock a crashed run for long. Expiry is compared store-side against the epoch-ms clock the store already has, so every process on the box uses one clock and the platform-free engine gains no second notion of time. And a lease loss gets exit code 6, distinct from the blanket EngineStateError → exit 2, because it is transient and retryable while every other one is a permanent invocation fault. `reconcile()` becomes lease-aware here rather than later: it writes a terminal for every non-resumable interrupted run, from a process that may not own it. Latent today, cheap now, a data-loss bug once any surface wires it. Scope stated rather than inferred: runs only (AgentSession keeps ADR-0070's recorded drift, and the asymmetry is named honestly); local only (Phase-2 Postgres gets its own mechanism, mirroring ADR-0073's scoping); ADR-0073 is precedent, not a component; ADR-0075 is cited, not superseded; ADR-0050's durability-first posture is unchanged because a stale-fence rejection is an expected refusal, not data loss. Two things deferred with their triggers: the observer handle (a typed refusal is in scope, tailing another process's log is not), and the fact that the two-process race cannot be proven in one Node process — `better-sqlite3` is synchronous, so the regression follows `migrate-lock.e2e.test.ts` and is visibly SKIPPED rather than silently passing when the build output is absent. ADR-0036 carries the dated back-pointer. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three stand and all three are cheap.
The Related field was the longest in the corpus as one inline `·` run; it is a
bulleted list now, following ADR-0078 rather than ADR-0075/0076. The review
suggested this for "the next ADR of this complexity" — but the improvement lands
directly on this one, so it lands here.
§1 said `generation` increments "on every successful acquire" without naming
`reconcile()`'s expired-lease takeover as one of them, which §7 introduces. It is
consistent in practice — reconcile IS acquiring — but the reader had to connect
two sections to see it. Named inline.
The phase document's CR-11 acceptance says the loser "degrades to observer with a
typed, actionable error", which reads as requiring the full observer now. ADR-0079
§4 splits that into two deliverables and scopes only the typed refusal in. The
phase doc now carries a note pointing at that scoping, so the acceptance cannot be
read as demanding a capability the decision deliberately deferred.
While there: §4 identified the in-scope half positionally ("only the second"),
which is correct but invites a misread of the very distinction the paragraph
exists to draw. Stated explicitly instead.
Refs: ADR-0079, CR-11
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ADR-0079 §1 and §6, db-side only. No engine change, so this lands on its own with the drizzle snapshot regenerated and `db:sync-check` green. A table keyed by `run_id`, not columns on `runs`: that row is a derived projection the event fold rewrites, so authoritative ownership state in it mixes two lifetimes in one row and invites a fold to clobber it. `generation` is the fence — monotonic, never reset, bumped on EVERY successful acquire including a renewal by the same owner. That last part is deliberate and tested: it means a caller must carry the token it was just handed rather than a remembered one, which keeps the fence strictly ordered. Four operations, each one `BEGIN IMMEDIATE`. A DEFERRED read-then-write would take a read lock first and lose the upgrade race under exactly the contention this exists for. `acquire` refuses only a DIFFERENT owner holding a LIVE lease — same owner is a renewal, expired is a takeover. `heartbeat` matches on (owner, generation), so a fenced-out owner discovers it lost from the update matching nothing, with no second query. `release` is scoped the same way, so a process that has already been fenced out cannot free the new holder's lease on its way down — a quiet failure that would otherwise let a third process in while the real owner is mid-run. Expiry is evaluated against the store's own injected clock (§6), never a caller-supplied time: a caller passes a TTL and nothing else, so it cannot widen its own lease by lying about `now`, and every process on the machine compares against one clock. `read` returns the store's verdict on liveness rather than data for the caller to re-derive. Nine tests. Break-verified — and the first attempt did NOT apply (prettier had collapsed the anchored condition onto one line), so the green it produced was meaningless; re-anchored, removing the live-lease refusal reddens exactly the test that names it. Refs: ADR-0079, CR-11 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ADR-0079 §2. `DurableWriteContext` gains `fence: { ownerId, generation }` and the
store checks it inside the SAME IMMEDIATE transaction as the append guard,
through the same `tx` handle. A check outside would be two statements another
process could interleave — the race it exists to close.
This is the field that justifies `DurableWriteContext` being an object at all:
ADR-0078 §2 introduced it as one extensible parameter precisely so the items
after it would extend rather than re-break `persistEvent`. This is the first of
them; CR-12's journal correlation is the second.
Two decisions worth stating. The fence is checked AFTER the append guard: both
refuse the same write, but a stale belief about the log is the more specific
diagnosis when a writer has both problems — and a fenced writer's belief is stale
precisely BECAUSE it was fenced. And a MISSING lease row is a rejection, not a
pass: the run was taken over and released, or the row was never created; either
way the writer cannot prove ownership and ADR-0079 fails closed.
`LeaseFencedError` is distinct from `AppendConflictError`, and the distinction is
what a caller acts on. An append conflict says "the log moved under you, your
belief is stale". A fence rejection says "you are not the owner any more, stop" —
including stopping short of the terminal, because the run's real outcome now
belongs to somebody else (§5).
Five tests, break-verified: disabling the fence check reddens exactly the three
that name it. The accept case and the no-fence case are the controls — the two
halves of the context are independent, so a caller holding no lease still gets
its ordering checked and vice versa.
Refs: ADR-0079, CR-11
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ires before it reads ADR-0079 §4. `RunLeasePort` is REQUIRED on `ExecutionHost` — same reasoning as `TerminalOutbox`: the optional media ports are absent-tolerant because a text-only host legitimately has no media, and there is no legitimate host with no run ownership. `resumeFromCheckpoint` acquires BEFORE it reads anything — not after the checkpoint, not after the identity guard. Every line below the acquire is work only the owner is entitled to do, so a loser must never become a second producer even briefly. The refusal names the current holder, which is what makes it actionable rather than "something else has it", and it carries the new `run_owned_elsewhere` code. That code is the first TRANSIENT `EngineStateError`, and the distinction now has a name in the type: `run_owned_elsewhere` means "somebody else is running this right now", which resolves on its own; every other code is a mistake in the call that will fail identically forever. A surface uses that to tell "try again shortly" from "never call this again". The engine's owner id comes from `host.ids.newId()`, not a process id: the engine is platform-free and has no notion of a process, and two engines in ONE process must still be distinguishable — otherwise the second would silently RENEW the first one's lease instead of being refused, which is the whole failure this closes. **Implementation found a real gap in §4 and the ADR now says so.** The lease is released when the process stops WORKING on the run — which includes a re-pause, not only a terminal. A sequential multi-gate test caught it: the run resumes, re-pauses at the next gate, and the very next `relavium gate` was refused for the full TTL by a lease nobody was using. A parked run has no process executing it, and ownership exists to stop two processes ACTING. Every refusal path releases what it just took, too, or a run that does not exist would lock its own id. `createRunLeasePort` adapts the store's synchronous operations to the `Promise`-typed seam — wrapping at the boundary rather than making the store async, so nothing in `run-history-store.ts` pretends to await. Both `run` and `gate` wire the DURABLE lease built from the same store the run persists to; the in-memory reference the host defaults to guards nothing across processes, which is correct for a fixture and wrong for a surface. Refs: ADR-0079, CR-11 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…and a loser that stops A process that loses its run mid-flight now stops without claiming an outcome it does not know (ADR-0079 §5), and keeps its claim alive while it works (§6). The heartbeat rides `ExecutionHost.setTimer` as ADR-0079 §6 chose, but the seam now carries a `TimerKind`. The distinction is load-bearing, not bookkeeping: a work timer (gate deadline, retry backoff, media poll) ADVANCES the run when it fires, while the beat advances nothing and re-arms itself forever. Conflating them broke twenty-one tests in ways that were all the same bug — a drive-to-quiescence loop that never terminates, an `armedCount()` that stops answering "is this run waiting on anything", and a counted-fault harness whose second arm was no longer the one it meant. In production the same split is why the CLI `unref`s a liveness timer and only a liveness timer: a work timer SHOULD hold the event loop open, and a perpetual beat must never be what keeps a process from exiting. Four defects found while proving it, each by a test that now pins it: - The beat was never disarmed on a NORMAL terminal — only on the fenced path — so every finished run left a timer re-arming itself and renewing the lease of a completed run for the life of the process. Ownership now ends with the run, after the terminal is durable (the order is forced: the terminal is itself fence-checked). The lease is released there too, so `run_leases` stops growing a permanent row per run. - `resumeFromCheckpoint` released the lease it had just taken, immediately, via a `#releaseIfIdle` that checked nothing — and `beginResume` returns when the resume is KICKED, not when it finishes. Every cross-process gate resume handed its lease back mid-run and was fenced out by its own release, stopping with no terminal and hanging its caller. Ownership is now given up at the two moments the process actually stops working on the run, both inside the execution where that is observable. - A fenced run was torn down via `#schedule()`, but `#step()` returns early only on `#settled` — so a fenced run with nothing runnable never reached `#settle` and its consumer's `for await` hung forever, which is the opposite of what §5 promises. `#settleFenced()` is now called directly at each discovery point. - `resume()` re-acquired unconditionally, bumping the generation out from under a cross-process resume's own in-flight claim — the hazard `resumeFromCheckpoint`'s own comment named. It now re-acquires only when the park actually released. Arming the beat is idempotent for the same reason. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… runs, and the fence is proven across processes Closes CR-11. `reconcile()` writes a terminal for every non-resumable interrupted run, from a process that may own none of them — latent today because nothing called it, a data-loss bug the moment something did. It now takes the run over first, and takes it over ATOMICALLY: `acquire` already refuses when a different owner holds a live lease, so the acquire IS the check. A `read` followed by an `acquire` would be redundant with that refusal and, worse, not atomic with it — the lease can change hands between the two. An expired lease is a takeover that bumps the generation, which is what fences the dead owner if it ever wakes. The outbox drain gets the same guard, and it is sharper there: the held event is a terminal a CRASHED process built from its own view of the run, so writing it into a run a live owner has since resumed would durably contradict the run that owner is finishing right now. The CLI maps the one transient refusal to exit code 6 (ADR-0079 §7), documented in the exit-code taxonomy's canonical home. Every other engine-state refusal is a mistake in the call that will fail identically forever; `run_owned_elsewhere` resolves on its own within a TTL. An automation loop has to be able to tell "retry shortly" from "never call this again", and one blanket code cannot say it. And the property that cannot be shown in one Node process is now shown in two. `better-sqlite3` is synchronous, so two in-process owners are serialized by construction and the state that matters — one process still BELIEVING it owns a run another has taken over — never arises. That belief is the entire reason ADR-0079 chose a fencing token over a bare CAS. Two spawned children over one real `history.db`, deterministic by handshake rather than by racing spawns: the second is refused while the first holds, and a fenced-out holder is refused its write while the new owner's lands. With the store's fence check removed, the stale owner writes — measured, which is what makes the test worth having. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… at each caller The Opus review round found a blocker three agents reached independently, and proving it exposed two more defects and one build hazard. **The blocker.** A gate park hands the lease back (ADR-0079 §4), but a parked run is not inert: its gate deadline, the run-level `timeout_ms` and cooperative cancel all stay armed by design, and all of them end in durable writes. Since a terminal is exempt from the append guard (ADR-0078 §2) and an ABSENT fence is a pass rather than a refusal, a parked process could write `run:cancelled` into a run another process was finishing — two terminals in one log, the exact divergence ADR-0079 exists to prevent, reintroduced by §4 itself. **The mirror of it, on the happy path.** `#emitDurable` delivers to consumers, and an inline prompter resumes synchronously on `run:paused`. Dropping the claim after the emit let that resume observe itself as still owning, skip its re-acquire, and then have the row deleted out from under it — a healthy run died `uncertain`. The claim now drops as part of the write that creates the pause. **The fix is one choke point, not a guard per caller.** `#emitDurable` is provably the run's only durable writer, so ownership is reconciled there: re-take a claim a park handed back, pass one already held, refuse once it is lost. Guarding call sites instead would have needed edits in six methods and still missed `#onRunTimeout`, which had the identical bug and no test at all — it is fixed here without a line changing in it. Two booleans became one five-state field because `!owned` could not tell "before the first acquire" (where `run:started` must write unfenced, the FK deviation) from "parked, must re-take", and that confusion WAS the bug: the run fenced itself out against the row it had released one event earlier. The hot path deliberately does not await. Making it unconditional inserted a microtask between `await prior` and the persist, which reordered ADR-0077's money barrier so a rejected ledger write was attributed to "a durable write failed" instead of the cancellation that actually stopped the run. **The reference store now enforces the fence**, which is what let any of this be caught: deleting the fence from the engine's write choke point previously left all 3,568 tests green. The lease port binds to the STORE, so two hosts over one store share one lease table — modelling one `run_leases` per `history.db` — and an injected port is bound too, since binding only the defaulted one left a fixture that supplied its own port running with enforcement silently off. `run_leases` also reached its canonical home in database-schema.md: the ER diagram, a table section, the count, and a concurrency-policy bullet for the fence beside the compare-and-append one. Two reviewers filed its absence as a CLAUDE.md rule 8 blocker. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d reconcile stops killing its own runs Three verified findings from the Opus round, each now pinned by a test that dies under mutation. `reconcile()` could terminate a run THIS engine is executing. `acquire`'s refusal rule is "a different owner holding a live lease", so for our own runs it is not a refusal at all — it is a renewal that bumps the generation, which fences the live execution out at its next write and then deletes its row in the caller's `finally`. The lease cannot tell the two apart because both claimants share an `ownerId`; the in-memory run table can, and it is the authority on what this process is running. Latent today (no surface calls `reconcile()` directly), which is exactly why it was cheap to fix now. `resumeFromCheckpoint` acquires before it reads the checkpoint, so every exit between there and `adoptLease` owns that claim — including the ones that return rather than throw. The already-terminal no-op held it, converting the documented idempotent re-delivery into a transient refusal (exit 6) for a full TTL over a run that finished hours ago, and leaving a `run_leases` row per re-delivery. `buildRunPlan` on an edited workflow and the checkpoint rehydration both throw outside the existing try and stranded it the same way. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… success Two surfaces reported a run taken over by another process as an ordinary, successful outcome. Both are one discriminator away from correct, and it costs nothing: ADR-0079 §5 closes a fenced run's stream WITHOUT a terminal, while ADR-0078 §5's outbox case delivers one. `relavium gate` printed "run X already settled; nothing to resume" and exited 0 for a resume that was fenced mid-flight. That is the worst available answer on the one path §5 was built for: the run is executing elsewhere, this process's gate decision never became durable, and an automation loop records success. It now raises the transient refusal, naming what happened and where the truth is. `relavium run` mapped a mid-flight fence to exit 5, whose documented remedy — "held in the outbox and retried on the next start" — is false for it: §5 deliberately writes nothing to the outbox, so a script following that advice waits for a drain that never comes. ADR-0079's own Consequences say exit 6 is what makes this case actionable; until now 6 was reachable only from a pre-resume refusal, never from the mid-run loss the sentence is about. Both exit codes' docs are corrected rather than left to be inferred: 5 is now scoped to "a terminal was PRODUCED and its write did not land", 6 covers both ways ownership is lost, and the canonical table says what to do differently for each. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tops describing what did not ship Three engine fixes, each pinned by a test that dies under mutation, plus the ADR corrections the conformance review asked for. The heartbeat's tolerance for a failed write was unbounded, which hid exactly the failure §6 names as its reason for existing: a persistently unwritable store means the lease provably expires, another process takes the run over, and this one keeps dispatching nodes and calling tools with no beat ever telling it. The tolerance is now bounded by the TTL itself — once the misses cover it, the claim is one this process can no longer prove, and §5's rule is that an unprovable claim stops. A single blip still costs nothing. `#beat` also re-checked the wrong thing after its await. A gate park during that suspension hands the claim back WITHOUT setting `lost`, so the beat read this process's own deliberate release as somebody else's takeover and killed a healthy parked run. It now re-checks that it still holds the lease. `#settleFenced` never released the budget governor's legacy media holds, which `#settle` discharges deliberately — a `checkPreEgress` awaiting a job that will now never settle would hang forever. A teardown whose whole job is to leave nothing behind was leaking on the newest failure path. And a fresh run whose acquire is refused now says what happened. That acquire is uncontended by construction, so a refusal means the host is misconfigured — a locked, unmigrated or read-only history.db — and settling on the generic default gave the user `internal: "the run failed"` pointing at nothing. ADR-0079 gains three dated amendments rather than silent drift. §1's "generation never resets" is not what shipped: `release` deletes the row, so what keeps a stale owner out is (ownerId, generation) pair-equality plus fail-closed-on-a- missing-row. The residual risk is named with its trigger — a long-lived host keeping one engine across many park/resume cycles cannot use the token alone to tell a straggler from the current leg — along with what fixing it would cost, so the trade can be re-taken with evidence. §3 records the FK-forced ordering and the unfenced `run:started` that follows from it. §4's post-Accepted paragraph is marked as an amendment and carries the two corrections implementation forced. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t stops drifting from src Two build-graph defects, both of which cost real time this branch. `@relavium/db`'s two-process regressions spawn children that import the BUILT package — a child cannot use vitest's source resolution. `test` depends on `^build`, which builds only UPSTREAM deps, and the required CI lane runs `test` before `build`. So on a clean checkout this package's own `dist` was absent and both files skipped: visible in the log, green to the job. ADR-0079's headline property — the one the ADR states cannot be proven in a single process — was therefore unproven in the gate that guards it, and the same was true of ADR-0073's migrate-lock race. Confirmed by the task graph: `@relavium/db#build` was not in it and now is. `*.tsbuildinfo` was not declared a build output. With `incremental: true`, tsc skips emit when that file says the outputs are current — so a cache restore that brought back `dist/**` without it, or a stale one left by a build that ran while files were mid-edit, yields a `dist` that silently does not match `src`, and `--force` does not defeat it. Measured here: `dist/engine/engine.js` was missing a field `src` had, across repeated forced rebuilds, and several hours went into chasing product bugs that were this. The CLI ships as a bundle of these outputs, so a desynced one is shipped code that never existed in source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ooled by a stale pause The Sonnet round found that commit 636f0c3 reopened the bug it fixed, and reproduced it against a real `driveRun`. `outcomeToExitCode` discriminated "was a terminal delivered" with `outcome === undefined` — but `run:paused` sets `outcome` too, and the engine buffers that event before it discovers a fence. An inline gate prompt therefore delivers a stale `run:paused` AFTER the loss, so the flagship two-terminal race reported exit 5 — whose documented remedy ("held in the outbox, retried on the next start") is false for a fenced run, since §5 deliberately queues nothing. `isTerminalOutcome`, defined two functions away and already saying a pause is not terminal, is the right test. `relavium gate` had the same defect and needed the opposite correction: keyed on the outcome it is wrong in BOTH directions — `undefined` misses the buffered case, and `!isTerminalOutcome` sweeps up a legitimate re-pause at a later gate that must still exit 3. There the durability is the discriminator, and the outcome decides everything else. Two more things a user actually sees. `relavium run` hitting a takeover printed nothing at all — the renderer falls through to a bare "run ended" because there is no terminal to summarise — while `relavium gate` explained it; both now do. And both the CLI message and the canonical exit-code table named `relavium status <runId>` as the remedy, which is not a command: `status` takes no argument and lists only ACTIVE runs, so it cannot show a run another process has already finished. The remedy is `relavium logs <runId>`. Finally, `schema.ts` and database-schema.md still asserted the generation is "monotonic, per run, never reset" — the claim ADR-0079's own 2026-08-17 amendment retracts. Amending the ADR without them left the canonical schema doc contradicting the ADR that governs it. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd the claims the docs were overstating The Sonnet round's test-integrity lens showed the whole `run_owned_elsewhere` pathway was untested end to end: forcing `isTransientEngineStateError` to return `false` left every test in the repo green, because the CLI's own tests prove only the CliErrorCode→exit-code table and never that anything PRODUCES the code. The classification is now pinned directly, positive and negative. `createCliHost`'s durable-store-without-a-lease-port guard was in the same position — its own docblock says that wiring "silently fences every run" and "looks like a hung run", and replacing the condition with `false` left all 2,396 CLI tests passing. An untested throw is a guard that can be deleted in silence. ADR-0079 §1's amendment turns on a numeric claim nothing asserted: that a release-then-reacquire by the same owner restarts the generation at 1. It is now pinned, deliberately, as a documented LIMITATION — if a future change makes `release` a tombstone, this is the test that should fail and be updated together with the amendment. Two comments of mine claimed more than they could support, and the state-machine lens disproved both by mutation. The park-ordering comment read as though the placement alone prevents the inline-resume race; moving the hand-off back AND injecting a delay to force the race left the suite green, because `#emitDurable` re-reads ownership at write time and `#schedule`'s single-flight guard keeps a post-gate dispatch from starting until the pause has unwound. The ordering stays — it makes the invariant true by construction rather than by two coincidences — but it is now described as defence in depth. Likewise the "no test dies when this arm is made permissive" note was scoped to the `done` ownership arm when the same is true of `lost` and of `#settle`'s guard, for the same deliberate reason: `#fence` is retained after ownership ends, so the store's transactional check refuses the stale token independently. Finally, `durability.e2e.test.ts` now says that `apps/cli` resolves the engine from `dist` — a bare `vitest` run there tests the last build, which was measured to hide a real break. Refs: ADR-0079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… effect contract Decides CR-12 and CR-95's short-term fix. An effect can complete at its target, the process can die before the result persists, and resume re-runs the node — duplicate ticket, deploy, payment, commit. ADR-0079 closed the precondition; this closes the effect itself. The phase document's proposed key cannot work: with the retry attempt and the tool-call id in it, every retry and resume mints a new value, so the journal can never dedup and "safe retry under the same key" is unimplementable. It also contradicts two canonical sentences that name the attempt as replay correlation and explicitly not the idempotency key. So this ADR does NOT claim a universal replay-stable identity — claiming one is the trap. Five concepts are named separately, because collapsing any two is where the design goes wrong, and the primary guarantee is a resume gate at node granularity whose lookup key is the correlation with the ATTEMPT DROPPED: the node-retry attempt resets to 1 on both a crash-resume and a budget approval, so an attempt-scoped lookup would miss the row it exists to find. The gate examines every prior effect record, not only unresolved ones. An earlier draft left the main crash window open: settle succeeds, the process dies before `node:completed` persists, and a gate that only checked unresolved rows waves the re-run through. A `committed` row is not a green light — if the journal did not retain enough to re-deliver the result, it blocks the node exactly as an unresolved one does. Every effect that ships today is tier 3: at-most-once dispatch attempt, never auto-retried. Tiers 1 and 2 are specified and occupied by nothing, and saying so is the point — the headline is a NARROWING of the product claim, not a new capability. MCP is permanently tier 3, not pending better metadata: an annotation from the server the hostile-MCP class defends against may never raise trust. Five ADRs are amended, none reversed. ADR-0077 is the addition: it predicted this journal belongs at its own per-TURN barrier, "rather than inside ToolRegistry.dispatch". That is right for the money ledger and wrong for the journal — a turn dispatches a LOOP of tool calls, so a checkpoint running once per turn cannot record which individual effect was prepared or settled. The canonical home is new: effect-journal.md carries the identities, the gate, the predicate, the state machine, the ordering-and-failure matrix, the needs_attention contract, retention, and the crash matrix the implementation must be tested against. The DDL stays in database-schema.md. Four documents that promised a blanket idempotency guarantee now describe the tiered one, including architectural-principles.md §11, which CR-12 named explicitly. Refs: ADR-0080 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d this time a test says so The Sonnet review found the artifact field dead for a SECOND time, one layer above where the Opus fold fixed it. `createConsentGate` returned a two-parameter function where `StdioConsentGate` declares three, and TypeScript accepts a shorter parameter list wherever a longer one is expected — so both callers passed an artifact that vanished at runtime, `deps.artifact` was always `undefined`, and the "declared in <file>" line never rendered on any surface. Two of the five call sites carry comments claiming the property works. Nothing caught it because every gate test called `assertStdioConsent` directly: the production adapter the five surfaces actually wire had no test at all. - The adapter forwards `artifact`, and its return type is the shared `StdioConsentGate` alias, so a missing parameter is now a type error. - `chat`, `chat-resume` and Home never computed an artifact either. `resolveChatAgentSource` returns the resolved path beside the agent — beside, not folded into `AgentDefinition`, which is the parsed artifact, is persisted into a session snapshot, and comes from the pure core parser. A RESUMED session names the session rather than a file: it runs the agent snapshot frozen at start, so the original file may have changed or be gone, and naming it would send the user to review bytes that are not what is about to run. - A duplicated spread in `session-host.ts` — harmless, but it sat on the exact line the artifact wiring was supposed to have fixed. Three tests, each mutation-verified against the guarantee it pins: - `createConsentGate` forwards the artifact (and still refuses, and still records nothing on a refusal) — the adapter, not the function under it. - `buildChatSession` names the RESOLVED agent file at the gate, asserted through the surface that computes it rather than at the gate that receives it. - A repointed symlink is a different program (ADR-0084 §10.7's other half — the first half was tested, this one held only by inference), and the grant store never holds an env VALUE, asserted by scanning the written bytes for a seeded credential as §10.12 asks, rather than by re-reading the record's shape. Refs: ADR-0084 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the stale routes are retired §11's landing obligations. The gate had shipped with its specification living only in an ADR and in the code, which is precisely what CLAUDE.md rule 8 forbids: a reader consulting `commands.md` could not discover `--allow-mcp-stdio` exists, and the desktop's Rust spawner — which ADR-0084 §1 places OUTSIDE this gate and which owes the same contract — had nothing to conform to. - `mcp-integration.md` gains the cross-surface contract: the fingerprint's five inputs, the canonicalization rules a second implementation must reproduce, the env type-tagging (with the `secret:` collision it exists to close), the golden vectors, the grant-file format and its fold-closed rule, and the four-way interactivity precondition. The file's own line naming the Rust backend as the owner of stdio children now points at it. - `ipc-contract.md` states the language-neutral invariant: the Rust backend must not spawn a declared stdio MCP server without a matching grant. - `commands.md` documents `--allow-mcp-stdio` on both commands that take it, the exit-2 refusal with its digest listing, why the chat family has no flag, and the CI pattern. - `agent-yaml-spec.md` and `config-spec.md` each state §4's denylist as an authored error at parse — and the config one adds the thing a committed file makes easy to assume: registering a server does not authorize spawning it. - `security-review.md` gains both the shared declared-environment rule (one rule, now true of BOTH process hosts — it was a floor on one and not the other) and the local-spawn floor, plus a mandatory-review trigger. The routing is retired rather than left to rot: `deferred-tasks.md` splits the old "import-trust/consent gate + npx pinning" item — consent closed here and re-scoped, because provenance was the wrong axis (a `git pull` changes a committed artifact with no import step) — leaves `npx` pinning open with its own scheduling, and adds lazy connect with ADR-0052 §3 named as its blocker and the tool-list cache as its unblocker. CR-16 is closed in phase 2.6.5 with the four corrections its own review notes asked for recorded, and the six cross-phase pointers that still routed this gate to "2.6.B" now point where the work actually landed. Also fixes a stale ADR link in `config-spec.md` (`0059-in-place-model-reseat.md` never existed under that slug). Refs: ADR-0084 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… still marked open
Exit criterion 7 asks for a per-item register naming the code that closes each W1
item, "verified by reading the code, not by trusting the mark". Writing it caught
the failure it exists to catch, for the third time: `CR-14` and `CR-92` were both
implemented — `stream-grammar.ts` under ADR-0082, and the durable-truth hold plus
the terminal outbox — and both still carried an open heading ("needs an ADR", "in
the durability spine"). Corrected.
The table names, per item, the module that closes it and the test that would fail
if it were reverted. Two entries record why the obvious test would not have worked:
`CR-10`'s property is not expressible from the durable log at all (streamed events
take sequence numbers and are never persisted, so a lost event reads identically to
a skipped one — hence the append audit's separate witness), and `CR-11`'s fencing
token cannot be produced by a test that only does compare-and-swap.
It deliberately does not claim the phase is closed — W2–W9 remain — and it records
that CR-16's lazy-connect half was split out rather than closed, since deferring
the spawn against today's immutable registry would delete the agent's MCP tool
grant outright.
Refs: ADR-0082, ADR-0083, ADR-0084
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.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
|
Important Review skippedToo many files! This PR contains 211 files, which is 61 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to Pro+ to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (211)
You can disable this status message by setting the Comment |
Reviewer's GuideImplements CR-10/CR-11/CR-12/CR-14/CR-15/CR-16/CR-17 durability and MCP-consent work plus related admission, lease, effect journal, deadline, grammar, and CLI wiring changes across core engine, LLM chain, shared contracts, DB schema/store, CLI commands, and chat/agent runtimes, including new safety checks, outbox/journal/lease infrastructure, input validation, effect deduplication, and consent-before-spawn for local MCP servers. Sequence diagram for consent-before-stdio-MCP spawnsequenceDiagram
actor User
participant RunCommand
participant ConnectAgentMcp
participant ConsentGate
participant StartMcpClient
User->>RunCommand: runCommand
RunCommand->>ConnectAgentMcp: connectAgentMcp
ConnectAgentMcp->>ConsentGate: consentGate
ConsentGate-->>ConnectAgentMcp: Map<serverId,ResolvedStdioSpawn>
ConnectAgentMcp->>StartMcpClient: startMcpClient
StartMcpClient-->>ConnectAgentMcp: McpClient
ConnectAgentMcp-->>RunCommand: McpClient
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…ine running it CI failed this at 610ms against a `< 500ms` bound. The same input takes 33ms here, and measuring it at 25k/50k/100k/200k/400k gives 4.8/9.0/18.3/36.2/69.9ms — dead linear, doubling with the input at every step. There is no backtracking; there is an 18x machine-speed gap on a loaded shared runner. An absolute wall-clock bound never measured the claim. ReDoS is *super-linear growth*, and the old assertion would have gone on failing on the slow machine and passing on the fast one without either outcome saying anything about the regex. So the test now measures what it is named after: doubling the input doubles a linear scan and quadruples a quadratic one, and that ratio is the same on any hardware. The threshold is 3 — between linear's 2 and quadratic's 4 — with a median of five warmed runs so one descheduled run cannot decide the verdict, and a floor on the denominator so a sub-millisecond timing measures the algorithm rather than the clock. Verified against known implementations rather than by mutating the regex: a deliberately quadratic loop reads 3.62 and a linear one reads far below 1, while the real function reads 1.98. (A genuinely catastrophic pattern was also tried — it hangs rather than returning a ratio, which proves the vitest timeout works and says nothing about the assertion, so it is not the evidence to rely on.) Both correctness assertions are unchanged. They are the half that catches the regression a timing bound structurally cannot: a narrowed quantifier that leaks the token tail runs FASTER, not slower. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he reasons the rest are not **The one actual defect.** `authored-system-prompt.test.ts` asserted `typeof forged === 'string'` on a value assigned from a `string` — true of every string ever written. Sonar was right: the line proved nothing. The compile-time half (`@ts-expect-error`) was always the real assertion; the runtime line now pins the other property worth having — the brand is erased, so a branded value is byte-identical to the string it came from and costs nothing at run time. **Under load, the ReDoS test was still measuring the machine.** The growth ratio landed at 4.8 on a provably linear function when the whole monorepo's suites ran concurrently: measuring the small size and the large size in separate groups put them in different contention windows. They are interleaved now — one ratio per round, median across rounds — so both sizes share whatever the scheduler is doing. Eight consecutive full-turbo runs pass; the instrument still reads 2.00 for a known linear implementation and 3.97 for a known quadratic one, with the threshold of 3 between them. Also fixed: a nested ternary in `canonical.ts`'s sort (now a named `compareCodeUnits`, which is where "never `localeCompare`" can be written down) and in `resume-identity.ts` (now `inputMismatchMessage`, with the reason each of its three cases exists); repeated `Array#push` in the consent prompt; two `void` operators; and nine duplicate imports. **Two findings are rejected, in comments, at the code:** - `charCodeAt` → `codePointAt` in `canonical.ts`. The function's entire job is to find a surrogate CODE UNIT standing alone. `codePointAt` combines a well-formed pair into one code point, which makes the paired/unpaired distinction — the only thing being asked — unexpressible. Same call, same reason, as the existing NOSONAR in `render/tui/chat-projection.ts`. - `JSON.parse(JSON.stringify(…))` → `structuredClone` in `resume-identity.ts`. That round trip does two jobs a clone does not: it normalises to JSON shape, so two values compare equal exactly when they would serialise equal — the property the comparison is defined on — and it THROWS on input JSON cannot represent or cannot walk, which is what the `catch` around it exists for and what stopped a RangeError escaping `resumeFromCheckpoint` and stranding a lease for a full TTL. `structuredClone` clones a Date, a Map and a cycle happily, deleting both. `pnpm run ci` exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… that never closes a pair
`/\{\{[\s\S]*?\}\}/` retries its lazy scan from every `{{`, each attempt running to
the end of the string. On a `default` of `'{{'` repeated 60,000 times — a 120KB
authored value — that measures **1033ms**, against **0.011ms** for the `indexOf`
form, and the cost grows with the square.
It matters more than it did when the rule was written. ADR-0084 settled that an
artifact is often not the user's, so a parse-time stall is something a shared file
can cause rather than something an author only does to themselves.
The rewrite is exactly equivalent: a terminated pair exists iff some `}}` follows
the FIRST `{{`, and where the only `}}` sits before it (`}}{{`) neither form
matches. Both tests are new — one pins the linear cost on the hostile input, the
other walks the eleven shapes that would diverge if the equivalence were wrong,
including the overlapping-brace and close-before-open cases.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… complex, where the split had a name Cognitive complexity is a maintainability metric, not a defect, so each of these was taken only where the extracted piece is a coherent decision with a name — not to move a number. Every one is behaviour-preserving; the suites pass unchanged, and no test was edited. - `assertStdioConsent` (18) → four phases: resolve, `undecided` (the digest dedupe, which is a rule with a §10.18 acceptance item behind it), `refuseWithDigests`, `askAndRecord` + `recordGrant`. - `buildChatSession` (20) → `bindChatAgent` (the `/clear`-rebind vs resolve-from-disk decision) and `mcpOptionsFor` (the omit-vs-explicit-undefined shape `exactOptionalPropertyTypes` forces). - `resolveAndValidateWorkflowInputs` (27) → `unknownKeyIssues` and a per-input `admitOne` returning a typed outcome, with `absentOutcome` holding the one rule that differs between `admit` and `verify`. - `verifyResumeIdentity` (31) → `executionModeRefusal`, `reconcileRecorded`, `resuppliedSecret` (§6's "re-supplied or refused"), `unexpectedSuppliedRefusal`. - `deepStructuralEquals` (17) → `alreadyComparing` (the cycle guard, which reads as a question and is called for its answer), `arraysEqual`, `objectsEqual`. - `violatesInputContract` (22) → `violatesNumericBounds` and `violatesStringRules`, the latter carrying the length-before-pattern ordering that is the only ReDoS mitigation the contract offers. - `validateValidationBlock` (27) → `validatePattern`. Also: a nested ternary in `violatesDeclaredType` became a `DECLARED_TYPE_NOUN` table, which is where "number says finite because that is the part a caller gets wrong" can be written down. `EffectSlot = number` is rejected as a redundant alias, in a comment at the code: the docblock above it is where "what a slot is" lives, and replacing every occurrence with `number` would delete the concept from the type surface with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eDoS bound I got wrong twice **The flake was a defect.** `runMigrations — the two-process race` had been failing intermittently and reading as test noise. It was not: converting a database to WAL takes an EXCLUSIVE lock, and SQLite returns `SQLITE_BUSY` for it WITHOUT invoking the busy handler — waiting there could deadlock, so it refuses instead. Two Relavium processes opening one fresh `history.db` at the same moment therefore raced, and the loser's OPEN failed outright. Measured with paired spawns: **18 failures in 30**, and **0 in 30** with the WAL switch routed through the `withBusyRetry` this package already had. `busy_timeout` does not help — measured that too, before assuming it would: 15/25 with it first, 17/25 with it second. What a user saw was `relavium run` refusing to start because another Relavium happened to be starting. A new repeated open-race test pins it (one pair caught the defect only ~60% of the time, so it runs six); removing the retry fails both it and the original test. **The ReDoS test: two wrong instruments before the right one.** `< 500ms` was a machine-speed assertion — 33ms here, 610ms on a loaded runner, and neither says anything about backtracking. A growth RATIO across two input sizes is the textbook answer and is genuinely discriminating in isolation (1.98 for this function, 3.97 for a known quadratic one), but under whole-monorepo contention the per-round ratios scattered from 0.98 to 10.17: each measurement is milliseconds, and one descheduled window decides it. It also blew the default 5s test budget by measuring seventeen redactions on a machine where each costs 610ms. The threat is CATASTROPHIC backtracking, not a merely-quadratic scan: a quadratic pattern on 250KB is slow, an exponential one does not return — measured, a nested quantifier ran this input past 120 seconds. So the bound is now set where it separates those two worlds (~8x the slowest honest observation, orders of magnitude below a real regression), with an explicit test timeout so the assertion reports a failure rather than the runner. Eight consecutive full-turbo runs pass. Also in this batch, the remaining Sonar complexity findings where the split had a name: `parseWorkflow` → `safeParseGuarded`, `runCommand` → `parseOrRefuse`, and the three byte-identical attempt-failure tails in `#runStreamAttempt` → one `#failAttempt` spliced with `yield*` — which removes the duplication that let the `committed()` stamp go missing from one of them once already, and is mutation-verified to still be pinned. Plus two optional chains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eclined at the code Split, because the piece had a name: - `dispatch` (38) → `prepareEffect`, ADR-0080 §7 step 2. It is the line the whole mechanism draws — everything above it is a refusal that journals nothing, everything below may have reached a target — so it reads better as a named step than as a nested try inside the ladder. - `verdict` (21) in the append audit → `outOfOrderPairs`, shared by checks 3 and 4. They ask the same question of two different lists (what the engine ISSUED vs what the store COMMITTED) and are genuinely different properties, but the pair walk is one walk, and the two copies had already drifted on the `undefined` guard `noUncheckedIndexedAccess` requires. Declined, with the reason in a NOSONAR at the code rather than in a dashboard: - `#emitDurable`'s settled closure (23). Every branch in it is an ordering guarantee relative to `await prior` and the persist, and the `held`/`unclaimed` fast path exists specifically to avoid a microtask hop that the comments record as having reordered the log once already. Extracting any branch reinserts exactly that hop. The metric is not worth reopening the race CR-10/CR-92 closed. - `resumeFromCheckpoint` (16, one point over). The branches ARE the ordering — the lease before any read, the identity guard before the checkpoint, the checkpoint before the workflow — and each comment states which line it must precede. A helper hiding one step turns a sequence the reader can see into one they have to reconstruct. Also marked the standalone-credential alternation in `bounding.ts` NOSONAR, whose deliberate-exception rationale was already written above it: a per-family split is not equivalent, because an earlier pass inserting `[redacted]` truncates a later family's greedy match and can leave a trailing secret-shaped substring exposed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fixture EXISTS to contain the forms the fence must catch: the one-hop type alias and the type-predicate return annotation are the cases `assert-fence.mjs` asserts on, so "cleaning them up" would delete the coverage rather than improve it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onal chain `withBusyRetry(() => sqlite?.pragma(…))` expressed a real invariant as an optional one: TypeScript cannot narrow the outer `let` inside a closure, so the `?.` was there to satisfy the compiler — and it would silently SKIP the WAL switch rather than fail if the binding ever were undefined, which is the opposite of what a fail-closed open path wants. A local `const opened` carries the connection through the pragmas and the Drizzle bind; the outer `let` stays because it is what the `catch` closes. Also records what was measured and rejected: setting `busy_timeout` before `journal_mode` does NOT help this race (15/25 against 17/25), so the pragma order is left as it was rather than changed on a plausible-sounding theory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ect's actual Node floor `attempt-timer.test.ts` spawns a child that imports the REAL `hostAttemptTimer` from a `.ts` file — which is the point of the test, since an earlier version inlined a hand-written `setTimeout` and stayed green when `unref()` was put back into the function it was supposed to be measuring. Node unflagged type stripping in 22.18, and this project's floor is `>=22.13.0`. On the one CI leg that runs the exact floor the child died with `ERR_UNKNOWN_FILE_EXTENSION` — a red advisory check I introduced two steps ago and did not notice, because every other leg runs a newer Node where the flag is on by default. The flag has existed since 22.6, so passing it explicitly works on the floor and on every newer runtime alike. The consent gate's own two-process fixture already passes it; this was the one child that did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… pass — five suppressions that never applied
Checked the SonarCloud API rather than assuming the last pass landed, and it had
not: **five of nine `NOSONAR` comments were on the line ABOVE the issue**, and
SonarCloud only honours one on the SAME line. They were doing nothing. Moved to
end-of-line, with the explanation left as prose above.
Two of them cannot be suppressed that way at all — prettier relocates a trailing
comment after `{` into the block — so instead:
- `resumeFromCheckpoint` (16, one over) → `#ownedElsewhereMessage`. The ordering
the rest of the method defends is untouched: this is a message the ordering does
not depend on.
- `dispatch` (33) → `resolveGrantedTool`, the three refusals that reach no target.
REGISTERED IS NOT AUTHORIZED, which is why the grant check sits beside the lookup.
- `#runStreamAttempt` (16) → `#settleUsage`, which sits outside the attempt's
`try`/`finally` already and so touches no teardown ordering.
`#emitDurable`'s closure (23) is the one that stays: its branches ARE the ordering
relative to `await prior` and the persist. It needs an Accepted resolution in the
SonarCloud UI, which is a maintainer action, not a code one.
**And one of my "fixes" last pass did not fix what was reported.** Sonar still
flagged `authored-system-prompt.test.ts` — correctly. I had replaced
`expect(typeof forged).toBe('string')` with `expect(forged).toBe(HOSTILE)`, which
is *also* vacuous: `forged` is assigned from `dynamic`, which is assigned from
`HOSTILE`. This property only exists at the type level, so the assertion is
`expectTypeOf` now — already this repo's idiom in `llm/src/types.test.ts`.
**The extraction work surfaced a real coverage gap.** Mutating the fold-failure
path to drop its `committed()` stamp left all 773 tests in `@relavium/llm` green,
even though the code's own comment claims the stamp keeps "every error the chain
surfaces past content carries `contentCommitted`" true. That is the third stamp
site and the only one without a test; it has one now, and it fails when the stamp
is removed. `kind: 'unknown'` already derives `retryable: false`, so nothing
changes today — which is exactly why the invariant needed a test rather than a
second, unrelated mechanism holding it up.
Also: `indexOf(…) !== -1` → `includes` in the interpolation scan, and
`[A-Za-z0-9_]` → `\w` in the env echo-safety gate (exactly equivalent in JS; the
lead class cannot fold, since it excludes digits).
`pnpm run ci` exits 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… regex declines on their own lines Sonar went 17 → 9 after the previous pass. Of the nine: - `dispatch` (30) → `admitArgs`, steps 2-4. They belong together for a reason worth writing down: the effective set is assembled and VALIDATED before the guardrail check, so a `tool` node — whose args come entirely from `input_mapping`, with no model args at all — cannot bypass the allowlist by being checked before its args exist. Removing the policy call from the extracted helper fails 16 tests. - `isSecretishFlag` and the RFC-3339 `date-time` check are declined, each on its own line so the suppression actually applies. The first is one anchored alternation of literal flag names — the metric is counting NAMES, and a Set would trade that for hand-expanding `api[_-]?key` into three members apiece, which is where a real omission would hide. The second's complexity IS the per-component calendar and clock bounds; "simplifying" it means deleting bounds and reinstating the defect its own comment describes (`0000-99-99T99:99:99Z` was once accepted as an instant). **I attempted the five `String.raw` findings and reverted them.** Converting `'[^\\s\\u0000-\\u001f\\u007f]'` to its raw form is a genuine readability win, but the escapes were mangled twice on the way through the editing layer and landed as literal control bytes in the character class — silently WIDENING what `format:` accepts, which is the exact failure the surrounding comment warns about. A cosmetic MINOR is not worth a change I could not write reliably, so the doubled form stays. `#emitDurable` (23) also stays: its branches are the ordering relative to `await prior` and the persist. It needs an Accepted resolution in the SonarCloud UI — a maintainer action, not a code one. `pnpm run ci` exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nts, so the suppression lands Both `NOSONAR` markers were sitting three lines above their issue, inside a multi-line comment — SonarCloud honours one only on the issue's OWN line, so neither did anything. Two earlier placements failed the same way. Hoisting each regex to a named `const` fixes it for a reason worth stating: with the literal on its own line, prettier's own formatting puts the trailing comment exactly where Sonar wants it. The names are the better half of the change anyway — `RFC_3339` and `SECRETISH_FLAG` say what an inline literal made the reader derive, and each now carries its rationale in a docblock rather than a comment wedged into a `.test(` call. Neither is a simplification Sonar could get: the first's complexity IS the per-component calendar and clock bounds, and "simplifying" it means deleting bounds and reinstating the defect its own docblock describes; the second's is the length of a flag-name list, where a Set would trade one anchored alternation for hand-expanding `api[_-]?key` into three members apiece. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…what they exclude The three `format:` character classes were written as JS string literals with doubled backslashes, so every read cost a step of mental arithmetic in a file where getting a class wrong silently WIDENS what `format:` accepts. `String.raw` makes them read as the regex source they are. I abandoned this conversion once already: the escape sequences were mangled twice in transit and landed as literal control bytes in the character class — the exact silent widening the code warns about. Done here by constructing the bytes explicitly (`chr(92)`, never a typed escape) and proving byte-equality of the compiled sources before touching the file. The test is the part worth reading. Its FIRST draft re-declared the three classes locally and asserted against those — so it tested its own copy and would have passed against any drift in `workflow.ts` at all. A mutation exposed it: widening the real class was caught by a neighbouring suite and NOT by the new test. It goes through `violatesInputContract` now — the exported entry point admission itself calls — and each of the three classes was mutated in turn to confirm it fails. Closes the last five Sonar findings that were mine to close. The two remaining CRITICALs are declines: `#emitDurable`'s branches ARE the ordering, and both need an Accepted resolution in the SonarCloud UI rather than a code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erely expected (PR83-01) The compare-and-append guard checked only that the store's `max(seq)` equalled `expectedLastSequenceNumber`. It never checked that the incoming event's own sequence was GREATER than that maximum — and equality alone does not order a log. Sequence gaps are legitimate: a transient or streamed event consumes a number without becoming a durable row. So `(run_id, seq)` uniqueness cannot establish order either — a stale event's number is both unique and lower. Reproduced against the real SQLite store: `run:started` at 0, durable work at 5, then `run:failed` at 3 with a TRUTHFUL `expectedLastSequenceNumber: 5`. The append was accepted. Durable order became [0, 3, 5], `applyDerived` marked the run terminal, and `listInterruptedRuns()` stopped reporting it. The terminal-outbox drain is the reachable path, and it is where this bites: it passes the run's current maximum as its belief — truthfully — so the equality half always passes, and the held terminal was built by a crashed process from an older view. On that false success the drain deletes the outbox entry: the only evidence that the terminal was ever uncertain. Both stores now require `event.sequenceNumber > actual` whenever the guard is supplied, including the in-memory reference — a reference that accepted what SQLite rejects would hide this from every `packages/core` test. `AppendConflictError` carries the incoming sequence so a caller distinguishes "your belief is stale" from "your event is behind" without parsing a message. The drain needs no change: the store's refusal reaches its existing catch, which retains the entry. Four regression tests, each verified to fail with the guard removed: SQLite rejects behind/equal and keeps a legitimate gap legal; the reference store mirrors it; and the drain holds a stale terminal instead of writing it behind newer work. The `DurableWriteContext` contract claimed the equality check detected "an out-of-order commit, and a replayed append". It did not. It says what is enforced. Refs: ADR-0078 §2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…efined` (PR83-02)
`JSON.stringify` DELETES a property whose value is `undefined`. The registry goes
out of its way to treat `undefined` as a legitimate tool result — that is what the
`NOT_REPLAYED` sentinel exists for — and then handed the replay envelope to a store
that dropped exactly that key.
Reproduced against real SQLite: settling
`{ value: undefined, truncated: false, summary: '', hadMapping: false }` stored
`{"truncated":false,"summary":"","hadMapping":false}`. On reload the reader's
`'value' in stored` guard failed, the compatibility fallback treated the whole
METADATA OBJECT as an old bare result, and the replay delivered that object as the
tool's return value. First run and resumed run produce different values — only
after a crash, and only for a tool that returns nothing.
The stored form is versioned now (`v: 1`) with an explicit `hasValue` bit, so
presence is stated rather than inferred from whether JSON happened to keep a
property. An unknown or malformed `v` fails CLOSED to "cannot re-deliver" instead
of falling through to the bare-value arm — falling through is the defect itself.
Both legacy shapes still read: the pre-`v` envelope and a bare stored value. The
pre-`v` ambiguity is inherited, not introduced: the byte that would tell an old
`undefined` from a bare value was never written.
`mapped` needs no parallel tag — `hadMapping` already records whether a mapping was
CONFIGURED, so a mapping projecting to `undefined` reads back correctly. That
distinction was already deliberate; this one was missed.
**The reference journal is why no test caught it.** It held the retained result BY
REFERENCE, making it strictly more capable than the store it stands for — so every
core test over the replay gate passed while SQLite corrupted the value. It
serializes and parses like the real store now, which is the same principle already
written into its `prepare`: a reference that accepts what SQLite refuses makes the
tests over it vacuous. That change alone surfaced a second latent coupling —
`blocksResume` reads `{ state, result }`, so the parsed value has to be threaded
into it rather than the row.
The regression test dispatches a tier-3 tool that genuinely returns nothing, then
resumes; it fails when the envelope is written unversioned.
Refs: ADR-0080 §4, §7
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eadline (PR83-03) `openDeadline` woke active race waiters when a caller abort FIRED, but a caller that had already aborted at construction only got the provider-facing controller aborted. Nothing recorded the cancellation for a `race` that had not been called yet, and a native signal does not re-emit `abort` to a listener attached later. So `race()` saw `expired === false`, registered a waiter nothing would ever wake, and waited on the provider. Against a provider that ignores its signal — the exact case the hard race exists for — it stayed pending until the ABSOLUTE timer fired: 120 seconds on the shipped default, during which Ctrl-C looks ignored. `classify()` reported `caller` the whole time, so the label was right and the liveness was not. The gap is reachable, not theoretical: the outer loops do check cancellation, but `preAttempt` and credential resolution both run after that check and before `openDeadline`. A cancel landing there arrives already-aborted. Caller cancellation is latched now, and `race` consumes the latch on entry — sharing the abandoned-step arm, because the disposal is identical (abandon the step, handle its late rejection) and only the label differs, which `classify()` already owns. Five regression tests, all verified to fail with the latch removed: an immediate settle for a pre-aborted caller WITHOUT firing the timer, an abort landing between races, no unhandled rejection from the abandoned step, and the chain-level gap on both the generate and stream paths. Writing those chain tests turned up something worth stating: my first version wired no timer port, so `#openDeadline` returned undefined, the chain plain-`await`ed the provider, and both tests hung on a code path with no deadline in it at all. They wire the port and never fire the timer — settling is what proves the caller latch woke the race rather than the clock. Refs: ADR-0082 §5, §7, §12 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… success (PR83-05) The SQLite `settle` ran an update constrained by exact identity and `state = 'prepared'`, then discarded the statement's `changes` count. A missing identity and an already-terminal row therefore both resolved as success, and the registry proceeded believing a real external effect had been durably journaled. Reproduced against the real store: `settle` on an identity that had never been prepared returned normally, having changed zero rows. The code's own comment had the first half right — a settle against an already-terminal row is a caller bug, and the honest response is to leave the durable answer alone. Leaving truth alone is correct. Reporting that the requested transition happened is not: corruption, an accidental delete, or a state-machine race was converted from a loud fail-closed condition into silent success, past the `ToolEffectNeedsAttentionError` path the registry keeps for exactly this case. `EffectTransitionError` is typed distinctly from `EffectConflictError` because a caller acts differently on each: a conflict means another attempt legitimately holds the identity and this dispatch must not proceed; a transition failure means the durable record is not what the caller believed while the effect it describes may well have landed. Mirrored in the in-memory reference for the reason PR83-02 just proved the hard way — a reference more permissive than the real store makes every test over it vacuous. The existing terminal-settle test asserted the silent no-op, so it is updated rather than added to: same intent (the durable answer is not overwritten), now with the refusal that intent always implied. Refs: ADR-0080 §7 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ly unresolved effect (PR83-04) For an effectful tool the registry writes `prepared` before entering the dispatch arm. When dispatch throws `ToolUnavailableError` — a missing host capability, which throws synchronously before the host is ever touched — `neverLeftTheProcess` correctly concludes the effect could not have reached a target, so the catch declines to settle `ambiguous`. Declining was right. Doing nothing else was not. The row stayed `prepared`, which the state machine reads as UNRESOLVED: it blocks workflow resume, is disclosed on session resume as an effect that may have landed, and is exempt from every age sweep because unresolved rows are the record an operator needs. A wiring error therefore produced a permanent, misleading, operator-facing claim of uncertainty about a call that provably never happened. The existing test asserted exactly that — `state === 'prepared'` — directly under a comment saying the point was to avoid a permanently unresolved row. The assertion and the stated intent contradicted each other, and the assertion won. `EffectDispatchPort.discard` releases a `prepared` claim, in both the SQLite store and the in-memory reference. Constrained to `prepared` in both, so it can never erase a terminal row recording something that DID happen. A missing row is not an error — it releases a claim, and a claim already gone is the outcome it wanted. Chose this over the two alternatives deliberately. A new terminal `not_dispatched` state would need a schema change, a CHECK-constraint migration and a snapshot regen, to record a call that did not occur. A capability preflight derived from `ToolPolicyClass` looked cleaner until I checked the mapping: `egress: 'mcp'` routes to `requireMcp`, not `requireEgress`, and `read_media` has its own — so a policy-derived preflight could refuse a dispatch that would have worked, which is worse than the bug. Both sides are now pinned: a missing capability leaves no row, and a network failure still settles `ambiguous` and still blocks. The second test exists so the release cannot quietly widen into "any dispatch throw clears the row". Refs: ADR-0080 §7 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CLI reference said a durability-uncertain terminal is retried "on the next `relavium` start" and told scripts to re-check `relavium status <runId>` after a subsequent invocation. Neither half held. Only `run` and `gate` drain the outbox, and `status` never did — so a user who got exit 5 and did the natural thing, asking for status repeatedly, saw the same stale truth indefinitely. Recovery only happened as a side effect of running some unrelated workflow. The narrow constraint is real and worth stating: `drainTerminalOutbox` is a `WorkflowEngine` method, and `run`/`gate` are the only commands that construct one. `chat`, `agent run` and the bare-invocation Home run on `AgentSession` and have no engine to drain with, so "every start drains" is not a one-line change — it is a core refactor to lift the drain off the engine and onto its host ports. So `status` now NAMES the state instead of silently misreporting it. Such a run reads `running` in the derived projection because its terminal never became durable, which made it indistinguishable from a run still working; it is marked in the human listing and carries `terminalHeld` in `--json`. READ-ONLY, deliberately, and this is the part I would not trade away: draining claims a run lease, and a status read must not take ownership of a run another process may be finishing right now. An unreadable outbox degrades `status` to exactly what it printed before rather than failing it — pinned by its own test. The docs now say which two commands drain and why, rather than implying all of them do. `statusCommand` becomes async; its executor was already Promise-typed, so the change stops at the dispatcher. The engine-side lift — a drain callable from any surface — is the better long-term answer and is recorded as a follow-up rather than smuggled into this diff. Refs: ADR-0078 §4, §5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion (PR83-07) `database-schema.md` said run-effect retention "is not implemented yet — no sweep touches this table today". The same PR ships and wires both committed-effect sweeps (`effect-retention.ts`), and `effect-journal.md` §9 correctly says both ship. The canonical schema reference was telling maintainers and future store authors the opposite of what the code does. Corrected to state the actual contract, which is an asymmetry rather than a gap: `committed` rows are swept once their correlation can no longer be resumed; unresolved rows are never swept by age, because an unresolved row is the record an operator needs and outlives its run deliberately — the same reason the table carries no foreign key to `runs`. The ER diagram also drew `runs ||--|| run_leases`, implying every run has exactly one lease. A lease row is created on acquire and DELETED on release, so most runs have none for most of their lifetime and a finished run has none at all. Now `||--o|`, with a note saying why — the mandatory reading is precisely what would mislead someone porting this schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e of them protects My PR83-02 and PR83-04 fixes pushed `dispatch`'s cognitive complexity from 29 to 33 — a regression I introduced while fixing the review findings, and Sonar caught it. Two cohesive steps move out: - `journalDispatchFailure` — the whole of §7 step 4, where the three outcomes and the reasons they differ now sit together instead of nested in a catch. - `settleCommitted` — §7 step 8, including the `ToolEffectNeedsAttentionError` window where the effect provably happened and the record does not say so. Mutation-checking the extractions turned up a real coverage gap rather than just confirming the move. Storing `mapped` UNCONDITIONALLY — dropping the guard that omits it when the node configured no `output_mapping` — passed all 239 tool tests. That guard is load-bearing: without a mapping, `outputMapped` IS the full unbounded result, so persisting it would put the very thing "settle after bounding" exists to keep out of `history.db` straight back into it, with no cap and no sweep. The new test pairs both directions — no mapping means no `mapped` key, a configured mapping means there is one — because the negative alone would also pass for a row that stored nothing at all. It fails under the mutation. Its first draft asserted the stored envelope was smaller than a 50 KB body, which was wrong about the subject: 50 KB is within the bounding limit and is legitimately retained whole. Bounding has its own tests; this one is about the projection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes W1 of Phase 2.6.5 — the eight P0 blockers plus
CR-92, each with an accepted ADR landing before its implementation.pnpm run ciandpnpm coverageboth exit 0.The bulk of this branch is
CR-14/CR-92/CR-15/CR-17(ADR-0082, ADR-0083); the newest work isCR-16— consent before a local MCP spawn (ADR-0084), which is where I'd point your review first.What
CR-16decidesA
stdioMCP server is a local program the artifact chooses. Before ADR-0084, opening an agent that declared one ran it — no tool call needed,askmode did not apply, andshell: falsedid not stop the declaration from namingshornode.The gate is a host decision at one chokepoint, per machine and per resolved declaration. Four things the original register item did not anticipate, each settled in the ADR after you rejected two earlier drafts:
git pullchanges a committed artifact with no import step, so the gate covers every stdio server on every path.cwdis in the identity. Acommandmay be relative —node server.jsin two directories is two programs. (I asked you about this once with an incomplete premise and re-asked with the correct one.)NODE_OPTIONSis a name: a digest over names alone let one grant match every value of it. A sole{{secrets.NAME}}reference contributes only the name, so no credential enters the digest — and the tag is what stops a literalsecret:acmefrom colliding with a real reference.PATHchange in between cannot substitute a binary under an approved fingerprint. The declared-environment denylistrun_commandalready had is now shared with this host — that was a gap, not a decision.ADR-0084 §10 — the 19 acceptance items, and which test satisfies each
mcp-servers.test.ts— "a REFUSING gate means nothing is ever handed to the client"mcp-servers.test.ts"…an APPROVING gate hands them over exactly once" +mcp-consent-gate.test.ts"declining refuses the run, and records NOTHING"mcp-consent-gate.test.ts"approving records a grant, and a second invocation does not ask again"mcp-consent.test.ts"changes with the resolved command, the args, an env NAME, an env VALUE, and the cwd" + the secret-reference invariance casessecret:xand{{secrets.x}}differmcp-consent.test.ts"a LITERAL … and a … reference DIFFER"mcp-consent.test.ts(PATH walk) +mcp-servers.test.ts"spawns the CONSENTED absolute executable, not the authored word" +find-on-path.test.tsmcp-consent.test.ts"canonicalizes the cwd…" and "a REPOINTED symlink is a different program"declared-env.test.ts+process.test.ts"refuses EVERY member of the shared list — the drift test"--allow-mcp-stdioacceptsmcp-consent-gate.test.ts, same-named casemcp-consent-gate.test.ts, the four-case table--allow-mcp-stdiowrites no grantmcp-consent-gate.test.ts"--allow-mcp-stdiowrites NO grant"0600, directory0700, no env value in the written bytesmcp-consent.test.tsmode/repair cases, "creates a MISSING parent directory 0700", and "the store never holds an env VALUE — scanned in the written BYTES"mcp-consent.test.ts+fixtures/concurrent-granter.mjs— genuine OS processes, via a module-resolution hookmcp-consent.test.ts"ANY unparseable line folds the WHOLE store closed" +mcp-consent-gate.test.ts"REPORTS an unreadable grant store"v2:fails closed, lone surrogate refusedmcp-consent.test.tsgolden-vector table +mcp-consent-gate.test.ts"refuses a declaration carrying a LONE SURROGATE"consent-prompt.test.ts(9 cases) +mcp-consent-gate.test.ts"SANITIZES every displayed field"mcp-consent-gate.test.ts"shows the resolved executable, each argument separately, and the env with authored values" +createConsentGate"FORWARDS the artifact" +session-host.test.ts"names the RESOLVED agent file at the consent gate"mcp-consent-gate.test.ts"two ids naming the SAME declaration prompt ONCE and record ONE grant"mcp-consent-gate.test.ts"a network-only declaration needs no consent" + the existing SSRF suiteEvery new guarantee above was mutation-verified: the guard was broken and exactly the named test failed. Item 15's
v2:half is the one corollary I did not write a separate test for — it follows fromConsentGrantSchema'sz.literal(1).Where the reviews found real bugs
Three review rounds (Opus, then Sonnet, on top of two rounds you ran yourself on the ADR). What they caught is the honest part of this PR:
relavium run.chat,chat-resume, Home andagent runall spawned ungated — an imported agent is opened bychat --agent, not byrun. Fixed, then made structurally impossible:connectAgentMcp/connectWorkflowMcpnow refuse a stdio declaration when no gate was wired, so a sixth surface cannot forget.--allow-mcp-stdiowas registered and never forwarded. The action'soptswas typed{ input?: … }, so the documented CI escape hatch did nothing.createConsentGatereturned a two-parameter function whereStdioConsentGatedeclares three — TypeScript accepts a shorter parameter list, so both callers passed an artifact that vanished at runtime, and two of the five call sites carried comments claiming it worked. Nothing caught it because every test calledassertStdioConsentdirectly; the adapter the surfaces actually wire had no test at all. It has one now.canonicalJsondid not implement the lone-surrogate refusal the ADR claimed.Documentation (ADR-0084 §11)
The gate had shipped with its specification living only in an ADR and in the code.
mcp-integration.mdis now its canonical home — the fingerprint's five inputs, the canonicalization rules, the env type-tagging, the golden vectors, the grant-file format — stated as a cross-surface contract, because §1 places the desktop's Rust spawner outside this gate and it owes the same one. Also:ipc-contract.md(the language-neutral invariant),commands.md(--allow-mcp-stdio, the exit-2 refusal, the CI pattern), both schema specs (the denylist as an authored error),security-review.md(the shared declared-environment rule and the local-spawn floor), and the six cross-phase pointers that still routed this gate to "2.6.B".Landed after this PR was opened: the CI failure and the Sonar pass
Three of these are real defects the tooling surfaced, not metric noise.
A "flaky" test was reporting a real open race.
runMigrations — the two-process racehad been failing intermittently and reading as noise. Converting a database to WAL takes an EXCLUSIVE lock, and SQLite returnsSQLITE_BUSYfor it without invoking the busy handler — waiting there could deadlock, so it refuses. Two Relavium processes opening one freshhistory.dbat the same moment raced and the loser's open failed outright: 18 failures in 30 paired spawns, 0 in 30 with the WAL switch routed through thewithBusyRetrythis package already had. What a user saw wasrelavium runrefusing to start because another Relavium happened to be starting.busy_timeoutdoes not help — I measured that too before assuming it would (15/25 with it first, 17/25 with it second). A repeated open-race test pins it; removing the retry fails it.A quadratic regex on authored YAML.
/\{\{[\s\S]*?\}\}/retries its lazy scan from every{{, each attempt running to end of string: 1033ms on a 120KBdefaultof'{{'repeated, against 0.011ms for theindexOfform. It matters more since ADR-0084 settled that an artifact is often not the user's — a parse-time stall is now something a shared file can cause.A vacuous assertion in the
AuthoredSystemPromptfence.expect(typeof forged).toBe('string')on a value assigned from astring— true of every string ever written. The compile-time@ts-expect-errorwas always the real check; the runtime line now pins the property worth having (the brand is erased, so it costs nothing at run time).I got the ReDoS test wrong twice before getting it right, and the PR history shows it.
< 500mswas a machine-speed assertion (33ms locally, 610ms on a loaded runner). A growth ratio across two input sizes is the textbook answer and is genuinely discriminating in isolation — 1.98 for this function, 3.97 for a known quadratic one — but under whole-monorepo contention the per-round ratios scattered from 0.98 to 10.17, and it blew the default 5s budget by measuring seventeen redactions. The threat is catastrophic backtracking, not a merely-quadratic scan: measured, a nested quantifier ran the input past 120 seconds. The bound now sits where it separates those two worlds, with an explicit test timeout so the assertion reports a failure rather than the runner.The rest of the Sonar pass: nine complexity refactors where the extracted piece had a name (
assertStdioConsent,buildChatSession,resolveAndValidateWorkflowInputs,verifyResumeIdentity,deepStructuralEquals,violatesInputContract,validateValidationBlock,parseWorkflow,runCommand, plusdispatch→prepareEffectand the audit's shared order walk); three nested ternaries; nine duplicate imports; twovoidoperators; two optional chains.Five findings are rejected, each with the reason in a comment at the code rather than in a dashboard:
charCodeAt→codePointAtcodePointAtcombines a well-formed pair, which makes the paired/unpaired distinction unexpressible.JSON.parse(JSON.stringify(…))→structuredClonecatcharound it exists for, and what stopped a RangeError escapingresumeFromCheckpointand stranding a lease for a full TTL.structuredCloneclones a Date, a Map and a cycle happily.EffectSlot = numberis redundantnumberdeletes the concept from the type surface.#emitDurable's closure is too complexawait priorand the persist, and the fast path exists to avoid a microtask hop the comments record as having reordered the log once already.resumeFromCheckpointis too complex (by one)Two things I want your call on
69e6a86mixes ADR-0083's text withCR-14/CR-21code. I raised rebase-splitting it three times and proceeded without an answer rather than block. If you want the ADR extracted into its own commit, say so and I will rebase.docs/decisions/0069-…→ a renamed ADR-0047 slug, anddatabase-schema.md→keychain-and-secrets.mdmissing its../desktop/prefix). Unrelated to this work, so I left them out rather than widen the diff.Not closed here
deferred-tasks.mdwith the tool-list cache as its unblocker.npxversion/integrity pinning — the other half of the old register item. Consent answers "may this program run?", not "is this the same code as yesterday?": an approvednpx -y @acme/serverre-resolves on every spawn under a byte-identical declaration.🤖 Generated with Claude Code
Summary by Sourcery
Close the Phase 2.6.5 W1 reliability and security blockers by hardening durable execution, resume identity, provider liveness, input handling, and local MCP process consent across every CLI surface.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: