From 75c38ba35ac75a6b3fd3133a9ee74db0185b15fb Mon Sep 17 00:00:00 2001 From: t Date: Mon, 26 Oct 2026 12:01:03 -0700 Subject: [PATCH 1/3] fix(operator): record a parked story's follow-up review recommendation A park skips the review loop, so a follow-up review the build session recommended never runs, and the flag survived only in state.json. The `story-awaiting-operator` journal row and the committed park record now carry `followup_review_recommended` (the dev pass's, or the last review pass's for a review-demotion park), `ParkedStory` reads it back (None for a record that predates the field or holds a non-bool, never False), and the park notification tells the human to run the pass after confirming. The park short-circuit is unchanged: a park is still never reviewed, and nothing goes to the deferred-work ledger. --- src/bmad_loop/engine.py | 20 ++++- src/bmad_loop/operatoractions.py | 18 +++- tests/test_engine.py | 142 +++++++++++++++++++++++++++++-- tests/test_operatoractions.py | 34 +++++++- tests/test_tickets_engine.py | 36 +++++++- 5 files changed, 238 insertions(+), 12 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 40a2c3121..5a44e89d3 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -4899,12 +4899,16 @@ def validate_committed_publication(revision: str, staged_snapshot: object) -> No advance(task, Phase.AWAITING_OPERATOR) # The durable record of what a human owes: the actions themselves, # not just a count, because the journal and the spec frontmatter are - # what survive the run. + # what survive the run. The follow-up review recommendation is the + # dev pass's (a dev park) or the last review pass's (a review-demotion + # park); a park never runs the recommended pass, so this row and the + # park record are where it survives. self.journal.append( "story-awaiting-operator", story_key=task.story_key, commit=task.commit_sha, actions=list(task.operator_actions), + followup_review_recommended=task.followup_review_recommended, ) self._notify_park(task) else: @@ -5055,6 +5059,7 @@ def _write_park_record(self, task: StoryTask) -> tuple[Path, str | None] | None: run_id=self.state.run_id, parked_at=self._today(), root_identity=self._mount_root_identity(task, self.workspace.paths.project), + followup_review_recommended=task.followup_review_recommended, **self._park_record_fields(task), ) except (OSError, RuntimeError) as e: @@ -5174,16 +5179,27 @@ def _notify_park(self, task: StoryTask) -> None: Each action is agent-authored text, so it is folded through ``gates.notice_line`` (DW-417): an embedded line break stays on its own - numbered line as a `` ⏎ `` segment instead of landing loose in ATTENTION.""" + numbered line as a `` ⏎ `` segment instead of landing loose in ATTENTION. + + When the build session recommended a follow-up review, a line before the + confirm hint says so: a park skips that pass, so the human runs it after + confirming.""" actions = "\n".join( f" {i}. {gates.notice_line(a)}" for i, a in enumerate(task.operator_actions, 1) ) + followup = ( + "the build session also recommended a follow-up review, which a park skips: " + "after confirming, re-run the build skill on the spec for that pass.\n" + if task.followup_review_recommended + else "" + ) gates.notify( self.policy, self.run_dir, f"story awaiting operator: {task.story_key}", f"committed, but {len(task.operator_actions)} action(s) are owed outside the repo:\n" f"{actions}\n" + f"{followup}" f"run `bmad-loop confirm {task.story_key}` once they are done.", multiline=True, ) diff --git a/src/bmad_loop/operatoractions.py b/src/bmad_loop/operatoractions.py index ae5d8c662..99009cce5 100644 --- a/src/bmad_loop/operatoractions.py +++ b/src/bmad_loop/operatoractions.py @@ -154,6 +154,7 @@ def record_park( root_identity: os.stat_result | None = None, source: str | None = None, ref: str | None = None, + followup_review_recommended: bool | None = None, ) -> Path: """Write a story's park record, returning its path. Re-parking the same key overwrites rather than accumulates: a story owes whatever its latest park @@ -165,6 +166,11 @@ def record_park( Each is recorded only when given, so a sprint or stories record keeps its five keys. + ``followup_review_recommended`` records the build session's follow-up review + recommendation at park time, a pass the park skips, so the record is where the + operator learns it is still owed. Like ``source``/``ref`` it is written only + when given. + The write goes through :func:`platform_util.atomic_write_text` (#379), which carries the unlink-on-raise this used to hand-roll — hence no try/except here; two nested guards would only obscure which one runs — and adds the two things @@ -229,6 +235,8 @@ def record_park( record["source"] = source if ref is not None: record["ref"] = ref + if followup_review_recommended is not None: + record["followup_review_recommended"] = followup_review_recommended atomic_write_text_confined( path, json.dumps(record, indent=2, sort_keys=True), @@ -340,7 +348,12 @@ class ParkedStory: A ticket park (``source`` :data:`SOURCE_TICKETS`) has no board: its committed truth is the plan alone, parked as ``blocked`` with an operator-park ``blocked_reason`` (read from the plan, "" when unset), and ``ref`` is the - ticket whose plan ``confirm`` marks.""" + ticket whose plan ``confirm`` marks. + + ``followup_review_recommended`` is the build session's follow-up review + recommendation at park time, read from the record. None means the record + predates the field or holds a non-bool: unknown, deliberately distinct from + False (the engine always records a real bool).""" story_key: str actions: tuple[str, ...] @@ -354,6 +367,7 @@ class ParkedStory: source: str = "" ref: str = "" blocked_reason: str = "" + followup_review_recommended: bool | None = None @property def ticket(self) -> bool: @@ -500,6 +514,7 @@ def resolve(project: Path, paths: ProjectPaths) -> list[ParkedStory]: spec_path = _spec_path(entry, paths) spec_fm = _spec_frontmatter(spec_path) spec_actions = operator_actions_of(spec_fm) if spec_fm is not None else () + raw = entry.get("followup_review_recommended") out.append( ParkedStory( story_key=key, @@ -516,6 +531,7 @@ def resolve(project: Path, paths: ProjectPaths) -> list[ParkedStory]: source=str(entry.get("source") or ""), ref=str(entry.get("ref") or ""), blocked_reason=_blocked_reason(spec_fm), + followup_review_recommended=raw if isinstance(raw, bool) else None, ) ) return out diff --git a/tests/test_engine.py b/tests/test_engine.py index 8e78e56da..09e03902f 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -4194,6 +4194,64 @@ def test_dev_declared_park_commits_and_the_run_continues(project): assert parked_entry["actions"] == ACTIONS and parked_entry["commit"] == parked.commit_sha +def _committed_park_record(project, sha, key="1-1-a"): + """The park record as committed at ``sha`` — what `confirm` reads later.""" + return json.loads(git(project.project, "show", f"{sha}:.bmad-loop/operator/{key}.json")) + + +def _park_with_followup(project, recommended): + """Drive one dev park whose session reports ``recommended`` as its + `followup_review_recommended`, and return what the park left: the parked task, + the `story-awaiting-operator` row and the committed park record.""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [ + generic_dev_effect( + project, + "1-1-a", + final_status="awaiting-operator", + operator_actions=ACTIONS, + followup_review=recommended, + ) + ], + policy=_park_policy(), + ) + + engine.run() + + parked = engine.state.tasks["1-1-a"] + assert parked.phase == Phase.AWAITING_OPERATOR + # trigger="always": the park, not a recommendation-free trigger, skipped review + assert not any(s.role == "review" for s in adapter.sessions) + parked_entry = next( + e for e in engine.journal.entries() if e["kind"] == "story-awaiting-operator" + ) + return parked, parked_entry, _committed_park_record(project, parked.commit_sha) + + +def test_park_with_a_recommended_followup_records_it_and_still_skips_review(project): + """A park skips the review loop, so a follow-up review the dev pass recommended + never runs. The `story-awaiting-operator` row and the committed park record + keep the recommendation for the human; the park itself is unchanged. + + Ablation: drop the field from the journal row or from `_write_park_record`'s + `record_park` call and this fails on the KeyError.""" + _, parked_entry, record = _park_with_followup(project, True) + assert parked_entry["followup_review_recommended"] is True + assert record["followup_review_recommended"] is True + + +def test_park_without_a_followup_records_false(project): + """No recommendation is a real answer, written as False rather than left + absent: an absent key reads as unknown (a record that predates the field). + + Ablation: as above — the KeyError, not a falsy read, is what fails.""" + _, parked_entry, record = _park_with_followup(project, False) + assert parked_entry["followup_review_recommended"] is False + assert record["followup_review_recommended"] is False + + def test_park_commit_message_marks_the_story_awaiting_operator(project): write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) engine, _ = make_engine( @@ -5121,13 +5179,19 @@ def test_park_notice_keeps_its_numbered_action_list_on_separate_lines(project): the header line the way a reason-carrying notice does. Ablation: drop `multiline=True` from `_notify_park` and the actions fold onto - the header line with ` ⏎ `, so the per-line assertions redden.""" + the header line with ` ⏎ `, so the per-line assertions redden. No follow-up is + recommended, so the confirm hint follows the actions directly (the follow-up + line's own placement is pinned by the test below).""" write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) engine, _ = make_engine( project, [ generic_dev_effect( - project, "1-1-a", final_status="awaiting-operator", operator_actions=ACTIONS + project, + "1-1-a", + final_status="awaiting-operator", + operator_actions=ACTIONS, + followup_review=False, ) ], policy=_park_policy(), @@ -5143,6 +5207,41 @@ def test_park_notice_keeps_its_numbered_action_list_on_separate_lines(project): assert lines[header + len(ACTIONS) + 1].startswith("run `bmad-loop confirm 1-1-a`") +_FOLLOWUP_NOTICE = ( + "the build session also recommended a follow-up review, which a park skips: " + "after confirming, re-run the build skill on the spec for that pass." +) + + +@pytest.mark.parametrize("recommended", [True, False]) +def test_park_notification_mentions_a_recommended_followup(project, recommended): + """The notification is the one park artifact that reaches someone away from the + repo, so it says when the skipped follow-up review is still owed, on its own + line before the confirm hint. Without a recommendation the body is unchanged. + + Ablation: drop the follow-up line from `_notify_park` and the True case fails.""" + engine, _ = make_engine(project, [], policy=_park_policy()) + engine.run_dir.mkdir(parents=True, exist_ok=True) + task = StoryTask( + story_key="1-1-a", + epic=1, + operator_actions=list(ACTIONS), + followup_review_recommended=recommended, + ) + + engine._notify_park(task) + + lines = (engine.run_dir / "ATTENTION").read_text(encoding="utf-8").splitlines() + (header,) = [i for i, ln in enumerate(lines) if "story awaiting operator: 1-1-a" in ln] + after_actions = header + len(ACTIONS) + 1 + if recommended: + assert lines[after_actions] == _FOLLOWUP_NOTICE + after_actions += 1 + else: + assert not any("follow-up review" in ln for ln in lines) + assert lines[after_actions].startswith("run `bmad-loop confirm 1-1-a`") + + # A multi-line, control-bearing untrusted fragment (DW-417): its second line must # never land in ATTENTION as a loose, unprefixed line. _MULTILINE_FRAGMENT = "fatal: x\x1b[31m\nhint: y" @@ -5231,12 +5330,13 @@ def _demotion_policy(mode="park", **kw): return _park_policy(operator=OperatorPolicy(on_review_demotion=mode), **kw) -def _review_pass(paths, story_key, status, *, operator_actions=None, on_entry=None): +def _review_pass(paths, story_key, status, *, operator_actions=None, on_entry=None, followup=False): """A review pass that finalizes the spec at ``status`` and — like the real skill, which is told never to write the board — leaves sprint-status alone. (``review_effect`` writes the board to ``done`` itself, which would mask the orchestrator's own board writes these tests are about.) ``on_entry`` runs - first, to observe the state the pass was launched into.""" + first, to observe the state the pass was launched into. ``followup`` is the + pass's `followup_review_recommended`.""" def effect(spec: SessionSpec) -> SessionResult: if on_entry is not None: @@ -5252,7 +5352,7 @@ def effect(spec: SessionSpec) -> SessionResult: "spec_file": str(sp), "baseline_commit": baseline, "status": status, - "followup_review_recommended": False, + "followup_review_recommended": followup, "escalations": [], }, ) @@ -5309,6 +5409,38 @@ def test_review_demotion_parks_under_park_policy_and_the_run_continues(project): ) +def test_review_demotion_park_records_the_last_pass_followup(project): + """A demotion pass converges whatever its own follow-up flag says, so the + review it recommends is skipped exactly like a dev park's. The latest pass's + flag is the one the park carries: the dev pass here recommended a review (it + ran, as the demotion pass) and the demotion pass recommends another. + + Ablation: drop the field from the journal row or the park record write and + this fails.""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [ + generic_dev_effect(project, "1-1-a"), + _review_pass( + project, "1-1-a", "awaiting-operator", operator_actions=ACTIONS, followup=True + ), + ], + policy=_demotion_policy(), + ) + + engine.run() + + parked = engine.state.tasks["1-1-a"] + assert parked.phase == Phase.AWAITING_OPERATOR + assert len([s for s in adapter.sessions if s.role == "review"]) == 1 + parked_entry = next( + e for e in engine.journal.entries() if e["kind"] == "story-awaiting-operator" + ) + assert parked_entry["followup_review_recommended"] is True + assert _committed_park_record(project, parked.commit_sha)["followup_review_recommended"] is True + + def test_review_demotion_is_not_taken_under_the_default_escalate(project): """The same scripting under the default `escalate`: nothing changes from before DW-383 — the demotion is a non-terminal status, the loop cycles to the budget, diff --git a/tests/test_operatoractions.py b/tests/test_operatoractions.py index b6fb94d76..cddf35f43 100644 --- a/tests/test_operatoractions.py +++ b/tests/test_operatoractions.py @@ -60,8 +60,9 @@ def _legacy_entry(paths, key="1-1-a", *, actions=None, commit="abcdef1234567890" store.write_text(json.dumps(data, indent=2, sort_keys=True)) -def _park(paths, key="1-1-a", *, actions=None, status="awaiting-operator", board=None): - """Record a park and write the committed state it points at.""" +def _park(paths, key="1-1-a", *, actions=None, status="awaiting-operator", board=None, **record): + """Record a park and write the committed state it points at. ``record`` is + forwarded to `record_park` (its optional keys).""" if not (paths.project / "_bmad" / "bmm" / "config.yaml").is_file(): install_bmad_config(paths) acts = ACTIONS if actions is None else actions @@ -80,6 +81,7 @@ def _park(paths, key="1-1-a", *, actions=None, status="awaiting-operator", board spec_file=spec.relative_to(paths.project).as_posix(), run_id="run-1", parked_at="2026-07-28", + **record, ) return spec @@ -496,6 +498,34 @@ def test_resolve_falls_back_to_the_recorded_actions_when_the_spec_has_none(proje assert story.actions == tuple(ACTIONS) +@pytest.mark.parametrize("recommended", [True, False]) +def test_park_record_carries_the_followup_recommendation(project, recommended): + """The build's follow-up review recommendation is a pass the park skips, so + the record keeps it and `resolve` reads it back as itself: False is a real + answer, written, not an absent key.""" + _park(project, followup_review_recommended=recommended) + assert operatoractions.load(project.project)["1-1-a"]["followup_review_recommended"] is ( + recommended + ) + (story,) = operatoractions.resolve(project.project, project) + assert story.followup_review_recommended is recommended + + +def test_park_record_without_the_followup_key_reads_unknown(project): + """A record that predates the key, or holds a non-bool, says nothing about the + recommendation. It must read None (unknown), never False, which would claim + no review is owed.""" + _park(project) + assert "followup_review_recommended" not in operatoractions.load(project.project)["1-1-a"] + _park(project, "1-2-b") + rp = operatoractions.record_path(project.project, "1-2-b") + record = json.loads(rp.read_text()) + record["followup_review_recommended"] = "yes" + rp.write_text(json.dumps(record)) + stories = operatoractions.resolve(project.project, project) + assert [s.followup_review_recommended for s in stories] == [None, None] + + def test_resolve_is_sorted_by_story_key(project): _park(project, "1-2-b") _park(project, "1-1-a") diff --git a/tests/test_tickets_engine.py b/tests/test_tickets_engine.py index 9950be68a..4b584aa7b 100644 --- a/tests/test_tickets_engine.py +++ b/tests/test_tickets_engine.py @@ -2574,10 +2574,11 @@ def _write_parked_plan( plan.write_bytes("\n".join(lines).encode("utf-8")) -def _parks(layout: V7Layout, tree: FakeTicketTree, plan: Path): +def _parks(layout: V7Layout, tree: FakeTicketTree, plan: Path, *, followup: bool | None = None): """A dev session that builds what it can and parks the ticket: code in the tree, the plan at `awaiting-operator` with its actions, and the fake's row - reading the session's own token as `tickets.py` would hand it back.""" + reading the session's own token as `tickets.py` would hand it back. + ``followup``, when given, is the result's `followup_review_recommended`.""" def park(spec: SessionSpec) -> SessionResult: cwd = Path(spec.cwd) @@ -2589,6 +2590,8 @@ def park(spec: SessionSpec) -> SessionResult: tree.finds["1.1"], row=ticket_row("1.1", status="awaiting-operator") ) result = {"workflow": "auto-dev", "spec_file": str(here), "status": "awaiting-operator"} + if followup is not None: + result["followup_review_recommended"] = followup return SessionResult(status="completed", result_json=result) return park @@ -2659,6 +2662,35 @@ def test_park_converted_to_blocked_at_commit(project_v7, monkeypatch): ) == 1 +def test_ticket_park_records_the_followup_recommendation(project_v7, monkeypatch): + """A ticket park skips review like a sprint park, so a follow-up review the + build recommended is carried on the park record beside the source and ref + `confirm` needs: the operator runs it after confirming the plan. + + Ablation: drop the field from `_write_park_record`'s `record_park` call and + this fails on the KeyError.""" + tree = _tree(project_v7, ready_to_start=(ticket_row("1.1"),)) + plan = Path(tree.finds["1.1"].plan) + _park_marking_tree(tree, plan, monkeypatch) + engine, adapter = make_engine( + project_v7, tree, script=[_parks(project_v7, tree, plan, followup=True)] + ) + engine.state.max_stories = 1 + + engine.run() + + task = load_state(engine.run_dir).tasks["1.1"] + assert task.phase == Phase.AWAITING_OPERATOR and task.commit_sha + assert not any(s.role == "review" for s in adapter.sessions) + record = _park_record(project_v7) + assert (record["source"], record["ref"]) == ("tickets", "1.1") + assert record["followup_review_recommended"] is True + parked_entry = next( + e for e in engine.journal.entries() if e["kind"] == "story-awaiting-operator" + ) + assert parked_entry["followup_review_recommended"] is True + + @pytest.mark.parametrize("mark_done", ["auto", "never"]) def test_park_not_marked_done(project_v7, monkeypatch, mark_done): """A parked ticket is never accepted: whatever `[tickets] mark_done` says, the From 6634c719ec2ef19b1fd6f22ebb1549e4e54a9373 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 26 Oct 2026 12:08:50 -0700 Subject: [PATCH 2/3] fix(operator): surface a parked story's owed follow-up review in confirm and status `confirm --list` prints a `follow-up review recommended at park time` line under a parked story's actions, `confirm --json` and `status --json` carry `followup_review_recommended` (null for a park record that predates the field, never false), and a successful `confirm` names the spec or plan to hand bmad-build-auto for the owed pass. The escalations and tickets guides give the procedure: confirm first, then review the done (or built) spec, because the build skill does not route a parked one. The park short-circuit is unchanged and nothing goes to the ledger. CONFIRM_SCHEMA_VERSION stays 1: the fields are additive. --- CHANGELOG.md | 5 ++ docs/FEATURES.md | 1 + docs/escalations-guide.md | 12 ++++ docs/reference/cli.md | 4 +- docs/tickets-guide.md | 2 + src/bmad_loop/cli.py | 12 ++++ src/bmad_loop/documents.py | 11 ++- tests/test_cli.py | 137 ++++++++++++++++++++++++++++++++++++- 8 files changed, 178 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3191ea482..bb993d6f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -342,6 +342,11 @@ breaking changes may land in a minor release. severity (`P1`) files the entry without the line and journals `spec-deferrals-severity-unrecognized` instead of dropping it silently; a collection or NUL severity marks the item malformed (#913). +- Keep a parked story's follow-up review recommendation: the park record, the + `story-awaiting-operator` journal row, the park notification, `confirm --list`/`--json`, + `confirm`'s success output and `status --json` now carry `followup_review_recommended`, + and the escalations and tickets guides give the post-confirm review procedure. A park + still never runs the review (#888). ### Security diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 66b6831b2..bcb9cc00f 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -148,6 +148,7 @@ bmad-loop drives a BMAD sprint unattended: a deterministic Python loop launches - `bmad-loop confirm --list` shows each parked story and what it owes; `--json` emits it as a machine-readable document. - A confirmation adds an `## Operator Confirmation` section, moves spec and board to `done`, and commits. - `confirm --reverify` runs the environment probes and `[verify]` commands first; a failure blocks the confirmation. + - A park keeps the build's follow-up review recommendation on the park record and shows it in `confirm`, `status --json` and the park notification; the park never runs that review, which runs after confirmation. - `[operator] on_review_demotion = "park"` lets a review demote a `done` story to `awaiting-operator` (sprint and tickets mode); the default `escalate` rejects the demotion. - **Config:** `[operator] enabled`, `[operator] on_review_demotion`, `bmad-loop confirm --list`, `bmad-loop confirm --yes`, `bmad-loop confirm --reverify`, `bmad-loop confirm --json` - **Tested by:** `tests/test_engine.py`, `tests/test_cli.py`, `tests/test_operatoractions.py`, `tests/test_tickets_engine.py` diff --git a/docs/escalations-guide.md b/docs/escalations-guide.md index 9aa2068fd..ea69409d5 100644 --- a/docs/escalations-guide.md +++ b/docs/escalations-guide.md @@ -190,6 +190,18 @@ If `confirm` is interrupted, run the same command again; it finishes without pro In tickets mode the park lives on the ticket's plan, not a board. The unit's commit marks the plan `blocked` with a reason starting `awaiting operator: ` that lists the actions, and the park record names the ticket ref. Tickets that wait on it end the run with a parked summary. `confirm ` appends the same section, then marks the plan through `tickets.py mark`: `done` under `[tickets] mark_done = "auto"`, `built` under `"never"`. A review that demotes a built ticket parks the same way under `[operator] on_review_demotion = "park"`. +### When a parked story's build recommended a follow-up review + +A park skips the review loop: a review would either re-park the story or finalize the park away before the human acts. When the build session recommended a follow-up review, that pass is still owed, and the park keeps the recommendation. It shows in the park notification, in `confirm --list` (a `follow-up review recommended at park time` line under the actions), as `followup_review_recommended` in `confirm --json` and `status --json`, and on the `story-awaiting-operator` journal row. A park recorded before the field existed reads `null` and shows no line. + +To run the owed pass: + +1. Confirm the story first (`bmad-loop confirm `). Its success output repeats the recommendation and names the spec. +2. Invoke bmad-build-auto on the now-`done` spec. The skill routes a `done` spec to a fresh follow-up review pass. +3. Commit what the pass patches. + +Reviewing before you confirm does not work, because the build skill does not route a parked spec. If the owed action is a manual test, review the parked work first with a review that leaves the spec's status alone (for example bmad-code-review over the park commit), so the test covers reviewed code. Otherwise re-test after the post-confirm pass if it patches code. + ## Environment faults An environment fault is a failure caused by the environment rather than the code. It never charges a dev attempt. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 3508e3587..54ed52652 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -517,7 +517,7 @@ Each row: `name`, `origin`, `path` (POSIX path from the code root), `surface` (` ### `confirm --json` -`parked`: stories at `awaiting-operator`, in story-key order. Each has `story_key`, `actions` (list of str), `spec_file`, `spec_status`, `board_status`, `commit` (`""` until a commit carries the park record), `run_id`, `parked_at`, `confirmable`, `confirmation_recorded`, `resumable` (an interrupted confirmation `confirm` will finish), and `drift` (human-readable). +`parked`: stories at `awaiting-operator`, in story-key order. Each has `story_key`, `actions` (list of str), `spec_file`, `spec_status`, `board_status`, `commit` (`""` until a commit carries the park record), `run_id`, `parked_at`, `confirmable`, `confirmation_recorded`, `resumable` (an interrupted confirmation `confirm` will finish), `drift` (human-readable), and `followup_review_recommended` (bool: the build recommended a follow-up review the park skipped; null for a park record that predates the field). ### `list --json` @@ -547,7 +547,7 @@ Built from the run's `state.json`. | `tickets` | object \| null | tickets runs only, below; null for every other source | | `tasks` | array | one entry per story | -Each task: `story_key`, `epic`, `phase`, `attempt`, `review_cycle`, `tokens` (`input_tokens`, `output_tokens`, `cache_read_tokens`, `cache_creation_tokens`, `raw`, `weighted`), `commit_sha`, `defer_reason`, `preserve_ref`, `adapters_used` (`{role: {name, model}}`), `ticket` (object or null, below). +Each task: `story_key`, `epic`, `phase`, `attempt`, `review_cycle`, `tokens` (`input_tokens`, `output_tokens`, `cache_read_tokens`, `cache_creation_tokens`, `raw`, `weighted`), `commit_sha`, `defer_reason`, `preserve_ref`, `followup_review_recommended` (bool: the build session recommended a follow-up review), `adapters_used` (`{role: {name, model}}`), `ticket` (object or null, below). `epic` is always an integer. A unit of a source with no integer epics (stories, sweep, tickets) reports `0`; a ticket's epic is in its `ticket` object. diff --git a/docs/tickets-guide.md b/docs/tickets-guide.md index 6a918f23e..d45d0946e 100644 --- a/docs/tickets-guide.md +++ b/docs/tickets-guide.md @@ -143,6 +143,8 @@ The run records the document's `verdict` (or `unknown` when there is none) and p When a session parks a ticket for a person (an `awaiting-operator` result, or a review demotion under `[operator] on_review_demotion = "park"`), the unit commits and its plan is marked `blocked` with the reason `awaiting operator: `, never `done`. Do the actions, then run `bmad-loop confirm `: it marks the plan `done` (`built` under `mark_done = "never"`) and clears `blocked_*`. The plan is committed with the park record's deletion when the code checkout tracks the store, in the store's own repository when it has one, and not at all in an unversioned store. +When the build session recommended a follow-up review, the park keeps that recommendation (`confirm --list`, `followup_review_recommended` in `confirm --json` and `status --json`) and `confirm ` repeats it with the plan's path. Run the pass after confirming: the plan is then `done` (`built` under `mark_done = "never"`), and bmad-build-auto routes either status to a follow-up review. A parked plan reads `blocked`, which the skill refuses. See [When a parked story's build recommended a follow-up review](escalations-guide.md#when-a-parked-storys-build-recommended-a-follow-up-review). + An escalated ticket is recovered with `bmad-loop resolve `. The ticket's plan is the frozen spec, and its `` block is the intent. The re-arm moves the plan through `tickets.py mark` (back to `ready-for-dev`, or `in-review` for a restore) and clears `blocked_*`; a refused move aborts the re-arm and puts the plan back byte for byte. See [Resolve in tickets mode](escalations-guide.md#resolve-in-tickets-mode). A code rollback re-arms the plan too: a plan past `ready-for-dev` returns to `ready-for-dev`, and a plan that holds only frontmatter is deleted. diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index dceac6cfd..d74ad2dc7 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -7158,6 +7158,10 @@ def _print_parked(parked: list[operatoractions.ParkedStory]) -> int: print(f" {p.story_key} (parked {p.parked_at}{provenance}){suffix}") for i, action in enumerate(p.actions, 1): print(f" {i}. {action}") + # `is True`, not truthiness: None is a record that predates the field, + # which owes nothing we know of and so shows nothing. + if p.followup_review_recommended is True: + print(" follow-up review recommended at park time: run it after confirming") print("") print("confirm one when its actions are done: bmad-loop confirm ") return 0 @@ -7693,6 +7697,14 @@ def _commit_confirmation( except verify.GitError: pass # files are written; git history is best effort (as `decisions`) print(f"✓ {story.story_key} confirmed — {landed}") + if story.followup_review_recommended is True: + # The park skipped the pass; the now-done (or built) spec is one the build + # skill routes to a fresh review, which a parked spec is not. + target = story.spec_path if story.spec_path is not None else "the story's spec" + print( + f" follow-up review still owed: invoke bmad-build-auto on {target} " + "(it routes a done or built spec to a fresh review pass)" + ) return 0 diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 7b29e5748..427d8911e 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -167,7 +167,12 @@ def confirm_document(parked: list[ParkedStory]) -> dict[str, object]: DERIVED from the park record's git history rather than stored at park time (the committed record cannot contain its own sha), so it can be `""` for a record not yet in any commit — a consumer that pinned "always 40 hex chars" - must treat empty as "no provenance", which the field always could express.""" + must treat empty as "no provenance", which the field always could express. + + `followup_review_recommended` is the build session's follow-up review + recommendation at park time, a pass the park skipped and the operator still + owes after confirming. It is a real bool for a park recorded with it and null + for a record that predates the field: unknown, deliberately not false.""" return { "schema_version": CONFIRM_SCHEMA_VERSION, "parked": [ @@ -184,6 +189,7 @@ def confirm_document(parked: list[ParkedStory]) -> dict[str, object]: "confirmation_recorded": p.confirmation_recorded, "resumable": p.resumable, "drift": p.drift(), + "followup_review_recommended": p.followup_review_recommended, } for p in parked ], @@ -378,6 +384,9 @@ def status_document(state: RunState, *, graceful_stop_pending: bool = False) -> # null. Reported verbatim from state.json — never re-validated # against git here (retention may since have pruned it). "preserve_ref": task.preserve_ref, + # the build session's follow-up review recommendation; a park + # skips that pass, so a parked story still owes it after confirm. + "followup_review_recommended": task.followup_review_recommended, "adapters_used": adapters_used, "ticket": _ticket_projection(task), } diff --git a/tests/test_cli.py b/tests/test_cli.py index f50653e33..07dedb991 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2651,6 +2651,24 @@ def test_status_json_carries_the_preserve_ref(project, capsys): assert entry_done["preserve_ref"] is None +def test_status_json_carries_the_followup_recommendation(project, capsys): + """A parked story's follow-up review recommendation is reported from + state.json: the park skipped that pass, so it is still owed after confirm. + Ablation (performed): drop the field from the status task projection and + this FAILS on the missing key.""" + from bmad_loop.model import Phase, StoryTask + + parked = StoryTask(story_key="1-1-login", epic=1, phase=Phase.AWAITING_OPERATOR) + parked.followup_review_recommended = True + done = StoryTask(story_key="1-2-logout", epic=1, phase=Phase.DONE) + _make_run_with_tokens(project, {"1-1-login": parked, "1-2-logout": done}, weight=0.1) + + doc = _status_json(project, capsys) + entry_parked, entry_done = doc["tasks"] + assert entry_parked["followup_review_recommended"] is True + assert entry_done["followup_review_recommended"] is False + + def test_status_text_names_the_preserve_ref_beside_the_reason(project, capsys): from bmad_loop.model import Phase, StoryTask @@ -17212,8 +17230,11 @@ def test_mux_detection_failure_reports_instead_of_vanishing(mux_registry, monkey CONFIRM_ACTIONS = ["buy example.com at the registrar", "publish the _acme-challenge TXT record"] -def _park_story(paths, key="1-1-a", *, actions=None, spec_status="awaiting-operator", board=None): - """Put the project in the state a park leaves behind: spec, board, record.""" +def _park_story( + paths, key="1-1-a", *, actions=None, spec_status="awaiting-operator", board=None, followup=None +): + """Put the project in the state a park leaves behind: spec, board, record. + ``followup`` None writes a record without the follow-up key (a legacy park).""" from bmad_loop import operatoractions acts = CONFIRM_ACTIONS if actions is None else actions @@ -17227,6 +17248,7 @@ def _park_story(paths, key="1-1-a", *, actions=None, spec_status="awaiting-opera spec_file=sp.relative_to(paths.project).as_posix(), run_id="run-1", parked_at="2026-07-28", + followup_review_recommended=followup, ) return sp @@ -17404,6 +17426,30 @@ def test_confirm_refuses_a_park_declaring_nothing(project, capsys, monkeypatch): assert "no readable actions" in capsys.readouterr().err +@pytest.mark.parametrize("followup", [True, False, None], ids=["recommended", "not", "legacy"]) +def test_confirm_success_names_the_owed_followup_review(project, capsys, monkeypatch, followup): + """Confirming a park whose build recommended a follow-up review says the pass + is still owed and names the now-done spec the build skill routes to it; a park + recommending nothing, or a legacy record, keeps the one-line success. Ablation + (performed): drop the hint from `_commit_confirmation` and the recommended + case FAILS.""" + install_bmad_config(project) + sp = _park_story(project, followup=followup) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "park") + monkeypatch.setattr(cli, "_confirm", lambda _q: True) + + assert cli.main(_confirm_argv(project, "1-1-a")) == 0 + + out = capsys.readouterr().out + hint = f" follow-up review still owed: invoke bmad-build-auto on {sp} (it routes" + if followup is True: + tail = out[out.index("✓ 1-1-a confirmed") :].splitlines() + assert tail[1].startswith(hint) + else: + assert "follow-up review" not in out + + def test_confirm_list_shows_what_each_story_owes(project, capsys): install_bmad_config(project) _park_story(project) @@ -17415,6 +17461,35 @@ def test_confirm_list_shows_what_each_story_owes(project, capsys): assert action in out +FOLLOWUP_LIST_LINE = "follow-up review recommended at park time: run it after confirming" + + +@pytest.mark.parametrize( + ("followup", "shown"), + [ + pytest.param(True, True, id="recommended"), + pytest.param(False, False, id="not-recommended"), + pytest.param(None, False, id="legacy-record"), + ], +) +def test_confirm_list_flags_a_recommended_followup(project, capsys, followup, shown): + """A park skips the review loop, so a follow-up review the build recommended + is still owed; the listing says so under the story's actions. A record that + predates the field reads unknown and shows nothing, like one recommending no + pass. Ablation (performed): drop the line from `_print_parked` and the + recommended case FAILS.""" + install_bmad_config(project) + _park_story(project, followup=followup) + + assert cli.main(_confirm_argv(project, "--list")) == 0 + out = capsys.readouterr().out + assert (FOLLOWUP_LIST_LINE in out) is shown + if shown: + # under the story's numbered actions, not after the closing hint + assert out.index(CONFIRM_ACTIONS[-1]) < out.index(FOLLOWUP_LIST_LINE) + assert out.index(FOLLOWUP_LIST_LINE) < out.index("confirm one when") + + def test_confirm_list_marks_a_drifted_entry_rather_than_hiding_it(project, capsys): install_bmad_config(project) _park_story(project, spec_status="done") @@ -17469,6 +17544,31 @@ def test_confirm_json_carries_both_sides_of_a_disagreement(project, capsys): assert entry["confirmable"] is False and "done" in entry["drift"] +def test_confirm_json_carries_the_followup_recommendation(project, capsys): + """`followup_review_recommended` is the park's recorded bool, and null — not + false — for a record that predates the field: absence is unknown, never a + healthy-looking "nothing owed". Ablation (performed): drop the field from + `confirm_document` and every case FAILS on the missing key.""" + install_bmad_config(project) + _park_story(project, "1-1-a", followup=True) + _park_story(project, "1-2-b", followup=False) + _park_story(project, "1-3-c") # no key: a legacy record + write_sprint( + project, + { + "epic-1": "in-progress", + "1-1-a": "awaiting-operator", + "1-2-b": "awaiting-operator", + "1-3-c": "awaiting-operator", + }, + ) + + doc = machine_json(_confirm_argv(project, "--json"), capsys) + got = {e["story_key"]: e["followup_review_recommended"] for e in doc["parked"]} + assert got == {"1-1-a": True, "1-2-b": False, "1-3-c": None} + assert doc["schema_version"] == 1 + + def test_confirm_json_with_nothing_parked_is_an_empty_document(project, capsys): install_bmad_config(project) write_sprint(project, {"epic-1": "in-progress"}) @@ -24910,7 +25010,7 @@ def test_validate_trust_reads_the_configured_repo_root(project, capsys, monkeypa TICKET_PARK_ACTIONS = ["buy example.com at the registrar", "publish the acme TXT record"] -def _park_ticket(layout, *, policy_text: str): +def _park_ticket(layout, *, policy_text: str, followup: bool | None = None): """Leave ticket 1.1 as a tickets run's park commits it: the plan `blocked` with an operator-park reason and its owed actions, and a park record naming source and ref — all committed, under a tickets policy.""" @@ -24945,6 +25045,7 @@ def _park_ticket(layout, *, policy_text: str): parked_at="2026-10-15", source="tickets", ref="1.1", + followup_review_recommended=followup, ) git(layout.project, "add", "-A") git(layout.project, "commit", "-q", "-m", "park 1.1") @@ -25069,6 +25170,36 @@ def test_confirm_tickets_never_policy_marks_built(project_v7, capsys, monkeypatc assert "NOT confirmed" in capsys.readouterr().err +def test_confirm_tickets_names_the_plan_for_the_owed_followup_review( + project_v7, capsys, monkeypatch +): + """A ticket park's owed follow-up review names the PLAN: after confirm it is + `built` (here, under `mark_done = "never"`) or `done`, both of which the build + skill routes to a follow-up pass, where the parked `blocked` plan was refused. + Ablation (performed): drop the hint from `_commit_confirmation` and this + FAILS.""" + import dataclasses + + from conftest import FakeTicketTree, fake_find, ticket_row + + plan = _park_ticket( + project_v7, + policy_text='[stories]\nsource = "tickets"\n\n[tickets]\nmark_done = "never"\n', + followup=True, + ) + reason = "awaiting operator: " + "; ".join(TICKET_PARK_ACTIONS) + row = ticket_row("1.1", status="blocked", blocked_reason=reason) + tree = FakeTicketTree(project_v7.project, finds={"1.1": fake_find(row, plan.parent)}) + tree.finds["1.1"] = dataclasses.replace(tree.finds["1.1"], plan=str(plan)) + monkeypatch.setattr(cli, "_confirm_ticket_tree", lambda _project: tree) + monkeypatch.setattr(cli, "_confirm", lambda _q: True) + + assert cli.main(["confirm", "--project", str(project_v7.project), "1.1"]) == 0 + + tail = capsys.readouterr().out.split("✓ 1.1 confirmed — plan is built\n", 1)[1] + assert tail.startswith(f" follow-up review still owed: invoke bmad-build-auto on {plan} ") + + def test_resolve_restore_patch_latches_the_evacuated_copy(project_v7): """A tickets re-arm moves the plan's intent-gap patch to `runs.evacuated_patch_path`, so a later resolve cycle naming the original path From b49e4d09c48b17b977e05f01d55f917b1f31c059 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 26 Oct 2026 13:28:03 -0700 Subject: [PATCH 3/3] fix(operator): carry a parked spec's follow-up review flag through synthesis The park record read the flag from the dev result, but `synthesize_result` only carried it on a success status and `_followup_from_spec` gated on the same, so a real generic-path park always recorded False. Carry it on `awaiting-operator` too, and let the fallback read a parked spec. The confirm hint now names the dev primitive the review session would drive (`bmad-dev-auto` on a pre-rename install), and FEATURES says the operator runs the owed pass. --- docs/FEATURES.md | 2 +- docs/escalations-guide.md | 2 +- src/bmad_loop/cli.py | 6 ++++- src/bmad_loop/devcontract.py | 8 +++--- src/bmad_loop/engine.py | 13 ++++++---- tests/test_cli.py | 19 ++++++++++++++ tests/test_devcontract.py | 25 +++++++++++++++--- tests/test_engine.py | 50 ++++++++++++++++++++++++++++++++++++ 8 files changed, 111 insertions(+), 14 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index bcb9cc00f..0d62146f8 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -148,7 +148,7 @@ bmad-loop drives a BMAD sprint unattended: a deterministic Python loop launches - `bmad-loop confirm --list` shows each parked story and what it owes; `--json` emits it as a machine-readable document. - A confirmation adds an `## Operator Confirmation` section, moves spec and board to `done`, and commits. - `confirm --reverify` runs the environment probes and `[verify]` commands first; a failure blocks the confirmation. - - A park keeps the build's follow-up review recommendation on the park record and shows it in `confirm`, `status --json` and the park notification; the park never runs that review, which runs after confirmation. + - A park keeps the build's follow-up review recommendation on the park record and shows it in `confirm`, `status --json` and the park notification. The park never runs that review: after confirming, the operator invokes the build skill on the spec to run it. - `[operator] on_review_demotion = "park"` lets a review demote a `done` story to `awaiting-operator` (sprint and tickets mode); the default `escalate` rejects the demotion. - **Config:** `[operator] enabled`, `[operator] on_review_demotion`, `bmad-loop confirm --list`, `bmad-loop confirm --yes`, `bmad-loop confirm --reverify`, `bmad-loop confirm --json` - **Tested by:** `tests/test_engine.py`, `tests/test_cli.py`, `tests/test_operatoractions.py`, `tests/test_tickets_engine.py` diff --git a/docs/escalations-guide.md b/docs/escalations-guide.md index ea69409d5..18ffa436c 100644 --- a/docs/escalations-guide.md +++ b/docs/escalations-guide.md @@ -197,7 +197,7 @@ A park skips the review loop: a review would either re-park the story or finaliz To run the owed pass: 1. Confirm the story first (`bmad-loop confirm `). Its success output repeats the recommendation and names the spec. -2. Invoke bmad-build-auto on the now-`done` spec. The skill routes a `done` spec to a fresh follow-up review pass. +2. Invoke the build skill that `confirm` names (bmad-build-auto, or bmad-dev-auto on a pre-rename install) on the now-`done` spec. The skill routes a `done` spec to a fresh follow-up review pass. 3. Commit what the pass patches. Reviewing before you confirm does not work, because the build skill does not route a parked spec. If the owed action is a manual test, review the parked work first with a review that leaves the spec's status alone (for example bmad-code-review over the park commit), so the test covers reviewed code. Otherwise re-test after the post-confirm pass if it patches code. diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index d74ad2dc7..729e8a5e1 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -7700,9 +7700,13 @@ def _commit_confirmation( if story.followup_review_recommended is True: # The park skipped the pass; the now-done (or built) spec is one the build # skill routes to a fresh review, which a parked spec is not. + # Named as the review session would spell it (a pre-rename project still + # drives `bmad-dev-auto`); an unreadable policy leaves it generic. target = story.spec_path if story.spec_path is not None else "the story's spec" + pol = _confirm_policy(project) + skill = "the build skill" if pol is None else _dev_skill_for_role(pol, project, "review") print( - f" follow-up review still owed: invoke bmad-build-auto on {target} " + f" follow-up review still owed: invoke {skill} on {target} " "(it routes a done or built spec to a fresh review pass)" ) return 0 diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index dbbe3c087..a462aa5da 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -638,9 +638,11 @@ def synthesize_result( # bmad-build-auto (BMAD-METHOD PR #2505) self-reviews inline and, on a `done` # exit, sets `followup_review_recommended: true` when its review-driven # changes warrant an independent second-opinion pass. The skill never sets it - # on a blocked exit, so only carry it through on a successful exit: `done`, or - # the caller's `success_statuses` (`built` for a v7 ticket). - if status in success_statuses: + # on a blocked exit, so only carry it through on a successful exit (`done`, or + # the caller's `success_statuses`: `built` for a v7 ticket) or a park. A park + # never routes into the review loop, but its record keeps the recommendation + # so the operator can run that skipped pass after confirming. + if status in success_statuses or status == AWAITING_OPERATOR: result["followup_review_recommended"] = bool(fm.get("followup_review_recommended", False)) # A park's obligations travel with its result so the engine never has to # re-read the spec to learn them. Folded on the awaiting-operator status diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 5a44e89d3..c0fb59d4e 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5324,8 +5324,10 @@ def _followup_from_spec(self, task: StoryTask, rj: dict) -> bool: flag — so without this fallback a recommended follow-up review would be silently skipped. Gates on the frontmatter's own status (mirroring the fold): a faulted replay leaves ``rj["status"]`` at the stale snapshot - value, so the result status must not decide. Degrades to False on a read - fault (journaled) — the pre-existing absent-key default. + value, so the result status must not decide. A parked spec counts too, + as it does in the synthesis: its park record carries the recommendation. + Degrades to False on a read fault (journaled) — the pre-existing + absent-key default. """ if not self._generic_dev(): return False @@ -5338,9 +5340,10 @@ def _followup_from_spec(self, task: StoryTask, rj: dict) -> bool: fm = self._observed_frontmatter(spec_path, task.story_key, "followup-routing") if fm is None: return False - return verify.status_of(fm) == self._dev_success_status() and bool( - fm.get("followup_review_recommended", False) - ) + return verify.status_of(fm) in ( + self._dev_success_status(), + verify.AWAITING_OPERATOR, + ) and bool(fm.get("followup_review_recommended", False)) def _reconcile_generic_terminal_status(self, task: StoryTask, result_json: dict | None) -> None: """Repair a generic-skill spec the session finalized in prose but not in diff --git a/tests/test_cli.py b/tests/test_cli.py index 07dedb991..41d548775 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -17434,6 +17434,7 @@ def test_confirm_success_names_the_owed_followup_review(project, capsys, monkeyp (performed): drop the hint from `_commit_confirmation` and the recommended case FAILS.""" install_bmad_config(project) + install_build_auto_skill(project.project) sp = _park_story(project, followup=followup) git(project.project, "add", "-A") git(project.project, "commit", "-q", "-m", "park") @@ -17450,6 +17451,23 @@ def test_confirm_success_names_the_owed_followup_review(project, capsys, monkeyp assert "follow-up review" not in out +def test_confirm_followup_hint_names_the_installed_build_skill(project, capsys, monkeypatch): + """The hint spells the dev primitive the project's review session would drive, + so a pre-rename project that only carries `bmad-dev-auto` is told that name. + Ablation (performed): hard-code `bmad-build-auto` in the hint and this FAILS.""" + install_bmad_config(project) + install_dev_base_skills(project.project, folder_id=True) # the legacy era alone + sp = _park_story(project, followup=True) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "park") + monkeypatch.setattr(cli, "_confirm", lambda _q: True) + + assert cli.main(_confirm_argv(project, "1-1-a")) == 0 + + tail = capsys.readouterr().out.split("✓ 1-1-a confirmed", 1)[1].splitlines() + assert tail[1].startswith(f" follow-up review still owed: invoke bmad-dev-auto on {sp} ") + + def test_confirm_list_shows_what_each_story_owes(project, capsys): install_bmad_config(project) _park_story(project) @@ -25187,6 +25205,7 @@ def test_confirm_tickets_names_the_plan_for_the_owed_followup_review( policy_text='[stories]\nsource = "tickets"\n\n[tickets]\nmark_done = "never"\n', followup=True, ) + install_build_auto_skill(project_v7.project) reason = "awaiting operator: " + "; ".join(TICKET_PARK_ACTIONS) row = ticket_row("1.1", status="blocked", blocked_reason=reason) tree = FakeTicketTree(project_v7.project, finds={"1.1": fake_find(row, plan.parent)}) diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index e03ff4f26..0e282dead 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -482,16 +482,35 @@ def test_synth_awaiting_operator_synthesizes_no_escalation(tmp_path): """The distinction the whole state exists for: `blocked` means the session could not proceed and the run must halt; a park means it finished everything an agent could. A CRITICAL here would collapse the two into the pause - channel. `followup_review_recommended` is likewise absent — a parked story - does not route into the review loop at all.""" + channel.""" sp = _spec(tmp_path / "s.md", status="awaiting-operator", auto_run=None, actions="['do it']") out = devcontract.synthesize_result(sp, story_key="1-1-a") assert out.result_json["escalations"] == [] - assert "followup_review_recommended" not in out.result_json assert out.result_json["park_asserted"] is False +@pytest.mark.parametrize("followup", [True, False, None]) +def test_synth_awaiting_operator_carries_the_followup_recommendation(tmp_path, followup): + """A park skips the review loop, but its record keeps the session's follow-up + review recommendation for the operator, so the synthesis carries the + frontmatter flag on a park as it does on a success (an absent key reads + False, as on `done`). The engine's park record reads this key. + + Ablation: drop the AWAITING_OPERATOR arm of the followup carry and the key is + absent, so every case fails.""" + sp = _spec( + tmp_path / "s.md", + status="awaiting-operator", + auto_run="awaiting-operator", + followup=followup, + actions="['do it']", + ) + rj = devcontract.synthesize_result(sp, story_key="1-1-a").result_json + + assert rj is not None and rj["followup_review_recommended"] is bool(followup) + + def test_synth_park_assertion_uses_only_the_last_real_marker(tmp_path): """A fenced example and an older genuine park cannot authorize a later result.""" sp = _spec( diff --git a/tests/test_engine.py b/tests/test_engine.py index 09e03902f..35e8b54a8 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -4252,6 +4252,56 @@ def test_park_without_a_followup_records_false(project): assert record["followup_review_recommended"] is False +@pytest.mark.parametrize("keyed", [True, False]) +def test_park_followup_survives_the_real_synthesis(project, keyed): + """The two tests above hand the engine a result that already holds the key. + Here the result is what `devcontract.synthesize_result` builds from a parked + spec whose frontmatter recommends a follow-up review, so the recommendation + must survive the synthesis (``keyed``) or, for a result without the key (a + resumed pre-reconcile snapshot), the `_followup_from_spec` re-derivation. + + Ablation: drop the AWAITING_OPERATOR arm of the synthesis carry (keyed: the + key reads None) or of `_followup_from_spec`'s status gate (not keyed) and the + park records something other than True.""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + + def effect(spec: SessionSpec) -> SessionResult: + baseline = rev_parse_head(project.project) + src = project.project / "src.txt" + src.write_text(src.read_text() + "change for 1-1-a\n") + sp = spec_path(project, "1-1-a") + write_spec( + sp, + "awaiting-operator", + baseline, + prose_status="awaiting-operator", + operator_actions=ACTIONS, + ) + text = sp.read_text() + sp.write_text(text.replace("---\n", "---\nfollowup_review_recommended: true\n", 1)) + rj = devcontract.synthesize_result( + sp, story_key="1-1-a", park_marker_session_authored=True + ).result_json + assert rj is not None + carried = rj.pop("followup_review_recommended", None) + if keyed: + rj["followup_review_recommended"] = carried + return SessionResult(status="completed", result_json=rj) + + engine, adapter = make_engine(project, [effect], policy=_park_policy()) + + engine.run() + + parked = engine.state.tasks["1-1-a"] + assert parked.phase == Phase.AWAITING_OPERATOR + assert not any(s.role == "review" for s in adapter.sessions) + parked_entry = next( + e for e in engine.journal.entries() if e["kind"] == "story-awaiting-operator" + ) + assert parked_entry["followup_review_recommended"] is True + assert _committed_park_record(project, parked.commit_sha)["followup_review_recommended"] is True + + def test_park_commit_message_marks_the_story_awaiting_operator(project): write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) engine, _ = make_engine(