ORB-163 C2: split tools/test-tools.mjs into one case module per tool - #666
Conversation
…prints Section C2 moves every decision-path case body out of tools/test-tools.mjs into one module per tool under tools/__tests__/, behind a thin runner that keeps the filename and the exit codes. Three documents describe the old internals and one gate is keyed to the old text, so they move first. - rubric.md dimension 15 no longer says coverage lives "in tools/test-tools.mjs". Coverage is now the tool's own case module plus its row in the runner, and a case key naming no real script fails the runner by name instead of being skipped. Reworded, not weakened. - CONVENTIONS.md's "The gate" section described the file's internals rather than its shape. It now says what the runner owns, where a new tool's cases go, and that helpers are imported from the shared prelude rather than copied. - README.md states affirmatively that nothing under tools/__tests__/ carries a lockstep obligation, because check-lockstep.mjs fixes REQUIRED_PATHS at six paths and readManifest rejects any other set. Nobody should add a seventh. - lockstep-declarations.json: editing the rubric staled two fingerprints of declaration repository-rubric-dimensions. Both replacements were read off the checker's own drift output rather than computed by hand: 775306439e6ac6b29368e6d372c1ea592906c8c1ef3c791e924c5690b547230a became f82c87488ad948028fd54e0227835119051e9534a7e00df14b0e7b5ecc92605b, and 04a771464204f4f33f213c0fac476376860253f9ca841550fa899fb60da4a849 became 757aa4ef2ad3e6f3ccad8e46fe0e26934be69be1c6df9e12e68c9d66bbfb37a2. The second fingerprint covers dimension 15, which exists only in this repository, so the paired orbit-api pull request carries the twin's own correction rather than a copy of a dimension it cannot host. Verified with the checker run against the paired orbit-api worktree, exit code read from a file: exit 0, "HARNESS LOCKSTEP OK: 6 pairs checked". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Intended module boundaries, posted before the split commitRead this before the diff. The split is mechanical: every case body moves verbatim, so the review question is not "is this code right" but "is each body in the right module and is nothing shared twice". The partition rule I applied
TOOLS_DIRResolved once in the runner from its own location and injected with The two inverted guardsBoth
Three new assertions ship with them in the same commit: one fixture case proving a staged, existing script is not reported, one proving a nonexistent key is reported by name, and the real guard over the registry. A fourth new assertion proves the injection: a path resolved inside a module equals the runner's and points at a file that exists. The modules
One boundary is not one-to-one and it is deliberate: What C4 gets out of thisA slice adding a harness case for tool X now edits |
Every decision-path case body moves out of tools/test-tools.mjs into tools/__tests__/<tool>.mjs, behind a thin runner that keeps the exact filename, the exact exit codes (0 pass, 1 a failing check, 2 usage error) and the same assertion names in the same order. The file goes from 7,214 lines to 197. The reason is C4, not collision counting: every slice of a harness ticket must add a harness case, so while all cases live in one file every slice intersects on that file and the whole ticket serialises to one slice. Two slices for two different tools now edit two different files. The partition rule. Everything above the first *Cases definition is the shared prelude and goes to tools/__tests__/_harness.mjs. Everything below goes to the module of the tool whose cases use it. A helper used by two or more tool modules moves up into _harness.mjs rather than being copied, and no test module imports another test module. Five helpers qualified on that rule: stageWorkerPidMarker and exitedProbePid, budgetRecord, VALID_TICKET_BODY and VALID_ISSUE. TOOLS_DIR is resolved once in the runner and injected with configure() before a single case module is imported. _harness.mjs exports TOOLS_DIR, REPO_ROOT and SELF as live bindings that configure() fills, so a case body can never resolve tools/__tests__ as the tools directory and silently break every join against it. The prelude is loaded after the --help and bad-argument branches, so those two paths still stage no fixture root. Both existsSync(join(TOOLS_DIR, ...)) skip guards are inverted through one shared detector, orphanCaseKeys(). A case key naming a file that is not in tools/ used to be skipped with fails untouched, so the gate printed ORBIT TOOLS GATE OK and exited 0; it now fails naming the key. The same applies to the merge-sweep CLI flag scan, which filtered a missing filename out instead of failing on it. Four assertions ship with those decision paths in the same commit: a fixture case proving a staged existing script is not reported as an orphan, one proving a nonexistent key is reported by name, the real guard over the registry, and one proving a path resolved inside a module equals the runner's. Coverage did not drop. Every T(...) and check(...) label was extracted from the AST of the old file and of the runner plus all 26 modules, sorted and diffed: 513 labels before, 517 after, and the only difference is the four new cases above. The suite ran end to end before and after with the exit code written to a file and read back separately, never through a pipe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
The Harness Execution job caught this, which is the point of the job. The
orchestrate tracked-doc guard scans every tracked .md/.mjs/.json/.yml/.txt file
for stale one-ticket-only --single documentation and excluded exactly one path,
tools/test-tools.mjs, because that file holds the fixture strings the guard would
otherwise flag as stale prose. After the split those fixtures live in
tools/__tests__/wave-plan.mjs, so the guard flagged the suite's own case body:
FAIL orchestrate flags: tracked-doc guard reads files and finds no
one-ticket-only --single use
scanned 201 tracked text files; stale uses: tools/__tests__/wave-plan.mjs
The harness's source is no longer one file, so the exclusion is no longer one
path. It now covers the runner and everything under tools/__tests__/, which is
the same set as before the split and not one file more. No assertion name
changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
![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: 29b5494e9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: ORB-163 C2 (PR #666)
Recommendation: APPROVE
Summary
Pure mechanical refactor: every decision-path case body moves out of the 7,214-line tools/test-tools.mjs into one case module per tool under tools/__tests__/, behind a 197-line runner that injects TOOLS_DIR via configure() before any case module loads. Docs (rubric.md dimension 15, tools/CONVENTIONS.md, tools/README.md) and two updated lockstep-declarations.json fingerprints land in an earlier commit on the same branch, so they're part of this PR's diff too. No apps/*, packages/shared, or orbit-api surface is touched, so the parity-checker, i18n-syncer, contract-aligner, security-reviewer, and design-reviewer dimensions are all N/A, as are rubric dimensions 8–14.
Findings
Critical / High / Medium: None.
Low/Info (not posted per the rubric's Signal gate, noted for the record): tools/__tests__/_harness.mjs:99 exports resolveBash, but its only consumer is the same file's BASH = resolveBash() one line below — no case module imports resolveBash itself. Inert artifact of the local-scope-to-module-export conversion; not worth its own follow-up.
Independent correctness verification (dimension 15)
- Line count: old file 7,214 lines → new runner 197. Exact match.
- Every one of the 25 case modules imports only from
./_harness.mjs— no case module imports another (the ticket's stated invariant holds). - The runner's 26
CASE_MODULESrows resolve to exactly the 25 files on disk (merge-sweep.sh/merge-sweep-cov.shboth point atmerge-sweep.mjs, which exports bothcasesandcoverageCases). Every module exports what its row expects. - Assertion-count invariant: counted
T(/check(call sites — old file 513, new (508 in__tests__/*.mjs+ 9 in the new runner) = 517, matching the commit message's own claimed "513→517, +4 new cases." - The described orphan-detection fix is real: old
mergeSweepCliFlagCasessilently filtered missing files (.filter((filename) => existsSync(...))); newtools/__tests__/merge-sweep.mjs:8callsorphanCaseKeys(...)and reports the missing name by name instead. - The five helpers promoted to
_harness.mjs(stageWorkerPidMarker,exitedProbePid,budgetRecord,VALID_TICKET_BODY,VALID_ISSUE) are all genuinely multi-consumer, matching the "intended module boundaries" described in the PR. - Spot-checked three narration-style comment blocks that looked like new comment-policy violations (
wave-plan.mjs:7-10,launch-worker.mjs:774-776,_harness.mjs:908-912) — all three pre-exist verbatim in the old file; relocated, not introduced.
Prior review context
No unresolved threads and no prior review from this account on this PR. The one existing automated comment (Codex/chatgpt-codex-connector) surfaced no suggestions.
Validation (CI adaptation per workflow instructions)
Build / Unit Tests / SonarCloud run as required checks outside this review; not re-run here. Guards jobs (Harness Lockstep, Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, Cross-Platform Parity, Context Budget, Harness Calibration, Worker Tier Label Snapshot) and SonarCloud's Quality Gate are the authority for mechanical checks; this review does not re-flag anything they already own.
Deferred
- Dimensions 8–14 (DESIGN.md, parity, i18n, contract drift, security-API, backend hard rules, FEATURES.md): N/A, diff never touches those surfaces.
lockstep-declarations.jsonfingerprints: not recomputed by hand (the hash is over diff-hunks against orbit-api's twin, which is not checked out in this job); deferred to CI's "Harness Lockstep" job.- Full line-by-line diff of all 25 case-module bodies against the old monolith wasn't done file-by-file; the assertion-count invariant plus import/export-surface checks stand in as the correctness proof for a diff whose stated intent is verbatim relocation.
There was a problem hiding this comment.
Code Review: PR #666
Scope: PR #666 in thomasluizon/orbit-ui-mobile — "ORB-163 C2: split tools/test-tools.mjs into one case module per tool"
Recommendation: APPROVE
Summary
This is the mechanical C2 slice of ORB-163: the 7,200+ line monolithic tools/test-tools.mjs is split into a shared prelude (tools/__tests__/_harness.mjs, 935 lines) plus one case module per tool under tools/__tests__/, with the runner (tools/test-tools.mjs) reduced to 197 lines that own only the CLI contract, TOOLS_DIR injection, the CASE_MODULES registry, and the three coverage layers. The split was verified against the actual diff, not taken on the PR description's word:
- No cross-module imports. Every
importline intools/__tests__/*.mjsimports only from./_harness.mjsand Node builtins. - The five claimed shared helpers land exactly where claimed.
stageWorkerPidMarker/exitedProbePidare used only byteardown-worktree.mjs,worker-watch.mjs,automation-budget.mjs;budgetRecordonly bylaunch-worker.mjs/automation-budget.mjs;VALID_TICKET_BODY/VALID_ISSUEonly bycheck-ticket.mjs/new-ticket.mjs— no duplication elsewhere. - Nothing dropped.
check(/T(occurrence count: 513 in the pre-splittools/test-tools.mjs(base commit27febbf9) vs. 517 across the new files — exactly the "+4 new assertions, 0 dropped" claimed. - The two previously-silent guards are now real failures. Both
tools/test-tools.mjs(the decision-path loop) andtools/__tests__/merge-sweep.mjs(mergeSweepCliFlagCases) now call the sharedorphanCaseKeys()detector and fail by name instead of silently skipping/filtering a case key naming a nonexistent script — spot-checked directly in both files. - Every import resolves. Cross-checked every symbol imported by every case module against
_harness.mjs's actualexportlist — all present. - A genuine bug was caught by actually running the harness. Commit
cfcd6089fixes theorchestratetracked-doc guard, which started flaggingtools/__tests__/wave-plan.mjs's own fixture strings as stale prose after the split (the old single-path exclusion no longer covered the new multi-file layout) — real evidence the harness was executed during authoring, not just read.
CI evidence (rubric dimension 15's bar) on the reviewed commit (cfcd6089b7b34c30cb55e25e0a921427ee25d646): Harness Execution = SUCCESS, Harness Lockstep = SUCCESS (validates the recomputed tools/lockstep-declarations.json fingerprints against the real orbit-api rubric), Harness Calibration, Lint, Type Check, Build, Context Budget, Unit Tests = all SUCCESS.
No apps/*, orbit-api, packages/shared/src/types/*, or i18n files changed, so parity, i18n, contract-drift, security, backend-hard-rules, and FEATURES.md are all N/A — none of the five gated subagents fire for this diff.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [INFO] One new narration-style comment, consistent with existing convention
· location:tools/test-tools.mjs:61
· issue:// Loaded after the CLI contract, so --help and a bad argument stage no fixture root.has nohttp(s)://WHY link, so it's outside the rubric's strict allowed set.
· risk: None practically.tools/sits outside the npmworkspacesglob (apps/*,packages/*,eslint-rulesonly), so ESLint'slocal/no-commentsnever runs against it, and_harness.mjs(verbatim-carried from the pre-split file) is already dense with the same narrative-comment style throughout the directory.
· fix: none required — noted for completeness, not actionable.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A (no apps/web or apps/mobile files 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 changes) |
| security-reviewer | N/A (no orbit-api code changed) |
| design-reviewer | N/A (no apps/* or orbit-landing-page UI files changed) |
Validation (CI on commit cfcd6089)
| Check | Result |
|---|---|
| Lint | PASS |
| Type Check | PASS |
| Build | PASS |
Harness Execution (node tools/test-tools.mjs) |
PASS — dimension 15 evidence bar |
Harness Lockstep (node tools/check-lockstep.mjs) |
PASS |
| Harness Calibration | PASS |
| Context Budget | PASS |
| Unit Tests | PASS |
What's good
- The partition rule (shared prelude vs. per-tool module, helper promoted only on real multi-tool use) is followed exactly, independently verified rather than trusted.
- Both previously-silent orphan-key guards are now loud failures with dedicated positive/negative-case coverage.
- Docs (
tools/CONVENTIONS.md,tools/README.md, and the rubric's own dimension 15) were updated in the same commit set to describe the new architecture accurately.
Recommendation
No Critical or High findings. Approve as-is; the single Info note is not actionable and not gated by any CI check.
1 participant
Harness-Lockstep-Peer: 444
ORB-163 section C2.
tools/test-tools.mjsbecomes a thin runner over one case module per tool undertools/__tests__/, keeping the exact filename, the exact exit codes and the same assertion names in the same order. 7,214 lines to 197.The rationale is C4, not collision counting. C2's original "21 of 21 pairs" argument is withdrawn in the specification itself. What survives is this: every slice of a harness ticket must add a harness case, so while all cases live in one file every slice intersects on that file and the whole ticket serialises to one slice. After the split, two slices for two different tools edit two different files.
The intended module boundaries were posted as a comment before the split commit, so the partition rule is reviewable separately from the 7,300 moved lines.
What changed
tools/test-tools.mjskeeps the filename (const SELF = "test-tools.mjs"still excludes the harness from its own coverage enumeration, so the gate cannot recurse into itself) and now owns four things only: the CLI contract,TOOLS_DIR, theCASE_MODULESregistry withINVALID_INPUT, and the three layers.tools/__tests__/_harness.mjsis the shared prelude: everything above the first*Casesdefinition in the old file, plus five helpers that two or more tool modules use (stageWorkerPidMarker,exitedProbePid,budgetRecord,VALID_TICKET_BODY,VALID_ISSUE). Nothing is copied into two modules and no test module imports another test module.merge-sweep.shandmerge-sweep-cov.shshare one module, because they sharemergeSweepCases(file); splitting them would either duplicate 670 lines or create a test-module-to-test-module import.tools/CONVENTIONS.mdline 42 described the file's internals rather than its shape. Rewritten, along with the "a new tool lands with its coverage" paragraph that namedgateCases.tools/README.mdnow states affirmatively that nothing undertools/__tests__/carries a lockstep obligation, with the reason:check-lockstep.mjshardcodes sixREQUIRED_PATHSandreadManifestrejects any manifest declaring a different set, so a seventh entry would fail the checker rather than gate anything..claude/skills/pr-review/rubric.mddimension 15 no longer says coverage lives "intools/test-tools.mjs".tools/lockstep-declarations.json: two fingerprints ofrepository-rubric-dimensionsrecomputed.TOOLS_DIR is injected, never re-derived
TOOLS_DIRwasdirname(fileURLToPath(import.meta.url)). Moving a case body intotools/__tests__/foo.mjswould have made it resolvetools/__tests__, breaking the--helpcontract loop, the invalid-input check, the coverage enumeration and bothexistsSyncguards. That is a guaranteed break, not a risk.So the runner resolves it once and calls
configure({ toolsDir, self })before a single case module is imported._harness.mjsexportsTOOLS_DIR,REPO_ROOTandSELFas liveletbindings thatconfigurefills. The prelude itself is loaded withawait importafter the--helpand bad-argument branches, so those two paths still stage no fixture root, exactly as before the split.Proved by an assertion, not by inspection:
a path resolved inside a case module equals the runner'scomparestoolPath(SELF)against the runner's ownjoin(TOOLS_DIR, SELF)and requires the result to exist on disk.Both skip guards are inverted
There were exactly two
existsSync(join(TOOLS_DIR, ...))guards and both are now failures naming the key, through one shared detector,orphanCaseKeys(caseKeys, toolsDir):tools/used tocontinuewithfailsuntouched, so the suite printedORBIT TOOLS GATE OKand exited 0 while that tool's cases never ran. It now fails naming the key.mergeSweepCliFlagCases, which filtered a missing filename out of its scan rather than failing on it.Four assertions ship in the same commit as those decision paths:
a case key naming a real tools/ script is not an orphan(staged fixture, positive case)a case key naming no real tools/ script is reported by name(staged fixture, negative case, asserts the key is in the output)every registered case key names a real tools/ script(the live guard over the registry)a path resolved inside a case module equals the runner's(the injection)Coverage did not drop, and here is the proof
Every
T(...)andcheck(...)label was read off the AST of the old file and off the runner plus all 26 modules, sorted, and diffed. Not a text grep: an AST walk, so a label inside a staged sub-script template cannot be miscounted.The only difference is the four new cases above.
The same holds at runtime, comparing the PASS/FAIL name list of the full suite before and after on the same machine:
Zero assertions lost, four added, and every remaining name identical.
Verification
Every exit code below was written to a file and read back separately. No pipe touched a status.
node tools/test-tools.mjsRepo-tool appeal: rubric dimension 15 requires a review to cite this execution, and ORB-163's definition of done specifies this invocation as the acceptance criterion. Before the split, at27febbf9: exit0,ORBIT TOOLS GATE OK, 750 assertions. After the split, at this head: exit0,ORBIT TOOLS GATE OK, 754 assertions, zeroFAILlines. The CI Harness Execution job passes on this head too.orchestrate flags: tracked-doc guard reads files and finds no one-ticket-only --single use, withstale uses: tools/__tests__/wave-plan.mjs. That guard scans every tracked text file for stale one-ticket-only--singleprose and excluded exactly one path, the old single-file suite, because that file holds the fixture strings it would otherwise flag. The harness's source is no longer one file, so the exclusion is no longer one path; it now covers the runner and everything undertools/__tests__/, which is the same set as before and not one file more. Reading the diff would not have found it.node .claude/hooks/test-hooks.mjsRepo-tool appeal: the same rubric dimension pairs the two harnesses as one citation. Exit0,ORBIT HOOK PARITY OK.node tools/check-lockstep.mjs --ui-root . --api-root <orbit-api worktree>Repo-tool appeal: the fingerprint recompute is the change, so the checker's exit code is the evidence for it. Exit0,HARNESS LOCKSTEP OK: 6 pairs checked. Before the fingerprint update the same command exited1, naming both stale declarations and printing the two replacements, which is where they came from..claude/wholesale, so every added line of both repositories' diffs was scanned by hand for U+2014: zero.Decisions taken unattended
*Casesdefinition, as the work order's clause 4 says, even where a helper has only one consumer today (the lockstep fixtures, the merge-sweep stubs). The alternative, pushing single-consumer helpers down into their tool module, would serve C4 slightly better but contradicts a binding instruction and would have made the partition a judgement call per helper instead of a rule. Helpers below that line are placed by consumer, with two-or-more consumers promoting into the prelude.merge-sweep.shandmerge-sweep-cov.shshare one module. They sharemergeSweepCases(file), which drives both scripts through the same cases with one flag. The two alternatives were duplicating 670 lines or importing one test module from another; both are worse than one module exportingcasesandcoverageCases.CASE_MODULESarray and loads them withawait importafterconfigure, rather than static imports. Static imports evaluate before the runner body, soconfigurewould run after every module had been evaluated. No module readsTOOLS_DIRat evaluation time today, so static imports would work by accident; dynamic import makes the ordering a property of the code rather than of the current case bodies..claude/namestest-tools.mjsnowhere. Adding it would make every orbit-api review cite a Harness Execution job that does not exist there. The Linear ticket outranks the work order, and ORB-163 asks to "land orbit-api's rubric copy" for C2, so the paired pull request lands the correction the split actually forces on that copy: the paragraph claiming the sanctioned divergences are "backend-only material", when the largest one is the UI-only harness dimension whose fingerprint this pull request recomputes.INVALID_INPUT'sPENDINGmechanism was left alone. A key there naming a script absent from the branch printsPENDINGand does not fail, which is the same shape as the guards C2 inverts. It is deliberately out of scope: the specification scopes the inversion toexistsSync(join(TOOLS_DIR, ...))guards, and this one is a branch-tolerance affordance keyed onscripts.includes(file). It is also currently empty, so nothing is being hidden by it today (zeroPENDINGlines in either full run).REQUIRED_PATHSwas not extended, per the specification, andtools/README.mdnow says so affirmatively so nobody adds a seventh entry later.A process finding, for whoever splits a file next
The documentation commit landed before the split commit, so the module boundaries could be posted as a comment and reviewed before the 7,300 moved lines arrived. That ordering cost a round: the automated reviewer reads per commit, saw a commit whose
tools/README.mdand rubric described atools/__tests__/layout that commit had not created, and filed a correct P1 against it. Resolved on the thread with the successor commit named and the head verified.The rule the next agent should apply: each commit must be self-consistent on its own. Land the split and its documentation in one commit, or land the code first and the documentation second. Reviewer-facing ordering is worth having, but it belongs in a comment, not in the commit sequence.
One defect found on the way, filed as ORB-165
.claude/hooks/forbid-raw-repo-tool-surfacing.mjsblocks everyEditandWritea worker makes to a repository file, because the worker works in an Orca worktree. The hook's own header says artifact checks "skip files under declared repos, where source and documentation references are owned by CI", anddeclaredRepoRoots()builds that list from.claude/orchestrator.json'sreposmap plusHOOK_REPO_ROOT, which in a worktree resolves to$CLAUDE_PROJECT_DIR, the root checkout. SoC:\Users\thoma\orca\workspaces\...matches no declared root, andtools/README.md's catalog row and this suite's own header comment are treated as surfacing raw commands to Thomas.Filed as ORB-165 with a red repro as its first acceptance criterion, rather than fixed here, and that is a deliberate call rather than a scope excuse: the fix lands in
.claude/hooks/forbid-raw-repo-tool-surfacing.mjsplus a case in.claude/hooks/test-hooks.mjs, and every other concurrent slice of ORB-163 adds cases to that same hook harness. Landing it in this pull request would manufacture exactly the concurrent-slice collision C2 exists to remove. The block is also non-fatal: it replaces the tool result with an error after the write has already landed, so it cost rounds here, never correctness.