diff --git a/agent-team/agent_team/nodes/builders.py b/agent-team/agent_team/nodes/builders.py index 3daba97..079f4dd 100644 --- a/agent-team/agent_team/nodes/builders.py +++ b/agent-team/agent_team/nodes/builders.py @@ -208,7 +208,48 @@ def default_diff_builder( allowed_tools=_BUILDER_ALLOWED_TOOLS, budget_usd=_BUILDER_BUDGET_USD, ) - return result.text + return _extract_unified_diff(result.text) + + +# The agentic builder uses read-only tools, so its response can wrap the diff in +# a fenced code block and/or surround it with narration ("Let me read the source +# files first…"). Recover the unified diff from that response: a ```diff fence is +# preferred, else the slice from the first ``diff --git`` header. A response with +# NO diff header at all is a hard BuildError — narration must never dispatch as a +# candidate diff (it would fail closed at CI in a confusing way). +_DIFF_FENCE_RE = re.compile(r"```(?:diff|patch)?[ \t]*\n(.*?)```", re.DOTALL) +_DIFF_GIT_MARKER = "diff --git " + + +def _extract_unified_diff(text: str) -> str: + """Recover the unified diff from the (tool-using) builder response. + + Prefers the last fenced ```diff block containing a ``diff --git`` header; + otherwise slices from the first ``diff --git`` marker to the end, dropping any + surrounding narration. Raises :class:`BuildError` when the response carries no + diff header at all (e.g. tool narration with no patch) so a prose-only reply + fails loudly here rather than dispatching as a bogus candidate diff. + """ + if not isinstance(text, str): + raise BuildError("builder returned a non-string response") + + diff = "" + for block in reversed(_DIFF_FENCE_RE.findall(text)): + if _DIFF_GIT_MARKER in block: + diff = block + break + if not diff: + marker = text.find(_DIFF_GIT_MARKER) + if marker != -1: + diff = text[marker:] + + diff = diff.strip() + if _DIFF_GIT_MARKER not in diff: + raise BuildError( + "builder response contained no unified diff (no 'diff --git' header " + "— the model returned narration / tool output instead of a patch)" + ) + return diff + "\n" def _render_build_prompt(plan: Mapping[str, Any]) -> str: @@ -232,7 +273,13 @@ def _render_build_prompt(plan: Mapping[str, Any]) -> str: "Dependabot config.\n\n" f"Title: {title}\n" f"Declared scope (paths you may edit):\n{scope_lines}\n" - f"Phases:\n{phase_lines}\n" + f"Phases:\n{phase_lines}\n\n" + "You may use the read-only tools (Read/Grep/Glob) to inspect the repo " + "before writing the patch. Your FINAL message MUST be ONLY the unified " + "diff in `git diff` format (each file section beginning with a " + "`diff --git a/… b/…` header), wrapped in a single ```diff fenced code " + "block. Do NOT include any narration, explanation, or text before or " + "after the diff.\n" ) diff --git a/agent-team/tests/test_builders.py b/agent-team/tests/test_builders.py index c14f123..21a6cb6 100644 --- a/agent-team/tests/test_builders.py +++ b/agent-team/tests/test_builders.py @@ -136,6 +136,57 @@ def test_default_diff_builder_fails_loud_when_unwired() -> None: 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 # --------------------------------------------------------------------------- #