[thomasluizon]

What this is

The /drive work-order harness has lived only on feature/539-b5-apply-design. Nothing on main could run it, so /drive was usable for exactly one issue. This ports the tooling so it works for any issue.

Why it needed five review rounds first

The harness exists because ten consecutive sessions reported #539 done while git showed 9 of 171 surfaces had cleared the depth floor. Before moving it, it was reviewed by five independent rounds of fresh agents, each handed the previous round's confirmed defects and told to hunt that class hardest. Thirty-one Critical/High defects were confirmed by adversarial skeptics who reproduced each one in their own clone.

The three that mattered most, all fixed:

1. A child that did nothing was recorded ready-for-review. Both gates exit 0 on an empty diff, so the contradiction path never fired. An empty base...head range is now a failure, not a pass.
2. The gates judged whatever HEAD the child left in the working tree. A child could commit an escape, push, open the PR, then git checkout <base> and score clean. The gate now judges an explicit base...head range, --base is required, and the merge-base guessing ladder is deleted.
3. forbid-gate-tamper allowed an interpreter write to the human-only signoff file when wrapped as if x=$(node -e "..."). All five wrapper shapes are now blocked, pinned by tests.

What is deliberately excluded

The spacing-scale and no-arbitrary-zindex lint rules, the eslint config changes that flip no-decorative-glow and no-raw-gradient to error, and the populated suppressions ledgers. Those are #539 design work, not harness: main still has the glow those rules ban, so porting them would fail CI for the right reason. The ledgers land empty so the tools have their required input, and fill when the rules arrive with the design work.

Two changes main needed to accept the port

• The typed-uppercase corpus guard becomes a ratchet instead of asserting zero. Main carries two pre-existing values per locale. One of them, confirmPlaceholder = "ORBIT", is load-bearing: the user types it verbatim to confirm account deletion, so it must never be recased. The guard still blocks anything new creeping in.
• tools/visual-signature.mjs carried a stray NUL byte that made git treat a source file as binary and its diff unreviewable.

Scope

• 2 files under apps/, both new empty suppressions ledgers. Zero application code changes.
• 169 work orders and the surfaces manifest are derived from main's own router and component graph, not the design branch's.
• 733 hook assertions pass locally.

Known limits, stated plainly

• The harness has never completed a real run. Every run directory on disk predates this tooling. The next step is one small attended /drive run, not another review round.
• Mechanical debt is a lint-count axis, not evidence that a pixel moved. Clearing lint by hoisting a literal into a named constant passes both gates and changes nothing visually. The depth measurement prints beside it for exactly this reason.
• Only a human tick in signoff.json completes a surface. Nothing here grants completion.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
orbit-ui-mobile-web	Ignored		Jul 23, 2026 10:36am

[claude]

/pr-review — PR #570 "chore(harness): bring the /drive work-order harness to main"

