diff --git a/CHANGELOG.md b/CHANGELOG.md index 92bef22..7ba57fc 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index c537ecd..25e9e01 100644 --- a/src/bmad_loop/devcontract.py +++ b/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)) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 342717a..16edb2e 100644 --- a/src/bmad_loop/engine.py +++ b/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) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 02b74c3..79006b0 100644 --- a/src/bmad_loop/sweep.py +++ b/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}" diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 16c7228..faf2264 100644 --- a/src/bmad_loop/verify.py +++ b/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") diff --git a/tests/conftest.py b/tests/conftest.py index c7a9dd9..ae12aed 100644 --- a/tests/conftest.py +++ b/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) diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index 4ba2f31..fbe2fbd 100644 --- a/tests/test_devcontract.py +++ b/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 diff --git a/tests/test_engine.py b/tests/test_engine.py index dde75d9..fb54fb2 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -9,6 +9,7 @@ from conftest import ( _file_exists_cmd, dev_effect, + fault_read_text, generic_dev_effect, git, review_effect, @@ -17,7 +18,7 @@ write_sprint, ) -from bmad_loop import platform_util +from bmad_loop import platform_util, verify from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter from bmad_loop.engine import Engine, RunPaused, RunStopped @@ -1079,6 +1080,209 @@ def test_generic_reconcile_skips_out_of_tree_spec(project, tmp_path): assert "spec-status-reconciled" not in kinds # no reconcile happened +def test_reconcile_skips_and_journals_on_unreadable_spec(project, monkeypatch): + """Reconcile is a bookkeeping *observation* pass over a spec the dev skill may + still be writing. An OSError there used to crash the whole run; it now skips the + pass and journals `spec-read-failed`. Skipping is safe: the deterministic verify + gate re-reads the spec straight after and supplies the retry ladder.""" + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + original = ( + "---\ntitle: 'x'\nstatus: 'in-progress'\n---\n\n## Auto Run Result\n\n- Status: done\n" + ) + sp.write_text(original, encoding="utf-8") + before = sp.read_bytes() # snapshot: text-mode write newline-translates on Windows + task = StoryTask(story_key="1-1-a", epic=1) + rj = {"workflow": "auto-dev", "spec_file": str(sp), "status": "in-progress"} + fault_read_text(monkeypatch, sp) + + engine._reconcile_generic_terminal_status(task, rj) + + assert sp.read_bytes() == before # never written (repair skipped) + assert rj["status"] == "in-progress" # result dict untouched + events = [e for e in engine.journal.entries() if e["kind"] == "spec-read-failed"] + assert len(events) == 1 + assert events[0]["site"] == "reconcile" + assert events[0]["story_key"] == "1-1-a" and events[0]["spec"] == str(sp) + assert "PermissionError" in events[0]["error"] + assert "spec-status-reconciled" not in [e["kind"] for e in engine.journal.entries()] + + +def test_reconcile_folds_followup_without_reread(project, monkeypatch): + """`reset_spec_status` rewrites only the frontmatter status line, so a re-read + after it could only return the followup flag the first read already carried — + at the cost of a second racy read that can now fail. Exactly one frontmatter + read, and the flag still folds into the live result dict.""" + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text( + "---\ntitle: 'x'\nstatus: 'in-progress'\nfollowup_review_recommended: true\n---\n\n" + "## Auto Run Result\n\n- Status: done\n", + encoding="utf-8", + ) + calls, real = [], verify.read_frontmatter + + def counting(path): + calls.append(path) + return real(path) + + monkeypatch.setattr(verify, "read_frontmatter", counting) + task = StoryTask(story_key="1-1-a", epic=1) + rj = {"workflow": "auto-dev", "spec_file": str(sp), "status": "in-progress"} + engine._reconcile_generic_terminal_status(task, rj) + + assert len(calls) == 1 # the deleted re-read stays deleted + assert rj["status"] == "done" + assert rj["followup_review_recommended"] is True # folded from the single read + assert verify.status_of(real(sp)) == "done" # the repair write still happened + + +def test_post_dev_state_sync_skips_on_unreadable_spec(project, monkeypatch): + """Same degrade for the sprint-board sync: an unreadable spec must not advance + the board, and must not crash the run.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + write_spec(sp, "done", "abc123") + before = project.sprint_status.read_bytes() + fault_read_text(monkeypatch, sp) + + engine._post_dev_state_sync(StoryTask(story_key="1-1-a", epic=1), {"spec_file": str(sp)}) + + assert project.sprint_status.read_bytes() == before # board not advanced + events = [e for e in engine.journal.entries() if e["kind"] == "spec-read-failed"] + assert len(events) == 1 and events[0]["site"] == "post-dev-sync" + assert events[0]["story_key"] == "1-1-a" + + +def test_transient_spec_read_fault_does_not_crash_run(project, monkeypatch): + """Integration capstone for #97. A single transient OSError on the spec — a + TOCTOU truncation while the dev skill rewrites the file the orchestrator is + reading back — used to escape to `engine.run()`'s `except Exception` and mark + the WHOLE RUN crashed, abandoning every remaining story. + + The run now absorbs it: the first read (the reconcile bookkeeping pass) skips + and journals, every later read succeeds against the real spec, and the story + lands DONE. One fault, one journal event, no crash.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine(project, [generic_dev_effect(project, "1-1-a", followup_review=False)]) + sp = spec_path(project, "1-1-a") + real, fired = Path.read_text, [] + + def raise_once_then_delegate(self, *a, **kw): + if self == sp and not fired: + fired.append(self) + raise PermissionError(13, "Permission denied") + return real(self, *a, **kw) + + monkeypatch.setattr(Path, "read_text", raise_once_then_delegate) + summary = engine.run() + + assert fired # the fault really fired (a green run proves nothing otherwise) + assert not summary.crashed and summary.done == 1 + assert engine.state.tasks["1-1-a"].phase == Phase.DONE + events = [e for e in engine.journal.entries() if e["kind"] == "spec-read-failed"] + assert len(events) == 1 and events[0]["site"] == "reconcile" + + +def _crash_replay_setup(project): + """A host death that leaves the replay-fold's exact preconditions on disk. + + The dev session runs in the reconcile scenario (prose done, frontmatter + lagging), so its durable record is the pre-reconcile snapshot + `devcontract.synthesize_result` produces there: status "in-progress" and NO + `followup_review_recommended` key (only written on a done synth). The host + dies in the post-session window — phase persists as DEV_RUNNING — but only + AFTER the original run's reconcile repaired the spec on disk (that write + lands before the next state save), so the resumed reconcile enters the + already-finalized branch whose re-fold is the sole carrier of the followup + flag back onto the replay.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + inner = dev_effect(project, "1-1-a", final_status="in-progress", prose_status="done") + + def snapshot_effect(spec): + result = inner(spec) + result.result_json["status"] = "in-progress" + del result.result_json["followup_review_recommended"] + return result + + engine, _ = make_engine(project, [snapshot_effect]) + original_emit = engine._emit + + def crashing_emit(stage, *args, **kwargs): + if stage == "post_session": + raise RuntimeError("host died in the post-session window") + return original_emit(stage, *args, **kwargs) + + engine._emit = crashing_emit + assert engine.run().crashed + saved = load_state(engine.run_dir).tasks["1-1-a"] + assert saved.phase == Phase.DEV_RUNNING + assert "followup_review_recommended" not in saved.sessions[0].result_json + + sp = spec_path(project, "1-1-a") + sp.write_text( + "---\ntitle: 'test'\ntype: 'feature'\nstatus: 'done'\n" + f"baseline_revision: '{rev_parse_head(project.project)}'\n" + "followup_review_recommended: true\n---\n\n## Intent\n\ntest spec\n" + "\n## Auto Run Result\n\n- Status: done\n\nSummary: test.\n", + encoding="utf-8", + ) + return engine, sp + + +def test_resume_replay_fault_still_routes_recommended_review(project, monkeypatch): + """The resume counterpart of the capstone above. A replayed dev result is a + pre-reconcile snapshot with no followup key, and the reconcile re-fold is + what restores it — a transient read fault used to drop that fold silently. + The verify gate re-supplies only *status*, so the story committed with its + recommended follow-up review skipped. Routing now re-derives from the + finalized spec at consumption (`_followup_from_spec`): the fault costs one + journal event, not the review.""" + engine, sp = _crash_replay_setup(project) + real, fired = Path.read_text, [] + + def raise_once_then_delegate(self, *a, **kw): + if self == sp and not fired: + fired.append(self) + raise PermissionError(13, "Permission denied") + return real(self, *a, **kw) + + resumed, adapter = resume_engine(project, engine, [review_effect(project, "1-1-a", clean=True)]) + monkeypatch.setattr(Path, "read_text", raise_once_then_delegate) + summary = resumed.run() + + assert fired # the reconcile read really faulted + assert not summary.crashed and summary.done == 1 + assert [s.role for s in adapter.sessions] == ["review"] # routed, dev not re-run + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "review-not-recommended" not in kinds + events = [e for e in resumed.journal.entries() if e["kind"] == "spec-read-failed"] + assert len(events) == 1 and events[0]["site"] == "reconcile" + + +def test_resume_replay_persistent_fault_degrades_and_defers(project, monkeypatch): + """When the fault outlives the routing fallback too, the degrade stays the + decided one: routing falls back to False (journaled at site + `followup-routing`), the verify gate's own faulted read turns each attempt + into a retry, and the attempt budget lands the story in DEFERRED — never a + crash, never a phantom review.""" + engine, sp = _crash_replay_setup(project) + resumed, adapter = resume_engine(project, engine, [dev_effect(project, "1-1-a")]) + fault_read_text(monkeypatch, sp) + summary = resumed.run() + + assert not summary.crashed and summary.done == 0 + final = load_state(resumed.run_dir).tasks["1-1-a"] + assert final.phase == Phase.DEFERRED + assert [s.role for s in adapter.sessions] == ["dev"] # the one budgeted retry + sites = [e["site"] for e in resumed.journal.entries() if e["kind"] == "spec-read-failed"] + assert "reconcile" in sites and "followup-routing" in sites + + def test_generic_reconcile_idempotent_when_already_done(project): """When the skill DID advance the frontmatter to done, reconcile is a no-op: no second write, no `spec-status-reconciled` journal entry.""" diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 57bdf0b..a028682 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -1298,6 +1298,27 @@ def test_post_kill_reconcile_non_utf8_stories_spec_keeps_stall(tmp_path): ) +def test_stories_readback_oserror_spec_returns_none(tmp_path, monkeypatch): + """The read-back *poll* (not the post-kill hook) is where the issue's headline + crash lived: this path guards only UnicodeDecodeError, so an OSError escaped to + engine.run()'s `except Exception` and marked the whole run crashed. It now reads + like a spec that has not terminated yet — poll returns None, grace expires, the + stall/timeout verdict routes through the designed ladder. + + `devcontract` binds `read_frontmatter` by ``from .verify import``, so patch the + name on `devcontract`; patching `verify.read_frontmatter` would not rebind it. + Faulting `Path.read_text` instead would also trip `stories.resolve_story_spec`, + whose own guard would mask which read actually failed.""" + adapter, _ = make_dev_adapter(tmp_path) + _write_story_spec(tmp_path, "1", "slug", _DONE_SPEC) + + def boom(_path): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(generic.devcontract, "read_frontmatter", boom) + assert adapter._stories_synth_result(_dev_handle(), _stories_spec(tmp_path), wait=False) is None + + def test_post_kill_reconcile_blank_frontmatter_prose_done_rescues(tmp_path): """status_consistent is "no active disagreement": a blank frontmatter with prose `done` is exactly what a delivered Stop would have synthesized (the engine's diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 93c6287..6e0e72f 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -9,6 +9,7 @@ bundle_dev_effect, bundle_dev_escalates, bundle_review_effect, + fault_read_text, git, migrate_effect, triage_effect, @@ -472,6 +473,36 @@ def test_generic_skill_bundle_orchestrator_closes_ledger(project): assert "sweep-bundle-closed" in kinds +def test_bundle_ledger_close_skips_on_unreadable_spec(project, monkeypatch): + """The bundle counterpart of the sprint-board sync: an unreadable bundle spec + must not close any dw id (the ledger write is a repair — it must never fire off + an observation the orchestrator could not make) and must not crash the sweep.""" + write_ledger(project, {"DW-1": "open", "DW-2": "open"}) + pol = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + review=ReviewPolicy(enabled=False), + dev=DevPolicy(skill="bmad-dev-auto"), + scm=ScmPolicy(rollback_on_failure=True), + ) + engine, _ = make_sweep(project, [], policy=pol) + sp = project.implementation_artifacts / "spec-dw-fix-things.md" + sp.parent.mkdir(parents=True, exist_ok=True) + write_spec(sp, "done", "abc123") + task = StoryTask(story_key="dw-fix-things", epic=0, dw_ids=["DW-1", "DW-2"]) + fault_read_text(monkeypatch, sp) + + engine._post_dev_state_sync(task, {"spec_file": str(sp)}) + + entries = ledger_entries(project) + assert entries["DW-1"].status == "open" and entries["DW-2"].status == "open" + kinds = [e["kind"] for e in engine.journal.entries()] + assert "sweep-bundle-closed" not in kinds + events = [e for e in engine.journal.entries() if e["kind"] == "spec-read-failed"] + assert len(events) == 1 and events[0]["site"] == "bundle-ledger-close" + assert events[0]["story_key"] == "dw-fix-things" + + def test_generic_bundle_review_verify_recloses_ledger_after_review_rewrites_it(project): """A follow-up review can rewrite deferred-work.md from its own snapshot and re-open entries the orchestrator already closed after dev. The review gate diff --git a/tests/test_verify.py b/tests/test_verify.py index e0ed7e8..1fd5512 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -4,7 +4,7 @@ from pathlib import Path import pytest -from conftest import git, spec_path, write_spec, write_sprint +from conftest import fault_read_text, git, spec_path, write_spec, write_sprint from bmad_loop import verify from bmad_loop.model import StoryTask @@ -641,6 +641,71 @@ def test_verify_review_bundle_missing_entry_fails(project): assert not out.ok and out.fixable and "DW-2" in out.reason +def test_verify_shared_gates_oserror_degrades_to_retry(project, monkeypatch): + """An unreadable spec at the dev gate is a retryable outcome, not a whole-run + crash: the dev skill is still rewriting the spec this gate reads back, so a + transient OSError has a designed producer. The reason must name the read fault + — degrading to frontmatter {} would read as status "" and send a repair + session after a status bug that does not exist.""" + write_sprint(project, {"1-1-a": "review"}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-review", task.baseline_commit) + (project.project / "src.txt").write_text("changed\n") + fault_read_text(monkeypatch, sp) + + out = verify.verify_dev(task, project, dev_result(sp)) + assert not out.ok and out.retryable and not out.fixable + assert "spec unreadable" in out.reason and "PermissionError" in out.reason + assert "spec status is" not in out.reason # never masquerades as a status mismatch + + +@pytest.mark.parametrize("mode", ["review", "review_stories", "review_bundle"]) +def test_verify_review_gates_oserror_degrades_to_retry(project, monkeypatch, mode): + """Same degrade at all three review gates.""" + if mode == "review": + write_sprint(project, {"1-1-a": "done"}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "done", task.baseline_commit) + gate = verify.verify_review + elif mode == "review_stories": + task = make_stories_task(project, "1") + sp = write_story( + project.planning_artifacts / "epic-a", "1", "x", "done", task.baseline_commit + ) + gate = verify.verify_review_stories + else: + task = make_bundle_task(project) + sp = project.implementation_artifacts / "spec-dw-test-bundle.md" + write_spec(sp, "done", task.baseline_commit) + bundle_ledger(project, {"DW-1": "done 2026-06-11", "DW-2": "done 2026-06-11"}) + gate = verify.verify_review_bundle + task.spec_file = str(sp) + fault_read_text(monkeypatch, sp) + + out = gate(task, project, Policy()) + assert not out.ok and out.retryable + assert "spec unreadable" in out.reason and "PermissionError" in out.reason + assert "expected 'done'" not in out.reason + + +def test_verify_review_bundle_ledger_oserror_degrades_to_retry(project, monkeypatch): + """The ledger read is the same TOCTOU class as the spec read beside it — the + orchestrator's own `mark_done` rewrites it between the dev and review gates.""" + task = make_bundle_task(project) + sp = project.implementation_artifacts / "spec-dw-test-bundle.md" + write_spec(sp, "done", task.baseline_commit) + task.spec_file = str(sp) + bundle_ledger(project, {"DW-1": "done 2026-06-11", "DW-2": "done 2026-06-11"}) + fault_read_text(monkeypatch, project.deferred_work) # spec reads fine + + out = verify.verify_review_bundle(task, project, Policy()) + assert not out.ok and out.retryable and not out.fixable + assert "deferred-work ledger unreadable" in out.reason and "PermissionError" in out.reason + assert "DW-1" not in out.reason # not the "entries not marked done" verdict + + def test_safe_rollback_reverts_tracked_and_removes_run_created(project): repo = project.project baseline = verify.rev_parse_head(repo) @@ -1175,6 +1240,21 @@ def test_read_frontmatter_tolerates_non_utf8(project): assert verify.read_frontmatter(p) == {} +def test_read_frontmatter_oserror_still_raises(project, monkeypatch): + """`read_frontmatter` degrades a *decode* fault to {} but must keep RAISING on + OSError. Repair callers (`reset_spec_status`, `mark_done`) read through it and + depend on the raise: silently skipping a rewrite leaves the spec in a state the + caller believes it fixed. The observation callers wrap it instead — + `verify._gate_frontmatter`, `Engine._observed_frontmatter`, + `devcontract.synthesize_result`. Widening the except here would erase that + distinction everywhere at once.""" + p = project.project / "x.md" + p.write_text("---\nstatus: done\n---\n") + fault_read_text(monkeypatch, p) + with pytest.raises(PermissionError): + verify.read_frontmatter(p) + + def test_artifact_relpaths_returns_in_repo_folders(project): """The orchestrator-owned artifact folders, repo-relative posix.""" rels = verify.artifact_relpaths(project)