diff --git a/CHANGELOG.md b/CHANGELOG.md index 64da7b7..8f30c2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,64 @@ breaking changes may land in a minor release. new resultless-stop breadcrumb verdicts `terminal-frontmatter-pending` and `ambiguous-frontmatter` record the fingerprint's progress. +- **Deterministic missing-marker catch + repair (#276).** Hardens the #224 missing-marker + fallback, whose attribution was heuristic (a 2-stable-Stops fingerprint, or a single sighting + post-kill) and left the spec non-compliant on disk: + - _Launch-state snapshot + content-hash gate._ Before every review launch the engine now + captures a `SpecSnapshot` (content hash, mtime, frontmatter status) of the spec immediately + after the #160 marker strip and threads it onto the review `SessionSpec`. The fallback + deterministically refuses to synthesize from a candidate whose bytes still hash equal to that + snapshot — the spec is provably untouched by this session (a `done` spec re-opened for review, + never re-written) — in **every** mode, including the dead-window post-kill reconcile. This kills + the documented dead-window false positive (a review killed after an mtime-only bump but before + the `in-review` flip, previously scored `done` without having run). New resultless-stop verdict + `unmodified-since-launch` and dead-window lifecycle crumb `frontmatter-unmodified-refused` + record each refusal. The snapshot is process-transient (a crash-resume degrades to the + conservative 2-observation path), and the review-launch frontmatter `status` is never + mutated — it remains load-bearing skill routing. The same launch-snapshot decision (M1 hash + + M2 transition, a single shared `_snapshot_verdict`) also guards the stories-mode folder+id + read-back, closing the identical false completion on that path; snapshot/candidate identity is + by resolved filesystem path, not raw string spelling. + - _Mid-session status-transition observation._ On each heartbeat tick the generic (and + OpenCode) dev adapter now samples the snapshotted spec's frontmatter and records the first + status it observes this session drive off its launch state to a live, non-terminal value + (in practice `in-review`), with a `spec-status-transition-observed` lifecycle crumb. A + recorded transition is deterministic proof the terminal frontmatter the spec later carries + is this session's own write, so the fallback synthesizes on a single terminal sighting — + live or dead window — instead of the 2-observation fingerprint (the synthesized crumb gains + a `transition` flag). A recorded transition now **outranks** the content-hash gate: a clean + review can round-trip `done → in-review → done` back to the launch bytes while still omitting + its marker, and the observed `in-review` proves it ran, so the hash gate refuses only when no + transition was seen. A transition that flips entirely between two ticks is missed and falls + back to the conservative fingerprint path. + - _Artifact repair._ When the fallback synthesizes a result the engine now appends the + `## Auto Run Result` marker the skill owed onto the on-disk spec (new + `devcontract.append_auto_run_result`, the inverse of the #160 strip), so the once-invisible + spec re-enters the normal marker scan and the next review launch strips it exactly like a + skill-written marker. Best-effort at the `session-synthesized-from-frontmatter` site (covers + live-Stop, crash-path, and post-kill synthesis): guarded to the generic path, refused for a + spec resolved outside the orchestrator-owned roots or whose fresh frontmatter no longer agrees + with the synthesized status (journal `spec-marker-repaired` / `spec-marker-repair-failed` / + `spec-marker-repair-skipped`), so it can never author a marker that disagrees with the + frontmatter. The append and the #160 strip/reset now rewrite the spec atomically (temp + + `atomic_replace`), so an interrupted or disk-full repair leaves the original spec intact rather + than truncated, and a spec ending in a bare `\r` no longer receives an invisible (unparsed) + heading. + - _Targeted contract nudge_ (`limits.dev_contract_nudge`, default `true`). On the first + `terminal-frontmatter-pending` Stop — a marker-less terminal spec that is not the hash-gate + refusal and whose transition is not yet proven — the dev adapter sends one tmux nudge asking + the skill to append the `## Auto Run Result` section it owed and end its turn, repairing the + omission at its source; a compliant append is then harvested by the ordinary marker scan (no + synthesis, no `synthesized_from_frontmatter` flag). It fires exactly once per session — marked + before the send (a raising transport still counts), never refilled, and touching no stall + counters, so an mtime bump that resets the observation counter can never re-nudge (the #149 + refill hazard structurally cannot apply). `contract-nudge-sent` lifecycle crumb; set + `dev_contract_nudge = false` to rely on harness-side synthesis alone. + + Together these hold one HARD CONSTRAINT: the spec's `status:` at review launch is load-bearing + routing input to the upstream skill, so the frontmatter is **never** mutated at review launch — + every mechanism is observation or a prose-append, never a status write. + ## [0.9.0] — 2026-07-21 ### Added diff --git a/docs/FEATURES.md b/docs/FEATURES.md index d4da128..f73edd0 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -40,6 +40,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Spec + implementation (dev stage) - Drives the upstream `bmad-dev-auto` skill (unmodified) in a fresh tmux session: it plans a 1.5–4k-token spec, auto-approves it, implements, and self-finalizes the spec; the orchestrator syncs `sprint-status` and synthesizes `result.json` from the spec the skill leaves on disk. +- Deterministic missing-marker catch + repair (#276): the review HALT intermittently finalizes a spec's frontmatter to a terminal `status:` without appending the `## Auto Run Result` section the harvest scan keys on, which once livelocked a finished story to a DEFER-drop (#224). Four harness-side mechanisms make catch + fix deterministic without ever mutating the launch frontmatter (load-bearing skill routing): a **launch-state content-hash snapshot** the fallback refuses to synthesize from when the candidate is byte-identical (a `done` spec merely re-opened for review — in every mode, killing the dead-window false positive); **mid-session status-transition observation** (a heartbeat-tick sighting of the spec moving off its launch status proves a later terminal frontmatter is this session's write, so synthesis needs one sighting, not two); **artifact repair** that appends the owed marker onto the spec on synthesis so it re-enters the normal scan; and — gated by `limits.dev_contract_nudge` (default on) — one **targeted contract nudge** per session asking the skill to append the section itself. Frontmatter synthesis stays the backstop for a session that never complies. - Spec-only contract between stages — review consumes the frozen spec, not the dev session's context. ### Verification (trust-nothing gate) @@ -93,7 +94,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`, `journal.jsonl` (every decision), `events/` (hook signals), `tasks//` (per-session prompt + `result.json` + diagnostic breadcrumbs — `session-lifecycle.jsonl` records timeout-fire, budget-guard trips/terminations (`budget-tripped` / `over-budget-fired`), and transport-failure classification (`env-fault-classified`, #194), `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/`, `deferred/`, `resolve/`, `ATTENTION`. +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`, `journal.jsonl` (every decision — including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` marker repair, #276), `events/` (hook signals), `tasks//` (per-session prompt + `result.json` + diagnostic breadcrumbs — `session-lifecycle.jsonl` records timeout-fire, budget-guard trips/terminations (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), and the #276 missing-marker forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`), `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch` (bytes unchanged since launch, #276), or `terminal-frontmatter-pending`), `logs/`, `deferred/`, `resolve/`, `ATTENTION`. - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 36f185b..486120d 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -219,10 +219,15 @@ One row per story (or sweep bundle/triage task) in the selected run: (the guard's mid-session sample at trip time). The matching `tasks//` dir holds the forensic breadcrumbs the adapter wrote while the session ran: `session-lifecycle.jsonl` (timeout-fire, - budget-guard `budget-tripped` / `over-budget-fired`, and kill-escalation - lines), `heartbeat.json` (the wait loop's proof-of-life — + budget-guard `budget-tripped` / `over-budget-fired`, kill-escalation, and the + #276 missing-marker forensics `spec-status-transition-observed` / + `frontmatter-unmodified-refused` / `contract-nudge-sent`), + `heartbeat.json` (the wait loop's proof-of-life — stale under a live session means the orchestrator itself was frozen), and - `resultless-stops.jsonl`. + `resultless-stops.jsonl` (each give-up Stop with its verdict: `no-artifact`, + `ambiguous-frontmatter`, `unmodified-since-launch` — the spec's bytes were + unchanged since review launch, so it is a prior `done` re-opened, not this + session's output (#276) — or `terminal-frontmatter-pending`). - **Log** — the active agent session's pane output (`logs/.log`), ANSI colors preserved, starting with a dim `— .log —` header. The active task is the last `session-start` without a matching `session-end` diff --git a/src/bmad_loop/adapters/base.py b/src/bmad_loop/adapters/base.py index 76b2d9b..a13b789 100644 --- a/src/bmad_loop/adapters/base.py +++ b/src/bmad_loop/adapters/base.py @@ -22,6 +22,29 @@ from ..model import TokenUsage +@dataclass(frozen=True) +class SpecSnapshot: + """Launch-state fingerprint of a review session's spec, captured by the + engine immediately after the pre-review-launch marker strip + (``_reset_spec_for_review``) and threaded onto its ``SessionSpec``. + + Lets the generic adapter's missing-marker fallback deterministically refuse + to synthesize from a candidate whose bytes are byte-identical to the spec's + launch state: such a spec is provably untouched by this session, so the + terminal ``status:`` it carries is the PRIOR pass's ``done`` (re-opened for + review), not proof this session finished (#276 M1). + + Process-transient: it rides the live ``SessionSpec`` only and is deliberately + NOT persisted (no ``StoryTask.to_dict`` entry). A crash-resume that + reconstructs the ``SessionSpec`` therefore carries no snapshot, and the + fallback degrades to its conservative 2-observation fingerprint path.""" + + path: str + mtime_ns: int + sha256: str + fm_status: str + + @dataclass(frozen=True) class SessionSpec: task_id: str @@ -53,6 +76,13 @@ class SessionSpec: token_budget_mode: str = "off" # "off" | "warn" | "enforce" token_budget_grace_s: float = 240.0 cache_read_weight: float = 0.1 + # Launch-state snapshot of a review session's spec (#276 M1): captured by the + # engine right after the pre-review-launch marker strip and threaded here so + # the generic adapter's missing-marker fallback can deterministically refuse + # to synthesize from a spec still byte-identical to its launch state. None for + # every non-review session and on a crash-resume (process-transient — see + # SpecSnapshot). Kept LAST so positional SessionSpec constructions stay valid. + spec_snapshot: SpecSnapshot | None = None @dataclass(frozen=True) @@ -142,6 +172,18 @@ def _post_kill_reconcile( verdict had to leave open.""" return result + def _observe_tick(self, handle: SessionHandle, spec: SessionSpec) -> None: + """Heartbeat-cadence hook for mid-session on-disk observation, called from + the wait loop's heartbeat-throttled block (~every HEARTBEAT_INTERVAL_S; the + first tick always fires). Base behavior: nothing. Adapters that drive a + skill whose terminal on-disk state is heuristic to attribute may sample it + here (see GenericDevAdapter's `_DevSynthesisMixin`, which records the spec's + first non-terminal status transition to make a later terminal frontmatter + deterministic proof this session wrote it, #276 M2). An observation seam + only — it MUST NOT mutate session state or the spec, and any read failure is + a sample it silently skips, never a verdict.""" + return None + def _classify_env_fault( self, handle: SessionHandle, spec: SessionSpec, result: SessionResult ) -> SessionResult: diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index 99be2fc..ce28585 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/src/bmad_loop/adapters/generic.py @@ -22,10 +22,13 @@ from __future__ import annotations import dataclasses +import enum +import hashlib import json import re import shlex import time +from collections.abc import Callable from pathlib import Path from typing import TYPE_CHECKING @@ -40,7 +43,7 @@ from ..signals import SignalWatcher from ..tokens import read_usage as tally_usage from ..verify import read_frontmatter -from .base import CodingCLIAdapter, SessionHandle, SessionResult, SessionSpec +from .base import CodingCLIAdapter, SessionHandle, SessionResult, SessionSpec, SpecSnapshot from .multiplexer import MultiplexerError, TerminalMultiplexer, get_multiplexer from .profile import CLIProfile @@ -64,6 +67,27 @@ # a session mid-edit cannot produce. A dead window skips the counter entirely: # the kill settled liveness, so the frontmatter is as final as it will ever get. FM_FALLBACK_MIN_OBS = 2 + + +class _SnapVerdict(enum.Enum): + """Launch-snapshot (#276 M1/M2) decision, shared by the mtime-scan fallback and + the stories read-back so the two completion paths can never drift. + + NEUTRAL — no snapshot, a different file, or bytes changed since launch: fall + through to the path's normal accept logic. + PROVEN — a mid-session status transition (M2) was observed for this spec: + single-sighting harvest, and it OUTRANKS a byte-identical hash (a clean review + can round-trip back to the launch bytes yet provably ran). + REFUSE — bytes still byte-identical to the review-launch snapshot AND no + transition was observed (M1): the documented dead-window false positive + (a `done` spec re-opened for review, mtime-bumped but never re-driven). + """ + + NEUTRAL = "neutral" + PROVEN = "proven" + REFUSE = "refuse" + + # Post-mortem transport-failure classification (#194): how much of the tee'd # pane log's tail to scan, how long an evidence excerpt to keep, and which # non-completed statuses are eligible. over_budget is excluded — a budget @@ -123,6 +147,25 @@ "completion artifact does; if you cannot finish, mark the work blocked in it " "and end your turn." ) +# Targeted contract-repair nudge (#276 M4): a Stop found the spec at +# {spec_path} finalized to terminal frontmatter status {status} but WITHOUT the +# `## Auto Run Result` section bmad-loop's harvest scan keys on. Ask the skill to +# append that section itself so the omission is fixed at the source (a compliant +# append is then harvested by the normal scan; harness-side frontmatter synthesis +# stays the backstop). Sent at most once per session and never re-armed, so it is +# safe to be specific and directive. Guarded ("if this spec is not yours or the +# work is unfinished") so a session legitimately mid-workflow is not derailed. +CONTRACT_NUDGE_TEXT = ( + "You are running in bmad-loop automation mode. The spec at {spec_path} now " + "carries a terminal frontmatter `status: {status}`, but it is missing the " + "`## Auto Run Result` section your contract requires — bmad-loop harvests " + "that section, not the frontmatter, so without it this finished story looks " + "unfinished. If this spec is yours and the work is done, append the section " + "to the spec now — the `## Auto Run Result` heading, a `Status: {status}` " + "line matching the frontmatter, and a brief summary — then end your turn. If " + "this spec is not yours, or the work is not actually finished, ignore this " + "and continue your workflow instead." +) class _ResultFileMixin: @@ -458,6 +501,10 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi "stall_nudges_sent": stall_nudges_sent, }, ) + # Mid-session spec-status transition sampling (#276 M2) rides the + # same heartbeat cadence — a no-op unless this adapter drives the + # generic skill and the engine threaded a launch snapshot. + self._observe_tick(handle, spec) # Budget sampling rides the heartbeat cadence — no extra knob. # transcript_path is unknown until the first hook event carries # it (SessionStart for claude); until then the guard is inert. @@ -996,6 +1043,12 @@ class _DevSynthesisMixin(_ResultFileMixin): # (no runtime effect) tell the type checker the host attributes this reads. paths: ProjectPaths policy: Policy + # The concrete adapter's real transport (GenericDevAdapter/OpencodeDevAdapter + # both define `def send_text`). Declared here as a BARE annotation, never a + # `def`: the mixin precedes the concrete adapter in MRO, so a stub method + # would shadow the real one on both adapters. The contract nudge (#276 M4) + # sends through it. + send_text: Callable[[SessionHandle, str], None] def _configure_dev_knobs(self) -> None: """Override the base result-file knobs for the bmad-dev-auto contract; @@ -1013,6 +1066,20 @@ def _configure_dev_knobs(self) -> None: # Task ids are unique per session, so entries never need resetting # between sessions; the dict lives for the adapter's lifetime. self._fm_fallback_obs: dict[str, tuple[str, int, str, int]] = {} + # First mid-session spec-status transition observed per session (#276 M2): + # task_id -> normalized status. Recorded by `_observe_tick` when the spec's + # frontmatter first moves off its launch status to a non-terminal state (in + # practice `in-review`), which makes a later terminal frontmatter proof THIS + # session wrote it. Same lifetime doctrine as `_fm_fallback_obs` — task_ids + # are unique per session, so entries are recorded once and never cleared. + self._fm_transition_obs: dict[str, str] = {} + # Targeted contract-nudge budget (#276 M4): task_ids that have already been + # sent the one CONTRACT_NUDGE_TEXT nudge. A set, never cleared, so the nudge + # fires at most once per session even though an mtime bump resets the + # `_fm_fallback_obs` observation counter to 1 (#149's refill hazard cannot + # apply — this budget is not a counter and touches no stall counters). + self._contract_nudge_sent: set[str] = set() + self._contract_nudge_enabled = self.policy.limits.dev_contract_nudge def _probe_alive(self, handle: SessionHandle) -> bool | None: """Liveness of the session's native surface (tmux window, server @@ -1076,6 +1143,77 @@ def _synthesize_from(self, spec_path: Path, spec: SessionSpec) -> devcontract.Sy dw_ids = [tok for tok in (i.strip() for i in raw_dw_ids) if tok] return devcontract.synthesize_result(spec_path, story_key=story_key, dw_ids=dw_ids or None) + def _observe_tick(self, handle: SessionHandle, spec: SessionSpec) -> None: + """Mid-session status-transition observation (#276 M2), called each + heartbeat tick (~every HEARTBEAT_INTERVAL_S; the first tick fires too). + Records the FIRST spec frontmatter status this session drives off its + launch state to a live, non-terminal value (in practice ``in-review``) + into ``_fm_transition_obs``. That single sighting is what lets + ``_frontmatter_fallback`` treat a later terminal frontmatter as + deterministic proof THIS session wrote it (``transition_proven``), so it + can synthesize on ONE terminal sighting instead of the 2-observation + fingerprint. + + A pure sampling path, never a verdict path: it needs a launch snapshot to + observe against, fires at most once per session (task_ids are unique; + entries are never cleared), and any unreadable/torn read is a skipped + sample (silent OSError return), never evidence. Blank/torn parses (``s == + ""``) and terminal states (``done``/``blocked``) are NOT recorded — a + terminal frontmatter is the Stop harvest's business, and the launch status + itself (the ``done`` a review re-opens) is not a transition. A transition + that flips entirely between two ticks is simply missed, and the fallback + keeps its conservative 2-observation path.""" + task_id = handle.task_id + snap = spec.spec_snapshot + if snap is None or task_id in self._fm_transition_obs: + return + try: + s = str(read_frontmatter(Path(snap.path)).get("status", "")).strip().lower() + except OSError: + return + if s != "" and s not in (devcontract.DONE, devcontract.BLOCKED) and s != snap.fm_status: + self._fm_transition_obs[task_id] = s + self._note_lifecycle( + task_id, "spec-status-transition-observed", spec=snap.path, status=s + ) + + @staticmethod + def _same_spec(candidate: Path, snap_path: str) -> bool: + """Whether ``candidate`` and the snapshot's recorded path are the SAME file + by filesystem identity (#276 M1), not raw string spelling. ``snap_path`` is + the engine's ``str(task.spec_file)``; a ``..`` segment, a symlinked artifacts + dir, or a case-variant alias makes an equivalent path compare unequal + lexically and would silently disable the hash/transition gate. ``resolve()`` + (the repo's identity convention, non-strict) collapses those; an unresolvable + path degrades to "not the same file" — conservative, the gate stays inert + rather than ever falsely refusing.""" + try: + return candidate.resolve() == Path(snap_path).resolve() + except OSError: + return False + + def _snapshot_verdict( + self, + *, + same_file: bool, + snap: SpecSnapshot | None, + task_id: str, + digest: str | None, + ) -> _SnapVerdict: + """Shared M1/M2 launch-snapshot decision (see ``_SnapVerdict``). Pure logic, + no I/O: each caller precomputes ``same_file`` (via ``_same_spec``) and, only + when it holds, ``digest`` (so an unrelated spec is never hashed), preserving + each path's own read-error semantics. A recorded transition (M2) outranks the + content hash (M1); the hash gate refuses only when no transition was observed + and the bytes are still identical to the launch snapshot.""" + if snap is None or not same_file: + return _SnapVerdict.NEUTRAL + if task_id in self._fm_transition_obs: + return _SnapVerdict.PROVEN + if digest is not None and digest == snap.sha256: + return _SnapVerdict.REFUSE + return _SnapVerdict.NEUTRAL + def _frontmatter_fallback( self, handle: SessionHandle, @@ -1097,9 +1235,26 @@ def _frontmatter_fallback( evidence than the marker, so the live path harvests only a fingerprint (path, mtime, status) that held stable across ``FM_FALLBACK_MIN_OBS`` resultless Stops; a dead window (post-kill reconcile) harvests on one - sighting, liveness having been settled by the kill. Several candidates - mean the scan cannot know which spec is this session's — refuse to - guess. Every synthesized result still runs the engine's full + sighting, liveness having been settled by the kill. A recorded mid-session + transition (#276 M2) is a third single-sighting route, live or dead: having + observed this session drive the spec off its launch ``status:`` to a live + non-terminal state (``in-review``) proves the terminal frontmatter it now + carries is this session's own write, not a stale prior ``done``, so one + terminal sighting suffices — the ``transition=`` flag on the synthesized + crumb marks it. A transition that flips entirely between two ticks is simply + missed, and this stays on the conservative 2-observation fingerprint. + Several candidates mean the scan cannot know which spec is this session's — + refuse to guess. The launch-snapshot hash (#276 M1) and the transition (M2) + interact via ``_snapshot_verdict``: when the engine threaded a + ``spec_snapshot`` (review sessions) and the candidate's bytes still hash + equal to it, synthesis is deterministically REFUSED in every mode, including + the dead window (the ``unmodified-since-launch`` verdict / + ``frontmatter-unmodified-refused`` crumb) — the ``done`` spec re-opened for + review, mtime-bumped but never re-driven. A recorded transition OUTRANKS the + hash, though: a clean review can round-trip ``done -> in-review -> done`` back + to the launch bytes while still omitting its marker, and the observed + ``in-review`` proves it ran — so REFUSE fires only when NO transition was + seen. Every synthesized result still runs the engine's full deterministic verify downstream, the same #61 trust model as the post-kill rescue. With this in place a marker-less ``done`` spec completes here and never reaches the review-timeout path, so the @@ -1107,10 +1262,23 @@ def _frontmatter_fallback( case: a review that died with a NON-terminal frontmatter. Owns the give-up breadcrumb: exactly one of ``no-artifact``, - ``ambiguous-frontmatter``, or ``terminal-frontmatter-pending`` per - wait=True pass (none on a harvest). A plain wait=False read (the crash - path) is compare-only — it may harvest an already-stable fingerprint but - never records observations or breadcrumbs. + ``ambiguous-frontmatter``, ``unmodified-since-launch``, or + ``terminal-frontmatter-pending`` per wait=True pass (none on a harvest). + A plain wait=False read (the crash path) is compare-only — it may harvest + an already-stable fingerprint but never records observations or + breadcrumbs; the hash gate is the one wait=False path that leaves a crumb, + and only under a dead window (``frontmatter-unmodified-refused``). + + On the FIRST ``terminal-frontmatter-pending`` observation (wait=True, one + candidate, not the hash-gate refusal, transition not yet proven) it also + fires the #276 M4 contract nudge when ``limits.dev_contract_nudge`` is on: + one ``CONTRACT_NUDGE_TEXT`` send asking the skill to append the marker it + owed, then repair at the source rather than only synthesizing here. It is + bounded by the never-cleared ``_contract_nudge_sent`` set (marked before + the send, ``MultiplexerError`` swallowed) — exactly once per session, + touching no stall counters, so an mtime bump that resets ``observations`` + to 1 never re-nudges. A compliant append is harvested by the ordinary + marker scan on a later Stop, leaving synthesis as the backstop. """ task_id = handle.task_id candidates: list[Path] = [] @@ -1141,9 +1309,15 @@ def _frontmatter_fallback( ) return None path = candidates[0] + snap = spec.spec_snapshot + same_file = snap is not None and self._same_spec(path, snap.path) try: mtime_ns = path.stat().st_mtime_ns fm_status = str(read_frontmatter(path).get("status", "")).strip().lower() + # Content hash only when the candidate IS the snapshotted spec (compared + # by filesystem identity) — an unrelated marker-less spec under the same + # artifacts dir shares no launch state, so hashing it is meaningless work. + digest = hashlib.sha256(path.read_bytes()).hexdigest() if same_file else None except OSError: # Torn mid-write read: not evidence of anything — same degrade as # the read-back doctrine everywhere else on this path. @@ -1152,11 +1326,44 @@ def _frontmatter_fallback( task_id, "no-artifact", f"unreadable marker-less candidate {path}" ) return None + # Launch-snapshot verdict (#276 M1/M2), shared with the stories read-back. + # REFUSE (M1) — bytes byte-identical to the review-launch snapshot with NO + # transition observed — is the documented dead-window false positive (a + # `done` spec re-opened for review, mtime-bumped but never re-driven); + # refuse in EVERY mode, including `dead_window`. A PROVEN transition (M2) + # outranks it and falls through to synthesis below. No observation is + # recorded or popped on REFUSE — an unchanged spec is neither progress nor a + # stall. + verdict = self._snapshot_verdict( + same_file=same_file, snap=snap, task_id=task_id, digest=digest + ) + if verdict is _SnapVerdict.REFUSE: + assert snap is not None # REFUSE is returned only for a matched snapshot + if wait: + self._note_resultless_stop( + task_id, + "unmodified-since-launch", + f"{path} byte-identical to review-launch snapshot " + f"(snapshot mtime_ns={snap.mtime_ns}, candidate mtime_ns={mtime_ns}); " + "refusing frontmatter synthesis", + ) + elif dead_window: + self._note_lifecycle( + task_id, + "frontmatter-unmodified-refused", + spec=str(path), + status=fm_status, + dead_window=True, + ) + return None fingerprint = (str(path), mtime_ns, fm_status) prev = self._fm_fallback_obs.get(task_id) stable = prev is not None and prev[:3] == fingerprint observations = (prev[3] + 1) if (stable and prev is not None) else 1 - if dead_window or (stable and observations >= FM_FALLBACK_MIN_OBS): + # A recorded mid-session transition (#276 M2) proves the terminal frontmatter + # is this session's write → single-sighting harvest, like a dead window. + transition_proven = verdict is _SnapVerdict.PROVEN + if dead_window or transition_proven or (stable and observations >= FM_FALLBACK_MIN_OBS): sr = self._synthesize_from(path, spec) if sr.result_json is not None: sr.result_json["synthesized_from_frontmatter"] = True @@ -1166,6 +1373,7 @@ def _frontmatter_fallback( spec=str(path), status=fm_status, dead_window=dead_window, + transition=transition_proven, ) return sr if wait: @@ -1176,6 +1384,31 @@ def _frontmatter_fallback( f"{path} frontmatter status={fm_status!r} with no '## Auto Run Result'" f" marker; observation {observations}/{FM_FALLBACK_MIN_OBS} before synthesis", ) + # Contract nudge (#276 M4): at the FIRST pending observation, ask the + # skill to append the `## Auto Run Result` section it owed so the + # omission is repaired at the source (a compliant append is then + # harvested by the normal marker scan on a later Stop; synthesis stays + # the backstop). Exactly once per session: the task_id is marked BEFORE + # the send so a raising transport still satisfies exactly-once, and the + # never-cleared set — not the mtime-resettable observation counter — is + # the budget, so the #149 refill hazard cannot apply. Touches no stall + # counters. + if ( + self._contract_nudge_enabled + and observations == 1 + and task_id not in self._contract_nudge_sent + ): + self._contract_nudge_sent.add(task_id) + self._note_lifecycle( + task_id, "contract-nudge-sent", spec=str(path), status=fm_status + ) + try: + self.send_text( + handle, + CONTRACT_NUDGE_TEXT.format(spec_path=path, status=fm_status), + ) + except MultiplexerError: + pass return None def _stories_synth_result( @@ -1200,7 +1433,19 @@ def _stories_synth_result( On a plan-halt leg (``BMAD_LOOP_PLAN_HALT`` set by the engine for a spec_checkpoint story's first dispatch) the skill HALTs at ``ready-for-dev``; pass ``plan_halt=True`` so synthesize treats that as a - successful terminal (marked ``plan_halt``) rather than died-mid-flight.""" + successful terminal (marked ``plan_halt``) rather than died-mid-flight. + + A review session also carries a launch ``spec_snapshot``, so before + synthesizing this applies the shared ``_snapshot_verdict`` gate (#276 M1/M2): + an unmodified-since-launch ``done`` spec with no observed transition is + REFUSED (the ``unmodified-since-launch`` verdict), closing the same + false-positive completion the mtime-scan fallback closes — a review that only + bumped the mtime of the stripped launch spec no longer reads as done. A dev + leg carries no snapshot → the gate is inert (mtime-floor accept). Identity is + filesystem-based (``_same_spec``): under worktree isolation, if ``base`` + resolves into the worktree but the snapshot path is the main checkout the two + differ and the gate stays inert — conservative (no false accept, just no + extra protection).""" from .. import stories story_key = spec.env.get("BMAD_LOOP_STORY_KEY") or "" @@ -1230,23 +1475,54 @@ def _stories_synth_result( verdict = "stale-mtime" detail = f"{state.path} predates session launch" else: - try: - sr = devcontract.synthesize_result( - state.path, story_key=story_key or None, plan_halt=plan_halt + # Launch-snapshot gate (#276 M1/M2), shared with the mtime-scan + # fallback so the stories read-back can't false-complete on an + # unmodified `done` spec. Only bites on a review session (the + # engine threads `spec_snapshot` there); a dev leg leaves it None + # → NEUTRAL → the mtime-floor accept below. + snap = spec.spec_snapshot + same_file = snap is not None and self._same_spec(state.path, snap.path) + digest = None + if same_file: + try: + digest = hashlib.sha256(state.path.read_bytes()).hexdigest() + except OSError: + digest = None # torn read → NEUTRAL; synthesize keeps its degrade + snap_verdict = self._snapshot_verdict( + same_file=same_file, snap=snap, task_id=handle.task_id, digest=digest + ) + if snap_verdict is _SnapVerdict.REFUSE: + assert snap is not None # REFUSE implies a matched snapshot + # Byte-identical to the review-launch snapshot with no + # transition observed — the same dead-window false positive + # the scan path refuses. Fall through to keep polling the + # grace (a real mid-grace write flips the verdict), then + # breadcrumb + None on the deadline, like `stale-mtime`. + verdict = "unmodified-since-launch" + detail = ( + f"{state.path} byte-identical to review-launch snapshot " + f"(snapshot mtime_ns={snap.mtime_ns}); refusing stories synthesis" + ) + else: + try: + sr = devcontract.synthesize_result( + state.path, story_key=story_key or None, plan_halt=plan_halt + ) + except UnicodeDecodeError: + # A non-UTF-8 read is either a torn glimpse of a spec still + # being written (keep polling — a later pass sees the finished + # write) or a genuinely corrupt file: then the grace expires + # result-less and the next _pick_next re-classifies it as a + # wedge (resolve_story_spec degrades an undecodable PRESENT + # spec to status "" → pause for resolve), never a crash of + # the read-back poll. + sr = None + if sr is not None and sr.result_json is not None: + return sr + verdict = "not-terminal" + detail = ( + f"{state.path} has no terminal status (frontmatter {state.status!r})" ) - except UnicodeDecodeError: - # A non-UTF-8 read is either a torn glimpse of a spec still - # being written (keep polling — a later pass sees the finished - # write) or a genuinely corrupt file: then the grace expires - # result-less and the next _pick_next re-classifies it as a - # wedge (resolve_story_spec degrades an undecodable PRESENT - # spec to status "" → pause for resolve), never a crash of - # the read-back poll. - sr = None - if sr is not None and sr.result_json is not None: - return sr - verdict = "not-terminal" - detail = f"{state.path} has no terminal status (frontmatter {state.status!r})" if not wait or time.monotonic() >= deadline: if wait: self._note_resultless_stop(handle.task_id, verdict, detail) diff --git a/src/bmad_loop/adapters/opencode_http.py b/src/bmad_loop/adapters/opencode_http.py index e38eb36..b2f1c87 100644 --- a/src/bmad_loop/adapters/opencode_http.py +++ b/src/bmad_loop/adapters/opencode_http.py @@ -616,6 +616,11 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi "stall_nudges_sent": stall_nudges_sent, }, ) + # Mid-session spec-status transition sampling (#276 M2) rides the + # same heartbeat cadence — a no-op on the plain HTTP adapter; the + # OpencodeDevAdapter shares _DevSynthesisMixin, so the hook records + # transitions there exactly as on the generic dev adapter. + self._observe_tick(handle, spec) # Budget sampling rides the heartbeat cadence — no extra knob. # Usage comes over HTTP (server state is sqlite, not a file). if ( diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index edb8069..804784c 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/src/bmad_loop/data/settings/core.toml @@ -136,6 +136,12 @@ minimum = 0 default_ref = "LimitsPolicy.workflow_stall_nudges_cap" description = "total (never-restored) stall nudges for an injected plugin-workflow session before it is called stalled; bounds a session that finished its work but never wrote its completion marker · 0 = stall on first grace expiry" [[section.field]] +key = "dev_contract_nudge" +kind = "switch" +default_ref = "LimitsPolicy.dev_contract_nudge" +label = "dev contract nudge" +description = "ON: one targeted nudge per session when a Stop finds a spec finalized to a terminal frontmatter status but missing its `## Auto Run Result` marker, asking the skill to append it and end its turn (sent exactly once, never refilled, touches no stall counters) · OFF: rely only on harness-side frontmatter synthesis" +[[section.field]] key = "max_tokens_per_story" kind = "int" minimum = 1 diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index 2b2a612..b50e6d4 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -25,6 +25,7 @@ from pathlib import Path from typing import Any +from .platform_util import atomic_replace from .verify import DEV_WORKFLOW, read_frontmatter # The section the skill appends on EVERY terminal path (success and blocked), @@ -369,6 +370,29 @@ def find_frontmatter_candidates(impl_artifacts: Path, *, since_ns: int) -> list[ return [p for _, p in found] +def _atomic_write_spec(spec_path: Path, text: str) -> None: + """Rewrite ``spec_path`` with ``text`` via a same-directory temp file + atomic + rename, so an interrupted / short / disk-full write can never truncate the + canonical spec — a failed repair must lose no work (fault injection on the old + truncating ``write_text`` reduced a 46-byte spec to 12). Bytes are written + verbatim (``write_bytes``, not ``write_text``): every caller here has already + captured and preserved the file's own line endings, and ``write_text``'s + ``newline=None`` default would re-translate ``\\n``→``\\r\\n`` on Windows. The + ``.tmp`` sibling ends in ``.tmp`` (not ``.md``), so the ``*.md`` artifact scans + never see it. On any failure the temp file is removed and the error re-raised — + the callers impose best-effort, the writer never swallows.""" + tmp = spec_path.with_suffix(spec_path.suffix + ".tmp") + try: + tmp.write_bytes(text.encode("utf-8")) + atomic_replace(tmp, spec_path) + except BaseException: + try: + tmp.unlink() + except OSError: + pass + raise + + def reset_spec_status(spec_path: Path, new_status: str) -> bool: """Rewrite the frontmatter ``status:`` value of a spec in place. @@ -421,7 +445,7 @@ def _repl(m: re.Match[str]) -> str: changed = True if not changed: return False - spec_path.write_text(head + new_body + tail + text[fm.end() :], encoding="utf-8") + _atomic_write_spec(spec_path, head + new_body + tail + text[fm.end() :]) return True @@ -457,5 +481,79 @@ def strip_auto_run_result(spec_path: Path) -> bool: kept.append(text[pos : m.start()]) pos = _next_heading_start(text, m.end()) kept.append(text[pos:]) - spec_path.write_text("".join(kept), encoding="utf-8") + _atomic_write_spec(spec_path, "".join(kept)) + return True + + +# Provenance stamped into a synthesized `## Auto Run Result` section so a human +# (or a later re-derivation) can tell an orchestrator-repaired marker from one +# the skill wrote itself. Single line (no internal newlines) so the writer's +# line-ending detection governs every break in the appended block. +ORCHESTRATOR_SYNTH_NOTE = ( + "_Appended by the bmad-loop orchestrator (missing-marker repair, #224): the " + "session finalized this spec's frontmatter without its `## Auto Run Result` " + "marker, so the orchestrator synthesized the result from the frontmatter and " + "appended this section._" +) + + +def append_auto_run_result(spec_path: Path, status: str, *, detail: str = "") -> bool: + """Append a synthesized ``## Auto Run Result`` marker section — the inverse of + `strip_auto_run_result`. + + The missing-marker fallback (#224) synthesizes a session's result from a spec + that carries a terminal frontmatter ``status:`` but never got the marker the + skill owed; this writer brings that spec back into contract. Before the append + a marker-less terminal spec sits in `find_frontmatter_candidates`' territory + (it requires ZERO real headings); after it the spec carries exactly one real + ``## Auto Run Result`` heading and moves into `find_result_artifact`'s + (requires >= 1), so a later re-read is harvested on the normal marker path + instead of the fallback scan, and the next review launch strips it exactly + like a skill-written marker (#160). + + Returns False when the spec is absent, or when a REAL (non-fenced) + ``## Auto Run Result`` heading is already present — idempotence, and the same + #52 symmetry the strip honors: a fence-quoted heading (a frozen intent's + example) is documentation, not a marker, so it does NOT block the append. A + present-but-unreadable spec RAISES rather than no-ops (the repair-write + doctrine `strip_auto_run_result` and `reset_spec_status` share — the CALLER + imposes best-effort); silently skipping would leave the spec in a state the + caller believes it repaired. + + Newline handling follows `reset_spec_status`'s intent: the file's line ending + is detected (CRLF vs LF) and reused for the appended block, and a missing + trailing newline is added so the heading can never glue onto the last body line + (``...body## Auto Run Result``). The spec is read as raw bytes rather than via + `read_text`, whose universal-newline translation would both hide a CRLF file's + ending and silently rewrite its whole body to LF — an in-place repair must not + mutate line endings it did not author. The section is ``## Auto Run Result`` / + blank / ``Status: `` / blank / the provenance note (plus an optional + detail paragraph). ``status`` is normalized lowercase and MUST be the spec's + own frontmatter ``status`` — the caller passes exactly that — so + `synthesize_result`'s ``consistent`` cross-check holds on every later re-read.""" + if not spec_path.is_file(): + return False + # Raw read (not read_text): preserve the file's exact line endings, and let an + # undecodable spec raise UnicodeDecodeError like the strip's read (repair-write + # doctrine — the caller imposes best-effort, never the writer). + text = spec_path.read_bytes().decode("utf-8") + if _section_headings(text): + return False # a real marker is already present — idempotent + status = status.strip().lower() + nl = "\r\n" if "\r\n" in text else "\n" + # Ensure the heading lands after a newline the scan can recognize. A file + # already ending in "\n" (LF, or the "\n" of a CRLF) is left byte-for-byte + # intact, so a strip of this exact append round-trips. A file ending in a BARE + # "\r" is the trap: it reads as "already terminated", but AUTO_RUN_HEADING_RE's + # "^" (MULTILINE) only matches after "\n" — a heading glued directly after "\r" + # is invisible to the scan. Complete such a bare CR to CRLF (preserving the + # authored CR) so the heading begins on a recognized line boundary. + if text.endswith("\r"): + text += "\n" + elif text and not text.endswith("\n"): + text += nl + section = f"## Auto Run Result{nl}{nl}Status: {status}{nl}{nl}{ORCHESTRATOR_SYNTH_NOTE}{nl}" + if detail: + section += f"{nl}{detail.strip()}{nl}" + _atomic_write_spec(spec_path, text + section) return True diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 04fa3eb..6cd04e9 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -10,6 +10,7 @@ import contextlib import functools +import hashlib import shutil import signal import sys @@ -20,7 +21,7 @@ from typing import TYPE_CHECKING, Callable, NoReturn from . import deferredwork, devcontract, envvars, gates, verify -from .adapters.base import CodingCLIAdapter, SessionResult, SessionSpec +from .adapters.base import CodingCLIAdapter, SessionResult, SessionSpec, SpecSnapshot from .bmadconfig import ProjectPaths from .escalation import ( Action, @@ -1302,12 +1303,13 @@ def _review_and_commit( # marker past the adapter's launch-mtime floor and end the review # on its first result-less Stop (issue #160). Non-replay branch # only — the replay path above launches no session. - self._reset_spec_for_review(task) + snapshot = self._reset_spec_for_review(task) result = self._run_session( task, role="review", prompt=self._review_prompt(task), seq=task.review_cycle, + spec_snapshot=snapshot, ) advance(task, Phase.REVIEW_VERIFY) self._save() @@ -1878,6 +1880,94 @@ def _reconcile_generic_terminal_status(self, task: StoryTask, result_json: dict to=success_status, ) + def _repair_spec_marker(self, task: StoryTask, rj: dict) -> None: + """Append the ``## Auto Run Result`` marker a missing-marker synthesis + (#224) proved the session owed but never wrote — #276 Mechanism 3, the + artifact-repair leg. Called at the ``session-synthesized-from-frontmatter`` + journal site, which fires for live-Stop, crash-path, and post-kill + dead-window synthesis alike (all carry the ``synthesized_from_frontmatter`` + flag), so this ONE call site covers every synthesis path. After the append + the spec leaves `find_frontmatter_candidates`' territory (zero real + markers) and enters `find_result_artifact`'s (>= 1), so a later re-read is + harvested on the normal marker path and the next review launch strips it + exactly like a skill-written marker. + + Best-effort by doctrine: the result was already synthesized, so a failed + or skipped repair only leaves the spec non-compliant — it never loses work. + Guards mirror `_reconcile_generic_terminal_status`, the sibling + session-path spec writer: the generic path only; the session-supplied + ``spec_file`` must resolve to a real file inside the orchestrator-owned + roots (else `spec-marker-repair-skipped`, reason ``out-of-tree`` — this is + a write keyed off a session-reported path); and a FRESH frontmatter re-read + must be terminal (``done``/``blocked``) AND agree with the synthesized + ``rj["status"]`` (else reason ``fm-mismatch``). Never author a marker whose + ``Status:`` disagrees with the frontmatter the synthesis trusted — that + would trip `synthesize_result`'s consistency cross-check on the next read. + + Non-interference: `_reconcile_generic_terminal_status` only acts when the + frontmatter LAGS the prose, so once this append lands (frontmatter already + terminal) reconcile hits its idempotent / refusal branches; + `_salvage_review_timeout` reads the frontmatter fresh and stays disjoint. + The append is engine-side ONLY — an adapter-side write would perturb the + adapter's own mtime/hash observation state (#276 M1/M2).""" + if not self._generic_dev(): + return + spec_file = (rj or {}).get("spec_file") + if not spec_file: + return + spec_path = verify.resolve_spec_path(str(spec_file), self.workspace.paths) + if not spec_path.is_file(): + return + if not verify.spec_within_roots(spec_path, self.workspace.paths): + self.journal.append( + "spec-marker-repair-skipped", + story_key=task.story_key, + spec=str(spec_path), + reason="out-of-tree", + ) + return + fm = self._observed_frontmatter(spec_path, task.story_key, "marker-repair") + if fm is None: + return + fm_status = str(fm.get("status", "")).strip().lower() + rj_status = str(rj.get("status", "")).strip().lower() + if fm_status not in (devcontract.DONE, devcontract.BLOCKED) or fm_status != rj_status: + self.journal.append( + "spec-marker-repair-skipped", + story_key=task.story_key, + spec=str(spec_path), + reason="fm-mismatch", + ) + return + detail = ( + f"Synthesized by the bmad-loop orchestrator from frontmatter status " + f"`{fm_status}` for story `{task.story_key}` (session finalized the spec " + f"without appending its marker)." + ) + try: + repaired = devcontract.append_auto_run_result(spec_path, fm_status, detail=detail) + except (OSError, UnicodeDecodeError) as e: + # UnicodeDecodeError as well as OSError: the writer reads the spec's raw + # bytes and, by contract, raises on an undecodable spec (the same + # torn-mid-write hazard `_post_kill_reconcile` guards — a spec truncated + # through a multi-byte UTF-8 sequence between `_observed_frontmatter`'s + # read and this one). This repair is pure best-effort forensics; it must + # never turn a synthesized-and-recorded result into a run crash. + self.journal.append( + "spec-marker-repair-failed", + story_key=task.story_key, + spec=str(spec_path), + error=f"{e.__class__.__name__}: {e}", + ) + return + if repaired: + self.journal.append( + "spec-marker-repaired", + story_key=task.story_key, + spec=str(spec_path), + status=fm_status, + ) + def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> None: """Single-writer for the on-disk bookkeeping the generic skill never touches. @@ -2036,6 +2126,7 @@ def _run_session( seq: int, session_stage: str | None = None, label: str | None = None, + spec_snapshot: SpecSnapshot | None = None, ) -> SessionResult: # ``label`` names a non-standard session (a plugin-provided workflow) so # its task_id stays distinct from the role's own dev/review attempts. @@ -2117,6 +2208,9 @@ def _run_session( token_budget_mode=self.policy.limits.session_budget_mode, token_budget_grace_s=float(self.policy.limits.session_budget_grace_s), cache_read_weight=self.policy.limits.cache_read_weight, + # Launch-state snapshot of a review session's spec (#276 M1); None for + # every other session and on a crash-resume (process-transient). + spec_snapshot=spec_snapshot, ) self.journal.set_active_log(task_id) self.journal.append( @@ -2149,6 +2243,11 @@ def _run_session( self.journal.append( "session-synthesized-from-frontmatter", task_id=task_id, role=role ) + # #276 M3: the marker the skill owed was never appended. Repair the + # on-disk spec (best-effort) so the next re-read is harvested on the + # normal marker path. Covers live-Stop, crash-path, and post-kill + # dead-window synthesis — every path that sets this flag. + self._repair_spec_marker(task, result.result_json) # Only dev/review sessions are resumable — `_resumable_session` matches # exactly those task ids under DEV_RUNNING/REVIEW_RUNNING. For everything # else (triage/sweep, labeled plugin-workflow sessions) the payload is @@ -2295,8 +2394,9 @@ def _reset_spec_for_repair(self, task: StoryTask) -> None: devcontract.reset_spec_status(spec_path, "in-progress") devcontract.strip_auto_run_result(spec_path) - def _reset_spec_for_review(self, task: StoryTask) -> None: - """Strip the prior pass's stale `## Auto Run Result` before a review launch. + def _reset_spec_for_review(self, task: StoryTask) -> SpecSnapshot | None: + """Strip the prior pass's stale `## Auto Run Result` before a review launch, + then capture a launch-state snapshot of the spec (#276 M1). A follow-up review session re-invokes bmad-dev-auto on the FINALIZED spec, which still carries the dev pass's terminal `## Auto Run Result` section. @@ -2308,15 +2408,40 @@ def _reset_spec_for_review(self, task: StoryTask) -> None: PREVIOUS review pass's own marker, so this runs before every review launch, never on the crash-resume replay branch (no session launches there). Unlike `_reset_spec_for_repair` the frontmatter is left untouched: `status: done` is - what routes the re-invocation's step-01 to a fresh step-04 review pass. - - No-op when the dev skill is not the generic one or no spec is recorded yet. - Repair-write doctrine (see `devcontract.strip_auto_run_result`): a present- - but-unreadable spec raises rather than silently proceeding stale — skipping - the strip recreates the exact bug state, so it must surface.""" + what routes the re-invocation's step-01 to a fresh step-04 review pass — the + HARD CONSTRAINT that the review-launch frontmatter status is NEVER mutated + (it is load-bearing skill routing), so every #276 mechanism observes only. + + Returns a `SpecSnapshot` of the on-disk spec as it stood at launch — its + content hash, mtime, and normalized frontmatter status — so the generic + adapter's missing-marker fallback can refuse to synthesize from a candidate + whose bytes never changed this session (a `done` spec re-opened for review, + never re-written). Snapshot capture is best-effort: a torn/unreadable read + degrades to `None` (journaled, `review-launch-snapshot`), and the fallback + then keeps its conservative 2-observation fingerprint path. Only the capture + is guarded — the strip keeps its raise-on-unreadable repair doctrine (see + `devcontract.strip_auto_run_result`): skipping the strip recreates the exact + #160 bug state, so it must surface. + + No-op (returns `None`) when the dev skill is not the generic one or no spec + is recorded yet.""" if not self._generic_dev() or not task.spec_file: - return - devcontract.strip_auto_run_result(Path(task.spec_file)) + return None + spec_path = Path(task.spec_file) + devcontract.strip_auto_run_result(spec_path) + try: + raw = spec_path.read_bytes() + mtime_ns = spec_path.stat().st_mtime_ns + fm_status = str(verify.read_frontmatter(spec_path).get("status", "")).strip().lower() + except OSError as e: + self._journal_spec_read_failed(spec_path, task.story_key, "review-launch-snapshot", e) + return None + return SpecSnapshot( + path=str(spec_path), + mtime_ns=mtime_ns, + sha256=hashlib.sha256(raw).hexdigest(), + fm_status=fm_status, + ) def _write_feedback(self, task: StoryTask, reason: str) -> Path: """Persist a verification failure where the next session can read it — diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index 81f9f36..52cb885 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -130,6 +130,16 @@ class LimitsPolicy: # after this many total nudges (non-blocking workflows then advance the # phase). workflow_stall_nudges_cap: int = 3 + # One targeted nudge per session (#276 M4) when a Stop finds a marker-less + # terminal-frontmatter spec (a review that finalized `status: done`/`blocked` + # without appending its required `## Auto Run Result` section): ask the skill + # to append that section now, then end its turn — repairing the omission at + # the source so the normal marker scan harvests it. Sent exactly once per + # session (a never-cleared set), never refilled, and touching no stall + # counters; the harness-side frontmatter synthesis (#224) stays the backstop + # for a session that never complies. True enables it; False keeps the pre-M4 + # behavior (synthesis only). + dev_contract_nudge: bool = True max_tokens_per_story: int = 2_000_000 # weight of cache-read tokens in the budget check (1.0 = count raw) cache_read_weight: float = 0.1 @@ -707,6 +717,9 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: workflow_stall_nudges_cap=int( limits_d.get("workflow_stall_nudges_cap", LimitsPolicy.workflow_stall_nudges_cap) ), + dev_contract_nudge=bool( + limits_d.get("dev_contract_nudge", LimitsPolicy.dev_contract_nudge) + ), max_tokens_per_story=int( limits_d.get("max_tokens_per_story", LimitsPolicy.max_tokens_per_story) ), @@ -999,6 +1012,7 @@ def _fold_deprecated_engine( dev_stall_nudges = 2 # times an idle dev session is nudged awake on grace expiry before it is called stalled (bmad-loop has no background-completion re-invocation); pane output re-arms the grace window and a fresh Stop restores the budget. 0 = stall on grace expiry dev_stall_nudges_cap = 6 # total (never-restored) stall nudges for a dev/review session before it is called stalled; bounds a session whose reply to the wake nudge is itself a result-less Stop that would refill the budget forever (#149). 0 = stall on first grace expiry workflow_stall_nudges_cap = 3 # total (never-restored) stall nudges for an injected plugin-workflow session before it is called stalled; bounds a session that finished its work but never wrote its completion marker. 0 = stall on first grace expiry +dev_contract_nudge = true # true: one targeted nudge per session (#276) when a Stop finds a spec finalized to a terminal frontmatter status but missing its `## Auto Run Result` marker, asking the skill to append it and end its turn; sent exactly once, never refilled, touches no stall counters. false: rely only on harness-side frontmatter synthesis max_tokens_per_story = 2000000 cache_read_weight = 0.1 # cache reads bill at ~0.1x input on all vendors; 1.0 = count raw session_budget_mode = "warn" # off | warn | enforce — weighted per-SESSION cap sampled every ~30s mid-session; warn = one ATTENTION + breadcrumb; enforce = wrap-up nudge (best-effort courtesy; the kill is the guarantee) then over_budget termination (retry→defer). Live-verified on claude; other transcript parsers sample best-effort (mid-turn flush unverified); inert where no mid-session usage signal exists (usage_parser "none", copilot's shutdown-only flush) diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index c06b85e..a04f317 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -784,3 +784,160 @@ def test_frontmatter_candidates_skips_unreadable_file(tmp_path): def test_frontmatter_candidates_missing_dir_is_empty(tmp_path): assert devcontract.find_frontmatter_candidates(tmp_path / "nope", since_ns=0) == [] + + +# ------------------------------- append_auto_run_result (#276 M3) +# +# The artifact-repair writer: the inverse of `strip_auto_run_result`. Appends the +# `## Auto Run Result` marker a missing-marker synthesis proved the session owed, +# bringing a marker-less terminal spec back into contract. + + +def test_append_auto_run_result_minimal_shape(tmp_path): + """The appended section carries a parseable Status + the provenance note, and + leaves the spec `synthesize_result`-consistent with its unchanged frontmatter.""" + sp = _write( + tmp_path, + "spec-a.md", + "---\nstatus: done\nbaseline_revision: 'abc123'\n---\n\n## Intent\n\nbody\n", + ) + assert devcontract.append_auto_run_result(sp, "done") is True + text = sp.read_text() + arr = devcontract.parse_auto_run_result(text) + assert arr.present and arr.status == "done" + assert devcontract.ORCHESTRATOR_SYNTH_NOTE in arr.detail + # frontmatter is untouched, so the prose Status must equal it → consistent + assert "status: done\n" in text + sr = devcontract.synthesize_result(sp, story_key="1-1-a") + assert sr.status_consistent is True + assert sr.result_json is not None and sr.result_json["status"] == "done" + + +def test_append_moves_spec_to_marker_scan_territory(tmp_path): + """Before: a missing-marker candidate, invisible to the marker scan. After: + gone from the fallback scan, found by the normal `find_result_artifact`.""" + sp = _write(tmp_path, "spec-a.md", "---\nstatus: done\n---\n\nbody\n") + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [sp] + assert devcontract.find_result_artifact(tmp_path, since_ns=0) is None + + assert devcontract.append_auto_run_result(sp, "done") is True + + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] + assert devcontract.find_result_artifact(tmp_path, since_ns=0) == sp + + +def test_append_refuses_existing_real_heading(tmp_path): + """Idempotence: a real (non-fenced) marker already present blocks the append, + and the bytes are left identical.""" + original = "---\nstatus: done\n---\n\n## Auto Run Result\n\nStatus: done\n" + sp = _write(tmp_path, "spec-a.md", original) + before = sp.read_bytes() + assert devcontract.append_auto_run_result(sp, "done") is False + assert sp.read_bytes() == before + + +def test_append_missing_file_false(tmp_path): + assert devcontract.append_auto_run_result(tmp_path / "ghost.md", "done") is False + + +def test_append_raises_on_undecodable_spec(tmp_path): + """Repair-write doctrine (shared with the strip): a present-but-unreadable spec + RAISES, never no-ops — the caller imposes best-effort, not the writer.""" + sp = tmp_path / "spec-a.md" + sp.write_bytes(b"---\nstatus: done\n---\n\n\xff\xfebody\n") + with pytest.raises(UnicodeDecodeError): + devcontract.append_auto_run_result(sp, "done") + + +def test_append_over_fence_quoted_heading_appends(tmp_path): + """#52 symmetry with the strip: a heading quoted inside a fence is + documentation, not a real marker, so it does NOT block the append. Exactly one + REAL heading exists afterward — the appended one — and it parses.""" + sp = _write( + tmp_path, + "spec-a.md", + "---\nstatus: done\n---\n\n## Intent\n\n```md\n## Auto Run Result\n\nStatus: done\n```\n", + ) + assert devcontract.append_auto_run_result(sp, "done") is True + text = sp.read_text() + assert len(devcontract._section_headings(text)) == 1 + arr = devcontract.parse_auto_run_result(text) + assert arr.present and arr.status == "done" + + +def test_append_handles_missing_trailing_newline_and_crlf(tmp_path): + """A CRLF spec with no trailing newline: the heading lands on its own line + (never glued to the last body line) and the file's CRLF ending is preserved — + no bare LF is introduced into the appended block.""" + sp = tmp_path / "spec-a.md" + sp.write_bytes(b"---\r\nstatus: done\r\n---\r\n\r\n## Intent\r\n\r\nbody") + assert devcontract.append_auto_run_result(sp, "done") is True + text = sp.read_bytes().decode("utf-8") + assert "body## Auto Run Result" not in text # never glued + assert "body\r\n## Auto Run Result\r\n" in text # own line, trailing newline ensured + assert "\r\nStatus: done\r\n" in text # CRLF preserved in the block + assert "\n" not in text.replace("\r\n", "") # no bare LF introduced + + +def test_append_then_strip_roundtrip(tmp_path): + """The strip removes exactly the appended section: a spec that ended in a + newline round-trips byte-for-byte through append → strip, detail paragraph and + all.""" + original = "---\nstatus: done\n---\n\n## Intent\n\nbody\n" + sp = _write(tmp_path, "spec-a.md", original) + assert devcontract.append_auto_run_result(sp, "done", detail="extra context") is True + assert "## Auto Run Result" in sp.read_text() and "extra context" in sp.read_text() + assert devcontract.strip_auto_run_result(sp) is True + assert sp.read_text() == original + + +def test_append_bare_cr_terminated_spec_makes_heading_visible(tmp_path): + """#276: a spec ending in a BARE ``\\r`` (no ``\\n``) must not glue an invisible + heading. The append completes the CR to CRLF so ``## Auto Run Result`` lands on a + line the scan's ``^``-anchored regex recognizes: the heading parses, exactly one + real heading exists, and a second append is idempotent (proving the heading is + visible to the scan — under the old logic it was glued after ``\\r`` and unseen).""" + sp = tmp_path / "spec-a.md" + sp.write_bytes(b"---\nstatus: done\n---\n\nbody\r") + assert devcontract.append_auto_run_result(sp, "done") is True + text = sp.read_bytes().decode("utf-8") + assert len(devcontract._section_headings(text)) == 1 + arr = devcontract.parse_auto_run_result(text) + assert arr.present and arr.status == "done" + assert devcontract.append_auto_run_result(sp, "done") is False # idempotent → visible + + +@pytest.mark.parametrize( + "writer, original", + [ + ( + lambda sp: devcontract.append_auto_run_result(sp, "done"), + "---\nstatus: done\n---\n\n# Story\n\nbody\n", # no marker → append writes + ), + ( + lambda sp: devcontract.strip_auto_run_result(sp), + "---\nstatus: done\n---\n\n## Auto Run Result\n\nStatus: done\n", # marker → strip writes + ), + ( + lambda sp: devcontract.reset_spec_status(sp, "in-progress"), + "---\nstatus: done\n---\n\nbody\n", # status differs → reset writes + ), + ], + ids=["append", "strip", "reset"], +) +def test_repair_write_failure_never_truncates_spec(tmp_path, monkeypatch, writer, original): + """#276: the three in-place spec rewriters are atomic (tmp + ``atomic_replace``). + A failed rename (disk-full, interruption, short write) leaves the original spec + byte-for-byte intact with no ``.tmp`` litter — the "a failed repair never loses + work" invariant (fault injection on the old truncating write reduced a 46-byte + spec to 12).""" + sp = _write(tmp_path, "spec-a.md", original) + + def boom(tmp, target): + raise OSError("no space left on device") + + monkeypatch.setattr(devcontract, "atomic_replace", boom) + with pytest.raises(OSError, match="no space left"): + writer(sp) + assert sp.read_text(encoding="utf-8") == original # original untouched + assert list(tmp_path.glob("*.tmp")) == [] # temp file cleaned up, no litter diff --git a/tests/test_engine.py b/tests/test_engine.py index 96441a8..9ab97d3 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -1,6 +1,7 @@ """Engine scenario tests against the mock adapter — no tmux, no LLM.""" import dataclasses +import hashlib import os import re import signal @@ -1992,6 +1993,53 @@ def test_reset_spec_for_repair_strips_stale_terminal_section(project): assert "## Intent\n\nbody\n" in text # frozen intent untouched +def test_review_launch_snapshot_threaded_into_session_spec(project): + """The engine captures a launch-state SpecSnapshot right after the review + marker strip and threads it onto the review SessionSpec; the dev session that + precedes it carries none (#276 M1).""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + captured: dict[str, str] = {} + + def capturing_review(spec): + # The engine captured the snapshot just before launching this session; the + # spec on disk is still the dev pass's bytes until this effect rewrites it, + # so hashing it here recomputes exactly what the snapshot recorded. + sp = spec_path(project, "1-1-a") + captured["digest"] = hashlib.sha256(sp.read_bytes()).hexdigest() + return review_effect(project, "1-1-a", clean=True)(spec) + + engine, adapter = make_engine(project, [dev_effect(project, "1-1-a"), capturing_review]) + summary = engine.run() + + assert summary.done == 1 + dev_spec, review_spec = adapter.sessions + assert dev_spec.role == "dev" and dev_spec.spec_snapshot is None + snap = review_spec.spec_snapshot + assert snap is not None + assert snap.path == str(spec_path(project, "1-1-a")) + assert snap.fm_status == "done" + assert snap.sha256 == captured["digest"] + + +def test_review_launch_snapshot_degrades_on_unreadable_spec(project): + """A spec path that cannot be read degrades the snapshot capture to None and + journals `spec-read-failed` at site `review-launch-snapshot`. A directory where + a file is expected is the trigger: it slips past the strip's `is_file()` guard + (the strip's raise-on-unreadable doctrine is untouched), then `read_bytes` raises + IsADirectoryError, which the capture catches.""" + engine, _ = make_engine(project, []) + bad = project.implementation_artifacts / "spec-1-1-a.md" + bad.mkdir(parents=True, exist_ok=True) + task = StoryTask(story_key="1-1-a", epic=1, spec_file=str(bad)) + + snap = engine._reset_spec_for_review(task) + + assert snap is None + events = [e for e in engine.journal.entries() if e["kind"] == "spec-read-failed"] + assert events and events[-1]["site"] == "review-launch-snapshot" + assert events[-1]["story_key"] == "1-1-a" + + def test_generic_reconcile_skips_blocked_prose(project): """A blocked outcome (prose Status: blocked) is NEVER reconciled: the frontmatter stays non-terminal, no `spec-status-reconciled` is emitted, and the @@ -5767,3 +5815,173 @@ def synthesized_dev(spec): e for e in engine.journal.entries() if e["kind"] == "session-synthesized-from-frontmatter" ] assert crumb["role"] == "dev" + + +def test_synthesized_result_repairs_spec_marker(project): + """#276 M3: when the fallback synthesizes a dev result the engine appends the + marker the skill owed onto the on-disk spec — with provenance — and journals + `spec-marker-repaired`. The story still completes; verify/commit are unaffected + (the marker is prose; the frontmatter status the gates read is unchanged).""" + from bmad_loop import devcontract + + write_sprint(project, {"1-1-a": "ready-for-dev"}) + # No follow-up review, so nothing rewrites the spec after the repair lands. + inner = dev_effect(project, "1-1-a", followup_review=False) + + def synthesized_dev(spec): + result = inner(spec) + result.result_json["synthesized_from_frontmatter"] = True + result.result_json["status"] = "done" # what synthesize_result would carry + return result + + engine, _ = make_engine(project, [synthesized_dev]) + summary = engine.run() + + assert summary.done == 1 + text = spec_path(project, "1-1-a").read_text() + arr = devcontract.parse_auto_run_result(text) + assert arr.present and arr.status == "done" + assert devcontract.ORCHESTRATOR_SYNTH_NOTE in text + (repaired,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repaired"] + assert repaired["status"] == "done" + assert not [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-skipped"] + + +def test_marker_repair_failure_is_best_effort(project, monkeypatch): + """An OSError out of the append is swallowed (journaled `spec-marker-repair- + failed`): the result was already synthesized, so the story still completes — + only the on-disk spec is left non-compliant.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + inner = dev_effect(project, "1-1-a", followup_review=False) + + def synthesized_dev(spec): + result = inner(spec) + result.result_json["synthesized_from_frontmatter"] = True + result.result_json["status"] = "done" + return result + + def boom(*args, **kwargs): + raise OSError("disk full") + + monkeypatch.setattr("bmad_loop.devcontract.append_auto_run_result", boom) + engine, _ = make_engine(project, [synthesized_dev]) + summary = engine.run() + + assert summary.done == 1 + (failed,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-failed"] + assert "OSError" in failed["error"] + + +def test_synthesized_review_result_repairs_marker_and_story_completes(project): + """#276 M3, review path: when the fallback synthesizes a REVIEW result the engine + appends the marker onto the on-disk spec (with provenance) and the story still + completes. Prior engine coverage only exercised a synthesized DEV result with no + follow-up review — this covers the primary review-repair path.""" + from bmad_loop import devcontract + + write_sprint(project, {"1-1-a": "ready-for-dev"}) + dev = dev_effect(project, "1-1-a", followup_review=True) + review = review_effect(project, "1-1-a", clean=True) + + def synthesized_review(spec): + result = review(spec) + result.result_json["synthesized_from_frontmatter"] = True + result.result_json["status"] = "done" + return result + + engine, _ = make_engine(project, [dev, synthesized_review]) + summary = engine.run() + + assert summary.done == 1 + text = spec_path(project, "1-1-a").read_text() + arr = devcontract.parse_auto_run_result(text) + assert arr.present and arr.status == "done" + assert devcontract.ORCHESTRATOR_SYNTH_NOTE in text + (repaired,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repaired"] + assert repaired["status"] == "done" + synth = [ + e for e in engine.journal.entries() if e["kind"] == "session-synthesized-from-frontmatter" + ] + assert [e["role"] for e in synth] == ["review"] # the REVIEW session synthesized + + +def test_synthesized_review_repair_survives_followup_rewrite(project): + """The finding's core case: a synthesized review cycle repairs the marker, then a + subsequent clean review cycle rewrites the whole spec (dropping the marker). The + story still converges to done, the run does not livelock, and exactly one repair + fired (the synthesized cycle) — the repair never fights the follow-up rewrite.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + dev = dev_effect(project, "1-1-a", followup_review=True) + review1 = review_effect(project, "1-1-a", clean=False) # recommends another review + + def synthesized_review1(spec): + result = review1(spec) + result.result_json["synthesized_from_frontmatter"] = True + result.result_json["status"] = "done" + return result + + review2 = review_effect(project, "1-1-a", clean=True) # converges; rewrites the spec + engine, _ = make_engine(project, [dev, synthesized_review1, review2]) + summary = engine.run() + + assert summary.done == 1 + repaired = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repaired"] + assert len(repaired) == 1 # only the synthesized cycle repaired + + +def test_marker_repair_undecodable_spec_is_best_effort(project, monkeypatch): + """`append_auto_run_result` reads raw bytes and raises `UnicodeDecodeError` + (a `ValueError`, not an `OSError`) on an undecodable spec — a spec torn mid- + write through a multi-byte UTF-8 sequence between the frontmatter read and the + append. The best-effort repair must swallow it too (journaled + `spec-marker-repair-failed`) so the run still completes rather than crashing.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + inner = dev_effect(project, "1-1-a", followup_review=False) + + def synthesized_dev(spec): + result = inner(spec) + result.result_json["synthesized_from_frontmatter"] = True + result.result_json["status"] = "done" + return result + + def boom(*args, **kwargs): + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + + monkeypatch.setattr("bmad_loop.devcontract.append_auto_run_result", boom) + engine, _ = make_engine(project, [synthesized_dev]) + summary = engine.run() + + assert summary.done == 1 + (failed,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-failed"] + assert "UnicodeDecodeError" in failed["error"] + + +def test_marker_repair_skips_out_of_tree_spec(project, tmp_path): + """A session-reported spec_file outside the orchestrator-owned roots is never + written — the repair skips with reason `out-of-tree`, like the reconcile.""" + engine, _ = make_engine(project, []) + outside = tmp_path / "outside-spec.md" # sibling of the sandbox root, not under it + outside.write_text("---\nstatus: done\n---\n\nbody\n", encoding="utf-8") + task = StoryTask(story_key="1-1-a", epic=1) + + engine._repair_spec_marker(task, {"spec_file": str(outside), "status": "done"}) + + (skipped,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-skipped"] + assert skipped["reason"] == "out-of-tree" + assert "## Auto Run Result" not in outside.read_text() + + +def test_marker_repair_skips_on_fm_mismatch(project): + """A fresh frontmatter read that no longer agrees with the synthesized status + (here still `in-progress` while `rj` claims `done`) is refused with reason + `fm-mismatch` and no marker is appended — never author an inconsistent one.""" + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-progress", "abc123") + task = StoryTask(story_key="1-1-a", epic=1, spec_file=str(sp)) + + engine._repair_spec_marker(task, {"spec_file": str(sp), "status": "done"}) + + (skipped,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-skipped"] + assert skipped["reason"] == "fm-mismatch" + assert "## Auto Run Result" not in sp.read_text() diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index d3dde99..5c12462 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -8,6 +8,7 @@ """ import dataclasses +import hashlib import json import os import shutil @@ -21,7 +22,7 @@ import regex from bmad_loop.adapters import generic, tmux_base -from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec +from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec, SpecSnapshot from bmad_loop.adapters.generic import GenericDevAdapter, GenericTmuxAdapter from bmad_loop.adapters.multiplexer import MultiplexerError from bmad_loop.adapters.profile import get_profile @@ -475,7 +476,7 @@ def test_kill_grace_zero_is_the_legacy_single_strike(tmp_path): # Stop event, via devcontract. These exercise that override in isolation. -def make_dev_adapter(tmp_path, profile_name="claude"): +def make_dev_adapter(tmp_path, profile_name="claude", policy=None): impl = tmp_path / "impl" impl.mkdir() # project root == tmp_path so rebased(spec.cwd=tmp_path) is a no-op: these @@ -487,7 +488,7 @@ def make_dev_adapter(tmp_path, profile_name="claude"): ) adapter = GenericDevAdapter( run_dir=tmp_path / "run", - policy=Policy(limits=LimitsPolicy()), + policy=policy or Policy(limits=LimitsPolicy()), profile=get_profile(profile_name), paths=paths, ) @@ -2954,6 +2955,119 @@ def test_tmux_timeout_with_flushed_spec_rescued_post_kill(tmp_path): _MARKERLESS_BLOCKED = "---\nstatus: blocked\n---\n\n# Story\n\nStuck.\n" +def _snap(spec_file: Path) -> SpecSnapshot: + """A review-launch snapshot of an on-disk spec, exactly as + ``_reset_spec_for_review`` captures it: hash + mtime + normalized fm status.""" + raw = spec_file.read_bytes() + return SpecSnapshot( + path=str(spec_file), + mtime_ns=spec_file.stat().st_mtime_ns, + sha256=hashlib.sha256(raw).hexdigest(), + fm_status="done", + ) + + +def _snapshotted_spec(tmp_path, spec_file: Path, story_key="3-1") -> SessionSpec: + """A review SessionSpec carrying the launch snapshot of ``spec_file`` (#276 M1).""" + return dataclasses.replace( + _dev_spec(tmp_path, story_key), role="review", spec_snapshot=_snap(spec_file) + ) + + +def _snapshotted_stories_spec(tmp_path, story_spec: Path, story_key="1") -> SessionSpec: + """A review-role stories SessionSpec carrying the launch snapshot of its story + spec (#276 M1/M2), as the engine threads for a stories-mode review leg.""" + return dataclasses.replace( + _stories_spec(tmp_path, story_key), role="review", spec_snapshot=_snap(story_spec) + ) + + +def test_stories_readback_refuses_unmodified_snapshot_no_transition(tmp_path, monkeypatch): + """#276 M1 on the stories read-back: a `done` story spec byte-identical to its + review-launch snapshot with no transition observed is the dead-window false + positive (a review that only bumped the mtime). The gate REFUSES synthesis + (`unmodified-since-launch`) instead of accepting on the mtime floor alone.""" + adapter, _ = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + story = _write_story_spec(tmp_path, "1", "foo", _MARKERLESS_DONE) + spec = _snapshotted_stories_spec(tmp_path, story) # snapshot == current bytes + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + (crumb,) = _breadcrumbs(adapter) # keyed by handle.task_id (3-1-dev-1) + assert crumb["verdict"] == "unmodified-since-launch" + + +def test_stories_readback_transition_beats_snapshot(tmp_path, monkeypatch): + """M2 outranks M1 on the stories path too: with a recorded transition the same + byte-identical `done` spec synthesizes (the review provably ran).""" + adapter, _ = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + story = _write_story_spec(tmp_path, "1", "foo", _MARKERLESS_DONE) + spec = _snapshotted_stories_spec(tmp_path, story) + adapter._fm_transition_obs["3-1-dev-1"] = "in-review" # keyed by handle.task_id + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj is not None and rj["status"] == "done" + assert _breadcrumbs(adapter) == [] + + +def test_stories_readback_changed_bytes_synthesizes(tmp_path, monkeypatch): + """Over-refusal guard: when the spec's bytes differ from the launch snapshot the + gate is NEUTRAL and the read-back synthesizes as before.""" + adapter, _ = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + story = _write_story_spec(tmp_path, "1", "foo", _MARKERLESS_DONE) + spec = _snapshotted_stories_spec(tmp_path, story) # snapshot of the original bytes + story.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nReviewed and done.\n", + encoding="utf-8", + ) # this session actually rewrote the spec + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj is not None and rj["status"] == "done" + + +def test_stories_readback_dev_leg_no_snapshot_accepts(tmp_path, monkeypatch): + """Inert on a dev leg: no launch snapshot → NEUTRAL → the unchanged mtime-floor + accept, even for a spec that would be byte-identical to some snapshot.""" + adapter, _ = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + _write_story_spec(tmp_path, "1", "foo", _MARKERLESS_DONE) + rj = adapter._result_json(_dev_handle(), _stories_spec(tmp_path), wait=True) # snap None + assert rj is not None and rj["status"] == "done" + + +def test_stories_readback_snapshot_other_path_inert(tmp_path, monkeypatch): + """The gate only bites when the resolved story spec IS the snapshotted file. A + snapshot for a different path leaves the read-back on its normal accept.""" + adapter, _ = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + story = _write_story_spec(tmp_path, "1", "foo", _MARKERLESS_DONE) + other = tmp_path / "epic" / "stories" / "9-other.md" + snap = dataclasses.replace(_snap(story), path=str(other)) + spec = dataclasses.replace(_stories_spec(tmp_path), role="review", spec_snapshot=snap) + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj is not None and rj["status"] == "done" + + +def test_snapshot_verdict_truth_table(tmp_path): + """Direct unit test of the shared M1/M2 decision — the anti-drift guard both the + scan fallback and the stories read-back route through.""" + adapter, _ = make_dev_adapter(tmp_path) + snap = SpecSnapshot(path="/x/spec.md", mtime_ns=1, sha256="deadbeef", fm_status="done") + decide = adapter._snapshot_verdict + V = generic._SnapVerdict + # no snapshot / different file → NEUTRAL regardless of digest + assert decide(same_file=False, snap=None, task_id="t", digest="deadbeef") is V.NEUTRAL + assert decide(same_file=False, snap=snap, task_id="t", digest="deadbeef") is V.NEUTRAL + # matching digest, no transition → REFUSE + assert decide(same_file=True, snap=snap, task_id="t", digest="deadbeef") is V.REFUSE + # transition observed → PROVEN, even with a matching digest + adapter._fm_transition_obs["t"] = "in-review" + assert decide(same_file=True, snap=snap, task_id="t", digest="deadbeef") is V.PROVEN + # mismatched digest, no transition → NEUTRAL + assert decide(same_file=True, snap=snap, task_id="u", digest="feed") is V.NEUTRAL + # digest None (unreadable) → NEUTRAL + assert decide(same_file=True, snap=snap, task_id="u", digest=None) is V.NEUTRAL + + def test_frontmatter_fallback_synthesizes_on_second_stable_stop(tmp_path, monkeypatch): adapter, impl = make_dev_adapter(tmp_path) monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) @@ -3132,3 +3246,539 @@ def test_wait_loop_completes_markerless_spec_on_second_stop(tmp_path, monkeypatc assert result.status == "completed" assert result.result_json["status"] == "done" assert result.result_json["synthesized_from_frontmatter"] is True + + +# ---------------------------- launch-state snapshot + content-hash gate (#276 M1) +# +# A review session inherits the dev pass's `done` spec. When the review is killed +# after an mtime-only bump but before it flips the frontmatter to `in-review`, the +# #224 fallback would score `done` without the review ever running. The engine now +# threads a SpecSnapshot (hash + mtime + fm status) captured at review launch; the +# fallback refuses to synthesize from a candidate still byte-identical to it — in +# every mode, live or dead-window. + + +def test_frontmatter_fallback_refuses_unmodified_snapshot_live(tmp_path, monkeypatch): + """A byte-identical spec is provably untouched by this session: no wait=True + pass ever harvests, every verdict is `unmodified-since-launch`, and the + fingerprint dict stays empty (no observation recorded or popped).""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + spec = _snapshotted_spec(tmp_path, spec_file) + for _ in range(3): + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + verdicts = {c["verdict"] for c in _breadcrumbs(adapter)} + assert verdicts == {"unmodified-since-launch"} + assert "byte-identical to review-launch snapshot" in _breadcrumbs(adapter)[0]["detail"] + assert adapter._fm_fallback_obs == {} + + +def test_frontmatter_fallback_refuses_unmodified_snapshot_dead_window(tmp_path): + """The kill shot: a dead window over a markerless `done` spec whose only change + since launch is an mtime bump. The single-sighting post-kill rescue must refuse + — returning the ORIGINAL result — and leave the dead-window lifecycle crumb.""" + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + spec = _snapshotted_spec(tmp_path, spec_file) + # mtime-only bump: bytes unchanged, so the hash still matches the snapshot. + os.utime(spec_file, ns=(spec.spec_snapshot.mtime_ns + 10**9,) * 2) + original = _unvouched() + assert adapter._post_kill_reconcile(_dev_handle(), spec, original) is original + events = [ln["event"] for ln in _lifecycle_lines(adapter)] + assert "frontmatter-unmodified-refused" in events + # a refusal is not a resultless-Stop breadcrumb (that channel is wait=True only) + assert _breadcrumbs(adapter) == [] + + +def test_frontmatter_fallback_hash_mismatch_still_two_obs(tmp_path, monkeypatch): + """Content edited since the snapshot (the review actually wrote): the hash gate + is inert and today's fingerprint behavior stands — pending on the first stable + Stop, harvested on the second.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nLaunch.\n" + ) + spec = _snapshotted_spec(tmp_path, spec_file) + # the review rewrote the spec's body: bytes differ from the launch snapshot. + spec_file.write_text(_MARKERLESS_DONE) + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "terminal-frontmatter-pending" + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj["synthesized_from_frontmatter"] is True + + +def test_frontmatter_fallback_snapshot_other_path_inert(tmp_path, monkeypatch): + """The gate only fires when the candidate IS the snapshotted spec. A snapshot + for a different path leaves the fallback on its normal fingerprint path even + when the candidate happens to be byte-identical to that snapshot's content.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + snap = dataclasses.replace(_snap(spec_file), path=str(impl / "spec-9-9-other.md")) + spec = dataclasses.replace(_dev_spec(tmp_path), role="review", spec_snapshot=snap) + # not the snapshotted path -> no hash comparison -> ordinary 2-obs harvest + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "terminal-frontmatter-pending" + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj["synthesized_from_frontmatter"] is True + + +def test_frontmatter_fallback_unmodified_wait_false_silent(tmp_path, monkeypatch): + """The plain crash path (wait=False, live window) is compare-only over an + unmodified spec: it refuses to synthesize and leaves zero crumbs of either + kind.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + spec = _snapshotted_spec(tmp_path, spec_file) + assert adapter._result_json(_dev_handle(), spec, wait=False) is None + assert _breadcrumbs(adapter) == [] + assert _lifecycle_lines(adapter) == [] + assert adapter._fm_fallback_obs == {} + + +# ------------------------- mid-session status-transition observation (#276 M2) +# +# The engine threads a launch snapshot whose fm_status is the review's `done` +# (re-opened). On each heartbeat tick the dev adapter samples the snapshotted +# spec and records the FIRST non-terminal status it observes this session drive +# it to (in practice `in-review`). That single sighting makes a later terminal +# frontmatter deterministic proof THIS session wrote it, so the fallback +# harvests on one sighting — but the M1 hash gate still outranks it. + + +def _transitioned_spec(tmp_path, spec_file: Path, live_status: str = "in-review") -> SessionSpec: + """A review spec whose launch snapshot carries `done` but whose on-disk file + now sits at `live_status` — the state a mid-session tick observes.""" + spec_file.write_text(_MARKERLESS_DONE) # launch state: terminal `done` + spec = _snapshotted_spec(tmp_path, spec_file) + spec_file.write_text(f"---\nstatus: {live_status}\n---\n\n# Story\n\nReviewing.\n") + return spec + + +def _lifecycle_events(adapter, event, task_id="3-1-dev-1"): + """Lifecycle breadcrumbs of one `event` kind — the shared filter behind the + per-event helpers (`spec-status-transition-observed`, `contract-nudge-sent`).""" + return [ln for ln in _lifecycle_lines(adapter, task_id) if ln["event"] == event] + + +def test_observe_tick_records_first_transition(tmp_path): + """The first observed non-terminal, non-launch status is recorded once with a + single `spec-status-transition-observed` crumb; a second tick — even a + different non-terminal status — neither overwrites the record nor re-crumbs.""" + adapter, impl = make_dev_adapter(tmp_path) + spec_file = impl / "spec-3-1-foo.md" + spec = _transitioned_spec(tmp_path, spec_file) # on disk: in-review + + adapter._observe_tick(_dev_handle(), spec) + assert adapter._fm_transition_obs == {"3-1-dev-1": "in-review"} + (crumb,) = _lifecycle_events(adapter, "spec-status-transition-observed") + assert crumb["status"] == "in-review" + assert crumb["spec"] == str(spec_file) + + # a later tick at a different non-terminal status must not overwrite or re-crumb + spec_file.write_text("---\nstatus: in-progress\n---\n\n# Story\n\nStill.\n") + adapter._observe_tick(_dev_handle(), spec) + assert adapter._fm_transition_obs == {"3-1-dev-1": "in-review"} + assert len(_lifecycle_events(adapter, "spec-status-transition-observed")) == 1 + + +@pytest.mark.parametrize( + "on_disk, launch_status", + [ + ("done", "done"), # terminal — and this review's own launch status + ("blocked", "done"), # terminal — belongs to the Stop harvest + ("", "done"), # blank/torn parse — not evidence of anything + ("in-review", "in-review"), # equals the launch status — not a transition + ], +) +def test_observe_tick_ignores_terminal_blank_and_launch_status(tmp_path, on_disk, launch_status): + """A tick records nothing when the on-disk status is terminal (`done`/ + `blocked`), a blank/torn parse, or unchanged from the snapshot's launch + status — only a live, non-terminal transition off the launch state counts.""" + adapter, impl = make_dev_adapter(tmp_path) + spec_file = impl / "spec-3-1-foo.md" + if on_disk == "": + spec_file.write_text("# Story\n\nno frontmatter\n") # parses to status "" + else: + spec_file.write_text(f"---\nstatus: {on_disk}\n---\n\n# Story\n\nbody\n") + snap = dataclasses.replace(_snap(spec_file), fm_status=launch_status) + spec = dataclasses.replace(_dev_spec(tmp_path), role="review", spec_snapshot=snap) + + adapter._observe_tick(_dev_handle(), spec) + + assert adapter._fm_transition_obs == {} + assert _lifecycle_events(adapter, "spec-status-transition-observed") == [] + + +def test_observe_tick_without_snapshot_or_unreadable_is_noop(tmp_path, monkeypatch): + """No snapshot (every non-review session) is a pure no-op, and a torn/ + unreadable snapshot read (OSError) is a skipped sample — never a verdict — + so neither records a transition or a crumb.""" + adapter, impl = make_dev_adapter(tmp_path) + + # (a) no snapshot at all: returns before reading anything + adapter._observe_tick(_dev_handle(), _dev_spec(tmp_path)) + assert adapter._fm_transition_obs == {} + + # (b) a snapshot whose path raises on read: the sampling path swallows OSError + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text("---\nstatus: in-review\n---\n\n# Story\n\nx\n") + spec = _snapshotted_spec(tmp_path, spec_file) + + def boom(_path): + raise OSError("torn read") + + monkeypatch.setattr(generic, "read_frontmatter", boom) + adapter._observe_tick(_dev_handle(), spec) + assert adapter._fm_transition_obs == {} + assert _lifecycle_lines(adapter) == [] + + +def test_frontmatter_fallback_transition_single_sighting_harvest(tmp_path, monkeypatch): + """A recorded transition + a terminal frontmatter whose bytes differ from the + snapshot: the FIRST wait=True pass harvests (no 2-obs wait), the result is + marked synthesized, the synth crumb carries transition=True, and no + `terminal-frontmatter-pending` breadcrumb is left.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nLaunch.\n" + ) + spec = _snapshotted_spec(tmp_path, spec_file) # snapshot of the launch bytes + # the review actually ran: terminal spec written, bytes differ from launch + spec_file.write_text(_MARKERLESS_DONE) + adapter._fm_transition_obs["3-1-dev-1"] = "in-review" + + rj = adapter._result_json(_dev_handle(), spec, wait=True) + + assert rj is not None and rj["synthesized_from_frontmatter"] is True + assert _breadcrumbs(adapter) == [] # single-sighting: no pending crumb + synth = [ln for ln in _lifecycle_lines(adapter) if ln["event"] == "frontmatter-synthesized"] + assert len(synth) == 1 and synth[0]["transition"] is True + + +def test_frontmatter_fallback_missed_transition_stays_conservative(tmp_path, monkeypatch): + """Snapshot present, bytes differ, but NO transition was recorded (the flip + happened between ticks): the fallback keeps the conservative 2-observation + fingerprint — pending on the first stable Stop, harvested on the second with + transition=False.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nLaunch.\n" + ) + spec = _snapshotted_spec(tmp_path, spec_file) + spec_file.write_text(_MARKERLESS_DONE) # bytes differ; transition unrecorded + assert adapter._fm_transition_obs == {} + + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "terminal-frontmatter-pending" + + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj["synthesized_from_frontmatter"] is True + synth = [ln for ln in _lifecycle_lines(adapter) if ln["event"] == "frontmatter-synthesized"] + assert len(synth) == 1 and synth[0]["transition"] is False + + +def test_transition_beats_hash_gate(tmp_path, monkeypatch): + """A recorded transition (#276 M2) OUTRANKS the M1 hash gate: a clean review that + round-tripped `done -> in-review -> done` back to byte-identical launch bytes + while omitting its marker still harvests — the observed transition proves it ran. + No `unmodified-since-launch` refusal; one `frontmatter-synthesized` crumb marked + transition=True.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + spec = _snapshotted_spec(tmp_path, spec_file) # snapshot == current bytes + adapter._fm_transition_obs["3-1-dev-1"] = "in-review" # a transition WAS seen + + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj["synthesized_from_frontmatter"] is True + assert _breadcrumbs(adapter) == [] # a harvest leaves no give-up breadcrumb + synth = [ln for ln in _lifecycle_lines(adapter) if ln["event"] == "frontmatter-synthesized"] + assert len(synth) == 1 and synth[0]["transition"] is True + + +def test_frontmatter_fallback_alias_path_recognized_as_same_spec(tmp_path, monkeypatch): + """#5: snapshot/candidate identity is filesystem-based, not lexical. A snapshot + recorded under a non-normalized alias of the candidate (a `..` detour to the same + file) is still recognized as the same spec, so the M1 hash gate fires on a + byte-identical unmodified spec instead of wrongly harvesting (a raw string compare + would miss the alias).""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + alias = impl / ".." / impl.name / "spec-3-1-foo.md" # same file, different spelling + assert alias.resolve() == spec_file.resolve() + snap = dataclasses.replace(_snap(spec_file), path=str(alias)) + spec = dataclasses.replace(_dev_spec(tmp_path), role="review", spec_snapshot=snap) + + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "unmodified-since-launch" + + +def test_wait_loop_heartbeat_drives_observe_tick(tmp_path, monkeypatch): + """The wait loop invokes _observe_tick inside the heartbeat-throttled block: + the first tick always fires (last_heartbeat is None) and each later tick a + HEARTBEAT_INTERVAL_S apart fires again, always with the session's handle.""" + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + monkeypatch.setattr(generic, "RESULT_POLL_S", 0.0) + adapter, _ = make_dev_adapter(tmp_path) + adapter._stall_grace_s = 100.0 # let several idle ticks pass before the stall + adapter._stall_nudges = 0 + adapter._window_alive = lambda handle: True + + observed: list[str] = [] + monkeypatch.setattr( + adapter, "_observe_tick", lambda handle, spec: observed.append(handle.task_id) + ) + + clock = {"t": 1000.0} + + class _Clock: + monotonic = staticmethod(lambda: clock["t"]) + time = staticmethod(lambda: 0.0) # frozen wall clock: the co-bound never fires + sleep = staticmethod(lambda *_: None) + time_ns = staticmethod(lambda: 0) + + monkeypatch.setattr(generic, "time", _Clock) + + def advance(call_n): + if call_n >= 2: # each idle tick crosses one HEARTBEAT_INTERVAL_S (30s) + clock["t"] += 31.0 + + adapter.watcher = _ScriptedWatcher( + [_stop_event("3-1-dev-1", "sess", "/run/events.jsonl")], # then None forever + on_call=advance, + ) + result = adapter.wait_for_completion(_dev_handle(), _dev_spec(tmp_path)) + + assert result.status == "stalled" + # the first tick plus at least one later heartbeat crossing fired the hook + assert len(observed) >= 2 and all(tid == "3-1-dev-1" for tid in observed) + + +# ------------------------------ contract nudge (#276 M4) +# +# On the FIRST `terminal-frontmatter-pending` observation (a Stop that found the +# spec finalized to a terminal frontmatter status but missing its `## Auto Run +# Result` marker), the dev adapter sends ONE targeted nudge asking the skill to +# append the section it owed — repairing the omission at the source. Sent exactly +# once per session via a never-cleared set (marked before the send), gated by +# `limits.dev_contract_nudge`, and touching no stall counters. Frontmatter +# synthesis stays the backstop for a session that never complies. + + +def _record_sent(adapter): + sent: list[str] = [] + adapter.send_text = lambda handle, text: sent.append(text) + return sent + + +def test_contract_nudge_sent_on_first_pending_observation(tmp_path, monkeypatch): + """The first pending observation sends exactly the formatted nudge, journals a + `contract-nudge-sent` crumb, and still records the `terminal-frontmatter- + pending` verdict — the nudge is additive, not a replacement for the fallback.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + + assert sent == [generic.CONTRACT_NUDGE_TEXT.format(spec_path=spec_file, status="done")] + (crumb,) = _lifecycle_events(adapter, "contract-nudge-sent") + assert crumb["spec"] == str(spec_file) and crumb["status"] == "done" + (verdict,) = _breadcrumbs(adapter) + assert verdict["verdict"] == "terminal-frontmatter-pending" + assert adapter._contract_nudge_sent == {"3-1-dev-1"} + + +def test_contract_nudge_exactly_once_across_mtime_reset(tmp_path, monkeypatch): + """#149's refill hazard cannot apply: an mtime bump resets the observation + counter to 1, but the nudge budget is the never-cleared set, so a second + first-observation Stop over the same session sends nothing more.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + os.utime(spec_file, ns=(1_000_000_000, 1_000_000_000)) + + # first pending observation -> one nudge + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + assert len(sent) == 1 + # the session wrote again: fingerprint changes, observation count resets to 1 + os.utime(spec_file, ns=(2_000_000_000, 2_000_000_000)) + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + + assert len(sent) == 1 # not re-nudged despite observations back at 1 + assert len(_lifecycle_events(adapter, "contract-nudge-sent")) == 1 + + +def test_contract_nudge_not_sent_when_transition_proven(tmp_path, monkeypatch): + """A recorded mid-session transition (#276 M2) harvests on the first sighting, + so the record-obs branch — and its nudge — is never reached.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nLaunch.\n" + ) + spec = _snapshotted_spec(tmp_path, spec_file) + spec_file.write_text(_MARKERLESS_DONE) # bytes differ from the launch snapshot + adapter._fm_transition_obs["3-1-dev-1"] = "in-review" + + rj = adapter._result_json(_dev_handle(), spec, wait=True) + + assert rj is not None and rj["synthesized_from_frontmatter"] is True + assert sent == [] + assert _lifecycle_events(adapter, "contract-nudge-sent") == [] + + +def test_contract_nudge_not_sent_on_ambiguous(tmp_path, monkeypatch): + """Several marker-less candidates refuse to guess before the fingerprint/nudge + branch, so no nudge is sent.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + (impl / "spec-3-2-bar.md").write_text(_MARKERLESS_DONE) + + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "ambiguous-frontmatter" + assert sent == [] + assert _lifecycle_events(adapter, "contract-nudge-sent") == [] + + +def test_contract_nudge_not_sent_on_dead_window(tmp_path): + """A dead-window post-kill reconcile synthesizes on a single sighting via the + wait=False path, which never reaches the nudge (wait=True only).""" + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + sent = _record_sent(adapter) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + + result = adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), _unvouched()) + + assert result.status == "completed" # synthesized on the dead window + assert sent == [] + assert _lifecycle_events(adapter, "contract-nudge-sent") == [] + + +def test_contract_nudge_not_sent_on_unmodified_refusal(tmp_path, monkeypatch): + """The M1 hash gate refuses a byte-identical spec before the fingerprint/nudge + branch: a spec provably untouched by this session is not this session's to + repair, so no nudge fires.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + spec = _snapshotted_spec(tmp_path, spec_file) # snapshot == current bytes + + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "unmodified-since-launch" + assert sent == [] + assert _lifecycle_events(adapter, "contract-nudge-sent") == [] + + +def test_contract_nudge_send_failure_marks_sent(tmp_path, monkeypatch): + """A raising transport still satisfies exactly-once: the task is marked (and + the crumb journaled) BEFORE the send, so a `MultiplexerError` is swallowed and + the next Stop attempts no retry.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + calls = {"n": 0} + + def boom(handle, text): + calls["n"] += 1 + raise generic.MultiplexerError("transport down") + + adapter.send_text = boom + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + + # first observation: nudge attempted, raises, swallowed (no crash) + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + assert calls["n"] == 1 + assert adapter._contract_nudge_sent == {"3-1-dev-1"} + assert len(_lifecycle_events(adapter, "contract-nudge-sent")) == 1 # marked before the send + + # second stable Stop: task already marked -> no retry, and it harvests + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["synthesized_from_frontmatter"] is True + assert calls["n"] == 1 # never re-sent + + +def test_contract_nudge_disabled_by_policy(tmp_path, monkeypatch): + """Knob off: no send, no `contract-nudge-sent` crumb, and the fallback behaves + exactly as it did pre-M4 (pending crumb recorded, harvest on the second Stop).""" + adapter, impl = make_dev_adapter( + tmp_path, policy=Policy(limits=LimitsPolicy(dev_contract_nudge=False)) + ) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + assert sent == [] + assert _lifecycle_events(adapter, "contract-nudge-sent") == [] + assert adapter._contract_nudge_sent == set() + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "terminal-frontmatter-pending" + # pre-M4 behavior intact: harvest on the second stable Stop + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["synthesized_from_frontmatter"] is True + + +def test_wait_loop_contract_nudge_then_skill_appends_marker(tmp_path, monkeypatch): + """End to end: Stop 1 finds a marker-less terminal spec and nudges; the skill + complies by appending its `## Auto Run Result` section before Stop 2; Stop 2 + then completes via the NORMAL marker scan — no synthesis, no + `synthesized_from_frontmatter` flag, and no second nudge.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + sent = _record_sent(adapter) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + + # Stop 1: marker-less -> pending + one nudge + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + assert sent == [generic.CONTRACT_NUDGE_TEXT.format(spec_path=spec_file, status="done")] + + # the skill heeds the nudge and appends its required marker section + spec_file.write_text( + "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nAll done.\n\n" + "## Auto Run Result\n\nStatus: done\nImplemented the thing.\n" + ) + + # Stop 2: harvested by the ordinary scan, not the fallback + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["status"] == "done" + assert "synthesized_from_frontmatter" not in rj + assert len(sent) == 1 # no second nudge diff --git a/tests/test_opencode_http.py b/tests/test_opencode_http.py index 292f247..52d8688 100644 --- a/tests/test_opencode_http.py +++ b/tests/test_opencode_http.py @@ -1499,3 +1499,25 @@ def test_e2e_dev_post_kill_rescue(tmp_path, fake_opencode): assert result.result_json["status"] == "done" assert result.result_json["post_kill_reconciled"] is True assert_server_gone(rec) + + +def test_e2e_dev_wait_loop_drives_observe_tick(tmp_path, fake_opencode, monkeypatch): + """Cross-adapter parity (#276 M2): the OpenCode wait loop invokes _observe_tick + from its heartbeat-throttled block, exactly as the generic adapter does. The + first tick always fires (last_heartbeat is None), so any run drives the hook — + with this session's handle and the very spec that was dispatched. OpencodeDev- + Adapter shares _DevSynthesisMixin, so the observation seam is live over HTTP.""" + launcher, rec = fake_opencode + adapter, impl = make_dev_adapter(tmp_path, binary=str(launcher)) + spec = make_dev_spec(tmp_path, rec, "completed", impl / "spec-3-1-foo.md") + + seen: list[tuple[str, bool]] = [] + monkeypatch.setattr( + adapter, "_observe_tick", lambda handle, s: seen.append((handle.task_id, s is spec)) + ) + + result = adapter.run(spec) + + assert result.status == "completed" + assert seen and all(tid == "3-1-dev-1" and same for tid, same in seen) + assert_server_gone(rec) diff --git a/tests/test_policy.py b/tests/test_policy.py index 86f39c8..d3515cb 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -389,6 +389,18 @@ def test_dev_stall_nudges_cap_default_parse_and_template(): policy.loads("[limits]\ndev_stall_nudges_cap = -1\n") +def test_dev_contract_nudge_default_parse_and_template(): + import tomllib + + # defaults on; overridable to false (no range validation — it is a bool) + assert policy.loads("").limits.dev_contract_nudge is True + loaded = policy.loads("[limits]\ndev_contract_nudge = false\n") + assert loaded.limits.dev_contract_nudge is False + # the emitted template documents the knob at its dataclass default + doc = tomllib.loads(policy.POLICY_TEMPLATE) + assert doc["limits"]["dev_contract_nudge"] == policy.LimitsPolicy.dev_contract_nudge + + def test_session_budget_mode_default_parse_and_template(): import tomllib