fix(pipeline): single-shot Claude calls + planner actually reads review findings
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.
This commit is contained in:
parent
655f6d80f4
commit
e7ef4387e6
5 changed files with 89 additions and 5 deletions
|
|
@ -827,6 +827,16 @@ class Coordinator:
|
||||||
# and say WHERE it got to and WHAT is blocking it.
|
# and say WHERE it got to and WHAT is blocking it.
|
||||||
status = values.get("status")
|
status = values.get("status")
|
||||||
phase = str(values.get("current_phase") or "unknown")
|
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:
|
if status == TaskStatus.PARKED.value:
|
||||||
blocker = self._summarize_blocker(values)
|
blocker = self._summarize_blocker(values)
|
||||||
self._emit(
|
self._emit(
|
||||||
|
|
|
||||||
|
|
@ -49,7 +49,13 @@ API_MODEL = "claude-sonnet-4-6"
|
||||||
|
|
||||||
# Default per-call agent budget for the headless subscription path, in USD.
|
# Default per-call agent budget for the headless subscription path, in USD.
|
||||||
_DEFAULT_BUDGET_USD = 2.0
|
_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,
|
model=model,
|
||||||
max_turns=max_turns,
|
max_turns=max_turns,
|
||||||
max_budget_usd=budget_usd,
|
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] = []
|
texts: list[str] = []
|
||||||
|
|
|
||||||
|
|
@ -122,8 +122,21 @@ def _format_review_feedback(review_verdicts: list[Any]) -> str:
|
||||||
lines: list[str] = []
|
lines: list[str] = []
|
||||||
for idx, verdict in enumerate(review_verdicts, start=1):
|
for idx, verdict in enumerate(review_verdicts, start=1):
|
||||||
if isinstance(verdict, dict):
|
if isinstance(verdict, dict):
|
||||||
decision = str(verdict.get("decision", "")).strip()
|
# The review stage (review_loop.ReviewResult.to_dict) writes the keys
|
||||||
notes = str(verdict.get("notes") or verdict.get("comment") or "").strip()
|
# "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())
|
lines.append(f"Review {idx} [{decision}]: {notes}".rstrip())
|
||||||
else:
|
else:
|
||||||
lines.append(f"Review {idx}: {str(verdict).strip()}")
|
lines.append(f"Review {idx}: {str(verdict).strip()}")
|
||||||
|
|
|
||||||
|
|
@ -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 "add a smoke-test file" in m # WHAT the task is
|
||||||
assert "review" in m # WHERE it got to
|
assert "review" in m # WHERE it got to
|
||||||
assert "Missing a final review/commit phase" in m # WHY it's blocked
|
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]
|
||||||
|
|
|
||||||
|
|
@ -172,19 +172,40 @@ def test_build_prompt_no_task_uses_placeholder() -> None:
|
||||||
|
|
||||||
|
|
||||||
def test_build_prompt_loopback_includes_feedback_and_prior_plan() -> 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(
|
state = _state(
|
||||||
plan={"task": "t", "phases": [{"name": "old", "steps": ["x"]}]},
|
plan={"task": "t", "phases": [{"name": "old", "steps": ["x"]}]},
|
||||||
review_verdicts=[
|
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)
|
prompt = build_plan_prompt(state)
|
||||||
assert "Reviewer feedback" in prompt
|
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 "Previous plan" in prompt
|
||||||
assert '"old"' 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:
|
def test_build_prompt_no_feedback_omits_review_sections() -> None:
|
||||||
prompt = build_plan_prompt(_state(plan={"task": "t"}))
|
prompt = build_plan_prompt(_state(plan={"task": "t"}))
|
||||||
assert "Reviewer feedback" not in prompt
|
assert "Reviewer feedback" not in prompt
|
||||||
|
|
|
||||||
Reference in a new issue