fix(agent-team): builder returns a real unified diff, not tool narration (#60 output contract)
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.
This commit is contained in:
parent
d12880ed09
commit
694161a52c
2 changed files with 100 additions and 2 deletions
|
|
@ -208,7 +208,48 @@ def default_diff_builder(
|
||||||
allowed_tools=_BUILDER_ALLOWED_TOOLS,
|
allowed_tools=_BUILDER_ALLOWED_TOOLS,
|
||||||
budget_usd=_BUILDER_BUDGET_USD,
|
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:
|
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"
|
"Dependabot config.\n\n"
|
||||||
f"Title: {title}\n"
|
f"Title: {title}\n"
|
||||||
f"Declared scope (paths you may edit):\n{scope_lines}\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"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -136,6 +136,57 @@ def test_default_diff_builder_fails_loud_when_unwired() -> None:
|
||||||
default_diff_builder(plan={"title": "x"}, config=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
|
# Diff parsing / canonicalization
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
|
|
|
||||||
Reference in a new issue