fix(tools): refuse patch/write_file when content contains a truncation placeholder

[djbclark]

Summary

Defense-in-depth fix for #83714 (and related AI “omitted the rest” write corruption).

write_file / patch (replace + V4A) previously wrote model-supplied string arguments straight to disk. When a model emitted a truncation placeholder (observed as literal ...[truncated] in new_string), the file was corrupted with no error.

Change

• Detect high-confidence truncation placeholders before any write
• write_file: scan content
• patch replace: scan new_string, allowing markers already present in old_string (legitimate docs/edits)
• patch V4A: scan added content only (+ lines / Add-File bodies) via _extract_v4a_added_content(), so removing or anchoring on a pre-existing placeholder-like line is not a false positive
• Fail closed with a clear diagnostic; target file untouched

Relationship to root cause

The confirmed primer for the bare ...[truncated] pattern in long sessions is context compression rewriting past assistant tool-call args — fixed in #83843. This PR remains useful even after that lands because:

• Already-compacted session history may still contain the old marker
• Ordinary tool results still use similar markers (... [truncated], … [truncated]) in several tools
• Models can still emit other “unchanged / rest of file” stubs (feat(tui): add /sessions slash command for browsing and resuming previous sessions #20805 family)

Overlap with #68512

#68512 (open, currently CONFLICTING) proposes a broader signature list and a count-based original-vs-content check at the same pipeline points. This PR is a narrower, current-on-main fix with regression coverage from the #83714 repro. Maintainers should pick one write-guard path rather than landing both; folding any missing signatures / count-based logic from #68512 into this PR is welcome.

Testing

• New regression tests for replace, V4A, and false-positive avoidance
• pytest tests/tools/test_file_tools.py -q — green aside from 2 pre-existing macOS /tmp → /private/tmp mock path mismatches (reproduced on main without this change)

Suggested landing order

1. fix(context-compressor): stop priming models to abbreviate their own tool-call arguments #83843 (root cause)
2. This PR (fix(tools): refuse patch/write_file when content contains a truncation placeholder #83752) (write-path backstop)

Related

• Issue: [Bug]: patch tool truncates new_string content with literal '...[truncated]' text #83714
• Root cause: fix(context-compressor): stop priming models to abbreviate their own tool-call arguments #83843
• Prior art: fix(file-tools): abort write/patch when AI truncation placeholders detected #68512 (credit to @ygd58)
• Related class: feat(tui): add /sessions slash command for browsing and resuming previous sessions #20805

Thank you for reviewing — happy to adjust patterns or wording.

[djbclark]

Rebased onto current upstream/main (dropped unrelated fork-main commit that bloated the diff to 300+ files). Now MERGEABLE, 2 files / +232. Complementary root-cause PR: #83843. Issue: #83714.

[djbclark]

Review follow-up after root-cause confirmation (#83843) + local retest

Code quality here looks solid (V4A added-only scan, old_string exception, parse fallback, good regression tests). A few clear residual gaps worth addressing before/with merge — not blockers on the idea, just tightening defense-in-depth now that we know the real primer.

1. Also refuse the new compressor marker (after #83843)

#83843 replaces bare ...[truncated] in compressed assistant tool_call args with:

⟪HERMES-CONTEXT-COMPRESSION: {omitted} of {total} chars omitted here ...⟫

_find_truncation_placeholder() currently only matches the old family (...[truncated], … [truncated], bare [truncated], // ... unchanged ...). If a model ever copies the new marker (less likely, but the whole point of this PR is fail-closed when imitation happens), we would still write it to disk.

Suggestion: add patterns for HERMES-CONTEXT-COMPRESSION and/or ⟪…⟫ (or a shared constant imported from the compressor module so the two PRs cannot drift).

2. PR body framing is slightly stale vs #83843

Investigation still emphasizes “model imitating truncated tool output.” That remains a secondary source (see below), but the confirmed primary primer for #83714 is Pass‑3 _truncate_tool_call_args_json rewriting past assistant tool_calls[].function.arguments (#83843 / #11762 lineage). Worth one sentence in the description so reviewers don’t underweight the compressor PR.

3. Why this guard stays necessary even after #83843

Many model-visible tool results still emit the old marker family, independent of the compressor tool_call-arg path, e.g.:

• tools/file_operations.py (... [truncated])
• tools/todo_tool.py / tools/registry.py (… [truncated])
• tools/mcp_tool.py, tools/delegate_tool.py, etc.

Those land as role=tool content, not assistant tool_calls, so #83843 alone does not eliminate every imitable stimulus. This PR is still the right write-path backstop.

4. Optional: bare [truncated] breadth

The bare \\[truncated\\] pattern is aggressive (will match legitimate docs/tests that discuss the bug, unless going through the old_string-already-contains exception). Probably acceptable given fail-closed cost, but if maintainers worry about false positives, requiring an ellipsis neighborhood (... / … within N chars) would be safer while still catching the observed failure.

5. Overlap with #68512

#68512 is still open/CONFLICTING and targets the same three pipeline points with a broader signature set. This PR is cleaner/current on main after rebase. Maintainers may want to close one as duplicate or fold any missing patterns from #68512 into this one rather than landing both.

Happy to push a small follow-up commit for (1) if useful.

[djbclark]

Follow-up implemented on combined branch djbclark:fix/truncation-combined-83714 (shared tools/truncation_markers.py, count-based #68512 logic, refuse new compressor marker). Local install has this deployed pending gateway restart. Combined PR opened for maintainers who prefer one stack.

[djbclark]

Housekeeping: description rewritten for a cleaner review path.

Canonical pair for #83714: #83843 first (root cause) → this PR second (write guard).

The combined stack PR #83858 is closed as redundant (local-deploy convenience only). Please ignore it.

Overlap with #68512 is called out in the description with credit to @ygd58; happy for maintainers to fold any missing signatures rather than landing two guards.

Thank you for your time reviewing.

[yuzilongleif-collab]

Independent verification on exact head c484358343b677c66596d517c71638d1ecf8e096 (Linux x86_64, Python 3.11):

pytest -q tests/tools/test_file_tools.py → 53 passed, 2 skipped.

We encountered the reported failure class in real use: a long file-tool argument was replaced by a literal truncation placeholder, while the surrounding tool-call JSON remained valid. The added replace/V4A/write guards and untouched-target regressions passed on the exact PR head. This is useful defense-in-depth even with #83843, because already-compacted histories and model-visible tool-result markers can remain.

No additional blocker found in this verification.

[Enough1122]

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

fix(tools): refuse patch/write_file when content contains a truncation placeholder

Good safety net with carefully reasoned false-positive handling (added-only scan for V4A, old_string exemption for patch). The tests cover the important branches. A few observations:

1. write_file_tool scans the entire content with no exemption mechanism: a legitimate write that must contain the literal marker (e.g. regenerating a test file that asserts "...[truncated]", or fixing a doc file that mentions it) is refused with no opt-out — unlike patch, which has the old_string exemption. The error message is clear and the file is untouched, but consider limiting the write_file check to content that looks truncated (marker at/near the end of the value) or adding an explicit escape hatch, since the whole-file scan will false-positive on any content that legitimately mentions the marker.

2. The patch old_string exemption uses case-insensitive substring containment (_placeholder.lower() not in (old_string or "").lower()): if old_string already contains "[truncated]" and new_string introduces a new truncation marker elsewhere in the string, the check passes. This is a deliberate tradeoff for legitimate edits, but it weakens the guard precisely in the scenario where a model edits a file that already mentions the marker — acceptable, but worth a comment documenting the residual.

3. _extract_v4a_added_content falls back to the full raw patch text on parse failure, which means context/removal lines containing the marker would then be flagged — the exemption tested in test_patch_v4a_allows_marker_on_removed_or_context_line only holds when parsing succeeds. If parse_v4a_patch ever regresses on a real input shape, legitimate patches that merely remove/context-anchor a marker would start being blocked. Consider falling back to scanning only + lines (or scanning nothing) on parse failure, so a parse regression degrades to "no scan" rather than "over-scan".