From d8567cec031d3e7eb5a1aee53453dea485ef90c7 Mon Sep 17 00:00:00 2001 From: NagyVikt Date: Wed, 22 Apr 2026 21:42:09 +0200 Subject: [PATCH] Keep finish cleanup alive when the source branch disappears first GitHub can delete the local source branch during PR merge while the Guardex cleanup phase is still about to run its own branch delete step. That race was failing finish even though the branch was already gone and the remaining cleanup work could succeed. This change makes the local cleanup step re-check branch existence before and after `git branch -d`, treat an already-missing ref as informational, mirror the behavior into the template script, and lock the path with a focused finish regression plus metadata parity coverage. Constraint: Finish cleanup must still fail on real local branch deletion errors Rejected: Ignore all `git branch -d` failures | would hide genuine cleanup problems Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep the branch-exists recheck paired with the concrete already-missing log line in both runtime and template scripts Tested: node --test test/finish.test.js Tested: node --check bin/multiagent-safety.js Tested: node --test test/metadata.test.js Tested: openspec validate agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43 --type change --strict Tested: openspec validate --specs Not-tested: live GitHub PR merge against the real repository before finish flow --- .../proposal.md | 17 ++++ .../specs/workflow-guardrails/spec.md | 21 +++++ .../tasks.md | 23 +++++ scripts/agent-branch-finish.sh | 31 ++++++- templates/scripts/agent-branch-finish.sh | 31 ++++++- test/finish.test.js | 83 +++++++++++++++++++ test/metadata.test.js | 1 + 7 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/proposal.md create mode 100644 openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/specs/workflow-guardrails/spec.md create mode 100644 openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/tasks.md diff --git a/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/proposal.md b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/proposal.md new file mode 100644 index 0000000..09e4e41 --- /dev/null +++ b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/proposal.md @@ -0,0 +1,17 @@ +## Why + +- `scripts/agent-branch-finish.sh` currently treats `git branch -d ` as a hard cleanup failure even after the merge already succeeded. +- In the observed false-negative, `gh pr merge --delete-branch` reported that local branch deletion failed because of an active worktree, but by the time Guardex continued its own cleanup the local branch ref was already gone. +- That leaves the merge outcome correct but still exits non-zero, which forces needless manual bookkeeping follow-ups. + +## What Changes + +- Make finish cleanup tolerate an already-missing local source branch during the post-merge branch-delete step. +- Keep the existing warning for the GitHub CLI local-delete error, but continue cleanup when the branch ref has already disappeared. +- Add a focused finish regression for the race where the local branch is gone by the time Guardex reaches its own cleanup. + +## Impact + +- Affects only the post-merge cleanup path in `agent-branch-finish.sh`. +- Keeps true branch-delete failures fatal, but downgrades the already-deleted case to an informational cleanup warning. +- Reduces false-negative finish exits while preserving merge, remote-delete, and worktree-prune behavior. diff --git a/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/specs/workflow-guardrails/spec.md b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/specs/workflow-guardrails/spec.md new file mode 100644 index 0000000..0a46cf1 --- /dev/null +++ b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/specs/workflow-guardrails/spec.md @@ -0,0 +1,21 @@ +## ADDED Requirements + +### Requirement: finish cleanup tolerates an already-missing local source branch after merge +The `gx branch finish` cleanup flow SHALL treat the local source-branch delete step as successful when the branch ref is already absent by the time post-merge cleanup runs. + +#### Scenario: GitHub merge reports a local-branch delete problem but the branch is already gone during Guardex cleanup +- **GIVEN** `scripts/agent-branch-finish.sh` merges an `agent/*` branch through the PR flow +- **AND** the GitHub CLI reports a local branch delete problem during `gh pr merge --delete-branch` +- **AND** the local `refs/heads/` ref is already missing by the time Guardex reaches its own cleanup branch-delete step +- **WHEN** Guardex continues cleanup +- **THEN** the finish command SHALL keep going without failing +- **AND** it SHALL emit an informational warning that the local branch was already deleted +- **AND** it SHALL still continue remote-branch cleanup and worktree pruning + +#### Scenario: real local branch delete failures still fail finish cleanup +- **GIVEN** `scripts/agent-branch-finish.sh` reaches the local source-branch delete step +- **AND** the local `refs/heads/` ref still exists +- **AND** `git branch -d ` fails for a reason other than the branch already being absent +- **WHEN** Guardex handles cleanup +- **THEN** the finish command SHALL still fail +- **AND** it SHALL preserve the underlying git error output diff --git a/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/tasks.md b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/tasks.md new file mode 100644 index 0000000..5b5f87f --- /dev/null +++ b/openspec/changes/agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43/tasks.md @@ -0,0 +1,23 @@ +## 1. Specification + +- [x] 1.1 Finalize proposal scope and acceptance criteria for `agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43`. +- [x] 1.2 Define normative requirements in `specs/workflow-guardrails/spec.md`. + +## 2. Implementation + +- [x] 2.1 Update `scripts/agent-branch-finish.sh` so post-merge cleanup tolerates an already-missing local source branch. +- [x] 2.2 Mirror the same finish cleanup change into `templates/scripts/agent-branch-finish.sh`. +- [x] 2.3 Add a focused regression in `test/finish.test.js` for the local-branch-already-deleted race. + +## 3. Verification + +- [x] 3.1 Run focused finish verification (`node --test test/finish.test.js`, `node --check bin/multiagent-safety.js`). +- [x] 3.2 Run parity verification (`node --test test/metadata.test.js`). +- [x] 3.3 Run `openspec validate agent-codex-tolerate-already-deleted-local-branch-in-2026-04-22-20-43 --type change --strict`. +- [x] 3.4 Run `openspec validate --specs`. + +## 4. Completion + +- [ ] 4.1 Finish the agent branch via PR merge + cleanup (`gx finish --via-pr --wait-for-merge --cleanup` or `bash scripts/agent-branch-finish.sh --branch --base --via-pr --wait-for-merge --cleanup`). +- [ ] 4.2 Record PR URL + final `MERGED` state in the completion handoff. +- [ ] 4.3 Confirm sandbox cleanup (`git worktree list`, `git branch -a`) or capture a `BLOCKED:` handoff if merge/cleanup is pending. diff --git a/scripts/agent-branch-finish.sh b/scripts/agent-branch-finish.sh index 6c47249..3e7e149 100755 --- a/scripts/agent-branch-finish.sh +++ b/scripts/agent-branch-finish.sh @@ -409,6 +409,33 @@ is_remote_branch_missing_error() { return 1 } +local_branch_exists() { + local branch="$1" + git -C "$repo_root" show-ref --verify --quiet "refs/heads/${branch}" +} + +delete_local_branch_for_cleanup() { + local branch="$1" + local delete_output="" + + if ! local_branch_exists "$branch"; then + echo "[agent-branch-finish] Local branch '${branch}' was already deleted; continuing cleanup." >&2 + return 0 + fi + + if delete_output="$(git -C "$repo_root" branch -d "$branch" 2>&1)"; then + return 0 + fi + + if ! local_branch_exists "$branch"; then + echo "[agent-branch-finish] Local branch '${branch}' was already deleted; continuing cleanup." >&2 + return 0 + fi + + echo "$delete_output" >&2 + return 1 +} + read_pr_state() { local state_line state_line="$("$GH_BIN" pr view "$SOURCE_BRANCH" --json state,mergedAt,url --jq '[.state, (.mergedAt // ""), (.url // "")] | join("\u001f")' 2>/dev/null || true)" @@ -607,7 +634,9 @@ if [[ "$CLEANUP_AFTER_MERGE" -eq 1 ]]; then git -C "$repo_root" worktree remove "$source_worktree" --force >/dev/null 2>&1 || true fi - git -C "$repo_root" branch -d "$SOURCE_BRANCH" + if ! delete_local_branch_for_cleanup "$SOURCE_BRANCH"; then + exit 1 + fi if [[ "$PUSH_ENABLED" -eq 1 && "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then if git -C "$repo_root" ls-remote --exit-code --heads origin "$SOURCE_BRANCH" >/dev/null 2>&1; then diff --git a/templates/scripts/agent-branch-finish.sh b/templates/scripts/agent-branch-finish.sh index 6c47249..3e7e149 100755 --- a/templates/scripts/agent-branch-finish.sh +++ b/templates/scripts/agent-branch-finish.sh @@ -409,6 +409,33 @@ is_remote_branch_missing_error() { return 1 } +local_branch_exists() { + local branch="$1" + git -C "$repo_root" show-ref --verify --quiet "refs/heads/${branch}" +} + +delete_local_branch_for_cleanup() { + local branch="$1" + local delete_output="" + + if ! local_branch_exists "$branch"; then + echo "[agent-branch-finish] Local branch '${branch}' was already deleted; continuing cleanup." >&2 + return 0 + fi + + if delete_output="$(git -C "$repo_root" branch -d "$branch" 2>&1)"; then + return 0 + fi + + if ! local_branch_exists "$branch"; then + echo "[agent-branch-finish] Local branch '${branch}' was already deleted; continuing cleanup." >&2 + return 0 + fi + + echo "$delete_output" >&2 + return 1 +} + read_pr_state() { local state_line state_line="$("$GH_BIN" pr view "$SOURCE_BRANCH" --json state,mergedAt,url --jq '[.state, (.mergedAt // ""), (.url // "")] | join("\u001f")' 2>/dev/null || true)" @@ -607,7 +634,9 @@ if [[ "$CLEANUP_AFTER_MERGE" -eq 1 ]]; then git -C "$repo_root" worktree remove "$source_worktree" --force >/dev/null 2>&1 || true fi - git -C "$repo_root" branch -d "$SOURCE_BRANCH" + if ! delete_local_branch_for_cleanup "$SOURCE_BRANCH"; then + exit 1 + fi if [[ "$PUSH_ENABLED" -eq 1 && "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then if git -C "$repo_root" ls-remote --exit-code --heads origin "$SOURCE_BRANCH" >/dev/null 2>&1; then diff --git a/test/finish.test.js b/test/finish.test.js index 9160e21..a03eaf5 100644 --- a/test/finish.test.js +++ b/test/finish.test.js @@ -383,6 +383,89 @@ fi assert.equal(result.stdout.trim(), '', 'agent branch should be absent on origin'); }); +test('agent-branch-finish cleanup tolerates an already-deleted local branch after gh delete warning', () => { + 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); + + const agentWorktreePath = path.join(repoDir, '.omx', 'agent-worktrees', 'agent__local-delete-race'); + result = runCmd( + 'git', + ['worktree', 'add', '-b', 'agent/test-pr-local-delete-race', agentWorktreePath, 'dev'], + repoDir, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + + fs.writeFileSync(path.join(agentWorktreePath, 'local-delete-race.txt'), 'cleanup race\n', 'utf8'); + result = runCmd('git', ['add', 'local-delete-race.txt'], agentWorktreePath); + assert.equal(result.status, 0, result.stderr); + result = runCmd('git', ['commit', '--no-verify', '-m', 'local delete race change'], agentWorktreePath); + assert.equal(result.status, 0, result.stderr || result.stdout); + + const { fakePath: fakeGhPath } = createFakeGhScript(` +if [[ "$1" == "pr" && "$2" == "create" ]]; then + exit 0 +fi +if [[ "$1" == "pr" && "$2" == "view" ]]; then + if [[ " $* " == *" --json url "* ]]; then + echo "https://example.test/pr/local-delete-race" + exit 0 + fi + echo "unexpected gh pr view args: $*" >&2 + exit 1 +fi +if [[ "$1" == "pr" && "$2" == "merge" ]]; then + git_bin="$(command -v git)" + "$git_bin" -C "${'${GUARDEX_TEST_AGENT_WORKTREE}'}" checkout --detach >/dev/null 2>&1 || true + "$git_bin" -C "${'${GUARDEX_TEST_REPO_DIR}'}" branch -D "$3" >/dev/null 2>&1 || true + echo "failed to delete local branch $3: error: cannot delete branch '$3' used by worktree at '${'${GUARDEX_TEST_AGENT_WORKTREE}'}'" >&2 + echo "/usr/bin/git: exit status 1" >&2 + exit 1 +fi +echo "unexpected gh args: $*" >&2 +exit 1 +`); + + const finish = runBranchFinish( + ['--branch', 'agent/test-pr-local-delete-race', '--base', 'dev', '--mode', 'pr', '--cleanup'], + repoDir, + { + GUARDEX_GH_BIN: fakeGhPath, + GUARDEX_TEST_REPO_DIR: repoDir, + GUARDEX_TEST_AGENT_WORKTREE: agentWorktreePath, + }, + ); + assert.equal(finish.status, 0, finish.stderr || finish.stdout); + assert.match( + finish.stderr, + /PR merged but gh could not delete the local branch \(active worktree\); continuing local cleanup\./, + ); + assert.match( + finish.stderr, + /Local branch 'agent\/test-pr-local-delete-race' was already deleted; continuing cleanup\./, + ); + assert.match( + finish.stdout, + /Merged 'agent\/test-pr-local-delete-race' into 'dev' via pr flow and cleaned source branch\/worktree\./, + ); + + result = runCmd('git', ['show-ref', '--verify', '--quiet', 'refs/heads/agent/test-pr-local-delete-race'], repoDir); + assert.notEqual(result.status, 0, 'agent branch should stay deleted locally'); + result = runCmd('git', ['ls-remote', '--heads', 'origin', 'agent/test-pr-local-delete-race'], 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(); diff --git a/test/metadata.test.js b/test/metadata.test.js index 0ddc5e3..dcf634f 100644 --- a/test/metadata.test.js +++ b/test/metadata.test.js @@ -126,6 +126,7 @@ test('frontend mirror workflow skips cleanly when the mirror PAT is missing', () test('critical runtime helper scripts and active-agents sources stay in sync with templates', () => { const pairs = [ ['templates/scripts/agent-branch-start.sh', 'scripts/agent-branch-start.sh'], + ['templates/scripts/agent-branch-finish.sh', 'scripts/agent-branch-finish.sh'], ['templates/scripts/codex-agent.sh', 'scripts/codex-agent.sh'], ['templates/scripts/openspec/init-plan-workspace.sh', 'scripts/openspec/init-plan-workspace.sh'], ['templates/scripts/openspec/init-change-workspace.sh', 'scripts/openspec/init-change-workspace.sh'],