fix(agent-team): plan-gate approve routes into the build subgraph (not END)
Discovered while smoke-testing the #60 builder fix on the R720: a task that reaches the human plan-gate and is APPROVED never built. The gate's GATE_APPROVE_ROUTE was hard-wired to END in build_graph, so plan_gate_node set phase=BUILD/status=ACTIVE and the graph terminated WITHOUT entering the build subgraph — the task wedged at phase=build with no build, no error. Only the reviewer's auto-approve path (review -> build_node) reached the builder; every human-gate-approved plan silently dead-ended. The P3 splice only repoints the REVIEW node's build route to BUILD_NODE; the plan_gate edges are independent and were never updated, so even with P3 fully wired the gate approve went to END. _apply_plan_decision's 'settle exactly as an auto-approved plan' intent was broken by the edge map. - graph.py: when build_verify (P3) is wired, point GATE_APPROVE_ROUTE at BUILD_NODE (mirroring REVIEW's BUILD_ROUTE: BUILD_NODE); keep END when P3 is inert (P2 approved-plan terminus). LangGraph resolves the forward reference to BUILD_NODE at compile(). - tests: gate-approve with P3 wired traverses BUILD -> VERIFY -> DONE on an authenticated CI pass, and parks at VERIFY (never fabricates a pass) when the CI fetcher is inert. Existing P2 gate-approve -> END behavior unchanged.
This commit is contained in:
parent
21f2fe54c1
commit
e42b910a80
2 changed files with 90 additions and 3 deletions
|
|
@ -526,7 +526,8 @@ def route_after_plan_gate(state: PipelineState) -> str:
|
|||
Reads the routing state :func:`plan_gate_node` wrote on resume (or on the
|
||||
ceiling-reached terminal park) and maps it to a route id:
|
||||
|
||||
* status ACTIVE + phase BUILD -> :data:`GATE_APPROVE_ROUTE` (END/approved);
|
||||
* status ACTIVE + phase BUILD -> :data:`GATE_APPROVE_ROUTE` (BUILD_NODE when
|
||||
the P3 build subgraph is wired, else the approved-plan END terminus);
|
||||
* status ACTIVE + phase PLAN -> :data:`GATE_REVISE_ROUTE` (loop to planner);
|
||||
* anything else (FAILED, or PARKED ceiling) -> :data:`GATE_TERMINAL_ROUTE`.
|
||||
"""
|
||||
|
|
@ -770,9 +771,17 @@ def build_graph(
|
|||
# the plan gate is wired it is REPOINTED at the PLAN_GATE vertex instead:
|
||||
# the cap dead-end suspends on a resumable human decision rather than
|
||||
# terminally parking. The gate's own conditional edges then route
|
||||
# approve -> END, request_changes -> PLAN (loop back), terminal -> END.
|
||||
# request_changes -> PLAN (loop back), terminal -> END, and approve ->
|
||||
# the SAME target the reviewer's auto-approve "build" route reaches:
|
||||
# BUILD_NODE when the P3 build subgraph is wired, else END (the P2
|
||||
# approved-plan terminus). Pointing approve at END unconditionally was a
|
||||
# bug — a human-approved-at-gate plan settled at phase BUILD without ever
|
||||
# entering the builder, so it never built (issue #60 sibling). BUILD_NODE
|
||||
# is added below in the build_verify branch; LangGraph resolves the
|
||||
# forward reference at compile().
|
||||
if plan_gate:
|
||||
parked_target = PLAN_GATE
|
||||
gate_approve_target = BUILD_NODE if build_verify is not None else END
|
||||
builder.add_node(
|
||||
PLAN_GATE,
|
||||
_instrument(PLAN_GATE, plan_gate_node, transition_recorder),
|
||||
|
|
@ -781,7 +790,7 @@ def build_graph(
|
|||
PLAN_GATE,
|
||||
route_after_plan_gate,
|
||||
{
|
||||
GATE_APPROVE_ROUTE: END,
|
||||
GATE_APPROVE_ROUTE: gate_approve_target,
|
||||
GATE_REVISE_ROUTE: PLAN,
|
||||
GATE_TERMINAL_ROUTE: END,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -587,6 +587,84 @@ def test_plan_gate_approve_settles_as_approved_plan(restore_review_invoker) -> N
|
|||
assert final["status"] == TaskStatus.ACTIVE.value
|
||||
|
||||
|
||||
def _p3_plan_gate_graph(review_text: str, *, ci_result_fetcher):
|
||||
"""Compile a graph with BOTH the plan gate AND the P3 build->verify subgraph.
|
||||
|
||||
The reviewer (``review_text``) never approves, so the plan<->review loop hits
|
||||
the cap and suspends on the human PLAN_GATE; a human ``approve`` must then
|
||||
route into the build subgraph (BUILD -> VERIFY), exactly as the reviewer's
|
||||
own auto-approve "build" route does.
|
||||
"""
|
||||
from agent_team.nodes import review_loop
|
||||
from agent_team.nodes.build_verify_subgraph import (
|
||||
make_build_node,
|
||||
make_verify_node,
|
||||
route_after_verify,
|
||||
)
|
||||
from agent_team.nodes.verifier import VerifierConfig
|
||||
|
||||
review_loop.set_review_invoker(lambda prompt, **kw: review_text)
|
||||
|
||||
def fake_builder(*, plan, config):
|
||||
return _p3_diff()
|
||||
|
||||
build_node = make_build_node(diff_builder=fake_builder)
|
||||
verify_node = make_verify_node(
|
||||
VerifierConfig(expected_run_id="r1", allowed_scope=["src"]),
|
||||
ci_result_fetcher=ci_result_fetcher,
|
||||
)
|
||||
return build_graph(
|
||||
checkpointer=_Saver(),
|
||||
live_plan_node=_p3_plan_stub,
|
||||
review_node=review_loop.bind_review_node(),
|
||||
route_review=review_loop.route_after_review,
|
||||
plan_gate=True,
|
||||
build_verify=(build_node, verify_node, route_after_verify),
|
||||
)
|
||||
|
||||
|
||||
def test_plan_gate_approve_enters_build_subgraph_when_p3_wired(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# Regression for the gate sibling of issue #60: a human approve at the plan
|
||||
# gate MUST route into the P3 build subgraph (BUILD -> VERIFY), not dead-end
|
||||
# at END leaving the task stuck at phase=build. The reviewer never approves,
|
||||
# so the loop caps to the human gate; an authenticated CI pass then carries
|
||||
# build -> verify -> DONE — proving the approve edge reached BUILD_NODE.
|
||||
def pass_fetcher(state):
|
||||
from agent_team.state_store import compute_content_hash
|
||||
|
||||
diff_hash = compute_content_hash(_p3_diff().encode("utf-8"))
|
||||
return {"run_id": "r1", "conclusion": "success", "diff_hash": diff_hash}
|
||||
|
||||
graph = _p3_plan_gate_graph(
|
||||
"VERDICT: REQUEST CHANGES\nnot yet", ci_result_fetcher=pass_fetcher
|
||||
)
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
final = resume_task(graph, thread_id=thread_id, answer={"decision": "approve"})
|
||||
|
||||
# Reached the build->verify PASS terminus, NOT a phase=build dead-end.
|
||||
assert final["current_phase"] == Phase.DONE.value
|
||||
assert final["status"] == TaskStatus.DONE.value
|
||||
|
||||
|
||||
def test_plan_gate_approve_inert_p3_still_settles_at_approved_terminus(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# With the P3 build subgraph wired but its CI fetcher INERT (no authenticated
|
||||
# result), a gate approve enters build->verify and parks at VERIFY (the
|
||||
# production-safe default) — it must NOT fabricate a pass. Confirms the
|
||||
# approve edge routes through the subgraph, not to END.
|
||||
graph = _p3_plan_gate_graph(
|
||||
"VERDICT: REQUEST CHANGES\nnot yet", ci_result_fetcher=lambda state: None
|
||||
)
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
final = resume_task(graph, thread_id=thread_id, answer={"decision": "approve"})
|
||||
|
||||
assert final["current_phase"] == Phase.PARKED.value
|
||||
assert final["status"] == TaskStatus.PARKED.value
|
||||
|
||||
|
||||
def test_plan_gate_request_changes_loops_back_with_notes(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
|
|
|
|||
Reference in a new issue