From 21f2fe54c16f3a37d753cdbd2a25677331b6091a Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 13:13:28 -0400 Subject: [PATCH] fix(agent-team): builder uses agentic invoker config (read-only tools + turn headroom) The Plane-2 builder (default_diff_builder) called claude_invoke with no overrides, inheriting the subscription invoker's single-shot defaults (max_turns=1, allowed_tools=[]). Diff synthesis is agentic, so the call died with 'Reached maximum number of turns (1)' and every task failed at phase=build. - invoker.py: thread allowed_tools through subscription_invoker and _collect_subscription_text (default None -> []), so callers can opt in; single-shot reasoning nodes are unchanged. - builders.py: default_diff_builder now passes max_turns=8, a read-only tool allowlist (Read/Grep/Glob), and budget_usd=4.0. No write tools -- the builder returns the diff as data and performs no repo writes (D2/D11). - Tests: builder agentic-config passthrough; invoker allowed_tools thread + tool-less default guard (so future nodes must opt in explicitly). Closes #60 --- agent-team/agent_team/invoker.py | 14 +++++--- agent-team/agent_team/nodes/builders.py | 24 +++++++++++++- agent-team/tests/test_builders.py | 24 ++++++++++++++ agent-team/tests/test_invoker.py | 44 +++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 5 deletions(-) diff --git a/agent-team/agent_team/invoker.py b/agent-team/agent_team/invoker.py index f52dadc..18155b3 100644 --- a/agent-team/agent_team/invoker.py +++ b/agent-team/agent_team/invoker.py @@ -89,6 +89,7 @@ async def _collect_subscription_text( max_turns: int, budget_usd: float, model: str | None, + allowed_tools: list[str] | None = None, _query: Callable[..., Any] | None = None, _options_cls: Callable[..., Any] | None = None, ) -> tuple[str, dict[str, Any], list[Any]]: @@ -113,10 +114,13 @@ 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=[], + # Tools are OPT-IN and default to none. The single-shot reasoning→JSON + # callers (clarifier/planner/fixer/verifier) leave this empty so the call + # stays a fast deterministic turn (no repo exploration / tool loops that + # produce slow, unparseable output). A genuinely agentic caller — e.g. the + # builder synthesizing a unified diff — passes a read-only allowlist + # (Read/Grep/Glob) so it can inspect the repo and finish its diff. + allowed_tools=list(allowed_tools or []), ) texts: list[str] = [] @@ -175,6 +179,7 @@ def subscription_invoker( max_turns: int = _DEFAULT_MAX_TURNS, budget_usd: float = _DEFAULT_BUDGET_USD, model: str | None = None, + allowed_tools: list[str] | None = None, _query: Callable[..., Any] | None = None, _options_cls: Callable[..., Any] | None = None, **kw: Any, @@ -205,6 +210,7 @@ def subscription_invoker( max_turns=max_turns, budget_usd=budget_usd, model=model, + allowed_tools=allowed_tools, _query=_query, _options_cls=_options_cls, ) diff --git a/agent-team/agent_team/nodes/builders.py b/agent-team/agent_team/nodes/builders.py index 9920954..3daba97 100644 --- a/agent-team/agent_team/nodes/builders.py +++ b/agent-team/agent_team/nodes/builders.py @@ -172,6 +172,19 @@ class DiffBuilder(Protocol): ... +# Unlike the single-shot reasoning→JSON nodes (clarifier/planner/fixer/verifier), +# the builder is GENUINELY AGENTIC: synthesizing a unified diff needs to inspect +# the repo and iterate, so it must override the invoker's single-shot defaults +# (max_turns=1, allowed_tools=[]) — otherwise the call dies with "Reached maximum +# number of turns (1)" (issue #60). Tools are READ-ONLY: the builder returns the +# diff as DATA and performs no repo writes (D2/D11), so no Edit/Write/Bash. +_BUILDER_MAX_TURNS = 8 +_BUILDER_ALLOWED_TOOLS = ["Read", "Grep", "Glob"] +# A tool-using diff session runs longer than a one-shot completion; give it more +# headroom than the $2 reasoning-node default. +_BUILDER_BUDGET_USD = 4.0 + + def default_diff_builder( *, plan: Mapping[str, Any], config: Mapping[str, Any] | None ) -> str: @@ -183,9 +196,18 @@ def default_diff_builder( :func:`agent_team.billing.set_invoker` is called, an un-wired environment fails loudly here rather than emitting an empty diff. The coordinator binds the real Claude-spec + DeepSeek-edit path at startup. + + Diff synthesis is agentic, so this passes an explicit non-single-shot config + (turn/budget headroom + a read-only tool allowlist); see ``_BUILDER_*``. """ prompt = _render_build_prompt(plan) - result: ClaudeResult = claude_invoke(prompt, config=config) + result: ClaudeResult = claude_invoke( + prompt, + config=config, + max_turns=_BUILDER_MAX_TURNS, + allowed_tools=_BUILDER_ALLOWED_TOOLS, + budget_usd=_BUILDER_BUDGET_USD, + ) return result.text diff --git a/agent-team/tests/test_builders.py b/agent-team/tests/test_builders.py index 23a820e..c14f123 100644 --- a/agent-team/tests/test_builders.py +++ b/agent-team/tests/test_builders.py @@ -105,6 +105,30 @@ def test_default_diff_builder_calls_claude_invoke() -> None: assert "src" in seen["prompt"] +def test_default_diff_builder_passes_agentic_config_to_invoke_seam() -> None: + # Issue #60: the builder must NOT inherit the invoker's single-shot default + # (max_turns=1, allowed_tools=[]) — diff synthesis is agentic and dies with + # "Reached maximum number of turns (1)" otherwise. It opts in to turn/budget + # headroom plus a READ-ONLY tool allowlist (no repo writes, D2/D11). + seen: dict = {} + + def fake(prompt: str, *, mode: BillingMode, **kw): + seen["kw"] = kw + return ClaudeResult(text=CLEAN_DIFF, mode=mode) + + billing.set_invoker(fake) + default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None) + + kw = seen["kw"] + assert kw.get("max_turns") == builders._BUILDER_MAX_TURNS + assert kw["max_turns"] > 1 + allowed = kw.get("allowed_tools") + assert allowed and {"Read", "Grep", "Glob"} <= set(allowed) + # Read-only only: never hand the diff-as-data builder write/exec tools. + assert not ({"Edit", "Write", "Bash"} & set(allowed)) + assert kw.get("budget_usd") == builders._BUILDER_BUDGET_USD + + def test_default_diff_builder_fails_loud_when_unwired() -> None: # Foundation contract: the seam raises until an invoker is bound. billing._invoker = billing._unconfigured_invoker diff --git a/agent-team/tests/test_invoker.py b/agent-team/tests/test_invoker.py index 00bece5..382e5a5 100644 --- a/agent-team/tests/test_invoker.py +++ b/agent-team/tests/test_invoker.py @@ -151,6 +151,50 @@ def test_subscription_invoker_returns_result( assert result.raw == messages +def _make_capturing_query(messages, captured: dict): + """Async ``query`` that records the ``options`` it was handed, then yields.""" + + async def _query(*, prompt, options): + captured["options"] = options + for msg in messages: + yield msg + + return _query + + +def test_subscription_invoker_threads_allowed_tools( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Issue #60: an agentic caller (the builder) opts in to tools; the allowlist + # must reach the Agent SDK options, not be swallowed by **kw. + monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "oauth-tok") + captured: dict = {} + invoker.subscription_invoker( + "build it", + mode=BillingMode.SUBSCRIPTION, + allowed_tools=["Read", "Grep"], + _query=_make_capturing_query([ResultMessage("ok")], captured), + _options_cls=_fake_options, + ) + assert captured["options"]["allowed_tools"] == ["Read", "Grep"] + + +def test_subscription_invoker_defaults_to_no_tools( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The single-shot reasoning nodes (clarifier/planner/fixer/verifier) must + # keep their tool-less default — this is the opt-in guard for future nodes. + monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "oauth-tok") + captured: dict = {} + invoker.subscription_invoker( + "reason", + mode=BillingMode.SUBSCRIPTION, + _query=_make_capturing_query([ResultMessage("ok")], captured), + _options_cls=_fake_options, + ) + assert captured["options"]["allowed_tools"] == [] + + def test_subscription_invoker_falls_back_to_assistant_text( monkeypatch: pytest.MonkeyPatch, ) -> None: