[haotianqi666]

Fixes #7298

Why

A failed LocalSandbox file revision can destroy the previous complete report: the destination is truncated before the replacement is written. A real per-process file-size limit reproduces this through both write_file and str_replace, even after reading the current file. Ordinary saves without the limit succeed.

What changed

• On Linux and macOS, local non-append text saves stage and fsync complete bytes before replacing the destination. A write, flush, metadata-transfer or pre-publication error leaves the previous file intact; a failed first save does not publish a partial destination.

• Check target write access without truncation. Preserve ownership, modes, ACLs and extended attributes using open descriptors before writing staging content; remove inherited staging grants not present on the original. Metadata-transfer errors abort rather than silently change access controls.

• Refuse set-id bits, macOS file flags and content-bound Linux privilege attributes. Atomic replacement needs parent-directory write access and permissions to preserve metadata. It creates a new inode, so hard-link aliases retain the old version; existing symlink/path rules are retained.

• Keep append, Windows/other hosts, shell writes and remote providers unchanged. Existing browser-output publisher callers retain their mode-only behavior. No dependency changes, concurrent-writer arbitration or power-loss durability claim.

• Review follow-up: keep the new revision's mtime (including empty saves); make source-descriptor close failures best-effort warnings without masking a saved result or the primary exception; compare native macOS ACL data instead of DirectoryService-dependent display names.

Surface area

• Frontend UI

• Backend API

• Agents / LangGraph

• Sandbox

• Skills

• Dependencies

• Default behavior change - local non-append file revisions on Linux/macOS

• Docs / tests / CI only

Screenshots / Recording

N/A: backend filesystem behavior; no UI changes.

Bug fix verification

• Regression suite: backend/tests/test_local_sandbox_atomic_writes.py.

• Before the production fix, the initial suite gave 10 failed / 15 passed on main e830c85adfc39c5526d3c896598c428caa3067eb: six loss-of-content/partial-file failures and four assertions for the proposed fsync/replace contract.

• The standalone real-kernel reproduction replaces a 54-byte existing file with 11,000 bytes under RLIMIT_FSIZE=1024. Main raises EFBIG and leaves 1024 partial new bytes; the fix raises the same error and retains all 54 old bytes.

• Tool-function cases execute real local reads, writes, path/size checks, locks and error conversion. Only sandbox acquisition/custom-mount lookup is substituted; this is not a full Gateway/graph run.

• Compatibility review caught an ACL regression in the first prototype. The corrected implementation passes real macOS ACL, inherited-grant, deny-write, xattr and private-staging checks, plus injected metadata-error cases. The failing checks were retained, not weakened.

• Keep stale upload-companion coverage for both same-inode in-place writes and new-inode sandbox saves. Both must reject a stale outline after a same-size source revision.

• Follow-up regressions on previous PR head c3db27fd7: 7 failed / 3 passed before the repair, then all passed. Failures cover the empty-save mtime and six source-close cases (success, ENOSPC and interruption, each with working/failing logging). Nonempty and large same-size rewrites are passing controls. Native ACL checks include removed/changed-permission controls; a separate macOS probe passed 30 real save/ACL round trips. The reviewer-reported name/UUID display flake was not independently reproduced.

Validation

Environment: macOS arm64, Python 3.12.14, locked backend dependencies; base e830c85adfc39c5526d3c896598c428caa3067eb.

• Related sandbox, browser-output and upload-companion suites: 537 passed / 1 skipped (the native Linux ACL/xattr case on macOS).

• cd backend && make test: 35,533 passed / 746 reported skips / 0 failed across all four default shards, exit 0.

• cd backend && make test-blocking-io: 275 passed / 1 skipped / 2 deprecation warnings, exit 0. The skip is the existing SQL-store-only import case.

• Whole-backend make lint: passed.

• git diff --check: passed.

• cd backend && make format: passed, all 2,286 files unchanged.

• python3 scripts/check_agent_guidance.py --base-ref upstream/main --head-ref HEAD: 0 errors / 2 soft size warnings for the sandbox AGENTS file and its inheritance chain; both remain under the hard limits.

• Untouched baseline, recorded separately: make test 35,474 passed / 745 reported skips / 0 failed; make test-blocking-io 275 passed / 1 skipped. These are not post-change totals.

• Before the review follow-up, unchanged PR head c3db27fd7 was tested again: 35,524 passed / 746 reported skips / 0 failed; blocking-I/O: 275 passed / 1 skipped. No test or skip condition was weakened for the follow-up.

• A first restricted local run could not retain a setuid bit on its disposable fixture. The same unchanged test passed outside the tool sandbox; subsequent validation uses that local test environment. No live APIs or user files are involved.

• Native Linux/Windows, Docker, live providers and deployed application tests have not been run. The native Linux metadata case is included for Linux execution; a local API-double test is not claimed as native Linux validation.

• Full-suite skips include existing optional services/platform cases plus the new native-Linux metadata test. An initial modified full run failed an old test's in-place inode assumption. It was expanded to retain the original same-inode case and cover the atomic replacement case; the complete suite was then rerun successfully. No checks were removed or weakened.

• Tested commit: 50c85ba5f over e830c85ad. Upstream advanced to 66508d752 during validation; the sandbox/publisher path, affected companion test, dependencies and templates are unchanged there. These totals describe the tested commit, not a full run on the newer upstream. Local validation is separate from GitHub CI.

