Phase 2.6.5 — the prerequisite, the oracle, and W0 - #82
Merged
Conversation
Verified every ✅ in PR #81's closing register against the code rather than trusting the marks: `#W15-3`'s `/cost --release`, `#W15-4`'s durability latch, `#W15-5`'s projection check, `#W15-6`'s folds, `#W15-7`'s refinement, `#W15-9`'s fold guard, `#W15-10`'s escaper, `#W15-13`'s opt-in heal, `#W15-14`'s `hostSleep`, `#W15-15`'s degraded flag, `#W15-22`'s `Reflect.apply`, `#W15-23`'s `TxDb`. All present. ADR-0074/0075/0076 are Accepted and both new ones are indexed. Two markings were wrong, both in the same milestone note: - it said ADR-0074 §1 "remains open". It closed on 2026-08-09 with `/cost --release` (`commands/chat.ts` calls `releaseConservativeCommitments`; `repl-info.ts` renders the durability state and points at the flag). - it called that work "Wave 1.5". That framing was retracted — the work was PR #81's own closing list, not a later wave — and this was the last place still carrying it. Also: the register said the fixes were "committed on `development`". They are merged to `main`. `#W15-1` stays OPEN and is NOT marked. Its decision is made (ADR-0076, Accepted) but no implementation has landed, so the count remains 23 of 24. Marking it now is exactly the false-completion error this register was rebuilt to correct. Refs: PR #81, ADR-0074 §1, ADR-0076 Co-Authored-By: Claude <noreply@anthropic.com>
Three independent core reviews of the post-Wave-1 tree converged, without seeing each other, on the SAME seven blocking gaps: no durable effect journal, stdio MCP spawning before consent, no cross-process run ownership, compaction summaries elevated to `system` authority, a stream ending without a terminal counted as success, the engine not enforcing the authored input contract, and the durable event log not being a gap-free ordered prefix. Three separate reviews landing on the same seven points is not a matter of opinion, and it is why this interlude exists. All three also reached the same verdict on scope: the architecture is right and nothing here calls for a rewrite. The gaps are in the last ten percent of reliable execution — durable ownership, effect identity, trust provenance, stream grammar, admission and ordering — and the existing seams can carry the fixes. The phase document is deliberately SELF-CONTAINED: 40 items (`CR-01`…`CR-95`) each carrying its own evidence, failure scenario, fix and acceptance criteria, so the work can be done from that file alone without the source analyses. It inherits Wave 1's discipline explicitly — ADR before code for anything that changes what a contract means, break-verify with the mutation confirmed applied, never ship a hollow test, record rather than assert, fix the canonical doc in the same change. Three items are already half-closed by Wave 1 and finish first. One of them, `CR-03`, is a propagation gap from `#W15-10`'s own fix: three `--json` paths still use bare `JSON.stringify` and never got the safe serializer — verified in the current tree, not inferred. Wave 2 is amended rather than replaced: its MCP cluster stays and consent-before-spawn joins it as a P0 in the same code area. The realized-cost ledger and the transaction-handle cleanup keep their own tracking and are named in the scope note so nobody schedules them twice. Refs: phase 2.6.5, Wave 2 Co-Authored-By: Claude <noreply@anthropic.com>
…ule, effect semantics An adversarial review of the phase document itself. Every mechanical and structural claim was re-verified against the tree before acting; all of them held. Five blocking corrections: - **Phase boundary.** The metadata named Wave 2 as successor while CR-16 and W4 joined Wave 2 and the exit criteria demanded every item close — so the phase could not close before its own successor. Wave 2's MCP queue now EXECUTES as 2.6.5's W4; Wave 2 keeps the fs jail, secrets, config trust and the 2.5.5 certification. The hostile-MCP security sitting moves with the code, so the reviewer is booked once, not twice. - **#W15-1 is a prerequisite, not a parallel track.** Its five staged steps touch the same four files CR-10 and CR-12 restructure. Landing it after the durability spine means writing its barrier against a persistence path about to change. - **Exit criteria permitted a vacuous close.** The old rule let all 43 items be deferred with a reason and the phase declared complete. Fourteen are now non-deferrable — every W0/W1 item plus CR-50/55/73/80/92 and CR-95's short-term fix — on the ground that each has a cheap fail-closed option, so size argues for the cheap option, never deferral. - **CR-12's "exactly once" was technically unachievable** for an opaque non-idempotent effect: the interval target-completed → process-died → no-receipt is irreducibly ambiguous. Restated as three tiers (idempotency-key / queryable / opaque→ambiguous+ needs_attention, never auto-retried). architectural-principles.md §11 asserts the false version today and is named as a required correction in that ADR's own PR; the ADR now amends ADR-0041 and ADR-0037 rather than sitting beside them. - **CR-17 added** — engine.ts's ResumeFromCheckpointInput documents its own gap: inputs, executionMode and planOptions are caller-supplied and never verified, so a resume can silently be a different run even with a perfect log and a single owner. This falsifies crash-safe resume independently of CR-10/CR-11. Ordering and accuracy: - Oracle before spine — CR-90/CR-91 land first (they are the instrument CR-10/11/12/92 are proven with); CR-92 moves into the spine, since an API returning `completed` while history says `failed` is a runtime defect, not a harness concern. - CR-30 was presented as an open choice; ADR-0036 already decided bounded producer-await no-drop. Restated as "implement the accepted decision". - CR-14 and CR-15 acceptance widened to the full grammar / full InputValidationSchema. - CR-61 split: output_schema deep validation (ADR-0038 drift, needs a validator-dependency ADR) vs new CR-63 — agent input_schema is documented as metadata, so its absence is correct and the action is to verify no doc claims otherwise. - CR-52 now cites ADR-0039's deliberate deferral instead of reading as new drift. - CR-02's decision made from the code: the counter stays, because the rule both catch paths already apply is "count only when a provider engaged". - Unimplementable acceptance criteria fixed: #turnCount is a private field (assert through the turn cap), CR-13 must not assert model behaviour (structural + type-level only), CR-90 uses a synthetic fixture rather than a real git worktree. - Decision/ADR/gate register added; nine items marked decision-open do not start until the maintainer settles them. - Mechanics: 12 H1s → 1, 40 → 45 items, stale last-updated, "six of seven" → eight, review provenance table, and `pnpm coverage` added to the gate (it is a separate required check, not inside `pnpm run ci`, and this phase edits core heavily). pnpm run ci EXIT=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… barrier #W15-1 step 1 of 5, plus the ADR that had to precede it. **ADR-0077 — why an amendment was needed before any code.** ADR-0076 §1 decided the mechanism as "the awaited emit plus an explicit check, NOT a §2-style queue-and-flush", justified by an asymmetry between where the two money events are emitted. That asymmetry does not exist, and the code says so three times: - fallback-chain.ts:148 — `onAttempt?: (record) => void`, synchronous, invoked bare at :902. - agent-turn.ts:783-847 — `settleAtReservedEstimate()` and `cost:updated` are emitted from that ONE callback, a few lines apart. - budget-governor.ts:273 — "The ledger mutation is SYNCHRONOUS ... called from inside the fallback chain's onAttempt callback, WHICH CANNOT AWAIT." So §1 asked for an `await` where none can be placed. node-executor.ts:46-52 had already written the warning for exactly this mistake. ADR-0077 adopts ADR-0074 §2's shape instead: start the write at the settle instant on a chained in-flight promise, join it at THREE barriers — pre-egress, before tool dispatch (the one this adds beyond §2), and the turn/node terminal — each awaiting AND observing, since #emitDurable is total for store faults and resolves. A fold of the ADR review added Decision §4 (one joining entry point owning both money chains, so "await the wrong one" is not expressible) and named the tool-dispatch barrier as the seam CR-12's effect journal will extend. The review also asked whether the session path loses realized cost the same way. It does not, and the ADR now records why with evidence: `recordSessionCost` is `(entry) => void` — a synchronous better-sqlite3 write that `persistDurably` calls inline and re-throws from, so the money is committed before the handler returns. The run path needs a barrier because its store is `persistEvent => Promise<void>` by seam design; the session path is stronger here, not weaker. **Step 1 — the schema.** RUN-only (`runBase`), because the session path already has this ledger. Two fields diverge from their siblings and the divergence is deliberate: `attemptNumber` and `priced` are REQUIRED here where `cost:updated` and `budget:estimate_committed` have them optional. Those two carry historical rows; this type has none, so requiring now is free while requiring LATER would be the one-way door parseStoredRunEvent describes. A ledger row that cannot say which attempt it is, or whether it could be priced, is not a ledger. **Break-verify, with the mutation confirmed applied each time.** Six fixtures, six mutations, all red: drop the refinement; attemptNumber optional; priced optional; attemptNumber nonNegative; costMicrocents as z.number() (reddens both fractional and negative). The first pass was HOLLOW and the mutation check is what caught it. Three reject fixtures omitted `priced` — so they were rejected on the MISSING FIELD and never reached the invariant they were named for; deleting the refinement left the suite green. A fourth then failed on a fractional `cumulativeCostMicrocents` instead of the `costMicrocents` it claimed to test. Every fixture now carries a complete payload with exactly one thing wrong, and says so in a comment. Recorded for steps 3-5: `pnpm turbo run typecheck` passes across all 11 tasks with the new arm unhandled everywhere. No consumer switch has an exhaustiveness guard over RunEvent, so every downstream site must be found by reading, not by the compiler. pnpm run ci EXIT=0. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o home Six findings, three High. All verified against code before folding; the two that mattered are ones I had not reached. **The ledger cannot live on `BudgetGovernor`, and every signpost said it should.** ADR-0077 §1 read "exactly as §2's `#commitmentsInFlight`", and `node-executor.ts`'s comment literally says "add it to the governor's emit type instead". Following that would have been silently wrong, because the governor is conditional and the ledger is not: - `engine.ts:412` builds one only `if (params.plan.budget !== undefined)`; `budget` is optional in the workflow schema. An unbudgeted run spends real money and would have recorded nothing. - `engine.ts:2109` returns early with no governor → barrier B3 is a no-op. - `engine.ts:1190` returns `undefined` → the turn core never installs the `preAttempt` wrapper → barrier B1 does not exist. - `engine.ts:1214` — `budgetApproved ? undefined : this.#makePreEgressHook()` drops the hook for an approved re-dispatch, so B1 vanishes on the exact path the user just authorised more money on. An unbudgeted run would have got one barrier of three; one with no tool calls, none — and every test written against a budgeted fixture would have passed. New Decision §5 makes the owner the engine, unconditionally, names all three holes, and requires the regression to be an UNBUDGETED run. The correct composition already exists one surface over: `session-host.ts:382` installs a preEgress hook that is "ALWAYS present, cap or no cap", reads the durability probe FIRST, then delegates to `governor?.preEgress(info)`. This ADR's own "the session path is stronger here" section had the answer in it. **ADR-0076 §1 now carries the amendment marker**, following the precedent `f1b7773` set when ADR-0075 landed — it edited ADR-0074 §5's heading in place to add "(amended by ADR-0075)". Without it a reader arriving from `current.md`, `constants.ts` or the `run-event.ts` docblock — the likely paths — reads the withdrawn mechanism with nothing on the page saying so. Append-only-legal, and the mitigation ADR-0077 claimed (its own title and Related line) only ever reached someone who had already found ADR-0077. **Two implementation traps recorded**, both silent, both would review as correct: - The ledger emit must FOLLOW the cumulative fold. Emitting first leaves the cumulative stale, `refineCostAttemptSettled` rejects at the producer gate, and that gate runs in `#bus.next` — OUTSIDE `#emitDurable`'s try. So the wrong order does not degrade: `#emitDurable` REJECTS in the one place the design assumes it cannot. - The durable emit must not route through `#nodeEmit`; it returns `void`, so the promise is unawaitable — the unbarriered shape this ADR exists to prevent. Also: "Concretely, three things" listed four (now five); the hook into the turn core is stated as an optional `AgentTurnParams` field like `preEgress`, since `agent-turn.ts` is the boundary `AgentSession` shares. Verified clean by the review and left alone: ADR-0077's three central claims all hold; both required-field divergences are sound and every emit site can supply them; the run-only justification is STRONGER than ADR-0076 states (`event-bus.ts` delivers synchronously, so the session persister's write runs inside `onAttempt`'s own stack, and `session-host.ts:382` gates per-attempt, not per-turn); and all six reject fixtures are honest — one issue each, each the reason its name claims. Correction to `8d7ffcf`'s message: the bare `onAttempt` invocation is `fallback-chain.ts:903`, not `:902` (`:902` is the enclosing `#emit` signature). Still open and next: `sse-event-schema.md` has not been updated, so the canonical home still presents a closed union that no longer is. That is step 2 and lands before step 3. pnpm run ci EXIT=0. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#W15-1 step 2 of 5. The Opus review flagged that step 1 shipped the Zod implementation while the canonical contract still presented a CLOSED union that no longer is — which inverts CLAUDE.md rule 8 and ADR-0076's own sentence that `run-event.ts` is "the Zod validation that implements it, not a second specification". This closes that. Seven insertion points, but two of them are RECONCILIATIONS, not additions — this event makes two existing sentences untrue and appending without fixing them would leave the spec contradicting itself: - The `attemptNumber`-families note said "Run totals are unaffected: `cost:updated.cumulativeCostMicrocents` is the engine's authoritative running total." True only for a LIVE reader. `cost:updated` is never persisted, so a reader reconstructing from the durable log cannot use it at all. - `node:completed.cumulativeCostMicrocents` was "THE durable cost source checkpoint/resume restores from, since `cost:updated` is streamed-only". It is now one of two — and it remains the ONLY carrier of media spend, which emits no attempt row. That is why the restore is a max and not a replacement. So the doc now states the three-way split once, as ADR-0076's last Negative requires and did not have: `cost:updated` = the live observation; `cost:attempt_settled` = the durable record of what was charged; the RESTORE = the greater of (SUM of attempt deltas) and (the node-boundary snapshots). Sum within the family, max across families. Adding them would double-count, because a completed node's snapshot already contains its own attempts. The row goes in the MAIN union table, not the governance table: that section hard-codes "adds four run events", repeats "These four", and declares all of them non-terminal governance/suspension signals. A realized-cost ledger row is neither. Placed last, mirroring its position in the union and in RUN_EVENT_TYPES. Forward-compatibility gets a new paragraph, because this is the FIRST event to exercise ADR-0075's replay carve-out and that is precisely why ADR-0075 landed first: adding it is additive for every rendering consumer and deliberately NOT additive for a replay. "Adding a new event type is never a breaking change" is true of the stream and false of the resume, and the two halves now have to be read together. `run_costs` is cited, not restated — it has its home in database-schema.md (rule 8). The dual-envelope correlation-key paragraph is deliberately untouched: this event is run-only. pnpm run ci EXIT=0. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cost regression #W15-1 step 3 of 5. Two things, and the second was not in the plan. **The new `applyDerived` arm.** One settled attempt becomes a `run_costs` row in the SAME transaction as its event — which is what makes the ledger idempotent with no second uniqueness key, since `UNIQUE(run_id, seq)` already bars a duplicate event and the derived row cannot outlive it. The row is a TELESCOPING delta off the event's cumulative, not the event's raw `costMicrocents` — and I had it the other way round until a break-verify refused to go red. My fixtures all had `cumulative - prev == costMicrocents`, so the two were indistinguishable; the mutation that should have failed passed. Chasing why found the case where they differ, and it is not hypothetical: `#emitDurable` starts every `persistEvent` immediately and serializes only DELIVERY — "persists stay concurrent", in its own comment. So a sibling's `node:completed` can commit FIRST carrying a cumulative that already includes this attempt, and a raw-delta row then adds that money a second time. The new test proves it: raw delta yields 1_400 where the truth is 1_000. Telescoping cannot double-count, because every money event carries an ABSOLUTE cumulative, so the total converges on the largest one seen regardless of commit order — and `SUM(run_costs) == total` holds by construction rather than by hoping the log is ordered. The event still carries the exact per-attempt charge; that is what a reader and the step-5 checkpoint fold sum. Only the derived row telescopes. Per-attempt attribution degrades under a `fan_out` exactly as the per-node delta already does, and now says so. **The step-cost regression ADR-0076 did not see.** `node:completed` computed `nodeCost` once and used it TWICE — the `run_costs` row and `step_executions.costMicrocents`. ADR-0076 property 3 celebrates that delta telescoping to zero and says nothing about the step row. So every agent node's step cost would have become 0, and that column is user-visible through `relavium status --json`. No test pinned it, so it would have shipped silently. `node:failed` shared the bug plus an `if (nodeCost > 0)` guard that would have skipped the write on exactly the failures whose cost matters most. Both now read `nodeSettledCost` — the sum of the node's rows — with the node-retry attribution caveat written down rather than left to be discovered. Also: `run_costs.input_tokens`/`output_tokens` carry the REAL per-attempt tokens (nothing sums those columns; only cost carries ADR-0070's invariant), the run's token totals are NOT bumped here (`node:completed` already adds the sum across attempts), and `modelId` stays unwritten — it is an FK to a catalog UUID that `schema.ts` documents as a dead column, and `event.model` is a raw provider string this store cannot resolve. The `default` arm's comment now says plainly that it is NOT an exhaustiveness guard: `RunEvent` has no `assertNever` anywhere, so the next durable money event added without an arm lands there silently — a row, no derived write, no compile error, no failing test. Seven tests, six break-verify mutations, each with the mutation confirmed applied before trusting the red: drop the `runs` bump; drop the `run_costs` insert; raw delta instead of telescoping; step cost back to `nodeCost` on completed; on failed; and bumping the run token totals. pnpm run ci EXIT=0. Refs: ADR-0070, ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second pass, deliberately aimed where the Opus round did not look. Four findings, all valid. **The required-field defence was half-built.** `attemptNumber` and `priced` got reject fixtures because their required-ness is a DIVERGENCE worth arguing for. `nodeId` and `model` are required too — and had none, so loosening either to `.optional()` reddened nothing. That is the same silent one-way door those other two fixtures exist to hold shut, one field over, and the file's own convention already defends it: `budget:estimate_committed` carries `(no model)`, `budget:paused` carries `(missing/empty nodeId)`. Four fixtures added covering both the missing and the empty-string form; each break-verified with the mutation confirmed applied, and each reddens exactly its own named case. **ADR-0077 used B1/B2/B3 without ever defining them.** The Opus fold introduced the shorthand in the new Decision §5 — the most load-bearing part of the ADR — while §2 listed the three barriers in prose only, and B2 appeared nowhere. §2 now names them where they are introduced. **ADR-0076 §1's amendment marker was scoped too narrowly to do its job.** Two sentences past the marker, the same unmarked paragraph still asserts "the attempt boundary AWAITS that call" as fact, and Consequences > Negative still says "each awaited". A reader who reads past the parenthetical lands on the retracted mechanism stated plainly — which is exactly the failure the marker was added to prevent. Both now carry an inline pointer. Correction to `abc8a6e`'s message: the two guard lines are `engine.ts:1192` (`#makePreEgressHook`'s `return undefined`) and `:2111` (`#flushBudgetCommitments`'s `if (governor === undefined) return`); `:1190` and `:2109` are the enclosing signatures. The ADR body itself cites no line numbers, per the corpus convention, so only the commit message was off. Verified clean by this pass and left alone: ADR-0077's H1 matches its index row; every code citation in it checks out, including all four governor-conditional holes; every factual claim in the schema docblock is true against `persister.ts`, `session_costs`, ADR-0045 §5 and `parseStoredRunEvent`; the accept-boundary (`cumulative == cost`) is already covered by the valid fixture; and all four `RunEvent` consumers are true no-ops on the new type today — none renders a wrong number, which is what made deferring them to steps 4-5 safe rather than lucky. pnpm run ci EXIT=0. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…max, not a sum #W15-1 step 5 of 5. The staged plan said "fold it as a SUM of deltas, never a last-wins snapshot", borrowing ADR-0074 §2's reasoning. Half right, and the wrong half cost two rewrites — both caught by a test, not by review. **First attempt: sum the attempt deltas into the realized total.** Wrong — a node terminal's snapshot already contains the attempts it covers, and both write that field, so it double-counts. **Second attempt: sum into a SEPARATE accumulator, then max the two families.** This is what I wrote, and the motivating-scenario test refused to go green. The two families cover DIFFERENT money: a media node writes a cumulative snapshot and emits no attempt row at all. So on a run where media spend is the larger figure, `max(snapshots, sum(attempts))` silently drops every attempt made after the last node boundary — which is exactly the crash-mid-agent-loop case the ledger exists to fix. The test now pins it explicitly: node `a` is media at 1_000, node `b` then spends 800 and dies before its terminal; the two-family max restores 1_000 and a resume re-spends b's 800. **What is correct is the fold already two arms above it:** `Math.max` over every durable ABSOLUTE total — the node snapshots, `budget:paused.spentMicrocents`, and now `cost:attempt_settled.cumulativeCostMicrocents`. Each is read immediately after its own increment, so each is a true run-wide total and the largest is the engine's real one regardless of commit order. ADR-0074 §2 rejected `Math.max` for the CONSERVATIVE total, and that is why the plan said "sum". The reasoning does not carry: it was rejected only because a future deliberate release would DECREASE that total. Realized spend has no release and is monotonic, which is the one property that makes maxing safe. `sse-event-schema.md` and the schema docblock said "a sum within one family, a max across families" — my own sentence from step 2, now false. Both corrected, each naming BOTH wrong routes so the next reader does not re-derive them. Five tests. Break-verify with the mutation confirmed applied each time: last-wins instead of max, and dropping the arm, both redden; adding the node snapshot instead of maxing it reddens two. **One mutation reddens nothing, and the test says so rather than implying coverage.** Replacing the attempt arm's `Math.max` with `+= costMicrocents` leaves every test green, because on a well-formed log the two are equivalent — a node's attempts always carry a lower `sequenceNumber` than its own terminal, so the delta is summed exactly once before the snapshot maxes over it. `Math.max` is chosen for robustness and for matching the sibling arms, not because a fixture can tell them apart. The gap is named in the test, with the log shape that would close it (a terminal ordered before the attempts it covers) and the note that the engine does not produce one. pnpm run ci EXIT=0. Refs: ADR-0074, ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oins
#W15-1 step 4 of 5, implementing ADR-0077 rather than ADR-0076 §1's withdrawn
"awaited emit at the attempt boundary", which cannot be written: `onAttempt` is
`(record) => void`.
**New `money-durability.ts`, owned by the engine and constructed
UNCONDITIONALLY.** Every signpost pointed at `BudgetGovernor` — ADR-0077 §1's own
"exactly as §2's `#commitmentsInFlight`", and `node-executor.ts`'s "add it to the
governor's emit type instead" — and following any of them would have been
silently wrong: the governor exists only `if (params.plan.budget !== undefined)`,
and an unbudgeted run spends real money. It copies §2's shape field for field
(chained in-flight promise, pending guard, retained failure, sticky broken flag,
`Promise.resolve().then(...)` so a synchronously-throwing store cannot brick the
chain) and adds the thing §4 requires: ONE `join()` that fronts both money
chains, so "await the wrong one" is not expressible.
**Three barrier holes closed, all of which read as correct code:**
- `#flushBudgetCommitments`'s `if (governor === undefined) return` — B3 was a
no-op on every unbudgeted run. It is now the single join.
- the `preAttempt` wrapper gated on `preEgress === undefined` — B1 did not exist
without a governor. Now installed when either the hook or the money port is
present.
- `budgetApproved ? undefined : #makePreEgressHook()` — B1 vanished on an
approved re-dispatch. The money port is threaded unconditionally, because an
approved node is the last place to stop recording what the user just authorised.
**B2 is new**: `await params.money?.join()` immediately before
`dispatchToolCalls`. This is what makes the ledger extend §2's guarantee rather
than repeat it — realized spend durable before the run MUTATES THE WORLD, not
merely before it spends again. One join per tool turn, before the loop.
The emit sits in `onAttempt` strictly AFTER `params.emit({type:'cost:updated'})`,
and the order is load-bearing: `#nodeEmit`'s `cost:updated` arm advances
`#cumulativeCostMicrocents`, which the engine then stamps onto the ledger draft.
Recording first would stamp a stale total, `refineCostAttemptSettled` would
reject it at the producer gate, and that gate runs in `#bus.next` — OUTSIDE
`#emitDurable`'s try — so `#emitDurable` would REJECT in the one place the whole
design assumes it cannot.
Run-only is now a runtime fact, not a comment: `money` is an optional
`AgentTurnParams` field modelled on `preEgress`, `AgentSession` never sets it, and
`agent-runner.ts` reads it from the ctx with no `deps` fallback — a host wiring a
runner directly has no run to record against.
Eight unit tests, all green: serialization, retained-failure-thrown-once, the
sticky flag, secret-free message with the cause on `cause`, the sync-throw brick,
the conservative join, a conservative failure surfacing through the same join,
and the zero-egress no-op.
**The wiring is NOT pinned, and I am naming that rather than implying it is.**
Deleting all three joins leaves 1096 of 1097 core tests green; the one failure is
ADR-0074 §2's existing conservative-ordering e2e, which sees B3 only because B3
now fronts its flush. So nothing today pins B1, B2, or the realized ledger's
ordering at all. Three tests close it, and they belong on the
`m2-e2e-harness.e2e.test.ts` delaying-store harness that already exists for §2:
(1) `cost:attempt_settled` persists before the next attempt's egress, (2) before
the tool registry's dispatch — asserted on a dispatch spy, since that is B2's
whole point, (3) before `node:completed`. Plus ADR-0077's required regression: a
rejecting `persistEvent` must leave the dispatch spy UNCALLED — the mutation that
matters most, because `#emitDurable` resolves on a store fault, so an await-only
barrier passes every ordering test above while observing nothing.
pnpm run ci EXIT=0.
Refs: ADR-0074, ADR-0076, ADR-0077
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#W15-1 step 4's ordering tests, on the delaying-store harness ADR-0074 §2 already established. All four run on HAPPY_PATH, which declares NO `budget` — deliberately, since ADR-0077 §5's finding is that a budgeted fixture passes even with every barrier hole open. Also fixes a real defect the tests exposed in `351f3af`. **The observe half did not work, and the ADR had already named why.** `MoneyDurability`'s `.catch` never fires for a store fault, because `#emitDurable` is TOTAL: it absorbs a `persistEvent` rejection into `#failure` and RESOLVES. So the ledger saw a successful write. The engine's emit wrapper now compares `#failure` across the await and throws when it moved — turning the absorbed fault back into something a barrier can act on. It can over-trigger when a sibling fails in the same window; that direction is fail-closed, which is the right way to be wrong at a money barrier. Two of my four assertions were also wrong about real behaviour, and the tests now say what actually happens: - The terminal message is `#emitDurable`'s "a durable run-event write failed", not the barrier's. Correct, not a miss: `#emitDurable` sets `#failure` first and `??=` keeps the first cause. The barrier's job here is to stop the dispatch, not to rename the failure. - `persistOrder` records `type:nodeId`, not the bare type. HAPPY_PATH's `input` node completes long before the agent spends anything, so the first version of the B3 test asserted on the WRONG node's terminal and would have passed for the wrong reason. **What these four tests actually pin — stated plainly, because it is less than their names suggest.** Deleting B1, B2 or B3 individually reddens NOTHING. Only removing the `record` call in `onAttempt` reddens, and it reddens all four. So they pin that the ledger event is emitted and that a stalled ledger write stalls the run; they do NOT pin that any of the three joins is what stalls it. The likely mechanism is `#emitDurable`'s `#deliveryTail`, which already serializes delivery behind a blocked persist — the same pre-existing serialization that made ADR-0074 §2's own barrier look correct before its flush was removed. That leaves an open question worth answering before this is called done: whether B1/B2/B3 add a guarantee the delivery tail does not already provide, or whether the tail is doing the work and the joins are belt-and-braces. Either answer is fine; shipping without knowing which is not. The instrument that would settle it is a store that resolves the ledger persist but delays only its DELIVERY, so the tail and the joins can be told apart — which the current harness cannot express. Recorded rather than asserted, per this phase's own discipline: a test that implies coverage it does not have is worse than none. pnpm run ci EXIT=0. Refs: ADR-0074, ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…were fine, the tests were not The review settled `50c60bd`'s open question empirically, in a sandbox, and the answer is the opposite of my hypothesis: B1/B2/B3 each add a guarantee the delivery tail does NOT provide. `#emitDurable` awaits `persistEvent` BEFORE `await prior`, so the tail serializes delivery only — a later event's persist still commits while an earlier one is blocked. And the tool dispatch never touches `#emitDurable` at all: `#nodeEmit` routes in-node events through `#bus.emit`, which neither persists nor chains onto the tail. There is no awaited durable emit between `onAttempt` and `dispatchToolCalls`. **The defect was in my tests, not the design.** They asserted one tick after `ledger.blocked()` flipped — which happens INSIDE `persistEvent`, several turns before the run would have reached the guarded action. The dispatch was still sitting in the queue, so the assertion passed with or without a barrier. Draining to quiescence first (microtasks plus a macrotask turn, repeatedly) is the whole fix, and `50c60bd`'s claim that settling this "needs a store that delays only DELIVERY, which the current harness cannot express" was false — the harness was fine, it was sampled too early. Verified: deleting B2 now reddens the tool-dispatch test; deleting B3 reddens the node-terminal test. B1 alone still reddens nothing, and the reason is now known rather than assumed — on this fixture the second egress only comes after the tool, so B2 masks it. Named, not implied. **A money-losing bug found next door.** `checkpoint.ts`'s `budget:paused` arm ASSIGNED `spentMicrocents` while the comment two lines above already claimed the sources "reconcile via that `Math.max` fold". `spentMicrocents` is captured at `checkPreEgress` and the event is emitted much later; under a `fan_out` a sibling attempt settles a HIGHER cumulative in that window and the assign clobbers it — handing a resumed cap headroom for money already spent, the exact bypass ADR-0074/ADR-0076 exist to close. Latent before this work (node boundaries are rare); the ledger writes per attempt and multiplies the high-water marks available to clobber. Now a `Math.max`, with a break-verified regression. **Three false claims in canonical homes, all mine, all corrected:** - `checkpoint.ts`'s field doc listed `run:*` and `budget:paused` as carriers the fold maxes. `run:*` is not folded at all (the engine says so at the emit site), and `budget:paused` was the assign above. The same sentence was duplicated verbatim into `run-event.ts` and `sse-event-schema.md`. - `run-event.ts` still called `costMicrocents` "the field checkpoint reconstruction adds up" twenty lines above the field that says not to sum it — a contradiction inside one Zod object. `83f7990` claimed the docblock was corrected; only the second half was. - `sse-event-schema.md`'s `node:completed` row still described the max-of-two- families design that the same file's fold rule says under-counts, thirteen lines apart in the one canonical file. **The rejected-write test's comment was false and now says so.** What stops the dispatch there is the engine's own abort, not the barrier: deleting the observe half OR B2 leaves it green. ADR-0077's stated required regression is therefore still unmet, and the shape that would meet it is named in the test — reject the ledger write while a sibling has already set `#failure`, so `#emitDurable`'s `this.#failure === undefined` guard skips the abort and only the barrier is left. `current.md`'s staged plan no longer describes the retracted SUM fold, and now records that all five steps are landed. Carried forward, not fixed here, each verified by the review: the observe half uses `#failure` (a "run started failing" signal) where `#emitDurable` could return a per-event outcome — it under-triggers during a cancel and on an already-failed run, and loses the store error as `cause`; `MoneyDurability`'s `#broken` flag is dead and has no production reader, so a broken ledger has no surface, unlike the estimate twin's; `join()` clears `#failure` after throwing, so under a `fan_out` a sibling can consume another node's failure; `agent-turn`'s `throwMappedChainError` rescues `CommitmentDurabilityError` but not its realized twin, so a B1 failure loses its `nodeId` and reports the conservative message; `run:completed` assigns the run total without a matching `run_costs` row, so the "holds by construction" claim is broader than the code; and no test covers the node-retry attribution the `nodeSettledCost` docblock spends a paragraph on. pnpm run ci EXIT=0. Refs: ADR-0074, ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lse claim, one file down The second pass found what the first fold missed, in the file the first fold was correcting three other instances in. **A fourth false claim, and it is the same drift one function lower.** `refineCostAttemptSettled`'s docblock still said "reconstruction sums the deltas (so the RESTORE survives it)" — the design step 5 retracted — twenty lines below the field doc that `9308798` had just corrected to say the opposite. The test fixture repeated it verbatim. So `run-event.ts` contradicted itself internally while a commit message claimed all such claims were corrected. Both now say the cumulative IS the restore path, and record which revision was wrong so the next reader does not re-derive it. **Two standards violations closed:** - `engine.ts`'s observe half threw a bare, uncaused `Error`, against error-handling.md's typed-discriminated rule. Now a `LedgerDurabilityError` carrying the node — which matters more than the class name, because of the fix below. - `agent-turn.ts`'s `throwMappedChainError` had an arm for `CommitmentDurabilityError` and none for its realized twin, so a barrier-B1 failure was flattened into a generic `AgentTurnError`. That arm exists to keep the `nodeId` identifying WHOSE write broke — under a `fan_out` the observing node is not the owner — and to keep `isLedgerDurabilityError` narrowing at the engine's B3 catch. Both were being lost. Correction to `9308798`'s carried-forward list: I described this as "reports the conservative message". The review checked and it does not — the message was already the realized one. What is lost is the typed identity and the node attribution. **B1 is untested and now says so, with the reason rather than a hope.** Deleting it leaves all 1102 core tests green, and the review confirmed no fixture reddens it anywhere. The reason is structural, not a fixture gap: on today's engine every path reaching a SECOND egress after a settled attempt passes through B2 or B3 first — within one chain a settled attempt ends it, so a second egress means another tool round or another node dispatch. B1 is defence in depth against a future path that crosses neither. Kept deliberately, and the comment says that if a later reader proves it unreachable the honest move is to delete it, not to leave an untestable line with a hopeful comment. **Roadmap staleness closed**, both files the review named: `current.md` said "23 of 24 closed" while its own status line said all five steps landed, and `#W15-1` had no ✅ unlike its sibling. The 2.6.5 phase doc was untouched by all nine commits and still called its own prerequisite "staged". It now records the prerequisite satisfied — and, more usefully, the two things `W1` inherits: `CR-10` gains a concrete adversary (the telescoping `run_costs` delta exists because out-of-order commit is real today, not a cloud hypothetical), and `CR-11`/`CR-92` gain a known-open seam (the observe half reads `#failure` because `#emitDurable` discards the store error, which under-triggers on cancel and on an already-failing run — a per-event durable outcome would close it, at the same choke point `CR-10` restructures). Still carried forward, unchanged: `MoneyDurability#broken` is dead with no production reader, so a broken ledger has no surface unlike the estimate twin; `join()` clears the retained failure, so a fan-out sibling can consume another node's; a simultaneous conservative+ledger failure surfaces the conservative one and delays the ledger one by exactly one barrier; `run:completed` assigns the run total with no matching `run_costs` row and no test pins the SUM invariant there; no node-retry test covers the `nodeSettledCost` attribution caveat; and ADR-0077's required regression (the barrier, not the abort, refusing the dispatch) is still unmet — its shape is named in the test. pnpm run ci EXIT=0. Core 1102 green. Refs: ADR-0074, ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`CR-90`, the first item of phase 2.6.5. The defect was live in this working
tree, not a stale note: a root `vitest list` discovered **472** test files,
**234** of them under `.claude/worktrees/<id>/packages/...` — a full agent-tooling
checkout of this same repo, complete with its own sources. So a repo-wide run was
executing a foreign tree's suites, and `pnpm coverage` was counting that tree's
sources as 0%-covered files against the engine floor.
It hid unusually well. `.claude/worktrees/` is excluded through
`.git/info/exclude` — LOCAL and untracked — so `git status` stays clean, a fresh
clone does not carry the rule at all, and Vitest never consulted git in the first
place. After the fix: 238 files, exactly this repo's own.
Both halves of the exclusion are needed and they fail differently. `test.exclude`
stops the foreign TESTS from running; `coverage.exclude` stops its SOURCES from
being counted — without the second, a nested checkout's `packages/*/src/**`
matches the coverage `include` and lands in the report as 0%-covered files,
dragging a per-package threshold under its floor for a reason invisible in any
diff. `defaultExclude` is SPREAD rather than replaced, since setting `exclude`
overrides Vitest's own list and dropping `**/node_modules/**` would be a far worse
collection bug than the one being fixed.
**A list of named locations, deliberately, not a structural predicate.** The
obvious general rule — "a workspace root nested inside another one",
`'**/*/{packages,apps,tools}/**'` — is wrong, and I checked rather than assumed:
it also matches this repo's own `packages/core/src/tools/*.test.ts`, because `**`
eats `packages/core` and leaves `src` for the `*`. A rule that silently drops real
tests is worse than the leak it closes.
**The regression is `tools/test-isolation/check.mjs`, wired into `pnpm run ci`.**
It is a tools check rather than a `*.test.ts` for a structural reason: a test
running inside Vitest cannot observe which files Vitest chose to collect — only a
subprocess can. It plants a SYNTHETIC checkout fixture (per the phase doc: not a
real `git worktree`, which would make the check depend on git state and on the cwd
being a repo at all — the exact non-determinism this item is about), and plants it
under `.worktrees/` rather than `.claude/`, so it exercises a pattern that is NOT
the one that actually bit us and the list stays honest rather than tautological.
Two assertions, and the second is what keeps the fix honest: the fixture must NOT
be collected, and a canary real test file must still BE collected. Without the
canary the cheapest way to pass is an over-broad exclude that drops the repo's own
suites — going green by not running.
Break-verified, mutation confirmed applied, real exit codes checked: emptying
`REPO_LOCAL_CHECKOUTS` exits 1 naming the leaked fixture; adding `**/shared/**`
exits 1 naming the missing canary and reporting 226 collected instead of 238.
`pnpm coverage` now measures only our sources — 96.95% lines, 93.32% branches —
and still exits 0.
pnpm run ci EXIT=0.
Refs: CR-90
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… defect it was written for
Thirteen findings. The two that mattered were both in what I had claimed was the
regression.
**BLOCKER — a false mechanism, committed into a permanent config comment.** I
wrote that the leaked sources drag "the per-package threshold below its floor".
The review ran the counterfactual: with the leak, coverage reports 48.85% lines
against 96.95%, and still exits 0. Vitest matches threshold globs ROOT-RELATIVE
with no implicit leading `**`, so `.claude/worktrees/x/packages/llm/src/a.ts`
matches none of the three per-package globs and lands in the `global` group,
which sets no thresholds and is skipped. What the leak destroys is the reported
number and the lcov/html artifact — real, but not what I said. Stated precisely
now, including that the line becomes genuinely floor-load-bearing the moment
anyone adds a global threshold, because that comment is what a future maintainer
reads to decide whether the line is still needed.
**HIGH — the guard passed green on the exact defect it existed to prevent.** It
planted one fixture under `.worktrees/` and never exercised `**/.claude/**`, the
one entry that actually bit this repo. I fixed that by deriving fixtures FROM the
list — and that fix has the same hole, which the break-verify caught: deleting the
entry also deletes its probe, so the guard re-collected all 234 foreign suites and
reported `✓ 472 file(s) collected, none from a repo-local checkout`.
Fixture-per-entry can only prove every LISTED location is excluded. It cannot
prove the list is COMPLETE. So the guard's primary assertion now consults no list
at all: it walks the collected paths and fails on any whose ancestor carries its
own `pnpm-workspace.yaml` — a second checkout of a pnpm monorepo always does.
Verified: dropping `'**/.claude/**'` now exits 1 with
`234 collected file(s) belong to a SECOND checkout ... .claude/worktrees/wf_.../`.
A structural predicate is safe here precisely because it runs in JS against the
filesystem; the same idea as a glob is what wrongly matched `packages/core/src/tools/`.
**HIGH — the guard was in `pnpm run ci` and not in `ci.yml`**, recreating incident
#312's exact shape, in a file whose own comments cite that incident twice. Added.
This is also the honest answer to "does the fix change CI": no. `.claude/worktrees/`
is hidden by a LOCAL `.git/info/exclude` and CI checks out clean, so both excludes
are no-ops there. The exclusions defend a developer's tree; the guard is the only
CI-facing artifact — and it was the one thing not wired in.
Also fixed, each verified rather than assumed:
- A single canary cannot catch an over-broad exclude that misses its package. The
rejected structural rule drops four files under `packages/core/src/tools/` while
a `packages/shared` canary stays green. Now every one of the six workspaces must
yield tests.
- `vitest list` observes `test.exclude` only, so nothing checked the
`coverage.exclude` half of the acceptance criterion. Checked as config text.
- `execFileSync('npx', …)` contradicted the convention `tools/coverage-gate/run.mjs`
documents in so many words, and breaks on Windows. Now `process.execPath` + a
module-resolved vitest entry, `shell: false`, `cwd: repoRoot`.
- No cwd pinning; stdout parsing that a `projects` config or a colorized run would
break; a POSIX-separator canary compared against `path.relative` output on
Windows. All three closed by `--json` + `relative(repoRoot, file)`.
- The fixture left an empty `.worktrees/` behind and cleanup was not signal-safe.
Now removes the parent it created, and clears before planting so a Ctrl-C'd run
self-heals.
- `.gitignore` gained the tracked entries the local `.git/info/exclude` was
standing in for — the commit diagnosed that gap and then did not fix it.
- `**/worktrees/**` root-anchored to `worktrees/**`: this product's CLI manages git
checkouts, so `apps/cli/src/worktrees/` is plausible, and the undotted pattern
would have deleted its tests from collection and its sources from coverage.
- `lint:test-isolation` moved to the FRONT of the `ci` chain — a collection
regression should not be reported after the whole suite already ran under the
wrong file set. It costs 0.4s.
- Scope is `repo`, not `tooling`; `commit-style.md` enumerates the scopes and
`tooling` is not one. `5f69ddc` used it.
`docs/standards/testing.md`'s CI-gate section now names the guard, and the phase
doc marks CR-90 closed with three corrections to its own recorded evidence: the
re-measured 472 → 238, that the floor was never at risk, and that CI never saw it.
Break-verified, mutation confirmed applied, real exit codes: dropping `.claude`
exits 1 naming the checkout; emptying the list exits 1; removing it from
`coverage.exclude` only exits 1; the over-broad structural rule exits 1 as an
unplantable pattern. Guard runs identically from any cwd and leaves no litter.
pnpm run ci EXIT=0.
Refs: CR-90
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s never exercised
Two HIGH findings, both in the thing I had just called the fix.
**The primary assertion had zero coverage of its own detection logic.** The fold
in `3d8f237` replaced the list-based check with a structural one — any collected
file whose ancestor carries its own `pnpm-workspace.yaml` is in a second checkout
— and then never ran it. `plant()` wrote only `packages/probe/src/{probe.ts,
probe.test.ts}`, no workspace marker, so the fixtures did not shape-match a real
checkout for THAT assertion; and the listed fixtures are (correctly) excluded from
collection, so the walk only ever saw this repo's own paths and returned undefined
every time. The one demonstration that it worked was a manual check against a real
worktree that happened to be sitting on this machine — not repeatable, absent on a
fresh clone and in CI. A refactor breaking the walk would have shipped green
forever.
Closed with a pure-function self-test that runs BEFORE the guard trusts itself: it
plants a positive control carrying a `pnpm-workspace.yaml` at a deliberately
UNLISTED path, asserts the detector flags it, and asserts it does not flag
`packages/core/src/dag.test.ts`. Three break-verified mutations now redden it —
`i = 0` instead of `1`, the wrong marker filename, and a detector that never
matches. The listed fixtures also carry the marker now, so a broken exclusion trips
both assertions and the error names the cause twice.
**`process.exit(1)` from inside the try skipped the `finally`, leaking fixtures.**
Node does not unwind a `finally` on `process.exit`, and `collectedFiles()` called
`fail()` in three places — a broken lockfile, a vitest crash, unparseable JSON. All
three left every fixture on disk. Two parents were gitignored; the third was not:
`plantableDir('**/.claude/**')` strips to `.claude`, so the fixture lands as a
SIBLING of `.claude/worktrees/`, which `.claude/worktrees/` does not cover. The
next plain `pnpm test` would then have collected it — CR-90's own defect,
self-inflicted by its guard's crash path. Those paths now throw; a single top-level
catch cleans up and then reports. Verified with a forced mid-run failure: exit 1,
no fixtures left behind. The two scratch directory names are also gitignored now,
as belt-and-braces.
**Two overclaims corrected, per the phase's "record rather than assert" clause:**
- "A second checkout ALWAYS carries its own `pnpm-workspace.yaml`" — true for
`git worktree add`, `git clone` and a full copy, false for a sparse checkout, a
`--no-checkout` worktree, or a partial rsync of `packages/**`. Such a tree would
pass the primary assertion in silence. Recorded as a limitation in both the guard
and the phase doc, with the second marker that would close it.
- `WORKSPACES` was a hand-maintained six-entry array — the exact shape the primary
assertion was rebuilt to escape. It is now DERIVED by walking the tree for
`*.test.ts`, which is non-circular: the filesystem says which workspaces have
tests, and Vitest must then have found them. `packages/ui`'s first test will be
checked without anyone remembering to add it.
Confirmed clean by the review and left alone: `i = 1` is deliberate and right
(`i = 0` resolves to the repo's OWN marker and would flag every file); the
48.85/96.95, four-files-under-`core/src/tools`, 0.4s and "`vitest list` observes
`test.exclude` only" claims all reproduce exactly; no tracked file is shadowed by
the new `.gitignore` entries.
Two LOWs acknowledged and not changed: `.gitignore`'s `.claude/worktrees/` is
root-anchored while its vitest counterpart is not (`.claude/` only exists at the
root); and `spreadCount < 2` proves two spreads exist, not that they are in the two
right places — a pathological double-mutation is absorbed by the fixture check,
which the documented break-verify confirms.
pnpm run ci EXIT=0.
Refs: CR-90
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… agree `CR-91`, the second half of phase 2.6.5's instrument. The e2e harness could prove a run STREAMED a terminal and that it said what the test expected. That is one view, taken in-process while the engine that produced it is still alive, and it cannot see the failure class this phase exists for: a caller told `completed` while the durable log says `failed`, a restart reconciling to a different terminal, or a checkpoint fold that disagrees with the terminal so a resume does the wrong work. All three are invisible to a live assertion. `checkDurableTruth` compares four views — the live terminal, the durable history, the history a FRESH engine leaves behind after `reconcile()`, and the status `reconstructCheckpointState` derives from the log alone — and returns a structured verdict. `formatDurableTruth` renders the diff. **A verdict rather than a throw**, so callers can assert on the specific disagreement and the oracle can have its own tests. A thrower would force every caller into one message and would itself be awkward to test — which matters here, because this is the instrument `CR-10`, `CR-11`, `CR-12` and `CR-92` will be proven with, and an unproven instrument proves nothing. Two design points, both deliberate: - **Envelope fields are excluded from the comparison.** `timestamp` and `sequenceNumber` move on every restart; comparing them would make every run look like a disagreement while catching nothing. What must agree is the terminal's TYPE and the payload a consumer acts on — outputs, error code, cost. One test pins that envelope drift is NOT a disagreement, so a later "make it stricter" refactor has to argue with a red test. - **The checkpoint view is not decoration.** A resume seeds itself from the fold, so a fold disagreeing with the durable terminal means a resumed run does the wrong work even when every event on disk is intact. Ten unit tests cover the detection logic: the headline live-says-completed / history-says-failed case, a SAME-TYPE terminal whose payload differs (a type-only comparison would pass it), a restart that changes the terminal, a reconcile that adds a second terminal to a closed run, a fold that disagrees, a run that streamed a terminal and persisted none, and the async-store shape. Three e2e tests apply it to real engine runs on all three terminals, restarting a fresh engine over the same store to reconcile — which also pins that `reconcile()` produces NOTHING for a run that already closed, since repairing a run that died WITHOUT a terminal is its job and touching one that landed is the defect. Break-verified: stopping the engine from persisting terminals reddens all three e2e tests with the four-view diff. Coverage 98.36% lines on the new module. pnpm run ci EXIT=0, pnpm coverage EXIT=0. Refs: CR-91 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nstrument the spine
Fourteen findings. The central one invalidated the claim this item exists for.
**It could not express CR-10, CR-11 or CR-12, and three places said it could.**
Measured by the reviewer against the built module: a durable log with a hole,
committed out of order, or with an event AFTER its terminal all verdicted
`agrees=true`. CR-10 IS "the log is not a gap-free ordered prefix". CR-11 has no
ownership concept in the oracle at all; CR-12 is about external effects and needs
a journal view this module does not model. The docblock, the commit message and
the phase doc each claimed all four. They now say what is true — CR-92 fully,
CR-10 partly, CR-11 and CR-12 not at all, budget for their own predicates.
**CR-10's property turns out to be only half-checkable, and finding that out cost
a wrong assertion.** I added a `0..n-1` gap-free check; it failed a perfectly
healthy engine, because the durable log's sequence numbers are a strictly
increasing SUBSEQUENCE — streamed events take numbers and are deliberately never
persisted. A real completed run reads `[0,1,2,3,5,10,11,12,13,14]`. So "no
persisted event is missing from the middle" is NOT expressible from the log alone:
a streamed event's absence is indistinguishable from a lost one. CR-10's
acceptance will need a store harness that records what it was ASKED to persist —
worth knowing now rather than when that test is written. What the log proves
unaided is that it starts at seq 0 and only moves forward, which does catch the
out-of-order commit `#emitDurable` currently permits.
**Four soundness holes, each measured, each closed:**
- **No `runId` binding.** `eventsFor` returning another run's log verdicted
`agrees=true`, and `durableTerminalCount === 1` did not catch it — the other run
has a terminal too. The oracle's own tests all used `eventsFor: () => stored`,
which ignores its argument, so nothing pinned that the id was honoured. CR-11's
scenario is two owners over one store, i.e. exactly this.
- **`reconciledCount` blamed this run for every run reconcile touched.**
`reconcile()` repairs all interrupted runs and returns their events; attributing
the raw array produced a factually wrong message and a false failure. Now
filtered per run.
- **A CORRECT crash repair was reported as a disagreement** — `a restart CHANGED
the terminal: before=none after=run:failed` — so no crash test could use the
oracle, which is most of what CR-91 is for. `expect: 'repaired'` inverts the
pass condition for a run that died mid-flight.
- **`terminalIn` took the FIRST terminal** while `reconstructCheckpointState`
folds status last-wins, so two views read different terminals on a duplicated
log — and a reconcile that APPENDED a second was invisible to the
terminal-changed check. Now the last, with the count owning duplication.
**The comparison was too narrow and too fragile.** `TerminalView` dropped
`error.retryable`, `error.nodeId`, `correlationId` and `totalTokensUsed`: a live
`{tool_failed, nodeId:'A', retryable:false}` compared EQUAL to a durable
`{tool_failed, nodeId:'B', retryable:true}`, and a flipped `retryable` changes
whether a surface offers a retry. `correlationId` is the best discriminator of the
lot — it identifies THIS terminal event. And `JSON.stringify` made `{a:1,b:2}`
disagree with `{b:2,a:1}` (a false failure the first time either side round-trips
through a store, which is the module's whole purpose) and THREW out of the oracle
on a circular or bigint payload, so the instrument became the failure with no
verdict. Both fixed by a canonical, non-throwing stringify.
**The resume view the acceptance criterion names still is not the checkpoint
fold**, and the closure note had substituted one for the other silently. A real
resume goes through the host's `Checkpointer` — the CLI's uses ADR-0075's strict
read that REFUSES a log with an uninterpretable row, which a local fold reads
happily. `loadCheckpoint` now takes the real port and the verdict reports
`checkpointSource`, so a reader can tell which was used. Wiring it is CR-92's job,
and the phase doc says so instead of implying it is done.
Also: exported from `packages/core/src/index.ts` as a supported testing API. It is
engine-pure and ships in `dist` either way, and the surface that most needs it —
`apps/cli`'s harness against the real `history.db`, where CR-92 has to be
certified — is outside this package. The `apps/cli/src/test-support.ts` precedent
does not transfer: that lives in an app, never built as a library.
And the CANCELLED e2e now uses `handle.cancel()`, which swallows
`run_already_terminal`; `engine.cancel(runId)` throws it, and the stream is a
buffered adapter, so a lagging consumer would have thrown inside the `for await`
rather than failing an assertion. Latent, not live — green 5/5 — but the file
already uses the safe idiom three times elsewhere.
Twenty-two unit tests (up from ten), three e2e. Coverage 94.11%.
pnpm run ci EXIT=0, pnpm coverage EXIT=0.
Refs: CR-91
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sed the failure it prevented
Nine findings. The two HIGH ones were both introduced by the previous fold.
**`canonical()`'s cycle guard flagged SHARED references, not circular ones.** The
`WeakSet` was never un-marked on the way back up, so it accumulated every object
seen anywhere in the tree rather than the current lineage. Measured:
`{ a: shared, b: shared }` serialized as `{"a":{"x":1},"b":"[circular]"}` while a
structurally identical payload built from two separate objects serialized
normally — two equal outputs disagreeing. A `fan_in` echoing one value into two
keys is enough to hit it. That is precisely the false-failure-from-a-
serialization-quirk class the function's own docblock says it exists to
eliminate.
Worse, it was unverified: deleting the whole mechanism left all tests green, so
it was both wrong and unexercised. Native `JSON.stringify` already tracks ancestry
correctly, so the fix is to delete the guard and catch its throw — simpler and
right. Pinned by a shared-but-non-circular test that reddens when the flat
`WeakSet` is put back.
**`formatDurableTruth` never rendered the fields the previous fold added.** A pair
differing only in `errorRetryable` or `correlationId` printed BYTE-IDENTICALLY on
both sides of a `DISAGREES` verdict — no visible reason, for exactly the fields
that commit called "the best discriminator of the lot" and "changes whether a
surface offers a retry". `describe()` now renders every compared field, pinned by
a test that asserts both sides appear.
**Three tests proved less than their names.** The retryable/nodeId test flipped
both fields at once, so dropping either comparison alone left it green — it only
proved "at least one is compared". And `correlationId` and `tokens` had NO test at
all: removing either comparison from `sameView` left all 22 green. Now one test
per field via `it.each`, each flipping exactly one, plus a token test. All six
mutations verified red: drop retryable / nodeId / correlationId / tokens compare,
strip `describe()`'s new fields, restore the flat `WeakSet`.
**The phase doc's overclaim survived in the two places the last fold did not
reach** — `:170` (the phase-wide execution-order rationale) and `:829` (inside
CR-91's own acceptance paragraph), both still saying the oracle is what proves
CR-10/11/12/92, ten lines above the corrected table. Both now qualified, with the
original wording preserved and dated so the correction is legible rather than
silent. Test count corrected 20 → 27.
**And the review answered a question I had left open: what CR-92 still needs.**
Recorded in the phase doc rather than discovered later — `live: RunEvent |
undefined` structurally cannot carry "a distinct typed result that is not a
RunEvent", which is what CR-92's acceptance requires the API to return when
durability is uncertain; the resume leg can only ever compare `RunStatus` because
`CheckpointState` carries no payload, so "type and payload" is half-checkable
there; and `expect` has no mode for "durability uncertain, outbox retry pending".
Verified TRUE by the review and left alone: the `[0,1,2,3,5,10,11,12,13,14]` seq
sequence on a real run, `handle.cancel()` swallowing `run_already_terminal` while
`engine.cancel(runId)` throws it, engine purity under the purity gate, and every
other branch of the previous fold (runId binding, per-run reconcile counting,
`expect:'repaired'`, last-terminal, order/head/count checks) mutation-clean with
no double-reporting.
27 unit tests, 3 e2e. Coverage 95.04%; the uncovered lines are the two guards
against MISUSING `expect:'repaired'` on a run that did not crash.
pnpm run ci EXIT=0, pnpm coverage EXIT=0.
Refs: CR-91
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h path Phase 2.6.5's `W0`. Each was half-closed by Wave 1; leaving them half-done is the exact failure this phase corrects. **`CR-01` — `session:cancelled` was the one arm still calling the store bare.** `RunEventBus` isolates a listener throw from the producer, so a failing terminal write let the cancel path report success while the durability latch never set — and the latch is what refuses the next egress. A session whose terminal row never landed would resume believing it had ended cleanly. Now through `persistDurably`, like every sibling. One correction to the finding: the arm writes through `updateSession`, not `writeTurn` — the terminal marks the session `ended` rather than appending a turn. And the self-detach stays OUTSIDE the wrapper, in a `finally`: `persistDurably` re-throws, and a failed write is exactly when a leaked bus listener is worst, since every later event would re-enter a persister that cannot write. **`CR-02` — the counter stays where it is, and the code now says why.** The decision came out of the code rather than being chosen: both catch paths already apply *count a turn only when a provider ENGAGED*, and by the increment `#runTurn` has resolved. A `flushBudgetCommitments` rejection past that point is a durability failure, not evidence the turn never happened — moving the increment below the flush would hand back a turn the provider billed. Asserted through the CAP, never through `#turnCount`: it is a JS private field, and adding a production accessor to observe it would weaken encapsulation to satisfy a test. The regression drives `maxTurns: 1` with a rejecting flush, then proves the next turn is refused with `turn_limit`. Worth recording for whoever reads it: the rejection SURFACES out of `sendMessage` (ADR-0074 §2 fails the active owner loudly), so the test awaits a rejection before driving the cap. **`CR-03` — it was FOUR paths, not three.** `apps/cli/src/commands/import.ts` writes the same shape and was not in the finding; its payload carries `parsed.slug`, straight out of an imported artifact, which makes it if anything the most attacker-reachable of the set. All four now go through `stringifyJsonLine`, whose escape is lossless so no machine contract changes. The regression asserts at the SOURCE rather than driving four command harnesses. What the finding is about is a call site, and a call site is what a future edit reintroduces — driving harnesses would mostly test the harnesses, and the escaping behaviour is already covered in `sanitize.test.ts`. Its hostile payload is built from code points at runtime rather than typed into the file, because a raw C1/bidi byte in source is a Trojan-Source hazard in its own right. Break-verified, each with the mutation confirmed applied: reverting the cancel arm to the bare store call reddens the latch test; moving the turn increment below the flush reddens the cap test; reverting one `--json` surface reddens its row in the per-surface matrix. pnpm run ci EXIT=0. Refs: CR-01, CR-02, CR-03 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o vacuous assertions
Ten findings. Three of my own claims were false, and two of the tests I had just
break-verified turned out to prove nothing.
**`CR-03` was FIVE paths, not four — my headline correction was itself wrong.**
`apps/cli/src/commands/export.ts` writes the identical record shape as the
`import.ts` I had just added, and `docs/reference/cli/commands.md` pairs the two
by that shape in one sentence. I reasoned from the shape and missed the file the
spec names. Worse for the threat model: `export`'s `path` derives from the
user-typed `--out`, so it genuinely can carry a C1 or bidi byte — which brings me
to the second false claim.
**`import --json` is the LEAST attacker-reachable of the set, not the most.** I
justified adding it by saying `parsed.slug` "comes straight out of an imported
artifact". It does — and `kebabIdSchema` rejects the document before that value
exists, so it cannot carry a hostile code point at all. Cheap hygiene, wrongly
ranked, and the ranking was already recorded in the phase doc as the reason the
item's scope grew.
**The source-scanning regression was the wrong mechanism and proved it
immediately.** It matched `writeOut(`${JSON.stringify(` and missed `export.ts`
purely because prettier wrapped the argument onto its own line. A guard that
checks only the places someone remembered catches nothing new by construction —
which is how this gap reopened twice already (`#W15-10`, then `CR-03`). The
call-site half is now an ESLint `no-restricted-syntax` selector firing on the
SHAPE anywhere in `apps/cli/src`; break-verified by reverting `export.ts` to the
prettier-wrapped form the regex could not see. `render-error.ts` is allowlisted —
it pre-strips, so the error envelope is lossy on purpose. The test keeps only the
behavioural assertion.
**`CR-01`'s stated rationale was false, and the test asserted the wrong half.**
The latch cannot refuse a next egress for a TERMINAL event: `session:cancelled`
sets `#status = 'cancelled'` and every later entry point is already refused by
`#assertSendable`. Nothing reads `durabilityFailure` again. The wrapper is
symmetry; the load-bearing half is the `finally`, which stops a throwing write
jumping over the unsubscribe and leaving the persister attached to a bus it can
no longer serve. (The user was told either way — the throw always escaped into
`deliver`'s listener-error sink.)
Two attempts at pinning that were vacuous and BOTH passed with the `finally`
deleted: `expect(close()).not.toThrow()` (`close()` calls the same idempotent
unsubscribe) and driving a second `cancel()` (which emits no event on an
already-cancelled session). The working assertion wraps the handle's unsubscribe
and counts it — `expected +0 to be 1` under the mutation.
**`CR-02`'s decision was right and its accounting stopped one line short.** A
flush rejection settles through the unclassified branch, which emitted `{0,0}` —
so the turn the decision insists the provider billed consumed its cap slot AND
silently dropped its real tokens. That is the mirror of the error the decision
refuses to make, and it contradicts EA2/ADR-0055. The success path now captures
the usage just before the flush; the unclassified terminal reports it.
The mutation for that first "did not apply" — prettier had collapsed the line my
anchor matched — and it silently passed. Caught only by asserting the anchor
exists, which is the whole reason that discipline is written down.
Also: the two comments in `models.ts` / `models-pricing.ts` asserting that
"JSON.stringify escapes any control byte on its own" — the exact false premise
this item exists to correct, left standing in the same package. And
`security-review.md` now carries the machine-output arm of the terminal floor, so
the rule has a normative home instead of living in one TSDoc block.
pnpm run ci EXIT=0.
Refs: CR-01, CR-02, CR-03
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the fence's blind spots
Five findings, no Blocker and no High — the first pass this session where the
implementation itself held up. Every one was about where a rule lives and what it
honestly guarantees.
**The machine-output rule was filed under `## Logging`.** Content accurate, home
wrong: it is a terminal-injection rule, not a confidentiality one, and
`security-review.md` already has `### CLI terminal-render safety` documenting its
human-display sibling — the very bullet the new paragraph calls "the opposite
choice". A reviewer looking for every terminal-injection defense under that
header would not have found half of it. Moved beside its sibling.
**The ESLint fence guarantees less than its docblock claimed.** The reviewer
probed it with a scratch file: it correctly fires through a template literal, a
ternary, an object literal and a `.map()` callback — esquery walks the whole
subtree — but it is blind to three shapes, none of which existed in the docblock:
a destructured writer (`const { writeOut } = io`, whose callee is an `Identifier`
not a `MemberExpression`), indirection through a helper that stringifies and
returns, and a writer under a different name. None is in the tree today, but
"caught the first time it is written" was true only of the inline shape, and this
gap has already reopened twice. Named as residual risk, the way the seam fence
above it names its own.
**The fence did not cover `.tsx`.** `apps/cli/src/**/*.ts` misses nineteen files
under `render/tui/` and `home/` — and the TUI is exactly where a debug or export
affordance would grow one. Widened.
**`sanitize.ts`'s allowlist entry is inert** — verified by removing it: no
diagnostic either way, because the file defines `stringifyJsonLine` and has no
writer call of its own. Kept, but labelled defensive so the next reader does not
assume it is load-bearing. `render-error.ts`'s entry IS load-bearing; removing it
produces a real diagnostic, which is how the distinction was established.
**And the rule had no home in the canonical contract doc.** `commands.md`'s
`### The --json machine-output contract` is where a consumer learns what stdout
holds, and it said nothing about the escaping. Added, with the wire contract
stated as unchanged — `JSON.parse` round-trips identically, which is the whole
reason the escape was chosen over a strip — and cross-linked rather than restated.
The review independently re-derived and confirmed every claim from the previous
fold, by reverting each fix and checking the exact failure: the `finally`'s
`expected +0 to be 1`, the ESLint rule firing on the prettier-wrapped shape the
regex missed, `#lastEngagedUsage` reporting `{0,0}` when captured after the flush,
and `render-error.ts`'s exemption being exercised. It also walked
`#lastEngagedUsage` through every path — compaction, abort, cancel, resumed first
turn, a throw before `#runTurn` resolves — and proved concurrent `sendMessage` is
unreachable (`#status` flips before the first `await`, so the second throws
`not_active`). It can never report a previous turn's tokens.
pnpm run ci EXIT=0.
Refs: CR-01, CR-02, CR-03
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… why the rest is out Two design reviews — one of the YAML definition layer, one of the git-native posture — read against the tree. **Exactly one item belongs in this phase**, and the triage is recorded rather than discarded, because the same documents will be read again. **CR-64 — node-`tools` narrowing is enforced at RUN time while two ADRs say "parser-enforced".** `resolveGrant` lives in `agent-runner.ts`, the node executor, reached only after a run starts; nothing in `parser.ts`, `dag.ts` or `run-plan.ts` touches `tools` at all. ADR-0038 states the opposite in parentheses. The security boundary HOLDS — a widening node is refused at dispatch, ADR-0029(b) is not violated — but a workflow that widens passes `relavium validate`, reviews clean, and fails partway through a run after upstream nodes have spent real money. And a canonical ADR tells the next reader the parser caught it, which is how someone later removes the runtime check as redundant. Exactly the phase's second reason for existing: a claim the code does not keep. **Two of the reviews' own HIGH-priority findings are not real, and the phase doc says so with the evidence:** - "Circular `$ref` has no guard" — it cannot happen. `AgentSchema` is `.strict()` and declares neither `agents:` nor `$ref`, so an agent file cannot reference another agent. Resolution depth is exactly one hop. - "A `1.0` workflow could `$ref` a `0.9` agent" — it cannot. `schema_version` is `z.literal(SCHEMA_VERSION)`, so a file at another version fails its own parse long before cross-file skew is reachable. Checking those two took longer than adding CR-64 would have, and that is the point: a finding added on a reviewer's say-so is a finding the next person has to re-derive. **Everything else is real and belongs elsewhere** — Zod→JSON Schema for IDE validation (the reviews' strongest UX point, and tooling), `expression_type` ergonomics, the edge-condition/condition-node duplication (whose dangerous half is already CR-62), `parallel_of` DRY, "did you mean" errors, property-based schema tests, `merge_strategy` widening, session-export turn grouping (ADR-0026 scoped that deliberately), `reasoning_effort` provider validation (no false claim — ADR-0066 has the adapters withhold it), and the desktop/product items. Putting them here would dilute a phase whose scope line is "no new product surface, only the invariants an existing surface already claims". Two items the reviews raised are already covered and stay where they are: the QuickJS sandbox sits under "What must NOT change", and MCP command injection is CR-16. 46 items now; `current.md`'s count updated. pnpm run ci EXIT=0. Refs: CR-64 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds realized-cost durability, cumulative cost restoration, durable-truth validation, CLI output sanitization, test-isolation checks, session failure handling, and updated product and reliability documentation. ChangesReliability and hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant AgentTurn
participant MoneyDurability
participant RunHistoryStore
participant WorkflowEngine
Provider->>AgentTurn: settle provider attempt
AgentTurn->>MoneyDurability: record realized charge
MoneyDurability->>RunHistoryStore: persist cost:attempt_settled
AgentTurn->>MoneyDurability: join before tool dispatch
MoneyDurability-->>WorkflowEngine: return durable result
WorkflowEngine->>MoneyDurability: join at node barrier
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/core/src/engine/durable-truth.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/core/src/engine/durable-truth.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/core/src/engine/engine.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
Reviewer's GuidePhase 2.6.5 prerequisite work plus reliability/test infrastructure: introduces a durable per-attempt realized-cost ledger and its barriers, a durable-truth oracle for runs, stronger session/agent durability behaviours, stricter test isolation and JSON output safety, and refreshed README/roadmap/ADR documentation to reflect the current architecture and phase plan. Sequence diagram for per-attempt realized-cost ledger barrierssequenceDiagram
participant AgentTurn as AgentTurn
participant Money as MoneyDurability
participant Engine as RunExecution
participant Store as RunHistoryStore
participant Provider as LlmProvider
participant Tools as ToolRegistry
AgentTurn->>Provider: generate()
Provider-->>AgentTurn: AttemptRecord
AgentTurn->>Engine: emit(cost:updated)
AgentTurn->>Money: record(SettledAttemptDraft)
Note over Money,Store: MoneyDurability chains emit via emitDurable
Money->>Engine: emit(cost:attempt_settled)
Engine->>Store: persistEvent(cost:attempt_settled)
Store-->>Engine: ok
alt Barrier B1 preAttempt
AgentTurn->>Money: join()
Money->>Engine: join()
Engine->>BudgetGovernor: flushCommitments()
BudgetGovernor-->>Engine: ok
end
AgentTurn->>Tools: dispatchToolCalls()
alt Barrier B2 before tool dispatch
AgentTurn->>Money: join()
Money->>Engine: join()
end
Tools-->>AgentTurn: ToolResult
AgentTurn->>Engine: node terminal (node:completed)
alt Barrier B3 at node terminal
AgentTurn->>Money: join()
Money->>Engine: join()
Engine->>BudgetGovernor: flushCommitments()
end
Engine->>Store: persistEvent(node:completed)
Store-->>Engine: ok
Sequence diagram for the durable-truth oracle checkDurableTruthsequenceDiagram
participant Test as M2E2EHarnessTest
participant Oracle as checkDurableTruth
participant Store as RunStore
participant Engine as WorkflowEngine
participant Checkpoint as reconstructCheckpointState
Test->>Store: eventsFor(runId)
Store-->>Test: RunEvent[]
Test->>Engine: reconcile()
Engine-->>Test: RunEvent[] (repaired)
Test->>Oracle: checkDurableTruth({ runId, live, eventsFor, reconcile, loadCheckpoint? })
activate Oracle
Oracle->>Store: eventsFor(runId)
Store-->>Oracle: durableEvents
Oracle->>Checkpoint: reconstructCheckpointState(durableEvents)
Checkpoint-->>Oracle: CheckpointState
Oracle-->>Test: DurableTruthVerdict
deactivate Oracle
Test->>Test: expect(verdict.agrees).toBe(true)
Test->>Test: formatDurableTruth(verdict) for diagnostics
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
packages/core/src/engine/durable-truth.ts (2)
122-122: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a codepoint sort instead of
localeCompare.
localeCompareuses ICU collation. Its ordering depends on the runtime locale data. This module targets Node, the Tauri WebView, VS Code, and Bun, so two environments can order the same keys differently. Both compared views are canonicalized in the same process today, so this is not a live defect. A codepoint comparison removes the environment dependency and is faster.♻️ Proposed change
- return Object.fromEntries(Object.entries(val).sort(([a], [b]) => a.localeCompare(b))); + return Object.fromEntries(Object.entries(val).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/durable-truth.ts` at line 122, Replace the locale-dependent comparator in the Object.entries sorting expression with a deterministic codepoint-based comparison, preserving the existing key ordering and Object.fromEntries reconstruction.
249-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
checkDurableTruthto clear the SonarCloud complexity gate.SonarCloud reports cognitive complexity 39 against a limit of 15, and the check is failing. The function is a linear sequence of independent checks, so extraction is mechanical and behavior-preserving. Extract one helper per concern, each returning
readonly string[]:
checkRunBinding(input, before)— lines 259 to 268.checkLiveVsHistory(expectRepaired, live, history)— lines 273 to 289.checkLogOrder(ours, terminalCount)— lines 346 to 367.checkCheckpoint(...)— lines 372 to 379.Then
checkDurableTruthconcatenates the results. The reconcile branch keeps itsawaitand stays in place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/durable-truth.ts` at line 249, Reduce cognitive complexity in checkDurableTruth by extracting checkRunBinding(input, before), checkLiveVsHistory(expectRepaired, live, history), checkLogOrder(ours, terminalCount), and checkCheckpoint(...) helpers, with each returning readonly string[]. Have checkDurableTruth concatenate their results while preserving the existing check order, behavior, and reconcile branch await in place.Source: Linters/SAST tools
packages/core/src/engine/durable-truth.test.ts (3)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
outputsparameter instead of asserting it.Line 20 asserts
unknowntoRecord<string, unknown>. The coding guidelines forbid unsafe type assertions. Every call site passes an object literal, so typing the parameter removes the assertion and keeps the same coverage.♻️ Proposed change
-const completed = (seq: number, outputs: unknown = { a: 1 }, cost = 500): RunEvent => ({ +const completed = ( + seq: number, + outputs: Record<string, unknown> = { a: 1 }, + cost = 500, +): RunEvent => ({ type: 'run:completed', ...base(seq), - outputs: outputs as Record<string, unknown>, + outputs,As per coding guidelines: "Use strict TypeScript for all source code; do not use
anyor unsafe type assertions, and prefer type guards."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/durable-truth.test.ts` around lines 17 - 24, Update the completed helper’s outputs parameter to use the required record type directly, then remove the `as Record<string, unknown>` assertion from the returned run:completed event. Preserve the existing default object and all call-site behavior.Source: Coding guidelines
313-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
as neverwith the error type from theRunEventcontract.Line 317 asserts the error object to
never. That silences every type check on it. A misspelled field such asnodID, or an invalidcodevalue, would compile and the test would still pass while asserting nothing about the real contract. The coding guidelines forbid unsafe type assertions.Derive the parameter type from
RunEventinstead. The assertion then disappears and the fixture stays as short.♻️ Proposed change
+type FailedError = Extract<RunEvent, { type: 'run:failed' }>['error']; + /** A `run:failed` differing from the baseline in exactly one error field. */ - const failedWith = (error: Partial<Record<string, unknown>>): RunEvent => ({ + const failedWith = (error: Partial<FailedError>): RunEvent => ({ type: 'run:failed', ...base(1), - error: { code: 'tool_failed', message: 'boom', retryable: false, ...error } as never, + error: { code: 'tool_failed', message: 'boom', retryable: false, ...error }, partialOutputs: {}, });As per coding guidelines: "do not use
anyor unsafe type assertions, and prefer type guards."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/durable-truth.test.ts` around lines 313 - 319, Update the failedWith fixture to derive its error parameter type from the error field of the RunEvent contract, then construct the error without the as never assertion. Preserve the existing default fields and override behavior while allowing TypeScript to validate field names and code values.Source: Coding guidelines
278-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two unchecked
expect: 'repaired'branches.The suite covers the pass case and the nothing-repaired case. Two
expectRepairedguards indurable-truth.tshave no test:
- Lines 275 to 279: the log already holds a terminal, so the run did not need repair.
- Lines 280 to 284: the live stream produced a terminal, so the run did not die mid-flight.
Both push a disagreement string. Without a test, a later refactor can drop either guard and stay green.
💚 Proposed tests
+ it('under expect:"repaired", an already-terminated log is the failure', async () => { + const { eventsFor } = log(completed(1)); + const verdict = await checkDurableTruth({ + runId: 'r1', + live: undefined, + expect: 'repaired', + eventsFor, + reconcile: () => Promise.resolve([]), + }); + + expect(verdict.agrees).toBe(false); + expect(verdict.disagreements.some((d) => d.includes('already holds a terminal'))).toBe(true); + }); + + it('under expect:"repaired", a live terminal means the run did not die', async () => { + const stored: RunEvent[] = [started]; + const verdict = await checkDurableTruth({ + runId: 'r1', + live: completed(1), + expect: 'repaired', + eventsFor: () => stored, + reconcile: () => { + stored.push(failed(1)); + return Promise.resolve([failed(1)]); + }, + }); + + expect(verdict.agrees).toBe(false); + expect(verdict.disagreements.some((d) => d.includes('died without a terminal'))).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/durable-truth.test.ts` around lines 278 - 311, Add two tests covering the remaining expect:'repaired' disagreement branches in checkDurableTruth: one where eventsFor already contains a durable terminal, and another where live provides a terminal. Assert each verdict disagrees and includes the corresponding repair-not-needed disagreement, while preserving the existing pass and nothing-repaired tests.packages/core/src/engine/m2-e2e-harness.e2e.test.ts (1)
1188-1193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the queue drain condition-based instead of a fixed 50-turn sleep.
settle()always runs 50 macrotask turns, so every call costs a fixed real delay even when the queue is already quiet. The two spin loops at Line 1242 and Line 1265 have the mirror problem: they only drain microtasks, so a step that needs a macrotask turn would leaveledger.blocked()false and fail the assertion that follows. One helper that waits on a predicate, with a bounded turn budget, covers both cases and removes the fixed cost.♻️ Suggested helper
async function settle(): Promise<void> { for (let i = 0; i < 50; i += 1) { for (let j = 0; j < 200; j += 1) await Promise.resolve(); await new Promise((resolve) => setTimeout(resolve, 0)); } } + + /** Drain the queue until `ready()` holds, or the turn budget runs out. */ + async function settleUntil(ready: () => boolean): Promise<void> { + for (let i = 0; i < 50 && !ready(); i += 1) { + for (let j = 0; j < 200; j += 1) await Promise.resolve(); + await new Promise((resolve) => setTimeout(resolve, 0)); + } + }Then replace each spin loop with
await settleUntil(ledger.blocked);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts` around lines 1188 - 1193, Replace the fixed-turn settle() implementation with a condition-based helper, such as settleUntil, that repeatedly yields microtasks and macrotasks until its predicate is true, while enforcing a bounded turn budget to prevent hangs. Update both spin-loop call sites around the ledger.blocked() assertions to use await settleUntil(ledger.blocked), preserving the existing assertions and removing the fixed 50-turn delay.packages/core/src/engine/money-durability.test.ts (1)
93-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert recovery on the same instance, not a fresh one.
The comment states "The chain is still usable", but the assertion runs against a new
healthyinstance. A freshMoneyDurabilityproves nothing about the instance whose sink threw. Record a second, successful write on the originalmoneyinstance instead.💚 Suggested test change
- // The chain is still usable: a later successful write joins cleanly rather than re-throwing forever. - const seen: string[] = []; - const healthy = new MoneyDurability({ - emit: (d) => { - seen.push(d.nodeId); - }, - }); - healthy.record(draft('b')); - await expect(healthy.join()).resolves.toBeUndefined(); - expect(seen).toEqual(['b']); + // The SAME chain is still usable: a later successful write joins cleanly rather than re-throwing forever. + const seen: string[] = []; + let fail = true; + const recovering = new MoneyDurability({ + emit: (d) => { + if (fail) throw new Error('sync boom'); + seen.push(d.nodeId); + }, + }); + recovering.record(draft('a')); + await expect(recovering.join()).rejects.toSatisfy(isLedgerDurabilityError); + fail = false; + recovering.record(draft('b')); + await expect(recovering.join()).resolves.toBeUndefined(); + expect(seen).toEqual(['b']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/money-durability.test.ts` around lines 93 - 103, Update the recovery test to reuse the original money instance whose sink previously threw, rather than creating a new healthy MoneyDurability instance. Record the later successful draft and assert join resolves without error and emits the expected nodeId through the original instance.packages/core/src/engine/engine.ts (1)
2160-2166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
#flushBudgetCommitmentsto match its new scope.The method now joins both money chains, not only the conservative commitments. The name and the doc block above it still describe a budget-only barrier. A reader who greps for the realized-ledger join will not find it here.
♻️ Suggested rename
- async `#flushBudgetCommitments`(nodeId: string): Promise<void> { + async `#joinMoneyDurability`(nodeId: string): Promise<void> {Update the call site in
#dispatchand the doc block above the method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/engine.ts` around lines 2160 - 2166, Rename `#flushBudgetCommitments` to a name that reflects joining both money chains, and update its doc block accordingly. Replace the corresponding invocation in `#dispatch`, preserving the existing MoneyDurability.join() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/chat/persister.test.ts`:
- Around line 284-303: Ensure Vitest mocks are restored even when assertions
fail by adding a suite-level afterEach hook that calls vi.restoreAllMocks().
Remove the test-local restoreAllMocks call from the failing persistence test so
updateSession cannot leak its SQLITE_BUSY mock into subsequent tests.
In
`@docs/decisions/0077-realized-cost-ledger-uses-the-conservative-commitment-barrier.md`:
- Around line 7-214: Condense this ADR into a concise MADR document while
preserving the decision, constraints, rejected alternatives, and consequences.
Remove repeated rationale, implementation-level sequencing, private symbol
names, and detailed code-path discussion; replace operational details with links
to the canonical contracts and implementation. Keep the document append-only and
retain only architecture-level context needed to explain the engine-owned
chained durability barrier and its three required checkpoints.
In `@docs/reference/contracts/sse-event-schema.md`:
- Around line 95-97: Update docs/reference/contracts/sse-event-schema.md:95-97
to identify cost:attempt_settled.cumulativeCostMicrocents as a durable absolute
input for restoration and explicitly state that costMicrocents deltas must not
be summed. Update the interface comments at
docs/reference/contracts/sse-event-schema.md:231-242 to apply the same
Math.max-over-absolute-totals rule; both sites require documentation-only
changes with consistent wording.
In `@docs/roadmap/current.md`:
- Line 5: Synchronize the roadmap metadata and status with
phase-2.6.5-core-reliability-remediation: update the Last updated date to August
11, 2026 and revise the CR-01–CR-03 status from half-closed to closed, unless
the one-day discrepancy is intentionally retained and explicitly documented.
- Around line 92-94: Verify the canonical Phase 2.6.5 count against the phase
document and related references, then update the roadmap graph’s P265 label and
the corresponding lines around the Phase 2.6.5 summary to use that single count
consistently. Preserve the existing graph structure and wording aside from
correcting the item count.
- Around line 248-250: Correct the roadmap closure metadata near the status
summary so PR `#81` records only its 23 completed items on 2026-08-09, while
`#W15-1` is attributed to PR `#82` with its 2026-08-10 landing date. Keep the
24-item total and surrounding verification details consistent with the separated
PR records.
In `@docs/roadmap/phases/phase-2.6.5-core-reliability-remediation.md`:
- Around line 317-323: Update the remediation and acceptance text in the CR-03
section to refer to all five serializer paths, including import.ts and
export.ts, while preserving the original three-path finding in the historical
evidence. Ensure the shared adversarial test requirement explicitly covers all
five surfaces and retains the C1/bidi and JSON.parse round-trip assertions.
In `@packages/core/src/engine/durable-truth.ts`:
- Around line 327-332: Update checkDurableTruth in
packages/core/src/engine/durable-truth.ts at lines 327-332, 271, 301, and 369 to
compute and reuse the input.runId-filtered ours array before all event checks:
pass filtered before and finalEvents to terminalIn, count terminals from ours,
and pass ours to reconstructCheckpointState. In
packages/core/src/engine/durable-truth.test.ts lines 175-191, assert the
foreign-log verdict has durableTerminalCount equal to 0.
In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts`:
- Around line 1275-1277: Update the assertion around ledger.persistOrder to
first verify that both `cost:attempt_settled:work` and `node:completed:work` are
present, then compare their indices to preserve the required ordering. Do not
rely on the current `indexOf` comparison alone, since a missing event returns
-1.
In `@packages/db/src/run-history-store.ts`:
- Around line 544-549: Update nodeSettledCost to attribute costs to the specific
node-retry execution identity rather than aggregating only by runId and nodeId.
Persist or derive that identity on each settled-cost row, use it when
aggregating step_executions.costMicrocents, and do not use the within-chain
attemptNumber because it resets across node retries.
In `@README.md`:
- Around line 102-106: Replace the rendered SVG under the README Architecture
section with an editable Mermaid diagram, preserving the documented architecture
content. Keep the Mermaid source directly in README.md and remove the image-only
representation.
In `@tools/test-isolation/check.mjs`:
- Around line 128-137: Replace the global spreadCount check with validation
targeted to the test.exclude and coverage.exclude properties in the
configuration text. Inspect each property’s bounded syntax region and assert
that `...REPO_LOCAL_CHECKOUTS` appears exactly once in both, failing with the
existing diagnostic flow when either is missing or duplicated.
---
Nitpick comments:
In `@packages/core/src/engine/durable-truth.test.ts`:
- Around line 17-24: Update the completed helper’s outputs parameter to use the
required record type directly, then remove the `as Record<string, unknown>`
assertion from the returned run:completed event. Preserve the existing default
object and all call-site behavior.
- Around line 313-319: Update the failedWith fixture to derive its error
parameter type from the error field of the RunEvent contract, then construct the
error without the as never assertion. Preserve the existing default fields and
override behavior while allowing TypeScript to validate field names and code
values.
- Around line 278-311: Add two tests covering the remaining expect:'repaired'
disagreement branches in checkDurableTruth: one where eventsFor already contains
a durable terminal, and another where live provides a terminal. Assert each
verdict disagrees and includes the corresponding repair-not-needed disagreement,
while preserving the existing pass and nothing-repaired tests.
In `@packages/core/src/engine/durable-truth.ts`:
- Line 122: Replace the locale-dependent comparator in the Object.entries
sorting expression with a deterministic codepoint-based comparison, preserving
the existing key ordering and Object.fromEntries reconstruction.
- Line 249: Reduce cognitive complexity in checkDurableTruth by extracting
checkRunBinding(input, before), checkLiveVsHistory(expectRepaired, live,
history), checkLogOrder(ours, terminalCount), and checkCheckpoint(...) helpers,
with each returning readonly string[]. Have checkDurableTruth concatenate their
results while preserving the existing check order, behavior, and reconcile
branch await in place.
In `@packages/core/src/engine/engine.ts`:
- Around line 2160-2166: Rename `#flushBudgetCommitments` to a name that reflects
joining both money chains, and update its doc block accordingly. Replace the
corresponding invocation in `#dispatch`, preserving the existing
MoneyDurability.join() behavior.
In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts`:
- Around line 1188-1193: Replace the fixed-turn settle() implementation with a
condition-based helper, such as settleUntil, that repeatedly yields microtasks
and macrotasks until its predicate is true, while enforcing a bounded turn
budget to prevent hangs. Update both spin-loop call sites around the
ledger.blocked() assertions to use await settleUntil(ledger.blocked), preserving
the existing assertions and removing the fixed 50-turn delay.
In `@packages/core/src/engine/money-durability.test.ts`:
- Around line 93-103: Update the recovery test to reuse the original money
instance whose sink previously threw, rather than creating a new healthy
MoneyDurability instance. Record the later successful draft and assert join
resolves without error and emits the expected nodeId through the original
instance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d683ea8d-11b7-4045-be79-af1c476dc594
⛔ Files ignored due to path filters (3)
assets/readme/agent-to-workflow.svgis excluded by!**/*.svgassets/readme/relavium-architecture.svgis excluded by!**/*.svgassets/readme/relavium-hero.svgis excluded by!**/*.svg
📒 Files selected for processing (46)
.github/workflows/ci.yml.gitignoreREADME.mdapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat-export.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/export.tsapps/cli/src/commands/import.tsapps/cli/src/commands/models-pricing.tsapps/cli/src/commands/models.tsapps/cli/src/render/json-line-surfaces.test.tsdocs/decisions/0076-durable-per-attempt-realized-cost-ledger.mddocs/decisions/0077-realized-cost-ledger-uses-the-conservative-commitment-barrier.mddocs/decisions/README.mddocs/reference/cli/commands.mddocs/reference/contracts/sse-event-schema.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mddocs/roadmap/phases/phase-2.6.5-core-reliability-remediation.mddocs/standards/security-review.mddocs/standards/testing.mdeslint.config.mjspackage.jsonpackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/checkpoint.test.tspackages/core/src/engine/checkpoint.tspackages/core/src/engine/durable-truth.test.tspackages/core/src/engine/durable-truth.tspackages/core/src/engine/engine.tspackages/core/src/engine/m2-e2e-harness.e2e.test.tspackages/core/src/engine/money-durability.test.tspackages/core/src/engine/money-durability.tspackages/core/src/engine/node-executor.tspackages/core/src/index.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/shared/src/constants.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.tstools/test-isolation/check.mjsvitest.config.ts
Comment on lines
+7
to
+214
| ## Context | ||
|
|
||
| [ADR-0076](0076-durable-per-attempt-realized-cost-ledger.md) decided that a settled provider attempt's realized | ||
| charge becomes a durable run event, `cost:attempt_settled`. That decision is right and is not reopened here. | ||
|
|
||
| Its §1 also decided **how** the write becomes a barrier, and drew a line against its own sibling: | ||
|
|
||
| > **The mechanism is the awaited emit plus an explicit check, NOT a §2-style queue-and-flush.** §2 needs | ||
| > `flushBudgetCommitments` because a conservative commitment is emitted fire-and-forget from inside the | ||
| > governor, so something has to join the outstanding writes at the turn boundary. A settled attempt is emitted | ||
| > by the engine at the point it settles, on the path that is about to continue, so no queue is needed — and | ||
| > adding one would introduce the very concurrency the await removes. | ||
|
|
||
| **The asymmetry that paragraph rests on does not exist**, and the code says so in three places: | ||
|
|
||
| 1. The seam's attempt observer is synchronous and returns nothing — | ||
| `onAttempt?: (record: AttemptRecord) => void` ([fallback-chain.ts](../../packages/llm/src/fallback-chain.ts)), | ||
| invoked as a bare synchronous call inside the chain's attempt loop. | ||
| 2. Both money events are emitted from **that same callback, a few lines apart** — the conservative | ||
| `settleAtReservedEstimate()` and the realized `cost:updated` sit in one `onAttempt` body | ||
| ([agent-turn.ts](../../packages/core/src/engine/agent-turn.ts)). There is no "engine path that is about to | ||
| continue" for one of them and not the other; there is one synchronous observer for both. | ||
| 3. The governor's own field documentation already states the constraint in plain words: *"The ledger mutation | ||
| is SYNCHRONOUS — `settleAtReservedEstimate` is called from inside the fallback chain's `onAttempt` callback, | ||
| **which cannot await**"* ([budget-governor.ts](../../packages/core/src/engine/budget-governor.ts)). | ||
|
|
||
| So §1 asked for an `await` at a point where no `await` can be placed. Worse, the codebase had already written | ||
| the warning for exactly this mistake: [node-executor.ts](../../packages/core/src/engine/node-executor.ts)'s | ||
| comment on why `budget:estimate_committed` is absent from the streamed in-node event union ends with *"If you | ||
| came here to add it, add it to the governor's emit type instead."* | ||
|
|
||
| Two things §1 got **right** are worth keeping explicit, because this ADR narrows one paragraph and not the | ||
| section: | ||
|
|
||
| - `#emitDurable` is **total for store faults** — a `persistEvent` rejection is absorbed into the run's failure | ||
| state and the promise RESOLVES. Awaiting it alone is therefore never a barrier. §1 named this trap and the | ||
| trap is real; it survives unchanged. | ||
| - The guarantee §1 wanted — *written before the next thing that can spend or mutate* — is the right guarantee. | ||
| Only its shape was wrong. | ||
|
|
||
| ## Decision | ||
|
|
||
| **`cost:attempt_settled` uses the same mechanism ADR-0074 §2 built for its estimate twin: the emit is started | ||
| synchronously at the settle point onto a chained in-flight promise, and JOINED at every barrier that precedes | ||
| spending or mutating. It is not a bare awaited emit.** | ||
|
|
||
| Concretely, five things: | ||
|
|
||
| 1. **Start the write at the settle instant.** `onAttempt` cannot await, but it can *begin*. The emit is chained | ||
| onto a per-owner in-flight promise, copying the shape of §2's `#commitmentsInFlight` — which also serializes | ||
| the writes, so two settles in the same tick cannot interleave their persists. **The shape is copied; the | ||
| owner is not** — see §5. | ||
| 2. **Join at three barriers, not two.** §2 joins at the pre-egress check and at the turn/node boundary. | ||
| ADR-0076's guarantee names a third thing to precede — the next **tool side effect** — so the ledger's | ||
| barrier set is, and these three names are used throughout this ADR: | ||
|
|
||
| - **B1** — before the next egress admission (§2 has this one); | ||
| - **B2** — **before tool dispatch** (new here); | ||
| - **B3** — at the turn/node terminal (§2 has this one). | ||
|
|
||
| **B2** is what this ADR adds to the §2 shape; without it the ledger would repeat §2's coverage rather than | ||
| extend it, and the duplicate-effect window ADR-0076 exists to narrow would stay open on the path that | ||
| mutates the world. | ||
| 3. **Every barrier awaits AND observes.** Because `#emitDurable` is total, a barrier that only awaits proceeds | ||
| on a run whose ledger write did not land. Each barrier must therefore also read the failure state and refuse | ||
| to continue, and §2's retained-failure pattern (`#commitmentFailure`, re-thrown at the next barrier) is the | ||
| shape that carries it — a rejection nobody awaits at the call site would otherwise be unhandled. | ||
| 4. **One join, not two.** After this ADR there are two chained in-flight promises — the conservative one and | ||
| the realized one — and three barriers that must join both. Joining them individually is a correctness bug | ||
| waiting for the next barrier someone adds. So the barriers call **one** joining entry point that owns both | ||
| chains and both retained failures; there is no supported way to await half the money. This is what turns | ||
| the "a future barrier joins only one chain" hazard from a review item into something a caller cannot | ||
| express. | ||
| 5. **The ledger and its join are owned by the ENGINE, unconditionally — never by `BudgetGovernor`.** Reading | ||
| §1's "as §2's `#commitmentsInFlight`" as "put it on the governor" would silently omit most runs, and every | ||
| signpost in the codebase points that way (`node-executor.ts`'s comment even says "add it to the governor's | ||
| emit type instead"). It is wrong here, because the governor is **conditional and the ledger is not**: | ||
|
|
||
| - `engine.ts` constructs a `BudgetGovernor` only `if (params.plan.budget !== undefined)`, and `budget` is | ||
| optional in the workflow schema. An unbudgeted run spends real money and would get no ledger at all. | ||
| - `#flushBudgetCommitments` returns immediately when there is no governor, so **barrier B3 becomes a | ||
| no-op**. | ||
| - `#makePreEgressHook()` returns `undefined` without a governor, so the turn core never installs the | ||
| `preAttempt` wrapper and **barrier B1 does not exist**. | ||
| - Even on a BUDGETED run, `const preEgress = budgetApproved ? undefined : this.#makePreEgressHook()` drops | ||
| the hook for an approved `pause_for_approval` re-dispatch — so B1 disappears on the exact path the user | ||
| just authorised more money on. | ||
|
|
||
| Implemented literally, an unbudgeted run would get one barrier of three, and an unbudgeted run with no tool | ||
| calls would get none — the fire-and-forget state this ADR exists to remove, passing every test written | ||
| against a budgeted fixture. | ||
|
|
||
| **The correct shape already exists one surface over.** `session-host.ts` installs an `preEgress` hook that | ||
| is *"ALWAYS present, cap or no cap"*, reads the durability probe FIRST, and only then delegates to | ||
| `governor?.preEgress(info)` — *"It composes with the governor rather than replacing it, and it runs | ||
| FIRST."* The run path never got that. So this ADR's own "the session path is stronger here" section had the | ||
| answer to this flaw in it; the run path adopts the same composition. | ||
|
|
||
| ### Two implementation traps this decision creates | ||
|
|
||
| Named because both are silent, and both would look correct in review: | ||
|
|
||
| - **The ledger emit must follow the cumulative fold, not precede it.** The engine advances its run-wide total | ||
| in `#nodeEmit`'s `cost:updated` arm. Emitting the ledger draft *before* that leaves the cumulative stale, and | ||
| `refineCostAttemptSettled` rejects `cumulative < cost` at the producer gate — which runs in `#bus.next`, | ||
| **outside** `#emitDurable`'s `try`. So the wrong order does not degrade: `#emitDurable` **rejects** instead | ||
| of resolving, in the one place the whole design assumes it cannot. Nothing pins that order today. | ||
| - **The durable emit must not be routed through `#nodeEmit`.** It returns `void`, so the promise would be | ||
| unawaitable — the exact unbarriered shape this ADR exists to prevent, reached through the door | ||
| `node-executor.ts`'s in-node-event-union comment already warns about. | ||
|
|
||
| The hook that carries the emit into the turn core is an OPTIONAL `AgentTurnParams` field, modelled on | ||
| `preEgress`, because `agent-turn.ts` is the boundary `AgentSession` shares. Leaving it unset on the session | ||
| path is what makes this event run-only as a runtime fact rather than a comment. | ||
|
|
||
| ### Why the tool-dispatch barrier matters beyond this ADR | ||
|
|
||
| It is the first place the engine is required to reach a durability checkpoint *before dispatching a side | ||
| effect*, and that checkpoint is the seam the durable **effect journal** (`CR-12` in the 2.6.5 phase) needs: | ||
| `prepared → dispatched → committed | ambiguous` has to be written at exactly this point in exactly this path. | ||
| Landing it here means the effect journal extends an existing barrier rather than threading a new one through | ||
| `ToolRegistry.dispatch`. Named so the two are not built twice, and so a reviewer of either can check the other. | ||
|
|
||
| Considered and rejected: | ||
|
|
||
| - **Make the seam's `onAttempt` awaitable** (`void | Promise<void>`, awaited by the chain), so §1's sentence | ||
| becomes literally true. Rejected on three counts. It widens `@relavium/llm`'s public API for a concern that | ||
| is not the seam's — durability belongs to the engine, and [ADR-0011](0011-internal-llm-abstraction.md) keeps | ||
| the seam a provider contract, not an execution-host one. It would let a durable store write block a live | ||
| provider stream, and an observer that hangs would stall the chain with no timeout of our own. And it would | ||
| give the two money events emitted from one callback two different durability mechanisms, which is precisely | ||
| the drift that made this correction necessary. | ||
| - **Emit from the turn loop after the chain call returns**, where an `await` is genuinely available. Rejected | ||
| because it is strictly worse than the chosen mechanism on the failure this ADR is about: the write would not | ||
| even *start* until the whole chain call finished, so a crash mid-chain loses every failover attempt inside | ||
| it. Chaining starts the write at the settle instant and only *joins* later, which is a narrower window, not a | ||
| wider one. | ||
| - **Leave §1 as written and implement something else.** Rejected outright. An ADR whose mechanism paragraph | ||
| describes something the code does not do is the corpus drift this project has repeatedly paid for; the | ||
| remedy is an append-only correction, not a quiet reinterpretation. | ||
|
|
||
| ### The session path needs no barrier, and the reason is structural | ||
|
|
||
| ADR-0076 scoped its event to the run path on the ground that "the session path already has this ledger". A | ||
| review of this ADR asked whether that leaves a session resume losing realized cost the same way. It does not, | ||
| and the reason is worth recording once so the question stops recurring: the session write is **synchronous and | ||
| already committed** when the handler returns. | ||
|
|
||
| `persister.ts` handles `cost:updated` through `persistDurably(() => store.recordSessionCost(...))`, and | ||
| `recordSessionCost` is declared `(entry: SessionCostEntry) => void` — a synchronous `better-sqlite3` | ||
| transaction, not a promise. `persistDurably` calls it inline and re-throws, so the money is on disk before the | ||
| next line of the handler runs, and the handler itself runs on the delivery of an event emitted from the | ||
| synchronous `onAttempt`. There is no in-flight window for a crash to land in. | ||
|
|
||
| So the asymmetry this ADR institutionalises is not "run path protected, session path forgotten". It is that | ||
| the run path's store is `persistEvent: (event) => Promise<void>` — asynchronous by seam design, because a | ||
| cloud store must plug in — and an asynchronous write is exactly what needs a barrier. The session path is | ||
| stronger here, not weaker, and stays so until its store becomes asynchronous; at that point it inherits this | ||
| decision rather than needing a new one. | ||
|
|
||
| ### What ADR-0076 keeps | ||
|
|
||
| Unchanged and not reopened: the event exists, its name, its meaning, its shape's canonical home, the | ||
| idempotency boundary and the crash-after-commit case it explicitly does NOT cover, the no-double-count | ||
| arithmetic against `node:completed`'s telescoping delta, the run-path-only scope and why the session path needs | ||
| no arm, the four rejected alternatives, and the reason it lands after | ||
| [ADR-0075](0075-fail-closed-resume-on-an-unreadable-event-log.md). This ADR replaces one paragraph. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - **One mechanism for both money events, emitted from one callback.** They cannot drift apart, and a future | ||
| reader who finds one finds the other. The alternative left two durability shapes three lines apart in the | ||
| same function. | ||
| - **The guarantee ADR-0076 wanted is actually obtainable**, and the tool-dispatch barrier makes it stronger | ||
| than §2's: realized spend is durable before the run mutates the world, not merely before it spends again. | ||
| - **No seam change.** `@relavium/llm` keeps its provider contract, and a slow disk cannot stall a provider | ||
| stream. | ||
| - The write starts earlier than any barrier-only design would allow, so the crash window is the narrowest of | ||
| the three mechanisms considered. | ||
|
|
||
| ### Negative | ||
|
|
||
| - **A third barrier on the hot path.** Mitigation: §2's outstanding-count guard applies unchanged — the barrier | ||
| costs nothing when nothing is in flight, which is the common case, and an unconditional await would change | ||
| observable interleaving that existing tests legitimately pin. | ||
| - **Two chained in-flight promises now exist** (the governor's conservative chain and the ledger's realized | ||
| chain), so a barrier that joined only one would be silently half-safe. Mitigation is structural rather than | ||
| advisory — Decision §4: the barriers join through a single entry point that owns both chains and both | ||
| retained failures, so "await the wrong one" is not an expressible mistake. What a reviewer still has to | ||
| catch is a new barrier that calls neither. | ||
| - **Three barrier holes have to close for this to be true at all**, and each is a line that reads as correct | ||
| today: the governor-conditional `#flushBudgetCommitments` early return, the `undefined` from | ||
| `#makePreEgressHook`, and `budgetApproved ? undefined`. Mitigation: Decision §5 names all three, and the | ||
| regression that proves them is an **unbudgeted** run — a fixture with a budget passes even when every | ||
| barrier is missing. | ||
| - **The tool-dispatch barrier is new engine surface**, on the hottest path a run has, and it lands before the | ||
| effect journal that will build on it. If it is placed wrong, both this ledger and `CR-12` inherit the same | ||
| crash window. Mitigation: it goes in at `agent-turn.ts`'s single `await dispatchToolCalls(...)` — one call | ||
| site, before any tool runs — rather than inside `ToolRegistry.dispatch`, where per-call placement would have | ||
| to be re-proven for every dispatch path. | ||
| - **A durability failure now fails a turn at the tool boundary too**, where previously it would have surfaced | ||
| only at the next egress or the terminal. Deliberate, and the same posture §2 accepted: a run that cannot | ||
| record what it spent must not go on to mutate anything. | ||
| - **ADR-0076 §1 must be read together with this ADR**, since the corpus is append-only and that paragraph | ||
| stays on the page. Mitigation: the title, the `Related` line and the "What ADR-0076 keeps" section above make | ||
| the scope of the correction unambiguous — one paragraph, not a section. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Condense this ADR.
This ADR repeats implementation-level details and private symbol names that will drift with the code. Keep the decision, constraints, alternatives, and consequences. Link to the canonical contracts and implementation for operational detail.
As per coding guidelines, “Architecture decisions must be condensed MADR documents under docs/decisions/ and append-only.”
🧰 Tools
🪛 LanguageTool
[style] ~163-~163: Consider an alternative for the overused word “exactly”.
Context: ... plug in — and an asynchronous write is exactly what needs a barrier. The session path ...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/decisions/0077-realized-cost-ledger-uses-the-conservative-commitment-barrier.md`
around lines 7 - 214, Condense this ADR into a concise MADR document while
preserving the decision, constraints, rejected alternatives, and consequences.
Remove repeated rationale, implementation-level sequencing, private symbol
names, and detailed code-path discussion; replace operational details with links
to the canonical contracts and implementation. Keep the document append-only and
retain only architecture-level context needed to explain the engine-owned
chained durability barrier and its three required checkpoints.
Source: Coding guidelines
Comment on lines
+544
to
+549
| const nodeSettledCost = (tx: TxDb, runId: string, nodeId: string): number => | ||
| tx | ||
| .select({ c: sql<number>`coalesce(sum(${runCosts.costMicrocents}), 0)` }) | ||
| .from(runCosts) | ||
| .where(and(eq(runCosts.runId, runId), eq(runCosts.nodeId, nodeId))) | ||
| .get()?.c ?? 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve cost attribution for each step_executions attempt.
Line 544 aggregates by runId and nodeId only. If a retry attempt settles cost and then emits node:retrying, its failed step row retains zero cost. A later retry then receives the aggregate cost from all earlier attempts.
Persist or derive a node-retry execution identity for each settled-cost row. Aggregate step_executions.costMicrocents by that identity. The within-chain attemptNumber cannot provide this identity because it resets for every node retry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/run-history-store.ts` around lines 544 - 549, Update
nodeSettledCost to attribute costs to the specific node-retry execution identity
rather than aggregating only by runId and nodeId. Persist or derive that
identity on each settled-cost row, use it when aggregating
step_executions.costMicrocents, and do not use the within-chain attemptNumber
because it resets across node retries.
Comment on lines
102
to
+106
| ## Architecture | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| subgraph Surfaces | ||
| D[Desktop · Tauri] | ||
| C[CLI] | ||
| V[VS Code extension] | ||
| P[Web portal · planned] | ||
| end | ||
| subgraph Engine["@relavium/core — one pure-TypeScript engine"] | ||
| WE[WorkflowEngine] | ||
| AS[AgentSession] | ||
| BUS[(RunEventBus · ToolRegistry)] | ||
| WE --- BUS | ||
| AS --- BUS | ||
| end | ||
| SEAM["@relavium/llm seam"] | ||
| PROV[Anthropic · OpenAI/DeepSeek · Gemini] | ||
| D --> Engine | ||
| C --> Engine | ||
| V --> Engine | ||
| P --> Engine | ||
| Engine --> SEAM --> PROV | ||
| ``` | ||
| <p align="center"> | ||
| <img src="assets/readme/relavium-architecture.svg" alt="Relavium architecture: multiple surfaces over one engine with AgentSession, WorkflowEngine, the LLM seam, MCP, and durable storage" width="100%"> | ||
| </p> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the architecture diagram in Mermaid.
This change replaces the README architecture Mermaid diagram with a rendered SVG. Keep the Mermaid source in README.md, or include it alongside the image so the architecture remains editable and reviewable.
As per coding guidelines, **/*.md documentation must use Mermaid diagrams.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 102 - 106, Replace the rendered SVG under the README
Architecture section with an editable Mermaid diagram, preserving the documented
architecture content. Keep the Mermaid source directly in README.md and remove
the image-only representation.
Source: Coding guidelines
… its run Verified each finding against the tree; fixed the ones that still hold. **Correctness.** `checkDurableTruth` reported foreign events and then compared against them anyway: `history` read ANOTHER run's terminal and `durableTerminalCount` counted it, so the verdict named the right cause while every downstream number described the wrong run. Every view now reads the runId-filtered log. Break-verified: reverting the filter reddens the foreign-log test at `durableTerminalCount`. `ledger B3`'s ordering assertion was vacuous — `indexOf` returns `-1` for a missing entry, which is less than everything, so a run that never settled a charge satisfied the ordering it exists to prove. Presence is asserted first. `tools/test-isolation` counted `...REPO_LOCAL_CHECKOUTS` repo-wide and required 2 — which is also what two spreads in `test.exclude` and none in `coverage.exclude` looks like, the exact half-failure assertion 4 is for. Now counted per property, exactly once each. Break-verified with that mutation. `persister.test.ts` restored its SQLITE_BUSY spies at the END of a test, so one failing assertion leaked a throwing `updateSession` into every later test in the file. Restore is suite-level now. **Docs — the contract contradicted itself.** `sse-event-schema.md` called `cost:attempt_settled.cumulativeCostMicrocents` "NOT the restore source" and `costMicrocents` "the delta a reader SUMS", while the fold rule three lines below says restore is `Math.max` over the absolutes and must never sum the deltas. The fold rule is what `checkpoint.ts` implements; both call-outs now agree with it. Roadmap: PR #81 (23 items, merged) and `#W15-1`/PR #82 (open) were recorded as one closure; separated. `45 CR items` → 46, the count the phase doc states. CR-03's Fix/Acceptance said three paths where five shipped. **Skipped, with reason.** The `run_costs` node-retry attribution identity needs a schema column + migration + snapshot regen — the caveat is recorded in `nodeSettledCost` and the run-level SUM stays exact either way; it is already a `#W15-1` follow-up. Condensing ADR-0077 conflicts with append-only, and at 214 lines it sits between ADR-0070 (204) and ADR-0073 (213), so it is not an outlier. The README architecture SVG is a curated asset, not a diagram that drifted. `settle()` stays fixed-turn: it drains to quiescence to prove an action did NOT run, which no predicate can express. Also folded: codepoint sort in `canonical()` (`localeCompare` is locale-dependent, so the instrument could manufacture a disagreement), `#flushBudgetCommitments` → `#joinMoneyDurability` (it fronts both money chains since ADR-0077; the host-supplied `AgentSessionDeps.flushBudgetCommitments` keeps its name and is correct), cognitive complexity 39 → five named helpers, the sync-throw recovery test now reuses the instance that took the throw, and two `expect:'repaired'` branches plus the derived fixture types the `as never` was hiding. `pnpm run ci` and `pnpm coverage` green — 96.97% lines / 93.34% branches. Refs: ADR-0076, ADR-0077 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are in code the previous commit introduced. `canonical`'s key comparator was a nested ternary; it is now a named `byCodepoint` function carrying the reason it is not `localeCompare` — the comparison must be identical on every machine, or the instrument manufactures the disagreement it exists to detect. `.filter(…).at(-1)` → `.findLast(…)` when picking `test.exclude` (the last exclude array before the coverage block). Break-verified again after the change: dropping `...REPO_LOCAL_CHECKOUTS` from `coverage.exclude` still exits 1. `pnpm run ci` and `pnpm coverage` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md (1)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project's dated amendment note for this in-place edit.
The edit amends an Accepted ADR with inline parentheticals and bold markers.
docs/decisions/README.mdstep 5 requires a dated> Amended YYYY-MM-DD: …note that points to the driving ADR. Add that note once near the metadata block, then keep the inline markers short. A dated note makes every amended ADR discoverable by one grep.As per coding guidelines, "ADRs are append-only, and changes must be documented in a new superseding ADR" — and the repo's own README defines the in-place refinement form for a non-reversing amendment.
♻️ Proposed amendment header
- **Status**: Accepted - **Date**: 2026-08-09 + +> Amended 2026-08-10 by [ADR-0077](0077-realized-cost-ledger-uses-the-conservative-commitment-barrier.md): +> §1's "awaited emit" mechanism is superseded. The write is STARTED at the settle instant and JOINED at each +> barrier. The guarantee itself is unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md` around lines 33 - 43, Add one dated “Amended YYYY-MM-DD” note near the metadata block of ADR-0076, linking to ADR-0077 and briefly describing this non-reversing refinement; retain the existing inline amendment markers but keep them concise. Follow the amendment-note format required by docs/decisions/README.md step 5 and do not alter the ADR’s substantive append-only history.Source: Coding guidelines
packages/core/src/engine/m2-e2e-harness.e2e.test.ts (1)
1239-1243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed microtask spin with
vi.waitFor.Both tests spin exactly 500 microtask yields to wait for the ledger write to block. The count is tied to current engine internals. If a future change adds a macrotask turn before the first
cost:attempt_settledpersist, the loop exhausts and the test fails for a reason unrelated to the barrier.vi.waitForyields the macrotask queue as well and reports a clear timeout.♻️ Proposed test change
- for (let i = 0; i < 500 && !ledger.blocked(); i += 1) await Promise.resolve(); - expect(ledger.blocked()).toBe(true); + await vi.waitFor(() => { + expect(ledger.blocked()).toBe(true); + });Add
vito the existingvitestimport if it is not already present.Also applies to: 1262-1266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts` around lines 1239 - 1243, Replace the fixed 500-iteration microtask loops in both ledger-blocking tests with vi.waitFor assertions that wait until ledger.blocked() is true and provide a clear timeout. Add vi to the existing vitest import if needed, while preserving the subsequent blocked-state validation.packages/core/src/engine/money-durability.test.ts (1)
115-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the interaction between a conservative failure and a retained ledger failure.
In
join(),flushConservativeis awaited before the retained#failureis read and cleared. If both halves fail in the same window, the conservative error is thrown first and the ledger error stays retained for the next join. That ordering is deliberate and observable. A test would pin it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/money-durability.test.ts` around lines 115 - 125, Add a test covering both failure sources through MoneyDurability.join(): configure flushConservative to reject and retain a separate ledger failure via the existing failure-producing path, then assert the first join rejects with the conservative error and a subsequent join surfaces the retained ledger error. Use the established draft and MoneyDurability test helpers, preserving the documented ordering and clearing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap/phases/phase-2.6.5-core-reliability-remediation.md`:
- Around line 77-83: Qualify the `#W15-1` status statement to distinguish
implementation or merge into development from merge into main. Update the
wording near the `#W15-1` heading so it states the accurate current state, using
“implemented and pending merge to main” if it has not yet merged to main.
In `@docs/standards/security-review.md`:
- Line 397: Update the documentation statement around the --json machine
contract to remove the byte-for-byte preservation claim and state that
JSON.parse preserves the original JSON value, reflecting stringifyJsonLine’s
escaping behavior.
In `@docs/standards/testing.md`:
- Around line 139-145: Update the documentation references to use relative
Markdown links: link tools/test-isolation to
../../tools/test-isolation/check.mjs and vitest.config.ts to
../../vitest.config.ts, preserving the surrounding text.
In `@packages/core/src/engine/agent-turn.ts`:
- Around line 676-682: Update `#runAttempt` to handle LedgerDurabilityError before
the generic catch, preserving its nodeId instead of replacing it with the
generic internal error. Ensure `#settleFailed` can use that nodeId to attribute
failures originating at barrier B2, while leaving other error handling
unchanged.
In `@packages/core/src/engine/durable-truth.ts`:
- Around line 129-147: Update canonical in
packages/core/src/engine/durable-truth.ts lines 129-147 to detect circular
references using ancestry-aware tracking or a cycle-detecting pre-pass,
producing a path-distinguishing marker instead of recursing until stack
exhaustion; preserve shared non-circular references. Update the test case in
packages/core/src/engine/durable-truth.test.ts lines 420-437 to compare two
different circular payloads and assert agrees === false.
In `@packages/core/src/engine/engine.ts`:
- Around line 433-442: Update the cost:attempt_settled emission flow around
MoneyDurability.record() and the shown await this.#emitDurable call to capture
`#cumulativeCostMicrocents` when record() is invoked, then use that captured value
in the queued durable write. Ensure cumulativeCostMicrocents represents the
total immediately after the current attempt rather than a later run-wide update.
In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts`:
- Around line 1132-1136: Update the assertions in the test around
assertDurableTruth to first verify verdict.history?.errorCode against the
concrete expected error code, then retain the equality assertion between
verdict.history?.errorCode and verdict.live?.errorCode so both the value and
consistency are validated.
In `@tools/test-isolation/check.mjs`:
- Around line 161-168: Update the fixture construction near the patterns-derived
fixtures so positive-control probes for the established .claude, .worktrees, and
root worktrees locations are always included independently of
REPO_LOCAL_CHECKOUTS, while retaining the existing pattern-derived fixtures for
newly added entries.
---
Nitpick comments:
In `@docs/decisions/0076-durable-per-attempt-realized-cost-ledger.md`:
- Around line 33-43: Add one dated “Amended YYYY-MM-DD” note near the metadata
block of ADR-0076, linking to ADR-0077 and briefly describing this non-reversing
refinement; retain the existing inline amendment markers but keep them concise.
Follow the amendment-note format required by docs/decisions/README.md step 5 and
do not alter the ADR’s substantive append-only history.
In `@packages/core/src/engine/m2-e2e-harness.e2e.test.ts`:
- Around line 1239-1243: Replace the fixed 500-iteration microtask loops in both
ledger-blocking tests with vi.waitFor assertions that wait until
ledger.blocked() is true and provide a clear timeout. Add vi to the existing
vitest import if needed, while preserving the subsequent blocked-state
validation.
In `@packages/core/src/engine/money-durability.test.ts`:
- Around line 115-125: Add a test covering both failure sources through
MoneyDurability.join(): configure flushConservative to reject and retain a
separate ledger failure via the existing failure-producing path, then assert the
first join rejects with the conservative error and a subsequent join surfaces
the retained ledger error. Use the established draft and MoneyDurability test
helpers, preserving the documented ordering and clearing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df014cb0-81d9-4ef9-be81-9c241a97abfe
⛔ Files ignored due to path filters (3)
assets/readme/agent-to-workflow.svgis excluded by!**/*.svgassets/readme/relavium-architecture.svgis excluded by!**/*.svgassets/readme/relavium-hero.svgis excluded by!**/*.svg
📒 Files selected for processing (46)
.github/workflows/ci.yml.gitignoreREADME.mdapps/cli/src/chat/persister.test.tsapps/cli/src/chat/persister.tsapps/cli/src/commands/agent-run.tsapps/cli/src/commands/chat-export.tsapps/cli/src/commands/chat.tsapps/cli/src/commands/export.tsapps/cli/src/commands/import.tsapps/cli/src/commands/models-pricing.tsapps/cli/src/commands/models.tsapps/cli/src/render/json-line-surfaces.test.tsdocs/decisions/0076-durable-per-attempt-realized-cost-ledger.mddocs/decisions/0077-realized-cost-ledger-uses-the-conservative-commitment-barrier.mddocs/decisions/README.mddocs/reference/cli/commands.mddocs/reference/contracts/sse-event-schema.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mddocs/roadmap/phases/phase-2.6.5-core-reliability-remediation.mddocs/standards/security-review.mddocs/standards/testing.mdeslint.config.mjspackage.jsonpackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-session.test.tspackages/core/src/engine/agent-session.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/checkpoint.test.tspackages/core/src/engine/checkpoint.tspackages/core/src/engine/durable-truth.test.tspackages/core/src/engine/durable-truth.tspackages/core/src/engine/engine.tspackages/core/src/engine/m2-e2e-harness.e2e.test.tspackages/core/src/engine/money-durability.test.tspackages/core/src/engine/money-durability.tspackages/core/src/engine/node-executor.tspackages/core/src/index.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.tspackages/shared/src/constants.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.tstools/test-isolation/check.mjsvitest.config.ts
Verified each finding against the tree.
**The oracle reported two different cyclic payloads as equal.** Sorting the keys
means the replacer hands `JSON.stringify` a fresh object at every level, which
defeats its native cycle detector — so a cyclic payload never raised the
`TypeError` the catch was written for; it recursed until the stack blew and BOTH
sides came back `[uncomparable: RangeError]`. `canonical` now walks the tree with
an ancestor stack (popped on the way back up, so a shared reference stays a
shared reference) and emits a path-local `[circular]` marker. `toJSON` is applied
by hand for the same reason native `stringify` applies it: without it every
`Date` reduces to `{}` and any two compare equal — a false agreement the fix
would have introduced. Break-verified both ways.
**A ledger row could report a total that included money it never spent.** The
chained write reads the engine's live counter when it RUNS — behind the previous
write's `persistEvent`, which is real I/O — so under a `fan_out` (concurrent
nodes, one chain) it can report attempts that settled during the wait.
`MoneyDurability.record` now takes the total captured at record time and
`turnPort` snapshots it synchronously. The contract's claim that each absolute is
"a true run-wide total at that instant" is only true of a captured value.
**`#runAttempt`'s catch-all flattened money-durability failures.**
`throwMappedChainError` has two arms whose only job is to keep
`LedgerDurabilityError` / `CommitmentDurabilityError` and their owning `nodeId`
intact out of the turn; the catch-all then reported `the node handler threw an
unexpected error`. Preserved for one frame, then discarded — and unrecoverable,
since `join()` surfaces a retained failure exactly once. Corrected in the code
comment after measuring: the arm is NOT reached on the ordinary path (the abort
wins and the node classifies as cancelled), only where `#emitDurable` skips the
abort because a sibling already set `#failure` — ADR-0077's still-unbuilt
required regression. An assertion claiming otherwise was written and removed.
**The isolation guard's probes all came from the list it was checking**, so
deleting an entry deleted its probe. Three known locations are now planted
unconditionally; the structural assertion covers this only when a real second
checkout is on disk, which CI never has. Break-verified by removing `.claude`
from `REPO_LOCAL_CHECKOUTS`: the control fires with the entry to paste back.
Docs: `security-review.md` claimed `--json` reproduces its data "byte for byte",
which contradicts escaping — what is preserved is the parsed value. The phase doc
said `#W15-1` was "merged"; it is landed on `development`, pending merge to
`main`. `testing.md` referred to two files in backticks rather than links.
ADR-0076 gained the dated `> Amended` note `docs/decisions/README.md` step 5
requires; the in-place §1 markers stay.
Also: `m2-e2e` asserted `history.errorCode === live.errorCode`, which
`undefined === undefined` satisfies — the concrete code is pinned first. New
tests: the record-time stamp, and both failure sources through one `join()`
(whose fixture rejects ONCE, because a permanently-rejecting conservative half
would mask the retained ledger failure for the rest of the run).
**Skipped.** `vi.waitFor` for the two bounded microtask spins: the condition
flips on a microtask, so a wall-clock poller trades a deterministic loop for a
timer that can flake under CI load.
`pnpm run ci` and `pnpm coverage` green — 96.91% lines / 93.31% branches.
Refs: ADR-0076, ADR-0077
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Aug 11, 2026
Phase 2.6.5 status moves to in progress with a Progress block naming the six closed items, their closure dates, and the three gaps carried forward rather than implied: ADR-0077's unbuilt required regression (without it `#runAttempt`'s money-durability arm is unreached), `CR-10`'s property being inexpressible from the durable log alone, and the oracle's three remaining debts to `CR-92`. `current.md`: the Wave 1 closure is now merged on both PRs (#81 on 2026-08-09, `#W15-1` + the first 2.6.5 batch via #82 on 2026-08-11), the ledger node is marked complete in the graph, and the 2.6.5 section carries a live 6-of-46 count pointing at `CR-10` as next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2.6.5's prerequisite (
#W15-1) plus its first two groups — the test-honesty oracle (CR-90/CR-91) and the three half-done items ofW0(CR-01–CR-03, plus a fourth and fifth path the findings had missed). Every step carries an Opus and a Sonnet review round, folded as its own commit.pnpm run ciEXIT=0 ·pnpm coverageEXIT=0 (96.92% lines, 93.32% branches).What landed
#W15-1— the durable per-attempt realized-cost ledger.cost:attempt_settledis now a durable run event, folded intorun_costsin the same transaction as its event, restored on resume, and joined at three barriers before the run spends or mutates again.It needed a new decision on the way: ADR-0077 amends ADR-0076 §1, whose stated mechanism — an inline
awaitat the attempt boundary — is unimplementable. The seam's attempt observer is(record) => void, andbudget-governor.tsalready said so in those words. The ledger takes ADR-0074 §2's chain-and-join shape instead, with a third barrier before tool dispatch thatCR-12's effect journal is expected to extend rather than re-thread.CR-90— test isolation. A rootvitest listwas collecting 472 test files, 234 of them from a full agent-tooling checkout under.claude/worktrees/. Hidden by a LOCAL.git/info/exclude, sogit statusstayed clean and CI never saw it. Now 238, with an ESLint-style guard inpnpm run ciandci.ymlwhose primary assertion consults no list — it fails on any collected file whose ancestor carries its ownpnpm-workspace.yaml.CR-91— the durable-truth oracle.checkDurableTruthcompares the live terminal, the durable history, the history a fresh engine leaves afterreconcile(), the checkpoint status, and the log's own order. Exported as a supported testing API, because the surface that most needs it (apps/cliagainst the realhistory.db) is outsidepackages/core.W0.session:cancellednow goes through the durability latch; the turn counter's position is a documented decision pinned through the cap; and five--jsonpaths — not the three the finding named — go throughstringifyJsonLine, enforced by ano-restricted-syntaxselector rather than by review.CR-64was added from two architecture reviews; the same commit records why everything else in them belongs elsewhere.What the review rounds cost, and why that is the point
Ten of the eleven review rounds found something real, and most of what they found was in the fix, not the original defect:
CR-90guard passed green on the exact defect it was written for, twice: first because it probed only one location, then because deriving probes from the exclusion list meant deleting an entry also deleted its probe.CR-91oracle could not expressCR-10,CR-11orCR-12while three places claimed it could; and its cycle guard flagged shared references, causing the false-failure class it existed to prevent.CR-01assertions were vacuous and passed with the fix deleted.Four false claims of mine were corrected in canonical homes, and the phase doc now records what each item does not prove alongside what it does.
Deliberately open, named rather than implied
CR-10's "no persisted event is missing from the middle" is not expressible from the log alone: streamed events take sequence numbers and are never persisted, so a real run reads[0,1,2,3,5,10,…]. Its acceptance needs a store harness that records what it was asked to persist.CR-92needs three things from the oracle before it can certify itself; all three are listed in the phase doc.🤖 Generated with Claude Code
Summary by Sourcery
Refresh README and roadmap to document current architecture, product status, and Phase 2.6.5, introduce a durable per-attempt cost ledger and crash-truth oracle in the core engine, harden run/session durability and budgeting semantics, and add test isolation plus secure JSON output enforcement in the CLI and CI.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Security
Documentation
Chores