Uh oh!
There was an error while loading. Please reload this page.
fix(guardrails): block merge bypass commands - #191
Conversation
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
c333d10 to
c36d867CompareThere was a problem hiding this comment.
Pull request overview
This PR tightens the guardrail-git plugin so PR merges can’t bypass review/conflict gates via alternative merge commands, and adds regression tests to cover newly blocked flows.
Changes:
- Treat
gh api .../pulls/<n>/mergeas a PR merge so merge/review gates run for GitHub REST API merges. - Block
git reset --soft|--mixedresets to base branches to prevent bypassing rebase/merge conflict guardrails. - Require an explicit worktree flag for
codex execto avoid reviewing the wrong checkout; add focused tests for these guardrails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/opencode/test/plugin/guardrail-git.test.ts | Adds unit tests for API-merge bypass, reset-to-base bypass, and codex exec worktree requirement. |
| packages/guardrails/profile/plugins/guardrail-git.ts | Expands merge detection to include gh api merges, adds reset-to-base blocking, and enforces explicit worktree for codex exec. |
Comments suppressed due to low confidence (1)
packages/guardrails/profile/plugins/guardrail-git.ts:313
- The reset-to-base detection regex will also match refs like
origin/dev-featurebecausedevis followed by-(a word boundary), so it can over-block resets that are not targeting the protected base branches. Tighten the ref matching so it only triggers when the target is exactly(origin/)?main|master|develop|devas a whole token (e.g., require(?:\s|$)or end-of-ref delimiters after the branch name).
if (/\bgit\s+rebase\b/i.test(cmd) && !/--abort\b/i.test(cmd)) {
if (/\bgit\s+rebase\s+(origin\/)?(main|master|develop)\b/i.test(cmd)) {
await ctx.mark({ rebase_session_active: true, rebase_session_at: new Date().toISOString() })
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd.match(/\bgh\s+api\b(?=[\s\S]*(?:^|\s)-X(?:=|\s*)PUT\b)[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i)?.[1] ?? | ||
| cmd.match( | ||
| /\bgh\s+api\b(?=[\s\S]*(?:^|\s)--method(?:=|\s+)PUT\b)[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i, |
There was a problem hiding this comment.
ghPrMergeNumber only detects gh api merges when the method is spelled with a space (-X PUT / --method PUT). gh api (cobra/pflag) also supports --method=PUT and commonly -X=PUT / -XPUT, which would bypass the merge guard. Update the method detection regexes to accept the =PUT (and no-space short flag) forms so the bypass is reliably blocked.
| cmd.match(/\bgh\s+api\b(?=[\s\S]*(?:^|\s)-X(?:=|\s*)PUT\b)[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i)?.[1]?? | |
| cmd.match( | |
| /\bgh\s+api\b(?=[\s\S]*(?:^|\s)--method(?:=|\s+)PUT\b)[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i, | |
| cmd.match(/\bgh\s+api\b(?=[\s\S]*(?:^|\s)-X(?:PUT|=PUT|\s+PUT\b))[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i)?.[1]?? | |
| cmd.match( | |
| /\bgh\s+api\b(?=[\s\S]*(?:^|\s)--method(?:=PUT\b|\s+PUT\b))[\s\S]*\brepos\/\S+\/pulls\/(\d+)\/merge\b/i, |
| function codexExecWorktree(cmd: string) { | ||
| return ( | ||
| cmd | ||
| .match(/\bcodex\s+exec\b[\s\S]*(?:\s-C|\s--cd|\s--cwd)\s+(?:"([^"]+)"|'([^']+)'|(\S+))/i) |
There was a problem hiding this comment.
The codex exec worktree check only matches -C <path>, --cd <path>, --cwd <path> with a whitespace-separated value. If the CLI is invoked with --cwd=<path> / --cd=<path> (or quoted paths with spaces), this guard will incorrectly block even though an explicit worktree was provided. Consider broadening the regex to accept --cwd= / --cd= / -C= forms and quoted values.
| .match(/\bcodex\s+exec\b[\s\S]*(?:\s-C|\s--cd|\s--cwd)\s+(?:"([^"]+)"|'([^']+)'|(\S+))/i) | |
| .match( | |
| /\bcodex\s+exec\b[\s\S]*(?:\s-C|\s--cd|\s--cwd)(?:\s*=\s*|\s+)(?:"([^"]+)"|'([^']+)'|(\S+))/i, | |
| ) |
| } | ||
| function sameWorktree(left: string, right: string) { |
There was a problem hiding this comment.
This guard currently blocks anycodex exec invocation without an explicit worktree, but the error message says "codex exec review must…". Either scope the check to review-related codex commands, or adjust the message so it matches the actual behavior (all codex exec commands).
| test("blocks GitHub API pull request merge bypasses", async () => { | ||
| await using fixture = await context() | ||
| const git = createGitHandlers(fixture.ctx, review()) | ||
| await expect( | ||
| git.bashBeforeGit("gh api -X PUT repos/Cor-Incorporated/nfc-profile-card/pulls/42/merge", {}, {}), | ||
| ).rejects.toThrow("merge blocked") | ||
| expect(fixture.marks.some((item) => String(item.last_reason).includes("GLM code-reviewer"))).toBe(true) | ||
| }) | ||
| test("blocks GitHub API pull request merge bypasses with equals method flags", async () => { | ||
| await using fixture = await context() | ||
| const git = createGitHandlers(fixture.ctx, review()) | ||
| await expect( | ||
| git.bashBeforeGit("gh api --method=PUT repos/Cor-Incorporated/nfc-profile-card/pulls/42/merge", {}, {}), | ||
| ).rejects.toThrow("merge blocked") | ||
| }) | ||
| test("blocks reset-to-base sync bypasses", async () => { | ||
| await using fixture = await context() | ||
| const git = createGitHandlers(fixture.ctx, review()) | ||
| await expect(git.bashBeforeGit("git reset --soft origin/dev", {}, {})).rejects.toThrow("reset-to-base sync blocked") | ||
| expect(fixture.marks.at(-1)?.last_reason).toBe("branch reset sync blocked") | ||
| }) | ||
| test("requires explicit worktree for codex exec reviews", async () => { |
There was a problem hiding this comment.
The new tests cover gh api -X PUT …/merge and codex exec -C …, but they don’t cover important accepted flag variants that could regress or bypass the guardrails (e.g. gh api --method=PUT …/merge, codex exec --cwd=/tmp/project …, and a non-base ref like git reset --soft origin/dev-feature). Adding these cases would help ensure the regexes don’t allow bypasses or cause false positives.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Validation