Adam Moussa
4b17e8ebd4
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.
354 lines
13 KiB
Python
354 lines
13 KiB
Python
"""Unit tests for agent_team.nodes.builders_llm (§3.3, §7.1 P3).
|
|
|
|
The DeepSeek-backed builders binding is exercised with a FAKE ``build`` callable
|
|
that returns canned text — no network, no subprocess. The load-bearing
|
|
properties under test:
|
|
|
|
* **Clean import.** The module imports without importing the orchestrator at
|
|
module top (the orchestrator package is not importable from this tree).
|
|
* **Happy path.** A fake build returning a valid unified diff yields an ``ok``
|
|
:class:`CandidateDiff` with the right diff and a real content hash.
|
|
* **Fail SAFE.** Garbage / empty / prose-only model output yields a FAILED
|
|
no-op candidate (empty diff, ``ok is False``), never a fabricated success.
|
|
A build that raises also fails safe.
|
|
* **Inert boundary.** The module exposes no patch-applying / git / fs-write
|
|
function — by construction it cannot mutate the repo.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import inspect
|
|
import sys
|
|
from pathlib import Path
|
|
from typing import Any
|
|
|
|
import pytest
|
|
|
|
from agent_team.nodes import builders, builders_llm
|
|
from agent_team.nodes.builders import BuildError, builders_node
|
|
from agent_team.nodes.builders_llm import (
|
|
CandidateDiff,
|
|
as_diff_builder,
|
|
build_candidate_diff,
|
|
default_build,
|
|
)
|
|
from agent_team.state_store import compute_content_hash
|
|
|
|
# A minimal but realistic unified diff the fake build can return.
|
|
_VALID_DIFF = (
|
|
"diff --git a/agent_team/example.py b/agent_team/example.py\n"
|
|
"--- a/agent_team/example.py\n"
|
|
"+++ b/agent_team/example.py\n"
|
|
"@@ -1,2 +1,2 @@\n"
|
|
"-old = 1\n"
|
|
"+new = 2\n"
|
|
)
|
|
|
|
_PLAN = {
|
|
"title": "Add a thing",
|
|
"scope": ["agent_team/"],
|
|
"phases": ["edit example.py"],
|
|
}
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Import hygiene
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_module_imports_without_orchestrator_at_top() -> None:
|
|
"""The module imports cleanly with NO orchestrator import at module top.
|
|
|
|
Parses the module's own top-level import statements (AST) and asserts none of
|
|
them pull in the orchestrator's top-level modules — the real DeepSeek route
|
|
defers to a subprocess, mirroring graph.build_sqlite_checkpointer's deferred
|
|
import. (We inspect the AST rather than reload the module, so the
|
|
``CandidateDiff`` identity used by other tests stays stable.)
|
|
"""
|
|
import ast
|
|
|
|
tree = ast.parse(inspect.getsource(builders_llm))
|
|
orchestrator_mods = {"graph", "agents", "run", "models", "tools", "retriever"}
|
|
top_level_imports: set[str] = set()
|
|
for node in tree.body: # module body only -> top-level imports
|
|
if isinstance(node, ast.Import):
|
|
top_level_imports.update(alias.name.split(".")[0] for alias in node.names)
|
|
elif isinstance(node, ast.ImportFrom) and node.module:
|
|
top_level_imports.add(node.module.split(".")[0])
|
|
|
|
leaked = top_level_imports & orchestrator_mods
|
|
assert not leaked, f"builders_llm imports orchestrator modules at top: {leaked}"
|
|
# And the module imports cleanly (already imported above).
|
|
assert builders_llm.build_candidate_diff is build_candidate_diff
|
|
assert "agent_team.nodes.builders_llm" in sys.modules
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Happy path
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_valid_diff_yields_ok_candidate() -> None:
|
|
calls: list[str] = []
|
|
|
|
def fake_build(instruction: str) -> str:
|
|
calls.append(instruction)
|
|
return _VALID_DIFF
|
|
|
|
candidate = build_candidate_diff(_PLAN, {"repo": "demo"}, build=fake_build)
|
|
|
|
assert isinstance(candidate, CandidateDiff)
|
|
assert candidate.ok is True
|
|
assert candidate.failed is False
|
|
assert candidate.reason == ""
|
|
assert candidate.diff == _VALID_DIFF.strip()
|
|
assert candidate.diff_hash == compute_content_hash(candidate.diff.encode("utf-8"))
|
|
# The instruction was rendered from the plan and handed to the builder.
|
|
assert calls and "Add a thing" in calls[0]
|
|
assert "unified diff" in calls[0]
|
|
|
|
|
|
def test_orchestrator_framing_lines_are_stripped() -> None:
|
|
"""run.py prints framing lines before the result; they must be stripped."""
|
|
framed = "[retrieved: none]\n[fast_coder]\n\n" + _VALID_DIFF
|
|
|
|
candidate = build_candidate_diff(_PLAN, build=lambda _i: framed)
|
|
|
|
assert candidate.ok is True
|
|
assert candidate.diff.startswith("diff --git ")
|
|
assert "[fast_coder]" not in candidate.diff
|
|
assert "[retrieved" not in candidate.diff
|
|
|
|
|
|
def test_fenced_diff_is_unwrapped() -> None:
|
|
fenced = "Here is the diff:\n```diff\n" + _VALID_DIFF + "```\n"
|
|
|
|
candidate = build_candidate_diff(_PLAN, build=lambda _i: fenced)
|
|
|
|
assert candidate.ok is True
|
|
assert candidate.diff.endswith("+new = 2")
|
|
assert "```" not in candidate.diff
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Fail SAFE — never a fabricated success
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_garbage_output_yields_failed_no_op() -> None:
|
|
candidate = build_candidate_diff(
|
|
_PLAN, build=lambda _i: "Sure! I cannot produce a diff right now."
|
|
)
|
|
|
|
assert candidate.ok is False
|
|
assert candidate.failed is True
|
|
assert candidate.diff == ""
|
|
assert candidate.reason
|
|
assert candidate.diff_hash == compute_content_hash(b"")
|
|
|
|
|
|
def test_empty_output_yields_failed_no_op() -> None:
|
|
candidate = build_candidate_diff(_PLAN, build=lambda _i: " \n ")
|
|
|
|
assert candidate.ok is False
|
|
assert candidate.failed is True
|
|
assert candidate.diff == ""
|
|
|
|
|
|
def test_build_exception_fails_safe() -> None:
|
|
def boom(_instruction: str) -> str:
|
|
raise RuntimeError("model exploded")
|
|
|
|
candidate = build_candidate_diff(_PLAN, build=boom)
|
|
|
|
assert candidate.ok is False
|
|
assert candidate.failed is True
|
|
assert candidate.diff == ""
|
|
assert "build error" in candidate.reason
|
|
|
|
|
|
def test_non_mapping_plan_fails_safe() -> None:
|
|
candidate = build_candidate_diff("not a plan", build=lambda _i: _VALID_DIFF) # type: ignore[arg-type]
|
|
|
|
assert candidate.ok is False
|
|
assert candidate.failed is True
|
|
assert candidate.diff == ""
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# DiffBuilder adapter (node seam parity)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_as_diff_builder_returns_string_on_success() -> None:
|
|
builder = as_diff_builder(build=lambda _i: _VALID_DIFF)
|
|
diff = builder(plan=_PLAN, config={"repo": "demo"})
|
|
assert isinstance(diff, str)
|
|
assert diff.startswith("diff --git ")
|
|
|
|
|
|
def test_as_diff_builder_returns_empty_on_failure() -> None:
|
|
"""A failed build must surface as an empty string (node's fail-closed input)."""
|
|
builder = as_diff_builder(build=lambda _i: "no diff here")
|
|
diff = builder(plan=_PLAN, config=None)
|
|
assert diff == ""
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Inert / no-apply boundary
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_module_exposes_no_apply_or_fs_mutation_function() -> None:
|
|
"""No public callable hints at applying a patch, git, or writing files."""
|
|
forbidden_tokens = (
|
|
"apply",
|
|
"git",
|
|
"commit",
|
|
"push",
|
|
"write",
|
|
"mutat",
|
|
"patch",
|
|
"checkout",
|
|
"remove",
|
|
"delete",
|
|
)
|
|
public = [
|
|
name
|
|
for name in dir(builders_llm)
|
|
if not name.startswith("_") and callable(getattr(builders_llm, name))
|
|
]
|
|
for name in public:
|
|
lowered = name.lower()
|
|
for token in forbidden_tokens:
|
|
assert token not in lowered, (
|
|
f"public callable {name!r} suggests a mutation/apply path"
|
|
)
|
|
|
|
|
|
def test_source_has_no_patch_application_or_fs_write_paths() -> None:
|
|
"""Static guard: NO executable call applies a patch, runs git, or writes files.
|
|
|
|
Inspects the AST (so the SECURITY-BOUNDARY docstring's mentions of what the
|
|
module does NOT do are ignored) and asserts no call/attribute names a
|
|
git/patch/apply/fs-mutation primitive. The only subprocess permitted is the
|
|
read-only ``subprocess.run`` model call to the orchestrator.
|
|
"""
|
|
import ast
|
|
|
|
src = inspect.getsource(builders_llm)
|
|
tree = ast.parse(src)
|
|
|
|
banned_attrs = {
|
|
"Popen",
|
|
"write_text",
|
|
"write_bytes",
|
|
"unlink",
|
|
"rmtree",
|
|
"remove",
|
|
"mkdir",
|
|
"rename",
|
|
"replace",
|
|
}
|
|
banned_names = {"open"}
|
|
subprocess_attrs: set[str] = set()
|
|
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Attribute):
|
|
assert node.attr not in banned_attrs, (
|
|
f"module calls a banned fs/git primitive: .{node.attr}"
|
|
)
|
|
if isinstance(node.value, ast.Name) and node.value.id == "subprocess":
|
|
subprocess_attrs.add(node.attr)
|
|
if isinstance(node, ast.Name):
|
|
assert node.id not in banned_names, (
|
|
f"module references a banned builtin: {node.id}"
|
|
)
|
|
|
|
# The only subprocess primitives used are the read-only ``run`` call plus the
|
|
# exception types caught around it — never Popen/call/etc. that could shell a
|
|
# patch-apply.
|
|
assert subprocess_attrs <= {
|
|
"run",
|
|
"TimeoutExpired",
|
|
"CalledProcessError",
|
|
}, f"module uses unexpected subprocess primitives: {subprocess_attrs}"
|
|
|
|
|
|
def test_default_build_invokes_run_py_as_list_argv(
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
) -> None:
|
|
"""``default_build`` shells ``run.py`` via list-form argv (no shell) + maps stdout.
|
|
|
|
Executes the subprocess path (not just AST-checks it): monkeypatches
|
|
``subprocess.run`` to capture the invocation and return canned stdout. The
|
|
argv MUST be the list form ``["python3", <root>/run.py, <instruction>]`` so
|
|
the instruction can never be interpreted by a shell (no ``shell=True``), and
|
|
the return value is the subprocess stdout verbatim.
|
|
"""
|
|
captured: dict[str, Any] = {}
|
|
|
|
class _FakeCompleted:
|
|
stdout = "DIFF-FROM-SUBPROCESS"
|
|
|
|
def _fake_run(argv: Any, **kwargs: Any) -> _FakeCompleted:
|
|
captured["argv"] = argv
|
|
captured["kwargs"] = kwargs
|
|
return _FakeCompleted()
|
|
|
|
monkeypatch.setattr(builders_llm.subprocess, "run", _fake_run)
|
|
|
|
root = Path("/tmp/fake-orchestrator-root")
|
|
route = builders_llm._OrchestratorRoute(root=root)
|
|
out = default_build("do the edit", route=route)
|
|
|
|
assert out == "DIFF-FROM-SUBPROCESS"
|
|
# List-form argv (no shell): exactly python3, the run.py path, the instruction.
|
|
assert captured["argv"] == ["python3", str(root / "run.py"), "do the edit"]
|
|
# No shell=True anywhere in the call (defense against shell injection).
|
|
assert captured["kwargs"].get("shell", False) is False
|
|
|
|
|
|
def test_as_diff_builder_empty_raises_build_error_in_real_node() -> None:
|
|
"""A failed build surfaces as the real ``builders_node``'s BuildError.
|
|
|
|
Integration across the seam boundary: ``as_diff_builder`` adapts a failing
|
|
build ("no diff here" has no diff header -> empty candidate -> empty string),
|
|
and the REAL ``agent_team.nodes.builders.builders_node`` raises its own
|
|
``BuildError`` on the empty-diff path rather than emitting a fabricated diff.
|
|
"""
|
|
builder = as_diff_builder(build=lambda _i: "no diff here")
|
|
state = {"plan": dict(_PLAN)}
|
|
|
|
with pytest.raises(BuildError, match="empty candidate diff"):
|
|
builders_node(state, builder=builder) # type: ignore[arg-type]
|
|
|
|
# Sanity: the node + error are the foundation's, not a redefinition.
|
|
assert builders_node.__module__ == builders.__name__
|
|
|
|
|
|
def test_default_build_is_the_injection_default() -> None:
|
|
"""The default build seam is default_build (the DeepSeek/orchestrator route)."""
|
|
sig = inspect.signature(build_candidate_diff)
|
|
assert sig.parameters["build"].default is None
|
|
# default_build is what gets used when build is None — assert it's callable
|
|
# and routes to a subprocess to run.py (string check, no execution).
|
|
src = inspect.getsource(default_build)
|
|
assert "run.py" in src
|
|
assert "subprocess.run" in src
|
|
|
|
# builders are DeepSeek (orchestrator fast_coder), NOT Claude: assert the
|
|
# module never CALLS billing.claude_invoke (AST, so docstring mentions of the
|
|
# "NOT claude_invoke" contrast don't trip the check).
|
|
import ast
|
|
|
|
tree = ast.parse(inspect.getsource(builders_llm))
|
|
called = set()
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
fn = node.func
|
|
if isinstance(fn, ast.Name):
|
|
called.add(fn.id)
|
|
elif isinstance(fn, ast.Attribute):
|
|
called.add(fn.attr)
|
|
assert "claude_invoke" not in called
|