chore(tools): execute the harness in CI instead of only reading it - #606
Conversation
The repo had independence but no execution: claude-review.yml runs a fresh session on every PR and reviews the DIFF, so a broken tool can be read, approved and merged. tools/launch-worker.mjs shipped in PR #604 treating `orca terminal wait`'s "not yet" (exit 1 with an ok:false payload) as fatal, which only running it caught. node .claude/hooks/test-hooks.mjs was the one executable proof in this layer and it covers hooks only. - tools/test-tools.mjs: runs every script in tools/, asserting the CONVENTIONS.md CLI contract (--help exits 0 with usage, invalid input refused before any work) plus each tool's real decision paths, with orca stubbed and every side effect staged in a temp dir. Hermetic: no network, no worktree, no Linear. - Structural coverage guard: a tools/*.{mjs,sh,ps1} with no coverage entry fails the suite, so tool N+1 cannot land uncovered. - guards.yml Harness Execution job: runs the tools gate and the hook suite, scoped to tools/** and .claude/**, always reporting so it can be required. - The contract fixes the gate found: --help exited 2 (check-copy, check-dashes) or ran the tool (check-frontmatter, check-push-target, check-suppressions-ratchet, redesign-coverage); a bogus flag silently rewrote architecture.json and surfaces.json (arch-map, surface-manifest) or reached the GitHub API as a 404 that read as "nothing to merge" (merge-sweep, merge-sweep-cov); check-ticket --file threw an unhandled ENOENT; check-frontmatter resolved its roots from the caller's cwd. - Review rubric dimension 15: a diff touching tools/, skills, agents, hooks or orchestrator.json needs EXECUTED evidence, and "verified" with no command behind it is itself a finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBFbK416nnXc7kJwkknZpo
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: PR #606
Recommendation: APPROVE
Summary
PR #606 adds tools/test-tools.mjs, wired into guards.yml as the "Harness Execution" job, which actually runs every script in tools/ against the CONVENTIONS.md CLI contract (--help exits 0, bad input refused before any work) plus per-tool decision paths, with orca stubbed hermetically via a --require-preloaded shim on NODE_OPTIONS. It fixes seven real contract violations the new gate found on main (broken/missing --help, silently-swallowed bad arguments, a cwd-unsafe check-frontmatter.mjs), adds rubric dimension 15 ("harness changes need EXECUTED evidence"), and updates TESTING.md, tools/CONVENTIONS.md, tools/README.md, and CLAUDE.md in the same PR. Scope is entirely tools/, .claude/, .github/workflows/guards.yml, and root docs — no apps/*, packages/shared, or orbit-api surface touched, so five of six subagents and six rubric dimensions are cleanly N/A.
Static verification: traced the orca-stub mechanism (correct — the shim no-ops when argv[1] resolves to a real file on disk, i.e. when node is running the tool itself, and only answers the stub plan when it's being invoked as orca's subcommand path), confirmed all 13 patched scripts' new --help/unknown-argument logic is internally consistent with their existing usage/exit-code conventions, verified merge-sweep-cov.sh defines the usage() function the new validation block calls, and cross-checked the PR body's "16 scripts, 3 pending" claim against the actual tools/ directory listing — it matches exactly (launch-worker.mjs, nudge-worker.mjs, worker-status.mjs are genuinely absent from this branch, consistent with living on the still-open PR #604).
Findings
Critical: None
High: None
Medium: None
Low/Info:
- [Info]
tools/test-tools.mjsis well-structured: no function near the 50-line soft cap, and the orca stub is a genuine root-cause solution for hermetically testing tools that shell out toorca, not a workaround (CLAUDE.md rules 3, 5). - [Info]
tools/capture-surfaces.mjs:40— the new--api-baseline in the printedUSAGEblock documents a flag whose parsing and four call sites already existed pre-PR; confirmed via grep so it isn't miscounted as new, ungated functionality.
Subagents
All five (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) — N/A, none of their gating surfaces changed.
Validation
Per the CI adaptation for this PR: Build, Unit Tests, and SonarCloud are separate required checks (skipped here). All relevant guards.yml jobs are green on the head commit (e5178a3a): Harness Execution, Lint, Type Check, Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, Cross-Platform Parity, Contract Drift, Design Token Guard, Skill and Agent Frontmatter, Dependency Audit, Architecture map drift — all SUCCESS.
Deferred
- Dimensions 8, 9, 10, 11, 13, 14 — N/A, none of their surfaces touched.
- Cross-repo dimensions (contract-aligner's orbit-api side, the orbit-api half of the backward-compat guard) — not verifiable in this job (orbit-api not checked out).
- All changed files received an explicit verdict; nothing else deferred.
What's good
Genuine root-cause fix matching its own thesis (runs code instead of reading it), with two "red proof" demonstrations in the PR body showing the gate actually fails on a reintroduced defect and an uncovered new script. Every patched tool got the same minimal, symmetric treatment. check-frontmatter.mjs's cwd-unsafety fix is a real fix, not a patch. worker-status.mjs is honestly scoped out with a stated reason rather than faked. Docs updated in the same PR, including an incidental fix to a pre-existing CLAUDE.md gate-list gap.
|
There was a problem hiding this comment.
Code Review: PR #606
Recommendation: APPROVE
Summary
Adds tools/test-tools.mjs, a hermetic harness-execution gate that runs every script in tools/ against the CONVENTIONS.md CLI contract (--help exits 0, invalid input refused before work) plus per-tool decision paths, with orca stubbed via a NODE_OPTIONS --require shim. Wires it into a new Harness Execution job in guards.yml (path-scoped to tools/** / .claude/**), fixes the seven contract violations the gate found on main, and adds rubric dimension 15 (harness changes need EXECUTED evidence) plus matching doc updates.
I traced every new/changed script against its test-tools.mjs case by hand (argument parsing, exit codes, stderr patterns) — launch-worker.mjs's 11 decision-path cases, nudge-worker.mjs's 7, wave-plan.mjs's DAG-ordering cases, and the --help/invalid-input contract on all 19 covered scripts — and found no mismatch between the stubbed expectation and the real implementation. The PR's own motivating example (orca terminal wait's exit-1 timeout payload) is correctly modeled in both launch-worker.mjs and nudge-worker.mjs, and the matching test-tools.mjs cases (BUSY_STUB, BROKEN_STUB) exercise the exit-code-vs-payload branch, not just the exit code.
Findings
Critical: None. High: None. Medium: None.
Info — gateCases["check-frontmatter.mjs"]'s "runs from any cwd" case reads the repo's actual .claude/skills/.claude/agents tree rather than a staged fixture: hermetic per CONVENTIONS.md (no network/worktree/Linear/writes outside the temp root) but not fixture-isolated, so an unrelated frontmatter regression elsewhere would fail this case for a reason unrelated to cwd-independence. Reasonable trade-off given check-frontmatter.mjs already has its own CI job; no action needed.
Subagents
parity-checker / i18n-syncer / contract-aligner / security-reviewer / design-reviewer: all N/A — the diff touches only tools/**, .claude/**, .github/workflows/guards.yml, CLAUDE.md, TESTING.md; no apps/* or orbit-api file changed.
Validation
Dimension 15 (Harness changes need EXECUTED evidence) requires citing the Harness Execution job's result for this PR: PASS — confirmed via gh pr view 606 --json statusCheckRollup, Harness Execution: SUCCESS. Full rollup for the head SHA is green across the board (Lint, Type Check, Unit Tests, Build, Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, Cross-Platform Parity, Contract Drift, Architecture map drift, Skill and Agent Frontmatter, SonarCloud, CodeQL, GitGuardian, Dependency Audit/Review).
Deferred
Dimensions 8/9/10/12(frontend)/13/14: N/A, no apps/*/orbit-api surface touched. Dimension 11 (contract drift): N/A, no Zod schema or DTO field touched.
What's good
- Genuinely hermetic gate:
orcastubbed via shim that exits 9 (loud failure) on any unstubbed call; no worktree, no network, no Linear touched. - Structural coverage closes exactly the named hole (tool N+1 cannot land uncovered); verified against the current 19-script
tools/directory — all covered. - The two "red proof" demonstrations in the PR body (reintroducing the
nudge-worker.mjsdefect, adding an uncoveredzz-probe.mjs) are backed by real, checkable code paths, not just prose. - Docs (
TESTING.md,tools/README.md,tools/CONVENTIONS.md,CLAUDE.md) stay accurate to the CI wiring they describe.
Recommendation
Approve as-is. PR #604 (the three orchestrate tools this PR adds coverage for) is already merged to main, so the PR body's "PENDING" caveat no longer applies at merge time — this branch's own CI run shows the full 19-script suite green.
… than promoted (#608) The queue read 'Queue is empty' while the reasoning behind a real decision sat only in a session that has since ended. The lesson (a defect is not fixed until the mechanism preventing its recurrence exists) was deliberately NOT promoted to .claude/rules/core.md. Root CLAUDE.md rule 6 already says gates over prose and did not hold, so a ninth prose rule in the always-loaded tier would have been more of the thing that already failed. What was built instead, the same day, is the gate half: PR #604 turns harness defects found in a run into one Linear ticket mechanically, and PR #606 adds a guards.yml job that EXECUTES tools/** plus a structural guard that goes red when a tool lands without coverage. The original entry is kept below the decision so a future reader can disagree with the judgement rather than rediscover the evidence. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>



The hole
The repo already has reviewer independence:
claude-review.ymlruns a fresh Claude session on every PR. What it does not do is execute the harness. It reviews the diff. So a broken tool can be read, approved and merged.node .claude/hooks/test-hooks.mjswas the only executable proof in this layer, and it covers hooks only. Everything undertools/(16 scripts) had no executable gate at all.The measured evidence
tools/launch-worker.mjsshipped in PR chore(orchestrate): make the six measured worker-launch gotchas structural #604 treatingorca terminal wait's "not yet" (exit 1 carrying anok:falsetimeout payload) as a fatal error, which would have broken the exact loop the tool exists for. Reading it did not catch that; running it did.What the gate executes
node tools/test-tools.mjs, wired intoguards.ymlas the Harness Execution job (which also runsnode .claude/hooks/test-hooks.mjs), scoped totools/**and.claude/**by a detect step in the Expo SDK Pin job's style, so the job always reports and can be made a required check.Three layers:
tools/*.{mjs,sh,ps1}and fails on any script with no coverage entry. This is the part that keeps the suite from rotting at tool N+1.tools/CONVENTIONS.md), per script:--helpexits 0 with usage on stdout, and invalid input is refused before the tool does any work..shruns under a resolved real bash (the PATHbashon Windows is the WSL stub),.ps1underpwsh.ORCA_BINat the node binary with a preloaded shim that answers a per-case plan; an unstubbed orca call exits 9 with a stub-miss payload, so an unexpected external call is a loud failure rather than a silent pass. No worktree is created, no network is touched, no Linear issue is read. Covered today:wave-plan(blockedBy DAG ordering, launchable set, empty project),check-dashes,check-ticket,check-push-target,check-frontmatter(cwd safety), plus the three orchestrate tools below.The three orchestrate tools
launch-worker.mjs,nudge-worker.mjsandworker-status.mjslive on the open PR #604 branch, not onmain, so on this branch they printPENDINGand their cases are inert. They were written and verified against that branch's head by materialising the three files locally (uncommitted) and running the suite: all 18 cases pass. The moment #604 merges, they execute for real. Cases include repo resolution from arepo:*label, refusal when the label has noreposentry, refusal of a prompt file inside a repo, refusal of a non-interactive engine (interactive: falseand the field omitted entirely), refusal of headless args behind aninteractive: truedeclaration,worker:sonnetmodel routing, rejection of multi-line nudge text, and the exit-code-vs-payload distinction onorca terminal wait(timeout payload means busy: exit 1, nothing sent; any otherok:falsemeans tool error: exit 3).Red proof 1: break a tool, the gate goes red
Reintroduced PR #604's original defect in
nudge-worker.mjs(if (result.status !== 0) fail(3, ...)before parsing the payload):Reverted; the suite is green again.
Red proof 2: add an uncovered tool, the structural guard goes red
Added a dummy
tools/zz-probe.mjswith no coverage entry:Probe deleted.
What the gate found on main (fixed here)
The contract in
tools/CONVENTIONS.mdwas prose, so seven of sixteen scripts did not meet it:check-copy,check-dashes--helpexited 2--helpprints usage, exits 0check-frontmatter,check-suppressions-ratchet,redesign-coverage--helpran the tool instead of printing usagecheck-push-target--help, silent exit 0arch-map,surface-manifestarchitecture.json/surfaces.jsonand exited 0merge-sweep,merge-sweep-covSKIPand exited 0, so a typo read as "nothing to merge"<owner/repo>and numeric PR ids before the firstghcall, exit 2check-ticket --file <missing>ENOENTstack tracecheck-frontmatter.claude/skillsfrom the caller's cwd, so it failed from any subdirectoryTwo tools were also missing from the
tools/README.mdcatalog (check-frontmatter,check-push-target); both are added.The judgement half
Review rubric dimension 15: a diff touching
tools/**,.claude/skills/**,.claude/agents/**,.claude/hooks/**or.claude/orchestrator.jsonrequires executed evidence. The reviewer cites the Harness Execution result; a red job is Critical, a job that never ran on an in-scope diff is High, a new tool with no coverage is High, and a claim of "verified" with no command behind it is itself a High finding. Kept consistent with the Signal gate: it fires on missing execution and missing coverage, never on a tool's style or flag naming.Docs
tools/README.md(catalog rows, At a glance),tools/CONVENTIONS.md(new "The gate" section, At a glance),TESTING.md(ninth suite, CI mapping; the hook suite is no longer local-only), rootCLAUDE.md(Code standards gate list, a Conventions bullet, the hooks bullet).Verification
node tools/test-tools.mjsgreen on this branch (16 scripts, 3 pending) and green with the three PR chore(orchestrate): make the six measured worker-launch gotchas structural #604 tools materialised (19 scripts, 0 pending).node .claude/hooks/test-hooks.mjs,node tools/check-frontmatter.mjs,node tools/check-copy.mjs --check,node tools/check-dashes.mjs --files <changed>,npm run lint,npm run type-check: all green.surface-manifest --baseline origin/main,redesign-coverage --json,check-suppressions-ratchet,capture-surfaces --filter,merge-sweep <valid slug> zzz).Deliberately out of scope
worker-status.mjsgets contract coverage only. Every one of its checks reads a real git worktree,ghand Linear, and stubbingghneeds an executable shim, which Node refuses to spawn from a.cmdon Windows. A hermetic case for it needs a git-fixture harness that is its own change.Not for merge yet.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LBFbK416nnXc7kJwkknZpo