Gate surfaced repo tool commands (ORB-122) - #633
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea742a0cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the-rule-that-a-tools
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
/pr-review — PR #633 (ORB-122: Gate surfaced repo tool commands)
Recommendation: REQUEST CHANGES
Diff scope: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs (new), .claude/hooks/test-hooks.mjs, .claude/settings.json, CLAUDE.md. No apps/*, packages/shared, or orbit-api touched — parity, i18n, contract-drift, design, security-reviewer, and FEATURES.md dimensions are all N/A for this diff.
High
The appeal is not scoped to the command it appeals — the gate can be bypassed by an unrelated appeal line anywhere in the same message. .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:102-118 (checkRawRepoToolSurfacing), root cause in surfacedCommand (lines 55-89).
checkRawRepoToolSurfacing computes command = surfacedCommand(text, source), which scans top-down and returns on the first raw-tool command it finds (unconditional early returns at 78/84/85/86 — it never looks past the first hit). Separately, appeal = APPEAL.exec(text) searches the entire text for any line matching /^\s*Repo-tool appeal:\s*(\S.*)$/im, with no positional link to command at all.
Concretely: a message containing an appeal line early in the text (e.g. referencing one file/command) followed later by a completely different, unrelated raw command will have that later command detected by surfacedCommand, but the appeal check matches the earlier unrelated line and reports it as "appealed" — emit() then writes the systemMessage and exit(0), letting the actually-surfaced command through with no genuine appeal for it. Verified by hand-tracing:
- Text:
"Repo-tool appeal: this is for tools/foo.mjs\n\nActually just kidding, run node tools/bar.mjs to fix everything." surfacedCommandfinds no command on line 1 (tools/foo.mjslacks the requirednode/npx/.shframing theCOMMANDregex demands), then matchesnode tools/bar.mjs to fix everythingon line 3.APPEAL.exec(text)matches line 1 regardless, returning reason"this is for tools/foo.mjs".- Result:
{appeal: true, command: "node tools/bar.mjs to fix everything", message: "Repo-tool appeal recorded: this is for tools/foo.mjs"}→ hook exits 0. The surfacedbar.mjscommand was never actually appealed.
This defeats the hook's own stated purpose (gating a specific raw command surfaced to Thomas): any text carrying a Repo-tool appeal: line anywhere authorizes whatever raw command the scanner happens to find first, appealed or not, and any commands after the first are never independently evaluated regardless of appeal.
Fix: scope the appeal positionally to the command it's adjacent to (e.g. require the appeal line to immediately follow/precede the matched command), or re-scan after consuming an appealed command so a later un-appealed command still blocks. Add a test with two distinct commands where only one carries an adjacent appeal, and confirm the unappealed one still blocks.
Medium
-
commandInShellFenceis dead code..claude/hooks/forbid-raw-repo-tool-surfacing.mjs:40-53(function),:77-78(call site). Its documentation-fence guard duplicates the outer loop's own fence handling (which alreadycontinues past documentation fences before reaching this call), and any command it could find is also caught by the unconditionalDIRECT_COMMANDcheck later in the same loop body. Removing it changes no observable behavior. -
The "quoted
--helpoutput" exemption doesn't match realistic inline-code markdown, and its test passes for an unrelated reason..claude/hooks/forbid-raw-repo-tool-surfacing.mjs:15-16(DOCUMENTATIONregex) vs. thetest-hooks.mjscase"cc raw-tool: quoted --help output -> 0".DOCUMENTATIONneeds the literal substringhelp output(single space), but the test fixture wraps--helpin backticks (`--help` output), which breaks the substring match — the exemption never actually fires in that test. The test only passes because its command line happens to be prefixed"Usage: ", which independently defeats the anchoredDIRECT_COMMANDmatch via a different path. Real quoted--helpoutput without that incidental"Usage:"prefix would be wrongly blocked, contrary to the PR's stated "false positives cost more than misses" priority. Fix the regex to tolerate a backtick between the words, and add a test that doesn't rely on the"Usage:"prefix to pass.
What's good
stop_hook_active loop-guard is correctly implemented. Documentation-fence tracking and the DOCUMENT_PATH/HELP_PATH artifact exemptions are structurally sound. Most of the added test matrix (skill/agent/ticket/PR-description bodies, ordinary commands, npx-gap messaging) is solid coverage. CLAUDE.md's two edits accurately describe the new hook with no drift. .claude/settings.json wiring is correct (Stop with no matcher, PostToolUse gated to Edit|Write|MultiEdit).
Deferred / not verifiable in this job
- Dimensions covering
apps/*, contract drift, security-reviewer, parity, i18n: N/A, no matching surface in this diff. - Phase 6 (
/validate) skipped per instructions — Build / Unit Tests / SonarCloud run as separate required CI checks on this PR. - Cross-repo dimensions (contract-aligner, orbit-api side of backward-compat guard): not applicable,
orbit-apinot touched and not checked out in this job.
Files referenced: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs, .claude/hooks/test-hooks.mjs, .claude/settings.json, CLAUDE.md.
There was a problem hiding this comment.
/pr-review — PR #633: Gate surfaced repo tool commands (ORB-122)
Recommendation: REQUEST CHANGES (2 High findings)
Scope
Diff touches only .claude/hooks/forbid-raw-repo-tool-surfacing.mjs (new), .claude/hooks/test-hooks.mjs, .claude/settings.json, CLAUDE.md. No apps/*, packages/shared, or orbit-api changes — parity, i18n, contract-drift, DESIGN.md, and backend rules are all N/A; none of the five review subagents fired. CI (Lint, Type Check, Unit Tests, Harness Execution, SonarCloud) is all green, but green here only means the added test matrix doesn't exercise the failure modes below.
This PR carries a prior CI review (on commit 3ea742a0) that flagged one High and two Medium issues; the round-1 fix (008a67b0) added standalone-code-span detection, a settings-parsing test, a CLAUDE.md trim, and a wave-plan timing fix, but diffing 3ea742a0..008a67b0 and hand-tracing current HEAD shows none of the three original findings were actually fixed. The findings below are the same substantive gaps re-verified against the current hook logic, plus one new High.
High
-
Appeal has no positional link to the matched command (
forbid-raw-repo-tool-surfacing.mjs,checkRawRepoToolSurfacing).surfacedCommand(text, source)finds the first raw command anywhere in the whole message; separately,APPEAL.exec(text)scans the entire message for aRepo-tool appeal: <reason>line. If both are present anywhere in the same text, the appeal is treated as authorizing whatever commandsurfacedCommandfound — with no check that the appeal line is adjacent to, or even about, that command. A message containing one legitimate appeal for command A followed later by an unrelated raw command B has B silently pass as appealed. This defeats the gate's own purpose (a targeted, reasoned appeal per surfaced command). -
npxbranch false-positives on ordinary prose starting with "npx" (bothCOMMANDandDIRECT_COMMANDregexes). The npx alternative isnpx(?:\.cmd)?\s+(?:(?:--yes|-y)\s+)?(?:@?[a-z0-9_][a-z0-9_./@-]*)— any lowercase word afternpxsatisfies the "package name" group, so"npx is a great tool for running one-off packages."matches bothDIRECT_COMMAND(line starts with the pattern) andCOMMAND(which then greedily captures the rest of the sentence as the "command"). Traced by hand and confirmed with a standalone regex test against exactly this string plus"npx invocations without --yes will prompt for confirmation."and"npx runs whatever package you name, unlike a pinned devDependency."— all three trip the hook and get blocked as raw commands, despite being plain discussion ofnpx, not an instruction to run it. This is the opposite of the PR's own stated "false positives cost more than misses" design goal, and the added test matrix has no case that would catch it (no test discussesnpxin prose without also intending a command).
Medium
-
commandInShellFenceis redundant, not exercised for anything the main loop wouldn't already catch. It only fires on a fence-opener line and, for a real (non-documentation) fence, the outer loop's own per-line pass reaches the fence's first content line on the very next iteration and detects the same command via the ordinarycommandFrom+DIRECT_COMMANDpath — so its result never differs from what falls out of the main loop one iteration later. It also misfires on closing fence markers (```with no language matchesopening, so a closing fence is treated as if it opens a new one and the function scans forward into the following prose looking for a command) — harmless in the current test corpus only because the first non-blank line after the false "opening" happens to failDIRECT_COMMAND. Per code standard #2 (delete unused code) / #6 (no premature abstraction), this function should either be deleted or given a case where it produces a genuinely different, needed result. -
The "quoted
--helpoutput" exemption is untested for its intended reason.DOCUMENTATIONmatches the literal phrase--help output(orhelp output) as contiguous text. The test's actual line is"The captured `--help` output is:"— the backtick sits between--helpandoutput, so the literal phrase never matches andDOCUMENTATION.teston that line returnsfalse. The test still passes (exit 0), but only because the fenced content is```text(not a shell/bash/sh fence, socommandInShellFenceno-ops) and the one candidate line inside it,"Usage: node tools/wave-plan.mjs --all", failsDIRECT_COMMAND(leading"Usage: "isn't an allowed prefix) and fails theINSTRUCTIONcheck ("Usage"doesn't contain the word"use"as a whole word). So the test is green for an incidental reason unrelated to the feature it claims to verify — a real doc string like"Per the tool's `--help` output, run `node tools/wave-plan.mjs --all` next."would not get the documentation exemption and could still block.
Not verifiable in CI
None — this PR has no orbit-api or cross-repo surface.
Prior review round tracked at 3ea742a0 → 008a67b0; the above supersedes it against current HEAD.
|
Review round 2 is fixed in
Both harnesses are green. ORB-118 was integrated, the required 107-byte context clause was tool-reseeded after EOL normalization, committed blob sizes match the generated baseline, and |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Recommendation: REQUEST CHANGES
Summary
This PR adds a new session hook (forbid-raw-repo-tool-surfacing.mjs) wired to Stop and PostToolUse that blocks raw tools/* / npx commands surfaced outside a skill/agent, plus a large regression suite for it, a generalized settings-hook-path scan, and a deterministic replacement for a flaky wall-clock concurrency assertion in wave-plan.mjs's test. The core hook logic is sound — traced roughly 20 of the ~35 new test cases by hand against the regex/state-machine logic (fence tracking, appeal parsing, document-path exemptions, npx-specific handling) and found no behavioral bugs. The blocking issue is that the PR's own merge with main silently invalidated its self-reported context-budget validation evidence.
Findings
Critical
None.
High
[HIGH] PR's context-budget evidence is stale; the Context Budget guard will very likely fail as configured
· location: CLAUDE.md:22 (the edited /next line), tools/context-budget.json
· issue: The PR's "Final context budget" JSON in the PR body (CLAUDE.md: 18794, enforcedBytes: 22237, deltaBytes: -7) was captured before the branch merged current main, which had meanwhile landed "ORB-118 trim always-loaded context" shrinking CLAUDE.md to 11828 bytes. origin/main's committed tools/context-budget.json is {"bytes": 15208, "CLAUDE.md": 11828, "core.md": 3380}; this PR's actual HEAD has CLAUDE.md at 11935 bytes (the /next line's added annotation is +107 bytes), for an enforced total of 15315 — i.e. +107 over main's current baseline, not the "-7" the PR claims. The PR carries no context:reseed label.
· risk: check-context-budget.mjs reads the baseline from the base branch's committed context-budget.json (main = 15208). With deltaBytes = 15315 - 15208 = +107 > 0, the job's main() returns exit 1 ("Always-loaded context grew ... beyond the committed baseline"). The PR will very likely fail a required gate it claims already passed.
· fix: Either request the context:reseed label with a one-line justification, or shrink the /next annotation to fit back under 11828 bytes, and re-run node tools/check-context-budget.mjs --check against current origin/main post-merge, pasting that output in the PR body instead of the pre-merge transcript.
Medium
[MEDIUM] alternativeFor() falsely claims "no skill exposes this" for tools that already have one
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs (alternativeFor, ~line 90)
· issue: alternativeFor(command) special-cases only wave-plan (redirecting to /next); every other surfaced command — including ones with an existing skill wrapper, e.g. tools/rollup.sh (wrapped by /rollup) — falls through to "No skill currently exposes this capability... describe the skill to build instead."
· risk: A future session surfacing e.g. tools/rollup.sh directly gets told to propose building a brand-new skill for a capability /rollup already provides.
· fix: Widen alternativeFor to check tools/README.md's catalog (or a small map of known tool→skill pairs) before falling back, or soften the fallback wording so it never asserts a negative it hasn't checked.
[MEDIUM] The "closing fence does not scan following prose" test doesn't isolate what it's named for
· location: .claude/hooks/test-hooks.mjs (the closing fence does not scan following prose -> 0 case)
· issue: The fixture's trailing sentence contains the word "internally", which independently matches the DOCUMENTATION keyword list and exempts the line on its own — so this test passes whether or not the fence-state machine correctly resets insideFence on the closing fence.
· risk: The one test meant to cover "prose after a closed fence is still scanned" gives false confidence; a real regression in that specific path has no case that would catch it.
· fix: Add/replace with a case whose trailing line has no independent exemption keyword, e.g. asserting exit 2 on a line like "Run node tools/wave-plan.mjs --all now."
Low / Info
None (per rubric signal gate).
Subagents
No gated subagent fired — diff touches only .claude/hooks/**, .claude/settings.json, CLAUDE.md, and tools/**; no apps/web|mobile, no packages/shared, no orbit-api, no UI files. All N/A: parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer.
Validation
Per task scope, Phase 6 (/validate) and Phase 7 posting were skipped (CI runs Build/Unit Tests/SonarCloud separately). Sandbox restrictions prevented independently re-running node .claude/hooks/test-hooks.mjs / node tools/test-tools.mjs this session; findings rely on static tracing of the diff plus independently reproducible byte-count arithmetic for the context-budget finding (not test execution).
Cross-repo dimensions requiring the orbit-api sibling repo (contract-aligner, the orbit-api side of the backward-compat guard) are not verifiable in this CI job — orbit-api is not checked out here.
What's good
- The hook's regex/state machine is genuinely careful: distinguishes skill/agent/ticket/PR-description bodies from live chat instructions, handles multi-command appeals per-command, and self-exempts its own
.claude/hooks/directory via the same document-path rule rather than a bespoke carve-out. - The
tools/test-tools.mjschange (replacing a wall-clockDate.now() < 3000assertion with direct start/end event instrumentation) is a solid fix for a genuinely flaky test; the asserted bound (peak <= 8) matcheswave-plan.mjs's actualRELATION_FETCH_CONCURRENCY = 8constant. - The generalized
configuredHookPathScan(scanning every configuredsettings.jsonhook path generically instead of a hardcoded list) is a real improvement, proven non-vacuous by the added "renamed hook fixture reports the missing file" case.
Recommendation
Request changes: fix the High finding before merge (request context:reseed or shrink the CLAUDE.md addition, then re-paste a post-merge validation transcript against current main). The two Medium findings are worth a follow-up but don't block merge on their own.
|
Round 3 is fixed in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddd68a3809
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
PR #633 Review (ORB-122: gate surfaced repo-tool commands)
Recommendation: REQUEST CHANGES
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 0 |
| Low / Info | 1 |
High
The new PostToolUse/Stop gate (forbid-raw-repo-tool-surfacing.mjs) has no exemption for a tool's own self-documenting Usage:/Run: comment, so it would block future edits to files that exist today — including one this PR itself edits.
- The hook is wired with matcher
Edit|Write|MultiEdit(no path scope —.claude/settings.json), sowrittenArtifact()rescans the entire file on every write. DOCUMENTATIONonly recognizes phrases likeskill body,agent body,tool help,internally,under the hood,--help ... output/text.DOCUMENT_PATH/HELP_PATHonly exempt.claude/{skills,agents,hooks}/*, ticket/PR-named.md/.txt, and files literally namedhelp*.md/txt/log— not ordinarytools/*.mjssource.- Verified against the actual file content at HEAD:
tools/test-tools.mjs:23—* Run: node tools/test-tools.mjs (exits non-zero on any failure)— this is a file this same PR edits (the concurrency-timing rewrite). Neither this line nor its preceding line matchesDOCUMENTATION, and the file path matches neitherDOCUMENT_PATHnorHELP_PATH, so a futureEdit/Writeto this file trips the gate.tools/new-ticket.mjs:17—* Usage: node tools/new-ticket.mjs --title "<t>" --project "<name>" --body-file - < body.md— same gap.
- This contradicts the PR's own stated scope ("Correct machine-to-machine uses in skills, agents, tickets, PR descriptions, and help output remain allowed") — a tool's self-documenting usage comment is exactly that kind of documentation, and it isn't covered.
- Confirmed by omission: the PR's own
test-hooks.mjsadditions cover skill bodies, agent bodies, ticket bodies, PR descriptions, and--helpoutput as exempt artifact cases, but there is no fixture for an ordinarytools/*.mjssource file carrying aUsage:/Run:JSDoc line — the gap has no regression test either.
Fix: extend DOCUMENTATION to recognize a leading Usage:/Run: label, or exempt commands whose enclosing comment is a /** ... */ JSDoc block (mirroring eslint-rules/no-comments.cjs's own JSDoc allowance), and add regression fixtures pinned to tools/new-ticket.mjs:17 and tools/test-tools.mjs:23.
Info (not blocking)
The new hook's top // narration comment block isn't JSDoc/directive/WHY-URL, but local/no-comments isn't wired to .claude/hooks/**, and every sibling hook (git-guardrails.mjs, etc.) uses the identical style — noted for completeness, not a finding.
What's good
- Careful, well-tested separation of "surfaced to Thomas" vs. machine-to-machine framing; fence-state tracking,
npx-ambiguity heuristic, and per-command same-line appeal binding are all solid, evidenced designs. tools/test-tools.mjs's concurrency-timing rewrite replaces a genuinely flaky wall-clock assertion with real start/end event evidence — a real reliability improvement, not just new coverage.- Three rounds of prior self-review already closed a wide set of false-positive classes, which is why the one gap that remains is a genuine edge case, not a first-pass miss.
- All deterministic CI gates are green (Lint, Type Check, Unit Tests, both Harness Execution legs, Harness Lockstep, Contract Drift, Cross-Platform Parity, Expo SDK Pin, Suppressions Ratchet, CodeQL, GitGuardian); Context Budget is SKIPPED consistent with the
context:reseedlabel.
Deferred: Dimensions 8/9/10/11/13/14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) are N/A — diff touches only .claude/hooks/**, .claude/settings.json, root CLAUDE.md, and tools/**, no apps/*/packages/shared/orbit-api surface. Dimension 15 harness-execution evidence relies on the PR's own CI run (Harness Execution SUCCESS both matrix legs) rather than a re-execution in this session (sandbox blocks git worktree/git checkout/direct node execution here); the High finding above was instead verified by manually tracing the hook's regexes against the actual file content at HEAD.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0182a2493
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Scope: PR #633 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR adds a new Stop/PostToolUse session hook (forbid-raw-repo-tool-surfacing.mjs) that blocks the
assistant from surfacing raw node tools/*, npx *, or tools/*.sh commands to Thomas in chat or in
written artifacts, with a command-scoped Repo-tool appeal: <reason> override. The diff is entirely
.claude/hooks/**, .claude/settings.json, CLAUDE.md, and tools/** (no apps/, packages/, or
orbit-api surface touched), so the platform-parity/i18n/contract/security/design subagents are all N/A.
The detection regex, fence-state machine, appeal binding, and byte-for-byte context-budget arithmetic all
check out under manual trace against the 45+ added test cases and the PR's four rounds of cited executed
output. Two Medium test-coverage/consistency gaps are worth a fast follow-up but do not block merge.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] transcriptAssistantMessage() JSONL fallback has zero test coverage
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: orbit-ui-mobile/.claude/hooks/forbid-raw-repo-tool-surfacing.mjs (transcriptAssistantMessage, writtenArtifact's HELP_PATH branch)
· issue: Every added Stop-event test in .claude/hooks/test-hooks.mjs builds its payload with `stopPayload(text)`, which sets `last_assistant_message` directly. That means `runHook()`'s `typeof input.last_assistant_message === "string" ? input.last_assistant_message : transcriptAssistantMessage(input.transcript_path)` branch always takes the left arm in every test — the JSONL-parsing fallback (multi-record scan, string-vs-array content blocks, malformed-line try/catch, missing-file guard) is never exercised once. Separately, the PostToolUse `HELP_PATH` artifact exemption also has no dedicated artifact-write fixture; the four artifact tests added only cover `DOCUMENT_PATH` (skill/agent/ticket/PR paths).
· risk: Claude Code's actual Stop hook payload (session_id, transcript_path, hook_event_name, stop_hook_active) does not appear to include a synthesized `last_assistant_message` field, which would make `transcriptAssistantMessage()` the real production path on every turn rather than a defensive fallback. `runHook()`'s outer try/catch exits 0 (allow) on any thrown error, so a latent bug here fails open silently — the gate simply stops firing rather than crashing — but that silent-bypass failure mode is exactly what a completely untested "real" path can produce, and it would look identical to "nothing to flag" from the outside.
· fix: Add a fixture that sets `transcript_path` to a real JSONL file (written via the existing `write()` helper) containing assistant records with both string and array-of-text-block `message.content`, omit `last_assistant_message` from the payload, and assert the hook still fires/allows correctly. Add one more fixture for a malformed JSON line in the transcript (proving the per-line try/catch is not silently swallowing the whole file) and one for a missing `transcript_path`. Add one artifact-write fixture whose path matches `HELP_PATH` (e.g. `help-output.md`) to exercise that exemption directly.
· reference: rubric.md dimension 15 ("a new decision path added to a tool that already has coverage... needs its own case")
[MEDIUM] New gate's core logic bypasses this file's own "_lib unit" test pattern
· dimension: 3 (SOLID / clean architecture) and 2 (Dead / stale code)
· location: orbit-ui-mobile/.claude/hooks/forbid-raw-repo-tool-surfacing.mjs:93 (export function checkRawRepoToolSurfacing)
· issue: The other three checked-in gates in this same suite (checkGitCommand/checkGitWorktreeRemove in _lib/rules-git.mjs, checkEfMigrationRawIndex in _lib/rules-source.mjs, checkLinearMutation in _lib/rules-linear.mjs) keep their core predicate in `_lib/rules-*.mjs`, imported directly into test-hooks.mjs's "# _lib unit" section (fast, no subprocess) in addition to the full-process "claude code hooks (real files)" section. This PR's `checkRawRepoToolSurfacing` instead lives inline in the hook adapter file and is `export`-ed, but nothing in the repo imports it (confirmed via `git grep checkRawRepoToolSurfacing` at the PR head — the only three hits are the definition and its two call sites inside the same file). Every one of the ~45 new test cases for this gate pays a full `spawnSync` + node-process-startup cost that the existing three gates avoid for their core-logic assertions.
· risk: Not a functional bug — CI's Harness Execution job is green — but it is an unnecessary export (rule 2) and a real deviation from the pattern this exact file demonstrates three times over, which will make the next contributor either copy the slower pattern forward or have to reconcile two conventions in one file.
· fix: Move `checkRawRepoToolSurfacing` and its helpers into `.claude/hooks/_lib/rules-raw-repo-tool.mjs`, import it into both the adapter (`forbid-raw-repo-tool-surfacing.mjs`) and directly into test-hooks.mjs's "_lib unit" section, and drop the adapter-level `export`.
· reference: CLAUDE.md rule 2 ("Delete unused code immediately... no 'just in case' exports"); rubric.md dimension 3
Low / Info
None posted (signal gate: Low/Info are not posted on a PR review).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — neither packages/shared/src/types/* nor orbit-api changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI: Lint = SUCCESS) |
| Type check | PASS (CI: Type Check = SUCCESS) |
| Tests | PASS (CI: Unit Tests = SUCCESS) |
Harness Execution (tools/test-tools.mjs + .claude/hooks/test-hooks.mjs) |
PASS (CI: Harness Execution = SUCCESS, both required contexts) |
| Harness Lockstep | PASS (CI: SUCCESS) |
| Context Budget | SKIPPED by CI — expected: PR carries the context:reseed label after regenerating the baseline (CLAUDE.md +107 bytes, arithmetic verified by hand below) |
This session could not re-run node .claude/hooks/test-hooks.mjs / node tools/test-tools.mjs directly
(the review sandbox does not grant this session shell write/exec approval), so validation leans on the
PR's own four rounds of pasted executed output plus the independently-run CI "Harness Execution",
"Unit Tests", "Lint", and "Type Check" jobs (all SUCCESS as of this review) rather than a fresh local run.
Byte arithmetic was hand-verified: CLAUDE.md 11683→11790 is a +107 byte delta, matching the added
(repo-tool gate: ... ) suffix's literal length, and tools/context-budget.json's new total
(11790 + 3380 = 15170) matches the committed file exactly.
Deferred — N/A dimensions & files not verdicted
- Parity (#9), i18n (#10), Contract drift (#11), Security/API (#12 backend half), DESIGN.md/AI-slop (#8),
FEATURES.md parity (#14): all N/A — diff touches only.claude/hooks/**,.claude/settings.json,
CLAUDE.md, andtools/**; noapps/,packages/shared, ororbit-apisurface, and no user-facing
feature surface changed. - Local execution of the harness suites: deferred to CI's own green "Harness Execution" run (see
Validation) because this session's sandbox does not permit shell write/exec for this review. - Adversarial skeptic / cross-model second opinion (verification protocol §2): N/A — no Critical/High
finding survived to require one. - All 6 changed files (
forbid-raw-repo-tool-surfacing.mjs,test-hooks.mjs,settings.json,
CLAUDE.md,tools/context-budget.json,tools/test-tools.mjs) received a verdict; nothing else in
scope was left unexamined.
What's good
- The detection engine (fence-state machine, same-line appeal binding, per-command independent appeal
requirement, ambiguous-bare-npx-name heuristic) traces cleanly against every one of the ~45 added
fixtures; manual regex trace found no false-positive/false-negative beyond the two Medium coverage gaps
above. - The
wave-plan.mjsconcurrency test replacement (tools/test-tools.mjs) is a genuine root-cause fix:
it swaps a wall-clock elapsed-time assertion (flaky under CI/Windows child-process startup variance) for
direct start/end event counting with a verified peak-concurrency bound (> 1and<= 8, matching
RELATION_FETCH_CONCURRENCY = 8inwave-plan.mjs) — exactly the kind of deterministic replacement
CLAUDE.md rule 1 asks for. .claude/hooks/test-hooks.mjs's settings-hook-path scan was generalized from a brittle hardcoded
four-string list to a real contract (configuredHookPathScan) with its own regression fixture (a
renamed-hook-path case proving the scan actually catches a break) — good defense against the exact
silent-drift failure mode dimension 15's own preamble warns about.DOCUMENT_PATH's inclusion of.claude/hooks/mirrors the precedent already set by
forbid-raw-linear-mutation.mjs'sGATE_SOURCEself-exemption ("the gate cannot police its own
source"), correctly extended to cover skill/agent bodies too.- Context-budget byte delta is exact and independently re-derivable by hand; the
context:reseedlabel
and CI's skip are consistent and expected.
Recommendation
Approve as-is. The two Medium findings are real but non-blocking test-coverage/consistency gaps —
worth a fast follow-up (or folding into the fix commit if another round happens anyway) rather than
holding up this PR, since the shipped behavior is correct and CI-green across every gate this diff
touches.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 144b9cc2d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the-rule-that-a-tools # Conflicts: # tools/context-budget.json
There was a problem hiding this comment.
/pr-review — PR #633 (ORB-122: Gate surfaced repo tool commands)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
Summary
This PR adds a session-level gate (.claude/hooks/forbid-raw-repo-tool-surfacing.mjs, wired to Stop and PostToolUse) that blocks an agent from surfacing raw node tools/*, npx *, or bare tools/*.sh commands to Thomas in chat or in written artifacts, unless the surfacing line carries its own Repo-tool appeal: <reason>. The diff is entirely infrastructure: the new hook, ~60 test cases in .claude/hooks/test-hooks.mjs plus a refactor of the settings-hook-path test from a hardcoded list to a dynamic scan, settings.json wiring, a one-line CLAUDE.md doc addition, the corresponding tools/context-budget.json baseline bump, and an unrelated-but-bundled tools/test-tools.mjs improvement replacing a wall-clock concurrency assertion in the wave-plan test with direct start/end event evidence. The diff touches no apps/*, no packages/shared, and no orbit-api surface, so most rubric dimensions are N/A.
This review checked prior threads first: a prior claude review already APPROVED this PR at commit a0182a2493, flagging the same Medium finding below as a known, non-blocking gap. Commit 144b9cc2 was pushed after that approval (fix(hooks): scope repo tool artifact checks); it does not touch alternativeFor(), so the finding carries forward unchanged rather than being newly introduced. No Critical/High exists anywhere in the current diff.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] alternativeFor() tells the agent "No skill currently exposes this capability" for commands that already have one
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:144-147
· issue: alternativeFor(command) special-cases only wave-plan (→ "Use /next"); every other matched raw command falls through to "No skill currently exposes this capability. Say that plainly and describe the skill to build instead of giving Thomas the raw command." This is false for commands tools/README.md documents as already skill-backed, e.g. tools/rollup.sh ("Backs the /rollup skill") and tools/worker-watch.mjs ("Backs /watch"). The PR's own test fixture (bareToolSurfacing) exercises exactly the tools/rollup.sh case but only asserts status === 2, never the message text, so the wrong claim ships unguarded.
· risk: When an agent surfaces tools/rollup.sh (or worker-watch.mjs/pr-watch.mjs) to Thomas, this hook tells it a skill needs to be built when /rollup (or /watch) already exists — undermining the gate's own stated purpose and risking duplicate-skill churn or a wrong answer reaching Thomas.
· fix: Derive the command→skill mapping from tools/README.md's "Backs" column, or a small explicit table covering currently skill-backed tools, and reserve the generic "no skill exists" message for commands genuinely uncovered. Add a message-content assertion to the rollup.sh fixture so a regression here is caught going forward.
· status: previously flagged in the prior APPROVE review at commit a0182a2493; still open, non-blocking.
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts, no orbit-api change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or orbit-landing-page/src/** UI file changed |
Validation
| Check | Result |
|---|---|
| Lint / Type check / Tests | N/A — CI wrapper context, /validate phase skipped per workflow instructions |
| Build (api) | N/A — orbit-api not touched |
| Harness Execution (guards.yml) | SUCCESS (live CI status) |
| Context Budget (guards.yml) | SKIPPED — PR carries context:reseed; committed tools/context-budget.json independently spot-checked against CLAUDE.md/core.md byte counts |
Cross-Platform Parity / Suppressions Ratchet / Harness Lockstep / other guards.yml jobs |
SUCCESS (live CI status) |
What's good
- Genuinely thorough test coverage for a regex-heavy text classifier: shell-chain splitting, per-command appeal scoping, fenced-code-block state tracking, documentation-context exemptions, ambiguous bare-
npxhandling, and cross-platform (Windows/POSIX) repo-root path containment are all exercised. - The
settings.jsonhook-path test refactor (hardcoded expected-occurrence list → dynamically-scannedconfiguredHookPathScan) removes a maintenance trap that would otherwise need manual updates on every hook add/remove. - The
tools/test-tools.mjschange replaces a wall-clock-based concurrency assertion with direct start/end event instrumentation bounded against the realRELATION_FETCH_CONCURRENCY = 8pool size — a genuine flakiness fix, not just a refactor. - The PR body's iterative round-by-round log is unusually good evidence hygiene.
Recommendation
Approve as-is. The one Medium finding is a known, previously-approved-over, non-blocking gap that degrades gracefully (the hook still blocks the raw command every time regardless of the guidance text's accuracy) and does not warrant holding up this PR per the signal gate.
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07ae1960cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Scope: PR #633 in thomasluizon/orbit-ui-mobile, diff since the last posted review (commit 872f76e0, APPROVED) through the true current head 07ae1960 ("fix(hooks): bind documentation governors"; fetched via refs/pull/633/head, one commit ahead of the 35134ff6 this session's initial checkout had). PR-owned deltas in that range: 3bf9a4ab ("fix(hooks): bound raw tool classification") and 07ae1960, both touching .claude/hooks/forbid-raw-repo-tool-surfacing.mjs and .claude/hooks/test-hooks.mjs. 07ae1960 reworks the documented branch (splitting documentationText into documentationPrefix/documentationSuffix, adding a DESCRIPTIVE_OWNER guard and instruction-framing checks) and adds two new documentation-instruction fixtures — but does not touch isClearlyDescriptiveNpxMention at all, so it does not affect the Critical finding below, which was independently re-verified directly against the 07ae1960 file content (function body unchanged, still at lines 217-220). The merge commit 35134ff6 itself changes nothing further in the two hook files versus 3bf9a4ab — the other files that merge touched (orchestrator.json, orchestrate/SKILL.md, launch-worker.mjs, nudge-worker.mjs) are unrelated ORB-129 content absorbed from main, not part of this PR's own diff against main.
Recommendation: NEEDS WORK
Summary
This round rewrote the hook's command classifier from a single-match-per-line design to a clause/atom-based parser (splitClauseAtoms, commandContexts) with a much larger fixture corpus and a 1,280-case deterministic fuzz suite, then (in 07ae1960) further tightened the documented exemption to require instruction-framing checks on both the prefix and suffix side of a documentation clause. The refactor is a real improvement in structure and the fuzz suite is genuinely valuable evidence. However, static tracing of the separate npx exemption path (isClearlyDescriptiveNpxMention, untouched by 07ae1960) found a concrete, easily-triggered bypass of the entire gate for npx commands, and a secondary latent bug in the new overlapping-match resolution logic that neither the corpus nor the fuzz suite exercises.
Findings
Critical
[Critical] isClearlyDescriptiveNpxMention's "as/about/regarding" prefix check exempts real imperative npx commands, defeating the gate
· dimension: 1. Correctness (this is also the literal mechanism ORB-122 exists to build)
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:217-220 (confirmed unchanged at true PR head 07ae1960)
· issue: isClearlyDescriptiveNpxMention returns true (exempt, do not block) whenever the text immediately preceding an npx mention ends in as|about|regarding|describes?|explains?|tells?|mentions? (line 219), before any check of the token-level confirmation state (--yes, -y, =value, quoted arg) that the rest of the function computes, and without consulting hasInstructionFraming/instructionFramed at all — that gating is only wired into the separate documented branch (and, as of 07ae1960, tightened further there), not into this one. For chat text "You should run this as npx --yes @orbit/cli deploy." (plain prose, no fence, no backticks, no unmatched quotes so context.ambiguous is false): splitClauseAtoms yields one prose atom for the whole sentence; commandContexts produces prefix = "You should run this as " for the npx match; isClearlyDescriptiveNpxMention hits the line-219 regex and returns true immediately; back in surfacedCommands, !context.ambiguous && (documented || descriptiveNpx) is true → the command context is dropped via continue; surfacedCommands returns []; checkRawRepoToolSurfacing returns null — no block, no appeal prompt, nothing recorded. A confirmed (--yes) npx invocation, phrased as an ordinary imperative sentence, reaches the user with zero gate involvement. Independently re-traced and confirmed by a second pass (subagent skeptic check): file content matches, trace holds, and no fixture in test-hooks.mjs at this commit (including the two fixtures 07ae1960 added) combines imperative "run/should run" framing with an "as npx" construction — the only existing "as npx" fixture ("The package is invoked as npx cowsay.", expected status 0) is genuinely descriptive prose, not an instruction, so this exact interaction is untested.
· risk: This is precisely the failure mode ORB-122 was opened to close — a raw, confirmed shell command reaching Thomas in chat without going through a supported skill and without a recorded appeal reason. The trigger requires no adversarial phrasing, just an ordinary sentence pattern ("run this as", "do it as", "invoke it as") that an agent could plausibly produce unprompted while trying to explain what a command does.
· fix: Gate the line-219 (and, for the same reason, line-220) early-return the same way the documented branch now is: e.g. if (!hasInstructionFraming(prefix) && /\b(?:as|about|regarding|...)\s*$/i.test(prefix)) return true, i.e. require the sentence NOT already carry instruction framing before treating a trailing "as/about/…" as merely descriptive. Add a corpus fixture pairing imperative framing with an "as npx --yes …" construction (status 2) alongside the existing descriptive-prose control, and fold at least one such case into the fuzz fuzzFrames/fuzzBoundaries matrix so a future regex change can't silently reopen this.
· reference: CLAUDE.md rule 1 (root cause) / TESTING.md Harness Execution — the rubric's dimension 15 requires new decision paths to carry their own test case, which this one does not.
· Second opinion: UNAVAILABLE — opencode is not installed in this environment, so /second-opinion could not run; the finding stands as verified by the adversarial skeptic pass and by direct re-inspection of the true PR head (07ae1960) performed after discovering the initial checkout was one commit stale.
High
None.
Medium
[Medium] commandMatches's overlap filter compares each candidate to the previous array entry, not the previous kept match, mis-slicing three-or-more overlapping hits
· dimension: 3. SOLID / clean architecture (correctness of a shared helper) + 15. Harness coverage
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:51-59
· issue: matches.filter((match, index) => index === 0 || match.index >= matches[index - 1].end) checks each candidate against matches[index - 1] — the immediately preceding element in the sorted array — even when that preceding element was itself excluded (rejected) for overlapping something earlier. For an input like node --require tools/npx-a.mjs --loader tools/npx-b.mjs tools/wave-plan.mjs, NODE_TOOL_COMMAND (now trimmed to match only through the required trailing tools path, per this same commit) greedily spans the whole string via its flag-value alternation ([ \t]+[^\\s]+), producing one outer match A = (0, end-of-line). NPX_COMMANDseparately matches the literalnpxsubstring inside bothnpx-a.mjs (B1, nested, correctly excluded against A) and npx-b.mjs (B2, later in the string). B2is checked againstmatches[index-1] = B1(already excluded), not against the still-openA, so B2.index >= B1.endis true andB2is wrongly kept even though it lies entirely insideA. The two survivors [A, B2]are then run throughcommandContexts, where A's captured "command" text gets truncated to stop at B2's start (chopping off the real trailing path), and a bogus second "command" is derived starting mid-token at B2. One genuine command line ends up reported/appealable as two mis-sliced ones. · risk: A single legitimate node invocation with a flag value that happens to contain the substring "npx" could require two separate appeals instead of one, and/or display a garbled, truncated command string in the block message — a real, if narrow, false-positive/UX defect in newly-added code that the fuzz suite (which only chains two whole separate commands via &&/;/period, never nests a third overlapping match inside a wider one) does not exercise. This fails toward over-blocking rather than a silent pass, so it is not a gate-defeating bug like the Critical above, but it is a genuine logic error in shared interval-resolution code that will resurface the next time these regexes are extended. · fix: Track the end of the last **accepted** match instead of the raw array predecessor, e.g. accumulate into a result array and compare each candidate against result.at(-1)?.endrather thanmatches[index - 1].end. · reference: CLAUDE.md rule 1 (root cause) / rubric dimension 15 ("a new decision path... needs its own case, not an extension of an existing assertion"). · Note: I could not execute Node in this review session (the sandbox required interactive approval for every nodeinvocation, including a trivialnode -e` smoke test, and that approval was unavailable) so this is a static trace, not a dynamic repro. The trace was done carefully against the exact regex definitions and control flow in the diff; I'd rate confidence as high but flag the lack of an executed repro explicitly per the "no red-capable command" standard.
Low / Info
None posted (Signal gate: Low/Info are not posted as PR-review findings).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file in this diff |
| i18n-syncer | N/A — no user-facing strings / packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts / orbit-api change; orbit-api sibling repo is also not checked out in this CI job regardless |
| 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: SUCCESS) |
| Type check | PASS (CI: SUCCESS) |
| Tests | PASS (CI: "Unit Tests" SUCCESS) |
| Build (api) | N/A — not touched |
Harness Execution (tools/test-tools.mjs + .claude/hooks/test-hooks.mjs) |
Not independently re-confirmed on the true head 07ae1960 in this session (local Node execution required interactive approval that was unavailable) — confirm this CI job is green on 07ae1960 before merge. |
Per the caller's scope, /validate was not re-run locally in this session (delegated to CI), and this decision is posted directly rather than left as a standalone report.
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A — diff touches no
apps/*UI file. - Dimension 9 (Parity): N/A — same reason.
- Dimension 10 (i18n): N/A — no locale/string changes.
- Dimension 11 (Contract drift): N/A — no shared-type/DTO changes; orbit-api sibling repo is not checked out in this session regardless, so that side is not independently verifiable here.
- Dimension 13 (Backend hard rules): N/A — orbit-api untouched.
- Dimension 14 (FEATURES.md parity): N/A — no user-facing feature surface change; this is internal agent tooling.
- Harness Execution (dimension 15): not independently re-run against
07ae1960in this session — confirm green on that exact head before merge. tools/context-budget.json,.claude/settings.json,CLAUDE.md,tools/test-tools.mjs: unchanged since the last APPROVED review (commit872f76e0) — not re-reviewed here, per the caller's instruction to focus on the diff since the last review.- Every file actually in the reviewed diff (
forbid-raw-repo-tool-surfacing.mjs,test-hooks.mjs) received a verdict above; nothing in that diff was left unverdicted.
What's good
- The move from a single regex-match-per-line model to clause/atom-aware parsing (
splitClauseAtoms,fencePairs,commandContexts) is a real structural improvement over the previous ad hoccommandMatch, and correctly fixes several genuine prior gaps (contraction quotes, drive-letter colons, filename-extension periods not splitting clauses, fence-pair ambiguity failing closed). 07ae1960's split ofdocumentationTextinto a prefix/suffix pair with an explicitDESCRIPTIVE_OWNERguard closes a real hole in the previous "documented" exemption (a documentation clause on either side of the command, only when not itself instruction-framed) and ships with matching new fixtures.- The 1,280-case deterministic fuzz suite (
clauseFuzzCases) is a strong, concrete piece of harness evidence exactly matching what dimension 15 asks for, and the reported 1,280/1,280-block, 0-unexpected-pass result (if it holds under CI) is meaningful coverage for the scenarios it was designed to probe (documentation-clause laundering across placements/boundaries/layouts/frames/separators). - The explicit fail-closed design note added to the top-of-file comment and the block message ("Ambiguous command context is blocked by design...") correctly documents intent for the ambiguity-handling paths that do work as designed.
Recommendation
Fix the isClearlyDescriptiveNpxMention bypass (Critical) before merge — this is the one finding that must not ship, since it silently defeats the feature the PR exists to add for a whole, natural class of phrasing, and it survived the latest commit (07ae1960) untouched. Fix or at least add a tracked follow-up ticket for the commandMatches overlap-filter bug (Medium) — lower urgency since it fails toward over-blocking, not under-blocking, but it is a real latent defect in shared logic. Confirm the Harness Execution CI run is green on 07ae1960 (the true current head) before merging.
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Scope: PR #633 in thomasluizon/orbit-ui-mobile
Recommendation: NEEDS WORK
Summary
This PR adds a new Stop/PostToolUse session hook (forbid-raw-repo-tool-surfacing.mjs, 516 lines) that blocks the assistant from surfacing raw node tools/*, tools/*.sh/.ps1, or npx commands to Thomas in chat or written artifacts, nudging toward the equivalent skill instead, with an inline Repo-tool appeal: <reason> escape hatch. It ships with an unusually large, well-organized test suite (735+ new lines: a hand-written corpus, a 1,280-case deterministic clause fuzz, transcript-fallback tests, and artifact-scope tests) and matching CLAUDE.md/settings.json/context-budget updates. The engineering discipline across the PR's eight documented review rounds is genuinely strong. However, manual tracing of the classifier (verified by two independent adversarial skeptic passes, since node execution was unavailable in this review sandbox) found two concrete gaps in the exact mechanism the PR exists to deliver: a content-independent prefix bypass that silently defeats the gate for any command type, and a regex shape in the hot path that is structurally a textbook ReDoS pattern with no test or mitigation. Diff is .claude/ + tools/ + CLAUDE.md only — no apps/*/packages/shared/orbit-api surface, so most rubric dimensions are N/A.
Findings
Critical
None.
High
[High] isClearlyDescriptiveNpxMention silently allows any command type through a content-independent connector-word prefix
· dimension: 1 (Correctness) — the hook's own stated design principle at the bottom of checkRawRepoToolSurfacing's block message is "Ambiguous command context is blocked by design: a false block can be appealed, while a false pass disables the gate."
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:217-224 (function), called unconditionally for every matched command at :346
· issue: isClearlyDescriptiveNpxMention(command, ctx) is called for every command type (node tools/*.mjs, tools/*.sh/.ps1, and npx), not just npx despite its name. Its second line, if (/\b(?:as|about|regarding|describes?|explains?|tells?|mentions?)\s*$/i.test(prefix)) return true, fires and short-circuits before the function ever checks whether the command is even npx (tokens[0] test on line 222) and before npxOptionStates gets a chance to flag dangerous flags like --yes. Confirmed by two independent skeptic traces through commandContexts/surfacedCommands: for the plain, fence-free chat sentence "You can think of it as node tools/wave-plan.mjs --all", context.ambiguous is false, documented is false (unrelated DOCUMENTATION regex doesn't match), and descriptiveNpx alone causes surfacedCommands to skip the command — checkRawRepoToolSurfacing returns null, the Stop hook exits 0, nothing is blocked or logged.
· risk: Any raw command — including a flag-bearing npx --yes <package> or a node tools/*.mjs instruction — silently bypasses the entire gate whenever it's phrased with one of 7 common English connector words ("as", "about", "regarding", "describes/explains/tells/mentions") anywhere earlier in the same clause. This is exactly the "false pass disables the gate" failure the hook's own design note calls out as the one to avoid, and it is not a contrived edge case — "think of it as node tools/x.mjs" or "described as npx --yes ..." are natural phrasings a model could produce unprompted.
· fix: Gate the "as/about/regarding/…" prefix check on tokens[0] actually being npx (move the if (!/^npx.../.test(tokens[0]))return false check above the prefix check), and additionally require the npx invocation itself to be flag-/argument-free (i.e., still run it through npxOptionStates before granting the descriptive exemption) so a confirmed/flagged npx call can never be waved through by a connector word alone. Add a corpus case for "You can think of it as node tools/wave-plan.mjs --all" (expect block) and "...as npx --yes @orbit/cli check" (expect block) to RAW_TOOL_REVIEW_CORPUS.
· reference: hook's own design note (forbid-raw-repo-tool-surfacing.mjs:411, the "Ambiguous command context is blocked by design" message); CLAUDE.md rule 1 (root-cause, not a defensive carve-out that reopens the hole it's meant to close).
[High] NODE_TOOL_COMMAND's quoted-path alternatives are a textbook ReDoS shape, unmitigated and untested, in a hook that runs on every session turn
· dimension: 1 (Correctness) / 15 (Harness — new decision path with no coverage)
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:19-20
· issue: The double- and single-quoted alternatives inside NODE_TOOL_COMMAND contain (?:[^"\r\n]+[\/])/(?:[^'`\r\n]+[\\/])*`. The inner `[^"`\r\n]/[^]for the "no-slash" run so each character deterministically belongs to one alternation branch (mirroring the already-safe unquoted alternative's disjoint-class approach), or drop the interior segment-walking entirely and just match"[^"`\r\n]*tools[\\/][a-z0-9_./\\-]+\.(?:mjs|cjs|js|ts)"` with a bounded `[^"`\r\n]prefix (no nested+/ compounding). Before merging, add a timing-bounded test (node "tools" + "/a".repeat(N)variants at increasing N with no closing quote / notools literal) asserting the match completes within a fixed budget, per dimension 15's "new decision path needs its own case." · reference: dimension 15 (tools/CONVENTIONS.md` / Harness Execution job — a new regex branch with no coverage of its own); OWASP ReDoS (CWE-1333).
Medium
None beyond what's folded into the High fixes above (the missing test cases are called out inline as part of each fix, not duplicated here per the Signal gate).
Low / Info
None posted (Signal gate: Low/Info are not posted on a PR review).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts or orbit-api changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI: PR Tests / Lint) |
| Type check | PASS (CI: PR Tests / Type Check) |
| Tests | PASS (CI: PR Tests / Unit Tests, Guards / Harness Execution) |
| Build (api) | N/A — orbit-api not touched |
Per the CI-adaptation instructions for this workflow, Phase 7 (/validate) was skipped in favor of this PR's own required checks, all of which are green as of this review (gh pr view --json statusCheckRollup): Lint, Type Check, Unit Tests, Harness Execution, Harness Lockstep, Context Budget (skipped — no reseed needed beyond what's declared), Cross-Platform Parity, Design Token Guard, Contract Drift, Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, CodeQL, SonarCloud all report SUCCESS.
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A, no
apps/*UI files in this diff. - Dimension 9 (Parity): N/A, no
apps/web/apps/mobilefiles in this diff. - Dimension 10 (i18n): N/A, no user-facing/locale strings in this diff.
- Dimension 11 (Contract drift): N/A, no shared types / DTOs in this diff.
- Dimension 12 (Security, API-side) / 13 (Backend hard rules): N/A, no
orbit-apicode checked out or touched in this CI job. - Dimension 14 (FEATURES.md parity): N/A, this is internal session-tooling/dev-workflow behavior, not an end-user product feature surface.
- Every changed file (
forbid-raw-repo-tool-surfacing.mjs,test-hooks.mjs,settings.json,CLAUDE.md,tools/context-budget.json,tools/test-tools.mjs) was read in full and given a verdict; the two High findings above are the only ones that survived an adversarial skeptic pass (each was independently traced by a separate skeptic subagent instructed to default to "refuted" — both confirmed). - Dynamic execution (
node .claude/hooks/test-hooks.mjs, timing repro for the ReDoS finding) could not be run in this CI review sandbox — relied on the PR's own extensive round-by-round pasted execution output plus the greenHarness Execution/Unit TestsCI checks as external verification instead of re-running locally.
What's good
- The pure-function/thin-adapter split (
checkRawRepoToolSurfacingas the testable core,runHook/emitas the I/O shell) is exactly the shape the rest of.claude/hooks/already uses — good consistency. - The appeal mechanism (
Repo-tool appeal: <reason>bound to the same physical line/segment, one appeal per chained command) is a thoughtful, minimal escape hatch that avoids the gate becoming a hard wall. - Test discipline is genuinely excellent for this class of change: a 1,280-case deterministic clause fuzz (4 placements × 5 boundaries × 4 layouts × 2 doc phrases × 2 frames × 4 separators) plus transcript-fallback and artifact-scope coverage, with the PR body carrying real pasted output across eight iterative hardening rounds.
tools/context-budget.json's +107 bytes reconciles exactly with theCLAUDE.mddiff (12105 - 11998 = 107) — no unexplained reseed padding.declaredRepoRoots()/isRepoArtifact()correctly handle both Windows and POSIX path semantics for the artifact-scope exemption, with a fail-closed catch around the orchestrator-config read.
Recommendation
Fix the two High findings before merge: (1) gate the connector-word descriptive-mention exemption behind an actual npx-and-flag-free check so it can't wave through arbitrary commands, and (2) remove the ReDoS-shaped nested quantifier from the quoted-path regex alternatives (or add a proven timing bound) since this hook runs unconditionally on every session turn. Both are concrete, scoped fixes to code this PR itself introduces — not a rewrite of the surrounding design, which is otherwise sound and unusually well-tested.
* chore(tools): reject acceptance criteria with no provable finish line check-ticket.mjs now fails an acceptance criterion that quantifies over an open set (every/all/any/each with nothing in the same criterion bounding it) or trails off into an unnamed remainder (etc., and so on, ellipsis). Measured cause: ORB-122 (PR #633) took 24 review rounds and 12 hours because "block every phrasing an agent could emit" is not a set anyone can enumerate, so every round the reviewer legitimately found one more member and was right. ORB-106 (PR #625) shows the same shape at 32 reviews. A criterion that names its enumeration (a count, a named list, a file, or a backticked command) has a finish line; one that does not cannot be proven done by anyone. The gate runs at every point a ticket already passes through: /ticket and /feature on the draft, /orchestrate again per ticket at wave-plan time, and the four /audit-* skills plus /prod-readiness on what they propose. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC * fix(tools): tie the bound to the clause its quantifier is in Review finding on PR #638: BOUNDED_BY searched the whole criterion, so an unrelated token rescued an unbounded claim. Measured bypass, which the gate accepted before this commit and rejects after it: "every phrasing a worker could emit is blocked and the command exits 1" The stray 1 satisfied the bound while the unprovable finish line survived untouched. The bound now has to appear in the same clause as the quantifier. Code spans and file paths are masked before the split so a backticked command containing dots is not cut in half by its own extension. Six measured cases through the real tool, all correct: the bypass above and "...blocked, see tools/check-ticket.mjs" reject; "every phrasing rejected by `node tools/check-ticket.mjs` is blocked", "The three docs each carry a WHY note" and "all 5 call sites route through the shared reader" pass; "any single check failing produces a FAIL row" rejects. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC * fix(tools): accept a bound stated before its quantifier, and hyphenated compounds Two precision defects found by running the gate against the three ticket bodies still queued in this run, before their workers start. ORB-107: "A clean environment produces an all-PASS table and exit 0" was rejected because "all-PASS" is a hyphenated compound adjective, not a quantifier. The quantifier now requires no following hyphen. ORB-120: "`design-specialist.md` and `product-manager.md`, which currently declare no `model:`, each gain one" was rejected because the two named files sit in clauses BEFORE the one holding "each". A bound now counts when it appears in the quantifier's own clause or any clause preceding it, never in a clause after it, which is exactly the direction the PR #638 review bypass exploited ("...is blocked and the command exits 1"). Verdicts after this commit, via --issue against the live bodies: ORB-89 clean, ORB-107 "ticket ok", ORB-120 clean. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Also fixed the review body's overlap finding in fc54527. commandMatches now compares each candidate with the last accepted interval, and the executed regression proves the nested npx-looking paths remain one full command and need one appeal. node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs both pass on this head. |
|
@codex review |
…the-rule-that-a-tools
|
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". |
|
Merged current origin/main on 1ebf7af. node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs both pass on this exact head. |
|
@codex review |
…the-rule-that-a-tools
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Scope: PR #633 in thomasluizon/orbit-ui-mobile (head 1ebf7af0)
Recommendation: NEEDS WORK
Summary
This PR adds .claude/hooks/forbid-raw-repo-tool-surfacing.mjs, a Stop/PostToolUse session hook that blocks raw node tools/*, tools/*.sh/.ps1, and npx commands from being surfaced to Thomas, plus a large, well-built test corpus (hand-written cases, a 1,280-case deterministic clause fuzz, transcript-fallback and artifact-scope tests) and the matching CLAUDE.md/settings.json/context-budget updates. The engineering across eight documented review rounds is strong, and the diff touches only .claude/ + tools/ + CLAUDE.md (no apps/*/packages/shared/orbit-api), so most rubric dimensions are N/A. A manual structural trace of the classifier (node execution was not available in this review sandbox, same limitation the prior review round hit) found that the two High findings from the immediately preceding review round (submitted 2026-07-28T15:22:16Z) are still reproducible on the current head — the intervening fix commit (fc54527c, "close classifier edge cases") narrowed the wording of the connector-word exemption but did not gate it on command type, and did not touch the flagged regex at all. A third, new gap was found in the same pass.
Findings
Critical
None.
High
[High] The "descriptive mention" exemption still waves through non-npx commands, not just npx — the specific gap the prior review flagged is narrowed but not closed
· dimension: 1 (Correctness) — contradicts the hook's own design note: "Ambiguous command context is blocked by design: a false block can be appealed, while a false pass disables the gate."
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:32-33 (DESCRIPTIVE_NPX_PREFIX), :228 (called before the npx-type check at :230-231), reached unconditionally for every command type at :379
· issue: isClearlyDescriptiveNpxMention is invoked for every matched command (node tools/*.mjs, tools/*.sh/.ps1, and npx) via descriptiveNpx at line 379, despite its name and intent. DESCRIPTIVE_NPX_PREFIX.test(prefix) (line 228) returns early — before line 230's tokens[0] === "npx" check ever runs — so it grants the exemption regardless of what the matched command actually is. Traced character-by-character: for the plain chat sentence "The tool described as node tools/wave-plan.mjs --all is deprecated.", the extracted prefix is "The tool described as ", which matches DESCRIPTIVE_NPX_PREFIX (the + tool + .* + as + $). isClearlyDescriptiveNpxMention returns true without ever inspecting that the command is node tools/wave-plan.mjs, not npx. Back in surfacedCommands, context.ambiguous is false and documented is false (the unrelated DOCUMENTATION regex doesn't match this sentence), so descriptiveNpx alone causes the command to be skipped — checkRawRepoToolSurfacing returns null and the Stop hook exits 0.
· risk: Same failure class the prior review round already flagged as High (2026-07-28T15:22:16Z review), just needing marginally more specific phrasing now: any raw command — node tools/*.mjs, tools/*.sh, tools/*.ps1 — silently bypasses the entire gate when phrased as "the/this/that {package|command|tool|option|flag|example|implementation} … {described/mentioned/explained} as {command}". This is a natural, non-adversarial phrasing an assistant could produce unprompted, and it is untested: every existing corpus fixture for DESCRIPTIVE_NPX_PREFIX/DESCRIPTIVE_NPX_CLAUSE (e.g. "The command is documented as npx --yes @orbit/cli deploy.", "The package is described as npx --yes @orbit/cli deploy.") uses an npx command, so nothing in the corpus exercises this exemption against a node tools/* or tools/*.sh command, which is exactly where the hole is.
· fix: Move the tokens[0] npx-type check (currently line 230-231) above the DESCRIPTIVE_NPX_PREFIX/DESCRIPTIVE_NPX_CLAUSE checks (lines 228-229), so the descriptive exemption can only ever apply to an actual npx invocation, per the prior review's exact suggested fix. Add RAW_TOOL_REVIEW_CORPUS cases for "The tool described as node tools/wave-plan.mjs --all is deprecated." and "The command mentioned as tools/rollup.sh runs nightly.", both expecting status: 2.
· reference: hook's own design note (forbid-raw-repo-tool-surfacing.mjs:453); CLAUDE.md rule 1 (root-cause fix, not a partial narrowing that leaves the original hole open for other command types); dimension 15 (new/changed decision path needs its own corpus case).
[High] NODE_TOOL_COMMAND's quoted-path alternatives remain an unmitigated, untested catastrophic-backtracking shape — unchanged since the prior review flagged it
· dimension: 1 (Correctness) / 15 (Harness — no coverage for this decision path)
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:19-20
· issue: The double- and single-quoted alternatives contain (?:[^"\\r\n]+[\/])/(?:[^'\`\r\n]+[\/]). The inner [^"\`\r\n]/[^'\`\r\n]character class is a strict superset of the mandatory trailing[\/]each repetition ends on (it excludes only the quote/backtick/CR/LF, not/or`), which is the textbook (A+B)* overlapping-quantifier shape that causes exponential backtracking in V8's Irregexp engine on a non-matching input. The third, unquoted alternative uses [a-z0-9_.-]+ instead, which is disjoint from [\\/], so it is not vulnerable — the bug is scoped to the two quoted forms only, and fc54527c did not touch this regex.
· risk: This regex runs unconditionally on every assistant Stop message and every Write/Edit/MultiEdit new-string in every session in this repo (both Stop and PostToolUse are blocking hooks). A long, slash-heavy quoted path in ordinary technical prose (a Windows path in an error message, a long glob discussion) that happens not to end in a literal tools/*.{mjs,cjs,js,ts} segment can force the engine through an exponential number of partitions before concluding no match — hanging or badly stalling the hook, and therefore the session. No length cap, timeout, or worker-thread isolation exists anywhere in the file.
· fix: Same fix the prior review proposed and that is still unapplied — make the "no-slash-yet" run disjoint from the mandatory separator, e.g. [^"\\r\n/\]*instead of[^"\`\r\n]+`, mirroring the already-safe unquoted alternative's disjoint-class approach. Add a bounded-time corpus case (a long non-matching quoted slash run) asserting the match completes within a fixed budget.
· reference: dimension 15 (a regex branch with no timing coverage of its own); OWASP ReDoS (CWE-1333); same finding, still open, from the 2026-07-28T15:22:16Z review round.
Medium
[Medium] npm run <script> aliases for tools/*.mjs scripts are a complete, untested bypass of the whole gate
· dimension: 1 (Correctness) — the problem statement in the PR body ("repo tooling could be handed to Thomas as a shell instruction even though skills and agents are the supported interface") is not actually closed for this real invocation surface.
· location: .claude/hooks/forbid-raw-repo-tool-surfacing.mjs:19-23 (NODE_TOOL_COMMAND, TOOL_SCRIPT_COMMAND, NPX_COMMAND — none match an npm run/npm exec prefix); root package.json scripts.surfaces:manifest = "node tools/surface-manifest.mjs", scripts.surfaces:capture = "node tools/capture-surfaces.mjs", scripts.redesign:coverage = "node tools/redesign-coverage.mjs"
· issue: The classifier only matches a literal node/npx/bare-script prefix. npm run surfaces:manifest (or surfaces:capture, redesign:coverage) is a real, already-defined package.json alias that runs the exact same tools/*.mjs script, but contains none of the three literal prefixes, so it passes through completely undetected — confirmed by tracing NODE_TOOL_COMMAND/TOOL_SCRIPT_COMMAND/NPX_COMMAND against the string "npm run surfaces:manifest": no match. The existing corpus only asserts that "npm run lint" (an ordinary, non-tooling script) passes through — there is no fixture distinguishing that from an npm run alias that IS a raw tool invocation.
· risk: An assistant recommending npm run surfaces:manifest — arguably the more idiomatic phrasing for a defined package script, so if anything more likely to be produced than the raw node tools/... form — hands Thomas the exact raw-tool capability this gate exists to prevent, with zero detection or corrective message.
· fix: Add an NPM_RUN_TOOL_COMMAND pattern (or resolve npm run <script>/npm exec <script> against package.json's scripts map at hook-load time and flag any alias whose value matches the existing NODE_TOOL_COMMAND/TOOL_SCRIPT_COMMAND patterns) and add corpus cases for "Run npm run surfaces:manifest" (expect block) alongside the existing "npm run lint" (expect pass) control.
· reference: dimension 15 (an uncovered command-surface, same bar as the two High findings above, downgraded here only because it is outside the PR's explicitly enumerated node tools/* / npx * / tools/*.sh scope).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts or orbit-api changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI PR Tests / Lint, per gh pr checks) |
| Type check | PASS (CI PR Tests / Type Check) |
| Tests | PASS (CI PR Tests / Unit Tests, Guards / Harness Execution) |
| Build (api) | N/A — orbit-api not touched |
node .claude/hooks/test-hooks.mjs and node tools/test-tools.mjs could not be re-run in this review sandbox (write access to .claude/hooks/** and shell execution of node <script> both require approval this session cannot grant). Relying on the PR's own extensive round-by-round pasted transcripts plus the green CI checks above as external verification. Both High findings above concern code paths with no existing corpus coverage, so a green harness does not contradict either finding — it confirms the gap in coverage that each fix item asks for.
Deferred — N/A dimensions & files not verdicted
- Dimensions 8, 9, 10, 11, 12/13, 14: N/A — no
apps/*UI, no web/mobile files, no i18n strings, no shared types/DTOs, noorbit-apicode, and no end-user feature surface in this diff. - Every changed file (
forbid-raw-repo-tool-surfacing.mjs,test-hooks.mjs,settings.json,CLAUDE.md,tools/context-budget.json,tools/test-tools.mjs) was read in full and given a verdict. - Dynamic execution / timing repro for the ReDoS finding could not be run in this sandbox (approval-gated); the finding rests on regex structure analysis, consistent with how the prior review round reported the same limitation.
What's good
- The pure-function/thin-adapter split (
checkRawRepoToolSurfacingas the testable core,runHook/emitas the I/O shell) matches the rest of.claude/hooks/. - The appeal mechanism (
Repo-tool appeal: <reason>, one per physical command, chain-aware) is a well-considered escape hatch. tools/context-budget.json's +107 bytes reconciles exactly with theCLAUDE.mddiff.- The
tools/test-tools.mjswave-plan concurrency test rewrite (wall-clock → event-log based) is a genuine flakiness fix, well-justified in the PR body. declaredRepoRoots()/isRepoArtifact()correctly handle Windows and POSIX path semantics with a fail-closed catch.
Recommendation
Both High findings are narrow, mechanical fixes to code this PR itself introduces, not a design rewrite: (1) reorder isClearlyDescriptiveNpxMention so the npx-type check runs before the descriptive-prefix exemption, and (2) make the quoted-path regex's "no separator yet" character class disjoint from the separator it's paired with. Add the two corpus cases named above for each, plus the npm run Medium if there's room in the same pass. Given this PR already reworked this exact function in fc54527c to respond to the same review finding, a fix here is very achievable — the remaining gap is that the reorder itself wasn't applied.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Merged current origin/main on a61a26c. node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs both pass on this exact head. |
|
@codex review |
|
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". |
There was a problem hiding this comment.
Review Complete
Scope: PR #633 (feature/orb-122-gate-the-rule-that-a-tools → main)
Recommendation: REQUEST CHANGES
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 0 |
| Low / Info | 0 (not posted per signal gate) |
High
isClearlyDescriptiveNpxMention exempts a common bare-command shape from the new repo-tool gate
- Location:
.claude/hooks/forbid-raw-repo-tool-surfacing.mjs:226-248(isClearlyDescriptiveNpxMention) - Issue: For a bare
npx <package>invocation with no flags, no--yes, no trailing arguments, and a package name containing none of. / @ -(e.g.npx serve,npx nodemon,npx tsc), every disjunct in the.some(...)predicate overnpxOptionStates(tokens)evaluates false:argumentsAfterPackageis empty, so both the "trailing args" and "consumed-with-no-args" branches are false, and there's no flag/--yes/quote/special-character evidence either..some()returns false, so the function returnstrue("clearly descriptive"), andsurfacedCommands'sif (!context.ambiguous && (documented || descriptiveNpx)) continueexempts it from being flagged. - Independently re-derived by hand-trace: for the standalone message
"npx serve",tokens = ["npx", "serve"];npxOptionStatesreturns a single state at index 1 with all flags false; the.some()predicate's three conditions (startsWith("-")on an undefined next token,positionalInvocation && length > 0with length 0,state.consumedfalse) are all false, so.some()is false and the function returnstrue. The command is silently exempted from the gate this PR exists to add. - Risk: The Stop hook would let the assistant say
"Next: npx serve", a bare"npx serve", or"Just run npx nodemon"to the user with zero appeal required — exactly the raw-command surfacing this PR is meant to block. No fixture in the new test suite covers this shape: every existing "blocked" npx fixture in.claude/hooks/test-hooks.mjscarries a flag (--yes,--package=), a special character in the package name (@scope/package), or a trailing argument (npx prisma generate,npx turbo run lint). - Fix: Drop the "needs trailing evidence" requirement for the zero-argument case — a bare
npx <token>is exactly as executable with zero arguments as with one. LetDESCRIPTIVE_NPX_PREFIX/DESCRIPTIVE_NPX_CLAUSE(which already look for genuine descriptive sentence shapes) carry the whole burden of the prose exemption instead of inferring intent from argument count. Add fixtures for"Next: npx serve","npx nodemon","Just run npx tsc"expecting block. - Reference: CLAUDE.md rule 1 (root cause — the gate exists specifically to catch raw npx surfacing); this PR's own stated scope ("Raw
node tools/*,npx *, and baretools/*.shinstructions now continue the agent with a corrective message").
What's good
- The 1,600-case deterministic clause-fuzz matrix (
RAW_TOOL_CLAUSE_FUZZ_BUDGET) with an explicit zero-unexpected-passes assertion is strong harness practice, well beyond typical fixture coverage. tools/test-tools.mjs's wave-plan concurrency test replaced a flaky wall-clock assertion with direct start/end event logging and a peak-concurrency bound — a real root-cause fix (rule 1), not a band-aid.- The gate's fail-closed posture (ambiguous fence/quote/backtick parses default to blocking) is consistent with its own stated design philosophy.
Subagents
parity-checker / i18n-syncer / contract-aligner / security-reviewer / design-reviewer: N/A — no apps/*, packages/shared/src/types/*, or orbit-api file in this diff.
Validation
Lint, Type Check, Cross-Platform Parity, Dash Ban, Copy Register, Expo SDK Pin, Suppressions Ratchet, Harness Lockstep, and Skill/Agent Frontmatter checks are handled by this PR's own required CI jobs (Build / Unit Tests / SonarCloud), per this review's CI-adaptation instructions — Phase 6 (/validate) was skipped accordingly.
Deferred — not verifiable in this job
- Cross-repo dimensions (contract-aligner, the orbit-api side of the backward-compat guard): not verifiable —
orbit-apiis not checked out in this job.
Recommendation
Fix the isClearlyDescriptiveNpxMention gap (drop the argument-count requirement for the no-flag case) and add the three missing fixtures before merge — a one-word npx <package> shape (npx serve, npx nodemon, npx tsc) is common enough that this is a real, not theoretical, hole in the exact protection this PR ships. Everything else in this diff — the hook design, the settings wiring, the CLAUDE.md/context-budget bookkeeping, and the test-tools.mjs concurrency fix — is sound.
There was a problem hiding this comment.
Code Review: PR #633 — Gate surfaced repo tool commands (ORB-122)
Scope: PR #633 in thomasluizon/orbit-ui-mobile (feature/orb-122-gate-the-rule-that-a-tools → main)
Recommendation: NEEDS WORK
Summary
The PR adds a new Stop/PostToolUse hook (.claude/hooks/forbid-raw-repo-tool-surfacing.mjs) that blocks raw node tools/*, tools/*.sh, and npx … commands from being surfaced to Thomas in chat or written artifacts, plus a very large, well-organized test corpus (including a 1,600-case deterministic clause fuzz) and supporting changes to tools/test-tools.mjs, .claude/settings.json, CLAUDE.md, and tools/context-budget.json. The diff touches only .claude/hooks/** and tools/** — no apps/*, packages/shared, or orbit-api surface — so the frontend/backend/parity/i18n/contract/design dimensions are all N/A. Manual trace plus an independent adversarial skeptic pass confirmed one real correctness gap in the hook's core npx-detection logic: a whole class of common bare single-word npx invocations (npx serve, npx jest, npx cowsay, …) silently bypasses the gate even under explicit imperative framing, which is exactly the failure mode this ticket exists to close. Everything else — the settings wiring, the CLAUDE.md pointer, the context-budget numbers, and the wave-plan timing-test replacement — is correct and an improvement over what it replaces.
Findings
Critical
None.
High
1. npx <bare-single-word-package> with zero trailing args bypasses the gate regardless of imperative framing
.claude/hooks/forbid-raw-repo-tool-surfacing.mjs:226-247 (isClearlyDescriptiveNpxMention), skip site at :379-380.
isClearlyDescriptiveNpxMention is meant to exempt genuinely ambiguous prose mentions of npx (e.g. "The package is invoked as npx cowsay.") from blocking. But its token-based fallback heuristic (lines ~234-247) treats any npx invocation whose package name contains none of .///@/- and has zero trailing arguments as "descriptive," independent of whether the surrounding text is descriptive prose or a direct instruction. Concretely, for "npx cowsay" with prefix = "Run ":
hasPrefix = true→standaloneCommandshort-circuits tofalse.positionalInvocation = true(prefix present,tokens[1]isn't a flag).npxOptionStates(["npx","cowsay"])returns one terminal state (no leading-/--option to recurse into), withassigned/confirmed/quotedallfalse.- In the
.some()callback:argumentsAfterPackage = [], so all three OR branches (startsWith("-"),positionalInvocation && length > 0,state.consumed && length === 0) evaluatefalse. .some()→false→!false→ function returnstrue("descriptive").
At the call site (:376-380), descriptiveNpx gates the skip independent of instructionFramed — only the separate documented branch is gated by !instructionFramed. So even though hasNpxInstructionFraming("Run ") correctly identifies "Run npx cowsay" as an instruction, the command is still skipped (continue), i.e. not blocked.
Because npm packages commonly ship as single dash-free words (serve, nodemon, eslint, prettier, vite, jest, webpack, cowsay, playwright, cypress, …), this isn't a narrow edge case — it's the single most common npx <tool> shape. An agent can write "Run npx serve" or just "npx jest" as a standalone instruction and it passes straight through the Stop hook silently, undermining the PR's stated purpose ("Raw … npx * … instructions now continue the agent with a corrective message").
Test coverage confirms the gap: the corpus's "imperative npx connector" fixtures (.claude/hooks/test-hooks.mjs, "imperative as npx remains executable" and friends) all use npx --yes @orbit/cli deploy, which has both a --yes/confirmed flag and a scoped, dashed package name — neither of which exercises the bare-word/zero-arg code path above. No fixture in the diff's ~100+ npx cases uses a bare single-word package with zero trailing arguments under imperative framing.
Verification: independently re-traced by an adversarial skeptic subagent (per the review's verification protocol) against the actual committed source, re-deriving npxOptionStates and the .some() boolean logic step by step. Verdict: CONFIRMED. (Full harness execution to add a red repro was not possible in this review session — see Validation section — so this is a static/mechanical trace, not an executed one; the fix should land with a concrete runHook(RAW_TOOL_HOOK, stopPayload("Run npx cowsay")) / stopPayload("npx serve") regression test asserting status === 2.)
Fix direction: thread instructionFramed (or equivalent) into isClearlyDescriptiveNpxMention, or drop the "zero trailing args" carve-out from the token heuristic and rely solely on DESCRIPTIVE_NPX_PREFIX/DESCRIPTIVE_NPX_CLAUSE/fence context for the genuinely-ambiguous-prose case, since those already require actual descriptive phrasing ("the/this/that … as/about/describes/…").
Medium
None.
Low / Info
1. Dead-execution style nit (not a bug). runHook() (.claude/hooks/forbid-raw-repo-tool-surfacing.mjs:499-511) relies on emit() always calling process.exit() to prevent falling through from the Stop branch into emit(artifactVerdict(input)). It is correct (every branch of emit exits), but an explicit return after the Stop block would make the control flow legible without relying on a reader to confirm emit never returns. Not worth a revision on its own.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no shared types / orbit-api changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or landing UI changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — could not execute in this session (see note) |
| Type check | N/A — could not execute in this session (see note) |
| Tests | N/A — could not execute in this session (see note) |
| Build (api) | N/A — orbit-api not touched |
Note: this review session's sandbox requires interactive approval for git checkout/git worktree add (to restore the working tree, which arrived with these exact files pre-reverted) and for any direct node/npm process execution, neither of which was grantable mid-review. Validation therefore falls back to the PR body's own self-reported harness output (8 review rounds, each showing npm run lint / type-check / test, node .claude/hooks/test-hooks.mjs, and node tools/test-tools.mjs all green, plus a 1,600-case deterministic fuzz sweep with 0 unexpected passes) — real evidence, but not independently reproduced here. The High finding above was reached by static trace and confirmed by an independent skeptic reading the same committed source, not by running the suite, precisely because the existing fuzz/test matrix does not cover the input shape in question.
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8), Parity (#9), i18n (#10), Contract drift + backward-compat (#11 — no Zod/DTO schema hunks), Backend hard rules (#13), FEATURES.md parity (#14): all N/A, diff never touches
apps/*,packages/shared/src/types/*, ororbit-api. - All 6 changed files (
.claude/hooks/forbid-raw-repo-tool-surfacing.mjs,.claude/hooks/test-hooks.mjs,.claude/settings.json,CLAUDE.md,tools/context-budget.json,tools/test-tools.mjs) received a verdict; nothing left unexamined. - Full-suite execution (Validation table) not independently reproduced in this session — see note above.
What's good
- The
.claude/settings.jsonwiring is correct:Stophooks don't take amatcher(they aren't tool-scoped), matching the existing schema used elsewhere in the file. tools/context-budget.json's new byte counts (CLAUDE.md12105,.claude/rules/core.md3383, total 15488) were independently re-verified against the committed blobs and match exactly.- The
tools/test-tools.mjschange replaces a flaky wall-clock-based concurrency assertion (Date.now() - start < 3000) with a deterministic event-log-based peak-concurrency check — a real reliability improvement, not just a refactor. - The property-based
settings: every configured hook path resolvestest (replacing a frozen, hand-maintained list of exactly four hook-path strings) is a good design: it can't silently go stale the next time a hook is added or renamed. - The clause-splitting / fence-state / appeal-per-shell-segment machinery is genuinely careful engineering, and the 1,600-case deterministic fuzz matrix (placement × boundary × layout × document × frame × separator) is a strong testing discipline for a hand-written classifier like this.
- The
Repo-tool appeal: <reason>escape hatch, scoped per-command and per-line, is a sound design for the rare legitimate case.
Recommendation
Fix the High finding before merge: either gate descriptiveNpx on !instructionFramed the same way documented already is, or tighten isClearlyDescriptiveNpxMention's token heuristic so a bare, zero-arg, dash-free package name isn't unconditionally treated as non-executable. Add the missing regression fixtures ("Run npx cowsay", "npx serve", "Just do: npx jest" or similar bare single-word forms with zero trailing args) to .claude/hooks/test-hooks.mjs so this exact shape can't regress silently again. Everything else in the diff is solid and can ship as-is once that fix lands.
|
Fixed in 5882634. The npx classifier now verifies command type before descriptive exemptions and treats bare zero argument invocations as executable unless an explicit bounded prose grammar applies. The quoted Node path segments are disjoint from separators, with a 2,048 segment child process timeout regression. Root npm aliases are resolved from package.json and only aliases that reach repo tooling are blocked, including transitive aliases. Added permanent corpus coverage for all reported cases. Verification: node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK; deterministic clause fuzz 1,600 cases, zero unexpected passes; node tools/test-tools.mjs -> ORBIT TOOLS GATE OK. |
|
@codex review |
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review: PR #633
Scope: PR #633 in thomasluizon/orbit-ui-mobile — "Gate surfaced repo tool commands (ORB-122)"
Recommendation: APPROVE
Summary
This PR adds a new Claude Code session hook, .claude/hooks/forbid-raw-repo-tool-surfacing.mjs,
that blocks the agent from surfacing raw node tools/*, npx *, and tools/*.sh commands to
Thomas — either in the final chat message (Stop hook) or in written file artifacts
(PostToolUse hook on Edit|Write|MultiEdit) — with a same-line Repo-tool appeal: <reason>
escape hatch. It is wired into .claude/settings.json, documented with a one-line pointer in
CLAUDE.md, and covered by ~64 new assertions in .claude/hooks/test-hooks.mjs plus a
regenerated tools/context-budget.json. tools/test-tools.mjs also swaps a flaky wall-clock
concurrency assertion for direct event-log evidence. The change touches only .claude/**,
tools/test-tools.mjs, tools/context-budget.json, and CLAUDE.md — no apps/web,
apps/mobile, packages/shared, or orbit-api files, so none of the five gated subagents
(parity/i18n/contract/security/design) fire. The parsing logic (fence tracking, clause/quote
splitting, shell-segment splitting, npx flag parsing, Windows/POSIX path containment) is dense
but was exercised through eight documented review rounds and a stated 1,600-case fuzz sweep; spot
reads of the regexes and control flow did not surface a correctness defect in that core matching
engine.
Findings
Critical
None.
High
None.
Medium
1. The PostToolUse artifact gate exempts every file inside the repo (or any repo declared in
.claude/orchestrator.json), and the code's own justification for that exemption doesn't match
what actually exists in this codebase.
file:.claude/hooks/forbid-raw-repo-tool-surfacing.mjsline: 505 (artifactVerdict):if (isRepoArtifact(filePath)) return nullsummary: This check runs before the narrower, purpose-builtisDocumentationArtifact()
allowlist (lines 260-268, scoped to.claude/(skills|agents|hooks)/paths, ticket/PR-body
basenames, and help-output basenames). The effect: writing a brand-new raw command into an
ordinary repo-tracked doc —README.md,DESIGN.md,TESTING.md, evenCLAUDE.mditself —
is unconditionally exempt, regardless of whether that doc has anything to do with
skills/agents/tooling. This is deliberate and tested:.claude/hooks/test-hooks.mjs
("repo doc new raw command remains artifact-out-of-scope") editsREADME.mdwith the new
string"Run node tools/wave-plan.mjs --all"and asserts exit0(not blocked).failure_scenario: A session editsREADME.md,DESIGN.md, or another ordinary repo doc and
writes a sentence like "Runnode tools/wave-plan.mjs --allto see the plan" instead of
pointing at/next. ThePostToolUsehook never fires (the file is inside the repo), and — per
a grep of.github/workflows/guards.ymland everytools/check-*.mjsscript — no CI job
content-scans doc/markdown prose for raw tool-command patterns. The hook's own header comment
and the PR's "Review round 5" decision log justify the exemption with "repository sources
remain owned by CI," but that CI check does not exist anywhere in this repo today, so the
stated justification is inaccurate as a literal claim.- Downgraded from an initial High read: an independent skeptic pass confirmed the facts above but
pointed out a real mitigating factor — unlike chat output or an out-of-repo scratch file
(which never enter git history and have no backstop), anything landing in a tracked repo doc
is by construction part of this PR's own diff and goes through the mandatory human/@codex
PR-review gate before merge. So "owned by CI" is imprecise (it should say "owned by PR review"),
but the surface isn't fully unguarded — it just relies on a human catching it in a large diff
rather than an automated gate, which is exactly the kind of lapse this ticket exists to remove
for the surfaces it does cover (chat, external scratch files). fix: Either (a) narrow theisRepoArtifactearly-return so it only exempts paths that also
satisfy the existingisDocumentationArtifact()pattern (skills/agents/hooks/ticket/PR-body/
help-output), so an ordinary doc edit is still scanned; or (b) correct the comment/decision-log
claim to name the real backstop ("owned by PR review," not "owned by CI") and open a follow-up
ticket for an actual CI content scan of committed docs, so the gap is tracked rather than
silently assumed-covered.category: correctness / defense-in-depth-gap
Low / Info
None posted (per the rubric's 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 packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — neither repo's contract surface (packages/shared/src/types/*, endpoints.ts) changed, and only one repo is touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI-invoked review context; repo's own CI runs lint independently |
| Type check | N/A — same |
| Tests | N/A — same (PR body reports npm run lint / type-check / test, node .claude/hooks/test-hooks.mjs, and node tools/test-tools.mjs all green as of the latest round) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Parity (#9), i18n (#10), Contract drift (#11), Security (#12, API side), Backend hard rules
(#13), FEATURES.md parity (#14), DESIGN.md/AI-slop (#8): all N/A — the diff never touches
apps/web,apps/mobile,packages/shared,orbit-api, or user-facing feature surface. .claude/hooks/test-hooks.mjs(854 additions / 13 deletions): given a verdict at the
structural level (fixture-matrix breadth, the wave-plan concurrency-log rewrite) rather than
line-by-line re-derivation of all ~64 new assertions; the harness itself is executed by CI
(Harness Execution gate) on every.claude/**/tools/**change, so re-deriving each assertion
by eye would duplicate that gate rather than add signal.tools/context-budget.json: verdicted as a mechanical byte-count regeneration consistent
with the PR's reportednode tools/check-context-budget.mjs --check --jsonoutput; not
independently re-computed by hand.- Validate (Phase 7): skipped — CI-invoked review context; the repo's own CI jobs
(lint/type-check/test, Harness Execution) run independently on this PR.
What's good
- The
Stop+PostToolUsedual-hook design correctly targets the two surfaces diff-time CI
structurally cannot see (final chat text, and file writes outside any repo), and fails open
(catch { process.exit(0) }) consistent with every sibling hook in this repo. - Cross-platform path containment (
isRepoArtifact) correctly branches on Windows vs POSIX path
syntax by inspecting the path text itself rather than the runtime OS — necessary because
.claude/orchestrator.json's declared repo roots are Windows paths (Thomas's dev machine) while
CI runs on Linux. - The
tools/test-tools.mjschange replaces a wall-clock-based concurrency assertion (which the
PR notes was measuring Windows child-process startup latency, not actual concurrency) with a
direct start/end event log and a peak-concurrency computation — a genuine flake fix, not a
workaround. - Appeal semantics (
Repo-tool appeal: <reason>) are scoped per-command and per-line, including
correct handling of&&/||/;/|shell chains needing one appeal per segment — matches the
stated design intent and is exercised by dedicated fixtures.
Recommendation
Approve. The one surviving finding is Medium (an inaccurate "owned by CI" justification for a
real but human-review-backstopped gap in the artifact check's repo-file exemption) and does not
block merge under the rubric's deterministic gate. Consider a fast-follow ticket to either narrow
the isRepoArtifact exemption to documentation-shaped paths only, or to correct the comment and
decision-log language to name PR review — not CI — as the actual backstop for repo-committed
content.
|
READY-TO-MERGE 5882634 |
1 participant
Summary- Add a deterministic session gate for raw repo-tool commands surfaced to Thomas in chat or written artifacts.- Route wave-plan guidance to
/next, identify missing skill coverage for other commands, and support a recordedRepo-tool appeal: <reason>path.- Wire the gate toStopandPostToolUse, document the enforced rule, and add catch plus false-positive coverage.## WhyORB-122 closes the gap where repo tooling could be handed to Thomas as a shell instruction even though skills and agents are the supported interface. Diff-time CI cannot inspect conversational output or files written outside the repository, so enforcement belongs in session hooks.## ImpactRawnode tools/*,npx *, and baretools/*.shinstructions now continue the agent with a corrective message. Correct machine-to-machine uses in skills, agents, tickets, PR descriptions, and help output remain allowed. An explicit appeal proceeds and records its reason in the hook output and session transcript.## Initial validationtext$ npm run lintTasks: 3 successful, 3 totalExit code: 0$ npm run type-checkTasks: 3 successful, 3 totalExit code: 0$ npm run testTasks: 4 successful, 4 totalExit code: 0$ node .claude/hooks/test-hooks.mjsORBIT HOOK PARITY OKExit code: 0$ node tools/test-tools.mjsORBIT TOOLS GATE OKExit code: 0## Fired gate messagetextRaw repo-tool command surfaced for Thomas: node tools/wave-plan.mjs --allUse /next for the supported read-only recommendation.Remove the raw command, or put "Repo-tool appeal: <reason>" on the same line when it genuinely must be shown. Every surfaced command needs its own recorded reason.## Review round 1- Minimized the always-loaded CLAUDE.md edit to a single gate reference. No baseline reseed orcontext:reseedlabel is needed.- Replaced the frozen settings hook list with a property test that validates every configured path, proves the scan is nonempty, and fails for a renamed missing hook.- Added standalone inline-code instruction detection plus an explanatory-prose false-positive fixture.- Replaced the wave-plan wall-clock harness assertion, which measured Windows child-process startup, with direct bounded-concurrency evidence.### Final hook harnesstext$ node .claude/hooks/test-hooks.mjsPASS cc raw-tool: instructed standalone code span -> 2PASS cc raw-tool: instructed standalone code span names /nextPASS cc raw-tool: inline code span in explanatory prose -> 0PASS settings: every configured hook path resolvesPASS settings: configured hook path scan is nonemptyPASS settings: renamed configured hook fixture reports the missing fileORBIT HOOK PARITY OKExit code: 0### Final tools harnesstext$ node tools/test-tools.mjsPASS every tools/ script has coverage (26 scripts)PASS wave-plan.mjs: fetches 100 relations in a bounded pool while preserving the table orderPASS check-context-budget.mjs: total over baseline exits 1 and names the offending filePASS check-context-budget.mjs: total under baseline exits 0ORBIT TOOLS GATE OKExit code: 0### Final context budgetjson{ "files": { "CLAUDE.md": 18794, ".claude/rules/core.md": 3443 }, "enforcedBytes": 22237, "estimatedTokens": 5559, "imports": [ "@../orbit-api/CLAUDE.md", "@../orbit-landing-page/CLAUDE.md" ], "unconditionalRules": [ ".claude/rules/core.md" ], "unexpectedImports": [], "unexpectedRules": [], "siblingFiles": [], "fullSessionBytes": 22237, "fullSessionEstimatedTokens": 5559, "baselineBytes": 22244, "baselineSource": "refs/remotes/origin/main", "deltaBytes": -7, "fileGrowth": [ { "file": ".claude/rules/core.md", "bytes": 63 } ], "structuralFindings": [], "status": "ok"}No visible-effect screenshot applies. CI is intentionally not polled under the finishing contract.## Decisions taken unattended- Used bothStopandPostToolUse: the first evaluates completed chat text, while the second catches completed artifact writes and feeds correction back to the agent.- Used conservative structural framing plus explicit document-path and quoted-output exemptions because false positives are more costly than misses for this gate.- DefinedRepo-tool appeal: <reason>as the explicit override interface and record the reason with a session-visible system message.- Integrated currentmainwith a merge commit so the published feature branch was not force-pushed.- Kept the context baseline unchanged because the final enforced total is 7 bytes below it.- Replaced elapsed-time inference with direct concurrency events after the mandatory tools harness reproduced the same timing-only failure in isolation.## Review round 2- Bound appeals to the same physical line as one command and evaluate every surfaced command independently.- Tightened directnpxdetection and retained structurally framed command detection, so ordinarynpxprose passes while real invocations still block.- Deleted the redundant fence lookahead and replaced it with opening and closing fence state.- Made help-output framing tolerate inline-code punctuation and added a Bash-fence fixture that passes only through that exemption.- Integrated ORB-118 frommain. Its lower context baseline made the required 107-byte hook-path and appeal clause genuine growth, so the generated baseline was reseeded and the PR carriescontext:reseed.Fix commit:3c48a333Integrated head:cf7ba80b### Review round 2 hook harnesstext$ node .claude/hooks/test-hooks.mjsPASS cc raw-tool: npx prose "npx is a great tool for running one-off packages." -> 0PASS cc raw-tool: npx prose "npx invocations without --yes will prompt for confirmation." -> 0PASS cc raw-tool: npx prose "npx runs whatever package you name, unlike a pinned devDependency." -> 0PASS cc raw-tool: quoted --help output -> 0PASS cc raw-tool: closing fence does not scan following prose -> 0PASS cc raw-tool: one appeal cannot cover another command -> 2PASS cc raw-tool: every command has its own appeal -> 0ORBIT HOOK PARITY OKExit code: 0### Review round 2 tools harnesstext$ node tools/test-tools.mjsPASS every tools/ script has coverage (27 scripts)PASS check-context-budget.mjs: a grown branch can regenerate its working baselinePASS check-context-budget.mjs: a regenerated working baseline cannot hide growth from the target branchORBIT TOOLS GATE OKExit code: 0### Review round 2 context budgettext$env:CONTEXT_BUDGET_BASE_REF='HEAD'; node tools/check-context-budget.mjs --check --json{ "files": { "CLAUDE.md": 11935, ".claude/rules/core.md": 3380 }, "enforcedBytes": 15315, "baselineBytes": 15315, "baselineSource": "HEAD", "deltaBytes": 0, "structuralFindings": [], "status": "ok"}Committed-blob verification:11935 + 3380 = 15315, exactly matchingtools/context-budget.json.### Review round 2 decisions taken unattended- Chose same-line appeals because the positional rule is unambiguous in prose, shell fences, and written artifacts.- Treat bare-wordnpxforms as commands only when structural framing supports that interpretation; high-confidence package shapes remain direct commands.- Merged currentmaininstead of force-pushing so ORB-118's context trim and subsequent tool changes remain intact.- Reseeded only after EOL normalization and exact blob-size verification. This supersedes round 1's pre-ORB-118 note that no reseed was needed.Review round 3
npxnames, and non-shell fences.Next:, bothJust doforms, the Bash-script form, and the scoped confirmednpxform.npxprose passes and added direct fixtures for the bare-name, following-flag, and non-shell-fence boundaries.context:reseedwas already present so the new PR event carries the label.Fix commit and head:
ddd68a38Review round 3 hook harness
Review round 3 tools harness
Review round 3 context budget
Review round 3 decisions taken unattended
npxmentions exempt outside shell fences because their syntax is indistinguishable from ordinary discussion and this ticket prioritizes false-positive avoidance.npxforms.Review round 4
origin/mainat ORB-119 (58f40476) without rebasing.@import was restored.main's context baseline whole, normalized EOLs, then regenerated the required 107-byte ORB-122 growth through the tool.a0182a24; GitHub reportsmergeStateStatus: BLOCKED, notDIRTY.Review round 4 hook harness
Review round 4 tools harness
Review round 4 context budget
Committed-blob comparison:
Review round 4 decisions taken unattended
/nextgate suffix.mainbaseline.BLOCKEDas proof that merge conflicts are cleared; it is distinct fromDIRTY, and the finishing contract forbids waiting on checks or review verdicts.Review round 5
.claude/orchestrator.json; repository sources remain owned by CI.Run npx prisma generate nowframing blocks while the three ordinary npx prose cases remain allowed.&&,||,;, and|, with one appeal required per resulting command.Fix commit and head:
144b9cc2Review round 5 hook harness
Review round 5 tools harness
Review round 5 context budget
Review round 5 decisions taken unattended
Review round 6
origin/mainat ORB-116 (962b3f3f) with merge commit7d06a98d.main's context baseline whole, normalized EOLs, then regenerated only the existing 107-byte ORB-122 growth through the budget tool.--yesconfirmation now block.npx --packagemention remain allowed.99a45df1; GitHub reportsmergeStateStatus: BLOCKED, notDIRTY.Fix commit:
99a45df1Integrated head:
7d06a98dReview round 6 hook harness
Review round 6 tools harness
Review round 6 context budget
Committed-blob comparison:
Review round 6 decisions taken unattended
--yesconfirmation signal while limiting it to the parsed leading option run, so prose that mentions--yeslater in the sentence remains exempt.Review round 7
Fix commit and head:
8d398310Review round 7 hook harness
Review round 7 tools harness
Review round 7 context budget
Review round 7 decisions taken unattended
Review round 8
origin/mainat ORB-113 (f1513383) without rebasing./ticketrename, new shared scope-completeness contract, and orchestrate section 0a.@../imports were restored.main's context baseline whole, normalized EOLs, then regenerated only ORB-122's existing 107-byte context growth through the budget tool.dc2b19e5; GitHub reportsmergeStateStatus: BLOCKED, notDIRTY.Review round 8 hook harness
Review round 8 tools harness
Review round 8 context budget
Committed-blob comparison:
Review round 8 decisions taken unattended
mainwhole before running the sanctioned reseed path.Classifier fuzz evidence
Deterministic clause fuzz: 1,600/1,600 cases blocked as expected; 0 unexpected passes (4 placements x 5 boundaries x 5 command layouts x 2 documentation phrases x 2 instruction frames x 4 separators).