Scope: 227 files, entirely .claude/ tooling + 169 generated work orders. No apps/** application code changes beyond two new empty ESLint-suppression ledgers ({}).

Recommendation: REQUEST CHANGES (1 Critical, 1 Medium, 1 Low)

All three findings below were independently verified against the actual committed diff (git show/git diff between 478dbc1e and d5dc4a28), not the working tree — this sandbox's working tree carries unrelated pre-existing uncommitted deletions that do not reflect the PR's real content.

Critical — .claude/hooks/_lib/rules-gate-tamper.mjs:127-171 — the signoff.json tamper guard has an alias bypass

inlineCodeIsReadOnly allowlists an inline node -e/python -c call by checking only the name of the identifier immediately preceding (, never what that identifier is bound to. READ_ONLY_CALLS necessarily includes generic method names for their legitimate .method() uses (map, filter, test, parse, sort, get, join, find, size, …). This lets an agent alias the real write API to one of those names and defeat the guard entirely:

node -e "const map=require('fs').writeFileSync; map('.claude/manifests/signoff.json','fake')"

Hand-traced step by step against the exact committed code:

• require('fs') passes the literal-require carve-out (fs matches REQUIRABLE).
• The map(...) call site has callee map, which is in READ_ONLY_CALLS — so inlineCodeIsReadOnly returns true.
• segmentIsReadOnly → commandIsReadOnly → true, so blockedSegment never flags the segment even though SIGNOFF_RE matches the path in it.
• checkGateTamperBash returns null (not blocked), and the write to signoff.json — the harness's only human-only completion grant, with no sanctioned agent writer at all — goes through.

The identical technique defeats surfaces.json, verdicts.json, and calibrate-judge.mjs protection (confirmed via the same trace pattern, aliasing to parse/test respectively).

I could not execute node directly in this review sandbox (blocked by its own approval policy), so this is a rigorous manual trace against the real committed source rather than an observed run. The existing test suite (.claude/hooks/test-hooks.mjs, ~733 assertions per the PR description) only exercises the direct-call form (node -e "require('./file.json').x"-style reads and the documented bypasses 1-4 the file's own header lists) — never an aliased-callee call — so nothing currently catches this. This defeats exactly the protection this PR's stated purpose depends on, and the file's own header documents four prior live-caught bypasses of this same "gate looks at syntax, not semantics" shape — this is a fifth.

Suggested direction (not prescriptive): resolve the callee by checking what require('fs')/require('node:fs') was assigned to, not just the call-site identifier name — e.g. track identifiers bound to a fs/node:fs module reference (or any of its methods) and refuse the call regardless of what name it's aliased to, rather than allowlisting by bare identifier name alone.

Medium — .claude/hooks/flag-new-todos.mjs — dead file with a broken import left behind

Confirmed via diff: this PR removes checkNewTodos from _lib/rules-source.mjs, its settings.json PostToolUse wiring, and its test assertions in test-hooks.mjs — but leaves .claude/hooks/flag-new-todos.mjs itself on disk, still doing:

import { checkNewTodos } from "./_lib/rules-source.mjs"

against an export that no longer exists. It's no longer wired into any hook path, so it won't fire in normal Claude Code operation, but it's dead code with a broken import sitting in the tree — violates CLAUDE.md rule 2 ("delete unused code immediately"). Fix: git rm .claude/hooks/flag-new-todos.mjs (and check the .opencode/plugin equivalent doesn't reference it either).

Low — .claude/hooks/test-hooks.mjs:203-215 — two added comment blocks contradict each other

One new comment says the locale corpus "now carries ZERO typed-uppercase violations"; the very next new comment (also added in this same hunk) says "main still carries two pre-existing values [per locale]." The PR description and the actual test assertion (uppercaseFindings <= 2) confirm the second is correct — the first reads like leftover text from the feature branch where the count really was zero. No functional impact; worth a one-line cleanup to remove the stale "ZERO" claim.

What's good

• The gate-tamper file's self-documented history of four prior live-caught bypasses (segmentation, --output= forging, unanchored sanction matching, keyword-stripped assignment laundering) shows real adversarial hardening — this PR's remaining gap is a fifth instance of the same failure mode, not a design that skipped the work.
• tools/check-diff-ownership.mjs's explicit --base/--head range handling (refuses to guess a range; an empty range is a failure, not a pass) closes a real "vacate the branch to score a false green" hole.
• The typed-uppercase corpus guard correctly becomes a ratchet (<= 2) instead of asserting an exact zero that would've broken on the pre-existing, load-bearing confirmPlaceholder: "ORBIT" string.
• The .gitattributes LF pin and the stray-NUL-byte fix in tools/visual-signature.mjs (which made the file diff unreviewable as binary) are both legitimate, unglamorous fixes.

Scope/tooling limits this session

• No Agent-spawn tool was available to run the five review subagents (parity-checker/i18n-syncer/contract-aligner/security-reviewer/design-reviewer); assessed each inline instead. Only parity gates in (two trivial empty-object ledgers); i18n/contract/design are N/A — no i18n, API-contract, or apps/** UI surface is touched by this diff.
• orbit-api is not checked out in this job — cross-repo contract-aligner dimension is not verifiable here.
• Skipped per the workflow's CI adaptations: Phase 6 (/validate — Build/Unit Tests/SonarCloud run as separate required checks) and Phase 7 (posting; this review is the Step 2 post).
• Not given a full line-by-line pass (spot-checked only, no anomaly found): the 169 generated .claude/workorders/*.md files, .claude/manifests/surfaces.json (~12.5k lines, generated), and the larger tool files (drive-queue.mjs, workorder.mjs, drive/run.mjs, visual-signature.mjs, capture-surfaces.mjs, judge-surfaces.mjs) beyond verifying the specific claims made in the PR description.
• Note on this session's working tree: it carries a large set of pre-existing uncommitted file deletions unrelated to this review (present before this review began) that do not reflect PR #570's actual content. All findings above were verified against the real committed diff via git show/git diff, not the working tree.

[claude]

Critical — alias bypass on the signoff.json tamper guard.

inlineCodeIsReadOnly allowlists a call by checking only the identifier name immediately before ( against READ_ONLY_CALLS, never what that identifier is bound to. Since that set necessarily includes generic method names (map, filter, test, parse, sort, get, join, find, size, …) for their legitimate .method() uses, an agent can alias the real write API to one of those names and defeat the whole guard:

node -e "const map=require('fs').writeFileSync; map('.claude/manifests/signoff.json','fake')"

Hand-traced: require('fs') passes the literal-require carve-out, the map(...) call's callee is map which is in READ_ONLY_CALLS, so inlineCodeIsReadOnly returns true → segmentIsReadOnly → true → blockedSegment never flags it even though SIGNOFF_RE matches the path inside the string → checkGateTamperBash returns null (not blocked). The write to signoff.json — the harness's only human-only completion grant, with no sanctioned agent writer at all — goes through. The same technique defeats surfaces.json/verdicts.json/calibrate-judge.mjs protection identically (alias to parse/test etc.).

This is the same "gate judges syntax, not semantics" failure shape as the four bypasses this file's own header documents as previously caught live — a fifth instance. Suggested direction: resolve what require('fs')/require('node:fs') was assigned to and refuse any call through that binding (or its methods) regardless of the name it's aliased to, rather than allowlisting by bare call-site identifier name.

[thomasluizon]

All three findings addressed in f1f2df93. Notes on each, including where the review was right about more than it knew.

Critical, alias bypass — confirmed by execution, then fixed. The review traced this by hand because it could not run node. I reproduced it first: of five write shapes, four were ALLOWED (map/parse/test aliases and a destructured rename); only the python form was already blocked. So the finding was correct and under-stated, not over-stated.

The fix judges the member lifted off a module, not the identifier at the call site. A computed member (require('fs')['writeFileSync']) is refused outright because the name is not visible. A destructured binding is judged by the keys it lifts, never the local names it gives them.

One deliberate asymmetry: a required .json is data, not a module, so require('./surfaces.json').cells still reads. Over-blocking that would break the exact pattern this hook's own error message advertises, and a guard that turns away honest reads is what tempts a future session to disarm it. Renaming even a read API is refused, fail-closed — the guard judges names, and the Read tool covers what that turns away.

Six regression tests pin every shape. Suite is 740 assertions, 0 failures.

Medium, dead file — the cause was broader than the symptom. flag-new-todos.mjs was not an oversight about one file. git checkout <ref> -- <dir> adds and updates but never deletes, so every path the harness branch had removed survived the port. That was 18 files: the dead hook, the rules README plus the three rule files that became playbooks, the /execute and /night-run skills that /drive replaces, and the eight .opencode/agents mirrors whose deletion is a locked decision the ported CLAUDE.md already documents.

eslint-rules/no-draggable-onscroll.cjs is deliberately kept. It is main's own rule from #568, absent from the harness branch only because the branch predates that merge. Deleting it would have silently reverted a shipped fix.

Low, contradictory comment — the stale "carries ZERO typed-uppercase violations" line is gone; the ratchet and its comment now agree.

On the one red check: "Authed Today Lighthouse budget" fails on total-blocking-time. This PR contains zero application code, so it is either runner variance or a pre-existing regression on main. Tracking it separately rather than muddling it into this PR.

[thomasluizon]

CI note, not a PR problem: GitHub Actions is currently in a partial outage (Webhooks and Pull Requests both degraded, per githubstatus.com). Pushing f1f2df93 fired no workflow runs, and neither did a close/reopen of this PR. The green checks visible above are from d5dc4a28, the previous commit.

On d5dc4a28 everything passed except "Authed Today Lighthouse budget" (total-blocking-time), which this PR cannot have caused since it contains zero application code.

Once Actions recovers, re-trigger with an empty commit:

git commit --allow-empty -m "chore: re-trigger CI" && git push

[claude]

Code Review: PR #570 — chore(harness): bring the /drive work-order harness to main

Scope: PR #570 in thomasluizon/orbit-ui-mobile (chore/drive-harness → main)
Recommendation: NEEDS WORK

Summary

This PR is a pure tooling/harness port — 241 files, ~35k additions, entirely under .claude/,
.opencode/, tools/, .github/workflows/, root docs, and two empty ({}) eslint-suppressions
ledgers under apps/web / apps/mobile. No application code, no packages/shared, no
orbit-api changes. The PR's own body documents five prior review rounds and 31 confirmed
Critical/High defects already fixed, several of them exactly this class of bug (a regex/segment-
based shell-command classifier being bypassed by a new obfuscation shape). This review found one
more member of that same family that survived those five rounds: a two-step variable indirection
that lets an agent write .claude/manifests/signoff.json (the human-only completion grant) while
the guard classifies the command as read-only. It also found a stale cross-reference in the
CLAUDE.md docs registry the PR itself edits. Everything else inspected (the spawnSync-based
shell-outs in tools/*.mjs and .claude/skills/drive/run.mjs, the CI workflow additions, the
.gitignore/.gitattributes changes, the parity of the two ledger files) was sound and well
reasoned.

Findings

Critical

[Critical] The tamper guard's inline-code allowlist can be bypassed by a two-step module-member indirection, letting an agent write the human-only signoff.json
· dimension: 5 (No-workaround / root-cause) and 12 (Security — authorization / self-certification bypass)
· location: orbit-ui-mobile/.claude/hooks/_lib/rules-gate-tamper.mjs:150-186 (inlineCodeIsReadOnly), reached via commandIsReadOnly (:188-192) → segmentIsReadOnly (:257-269) → checkGateTamperBash (:281-286)
· issue: inlineCodeIsReadOnly only recognizes a module-member write when the member access is
syntactically chained directly onto the require(...) call text (MODULE_MEMBER_RE =
require\(...\)\s*(?:\.\s*(\w+)|\[)) or onto a destructuring assignment
(MODULE_DESTRUCTURE_RE). The PR's own "BYPASS #5" fix (test cases at
.claude/hooks/test-hooks.mjs:2910-2914) closes exactly this for the one-line form
const map = require('fs').writeFileSync; map(...). But splitting the require and the member
access across two statements defeats both regexes: const fsmod = require('fs'); const map = fsmod.writeFileSync; map('.claude/manifests/signoff.json', '{}'). Traced end-to-end against
the actual source: require('fs') here is not immediately followed by . or [ in the text
(it's followed by ;), so MODULE_MEMBER_RE never matches it and the function falls through to
the generic callee-name scan at the bottom of inlineCodeIsReadOnly. That scan only checks the
identifier actually being called (map() against READ_ONLY_CALLS — and "map" is on that
allowlist (as Array.prototype.map), so the call passes. inlineCodeIsReadOnly returns true;
commandIsReadOnly therefore returns true; in segmentIsReadOnly, WRITE_VERB_RE /
OUTPUT_FLAG_RE never see this because withoutQuotedText blanks the entire quoted -e payload
before those checks run, and commandIsReadOnly(text) returning true short-circuits the rest
of the function; in blockedSegment the segment matches SIGNOFF_RE but
!segmentIsReadOnly(segment) is false, so the segment is never returned as blocked.
checkGateTamperBash returns null (no block) for the full command
node -e "const fsmod = require('fs'); const map = fsmod.writeFileSync; map('.claude/manifests/signoff.json', '{}')".
Any of the other generic read-named identifiers on READ_ONLY_CALLS (get, has, log, join,
filter, …) works as the alias just as well.
· risk: .claude/manifests/signoff.json is documented as "the ONLY axis of the completion oracle
that GRANTS a cell" and is explicitly designed to have "no sanctioned agent writer... at all" —
the entire point being to prevent an agent from self-certifying the #539 visual work as done.
This bypass defeats that guarantee with one extra const statement inserted ahead of the exact
pattern the PR's own tests already cover, i.e. it is a small, discoverable variant of a fix the
PR just shipped and tested. An agent under time pressure to report a bundle "done" (the failure
mode the whole harness exists to stop, per the PR's own description of the #539 post-mortem) has
a working one-liner to fabricate a human sign-off.
· fix: Make the classification dataflow-aware instead of purely syntactic: track any identifier
bound (directly or transitively) to a property pulled off a require(...) result — not just
literal require(...).member or {...} = require(...) text — and reject the call if that
binding's underlying module member is not itself on READ_ONLY_CALLS. A minimal fix: when a
plain const NAME = require(MOD) (no immediate member/destructure) is seen, track NAME as "a
live module reference"; then treat any NAME.member or NAME[...] access as equivalent to
require(MOD).member for the purposes of MODULE_MEMBER_RE's check, and refuse the whole
segment if that identifier is later called through any alias. Add regression tests for the
two-step form (const x = require('fs'); const y = x.writeFileSync; y(path, data)) and for a
destructure-of-a-bound-variable form (const x = require('fs'); const {writeFileSync} = x; ...),
which is a second variant of the same gap.
· reference: CLAUDE.md rule 1 (root cause over workarounds — the guard is itself the "upstream fix"
for this class of bug and should close the gap structurally, not per-payload); the PR's own
.claude/hooks/_lib/rules-gate-tamper.mjs header comment, which states the exact same posture
("every called name must be on the read list... a computed or concatenated writer name cannot
smuggle past the list") that this bypass violates.

Note on verification: this session's Bash tool is restricted (script/node execution
consistently returned "requires approval" with no available approver), so the bypass could not be
confirmed by actually running checkGateTamperBash. It was instead traced by hand, statement by
statement, against the exact regex definitions and control flow in the PR's own diff (quoted
above), including manually reproducing what each intermediate variable (bare, unquoted,
literalRequires, the MODULE_MEMBER_RE match set) evaluates to for the given payload. Recommend
a human or a session with unrestricted node execution run the one-liner above against
checkGateTamperBash before merge to get a live confirmation; the trace does not depend on any
assumption that isn't visible directly in the diff.

High

None.

Medium

[Medium] CLAUDE.md's docs registry misdescribes where product-and-content.md and visual-delivery.md live after this PR's own rules→playbooks split
· dimension: 2 (Dead/stale code — stale documentation) / general correctness of the PR's own stated intent
· location: orbit-ui-mobile/CLAUDE.md:80 (docs registry row for .claude/playbooks/)
· issue: The registry row reads: ".claude/playbooks/ | The on-demand judgement tier: 47 standing
rules no gate can check, themed by activity (debugging, review, planning, product/content,
visual-delivery)... visual-delivery.md is mandatory..." — naming product/content and
visual-delivery as if they live under .claude/playbooks/. They don't: the diff only renames
debugging.md, planning-and-artifacts.md, and review-and-audit.md into .claude/playbooks/;
product-and-content.md and the new visual-delivery.md both stay under .claude/rules/ as
path-scoped auto-loading rules (confirmed by this same PR's new .claude/playbooks/README.md,
which explicitly tables ../rules/visual-delivery.md and ../rules/product-and-content.md
under "Path-scoped, still in .claude/rules/", and by the correct cross-reference a few lines
above in CLAUDE.md's own "visual completion gate" bullet: "See .claude/rules/visual-delivery.md").
The "47" count is the correct total across all five files, but the row conflates two directories
into one description.
· risk: CLAUDE.md is the root, always-loaded project doc; a future session (human or agent) that
wants to find or update visual-delivery.md or product-and-content.md and trusts this registry
row will look in .claude/playbooks/ and not find them there. Low practical damage but directly
contradicts the rule stated two lines below it ("When you change a doc, update its At a glance
header and this registry in the same edit"), which this very edit was supposed to satisfy.
· fix: Split the row into two — one for .claude/playbooks/ (debugging, review-and-audit,
planning-and-artifacts only, 30 rules) and reuse/extend the existing description of
.claude/rules/ to mention product-and-content.md and visual-delivery.md as the two
path-scoped files alongside core.md.
· reference: CLAUDE.md "Docs registry" section, "When you change a doc, update its At a glance
header and this registry in the same edit."

Low / Info

None posted (signal gate).

Subagents

Dedicated subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer,
design-reviewer) could not be spawned in this session (no subagent-launch tool available) — per
the skill's Phase 4 fallback, each gated check was run inline instead:

Agent	Gate fired?	Inline check performed	Verdict
parity-checker	Yes — apps/web/eslint-suppressions.json + apps/mobile/eslint-suppressions.json both changed	Diffed both files: both new, both {}, byte-identical	PAIRED
i18n-syncer	No — no user-facing strings or packages/shared/src/i18n/*.json touched	—	N/A
contract-aligner	No — no packages/shared/src/types/*/endpoints.ts, no orbit-api touched	—	N/A
security-reviewer	No — no orbit-api code touched	—	N/A
design-reviewer	No — no apps/web/**/apps/mobile/**/landing UI file touched (the two changed files are lint-suppression ledgers, not UI)	—	N/A