AI assistance

Tool(s) used: Codex

How you used it: Assisted with source investigation and duplicate searching, reproducing the issue, implementing the fix and regression tests, checking file-permission compatibility, addressing review feedback, running local validation, and preparing documentation, issue/PR text and review replies.

• I've read and understand every line of this change and take responsibility for it - it's not unreviewed AI output.

[willem-bd]

Reviewed at head c3db27f (base e830c85). Verification performed against a worktree at the head commit with deerflow.* confirmed importing from the head tree.

Red/green reproduced. At base, the kernel-limit probe (RLIMIT_FSIZE + SIGXFSZ ignored) corrupts the destination through both write_file and str_replace: the previous report is replaced by a truncated prefix of the new content, plus leftover state. At head the same probe keeps previous report\n byte-identical, reports "File too large", and leaves no staging file. The mocked-stage tests (write/flush/fsync/replace ENOSPC) also cover cleanup and staging removal.

Family check — good. This reuses publish_bytes_atomically from #7200 (keyword extension with temporary_prefix and preserve_metadata_from) rather than adding another atomic-write variant; cleanup_failed_atomic_write, except BaseException, and the bare re-raise are preserved, and the primary publication error survives staging cleanup.

Mechanism review. Temp+os.replace on POSIX only; append, Windows, shell writes and remote providers unchanged, as documented. The O_WRONLY probe refuses read-only targets before any staging (parent-writability cannot bypass it), refuses non-regular files, and str_replace is one in-memory transform plus a single publish, so the whole edit is atomic (no per-hunk rollback window). Staging starts 0600 and metadata is installed before content, avoiding an exposure window. Per-path tool lock still serializes read-modify-write in-process; cross-writer arbitration and power-loss durability are explicitly disclaimed. Docs (README + both AGENTS.md) are updated in the same change set. test_upload_companions.py correctly encodes the new-inode consequence — companion identity includes ino/ctime, so stale outlines are still rejected despite same size.

Test run on macOS (darwin): 49 passed, 1 skipped in the new file, plus the modified upload-companion tests green. Three findings below, none blocking.

[willem-bd]

This os.utime is installed before the staging content is written, so for any non-empty save the subsequent handle.write(content) immediately bumps mtime again and the restoration never survives (verified empirically: a same-size rewrite leaves st_mtime_ns at the write time, not the previous version's). Only an empty save keeps the old mtime — producing a new inode whose content changed but whose mtime claims the previous write, which breaks mtime-based change detection (e.g. workspace_changes/diff.py::_same_file falls back to size+mtime_ns for files above 256 KiB).

Since neither the PR description nor the README claims timestamp preservation, I'd suggest dropping this line so rewrites behave like the in-place writes they replace (mtime bumped). If timestamp preservation is actually wanted, it must be installed after the write — and then the >256 KiB same-size blind spot in workspace-change detection needs addressing too.

[haotianqi666]

Addressed in 50c85ba. Thanks, removed the timestamp restoration. Revisions now keep their new filesystem mtime, including empty saves. Added empty, nonempty and >256 KiB same-size rewrite cases: the empty-save case fails on the previous PR head and all three pass with this change. Ownership, modes, ACLs and xattrs are still copied before content; timestamps are not part of that preservation contract.

[willem-bd]

Unguarded os.close(fd) in the finally can mask the primary publication error — exactly the bug class #7200 fixed and that this PR's own utils/AGENTS.md addition warns about ("a close failure must not prevent the unlink attempt"). If staging fails with ENOSPC and the source-descriptor close then fails (e.g. NFS close flushing dirty pages for this inode returns EIO), the caller sees the close error and the ENOSPC is lost. On the success path a close failure would also report failure after the replacement already published, contradicting "failure leaves the previous file intact". cleanup_failed_atomic_write guards its own descriptor close with except OSError + warning; the same treatment here would keep the publication error authoritative.

[haotianqi666]

Addressed in 50c85ba. Thanks, source-descriptor close OSErrors are now best-effort warnings, with logging exceptions guarded as well. The new fault-injection tests cover ENOSPC, interruption and successful publication, each with working and failing logging. All six fail on the previous PR head and pass after the change; they assert the original exception object or successful result, destination bytes, staging cleanup and a single source-close attempt. This does not suppress errors from writing, flushing, fsync or replacement.

[willem-bd]

Exact ls -le string equality depends on DirectoryService resolving the ACL entry's GUID back to user:nobody. In one full-suite run on macOS this assertion failed with the entry printed in raw-UUID form (FFFFEEEE-DDDD-CCCC-BBBB-AAAAFFFFFFFE deny read vs user:nobody deny read); a direct 30-iteration probe of the same rewrite shows the acl_get_fd_np/acl_set_fd_np round trip itself is byte-stable, so the flake is in ls's name resolution under load. Consider asserting the ACL via acl_to_text/entry semantics (tag, perms, resolved qualifier) or tolerating the UUID spelling, so loaded macOS machines don't produce red CI for a green transfer.

[haotianqi666]

Addressed in 50c85ba. Thanks, replaced the ls -le string comparison with an independent native ACL reader (acl_get_file + acl_copy_ext), so username/UUID display resolution is no longer involved. The test verifies that an ACL was actually installed, compares it after the save, and includes removal and changed-permission controls. A separate macOS probe also passed 30 real save/ACL round trips. The permission assertions remain intact; no skip or tolerance was added.