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/tests/test_builders.py
Adam Moussa ad6f31c115 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.
2026-06-24 14:58:48 -04:00

548 lines
19 KiB
Python

"""Unit tests for the Plane-2 builders node (design §3.3.2, §7.1 P3).
Covers the two box-side halves of the §3.3.2 trust boundary this leaf owns:
the trust-control-surface denylist scan (boundary #2) and the diff integrity
hash (boundary #3), plus the LangGraph node's clean-vs-park state transitions.
The tests import the committed foundation contracts (``billing``,
``state_store``, ``task_model``) verbatim and assert the leaf builds on them
without redefining them.
"""
from __future__ import annotations
import pytest
from agent_team import billing
from agent_team.billing import BillingMode, ClaudeResult
from agent_team.nodes import builders
from agent_team.nodes.builders import (
BuildError,
TrustBoundaryViolation,
build_candidate_diff,
builders_node,
default_diff_builder,
iter_diff_target_paths,
scan_trust_control_surface,
)
from agent_team.state_store import compute_content_hash
from agent_team.task_model import Phase, TaskStatus
# --------------------------------------------------------------------------- #
# Diff fixtures
# --------------------------------------------------------------------------- #
CLEAN_DIFF = """diff --git a/src/app.py b/src/app.py
--- a/src/app.py
+++ b/src/app.py
@@ -1,2 +1,2 @@
-old = 1
+new = 2
"""
NEW_FILE_DIFF = """diff --git a/src/util/helpers.py b/src/util/helpers.py
new file mode 100644
--- /dev/null
+++ b/src/util/helpers.py
@@ -0,0 +1 @@
+def f(): ...
"""
WORKFLOW_DIFF = """diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -1 +1 @@
-on: push
+on: [push, pull_request_target]
"""
def _scope_plan(diff_builder, scope=("src",), **extra):
"""Build a plan dict whose declared scope is ``scope``."""
plan = {"title": "t", "scope": list(scope), "phases": ["p1"]}
plan.update(extra)
return plan
@pytest.fixture(autouse=True)
def _restore_invoker():
"""Restore the billing invoker after each test (shared module state)."""
original = billing._invoker
yield
billing._invoker = original
# --------------------------------------------------------------------------- #
# Foundation imports are used verbatim (no redefinition)
# --------------------------------------------------------------------------- #
def test_imports_foundation_contracts_verbatim() -> None:
# The leaf imports, not redefines, the foundation symbols.
assert builders.compute_content_hash is compute_content_hash
assert builders.claude_invoke is billing.claude_invoke
assert builders.Phase is Phase
assert builders.TaskStatus is TaskStatus
# --------------------------------------------------------------------------- #
# default_diff_builder uses the §3.1 billing seam
# --------------------------------------------------------------------------- #
def test_default_diff_builder_calls_claude_invoke() -> None:
seen: dict = {}
def fake(prompt: str, *, mode: BillingMode, **kw):
seen["prompt"] = prompt
return ClaudeResult(text=CLEAN_DIFF, mode=mode)
billing.set_invoker(fake)
out = default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
assert out == CLEAN_DIFF
# The plan title and scope are surfaced into the build prompt.
assert "x" in seen["prompt"]
assert "src" in seen["prompt"]
def test_default_diff_builder_is_single_shot_and_tool_less() -> None:
# Issue #60: the FALLBACK Claude builder must NOT run agentic with tools.
# Tools make each call consume a turn and the session exhausts the cap before
# emitting a diff (observed live at max_turns=1 and =8). It runs tool-less
# with a few turns of headroom (planner pattern); the live builder is the
# DeepSeek single-completion path, not this one.
seen: dict = {}
def fake(prompt: str, *, mode: BillingMode, **kw):
seen["kw"] = kw
return ClaudeResult(text=CLEAN_DIFF, mode=mode)
billing.set_invoker(fake)
default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
kw = seen["kw"]
assert kw.get("max_turns") == builders._BUILDER_MAX_TURNS
assert kw["max_turns"] > 1 # headroom so the completion can finish the diff
# Tool-less: no agentic tools are requested (that is what caused #60's
# turn-exhaustion). allowed_tools is left unset → the invoker default [].
assert "allowed_tools" not in kw or not kw["allowed_tools"]
def test_default_diff_builder_fails_loud_when_unwired() -> None:
# Foundation contract: the seam raises until an invoker is bound.
billing._invoker = billing._unconfigured_invoker
with pytest.raises(RuntimeError):
default_diff_builder(plan={"title": "x"}, config=None)
def test_extract_unified_diff_from_fenced_block_with_narration() -> None:
# The agentic builder may narrate around a fenced ```diff block; recover only
# the diff, dropping the prose.
text = (
"Let me read the key source files to get exact signatures first.\n"
"Here is the patch:\n\n"
"```diff\n" + CLEAN_DIFF + "```\n"
"That implements the plan."
)
assert builders._extract_unified_diff(text) == CLEAN_DIFF
def test_extract_unified_diff_from_bare_diff_with_leading_narration() -> None:
# No fence: slice from the first ``diff --git`` header, dropping the prose.
text = "I inspected the repo. Applying this change:\n\n" + CLEAN_DIFF
assert builders._extract_unified_diff(text) == CLEAN_DIFF
def test_extract_unified_diff_rejects_narration_only() -> None:
# The exact failure observed live: tool narration with no patch. It MUST fail
# closed (BuildError), never pass narration through as a candidate diff.
text = "Let me read the key source files to get exact signatures before writing the diff."
with pytest.raises(BuildError, match="no unified diff"):
builders._extract_unified_diff(text)
def test_default_diff_builder_extracts_diff_from_narrated_response() -> None:
# End to end: the invoker returns narration + a fenced diff; the builder
# returns the clean unified diff, not the prose.
def fake(prompt: str, *, mode: BillingMode, **kw):
return ClaudeResult(
text="Sure — let me inspect the files.\n```diff\n" + CLEAN_DIFF + "```",
mode=mode,
)
billing.set_invoker(fake)
out = default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
assert out == CLEAN_DIFF
def test_default_diff_builder_raises_on_prose_only_response() -> None:
# A prose-only builder reply parks/fails the task rather than dispatching
# garbage to CI (issue #60 output-contract hardening).
def fake(prompt: str, *, mode: BillingMode, **kw):
return ClaudeResult(text="Let me read the source files first.", mode=mode)
billing.set_invoker(fake)
with pytest.raises(BuildError, match="no unified diff"):
default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
# --------------------------------------------------------------------------- #
# Diff parsing / canonicalization
# --------------------------------------------------------------------------- #
def test_iter_targets_reads_plus_header() -> None:
targets = iter_diff_target_paths(CLEAN_DIFF)
assert [t.path for t in targets] == ["src/app.py"]
def test_iter_targets_ignores_dev_null_source() -> None:
targets = iter_diff_target_paths(NEW_FILE_DIFF)
assert [t.path for t in targets] == ["src/util/helpers.py"]
def test_iter_targets_canonicalizes_dot_segments() -> None:
diff = "+++ b/src/./sub/../app.py\n"
targets = iter_diff_target_paths(diff)
assert [t.path for t in targets] == ["src/app.py"]
def test_iter_targets_dedupes() -> None:
diff = "+++ b/src/app.py\n+++ b/src/app.py\n"
assert len(iter_diff_target_paths(diff)) == 1
# --------------------------------------------------------------------------- #
# Denylist: each surface (§3.3.2 boundary #2)
# --------------------------------------------------------------------------- #
def test_scan_clean_diff_is_empty() -> None:
assert scan_trust_control_surface(CLEAN_DIFF, scope=["src"]) == []
def test_scan_flags_github_workflow() -> None:
v = scan_trust_control_surface(WORKFLOW_DIFF, scope=["src", ".github"])
assert len(v) == 1
assert ".github/workflows" in v[0].reason
@pytest.mark.parametrize(
"path",
[
"infra/template.yaml",
"samconfig.toml".replace("toml", "yaml"),
"service/serverless.yml",
"cdk.json",
"iam/read-policy.json",
"policies/deploy.policy.yaml",
"CODEOWNERS",
".github/CODEOWNERS",
".github/dependabot.yml",
".github/settings.yml",
],
)
def test_scan_flags_trust_control_surface(path: str) -> None:
diff = f"+++ b/{path}\n"
# Declare a wide scope so the only possible failure is the denylist itself.
v = scan_trust_control_surface(diff, scope=[path.split("/")[0], "."])
assert v, f"expected {path} to be denied"
assert v[0].path == path
def test_scan_denylist_beats_scope() -> None:
# A workflow file inside the declared scope is still denied.
v = scan_trust_control_surface(WORKFLOW_DIFF, scope=[".github"])
assert len(v) == 1
assert "workflow" in v[0].reason
# --------------------------------------------------------------------------- #
# Scope enforcement
# --------------------------------------------------------------------------- #
def test_scan_flags_out_of_scope_path() -> None:
diff = "+++ b/other/module.py\n"
v = scan_trust_control_surface(diff, scope=["src"])
assert len(v) == 1
assert "declared scope" in v[0].reason
def test_empty_scope_rejects_everything() -> None:
# An undeclared scope is a hard stop, not a wildcard.
v = scan_trust_control_surface(CLEAN_DIFF, scope=[])
assert len(v) == 1
assert "declared scope" in v[0].reason
def test_scope_prefix_match_is_boundary_safe() -> None:
# "src" must not accidentally allow "srcfoo/...".
diff = "+++ b/srcfoo/app.py\n"
v = scan_trust_control_surface(diff, scope=["src"])
assert len(v) == 1
assert "declared scope" in v[0].reason
# --------------------------------------------------------------------------- #
# Renames cannot launder a forbidden destination
# --------------------------------------------------------------------------- #
def test_rename_into_denied_path_is_flagged() -> None:
diff = (
"diff --git a/src/app.py b/.github/workflows/evil.yml\n"
"similarity index 100%\n"
"rename from src/app.py\n"
"rename to .github/workflows/evil.yml\n"
)
v = scan_trust_control_surface(diff, scope=["src", ".github"])
assert len(v) == 1
assert v[0].path == ".github/workflows/evil.yml"
assert v[0].rename_from == "src/app.py"
def test_rename_into_out_of_scope_is_flagged() -> None:
diff = "rename from src/app.py\nrename to other/app.py\n"
v = scan_trust_control_surface(diff, scope=["src"])
assert len(v) == 1
assert v[0].path == "other/app.py"
assert v[0].rename_from == "src/app.py"
# --------------------------------------------------------------------------- #
# Deletes / mode-changes / copies cannot bypass the scan
# (regression: header-only sections carry their path in ``diff --git``, not the
# ``+++ b/`` body line, so a ``+++``-only scan missed all four of these)
# --------------------------------------------------------------------------- #
def test_delete_of_denied_path_is_flagged() -> None:
diff = (
"diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n"
"deleted file mode 100644\n"
"--- a/.github/workflows/ci.yml\n"
"+++ /dev/null\n"
"@@ -1 +0,0 @@\n"
"-on: push\n"
)
v = scan_trust_control_surface(diff, scope=["src", ".github"])
assert len(v) == 1
assert v[0].path == ".github/workflows/ci.yml"
assert "workflow" in v[0].reason
def test_mode_change_only_on_denied_path_is_flagged() -> None:
# A chmod with no +++ line at all — only the diff --git header exists.
diff = (
"diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n"
"old mode 100644\n"
"new mode 100755\n"
)
v = scan_trust_control_surface(diff, scope=["src", ".github"])
assert len(v) == 1
assert v[0].path == ".github/workflows/ci.yml"
assert "workflow" in v[0].reason
def test_copy_into_denied_path_is_flagged() -> None:
# git ``copy to`` (not ``rename to``) must not launder a forbidden dest.
diff = (
"diff --git a/src/x.py b/.github/workflows/evil.yml\n"
"similarity index 100%\n"
"copy from src/x.py\n"
"copy to .github/workflows/evil.yml\n"
)
v = scan_trust_control_surface(diff, scope=["src", ".github"])
assert [x.path for x in v] == [".github/workflows/evil.yml"]
assert v[0].rename_from == "src/x.py"
assert "workflow" in v[0].reason
def test_out_of_scope_delete_is_flagged() -> None:
diff = (
"diff --git a/secret/key.py b/secret/key.py\n"
"deleted file mode 100644\n"
"--- a/secret/key.py\n"
"+++ /dev/null\n"
"@@ -1 +0,0 @@\n"
"-KEY = 1\n"
)
v = scan_trust_control_surface(diff, scope=["src"])
assert len(v) == 1
assert v[0].path == "secret/key.py"
assert "scope" in v[0].reason
# --------------------------------------------------------------------------- #
# Box-side / CI-side denylist parity (security-review: box-side was narrower)
# --------------------------------------------------------------------------- #
@pytest.mark.parametrize(
"path",
[
"infra/main.tf",
"infra/app-stack.ts",
"infra/app_stack.py",
"secrets/deploy.pem",
"signing.key",
"config/policy.json",
"roles/myiam.json",
".github/actions/build/action.yml",
],
)
def test_box_side_denylist_covers_ci_surface(path: str) -> None:
# These IaC/key families were caught by the CI guard but slipped the box-side
# scan. The box is the first backstop; it must reject them too (no auto-build).
diff = (
f"diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n"
"@@ -1 +1 @@\n-x\n+y\n"
)
scope = [path.split("/")[0], "."]
v = scan_trust_control_surface(diff, scope=scope)
assert v, f"expected {path} to be denied box-side"
# --------------------------------------------------------------------------- #
# Unsafe paths (absolute / parent-escaping / indirection)
# --------------------------------------------------------------------------- #
@pytest.mark.parametrize(
"raw",
[
"/etc/passwd",
"../../../etc/passwd",
"..",
"src/../../escape.py",
"c:/windows/system32", # absolute-ish via backslash normalization below
],
)
def test_scan_flags_unsafe_paths(raw: str) -> None:
diff = f"+++ b/{raw}\n"
v = scan_trust_control_surface(diff, scope=["src", "."])
assert v, f"expected {raw} to be flagged"
assert any("unsafe" in x.reason or "scope" in x.reason for x in v)
def test_backslash_separator_is_normalized() -> None:
# A Windows separator must not smuggle a denied path past the POSIX matcher.
diff = "+++ b/.github\\workflows\\ci.yml\n"
v = scan_trust_control_surface(diff, scope=[".github"])
assert len(v) == 1
assert "workflow" in v[0].reason
# --------------------------------------------------------------------------- #
# build_candidate_diff: hashing (§3.3.2 boundary #3) + errors
# --------------------------------------------------------------------------- #
def _stub_builder(diff: str):
def _b(*, plan, config):
return diff
return _b
def test_build_hash_matches_foundation_primitive() -> None:
plan = _scope_plan(None)
outcome = build_candidate_diff(plan, builder=_stub_builder(CLEAN_DIFF))
assert outcome.diff == CLEAN_DIFF
assert outcome.diff_hash == compute_content_hash(CLEAN_DIFF.encode("utf-8"))
assert outcome.clean is True
def test_build_clean_outcome_has_no_violations() -> None:
plan = _scope_plan(None)
outcome = build_candidate_diff(plan, builder=_stub_builder(NEW_FILE_DIFF))
assert outcome.violations == []
assert outcome.clean is True
def test_build_violation_still_hashes() -> None:
plan = _scope_plan(None, scope=("src", ".github"))
outcome = build_candidate_diff(plan, builder=_stub_builder(WORKFLOW_DIFF))
# Hash recorded for provenance even though the diff is rejected.
assert outcome.diff_hash == compute_content_hash(WORKFLOW_DIFF.encode("utf-8"))
assert outcome.clean is False
def test_build_rejects_non_mapping_plan() -> None:
with pytest.raises(BuildError):
build_candidate_diff(["not", "a", "mapping"], builder=_stub_builder(CLEAN_DIFF))
def test_build_rejects_empty_diff() -> None:
plan = _scope_plan(None)
with pytest.raises(BuildError):
build_candidate_diff(plan, builder=_stub_builder(" \n "))
def test_build_uses_default_builder_when_none() -> None:
billing.set_invoker(
lambda prompt, *, mode, **kw: ClaudeResult(text=CLEAN_DIFF, mode=mode)
)
outcome = build_candidate_diff(_scope_plan(None))
assert outcome.diff == CLEAN_DIFF
assert outcome.clean is True
# --------------------------------------------------------------------------- #
# builders_node: state transitions
# --------------------------------------------------------------------------- #
def test_node_clean_advances_to_verify() -> None:
state = {"plan": _scope_plan(None)}
update = builders_node(state, builder=_stub_builder(CLEAN_DIFF))
assert update["candidate_diff"] == CLEAN_DIFF
assert update["diff_hash"] == compute_content_hash(CLEAN_DIFF.encode("utf-8"))
assert update["current_phase"] == Phase.VERIFY.value
assert update["status"] == TaskStatus.ACTIVE.value
assert "park_reason" not in update
def test_node_violation_parks_for_human_review() -> None:
state = {"plan": _scope_plan(None, scope=("src", ".github"))}
update = builders_node(state, builder=_stub_builder(WORKFLOW_DIFF))
# Diff + hash recorded for provenance/ALARM, but parked — never auto-built.
assert update["candidate_diff"] == WORKFLOW_DIFF
assert update["current_phase"] == Phase.PARKED.value
assert update["status"] == TaskStatus.PARKED.value
assert "cross-review" in update["park_reason"]
assert "workflow" in update["park_reason"]
def test_node_out_of_scope_parks() -> None:
diff = "+++ b/other/x.py\n"
state = {"plan": _scope_plan(None, scope=("src",))}
update = builders_node(state, builder=_stub_builder(diff))
assert update["status"] == TaskStatus.PARKED.value
assert "declared scope" in update["park_reason"]
def test_node_requires_plan() -> None:
with pytest.raises(BuildError):
builders_node({}, builder=_stub_builder(CLEAN_DIFF))
def test_node_uses_default_builder_and_config() -> None:
captured: dict = {}
def fake(prompt: str, *, mode: BillingMode, **kw):
captured["mode"] = mode
return ClaudeResult(text=CLEAN_DIFF, mode=mode)
billing.set_invoker(fake)
state = {"plan": _scope_plan(None)}
update = builders_node(state, config={"billing_mode": "api"})
assert update["current_phase"] == Phase.VERIFY.value
# The config-selected billing mode reached the seam.
assert captured["mode"] is BillingMode.API
def test_violation_dataclass_shape() -> None:
v = TrustBoundaryViolation(path="a", reason="b", rename_from="c")
assert (v.path, v.reason, v.rename_from) == ("a", "b", "c")