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.
This commit is contained in:
parent
48a81c0802
commit
42f2438d0c
2 changed files with 53 additions and 8 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Reference in a new issue