Degraded-run banner: this review ran without the ability to fan out to isolated subagent
contexts and without a working node/script-execution path in this session's Bash tool (repeated
"requires approval" with no approver available for node -e, node <file>, rm, mkdir, and
git ls-tree). Every check above that would normally be dynamic (the bypass trace, running
/validate) was done as a manual static trace instead. This is stated per review-and-audit rule 5
("a degraded run is a failed run, not a cheaper one — lead with an explicit degraded banner").

Validation

Check	Result
Lint	N/A — could not execute in this session (see degraded-run banner); no lint-affecting app code changed
Type check	N/A — same reason; diff contains no .ts/.tsx
Tests	N/A — same reason. .claude/hooks/test-hooks.mjs (2477 new/changed lines) presumably covers the shipped bypasses (confirmed by reading its assertions), but was not executed live in this session
Build (api)	N/A — orbit-api untouched

Deferred — N/A dimensions & files not verdicted

• Dimension 6 (Type safety) — N/A by gate in the TS/C# sense (no any/as any); the .mjs
  tooling files are plain JS with no static types to check.
• Dimension 8 (DESIGN.md/AI-slop) — N/A: no apps/* UI file changed.
• Dimension 9 (Parity) — checked inline above (PAIRED); i18n and contract-drift dimensions are
  N/A per the gates.
• Dimension 13 (Backend hard rules) — N/A: no orbit-api file changed.
• Dimension 14 (FEATURES.md parity) — N/A: no user-facing feature surface changed (pure
  tooling).
• .claude/manifests/surfaces.json (12,537 new lines) — not reviewed line-by-line; it is
  generated data (tools/surface-manifest.mjs output), not hand-authored logic.
• The 169 new .claude/workorders/*.md files and INDEX.md — spot-checked for structural
  consistency (frontmatter shape, pairing), not read individually; they are machine-generated
  per-surface contract files with a templated shape.
• .claude/hooks/test-hooks.mjs (2477 changed lines) and .claude/skills/drive/run.mjs
  (1187 new lines) — read for shell-out safety (spawnSync usage, array args, no shell: true;
  all clean) and cross-checked against the gate-tamper bypass tests, but not audited line-by-line
  for every code path given the size.
• Dynamic validation (running npm run / node .claude/hooks/test-hooks.mjs / /validate) —
  could not run in this sandboxed session; see the degraded-run banner above.
• Cross-model second opinion (/second-opinion) — not run; no opencode/GLM path available in
  this session (same "UNAVAILABLE" path the skill names for CI runs).

What's good

• The PR's own narrative is unusually rigorous for a harness change: it documents the five prior
  review rounds, cites the specific prior Critical bugs (PR #560) it fixed, and explains exactly
  why each earlier version of the guard was both too strict (false positives on read-only commands)
  and too loose (real bypasses) — this is exactly the kind of WHY-with-provenance comment the repo's
  comment policy asks for.
• Every shell-out in the new tools/*.mjs and .claude/skills/drive/run.mjs uses spawnSync with
  an argument array (never a shell string), so there is no command-injection surface from
  attacker-controlled branch names, PR titles, etc.
• The .gitattributes/.gitignore/CI workflow changes (LF pinning for workorders/.mjs, the
  work-order ledger freshness CI check, the typescript install step) are all backed by a
  concrete, stated failure mode rather than speculative hardening.
• The two eslint-suppressions.json ledgers land paired and identical across web/mobile, matching
  the parity contract exactly.
• The Stop-hook honesty gate (surface-coverage-gate.mjs) is explicitly designed to fail silent on
  a turn that makes no completion claim, and fails closed on its own internal errors — a good,
  documented "verifier that errors is UNKNOWN, not a pass" property.

Recommendation

Fix the two-step module-member indirection bypass in inlineCodeIsReadOnly (the Critical finding)
before merge — this is the exact class of defect five prior rounds were run to close, and the fix
should be dataflow-aware rather than adding one more syntactic special case, so the next
indirection variant doesn't reopen the same gap. Add the regression tests named above. Fix the
CLAUDE.md docs-registry row (the Medium finding) in the same pass since it's a one-line change.
Once both land, re-run node .claude/hooks/test-hooks.mjs to confirm the new tests are red before
the fix and green after, and diff the result against this report's Critical finding before
re-requesting review.

[thomasluizon]

Bypass #6 closed in e92ae6e1. The finding was correct, and again I reproduced it before touching anything: three of the four write shapes were ALLOWED, including the reviewer's exact payload. The one that was already blocked was blocked for an unrelated reason, so the gap was real.

Root cause, and why the previous fix was insufficient. Chaining the member onto require() is only the one-statement spelling of the attack. My #5 fix matched that syntax, so splitting it across statements put the write straight back behind an allowlisted call-site name.

The classifier now tracks the module reference rather than the syntax around it: an identifier bound to a require() of a module is followed through plain rebinding, so a chain of aliases inherits it, and every member lifted off any of them must be a read API. Computed members are refused because the name is not visible.

Verified blocked, each by execution: two-step, three-step transitive rebind, computed member off a bound ref, destructure off a bound ref, plus the five one-step shapes from the previous round. Nine total.

Verified still allowed: require('./surfaces.json').cells, a bound module ref used for a direct read, and a plain JSON require. Over-blocking honest reads is what tempts a future session to disarm the hook, so those matter as much as the blocks.

One thing worth flagging about my own first attempt at this fix. I initially built the per-identifier pattern inside a JS template literal, where \b and \s are string escapes, not regex ones. It threw at parse time — and while it threw, the hook failed open: every command sailed through. I caught it because the test suite crashed rather than failed. It is now static regex literals, which cannot fail that way. Worth knowing that this guard's failure mode under an exception is permissive.

745 assertions, 0 failures.

Separately: the Lighthouse total-blocking-time breach is now filed as #571 with the measured number (861.5ms vs 800ms budget, reproduced on two commits of this branch, which contains no application code).

[claude]

Code Review: PR #570 — chore(harness): bring the /drive work-order harness to main

Recommendation: APPROVE

Summary

This is a large (242 files, ~35k insertion) but scope-disciplined tooling-only port: the /drive work-order harness, the visual completion gate (surface-coverage-gate.mjs / rules-gate-tamper.mjs / forbid-gate-tamper.mjs), 169 generated work orders, and the reorg of .claude/rules/ into an always-loaded core.md + on-demand .claude/playbooks/. No apps/* or orbit-api application code changes (the 2 apps/* files are new empty {} suppression ledgers, as the PR body states). I independently verified the PR body's three headline fixes (empty-diff-is-a-failure and required --base in tools/check-diff-ownership.mjs; the forbid-gate-tamper wrapper-shape closes in rules-gate-tamper.mjs) and the two "changes main needed" (the typed-uppercase corpus ratchet at <=2 in test-hooks.mjs, and a clean, non-binary tools/visual-signature.mjs) — all present and consistent with the description.

Findings

Critical

None.

High

None.

Medium

[Medium] Stale /execute reference survives in the very file this PR rewrote to remove it

• dimension: Dead/stale code (correctness of the diff's own doc edit)
• location: WORKFLOW.md:21 (Step 1 table, "How" row, "Slice" column)
• issue: This PR extensively reworks WORKFLOW.md to replace /execute and /night-run with /drive and /drive --sleep everywhere (title table, Step 2, Step 3, campaign section, routing table) - .claude/skills/execute/SKILL.md is deleted in this same diff. One row was missed: "| How | /execute + the slice ladder below | ... |" still names a command that no longer exists in the repo, directly contradicting this same file's own new "At a glance" line ("/drive is the one execution skill").
• risk: An agent or human reading this table for path-picking (root CLAUDE.md names WORKFLOW.md as the mandatory pre-work read) is pointed at a deleted skill.
• fix: Change WORKFLOW.md:21 to name /drive instead of /execute, matching the rest of the file's edits.
• reference: CLAUDE.md rule 2 (delete unused code / stale references immediately)

[Medium] Orphaned test fixture left behind by the flag-new-todos removal

• dimension: Dead/stale code
• location: .claude/hooks/test-hooks.mjs:537 (const todoBad = write("apps/web/todo.ts", ...)), and the MARK constant at line 41
• issue: This PR correctly deletes .claude/hooks/flag-new-todos.mjs, its checkNewTodos export in rules-source.mjs, its settings.json wiring, its README row, and its old test assertions. But the fixture setup line that used to feed those assertions - const todoBad = write(...) - was left behind. todoBad is now assigned and never read anywhere in the file (unlike every sibling fixture on adjacent lines - tsBad, tsGood, ctrlBad, tzBad, supabaseBad, supabaseGood - which are all consumed by a runHook(...) call immediately after). The MARK constant it is built from is now used nowhere else either (one comment mention at line 216, no live use).
• risk: Dead code in the diff's own heavily-touched file, plus an unnecessary disk write during every test run.
• fix: Delete the todoBad fixture line; if MARK becomes unused as a result, delete it too.
• reference: CLAUDE.md rule 2 (delete unused code immediately)

Low / Info

None posted (signal gate).

Subagents

Not run as isolated subagents in this pass (single-context review of a tooling-only diff with no apps/* or orbit-api application-code surface):

Agent	Verdict
parity-checker	N/A - no non-ledger apps/web or apps/mobile file changed
i18n-syncer	N/A - no user-facing string / locale JSON changed
contract-aligner	N/A - no packages/shared/src/types or orbit-api DTO changed
security-reviewer	N/A - no orbit-api code changed
design-reviewer	N/A - no apps/* UI file changed

Validation

Not runnable in this sandboxed review environment (no writable path outside the repo checkout, git worktree add blocked). The PR body's own claim ("733 hook assertions pass locally") was not independently re-run; static review of the new/changed logic in rules-gate-tamper.mjs, forbid-gate-tamper.mjs, surface-coverage-gate.mjs, tools/check-diff-ownership.mjs, and the WORKFLOW.md / CLAUDE.md / .claude/hooks/_lib/README.md cross-references found them internally consistent and free of dangling references, aside from the two Medium findings above.

Check	Result
Lint	N/A - not runnable in this environment
Type check	N/A - not runnable in this environment
Tests	N/A - not runnable in this environment (PR claims 733 passing locally, not independently re-verified)
Build (api)	N/A - no orbit-api changes

Deferred - N/A dimensions & files not verdicted

• DESIGN.md/AI-slop (#8), Parity (#9), i18n (#10), Contract drift (#11), Backend hard rules (#13), FEATURES.md parity (#14) - all N/A, the diff touches no apps/* UI/parity surface, no shared contract type, and no orbit-api code (confirmed by file-list scan).
• Security (#12) - reviewed only the frontend-adjacent categories relevant to a .claude-tooling diff (no injection/authz/secrets surface found); no orbit-api code to run security-reviewer against.
• Not individually re-verdicted line-by-line: the 169 generated .claude/workorders/*.md files and .claude/manifests/surfaces.json (12,537 lines) - spot-checked structurally (parses as JSON, INDEX.md counts referenced) rather than exhaustively, since they are machine-generated output of tools/workorder.mjs / tools/surface-manifest.mjs, whose generator source was reviewed instead.
• Dynamic execution (npm test, node .claude/hooks/test-hooks.mjs) - could not run; sandbox blocks filesystem writes and git worktree outside the existing checkout. Static review only.

What's good

• The PR body is unusually honest about its own limits ("the harness has never completed a real run," "mechanical debt is a lint-count axis, not evidence a pixel moved").
• rules-gate-tamper.mjs's header comment documents four previously-shipped false-positive/false-negative bugs with their root cause, and the two bypasses fixed mid-review (module-member lift, cross-statement alias) are each pinned with a WHY-with-URL comment per the repo's own comment policy.
• The --base requirement and empty-diff-as-failure fix in tools/check-diff-ownership.mjs closes a real self-certification hole (a child could git checkout and score clean) - good root-cause fix, not a patch.
• CLAUDE.md, WORKFLOW.md, and the hooks _lib/README.md were kept in sync with the .claude/rules/ reorg almost everywhere (the one miss is the Medium finding above).

Recommendation

Approve. Fix the two Medium findings (one-line each) in this PR or a fast immediate follow-up - neither blocks merge, but both are real leftovers from the port that should not linger given how much of this PR's own value proposition is closing every drift the harness generates.

[claude]

PR Review: #570 "chore(harness): bring the /drive work-order harness to main"

Scope of this pass: read all prior claude reviews on this PR and diffed against them. The prior APPROVED review was submitted against commit e92ae6e1, but 10f0d521 ("test(harness): pin the rest of the tamper-bypass class") was pushed before that review completed, so it is genuinely unreviewed. The diff since then is small and precise:

git diff e92ae6e1 10f0d521 --stat
 .claude/hooks/test-hooks.mjs | 9 +++++++++
 1 file changed, 9 insertions(+)

Nine lines, six new assertions in the gate-tamper test block.

Recommendation: REQUEST CHANGES (1 Critical, independently verified by hand-tracing the committed source)

Critical — the new "path built by concatenation" test gives false confidence; further fragmentation still fully defeats the gate-tamper guard

• File: .claude/hooks/test-hooks.mjs:2932 (new test, in-diff) and the still-open root cause it fails to close: .claude/hooks/_lib/rules-gate-tamper.mjs:378-384 (blockedSegment) + :47-58 (MANIFEST_RE/SIGNOFF_RE/GATE_DIR_RE, etc.)
• Issue: checkGateTamperBash only inspects read/write status for a segment after one of six path regexes matches a contiguous literal substring of the raw shell text (blockedSegment: if (!pathPattern.test(segment)) continue). The new test —
  map('.claude/manifests/'+'signoff'+'.json','x') — only fragments the filename, leaving the directory prefix .claude/manifests/ intact as one literal. It passes because GATE_DIR_RE (which only requires the contiguous substring .claude/manifests, no filename needed) still matches that untouched prefix — not because path-concatenation is actually handled.
  Fragmenting the directory name too defeats every one of the six regexes at once and the command is not blocked:
  node -e "const m=require('fs'); const w=m.writeFileSync; w('.claude/manif'+'ests/sig'+'noff.json','{}')"
  Traced by hand against the exact committed source: the raw text contains .claude/manif then '+' then ests/sig then '+' then noff.json — no contiguous manifests or signoff.json token survives for SIGNOFF_RE, MANIFEST_RE, VERDICTS_RE, CALIBRATION_RE, or GATE_DIR_RE to match. All blockedSegment(...) calls in checkGateTamperBash return null, so the function returns null (not blocked) and .claude/manifests/signoff.json — the harness's one human-only completion grant, "no sanctioned agent writer... at all" per this file's own header — gets overwritten.
• Risk: this is the same failure class as bypasses #1–6 already documented and fixed in this file's own header, and it defeats the single guarantee the /drive harness rebuild exists to protect (no agent self-certification of #539 visual work). The new test actively masks this: a reader sees "path built by concatenation blocks" ✅ in the suite and reasonably concludes the concatenation class is closed, when only the shallowest instance of it was tested.
• Fix: the path-pattern check needs to run against the decoded string value where feasible (evaluate simple '...'+'...'+'...' literal concatenations before pattern-matching), or more robustly, blockedSegment should also flag any segment containing a call to a tracked write member (the moduleRefs mechanism already resolves this) regardless of whether a path regex matched — i.e. do not gate the write check behind the path regex when the callee is already known to be a write API. Add a regression test for directory-name fragmentation, not just filename fragmentation.
• Verification: independently confirmed by reading the exact committed source (git show HEAD:.claude/hooks/_lib/rules-gate-tamper.mjs) and hand-tracing blockedSegment/GATE_DIR_RE/SIGNOFF_RE/MANIFEST_RE against both the shipped test's payload and the fragmented variant above. Live node execution was not run in this session; recommend a human or a session with unrestricted node execute the payload above against checkGateTamperBash directly before merge as a final confirmation.

Carried forward from the prior APPROVED review (not new, already called non-blocking there; not re-verified here)

• WORKFLOW.md:21 — the "How" column still names the deleted /execute skill instead of /drive.
• .claude/hooks/test-hooks.mjs:537 (const todoBad = write(...)) and the MARK constant at line 41 — orphaned fixture left behind by the flag-new-todos.mjs removal.

Subagents

Diff since the last review touches only .claude/hooks/test-hooks.mjs (9 lines) — no apps/**, no i18n strings, no packages/shared types, no orbit-api, no UI surface. All five gates are N/A for this delta:

Agent	Verdict
parity-checker	N/A — no apps/web/apps/mobile file in this delta
i18n-syncer	N/A — no locale JSON / user-facing string
contract-aligner	N/A — no shared type/DTO; sibling orbit-api also not checked out in this job (not verifiable in CI)
security-reviewer	N/A — no orbit-api code
design-reviewer	N/A — no UI surface

Backward-compat guard: N/A, no schema/DTO field touched in this delta.

Validation

Skipped per instructions — Build / Unit Tests / SonarCloud run as separate required checks on this PR.

Deferred

• Cross-model second opinion (/second-opinion) on the Critical finding: unavailable in this environment.
• Live node execution of the payload above against the shipped module: not run in this session (sandbox limitation noted in three prior reviews on this PR); the finding is a hand-trace against the committed source, matched character-for-character against the regex definitions, not an observed run.
• Everything outside the 9-line delta was not re-reviewed from scratch — already covered by prior reviews on this PR; only the delta and its interaction with previously-flagged logic were audited fresh here.

[thomasluizon]

Bypass #7 closed in 53180904, and the review was right about my own test as well as the code.

Verified before fixing: three of four fragmentation shapes were ALLOWED, including writes to both signoff.json and surfaces.json. The diagnosis is exactly as traced: every path rule matches a contiguous literal in the raw text, so a fragmented path means no rule fires and the segment is never examined at all.

The criticism of my test is the more useful half. map('.claude/manifests/'+'signoff'+'.json') fragmented only the filename, so GATE_DIR_RE still matched the untouched directory prefix. It passed for the wrong reason and read as "concatenation is handled" when only its shallowest instance was. That is a test manufacturing false confidence, which is worse than no test. Replaced with directory-level fragmentation, spaced-plus, and a second target.

Adjacent same-quoted literals are joined before the path rules run, so the path is judged by the value it builds rather than how it is spelled.

One thing I found while fixing this that is worse than the finding. The adapter ended catch { process.exit(0) } — the fence failed open on any internal error. Not theoretical: my first attempt at the #6 fix built a regex inside a template literal, where \b and \s are string escapes rather than regex ones. It threw on every invocation, and every payload sailed through while it did. I caught it only because the test suite crashed instead of failing.

It now exits 2 with an explanation, proven by stubbing the rule core to throw. A command the guard cannot classify is refused rather than waved through. Note this makes the guard's failure mode strict rather than permissive, which is the correct direction for the one human-only completion grant but is a behaviour change worth knowing about.

Also carried forward: WORKFLOW.md named the deleted /execute skill and still described bundles as opening draft PRs, contradicting the ready-for-review canon; and the flag-new-todos removal left an orphaned fixture whose MARK constant no longer existed.

Current standing: 20 write shapes across four adversarial batteries refused, 5 honest reads still pass (including require('./surfaces.json').cells and a jq read). 755 assertions, 0 failures.

[claude]

[Critical] joinConcatenatedLiterals only rejoins same-quote-type literal fragments — the regex (['"])\s*\+\s*\1 requires the backreference \1, so a mixed-quote (or escaped-quote) split still fully defeats every path check in blockedSegment.

Hand-traced PoC against this exact source:

node -e "const m=require('fs'); const w=m.writeFileSync; w('.claude/manif'+\"ests/sig\"+'noff.json','{}')"

Neither + boundary matches the backreferenced regex (boundary 1 is ' then \", boundary 2 is \" then '), so the text is returned unchanged, no path regex matches, and checkGateTamperBash returns null (not blocked) — signoff.json gets overwritten with no sanctioned writer.

Fix: drop the backreference — text.replace(/['"]\s*\+\s*['"]/g, "") — since a JS + between two string literals concatenates regardless of which quote characters delimited them. Add a regression test alternating quote types across each + boundary (same shape as the existing bypass-#7 tests, which only cover same-quote fragmentation).

See full review for details.

[claude]

Code Review: PR #570 — "chore(harness): bring the /drive work-order harness to main"

Scope: PR #570, thomasluizon/orbit-ui-mobile, chore/drive-harness → main. Diffed base 478dbc1e → head 53180904 (241 files, +35135/-1149). This review reads all five prior automated review rounds on this PR and diffs against them — the diff since the last review (submitted against e92ae6e1/10f0d521) is the final commit 53180904, which claims to fix the directory-fragmentation bypass that review flagged.

Recommendation: REQUEST CHANGES

Summary

This PR ports the /drive work-order harness (tamper-guard hooks, the visual-completion gate, the drive engine, tools/*.mjs, and 168 generated per-surface work-order files) from a feature branch to main. It is infrastructure-only — the only files under apps/** are two new empty eslint-suppressions.json ledgers, so Parity/i18n/Contract-drift/Security(API)/DESIGN.md/FEATURES.md are all N/A.

The PR's history (5 prior review rounds, 3 CHANGES_REQUESTED) shows the rules-gate-tamper.mjs path-obfuscation defenses have been through repeated adversarial rounds, each finding a real bypass that got fixed in a following commit. The most recent fix, 53180904 ("judge the path by the value it builds, and fail the fence closed"), closes the specific bypass the last review found (fragmenting a path across same-quoted literals, e.g. '.claude/manif'+'ests/sig'+'noff.json') by rejoining adjacent same-quote literal fragments before pattern-matching. Hand-tracing that same fix against a mixed-quote-type variant of the identical attack shows the fix is incomplete: it closes the same-quote case but not the mixed/escaped-quote case, so the guard's central promise — no agent can self-certify .claude/manifests/signoff.json, the harness's one human-only completion grant — is still defeatable with an ordinary one-liner.

Findings

Critical

[Critical] joinConcatenatedLiterals only rejoins same-quote-type literal fragments — a mixed-quote (or escaped-quote) split still fully defeats every gate-tamper path check
· dimension: Security (tooling-security analog of injection/validation-bypass, for the harness's own guard)
· location: .claude/hooks/_lib/rules-gate-tamper.mjs (joinConcatenatedLiterals + blockedSegment, in the fix committed at 53180904)
· issue: The fix — text.replace(/(['"])\s*\+\s*\1/g, "") — requires the backreference \1: the quote character on both sides of a + must be identical. It does not rejoin a boundary where the quote types differ or where one side is an escaped quote. Hand-traced against the committed source: for
node -e "const m=require('fs'); const w=m.writeFileSync; w('.claude/manif'+\"ests/sig\"+'noff.json','{}')"
neither boundary matches (['"])\s*\+\s*\1 (boundary 1 is ' then \", boundary 2 is \" then ' — the backslash breaks the immediate adjacency the regex requires), so joinConcatenatedLiterals returns the text unchanged. No contiguous .claude/manifests or signoff.json substring then exists, so SIGNOFF_RE, GATE_DIR_RE, MANIFEST_RE, VERDICTS_RE, and CALIBRATION_RE all fail to match, blockedSegment returns null for every protected path, and checkGateTamperBash returns null (not blocked) — segmentIsReadOnly is never even reached. The four new regression tests this same commit added (test-hooks.mjs, the bypass-#7 block) only exercise same-quote fragmentation (including a "spaces around the plus" variant), so the suite's own green run does not exercise this case. This is the same root defect the last review flagged one layer up (directory-name fragmentation defeating contiguous-substring matching) — the fix narrowed the same-quote instance of it but not the general case.
· risk: An agent using an ordinary Bash one-liner that happens to mix quote styles (common when composing a JS string literal that itself needs both quote characters) can overwrite .claude/manifests/signoff.json — the file this harness's own header calls out as having "no sanctioned agent writer... at all" — or the surfaces.json manifest, undetected. This defeats the PR's stated purpose (preventing self-certified "done" on #539 visual work) with a construction well short of the "determined adversarial payload" the harness's own honest-limits section says it doesn't defend against.
· fix: Generalize the join to strip any quote character adjacent to a bare +, not just a matching pair — e.g. drop the backreference: text.replace(/['"]\s*\+\s*['"]/g, "") — since a JS + between two string literals concatenates regardless of which quote characters delimited them. Add a regression test mirroring the existing bypass-#7 tests but alternating quote types across each boundary (including the escaped-double-quote-inside-outer-double-quote shape above), and re-verify the "an honest concatenated read of an unrelated file still allows" test still passes against the generalized regex.
· verification: Independently hand-traced against the exact committed source (git show 53180904:.claude/hooks/_lib/rules-gate-tamper.mjs) and the corresponding test block in test-hooks.mjs, the same method the prior review used for the bypass it found (subsequently confirmed correct by this fix). Live node execution was not run in this sandbox (blocked); a human or an unrestricted-node session should run the PoC directly against checkGateTamperBash before merge as final confirmation.

High

None.

Medium

None.

Low / Info

None (per the rubric's Signal gate).

Subagents

No Task/Agent tool available in this execution context; each gated check was performed inline.

Agent	Verdict
parity-checker	N/A — only file under apps/** is a new empty eslint-suppressions.json ledger in both apps/web and apps/mobile (byte-identical {})
i18n-syncer	N/A — no user-facing string or locale JSON touched
contract-aligner	N/A — no packages/shared/src/types/*, no orbit-api change; sibling repo not checked out in this job (not verifiable in CI)
security-reviewer	N/A — orbit-api not touched
design-reviewer	N/A — no apps/* UI file touched

Backward-compat guard: N/A — no Zod schema / DTO field touched.

Validation

Skipped per CI-wrapper instructions. GitHub Checks show Build / Lint / Type Check / Unit Tests / Harness Hook Parity all green as of 53180904. None of those checks execute the specific bypass shown above (it requires driving checkGateTamperBash with a crafted payload, not a static scan), so green CI does not contradict this finding.

What's good

• The prior review rounds on this PR each found a real, hand-verifiable bypass in rules-gate-tamper.mjs, and every one was fixed in a subsequent commit with an honest commit message naming the exact defect and how it was verified — a well-functioning iterate-to-saturation loop.
• .claude/skills/drive/run.mjs avoids shell-injection risk entirely via spawnSync with argument arrays rather than shell strings.
• Dead code removed cleanly: flag-new-todos.mjs deleted with its settings.json wiring and its now-orphaned test fixture, in the same commit.
• CLAUDE.md's .opencode/agents/* deletion is already documented and reasoned through in this PR's own CLAUDE.md diff.

Recommendation

Fix the joinConcatenatedLiterals backreference (drop \1, generalize to any quote pair) before merge, add a regression test for the mixed/escaped-quote fragmentation shape shown above, and re-run test-hooks.mjs to confirm the fix doesn't loosen the read-only allowances. Everything else in this large, primarily-tooling PR is solid.

[thomasluizon]

Bypass #8 closed in 45b5841e, but not the way the review suggested, and the reason matters.

Verified, and worse than reported. The mixed-quote case bypasses as traced. I then went looking for siblings and found two more that also walk past the same fix: splitting the path across variables (const a='.claude/mani'; const b='fests/sig'; ...; w(a+b+c)) and assembling it with Array.join. The suggested fix — dropping the backreference to /['"]\s*\+\s*['"]/ — closes only the mixed-quote spelling and leaves both of those open.

That is the lesson of the last four rounds. #5 aliased the writer. #6 split it across statements. #7 fragmented the literal. #8 mixed the quotes. Each fix closed one spelling and the next round found the next. Working out what a path argument evaluates to inside arbitrary code is not something a regex can do, so I stopped trying: the path is no longer what decides.

An interpreter one-liner that cannot be proven a pure read is now refused whatever it names. This is not a new doctrine — it is the one this file already states, that code is judged by an allowlist of what it may call — applied at the point that previously required a path rule to fire first. There is nothing left to obfuscate, because the filename stopped mattering.

The cost, stated plainly rather than buried. A legitimate node -e that writes any file is now refused and must use the Write tool. A node -e whose require() argument is itself concatenated is refused one step earlier than before (it was already unverifiable — the tool cannot tell which module it names). Both are behaviour changes beyond gate state, and I would rather flag them than have you find them.

The reads this guard advertises still work, and I keep testing them because over-blocking is what gets a hook disarmed: node -p "require('./.claude/manifests/surfaces.json').cells.length", a bound module ref used for a direct read, cat/jq/grep, and the Read tool.

23 write shapes across five adversarial batteries refused. 761 assertions, 0 failures.

Also: I was wrong about the Lighthouse failure and have corrected #571. Four data points show it alternating pass/fail on a branch with zero application code — 861ms against an 800ms budget. It is a budget calibrated too tightly, not a regression, and I retitled the issue accordingly.

[claude]

Code Review: PR #570 — "chore(harness): bring the /drive work-order harness to main"

Scope: PR #570, thomasluizon/orbit-ui-mobile, chore/drive-harness → main. Head 45b5841e (241 files, entirely .claude//.opencode//tools//.github/workflows//root-docs harness tooling; the only apps/** change is two new empty eslint-suppressions.json ledgers).

This pass: read all six prior automated review rounds on this PR before reviewing, and scoped to the diff since the last review (53180904 → 45b5841e, one commit: "fix(harness): stop matching obfuscated paths; refuse the interpreter instead"), plus a fresh pass over dimensions no prior round covered.

Recommendation: REQUEST CHANGES — one High finding, not previously flagged, survives.

Verified: the last review's Critical finding is fixed

The prior CHANGES_REQUESTED round found a mixed-quote bypass in joinConcatenatedLiterals that let an agent write .claude/manifests/signoff.json via a node -e one-liner. Commit 45b5841e replaces the whole path-matching approach for interpreter one-liners: instead of trying to detect which path a node -e/python -c one-liner touches, checkGateTamperBash now refuses any interpreter one-liner that cannot be proven read-only by inlineCodeIsReadOnly's call-allowlist, regardless of what path it names. I hand-traced this against the exact PoC from the last review (w('.claude/manif'+"ests/sig"+'noff.json','{}') via an aliased m.writeFileSync) — the module-reference tracking (moduleRefs) still flags m.writeFileSync as a non-allowlisted call, and even if that layer had a gap, the new fail-closed design means the burden is "prove read-only," not "detect the write," which closes this bypass class structurally rather than per-payload. New tests cover the exact mixed-quote case plus two more shapes the commit found (variable-split path, Array.join) and confirm reads (node -p, jq) still pass. This is resolved.

Findings

High

[High] The new .claude/plans/* gitignore negation cannot reach the one place finished plans are actually filed
· dimension: 1 (Root cause over workarounds) / general correctness of the PR's own stated fix
· location: .gitignore:20-26, paired with .claude/skills/implement/SKILL.md:281-284 (unchanged by this PR)
· issue:

.claude/plans/*
!.claude/plans/*.plan.md

Git's documented gitignore semantics ("it is not possible to re-include a file if a parent directory of that file is excluded") mean .claude/plans/* excludes the completed directory entry itself as a whole; git never descends into an excluded directory to evaluate a nested negation, so !.claude/plans/*.plan.md can only ever re-include a .plan.md file directly inside .claude/plans/, never one inside .claude/plans/completed/. implement/SKILL.md's own Phase 9 still does mkdir -p .claude/plans/completed && mv $ARGUMENTS .claude/plans/completed/ — the one place a finished plan is filed away is exactly the directory this negation can't reach. The adjacent WHY comment says the goal is to stop "every fresh checkout with plan bundles whose Tier and acceptance criteria named a file that did not exist" — a plan that finishes and moves to completed/ recreates precisely that failure mode, silently, with no error.
· fix: use the standard nested-unignore idiom so the negation can descend into subdirectories: .claude/plans/** / !.claude/plans/**/ / !.claude/plans/**/*.plan.md.
· verification: confirmed against the actual committed .gitignore (git show HEAD:.gitignore) and the actual implement/SKILL.md Phase 9 content in this PR's tree; no later commit in this PR touches either file, so the gap is live at head. Not previously flagged in any of the six prior review rounds.

