Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions plugins/review-suite/scripts/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ def build_parser() -> argparse.ArgumentParser:
parser.add_argument("--reason")
parser.add_argument("--cd")
parser.add_argument("--base", help="Override the detected default branch ref.")
parser.add_argument(
"--review-brief",
help="Optional Markdown goal and constraints, frozen for this review cycle.",
)
parser.add_argument("--decision", choices=(DECISION_CLEAN, DECISION_FINDINGS))
parser.add_argument("--github-review", action="store_true")
parser.add_argument("--github-force", action="store_true")
Expand Down Expand Up @@ -684,7 +688,11 @@ def _load_cycle_and_state_dir(


def _reject_id_creation_args(args: argparse.Namespace, state: dict[str, Any]) -> None:
sent = [name for name in ("mode", "cd", "base") if getattr(args, name) is not None]
sent = [
name
for name in ("mode", "cd", "base", "review_brief")
if getattr(args, name) is not None
]
if not sent:
return
mode = dict(state.get("mode") or {})
Expand All @@ -702,6 +710,15 @@ def _reject_id_creation_args(args: argparse.Namespace, state: dict[str, Any]) ->
)


def _reject_review_brief_replacement(
state: dict[str, Any], review_brief: str | None
) -> None:
if review_brief is not None and review_brief != state.get("review_brief"):
raise ValueError(
"review brief is frozen for this cycle; start a new cycle to replace it"
)


