Consolidates the 18 leaf modules from the r720-plane2-scaffold workflow onto the foundation commit. Full suite: 535 passed, 1 skipped; ruff + format clean. Built (pre-deployment scaffold only — nothing provisioned/enabled): - LangGraph pipeline graph.py (INTAKE->CLARIFY->PLAN, interrupt()/resume, checkpointer-injectable) - nodes: clarifier (98% gate), planner, review_loop (GPT-4.1), builders->candidate diff, verifier - §3.3.1 HITL: ledger ops, resume_worker, deadline_timer, recovery sweep, responder - transports: slack / github / claude_code adapters - ci_gate (pure-code pass/fail), operator_cli, run-team.py entry, P1 sim harness - ci/agent-team-apply-verify.yml (split untrusted/privileged jobs) — authored, disabled KNOWN OPEN FINDINGS (verifier/cross-review, not yet fixed — see follow-up): - builders denylist: 4 execution-proven bypasses (delete, mode-change, copy-to, out-of-scope delete) - §3.3.1 CAS: BEGIN IMMEDIATE outside try/except; shared-connection txn nesting unsafe under concurrency - operator_cli: missing re-deliver/force-resume; audit-after-mutate ordering gap - ci yaml: GPT-4.1 cross-review PASS w/ 4 FIX items (symlink path escape, etc.) - P1 sim harness models the ledger layer, not real LangGraph interrupt/resume; P1 exit criteria not yet truly proven Deploy-gated (NOT done): IAM/step-ca/Roles Anywhere/confluence-bot provisioning, /sh-security-review sign-off, live Slack/CI, rsync, live dry-runs, Adam approval.
352 lines
13 KiB
Python
352 lines
13 KiB
Python
"""Unit tests for agent_team.nodes.review_loop (design §3.3, §7.1 P2)."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from typing import Any
|
|
|
|
import pytest
|
|
|
|
from agent_team.nodes import review_loop
|
|
from agent_team.nodes.review_loop import (
|
|
DEFAULT_MAX_REVIEW_ROUNDS,
|
|
ReviewOutcome,
|
|
ReviewResult,
|
|
ReviewVerdict,
|
|
build_review_prompt,
|
|
parse_verdict,
|
|
review_node,
|
|
route_after_review,
|
|
set_review_invoker,
|
|
)
|
|
from agent_team.task_model import Phase, PipelineState, TaskStatus
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _restore_invoker():
|
|
"""Restore the module review invoker after each test."""
|
|
original = review_loop._review_invoker
|
|
yield
|
|
review_loop._review_invoker = original
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _clear_env(monkeypatch: pytest.MonkeyPatch):
|
|
"""Clear review-loop env knobs so tests are hermetic."""
|
|
monkeypatch.delenv("AGENT_TEAM_MAX_REVIEW_ROUNDS", raising=False)
|
|
monkeypatch.delenv("AGENT_TEAM_ORCHESTRATOR_RUN_PY", raising=False)
|
|
|
|
|
|
def _state(**overrides: Any) -> PipelineState:
|
|
"""Build a minimal PipelineState with a plan present."""
|
|
base: PipelineState = {
|
|
"thread_id": "t1",
|
|
"plan": {"phases": ["P1", "P2"]},
|
|
"review_verdicts": [],
|
|
}
|
|
base.update(overrides) # type: ignore[typeddict-item]
|
|
return base
|
|
|
|
|
|
def _invoker_returning(text: str):
|
|
"""Return a review invoker that always yields ``text`` and records calls."""
|
|
calls: list[dict[str, Any]] = []
|
|
|
|
def invoker(prompt: str, **kw: Any) -> str:
|
|
calls.append({"prompt": prompt, "kw": kw})
|
|
return text
|
|
|
|
invoker.calls = calls # type: ignore[attr-defined]
|
|
return invoker
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# parse_verdict
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_parse_verdict_approve() -> None:
|
|
assert parse_verdict("VERDICT: APPROVE\nlooks good") is ReviewVerdict.APPROVE
|
|
assert parse_verdict("LGTM, ship it") is ReviewVerdict.APPROVE
|
|
|
|
|
|
def test_parse_verdict_request_changes() -> None:
|
|
assert (
|
|
parse_verdict("VERDICT: REQUEST CHANGES\nmissing rollback")
|
|
is ReviewVerdict.REQUEST_CHANGES
|
|
)
|
|
assert parse_verdict("BLOCK: unsafe IAM policy") is ReviewVerdict.REQUEST_CHANGES
|
|
|
|
|
|
def test_parse_verdict_is_case_insensitive() -> None:
|
|
assert parse_verdict("verdict: approve") is ReviewVerdict.APPROVE
|
|
|
|
|
|
def test_parse_verdict_fails_closed_on_ambiguous() -> None:
|
|
# Neither token present -> REQUEST_CHANGES (fail closed).
|
|
assert parse_verdict("hmm, not sure") is ReviewVerdict.REQUEST_CHANGES
|
|
assert parse_verdict("") is ReviewVerdict.REQUEST_CHANGES
|
|
assert parse_verdict(None) is ReviewVerdict.REQUEST_CHANGES # type: ignore[arg-type]
|
|
|
|
|
|
def test_parse_verdict_request_changes_wins_on_conflict() -> None:
|
|
# Both tokens present -> REQUEST_CHANGES wins (fail closed).
|
|
text = "Some phases APPROVE-able but VERDICT: REQUEST CHANGES overall"
|
|
assert parse_verdict(text) is ReviewVerdict.REQUEST_CHANGES
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# build_review_prompt
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_build_review_prompt_embeds_plan() -> None:
|
|
prompt = build_review_prompt(_state(plan={"phases": ["alpha"]}))
|
|
assert "alpha" in prompt
|
|
assert "VERDICT: APPROVE" in prompt
|
|
assert "VERDICT: REQUEST CHANGES" in prompt
|
|
|
|
|
|
def test_build_review_prompt_includes_prior_findings() -> None:
|
|
state = _state(
|
|
review_verdicts=[{"verdict": "request_changes", "findings": "rollback missing"}]
|
|
)
|
|
prompt = build_review_prompt(state)
|
|
assert "rollback missing" in prompt
|
|
assert "REVISED" in prompt
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# review_node — APPROVE path
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_review_node_approve_advances_to_build() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: APPROVE\nsolid plan"))
|
|
update = review_node(_state())
|
|
|
|
assert update["current_phase"] == Phase.BUILD.value
|
|
assert update["status"] == TaskStatus.ACTIVE.value
|
|
assert update["updated_at"]
|
|
|
|
verdicts = update["review_verdicts"]
|
|
assert len(verdicts) == 1
|
|
assert verdicts[0]["verdict"] == ReviewVerdict.APPROVE.value
|
|
assert verdicts[0]["outcome"] == ReviewOutcome.APPROVED.value
|
|
assert verdicts[0]["round_index"] == 1
|
|
assert verdicts[0]["reviewer"] == "cross_reviewer"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# review_node — REQUEST_CHANGES loop-back vs escalate
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_review_node_request_changes_loops_back_under_cap() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nfix it"))
|
|
update = review_node(_state(), config={"max_review_rounds": 3})
|
|
|
|
assert update["current_phase"] == Phase.PLAN.value
|
|
assert update["status"] == TaskStatus.ACTIVE.value
|
|
assert update["review_verdicts"][-1]["outcome"] == ReviewOutcome.LOOP_BACK.value
|
|
|
|
|
|
def test_review_node_escalates_at_round_cap() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nstill broken"))
|
|
# Two prior REQUEST_CHANGES rounds already recorded; cap is 3 -> this is
|
|
# round 3 -> escalate.
|
|
state = _state(
|
|
review_verdicts=[
|
|
{"verdict": "request_changes", "outcome": "loop_back"},
|
|
{"verdict": "request_changes", "outcome": "loop_back"},
|
|
]
|
|
)
|
|
update = review_node(state, config={"max_review_rounds": 3})
|
|
|
|
assert update["current_phase"] == Phase.PARKED.value
|
|
assert update["status"] == TaskStatus.PARKED.value
|
|
last = update["review_verdicts"][-1]
|
|
assert last["outcome"] == ReviewOutcome.ESCALATE.value
|
|
assert last["round_index"] == 3
|
|
|
|
|
|
def test_review_node_approve_at_cap_still_advances() -> None:
|
|
# Even at the round cap, an APPROVE advances to build (cap only bounds
|
|
# REQUEST_CHANGES looping).
|
|
set_review_invoker(_invoker_returning("VERDICT: APPROVE"))
|
|
state = _state(
|
|
review_verdicts=[
|
|
{"verdict": "request_changes", "outcome": "loop_back"},
|
|
{"verdict": "request_changes", "outcome": "loop_back"},
|
|
]
|
|
)
|
|
update = review_node(state, config={"max_review_rounds": 3})
|
|
assert update["current_phase"] == Phase.BUILD.value
|
|
assert update["review_verdicts"][-1]["outcome"] == ReviewOutcome.APPROVED.value
|
|
|
|
|
|
def test_review_node_appends_to_prior_verdicts() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES"))
|
|
state = _state(
|
|
review_verdicts=[{"verdict": "request_changes", "outcome": "loop_back"}]
|
|
)
|
|
update = review_node(state, config={"max_review_rounds": 5})
|
|
assert len(update["review_verdicts"]) == 2
|
|
assert update["review_verdicts"][-1]["round_index"] == 2
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# review_node — invoker wiring & errors
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_review_node_passes_prompt_and_config_to_invoker() -> None:
|
|
invoker = _invoker_returning("VERDICT: APPROVE")
|
|
set_review_invoker(invoker)
|
|
cfg = {"max_review_rounds": 2, "orchestrator_run_py": "/tmp/run.py"}
|
|
review_node(_state(plan={"phases": ["zeta"]}), config=cfg)
|
|
|
|
assert len(invoker.calls) == 1 # type: ignore[attr-defined]
|
|
call = invoker.calls[0] # type: ignore[attr-defined]
|
|
assert "zeta" in call["prompt"]
|
|
assert call["kw"]["run_py"] == "/tmp/run.py"
|
|
assert call["kw"]["config"] is cfg
|
|
|
|
|
|
def test_review_node_requires_plan() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: APPROVE"))
|
|
with pytest.raises(ValueError):
|
|
review_node({"thread_id": "t1", "review_verdicts": []})
|
|
|
|
|
|
def test_review_node_rejects_bad_round_cap() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: APPROVE"))
|
|
with pytest.raises(ValueError):
|
|
review_node(_state(), config={"max_review_rounds": 0})
|
|
with pytest.raises(ValueError):
|
|
review_node(_state(), config={"max_review_rounds": "lots"})
|
|
|
|
|
|
def test_review_node_uses_env_round_cap(monkeypatch: pytest.MonkeyPatch) -> None:
|
|
monkeypatch.setenv("AGENT_TEAM_MAX_REVIEW_ROUNDS", "1")
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES"))
|
|
# Cap 1 from env -> first REQUEST_CHANGES round escalates immediately.
|
|
update = review_node(_state())
|
|
assert update["status"] == TaskStatus.PARKED.value
|
|
|
|
|
|
def test_review_node_default_cap_is_three() -> None:
|
|
assert DEFAULT_MAX_REVIEW_ROUNDS == 3
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES"))
|
|
# Rounds 1 and 2 loop back under the default cap of 3.
|
|
state = _state(
|
|
review_verdicts=[{"verdict": "request_changes", "outcome": "loop_back"}]
|
|
)
|
|
update = review_node(state) # round 2
|
|
assert update["current_phase"] == Phase.PLAN.value
|
|
|
|
|
|
def test_review_node_coerces_non_string_invoker_output() -> None:
|
|
set_review_invoker(lambda prompt, **kw: 12345) # type: ignore[return-value]
|
|
# No verdict token in "12345" -> fails closed to REQUEST_CHANGES.
|
|
update = review_node(_state(), config={"max_review_rounds": 3})
|
|
assert (
|
|
update["review_verdicts"][-1]["verdict"] == ReviewVerdict.REQUEST_CHANGES.value
|
|
)
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# route_after_review
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_route_after_review_approved_to_build() -> None:
|
|
state: PipelineState = {"review_verdicts": [{"outcome": "approved"}]}
|
|
assert route_after_review(state) == "build"
|
|
|
|
|
|
def test_route_after_review_loop_back_to_plan() -> None:
|
|
state: PipelineState = {"review_verdicts": [{"outcome": "loop_back"}]}
|
|
assert route_after_review(state) == "plan"
|
|
|
|
|
|
def test_route_after_review_escalate_to_parked() -> None:
|
|
state: PipelineState = {"review_verdicts": [{"outcome": "escalate"}]}
|
|
assert route_after_review(state) == "parked"
|
|
|
|
|
|
def test_route_after_review_no_verdict_parks_fail_closed() -> None:
|
|
assert route_after_review({"review_verdicts": []}) == "parked"
|
|
assert route_after_review({}) == "parked"
|
|
|
|
|
|
def test_route_after_review_unexpected_outcome_parks() -> None:
|
|
state: PipelineState = {"review_verdicts": [{"outcome": "weird"}]}
|
|
assert route_after_review(state) == "parked"
|
|
|
|
|
|
def test_route_uses_most_recent_verdict() -> None:
|
|
state: PipelineState = {
|
|
"review_verdicts": [{"outcome": "loop_back"}, {"outcome": "approved"}]
|
|
}
|
|
assert route_after_review(state) == "build"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# node + router integration (the full loop decision)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_node_then_route_round_trip_approve() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: APPROVE"))
|
|
update = review_node(_state())
|
|
# Merge update back into state (LangGraph reducer would do this).
|
|
state = {**_state(), **update}
|
|
assert route_after_review(state) == "build"
|
|
|
|
|
|
def test_node_then_route_round_trip_loop_back() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES"))
|
|
update = review_node(_state(), config={"max_review_rounds": 3})
|
|
state = {**_state(), **update}
|
|
assert route_after_review(state) == "plan"
|
|
|
|
|
|
def test_node_then_route_round_trip_escalate() -> None:
|
|
set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES"))
|
|
update = review_node(_state(), config={"max_review_rounds": 1})
|
|
state = {**_state(), **update}
|
|
assert route_after_review(state) == "parked"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# ReviewResult serialization
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_review_result_to_dict_round_trips_fields() -> None:
|
|
result = ReviewResult(
|
|
verdict=ReviewVerdict.APPROVE,
|
|
round_index=2,
|
|
outcome=ReviewOutcome.APPROVED,
|
|
findings="all good",
|
|
created_at="2026-06-17T00:00:00+00:00",
|
|
)
|
|
d = result.to_dict()
|
|
assert d == {
|
|
"verdict": "approve",
|
|
"round_index": 2,
|
|
"outcome": "approved",
|
|
"findings": "all good",
|
|
"reviewer": "cross_reviewer",
|
|
"created_at": "2026-06-17T00:00:00+00:00",
|
|
}
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# default invoker (orchestrator shell-out) — error surface only, no real call
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_default_invoker_missing_run_py_raises() -> None:
|
|
with pytest.raises(FileNotFoundError):
|
|
review_loop._orchestrator_invoker("prompt", run_py="/nonexistent/path/run.py")
|