fix(agent-team): wire the DeepSeek mechanical-edit builder as the live diff_builder (#60 root fix)
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.
This commit is contained in:
parent
694161a52c
commit
ad6f31c115
4 changed files with 80 additions and 39 deletions
|
|
@ -545,8 +545,22 @@ def failsafe_production_p3_wiring(
|
||||||
if _p3_env_is_configured():
|
if _p3_env_is_configured():
|
||||||
owner = os.environ.get("AGENT_TEAM_REPO_OWNER", "").strip()
|
owner = os.environ.get("AGENT_TEAM_REPO_OWNER", "").strip()
|
||||||
repo = os.environ.get("AGENT_TEAM_REPO_NAME", "").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 (
|
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,
|
default_dispatch_node_factory,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -172,41 +172,40 @@ class DiffBuilder(Protocol):
|
||||||
...
|
...
|
||||||
|
|
||||||
|
|
||||||
# Unlike the single-shot reasoning→JSON nodes (clarifier/planner/fixer/verifier),
|
# default_diff_builder is the FALLBACK builder — used only when no real
|
||||||
# the builder is GENUINELY AGENTIC: synthesizing a unified diff needs to inspect
|
# diff_builder is bound (the live coordinator binds the DeepSeek mechanical-edit
|
||||||
# the repo and iterate, so it must override the invoker's single-shot defaults
|
# builder, builders_llm.as_diff_builder, which is the intended P3 path). It is
|
||||||
# (max_turns=1, allowed_tools=[]) — otherwise the call dies with "Reached maximum
|
# deliberately SINGLE-SHOT and TOOL-LESS: enabling agentic tools makes every tool
|
||||||
# number of turns (1)" (issue #60). Tools are READ-ONLY: the builder returns the
|
# call consume an agent turn, and the session exhausts the turn cap mid-
|
||||||
# diff as DATA and performs no repo writes (D2/D11), so no Edit/Write/Bash.
|
# exploration before it ever emits a diff (issue #60 — observed live at both
|
||||||
_BUILDER_MAX_TURNS = 8
|
# max_turns=1 and =8). A few tool-less turns of headroom let the model FINISH the
|
||||||
_BUILDER_ALLOWED_TOOLS = ["Read", "Grep", "Glob"]
|
# diff text in one completion, the planner pattern. Tool-driven diff accuracy is
|
||||||
# A tool-using diff session runs longer than a one-shot completion; give it more
|
# the DeepSeek builder's job, not this fallback's.
|
||||||
# headroom than the $2 reasoning-node default.
|
_BUILDER_MAX_TURNS = 4
|
||||||
_BUILDER_BUDGET_USD = 4.0
|
|
||||||
|
|
||||||
|
|
||||||
def default_diff_builder(
|
def default_diff_builder(
|
||||||
*, plan: Mapping[str, Any], config: Mapping[str, Any] | None
|
*, plan: Mapping[str, Any], config: Mapping[str, Any] | None
|
||||||
) -> str:
|
) -> 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
|
Renders the approved plan into an instruction and calls
|
||||||
:func:`agent_team.billing.claude_invoke` (the §3.1 seam) to produce the
|
:func:`agent_team.billing.claude_invoke` (the §3.1 seam) as a single-shot,
|
||||||
unified diff. Because the seam's default invoker raises until
|
tool-less completion to produce the unified diff, then extracts the diff from
|
||||||
:func:`agent_team.billing.set_invoker` is called, an un-wired environment
|
the response (:func:`_extract_unified_diff`). Because the seam's default
|
||||||
fails loudly here rather than emitting an empty diff. The coordinator binds
|
invoker raises until :func:`agent_team.billing.set_invoker` is called, an
|
||||||
the real Claude-spec + DeepSeek-edit path at startup.
|
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
|
This is the INERT fallback only: the live coordinator binds the DeepSeek
|
||||||
(turn/budget headroom + a read-only tool allowlist); see ``_BUILDER_*``.
|
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)
|
prompt = _render_build_prompt(plan)
|
||||||
result: ClaudeResult = claude_invoke(
|
result: ClaudeResult = claude_invoke(
|
||||||
prompt,
|
prompt,
|
||||||
config=config,
|
config=config,
|
||||||
max_turns=_BUILDER_MAX_TURNS,
|
max_turns=_BUILDER_MAX_TURNS,
|
||||||
allowed_tools=_BUILDER_ALLOWED_TOOLS,
|
|
||||||
budget_usd=_BUILDER_BUDGET_USD,
|
|
||||||
)
|
)
|
||||||
return _extract_unified_diff(result.text)
|
return _extract_unified_diff(result.text)
|
||||||
|
|
||||||
|
|
@ -274,12 +273,10 @@ def _render_build_prompt(plan: Mapping[str, Any]) -> str:
|
||||||
f"Title: {title}\n"
|
f"Title: {title}\n"
|
||||||
f"Declared scope (paths you may edit):\n{scope_lines}\n"
|
f"Declared scope (paths you may edit):\n{scope_lines}\n"
|
||||||
f"Phases:\n{phase_lines}\n\n"
|
f"Phases:\n{phase_lines}\n\n"
|
||||||
"You may use the read-only tools (Read/Grep/Glob) to inspect the repo "
|
"Your response MUST be ONLY the unified diff in `git diff` format (each "
|
||||||
"before writing the patch. Your FINAL message MUST be ONLY the unified "
|
"file section beginning with a `diff --git a/… b/…` header), wrapped in a "
|
||||||
"diff in `git diff` format (each file section beginning with a "
|
"single ```diff fenced code block. Do NOT include any narration, "
|
||||||
"`diff --git a/… b/…` header), wrapped in a single ```diff fenced code "
|
"explanation, or text before or after the diff.\n"
|
||||||
"block. Do NOT include any narration, explanation, or text before or "
|
|
||||||
"after the diff.\n"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -105,11 +105,12 @@ def test_default_diff_builder_calls_claude_invoke() -> None:
|
||||||
assert "src" in seen["prompt"]
|
assert "src" in seen["prompt"]
|
||||||
|
|
||||||
|
|
||||||
def test_default_diff_builder_passes_agentic_config_to_invoke_seam() -> None:
|
def test_default_diff_builder_is_single_shot_and_tool_less() -> None:
|
||||||
# Issue #60: the builder must NOT inherit the invoker's single-shot default
|
# Issue #60: the FALLBACK Claude builder must NOT run agentic with tools.
|
||||||
# (max_turns=1, allowed_tools=[]) — diff synthesis is agentic and dies with
|
# Tools make each call consume a turn and the session exhausts the cap before
|
||||||
# "Reached maximum number of turns (1)" otherwise. It opts in to turn/budget
|
# emitting a diff (observed live at max_turns=1 and =8). It runs tool-less
|
||||||
# headroom plus a READ-ONLY tool allowlist (no repo writes, D2/D11).
|
# with a few turns of headroom (planner pattern); the live builder is the
|
||||||
|
# DeepSeek single-completion path, not this one.
|
||||||
seen: dict = {}
|
seen: dict = {}
|
||||||
|
|
||||||
def fake(prompt: str, *, mode: BillingMode, **kw):
|
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"]
|
kw = seen["kw"]
|
||||||
assert kw.get("max_turns") == builders._BUILDER_MAX_TURNS
|
assert kw.get("max_turns") == builders._BUILDER_MAX_TURNS
|
||||||
assert kw["max_turns"] > 1
|
assert kw["max_turns"] > 1 # headroom so the completion can finish the diff
|
||||||
allowed = kw.get("allowed_tools")
|
# Tool-less: no agentic tools are requested (that is what caused #60's
|
||||||
assert allowed and {"Read", "Grep", "Glob"} <= set(allowed)
|
# turn-exhaustion). allowed_tools is left unset → the invoker default [].
|
||||||
# Read-only only: never hand the diff-as-data builder write/exec tools.
|
assert "allowed_tools" not in kw or not kw["allowed_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:
|
def test_default_diff_builder_fails_loud_when_unwired() -> None:
|
||||||
|
|
|
||||||
|
|
@ -953,6 +953,37 @@ def test_default_review_wiring_binds_and_returns_node_and_router() -> None:
|
||||||
review_loop._review_invoker = saved
|
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:
|
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."""
|
"""Injecting the P2 factories compiles a graph that includes the review vertex."""
|
||||||
from agent_team.nodes import review_loop
|
from agent_team.nodes import review_loop
|
||||||
|
|
|
||||||
Reference in a new issue