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.
This commit is contained in:
parent
2e2d560d2d
commit
dff64161de
2 changed files with 104 additions and 1 deletions
|
|
@ -55,6 +55,7 @@ __all__ = [
|
||||||
"BuildError",
|
"BuildError",
|
||||||
"DiffBuilder",
|
"DiffBuilder",
|
||||||
"TrustBoundaryViolation",
|
"TrustBoundaryViolation",
|
||||||
|
"assert_applicable_diff_shape",
|
||||||
"build_candidate_diff",
|
"build_candidate_diff",
|
||||||
"builders_node",
|
"builders_node",
|
||||||
"default_diff_builder",
|
"default_diff_builder",
|
||||||
|
|
@ -583,6 +584,55 @@ class _BuildOutcome:
|
||||||
return not self.violations
|
return not self.violations
|
||||||
|
|
||||||
|
|
||||||
|
# A canonical unified-diff hunk header: "@@ -<start>[,<len>] +<start>[,<len>] @@".
|
||||||
|
# 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(
|
def build_candidate_diff(
|
||||||
plan: Mapping[str, Any],
|
plan: Mapping[str, Any],
|
||||||
*,
|
*,
|
||||||
|
|
@ -611,6 +661,11 @@ def build_candidate_diff(
|
||||||
if not isinstance(diff, str) or not diff.strip():
|
if not isinstance(diff, str) or not diff.strip():
|
||||||
raise BuildError("diff builder produced an empty candidate diff")
|
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"))
|
diff_hash = compute_content_hash(diff.encode("utf-8"))
|
||||||
violations = scan_trust_control_surface(diff, scope=plan.get("scope"))
|
violations = scan_trust_control_surface(diff, scope=plan.get("scope"))
|
||||||
return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations)
|
return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations)
|
||||||
|
|
|
||||||
|
|
@ -480,6 +480,45 @@ def test_build_rejects_empty_diff() -> None:
|
||||||
build_candidate_diff(plan, builder=_stub_builder(" \n "))
|
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:
|
def test_build_uses_default_builder_when_none() -> None:
|
||||||
billing.set_invoker(
|
billing.set_invoker(
|
||||||
lambda prompt, *, mode, **kw: ClaudeResult(text=CLEAN_DIFF, mode=mode)
|
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:
|
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",))}
|
state = {"plan": _scope_plan(None, scope=("src",))}
|
||||||
update = builders_node(state, builder=_stub_builder(diff))
|
update = builders_node(state, builder=_stub_builder(diff))
|
||||||
assert update["status"] == TaskStatus.PARKED.value
|
assert update["status"] == TaskStatus.PARKED.value
|
||||||
|
|
|
||||||
Reference in a new issue