This repository has been archived on 2026-08-04. You can view files and clone it, but cannot push or open issues or pull requests.
orchestrator/agent-team/agent_team/nodes/builders_llm.py
Adam Moussa 75f1dc6f75 fix(ws1/ws5): bootstrap orchestrator root in in-process invokers (G1); thread handbook into clarifier (G4)
Gap-audit findings:
- G1 (blocks-feature): make_cross_reviewer_invoker / make_fast_coder_invoker did
  'from models import' without putting the orchestrator root on sys.path. The
  run-team serve daemon only bootstraps agent-team/, so on the live box every
  GPT-4.1 plan review hit ModuleNotFoundError -> review_plan's blanket except
  silently fail-closed to REQUEST_CHANGES (GPT-4.1 never actually ran). Both
  in-process invokers now call invoker_multi._ensure_orchestrator_on_path()
  before the deferred import. WS1 introduced this when it swapped the review
  default from the subprocess invoker to in-process.
- G4 (degrades): the handbook context_provider was wired into the planner only;
  default_clarify_node_factory now accepts + forwards it, and run-team wires it
  into build_clarify_node too, so clarifying questions are handbook-aware.

Tests: +2 regression tests (path-bootstrap, clarifier threading); _FakeCoordinator
gains build_clarify_node. 1142 passed, ruff clean.
2026-06-23 15:49:40 -04:00

420 lines
17 KiB
Python

"""DeepSeek-backed builders binding — the real §3.3 / §7.1 P3 build seam.
:mod:`agent_team.nodes.builders` owns the Plane-2 builders *node* (the §3.3.2
box-side trust-control-surface denylist + diff-integrity hash) but deliberately
injects the diff-synthesis step behind an ``DiffBuilder`` seam so the leaf stays
pure and unit-testable. Its committed default
(:func:`agent_team.nodes.builders.default_diff_builder`) is a Claude-billing
*stub* whose docstring (builders.py line ~164) notes the REAL implementation
wires "DeepSeek (mechanical edits, via the local orchestrator)". This module is
that real implementation.
Per the locked design, builders are P3 mechanical edits and route to the
orchestrator's ``fast_coder`` (DeepSeek), NOT to Claude. This module therefore
does NOT call :func:`agent_team.billing.claude_invoke`; it calls the local
orchestrator's ``fast_coder`` to produce the candidate diff.
================================ SECURITY BOUNDARY ========================
Builders are P3 in the locked design and HARD-GATED: the live CI apply/verify
trust boundary (§3.3.2) must clear ``/sh-security-review`` + GPT-4.1 cross-review
BEFORE it goes live. This module is MODEL LOGIC ONLY and MUST stay INERT:
* It PROPOSES a candidate diff as DATA (a :class:`CandidateDiff` record). It
NEVER applies a patch, NEVER shells out to ``git``, NEVER writes to or
otherwise mutates the working tree / filesystem, and NEVER makes a live CI
call. Applying a diff is the GATED CI path — not this module's job.
* The only subprocess this module spawns is a read-only call to the local
orchestrator's ``run.py`` to ask ``fast_coder`` for diff TEXT. That
subprocess is a model invocation, not a patch application: its stdout is
parsed as untrusted data and returned; it touches nothing in the target
repo. There is no ``git apply``/``patch``/``git``/``write_text``/``open(...,
"w")`` path anywhere in this file — by construction, the builder cannot
mutate state.
Because the model output is UNTRUSTED, parsing is defensive and FAILS SAFE: on
unparseable output, an empty/whitespace diff, or any build error, the builder
returns an EMPTY/NO-OP candidate marked ``failed`` (``ok is False``) so the
downstream verifier / CI REJECTS it. It NEVER fabricates a "success" diff.
============================================================================
Wiring note (no node edit): this module is a standalone real binding. The
node's injection point is its ``DiffBuilder`` seam — the coordinator should bind
:func:`default_build` (adapted via :func:`as_diff_builder`) into
:func:`agent_team.nodes.builders.builders_node` / ``build_candidate_diff`` at
startup. That wiring edit is deliberately left to the coordinator; this module
does not edit the node.
"""
from __future__ import annotations
import subprocess
from collections.abc import Callable, Mapping
from dataclasses import dataclass
from pathlib import Path
from typing import Any
from agent_team.state_store import compute_content_hash
__all__ = [
"BuildCallable",
"CandidateDiff",
"as_diff_builder",
"build_candidate_diff",
"default_build",
"make_fast_coder_invoker",
"subprocess_build",
]
# The injectable build seam: given the rendered build instruction (a string),
# return the model's raw candidate-diff text. Tests pass a fake; the default
# (:func:`default_build`) routes to the orchestrator's DeepSeek ``fast_coder``.
BuildCallable = Callable[[str], str]
# Default subprocess timeout (seconds) for the orchestrator fast_coder call.
_DEFAULT_TIMEOUT_S = 600
@dataclass
class CandidateDiff:
"""A proposed candidate diff emitted as DATA (never applied here).
This is the record the builders pipeline carries downstream. It mirrors the
fields :func:`agent_team.nodes.builders.build_candidate_diff` records on the
task — the unified-diff text plus its content-hash — and adds the explicit
fail-safe flags so an unparseable/failed build is propagated as a NO-OP the
verifier/CI rejects, rather than as a fabricated success.
Attributes:
diff: The candidate unified diff (empty string on a failed/no-op build).
diff_hash: Content hash of ``diff`` via
:func:`agent_team.state_store.compute_content_hash` (always computed,
including over the empty diff, so CI keys against it deterministically).
ok: ``True`` only when a non-empty, plausibly-unified diff was produced.
failed: ``True`` when the build failed or produced nothing usable (the
inverse of :attr:`ok`); kept explicit so a downstream check can read
either flag.
reason: Human-readable explanation when :attr:`failed`; empty when ``ok``.
"""
diff: str
diff_hash: str
ok: bool
failed: bool
reason: str = ""
@classmethod
def success(cls, diff: str) -> "CandidateDiff":
"""Build an ``ok`` candidate from a validated non-empty diff string."""
return cls(
diff=diff,
diff_hash=compute_content_hash(diff.encode("utf-8")),
ok=True,
failed=False,
reason="",
)
@classmethod
def no_op(cls, reason: str) -> "CandidateDiff":
"""Build a FAILED no-op candidate (empty diff) the verifier/CI rejects.
The empty diff is still hashed so the record shape is uniform and CI's
hash check has a deterministic value to compare; the ``failed`` flag is
what makes the downstream reject it.
"""
return cls(
diff="",
diff_hash=compute_content_hash(b""),
ok=False,
failed=True,
reason=reason,
)
@dataclass
class _OrchestratorRoute:
"""Resolved location + runner for the local orchestrator ``run.py``.
Kept as a tiny dataclass (rather than module-level constants) so the default
build call resolves the orchestrator root lazily and a test could swap the
runner without importing the orchestrator. No orchestrator code is imported
at module top (mirrors :func:`agent_team.graph.build_sqlite_checkpointer`'s
deferred-import discipline).
"""
root: Path
timeout_s: int = _DEFAULT_TIMEOUT_S
def _orchestrator_root() -> Path:
"""Resolve the orchestrator root (the dir holding ``run.py``).
This file lives at ``<root>/agent-team/agent_team/nodes/builders_llm.py``,
so the orchestrator root is ``parents[3]`` (nodes -> agent_team -> agent-team
-> <root>). Verified against the real tree: ``parents[2]`` is ``agent-team``,
not the root.
"""
return Path(__file__).resolve().parents[3]
def make_fast_coder_invoker() -> "BuildCallable":
"""Return an in-process :data:`BuildCallable` backed by DeepSeek ``fast_coder``.
Lazy-imports the orchestrator ``models`` module (never at module load) to
call ``models.get_fast_coder()`` in-process. Mirrors
:func:`~agent_team.nodes.review_loop_llm.make_cross_reviewer_invoker` for
the builder seam (WS1). Any error propagates to the caller
(:func:`build_candidate_diff`), which wraps errors as a ``no_op`` result.
"""
def _invoke(instruction: str) -> str:
# Bootstrap the orchestrator root onto sys.path (models.py lives there;
# the run-team serve daemon does not add it) before the deferred import,
# mirroring make_cross_reviewer_invoker. Without it `from models import`
# raises ModuleNotFoundError in the daemon.
from agent_team.invoker_multi import _ensure_orchestrator_on_path # noqa: PLC0415
_ensure_orchestrator_on_path()
from models import get_fast_coder # noqa: PLC0415 - intentional deferred import
coder = get_fast_coder()
result = coder.invoke(instruction)
text = getattr(result, "content", result)
return text if isinstance(text, str) else str(text)
return _invoke
def subprocess_build(
instruction: str, *, route: _OrchestratorRoute | None = None
) -> str:
"""Opt-in fallback :data:`BuildCallable`: shell out to ``run.py fast_coder``.
The original subprocess-based build path, retained as an opt-in alternative
to the in-process default (:func:`make_fast_coder_invoker`). Use this in
environments where the orchestrator's ``models`` stack is unavailable or for
debugging.
SECURITY: this subprocess only ASKS the model for diff text — it does not
run ``git``, apply anything, or touch the target repo. Its stdout is
untrusted input handed back to :func:`build_candidate_diff` for defensive
parsing.
"""
route = (
route if route is not None else _OrchestratorRoute(root=_orchestrator_root())
)
run_py = route.root / "run.py"
completed = subprocess.run(
["python3", str(run_py), instruction],
capture_output=True,
text=True,
timeout=route.timeout_s,
check=True,
cwd=str(route.root),
)
return completed.stdout
def default_build(instruction: str, *, route: _OrchestratorRoute | None = None) -> str:
"""Default :data:`BuildCallable`: route the build to DeepSeek ``fast_coder`` in-process.
Calls DeepSeek in-process via the orchestrator's ``models.get_fast_coder()``
(WS1). The subprocess path is available as :func:`subprocess_build` for
environments where the models stack is absent.
No orchestrator module is imported at module top (deferred, mirroring
:func:`agent_team.graph.build_sqlite_checkpointer`); any import error
propagates to the caller (:func:`build_candidate_diff`) which wraps it as a
``no_op`` result so the builder fails SAFE.
SECURITY: this call only ASKS the model for diff text — it is a model
invocation, not a patch application. The returned text is untrusted and must
be defensively parsed by the caller.
"""
invoker = make_fast_coder_invoker()
return invoker(instruction)
def _render_build_instruction(plan: Mapping[str, Any], state: Mapping[str, Any]) -> str:
"""Render the approved plan into a mechanical-edit instruction for fast_coder.
Pure string assembly over the plan/state (no I/O) so the instruction shape is
directly unit-testable. The instruction tells the coder to emit ONLY a single
unified diff and to stay inside the declared scope — the box-side denylist in
:mod:`agent_team.nodes.builders` is the real enforcement, but reinforcing it
in the prompt keeps the model on-task.
"""
title = str(plan.get("title") or plan.get("task") or "(untitled task)")
scope = plan.get("scope") or []
phases = plan.get("phases") or []
repo = ""
raw_repo = state.get("repo") if isinstance(state, Mapping) else None
if isinstance(raw_repo, str) and raw_repo.strip():
repo = raw_repo.strip()
scope_lines = "\n".join(f" - {p}" for p in scope) or " (no scope declared)"
phase_lines = (
"\n".join(f" {i + 1}. {p}" for i, p in enumerate(phases)) or " (none)"
)
sections = [
(
"You are performing a mechanical code edit. Implement the approved "
"plan below as a SINGLE unified diff in git format. Output ONLY the "
"diff — no prose, no explanation, no code fences. Touch ONLY files "
"within the declared scope. Do NOT modify CI workflows, IAM/policy "
"IaC, branch-protection, CODEOWNERS, or Dependabot config."
),
"",
f"Title: {title}",
]
if repo:
sections += [f"Repository: {repo}"]
sections += [
f"Declared scope (paths you may edit):\n{scope_lines}",
f"Phases:\n{phase_lines}",
]
return "\n".join(sections)
# A line is plausibly part of a unified diff if it opens a git/file/hunk header.
# Used only to validate that the model returned a diff (not prose) and to strip
# the orchestrator's framing lines (e.g. ``[retrieved: ...]``, ``[fast_coder]``)
# that run.py prints before the result body. This is validation/extraction over
# UNTRUSTED text — never application.
_DIFF_HEADER_PREFIXES = (
"diff --git ",
"--- ",
"+++ ",
"@@ ",
"index ",
"rename from ",
"rename to ",
"copy from ",
"copy to ",
"new file mode ",
"deleted file mode ",
"old mode ",
"new mode ",
)
def _extract_diff(text: str) -> str | None:
"""Extract a unified diff from UNTRUSTED model/orchestrator output, or ``None``.
The orchestrator's ``run.py`` prints framing lines (``[retrieved: ...]``, a
``[route]`` line, a blank line) before the agent's result. We locate the
first real diff header (``diff --git`` / ``--- `` / ``@@ ``) and return from
there to the end, stripping a trailing code-fence if the model wrapped the
diff. Returns ``None`` when no diff header is present at all (prose-only /
empty output) so the caller fails SAFE to a no-op candidate. Pure text
inspection — it never executes or applies the diff.
"""
if not isinstance(text, str) or not text.strip():
return None
lines = text.splitlines()
start: int | None = None
for idx, line in enumerate(lines):
stripped = line.strip()
# ``diff --git`` and a real ``--- a/...`` header are the strongest
# signals; a lone ``@@`` hunk header also anchors a body-only diff.
if (
stripped.startswith("diff --git ")
or line.startswith("--- ")
or stripped.startswith("@@ ")
):
start = idx
break
if start is None:
return None
body_lines = lines[start:]
# Drop a trailing markdown fence if the model wrapped the diff in ```.
while body_lines and body_lines[-1].strip() in ("```", ""):
if body_lines[-1].strip() == "```":
body_lines.pop()
break
body_lines.pop()
diff = "\n".join(body_lines).strip()
if not diff:
return None
# Require at least one recognizable diff header line, so a stray ``--- ``
# inside prose cannot masquerade as a diff.
if not any(
any(ln.startswith(p) or ln.strip().startswith(p) for p in _DIFF_HEADER_PREFIXES)
for ln in diff.splitlines()
):
return None
return diff
def build_candidate_diff(
plan: Mapping[str, Any],
state: Mapping[str, Any] | None = None,
*,
build: BuildCallable | None = None,
) -> CandidateDiff:
"""Propose a candidate diff for ``plan`` via DeepSeek ``fast_coder`` (P3).
Renders the approved ``plan`` (+ optional ``state``) into a mechanical-edit
instruction, calls the injected ``build`` callable (default
:func:`default_build`, which routes to the orchestrator's DeepSeek
``fast_coder``), defensively parses the UNTRUSTED result, and returns a
:class:`CandidateDiff` record.
FAIL SAFE (never fabricate success): if ``plan`` is not a mapping, the build
raises, or the output does not parse to a non-empty unified diff, this
returns ``CandidateDiff.no_op(reason)`` — an empty diff marked ``failed`` so
the verifier / CI rejects it. A valid diff yields ``CandidateDiff.success``.
INERT: this function only PROPOSES a diff as data. It does not apply it, run
``git``, or write to the filesystem; applying is the gated CI path.
"""
if not isinstance(plan, Mapping):
return CandidateDiff.no_op("approved plan must be a mapping")
instruction = _render_build_instruction(plan, state or {})
build_fn: BuildCallable = build if build is not None else default_build
try:
raw = build_fn(instruction)
except subprocess.TimeoutExpired:
return CandidateDiff.no_op("build timed out")
except subprocess.CalledProcessError as exc:
return CandidateDiff.no_op(f"build process failed (exit {exc.returncode})")
except Exception as exc: # noqa: BLE001 - any builder failure must fail SAFE
return CandidateDiff.no_op(f"build error: {type(exc).__name__}")
if not isinstance(raw, str) or not raw.strip():
return CandidateDiff.no_op("builder produced empty output")
diff = _extract_diff(raw)
if diff is None:
return CandidateDiff.no_op("builder output is not a usable unified diff")
return CandidateDiff.success(diff)
def as_diff_builder(
build: BuildCallable | None = None,
) -> Callable[..., str]:
"""Adapt this binding to the node's ``DiffBuilder`` seam (keyword signature).
:func:`agent_team.nodes.builders.build_candidate_diff` calls its injected
``DiffBuilder`` as ``builder(plan=..., config=...)`` and expects a unified
-diff STRING back (it then hashes + denylist-scans). This adapter lets the
coordinator bind the real DeepSeek path there: it runs
:func:`build_candidate_diff` and returns the diff string on success.
On a failed/no-op build it returns an EMPTY string. The node treats an empty
diff as ``BuildError`` (its own fail-closed contract), so the adapter never
smuggles a fabricated success past the node either. (The richer
:class:`CandidateDiff` record path is available directly via
:func:`build_candidate_diff` for callers that want the explicit failed flag.)
"""
def _builder(*, plan: Mapping[str, Any], config: Mapping[str, Any] | None) -> str:
state = config if isinstance(config, Mapping) else {}
candidate = build_candidate_diff(plan, state, build=build)
return candidate.diff
return _builder