From ad6f31c115bfbb89d519536115bef241c2de3610 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 14:58:48 -0400 Subject: [PATCH] fix(agent-team): wire the DeepSeek mechanical-edit builder as the live diff_builder (#60 root fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real #60 root cause: failsafe_production_p3_wiring never bound a diff_builder, so the build node fell back to builders.default_diff_builder (Claude). The Claude agentic builder (max_turns + read-only tools) then exhausted its turn cap reading files before it could emit a diff — 'Reached maximum number of turns (8)' live. The intended builder already exists: builders_llm.as_diff_builder() routes the build through DeepSeek fast_coder as a SINGLE model completion (with the orchestrator's retrieval context + defensive _extract_diff + fail-safe no-op). A single completion has no agent turn loop, so it CANNOT exhaust turns. Confirmed get_fast_coder() works in the daemon environment. - coordinator.py: failsafe_production_p3_wiring (P3-configured branch) now binds builders_llm.as_diff_builder() as the diff_builder. Without it the build node silently used the turn-exhausting Claude fallback. - builders.py: default_diff_builder reverted to a safe SINGLE-SHOT, TOOL-LESS fallback (max_turns=4, no allowed_tools/budget) — tools are what consumed the turns; this fallback is no longer the live builder. Keeps _extract_unified_diff. - tests: failsafe binds a real (callable) diff_builder when P3 is configured; default_diff_builder is single-shot + tool-less. Full suite 1504 passed; ruff clean. --- agent-team/agent_team/coordinator.py | 16 +++++++- agent-team/agent_team/nodes/builders.py | 51 ++++++++++++------------- agent-team/tests/test_builders.py | 21 +++++----- agent-team/tests/test_coordinator.py | 31 +++++++++++++++ 4 files changed, 80 insertions(+), 39 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index e4399df..12b7b94 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -545,8 +545,22 @@ def failsafe_production_p3_wiring( if _p3_env_is_configured(): owner = os.environ.get("AGENT_TEAM_REPO_OWNER", "").strip() repo = os.environ.get("AGENT_TEAM_REPO_NAME", "").strip() + # Bind the INTENDED diff builder: the DeepSeek ``fast_coder`` mechanical + # -edit path (:func:`agent_team.nodes.builders_llm.as_diff_builder`). It + # produces the diff as a SINGLE model completion, so — unlike the Claude + # agentic fallback (``builders.default_diff_builder``) — it cannot exhaust + # the agent turn cap mid-exploration (issue #60: a tool-using Claude + # session burned all its turns reading files and never emitted a diff). + # Without this bind the build node falls back to the Claude single-shot + # path, which is exactly the #60 failure. Lazy import keeps the + # orchestrator models stack off this module's import surface. + from agent_team.nodes.builders_llm import as_diff_builder + + diff_builder = as_diff_builder() return ( - lambda: gated_build_verify_wiring(owner=owner, repo=repo), + lambda: gated_build_verify_wiring( + owner=owner, repo=repo, diff_builder=diff_builder + ), default_dispatch_node_factory, ) diff --git a/agent-team/agent_team/nodes/builders.py b/agent-team/agent_team/nodes/builders.py index 079f4dd..d69cfdc 100644 --- a/agent-team/agent_team/nodes/builders.py +++ b/agent-team/agent_team/nodes/builders.py @@ -172,41 +172,40 @@ 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 +# default_diff_builder is the FALLBACK builder — used only when no real +# diff_builder is bound (the live coordinator binds the DeepSeek mechanical-edit +# builder, builders_llm.as_diff_builder, which is the intended P3 path). It is +# deliberately SINGLE-SHOT and TOOL-LESS: enabling agentic tools makes every tool +# call consume an agent turn, and the session exhausts the turn cap mid- +# exploration before it ever emits a diff (issue #60 — observed live at both +# max_turns=1 and =8). A few tool-less turns of headroom let the model FINISH the +# diff text in one completion, the planner pattern. Tool-driven diff accuracy is +# the DeepSeek builder's job, not this fallback's. +_BUILDER_MAX_TURNS = 4 def default_diff_builder( *, plan: Mapping[str, Any], config: Mapping[str, Any] | None ) -> str: - """Default :class:`DiffBuilder`: author the diff via the Claude billing seam. + """Fallback :class:`DiffBuilder`: author the diff via the Claude billing seam. Renders the approved plan into an instruction and calls - :func:`agent_team.billing.claude_invoke` (the §3.1 seam) to produce the - unified diff. Because the seam's default invoker raises until - :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. + :func:`agent_team.billing.claude_invoke` (the §3.1 seam) as a single-shot, + tool-less completion to produce the unified diff, then extracts the diff from + the response (:func:`_extract_unified_diff`). Because the seam's default + invoker raises until :func:`agent_team.billing.set_invoker` is called, an + un-wired environment fails loudly here rather than emitting an empty diff. - Diff synthesis is agentic, so this passes an explicit non-single-shot config - (turn/budget headroom + a read-only tool allowlist); see ``_BUILDER_*``. + This is the INERT fallback only: the live coordinator binds the DeepSeek + mechanical-edit builder (:func:`agent_team.nodes.builders_llm.as_diff_builder`) + as the real ``diff_builder``, so this Claude path is not the production + builder. It stays tool-less on purpose — see ``_BUILDER_MAX_TURNS``. """ prompt = _render_build_prompt(plan) result: ClaudeResult = claude_invoke( prompt, config=config, max_turns=_BUILDER_MAX_TURNS, - allowed_tools=_BUILDER_ALLOWED_TOOLS, - budget_usd=_BUILDER_BUDGET_USD, ) return _extract_unified_diff(result.text) @@ -274,12 +273,10 @@ def _render_build_prompt(plan: Mapping[str, Any]) -> str: f"Title: {title}\n" f"Declared scope (paths you may edit):\n{scope_lines}\n" f"Phases:\n{phase_lines}\n\n" - "You may use the read-only tools (Read/Grep/Glob) to inspect the repo " - "before writing the patch. Your FINAL message MUST be ONLY the unified " - "diff in `git diff` format (each file section beginning with a " - "`diff --git a/… b/…` header), wrapped in a single ```diff fenced code " - "block. Do NOT include any narration, explanation, or text before or " - "after the diff.\n" + "Your response MUST be ONLY the unified diff in `git diff` format (each " + "file section beginning with a `diff --git a/… b/…` header), wrapped in a " + "single ```diff fenced code block. Do NOT include any narration, " + "explanation, or text before or after the diff.\n" ) diff --git a/agent-team/tests/test_builders.py b/agent-team/tests/test_builders.py index 21a6cb6..072ce76 100644 --- a/agent-team/tests/test_builders.py +++ b/agent-team/tests/test_builders.py @@ -105,11 +105,12 @@ 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). +def test_default_diff_builder_is_single_shot_and_tool_less() -> None: + # Issue #60: the FALLBACK Claude builder must NOT run agentic with tools. + # Tools make each call consume a turn and the session exhausts the cap before + # emitting a diff (observed live at max_turns=1 and =8). It runs tool-less + # with a few turns of headroom (planner pattern); the live builder is the + # DeepSeek single-completion path, not this one. seen: dict = {} def fake(prompt: str, *, mode: BillingMode, **kw): @@ -121,12 +122,10 @@ def test_default_diff_builder_passes_agentic_config_to_invoke_seam() -> 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 + assert kw["max_turns"] > 1 # headroom so the completion can finish the diff + # Tool-less: no agentic tools are requested (that is what caused #60's + # turn-exhaustion). allowed_tools is left unset → the invoker default []. + assert "allowed_tools" not in kw or not kw["allowed_tools"] def test_default_diff_builder_fails_loud_when_unwired() -> None: diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py index 341e1cd..6b2c1d1 100644 --- a/agent-team/tests/test_coordinator.py +++ b/agent-team/tests/test_coordinator.py @@ -953,6 +953,37 @@ def test_default_review_wiring_binds_and_returns_node_and_router() -> None: review_loop._review_invoker = saved +def test_failsafe_p3_wiring_binds_the_deepseek_diff_builder(monkeypatch: Any) -> None: + # Issue #60 root fix: when the P3 env is configured the live failsafe MUST + # bind a real diff_builder (the DeepSeek mechanical-edit path) onto the build + # node, not leave it None — None falls back to the Claude single-shot builder + # that exhausts its turn cap and fails every build. + from agent_team import coordinator as coord_mod + + monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries") + monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "orchestrator") + monkeypatch.setenv( + "GITHUB_TOKEN", "ci-read-token" + ) # _p3_env_is_configured CI token + + captured: dict = {} + + def spy_gated(*, owner: str, repo: str, **kw: Any) -> tuple: + captured["owner"] = owner + captured["diff_builder"] = kw.get("diff_builder") + return ("build", "verify", "route") + + monkeypatch.setattr(coord_mod, "gated_build_verify_wiring", spy_gated) + + build_verify_thunk, dispatch_factory = coord_mod.failsafe_production_p3_wiring() + assert build_verify_thunk is not None and dispatch_factory is not None + + # The diff_builder is bound when the thunk is invoked at graph-build. + build_verify_thunk() + assert captured["owner"] == "Sea-Haven-Industries" + assert callable(captured["diff_builder"]) # a REAL builder, not None (the bug) + + def test_setup_with_p2_factories_builds_a_review_node(db_path: Path) -> None: """Injecting the P2 factories compiles a graph that includes the review vertex.""" from agent_team.nodes import review_loop