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'],