From 1a142d4e38b0dccf2f0441027c9e84f81b02dfbc Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 29 Sep 2026 18:29:00 -0500 Subject: [PATCH] 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. --- tests/tools/test_skill_pending_diff_batch.py | 54 ++++++++++++++++++++ tools/write_approval.py | 43 ++++++++++++++-- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/tests/tools/test_skill_pending_diff_batch.py b/tests/tools/test_skill_pending_diff_batch.py index c5a823b2c5..717e6aeeab 100644 --- a/tests/tools/test_skill_pending_diff_batch.py +++ b/tests/tools/test_skill_pending_diff_batch.py @@ -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( diff --git a/tools/write_approval.py b/tools/write_approval.py index eeb088a299..b27db26e4d 100644 --- a/tools/write_approval.py +++ b/tools/write_approval.py @@ -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)" -