diff --git a/AGENTS.md b/AGENTS.md index 76c988fdd9904..ed5f5490a99b2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1135,15 +1135,15 @@ kanban task. - **CLI:** `hermes_cli/kanban.py` wires `hermes kanban` with verbs `init`, `create`, `list` (alias `ls`), `show`, `assign`, `link`, `unlink`, `comment`, `attach`, `attachments`, `attach-rm`, `complete`, - `block`, `unblock`, `archive`, `tail`, plus less-commonly-used `watch`, - `stats`, `runs`, `log`, `assignees`, `heartbeat`, `notify-*`, - `dispatch`, `daemon`, `gc`. + `request-review`, `reopen-review`, `block`, `unblock`, `archive`, + `tail`, plus less-commonly-used `watch`, `stats`, `runs`, `log`, + `assignees`, `heartbeat`, `notify-*`, `dispatch`, `daemon`, `gc`. - **Worker/orchestrator toolset:** `tools/kanban_tools.py` exposes - `kanban_show`, `kanban_complete`, `kanban_block`, `kanban_heartbeat`, - `kanban_comment`, `kanban_create`, `kanban_link`, `kanban_attach`, - `kanban_attach_url`, `kanban_attachments`; profiles that explicitly - enable the `kanban` toolset outside a dispatcher-spawned task also get - `kanban_list` and `kanban_unblock` for board routing. + `kanban_show`, `kanban_complete`, `kanban_request_review`, `kanban_block`, + `kanban_heartbeat`, `kanban_comment`, `kanban_create`, `kanban_link`, + `kanban_attach`, `kanban_attach_url`, `kanban_attachments`; profiles that + explicitly enable the `kanban` toolset outside a dispatcher-spawned + task also get `kanban_list` and `kanban_unblock` for board routing. - **Dispatcher:** long-lived loop that (default every 60s) reclaims stale claims, promotes ready tasks, atomically claims, and spawns assigned profiles. Runs **inside the gateway** by default via diff --git a/acp_adapter/tools.py b/acp_adapter/tools.py index e4bcaef9b6adb..6e756c3fbfabd 100644 --- a/acp_adapter/tools.py +++ b/acp_adapter/tools.py @@ -75,7 +75,7 @@ _POLISHED_TOOLS = { "feishu_doc_read", "feishu_drive_list_comments", "feishu_drive_list_comment_replies", "feishu_drive_reply_comment", "feishu_drive_add_comment", "kanban_create", "kanban_show", "kanban_comment", "kanban_complete", - "kanban_block", "kanban_link", "kanban_heartbeat", + "kanban_block", "kanban_request_review", "kanban_link", "kanban_heartbeat", "yb_query_group_info", "yb_query_group_members", "yb_search_sticker", "yb_send_dm", "yb_send_sticker", } diff --git a/agent/prompt_builder.py b/agent/prompt_builder.py index dc583c7529580..469b5974e1b59 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -247,8 +247,11 @@ KANBAN_GUIDANCE = ( "before counting as merged/done (most coding tasks), drop the " "structured metadata (changed_files / tests_run / diff_path) into a " "`kanban_comment` first, then end with " - "`kanban_block(reason=\"review-required: \")` so a " - "reviewer can approve+unblock or request changes. Reviewing-then-" + "`kanban_request_review(summary=\"\")` " + "so a reviewer can approve (→ `kanban_complete`) or send it back for " + "changes. Use `kanban_request_review`, NOT `kanban_block`, for review " + "hand-offs: review is not a block, so cycling through review across " + "follow-ups never trips unblock-loop detection. Reviewing-then-" "completing is more honest than auto-completing work that still needs " "eyes on it.\n" "6. **If follow-up work appears, create it; don't do it.** Use " diff --git a/agent/transports/hermes_tools_mcp_server.py b/agent/transports/hermes_tools_mcp_server.py index 7fc23cf506d96..e5bfe6f8f7a55 100644 --- a/agent/transports/hermes_tools_mcp_server.py +++ b/agent/transports/hermes_tools_mcp_server.py @@ -135,6 +135,7 @@ EXPOSED_TOOLS: tuple[str, ...] = ( # the env var and write to ~/.hermes/kanban.db. "kanban_complete", "kanban_block", + "kanban_request_review", "kanban_comment", "kanban_heartbeat", "kanban_show", diff --git a/gateway/kanban_watchers.py b/gateway/kanban_watchers.py index 17b65e1110f5b..c09f20475b48c 100644 --- a/gateway/kanban_watchers.py +++ b/gateway/kanban_watchers.py @@ -143,10 +143,13 @@ class GatewayKanbanWatchersMixin: For each subscription row, fetches ``task_events`` newer than the stored cursor with kind in the terminal set (``completed``, - ``blocked``, ``gave_up``, ``crashed``, ``timed_out``). Sends one + ``blocked``, ``gave_up``, ``crashed``, ``timed_out``, + ``review_requested``, ``block_loop_detected``). Sends one message per new event to ``(platform, chat_id, thread_id)``, - then advances the cursor. When a task reaches a terminal state - (``completed`` / ``archived``), the subscription is removed. + then advances the cursor. The subscription is removed only when the + task reaches a truly final *status* (``done`` / ``archived``), not on + any terminal event kind — so review cycles and re-block loops keep + notifying. Runs in the gateway event loop; all SQLite work is pushed to a thread via ``asyncio.to_thread`` so the loop never blocks on the @@ -171,7 +174,11 @@ class GatewayKanbanWatchersMixin: # "status" covers dashboard drag-drop and `_set_status_direct()` # writes — surface those transitions to subscribers too. - TERMINAL_KINDS = ("completed", "blocked", "gave_up", "crashed", "timed_out", "status", "archived", "unblocked", "block_loop_detected") + # ``review_requested`` wakes the origin subscriber like a block does, + # but is not a block (see kanban_db.request_review); the task is not + # done/archived, so the subscription stays alive and later review + # cycles keep notifying. + TERMINAL_KINDS = ("completed", "blocked", "gave_up", "crashed", "timed_out", "status", "archived", "unblocked", "block_loop_detected", "review_requested") # Subscriptions are removed only when the task reaches a truly final # status (done / archived). We used to also unsub on any terminal # event kind (gave_up / crashed / timed_out / blocked), but that @@ -479,6 +486,16 @@ class GatewayKanbanWatchersMixin: if ev.payload and ev.payload.get("status"): new_status = str(ev.payload["status"]) msg = f"🔄 {board_tag}{tag}Kanban {sub['task_id']} → {new_status}" + elif kind == "review_requested": + # Implementation complete; task moved to 'review' + # and awaits a human. Wake the origin thread. + handoff = "" + if ev.payload and ev.payload.get("summary"): + handoff = f"\n{str(ev.payload['summary'])[:200]}" + msg = ( + f"👀 {board_tag}{tag}Kanban {sub['task_id']} ready for review" + f" — {title}{handoff}" + ) elif kind == "block_loop_detected": # A task re-blocked for the same cause past the # recurrence limit and was routed to `triage` for a @@ -1321,6 +1338,13 @@ class GatewayKanbanWatchersMixin: here keeps the stuck-warn fire only on real failures (broken PATH, missing venv, credential loss for a real Hermes profile). """ + # Only probe the review column when autonomous review dispatch is + # actually on. With ``review_dispatch`` off (the default — no + # sdlc-review agent), a task parked in 'review' is "correctly idle" + # waiting for a human, not a stuck dispatcher; probing it here would + # fire a false "dispatcher stuck" warning that never clears. Shares + # the exact gate the dispatcher uses so the two can't drift. + _review_probe = _kb.review_dispatch_enabled() try: boards = _kb.list_boards(include_archived=False) except Exception: @@ -1332,7 +1356,7 @@ class GatewayKanbanWatchersMixin: conn = _kb.connect(board=slug) if _kb.has_spawnable_ready(conn): return True - if _kb.has_spawnable_review(conn): + if _review_probe and _kb.has_spawnable_review(conn): return True except Exception: continue diff --git a/hermes_cli/goals.py b/hermes_cli/goals.py index b906979e8d319..b0d0258f83109 100644 --- a/hermes_cli/goals.py +++ b/hermes_cli/goals.py @@ -1975,9 +1975,11 @@ KANBAN_GOAL_CONTINUATION_TEMPLATE = ( "[Continuing toward this kanban task — judge says it is not done yet]\n" "Reason: {reason}\n\n" "Take the next concrete step toward completing the task. When the work " - "is genuinely finished, call kanban_complete with a summary. If you are " - "blocked and need human input, call kanban_block with a reason. Do not " - "stop without calling one of them." + "is genuinely finished, call kanban_complete with a summary. If it is a " + "code change that needs human review before counting as done, call " + "kanban_request_review with a summary instead. If you are blocked and " + "need human input, call kanban_block with a reason. Do not stop without " + "calling one of them." ) # Fed when the judge believes the work is done but the worker never called @@ -1987,8 +1989,9 @@ KANBAN_GOAL_FINALIZE_TEMPLATE = ( "[The work looks complete, but the task is still open]\n" "Reason: {reason}\n\n" "If the task is genuinely done, call kanban_complete now with a short " - "summary of what you did. If something still blocks completion, call " - "kanban_block with the reason instead." + "summary of what you did. If it is a code change awaiting human review, " + "call kanban_request_review with that summary instead. If something still " + "blocks completion, call kanban_block with the reason instead." ) @@ -2027,8 +2030,8 @@ def run_kanban_goal_loop( (reason: str -> None). Returns a decision dict: ``{"outcome", "turns_used", "reason"}`` where - outcome is one of ``"completed_by_worker"``, ``"blocked_budget"``, - ``"blocked_by_worker"``, or ``"stopped"``. + outcome is one of ``"completed_by_worker"``, ``"review_requested_by_worker"``, + ``"blocked_budget"``, ``"blocked_by_worker"``, or ``"stopped"``. """ def _log(msg: str) -> None: @@ -2061,6 +2064,12 @@ def run_kanban_goal_loop( if status == "blocked": _log(f"kanban goal loop: task {task_id} blocked by worker after {turns_used} turn(s)") return {"outcome": "blocked_by_worker", "turns_used": turns_used, "reason": "worker blocked the task"} + if status == "review": + # A legitimate worker-driven terminator (kanban_request_review), + # not an unexpected stop: the implementation is done and the task + # is awaiting a human. Stop the loop cleanly. + _log(f"kanban goal loop: task {task_id} handed off for review by worker after {turns_used} turn(s)") + return {"outcome": "review_requested_by_worker", "turns_used": turns_used, "reason": "worker requested review"} if status not in ("running", "ready"): # Reclaimed / archived / unexpected — let the dispatcher own it. _log(f"kanban goal loop: task {task_id} status={status!r}; stopping") diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index a08cb8f9b4076..f8c121e7e0f3f 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -655,6 +655,30 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu ) p_unblock.add_argument("task_ids", nargs="+") + p_request_review = sub.add_parser( + "request-review", + help="Move a task to 'review' (implementation done, awaiting review) — NOT a block", + ) + p_request_review.add_argument("task_id") + p_request_review.add_argument( + "--summary", default=None, + help="What was implemented and how it was verified — shown to the reviewer.", + ) + p_request_review.add_argument( + "--reviewer", default=None, + help="Optional profile/handle to attribute the review to (informational only).", + ) + + p_reopen_review = sub.add_parser( + "reopen-review", + help="Send one or more review tasks back for changes (review -> ready/todo)", + ) + p_reopen_review.add_argument("task_ids", nargs="+") + p_reopen_review.add_argument( + "--reason", default=None, + help="Optional reason/note — recorded as a comment before reopening. Quote multi-word reasons.", + ) + p_promote = sub.add_parser( "promote", help="Manually move one or more todo/blocked tasks to ready (recovery path)", @@ -1066,6 +1090,8 @@ def kanban_command(args: argparse.Namespace) -> int: "block": _cmd_block, "schedule": _cmd_schedule, "unblock": _cmd_unblock, + "request-review": _cmd_request_review, + "reopen-review": _cmd_reopen_review, "promote": _cmd_promote, "archive": _cmd_archive, "tail": _cmd_tail, @@ -2336,6 +2362,48 @@ def _cmd_unblock(args: argparse.Namespace) -> int: return 0 if not failed else 1 +def _cmd_request_review(args: argparse.Namespace) -> int: + tid = args.task_id + summary = getattr(args, "summary", None) + if summary is not None: + summary = summary.strip() or None + reviewer = getattr(args, "reviewer", None) + with kb.connect_closing() as conn: + if not kb.request_review( + conn, tid, summary=summary, reviewer=reviewer, + expected_run_id=_worker_run_id_for(tid), + ): + print( + f"cannot request review for {tid} (not running/ready?)", + file=sys.stderr, + ) + return 1 + print(f"Requested review for {tid}" + (f": {summary}" if summary else "")) + return 0 + + +def _cmd_reopen_review(args: argparse.Namespace) -> int: + ids = list(args.task_ids or []) + if not ids: + print("at least one task_id is required", file=sys.stderr) + return 1 + reason = getattr(args, "reason", None) + if reason is not None: + reason = reason.strip() or None + author = _profile_author() if reason else None + failed: list[str] = [] + with kb.connect_closing() as conn: + for tid in ids: + if reason: + kb.add_comment(conn, tid, author, f"CHANGES REQUESTED: {reason}") + if not kb.reopen_review_task(conn, tid): + failed.append(tid) + print(f"cannot reopen {tid} (not in review?)", file=sys.stderr) + else: + print(f"Reopened {tid}" + (f": {reason}" if reason else "")) + return 0 if not failed else 1 + + def _cmd_promote(args: argparse.Namespace) -> int: reason = " ".join(args.reason).strip() if args.reason else None author = _profile_author() @@ -3145,6 +3213,7 @@ Common subcommands: `comment ` Append a comment `attach ` Attach a local file; `attachments ` to list `complete …` Mark task(s) done + `request-review ` Hand off for human review (moves to `review`, not a block); `reopen-review …` sends it back for changes `block [reason]` Mark blocked; `schedule [reason]` parks time-delay work; `unblock ` to revive `assign ` Reassign `boards list` Show all boards diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c4bb7caf9482b..ab491c85656bf 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -4843,11 +4843,15 @@ def complete_task( created_cards: Optional[Iterable[str]] = None, expected_run_id: Optional[int] = None, ) -> bool: - """Transition ``running|ready -> done`` and record ``result``. + """Transition ``running|ready|blocked|review -> done`` and record ``result``. Accepts a task that is merely ``ready`` too, so a manual CLI completion (``hermes kanban complete ``) works without requiring - a claim/start/complete sequence. + a claim/start/complete sequence. ``review`` is accepted so a human + (or reviewer) can approve a task parked in the review lane by + :func:`request_review` — even when it has no active run + (``current_run_id IS NULL``), the handoff fields are preserved via + :func:`_synthesize_ended_run`. ``summary`` and ``metadata`` are stored on the closing run (if any) and surfaced to downstream children via :func:`build_worker_context`. @@ -4917,7 +4921,7 @@ def complete_task( block_kind = NULL, block_recurrences = 0 WHERE id = ? - AND status IN ('running', 'ready', 'blocked') + AND status IN ('running', 'ready', 'blocked', 'review') """, (result, now, task_id), ) @@ -4934,7 +4938,7 @@ def complete_task( block_kind = NULL, block_recurrences = 0 WHERE id = ? - AND status IN ('running', 'ready', 'blocked') + AND status IN ('running', 'ready', 'blocked', 'review') AND current_run_id = ? """, (result, now, task_id, int(expected_run_id)), @@ -4974,8 +4978,8 @@ def complete_task( # notifiers and dashboard WS consumers can render it without a # second SQL round-trip. First line only, 400 char cap — the # full summary stays on the run row. - ev_summary = (summary if summary is not None else result) or "" - ev_summary = ev_summary.strip().splitlines()[0][:400] if ev_summary else "" + _ev_lines = ((summary if summary is not None else result) or "").strip().splitlines() + ev_summary = _ev_lines[0][:400] if _ev_lines else "" completed_payload: dict = { "result_len": len(result) if result else 0, "summary": ev_summary or None, @@ -5596,10 +5600,8 @@ def edit_completed_task_result( "UPDATE task_runs SET metadata = ? WHERE id = ?", (json.dumps(metadata, ensure_ascii=False), run_id), ) - ev_summary = ( - handoff_summary.strip().splitlines()[0][:400] - if handoff_summary else "" - ) + _ev_lines = (handoff_summary or "").strip().splitlines() + ev_summary = _ev_lines[0][:400] if _ev_lines else "" _append_event( conn, task_id, "edited", { @@ -5828,6 +5830,103 @@ def block_task( +def request_review( + conn: sqlite3.Connection, + task_id: str, + *, + summary: Optional[str] = None, + reviewer: Optional[str] = None, + expected_run_id: Optional[int] = None, +) -> bool: + """Transition ``running``/``ready`` → ``review`` (implementation done, awaiting review). + + A first-class "request review" transition. Unlike :func:`block_task` + this is NOT a blocker: it does NOT read or increment + ``block_recurrences`` and never routes to ``triage``. Repeated review + requests on the same task (e.g. after a follow-up rerun) are + legitimate and must never be mistaken for an unblock↔re-block loop. + + Releases the claim lock, closes the active run with + ``outcome="review_requested"`` / ``status="review"`` (synthesizing a + zero-duration run when the task was never claimed so the handoff + fields survive), and emits a ``review_requested`` event carrying the + handoff ``summary`` plus the ``implementer`` (current assignee) and an + optional ``reviewer``. When ``expected_run_id`` is given the + transition is gated on it (CAS), like :func:`complete_task`, so a + stale/superseded worker can't move the task. + + ``reviewer`` is informational only — recorded on the event payload, not + used to reassign the task (reviewer-profile routing belongs to the + autonomous-review path, which this build does not ship). + + Returns True on a successful transition, False when the task wasn't in + a running/ready state (or the expected run no longer matches). + """ + with write_txn(conn): + trow = conn.execute( + "SELECT assignee FROM tasks WHERE id = ?", (task_id,), + ).fetchone() + if trow is None: + return False + implementer = trow["assignee"] + if expected_run_id is None: + cur = conn.execute( + """ + UPDATE tasks + SET status = 'review', + claim_lock = NULL, + claim_expires = NULL, + worker_pid = NULL + WHERE id = ? + AND status IN ('running', 'ready') + """, + (task_id,), + ) + else: + cur = conn.execute( + """ + UPDATE tasks + SET status = 'review', + claim_lock = NULL, + claim_expires = NULL, + worker_pid = NULL + WHERE id = ? + AND status IN ('running', 'ready') + AND current_run_id = ? + """, + (task_id, int(expected_run_id)), + ) + if cur.rowcount != 1: + return False + run_id = _end_run( + conn, task_id, + outcome="review_requested", status="review", + summary=summary, + ) + # Preserve the handoff summary when the task was never claimed + # (e.g. a manual/CLI request-review on a ready task). + if run_id is None and summary: + run_id = _synthesize_ended_run( + conn, task_id, + outcome="review_requested", + summary=summary, + ) + # First line of the summary on the event payload so the gateway + # notifier can render the wake without a second SQL round-trip. + _ev_lines = (summary or "").strip().splitlines() + ev_summary = _ev_lines[0][:400] if _ev_lines else "" + _append_event( + conn, task_id, "review_requested", + { + "summary": ev_summary or None, + "implementer": implementer, + "reviewer": reviewer, + }, + run_id=run_id, + ) + return True + + def promote_task( conn: sqlite3.Connection, task_id: str, @@ -5898,6 +5997,54 @@ def promote_task( return True, None +def _reclaim_dangling_run( + conn: sqlite3.Connection, task_id: str, *, statuses, now: int, note: str, +) -> None: + """Close a leaked ``current_run_id`` (run row still open) before a status + flip, preserving the runs invariant (``current_run_id IS NULL`` ⇔ run row + terminal). No-op in the common path where the prior transition already + closed the run. Shared by :func:`unblock_task` and + :func:`reopen_review_task` so the recovery can't drift. + """ + placeholders = ", ".join("?" for _ in statuses) + stale = conn.execute( + f"SELECT current_run_id FROM tasks WHERE id = ? AND status IN ({placeholders})", + (task_id, *statuses), + ).fetchone() + if stale and stale["current_run_id"]: + conn.execute( + """ + UPDATE task_runs + SET status = 'reclaimed', outcome = 'reclaimed', + summary = COALESCE(summary, ?), + ended_at = ?, + claim_lock = NULL, claim_expires = NULL, worker_pid = NULL + WHERE id = ? AND ended_at IS NULL + """, + (note, now, int(stale["current_run_id"])), + ) + + +def _landing_status_after_parents(conn: sqlite3.Connection, task_id: str) -> str: + """Return ``'todo'`` if any parent isn't ``done`` yet, else ``'ready'``. + + The parent-completion re-gate shared by :func:`unblock_task` and + :func:`reopen_review_task`: flipping straight to ``ready`` would bypass the + parent-completion invariant the dispatcher trusts (it would spawn a child + whose upstream work isn't finished). If parents are still in progress the + task waits in ``todo`` until ``recompute_ready`` picks it up. RCA: Bug 2 at + kanban/boards/cookai/workspaces/t_a6acd07d/root-cause.md. Kept in one place + so the two transitions can't drift. + """ + undone_parents = conn.execute( + "SELECT 1 FROM task_links l " + "JOIN tasks p ON p.id = l.parent_id " + "WHERE l.child_id = ? AND p.status != 'done' LIMIT 1", + (task_id,), + ).fetchone() + return "todo" if undone_parents else "ready" + + def unblock_task(conn: sqlite3.Connection, task_id: str) -> bool: """Transition ``blocked``/``scheduled`` -> ready or todo. @@ -5910,35 +6057,13 @@ def unblock_task(conn: sqlite3.Connection, task_id: str) -> bool: """ now = int(time.time()) with write_txn(conn): - stale = conn.execute( - "SELECT current_run_id FROM tasks WHERE id = ? AND status IN ('blocked', 'scheduled')", - (task_id,), - ).fetchone() - if stale and stale["current_run_id"]: - conn.execute( - """ - UPDATE task_runs - SET status = 'reclaimed', outcome = 'reclaimed', - summary = COALESCE(summary, 'invariant recovery on unblock'), - ended_at = ?, - claim_lock = NULL, claim_expires = NULL, worker_pid = NULL - WHERE id = ? AND ended_at IS NULL - """, - (now, int(stale["current_run_id"])), - ) + _reclaim_dangling_run( + conn, task_id, statuses=("blocked", "scheduled"), now=now, + note="invariant recovery on unblock", + ) # Re-gate on parent completion before flipping 'blocked' back to - # 'ready'. Unconditionally setting status='ready' here bypasses the - # parent-completion invariant (the dispatcher trusts that column); - # if parents are still in progress the task must wait in 'todo' - # until recompute_ready picks it up. RCA: Bug 2 at - # kanban/boards/cookai/workspaces/t_a6acd07d/root-cause.md. - undone_parents = conn.execute( - "SELECT 1 FROM task_links l " - "JOIN tasks p ON p.id = l.parent_id " - "WHERE l.child_id = ? AND p.status != 'done' LIMIT 1", - (task_id,), - ).fetchone() - new_status = "todo" if undone_parents else "ready" + # 'ready' (see :func:`_landing_status_after_parents`). + new_status = _landing_status_after_parents(conn, task_id) # NOTE: deliberately does NOT touch ``block_recurrences`` or # ``block_kind``. Resetting the recurrence counter on unblock is exactly # the amnesia that let a cron unblock → worker re-block loop run @@ -5964,6 +6089,43 @@ def unblock_task(conn: sqlite3.Connection, task_id: str) -> bool: return True +def reopen_review_task(conn: sqlite3.Connection, task_id: str) -> bool: + """Transition ``review`` -> ready (or todo) so the implementer re-runs. + + The "changes requested" counterpart of :func:`request_review`: sends the + task back out of the review lane so the dispatcher re-runs the implementer + on the new comments. Mirrors :func:`unblock_task` (parent re-gating, + defensive stale-run close, ``consecutive_failures`` reset) and emits a + ``review_reopened`` event. + + Deliberately does NOT touch ``block_recurrences``/``block_kind``: review is + not a block, so there is no loop counter to reset. (A stale counter from a + genuine block *before* review is left intact — only :func:`complete_task` + clears it.) Returns False when the task is missing or not in ``review``. + """ + now = int(time.time()) + with write_txn(conn): + _reclaim_dangling_run( + conn, task_id, statuses=("review",), now=now, + note="invariant recovery on review reopen", + ) + new_status = _landing_status_after_parents(conn, task_id) + cur = conn.execute( + "UPDATE tasks SET status = ?, current_run_id = NULL, " + "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, " + "consecutive_failures = 0, last_failure_error = NULL " + "WHERE id = ? AND status = 'review'", + (new_status, task_id), + ) + if cur.rowcount != 1: + return False + _append_event( + conn, task_id, "review_reopened", + {"status": new_status} if new_status != "ready" else None, + ) + return True + + def specify_triage_task( conn: sqlite3.Connection, task_id: str, @@ -8295,6 +8457,26 @@ def has_spawnable_review(conn: sqlite3.Connection) -> bool: return False +def review_dispatch_enabled() -> bool: + """Whether the dispatcher should auto-claim ``review`` tasks and spawn a + reviewer agent (``kanban.review_dispatch``, default **False**). + + Single source of truth for the two gates that must never drift: the + review-column dispatch loop in :func:`_dispatch_once_locked` and the + gateway ``_ready_nonempty`` health probe. Defaults to False because this + build ships no autonomous reviewer (no ``sdlc-review`` skill), so a task + parked in ``review`` waits for a human rather than a phantom reviewer. + Best-effort: any config error reads as disabled. + """ + try: + from hermes_cli.config import load_config + return bool( + (load_config() or {}).get("kanban", {}).get("review_dispatch", False) + ) + except Exception: + return False + + def dispatch_once( conn: sqlite3.Connection, *, @@ -8702,11 +8884,20 @@ def _dispatch_once_locked( # Same concurrency model as ready dispatch: review spawns count # against max_spawn alongside ready tasks, so the total number of # running workers stays bounded. - review_rows = conn.execute( - "SELECT id, assignee FROM tasks " - "WHERE status = 'review' AND claim_lock IS NULL " - "ORDER BY priority DESC, created_at ASC" - ).fetchall() + # Gated by ``kanban.review_dispatch`` (default OFF; see + # :func:`review_dispatch_enabled`). This build ships no autonomous reviewer + # (no sdlc-review skill), so by default a task parked in 'review' by + # ``request_review`` waits for a human instead of the dispatcher + # auto-claiming it and spawning a phantom ``sdlc-review`` worker on the + # implementer's own profile. Deployments that actually install an + # sdlc-review agent set it true to re-enable autonomous review dispatch. + review_rows = [] + if review_dispatch_enabled(): + review_rows = conn.execute( + "SELECT id, assignee FROM tasks " + "WHERE status = 'review' AND claim_lock IS NULL " + "ORDER BY priority DESC, created_at ASC" + ).fetchall() for row in review_rows: if max_spawn is not None and running_count + spawned >= max_spawn: break diff --git a/plugins/kanban/dashboard/dist/index.js b/plugins/kanban/dashboard/dist/index.js index 959e4e6d6da25..126aa8d180632 100644 --- a/plugins/kanban/dashboard/dist/index.js +++ b/plugins/kanban/dashboard/dist/index.js @@ -86,8 +86,8 @@ return body || raw; } - // Order matches BOARD_COLUMNS in plugin_api.py. - const COLUMN_ORDER = ["triage", "todo", "ready", "running", "blocked", "done"]; + // Board column display order; any backend status not listed here renders after these. + const COLUMN_ORDER = ["triage", "todo", "ready", "running", "blocked", "review", "done"]; // English fallback dictionaries — used when the i18n catalog is missing // a key, and as defaults for the get*() helpers below so callers running // outside any React component (where there's no `t`) still get sane text. @@ -97,6 +97,7 @@ ready: "Ready", running: "In Progress", blocked: "Blocked", + review: "Review", done: "Done", archived: "Archived", }; @@ -106,6 +107,7 @@ ready: "Dependencies satisfied; assign a profile to dispatch", running: "Claimed by a worker — in-flight", blocked: "Worker asked for human input", + review: "Implementation complete — awaiting human review", done: "Completed", archived: "Archived", }; @@ -157,6 +159,7 @@ ready: "hermes-kanban-dot-ready", running: "hermes-kanban-dot-running", blocked: "hermes-kanban-dot-blocked", + review: "hermes-kanban-dot-review", done: "hermes-kanban-dot-done", archived: "hermes-kanban-dot-archived", }; diff --git a/plugins/kanban/dashboard/dist/style.css b/plugins/kanban/dashboard/dist/style.css index 352f55525b2a7..590a8ca314d48 100644 --- a/plugins/kanban/dashboard/dist/style.css +++ b/plugins/kanban/dashboard/dist/style.css @@ -175,6 +175,7 @@ .hermes-kanban-dot-ready { background: #d4b348; } /* amber */ .hermes-kanban-dot-running { background: #3fb97d; } /* green */ .hermes-kanban-dot-blocked { background: var(--color-destructive, #d14a4a); } +.hermes-kanban-dot-review { background: #48b0c4; } /* cyan — awaiting human review */ .hermes-kanban-dot-done { background: #4a8cd1; } /* blue */ .hermes-kanban-dot-archived { background: var(--color-border); } diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index fdc49da34f135..e8c08bf610cc1 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -851,6 +851,19 @@ class UpdateTaskBody(BaseModel): clear_reasoning_effort: bool = False +def _reopen_if_review(conn, task_id: str, current) -> Optional[bool]: + """Route a task leaving the ``review`` lane through ``reopen_review_task`` + (proper transition: stale-run recovery, parent re-gate, ``review_reopened`` + event) instead of a raw status write. Returns the transition result, or + ``None`` when the task isn't in ``review`` so the caller falls through to + its normal handling. Shared by the single-task and bulk status handlers so + the review-reopen routing can't drift between them. + """ + if current is not None and getattr(current, "status", None) == "review": + return kanban_db.reopen_review_task(conn, task_id) + return None + + @router.patch("/tasks/{task_id}") def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Query(None)): board = _resolve_board(board) @@ -886,14 +899,25 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu ok = kanban_db.block_task(conn, task_id, reason=payload.block_reason) elif s == "scheduled": ok = kanban_db.schedule_task(conn, task_id, reason=payload.block_reason) + elif s == "review": + # Manual "request review" from the board: implementation done, + # awaiting a human. Routes through request_review so it is NOT a + # block (never trips unblock-loop detection). Only valid from + # running/ready — a False return becomes the 409 toast below. + ok = kanban_db.request_review( + conn, task_id, summary=payload.summary, + ) elif s == "ready": - # Re-open a blocked/scheduled task, or just an explicit status set. + # Re-open a blocked/scheduled/review task, or just an explicit + # status set. "Changes requested" (review -> ready) goes through + # reopen_review_task via _reopen_if_review. current = kanban_db.get_task(conn, task_id) if current and current.status in ("blocked", "scheduled"): ok = kanban_db.unblock_task(conn, task_id) else: + reopened = _reopen_if_review(conn, task_id, current) # Direct status write for drag-drop (todo -> ready etc). - ok = _set_status_direct(conn, task_id, "ready") + ok = reopened if reopened is not None else _set_status_direct(conn, task_id, "ready") elif s == "archived": ok = kanban_db.archive_task(conn, task_id) elif s == "running": @@ -902,7 +926,11 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu detail="Cannot set status to 'running' directly; use the dispatcher/claim path", ) elif s in ("todo", "triage", "scheduled"): - ok = _set_status_direct(conn, task_id, s) + # Only a review task moving to 'todo' needs the reopen + # transition; fetch lazily so triage/scheduled skip the query. + current = kanban_db.get_task(conn, task_id) if s == "todo" else None + reopened = _reopen_if_review(conn, task_id, current) + ok = reopened if reopened is not None else _set_status_direct(conn, task_id, s) else: raise HTTPException(status_code=400, detail=f"unknown status: {s}") if not ok: @@ -1263,12 +1291,18 @@ def bulk_update(payload: BulkTaskBody, board: Optional[str] = Query(None)): ) elif s == "blocked": ok = kanban_db.block_task(conn, tid) + elif s == "review": + # Non-block review handoff (mirror of PATCH /tasks/{id}). + ok = kanban_db.request_review( + conn, tid, summary=payload.summary, + ) elif s == "ready": cur = kanban_db.get_task(conn, tid) if cur and cur.status in ("blocked", "scheduled"): ok = kanban_db.unblock_task(conn, tid) else: - ok = _set_status_direct(conn, tid, "ready") + reopened = _reopen_if_review(conn, tid, cur) + ok = reopened if reopened is not None else _set_status_direct(conn, tid, "ready") elif s == "running": entry.update( ok=False, @@ -1282,7 +1316,10 @@ def bulk_update(payload: BulkTaskBody, board: Optional[str] = Query(None)): elif s == "scheduled": ok = kanban_db.schedule_task(conn, tid) elif s in {"todo", "triage"}: - ok = _set_status_direct(conn, tid, s) + # Fetch lazily: only review->todo needs reopen. + cur = kanban_db.get_task(conn, tid) if s == "todo" else None + reopened = _reopen_if_review(conn, tid, cur) + ok = reopened if reopened is not None else _set_status_direct(conn, tid, s) else: entry.update(ok=False, error=f"unknown status {s!r}") results.append(entry) diff --git a/tests/hermes_cli/test_kanban_review_lifecycle.py b/tests/hermes_cli/test_kanban_review_lifecycle.py new file mode 100644 index 0000000000000..441f313245f45 --- /dev/null +++ b/tests/hermes_cli/test_kanban_review_lifecycle.py @@ -0,0 +1,442 @@ +"""Review-lifecycle tests: the first-class ``running -> review`` transition. + +``request_review`` is the "implementation complete, awaiting review" +transition used by executor workers instead of encoding ``review-required:`` +prose into a ``kanban_block`` call. The critical contract these tests pin +down: + +* It transitions ``running``/``ready`` -> ``review`` and closes the active + run with ``outcome="review_requested"``. +* It emits exactly one ``review_requested`` event carrying the handoff + summary + implementer. +* Crucially, it is NOT a blocker: repeated review requests on the same task + (a review -> rerun -> review follow-up cycle) never touch + ``block_recurrences`` and never route to ``triage`` — the false + ``block_loop_detected`` escalation that plagued the block-reason approach + cannot happen. +* ``expected_run_id`` is honoured as a CAS guard so a stale/superseded + worker cannot move the task. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def kanban_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Isolated HERMES_HOME with an empty kanban DB.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb.init_db() + return home + + +def _row(conn, tid): + return conn.execute( + "SELECT status, block_kind, block_recurrences, current_run_id " + "FROM tasks WHERE id = ?", + (tid,), + ).fetchone() + + +def _events(conn, tid, kind=None): + rows = conn.execute( + "SELECT kind, payload FROM task_events WHERE task_id = ? ORDER BY id", + (tid,), + ).fetchall() + out = [ + (r["kind"], json.loads(r["payload"]) if r["payload"] else None) + for r in rows + ] + if kind is not None: + out = [e for e in out if e[0] == kind] + return out + + +def _last_run(conn, tid): + return conn.execute( + "SELECT status, outcome, summary FROM task_runs " + "WHERE task_id = ? ORDER BY id DESC LIMIT 1", + (tid,), + ).fetchone() + + +# --------------------------------------------------------------------------- +# Happy path: running -> review +# --------------------------------------------------------------------------- + + +def test_request_review_transitions_running_to_review(kanban_home: Path) -> None: + with kb.connect() as conn: + tid = kb.create_task(conn, title="impl a feature", assignee="worker") + kb.claim_task(conn, tid) + run_id = kb.get_task(conn, tid).current_run_id + assert run_id is not None + + ok = kb.request_review( + conn, tid, + summary="Implementation complete\nfull details below", + reviewer="reviewer", + expected_run_id=run_id, + ) + assert ok is True + + row = _row(conn, tid) + assert row["status"] == "review" + # The active run is closed and the pointer cleared. + assert row["current_run_id"] is None + # Not a block: recurrence machinery is untouched. + assert (row["block_recurrences"] or 0) == 0 + assert row["block_kind"] is None + + run = _last_run(conn, tid) + assert run["outcome"] == "review_requested" + assert run["status"] == "review" + + # Exactly one review_requested event, with the handoff payload. + rr = _events(conn, tid, kind="review_requested") + assert len(rr) == 1 + payload = rr[0][1] + assert payload["implementer"] == "worker" + assert payload["reviewer"] == "reviewer" + # First line of the summary rides the event payload. + assert payload["summary"] == "Implementation complete" + # No block / triage events were emitted. + assert _events(conn, tid, kind="blocked") == [] + assert _events(conn, tid, kind="block_loop_detected") == [] + + +# --------------------------------------------------------------------------- +# Core regression: repeated review requests never escalate to triage +# --------------------------------------------------------------------------- + + +def test_repeated_review_requests_never_triage(kanban_home: Path) -> None: + """A task that goes review -> rerun -> review again (the executor + follow-up cycle) must stay in ``review`` every time. Under the old + ``kanban_block(review-required:)`` approach the second pass hit + ``block_recurrences >= 2`` and was wrongly routed to ``triage`` with a + ``block_loop_detected`` event. ``request_review`` must never do that.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="cycle me", assignee="worker") + + for _ in range(4): + # Executor claims (ready->running or review->running) and finishes + # with a review request. claim_review_task handles review->running. + task = kb.get_task(conn, tid) + if task.status == "ready": + kb.claim_task(conn, tid) + else: + assert task.status == "review" + claimed = kb.claim_review_task(conn, tid) + assert claimed is not None + + run_id = kb.get_task(conn, tid).current_run_id + ok = kb.request_review( + conn, tid, + summary="pass complete", + expected_run_id=run_id, + ) + assert ok is True + row = _row(conn, tid) + assert row["status"] == "review", "must never leave the review lane" + assert (row["block_recurrences"] or 0) == 0 + + # After several cycles: never triaged, never a false loop. + assert _row(conn, tid)["status"] == "review" + assert _events(conn, tid, kind="block_loop_detected") == [] + assert len(_events(conn, tid, kind="review_requested")) == 4 + + +# --------------------------------------------------------------------------- +# CAS guard + bad-input behaviour +# --------------------------------------------------------------------------- + + +def test_request_review_expected_run_id_mismatch_is_noop(kanban_home: Path) -> None: + with kb.connect() as conn: + tid = kb.create_task(conn, title="stale worker", assignee="worker") + kb.claim_task(conn, tid) + real_run = kb.get_task(conn, tid).current_run_id + + # A superseded worker passes a run id that is not the current one. + ok = kb.request_review(conn, tid, expected_run_id=(real_run or 0) + 999) + assert ok is False + # Task is untouched — still running under the real run. + row = _row(conn, tid) + assert row["status"] == "running" + assert row["current_run_id"] == real_run + assert _events(conn, tid, kind="review_requested") == [] + + +def test_request_review_unknown_task_returns_false(kanban_home: Path) -> None: + with kb.connect() as conn: + assert kb.request_review(conn, "t_deadbeefcafe") is False + + +@pytest.mark.parametrize("blank", [" ", "\n", "\t\n "]) +def test_request_review_whitespace_only_summary_does_not_crash( + kanban_home: Path, blank: str +) -> None: + """A whitespace-only handoff summary must not crash the review transition. + + Regression: the event-summary extraction tested the truthiness of the + *pre-strip* value while indexing the *post-strip* (empty) list, so a + summary like ``" "`` is truthy, ``.strip()`` collapses it to ``""``, + ``"".splitlines()`` is ``[]`` and ``[][0]`` raised ``IndexError`` inside + ``write_txn`` — a 500 on the dashboard PATCH/bulk path, which forwards + ``summary`` unstripped (the tool/CLI paths pre-strip to ``None`` and were + never exposed). The transition must still succeed and the event must + carry ``summary=None`` (whitespace collapses to no summary). + """ + with kb.connect() as conn: + tid = kb.create_task(conn, title="blank summary", assignee="worker") + kb.claim_task(conn, tid) + run_id = kb.get_task(conn, tid).current_run_id + + ok = kb.request_review(conn, tid, summary=blank, expected_run_id=run_id) + assert ok is True + assert kb.get_task(conn, tid).status == "review" + + rr = _events(conn, tid, kind="review_requested") + assert len(rr) == 1 + # Whitespace collapses to no summary on the event payload. + assert rr[0][1]["summary"] is None + + +# --------------------------------------------------------------------------- +# review -> done: a human can approve/close a task parked in review +# --------------------------------------------------------------------------- + + +def test_complete_task_closes_review_to_done(kanban_home: Path) -> None: + """A task parked in ``review`` (with no active run — request_review + closed it, so ``current_run_id IS NULL``, the #54823 shape) must be + completable by a human approval via ``complete_task``.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="approve me", assignee="worker") + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="ready", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "review" + # The review lane has no active run — the exact state that used to + # make `hermes kanban complete` a no-op (#54823). + assert kb.get_task(conn, tid).current_run_id is None + + ok = kb.complete_task(conn, tid, summary="LGTM — merged", result="approved") + assert ok is True + assert kb.get_task(conn, tid).status == "done" + assert _events(conn, tid, kind="completed") + + +# --------------------------------------------------------------------------- +# Wake plumbing: review_requested is a claimable terminal event for a sub +# --------------------------------------------------------------------------- + + +def test_review_requested_event_is_claimable_for_wake(kanban_home: Path) -> None: + """The gateway kanban-notifier wakes an origin subscription by claiming + unseen events whose kind is in its terminal set. ``review_requested`` is + now in that set, so a wake subscription must see the event — and the + subscription is NOT torn down (task is in ``review``, not done/archived), + so later review cycles keep notifying.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="wake me", assignee="worker") + kb.add_notify_sub( + conn, + task_id=tid, + platform="slack", + chat_id="C123", + thread_id="T1", + ) + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="please review", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + + # Same terminal set the notifier now uses (incl. review_requested). + terminal_kinds = ( + "completed", "blocked", "gave_up", "crashed", "timed_out", + "review_requested", + ) + _old, _new, events = kb.claim_unseen_events_for_sub( + conn, + task_id=tid, + platform="slack", + chat_id="C123", + thread_id="T1", + kinds=terminal_kinds, + ) + kinds_seen = [e.kind for e in events] + assert "review_requested" in kinds_seen + # Task is parked in review — the subscription must survive (only + # done/archived tears it down), so subsequent cycles still wake. + assert kb.get_task(conn, tid).status == "review" + + +# --------------------------------------------------------------------------- +# Dispatcher gate: no phantom reviewer without an autonomous reviewer agent +# --------------------------------------------------------------------------- + + +def test_review_dispatch_gate_prevents_phantom_reviewer( + kanban_home: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """With ``kanban.review_dispatch=false`` the dispatcher must NOT claim a + task parked in ``review`` (no autonomous reviewer in this deployment — + it waits for a human). Flipping the knob back on proves the gate, not + something else, is what suppressed the claim.""" + import hermes_cli.config as cfgmod + import hermes_cli.profiles as profmod + + with kb.connect() as conn: + tid = kb.create_task(conn, title="park", assignee="worker") + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="done", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "review" + + # The assignee profile is spawnable — so ONLY the gate can stop the + # review-column dispatch from claiming it. + monkeypatch.setattr(profmod, "profile_exists", lambda name: True) + + # Gate OFF -> review task is left alone. + monkeypatch.setattr( + cfgmod, "load_config", + lambda *a, **k: {"kanban": {"review_dispatch": False}}, + ) + res_off = kb.dispatch_once(conn, dry_run=True) + assert tid not in [s[0] for s in res_off.spawned] + assert kb.get_task(conn, tid).status == "review" + + # Gate ON (opt-in; requires an installed sdlc-review agent) -> the + # review task is picked up by the dispatcher. + monkeypatch.setattr( + cfgmod, "load_config", + lambda *a, **k: {"kanban": {"review_dispatch": True}}, + ) + res_on = kb.dispatch_once(conn, dry_run=True) + assert tid in [s[0] for s in res_on.spawned] + + +# --------------------------------------------------------------------------- +# reopen: a follow-up sends a review task back out for another pass +# --------------------------------------------------------------------------- + + +def test_reopen_review_task_returns_to_ready(kanban_home: Path) -> None: + """The "changes requested" / follow-up path: a task parked in ``review`` + goes back to ``ready`` so the dispatcher re-runs the implementer. It must + NOT touch ``block_recurrences`` (review was never a block).""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="reopen me", assignee="worker") + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="v1", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "review" + + ok = kb.reopen_review_task(conn, tid) + assert ok is True + row = _row(conn, tid) + assert row["status"] == "ready" + assert row["current_run_id"] is None + assert (row["block_recurrences"] or 0) == 0 + assert _events(conn, tid, kind="review_reopened") + + # Idempotent: not in review anymore -> reopening again is a no-op. + assert kb.reopen_review_task(conn, tid) is False + + +def test_review_cycle_end_to_end(kanban_home: Path) -> None: + """Full loop: run -> review -> follow-up reopen -> re-run -> review -> + approve -> done. Never blocks, never triages, and stays wake-subscribed + until done.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="cycle", assignee="worker") + + # Pass 1: implement -> review. + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="v1", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "review" + + # Human asks for changes -> reopen -> re-run. + assert kb.reopen_review_task(conn, tid) is True + assert kb.get_task(conn, tid).status == "ready" + kb.claim_task(conn, tid) + kb.request_review( + conn, tid, summary="v2", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "review" + + # Human approves. + assert kb.complete_task(conn, tid, summary="approved") is True + row = _row(conn, tid) + assert row["status"] == "done" + assert (row["block_recurrences"] or 0) == 0 + assert _events(conn, tid, kind="block_loop_detected") == [] + + +# --------------------------------------------------------------------------- +# never-claimed 'ready' task: handoff must survive via a synthesized run +# --------------------------------------------------------------------------- + + +def test_request_review_on_unclaimed_ready_synthesizes_run(kanban_home: Path) -> None: + """A manual/CLI request-review on a never-claimed ``ready`` task has no + active run to close. The handoff summary must still be preserved on a + synthesized run so the reviewer keeps the context.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="ready then review", assignee="worker") + assert kb.get_task(conn, tid).status == "ready" + assert kb.get_task(conn, tid).current_run_id is None + + ok = kb.request_review(conn, tid, summary="done without a claim") + assert ok is True + assert kb.get_task(conn, tid).status == "review" + + run = _last_run(conn, tid) + assert run is not None + assert run["outcome"] == "review_requested" + assert run["summary"] == "done without a claim" + # Exactly one review_requested event, carrying the handoff summary. + evs = _events(conn, tid, kind="review_requested") + assert len(evs) == 1 + assert evs[0][1]["summary"] == "done without a claim" + + +def test_reviewer_is_informational_and_does_not_reassign(kanban_home: Path) -> None: + """``reviewer`` is recorded on the event but must NOT reassign the task — + in the human-review model the task stays attributed to the implementer.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="keep assignee", assignee="worker") + kb.claim_task(conn, tid) + ok = kb.request_review( + conn, tid, summary="v1", reviewer="lead-reviewer", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert ok is True + # Assignee unchanged. + assert kb.get_task(conn, tid).assignee == "worker" + # Reviewer captured on the event payload for downstream context. + ev = _events(conn, tid, kind="review_requested")[0][1] + assert ev["reviewer"] == "lead-reviewer" + assert ev["implementer"] == "worker" diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index fdf846d7353a4..1a40acd74845b 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -890,6 +890,59 @@ def _handle_block(args: dict, **kw) -> str: return tool_error(f"kanban_block: {e}") +def _handle_request_review(args: dict, **kw) -> str: + """Move the task to 'review' — implementation done, awaiting a human.""" + tid = _default_task_id(args.get("task_id")) + if not tid: + return tool_error( + "task_id is required (or set HERMES_KANBAN_TASK in the env)" + ) + ownership_err = _enforce_worker_task_ownership(tid) + if ownership_err: + return ownership_err + summary = args.get("summary") + if not summary or not str(summary).strip(): + return tool_error( + "summary is required — describe what was implemented and how it " + "was verified so the reviewer has context" + ) + summary = redact_sensitive_text(str(summary), force=True) + reviewer = args.get("reviewer") or None + if reviewer: + # Model-supplied free text stored durably on the event payload — + # redact like summary / kanban_block's reason. + reviewer = redact_sensitive_text(str(reviewer), force=True) + board = args.get("board") + try: + kb, conn = _connect(board=board) + try: + ok = kb.request_review( + conn, tid, + summary=summary, + reviewer=reviewer, + expected_run_id=_worker_run_id(tid), + ) + if not ok: + return tool_error( + f"could not request review for {tid} (unknown id or not " + f"in running/ready)" + ) + run = kb.latest_run(conn, tid) + landed = kb.get_task(conn, tid) + return _ok( + task_id=tid, + run_id=run.id if run else None, + status=landed.status if landed else "review", + ) + finally: + conn.close() + except ValueError as e: + return tool_error(f"kanban_request_review: {e}") + except Exception as e: + logger.exception("kanban_request_review failed") + return tool_error(f"kanban_request_review: {e}") + + def _handle_heartbeat(args: dict, **kw) -> str: """Signal that the worker is still alive during a long operation. @@ -1765,6 +1818,46 @@ KANBAN_BLOCK_SCHEMA = { }, } +KANBAN_REQUEST_REVIEW_SCHEMA = { + "name": "kanban_request_review", + "description": ( + "Hand the task off for review: implementation, self-review, and " + "verification are complete and you want a human (or reviewer) to " + "look before it is marked done. Moves the task to the 'review' " + "column and notifies the subscriber. Unlike ``kanban_block`` this is " + "NOT a blocker — it never counts toward unblock-loop detection, so a " + "task can cycle through review across follow-ups without ever being " + "falsely escalated to triage. Use this instead of blocking with a " + "free-form 'review-required:' reason." + ), + "parameters": { + "type": "object", + "properties": { + "task_id": { + "type": "string", + "description": _DESC_TASK_ID_DEFAULT, + }, + "summary": { + "type": "string", + "description": ( + "What was implemented and how it was verified, in one or " + "two sentences — shown to the human reviewer. Don't paste " + "the whole diff; the reviewer has the board and the PR." + ), + }, + "reviewer": { + "type": "string", + "description": ( + "Optional profile/handle to attribute the review to. " + "Omit when a human reviews." + ), + }, + "board": _board_schema_prop(), + }, + "required": ["summary"], + }, +} + KANBAN_HEARTBEAT_SCHEMA = { "name": "kanban_heartbeat", "description": ( @@ -2177,6 +2270,15 @@ registry.register( emoji="⏸", ) +registry.register( + name="kanban_request_review", + toolset="kanban", + schema=KANBAN_REQUEST_REVIEW_SCHEMA, + handler=_handle_request_review, + check_fn=_check_kanban_mode, + emoji="👀", +) + registry.register( name="kanban_heartbeat", toolset="kanban", diff --git a/toolsets.py b/toolsets.py index b1001bf116eb4..d7ab3f8ab50c0 100644 --- a/toolsets.py +++ b/toolsets.py @@ -81,7 +81,8 @@ _HERMES_CORE_TOOLS = [ # profile explicitly enables the kanban toolset. Gated via check_fn in # tools/kanban_tools.py. "kanban_show", "kanban_list", - "kanban_complete", "kanban_block", "kanban_heartbeat", + "kanban_complete", "kanban_block", "kanban_request_review", + "kanban_heartbeat", "kanban_comment", "kanban_create", "kanban_link", "kanban_unblock", "kanban_attach", "kanban_attach_url", "kanban_attachments", @@ -314,12 +315,14 @@ TOOLSETS = { "is spawned by the kanban dispatcher (HERMES_KANBAN_TASK env " "set). The dispatcher runs inside the gateway by default; see " "`kanban.dispatch_in_gateway` in config.yaml. Lets workers mark " - "tasks done with structured handoffs, block for human input, " + "tasks done with structured handoffs, hand off for human review " + "(request_review — not a block), block for human input, " "heartbeat during long ops, comment on threads, attach files, and " "(for orchestrators) list, unblock, and fan out tasks." ), "tools": [ "kanban_show", "kanban_list", "kanban_complete", "kanban_block", + "kanban_request_review", "kanban_heartbeat", "kanban_comment", "kanban_create", "kanban_link", "kanban_unblock", diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index c9dbb927a5e47..74beb1643b6d8 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -596,7 +596,7 @@ Multi-profile, multi-project collaboration board. Each install can host many boa |------|---------| | `--board ` | Operate on a specific board. Defaults to the current board (set via `hermes kanban boards switch`, the `HERMES_KANBAN_BOARD` env var, or `default`). | -**This is the human / scripting surface.** Agent workers spawned by the dispatcher drive the board through a dedicated `kanban_*` [toolset](/user-guide/features/kanban#how-workers-interact-with-the-board) (`kanban_show`, `kanban_complete`, `kanban_block`, `kanban_create`, `kanban_link`, `kanban_comment`, `kanban_heartbeat`; orchestrator profiles also get `kanban_list` and `kanban_unblock`) instead of shelling to `hermes kanban`. Workers have `HERMES_KANBAN_BOARD` pinned in their env so they physically cannot see other boards. +**This is the human / scripting surface.** Agent workers spawned by the dispatcher drive the board through a dedicated `kanban_*` [toolset](/user-guide/features/kanban#how-workers-interact-with-the-board) (`kanban_show`, `kanban_complete`, `kanban_request_review`, `kanban_block`, `kanban_create`, `kanban_link`, `kanban_comment`, `kanban_heartbeat`; orchestrator profiles also get `kanban_list` and `kanban_unblock`) instead of shelling to `hermes kanban`. Workers have `HERMES_KANBAN_BOARD` pinned in their env so they physically cannot see other boards. | Action | Purpose | |--------|---------| @@ -617,6 +617,8 @@ Multi-profile, multi-project collaboration board. Each install can host many boa | `comment ""` | Append a comment. The next worker that claims the task reads it as part of its `kanban_show()` response. | | `complete ` | Mark task done. Flags: `--result`, `--summary`, `--metadata`. | | `block ""` | Mark task blocked for human input. Also appends the reason as a comment. | +| `request-review ` | Move a task to `review` (implementation done, awaiting human review) — NOT a block. Flags: `--summary`, `--reviewer`. | +| `reopen-review ...` | Send review task(s) back for changes (`review` → ready/todo). Flag: `--reason` (appended as a comment). | | `schedule ""` | Park time-delay/follow-up work in `scheduled` so it is not shown as a human blocker. | | `unblock ` | Return a blocked or scheduled task to ready (or `todo` if dependencies are still open). | | `archive ` | Hide from default list. `gc` will remove scratch workspaces. | diff --git a/website/docs/reference/tools-reference.md b/website/docs/reference/tools-reference.md index c05f93e193409..3aed7de961e15 100644 --- a/website/docs/reference/tools-reference.md +++ b/website/docs/reference/tools-reference.md @@ -126,6 +126,7 @@ Registered when the agent is either (a) spawned by the kanban dispatcher (`HERME | `kanban_list` | List board tasks with filters. Orchestrator-only; hidden from dispatcher-spawned task workers. | profile with `kanban` toolset | | `kanban_complete` | Mark the current task done with a structured handoff payload (results, artifacts, follow-ups). | `HERMES_KANBAN_TASK` or `kanban` toolset | | `kanban_block` | Block the current task on a question for the user — the dispatcher pauses, surfaces the question, and resumes once a human replies. | `HERMES_KANBAN_TASK` or `kanban` toolset | +| `kanban_request_review` | Hand the task off for human review (implementation complete): moves it to the `review` column and wakes the subscriber. NOT a block — never counts toward unblock-loop detection, so review can cycle across follow-ups. Use instead of `kanban_block("review-required: …")`. | `HERMES_KANBAN_TASK` or `kanban` toolset | | `kanban_heartbeat` | Send a progress heartbeat during a long-running operation so the dispatcher knows the worker is still alive. | `HERMES_KANBAN_TASK` or `kanban` toolset | | `kanban_comment` | Add a comment to the task thread without changing its state — useful for surfacing intermediate findings. | `HERMES_KANBAN_TASK` or `kanban` toolset | | `kanban_create` | Fan out child tasks from the current task. Used by orchestrators and follow-up-spawning workers. | `HERMES_KANBAN_TASK` or `kanban` toolset | diff --git a/website/docs/reference/toolsets-reference.md b/website/docs/reference/toolsets-reference.md index fc92521ad90b4..131fe4b640c44 100644 --- a/website/docs/reference/toolsets-reference.md +++ b/website/docs/reference/toolsets-reference.md @@ -69,7 +69,7 @@ Or in-session: | `context_engine` | (varies) | Runtime tools exposed by the active context-engine plugin (empty until a plugin populates it). | | `image_gen` | `image_generate` | Text-to-image generation via FAL.ai (with opt-in OpenAI / xAI backends). | | `video_gen` | `video_generate`, `xai_video_edit`, `xai_video_extend` | Text-to-video and image-to-video via plugin-registered backends (xAI Grok-Imagine, FAL.ai Veo 3.1 / Pixverse v6 / Kling O3). Pass `image_url` to animate an image; omit it for text-to-video. `xai_video_edit` / `xai_video_extend` are provider-specific edit/extend tools, gated on xAI Imagine credentials. | -| `kanban` | `kanban_attach`, `kanban_attach_url`, `kanban_attachments`, `kanban_block`, `kanban_comment`, `kanban_complete`, `kanban_create`, `kanban_heartbeat`, `kanban_link`, `kanban_list`, `kanban_show`, `kanban_unblock` | Multi-agent coordination tools. Registered for dispatcher-spawned task workers (`HERMES_KANBAN_TASK`) and for profiles that explicitly list the `kanban` toolset by name (the `all`/`*` wildcard does **not** enable it). Workers mark tasks done, block, heartbeat, comment, and create/link follow-up tasks; orchestrator profiles additionally get board-routing tools like list/unblock. `delegate_task` children are not Kanban run owners: their schema strips/disables this toolset and runtime guards reject direct board mutations, even if parent `HERMES_KANBAN_*` env vars are present. | +| `kanban` | `kanban_attach`, `kanban_attach_url`, `kanban_attachments`, `kanban_block`, `kanban_comment`, `kanban_complete`, `kanban_create`, `kanban_heartbeat`, `kanban_link`, `kanban_list`, `kanban_request_review`, `kanban_show`, `kanban_unblock` | Multi-agent coordination tools. Registered for dispatcher-spawned task workers (`HERMES_KANBAN_TASK`) and for profiles that explicitly list the `kanban` toolset by name (the `all`/`*` wildcard does **not** enable it). Workers mark tasks done, request first-class review, block, heartbeat, comment, and create/link follow-up tasks; orchestrator profiles additionally get board-routing tools like list/unblock. `delegate_task` children are not Kanban run owners: their schema strips/disables this toolset and runtime guards reject direct board mutations, even if parent `HERMES_KANBAN_*` env vars are present. | | `memory` | `memory` | Persistent cross-session memory management. | | `desktop_ui` | `close_terminal`, `focus_pane`, `open_preview`, `react_to_message`, `read_preview`, `read_terminal`, `read_window_below` | Affordances that act on the Hermes desktop app itself — read/close the embedded terminal pane, open and read the in-app browser, identify the OS window behind the app, reveal a pane, react to a message. Enabled for sessions whose source is the desktop app, whichever backend it's connected to (local, SSH, URL, or Hermes Cloud). Never present on CLI, TUI, messaging, or cron sessions. | | `project` | `project_create`, `project_list`, `project_switch` | Create and switch desktop [Projects](../user-guide/cli.md) (named, multi-folder workspaces). GUI / desktop sessions only. | diff --git a/website/docs/user-guide/features/kanban-worker-lanes.md b/website/docs/user-guide/features/kanban-worker-lanes.md index 69f879c6b1132..bbf2559983693 100644 --- a/website/docs/user-guide/features/kanban-worker-lanes.md +++ b/website/docs/user-guide/features/kanban-worker-lanes.md @@ -18,7 +18,7 @@ Reviewer = human or human-proxy that gates "done" GitHub PR = upstreamable artifact (optional, for code lanes) ``` -Hermes Kanban owns lifecycle truth — `ready` → `running` → `blocked` / `done` / `archived`. Worker lanes execute work but never own that truth; everything they do flows back through the kanban kernel via the `kanban_*` tools (or, for non-Hermes external workers, via the API). Reviewers gate the transition from "code change written" to "task done." +Hermes Kanban owns lifecycle truth — `ready` → `running` → `review` / `blocked` / `done` / `archived`. Worker lanes execute work but never own that truth; everything they do flows back through the kanban kernel via the `kanban_*` tools (or, for non-Hermes external workers, via the API). Reviewers gate the transition from "code change written" to "task done." ## What a lane provides @@ -51,27 +51,28 @@ For non-Hermes lanes (registered via a plugin), the plugin supplies its own `spa Every claim must end in exactly one of: - `kanban_complete(summary=..., metadata=...)` — task succeeds, status flips to `done`. +- `kanban_request_review(summary=...)` — implementation is complete but needs a human review before counting as done; status flips to `review` and the subscriber is woken. This is NOT a block — it never counts toward unblock-loop detection, so a task can cycle through review across follow-ups. The reviewer approves with `kanban_complete` or sends it back for changes (`review -> ready`). - `kanban_block(reason=...)` — task waits for human input, status flips to `blocked`. The dispatcher respawns when `kanban_unblock` runs. - The worker process exits without a tool call. The kernel reaps it and emits `crashed` (PID died) or `gave_up` (consecutive-failure breaker tripped) or `timed_out` (max_runtime exceeded). This is the failure path; healthy workers don't end here. The kanban kernel enforces that exactly one of these terminates each run. A worker that calls neither and exits normally is treated as crashed. -## Outputs and the review-required convention +## Outputs and the review handoff -For most code-changing tasks, the work isn't truly *done* the moment the worker finishes — it needs a human reviewer. The kanban kernel doesn't enforce this distinction (a "code-changing task" is fuzzy and forcing block-instead-of-complete on every code worker would break flows where no review is wanted). It's a convention layered on top: +For most code-changing tasks, the work isn't truly *done* the moment the worker finishes — it needs a human reviewer. The kanban kernel doesn't enforce this distinction (a "code-changing task" is fuzzy and forcing it on every code worker would break flows where no review is wanted). It's a convention layered on top, with a first-class terminator: -- **Block instead of complete**, with `reason` prefixed `review-required: ` so the dashboard / `hermes kanban show` surfaces the row as awaiting review. -- **Drop structured metadata into a `kanban_comment` first** since `kanban_block` only carries the human-readable `reason`. Comments are the durable annotation channel — every audit-relevant field (changed_files, tests_run, diff_path or PR url, decisions) belongs there. -- **Reviewer either approves and unblocks**, which respawns the worker with the comment thread for follow-ups; or asks for changes via another comment, which the next worker run sees as part of `kanban_show`'s context. +- **Request review instead of completing**: end with `kanban_request_review(summary=...)`. The task moves to the `review` column (implementation complete, awaiting a human) and the subscriber is woken. Unlike a block, this never counts toward unblock-loop detection, so a task can cycle through review across follow-ups without being falsely escalated to triage. Do NOT encode `review-required:` into a `kanban_block` reason — that routes through the block loop-breaker and eventually strands the task in triage. +- **Drop structured metadata into a `kanban_comment` first** since the review handoff carries only the human-readable `summary`. Comments are the durable annotation channel — every audit-relevant field (changed_files, tests_run, diff_path or PR url, decisions) belongs there. +- **Reviewer either approves** — `kanban_complete`, or move the card to `done` — which finishes the task; **or asks for changes**, sending the card back out of `review` to `ready`/`todo` (`hermes kanban reopen-review`, or drag it on the dashboard), which respawns the worker with the comment thread as part of `kanban_show`'s context. -The injected `KANBAN_GUIDANCE` covers both `kanban_complete` (truly terminal tasks — typo fixes, docs changes, research writeups) and the `review-required` block pattern. +The injected `KANBAN_GUIDANCE` covers `kanban_complete` (truly terminal tasks — typo fixes, docs changes, research writeups), the `kanban_request_review` handoff, and `kanban_block` (genuine blockers awaiting human input). ## Logs and audit trail The dispatcher writes per-task worker stdout/stderr to `/logs/.log`. Logs are auditable from kanban metadata: - `task_runs` rows carry the `log_path`, exit code (where available), summary, and metadata. -- `task_events` rows carry every state transition (`promoted`, `claimed`, `heartbeat`, `completed`, `blocked`, `gave_up`, `crashed`, `timed_out`, `reclaimed`, `claim_extended`). +- `task_events` rows carry every state transition (`promoted`, `claimed`, `heartbeat`, `completed`, `blocked`, `review_requested`, `review_reopened`, `gave_up`, `crashed`, `timed_out`, `reclaimed`, `claim_extended`). - `kanban_show` returns both, so a reviewer (or a follow-up worker) reading the task gets the full history without needing dashboard access. The dashboard renders run history with summaries, metadata blocks, and exit-status badges. CLI users can run `hermes kanban tail ` to follow live, or `hermes kanban runs ` for the historical attempt list. diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index 9f05c44894526..ce87c5fbd5d6d 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -14,7 +14,7 @@ Hermes Kanban is a durable task board, shared across all your Hermes profiles, t The board has two front doors, both backed by the same `~/.hermes/kanban.db`: -- **Agents drive the board through a dedicated `kanban_*` toolset** — `kanban_show`, `kanban_list`, `kanban_complete`, `kanban_block`, `kanban_heartbeat`, `kanban_comment`, `kanban_attach`, `kanban_attach_url`, `kanban_attachments`, `kanban_create`, `kanban_link`, `kanban_unblock`. The dispatcher spawns each worker with these tools already in its schema; orchestrator profiles can also enable the `kanban` toolset explicitly. The model reads and routes tasks by calling tools directly, *not* by shelling out to `hermes kanban`. See [How workers interact with the board](#how-workers-interact-with-the-board) below. +- **Agents drive the board through a dedicated `kanban_*` toolset** — `kanban_show`, `kanban_list`, `kanban_complete`, `kanban_request_review`, `kanban_block`, `kanban_heartbeat`, `kanban_comment`, `kanban_attach`, `kanban_attach_url`, `kanban_attachments`, `kanban_create`, `kanban_link`, `kanban_unblock`. The dispatcher spawns each worker with these tools already in its schema; orchestrator profiles can also enable the `kanban` toolset explicitly. The model reads and routes tasks by calling tools directly, *not* by shelling out to `hermes kanban`. See [How workers interact with the board](#how-workers-interact-with-the-board) below. - **You (and scripts, and cron) drive the board through `hermes kanban …`** on the CLI, `/kanban …` as a slash command, or the dashboard. These are for humans and automation — the places without a tool-calling model behind them. Both surfaces route through the same `kanban_db` layer, so reads see a consistent view and writes can't drift. The rest of this page shows CLI examples because they're easy to copy-paste, but every CLI verb has a tool-call equivalent the model uses. @@ -59,7 +59,7 @@ They coexist: a kanban worker may call `delegate_task` internally during its run (e.g. one per project, repo, or domain); see [Boards (multi-project)](#boards-multi-project) below. Single-project users stay on the `default` board and never see the word "board" outside this docs section. -- **Task** — a row with title, optional body, one assignee (a profile name), status (`triage | todo | ready | running | blocked | done | archived`), optional tenant namespace, optional idempotency key (dedup for retried automation). +- **Task** — a row with title, optional body, one assignee (a profile name), status (`triage | todo | ready | running | blocked | review | done | archived`), optional tenant namespace, optional idempotency key (dedup for retried automation). - **Link** — `task_links` row recording a parent → child dependency. The dispatcher promotes `todo → ready` when all parents are `done`. - **Comment** — the inter-agent protocol. Agents and humans append comments; when a worker is (re-)spawned it reads the full comment thread as part of its context. - **Workspace** — the directory a worker operates in. Three kinds: @@ -226,6 +226,9 @@ up on the next tick (60s by default). kanban: dispatch_in_gateway: true # default dispatch_interval_seconds: 60 # default + review_dispatch: false # default: no autonomous reviewer — tasks in + # 'review' wait for a human. Set true only if + # you install an sdlc-review agent. ``` Override the config flag at runtime via `HERMES_KANBAN_DISPATCH_IN_GATEWAY=0` @@ -293,6 +296,7 @@ parent, missing input, unmet capability) before unblocking, or raise | `kanban_show` | Read the current task (title, body, prior attempts, parent handoffs, comments, full pre-formatted `worker_context`). Defaults to the env's task id. | — | | `kanban_list` | List task summaries with filters for `assignee`, `status`, `tenant`, archived visibility, and limit. Intended for orchestrators discovering board work. | — | | `kanban_complete` | Finish with `summary` + `metadata` structured handoff. | at least one of `summary` / `result` | +| `kanban_request_review` | Hand off for human review: implementation done, moves the task to `review` and wakes the subscriber. NOT a block — never counts toward unblock-loop detection, so review can cycle across follow-ups. Use instead of `kanban_block("review-required: …")`. | `summary` | | `kanban_block` | Stop work and route by why: `kind=dependency` (waits in `todo`, auto-resumes), `needs_input`/`capability`/`transient` (surface to a human). Repeated same-kind re-blocks auto-escalate to `triage`. | `reason` | | `kanban_heartbeat` | Signal liveness during long operations. Pure side-effect. | — | | `kanban_comment` | Append a durable note to the task thread. | `task_id`, `body` | @@ -743,6 +747,9 @@ hermes kanban block "" [--ids ...] hermes kanban unblock ... hermes kanban archive ... +hermes kanban request-review [--summary "..."] [--reviewer NAME] # implementation done -> 'review' (not a block) +hermes kanban reopen-review ... [--reason "..."] # changes requested: 'review' -> ready/todo + hermes kanban tail # follow a single task's event stream hermes kanban watch [--assignee P] [--tenant T] # live stream ALL events to the terminal [--kinds completed,blocked,…] [--interval SECS] diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban-worker-lanes.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban-worker-lanes.md index 5d728eed7fbc1..8430c2d16ee59 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban-worker-lanes.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban-worker-lanes.md @@ -18,7 +18,7 @@ Reviewer = 人工或人工代理,负责把关"完成"状态 GitHub PR = 可上游的产物(可选,适用于代码通道) ``` -Hermes Kanban 拥有生命周期的真实状态——`ready` → `running` → `blocked` / `done` / `archived`。Worker lane 执行工作,但从不拥有该真实状态;它们所做的一切都通过 `kanban_*` 工具回流至 kanban 内核(对于非 Hermes 外部 worker,则通过 API)。Reviewer 负责把关从"代码变更已写入"到"任务完成"的转换。 +Hermes Kanban 拥有生命周期的真实状态——`ready` → `running` → `review` / `blocked` / `done` / `archived`。Worker lane 执行工作,但从不拥有该真实状态;它们所做的一切都通过 `kanban_*` 工具回流至 kanban 内核(对于非 Hermes 外部 worker,则通过 API)。Reviewer 负责把关从"代码变更已写入"到"任务完成"的转换。 ## 通道需提供的内容 @@ -51,27 +51,28 @@ Hermes Kanban 拥有生命周期的真实状态——`ready` → `running` → ` 每次 claim 必须以以下之一结束: - `kanban_complete(summary=..., metadata=...)` — 任务成功,状态切换为 `done`。 +- `kanban_request_review(summary=...)` — 实现已完成,但在计为 done 之前需要人工审查;状态切换为 `review`,并唤醒订阅者。这**不是** block——它从不计入解除阻塞循环检测,因此任务可以在多次后续跟进中反复进入审查。Reviewer 通过 `kanban_complete` 批准,或将其退回修改(`review -> ready`)。 - `kanban_block(reason=...)` — 任务等待人工输入,状态切换为 `blocked`。调度器在 `kanban_unblock` 运行时重新生成。 - worker 进程退出而未调用任何工具。内核回收该进程并发出 `crashed`(PID 已消亡)、`gave_up`(连续失败断路器触发)或 `timed_out`(超过 max_runtime)。这是失败路径;健康的 worker 不会在此结束。 kanban 内核强制要求每次运行恰好由其中一项终止。既未调用任何终止工具又正常退出的 worker 将被视为崩溃。 -## 输出与 review-required 约定 +## 输出与审查交接 -对于大多数涉及代码变更的任务,worker 完成的那一刻并不意味着真正*完成*——还需要人工审查。kanban 内核不强制执行这一区分("涉及代码变更的任务"定义模糊,且在每个代码 worker 上强制 block 而非 complete 会破坏不需要审查的流程)。这是叠加在上层的约定: +对于大多数涉及代码变更的任务,worker 完成的那一刻并不意味着真正*完成*——还需要人工审查。kanban 内核不强制执行这一区分("涉及代码变更的任务"定义模糊,且在每个代码 worker 上强制执行会破坏不需要审查的流程)。这是叠加在上层的约定,并拥有一等的终止动作: -- **使用 block 而非 complete**,`reason` 以 `review-required: ` 为前缀,使仪表板 / `hermes kanban show` 将该行显示为等待审查。 -- **先将结构化元数据写入 `kanban_comment`**,因为 `kanban_block` 只携带人类可读的 `reason`。Comment 是持久的注解通道——所有与审计相关的字段(changed_files、tests_run、diff_path 或 PR url、决策记录)都应放在这里。 -- **Reviewer 批准并解除阻塞**,这将重新生成 worker 并附带 comment 线程用于后续跟进;或通过另一条 comment 要求修改,下一次 worker 运行时将通过 `kanban_show` 的上下文看到这些内容。 +- **请求审查而非完成**:以 `kanban_request_review(summary=...)` 结束。任务移入 `review` 列(实现已完成,等待人工),并唤醒订阅者。与 block 不同,它从不计入解除阻塞循环检测,因此任务可以在多次后续跟进中反复进入审查而不会被误升级到 triage。**不要**把 `review-required:` 编码进 `kanban_block` 的 reason——那会经过 block 循环断路器,最终把任务困在 triage。 +- **先将结构化元数据写入 `kanban_comment`**,因为审查交接只携带人类可读的 `summary`。Comment 是持久的注解通道——所有与审计相关的字段(changed_files、tests_run、diff_path 或 PR url、决策记录)都应放在这里。 +- **Reviewer 要么批准**(`kanban_complete`,或将卡片移至 `done`)以结束任务;**要么要求修改**,将卡片从 `review` 退回 `ready`/`todo`(`hermes kanban reopen-review`,或在仪表板上拖拽),这将重新生成 worker 并附带 comment 线程作为 `kanban_show` 上下文的一部分。 -自动注入的 `KANBAN_GUIDANCE` 同时涵盖 `kanban_complete`(真正终态的任务——拼写修复、文档变更、研究报告)和 `review-required` block 模式。 +自动注入的 `KANBAN_GUIDANCE` 涵盖 `kanban_complete`(真正终态的任务——拼写修复、文档变更、研究报告)、`kanban_request_review` 交接,以及 `kanban_block`(等待人工输入的真正阻塞)。 ## 日志与审计追踪 调度器将每个任务的 worker stdout/stderr 写入 `/logs/.log`。日志可通过 kanban 元数据进行审计: - `task_runs` 行携带 `log_path`、退出码(如有)、摘要和元数据。 -- `task_events` 行携带每次状态转换(`promoted`、`claimed`、`heartbeat`、`completed`、`blocked`、`gave_up`、`crashed`、`timed_out`、`reclaimed`、`claim_extended`)。 +- `task_events` 行携带每次状态转换(`promoted`、`claimed`、`heartbeat`、`completed`、`blocked`、`review_requested`、`review_reopened`、`gave_up`、`crashed`、`timed_out`、`reclaimed`、`claim_extended`)。 - `kanban_show` 同时返回两者,因此 reviewer(或后续 worker)读取任务时无需访问仪表板即可获得完整历史。 仪表板以摘要、元数据块和退出状态徽章渲染运行历史。CLI 用户可运行 `hermes kanban tail ` 实时跟踪,或运行 `hermes kanban runs ` 查看历史尝试列表。 diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md index 763f60ac42bf0..5ca0614266af0 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/kanban.md @@ -14,7 +14,7 @@ Hermes Kanban 是一个持久化任务看板,在所有 Hermes 配置文件之 看板有两个入口,均由同一个 `~/.hermes/kanban.db` 支撑: -- **Agent 通过专用 `kanban_*` 工具集驱动看板** —— `kanban_show`、`kanban_list`、`kanban_complete`、`kanban_block`、`kanban_heartbeat`、`kanban_comment`、`kanban_create`、`kanban_link`、`kanban_unblock`。调度器在 schema 中已内置这些工具来启动每个 worker;编排器(orchestrator)配置文件也可以通过 `kanban` 工具集显式启用。模型通过直接调用工具来读取和路由任务,*而不是*通过 shell 执行 `hermes kanban`。详见下方[Worker 如何与看板交互](#how-workers-interact-with-the-board)。 +- **Agent 通过专用 `kanban_*` 工具集驱动看板** —— `kanban_show`、`kanban_list`、`kanban_complete`、`kanban_block`、`kanban_request_review`、`kanban_heartbeat`、`kanban_comment`、`kanban_create`、`kanban_link`、`kanban_unblock`。调度器在 schema 中已内置这些工具来启动每个 worker;编排器(orchestrator)配置文件也可以通过 `kanban` 工具集显式启用。模型通过直接调用工具来读取和路由任务,*而不是*通过 shell 执行 `hermes kanban`。详见下方[Worker 如何与看板交互](#how-workers-interact-with-the-board)。 - **你(以及脚本和 cron)通过 CLI 上的 `hermes kanban …`、斜杠命令 `/kanban …` 或仪表盘驱动看板。** 这些界面面向人类和自动化场景——即没有工具调用模型的场合。 两个界面都通过同一个 `kanban_db` 层路由,因此读取视图一致,写入不会产生偏差。本页其余部分展示 CLI 示例,因为它们便于复制粘贴,但每个 CLI 动词都有模型使用的等效工具调用。 @@ -55,7 +55,7 @@ Hermes Kanban 是一个持久化任务看板,在所有 Hermes 配置文件之 ## 核心概念 - **Board(看板)** —— 一个独立的任务队列,拥有自己的 SQLite DB、工作区目录和调度器循环。单次安装可以有多个看板(例如每个项目、仓库或领域一个);详见下方[看板(多项目)](#boards-multi-project)。单项目用户保持使用 `default` 看板,在本文档章节之外不会看到"board"这个词。 -- **Task(任务)** —— 包含标题、可选正文、一个受让人(配置文件名称)、状态(`triage | todo | ready | running | blocked | done | archived`)、可选租户命名空间、可选幂等键(用于重试自动化的去重)的一行记录。 +- **Task(任务)** —— 包含标题、可选正文、一个受让人(配置文件名称)、状态(`triage | todo | ready | running | blocked | review | done | archived`)、可选租户命名空间、可选幂等键(用于重试自动化的去重)的一行记录。 - **Link(链接)** —— `task_links` 行,记录父 → 子依赖关系。当所有父任务变为 `done` 时,调度器将 `todo → ready`。 - **Comment(评论)** —— agent 间协议。Agent 和人类追加评论;当 worker 被(重新)启动时,它将完整的评论线程作为上下文的一部分读取。 - **Workspace(工作区)** —— worker 操作的目录。三种类型: @@ -161,6 +161,8 @@ hermes kanban stats kanban: dispatch_in_gateway: true # 默认 dispatch_interval_seconds: 60 # 默认 + review_dispatch: false # 默认:无自动 reviewer —— 'review' 中的任务 + # 等待人工。仅在安装了 sdlc-review agent 时设为 true。 ``` 通过 `HERMES_KANBAN_DISPATCH_IN_GATEWAY=0` 在运行时覆盖配置标志以进行调试。标准 gateway 监督适用:直接运行 `hermes gateway start`,或将 gateway 配置为 systemd 用户单元(参见 gateway 文档)。没有运行中的 gateway,`ready` 任务会保持原状,直到 gateway 启动 —— `hermes kanban create` 在创建时会对此发出警告。 @@ -198,6 +200,7 @@ hermes kanban block t_abc "need input" --ids t_def t_hij | `kanban_show` | 读取当前任务(标题、正文、先前尝试、父级交接、评论、完整预格式化的 `worker_context`)。默认使用环境变量中的任务 id。 | — | | `kanban_list` | 列出带有 `assignee`、`status`、`tenant`、归档可见性和限制过滤器的任务摘要。供编排器发现看板工作使用。 | — | | `kanban_complete` | 以 `summary` + `metadata` 结构化交接完成任务。 | `summary` / `result` 至少一个 | +| `kanban_request_review` | 交接人工审查:实现已完成,任务移入 `review` 列并唤醒订阅者。**不是** block —— 从不计入解除阻塞循环检测,因此审查可在多次后续跟进中反复进行。用它替代 `kanban_block("review-required: …")`。 | `summary` | | `kanban_block` | 以 `reason` 上报需要人工输入。 | `reason` | | `kanban_heartbeat` | 在长时间操作期间发出存活信号。纯副作用。 | — | | `kanban_comment` | 向任务线程追加持久化备注。 | `task_id`、`body` | @@ -557,6 +560,9 @@ hermes kanban block "" [--ids ...] hermes kanban unblock ... hermes kanban archive ... +hermes kanban request-review [--summary "..."] [--reviewer NAME] # 实现完成 -> 'review'(非 block) +hermes kanban reopen-review ... [--reason "..."] # 请求修改:'review' -> ready/todo + hermes kanban tail # 跟踪单个任务的事件流 hermes kanban watch [--assignee P] [--tenant T] # 将所有事件实时流式传输到终端 [--kinds completed,blocked,…] [--interval SECS]