def _restart_reason(args: argparse.Namespace) -> str:
reason = str(args.reason or "").strip()
if not reason or reason == "REASON":
Expand Down Expand Up @@ -1151,6 +1168,10 @@ def _show_status(state: dict[str, Any], *, state_dir: Path) -> int:
payload: dict[str, Any] = {
"review": state.get("public_id"),
"status": state.get("stage") or "unknown",
"review_brief": "available" if state.get("review_brief") else "unavailable",
"design_conformance_context": "available"
if state.get("review_brief")
else "unavailable",
}
if mode := _mode_label(state):
payload["mode"] = mode
Expand Down Expand Up @@ -1798,6 +1819,7 @@ def _compatible_continuation_cycle(
head: str,
merge_base_head: str,
effective_mode: str,
review_brief: str | None = None,
skip_deslop: bool = False,
) -> dict[str, Any] | None:
normalized_cwd = normalize_cwd(str(review_root))
Expand Down Expand Up @@ -1882,8 +1904,10 @@ def _compatible_continuation_cycle(
f"rerun with --id for one of: {', '.join(public_ids)}"
)
selected_base_drift = selected_candidates[0][3]
selected_state = selected_candidates[0][2]
_reject_review_brief_replacement(selected_state, review_brief)
resumed = _with_current_identity(
selected_candidates[0][2],
selected_state,
head=head,
merge_base_head=merge_base_head,
base_drift=selected_base_drift,
Expand Down Expand Up @@ -1922,6 +1946,7 @@ def _create_or_resume_cycle(
head=head,
merge_base_head=merge_base_head,
effective_mode=resolution.effective_mode,
review_brief=args.review_brief,
skip_deslop=skip_deslop,
)
if continuation is not None:
Expand All @@ -1939,6 +1964,7 @@ def _create_or_resume_cycle(
deslop_enabled=profile_deslop_enabled and not skip_deslop,
deslop_skip_source="cli" if skip_deslop else None,
cycle_token="skip-deslop" if skip_deslop else None,
review_brief=args.review_brief,
)
if str(base_info["requested_base"]) != base:
state["identity"]["requested_base"] = str(base_info["requested_base"])
Expand All @@ -1948,6 +1974,7 @@ def _create_or_resume_cycle(
state = _apply_runtime_options(state, args)
existing = load_cycle_by_key(state_dir, str(state["cycle_key"]))
if existing is not None:
_reject_review_brief_replacement(existing, args.review_brief)
return _apply_runtime_options(existing, args)
return _apply_profile_resolution(state, resolution)

Expand Down Expand Up @@ -2064,6 +2091,7 @@ def _create_successor_cycle(
else resolution.profile.deslop_enabled,
deslop_skip_source=deslop_skip_source,
restart_token=restart_token,
review_brief=state.get("review_brief"),
)
existing = load_cycle_by_key(state_dir, str(replacement["cycle_key"]))
if existing is not None:
Expand Down Expand Up @@ -2606,6 +2634,10 @@ def _action_payload(state: dict[str, Any], *, state_dir: Path) -> dict[str, Any]
def _render(state: dict[str, Any], *, state_dir: Path) -> None:
payload: dict[str, Any] = {
"review": state.get("public_id"),
"review_brief": "available" if state.get("review_brief") else "unavailable",
"design_conformance_context": "available"
if state.get("review_brief")
else "unavailable",
}
action = _action_payload(state, state_dir=state_dir)
summary = _add_review_ladder_fields(payload, state, action)
Expand Down Expand Up @@ -2710,6 +2742,7 @@ def main() -> int:
or args.validation_note
or args.deslop_done
or args.skip_deslop
or args.review_brief
or args.show_findings
or args.show_status
):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ def create_cycle(
deslop_skip_source: str | None = None,
cycle_token: str | None = None,
restart_token: str | None = None,
review_brief: str | None = None,
) -> dict[str, Any]:
identity = normalize_cycle_identity(
cwd=cwd, base=base, branch=branch, head=head, merge_base=merge_base
Expand Down Expand Up @@ -412,6 +413,8 @@ def create_cycle(
if fresh_token is not None and restart is not None:
raise ValueError("cycle_token cannot be combined with restart_token")
key_token = restart or fresh_token
if review_brief is not None and not review_brief.strip():
raise ValueError("review_brief cannot be blank")
state = {
"schema_version": ORCHESTRATOR_STATE_SCHEMA_VERSION,
"cycle_key": cycle_key(
Expand All @@ -431,6 +434,7 @@ def create_cycle(
"requested": requested_selection,
"effective": resolved_selection,
},
"review_brief": review_brief,
"stage": STAGE_CREATED,
"pending_action": None,
"deslop": {
Expand Down
69 changes: 69 additions & 0 deletions plugins/review-suite/tests/test_review_orchestrator_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -1556,6 +1556,73 @@ def test_create_with_skip_deslop_runs_review_without_sidecar(
assert state["rounds"][0]["round_id"] == "phase_review-round-1"


def test_review_brief_is_frozen_and_public_output_reports_coverage(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
) -> None:
_stub_review(monkeypatch)
repo = tmp_path / "repo"
state_dir = tmp_path / "state"
_init_repo(repo)
_commit_file(repo, "app.txt", "base\n", "base")

_, created = _run_review(
monkeypatch,
[
"--mode",
"fast",
"--review-brief",
"# Goal\n\nKeep it neutral.",
"--cd",
str(repo),
"--base",
"main",
"--state-dir",
str(state_dir),
],
)
public_id = str(created["review"])

assert _cycle_payload(state_dir, public_id)["review_brief"] == (
"# Goal\n\nKeep it neutral."
)
assert created["review_brief"] == "available"
assert created["design_conformance_context"] == "available"

errors: list[str] = []
monkeypatch.setattr(
review,
"emit_error",
lambda message, **kwargs: errors.append(str(message)) or 2,
)
monkeypatch.setattr(
sys,
"argv",
[
"review.py",
"--mode",
"fast",
"--review-brief",
"Changed goal",
"--cd",
str(repo),
"--base",
"main",
],
)

assert review.main() == 2
assert errors[-1] == (
"review brief is frozen for this cycle; start a new cycle to replace it"
)

terminal = _cycle_payload(state_dir, public_id)
terminal["stage"] = "aborted"
_write_cycle_payload(state_dir, public_id, terminal)
errors.clear()
assert review.main() == 2
assert "review brief is frozen" in errors[-1]


def test_skip_deslop_does_not_resume_same_head_sidecar_cycle(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
) -> None:
Expand Down Expand Up @@ -2109,6 +2176,8 @@ def fail_deslop(*, command: list[str], cwd: Path) -> subprocess.CompletedProcess
)
assert payload["rounds"] == 1
assert payload["deslop"] == "skipped-fast"
assert payload["review_brief"] == "unavailable"
assert payload["design_conformance_context"] == "unavailable"
assert dict(payload["worktree"]) == {
"branch": "feature/show-status",
"head": str(dict(before_state["identity"])["head"])[:12],
Expand Down
25 changes: 25 additions & 0 deletions plugins/review-suite/tests/test_review_orchestrator_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,31 @@ def test_create_cycle_is_compact_json_state_keyed_by_normalized_inputs(
}


def test_create_cycle_preserves_exact_optional_review_brief(tmp_path: Path) -> None:
brief = "# Goal\n\nKeep the provider-neutral contract.\n"

briefed = create_cycle(
cwd=tmp_path / "repo",
base="main",
branch="feature/brief",
head="head-1",
merge_base="base-1",
requested_mode="fast",
review_brief=brief,
)
briefless = create_cycle(
cwd=tmp_path / "repo",
base="main",
branch="feature/brief",
head="head-1",
merge_base="base-1",
requested_mode="fast",
)

assert briefed["review_brief"] == brief
assert briefless["review_brief"] is None


def test_mark_deslop_closed_disables_tracked_sidecar_and_leaves_fast_untracked(
tmp_path: Path,
) -> None:
Expand Down