Skip to content

fix(tools): guard replayed JSON value patches - #84278

Open
KCAYAAI wants to merge 2 commits into
NousResearch:mainfrom
KCAYAAI:fix/patch-replay-before-similarity
Open

fix(tools): guard replayed JSON value patches#84278
KCAYAAI wants to merge 2 commits into
NousResearch:mainfrom
KCAYAAI:fix/patch-replay-before-similarity

Conversation

@KCAYAAI

@KCAYAAI KCAYAAI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop replayed JSON scalar-string patches before block_anchor or context_aware can reinterpret a target value as a larger property
  • require the selected candidate to be exactly one JSON property whose complete decoded string value equals the requested target
  • preserve precise-strategy priority and reject substring-only replay proof, so retained context or an identical target elsewhere cannot suppress a legitimate edit
  • add real patch file-I/O regressions for the production JSON replay and the selected-candidate false-positive class

Root cause

The merged generic already-applied detector runs only after fuzzy_find_and_replace() returns no match. In the incident shape, a stale JSON string value reached context_aware, which selected the enclosing JSON property. The JSON validator prevented the destructive write, but the patch failed instead of recognizing the replay.

An earlier draft used new_string in selected_candidate as replay proof. Independent adversarial review showed that was too broad: a legitimate block replacement can retain the target phrase as unchanged context. This revision fails closed unless the full file is valid JSON and the selected candidate parses as exactly one property with a complete string value equal to new_string.

Why this placement and scope

Precise strategies still run first. The no-op guard runs only after a similarity strategy selects a candidate, and only for the narrow JSON scalar-string incident shape. This preserves legitimate normalized edits while preventing block_anchor or context_aware from expanding an already-landed JSON value edit into a larger mutation.

This differs from #56570, which checks target presence before every matcher. That whole-file approach can skip a valid edit when the same target text exists elsewhere. It also differs from substring containment inside a selected candidate, which can skip a valid block contraction when the target is merely retained context.

Tests

  • affected patch/file/skill-manager floor: 333 passed, 4 Windows-only skipped
  • focused matcher/already-applied/multimatch floor: 75 passed
  • Ruff and Python bytecode compilation on changed files
  • git diff --check
  • behavioral sabotage harness:
    • superseded containment implementation reproduces the retained-context false no-op
    • revised JSON-property predicate applies the retained-context edit for replace_all=False and True
    • both implementations safely no-op the original JSON replay
  • Windows-footgun scan adds zero findings versus the parent commit; the scanner reports the same 10 pre-existing findings in unchanged test lines on both trees

Separate pre-existing behavior

V4A patch mode can report an unchanged replay under files_modified. The same behavior reproduces on origin/main, so it is not introduced or expanded here and remains outside this replace-mode fix.

Closes #18426

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists labels Aug 12, 2026
@KCAYAAI
KCAYAAI marked this pull request as draft August 12, 2026 04:13
@KCAYAAI KCAYAAI changed the title fix(tools): scope replay guard to similarity candidate fix(tools): guard replayed JSON value patches Aug 12, 2026
@KCAYAAI
KCAYAAI force-pushed the fix/patch-replay-before-similarity branch from c3e9b24 to fc865a8 Compare August 12, 2026 04:15
@KCAYAAI
KCAYAAI marked this pull request as ready for review August 12, 2026 04:21
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(tools): guard replayed JSON value patches

Targeted fix for a real false-positive class; the exact-equality + single-property constraints are the right conservative shape. Observations:

  1. tools/fuzzy_match.py:172json.loads(content) parses the entire file on every similarity-strategy match attempt. For a large JSON file (or a file with JSON-like content that fails to parse) this is a full-file parse per patch attempt on a hot path. The try/except makes failures cheap-ish, but a successful parse of a multi-MB file per attempt could add noticeable latency; worth a short-circuit or a size guard if this path is hit repeatedly.
  2. Exact-equality semantics (parsed_pairs[0][1] == new_string) mean a replay whose target is a substring of the property value (e.g. value https://correct.example/item?x=1 vs target https://correct.example/item) is not detected, and the similarity strategy proceeds to replace the enclosing property. That is the safe direction (no false "already applied"), but it does mean a partial-value edit can still rewrite a larger region than the scalar change implied — consistent with how similarity strategies already behave elsewhere, just confirming it's intended.
  3. Tests are strong and directly target the risk: the parametrized rejection cases (multi-property candidate, trailing comma/newline, non-string values), the JSON-replay E2E noop, and — most importantly — the retained-context cases (test_unrelated_target_text_does_not_block_similarity_edit, test_target_retained_as_candidate_context_does_not_skip_edit for both replace_all modes) prove the guard doesn't suppress legitimate block replacements.

Keep replayed JSON value-patch guard on current main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

patch tool can create duplicate content loops when used repeatedly

3 participants