diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 8cef5141b216..275c8914db82 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -638,6 +638,14 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu p_complete.add_argument("--metadata", default=None, help='JSON dict of structured facts (e.g. \'{"changed_files": [...], ' '"tests_run": 12}\'). Stored on the closing run.') + p_complete.add_argument( + "--review-override-reason", + default=None, + help=( + "Explicitly accept a protected review's structured contract violations. " + "The reason is recorded in a distinct audit event. Manual use only." + ), + ) p_edit = sub.add_parser( "edit", @@ -2357,12 +2365,14 @@ def _cmd_complete(args: argparse.Namespace) -> int: return 1 summary = getattr(args, "summary", None) raw_meta = getattr(args, "metadata", None) + review_override_reason = getattr(args, "review_override_reason", None) # Guard: structured handoff fields are per-run, so they'd be # copy-pasted identically across N runs — almost always a footgun. # Refuse instead of silently doing the wrong thing. - if len(ids) > 1 and (summary or raw_meta): + if len(ids) > 1 and (summary or raw_meta or review_override_reason): print( - "kanban: --summary / --metadata are per-task and can't be used " + "kanban: --summary / --metadata / --review-override-reason are per-task " + "and can't be used " "with multiple ids (would apply the same handoff to every task). " "Complete tasks one at a time, or drop the flags for the bulk close.", file=sys.stderr, @@ -2407,13 +2417,20 @@ def _cmd_complete(args: argparse.Namespace) -> int: failed.append(tid) continue - if not kb.complete_task( - conn, tid, - result=args.result, - summary=summary, - metadata=metadata, - expected_run_id=_worker_run_id_for(tid), - ): + try: + completed = kb.complete_task( + conn, tid, + result=args.result, + summary=summary, + metadata=metadata, + expected_run_id=_worker_run_id_for(tid), + review_override_reason=review_override_reason, + ) + except ValueError as exc: + failed.append(tid) + print(f"kanban: {exc}", file=sys.stderr) + continue + if not completed: failed.append(tid) print(f"cannot complete {tid} (unknown id or terminal state)", file=sys.stderr) else: diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index d685853066c9..2ce82fbba842 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -878,6 +878,7 @@ def write_board_metadata( archived: Optional[bool] = None, default_workdir: Optional[str] = None, project_id: Optional[str] = None, + review_contract: Any = ..., ) -> dict: """Create / update ``board.json`` for ``board``. @@ -908,6 +909,11 @@ def write_board_metadata( meta["default_workdir"] = str(default_workdir) if default_workdir else None if project_id is not None: meta["project_id"] = str(project_id) if project_id else None + if review_contract is not ...: + if review_contract is None: + meta.pop("review_contract", None) + else: + meta["review_contract"] = review_contract if not meta.get("created_at"): meta["created_at"] = int(time.time()) path = board_metadata_path(slug) @@ -5360,6 +5366,140 @@ class ArtifactPreservationError(RuntimeError): """Raised when a declared scratch deliverable cannot be preserved.""" +class ReviewCompletionRejected(ValueError): + """Raised when a protected review supplies invalid approval evidence.""" + + def __init__(self, reasons: list[str]): + self.reasons = list(reasons) + super().__init__( + "structured review completion rejected: " + + "; ".join(reasons) + + ". Use request_changes for implementation defects or block for " + "an external review dependency." + ) + + +def _review_completion_contract( + conn: sqlite3.Connection, + task_id: str, + *, + board: Optional[str] = None, +) -> tuple[bool, Optional[Any], Optional[int]]: + """Return the declared contract and active review run, if this is review.""" + task_row = conn.execute( + "SELECT status, current_run_id FROM tasks WHERE id = ?", (task_id,), + ).fetchone() + if task_row is None: + return False, None, None + + current_run_id = task_row["current_run_id"] + is_review = task_row["status"] == "review" + if task_row["status"] == "running" and current_run_id is not None: + claimed = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND run_id = ? AND kind = 'claimed' " + "ORDER BY id DESC LIMIT 1", + (task_id, int(current_run_id)), + ).fetchone() + try: + payload = json.loads(claimed["payload"]) if claimed and claimed["payload"] else {} + except (json.JSONDecodeError, TypeError): + payload = {} + is_review = isinstance(payload, dict) and payload.get("source_status") == "review" + if not is_review: + return False, None, None + + requested_runs = conn.execute( + "SELECT metadata FROM task_runs " + "WHERE task_id = ? AND outcome = 'review_requested' " + "ORDER BY id DESC", + (task_id,), + ).fetchall() + for requested in requested_runs: + if not requested["metadata"]: + continue + try: + requested_metadata = json.loads(requested["metadata"]) + except (json.JSONDecodeError, TypeError): + continue + if isinstance(requested_metadata, dict) and "review_contract" in requested_metadata: + return True, requested_metadata["review_contract"], current_run_id + + board_metadata = read_board_metadata(board if board is not None else get_current_board()) + if "review_contract" in board_metadata: + return True, board_metadata["review_contract"], current_run_id + return False, None, current_run_id + + +def _review_completion_reasons(contract: Any, metadata: Optional[dict]) -> list[str]: + """Validate schema-v1 review evidence and return all contract violations.""" + reasons: list[str] = [] + if not isinstance(contract, dict): + return ["review_contract must be an object"] + if type(contract.get("schema")) is not int or contract["schema"] != 1: + reasons.append("review_contract.schema must be 1") + + required = contract.get("required_criteria", []) + if ( + not isinstance(required, list) + or any(not isinstance(item, str) or not item.strip() for item in required) + or len({item.strip() for item in required}) != len(required) + ): + reasons.append("review_contract.required_criteria must contain unique non-empty strings") + required = [] + else: + required = [item.strip() for item in required] + if not isinstance(contract.get("require_commit", False), bool): + reasons.append("review_contract.require_commit must be a boolean") + + evidence = metadata if isinstance(metadata, dict) else {} + if type(evidence.get("review_schema")) is not int or evidence["review_schema"] != 1: + reasons.append("review_schema must be 1") + if evidence.get("review_outcome") != "approved": + reasons.append("review_outcome must be approved") + + for key in ("reviewer_checks_failed", "unresolved_findings", "untested_required"): + if evidence.get(key) != []: + reasons.append(f"{key} must be an empty list") + + acceptance = evidence.get("acceptance") + rows_by_id: dict[str, list[dict]] = {} + if not isinstance(acceptance, list): + reasons.append("acceptance must be a list") + acceptance = [] + for row in acceptance: + if not isinstance(row, dict): + reasons.append("each acceptance row must be an object") + continue + criterion_id = row.get("criterion_id") + if not isinstance(criterion_id, str) or not criterion_id.strip(): + reasons.append("each acceptance row needs a non-empty criterion_id") + continue + criterion_id = criterion_id.strip() + rows_by_id.setdefault(criterion_id, []).append(row) + if row.get("status") != "PASS": + reasons.append(f"acceptance criterion {criterion_id} must be PASS") + if not isinstance(row.get("evidence"), str) or not row["evidence"].strip(): + reasons.append(f"acceptance criterion {criterion_id} needs evidence") + for criterion_id in required: + count = len(rows_by_id.get(criterion_id, [])) + if count != 1: + reasons.append( + f"required criterion {criterion_id} must appear exactly once (found {count})" + ) + + if contract.get("require_commit", False): + commit = evidence.get("commit") + if not isinstance(commit, str) or re.fullmatch(r"[0-9a-fA-F]{40}", commit) is None: + reasons.append("commit must be a full 40-character hexadecimal SHA") + + if "approved" in evidence and evidence["approved"] is not True: + reasons.append("approved must be true") + if "verdict" in evidence and evidence["verdict"] != "approved": + reasons.append("verdict must be approved") + return reasons + + def complete_task( conn: sqlite3.Connection, task_id: str, @@ -5370,6 +5510,8 @@ def complete_task( created_cards: Optional[Iterable[str]] = None, expected_run_id: Optional[int] = None, fire_lifecycle_hook: bool = True, + review_override_reason: Optional[str] = None, + board: Optional[str] = None, ) -> bool: """Transition ``running|ready|blocked|review -> done`` and record ``result``. @@ -5388,6 +5530,12 @@ def complete_task( (e.g. ``{"changed_files": [...], "tests_run": [...]}``) — workers are encouraged to use it for structured handoff facts. + When the latest ``review_requested`` run metadata or board metadata + declares a ``review_contract``, review completions must satisfy that + structured contract. ``review_override_reason`` is an explicit operator + risk acceptance; it emits ``review_completion_overridden`` rather than + masquerading as an ordinary approval. + ``created_cards`` is an optional list of task ids the completing worker claims to have created. Each id is verified against ``tasks.created_by``. If any id is phantom (does not exist or was @@ -5409,6 +5557,40 @@ def complete_task( if not _parents_satisfied(conn, task_id): return False + if expected_run_id is not None: + active = conn.execute( + "SELECT current_run_id FROM tasks WHERE id = ?", (task_id,), + ).fetchone() + if ( + active is None + or active["current_run_id"] is None + or int(active["current_run_id"]) != int(expected_run_id) + ): + return False + + contract_enabled, contract, review_run_id = _review_completion_contract( + conn, task_id, board=board, + ) + review_rejection_reasons = ( + _review_completion_reasons(contract, metadata) if contract_enabled else [] + ) + override_reason = str(review_override_reason or "").strip() + if override_reason and expected_run_id is not None: + raise ValueError( + "review_override_reason is restricted to manual operator actions; " + "an active reviewer must use request_changes or block" + ) + if review_rejection_reasons and not override_reason: + with write_txn(conn): + _append_event( + conn, + task_id, + "review_completion_rejected", + {"reasons": review_rejection_reasons}, + run_id=review_run_id, + ) + raise ReviewCompletionRejected(review_rejection_reasons) + # Gate: verify created_cards BEFORE the main write txn. A rejected # completion still needs an auditable event, so we emit it in a # tiny dedicated txn, then raise. The caller is responsible for @@ -5487,6 +5669,17 @@ def complete_task( ) if cur.rowcount != 1: return False + if review_rejection_reasons: + _append_event( + conn, + task_id, + "review_completion_overridden", + { + "reason": override_reason, + "contract_violations": review_rejection_reasons, + }, + run_id=review_run_id, + ) if isinstance(metadata, dict): _persist_scratch_completion_artifacts(conn, task_id, metadata) for stored_path in metadata.pop("_staged_artifacts", []): diff --git a/hermes_cli/kanban_transfer.py b/hermes_cli/kanban_transfer.py index a5bf3aaab438..e43098d703d4 100644 --- a/hermes_cli/kanban_transfer.py +++ b/hermes_cli/kanban_transfer.py @@ -439,14 +439,16 @@ def import_board( # Rewritten rather than moved across: the archive's copy names a slug # and a workdir that belong to the exporting machine. name = str(staged_meta.get("name") or manifest.get("board_name") or target) - kb.write_board_metadata( - target, - name=name, - description=str(staged_meta.get("description") or ""), - icon=str(staged_meta.get("icon") or ""), - color=str(staged_meta.get("color") or ""), - archived=False, - ) + metadata_fields: dict[str, Any] = { + "name": name, + "description": str(staged_meta.get("description") or ""), + "icon": str(staged_meta.get("icon") or ""), + "color": str(staged_meta.get("color") or ""), + "archived": False, + } + if "review_contract" in staged_meta: + metadata_fields["review_contract"] = staged_meta["review_contract"] + kb.write_board_metadata(target, **metadata_fields) # Bring the imported schema up to this install's version before the # relocation pass writes to it. kb.init_db(board=target) diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index 9a76f1d85230..11ce87ca20c6 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -839,6 +839,7 @@ class UpdateTaskBody(BaseModel): # complete --summary ... --metadata ...``. summary: Optional[str] = None metadata: Optional[dict] = None + review_override_reason: Optional[str] = None # Per-task model/provider override (the board's model dropdown). # ``model_override=""`` clears both. ``clear_model_override=True`` is # the explicit clear signal — needed because Optional[str]=None means @@ -898,12 +899,17 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu s = payload.status ok = True if s == "done": - ok = kanban_db.complete_task( - conn, task_id, - result=payload.result, - summary=payload.summary, - metadata=payload.metadata, - ) + try: + ok = kanban_db.complete_task( + conn, task_id, + result=payload.result, + summary=payload.summary, + metadata=payload.metadata, + review_override_reason=payload.review_override_reason, + board=board, + ) + except kanban_db.ReviewCompletionRejected as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc elif s == "blocked": ok = kanban_db.block_task(conn, task_id, reason=payload.block_reason) elif s == "scheduled": @@ -1347,6 +1353,7 @@ def bulk_update(payload: BulkTaskBody, board: Optional[str] = Query(None)): result=payload.result, summary=payload.summary, metadata=payload.metadata, + board=board, ) elif s == "blocked": ok = kanban_db.block_task(conn, tid) diff --git a/tests/hermes_cli/test_kanban_cli.py b/tests/hermes_cli/test_kanban_cli.py index 658bf2d8c8c1..98d6bd51dc44 100644 --- a/tests/hermes_cli/test_kanban_cli.py +++ b/tests/hermes_cli/test_kanban_cli.py @@ -70,6 +70,37 @@ def test_kanban_show_text_renders_graph_with_open_connection(kanban_home): assert "Cannot operate on a closed database" not in output +def test_cli_protected_review_rejects_then_allows_operator_override(kanban_home): + with kb.connect_closing() as conn: + task_id = kb.create_task(conn, title="protected", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + + rejected = kc.run_slash(f'complete {task_id} --summary "missing evidence"') + assert "structured review completion rejected" in rejected + with kb.connect_closing() as conn: + assert kb.get_task(conn, task_id).status == "review" + + overridden = kc.run_slash( + f'complete {task_id} --summary "accepted risk" ' + '--review-override-reason "maintainer approved follow-up"' + ) + assert f"Completed {task_id}" in overridden + with kb.connect_closing() as conn: + assert kb.get_task(conn, task_id).status == "done" + assert any( + event.kind == "review_completion_overridden" + for event in kb.list_events(conn, task_id) + ) + + def test_board_override_is_isolated_per_concurrent_call(kanban_home, monkeypatch): kb.create_board("alpha") kb.create_board("beta") diff --git a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py index 4dc65d0f9ceb..c07607da74f7 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py @@ -149,6 +149,301 @@ def test_same_card_review_supports_changes_and_approval_without_block_loop(conn) assert completed.block_recurrences == 0 +def test_protected_review_rejects_contradictory_approval_and_stays_recoverable(conn): + task_id = kb.create_task(conn, title="Validate duration hour choices", assignee="builder") + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Ready for structured review.", + metadata={ + "review_contract": { + "schema": 1, + "required_criteria": ["duration-hours"], + "require_commit": True, + } + }, + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + + with pytest.raises(ValueError, match="structured review completion rejected"): + kb.complete_task( + conn, + task_id, + summary="Approved, with a required mismatch still present.", + metadata={ + "review_schema": 1, + "review_outcome": "approved", + "commit": "a" * 40, + "acceptance": [ + { + "criterion_id": "duration-hours", + "status": "PASS", + "evidence": "browser check", + } + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [], + "approved": False, + }, + expected_run_id=review.current_run_id, + ) + + task = kb.get_task(conn, task_id) + assert task is not None + assert task.status == "running" + assert task.current_run_id == review.current_run_id + rejected = _event(kb.list_events(conn, task_id), "review_completion_rejected") + assert rejected.run_id == review.current_run_id + assert "approved must be true" in rejected.payload["reasons"] + + +def test_malformed_declared_review_contract_fails_closed(conn): + task_id = kb.create_task(conn, title="Malformed contract", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": None}, + expected_run_id=implementation.current_run_id, + ) + + with pytest.raises(ValueError, match="review_contract must be an object"): + kb.complete_task(conn, task_id, summary="manual approval") + + assert kb.get_task(conn, task_id).status == "review" + + +def test_board_review_contract_protects_manual_review_completion( + conn, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + kb.write_board_metadata( + "default", + review_contract={"schema": 1, "required_criteria": ["tests"]}, + ) + task_id = kb.create_task(conn, title="Board-protected review", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + expected_run_id=implementation.current_run_id, + ) + + with pytest.raises(ValueError, match="review_schema must be 1"): + kb.complete_task(conn, task_id, summary="missing evidence") + + assert kb.get_task(conn, task_id).status == "review" + + +def test_protected_review_accepts_complete_structured_approval(conn): + task_id = kb.create_task(conn, title="Valid protected review", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={ + "review_contract": { + "schema": 1, + "required_criteria": ["tests", "behavior"], + "require_commit": True, + } + }, + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id) + assert review is not None + assert kb.complete_task( + conn, + task_id, + summary="All required checks passed.", + metadata={ + "review_schema": 1, + "review_outcome": "approved", + "commit": "0123456789abcdef0123456789abcdef01234567", + "acceptance": [ + {"criterion_id": "tests", "status": "PASS", "evidence": "suite green"}, + {"criterion_id": "behavior", "status": "PASS", "evidence": "reproduced"}, + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [], + "approved": True, + "verdict": "approved", + }, + expected_run_id=review.current_run_id, + ) + assert kb.get_task(conn, task_id).status == "done" + + +@pytest.mark.parametrize("status", ["FAIL", "BLOCKED", "UNTESTED"]) +def test_protected_review_rejects_nonpassing_acceptance_rows(conn, status: str): + task_id = kb.create_task(conn, title=f"Rejected {status} review", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id) + assert review is not None + with pytest.raises(ValueError, match="criterion tests must be PASS"): + kb.complete_task( + conn, + task_id, + summary="Not actually approved.", + metadata={ + "review_schema": 1, + "review_outcome": "approved", + "acceptance": [ + {"criterion_id": "tests", "status": status, "evidence": "result"} + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [], + }, + expected_run_id=review.current_run_id, + ) + assert kb.get_task(conn, task_id).status == "running" + + +@pytest.mark.parametrize( + ("patch", "reason"), + [ + ({"review_schema": 2}, "review_schema must be 1"), + ({"review_outcome": "changes_requested"}, "review_outcome must be approved"), + ({"reviewer_checks_failed": ["lint"]}, "reviewer_checks_failed must be an empty list"), + ({"unresolved_findings": ["bug"]}, "unresolved_findings must be an empty list"), + ({"untested_required": ["browser"]}, "untested_required must be an empty list"), + ({"acceptance": []}, "required criterion tests must appear exactly once"), + ({"commit": "abc123"}, "commit must be a full 40-character hexadecimal SHA"), + ({"verdict": "request_changes"}, "verdict must be approved"), + ], +) +def test_review_contract_validator_rejects_each_conflicting_field(patch: dict, reason: str): + evidence = { + "review_schema": 1, + "review_outcome": "approved", + "commit": "a" * 40, + "acceptance": [ + {"criterion_id": "tests", "status": "PASS", "evidence": "suite green"} + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [], + "approved": True, + "verdict": "approved", + } + evidence.update(patch) + reasons = kb._review_completion_reasons( + {"schema": 1, "required_criteria": ["tests"], "require_commit": True}, + evidence, + ) + assert any(reason in item for item in reasons) + + +def test_review_risk_override_is_distinct_and_audited(conn): + task_id = kb.create_task(conn, title="Accepted review risk", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + + assert kb.complete_task( + conn, + task_id, + summary="Maintainer accepted the documented risk.", + review_override_reason="Shipping is time-critical; follow-up is scheduled.", + ) + assert kb.get_task(conn, task_id).status == "done" + events = kb.list_events(conn, task_id) + overridden = _event(events, "review_completion_overridden") + assert overridden.payload["reason"] == "Shipping is time-critical; follow-up is scheduled." + assert overridden.payload["contract_violations"] + assert not [event for event in events if event.kind == "review_completion_rejected"] + + +def test_active_reviewer_cannot_self_grant_manual_risk_override(conn): + task_id = kb.create_task(conn, title="Operator-only risk acceptance", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id) + assert review is not None + + with pytest.raises(ValueError, match="manual operator actions"): + kb.complete_task( + conn, + task_id, + summary="Reviewer attempted to accept their own risk.", + review_override_reason="self-approved", + expected_run_id=review.current_run_id, + ) + assert kb.get_task(conn, task_id).status == "running" + + +def test_task_review_contract_survives_rereview_handoffs(conn): + task_id = kb.create_task(conn, title="Persistent protected review", assignee="builder") + implementation = kb.claim_task(conn, task_id) + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id) + assert review is not None + assert kb.request_changes( + conn, + task_id, + reason="Fix the failing behavior.", + expected_run_id=review.current_run_id, + ) == (True, "builder") + rework = kb.claim_task(conn, task_id) + assert rework is not None + assert kb.request_review( + conn, + task_id, + summary="Corrected implementation; contract intentionally omitted here.", + expected_run_id=rework.current_run_id, + ) + + with pytest.raises(ValueError, match="review_schema must be 1"): + kb.complete_task(conn, task_id, summary="approval without structured evidence") + assert kb.get_task(conn, task_id).status == "review" + + @pytest.mark.parametrize("bad_payload", [None, "{not-json", "{}"]) def test_rereview_requires_explicit_reviewer_when_provenance_is_invalid( conn, diff --git a/tests/hermes_cli/test_kanban_transfer.py b/tests/hermes_cli/test_kanban_transfer.py index fdf37c582e9c..ef36b5752d03 100644 --- a/tests/hermes_cli/test_kanban_transfer.py +++ b/tests/hermes_cli/test_kanban_transfer.py @@ -253,6 +253,66 @@ def test_board_metadata_loses_exporter_local_paths(kanban_root, tmp_path): assert meta["project_id"] is None +def test_board_review_contract_protects_completion_after_round_trip( + kanban_root, tmp_path +): + contract = { + "schema": 1, + "required_criteria": ["duration-hours"], + "require_commit": True, + } + kb.create_board("alpha", name="Alpha Board") + kb.write_board_metadata("alpha", review_contract=contract) + archive = kt.export_board("alpha", str(tmp_path / "alpha"))["archive"] + + kanban_root("target") + result = kt.import_board(archive) + imported = result["board"] + assert kb.read_board_metadata(imported)["review_contract"] == contract + + with kb.connect_closing(board=imported) as conn: + task_id = kb.create_task(conn, title="Validate duration hours", assignee="builder") + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + + with pytest.raises(kb.ReviewCompletionRejected): + kb.complete_task( + conn, + task_id, + summary="Approved despite contradictory structured evidence.", + metadata={ + "review_schema": 1, + "review_outcome": "approved", + "commit": "a" * 40, + "acceptance": [ + { + "criterion_id": "duration-hours", + "status": "PASS", + "evidence": "browser check", + } + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [], + "approved": False, + }, + expected_run_id=review.current_run_id, + board=imported, + ) + + task = kb.get_task(conn, task_id) + assert task is not None + assert task.status == "running" + + # --------------------------------------------------------------------------- # Import never overwrites # --------------------------------------------------------------------------- diff --git a/tests/plugins/test_kanban_dashboard_plugin.py b/tests/plugins/test_kanban_dashboard_plugin.py index 4ae35cb9463c..e77caec36100 100644 --- a/tests/plugins/test_kanban_dashboard_plugin.py +++ b/tests/plugins/test_kanban_dashboard_plugin.py @@ -273,6 +273,33 @@ def test_patch_review_lifecycle_preserves_handoff_and_reopens(client): ) +def test_patch_done_returns_conflict_for_invalid_protected_review(client): + task = client.post( + "/api/plugins/kanban/tasks", + json={"title": "protected review", "assignee": "builder"}, + ).json()["task"] + requested = client.patch( + f"/api/plugins/kanban/tasks/{task['id']}", + json={ + "status": "review", + "assignee": "reviewer", + "metadata": { + "review_contract": {"schema": 1, "required_criteria": ["tests"]} + }, + }, + ) + assert requested.status_code == 200, requested.text + + rejected = client.patch( + f"/api/plugins/kanban/tasks/{task['id']}", + json={"status": "done", "summary": "approved without evidence"}, + ) + assert rejected.status_code == 409 + assert "structured review completion rejected" in rejected.json()["detail"] + with kb.connect() as conn: + assert kb.get_task(conn, task["id"]).status == "review" + + def test_reopening_parent_demotes_ready_child(client): """Reopening a completed parent must invalidate ready children immediately. diff --git a/tests/tools/test_kanban_tools.py b/tests/tools/test_kanban_tools.py index b9d2a52e3538..5d5e138c8e63 100644 --- a/tests/tools/test_kanban_tools.py +++ b/tests/tools/test_kanban_tools.py @@ -132,6 +132,30 @@ def test_complete_happy_path(worker_env): conn.close() +def test_complete_tool_surfaces_protected_review_rejection(worker_env): + from hermes_cli import kanban_db as kb + from tools import kanban_tools as kt + + with kb.connect() as conn: + implementation = kb.get_task(conn, worker_env) + assert implementation is not None + assert kb.request_review( + conn, + worker_env, + reviewer="test-worker", + metadata={"review_contract": {"schema": 1, "required_criteria": ["tests"]}}, + expected_run_id=implementation.current_run_id, + ) + assert kb.claim_review_task(conn, worker_env) is not None + + response = json.loads(kt._handle_complete({"summary": "approved without evidence"})) + assert "structured review completion rejected" in response["error"] + with kb.connect() as conn: + task = kb.get_task(conn, worker_env) + assert task is not None + assert task.status == "running" + + def test_complete_retry_with_empty_created_cards_succeeds(worker_env): """After a phantom rejection, retrying kanban_complete with created_cards=[] (the documented escape hatch) must complete the diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index dd1db3ed3daf..91b29b2373a8 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -785,6 +785,7 @@ def _handle_complete(args: dict, **kw) -> str: result=result, summary=summary, metadata=metadata, created_cards=created_cards, expected_run_id=_worker_run_id(tid), + board=board, ) except kb.ArtifactPreservationError as artifact_err: return tool_error( @@ -1821,7 +1822,10 @@ def _board_schema_prop() -> dict[str, str]: "Free-form dict of structured facts about this " "attempt — {\"changed_files\": [...], \"tests_run\": 12, " "\"findings\": [...]}. Surfaced to downstream " - "workers alongside ``summary``." + "workers alongside ``summary``. When a review_contract is " + "declared, completion metadata must include review_schema, " + "review_outcome, acceptance, reviewer_checks_failed, " + "unresolved_findings, and untested_required evidence." ), }, "result": { @@ -1958,7 +1962,9 @@ def _board_schema_prop() -> dict[str, str]: "type": "object", "description": ( "Optional structured handoff facts for the reviewer, such " - "as changed_files, tests_run, commit, or decisions." + "as changed_files, tests_run, commit, or decisions. To protect " + "this and later review passes, include review_contract with " + "schema, required_criteria, and optional require_commit." ), "additionalProperties": True, }, diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index 084a36f3f843..4e451390418a 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -392,6 +392,54 @@ Keep secrets, raw logs, tokens, OAuth material, and unrelated transcripts out of tests, say so explicitly in `summary` and use `metadata` for the evidence that does exist, such as source URLs, issue ids, or manual review steps. +#### Protected structured reviews + +Boards that need machine-enforced approval can opt into review schema 1. For a +single task, declare the contract in the metadata passed to +`kanban_request_review`: + +```json +{ + "review_contract": { + "schema": 1, + "required_criteria": ["tests", "duration-hours"], + "require_commit": true + } +} +``` + +To protect every review on a board, put the same `review_contract` object in +that board's `board.json`. Existing boards and tasks remain unchanged when no +contract is declared. A task-level declaration takes precedence and remains in +force across requested-changes/re-review cycles. + +A protected review completes only with matching structured metadata: + +```json +{ + "review_schema": 1, + "review_outcome": "approved", + "commit": "0123456789abcdef0123456789abcdef01234567", + "acceptance": [ + {"criterion_id": "tests", "status": "PASS", "evidence": "suite green"}, + {"criterion_id": "duration-hours", "status": "PASS", "evidence": "browser check"} + ], + "reviewer_checks_failed": [], + "unresolved_findings": [], + "untested_required": [] +} +``` + +Missing rows, non-`PASS` status, malformed commit identifiers, non-empty failure +lists, and conflicting `approved` or `verdict` fields reject completion and emit +a `review_completion_rejected` event without ending the review run. Free-form +summary text stays explanatory and is not parsed as approval evidence. + +An operator can intentionally accept documented risk with +`hermes kanban complete --review-override-reason "…"`. This is unavailable +to an active reviewer-owned run and records `review_completion_overridden` with +both the reason and contract violations. + ### The worker lifecycle Every profile that works kanban tasks automatically gets the worker lifecycle — it's injected into the worker's system prompt at spawn (the `KANBAN_GUIDANCE` block), so there is **nothing to install or configure**. It teaches the worker the full lifecycle in **tool calls**, not CLI commands: