fix(skills): preview a staged patch through the approval matcher
_fold_staged()/skill_pending_diff() folded patch ops with plain str.replace(),
ignoring replace_all and the fuzzy-match contract the approve path runs
(_patch_skill -> fuzzy_find_and_replace). A patch whose anchor repeats in the
target content previewed a folded diff ('X X') that /skills approve would then
reject with 'Found 2 matches'.
Fold previews through fuzzy_find_and_replace with the payload's replace_all and
render an explicit '(patch would fail: ...)' note instead of a fabricated
result; in a staged batch a failed patch leaves the later ops' base at the
pre-patch content, since approve aborts the batch at that op. Also drops the
trailing blank line at EOF flagged by git diff --check.
This commit is contained in:
committed by
brooklyn!
parent
bf09d25c7a
commit
1a142d4e38
@@ -105,6 +105,60 @@ class TestSkillPendingDiffBatch(unittest.TestCase):
|
||||
self.assertIn("## op 1/1:", out)
|
||||
self.assertNotIn("/2:", out)
|
||||
|
||||
def test_patch_preview_uses_the_approval_matcher(self):
|
||||
"""A patch whose anchor repeats must NOT preview a fake folded result: approve runs
|
||||
``fuzzy_find_and_replace`` (unique-match unless replace_all), so the preview renders
|
||||
the matcher's own ambiguous error instead of a diff approval would reject with
|
||||
'Found 2 matches' (review feedback on the original diff-fold preview)."""
|
||||
rec = {"id": "jkl012", "payload": {
|
||||
"action": "patch", "name": "probe",
|
||||
"old_string": "Step 1.", "new_string": "Step ONE.",
|
||||
# content on disk: the anchor repeats (SK ends "Step 1.\n"; fold in a second)
|
||||
"replace_all": False}}
|
||||
with open(os.path.join(self.home, "skills", "probe", "SKILL.md"), "a",
|
||||
encoding="utf-8") as f:
|
||||
f.write("Step 1.\n")
|
||||
out = self.wa.skill_pending_diff(rec)
|
||||
self.assertIn("(patch would fail:", out)
|
||||
self.assertIn("Found 2 matches", out)
|
||||
# never a folded diff: approving this payload aborts the batch at this op
|
||||
self.assertNotIn("+Step ONE.", out)
|
||||
|
||||
def test_patch_preview_replace_all_folds_every_match(self):
|
||||
"""With replace_all=True the repeated anchor folds the same way approve would: every
|
||||
occurrence replaced, so the preview shows the diff approval would actually commit."""
|
||||
rec = {"id": "mno345", "payload": {
|
||||
"action": "patch", "name": "probe",
|
||||
"old_string": "Step 1.", "new_string": "Step ONE.",
|
||||
"replace_all": True}}
|
||||
with open(os.path.join(self.home, "skills", "probe", "SKILL.md"), "a",
|
||||
encoding="utf-8") as f:
|
||||
f.write("Step 1.\n")
|
||||
out = self.wa.skill_pending_diff(rec)
|
||||
self.assertNotIn("(patch would fail", out)
|
||||
self.assertEqual(out.count("+Step ONE."), 2)
|
||||
|
||||
def test_batch_preview_flags_failed_patch_without_folding(self):
|
||||
"""In a batch, a failed patch op renders the failure and the NEXT op's base stays at
|
||||
the pre-patch staged content — approve aborts the batch at the failed op, so nothing
|
||||
after it should preview against a result that will never be committed."""
|
||||
sk = SK.format(n="probe")
|
||||
doubled = sk + "Step 1.\n"
|
||||
rec = {"id": "pqr678", "payload": {"action": "batch", "operations": [
|
||||
# op 1: patch whose anchor repeats in the on-disk content (fails, no replace_all)
|
||||
{"action": "patch", "name": "probe",
|
||||
"old_string": "Step 1.", "new_string": "Step ONE."},
|
||||
# op 2: write_file whose target must still show a diff against the UNfolded base
|
||||
{"action": "write_file", "name": "probe", "file_path": "ref.md",
|
||||
"file_content": "z\n"},
|
||||
]}}
|
||||
with open(os.path.join(self.home, "skills", "probe", "SKILL.md"), "w",
|
||||
encoding="utf-8") as f:
|
||||
f.write(doubled)
|
||||
out = self.wa.skill_pending_diff(rec)
|
||||
self.assertIn("(patch would fail:", out)
|
||||
self.assertIn("Found 2 matches", out)
|
||||
|
||||
def test_diff_subcommand_end_to_end(self):
|
||||
from hermes_cli.write_approval_commands import handle_pending_subcommand
|
||||
rec = self.wa.stage_write(
|
||||
|
||||
@@ -323,7 +323,16 @@ def skill_pending_diff(
|
||||
|
||||
if action == "patch":
|
||||
old_s, new_s = payload.get("old_string") or "", payload.get("new_string") or ""
|
||||
new = current.replace(old_s, new_s) if current else f"(patch {old_s!r} → {new_s!r})"
|
||||
if not current:
|
||||
new = f"(patch {old_s!r} → {new_s!r})"
|
||||
else:
|
||||
# Fold through the same matcher approve will run, so the preview can't
|
||||
# fabricate a result the approve path would reject (repeated anchor without
|
||||
# replace_all, whitespace-only anchor, escape drift, old_string == new_string).
|
||||
folded, patch_err = _fold_patch(current, old_s, new_s, payload.get("replace_all"))
|
||||
if patch_err:
|
||||
return f"(patch would fail: {patch_err})"
|
||||
new = folded
|
||||
else:
|
||||
new = payload.get("content" if action == "edit" else "file_content") or ""
|
||||
diff = difflib.unified_diff(current.splitlines(keepends=True), new.splitlines(keepends=True),
|
||||
@@ -331,6 +340,27 @@ def skill_pending_diff(
|
||||
return "".join(diff) or "(no textual change)"
|
||||
|
||||
|
||||
def _fold_patch(base: str, old_string: str, new_string: str, replace_all: Any = False
|
||||
) -> tuple[str, Optional[str]]:
|
||||
"""Fold one patch op through ``fuzzy_find_and_replace`` — the SAME matcher the approve
|
||||
path runs (``_patch_skill`` / ``apply_skill_pending``) — so a previewed diff is exactly
|
||||
what an approval would commit, and a patch approval would reject (repeated anchor with
|
||||
``replace_all`` unset, whitespace-only anchor, escape drift, ``old_string == new_string``)
|
||||
renders as an explicit failed-patch note instead of a fabricated folded result.
|
||||
|
||||
Returns ``(folded_content, None)`` or ``(base, error)``; base is returned unchanged on
|
||||
error so the caller can still render the unchanged content, matching the engine's own
|
||||
``(content, 0, None, error)`` contract. Local import: keeps write_approval importable
|
||||
without the tool chain (fuzzy_match is core-adjacent but guarded for safety anyway).
|
||||
"""
|
||||
from tools.fuzzy_match import fuzzy_find_and_replace
|
||||
folded, match_count, _strategy, error = fuzzy_find_and_replace(
|
||||
base, old_string, str(new_string), bool(replace_all))
|
||||
if error or match_count == 0:
|
||||
return base, error or "Could not find a match for old_string in the file"
|
||||
return folded, None
|
||||
|
||||
|
||||
def _fold_staged(staged: Dict[str, Dict[str, str]], action: str, name: str,
|
||||
op: Dict[str, Any]) -> None:
|
||||
"""Record what one op leaves in the staged view the next op diffs against."""
|
||||
@@ -346,8 +376,14 @@ def _fold_staged(staged: Dict[str, Dict[str, str]], action: str, name: str,
|
||||
elif action == "write_file":
|
||||
files[label] = op.get("file_content") or ""
|
||||
elif action == "patch":
|
||||
files[label] = _staged_base(name, label, staged).replace(
|
||||
op.get("old_string") or "", op.get("new_string") or "")
|
||||
base = _staged_base(name, label, staged)
|
||||
folded, patch_err = _fold_patch(base, op.get("old_string") or "",
|
||||
op.get("new_string") or "", op.get("replace_all"))
|
||||
if patch_err is None:
|
||||
files[label] = folded
|
||||
# On error the staged label keeps its prior content: approve would abort the batch
|
||||
# at this op, so nothing later in the preview should diff against a folded result
|
||||
# that will never be committed.
|
||||
elif action == "remove_file":
|
||||
files.pop(label, None)
|
||||
|
||||
@@ -371,4 +407,3 @@ def _batch_pending_diff(payload: Dict[str, Any]) -> str:
|
||||
parts.append(f"## op {i + 1}/{total}: {gist}\n\n{diff}")
|
||||
_fold_staged(staged, op_action, op_name, op)
|
||||
return "\n\n".join(parts) or "(empty batch)"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user