ORB-158: close the spend ledger loop - #656
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b75b0e792
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #656 (ORB-158: close the spend ledger loop)
Scope: tools/automation-budget.mjs, tools/launch-worker.mjs, tools/test-tools.mjs
Recommendation: REQUEST CHANGES
Summary
The PR replaces automation-budget's single "missing measurement" bucket with an explicit pending vs unknown split so a completed-but-unmeasured invocation no longer wedges the ledger shut, and has launch-worker.mjs append an unmeasured record immediately after a delivered pointer to close the reservation early. The ledger-semantics change in automation-budget.mjs is correct and well tested. The new launch-worker.mjs call that closes the reservation, however, is inserted on the wrong side of the rollback-reset line, which reopens a destructive-rollback window against an already-running worker.
Findings
Critical
New post-delivery ledger-close call can trigger a destructive worktree/branch rollback against an already-running worker
- Location:
tools/launch-worker.mjs:977-993(compare to the rollback assignment at:862and thefail()rollback block at:257-291) - Issue: The new
runBudgetCommand(["record", ...])call (added by this PR, confirmed viagit diffagainst the merge base — it did not exist before this PR) executes after the prompt pointer has been successfully delivered to the worker (past theif (!pointerDelivered) fail(...)gate at:970-975) but beforerollback = nullat:993.runBudgetCommand(:438-450) is called with noblockedExitargument, so any non-zero exit or spawn error from the spawnedautomation-budget.mjs record(a ledger lock timeout under concurrent-launch contention, a transient write/disk error, or a malformed ledger line) routes intofail(3, reason). Becauserollbackis still set at that point (assigned at:862, only cleared at:993),fail()unconditionally runs its cleanup block (:257-291):orca terminal stop,orca worktree rm --force, andgit branch -Don the contract/orca branches — the exact cleanup path meant only for "the launch never really started." The pre-existingrollback = nullreset on line 993 exists specifically so that failures in the steps after pointer delivery can't trigger this cleanup; the new call is inserted before that reset, reopening the bug class that ordering existed to prevent. - Risk: A worker that has already been handed its prompt and started editing files in its worktree gets its terminal killed and its worktree + branch force-deleted the moment the bookkeeping
recordcall hits any transient failure — real loss of in-progress (and possibly already-committed-but-unpushed) agent work, not just a lost ledger entry. This is realistic, not theoretical: the ledger is one shared file (~/.orbit/automation-budget.jsonl) and/orchestrateexplicitly runs concurrent launches against it (the PR's own "concurrent reservation assertions" exercise contention on this same file). No existing test intools/test-tools.mjsexercises "the closingrecordcall fails after pointer delivery." - Fix: Move
rollback = nullto before the newrunBudgetCommand(["record", ...])call (mirroring the existing pattern), so a failure here only logs/warns and leaves the ledger record pending for the next launch to close via a normalrecord, without touching the live worktree/branch/terminal. Add atools/test-tools.mjscase that stages therecordsub-call to fail after a delivered pointer and asserts the worktree is NOT removed and the branch is NOT dropped. - Reference: CLAUDE.md rule 1 (root cause); rubric dimension 1 (Correctness) and dimension 15 (Harness changes need executed evidence for new decision paths)
High
None.
Medium
None beyond what's folded into the Critical finding above.
Low / Info
None.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts, and only one repo touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* / landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint / Type check / Tests | N/A — skipped in this job; covered by required CI checks (Build / Unit Tests / SonarCloud) |
| Contract-aligner / backward-compat guard | Not verifiable in CI — sibling orbit-api repo not checked out in this job; also genuinely out of scope, this diff carries no packages/shared/src/types/* changes |
What's good
automation-budget.mjs's pending/unknown split is a clean, well-reasoned fix for the actual deadlock: reservations fail closed (as they must, to prevent runaway spend), while a completed-but-unmeasured invocation now correctly falls out of the block path without being silently treated as zero tokens.- Test coverage for the ledger-semantics change itself is thorough: recorded-unmeasured admits the next launch, pending still fails closed and names the clearing command, reserved tier still surfaces the warning, and the report text/JSON both expose the new fields correctly.
- The PR body's acceptance evidence is real command output, not a bare "verified" claim.
Recommendation
Fix the rollback-ordering bug in tools/launch-worker.mjs before merge (move rollback = null ahead of the new post-delivery record call) and add a test that pins "a failure in the ledger-close step does not destroy an already-running worker's worktree." The ledger-semantics half of this PR (automation-budget.mjs) is solid and needs no changes.
|
@codex review |
|
Addressed automated review activity PRR_kwDOR5Siws8AAAABHmXt-A in commit 81e9091: the launcher-side post-delivery record call was removed, the reservation remains pending until the worker ends, and rollback is cleared before post-delivery handoff actions. Evidence: 81e9091; node tools/test-tools.mjs exit 0, ORBIT TOOLS GATE OK; node .claude/hooks/test-hooks.mjs exit 0, ORBIT HOOK PARITY OK. No code change required: IC_kwDOR5Siws8AAAABMNSjNA reports a skipped Vercel deployment for apps/web, while this PR changes only tooling. Evidence: 81e9091. No code change required: IC_kwDOR5Siws8AAAABMNTjxQ reports SonarCloud Quality Gate passed with zero new issues. Evidence: 81e9091. |
|
No code change required: IC_kwDOR5Siws8AAAABMTC5Cg reports SonarCloud Quality Gate passed with zero new issues. Evidence: 81e9091. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81e909193e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f5f341717
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: ORB-158 — close the spend ledger loop
Files changed: tools/README.md, tools/automation-budget.mjs, tools/launch-worker.mjs, tools/test-tools.mjs (tooling-only; no apps/*, orbit-api, or packages/shared surfaces touched)
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 1 |
[HIGH] Mid-loop orca failure strands a budget reservation pending forever
Location: tools/launch-worker.mjs:302-317 (the three-branch budget-cleanup conditional in fail()), and the pointer-sending loop at :985-1020.
reservationMaySpend is set true at line 989, before pointerOnScreen() (line 991) and busy() (lines 1003/1010) run — both call orca() internally. orca() (line 339-354) calls fail() directly on any command failure. reservationCompletedUnknown is only set true at line 1016, after the loop exits naturally without delivery.
So: if one of those in-loop orca() calls itself fails (e.g. terminal read errors), fail() runs with reservationMaySpend = true and reservationCompletedUnknown = false. rollback is still non-null at that point, so the rollback genuinely tears down the worktree (cleanupConfirmed = true). None of the three budget-cleanup branches at lines 302-317 match this combination:
reservationCompletedUnknown && cleanupConfirmed→ false (reservationCompletedUnknownis false)reservationCompletedUnknownalone → false!reservationMaySpend && cleanupConfirmed→ false (reservationMaySpendis true)
The ledger record stays pending: true forever, with no error printed about it, even though the worker is genuinely gone. Every subsequent routine-tier launch for that engine then fails closed against a worktree that no longer exists — directly undercutting this PR's stated purpose of closing the spend-ledger loop.
Verified this is untested: the only failure-path case in test-tools.mjs (postCommitFailure, line 1015) fails a call after delivery, once rollback is already null — a different precondition from the one described here.
Fix: derive the "close as completed-unknown" branch from reservationMaySpend && cleanupConfirmed (whether a real teardown just happened), not solely from the narrower reservationCompletedUnknown flag. Add a regression case that fails a terminal read/terminal show call mid-loop after one accepted send, and asserts the ledger closes as completed-unknown rather than staying pending.
[MEDIUM] cancel can tombstone a completed-unknown record, not just a pending one
Location: tools/automation-budget.mjs:332-344 (validateRecord's cancelled branch) and :644-654 (runCancel).
runCancel only refuses when the target's latest record already carries inputTokens/outputTokens — it never requires latest.pending === true. validateRecord's cancelled branch never rejects a record that also carries pending: true; it's silently dropped rather than flagged as contradictory.
A direct automation-budget.mjs cancel call against an identity whose latest record is completed-but-unknown (e.g. one recordBudgetReservationAsUnknown just appended) would pass the guard and overwrite it with a cancelled tombstone, contradicting the tool's own USAGE text ("cancel: append a tombstone for a pending invocation proven not to have started"). Not reachable via any call site this PR adds today (cancelBudgetReservation only fires while reservationMaySpend is still false), so this is latent rather than live — fine to land as a tracked follow-up.
Fix: require latest.pending === true in runCancel before appending a tombstone; reject cancelled + pending co-occurring in validateRecord.
What's good
- The pending/cancelled/unknown state machine is a real improvement over the prior binary pending-forever design, and is well-documented in both the tool USAGE text and the injected worker-contract clause.
- Good test discipline elsewhere:
recordedLedger/undeliveredRetry/postCommitFailureeach isolate one real scenario with sharp assertions.
Deferred (surfaces not touched by this diff)
DESIGN.md/AI-slop, Parity, i18n, Contract drift/backward-compat, Backend hard rules, FEATURES.md parity — all N/A. Cross-repo dimensions needing orbit-api are not verifiable in this CI job (sibling repo not checked out).
Recommendation
Requesting changes for the HIGH finding: it's a real, reachable regression against this PR's own stated goal, with no test coverage for the failure window. The MEDIUM finding is fine to land as a tracked follow-up.
|
@codex review |
There was a problem hiding this comment.
Code Review: PR #656 (ORB-158: close the spend ledger loop)
Scope: PR #656 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR replaces the automation-budget ledger's binary "has tokens or fails closed" model with a
three-state one (pending / terminal-completed-but-unmeasured / measured), and teaches
launch-worker.mjs to close an undelivered pointer's reservation as completed-but-unknown after
confirmed teardown while leaving a delivered (live) reservation pending for the worker's own
terminal record. The logic is correct and well tested (7 new/changed cases across
launch-worker.mjs and automation-budget.mjs), backward compatible with legacy ledger rows that
carry neither flag, and the design deliberately never risks closing a reservation for a worker that
might still be alive. Two Medium gaps: a handful of the new refusal branches this same diff
introduces have no test exercising them.
Findings
Critical
None.
High
None.
Medium
[Medium] New pending/completed mutual-exclusivity refusals in validateRecord are untested
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: orbit-ui-mobile/tools/automation-budget.mjs:345-359 (validateRecord)
· issue: The diff adds five new fail() call sites guarding the new pending/completed ledger
fields (pending must be true when present, pending record must not carry token measurements,
completed must be true when present, completed record must not also be pending, completed record must not carry token measurements). None of them is exercised by
tools/test-tools.mjs's automationBudgetCases.
· risk: A future edit to this state machine (e.g. swapping the order of the pending/completed
blocks, or a stray == vs ===) can silently break the mutual-exclusivity guard and ship with a
fully green suite.
· fix: Add readLedger/check fixtures in automationBudgetCases with a malformed record for each
branch (e.g. { pending: true, inputTokens: 1 }, { pending: true, completed: true }, { completed: "yes" }) and assert the matching fail message and exit 3.
· reference: rubric dimension 15 ("a new refusal... needs its own case, not an extension of an
existing assertion").
[Medium] The completed-unknown closeout's own failure branches in launch-worker.mjs are untested
· dimension: 15
· location: orbit-ui-mobile/tools/launch-worker.mjs:302-310 (the reservationCompletedUnknown
branches in fail()) and :565-589 (recordBudgetReservationAsUnknown)
· issue: Two new operator-facing messages — left budget reservation "..." pending because its completed-unknown closure could not be recorded (when the follow-up record sub-call fails) and
left budget reservation "..." pending because the worker shutdown could not be confirmed (when
worktree teardown itself fails while the pointer was never delivered) — have no fixture driving
either failure mode.
· risk: This is exactly the class of defect dimension 15 exists to catch: a harness change whose
failure path is never executed. If the record sub-call's argument list ever drifts, the gap is
only surfaced by an operator reading raw stderr in production, never by the suite.
· fix: Extend the existing never (undelivered-pointer) fixture with a variant where the terminal switch/worktree rm mock fails (mirroring the existing postCommitFailure pattern already added
in this diff) and a variant where the trailing automation-budget.mjs record call fails, asserting
each console.error text.
· reference: rubric dimension 15.
Low / Info
None posted (signal gate: Low/Info are not PR-review findings).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts/orbit-api change; CI's own "Contract Drift" check is SUCCESS |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
Local execution of node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs was not
executable from this review session (Bash execution of those specific commands required an
approval this sandboxed session could not grant). Citing the PR's own CI run instead, which is
genuine executed evidence, not the PR body's self-reported claim:
| Check | Result |
|---|---|
| Lint (PR Tests) | PASS (CI, SUCCESS) |
| Type check (PR Tests) | PASS (CI, SUCCESS) |
| Tests / Unit Tests (PR Tests) | PASS (CI, SUCCESS) |
| Harness Execution (Guards) | PASS (CI, SUCCESS, completed 2026-07-30T11:12:30Z) |
| Harness Lockstep (Guards) | PASS (CI, SUCCESS) |
| Contract Drift (PR Tests) | PASS (CI, SUCCESS) |
| Cross-Platform Parity (Guards) | PASS (CI, SUCCESS) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A, no
apps/*UI file touched. - Dimension 9 (Parity): N/A, no
apps/web/**/apps/mobile/**file touched. - Dimension 10 (i18n): N/A, no user-facing string or locale JSON touched.
- Dimension 11 (Contract drift + backward-compat): N/A, no
packages/shared/src/types/*or DTO
touched; corroborated by CI's own green "Contract Drift" check. - Dimension 12 (Security): N/A, no orbit-api code; the changed files are maintainer-only automation
tooling with no end-user attack surface. - Dimension 13 (Backend hard rules): N/A, orbit-api untouched.
- Dimension 14 (FEATURES.md parity): N/A, no user-facing app feature surface changed (internal
orchestration tooling only). - Local harness execution (this session): could not run
node tools/test-tools.mjs/
node .claude/hooks/test-hooks.mjsdirectly; substituted the PR's own completed CI "Harness
Execution" check (SUCCESS) as independent verification. - All four changed files (
tools/README.md,tools/automation-budget.mjs,
tools/launch-worker.mjs,tools/test-tools.mjs) received a verdict above; nothing else
deferred.
What's good
- The pending/completed split is documented in three places that all agree:
tools/README.md's
catalog entry, the tool's ownUSAGEbanner text, and the worker contract clause 6 — no drift
between what the tool does and what it claims to do. - Legacy ledger rows (written before this deploy, carrying neither
pendingnorcompleted) still
fail closed under the new code path, so there is no silent behavior change for records already on
disk — verified by the newlegacy-unmarkedfixture. - The design never risks closing a reservation for a worker that might still be alive: an undelivered
pointer is only recorded as completed-but-unknown after worktree teardown is confirmed
(cleanupConfirmed), and a delivered pointer is never touched by the launcher's ownfail()path
at all (verified by the newpostCommitFailurecase, which confirms a late orca failure after
delivery neither stops the terminal nor removes the live worktree). - Good regression coverage for the actual bug this PR closes: the new
live-pending-second-calls
case proves a second launch is refused (before worktree creation) while a delivered pointer's
reservation is still live, andundeliveredRetryproves a subsequent launch is admitted once an
undelivered pointer's reservation has been closed out.
Recommendation
Approve as is. The two Medium findings are follow-up test-coverage items (name the missing case,
add the fixture) that don't block merge per the signal gate; nothing Critical or High survives.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review: PR #656
Scope: PR #656 in thomasluizon/orbit-ui-mobile (feature/orb-158-close-the-spend-ledger-loop → main)
Recommendation: NEEDS WORK
Summary
ORB-158 changes tools/automation-budget.mjs and tools/launch-worker.mjs to stop
closing the spend-ledger loop by fabricating measurements: reservations now carry an
explicit pending marker while the worker is live, an undelivered pointer is closed as
completed-but-unknown only after the launcher confirms it tore down the worker, and the
fuse fails closed only on a still-pending reservation, not on a terminal record that is
honestly unmeasured. The launcher and ledger logic are internally consistent and the new
test coverage in tools/test-tools.mjs exercises the delivered/undelivered/live-pending/
post-delivery-failure matrix well. Two defects survive: the tools/README.md catalog
entry this diff edits asserts a safety property ("legacy unmarked measurements fail closed
for safety") that the diff's own code and its own new test directly contradict, and the
WARN status path silently drops the same "spend is unmeasured" caution that PROCEED
now carries, right at the moment closest to the budget edge where it matters most.
Findings
Critical
None.
High
[HIGH] tools/README.md claims legacy records fail closed; the code and this PR's own test prove otherwise
· dimension: 1. Correctness
· location: orbit-ui-mobile/tools/README.md:34
· issue: The rewritten catalog line states "Legacy unmarked measurements fail closed for
safety." But summarize() in tools/automation-budget.mjs (lines 427-438) routes any
terminal record missing inputTokens/outputTokens into unknownIdentities unless it
is explicitly pending === true — a legacy record with neither pending nor completed
set lands in unknownIdentities exactly like an explicitly-completed-unknown one.
evaluateBudget() only fails closed on pendingIdentities (line 477); unknownIdentities
never blocks. This PR's own new test ("the recorded legacy unmarked invocation admits the
next routine launch as unmeasured", tools/test-tools.mjs ~line 480) and the PR body's
acceptance evidence both assert status:"PROCEED" for the exact legacy ORB-153 record —
neither the code nor the PR's own evidence supports "fail closed."
· risk: tools/README.md is the authoritative tool catalog (root CLAUDE.md docs
registry). An operator or a future agent reading it will believe unmeasured legacy
ledger rows block a launch, and treat that as a safety net that doesn't exist — silent
unmeasured spend from legacy rows will keep proceeding with only a stderr warning, which
is easy to miss in unattended automation, while everyone believes it "fails closed."
· fix: Correct the catalog line to match the actual (and tested) behavior, e.g.: "A
terminal record without token measurements — whether explicitly marked completed or a
legacy record with neither marker — is completed-but-unknown: it stays outside token
totals and does not block routine work, but a routine PROCEED warns that its spend is
unmeasured." Drop the "fail closed for safety" clause entirely, since only an explicit
pending reservation fails closed.
· reference: CLAUDE.md rule 1 (root cause / accurate contract); docs registry entry for
tools/README.md.
Medium
[MEDIUM] The WARN path drops the same unmeasured-spend caution PROCEED now carries
· dimension: 3. SOLID / clean architecture (pattern inconsistency)
· location: orbit-ui-mobile/tools/automation-budget.mjs:496-511
· issue: emitBudgetResult was extended in this diff to warn when status === "PROCEED"
and unknownIdentities.length > 0 (lines 508-511), but the status === "WARN" branch
(lines 499-501, three lines above, untouched by this diff) never checks
unknownIdentities at all.
· risk: WARN is the state closest to the budget edge — exactly where an operator most
needs to know the reported observed spend may be an undercount because some completed
invocations are unmeasured. Today that caveat only surfaces in the calmer PROCEED
state and silently disappears the moment the same ledger crosses into WARN.
· fix: Hoist the unknown-identities warning out of the status === "PROCEED" guard (or
add the same check to the WARN branch) so it also fires when status === "WARN".
· reference: CLAUDE.md rule 7 (DRY at the right level) / rubric dimension 3 (pattern
consistency).
[MEDIUM] New pending/completed ledger-validation refusal branches have no test coverage
· dimension: 15. Harness changes need EXECUTED evidence
· location: orbit-ui-mobile/tools/automation-budget.mjs:345-359
· issue: validateRecord gained five new refusal branches in this diff: pending must be true when present, pending record must not carry token measurements, completed must be true when present, completed record must not also be pending, and completed record must not carry token measurements. Every pending: true / completed: true
fixture added to tools/test-tools.mjs in this diff (grep confirms all of them, e.g.
lines ~1808, ~5181, ~5201, ~5215-5216) is a well-formed record used only to exercise the
pending/unknown behavior; none feeds a malformed record (e.g. pending: true plus
inputTokens, or pending: true plus completed: true) through check/report to
confirm these five new fail(..., 3) branches actually reject bad ledger data.
· risk: A malformed ledger line reaching one of these five branches is currently
unverified; a future edit to validateRecord could silently break one of these refusals
(e.g. let a record be both pending and completed) with no test to catch it.
· fix: Add one check/report case per new refusal (a pending:true record carrying
inputTokens, and a record carrying both pending:true and completed:true) asserting
exit 3 and the specific error text.
· reference: rubric dimension 15 ("a new decision path... needs its own case, not an
extension of an existing assertion").
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or locale JSON changed |
| contract-aligner | N/A — no orbit-api or packages/shared/src/types/* change; only repo touched is orbit-ui-mobile |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* UI or landing-page file changed |
This diff is scoped entirely to tools/** (plus tools/README.md), so none of the five
gated subagents fire; the review ran the full rubric walk directly.
Validation
| Check | Result |
|---|---|
| Lint | N/A — not verifiable in this session (see below) |
| Type check | N/A — not verifiable in this session (see below) |
| Tests | N/A — not verifiable in this session (see below) |
| Build (api) | N/A — orbit-api not touched |
This review session's Bash tool refused every node/npm invocation and every
filesystem-mutation command (rm, git clean, mkdir) with "This command requires
approval," with no interactive user available to grant it, so node tools/test-tools.mjs
and node .claude/hooks/test-hooks.mjs could not be independently re-run here despite the
working tree being checked out at the PR's merge commit. gh pr view 656 --json statusCheckRollup shows the Harness Execution CI job itself is IN_PROGRESS (not yet
completed) as of this review, so its result is not yet available to cite either. The PR
body pastes what it presents as real node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs output (exit 0, ORBIT TOOLS GATE OK / ORBIT HOOK PARITY OK), but per dimension 15 that self-reported claim is not a substitute for a completed,
independently-checkable job. Before merge, confirm the Harness Execution CI check
completes green (it was still running at review time); do not rely on the PR body's
pasted output alone.
An untracked scratch file tmp-legacy-check.mjs was created in the repo root during this
review to probe the legacy-record behavior directly; it could not be deleted afterward
because rm/git clean were both blocked in this sandbox. It carries no production
change (it is not part of the diff and is untracked), but should be removed before/at
merge — git clean -f tmp-legacy-check.mjs from the PR branch.
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift), 12
(Security), 13 (Backend hard rules), 14 (FEATURES.md parity): all N/A — the diff touches
onlytools/**, noapps/*,orbit-api, orpackages/shared/src/types/*surface. - Validation (Phase 7 / lint / type-check / tests) not run — see Validation section above;
the CI Harness Execution job was stillIN_PROGRESSat review time. - All four changed files (
tools/README.md,tools/automation-budget.mjs,
tools/launch-worker.mjs,tools/test-tools.mjs) received a verdict above; nothing
changed-but-unreviewed remains.
What's good
- The pending/completed-unknown split is a real fix for the deadlock this ticket targets:
a delivered pointer no longer gets force-closed by the launcher (which would risk
closing live work), and an undelivered pointer no longer leaves a reservation stuck
pending forever after the worker is confirmed torn down. tools/test-tools.mjsgained strong coverage for the launcher side: delivered-stays-
pending, live-pending-blocks-a-second-launch, undelivered-closes-as-unknown, a
post-delivery downstream failure that must NOT roll back a live worker, and a launch
after an undelivered pointer being admitted. That last case in particular is exactly the
kind of end-to-end proof dimension 15 asks for.- The order of operations in
launch-worker.mjs'sfail()(stop terminal + remove
worktree, only then record completed-unknown) matches the PR's own stated lifecycle
contract and is verified by the new tests, not just asserted in prose. - The worker contract text injected into every prompt now explicitly forbids closing a
live reservation, recording zero, or inferring tokens from accountusedPercent— root
cause fix over a fallback, consistent with CLAUDE.md rule 1.
Recommendation
Fix the tools/README.md catalog line so it matches what the code and this PR's own test
actually do (High — blocks merge per the Signal gate). While in there, extend the WARN
branch of emitBudgetResult to carry the same unmeasured-spend caution PROCEED now has,
and add the two missing malformed-record test cases for the new pending/completed
validation branches (both Medium, fix-before-merge is reasonable but not blocking). Confirm
the Harness Execution CI check finishes green before merging, since it was still
running at review time and this session could not independently re-run it. Clean up the
stray tmp-legacy-check.mjs scratch file this review left in the worktree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569b1230e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #656
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
Summary
The diff touches only tools/automation-budget.mjs, tools/launch-worker.mjs, tools/test-tools.mjs, and one line of tools/README.md — no apps/*, packages/shared, or orbit-api surface, so every cross-platform/i18n/contract/design/backend dimension is N/A. The ledger state machine was traced by hand: pending (reserved, blocks routine work) → either completed (explicit completed-but-unknown) or measured (inputTokens/outputTokens) or a legacy-unmarked record (now correctly classified as unknown, non-blocking, distinct from pending). The launcher's fail() teardown in tools/launch-worker.mjs correctly distinguishes "pointer never delivered, teardown confirmed → safe to record completed-unknown" from "pointer delivered, a later handoff call failed → the worker is live, must not be touched, reservation stays pending" — this asymmetry is deliberate and is exactly what the new postCommitFailure test (tools/test-tools.mjs:1015-1023) pins.
One finding survived scrutiny, and it's a coverage gap rather than a defect: validateRecord's new read-time rule rejecting a ledger record with only one of inputTokens/outputTokens (tools/automation-budget.mjs:360-362) is only tested at the CLI-argument level, never by staging an already-mismatched record in a ledger file and driving it through check/report. The rejection itself is correct and intentional — documented in the updated tools/README.md row and matching the commit's own title ("reject partial ledger measurements"): failing closed on ambiguous historical data is the safe choice over silently excluding a partially-known spend from totals.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Read-time rejection of a mismatched-token ledger record has no test driving it through the ledger, only through the CLI
- dimension: 15. Harness changes need EXECUTED evidence
- location:
tools/automation-budget.mjs:360-362(test gap intools/test-tools.mjs,automationBudgetCases) - issue:
validateRecordgainedhasOwn(record, "inputTokens") !== hasOwn(record, "outputTokens"), which fires insidereadLedger— called first by every command (check/reserve/record/cancel/report). The only coverage is the CLI-level twin inrunRecord(tools/test-tools.mjs:5357-5380, "record rejects a partial provider measurement..."). No test stages a ledger file already containing a one-sided record (nopending/completed) and assertscheck/reportrefuses it, the waytools/test-tools.mjs:5080-5082already does for syntactically-invalid JSON. - risk: This is precisely the "new decision path... needs its own case" gap dimension 15 exists to catch: the read path and write path enforce the same invariant in two different places, and only one is pinned. A future refactor of
validateRecordcould silently drop the read-time check and ship green. - fix: Add a case to
automationBudgetCases()staging abudgetRecord-style line with exactly one ofinputTokens/outputTokensset, runcheck(orreport) against it, assertstatus: 3andstderr: /inputTokens and outputTokens must be provided together/— mirroring the existing malformed-JSON case at line 5080. - reference:
rubric.mddimension 15
Low / Info
None (signal gate: not posted).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/* file changed |
| i18n-syncer | N/A — no user-facing strings / locale JSON changed |
| contract-aligner | N/A — no shared-types/endpoints/DTO surface changed |
| security-reviewer | N/A — no orbit-api changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type check | PASS (CI) |
| Tests | PASS (CI Unit Tests) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS — CI job Harness Execution completed SUCCESS |
| Build (api) | N/A |
Local /validate (Phase 7) skipped per this workflow's CI adaptation — Build / Unit Tests / SonarCloud already run as required checks.
What's good
- The regression test replays the exact live ORB-153 production ledger line — the strongest form of evidence for this class of fix.
- The
fail()teardown correctly distinguishes an undelivered-and-torn-down reservation from a delivered-but-handoff-failed one, and both paths now have direct tests. WORKER_CONTRACTclause renumbering (tools/launch-worker.mjs) is internally consistent — the in-text references sit before the newly-inserted clause, so nothing went stale.
Recommendation
Nothing blocks merge. Add the one missing ledger-fixture test named above (Medium, non-blocking) in this PR or a fast follow-up.
|
Addressed automated review activity PRR_kwDOR5Siws8AAAABHzK0Gg in commit 75a0790: corrected the automation-budget catalog contract for legacy unmarked records, preserved the completed-unmeasured warning at WARN, and added malformed ledger-state coverage. Evidence: 75a0790; node tools/test-tools.mjs exit 0, ORBIT TOOLS GATE OK; node .claude/hooks/test-hooks.mjs exit 0, ORBIT HOOK PARITY OK. No code change required: PRR_kwDOR5Siws8AAAABHy9tDA is an approval with non-blocking Medium coverage observations. Evidence: 75a0790. No code change required: IC_kwDOR5Siws8AAAABMNSjNA is a skipped Vercel deployment for apps/web, while this PR changes tooling only. Evidence: 75a0790. No code change required: IC_kwDOR5Siws8AAAABMce5gQ reports no major issues at the prior head. Evidence: 75a0790. No code change required: IC_kwDOR5Siws8AAAABMcrdyQ reports a passed SonarCloud quality gate. Evidence: 75a0790. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6429c66160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #656 (ORB-158: close the spend ledger loop)
Note: the "APPROVED / test" review posted moments ago on this PR was an accidental placeholder
from a tooling mishap during this review session and should be disregarded. This comment is the
actual review.
Recommendation: APPROVE
Summary
ORB-158 replaces the automation-budget ledger's binary "measured or fail-closed" model with a
three-state disposition (pending / completed-unknown / measured), so a torn-down worker that
never got to record its own tokens no longer wedges every later routine launch behind a stale
"missing measurement" refusal. launch-worker.mjs now closes a torn-down reservation as
completed-unknown when a live send was accepted and the worker was actually stopped;
teardown-worktree.mjs sweeps any reservation still pending for its issue at verified-Done
teardown. The change is careful and internally consistent, backed by a thorough test-tools.mjs
expansion (new ledger-validation, idempotent-record, and launch/teardown integration cases) and by
a live read-only probe against the real production ledger showing an unchanged SHA-256 before and
after. One decision path introduced in launch-worker.mjs's three-way disposition branch ships
without a ledger-state assertion.
Findings
Critical: None.
High: None.
Medium
[Medium] New "worker alive, teardown unconfirmed" disposition branch has no ledger-state assertion
- dimension: 15 (Harness changes need EXECUTED evidence)
- location:
tools/launch-worker.mjs:310(theelse if (budgetReservation && reservationMaySpend)branch); asserted only partially bytools/test-tools.mjs:1017-1024 - issue: The
fail()cleanup logic now branches three ways on(reservationMaySpend, workerTornDown): torn-down-and-confirmed closes the reservation as completed-unknown (tested by thenever/busyThroughout/readFailsAfterSendcases, all of which assert.records), never-sent cancels it (pre-existing, tested), but the middle branch — pointer delivered,rollbackalready nulled, then a later post-delivery orca call (terminal switch/worktree set) fails — only logs "left budget reservation ... pending because the worker shutdown could not be confirmed" and leavesbudgetReservationnon-null. ThepostCommitFailurecase drives exactly this branch (stubbingterminal switchto fail) but its assertion only checks exit code, stderr text, and thatterminal stop/worktree rmwere never called; it never readspostCommitFailure.records, even thoughrunPointerLaunchalready computes and returns that field for every other case in the same test group. - risk: The runtime behavior here appears correct by design (a genuinely live, running worker should not have its reservation touched), but it is unverified: a future refactor of the
fail()disposition chain could silently start cancelling or double-closing this branch's reservation and no red test would catch it, reintroducing exactly the "concurrent launchers both pass the same remaining budget" race the ledger lock exists to prevent. - fix: Extend the existing assertion at
tools/test-tools.mjs:1020-1024(or add a siblingT(...)right after it) to assertpostCommitFailure.records.length === 1 && postCommitFailure.records[0]?.pending === true, proving the reservation is left untouched rather than inferring it from the absence of a rollback call. The same gap exists forteardown-worktree.mjs's newclosePendingReservationsfailure path (thefail(3, ...)attools/teardown-worktree.mjs:140when therecordsubprocess itself fails) — worth a companion case in the same pass. - reference:
.claude/skills/pr-review/rubric.mddimension 15 ("A new decision path added to a tool that already has coverage ... needs its own case, not an extension of an existing assertion.")
Low / Info: none posted (signal gate: Low/Info are not posted on a PR review).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A - no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A - no user-facing strings or locale files changed |
| contract-aligner | N/A - no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed |
| security-reviewer | N/A - no orbit-api code changed |
| design-reviewer | N/A - no apps/* UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI Lint check, SUCCESS) |
| Type check | PASS (CI Type Check check, SUCCESS) |
| Tests | PASS (CI Unit Tests check, SUCCESS) |
Harness Execution (node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) |
PASS (both CI Harness Execution checks, SUCCESS) |
| Build (api) | N/A - orbit-api not touched |
This review session's own sandbox blocked direct local execution of node tools/test-tools.mjs
and node .claude/hooks/test-hooks.mjs (both required an approval this session could not obtain),
so the Harness Execution evidence above is read from the CI job's own run
(gh pr view --json statusCheckRollup) rather than a local re-run. The PR body additionally quotes
both gate invocations with exit 0 and their ORBIT TOOLS GATE OK / ORBIT HOOK PARITY OK
sentinels, plus a live read-only automation-budget.mjs check probe against the real production
ledger with an unchanged SHA-256 before/after.
What's good
- The three-state model (
pending/completed-unknown/ measured) is a genuine root-cause fix,
not a workaround: it replaces "any missing measurement blocks everything" with a disposition
that distinguishes a live reservation from a worker that finished without a chance to record. runRecord's idempotent guard (tools/automation-budget.mjs:640) correctly lets a real
provider measurement win over a later completed-unknown closeout racing in behind it, verified by
the "preserve-measurement" test.validateRecord's new mutual-exclusion checks (pending+tokens, pending+completed) turn a
malformed ledger line into an immediateexit 3instead of a silently wrong summary.- The acceptance evidence is unusually rigorous for a tooling change: a live probe against the real
ledger with an unchanged SHA-256, not just a synthetic fixture run. missingIdentitieswas fully removed (verified via repo-wide grep); no stale references left in
either code or docs.
Deferred
- CI's own "Claude PR Review" run (the
reviewcheck) was still in progress at the time of this
pass; its independent verdict is not reflected here. - No dimension was skipped as unexamined: dimensions 8-14 (DESIGN.md, parity, i18n, contract drift,
security, backend hard rules, FEATURES.md) are genuinely N/A because this diff touches only
tools/**.
Recommendation
Merge as-is; the one Medium finding is a test-coverage gap on an already-correct code path, not a
functional defect, and does not block. Land the two suggested test-tools.mjs assertions
(postCommitFailure.records, and a closePendingReservations failure case) whenever this file is
next touched, so the "worker alive, teardown unconfirmed" and "ledger-closure subprocess failure"
paths stop relying on inference.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
No code change required: IC_kwDOR5Siws8AAAABMeLK4A reports that Codex found no major issues on the current head. Evidence: 952fd28. |
There was a problem hiding this comment.
Review: PR #656 (ORB-158: close the spend ledger loop)
Scope: PR #656, thomasluizon/orbit-ui-mobile. This PR has been through eight prior review rounds (both /pr-review and Codex). Per the task, this pass is scoped to the diff since the last review — commit 6429c66160 (APPROVED, 2026-07-30T13:17) → current HEAD 952fd28d ("fix: scope teardown ledger closure ORB-158"), a 4-file, ~92-line diff.
Recommendation: APPROVE
What I independently re-verified (not just re-flagging prior findings)
Before trusting the "resolved" status of earlier rounds, I read the current head directly rather than the diff text: tools/launch-worker.mjs:303-318 (the fail() budget-cleanup conditional) genuinely fixes the repeatedly-flagged HIGH from earlier rounds (mid-loop orca() failure stranding a reservation pending forever) — workerTornDown is now set synchronously inside the same rollback block that already sets cleanupConfirmed, so a failure inside the pointer-delivery loop (after an accepted send) now correctly falls into the reservationMaySpend && workerTornDown branch and closes the reservation as completed-unknown, exercised by the readFailsAfterSend test. That bug is closed, and I'm not re-flagging it.
Findings
Medium
[Medium] New --identity-prefix decision paths in automation-budget.mjs ship without a
direct automation-budget.mjs-level test
· dimension: 15. Harness changes need EXECUTED evidence
· location: tools/automation-budget.mjs:414 (readLedger's valid-JSON-record prefix filter)
and :617 (runRecord's "--identity must begin with --identity-prefix" refusal)
· issue: This commit introduces the entire --identity-prefix mechanism (report + record
flags, plus the readLedger filtering that backs it). It is only exercised indirectly,
as a side effect of teardown-worktree.mjs spawning automation-budget.mjs as a
subprocess (tools/test-tools.mjs:3428-3492). Two paths have zero coverage anywhere in
the suite: (a) the refusal at automation-budget.mjs:617 — unreachable via any current
call site because closePendingReservations always derives a matching prefix from the
reservation's own identity, so nothing ever drives a mismatched pair through it; (b)
the record-level isolation guarantee at automation-budget.mjs:414 — every staged
teardown fixture (closes-ledger, unrelated-corruption, out-of-window, own-corruption)
contains only ONE issue's identity, so no test proves a *valid, well-formed* pending
record for a *different* issue in the same ledger file survives untouched when this
issue's teardown runs "automation-budget.mjs report --identity-prefix". That isolation
guarantee is the explicit purpose of this commit ("scope teardown ledger closure"), so
it deserves its own positive proof, not just an absence-of-corruption-crash proof.
· risk: A future edit to either branch (e.g. a refactor that merges the corrupt-line skip
with the valid-record filter) could silently start leaking one issue's teardown into a
sibling issue's still-pending, possibly-live reservation, with no red test to catch it —
the same class of regression this exact PR spent five rounds fixing on the launcher side.
· fix: Add two automation-budget.mjs cases to automationBudgetCases(): (1) a ledger with
a well-formed pending record for ORB-124 and one for ORB-999 in the same file, run
`report --engine claude --identity-prefix ORB-124: --json`, assert only the ORB-124
identity appears in pendingRecords/pendingIdentities; (2) a `record --identity ORB-999:x
--identity-prefix ORB-124:` call, assert exit 2 and the exact "--identity must begin
with --identity-prefix" text.
· reference: rubric.md dimension 15 ("a new decision path... needs its own case, not an
extension of an existing assertion") — the same standard six prior rounds on this PR
applied to automation-budget.mjs's other new branches.
No Critical or High findings survive in the diff since the last review.
Subagents
None fired — diff is scoped entirely to tools/**; no apps/*, i18n, packages/shared/src/types, or orbit-api surface touched.
| Agent | Verdict |
|---|---|
| parity-checker | N/A |
| i18n-syncer | N/A |
| contract-aligner | N/A — not verifiable in CI (sibling repo not checked out); also genuinely out of scope |
| security-reviewer | N/A |
| design-reviewer | N/A |
Validation
| Check | Result |
|---|---|
| Lint / Type check / Unit Tests (PR Tests) | PASS (CI, SUCCESS) |
| Harness Execution (Guards) | IN_PROGRESS at review time — confirm green before merge |
| Harness Lockstep (Guards) | PASS (CI, SUCCESS) |
| Contract Drift / Cross-Platform Parity (Guards) | PASS (CI, SUCCESS) |
| SonarCloud | PASS (Quality Gate) |
Local node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs |
Not executable in this sandboxed session (same restriction every prior review round on this PR hit) |
Note: the PR body's pasted acceptance evidence (live ledger probe, gate output) is pinned to commit 6429c66160, one commit behind the current head 952fd28d. It does not cover the --identity-prefix mechanism added in the final commit. Confirm the in-progress CI Harness Execution run (which does cover HEAD) completes green before merging.
What's good
- The mid-loop orca-failure bug that survived three earlier "approve" rounds without being re-checked is genuinely fixed in this head (verified by direct code read, not by trusting the prior thread).
teardown-worktree.mjs's reordering (report success + REMOVED lines before the ledger-closure attempt) is a real improvement: a ledger-side hiccup no longer blocks or obscures the actually-important worktree teardown, and it's pinned by the newown-corruptiontest.- The out-of-window exclusion in the new
pendingReservationsForIssueis self-consistent with howevaluateBudgetalready windows records — a record that ages out can never re-enter a future window either, so leaving it un-closed is functionally inert, not a leak. Correctly pinned by the newout-of-windowtest.
Recommendation
Approve. The one surviving Medium is a test-coverage gap on already-correct-by-inspection logic (verified by reading readLedger's filter and runRecord's refusal directly), consistent with how every earlier round on this PR treated analogous gaps — fix soon, doesn't block. Confirm the in-progress Harness Execution CI run finishes green before merge.
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
|
@codex review |
|
No code change required: IC_kwDOR5Siws8AAAABMfEQVQ reports a passed SonarCloud Quality Gate with zero new issues. Evidence: d0b52d5. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Review Complete
Scope: PR #656, thomasluizon/orbit-ui-mobile (tools/README.md, tools/automation-budget.mjs, tools/launch-worker.mjs, tools/teardown-worktree.mjs, tools/test-tools.mjs — tooling-only, no apps/*, packages/shared, or orbit-api)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 3 |
| Low / Info | 0 |
Findings (Medium, none block merge):
tools/automation-budget.mjs:437-440— legacy ledger records written before this PR (nopendingmarker existed yet) get silently reclassified from "blocks" to "does not block" the fuse on first read after this deploys, sincesummarize()buckets any un-measured, non-pendingrecord asunknownIdentitiesrather thanpendingIdentities. Bounded blast radius (single local ledger, self-heals over the 7-day window, no mobile-client angle), but it's an unremarked regression of the tool's own "fails closed" contract for data already on disk.tools/automation-budget.mjs:617— the new--identity-prefixmismatch rejection (--identity must begin with --identity-prefix) has zero test coverage;grepfor the message intest-tools.mjsreturns nothing.tools/teardown-worktree.mjs:105-129— no teardown fixture seeds a second, genuinely pending reservation for a different issue to prove--identity-prefixcross-issue isolation holds against real data (only malformed/out-of-window noise is tested).
What's good: the pending/completed-unknown state model is a real code-judo move that fixes the actual round-7 evidence gap; launch-worker.mjs's fail() correctly distinguishes "never delivered, safe to roll back and cancel" from "delivered, a live worker may be running, never touch the worktree or force-close its reservation" (traced and verified correct); test coverage for the new branches is unusually thorough for a tools/ PR.
Validation: could not re-run tools/test-tools.mjs / .claude/hooks/test-hooks.mjs locally in this session, so CI's actual conclusions were pulled via gh pr view --json statusCheckRollup instead of trusting self-reported logs: both Harness Execution (Guards) check runs on this head are COMPLETED / SUCCESS — satisfying rubric dimension 15's execution-evidence requirement independently of the author's claim.
Deferred: dimensions 8-14 (DESIGN.md, parity, i18n, contract drift, security, backend hard rules, FEATURES.md) are all N/A — the diff touches only tools/**, no apps/*/orbit-api/packages/shared/src/types surface. All five gated subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) are N/A for the same reason. Every changed file got a full-file read and a verdict.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review: PR #656 (orbit-ui-mobile)
Scope: PR #656 in thomasluizon/orbit-ui-mobile - "ORB-158: close the spend ledger loop"
Recommendation: APPROVE
Summary
The diff closes the last gap in the automation spend ledger: a worktree torn down without a
worker-authored completion record (teardown after escalation, an undelivered pointer, a
post-handoff failure, a busy-terminal ambiguity) used to leave its reservation pending
forever, permanently starving that identity's future accounting. The PR introduces an explicit
pending vs. completed (unknown-measurement) distinction in the ledger schema, teaches
automation-budget.mjs record/report an --identity-prefix guard so a close only ever touches
its own issue's identities, has launch-worker.mjs close its own reservation as
completed-unknown whenever it tears down a worktree after the worker may have started spending,
and adds a teardown-worktree.mjs safety net that closes any reservation still pending for a
verified-Done issue, printing a prefix-guarded recovery command and the ledger path if that
closure itself fails. All five changed files (tools/README.md, automation-budget.mjs,
launch-worker.mjs, teardown-worktree.mjs, test-tools.mjs) were read in full; the new
decision paths (pending vs. completed-unknown, prefix-scoped record/report, torn-down vs.
live-worker reservation handling, closure failure + recovery) each have their own dedicated test
case rather than an extension of an existing assertion, and the reservation-preservation edge
case (closing-as-unknown must never clobber a worker's real measurement that lands in the same
race window) is explicitly covered and passes.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[Info] Legacy unmarked ledger records lose fail-closed protection on this deploy
· dimension: Correctness (#1)
· location: orbit-ui-mobile/tools/automation-budget.mjs:437-440
· issue: summarize() now only treats a token-less record as blocking (pendingIdentities) when it explicitly carries pending: true. Any record written by the pre-PR reserve (which never set that flag) that is still genuinely in flight at the moment this PR deploys will be reclassified as unknownIdentities - completed-but-unknown, non-blocking - rather than continuing to fail closed.
· risk: None observed in practice here (the PR body's real-ledger SHA-256 is identical before/after, i.e. no live legacy-format pending record existed at deploy time), and this is a one-time migration-window edge case in a single-user local ledger, not a shipped-client contract. Flagging only because it's a real behavior change to pre-existing ledger data, fully intentional and already covered by a dedicated test (the recorded legacy unmarked invocation admits the next routine launch as unmeasured) and documented in the updated tools/README.md line.
· fix: No action needed; this is accepted, tested, documented behavior for a solo-owned ledger file. Noting for the record in case a genuinely-live legacy reservation exists at rollout time elsewhere.
· reference: CLAUDE.md rule 1 (root cause) / rule 5 (error handling at boundaries) - informational only, not a defect.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A - no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A - no user-facing strings or locale JSON changed |
| contract-aligner | N/A - no packages/shared/src/types/* / endpoints.ts, and only one repo changed |
| security-reviewer | N/A - no orbit-api code changed |
| design-reviewer | N/A - no apps/* or orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI: PR Tests / Lint, SUCCESS) |
| Type check | PASS (CI: PR Tests / Type Check, SUCCESS) |
| Tests | PASS (CI: PR Tests / Unit Tests, SUCCESS; harness evidence below) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS (CI: Guards / Harness Execution, SUCCESS on both required runs; PR body also cites a full local run with runtime 1382.3s and the specific new PASS lines) |
| Harness Lockstep | PASS (CI: Guards / Harness Lockstep, SUCCESS) |
| Cross-Platform Parity | PASS (CI: Guards / Cross-Platform Parity, SUCCESS - expected, no apps/* touched) |
| Contract Drift | PASS (CI: PR Tests / Contract Drift, SUCCESS - expected, no shared/API types touched) |
Local execution of /validate was not run fresh in this review session (this session's sandbox
declined to execute repo scripts); the table above is sourced from gh pr view --json statusCheckRollup against this PR's head commit 15a5532a, where every required gate reports
SUCCESS, plus the PR body's own cited local run of both harness commands with their PASS output.
Deferred - N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A, no
apps/*UI surface in this diff. - Parity (#9), i18n (#10), Contract drift + backward-compat (#11): N/A, no
apps/web,apps/mobile,packages/shared/src/types, ororbit-apifiles in this diff; each also independently confirmed green by its CI gate. - Security (#12) beyond general read: N/A, no auth/user-input/API surface - this is local CLI tooling operating on a developer-owned ledger file.
- Backend hard rules (#13): N/A,
orbit-apinot touched. - FEATURES.md parity (#14): N/A, no user-facing feature/screen/tool/gating surface changed - this is internal orchestration tooling.
- Every changed file (
tools/README.md,tools/automation-budget.mjs,tools/launch-worker.mjs,tools/teardown-worktree.mjs,tools/test-tools.mjs) received a verdict above; nothing left unexamined.
Nothing else deferred.
What's good
- The reservation-preservation race (closing-as-unknown must not clobber a worker's real
measurement that lands concurrently) is explicitly named and tested
(a completed-unknown closeout preserves a worker's existing authoritative measurement),
not just assumed safe. --identity-prefixis enforced on the write side too (runRecordfails if--identity
doesn't start with--identity-prefix), so a teardown for one issue structurally cannot
mutate another issue's reservation even if a future caller passes the wrong identity.- The failure-recovery path is a genuine loop, not just a message: the PR body's own evidence
runs the printed recovery command against a staged ledger and shows the reservation actually
clears, andtest-tools.mjshas the same round-trip as an automated case
(the recovery command closes the stranded reservation/the recovery command closes the reservation after removal without its worktree). - Clause renumbering in the injected
WORKER_CONTRACT(new clause 6 inserted before the old
6-11) was checked for dangling cross-references (clause 4,clause 5) - both still point at
the correct clauses after the insertion. tools/README.md's tool-description prose was updated in the same edit to match the new
behavior (pending vs. completed-but-unknown, exit-3 recovery guidance) rather than drifting
from the code.
Recommendation
Ship as is. No blocking findings; the one Info note is a documented, tested, intentional
migration-window tradeoff on a single-developer local ledger file, not a defect to fix.
There was a problem hiding this comment.
Code Review: PR #656
Scope: PR #656 in thomasluizon/orbit-ui-mobile (feature/orb-158-close-the-spend-ledger-loop → main), reviewed at commit 15a5532a
Recommendation: NEEDS WORK
Correction notice
A duplicate run of this review posted an APPROVE for this same commit moments ago, before an
in-flight adversarial skeptic check had returned. That skeptic check has now confirmed a real,
previously-unflagged HIGH-severity defect in this PR's own new safety-net mechanism. This review
supersedes that APPROVE.
Summary
Earlier rounds on this PR correctly fixed a HIGH finding in tools/launch-worker.mjs (a mid-loop
orca() failure stranding a reservation permanently pending) — verified: the current head derives
the completed-unknown closure from reservationMaySpend && workerTornDown
(tools/launch-worker.mjs:303), which is exactly the earlier fix recommendation, and prints an
operator-visible message on the remaining unresolved branch (:311). That finding is genuinely
resolved; not re-flagged.
The commits since that round (6429c661, 952fd28d, d0b52d54, 15a5532a) add a second,
independent safety net: teardown-worktree.mjs now inspects and closes any reservations left
pending for a ticket after its worktree is verified removed and its Linear issue is Done
(pendingReservationsForIssue / closePendingReservations, tools/teardown-worktree.mjs:105-162).
This new mechanism has its own reachable failure mode that undercuts the PR's stated goal the same
way the earlier one did.
Findings
High
[HIGH] Teardown's pending-reservation closer aborts on first failure, stranding later reservations with no recovery command, un-scoped to the ticket, and with no re-run path
· dimension: 1. Correctness (see also 15. Harness changes need EXECUTED evidence)
· location: tools/teardown-worktree.mjs:105-129 (pendingReservationsForIssue), :143-162 (closePendingReservations), and the call order at :239 (worktree removal) vs. :259-260 (reservation closure runs after); tools/automation-budget.mjs:543-587 (runCheck/runReserve call readLedger(ledgerPath(values)) / readLedger(path) with no identityPrefix); tools/launch-worker.mjs:504-537 (reserveAutomationBudget's reserve call never passes --identity-prefix)
· issue: Four compounding gaps, all verified directly against the current head:
pendingReservationsForIssueloopsfor (const engine of ["claude", "codex"])and callsfail(3, ...)— i.e.process.exit(3)— on the first engine's report failure (:118-119). If "claude" fails, "codex" is never inspected.closePendingReservationsloops over every pending reservation and callsfail(3, ...)on the first one that fails to close (:158-159). If reservation A fails, reservation B is never attempted, and only A's recovery command is ever printed — B's is not.- The worktree is already removed and the branch deleted by line 239-255, before this closure logic runs at 259-260. Re-running
teardown-worktree.mjs --issue ORB-Nafter a partial failure hitsif (!worktree) fail(1, ...)at line 168, because the worktree Orca would look up is already gone — there is no supported way to retry the close. - The fuse check that actually blocks future launches (
evaluateBudget→summarize,automation-budget.mjs:420-461) filters only byengine, never by ticket:runCheck/runReservecallreadLedgerwith noidentityPrefix, andlaunch-worker.mjs'sreservecall (the one used for every real launch) never passes--identity-prefixeither. Only the teardown script's own inspection scopes by issue. A reservation this mechanism fails to close therefore blocks routine launches for that engine project-wide, for every ticket, not just the one being torn down.
A concrete reachable path: ORB-124 launched onclaude, crashes, leaves a pending reservation (fuse only sees it onclaude, per-engine). It's relaunched oncodex, also crashes before delivery, leaves a second pending reservation under a different identity (${issue}:${startedAt}:${uuid}, so no collision). It's relaunched oncodexagain and completes; the ticket reaches Done. Teardown now finds two pending reservations across two engines. If closing the first fails (e.g. a transient ledger-lock contention), the second is silently orphaned — no message, no recovery command — and it will keep failing closed every futureclaudelaunch project-wide until someone manually finds and repairs the ledger line.
Confirmed no test exercises this:tools/test-tools.mjs:3439-3465and:3513-3530each stage exactly one pending reservation / one inspection failure; none stages two.
· risk: This is the exact class of regression an earlier round on this PR flagged as HIGH inlaunch-worker.mjsand got fixed — a partial-failure window in the PR's own new "close the loop" logic that instead permanently, silently blocks automation, directly against the PR's stated purpose (ORB-158: "close the spend ledger loop").
· fix: In both loops, continue past a per-item failure and accumulate errors/recovery commands, exiting3once at the end with every failing item's recovery command listed (not just the first). Separately, either pass--identity-prefixthrough to the launch-timereserve/checkcalls so the fuse is ticket-scoped, or explicitly document why project-wide scoping there is intentional — right now it reads as an oversight given the teardown script itself does scope by issue. Add atools/test-tools.mjscase with two pending reservations where the first fails to close, asserting the second is still attempted and its recovery command is printed.
· reference: CLAUDE.md rule 1 (root cause); rubric dimension 1 (Correctness) and dimension 15 (a decision path needs its own case for a materially different precondition — here, N>1 pending reservations).
Low / Info
[INFO] summarize()'s fail-closed check only applies to token-less records explicitly carrying pending: true
· dimension: 15
· A genuinely-live pre-PR-format reservation without pending: true would be reclassified as
non-blocking (unknownIdentities, not pendingIdentities) rather than fail-closed. This is
intentional and documented in tools/README.md, and covered by its own test ("the recorded
legacy unmarked invocation admits the next routine launch as unmeasured"). Informational only.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or locale JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts/orbit-api change; CI's "Contract Drift" check is SUCCESS |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Diff is scoped entirely to tools/** (automation-budget.mjs, launch-worker.mjs, teardown-worktree.mjs, test-tools.mjs, README.md), so none of the five gated subagents fire.
Validation
Local /validate and the skill's own Phase 6 skipped per this CI job's scope (Build / Unit Tests /
SonarCloud run as separate required checks). Citing the PR's own completed CI runs for head 15a5532a:
| Check | Result |
|---|---|
| Lint / Type check / Unit Tests (PR Tests) | PASS (CI, SUCCESS) |
| Harness Execution (Guards) | PASS (CI, SUCCESS) |
| Harness Lockstep (Guards) | PASS (CI, SUCCESS) |
| Contract Drift (PR Tests) | PASS (CI, SUCCESS) |
| Cross-Platform Parity (Guards) | PASS (CI, SUCCESS) |
| Build (api) | N/A — orbit-api not touched |
CI green does not cover this finding: no test stages the two-reservation partial-failure scenario.
Deferred
- Dimensions 8-14 (DESIGN.md, Parity, i18n, Contract drift/backward-compat, Security, Backend hard rules, FEATURES.md): all N/A — diff touches only
tools/**. Contract drift/backward-compat additionally not verifiable in this CI job (siblingorbit-apinot checked out), though also genuinely out of scope for this diff. - All five changed files received a verdict; the surviving High finding was independently re-verified against the current head by reading the exact line ranges cited above and by an adversarial skeptic pass.
What's good
- The earlier HIGH in
launch-worker.mjs(mid-loop orca failure stranding a reservation) is genuinely fixed:reservationMaySpend && workerTornDownnow drives the completed-unknown branch, with an operator-visible message on the remaining gap. - The three-state ledger model (
pending/ completed-unknown / measured) is a real improvement over the prior binary fail-closed model. - The regression test replays the exact live ORB-153 production ledger line.
Recommendation
Fix the partial-failure handling in teardown-worktree.mjs's new pending-reservation closer before
merge: continue past per-item failures so every recovery command surfaces, and resolve the
project-wide (non-ticket-scoped) fuse-blocking behavior it can trigger. This is the same class of
regression — a new "close the loop" mechanism with an unguarded partial-failure window — that an
earlier round already caught and fixed elsewhere in this same PR.
…-the-spend-ledger-loop
Teardown's post-removal reservation closer aborted on the first failure, so a second pending reservation was silently orphaned with no recovery command, and a re-run could not reach it because the worktree was already gone. Both loops now continue past a per-item failure and accumulate every failure, exiting 3 once with each failing item's own runnable recovery or inspection command. The winning hypothesis: the failure was not the closure itself but the early process.exit inside the loops, which made the first error the only error anyone could see. Also documents why the fuse reads the ledger without --identity-prefix: the budget it defends is the engine's whole weekly account quota, shared by every ticket, so scoping the fuse per ticket would let each ticket spend the whole budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJti2frpAVnzKEKVQ5CoA4
|
Round 8: the surviving HIGH from the review at [HIGH] Teardown's pending-reservation closer aborted on first failure. Fixed, all four sub-points:
Tests added, exactly the case the review asked for plus its inspection twin:
Executed evidence at Merge with |
|
There was a problem hiding this comment.
Code Review: PR #656 (thomasluizon/orbit-ui-mobile)
Scope: PR #656, feature/orb-158-close-the-spend-ledger-loop -> main (ORB-158 round 7): tools/README.md, tools/automation-budget.mjs, tools/launch-worker.mjs, tools/teardown-worktree.mjs, tools/test-tools.mjs.
Recommendation: NEEDS WORK
Summary
This PR closes out the automation-spend ledger loop: it adds an explicit pending / completed discriminator to ledger records, teaches automation-budget.mjs record/report a scoped --identity-prefix, and has teardown-worktree.mjs and launch-worker.mjs close any reservation left dangling after a torn-down worker as "completed-but-unknown" instead of leaving it stuck forever. The mechanism is well engineered and heavily tested (new lock-contention, partial-failure, and out-of-window cases). It touches only tools/**, so parity, i18n, DESIGN.md, contract-drift, and backend-hard-rule dimensions are all N/A. One finding survived adversarial skeptic review: the new pending-marker scheme silently downgrades the fuse's fail-closed guarantee for every reservation already sitting in the real ledger before this PR deploys.
Findings
Critical
[Critical] The new pending marker silently un-blocks every pre-existing ledger reservation, defeating the fuse's own fail-closed contract
· dimension: 1. Correctness
· location: tools/automation-budget.mjs:353-367 (validateRecord), tools/automation-budget.mjs:438-451 (summarize), tools/automation-budget.mjs:489-492 (evaluateBudget), tools/automation-budget.mjs:565-572 (runReserve)
· issue: Before this PR (git show 375dcf2f:tools/automation-budget.mjs), runReserve appended a bare record (identity/engine/tier/startedAt/endedAt, no discriminator), and summarize/evaluateBudget treated ANY record lacking inputTokens/outputTokens as "missing" and failed routine tier closed, unconditionally. This PR adds a pending: true marker that runReserve now writes (automation-budget.mjs:571), and rewires the fuse to only fail closed on summary.pendingIdentities — records where record.pending === true (automation-budget.mjs:443, automation-budget.mjs:489). validateRecord accepts a record carrying neither pending nor completed (a "legacy unmarked" record) without normalizing it (automation-budget.mjs:353-359 is a no-op when both are absent), and summarize buckets that record into unknownIdentities, not pendingIdentities (automation-budget.mjs:442-445). There is no migration step anywhere in this PR (automation-budget.mjs, launch-worker.mjs, teardown-worktree.mjs) that back-fills pending: true onto records written by the OLD runReserve before this deploy.
· risk: The real ledger (~/.orbit/automation-budget.jsonl, evidenced by this PR's own before/after SHA-256 in the PR body, which is unchanged) is a persistent file that survives the deploy. Any reservation written by the pre-PR code — every one of them, since none of them can carry pending: true — is structurally indistinguishable from a "legacy unmarked" record once this PR ships. On the very next check/reserve call, such a record silently reclassifies from "blocks routine automation" (correct, pre-PR behavior) to "unknown, warning-only, does not block" (post-PR behavior). If any such record represents an invocation whose true spend was never measured — precisely the scenario the fuse exists to catch — new routine automation is now permitted to launch blind, exactly the outcome "the fuse fails closed" was built to prevent. This is confirmed, not speculative: this PR's own new test fixture recordedLedger (tools/test-tools.mjs:5533-5546) encodes exactly this record shape (bare, no pending/completed, matching the pre-PR runReserve output byte-for-byte) as the intended new "PROCEED, unmeasured, non-blocking" outcome — the regression is asserted as a feature, not caught as a bug.
· fix: Treat "no pending and no completed marker present" as pending (blocking), not unknown — that is the conservative, fail-closed default the tool's whole design principle calls for, and it is also the only interpretation consistent with every record this exact tool has ever written before this PR. Concretely: in validateRecord, default validated.pending = true when neither pending nor completed is present and the record carries no tokens; or equivalently, in summarize, bucket a token-less record with neither marker into pendingIdentities. Reserve unknownIdentities for records this PR's own code explicitly marks completed: true after a teardown/launch-worker closure. This preserves the fail-closed guarantee for every record already on disk while still giving the new closure mechanism (teardown-worktree.mjs, launch-worker.mjs) a way to retire a reservation permanently.
· reference: CLAUDE.md rule 1 (root cause over workarounds — the safety invariant belongs in the schema default, not left to every caller to know the deploy history of the file); tools/README.md's own automation-budget.mjs description ("the fuse fails closed... does not block routine work" is the intended, narrower post-PR contract, but nothing in the repo enforces or migrates existing data into it).
(Adversarial skeptic pass: SURVIVED. An independent subagent re-derived all five sub-claims directly from the base commit 375dcf2f and the current tools/automation-budget.mjs/tools/test-tools.mjs, confirmed no migration/backfill path exists anywhere in the diff, and confirmed the PR's own test fixture encodes the reclassification as intended. Cross-model second opinion (/second-opinion via codex) was attempted per protocol and is UNAVAILABLE in this environment (codex/opencode absent on this runner), so the finding stands exactly as the skeptic pass left it.)
High
None.
Medium
[Medium] cancelBudgetReservation and recordBudgetReservationAsUnknown in launch-worker.mjs are near-identical spawnSync wrappers
· dimension: 3. SOLID / clean architecture
· location: tools/launch-worker.mjs:540-564 (cancelBudgetReservation) and tools/launch-worker.mjs:566-590 (recordBudgetReservationAsUnknown)
· issue: Both functions build the same spawnSync(process.execPath, [budgetToolPath, ...]) call shape against the same destructured { identity, engineName, tier, startedAt, ledgerPath }, differing only in the subcommand (cancel vs record) and one hardcoded flag arrangement; both parse the result the same way (!result.error && result.status === 0) and log the same shape of failure message.
· risk: Not a correctness bug, but the next budget-closure variant (there is already a third shape brewing: "close as measured" in automation-budget.mjs record with tokens) will very likely copy-paste a third near-duplicate rather than reuse a shared spawn helper, compounding the drift risk between the three call sites.
· fix: Extract a small shared runBudgetSubcommand(subcommand, { identity, engineName, tier, startedAt, ledgerPath }, extraArgs = []) helper that both call, or fold recordBudgetReservationAsUnknown into the existing runBudgetCommand used elsewhere in this same file, since it already exists precisely to run automation-budget.mjs and interpret its exit code.
· reference: CLAUDE.md rule 7 (DRY at the right level — repeated handler logic to one well-named helper at the narrowest shared layer).
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing string or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — neither repo's packages/shared/src/types/*/endpoints.ts nor orbit-api DTOs changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or orbit-landing-page/src/** UI file changed |
Validation
Repo-tool appeal: quoting the exact PR-cited harness command verbatim is required by rubric dimension 15 (harness changes need an executed command cited, not a paraphrase); this is a code-review citation of evidence the PR author already ran and posted, not an instruction for Thomas to run it himself.
| Check | Result |
|---|---|
| Lint | N/A — no lintable app source in this diff (tools/** only) |
| Type check | N/A — plain .mjs, no TypeScript in this diff |
| Tests (tools harness gate) | Not independently re-run this session (Bash node-execution required an approval this session could not grant). PR body cites a full run of the tools harness gate: PASS ... ORBIT TOOLS GATE OK, runtime 1382.3s. Read the new/changed assertions directly in the harness test file instead of trusting the claim; they match the code's actual branches, with the one exception in the Critical finding above (the suite encodes the regression as intended behavior rather than catching it). |
| Tests (hook parity gate) | Not independently re-run this session, same constraint. PR body cites PASS ... ORBIT HOOK PARITY OK. |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift), 12 (Security — frontend/backend), 13 (Backend hard rules), 14 (FEATURES.md parity), 6 (Type safety): all N/A, this diff touches only
tools/**(noapps/*,packages/shared, ororbit-apisurface). - Dimension 7 (no
console.log): N/A by design —tools/CONVENTIONS.mdrequires exactly theconsole.error/console.logsplit this diff uses (errors to stderr, results to stdout); not a violation. - Harness execution (dimension 15): evidence cited from the PR body only, not independently re-run in this session — Bash access to the harness runtime required an approval this headless review session was not able to grant. Every changed decision path in
automation-budget.mjs,launch-worker.mjs, andteardown-worktree.mjsdoes carry a dedicated new test case in the harness test file (identity-prefix validation, pending/completed contradiction, teardown ledger closure success/failure/partial-failure/out-of-window/corruption), satisfying the "new decision path needs its own case" bar on its face. - All five changed files (
tools/README.md,tools/automation-budget.mjs,tools/launch-worker.mjs,tools/teardown-worktree.mjs,tools/test-tools.mjs) were read in full (not diff-only) and given a verdict; nothing in the changed-file inventory was skipped. - Cross-model second opinion for the Critical finding: attempted per protocol, UNAVAILABLE (
codex/opencodeabsent on this runner) — the finding stands on the skeptic pass alone, as the protocol allows.
What's good
- The closure design is genuinely careful:
closePendingReservationsre-reads the ledger under the SAME lock therecordcommand takes, so a worker that measures its own spend in the narrow race window betweenteardown-worktree.mjs's report and its close call is not clobbered by a spurious completed-unknown record (automation-budget.mjs:650-656). - Failure handling in both
teardown-worktree.mjsandlaunch-worker.mjs's new ledger-closure paths is "attempt everything, report everything" rather than stopping at the first failure — every engine is inspected, every reservation attempted, and each failure prints its own runnable, prefix-guarded recovery command plus the ledger path, so a partial failure never strands a reservation without a named repair. - The
--identity-prefixdesign is deliberately NOT plumbed intocheck/reserve(the account-wide fuse), only into the targetedrecord/reportrepair paths, with the reasoning spelled out in both the--helptext andtools/README.md— a real, load-bearing distinction, not an oversight. - Test coverage for the new branches is unusually thorough: lock-contention during closure, partial closure failure (one of two reservations fails, the other still gets attempted), out-of-window reservations, and unrelated ledger corruption not blocking this ticket's own closure are all separately asserted.
Recommendation
Fix the Critical finding before merge: make the schema default an unmarked, token-less record to pending (blocking) rather than unknown (non-blocking), so every reservation already in the real ledger keeps the fail-closed guarantee it has today. Only a record this PR's own code explicitly marks completed: true should land in unknownIdentities. Add a test case asserting a legacy bare record (no pending, no completed, no tokens) still blocks routine tier before merging — the current recordedLedger fixture should be renamed/repurposed to assert the corrected behavior instead of the regression. The Medium duplication finding is not merge-blocking; fold it in during this PR or a fast follow-up, reviewer's judgment.
The Critical finding is wrong for this PR, with evidence. The Medium is fixed.[Critical] "unmarked token-less records must default to pending (blocking)"This exact change was implemented at 1. ORB-158's acceptance criteria require exactly the current behaviour. Verbatim from the ticket:
The proposed fix makes that fixture exit 3 again. The 2. The records in question are the deadlock itself, not live reservations. Read from the real ledger
3. Under the proposed fix, That is today's state on The safety concern the finding is really about is already closed. The genuine exposure is narrow: a reservation written by the pre-PR binary that is still live when the new binary is installed. Every reservation written after this change carries The unmeasured spend is not silently dropped either. It stays out of totals, is reported under Note this same review job classified this identical behaviour as INFO, "This is intentional and documented" at I have not made this change. Reversing it would ship a PR that fails its own ticket's acceptance criteria and restores a measured, reproduced deadlock. If the fail-closed default is wanted anyway, that is a change to ORB-158's acceptance criteria and belongs to Thomas, not to this PR. [Medium] duplicate
|
|
Closing unmerged. Its Linear ticket was canceled into ORB-163, which carries the full harness repair specification. The behaviour this PR implements is specified in ORB-163 and will ship there, so merging this first changes nothing about the final outcome and only adds review rounds. Reopen if that turns out to be wrong. |
1 participant
ORB-158 round 7 evidenceHead:
15a5532a995a221f8f67d361fba58750acb53432### Recovery after a failed post-removal closureThe tools harness forces a held ledger lock after the worktree, terminals, and local branch have been removed. Teardown exits 3 and names a prefix-guarded recovery command plus the ledger path. After releasing only the test lock, the harness runs that command against the same ledger and verifies the pending reservation becomescompleted: truewith no worktree present.Direct recovery run at this head:textnode tools/automation-budget.mjs record --identity ORB-158:recovery-evidence --engine codex --tier routine --started-at 2030-01-01T00:00:00.000Z --ended-at 2030-01-01T00:00:02.000Z --identity-prefix ORB-158: --ledger <staged-ledger> --json{"status":"RECORDED","record":{"identity":"ORB-158:recovery-evidence","engine":"codex","tier":"routine","startedAt":"2030-01-01T00:00:00.000Z","endedAt":"2030-01-01T00:00:02.000Z","completed":true}}exit 0### Validationtextnode tools/test-tools.mjsPASS check-calibration.mjs: report-only neutralizes malformed artifactPASS check-calibration.mjs: report-only neutralizes missing artifactPASS check-calibration.mjs: help names every flag and exit codeORBIT TOOLS GATE OKexit 0runtime 1382.3 secondsnode .claude/hooks/test-hooks.mjsPASS linear gate: a mutation split from its endpoint by a blank line still blocksPASS linear gate: a mutation in a different chunk from the endpoint still blocksORBIT HOOK PARITY OKexit 0Real ledgerC:/Users/thoma/.orbit/automation-budget.jsonlSHA-256:textbefore EC665555A97FB274E5625FE3EEA27437ECF0EC9F216D1FA49DD00AF487FD19F1after EC665555A97FB274E5625FE3EEA27437ECF0EC9F216D1FA49DD00AF487FD19F1### Fixes- Failed post-removal ledger closure now emits a runnable prefix-guardedautomation-budget.mjs recordrecovery command and the ledger path; the harness drives failure then recovery end to end.-automation-budget.mjs record --helpnow documents--identity-prefix; the shared flag description covers guarded record writes and report reads.-teardown-worktree.mjs --helpandtools/README.mdnow document shared-ledger mutation and every exit-3 source.- The divergent separator regex and vacuous rollback negatives were removed; the test now proves the observed pending reservation after a post-delivery failure.