fix: block stale write_file overwrites - #65605
danspicytaco wants to merge 3 commits into
Conversation
Require an explicit full-file baseline before replacing existing host-visible files with write_file, and fail closed when that baseline is stale. This prevents stale conversation context from clobbering manual or external edits.\n\nRefs NousResearch#65604
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Scope
- 1 file, +305/-115 lines. Blocks stale write_file overwrites (part of related PR chain with 65618).
Quality
- Test suite updates reflect the new blocking behavior.
- Consistent with the staleness-check hardening pattern.
Looks Good
- Test coverage for the blocking behavior.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the pre-mutation clobber path. Current main still writes at tools/file_tools.py:1629 before returning its staleness warning at :1631-1633, so the core premise is confirmed.
Problems
tools/file_tools.py:1390-1392marks every non-partial read as a full-write baseline, butread_fileredacts content before returning it (tools/file_tools.py:1316-1319).agent/redact.py:510-519explicitly says the file-read sentinel is non-round-trippable to prevent credential corruption. This new baseline would nevertheless permit a later full overwrite from that incomplete view.
Suggested changes
- Do not establish a full-write baseline when file-read redaction altered the returned content; add an integration test that verifies a secret-bearing file remains unchanged after the attempted full overwrite.
Automated hermes-sweeper review.
SummaryOne PR, #65605, addresses issue #65604. Its diff moves the existing staleness handling ahead of disk mutation, requires a current complete and round-trippable baseline for full-file replacement, and adds refusal-and-reread coverage for stale, partial, redacted, cross-task, and post-compression cases. Related pull requests
Suggested consolidationKeep #65605 open with a salvage path: retain the pre-mutation stale/no-baseline refusal, the complete round-trippable baseline tracking, and the regression tests, including the redacted-read case requested by the contributor review. No duplicate PR is present in this complex. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I65604(["issue #65604 (open)"])
P65605["PR #65605 (open)"]
P65605 -->|best fix| I65604
class I65604 open
class P65605 open
class P65605 best
class P65605 target
click I65604 "https://github.com/NousResearch/hermes-agent/issues/65604"
click P65605 "https://github.com/NousResearch/hermes-agent/pull/65605"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 39 kB of PR diffs, 3 kB of issue/PR text, 4 kB of discussion (6 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…overwrite-guard # Conflicts: # tests/tools/test_cross_profile_guard.py # tests/tools/test_file_read_guards.py # tests/tools/test_file_staleness.py # tests/tools/test_file_state_registry.py # tests/tools/test_file_tools.py # tools/file_tools.py
|
Hi @kshitijk4poor, bumping this for your review. It prevents stale |
|
Salvaged onto current main in #91238 with your two commits cherry-picked (authorship preserved), plus a hardening commit on top: the redacted-read regression test now force-enables redaction so it exercises the sentinel path in hermetic CI, a sibling test pinning the old overwrite-without-read behavior was updated to the new contract, and the docs now describe the read-before-overwrite rule. Thanks @danspicytaco — this PR will be closed with credit once the salvage merges. |
…seline, docs - test_file_staleness redacted-read case now force-enables redaction (matches tests/agent/test_redact.py convention) so it exercises the sentinel path in hermetic CI where security.redact_secrets is unset. - test_write_verification CRLF case establishes a read baseline first (the new guard refuses unread existing-file overwrites by design). - tools-reference.md documents the read-before-overwrite contract. - contributors/emails mapping for DanSpicyTaco.
…seline, docs - test_file_staleness redacted-read case now force-enables redaction (matches tests/agent/test_redact.py convention) so it exercises the sentinel path in hermetic CI where security.redact_secrets is unset. - test_write_verification CRLF case establishes a read baseline first (the new guard refuses unread existing-file overwrites by design). - tools-reference.md documents the read-before-overwrite contract. - contributors/emails mapping for DanSpicyTaco.
…seline, docs - test_file_staleness redacted-read case now force-enables redaction (matches tests/agent/test_redact.py convention) so it exercises the sentinel path in hermetic CI where security.redact_secrets is unset. - test_write_verification CRLF case establishes a read baseline first (the new guard refuses unread existing-file overwrites by design). - tools-reference.md documents the read-before-overwrite contract. - contributors/emails mapping for DanSpicyTaco.
…edaction-gated test, sibling test baseline, docs
…ling test baseline, docs - test_file_staleness redacted-read case now force-enables redaction (matches tests/agent/test_redact.py convention) so it exercises the sentinel path in hermetic CI where security.redact_secrets is unset. - test_write_verification CRLF case establishes a read baseline first (the new guard refuses unread existing-file overwrites by design). - tools-reference.md documents the read-before-overwrite contract. - contributors/emails mapping for DanSpicyTaco.
Context
write_fileis a full-file replacement tool. If the model has an older copy of a file in conversation context, it can use that stale copy as its working base and overwrite manual or external edits made after the last read.Problem
The existing staleness checks only attached warnings after the write had already landed. That meant the guard could tell the model the file was stale while still allowing the data-loss overwrite that the warning was meant to prevent.
Solution
This adds an explicit full-file baseline for
write_file: existing host-visible files need a current non-partialread_fileor prior successful same-taskwrite_filebefore full replacement is allowed. Known-stale baselines now fail closed before disk mutation, and the refusal tells the model to re-read, merge, and retry. Targetedpatchremains warning-only and does not bless a later full-file overwrite.Fixes #65604