From dff64161de953a1eac6b851e6f60b7111deb99b5 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 25 Jun 2026 13:37:31 -0400 Subject: [PATCH] Reject malformed or no-op candidate diffs at build The builders node accepted any non-empty diff string and advanced it to dispatch. Two failure modes seen live slipped through: a diff with placeholder hunk headers (`@@ -X,Y +A,B @@`) that git apply rejects with exit 128 (the task then parked at dispatch with an opaque error), and a no-op empty-file creation that applied cleanly and opened a blank draft PR. Add a pure, deterministic shape check in build_candidate_diff that fails with an actionable BuildError when a hunk header is non-numeric or the patch carries no added/removed content, so the cause surfaces at build instead of downstream. --- agent-team/agent_team/nodes/builders.py | 55 +++++++++++++++++++++++++ agent-team/tests/test_builders.py | 50 +++++++++++++++++++++- 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/agent-team/agent_team/nodes/builders.py b/agent-team/agent_team/nodes/builders.py index d69cfdc..cc35021 100644 --- a/agent-team/agent_team/nodes/builders.py +++ b/agent-team/agent_team/nodes/builders.py @@ -55,6 +55,7 @@ __all__ = [ "BuildError", "DiffBuilder", "TrustBoundaryViolation", + "assert_applicable_diff_shape", "build_candidate_diff", "builders_node", "default_diff_builder", @@ -583,6 +584,55 @@ class _BuildOutcome: return not self.violations +# A canonical unified-diff hunk header: "@@ -[,] +[,] @@". +# A builder that emits PLACEHOLDER line numbers (observed live: "@@ -X,Y +A,B @@") +# produces a patch that fails `git apply` with exit 128 at dispatch — the task +# then parks with an opaque "git apply failed" error far from the real cause. We +# reject it HERE, at build, with an actionable reason instead. +_HUNK_HEADER_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@") + + +def assert_applicable_diff_shape(diff: str) -> None: + """Reject a candidate diff that cannot apply or is a no-op, BEFORE dispatch. + + Two failure modes seen live both slipped past the empty-string guard in + :func:`build_candidate_diff` and only manifested downstream — one as a cryptic + dispatch crash, the other as a BLANK draft PR: + + * **Malformed hunk header** — a builder that emits placeholder line numbers + (``@@ -X,Y +A,B @@``) yields a patch ``git apply`` rejects (exit 128). The + task parks at dispatch with an opaque error instead of here with the cause. + * **No-op patch** — a diff that authors no content (e.g. a bare empty-file + creation, ``new file mode … index 0000000..e69de29`` with no hunk) applies + cleanly and becomes a blank PR with "no actual changes". + + Pure + deterministic (no network, no checkout): it inspects the diff text + only. Raises :class:`BuildError` (the build-failure contract) naming the + reason, so the coordinator parks with an actionable signal rather than + dispatching a patch that is already known not to apply. + """ + has_content_change = False + for line in diff.splitlines(): + if line.startswith("@@"): + if not _HUNK_HEADER_RE.match(line): + raise BuildError( + "candidate diff has a malformed/placeholder hunk header " + f"(it will not apply): {line!r}" + ) + continue + # Skip the file-marker lines so only true body content counts; an added + # or removed line in the file body starts with a bare '+'/'-'. + if line.startswith(("+++ ", "--- ")): + continue + if line.startswith(("+", "-")): + has_content_change = True + if not has_content_change: + raise BuildError( + "candidate diff makes no content changes (empty/no-op patch — it " + "would open a blank PR)" + ) + + def build_candidate_diff( plan: Mapping[str, Any], *, @@ -611,6 +661,11 @@ def build_candidate_diff( if not isinstance(diff, str) or not diff.strip(): raise BuildError("diff builder produced an empty candidate diff") + # Catch a malformed (placeholder-hunk) or no-op (blank-PR) diff at build, + # where the reason is actionable, instead of at dispatch (opaque git-apply + # crash) or in a blank draft PR. + assert_applicable_diff_shape(diff) + diff_hash = compute_content_hash(diff.encode("utf-8")) violations = scan_trust_control_surface(diff, scope=plan.get("scope")) return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations) diff --git a/agent-team/tests/test_builders.py b/agent-team/tests/test_builders.py index 072ce76..09f096f 100644 --- a/agent-team/tests/test_builders.py +++ b/agent-team/tests/test_builders.py @@ -480,6 +480,45 @@ def test_build_rejects_empty_diff() -> None: build_candidate_diff(plan, builder=_stub_builder(" \n ")) +# Builder failure modes seen live (both slipped past the empty-string guard): +# * a placeholder-hunk diff (correct content, "@@ -X,Y +A,B @@" headers) that +# git apply rejects (exit 128) -> the task parked at dispatch with an opaque +# error and no PR. +# * a no-op empty-file creation that applied cleanly -> a BLANK draft PR. +PLACEHOLDER_HUNK_DIFF = """diff --git a/README.md b/README.md +index abcdef1..1234567 100644 +--- a/README.md ++++ b/README.md +@@ -X,Y +A,B @@ ++## New section ++body line +""" + +EMPTY_NEW_FILE_DIFF = """diff --git a/tests/test_smoke.py b/tests/test_smoke.py +new file mode 100644 +index 0000000..e69de29 +""" + + +def test_build_rejects_placeholder_hunk_header() -> None: + plan = _scope_plan(None) + with pytest.raises(BuildError, match="hunk header"): + build_candidate_diff(plan, builder=_stub_builder(PLACEHOLDER_HUNK_DIFF)) + + +def test_build_rejects_noop_empty_file_diff() -> None: + plan = _scope_plan(None) + with pytest.raises(BuildError, match="no content changes"): + build_candidate_diff(plan, builder=_stub_builder(EMPTY_NEW_FILE_DIFF)) + + +def test_valid_diffs_pass_shape_check() -> None: + # Regression guard: the new shape check must NOT reject the legitimate + # fixtures (a modify-in-place and a new-file-with-content diff). + for diff in (CLEAN_DIFF, NEW_FILE_DIFF): + builders.assert_applicable_diff_shape(diff) # does not raise + + def test_build_uses_default_builder_when_none() -> None: billing.set_invoker( lambda prompt, *, mode, **kw: ClaudeResult(text=CLEAN_DIFF, mode=mode) @@ -516,7 +555,16 @@ def test_node_violation_parks_for_human_review() -> None: def test_node_out_of_scope_parks() -> None: - diff = "+++ b/other/x.py\n" + # A realistic out-of-scope diff (with content, so it passes the shape check + # and reaches the scope scan that parks it). + diff = ( + "diff --git a/other/x.py b/other/x.py\n" + "--- a/other/x.py\n" + "+++ b/other/x.py\n" + "@@ -1 +1 @@\n" + "-a = 1\n" + "+a = 2\n" + ) state = {"plan": _scope_plan(None, scope=("src",))} update = builders_node(state, builder=_stub_builder(diff)) assert update["status"] == TaskStatus.PARKED.value