chore(harness): three memory-derived guardrail gates - #556
Conversation
Convert three operational memories into deterministic dual-target gates (pure _lib rule -> Claude Code hook + .opencode plugin + test-hooks), so the knowledge is enforced structurally instead of living only in recallable memory. - mobile supabase-lazy (checkMobileSupabaseLazy, rules-source.mjs): blocks a module-scope throw or top-level createClient() in apps/mobile/**/supabase.ts; eager module-eval crashes to a grey screen at launch (#172/#174). - EF-migration IF-NOT-EXISTS (checkEfMigrationRawIndex, rules-source.mjs): blocks a raw migrationBuilder.Sql CREATE INDEX / DROP INDEX lacking IF [NOT] EXISTS in orbit-api Migrations; EF runs migrations at startup on Render and a duplicate raw index throws Postgres 42P07, failing the deploy. Leaves CreateIndex() alone. - worktree junction guard (checkGitWorktreeRemove, rules-git.mjs): blocks the forced form of git worktree remove; on Windows it follows a junction and deletes the link TARGET. Strips heredoc bodies so a message naming the flag is not a false positive. Wired into the existing git-guardrails PreToolUse. Gate: node .claude/hooks/test-hooks.mjs is green (existing + new assertions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #556 (chore(harness): three memory-derived guardrail gates)
Scope: PR #556 in thomasluizon/orbit-ui-mobile
Recommendation: REQUEST CHANGES
Summary
Converts three operational memories into dual-target _lib gates (Claude Code hooks + .opencode plugin + test-hooks.mjs). The wiring pattern is sound and matches existing conventions. However, two of the three new gates have verified false-negative gaps that let the exact anti-pattern each targets slip through when written in ordinary, idiomatic style — one of them matching the real style already used in the file the gate protects. Diff touches only .claude/hooks/**, .opencode/plugin/**, .claude/settings.json — no apps/*/packages/shared/orbit-api source, so parity/i18n/contract/design/backend/FEATURES.md dimensions are all N/A.
Findings
Critical
None.
High
[High] Mobile-Supabase-lazy guard's regexes miss the realistic form of the anti-pattern it targets
· location: .claude/hooks/_lib/rules-source.mjs (checkMobileSupabaseLazy, added this PR)
· issue: /^throw\b/gm only matches a bare, zero-indentation throw at column 0 — it misses if (!SUPABASE_URL) throw new Error(...) or an indented throw inside a block, the only way prettier would format the guard-clause style. The createClient regex (const\s+\w+\s*=\s*createClient) requires nothing between the identifier and =, so it misses a typed declaration: export const supabase: SupabaseClient = createClient(url, key). That typed style is exactly what apps/mobile/lib/supabase.ts already uses today (let client: SupabaseClient | null = null) — the very file this gate protects.
· risk: A future regression written in ordinary TypeScript/prettier style (guard-clause throw, or a typed const matching the surrounding file's existing conventions) sails through silently, recreating the #172/#174 grey-screen launch crash this gate exists to prevent, while the passing gate gives false confidence. The added test-hooks.mjs cases only exercise the bare, untyped forms — the gap isn't covered by the new tests either.
· fix: Track brace/paren depth instead of anchoring to column 0 so an indented or guard-clause throw inside module scope still counts; allow an optional type annotation in the createClient regex: const\s+\w+\s*(?::\s*[\w.<>[\]]+\s*)?=\s*createClient\s*\(.
[High] EF raw-index idempotency guard checks a whole Sql() call as one blob, not per statement
· location: .claude/hooks/_lib/rules-source.mjs (checkEfMigrationRawIndex, added this PR)
· issue: IF NOT EXISTS/IF EXISTS is tested once against the entire captured sql text per migrationBuilder.Sql(...) call. A batched call with two statements — e.g. migrationBuilder.Sql(@"CREATE INDEX ix_a ON foo (a); CREATE INDEX IF NOT EXISTS ix_b ON foo (b);") — has ix_a's non-idempotent statement masked by ix_b's clause appearing anywhere else in the same string, so no finding fires for ix_a.
· risk: The exact Postgres 42P07 deploy failure the PR's own commit message cites ("a raw CREATE INDEX for an index that already exists throws Postgres 42P07 and fails the deploy") ships silently for any migration batching more than one raw index statement per Sql() call — a plausible EF Core pattern. No test in this PR covers a multi-statement Sql() call.
· fix: Split the captured sql on ; (or iterate per CREATE/DROP INDEX match) and check each statement's own idempotency clause independently.
Medium
[Medium] Worktree-removal block message cites a nonexistent CLAUDE.md section
· location: .claude/hooks/_lib/rules-git.mjs (checkGitWorktreeRemove, added this PR)
· issue: The block message ends "See the SAFE worktree-junction cleanup order in CLAUDE.md" — verified via grep that root CLAUDE.md contains zero occurrences of "junction" or "SAFE worktree" anywhere, and CLAUDE.md isn't part of this diff.
· risk: Anyone blocked by the gate is pointed at guidance that doesn't exist.
· fix: Add the safe cleanup order to CLAUDE.md in this PR, or inline the guidance directly in the message instead of pointing at a missing section.
[Medium] --force/-f detection isn't scoped to the same command segment as worktree remove
· location: .claude/hooks/_lib/rules-git.mjs (checkGitWorktreeRemove, added this PR)
· issue: Unlike checkGitCommand's push logic in the same file, which splits the command on /[&|;\n]/ and judges each segment against its own target, checkGitWorktreeRemove tests worktree\s+remove and --force/-f against the whole scannable string with no segment scoping.
· risk: git worktree remove path && npm test -- --force false-blocks with a misleading "worktree junction footgun" message, even though the actual worktree remove carries no force flag.
· fix: Split on /[&|;\n]/ first (mirroring checkGitCommand), then test each segment independently for both the worktree remove and the force flag.
Low / Info
None (signal gate).
Subagents
All N/A — no apps/web, apps/mobile, packages/shared/src/types, or orbit-api files changed.
Validation
Lint/Type/Build/Tests: skipped per this workflow's CI adaptation (Build / Unit Tests / SonarCloud run as separate required checks on this PR). Findings verified via direct diff read against the PR commit (027d0467) and cross-check against the real apps/mobile/lib/supabase.ts file and checkGitCommand's existing segment-scoping pattern in the same file.
Deferred
Parity, i18n, contract drift, DESIGN.md, backend hard rules, FEATURES.md — all N/A, diff is harness-only.
What's good
Clean dual-target _lib wiring identical to existing conventions; correct reuse of heredoc-body stripping with a regression test against a commit message that names the flag as data, not a command; solid test coverage added on both engines for all three gates (even though narrower than the real anti-pattern, per the High findings above).
Recommendation
Request changes: fix both High findings before merge — each is a false negative in a gate whose entire purpose is preventing a specific, previously-shipped incident from recurring, and as written both can be defeated by the most natural / already-in-use way to write the exact anti-pattern targeted. Fold in the two Medium fixes in the same pass.
Addresses the #556 review — each finding was a verified false negative in a gate whose purpose is preventing a specific prior incident from recurring. - supabase-lazy (High): the `throw` check anchored to column 0, missing an indented / `if (!x) throw` guard-clause form; the createClient regex rejected a typed const `export const supabase: SupabaseClient = createClient(...)` — the exact style the real apps/mobile/lib/supabase.ts already uses. Now a string/comment-aware bracket-depth scan flags any module-scope throw, and the regex allows an optional type annotation. The lazy `() => createClient` arrow still passes (a new test pins that no false positive was introduced). - ef-migration-idempotency (High): IF [NOT] EXISTS was tested once against the whole Sql() blob, so one idempotent statement masked a sibling raw index in a batched call. Now each `;`-separated statement is checked independently. - worktree-junction (Medium x2): force detection is now segment-scoped like checkGitCommand, so `git worktree remove path && npm test -- --force` no longer false-blocks; and the block message inlines the SAFE cleanup order instead of pointing at a CLAUDE.md section that does not exist. Gate: node .claude/hooks/test-hooks.mjs green (existing + 6 new regression assertions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed all four findings in 1ef4e5a with regression tests for each:
|
|
There was a problem hiding this comment.
Code Review: PR #556 — chore(harness): three memory-derived guardrail gates
Scope: PR #556 in thomasluizon/orbit-ui-mobile (chore/harness-gates → main)
Recommendation: APPROVE
Summary
Converts three previously-memory-only guardrails (mobile Supabase eager-init, EF raw-index idempotency, git worktree remove --force junction footgun) into deterministic _lib rules enforced by both the Claude Code hook engine and the .opencode plugin, with parallel unit/hook/plugin regression tests in test-hooks.mjs. This PR already includes a self-correction: commit 1ef4e5a7 fixed three false-negatives an earlier review pass found (column-0-anchored throw detection, an untyped-only createClient regex, and whole-blob instead of per-statement IF [NOT] EXISTS checking). I independently re-verified the fixed logic by hand-tracing the bracket-depth scanner, the per-statement SQL split, and the segment-scoped force-flag match against the new test cases, and found no remaining correctness gap.
Findings
Critical: None
High: None
Medium: None
Low / Info: None (no nits worth posting — see notes below, kept out of the findings list per the signal gate)
Two things I checked and deliberately did not turn into findings, in case a human reviewer wants the context:
_lib/rules-git.mjsand_lib/rules-source.mjscarry several multi-line//WHY-style narration comments on non-exported helpers (e.g.moduleScopeThrowLines) without a URL. Strictly read, dimension 4 would flag these, but.claude/hooks/**isn't covered by anyeslint.config.*in the repo (onlyapps/web,apps/mobile,packages/sharedare), and this exact comment style is pervasive throughout the pre-existing file (e.g. the "Fails SAFE" and "heredoc body is data" comments already inrules-git.mjsbefore this PR). The new code matches established convention for this subsystem, not a new deviation — not worth flagging.- The two new whole-file checks (
checkMobileSupabaseLazy,checkEfMigrationRawIndex) match paths via a regex requiring a literal leading/(e.g./\/apps\/mobile\/.*supabase\.ts$/), so if the.opencodeplugin ever passes a relativefilePath(rather than absolute), path matching would silently no-op. This is inherited unchanged from every existing sibling check (checkCsharpAuthz,checkCsharpTimezone, etc.) — the PR doesn't touch how paths are resolved, so it's not a regression this diff introduces.
Subagents
None fired — the diff touches only .claude/hooks/**, .claude/settings.json, and .opencode/plugin/orbit-guardrails.js. No apps/web/apps/mobile file (parity-checker N/A), no user-facing string or i18n JSON (i18n-syncer N/A), no packages/shared/types or dual-repo change (contract-aligner N/A), no orbit-api source (security-reviewer N/A), no UI file (design-reviewer N/A).
Validation
Not run — this CI wrapper skips Phase 6 (/validate) by instruction since Build / Unit Tests / SonarCloud already run as separate required checks on this PR. I verified the new logic by static trace instead of execution. The PR body states the author ran it locally: ORBIT HOOK PARITY OK.
| Check | Result |
|---|---|
| Lint | Skipped — covered by required CI checks |
| Type check | N/A (plain .mjs, no TS in this diff) |
| Tests | Skipped — covered by required CI checks; traced by hand instead (see Summary) |
| Build (api) | N/A — no orbit-api code touched |
Deferred — N/A dimensions & files not verdicted
- #8 DESIGN.md/AI-slop — N/A, no
apps/*UI file changed. - #9 Parity — N/A, no
apps/web/**orapps/mobile/**file changed. - #10 i18n — N/A, no user-facing string or locale JSON changed.
- #11 Contract drift/backward-compat — N/A, no
packages/shared/src/types/*or orbit-api DTO changed. - #13 Backend hard rules — N/A, no
orbit-apisource changed (the two new checks only reference orbit-api path patterns inside JS regex strings). - #14 FEATURES.md parity — N/A, pure dev-tooling change, no user-facing feature surface.
All 8 changed files (_lib/rules-git.mjs, _lib/rules-source.mjs, the two new forbid-*.mjs adapters, git-guardrails.mjs, test-hooks.mjs, settings.json, .opencode/plugin/orbit-guardrails.js) were read in full and given a verdict. Nothing deferred beyond the N/A dimensions above.
What's good
- The two new adapter files (
forbid-mobile-supabase-eager.mjs,forbid-ef-migration-raw-index.mjs) are byte-for-byte template matches of the existingcsharp-authz.mjspattern — good consistency discipline. checkGitWorktreeRemovecorrectly reusesstripHeredocBodiesand is segment-scoped likecheckGitCommand, so a commit message naming--forceor a later unrelated--forcein a chained command doesn't false-block — and there's a regression test for exactly that (the PR's own commit message names the flag, which is a nice self-test).- The EF-index per-statement split correctly prevents one idempotent statement in a batched
Sql()call from masking a sibling non-idempotent one — hand-traced the exact multi-statement test case and the split/regex logic produces the right per-statement segments. - Both hook engines (
.claude/hooksand.opencode/plugin) were updated in the same diff for all three gates, honoring the dual-target enforcement contract this whole PR is about. - The fix commit (
1ef4e5a7) is a genuine, well-targeted correction of a prior review's findings with new regression tests pinned to each fix — good self-review discipline already baked into this PR.
Recommendation
Merge as-is. No Critical/High findings survived review; the diff is internally consistent, narrowly scoped to the harness's own guardrail files, and the one prior review round's false-negatives were already closed with matching regression tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
1 participant
What
Converts three operational memories into deterministic dual-target gates — a pure
_librule enforced by both the Claude Code hook engine and the.opencodeplugin, proven bytest-hooks.mjs. The knowledge stops living only in recallable memory and becomes structural (guardrails-over-memory-rules)._lib)checkMobileSupabaseLazy(rules-source.mjs)throwor top-levelcreateClient()inapps/mobile/**/supabase.tscheckEfMigrationRawIndex(rules-source.mjs)migrationBuilder.SqlCREATE INDEX/DROP INDEXlackingIF [NOT] EXISTSin orbit-api MigrationscheckGitWorktreeRemove(rules-git.mjs)git worktree removeWhy each
getSupabaseClient()accessor.CREATE INDEXfor an index that already exists throws Postgres 42P07 and fails the deploy. Mirrors the Guard-Migrations CI.migrationBuilder.CreateIndex(...)is already idempotent-safe and is left alone.--forcefollows a junction/reparse-point inside the worktree and deletes the link TARGET, not the link.rmdir+ verify each junction first, then remove.Wiring (per gate, dual-target)
forbid-mobile-supabase-eager.mjs+forbid-ef-migration-raw-index.mjs, registered insettings.json.git-guardrails.mjsPreToolUse adapter (chained aftercheckGitCommand)..opencode/plugin/orbit-guardrails.js(tool.execute.afterfor source,.beforefor git).stripHeredocBodiesso a commit message naming the flag is data, not a false positive (regression-tested; this PR's own commit message exercises it).Verification
```
$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
```
All existing assertions plus the new
_libunit, real-file Claude Code hook, and opencode-plugin assertions for the three gates pass.The three now-redundant memory files live outside the repo and are removed separately (not part of this diff).
🤖 Generated with Claude Code