fix(tools): atomic writes and ACP/compaction file-tool continuity

[Yeachan-Heo]

What

Fix independently reproduced file-tool defects from #4734:

• Write publishes through exclusive sibling temp + rename so a failed write cannot leave a 0-byte destination. Destination symlinks are followed so the referent is replaced and the link stays.
• Permission errors (EACCES/EPERM/EROFS) become an actionable ToolError. Atomic-helper failures state the original file was unchanged; later formatter-path failures do not claim that.
• Read consults the ACP readTextFile buffer when disk stat misses a just-written path, using the same truncation route as the disk path.
• ACP reads fail closed for ambiguous EACCES/EPERM errors; disk fallback is reserved for an explicit transport_unavailable bridge result, including during structural summarize.
• Successful writes invalidate fileReadCache.
• Compaction-state lists recent successful write/edit paths and does not treat those mutations as unfinished work (independent of refactor(workflows): make Ralplan and Ultragoal validation compaction-resilient and risk-proportional #4560).

Why

Reporter on @gajae-code/coding-agent@0.14.1 Linux saw Read report Path ... not found for Write-created files that ls/cat could see, Write fail with EPERM and leave a 0-byte file, and long-session compaction lose recent file-tool context.

Three hypotheses were separated and fixture-tested:

1. ACP file-bridge path/cache/negative lookup — Read required a disk stat before the editor buffer, and treated OS EPERM as a client denial (no disk fallback). Summarize also swallowed denials.
2. Write atomicity — Bun.write truncates then copies; a failure after truncate leaves a 0-byte file. Not Windows directory-fsync EPERM (Coordinator MCP: all mutating tools fail with EPERM fsync on Windows (directory fsync unsupported) #4457); user-file writes still do not fsync the parent directory.
3. Compaction continuity — keep-recent still holds the last window, but compaction-state did not name recently mutated files. Separate from ralplan/ultragoal validation (refactor(workflows): make Ralplan and Ultragoal validation compaction-resilient and risk-proportional #4560).

Shell fallback is not the product fix.

Testing

Exact head 8a3d7d959d, base 4d39d58391 (live dev):

• five-property coding-agent: file tools intermittently fail (Read 'not found' on fresh files, Write EPERM on Linux) and session appears to lose context mid-task #4734 harness: all green (digest c1b425f7)

• file-tools-atomicity + read-acp-fs + write-acp-fs + state-aware-compaction + lsp-batching + mcp-autoload-precedence: 84 pass / 0 fail

• bun --cwd=packages/coding-agent run check: exit 0

• bun test packages/coding-agent/test/file-tools-atomicity.test.ts

• bun test packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts (including ACP denial, transport fallback, and missing-disk regressions)

• bun test packages/coding-agent/test/tools/lsp-batching.test.ts (including BunFile targeting and publication-state diagnostics)

• bun test scripts/ci-dev-affected.test.ts (affected CI routes ACP/file-tool suites)

• bun test packages/coding-agent/test/agent-session-state-aware-compaction.test.ts

• bun --cwd=packages/coding-agent run check

Risk classification

• low-risk — ordinary fix/maintenance; the repository owner may use the explicit merge-self-approved solo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.
• regression-risk — fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-head APPROVED review the gate verifies (extra:independent:<login>; the token alone never suffices).
• high-risk — large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-head APPROVED review (extra:independent:<login>).

GJC verdict

gajae.pr-review-verdict.v1 merge-approved sha256:4bdc4d3386bee5cf942476e8c5d050a92d148e4c96bb15fe6375473eb60873d9 reviewer:human reviewer-id:probepark evidence:exact-head-e5ff5192-atomic-write-and-lsp-files-byte-identical-to-approved-10a79d9b-delta-is-rebase-uptake-of-merged-4778

---

• Target branch is dev
• bun --cwd=packages/coding-agent run check passes
• Tested locally
• CHANGELOG updated (if user-facing)
• Verdict above matches the exact PR head, not an earlier commit
• Risk classification above matches the actual review path taken

Fixes #4734

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Yeachan-Heo]

Follow-up 0b23d78 addresses architect BLOCK findings on #4735: ACP summarize now rethrows permission_denied; atomic write follows destination symlinks and exclusively creates the sibling temp; missing-disk ACP reads use the same truncation route as disk; compaction mutations do not authorize auto-continue. Independent exact-head GitHub approval still required (author cannot self-approve).

[Yeachan-Heo]

Owner evidence for exact head 38934ade8a241ae16f7d9f6a7c1058864e2320a0 against immutable base 02c739e1ba19efed9f1fb69a01eedc986915e712:

• Worktree is clean and exactly matches the live PR branch.
• bun --cwd=packages/coding-agent run check passed; bun scripts/verify-gjc-state-writers.ts --fail passed.
• Focused current-head tests passed: file-tools atomicity (11), state-aware compaction (20), ACP read/write/capability tests (8), and read/write tool regressions (31).
• Audit coverage includes exclusive temp creation/cleanup, symlink referent semantics, formatter-path error honesty, ACP denial vs OS fallback, truncation-route parity, successful multi-path mutation continuity, and no auto-continue from mutation hints alone.

Three eligible human reviewers remain requested: HaD0Yun, probepark, and snowykr. No release work has been performed.

[Yeachan-Heo]

@probepark Please provide an independent approval or blocking review for exact head 38934ade8a241ae16f7d9f6a7c1058864e2320a0. The owner audit and focused checks are recorded above; the current verdict remains needs-human until your exact-head review is submitted.

[Yeachan-Heo]

CI exact-head validation recomputed the canonical base...head digest as 6047b72c3fb0b963321f0e43d6cd141056db3a2c362b9bd9045e20b626de9ef7; the single PR verdict was regenerated to that digest without any code commit. Head remains 38934ade8a241ae16f7d9f6a7c1058864e2320a0 and the three human reviewer requests remain unchanged.

[snowykr]

Verdict

CHANGES_REQUESTED

Summary

Reviewed exact head 38934ade8a241ae16f7d9f6a7c1058864e2320a0 across all required axes. The recent formatter-diagnostic fix resolves the prior false “destination unchanged” report on that path, but the atomic-write and ACP changes still introduce authorization, confidentiality, and data-integrity regressions.

Findings / Required Changes

1. [P1] Preserve ACP provider denials before disk fallback. packages/coding-agent/src/sdk/host/reverse-leases.ts:280-285 stores a provider error code in Error.name, while packages/coding-agent/src/tools/read.ts:1437-1458 checks only .code and message text. A reverse-provider { code: "permission_denied", message: "Request rejected" } can therefore be treated as an availability failure and fall back to Bun.file(...), exposing an on-disk file despite the provider’s refusal. Preserve the structured code (or classify the canonical error name/nested data) and add a reverse-bridge denied-read regression with a disk-backed secret.

2. [P1] Do not bypass existing-file write protection through rename. packages/coding-agent/src/tools/atomic-file-write.ts:126-135 stages a sibling inode and renames it over the target. On POSIX, a writable parent permits this even when the destination itself is 0444 or ACL-denied, where the prior direct write would fail. Preserve effective write/immutable authorization on the resolved referent before publication, including platform-appropriate Windows handling, and cover a writable-directory/unwritable-target case.

3. [P1] Keep archive writes atomic or narrow the stated contract. packages/coding-agent/src/tools/write.ts:279,308 still publishes rebuilt ZIP/TAR bytes directly. A failed existing-archive write may truncate or corrupt it, while the PR’s file-tool/changelog contract says failed writes do not leave a zero-byte destination. Stage archive bytes and publish through the guarded atomic path, or explicitly scope the guarantee in the implementation, docs, and changelog.

4. [P1] Enforce the /var/folders/cp/9506bhz103gc1rg1k4xq3vcw0000gn/T/gjc-local/01a01add-01a3-7000-96fd-0fb0a49e80bc trust boundary after symlink resolution. packages/coding-agent/src/tools/atomic-file-write.ts:98-103,128-135 follows final and parent symlinks without validating realpath containment. A link inside a session-scoped local root can redirect a write (including newly-created parents) outside that root. Validate resolved parent/target containment and revalidate identity immediately before publication, or route /var/folders/cp/9506bhz103gc1rg1k4xq3vcw0000gn/T/gjc-local/01a01add-01a3-7000-96fd-0fb0a49e80bc writes through its authorized handler.

5. [P2] Always remove owned staging files after publication failure. At packages/coding-agent/src/tools/atomic-file-write.ts:111-141, a failure after staging succeeds (for example an exhausted Windows sharing violation) leaves a hidden sibling temp containing the attempted content. Clean owned temps in a finally whenever publication does not succeed; preserve genuine collision files and add retry-exhaustion coverage.

6. [P2] Restore exact destination modes and guard symlink retarget races. fs.open(..., existingMode) at atomic-file-write.ts:111-129 is umask-masked and never followed by an exact chmod, silently narrowing existing permissions. The separately resolved link chain at :97-104 is also not revalidated before rename. Restore mode bits after staging and validate parent/target identities before publishing.

7. [P2] Retain move destinations in compaction context. packages/coding-agent/src/session/agent-session.ts:499-512 extracts source paths but drops edits[].rename and parsed apply_patch entry.rename. After a successful move, compaction retains the stale source but not the live destination. Include both paths with existing deduplication/cap behavior and add edit/apply-patch rename coverage.

8. [P2] Complete exact-head verification. The new file-tool tests do not exercise formatter-first-publication/second-failure, real temp-collision preservation, ACP selector/truncation routing, or move destinations. Add those cases. Dev CI #14087 is still waiting, so there is no completed exact-head test-matrix result for these changes.

CI / Verification

• Public site sync #10038: succeeded; Local public surfaces succeeded, and the deployed-release-state job was skipped.
• Dev CI #14087: waiting behind prior Dev CI; no completed exact-head matrix evidence yet.
• PR contract #1652: failed only on the intentional needs-human verdict gate / stale verdict digest; ignored under the review policy.
• This review used source, diff, caller, test, and GitHub CI-artifact inspection. No PR code, test suite, build, or formatter was executed by the review lanes.

Axis Coverage

Axis	Status	Review outcome
A1 — Intent / Policy / Contract	Block	Atomic-write/error contracts remain incomplete; compaction omits rename destinations.
A2 — Architecture / Correctness / Failure	Block	Reverse-denial fallback, atomic publication lifecycle, mode, and symlink-race defects remain.
A3 — Security / Privacy / Trust	Block	ACP-denial bypass, protected-file replacement, /var/folders/cp/9506bhz103gc1rg1k4xq3vcw0000gn/T/gjc-local/01a01add-01a3-7000-96fd-0fb0a49e80bc escape, and temp-content leakage require correction.
A4 — Verification / Tests / CI	Watch	Relevant tests exist but have material gaps; exact-head Dev CI has not completed.
A5 — Context / Compatibility / Platform	Block	Archive non-atomicity and POSIX/Windows permission and filesystem compatibility are unresolved.

[Yeachan-Heo]

Fix-forward is pushed at exact head 110a1e5449385df376891d7eff9b6a443a0a71fd. It addresses the blocking review findings: structured ACP denial preservation, existing-target authorization, atomic ZIP/TAR publication, gjc-local trust-boundary and retarget checks, staging cleanup, exact mode restoration, and compaction move destinations. Focused tests and bun --cwd=packages/coding-agent run check pass locally. The single canonical verdict was regenerated for the new head. @probepark @HaD0Yun please re-review this exact head.

Superseded by fix-forward commit 110a1e5; current accountable reviewers are probepark and HaD0Yun.

[Yeachan-Heo]

Exact-head product CI is now complete for 110a1e5449385df376891d7eff9b6a443a0a71fd: 22 current jobs succeeded, including native build, all state gates, affected tests, ACP read, atomicity, compaction, TypeScript, CLI smoke, evidence, and virtual integration. The only current failure is the intentional PR contract bootstrap needs-human gate. @probepark @HaD0Yun please submit an exact-head approval or blocking review.

[Yeachan-Heo]

Reminder for the current exact head 110a1e5449385df376891d7eff9b6a443a0a71fd: product CI is green (22 successful jobs); only the intentional contract gate awaits an independent exact-head approval. @probepark @HaD0Yun please review and approve or post any remaining blocker.

[Yeachan-Heo]

Current exact head remains 110a1e5449385df376891d7eff9b6a443a0a71fd; product CI is green and the superseded changes-requested review is dismissed. @probepark @HaD0Yun please provide the required independent approval or any remaining blocking finding so the merge gate can be completed.

[probepark]

Independent maintainer review at exact head 110a1e54 — merge blocked. Atomic visibility is right; atomic durability and identity are not.

the checklist

I walked the standard atomic-write requirements one at a time:

requirement	status
exclusive temp creation	yes — wx with EEXIST retry
same-filesystem rename	yes — sibling staging, no EXDEV copy fallback
mode preservation	yes
durable flush before rename	no
ownership preservation	no
cleanup on every failure path	no

Symlink handling is deliberate and correct — resolve and replace the referent while retaining the link, with a real test. That is the behavior users who symlink config files expect, and it is good that it is pinned.

major 1 — no fsync before rename

atomic-file-write.ts:132-141. The staged file is closed and renamed with no sync()/datasync(). Rename gives old-or-new visibility, but not durable bytes — after a crash the new name can resolve to unflushed content. The module advertises a crash-atomic contract, so this needs a sync of the owned temp before rename, and a platform-gated parent-directory sync if durable name publication is promised.

major 2 — ownership is silently changed

:114-128,231-248. Only mode is captured and restored. The new inode belongs to the writing process, so uid/gid change silently on every write. For a root-owned or shared-group file that is a real behavior change from the previous direct write. Capture and apply ownership, or reject when preservation is impossible.

major 3 — hardlinks are split

:231-248. Renaming over a hardlinked destination breaks the link group: other names keep the old inode. Direct writing did not do this. Detect nlink > 1 and either reject or define a supported update path, with a real hardlink test.

major 4 — check-then-commit on the pathname

:193-248. Writability and destination intent are validated, then a later rename commits against the name. A concurrent actor can install a new regular inode at that name in between; string equality still passes and stale mode/authorization is used to overwrite the successor. Bind validation and commit to identity, or reject on changed identity. Needs a real swap/two-writer test.

major 5 — realpathOrSelf flattens every error

:148-181. Every realpath failure — including EACCES and EIO — degrades to the lexical path. This is the error-distinction collapse that has been a blocking finding on several PRs here. Only ENOENT may fall back; everything else must propagate or fail closed, especially since this feeds trust-boundary validation.

major 6 — temp leak on close failure

:132-141,237-257. If handle.close() fails, the outer owned flag is still false and the exclusive temp leaks; unlink failures are also swallowed. Track ownership immediately after open, clean in finally, and keep cleanup-failure evidence.

minor

tools/write.ts:763-770: every writethrough failure is formatted with destUnchanged: false, so direct atomic failures never produce the body's promised "original unchanged" message. Reserve the weaker wording for the multi-publication formatter and assert the exact direct-path message.

session/agent-session.ts:493-561: this is several independently testable changes — atomic disk/archive/LSP publication, ACP missing-path/error routing plus reverse-lease code propagation, cache invalidation, and compaction mutation summaries. Split them, especially with the head conflicting.

coverage

Real pins: new parent creation, failure injected at temp open (leaves existing file unchanged, no 0-byte destination), read-only directory with a genuine POSIX permission state, ACP buffer missing on disk, OS EPERM fallback, structured denial, cache invalidation, actual symlink preservation, POSIX mode preservation, trust-boundary symlink escape, and cleans an owned staging file when publication fails — which does inject rename failure after content staging, so it covers write-to-rename.

The EEXIST retry is mocked without a real colliding file or concurrent writer. Missing entirely: fsync/durability, uid/gid, hardlinks, close failure, unlink failure, non-ENOENT realpath failure, destination identity swap, a real two-writer test, and a reader racing publication. Each maps to a major above.

merge precondition

Conflicts with dev.

_{Reviewed by @probepark — method: detached worktree at 110a1e54, requirement-by-requirement audit of the atomic-write sequence, ownership/hardlink/identity analysis against the previous direct-write behavior, error-classification sweep of realpathOrSelf, failure-path ownership tracking, per-test pin analysis. Tests not executed.}

gajae.pr-review-verdict.v1 merge-blocked sha256:c5092dff4ce460b10d0e431bfafb15a920bfb839505bd19d7587a7cf46115e4d reviewer:human reviewer-id:probepark evidence:exact-head-110a1e54-no-fsync-before-rename-ownership-and-hardlinks-lost-and-commit-binds-to-pathname

Superseded by fix-forward e913a07, which addresses the atomic durability, ownership, hard-link, identity, realpath, cleanup, and direct-error findings.

[Yeachan-Heo]

Fix-forward is now at exact head e913a0769e0ad54fac62be3eb7be65acf8c557bd, rebased onto current dev d97b79eff2be5f25bf3ae253de72430e2e2fab1b. It addresses the review checklist: temp sync() before rename, uid/gid preservation, hard-link rejection, destination/parent identity revalidation, non-ENOENT realpath propagation, owned-temp cleanup after close/unlink failures, and direct-write unchanged wording. Focused local checks pass: coding-agent check, 21 atomicity tests, 22 compaction tests, and 38 ACP tests. @probepark please re-review this exact head.

[probepark]

Re-review at exact head e913a076 — merge blocked. Four of six majors are properly fixed; the check-then-commit race remains, and two tests do not prove what they claim.

fixed

fsync before rename — atomic-file-write.ts:300-303: await handle.writeFile(content); await handle.sync(); then rename at :312. Parent-directory fsync is explicitly not promised in the contract, which is the honest scoping.

non-ENOENT realpath — :169-175 now rethrows: if (!isEnoent(error)) throw error, with an EIO injection test. That closes the trust-boundary error collapse.

temp leak on close failure — owned = true is set immediately after exclusive open, so a close failure reaches cleanup, and cleanupOwnedTemp() rethrows non-ENOENT unlink failures as an AggregateError (:249-257).

pre-existing hardlinks — rejected via existing.nlink > 1 (:286-290), pinned with a real fs.link.

major — the final gap is still open

:245-312. const after = await resolvePublishPath(dest) and the dev/ino checks complete, and only then does await renameIntoPlace(tmp, publishPath) run. A writer that installs a successor between validation and rename still gets overwritten.

The added final identity check narrows the window but does not close it — the commit is still a pathname rename, so validation and commit remain separate operations on a mutable name.

sameFileIdentity() at :237-239 also compares only dev/ino, ignoring uid/gid/mode/nlink. So a same-inode authorization change, or a hard link added during staging, passes validation and is then split at rename. That is why I am carrying the hardlink finding as partially fixed rather than fixed.

Needs a genuinely conditional publication primitive, or a narrower documented overwrite contract that admits the race.

The new race test does not exercise this. It swaps paths synchronously inside mocked fs.chmod — before final validation — so it pins the earlier window, not the remaining one. A coordinated writer that swaps exactly after validation and before commit is the test that would close this.

minor — the "original unchanged" message now lies in the other direction

tools/write.ts:775-779. Flipping the catch from { destUnchanged: false } to { destUnchanged: true } fixes direct atomic failures, but src/lsp/index.ts:936-941 publishes content before fallible formatting and a second publication. A failure after that first publication now reports "The original file was left unchanged" when it was not.

All-false was wrong and all-true is wrong in the opposite direction. Carry publication state through writethrough and distinguish pre-commit from post-commit failure.

minor — the ownership test is vacuous

test/file-tools-atomicity.test.ts:235-351. Both inodes are created by the same process, so uid/gid already match and the assertion passes against the old implementation too. It does not prove chown runs.

The source fix is correct (:151-152 captures uid/gid, :241-247 calls fs.chown then fs.chmod) — it just is not pinned. Needs a distinct-owner fixture or controlled metadata injection. Unlink failure is also still untested, and there is no sync-failure test.

minor — cohesion, unchanged

session/agent-session.ts:493-576: tool-schema parsing, success correlation, sanitization and compaction rendering still live in the 21k-line AgentSession coordinator. Extract a compaction-state/mutation collector.

still correct

Exclusive wx with EEXIST retry; sibling staging so the rename is same-filesystem with no EXDEV fallback; mode preservation; symlink referent replacement retaining the link, with a real symlink test. The rename-failure and close-failure tests genuinely inject between write and rename.

_{Reviewed by @probepark — method: detached worktree at e913a076, re-verification of each prior major at its current line, ordering analysis of resolvePublishPath against renameIntoPlace, field-level inspection of sameFileIdentity, trace of the destUnchanged flag through the LSP two-publication path, and per-test hostility check for ownership, unlink and swap timing. Tests not executed.}

gajae.pr-review-verdict.v1 merge-blocked sha256:ef44c4a4dd15131b56a062e0e7c884eb305fe8168d38ac8cb0ee2a48c2fb0ca8 reviewer:human reviewer-id:probepark evidence:exact-head-e913a076-validation-to-rename-gap-remains-and-race-test-swaps-before-validation

Superseded by exact-head fix-forward 917b63b; please review the current head.

[probepark]

Re-review at exact head 190f07e4 — merge blocked. Both prior minors are fixed; the race moved into the native primitive rather than being closed, and the new boundary leaks debris into user directories.

fixed

sameFileIdentity() now includes nlink, mode, uid and gid (atomic-file-write.ts:270-278). The formatter message is correct: runLspWritethrough() records publishedContent = true after the first publication and wraps a later FileWriteNotPublishedError with { destUnchanged: false } (src/lsp/index.ts:920-931). The ownership test now falsifies staged uid/gid through the fs.stat seam and asserts fs.chown receives the destination ownership (file-tools-atomicity.test.ts:263-290), and sync-failure cleanup (:399-415) and non-ENOENT unlink/AggregateError behavior (:448-468) are pinned.

Nothing previously fixed regressed: handle.sync() before publication, realpathOrSelf() rethrowing non-ENOENT with its EIO test, owned = true right after exclusive open, initial nlink > 1 rejection, sibling wx with EEXIST retry, chown/chmod restore, deliberate symlink referent following.

major 1 — exchange-then-validate is not conditional publication

atomic-file-write.ts:405-410 calls exactReplacePath(tmp, publishPath, expectedSource, expectedDestination). In crates/pi-natives/src/path_identity.rs:4605-4626 that validates identity, runs rename_exchange(...), and checks identity only afterward.

The repository's own native race test states the outcome (path_identity.rs:10136-10145): after a concurrent pre-exchange substitution, result.ok is false — but fs::read(&destination) equals b"successor", and the concurrent b"attacker-destination" has been displaced to a private path.

So the call reports failure while the canonical pathname already holds the staged bytes and the successor is gone. A concurrent hard link added in that gap is likewise detected only after the canonical name has been split from its link group. That is the same check-then-commit defect, relocated from JS into the primitive.

The publication protocol needs a mismatching successor to remain at the canonical pathname. Exchange-then-validate cannot provide that.

Note the new TypeScript pin (file-tools-atomicity.test.ts:331-351) swaps inside the JS mock before the real primitive is called, so it exercises pre-validation rejection rather than the in-primitive window — which is why the native test is the one showing the real behavior.

major 2 — every overwrite leaks hidden protocol files into the user's directory

Routing all existing-file writes through the managed-state exactReplacePath primitive brings its artifacts along. Its own success test expects three sibling remnants after one replacement (path_identity.rs:10049-10056), and packages/coding-agent/CHANGELOG.md:291 documents that these zero-length .gjc-exact-* remnants grow without bound, observed at 83k directory entries.

Cleanup exists for managed scope preparation, not for arbitrary source directories. So an ordinary user editing files in their own repo accumulates hidden debris indefinitely — user-visible harm from a primitive chosen for a different context.

Either use a publication primitive that removes its success artifacts for user files, or provide bounded cleanup for user directories before enabling this path.

_{Reviewed by @probepark — method: detached worktree at 190f07e4, followed the JS publication call into the native primitive to establish the actual ordering, read the native race and success tests as the authority on observable behavior, and checked the debris claim against the changelog entry describing it. Tests not executed.}

gajae.pr-review-verdict.v1 merge-blocked sha256:2b8d71210ad0873ec3f1c52e79e1966c6173c9cb9b91546792c5858771398371 reviewer:human reviewer-id:probepark evidence:exact-head-190f07e4-native-exchange-then-validate-displaces-a-successor-and-leaks-three-remnants-per-overwrite

[snowykr]

Summary

Current lane findings require changes.

Scope and Evidence

• head: 190f07e
• base: dadaf19
• evidence digest: a7cbfe214f317314a98d134adf2195499c667b2c339b4db542ebf18b4b192246
• Repository policy permitted review before all checks passed; current checks were not all passing.
• Controller synthesis decision: CHANGES_REQUESTED.

Findings

1. [P1] The atomic writer performs JavaScript-side destination and parent identity checks before invoking the native publication primitive; correctness under concurrent rename/replace depends entirely on the native exactReplacePath implementation honoring both expected identities atomically. The diff does not provide enough implementation evidence here to verify that guarantee across platforms.
2. [P2] Agent-session compaction now records recent file mutation paths, including absolute or user-supplied paths, in compaction context. This can disclose sensitive workspace structure, filenames, or local paths to model/context consumers; consider redacting, relativizing, or explicitly restricting these paths before persistence or transmission.
3. [P2] CI summary is empty, so there is no brokered evidence that the added atomic-write, ACP fallback, compaction, or LSP batching tests passed.
4. [P2] Recent file mutation collection scans only a bounded suffix of prior assistant messages and caps paths at 12. This is appropriate for prompt size, but concurrent or long-running sessions can omit older still-relevant mutations; the diff does not show a reconciliation mechanism with current filesystem state.
5. [P2] The atomic writer introduces platform- and filesystem-dependent native replacement paths, symlink handling, metadata preservation, and cleanup behavior; without executed CI results, regressions in these paths cannot be ruled out.
6. [P2] The non-native fallback path is intentionally not used for existing-file replacement, but the reviewed evidence does not show a clear behavior for environments where the native binding is unavailable or where its operation returns an uncertain state. Such cases may fail closed, but availability and recovery behavior should be verified explicitly.

Lane Results

Axis	Coverage
A1	API and compatibility boundaries across changed file-tool, ACP/LSP routing, compaction-state, reverse-lease, and atomic-write paths; reviewed listed tests and documentation changes.
A2	Manifest, complete bounded diff at offset 0, PR metadata, and CI summary were read. Reviewed atomic file publication, symlink/trust-boundary checks, concurrent replacement handling, read fallback routing, cache invalidation, compaction mutation tracking, and related tests.
A3	Security and privacy review of changed file read/write routing, atomic writes, symlink and trust-boundary enforcement, ACP error handling, session compaction metadata, and related tests.
A4	Reviewed all manifest entries, the bounded PR diff from offset 0, PR metadata, and the complete brokered CI summary; assessed changed tests, atomic file writes, ACP read routing, cache invalidation, compaction context, LSP batching, and observable regression risk.
A5	Integration, documentation, platform-specific filesystem behavior, ACP read/write fallback behavior, atomic publication, symlink and trust-boundary handling, cache invalidation, and related tests.

Limitations

• limitation: CI summary was empty, so no CI result was available.
• limitation: Review was limited to the brokered manifest, diff, metadata, and CI summary; no additional repository files or runtime behavior were examined.
• limitation: CI summary broker response was empty; no test or build result was available.
• limitation: The brokered diff was reviewed as supplied; native binding implementation details outside this diff were not available.
• limitation: No runtime execution or stress testing was performed.
• limitation: CI summary was empty, so no CI results were available to corroborate the review.
• limitation: Only the brokered manifest, bounded diff, PR metadata, and CI summary were examined; runtime behavior and unchanged surrounding code were not independently executed or inspected.
• limitation: No CI jobs, pass/fail statuses, logs, or test execution output were available in the brokered CI summary.
• limitation: Review was limited to the manifest, bounded diff, PR metadata, and CI summary; no additional repository files were read.
• limitation: CI summary contained no reported checks or results.
• limitation: Review was limited to the brokered manifest, bounded diff, metadata, and CI summary; runtime execution was not performed.

Review Decision

Verdict: CHANGES_REQUESTED

[Yeachan-Heo]

Issue-owner coordination from the #4734 lane (@Yeachan-Heo, cc @probepark @snowykr). I own #4734's closure, not this PR's implementation — I am not pushing code here. But this PR is the sole fix path for #4734 and it has been nonterminal since 190f07e4 (2026-08-20T06:53Z) with a base that is now three merges stale, so I am stating the acceptance bar for issue closure concretely instead of waiting.

The two blockers are real — I verified them in this repo's own tests, not from the review text

crates/pi-natives/src/path_identity.rs, exact_replace_path_reports_both_mutated_names_after_pre_exchange_substitution asserts, after a concurrent pre-exchange substitution:

assert!(!result.ok);
assert_eq!(result.code.as_deref(), Some("identity_mismatch"));
assert_eq!(fs::read(&destination).expect("read committed destination"), b"successor");

Failure reported, staged bytes already at the canonical name, concurrent content displaced to a private path. The ordering in platform::exact_replace_path confirms it: pin identity → rename_exchange → pause_exact_replace_after_exchange_for_test() → re-check. Validation after commit cannot decline a commit. No amount of narrowing the JS-side window in atomic-file-write.ts:373-395 changes that, because the commit happens inside the primitive.

And the success path asserts its own residue:

assert_eq!(retained.len(), 3, "only scrubbed internal placeholders may remain");

CHANGELOG.md:298 already documents this debris class unbounded — observed at 83k dirents — with reaping scoped to managed scope preparation, not arbitrary directories. atomic-file-write.ts:405 routes every existing-file user write through it. That exports a known unbounded-debris failure into every repo a user edits.

Net for #4734: this trades "0-byte file on failure" for "three hidden files per successful save, plus a destination that can still be clobbered by a call that reports failure". That is not a resolution of the issue I own.

Required shape for #4734 to close

Same-directory staged write + plain rename(2). Drop exactReplacePath from the user-file path entirely. Two properties, both of which I verified empirically on this baseline (Linux 5.15 x64, ext4, Bun 1.3.14):

=== failure injected mid-write (ulimit -f 0) ===
PUBLISH FAILED: EFBIG: file too large, write
cleaned owned temp
dest size: 30 content: "ORIGINAL CONTENT MUST SURVIVE\n"
dir entries: ["target.txt"]
=== success path ===
PUBLISH OK
dest size: 12 content: "NEW CONTENT\n"
dir entries: ["target.txt"]

Destination byte-identical on failure. Zero residue on success. That is the whole requirement, and open(wx) → writeFile → sync → chmod → rename → unlink on failure in finally already delivers it — which is exactly what atomic-file-write.ts:361-370 and :424-427 already do correctly today. The only change needed is replacing the :405 native-exchange publication with the rename you already use for the create path's semantics.

On the race that motivated the exchange: a plain rename(2) overwriting a concurrent successor is a strictly smaller harm than the current primitive, which overwrites the successor and reports success-shaped failure and leaves debris. Write is a last-writer-wins tool by contract; it does not need identity-pinned managed publication. If you want the race documented rather than eliminated, scope it explicitly in docs/tools/write.md — a documented last-writer-wins overwrite is honest and closes #4734. An undocumented commit-then-fail is neither.

Do not regress these two — they are the parts that already fix #4734

• ACP-buffer read ordering (read.ts). On dev at 6abb2260b8 I reproduced the reporter's exact failure verbatim: ToolError: Path 'frontend/e2e/zzop-repro.spec.ts' not found with a readTextFile-capable bridge holding the content, because read.ts:2661 stats disk before the bridge at :2394 is ever reachable. Your fallback ordering plus the structured-denial preservation in reverse-leases.ts is correct — keep it, and keep OS-errno fallback distinct from permission_denied.
• Mutation compaction continuity (agent-session.ts). #compactionStateSnapshot() on dev carries exactly five fields, none naming mutated files, so the summarizer is never told what was just written. Your addition is the right fix and must keep not authorizing auto-continue from mutation hints alone.

Escalation, not passive wait

Base is dadaf198; origin/dev is now bc460bfa2b, and mergeable currently reports UNKNOWN after previously being CONFLICTING. None of the three merges since your base touch read.ts, lsp/index.ts, write.ts, or agent-session.ts, so the conflict is mechanical, not semantic — rebase onto current dev and it clears. Please fix-forward against current dev rather than the stale base, since exact-head CI and both reviewers' verdicts have to be recomputed anyway.

HaD0Yun remains the only outstanding review request. @probepark @snowykr your findings are the gate here and I have independently confirmed both; a fix-forward that removes the exchange from the user-write path should clear major 1 and major 2 together.

I will re-run all three #4734 reproductions against the exact dev merge SHA once this lands and close the issue on that evidence. No release, tag, or publish work in this lane.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

Fix-forward pushed at exact head 221c21df6f6414f6f0740a77210d184315d424fe, rebased onto current dev 45032be6d05e78d2c3584301a00f5de123d417b8.

Addressed all four fresh findings: guarded archive parent creation, opened-handle Windows fallback identity checks with deterministic post-open race coverage, corrected ZIP/TAR atomic reconstruction docs, and planner existence validation with regression coverage. Focused harness: 146 pass. Please review and approve only this exact head.

Superseded by fix-forward exact head 221c21d; request fresh review on the current head.

[Yeachan-Heo]

Final fix-forward head is 10a79d9b943a7838e990d00fc672d39ba28c5e90, rebased onto current dev c89da3f38a24daab6b573833a9303bbe5bfb3aaa. The four snowykr findings remain fixed and the focused harness is 146/146. Please issue a fresh independent review for this exact head only.

[probepark]

Re-review at exact head 10a79d9b — approved, with one thing I want changed that is not a blocker.

my prior blocker is fixed

lsp/index.ts:750 and :927 now await file.write(content) before returning success, so a rejection can no longer escape the surrounding try/catch while the success flag is already set. Pinned by delayed-rejection coverage at lsp-batching.test.ts:79-100.

The other prior findings are also closed: Windows replacement and rollback use absolute position 0 (atomic-file-write.ts:349-357, :413-424) with byte-exact failure coverage at file-tools-atomicity.test.ts:484-538, and the crash-durability wording is now honest.

the durability disclaimer is the right call

atomic-file-write.ts:3-7 and docs/tools/write.md:140 state plainly that the parent directory is not fsynced, so the rename is not crash-durable. That is accurate — a rename is not guaranteed visible after power loss without an fsync of the parent — and saying so is better than quietly implying durability the code does not provide. I raised this wording last round and it was fixed properly rather than papered over.

The normal path is genuinely same-filesystem: the temp is a sibling (:127-130) renamed in place (:547-552), with no EXDEV copy fallback that could break atomicity. Only ENOENT is read as absence (:154-159, :172-178, :297-303); other errors propagate.

the post-validation window is still open — and now pinned as correct

assertPublishTargetStillIntended (:547) is a real security check: it re-resolves the publish path and rejects a retarget, then compares the parent's realpath, dev and ino. renameIntoPlace runs immediately after. A swap landing between them publishes through a path the check already rejected.

I am not blocking on it. This PR introduces that validation — the prior state had none — POSIX offers no portable conditional-rename primitive, and :9-16 plus docs/tools/write.md:142 disclose the limitation rather than hiding it. Blocking a change for not fully closing a race it substantially narrowed would be the wrong incentive.

What I do want changed is the test. file-tools-atomicity.test.ts:435-459 injects a concurrent successor inside the rename spy and then asserts the successor is overwritten — "never a mix of the successor and our bytes". That converts a known limitation into pinned behavior. If someone later implements conditional replacement, this test fails and reads as a regression, so the pin actively discourages the fix.

Rewrite it to assert what is genuinely guaranteed — that the destination is never a byte mix — and leave the winner unasserted, or mark it explicitly as documenting current non-atomic behavior with a pointer to the limitation. That keeps the torn-write guarantee pinned without freezing the part that should still improve.

coverage note

The Windows sharing-violation fallback (:359-454) is rollback-capable but not visibility-atomic, and its tests simulate platform === "win32" on POSIX while skipping on real win32 — so that path has no executable Windows coverage. Same gap I have flagged on other Windows-only work; worth routing into the Windows lane that now exists.

_{Reviewed by @probepark — method: verified the awaited writes at both prior sites, read what assertPublishTargetStillIntended actually guards before judging the residual window's severity, and checked what the race test asserts rather than that it exists.}

gajae.pr-review-verdict.v1 merge-approved sha256:b3b1d1adde218c09aa46c5f56d799e46644364b366e548a10a1eb352d37a7ad6 reviewer:human reviewer-id:probepark evidence:exact-head-10a79d9b-unawaited-write-fixed-and-durability-wording-honest-post-validation-window-documented-not-blocking

Superseded by provider-safety fix and rebase onto current dev at a69ad76; fresh exact-head approval required.

[Yeachan-Heo]

Provider safety fix is now included from current dev #4778. Latest exact head: a69ad7681356f949af39aa0a685e0de782093980, based on dev a34cbd700b56b9c663daef700e5edd14cbd20470.

Latest dev provider-safety e2e: 5/5 passed. PR focused harness: 170/170 passed. Prior approval was dismissed as stale; please issue fresh approval for this exact head after CI.

[Yeachan-Heo]

Updated exact head e5ff519214fbdd3006c2661e7210ab34dee10e61 with the minimal #4669 harness fix ecb8c7aa (stable index-derived shard expectation; no hardcoded shard).

Verified: provider safety e2e 5/5, file-tool suites 72/72, CI selftest suite 156/156, focused harness 170/170, coding-agent check passed. Fresh exact-head approval is required because the previous approval is stale.

[probepark]

Re-review at exact head e5ff5192 — approved, reinstating the approval that was dismissed when the head moved.

The 469-line delta since 10a79d9b is entirely dev rebase uptake of #4778, which I reviewed and merged separately: provider_safety_stop preservation on error turns, typed stops staying terminal despite transport facts, the hint classifier requiring an error stop, plus coverage and CI routing.

Every file this PR is actually about is byte-identical to my approved head — atomic-file-write.ts, lsp/index.ts, file-tools-atomicity.test.ts, and docs/tools/write.md all unchanged. So nothing in the delta touches the properties I approved on, and I re-checked them rather than assuming:

• my prior blocker stays fixed — lsp/index.ts:750 and :928 await the BunFile writes before returning success, pinned by delayed-rejection coverage at lsp-batching.test.ts:79-100
• Windows replacement and rollback still use positioned writes from absolute offset 0, with byte-exact rollback coverage
• the crash-durability disclaimer is intact in both source and docs — the parent directory is not fsynced and the code says so
• sibling staging plus same-directory rename remains the normal path, with no EXDEV copy fallback, and non-ENOENT errors still propagate

the one request I made is partly taken

file-tools-atomicity.test.ts:441-459 is now clearly titled and commented as documenting current last-writer-wins behavior, which satisfies the weaker of the two alternatives I offered. Good — the intent is no longer ambiguous to a future reader.

It still mechanically requires the destination to equal "ours\n", so a conditional-replacement implementation would fail it. That is the part I would still change: assert only the guaranteed property, that the destination is never a byte mix of the two writers, and leave the winner unasserted. A test that documents a limitation in prose but pins it in code will still read as a regression when someone fixes the limitation.

Non-blocking, same as last round. The check-then-rename window itself also remains open and remains non-blocking for the same reasons: this PR introduces the validation that narrows it, POSIX offers no portable conditional-rename primitive, and the limitation is disclosed rather than hidden.

_{Reviewed by @probepark — method: blob-compared every file this PR is about against my previously approved head to establish the delta was rebase-only, then re-verified each approved property at this head rather than carrying the approval forward on the file comparison alone.}

gajae.pr-review-verdict.v1 merge-approved sha256:4bdc4d3386bee5cf942476e8c5d050a92d148e4c96bb15fe6375473eb60873d9 reviewer:human reviewer-id:probepark evidence:exact-head-e5ff5192-atomic-write-and-lsp-files-byte-identical-to-approved-10a79d9b-delta-is-rebase-uptake-of-merged-4778