diff --git a/CHANGELOG.md b/CHANGELOG.md index 87da55d1..31a81d6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,20 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Changed +- **A review that revokes the sprint sign-off now escalates instead of burning review cycles + (#334).** The orchestrator advances `sprint-status` to `done` at dev time; a review session that + judges the story unfinished and writes the board back to an earlier stage contradicts that — and + nothing in the review loop re-advances it, so every remaining cycle re-read the same failure and + the story ended deferred with its work rolled back. That regression is now detected at the + review-verify gate and pauses the run with both sides named and the two ways out (finish the work + and re-arm, or accept the story and advance the board). Keys on `sprint-status` only — the spec's + own frontmatter legitimately cycles, and `status: blocked` stays the sanctioned hand-back channel + — and stays conservative: without the orchestrator's own launch-time sign-off, or on a status + outside the known lifecycle, it falls back to the old retry. New knob + `[review] on_status_contradiction`, default `escalate`; `retry` restores the previous + burn-cycles-then-defer behavior verbatim. Review prompts are unchanged by design: forbidding the + revert would make a correct reviewer comply and the story would commit without sign-off. + - **`bmad-loop-setup` stops registering BMAD config; the installer owns it (#258).** The skill wrote `_bmad/config.yaml`, `_bmad/config.user.yaml` and a root `_bmad/module-help.csv` — the pre-v6.10 layout, which BMAD's own resolver never reads (it merges four TOML layers, and diff --git a/README.md b/README.md index 5ec4d9bc..e8e29931 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,10 @@ enabled = true # false = skip the separate review session; the dev p trigger = "recommended" # when enabled: "recommended" runs the separate review only when # bmad-dev-auto flags followup_review_recommended; "always" = every story # (the loop is bounded by limits.max_review_cycles either way) +on_status_contradiction = "escalate" + # a review that writes sprint-status back off "done" (revoking the + # sign-off the orchestrator recorded at dev time) pauses the run naming + # both sides; "retry" = legacy — burn review cycles, then defer + roll back [adapter] name = "claude" # CLI profile: claude | codex | gemini | copilot | antigravity | opencode-http (alias: opencode) | custom diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index 804784c2..58559d1c 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/src/bmad_loop/data/settings/core.toml @@ -52,6 +52,13 @@ options_ref = "REVIEW_ON_TIMEOUT_MODES" default_ref = "ReviewPolicy.on_timeout" label = "review timeout policy" description = "retry: burn a review cycle per timeout (default) · salvage-if-done: commit the verified dev product and refile the follow-up · defer: give up on the first timeout" +[[section.field]] +key = "on_status_contradiction" +kind = "select" +options_ref = "REVIEW_ON_STATUS_CONTRADICTION_MODES" +default_ref = "ReviewPolicy.on_status_contradiction" +label = "review revokes sprint sign-off" +description = "escalate: pause naming both sides when a review writes sprint-status back off done (default) · retry: legacy — burn review cycles, then defer + roll back" [[section]] name = "stories" diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index baa9e4d2..9281140b 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -1475,11 +1475,13 @@ def _review_and_commit( story_key=task.story_key, reason=outcome.reason, env_fault=outcome.env_fault, + contradiction=outcome.contradiction, ) if not outcome.retryable: - # escalate-grade failure (environment fault, git error): a - # repair session cannot fix it and another review cycle - # would replay it — pause the run instead of burning budget + # escalate-grade failure (environment fault, git error, a + # review revoking the sprint sign-off): a repair session + # cannot fix it and another review cycle would replay it — + # pause the run instead of burning budget self._escalate(task, outcome.reason) if outcome.fixable and task.review_cycle < self.policy.limits.max_review_cycles: # failing verify commands are dev work, not review work: a @@ -1525,10 +1527,30 @@ def _review_and_commit( # the non-isolated path: in worktree isolation a defer already keeps the # unit's worktree + patch (no work is lost), so there is nothing to # rescue and committing into the main repo would be wrong. - if refileable_followup and not self._isolated and self._verify_review(task).ok: - self._record_review_budget_followup(task) - self._commit(task) - return + if refileable_followup and not self._isolated: + rescue = self._verify_review(task) + if rescue.ok: + self._record_review_budget_followup(task) + self._commit(task) + return + if rescue.contradiction: + # The rescue gate is the first place this story's sprint + # sign-off was re-read (every in-loop cycle recommended its own + # follow-up, so none of them verified). A defer here would roll + # the work back under a "did not converge" reason that names + # neither side of the disagreement — pause with both instead. + # Journaled under the same kind as the two in-loop gates so a + # consumer keying on `contradiction` sees all three escalating + # paths. The non-contradiction arm keeps its existing silence: + # its story is told by the defer reason below. + self.journal.append( + "review-verify-failed", + story_key=task.story_key, + reason=rescue.reason, + env_fault=rescue.env_fault, + contradiction=rescue.contradiction, + ) + self._escalate(task, rescue.reason) # Name the last completed pass's real outcome (issue #160): the fixed # follow-up wording is only correct when a finalized pass actually left # a refileable recommendation. "did not converge" stays in every variant @@ -1669,10 +1691,12 @@ def _skip_review_and_commit(self, task: StoryTask) -> None: story_key=task.story_key, reason=outcome.reason, env_fault=outcome.env_fault, + contradiction=outcome.contradiction, ) if not outcome.retryable: - # escalate-grade failure (environment fault, git error): a - # defer would just replay it on the next story — pause the run + # escalate-grade failure (environment fault, git error, a review + # revoking the sprint sign-off): a defer would just replay it on + # the next story — pause the run self._escalate(task, outcome.reason) self._defer(task, f"verify failed with review disabled: {outcome.reason}") return @@ -2376,7 +2400,16 @@ def _verify_dev_artifacts(self, task: StoryTask, result_json: dict | None): ) def _verify_review(self, task: StoryTask): - return verify.verify_review(task, self.workspace.paths, self.policy) + # `not _dev_review_enabled()` is exactly the case where _post_dev_state_sync + # targeted "done" and verify_dev asserted the board got there, so a board + # now short of done is a review revoking that sign-off, not a stage never + # reached (#334). + return verify.verify_review( + task, + self.workspace.paths, + self.policy, + sprint_reached_done=not self._dev_review_enabled(), + ) def _review_prompt(self, task: StoryTask) -> str: # Re-invoking bmad-dev-auto on a `done` spec resets review_loop_iteration diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 8e8c6ea0..a830d537 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -483,6 +483,11 @@ class VerifyOutcome: # not found / not executable): no repair session can fix it and every # story shares the same commands, so it must never charge attempt budgets env_fault: bool = False + # a session deliberately contradicted a state the orchestrator had already + # established (a review revoking the sprint sign-off it advanced at dev + # time): no further session can reconcile it, so it routes to a pause with + # both sides named rather than to another cycle (#334) + contradiction: bool = False @classmethod def passed(cls) -> "VerifyOutcome": @@ -494,9 +499,19 @@ def retry(cls, reason: str, fixable: bool = False) -> "VerifyOutcome": @classmethod def escalate( - cls, reason: str, severity: str = "CRITICAL", env_fault: bool = False + cls, + reason: str, + severity: str = "CRITICAL", + env_fault: bool = False, + contradiction: bool = False, ) -> "VerifyOutcome": - return cls(ok=False, reason=reason, severity=severity, env_fault=env_fault) + return cls( + ok=False, + reason=reason, + severity=severity, + env_fault=env_fault, + contradiction=contradiction, + ) @property def retryable(self) -> bool: diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index b159892a..901ff5db 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -29,6 +29,7 @@ SWEEP_AUTO_MODES = {"never", "per-epic", "run-end"} REVIEW_TRIGGER_MODES = {"always", "recommended"} REVIEW_ON_TIMEOUT_MODES = {"retry", "salvage-if-done", "defer"} +REVIEW_ON_STATUS_CONTRADICTION_MODES = {"escalate", "retry"} # Where the run gets its story queue. "sprint-status" (default) is the classic # flow — bmad-sprint-planning writes sprint-status.yaml from prose epics. # "stories" is the opt-in folder+id dispatch flow (BMAD-METHOD #2549): a typed, @@ -204,6 +205,21 @@ class ReviewPolicy: # "defer" — give up on the first timeout-like verdict (no retries). # `crashed` and env-fault (#194) verdicts keep their own routing in every mode. on_timeout: str = "retry" + # What a review that revokes the story's sprint sign-off costs (#334). The + # orchestrator advances sprint-status to `done` at dev time; a review session + # that judges the story unfinished and writes the board back to an earlier + # stage contradicts that — and nothing in the review loop re-advances the + # board, so every remaining cycle re-reads the same failure and the story + # ends deferred + rolled back. + # "escalate" (default) — pause the run naming both sides of the + # disagreement, so a human resolves it instead of the budget burning + # down onto a rollback. + # "retry" — legacy behavior: treat it as an ordinary verify failure, burn + # review cycles to limits.max_review_cycles, then defer. + # Keys on sprint-status only: the spec's own frontmatter status legitimately + # cycles (in-review/in-progress) while a review patches, and `status: blocked` + # remains the sanctioned way for a review to hand a story back to a human. + on_status_contradiction: str = "escalate" @dataclass(frozen=True) @@ -779,6 +795,9 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: enabled=bool(review_d.get("enabled", ReviewPolicy.enabled)), trigger=str(review_d.get("trigger", ReviewPolicy.trigger)).strip(), on_timeout=str(review_d.get("on_timeout", ReviewPolicy.on_timeout)).strip(), + on_status_contradiction=str( + review_d.get("on_status_contradiction", ReviewPolicy.on_status_contradiction) + ).strip(), ) if review.trigger not in REVIEW_TRIGGER_MODES: raise PolicyError( @@ -789,6 +808,12 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: f"review.on_timeout must be one of {sorted(REVIEW_ON_TIMEOUT_MODES)}:" f" got {review.on_timeout!r}" ) + if review.on_status_contradiction not in REVIEW_ON_STATUS_CONTRADICTION_MODES: + raise PolicyError( + "review.on_status_contradiction must be one of " + f"{sorted(REVIEW_ON_STATUS_CONTRADICTION_MODES)}:" + f" got {review.on_status_contradiction!r}" + ) stories = StoriesPolicy( source=str(stories_d.get("source", StoriesPolicy.source)).strip(), spec_folder=str(stories_d.get("spec_folder", StoriesPolicy.spec_folder)).strip(), @@ -1048,6 +1073,12 @@ def _fold_deprecated_engine( # commit it and refile the outstanding follow-up to deferred work. # "defer" -> give up on the first timeout-like verdict. on_timeout = "retry" +# What a review that writes sprint-status back off `done` (the sign-off the +# orchestrator recorded at dev time) costs. Nothing in the review loop +# re-advances the board, so every remaining cycle re-reads the same failure: +# "escalate" -> pause the run naming both sides of the disagreement (default). +# "retry" -> legacy: burn review cycles to max_review_cycles, then defer. +on_status_contradiction = "escalate" [stories] # Story-queue source. "sprint-status" (default) walks sprint-status.yaml written diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 6c6426d2..e0572b34 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -24,7 +24,7 @@ from .frontmatter import _split_frontmatter, read_frontmatter, status_of from .model import StoryTask, VerifyOutcome from .policy import POLICY_FILE, Policy -from .sprintstatus import story_status +from .sprintstatus import STATUS_ORDER, story_status GIT_TIMEOUT_S = 120 COMMAND_TIMEOUT_S = 30 * 60 @@ -1562,7 +1562,26 @@ def verify_commands_outcome(policy: Policy, cwd: Path) -> VerifyOutcome: return VerifyOutcome.passed() -def verify_review(task: StoryTask, paths: ProjectPaths, policy: Policy) -> VerifyOutcome: +def verify_review( + task: StoryTask, + paths: ProjectPaths, + policy: Policy, + *, + sprint_reached_done: bool = False, +) -> VerifyOutcome: + """Gate a completed review pass: spec at ``done``, sprint-status at ``done``, + deterministic verify commands green. + + ``sprint_reached_done`` tells the gate that the orchestrator had already + advanced this story's sprint-status to ``done`` before the review ran (it is + the sole ``sprint_advance`` caller, ``verify_dev`` asserted the write landed, + and ``advance`` never regresses). A board now sitting *earlier* than ``done`` + is therefore not a stage the story never reached — it is a review session + deliberately revoking the sign-off. Nothing in the review loop re-advances + 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.""" if not task.spec_file: return VerifyOutcome.retry("no spec file recorded for task") fm = _gate_frontmatter(Path(task.spec_file)) @@ -1574,6 +1593,22 @@ def verify_review(task: StoryTask, paths: ProjectPaths, policy: Policy) -> Verif sprint = story_status(paths.sprint_status, task.story_key) if sprint != "done": + if _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, " + f"and the review session wrote it back to {sprint!r} while leaving " + f"the spec frontmatter at 'done'. The two sides disagree about " + f"whether the story is finished, and no further review cycle can " + f"reconcile them — the review loop never re-advances the board, so " + f"the remaining cycles would burn down onto a defer that rolls the " + f"work back. Resolve by either completing the outstanding work and " + f"re-arming the escalation (the attempt budget resets on re-arm), " + f"or accepting the story and advancing the board yourself; set " + f'review.on_status_contradiction = "retry" to restore the legacy ' + f"retry-until-budget behavior.", + contradiction=True, + ) return VerifyOutcome.retry( f"sprint-status for {task.story_key} is {sprint!r}, expected 'done'" ) @@ -1581,6 +1616,22 @@ def verify_review(task: StoryTask, paths: ProjectPaths, policy: Policy) -> Verif return verify_commands_outcome(policy, paths.project) +def _is_signoff_regression(sprint: str | None, sprint_reached_done: bool, policy: Policy) -> bool: + """Whether a non-``done`` sprint status is a review deliberately walking the + board backward, as opposed to a stage the story simply never reached. + + Conservative on every uncertainty: without the launch-time guarantee, with + the knob set to ``retry``, or when the fresh read yields no status at all + (missing story entry) or a token outside the known lifecycle (a hand-edited + or future board), the caller falls through to the ordinary retry — a wrong + escalation halts an otherwise healthy run.""" + if not sprint_reached_done or policy.review.on_status_contradiction != "escalate": + return False + if sprint is None or sprint not in STATUS_ORDER: + return False + return STATUS_ORDER.index(sprint) < STATUS_ORDER.index("done") + + def verify_review_stories(task: StoryTask, paths: ProjectPaths, policy: Policy) -> VerifyOutcome: """verify_review for stories mode: same spec-done + verify-commands gates, minus the sprint-status gate (stories mode has no sprint board — the story diff --git a/tests/test_engine.py b/tests/test_engine.py index d28e1781..6830b4bf 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -3407,6 +3407,165 @@ def test_budget_exhausted_failed_review_sessions_defer_not_commit(project): assert "story-deferred" in kinds and "review-budget-committed" not in kinds +# -------------------------------- review revokes the sprint sign-off (#334) + + +def _signoff_revoking_review(project, story_key, *, clean, board="in-progress"): + """A review pass that finalizes the spec to `done` but writes the sprint board + back off `done` — the reviewer judging the story unfinished while the spec + (which the dev pass owns) still claims otherwise.""" + + def effect(spec): + result = review_effect(project, story_key, clean=clean)(spec) + set_sprint(project, story_key, board) + return result + + return effect + + +def test_review_signoff_regression_escalates(project): + """The reported livelock (#334): the review revokes the sprint sign-off the + orchestrator recorded at dev time. Nothing re-advances the board, so the + remaining cycles would replay the same failure and end in a defer + rollback. + The run pauses on the first one instead — two sessions total. + + Ablation: with the escalate branch deleted from verify_review, the loop + `continue`s onto review cycle 2, requests a 3rd session the script does not + have, and the run crashes — `not summary.crashed` and the session-role list + both fail.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [ + dev_effect(project, "1-1-a"), + _signoff_revoking_review(project, "1-1-a", clean=True), + ], + ) + summary = engine.run() + + assert not summary.crashed + assert summary.paused and summary.escalated == 1 and summary.deferred == 0 + assert [s.role for s in adapter.sessions] == ["dev", "review"] # no cycle 2 + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.ESCALATED and task.review_cycle == 1 + saved = load_state(engine.run_dir) + assert saved.paused_stage == PAUSE_ESCALATION + # the paused reason travels to `bmad-loop resolve` — it must name both sides + assert "revoked the sprint sign-off" in saved.paused_reason + assert "'in-progress'" in saved.paused_reason + failed = [e for e in engine.journal.entries() if e["kind"] == "review-verify-failed"][-1] + assert failed["contradiction"] is True + # the escalation path never rolls back or defers + assert "change for 1-1-a" in (project.project / "src.txt").read_text() + kinds = [e["kind"] for e in engine.journal.entries()] + assert "story-deferred" not in kinds + + +def test_review_signoff_regression_retry_mode_keeps_legacy_defer(project): + """`review.on_status_contradiction = "retry"` is the compatibility opt-out: + the same regression burns all three review cycles and defers + rolls back, + exactly as the released build does.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + legacy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=True), + review=ReviewPolicy(on_status_contradiction="retry"), + ) + engine, adapter = make_engine( + project, + [dev_effect(project, "1-1-a")] + + [_signoff_revoking_review(project, "1-1-a", clean=True) for _ in range(3)], + policy=legacy, + ) + summary = engine.run() + + assert summary.deferred == 1 and summary.escalated == 0 and not summary.paused + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.DEFERRED and task.review_cycle == 3 + assert "did not converge" in task.defer_reason + assert [s.role for s in adapter.sessions] == ["dev", "review", "review", "review"] + assert (project.project / "src.txt").read_text() == "original\n" # rolled back + failed = [e for e in engine.journal.entries() if e["kind"] == "review-verify-failed"] + assert failed and all(e["contradiction"] is False for e in failed) + + +def test_review_verify_failure_without_regression_still_retries(project): + """The new gate must not swallow ordinary review-verify failures: with the + board still at `done`, a failing verify command keeps its fixable-retry + routing (repair session, then a fresh review) under the default knob.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + marker = project.project / "fixed.marker" + + def dev_with_marker(spec): + marker.write_text("ok\n") + return dev_effect(project, "1-1-a")(spec) + + def breaking_review(spec): + marker.unlink() # the review's patch broke the gate; the board stays done + return review_effect(project, "1-1-a", clean=True)(spec) + + def fix(spec): + marker.write_text("ok\n") + return SessionResult( + status="completed", result_json={"workflow": "auto-dev", "escalations": []} + ) + + engine, adapter = make_engine( + project, + [dev_with_marker, breaking_review, fix, review_effect(project, "1-1-a", clean=True)], + policy=Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + verify=VerifyPolicy(commands=(_file_exists_cmd(marker),)), + ), + ) + summary = engine.run() + + assert summary.done == 1 and summary.escalated == 0 and not summary.paused + assert [s.role for s in adapter.sessions] == ["dev", "review", "dev", "review"] + failed = [e for e in engine.journal.entries() if e["kind"] == "review-verify-failed"] + assert failed and all(e["contradiction"] is False for e in failed) + + +def test_review_signoff_regression_at_rescue_gate_escalates(project): + """When every cycle recommends its own follow-up, the in-loop verify never + runs — the budget-exhaustion rescue is the first read of the board. A + revoked sign-off there must escalate too, not fall through to the "did not + converge" defer that rolls the work back naming neither side. + + max_followup_reviews=5 keeps damping from firing, so all three cycles stay + on the recommend-a-follow-up arm and the loop exits by its own bound.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [dev_effect(project, "1-1-a")] + + [_signoff_revoking_review(project, "1-1-a", clean=False) for _ in range(3)], + policy=Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=True), + limits=LimitsPolicy(max_followup_reviews=5), + ), + ) + summary = engine.run() + + assert not summary.crashed + assert summary.paused and summary.escalated == 1 and summary.deferred == 0 + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.ESCALATED and task.followup_reviews_spent == 3 + assert [s.role for s in adapter.sessions] == ["dev", "review", "review", "review"] + assert "revoked the sprint sign-off" in load_state(engine.run_dir).paused_reason + # journaled under the same kind as the in-loop gates: a consumer keying on + # `contradiction` must see this path too, not just `story-escalated` + failed = [e for e in engine.journal.entries() if e["kind"] == "review-verify-failed"] + assert len(failed) == 1 and failed[0]["contradiction"] is True + kinds = [e["kind"] for e in engine.journal.entries()] + # neither the rescue commit nor the exhaustion defer + assert "review-budget-committed" not in kinds and "story-deferred" not in kinds + assert "change for 1-1-a" in (project.project / "src.txt").read_text() + + def _damp_policy(max_followup_reviews: int) -> Policy: return Policy( gates=GatesPolicy(mode="none"), diff --git a/tests/test_policy.py b/tests/test_policy.py index d3515cb4..87615cb5 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -50,6 +50,25 @@ def test_review_on_timeout_invalid(): policy.loads('[review]\non_timeout = "salvage"\n') +def test_review_on_status_contradiction_default_parse_and_template(): + import tomllib + + # default is the new behavior: the released retry-until-budget loop is the + # defect (#334), "retry" is the compatibility opt-out. + assert policy.loads("").review.on_status_contradiction == "escalate" + for mode in sorted(policy.REVIEW_ON_STATUS_CONTRADICTION_MODES): + loaded = policy.loads(f'[review]\non_status_contradiction = "{mode}"\n') + assert loaded.review.on_status_contradiction == mode + # the emitted template documents the knob at its dataclass default + doc = tomllib.loads(policy.POLICY_TEMPLATE) + assert doc["review"]["on_status_contradiction"] == policy.ReviewPolicy.on_status_contradiction + + +def test_review_on_status_contradiction_invalid(): + with pytest.raises(policy.PolicyError, match=r"review\.on_status_contradiction"): + policy.loads('[review]\non_status_contradiction = "defer"\n') + + def test_stories_defaults(): pol = policy.loads("") assert pol.stories.source == "sprint-status" diff --git a/tests/test_settings_schema.py b/tests/test_settings_schema.py index be2d5123..8d22464f 100644 --- a/tests/test_settings_schema.py +++ b/tests/test_settings_schema.py @@ -21,6 +21,7 @@ MERGE_STRATEGIES, POLICY_TEMPLATE, RETRO_MODES, + REVIEW_ON_STATUS_CONTRADICTION_MODES, REVIEW_TRIGGER_MODES, SESSION_BUDGET_MODES, STORIES_SOURCES, @@ -76,6 +77,7 @@ ("gates", "mode"): GATE_MODES, ("gates", "retrospective"): RETRO_MODES, ("review", "trigger"): REVIEW_TRIGGER_MODES, + ("review", "on_status_contradiction"): REVIEW_ON_STATUS_CONTRADICTION_MODES, ("stories", "source"): STORIES_SOURCES, ("limits", "session_budget_mode"): SESSION_BUDGET_MODES, ("sweep", "auto"): SWEEP_AUTO_MODES, diff --git a/tests/test_verify.py b/tests/test_verify.py index e250826d..c40d269d 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -18,7 +18,7 @@ from bmad_loop import verify from bmad_loop.model import StoryTask -from bmad_loop.policy import Policy, VerifyPolicy +from bmad_loop.policy import Policy, ReviewPolicy, VerifyPolicy def make_task(paths, story_key="1-1-a"): @@ -362,6 +362,79 @@ def test_verify_review_sprint_not_done(project): assert not out.ok and "sprint-status" in out.reason +# ------------------------------------- review revokes the sprint sign-off (#334) + + +def _signoff_regression_task(project, sprint_status="in-progress"): + """A story the orchestrator advanced to `done` whose board a review then + wrote back, leaving the spec frontmatter at `done`.""" + write_sprint(project, {"1-1-a": sprint_status}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "done", task.baseline_commit) + task.spec_file = str(sp) + return task + + +def test_verify_review_signoff_regression_escalates(project): + """The default knob turns a review's board regression into a CRITICAL pause + that names both sides, instead of a retry no later cycle can satisfy.""" + task = _signoff_regression_task(project) + out = verify.verify_review(task, project, Policy(), sprint_reached_done=True) + + assert not out.ok and not out.retryable + assert out.severity == "CRITICAL" and out.contradiction is True + assert not out.fixable # no repair session can reconcile a disagreement + # both sides are named, plus the two ways out and the opt-out knob + assert "'in-progress'" in out.reason and "'done'" in out.reason + assert "1-1-a" in out.reason + assert 'review.on_status_contradiction = "retry"' in out.reason + + +def test_verify_review_signoff_regression_retry_mode_is_legacy(project): + """`retry` restores the pre-#334 routing verbatim: an ordinary retryable + sprint-status failure that burns review cycles and ends in a defer.""" + task = _signoff_regression_task(project) + pol = Policy(review=ReviewPolicy(on_status_contradiction="retry")) + out = verify.verify_review(task, project, pol, sprint_reached_done=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 'done'" + + +def test_verify_review_signoff_regression_needs_the_launch_flag(project): + """Without the caller's guarantee that the board actually reached `done`, an + earlier status is a stage never reached — the gate must not escalate. Sweep + and stories callers never pass the flag.""" + task = _signoff_regression_task(project) + out = verify.verify_review(task, project, Policy()) # sprint_reached_done defaults False + + assert not out.ok and out.retryable and out.contradiction is False + assert "sprint-status" in out.reason + + +@pytest.mark.parametrize( + "board, why", + [ + ({"1-2-b": "done"}, "story absent from the board -> story_status returns None"), + ({"1-1-a": "awaiting-operator"}, "token outside STATUS_ORDER (hand-edited/future board)"), + ], +) +def test_verify_review_unrecognized_sprint_status_retries(project, board, why): + """Conservative on every uncertainty: a status the lifecycle does not know + cannot be *ordered* against `done`, so it is never called a regression — a + wrong escalation halts an otherwise healthy run.""" + write_sprint(project, board) + task = make_task(project) + sp = spec_path(project, "1-1-a") + write_spec(sp, "done", task.baseline_commit) + task.spec_file = str(sp) + + out = verify.verify_review(task, project, Policy(), sprint_reached_done=True) + + assert not out.ok and out.retryable and out.contradiction is False, why + + # --------------------------------------------------- verify command exit codes (issue #126)