From ae23b1f676276d8fa55ea9d9b7483f4925bc7d6a Mon Sep 17 00:00:00 2001 From: Jakub Wolniewicz <4850809+frizikk@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:29:28 +0200 Subject: [PATCH] fix: complete kanban review lifecycle Close the autonomous implement-review-rework loop, preserve parent gating and implementer provenance, distinguish downstream review cards, and surface legacy review dependency deadlocks immediately. Co-authored-by: kaishi00 <6590895+kaishi00@users.noreply.github.com> --- AGENTS.md | 5 +- acp_adapter/tools.py | 3 +- agent/prompt_builder.py | 32 +- agent/transports/hermes_tools_mcp_server.py | 1 + cli-config.yaml.example | 10 + cli.py | 12 +- gateway/kanban_watchers.py | 4 +- hermes_cli/config_defaults.py | 4 + hermes_cli/goals.py | 12 +- hermes_cli/kanban.py | 172 ++++++--- hermes_cli/kanban_db.py | 289 +++++++++++----- hermes_cli/kanban_diagnostics.py | 87 ++++- plugins/kanban/dashboard/dist/index.js | 2 +- plugins/kanban/dashboard/dist/style.css | 2 +- plugins/kanban/dashboard/plugin_api.py | 8 +- skills/devops/sdlc-review/SKILL.md | 141 ++++++++ tests/hermes_cli/test_kanban_notify.py | 54 +++ .../test_kanban_review_lifecycle.py | 16 +- .../test_kanban_review_lifecycle_complete.py | 260 ++++++++++++++ .../hermes_cli/test_kanban_review_surfaces.py | 325 ++++++++++++++++++ tests/plugins/test_kanban_dashboard_plugin.py | 34 ++ tools/kanban_tools.py | 189 ++++++++-- toolsets.py | 7 +- website/docs/reference/cli-commands.md | 5 +- website/docs/reference/tools-reference.md | 3 +- website/docs/reference/toolsets-reference.md | 2 +- .../features/kanban-worker-lanes.md | 17 +- website/docs/user-guide/features/kanban.md | 14 +- .../features/kanban-worker-lanes.md | 17 +- .../current/user-guide/features/kanban.md | 12 +- 30 files changed, 1506 insertions(+), 233 deletions(-) create mode 100644 skills/devops/sdlc-review/SKILL.md create mode 100644 tests/hermes_cli/test_kanban_review_lifecycle_complete.py create mode 100644 tests/hermes_cli/test_kanban_review_surfaces.py diff --git a/AGENTS.md b/AGENTS.md index ed5f5490a9..8230d860aa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1135,11 +1135,12 @@ 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`, - `request-review`, `reopen-review`, `block`, `unblock`, `archive`, + `request-review`, `request-changes`, `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_request_review`, `kanban_block`, + `kanban_show`, `kanban_complete`, `kanban_request_review`, + `kanban_request_changes`, `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 diff --git a/acp_adapter/tools.py b/acp_adapter/tools.py index 6e756c3fbf..bb997dc234 100644 --- a/acp_adapter/tools.py +++ b/acp_adapter/tools.py @@ -75,7 +75,8 @@ _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_request_review", "kanban_link", "kanban_heartbeat", + "kanban_block", "kanban_request_review", "kanban_request_changes", + "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 469b5974e1..42f608d655 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -237,23 +237,21 @@ KANBAN_GUIDANCE = ( "infer (missing credentials, UX choice, paywalled source, peer output you " "need first), call `kanban_block(reason=\"...\")` and stop. Don't guess. " "The user will unblock with context and the dispatcher will respawn you.\n" - "5. **Complete with structured handoff.** Call `kanban_complete(summary=..., " - "metadata=...)`. `summary` is 1–3 human-readable sentences naming concrete " - "artifacts. `metadata` is machine-readable facts " - "(`{changed_files: [...], tests_run: N, decisions: [...]}`). Downstream " - "workers read both via their own `kanban_show`. Never put secrets / " - "tokens / raw PII in either field — run rows are durable forever. " - "Exception: if your output is a code change that needs human review " - "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_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" + "5. **Finish with the review model encoded by the task graph.** Always " + "include the structured handoff (`summary`, `metadata`) on the lifecycle " + "transition itself; never put secrets, tokens, or raw PII in these durable " + "fields. If `kanban_show()` lists pre-created downstream review, " + "QA, or release children that depend on your task, call `kanban_complete`: " + "your implementation phase is done, and completion is what releases those " + "children. Never sticky-block that parent for `review-required` and never " + "request same-card review as well — either choice would strand or duplicate " + "the downstream lane. Otherwise, when this same task needs review before it " + "is final, call `kanban_request_review(summary=..., metadata=..., " + "reviewer=)`. The reviewer approves with " + "`kanban_complete`, returns actionable rework with " + "`kanban_request_changes`, or uses `kanban_block` only for a genuine " + "external escalation. Review is not a block, so repeated review cycles do " + "not trip unblock-loop detection.\n" "6. **If follow-up work appears, create it; don't do it.** Use " "`kanban_create(title=..., assignee=, parents=[your-task-id])` " "to spawn a child task for the appropriate specialist profile instead of " diff --git a/agent/transports/hermes_tools_mcp_server.py b/agent/transports/hermes_tools_mcp_server.py index e5bfe6f8f7..5595c48d84 100644 --- a/agent/transports/hermes_tools_mcp_server.py +++ b/agent/transports/hermes_tools_mcp_server.py @@ -136,6 +136,7 @@ EXPOSED_TOOLS: tuple[str, ...] = ( "kanban_complete", "kanban_block", "kanban_request_review", + "kanban_request_changes", "kanban_comment", "kanban_heartbeat", "kanban_show", diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 32c75929cb..d7df990a0d 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -214,6 +214,16 @@ model: # worktree_sync: true # Default — branch from the fetched remote tip # worktree_sync: false # Branch from local HEAD (offline / pinned base) +# ============================================================================= +# Kanban Review Dispatch +# ============================================================================= +# First-class review tasks are dispatched automatically by default. The worker +# is spawned with the bundled sdlc-review skill and can approve, request changes +# back to the original implementer, or escalate a genuine external blocker. +# Disable this only when every review is performed manually from the dashboard. +kanban: + review_dispatch: true + # ============================================================================= # Terminal Tool Configuration # ============================================================================= diff --git a/cli.py b/cli.py index 0ad7c2ed8a..65bb56ad04 100644 --- a/cli.py +++ b/cli.py @@ -18294,7 +18294,17 @@ def _run_kanban_goal_loop_q(cli: "HermesCLI", first_response: str) -> None: c = _kb.connect() try: t = _kb.get_task(c, task_id) - return t.status if t is not None else None + if t is None: + return None + # ``request_changes`` deliberately lands back in ready/todo. For a + # goal-mode reviewer that is nevertheless a terminal outcome for + # this process; otherwise the review agent would keep consuming + # implementation turns after it released its claim. + if t.status in {"ready", "todo"}: + events = _kb.list_events(c, task_id) + if events and events[-1].kind == "changes_requested": + return "changes_requested" + return t.status finally: try: c.close() diff --git a/gateway/kanban_watchers.py b/gateway/kanban_watchers.py index c09f20475b..1a34feb12b 100644 --- a/gateway/kanban_watchers.py +++ b/gateway/kanban_watchers.py @@ -487,8 +487,8 @@ class GatewayKanbanWatchersMixin: 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. + # Implementation complete; task moved to the + # first-class review lane. Wake the origin thread. handoff = "" if ev.payload and ev.payload.get("summary"): handoff = f"\n{str(ev.payload['summary'])[:200]}" diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index e1bda5ebe1..c32d5950d3 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -2341,6 +2341,10 @@ DEFAULT_CONFIG = { # only if you run the dispatcher as a separate systemd unit or # don't want the gateway to spawn workers. "dispatch_in_gateway": True, + # Automatically claim tasks in the first-class review column and spawn + # the assigned profile with the bundled sdlc-review skill. Disable for + # boards where every review is performed manually from the dashboard. + "review_dispatch": True, # Seconds between dispatcher ticks (idle or not). Lower = snappier # pickup of newly-ready tasks; higher = less SQL pressure. "dispatch_interval_seconds": 60, diff --git a/hermes_cli/goals.py b/hermes_cli/goals.py index b0d0258f83..891c3a66bb 100644 --- a/hermes_cli/goals.py +++ b/hermes_cli/goals.py @@ -1976,7 +1976,7 @@ KANBAN_GOAL_CONTINUATION_TEMPLATE = ( "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 it is a " - "code change that needs human review before counting as done, call " + "code change that needs same-card 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." @@ -1989,7 +1989,7 @@ 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 it is a code change awaiting human review, " + "summary of what you did. If it is a code change awaiting same-card review, " "call kanban_request_review with that summary instead. If something still " "blocks completion, call kanban_block with the reason instead." ) @@ -2031,7 +2031,8 @@ def run_kanban_goal_loop( Returns a decision dict: ``{"outcome", "turns_used", "reason"}`` where outcome is one of ``"completed_by_worker"``, ``"review_requested_by_worker"``, - ``"blocked_budget"``, ``"blocked_by_worker"``, or ``"stopped"``. + ``"changes_requested_by_reviewer"``, ``"blocked_budget"``, + ``"blocked_by_worker"``, or ``"stopped"``. """ def _log(msg: str) -> None: @@ -2067,9 +2068,12 @@ def run_kanban_goal_loop( 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. + # is awaiting a reviewer. 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 == "changes_requested": + _log(f"kanban goal loop: reviewer returned task {task_id} for changes after {turns_used} turn(s)") + return {"outcome": "changes_requested_by_reviewer", "turns_used": turns_used, "reason": "reviewer requested changes"} 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 f8c121e7e0..128c5ae990 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -666,7 +666,20 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu ) p_request_review.add_argument( "--reviewer", default=None, - help="Optional profile/handle to attribute the review to (informational only).", + help="Optional reviewer profile; reassigns the task before review dispatch.", + ) + p_request_review.add_argument( + "--metadata", default=None, + help="JSON object with structured reviewer handoff facts.", + ) + + p_request_changes = sub.add_parser( + "request-changes", + help="Reviewer verdict: return the active review run to its implementer", + ) + p_request_changes.add_argument("task_id") + p_request_changes.add_argument( + "reason", nargs="+", help="Concrete changes required before re-review", ) p_reopen_review = sub.add_parser( @@ -1091,6 +1104,7 @@ def kanban_command(args: argparse.Namespace) -> int: "schedule": _cmd_schedule, "unblock": _cmd_unblock, "request-review": _cmd_request_review, + "request-changes": _cmd_request_changes, "reopen-review": _cmd_reopen_review, "promote": _cmd_promote, "archive": _cmd_archive, @@ -1741,7 +1755,9 @@ def _cmd_show(args: argparse.Namespace) -> int: # of show output so CLI users see them before scrolling through # comments / runs. from hermes_cli import kanban_diagnostics as kd - diags = kd.compute_task_diagnostics(task, events, runs) + diags = kd.compute_task_diagnostics( + task, events, runs, graph=kb.task_graph_context(conn, task.id) + ) if diags: sev_marker = {"warning": "⚠", "error": "!!", "critical": "!!!"} print(f"\n Diagnostics ({len(diags)}):") @@ -1909,6 +1925,7 @@ def _cmd_diagnostics(args: argparse.Namespace) -> int: task, kb.list_events(conn, args.task), kb.list_runs(conn, args.task), + graph=kb.task_graph_context(conn, args.task), config=diag_config, ) } @@ -1934,6 +1951,7 @@ def _cmd_diagnostics(args: argparse.Namespace) -> int: tuple(ids), ): run_by.setdefault(row["task_id"], []).append(row) + graph_by = kb.task_graph_contexts(conn, ids) diags_by_task = {} for r in rows: tid = r["id"] @@ -1941,6 +1959,7 @@ def _cmd_diagnostics(args: argparse.Namespace) -> int: r, ev_by.get(tid, []), run_by.get(tid, []), + graph=graph_by.get(tid), config=diag_config, ) if dl: @@ -2164,6 +2183,39 @@ def _worker_run_id_for(task_id: str) -> Optional[int]: return None +def _goal_mode_handoff_rejection(task: Optional[kb.Task], evidence: str) -> Optional[str]: + """Apply the goal judge to every terminal worker handoff, including review.""" + if task is None or not task.goal_mode: + return None + try: + from agent.auxiliary_client import get_text_auxiliary_client + + client, model = get_text_auxiliary_client("goal_judge") + except Exception: + return None + if client is None or not model: + return None + + from hermes_cli.goals import judge_goal + + verdict = "done" + reason = "" + try: + verdict, reason, _, _, _ = judge_goal( + goal=f"{task.title}\n\n{task.body or ''}".strip(), + last_response=evidence.strip(), + ) + except Exception as judge_exc: + import logging as _logging + + _logging.getLogger(__name__).warning( + "goal judge check failed, allowing lifecycle handoff: %s", + judge_exc, + exc_info=True, + ) + return reason if verdict != "done" else None + + def _cmd_complete(args: argparse.Namespace) -> int: """Mark one or more tasks done. Supports a single id or a list.""" ids = list(args.task_ids or []) @@ -2195,49 +2247,22 @@ def _cmd_complete(args: argparse.Namespace) -> int: failed: list[str] = [] with kb.connect_closing() as conn: for tid in ids: - # Goal-mode pre-completion judge gate (mirrors the gate in - # tools/kanban_tools.py:_handle_complete — Issue #38367). - # Without this, a goal_mode worker can call - # `hermes kanban complete ` from the terminal tool and - # bypass the auxiliary judge that the tool-call path enforces. + # Goal-mode judge gate (mirrors tools/kanban_tools.py). Apply it + # to every terminal handoff so request-review cannot bypass the + # acceptance contract that protects complete. task = kb.get_task(conn, tid) - if task and task.goal_mode: - judge_available = False - try: - from agent.auxiliary_client import get_text_auxiliary_client - _client, _model = get_text_auxiliary_client("goal_judge") - judge_available = _client is not None and bool(_model) - except Exception: - pass - if judge_available: - from hermes_cli.goals import judge_goal - verdict = "done" - reason = "" - try: - # judge_goal returns (verdict, reason, parse_failed, - # wait_directive, transport_failed) — see - # hermes_cli/goals.py. Unpacking fewer raises - # ValueError into the fail-open handler below, - # silently disabling the gate. - verdict, reason, _, _, _ = judge_goal( - goal=f"{task.title}\n\n{task.body or ''}".strip(), - last_response=(summary or args.result or "").strip(), - ) - except Exception as judge_exc: - import logging as _logging - _logging.getLogger(__name__).warning( - "goal judge check failed, allowing completion: %s", - judge_exc, - exc_info=True, - ) - if verdict != "done": - print( - f"kanban: goal completion of {tid} rejected by judge: {reason}. " - f"Provide evidence matching the task's acceptance criteria.", - file=sys.stderr, - ) - failed.append(tid) - continue + rejection = _goal_mode_handoff_rejection( + task, + (summary or args.result or "").strip(), + ) + if rejection is not None: + print( + f"kanban: goal completion of {tid} rejected by judge: {rejection}. " + f"Provide evidence matching the task's acceptance criteria.", + file=sys.stderr, + ) + failed.append(tid) + continue if not kb.complete_task( conn, tid, @@ -2367,10 +2392,35 @@ def _cmd_request_review(args: argparse.Namespace) -> int: summary = getattr(args, "summary", None) if summary is not None: summary = summary.strip() or None + raw_metadata = getattr(args, "metadata", None) + metadata = None + if raw_metadata: + try: + metadata = json.loads(raw_metadata) + if not isinstance(metadata, dict): + raise ValueError("must be a JSON object") + except (ValueError, json.JSONDecodeError) as exc: + print(f"kanban: --metadata: {exc}", file=sys.stderr) + return 2 reviewer = getattr(args, "reviewer", None) with kb.connect_closing() as conn: + rejection = _goal_mode_handoff_rejection( + kb.get_task(conn, tid), + summary or "", + ) + if rejection is not None: + print( + f"kanban: goal review handoff of {tid} rejected by judge: " + f"{rejection}. Provide acceptance evidence matching the task.", + file=sys.stderr, + ) + return 1 if not kb.request_review( - conn, tid, summary=summary, reviewer=reviewer, + conn, + tid, + summary=summary, + metadata=metadata, + reviewer=reviewer, expected_run_id=_worker_run_id_for(tid), ): print( @@ -2382,6 +2432,29 @@ def _cmd_request_review(args: argparse.Namespace) -> int: return 0 +def _cmd_request_changes(args: argparse.Namespace) -> int: + tid = args.task_id + reason = " ".join(args.reason).strip() + with kb.connect_closing() as conn: + ok, detail = kb.request_changes( + conn, + tid, + reason=reason, + expected_run_id=_worker_run_id_for(tid), + ) + if not ok: + print( + f"cannot request changes for {tid}: {detail or 'invalid review state'}", + file=sys.stderr, + ) + return 1 + print( + f"Requested changes for {tid}" + + (f"; routed to {detail}" if detail else "") + ) + return 0 + + def _cmd_reopen_review(args: argparse.Namespace) -> int: ids = list(args.task_ids or []) if not ids: @@ -2395,7 +2468,12 @@ def _cmd_reopen_review(args: argparse.Namespace) -> int: with kb.connect_closing() as conn: for tid in ids: if reason: - kb.add_comment(conn, tid, author, f"CHANGES REQUESTED: {reason}") + kb.add_comment( + conn, + tid, + author or "operator", + 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) @@ -3213,7 +3291,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 + `request-review ` Enter first-class review; `request-changes ` returns an active review to its implementer `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 ab491c8565..1bb08f5e68 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -3616,6 +3616,49 @@ def child_ids(conn: sqlite3.Connection, task_id: str) -> list[str]: return [r["child_id"] for r in rows] +def task_graph_contexts( + conn: sqlite3.Connection, task_ids: Iterable[str] +) -> dict[str, dict]: + """Bulk-load compact direct graph state for graph-aware diagnostics.""" + ordered_ids = list(dict.fromkeys(str(task_id) for task_id in task_ids if task_id)) + contexts = { + task_id: {"parents": [], "children": []} + for task_id in ordered_ids + } + if not ordered_ids: + return contexts + + placeholders = ",".join("?" for _ in ordered_ids) + for row in conn.execute( + "SELECT l.child_id AS owner_id, t.id, t.title, t.status " + "FROM task_links l JOIN tasks t ON t.id = l.parent_id " + f"WHERE l.child_id IN ({placeholders}) ORDER BY l.child_id, t.id", + tuple(ordered_ids), + ).fetchall(): + contexts[row["owner_id"]]["parents"].append({ + "id": row["id"], + "title": row["title"], + "status": row["status"], + }) + for row in conn.execute( + "SELECT l.parent_id AS owner_id, t.id, t.title, t.status " + "FROM task_links l JOIN tasks t ON t.id = l.child_id " + f"WHERE l.parent_id IN ({placeholders}) ORDER BY l.parent_id, t.id", + tuple(ordered_ids), + ).fetchall(): + contexts[row["owner_id"]]["children"].append({ + "id": row["id"], + "title": row["title"], + "status": row["status"], + }) + return contexts + + +def task_graph_context(conn: sqlite3.Connection, task_id: str) -> dict: + """Return compact direct parent/child state for one task.""" + return task_graph_contexts(conn, [task_id])[task_id] + + def parent_results(conn: sqlite3.Connection, task_id: str) -> list[tuple[str, Optional[str]]]: """Return ``(parent_id, result)`` for every done parent of ``task_id``.""" rows = conn.execute( @@ -4175,7 +4218,7 @@ def recompute_ready( task_id = row["id"] cur_status = row["status"] if cur_status == "blocked" and _has_sticky_block(conn, task_id): - # Worker / operator asked for human review — do not + # Worker / operator asked for explicit human intervention — do not # silently auto-recover. ``unblock_task`` is the only # legitimate exit (it emits ``"unblocked"`` which flips # this predicate back). @@ -5835,32 +5878,17 @@ def request_review( task_id: str, *, summary: Optional[str] = None, + metadata: Optional[dict] = None, reviewer: Optional[str] = None, expected_run_id: Optional[int] = None, ) -> bool: - """Transition ``running``/``ready`` → ``review`` (implementation done, awaiting review). + """Transition implementation work into the first-class review phase. - 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). + Unlike :func:`block_task`, this transition never touches block recurrence + accounting. The current implementer and optional reviewer are recorded on + the event so an autonomous reviewer can route requested changes back to the + right profile. Supplying ``reviewer`` also reassigns the task before it is + exposed to the review dispatcher. """ with write_txn(conn): trow = conn.execute( @@ -5869,56 +5897,58 @@ def request_review( if trow is None: return False implementer = trow["assignee"] + reviewer = _canonical_assignee(reviewer) if reviewer is not None else None + assignee_sql = ", assignee = ?" if reviewer is not None else "" + params: tuple[Any, ...] 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,), - ) + params = (reviewer, task_id) if reviewer is not None else (task_id,) + run_guard = "" 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)), + params = ( + (reviewer, task_id, int(expected_run_id)) + if reviewer is not None + else (task_id, int(expected_run_id)) ) + run_guard = " AND current_run_id = ?" + cur = conn.execute( + """ + UPDATE tasks + SET status = 'review', + claim_lock = NULL, + claim_expires = NULL, + worker_pid = NULL + """ + assignee_sql + """ + WHERE id = ? + AND status IN ('running', 'ready') + """ + run_guard, + params, + ) if cur.rowcount != 1: return False run_id = _end_run( - conn, task_id, - outcome="review_requested", status="review", + conn, + task_id, + outcome="review_requested", + status="review", summary=summary, + metadata=metadata, ) - # 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: + if run_id is None and (summary or metadata): run_id = _synthesize_ended_run( - conn, task_id, + conn, + task_id, outcome="review_requested", summary=summary, + metadata=metadata, ) - # 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 "" + lines = (summary or "").strip().splitlines() + event_summary = lines[0][:400] if lines else "" _append_event( - conn, task_id, "review_requested", + conn, + task_id, + "review_requested", { - "summary": ev_summary or None, + "summary": event_summary or None, "implementer": implementer, "reviewer": reviewer, }, @@ -5927,6 +5957,113 @@ def request_review( return True +def request_changes( + conn: sqlite3.Connection, + task_id: str, + *, + reason: str, + expected_run_id: Optional[int] = None, +) -> tuple[bool, Optional[str]]: + """Finish an active review run and route the task back for rework. + + The transition is valid only for a run claimed from ``review``. It closes + that reviewer run, restores the implementer recorded by the latest + ``review_requested`` event, reapplies parent gating, and emits an auditable + ``changes_requested`` event. The second tuple item is the implementer on + success or a diagnostic reason on failure. + """ + reason = (reason or "").strip() + if not reason: + return False, "reason is required" + + with write_txn(conn): + task_row = conn.execute( + "SELECT status, assignee, current_run_id FROM tasks WHERE id = ?", + (task_id,), + ).fetchone() + if task_row is None: + return False, "task not found" + current_run_id = task_row["current_run_id"] + if task_row["status"] != "running" or current_run_id is None: + return False, "task is not in an active review run" + if expected_run_id is not None and int(current_run_id) != int(expected_run_id): + return False, "run_id mismatch" + + claimed_event = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND run_id = ? AND kind = 'claimed' " + "ORDER BY id DESC LIMIT 1", + (task_id, int(current_run_id)), + ).fetchone() + try: + claimed_payload = ( + json.loads(claimed_event["payload"]) + if claimed_event and claimed_event["payload"] + else {} + ) + except (json.JSONDecodeError, TypeError): + claimed_payload = {} + if claimed_payload.get("source_status") != "review": + return False, "active run was not claimed from review" + + requested_event = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND kind = 'review_requested' " + "ORDER BY id DESC LIMIT 1", + (task_id,), + ).fetchone() + if requested_event is None: + return False, "no prior review_requested event" + try: + requested_payload = ( + json.loads(requested_event["payload"]) + if requested_event["payload"] + else {} + ) + except (json.JSONDecodeError, TypeError): + requested_payload = {} + implementer = requested_payload.get("implementer") + if not isinstance(implementer, str) or not implementer.strip(): + return False, "review handoff has no valid implementer provenance" + + new_status = _landing_status_after_parents(conn, task_id) + cur = conn.execute( + """ + UPDATE tasks + SET status = ?, + assignee = COALESCE(?, assignee), + claim_lock = NULL, + claim_expires = NULL, + worker_pid = NULL, + consecutive_failures = 0, + last_failure_error = NULL + WHERE id = ? AND status = 'running' AND current_run_id = ? + """, + (new_status, implementer, task_id, int(current_run_id)), + ) + if cur.rowcount != 1: + return False, "task changed during review handoff" + run_id = _end_run( + conn, + task_id, + outcome="changes_requested", + status=new_status, + summary=reason, + ) + _append_event( + conn, + task_id, + "changes_requested", + { + "reason": reason, + "implementer": implementer, + "status": new_status, + }, + run_id=run_id, + ) + return True, implementer + + def promote_task( conn: sqlite3.Connection, task_id: str, @@ -6039,7 +6176,8 @@ def _landing_status_after_parents(conn: sqlite3.Connection, task_id: str) -> str 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", + "WHERE l.child_id = ? " + "AND p.status NOT IN ('done', 'archived') LIMIT 1", (task_id,), ).fetchone() return "todo" if undone_parents else "ready" @@ -8298,8 +8436,8 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str] ``"recent_success"`` A completed run exists within ``_RESPAWN_GUARD_SUCCESS_WINDOW`` - seconds. Useful work already succeeded for this task; wait for - human review rather than immediately re-spawning. Bypassed when an + seconds. Useful work already succeeded for this task; wait for an + explicit re-queue rather than immediately re-spawning. Bypassed when an explicit re-queue event (status change, promote, unblock, reclaim) arrives AFTER that completion — that's a deliberate re-run request. @@ -8458,23 +8596,19 @@ def has_spawnable_review(conn: sqlite3.Connection) -> bool: def review_dispatch_enabled() -> bool: - """Whether the dispatcher should auto-claim ``review`` tasks and spawn a - reviewer agent (``kanban.review_dispatch``, default **False**). + """Return whether first-class review tasks should dispatch automatically. - 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. + The default is true because Hermes ships the ``sdlc-review`` skill and the + review lifecycle includes a supported reviewer-owned changes-requested + transition. Operators can disable it for human-only review boards. """ try: from hermes_cli.config import load_config return bool( - (load_config() or {}).get("kanban", {}).get("review_dispatch", False) + (load_config() or {}).get("kanban", {}).get("review_dispatch", True) ) except Exception: - return False + return True def dispatch_once( @@ -8884,13 +9018,10 @@ 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. - # 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. + # Auto-dispatch is enabled by default because Hermes bundles the + # ``sdlc-review`` skill and reviewer workers can now approve, request + # changes without block-loop accounting, or escalate a genuine blocker. + # Human-only boards can disable it with ``kanban.review_dispatch``. review_rows = [] if review_dispatch_enabled(): review_rows = conn.execute( diff --git a/hermes_cli/kanban_diagnostics.py b/hermes_cli/kanban_diagnostics.py index 8173f1e333..1a0309aa01 100644 --- a/hermes_cli/kanban_diagnostics.py +++ b/hermes_cli/kanban_diagnostics.py @@ -10,8 +10,8 @@ stuck blocked for too long, etc. Each one carries: * A list of **suggested actions** — structured entries the dashboard turns into buttons and the CLI turns into hints. -Rules run over (task, recent events, recent runs) and emit diagnostics. -They are stateless and read-only — no DB writes. Callers compute +Rules run over (task, recent events, recent runs, optional graph context) and +emit diagnostics. They are stateless and read-only — no DB writes. Callers compute diagnostics on demand (on ``/board`` load, ``/tasks/:id`` fetch, or ``hermes kanban diagnostics``). @@ -750,6 +750,84 @@ def _rule_repeated_crashes(task, events, runs, now, cfg) -> list[Diagnostic]: )] +def _rule_review_dependency_deadlock(task, events, runs, now, cfg) -> list[Diagnostic]: + """Detect a legacy review handoff that starves downstream children. + + Older workers were instructed to sticky-block an implementation with a + ``review-required:`` reason. A separately modelled reviewer child cannot + promote until that parent is terminal, so the lane has no autonomous next + step. This compatibility diagnostic is graph-aware but deliberately leaves + both the dependency graph and the user's sticky block unchanged. + """ + if _task_field(task, "status") != "blocked": + return [] + + latest_block = None + for event in events: + if _event_kind(event) == "blocked": + latest_block = event + if latest_block is None: + return [] + reason = str(_parse_payload(latest_block).get("reason") or "").strip() + if not reason.lower().startswith("review-required:"): + return [] + + graph = cfg.get("_graph") + if not isinstance(graph, dict): + return [] + waiting_children = [ + child + for child in (graph.get("children") or []) + if isinstance(child, dict) and child.get("status") == "todo" + ] + if not waiting_children: + return [] + + task_id = str(_task_field(task, "id") or "") + child_ids = [ + str(child.get("id")) + for child in waiting_children + if child.get("id") + ] + actions: list[DiagnosticAction] = [] + if task_id: + actions.append(DiagnosticAction( + kind="cli_hint", + label="Complete the finished implementation phase", + payload={"command": f"hermes kanban complete {task_id}"}, + suggested=True, + )) + if task_id and child_ids: + actions.append(DiagnosticAction( + kind="cli_hint", + label="Or unlink the incorrectly gated reviewer", + payload={"command": f"hermes kanban unlink {task_id} {child_ids[0]}"}, + )) + + blocked_at = _event_ts(latest_block) or now + return [Diagnostic( + kind="review_dependency_deadlock", + severity="error", + title=f"Review handoff blocks {len(child_ids)} dependent task(s)", + detail=( + "This implementation is sticky-blocked for review while its " + "downstream task(s) require the implementation to be done or " + "archived before they can run. Complete the finished phase, unlink " + "the incorrect dependency, or migrate this workflow to the " + "first-class review lifecycle." + ), + actions=actions, + first_seen_at=blocked_at, + last_seen_at=blocked_at, + count=len(child_ids), + data={ + "blocked_parent_id": task_id, + "waiting_child_ids": child_ids, + "block_reason": reason, + }, + )] + + def _rule_stuck_in_blocked(task, events, runs, now, cfg) -> list[Diagnostic]: """Task has been in ``blocked`` status for too long without a comment. @@ -1008,6 +1086,7 @@ _RULES: list[RuleFn] = [ _rule_prose_phantom_refs, _rule_repeated_failures, _rule_repeated_crashes, + _rule_review_dependency_deadlock, _rule_stuck_in_blocked, _rule_block_unblock_cycling, _rule_stranded_in_ready, @@ -1022,6 +1101,7 @@ DIAGNOSTIC_KINDS = ( "prose_phantom_refs", "repeated_failures", "repeated_crashes", + "review_dependency_deadlock", "stuck_in_blocked", "block_unblock_cycling", "stranded_in_ready", @@ -1095,6 +1175,7 @@ def compute_task_diagnostics( *, now: Optional[int] = None, config: Optional[dict] = None, + graph: Optional[dict] = None, ) -> list[Diagnostic]: """Run every rule against a single task's state and return a severity-sorted list of active diagnostics. @@ -1105,6 +1186,8 @@ def compute_task_diagnostics( now_ts = int(now if now is not None else time.time()) config = config or {} cfg = {**DEFAULT_CONFIG, **config} + if graph is not None: + cfg["_graph"] = graph if ( "failure_threshold" not in config and "spawn_failure_threshold" not in config diff --git a/plugins/kanban/dashboard/dist/index.js b/plugins/kanban/dashboard/dist/index.js index 126aa8d180..f15850f734 100644 --- a/plugins/kanban/dashboard/dist/index.js +++ b/plugins/kanban/dashboard/dist/index.js @@ -107,7 +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", + review: "Implementation complete — awaiting review", done: "Completed", archived: "Archived", }; diff --git a/plugins/kanban/dashboard/dist/style.css b/plugins/kanban/dashboard/dist/style.css index 590a8ca314..fe0fd72a26 100644 --- a/plugins/kanban/dashboard/dist/style.css +++ b/plugins/kanban/dashboard/dist/style.css @@ -175,7 +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-review { background: #48b0c4; } /* cyan — awaiting 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 e8c08bf610..465a0b7f8c 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -299,6 +299,7 @@ def _compute_task_diagnostics( ).fetchall(): runs_by_task.setdefault(run_row["task_id"], []).append(run_row) + graph_by_task = kanban_db.task_graph_contexts(conn, row_ids) out: dict[str, list[dict]] = {} for r in rows: tid = r["id"] @@ -307,6 +308,7 @@ def _compute_task_diagnostics( events_by_task.get(tid, []), runs_by_task.get(tid, []), config=diag_config, + graph=graph_by_task.get(tid), ) if diags: out[tid] = [d.to_dict() for d in diags] @@ -900,12 +902,13 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu 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 + # Manual "request review" from the board. 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, + metadata=payload.metadata, ) elif s == "ready": # Re-open a blocked/scheduled/review task, or just an explicit @@ -1295,6 +1298,7 @@ def bulk_update(payload: BulkTaskBody, board: Optional[str] = Query(None)): # Non-block review handoff (mirror of PATCH /tasks/{id}). ok = kanban_db.request_review( conn, tid, summary=payload.summary, + metadata=payload.metadata, ) elif s == "ready": cur = kanban_db.get_task(conn, tid) diff --git a/skills/devops/sdlc-review/SKILL.md b/skills/devops/sdlc-review/SKILL.md new file mode 100644 index 0000000000..fddb61212c --- /dev/null +++ b/skills/devops/sdlc-review/SKILL.md @@ -0,0 +1,141 @@ +--- +name: sdlc-review +description: >- + Review Kanban tasks spawned from the review lane. Verify the implementer + handoff and choose approve, request changes, or escalate. +tags: + - kanban + - review + - quality + - verification +environments: + - kanban +--- + +# Kanban Review Skill + +You have been spawned as a **reviewer** for a Kanban task that the implementer +submitted for review. Your job is to independently verify the work and reach a +verdict: **approve**, **request changes**, or **escalate**. + +## How you got here + +1. An implementer agent finished its work and called + ``kanban_request_review(summary=..., metadata=...)`` instead of + ``kanban_complete``. +2. The task transitioned ``running → review``. +3. The dispatcher claimed it and spawned you (with this skill loaded). + +## Orientation + +1. **Call ``kanban_show()`` first.** The response includes: + - The task title and body (the original spec / acceptance criteria). + - The implementer's handoff ``summary`` and ``metadata`` from the + ``review_requested`` event — what they claim to have done. + - The comment thread (may contain design decisions, constraints). + - Prior runs (attempt history — useful if this is a re-review). + +2. **Understand what was asked vs what was done.** Read the acceptance criteria + in the task body. Read the implementer's summary. Note any gaps between the + two before you start verifying. + +## Verification + +### For code changes + +1. **Review the diff.** If the implementer provided a ``diff_path`` in metadata, + read it. Otherwise, find the changed files (``metadata.changed_files``) and + read them in the workspace. Check: + - Does the code do what the acceptance criteria require? + - Are there obvious bugs, edge cases, or error paths not handled? + - Does the code follow existing conventions in the file/project? + - Are there unused imports, dead code, or leftover debug statements? + +2. **Run the tests / linter** if available: + - ``flutter analyze``, ``pytest``, ``ruff``, ``eslint``, etc. + - If tests were listed as passing in metadata, spot-check by running them. + - If no tests exist, verify the change manually by reading the logic. + +3. **Check for scope creep.** Did the implementer change files outside the + task's scope? Flag unrelated changes in your review comment. + +### For non-code work + +1. Verify the deliverable matches what the task body asked for. +2. Check data quality, formatting, completeness. +3. Validate any URLs, references, or external links the work depends on. + +## Verdict + +Choose **one** of these three outcomes: + +### ✅ Approve → task complete + +The work meets all acceptance criteria. Call: + +``` +kanban_complete( + summary="Reviewed and approved. <1-2 sentences on what was verified>", + metadata={"review_outcome": "approved", "reviewer_checks": [...]} +) +``` + +This transitions ``review → done``. The task is complete. + +### ❌ Request changes → back to implementer + +The work needs fixes before it can be approved. Write a detailed comment +explaining exactly what needs to change, then call: + +``` +kanban_comment( + task_id="", + body="Changes requested:\n1. \n2. ", +) +kanban_request_changes(reason="Changes requested: ") +``` + +This transitions ``running → ready``, reassigns the task back to the +original implementer (looked up from the review event), and lets the +dispatcher respawn them automatically. No human intervention needed — +the loop closes itself. When the implementer re-submits for review, +you'll be spawned again to re-review. + +**Be specific** in your change requests. Don't write "the code needs work" — +write "the `_AlnavBookmarkTile` widget doesn't handle null `message.subject`, +add a fallback like the NAVADMIN tile has at line 45." + +### ⚠️ Escalate → human needed + +The task has a fundamental problem that can't be fixed by the implementer +alone (wrong approach, missing requirements, ambiguous spec). Block with: + +``` +kanban_block(reason="escalation: ") +``` + +## Pitfalls + +- **Don't rubber-stamp.** Actually read the code / deliverable. The whole + point of a review phase is independent verification, not a second pair of + eyes that glances and approves. + +- **Don't fix the code yourself.** If you find bugs, request changes — don't + edit the files. Your job is verification, not implementation. The + implementer fixes; you verify the fix. + +- **Don't complete without checking acceptance criteria.** Read the task body. + If the spec says "3 things" and the summary says "done", verify all 3. + +- **Don't block for style nits.** If the code is correct and follows + conventions, approve. Save "request changes" for things that would break or + mislead — wrong logic, missing error handling, incomplete acceptance criteria. + +## What a good review looks like + +**Bad:** "Looks good, approved." + +**Good:** "Reviewed the diff — `_AlnavBookmarkTile` correctly mirrors the +NAVADMIN pattern with Dismissible, proper `removeBookmark(id, 'alnav')` call, +and navigation to `AlnavDetailScreen`. Empty state text updated. Ran +`flutter analyze` — 0 issues. All 6 acceptance criteria verified." \ No newline at end of file diff --git a/tests/hermes_cli/test_kanban_notify.py b/tests/hermes_cli/test_kanban_notify.py index 69ff262b4f..edab38a350 100644 --- a/tests/hermes_cli/test_kanban_notify.py +++ b/tests/hermes_cli/test_kanban_notify.py @@ -64,6 +64,60 @@ def _assert_inherited_notify_sub(subs: list[dict]) -> None: +@pytest.mark.asyncio +async def test_notifier_wakes_origin_for_review_and_keeps_subscription(kanban_home): + from gateway.config import Platform + from gateway.run import GatewayRunner + + with kb.connect() as conn: + task_id = kb.create_task(conn, title="review handoff", assignee="builder") + kb.add_notify_sub( + conn, + task_id=task_id, + platform="telegram", + chat_id="chat1", + ) + task = kb.claim_task(conn, task_id, claimer="builder:1") + assert task is not None + assert kb.request_review( + conn, + task_id, + summary="Implementation and tests ready.", + reviewer="reviewer", + expected_run_id=task.current_run_id, + ) + + runner = object.__new__(GatewayRunner) + runner._running = True + runner._kanban_sub_fail_counts = {} + delivered: list[str] = [] + + async def _send(chat_id, message, metadata=None): + delivered.append(message) + runner._running = False + + adapter = MagicMock() + adapter.name = "telegram" + adapter.send = AsyncMock(side_effect=_send) + runner.adapters = {Platform.TELEGRAM: adapter} + + real_sleep = asyncio.sleep + + async def _fast_sleep(_seconds): + await real_sleep(0) + + with patch("gateway.run.asyncio.sleep", side_effect=_fast_sleep): + await asyncio.wait_for( + runner._kanban_notifier_watcher(interval=1), + timeout=10.0, + ) + + assert any("ready for review" in message for message in delivered) + assert any("Implementation and tests ready" in message for message in delivered) + with kb.connect() as conn: + assert kb.list_notify_subs(conn), "review is non-final; subscription must survive" + + @pytest.mark.asyncio async def test_gateway_create_autosubscribes_on_explicit_board(kanban_home): """`/kanban --board create ...` must subscribe on that board. diff --git a/tests/hermes_cli/test_kanban_review_lifecycle.py b/tests/hermes_cli/test_kanban_review_lifecycle.py index 441f313245..47cdaf8f43 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle.py @@ -423,20 +423,18 @@ def test_request_review_on_unclaimed_ready_synthesizes_run(kanban_home: Path) -> 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.""" +def test_reviewer_reassigns_for_autonomous_dispatch(kanban_home: Path) -> None: + """An explicit reviewer routes the review run while preserving implementer provenance.""" with kb.connect() as conn: - tid = kb.create_task(conn, title="keep assignee", assignee="worker") - kb.claim_task(conn, tid) + tid = kb.create_task(conn, title="route reviewer", assignee="worker") + claimed = kb.claim_task(conn, tid) + assert claimed is not None ok = kb.request_review( conn, tid, summary="v1", reviewer="lead-reviewer", - expected_run_id=kb.get_task(conn, tid).current_run_id, + expected_run_id=claimed.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. + assert kb.get_task(conn, tid).assignee == "lead-reviewer" ev = _events(conn, tid, kind="review_requested")[0][1] assert ev["reviewer"] == "lead-reviewer" assert ev["implementer"] == "worker" diff --git a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py new file mode 100644 index 0000000000..97cf76b7c9 --- /dev/null +++ b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py @@ -0,0 +1,260 @@ +"""End-to-end regressions for the Kanban review lifecycle. + +These tests cover the two review models that must coexist: + +* first-class same-card review, including an autonomous reviewer requesting + changes and routing the task back to the original implementer; and +* legacy downstream review cards, where a sticky ``review-required`` parent + can silently starve its reviewer child and therefore needs an immediate, + graph-aware diagnostic. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb +from hermes_cli import kanban_diagnostics as kd + + +@pytest.fixture +def conn(tmp_path: Path): + db = kb.connect(tmp_path / "kanban.db") + try: + yield db + finally: + db.close() + + +def _event(events, kind: str): + return [event for event in events if event.kind == kind][-1] + + +def _run(runs, outcome: str): + return [run for run in runs if run.outcome == outcome][-1] + + +def test_same_card_review_supports_changes_and_approval_without_block_loop(conn): + task_id = kb.create_task(conn, title="Implement guarded export", assignee="builder") + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Implementation and focused tests are ready.", + metadata={"commit": "abc123"}, + expected_run_id=implementation.current_run_id, + ) + + awaiting_review = kb.get_task(conn, task_id) + assert awaiting_review is not None + assert awaiting_review.status == "review" + assert awaiting_review.assignee == "reviewer" + assert awaiting_review.current_run_id is None + + first_events = kb.list_events(conn, task_id) + requested = _event(first_events, "review_requested") + assert requested.payload["implementer"] == "builder" + assert requested.payload["reviewer"] == "reviewer" + assert requested.payload["summary"] == "Implementation and focused tests are ready." + implementation_run = _run(kb.list_runs(conn, task_id), "review_requested") + assert implementation_run.summary == "Implementation and focused tests are ready." + assert implementation_run.metadata == {"commit": "abc123"} + + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + assert kb.request_changes( + conn, + task_id, + reason="Add a regression for the fallback branch.", + expected_run_id=review.current_run_id, + ) == (True, "builder") + + rework = kb.get_task(conn, task_id) + assert rework is not None + assert rework.status == "ready" + assert rework.assignee == "builder" + assert rework.current_run_id is None + changes = _event(kb.list_events(conn, task_id), "changes_requested") + assert changes.payload["reason"] == "Add a regression for the fallback branch." + assert changes.payload["implementer"] == "builder" + _run(kb.list_runs(conn, task_id), "changes_requested") + + implementation_2 = kb.claim_task(conn, task_id, claimer="builder:2") + assert implementation_2 is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Fallback regression added.", + expected_run_id=implementation_2.current_run_id, + ) + review_2 = kb.claim_review_task(conn, task_id, claimer="reviewer:2") + assert review_2 is not None + assert kb.complete_task( + conn, + task_id, + summary="Approved after independent verification.", + expected_run_id=review_2.current_run_id, + ) + + completed = kb.get_task(conn, task_id) + assert completed is not None + assert completed.status == "done" + assert completed.block_recurrences == 0 + + +def test_review_changes_reapply_parent_gate(conn): + parent_id = kb.create_task(conn, title="Upstream prerequisite", assignee="planner") + task_id = kb.create_task( + conn, + title="Dependent implementation", + assignee="builder", + parents=[parent_id], + ) + + # Move the task through review while its parent is temporarily terminal, + # then make the parent non-terminal again before changes are requested. + assert kb.complete_task(conn, parent_id) + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Ready for review.", + expected_run_id=implementation.current_run_id, + ) + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + conn.execute("UPDATE tasks SET status = 'ready' WHERE id = ?", (parent_id,)) + conn.commit() + + assert kb.request_changes( + conn, + task_id, + reason="Parent contract changed; rework after it lands.", + expected_run_id=review.current_run_id, + ) == (True, "builder") + regated = kb.get_task(conn, task_id) + assert regated is not None + assert regated.status == "todo" + + +def test_request_changes_fails_closed_on_malformed_review_provenance(conn): + task_id = kb.create_task(conn, title="Malformed handoff", assignee="builder") + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Ready.", + expected_run_id=implementation.current_run_id, + ) + conn.execute( + "UPDATE task_events SET payload = ? " + "WHERE task_id = ? AND kind = 'review_requested'", + ("{malformed-json", task_id), + ) + conn.commit() + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + + ok, detail = kb.request_changes( + conn, + task_id, + reason="Needs changes.", + expected_run_id=review.current_run_id, + ) + assert ok is False + assert "implementer provenance" in (detail or "") + task = kb.get_task(conn, task_id) + assert task is not None + assert task.status == "running" + assert task.assignee == "reviewer" + assert task.current_run_id == review.current_run_id + + +def test_legacy_review_child_deadlock_is_reported_immediately(conn): + implementation_id = kb.create_task( + conn, + title="Implement export", + assignee="builder", + ) + reviewer_id = kb.create_task( + conn, + title="Review export", + assignee="reviewer", + parents=[implementation_id], + ) + implementation = kb.claim_task(conn, implementation_id, claimer="builder:1") + assert implementation is not None + assert kb.block_task( + conn, + implementation_id, + reason="review-required: implementation ready for independent review", + expected_run_id=implementation.current_run_id, + ) + reviewer_task = kb.get_task(conn, reviewer_id) + assert reviewer_task is not None + assert reviewer_task.status == "todo" + assert kb.recompute_ready(conn) == 0 + + task = kb.get_task(conn, implementation_id) + diagnostics = kd.compute_task_diagnostics( + task, + kb.list_events(conn, implementation_id), + kb.list_runs(conn, implementation_id), + graph={ + "children": [ + { + "id": reviewer_id, + "title": "Review export", + "status": "todo", + } + ] + }, + ) + + deadlocks = [d for d in diagnostics if d.kind == "review_dependency_deadlock"] + assert len(deadlocks) == 1 + deadlock = deadlocks[0] + assert deadlock.severity == "error" + assert deadlock.data["blocked_parent_id"] == implementation_id + assert deadlock.data["waiting_child_ids"] == [reviewer_id] + assert any(action.kind == "cli_hint" for action in deadlock.actions) + + +def test_hard_block_with_waiting_child_is_not_mislabeled_as_review_deadlock(conn): + implementation_id = kb.create_task( + conn, title="Implement export", assignee="builder" + ) + child_id = kb.create_task( + conn, + title="Publish export", + assignee="release", + parents=[implementation_id], + ) + implementation = kb.claim_task(conn, implementation_id, claimer="builder:1") + assert implementation is not None + assert kb.block_task( + conn, + implementation_id, + reason="needs_input: production credentials unavailable", + expected_run_id=implementation.current_run_id, + ) + + diagnostics = kd.compute_task_diagnostics( + kb.get_task(conn, implementation_id), + kb.list_events(conn, implementation_id), + kb.list_runs(conn, implementation_id), + graph={ + "children": [{"id": child_id, "title": "Publish export", "status": "todo"}] + }, + ) + assert not any(d.kind == "review_dependency_deadlock" for d in diagnostics) diff --git a/tests/hermes_cli/test_kanban_review_surfaces.py b/tests/hermes_cli/test_kanban_review_surfaces.py new file mode 100644 index 0000000000..f9efc15069 --- /dev/null +++ b/tests/hermes_cli/test_kanban_review_surfaces.py @@ -0,0 +1,325 @@ +"""Cross-surface regressions for the complete Kanban review lifecycle.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from hermes_cli import kanban as kc +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def review_worker(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> str: + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.setenv("HERMES_PROFILE", "builder") + monkeypatch.delenv("HERMES_DELEGATED_CHILD_CONTEXT", raising=False) + kb._INITIALIZED_PATHS.clear() + kb.init_db() + with kb.connect() as conn: + task_id = kb.create_task(conn, title="Review tool contract", assignee="builder") + task = kb.claim_task(conn, task_id, claimer="builder:1") + assert task is not None + monkeypatch.setenv("HERMES_KANBAN_TASK", task_id) + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(task.current_run_id)) + return task_id + + +def test_review_tools_redact_handoff_and_route_changes( + review_worker: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + from tools import kanban_tools as tools + + secret = "ghp_" + "A" * 40 + requested = json.loads( + tools._handle_request_review({ + "summary": f"Ready; temporary token was {secret}", + "metadata": {"token": secret, "tests_run": 7}, + "reviewer": "reviewer", + }) + ) + assert requested["ok"] is True + + with kb.connect() as conn: + task = kb.get_task(conn, review_worker) + assert task is not None + assert task.status == "review" + assert task.assignee == "reviewer" + handoff = kb.latest_run(conn, review_worker) + assert handoff is not None + assert secret not in (handoff.summary or "") + assert secret not in json.dumps(handoff.metadata) + review = kb.claim_review_task(conn, review_worker, claimer="reviewer:1") + assert review is not None + + monkeypatch.setenv("HERMES_PROFILE", "reviewer") + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(review.current_run_id)) + change_secret = "sk-" + "B" * 32 + changed = json.loads( + tools._handle_request_changes({ + "reason": f"Add a boundary assertion; leaked={change_secret}", + }) + ) + assert changed["ok"] is True + assert changed["implementer"] == "builder" + + with kb.connect() as conn: + task = kb.get_task(conn, review_worker) + assert task is not None + assert task.status == "ready" + assert task.assignee == "builder" + event = [ + item + for item in kb.list_events(conn, review_worker) + if item.kind == "changes_requested" + ][-1] + assert event.payload is not None + assert change_secret not in event.payload["reason"] + assert event.payload["reason"] != ( + "Add a boundary assertion; leaked=" + change_secret + ) + + +def test_review_tools_are_gated_and_visible_to_kanban_workers( + review_worker: str, +) -> None: + import tools.kanban_tools # noqa: F401 - registers the tools + from tools.registry import invalidate_check_fn_cache, registry + from toolsets import resolve_toolset + + invalidate_check_fn_cache() + definitions = registry.get_definitions( + set(resolve_toolset("hermes-cli")), quiet=True + ) + names = { + definition["function"]["name"] + for definition in definitions + if "function" in definition + } + assert "kanban_request_review" in names + assert "kanban_request_changes" in names + + from acp_adapter.tools import _POLISHED_TOOLS + from agent.transports.hermes_tools_mcp_server import EXPOSED_TOOLS + + assert "kanban_request_changes" in _POLISHED_TOOLS + assert "kanban_request_changes" in EXPOSED_TOOLS + assert "kanban_request_changes" in resolve_toolset("kanban") + + +def test_review_cli_round_trip_preserves_handoff( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb._INITIALIZED_PATHS.clear() + kb.init_db() + + with kb.connect() as conn: + task_id = kb.create_task(conn, title="CLI review", assignee="builder") + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + monkeypatch.setenv("HERMES_KANBAN_TASK", task_id) + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(implementation.current_run_id)) + + output = kc.run_slash( + f"request-review {task_id} --summary 'ready for review' " + "--reviewer reviewer --metadata '{\"tests_run\": 3}'" + ) + assert "Requested review" in output + + with kb.connect() as conn: + task = kb.get_task(conn, task_id) + assert task is not None + assert task.assignee == "reviewer" + handoff = kb.latest_run(conn, task_id) + assert handoff is not None + assert handoff.metadata == {"tests_run": 3} + review = kb.claim_review_task(conn, task_id, claimer="reviewer:1") + assert review is not None + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(review.current_run_id)) + + output = kc.run_slash( + f"request-changes {task_id} 'cover the malformed payload case'" + ) + assert "Requested changes" in output + with kb.connect() as conn: + task = kb.get_task(conn, task_id) + assert task is not None + assert task.status == "ready" + assert task.assignee == "builder" + + +def test_worker_guidance_distinguishes_same_card_and_downstream_review() -> None: + from agent.prompt_builder import KANBAN_GUIDANCE + from hermes_cli.config_defaults import DEFAULT_CONFIG + + assert "pre-created downstream review" in KANBAN_GUIDANCE + assert "call `kanban_complete`" in KANBAN_GUIDANCE + assert "Never sticky-block that parent for `review-required`" in KANBAN_GUIDANCE + assert "`kanban_request_changes`" in KANBAN_GUIDANCE + assert "metadata=..." in KANBAN_GUIDANCE + kanban_defaults = DEFAULT_CONFIG["kanban"] + assert isinstance(kanban_defaults, dict) + assert kanban_defaults["review_dispatch"] is True + + repo_root = Path(__file__).resolve().parents[2] + review_skill = repo_root / "skills" / "devops" / "sdlc-review" / "SKILL.md" + skill_text = review_skill.read_text() + assert "kanban_request_changes" in skill_text + assert "approve" in skill_text.lower() + assert "escalate" in skill_text.lower() + + +def test_goal_mode_review_handoff_cannot_bypass_judge( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb._INITIALIZED_PATHS.clear() + kb.init_db() + + with kb.connect() as conn: + tool_task = kb.create_task( + conn, + title="Goal-mode tool task", + assignee="builder", + goal_mode=True, + ) + claimed = kb.claim_task(conn, tool_task, claimer="builder:1") + assert claimed is not None + monkeypatch.setenv("HERMES_KANBAN_TASK", tool_task) + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(claimed.current_run_id)) + + from tools import kanban_tools as tools + + monkeypatch.setattr(tools, "_goal_judge_available", lambda: True) + monkeypatch.setattr( + tools, + "judge_goal", + lambda *args, **kwargs: ( + "continue", + "acceptance evidence is missing", + False, + None, + False, + ), + ) + rejected = json.loads(tools._handle_request_review({"summary": "Looks ready."})) + assert "error" in rejected + assert "rejected by judge" in rejected["error"] + with kb.connect() as conn: + tool_after = kb.get_task(conn, tool_task) + assert tool_after is not None + assert tool_after.status == "running" + + # The shell/CLI path applies the same gate and must not bypass the tool. + with kb.connect() as conn: + cli_task = kb.create_task( + conn, + title="Goal-mode CLI task", + assignee="builder", + goal_mode=True, + ) + cli_claimed = kb.claim_task(conn, cli_task, claimer="builder:2") + assert cli_claimed is not None + monkeypatch.setenv("HERMES_KANBAN_TASK", cli_task) + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(cli_claimed.current_run_id)) + + import agent.auxiliary_client as auxiliary_client + from hermes_cli import goals + + monkeypatch.setattr( + auxiliary_client, + "get_text_auxiliary_client", + lambda purpose: (object(), "judge-model"), + ) + monkeypatch.setattr( + goals, + "judge_goal", + lambda *args, **kwargs: ("continue", "tests are missing", False, None, False), + ) + output = kc.run_slash(f"request-review {cli_task} --summary 'Looks ready.'") + assert "rejected by judge" in output + with kb.connect() as conn: + cli_after = kb.get_task(conn, cli_task) + assert cli_after is not None + assert cli_after.status == "running" + + +def test_goal_loop_stops_after_reviewer_requests_changes( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from hermes_cli import goals + + monkeypatch.setattr( + goals, + "judge_goal", + lambda *args, **kwargs: pytest.fail( + "a terminal review verdict must not be judged" + ), + ) + result = goals.run_kanban_goal_loop( + task_id="t_review", + goal_text="review the change", + run_turn=lambda prompt: pytest.fail("must not run another reviewer turn"), + task_status_fn=lambda: "changes_requested", + block_fn=lambda reason: pytest.fail("must not block"), + first_response="Changes requested.", + ) + assert result["outcome"] == "changes_requested_by_reviewer" + assert result["turns_used"] == 1 + + +def test_cli_and_dashboard_receive_graph_aware_deadlock_diagnostic( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb._INITIALIZED_PATHS.clear() + kb.init_db() + + with kb.connect() as conn: + parent_id = kb.create_task(conn, title="Implementation", assignee="builder") + child_id = kb.create_task( + conn, + title="Review", + assignee="reviewer", + parents=[parent_id], + ) + parent = kb.claim_task(conn, parent_id, claimer="builder:1") + assert parent is not None + assert kb.block_task( + conn, + parent_id, + reason="review-required: ready", + expected_run_id=parent.current_run_id, + ) + + payload = json.loads(kc.run_slash(f"diagnostics --task {parent_id} --json")) + assert any( + item["kind"] == "review_dependency_deadlock" + for item in payload[0]["diagnostics"] + ) + + from plugins.kanban.dashboard.plugin_api import _compute_task_diagnostics + + with kb.connect() as conn: + dashboard = _compute_task_diagnostics(conn, task_ids=[parent_id]) + assert dashboard[parent_id][0]["kind"] == "review_dependency_deadlock" + assert dashboard[parent_id][0]["data"]["waiting_child_ids"] == [child_id] diff --git a/tests/plugins/test_kanban_dashboard_plugin.py b/tests/plugins/test_kanban_dashboard_plugin.py index 5fdb750a38..bab63e8c78 100644 --- a/tests/plugins/test_kanban_dashboard_plugin.py +++ b/tests/plugins/test_kanban_dashboard_plugin.py @@ -224,6 +224,40 @@ def test_task_detail_includes_links_and_events(client): # --------------------------------------------------------------------------- +def test_patch_review_lifecycle_preserves_handoff_and_reopens(client): + task = client.post( + "/api/plugins/kanban/tasks", json={"title": "review me", "assignee": "builder"}, + ).json()["task"] + + response = client.patch( + f"/api/plugins/kanban/tasks/{task['id']}", + json={ + "status": "review", + "summary": "Implementation ready.", + "metadata": {"tests_run": 4}, + }, + ) + assert response.status_code == 200, response.text + assert response.json()["task"]["status"] == "review" + with kb.connect() as conn: + run = kb.latest_run(conn, task["id"]) + assert run is not None + assert run.outcome == "review_requested" + assert run.metadata == {"tests_run": 4} + + response = client.patch( + f"/api/plugins/kanban/tasks/{task['id']}", + json={"status": "ready"}, + ) + assert response.status_code == 200, response.text + assert response.json()["task"]["status"] == "ready" + with kb.connect() as conn: + assert any( + event.kind == "review_reopened" + for event in kb.list_events(conn, task["id"]) + ) + + def test_reopening_parent_demotes_ready_child(client): """Reopening a completed parent must invalidate ready children immediately. diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 1a40acd748..1aaffd6e7c 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -251,6 +251,28 @@ def _goal_judge_available() -> bool: return client is not None and bool(model) +def _goal_mode_handoff_rejection(task, evidence: str) -> Optional[str]: + """Return a rejection reason when a goal-mode terminal handoff is premature.""" + if not task or not task.goal_mode or not _goal_judge_available(): + return None + verdict = "done" + reason = "" + try: + verdict, reason, _, _, _ = judge_goal( + goal=f"{task.title}\n\n{task.body or ''}".strip(), + last_response=evidence.strip(), + ) + except Exception as judge_exc: + # Keep the existing fail-open semantics: an unavailable/broken + # auxiliary judge must not permanently wedge goal-mode work. + logger.warning( + "goal judge check failed, allowing lifecycle handoff: %s", + judge_exc, + exc_info=True, + ) + return reason if verdict != "done" else None + + # --------------------------------------------------------------------------- # Runtime-activity → board-heartbeat bridge (#31752) # --------------------------------------------------------------------------- @@ -730,35 +752,18 @@ def _handle_complete(args: dict, **kw) -> str: # Only enforce when a judge is actually reachable — see # _goal_judge_available for why an unavailable judge fails open. task = kb.get_task(conn, tid) - if task and task.goal_mode and _goal_judge_available(): - verdict = "done" - reason = "" - try: - # judge_goal returns (verdict, reason, parse_failed, - # wait_directive, transport_failed) — see - # hermes_cli/goals.py. Unpacking fewer raises ValueError, - # which the defensive handler below swallows, leaving - # verdict="done" and silently disabling the gate. - verdict, reason, _, _, _ = judge_goal( - goal=f"{task.title}\n\n{task.body or ''}".strip(), - last_response=(summary or result or "").strip(), - ) - except Exception as judge_exc: - # Defensive: judge_goal swallows its own errors, but if - # it ever raises, fail open rather than wedge the worker. - logger.warning( - "goal judge check failed, allowing completion: %s", - judge_exc, - exc_info=True, - ) - if verdict != "done": - return tool_error( - f"Goal completion rejected by judge: {reason}. " - f"To proceed, either: (1) provide explicit acceptance " - f"evidence in your summary matching the task's criteria, " - f"or (2) create continuation tasks with parents=[{tid}] " - f"and keep this task alive." - ) + rejection = _goal_mode_handoff_rejection( + task, + (summary or result or "").strip(), + ) + if rejection is not None: + return tool_error( + f"Goal completion rejected by judge: {rejection}. " + f"To proceed, either: (1) provide explicit acceptance " + f"evidence in your summary matching the task's criteria, " + f"or (2) create continuation tasks with parents=[{tid}] " + f"and keep this task alive." + ) try: ok = kb.complete_task( @@ -891,7 +896,10 @@ def _handle_block(args: dict, **kw) -> str: def _handle_request_review(args: dict, **kw) -> str: - """Move the task to 'review' — implementation done, awaiting a human.""" + """Move implementation into the first-class review phase.""" + delegated_err = _reject_delegated_child_mutation("kanban_request_review") + if delegated_err: + return delegated_err tid = _default_task_id(args.get("task_id")) if not tid: return tool_error( @@ -907,6 +915,18 @@ def _handle_request_review(args: dict, **kw) -> str: "was verified so the reviewer has context" ) summary = redact_sensitive_text(str(summary), force=True) + metadata = args.get("metadata") + if metadata is not None and not isinstance(metadata, dict): + return tool_error( + f"metadata must be an object/dict, got {type(metadata).__name__}" + ) + if metadata is not None: + metadata_json = redact_sensitive_text(json.dumps(metadata), force=True) + try: + metadata = json.loads(metadata_json) + except json.JSONDecodeError: + return tool_error("metadata could not be safely serialized") + metadata = _stamp_worker_session_metadata(tid, metadata) reviewer = args.get("reviewer") or None if reviewer: # Model-supplied free text stored durably on the event payload — @@ -916,9 +936,18 @@ def _handle_request_review(args: dict, **kw) -> str: try: kb, conn = _connect(board=board) try: + task = kb.get_task(conn, tid) + rejection = _goal_mode_handoff_rejection(task, summary) + if rejection is not None: + return tool_error( + f"Goal review handoff rejected by judge: {rejection}. " + "Provide acceptance evidence matching the card before " + "requesting review." + ) ok = kb.request_review( conn, tid, summary=summary, + metadata=metadata, reviewer=reviewer, expected_run_id=_worker_run_id(tid), ) @@ -943,6 +972,54 @@ def _handle_request_review(args: dict, **kw) -> str: return tool_error(f"kanban_request_review: {e}") +def _handle_request_changes(args: dict, **kw) -> str: + """Return a reviewer-owned running task to its implementer.""" + delegated_err = _reject_delegated_child_mutation("kanban_request_changes") + if delegated_err: + return delegated_err + 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 + reason = args.get("reason") + if not reason or not str(reason).strip(): + return tool_error("reason is required — describe the changes needed") + reason = redact_sensitive_text(str(reason), force=True) + board = args.get("board") + try: + kb, conn = _connect(board=board) + try: + ok, detail = kb.request_changes( + conn, + tid, + reason=reason, + expected_run_id=_worker_run_id(tid), + ) + if not ok: + return tool_error( + f"could not request changes for {tid}: {detail or 'invalid review state'}" + ) + landed = kb.get_task(conn, tid) + run = kb.latest_run(conn, tid) + return _ok( + task_id=tid, + run_id=run.id if run else None, + status=landed.status if landed else "ready", + implementer=detail, + ) + finally: + conn.close() + except ValueError as e: + return tool_error(f"kanban_request_changes: {e}") + except Exception as e: + logger.exception("kanban_request_changes failed") + return tool_error(f"kanban_request_changes: {e}") + + def _handle_heartbeat(args: dict, **kw) -> str: """Signal that the worker is still alive during a long operation. @@ -1841,23 +1918,60 @@ KANBAN_REQUEST_REVIEW_SCHEMA = { "type": "string", "description": ( "What was implemented and how it was verified, in one or " - "two sentences — shown to the human reviewer. Don't paste " + "two sentences — shown to the 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." + "Optional reviewer profile. When provided, the task is " + "reassigned to that profile before review dispatch." ), }, + "metadata": { + "type": "object", + "description": ( + "Optional structured handoff facts for the reviewer, such " + "as changed_files, tests_run, commit, or decisions." + ), + "additionalProperties": True, + }, "board": _board_schema_prop(), }, "required": ["summary"], }, } +KANBAN_REQUEST_CHANGES_SCHEMA = { + "name": "kanban_request_changes", + "description": ( + "Reviewer verdict: return the current review run to the original " + "implementer with concrete required changes. This closes the review " + "run, reapplies parent dependency gating, and requeues the task without " + "using block-loop accounting. Only use from a task claimed from the " + "review column; use kanban_block only for a genuine external blocker." + ), + "parameters": { + "type": "object", + "properties": { + "task_id": { + "type": "string", + "description": _DESC_TASK_ID_DEFAULT, + }, + "reason": { + "type": "string", + "description": ( + "Specific, actionable changes the implementer must make " + "before requesting another review." + ), + }, + "board": _board_schema_prop(), + }, + "required": ["reason"], + }, +} + KANBAN_HEARTBEAT_SCHEMA = { "name": "kanban_heartbeat", "description": ( @@ -2279,6 +2393,15 @@ registry.register( emoji="👀", ) +registry.register( + name="kanban_request_changes", + toolset="kanban", + schema=KANBAN_REQUEST_CHANGES_SCHEMA, + handler=_handle_request_changes, + check_fn=_check_kanban_mode, + emoji="↩", +) + registry.register( name="kanban_heartbeat", toolset="kanban", diff --git a/toolsets.py b/toolsets.py index d7ab3f8ab5..235f341fc7 100644 --- a/toolsets.py +++ b/toolsets.py @@ -82,6 +82,7 @@ _HERMES_CORE_TOOLS = [ # tools/kanban_tools.py. "kanban_show", "kanban_list", "kanban_complete", "kanban_block", "kanban_request_review", + "kanban_request_changes", "kanban_heartbeat", "kanban_comment", "kanban_create", "kanban_link", "kanban_unblock", @@ -315,14 +316,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, hand off for human review " - "(request_review — not a block), block for human input, " + "tasks done with structured handoffs, enter first-class review " + "(request_review — not a block), return review changes, 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_request_review", "kanban_request_changes", "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 74beb1643b..88acf6e1ce 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_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. +**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_request_changes`, `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,7 +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`. | +| `request-review ` | Move a task to `review` with a reviewer handoff — NOT a block. Flags: `--summary`, `--metadata`, `--reviewer` (reassigns before review dispatch). | +| `request-changes ` | Reviewer verdict for an active review run: close the review attempt and route the task back to its original implementer. | | `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). | diff --git a/website/docs/reference/tools-reference.md b/website/docs/reference/tools-reference.md index 3aed7de961..c435c16bc2 100644 --- a/website/docs/reference/tools-reference.md +++ b/website/docs/reference/tools-reference.md @@ -126,7 +126,8 @@ 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_request_review` | Hand the implementation to a reviewer with `summary`, optional structured `metadata`, and an optional reviewer profile. Moves the same task to `review`; it is not a block and does not affect block-loop accounting. | `HERMES_KANBAN_TASK` or `kanban` toolset | +| `kanban_request_changes` | Reviewer verdict for an actively claimed review run. Closes the review run, reapplies parent gating, and routes the task back to the original implementer without using a block. | `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 131fe4b640..f830261c57 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_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. | +| `kanban` | `kanban_attach`, `kanban_attach_url`, `kanban_attachments`, `kanban_block`, `kanban_comment`, `kanban_complete`, `kanban_create`, `kanban_heartbeat`, `kanban_link`, `kanban_list`, `kanban_request_changes`, `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 bbf2559983..3b12a53c15 100644 --- a/website/docs/user-guide/features/kanban-worker-lanes.md +++ b/website/docs/user-guide/features/kanban-worker-lanes.md @@ -51,7 +51,7 @@ 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_request_review(summary=..., metadata=..., reviewer=...)` — same-card implementation is complete and enters first-class review; status flips to `review`. The dispatcher loads the bundled `sdlc-review` skill unless `kanban.review_dispatch` is disabled. A reviewer approves with `kanban_complete`, returns actionable rework with `kanban_request_changes`, or escalates a genuine external blocker with `kanban_block`. - `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. @@ -59,20 +59,22 @@ The kanban kernel enforces that exactly one of these terminates each run. A work ## 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 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: +For code-changing tasks, pick the review model encoded by the task graph: -- **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. +- **Same-card review:** call `kanban_request_review(summary=..., metadata=..., reviewer=...)`. The task enters `review` without touching block recurrence accounting. The dispatcher claims it with the bundled `sdlc-review` skill by default. The reviewer approves with `kanban_complete`, calls `kanban_request_changes(reason=...)` to close the review run and route the task back to its original implementer, or blocks only for a genuine external escalation. +- **Pre-created downstream review/QA/release card:** call `kanban_complete` on the implementation phase. Its dependent child cannot promote until this parent is `done`/`archived`. Do not additionally request same-card review and never sticky-block the parent with `review-required:` — either choice strands or duplicates the downstream lane. +- **Human-only boards:** set `kanban.review_dispatch: false`. A task can then remain in `review` until a human approves it or uses `reopen-review`/the dashboard to return it to `ready`/`todo`. -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). +Both review models carry their structured handoff on the lifecycle transition itself. Do not place secrets, tokens, or raw PII in `summary` or `metadata`; run rows are durable. + +The injected `KANBAN_GUIDANCE` covers both graph shapes, `kanban_complete`, the same-card review loop, and `kanban_block` for genuine blockers. ## 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`, `review_requested`, `review_reopened`, `gave_up`, `crashed`, `timed_out`, `reclaimed`, `claim_extended`). +- `task_events` rows carry every state transition (`promoted`, `claimed`, `heartbeat`, `completed`, `blocked`, `review_requested`, `changes_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. @@ -106,6 +108,7 @@ So lane authors don't have to reimplement these: - **Run-level retry** — when a task is retried (post-block, post-crash, post-reclaim), the worker can use the `expected_run_id` parameter on terminating tools to fail fast if its own run was already superseded. - **Per-task max runtime** — `task.max_runtime_seconds` hard-caps wall-clock time per run, regardless of PID liveness. Catches genuinely-deadlocked workers that the live-PID extension would otherwise keep running. - **Stranded-task detection** — a ready task whose assignee never produces a claim within `kanban.stranded_threshold_seconds` (default 30 min) shows up in `hermes kanban diagnostics` as a `stranded_in_ready` warning. Severity escalates to error at 2x the threshold and critical at 6x. Catches typo'd assignees, deleted profiles, and down external worker pools in one signal — identity-agnostic, no per-board allowlist to curate. +- **Legacy review dependency deadlock** — a parent sticky-blocked with `review-required:` while one or more direct children remain dependency-gated in `todo` produces an immediate `review_dependency_deadlock` error. The diagnostic is read-only: it suggests completing the finished phase or unlinking the incorrect edge but never removes a user block automatically. ## Related diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index ce87c5fbd5..39c77a4887 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_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. +- **Agents drive the board through a dedicated `kanban_*` toolset** — `kanban_show`, `kanban_list`, `kanban_complete`, `kanban_request_review`, `kanban_request_changes`, `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. @@ -226,9 +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. + review_dispatch: true # default: spawn the assigned profile with + # the bundled sdlc-review skill. Set false + # for human-only review boards. ``` Override the config flag at runtime via `HERMES_KANBAN_DISPATCH_IN_GATEWAY=0` @@ -296,7 +296,8 @@ 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_request_review` | Start same-card review with a durable `summary`, optional `metadata`, and optional reviewer profile. The task moves to `review`; this is not a block. | `summary` | +| `kanban_request_changes` | Reviewer verdict from an active review run. Closes that run, reapplies parent gating, and routes the task to its original implementer without block-loop accounting. | `reason` | | `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` | @@ -747,7 +748,8 @@ 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 request-review [--summary "..."] [--metadata JSON] [--reviewer PROFILE] +hermes kanban request-changes "" # active reviewer -> implementer hermes kanban reopen-review ... [--reason "..."] # changes requested: 'review' -> ready/todo hermes kanban tail # follow a single task's event stream 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 8430c2d16e..424e8607f2 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 @@ -51,7 +51,7 @@ Hermes Kanban 拥有生命周期的真实状态——`ready` → `running` → ` 每次 claim 必须以以下之一结束： - `kanban_complete(summary=..., metadata=...)` — 任务成功，状态切换为 `done`。 -- `kanban_request_review(summary=...)` — 实现已完成，但在计为 done 之前需要人工审查；状态切换为 `review`，并唤醒订阅者。这**不是** block——它从不计入解除阻塞循环检测，因此任务可以在多次后续跟进中反复进入审查。Reviewer 通过 `kanban_complete` 批准，或将其退回修改（`review -> ready`）。 +- `kanban_request_review(summary=..., metadata=..., reviewer=...)` — 同卡实现进入一等审查。默认由内置 `sdlc-review` skill 启动 reviewer；reviewer 可用 `kanban_complete` 批准、用 `kanban_request_changes` 退回原 implementer，或仅在真正需要外部介入时 block。 - `kanban_block(reason=...)` — 任务等待人工输入，状态切换为 `blocked`。调度器在 `kanban_unblock` 运行时重新生成。 - worker 进程退出而未调用任何工具。内核回收该进程并发出 `crashed`（PID 已消亡）、`gave_up`（连续失败断路器触发）或 `timed_out`（超过 max_runtime）。这是失败路径；健康的 worker 不会在此结束。 @@ -59,20 +59,22 @@ kanban 内核强制要求每次运行恰好由其中一项终止。既未调用 ## 输出与审查交接 -对于大多数涉及代码变更的任务，worker 完成的那一刻并不意味着真正*完成*——还需要人工审查。kanban 内核不强制执行这一区分（"涉及代码变更的任务"定义模糊，且在每个代码 worker 上强制执行会破坏不需要审查的流程）。这是叠加在上层的约定，并拥有一等的终止动作： +代码变更任务必须按照任务图选择审查模型： -- **请求审查而非完成**：以 `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_request_review(summary=..., metadata=..., reviewer=...)`。任务进入 `review`，不会触碰 block 循环计数。默认情况下，调度器使用内置 `sdlc-review` skill 启动 reviewer。Reviewer 用 `kanban_complete` 批准，用 `kanban_request_changes(reason=...)` 关闭审查 run 并将任务退回原 implementer，或只在真正需要外部决策时 block。 +- **预先创建的下游 review/QA/release 卡：**implementation 阶段必须调用 `kanban_complete`。依赖它的子卡只有在父卡为 `done`/`archived` 后才能启动。不要再请求同卡审查，也不要用 `review-required:` sticky-block 父卡，否则会让下游通道卡死或重复。 +- **纯人工审查看板：**设置 `kanban.review_dispatch: false`。任务会停在 `review`，直到人工批准，或通过 `reopen-review`/仪表盘退回 `ready`/`todo`。 -自动注入的 `KANBAN_GUIDANCE` 涵盖 `kanban_complete`（真正终态的任务——拼写修复、文档变更、研究报告）、`kanban_request_review` 交接，以及 `kanban_block`（等待人工输入的真正阻塞）。 +两种审查模型都在生命周期转换本身携带结构化 `summary` 和 `metadata`。这些字段会持久保存，因此不得写入 secret、token 或原始 PII。 + +自动注入的 `KANBAN_GUIDANCE` 同时说明两种任务图、同卡审查循环、`kanban_complete` 和真正外部阻塞所用的 `kanban_block`。 ## 日志与审计追踪 调度器将每个任务的 worker stdout/stderr 写入 `/logs/.log`。日志可通过 kanban 元数据进行审计： - `task_runs` 行携带 `log_path`、退出码（如有）、摘要和元数据。 -- `task_events` 行携带每次状态转换（`promoted`、`claimed`、`heartbeat`、`completed`、`blocked`、`review_requested`、`review_reopened`、`gave_up`、`crashed`、`timed_out`、`reclaimed`、`claim_extended`）。 +- `task_events` 行携带每次状态转换（`promoted`、`claimed`、`heartbeat`、`completed`、`blocked`、`review_requested`、`changes_requested`、`review_reopened`、`gave_up`、`crashed`、`timed_out`、`reclaimed`、`claim_extended`）。 - `kanban_show` 同时返回两者，因此 reviewer（或后续 worker）读取任务时无需访问仪表板即可获得完整历史。 仪表板以摘要、元数据块和退出状态徽章渲染运行历史。CLI 用户可运行 `hermes kanban tail ` 实时跟踪，或运行 `hermes kanban runs ` 查看历史尝试列表。 @@ -106,6 +108,7 @@ profile 通道的特化形态：orchestrator 是一个 Hermes profile，其工 - **运行级重试** — 任务重试时（post-block、post-crash、post-reclaim），worker 可在终止工具上使用 `expected_run_id` 参数，在自身运行已被取代时快速失败。 - **每任务最大运行时间** — `task.max_runtime_seconds` 对每次运行的挂钟时间进行硬性限制，与 PID 存活状态无关。可捕获真正死锁的 worker——否则存活 PID 延期机制会让其持续运行。 - **滞留任务检测** — assignee 在 `kanban.stranded_threshold_seconds`（默认 30 分钟）内始终未产生 claim 的 ready 任务，会在 `hermes kanban diagnostics` 中显示为 `stranded_in_ready` 警告。严重程度在 2 倍阈值时升级为 error，在 6 倍时升级为 critical。可通过单一信号捕获拼写错误的 assignee、已删除的 profile 以及宕机的外部 worker 池——与标识无关，无需维护每个看板的白名单。 +- **旧版审查依赖死锁** —— 如果父卡以 `review-required:` sticky-block，而直接子卡仍因依赖停在 `todo`，系统会立即产生 `review_dependency_deadlock` error。该诊断只读：它建议完成已经结束的阶段或解除错误依赖，不会自动删除用户的 block。 ## 相关资源 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 5ca0614266..9a5d40514f 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_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)。 +- **Agent 通过专用 `kanban_*` 工具集驱动看板** —— `kanban_show`、`kanban_list`、`kanban_complete`、`kanban_block`、`kanban_request_review`、`kanban_request_changes`、`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 动词都有模型使用的等效工具调用。 @@ -161,8 +161,8 @@ hermes kanban stats kanban: dispatch_in_gateway: true # 默认 dispatch_interval_seconds: 60 # 默认 - review_dispatch: false # 默认：无自动 reviewer —— 'review' 中的任务 - # 等待人工。仅在安装了 sdlc-review agent 时设为 true。 + review_dispatch: true # 默认：使用内置 sdlc-review skill 自动启动 reviewer。 + # 纯人工审查看板可设为 false。 ``` 通过 `HERMES_KANBAN_DISPATCH_IN_GATEWAY=0` 在运行时覆盖配置标志以进行调试。标准 gateway 监督适用：直接运行 `hermes gateway start`，或将 gateway 配置为 systemd 用户单元（参见 gateway 文档）。没有运行中的 gateway，`ready` 任务会保持原状，直到 gateway 启动 —— `hermes kanban create` 在创建时会对此发出警告。 @@ -200,7 +200,8 @@ 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_request_review` | 启动同卡审查，携带 `summary`、可选 `metadata` 和 reviewer profile；任务移入 `review`，且不计入 block 循环。 | `summary` | +| `kanban_request_changes` | reviewer 在活动审查 run 中要求修改：关闭审查 run，重新检查父依赖，并把任务交还原 implementer。 | `reason` | | `kanban_block` | 以 `reason` 上报需要人工输入。 | `reason` | | `kanban_heartbeat` | 在长时间操作期间发出存活信号。纯副作用。 | — | | `kanban_comment` | 向任务线程追加持久化备注。 | `task_id`、`body` | @@ -560,7 +561,8 @@ hermes kanban block "" [--ids ...] hermes kanban unblock ... hermes kanban archive ... -hermes kanban request-review [--summary "..."] [--reviewer NAME] # 实现完成 -> 'review'（非 block） +hermes kanban request-review [--summary "..."] [--metadata JSON] [--reviewer PROFILE] +hermes kanban request-changes "<所需修改>" # reviewer -> implementer hermes kanban reopen-review ... [--reason "..."] # 请求修改：'review' -> ready/todo hermes kanban tail # 跟踪单个任务的事件流