fix: degrade spec read faults instead of crashing the run#113
Merged
Conversation
Nine spec read-back sites raced the dev skill's own writes, so a transient
OSError escaped to engine.run()'s `except Exception` and marked the whole
run crashed. Route them through three chokepoints, per the doctrine
`devcontract._read_text_or_empty` already states: observation degrades,
repair raises.
- verify._gate_frontmatter: the four verify gates return a retryable outcome
naming the read fault. Never `{}` -> status "" -- a fault must not
masquerade as a status mismatch and send a repair session after a bug that
is not there. The adjacent deferred-work ledger read degrades the same way.
- Engine._observed_frontmatter: the reconcile, post-dev-sync and
bundle-ledger-close bookkeeping passes skip and journal a new
`spec-read-failed` event (story_key, spec, site, error). Skipping is safe:
the deterministic verify gate re-reads the spec right after and supplies
the retry ladder.
- devcontract.synthesize_result: an unreadable spec reads as not-yet-terminal,
so the read-back poll keeps waiting and a persistent fault lands as a
stall/timeout verdict that _post_kill_reconcile can still rescue. One
chokepoint covers the unwrapped scan path and the UnicodeDecodeError-only
stories path.
Also deletes the frontmatter re-read after `reset_spec_status` (it rewrites
only the status line, so the re-read could only return the followup flag the
first read already carried -- at the cost of a second racy read).
`read_frontmatter` itself still raises on OSError, now pinned by a test:
repair writes depend on it, since silently skipping a rewrite leaves the spec
in a state the caller believes it fixed.
Closes #97
WalkthroughUnreadable spec and ledger reads now degrade to retryable or non-terminal outcomes. Engine paths journal failures and skip unsafe reconciliation or synchronization, while replay routing can derive follow-up review recommendations from finalized frontmatter. Tests cover verification, polling, reconciliation, replay, and bundle-ledger behavior. ChangesRead fault resilience
Sequence Diagram(s)sequenceDiagram
participant Engine
participant Verify
participant Spec
participant Journal
participant Review
Engine->>Spec: observe frontmatter
Spec-->>Engine: frontmatter or read failure
Engine->>Journal: record spec-read-failed
Engine->>Verify: run retryable gate
Verify-->>Engine: retryable outcome
Engine->>Review: dispatch follow-up when finalized spec recommends it
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…e followup key Final-review finding on this PR: the _observed_frontmatter early return has one consumer the verify gate does not re-supply. A resumed dev result is the pre-reconcile snapshot (synthesize_result only writes followup_review_recommended on a done synth), and the reconcile already-finalized branch's re-fold is the sole carrier of the flag back onto the replay. A transient read fault dropped that fold silently; the gate then passed on status alone and trigger=recommended skipped the recommended follow-up review. The live path was never exposed — there the repair also didn't run, so the status gate fails and the retry re-drives reconcile. The post-dev assignment now treats a present key as authoritative and, only when absent, re-derives from the finalized spec frontmatter via _followup_from_spec (through _observed_frontmatter, site followup-routing): a persistent fault degrades to the pre-existing absent-key False, journaled, and the verify gate's own faulted read still bounds the story into DEFER. Gated on the frontmatter's status, not rj["status"] — a faulted replay leaves the result status at the stale snapshot value. Both new tests mutate-verified: pre-fix the transient-fault replay commits with no review session at all ([] == ['review']); the persistent-fault companion pins the bounded degrade (DEFERRED, sites reconcile + followup-routing, never a crash).
![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/engine.py`:
- Around line 1843-1871: Guard the spec-file existence check in
_followup_from_spec against filesystem errors: wrap spec_path.is_file() in
try/except OSError and return False when the check raises, matching the handling
of other unreadable-spec paths.
🪄 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: 2dc9bad5-ad93-4092-817a-c90564189b20
📒 Files selected for processing (11)
CHANGELOG.mdsrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/sweep.pysrc/bmad_loop/verify.pytests/conftest.pytests/test_devcontract.pytests/test_engine.pytests/test_generic_tmux.pytests/test_sweep.pytests/test_verify.py
Comment on lines
+1843
to
+1871
| def _followup_from_spec(self, task: StoryTask, rj: dict) -> bool: | ||
| """Review-routing fallback for a result that carries no | ||
| ``followup_review_recommended`` key: re-derive it from the finalized spec | ||
| frontmatter — the source ``devcontract.synthesize_result`` and the | ||
| reconcile folds read it from, so a readable spec can never disagree. | ||
|
|
||
| The key is absent exactly when the result is a *resumed* pre-reconcile | ||
| snapshot (``synthesize_result`` only writes it on a ``done`` synth, and the | ||
| durable record is persisted before reconcile mutates the live dict). The | ||
| reconcile re-fold normally restores it on replay, but a spec read fault | ||
| skips that fold — and the verify gate only re-supplies *status*, not this | ||
| flag — so without this fallback a recommended follow-up review would be | ||
| silently skipped. Gates on the frontmatter's own status (mirroring the | ||
| fold): a faulted replay leaves ``rj["status"]`` at the stale snapshot | ||
| value, so the result status must not decide. Degrades to False on a read | ||
| fault (journaled) — the pre-existing absent-key default. | ||
| """ | ||
| if not self._generic_dev(): | ||
| return False | ||
| spec_file = rj.get("spec_file") | ||
| if not spec_file: | ||
| return False | ||
| spec_path = verify.resolve_spec_path(str(spec_file), self.workspace.paths) | ||
| if not spec_path.is_file(): | ||
| return False | ||
| fm = self._observed_frontmatter(spec_path, task.story_key, "followup-routing") | ||
| if fm is None: | ||
| return False | ||
| return verify.status_of(fm) == "done" and bool(fm.get("followup_review_recommended", False)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Python 3.14 change pathlib Path.is_file() to no longer raise PermissionError?
💡 Result:
Yes, in Python 3.14, pathlib.Path.is_file and related methods have been changed to suppress OSError exceptions, including PermissionError [1][2][3]. In previous versions, while some exceptions were already suppressed, others like PermissionError when accessing directories without sufficient read permissions would propagate [2][3][4]. As of Python 3.14, the method now returns False if the path is invalid, inaccessible, or missing, providing consistent behavior across these conditions [1][2][5]. The official documentation explicitly notes that any OSError exceptions raised from scanning the filesystem are suppressed, which includes PermissionError [1][2]. If you need to distinguish between these different states (e.g., a file not existing versus lacking permission to check it), you are advised to use Path.stat instead, as it does not suppress such exceptions [1][2][3].
Citations:
- 1: https://docs.python.org/3/library/pathlib.html
- 2: https://github.com/python/cpython/blob/main/Doc/library/pathlib.rst
- 3: hugovk/cpython@fbe6a09
- 4: https://fossies.org/linux/Python/Doc/library/pathlib.rst
- 5: https://docs.w3cub.com/python~3.14/library/pathlib
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate version files =="
git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|noxfile\.py|requirements.*\.txt|Pipfile|poetry\.lock|\.github/workflows/.*\.ya?ml)$' || true
echo
echo "== python version / requires-python references =="
rg -n --hidden --no-messages -S \
'(requires-python|python-version|python\s*>=|python3\.[0-9]+|matrix:.*python|setup-python|actions/setup-python|Programming Language :: Python)' \
pyproject.toml setup.cfg setup.py tox.ini noxfile.py .github/workflows . 2>/dev/null || trueRepository: bmad-code-org/bmad-loop
Length of output: 1927
Guard spec_path.is_file() against OSError. This repo supports Python 3.11–3.13, where Path.is_file() can still raise on unreadable paths, so this fallback can crash instead of returning False. Wrap it in try/except OSError like the other unreadable-spec paths.
🤖 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/engine.py` around lines 1843 - 1871, Guard the spec-file
existence check in _followup_from_spec against filesystem errors: wrap
spec_path.is_file() in try/except OSError and return False when the check
raises, matching the handling of other unreadable-spec paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #97
Problem
Nine spec read-back sites raced the dev skill's own writes. A transient
OSErrorat any of them — a TOCTOU truncation whilebmad-dev-autorewrites the spec, a lock, an EACCES — escaped toengine.run()'sexcept Exception, wrotecrash.txt, and abandoned every remaining story. All nine are observation reads, so the crash was never proportionate to the fault.The doctrine already exists in the codebase, stated at
devcontract._read_text_or_empty: observation degrades, repair raises. These sites just never followed it.Change (9 sites → 3 chokepoints + 1 deletion)
verify.py)verify._gate_frontmatter→VerifyOutcome.retry("spec unreadable (…)")verify_review_bundle)except OSError→ retry (same TOCTOU class — the orchestrator's ownmark_donerewrites it between the dev and review gates)Engine._observed_frontmatter→ journal new eventspec-read-failed(story_key,spec,site,error) + skip the bookkeeping passreset_spec_statusdevcontract.synthesize_result(Stop read-back)except OSError→SynthResult(None, True)(not-terminal)Two details worth review:
{}would read as status""and let a read fault masquerade as "the skill forgot to set the status" — sending a repair session after a bug that is not there. The reason names the fault instead._followup_from_specat consumption (see the final-review section below — the original "always safe via the verify gate" claim had one hole). Repair writes (reset_spec_status,set_frontmatter_status,mark_done) still raise, andread_frontmatter's own contract is unchanged — now pinned bytest_read_frontmatter_oserror_still_raises, since widening itsexceptwould erase the observe/repair distinction everywhere at once.The deletion:
reset_spec_statusrewrites only the frontmatter status line, so the re-read after it could only ever return thefollowup_review_recommendedthe first read already carried — at the cost of a second racy read that can now fail. Folded from the existingfm;test_reconcile_folds_followup_without_rereadcounts the reads.Retry, not escalate
VerifyOutcome.escalateat the verify gates would be dead code: the review-loop consumers ignoreseverity. TheGitError → escalateprecedent beside it is about broken verify machinery, not a read with a designed transient producer.Retry is not silent. Reasons journal via
dev-decision/review-verify-failed;escalation.decide_dev/decide_review_sessionbound a persistent fault into DEFER (or PAUSE whenresolved_redriveis latched); and under the defaultscm.rollback_on_failure = Falsea dirty-tree dev retry already routes through_pause_for_manual_recovery— a loud pause. Thedevcontractdegrade feeds the designed #95/#61 ladder: grace-bounded poll → stall/timeout verdict →_post_kill_reconcilerescue.Final review: the replay-fold hole
Adversarial final review found one path where "the verify gate re-supplies it" was false — the gate re-supplies status, not review routing.
A resumed dev result is the pre-reconcile snapshot (
synthesize_resultonly writesfollowup_review_recommendedon adonesynth; the durable record persists before reconcile mutates the live dict). On replay, the reconcile already-finalized branch's re-fold is the sole carrier of the flag back onto the result — and this PR'sfm is Noneearly return dropped it on a read fault. If the fault cleared before the verify gate (which then passes on status alone),task.followup_review_recommendeddefaulted False andtrigger = "recommended"silently skipped the recommended follow-up review. The live path was never exposed: there the repair also didn't run, so the status gate fails and the retry re-drives reconcile.Fix, in this PR: the post-dev assignment in
_dev_phasetreats a present key as authoritative and, only when absent, re-derives from the finalized spec frontmatter via newEngine._followup_from_spec(through_observed_frontmatter, sitefollowup-routing— so a persistent fault degrades to the pre-existing absent-key False, journaled, and the verify gate's own faulted read still bounds the story into DEFER). Gates on the frontmatter's status, notrj["status"]— a faulted replay leaves the result status at the stale snapshot value.Tests
13 new tests (15 items). Every one was mutate-verified — checked to fail against the unpatched source, not merely to pass against the patched one:
test_verify.py— dev gate, all three review gates (parametrized), the ledger read, plus theread_frontmatterstill-raises pin.test_devcontract.py, 1 intest_generic_tmux.py— the read-back poll, which is where the issue's headline crash lived (that path guarded onlyUnicodeDecodeError).test_engine.py/test_sweep.py— each bookkeeping site skips, journals the rightsite, and leaves its artifact byte-identical.test_transient_spec_read_fault_does_not_crash_run: raise-once-then-delegate wrapper on the spec. Pre-fix it reproduces the issue exactly —RunSummary(done=0, crashed=True, crash_error='PermissionError: [Errno 13] Permission denied'). Post-fix the run completes, story DONE, exactly onespec-read-failedjournaled.test_engine.py(from the final-review finding): a shared_crash_replay_setupkills the host in the post-session window with the pre-reconcile snapshot durably recorded and the spec already repaired on disk.test_resume_replay_fault_still_routes_recommended_review— transient fault at the resumed reconcile; pre-fix the story committed with no review session at all ([] == ['review']), post-fix the review runs and the fault costs exactly onespec-read-failed.test_resume_replay_persistent_fault_degrades_and_defers— the fault outlives the fallback: routing degrades to False at sitefollowup-routing, the verify gate's faulted read retries, and the attempt budget lands DEFERRED. Never a crash, never a phantom review.Fault injection is a selective
Path.read_textmonkeypatch (conftest.fault_read_text), notchmod: chmod is a no-op for root and carries no read bit on Windows, so the fault would silently not fire on half the CI matrix.read_bytesis left untouched so a test can still assert the faulted file is unchanged.Full suite green (2045 passed, 1 skipped); full
trunk checkclean.Deliberately out of scope
is_file()calls atengine.py:1825/:1905,sweep.py:1050,verify.py:1178.Path.is_file()itself raises EACCES on an unsearchable parent — but that is a persistent config fault, not a transient, and the loud crash is arguably the correct outcome. Left alone rather than swept in silently.escalateinstead ofretryat the verify gates — considered and not adopted, for the reason above. It would need consumer changes to be anything other than dead code.stories.py:328's guard,generic.py:539/:607's guards, and every repair write are untouched.Windows CI caught one test bug on the first push:
Path.write_textnewline-translates\n→\r\n, so assertingread_bytes() == original.encode()compared LF against CRLF. Fixed by snapshotting the on-disk bytes after the write — which is also the truer statement of "these bytes never changed". All 8 checks green.Summary by CodeRabbit
Bug Fixes
Documentation