Skip to content

fix(file-operations): verify write_file persisted content before reporting success - #23142

Open
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/file-operations-write-verification
Open

fix(file-operations): verify write_file persisted content before reporting success#23142
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/file-operations-write-verification

Conversation

@Frowtek

@Frowtek Frowtek commented May 10, 2026

Copy link
Copy Markdown
Contributor

Summary

ShellFileOperations.write_file() could report success after cat > file
returned cleanly even if the target file was not actually updated on disk.

This change adds a post-write verification step that re-reads the file and
compares the normalized content against the intended write before returning a
successful WriteResult.

What changed

  • Added post-write verification to ShellFileOperations.write_file()
  • Return a clear error when the verify-read fails
  • Return a clear error when the re-read content does not match the intended write
  • Added deterministic tests for:
    • silent persistence failure
    • normal persisted write success

Why

patch_replace() already verifies persistence after writing, but write_file()
did not. That left a false-success path where a backend write could appear to
succeed while the on-disk file remained stale or truncated.

This aligns both write paths around the same invariant: do not report success
until the file content is actually present on disk.

Tests

  • tests/tools/test_file_operations.py::TestWriteFilePostWriteVerification::test_write_file_fails_when_file_not_persisted
  • tests/tools/test_file_operations.py::TestWriteFilePostWriteVerification::test_write_file_succeeds_when_file_persisted
  • tests/tools/test_file_operations.py::TestPatchReplacePostWriteVerification::test_patch_replace_fails_when_file_not_persisted
  • tests/tools/test_file_operations.py::TestPatchReplacePostWriteVerification::test_patch_replace_succeeds_when_file_persisted
  • tests/tools/test_file_operations.py::TestPatchReplacePostWriteVerification::test_patch_replace_fails_when_verify_read_errors

Result: 5 passed

@alt-glitch alt-glitch added type/bug Something isn't working tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists labels May 10, 2026
@yigong-github

yigong-github commented May 24, 2026

Copy link
Copy Markdown

+1 — confirming this exact silent-failure repro on WSL2 (Ubuntu 22.04) writing to /mnt/c/... (Windows 9P share). write_file returned success: true with a plausible bytes_written while the file's mtime on the Windows side never updated. Patching the agent prompt layer to "always re-read after write" is fragile; the fix belongs at the tool layer, exactly as this PR does.

Tested locally with the same approach as this PR (sha256 read-back + CRLF/LF normalize) and it cleanly converts the silent-success path into a hard error. Highly recommend merging.

One small WSL-specific addition worth considering — on /mnt/* paths the 9P client cache can serve a stale view to the verify-read immediately after cat > path, producing false positives in either direction. A targeted sync before re-read fixes it without affecting POSIX-only paths:

python
if path.startswith("/mnt/"):
    self._exec(f"sync {self._escape_shell_arg(path)} 2>/dev/null || sync")


Inserted between the write and the verify-read in both write_file and patch_replace. Cost: ~5ms on /mnt, zero elsewhere. Without it, WSL→Windows users still see occasional verify mismatches even with this PR applied.

Happy to open a follow-up PR layering the WSL sync on top of this one once it lands. Either way — please get this merged, the silent-success path is a real footgun for agents that trust tool return values.

@teknium1 teknium1 left a comment

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.

Thanks for targeting a real false-success path. Current write_file still returns success after _atomic_write and a wc -c probe without comparing read-back content (tools/file_operations.py:1497-1533), while patch_replace already performs a content comparison (tools/file_operations.py:1604-1638).

Problems

  • tests/tools/test_file_operations.py:617 models a successful write only when the command starts with cat >. Main now writes through _atomic_write, whose generated shell script begins with set -e; (tools/file_operations.py:1482-1497), so this test will leave its fake state unchanged and fail after salvage.
  • The new write-file verification-read failure branch has no dedicated test.

Suggested changes

  • Key the test fake on stdin_data is not None, as the current atomic-write tests do at tests/tools/test_file_operations.py:640-647.
  • Add a nonzero verify-read test for write_file.

Automated hermes-sweeper review.


def test_write_file_succeeds_when_file_persisted(self, mock_env):
"""Normal success path: persisted bytes should still report success."""
state = {"content": "old content\n"}

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.

Current main no longer invokes a command beginning with cat >: write_file delegates to _atomic_write, whose shell script begins with set -e; before writing stdin to a temporary file. Key this fake on stdin_data is not None instead, as the current atomic-write tests do, so the persisted-write case updates state after salvage.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows 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.

4 participants