From 9a31a34fccd6199ca51f383c43ec714435d9f341 Mon Sep 17 00:00:00 2001 From: NagyVikt Date: Thu, 23 Apr 2026 00:05:30 +0200 Subject: [PATCH] Prevent finish reruns from reopening duplicate PR waits A stale post-merge branch can survive with its worktree still present, and rerunning gx branch finish on that same head was creating a brand-new PR instead of treating the lane as already shipped. The finish flow now checks merged PR history for the exact source HEAD before PR creation so cleanup reruns stay bounded and idempotent. Constraint: Keep legitimate follow-up PRs working when the branch name is reused with a different head commit Rejected: Skip PR flow for any previously merged branch name | branch names can be reused for new commits and still need a real PR Confidence: high Scope-risk: narrow Directive: Keep the merged-head guard tied to exact HEAD SHA matches; do not widen it to branch-name-only detection Tested: bash -n scripts/agent-branch-finish.sh templates/scripts/agent-branch-finish.sh Tested: node --test test/finish.test.js Tested: node --test test/metadata.test.js Tested: git diff --check Not-tested: Live GitHub cleanup rerun against a hosted repo after a previously merged PR --- .../.openspec.yaml | 2 + .../notes.md | 20 +++++ scripts/agent-branch-finish.sh | 49 ++++++++++++ templates/scripts/agent-branch-finish.sh | 49 ++++++++++++ test/finish.test.js | 79 +++++++++++++++++++ 5 files changed, 199 insertions(+) create mode 100644 openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/.openspec.yaml create mode 100644 openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/notes.md diff --git a/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/.openspec.yaml b/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/.openspec.yaml new file mode 100644 index 00000000..25345f42 --- /dev/null +++ b/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-04-22 diff --git a/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/notes.md b/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/notes.md new file mode 100644 index 00000000..fa555ff3 --- /dev/null +++ b/openspec/changes/agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58/notes.md @@ -0,0 +1,20 @@ +# agent-codex-prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58 (minimal / T1) + +Branch: `agent/codex/prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58` + +`gx branch finish --via-pr --wait-for-merge --cleanup` can reopen a fresh PR when the same source branch head already shipped in an earlier merged PR but the local branch/worktree cleanup was left behind. That turns a cleanup rerun into a new merge-wait loop instead of the bounded cleanup pass the user expected. + +Scope: +- Before PR create/merge, detect whether the current source branch HEAD already matches a merged PR for the same branch/base pair. +- If that exact head already landed, skip new PR creation and continue straight to local cleanup while preserving merged PR context in the logs. +- Add a focused finish regression that fails if a rerun opens or merges a duplicate PR for an already-merged head. + +Verification: +- `bash -n scripts/agent-branch-finish.sh templates/scripts/agent-branch-finish.sh` +- `node --test test/finish.test.js` + +## Cleanup + +- [ ] Run `gx branch finish --branch agent/codex/prevent-finish-rerun-from-opening-duplic-2026-04-22-23-58 --base main --via-pr --wait-for-merge --cleanup` +- [ ] Record PR URL + `MERGED` state in the completion handoff. +- [ ] Confirm sandbox worktree is gone (`git worktree list`, `git branch -a`). diff --git a/scripts/agent-branch-finish.sh b/scripts/agent-branch-finish.sh index 3e7e1493..456abaec 100755 --- a/scripts/agent-branch-finish.sh +++ b/scripts/agent-branch-finish.sh @@ -455,6 +455,44 @@ read_pr_state() { return 0 } +read_merged_pr_for_head() { + local head_sha="${1:-}" + local state_line="" + local parsed_state="" + local parsed_merged_at="" + local parsed_url="" + + if [[ -z "$head_sha" ]]; then + return 1 + fi + + state_line="$("$GH_BIN" pr list \ + --state merged \ + --head "$SOURCE_BRANCH" \ + --base "$BASE_BRANCH" \ + --json state,mergedAt,url,headRefOid \ + --jq "map(select(.headRefOid == \"$head_sha\")) | sort_by(.mergedAt // \"\") | reverse | (.[0] // {}) | [(.state // \"\"), (.mergedAt // \"\"), (.url // \"\")] | join(\"\u001f\")" \ + 2>/dev/null || true)" + if [[ -z "$state_line" ]]; then + return 1 + fi + + IFS=$'\x1f' read -r parsed_state parsed_merged_at parsed_url <<< "$state_line" + if [[ -z "$parsed_state" && -z "$parsed_merged_at" && -z "$parsed_url" ]]; then + return 1 + fi + if [[ "$parsed_state" != "MERGED" && -z "$parsed_merged_at" ]]; then + return 1 + fi + + PR_STATE="$parsed_state" + PR_MERGED_AT="$parsed_merged_at" + if [[ -n "$parsed_url" ]]; then + pr_url="$parsed_url" + fi + return 0 +} + wait_for_pr_merge() { local deadline deadline=$(( $(date +%s) + WAIT_TIMEOUT_SECONDS )) @@ -509,11 +547,22 @@ wait_for_pr_merge() { } run_pr_flow() { + local source_head_sha="" + if ! command -v "$GH_BIN" >/dev/null 2>&1; then echo "[agent-branch-finish] PR fallback requested but GitHub CLI not found: ${GH_BIN}" >&2 return 1 fi + source_head_sha="$(git -C "$repo_root" rev-parse "$SOURCE_BRANCH" 2>/dev/null || true)" + if read_merged_pr_for_head "$source_head_sha"; then + echo "[agent-branch-finish] Source branch head already landed in a merged PR; skipping new PR creation and continuing cleanup." >&2 + if [[ -n "$pr_url" ]]; then + echo "[agent-branch-finish] Merged PR: ${pr_url}" >&2 + fi + return 0 + fi + git -C "$source_worktree" push -u origin "$SOURCE_BRANCH" pr_title="$(git -C "$repo_root" log -1 --pretty=%s "$SOURCE_BRANCH" 2>/dev/null || true)" diff --git a/templates/scripts/agent-branch-finish.sh b/templates/scripts/agent-branch-finish.sh index 3e7e1493..456abaec 100755 --- a/templates/scripts/agent-branch-finish.sh +++ b/templates/scripts/agent-branch-finish.sh @@ -455,6 +455,44 @@ read_pr_state() { return 0 } +read_merged_pr_for_head() { + local head_sha="${1:-}" + local state_line="" + local parsed_state="" + local parsed_merged_at="" + local parsed_url="" + + if [[ -z "$head_sha" ]]; then + return 1 + fi + + state_line="$("$GH_BIN" pr list \ + --state merged \ + --head "$SOURCE_BRANCH" \ + --base "$BASE_BRANCH" \ + --json state,mergedAt,url,headRefOid \ + --jq "map(select(.headRefOid == \"$head_sha\")) | sort_by(.mergedAt // \"\") | reverse | (.[0] // {}) | [(.state // \"\"), (.mergedAt // \"\"), (.url // \"\")] | join(\"\u001f\")" \ + 2>/dev/null || true)" + if [[ -z "$state_line" ]]; then + return 1 + fi + + IFS=$'\x1f' read -r parsed_state parsed_merged_at parsed_url <<< "$state_line" + if [[ -z "$parsed_state" && -z "$parsed_merged_at" && -z "$parsed_url" ]]; then + return 1 + fi + if [[ "$parsed_state" != "MERGED" && -z "$parsed_merged_at" ]]; then + return 1 + fi + + PR_STATE="$parsed_state" + PR_MERGED_AT="$parsed_merged_at" + if [[ -n "$parsed_url" ]]; then + pr_url="$parsed_url" + fi + return 0 +} + wait_for_pr_merge() { local deadline deadline=$(( $(date +%s) + WAIT_TIMEOUT_SECONDS )) @@ -509,11 +547,22 @@ wait_for_pr_merge() { } run_pr_flow() { + local source_head_sha="" + if ! command -v "$GH_BIN" >/dev/null 2>&1; then echo "[agent-branch-finish] PR fallback requested but GitHub CLI not found: ${GH_BIN}" >&2 return 1 fi + source_head_sha="$(git -C "$repo_root" rev-parse "$SOURCE_BRANCH" 2>/dev/null || true)" + if read_merged_pr_for_head "$source_head_sha"; then + echo "[agent-branch-finish] Source branch head already landed in a merged PR; skipping new PR creation and continuing cleanup." >&2 + if [[ -n "$pr_url" ]]; then + echo "[agent-branch-finish] Merged PR: ${pr_url}" >&2 + fi + return 0 + fi + git -C "$source_worktree" push -u origin "$SOURCE_BRANCH" pr_title="$(git -C "$repo_root" log -1 --pretty=%s "$SOURCE_BRANCH" 2>/dev/null || true)" diff --git a/test/finish.test.js b/test/finish.test.js index a03eaf5e..07b39594 100644 --- a/test/finish.test.js +++ b/test/finish.test.js @@ -466,6 +466,85 @@ exit 1 assert.equal(result.stdout.trim(), '', 'agent branch should be deleted on origin'); }); +test('agent-branch-finish cleanup skips duplicate PR creation when the current head already landed in a merged PR', () => { + const repoDir = initRepo(); + seedCommit(repoDir); + attachOriginRemote(repoDir); + + let result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + result = runCmd('git', ['add', '.'], repoDir); + assert.equal(result.status, 0, result.stderr); + result = runCmd('git', ['commit', '-m', 'apply gx setup'], repoDir, { + ALLOW_COMMIT_ON_PROTECTED_BRANCH: '1', + }); + assert.equal(result.status, 0, result.stderr); + result = runCmd('git', ['push', 'origin', 'dev'], repoDir); + assert.equal(result.status, 0, result.stderr); + + result = runCmd('git', ['checkout', '-b', 'agent/test-pr-merged-head-rerun'], repoDir); + assert.equal(result.status, 0, result.stderr); + commitFile(repoDir, 'agent-pr-merged-head-rerun.txt', 'merged head rerun\n', 'merged head rerun change'); + result = runCmd('git', ['push', '-u', 'origin', 'agent/test-pr-merged-head-rerun'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + + result = runCmd('git', ['rev-parse', 'HEAD'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + const headSha = result.stdout.trim(); + + const { fakePath: fakeGhPath } = createFakeGhScript(` +if [[ "$1" == "pr" && "$2" == "list" ]]; then + if [[ " $* " == *" --state merged "* ]] && [[ " $* " == *" --json state,mergedAt,url,headRefOid "* ]]; then + printf 'MERGED\\x1f2026-04-22T21:37:13Z\\x1fhttps://example.test/pr/already-merged-head\\n' + exit 0 + fi + echo "unexpected gh pr list args: $*" >&2 + exit 1 +fi +if [[ "$1" == "pr" && "$2" == "create" ]]; then + echo "duplicate PR creation should have been skipped" >&2 + exit 1 +fi +if [[ "$1" == "pr" && "$2" == "merge" ]]; then + echo "duplicate PR merge should have been skipped" >&2 + exit 1 +fi +if [[ "$1" == "pr" && "$2" == "view" ]]; then + echo "unexpected gh pr view args: $*" >&2 + exit 1 +fi +echo "unexpected gh args: $*" >&2 +exit 1 +`); + + const finish = runBranchFinish( + ['--branch', 'agent/test-pr-merged-head-rerun', '--base', 'dev', '--mode', 'pr', '--cleanup'], + repoDir, + { + GUARDEX_GH_BIN: fakeGhPath, + GUARDEX_TEST_HEAD_SHA: headSha, + }, + ); + assert.equal(finish.status, 0, finish.stderr || finish.stdout); + assert.match( + finish.stderr, + /Source branch head already landed in a merged PR; skipping new PR creation and continuing cleanup\./, + ); + assert.match( + finish.stderr, + /Merged PR: https:\/\/example\.test\/pr\/already-merged-head/, + ); + assert.match( + finish.stdout, + /Merged 'agent\/test-pr-merged-head-rerun' into 'dev' via pr flow and cleaned source branch\/worktree\./, + ); + + result = runCmd('git', ['show-ref', '--verify', '--quiet', 'refs/heads/agent/test-pr-merged-head-rerun'], repoDir); + assert.notEqual(result.status, 0, 'agent branch should be deleted locally'); + result = runCmd('git', ['ls-remote', '--heads', 'origin', 'agent/test-pr-merged-head-rerun'], repoDir); + assert.equal(result.stdout.trim(), '', 'agent branch should be deleted on origin'); +}); + test('agent-branch-finish cleanup succeeds from active agent worktree when base branch is checked out elsewhere', () => { const repoDir = initRepo();