This repository has been archived on 2026-08-04. You can view files and clone it, but cannot push or open issues or pull requests.
orchestrator/agent-team/agent_team/nodes/review_loop.py
Adam Moussa 71edeb3f3f fix(agent-team): review-round cap counts only reviewer verdicts (LOGIC-04)
_review_round_index counted EVERY review_verdicts entry, including the synthetic
human-gate verdict graph._apply_plan_decision folds in on a "request changes"
(reviewer == "human_plan_gate"). That inflated the count so a revised plan could
escalate prematurely without a fresh adversarial review.

Now counts only reviewer-authored verdicts: a new _is_reviewer_verdict excludes
entries tagged reviewer=="human_plan_gate" (read from the verdict dict's own
field — no graph.py import). A human request_changes now grants the revised plan
a fresh reviewer-round budget. Termination still bounded by MAX_PLAN_GATE_VISITS
(each request_changes consumes one gate visit). 1490 passed.
2026-06-24 11:57:03 -04:00

529 lines
21 KiB
Python

"""GPT-4.1 review-loop node (design §3.3, §7.1 Phase P2).
The review loop is the adversarial plan-review stage of the Plane-2 pipeline::
INTAKE -> CLARIFIER -> PLANNER -> REVIEW LOOP -> BUILDERS -> ...
After the planner produces a phased plan, this node runs an **adversarial
cross-family review** of that plan — the ``sh-plan-review`` / ``cross_reviewer``
discipline (GPT-4.1, a different model family than the Claude planner, so it
catches different blind spots). The reviewer returns a verdict:
* ``APPROVE`` — the plan clears the bar; the task advances to the builders.
* ``REQUEST_CHANGES`` — the plan has gaps; the task **loops back to the
planner** carrying the reviewer's findings, and the planner revises.
The loop is bounded. After ``max_rounds`` of ``REQUEST_CHANGES`` without
convergence the node **escalates to Adam** (parks the task with an ALARM)
rather than spinning — design §3.3 stability bound (6): "a task that stalls ...
parks and ALARMs rather than spinning", and §7.1 P2 "including loop-back and
the escalate-to-Adam path".
Design notes honoured here:
* The reviewer is **GPT-4.1 via the orchestrator's** ``cross_reviewer`` agent,
reached through the local ``run.py`` (design §3.2: the R720 coordinator calls
the local ``~/orchestrator/run.py`` for non-Claude single-shot sub-tasks, so
they keep API billing + LangSmith tracing). It is therefore **not** routed
through :mod:`agent_team.billing` (which is the *Claude* seam). The actual
call is delegated to an injectable :data:`ReviewInvoker` so this node stays
dependency-free and unit-testable, mirroring the ``billing.set_invoker``
pattern in the foundation.
* The node is a pure function over
:class:`~agent_team.task_model.PipelineState`: it returns only the partial
state keys it changes (``review_verdicts``, ``current_phase``, ``status``),
which the LangGraph reducer merges. It never writes to repos and performs no
durable I/O of its own — the SQLite checkpointer persists the merged state.
* :func:`route_after_review` is the LangGraph conditional-edge function that
reads the verdict this node recorded and returns the next node name
(``"build"`` / ``"plan"`` / ``"parked"``).
"""
from __future__ import annotations
import json
import os
import re
import subprocess
from dataclasses import dataclass
from datetime import datetime, timezone
from enum import Enum
from typing import Any, Callable, Mapping
from agent_team.task_model import Phase, PipelineState, TaskStatus
__all__ = [
"DEFAULT_MAX_REVIEW_ROUNDS",
"ReviewInvoker",
"ReviewOutcome",
"ReviewResult",
"ReviewVerdict",
"bind_review_node",
"review_node",
"route_after_review",
"set_review_invoker",
]
# Node names this stage routes to (LangGraph graph node ids). Kept as module
# constants so the conditional-edge mapping and the node body cannot drift.
BUILD_NODE = "build"
PLAN_NODE = "plan"
PARKED_NODE = "parked"
# Default cap on adversarial review rounds before the loop escalates to Adam
# (design §3.3 stability bound 6; §7.1 P2 escalate-to-Adam path). Overridable
# per task via config["max_review_rounds"].
DEFAULT_MAX_REVIEW_ROUNDS = 3
# ``reviewer`` tag on the SYNTHETIC verdict that graph._apply_plan_decision folds
# into ``review_verdicts`` when the owner picks "request changes" at the plan
# gate. Those are HUMAN-gate verdicts, not adversarial reviewer rounds, so they
# must NOT count toward the review-round cap (LOGIC-04): a human request_changes
# should grant the revised plan a fresh review-round budget. We match on the
# verdict dict's ``reviewer`` field (data already present in ``review_verdicts``)
# rather than importing graph.py, to avoid a layering cycle. The plan-review GATE
# is independently bounded by graph.MAX_PLAN_GATE_VISITS, so excluding these from
# the REVIEW cap cannot create an unbounded loop.
HUMAN_PLAN_GATE_REVIEWER = "human_plan_gate"
# Config / env key naming the per-task review-round cap.
_MAX_ROUNDS_CONFIG_KEY = "max_review_rounds"
_MAX_ROUNDS_ENV = "AGENT_TEAM_MAX_REVIEW_ROUNDS"
# Config key naming the orchestrator entry point (the local run.py). Defaults
# to the rsync'd path on the R720; overridable for tests / non-default installs.
_RUN_PY_CONFIG_KEY = "orchestrator_run_py"
_DEFAULT_RUN_PY = os.path.expanduser("~/Documents/repositories/orchestrator/run.py")
# Config / env keys + default for the orchestrator subprocess timeout (seconds).
# Bounds the default shell-out so a hung run.py cannot stall the bounded review
# loop. Mirrors the resolver in review_loop_llm so the two cannot drift.
_TIMEOUT_CONFIG_KEY = "review_timeout_seconds"
_TIMEOUT_ENV = "AGENT_TEAM_REVIEW_TIMEOUT_SECONDS"
_DEFAULT_TIMEOUT_SECONDS = 600.0
# Sentinel returned by the default invoker when the orchestrator subprocess
# times out. It parses (via parse_verdict) to REQUEST_CHANGES, so a hung run.py
# fails CLOSED (loops back / escalates) instead of blocking the bounded loop.
_TIMEOUT_VERDICT_TEXT = (
"REQUEST CHANGES: orchestrator review timed out (failing closed)."
)
# Verdict tokens the reviewer output is scanned for, matched on WORD BOUNDARIES
# (not substrings). The change marker is the sh-plan-review rubric token
# ``BLOCK`` (e.g. "BLOCK: ..."), matched as a whole word so it does NOT fire
# inside benign prose like "no blockers" / "no blocking issues" (the substring
# false-positive this guards against — those inflected words are deliberately
# NOT change tokens). The former "NO BLOCKERS"/"NO BLOCKING" approve tokens
# existed only to undo that substring false-positive; with word-boundary
# matching they are unreachable (such bare prose is genuinely ambiguous and must
# fail closed), so they are dropped. REQUEST_CHANGES still wins on a tie so an
# ambiguous review fails closed (loops back / escalates) rather than advancing a
# flagged plan.
_APPROVE_TOKENS = ("APPROVE", "APPROVED", "LGTM")
_CHANGES_TOKENS = (
"REQUEST CHANGES",
"REQUEST_CHANGES",
"REQUESTCHANGES",
"BLOCK",
"NEEDS CHANGES",
"NEEDS WORK",
)
def _compile_token_pattern(tokens: tuple[str, ...]) -> re.Pattern[str]:
"""Compile an alternation of ``tokens`` matched on word boundaries.
Word-boundary anchoring is what keeps ``BLOCK`` from matching inside
``BLOCKERS``/``BLOCKING`` (the substring false-positive this guards against).
Tokens are sorted longest-first so a multi-word token (e.g. ``REQUEST
CHANGES``) is preferred over a shorter overlapping one.
"""
ordered = sorted(tokens, key=len, reverse=True)
alternation = "|".join(re.escape(tok) for tok in ordered)
return re.compile(rf"\b(?:{alternation})\b", re.IGNORECASE)
_APPROVE_RE = _compile_token_pattern(_APPROVE_TOKENS)
_CHANGES_RE = _compile_token_pattern(_CHANGES_TOKENS)
class ReviewVerdict(Enum):
"""The adversarial reviewer's verdict on a plan (design §3.3)."""
APPROVE = "approve"
REQUEST_CHANGES = "request_changes"
class ReviewOutcome(Enum):
"""What the loop decided to do after recording a verdict.
``APPROVED`` — advance to the builders. ``LOOP_BACK`` — return to the
planner with the findings. ``ESCALATE`` — the round cap was hit without
convergence; park the task and ALARM Adam (design §3.3 bound 6, §7.1 P2).
"""
APPROVED = "approved"
LOOP_BACK = "loop_back"
ESCALATE = "escalate"
@dataclass
class ReviewResult:
"""One adversarial review pass, appended to ``review_verdicts`` (design §3.3).
``verdict`` is the parsed :class:`ReviewVerdict`; ``round_index`` is the
1-based review round; ``outcome`` is the loop decision this pass produced;
``findings`` is the reviewer's full text (the planner consumes it on a
loop-back); ``reviewer`` records the agent/model for the audit trail;
``raw`` is the untouched invoker payload; ``created_at`` is an ISO-8601 UTC
timestamp.
"""
verdict: ReviewVerdict
round_index: int
outcome: ReviewOutcome
findings: str
reviewer: str = "cross_reviewer"
raw: Any = None
created_at: str | None = None
def to_dict(self) -> dict[str, Any]:
"""Serialize to a JSON-safe dict for the ``review_verdicts`` ledger."""
return {
"verdict": self.verdict.value,
"round_index": self.round_index,
"outcome": self.outcome.value,
"findings": self.findings,
"reviewer": self.reviewer,
"created_at": self.created_at,
}
# Pluggable reviewer call: signature (prompt, **kw) -> str (the reviewer's
# text). The default shells out to the orchestrator's cross_reviewer via the
# local run.py. Leaves/tests rebind it with set_review_invoker().
ReviewInvoker = Callable[..., str]
def _orchestrator_invoker(
prompt: str,
*,
run_py: str,
config: Mapping[str, Any] | None = None,
**_kw: Any,
) -> str:
"""Default reviewer: call the orchestrator's ``cross_reviewer`` (GPT-4.1).
Invokes the local ``run.py`` with the review prompt. The orchestrator's
router sends adversarial-review tasks to ``cross_reviewer`` (GPT-4.1); this
keeps the review cross-family (a different model than the Claude planner)
and API-billed + LangSmith-traced per design §3.2. Returns the orchestrator's
stdout (the reviewer's verdict + findings).
The call is bounded by :func:`_resolve_timeout`. If ``run.py`` hangs past the
timeout the subprocess is killed and a REQUEST_CHANGES sentinel is returned
(fail CLOSED) so a stuck review cannot block the bounded loop — rather than
raising, which would crash :func:`review_node` (it does not wrap the call).
"""
if not os.path.exists(run_py):
raise FileNotFoundError(
f"orchestrator entry point not found: {run_py}; set "
f"config[{_RUN_PY_CONFIG_KEY!r}] or rebind via set_review_invoker()."
)
try:
completed = subprocess.run( # noqa: S603 - args are not shell-interpolated
["python3", run_py, prompt],
capture_output=True,
text=True,
check=False,
timeout=_resolve_timeout(config),
)
except subprocess.TimeoutExpired:
# Hung run.py -> fail CLOSED (do not block the bounded loop).
return _TIMEOUT_VERDICT_TEXT
if completed.returncode != 0:
raise RuntimeError(
"orchestrator review call failed "
f"(exit {completed.returncode}): {completed.stderr.strip()}"
)
return completed.stdout
_review_invoker: ReviewInvoker = _orchestrator_invoker
def set_review_invoker(invoker: ReviewInvoker) -> None:
"""Bind the function that performs the adversarial review call.
Leaves call this once at startup with an implementation that returns the
reviewer's text for a prompt. Keeping the call injectable keeps this node
dependency-free and unit-testable (mirrors ``billing.set_invoker``).
"""
global _review_invoker
_review_invoker = invoker
def _utcnow_iso() -> str:
"""Return the current UTC time as an ISO-8601 string."""
return datetime.now(timezone.utc).isoformat()
def _resolve_max_rounds(config: Mapping[str, Any] | None) -> int:
"""Resolve the review-round cap from config, then env, then the default.
A non-positive or non-integer value is rejected so a misconfigured cap
cannot turn the bounded loop into an unbounded one.
"""
raw: Any = None
if config is not None:
raw = config.get(_MAX_ROUNDS_CONFIG_KEY)
if raw is None:
raw = os.environ.get(_MAX_ROUNDS_ENV)
if raw is None:
return DEFAULT_MAX_REVIEW_ROUNDS
try:
value = int(raw)
except (TypeError, ValueError) as exc:
raise ValueError(
f"invalid {_MAX_ROUNDS_CONFIG_KEY!r}={raw!r}; expected a positive int"
) from exc
if value < 1:
raise ValueError(f"invalid {_MAX_ROUNDS_CONFIG_KEY!r}={value!r}; must be >= 1")
return value
def _resolve_timeout(config: Mapping[str, Any] | None) -> float:
"""Resolve the orchestrator subprocess timeout (seconds) from config/env/default.
A non-positive or non-numeric value falls back to the default so a
misconfigured knob cannot disable the bound. Mirrors the resolver in
:mod:`agent_team.nodes.review_loop_llm` so the two cannot drift.
"""
raw: Any = None
if config is not None:
raw = config.get(_TIMEOUT_CONFIG_KEY)
if raw is None:
raw = os.environ.get(_TIMEOUT_ENV)
if raw is None:
return _DEFAULT_TIMEOUT_SECONDS
try:
value = float(raw)
except (TypeError, ValueError):
return _DEFAULT_TIMEOUT_SECONDS
return value if value > 0 else _DEFAULT_TIMEOUT_SECONDS
def _resolve_run_py(config: Mapping[str, Any] | None) -> str:
"""Resolve the orchestrator ``run.py`` path from config, env, or default."""
if config is not None:
configured = config.get(_RUN_PY_CONFIG_KEY)
if configured:
return os.path.expanduser(str(configured))
env = os.environ.get("AGENT_TEAM_ORCHESTRATOR_RUN_PY")
if env:
return os.path.expanduser(env)
return _DEFAULT_RUN_PY
def parse_verdict(text: str) -> ReviewVerdict:
"""Parse a :class:`ReviewVerdict` from the reviewer's free text.
Scans for explicit ``REQUEST CHANGES`` / ``BLOCK`` tokens and ``APPROVE`` /
``LGTM`` tokens (case-insensitive, **word-boundary** matched so that prose
like "no blocking issues" inside an APPROVE does not trip a change token).
The result **fails closed**: if a change-requesting token is present, or if
neither token class is present (an ambiguous / empty review), the verdict is
``REQUEST_CHANGES`` so an unclear review never silently advances a plan to
the builders.
"""
haystack = text or ""
if _CHANGES_RE.search(haystack):
return ReviewVerdict.REQUEST_CHANGES
if _APPROVE_RE.search(haystack):
return ReviewVerdict.APPROVE
# Ambiguous / empty review -> fail closed.
return ReviewVerdict.REQUEST_CHANGES
def build_review_prompt(state: PipelineState) -> str:
"""Compose the adversarial-review prompt sent to the GPT-4.1 reviewer.
Embeds the planner's phased plan plus any prior-round findings, and asks for
a structured verdict the node can parse. Kept deterministic so the prompt is
testable and the verdict tokens line up with :func:`parse_verdict`.
"""
plan = state.get("plan") or {}
plan_text = json.dumps(plan, indent=2, sort_keys=True)
prior = state.get("review_verdicts") or []
prior_text = ""
if prior:
last = prior[-1]
if isinstance(last, Mapping):
prior_text = (
"\n\nThis plan was REVISED after prior review feedback. The "
"previous round's findings were:\n"
f"{last.get('findings', '')}\n"
"Confirm they are resolved and look for anything new."
)
return (
"You are the adversarial plan reviewer (sh-plan-review / cross_reviewer "
"discipline). Audit the following phased plan for flawed assumptions, "
"missing phases, ordering hazards, and convention violations.\n\n"
"Respond with a verdict line that is exactly one of:\n"
" VERDICT: APPROVE\n"
" VERDICT: REQUEST CHANGES\n"
"followed by your findings. Default to REQUEST CHANGES if anything is "
"unclear or risky.\n\n"
f"PLAN:\n{plan_text}"
f"{prior_text}"
)
def _is_reviewer_verdict(entry: Any) -> bool:
"""True if ``entry`` is an adversarial-REVIEWER verdict (not a human-gate one).
The plan gate folds SYNTHETIC verdicts into ``review_verdicts`` tagged
``reviewer == "human_plan_gate"`` (graph._apply_plan_decision) when the owner
requests changes. Those are human decisions, not reviewer rounds, so they are
excluded from the round count (LOGIC-04). Any entry without that tag — every
real reviewer verdict this node appends — counts as a reviewer round.
"""
if isinstance(entry, Mapping):
return entry.get("reviewer") != HUMAN_PLAN_GATE_REVIEWER
return True
def _review_round_index(state: PipelineState) -> int:
"""Return the 1-based index of the review round about to run.
Counts only prior *reviewer* verdict entries already in ``review_verdicts``
(entries this node appended), EXCLUDING the synthetic ``human_plan_gate``
verdicts the plan gate folds in on a human request_changes (LOGIC-04). So a
reviewer loop-back/re-entry increments correctly, while a human request_changes
grants the revised plan a fresh review-round budget. Termination is still
guaranteed: each human request_changes consumes one of the finite
graph.MAX_PLAN_GATE_VISITS gate visits.
"""
prior = state.get("review_verdicts") or []
reviewer_rounds = sum(1 for entry in prior if _is_reviewer_verdict(entry))
return reviewer_rounds + 1
def review_node(
state: PipelineState,
config: Mapping[str, Any] | None = None,
) -> PipelineState:
"""LangGraph node: run one adversarial review round on the current plan.
Returns a **partial** :class:`PipelineState` the reducer merges:
* ``review_verdicts`` — the prior verdicts **plus** this round's
:class:`ReviewResult` (as a dict). (Returned as the full list rather than
a single item so the node is reducer-agnostic — it works whether or not
``review_verdicts`` has an append-reducer configured.)
* ``current_phase`` / ``status`` — set per the loop decision:
- ``APPROVE`` -> phase ``BUILD``, status ``ACTIVE`` (advance to builders).
- ``REQUEST_CHANGES`` under the round cap -> phase ``PLAN``, status
``ACTIVE`` (loop back to the planner with the findings).
- ``REQUEST_CHANGES`` at/over the round cap -> phase ``PARKED``, status
``PARKED`` (escalate to Adam / ALARM; design §3.3 bound 6, §7.1 P2).
* ``updated_at`` — refreshed ISO-8601 UTC timestamp.
The plan is required; calling this node with no ``plan`` in state is a
programming error (the planner runs first) and raises ``ValueError``.
"""
if not state.get("plan"):
raise ValueError(
"review_node requires a plan in state; the planner stage must run "
"before the review loop (design §3.3 INTAKE->...->PLANNER->REVIEW)."
)
max_rounds = _resolve_max_rounds(config)
run_py = _resolve_run_py(config)
round_index = _review_round_index(state)
prompt = build_review_prompt(state)
raw = _review_invoker(prompt, run_py=run_py, config=config)
text = raw if isinstance(raw, str) else str(raw)
verdict = parse_verdict(text)
if verdict is ReviewVerdict.APPROVE:
outcome = ReviewOutcome.APPROVED
next_phase = Phase.BUILD
next_status = TaskStatus.ACTIVE
elif round_index >= max_rounds:
# Cap reached without convergence -> escalate to Adam (park + ALARM).
outcome = ReviewOutcome.ESCALATE
next_phase = Phase.PARKED
next_status = TaskStatus.PARKED
else:
outcome = ReviewOutcome.LOOP_BACK
next_phase = Phase.PLAN
next_status = TaskStatus.ACTIVE
result = ReviewResult(
verdict=verdict,
round_index=round_index,
outcome=outcome,
findings=text,
raw=raw,
created_at=_utcnow_iso(),
)
prior = list(state.get("review_verdicts") or [])
prior.append(result.to_dict())
update: PipelineState = {
"review_verdicts": prior,
"current_phase": next_phase.value,
"status": next_status.value,
"updated_at": _utcnow_iso(),
}
return update
def bind_review_node(
config: Mapping[str, Any] | None = None,
) -> Callable[[PipelineState], PipelineState]:
"""Return a **single-argument** review node bound to ``config`` (P2 wiring).
:func:`review_node` takes an optional ``config`` second argument. If it is
handed to LangGraph directly, LangGraph sees the ``config`` parameter and
injects its own ``RunnableConfig`` there, which (a) emits a typing
``UserWarning`` and (b) means the task's ``max_review_rounds`` / timeout /
``run_py`` overrides never reach the node. Wrapping it as a one-arg closure
over the intended ``config`` keeps the node free of a LangGraph-managed
``config`` param (no warning, no injection) and threads the *real* review
config through. The coordinator passes the bound node to
:func:`agent_team.graph.build_graph` as ``review_node``.
"""
def node(state: PipelineState) -> PipelineState:
return review_node(state, config)
return node
def route_after_review(state: PipelineState) -> str:
"""LangGraph conditional-edge: next node after the review loop.
Reads the outcome of the most recent verdict this node recorded and maps it
to a node id: ``APPROVED`` -> ``"build"``, ``LOOP_BACK`` -> ``"plan"``,
``ESCALATE`` -> ``"parked"``. A state with no recorded verdict is a
programming error (route is called after :func:`review_node`) and parks
fail-closed rather than advancing.
"""
verdicts = state.get("review_verdicts") or []
if not verdicts:
return PARKED_NODE
last = verdicts[-1]
outcome = last.get("outcome") if isinstance(last, Mapping) else None
if outcome == ReviewOutcome.APPROVED.value:
return BUILD_NODE
if outcome == ReviewOutcome.LOOP_BACK.value:
return PLAN_NODE
# ESCALATE or any unexpected value -> park fail-closed.
return PARKED_NODE