ORB-136: report structured worker status - #652
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No code change required: Vercel reports an intentionally skipped deployment and no review finding. Evidence: 009f02a. Review item: IC_kwDOR5Siws8AAAABMLTM3g |
|
No code change required: SonarQube reports a passing quality gate with zero new issues. Evidence: 009f02a. Review item: IC_kwDOR5Siws8AAAABMLT9Zw |
![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: 009f02a40f
ℹ️ 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".
|
No code change required: Vercel reports an intentionally skipped deployment and no review finding. Evidence: eaebb84. Review item: IC_kwDOR5Siws8AAAABMLTM3g |
|
No code change required: SonarQube reports a passing quality gate with zero new issues. Evidence: eaebb84. Review item: IC_kwDOR5Siws8AAAABMLXgDQ |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #652 — ORB-136: report structured worker status
Scope: PR #652 in thomasluizon/orbit-ui-mobile (base main, head feature/orb-136-have-workers-report-status-instead-of)
Recommendation: NEEDS WORK
Summary
The PR adds a Claude+Codex Stop hook (.claude/hooks/report-worker-turn.mjs) that appends a structured JSONL record per worker turn to a shared reports.jsonl, wires tools/launch-worker.mjs to provision that file and install the worktree-local hooks, extends tools/worker-status.mjs with hard git-state checks (staged-but-uncommitted, merge-in-progress, unpushed commits) plus a report-freshness liveness check, and rewrites the orchestrate skill's babysitting procedure around tailing that shared file. The diff is entirely tooling/orchestration (.claude/hooks/**, tools/**, .claude/skills/orchestrate/SKILL.md) — no apps/*, orbit-api, packages/shared, or i18n surface is touched, so the five review subagents and the backward-compat guard are all N/A. The new hard-git-state checks and the concurrency-safe append logic are solid and well tested (8-concurrent-writer race test, staged/merge/unpushed fixtures). Two High findings survive adversarial verification: the babysitting procedure this PR writes has no trigger that would ever surface the "suspect" verdict for the exact failure mode (a worker that goes fully silent) the feature exists to catch, and the Codex-side hook wiring (.codex/hooks.json shape, the last_assistant_message/cwd payload fields, and --dangerously-bypass-hook-trust) is asserted by direct analogy to Claude Code's hook schema with no citation and no execution against a real codex process.
Findings
Critical
None.
High
[HIGH] The babysitting procedure has no trigger that ever surfaces a fully-silent worker's "suspect" verdict
· dimension: 1 (Correctness) / process correctness in a skill doc
· location: .claude/skills/orchestrate/SKILL.md (section "## 3. Babysit")
· issue: The only described invocation of worker-status.mjs --reports-file is "On each new record, run the artifact check for that ticket." The paragraph right after it, "A missing report is the diagnostic trigger... worker-status.mjs marks a worker suspect when it has not reported within the expected window," implies staleness detection is reachable in practice, but no periodic/timeout re-check is described anywhere in the file (confirmed by an independent re-read of the whole "Babysit" section and the unrelated "Waiting is foreground work" polling paragraph, which governs a different actor — a worker polling CI/review, not the orchestrator tailing reports.jsonl).
· risk: The PR's own stated root cause is "Worker supervision treated terminal state as status... allowed silent workers... to look complete." A worker that crashes, hangs, or whose Stop hook itself fails to fire produces, by definition, no new record — so per the literal text there is no trigger that would ever cause the orchestrator to invoke worker-status.mjs --reports-file for that ticket again and see the "suspect" verdict. The exact failure mode this feature was built to catch is unreachable through the documented procedure, silently reverting to the old behavior (nobody checks until a human happens to notice).
· fix: Add an explicit periodic re-check to the babysitting loop, independent of new records — e.g. "every time the loop wakes (poll every N seconds/minutes, per the standing foreground-waiting rule), also run worker-status.mjs --reports-file for every ticket that has NOT produced a record within --expected-window-minutes," not only on new-record arrival.
· reference: CLAUDE.md rule 1 (root cause) / the PR body's own stated root cause; verified by an independent adversarial re-read of .claude/skills/orchestrate/SKILL.md finding no periodic mechanism.
[HIGH] Codex-side hook wiring is unverified against Codex CLI's real behavior
· dimension: 15 (Harness changes need EXECUTED evidence) / 1 (Correctness)
· location: tools/launch-worker.mjs:684-731 (the .codex/hooks.json installation and the codexReportCommand construction) and tools/launch-worker.mjs:609-611 (--dangerously-bypass-hook-trust)
· issue: The Codex Stop-hook config is written in the exact same shape as Claude Code's .claude/settings.local.json hooks ({"hooks":{"Stop":[{"hooks":[{"type":"command",...}]}]}}), and it points at the SAME report-worker-turn.mjs script, which unconditionally reads hookInput.last_assistant_message and hookInput.cwd from stdin with no engine branch. --dangerously-bypass-hook-trust is appended to every codex launch with no comment or citation. None of these three claims (the hooks.json shape/event name, the payload field names, the flag's existence) is demonstrated by a test that runs the real codex binary — every test-tools.mjs case stubs the terminal/orca layer, so command is only ever asserted as a string. Notably, .claude/orchestrator.json's codex entry is an exceptionally detailed, dated log of codex's real measured quirks (exact trust-prompt keystroke, sandbox flags, -p meaning --profile not --print) — and it never once mentions hooks, corroborating that this integration was never actually run against real codex.
· risk: If Codex CLI's real hook system uses a different config file, event name, payload field, or doesn't support this flag, every Codex-engine worker's structured report silently degrades forever to the "missing or malformed WORKER_REPORT marker" fallback (needsHuman: true), defeating this PR's purpose for that half of the fleet, or the codex launch command could fail outright on an unrecognized flag.
· fix: Verify against an installed codex binary (codex --help, or a real launch) before merge, and once confirmed, record the measured behavior in .claude/orchestrator.json's codex entry the same way every other codex quirk in that file is documented, with a date and citation. If the hook mechanism doesn't exist for Codex, gate the hook installation to Claude only until it does.
· reference: CLAUDE.md rule 1 (root cause / no unverified assumptions) and rubric dimension 15 ("Verified" without an execution is itself a finding).
Medium
[MEDIUM] report-worker-turn.mjs's own argument-validation failure paths are untested
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: .claude/hooks/report-worker-turn.mjs:30-31 (the --reports-file-must-be-absolute and --ticket-must-match-ORB-\d+ checks) and lines 98-106 (the fail() on an unwritable reports file)
· issue: .claude/hooks/test-hooks.mjs's new suite covers the complete-turn, missing-marker fallback, and 8-way-concurrent-write paths, but never exercises the hook's own fail() branches for a missing/relative --reports-file, a malformed --ticket, or a reports file that cannot be opened for append.
· risk: A regression in argument validation (e.g. the absolute-path check silently dropped) would not be caught by the harness gate that exists precisely to certify this file by execution, not by reading it.
· fix: Add three spawnSync cases to the new suite: relative --reports-file exits 1 with the expected stderr, malformed --ticket exits 1, and an unwritable path (e.g. inside a nonexistent, uncreatable directory) exits 1 via the could not append branch.
· reference: rubric dimension 15, "A new decision path added to a tool that already has coverage... needs its own case."
[MEDIUM] worker-status.mjs's new unknown-flag rejection is untested
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/worker-status.mjs:69-71 (KNOWN_FLAGS / unknown option(s))
· issue: The new KNOWN_FLAGS allowlist and its rejection of any unrecognized --prefixed token is a new decision path added in this PR, but tools/test-tools.mjs's workerStatusCases() (also new in this PR) never asserts it — only the three existing required-argument checks are re-verified.
· fix: Add a check(...) case passing an unrecognized flag (e.g. --bogus) and asserting exit 2 with /unknown option\(s\)/.
· reference: rubric dimension 15, same bullet as above.
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/* or orbit-api DTO 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, latest commit eaebb844) |
| Type check | PASS (CI, latest commit eaebb844) |
| Tests (Unit Tests) | PASS (CI, latest commit eaebb844) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS on the code-bearing commit 009f02a4 (all six changed files); IN PROGRESS on the latest commit at review time, which touches only .claude/skills/orchestrate/SKILL.md prose (no tools/** or .claude/hooks/** code) |
| Build | PASS (CI, latest commit eaebb844) |
Local execution of node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs was not independently reproduced in this review session: the sandbox's git-write operations (worktree add, clone, checkout) require an interactive approval this session cannot grant itself, so CI's own job runs are cited above instead of a locally-run trace.
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift), 12 (Security, frontend categories), 13 (Backend hard rules), 14 (FEATURES.md parity): all N/A, the diff touches no
apps/*,orbit-api,packages/shared, or i18n surface. - Every one of the 6 changed files (
.claude/hooks/report-worker-turn.mjs,.claude/hooks/test-hooks.mjs,.claude/skills/orchestrate/SKILL.md,tools/launch-worker.mjs,tools/test-tools.mjs,tools/worker-status.mjs) received a verdict above; nothing was left unexamined. - The PR branch received a live push (
eaebb844, "fix: start report monitor before workers") during this review session, a.claude/skills/orchestrate/SKILL.md-only diff fixing a separate race (the monitor's read-offset being captured before vs. after the first worker launch). That fix does not touch the trigger gap described in the first High finding above; both were re-checked against the latest diff before this report was written. - Local harness re-execution: deferred to CI (see Validation table) for the sandbox reason stated there.
What's good
- The 8-concurrent-writer test for
report-worker-turn.mjs(worker-report: eight concurrent hooks all exit successfully/...lose no records/...every concurrent line is well formed) is exactly the kind of race-condition proof dimension 15 exists to demand, and it is a real fixture (real git init/commit/push), not a mock. - The new
worker-status.mjshard-git-state checks (staged-uncommitted,merge-in-progress,unpushed-commits) each ship with a dedicated real-git fixture (stageWorkerStatus) proving the exact scenario measured in the PR body, not just a unit-level assertion. - Rolling the worktree-local
.claude/hooks/report-worker-turn.mjs,.claude/settings.local.json, and.codex/hooks.jsonintoinfo/excluderather than the tracked.gitignoreis the right call — these are per-launch generated artifacts, and the worker contract's "stage explicitly" clause (clause 8) benefits from a cleangit status. - The comment policy is respected throughout the new code (no narration comments; the one block comment in
report-worker-turn.mjs's surrounding file is pre-existing JSDoc-style WHY prose, not new).
Recommendation
Fix the two High findings before merge: (1) close the trigger gap so a fully-silent worker actually gets checked by the babysitting procedure this PR writes, and (2) verify the Codex hook wiring against a real codex binary (or gate hook installation to Claude only until verified) and record the measured result in .claude/orchestrator.json the way every other codex quirk in that file already is. The two Medium test-coverage gaps are worth closing in the same PR since they're each a few lines, but are not blocking.
There was a problem hiding this comment.
Code Review: PR #652
Scope: PR #652 in thomasluizon/orbit-ui-mobile (base main, head feature/orb-136-have-workers-report-status-instead-of @ eaebb844)
Recommendation: REQUEST CHANGES
Summary
The diff is entirely orchestration tooling (.claude/hooks/**, tools/**, .claude/skills/orchestrate/SKILL.md) — no apps/*, orbit-api, packages/shared, or i18n surface changed, so all five review subagents and the backward-compat guard are N/A. This is a re-review of a PR that already carries an open CHANGES_REQUESTED review from a prior automated pass; I independently re-verified its findings against git show HEAD: content and ran an additional adversarial check on each before posting.
Correction from the prior pass: the prior review's finding that report-worker-turn.mjs was wrong to trust hookInput.last_assistant_message without a transcript-parsing fallback does not survive verification. Official Claude Code hooks documentation confirms last_assistant_message is the documented, intended field for Stop hooks, and explicitly recommends against reading transcript_path as a fallback because the transcript file is written asynchronously and may lag the in-memory conversation. The sibling hook's (forbid-raw-repo-tool-surfacing.mjs) transcript fallback is defensive belt-and-suspenders coding, not evidence the field is unreliable. That finding is dropped from this review.
Two other High findings from the prior pass were independently re-verified and still stand.
Findings
Critical
None.
High
[HIGH] (re-confirmed) The babysitting procedure has no trigger for a fully-silent worker's "suspect" verdict
· dimension: 1 (Correctness)
· location: .claude/skills/orchestrate/SKILL.md §"## 3. Babysit"
· issue: The described loop is "Use the monitor... surface each complete line appended after its captured offset... On each new record, run the artifact check for that ticket." The very next paragraph, "A missing report is the diagnostic trigger... worker-status.mjs marks a worker suspect when it has not reported within the expected window," presupposes the artifact check gets run for that ticket — but the only documented trigger for running it is a new monitor line. worker-status.mjs does implement the time-based check (--expected-window-minutes, default 30, verified at tools/worker-status.mjs:111,185,402), but nothing in the babysit procedure calls it independent of new-record arrival. A worker that goes fully silent (crash, hang, Stop hook fails to install or fire) produces zero records by definition, so the monitor never fires for it and the procedure as written never re-examines it.
· risk: This is the PR's own stated root cause reappearing through the new mechanism instead of the old one — a silent worker looks unremarkable because nothing ever polls for its absence.
· fix: Add an explicit periodic re-check to the babysit loop, independent of new-record arrival — e.g., on each wake of the standing poll cadence (see "Waiting is foreground work"), also run worker-status.mjs --reports-file for every launched ticket with no record inside --expected-window-minutes, not only tickets that just produced one.
· reference: CLAUDE.md rule 1; PR body's own stated root cause.
[HIGH] (re-confirmed) Codex-side hook wiring is unverified against the real codex CLI
· dimension: 15 (Harness changes need EXECUTED evidence) / 1 (Correctness)
· location: tools/launch-worker.mjs:684-731 (the .codex/hooks.json shape and codexReportCommand), :609-611 (--dangerously-bypass-hook-trust)
· issue: The .codex/hooks.json shape, event name, and payload fields are asserted by direct analogy to Claude Code's own hook schema, with no citation and no execution against a real codex binary — every test-tools.mjs case stubs the terminal/orca layer, so command is only ever asserted as a string. I independently confirmed .claude/orchestrator.json's codex entry contains zero mentions of "hook" anywhere (grep -i hook returns nothing), despite being this repo's own established practice of logging every measured codex CLI quirk in detail (exact trust-prompt keystroke, sandbox flags, -p semantics) down to the CLI version tested. That corroborates this specific integration was never actually run against real codex, breaking the file's own standing convention of measuring before relying.
· risk: If Codex's real hook mechanism uses a different config path, event name, or payload shape, or the flag doesn't exist, every Codex-engine worker's structured report silently and permanently degrades to the "missing marker" fallback, or the launch command fails outright on an unrecognized flag.
· fix: Verify against an installed codex binary before merge; once confirmed, record the measured result in .claude/orchestrator.json's codex entry with a date, the same way every other codex quirk there already is. Until verified, gate hook installation to the Claude engine only.
· reference: CLAUDE.md rule 1; rubric dimension 15.
Medium
[MEDIUM] The generated-file info/exclude entry for .claude/hooks/report-worker-turn.mjs is a no-op once the file is git-tracked
· dimension: 1 (Correctness) / documentation accuracy
· location: tools/launch-worker.mjs:684-701; .claude/skills/orchestrate/SKILL.md ("The launcher excludes these generated runtime files from the target repo's local git status")
· issue: .claude/hooks/report-worker-turn.mjs is itself a new file this PR commits to the repo (tracked at HEAD). git info/exclude only ever suppresses untracked paths from git status; it has no effect on a tracked file's modified/clean state. Once this PR merges to main, every future worktree already carries this file tracked, so the exclude entry for it does nothing (harmless today only because copyFileSync writes byte-identical content back over itself).
· risk: If the orchestrator's own copy of the hook script ever drifts from what a given worktree's base-branch commit carries, the re-copy would leave a tracked file locally modified, and worker-status.mjs's worktree-clean check would report a false "uncommitted work" failure the exclude entry cannot suppress — right at launch, before the worker does anything.
· fix: Drop .claude/hooks/report-worker-turn.mjs from generatedWorktreePaths (it needs no exclusion once tracked), or stop unconditionally re-copying it when the worktree's checked-out copy already matches the source by content.
· reference: CLAUDE.md rule 1 (root cause) — the claim in SKILL.md overstates what git exclude actually does for this one path.
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/* or orbit-api DTO 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, head eaebb844) |
| Type check | PASS (CI, head eaebb844) |
| Tests (Unit Tests) | PASS (CI, head eaebb844) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS (CI, head eaebb844) |
| Build | PASS (CI, head eaebb844) |
Phase 6 (/validate) and Phase 7 (posting) skipped per this workflow's CI-wrapper contract; CI's own job results are cited above.
What's good
- The 8-concurrent-writer test for
report-worker-turn.mjsis a real git fixture proving append-atomicity under concurrency, not a mock. - The new
worker-status.mjshard-git-state checks (staged-uncommitted,merge-in-progress,unpushed-commits) each ship with a dedicated real-git fixture reproducing the exact scenario named in the PR body's root-cause section. - Comment policy is respected throughout the new code — no narration comments in any added block.
- The
--reports-fileabsolute-path and inside-a-repo/worktree refusals inlaunch-worker.mjsmirror the existing--prompt-fileguards precisely. report-worker-turn.mjscorrectly relies onlast_assistant_messageper Claude Code's documented Stop-hook contract, rather than the async-lagging transcript file.
Recommendation
Not ready to merge. Two High findings need fixing: (1) add a periodic liveness re-check to the babysit loop, independent of new-record arrival, so a fully-silent worker is ever actually inspected; (2) verify the Codex hook wiring against a real codex binary (or gate to Claude-only until verified) and record the result in .claude/orchestrator.json per this repo's own established practice. The Medium finding (drop or fix the now-inert exclude entry for the tracked hook file) is a few lines and worth folding into the same pass.
|
Fixed in 81f6e9d: the monitor now checks every active ticket every 60 to 120 seconds even without new JSONL lines; installed codex-cli 0.145.0 executed a real Stop hook and delivered cwd plus last_assistant_message, with the dated official contract recorded in orchestrator.json; reporter failure paths and worker-status unknown-option handling now have executable coverage. Evidence: ORBIT TOOLS GATE OK, ORBIT HOOK PARITY OK, and current-head push 81f6e9d. Review item: PRR_kwDOR5Siws8AAAABHlBUrA |
|
Fixed in 2ec2bd0: periodic silent-worker checks, real Codex Stop-hook verification, and negative-path coverage remain in place; the delayed exclusion finding is now fixed by keeping report-worker-turn.mjs tracked and ignoring only generated settings files. The full fixture proves the tracked reporter and ends with ORBIT TOOLS GATE OK. Review items: PRR_kwDOR5Siws8AAAABHlBUrA, PRR_kwDOR5Siws8AAAABHlDwHw |
|
No code change required: Vercel reports an intentionally skipped deployment and no review finding. Evidence: 2ec2bd0. Review item: IC_kwDOR5Siws8AAAABMLTM3g |
6c0faca to
049a938
Compare
|
Current-history acknowledgement for automated review activity after the rebase. Fixed in 049a938: periodic silent-worker checks, real Codex Stop-hook verification, negative-path coverage, tracked reporter handling, and the ORB-131 concurrent budget-reservation integration all pass in the rebased history. ORBIT TOOLS GATE OK and ORBIT HOOK PARITY OK. Review items: PRR_kwDOR5Siws8AAAABHlBUrA, PRR_kwDOR5Siws8AAAABHlDwHw. No code finding from Vercel item IC_kwDOR5Siws8AAAABMLTM3g on this head; it is deployment status only. |
|
No code change required: SonarQube reports a passing quality gate with zero new issues on current head 049a938. Review item: IC_kwDOR5Siws8AAAABMLXgDQ |
|
No code change required: SonarQube reports a passing quality gate with zero new issues on current head 049a938. Review item: IC_kwDOR5Siws8AAAABMMGubg |
There was a problem hiding this comment.
Code Review: PR #652
Scope: PR #652 in thomasluizon/orbit-ui-mobile (base main, head feature/orb-136-have-workers-report-status-instead-of @ 049a938a)
Recommendation: APPROVE
Summary
This is a re-review focused on the diff since the last automated review (two prior claude
CHANGES_REQUESTED reviews, both at commit eaebb844). The four commits since then
(6af61f30 "start report monitor before workers", bb142b19 "close worker review gaps",
dad0175b "keep reporter tracked", 049a938a "keep concurrent launch coverage") were read in
full and cross-checked against the actual committed source, not just their commit messages.
Every High and Medium finding from both prior reviews is now closed by an executable test or a
verifiable documentation fix, not by prose alone. No new Critical/High issue surfaced in the
delta. The diff remains entirely orchestration tooling (.claude/hooks/**, tools/**,
.claude/skills/orchestrate/SKILL.md) — no apps/*, orbit-api, packages/shared, or i18n
surface changed, so the five review subagents and the backward-compat guard stay N/A, as in both
prior passes.
Findings
Critical
None.
High
None. Both High findings from the prior reviews are resolved:
- Babysit loop had no trigger for a fully-silent worker.
bb142b19adds to
.claude/skills/orchestrate/SKILL.md§"Babysit": "The monitor loop also wakes every 60 to 120
seconds, following the foreground-waiting rule below. On every wake, run that same artifact
check for EVERY active ticket, including when no line was appended." Combined with6af61f30
(start the monitor before the first worker launch), this ties the babysit loop explicitly to
the pre-existing "Waiting is foreground work" standing rule (poll 60-120s per loop, in the
foreground) and makes the periodic re-check independent of new-record arrival — the exact gap
both prior reviews identified.tools/worker-status.mjs'sexpectedWindowMinutes/
reportFreshlogic (confirmed unchanged and already correct) now has a documented trigger that
reaches it for a worker that produces zero records. - Codex-side hook wiring unverified against real
codex.bb142b19adds a
hookVerificationfield to the codex entry in.claude/orchestrator.json, dated 2026-07-28,
naming the exact CLI version (codex-cli 0.145.0), the exact flag tested
(--dangerously-bypass-hook-trust), the payload fields observed (hook_event_name,cwd,
last_assistant_message), and a citation to the official Codex Hooks contract — matching this
file's own established convention for recording every other measured codex quirk (already
verified by the prior review to otherwise be spotless on this point). This is a documentation
claim that cannot be re-executed in this sandbox (nocodexbinary available), so it is taken
as good faith per the repo's own standing practice, consistent with how every other dated codex
measurement in this file is treated; see Deferred.
Medium
None. All three Medium findings are resolved with real, executed coverage, verified by reading
the actual code paths they exercise:
bb142b19adds four new cases to.claude/hooks/test-hooks.mjsthatspawnSyncthe real
report-worker-turn.mjshook: relative--reports-file(rejected, matches
report-worker-turn.mjs:25), missing--reports-file, malformed--ticket(orb-136fails
/^ORB-\d+$/atreport-worker-turn.mjs:26), and an unwritable target (a directory passed as
the reports path, exercising thecatcharoundopenSync/writeSyncat
report-worker-turn.mjs:93-99). All four assert both the exit code and the exact stderr
contract.bb142b19adds aworker-status.mjscase for an unknown flag (--bogus), exercising the
pre-existingKNOWN_FLAGSallowlist attools/worker-status.mjs:69-71(confirmed unchanged
and already correct — the finding was purely a coverage gap).dad0175bfixes theinfo/excludeno-op:report-worker-turn.mjsis removed from
generatedWorktreePathsintools/launch-worker.mjs:908-911(so it is no longer excluded) and
tools/test-tools.mjs'sstageCheckoutnow pre-commits the hook file into the fixture repo, with
a new assertion (git ls-files --error-unmatch) proving the installed hook stays a tracked
worktree file. This does not eliminate future drift between the launcher's source copy and a
worktree's base-branch commit, but it correctly converts that drift from silently masked (the
original bug) to visible ingit status, which is what the finding actually asked for.049a938afixes a real regression introduced by the PR's own "make--reports-filerequired"
decision: the concurrent-launch race test intools/test-tools.mjsspawnslaunch-worker.mjs
through a bespoke standalone runner script that bypasses the suite's sharedrun()helper (the
helper auto-injects--reports-fileattools/test-tools.mjs:410-411, but this runner does
not use it). Without this fix both concurrent invocations would fail at the new
--reports-file is requiredusage check (tools/launch-worker.mjs:645) before ever reaching
the budget-lock scenario the test exists to prove. Confirmed this is a legitimate, necessary
fix and not decorative.
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/* or orbit-api DTO 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, head 049a938a) |
| Type check | PASS (CI, head 049a938a) |
| Tests (Unit Tests) | PASS (CI, head 049a938a) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS (CI, head 049a938a, completed after this review's first pass) — the specific new/changed cases were also independently traced by hand above against the committed source and found to exercise real code paths, not stubs |
| Build | PASS (CI, head 049a938a) |
Phase 6 (/validate) and Phase 7 (posting) skipped per this workflow's CI-wrapper contract; CI's
own job results are cited above via gh pr view --json statusCheckRollup.
Deferred — N/A dimensions & files not verdicted
- Dimensions 8-14 (DESIGN.md/AI-slop, Parity, i18n, Contract drift, Security frontend/backend,
FEATURES.md parity): all N/A, the diff touches noapps/*,orbit-api,packages/shared, or
i18n surface — unchanged from both prior passes. - All 7 changed files in the PR (
report-worker-turn.mjs,test-hooks.mjs,orchestrator.json,
orchestrate/SKILL.md,launch-worker.mjs,test-tools.mjs,worker-status.mjs) received a
verdict: two (report-worker-turn.mjs,worker-status.mjs) are byte-identical to the
already-reviewedeaebb844content (confirmed via diff), the other five carry this session's
reviewed delta. - The
hookVerificationclaim in.claude/orchestrator.json(realcodex-cli 0.145.0Stop-hook
behavior) is a documentation assertion this sandbox cannot re-execute — nocodexbinary is
available here. Accepted on the same basis the repo already accepts every other dated,
cited codex-quirk entry in that file; not independently re-verified against a live process. - The sibling
orbit-apirepo is not checked out in this job, but no dimension in this PR needs
it (no cross-repo surface changed).
What's good
- Both High findings and all three Medium findings from the two prior reviews are closed with
real, traceable fixes rather than reworded prose — each one checked against the actual
committed source above, not taken on the commit message's word. - The concurrent-launch coverage fix (
049a938a) shows real attentiveness: it is a one-line-root
fix for a regression the PR's own--reports-file-required decision silently caused in a test
that bypasses the suite's shared helper, exactly the kind of thing that is easy to miss. - The
hookVerificationfield's format (dated, CLI-versioned, cited) matches this repo's own
established convention for recording measured CLI quirks inorchestrator.json, rather than
inventing a new documentation style for this one claim. - Comment policy is respected throughout the delta — no narration comments in any added code.
Recommendation
Approve. Both High findings and all Medium findings from the two prior CHANGES_REQUESTED
reviews are resolved with executable coverage or a verifiable, convention-consistent
documentation fix, and no new Critical/High issue surfaced in the diff since the last review.
The one residual soft spot — the Codex hook-wiring claim cannot be independently re-executed in
this sandbox — is a sandbox limitation, not a defect in the fix, and is handled the same way the
repo already handles every other codex measurement of this kind.
|
No code change required: the current-head automated review approved 049a938 with no Critical, High, or Medium findings. Review item: PRR_kwDOR5Siws8AAAABHlZt-w |
|
No code change required: Vercel reports an intentionally skipped deployment and no review finding on current head 7283e7c. Review item: IC_kwDOR5Siws8AAAABMLTM3g |
|
No code change required: SonarQube reports a passing quality gate with zero new issues on current head 7283e7c. Review item: IC_kwDOR5Siws8AAAABMMcrvg |
There was a problem hiding this comment.
Code Review: ORB-136 report structured worker status (PR #652)
Scope: PR #652 in thomasluizon/orbit-ui-mobile
Recommendation: REQUEST CHANGES
Summary
This PR replaces terminal-scraping worker supervision with structured JSONL reports: a new worktree-local Stop hook (report-worker-turn.mjs) appends one record per worker turn, launch-worker.mjs provisions the shared reports file and installs the hook for both Claude and Codex, worker-status.mjs folds report liveness into its existing hard git/PR/Linear checks, and orchestrate/SKILL.md documents the new babysitting loop. The new hook and the new worker-status.mjs checks (staged-uncommitted, merge-in-progress, unpushed-commits, report-fresh) are well designed and thoroughly tested, including a concurrency test proving 8 simultaneous appends lose no records. One real defect survives review: a design decision made in dad0175b ("fix: keep reporter tracked") removes the copied hook script from the worktree's git-status exclusion list so that source drift stays visible for the ui repo — but the same unconditional copy also runs for worktrees opened against orbit-api and orbit-landing-page, which are separate git histories that have never seen this file. That leaves it permanently untracked (and therefore permanently "dirty") there, which structurally breaks the new worktree-clean check this very PR introduces for two of the three configured repos.
Findings
Critical
None.
High
[HIGH] Copied worker-report hook is left untracked in orbit-api/orbit-landing-page worktrees, permanently failing the new worktree-clean check
· dimension: 1. Correctness
· location: tools/launch-worker.mjs:908-911, 926-937
· issue: launch-worker.mjs copies orbit-ui-mobile's own .claude/hooks/report-worker-turn.mjs into
every launched worktree unconditionally (line 937: `copyFileSync(hookSource, hookTarget)`) for
whichever repo the ticket's repo:* label resolves to (line 731-734: `repoPath =
config.repos[repoKey]`, where repoKey is "ui", "api", or "landing" per
.claude/orchestrator.json). `generatedWorktreePaths` (lines 908-911) only excludes
`/.claude/settings.local.json` and `/.codex/hooks.json` from the worktree's git status —
commit dad0175b ("fix: keep reporter tracked") deliberately dropped the hook script from this
list so drift stays visible for the ui-mobile repo's own history. But
report-worker-turn.mjs is brand-new to this PR and lives only in orbit-ui-mobile's own git
history (root CLAUDE.md: orbit-api and orbit-landing-page are separate histories), so it cannot
already be tracked in either sibling repo.
· risk: Any ticket launched with `--repo api` or `--repo landing` (a routine, first-class scenario
per root CLAUDE.md's cross-repo workflow, not an edge case) leaves an untracked
`.claude/hooks/report-worker-turn.mjs` sitting in the worktree. The worker's own contract
forbids blanket-staging (`git add -A`/`git add .`), so nothing ever commits or clears this file.
tools/worker-status.mjs's new `worktree-clean` check (`dirty = git(["status","--porcelain"])`,
`ok: dirty.length === 0` at worker-status.mjs:145,319) then reports UNMET forever for that
ticket, even once the worker's real work is committed, pushed, reviewed, and merge-ready. This
breaks the exact done-detection contract this PR exists to add, for 2 of the 3
orchestrator-tracked repos. Verified directly: config.repos supports ui/api/landing keys
(launch-worker.mjs:710,733), the hook-copy path has no repoKey branching, and git status
--porcelain includes untracked files by default.
· fix: Before copying, check whether the target repo already tracks the hook path
(e.g. `git -C worktreePath ls-files --error-unmatch .claude/hooks/report-worker-turn.mjs`,
allowing failure). When it is NOT already tracked (the api/landing case), add
`/.claude/hooks/report-worker-turn.mjs` back to `generatedWorktreePaths` before writing the
exclude file. Keep it un-excluded for orbit-ui-mobile itself, since SKILL.md states the
reporter "stays tracked, so any source drift remains visible" — that drift-detection benefit
should be preserved for the repo that actually owns the file, without breaking the check for
the other two.
· reference: CLAUDE.md rule 1 (root cause over workarounds — the hook-installation step should
handle all three configured repos, not just the one it was tested against); root CLAUDE.md
"cross-repo work uses an API ticket blocking a UI ticket" (cross-repo is a designed, routine
path, not exotic).
Medium
[MEDIUM] Hook-copy test fixture always pre-tracks the file, masking the untracked-file gap
· dimension: 15. Harness changes need EXECUTED evidence
· location: tools/test-tools.mjs:792-812 (stageCheckout)
· issue: stageCheckout pre-creates and commits .claude/hooks/report-worker-turn.mjs into the
fixture repo (path literally `repos/ui`) BEFORE `worktree add` runs, and every test that
exercises the hook-install path (pointerDeliveryCases, launchWorkerCases) uses this one
fixture. No fixture simulates a target repo where the copied file starts out untracked, which
is exactly the orbit-api/orbit-landing-page scenario above.
· risk: The harness stays green even though the hook-installation step is broken for 2 of 3
configured repos — a new decision path (repo already tracks the hook vs. does not) shipped with
no case of its own, so a regression here would not be caught either.
· fix: Add a second fixture (or parameterize stageCheckout) that creates the worktree's target
repo WITHOUT pre-committing report-worker-turn.mjs, then assert after a real launch that (a) the
copied file exists, and (b) `git status --porcelain` in that worktree is clean immediately
after installation (i.e., the file is either tracked-and-clean or properly excluded).
· reference: rubric dimension 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 (signal gate: Low/Info are not surfaced 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 i18n JSON changed |
| contract-aligner | N/A — diff touches neither packages/shared/src/types/* nor orbit-api |
| 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 workflow's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR.
Prior reviews on this PR
Earlier rounds on this PR requested changes twice and were then approved at 049a938a. That approval predates this finding's root cause: commit dad0175b ("fix: keep reporter tracked"), which removed the hook script from the git-status exclusion list, landed before 049a938a and was not flagged by the prior review. This is not a re-flag of an already-resolved concern — it is a defect that has been present since before the last approval and was missed.
What's good
- The Stop hook (
report-worker-turn.mjs) is small, defensive, and correctly validates its own inputs (--reports-fileabsolute,--ticketmatchesORB-\d+, gate values whitelisted,contractItemstype-checked) before trusting anything from the model's own output. - Genuinely excellent test coverage for the new pieces: a real concurrency test proves 8 simultaneous hook invocations append exactly 8 well-formed records with no interleaving corruption; the fallback path (missing/malformed
WORKER_REPORT:marker) is tested and correctly marksneedsHuman: true. worker-status.mjs's new checks (staged-uncommitted, merge-in-progress viaMERGE_HEAD, unpushed-commits via a freshls-remote+ targeted fetch) are all individually correct and each has a dedicated git fixture reproducing the exact failure mode.- The Codex
--dangerously-bypass-hook-trustflag is deduplicated correctly and documented with genuine measured evidence plus an official-docs URL inorchestrator.json. launch-worker.mjscorrectly refuses a--reports-fileliving inside any configured repo or inside the new worktree itself, mirroring the existing--prompt-fileprotection.
Recommendation
Fix the High finding (make the hook-exclude decision conditional on whether the target repo already tracks the file, so the copy doesn't pollute git status in non-ui-mobile worktrees) before merge — it silently breaks the PR's own headline feature for orbit-api and orbit-landing-page tickets. Add the Medium's fixture case in the same pass so the gap can't regress silently. Everything else in the diff is solid and well-tested; re-review should be fast once those two related items land.
|
Fixed in 6e87ceb8522861562482a5ce85d8c919173ea801: target repositories that do not track the worker report hook now exclude the generated copy, while orbit-ui-mobile keeps its tracked reporter visible for drift. The new real-launch fixture proves installation and a clean worktree for the untracked case. Review item: PRR_kwDOR5Siws8AAAABHliv9g |
|
Fixed in 6e87ceb: target repositories that do not track the worker report hook now exclude the generated copy, while orbit-ui-mobile keeps its tracked reporter visible for drift. The new real-launch fixture proves installation and a clean worktree for the untracked case. Review item: PRR_kwDOR5Siws8AAAABHliv9g |
|
No code change required: Vercel reports an intentionally skipped deployment and no review finding on current head 6e87ceb. Review item: IC_kwDOR5Siws8AAAABMLTM3g |
|
Acknowledged automated activity IC_kwDOR5Siws8AAAABMLTM3g on current PR head 258a832. The skipped Vercel deployment is expected because this tools-only change does not affect the web deployment. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 258a832231
ℹ️ 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 |
|
Acknowledged automated activity IC_kwDOR5Siws8AAAABMLTM3g on current PR head 40c942c: the skipped Vercel deployment is expected for this tools-only change. Acknowledged automated activity IC_kwDOR5Siws8AAAABMN6V9g on the same head: SonarCloud passed with zero new issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40c942ce54
ℹ️ 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".
Escalation for ORB-136Current PR head: Thread A correct remaining fix needs a canonical required-gate schema across every repository this monitor checks. Evidence on this head:
Acknowledged automated activity |
…workers-report-status-instead-of
|
@codex review |
|
No code change required for d68fbcc: Vercel item IC_kwDOR5Siws8AAAABMLTM3g reports an intentionally skipped deployment and no review finding for this tools-only change. |
|
No code change required for d68fbcc: SonarCloud item IC_kwDOR5Siws8AAAABMPgHVg passed with zero new issues, zero accepted issues, and zero security hotspots. |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
thomasluizon
left a comment
thomasluizon
left a comment
There was a problem hiding this comment.
Code Review: PR #652 (ORB-136, report structured worker status)
Scope: PR #652 in thomasluizon/orbit-ui-mobile (head d68fbcc6, 7 files, +978/-59)
Recommendation: NEEDS WORK
Summary
This is a harness-only change: a worktree-local Stop hook that appends one structured JSONL
record per worker turn, launcher wiring that provisions the shared reports file and installs the
hook for both engines, and a worker-status.mjs that now combines report liveness with hard git
state. The design is sound and the test additions are unusually thorough: the concurrency case,
the fallback-marker case, and the new git-state fixtures are real executed coverage, and the
Harness Execution job is green on this head. Two things block: the shared reports file has no
tolerance for a single corrupt line, which takes down status for every ticket in the run at once;
and the installed Stop hook has never been observed firing under either shipped engine
invocation, which is exactly the gap rubric dimension 15 exists to catch.
Findings
Critical
None.
High
[HIGH] One corrupt line in the shared reports.jsonl takes worker-status down for every ticket in the run
· dimension: 1 (Correctness) / 12 (Error handling at boundaries)
· location: orbit-ui-mobile/tools/worker-status.mjs:169-186 (the `if (reportsFile)` parse block;
the `fail(3, ...)` is at :180)
· issue: the reports file is ONE path shared by every worker in every wave of a run
(SKILL.md §2: "Create ONE reports.jsonl path ... Reuse that absolute path for every worker in
every wave"). The parse does `lines.map(line => { try { return JSON.parse(line) } catch {
fail(3, ...) } })`, and `fail` exits the process. So a line written by worker B kills the status
check for worker A, C and D as well.
· risk: compounded by the SKILL.md rewrite. §3 now says "Terminal text is never the normal status
source" and "A missing report is the diagnostic trigger ... worker-status.mjs marks a worker
suspect ... Only after that verdict may you inspect its terminal". An exit 3 produces no verdict
at all, so the one documented escape hatch never opens. In a `--sleep` run the whole fleet goes
dark behind a one-line parse error. Reachable whenever a hook process dies mid-append: the hook
itself (.claude/hooks/report-worker-turn.mjs:100-106) throws on a short `writeSync` AFTER the
partial bytes are already on disk, and teardown-worktree.mjs kills terminals while sibling
workers are still writing.
· repro (red, ran against this PR's head):
line 1: valid ORB-136 record
line 2: '{"reportedAt":"...","ticket":"ORB-999","headSh' <- truncated, no newline
line 3: valid ORB-136 record
$ node tools/worker-status.mjs --worktree <wt> --issue ORB-136 --reports-file reports.jsonl --json
reports file line 2 is not valid JSON
EXIT=3
ORB-136's own two records are intact and it still gets nothing.
· fix: the JSONL file is a multi-writer external input, so treat it as a trust boundary and
degrade instead of aborting: skip unparseable lines, count them, and surface the count as part
of the `report-fresh` detail (e.g. `2 unreadable line(s) skipped`). A worker whose own latest
record was the corrupt one then falls through to the existing `no report exists for ORB-N`
rejection and the `suspect` liveness verdict, which is the behaviour the skill already
documents and the orchestrator already knows how to act on.
· reference: CLAUDE.md rule 8 (error handling at boundaries; never let a boundary failure take
down the caller), rubric dimension 1 (boundary conditions)
[HIGH] The installed Stop hook has never been observed firing on either shipped engine invocation
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: orbit-ui-mobile/tools/launch-worker.mjs:951-970 (generated hook commands);
.claude/orchestrator.json (the new `hookVerification` entry)
· issue: the PR makes structured reports "the normal status path", but nothing in the change set
executes the path end to end.
- Codex: the only recorded measurement is `hookVerification`, which states the evidence came
from "a real codex exec invocation". The launcher launches BARE INTERACTIVE `codex`, and this
repo refuses `codex exec` outright as a headless token (orchestrator.json codex `notes`:
"codex exec is the NON-interactive subcommand and lands in the same unsupervisable place as
claude -p"). The measured invocation is not the shipped invocation.
- Claude: no measurement, no doc citation, nothing. The Claude Stop hook is the status channel
for every claude-engine worker and its only evidence is a string assertion.
- The tests assert the generated command STRINGS (tools/test-tools.mjs, the `reportCommands`
block) and separately execute the hook script with a synthetic payload
(.claude/hooks/test-hooks.mjs), but nothing executes the generated command, and nothing
observes a real engine emitting Stop.
· risk: if either engine does not emit Stop per turn in TUI mode, or the generated command does
not resolve as written, that engine's workers are silently permanently suspect. The degradation
is safe (suspect -> terminal diagnosis) but it costs a 30-minute window per worker per check and
it silently reverts the run to the expensive terminal-scraping this ticket exists to replace.
· fix: run one real launch per engine and paste the appended `reports.jsonl` line into the PR
body, using the shipped `node tools/launch-worker.mjs --issue ORB-N --prompt-file ... --reports-file
...` invocation, bare interactive `codex` for one and `claude` for the other, one worker turn,
then `cat` the record. Two launches, and both halves of the claim become executed evidence.
Then correct the `hookVerification` note to name the invocation that was actually measured.
· reference: rubric dimension 15 ("A claim that a tool, hook, skill, or agent works (in the PR
body, a code comment, or the review) must trace to a command that ran and its output")
Medium
[MEDIUM] The launcher writes exclude entries into the SHARED .git/info/exclude, not the worktree's
· dimension: 5 (No-workaround / root-cause)
· location: orbit-ui-mobile/tools/launch-worker.mjs:932-942
· context: this is not a duplicate of the resolved thread "Copied worker-report hook is left
untracked in orbit-api/orbit-landing-page worktrees". The exclude write IS that thread's remedy;
the finding is a side effect of the remedy that the thread did not cover.
· issue: `git rev-parse --git-path info/exclude` inside a linked worktree resolves to the parent
repo's COMMON exclude file, not a per-worktree one. Verified:
$ git worktree add --detach ../wt HEAD && cd ../wt && git rev-parse --git-path info/exclude
.../repo/.git/info/exclude
So every launch permanently edits the parent repo's exclude file, affecting the main checkout
and every sibling worktree. `tools/teardown-worktree.mjs` contains no reference to `exclude`,
`settings.local` or `.codex`, so nothing ever removes them.
· risk: `/.codex/hooks.json` (and, in orbit-api / orbit-landing-page, `/.claude/hooks/
report-worker-turn.mjs`) become invisible to `git status` and `git add .` in the real repo,
forever, as a side effect of an unrelated worker launch. The dedupe via the `excludeLines` Set
keeps the file from growing, so this is silent rather than noisy.
· fix: either commit the two paths to each repo's tracked `.gitignore` (they are generated runtime
files in all three repos, so this is the honest home for them), or scope the exclusion to the
worktree with `git config --worktree core.excludesFile <worktree-local file>` after enabling
`extensions.worktreeConfig`. Whichever is chosen, have `teardown-worktree.mjs` reverse it.
· reference: CLAUDE.md rule 1 (root cause over workarounds); "a permanent machine-enforced fix is
the deliverable"
[MEDIUM] tools/README.md still documents an invocation that now exits 2
· dimension: 2 (stale) / repo docs-registry convention
· location: orbit-ui-mobile/tools/README.md:51
· issue: the catalog's Invocation column still reads `node tools/launch-worker.mjs --issue ORB-N
--prompt-file <path>`. `--reports-file` is now required, and this PR's own test asserts it
(`launch-worker.mjs: requires --reports-file` -> status 2), and that test is green in the
Harness Execution job. The README row's description also still enumerates "the four measured
launch gotchas" and says nothing about hook installation or the reports file.
· risk: `tools/README.md` is the tool catalog an agent reads instead of the source. Copy-pasting
the documented command now fails with exit 2 and the agent has to go read the tool anyway.
· fix: update the row's Invocation to include `--reports-file <absolute path>` and add one clause
to the description covering the reports-file provisioning and the two Stop hooks, in this PR.
· reference: root CLAUDE.md ("Tools follow tools/CONVENTIONS.md; catalog ... tools/README.md";
"When you change a doc, update its At a glance header and this registry in the same edit")
[MEDIUM] worker-status's report checks fail OPEN when --reports-file is omitted
· dimension: 6 (gates over prose)
· location: orbit-ui-mobile/tools/worker-status.mjs:462, 498
· issue: `--reports-file` is optional. Omit it and the `report-fresh` / `report-accepted` checks
are never pushed into `checks`, `liveness` becomes `"not-checked"`, and the tool can print
CONTRACT MET having verified no liveness at all. The only thing that makes the orchestrator pass
the flag is a sentence in SKILL.md §3, and that is prose, which decays as context fills.
· risk: the exact failure this ticket exists to remove (a silent worker read as complete) returns
the first time the flag is dropped from the command, and the verdict looks identical to a
genuinely healthy one.
· fix: keep the flag optional for the standalone use the PR body describes, but make the
difference visible rather than silent: when `--reports-file` is absent, print the liveness line
as `NOT-CHECKED liveness: no reports file supplied; completion is artifact-only` in the text
output too (today that line is inside `if (reportsFile)`), so a verdict with no liveness can
never be mistaken for one with liveness. Better still, have `launch-worker.mjs` record the
reports path in the worktree so `worker-status.mjs` can default to it.
· reference: CLAUDE.md "Gates over prose"; rubric dimension 6
[MEDIUM] New refusal paths and payload branches ship without their own cases
· dimension: 15 (a new decision path needs its own case)
· location: orbit-ui-mobile/tools/worker-status.mjs, tools/launch-worker.mjs
· issue: the diff adds these decision paths, none of which has a test:
- worker-status: `--reports-file` present with no value -> exit 2 (:117)
- worker-status: reports file unreadable -> exit 3 (:174)
- worker-status: malformed JSONL line -> exit 3 (:180), the High above
- worker-status: `gates` not an object -> "gates payload is invalid" (:227)
- worker-status: a gate value outside passed/failed/not-run -> "gate results are invalid" (:235)
- launch-worker: reports file inside the NEW worktree -> exit 3 (:914)
- launch-worker: reports file cannot be provisioned -> exit 3 (:876)
- launch-worker: exclude write fails -> exit 3 (:942)
- launch-worker: Stop hook install fails -> exit 3 (:972)
- launch-worker: `gitTracksPath` exit > 1 -> exit 3 (:423)
- launch-worker: the config ALREADY declares `--dangerously-bypass-hook-trust` (the dedupe
branch at :824); only the not-declared side is asserted
· risk: each is a refusal that merges unexecuted; the rubric's own history is that the
unexecuted refusal is where harness bugs live (the `orca terminal wait` "not yet" case).
· fix: add a case per path. The three fail(3) install paths are cheap to drive by pointing the
launcher at a read-only directory; the gates-payload branches are pure fixture edits alongside
the existing `rejectedReports` table, which is already the right shape for them.
· reference: rubric dimension 15
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A, no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A, no user-facing string or locale file changed |
| contract-aligner | N/A, single repo; no packages/shared/src/types/* or endpoints.ts change |
| security-reviewer | N/A, no orbit-api code changed |
| design-reviewer | N/A, no UI file changed |
Validation
Evidence is the CI run on this PR's head (d68fbcc6), not a local re-run: the working tree is on
an unrelated branch, so a local /validate would have validated the wrong code.
| Check | Result |
|---|---|
| Lint | PASS (job 90548985744) |
| Type check | PASS (job 90548985793) |
| Tests | PASS, Unit Tests (job 90549235943) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS (job 90548986504, 5m46s). Rubric dimension 15's required citation |
| Build (api) | N/A, no orbit-api change |
review (claude-review.yml) |
FAIL. Not a code defect: 429 rate_limit_event, rateLimitType: seven_day, "You've hit your weekly limit, resets 7am (UTC)". The reviewer never rendered a verdict; the red check is a quota artefact. |
Deferred: N/A dimensions & files not verdicted
Every changed file got a verdict; nothing is deferred on coverage. Dimensions marked N/A, each
because the diff never touches its surface:
- 8 DESIGN.md / AI-slop: no
apps/*UI file changed. - 9 Parity: no
apps/web/**orapps/mobile/**file changed. - 10 i18n: no locale file or user-facing string changed.
- 11 Contract drift + backward-compat: no
packages/sharedschema and no orbit-api DTO in
the diff, so Phase 5's field add/remove/rename scan had no candidates. No
⚠️ breaks old mobile clientsrisk exists in this change. - 13 Backend hard rules: no
orbit-apichange. - 14 FEATURES.md parity: harness-only; no screen, route, Astra/MCP tool, gating, platform or
locale behaviour changed. - 6 Type safety:
.mjstooling, no TypeScript surface; noany-class escape hatch exists to
check.
Checked and clean, listed so "clean" does not read as "not looked at": 2 dead code (one note
below), 3 SOLID (threadHasFix is 54 lines, marginally over the ~50 soft cap and well under
the ~100 hard cap, defensible as one cohesive predicate), 4 comment policy (the new hook
carries zero comments; no narration added anywhere), 5 no-workaround (the
--dangerously-bypass-hook-trust flag is the vendor-sanctioned path for vetted automation and
carries a WHY-with-URL in hookVerification), 7 no console.log (the CLI's stdout contract,
pre-existing), 12 security (the generated hook command interpolates only a resolved absolute
path and an issue already validated against ^[A-Z]+-\d+$).
One sub-Medium note not posted as a finding, per the signal gate: the new staged-uncommitted
check (worker-status.mjs:374) can never fail independently. git status --porcelain, which
already backs the existing worktree-clean check, reports every staged path. The PR's own
staged-merge fixture demonstrates it: both checks go red together.
What's good
- The concurrency test is the right test. Eight simultaneous hook processes, asserting both
that all exit 0 and that all eight lines survive and parse, is exactly the property a shared
append-only channel needs, and it is proved by execution rather than argued from O_APPEND
semantics. - The fallback record is fail-safe by construction. A missing or malformed
WORKER_REPORT
marker still appends a record, withblockedOnset to the explicit reason andneedsHuman: true,
so a worker that forgets the contract escalates instead of vanishing. That is the correct
default and it is tested. - Hard git state now outranks a healthy PR.
staged-uncommitted,merge-in-progressand
unpushed-commitsclose the exact hole the ticket names, and the two fixtures prove the
interesting half: a green Linear state and an approved PR no longer mask a wedged worktree. - The launcher refuses a reports path inside any repo or the new worktree with the same shape
as the existing prompt-file refusal, and the consistency means an agent that learned one already
knows the other. - Ordering is right: the hook is installed before
createTerminal, so the engine picks it up
at startup rather than needing a mid-session reload. hookVerificationis an honest artefact even though it is incomplete: it names the date, the
CLI version, the exact payload fields observed and the doc URL. The fix for the High above is to
extend it, not to write it from scratch.
Recommendation
Two things before merge:
- Make the reports-file parse degrade instead of aborting (High #1). This one is small and it is
what stops a single truncated line from blacking out an unattended run. - Produce the executed evidence for the shipped path (High #2): one real launch per engine, the
appendedreports.jsonlrecord pasted into the PR body, andhookVerificationcorrected to
name the invocation actually measured.
The four Mediums are all cheap and land naturally alongside those two, the README row and the
info/exclude scoping in particular, since both are the kind of silent drift that is much harder
to find later than it is to fix now.
Correction to my review above: withdrawing the second HighMy review posted two High findings. The second one is wrong and I am withdrawing it. The What I claimed"The installed Stop hook has never been observed firing on either shipped engine invocation" What measurement showedBoth engines were run under a real PTY via Codex, bare interactive TUI, codex-cli 0.145.0 on Windows 11, exact shipped flag set including {"hook_event_name":"Stop","cwd":"C:\\...\\hooktest","last_assistant_message":"pong","turn_id":"019fad99-..."}
Claude, bare interactive TUI, brand-new untrusted folder, {"hook_event_name":"Stop","cwd":"...","last_assistant_message":"pong","permission_mode":"bypassPermissions"}So on Windows Why the severity was wrong twice overBesides being empirically false, the finding also mis-weighted the failure mode, which is What survives, downgraded to Low and non-blocking
That is a documentation gap, not a defect. The fix is to paste the two measurements above into the Net effect on the verdictUnchanged: still NEEDS WORK, now on one High rather than two. The blocker is the reports-file |
|
Acknowledgement for review activity Fix commit:
Verified on the fix commit: exhaustive tools gate, lint, type-check, tests, hook parity, dash baseline, copy baseline, syntax, and diff checks all pass. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c09db99c
ℹ️ 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".
|
|
No code change required for automated activity |
|
@codex review |
|
No code change required for automated activity |
|
Codex Review: Didn't find any major issues. Breezy! 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 for automated review activity |
|
Closing unmerged. This ticket's premise was replacing TUI terminal scraping with structured worker reports. The harness is moving to headless What remains in this PR is worker-authored status, which repeats the dominant defect class in the 2026-07-28/29 ledger: a report asserting an outcome nothing independently verified. Supervision moves instead to independent reconciliation against git, the forge, process exit and Linear, none of which the worker can author. The open High on this branch (one corrupt line in the run-wide shared JSONL calls |
1 participant
Summary
Root cause
Worker supervision treated terminal state as status. That made healthy runs expensive to scrape and allowed silent workers, staged merge state, or unpushed commits to look complete when PR and Linear state appeared healthy.
Decisions taken unattended
--reports-filerequired for launches so every worker has an explicit shared reporting destination.NOT-CHECKED.cwdthrough a lock-protected registry under Git's common directory..git/info/exclude.Real Stop hook verification
claude --permission-mode bypassPermissions --model opus, with the exact generated.claude/settings.local.jsonStop hook. One turn appended a complete 265-byte worker report. The full launcher could not cross quota preflight becauseai-quotacould not read the Claude quota, so passage through that launcher gate is not verified.Running Stop hook, but the worktree-local report file remained empty because Codex loaded project hooks from the primary root.9b86a2f58d8c3352e1563eda682e8d4bc732d156. The primary hook invokes the stable dispatcher, registered worktreecwdvalues route to separate JSONL files, unrelated roots exit cleanly without a record, simultaneous workers retain independent routes, one deregistration preserves the other route, launch rollback removes its route, and the final deregistration restores the original primary hooks, config, and generated excludes state byte for byte. A separate direct lifecycle run returned{ok:true,statusUnchanged:true,firstExit:0,secondExit:0,unrelatedExit:0,routesAfterFirst:1,hooksRestored:true,configRestored:true,excludeRestored:true,stateRemoved:true,runtimeRemoved:true}.Verification
Linear: ORB-136