fix: degrade spec read faults instead of crashing the run

CHANGELOG.md

@@ -46,6 +46,17 @@ breaking changes may land in a minor release.
 
 ### Fixed
 
+- **An unreadable spec no longer crashes the whole run.** Every spec read-back — the four verify
+  gates, the reconcile/sprint/ledger bookkeeping passes, and the generic adapter's Stop poll — raced
+  the dev skill's own writes, so a transient `OSError` (a TOCTOU truncation, a lock, an EACCES)
+  escaped to `engine.run()` and abandoned every remaining story. Observation now degrades where
+  repair still raises: verify gates return a retryable outcome naming the read fault (never a phantom
+  status mismatch), bookkeeping passes skip and journal `spec-read-failed`, and the read-back poll
+  treats it as not-yet-terminal, falling through to the existing stall/timeout → post-kill-reconcile
+  ladder. Review routing re-derives `followup_review_recommended` from the finalized spec when a
+  replayed result lacks it, so a fault that skips the reconcile re-fold can no longer silently skip
+  a recommended follow-up review on resume (#97).
+
 - **A resumed sweep re-drives its in-flight bundles by identity, not by bundle name.** `SweepEngine`
   recovered a bundle only from inside `_run_bundle`, which a cycle reaches after re-deriving the key
   from the _current_ triage plan — so a bundle re-armed by `bmad-loop resolve` survived a resume only

src/bmad_loop/devcontract.py

@@ -224,7 +224,17 @@ def synthesize_result(
     so a plan-halt ``ready-for-dev`` (no such prose) is never reconciled to
     ``done`` and this leg's success outcome is not clobbered.
     """
-    fm = read_frontmatter(spec_path)
+    try:
+        fm = read_frontmatter(spec_path)
+    except OSError:
+        # Same degrade as `_read_text_or_empty` below, for the same reason: this is
+        # the read-back path. An unreadable spec is not evidence a session
+        # finished, so treat it exactly like one that has not terminated yet — the
+        # caller keeps polling, and a fault that persists past the grace window
+        # lands as a stall/timeout verdict that `_post_kill_reconcile` can still
+        # rescue. Crashing here would take the whole run down for a spec the CLI
+        # merely had open for writing.
+        return SynthResult(result_json=None, status_consistent=True)
     fm_status = str(fm.get("status", "")).strip().lower()
     arr = parse_auto_run_result(_read_text_or_empty(spec_path))
 

src/bmad_loop/engine.py

@@ -1483,9 +1483,16 @@ def _dev_phase(self, task: StoryTask, resume_result: SessionResult | None = None
                 self._post_dev_state_sync(task, result.result_json)
                 # carry the skill's follow-up-review recommendation (PR #2505)
                 # onto the task so _review_and_commit can gate the review loop.
-                task.followup_review_recommended = bool(
-                    (result.result_json or {}).get("followup_review_recommended", False)
-                )
+                # A present key is authoritative (folded from the frontmatter, or
+                # the legacy skill's own result.json); an absent one is a resumed
+                # pre-reconcile snapshot whose re-fold may have been dropped by a
+                # spec read fault — re-derive from the spec instead of defaulting
+                # a recommended review away.
+                rj = result.result_json or {}
+                if "followup_review_recommended" in rj:
+                    task.followup_review_recommended = bool(rj["followup_review_recommended"])
+                else:
+                    task.followup_review_recommended = self._followup_from_spec(task, rj)
                 outcome = self._verify_dev_artifacts(task, result.result_json)
                 if outcome.ok and self._run_verify_commands_after_dev(task, result.result_json):
                     # deterministic gates run here too: a broken build must not
@@ -1797,6 +1804,72 @@ def _dev_review_enabled(self) -> bool:
             return False
         return self.policy.review.enabled
 
+    def _observed_frontmatter(self, spec_path: Path, story_key: str, site: str) -> dict | None:
+        """Read a spec's frontmatter on a *bookkeeping* path, degrading an
+        unreadable spec to ``None`` (journaled) instead of a whole-run crash.
+
+        These reads observe what the dev skill left behind so the orchestrator can
+        sync the sprint board / ledger. They race the skill's own writes, so an
+        OSError is a designed transient, not a broken orchestrator. Returning None
+        tells the caller to skip its bookkeeping pass entirely: skipping is safe
+        because everything a skipped pass would have derived is re-supplied later —
+        the spec *status* by the deterministic verify gate's own re-read (which
+        turns a still-unrepaired spec into a retry), and the review-routing flag by
+        ``_followup_from_spec`` at the point ``_dev_phase`` consumes it. Silent it
+        is not — every skip lands a ``spec-read-failed`` event in the journal.
+
+        Repair writes (``reset_spec_status``, ``mark_done``) deliberately do the
+        opposite and let OSError raise: silently skipping a rewrite would leave the
+        spec in a state the caller believes it fixed. Observation degrades, repair
+        raises.
+        """
+        try:
+            return verify.read_frontmatter(spec_path)
+        except OSError as e:
+            self._journal_spec_read_failed(spec_path, story_key, site, e)
+            return None
+
+    def _journal_spec_read_failed(
+        self, spec_path: Path, story_key: str, site: str, e: OSError
+    ) -> None:
+        self.journal.append(
+            "spec-read-failed",
+            story_key=story_key,
+            spec=str(spec_path),
+            site=site,
+            error=f"{e.__class__.__name__}: {e}",
+        )
+
+    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))
+
     def _reconcile_generic_terminal_status(self, task: StoryTask, result_json: dict | None) -> None:
         """Repair a generic-skill spec the session finalized in prose but not in
         frontmatter. ``bmad-dev-auto`` sometimes appends a terminal
@@ -1838,7 +1911,9 @@ def _reconcile_generic_terminal_status(self, task: StoryTask, result_json: dict
         # "none" through verify.status_of (str(None)), which would dodge the
         # RECONCILABLE_FROM allowlist; normalize it (and a missing key) to "" so the
         # blank-status case reconciles. A literal `status: none` stays "none".
-        fm = verify.read_frontmatter(spec_path)
+        fm = self._observed_frontmatter(spec_path, task.story_key, "reconcile")
+        if fm is None:
+            return
         raw_status = fm.get("status")
         fm_status = "" if raw_status is None else str(raw_status).strip().lower()
         if fm_status == success_status:
@@ -1863,18 +1938,26 @@ def _reconcile_generic_terminal_status(self, task: StoryTask, result_json: dict
             return
         if fm_status not in devcontract.RECONCILABLE_FROM:
             return  # blocked / unknown custom status: never override a deliberate one
-        arr = devcontract.parse_auto_run_result(spec_path.read_text(encoding="utf-8"))
+        try:
+            text = spec_path.read_text(encoding="utf-8")
+        except OSError as e:
+            self._journal_spec_read_failed(spec_path, task.story_key, "reconcile-prose", e)
+            return
+        arr = devcontract.parse_auto_run_result(text)
         if not arr.present or arr.status != devcontract.DONE:
             return  # no terminal prose, or a blocked outcome: leave for the escalation path
         if not devcontract.reset_spec_status(spec_path, success_status):
             return
         # Keep the in-place result_json the rest of _dev_phase reads consistent with
         # the now-reconciled spec (the followup flag is only carried on a done exit).
+        # `reset_spec_status` rewrites only the status line, so `fm` (read above)
+        # still holds every other key — a re-read here could only return the same
+        # followup flag, at the cost of a second racy read that can now fail.
         if isinstance(result_json, dict):
             result_json["status"] = success_status
             if success_status == "done":
                 result_json["followup_review_recommended"] = bool(
-                    verify.read_frontmatter(spec_path).get("followup_review_recommended", False)
+                    fm.get("followup_review_recommended", False)
                 )
         self.journal.append(
             "spec-status-reconciled",
@@ -1906,8 +1989,10 @@ def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> Non
             return
         review_enabled = self._dev_review_enabled()  # always False for the generic path
         success_status = "in-review" if review_enabled else "done"
-        status = verify.status_of(verify.read_frontmatter(spec_path))
-        if status != success_status:
+        fm = self._observed_frontmatter(spec_path, task.story_key, "post-dev-sync")
+        if fm is None:
+            return
+        if verify.status_of(fm) != success_status:
             return
         target = "review" if review_enabled else "done"
         sprint_advance(self.workspace.paths.sprint_status, task.story_key, target)

src/bmad_loop/sweep.py

@@ -1174,7 +1174,10 @@ def _close_bundle_ledger_when_spec_status(
         spec_path = verify.resolve_spec_path(spec_file, self.workspace.paths)
         if not spec_path.is_file():
             return
-        if verify.status_of(verify.read_frontmatter(spec_path)) != success_status:
+        fm = self._observed_frontmatter(spec_path, task.story_key, "bundle-ledger-close")
+        if fm is None:
+            return
+        if verify.status_of(fm) != success_status:
             return
         ledger = self.workspace.paths.deferred_work
         note = f"resolved by sweep bundle {task.story_key}"

src/bmad_loop/verify.py

@@ -1088,6 +1088,29 @@ def resolve_spec_path(spec_file: str, paths: ProjectPaths) -> Path:
     return paths.implementation_artifacts / p
 
 
+def _gate_frontmatter(spec_path: Path) -> dict[str, Any] | VerifyOutcome:
+    """Read a spec's frontmatter for a verify gate, degrading an unreadable spec
+    to a retryable :class:`VerifyOutcome` instead of a whole-run crash.
+
+    Every verify gate reads the spec back while the dev skill may still be
+    rewriting it, so an OSError here (a TOCTOU truncation, a transient lock, a
+    momentarily unsearchable parent) is a fault with a *designed* transient
+    producer — not a broken orchestrator. `read_frontmatter` itself keeps raising
+    (repair callers depend on that); only these observation gates degrade.
+
+    The reason is deliberately distinct from a status mismatch: returning ``{}``
+    here 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. Retries are not silent — the reason lands in the journal via
+    `dev-decision` / `review-verify-failed`, and a persistent fault is bounded
+    into DEFER (or PAUSE) by `escalation.decide_dev` / `decide_review_session`.
+    """
+    try:
+        return read_frontmatter(spec_path)
+    except OSError as e:
+        return VerifyOutcome.retry(f"spec unreadable ({e.__class__.__name__}: {e}): {spec_path}")
+
+
 def _verify_shared_gates(
     spec_path: Path,
     rj: dict[str, Any],
@@ -1118,7 +1141,9 @@ def _verify_shared_gates(
             f"dev result.json workflow is {workflow!r}, expected {DEV_WORKFLOW!r}"
         )
 
-    fm = read_frontmatter(spec_path)
+    fm = _gate_frontmatter(spec_path)
+    if isinstance(fm, VerifyOutcome):
+        return fm
     status = status_of(fm)
     if status != expected_status:
         return VerifyOutcome.retry(
@@ -1405,7 +1430,9 @@ def verify_commands_outcome(policy: Policy, cwd: Path) -> VerifyOutcome:
 def verify_review(task: StoryTask, paths: ProjectPaths, policy: Policy) -> VerifyOutcome:
     if not task.spec_file:
         return VerifyOutcome.retry("no spec file recorded for task")
-    fm = read_frontmatter(Path(task.spec_file))
+    fm = _gate_frontmatter(Path(task.spec_file))
+    if isinstance(fm, VerifyOutcome):
+        return fm
     status = status_of(fm)
     if status != "done":
         return VerifyOutcome.retry(f"spec status is {status!r}, expected 'done'")
@@ -1426,7 +1453,10 @@ def verify_review_stories(task: StoryTask, paths: ProjectPaths, policy: Policy)
     id-keyed story spec ``verify_dev_stories`` recorded on the dev pass."""
     if not task.spec_file:
         return VerifyOutcome.retry("no spec file recorded for task")
-    status = status_of(read_frontmatter(Path(task.spec_file)))
+    fm = _gate_frontmatter(Path(task.spec_file))
+    if isinstance(fm, VerifyOutcome):
+        return fm
+    status = status_of(fm)
     if status != "done":
         return VerifyOutcome.retry(f"spec status is {status!r}, expected 'done'")
     return verify_commands_outcome(policy, paths.project)
@@ -1441,13 +1471,22 @@ def verify_review_bundle(task: StoryTask, paths: ProjectPaths, policy: Policy) -
     can trust it happened."""
     if not task.spec_file:
         return VerifyOutcome.retry("no spec file recorded for task")
-    fm = read_frontmatter(Path(task.spec_file))
+    fm = _gate_frontmatter(Path(task.spec_file))
+    if isinstance(fm, VerifyOutcome):
+        return fm
     status = status_of(fm)
     if status != "done":
         return VerifyOutcome.retry(f"spec status is {status!r}, expected 'done'")
 
     ledger = paths.deferred_work
-    text = ledger.read_text(encoding="utf-8") if ledger.is_file() else ""
+    # Same TOCTOU class as the spec read above: the ledger is rewritten by the
+    # orchestrator's own mark_done between the dev and review gates.
+    try:
+        text = ledger.read_text(encoding="utf-8") if ledger.is_file() else ""
+    except OSError as exc:
+        return VerifyOutcome.retry(
+            f"deferred-work ledger unreadable ({exc.__class__.__name__}: {exc}): {ledger}"
+        )
     entries = {e.id: e for e in deferredwork.parse_ledger(text)}
     not_done = sorted(
         i for i in task.dw_ids if i not in entries or not entries[i].status.startswith("done")

tests/conftest.py

@@ -205,6 +205,22 @@ def install_base_skills(paths: ProjectPaths, trees=(".claude/skills", ".agents/s
         _write_skill_stubs(paths.project / tree, BASE_SKILLS)
 
 
+def fault_read_text(monkeypatch, target: Path) -> None:
+    """Make exactly ``target``'s ``read_text`` raise PermissionError; every other
+    path still reads normally. A selective monkeypatch rather than chmod: 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_bytes`` is untouched, so a test can still
+    assert the faulted file's contents are unchanged."""
+    real = Path.read_text
+
+    def fake(self, *a, **kw):
+        if self == target:
+            raise PermissionError(13, "Permission denied")
+        return real(self, *a, **kw)
+
+    monkeypatch.setattr(Path, "read_text", fake)
+
+
 def write_sprint(paths: ProjectPaths, statuses: dict[str, str]) -> None:
     doc = dict(SPRINT_TEMPLATE)
     doc["development_status"] = dict(statuses)

tests/test_devcontract.py

@@ -377,6 +377,29 @@ def test_synthesize_result_non_utf8_fallback_marker_is_not_terminal(tmp_path):
     assert sr.status_consistent is True
 
 
+def test_synthesize_result_oserror_is_not_terminal(tmp_path, monkeypatch):
+    """An unreadable spec is not evidence a session finished, so it reads exactly
+    like one that has not terminated yet — no result_json, no crash. The caller
+    keeps polling; a fault that outlives the grace window becomes a stall/timeout
+    verdict that `_post_kill_reconcile` can still rescue. Before this, an OSError
+    here took the whole run down (engine.run()'s `except Exception` → crash.txt).
+
+    `devcontract` binds `read_frontmatter` by ``from .verify import``, so the name
+    to patch is `devcontract.read_frontmatter` — patching `verify.read_frontmatter`
+    would leave this module's already-bound reference untouched and the test would
+    pass for the wrong reason."""
+    spec = tmp_path / "spec-1-1-a.md"
+    spec.write_text("---\nstatus: done\n---\n\n## Auto Run Result\n\n- Status: done\n")
+
+    def boom(_path):
+        raise PermissionError(13, "Permission denied")
+
+    monkeypatch.setattr(devcontract, "read_frontmatter", boom)
+    sr = devcontract.synthesize_result(spec, story_key="1-1")
+    assert sr.result_json is None
+    assert sr.status_consistent is True
+
+
 # ----------------------------------------------------------- reset_spec_status