From 694161a52c40bc8edbe4e81625206adf72fa68e3 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 14:39:50 -0400 Subject: [PATCH] fix(agent-team): builder returns a real unified diff, not tool narration (#60 output contract) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #60 max_turns/tools fix stopped the build-node crash but exposed the next gap: with tools enabled the agentic builder reads files and its final text is NARRATION (observed live: candidate_diff = 'Let me read the key source files to get exact signatures bef…'), not a unified diff. That non-diff dispatched to CI, could not be applied, and the task parked at verify. - builders.py: default_diff_builder now extracts the unified diff from the response via _extract_unified_diff — prefers a fenced ```diff block, else slices from the first 'diff --git' header, dropping surrounding prose. A reply with NO diff header raises BuildError so narration fails closed (the node fails the task with a clear reason) instead of dispatching a bogus diff. - builders.py: _render_build_prompt now instructs the model that its FINAL message must be ONLY the unified diff in a single ```diff fenced block, no narration before/after. - tests: extract from fenced/bare diff with narration; reject narration-only (BuildError); default_diff_builder returns the clean diff from a narrated response and raises on a prose-only reply. Full suite 1503 passed; ruff clean. --- agent-team/agent_team/nodes/builders.py | 51 ++++++++++++++++++++++++- agent-team/tests/test_builders.py | 51 +++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 2 deletions(-) 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 # --------------------------------------------------------------------------- #