From 12395e57b4a3e7fa3610408c043011ec7ba2f8ad Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:59:35 -0700 Subject: [PATCH] =?UTF-8?q?feat:=20/review=20command=20=E2=80=94=20indepen?= =?UTF-8?q?dent=20reviewer=20subagent=20on=20every=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /review takes the last 10 chat messages plus optional instructions, spawns a full-privilege background subagent (the async delegation rail) that investigates the referenced work (PR, code, docs), and its complete review re-enters the spawning session as a normal async-delegation completion the primary agent can act on. - agent/review_engine.py: shared engine (snapshot, briefing, auxiliary.review credential resolution, dispatch, note formatting) - tools/delegate_tool.py: internal credentials_cfg per-call override (never model-facing) resolved through the same credential system as delegation.provider pins - auxiliary.review config block (provider/model/base_url/api_key/ api_mode); provider auto + empty model = inherit the main model - Surfaces: CLI process_command, gateway run.py dispatch + slash_commands handler (binds the approval session key so the completion routes back), TUI/Desktop live dispatch in tui_gateway/server.py, CommandDef registry (+Slack /hermes-only cap) - Docs: delegation.md section + slash-commands.md (both tables) - Tests: 15 engine tests (sabotage-verified: credentials_cfg and dispatch tests fail without the fix), 4 gateway handler tests through the real async rail --- agent/review_engine.py | 232 ++++++++++++++ cli.py | 2 + gateway/run.py | 2 + gateway/slash_commands.py | 58 ++++ hermes_cli/cli_commands_mixin.py | 32 ++ hermes_cli/commands.py | 4 +- hermes_cli/config_defaults.py | 14 + tests/agent/test_review_engine.py | 288 ++++++++++++++++++ tests/gateway/test_review_command.py | 155 ++++++++++ tools/delegate_tool.py | 11 +- tui_gateway/server.py | 47 +++ website/docs/reference/slash-commands.md | 2 + .../docs/user-guide/features/delegation.md | 32 ++ 13 files changed, 877 insertions(+), 2 deletions(-) create mode 100644 agent/review_engine.py create mode 100644 tests/agent/test_review_engine.py create mode 100644 tests/gateway/test_review_command.py diff --git a/agent/review_engine.py b/agent/review_engine.py new file mode 100644 index 0000000000..aceaf0b5ed --- /dev/null +++ b/agent/review_engine.py @@ -0,0 +1,232 @@ +"""Shared engine for the /review command — every surface calls this. + +/review spawns an independent, full-privilege background subagent (the same +async delegation rail as ``delegate_task(background=true)``) whose job is to +thoroughly review whatever the recent conversation presented: a PR, a diff, +code, documentation, or any other work product. The reviewer's result +re-enters the spawning session as a normal async-delegation completion, so +the primary agent sees the review and can act on it. + +Model routing: the reviewer runs on ``auxiliary.review`` (provider/model/ +base_url/api_key/api_mode in config.yaml) when configured; otherwise it +inherits the parent agent's credentials — main-model-first, same convention +as every other auxiliary task. Resolution reuses the delegation credential +resolver (``tools.delegate_tool._resolve_delegation_credentials``) via the +internal ``credentials_cfg`` parameter of ``delegate_task`` so native-SDK +providers, api_mode detection, and credential pools all behave identically +to ``delegation.provider`` pins. + +Surfaces (CLI ``/review``, gateway ``/review``, TUI/Desktop live dispatch) +are thin adapters: snapshot the conversation, call :func:`start_review`, +print the dispatch note. +""" + +from __future__ import annotations + +import json +import logging +from typing import Any, Dict, List, Optional + +logger = logging.getLogger(__name__) + +# How many recent chat messages (user + assistant turns) the reviewer gets. +DEFAULT_CONTEXT_MESSAGES = 10 + +# Per-message excerpt cap. Generous — a PR summary or diff excerpt the primary +# agent just printed is exactly what the reviewer needs — but bounded so a +# pathological turn can't blow up the child's opening context. +_MESSAGE_CHAR_CAP = 12_000 + + +def _message_text(message: Dict[str, Any]) -> str: + """Extract display text from a conversation message dict. + + Handles both plain-string content and OpenAI-style multimodal content + lists (text parts joined; non-text parts noted). + """ + content = message.get("content") + if isinstance(content, str): + return content + if isinstance(content, list): + parts: List[str] = [] + for part in content: + if isinstance(part, dict): + if part.get("type") == "text": + parts.append(str(part.get("text") or "")) + else: + parts.append(f"[{part.get('type', 'attachment')}]") + return "\n".join(p for p in parts if p) + return "" + + +def snapshot_recent_messages( + messages: List[Dict[str, Any]], + limit: int = DEFAULT_CONTEXT_MESSAGES, +) -> List[Dict[str, str]]: + """Return the last ``limit`` user/assistant messages as {role, text} dicts. + + System messages and tool results are excluded — the chat turns are what + the user and their primary agent actually said (the PR link, the summary, + the diff excerpt). Empty-text messages (pure tool-call assistant stubs) + are skipped. + """ + out: List[Dict[str, str]] = [] + for message in reversed(list(messages or [])): + if not isinstance(message, dict): + continue + role = str(message.get("role") or "") + if role not in ("user", "assistant"): + continue + text = _message_text(message).strip() + if not text: + continue + if len(text) > _MESSAGE_CHAR_CAP: + text = text[:_MESSAGE_CHAR_CAP] + "\n[... truncated ...]" + out.append({"role": role, "text": text}) + if len(out) >= limit: + break + out.reverse() + return out + + +def build_review_task( + snapshot: List[Dict[str, str]], + user_prompt: str = "", +) -> tuple: + """Compose the reviewer subagent's (goal, context) pair.""" + goal = ( + "Act as an independent senior reviewer. Thoroughly review the work " + "presented in the conversation excerpt provided in your context: " + "investigate any code, pull request, branch, commit, documentation, " + "design, or other artifact it references (open the PR, read the " + "diff, run the code or tests where feasible) rather than judging " + "from the excerpt alone. Produce a full, structured review: what " + "the work does, whether it is correct and complete, concrete " + "defects or risks found (with file/line references where possible), " + "what was verified vs. only read, and a clear final verdict with " + "recommended next steps." + ) + + lines = [ + "You were spawned by the /review command. The following is an " + "excerpt of the most recent conversation between the user and " + "their primary agent. It is your starting evidence — the work to " + "review is referenced in it.", + "", + "--- Recent conversation (oldest first) ---", + ] + for message in snapshot: + label = "USER" if message["role"] == "user" else "PRIMARY AGENT" + lines.append(f"[{label}]") + lines.append(message["text"]) + lines.append("") + lines.append("--- End of conversation excerpt ---") + if user_prompt.strip(): + lines.append("") + lines.append("Additional review instructions from the user:") + lines.append(user_prompt.strip()) + lines.append("") + lines.append( + "Your review is delivered back into that conversation, addressed to " + "the primary agent and its user. Be direct and specific; do not " + "soften findings." + ) + return goal, "\n".join(lines) + + +def _load_review_credentials_cfg() -> Optional[Dict[str, Any]]: + """Read ``auxiliary.review`` into a delegation-credentials-shaped dict. + + Returns None when the user configured nothing (provider=auto/empty and no + model/base_url), which makes the reviewer inherit the parent agent's + credentials — the main-model-first default. + """ + try: + from hermes_cli.config import load_config_readonly + + full = load_config_readonly() + aux = full.get("auxiliary") or {} + review = aux.get("review") or {} + if not isinstance(review, dict): + return None + except Exception: + return None + + provider = str(review.get("provider") or "").strip() + if provider.lower() == "auto": + provider = "" + model = str(review.get("model") or "").strip() + base_url = str(review.get("base_url") or "").strip() + if not (provider or model or base_url): + return None + return { + "provider": provider, + "model": model, + "base_url": base_url, + "api_key": str(review.get("api_key") or "").strip(), + "api_mode": str(review.get("api_mode") or "").strip(), + } + + +def start_review( + parent_agent, + messages: List[Dict[str, Any]], + user_prompt: str = "", +) -> Dict[str, Any]: + """Dispatch the reviewer subagent in the background. + + Returns the parsed ``delegate_task`` dispatch dict (``status: + "dispatched"`` with a ``delegation_id`` on success, or the synchronous + result dict on channels that cannot route async completions). + + Raises ValueError when there is nothing to review or the dispatch is + rejected/errored. + """ + if parent_agent is None: + raise ValueError("No active agent — send a message first.") + + snapshot = snapshot_recent_messages(messages) + if not snapshot: + raise ValueError("Nothing to review yet — the conversation is empty.") + + goal, context = build_review_task(snapshot, user_prompt) + credentials_cfg = _load_review_credentials_cfg() + + from tools.delegate_tool import delegate_task + + raw = delegate_task( + goal=goal, + context=context, + background=True, + parent_agent=parent_agent, + credentials_cfg=credentials_cfg, + ) + try: + result = json.loads(raw) + except Exception: + raise ValueError(f"Review dispatch failed: {raw!r}") + if isinstance(result, dict) and result.get("error"): + raise ValueError(str(result["error"])) + if not isinstance(result, dict): + raise ValueError(f"Review dispatch failed: {raw!r}") + result.setdefault("review_model", (credentials_cfg or {}).get("model") or "") + return result + + +def format_dispatch_note(result: Dict[str, Any], user_prompt: str = "") -> str: + """Human-facing one-liner for a successful dispatch. Shared by surfaces.""" + model = str(result.get("review_model") or "").strip() + model_note = f" on {model}" if model else "" + focus_note = f" (focus: {user_prompt.strip()})" if user_prompt.strip() else "" + if result.get("status") == "dispatched": + return ( + f"⚖ Review subagent dispatched{model_note}{focus_note} — it is " + f"investigating the last {DEFAULT_CONTEXT_MESSAGES} messages in " + f"the background and its full review will re-enter this " + f"conversation when it finishes." + ) + # Synchronous fallback (channels that cannot route async completions). + return ( + f"⚖ Review completed synchronously{model_note}{focus_note} — " + f"results:\n{json.dumps(result.get('results', result), ensure_ascii=False)[:4000]}" + ) diff --git a/cli.py b/cli.py index 8c235aaeab..27c52e5523 100644 --- a/cli.py +++ b/cli.py @@ -12314,6 +12314,8 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): self._handle_heartbeat_command(cmd_original) elif canonical == "refine": self._handle_refine_command(cmd_original) + elif canonical == "review": + self._handle_review_command(cmd_original) elif canonical == "loop": self._handle_loop_command(cmd_original) elif canonical == "moa": diff --git a/gateway/run.py b/gateway/run.py index fca3365cfa..606412d633 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -17867,6 +17867,8 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew return await self._handle_heartbeat_command(event) if canonical == "refine": return await self._handle_refine_command(event) + if canonical == "review": + return await self._handle_review_command(event) if canonical == "moa": # /moa is one-shot sugar only: run a single prompt through the diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index cd54164bd0..08b59357b5 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -2996,6 +2996,64 @@ class GatewaySlashCommandsMixin: f"any memory/skill updates will be reported when done." ) + async def _handle_review_command(self, event: "MessageEvent") -> str: + """Handle /review — spawn an independent reviewer subagent. + + Snapshots the last 10 chat messages from the session's cached agent, + wraps them (plus any argument text) in a reviewer briefing, and + dispatches a full-privilege background subagent on the async + delegation rail. The completed review re-enters this session as a + normal async-delegation completion turn. + + The approval session-key contextvar is only bound during agent + turns, so it is bound explicitly here — without it the completion + event would carry no gateway route and never re-enter this chat. + """ + args = (event.get_command_args() or "").strip() + quick_key = self._session_key_for_source(event.source) if event.source else None + if not quick_key: + return "Review unavailable (no session)." + if quick_key in self._running_agents: + return "Agent is running — wait for the turn to finish, then /review." + + agent = None + cache_lock = getattr(self, "_agent_cache_lock", None) + if cache_lock is not None: + with cache_lock: + cached = self._agent_cache.get(quick_key) + agent = cached[0] if isinstance(cached, tuple) else cached if cached else None + if agent is None: + return "Nothing to review yet — send a message first." + + snapshot = list(getattr(agent, "_session_messages", None) or []) + + from tools.approval import ( + reset_current_session_key, + set_current_session_key, + ) + + loop = asyncio.get_running_loop() + + def _dispatch(): + token = set_current_session_key(quick_key) + try: + from agent.review_engine import start_review + + return start_review(agent, snapshot, args) + finally: + reset_current_session_key(token) + + try: + result = await loop.run_in_executor(None, _dispatch) + except ValueError as exc: + return str(exc) + except Exception as exc: + return f"/review failed to start: {exc}" + + from agent.review_engine import format_dispatch_note + + return format_dispatch_note(result, args) + async def _handle_subgoal_command(self, event: "MessageEvent") -> str: """Handle /subgoal for gateway platforms (mirror of CLI handler). diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index 93b221fc59..2bde61a90c 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -2778,6 +2778,38 @@ class CLICommandsMixin: f"any memory/skill updates will be reported when done." ) + def _handle_review_command(self, cmd: str) -> None: + """Dispatch /review — spawn an independent reviewer subagent. + + Snapshots the last N chat messages, wraps them (plus any argument + text as extra instructions) in a reviewer briefing, and dispatches a + full-privilege background subagent via the async delegation rail. + The review re-enters this session as a normal async-delegation + completion, addressed to the primary agent. + """ + from cli import _DIM, _RST, _cprint + + parts = (cmd or "").strip().split(None, 1) + prompt = parts[1].strip() if len(parts) > 1 else "" + + agent = getattr(self, "agent", None) + if agent is None: + _cprint(f" {_DIM}Nothing to review yet — send a message first.{_RST}") + return + + snapshot = list(getattr(self, "conversation_history", None) or []) + try: + from agent.review_engine import format_dispatch_note, start_review + + result = start_review(agent, snapshot, prompt) + except ValueError as exc: + _cprint(f" {_DIM}{exc}{_RST}") + return + except Exception as exc: + _cprint(f" /review failed to start: {exc}") + return + _cprint(f" {format_dispatch_note(result, prompt)}") + def _handle_goal_command(self, cmd: str) -> None: """Dispatch /goal subcommands: set / draft / show / gate / status / pause / resume / clear.""" from cli import _DIM, _RST, _cprint diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index 04deecd0ad..cba38bd854 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -211,6 +211,8 @@ COMMAND_REGISTRY: list[CommandDef] = [ busy_policy="dispatch"), CommandDef("refine", "Review this conversation now and save lessons to memory/skills", "Session", args_hint="[focus instructions]"), + CommandDef("review", "Spawn an independent subagent to review the work just discussed (PR, code, docs)", "Session", + args_hint="[review instructions]"), CommandDef("loop", "Re-run a prompt on a recurring interval in this session", "Session", aliases=("proactive",), args_hint="[interval] [--times N] [--until ] | status | pause | resume | stop", @@ -1366,7 +1368,7 @@ _SLACK_PRIORITY_ALIASES = ("btw", "bg") # (session export is an interactive surface; platform is a rare # informational lookup) — without this entry /save tips the registry # past the 50-cap and silently clamps /platform, breaking parity. -_SLACK_VIA_HERMES_ONLY = frozenset({"topup", "moa", "debug", "egress", "init", "version", "diff", "update", "heartbeat", "refine", "pause", "whoami", "platform"}) +_SLACK_VIA_HERMES_ONLY = frozenset({"topup", "moa", "debug", "egress", "init", "version", "diff", "update", "heartbeat", "refine", "review", "pause", "whoami", "platform"}) def _sanitize_slack_name(raw: str) -> str: diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index aa2fcaba95..3038612cbb 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -1110,6 +1110,20 @@ DEFAULT_CONFIG = { "extra_body": {}, "reasoning_effort": "", # per-task thinking level: none|minimal|low|medium|high|xhigh|max|ultra (empty = provider default) }, + # /review — the independent reviewer subagent's model. Unlike other + # aux tasks this is not a single LLM call: the reviewer is a full + # subagent (all normal subagent tools) spawned on the async + # delegation rail. provider/model/base_url/api_key/api_mode are + # resolved through the same credential system as delegation.provider + # pins. Leave provider "auto" + model empty to run the reviewer on + # the main agent's model. + "review": { + "provider": "auto", # auto (= inherit main model) | openrouter | nous | anthropic | ... + "model": "", # e.g. "anthropic/claude-opus-4.6" — a strong reviewer model + "base_url": "", # direct OpenAI-compatible endpoint (takes precedence over provider) + "api_key": "", # API key for base_url / provider override + "api_mode": "", # force transport: chat_completions | anthropic_messages | codex_responses + }, "mcp": { "provider": "auto", "model": "", diff --git a/tests/agent/test_review_engine.py b/tests/agent/test_review_engine.py new file mode 100644 index 0000000000..7a5598aab2 --- /dev/null +++ b/tests/agent/test_review_engine.py @@ -0,0 +1,288 @@ +"""Tests for the /review command engine — agent/review_engine.py. + +Covers conversation snapshotting, reviewer-task composition, +auxiliary.review credential resolution, background dispatch through +delegate_task (including the internal per-call ``credentials_cfg`` +override), and the shared dispatch-note formatter. +""" + +import json +import threading +import time +from unittest.mock import MagicMock + +import pytest + +from agent import review_engine as re_mod +from agent.review_engine import ( + build_review_task, + format_dispatch_note, + snapshot_recent_messages, + start_review, +) +from tools import async_delegation as ad +from tools.process_registry import process_registry + + +@pytest.fixture(autouse=True) +def _clean_state(): + ad._reset_for_tests() + while not process_registry.completion_queue.empty(): + process_registry.completion_queue.get_nowait() + yield + deadline = time.monotonic() + 2.0 + while ad.active_count() and time.monotonic() < deadline: + time.sleep(0.02) + ad._reset_for_tests() + while not process_registry.completion_queue.empty(): + process_registry.completion_queue.get_nowait() + + +# --------------------------------------------------------------------------- +# snapshot_recent_messages +# --------------------------------------------------------------------------- + +def test_snapshot_takes_last_ten_chat_messages_only(): + msgs = ( + [{"role": "system", "content": "sys"}] + + [{"role": "user", "content": f"m{i}"} for i in range(15)] + + [{"role": "tool", "content": "tool out"}] + ) + snap = snapshot_recent_messages(msgs) + assert len(snap) == 10 + assert snap[0]["text"] == "m5" + assert snap[-1]["text"] == "m14" + assert all(m["role"] == "user" for m in snap) + + +def test_snapshot_skips_toolcall_stub_assistant_messages(): + msgs = [ + {"role": "user", "content": "make a PR"}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "x"}]}, + {"role": "tool", "content": "created"}, + {"role": "assistant", "content": "PR #123: https://example.com/pr/123"}, + ] + snap = snapshot_recent_messages(msgs) + assert [m["text"] for m in snap] == [ + "make a PR", + "PR #123: https://example.com/pr/123", + ] + + +def test_snapshot_handles_multimodal_content_lists(): + msgs = [{ + "role": "user", + "content": [ + {"type": "text", "text": "look at this"}, + {"type": "image_url", "image_url": {"url": "x"}}, + ], + }] + snap = snapshot_recent_messages(msgs) + assert snap[0]["text"] == "look at this\n[image_url]" + + +def test_snapshot_caps_oversized_messages(): + msgs = [{"role": "user", "content": "x" * 50_000}] + snap = snapshot_recent_messages(msgs) + assert len(snap[0]["text"]) < 13_000 + assert snap[0]["text"].endswith("[... truncated ...]") + + +# --------------------------------------------------------------------------- +# build_review_task +# --------------------------------------------------------------------------- + +def test_build_review_task_includes_excerpt_and_prompt(): + snap = [ + {"role": "user", "text": "review my PR"}, + {"role": "assistant", "text": "PR #99 opened"}, + ] + goal, context = build_review_task(snap, "focus on security") + assert "reviewer" in goal.lower() + assert "[USER]" in context and "[PRIMARY AGENT]" in context + assert "PR #99 opened" in context + assert "focus on security" in context + + +def test_build_review_task_without_prompt_has_no_instruction_block(): + goal, context = build_review_task([{"role": "user", "text": "hi"}]) + assert "Additional review instructions" not in context + + +# --------------------------------------------------------------------------- +# auxiliary.review credential resolution +# --------------------------------------------------------------------------- + +def test_load_review_credentials_cfg_reads_config(monkeypatch): + monkeypatch.setattr( + "hermes_cli.config.load_config_readonly", + lambda: {"auxiliary": {"review": { + "provider": "openrouter", + "model": "anthropic/claude-opus-4.6", + }}}, + ) + cfg = re_mod._load_review_credentials_cfg() + assert cfg == { + "provider": "openrouter", + "model": "anthropic/claude-opus-4.6", + "base_url": "", + "api_key": "", + "api_mode": "", + } + + +def test_load_review_credentials_cfg_auto_means_inherit(monkeypatch): + monkeypatch.setattr( + "hermes_cli.config.load_config_readonly", + lambda: {"auxiliary": {"review": {"provider": "auto", "model": ""}}}, + ) + assert re_mod._load_review_credentials_cfg() is None + + +def test_load_review_credentials_cfg_missing_section(monkeypatch): + monkeypatch.setattr( + "hermes_cli.config.load_config_readonly", lambda: {"auxiliary": {}} + ) + assert re_mod._load_review_credentials_cfg() is None + + +# --------------------------------------------------------------------------- +# delegate_task credentials_cfg override (the internal /review routing hook) +# --------------------------------------------------------------------------- + +def _fake_parent(): + parent = MagicMock() + parent._delegate_depth = 0 + parent.session_id = "review-parent-sess" + parent._interrupt_requested = False + parent._active_children = [] + parent._active_children_lock = None + return parent + + +def test_delegate_task_credentials_cfg_overrides_delegation_config(monkeypatch): + """The per-call credentials_cfg dict must reach the credential resolver + instead of the global delegation config section.""" + import tools.delegate_tool as dt + + seen = {} + + def fake_resolve(cfg, parent_agent): + seen["cfg"] = cfg + return { + "model": cfg.get("model"), "provider": None, "base_url": None, + "api_key": None, "api_mode": None, "command": None, "args": None, + } + + fake_child = MagicMock() + fake_child._delegate_role = "leaf" + monkeypatch.setattr(dt, "_resolve_delegation_credentials", fake_resolve) + monkeypatch.setattr(dt, "_build_child_agent", lambda **kw: fake_child) + monkeypatch.setattr( + dt, "_run_single_child", + lambda *a, **k: { + "task_index": 0, "status": "completed", "summary": "ok", + "api_calls": 1, "duration_seconds": 0.1, "model": "m", + "exit_reason": "completed", + }, + ) + + override = {"provider": "openrouter", "model": "review-model-x"} + out = dt.delegate_task( + goal="review this", + background=True, + parent_agent=_fake_parent(), + credentials_cfg=override, + ) + parsed = json.loads(out) + assert parsed["status"] == "dispatched" + assert seen["cfg"] == override + + +# --------------------------------------------------------------------------- +# start_review end-to-end through the async delegation rail +# --------------------------------------------------------------------------- + +def test_start_review_dispatches_background_and_completes(monkeypatch): + import tools.delegate_tool as dt + + captured = {} + + def fake_run_single_child(task_index, goal, child=None, parent_agent=None, **kw): + captured["goal"] = goal + return { + "task_index": 0, "status": "completed", + "summary": "REVIEW: looks good", "api_calls": 2, + "duration_seconds": 0.1, "model": "m", "exit_reason": "completed", + } + + fake_child = MagicMock() + fake_child._delegate_role = "leaf" + creds = { + "model": "m", "provider": None, "base_url": None, "api_key": None, + "api_mode": None, "command": None, "args": None, + } + built = {} + + def fake_build(**kw): + built.update(kw) + return fake_child + + monkeypatch.setattr(dt, "_build_child_agent", fake_build) + monkeypatch.setattr(dt, "_run_single_child", fake_run_single_child) + monkeypatch.setattr(dt, "_resolve_delegation_credentials", lambda *a, **k: creds) + monkeypatch.setattr(re_mod, "_load_review_credentials_cfg", lambda: None) + + msgs = [ + {"role": "user", "content": "open a PR for the fix"}, + {"role": "assistant", "content": "PR #77 opened: https://x/pull/77"}, + ] + result = start_review(_fake_parent(), msgs, "check the tests") + assert result["status"] == "dispatched" + + # The reviewer briefing carries the conversation excerpt + user prompt. + assert "PR #77 opened" in built["context"] + assert "check the tests" in built["context"] + assert "reviewer" in built["goal"].lower() + + # The completion re-enters via the shared queue like any subagent. + deadline = time.monotonic() + 5.0 + evt = None + while time.monotonic() < deadline: + try: + evt = process_registry.completion_queue.get(timeout=0.2) + break + except Exception: + continue + assert evt is not None and evt["type"] == "async_delegation" + assert evt["results"][0]["summary"] == "REVIEW: looks good" + + +def test_start_review_rejects_empty_conversation(): + with pytest.raises(ValueError, match="empty"): + start_review(_fake_parent(), [], "") + + +def test_start_review_requires_agent(): + with pytest.raises(ValueError, match="No active agent"): + start_review(None, [{"role": "user", "content": "x"}], "") + + +# --------------------------------------------------------------------------- +# format_dispatch_note +# --------------------------------------------------------------------------- + +def test_format_dispatch_note_dispatched(): + note = format_dispatch_note( + {"status": "dispatched", "review_model": "opus"}, "security" + ) + assert "dispatched on opus" in note + assert "focus: security" in note + assert "re-enter" in note + + +def test_format_dispatch_note_sync_fallback(): + note = format_dispatch_note( + {"results": [{"summary": "fine"}], "review_model": ""}, "" + ) + assert "synchronously" in note diff --git a/tests/gateway/test_review_command.py b/tests/gateway/test_review_command.py new file mode 100644 index 0000000000..ddeb5b47b9 --- /dev/null +++ b/tests/gateway/test_review_command.py @@ -0,0 +1,155 @@ +"""Gateway /review command — direct handler tests. + +Drives the REAL GatewayRunner._handle_review_command on a bare runner with a +cached agent, dispatching through the REAL delegate_task background rail +(child build/run stubbed at the delegate_tool seam, same pattern as +tests/tools/test_async_delegation.py). +""" + +import json +import time +from unittest.mock import MagicMock + +import pytest + +from tools import async_delegation as ad +from tools.process_registry import process_registry + + +@pytest.fixture(autouse=True) +def _clean_state(): + ad._reset_for_tests() + while not process_registry.completion_queue.empty(): + process_registry.completion_queue.get_nowait() + yield + deadline = time.monotonic() + 2.0 + while ad.active_count() and time.monotonic() < deadline: + time.sleep(0.02) + ad._reset_for_tests() + while not process_registry.completion_queue.empty(): + process_registry.completion_queue.get_nowait() + + +SESSION_KEY = "agent:main:test:dm:1" + + +def _make_agent(): + agent = MagicMock() + agent._delegate_depth = 0 + agent.session_id = "gw-review-sess" + agent._interrupt_requested = False + agent._active_children = [] + agent._active_children_lock = None + agent._session_messages = [ + {"role": "user", "content": "open a PR"}, + {"role": "assistant", "content": "PR #5 opened: https://x/pull/5"}, + ] + return agent + + +def _make_runner(agent): + import threading + + from gateway.run import GatewayRunner + + runner = object.__new__(GatewayRunner) + runner._running_agents = {} + runner._agent_cache = {SESSION_KEY: agent} + runner._agent_cache_lock = threading.Lock() + runner._session_key_for_source = lambda source: SESSION_KEY + return runner + + +class _Event: + source = object() # any non-None sentinel + + def __init__(self, args=""): + self._args = args + + def get_command_args(self): + return self._args + + +@pytest.mark.asyncio +async def test_review_command_dispatches_background_subagent(monkeypatch): + import tools.delegate_tool as dt + from agent import review_engine as re_mod + + fake_child = MagicMock() + fake_child._delegate_role = "leaf" + creds = { + "model": "m", "provider": None, "base_url": None, "api_key": None, + "api_mode": None, "command": None, "args": None, + } + built = {} + + def fake_build(**kw): + built.update(kw) + return fake_child + + monkeypatch.setattr(dt, "_build_child_agent", fake_build) + monkeypatch.setattr(dt, "_resolve_delegation_credentials", lambda *a, **k: creds) + monkeypatch.setattr( + dt, "_run_single_child", + lambda *a, **k: { + "task_index": 0, "status": "completed", "summary": "review done", + "api_calls": 1, "duration_seconds": 0.1, "model": "m", + "exit_reason": "completed", + }, + ) + monkeypatch.setattr(re_mod, "_load_review_credentials_cfg", lambda: None) + + agent = _make_agent() + runner = _make_runner(agent) + out = await runner._handle_review_command(_Event("check tests")) + + assert "dispatched" in out + assert "PR #5 opened" in built["context"] + assert "check tests" in built["context"] + + # Completion event routes back to the gateway session key (captured from + # the approval contextvar the handler binds around the dispatch). + deadline = time.monotonic() + 5.0 + evt = None + while time.monotonic() < deadline: + try: + evt = process_registry.completion_queue.get(timeout=0.2) + break + except Exception: + continue + assert evt is not None + assert evt["type"] == "async_delegation" + assert evt["session_key"] == SESSION_KEY + assert evt["results"][0]["summary"] == "review done" + + +@pytest.mark.asyncio +async def test_review_command_rejects_while_agent_running(): + agent = _make_agent() + runner = _make_runner(agent) + runner._running_agents = {SESSION_KEY: object()} + out = await runner._handle_review_command(_Event()) + assert "Agent is running" in out + + +@pytest.mark.asyncio +async def test_review_command_requires_cached_agent(): + runner = _make_runner(None) + runner._agent_cache = {} + out = await runner._handle_review_command(_Event()) + assert "send a message first" in out + + +@pytest.mark.asyncio +async def test_review_dispatch_branch_reaches_handler(monkeypatch): + """/review typed in a gateway chat must not fall through to the agent. + + Proves the gateway/run.py dispatch branch exists by resolving the command + through the registry the same way _handle_message does. + """ + from hermes_cli.commands import resolve_command + + cmd = resolve_command("review") + assert cmd is not None + assert cmd.name == "review" + assert not cmd.cli_only diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 8699edb984..28c7bb9c4e 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -3606,6 +3606,7 @@ def delegate_task( subagent_id: Optional[str] = None, message: Optional[str] = None, parent_agent=None, + credentials_cfg: Optional[Dict[str, Any]] = None, ) -> str: """ Spawn one or more child agents to handle delegated tasks, or control @@ -3699,8 +3700,16 @@ def delegate_task( # bundle (base_url, api_key, api_mode) via the same runtime provider system # used by CLI/gateway startup. When unconfigured, returns None values so # children inherit from the parent. + # + # ``credentials_cfg`` (internal callers only — never model-facing) is a + # per-call override shaped like the delegation config section + # ({provider, model, base_url, api_key, api_mode}); the /review engine + # uses it to route its reviewer subagent onto ``auxiliary.review`` + # without touching the global delegation pin. try: - creds = _resolve_delegation_credentials(cfg, parent_agent) + creds = _resolve_delegation_credentials( + credentials_cfg if credentials_cfg else cfg, parent_agent + ) except ValueError as exc: return tool_error(str(exc)) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index aa74daea42..6d1ca327dd 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -13970,6 +13970,7 @@ _LIVE_SESSION_DIRECT_COMMANDS = frozenset( "models", "prompt", "rename", + "review", "status", "usage", } @@ -13978,6 +13979,50 @@ _LIVE_SESSION_DIRECT_COMMANDS = frozenset( _ISOLATED_SESSION_READ_COMMANDS = frozenset({"context", "tools", "help"}) +def _format_live_review_output(session: Optional[dict], arg: str) -> str: + """Dispatch /review against the live TUI/desktop session's agent. + + Spawns the reviewer subagent on the async delegation rail; the TUI + notification poller already drains async-delegation completions for the + owning session, so the finished review re-enters this chat as a normal + completion turn. The dispatch stamps the parent agent's durable + session_id as the completion's session_key (the delegate_task CLI-path + fallback), which is exactly what ``_session_owns_notification_event`` + matches against. + """ + if session is None: + return "Nothing to review yet — send a message first." + if _session_uses_compute_host(session): + return ( + "/review runs on the local agent only for now — this session's " + "agent lives on a remote compute host." + ) + agent = session.get("agent") + if agent is None: + return "Nothing to review yet — send a message first." + if session.get("running"): + return "session busy — wait for the current turn to finish, then /review" + + history_lock = session.get("history_lock") + if history_lock is not None: + with history_lock: + snapshot = list(session.get("history", [])) + else: + snapshot = list(session.get("history", [])) + if not snapshot: + snapshot = list(getattr(agent, "_session_messages", None) or []) + + try: + from agent.review_engine import format_dispatch_note, start_review + + result = start_review(agent, snapshot, arg or "") + except ValueError as exc: + return str(exc) + except Exception as exc: + return f"/review failed to start: {exc}" + return format_dispatch_note(result, arg or "") + + def _format_live_usage_output(session: dict) -> str: agent = session.get("agent") usage = _session_usage_snapshot(session) @@ -14183,6 +14228,8 @@ def _live_slash_command_output(sid: str, session: Optional[dict], name: str, arg if session is None: return "(._.) No active agent -- send a message first." return _format_live_usage_output(session) + if name == "review": + return _format_live_review_output(session, arg) if name == "history": if session is None: return "No conversation history yet." diff --git a/website/docs/reference/slash-commands.md b/website/docs/reference/slash-commands.md index fb9fc5b2b5..e1317c216b 100644 --- a/website/docs/reference/slash-commands.md +++ b/website/docs/reference/slash-commands.md @@ -55,6 +55,7 @@ Type `/` in the CLI to open the autocomplete menu. Built-in commands are case-in | `/subgoal ` | Append a user-supplied criterion to the active goal mid-loop. The continuation prompt surfaces all subgoals to the agent verbatim, and the judge factors them into its DONE/CONTINUE verdict — so the goal isn't marked done until the original goal **and** every subgoal are met. Subcommands: `/subgoal` (list), `/subgoal remove `, `/subgoal clear`. Requires an active `/goal`. | | `/heartbeat every ` (alias: `/hb`) | Set a recurring prompt that re-enters **this session** as a normal user turn whenever it's idle and the interval has elapsed (min 60s; missed ticks coalesce). Subcommands: `/heartbeat status`, `/heartbeat pause`, `/heartbeat resume`, `/heartbeat clear`. Session-scoped and in-process — use `hermes cron` for durable isolated schedules. See [Session Heartbeats](/user-guide/features/heartbeat). | | `/refine [focus]` | Run the background memory/skill self-improvement review **now** instead of waiting for the automatic post-turn trigger. Optional focus text steers the review (e.g. `/refine save the deploy workflow as a skill`). Runs in a background fork against a conversation snapshot — the live session and prompt cache are untouched; results are reported when done. | +| `/review [instructions]` | Spawn an independent, full-privilege reviewer subagent to review the work just discussed — a PR, code, docs, any artifact referenced in the last 10 chat messages. It investigates in the background (opens the PR, reads the diff, runs code) and its full review re-enters this session as a background-subagent completion the primary agent can act on. Pin a dedicated review model via `auxiliary.review` in config.yaml (defaults to your main model). See [Subagent Delegation](/user-guide/features/delegation#the-review-command). | | `/moa ` | Run a single prompt through the default [Mixture of Agents](/user-guide/features/mixture-of-agents) preset, then restore your current model. One-shot — does not change your session model. | | `/resume [name]` | Resume a previously-named session | | `/sessions` (TUI alias: `/switch`) | Classic CLI: browse and resume previous sessions in an interactive picker. TUI: open the live session switcher for currently open TUI sessions. Use `/sessions new` in the TUI to start another live session immediately. | @@ -256,6 +257,7 @@ The messaging gateway supports the following built-in commands inside Telegram, | `/subgoal ` | Append criteria to the active `/goal` mid-loop (`/subgoal`, `/subgoal remove `, `/subgoal clear`). | | `/heartbeat every ` (alias: `/hb`) | Set a recurring prompt that re-enters this session when idle. Subcommands: `status`, `pause`, `resume`, `clear`. On Slack use `/hermes heartbeat …`. | | `/refine [focus]` | Run the memory/skill self-improvement review now, optionally with focus instructions. On Slack use `/hermes refine …`. | +| `/review [instructions]` | Spawn an independent reviewer subagent for the work just discussed (PR, code, docs); its review re-enters this chat when done. On Slack use `/hermes review …`. | | `/moa ` | Run one prompt through the default [Mixture of Agents](/user-guide/features/mixture-of-agents) preset, then restore the session model. | | `/branch [name]` (alias: `/fork`) | Branch the current session (explore a different path). | | `/agents` (alias: `/tasks`) | Show active agents and running tasks. | diff --git a/website/docs/user-guide/features/delegation.md b/website/docs/user-guide/features/delegation.md index 15d27cc20e..c4d548683b 100644 --- a/website/docs/user-guide/features/delegation.md +++ b/website/docs/user-guide/features/delegation.md @@ -170,6 +170,38 @@ Resolution order: `delegation.base_url` (direct endpoint) takes precedence, then Note that the pin is global: `delegate_task` has no per-task model parameter, so every child in a batch runs on the configured delegation model. For quality-sensitive subtasks that need a stronger model, either leave `delegation.model` unset for that session or hand the task to the [kanban board](kanban.md#per-task-model-override), which does support a per-task model override. +## The `/review` Command + +`/review` spawns an independent, full-privilege background subagent whose only job is to review the work your conversation just produced — a PR, a diff, code, documentation, a design. It works on every surface: CLI, TUI, the Desktop app, and every gateway messaging platform. + +``` +/review # review whatever the last 10 messages presented +/review focus on security # add extra instructions for the reviewer +``` + +What happens: + +1. The last 10 user/assistant messages are snapshotted as the reviewer's starting evidence (tool output and system messages are excluded). +2. A reviewer subagent is dispatched on the same background delegation rail as `delegate_task` — it gets the full normal subagent toolset (terminal, web, files, browser...), so it actually opens the PR, reads the diff, and runs code rather than judging from the excerpt. +3. When it finishes, its full review re-enters the same session as a normal background-subagent completion — your primary agent sees it and can act on it (fix the findings, push follow-ups, reply to you). + +The canonical flow: your main agent opens a PR, you type `/review`, and a second pair of eyes investigates it while you keep working; the review lands back in the chat addressed to the agent that created the PR. + +### Review model + +By default the reviewer runs on your main model. To pin a dedicated review model, set `auxiliary.review` in `config.yaml`: + +```yaml +auxiliary: + review: + provider: openrouter # or nous, anthropic, a direct base_url, ... + model: anthropic/claude-opus-4.6 # a strong reviewer model +``` + +Credentials resolve exactly like a `delegation.provider` pin (full runtime-provider bundle: base_url, api key, api_mode). `provider: auto` with an empty `model` means "inherit the main agent's model" — the default. + +`/review` is deliberately separate from `/refine`: `/refine` reviews the conversation to update memory and skills, `/review` reviews the *work product* the conversation created. + ## Inherited Tool Access `delegate_task` does not accept a model-facing `toolsets` parameter. Each subagent inherits the parent's enabled toolsets so the model cannot grant a child capabilities that the parent does not have. Configure the parent's tools before starting the conversation if delegated work needs additional capabilities.