From e850a333cfcc4d292e405ff5e595d4474bd406a7 Mon Sep 17 00:00:00 2001 From: "ang-fleet-workers[bot]" <333956806+ang-fleet-workers[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 09:00:13 -0700 Subject: [PATCH] feat(kanban): audited --operator send-back on request-changes (t_7481005e) #999 retired reopen-review and the dashboard review->ready drag, so an operator bouncing a card for a non-review reason (wrong repo, rebase first) had to fabricate a review_coverage record. request_changes now takes operator="" (home-guard #1074 vocabulary): operator profiles only (OPERATOR_PROFILES), coverage waived for that call, an operator_override event (coverage_waived) on the closed run. Reviewer runs keep the coverage gate; the CLI refuses --operator from a dispatched worker run. Wired on the CLI (existing --operator flag) and POST /tasks/{id}/request-changes (operator field). The home guard's own operator_override is deduped so a foreign-card send-back writes one. Verified: tests/.../test_kanban_review_sendback.py 26 passed (10 new); dedupe test fails with the dedupe mutated out. Neighbouring review/home guard suites: 8 failures identical on fork/main without this diff. --- hermes_cli/kanban.py | 27 ++- hermes_cli/kanban_db.py | 104 ++++++++- plugins/kanban/dashboard/plugin_api.py | 8 +- .../hermes_cli/test_kanban_review_sendback.py | 201 ++++++++++++++++++ 4 files changed, 334 insertions(+), 6 deletions(-) diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 39fcd54ba9f87..1c2945e9e497a 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -1161,10 +1161,13 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu "--coverage", default=None, help="Review coverage JSON; records a run-attributed comment before transition (human CLI)", ) + # ``--operator ""`` is added by the home-guard loop below; on + # request-changes it is also the operator send-back (coverage waived). p_reopen_review = sub.add_parser( "reopen-review", - help="Retired: claim review and request-changes with a full coverage comment instead", + help="Retired: claim review and request-changes with a full coverage comment " + "instead (operators: request-changes --operator \"\")", ) p_reopen_review.add_argument("task_ids", nargs="+") p_reopen_review.add_argument( @@ -4736,10 +4739,19 @@ def _cmd_request_review(args: argparse.Namespace) -> int: def _cmd_request_changes(args: argparse.Namespace) -> int: tid = args.task_id reason = " ".join(args.reason).strip() + operator = (getattr(args, "operator", None) or "").strip() or None with kb.connect_closing() as conn: # The caller must hold the review run: as its dispatcher-owned worker, # or as the operator session that made ``claim --review`` (human lane). worker_run = _worker_run_id_for(tid) + if operator is not None and worker_run is not None: + # A dispatched reviewer run always carries full coverage. + print( + f"cannot request changes for {tid}: --operator is not for a " + f"dispatched review run; post the review_coverage record", + file=sys.stderr, + ) + return 1 held_run = worker_run if worker_run is not None else _operator_review_run_id(conn, tid) parked_session = None if held_run is None: @@ -4770,6 +4782,7 @@ def _cmd_request_changes(args: argparse.Namespace) -> int: tid, reason=reason, expected_run_id=held_run, + operator=operator, **( { # Open the review run as this session, and record the @@ -4796,7 +4809,10 @@ def _cmd_request_changes(args: argparse.Namespace) -> int: _unused_run, session_ref = safe_comment_provenance(tid) kb.add_comment( conn, tid, _profile_author(), - "changes requested (human review lane): " + ( + f"changes requested (operator send-back, {operator}): " + if operator else "changes requested (human review lane): " + ) + str(kb.redact_review_value(reason)), run_id=( held_run if held_run is not None @@ -4820,7 +4836,12 @@ def _cmd_reopen_review(args: argparse.Namespace) -> int: with kb.connect_closing() as conn: for tid in ids: kb.reopen_review_task(conn, tid) - print(f"cannot reopen {tid}: legacy bypass retired; claim review and use request-changes with full coverage", file=sys.stderr) + print( + f"cannot reopen {tid}: legacy bypass retired; claim review and use " + f"request-changes with full coverage (operator send-back: " + f"hermes kanban request-changes {tid} \"\" --operator \"\")", + file=sys.stderr, + ) return 1 diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 73f56a80ca8c1..686b2e05d421f 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -5699,6 +5699,23 @@ def record_foreign_action( "SELECT session_id FROM tasks WHERE id = ?", (task_id,) ).fetchone() if actor.operator: + last = conn.execute( + "SELECT kind, payload FROM task_events WHERE task_id = ? " + "ORDER BY id DESC LIMIT 1", (task_id,), + ).fetchone() + if last is not None and last["kind"] == "operator_override": + try: + prev = json.loads(last["payload"] or "{}") + except (json.JSONDecodeError, TypeError): + prev = {} + if ( + isinstance(prev, dict) + and prev.get("action") == action + and prev.get("reason") == actor.operator + ): + # The mutator already recorded this override itself + # (request-changes' operator send-back); one event per call. + return with write_txn(conn, allow_nested=True): _append_event( conn, @@ -12685,6 +12702,50 @@ def _validate_review_coverage(conn: sqlite3.Connection, task_id: str, run_id: in return None +def _operator_caller_profiles() -> frozenset[str]: + """Profiles the current caller holds, for the operator send-back check. + + The bound actor (CLI / tool surface) resolves through + :func:`_actor_profiles` like the home-session guard; an unbound caller + (the dashboard server) falls back to its profile env, then the active + profile. + """ + actor = _EVENT_ACTOR.get() + if actor is not None: + names = set(_actor_profiles(actor)) + else: + profile, _sid = _event_actor() + names = {profile} if profile else set() + if not names: + try: + from .profiles import get_active_profile_name + + active = get_active_profile_name() + except Exception: + active = None + if active: + names.add(active) + return frozenset(names) + + +def _operator_send_back_refusal(task_id: str, reason: str) -> Optional[str]: + """Why an ``--operator`` send-back is refused, or ``None`` if allowed.""" + profiles = _operator_caller_profiles() + if not (profiles & OPERATOR_PROFILES): + return ( + f"refused request-changes on {task_id}: --operator is for operator " + f"profiles ({', '.join(sorted(OPERATOR_PROFILES))}); caller " + f"profile(s): {', '.join(sorted(profiles)) or 'unknown'}. A " + f"reviewer sends back with a full review_coverage record." + ) + if not _valid_operator_reason(reason): + return ( + f"refused request-changes on {task_id}: --operator needs " + f"\"\" (e.g. \"Ace via Apollo: wrong repo\")." + ) + return None + + class _SendBackRefused(Exception): """Roll back a send-back's own review claim when the handoff is refused.""" @@ -12703,6 +12764,7 @@ def request_changes( claimer: Optional[str] = None, coverage: Optional[str] = None, session_ref: Optional[str] = None, + operator: Optional[str] = None, ) -> tuple[bool, Optional[str]]: """Finish an active review run and route the task back for rework. @@ -12732,10 +12794,24 @@ def request_changes( ``session_ref`` (trusted runtime context, never model args) is recorded on the opened run's ``claimed`` event, as ``claim_review_task`` does for ``claim --review``. + + ``operator`` (``""``, the home-guard #1074 vocabulary) is the + operator send-back: an operator profile (:data:`OPERATOR_PROFILES`) + bouncing a card for a non-review reason ("wrong repo", "rebase first"). + It waives the coverage requirement for that call only and records an + ``operator_override`` event on the closed run. A non-operator caller or a + reason without ``who: why`` is refused before anything is written. The + coverage gate is unchanged for every call without it. """ reason = str(redact_review_value(reason or "")).strip() if not reason: return False, "reason is required" + # Same normalization as the home-guard actor, so its dedupe matches. + operator_reason = (str(operator).strip() or None) if operator else None + if operator_reason is not None: + refusal = _operator_send_back_refusal(task_id, operator_reason) + if refusal is not None: + return False, refusal coverage_text = str(redact_review_value(coverage or "")).strip() coverage_body = f"review_coverage: {coverage_text}" if coverage_text else None comment_author = str(claimer or "reviewer").strip() or "reviewer" @@ -12752,7 +12828,7 @@ def _in_txn() -> tuple[bool, Optional[str]]: return False, "task not found" current_run_id = task_row["current_run_id"] if claimer and expected_run_id is None and task_row["status"] == "review": - if coverage_body is None: + if coverage_body is None and operator_reason is None: # Same message the gate gives; refused before any claim churn. return False, _REVIEW_COVERAGE_MISSING if _prior_worker_still_alive(conn, task_id) is not None: @@ -12826,7 +12902,10 @@ def _in_txn() -> tuple[bool, Optional[str]]: run_id=int(current_run_id), ) posted.append((int(comment_cur.lastrowid or 0), int(current_run_id), now)) - coverage_error = _validate_review_coverage(conn, task_id, int(current_run_id)) + coverage_error = ( + None if operator_reason is not None + else _validate_review_coverage(conn, task_id, int(current_run_id)) + ) if coverage_error: return False, coverage_error reviewer = task_row["assignee"] @@ -12870,9 +12949,30 @@ def _in_txn() -> tuple[bool, Optional[str]]: "implementer": implementer, "reviewer": reviewer, "status": new_status, + **({"operator": operator_reason} if operator_reason else {}), }, run_id=run_id, ) + if operator_reason is not None: + by_profile, _sid = _event_actor() + bound = _EVENT_ACTOR.get() + home_row = conn.execute( + "SELECT session_id FROM tasks WHERE id = ?", (task_id,) + ).fetchone() + _append_event( + conn, + task_id, + "operator_override", + { + "action": "request-changes", + "reason": operator_reason, + "coverage_waived": True, + "by_sessions": list(bound.session_ids) if bound else [], + "by_profile": by_profile, + "home": home_row["session_id"] if home_row is not None else None, + }, + run_id=run_id, + ) return True, implementer try: diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index 1fd033dadf74f..87413faa69a49 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -981,7 +981,9 @@ class UpdateTaskBody(BaseModel): _REVIEW_EXIT_STATUSES = frozenset({"done", "blocked", "archived"}) _REVIEW_EXIT_REFUSAL = ( "Claim review and request changes with a full coverage comment " - "(POST /tasks/{id}/request-changes); direct review reopening is retired" + "(POST /tasks/{id}/request-changes); direct review reopening is retired. " + "An operator bounce without a review sends {\"operator\": \"\"} " + "to the same route" ) @@ -2013,6 +2015,9 @@ class RequestChangesBody(BaseModel): # review_coverage JSON text; required for a card parked in ``review``. coverage: Optional[str] = None author: Optional[str] = None + # ``""``: operator send-back without a review (coverage waived, + # recorded as an ``operator_override`` event). Operator profiles only. + operator: Optional[str] = None @router.post("/tasks/{task_id}/request-changes") @@ -2041,6 +2046,7 @@ def request_changes_endpoint( reason=payload.reason, claimer=(payload.author or "dashboard"), coverage=payload.coverage, + operator=payload.operator, ) if not ok: raise HTTPException( diff --git a/tests/hermes_cli/test_kanban_review_sendback.py b/tests/hermes_cli/test_kanban_review_sendback.py index aafe139289932..48b1e8b343625 100644 --- a/tests/hermes_cli/test_kanban_review_sendback.py +++ b/tests/hermes_cli/test_kanban_review_sendback.py @@ -386,3 +386,204 @@ def test_dashboard_request_changes_route_sends_back_parked_review(board: Path) - assert ok.json()["implementer"] == "builder" with kb.connect() as conn: _assert_sent_back(conn, tid, "apollo") + + +# --- operator send-back (t_7481005e) --------------------------------------- +# #999 made a review card leave review only with a full coverage record. An +# operator profile bouncing a card for a non-review reason ("wrong repo", +# "rebase first") uses ``operator=""`` instead: coverage waived for +# that call, an ``operator_override`` event on the closed run. Reviewer runs +# keep the gate; non-operator profiles are refused. + +OPERATOR = "Ace via Apollo: wrong repo, re-port onto hermes-home" + + +def _operator_overrides(conn, tid): + return [ + (payload, run_id) for kind, payload, run_id in _kinds(conn, tid) + if kind == "operator_override" + ] + + +def test_operator_send_back_without_coverage_succeeds_and_is_recorded( + board: Path, +) -> None: + with kb.connect() as conn: + tid = _parked_review(conn) + ok, detail = kb.request_changes( + conn, tid, reason="rebase first", claimer="apollo", + operator=OPERATOR, + ) + assert (ok, detail) == (True, "builder") + task = kb.get_task(conn, tid) + assert task.status == "ready" and task.assignee == "builder" + events = _kinds(conn, tid) + after = events[[k for k, _, _ in events].index("review_requested") + 1:] + assert [k for k, _, _ in after] == [ + "claimed", "changes_requested", "operator_override", + ] + run_id = after[0][2] + assert after[1][1]["operator"] == OPERATOR + overrides = _operator_overrides(conn, tid) + assert len(overrides) == 1 + payload, ev_run = overrides[0] + assert ev_run == run_id + assert payload["action"] == "request-changes" + assert payload["reason"] == OPERATOR + assert payload["coverage_waived"] is True + assert payload["by_profile"] == "apollo" + assert not [ + c for c in kb.list_comments(conn, tid) + if c.body.startswith("review_coverage:") + ] + + +@pytest.mark.parametrize("profile", ["argus", "daedalus"]) +def test_operator_send_back_refused_for_non_operator_profile( + board: Path, monkeypatch: pytest.MonkeyPatch, profile: str, +) -> None: + monkeypatch.setenv("HERMES_PROFILE", profile) + with kb.connect() as conn: + tid = _parked_review(conn) + before, runs_before = _snapshot(conn, tid) + ok, detail = kb.request_changes( + conn, tid, reason="rebase first", claimer=profile, + operator=OPERATOR, + ) + assert ok is False + assert "--operator is for operator profiles" in detail + assert profile in detail + _assert_untouched(conn, tid, before, runs_before) + + +@pytest.mark.parametrize("reason", ["just bounce it", ": no who", "Ace: "]) +def test_operator_send_back_needs_who_colon_why(board: Path, reason: str) -> None: + with kb.connect() as conn: + tid = _parked_review(conn) + before, runs_before = _snapshot(conn, tid) + ok, detail = kb.request_changes( + conn, tid, reason="rebase first", claimer="apollo", operator=reason, + ) + assert ok is False and "" in detail + _assert_untouched(conn, tid, before, runs_before) + + +def test_reviewer_run_without_coverage_is_still_refused( + board: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The gate is unchanged for a dispatched reviewer: no coverage, no + send-back -- and it cannot borrow --operator either.""" + with kb.connect() as conn: + tid = _parked_review(conn) + review = kb.claim_review_task(conn, tid, claimer="argus:1") + ok, detail = kb.request_changes( + conn, tid, reason="fix", expected_run_id=review.current_run_id, + ) + assert ok is False and detail == kb._REVIEW_COVERAGE_MISSING + monkeypatch.setenv("HERMES_PROFILE", "argus") + ok, detail = kb.request_changes( + conn, tid, reason="fix", expected_run_id=review.current_run_id, + operator=OPERATOR, + ) + assert ok is False and "--operator is for operator profiles" in detail + task = kb.get_task(conn, tid) + assert task.status == "running" + assert task.current_run_id == review.current_run_id + assert not _operator_overrides(conn, tid) + + +def test_cli_operator_send_back_parked_review( + board: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("HERMES_SESSION_ID", "20260927_090000_operator") + with kb.connect() as conn: + tid = _parked_review(conn) + rc = kc._cmd_request_changes(argparse.Namespace( + task_id=tid, reason=["rebase", "first"], coverage=None, + operator=OPERATOR, + )) + assert rc == 0 + with kb.connect() as conn: + assert kb.get_task(conn, tid).status == "ready" + assert len(_operator_overrides(conn, tid)) == 1 + rework = [ + c.body for c in kb.list_comments(conn, tid) + if c.body.startswith("changes requested (operator send-back,") + ] + assert rework == [ + f"changes requested (operator send-back, {OPERATOR}): rebase first" + ] + + +def test_cli_without_operator_still_needs_coverage( + board: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("HERMES_SESSION_ID", "20260927_090000_operator") + with kb.connect() as conn: + tid = _parked_review(conn) + before, runs_before = _snapshot(conn, tid) + rc = kc._cmd_request_changes(argparse.Namespace( + task_id=tid, reason=["rebase", "first"], coverage=None, + )) + assert rc == 1 + with kb.connect() as conn: + _assert_untouched(conn, tid, before, runs_before) + + +def test_foreign_card_operator_send_back_records_one_event(board: Path) -> None: + """On a foreign card the home guard also honours --operator; the send-back + and the guard must not both write an operator_override for one call.""" + with kb.connect() as conn: + tid = _parked_review(conn) + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET session_id = ? WHERE id = ?", + ("20260927_080000_homesess", tid), + ) + with kb.mutation_actor( + session_ids=("20260927_090000_callersess",), profile="apollo", + operator=OPERATOR, + ): + ok, detail = kb.request_changes( + conn, tid, reason="rebase first", claimer="apollo", + operator=OPERATOR, + ) + assert (ok, detail) == (True, "builder") + overrides = _operator_overrides(conn, tid) + assert len(overrides) == 1 + assert overrides[0][0]["coverage_waived"] is True + + +def test_dashboard_operator_send_back( + board: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + pytest.importorskip("fastapi") + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from plugins.kanban.dashboard import plugin_api + + app = FastAPI() + app.include_router(plugin_api.router, prefix="/api/plugins/kanban") + client = TestClient(app) + with kb.connect() as conn: + tid = _parked_review(conn) + before, runs_before = _snapshot(conn, tid) + url = f"/api/plugins/kanban/tasks/{tid}/request-changes" + monkeypatch.setenv("HERMES_PROFILE", "argus") + refused = client.post(url, json={ + "reason": "rebase first", "author": "argus", "operator": OPERATOR, + }) + assert refused.status_code == 409 + assert "--operator is for operator profiles" in refused.json()["detail"] + with kb.connect() as conn: + _assert_untouched(conn, tid, before, runs_before) + monkeypatch.setenv("HERMES_PROFILE", "apollo") + ok = client.post(url, json={ + "reason": "rebase first", "author": "apollo", "operator": OPERATOR, + }) + assert ok.status_code == 200, ok.text + assert ok.json()["implementer"] == "builder" + with kb.connect() as conn: + assert kb.get_task(conn, tid).status == "ready" + assert len(_operator_overrides(conn, tid)) == 1