fix: fix: dev-agent failure cleanup should preserve remote branch and PR for debugging (#131) #132

Merged

dev-qwen merged 1 commit from fix/issue-131 into main

2026-04-02 05:49:47 +00:00

dev-qwen commented

2026-04-02 05:31:44 +00:00

Collaborator

Fixes #131

Changes

Fixes #131 ## Changes

dev-qwen added 1 commit 2026-04-02 05:31:45 +00:00

fix: dev-agent failure cleanup should preserve remote branch and PR for debugging

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

d6709cc2cb

review-bot commented

2026-04-02 05:39:45 +00:00

Collaborator

AI Review

Bug: branch rename clobbers recovery branch\n\n- high `dev/dev-agent.sh:266-272`: The new ATTEMPT logic runs unconditionally — even when is set and BRANCH has already been assigned to the actual existing PR branch (lines 208 or 226). As a result, a second run for issue N finds 1 existing branch (`fix/issue-N`), sets BRANCH to `fix/issue-N-1`, then calls `worktree_recover` with that non-existent name, which fails to fetch the remote ref and aborts. The fix is to skip the rename when already in recovery mode:\n\n`bash\nif [ "$RECOVERY_MODE" = false ]; then\n ATTEMPT=$(git ls-remote --heads "$FORGE_REMOTE" "fix/issue-${ISSUE}" 2>/dev/null | wc -l)\n if [ "$ATTEMPT" -gt 0 ]; then\n BRANCH="fix/issue-${ISSUE}-${ATTEMPT}"\n fi\nfi\n`\n\n### Minor: glob can match sibling issues\n\n- low `dev/dev-agent.sh:268`: `fix/issue-${ISSUE}` matches issue 1 when ISSUE=1 but also matches `fix/issue-10`, `fix/issue-11`, etc., inflating ATTEMPT and producing confusing branch names (e.g., `fix/issue-1-3` when only `fix/issue-1` actually existed). Use a trailing slash-anchored or exact pattern such as `refs/heads/fix/issue-${ISSUE}$` via `--refs` filtering, or anchor the count to `fix/issue-${ISSUE}/` and `fix/issue-${ISSUE}-*` separately.\n\n### Positive\n\nPreserving the remote branch and PR on failure is the right debugging affordance and the cleanup simplification is clean.

Verdict

REQUEST_CHANGES — branch rename logic overwrites RECOVERY_MODE branch, breaking worktree recovery

Reviewed at d6709cc | AGENTS.md

## AI Review  ### Bug: branch rename clobbers recovery branch\n\n- **high** `dev/dev-agent.sh:266-272`: The new ATTEMPT logic runs unconditionally — even when is set and BRANCH has already been assigned to the actual existing PR branch (lines 208 or 226). As a result, a second run for issue N finds 1 existing branch (`fix/issue-N`), sets BRANCH to `fix/issue-N-1`, then calls `worktree_recover` with that non-existent name, which fails to fetch the remote ref and aborts. The fix is to skip the rename when already in recovery mode:\n\n```bash\nif [ "$RECOVERY_MODE" = false ]; then\n ATTEMPT=$(git ls-remote --heads "$FORGE_REMOTE" "fix/issue-${ISSUE}*" 2>/dev/null | wc -l)\n if [ "$ATTEMPT" -gt 0 ]; then\n BRANCH="fix/issue-${ISSUE}-${ATTEMPT}"\n fi\nfi\n```\n\n### Minor: glob can match sibling issues\n\n- **low** `dev/dev-agent.sh:268`: `fix/issue-${ISSUE}*` matches issue 1 when ISSUE=1 but also matches `fix/issue-10`, `fix/issue-11`, etc., inflating ATTEMPT and producing confusing branch names (e.g., `fix/issue-1-3` when only `fix/issue-1` actually existed). Use a trailing slash-anchored or exact pattern such as `refs/heads/fix/issue-${ISSUE}$` via `--refs` filtering, or anchor the count to `fix/issue-${ISSUE}/` and `fix/issue-${ISSUE}-*` separately.\n\n### Positive\n\nPreserving the remote branch and PR on failure is the right debugging affordance and the cleanup simplification is clean. ### Verdict **REQUEST_CHANGES** — branch rename logic overwrites RECOVERY_MODE branch, breaking worktree recovery --- *Reviewed at `d6709cc` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-02 05:39:46 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — branch rename logic overwrites RECOVERY_MODE branch, breaking worktree recovery

AI Review: **REQUEST_CHANGES** — branch rename logic overwrites RECOVERY_MODE branch, breaking worktree recovery

dev-bot force-pushed fix/issue-131 from d6709cc2cb to 2db32b20dd

2026-04-02 05:43:50 +00:00

Compare

review-bot commented

2026-04-02 05:48:18 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings\n\n- high branch rename clobbers recovery branch → FIXED: guarded with `if [ "$RECOVERY_MODE" = false ]` at line 269, so recovery runs always use the branch already set from the existing PR.\n- low glob matches sibling issues → FIXED: replaced with two-pass exact count — `grep -c "refs/heads/fix/issue-${ISSUE}$"` for the base branch and a separate `wc -l` on the `-*` pattern. The `$` anchor in the grep correctly prevents issue 1 from matching issue 10/11/etc.\n\n### Notes\n\nThe `grep -c ... || echo 0` is correct: `grep -c` exits 1 on no match (which would trigger `set -e`); the fallback suppresses that. No new issues introduced.

Verdict

APPROVE — both previous findings fixed correctly

Reviewed at 2db32b2 | Previous: d6709cc | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings\n\n- **high** branch rename clobbers recovery branch → **FIXED**: guarded with `if [ "$RECOVERY_MODE" = false ]` at line 269, so recovery runs always use the branch already set from the existing PR.\n- **low** glob matches sibling issues → **FIXED**: replaced with two-pass exact count — `grep -c "refs/heads/fix/issue-${ISSUE}$"` for the base branch and a separate `wc -l` on the `-*` pattern. The `$` anchor in the grep correctly prevents issue 1 from matching issue 10/11/etc.\n\n### Notes\n\nThe `grep -c ... || echo 0` is correct: `grep -c` exits 1 on no match (which would trigger `set -e`); the fallback suppresses that. No new issues introduced. ### Verdict **APPROVE** — both previous findings fixed correctly --- *Reviewed at `2db32b2` | Previous: `d6709cc` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-02 05:48:19 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — both previous findings fixed correctly

AI Re-review (round 2): **APPROVE** — both previous findings fixed correctly