Repository navigation
fix(file-tools): check remote binary overwrites on the execution target, not the controller - #124106
Merged
kshitijk4poor merged 6 commits intoSep 26, 2026
Conversation
…et filesystem _check_binary_document_write stat'ed the controller host only, so a binary (.sqlite/.pdf/...) that existed solely in the task's execution target (Docker/SSH/... namespace) was treated as a new file and destroyed by a plain-text write_file/patch that even reported verified:true (NousResearch#122662). The existence decision now goes through one tri-state helper (_target_regular_file_state: exists/absent/unavailable) that asks the LIVE file-ops layer - the same backend the write executes on. Host-backed envs keep today's Path.is_file semantics (OSError -> proceed); other backends are probed via ShellFileOperations._probe_regular_file, whose missing/not_regular answers prove absence while every other status fails closed with a retry message. Locality comes from the live environment object (_file_ops_uses_host_paths), never env_type strings or class-name hint tables (VercelSandboxEnvironment is unclassified there). The PDF and generic binary refusal messages are unchanged verbatim; opaque-document and SQLite-sidecar unconditional refusals are untouched. _stale_overwrite_blocker keeps its host-only probe on purpose: remote reads never record a full_write_baseline (version stability requires host metadata), so converting it would refuse every remote overwrite of a file the task fully read. Tracked as follow-up. (cherry picked from commit 8b5eb26)
_probe_regular_file returns "bad_size" only after `[ -f ]` succeeded and `wc -c` output was unparseable, so a regular file IS present on the execution target. Treat it as "exists" (the precise existing-binary refusal) instead of the generic "unavailable" retry message; both refuse, but the retry hint is wrong for a file that is known to exist. Mirrors how the other _probe_regular_file callers treat bad_size. Idea and the original report/Docker reproduction come from NousResearch#122663, the first submitted fix for this bug. Fixes NousResearch#122662 Co-authored-by: liuzikaii <2319582736@qq.com>
Keep the two contracts that pin NousResearch#122662: (1) write_file / patch replace / V4A update against a binary that exists only on an unhinted non-local backend (VercelSandboxEnvironment, scoped config claiming 'local') is refused with bytes untouched, for .sqlite and .pdf; (2) a probe transport failure (raise or error return) fails closed. Creation-allowed, locality and tilde-fallback cases are covered by the first invariant's setup or by tests/tools/test_binary_document_write_guard.py, and the salvage shape caps a stack at two invariant tests.
…ntract The helper docstring restated the inline why-comments (exact probe string, backend $HOME tilde fallback, which statuses prove absence). Keep only the contract callers need: three return values and that anything not proven absent is "unavailable" and must fail closed. Inline comments stay.
The substitution pairs were built by looping over a one-element tuple (left over from a multi-pair version), and _bash_safe() only existed to do a function-local import from a module already imported at the top. Import _bash_safe_path directly and build the three pairs as a literal list.
The remote guard contract only pinned refusal of an existing target-side binary. Nothing pinned the other half: a path the backend proves absent must stay writable, or a guard that over-reports "exists" (e.g. treating a "missing" probe as present) would block every new .pdf/.sqlite on Docker/SSH without a red test. Extend the kept T1 test instead of adding a function (test budget); verified it fails 6/6 under a missing->exists mutation.
kshitijk4poor
enabled auto-merge (rebase)
September 26, 2026 16:43
This was referenced Sep 26, 2026
6 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On a remote terminal backend,
write_file/patchno longer overwrite an existing binary file that only exists on the remote side. The existing-binary guard now checks the filesystem where the write actually runs, not the controller's disk. Fixes #122662. This salvages #123293 by @JoaoMarcos44, with @Liuzikaii (#122663, the first fix and the issue report) credited as co-author.Why
.sqlite/.pdf/ other binary target (_check_binary_document_write) ranPath(...).is_file()on the controller.write_file,patchand V4A run on the task's backend (Docker, SSH, Modal, Vercel sandbox, ...). A file that exists only there looked "new", so its bytes were replaced and the tool reportedverified: true.Changes
_target_regular_file_state()asks the task's ownfile_opswhether a regular file exists at the path the write will use. It returnsexists,absentorunavailable.Path.is_filebehaviour.file_opsobject, not from the env-type label or class name. fix(file-tools): probe remote targets before binary overwrites #122663 used the label, which still clobbered an environment class with no name hint (e.g. a Vercel sandbox configured as "local").[ -f ]succeeded but the size was unparseable counts as existing.Behaviour notes
Validation
tests/tools/test_remote_binary_write_guard.py: 2 test functions, 7 cases, all pass on the rebased head.write_file,patchand V4A, including an environment class with no name hint.missingas existing (file copy, restored after), 6 of the 7 cases fail.tests/tools/test_file_tools.pypasses too (48 passed, 2 skipped together)..sqliteis refused and intact, a new remote file is created, and both host cases behave as on main.Not included
_stale_overwrite_blockerstill checks the host (Path(resolved).exists()), so on remote backends a remote-only text file with no read baseline passes the stale-overwrite guard. Same bug class, but a different guard; it's left for a follow-up.Absence isn't enforced at write time (andrexibiza's P2 on fix(file-tools): decide binary overwrite checks on the execution target filesystem #123293). The guard's "absent" answer is a snapshot taken before the write. If another backend process creates the file between the probe and the write, the write can still replace it. This race predates this PR, and closing it needs a backend-enforced create-if-absent write, so it's left for a follow-up.
Credit
@JoaoMarcos44's commit from #123293 is kept with authorship. @Liuzikaii submitted the first fix (#122663) and filed #122662, and is credited with a Co-authored-by trailer.