fix(harness): scope the git push guard to the Orbit repos, stop matching heredoc bodies - #541
Conversation
…ing heredoc bodies Two false positives in the same guard, both from matching text that is not the command being run. 1. The push rules were repo-agnostic. Any `git push ... main` blocked, whatever repo it targeted. Sessions always launch from orbit-ui-mobile and drive sibling repos from there, and two of those are direct-to-main by design: the brain vault (whose daily cron literally does `git add -A && git commit && git push` on main) and thomas-brain. Pushing to either was impossible, which is why the standing workaround for thomas-brain was to hand the push back to the user via the `!` shell. The guard now resolves the target repo from origin's remote URL and enforces branch protection only for orbit-ui-mobile, orbit-api and orbit-landing-page. Resolving the target is the subtle part: the session cwd is NOT the target when the command cds first (`cd /c/brain && git push origin main` reports cwd as orbit-ui-mobile), so it honors `git -C <dir>`, then a `cd <dir> &&` earlier in the same chain, then cwd. 2. The rules matched the whole command string, heredoc bodies included, so writing a commit message ABOUT a banned flag tripped the guard. This very commit could not be written before this fix. Heredoc bodies are data, not flags, and are now stripped before matching. A heredoc feeding a shell is the exception, since there the body IS commands. Both fail safe. An unresolvable or throwing remote resolver is treated as protected: wrongly blocking a push is recoverable, wrongly allowing one onto main is not. Callers that pass no resolver keep blocking exactly as before. Hygiene rules stay universal: the no-verify, no-gpg-sign and `commit -n` bypasses are wrong in any repo, so they are checked before the repo scope is considered. Both adapters inject the resolver off the same _lib, so Claude Code and opencode stay in parity. 12 new unit tests cover the scoping, cd/-C resolution, heredoc stripping, and every fail-safe path. Full suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5p8K4UtdpYRHX5eQcVSRz
…n consumer The exception was a whole-string search, so any text containing the shell heredoc form switched stripping off for the entire command. Writing a PR body that DOCUMENTS the exception therefore re-armed the bug it documents, and the push guard then matched the prose in that body. Same text-is-not-command defect as the two it was meant to fix, one level down. The check now looks only at the consumer immediately preceding each `<<`, so a body that merely mentions the form cannot disable its own stripping. Bodies fed to a real shell still keep their contents in scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5p8K4UtdpYRHX5eQcVSRz
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #541 — fix(harness): scope the git push guard to the Orbit repos, stop matching heredoc bodies
Scope: PR #541 in thomasluizon/orbit-ui-mobile (fix/git-guardrails-scope-to-orbit-repos → main)
Recommendation: NEEDS WORK
Summary
This PR scopes the git-guardrails push guard to the three protected Orbit repos, strips heredoc bodies from the flag-matching text (with a shell-consumer exception), and adds 13 new unit tests. The heredoc handling and repo-name resolution are careful and correctly traced against every new test case. However, the new repo-scoping logic determines "is this push protected" from only the first git push occurrence in a chained bash command, then returns null (allow) for the entire command string if that first push targets a non-protected repo — silently skipping evaluation of any later git push in the same chain, including one that targets main in a protected repo. This is a newly-introduced bypass of the guard's core purpose, not present before this diff, and it lines up exactly with the chained cd <sibling-repo> && ... && cd <back> && ... pattern the PR itself describes as routine. The diff touches only harness tooling (.claude/hooks/, .opencode/plugin/) — no apps/*, packages/shared, or orbit-api surface — so most rubric dimensions (parity, i18n, contract drift, DESIGN.md, backend hard rules, FEATURES.md) are N/A by scope.
Findings
Critical
[Critical] Chained multi-push bash command bypasses branch-protection guard
· dimension: 1. Correctness (the guard's own contract: "push to main in a protected repo is forbidden")
· location: orbit-ui-mobile/.claude/hooks/_lib/rules-git.mjs (checkGitCommand, the `segments`/`pushIndex`/`targetsProtectedRepo` block, ~lines 95-101 of the new file)
· issue: `pushIndex = segments.findIndex((segment) => /\bgit\b[\s\S]*\bpush\b/.test(segment))` locates only the FIRST segment in the command that looks like a push. `targetDir` and the protected-repo check are derived from that first occurrence only. `if (!targetsProtectedRepo(targetDir, resolveRemoteUrl)) return null` then allows the **entire command** the instant the first push resolves to a non-protected repo — before the main/master regex or the bare-push/HEAD-branch check ever run against the rest of the string.
· risk: A single chained command such as
`cd /path/to/brain-vault && git push origin main && cd /path/to/orbit-ui-mobile && git push origin main`
is allowed through in full: the first push (to the unprotected sibling repo, which is meant to be allowed per this PR's stated goal) causes an early `return null`, so the second push — directly to `main` inside a protected repo (`orbit-ui-mobile`/`orbit-api`/`orbit-landing-page`) — is never evaluated and silently bypasses the guard. This is exactly the routine "bounce between sibling repos in one session" workflow this PR's own description calls out, so it is a realistic trigger, not a contrived edge case. Traced by hand step-by-step (JS `String.split(/[&|;\n]/)` behavior, `findIndex`, `pushTargetDir`'s backward `cd`-scan, `targetsProtectedRepo`'s early return) and independently confirmed by an adversarial skeptic pass, which found no error in the trace and no other enforcement layer this hook can rely on (CLAUDE.md credits this hook itself as the enforcement mechanism, not GitHub server-side rules).
Before this PR, the equivalent check ran the main/master-push regex against the *whole* command string unconditionally (no repo-scoping), so it would have caught the second push regardless of chaining — this is a regression introduced by this diff, not a pre-existing gap.
· fix: Don't gate on a single "first push" index. Either (a) iterate every segment that matches `/\bgit\b[\s\S]*\bpush\b/`, resolving `targetDir`/protection independently per occurrence and only skipping the ones that resolve to a non-protected repo, continuing to check the rest; or (b) run the main/master and bare-push checks per-segment against a segment-scoped protected/unprotected decision rather than returning early for the whole command. Add a regression test to `test-hooks.mjs` for a chained command with a non-protected push before a protected-repo push to main (the current 13 new cases all test single-push commands, which is why this slipped through).
· reference: CLAUDE.md "Git: ... `main` is protected (no direct or force push — enforced by the `git-guardrails` hook)"; this PR's own comment in the diff: "Wrongly blocking a push is recoverable; wrongly allowing one onto main is not."
High
None.
Medium
None (the missing regression test is folded into the Critical finding's fix, since it is the direct cause of the gap rather than an independent issue).
Low / Info
None posted (signal gate — no speculative or style findings included).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or locale files changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — no apps/web/packages/shared file changed; no ESLint config covers .claude/hooks/ or .opencode/plugin/ |
| Type check | N/A — plain JS, no TS project covers these paths |
| Tests | Not run — this session's sandbox blocked all node <file> execution (requires approval this review couldn't grant); the PR body states "Full suite green, all three layers" for node .claude/hooks/test-hooks.mjs, unverified by this review |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A, no
apps/*UI files in this diff. - Dimension 9 (Parity): N/A, no
apps/web/apps/mobilefiles in this diff. - Dimension 10 (i18n): N/A, no locale/string changes.
- Dimension 11 (Contract drift + backward-compat): N/A, no
packages/shared/src/typesor DTO changes. - Dimension 13 (Backend hard rules): N/A,
orbit-apinot touched. - Dimension 14 (FEATURES.md parity): N/A, this is internal harness tooling, not a user-facing feature.
- Dimension 4 (Comment policy): the new narrative
//comments inrules-git.mjs/git-guardrails.mjs/orbit-guardrails.jswould triplocal/no-commentsif it applied here, but no root/.claudeESLint config exists — onlyapps/web,apps/mobile, andpackages/sharedhaveeslint.config.*. Not enforced on this surface, consistent with the pre-existing style already in these files (e.g. the untouched header comments ingit-guardrails.mjsandorbit-guardrails.js). Not flagged as a finding. - Cross-model second opinion (Phase 6 step 2): skipped —
opencodeis not available/verifiable in this environment to run/second-opinion(this review's sandbox also blocks shell probes likecommand -v). Stated per protocol rather than treated as agreement. - All 4 changed files (
rules-git.mjs,git-guardrails.mjs,test-hooks.mjs,orbit-guardrails.js) received a verdict; nothing left unreviewed.
What's good
- The heredoc-body stripping is well-designed: the shell-consumer exception is correctly anchored to each heredoc's own immediate
beforeOperator(not a whole-string search), which is exactly the bug class the PR describes fixing, and every one of the 8 new heredoc-related test cases traces correctly against the implementation, including the subtle "body mentionsbash <<EOF" anti-re-arming case. - The fail-safe design (
targetsProtectedReporeturnstrueon missing resolver, empty/unresolvable remote, or a throwing resolver) is correct and well-tested — an unresolvable repo defaults to protected, matching the PR's own "wrongly blocking is recoverable, wrongly allowing is not" principle everywhere except the multi-push case above. - Hygiene rules (
--no-verify,--no-gpg-sign,commit -n) correctly stay universal (checked before repo-scoping), matching the PR's stated intent. - Both the Claude Code hook and the opencode plugin wire the new
resolveRemoteUrloff the same shared_lib, keeping enforcement parity between the two tools. - Extracted helpers (
unquote,repoNameFrom,targetsProtectedRepo,pushTargetDir,stripHeredocBodies,blocked) are well-named, appropriately sized, and remove duplication that existed in the old bare-push branch (the old inlinecMatch/dircomputation is now shared viapushTargetDir).
Recommendation
Fix the multi-push scoping bug (evaluate every push occurrence in the command independently rather than short-circuiting on the first one's target repo) and add a chained-command regression test before merging. Everything else in this diff is solid and can land as-is once that's addressed.
| const pushIndex = segments.findIndex((segment) => /\bgit\b[\s\S]*\bpush\b/.test(segment)) | ||
| if (pushIndex === -1) return null | ||
| const targetDir = pushTargetDir(segments, pushIndex, cwd) | ||
| if (!targetsProtectedRepo(targetDir, resolveRemoteUrl)) return null |
There was a problem hiding this comment.
[Critical] Chained multi-push command bypasses branch protection
pushIndex finds only the first segment matching a push, and line 100 returns null (allow) for the entire command the moment that first push resolves to a non-protected repo — before any later push in the same chain is ever evaluated.
Concretely: cd /sibling-repo && git push origin main && cd /orbit-ui-mobile && git push origin main sails through. The first push (to the unprotected sibling repo — exactly what this PR intends to allow) triggers the early return null on line 100, so the second push, straight to main inside a protected repo, is never checked. This is a regression: before this diff, the main/master regex ran against the whole command unconditionally, so it would have caught the second push regardless of chaining.
Fix: iterate every segment matching /\bgit\b[\s\S]*\bpush\b/, resolving targetDir/protection independently per occurrence, and only skip the ones that resolve to a non-protected repo — don't short-circuit the whole command on the first one. Add a regression test for a chained non-protected-push-then-protected-push-to-main command; none of the 13 new tests cover this.
…review) The repo-scoping introduced here decided the whole command from the FIRST push-bearing segment: findIndex located it, resolved protection for that segment's target, and returned null for the entire string if that target was unprotected. A chained git -C ~/brain push origin main && git -C ~/orbit-api push origin main was therefore allowed in full -- the brain push (unprotected by design) short -circuited the guard before the direct push to a protected main was ever evaluated. A regression introduced by this branch: the previous code ran the main/master regex against the whole command unconditionally, so chaining could not evade it. The same short-circuit had a mirror-image bug: the main/master regex ran against the whole scannable string rather than the matched segment, so a legitimate `git -C ~/orbit-api push origin feature/x && git -C ~/brain push origin main` was BLOCKED -- the `main` belonged to the unprotected segment. Both directions are fixed by evaluating each push segment independently. Now every segment matching a push is resolved to its own target dir, skipped if unprotected, and checked (explicit main/master ref, then bare-push-on-main via resolveHeadBranch) if protected. Adds 3 regression tests, all of which fail against the pre-fix code: the chained bypass, the chained false positive, and a chained bare push landing on a protected main. Full suite green (ORBIT HOOK PARITY OK). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYioRojgzWuHF9CVz1oAJK
|
Critical finding fixed, and the trace in the review was exactly right — including that it's a regression this branch introduced rather than a pre-existing gap. The fix. Every segment matching a push is now resolved to its own target dir, A second bug in the same block, mirror image of the first. The That's a legitimate command and the pre-fix code blocked it. The guard was simultaneously too permissive (chained bypass) and too strict (chained false positive) — both are the same root cause, and per-segment evaluation cures both. Tests. Three added, and I verified all three fail against the pre-fix code rather than just passing against the new one:
They use a per-directory remote resolver, since the existing cases all resolve one repo per command and structurally couldn't reach this. Full suite: Ready for re-review. |
There was a problem hiding this comment.
Code Review: PR #541
Scope: PR #541 in thomasluizon/orbit-ui-mobile — fix/git-guardrails-scope-to-orbit-repos
Recommendation: APPROVE
Summary
The PR fixes three false-positive classes in the git-guardrails hook (shared by the
Claude Code PreToolUse(Bash) hook and the .opencode plugin): the push-to-main guard
was repo-agnostic (blocking sanctioned direct-to-main pushes in sibling repos like the
brain vault), the hygiene rules matched heredoc bodies (blocking commit messages that
merely mention a banned flag), and the shell-heredoc exception was a whole-string
search that re-armed on any text containing bash <<. All three are traced to a single
root cause — matching text that isn't the command being run — and fixed at that root,
consistent with CLAUDE.md rule 1. This is infra/tooling code (.claude/hooks/,
.opencode/plugin/); it touches no apps/*, packages/shared, or orbit-api surface,
so most rubric dimensions are gated N/A (see Deferred).
I traced checkGitCommand's new segment-by-segment push logic, the heredoc-stripping
regex, and the repo-name resolver by hand against every new assertion in
test-hooks.mjs (13 new cases) plus additional edge cases of my own (aliased shells,
-c/-C ordering, chained &/;/\n splitting, ReDoS surface, injection safety via
execFileSync array-form args). All traced correctly to the expected block/allow
outcome. I could not execute the suite directly in this sandbox (the Bash tool declined
node invocation and any new mkdir/rm needed to stage a runnable copy outside the
.claude/ tree — see Deferred), so this is static/manual verification, not an executed
test run.
Findings
Critical
None.
High
None.
Medium
None concretely actionable enough to post (see Deferred for the one test-coverage
observation that didn't clear the signal gate).
Low / Info
None posted (signal gate: Low/Info are not PR-review findings).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or locale files changed |
| contract-aligner | N/A — no packages/shared/src/types/* or orbit-api DTOs changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — no lint-covered workspace file (apps/web, apps/mobile, packages/shared) touched |
| Type check | N/A — plain .mjs, no TS surface touched |
| Tests | Not executed in this session (sandbox blocked node/mkdir/rm); manually traced instead — see Summary |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md/AI-slop (#8), Parity (#9), i18n (#10), Contract drift +
backward-compat (#11), Backend hard rules (#13), FEATURES.md parity (#14) —
all N/A by gate: the diff touches only.claude/hooks/**and.opencode/plugin/**,
none of which areapps/*,packages/shared, ororbit-apisurface. - Comment policy (#4) — treated N/A rather than flagged. The new narrative WHY-style
comments (e.g. thePROTECTED_REPOSrationale, the heredoc-exception explanation) lack
anhttp(s)://link, which the strict policy would require verbatim. But
.claude/hooks/*.mjssits outside every workspace's ESLint config (apps/web,
apps/mobile,packages/shared) that carrieslocal/no-comments, so the lint gate
never touches this file, and the pre-existing (unmodified-by-this-diff) code in the
same file already uses the identical narrative-comment convention. Not a regression
this diff introduces. - Function size (rubric #3 note) — the rewritten
checkGitCommandin
.claude/hooks/_lib/rules-git.mjsis ~66 lines, over the ~50-line soft cap (well under
the 100-line hard cap). It's cleanly decomposed into named helpers
(pushTargetDir,targetsProtectedRepo,stripHeredocBodies,repoNameFrom) with the
orchestrator left as a single readable loop; didn't clear the signal gate as a
concretely-actionable Medium, noted here rather than manufactured as a finding. - Test-execution verification — the 13 new
_lib-unit cases intest-hooks.mjs
were verified by manual trace (see Summary), not by running
node .claude/hooks/test-hooks.mjs; this session's Bash tool declined every attempt to
invokenode,mkdir, orrmfor a scratch verification harness. All 4 changed files
(rules-git.mjs,git-guardrails.mjs,test-hooks.mjs,
.opencode/plugin/orbit-guardrails.js) were read in full and given a verdict; none
deferred for lack of review, only for lack of executed validation. - Report location — this report could not be written to the skill's usual
.claude/reviews/output path (the sandbox treats all.claude/**writes as
requiring interactive approval, which is unavailable here); it was written to
pr541-review-report.mdat the repo root instead.
What's good
- Root-cause fix, not a patch: all three false positives traced to the same
text-vs-command-being-run confusion and fixed at that layer, per CLAUDE.md rule 1. - Fails safe throughout: an unresolvable or throwing
resolveRemoteUrldefaults to
"protected" (blocks), and callers that don't pass a resolver keep the pre-existing
behavior — explicitly covered by two new tests
(unresolvable remote still blocks/throwing remote resolver still blocks).
Hygiene rules (--no-verify,--no-gpg-sign,commit -n) stay universal regardless of
repo, checked before the repo-scoped branch-protection logic. - Genuine cross-adapter parity:
.claude/hooks/git-guardrails.mjsand
.opencode/plugin/orbit-guardrails.jsinject the identicalresolveRemoteUrlshape off
the same_lib, so a fix to the shared logic covers both tools by construction rather
than by discipline. execFileSync(array-args form, no shell) is used consistently for the new
resolveRemoteUrl, matching the existingresolveHeadBranchpattern — no command
injection surface from directory strings that flow through-C/cdparsing.
Manual trace confirms "each push in a chain judged on its own target" (the stated goal
of the just-prior PR #541 review round, commit1248442d) holds for mixed
protected/unprotected chains in both orders.- Good regression coverage for the specific bug class fixed: the anchoring test
(body mentioning a shell heredoc still gets stripped) directly exercises the exact
self-referential failure mode described in the PR body (a PR description documenting
the bug tripping the bug).
Recommendation
Ready to merge. No Critical/High findings survived review. The one open item is
environmental, not code quality: re-run node .claude/hooks/test-hooks.mjs locally (or
let CI run it) before merge to get an executed — not just traced — green, since this
sandbox could not run it.
|
There was a problem hiding this comment.
Code Review: PR #541 — orbit-ui-mobile
Scope: PR #541 fix(harness): scope the git push guard to the Orbit repos, stop matching heredoc bodies (thomasluizon/orbit-ui-mobile)
Recommendation: APPROVE
Note on execution environment
This run could not write .claude/reviews/*.md or execute gh pr review / mkdir / node against materialized files — every write and several gh reads were blocked by the sandbox as touching "sensitive" paths, with no interactive approver available. The review below is delivered as text per instructions rather than a file. I also could not run node .claude/hooks/test-hooks.mjs directly (same restriction) or check CI status (gh pr checks / gh api blocked); I instead hand-traced every new regex/branch in checkGitCommand against every assertion the PR itself adds to test-hooks.mjs and confirmed each traces to the expected result. Validation is effectively N/A (sandbox-restricted) rather than run.
No new changes since last approval
git diff between the commit already carrying an APPROVED review (5ca53522) and current HEAD (56db453f) is empty — the only commits since that approval are merge commits with no content delta. No new findings to surface beyond the prior review.
Summary
This is the third iteration on this branch (two prior review-driven fixups already landed: judging each push in a chain on its own target, and anchoring the shell-heredoc exception to its own consumer). The diff touches only harness tooling — .claude/hooks/_lib/rules-git.mjs, .claude/hooks/git-guardrails.mjs, .claude/hooks/test-hooks.mjs, .opencode/plugin/orbit-guardrails.js — no apps/*, packages/shared, or orbit-api files. The core logic (repo-scoped protection via origin's remote URL, per-segment push-target resolution honoring -C then a preceding cd then cwd, heredoc-body stripping anchored to its own << consumer, and fail-safe defaults on any unresolvable/throwing resolver) checks out correctly against every scenario traced, including the trickier chained and nested-heredoc cases the PR's own new tests assert. No correctness, security, or dead-code issues found in the changed logic. One concrete test-coverage gap is worth flagging as a non-blocking follow-up.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] New resolveRemoteUrl repo-scoping is unit-tested at the _lib layer only, not through the real adapter files it was wired into
· dimension: 3 (SOLID/clean-arch — test-coverage half of the file's own stated contract)
· location: orbit-ui-mobile/.claude/hooks/test-hooks.mjs:130-131 (section 2, "claude code hooks — real files") and :173-174 (section 3, "opencode plugin")
· issue: test-hooks.mjs's own header comment states its job is to prove "the SAME rule, off the SAME _lib, must block/allow identically" across all three layers (_lib, the real Claude Code hook file, the real opencode plugin). The PR adds 13 new _lib-level cases covering repo scoping, chained-push judgment, and the heredoc exception, but section 2's and section 3's git assertions are untouched — still only the pre-existing "push main -> 2" / "feature -> 0" pair, run with no `cwd`/`directory` override, so they exercise resolveRemoteUrl against whatever repo the test process happens to be checked out in.
· risk: the new resolveRemoteUrl wiring in git-guardrails.mjs and orbit-guardrails.js (the actual code paths Claude Code / opencode invoke at runtime) has no regression test independent of "this suite happens to run inside a protected Orbit repo." If a future edit typos the parameter name, forgets to pass resolveRemoteUrl through in one of the two adapters, or breaks the `-C`/`cd`-into-a-sibling-repo path specifically in the real files (as opposed to the _lib function under direct test), this suite would keep reporting green.
· fix: add a couple of cases to section 2/3 that exercise a non-Orbit-repo allow and a chained cross-repo push through the real files — e.g. spin up a throwaway `git init` fixture dir with `git remote add origin https://github.com/thomasluizon/brain.git`, then run `git-guardrails.mjs` with `input.cwd` pointing at it (section 2) and `plugin({ directory: fixtureDir })` (section 3), asserting `git push origin main` is allowed there and still blocked when `cwd`/`directory` point back at the real repo checkout.
· reference: CLAUDE.md rule "Testing... every feature needs behavior tests"; test-hooks.mjs:2-8 (file's own stated three-layer parity contract)
Low / Info
None (per the signal gate, not posted).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts, and only one repo touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — .claude/hooks and .opencode/plugin are outside every ESLint config's scope (packages/shared, apps/web, apps/mobile only), confirmed by inspecting the three eslint.config.* files; this diff's comment style (WHY-narration without a linked URL) is consistent with the pre-existing, unlinted convention throughout the rest of this directory |
| Type check | N/A — plain .mjs/.js, no TypeScript surface |
| Tests | Not executed — sandbox blocked node/file-materialization; manually traced all 13 new _lib assertions plus the pre-existing ones against the actual regex/branch logic in the PR-head content and confirmed each expected result |
| Build (api) | N/A — no orbit-api changes |
| Build / Unit Tests / SonarCloud | Skipped per workflow instructions — these run as separate required CI checks on this PR |
Deferred — N/A dimensions & files not verdicted
- Parity (#9), i18n (#10), Contract drift (#11), Security-API (#12 API side), Backend hard rules (#13), FEATURES.md parity (#14), DESIGN.md/AI-slop (#8) — all N/A, diff's surface is confined to
.claude/hooks+.opencode/plugin, none of these dimensions' trigger conditions are met. - Comment policy (#4) — checked, not blindly skipped: confirmed no ESLint config covers this directory, and the WHY-without-URL narrative comment style added here matches the pre-existing, unlinted convention already used throughout
rules-git.mjs/git-guardrails.mjsbefore this diff. - Backward-compat guard (Phase 5) — N/A, no
packages/shared/src/types/*ororbit-apiDTO hunks in this diff. - contract-aligner (cross-repo) — not verifiable in CI;
orbit-apiis not checked out in this job. Diff doesn't touch contract surfaces anyway. - All 4 changed files (
rules-git.mjs,git-guardrails.mjs,test-hooks.mjs,orbit-guardrails.js) received a verdict above; nothing changed was skipped.
What's good
- The fail-safe posture is genuinely fail-safe: every path where
resolveRemoteUrlis missing, throws, or returns an unparseable value defaults to "protected" — no input was found that silently allows a push that should be blocked. - The per-segment push-judgment redesign correctly handles the subtle case of a
cd/-C-driven directory change occurring inside a shell-consumed heredoc body (the newline-based segment split naturally makes acdon one heredoc line apply to agit pushon a later line, matching real shell semantics). execFileSyncis used with an argv array (not a shell string) for bothresolveHeadBranchand the newresolveRemoteUrl, so no command-injection surface is introduced despitedircoming from regex-extracted, attacker/agent-influenced text.- Good self-awareness in the PR body: it explicitly documents that writing the PR body itself and the first commit message were blocked by the very bugs being fixed, and that the guard is now verified live end-to-end.
Recommendation
Merge is safe as-is; the one Medium (test-coverage gap in test-hooks.mjs sections 2/3) is a good follow-up but not a blocker — it's a "the regression suite doesn't yet regression-test its own regression-test's namesake feature" gap, not a functional defect in the shipped guard logic.
1 participant
What
Three false positives in
git-guardrails, all one root cause: the guard matched text that is not the command being run. Each one blocked real work in this session.1. The push rules were repo-agnostic
Any push to main blocked, whatever repo it targeted. Sessions always launch from
orbit-ui-mobileand drive sibling repos from there, and two of those are direct-to-main by design:!shell. That workaround existed because of this bug.The guard now resolves the target repo from origin's remote URL and enforces branch protection only for
orbit-ui-mobile,orbit-apiandorbit-landing-page.Resolving the target is the subtle part: the session cwd is not the target when the command cds first. A
cdinto the vault followed by a push reports cwd asorbit-ui-mobile. So it honorsgit -C <dir>, then acd <dir> &&earlier in the same chain, then cwd.2. The rules matched heredoc bodies
Writing a commit message about a banned flag tripped the guard. The first commit on this branch could not be written before this fix. Heredoc bodies are data, not flags, and are now stripped before matching.
3. The exception for shell heredocs re-armed the bug
A heredoc feeding a real shell must keep its body in scope, since there the body is commands. But that check was a whole-string search, so any text containing the form switched stripping off for the whole command. Writing a PR body documenting the exception re-armed the very bug it documents. This description could not be posted before the third commit. The check is now anchored to the consumer immediately preceding each heredoc operator.
Safety
All paths fail safe: an unresolvable or throwing remote resolver is treated as protected, because wrongly blocking a push is recoverable and wrongly allowing one onto main is not. Callers that pass no resolver keep blocking exactly as before, so the pre-existing tests are untouched and still green.
Hygiene rules stay universal, checked before repo scope: the no-verify, no-gpg-sign and short-alias commit bypasses are wrong in any repo.
Parity
Both adapters inject the new resolver off the same
_lib, so the Claude Code hook and the opencode plugin enforce identically.Tests
13 new unit cases in
test-hooks.mjs: repo scoping, cd and -C target resolution, heredoc stripping, the shell-heredoc exception plus its anchoring regression, and every fail-safe path. Full suite green, all three layers.Verified live end to end: the brain vault push this guard had blocked now succeeds, and pushing to this repo's main still blocks (this PR exists because of it).