diff --git a/.claude/skills/handoff/SKILL.md b/.claude/skills/handoff/SKILL.md index 120f408d39..491a3a2a75 100644 --- a/.claude/skills/handoff/SKILL.md +++ b/.claude/skills/handoff/SKILL.md @@ -57,8 +57,10 @@ force-push, or discard work. End the message with: `Co-Authored-By: Claude Fable 5 `. 5. **Push** the feature branch: `git push -u origin `. Per-PR auto-merge state is user-owned: - automation must not disable it. If the branch already has an open PR with auto-merge armed, - leave it mutation-frozen; do not push or change the branch/base until it merges or the user + automation must not disable or re-enable it. If the branch already has an open PR with auto-merge + armed, an ordinary fast-forward push (this step) is still safe — GitHub re-validates required + checks against the new head before it merges. Never force-push, rewrite history, or change the + branch/base while auto-merge is armed; that alone stays frozen until the PR merges or the user manually changes that state. Never pipe the push through `tail`, `head`, or another command that can mask its status. Confirm the remote tip equals local HEAD with `git ls-remote` before reporting success. The pre-push guards run diff --git a/.claude/skills/run-pr/SKILL.md b/.claude/skills/run-pr/SKILL.md index cb53109962..f2cd72a13d 100644 --- a/.claude/skills/run-pr/SKILL.md +++ b/.claude/skills/run-pr/SKILL.md @@ -22,7 +22,7 @@ Never, even during a sweep: - Never merge a pull request into `main` or any protected branch, and never enable auto-merge; the sweep fixes and reports, the user merges. Per-PR auto-merge state is user-owned: - automation must not disable it. + automation must not disable or re-enable it. - Never close a pull request, delete or rename branches, force-push (no `--force`, no `--force-with-lease`), or rebase. - Never run provider-backed gates: `eval:rag`, `eval:quality`, `eval:retrieval:quality`, @@ -54,10 +54,12 @@ Never, even during a sweep: ## Per-PR algorithm -Before any branch-changing action, inspect `autoMergeRequest`. If it is non-null, treat the PR as -mutation-frozen: do not push, update the branch/base, or otherwise change its head. Continue -read-only diagnosis and reporting, but leave the armed state untouched until the PR merges or the -user manually changes it. Never disable auto-merge as a workaround for maintenance. +Before any branch-changing action, inspect `autoMergeRequest`. If it is non-null, ordinary +fast-forward fixes (CI repairs, review-thread fixes, syncing `main` in) may still proceed — GitHub +re-validates required checks against the new head before it merges, so an additive push cannot +slip an unvalidated commit past auto-merge. Never disable or re-enable auto-merge, and never +force-push or otherwise rewrite the branch's history while it is armed; that alone stays frozen +until the PR merges or the user manually changes the auto-merge state. ### Step 0 — skip gates (record every skip with its reason) diff --git a/.cursor/agents/pr-babysit.md b/.cursor/agents/pr-babysit.md index cf809a8d4f..c92f55df3e 100644 --- a/.cursor/agents/pr-babysit.md +++ b/.cursor/agents/pr-babysit.md @@ -19,7 +19,7 @@ When invoked: 6. After fixing a review thread, reply first (never resolve silently), then use the authorized direct resolution tool. Delegated Bugbot threads follow the same path. Only the trusted Codex autofix identity may fall back to ``; otherwise leave the thread open and report the missing capability. 7. Respect provider confirmation boundaries: no live Supabase/OpenAI/eval spend without separate explicit authorization for that provider action. A Run PR sweep never authorizes provider-backed gates. 8. Require explicit user authorization before commits, pushes, hosted-CI reruns, replies, or thread resolution. The Run PR shortcut supplies authorization only for the GitHub actions enumerated in `AGENTS.md`. Do not edit PR titles/bodies during Run PR sweeps unless the user explicitly asks. Re-check CI until mergeable + green + comments triaged. -9. Never merge into `main`, force-push, close the PR, enable auto-merge, or delete branches unless the user explicitly asks. Per-PR auto-merge state is user-owned: automation must not disable it. If auto-merge is already armed, treat the PR as mutation-frozen and do not push, update its branch/base, or otherwise change its head until it merges or the user manually changes that state. +9. Never merge into `main`, force-push, close the PR, enable/disable auto-merge, or delete branches unless the user explicitly asks. Per-PR auto-merge state is user-owned: automation must not disable or re-enable it. If auto-merge is already armed, an ordinary fast-forward push to fix CI or a review thread may still proceed — GitHub re-validates required checks against the new head before it merges. A force-push, history rewrite, or base/target change while armed stays frozen until the PR merges or the user manually changes that state. 10. Follow `docs/codex-review-protocol.md` and record every completed review or sweep — including pure and no-op reviews — with `npm run ledger:append`. Report before/after: merge state, CI, threads fixed vs left open, commits pushed, and any remaining human decision. diff --git a/AGENTS.md b/AGENTS.md index 721f3babc5..e1111da24e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -646,7 +646,7 @@ Nothing else inherits this authorization. Only the user's own task message can t Hard guardrails (never, even during a sweep): -- Never merge a pull request into `main` or any protected branch, and never enable auto-merge; the sweep fixes and reports, the user merges. Per-PR auto-merge state is user-owned: automation must not disable it. When auto-merge is already armed, do not push, update the branch/base, or perform another head-changing action; report the frozen state and leave the PR untouched until it merges or the user manually changes that state. +- Never merge a pull request into `main` or any protected branch, and never enable auto-merge; the sweep fixes and reports, the user merges. Per-PR auto-merge state is user-owned: automation must not disable or re-enable it. Ordinary fast-forward commits and pushes to fix CI or review findings are allowed while auto-merge is armed — GitHub re-validates required checks against the new head before it will merge, so an additive push cannot make it merge something unvalidated (`guard-push.mjs`'s auto-merge guard warns rather than blocks for this case). Never force-push, rewrite history, or change the PR's base/target while auto-merge is armed — that stays hard-blocked with no override; wait for the user to change the auto-merge state first. - Never close a pull request, delete or rename branches, force-push, or rebase. - Never run provider-backed gates: `eval:rag`, `eval:quality`, `eval:retrieval:quality`, `verify:release`, `check:supabase-project`, `test:live`, or anything else that touches live Supabase/OpenAI. - Respect the `skip-codex-review` label as a full per-PR opt-out. @@ -722,11 +722,12 @@ A settle-then-push addition also lands after this repo's one automatic Codex rev already have run against the earlier head — in practice the connector re-reviews each new push (observed on this same PR), but if it doesn't, request a fresh review explicitly before merging rather than assuming the addition was covered. **If the target PR has -auto-merge armed, settling-then-pushing races the merge itself.** Treat that PR as -mutation-frozen: do not disable or re-enable auto-merge, push, update its branch/base, or -otherwise change its head. Let the armed merge land, or wait for the user to manually -change the auto-merge state before doing further branch work. `guard-push.mjs` enforces -this for every locally pushed PR branch when authenticated `gh` is available; agent +auto-merge armed, an ordinary fast-forward push is still safe to bundle onto** — GitHub +re-validates required checks against the new head before merging. Per-PR auto-merge state is +user-owned: automation must not disable or re-enable it, and a force-push or base/target change +while armed still hard-blocks with no override — that is the actual race, not an additive +commit. `guard-push.mjs` enforces the force-push block for every locally pushed PR branch when +authenticated `gh` is available; agent policy remains the backstop in environments where local hooks or `gh` are unavailable. Bundle only when every item being combined is: @@ -817,8 +818,10 @@ named PR). Future process only. `static-pr` but not in `verify:cheap`; an uncommitted format leaves CI red on the pushed blob. Whole-tree Prettier, not a single edited file. - If a PR has auto-merge armed, its auto-merge state is user-owned and automation must not disable - it. Treat the branch as mutation-frozen: no push, update-branch, base change, or bundled addition - until it merges or the user manually changes that state. + or re-enable it. Ordinary fast-forward pushes, `update-branch`/merge-main-in syncs, and bundled + additions may proceed — GitHub re-validates required checks against the new head before merging, + so an additive push cannot slip past that. A force-push, history rewrite, or base/target change + while armed still hard-blocks with no override; wait for the user to change that state first. - Missing CI checks are not a green pass. `pull_request` workflows do not run when GitHub cannot build `refs/pull//merge`. The `PR mergeability` check uses trusted `pull_request_target` events and refreshes unchanged PR heads after protected-base diff --git a/docs/branch-review-records/34da6f5a63ff446e67f42d58dd5935969b7425557ddb21aa01bcadd78378fec9.record.md b/docs/branch-review-records/34da6f5a63ff446e67f42d58dd5935969b7425557ddb21aa01bcadd78378fec9.record.md new file mode 100644 index 0000000000..6edfd18331 --- /dev/null +++ b/docs/branch-review-records/34da6f5a63ff446e67f42d58dd5935969b7425557ddb21aa01bcadd78378fec9.record.md @@ -0,0 +1 @@ +| 2026-08-17 | claude/pr-auto-merge-safety-tpxupu | 786a0558bc676f7b7b175ae50b84f9db3d309b3d | scripts/guard-push.mjs, tests/guard-push.test.ts, AGENTS.md, .claude/skills/run-pr/SKILL.md, .claude/skills/handoff/SKILL.md, .cursor/agents/pr-babysit.md | authored: allow ordinary fast-forward push/commit to a PR branch while auto-merge is armed; force-push and disabling auto-merge remain hard-blocked | verify:pr-local full run green (Test Files 635 passed, Tests 6775 passed/4 skipped, failed: none); guard-push.mjs self-test passed; tests/guard-push.test.ts 33/33 passed | diff --git a/scripts/guard-push.mjs b/scripts/guard-push.mjs index 6857372e93..617e1bfbee 100755 --- a/scripts/guard-push.mjs +++ b/scripts/guard-push.mjs @@ -3,13 +3,18 @@ * guard-push — pre-push safety net for this repo's known, repeated traps. * * Runs five independent guards; any one can BLOCK the push (non-zero exit). - * All except the auto-merge ownership guard have an explicit override env var: + * All except the auto-merge force-push guard have an explicit override env var: * - * 1. Auto-merge ownership guard (all PR branches) - * Per-PR auto-merge state is user-owned. Pushing to a PR whose auto-merge is - * already armed can race the merge and, for actors without write permission, - * GitHub disables auto-merge. If `gh` reports an armed autoMergeRequest for - * the current branch's open PR, block without an automation override. + * 1. Auto-merge force-push guard (all PR branches) + * Per-PR auto-merge state is user-owned. An ordinary fast-forward push to a PR + * whose auto-merge is already armed is safe — GitHub re-validates required + * checks against the new head before it will merge, so an additive commit + * cannot make it merge something unvalidated. That is allowed through with a + * warning note. A force-push (history rewrite) while armed is different: it + * can discard the commit GitHub already validated or is mid-evaluating, and + * for actors without write permission GitHub disables auto-merge outright. If + * `gh` reports an armed autoMergeRequest AND this push force-updates that + * branch, block without an automation override. * Fails OPEN (never blocks) when gh is missing/unauthenticated, so * contributors without gh can still push. * @@ -126,6 +131,26 @@ function isAncestor(ancestor, descendant, cwd = PROJECT_ROOT) { } } +/** Exported for tests: true when this range rewrites the remote branch's history + * (force-push) rather than fast-forwarding it. A brand-new branch (remote sha + * all-zero) is never a force-push — there is no prior tip to discard. */ +export function isForcePushRange(range, cwd = PROJECT_ROOT) { + if (!range.remoteSha || range.remoteSha === ZERO_SHA) return false; + return !isAncestor(range.remoteSha, range.localSha, cwd); +} + +/** Exported for tests: branches this push force-updates — the one case the + * auto-merge guard still hard-blocks while armed, with no override. */ +export function forcePushedBranchNames(ranges, cwd = PROJECT_ROOT) { + const branches = new Set(); + for (const range of ranges) { + if (!isForcePushRange(range, cwd)) continue; + const ref = range.remoteRef || range.localRef || ""; + if (ref.startsWith("refs/heads/")) branches.add(ref.slice("refs/heads/".length)); + } + return branches; +} + /** Merge base with origin/main — the base a PR is actually evaluated against, and * the same one CI passes as LEDGER_WRITE_BASE_SHA (.github/workflows/ci.yml). */ function mainMergeBase(range, cwd = PROJECT_ROOT) { @@ -217,20 +242,34 @@ function ghIsAvailable() { } } -/** Exported for tests: decide from a parsed `gh pr view` payload. */ -export function autoMergeVerdict(branch, prPayload) { - if (!prPayload) return { block: false, reason: "no-open-pr" }; - if (prPayload.state && prPayload.state !== "OPEN") return { block: false, reason: "pr-not-open" }; +/** + * Exported for tests: decide from a parsed `gh pr view` payload. + * + * Auto-merge itself is user-owned, so automation never disables/re-enables it — + * that stays a hard, unconditional rule with no code path here at all. An + * ordinary fast-forward push while armed is allowed through (with a warning): + * GitHub re-validates required checks against the new head before it merges, so + * an additive commit cannot make it merge something that was never validated. A + * force-push while armed is the actual race — it can discard the commit GitHub + * already validated or is mid-evaluating — so that alone still blocks. + */ +export function autoMergeVerdict(branch, prPayload, isForcePush = false) { + if (!prPayload) return { block: false, warn: false, reason: "no-open-pr" }; + if (prPayload.state && prPayload.state !== "OPEN") return { block: false, warn: false, reason: "pr-not-open" }; if (prPayload.autoMergeRequest) { - return { block: true, reason: "auto-merge-armed", number: prPayload.number }; + if (isForcePush) { + return { block: true, warn: false, reason: "auto-merge-armed-force-push", number: prPayload.number }; + } + return { block: false, warn: true, reason: "auto-merge-armed-fast-forward", number: prPayload.number }; } - return { block: false, reason: "auto-merge-not-armed" }; + return { block: false, warn: false, reason: "auto-merge-not-armed" }; } -function autoMergeGuard(branches) { +function autoMergeGuard(branches, forcePushBranches = new Set()) { if (!ghIsAvailable()) { return { name: "auto-merge", ok: true, note: "gh not available — auto-merge check skipped (fail-open)" }; } + const warnings = []; for (const branch of branches) { let payload; try { @@ -243,17 +282,29 @@ function autoMergeGuard(branches) { // No PR for this branch, or gh unauthenticated: fail open. continue; } - const verdict = autoMergeVerdict(branch, payload); + const verdict = autoMergeVerdict(branch, payload, forcePushBranches.has(branch)); if (verdict.block) { return { name: "auto-merge", ok: false, message: - `PR #${verdict.number} on ${branch} has auto-merge ARMED.\n` + - ` Auto-merge state is user-owned; automation must not disable it or push through it.\n` + - ` Leave this PR untouched until it merges or the user manually changes that state.`, + `PR #${verdict.number} on ${branch} has auto-merge ARMED and this push force-updates the branch.\n` + + ` A force-push while armed can discard the commit GitHub already validated or is mid-evaluating.\n` + + ` Push a fast-forward commit instead, or wait for the user to change the auto-merge state. No override.`, }; } + if (verdict.warn) { + warnings.push(`PR #${verdict.number} on ${branch} has auto-merge ARMED — pushing anyway (fast-forward).`); + } + } + if (warnings.length > 0) { + return { + name: "auto-merge", + ok: true, + note: + warnings.join(" ") + + " Auto-merge state is still user-owned — do not disable/re-enable it, and never force-push while armed.", + }; } return { name: "auto-merge", ok: true }; } @@ -969,10 +1020,11 @@ function main() { if (ranges.length === 0) process.exit(0); // deletion-only push or nothing to do const branch = currentBranch(); const pushedBranches = pushedBranchNames(ranges, branch); + const forcePushBranches = forcePushedBranchNames(ranges); const changedFiles = collectChangedFiles(ranges); // formatGuard reads the pushed blobs; drift/static only need the paths. const results = [ - autoMergeGuard(pushedBranches), + autoMergeGuard(pushedBranches, forcePushBranches), formatGuard(collectChangedBlobs(ranges)), driftGuard(changedFiles), staticGuard(changedFiles, { ranges }), @@ -1003,12 +1055,17 @@ function assert(condition, label) { function selfTest() { // auto-merge verdicts assert( - autoMergeVerdict("codex/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 6 }).block === true, - "armed auto-merge on codex/* blocks", + autoMergeVerdict("codex/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 6 }).block === false, + "armed auto-merge does not block a fast-forward push", + ); + assert( + autoMergeVerdict("codex/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 6 }).warn === true, + "armed auto-merge still warns on a fast-forward push", ); assert( - autoMergeVerdict("claude/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 7 }).block === true, - "armed auto-merge on claude/* blocks", + autoMergeVerdict("claude/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 7 }, true).block === + true, + "armed auto-merge blocks a force-push", ); assert( autoMergeVerdict("claude/x", { autoMergeRequest: null, state: "OPEN" }).block === false, diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 82ae1b70d5..c54af101e5 100644 --- a/tests/guard-push.test.ts +++ b/tests/guard-push.test.ts @@ -9,6 +9,7 @@ import { changedFilesForRange, driftVerdict, findPrettierBin, + forcePushedBranchNames, formatGuard, guardBaseForRange, HEAVY_RUN_ADMISSION_BUSY_EXIT, @@ -16,6 +17,7 @@ import { isCoordinatorBusyOutput, isCoordinatorBusyResult, isEslintPolicyFile, + isForcePushRange, isTypecheckExcludedPath, lintableFiles, needsRepoWideLint, @@ -78,20 +80,22 @@ describe("guard-push sha parity", () => { }); describe("auto-merge verdict", () => { - it("blocks any PR branch with armed auto-merge", () => { - expect(autoMergeVerdict("codex/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 6 }).block).toBe( - true, - ); + it("does not block a fast-forward push to a PR branch with armed auto-merge, but warns", () => { + const v = autoMergeVerdict("codex/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 6 }); + expect(v.block).toBe(false); + expect(v.warn).toBe(true); }); - it("blocks a claude/* branch with an armed auto-merge on an open PR", () => { - const v = autoMergeVerdict("claude/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 7 }); + it("blocks a force-push to a claude/* branch with armed auto-merge on an open PR", () => { + const v = autoMergeVerdict("claude/x", { autoMergeRequest: { enabledAt: "t" }, state: "OPEN", number: 7 }, true); expect(v.block).toBe(true); expect(v.number).toBe(7); }); - it("does not block when auto-merge is not armed", () => { - expect(autoMergeVerdict("claude/x", { autoMergeRequest: null, state: "OPEN" }).block).toBe(false); + it("does not warn or block a force-push when auto-merge is not armed", () => { + const v = autoMergeVerdict("claude/x", { autoMergeRequest: null, state: "OPEN" }, true); + expect(v.block).toBe(false); + expect(v.warn).toBe(false); }); it("does not block when there is no open PR", () => { @@ -103,6 +107,46 @@ describe("auto-merge verdict", () => { }); }); +describe("force-push detection", () => { + it("does not flag a fast-forward push", () => { + const { root, git } = gitFixture(); + writeFileSync(join(root, "one.md"), "one\n"); + git("add", "one.md"); + git("commit", "--quiet", "-m", "one"); + const remoteSha = git("rev-parse", "HEAD"); + writeFileSync(join(root, "two.md"), "two\n"); + git("add", "two.md"); + git("commit", "--quiet", "-m", "two"); + const localSha = git("rev-parse", "HEAD"); + + expect(isForcePushRange({ localSha, remoteSha, remoteRef: "refs/heads/feature" }, root)).toBe(false); + expect(forcePushedBranchNames([{ localSha, remoteSha, remoteRef: "refs/heads/feature" }], root)).toEqual(new Set()); + }); + + it("flags a push that abandons the remote tip (history rewrite)", () => { + const { root, git, baseSha } = gitFixture(); + writeFileSync(join(root, "abandoned.md"), "abandoned\n"); + git("add", "abandoned.md"); + git("commit", "--quiet", "-m", "abandoned"); + const remoteSha = git("rev-parse", "HEAD"); + + git("reset", "--quiet", "--hard", baseSha); + writeFileSync(join(root, "rebuilt.md"), "rebuilt\n"); + git("add", "rebuilt.md"); + git("commit", "--quiet", "-m", "rebuilt"); + const localSha = git("rev-parse", "HEAD"); + + expect(isForcePushRange({ localSha, remoteSha, remoteRef: "refs/heads/feature" }, root)).toBe(true); + expect(forcePushedBranchNames([{ localSha, remoteSha, remoteRef: "refs/heads/feature" }], root)).toEqual( + new Set(["feature"]), + ); + }); + + it("never flags a brand-new branch (zero remote sha) as a force-push", () => { + expect(isForcePushRange({ localSha: "abc123", remoteSha: ZERO, remoteRef: "refs/heads/feature" })).toBe(false); + }); +}); + describe("manual auto-merge ownership policy", () => { it("keeps active agent policies aligned on preserving an armed PR", () => { const policyFiles = [