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.
420 lines
17 KiB
Python
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
|