diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index 3615030..f9affdc 100644 --- a/agent-team/agent_team/graph.py +++ b/agent-team/agent_team/graph.py @@ -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, }, diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index d7a13ae..7e86e0f 100644 --- a/agent-team/tests/test_graph.py +++ b/agent-team/tests/test_graph.py @@ -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: