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 4b17e8ebd4 feat(agent-team): bind planner/review/builder/verifier nodes to their models
review_loop_llm -> GPT-4.1 cross_reviewer (orchestrator run.py); builders_llm -> DeepSeek fast_coder (INERT, proposes diff text only); verifier_llm -> ci_gate is sole PASS authority, Claude is fix-proposer only. Hardens review_loop.parse_verdict to word-boundary matching, adds a fail-closed subprocess timeout, and bind_review_node (single-arg, no LangGraph config injection). All fail safe on untrusted model output.
2026-06-18 12:56:42 -04:00

374 lines
15 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",
]
# 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 default_build(instruction: str, *, route: _OrchestratorRoute | None = None) -> str:
"""Default :data:`BuildCallable`: route the build to DeepSeek ``fast_coder``.
Calls the local orchestrator out-of-process — ``python3 <root>/run.py
"<instruction>"`` — and returns its stdout. The orchestrator routes a
well-specified coding task to its ``fast_coder`` agent (DeepSeek); this is
the design's "DeepSeek mechanical edits, via the local orchestrator" path,
deliberately NOT :func:`agent_team.billing.claude_invoke`.
No orchestrator module is imported at module top (deferred, mirroring
:func:`agent_team.graph.build_sqlite_checkpointer`); the call is a plain
subprocess so this binding adds no import-time dependency on the
orchestrator's package graph.
SECURITY: this subprocess only ASKS the model for diff text — it is a model
invocation, not a patch application. It does not run ``git``, does not apply
anything, and does not 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 _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