fix(verify): translate spawn OSError to GitSpawnError in _run_git (#343) - #344
Conversation
A spawn-level OSError (EMFILE/ENOMEM/ENOENT) out of subprocess.run escaped _run_git untyped, bypassing all 19 OSError-blind 'except GitError' guards and crashing the run under exactly the resource pressure the recovery paths owning those guards exist for (#341 found two unguarded frames this way). Translating at the chokepoint makes the default correct: every guard now holds as written, and GitSpawnError (a GitError) lets the rare caller tell an environment fault from git refusing, with the errno on __cause__. Site-level dispositions that change: the commit window escalates on a spawn fault (same as a git timeout there; non-spawn OSErrors still crash), a spawn fault opening a unit worktree pauses the run instead of marching the queue into DEFERRED, and merge-target reconciliation widens to (GitError, OSError) for clean_incoming_collisions' non-spawn FS ops — the one hole among the 19 the chokepoint cannot close. All four new guards ablation-verified (arm deleted, test fails, arm restored). Closes #343
|
Warning Review limit reached
Next review available in: 46 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)
WalkthroughChangesGit spawn fault recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant WorktreeFlow
participant verify._run_git
participant subprocess.run
Engine->>WorktreeFlow: open or merge unit worktree
WorktreeFlow->>verify._run_git: execute git operation
verify._run_git->>subprocess.run: spawn git
subprocess.run-->>verify._run_git: OSError
verify._run_git-->>WorktreeFlow: GitSpawnError
WorktreeFlow-->>Engine: pause or escalate
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/recovery_flow.py`:
- Around line 408-411: Update the adjacent preservation comment near _run_git to
state that spawn failures are translated into typed GitSpawnError, while
filesystem failures such as EMFILE or ENOMEM may still propagate as plain
OSError. Preserve the explanation that the OSError guard remains necessary for
snapshot_worktree and related rollback paths.
In `@tests/test_verify.py`:
- Around line 125-131: Extend the make_exc parameter matrix in the test
parametrization to include OSError(12, "Cannot allocate memory") and add the
corresponding "enomem" case ID, preserving the existing ENOENT and EMFILE
entries.
🪄 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: 022902d8-67ca-46d4-90a3-6439ec4ac206
📒 Files selected for processing (12)
CHANGELOG.mdsrc/bmad_loop/engine.pysrc/bmad_loop/recovery_flow.pysrc/bmad_loop/verify.pysrc/bmad_loop/worktree_flow.pytests/test_cleanup.pytests/test_engine.pytests/test_engine_worktree.pytests/test_recovery_flow.pytests/test_verify.pytests/test_verify_worktree.pytests/test_worktree_flow.py
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge — the change is a well-scoped translation at the sole git spawn point, all downstream guards hold as-is, and four ablation passes verified each new arm. The chokepoint translation is minimal and mirrors the existing TimeoutExpired handler exactly. Exception ordering in run_isolated is correct (subclass caught before base class). The merge_local isinstance branch correctly separates environment faults from git-refusing faults. All 3399 tests pass, Pyright reports 0 errors, and every new code path has a corresponding ablation-verified test. No pre-existing guard is narrowed. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/verify.py | Adds GitSpawnError(GitError) and translates spawn-level OSError at _run_git; mirrors the existing TimeoutExpired handler; cmd[3] access is safe for all call sites. |
| src/bmad_loop/worktree_flow.py | Adds GitSpawnError arm before GitError arm in run_isolated (correct subclass ordering); widens merge_local guard to (GitError, OSError) and branches error message on isinstance(e, (GitSpawnError, OSError)); logic is correct. |
| src/bmad_loop/engine.py | Comment-only update to _finalize_commit_phase's BaseException arm; correctly documents that spawn faults now arrive as GitSpawnError and take the GitError arm above. |
| src/bmad_loop/recovery_flow.py | Docstring-only corrections updating comments on (GitError, OSError) guards to reflect typed spawn faults while preserving the TemporaryDirectory ENOSPC net. |
| tests/test_verify.py | Parametrized unit test (ENOENT/EMFILE/ENOMEM) verifying subprocess.run OSError to GitSpawnError with isinstance(GitError) and cause chain; acceptance test for verify_dev escalation path. |
| tests/test_worktree_flow.py | New test verifies spawn fault in run_isolated pauses rather than defers; ablation target documented; phase and journal-entry assertions confirm the per-unit GitError path is unchanged. |
| tests/test_engine_worktree.py | Parametrized test (fs-oserror / git-spawn) covering merge_local's widened guard; verifies keep-branch escalation, correct error message branch, and no crash on DONE unit. |
| tests/test_engine.py | New commit-window spawn-fault test confirms ledger restored and escalated (not crashed); existing non-spawn OSError crash test updated in comment to reflect its now-narrower premise. |
| tests/test_cleanup.py | New test confirms reconcile_orphan_worktrees degrades to [] on spawn fault injected at subprocess.run; verifies the full translation chain end-to-end. |
| tests/test_recovery_flow.py | Comment-only updates; wording corrected to reflect spawn faults now arrive typed as GitSpawnError, but the OSError net for TemporaryDirectory faults remains load-bearing. |
| tests/test_verify_worktree.py | Comment-only update; rationale corrected to note GitSpawnError is now typed but the plain-OSError net in worktree_prune stays as belt-and-suspenders. |
| CHANGELOG.md | Accurate changelog entry documenting the spawn-fault translation, run-pause behavior change for worktree open, and corrected merge-reconciliation error message. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[subprocess.run] -->|success| B[CompletedProcess]
A -->|TimeoutExpired| C[raise GitError]
A -->|OSError EMFILE/ENOMEM/ENOENT| D[raise GitSpawnError - GitError - __cause__ = original OSError]
D --> E{Caller site}
C --> E
E -->|run_isolated| F{GitSpawnError?}
F -->|Yes - machine-wide| G[_pause RunPaused not DEFERRED]
F -->|No - plain GitError| H[defer unit]
E -->|merge_local| I{GitSpawnError or OSError?}
I -->|Yes - env fault| J[could not reconcile - keep_branch_and_escalate]
I -->|No - plain GitError| K[uncommitted changes - keep_branch_and_escalate]
E -->|_finalize_commit_phase| L{GitError?}
L -->|Yes incl GitSpawnError| M[restore ledger _escalate]
L -->|No BaseException| N[restore ledger re-raise crash]
E -->|15 observation guards| O[degrade no-op]
Reviews (2): Last reviewed commit: "fix(worktree): neutral merge-blocked mes..." | Re-trigger Greptile
1 participant
Summary
Closes #343 with the chokepoint shape:
_run_gitnow translates a spawn-levelOSError(EMFILE/ENOMEM, git gone from PATH) intoGitSpawnError(GitError)the same way it already translatesTimeoutExpired, so all 19 OSError-blindexcept GitErrorguards hold as written with no per-site edits.GitSpawnErrorfollows thePrunePreserveErrorprecedent; the errno stays reachable on__cause__.Site-level dispositions that change
engine._finalize_commit_phase) — a spawn fault now takes theGitErrorarm: restore the deferred-close ledger + escalate, same as a git timeout there. TheBaseExceptionarm's comment named the escaping spawn OSError as a case it restored against; its purpose was ledger protection, and translation completes what it compensated for. Non-spawn OSErrors still restore + re-raise + crash (existing test retained, premise reworded).worktree_flow.run_isolated) — a spawn fault is machine-wide, not per-unit: it now pauses the run instead of deferring every unit one notification at a time and ending the run "finished" over a broken environment (the Dev session lost to an API/transport failure is charged as a story timeout, exhausting the attempt budget #194 env-fault doctrine). Per-unitGitErrors still defer.worktree_flow.merge_local) — widened to(GitError, OSError):clean_incoming_collisionsmutates the checkout directly (unlink/rmdir), a non-spawn FS class the chokepoint cannot translate, and crashing there strands a DONE unit mid-merge past the keep-branch escalation the guard exists for. Precedent: therestore_patchwidening in fix(recovery): refuse the rollback reset when the snapshot fails (#340) #341.The other 15 blind sites need no edits — verified each degrades/escalates/reports correctly once the exception arrives typed. No existing
(GitError, OSError)guard is narrowed:snapshot_worktree'sTemporaryDirectoryand the patch-file reads remain genuine non-spawn OSError sources (docstrings corrected accordingly, includingsnapshot_worktree's false "before any git child is spawned" claim).Testing
subprocess.runOSError →GitSpawnError,isinstanceGitError,__cause__chained.subprocess.runper the issue's acceptance: observation-degrades (reconcile_orphan_worktrees→[]) and escalate (verify_dev→ CRITICAL, not retryable).uv run pytest -q -n auto: 3399 passed.trunk check: clean.uvx pyright@1.1.411: 0 errors.Summary by CodeRabbit