diff --git a/agent-team/agent_team/nodes/review_loop.py b/agent-team/agent_team/nodes/review_loop.py index d61e959..aa1061d 100644 --- a/agent-team/agent_team/nodes/review_loop.py +++ b/agent-team/agent_team/nodes/review_loop.py @@ -75,6 +75,17 @@ PARKED_NODE = "parked" # 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" @@ -371,14 +382,34 @@ def build_review_prompt(state: PipelineState) -> str: ) +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 *review* verdict entries already in ``review_verdicts`` - (entries this node appended), so a loop-back/re-entry increments correctly. + 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 [] - return len(prior) + 1 + reviewer_rounds = sum(1 for entry in prior if _is_reviewer_verdict(entry)) + return reviewer_rounds + 1 def review_node( diff --git a/agent-team/tests/test_review_loop.py b/agent-team/tests/test_review_loop.py index 5870d54..e060d42 100644 --- a/agent-team/tests/test_review_loop.py +++ b/agent-team/tests/test_review_loop.py @@ -223,6 +223,64 @@ def test_review_node_appends_to_prior_verdicts() -> None: assert update["review_verdicts"][-1]["round_index"] == 2 +# --------------------------------------------------------------------------- # +# review_node — human-gate verdicts excluded from the round cap (LOGIC-04) +# --------------------------------------------------------------------------- # + + +def test_review_round_index_excludes_human_plan_gate_verdicts() -> None: + # A human request_changes folds a synthetic verdict tagged + # reviewer == "human_plan_gate" into review_verdicts. It must NOT count as a + # reviewer round: with one reviewer round + two human-gate verdicts present, + # the next reviewer round is still round 2 (not round 4). + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "outcome": "loop_back"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + ) + assert review_loop._review_round_index(state) == 2 + + +def test_review_node_human_gate_verdict_grants_fresh_round() -> None: + # After a human request_changes (synthetic human_plan_gate verdict), the + # revised plan's review gets a fresh round rather than immediate escalation, + # even though len(review_verdicts) would otherwise be at the cap. + set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nstill rough")) + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "outcome": "loop_back"}, # reviewer r1 + {"verdict": "request_changes", "outcome": "loop_back"}, # reviewer r2 + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, # human + ] + ) + update = review_node(state, config={"max_review_rounds": 3}) + + # Only 2 reviewer rounds counted, so this is reviewer round 3 == cap: it + # escalates here, NOT one round early because of the human verdict. + last = update["review_verdicts"][-1] + assert last["round_index"] == 3 + assert last["outcome"] == ReviewOutcome.ESCALATE.value + + +def test_review_node_only_human_verdicts_is_fresh_first_round() -> None: + # With NO prior reviewer verdicts but a human-gate verdict present, the very + # first reviewer pass is round 1 and loops back under the cap (the bug made + # this escalate immediately at cap 1). + set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nredo")) + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + ) + update = review_node(state, config={"max_review_rounds": 2}) + + assert update["current_phase"] == Phase.PLAN.value + assert update["status"] == TaskStatus.ACTIVE.value + assert update["review_verdicts"][-1]["round_index"] == 1 + + # --------------------------------------------------------------------------- # # review_node — invoker wiring & errors # --------------------------------------------------------------------------- #