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.
This commit is contained in:
parent
42f2438d0c
commit
71edeb3f3f
2 changed files with 92 additions and 3 deletions
|
|
@ -75,6 +75,17 @@ PARKED_NODE = "parked"
|
||||||
# per task via config["max_review_rounds"].
|
# per task via config["max_review_rounds"].
|
||||||
DEFAULT_MAX_REVIEW_ROUNDS = 3
|
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.
|
# Config / env key naming the per-task review-round cap.
|
||||||
_MAX_ROUNDS_CONFIG_KEY = "max_review_rounds"
|
_MAX_ROUNDS_CONFIG_KEY = "max_review_rounds"
|
||||||
_MAX_ROUNDS_ENV = "AGENT_TEAM_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:
|
def _review_round_index(state: PipelineState) -> int:
|
||||||
"""Return the 1-based index of the review round about to run.
|
"""Return the 1-based index of the review round about to run.
|
||||||
|
|
||||||
Counts only prior *review* verdict entries already in ``review_verdicts``
|
Counts only prior *reviewer* verdict entries already in ``review_verdicts``
|
||||||
(entries this node appended), so a loop-back/re-entry increments correctly.
|
(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 []
|
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(
|
def review_node(
|
||||||
|
|
|
||||||
|
|
@ -223,6 +223,64 @@ def test_review_node_appends_to_prior_verdicts() -> None:
|
||||||
assert update["review_verdicts"][-1]["round_index"] == 2
|
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
|
# review_node — invoker wiring & errors
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
|
|
|
||||||
Reference in a new issue