From 42f2438d0c25c1c2e47bd37814c4d8694daa106f Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 11:53:12 -0400 Subject: [PATCH] fix(agent-team): make planner convergence cap count correctly (LOGIC-03) _revision_count read verdict.get("decision"), but verdicts are keyed "verdict" (both reviewer and synthetic human-gate), so the count was always 0 and the plan_node MAX_PLAN_REVISIONS self-park was dead code. Now reads "verdict" first (fallback "decision"), matching _format_review_feedback's precedence; the existing .strip().upper()==_REQUEST_CHANGES compare covers both request_changes and REQUEST_CHANGES. Counts reviewer + human request_changes. Cap composition: the review-loop round cap and MAX_PLAN_GATE_VISITS govern the live loops; the planner MAX_PLAN_REVISIONS is now a correct backstop (was inert), not a behavior change to the gate. Tests drive the real "verdict" key and prove the previously-dead park fires. 1487 passed. --- agent-team/agent_team/nodes/planner.py | 13 +++++-- agent-team/tests/test_planner.py | 48 +++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/agent-team/agent_team/nodes/planner.py b/agent-team/agent_team/nodes/planner.py index 8118590..ae7d9a3 100644 --- a/agent-team/agent_team/nodes/planner.py +++ b/agent-team/agent_team/nodes/planner.py @@ -289,9 +289,16 @@ def _revision_count(state: PipelineState) -> int: """ count = 0 for verdict in state.get("review_verdicts", []): - decision = ( - verdict.get("decision") if isinstance(verdict, dict) else str(verdict) - ) + if isinstance(verdict, dict): + # Mirror _format_review_feedback's precedence: real verdicts (both + # review_loop.ReviewResult.to_dict and graph._apply_plan_decision's + # synthetic human-gate verdict) write the decision under "verdict"; + # "decision" never existed on a real verdict and is kept only as a + # defensive fallback. Reading "decision" alone is why this counter + # was always 0 (LOGIC-03). + decision = verdict.get("verdict") or verdict.get("decision") + else: + decision = str(verdict) if isinstance(decision, str) and decision.strip().upper() == _REQUEST_CHANGES: count += 1 return count diff --git a/agent-team/tests/test_planner.py b/agent-team/tests/test_planner.py index 6e670c1..bf208cc 100644 --- a/agent-team/tests/test_planner.py +++ b/agent-team/tests/test_planner.py @@ -12,6 +12,7 @@ from agent_team.billing import BillingMode, ClaudeResult from agent_team.nodes.planner import ( MAX_PLAN_REVISIONS, PlannerError, + _revision_count, build_plan_prompt, parse_plan, plan_node, @@ -256,11 +257,43 @@ def test_plan_node_garbled_reply_raises() -> None: # --------------------------------------------------------------------------- # +def test_revision_count_reads_verdict_key_real_producer_shape() -> None: + # LOGIC-03: real verdicts (review_loop.ReviewResult.to_dict and the synthetic + # human-gate verdict) write the decision under "verdict" with the lowercase + # value "request_changes". The counter must read THAT key, not "decision" + # (which never exists on a real verdict and made this counter always 0). + verdicts: list[Any] = [ + {"verdict": "request_changes", "findings": "fix it"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + assert _revision_count(_state(review_verdicts=verdicts)) == 2 + + +def test_revision_count_matches_request_changes_case_insensitively() -> None: + # Both the canonical "request_changes" (reviewer/human producers) and the + # legacy "REQUEST_CHANGES" token are counted, regardless of case; approve and + # other verdicts are ignored. + verdicts: list[Any] = [ + {"verdict": "request_changes"}, # canonical lowercase + {"verdict": "REQUEST_CHANGES"}, # legacy uppercase token + {"verdict": "Request_Changes"}, # mixed case + {"verdict": "approve"}, # must NOT count + {"verdict": "something_else"}, # must NOT count + {"decision": "request_changes"}, # defensive fallback key still counts + ] + assert _revision_count(_state(review_verdicts=verdicts)) == 4 + + +def test_revision_count_ignores_non_request_changes() -> None: + verdicts: list[Any] = [{"verdict": "approve"}] * 5 + assert _revision_count(_state(review_verdicts=verdicts)) == 0 + + def test_plan_node_replans_on_loopback_and_counts_revision() -> None: _bind_invoker(json.dumps(_VALID_PLAN)) state = _state( plan={"task": "x"}, - review_verdicts=[{"decision": "REQUEST_CHANGES", "notes": "fix it"}], + review_verdicts=[{"verdict": "request_changes", "findings": "fix it"}], ) out = plan_node(state) assert out["current_phase"] == Phase.REVIEW.value @@ -268,10 +301,15 @@ def test_plan_node_replans_on_loopback_and_counts_revision() -> None: def test_plan_node_parks_after_max_revisions() -> None: - # Invoker bound but must NOT be called once we are over the bound. + # Convergence park (ยง3.3): once the plan has been sent back + # MAX_PLAN_REVISIONS times the planner parks instead of burning more budget. + # This was DEAD CODE before the LOGIC-03 fix: _revision_count read "decision" + # while real verdicts carry "request_changes" under "verdict", so the count + # was always 0 and this branch never fired in production. Drive it with the + # REAL verdict shape to prove the park is now live. calls = _bind_invoker(json.dumps(_VALID_PLAN)) verdicts = [ - {"decision": "REQUEST_CHANGES", "notes": f"round {i}"} + {"verdict": "request_changes", "findings": f"round {i}"} for i in range(MAX_PLAN_REVISIONS) ] out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) @@ -284,7 +322,7 @@ def test_plan_node_parks_after_max_revisions() -> None: def test_plan_node_does_not_park_just_below_bound() -> None: _bind_invoker(json.dumps(_VALID_PLAN)) verdicts = [ - {"decision": "REQUEST_CHANGES", "notes": f"round {i}"} + {"verdict": "request_changes", "findings": f"round {i}"} for i in range(MAX_PLAN_REVISIONS - 1) ] out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) @@ -295,7 +333,7 @@ def test_plan_node_does_not_park_just_below_bound() -> None: def test_plan_node_ignores_non_request_changes_verdicts_for_bound() -> None: # APPROVE/other verdicts must not count toward the park bound. _bind_invoker(json.dumps(_VALID_PLAN)) - verdicts = [{"decision": "APPROVE"}] * (MAX_PLAN_REVISIONS + 2) + verdicts = [{"verdict": "approve"}] * (MAX_PLAN_REVISIONS + 2) out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) assert out["current_phase"] == Phase.REVIEW.value assert out["plan"]["revision"] == 0