Medium

[Medium] _lib/README.md's module→hook table omits this PR's own new rules-gate-tamper.mjs
· location: .claude/hooks/_lib/README.md:14-22
· issue: the table is the canonical "what rule, what hook, which engine" map for the _lib core; the PR's largest new module (500+ lines, wired into two Claude Code matchers plus opencode) has no row, so a reader using this table to find gate-tamper's wiring won't.
· fix: add a row for rules-gate-tamper.mjs (rules: gate-state tamper protection; Claude Code hook: forbid-gate-tamper; opencode: tool.execute.before(bash)).

[Medium] The new mirror-harness skill's own source-directory list omits playbooks/, the directory this same PR creates
· location: .claude/skills/mirror-harness/SKILL.md:22
· issue: lists orbit-ui-mobile/.claude/{skills,agents,hooks,rules,manifests} as the read scope — missing playbooks, which this PR moves debugging.md, planning-and-artifacts.md, and review-and-audit.md into. The skill is stale on arrival: a run today would miss three of the four moved rule files.
· fix: add playbooks to the source-directory list.

What's good

• The joinConcatenatedLiterals-era arms race (bypasses #5–#8) is closed structurally rather than per-payload: the fail-closed "prove read-only or refuse" design for interpreter one-liners removes the whole obfuscation category rather than adding another regex.
• Each of the prior six review rounds found a real, hand-verifiable bypass, and every one was fixed in a following commit with an honest commit message naming the exact defect and how it was verified — a well-functioning iterate-to-saturation loop.
• .opencode/agents/* deletion is a well-reasoned, fully cross-referenced decision (CLAUDE.md, the harness docs, and the skill all agree), not an accidental parity break.
• Dead code removed cleanly alongside the features that replace it (no orphaned exports or stale test fixtures found in this pass).

Not verifiable in this job

• Cross-repo dimensions (contract-aligner, the orbit-api side of backward-compat) — orbit-api is not checked out here. N/A regardless: no packages/shared or API contract touched.
• Parity/i18n/DESIGN.md — no apps/** UI file touched (only two empty eslint-suppressions.json ledgers, byte-identical across web/mobile).

Validation

Skipped per CI-wrapper instructions (Build / Unit Tests / SonarCloud run as separate required checks). GitHub Checks show the "Harness Hook Parity" job green at head. That suite exercises test-hooks.mjs's own assertions; it does not independently check gitignore path-matching semantics, so green CI does not contradict the High finding above.

Recommendation

Fix the .gitignore nested-negation gap before merge (concrete, verifiable, silently drops data with no error — the same failure class the PR itself set out to close). Folding in the two Medium doc-omission fixes is optional but cheap since both files are new or directly touched by this PR.

[claude]

[High] This negation can't reach the one place finished plans are actually filed. Git's documented gitignore semantics mean .claude/plans/* excludes the completed directory entry itself as a whole — git never descends into an excluded directory to evaluate a nested negation, so !.claude/plans/*.plan.md only re-includes a .plan.md sitting directly in .claude/plans/, never one inside .claude/plans/completed/. But implement/SKILL.md's Phase 9 still does mkdir -p .claude/plans/completed && mv $ARGUMENTS .claude/plans/completed/ — exactly the directory this rule can't reach. A finished plan silently never gets committed, recreating the "fresh checkout with plan bundles whose Tier and acceptance criteria named a file that did not exist" failure this fix set out to close.

Fix: use the nested-unignore idiom so the negation can descend into subdirectories:

Suggested change

	.claude/plans/*
	!.claude/plans/*.plan.md
	.claude/plans/**
	!.claude/plans/**/
	!.claude/plans/**/*.plan.md

[thomasluizon]

The .gitignore finding was correct and is fixed in d57c88c7.

Verified with git add --dry-run rather than git check-ignore, whose exit code is ambiguous when the last matching rule is a negation — it reports a match and exits 0 even when the file ends up not ignored, which is how I would have talked myself out of this one:

	top-level plan	.claude/plans/completed/
before	staged	reported ignored
after	staged	staged

A non-plan file in the same directory still stays ignored, which is the half worth keeping.

The consequence was exactly as argued: implement/SKILL.md:282 does mkdir -p .claude/plans/completed && mv $ARGUMENTS .claude/plans/completed/, and pr-review/SKILL.md:87 reads it back, so a finished plan would silently never be committed — recreating the fresh-checkout-with-a-missing-contract failure the un-ignore was added to close.

The other two inline comments on this pass (the alias bypass at :208 and joinConcatenatedLiterals at :387) are the Criticals already closed in e92ae6e1 and 45b5841e; 45b5841e removed the path-matching approach those attacked entirely, so both are moot rather than merely patched.

[claude]

Code Review: PR #570 (orbit-ui-mobile)

Scope: PR #570 in thomasluizon/orbit-ui-mobile — "Port the /drive work-order harness to main" (241 files, +35188/-1149). This is a dev-tooling/harness PR: .claude/**, .opencode/**, tools/**, root docs, and two new empty apps/*/eslint-suppressions.json ledgers. Zero application (apps/* UI, packages/shared) logic changed.
Recommendation: NEEDS WORK

Degraded-run banner

This review ran in a single context with no subagent-spawning capability available to me (no Task/Agent tool in my toolset) and the Bash sandbox blocked every node invocation ("This command requires approval") and file-redirection outside a narrow allowlist, so the five /pr-review subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) were not run, and one Critical finding below is confirmed by manual regex trace rather than executing the code. Per review-and-audit.md rule 5, that is stated here rather than silently substituted. Given 241 changed files (mostly generated .claude/workorders/*.md and file renames), I worked worst-first: the new security-enforcement code (_lib/rules-gate-tamper.mjs, its two hook wrappers, settings.json), the new automation scripts (tools/*.mjs, .claude/skills/drive/run.mjs) for injection/secrets risk, the two parity-relevant eslint-suppressions.json files, and the doc/rule restructuring (CLAUDE.md, WORKFLOW.md, .claude/rules/*, .claude/playbooks/*) for internal consistency.

Findings

Critical

[CRITICAL] PAUSED — the one human-only disarm switch — has no bypass protection the other four protected files have
· dimension: Security (#12) / Correctness (#1)
· location: .claude/hooks/_lib/rules-gate-tamper.mjs:36 (PAUSED_RE) and its direct PAUSED_RE.test(text) call in checkGateTamperBash (vs. the blockedSegment(...) calls used for SIGNOFF_RE/MANIFEST_RE/VERDICTS_RE/GATE_DIR_RE)
· issue: SIGNOFF_RE, MANIFEST_RE, and VERDICTS_RE are all written as /(\.claude[\\/]manifests[\\/])?\bFILENAME\b/i — the directory prefix is optional specifically so a bare filename after cd .claude/manifests still matches (the file's own comment explains this: "cd .claude/manifests && rm signoff.json puts no directory in the segment that writes, so a path-only pattern would miss it" — and test-hooks.mjs:397 proves exactly this case blocks for signoff.json). PAUSED_RE, uniquely among the five patterns, has no such optional group: /\.claude[\\/]manifests[\\/]PAUSED/i requires the literal directory and filename contiguous in one string, and it is tested once against the whole raw command rather than per-segment via blockedSegment. Tracing the code by hand: cd .claude/manifests && touch PAUSED splits into segments "cd .claude/manifests" and "touch PAUSED". PAUSED_RE.test(fullText) fails (no contiguous .claude/manifests/PAUSED substring). GATE_DIR_RE (the directory-level backstop) does match segment 1, but segment 1 is cd, which is on READ_ONLY_LEADERS, so it is judged read-only and skipped; GATE_DIR_RE does not match segment 2 (touch PAUSED contains no .claude/manifests text at all), so it is never even write-checked. No other pattern is about PAUSED. Result: the command returns null (not blocked).
· risk: This is not an obfuscation — it is a completely unremarkable two-part shell command an agent could type without any intent to evade anything. It lets an agent silently fabricate .claude/manifests/PAUSED, which (a) forbid-gate-tamper.mjs exists specifically to prevent ("An agent never creates, edits, or reads-to-copy it"), and (b) surface-coverage-gate.mjs's Stop hook checks existsSync(PAUSED_PATH) and exits 0 immediately — so the same one command also silently disarms the honesty gate this whole PR's harness rebuild centers on. test-hooks.mjs has no test for cd .claude/manifests && touch PAUSED (only the un-split, no-cd touch .claude/manifests/PAUSED is covered at lines 329/569), so this gap shipped untested.
· A second, independent bypass shape hits the other four files: their bare-filename fallback is still a literal contiguous string match. joinConcatenatedLiterals only undoes JS-style 'a' + 'b' splitting (used to close bypasses #7/#8 inside node -e payloads) — it does not touch plain shell adjacent-quote concatenation (sign''off.json, a standard POSIX idiom that the shell parses identically to signoff.json). So cd .claude/manifests && rm sign''off.json is, by the same trace, also never write-checked: SIGNOFF_RE fails to match the quote-split segment, and GATE_DIR_RE again only catches the (read-only) cd segment.
· fix: Give PAUSED_RE the same optional-directory-prefix, bare-name shape as the other three (/(\.claude[\\/]manifests[\\/])?\bPAUSED\b/i) and route it through blockedSegment(text, PAUSED_RE, null) instead of a raw whole-text .test(). Separately, either (a) unquote each segment before path-matching (strip adjacent same-type quotes the way withoutQuotedText does, but for path-detection rather than write-detection), or (b) extend the already-stated doctrine one more step: since inline-interpreter code that "cannot be proven read-only" is refused outright regardless of what it names, do the analogous thing for bash segments — a segment naming a write verb (WRITE_VERB_RE) that touches any filename fragment plausibly inside .claude/manifests/.artifacts/surfaces (post cd) should fail closed too, not just when the full literal path is spelled in one segment. Add test-hooks.mjs cases for both: cd .claude/manifests && touch PAUSED, cd .claude/manifests && rm sign''off.json, cd .claude/manifests && rm surf''aces.json.
· reference: this PR's own stated purpose (closing gate-tamper bypasses, per the PR body's "five review rounds" and bypasses #1–#8 in code comments); CLAUDE.md "Agent tool scoping" (shell-allowlist-must-allowlist-arguments-not-just-prefixes doctrine, which this file otherwise follows correctly for the other four files)

High

None survive as a separate finding — the second bypass shape above is grouped into the Critical finding since it is the same file/mechanism/component family (review-and-audit.md rule 7).

Medium

[MEDIUM] flag-new-todos.mjs (untracked TODO/FIXME/HACK/XXX/BUG marker gate) is deleted with no stated rationale
· dimension: Correctness (#1) / Dead-code discipline in spirit (product-and-content.md rule 4: "record why a capability was removed")
· location: .claude/hooks/flag-new-todos.mjs (deleted), .claude/hooks/_lib/rules-source.mjs (checkNewTodos removed), .claude/settings.json (PostToolUse entry removed), .claude/hooks/test-hooks.mjs (its 3 tests removed)
· issue: Every other removal in this PR carries an explicit WHY — either a paragraph in the PR body (the #539 typed-uppercase ratchet, the NUL-byte fix, the .opencode/agents/ mirror deletion documented in the new CLAUDE.md text) or a code comment (the bypass-numbered comments throughout rules-gate-tamper.mjs). This removal has neither: it is not mentioned in the PR description at all, and no comment anywhere in the diff explains why the marker gate was cut rather than ported alongside everything else.
· risk: flag-new-todos was a real standing quality gate (it blocked new TODO/FIXME/HACK/XXX/BUG markers without an issue reference). Removing it silently means future sessions can reintroduce untracked workaround markers with nothing catching it, and a reader of this PR cannot tell whether that was an intentional trade (e.g., "too many false positives," "superseded by X") or an accidental drop during the port.
· fix: Either restore the hook (it is a small, self-contained file with no apparent interaction with the gate-tamper work) or add one sentence to the PR description / a code comment stating why it was cut.
· reference: CLAUDE.md rule 2 ("delete unused code" — this is the inverse: removing used, working code needs a stated reason); .claude/rules/product-and-content.md rule 4 (spirit)

[MEDIUM] WORKFLOW.md self-contradicts on whether /drive produces draft or ready-for-review PRs
· dimension: Correctness (#1) — internal documentation consistency
· location: WORKFLOW.md:8-9 vs. WORKFLOW.md:33-34 (both edited in this diff)
· issue: "The paths" table says both /drive (attended) and /drive --sleep end at "ready-for-review PR per bundle" / "ready-for-review PRs + an independent verifier verdict." Twenty-five lines later, the "Step 2" table (also touched by this PR) says attended /drive ends at "draft PRs you gated at each bundle" and --sleep ends at "draft PRs + an independent verifier verdict; nothing merges." These two tables in the same file, edited in the same diff, give opposite answers to the same question.
· risk: implement-opus.md/implement-sonnet.md (also new/changed in this PR) are unambiguous and consistent with each other: "Open a PR ready for review (gh pr create, never --draft)... a draft PR silently skips reviewers, so the work sits unassessed." So the Step 2 table's "draft PRs" language is very likely stale copy carried over from the pre-consolidation /night-run behavior (which genuinely always drafted) and never updated when the two tables were both touched — a reader following WORKFLOW.md alone cannot tell which of the two is true, and picking the wrong one means either not noticing your PRs never get reviewed, or expecting a review gate that was never coming.
· fix: Change WORKFLOW.md:33-34's "Ends at" row to "ready-for-review PR per bundle" / "ready-for-review PRs + verifier verdict," matching the top table and the agent specs.
· reference: CLAUDE.md docs-registry rule ("When you change a doc, update its At a glance header... in the same edit" — the same standard applies to not leaving two tables in one doc disagreeing)

Subagents

Agent	Verdict
parity-checker	N/A — not run (no subagent-spawn capability this session); manually verified apps/mobile/eslint-suppressions.json and apps/web/eslint-suppressions.json are both {}, symmetric
i18n-syncer	N/A — diff adds/changes no user-facing strings or packages/shared/src/i18n/*.json
contract-aligner	N/A — diff touches no packages/shared/src/types/* or endpoints.ts
security-reviewer	N/A — diff touches no orbit-api code
design-reviewer	N/A — diff touches no apps/* UI files (the two eslint-suppressions.json are config, not UI)

Validation

Check	Result
Lint	N/A — not run; the local working tree in this sandbox does not reflect this PR's HEAD (git object database has the PR's blobs via git show, but the checked-out files are the pre-merge base), so a local npm run lint here would not test this diff
Type check	N/A — same reason
Tests	N/A — same reason; PR body claims "733 hook assertions pass locally" and the new CI job (.github/workflows/test.yml) installs typescript then runs node .claude/hooks/test-hooks.mjs plus a new work-order-ledger-freshness check, both of which read as reasonable additions on inspection
Build (api)	N/A — orbit-api untouched

Deferred — N/A dimensions & files not verdicted

• DESIGN.md/AI-slop (#8), Parity (#9) mirror-behavior check beyond the two ledgers, i18n (#10), Contract drift (#11), Backend hard rules (#13), FEATURES.md parity (#14): all N/A, surface not touched by this diff (tooling-only PR; PR body itself states "Zero application code changes").
• The 169 .claude/workorders/*.md files and .claude/manifests/surfaces.json (12,537 lines): not individually read — these are stated to be machine-generated by tools/workorder.mjs / tools/surface-manifest.mjs and are now guarded by a new CI freshness check (regenerate + diff against HEAD); spot-checked that the generator/freshness-check wiring is coherent, did not verify every generated file's content.
• .claude/hooks/test-hooks.mjs (2523 additions): spot-checked the sections covering rules-gate-tamper.mjs (found the PAUSED/quote-split gaps above) and the drive-queue prompt-contract tests; did not read the full 2523-line diff line by line.
• .claude/skills/drive/run.mjs (1187 new lines) and the new tools/*.mjs scripts: scanned for console.log/execSync/eval/hardcoded-secret patterns and confirmed all subprocess calls use spawnSync/execFileSync with array args (no shell: true, no string-built commands) — did not review full control flow line by line.
• .claude/skills/plan/SKILL.md, .claude/skills/implement/SKILL.md, .claude/skills/mirror-harness/SKILL.md (new), .claude/skills/provider-update/SKILL.md (new), .claude/workflows/audit.mjs, .claude/workflows/prod-readiness.mjs: not reviewed in detail (lower blast radius — orchestration prose/config, not new enforcement logic).
• .opencode/plugin/orbit-guardrails.js: confirmed it wires the new rules-gate-tamper.mjs (checkGateTamperBash/checkGateTamperEdit) so the opencode side stays covered; did not re-derive the full plugin.

What's good

• The PR is unusually well-documented for its size: the body names the exact prior defects (empty-diff false-pass, working-tree-vs-explicit-range judging, the tamper wrapper shapes) and the code comments in rules-gate-tamper.mjs narrate the bypass history (#1–#8) with enough specificity to audit against — that history is exactly what let me find the PAUSED gap by extending the same pattern the file itself documents.
• Every subprocess invocation I checked in the new tools/*.mjs and drive/run.mjs uses spawnSync/execFileSync with array arguments, not string-interpolated shell commands — no command-injection surface introduced.
• The two eslint-suppressions.json ledgers are correctly symmetric empty objects, consistent with the PR's stated "design work excluded, ledgers land empty" scope.
• The .opencode/agents/ mirror-deletion story is coherently documented in both CLAUDE.md and .claude/hooks/_lib/README.md, and I initially mis-flagged the new Claude-only agents (completeness-critic, implement-opus, implement-sonnet) as missing required opencode mirrors before finding this PR's own CLAUDE.md edit explicitly retracts that requirement — worth calling out since it means the documentation update and the code change are actually consistent with each other on that point.
• Dead-code hygiene on the flag-new-todos removal itself is clean (no dangling references to checkNewTodos or the hook filename survive anywhere in the diff) even though the removal's rationale is missing (see Medium finding).

Recommendation

Fix the Critical PAUSED/bare-filename-bypass finding before merge — it defeats the one human-only control this whole harness rebuild is designed around, with an unremarkable, non-adversarial command shape, and ships with zero test coverage for the gap. The two Medium findings (undocumented flag-new-todos removal, WORKFLOW.md self-contradiction) are worth a follow-up commit in the same PR but would not block on their own.

[thomasluizon]

All three findings fixed in 8112a222. The Critical is the best defect of this whole series, because it needed no cunning at all.

Verified before fixing: four of five shapes were allowed, including cd .claude/manifests && echo x > PAUSED. The trace is exactly right — PAUSED_RE was the only one of the five patterns without the optional directory prefix, and the only one tested against the whole raw command rather than per segment, so the cd segment read clean and the touch segment carried no directory text to match.

What makes it the most serious one: every earlier bypass (#5 through #8) required deliberately hiding something. This is a command someone types by accident, and it silently creates the human-only disarm switch that makes the Stop gate exit immediately.

PAUSED now has the same optional-prefix bare-name shape as signoff/surfaces/verdicts and is judged per segment. A side effect worth naming: reads of PAUSED are now allowed, where the whole-text test blocked them. That retires a real false positive — an ordinary [ -f .claude/manifests/PAUSED ] existence test was blocked earlier in this very session. A guard that refuses honest reads is one a future session disarms, so I consider that a fix rather than a regression, but it is a behaviour change.

The sibling you grouped in was right too: plain shell adjacent-quote concatenation (sign''off.json, which the shell parses identically) split the literal for the other four. Empty quote pairs are now spliced out before path matching, alongside the JS concatenation join. Both rm sign''off.json and rm surf''aces.json now block.

On flag-new-todos — the rationale exists, it just was not visible from this PR. From baef43ff: "Deleted: flag-new-todos.mjs (local/no-comments and an orbit-api CI grep both ban a superset, and it permitted a TODO #123 form that both then reject)". So it was removed as redundant and weaker than the two gates that remain, not dropped in the port. Fair criticism that a reader of this PR alone could not tell; saying it here so the record carries it.

WORKFLOW.md said draft in seven places, not two — the second table plus five more in prose, all contradicting the top table and the implement agent specs (which state a draft PR silently skips reviewers). All seven now say ready-for-review.

766 assertions, 0 failures.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR 570 - chore(harness): bring the /drive work-order harness to main

Scope: PR #570 in thomasluizon/orbit-ui-mobile (chore(harness) port, 241 files, +35206/-1149)
Recommendation: APPROVE

Summary

This is a repo-tooling-only port (zero application code changes beyond two new empty eslint-suppressions ledgers) that brings the /drive work-order harness, its tamper-guard hooks, and 169 generated work-order files to main. The PR already went through 5 documented adversarial review rounds (31 confirmed Critical/High defects fixed) before reaching this state. I independently re-derived the diff (git diff main-ref..pr-570) and audited the highest-blast-radius surfaces directly: the new checkGateTamperBash/checkGateTamperEdit security guard (.claude/hooks/_lib/rules-gate-tamper.mjs), its hook wrapper and settings.json wiring, the opencode plugin mirror, the CI workflow change, .gitignore/.gitattributes fixes, package.json script wiring, the new implement-opus/implement-sonnet agent frontmatter, and the .claude/rules → .claude/rules + .claude/playbooks restructure. No Critical or High findings survived scrutiny.

One initial suspicion (all 8 .opencode/agents/*.md mirrors deleted with no replacement) turned out to be a locked, already-documented decision (CLAUDE.md "Conventions & tooling" is updated in this same diff to say so, and .opencode/plugin/orbit-guardrails.js still enforces the same _lib rule core). A second suspicion (drive/SKILL.md claiming specs are "gitignored" vs .gitignore no longer ignoring .claude/specs/) also resolved clean once diffed at the correct commit — the PR's own diff updates that exact line to "committed", matching .gitignore.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• Dangling doc cross-reference. .claude/rules/visual-delivery.md:10 and .claude/rules/product-and-content.md:11 each end their "At a glance" line with "See README.md for the tier's contract." Both files stayed in .claude/rules/ (as path-scoped auto-loaders) while .claude/rules/README.md was deleted and its content moved to the new .claude/playbooks/README.md (which now documents the whole tier, including these two files). The bare relative reference no longer resolves in .claude/rules/; it should read ../playbooks/README.md. Purely a doc pointer, no functional or gate impact — not blocking.

Subagents

Agent	Verdict
parity-checker	PAIRED (only touched apps/ files are apps/web/eslint-suppressions.json / apps/mobile/eslint-suppressions.json, both empty {}, identical)
i18n-syncer	N/A (no i18n JSON changed)
contract-aligner	N/A (no packages/shared/src/types/*, no orbit-api changes — single-repo diff)
security-reviewer	N/A (no orbit-api code changed)
design-reviewer	N/A (the two touched apps/* files are non-UI JSON config ledgers, not DESIGN.md-governed surfaces)

Validation

Check	Result
Lint	N/A — this review environment's sandbox blocked directory creation outside the ambient (inconsistent) working tree, so a clean pr-570 checkout could not be built to run /validate independently. Relying on the PR's own stated evidence instead (see below).
Type check	N/A (same reason)
Tests	N/A (same reason) — PR body + latest commit message state the hook-parity suite (node .claude/hooks/test-hooks.mjs) is at 740/740 assertions locally; .github/workflows/test.yml runs this same suite plus a new "Work-order ledger freshness" job (regenerates tools/workorder.mjs output and diffs against HEAD) in CI.
Build (api)	N/A (no orbit-api changes)

Deferred - N/A dimensions & files not verdicted

• 169 .claude/workorders/*.md files: homogeneous, machine-generated (tools/workorder.mjs) from main's own router/component graph. Spot-checked 2 (route-login.md, and the INDEX) for template correctness — well-formed, generatedFrom SHA correctly pins to main's actual HEAD (478dbc1e...). Full-corpus correctness is enforced by the new CI "Work-order ledger freshness" job (byte-identical regeneration check), so hand-verifying all 169 would duplicate that gate.
• tools/*.mjs new scripts (drive-queue.mjs, check-diff-ownership.mjs, capture-surfaces.mjs, judge-surfaces.mjs, calibrate-judge.mjs, contact-sheet.mjs, seed-visual-fixture.mjs, surface-manifest.mjs, visual-signature.mjs, drive-ab-report.mjs, workorder.mjs, .claude/skills/drive/run.mjs): confirmed all child-process invocation uses spawnSync/spawn with array args (no shell-string interpolation, no eval/new Function), and the disallowedTools/child-spawn config in run.mjs is a legitimate CLI-flag use per CLAUDE.md's agent-scoping rule (not the agent-frontmatter Bash(...) fail-open trap). Did not line-by-line audit the full ~5,000 lines of tool logic beyond the invariants above and the specific claims verified (ratchet test, gitignore/gitattributes rationale, freshness gate) — this is the honest limit of this pass given the diff's size.
• CI status of PR 570 itself: could not be read in this session (gh pr checks was blocked by the sandbox's approval gate non-deterministically); not used as evidence either way.

What's good

• The tamper-guard (rules-gate-tamper.mjs) is exceptionally well-documented: every one of the 9 numbered "BYPASS" comments names the exact defect, the PR that found it, and the fix's reasoning — a genuinely rare level of self-documenting security logic.
• .gitattributes/.gitignore changes each carry a WHY comment tracing to a concrete observed failure (CRLF corrupting work orders on Windows; plans directory swallowing completed/ via ** semantics), not speculative hardening.
• The .claude/rules → core.md (always-loaded) + .claude/rules/*.md (path-scoped auto-load) + .claude/playbooks/*.md (on-demand) three-way split is thoughtful and reduces the always-loaded budget from 738 lines/turn to ~50, with the reasoning recorded in .claude/playbooks/README.md.
• Deletion of .opencode/agents/* mirrors, flag-new-todos.mjs, /execute, and /night-run all check out clean — no dangling imports, no stale references outside intentional "replaces X" historical mentions.

Recommendation

Merge as-is. Optionally fix the two dangling README.md references noted above in a trivial follow-up (not blocking).