fix(file-tools): decide binary overwrite checks on the execution target filesystem

[JoaoMarcos44]

Summary

The binary overwrite refusal in tools/file_tools_write_guards.py::_check_binary_document_write decided 'does the destination already exist?' on the controller host while write_file/patch execute in the task's terminal backend, so a binary (.sqlite/.pdf/...) present only in the execution target was treated as new and destroyed by a plain-text write that even reported verified:true. The existence decision now runs on the filesystem the write will hit (the task's live file-ops backend); the database/document survives and the tool refuses.

Related Issue

Fixes #122662

Root cause

• The guard stat'ed Path(_resolve_path_for_task(...)).is_file() on the controller.
• Writes run through ShellFileOperations on the task's backend (tools/file_tools.py write_file_tool -> file_ops.write_file; patch replace mode and V4A Update/Add share the same guard via _write_precheck_error).
• Path normalization cannot make a host stat observe the backend file.

Fix

One tri-state helper _target_regular_file_state (exists/absent/unavailable) in tools/file_tools_write_guards.py:

• Host-backed envs answer with Path.is_file (OSError -> proceed, semantics unchanged).
• Every other backend is probed through its own shell via ShellFileOperations._probe_regular_file with file_ops._expand_path: ok -> exists; missing/not_regular -> absent (same answer Path.is_file gives on the host); anything else (env_unavailable, transport failure) -> unavailable.
• Policy: exists -> refuse with the existing PDF-specific and generic binary messages verbatim; absent -> allow (creating a new file with such an extension stays permitted - raw PDF syntax is text-authorable and text fixtures named *.db exist); unavailable -> refuse fail-closed with a retry message because absence was not established.
• Opaque-document and SQLite-sidecar unconditional refusals unchanged.
• Each branch probes the EXACT string its write path lands on (write_file(_resolved or path) + the file-ops layer's own _expand_path), including the resolution-failure fallback where a tilde path expands with the BACKEND's $HOME, never the host's.

Design decisions

(a) Locality comes from the LIVE file-ops environment object (tools.file_tools._file_ops_uses_host_paths), never from env_type strings, scoped config or class-name hint tables: those misclassify e.g. VercelSandboxEnvironment (absent from _ENV_CLASS_NAME_HINTS, tools/file_tools_paths.py) as local and would re-open the host-probe hole.
(b) Fail closed when the probe cannot establish absence.
(c) not_regular (dir/FIFO/dangling symlink) maps to absent exactly like Path.is_file.
(d) A guard probe failure cannot block anything the write would have completed, since the write itself needs the same file-ops object.
(e) _stale_overwrite_blocker (same file) keeps its host-only exists() on purpose - remote reads never record a full write baseline: tools/file_tools.py:673 version_before is None for non-host backends; tools/file_tools.py:544-558,571-572 _record_successful_read stores no baseline when the read version is None; tools/file_tools_read_tracking.py:278-291 _mark_full_write_baseline/_has_full_write_baseline compare host-side _file_version (os.stat/os.open), so remote baselines are always False; converting it now would refuse every remote overwrite of a fully-read file. Follow-up: teach read/write baselines backend file versions first.

Changed files

• tools/file_tools_write_guards.py (79 insertions, 22 deletions)
• tests/tools/test_remote_binary_write_guard.py (279 insertions, new)

Tests and results

New file: 13 behavioral contracts through real registry.dispatch with real shell/file I/O (the namespace boundary is a path-substituting transport around tools.environments.local.LocalEnvironment.execute that only rewrites PATHS; probe results are never mocked):

• TestRemoteExistingBinaryRefused::test_existing_target_binary_refused_and_untouched (write_file, patch replace, patch V4A x .sqlite, .pdf = 6 cases)
• TestTriStateContract::test_absent_on_target_creation_allowed (.sqlite/.pdf), test_absent_on_target_v4a_add_allowed, test_probe_transport_failure_fails_closed (transport raise/error), test_locality_authority_is_the_live_file_ops_env (env_type 'vercel_sandbox' with scoped config claiming 'local')
• TestResolutionFailureFallbackParity::test_fallback_probe_expands_tilde_on_the_backend

RED on unmodified source (scripts/run_tests.sh tests/tools/test_remote_binary_write_guard.py -q --file-retries 0): 9 failed, 3 passed in 25.09s - all 6 overwrite cases returned success-shaped results with bytes destroyed (assert 'Refusing' in ''), transport-failure cases failed open, and the locality case reported bytes_written: 22, verified: True on a remote-only .sqlite. GREEN at HEAD: 13 passed, 0 failed (independently re-run by the reviewer: 13 passed, 0 failed in 237.3s).

Sibling suites (14 files: test_binary_document_write_guard, test_file_operations, test_file_operations_edge_cases, test_file_staleness, test_file_tools, test_file_tools_cwd_resolution, test_file_tools_tilde_profile, test_file_tools_container_config, test_file_tools_plugin_container_cwd, test_patch_parser, test_patch_v4a_gate, test_patch_already_applied, test_read_file_utf8_binary_regression, test_read_binary_type_disclosure; --file-retries 0): 224 passed, 18 failed, 1 skipped - every failure pre-existing (identical failure set with the production files restored from the parent commit), Windows-environment semantics (umask/chmod, symlinks, NT-path-sensitive checks) plus mtime-timing flaky staleness tests.

Final smoke: test_binary_document_write_guard.py 17 passed; test_file_tools.py 39 passed, 4 failed (pre-existing Windows-platform failures, identical at base).

All runs used the documented wrapper scripts/run_tests.sh with a worktree venv built by python -m pm.build_env --source . --out .venv --group dev --group test. Hygiene: ruff clean on changed files; git diff --check clean.

Related work

• fix(tools): refuse text writes into database files and binary overwrites #114091 introduced the binary write protection with host-side existence checks.
• fix(tools): resolve symlinks in binary-document write guard (#108715) #109087 covers symlink aliases and extensions in the same guard area.
• fix(tools): let the write guard tell a gettext .pot template from a PowerPoint one #122756 distinguishes gettext .pot templates from PowerPoint ones in the same guard area.
• read_file dedup & staleness tracking stat the host filesystem, not the terminal environment #56380 and cross-agent file state coordination is a no-op on remote terminal backends #56656 concern read freshness and cross-agent file-state tracking.
• write_file/read_file/patch resolve paths on the local host instea d of the remote SSH backend, causing writes to fail (macOS host + term inal.backend: ssh) #79663 and fix(tools): keep ssh-backend file paths out of local host resolution #107365 concern remote path normalization.
• fix(code-execution): keep the RPC token out of remote argv and the session snapshot #122440 concerns remote file delivery errors and credentials.
• fix(file-tools): probe remote targets before binary overwrites #122663 is an open pull request also targeting this issue's symptom with a backend regular-file probe.

Limitations and follow-ups

• No live Docker/SSH/Modal/Vercel run on the test host (Docker daemon unavailable; the namespace boundary is a transport stub while probe and mutation use real shell I/O and real bytes; actual remote deployments remain unverified).
• Tests ran on Windows.
• V4A relative-header asymmetry: tools/file_tools.py patch_tool rewrites V4A header paths for host backends only, so a relative remote V4A header could in theory resolve differently (absolute paths, which the tests use, are exact).
• tests/tools/test_file_staleness.py has pre-existing mtime-timing flakiness.
• _stale_overwrite_blocker follow-up described in Design decisions.

Branch and remote state

• Base branch: main
• Branch cut from 1f30bdb
• Head: 8b5eb26 (single commit)
• Staleness at publication: behind main by 86, ahead by 1
• Checks at publication: no checks reported on the branch at read-back time (CI had not run yet; no checks are claimed green)

Security considerations

Data-integrity fix in an existing write-safety heuristic; no credentials or secrets involved; no sandbox-escape or authorization claim; this is not an official security advisory; tests use synthetic bytes only.

Infographic

[andrexibiza]

Moving the existence check onto the task’s actual file-operations backend fixes the namespace mistake reported by Liuzikaii in #122662. The live-environment check and the backend-home fallback test are meaningful improvements: neither a configuration label nor the controller’s expansion of ~ should decide whether a remote file exists.

I traced the new helper through write_file_tool, replace-mode patch, V4A prechecks, path resolution, the file-operations cache, the stale-overwrite check, and the writer’s post-write hash verification. There is one remaining P2, should-fix data-integrity gap: the absence decision is not carried into the mutation as a precondition. I have put the concrete interleaving and correction at the new absent return below.

This race predates this PR. It is not a newly introduced P1 regression, and it is not a reason to discard this namespace correction. The caller’s precheck-before-lock ordering is byte-identical at the merge base and this head (tools/file_tools.py blob 91c2fe03b44cdde3f152ea17b50e080637be7b21). The distinction matters: this change improves which filesystem is inspected; it does not establish how long that observation remains valid. The broader overwrite-protection class should remain open until the mutation itself enforces the relevant condition.

What the current code establishes

The helper asks the live backend rather than the controller for a non-host target, retains the existing PDF/binary refusal messages, and refuses on unavailable or unrecognized probe outcomes. Opaque-document and SQLite-sidecar denials still precede the conditional existence check. New-file creation remains allowed. The refusal occurs before the write/patch body, so the new unavailable-probe response is a pre-effect refusal, not an assertion that a failed write was rolled back.

The 13 cases in the added test file exercise static namespace disagreement, all three public operations for existing binary/PDF targets, allowed creation, two transport-failure modes, the unclassified live environment, and the resolver-failure tilde fallback. They do not place another writer between the successful absence probe and the eventual mutation. Your separate limitation about remote relative V4A headers also remains a coverage limit: _rewrite_v4a_patch_paths_for_host leaves those headers unchanged, so the absolute-path tests do not establish target equivalence for that surface. I am not claiming a separately reproduced V4A defect here.

Interlock and scope

#122663 by Liuzikaii is still open and addresses the same owning issue. These should be consolidated into one implementation, preserving the original report, reproduction, and tests, rather than landing two competing guard implementations. The stronger live-environment and fallback coverage here should be retained in whichever PR carries the final change. Remote read-baseline work under #56380/#56656 is related but is not a substitute for enforcing a create-only condition at the backend write.

This is a review of file-integrity protection inside the existing trust envelope, not a sandbox-escape or unauthorized-access claim. Current SECURITY.md §3.2 permits ordinary review of these heuristic/hardening improvements. Identity/routing, guard ordering, filesystem mutation, concurrency, and settlement were examined along the selected path. This diff does not add credential grants, a network ingress, or installer/build logic; those domains are not certified by this review. Backend replacement/reconnect, cancellation, symlink/hardlink races, packaged peers, and the complete multi-operation rollback matrix remain unverified.

Exact object and evidence limits

Reviewed head: 8b5eb26d57d7ef7d1d975de0ef5e1e8abd9a513d; PR-recorded base: e13b5e71ef7003389b0a01da1ff76e48f7fa9e89; verified merge base: 1f30bdbaff7926491188231686886173e51910be; observed main: d0288be5b3330d2442e3907185b8e9d0958297bb. The PR is open, non-draft, and reported mergeable. Main advancing is not itself a finding or a rebase demand.

This is source inspection, not a locally executed reproduction. The isolated checkout attempt failed at GitHub hostname resolution before obtaining source, so I ran no repository tests and do not adopt the PR body’s test runs as my own. Exact-head CI, Docker Build, Test, and Publish, and Nix flake check are action_required; the CI jobs endpoint returned no jobs, and combined statuses were empty. That is missing execution evidence, not passing CI or a demonstrated test failure. No independent 5×2×3 acceptance or real Docker/SSH/Vercel verification is claimed.

[andrexibiza]

P2 — Carry the absence condition into the write, rather than treating this snapshot as a lasting permission. This is a pre-existing concurrency gap exposed by the new helper, not a regression introduced by this PR.

Both consumers run the binary precheck before taking their path locks: write_file_tool and patch_tool. A concrete interleaving is: a remote .sqlite target is absent when this probe runs; a separate backend process creates a database at that path before write_file reaches its effect; the pending text write then replaces it. No hostile caller or sandbox escape is needed. The controller can still have no file at that path, so the host Path.exists() in _stale_overwrite_blocker is not a backstop. The lower writer’s probe and write sequence uses the observed file for BOM/line-ending handling, but does not consume an absence requirement. Its hash check can truthfully verify the replacement bytes while the database has been lost.

The violated invariant is narrower than a general freshness policy: permission to create a new conditional-binary target must not become permission to replace a file that appeared later. Resolve and retain the same backend/target through the operation, and make this branch request a backend-enforced atomic create-if-absent/no-replace operation. Rechecking under the existing path lock helps cooperating in-process writers, but does not serialize an independent process on the backend. Route the condition through the shared mutation implementation rather than adding separate ad hoc guards for each public tool.

For closure, add a deterministic barrier after the absence decision and before the effect; create a real database through the isolated backend; then release the pending write and assert pre-effect refusal, byte-identical database contents, and a successful SQLite integrity check. Retain the allowed-new-file control and cover the corresponding patch paths and normalized aliases. These are requested regressions, not tests I ran. Keep this gap explicit when consolidating #122663 and this PR under #122662; the namespace fix alone does not close it.

[kshitijk4poor]

Agreed: this is pre-existing, and #124106 doesn't close it. The PR body lists it under Not included. Closing it needs a backend-enforced create-if-absent write carried through the shared mutation path, rather than a re-probe under the path lock, which is a bigger change than this namespace fix. I'll file it separately.

[kshitijk4poor]

Thanks @JoaoMarcos44. Your commit lands through #124106 with authorship kept. Follow-ups there: a bad_size probe counts as an existing file, the tests are trimmed to two invariant functions (7 cases), and remote NEW binary creation is pinned as allowed. andrexibiza's P2 (the absence snapshot isn't enforced at write time) is pre-existing and listed under Not included in #124106 for a follow-up. Closing in favour of #124106.

[kshitijk4poor]

Merged: #124106 landed on main as 952f65c.