fix(recovery): probe for a free attempt-preserve-dirty ref name - #353
fix(recovery): probe for a free attempt-preserve-dirty ref name#353Pawel-N-pl wants to merge 2 commits into
Conversation
rearm_escalation resets task.attempt to 0, so a post-resolve re-drive's rollback recomputed the same attempt-preserve-dirty ref name as the pre-resolve rollback and overwrote that snapshot, destroying the only copy of the first attempt's uncommitted work. Probe for a free refname (new verify.ref_exists) and suffix -r2/-r3/... on collision instead of trusting the counter.
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughDirty rollback snapshots now avoid overwriting existing preservation refs by probing full Git refnames and adding deterministic retry suffixes. Tests cover repeated attempt counters, ref types, absent refs, and Git failures. ChangesRecovery ref uniqueness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a snapshot-collision bug where
Confidence Score: 5/5Safe to merge — the probe loop is correctly placed inside the existing try/except block and two targeted tests confirm the fix is correct and the fault-tolerance contract is preserved. The fix is narrowly scoped: one helper function and a change to the ref-naming logic in a single method. No pre-existing behaviour is changed on the non-collision path. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/verify.py | Adds ref_exists(repo, refname) using git show-ref --verify --quiet; correctly distinguishes non-zero exit (returns False) from spawn/timeout exceptions (propagates). |
| src/bmad_loop/recovery_flow.py | Replaces the fixed ref name with a probe loop appending -r2, -r3 suffixes; probe loop is correctly inside the existing try/except (GitError, OSError) block. |
| tests/test_engine.py | Adds integration test for the re-arm collision scenario covering three consecutive same-counter rollbacks with distinct refs. |
| tests/test_recovery_flow.py | Adds ablation test confirming probe loop inside try: GitSpawnError from ref_exists yields a typed Pause. |
| tests/test_verify.py | Adds unit coverage for ref_exists across full refnames, ordinary branches, absent refs, and git failure cases. |
Reviews (2): Last reviewed commit: "fix(recovery): run the refname probe ins..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/verify.py`:
- Around line 527-534: Update ref_exists to catch the typed Git exceptions
raised by _git for timeouts and process-spawn failures, returning False for
those observation failures while preserving the existing rc == 0 check. Do not
broaden handling to repair/write operations or unrelated exceptions; preserve
preserve_attempt_worktree’s behavior for actual ref-write errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac32a5cc-eb31-411a-ab71-2e787f5e8c5c
📒 Files selected for processing (4)
src/bmad_loop/recovery_flow.pysrc/bmad_loop/verify.pytests/test_engine.pytests/test_verify.py
| def ref_exists(repo: Path, refname: str) -> bool: | ||
| """Whether ``refname`` — a FULL refname, e.g. ``refs/attempt-preserve-dirty/…`` | ||
| — currently exists. Sibling of :func:`branch_exists`, which prepends | ||
| ``refs/heads/`` and so cannot see the snapshot refs that live outside it. | ||
| Best-effort by design: any git failure reads as "absent" (the caller's | ||
| subsequent ref write surfaces the real error).""" | ||
| rc, _ = _git(repo, "show-ref", "--verify", "--quiet", refname) | ||
| return rc == 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make ref_exists degrade typed Git failures to False.
_git raises on timeouts and spawn failures, but this function only handles nonzero return codes. Because preserve_attempt_worktree calls it before its try block, those failures can abort rollback instead of following the documented best-effort behavior.
Proposed fix
- rc, _ = _git(repo, "show-ref", "--verify", "--quiet", refname)
+ try:
+ rc, _ = _git(repo, "show-ref", "--verify", "--quiet", refname)
+ except GitError:
+ return False
return rc == 0Based on coding guidelines, observation may degrade, but repair writes must raise.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def ref_exists(repo: Path, refname: str) -> bool: | |
| """Whether ``refname`` — a FULL refname, e.g. ``refs/attempt-preserve-dirty/…`` | |
| — currently exists. Sibling of :func:`branch_exists`, which prepends | |
| ``refs/heads/`` and so cannot see the snapshot refs that live outside it. | |
| Best-effort by design: any git failure reads as "absent" (the caller's | |
| subsequent ref write surfaces the real error).""" | |
| rc, _ = _git(repo, "show-ref", "--verify", "--quiet", refname) | |
| return rc == 0 | |
| def ref_exists(repo: Path, refname: str) -> bool: | |
| """Whether ``refname`` — a FULL refname, e.g. ``refs/attempt-preserve-dirty/…`` | |
| — currently exists. Sibling of :func:`branch_exists`, which prepends | |
| ``refs/heads/`` and so cannot see the snapshot refs that live outside it. | |
| Best-effort by design: any git failure reads as "absent" (the caller's | |
| subsequent ref write surfaces the real error).""" | |
| try: | |
| rc, _ = _git(repo, "show-ref", "--verify", "--quiet", refname) | |
| except GitError: | |
| return False | |
| return rc == 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bmad_loop/verify.py` around lines 527 - 534, Update ref_exists to catch
the typed Git exceptions raised by _git for timeouts and process-spawn failures,
returning False for those observation failures while preserving the existing rc
== 0 check. Do not broaden handling to repair/write operations or unrelated
exceptions; preserve preserve_attempt_worktree’s behavior for actual ref-write
errors.
Source: Coding guidelines
`ref_exists` spawns git, and `_run_git` translates a timeout or spawn fault into GitError/GitSpawnError rather than a return code — so with the probe above the try, either would have crashed the rollback uncaught, which is precisely what that handler exists to prevent. Move the loop inside it: a probe that cannot run now degrades into the same "preservation is observation" pause as a snapshot that cannot be written. `ref_exists` deliberately does not swallow those faults: reading "git could not run" as "the name is free" would overwrite the snapshot the probe exists to protect. Docstring corrected to say so. Reported by Greptile on bmad-code-org#353.
cab1b1e to
04596ef
Compare
|
Good catch, and correct on both halves — thanks.
I kept Added |
|
Moving this to draft — I gather a larger rework is planned, so I'd rather not sit in your review queue against code that's about to change. The branch stays up and CI is green (py3.11–3.14, Windows, trunk, pyright). Happy to rebase it onto whatever lands, or close it if the rework covers this — just say the word. Issue #349 stays open either way, since the defect outlives the patch. |
1 participant
What
The dirty-tree rollback snapshot now probes for an unused ref name instead of deriving one from
task.attemptalone.Why
task.attemptis not monotonic across a story's life —runs.rearm_escalationsets it back to 0— so a post-resolve re-drive recomputes the pre-resolve rollback's exact ref name and overwrites
the only copy of that attempt's uncommitted work. The comment above the ref construction states
the opposite premise, which is what sent me looking.
Fixes #349.
How
verify.ref_exists(repo, refname), a sibling ofbranch_existsthat can see full refnamesoutside
refs/heads/. Best-effort: any git failure reads as absent.recovery_flow._preserve_dirty_attempt, keep{slug}-{baseline}-{attempt}as the base nameand append
-r2,-r3, … until free, so uniqueness is enforced against the refs that exist.Testing
Two tests, both confirmed to fail with the fix reverted: a re-arm that previously collided now
parks a second distinct ref with both snapshots intact, and
ref_existsis covered directly.Full suite: 3458 passed, 35 skipped.
Summary by CodeRabbit
Bug Fixes
Tests