From c749d87518e3dc95335f817e46674a1f0b353eed Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 14:53:21 -0400 Subject: [PATCH] fix(pipeline): single-shot Claude calls + planner actually reads review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two root causes behind 'every task parks, and slowly': 1. SINGLE-SHOT INVOKER: subscription Claude calls ran as 40-turn, tool-enabled agentic sessions (--max-turns 40, $2 budget) for what are pure reasoning->JSON completions — minutes-long, and Claude wandered/returned unparseable output. _DEFAULT_MAX_TURNS 40->1 + allowed_tools=[] -> fast deterministic single turn. 2. PLANNER FEEDBACK KEY MISMATCH: the review stage writes verdict/findings, but _format_review_feedback read decision/notes/comment (never present) -> the planner re-planned with EMPTY feedback, re-introduced the rejected flaw ('assumptions persist'), hit the review cap, parked. Now reads verdict/findings (old keys kept as fallback) so GPT-4.1's objections reach the re-plan. Plus: park notifications infer the phase the task was IN (review/plan/clarify) instead of the terminal 'parked'. Tests: planner real-verdict-keys regression + coordinator phase-inference. 1149 pass. --- agent-team/agent_team/coordinator.py | 10 +++++++++ agent-team/agent_team/invoker.py | 12 ++++++++++- agent-team/agent_team/nodes/planner.py | 17 +++++++++++++-- agent-team/tests/test_coordinator.py | 30 ++++++++++++++++++++++++++ agent-team/tests/test_planner.py | 25 +++++++++++++++++++-- 5 files changed, 89 insertions(+), 5 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index 061421a..3500878 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -827,6 +827,16 @@ class Coordinator: # and say WHERE it got to and WHAT is blocking it. status = values.get("status") phase = str(values.get("current_phase") or "unknown") + # When a task parks, current_phase is the terminal "parked" — not + # useful. Infer the phase it was IN when it escalated, so the human + # sees WHERE it died (review > plan > clarify by what state exists). + if phase == "parked": + if values.get("review_verdicts"): + phase = "review" + elif values.get("plan"): + phase = "plan" + else: + phase = "clarify" if status == TaskStatus.PARKED.value: blocker = self._summarize_blocker(values) self._emit( diff --git a/agent-team/agent_team/invoker.py b/agent-team/agent_team/invoker.py index 58d11b3..5778173 100644 --- a/agent-team/agent_team/invoker.py +++ b/agent-team/agent_team/invoker.py @@ -49,7 +49,13 @@ API_MODEL = "claude-sonnet-4-6" # Default per-call agent budget for the headless subscription path, in USD. _DEFAULT_BUDGET_USD = 2.0 -_DEFAULT_MAX_TURNS = 40 +# The agent-team's subscription Claude calls (clarify confidence/questions, +# planner) are SINGLE-SHOT reasoning→JSON completions, NOT agentic sessions. A +# 40-turn, tool-enabled session made each call take minutes and let Claude wander +# (use tools / explore) and return output the planner/clarifier couldn't parse → +# spurious parks. One turn + no tools = a fast, deterministic completion. A +# genuinely agentic caller (e.g. a future fixer) overrides max_turns/allowed_tools. +_DEFAULT_MAX_TURNS = 1 # --------------------------------------------------------------------------- # @@ -104,6 +110,10 @@ async def _collect_subscription_text( model=model, max_turns=max_turns, max_budget_usd=budget_usd, + # No tools: these are pure reasoning→JSON completions. Disallowing tools + # keeps the call a single deterministic turn (no repo exploration / tool + # loops that produce slow, unparseable output). + allowed_tools=[], ) texts: list[str] = [] diff --git a/agent-team/agent_team/nodes/planner.py b/agent-team/agent_team/nodes/planner.py index db11cbf..6d55b65 100644 --- a/agent-team/agent_team/nodes/planner.py +++ b/agent-team/agent_team/nodes/planner.py @@ -122,8 +122,21 @@ def _format_review_feedback(review_verdicts: list[Any]) -> str: lines: list[str] = [] for idx, verdict in enumerate(review_verdicts, start=1): if isinstance(verdict, dict): - decision = str(verdict.get("decision", "")).strip() - notes = str(verdict.get("notes") or verdict.get("comment") or "").strip() + # The review stage (review_loop.ReviewResult.to_dict) writes the keys + # "verdict" (decision) and "findings" (the reviewer's objections). + # Read those FIRST — the old "decision"/"notes"/"comment" keys never + # existed on a real verdict, so the planner re-planned with EMPTY + # feedback and kept re-introducing the rejected flaw ("assumptions + # persist" → cap → park). Old keys kept as fallbacks for safety. + decision = str( + verdict.get("verdict") or verdict.get("decision") or "" + ).strip() + notes = str( + verdict.get("findings") + or verdict.get("notes") + or verdict.get("comment") + or "" + ).strip() lines.append(f"Review {idx} [{decision}]: {notes}".rstrip()) else: lines.append(f"Review {idx}: {str(verdict).strip()}") diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py index ccb9e03..66a6d2b 100644 --- a/agent-team/tests/test_coordinator.py +++ b/agent-team/tests/test_coordinator.py @@ -1047,3 +1047,33 @@ def test_parked_message_includes_description_and_blocker( assert "add a smoke-test file" in m # WHAT the task is assert "review" in m # WHERE it got to assert "Missing a final review/commit phase" in m # WHY it's blocked + + +def test_parked_message_infers_phase_when_current_phase_is_parked( + db_path: Path, monkeypatch: Any +) -> None: + # current_phase is the terminal "parked"; the message should report the phase + # the task was IN (review, since verdicts exist), not "parked". + from agent_team import coordinator as coord_mod + from agent_team.task_model import TaskStatus + + msgs: list[str] = [] + coord = _make_coordinator(db_path) + coord._notify = msgs.append + coord.setup() + monkeypatch.setattr( + coord_mod.graph_mod, "pending_question", lambda _g, *, thread_id: None + ) + + class _Snap: + values = { + "status": TaskStatus.PARKED.value, + "current_phase": "parked", + "task": "do a thing", + "review_verdicts": [{"verdict": "request_changes", "findings": "nope"}], + } + + monkeypatch.setattr(coord._graph, "get_state", lambda _cfg: _Snap()) + coord._post_resume_followups([_resume_result("abcd1234ef00")]) + assert "Reached phase: review" in msgs[0] + assert "Reached phase: parked" not in msgs[0] diff --git a/agent-team/tests/test_planner.py b/agent-team/tests/test_planner.py index 4b6c39a..988ddad 100644 --- a/agent-team/tests/test_planner.py +++ b/agent-team/tests/test_planner.py @@ -172,19 +172,40 @@ def test_build_prompt_no_task_uses_placeholder() -> None: def test_build_prompt_loopback_includes_feedback_and_prior_plan() -> None: + # Use the REAL verdict shape that review_loop.ReviewResult.to_dict() writes + # ("verdict" + "findings") — NOT the old "decision"/"notes" keys, which never + # existed on a real verdict and silently produced empty re-plan feedback. state = _state( plan={"task": "t", "phases": [{"name": "old", "steps": ["x"]}]}, review_verdicts=[ - {"decision": "REQUEST_CHANGES", "notes": "Phase 1 missing rollback."} + { + "verdict": "request_changes", + "outcome": "loop_back", + "round_index": 1, + "findings": "Phase 1 missing rollback.", + } ], ) prompt = build_plan_prompt(state) assert "Reviewer feedback" in prompt - assert "Phase 1 missing rollback." in prompt + assert "Phase 1 missing rollback." in prompt # the findings reached the re-plan assert "Previous plan" in prompt assert '"old"' in prompt +def test_review_feedback_reads_real_verdict_keys() -> None: + # Regression: the planner must read the producer's keys (verdict/findings). + # The bug read decision/notes/comment -> empty feedback -> the planner kept + # re-introducing the rejected flaw -> review cap -> park. + from agent_team.nodes.planner import _format_review_feedback + + rendered = _format_review_feedback( + [{"verdict": "request_changes", "findings": "Add a teardown fixture."}] + ) + assert "Add a teardown fixture." in rendered + assert "request_changes" in rendered + + def test_build_prompt_no_feedback_omits_review_sections() -> None: prompt = build_plan_prompt(_state(plan={"task": "t"})) assert "Reviewer feedback" not in prompt