diff --git a/CHANGELOG.md b/CHANGELOG.md index 12013d09..91369481 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,23 @@ breaking changes may land in a minor release. ### Added +- **Stories can park at `awaiting-operator` (#335, part 2 of 4).** A dev session whose story needs + an action only a human can take outside the repo — buy a domain, publish a DNS record, grant an + API key — now finishes and **commits** everything an agent can do, records what is owed in the + spec's `operator_actions:` frontmatter, and parks. The run moves on to the next story instead of + stopping, and the board advances to `awaiting-operator` (a forward move; nothing regresses). + Previously such a story had only two dishonest outcomes: `done`, which hides the outstanding work + behind a green board, or `blocked`, which halts the whole run. + + A park clears the same deterministic gates a `done` story clears — the spec/board pair, the + project's verify commands, and a non-empty action list — and skips only the review loop, which has + nothing in the diff to converge on. A park with no readable actions is refused and repaired, not + committed. Under worktree isolation the unit merges like a `done` one. `[operator] enabled = +false` restores the old two-outcome behavior. + + `bmad-loop confirm` and the project-level registry it reads are part 3; for now a parked story's + obligations live in its spec and in the `story-awaiting-operator` journal entry. + - **`awaiting-operator` vocabulary, no writer yet (#335, part 1 of 4).** Names, at every layer, the state a story reaches when its agent-doable work is finished and committed but its acceptance criteria include external actions only a human can perform: `Phase.AWAITING_OPERATOR` (terminal, diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index b50db617..3ca2d827 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/src/bmad_loop/data/settings/core.toml @@ -428,6 +428,16 @@ default_ref = "TuiPolicy.low_frame_rate" label = "low frame rate" description = "cap to 15fps + disable animations — fixes repaint tearing/garbage over slow or SSH links · takes effect next time the TUI launches" +[[section]] +name = "operator" +description = "stories that owe external, human-only actions" +[[section.field]] +key = "enabled" +kind = "switch" +default_ref = "OperatorPolicy.enabled" +label = "allow awaiting-operator parks" +description = "let a session finish + commit a story whose acceptance criteria need a human action (buy a domain, publish a DNS record) and park it as awaiting-operator instead of forcing done or blocked · what is owed is recorded in the story spec's operator_actions: frontmatter" + [[section]] name = "mux" description = "terminal-multiplexer backend (machine-specific — policy.toml is gitignored)" diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index c46e3bcb..1475f874 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -26,7 +26,7 @@ from typing import Any from .platform_util import atomic_replace -from .verify import DEV_WORKFLOW, read_frontmatter +from .verify import DEV_WORKFLOW, operator_actions_of, read_frontmatter # The section the skill appends on EVERY terminal path (success and blocked), # per its step-02/03/04 finalize instructions. Its presence is our completion @@ -43,6 +43,14 @@ # Terminal frontmatter statuses the skill can leave behind. DONE = "done" BLOCKED = "blocked" +# The story's agent-doable work is finished, but its acceptance criteria include +# external actions only a human can perform (#335). Terminal beside DONE and +# BLOCKED, and deliberately NOT rendered as an escalation: BLOCKED means the +# session could not proceed and the run must halt, while a park means it finished +# everything an agent could and the run should move on. Synthesizing a CRITICAL +# here would collapse that distinction into the pause channel — exactly the +# failure the state exists to avoid. +AWAITING_OPERATOR = "awaiting-operator" # The status a plan-halt dispatch leaves behind: under folder+id dispatch a # `Halt after planning.` directive makes the skill HALT right after the @@ -239,7 +247,11 @@ def synthesize_result( fm_status = str(fm.get("status", "")).strip().lower() arr = parse_auto_run_result(_read_text_or_empty(spec_path)) - terminal = (DONE, BLOCKED, PLAN_HALT_STATUS) if plan_halt else (DONE, BLOCKED) + terminal = ( + (DONE, BLOCKED, AWAITING_OPERATOR, PLAN_HALT_STATUS) + if plan_halt + else (DONE, BLOCKED, AWAITING_OPERATOR) + ) # Not terminal yet: no result section AND frontmatter not at a terminal state. if not arr.present and fm_status not in terminal: return SynthResult(result_json=None, status_consistent=True) @@ -273,6 +285,14 @@ def synthesize_result( # on a blocked exit, so only carry it through on `done`. if status == DONE: 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 + # ONLY: an `operator_actions:` list left on a `done` or `blocked` spec is + # not a park, and carrying it would let a story register obligations the + # verify gates never held it to. Malformed shapes read as [] here and are + # refused by verify's non-empty gate, which owns the retry + feedback. + if status == AWAITING_OPERATOR: + result["operator_actions"] = list(operator_actions_of(fm)) # Mark the clean plan-halt success so verify/engine expect a planned spec # (status ready-for-dev, no implementation work). Never marked when a block # escalation is present — that routes to PAUSE, not a plan-review pause. @@ -350,8 +370,8 @@ def find_frontmatter_candidates(impl_artifacts: Path, *, since_ns: int) -> list[ A candidate must be modified at/after `since_ns` (same session-launch floor as the marker scan), carry ZERO real (non-fenced) marker headings, not be the no-spec fallback file (that one is already matched by name on the normal - path), and have frontmatter ``status:`` of ``done`` or ``blocked``. Returns - ALL matches, most-recent first — the caller refuses to guess between several + path), and have a terminal frontmatter ``status:`` (``done``, ``blocked``, or + ``awaiting-operator``). Returns ALL matches, most-recent first — the caller refuses to guess between several and must apply its own stability fingerprint before synthesizing, because a terminal frontmatter under a live window is weaker evidence than the marker. """ @@ -380,7 +400,8 @@ def is_frontmatter_candidate(path: Path, *, since_ns: int) -> bool: Qualifies when the file is modified at/after ``since_ns``, is NOT the no-spec fallback (already matched by name on the marker path), carries ZERO real (non-fenced) marker headings — one would put it in `find_result_artifact`'s - territory — and has a frontmatter ``status:`` of ``done`` or ``blocked``. Any + territory — and has a terminal frontmatter ``status:`` (``done``, + ``blocked``, or ``awaiting-operator``). Any unreadable/undecodable read degrades to False, never an exception.""" if path.name.startswith(FALLBACK_RESULT_PREFIX): return False @@ -399,7 +420,7 @@ def is_frontmatter_candidate(path: Path, *, since_ns: int) -> bool: fm = read_frontmatter(path) except OSError: return False - return str(fm.get("status", "")).strip().lower() in (DONE, BLOCKED) + return str(fm.get("status", "")).strip().lower() in (DONE, BLOCKED, AWAITING_OPERATOR) def _atomic_write_spec(spec_path: Path, text: str) -> None: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 68928cfd..7d765dec 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -1312,6 +1312,8 @@ def _record_dev_spec(self, task: StoryTask, result_json: dict | None) -> None: def _review_and_commit( self, task: StoryTask, resume_result: SessionResult | None = None ) -> None: + if self._park_awaiting_operator(task): + return if not self.policy.review.enabled: # review.enabled = false: the bmad-dev-auto session's own inline # review is the only review; verify the deterministic gates + commit. @@ -1692,12 +1694,53 @@ def _salvage_review_timeout(self, task: StoryTask, result: SessionResult) -> boo self._commit(task) return True - def _skip_review_and_commit(self, task: StoryTask) -> None: + def _park_awaiting_operator(self, task: StoryTask) -> bool: + """Take a dev-declared ``awaiting-operator`` park down the commit path, + skipping the review loop. Returns True when it did (the caller is done). + + The review loop is skipped because there is nothing for it to converge + ON. A review pass is bmad-dev-auto re-invoked on the spec to second-guess + the diff and finalize `done`; a park's outstanding work is not in the diff + at all — it is outside the repo, in a human's hands — so every cycle would + either re-park (no progress, budget burned) or "fix" the park away by + finalizing `done`, which is the exact false-green the state exists to + prevent. What the loop DOES contribute, the deterministic gate, is kept: + this delegates to the same skip-review commit path, whose `_verify_review` + now holds the park to its (awaiting-operator, awaiting-operator) pair, a + non-empty action list, and the project's verify commands. Parked work + clears every check `done` work clears — no commit path skips verification. + + No `_defer` machinery: a park is a SUCCESS that commits, so there is no + stash, no rollback, and no ledger snapshot to unwind. (A park that fails + verification is not a park yet; it defers like any other unverified work, + through the shared path below.)""" + if not self._operator_park_enabled(): + return False + spec_path = Path(task.spec_file) if task.spec_file else None + if spec_path is None or not spec_path.is_file(): + return False + fm = self._observed_frontmatter(spec_path, task.story_key, "operator-park") + if fm is None or verify.status_of(fm) != verify.AWAITING_OPERATOR: + return False + # Latched BEFORE _commit's advance(COMMITTING) + _save, so a host death + # anywhere in the commit window resumes into _finalize_commit_phase with + # the actions already on the persisted task — and that arm re-derives + # AWAITING_OPERATOR from them with no change of its own (#115). + task.operator_actions = list(verify.operator_actions_of(fm)) + self._skip_review_and_commit(task, kind="review-skipped-awaiting-operator") + return True + + def _skip_review_and_commit(self, task: StoryTask, *, kind: str = "review-skipped") -> None: """review.enabled = false: no separate review session runs. The bmad-dev-auto session ran its own inline review and finalized the story to done. Validate the deterministic gates (verify commands, - spec/sprint = done) and commit, repairing once if verify is fixable.""" - self.journal.append("review-skipped", story_key=task.story_key) + spec/sprint = done) and commit, repairing once if verify is fixable. + + Also the commit path for an ``awaiting-operator`` park, which reaches + here with ``kind="review-skipped-awaiting-operator"`` — same gates, same + repair-once, same commit; only the reason the review was skipped differs, + and the journal says which.""" + self.journal.append(kind, story_key=task.story_key) outcome = self._verify_review(task) if not outcome.ok and outcome.fixable and self._fix_phase(task, outcome.reason): outcome = self._verify_review(task) @@ -1805,8 +1848,28 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: # re-raise untouched. self._restore_deferred_closes(task, snapshot) raise - advance(task, Phase.DONE) - self.journal.append("story-done", story_key=task.story_key, commit=task.commit_sha) + # Final-phase rule: AWAITING_OPERATOR iff the task carries actions, + # otherwise DONE. Derived from PERSISTED task state, never from a local + # flag, so the crash-resume arm that re-enters this method reaches the + # same verdict the pre-crash run would have — the park latch is the only + # thing that has to survive, and it does. + if task.operator_actions: + advance(task, Phase.AWAITING_OPERATOR) + # The durable record of what a human owes, for now: the actions + # themselves, not just a count, because this and the spec frontmatter + # are the only places they survive the run. The project-level registry + # `bmad-loop confirm` reads, and the park notification, land with that + # command (part 3) — they are its data store and its prompt, and + # shipping them here would leave a store nothing reads. + self.journal.append( + "story-awaiting-operator", + story_key=task.story_key, + commit=task.commit_sha, + actions=list(task.operator_actions), + ) + else: + advance(task, Phase.DONE) + self.journal.append("story-done", story_key=task.story_key, commit=task.commit_sha) self._emit("post_commit", task) self._save() @@ -1821,6 +1884,19 @@ def _generic_dev(self) -> bool: a future alternative dev skill can re-introduce the legacy branch.""" return self.policy.dev.skill == "bmad-dev-auto" + def _operator_park_enabled(self) -> bool: + """Whether a dev session may park a story at ``awaiting-operator`` (#335). + + Sprint mode only. Stories mode has no sprint board, so the pair the + verify gates hold a park to does not exist there + (``verify_review_stories`` still demands ``done``); sweep bundles carry + no board entry either, and a deferred-work bundle owing a human action is + a different question from a story owing one. Both subclasses override + this to False rather than inheriting a path their verify tail cannot + gate — support for either is a follow-up, not an accident of where the + branch happens to sit.""" + return self.policy.operator.enabled + def _dev_review_enabled(self) -> bool: """Spec-status/sprint semantics for verify_dev and the sprint sync. The generic skill always self-finalizes to ``done`` (no in-review handoff), so @@ -2048,7 +2124,12 @@ def _repair_spec_marker(self, task: StoryTask, rj: dict) -> 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: + # Same terminal set `devcontract.is_frontmatter_candidate` scans for: that + # scan is what FINDS a marker-less finalized spec, and this repair is what + # brings it back into contract. A status accepted by one and refused by + # the other would leave a park harvested but permanently un-markered. + terminal = (devcontract.DONE, devcontract.BLOCKED, devcontract.AWAITING_OPERATOR) + if fm_status not in terminal or fm_status != rj_status: self.journal.append( "spec-marker-repair-skipped", story_key=task.story_key, @@ -2110,7 +2191,22 @@ def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> Non fm = self._observed_frontmatter(spec_path, task.story_key, "post-dev-sync") if fm is None: return - if verify.status_of(fm) != success_status: + status = verify.status_of(fm) + # A park is the other terminal a session can reach, so it gets the same + # mirror: the board moves to `awaiting-operator`, which sits immediately + # below `done` in STATUS_ORDER and is therefore an ordinary FORWARD + # advance through the sole writer — no exception to never-regress, and + # `bmad-loop confirm` later advances the same board the same way. Mirrored + # on the STATUS alone, before the actions are validated: verify_dev owns + # the "declared nothing" retry, and its feedback reads far better against + # a board that already agrees with the spec than against one two stages + # behind it. + if self._operator_park_enabled() and status == verify.AWAITING_OPERATOR: + sprint_advance( + self.workspace.paths.sprint_status, task.story_key, verify.AWAITING_OPERATOR + ) + return + if status != success_status: return target = "review" if review_enabled else "done" sprint_advance(self.workspace.paths.sprint_status, task.story_key, target) @@ -2408,7 +2504,11 @@ def _after_story(self, task: StoryTask) -> None: def _verify_dev_artifacts(self, task: StoryTask, result_json: dict | None): return verify.verify_dev( - task, self.workspace.paths, result_json, review_enabled=self._dev_review_enabled() + task, + self.workspace.paths, + result_json, + review_enabled=self._dev_review_enabled(), + operator_park=self._operator_park_enabled(), ) def _verify_review(self, task: StoryTask): @@ -2421,6 +2521,7 @@ def _verify_review(self, task: StoryTask): self.workspace.paths, self.policy, sprint_reached_done=not self._dev_review_enabled(), + operator_park=self._operator_park_enabled(), ) def _review_prompt(self, task: StoryTask) -> str: @@ -2454,6 +2555,16 @@ def _render_commit_template(self, task: StoryTask) -> str | None: ) def _commit_message(self, task: StoryTask) -> str: + # The park suffix is appended to a rendered template too. The template + # governs the message's SHAPE; whether the story is finished is a fact + # about the commit, and `git log` is where an operator looks for it long + # after the notification scrolled past. A plugin that wants the suffix + # gone can still rewrite the whole message at pre_commit. + return self._base_commit_message(task) + ( + " (awaiting operator)" if task.operator_actions else "" + ) + + def _base_commit_message(self, task: StoryTask) -> str: rendered = self._render_commit_template(task) if rendered is not None: return rendered @@ -2861,8 +2972,8 @@ def _generic_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: f"`{task.spec_file}`. The attempted change was restored onto " f"the working tree after an intent-gap resolution; review it " f"against the amended spec." - ) - return f"/bmad-dev-auto {task.story_key}" + ) + self._operator_park_instruction() + return f"/bmad-dev-auto {task.story_key}" + self._operator_park_instruction() self._reset_spec_for_repair(task) spec_ref = task.spec_file or task.story_key return ( @@ -2871,6 +2982,40 @@ def _generic_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: f"verification; repair the working tree so verification passes without " f"changing the spec's frozen intent contract. Verification evidence is " f"in `{feedback}`." + ) + self._operator_park_instruction() + + def _operator_park_instruction(self) -> str: + """The park contract, injected into every dev prompt while + ``[operator] enabled``. "" when the feature is off. + + Engine-injected rather than skill-owned because the durable home for it is + upstream — bmad-dev-auto's spec template and step-03/04 finalize rules — + and that PR is not landed. This is the shipped interim: the same words the + skill will eventually carry, said by the orchestrator so the state is + reachable now. When upstream lands, this method is what goes away. + + The "never blocked" clause is the load-bearing half. `blocked` is the + skill's existing escape for "I cannot finish", and a human-only action is + exactly the shape that tempts it — but blocked HALTS the run, which is the + original defect: one story owing a DNS record stops every story behind + it. + + Deliberately backtick-free. It is appended AFTER the repair prompt's + feedback-file pointer, and the last backtick-wrapped token in a dev prompt + is by convention that path — a backticked word here would quietly become + the "feedback file" to anything reading the prompt back.""" + if not self._operator_park_enabled(): + return "" + return ( + " — If this story's acceptance criteria include actions only a HUMAN " + "can perform outside the repo (buy a domain, publish a DNS record, " + "grant an API key, click through a vendor console): complete every " + "part an agent CAN do, commit it, then finalize the spec frontmatter " + "to status: awaiting-operator and enumerate what is owed under an " + "operator_actions: key — a YAML list of strings, one imperative " + "instruction each, non-empty. Never use the blocked status for this: " + "blocked halts the whole run, and this story is finished as far as " + "you can take it." ) def _reset_spec_for_repair(self, task: StoryTask) -> None: diff --git a/src/bmad_loop/frontmatter.py b/src/bmad_loop/frontmatter.py index e478d476..f509adbe 100644 --- a/src/bmad_loop/frontmatter.py +++ b/src/bmad_loop/frontmatter.py @@ -72,6 +72,36 @@ def status_of(fm: dict[str, Any]) -> str: return str(fm.get("status", "")).strip().lower() +def operator_actions_of(fm: dict[str, Any]) -> tuple[str, ...]: + """The external, human-only actions a spec's ``operator_actions:`` frontmatter + declares — normalized, order-preserving, deduped. + + Lives here rather than in ``devcontract`` because both sides of the park + contract need the same reading and ``verify`` cannot import ``devcontract`` + (the dependency runs the other way). + + Strict about the container, lenient about each scalar item — the + ``closes_deferred`` reading (:func:`deferredwork.parse_declaration`), for the + same reason: a bare ``operator_actions: buy the domain`` is iterable, so a + lenient container reading would silently turn one instruction into a list of + characters. Items that are themselves containers are dropped rather than + stringified: ``[{action: ..., check: ...}]`` is the deliberate v2 shape (a + per-action verification command), and ``str()``-ing it would hand a human a + line of Python repr as their instruction. ``None`` items drop for the same + reason — ``str(None)`` is the word "None", not an action. + + Every malformed shape therefore collapses to ``()``, which the verify gates + read as "declared nothing" and answer with one fixable retry naming the + expected shape — a park is *defined* by owing at least one action, so an + empty reading can never be mistaken for a valid park. + """ + raw = fm.get("operator_actions") + if not isinstance(raw, list): + return () + items = (str(x).strip() for x in raw if x is not None and not isinstance(x, (list, dict))) + return tuple(dict.fromkeys(a for a in items if a)) + + def set_frontmatter_status(path: Path, status: str) -> bool: """Rewrite the `status:` field in a spec's `---`…`---` frontmatter block. diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index 12feecd7..c75086c5 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -272,6 +272,21 @@ class TuiPolicy: tasks_height: int = 0 # Tasks table height, rows (detail column) +@dataclass(frozen=True) +class OperatorPolicy: + # Whether a dev session may park a story at `awaiting-operator` — the + # terminal state for a story whose agent-doable work is finished and + # committed but whose acceptance criteria include external actions only a + # human can perform (#335). Default-on: without it such a story has no honest + # outcome, and the two it would otherwise take are both wrong — `done` hides + # the outstanding work behind a green board, `blocked` halts a run over work + # the loop was never going to do. Off, the engine injects no park + # instruction, never targets the sprint token, and the verify gates do not + # know the status, so a session that writes it anyway is retried with that + # mismatch as feedback rather than silently committing. + enabled: bool = True + + @dataclass(frozen=True) class MuxPolicy: # Terminal-multiplexer backend for THIS machine (the transport axis — which @@ -553,6 +568,7 @@ class Policy: cleanup: CleanupPolicy = field(default_factory=CleanupPolicy) plugins: PluginsPolicy = field(default_factory=PluginsPolicy) tui: TuiPolicy = field(default_factory=TuiPolicy) + operator: OperatorPolicy = field(default_factory=OperatorPolicy) mux: MuxPolicy = field(default_factory=MuxPolicy) def to_dict(self) -> dict[str, Any]: @@ -683,6 +699,7 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: engine_d = _section(doc, "engine") # deprecated; folded into [plugins] below plugins_d = _section(doc, "plugins") tui_d = _section(doc, "tui") + operator_d = _section(doc, "operator") mux_d = _section(doc, "mux") gates = GatesPolicy( @@ -979,6 +996,7 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: deferred_height=_tui_dim(tui_d, "deferred_height"), tasks_height=_tui_dim(tui_d, "tasks_height"), ) + operator = OperatorPolicy(enabled=bool(operator_d.get("enabled", OperatorPolicy.enabled))) mux = MuxPolicy(backend=str(mux_d.get("backend", MuxPolicy.backend)).strip()) if mux.backend and not _MUX_NAME_RE.match(mux.backend): raise PolicyError( @@ -998,6 +1016,7 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: cleanup=cleanup, plugins=plugins, tui=tui, + operator=operator, mux=mux, ) @@ -1211,6 +1230,15 @@ def _fold_deprecated_engine( # deferred_height = 0 # Deferred pane height, rows # tasks_height = 0 # Tasks table height, rows +[operator] +# Let a dev session park a story at `awaiting-operator`: its agent-doable work is +# finished and COMMITTED, but its acceptance criteria include external actions +# only a human can perform (buy a domain, publish a DNS record, grant an API +# key). The story commits, the run moves on to the next one, and what is owed is +# recorded in the story spec's `operator_actions:` frontmatter. Turn this off to +# hold sessions to the two older outcomes (done / blocked). +enabled = true + [mux] # Terminal-multiplexer backend for this machine (the transport axis — which # tmux-like program hosts sessions; independent of [adapter], the coding-CLI diff --git a/src/bmad_loop/stories_engine.py b/src/bmad_loop/stories_engine.py index 38f280bf..2502d8aa 100644 --- a/src/bmad_loop/stories_engine.py +++ b/src/bmad_loop/stories_engine.py @@ -491,6 +491,15 @@ def _verify_review(self, task: StoryTask): # story spec's own `done` frontmatter is authoritative. return verify.verify_review_stories(task, self.workspace.paths, self.policy) + def _operator_park_enabled(self) -> bool: + # Stories mode has no sprint board, so the (awaiting-operator, + # awaiting-operator) pair the park is verified against does not exist + # here and `verify_review_stories` still demands `done`. Parking without + # that gate would commit on the spec's word alone. Support is a follow-up + # (spec-status + registry only); until then the token is simply not a + # terminal this mode knows. + return False + # -------------------------------------------------------- HITL checkpoints def _drive_story(self, task: StoryTask, dev_resume: SessionResult | None = None) -> None: diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 41feda0a..f9ece68d 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -1291,6 +1291,13 @@ def _verify_review(self, task: StoryTask): ) return verify.verify_review_bundle(task, self.workspace.paths, self.policy) + def _operator_park_enabled(self) -> bool: + # A bundle carries no sprint-status entry, so the pair a park is verified + # against does not exist, and `verify_review_bundle` gates on closed dw + # ids instead. Whether a deferred-work bundle can owe a human action is a + # separate question from whether a story can; not answered here. + return False + def _commit_message(self, task: StoryTask) -> str: rendered = self._render_commit_template(task) if rendered is not None: diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index c40def16..e2f7db3f 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -21,7 +21,7 @@ from . import deferredwork from .bmadconfig import ProjectPaths from .frontmatter import set_frontmatter_status # noqa: F401 — re-export -from .frontmatter import _split_frontmatter, read_frontmatter, status_of +from .frontmatter import _split_frontmatter, operator_actions_of, read_frontmatter, status_of from .model import StoryTask, VerifyOutcome from .policy import POLICY_FILE, Policy from .sprintstatus import STATUS_ORDER, story_status @@ -1111,13 +1111,16 @@ def _verify_shared_gates( expected_status: str, extra_exclude: tuple[str, ...] | None, allow_ancestor_baseline: bool = False, + fm: dict[str, Any] | None = None, ) -> VerifyOutcome | None: """The workflow-tag, expected-status, baseline-match, and proof-of-work gates shared verbatim by :func:`verify_dev`, :func:`verify_dev_bundle`, and :func:`verify_dev_stories` — factored out so the sprint-mode and stories-mode - gates can't silently drift. Reads frontmatter once (callers must not re-read - it). Returns a failing :class:`VerifyOutcome`, or ``None`` when every gate - passes and the caller may run its mode-specific tail. + gates can't silently drift. Reads frontmatter once; a caller that had to read + it first to *choose* ``expected_status`` passes what it read as ``fm`` so the + single-read contract still holds (no caller re-reads it). Returns a failing + :class:`VerifyOutcome`, or ``None`` when every gate passes and the caller may + run its mode-specific tail. The proof-of-work exclude is derived here from the `task` this gate already receives (`verify_dev_exclude_relpaths`, which needs the latched restore patch); @@ -1133,9 +1136,11 @@ def _verify_shared_gates( f"dev result.json workflow is {workflow!r}, expected {DEV_WORKFLOW!r}" ) - fm = _gate_frontmatter(spec_path) - if isinstance(fm, VerifyOutcome): - return fm + if fm is None: + read = _gate_frontmatter(spec_path) + if isinstance(read, VerifyOutcome): + return read + fm = read status = status_of(fm) if status != expected_status: return VerifyOutcome.retry( @@ -1184,11 +1189,45 @@ def _verify_shared_gates( return None +# The terminal spec status of a story whose agent-doable work is finished but +# whose acceptance criteria include external actions only a human can perform +# (#335). Mirrors devcontract.AWAITING_OPERATOR, kept literal here for the same +# reason PLAN_HALT_STATUS is: devcontract imports verify, never the reverse. +AWAITING_OPERATOR = "awaiting-operator" + + +def _operator_actions_gate(fm: dict[str, Any], story_key: str) -> VerifyOutcome | None: + """Refuse a park that enumerates nothing, with feedback a repair session can + act on. ``None`` when the spec declares at least one usable action. + + A park is *defined* by owing external work: a spec at ``awaiting-operator`` + with no readable ``operator_actions:`` names no obligation, so confirming it + later would be a human acknowledging a blank. Every malformed shape reaches + here as an empty reading (:func:`frontmatter.operator_actions_of`), and all + of them have the same remedy, so one message covers them: name the actions, + or finalize the status that matches reality. ``fixable=True`` — the tree is + real work and the defect is one frontmatter block, so the reason goes to a + repair session as feedback rather than throwing the attempt away. + """ + if operator_actions_of(fm): + return None + return VerifyOutcome.retry( + f"spec for {story_key} is 'awaiting-operator' but declares no usable " + f"operator_actions: add a YAML list of strings naming each external " + f"action a human must perform, or finalize the status the work actually " + f"reached ('done' when nothing is owed, 'blocked' when the story cannot " + f"proceed)", + fixable=True, + ) + + def verify_dev( task: StoryTask, paths: ProjectPaths, result_json: dict[str, Any] | None, review_enabled: bool = True, + *, + operator_park: bool = False, ) -> VerifyOutcome: """Verify a dev session's on-disk artifacts against its result.json claims. @@ -1198,6 +1237,15 @@ def verify_dev( orchestrator's, has produced changes since that baseline, and that the story's sprint-status was advanced to the matching stage. Returns a retryable VerifyOutcome on any mismatch, escalates on git failure, passes otherwise. + + ``operator_park`` (``[operator] enabled``, engine-supplied) adds one more + accepted spec/sprint pair: ``(awaiting-operator, awaiting-operator)``, the + park a dev session declares when the story's remaining work is a human's + (#335). The OBSERVED spec status selects which pair is demanded — the skill + decides whether it parked, and the gate then holds it to the matching board + state and to a non-empty action list. Off by policy, the token is simply not + a terminal the gate knows, so it fails the ordinary status check and the + session is retried with that mismatch as feedback. """ rj = result_json or {} spec_file = rj.get("spec_file") @@ -1207,20 +1255,33 @@ def verify_dev( if not spec_path.is_file(): return VerifyOutcome.retry(f"claimed spec file does not exist: {spec_path}") + fm = _gate_frontmatter(spec_path) + if isinstance(fm, VerifyOutcome): + return fm + parked = operator_park and status_of(fm) == AWAITING_OPERATOR + if parked: + actions = _operator_actions_gate(fm, task.story_key) + if actions is not None: + return actions + # With review disabled, the dev session runs its own internal review and - # finalizes straight to done; otherwise it hands off at in-review. + # finalizes straight to done; otherwise it hands off at in-review. A park + # short-circuits both: the story is finished as far as any agent can take it. gate = _verify_shared_gates( spec_path, rj, task, paths, - expected_status="in-review" if review_enabled else "done", + expected_status=( + AWAITING_OPERATOR if parked else ("in-review" if review_enabled else "done") + ), extra_exclude=(), + fm=fm, ) if gate is not None: return gate - expected_sprint = "review" if review_enabled else "done" + expected_sprint = AWAITING_OPERATOR if parked else ("review" if review_enabled else "done") sprint = story_status(paths.sprint_status, task.story_key) if sprint != expected_sprint: return VerifyOutcome.retry( @@ -1597,6 +1658,7 @@ def verify_review( policy: Policy, *, sprint_reached_done: bool = False, + operator_park: bool = False, ) -> VerifyOutcome: """Gate a completed review pass: spec at ``done``, sprint-status at ``done``, deterministic verify commands green. @@ -1610,19 +1672,39 @@ def verify_review( the board, so retrying only replays the same failure until the budget runs out and the work is rolled back; under ``review.on_status_contradiction = "escalate"`` (the default) the gate - escalates instead, naming both sides. See #334.""" + escalates instead, naming both sides. See #334. + + ``(awaiting-operator, awaiting-operator)`` is the second accepted pair, on + the same observed-spec-status selection ``verify_dev`` uses: this is the gate + the park path runs before committing (``Engine._park_awaiting_operator``), so + parked work clears exactly the deterministic checks every other commit path + clears — the pair, a non-empty action list, and the verify commands. The + sign-off-regression arm stays scoped to the ``done`` pair: a board short of + ``awaiting-operator`` is a stage never reached, not a revoked sign-off. + + ``operator_park`` is the SAME engine-supplied flag ``verify_dev`` takes, not a + second reading of ``policy.operator.enabled``, so the two gates cannot + disagree about whether this run parks. They would: the engine's + ``_operator_park_enabled`` is an override seam, and a mode that opts out of + parking while still reaching this gate would otherwise find it accepting a + park the engine itself refuses to take.""" if not task.spec_file: return VerifyOutcome.retry("no spec file recorded for task") fm = _gate_frontmatter(Path(task.spec_file)) if isinstance(fm, VerifyOutcome): return fm status = status_of(fm) - if status != "done": - return VerifyOutcome.retry(f"spec status is {status!r}, expected 'done'") + expected = AWAITING_OPERATOR if (operator_park and status == AWAITING_OPERATOR) else "done" + if status != expected: + return VerifyOutcome.retry(f"spec status is {status!r}, expected {expected!r}") + if expected == AWAITING_OPERATOR: + actions = _operator_actions_gate(fm, task.story_key) + if actions is not None: + return actions sprint = story_status(paths.sprint_status, task.story_key) - if sprint != "done": - if _is_signoff_regression(sprint, sprint_reached_done, policy): + if sprint != expected: + if expected == "done" and _is_signoff_regression(sprint, sprint_reached_done, policy): return VerifyOutcome.escalate( f"review revoked the sprint sign-off for {task.story_key}: the " f"orchestrator advanced the board to 'done' after dev verified, " @@ -1639,7 +1721,7 @@ def verify_review( contradiction=True, ) return VerifyOutcome.retry( - f"sprint-status for {task.story_key} is {sprint!r}, expected 'done'" + f"sprint-status for {task.story_key} is {sprint!r}, expected {expected!r}" ) return verify_commands_outcome(policy, paths.project) diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 92c3a380..0ed7f8eb 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -523,7 +523,12 @@ def failed_diff_max_bytes(self) -> int | None: def integrate_unit(self, task: StoryTask, unit: UnitWorkspace) -> None: self._emit("pre_integrate", task) scm = self.policy.scm - if task.phase == Phase.DONE: + # AWAITING_OPERATOR merges beside DONE: a parked story CARRIES A COMMIT + # (that is what separates it from DEFERRED/ESCALATED), and stranding that + # commit on a torn-down unit branch would lose finished work over an + # obligation that lives outside the repo entirely. The human's remaining + # actions are recorded in the registry, not in this worktree. + if task.phase in (Phase.DONE, Phase.AWAITING_OPERATOR): # Merge the unit branch into the target branch locally. We open PRs # ourselves by hand once the branch has landed; the orchestrator only # commits the worktree onto the selected target. diff --git a/tests/conftest.py b/tests/conftest.py index 8bc487e2..885501ba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -369,6 +369,7 @@ def write_spec( *, prose_status: str | None = None, closes_deferred: object = None, + operator_actions: object = None, ) -> None: """Write a spec the way the real bmad-dev-auto skill does. The skill's step-03 stamps `baseline_revision` and NEVER `baseline_commit` (that name exists only @@ -379,12 +380,24 @@ def write_spec( ``closes_deferred`` writes the story-declared ledger-closure field (#234): a list renders as a YAML flow sequence, and a bare string renders as a scalar — the wrong-container mistake whose handling must not depend on which file it - was made in.""" + was made in. + + ``operator_actions`` writes the park declaration (#335) as a real YAML BLOCK + sequence — the shape the dev prompt asks a session for, and the shape a human + reads back — so the reading under test is the one production sees. A non-list + renders as a scalar, which is the wrong-container mistake for this field + too (a bare string is iterable, so a lenient reader would turn one + instruction into a list of characters).""" declare = "" if isinstance(closes_deferred, list): declare = f"closes_deferred: [{', '.join(closes_deferred)}]\n" elif closes_deferred is not None: declare = f"closes_deferred: {closes_deferred}\n" + if isinstance(operator_actions, list): + rendered = "".join(f" - {a}\n" for a in operator_actions) + declare += f"operator_actions:\n{rendered}" if rendered else "operator_actions: []\n" + elif operator_actions is not None: + declare += f"operator_actions: {operator_actions}\n" body = ( f"---\ntitle: 'test'\ntype: 'feature'\nstatus: '{status}'\n" f"baseline_revision: '{baseline}'\n{declare}---\n\n## Intent\n\ntest spec\n" @@ -402,7 +415,9 @@ def spec_path(paths: ProjectPaths, story_key: str) -> Path: return paths.implementation_artifacts / f"spec-{story_key}.md" -def committing_crash_state(paths: ProjectPaths, engine, *, post_squash: bool = False) -> str: +def committing_crash_state( + paths: ProjectPaths, engine, *, post_squash: bool = False, operator_actions: list | None = None +) -> str: """Persist the exact state.json shape from issue #115: a task at COMMITTING (the save right after advance(COMMITTING), before finalize_commit / the DONE save that stamps commit_sha). Fully verified on disk: attempt work committed @@ -411,16 +426,23 @@ def committing_crash_state(paths: ProjectPaths, engine, *, post_squash: bool = F done, sprint synced at DEV time. review_cycle stays 0 — the _skip_review_and_commit path reaches COMMITTING with zero review sessions. With post_squash, finalize_commit already ran before the death (squashed - commit at HEAD, clean tree) but commit_sha was never persisted. Returns the - baseline sha.""" + commit at HEAD, clean tree) but commit_sha was never persisted. + + With ``operator_actions``, the crashed story is a PARK (#335): spec + board at + `awaiting-operator` and the actions already latched on the task, exactly as + `_park_awaiting_operator` leaves it before `advance(COMMITTING)`. That latch + is the whole point — it is what the resume arm re-derives the final phase + from, with no code of its own. Returns the baseline sha.""" + parked = bool(operator_actions) + status = "awaiting-operator" if parked else "done" baseline = rev_parse_head(paths.project) src = paths.project / "src.txt" src.write_text(src.read_text() + "change for 1-1-a\n") git(paths.project, "add", "src.txt") git(paths.project, "commit", "-q", "-m", "attempt work for 1-1-a") sp = spec_path(paths, "1-1-a") - write_spec(sp, "done", baseline) - write_sprint(paths, {"1-1-a": "done"}) + write_spec(sp, status, baseline, operator_actions=operator_actions) + write_sprint(paths, {"1-1-a": status}) if post_squash: finalize_commit(paths.project, baseline, "pre-crash squash") @@ -429,6 +451,7 @@ def committing_crash_state(paths: ProjectPaths, engine, *, post_squash: bool = F task.baseline_commit = baseline task.baseline_untracked = [] task.spec_file = str(sp) + task.operator_actions = list(operator_actions or []) task.record_session( SessionRecord( task_id="1-1-a-dev-1", @@ -459,6 +482,7 @@ def dev_effect( seen: list[str] | None = None, write_src: bool = True, closes_deferred: object = None, + operator_actions: object = None, ): """Simulate a successful bmad-dev-auto session: it self-finalizes the spec (no in-review handoff — always straight to ``done``) but never touches the @@ -472,6 +496,11 @@ def dev_effect( Status line — pair it with a non-terminal ``final_status`` to reproduce the skill leaving frontmatter behind its prose (the reconcile path). + ``operator_actions`` pairs with ``final_status="awaiting-operator"`` to + simulate a session that finished its agent-doable work and parked the rest on + a human (#335); it is written to the spec frontmatter only, never to + result.json, because the engine re-reads the spec for it. + ``seen``, when given, collects `src.txt` as the session found it on entry — the patch-restore tests assert the re-driven session ran against the RESTORED diff. ``write_src=False`` then keeps the session from appending its own line, so what @@ -487,7 +516,12 @@ def effect(spec: SessionSpec) -> SessionResult: source.write_text(source.read_text() + f"change for {story_key}\n") sp = spec_path(paths, story_key) write_spec( - sp, final_status, baseline, prose_status=prose_status, closes_deferred=closes_deferred + sp, + final_status, + baseline, + prose_status=prose_status, + closes_deferred=closes_deferred, + operator_actions=operator_actions, ) # deliberately NO set_sprint: the dev skill does not write sprint-status return SessionResult( diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index a04f3173..4879ee0e 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -17,12 +17,17 @@ def _spec( auto_run: str | None = "done", body_extra: str = "", followup: bool | None = None, + actions: str | None = None, ) -> Path: fm = f"---\ntitle: 'x'\ntype: 'feature'\nstatus: '{status}'\n" if baseline: fm += f"{baseline_field}: '{baseline}'\n" if followup is not None: fm += f"followup_review_recommended: {str(followup).lower()}\n" + if actions is not None: + # raw YAML: these tests are ABOUT the container/item shapes, so the + # fixture must be able to write a malformed one verbatim + fm += f"operator_actions: {actions}\n" fm += "---\n\n## Intent\n\nwhatever\n" text = fm + body_extra if auto_run is not None: @@ -217,6 +222,82 @@ def test_synth_followup_review_recommended_omitted_on_blocked(tmp_path): assert "followup_review_recommended" not in out.result_json +# ------------------------------------------- awaiting-operator terminal (#335) + + +def test_synth_awaiting_operator_is_terminal_and_folds_actions(tmp_path): + """The park is a third terminal beside done/blocked, and its obligations + travel with the result so the engine never re-reads the spec to learn them.""" + sp = _spec( + tmp_path / "s.md", + status="awaiting-operator", + auto_run="awaiting-operator", + actions="['buy example.com', 'publish the TXT record']", + ) + out = devcontract.synthesize_result(sp, story_key="1-1-a") + + assert out.status_consistent + rj = out.result_json + assert rj is not None and rj["status"] == "awaiting-operator" + assert rj["operator_actions"] == ["buy example.com", "publish the TXT record"] + + +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.""" + 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 + + +@pytest.mark.parametrize( + "actions, expected, why", + [ + ("['a', 'b']", ["a", "b"], "the normal shape"), + ("[' a ', '', 'a']", ["a"], "blanks drop, duplicates collapse, order preserved"), + ("[5]", ["5"], "a non-string scalar is str()-normalized, like closes_deferred ids"), + ("buy the domain", [], "a bare string is the wrong CONTAINER, never one action"), + ("[]", [], "an empty list declares nothing"), + ("[{action: a, check: b}]", [], "the v2 object shape is refused, not str()-ed to junk"), + ("[null]", [], "a null item is not the word 'None'"), + ], +) +def test_synth_operator_actions_shapes(tmp_path, actions, expected, why): + """Strict about the container, lenient about each scalar item — every + malformed shape collapses to [], which verify's non-empty gate turns into one + fixable retry naming the expected shape.""" + sp = _spec(tmp_path / "s.md", status="awaiting-operator", auto_run=None, actions=actions) + out = devcontract.synthesize_result(sp, story_key="1-1-a") + + assert out.result_json["operator_actions"] == expected, why + + +@pytest.mark.parametrize("status", ["done", "blocked"]) +def test_synth_operator_actions_folded_only_on_the_park_status(tmp_path, status): + """An `operator_actions:` list left behind on a done or blocked spec is not a + park. Carrying it would let a story register obligations no verify gate ever + held it to — and, on `done`, would make _finalize_commit_phase's final-phase + rule park a story the session declared finished.""" + sp = _spec(tmp_path / "s.md", status=status, auto_run=status, actions="['stale leftover']") + out = devcontract.synthesize_result(sp, story_key="1-1-a") + + assert "operator_actions" not in out.result_json + + +def test_frontmatter_candidates_include_awaiting_operator(tmp_path): + """The missing-marker fallback scan must find a parked spec too: the skill's + marker append is intermittent on EVERY terminal path, and a park invisible to + the harvest rides stall-nudges to timeout and loses its work.""" + sp = _spec(tmp_path / "spec-1-1-a.md", status="awaiting-operator", auto_run=None) + + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [sp] + + # --------------------------------------------------- plan-halt expected-terminal diff --git a/tests/test_engine.py b/tests/test_engine.py index b648862c..d0d51e89 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -48,9 +48,11 @@ ) from bmad_loop.policy import ( AdapterPolicy, + DevPolicy, GatesPolicy, LimitsPolicy, NotifyPolicy, + OperatorPolicy, Policy, ReviewPolicy, ScmPolicy, @@ -59,6 +61,7 @@ VerifyPolicy, ) from bmad_loop.runs import STOP_REQUEST_FILE, graceful_stop_requested, rearm_escalation +from bmad_loop.sprintstatus import story_status from bmad_loop.verify import ( GitError, PrunePreserveError, @@ -1221,6 +1224,183 @@ def test_run_summary_render_names_parked_stories_only_when_there_are_any(project assert "1 awaiting operator" in engine.summary().render() +# ---------------------------------------- awaiting-operator park path (#335) + + +def _park_policy(**kw): + """review.trigger = "always" so the park has a review loop to SKIP. Under the + "recommended" default a story that recommends no follow-up already skips it, + and the review-skip branch could be deleted without any test noticing.""" + return Policy( + **{ + "gates": GatesPolicy(mode="none"), + "notify": QUIET, + "review": ReviewPolicy(trigger="always"), + "dev": DevPolicy(skill="bmad-dev-auto"), + "scm": ScmPolicy(rollback_on_failure=True), + **kw, + } + ) + + +ACTIONS = ["buy example.com at the registrar", "publish the _acme-challenge TXT record"] + + +def test_dev_declared_park_commits_and_the_run_continues(project): + """The whole point, end to end: a session finishes what an agent can do and + hands the rest to a human. The story COMMITS (that is what separates a park + from a defer), the board reaches the token, the journal records what is owed, + and the run moves on to the next story rather than halting — one story owing + a DNS record must never block the ones behind it. + + Two ablation targets. Delete the `_park_awaiting_operator` branch in + `_review_and_commit` and this fails by requesting an unscripted review + session (trigger="always"). Delete the final-phase rule in + `_finalize_commit_phase` and the story lands DONE with its actions unrecorded + — a false green, the exact defect the state exists to prevent.""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [ + generic_dev_effect( + project, "1-1-a", final_status="awaiting-operator", operator_actions=ACTIONS + ), + generic_dev_effect(project, "1-2-b"), + review_effect(project, "1-2-b", clean=True), + ], + policy=_park_policy(), + ) + + summary = engine.run() + + parked = engine.state.tasks["1-1-a"] + assert parked.phase == Phase.AWAITING_OPERATOR + assert parked.operator_actions == ACTIONS + assert parked.commit_sha and parked.commit_sha != parked.baseline_commit + assert summary.awaiting_operator == 1 and not summary.paused + # the run continued: the next story was driven and finished + assert engine.state.tasks["1-2-b"].phase == Phase.DONE + assert summary.done == 1 and summary.deferred == 0 and summary.escalated == 0 + # no review session for the parked story — only its dev pass and 1-2-b's + assert [(s.role, s.task_id.split("-dev")[0]) for s in adapter.sessions if s.role == "dev"] == [ + ("dev", "1-1-a"), + ("dev", "1-2-b"), + ] + assert not any(s.role == "review" and "1-1-a" in s.task_id for s in adapter.sessions) + # the board reached the token — a forward advance through the sole writer + assert story_status(project.sprint_status, "1-1-a") == "awaiting-operator" + # the work is really in HEAD, not stashed on a recovery ref + assert "change for 1-1-a" in (project.project / "src.txt").read_text() + assert parked.preserve_ref is None and parked.defer_reason is None + + kinds = [e["kind"] for e in engine.journal.entries()] + assert "review-skipped-awaiting-operator" in kinds and "story-awaiting-operator" in kinds + assert "story-deferred" not in kinds and "story-done" in kinds # 1-2-b's + # the journal entry carries the actions themselves, not a count: with the + # registry not yet shipped (part 3), it and the spec are the only records + parked_entry = next( + e for e in engine.journal.entries() if e["kind"] == "story-awaiting-operator" + ) + assert parked_entry["actions"] == ACTIONS and parked_entry["commit"] == parked.commit_sha + + +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( + project, + [ + generic_dev_effect( + project, "1-1-a", final_status="awaiting-operator", operator_actions=ACTIONS + ) + ], + policy=_park_policy(), + ) + + engine.run() + + # one commit, the story's own, marked so `git log` still says what the run + # summary said long after the summary scrolled past + assert git(project.project, "log", "-1", "--format=%s").strip().endswith("(awaiting operator)") + assert engine.state.tasks["1-1-a"].commit_sha == rev_parse_head(project.project) + assert worktree_clean(project.project) + + +def test_park_without_usable_actions_is_repaired_not_committed(project): + """A spec at the park status declaring nothing is not a park yet. verify's + non-empty gate is fixable, so a repair session runs with the reason as + feedback; here it finalizes `done` honestly and the story commits as DONE. + + Ablation: delete the non-empty gate in `_operator_actions_gate` and this + fails — the blank park verifies green and commits as AWAITING_OPERATOR with + an empty obligation nobody can ever confirm.""" + 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=[] + ), + generic_dev_effect(project, "1-1-a", followup_review=False), + ], + policy=_park_policy(review=ReviewPolicy(enabled=False)), + ) + + summary = engine.run() + + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.DONE and task.operator_actions == [] + assert summary.done == 1 and summary.awaiting_operator == 0 + assert len(adapter.sessions) == 2 # the park attempt, then the repair + kinds = [e["kind"] for e in engine.journal.entries()] + assert "story-awaiting-operator" not in kinds and "story-done" in kinds + + +def test_park_disabled_by_policy_never_commits_the_token(project): + """`[operator] enabled = false` does not reinterpret the token — it makes it + unknown. The gate rejects it, the attempt budget runs out, and the story + defers with its work preserved rather than committing a phase the operator + turned off.""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + script = [ + generic_dev_effect( + project, "1-1-a", final_status="awaiting-operator", operator_actions=ACTIONS + ) + ] * 3 + engine, _ = make_engine( + project, script, policy=_park_policy(operator=OperatorPolicy(enabled=False)) + ) + + summary = engine.run() + + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.DEFERRED and summary.awaiting_operator == 0 + assert story_status(project.sprint_status, "1-1-a") != "awaiting-operator" + assert "story-awaiting-operator" not in [e["kind"] for e in engine.journal.entries()] + + +def test_resume_through_committing_re_derives_the_park(project): + """The crash-resume contract (#115) needs no park-specific code: the actions + are latched BEFORE `advance(COMMITTING)`, so the resume arm re-enters + `_finalize_commit_phase` and its final-phase rule reaches the same verdict the + pre-crash run would have. Ablation: latch the actions AFTER the advance and + the resumed story lands DONE, silently dropping the obligation.""" + engine, _ = make_engine(project, []) + baseline = committing_crash_state(project, engine, operator_actions=ACTIONS) + + resumed, adapter = resume_engine(project, engine, []) + summary = resumed.run() + + final = load_state(resumed.run_dir).tasks["1-1-a"] + assert final.phase == Phase.AWAITING_OPERATOR + assert final.operator_actions == ACTIONS + assert final.commit_sha == rev_parse_head(project.project) != baseline + assert summary.awaiting_operator == 1 and not summary.crashed + assert adapter.sessions == [] # no session re-run, gates included + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "resume-commit" in kinds and "story-awaiting-operator" in kinds + assert "story-done" not in kinds + + def test_run_summary_render_labels_both_units(project): """render() feeds stdout, the ATTENTION file and the desktop notification from one place, so this covers all three.""" @@ -1731,9 +1911,11 @@ def test_generic_dev_path_orchestrator_advances_sprint(project): assert engine.state.tasks["1-1-a"].phase == Phase.DONE # the orchestrator advanced sprint-status, not the skill assert story_status(project.sprint_status, "1-1-a") == "done" - # the generic dev invocation form + # the generic dev invocation form, plus the engine-injected awaiting-operator + # contract (#335) every dev prompt carries while [operator] enabled assert [s.role for s in adapter.sessions] == ["dev"] - assert adapter.sessions[0].prompt == "/bmad-dev-auto 1-1-a" + assert adapter.sessions[0].prompt.startswith("/bmad-dev-auto 1-1-a") + assert "status: awaiting-operator" in adapter.sessions[0].prompt def test_generic_dev_path_no_sprint_advance_when_spec_unfinalized(project): @@ -2969,7 +3151,10 @@ def test_expected_spec_pinned_only_when_the_prompt_names_the_spec(project): # carries a recorded spec, but the dispatch is a bare story key — the session is # free to write a different spec, and the scan is the only way to find it. fresh = engine._dev_prompt(task, None) - assert fresh == "/bmad-dev-auto 1-1-a" + # the dispatch itself is a bare key; the engine-injected awaiting-operator + # contract (#335) rides along but names no path, which is the property the + # pin reads + assert fresh.startswith("/bmad-dev-auto 1-1-a") assert _pin_probe(project, fresh, spec_file=owed) is None diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index b1c6f70a..85415d70 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -59,7 +59,13 @@ def commit_sprint(project, statuses: dict[str, str]) -> None: def wt_dev_effect( - project, story_key, *, final_status="done", followup_review=True, closes_deferred=None + project, + story_key, + *, + final_status="done", + followup_review=True, + closes_deferred=None, + operator_actions=None, ): """Dev session running inside the unit worktree (spec.cwd). Mirrors the bmad-dev-auto skill: self-finalizes the spec to done, never writes the sprint @@ -74,7 +80,13 @@ def effect(spec): src = cwd / "src.txt" src.write_text(src.read_text() + f"change for {story_key}\n") sp = wt.implementation_artifacts / f"spec-{story_key}.md" - write_spec(sp, final_status, baseline, closes_deferred=closes_deferred) + write_spec( + sp, + final_status, + baseline, + closes_deferred=closes_deferred, + operator_actions=operator_actions, + ) # NO set_sprint: the orchestrator is the single sprint-status writer return SessionResult( status="completed", @@ -199,6 +211,38 @@ def test_worktree_happy_path_merges_to_target(project): assert "worktree-teardown-degraded" not in kinds +def test_worktree_parked_unit_merges_like_a_done_one(project): + """`integrate_unit` branches on DONE-vs-everything-else, and a park is the one + non-DONE terminal that CARRIES A COMMIT. Left on the else arm the unit is torn + down as failed and finished work is stranded on a deleted branch — over an + obligation that lives outside the repo entirely. + + Ablation: narrow the merge test back to `== Phase.DONE` and this fails with + the story's change absent from the target branch.""" + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + actions = ["publish the _acme-challenge TXT record"] + engine, _ = make_engine( + project, + [ + wt_dev_effect( + project, "1-1-a", final_status="awaiting-operator", operator_actions=actions + ) + ], + ) + + summary = engine.run() + + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.AWAITING_OPERATOR and summary.awaiting_operator == 1 + # the merge happened: the unit's work is on the target branch, not stranded + assert "change for 1-1-a" in (project.project / "src.txt").read_text() + assert [p.resolve() for p in worktree_list(project.project)] == [project.project.resolve()] + assert worktree_clean(project.project) + kinds = journal_kinds(engine) + assert "unit-merged" in kinds and "story-awaiting-operator" in kinds + assert "unit-closed" not in kinds # the failed-unit teardown arm never ran + + def test_worktree_run_dir_is_outside_worktree(project): commit_sprint(project, {"1-1-a": "ready-for-dev"}) engine, _ = make_engine( diff --git a/tests/test_policy.py b/tests/test_policy.py index 169c404b..c2b0805b 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -766,6 +766,32 @@ def test_to_dict_roundtrips_for_snapshot(): assert snapshot["limits"]["max_followup_reviews"] == 1 +# --------------------------------------------------------------------------- +# [operator] — awaiting-operator parks (issue #335) + + +def test_operator_park_defaults_on(): + """Default-on because without it a story owing a human action has no honest + outcome: `done` hides the outstanding work, `blocked` halts the run over work + the loop was never going to do.""" + assert policy.loads("").operator.enabled is True + + +def test_operator_park_can_be_turned_off(): + assert policy.loads("[operator]\nenabled = false\n").operator.enabled is False + + +def test_operator_scalar_section_rejected(): + with pytest.raises(policy.PolicyError, match=r"\[operator\] must be a table"): + policy.loads('operator = "on"\n') + + +def test_template_operator_block_parses_to_the_default(): + # unlike [mux]'s commented anchor, this key ships uncommented — the template + # must therefore agree with the dataclass, not merely parse + assert policy.loads(policy.POLICY_TEMPLATE).operator.enabled is True + + # --------------------------------------------------------------------------- # [mux] — machine-scoped terminal-multiplexer backend choice (issue #87) diff --git a/tests/test_settings_schema.py b/tests/test_settings_schema.py index 8d22464f..e57d33b5 100644 --- a/tests/test_settings_schema.py +++ b/tests/test_settings_schema.py @@ -33,6 +33,7 @@ LimitsPolicy, MuxPolicy, NotifyPolicy, + OperatorPolicy, PluginsPolicy, Policy, ReviewPolicy, @@ -63,6 +64,7 @@ "scm": ScmPolicy, "cleanup": CleanupPolicy, "tui": TuiPolicy, + "operator": OperatorPolicy, "mux": MuxPolicy, "dev": DevPolicy, } diff --git a/tests/test_verify.py b/tests/test_verify.py index be543f61..bc0cbb7a 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -324,6 +324,141 @@ def test_verify_dev_review_disabled_expects_done(project): assert not out.ok and "expected 'done'" in out.reason +# ------------------------------------------- awaiting-operator pair (#335) + + +def _park(project, *, sprint="awaiting-operator", actions=("publish the TXT record",)): + """A dev session that finished its agent-doable work and parked the rest.""" + write_sprint(project, {"1-1-a": sprint}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "awaiting-operator", task.baseline_commit, operator_actions=list(actions)) + (project.project / "src.txt").write_text("changed\n") + return task, sp + + +def test_verify_dev_accepts_the_park_pair(project): + """The third accepted pair, selected by the OBSERVED spec status: the skill + decides whether it parked, and the gate then holds it to the matching board + state. `review_enabled` is irrelevant here — a park short-circuits both the + in-review handoff and the straight-to-done finish.""" + task, sp = _park(project) + + out = verify.verify_dev(task, project, dev_result(sp), review_enabled=False, operator_park=True) + + assert out.ok + assert task.spec_file == str(sp) + + +def test_verify_dev_park_needs_the_matching_board(project): + """Pair, not either half: a parked spec over a board the orchestrator never + advanced is a sync that did not land, and committing on the spec's word alone + would leave `bmad-loop confirm` advancing a board that never reached the + token.""" + task, sp = _park(project, sprint="in-progress") + + out = verify.verify_dev(task, project, dev_result(sp), review_enabled=False, operator_park=True) + + assert not out.ok and out.retryable + assert "expected 'awaiting-operator'" in out.reason + + +@pytest.mark.parametrize( + "actions, why", + [ + ([], "an empty list declares nothing"), + (["", " "], "blanks are not actions"), + ("buy the domain", "a bare string is the wrong container"), + ], +) +def test_verify_dev_park_without_usable_actions_retries_fixable(project, actions, why): + """A park is DEFINED by owing at least one action, so a spec at the status + with nothing readable under `operator_actions:` is refused — confirming it + later would be a human acknowledging a blank. + + `fixable=True` is the load-bearing half: the tree is real work and the defect + is one frontmatter block, so the reason goes to a repair session as feedback + instead of throwing the attempt away. This is the ablation detector for the + non-empty gate — delete it and the malformed park verifies green.""" + write_sprint(project, {"1-1-a": "awaiting-operator"}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "awaiting-operator", task.baseline_commit, operator_actions=actions) + (project.project / "src.txt").write_text("changed\n") + + out = verify.verify_dev(task, project, dev_result(sp), review_enabled=False, operator_park=True) + + assert not out.ok and out.retryable, why + assert out.fixable is True + assert "operator_actions" in out.reason and "YAML list of strings" in out.reason + + +def test_verify_dev_park_unknown_when_the_policy_is_off(project): + """`[operator] enabled = false` does not make the token mean something else — + it makes it mean nothing. The ordinary status gate then rejects it, and the + session is retried with that mismatch as feedback rather than committing.""" + task, sp = _park(project) + + out = verify.verify_dev(task, project, dev_result(sp), review_enabled=False) + + assert not out.ok and out.retryable + assert "'awaiting-operator'" in out.reason and "expected 'done'" in out.reason + + +def test_verify_review_accepts_the_park_pair(project): + """The gate the park path runs before committing: parked work clears the same + deterministic checks `done` work clears.""" + task, sp = _park(project) + task.spec_file = str(sp) + + out = verify.verify_review(task, project, Policy(), operator_park=True) + + assert out.ok + + +def test_verify_review_park_refused_without_the_engine_flag(project): + """The flag is the SAME one `verify_dev` takes, not a second reading of + `policy.operator.enabled`, so the two gates cannot disagree about whether + this run parks — `Engine._operator_park_enabled` is an override seam, and a + mode that opts out (stories, sweep) while still reaching this gate would + otherwise find it accepting a park the engine itself refuses to take. + + Conservative default, like `sprint_reached_done`: a caller that says nothing + gets the pre-#335 behavior.""" + task, sp = _park(project) + task.spec_file = str(sp) + + out = verify.verify_review(task, project, Policy()) # operator_park defaults False + + assert not out.ok and out.retryable + assert "'awaiting-operator'" in out.reason and "expected 'done'" in out.reason + + +def test_verify_review_park_requires_actions(project): + task, sp = _park(project, actions=[]) + task.spec_file = str(sp) + + out = verify.verify_review(task, project, Policy(), operator_park=True) + + assert not out.ok and out.retryable and out.fixable is True + assert "operator_actions" in out.reason + + +def test_verify_review_park_board_short_is_a_plain_retry_not_a_contradiction(project): + """The sign-off-regression arm (#334) stays scoped to the `done` pair. A board + short of `awaiting-operator` is a stage never reached — escalating it would + halt a run over a sync that simply has not landed.""" + task, sp = _park(project, sprint="in-progress") + task.spec_file = str(sp) + + out = verify.verify_review( + task, project, Policy(), sprint_reached_done=True, operator_park=True + ) + + assert not out.ok and out.retryable and out.contradiction is False + assert out.reason == "sprint-status for 1-1-a is 'in-progress', expected 'awaiting-operator'" + + def test_verify_dev_review_disabled_rejects_review_sprint(project): # Skip-review finalizes the sprint to 'done'; a run that left it at 'review' # must not slip through the sprint-status gate.