ORB-131: Build the two-sided quota ledger and routing fuse - #644
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: 3c6103c622
ℹ️ 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".
…-the-two-sided-quota-ledger # Conflicts: # .claude/orchestrator.json # tools/README.md # tools/test-tools.mjs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aeebe3ac0
ℹ️ 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".
|
ESCALATE: Codex reviewed head 0aeebe3 and repeated the P1 that completed launches do not call The blocked decision is the measurement contract:
Please choose one of these contract changes before implementation can continue:
The branch includes current |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #644 (ORB-131 — two-sided quota ledger and routing fuse)
Reviewed against .claude/skills/pr-review/rubric.md. This diff touches only .claude/orchestrator.json, .claude/skills/quota/SKILL.md, and tools/{README.md,ai-quota.mjs,automation-budget.mjs,launch-worker.mjs,test-tools.mjs} — no apps/*, no orbit-api, no packages/shared. The five specialist subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) are all gated out — none fires — so this is purely a dimensions-1/2/3/4/5/6/7/12/15 review of new tooling.
Recommendation: APPROVE
Findings
Medium — The routing fuse this ticket builds never actually fires in production, because nothing calls the writer.
tools/launch-worker.mjs'scheckAutomationBudget()(added ~L663-694) callsautomation-budget.mjs checkbefore every real launch, which sumsmeasuredCostfrom the JSONL ledger. But the only thing that ever appends to that ledger isautomation-budget.mjs record(tools/automation-budget.mjsrunRecord), and nothing outsidetools/test-tools.mjs's own fixtures callsrecord— notlaunch-worker.mjs, notworker-status.mjs, notnudge-worker.mjs, no workflow, no skill. Repo-wide grep plus a dedicated skeptic subagent pass confirmed this: the ledger stays empty in real usage, socheckalways computesweeklyPercent = 0and returnsPROCEED.- This is honestly disclosed in the PR body's own "Unmet items" section (recording is explicitly called out as not wired, with a sound reason: no provider exposes a measured per-invocation percent). Because it's surfaced prominently rather than hidden, this is downgraded from High to Medium. Still worth a concrete follow-up: either wire a
recordcall at the natural completion point (e.g., inworker-status.mjson verified Done) before treating ORB-131 as fully closed, or file the recording work as its own tracked ticket rather than letting "Closes ORB-131" imply the fuse is live. - Reference: root CLAUDE.md rule 1 (root cause) / the ticket's own stated purpose ("Build the two-sided quota ledger and routing fuse").
Medium — The WARN pass-through path isn't covered end-to-end through launch-worker.mjs.
automation-budget.mjs's WARN behavior (20-25%) is unit-tested directly, andlaunch-worker.mjs's BLOCK path is tested end-to-end (budget-blockedcase), but there's no end-to-end case asserting that a WARN-range ledger (21%) letslaunch-worker.mjsproceed to worktree creation while still surfacing the warning on stderr.- Fix: add one
launchWorkerCases()case staging a ~21% routine ledger and asserting exit 0, the warning text on stderr, and thatworktree createwas reached. - Reference: rubric dimension 15 ("a new decision path... needs its own case").
What's good
ai-quota.mjsandautomation-budget.mjsare cleanly separated (reader vs. ledger/fuse), each well under the size/nesting caps, noconsole.logmisuse, no comments beyond usage-text strings.- The retry logic in
readClaudeQuotagenuinely avoids the stale-element-index bug class (re-fetches the Usage control index on every attempt instead of reusing a captured one), and it's specifically tested for that. parseTimestamphand-rolls strict ISO-8601 parsing rather thanDate.parse, specifically to reject timezone-less input thatDate.parsewould silently interpret as local time — a real root-cause fix, not a workaround.- Ledger writes are
mode: 0o600, and the concurrent-append behavior (relying on POSIXO_APPENDatomicity) is verified with an actual concurrent-process test, not just asserted. - Fail-closed design throughout: an unreadable quota, a malformed reset duration, or a missing
automationBudget.tierin config all refuse to launch rather than silently degrading — consistent with root CLAUDE.md rule 1. - Extensive, concrete new test coverage lands in the same PR (
aiQuotaCases,automationBudgetCases, several newlaunchWorkerCases), satisfying rubric dimension 15's "coverage in the same PR" requirement.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** changed |
| i18n-syncer | N/A — no user-facing strings / locale JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* or both-repo change |
| security-reviewer | N/A — no orbit-api changed |
| design-reviewer | N/A — no UI files changed |
CI / Validation
Phase 6 (/validate) is skipped per this run's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR.
Deferred
- Dimensions 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift), 13 (Backend hard rules), 14 (FEATURES.md parity) — all N/A, surface not touched by this diff.
- Backward-compat guard — N/A, no
packages/shared/src/typesororbit-apiDTOs touched; cross-repo dimensions are not verifiable in this job (orbit-api is not checked out here).
No Critical or High findings survive, so this converges to APPROVE.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd420b7940
ℹ️ 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 #644 (ORB-131: two-sided quota ledger)
Scope: PR #644 in thomasluizon/orbit-ui-mobile
Recommendation: REQUEST CHANGES
Summary
This PR adds tools/ai-quota.mjs (reads Claude/Codex quota via Orca UI automation + the Codex JSON-RPC app-server) and tools/automation-budget.mjs (an append-only JSONL token ledger with a routine/reserved fuse), wires both into tools/launch-worker.mjs so a launch fails closed before creating a worktree when the projected token spend would cross budget, and adds substantial new coverage in tools/test-tools.mjs. The diff touches only .claude/** and tools/** (no apps/*, no packages/shared, no orbit-api), so it is pure dev-tooling/harness work. The design is careful about failing closed on missing measurements and about keeping account-level usage percentages as non-attributed context — but the check-then-record pattern that implements the actual fuse is not atomic across the concurrent launches the orchestrator's own workflow performs.
This is a re-review since a prior APPROVE (commit 0aeebe3a). Commit dd420b79 ("Switch quota ledger to token budgets") landed after that review and wired recordAutomationBudget() into launch-worker.mjs for the first time — resolving the prior Medium finding ("nothing calls the writer") but introducing the race below, which only became possible once recording was actually wired in.
Findings
Critical
None.
High
[High] Concurrent worker launches can jointly exceed the token budget: the fuse's check-then-record pattern is not atomic across separate launch-worker.mjs processes
· dimension: 1. Correctness (concurrency/ordering assumption the diff silently relies on)
· location: orbit-ui-mobile/tools/launch-worker.mjs:650 (checkAutomationBudget, runs before worktree creation) and :791 (recordAutomationBudget, runs only after the worktree, TUI, trust-prompt handling, and confirmed pointer delivery — plausibly 10s of seconds later); orbit-ui-mobile/tools/automation-budget.mjs:240 (readLedger), :307 (runCheck), :352 (runRecord), :403 (appendFileSync) — no lockfile/flock/exclusive-open anywhere in the file.
· issue: checkAutomationBudget() shells out to a fresh `automation-budget.mjs check` process that reads the on-disk ledger and compares the CURRENT summed tokens against budget. This invocation's own spend is not written to the ledger (recordAutomationBudget) until well after worktree creation, TUI startup, and pointer delivery succeed. `.claude/skills/orchestrate/SKILL.md` ("## 2. Launch a wave") explicitly launches "Per launchable ticket, up to `maxParallelWorktrees`" and states the next launch fires "only when ... a running member has completed its contract and freed a slot" — i.e. a genuine concurrency cap greater than 1, with multiple `launch-worker.mjs` processes for the same engine sharing the same ledger file live at once.
· risk: Two (or more) launches for the same engine started within that window each read the ledger before the other has recorded its pending spend, so each independently sees room under budget and each passes. Concretely: budget 1,000,000, current spend 950,000, two routine launches each projecting 50,000 tokens — process A's check reads 950k and passes, process B's check (started before A's record) also reads 950k and passes, both record, and the ledger ends at 1,050,000 — over budget despite the fuse's stated guarantee to block "before creating a worktree." This defeats the PR's central purpose under its own documented normal usage pattern, not an obscure edge case.
· fix: Make the check-and-reserve a single atomic operation against the ledger file: either (a) have `automation-budget.mjs` support a combined `check-and-reserve` subcommand that opens the ledger with an exclusive lock (e.g. `O_EXCL`/advisory flock via a `.lock` sibling file) for the read-compare-append sequence, or (b) have launch-worker.mjs write the "pending" record immediately after a successful check (still before worktree creation) instead of after pointer delivery, and hold a filesystem lock across both the check and that pending append so no other launch can interleave. Add a test that starts two `launch-worker.mjs` processes concurrently against a shared ledger near the budget boundary and asserts only one proceeds.
· reference: CLAUDE.md rule 1 (root cause — the race is a structural gap in the fuse's own contract, not a downstream symptom); rubric dimension 1 (Correctness — concurrency/ordering assumptions)
Medium
[Medium] The Windows-specific codex app-server spawn path is both untested and its process cleanup is unverified
· dimension: 15. Harness changes need EXECUTED evidence (new decision path with no dedicated case) + 1. Correctness (cleanup on the primary target platform)
· location: orbit-ui-mobile/tools/ai-quota.mjs:143-152 (spawnCodexServer: `if (process.env.CODEX_BIN) return spawn(...)` at :145-146 always short-circuits before the `if (process.platform === "win32")` branch at :148-150), and :170 (`child.kill()` inside `finish()`, called against the cmd.exe wrapper process, not the codex.exe grandchild it spawns)
· issue: `spawnCodexServer` checks `process.env.CODEX_BIN` first and returns immediately if set. Every test in `aiQuotaCases()` (tools/test-tools.mjs) sets `CODEX_BIN` via `aiQuotaEnv()`, so the `win32` branch that wraps `codex app-server` in `cmd.exe /d /s /c` is structurally unreachable by the existing suite — it has zero executed coverage, and cannot get any without either unsetting CODEX_BIN in a test or refactoring the branch to be independently testable. Separately, on Windows, `cmd.exe /c "codex app-server"` makes the actual codex process a *child of the cmd.exe wrapper*; `child.kill()` in `finish()` only signals the wrapper PID, and Windows does not cascade termination to grandchildren without a job object or `taskkill /T`, so a timed-out or successfully-finished quota read may leave an orphaned `codex app-server` process running. `ai-quota.mjs` runs on every worker launch (via `checkAutomationBudget`) as well as via `/quota`, so this would accumulate over the lifetime of the launcher on the machine it is actually used on (Windows, per `.claude/orchestrator.json`'s ORCA_BIN default path and the codex worker notes).
· risk: A silent, unbounded-over-time resource leak of orphaned `codex app-server` processes on the primary automation machine, and a code path (the one actually exercised in production, since CODEX_BIN is a test-only override) that ships with no execution evidence at all — exactly the gap dimension 15 exists to catch ("only running it caught that").
· fix: Add a test that exercises the win32 branch directly (e.g., factor the platform check into an exported/injectable function, or add an env var like `AI_QUOTA_FORCE_PLATFORM` for tests only) so the actual production spawn shape gets executed at least once. For cleanup, kill the process tree rather than the wrapper alone on Windows (`taskkill /pid <pid> /T /F` via spawnSync, or spawn with `{ detached: false }` plus tracking the codex.exe PID once codex itself prints something identifying it, or avoid the cmd.exe wrapper entirely by spawning `codex.exe` directly with `shell: true` only where required).
· reference: rubric dimension 15 ("A new or changed script under tools/ with no matching coverage... is High" / "a new branch on an external tool's payload needs its own case... Medium"); CLAUDE.md rule 8 (error handling / cleanup at boundaries)
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 i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts, and only one repo (ui-mobile) changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
Validation
Phase 6 (/validate) skipped per this run's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR (SonarCloud quality gate has passed per the bot comment on this PR). The repo's tool and hook harness suites were not independently re-run in this session (sandboxed, no interactive approval available for shelling out); the PR's prior self-reported execution output was not re-verified here.
What's good
- The core design principle — fail closed on any missing/unparseable quota or token measurement rather than guessing, defaulting to zero, or inferring cost from account usage percentage — is applied consistently and is exactly right for an unattended-automation guardrail.
- The escalation trail on this PR shows real engineering judgment: rather than fabricate a weekly-percentage measurement no provider actually exposes, the author raised the contract gap explicitly and resolved it by relaxing the ledger to token budgets (
dd420b79), which is a sound resolution and correctly wiresrecordintolaunch-worker.mjsfor the first time (closing the prior review's Medium finding). - Account-level usage percentage is correctly kept as non-attributed context (
accountContext.attributed === false) and never feeds the token fuse. - Rollback/idempotency discipline is preserved: the budget check runs before any worktree is created, so a blocked launch leaves nothing to clean up.
- Genuinely thorough new test coverage for the two new tools (provider availability, stale element-index reuse, threshold boundaries, malformed ledger lines, reserved-tier overflow, missing-measurement fail-closed, concurrent-append atomicity at the ledger-write layer).
Recommendation
Fix the check-then-record race before merge (High) — this is the one finding that changes the outcome, and it undermines the ledger's stated purpose under the orchestrator's own normal parallel-launch usage; it only became reachable once dd420b79 wired recording in, so it is new since the prior APPROVE, not a re-flag. The Windows spawn-path test-coverage/cleanup gap (Medium) is worth a follow-up in the same PR if easy, otherwise a tracked ticket is acceptable.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78339020fb
ℹ️ 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".
…-the-two-sided-quota-ledger # Conflicts: # tools/launch-worker.mjs # tools/test-tools.mjs
|
@codex review |
|
Review fixes are pushed on 05bcd30. The High finding is resolved by one locked reserve operation before worktree creation, plus append-only cancellation after verified rollback and a deterministic two-launcher test. The Medium finding is resolved by process-tree termination on Windows and Unix-like platforms, with a forced production-path Windows test. The exact merged tree passes the full tools gate, hook parity, lint, type-check, and all workspace tests. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05bcd300f8
ℹ️ 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 #644
Scope: PR #644 in orbit-ui-mobile (ORB-131: Build the two-sided quota ledger and routing fuse)
Recommendation: NEEDS WORK
Summary
This PR adds a two-provider quota reader (tools/ai-quota.mjs) and an append-only, lock-protected token ledger with an atomic pre-launch reservation fuse (tools/automation-budget.mjs), then wires both into tools/launch-worker.mjs so a launch is blocked before any worktree is created when it would cross the configured routine token budget. The diff is tooling-only (.claude/, tools/) — no apps/*, packages/shared, or orbit-api surface is touched, so parity, i18n, contract-drift, security-reviewer (API), and design-reviewer are all N/A.
This is a re-review since my prior CHANGES_REQUESTED (submitted 2026-07-28T20:40:55Z), which flagged a check-then-record race across concurrent launches. Commit 4ce0f166 ("Make quota reservations atomic") resolves that finding by introducing a reserveAutomationBudget step. That fix introduces a new, narrower failure window (below), and a follow-up commit (05bcd300) isolates launcher fixtures without adding coverage for one existing subcommand. Both new findings were independently verified against the current code in this session (not re-flags of prior findings).
Findings
Critical
None.
High
[HIGH] Unguarded prompt-file write between budget reservation and worktree creation can wedge the whole engine's automation fuse
· dimension: 1. Correctness (concurrency/ordering assumption) — also missing error handling
· location: tools/launch-worker.mjs:804-817
· issue: reserveAutomationBudget(...) (line 804) durably appends a pending ledger record before any cleanup mechanism is armed for it. The next mutating statement, appendFileSync(promptFile, WORKER_CONTRACT, "utf8") (line 817), is a raw synchronous fs call with no try/catch. If it throws (EACCES, ENOSPC, etc.), the exception is uncaught: fail() (tools/launch-worker.mjs:238-281) — the only place that invokes cancelBudgetReservation (line 273) — is never reached, and the registered process.on("exit", releaseConcurrencyReservation) handler (line 236) only releases the concurrency lock, not the budget reservation. rollback is also still null at this point (only set after orca worktree create succeeds, line 833), so there is no worktree to roll back either — but the pending, unmeasured ledger record survives.
· risk: Every subsequent check/reserve for that engine hits automation-budget.mjs's fail-closed rule (a pending record lacking token measurement blocks with exit 3), regardless of ticket or tier. One rare fs failure on one launch permanently halts ALL automated launches for that engine until a human finds the stuck record and issues a matching automation-budget.mjs cancel.
· fix: Wrap the appendFileSync call in try/catch that calls fail(3, ...), or move reserveAutomationBudget(...) to be the last statement before orca(["worktree", "create", ...]) so no unguarded fs call sits between the reservation and a fail()-covered call.
· reference: CLAUDE.md rule 8 (error handling at boundaries); rubric dimension 1.
[HIGH] automation-budget.mjs report subcommand ships with zero executed test coverage
· dimension: 15. Harness changes need EXECUTED evidence
· location: tools/automation-budget.mjs (runReport, invoked at line 611); tools/test-tools.mjs
· issue: automation-budget.mjs ships five subcommands (check, reserve, record, cancel, report — line 80). check/reserve/record/cancel each get dedicated test cases in test-tools.mjs. report — including its distinct non-JSON text-formatting branch — is invoked nowhere in the test file; the only matches for "report" are usage-string references, never an actual run("automation-budget.mjs", ["report", ...]) call.
· risk: Exactly the dimension-15 hazard by name: a new script merges with one whole subcommand never having been run once.
· fix: Add at least one case exercising automation-budget.mjs report --engine ... --reset-at ... --ledger ... in both --json and plain-text form.
· reference: .claude/skills/pr-review/rubric.md dimension 15.
Medium
[MEDIUM] No staleness/timeout recovery for a pending ledger reservation whose worker never records its measurement
· dimension: 1. Correctness
· location: tools/automation-budget.mjs (pending-record fail-closed check); tools/launch-worker.mjs:890-891
· issue: Once a worker's pointer is delivered (reservationMaySpend = true), the pending record is only closed by the worker itself running record. No TTL, no orchestrator-side sweep, no automatic recovery if the worker's session is killed or times out before finishing.
· risk: Same blast radius as the first High finding (engine-wide block), reachable through an ordinary operational event (closing a stuck worker), not just an fs error.
· fix: Have report (or a new flag on check) surface identities whose pending record exceeds a staleness threshold, so an operator can spot and cancel a stuck reservation proactively.
· reference: rubric dimension 1.
Low / Info
[INFO] Unix process-group termination of the Codex app-server child is exercised implicitly but never directly asserted
· dimension: 15
· location: tools/ai-quota.mjs (POSIX process.kill(-child.pid, "SIGTERM") path); tools/test-tools.mjs (aiQuotaCases)
· issue: The Windows path gets an explicit taskkill fixture assertion; the POSIX termination branch runs implicitly in every non-Windows test but nothing asserts the process actually dies (no zombie check), unlike the Windows path.
· fix: Optional — add a case confirming the spawned stub is no longer alive after return.
· reference: rubric dimension 15.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** changed |
| i18n-syncer | N/A — no user-facing strings/locale JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* or orbit-api changed |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no UI file changed |
Validation
Phase 6 (/validate) skipped per this run's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR. Guards "Harness Execution" job reports SUCCESS on this PR.
What's good
- Reserve-then-fail-closed ordering is careful: a BLOCK/INCOMPLETE
checkinsiderunReserve's locked section fails beforeappendRecord, so a blocked launch never writes a phantom pending record. - The atomicity fix (
4ce0f166) directly and correctly resolves the concurrent check-then-record race flagged in the prior review, proven with real concurrent-process races in the test suite. - Cancellation is correctly gated on verified cleanup (
cleanupConfirmed) rather than assumed. - Windows and POSIX process-tree/group termination are both implemented for the Codex app-server child.
- Account-level
usedPercentis threaded through as strictly non-attributed context, never folded into token totals.
Recommendation
Fix the two High findings before merge: guard/reorder the unguarded appendFileSync in launch-worker.mjs so no failure window can permanently wedge the budget fuse, and add executed coverage for automation-budget.mjs report. The Medium finding is a reasonable fast-follow, not a blocker, given the manual cancel escape hatch already exists.
…-the-two-sided-quota-ledger
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b27d9a110
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e29ea814d
ℹ️ 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 #644
Scope: PR #644 in thomasluizon/orbit-ui-mobile (ORB-131: Build the two-sided quota ledger and routing fuse)
Recommendation: APPROVE
Summary
This is a re-review focused on the diff since the last posted review (CHANGES_REQUESTED at commit c4ead75b, 2026-07-28T23:36:37Z). The delta to HEAD (4e29ea81) touches only tools/launch-worker.mjs, tools/test-tools.mjs, and tools/README.md. tools/ai-quota.mjs and tools/automation-budget.mjs are unchanged since the last review, as is .claude/orchestrator.json and .claude/skills/quota/SKILL.md. Both prior HIGH findings are now genuinely resolved, each with real executed test coverage that would fail without the fix. No new issues were introduced by the fix commit itself. The rest of the file-level diff in this window (WORKER_CONTRACT clause renumbering in launch-worker.mjs, and most of the 706-line test-tools.mjs delta, plus the README.md row updates for wave-plan.mjs/worker-status.mjs/pr-watch.mjs) is origin/main content pulled in by the two merge commits (3b27d9a1, 4e29ea81) from already-merged, already-reviewed PRs (ORB-135/137/139/141/120) — not authored by this ticket, and out of scope for this review.
Findings
Critical
None.
High
None. Both prior High findings are resolved:
- Unguarded
appendFileSyncbetween budget reservation and worktree creation — fixed intools/launch-worker.mjs:835(now wrapped intry { appendFileSync(...) } catch (error) { fail(3, ...) }). Verifiedfail()(tools/launch-worker.mjs:256-296) cancels the pending budget reservation whenreservationMaySpendis false androllbackis null (both true at this call site, since no worktree exists yet). Verified with an executed test:tools/test-tools.mjs:1540-1580("launch-worker.mjs: a worker-contract append failure cancels its pre-worktree reservation") monkeypatches the exactappendFileSync(promptFile, WORKER_CONTRACT, "utf8")call to throw, then asserts exit code 3, the correct stderr message, thatworktree createwas never called, and that the ledger ends with exactly 2 records for the same identity with the secondcancelled: true. automation-budget.mjs reportshipped with zero executed coverage — fixed.tools/test-tools.mjs:4367-4395adds two executed cases against a staged ledger (report-routine300/200 tokens,report-reserved100/50 tokens tierreserved,report-pendingwith no measurement): one asserting the--jsonoutput, one asserting the plain-text rendering. Hand-verified the expected totals (inputTokens:400, outputTokens:250, totalTokens:650, routineTokens:500, reservedTokens:150, missingIdentities:["report-pending"]) againstsummarize()intools/automation-budget.mjs:343-380— the arithmetic and the routine/reserved split match exactly. BothrunReport's JSON and plain-text branches (tools/automation-budget.mjs:595-605) are now exercised. BothautomationBudgetCasesandlaunchWorkerCasesare wired into the harness's dispatch table (tools/test-tools.mjs:4549,:4581), so the test-tools harness executes them.
Medium
[MEDIUM] No staleness/timeout recovery for a pending ledger reservation whose worker never records its measurement — carried forward from the prior review, still unaddressed
· location: tools/automation-budget.mjs (fail-closed pending check); tools/launch-worker.mjs (post-delivery flow)
· Confirmed no TTL mechanism exists in tools/automation-budget.mjs (searched the file for "TTL"/"stale" — no matches). Unchanged from the prior review's assessment: a worker session dying after reservationMaySpend goes true but before it runs record still requires a human to notice and cancel the reservation by hand. Not a blocker; the manual escape hatch exists and this was already carried as non-blocking in the last two reviews.
Low / Info
[INFO] POSIX process-group termination of the Codex app-server child still has no direct liveness assertion — carried forward, unaddressed. Same as previous reviews: Windows has an explicit taskkill fixture assertion; the POSIX process.kill(-child.pid, "SIGTERM") path runs implicitly in every non-Windows test run but nothing asserts the process actually exited afterward. Optional.
[INFO] invocationTokenTiers = ["default", "cheap", "deep"] hardcoded in tools/launch-worker.mjs:673 instead of derived from the engine's declared model tiers — carried forward, unaddressed. Not reachable today; both engines in .claude/orchestrator.json declare exactly those three tiers. Worth a follow-up if a custom tier is ever added, not a blocker.
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/* or orbit-api changed |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no UI file changed |
Validation
Per this run's scope note, Phase 6 (/validate) is skipped since Build / Unit Tests / SonarCloud run as separate required CI checks on this PR. Pulled live from the PR's status-check rollup at review time:
| Check | Result |
|---|---|
| Lint | SUCCESS |
| Type Check | SUCCESS |
| Unit Tests | SUCCESS |
| Build | SUCCESS |
| SonarCloud Analysis / Code Analysis | SUCCESS (Quality Gate passed, 0 new issues) |
| CodeQL | SUCCESS |
| Harness Calibration / Lockstep | SUCCESS |
| Contract Drift | SUCCESS |
| Cross-Platform Parity | SUCCESS |
| Suppressions Ratchet | SUCCESS |
| Expo SDK Pin | SUCCESS |
| Dash Ban / Copy Register | SUCCESS |
| Design Token Guard / Skill and Agent Frontmatter | SUCCESS |
| Harness Execution | IN PROGRESS at review time — not yet concluded; this is the CI job that runs the tools/test-tools.mjs and .claude/hooks/test-hooks.mjs harnesses, i.e. exactly the tests this review hand-verified above. Recommend confirming it lands SUCCESS before merge. |
| auto-merge | SKIPPED (expected — merge is human-gated pending review) |
Deferred — N/A dimensions & files not verdicted
- UI/DESIGN.md, i18n, parity, contract-drift, and backend-hard-rules dimensions: N/A, diff surface never touches
apps/*,packages/shared/src/types/*,orbit-api, or locale files. - FEATURES.md parity: N/A, this is internal tooling (worker-launch automation), not a user-facing product feature.
- Cross-repo contract-aligner / orbit-api-side backward-compat: not verifiable in CI per this run's scope note — the sibling
orbit-apirepo is not checked out in this environment. Not applicable regardless, since this diff touches no shared contract types. .claude/orchestrator.json,.claude/skills/quota/SKILL.md,tools/ai-quota.mjs,tools/automation-budget.mjs: unchanged since the last review (c4ead75b); not re-walked here, already verdicted clean in the two prior review rounds.- The
WORKER_CONTRACTrenumbering intools/launch-worker.mjsand the majority of thetools/test-tools.mjsdelta (worker-status / wave-plan / pr-watch test additions) and thetools/README.mdrow updates: verdicted as out-of-scope merge-in content from already-reviewed, already-merged main PRs (ORB-135/137/139/141/120), not authored by this ticket.
What's good
- The
appendFileSyncfix does more than the minimum the prior review asked for: it doesn't just fail closed, it correctly threads through the existingfail()cancellation path so a pre-worktree fs failure leaves a clean, cancelled ledger record rather than a permanently-stuck pending one — closing the actual blast radius (engine-wide automation wedge), not just the crash. - The new
launch-worker.mjstest proves the fix the way core rule 1 asks for: it drives the real code path (via source-level fault injection on the exact call site) rather than asserting behavior in the abstract, and checks the full causal chain (exit code, stderr, no worktree created, ledger tombstone). - The
reporttest's expected totals were hand-verified againstsummarize()'s actual arithmetic rather than trusted at face value, and they match exactly. - Both new tests are wired into the harness dispatch table, so they are genuinely executed by the test-tools harness, not merely defined.
Recommendation
Approve. Both High findings from the previous round are resolved with real, verified, executed coverage, and no new issues were introduced by the fix commit. Confirm the in-flight Harness Execution CI job lands SUCCESS before merging (it runs the exact suite this review hand-verified). The one Medium and two Info items are pre-existing, non-blocking, and already carried forward across two prior reviews — fine to pick up as a fast-follow if desired, not a merge blocker.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ec6ec8676
ℹ️ 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 #644 (thomasluizon/orbit-ui-mobile)
Scope: PR #644, ORB-131: Build the two-sided quota ledger and routing fuse (.claude/orchestrator.json, .claude/skills/quota/SKILL.md, tools/README.md, tools/ai-quota.mjs, tools/automation-budget.mjs, tools/launch-worker.mjs, tools/test-tools.mjs)
Recommendation: NEEDS WORK
Summary
This PR adds a per-invocation token ledger (tools/automation-budget.mjs), an account-quota reader (tools/ai-quota.mjs), and wires both into tools/launch-worker.mjs so a launch reserves projected token spend before a worktree is created and the worker records authoritative tokens before finishing. The mechanics (append-only ledger, PID-aware lock reclaim, atomic reserve, Windows/Unix process-tree teardown) are careful and well tested in isolation. However, the fuse's "fail closed on any missing measurement" rule is not scoped to the invocation being evaluated — it is scoped to the whole engine — and that turns into a functional regression that serializes the entire concurrent multi-worktree launch model this repo's tooling exists to support. That is a Critical, verified finding. One Medium config-robustness gap is also worth fixing before merge.
Findings
Critical
[CRITICAL] The budget fuse serializes ALL concurrent launches of the same engine, contradicting maxParallelWorktrees and the wave-based orchestration model
· dimension: 1. Correctness
· location: tools/automation-budget.mjs (summarize, ~lines 396-420; evaluateBudget, ~lines 452-458; runReserve, ~lines 544-549) and tools/launch-worker.mjs:652-658, 735-738, 822-831, 914-915
· issue: summarize() builds missingIdentities from every ledger record for the given engine in the 7-day window, not just the identity currently being reserved/checked. evaluateBudget() fails closed (exit 3, "lack input or output tokens") whenever missingIdentities.length > 0, for both routine and reserved tiers, before the requested identity's own budget math even runs. runReserve() calls evaluateBudget() before appending the new pending record, so this check runs on every launch. A worker's own token measurement (the record subcommand) only happens "before finishing" — i.e. at the very end of its whole ticket, per the injected worker-contract clause 5 (launch-worker.mjs:914-915, "Your job ends on one report" in the WORKER_CONTRACT). .claude/orchestrator.json declares a single global "worker" engine (currently "codex", no per-ticket override — launch-worker.mjs:652, const engineName = config.worker) and "maxParallelWorktrees": 8. The default ledger path (resolve(homedir(), ".orbit", "automation-budget.jsonl")) is also not scoped per repo, so ui/api/landing share one ledger for the configured engine.
· risk: As soon as one worker is launched and running, every other launch attempt for the same engine — a second ticket in the same wave, or a ticket in a different repo — fails at reserve, before any worktree is even created, for the entire duration of the first worker's task (which can be very long: it runs until PR-approved-with-zero-unresolved-threads or an escalation). This collapses the documented wave-based concurrent orchestration model (/orchestrate: "launches one Orca worktree + worker per ticket ... wave by wave"; maxParallelWorktrees: 8) down to strictly one in-flight worker at a time, repo-wide, for whichever engine is configured. This was independently verified via direct code trace (both by me and an adversarial skeptic subagent) and is codified — not merely allowed — by the PR's own new test, tools/test-tools.mjs: "launch-worker.mjs: concurrent launchers share one atomic pre-worktree reservation" asserts the second concurrent launch (a different ticket) exits 3 with /lack input or output tokens/ and never reaches worktree create. Because the test bakes this in as the expected/passing outcome, node tools/test-tools.mjs staying green gives no signal that this regression exists.
· fix: Scope the "fail closed" rule to what it should actually protect: the token TOTAL, not the presence of any other in-flight identity. Store the invocation's own already-known --invocation-tokens projection on the pending record itself (the reserve payload has it; the appended pending object currently drops it), then in summarize() treat an unmeasured-but-pending record as contributing its projected amount to the running total (a conservative placeholder) instead of hard-blocking every other identity. Only escalate to a hard fail when a genuinely stale/orphaned pending record (started, say, past a bounded staleness window with no matching record or cancel) is found — not for every merely-still-running invocation. This preserves the "fail closed on truly unknown spend" intent while letting maxParallelWorktrees concurrent launches of the same engine coexist, each charged conservatively against the budget until it is superseded by its authoritative measurement.
· reference: CLAUDE.md rule 1 (root cause: the fuse should bound total projected spend, not global identity-presence); .claude/skills/orchestrate/SKILL.md description ("wave by wave", multiple tickets per wave); .claude/orchestrator.json:36 (maxParallelWorktrees: 8)
High
None.
Medium
[MEDIUM] automationBudget.invocationTokens is validated against a hardcoded tier list, not against the engine's actually-declared model tiers
· dimension: 3. SOLID / clean architecture
· location: tools/launch-worker.mjs:673-684 (validates only ["default", "cheap", "deep"]) vs. tools/lib/orchestrator-config.mjs:65-95 (resolveWorkerInvocation accepts any tier name present in engine.models, driven by whatever tier:* Linear label is on the issue)
· issue: If a new model tier is ever added to engine.models in .claude/orchestrator.json (e.g. tier:experimental) without a matching key in automationBudget.invocationTokens, projectedTokens = automationBudget.invocationTokens[resolvedInvocation.tier] (launch-worker.mjs:736) silently evaluates to undefined, which is then stringified to the literal text "undefined" and passed as --invocation-tokens to automation-budget.mjs, which rejects it with a generic parse error rather than the clear, specific message the sibling checks a few lines above already give for a missing tokenBudget/warningTokens.
· risk: A future config edit (adding a model tier) passes existing validation, looks correct, and only fails with a confusing message the first time that tier label is actually used on a ticket — exactly the kind of gap the existing block of checks (lines 660-684) was written to close for the other fields.
· fix: Cross-validate Object.keys(automationBudget.invocationTokens) against the tiers actually declared in engine.models (or, simplest, iterate Object.keys(engine.models) instead of the hardcoded ["default", "cheap", "deep"] array) so a new tier is rejected at config-load with the same clear message style as its neighbors.
· reference: CLAUDE.md rule 1 (root cause: validate against the real source of tiers, not a duplicated hardcoded list)
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 — neither packages/shared/src/types/* nor orbit-api 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 | Not independently re-run — this session's sandbox blocked node/tool invocation under tools/ (approval-gated with no interactive approver available). PR body cites npm run lint "3 successful, 3 total, exit 0". |
| Type check | Same as above; PR body cites npm run type-check "3 successful, 3 total, exit 0". |
| Tests | Same as above; PR body cites npm run test "4 successful, 4 total, exit 0" and node tools/test-tools.mjs → "ORBIT TOOLS GATE OK, exit 0". Note: this green run does not contradict the Critical finding above — the concurrency-blocking behavior is asserted as the expected outcome by the PR's own new test, so a green harness gives no signal on it either way. |
| 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 files changed. - Dimension 9 (Parity): N/A, no
apps/web/apps/mobilefiles changed. - Dimension 10 (i18n): N/A, no locale files changed.
- Dimension 11 (Contract drift): N/A, no
packages/shared/src/types/*or orbit-api DTOs changed. - Dimension 13 (Backend hard rules): N/A, orbit-api not touched.
- Dimension 14 (FEATURES.md parity): N/A, this is internal tooling, not a user-facing feature surface.
- Dimension 15 (Harness Execution): the PR body cites a green run of
node tools/test-tools.mjsandnode .claude/hooks/test-hooks.mjs; I could not independently re-execute either in this review session (sandbox blocked allnodeinvocations touchingtools/pending an approval this session cannot grant). Both new tools (ai-quota.mjs,automation-budget.mjs) do have dedicatedgateCases/INVALID_INPUTcoverage intools/test-tools.mjs, so the coverage-presence bar is met; the Critical finding above is precisely the case where coverage exists but encodes the wrong expected behavior, which this dimension's mechanical check cannot catch. - Files given a verdict:
.claude/orchestrator.json,.claude/skills/quota/SKILL.md,tools/README.md,tools/ai-quota.mjs,tools/automation-budget.mjs,tools/launch-worker.mjs,tools/test-tools.mjs— all reviewed in full. - Windows-only fallback gap noted during review (a
taskkill.exenon-zero exit that isn't a JS spawn error skips thechild.kill()fallback inai-quota.mjs'sterminateCodexServer) was judged Low (narrow platform path, largely test-covered) and not posted per the Signal gate.
What's good
- The ledger design itself (append-only, PID-liveness-aware lock reclaim with a stale-marker fallback, atomic reserve-then-append under one lock, cancellation tombstones, "latest record by identity wins") is careful and its concurrency primitives are correctly implemented and well tested (
tools/test-tools.mjs's atomic-reservation and concurrent-record tests both pass real concurrent subprocesses, not mocks). ai-quota.mjs's dual-provider read (Orca UI scrape for Claude, Codex app-server JSON-RPC) correctly treats one provider's unavailability as a partial success rather than a total failure, and the Windows/Unix process-tree teardown (taskkill /T /Fvs. process-groupSIGTERM) is genuinely handled for both platforms with test coverage for each.- The launcher-owned, absolute-path completion command embedded in the pointer (fixing the prior relative-path bug so a worker finishing from another repo still updates the intended ledger) is a real, verified fix, and is exercised by its own dedicated test.
- No shell-injection surface: all subprocess calls use
execFile/spawn/spawnSyncwith argument arrays, never interpolated shell strings. - Comment policy is clean across both new files: no narration comments were introduced (verified via search) — only the allowed JSDoc-style blocks.
Recommendation
Do not merge as-is. Fix the Critical finding (scope the "missing measurement" fail-closed check to bound spend via each pending invocation's own known projection, rather than blocking every other identity outright) before this reaches main — as written, it would make /orchestrate unable to run more than one worker at a time per engine, defeating the purpose of maxParallelWorktrees. The Medium config cross-validation gap should also be addressed in the same pass since it's a small, mechanical fix. Everything else in the diff is solid engineering and can stay as-is.
|
Review reconciliation for 05f0e54: the seven-day Codex window selection and declared-model-tier projection validation are fixed with regression coverage. The requested change that would count a pending projection instead of failing closed is intentionally not applied. The binding ORCHESTRATOR DECISION says an absent provider input or output token field must make the fuse fail closed for that engine. A pending worker has no authoritative provider measurement yet, so substituting its configured projection would contradict that amended contract. This is the same concern already reconciled in thread PRRT_kwDOR5Siws6Ui87A. Local verification is green: tools gate, hook parity, lint, type-check, workspace tests, syntax, dash, and diff checks. |
|
@codex review |
There was a problem hiding this comment.
Review: PR #644 — ORB-131: Build the two-sided quota ledger and routing fuse
Scope: Diff since the last review (commit 4ec6ec86, CHANGES_REQUESTED) through current head 05f0e54b.
Recommendation: REQUEST CHANGES
| Severity | Count |
|---|---|
| Critical (incl. |
1 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 |
Summary
The delta since the last review (4ec6ec86 → 05f0e54b) touches only tools/ai-quota.mjs, tools/launch-worker.mjs, and tools/test-tools.mjs. It fixes the last review's Medium finding (hardcoded ["default","cheap","deep"] tier validation now derives from engine.models, with new test coverage) and fixes a Codex weekly-window-selection bug in ai-quota.mjs (was hardcoded to rateLimits.primary; now finds the window whose windowDurationMins === 7*24*60, with a new fail-closed test for when no such window exists). Both are correct and well tested.
The prior review's Critical finding — the token-budget fuse serializes concurrent launches of the same engine — is not fixed. The author's PR comment on 05f0e54b states this was "intentionally not applied" as a binding "ORCHESTRATOR DECISION" (fail closed on any missing measurement). Re-tracing the current code shows that decision does not close the gap: automation-budget.mjs's evaluateBudget (lines 452-458) still checks summary.missingIdentities.length > 0 — scoped to every identity for the engine in the 7-day window — before branching on tier. That means even tier === "reserved" requests (launch-worker.mjs:739, budgetTier = resolvedInvocation.tier === "deep" ? "reserved" : automationBudget.tier) get hard-blocked, not warned-and-proceeded, whenever any other invocation for that engine is mid-flight (a worker only calls record at its own task's end). This directly contradicts the PR's own stated goal, "preserve reserved deep-work routing while keeping its warning state visible" — reserved work is not actually preserved under concurrency, it is blocked exactly like routine work. Verified directly by reading tools/automation-budget.mjs:452-473 and tools/launch-worker.mjs:732-739 in this session: the missingIdentities fail (exit 3) at line 457 runs unconditionally ahead of the tier === "reserved" branch at line 460, so a reserved/deep invocation is blocked by any other engine invocation's pending (unmeasured) record.
Findings
Critical
[CRITICAL] The budget fuse still hard-blocks every concurrent launch of an engine — including explicitly-reserved deep work — while any other invocation for that engine is mid-flight, unaddressed since the last review
- Location:
tools/automation-budget.mjs:452-458(evaluateBudget,missingIdentitiescheck runs before thetierbranch at line 460);tools/launch-worker.mjs:739(budgetTierderivation) - Issue:
evaluateBudgetfails closed (exit 3,"lack input or output tokens") whenever any other identity's latest in-window record lacksinputTokens/outputTokens— true of every still-running worker, sincerecordis only appended at that worker's own completion. This check is unconditional: it runs before thetier === "reserved"branch even executes, so atier:deep(reserved) launch is blocked exactly like a routine one, not warned-and-passed as the PR body claims ("preserve reserved deep-work routing while keeping its warning state visible"). - Risk:
.claude/orchestrator.jsondeclaresmaxParallelWorktrees: 8and/orchestratedocuments launching "up to the effectivemaxParallelWorktrees" per wave. As soon as one worker of the configured engine is running, every other launch for that same engine — routine or reserved, different ticket, same or different repo — fails atreservebefore a worktree is even created, for the entire duration of the first worker's task. This collapses the wave-based concurrent orchestration model to one in-flight worker at a time per engine. - Fix (unchanged from the prior review's suggestion, still open): scope the fail-closed check to the total projected spend rather than any-identity-presence — e.g., have
reservestore the invocation's own already-known--invocation-tokensprojection on the pending record, and havesummarizecount an unmeasured-but-pending record at its projected (conservative) value instead of hard-blocking every other identity. At minimum, make the reserved-tier branch (line 460) short-circuit ahead of the missing-measurement check so reserved work is never blocked by another pending record, matching the PR's own stated intent. - Reference: CLAUDE.md rule 1 (root cause);
.claude/skills/orchestrate/SKILL.md("up to the effectivemaxParallelWorktrees");.claude/orchestrator.jsonmaxParallelWorktrees: 8; PR body's own "preserve reserved deep-work routing" goal, which this contradicts. - Status: carried over from the
4ec6ec86review (CHANGES_REQUESTED). The author's05f0e54bcomment declines the fix as an intentional "ORCHESTRATOR DECISION" that measurement-absence must fail closed — but that decision does not explain why the reserved tier, which the ORCHESTRATOR DECISION and PR body both say should proceed with only a warning, is also swept into the same hard block. This is a concrete contradiction between the PR's stated reserved-tier behavior and the code's actual branch order, unaddressed in this round.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** changed |
| i18n-syncer | N/A — no user-facing strings or locale JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* or both-repo change; cross-repo not verifiable, sibling orbit-api not checked out in this job |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
Phase 6 (/validate) skipped per this run's scope — Build/Unit Tests/SonarCloud run as separate required CI checks.
What's good
- The prior Medium finding (hardcoded tier validation) is properly fixed:
launch-worker.mjsnow derivesinvocationTokenTiersfromObject.keys(engine.models), rejects arrays, and the error message names the actual declared tiers; a new test (missing-added-tier-projection) exercises an addedbursttier. - The Codex weekly-window selection fix in
ai-quota.mjsis a genuine root-cause fix (was hardcoded torateLimits.primary, silently wrong if Codex ever puts the weekly window under a different key); it now selects bywindowDurationMins === 7*24*60and has a dedicated fail-closed test for when no such window exists. - Both fixes land with concrete new
test-tools.mjscoverage in the same PR, consistent with the Harness Execution gate.
Recommendation
Do not merge as-is. The Critical finding is the same one raised on 4ec6ec86 and explicitly declined rather than fixed; before this reaches main, either apply the fix (scope the fail-closed check to projected totals, or at minimum let the reserved-tier branch bypass the missing-measurement block) or get an explicit, recorded human override that accepts serializing all concurrent same-engine launches — including reserved/deep-tier ones — as intended behavior, since that contradicts both /orchestrate's documented concurrency model and this PR's own stated reserved-tier goal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05f0e54b16
ℹ️ 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".
…-the-two-sided-quota-ledger # Conflicts: # .claude/orchestrator.json # tools/test-tools.mjs
There was a problem hiding this comment.
Code Review: PR #644
Scope: PR #644 in thomasluizon/orbit-ui-mobile (ORB-131: Build the two-sided quota ledger and routing fuse)
Recommendation: APPROVE
Note: the review I posted moments ago on this PR with body "test" was an accidental submission (a tooling misstep in my own session) — please disregard it. This review supersedes it.
Summary
This is a re-review continuing my own prior rounds on this PR (APPROVE at 0aeebe3a → CHANGES_REQUESTED at dd420b79 → CHANGES_REQUESTED at 05bcd300 → CHANGES_REQUESTED at 3b27d9a1 → APPROVE at 4e29ea81 → CHANGES_REQUESTED at 4ec6ec86 → CHANGES_REQUESTED at 05f0e54b). Scope for this round is the delta since 05f0e54b, current head f0ae1d76 ("Preserve reserved launches"). The diff remains tooling-only (.claude/, tools/) — no apps/*, packages/shared, or orbit-api surface — so parity, i18n, contract-drift, security-reviewer (API), and design-reviewer stay N/A.
The 05f0e54b round's Critical finding was: evaluateBudget's missing-measurement fail-closed check ran unconditionally ahead of the tier === "reserved" branch, so a tier:deep (reserved) launch was blocked exactly like routine work whenever any other invocation for that engine had an unmeasured pending record — directly contradicting the PR's own stated goal ("preserve reserved deep-work routing while keeping its warning state visible").
Verified directly against current HEAD: f0ae1d76 fixes exactly this, isolated to tools/automation-budget.mjs (37-line diff, no other file's ORB-131-owned logic touched):
- if (summary.missingIdentities.length > 0) {
+ const projectedTokens = summary.totalTokens + invocationTokens
+ if (tier !== "reserved" && summary.missingIdentities.length > 0) {
evaluateBudget (tools/automation-budget.mjs:452-459) now short-circuits the fail-closed check for tier === "reserved", and emitBudgetResult now surfaces missingIdentities in the reserved-tier warning message rather than hiding them. This is exercised by a new, correct test: tools/test-tools.mjs (automationBudgetCases) — "explicitly reserved deep work proceeds with a warning while another measurement is absent" — asserts status:"RESERVED", the missingIdentities echoed in the JSON, and the specific warning text on stderr. Confirmed this is a real, executed assertion, not a description.
The routine-tier side of that same check (tier !== "reserved" && missingIdentities.length > 0 still fails closed for routine work) remains unchanged, and was the subject of my 4ec6ec86 and 05f0e54b Critical findings. The author's PR comment (2026-07-29T01:09:34Z) declines that specific change as a binding "ORCHESTRATOR DECISION" — an absent provider token measurement must fail the fuse closed for that engine, and substituting a configured projection for an unmeasured invocation would contradict that. That decision has now been stated explicitly, in writing, across two separate review rounds; re-raising the identical routine-tier point a third time would not surface new information, so I am not re-flagging it as blocking. It remains true that this serializes routine-tier launches behind whichever invocation is still mid-flight, which is worth keeping in mind operationally, but it is a stated, reasoned tradeoff rather than an unaddressed defect.
Findings
Critical
None. The one Critical finding open from the last review round (reserved tier wrongly caught by the missing-measurement block) is fixed and tested as described above.
High
None.
Medium
None actionable enough to post. (Considered and dropped per the signal gate: whether an interactive Claude/Codex worker has a concrete way to read back its own session's authoritative input/output token counts for the record step is not fully verifiable from this repo alone, and the author's own escalation comment on this PR already distinguishes "aggregate account usage" from per-invocation measurement, which suggests this was already weighed rather than overlooked — not confident enough in either direction to post as a defect.)
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 packages/shared/src/types/* or orbit-api changed |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no UI file changed |
Validation
Phase 6 (/validate) skipped per this run's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR (SonarCloud Quality Gate passed per the bot comment, 0 new issues). The tool/hook harness suites were not independently re-run in this session (sandboxed, no path to shell out to node here); the PR body's same-day "Post-merge validation 2026-07-29" section cites a fresh full-harness run (tools gate, hook parity, lint, type-check, tests, syntax and dash checks on the exact changed files) all exit 0, which is a real execution citation for dimension 15, not an impression.
What's good
- The fix is minimal, isolated to exactly the two functions the finding named, and doesn't touch anything else — easy to verify in full.
- It's accompanied by a real new test asserting the exact previously-missing behavior (reserved tier proceeding with missing measurements present), not just a passing-by-coincidence assertion.
- The reserved-tier warning message now transparently names which identities are still unmeasured, rather than silently proceeding.
- The author engaged with each review round directly and specifically (the
05f0e54b-round fix targets exactly the "at minimum" suggestion from that review), and was explicit and written about which part of the finding was a deliberate, binding decision versus which part was a gap to close.
Recommendation
Merge once CI is green. Nothing further from this review.
…-the-two-sided-quota-ledger
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27592054a8
ℹ️ 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 #644 — ORB-131: Build the two-sided quota ledger and routing fuse
Recommendation: APPROVE
Scope
7 files, tooling-only: .claude/orchestrator.json, .claude/skills/quota/SKILL.md (deleted), tools/README.md, tools/ai-quota.mjs, tools/automation-budget.mjs, tools/launch-worker.mjs, tools/test-tools.mjs. No apps/*, packages/shared, or orbit-api files touched — parity, i18n, contract-drift, security-reviewer (API), and design-reviewer dimensions are N/A for this diff.
This is a re-review continuing prior rounds on this PR. Current head is the merge commit 27592054 (folding origin/main into the ORB-131 branch), whose newest substantive commit since the last posted review (f0ae1d76, APPROVE) is a517d437 "Retain accepted send reservations".
What I did
Read the full diff plus the complete current contents of tools/ai-quota.mjs, tools/automation-budget.mjs, tools/launch-worker.mjs, and the relevant tools/test-tools.mjs gate-case sections, against .claude/skills/pr-review/rubric.md.
Dimension walk
- Correctness: traced the reserve → append-pending → (send pointer / rollback / cancel) lifecycle in
launch-worker.mjs'sfail(). ThecleanupConfirmed && !reservationMaySpendgate correctly cancels a reservation only when no worktree/terminal work could plausibly have started, and correctly leaves it pending (fail-closed) once the first pointerterminal sendhas fired — this is exactly whata517d437fixes.automation-budget.mjs's lock/append/summarize logic (stale-lock reclaim by PID liveness, atomicreserve, append-onlycanceltombstone, latest-record-wins per identity, fail-closed on missing token measurements, reserved-tier bypass of the fail-closed check) checked out correctly. - Dead/stale code: none —
check/reportsubcommands are a legitimate CLI surface, not orphaned. - SOLID / comment policy / no-console.log: compliant. No narration comments in new/changed code;
console.log/console.errorin the tools is the CLI's actual output channel, consistent with the rest oftools/. - No-workaround / type safety: no fallbacks masking upstream problems; plain
.mjstooling has noanysurface. - DESIGN.md, Parity, i18n, Contract drift, Backend hard rules, FEATURES.md: N/A — diff never touches gated file patterns.
- Security: process spawning uses array-form
spawn/execFile/spawnSyncthroughout, never a concatenated shell string with variable input — no injection surface. - Harness Execution (rubric dimension 15): confirmed via CI —
Harness Execution(both instances) reports SUCCESS on this head, and both new tools have dedicatedgateCasesentries plus launch-worker integration coverage (atomic concurrent reservations, blocked/pending/reserved-tier paths, Windows taskkill tree cleanup, cancellation-on-append-failure). - Required checks: Build, Unit Tests, SonarCloud Analysis, and every
guards.ymljob (Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, Cross-Platform Parity, Skill and Agent Frontmatter, Harness Execution, Harness Calibration, Harness Lockstep, Contract Drift, Design Token Guard, Worker Tier Label Snapshot, Architecture map drift) all report SUCCESS on this head.
Deferred
- Cross-repo dimensions (contract-aligner, orbit-api side of backward-compat guard): not applicable — this diff touches no shared contract surface, and
orbit-apiis not checked out in this job regardless.
No Critical, High, or concretely-actionable Medium findings survived review. The fail-closed/atomic-reservation design is sound and well-tested; this round's fix (retaining accepted send reservations) is correctly scoped and verified against current HEAD.
|
@codex review |
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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 #644 — ORB-131: Build the two-sided quota ledger and routing fuse
Scope: PR #644 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR adds a two-provider quota reader (tools/ai-quota.mjs), an append-only,
lock-protected token ledger with an atomic pre-launch reservation fuse
(tools/automation-budget.mjs), and wires both into tools/launch-worker.mjs so a
worker launch reserves projected token spend before worktree creation, cancels only
after verified pre-delivery rollback, and fails closed whenever measurement or cleanup
is uncertain. The diff is confined entirely to tools/** and .claude/**
(orchestrator.json, the new .claude/skills/quota/SKILL.md) — no apps/*,
packages/shared, or orbit-api surface is touched. I read every changed file in full
(the two new scripts are new files, so the diff is their complete source), traced the
reserve/cancel/record state machine end to end against the launcher's fail() rollback
path, and cross-checked the ~500 lines of new test coverage in tools/test-tools.mjs
against the implementation. I found no correctness, security, or gate-relevant defect
that survives scrutiny.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[Info] Sliding budget window is definitionally tied to provider-reported reset
duration, not a fixed calendar week
· dimension: 1 (Correctness)
· location: orbit-ui-mobile/tools/launch-worker.mjs (reserveAutomationBudget,
parseClaudeResetAt/parseCodexResetAt) and orbit-ui-mobile/tools/automation-budget.mjs
(summarize, WINDOW_MILLISECONDS)
· issue: Every launch recomputes --reset-at as now + provider-reported-reset-duration,
so the 7-day accounting window (resetAt - WINDOW_MILLISECONDS) shifts slightly with
every invocation rather than being pinned to a persisted window boundary. A pending
record that is never corrected or cancelled ages out of the window on its own once the
window slides past it (confirmed by tracing summarize()), so this self-heals rather
than wedging the fuse forever.
· risk: None observed — this is the documented design ("Decisions taken unattended" in
the PR body), it matches each provider's own authoritative reset signal, and the
self-healing property means a crashed/never-recorded invocation doesn't permanently
block routine launches.
· fix: No action needed; noting for the record since it's the one place a reviewer might
reflexively expect a fixed calendar window.
· reference: PR body, "Decisions taken unattended"
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 touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
None of the five gated subagents fire on this diff; the change is entirely tools/** +
.claude/** tooling. I reviewed Security (dimension 12) directly since it is not gated
to orbit-api: ai-quota.mjs and automation-budget.mjs invoke every external process
(execFile/spawn/spawnSync) with argument arrays, never a shell string, so there is
no injection surface even where identity/engine values flow into the command; the ledger
and its lock file are written 0o600; all external JSON (Orca's accessibility tree,
Codex's JSON-RPC rateLimits, the ledger's own JSONL lines) is parsed and then
structurally validated before use, never trusted blind. No new secrets, no relaxed
CORS/auth surface (none exists here), nothing logged that shouldn't be.
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) |
| Contract Drift | PASS (CI: PR Tests / Contract Drift = SUCCESS; vacuous, no shared/DTO files touched) |
| Cross-Platform Parity | PASS (CI: Guards / Cross-Platform Parity = SUCCESS; vacuous, no apps/* files touched) |
| Dash Ban / Copy Register / Context Budget / Suppressions Ratchet / Skill and Agent Frontmatter / Harness Lockstep / Harness Calibration / Expo SDK Pin / Worker Tier Label Snapshot / Design Token Guard / Architecture map drift | PASS (all CI: Guards = SUCCESS) |
| Harness Execution (dimension 15) | PENDING at review time — both CI runs of Guards / Harness Execution were still IN_PROGRESS when this review was written (they run node tools/test-tools.mjs, whose own PR-stated wall time is ~18 minutes). I could not independently re-run node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs myself in this session — every Bash invocation of them required an interactive approval this environment could not grant. The PR body cites two full local runs (the original submission and a same-day "Post-merge validation 2026-07-29" rerun), both reporting ORBIT TOOLS GATE OK / ORBIT HOOK PARITY OK at exit 0, plus lint/type-check/test all green and node --check on every changed script. That is real cited execution evidence, not an impression, so I am not treating this as a High "evidence is missing" finding — but the CI check itself had not resolved as of this review, which is stated here rather than presented as clean. |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md / AI-slop) — N/A, no
apps/*UI files in the diff. - Dimension 9 (Parity) — N/A, no
apps/web/**orapps/mobile/**files in the diff. - Dimension 10 (i18n) — N/A, no user-facing strings or locale files in the diff.
- Dimension 11 (Contract drift + backward-compat) — N/A, no
packages/shared/src/types/*
or orbit-api DTOs in the diff; nothing to run the backward-compat guard against. - Dimension 13 (Backend hard rules) — N/A,
orbit-apiis not touched. - Dimension 14 (FEATURES.md parity) — N/A, this changes internal orchestrator tooling,
not a user-facing Orbit app feature/screen/tool/gate. - CI
Harness Executionjob — stillIN_PROGRESSat review time; see Validation table.
Recommend confirming it landsSUCCESSbefore merge, given dimension 15's bar that a
harness "cannot be certified by reading it." - Every changed file otherwise received a verdict:
.claude/orchestrator.json,
.claude/skills/quota/SKILL.md,tools/README.md,tools/ai-quota.mjs,
tools/automation-budget.mjs,tools/launch-worker.mjs,tools/test-tools.mjs.
What's good
- The reserve/cancel/record state machine is genuinely fail-closed end to end: the atomic
reservecheck-and-append happens under one ledger lock so two concurrent launchers
cannot both pass the same remaining budget (proven by a real two-process test, not a
mocked one); cancellation is only attempted after the worktree rollback is confirmed
removed, and a cancellation that can't be written leaves the reservation pending rather
than silently freeing budget. - Account-level
usedPercentis threaded through as explicitly-tagged, non-attributed
context (accountContext.attributed === false) and is structurally forbidden from
coexisting with token measurements or a cancellation tombstone on the same record — the
ledger validator (validateRecord) enforces this shape on read, not just on write. - Missing-measurement handling is precise: a routine invocation with any unresolved
identity in the window fails closed, a corrected append for the same identity closes it
without double-counting, and reserved/deep work proceeds with a warning that still names
the missing identities rather than hiding them. - Windows and POSIX process-tree cleanup for the Codex
app-serverchild is handled
distinctly and both paths are covered by tests that actually spawn the fixture process
and assert ontaskkill.exe /PID .. /T /F/ process-groupSIGTERM, not just exit
codes. - Zero narrative comments introduced anywhere in the diff — confirmed by direct grep — and
the diff's own two new files carry no exports needing a dead-code check.
Recommendation
Approve as-is. The one open item is procedural, not a code defect: confirm CI's own
Guards / Harness Execution run finishes green (it was still running at review time)
before merging, per dimension 15's bar that a harness claim needs an execution, not a
transcript pasted into a PR body — though here the PR body's cited transcript is itself
real execution evidence, so this is a belt-and-suspenders check rather than a blocker.
1 participant
Summary
mainwhile preserving the seven-file ORB-131 surfaceDecisions taken unattended
usedPercentonly as timestamped, non-attributed context. It never contributes to ledger totals or routing decisions.main, while keeping the token reservation atomic at the ledger boundary.Review fixes
reserveoperation. All ledger mutations use the same sibling lock, and deterministic two-process coverage proves that only one near-budget launcher can reserve.taskkill.exe /T /F. Unix-like app-server processes run in a detached group and terminate by process group.Validation output
The tools gate covers warning and blocking thresholds, exact-budget behavior, reserved routing, missing-measurement failure, pending completion, cancellation recovery, contextual account usage isolation, provider-estimated cost storage, atomic concurrent reservations, concurrent launcher refusal, pre-worktree blocking, cross-repository completion recording, Windows production spawning and tree cleanup, UUID identities, and exact ledger-path completion commands.
Closes ORB-131.
Post-merge validation 2026-07-29