fix: prevent write_file from clobbering external edits via staleness check (#65604) - #65618
webtecnica wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Scope
- test files updated (tests/tools/test_file_staleness.py, tests/tools/test_file_state_registry.py)
- Changes assertion from _warning to error for external modification cases.
Quality
- Test updates reflect the new blocking behavior (error instead of warning when file modified externally).
- Clear docstring updates.
Looks Good
- Test-only changes — reflects the behavior change from the corresponding fix.
Reviewed by Hermes Agent
|
👋 Friendly bump — this PR is ready and CI is green, awaiting a maintainer decision. Could you take a look when you have a moment? Thanks @teknium1! |
4b0aa59 to
b36276f
Compare
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the real write-after-stale-read problem. Current main calls file_ops.write_file() before adding the stale warning at tools/file_tools.py:1623-1633, so a pre-mutation refusal is valuable.
Problems
- The proposed blocker at
tools/file_tools.py:1629only catches an existing read/write stamp._check_file_staleness()returns no signal without a read timestamp (tools/file_tools.py:1519-1524), so an existing unread file can still be fully replaced. The related #65605 discussion identifies this full-baseline case. - The diff also makes stale
patchcalls fail at proposedtools/file_tools.py:1777. Current patch behavior deliberately reports a warning (tools/file_tools.py:1742-1784), and the PR title/body are scoped towrite_file.
Suggested changes
- Require a current complete read or prior successful same-task full write before replacing an existing file, with a preservation test.
- Limit fail-closed behavior to
write_file; retain stale patch warnings unless a broader semantic change is intended.
Automated hermes-sweeper review.
| cross_warning = file_state.check_stale(task_id, _resolved) | ||
| stale_warning = _check_file_staleness(path, task_id) | ||
| # Block on staleness — refuse to clobber external edits or | ||
| # sibling-subagent changes. The caller must re-read first. |
There was a problem hiding this comment.
This only blocks when a tracker entry already exists. _check_file_staleness() returns None for an unread path, so a task can still fully overwrite an existing file without a fresh baseline. Add an existing-file/full-read baseline check here.
| cwd_warnings.append(_cwd) | ||
|
|
||
| # Block on staleness — refuse to apply a patch that would | ||
| # clobber external edits or sibling-subagent changes. |
There was a problem hiding this comment.
This broadens the write_file fix into a hard behavioral change for targeted patches. Current patch staleness is warning-only; please retain that behavior unless the patch semantic change is intentionally approved.
teknium1
added
sweeper:risk-compatibility
|
Closing as duplicate — #65605 covers the same stale write_file overwrite protection with a broader fix (still open, updated Aug 3). Thanks for the coordination! 🙏 |
4 participants
Adds a staleness check to write_file that detects if the file has been modified externally since the agent first read it. Uses mtime comparison to detect changes and warns before overwriting external edits from stale conversation context.\n\nCloses #65604