fix(runs): advance the re-arm baseline so a resolved re-drive cannot orphan resolution commits#78
fix(runs): advance the re-arm baseline so a resolved re-drive cannot orphan resolution commits#78Pawel-N-pl wants to merge 3 commits into
Conversation
rearm_escalation flipped the story back to PENDING but left baseline_commit at the escalated attempt's pre-attempt snapshot. When the resolve session had committed authorized work on the project branch (e.g. a baseline fixture the escalation was about), the redrive's reset-to-baseline in engine._rollback_or_pause parked those commits on an attempt-preserve ref and rebuilt against the unresolved tree — the re-driven dev session immediately hit the very gap the human had just resolved and re-escalated. Advance baseline_commit to the project's HEAD and refresh the untracked snapshot at re-arm time, so resolution work counts as the rebuild's starting point rather than attempt debris. Best-effort: a git failure keeps the old baseline. The story-escalation-resolved journal event now records the adopted baseline.
|
Warning Review limit reached
Next review available in: 36 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 Run ID: 📒 Files selected for processing (2)
WalkthroughThe ChangesRearm escalation baseline update
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant rearm_escalation
participant GitRepo
participant Journal
Caller->>rearm_escalation: rearm_escalation(task)
rearm_escalation->>GitRepo: rev_parse_head()
GitRepo-->>rearm_escalation: HEAD commit or GitError
rearm_escalation->>GitRepo: untracked_files()
GitRepo-->>rearm_escalation: untracked file list or GitError
rearm_escalation->>rearm_escalation: update baseline_commit / baseline_untracked
rearm_escalation->>Journal: append("story-escalation-resolved", baseline)
Possibly related PRs
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 |
🤖 Augment PR SummarySummary: This PR fixes escalation re-drive behavior so a completed resolve session can’t be rolled back as “attempt debris.” Changes:
Technical Notes: Baseline advancement is best-effort (git failures retain the old baseline) to keep re-arm resilient. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| # just loses this protection). | ||
| try: | ||
| repo = Path(state.project) | ||
| task.baseline_commit = verify.rev_parse_head(repo) |
There was a problem hiding this comment.
At task.baseline_commit = verify.rev_parse_head(repo): if rev_parse_head() succeeds but untracked_files() fails, the baseline commit will advance while baseline_untracked stays stale, which can make verify.safe_rollback() treat pre-existing untracked files as run-created and delete them.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| repo = Path(state.project) | ||
| task.baseline_commit = verify.rev_parse_head(repo) | ||
| task.baseline_untracked = sorted(verify.untracked_files(repo)) | ||
| except verify.GitError: |
There was a problem hiding this comment.
Catching only verify.GitError means other git-call failures (e.g. subprocess.TimeoutExpired/OSError from subprocess.run) would still raise and make rearm_escalation fail, despite the “best-effort” intent described above this block.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![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
🧹 Nitpick comments (1)
tests/test_resolve.py (1)
178-207: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM! Good coverage of the success and non-repo tolerance paths.
Consider also adding a case where
rev_parse_headsucceeds butuntracked_filesfails, to cover the partial-update risk flagged inruns.py(baseline_commitadvancing whilebaseline_untrackedstays stale).
🤖 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/runs.py`:
- Around line 550-566: The best-effort baseline refresh in runs.py can leave
task.baseline_commit and task.baseline_untracked out of sync if
verify.rev_parse_head succeeds but verify.untracked_files raises GitError.
Update the logic in the baseline refresh block so both git reads are completed
first and only then assign task.baseline_commit and task.baseline_untracked
together, keeping the pair consistent for attempt_dirty and safe_rollback. If
either git call fails, leave both existing baseline values unchanged.
🪄 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
Run ID: d3012ab2-2cc7-45a4-85ac-4c0be7d47ed9
📒 Files selected for processing (2)
src/bmad_loop/runs.pytests/test_resolve.py
…on partial git failure Both git reads in rearm_escalation's baseline refresh now land in locals before either task field is assigned, and the except widens from GitError to Exception (matching verify._prune_refs' best-effort contract for the whole subprocess surface: a timeout or missing-binary OSError must not fail re-arm either). Previously, rev_parse_head succeeding while untracked_files raised would advance baseline_commit alone, leaving baseline_untracked stale and the two fields describing different points in time - exactly the pairing attempt_dirty/safe_rollback rely on to tell pre-existing untracked files from run-created ones. Addresses review feedback from PR #78 (CodeRabbit + Augment, converging on the same finding).
…xcept CI's trunk check (bandit) flagged the bare except/pass in rearm_escalation's baseline refresh as B110 (try/except/pass). It is deliberately best-effort (a git failure must not fail re-arm), matching the same pattern already used elsewhere in the codebase (e.g. verify._prune_refs, Engine's teardown handlers) - add the same noqa/nosec suppression with a short inline reason. Verified locally with bandit (via uvx): 0 issues, 1 potential issue specifically suppressed by the new nosec marker.
1 participant
What
rearm_escalationnow advancesbaseline_committo the project's current HEAD (and refreshes the untracked-files snapshot) instead of leaving it at the pre-attempt snapshot.Why
A resolve session can legitimately commit work (e.g. the fixture the escalation asked for). The re-drive's reset-to-baseline still targeted the old baseline, so it parked that commit on an
attempt-preserve/*ref and rebuilt against the unresolved tree, and the re-driven session hit the same gap and re-escalated verbatim. This complements other recent fixes for staleAuto Run Resulthandling and resume durability.Rebased onto v0.8.1; the only conflict was a docstring/adjacent-lines overlap in
rearm_escalationwith an upstream addition to the same function, resolved by keeping both. Independent of the other pending PR (ledger-only proof-of-work fix); different files, either can land first.How
baseline_commit = verify.rev_parse_head(project), re-snapshotbaseline_untracked.GitErrorleaves the old baseline in place.Testing
Two new tests in
tests/test_resolve.py(adopts HEAD + re-snapshots; stays stale outside a repo). Full suite: 1407 passed, 7 skipped; ruff clean.Summary by CodeRabbit
Bug Fixes
Tests