Skip to content

fix(client): keep mutation-tool result confirmations off wire dedup (#1695)#1741

Merged
Hmbown merged 2 commits into
Hmbown:mainfrom
LeoLin990405:fix/1695-writefile-mutation-dedup
May 21, 2026
Merged

fix(client): keep mutation-tool result confirmations off wire dedup (#1695)#1741
Hmbown merged 2 commits into
Hmbown:mainfrom
LeoLin990405:fix/1695-writefile-mutation-dedup

Conversation

@LeoLin990405
Copy link
Copy Markdown
Contributor

Summary / 概述

EN: Addresses #1695. After repeated identical write_file calls on a file larger than ~1 KB, the model behaves and reports as if the write never happened ("No such file or directory" on a follow‑up read), and the same truncated sha (b39a442) appears across the "ghosted" writes. Root cause is not a disk‑write failure — it is the wire compactor collapsing a write confirmation into a <TOOL_RESULT_REF> reference, which destroys the model's proof that the write landed.

中文: 处理 #1695。对大于约 1 KB 的文件重复执行相同的 write_file 后,模型表现/汇报得就像写入从未发生(后续读取报 “No such file or directory”),并且这些“写丢了”的调用都带着同一个截断 sha(b39a442)。根因不是磁盘写入失败,而是 wire 压缩器把写入确认信息折叠成了 <TOOL_RESULT_REF> 引用,导致模型失去了“写入已落盘”的证据。

Root cause / 根因

EN: compact_tool_result_for_wire in crates/tui/src/client/chat.rs deduplicates any tool result whose body is ≥ TOOL_RESULT_DEDUP_MIN_CHARS (1024) when an identical SHA was seen before, replacing the later copy on the wire with a <TOOL_RESULT_REF sha="…" />. A write_file result body is "{unified_diff}\n{summary}" — it embeds the full written content, so two identical >1 KB writes hash to the same SHA (the constant b39a442 the reporter observed) and the second is collapsed to a reference. For retrievable read‑style outputs that is correct and valuable; for a mutation confirmation it is wrong — the model loses the success signal and concludes the file is missing. The 1024 constant lines up exactly with the reporter's ~991 B (lands) / ~1129 B (ghosts) boundary. The on‑disk fs::write in crates/tui/src/tools/file.rs always succeeded; this was always a wire context‑loss defect.

中文: crates/tui/src/client/chat.rscompact_tool_result_for_wire:当某工具结果体长度 ≥ TOOL_RESULT_DEDUP_MIN_CHARS(1024)且此前出现过相同 SHA 时,会在 wire 上把后一份替换为 <TOOL_RESULT_REF sha="…" />。而 write_file 的结果体是 "{unified_diff}\n{summary}"——内嵌了被写入的完整内容,因此两次相同的 >1 KB 写入会得到同一个 SHA(即报告者看到的常量 b39a442),第二份被折叠成引用。对可检索的读类输出这是正确且有价值的优化;但对变更确认而言是错误的——模型因此丢失写入成功信号,进而判定文件不存在。1024 这个常量恰好对应报告中 ~991 B(成功)/ ~1129 B(丢失)的分界。crates/tui/src/tools/file.rs 中的磁盘 fs::write 始终是成功的;这一直是 wire 层上下文丢失的缺陷。

Fix / 修复

One surgical change in crates/tui/src/client/chat.rs: add is_mutation_tool(tool_name) (write_file | edit_file | apply_patch) and AND !is_mutation_tool(tool_name) into the existing dedup_eligible predicate. Mutation‑tool confirmations now always stay inline; read‑style large outputs (read_file, grep_files, exec_shell, …) still dedup exactly as before.

中文:crates/tui/src/client/chat.rs 中做一处精准改动:新增 is_mutation_tool(tool_name)(匹配 write_file | edit_file | apply_patch),并把 !is_mutation_tool(tool_name) 与到既有的 dedup_eligible 判定中。变更类工具的确认信息从此始终内联;读类的大输出仍与此前完全一致地去重。

Honest scope / 改动边界(如实说明)

  • This fixes the observable behavior (model losing write‑confirmation context on repeated identical large writes and reporting the file as missing). It does not alter on‑disk write behavior, which already works correctly — so the original "data never reaches disk" framing in the report is, per code inspection at v0.8.39, a symptom of wire context loss, not a fs::write failure. Worth confirming on the reporter's exact repro.
  • No change to file.rs, the large‑output router, SHA persistence, or non‑mutation dedup.
  • 仅修复可观测行为;不改动磁盘写入逻辑(其本身正确)。报告中“数据没写入磁盘”的描述,按 v0.8.39 代码审查,是 wire 上下文丢失的表象,并非 fs::write 失败,建议在报告者的复现场景上再确认。不改动 file.rs、大输出路由、SHA 持久化或非变更类去重。

Testing / 测试

New regression test request_builder_never_dedups_large_identical_write_file_confirmations: two identical 2000‑char write_file results both stay inline (no <TOOL_RESULT_REF); two identical 2000‑char read_file results still collapse to a SHA ref (non‑mutation dedup preserved).

cargo test -p deepseek-tui --bin deepseek-tui -- \
  request_builder_never_dedups_large_identical_write_file_confirmations \
  request_builder_deduplicates_large_identical_tool_results_with_retrieval_hint
# 2 passed; 0 failed

新增回归测试 request_builder_never_dedups_large_identical_write_file_confirmations,并保留既有去重测试以证明非变更类去重不受影响;两者均通过。

Refs #1695, #1736.

…mbown#1695)

compact_tool_result_for_wire deduped any >=1024-char tool result with a
previously seen SHA into a <TOOL_RESULT_REF>. A write_file result embeds the
full unified diff + summary, so two identical >1KB writes hash to the same
SHA and the second collapsed to a reference, destroying the model's proof
the write landed -- it then reports the file as missing. The on-disk
fs::write always succeeded; this was a wire context-loss defect.

Exclude write_file/edit_file/apply_patch from dedup eligibility via
is_mutation_tool(); read-style large outputs still dedup unchanged. Adds a
regression test asserting mutation confirmations stay inline while
non-mutation dedup is preserved.

Refs Hmbown#1695, Hmbown#1736.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request prevents the deduplication of tool results for mutation-style tools, such as write_file and apply_patch, to ensure that write confirmations remain inline for the model. A review comment identifies that this change also inadvertently disables disk persistence for these tools; it recommends decoupling these checks so that large mutation outputs can still be retrieved if they are truncated.

Comment thread crates/tui/src/client/chat.rs Outdated
Comment on lines +952 to +953
let dedup_eligible =
original_chars >= TOOL_RESULT_DEDUP_MIN_CHARS && !is_mutation_tool(tool_name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation correctly prevents deduplication for mutation tools to ensure the model receives the write confirmation. However, by including !is_mutation_tool(tool_name) in dedup_eligible, it also disables disk persistence for these tools (see line 984 in the full file).

If a mutation tool produces a very large output (e.g., a large diff in write_file or apply_patch exceeding 12,000 characters), it will be truncated at line 1009. Since it wasn't persisted to disk, the model will see the truncation notice with a SHA but will be unable to retrieve the full content via retrieve_tool_result if it needs to see the elided parts.

Consider decoupling the deduplication check from the persistence check so that large mutation outputs are still saved to disk even if they are not eligible for wire-level deduplication.

…view Hmbown#1741)

Address gemini-code-assist review on PR Hmbown#1741 (MEDIUM): folding
!is_mutation_tool into the single dedup_eligible gate also disabled
SHA disk-persistence for mutation tools, so a >12k-char write_file /
apply_patch diff was truncated on the wire AND unrecoverable via
retrieve_tool_result.

Split into two predicates:
- persist_eligible = size only (>= TOOL_RESULT_DEDUP_MIN_CHARS): every
  large result, including mutation diffs, is persisted so truncated
  content stays retrievable by SHA.
- dedup_eligible = persist_eligible && !is_mutation_tool: only this
  gates collapsing a later identical result to <TOOL_RESULT_REF>.
The SHA is registered as dedup-able only when persisted && dedup_eligible.
For non-mutation tools persist_eligible == dedup_eligible, so control
flow is byte-identical; only mutation tools change (now persisted, still
never collapsed -- Hmbown#1695 preserved).

Also serialize cache_inspect_displays_tool_result_budget_metadata
through the existing TEST_SPILLOVER_GUARD: the new test overrides the
process-global spillover root and raced it under the parallel runner
(test-only guard, no production change).

Refs Hmbown#1695.
@LeoLin990405
Copy link
Copy Markdown
Contributor Author

Good catch — addressed in the latest push. The persistence and dedup concerns are now decoupled:

  • persist_eligible = size only (>= TOOL_RESULT_DEDUP_MIN_CHARS) — every large result, including mutation diffs, is persisted to the SHA store so a later truncation stays retrievable via retrieve_tool_result.
  • dedup_eligible = persist_eligible && !is_mutation_tool — only this gates collapsing a later identical result to <TOOL_RESULT_REF>. The SHA is registered as dedup-able only when persisted && dedup_eligible.

For non-mutation tools persist_eligible == dedup_eligible, so control flow is byte-identical to before; only mutation tools change (now persisted, still never collapsed — #1695 preserved). New test large_write_file_result_stays_inline_but_is_persisted_for_retrieval covers a 20k-char write_file: stays inline + truncated, full content persisted & byte-equal on read-back, while a large read_file still dedups. (Also had to serialize one pre-existing debug test through the existing TEST_SPILLOVER_GUARD — it raced the new test's spillover-root override under the parallel runner; test-only, no production change.)

@Hmbown Hmbown merged commit a5501e6 into Hmbown:main May 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants