From a4172853af9279d7d0a295514f64f3d271d4ab93 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Wed, 19 Aug 2026 16:12:53 +0000 Subject: [PATCH 01/13] fix(tools): make file writes atomic and keep ACP/compaction continuity Write used truncate-in-place Bun.write, so an EPERM/IO failure left a 0-byte target while the shell could still write. Read required a disk stat before the ACP buffer and treated OS EPERM as a client denial, so a just-written file could look missing. Compaction-state now lists recent file mutations so a long session does not silently drop in-flight write context. Lore-id: 4734file Constraint: never use shell fallback as the product fix Constraint: do not conflate with Windows directory-fsync EPERM #4457 or workflow validation #4560 Rejected: fsync-before-rename on user files | introduces Windows directory EPERM and is not needed for leftover prevention Rejected: disk fallback on ACP permission_denied | would bypass a client authority refusal Confidence: high Scope-risk: medium Reversibility: easy Tested: bun test packages/coding-agent/test/file-tools-atomicity.test.ts packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts packages/coding-agent/test/agent-session-state-aware-compaction.test.ts packages/coding-agent/test/tools.test.ts --test-name-pattern 'write tool|read tool'; bun --cwd=packages/coding-agent run check Not-tested: live ACP editor client against 0.14.1 reporter session --- docs/tools/read.md | 4 +- docs/tools/write.md | 7 +- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/lsp/index.ts | 15 +- .../coding-agent/src/session/agent-session.ts | 46 +++++ .../src/tools/atomic-file-write.ts | 91 +++++++++ packages/coding-agent/src/tools/read.ts | 61 ++++++ packages/coding-agent/src/tools/write.ts | 12 +- ...ent-session-state-aware-compaction.test.ts | 27 ++- .../test/file-tools-atomicity.test.ts | 186 ++++++++++++++++++ 10 files changed, 434 insertions(+), 16 deletions(-) create mode 100644 packages/coding-agent/src/tools/atomic-file-write.ts create mode 100644 packages/coding-agent/test/file-tools-atomicity.test.ts diff --git a/docs/tools/read.md b/docs/tools/read.md index 5d0da01f22..1c56cafcb0 100644 --- a/docs/tools/read.md +++ b/docs/tools/read.md @@ -81,7 +81,9 @@ URL selectors are parsed separately in `packages/coding-agent/src/tools/fetch.ts - `#readSqlite()` dispatches on `parseSqliteSelector()`. 6. Otherwise it treats the input as a local filesystem path. - `resolveReadPath()` expands `~`, resolves relative to session cwd, treats bare `/` as session cwd, and retries macOS screenshot/NFD/curly-quote variants. - - If the path does not exist, `findUniqueSuffixMatch()` does a workspace glob-based unique suffix lookup (skipped for remote mounts). + - If the path does not exist on disk and an ACP `readTextFile` bridge is present, the editor buffer is tried before suffix lookup so a just-written client buffer is not reported as missing. + - OS errno failures from the bridge (`EPERM`, `EACCES`, `ENOENT`, …) fall back to disk; structured ACP denials (`permission_denied`, `-32001`) do not. + - If the path still does not exist, `findUniqueSuffixMatch()` does a workspace glob-based unique suffix lookup (skipped for remote mounts). 7. Directories go through `#readDirectory()`. 8. Non-directories branch by content type: - image metadata / inline image diff --git a/docs/tools/write.md b/docs/tools/write.md index 18b5aa97b0..60d2f186e2 100644 --- a/docs/tools/write.md +++ b/docs/tools/write.md @@ -11,6 +11,7 @@ - `packages/coding-agent/src/lsp/index.ts` — format-on-write and diagnostics writethrough. - `packages/coding-agent/src/tools/auto-generated-guard.ts` — block overwriting generated files. - `packages/coding-agent/src/tools/fs-cache-invalidation.ts` — invalidate shared FS scan caches after writes. + - `packages/coding-agent/src/tools/atomic-file-write.ts` — sibling temp + rename so a failed write never leaves a 0-byte destination. - `packages/coding-agent/src/tools/plan-mode-guard.ts` — resolve paths and enforce plan-mode write policy. ## Inputs @@ -66,15 +67,15 @@ Single-shot result. 6. Otherwise the tool treats `path` as a plain filesystem file. - `enforcePlanModeWrite(..., { op: "create" })` runs before path resolution. - Existing files are checked by `assertEditableFile()` to block overwriting detected generated files. - - The session’s writethrough callback writes content. With LSP enabled and `lsp.formatOnWrite` / `lsp.diagnosticsOnWrite` settings on, `createLspWritethrough()` may format content, sync it through LSP servers, save it, and collect diagnostics. Otherwise `writethroughNoop()` writes directly with `Bun.write()` or `file.write()`. - - `invalidateFsScanAfterWrite()` runs on the file path. + - The session’s writethrough callback writes content. With LSP enabled and `lsp.formatOnWrite` / `lsp.diagnosticsOnWrite` settings on, `createLspWritethrough()` may format content, sync it through LSP servers, save it, and collect diagnostics. Otherwise `writethroughNoop()` writes through `writeFileAtomically()` (sibling temp, then rename). Permission errors (`EACCES`/`EPERM`/`EROFS`) become a `ToolError` that says the original file was left unchanged. + - `invalidateFsScanAfterWrite()` and `fileReadCache.invalidate()` run on the file path. 7. The tool returns a text result and optional diagnostics metadata. ## Modes / Variants ### Plain file path - Target is any path that does not resolve as an archive selector and does not resolve as an existing-or-new SQLite selector. - Existing files are overwritten. -- `write.ts` does not call `fs.mkdir()` on this path; parent-directory creation is only implemented in the archive branch. +- Parent directories are created by `writeFileAtomically()`. A failed write never truncates an existing destination to 0 bytes. Example: diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 71965e8251..f792226e4c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,7 @@ - Coordinator event journal rows can now be pushed to one opt-in webhook (#4706). External orchestrators that cannot stay attached to `gjc_coordinator_watch_events` long-poll (a 300s `await_turn` timeout is not session death) had no push of **existing** journal rows; they can now set `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_URL` to receive each row as an authenticated POST whose body is the exact native `watch_events` record — same `seq`, same stable `id`, at-least-once so sinks dedupe on `id`. The feature is env-only and default-off (no MCP tool can set or read it), destinations are allowlisted (`https:` anywhere, `http:` loopback only, no redirects), the bearer token comes from a secret file path rather than env, an optional session-id scope restricts delivery to authorized sessions, and delivery runs through a durable per-row outbox off the journal append path with bounded attempts, exponential backoff, and a bounded request timeout — a dead sink never delays or rewrites terminal turn/session persistence. The five `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_*` variables resolve through the trusted credential environment (`$credentialEnv`, the same provenance as the crash-relay DSN) rather than raw `process.env`, so a checkout's `.env` cannot select the egress destination or the token file. `watch_events` long-poll is unchanged and remains the source of truth; `gjc coordinator doctor` reports the resolved webhook state. - Extension activation is now transactional. `pi.registerFlag(..., { default })` and `pi.registerProvider(...)` used to mutate the shared `ExtensionRuntime` state directly with no rollback, so a factory that threw midway was discarded while its side effects leaked: the flag default stayed readable via `getFlag`/`getFlagValues` and the provider registration stayed queued for the ModelRegistry drain in `sdk/session.ts` and `runListModelsCommand`, activating providers from an extension that never activated. Each factory invocation now stages its shared-state writes in an `ExtensionActivationScope` (stage → factory completes without throwing → commit into the shared runtime); rollback discards the staged writes so a failed extension leaves no flag default and no provider registration behind, and earlier extensions' committed state is untouched. After commit the shared runtime is authoritative for `getFlag`, so runtime-side writes (CLI flag overrides, a later extension's committed default) stay observable to retained extension API objects exactly as before the transaction (#4718). Commit itself is transactional: prior flag entries and the provider-queue length are journaled before publication, so a throw partway through commit is undone before it escapes and the scope only becomes terminal once publication fully succeeds — a failed extension leaves nothing behind even when the failure happens during publication. - A content-free Anthropic capacity overload no longer ends the turn under the default retry configuration. Anthropic can answer with its typed `overloaded_error` as a statusless stream error, and session retry already classifies that as transient, but the bare-default admission list only covered watchdog timeouts and the Codex `server_is_overloaded` event — so the turn surfaced the raw provider envelope and went idle, leaving the operator to resend or switch models by hand for a failure the provider says to retry. The admission now also accepts Anthropic's own overload code, recognized by parsing the error envelope and requiring both the outer `type` and the nested `error.type` to match exactly. Nothing else changes: the attempt must still carry no assistant text, thinking, or tool call and no conflicting transport facts (a status-bearing or otherwise typed failure keeps failing closed), overload prose alone can never authorize a replay, and the existing capped exponential backoff and `retry.enabled: false` opt-out are untouched. +- File tools no longer lose a just-written path or leave a 0-byte target when a write fails (#4734). `writethroughNoop` and LSP writethrough now publish through a sibling temp + rename (`writeFileAtomically`) so a permission/IO error cannot truncate the destination; `EACCES`/`EPERM`/`EROFS` surface as an actionable `ToolError` that says the original file is unchanged. Read tries the ACP `readTextFile` bridge when disk stat misses, and treats OS errno codes such as `EPERM` as availability failures rather than client-authority denials so a file that exists on disk is still readable. Successful writes invalidate `fileReadCache`. Compaction-state now lists recent successful `write`/`edit`/`apply_patch`/`ast_edit` paths so a long-session compact does not silently drop in-flight file-tool context. This is independent of Windows directory-fsync `EPERM` (#4457) and of workflow-validation compaction (#4560). - Runtime skill discovery now scans `skills.customDirectories`. Session startup already loaded those directories through `loadSkills`, but `discoverRuntimeSkills` and `findRuntimeSkillByName` searched only the canonical project and user roots, so a configured custom skill was invocable by exact name yet absent from every `skill_discovery` search -- usable only by someone who already knew it existed. Both discovery entry points now scan the configured directories at user level (so project-scoped queries exclude them), deduplicated and tilde-expanded the same way `loadSkills` does. Naming a directory is explicit consent, so custom directories are not gated on `skills.trustUserSkills` -- matching the startup rule -- while the `skills.enabled` master switch still suppresses them. - A broker that cannot retain its own publication now names the object that withheld authority. The native layer opens `sdk`, `sdk/broker.lock`, `sdk/broker.lock/owner.json`, and `sdk/broker.json` no-follow and reports every refusal as one opaque `Retained broker publication authority is unavailable.`, so `gjc sdk` died with nothing to act on and the precondition could only be learned from the native source — a shared multi-account layout that symlinks the agent directory's `sdk` entry crashed every broker start this way. The failure is still fatal and still rolls back its publication; it now appends the first obstruction (missing entry, symlinked entry, wrong file kind, unreadable entry, or a non-fixed-width `heartbeatAt`) ahead of a bounded agent directory, so the named object survives the 512-character startup-failure reason, and stays verbatim when every precondition holds so a named condition is never invented. Each object is probed with the native's own access mode — the lock record read-only, only the published record read/write — and a file kind is only ever named through the open the native itself refuses, so a layout the native accepts is never reported as an obstruction; the published record is read through the descriptor the no-follow open already verified, never reopened by name. When rollback fails too, the aggregate message now carries the acquisition diagnostic, since the durable startup-failure marker persists only that message. - Retained broker publication probing now opens POSIX objects non-blocking, diagnoses exact-buffer malformed records, escapes control and bidi characters in persisted agent-directory diagnostics, and covers native-rejected wrong-kind objects without inventing a condition the native layer accepts. diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 9c4b45c590..a122841619 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -6,6 +6,7 @@ import type { BunFile } from "bun"; import { type Theme, theme } from "../modes/theme/theme"; import lspDescription from "../prompts/tools/lsp.md" with { type: "text" }; import type { ToolSession } from "../tools"; +import { writeFileAtomically } from "../tools/atomic-file-write"; import { formatPathRelativeToCwd, resolveToCwd } from "../tools/path-utils"; import { ToolAbortError, ToolError, throwIfAborted } from "../tools/tool-errors"; import { clampTimeout } from "../tools/tool-timeouts"; @@ -741,15 +742,11 @@ export async function writethroughNoop( dst: string, content: string, _signal?: AbortSignal, - file?: BunFile, + _file?: BunFile, _batch?: LspWritethroughBatchRequest, _getDeferred?: (dst: string) => WritethroughDeferredHandle | undefined, ): Promise { - if (file) { - await file.write(content); - } else { - await Bun.write(dst, content); - } + await writeFileAtomically(dst, content); return undefined; } @@ -906,7 +903,7 @@ async function runLspWritethrough( cwd: string, options: ResolvedWritethroughOptions, signal?: AbortSignal, - file?: BunFile, + _file?: BunFile, deferred?: { onDeferredDiagnostics: (diagnostics: FileDiagnosticsResult) => void; signal: AbortSignal; @@ -916,12 +913,12 @@ async function runLspWritethrough( const config = getConfig(cwd); const servers = getServersForFile(config, dst); if (servers.length === 0) { - return writethroughNoop(dst, content, signal, file); + return writethroughNoop(dst, content, signal, _file); } const { lspServers, customLinterServers } = splitServers(servers); let finalContent = content; - const writeContent = async (value: string) => (file ? file.write(value) : Bun.write(dst, value)); + const writeContent = async (value: string) => writeFileAtomically(dst, value); const getWritePromise = once(() => writeContent(finalContent)); const useCustomFormatter = enableFormat && customLinterServers.length > 0; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index c3722d5a09..94d299971f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -490,6 +490,40 @@ interface CompactionStateSnapshot { activeSkills: Array<{ skill: string; phase: string }>; queuedMessages: boolean; lastAssistantStopReason: StopReason | undefined; + recentFileMutations: string[]; +} + +const FILE_MUTATION_TOOLS = new Set(["write", "edit", "apply_patch", "ast_edit"]); +const MAX_RECENT_FILE_MUTATIONS = 12; + +function collectRecentFileMutations(messages: readonly AgentMessage[]): string[] { + const callsById = new Map(); + for (const message of messages) { + if (message.role !== "assistant") continue; + const content = (message as AssistantMessage).content; + if (!Array.isArray(content)) continue; + for (const block of content) { + if (block.type !== "toolCall" || !FILE_MUTATION_TOOLS.has(block.name)) continue; + const args = block.arguments; + if (typeof args !== "object" || args === null) continue; + const filePath = (args as { path?: unknown }).path; + if (typeof filePath === "string" && filePath.length > 0) callsById.set(block.id, filePath); + } + } + const seen = new Set(); + const paths: string[] = []; + for (let index = messages.length - 1; index >= 0; index--) { + const message = messages[index]; + if (message.role !== "toolResult") continue; + const result = message as { toolName?: string; toolCallId?: string; isError?: boolean }; + if (result.isError || !result.toolName || !FILE_MUTATION_TOOLS.has(result.toolName)) continue; + const filePath = result.toolCallId ? callsById.get(result.toolCallId) : undefined; + if (!filePath || seen.has(filePath)) continue; + seen.add(filePath); + paths.push(filePath); + if (paths.length >= MAX_RECENT_FILE_MUTATIONS) break; + } + return paths; } /** Escape XML-ish metacharacters and flatten newlines so state text cannot break compaction prompt framing. */ @@ -11867,6 +11901,7 @@ export class AgentSession { activeSkills: [], queuedMessages: false, lastAssistantStopReason: undefined, + recentFileMutations: [], }; try { const goalState = this.getGoalModeState(); @@ -11921,6 +11956,13 @@ export class AgentSession { error: error instanceof Error ? error.message : String(error), }); } + try { + snapshot.recentFileMutations = collectRecentFileMutations(this.messages); + } catch (error) { + logger.warn("Failed to read recent file mutations for compaction snapshot", { + error: error instanceof Error ? error.message : String(error), + }); + } return snapshot; } @@ -11943,6 +11985,10 @@ export class AgentSession { const todos = snapshot.openTodos.map(todo => sanitizeCompactionStateText(todo, 120)); context.push(`Open todos: ${todos.join("; ")}`); } + if (snapshot.recentFileMutations.length > 0) { + const files = snapshot.recentFileMutations.map(filePath => sanitizeCompactionStateText(filePath, 120)); + context.push(`Recent file mutations: ${files.join("; ")}`); + } return context; } diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts new file mode 100644 index 0000000000..dd136ebb89 --- /dev/null +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -0,0 +1,91 @@ +/** + * Crash-atomic user-file writes for the write/edit/LSP writethrough path. + * + * `Bun.write` truncates the destination then copies bytes. A permission or IO + * failure after that truncate leaves a 0-byte target even though the tool + * reported an error. Stage to a sibling temp, then rename over the destination + * so a failed attempt never publishes a truncated file. Directory fsync is + * intentionally omitted: Windows reports `EPERM` for it (#4457) and user-file + * publication does not need that durability barrier. + */ +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/utils"; + +const WINDOWS_RENAME_BACKOFF_MS = [10, 25, 50, 100, 200] as const; +const WINDOWS_SHARING_VIOLATION_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); + +export function isFileWritePermissionError(error: unknown): boolean { + return isEacces(error) || hasFsCode(error, "EPERM") || hasFsCode(error, "EROFS"); +} + +export function formatFileWriteError(error: unknown, dest: string): string { + if (isEisdir(error)) { + return `Cannot write '${dest}': path is a directory.`; + } + if (isFileWritePermissionError(error)) { + const code = isFsError(error) ? error.code : "EPERM"; + return `Permission denied writing '${dest}' (${code}). The original file was left unchanged. Check directory write bits, file immutability, and any sandbox policy. Do not retry the same path through the shell tool.`; + } + return error instanceof Error ? error.message : String(error); +} + +function tempPathFor(dest: string): string { + const unique = `${process.pid}.${Date.now().toString(36)}.${Math.random().toString(36).slice(2, 8)}`; + return path.join(path.dirname(dest), `.${path.basename(dest)}.${unique}.tmp`); +} + +async function renameIntoPlace(from: string, to: string): Promise { + try { + await fs.rename(from, to); + return; + } catch (error) { + if (process.platform !== "win32" || !isFsError(error) || !WINDOWS_SHARING_VIOLATION_CODES.has(error.code)) { + throw error; + } + } + let lastError: unknown; + for (const delay of WINDOWS_RENAME_BACKOFF_MS) { + await Bun.sleep(delay); + try { + await fs.rename(from, to); + return; + } catch (error) { + lastError = error; + if (!isFsError(error) || !WINDOWS_SHARING_VIOLATION_CODES.has(error.code)) throw error; + } + } + throw lastError; +} + +export async function writeFileAtomically(dest: string, content: string): Promise { + const dir = path.dirname(dest); + await fs.mkdir(dir, { recursive: true }); + + let existingMode: number | undefined; + try { + const stat = await fs.stat(dest); + if (stat.isDirectory()) { + const error = new Error(`EISDIR: illegal operation on a directory, write '${dest}'`) as Error & { + code: string; + }; + error.code = "EISDIR"; + throw error; + } + existingMode = stat.mode; + } catch (error) { + if (!isEnoent(error)) throw error; + } + + const tmp = tempPathFor(dest); + try { + await Bun.write(tmp, content); + if (existingMode !== undefined) { + await fs.chmod(tmp, existingMode); + } + await renameIntoPlace(tmp, dest); + } catch (error) { + await fs.unlink(tmp).catch(() => {}); + throw error; + } +} diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 9bb2cd131e..5a6a52c2b8 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -1436,6 +1436,21 @@ interface ResolvedSqliteReadPath { function isClientAuthorityDenial(error: unknown): boolean { const code = typeof error === "object" && error !== null && "code" in error ? (error as { code?: unknown }).code : undefined; + // OS errno codes are transport/availability failures, not an ACP permission + // decision. Treating `EPERM`/`EACCES` as denials skipped the disk fallback + // for files that already existed on disk. + if ( + code === "EPERM" || + code === "EACCES" || + code === "ENOENT" || + code === "EIO" || + code === "EBUSY" || + code === "EROFS" || + code === "EISDIR" || + code === "ENOTDIR" + ) { + return false; + } // ACP clients surface refusals as an application error; -32001 is the reserved // client-authority denial code and -32603 covers hosts without a dedicated code. if (code === "permission_denied" || code === "forbidden" || code === -32001 || code === -32603) return true; @@ -2395,6 +2410,44 @@ export class ReadTool implements AgentTool { if (!bridge?.capabilities.readTextFile || !bridge.readTextFile) return undefined; return bridge.readTextFile({ path: absolutePath, ...options }); } + async #readMissingPathThroughBridge( + absolutePath: string, + parsed: ParsedSelector, + _localReadPath: string, + truncation: TruncationDirection | undefined, + signal: AbortSignal | undefined, + ): Promise | undefined> { + const bridgePromise = this.#routeReadThroughBridge(absolutePath); + if (bridgePromise === undefined) return undefined; + throwIfAborted(signal); + try { + const bridgeText = await bridgePromise; + const direction = resolveEffectiveDirection(truncation, "local-bare-stream", this.session.settings); + if (isMultiRange(parsed) && parsed.kind === "lines") { + return this.#buildInMemoryMultiRangeResult(bridgeText, parsed.ranges, { + details: { resolvedPath: absolutePath }, + sourcePath: absolutePath, + entityLabel: "file", + raw: isRawSelector(parsed), + truncationDirection: direction, + cacheLinesFor: absolutePath, + }); + } + const { offset, limit } = selToOffsetLimit(parsed); + return this.#buildInMemoryTextResult(bridgeText, offset, limit, { + details: { resolvedPath: absolutePath }, + sourcePath: absolutePath, + entityLabel: "file", + raw: isRawSelector(parsed), + cacheLinesFor: absolutePath, + truncationDirection: direction, + }); + } catch (error) { + if (isClientAuthorityDenial(error)) throw error; + logger.warn("ACP fs readTextFile failed for a path missing on disk", { path: absolutePath, error }); + return undefined; + } + } async #trySummarize(absolutePath: string, fileSize: number, signal?: AbortSignal): Promise { if (fileSize > MAX_SUMMARY_BYTES) return null; @@ -2663,6 +2716,14 @@ export class ReadTool implements AgentTool { isDirectory = stat.isDirectory(); } catch (error) { if (isNotFoundError(error)) { + const bridged = await this.#readMissingPathThroughBridge( + absolutePath, + parsed, + localReadPath, + params.truncation, + signal, + ); + if (bridged) return bridged; // Attempt unique suffix resolution before falling back to fuzzy suggestions if (!isRemoteMountPath(absolutePath)) { const suffixMatch = await findUniqueSuffixMatch(localReadPath, this.session.cwd, signal); diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index 10e85a1e64..b55ef21ae9 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -17,6 +17,7 @@ import type { ToolSession } from "../sdk"; import { Ellipsis, Hasher, type RenderCache, renderStatusLine, truncateToWidth } from "../tui"; import { resolveFileDisplayMode } from "../utils/file-display-mode"; import { parseArchivePathCandidates } from "./archive-reader"; +import { formatFileWriteError } from "./atomic-file-write"; import { assertEditableFile } from "./auto-generated-guard"; import { type ConflictEntry, @@ -748,9 +749,10 @@ export class WriteTool implements AgentTool { expect(skillContext).not.toContain(""); expect(skillContext).not.toContain("\n"); }); + it("includes recent write paths in compaction-state extraContext", async () => { + const writeCall = assistantMessage("stop"); + writeCall.content = [ + { + type: "toolCall", + id: "write-recent", + name: "write", + arguments: { path: "frontend/e2e/zzop-repro.spec.ts", content: "test" }, + }, + ]; + session.agent.appendMessage(writeCall); + session.agent.appendMessage({ + role: "toolResult", + toolCallId: "write-recent", + toolName: "write", + content: [{ type: "text", text: "Successfully wrote 4 bytes to frontend/e2e/zzop-repro.spec.ts" }], + isError: false, + timestamp: Date.now(), + } as ToolResultMessage); + seedCompactionHistory(); + await session.compact(); + const options = compactSpy.mock.calls[0]?.[5]; + const files = options?.extraContext?.find(context => context.startsWith("Recent file mutations:")) ?? ""; + expect(files).toContain("frontend/e2e/zzop-repro.spec.ts"); + }); it("continues synthetic auto-continue for an active nonterminal workflow", async () => { await seedActiveSkillState("active"); diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts new file mode 100644 index 0000000000..d2e83a9e94 --- /dev/null +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -0,0 +1,186 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Settings } from "@gajae-code/coding-agent/config/settings"; +import type { ClientBridge } from "@gajae-code/coding-agent/session/client-bridge"; +import type { ToolSession } from "@gajae-code/coding-agent/tools"; +import { ReadTool } from "@gajae-code/coding-agent/tools/read"; +import { WriteTool } from "@gajae-code/coding-agent/tools/write"; +import { FileReadCache } from "../src/edit/file-read-cache"; +import { writeFileAtomically } from "../src/tools/atomic-file-write"; + +function createSession(cwd: string, extras: Partial = {}): ToolSession { + return { + cwd, + hasUI: false, + getSessionFile: () => path.join(cwd, "session.jsonl"), + getSessionSpawns: () => "*", + getArtifactsDir: () => path.join(cwd, "artifacts"), + allocateOutputArtifact: async () => ({ id: "artifact-1", path: path.join(cwd, "artifact-1.log") }), + settings: Settings.isolated(), + ...extras, + }; +} + +function textOf(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content + .filter(block => block.type === "text") + .map(block => block.text ?? "") + .join("\n"); +} + +describe("file tool atomicity and read-after-write (#4734)", () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "file-tools-4734-")); + }); + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); + }); + + it("reads a freshly written nested file without a not-found error", async () => { + const session = createSession(tmpDir); + const dest = path.join(tmpDir, "frontend", "e2e", "zzop-repro.spec.ts"); + const content = "import { test } from '@playwright/test';\n"; + const writeResult = await new WriteTool(session).execute("write-fresh", { path: dest, content }); + expect(textOf(writeResult)).toContain("Successfully wrote"); + const readResult = await new ReadTool(session).execute("read-fresh", { path: dest }); + expect(textOf(readResult)).toContain("import { test }"); + const stat = await fs.stat(dest); + expect(stat.size).toBeGreaterThan(0); + }); + + it("leaves an existing file unchanged when the staged write fails", async () => { + const dest = path.join(tmpDir, "backend", "app", "routers", "automation_snapshots.py"); + await fs.mkdir(path.dirname(dest), { recursive: true }); + await fs.writeFile(dest, "original = True\n"); + const original = spyOn(Bun, "write").mockImplementation(async target => { + if (String(target).includes(".tmp")) { + const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; + error.code = "EPERM"; + throw error; + } + throw new Error(`unexpected Bun.write to ${String(target)}`); + }); + try { + await expect(writeFileAtomically(dest, "mutated = True\n")).rejects.toMatchObject({ code: "EPERM" }); + expect(await fs.readFile(dest, "utf8")).toBe("original = True\n"); + const leftovers = await fs.readdir(path.dirname(dest)); + expect(leftovers.some(name => name.includes(".tmp"))).toBe(false); + } finally { + original.mockRestore(); + } + }); + + it("does not create a 0-byte destination when a new-file staged write fails", async () => { + const dest = path.join(tmpDir, "new-file.py"); + const original = spyOn(Bun, "write").mockImplementation(async target => { + if (String(target).includes(".tmp")) { + const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; + error.code = "EPERM"; + throw error; + } + throw new Error(`unexpected Bun.write to ${String(target)}`); + }); + try { + await expect(writeFileAtomically(dest, "print('hi')\n")).rejects.toMatchObject({ code: "EPERM" }); + expect( + await fs.stat(dest).then( + () => true, + () => false, + ), + ).toBe(false); + } finally { + original.mockRestore(); + } + }); + + it("surfaces a permission error without leaving a 0-byte file in a read-only directory", async () => { + if (typeof process.getuid === "function" && process.getuid() === 0) return; + const locked = path.join(tmpDir, "locked"); + await fs.mkdir(locked); + await fs.chmod(locked, 0o555); + const dest = path.join(locked, "automation_snapshots.py"); + try { + await expect( + new WriteTool(createSession(tmpDir)).execute("write-eperm", { + path: dest, + content: "print('nope')\n", + }), + ).rejects.toThrow(/Permission denied writing/); + expect( + await fs.stat(dest).then( + () => true, + () => false, + ), + ).toBe(false); + } finally { + await fs.chmod(locked, 0o755); + } + }); + + it("reads an ACP buffer that has not been flushed to disk", async () => { + const dest = path.join(tmpDir, "frontend", "e2e", "zzop-repro.spec.ts"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true, writeTextFile: true }, + writeTextFile: async () => undefined, + readTextFile: async () => "export const fromBridge = true;\n", + }; + const session = createSession(tmpDir, { getClientBridge: () => bridge }); + await new WriteTool(session).execute("acp-write", { path: dest, content: "export const fromBridge = true;\n" }); + expect( + await fs.stat(dest).then( + () => true, + () => false, + ), + ).toBe(false); + const readResult = await new ReadTool(session).execute("acp-read", { path: dest }); + expect(textOf(readResult)).toContain("fromBridge"); + }); + + it("falls back to disk when ACP read fails with an OS EPERM errno", async () => { + const dest = path.join(tmpDir, "on-disk.ts"); + await fs.writeFile(dest, "export const fromDisk = true;\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; + error.code = "EPERM"; + throw error; + }, + }; + const result = await new ReadTool(createSession(tmpDir, { getClientBridge: () => bridge })).execute( + "eperm-fallback", + { path: dest }, + ); + expect(textOf(result)).toContain("fromDisk"); + }); + + it("does not fall back to disk for a structured ACP permission denial", async () => { + const dest = path.join(tmpDir, "secret.ts"); + await fs.writeFile(dest, "export const leaked = true;\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + const error = new Error("permission denied by client") as Error & { code: string }; + error.code = "permission_denied"; + throw error; + }, + }; + await expect( + new ReadTool(createSession(tmpDir, { getClientBridge: () => bridge })).execute("denied", { path: dest }), + ).rejects.toThrow(/permission denied by client/); + }); + + it("invalidates the file-read cache after a successful write", async () => { + const dest = path.join(tmpDir, "cached.ts"); + const cache = new FileReadCache(); + cache.recordContiguous(dest, 1, ["old line"]); + const session = createSession(tmpDir, { fileReadCache: cache }); + await new WriteTool(session).execute("cache-write", { path: dest, content: "new line\n" }); + expect(cache.get(dest)).toBeNull(); + }); +}); From 7b8a3009845a5a2b0dd0565735292e51169235cb Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Wed, 19 Aug 2026 16:29:16 +0000 Subject: [PATCH 02/13] fix(tools): honor ACP denials and follow symlink write targets Architect review of #4735: structural summarize swallowed permission_denied and read disk; atomic rename replaced a destination symlink instead of its referent; staging used Bun.write on a guessed temp name. Summarize now rethrows client-authority denials, writes follow the link and exclusively create the sibling temp, and tests cover those paths plus compaction non-continuation. Lore-id: 4734file Constraint: no directory fsync on user-file writes Rejected: keep Bun.write staging | truncates a colliding leftover temp Confidence: high Scope-risk: medium Reversibility: easy Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/file-tools-atomicity.test.ts packages/coding-agent/test/agent-session-state-aware-compaction.test.ts packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts Not-tested: custom LSP formatter failure after first on-disk publication --- docs/tools/write.md | 2 +- .../src/tools/atomic-file-write.ts | 97 +++++++++++++++---- packages/coding-agent/src/tools/read.ts | 29 ++++-- ...ent-session-state-aware-compaction.test.ts | 28 ++++++ .../test/file-tools-atomicity.test.ts | 67 +++++++++++-- 5 files changed, 189 insertions(+), 34 deletions(-) diff --git a/docs/tools/write.md b/docs/tools/write.md index 60d2f186e2..744009521f 100644 --- a/docs/tools/write.md +++ b/docs/tools/write.md @@ -135,7 +135,7 @@ content: "" - Filesystem - Creates or overwrites plain files. - Rewrites entire archive files when writing an archive entry. - - Creates parent directories for archive files only. + - Creates parent directories for plain files and archive files. - Mutates existing SQLite databases; never creates a new SQLite DB. - Subprocesses / native bindings - Uses Bun SQLite bindings via `bun:sqlite`. diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index dd136ebb89..537356f082 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -7,6 +7,10 @@ * so a failed attempt never publishes a truncated file. Directory fsync is * intentionally omitted: Windows reports `EPERM` for it (#4457) and user-file * publication does not need that durability barrier. + * + * Destination symlinks are followed: the referent is replaced, the link stays. + * Staging uses exclusive create (`wx`) so a colliding leftover temp is not + * truncated or unlinked. */ import * as fs from "node:fs/promises"; import * as path from "node:path"; @@ -14,18 +18,38 @@ import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/ const WINDOWS_RENAME_BACKOFF_MS = [10, 25, 50, 100, 200] as const; const WINDOWS_SHARING_VIOLATION_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); +const TEMP_CREATE_ATTEMPTS = 8; +const DEFAULT_FILE_MODE = 0o666; + +export class FileWriteNotPublishedError extends Error { + readonly dest: string; + override readonly cause: unknown; + constructor(dest: string, cause: unknown) { + super(formatFileWriteError(cause, dest, { destUnchanged: true })); + this.name = "FileWriteNotPublishedError"; + this.dest = dest; + this.cause = cause; + if (isFsError(cause)) { + (this as Error & { code?: string }).code = cause.code; + } + } +} export function isFileWritePermissionError(error: unknown): boolean { return isEacces(error) || hasFsCode(error, "EPERM") || hasFsCode(error, "EROFS"); } -export function formatFileWriteError(error: unknown, dest: string): string { +export function formatFileWriteError(error: unknown, dest: string, options: { destUnchanged?: boolean } = {}): string { + if (error instanceof FileWriteNotPublishedError) return error.message; if (isEisdir(error)) { return `Cannot write '${dest}': path is a directory.`; } if (isFileWritePermissionError(error)) { const code = isFsError(error) ? error.code : "EPERM"; - return `Permission denied writing '${dest}' (${code}). The original file was left unchanged. Check directory write bits, file immutability, and any sandbox policy. Do not retry the same path through the shell tool.`; + const unchanged = options.destUnchanged + ? " The original file was left unchanged." + : " The destination may already have been replaced if a formatter published earlier in this write."; + return `Permission denied writing '${dest}' (${code}).${unchanged} Check directory write bits, file immutability, and any sandbox policy. Do not retry the same path through the shell tool.`; } return error instanceof Error ? error.message : String(error); } @@ -58,34 +82,67 @@ async function renameIntoPlace(from: string, to: string): Promise { throw lastError; } -export async function writeFileAtomically(dest: string, content: string): Promise { - const dir = path.dirname(dest); - await fs.mkdir(dir, { recursive: true }); +function eisdir(dest: string): Error & { code: string } { + const error = new Error(`EISDIR: illegal operation on a directory, write '${dest}'`) as Error & { + code: string; + }; + error.code = "EISDIR"; + return error; +} - let existingMode: number | undefined; +async function resolvePublishPath(dest: string, depth = 0): Promise<{ publishPath: string; existingMode?: number }> { + if (depth > 40) { + throw new Error(`ELOOP: too many symbolic links, write '${dest}'`); + } try { - const stat = await fs.stat(dest); - if (stat.isDirectory()) { - const error = new Error(`EISDIR: illegal operation on a directory, write '${dest}'`) as Error & { - code: string; - }; - error.code = "EISDIR"; - throw error; + const lst = await fs.lstat(dest); + if (lst.isDirectory()) throw eisdir(dest); + if (lst.isSymbolicLink()) { + const target = await fs.readlink(dest); + return resolvePublishPath(path.resolve(path.dirname(dest), target), depth + 1); } - existingMode = stat.mode; + return { publishPath: dest, existingMode: lst.mode }; } catch (error) { if (!isEnoent(error)) throw error; + return { publishPath: dest }; } +} - const tmp = tempPathFor(dest); +async function writeExclusiveTemp(tmp: string, content: string, mode: number): Promise { + const handle = await fs.open(tmp, "wx", mode); try { - await Bun.write(tmp, content); - if (existingMode !== undefined) { - await fs.chmod(tmp, existingMode); - } - await renameIntoPlace(tmp, dest); + await handle.writeFile(content); } catch (error) { + await handle.close().catch(() => {}); await fs.unlink(tmp).catch(() => {}); throw error; } + await handle.close(); +} + +export async function writeFileAtomically(dest: string, content: string): Promise { + let publishPath = dest; + try { + const resolved = await resolvePublishPath(dest); + publishPath = resolved.publishPath; + await fs.mkdir(path.dirname(publishPath), { recursive: true }); + const mode = resolved.existingMode ?? DEFAULT_FILE_MODE; + let lastError: unknown; + for (let attempt = 0; attempt < TEMP_CREATE_ATTEMPTS; attempt++) { + const tmp = tempPathFor(publishPath); + try { + await writeExclusiveTemp(tmp, content, mode); + await renameIntoPlace(tmp, publishPath); + return; + } catch (error) { + lastError = error; + if (hasFsCode(error, "EEXIST")) continue; + throw error; + } + } + throw lastError; + } catch (error) { + if (error instanceof FileWriteNotPublishedError) throw error; + throw new FileWriteNotPublishedError(dest, error); + } } diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 5a6a52c2b8..c36b345882 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -2422,7 +2422,16 @@ export class ReadTool implements AgentTool { throwIfAborted(signal); try { const bridgeText = await bridgePromise; - const direction = resolveEffectiveDirection(truncation, "local-bare-stream", this.session.settings); + if (parsed.kind === "conflicts") return undefined; + const bareEligible = parsed.kind === "none"; + const route = isMultiRange(parsed) + ? "local-multi-range" + : bareEligible + ? "local-bare-stream" + : isRawSelector(parsed) + ? "local-raw" + : "local-range"; + const direction = resolveEffectiveDirection(truncation, route, this.session.settings); if (isMultiRange(parsed) && parsed.kind === "lines") { return this.#buildInMemoryMultiRangeResult(bridgeText, parsed.ranges, { details: { resolvedPath: absolutePath }, @@ -2455,10 +2464,17 @@ export class ReadTool implements AgentTool { try { throwIfAborted(signal); const bridgePromise = this.#routeReadThroughBridge(absolutePath); - const code = - bridgePromise !== undefined - ? await bridgePromise.catch(() => Bun.file(absolutePath).text()) - : await Bun.file(absolutePath).text(); + let code: string; + if (bridgePromise !== undefined) { + try { + code = await bridgePromise; + } catch (error) { + if (isClientAuthorityDenial(error)) throw error; + code = await Bun.file(absolutePath).text(); + } + } else { + code = await Bun.file(absolutePath).text(); + } throwIfAborted(signal); if (countTextLines(code) > MAX_SUMMARY_LINES) return null; @@ -2469,7 +2485,8 @@ export class ReadTool implements AgentTool { minBodyLines: this.session.settings.get("read.summarize.minBodyLines"), minCommentLines: this.session.settings.get("read.summarize.minCommentLines"), }); - } catch { + } catch (error) { + if (isClientAuthorityDenial(error)) throw error; return null; } } diff --git a/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts b/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts index daea6a7991..85284099ef 100644 --- a/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts +++ b/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts @@ -418,6 +418,34 @@ describe("AgentSession state-aware compaction", () => { const files = options?.extraContext?.find(context => context.startsWith("Recent file mutations:")) ?? ""; expect(files).toContain("frontend/e2e/zzop-repro.spec.ts"); }); + it("does not auto-continue from recent file mutations alone", async () => { + const writeCall = assistantMessage("stop"); + writeCall.content = [ + { + type: "toolCall", + id: "write-done", + name: "write", + arguments: { path: "frontend/e2e/zzop-repro.spec.ts", content: "test" }, + }, + ]; + session.agent.appendMessage(writeCall); + session.agent.appendMessage({ + role: "toolResult", + toolCallId: "write-done", + toolName: "write", + content: [{ type: "text", text: "Successfully wrote 4 bytes to frontend/e2e/zzop-repro.spec.ts" }], + isError: false, + timestamp: Date.now(), + } as ToolResultMessage); + const promptSpy = vi.spyOn(session.agent, "prompt").mockResolvedValue(); + const notices: string[] = []; + session.subscribe(event => { + if (event.type === "notice") notices.push(event.message); + }); + await compact(); + expect(promptSpy).not.toHaveBeenCalled(); + expect(notices).toContain("Auto-continue skipped: no unfinished work detected"); + }); it("continues synthetic auto-continue for an active nonterminal workflow", async () => { await seedActiveSkillState("active"); diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index d2e83a9e94..ab4bde92c1 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -57,13 +57,14 @@ describe("file tool atomicity and read-after-write (#4734)", () => { const dest = path.join(tmpDir, "backend", "app", "routers", "automation_snapshots.py"); await fs.mkdir(path.dirname(dest), { recursive: true }); await fs.writeFile(dest, "original = True\n"); - const original = spyOn(Bun, "write").mockImplementation(async target => { - if (String(target).includes(".tmp")) { + const realOpen = fs.open.bind(fs); + const original = spyOn(fs, "open").mockImplementation(async (target, flags) => { + if (String(target).includes(".tmp") && flags === "wx") { const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; error.code = "EPERM"; throw error; } - throw new Error(`unexpected Bun.write to ${String(target)}`); + return realOpen(target, flags); }); try { await expect(writeFileAtomically(dest, "mutated = True\n")).rejects.toMatchObject({ code: "EPERM" }); @@ -77,13 +78,14 @@ describe("file tool atomicity and read-after-write (#4734)", () => { it("does not create a 0-byte destination when a new-file staged write fails", async () => { const dest = path.join(tmpDir, "new-file.py"); - const original = spyOn(Bun, "write").mockImplementation(async target => { - if (String(target).includes(".tmp")) { + const realOpen = fs.open.bind(fs); + const original = spyOn(fs, "open").mockImplementation(async (target, flags) => { + if (String(target).includes(".tmp") && flags === "wx") { const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; error.code = "EPERM"; throw error; } - throw new Error(`unexpected Bun.write to ${String(target)}`); + return realOpen(target, flags); }); try { await expect(writeFileAtomically(dest, "print('hi')\n")).rejects.toMatchObject({ code: "EPERM" }); @@ -99,7 +101,7 @@ describe("file tool atomicity and read-after-write (#4734)", () => { }); it("surfaces a permission error without leaving a 0-byte file in a read-only directory", async () => { - if (typeof process.getuid === "function" && process.getuid() === 0) return; + if (process.platform === "win32" || (typeof process.getuid === "function" && process.getuid() === 0)) return; const locked = path.join(tmpDir, "locked"); await fs.mkdir(locked); await fs.chmod(locked, 0o555); @@ -183,4 +185,55 @@ describe("file tool atomicity and read-after-write (#4734)", () => { await new WriteTool(session).execute("cache-write", { path: dest, content: "new line\n" }); expect(cache.get(dest)).toBeNull(); }); + + it("writes through a destination symlink without replacing the link", async () => { + const target = path.join(tmpDir, "real.ts"); + const link = path.join(tmpDir, "alias.ts"); + await fs.writeFile(target, "old\n"); + await fs.symlink(target, link); + await writeFileAtomically(link, "new\n"); + expect(await fs.readFile(target, "utf8")).toBe("new\n"); + expect((await fs.lstat(link)).isSymbolicLink()).toBe(true); + expect(await fs.readlink(link)).toBe(target); + }); + + it("retries exclusive temp creation when a sibling name already exists", async () => { + const dest = path.join(tmpDir, "retry.ts"); + const realOpen = fs.open.bind(fs); + let collisions = 0; + const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + if (String(target).includes(".tmp") && flags === "wx" && collisions < 1) { + collisions += 1; + const error = new Error("EEXIST: file already exists") as Error & { code: string }; + error.code = "EEXIST"; + throw error; + } + return realOpen(target, flags, mode); + }); + try { + await writeFileAtomically(dest, "after retry\n"); + expect(collisions).toBe(1); + expect(await fs.readFile(dest, "utf8")).toBe("after retry\n"); + } finally { + original.mockRestore(); + } + }); + + it("does not summarize a denied ACP file from disk", async () => { + const dest = path.join(tmpDir, "denied.ts"); + await fs.writeFile(dest, "export function secret() { return 1; }\nexport function other() { return 2; }\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + const error = new Error("permission denied by client") as Error & { code: string }; + error.code = "permission_denied"; + throw error; + }, + }; + await expect( + new ReadTool(createSession(tmpDir, { getClientBridge: () => bridge })).execute("summary-denied", { + path: dest, + }), + ).rejects.toThrow(/permission denied by client/); + }); }); From aae6906dc8506b01c2338449644e42d650e398a9 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Wed, 19 Aug 2026 16:32:22 +0000 Subject: [PATCH 03/13] fix(compaction): preserve multi-path file edits Compaction state already named recent single-path writes, but multi-file AST edits and apply-patch envelopes could disappear from the continuity hint. Extract all successful mutation paths so compaction retains the full recent file set. Lore-id: 4734file Constraint: preserve recent successful file-tool paths across compaction Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun --cwd=packages/coding-agent run check:types --- .../coding-agent/src/session/agent-session.ts | 39 ++++++++++++++----- ...ent-session-state-aware-compaction.test.ts | 30 ++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 94d299971f..2ef1a555b0 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -496,18 +496,34 @@ interface CompactionStateSnapshot { const FILE_MUTATION_TOOLS = new Set(["write", "edit", "apply_patch", "ast_edit"]); const MAX_RECENT_FILE_MUTATIONS = 12; +function collectFileMutationPaths(toolName: string, args: unknown): string[] { + if (!FILE_MUTATION_TOOLS.has(toolName) || !args || typeof args !== "object" || Array.isArray(args)) return []; + const record = args as Record; + const directPath = getStringProperty(record, "path") ?? getStringProperty(record, "file_path"); + if (directPath) return [directPath]; + + const paths = collectStringPaths(record.paths); + if (paths.length > 0) return paths; + + const input = getStringProperty(record, "input"); + if (!input) return []; + try { + return expandApplyPatchToEntries({ input }).map(entry => entry.path); + } catch { + return []; + } +} + function collectRecentFileMutations(messages: readonly AgentMessage[]): string[] { - const callsById = new Map(); + const callsById = new Map(); for (const message of messages) { if (message.role !== "assistant") continue; const content = (message as AssistantMessage).content; if (!Array.isArray(content)) continue; for (const block of content) { if (block.type !== "toolCall" || !FILE_MUTATION_TOOLS.has(block.name)) continue; - const args = block.arguments; - if (typeof args !== "object" || args === null) continue; - const filePath = (args as { path?: unknown }).path; - if (typeof filePath === "string" && filePath.length > 0) callsById.set(block.id, filePath); + const paths = collectFileMutationPaths(block.name, block.arguments).filter(path => path.length > 0); + if (paths.length > 0) callsById.set(block.id, paths); } } const seen = new Set(); @@ -517,10 +533,15 @@ function collectRecentFileMutations(messages: readonly AgentMessage[]): string[] if (message.role !== "toolResult") continue; const result = message as { toolName?: string; toolCallId?: string; isError?: boolean }; if (result.isError || !result.toolName || !FILE_MUTATION_TOOLS.has(result.toolName)) continue; - const filePath = result.toolCallId ? callsById.get(result.toolCallId) : undefined; - if (!filePath || seen.has(filePath)) continue; - seen.add(filePath); - paths.push(filePath); + const filePaths = result.toolCallId ? callsById.get(result.toolCallId) : undefined; + if (!filePaths) continue; + for (let pathIndex = filePaths.length - 1; pathIndex >= 0; pathIndex--) { + const filePath = filePaths[pathIndex]; + if (!filePath || seen.has(filePath)) continue; + seen.add(filePath); + paths.push(filePath); + if (paths.length >= MAX_RECENT_FILE_MUTATIONS) break; + } if (paths.length >= MAX_RECENT_FILE_MUTATIONS) break; } return paths; diff --git a/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts b/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts index 85284099ef..948cb0648b 100644 --- a/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts +++ b/packages/coding-agent/test/agent-session-state-aware-compaction.test.ts @@ -447,6 +447,36 @@ describe("AgentSession state-aware compaction", () => { expect(notices).toContain("Auto-continue skipped: no unfinished work detected"); }); + it("includes multi-path AST edits in compaction-state extraContext", async () => { + const astEditCall = assistantMessage("stop"); + astEditCall.content = [ + { + type: "toolCall", + id: "ast-edit-recent", + name: "ast_edit", + arguments: { + paths: ["src/first.ts", "src/second.ts"], + ops: [{ pat: "old", out: "new" }], + }, + }, + ]; + session.agent.appendMessage(astEditCall); + session.agent.appendMessage({ + role: "toolResult", + toolCallId: "ast-edit-recent", + toolName: "ast_edit", + content: [{ type: "text", text: "Applied 2 replacements in 2 files." }], + isError: false, + timestamp: Date.now(), + } as ToolResultMessage); + seedCompactionHistory(); + await session.compact(); + const options = compactSpy.mock.calls[0]?.[5]; + const files = options?.extraContext?.find(context => context.startsWith("Recent file mutations:")) ?? ""; + expect(files).toContain("src/first.ts"); + expect(files).toContain("src/second.ts"); + }); + it("continues synthetic auto-continue for an active nonterminal workflow", async () => { await seedActiveSkillState("active"); const promptSpy = vi.spyOn(session.agent, "prompt").mockResolvedValue(); From fa6c0269b40180e86b21828193e98a62b3727c9c Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Wed, 19 Aug 2026 16:33:10 +0000 Subject: [PATCH 04/13] fix(tools): do not claim dest unchanged after LSP writethrough A custom formatter can publish once before a later atomic write fails. The write-tool wrapper therefore refuses the 'original file was left unchanged' sentence; only the atomic helper itself still asserts that for a failed unpublished rename. Lore-id: 4734file Constraint: no directory fsync on user-file writes Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/file-tools-atomicity.test.ts Not-tested: injected failure after custom-formatter first publication --- packages/coding-agent/src/tools/atomic-file-write.ts | 2 +- packages/coding-agent/src/tools/write.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 537356f082..8422b23059 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -40,7 +40,7 @@ export function isFileWritePermissionError(error: unknown): boolean { } export function formatFileWriteError(error: unknown, dest: string, options: { destUnchanged?: boolean } = {}): string { - if (error instanceof FileWriteNotPublishedError) return error.message; + if (error instanceof FileWriteNotPublishedError && options.destUnchanged !== false) return error.message; if (isEisdir(error)) { return `Cannot write '${dest}': path is a directory.`; } diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index b55ef21ae9..e5aa99bb2e 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -765,7 +765,7 @@ export class WriteTool implements AgentTool Date: Wed, 19 Aug 2026 19:00:19 +0000 Subject: [PATCH 05/13] fix(file-tools): close exact-head review gaps The exact-head review identified ACP denial-shape loss, rename destinations omitted from compaction state, rename-based permission bypass, archive non-atomicity, trust-boundary escapes, mode drift, and staging leaks. Preserve authorization and publication invariants across all file-tool paths and add focused regressions. Lore-id: 4734file Constraint: preserve ACP authority decisions and atomic destination contracts Constraint: keep recent move destinations across compaction Confidence: high Scope-risk: wide Reversibility: easy Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/file-tools-atomicity.test.ts; bun test packages/coding-agent/test/agent-session-state-aware-compaction.test.ts; bun test packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts packages/coding-agent/test/acp-fs-provider-capabilities.test.ts; bun test packages/coding-agent/test/agent-session-acp-permission.test.ts --- .../src/sdk/host/reverse-leases.ts | 3 +- .../coding-agent/src/session/agent-session.ts | 29 ++-- .../src/tools/atomic-file-write.ts | 127 +++++++++++++++++- packages/coding-agent/src/tools/read.ts | 11 +- packages/coding-agent/src/tools/write.ts | 12 +- ...ent-session-state-aware-compaction.test.ts | 59 ++++++++ .../test/file-tools-atomicity.test.ts | 88 ++++++++++++ .../coding-agent/test/read-acp-fs.test.ts | 17 +++ 8 files changed, 326 insertions(+), 20 deletions(-) diff --git a/packages/coding-agent/src/sdk/host/reverse-leases.ts b/packages/coding-agent/src/sdk/host/reverse-leases.ts index 6db67007b4..6495f59dd0 100644 --- a/packages/coding-agent/src/sdk/host/reverse-leases.ts +++ b/packages/coding-agent/src/sdk/host/reverse-leases.ts @@ -281,8 +281,9 @@ export class ReverseLeaseRuntime { throw new ReverseLeaseError("not_lease_owner"); this.#takeOutstanding(id); if (error) { - const rejection = new Error(error.message); + const rejection = new Error(error.message) as Error & { code: string }; rejection.name = error.code; + rejection.code = error.code; request.reject(rejection); } else request.resolve(result); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 2ef1a555b0..5785548774 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -499,19 +499,32 @@ const MAX_RECENT_FILE_MUTATIONS = 12; function collectFileMutationPaths(toolName: string, args: unknown): string[] { if (!FILE_MUTATION_TOOLS.has(toolName) || !args || typeof args !== "object" || Array.isArray(args)) return []; const record = args as Record; + const paths: string[] = []; + const addPath = (value: unknown) => { + if (typeof value === "string" && value.length > 0 && !paths.includes(value)) paths.push(value); + }; const directPath = getStringProperty(record, "path") ?? getStringProperty(record, "file_path"); - if (directPath) return [directPath]; + addPath(directPath); - const paths = collectStringPaths(record.paths); - if (paths.length > 0) return paths; + for (const path of collectStringPaths(record.paths)) addPath(path); + const edits = Array.isArray(record.edits) ? record.edits : []; + for (const edit of edits) { + if (!edit || typeof edit !== "object" || Array.isArray(edit)) continue; + addPath(getStringProperty(edit as Record, "rename")); + } const input = getStringProperty(record, "input"); - if (!input) return []; - try { - return expandApplyPatchToEntries({ input }).map(entry => entry.path); - } catch { - return []; + if (input) { + try { + for (const entry of expandApplyPatchToEntries({ input })) { + addPath(entry.path); + addPath(entry.rename); + } + } catch { + // If the edit input is not an apply_patch envelope, retain direct paths. + } } + return paths; } function collectRecentFileMutations(messages: readonly AgentMessage[]): string[] { diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 8422b23059..ecb8d628fb 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -9,10 +9,21 @@ * publication does not need that durability barrier. * * Destination symlinks are followed: the referent is replaced, the link stays. - * Staging uses exclusive create (`wx`) so a colliding leftover temp is not - * truncated or unlinked. + * The referent is re-resolved immediately before publication so a retargeted + * link cannot silently repoint the write, and when the lexical destination + * sits inside a session-scoped `gjc-local` root the resolved referent and its + * parent must remain inside that root (a link there must not redirect the + * write out of the trust boundary). + * + * Staging uses exclusive create (`wx`) so a colliding leftover temp is never + * truncated or unlinked; only the temp this call created is cleaned on failure. + * Existing-file mode bits are re-applied after staging so a process umask never + * narrows a replaced file's permissions, and effective write authorization on an + * existing referent is checked before rename so a writable parent cannot bypass + * a read-only or ACL-denied target. */ import * as fs from "node:fs/promises"; +import * as os from "node:os"; import * as path from "node:path"; import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/utils"; @@ -54,6 +65,16 @@ export function formatFileWriteError(error: unknown, dest: string, options: { de return error instanceof Error ? error.message : String(error); } +export interface WriteFileAtomicallyOptions { + /** + * Trusted root that a resolved symlink referent and its parent must not + * leave. When omitted, the helper still enforces the session-scoped + * `gjc-local` boundary implied by a lexical destination under + * `/gjc-local/`. + */ + trustBoundary?: string; +} + function tempPathFor(dest: string): string { const unique = `${process.pid}.${Date.now().toString(36)}.${Math.random().toString(36).slice(2, 8)}`; return path.join(path.dirname(dest), `.${path.basename(dest)}.${unique}.tmp`); @@ -108,7 +129,7 @@ async function resolvePublishPath(dest: string, depth = 0): Promise<{ publishPat } } -async function writeExclusiveTemp(tmp: string, content: string, mode: number): Promise { +async function writeExclusiveTemp(tmp: string, content: string | Uint8Array, mode: number): Promise { const handle = await fs.open(tmp, "wx", mode); try { await handle.writeFile(content); @@ -120,23 +141,119 @@ async function writeExclusiveTemp(tmp: string, content: string, mode: number): P await handle.close(); } -export async function writeFileAtomically(dest: string, content: string): Promise { +function pathIsWithin(target: string, root: string): boolean { + return target === root || target.startsWith(`${root}${path.sep}`); +} + +async function realpathOrSelf(p: string): Promise { + try { + return await fs.realpath(p); + } catch { + return path.resolve(p); + } +} + +/** + * Derive the session-scoped `local://` trust boundary from a lexical write + * destination. Session roots live at `/gjc-local/`, and a + * symlink placed inside one must not be able to redirect a write out of it. + */ +function sessionLocalRootFor(lexicalDest: string): string | undefined { + const resolvedDest = path.resolve(lexicalDest); + const localParent = path.join(os.tmpdir(), "gjc-local"); + if (!pathIsWithin(resolvedDest, localParent)) return undefined; + const rest = resolvedDest.slice(localParent.length + path.sep.length); + const sessionSegment = rest.split(path.sep, 1)[0] ?? ""; + if (sessionSegment.length === 0) return undefined; + return path.join(localParent, sessionSegment); +} + +/** + * Reject publication when the resolved referent's real parent (and therefore + * the referent itself) leaves the trust boundary. The boundary root is + * realpathed so a symlinked `gjc-local` root cannot smuggle a write out. + */ +async function assertWithinTrustBoundary(publishPath: string, trustBoundary: string): Promise { + const boundary = path.resolve(trustBoundary); + const realBoundary = await realpathOrSelf(boundary); + const realParent = await realpathOrSelf(path.dirname(publishPath)); + if (!pathIsWithin(realParent, realBoundary)) { + throw new Error(`write target '${publishPath}' resolves outside trust boundary '${trustBoundary}'`); + } +} + +/** + * Rename replaces the referent without consulting its file permissions, so a + * writable parent could otherwise overwrite a read-only or ACL-denied target in + * a way a direct write would not. Probe effective write authorization the way a + * direct write would: open the existing referent for append (requires write + * permission and mutates nothing). Native Windows read-only attributes surface + * as EPERM/EACCES here just like POSIX immutable/`0444` targets. + */ +async function assertExistingTargetWritable(publishPath: string): Promise { + const handle = await fs.open(publishPath, "a"); + await handle.close(); +} + +/** + * Revalidate the resolved destination immediately before publication so a + * symlink retargeted while staging cannot silently repoint the write at a + * different file. + */ +async function assertPublishTargetStillIntended( + dest: string, + publishPath: string, + trustBoundary: string | undefined, +): Promise { + const after = await resolvePublishPath(dest); + if (after.publishPath !== publishPath) { + throw new Error(`destination '${dest}' was retargeted while staging; refusing to overwrite a different file`); + } + if (trustBoundary !== undefined) { + await assertWithinTrustBoundary(after.publishPath, trustBoundary); + } +} + +export async function writeFileAtomically( + dest: string, + content: string | Uint8Array, + options: WriteFileAtomicallyOptions = {}, +): Promise { let publishPath = dest; try { + const trustBoundary = options.trustBoundary ?? sessionLocalRootFor(dest); const resolved = await resolvePublishPath(dest); publishPath = resolved.publishPath; await fs.mkdir(path.dirname(publishPath), { recursive: true }); + if (trustBoundary !== undefined) { + await assertWithinTrustBoundary(publishPath, trustBoundary); + } const mode = resolved.existingMode ?? DEFAULT_FILE_MODE; + if (resolved.existingMode !== undefined) { + await assertExistingTargetWritable(publishPath); + } let lastError: unknown; for (let attempt = 0; attempt < TEMP_CREATE_ATTEMPTS; attempt++) { const tmp = tempPathFor(publishPath); + let owned = false; try { await writeExclusiveTemp(tmp, content, mode); + owned = true; + if (resolved.existingMode !== undefined) { + // Restore exact existing mode bits: the `wx` open applied the + // process umask, which would otherwise silently narrow them. + await fs.chmod(tmp, resolved.existingMode); + } + await assertPublishTargetStillIntended(dest, publishPath, trustBoundary); await renameIntoPlace(tmp, publishPath); return; } catch (error) { lastError = error; - if (hasFsCode(error, "EEXIST")) continue; + // A temp that never got created was a genuine pre-existing + // collision file: leave it alone and try a fresh sibling name. + if (hasFsCode(error, "EEXIST") && !owned) continue; + // Any failure after we exclusively created the temp must not leak it. + if (owned) await fs.unlink(tmp).catch(() => {}); throw error; } } diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index c36b345882..bdf7857dcd 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -1434,8 +1434,17 @@ interface ResolvedSqliteReadPath { * still fall back so an unreachable bridge cannot break local reads. */ function isClientAuthorityDenial(error: unknown): boolean { - const code = + const directCode = typeof error === "object" && error !== null && "code" in error ? (error as { code?: unknown }).code : undefined; + const namedCode = error instanceof Error ? error.name : undefined; + const nestedCode = + typeof error === "object" && + error !== null && + "data" in error && + typeof (error as { data?: unknown }).data === "object" + ? ((error as { data?: { code?: unknown } }).data?.code ?? undefined) + : undefined; + const code = directCode ?? nestedCode ?? namedCode; // OS errno codes are transport/availability failures, not an ACP permission // decision. Treating `EPERM`/`EACCES` as denials skipped the disk fallback // for files that already existed on disk. diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index e5aa99bb2e..b506615891 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -17,7 +17,7 @@ import type { ToolSession } from "../sdk"; import { Ellipsis, Hasher, type RenderCache, renderStatusLine, truncateToWidth } from "../tui"; import { resolveFileDisplayMode } from "../utils/file-display-mode"; import { parseArchivePathCandidates } from "./archive-reader"; -import { formatFileWriteError } from "./atomic-file-write"; +import { formatFileWriteError, writeFileAtomically } from "./atomic-file-write"; import { assertEditableFile } from "./auto-generated-guard"; import { type ConflictEntry, @@ -276,9 +276,9 @@ export class WriteTool implements AgentTool = {}; @@ -305,9 +305,11 @@ export class WriteTool implements AgentTool { expect(files).toContain("src/second.ts"); }); + it("includes edit rename source and destination in compaction-state extraContext", async () => { + const editCall = assistantMessage("stop"); + editCall.content = [ + { + type: "toolCall", + id: "edit-rename-recent", + name: "edit", + arguments: { + path: "src/old.ts", + edits: [{ op: "update", rename: "src/new.ts" }], + }, + }, + ]; + session.agent.appendMessage(editCall); + session.agent.appendMessage({ + role: "toolResult", + toolCallId: "edit-rename-recent", + toolName: "edit", + content: [{ type: "text", text: "Successfully edited src/old.ts" }], + isError: false, + timestamp: Date.now(), + } as ToolResultMessage); + seedCompactionHistory(); + await session.compact(); + const options = compactSpy.mock.calls[0]?.[5]; + const files = options?.extraContext?.find(context => context.startsWith("Recent file mutations:")) ?? ""; + expect(files).toContain("src/old.ts"); + expect(files).toContain("src/new.ts"); + }); + + it("includes apply_patch rename source and destination in compaction-state extraContext", async () => { + const applyPatchCall = assistantMessage("stop"); + applyPatchCall.content = [ + { + type: "toolCall", + id: "apply-patch-rename-recent", + name: "apply_patch", + arguments: { + input: "*** Begin Patch\n*** Update File: src/old.ts\n*** Move to: src/new.ts\n@@\n-old\n+new\n*** End Patch", + }, + }, + ]; + session.agent.appendMessage(applyPatchCall); + session.agent.appendMessage({ + role: "toolResult", + toolCallId: "apply-patch-rename-recent", + toolName: "apply_patch", + content: [{ type: "text", text: "Applied patch." }], + isError: false, + timestamp: Date.now(), + } as ToolResultMessage); + seedCompactionHistory(); + await session.compact(); + const options = compactSpy.mock.calls[0]?.[5]; + const files = options?.extraContext?.find(context => context.startsWith("Recent file mutations:")) ?? ""; + expect(files).toContain("src/old.ts"); + expect(files).toContain("src/new.ts"); + }); + it("continues synthetic auto-continue for an active nonterminal workflow", async () => { await seedActiveSkillState("active"); const promptSpy = vi.spyOn(session.agent, "prompt").mockResolvedValue(); diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index ab4bde92c1..ccdb89ddb4 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -219,6 +219,94 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("preserves exact mode bits when replacing an existing file", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "mode-preserved.ts"); + await fs.writeFile(dest, "old\n", { mode: 0o640 }); + await fs.chmod(dest, 0o640); + await writeFileAtomically(dest, "new\n"); + expect((await fs.stat(dest)).mode & 0o777).toBe(0o640); + }); + + it("does not replace an unwritable target through a writable parent", async () => { + if (process.platform === "win32" || (typeof process.getuid === "function" && process.getuid() === 0)) return; + const parent = path.join(tmpDir, "writable-parent"); + const dest = path.join(parent, "unwritable.ts"); + await fs.mkdir(parent); + await fs.writeFile(dest, "original\n"); + await fs.chmod(dest, 0o444); + try { + await expect(writeFileAtomically(dest, "replacement\n")).rejects.toThrow(/Permission denied|EACCES|EPERM/); + expect(await fs.readFile(dest, "utf8")).toBe("original\n"); + } finally { + await fs.chmod(dest, 0o644); + } + }); + + it("cleans an owned staging file when publication fails", async () => { + const dest = path.join(tmpDir, "rename-fails.ts"); + await fs.writeFile(dest, "original\n"); + const realRename = fs.rename.bind(fs); + const original = spyOn(fs, "rename").mockImplementation(async (from, to) => { + if (String(from).includes(".tmp")) { + const error = new Error("EIO: publication failed") as Error & { code: string }; + error.code = "EIO"; + throw error; + } + return realRename(from, to); + }); + try { + await expect(writeFileAtomically(dest, "replacement\n")).rejects.toMatchObject({ code: "EIO" }); + expect(await fs.readFile(dest, "utf8")).toBe("original\n"); + expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(false); + } finally { + original.mockRestore(); + } + }); + + it("rejects a symlink escape from the session-scoped gjc-local root", async () => { + if (process.platform === "win32") return; + const sessionRoot = path.join(os.tmpdir(), "gjc-local", "atomic-trust-test"); + const outside = path.join(tmpDir, "outside-secret.ts"); + const link = path.join(sessionRoot, "alias.ts"); + await fs.mkdir(sessionRoot, { recursive: true }); + await fs.writeFile(outside, "outside\n"); + await fs.symlink(outside, link); + try { + await expect(writeFileAtomically(link, "must-not-write\n")).rejects.toThrow(/outside trust boundary/); + expect(await fs.readFile(outside, "utf8")).toBe("outside\n"); + } finally { + await fs.rm(sessionRoot, { recursive: true, force: true }); + } + }); + + it("publishes rebuilt archive bytes atomically", async () => { + const archivePath = path.join(tmpDir, "archive.tar"); + await fs.writeFile(archivePath, await new Bun.Archive({ "pkg/old.txt": "old\n" }).bytes()); + const realOpen = fs.open.bind(fs); + const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + if (String(target).includes(".tmp") && flags === "wx") { + const error = new Error("EPERM: archive publication denied") as Error & { code: string }; + error.code = "EPERM"; + throw error; + } + return realOpen(target, flags, mode); + }); + try { + await expect( + new WriteTool(createSession(tmpDir)).execute("archive-atomic", { + path: `${archivePath}:pkg/new.txt`, + content: "new\n", + }), + ).rejects.toThrow(/Permission denied writing/); + const files = await new Bun.Archive(await fs.readFile(archivePath)).files(); + expect(await files.get("pkg/old.txt")?.text()).toBe("old\n"); + expect(files.has("pkg/new.txt")).toBe(false); + } finally { + original.mockRestore(); + } + }); + it("does not summarize a denied ACP file from disk", async () => { const dest = path.join(tmpDir, "denied.ts"); await fs.writeFile(dest, "export function secret() { return 1; }\nexport function other() { return 2; }\n"); diff --git a/packages/coding-agent/test/read-acp-fs.test.ts b/packages/coding-agent/test/read-acp-fs.test.ts index bd3f2b861f..fe093a055f 100644 --- a/packages/coding-agent/test/read-acp-fs.test.ts +++ b/packages/coding-agent/test/read-acp-fs.test.ts @@ -88,6 +88,23 @@ describe("read tool ACP fs routing", () => { } }); + it("does not fall back to a disk secret when the ACP provider denies the read", async () => { + const filePath = path.join(tmpDir, "secret.ts"); + await fs.writeFile(filePath, "export const secret = 'disk-only';\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + const error = new Error("Request rejected"); + error.name = "permission_denied"; + throw error; + }, + }; + + await expect( + new ReadTool(createSession(tmpDir, bridge)).execute("denied-reverse-read", { path: filePath }), + ).rejects.toThrow("Request rejected"); + }); + it("applies requested line ranges to bridge content exactly once", async () => { const filePath = path.join(tmpDir, "range.txt"); await fs.writeFile(filePath, "disk one\ndisk two\ndisk three\n"); From 5ab80c4c3896401e3f8fff07fbc375015a20409c Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 05:27:56 +0000 Subject: [PATCH 06/13] fix(file-tools): preserve atomic target invariants Sync staged bytes before publication, preserve ownership, reject hard-link replacement, detect identity and parent retargets, propagate realpath failures, and clean owned temps even when close or unlink fails. Direct write failures now retain the original-file wording. Lore-id: 4734atomic Constraint: preserve atomic visibility and target identity across current dev Constraint: no release or shared-worktree mutation Confidence: high Scope-risk: medium Reversibility: easy Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/file-tools-atomicity.test.ts; bun test packages/coding-agent/test/agent-session-state-aware-compaction.test.ts; bun test packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts packages/coding-agent/test/acp-fs-provider-capabilities.test.ts packages/coding-agent/test/agent-session-acp-permission.test.ts --- .../src/tools/atomic-file-write.ts | 117 +++++++++++++----- packages/coding-agent/src/tools/write.ts | 2 +- .../test/file-tools-atomicity.test.ts | 111 +++++++++++++++++ 3 files changed, 201 insertions(+), 29 deletions(-) diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index ecb8d628fb..29c4a8bd06 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -17,10 +17,12 @@ * * Staging uses exclusive create (`wx`) so a colliding leftover temp is never * truncated or unlinked; only the temp this call created is cleaned on failure. - * Existing-file mode bits are re-applied after staging so a process umask never - * narrows a replaced file's permissions, and effective write authorization on an - * existing referent is checked before rename so a writable parent cannot bypass - * a read-only or ACL-denied target. + * Existing-file mode and ownership are re-applied after staging so a process + * umask or replacement inode never changes the target's identity. Hard-linked + * targets are rejected because replacement would split their link group, and + * the target identity is revalidated immediately before rename. The staged + * bytes are synced before publication; directory fsync is intentionally not + * promised because Windows reports EPERM for it (#4457). */ import * as fs from "node:fs/promises"; import * as os from "node:os"; @@ -32,6 +34,20 @@ const WINDOWS_SHARING_VIOLATION_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); const TEMP_CREATE_ATTEMPTS = 8; const DEFAULT_FILE_MODE = 0o666; +interface ExistingFileMetadata { + mode: number; + uid: number; + gid: number; + nlink: number; + dev: number; + ino: number; +} + +interface ResolvedPublishPath { + publishPath: string; + existing?: ExistingFileMetadata; +} + export class FileWriteNotPublishedError extends Error { readonly dest: string; override readonly cause: unknown; @@ -111,7 +127,7 @@ function eisdir(dest: string): Error & { code: string } { return error; } -async function resolvePublishPath(dest: string, depth = 0): Promise<{ publishPath: string; existingMode?: number }> { +async function resolvePublishPath(dest: string, depth = 0): Promise { if (depth > 40) { throw new Error(`ELOOP: too many symbolic links, write '${dest}'`); } @@ -122,25 +138,23 @@ async function resolvePublishPath(dest: string, depth = 0): Promise<{ publishPat const target = await fs.readlink(dest); return resolvePublishPath(path.resolve(path.dirname(dest), target), depth + 1); } - return { publishPath: dest, existingMode: lst.mode }; + return { + publishPath: dest, + existing: { + mode: lst.mode, + uid: lst.uid, + gid: lst.gid, + nlink: lst.nlink, + dev: lst.dev, + ino: lst.ino, + }, + }; } catch (error) { if (!isEnoent(error)) throw error; return { publishPath: dest }; } } -async function writeExclusiveTemp(tmp: string, content: string | Uint8Array, mode: number): Promise { - const handle = await fs.open(tmp, "wx", mode); - try { - await handle.writeFile(content); - } catch (error) { - await handle.close().catch(() => {}); - await fs.unlink(tmp).catch(() => {}); - throw error; - } - await handle.close(); -} - function pathIsWithin(target: string, root: string): boolean { return target === root || target.startsWith(`${root}${path.sep}`); } @@ -148,7 +162,8 @@ function pathIsWithin(target: string, root: string): boolean { async function realpathOrSelf(p: string): Promise { try { return await fs.realpath(p); - } catch { + } catch (error) { + if (!isEnoent(error)) throw error; return path.resolve(p); } } @@ -195,6 +210,27 @@ async function assertExistingTargetWritable(publishPath: string): Promise await handle.close(); } +function sameFileIdentity(left: ExistingFileMetadata, right: ExistingFileMetadata): boolean { + return left.dev === right.dev && left.ino === right.ino; +} + +async function preserveExistingMetadata(tmp: string, existing: ExistingFileMetadata): Promise { + const staged = await fs.stat(tmp); + if (staged.uid !== existing.uid || staged.gid !== existing.gid) { + await fs.chown(tmp, existing.uid, existing.gid); + } + await fs.chmod(tmp, existing.mode); +} + +async function cleanupOwnedTemp(tmp: string, cause: unknown): Promise { + try { + await fs.unlink(tmp); + } catch (cleanupError) { + if (isEnoent(cleanupError)) return; + throw new AggregateError([cause, cleanupError], `Failed to clean up staging file '${tmp}'.`); + } +} + /** * Revalidate the resolved destination immediately before publication so a * symlink retargeted while staging cannot silently repoint the write at a @@ -204,11 +240,26 @@ async function assertPublishTargetStillIntended( dest: string, publishPath: string, trustBoundary: string | undefined, + expectedExisting: ExistingFileMetadata | undefined, + expectedParentRealpath: string, ): Promise { const after = await resolvePublishPath(dest); if (after.publishPath !== publishPath) { throw new Error(`destination '${dest}' was retargeted while staging; refusing to overwrite a different file`); } + const currentParentRealpath = await realpathOrSelf(path.dirname(publishPath)); + if (currentParentRealpath !== expectedParentRealpath) { + throw new Error( + `destination '${dest}' parent was retargeted while staging; refusing to overwrite a different file`, + ); + } + if (expectedExisting === undefined) { + if (after.existing !== undefined) { + throw new Error(`destination '${dest}' appeared while staging; refusing to overwrite a different file`); + } + } else if (after.existing === undefined || !sameFileIdentity(after.existing, expectedExisting)) { + throw new Error(`destination '${dest}' was replaced while staging; refusing to overwrite a different file`); + } if (trustBoundary !== undefined) { await assertWithinTrustBoundary(after.publishPath, trustBoundary); } @@ -225,11 +276,17 @@ export async function writeFileAtomically( const resolved = await resolvePublishPath(dest); publishPath = resolved.publishPath; await fs.mkdir(path.dirname(publishPath), { recursive: true }); + const expectedParentRealpath = await realpathOrSelf(path.dirname(publishPath)); if (trustBoundary !== undefined) { await assertWithinTrustBoundary(publishPath, trustBoundary); } - const mode = resolved.existingMode ?? DEFAULT_FILE_MODE; - if (resolved.existingMode !== undefined) { + const existing = resolved.existing; + if (existing !== undefined && existing.nlink > 1) { + throw new Error( + `Cannot atomically replace hard-linked file '${dest}': replacement would split its link group.`, + ); + } + if (existing !== undefined) { await assertExistingTargetWritable(publishPath); } let lastError: unknown; @@ -237,14 +294,18 @@ export async function writeFileAtomically( const tmp = tempPathFor(publishPath); let owned = false; try { - await writeExclusiveTemp(tmp, content, mode); + const handle = await fs.open(tmp, "wx", existing?.mode ?? DEFAULT_FILE_MODE); owned = true; - if (resolved.existingMode !== undefined) { - // Restore exact existing mode bits: the `wx` open applied the - // process umask, which would otherwise silently narrow them. - await fs.chmod(tmp, resolved.existingMode); + try { + await handle.writeFile(content); + await handle.sync(); + } finally { + await handle.close(); + } + if (existing !== undefined) { + await preserveExistingMetadata(tmp, existing); } - await assertPublishTargetStillIntended(dest, publishPath, trustBoundary); + await assertPublishTargetStillIntended(dest, publishPath, trustBoundary, existing, expectedParentRealpath); await renameIntoPlace(tmp, publishPath); return; } catch (error) { @@ -253,7 +314,7 @@ export async function writeFileAtomically( // collision file: leave it alone and try a fresh sibling name. if (hasFsCode(error, "EEXIST") && !owned) continue; // Any failure after we exclusively created the temp must not leak it. - if (owned) await fs.unlink(tmp).catch(() => {}); + if (owned) await cleanupOwnedTemp(tmp, error); throw error; } } diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index b506615891..05c0576d27 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -767,7 +767,7 @@ export class WriteTool implements AgentTool { expect((await fs.stat(dest)).mode & 0o777).toBe(0o640); }); + it("preserves ownership and syncs staged bytes before publication", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "metadata-preserved.ts"); + await fs.writeFile(dest, "old\n", { mode: 0o640 }); + const before = await fs.stat(dest); + const realOpen = fs.open.bind(fs); + let syncs = 0; + const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + const handle = await realOpen(target, flags, mode); + if (String(target).includes(".tmp") && flags === "wx") { + const realSync = handle.sync.bind(handle); + spyOn(handle, "sync").mockImplementation(async () => { + syncs += 1; + return realSync(); + }); + } + return handle; + }); + try { + await writeFileAtomically(dest, "new\n"); + const after = await fs.stat(dest); + expect(syncs).toBe(1); + expect(after.uid).toBe(before.uid); + expect(after.gid).toBe(before.gid); + } finally { + original.mockRestore(); + } + }); + + it("rejects hard-linked destinations instead of splitting the link group", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "hard-linked.ts"); + const peer = path.join(tmpDir, "hard-linked-peer.ts"); + await fs.writeFile(dest, "original\n"); + await fs.link(dest, peer); + await expect(writeFileAtomically(dest, "replacement\n")).rejects.toThrow(/hard-linked/); + expect(await fs.readFile(dest, "utf8")).toBe("original\n"); + expect(await fs.readFile(peer, "utf8")).toBe("original\n"); + }); + + it("rejects a destination identity swap during publication", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "identity-swap.ts"); + const replacement = path.join(tmpDir, "identity-replacement.ts"); + const originalPath = path.join(tmpDir, "identity-original.ts"); + await fs.writeFile(dest, "original\n"); + await fs.writeFile(replacement, "replacement\n"); + const realRename = (from: string, to: string) => fs.rename(from, to); + let swapped = false; + const realChmod = fs.chmod.bind(fs) as (target: string, mode: number) => Promise; + const original = spyOn(fs, "chmod").mockImplementation(async (target, mode) => { + if (String(target).includes(".tmp") && !swapped) { + swapped = true; + await realRename(dest, originalPath); + await realRename(replacement, dest); + } + return realChmod(String(target), mode as number); + }); + try { + await expect(writeFileAtomically(dest, "must-not-overwrite\n")).rejects.toThrow(/replaced while staging/); + expect(await fs.readFile(dest, "utf8")).toBe("replacement\n"); + } finally { + original.mockRestore(); + await fs.rm(originalPath, { force: true }); + } + }); + + it("does not flatten non-ENOENT trust-boundary resolution errors", async () => { + const dest = path.join(tmpDir, "realpath-error.ts"); + const error = new Error("EIO: realpath failed") as Error & { code: string }; + error.code = "EIO"; + const original = spyOn(fs, "realpath").mockRejectedValueOnce(error); + try { + await expect(writeFileAtomically(dest, "must-fail\n", { trustBoundary: tmpDir })).rejects.toMatchObject({ + code: "EIO", + }); + expect( + await fs.stat(dest).then( + () => true, + () => false, + ), + ).toBe(false); + } finally { + original.mockRestore(); + } + }); + + it("cleans a staged file when its close fails", async () => { + const dest = path.join(tmpDir, "close-fails.ts"); + const realOpen = fs.open.bind(fs); + const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + const handle = await realOpen(target, flags, mode); + if (String(target).includes(".tmp") && flags === "wx") { + const realClose = handle.close.bind(handle); + spyOn(handle, "close").mockImplementation(async () => { + await realClose(); + const error = new Error("EIO: close failed") as Error & { code: string }; + error.code = "EIO"; + throw error; + }); + } + return handle; + }); + try { + await expect(writeFileAtomically(dest, "must-fail\n")).rejects.toMatchObject({ code: "EIO" }); + expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(false); + } finally { + original.mockRestore(); + } + }); + it("does not replace an unwritable target through a writable parent", async () => { if (process.platform === "win32" || (typeof process.getuid === "function" && process.getuid() === 0)) return; const parent = path.join(tmpDir, "writable-parent"); From 8e0924ddd793f71dbb61aef616ad9036ecd5116d Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 06:18:16 +0000 Subject: [PATCH 07/13] fix(file-tools): bind publication to target identity A final JavaScript pathname check still left atomic replacement vulnerable to a concurrent destination swap. Native identity-bound publication now rejects a changed destination and no-replace creation refuses a concurrent creator instead of falling back to plain rename. Writethrough failures also retain whether an earlier formatter publication already committed. Lore-id: 4734atomic\nConstraint: no pathname rename fallback after identity validation\nConstraint: preserve honest post-publication error wording\nConfidence: high\nScope-risk: medium\nReversibility: easy\nTested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/file-tools-atomicity.test.ts packages/coding-agent/test/tools/lsp-batching.test.ts packages/coding-agent/test/read-acp-fs.test.ts packages/coding-agent/test/write-acp-fs.test.ts packages/coding-agent/test/agent-session-state-aware-compaction.test.ts\nNot-tested: unsupported native filesystem primitives --- packages/coding-agent/src/lsp/index.ts | 15 +- .../src/tools/atomic-file-write.ts | 165 +++++++++++++++--- .../test/file-tools-atomicity.test.ts | 111 +++++++++++- .../test/tools/lsp-batching.test.ts | 34 ++++ 4 files changed, 288 insertions(+), 37 deletions(-) diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index a122841619..6ca468632a 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -6,7 +6,7 @@ import type { BunFile } from "bun"; import { type Theme, theme } from "../modes/theme/theme"; import lspDescription from "../prompts/tools/lsp.md" with { type: "text" }; import type { ToolSession } from "../tools"; -import { writeFileAtomically } from "../tools/atomic-file-write"; +import { FileWriteNotPublishedError, writeFileAtomically } from "../tools/atomic-file-write"; import { formatPathRelativeToCwd, resolveToCwd } from "../tools/path-utils"; import { ToolAbortError, ToolError, throwIfAborted } from "../tools/tool-errors"; import { clampTimeout } from "../tools/tool-timeouts"; @@ -918,7 +918,18 @@ async function runLspWritethrough( const { lspServers, customLinterServers } = splitServers(servers); let finalContent = content; - const writeContent = async (value: string) => writeFileAtomically(dst, value); + let publishedContent = false; + const writeContent = async (value: string) => { + try { + await writeFileAtomically(dst, value); + publishedContent = true; + } catch (error) { + if (publishedContent && error instanceof FileWriteNotPublishedError) { + throw new FileWriteNotPublishedError(dst, error.cause, { destUnchanged: false }); + } + throw error; + } + }; const getWritePromise = once(() => writeContent(finalContent)); const useCustomFormatter = enableFormat && customLinterServers.length > 0; diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 29c4a8bd06..84b9d7acb0 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -20,20 +20,39 @@ * Existing-file mode and ownership are re-applied after staging so a process * umask or replacement inode never changes the target's identity. Hard-linked * targets are rejected because replacement would split their link group, and - * the target identity is revalidated immediately before rename. The staged + * the target identity is revalidated inside the native conditional publication + * primitive. The staged * bytes are synced before publication; directory fsync is intentionally not * promised because Windows reports EPERM for it (#4457). */ + +import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import type { NativeExactFileIdentity, NativeExactUnlinkResult, NativeNoReplaceResult } from "@gajae-code/natives"; import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/utils"; -const WINDOWS_RENAME_BACKOFF_MS = [10, 25, 50, 100, 200] as const; -const WINDOWS_SHARING_VIOLATION_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); const TEMP_CREATE_ATTEMPTS = 8; const DEFAULT_FILE_MODE = 0o666; +type NativeAtomicPublishBindings = { + exactReplacePath: ( + sourcePath: string, + destinationPath: string, + expectedSource: NativeExactFileIdentity, + expectedDestination: NativeExactFileIdentity, + ) => NativeExactUnlinkResult; + renameNoReplacePath: (sourcePath: string, destinationPath: string) => NativeNoReplaceResult; +}; + +let nativeAtomicPublishBindings: NativeAtomicPublishBindings | undefined; + +function getNativeAtomicPublishBindings(): NativeAtomicPublishBindings { + nativeAtomicPublishBindings ??= require("@gajae-code/natives") as NativeAtomicPublishBindings; + return nativeAtomicPublishBindings; +} + interface ExistingFileMetadata { mode: number; uid: number; @@ -43,6 +62,11 @@ interface ExistingFileMetadata { ino: number; } +type NativePublicationError = Error & { + code?: string; + publicationUncertain?: boolean; +}; + interface ResolvedPublishPath { publishPath: string; existing?: ExistingFileMetadata; @@ -50,11 +74,14 @@ interface ResolvedPublishPath { export class FileWriteNotPublishedError extends Error { readonly dest: string; + readonly destUnchanged: boolean; override readonly cause: unknown; - constructor(dest: string, cause: unknown) { - super(formatFileWriteError(cause, dest, { destUnchanged: true })); + constructor(dest: string, cause: unknown, options: { destUnchanged?: boolean } = {}) { + const destUnchanged = options.destUnchanged ?? true; + super(formatFileWriteError(cause, dest, { destUnchanged })); this.name = "FileWriteNotPublishedError"; this.dest = dest; + this.destUnchanged = destUnchanged; this.cause = cause; if (isFsError(cause)) { (this as Error & { code?: string }).code = cause.code; @@ -96,27 +123,57 @@ function tempPathFor(dest: string): string { return path.join(path.dirname(dest), `.${path.basename(dest)}.${unique}.tmp`); } -async function renameIntoPlace(from: string, to: string): Promise { +function sha256(bytes: ArrayBuffer): string { + return crypto.createHash("sha256").update(new Uint8Array(bytes)).digest("hex"); +} + +async function captureExactFileIdentity(file: string): Promise { try { - await fs.rename(from, to); - return; + const [bytes, stat, parent] = await Promise.all([ + Bun.file(file).arrayBuffer(), + fs.stat(file, { bigint: true }), + fs.stat(path.dirname(file), { bigint: true }), + ]); + return { + dev: stat.dev, + ino: stat.ino, + nlink: stat.nlink, + parentDev: parent.dev, + parentIno: parent.ino, + size: stat.size, + mtimeNs: stat.mtimeNs, + directory: false, + detachOnly: false, + sha256: sha256(bytes), + }; } catch (error) { - if (process.platform !== "win32" || !isFsError(error) || !WINDOWS_SHARING_VIOLATION_CODES.has(error.code)) { - throw error; - } - } - let lastError: unknown; - for (const delay of WINDOWS_RENAME_BACKOFF_MS) { - await Bun.sleep(delay); - try { - await fs.rename(from, to); - return; - } catch (error) { - lastError = error; - if (!isFsError(error) || !WINDOWS_SHARING_VIOLATION_CODES.has(error.code)) throw error; - } + if (isEnoent(error)) return undefined; + throw error; } - throw lastError; +} + +function nativePublicationError( + operation: string, + code: string | undefined, + publicationUncertain = false, +): NativePublicationError { + const error = new Error( + publicationUncertain + ? `${operation} reached an uncertain publication state (${code ?? "unknown"}).` + : `${operation} failed (${code ?? "unknown"}).`, + ) as NativePublicationError; + if (code !== undefined) error.code = code; + if (publicationUncertain) error.publicationUncertain = true; + return error; +} + +function hasRetainedPublication(result: NativeExactUnlinkResult): boolean { + return ( + result.detachedPath !== undefined || + result.retainedSuccessorPath !== undefined || + result.retainedPlaceholderPath !== undefined || + result.retainedUnknownPath !== undefined + ); } function eisdir(dest: string): Error & { code: string } { @@ -211,7 +268,14 @@ async function assertExistingTargetWritable(publishPath: string): Promise } function sameFileIdentity(left: ExistingFileMetadata, right: ExistingFileMetadata): boolean { - return left.dev === right.dev && left.ino === right.ino; + return ( + left.dev === right.dev && + left.ino === right.ino && + left.nlink === right.nlink && + left.mode === right.mode && + left.uid === right.uid && + left.gid === right.gid + ); } async function preserveExistingMetadata(tmp: string, existing: ExistingFileMetadata): Promise { @@ -306,7 +370,51 @@ export async function writeFileAtomically( await preserveExistingMetadata(tmp, existing); } await assertPublishTargetStillIntended(dest, publishPath, trustBoundary, existing, expectedParentRealpath); - await renameIntoPlace(tmp, publishPath); + if (existing === undefined) { + // A plain rename would allow a concurrent creator to be overwritten + // after the JavaScript identity check. Fail closed when the native + // no-replace primitive cannot establish the publication boundary. + const published = getNativeAtomicPublishBindings().renameNoReplacePath(tmp, publishPath); + if (!published.ok) { + const uncertain = published.mutationState !== "not_committed"; + if (uncertain) owned = false; + throw nativePublicationError("Atomic creation", published.code, uncertain); + } + owned = false; + } else { + // The native exchange validates both identities in the same namespace + // transaction. Do not fall back to pathname rename: validation and a + // plain rename are otherwise separable under a concurrent writer. + const expectedDestination = await captureExactFileIdentity(publishPath); + if (expectedDestination === undefined) { + throw new Error(`destination '${dest}' disappeared while staging`); + } + if ( + Number(expectedDestination.dev) !== existing.dev || + Number(expectedDestination.ino) !== existing.ino || + Number(expectedDestination.nlink ?? -1n) !== existing.nlink + ) { + throw new Error( + `destination '${dest}' was replaced while staging; refusing to overwrite a different file`, + ); + } + const expectedSource = await captureExactFileIdentity(tmp); + if (expectedSource === undefined) { + throw new Error(`staging file '${tmp}' disappeared before publication`); + } + const published = getNativeAtomicPublishBindings().exactReplacePath( + tmp, + publishPath, + expectedSource, + expectedDestination, + ); + if (!published.ok) { + const uncertain = hasRetainedPublication(published); + if (uncertain) owned = false; + throw nativePublicationError("Atomic replacement", published.code, uncertain); + } + owned = false; + } return; } catch (error) { lastError = error; @@ -321,6 +429,11 @@ export async function writeFileAtomically( throw lastError; } catch (error) { if (error instanceof FileWriteNotPublishedError) throw error; - throw new FileWriteNotPublishedError(dest, error); + const publicationUncertain = + error !== null && + typeof error === "object" && + "publicationUncertain" in error && + error.publicationUncertain === true; + throw new FileWriteNotPublishedError(dest, error, { destUnchanged: !publicationUncertain }); } } diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index be254074a3..0388b45fa8 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -1,4 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import type { PathLike, StatOptions } from "node:fs"; +import * as fsSync from "node:fs"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -7,6 +9,7 @@ import type { ClientBridge } from "@gajae-code/coding-agent/session/client-bridg import type { ToolSession } from "@gajae-code/coding-agent/tools"; import { ReadTool } from "@gajae-code/coding-agent/tools/read"; import { WriteTool } from "@gajae-code/coding-agent/tools/write"; +import * as natives from "@gajae-code/natives"; import { FileReadCache } from "../src/edit/file-read-cache"; import { writeFileAtomically } from "../src/tools/atomic-file-write"; @@ -257,6 +260,36 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("re-applies ownership when the staged inode has different metadata", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "ownership-reapplied.ts"); + await fs.writeFile(dest, "old\n", { mode: 0o640 }); + const before = await fs.stat(dest); + const realStat = fs.stat.bind(fs); + const realChown = fs.chown.bind(fs); + const chowns: Array<{ uid: number; gid: number }> = []; + const statImplementation = async (target: PathLike, options?: StatOptions) => { + const result = options === undefined ? await realStat(target) : await realStat(target, options); + if (result === undefined) return result; + if (String(target).includes(".tmp") && options?.bigint !== true) { + return Object.assign(result, { uid: before.uid + 1, gid: before.gid + 1 }); + } + return result; + }; + const stat = spyOn(fs, "stat").mockImplementation(statImplementation as typeof fs.stat); + const chown = spyOn(fs, "chown").mockImplementation(async (_target, uid, gid) => { + chowns.push({ uid, gid }); + await realChown(dest, before.uid, before.gid); + }); + try { + await writeFileAtomically(dest, "new\n"); + expect(chowns).toEqual([{ uid: before.uid, gid: before.gid }]); + } finally { + stat.mockRestore(); + chown.mockRestore(); + } + }); + it("rejects hard-linked destinations instead of splitting the link group", async () => { if (process.platform === "win32") return; const dest = path.join(tmpDir, "hard-linked.ts"); @@ -295,6 +328,30 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("uses the native identity-bound primitive at the publication boundary", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "native-boundary.ts"); + const successor = path.join(tmpDir, "native-successor.ts"); + const displaced = path.join(tmpDir, "native-displaced.ts"); + await fs.writeFile(dest, "original\n"); + await fs.writeFile(successor, "successor\n"); + const realExactReplace = natives.exactReplacePath; + const original = spyOn(natives, "exactReplacePath").mockImplementation( + (sourcePath, destinationPath, expectedSource, expectedDestination) => { + fsSync.renameSync(destinationPath, displaced); + fsSync.renameSync(successor, destinationPath); + return realExactReplace(sourcePath, destinationPath, expectedSource, expectedDestination); + }, + ); + try { + await expect(writeFileAtomically(dest, "must-not-overwrite\n")).rejects.toThrow(/identity_mismatch/); + expect(await fs.readFile(dest, "utf8")).toBe("successor\n"); + } finally { + original.mockRestore(); + await fs.rm(displaced, { force: true }); + } + }); + it("does not flatten non-ENOENT trust-boundary resolution errors", async () => { const dest = path.join(tmpDir, "realpath-error.ts"); const error = new Error("EIO: realpath failed") as Error & { code: string }; @@ -339,6 +396,24 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("cleans a staged file when syncing bytes fails", async () => { + const dest = path.join(tmpDir, "sync-fails.ts"); + const realOpen = fs.open.bind(fs); + const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + const handle = await realOpen(target, flags, mode); + if (String(target).includes(".tmp") && flags === "wx") { + spyOn(handle, "sync").mockRejectedValue(Object.assign(new Error("EIO: sync failed"), { code: "EIO" })); + } + return handle; + }); + try { + await expect(writeFileAtomically(dest, "must-fail\n")).rejects.toMatchObject({ code: "EIO" }); + expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(false); + } finally { + original.mockRestore(); + } + }); + it("does not replace an unwritable target through a writable parent", async () => { if (process.platform === "win32" || (typeof process.getuid === "function" && process.getuid() === 0)) return; const parent = path.join(tmpDir, "writable-parent"); @@ -357,15 +432,10 @@ describe("file tool atomicity and read-after-write (#4734)", () => { it("cleans an owned staging file when publication fails", async () => { const dest = path.join(tmpDir, "rename-fails.ts"); await fs.writeFile(dest, "original\n"); - const realRename = fs.rename.bind(fs); - const original = spyOn(fs, "rename").mockImplementation(async (from, to) => { - if (String(from).includes(".tmp")) { - const error = new Error("EIO: publication failed") as Error & { code: string }; - error.code = "EIO"; - throw error; - } - return realRename(from, to); - }); + const original = spyOn(natives, "exactReplacePath").mockImplementation(() => ({ + ok: false, + code: "EIO", + })); try { await expect(writeFileAtomically(dest, "replacement\n")).rejects.toMatchObject({ code: "EIO" }); expect(await fs.readFile(dest, "utf8")).toBe("original\n"); @@ -375,6 +445,29 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("reports cleanup failure without claiming the staged file was removed", async () => { + const dest = path.join(tmpDir, "unlink-fails.ts"); + await fs.writeFile(dest, "original\n"); + const replace = spyOn(natives, "exactReplacePath").mockImplementation(() => ({ + ok: false, + code: "EIO", + })); + const realUnlink = fs.unlink.bind(fs); + const unlink = spyOn(fs, "unlink").mockImplementation(async target => { + if (String(target).includes(".tmp")) { + throw Object.assign(new Error("EIO: unlink failed"), { code: "EIO" }); + } + return realUnlink(target); + }); + try { + await expect(writeFileAtomically(dest, "replacement\n")).rejects.toThrow(/Failed to clean up staging file/); + expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(true); + } finally { + replace.mockRestore(); + unlink.mockRestore(); + } + }); + it("rejects a symlink escape from the session-scoped gjc-local root", async () => { if (process.platform === "win32") return; const sessionRoot = path.join(os.tmpdir(), "gjc-local", "atomic-trust-test"); diff --git a/packages/coding-agent/test/tools/lsp-batching.test.ts b/packages/coding-agent/test/tools/lsp-batching.test.ts index da4681aac2..f28314c5a2 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -3,6 +3,9 @@ import * as path from "node:path"; import { createLspWritethrough } from "@gajae-code/coding-agent/lsp"; import * as lspConfig from "@gajae-code/coding-agent/lsp/config"; import { TempDir } from "@gajae-code/utils"; +import type { ServerConfig } from "../../src/lsp/types"; +import * as atomicFileWrite from "../../src/tools/atomic-file-write"; +import { FileWriteNotPublishedError } from "../../src/tools/atomic-file-write"; describe("createLspWritethrough batching", () => { let tempDir: TempDir; @@ -64,4 +67,35 @@ describe("createLspWritethrough batching", () => { expect(loadConfigSpy).toHaveBeenCalledTimes(1); expect(await Bun.file(filePath).text()).toBe("const single = true;\n"); }); + + it("reports a later publication failure as potentially replacing an earlier write", async () => { + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + const client = { + format: async () => "const formatted = true;\n", + lint: async () => [], + }; + const server: ServerConfig = { + command: "custom-formatter", + fileTypes: ["ts"], + rootMarkers: [], + createClient: () => client, + }; + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["custom", server]]); + const filePath = path.join(tempDir.path(), "later-failure.ts"); + let writes = 0; + const atomicFailure = new FileWriteNotPublishedError( + filePath, + Object.assign(new Error("EIO: publication failed"), { code: "EIO" }), + ); + vi.spyOn(atomicFileWrite, "writeFileAtomically").mockImplementation(async () => { + writes += 1; + if (writes > 1) throw atomicFailure; + }); + + const writethrough = createLspWritethrough(tempDir.path(), { enableFormat: true }); + await expect(writethrough(filePath, "const original = true;\n")).rejects.toMatchObject({ + destUnchanged: false, + }); + expect(writes).toBeGreaterThan(1); + }); }); From 11a569f1be8a6a8625cfe708d2507a22a91d4e46 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 12:59:33 +0000 Subject: [PATCH 08/13] fix(file-tools): use plain atomic rename for user writes Ordinary user-file writes do not need the native identity exchange used by managed configuration. Keep the staged bytes and metadata durable, publish with a same-directory atomic rename, and clean only the temp owned by this call. This keeps failed destinations byte-identical without retaining exchange recovery artifacts. Lore-id: 4734rename\nConstraint: ordinary user writes must not invoke exactReplacePath or exchange recovery\nConstraint: failed writes leave the destination unchanged and no owned residue\nConstraint: preserve ACP read ordering and compaction mutation continuity\nConfidence: high\nScope-risk: medium\nReversibility: easy\nTested: atomicity, ACP, LSP, compaction, coding-agent check, state-writer gate\nNot-tested: cross-platform native rename behavior --- .../src/tools/atomic-file-write.ts | 139 +----------------- .../test/file-tools-atomicity.test.ts | 58 ++++---- 2 files changed, 36 insertions(+), 161 deletions(-) diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 84b9d7acb0..e9494a78be 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -20,39 +20,19 @@ * Existing-file mode and ownership are re-applied after staging so a process * umask or replacement inode never changes the target's identity. Hard-linked * targets are rejected because replacement would split their link group, and - * the target identity is revalidated inside the native conditional publication - * primitive. The staged - * bytes are synced before publication; directory fsync is intentionally not + * target identity is revalidated before the final same-directory rename. The + * staged bytes are synced before publication; directory fsync is intentionally not * promised because Windows reports EPERM for it (#4457). */ -import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import type { NativeExactFileIdentity, NativeExactUnlinkResult, NativeNoReplaceResult } from "@gajae-code/natives"; import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/utils"; const TEMP_CREATE_ATTEMPTS = 8; const DEFAULT_FILE_MODE = 0o666; -type NativeAtomicPublishBindings = { - exactReplacePath: ( - sourcePath: string, - destinationPath: string, - expectedSource: NativeExactFileIdentity, - expectedDestination: NativeExactFileIdentity, - ) => NativeExactUnlinkResult; - renameNoReplacePath: (sourcePath: string, destinationPath: string) => NativeNoReplaceResult; -}; - -let nativeAtomicPublishBindings: NativeAtomicPublishBindings | undefined; - -function getNativeAtomicPublishBindings(): NativeAtomicPublishBindings { - nativeAtomicPublishBindings ??= require("@gajae-code/natives") as NativeAtomicPublishBindings; - return nativeAtomicPublishBindings; -} - interface ExistingFileMetadata { mode: number; uid: number; @@ -62,11 +42,6 @@ interface ExistingFileMetadata { ino: number; } -type NativePublicationError = Error & { - code?: string; - publicationUncertain?: boolean; -}; - interface ResolvedPublishPath { publishPath: string; existing?: ExistingFileMetadata; @@ -123,59 +98,6 @@ function tempPathFor(dest: string): string { return path.join(path.dirname(dest), `.${path.basename(dest)}.${unique}.tmp`); } -function sha256(bytes: ArrayBuffer): string { - return crypto.createHash("sha256").update(new Uint8Array(bytes)).digest("hex"); -} - -async function captureExactFileIdentity(file: string): Promise { - try { - const [bytes, stat, parent] = await Promise.all([ - Bun.file(file).arrayBuffer(), - fs.stat(file, { bigint: true }), - fs.stat(path.dirname(file), { bigint: true }), - ]); - return { - dev: stat.dev, - ino: stat.ino, - nlink: stat.nlink, - parentDev: parent.dev, - parentIno: parent.ino, - size: stat.size, - mtimeNs: stat.mtimeNs, - directory: false, - detachOnly: false, - sha256: sha256(bytes), - }; - } catch (error) { - if (isEnoent(error)) return undefined; - throw error; - } -} - -function nativePublicationError( - operation: string, - code: string | undefined, - publicationUncertain = false, -): NativePublicationError { - const error = new Error( - publicationUncertain - ? `${operation} reached an uncertain publication state (${code ?? "unknown"}).` - : `${operation} failed (${code ?? "unknown"}).`, - ) as NativePublicationError; - if (code !== undefined) error.code = code; - if (publicationUncertain) error.publicationUncertain = true; - return error; -} - -function hasRetainedPublication(result: NativeExactUnlinkResult): boolean { - return ( - result.detachedPath !== undefined || - result.retainedSuccessorPath !== undefined || - result.retainedPlaceholderPath !== undefined || - result.retainedUnknownPath !== undefined - ); -} - function eisdir(dest: string): Error & { code: string } { const error = new Error(`EISDIR: illegal operation on a directory, write '${dest}'`) as Error & { code: string; @@ -370,51 +292,11 @@ export async function writeFileAtomically( await preserveExistingMetadata(tmp, existing); } await assertPublishTargetStillIntended(dest, publishPath, trustBoundary, existing, expectedParentRealpath); - if (existing === undefined) { - // A plain rename would allow a concurrent creator to be overwritten - // after the JavaScript identity check. Fail closed when the native - // no-replace primitive cannot establish the publication boundary. - const published = getNativeAtomicPublishBindings().renameNoReplacePath(tmp, publishPath); - if (!published.ok) { - const uncertain = published.mutationState !== "not_committed"; - if (uncertain) owned = false; - throw nativePublicationError("Atomic creation", published.code, uncertain); - } - owned = false; - } else { - // The native exchange validates both identities in the same namespace - // transaction. Do not fall back to pathname rename: validation and a - // plain rename are otherwise separable under a concurrent writer. - const expectedDestination = await captureExactFileIdentity(publishPath); - if (expectedDestination === undefined) { - throw new Error(`destination '${dest}' disappeared while staging`); - } - if ( - Number(expectedDestination.dev) !== existing.dev || - Number(expectedDestination.ino) !== existing.ino || - Number(expectedDestination.nlink ?? -1n) !== existing.nlink - ) { - throw new Error( - `destination '${dest}' was replaced while staging; refusing to overwrite a different file`, - ); - } - const expectedSource = await captureExactFileIdentity(tmp); - if (expectedSource === undefined) { - throw new Error(`staging file '${tmp}' disappeared before publication`); - } - const published = getNativeAtomicPublishBindings().exactReplacePath( - tmp, - publishPath, - expectedSource, - expectedDestination, - ); - if (!published.ok) { - const uncertain = hasRetainedPublication(published); - if (uncertain) owned = false; - throw nativePublicationError("Atomic replacement", published.code, uncertain); - } - owned = false; - } + // The staged file lives beside the destination, so rename is one + // same-directory atomic publication. A failed rename leaves the + // destination untouched and the owned staging file is cleaned below. + await fs.rename(tmp, publishPath); + owned = false; return; } catch (error) { lastError = error; @@ -429,11 +311,6 @@ export async function writeFileAtomically( throw lastError; } catch (error) { if (error instanceof FileWriteNotPublishedError) throw error; - const publicationUncertain = - error !== null && - typeof error === "object" && - "publicationUncertain" in error && - error.publicationUncertain === true; - throw new FileWriteNotPublishedError(dest, error, { destUnchanged: !publicationUncertain }); + throw new FileWriteNotPublishedError(dest, error); } } diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index 0388b45fa8..a506c1c27e 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -1,6 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; import type { PathLike, StatOptions } from "node:fs"; -import * as fsSync from "node:fs"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -9,7 +8,6 @@ import type { ClientBridge } from "@gajae-code/coding-agent/session/client-bridg import type { ToolSession } from "@gajae-code/coding-agent/tools"; import { ReadTool } from "@gajae-code/coding-agent/tools/read"; import { WriteTool } from "@gajae-code/coding-agent/tools/write"; -import * as natives from "@gajae-code/natives"; import { FileReadCache } from "../src/edit/file-read-cache"; import { writeFileAtomically } from "../src/tools/atomic-file-write"; @@ -301,7 +299,7 @@ describe("file tool atomicity and read-after-write (#4734)", () => { expect(await fs.readFile(peer, "utf8")).toBe("original\n"); }); - it("rejects a destination identity swap during publication", async () => { + it("rejects a destination identity swap detected before publication", async () => { if (process.platform === "win32") return; const dest = path.join(tmpDir, "identity-swap.ts"); const replacement = path.join(tmpDir, "identity-replacement.ts"); @@ -328,27 +326,22 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); - it("uses the native identity-bound primitive at the publication boundary", async () => { - if (process.platform === "win32") return; - const dest = path.join(tmpDir, "native-boundary.ts"); - const successor = path.join(tmpDir, "native-successor.ts"); - const displaced = path.join(tmpDir, "native-displaced.ts"); - await fs.writeFile(dest, "original\n"); - await fs.writeFile(successor, "successor\n"); - const realExactReplace = natives.exactReplacePath; - const original = spyOn(natives, "exactReplacePath").mockImplementation( - (sourcePath, destinationPath, expectedSource, expectedDestination) => { - fsSync.renameSync(destinationPath, displaced); - fsSync.renameSync(successor, destinationPath); - return realExactReplace(sourcePath, destinationPath, expectedSource, expectedDestination); - }, - ); + it("publishes through a same-directory atomic rename", async () => { + const dest = path.join(tmpDir, "plain-rename.ts"); + const renamed: string[] = []; + const realRename = fs.rename.bind(fs); + const original = spyOn(fs, "rename").mockImplementation(async (from, to) => { + renamed.push(`${String(from)} -> ${String(to)}`); + return realRename(from, to); + }); try { - await expect(writeFileAtomically(dest, "must-not-overwrite\n")).rejects.toThrow(/identity_mismatch/); - expect(await fs.readFile(dest, "utf8")).toBe("successor\n"); + await writeFileAtomically(dest, "published\n"); + expect(renamed).toHaveLength(1); + expect(path.dirname(renamed[0]!.split(" -> ")[0])).toBe(path.dirname(dest)); + expect(await fs.readFile(dest, "utf8")).toBe("published\n"); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); } finally { original.mockRestore(); - await fs.rm(displaced, { force: true }); } }); @@ -432,10 +425,7 @@ describe("file tool atomicity and read-after-write (#4734)", () => { it("cleans an owned staging file when publication fails", async () => { const dest = path.join(tmpDir, "rename-fails.ts"); await fs.writeFile(dest, "original\n"); - const original = spyOn(natives, "exactReplacePath").mockImplementation(() => ({ - ok: false, - code: "EIO", - })); + const original = spyOn(fs, "rename").mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })); try { await expect(writeFileAtomically(dest, "replacement\n")).rejects.toMatchObject({ code: "EIO" }); expect(await fs.readFile(dest, "utf8")).toBe("original\n"); @@ -448,10 +438,7 @@ describe("file tool atomicity and read-after-write (#4734)", () => { it("reports cleanup failure without claiming the staged file was removed", async () => { const dest = path.join(tmpDir, "unlink-fails.ts"); await fs.writeFile(dest, "original\n"); - const replace = spyOn(natives, "exactReplacePath").mockImplementation(() => ({ - ok: false, - code: "EIO", - })); + const rename = spyOn(fs, "rename").mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })); const realUnlink = fs.unlink.bind(fs); const unlink = spyOn(fs, "unlink").mockImplementation(async target => { if (String(target).includes(".tmp")) { @@ -463,11 +450,22 @@ describe("file tool atomicity and read-after-write (#4734)", () => { await expect(writeFileAtomically(dest, "replacement\n")).rejects.toThrow(/Failed to clean up staging file/); expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(true); } finally { - replace.mockRestore(); + rename.mockRestore(); unlink.mockRestore(); } }); + it("leaves no staging residue across three successful writes", async () => { + const destinations = ["success-one.ts", "success-two.ts", "success-three.ts"].map(name => + path.join(tmpDir, name), + ); + for (const [index, dest] of destinations.entries()) { + await writeFileAtomically(dest, `success ${index}\n`); + expect(await fs.readFile(dest, "utf8")).toBe(`success ${index}\n`); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); + } + }); + it("rejects a symlink escape from the session-scoped gjc-local root", async () => { if (process.platform === "win32") return; const sessionRoot = path.join(os.tmpdir(), "gjc-local", "atomic-trust-test"); From 9f555249af9f396531534f4875d8bbb19d0ad96c Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 13:55:16 +0000 Subject: [PATCH 09/13] fix(file-tools): close atomic publication review gaps Permission checks, bounded Windows sharing retries, rollback-capable inode-preserving fallback, and truthful publication diagnostics keep failed writes from truncating user files. ACP reads now fail closed unless the bridge explicitly reports transport unavailability, and affected CI selects the file-tool regression suites. Constraint: preserve ordinary user-file no-truncate behavior Constraint: do not bypass ACP permission decisions with host-disk fallback Confidence: high Scope-risk: medium Reversibility: straightforward Tested: focused file-tool harness, coding-agent check, CI planner tests --- docs/tools/write.md | 2 + packages/coding-agent/src/lsp/index.ts | 26 +++- .../src/tools/atomic-file-write.ts | 131 +++++++++++++++++- packages/coding-agent/src/tools/read.ts | 34 ++--- packages/coding-agent/src/tools/write.ts | 2 +- .../test/file-tools-atomicity.test.ts | 36 ++++- .../coding-agent/test/read-acp-fs.test.ts | 42 ++++++ .../test/tools/lsp-batching.test.ts | 24 +++- scripts/ci-dev-affected.test.ts | 13 ++ scripts/ci-dev-affected.ts | 5 + 10 files changed, 274 insertions(+), 41 deletions(-) diff --git a/docs/tools/write.md b/docs/tools/write.md index 744009521f..223cab6116 100644 --- a/docs/tools/write.md +++ b/docs/tools/write.md @@ -76,6 +76,8 @@ Single-shot result. - Target is any path that does not resolve as an archive selector and does not resolve as an existing-or-new SQLite selector. - Existing files are overwritten. - Parent directories are created by `writeFileAtomically()`. A failed write never truncates an existing destination to 0 bytes. +- Existing referents must be writable and are checked before publication. Hard-linked regular files are rejected rather than silently leaving aliases with stale bytes; this preserves the no-truncate guarantee instead of switching to an unsafe in-place fallback. +- On Windows, a writable file held with write sharing but without delete sharing falls back to a rollback-capable in-place update after bounded rename retries, preserving the existing inode and editability. Example: diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 6ca468632a..32d6464d8b 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -742,10 +742,14 @@ export async function writethroughNoop( dst: string, content: string, _signal?: AbortSignal, - _file?: BunFile, + file?: BunFile, _batch?: LspWritethroughBatchRequest, _getDeferred?: (dst: string) => WritethroughDeferredHandle | undefined, ): Promise { + if (file !== undefined) { + file.write(content); + return undefined; + } await writeFileAtomically(dst, content); return undefined; } @@ -903,7 +907,7 @@ async function runLspWritethrough( cwd: string, options: ResolvedWritethroughOptions, signal?: AbortSignal, - _file?: BunFile, + file?: BunFile, deferred?: { onDeferredDiagnostics: (diagnostics: FileDiagnosticsResult) => void; signal: AbortSignal; @@ -913,7 +917,7 @@ async function runLspWritethrough( const config = getConfig(cwd); const servers = getServersForFile(config, dst); if (servers.length === 0) { - return writethroughNoop(dst, content, signal, _file); + return writethroughNoop(dst, content, signal, file); } const { lspServers, customLinterServers } = splitServers(servers); @@ -921,11 +925,18 @@ async function runLspWritethrough( let publishedContent = false; const writeContent = async (value: string) => { try { - await writeFileAtomically(dst, value); + if (file !== undefined) file.write(value); + else await writeFileAtomically(dst, value); publishedContent = true; } catch (error) { - if (publishedContent && error instanceof FileWriteNotPublishedError) { - throw new FileWriteNotPublishedError(dst, error.cause, { destUnchanged: false }); + if (error instanceof FileWriteNotPublishedError) { + if (publishedContent) { + throw new FileWriteNotPublishedError(dst, error.cause, { + destUnchanged: false, + publicationState: "published", + }); + } + throw error; } throw error; } @@ -994,7 +1005,8 @@ async function runLspWritethrough( }); } }); - } catch { + } catch (error) { + if (error instanceof FileWriteNotPublishedError) throw error; if (timedOut) { formatter = undefined; diagnostics = undefined; diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index e9494a78be..7634f376ce 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -32,6 +32,10 @@ import { hasFsCode, isEacces, isEisdir, isEnoent, isFsError } from "@gajae-code/ const TEMP_CREATE_ATTEMPTS = 8; const DEFAULT_FILE_MODE = 0o666; +const WINDOWS_RENAME_BACKOFF_MS = [10, 25, 50, 100, 200] as const; +const WINDOWS_SHARING_VIOLATION_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); + +export type FileWritePublicationState = "not_published" | "published" | "unknown"; interface ExistingFileMetadata { mode: number; @@ -50,13 +54,19 @@ interface ResolvedPublishPath { export class FileWriteNotPublishedError extends Error { readonly dest: string; readonly destUnchanged: boolean; + readonly publicationState: FileWritePublicationState; override readonly cause: unknown; - constructor(dest: string, cause: unknown, options: { destUnchanged?: boolean } = {}) { + constructor( + dest: string, + cause: unknown, + options: { destUnchanged?: boolean; publicationState?: FileWritePublicationState } = {}, + ) { const destUnchanged = options.destUnchanged ?? true; super(formatFileWriteError(cause, dest, { destUnchanged })); this.name = "FileWriteNotPublishedError"; this.dest = dest; this.destUnchanged = destUnchanged; + this.publicationState = options.publicationState ?? (destUnchanged ? "not_published" : "unknown"); this.cause = cause; if (isFsError(cause)) { (this as Error & { code?: string }).code = cause.code; @@ -91,6 +101,10 @@ export interface WriteFileAtomicallyOptions { * `/gjc-local/`. */ trustBoundary?: string; + /** Platform override used by deterministic retry tests. */ + platform?: NodeJS.Platform; + /** Sleep seam used by deterministic retry tests. */ + sleep?: (delayMs: number) => Promise; } function tempPathFor(dest: string): string { @@ -217,6 +231,98 @@ async function cleanupOwnedTemp(tmp: string, cause: unknown): Promise { } } +function isWindowsSharingViolation(error: unknown): boolean { + return isFsError(error) && WINDOWS_SHARING_VIOLATION_CODES.has(error.code); +} + +async function renameIntoPlace( + from: string, + to: string, + platform: NodeJS.Platform, + sleep: (delayMs: number) => Promise, +): Promise { + try { + await fs.rename(from, to); + return; + } catch (error) { + if (platform !== "win32" || !isWindowsSharingViolation(error)) throw error; + let lastError: unknown = error; + for (const delay of WINDOWS_RENAME_BACKOFF_MS) { + await sleep(delay); + try { + await fs.rename(from, to); + return; + } catch (retryError) { + lastError = retryError; + if (!isWindowsSharingViolation(retryError)) throw retryError; + } + } + throw lastError; + } +} + +/** + * Windows permits another process to share writes while denying delete/rename. + * In that narrow case preserve the existing inode and perform a rollback-capable + * in-place replacement. Hard-linked files never enter this path: they are + * rejected before staging because a failed write cannot safely preserve every + * alias with a pathname replacement contract. + */ +async function replaceInPlaceAfterSharingViolation( + dest: string, + tmp: string, + platform: NodeJS.Platform, +): Promise { + if (platform !== "win32") throw new Error("in-place sharing fallback is Windows-only"); + const original = new Uint8Array(await Bun.file(dest).arrayBuffer()); + const replacement = new Uint8Array(await Bun.file(tmp).arrayBuffer()); + const handle = await fs.open(dest, "r+"); + let failure: unknown; + let committed = false; + try { + try { + await handle.writeFile(replacement); + await handle.sync(); + await handle.truncate(replacement.byteLength); + await handle.sync(); + committed = true; + } catch (error) { + try { + await handle.writeFile(original); + await handle.truncate(original.byteLength); + await handle.sync(); + } catch (rollbackError) { + failure = new FileWriteNotPublishedError( + dest, + new AggregateError([error, rollbackError], "In-place write rollback failed."), + { destUnchanged: false, publicationState: "unknown" }, + ); + } + if (failure === undefined) failure = error; + } + } finally { + try { + await handle.close(); + } catch (closeError) { + if (committed) { + failure = new FileWriteNotPublishedError(dest, closeError, { + destUnchanged: false, + publicationState: "published", + }); + } else if (failure === undefined) { + failure = closeError; + } + } + } + if (failure !== undefined) { + if (failure instanceof FileWriteNotPublishedError) throw failure; + throw new FileWriteNotPublishedError(dest, failure, { + destUnchanged: !committed, + publicationState: committed ? "published" : "not_published", + }); + } +} + /** * Revalidate the resolved destination immediately before publication so a * symlink retargeted while staging cannot silently repoint the write at a @@ -258,6 +364,8 @@ export async function writeFileAtomically( ): Promise { let publishPath = dest; try { + const platform = options.platform ?? process.platform; + const sleep = options.sleep ?? (async (delayMs: number): Promise => await Bun.sleep(delayMs)); const trustBoundary = options.trustBoundary ?? sessionLocalRootFor(dest); const resolved = await resolvePublishPath(dest); publishPath = resolved.publishPath; @@ -295,7 +403,26 @@ export async function writeFileAtomically( // The staged file lives beside the destination, so rename is one // same-directory atomic publication. A failed rename leaves the // destination untouched and the owned staging file is cleaned below. - await fs.rename(tmp, publishPath); + try { + await renameIntoPlace(tmp, publishPath, platform, sleep); + } catch (error) { + if (existing !== undefined && platform === "win32" && isWindowsSharingViolation(error)) { + await replaceInPlaceAfterSharingViolation(publishPath, tmp, platform); + try { + await fs.unlink(tmp); + } catch (cleanupError) { + owned = false; + throw new FileWriteNotPublishedError( + dest, + new AggregateError([error, cleanupError], "Published bytes could not be cleaned up."), + { destUnchanged: false, publicationState: "published" }, + ); + } + owned = false; + return; + } + throw error; + } owned = false; return; } catch (error) { diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index bdf7857dcd..c680239e27 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -1429,11 +1429,12 @@ interface ResolvedSqliteReadPath { * Directories return a formatted listing with modification times. */ /** - * A client-authority denial is a decision, not a transport failure: falling back to - * disk would bypass the permission the client just refused. Availability failures - * still fall back so an unreachable bridge cannot break local reads. + * Only an explicit bridge transport-unavailable marker authorizes falling back to + * the agent host's disk. Raw OS errno values, including EACCES/EPERM, are + * ambiguous at this boundary: treating them as transport failures can bypass a + * remote client's access decision by reading the path locally. */ -function isClientAuthorityDenial(error: unknown): boolean { +function isExplicitTransportUnavailable(error: unknown): boolean { const directCode = typeof error === "object" && error !== null && "code" in error ? (error as { code?: unknown }).code : undefined; const namedCode = error instanceof Error ? error.name : undefined; @@ -1445,26 +1446,11 @@ function isClientAuthorityDenial(error: unknown): boolean { ? ((error as { data?: { code?: unknown } }).data?.code ?? undefined) : undefined; const code = directCode ?? nestedCode ?? namedCode; - // OS errno codes are transport/availability failures, not an ACP permission - // decision. Treating `EPERM`/`EACCES` as denials skipped the disk fallback - // for files that already existed on disk. - if ( - code === "EPERM" || - code === "EACCES" || - code === "ENOENT" || - code === "EIO" || - code === "EBUSY" || - code === "EROFS" || - code === "EISDIR" || - code === "ENOTDIR" - ) { - return false; - } - // ACP clients surface refusals as an application error; -32001 is the reserved - // client-authority denial code and -32603 covers hosts without a dedicated code. - if (code === "permission_denied" || code === "forbidden" || code === -32001 || code === -32603) return true; - const message = error instanceof Error ? error.message : typeof error === "string" ? error : ""; - return /permission denied|not permitted|access denied|forbidden/i.test(message); + return code === "transport_unavailable" || code === "bridge_unavailable"; +} + +function isClientAuthorityDenial(error: unknown): boolean { + return !isExplicitTransportUnavailable(error); } export class ReadTool implements AgentTool { diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index 05c0576d27..cb16d4d6bb 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -767,7 +767,7 @@ export class WriteTool implements AgentTool { expect(textOf(readResult)).toContain("fromBridge"); }); - it("falls back to disk when ACP read fails with an OS EPERM errno", async () => { + it("fails closed when ACP read returns an ambiguous OS permission errno", async () => { const dest = path.join(tmpDir, "on-disk.ts"); await fs.writeFile(dest, "export const fromDisk = true;\n"); const bridge: ClientBridge = { @@ -155,11 +155,9 @@ describe("file tool atomicity and read-after-write (#4734)", () => { throw error; }, }; - const result = await new ReadTool(createSession(tmpDir, { getClientBridge: () => bridge })).execute( - "eperm-fallback", - { path: dest }, - ); - expect(textOf(result)).toContain("fromDisk"); + await expect( + new ReadTool(createSession(tmpDir, { getClientBridge: () => bridge })).execute("eperm-denied", { path: dest }), + ).rejects.toThrow(/EPERM/); }); it("does not fall back to disk for a structured ACP permission denial", async () => { @@ -205,6 +203,7 @@ describe("file tool atomicity and read-after-write (#4734)", () => { const original = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { if (String(target).includes(".tmp") && flags === "wx" && collisions < 1) { collisions += 1; + await fs.writeFile(String(target), "pre-existing collision\n"); const error = new Error("EEXIST: file already exists") as Error & { code: string }; error.code = "EEXIST"; throw error; @@ -215,7 +214,11 @@ describe("file tool atomicity and read-after-write (#4734)", () => { await writeFileAtomically(dest, "after retry\n"); expect(collisions).toBe(1); expect(await fs.readFile(dest, "utf8")).toBe("after retry\n"); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toHaveLength(1); } finally { + for (const name of await fs.readdir(tmpDir)) { + if (name.endsWith(".tmp")) await fs.rm(path.join(tmpDir, name), { force: true }); + } original.mockRestore(); } }); @@ -435,6 +438,27 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("keeps writable Windows files editable when delete-sharing blocks rename", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "windows-share.ts"); + await fs.writeFile(dest, "old\n"); + const rename = spyOn(fs, "rename").mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })); + const delays: number[] = []; + try { + await writeFileAtomically(dest, "new\n", { + platform: "win32", + sleep: async delay => { + delays.push(delay); + }, + }); + expect(delays).toEqual([10, 25, 50, 100, 200]); + expect(await fs.readFile(dest, "utf8")).toBe("new\n"); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); + } finally { + rename.mockRestore(); + } + }); + it("reports cleanup failure without claiming the staged file was removed", async () => { const dest = path.join(tmpDir, "unlink-fails.ts"); await fs.writeFile(dest, "original\n"); diff --git a/packages/coding-agent/test/read-acp-fs.test.ts b/packages/coding-agent/test/read-acp-fs.test.ts index fe093a055f..0335cf2aa3 100644 --- a/packages/coding-agent/test/read-acp-fs.test.ts +++ b/packages/coding-agent/test/read-acp-fs.test.ts @@ -105,6 +105,48 @@ describe("read tool ACP fs routing", () => { ).rejects.toThrow("Request rejected"); }); + it("treats a neutral EACCES bridge error as a denial", async () => { + const filePath = path.join(tmpDir, "ambiguous.ts"); + await fs.writeFile(filePath, "export const local = true;\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + throw Object.assign(new Error("bridge rejected request"), { code: "EACCES" }); + }, + }; + await expect( + new ReadTool(createSession(tmpDir, bridge)).execute("denied-eacces", { path: filePath }), + ).rejects.toThrow("bridge rejected request"); + }); + + it("falls back only for an explicit transport-unavailable bridge error", async () => { + const filePath = path.join(tmpDir, "transport-fallback.ts"); + await fs.writeFile(filePath, "export const local = true;\n"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + throw Object.assign(new Error("bridge transport unavailable"), { code: "transport_unavailable" }); + }, + }; + const result = await new ReadTool(createSession(tmpDir, bridge)).execute("transport-fallback", { + path: filePath, + }); + expect(textOutput(result)).toContain("export const local = true;"); + }); + + it("uses the ACP buffer for a missing path with a range and normal truncation routing", async () => { + const filePath = path.join(tmpDir, "missing-buffer.ts"); + const bridge: ClientBridge = { + capabilities: { readTextFile: true }, + readTextFile: async () => "one\ntwo\nthree\nfour\n", + }; + const result = await new ReadTool(createSession(tmpDir, bridge)).execute("missing-range", { + path: `${filePath}:2+1`, + }); + const text = textOutput(result); + expect(text).toContain("two"); + }); + it("applies requested line ranges to bridge content exactly once", async () => { const filePath = path.join(tmpDir, "range.txt"); await fs.writeFile(filePath, "disk one\ndisk two\ndisk three\n"); diff --git a/packages/coding-agent/test/tools/lsp-batching.test.ts b/packages/coding-agent/test/tools/lsp-batching.test.ts index f28314c5a2..babeaeb642 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; -import { createLspWritethrough } from "@gajae-code/coding-agent/lsp"; +import { createLspWritethrough, writethroughNoop } from "@gajae-code/coding-agent/lsp"; import * as lspConfig from "@gajae-code/coding-agent/lsp/config"; import { TempDir } from "@gajae-code/utils"; import type { ServerConfig } from "../../src/lsp/types"; @@ -68,6 +68,14 @@ describe("createLspWritethrough batching", () => { expect(await Bun.file(filePath).text()).toBe("const single = true;\n"); }); + it("honors the exported BunFile writethrough target", async () => { + const requestedPath = path.join(tempDir.path(), "requested.ts"); + const virtualTargetPath = path.join(tempDir.path(), "virtual-target.ts"); + await writethroughNoop(requestedPath, "virtual content\n", undefined, Bun.file(virtualTargetPath)); + expect(await Bun.file(virtualTargetPath).text()).toBe("virtual content\n"); + expect(await Bun.file(requestedPath).exists()).toBe(false); + }); + it("reports a later publication failure as potentially replacing an earlier write", async () => { vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); const client = { @@ -98,4 +106,18 @@ describe("createLspWritethrough batching", () => { }); expect(writes).toBeGreaterThan(1); }); + + it("keeps a no-server publication failure marked unchanged", async () => { + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([]); + const filePath = path.join(tempDir.path(), "no-server-failure.ts"); + vi.spyOn(atomicFileWrite, "writeFileAtomically").mockRejectedValue( + new FileWriteNotPublishedError(filePath, Object.assign(new Error("EIO"), { code: "EIO" })), + ); + const writethrough = createLspWritethrough(tempDir.path(), { enableFormat: false }); + await expect(writethrough(filePath, "const unchanged = true;\n")).rejects.toMatchObject({ + destUnchanged: true, + publicationState: "not_published", + }); + }); }); diff --git a/scripts/ci-dev-affected.test.ts b/scripts/ci-dev-affected.test.ts index d38b270d84..8d5e50a9f7 100644 --- a/scripts/ci-dev-affected.test.ts +++ b/scripts/ci-dev-affected.test.ts @@ -1353,6 +1353,19 @@ test("tab-worker graph changes always include install-methods and are Darwin rel expect(tasks[2]?.command).toEqual(["bun", "run", "ci:test:smoke"]); expect(keys.filter(key => key === "native-linux-x64")).toHaveLength(1); }); + test("file-tool sources schedule their ACP and publication regression suites", () => { + const expected: Record = { + "packages/coding-agent/src/tools/atomic-file-write.ts": "packages/coding-agent/test/file-tools-atomicity.test.ts", + "packages/coding-agent/src/tools/read.ts": "packages/coding-agent/test/read-acp-fs.test.ts", + "packages/coding-agent/src/tools/write.ts": "packages/coding-agent/test/write-acp-fs.test.ts", + "packages/coding-agent/src/lsp/index.ts": "packages/coding-agent/test/tools/lsp-batching.test.ts", + "packages/coding-agent/src/config/model-registry.ts": "packages/coding-agent/test/model-registry-runtime-provider.test.ts", + }; + for (const [source, testFile] of Object.entries(expected)) { + const tasks = targeted([source]); + expect(tasks.map(task => task.key)).toContain(`test:${testFile}`); + } + }); test("native path identity changes select the POSIX regression suite", () => { const tasks = targeted(["crates/pi-natives/src/path_identity.rs"]); expect(tasks.map(task => task.key)).toContain("test:packages/natives/test/path-identity-posix.test.ts"); diff --git a/scripts/ci-dev-affected.ts b/scripts/ci-dev-affected.ts index 29d8392fda..85b2f338d9 100755 --- a/scripts/ci-dev-affected.ts +++ b/scripts/ci-dev-affected.ts @@ -65,6 +65,11 @@ const NATIVE_BUILD_KEYS: ReadonlySet = new Set(["native-build", "native- // replace, direct-basename test selection and owner fallback tasks. const BEHAVIORAL_OWNER_TESTS: Readonly> = { "packages/agent/src/agent-loop.ts": ["packages/coding-agent/test/provider-safety-stop-hint.e2e.test.ts"], + "packages/coding-agent/src/tools/atomic-file-write.ts": ["packages/coding-agent/test/file-tools-atomicity.test.ts"], + "packages/coding-agent/src/tools/read.ts": ["packages/coding-agent/test/read-acp-fs.test.ts"], + "packages/coding-agent/src/tools/write.ts": ["packages/coding-agent/test/write-acp-fs.test.ts"], + "packages/coding-agent/src/lsp/index.ts": ["packages/coding-agent/test/tools/lsp-batching.test.ts"], + "packages/coding-agent/src/config/model-registry.ts": ["packages/coding-agent/test/model-registry-runtime-provider.test.ts"], "packages/ai/src/providers/anthropic.ts": [ "packages/ai/test/anthropic-truncated-toolcall.test.ts", "packages/ai/test/anthropic-stream-envelope.test.ts", From 1659c30b504665985e25b3a9e53bbab64ffd62e3 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 15:59:37 +0000 Subject: [PATCH 10/13] fix(file-tools): await BunFile writes and restore rollback bytes exactly Review at 37558500 found three ways a failed durable write was reported as success. The unawaited BunFile.write escaped its try block, so a rejection could never reach the catch and publishedContent was set before the write completed. The Windows in-place fallback wrote replacement and rollback bytes from the handle's current offset, so a partially accepted replacement left interleaved content while the caller was told the destination was unchanged. Publication stays last-writer-wins. Identity is revalidated before the rename, but rename(2) commits against the pathname, so a successor published inside that window is overwritten. That is now stated in the module contract and in docs/tools/write.md instead of being an undocumented race. Lore-id: 4e7a1c93 Constraint: no exchange-based publication -- it validates after committing and leaks debris into user directories Rejected: keep the native exact-replace primitive | 3 zero-length siblings per successful save, unbounded at 83k dirents Rejected: fsync the parent directory | Windows reports EPERM for it (#4457) Confidence: high Scope-risk: narrow Reversibility: easy Tested: rejecting BunFile write propagates; partial replacement rolls back byte-exactly; post-validation successor swap Not-tested: real Windows sharing-violation behavior on a win32 host --- docs/tools/write.md | 10 +++ packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/lsp/index.ts | 4 +- .../src/tools/atomic-file-write.ts | 40 +++++++++- .../test/file-tools-atomicity.test.ts | 74 +++++++++++++++++++ .../test/tools/lsp-batching.test.ts | 23 ++++++ 6 files changed, 147 insertions(+), 5 deletions(-) diff --git a/docs/tools/write.md b/docs/tools/write.md index 223cab6116..5670da8569 100644 --- a/docs/tools/write.md +++ b/docs/tools/write.md @@ -133,6 +133,16 @@ path: "data/app.sqlite:users:42" content: "" ``` +## Publication contract + +Plain-file writes stage to a sibling temp and publish with a same-directory `rename(2)`. A write that fails at any point before that rename leaves the destination byte-identical to its prior contents -- a failed write never truncates the target or leaves a 0-byte file -- and the staging file it created is removed. Successful writes leave no residue beside the destination. + +The staged bytes are fsynced before publication, but the parent directory is not, so publication is **not** crash-durable: a rename can be lost across a system crash. + +Overwrites are **last-writer-wins**. Destination identity is revalidated immediately before the rename, which rejects a target that was replaced or retargeted while staging, but the rename commits against the pathname. A concurrent writer that publishes a successor between that check and the rename is overwritten rather than detected. + +Destination symlinks are followed: the referent is replaced and the link is preserved. Hard-linked targets are rejected, because replacement would split the link group. + ## Side Effects - Filesystem - Creates or overwrites plain files. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f792226e4c..5a4b767ef8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -20,6 +20,7 @@ - Extension activation is now transactional. `pi.registerFlag(..., { default })` and `pi.registerProvider(...)` used to mutate the shared `ExtensionRuntime` state directly with no rollback, so a factory that threw midway was discarded while its side effects leaked: the flag default stayed readable via `getFlag`/`getFlagValues` and the provider registration stayed queued for the ModelRegistry drain in `sdk/session.ts` and `runListModelsCommand`, activating providers from an extension that never activated. Each factory invocation now stages its shared-state writes in an `ExtensionActivationScope` (stage → factory completes without throwing → commit into the shared runtime); rollback discards the staged writes so a failed extension leaves no flag default and no provider registration behind, and earlier extensions' committed state is untouched. After commit the shared runtime is authoritative for `getFlag`, so runtime-side writes (CLI flag overrides, a later extension's committed default) stay observable to retained extension API objects exactly as before the transaction (#4718). Commit itself is transactional: prior flag entries and the provider-queue length are journaled before publication, so a throw partway through commit is undone before it escapes and the scope only becomes terminal once publication fully succeeds — a failed extension leaves nothing behind even when the failure happens during publication. - A content-free Anthropic capacity overload no longer ends the turn under the default retry configuration. Anthropic can answer with its typed `overloaded_error` as a statusless stream error, and session retry already classifies that as transient, but the bare-default admission list only covered watchdog timeouts and the Codex `server_is_overloaded` event — so the turn surfaced the raw provider envelope and went idle, leaving the operator to resend or switch models by hand for a failure the provider says to retry. The admission now also accepts Anthropic's own overload code, recognized by parsing the error envelope and requiring both the outer `type` and the nested `error.type` to match exactly. Nothing else changes: the attempt must still carry no assistant text, thinking, or tool call and no conflicting transport facts (a status-bearing or otherwise typed failure keeps failing closed), overload prose alone can never authorize a replay, and the existing capped exponential backoff and `retry.enabled: false` opt-out are untouched. - File tools no longer lose a just-written path or leave a 0-byte target when a write fails (#4734). `writethroughNoop` and LSP writethrough now publish through a sibling temp + rename (`writeFileAtomically`) so a permission/IO error cannot truncate the destination; `EACCES`/`EPERM`/`EROFS` surface as an actionable `ToolError` that says the original file is unchanged. Read tries the ACP `readTextFile` bridge when disk stat misses, and treats OS errno codes such as `EPERM` as availability failures rather than client-authority denials so a file that exists on disk is still readable. Successful writes invalidate `fileReadCache`. Compaction-state now lists recent successful `write`/`edit`/`apply_patch`/`ast_edit` paths so a long-session compact does not silently drop in-flight file-tool context. This is independent of Windows directory-fsync `EPERM` (#4457) and of workflow-validation compaction (#4560). +- Hardened the #4734 atomic write path after review: LSP writethrough awaited its `BunFile` write again (an unawaited call escaped the surrounding `try`, so a rejecting write was recorded as published and surfaced as an unhandled rejection), the Windows in-place sharing fallback now writes replacement and rollback bytes at absolute position 0 (`handle.writeFile()` resumes from the handle offset, so a partially accepted replacement left interleaved bytes while the result still reported `destUnchanged: true`), and the module contract no longer claims crash atomicity it does not provide. Publication is documented as last-writer-wins in `docs/tools/write.md`: identity is revalidated before the rename, but `rename(2)` commits against the pathname. - Runtime skill discovery now scans `skills.customDirectories`. Session startup already loaded those directories through `loadSkills`, but `discoverRuntimeSkills` and `findRuntimeSkillByName` searched only the canonical project and user roots, so a configured custom skill was invocable by exact name yet absent from every `skill_discovery` search -- usable only by someone who already knew it existed. Both discovery entry points now scan the configured directories at user level (so project-scoped queries exclude them), deduplicated and tilde-expanded the same way `loadSkills` does. Naming a directory is explicit consent, so custom directories are not gated on `skills.trustUserSkills` -- matching the startup rule -- while the `skills.enabled` master switch still suppresses them. - A broker that cannot retain its own publication now names the object that withheld authority. The native layer opens `sdk`, `sdk/broker.lock`, `sdk/broker.lock/owner.json`, and `sdk/broker.json` no-follow and reports every refusal as one opaque `Retained broker publication authority is unavailable.`, so `gjc sdk` died with nothing to act on and the precondition could only be learned from the native source — a shared multi-account layout that symlinks the agent directory's `sdk` entry crashed every broker start this way. The failure is still fatal and still rolls back its publication; it now appends the first obstruction (missing entry, symlinked entry, wrong file kind, unreadable entry, or a non-fixed-width `heartbeatAt`) ahead of a bounded agent directory, so the named object survives the 512-character startup-failure reason, and stays verbatim when every precondition holds so a named condition is never invented. Each object is probed with the native's own access mode — the lock record read-only, only the published record read/write — and a file kind is only ever named through the open the native itself refuses, so a layout the native accepts is never reported as an obstruction; the published record is read through the descriptor the no-follow open already verified, never reopened by name. When rollback fails too, the aggregate message now carries the acquisition diagnostic, since the durable startup-failure marker persists only that message. - Retained broker publication probing now opens POSIX objects non-blocking, diagnoses exact-buffer malformed records, escapes control and bidi characters in persisted agent-directory diagnostics, and covers native-rejected wrong-kind objects without inventing a condition the native layer accepts. diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 32d6464d8b..2358414edd 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -747,7 +747,7 @@ export async function writethroughNoop( _getDeferred?: (dst: string) => WritethroughDeferredHandle | undefined, ): Promise { if (file !== undefined) { - file.write(content); + await file.write(content); return undefined; } await writeFileAtomically(dst, content); @@ -925,7 +925,7 @@ async function runLspWritethrough( let publishedContent = false; const writeContent = async (value: string) => { try { - if (file !== undefined) file.write(value); + if (file !== undefined) await file.write(value); else await writeFileAtomically(dst, value); publishedContent = true; } catch (error) { diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 7634f376ce..bdd745a80e 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -1,5 +1,20 @@ /** - * Crash-atomic user-file writes for the write/edit/LSP writethrough path. + * Fail-atomic user-file writes for the write/edit/LSP writethrough path. + * + * The guarantee is visibility, not crash durability: a failed write never + * publishes partial or truncated bytes, and the destination is left byte- + * identical. It is deliberately NOT crash-atomic -- the parent directory is + * never fsynced, so a rename can be lost across a system crash. + * + * Publication is last-writer-wins, not conditional. Identity is revalidated + * immediately before the rename, which rejects a destination that was replaced + * or retargeted while staging, but `rename(2)` commits against the pathname: + * a writer that publishes a successor inside the window between that check and + * the rename is overwritten. Closing that window needs an OS conditional- + * replace primitive; the exchange-based one available here was removed because + * it validated only after committing and leaked protocol debris into user + * directories on every successful write. `write` is a last-writer-wins tool by + * contract, so this is the documented behavior rather than a silent race. * * `Bun.write` truncates the destination then copies bytes. A permission or IO * failure after that truncate leaves a 0-byte target even though the tool @@ -268,6 +283,22 @@ async function renameIntoPlace( * rejected before staging because a failed write cannot safely preserve every * alias with a pathname replacement contract. */ +/** + * Write `bytes` as the file's entire leading content starting at absolute + * position 0, looping until every byte is accepted. `FileHandle.writeFile()` + * writes from the handle's current position, which makes it unsafe for a + * rollback that must reproduce the original bytes exactly. + */ +async function writeWholeFileAtPositionZero(handle: fs.FileHandle, bytes: Uint8Array): Promise { + let written = 0; + while (written < bytes.byteLength) { + const result = await handle.write(bytes, written, bytes.byteLength - written, written); + if (result.bytesWritten === 0) + throw new Error(`in-place write stalled at ${written} of ${bytes.byteLength} bytes`); + written += result.bytesWritten; + } +} + async function replaceInPlaceAfterSharingViolation( dest: string, tmp: string, @@ -281,14 +312,17 @@ async function replaceInPlaceAfterSharingViolation( let committed = false; try { try { - await handle.writeFile(replacement); + // Write at an explicit absolute position: handle.writeFile() appends from + // the handle's current offset, so a retry or rollback after a partial + // write would otherwise land mid-file and interleave bytes. + await writeWholeFileAtPositionZero(handle, replacement); await handle.sync(); await handle.truncate(replacement.byteLength); await handle.sync(); committed = true; } catch (error) { try { - await handle.writeFile(original); + await writeWholeFileAtPositionZero(handle, original); await handle.truncate(original.byteLength); await handle.sync(); } catch (rollbackError) { diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index 009c069a20..3f14378d1a 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -438,6 +438,31 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("documents last-writer-wins when a successor is published after validation", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "post-validation-swap.ts"); + await fs.writeFile(dest, "original\n"); + // Publish a different regular file at the destination pathname strictly + // between the final identity check and the committing rename. This is the + // window rename(2) cannot close; the write is expected to win. + const realRename = fs.rename.bind(fs); + const rename = spyOn(fs, "rename").mockImplementation(async (from, to) => { + if (String(to) === dest) { + await fs.writeFile(dest, "successor-from-another-writer\n"); + } + return realRename(from as PathLike, to as PathLike); + }); + try { + await writeFileAtomically(dest, "ours\n"); + // Documented contract: last writer wins, and the publication is whole -- + // never a mix of the successor and our bytes. + expect(await fs.readFile(dest, "utf8")).toBe("ours\n"); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); + } finally { + rename.mockRestore(); + } + }); + it("keeps writable Windows files editable when delete-sharing blocks rename", async () => { if (process.platform === "win32") return; const dest = path.join(tmpDir, "windows-share.ts"); @@ -459,6 +484,55 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("restores the original byte-exactly when the Windows in-place fallback fails", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "windows-rollback.ts"); + // Longer than the replacement so a rollback written at an advanced offset + // would leave trailing original bytes behind instead of restoring exactly. + const original = "original-content-that-is-longer\n"; + await fs.writeFile(dest, original); + const rename = spyOn(fs, "rename").mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })); + const realOpen = fs.open.bind(fs); + const open = spyOn(fs, "open").mockImplementation(async (target, flags, mode) => { + const handle = await realOpen(target as PathLike, flags as string, mode as number); + if (String(target) !== dest) return handle; + // Accept the replacement's first chunk, then fail. This is the real + // hazard: the handle offset has advanced, so a rollback that writes from + // the current position interleaves instead of restoring from byte 0. + const realWrite = handle.write.bind(handle); + const realWriteFile = handle.writeFile.bind(handle); + let writeCalls = 0; + const partiallyAcceptThenFail = async (bytes: Uint8Array): Promise => { + // Unpositioned write: accepting a prefix advances the handle offset, so a + // rollback that also writes unpositioned resumes mid-file. + await realWrite(bytes.subarray(0, 4)); + throw Object.assign(new Error("EIO: write failed"), { code: "EIO" }); + }; + handle.write = (async (...args: any[]) => { + writeCalls++; + if (writeCalls === 1) return partiallyAcceptThenFail(args[0] as Uint8Array); + return realWrite(...(args as [any])); + }) as typeof handle.write; + handle.writeFile = (async (...args: any[]) => { + writeCalls++; + if (writeCalls === 1) return partiallyAcceptThenFail(args[0] as Uint8Array); + return realWriteFile(...(args as [any])); + }) as typeof handle.writeFile; + return handle; + }); + try { + await expect( + writeFileAtomically(dest, "new\n", { platform: "win32", sleep: async () => {} }), + ).rejects.toMatchObject({ publicationState: "not_published", destUnchanged: true }); + // The reported state must match reality: byte-exact original, no mixing. + expect(await fs.readFile(dest, "utf8")).toBe(original); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); + } finally { + open.mockRestore(); + rename.mockRestore(); + } + }); + it("reports cleanup failure without claiming the staged file was removed", async () => { const dest = path.join(tmpDir, "unlink-fails.ts"); await fs.writeFile(dest, "original\n"); diff --git a/packages/coding-agent/test/tools/lsp-batching.test.ts b/packages/coding-agent/test/tools/lsp-batching.test.ts index babeaeb642..b2d57b63c5 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -76,6 +76,29 @@ describe("createLspWritethrough batching", () => { expect(await Bun.file(requestedPath).exists()).toBe(false); }); + it("propagates a rejecting BunFile write instead of reporting success", async () => { + const failure = Object.assign(new Error("EIO: simulated device failure"), { code: "EIO" }); + const file = { + // Rejects on a later microtask so an unawaited call cannot be caught by + // the caller's synchronous try/catch. + write: async () => { + await Bun.sleep(0); + throw failure; + }, + } as unknown as Bun.BunFile; + + await expect( + writethroughNoop(path.join(tempDir.path(), "rejecting.ts"), "content\n", undefined, file), + ).rejects.toMatchObject({ code: "EIO" }); + + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([]); + const writethrough = createLspWritethrough(tempDir.path(), { enableFormat: true, enableDiagnostics: true }); + await expect( + writethrough(path.join(tempDir.path(), "rejecting-lsp.ts"), "content\n", undefined, file), + ).rejects.toMatchObject({ code: "EIO" }); + }); + it("reports a later publication failure as potentially replacing an earlier write", async () => { vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); const client = { From 20a00cc25f99bb8f5fa5666fed1bcba621021137 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 17:37:08 +0000 Subject: [PATCH 11/13] fix(file-tools): validate trust boundary before mkdir and pin publication parent identity Exact-head review found three ways the atomic write path could act outside its stated authorization. A dangling symlink inside a session-local root resolves outside it, and parents were created before the boundary check, so an attacker-selected directory tree was materialized outside the sandbox and only then refused. The publication parent was compared by realpath string, which cannot detect a parent unlinked and replaced by a different directory at the same path. The Windows in-place fallback mutates by pathname rather than publishing a staged inode, but did not recheck destination identity, so a successor substituted during rename backoff was overwritten with no rollback source. Read's ACP fallback documentation claimed OS errno failures fall back to disk. The implementation fails closed for those, deliberately, since an errno at that boundary is ambiguous and a local read would bypass a remote client's access decision. Documentation and changelog now match the tested behavior. Lore-id: 9c4d2f18 Constraint: publication stays last-writer-wins -- no exchange primitive, no residue in user directories Rejected: create parents then validate | materializes attacker-chosen dirs before refusing Rejected: compare parent realpath strings only | identical string across a replaced directory inode Rejected: relax read fallback to match the docs | reintroduces the ACP denial bypass its tests forbid Confidence: high Scope-risk: narrow Reversibility: easy Tested: dangling-symlink escape creates nothing outside the root; parent replaced while staging is refused; fallback refuses a substituted inode Not-tested: real win32 sharing-violation behavior on a Windows host --- docs/tools/read.md | 2 +- packages/coding-agent/CHANGELOG.md | 3 +- .../src/tools/atomic-file-write.ts | 68 ++++++++++++-- .../test/file-tools-atomicity.test.ts | 93 +++++++++++++++++++ packages/natives/native/index.d.ts | 1 - 5 files changed, 158 insertions(+), 9 deletions(-) diff --git a/docs/tools/read.md b/docs/tools/read.md index 1c56cafcb0..6cec1b0a7a 100644 --- a/docs/tools/read.md +++ b/docs/tools/read.md @@ -82,7 +82,7 @@ URL selectors are parsed separately in `packages/coding-agent/src/tools/fetch.ts 6. Otherwise it treats the input as a local filesystem path. - `resolveReadPath()` expands `~`, resolves relative to session cwd, treats bare `/` as session cwd, and retries macOS screenshot/NFD/curly-quote variants. - If the path does not exist on disk and an ACP `readTextFile` bridge is present, the editor buffer is tried before suffix lookup so a just-written client buffer is not reported as missing. - - OS errno failures from the bridge (`EPERM`, `EACCES`, `ENOENT`, …) fall back to disk; structured ACP denials (`permission_denied`, `-32001`) do not. + - Bridge failures fail closed by default: only an explicit `transport_unavailable` or `bridge_unavailable` code authorizes falling back to the agent host's disk. Structured denials (`permission_denied`, `-32001`) and raw OS errno values (`EPERM`, `EACCES`, …) do **not** fall back, because an errno at this boundary is ambiguous and reading the path locally would bypass a remote client's access decision. - If the path still does not exist, `findUniqueSuffixMatch()` does a workspace glob-based unique suffix lookup (skipped for remote mounts). 7. Directories go through `#readDirectory()`. 8. Non-directories branch by content type: diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5a4b767ef8..aee66a7aff 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,7 +19,8 @@ - Coordinator event journal rows can now be pushed to one opt-in webhook (#4706). External orchestrators that cannot stay attached to `gjc_coordinator_watch_events` long-poll (a 300s `await_turn` timeout is not session death) had no push of **existing** journal rows; they can now set `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_URL` to receive each row as an authenticated POST whose body is the exact native `watch_events` record — same `seq`, same stable `id`, at-least-once so sinks dedupe on `id`. The feature is env-only and default-off (no MCP tool can set or read it), destinations are allowlisted (`https:` anywhere, `http:` loopback only, no redirects), the bearer token comes from a secret file path rather than env, an optional session-id scope restricts delivery to authorized sessions, and delivery runs through a durable per-row outbox off the journal append path with bounded attempts, exponential backoff, and a bounded request timeout — a dead sink never delays or rewrites terminal turn/session persistence. The five `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_*` variables resolve through the trusted credential environment (`$credentialEnv`, the same provenance as the crash-relay DSN) rather than raw `process.env`, so a checkout's `.env` cannot select the egress destination or the token file. `watch_events` long-poll is unchanged and remains the source of truth; `gjc coordinator doctor` reports the resolved webhook state. - Extension activation is now transactional. `pi.registerFlag(..., { default })` and `pi.registerProvider(...)` used to mutate the shared `ExtensionRuntime` state directly with no rollback, so a factory that threw midway was discarded while its side effects leaked: the flag default stayed readable via `getFlag`/`getFlagValues` and the provider registration stayed queued for the ModelRegistry drain in `sdk/session.ts` and `runListModelsCommand`, activating providers from an extension that never activated. Each factory invocation now stages its shared-state writes in an `ExtensionActivationScope` (stage → factory completes without throwing → commit into the shared runtime); rollback discards the staged writes so a failed extension leaves no flag default and no provider registration behind, and earlier extensions' committed state is untouched. After commit the shared runtime is authoritative for `getFlag`, so runtime-side writes (CLI flag overrides, a later extension's committed default) stay observable to retained extension API objects exactly as before the transaction (#4718). Commit itself is transactional: prior flag entries and the provider-queue length are journaled before publication, so a throw partway through commit is undone before it escapes and the scope only becomes terminal once publication fully succeeds — a failed extension leaves nothing behind even when the failure happens during publication. - A content-free Anthropic capacity overload no longer ends the turn under the default retry configuration. Anthropic can answer with its typed `overloaded_error` as a statusless stream error, and session retry already classifies that as transient, but the bare-default admission list only covered watchdog timeouts and the Codex `server_is_overloaded` event — so the turn surfaced the raw provider envelope and went idle, leaving the operator to resend or switch models by hand for a failure the provider says to retry. The admission now also accepts Anthropic's own overload code, recognized by parsing the error envelope and requiring both the outer `type` and the nested `error.type` to match exactly. Nothing else changes: the attempt must still carry no assistant text, thinking, or tool call and no conflicting transport facts (a status-bearing or otherwise typed failure keeps failing closed), overload prose alone can never authorize a replay, and the existing capped exponential backoff and `retry.enabled: false` opt-out are untouched. -- File tools no longer lose a just-written path or leave a 0-byte target when a write fails (#4734). `writethroughNoop` and LSP writethrough now publish through a sibling temp + rename (`writeFileAtomically`) so a permission/IO error cannot truncate the destination; `EACCES`/`EPERM`/`EROFS` surface as an actionable `ToolError` that says the original file is unchanged. Read tries the ACP `readTextFile` bridge when disk stat misses, and treats OS errno codes such as `EPERM` as availability failures rather than client-authority denials so a file that exists on disk is still readable. Successful writes invalidate `fileReadCache`. Compaction-state now lists recent successful `write`/`edit`/`apply_patch`/`ast_edit` paths so a long-session compact does not silently drop in-flight file-tool context. This is independent of Windows directory-fsync `EPERM` (#4457) and of workflow-validation compaction (#4560). +- File tools no longer lose a just-written path or leave a 0-byte target when a write fails (#4734). `writethroughNoop` and LSP writethrough now publish through a sibling temp + rename (`writeFileAtomically`) so a permission/IO error cannot truncate the destination; `EACCES`/`EPERM`/`EROFS` surface as an actionable `ToolError` that says the original file is unchanged. Read tries the ACP `readTextFile` bridge when disk stat misses, and bridge failures fail closed: only an explicit `transport_unavailable`/`bridge_unavailable` code falls back to the agent host's disk, while structured denials and raw OS errno values such as `EPERM` do not, so a local read cannot bypass a remote client's access decision. Successful writes invalidate `fileReadCache`. Compaction-state now lists recent successful `write`/`edit`/`apply_patch`/`ast_edit` paths so a long-session compact does not silently drop in-flight file-tool context. This is independent of Windows directory-fsync `EPERM` (#4457) and of workflow-validation compaction (#4560). +- Closed exact-head review findings on the #4734 atomic write path: the session-local trust boundary is now validated **before** any parent directory is created (a dangling symlink inside a trusted root resolves outside it, so creating parents first materialized an attacker-selected tree outside the sandbox before publication was refused, and the boundary is re-checked after `mkdir -p` follows existing symlinked ancestors); the publication parent is pinned by device/inode rather than realpath string, so a parent unlinked and replaced by a different directory at the same path is detected instead of published into; and the Windows in-place sharing fallback revalidates destination inode identity before mutating by pathname, refusing with `destUnchanged: true`/`not_published` when a concurrent writer substituted a successor during rename backoff. Read's ACP bridge fail-closed policy is now documented accurately: only explicit `transport_unavailable`/`bridge_unavailable` codes fall back to disk, never structured denials or raw OS errno. - Hardened the #4734 atomic write path after review: LSP writethrough awaited its `BunFile` write again (an unawaited call escaped the surrounding `try`, so a rejecting write was recorded as published and surfaced as an unhandled rejection), the Windows in-place sharing fallback now writes replacement and rollback bytes at absolute position 0 (`handle.writeFile()` resumes from the handle offset, so a partially accepted replacement left interleaved bytes while the result still reported `destUnchanged: true`), and the module contract no longer claims crash atomicity it does not provide. Publication is documented as last-writer-wins in `docs/tools/write.md`: identity is revalidated before the rename, but `rename(2)` commits against the pathname. - Runtime skill discovery now scans `skills.customDirectories`. Session startup already loaded those directories through `loadSkills`, but `discoverRuntimeSkills` and `findRuntimeSkillByName` searched only the canonical project and user roots, so a configured custom skill was invocable by exact name yet absent from every `skill_discovery` search -- usable only by someone who already knew it existed. Both discovery entry points now scan the configured directories at user level (so project-scoped queries exclude them), deduplicated and tilde-expanded the same way `loadSkills` does. Naming a directory is explicit consent, so custom directories are not gated on `skills.trustUserSkills` -- matching the startup rule -- while the `skills.enabled` master switch still suppresses them. - A broker that cannot retain its own publication now names the object that withheld authority. The native layer opens `sdk`, `sdk/broker.lock`, `sdk/broker.lock/owner.json`, and `sdk/broker.json` no-follow and reports every refusal as one opaque `Retained broker publication authority is unavailable.`, so `gjc sdk` died with nothing to act on and the precondition could only be learned from the native source — a shared multi-account layout that symlinks the agent directory's `sdk` entry crashed every broker start this way. The failure is still fatal and still rolls back its publication; it now appends the first obstruction (missing entry, symlinked entry, wrong file kind, unreadable entry, or a non-fixed-width `heartbeatAt`) ahead of a bounded agent directory, so the named object survives the 512-character startup-failure reason, and stays verbatim when every precondition holds so a named condition is never invented. Each object is probed with the native's own access mode — the lock record read-only, only the published record read/write — and a file kind is only ever named through the open the native itself refuses, so a layout the native accepts is never reported as an obstruction; the published record is read through the descriptor the no-follow open already verified, never reopened by name. When rollback fails too, the aggregate message now carries the acquisition diagnostic, since the durable startup-failure marker persists only that message. diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index bdd745a80e..40b0e00298 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -195,6 +195,12 @@ function sessionLocalRootFor(lexicalDest: string): string | undefined { * Reject publication when the resolved referent's real parent (and therefore * the referent itself) leaves the trust boundary. The boundary root is * realpathed so a symlinked `gjc-local` root cannot smuggle a write out. + * + * This must run before any directory is created. A dangling symlink inside a + * trusted root resolves to a path outside it, so creating parents first would + * let an attacker-selected directory tree be materialized outside the sandbox + * before publication is refused. Missing ancestors therefore resolve + * lexically (`realpathOrSelf` tolerates ENOENT) and are still boundary-checked. */ async function assertWithinTrustBoundary(publishPath: string, trustBoundary: string): Promise { const boundary = path.resolve(trustBoundary); @@ -205,6 +211,25 @@ async function assertWithinTrustBoundary(publishPath: string, trustBoundary: str } } +/** + * Identity of the publication parent directory. A realpath string alone cannot + * detect a parent that was unlinked and replaced by a different directory at + * the same path: the string still matches while the rename would publish into + * the replacement. Pin device and inode instead. + */ +interface ParentIdentity { + realpath: string; + dev: number; + ino: number; +} + +async function captureParentIdentity(publishPath: string): Promise { + const parent = path.dirname(publishPath); + const realpath = await realpathOrSelf(parent); + const stat = await fs.stat(parent); + return { realpath, dev: stat.dev, ino: stat.ino }; +} + /** * Rename replaces the referent without consulting its file permissions, so a * writable parent could otherwise overwrite a read-only or ACL-denied target in @@ -303,8 +328,26 @@ async function replaceInPlaceAfterSharingViolation( dest: string, tmp: string, platform: NodeJS.Platform, + expectedExisting: ExistingFileMetadata, ): Promise { if (platform !== "win32") throw new Error("in-place sharing fallback is Windows-only"); + // This fallback mutates the destination by pathname instead of publishing a + // staged inode, so it must re-establish that the pathname still names the + // file we were authorized to replace. Rename retries and their backoff give a + // concurrent writer time to substitute a different inode; overwriting that + // one in place would be an unauthorized mutation with no rollback source. + // `dest` is already the resolved referent, so lstat identity here is the same + // inode the pre-staging check authorized. + const current = (await resolvePublishPath(dest)).existing; + if (current === undefined || !sameFileIdentity(current, expectedExisting)) { + throw new FileWriteNotPublishedError( + dest, + new Error( + `destination '${dest}' was replaced before the in-place fallback; refusing to overwrite a different file`, + ), + { destUnchanged: true, publicationState: "not_published" }, + ); + } const original = new Uint8Array(await Bun.file(dest).arrayBuffer()); const replacement = new Uint8Array(await Bun.file(tmp).arrayBuffer()); const handle = await fs.open(dest, "r+"); @@ -367,14 +410,18 @@ async function assertPublishTargetStillIntended( publishPath: string, trustBoundary: string | undefined, expectedExisting: ExistingFileMetadata | undefined, - expectedParentRealpath: string, + expectedParent: ParentIdentity, ): Promise { const after = await resolvePublishPath(dest); if (after.publishPath !== publishPath) { throw new Error(`destination '${dest}' was retargeted while staging; refusing to overwrite a different file`); } - const currentParentRealpath = await realpathOrSelf(path.dirname(publishPath)); - if (currentParentRealpath !== expectedParentRealpath) { + const currentParent = await captureParentIdentity(publishPath); + if ( + currentParent.realpath !== expectedParent.realpath || + currentParent.dev !== expectedParent.dev || + currentParent.ino !== expectedParent.ino + ) { throw new Error( `destination '${dest}' parent was retargeted while staging; refusing to overwrite a different file`, ); @@ -403,11 +450,20 @@ export async function writeFileAtomically( const trustBoundary = options.trustBoundary ?? sessionLocalRootFor(dest); const resolved = await resolvePublishPath(dest); publishPath = resolved.publishPath; + // Boundary first, then create parents. A dangling symlink inside a trusted + // root resolves outside it, so creating directories before this check would + // materialize an attacker-selected tree outside the sandbox and only then + // refuse to publish. + if (trustBoundary !== undefined) { + await assertWithinTrustBoundary(publishPath, trustBoundary); + } await fs.mkdir(path.dirname(publishPath), { recursive: true }); - const expectedParentRealpath = await realpathOrSelf(path.dirname(publishPath)); + // Re-check after creation: `mkdir -p` follows existing symlinked ancestors, + // so the post-creation parent is the one publication must be bound to. if (trustBoundary !== undefined) { await assertWithinTrustBoundary(publishPath, trustBoundary); } + const expectedParent = await captureParentIdentity(publishPath); const existing = resolved.existing; if (existing !== undefined && existing.nlink > 1) { throw new Error( @@ -433,7 +489,7 @@ export async function writeFileAtomically( if (existing !== undefined) { await preserveExistingMetadata(tmp, existing); } - await assertPublishTargetStillIntended(dest, publishPath, trustBoundary, existing, expectedParentRealpath); + await assertPublishTargetStillIntended(dest, publishPath, trustBoundary, existing, expectedParent); // The staged file lives beside the destination, so rename is one // same-directory atomic publication. A failed rename leaves the // destination untouched and the owned staging file is cleaned below. @@ -441,7 +497,7 @@ export async function writeFileAtomically( await renameIntoPlace(tmp, publishPath, platform, sleep); } catch (error) { if (existing !== undefined && platform === "win32" && isWindowsSharingViolation(error)) { - await replaceInPlaceAfterSharingViolation(publishPath, tmp, platform); + await replaceInPlaceAfterSharingViolation(publishPath, tmp, platform, existing); try { await fs.unlink(tmp); } catch (cleanupError) { diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index 3f14378d1a..0003712563 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -580,6 +580,99 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("creates no directories outside the trust boundary for a dangling symlink escape", async () => { + if (process.platform === "win32") return; + const sessionRoot = path.join(os.tmpdir(), "gjc-local", "atomic-dangling-test"); + const outsideRoot = path.join(tmpDir, "outside-root"); + // The link target does not exist, and neither do its parents. Resolving it + // escapes the session root, so nothing under outsideRoot may be created. + const danglingTarget = path.join(outsideRoot, "attacker", "nested", "payload.ts"); + const link = path.join(sessionRoot, "dangling.ts"); + await fs.mkdir(sessionRoot, { recursive: true }); + await fs.symlink(danglingTarget, link); + try { + await expect(writeFileAtomically(link, "must-not-write\n")).rejects.toThrow(/outside trust boundary/); + // The pre-mkdir boundary check is what this pins: creating parents first + // would materialize an attacker-selected tree before refusing to publish. + expect(await Bun.file(danglingTarget).exists()).toBe(false); + await expect(fs.stat(outsideRoot)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + await fs.rm(sessionRoot, { recursive: true, force: true }); + } + }); + + it("refuses the Windows in-place fallback when the destination inode was replaced", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "win-substituted.ts"); + await fs.writeFile(dest, "authorized-original\n"); + const realRename = fs.rename.bind(fs); + // Every rename attempt reports a sharing violation, and a concurrent writer + // substitutes a different inode at the same pathname during the retry + // backoff -- i.e. strictly after the pre-publication identity check, so only + // the fallback's own revalidation can catch it. The in-place fallback mutates + // by pathname, so it must refuse rather than overwrite the successor. + let renameAttempts = 0; + const rename = spyOn(fs, "rename").mockImplementation(async (from, to) => { + if (String(to) === dest) { + renameAttempts++; + if (renameAttempts === 1) { + // Publish a genuinely distinct inode. Deleting and recreating in place + // is not enough: ext4 reuses the just-freed inode number, so the + // substitution would be indistinguishable from the authorized file. + const successor = path.join(tmpDir, "win-successor-source.ts"); + await fs.writeFile(successor, "successor-inode\n"); + await realRename(successor, dest); + } + throw Object.assign(new Error("EBUSY"), { code: "EBUSY" }); + } + return realRename(from as PathLike, to as PathLike); + }); + try { + await expect( + writeFileAtomically(dest, "ours\n", { platform: "win32", sleep: async () => {} }), + ).rejects.toMatchObject({ destUnchanged: true, publicationState: "not_published" }); + // The successor must survive untouched, and the report must be truthful. + expect(await fs.readFile(dest, "utf8")).toBe("successor-inode\n"); + expect((await fs.readdir(tmpDir)).filter(name => name.endsWith(".tmp"))).toEqual([]); + } finally { + rename.mockRestore(); + } + }); + + it("refuses publication when the parent directory is replaced while staging", async () => { + if (process.platform === "win32") return; + const parent = path.join(tmpDir, "volatile-parent"); + await fs.mkdir(parent, { recursive: true }); + const dest = path.join(parent, "target.ts"); + await fs.writeFile(dest, "original\n"); + // Swap the parent for a *different* directory at the same path, after the + // temp is staged and just before publication. The realpath string is + // unchanged, so only dev/ino identity detects it. The staged temp is carried + // into the replacement so publication would otherwise succeed there. + const realStat = fs.stat.bind(fs); + let swapped = false; + const stat = spyOn(fs, "stat").mockImplementation((async (target: PathLike, opts?: any) => { + const isTemp = String(target).includes(".tmp"); + if (!swapped && isTemp) { + swapped = true; + const staged = String(target); + const replacement = path.join(tmpDir, "replacement-parent"); + await fs.mkdir(replacement, { recursive: true }); + await fs.writeFile(path.join(replacement, "target.ts"), "successor\n"); + await fs.copyFile(staged, path.join(replacement, path.basename(staged))); + await fs.rm(parent, { recursive: true, force: true }); + await fs.rename(replacement, parent); + } + return realStat(target, opts); + }) as typeof fs.stat); + try { + await expect(writeFileAtomically(dest, "ours\n")).rejects.toThrow(/parent was retargeted while staging/); + expect(await fs.readFile(dest, "utf8")).toBe("successor\n"); + } finally { + stat.mockRestore(); + } + }); + it("publishes rebuilt archive bytes atomically", async () => { const archivePath = path.join(tmpDir, "archive.tar"); await fs.writeFile(archivePath, await new Bun.Archive({ "pkg/old.txt": "old\n" }).bytes()); diff --git a/packages/natives/native/index.d.ts b/packages/natives/native/index.d.ts index 16bd3978fc..5201139179 100644 --- a/packages/natives/native/index.d.ts +++ b/packages/natives/native/index.d.ts @@ -51,7 +51,6 @@ export declare class ComputerController { keypress(expectedEpoch: number | undefined | null, keys: Array): void wait(expectedEpoch: number | undefined | null, ms: number): void } - /** * Long-lived macOS appearance observer. * From a69ad7681356f949af39aa0a685e0de782093980 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 19:13:23 +0000 Subject: [PATCH 12/13] fix(file-tools): close archive and Windows race gaps Guard archive parent creation behind the atomic boundary check, bind Windows fallback writes to an identity-checked handle, and prevent affected CI from emitting missing test paths. Update archive documentation to describe reconstruction and atomic failure semantics. Constraint: never create archive parents outside the authorized boundary Constraint: never mutate a successor inode during Windows fallback Confidence: high Scope-risk: medium Reversibility: straightforward Tested: atomic file-tool suite, ACP/LSP suites, planner suite, coding-agent check --- docs/tools/write.md | 14 +-- .../src/tools/atomic-file-write.ts | 109 ++++++++++++++---- packages/coding-agent/src/tools/write.ts | 6 - .../test/file-tools-atomicity.test.ts | 50 ++++++++ scripts/ci-dev-affected.test.ts | 5 + scripts/ci-dev-affected.ts | 27 +++-- 6 files changed, 166 insertions(+), 45 deletions(-) diff --git a/docs/tools/write.md b/docs/tools/write.md index 5670da8569..28d7d498ac 100644 --- a/docs/tools/write.md +++ b/docs/tools/write.md @@ -53,9 +53,9 @@ Single-shot result. 1. `WriteTool.execute()` in `packages/coding-agent/src/tools/write.ts` strips `LINE+ID|` hashline prefixes from `content` when the session is in hashline display mode. 2. It calls `#resolveArchiveWritePath()` first. That uses `parseArchivePathCandidates()` from `packages/coding-agent/src/tools/archive-reader.ts`, checks candidate archive files on disk, and falls back to the longest matching archive suffix even when the archive file does not exist yet. 3. Archive writes call `enforcePlanModeWrite(..., { op: exists ? "update" : "create" })`, then `#writeArchiveEntry()`. - - The parent directory of the archive file is created with `fs.mkdir(..., { recursive: true })`. - - `.zip` archives are read with `fflate.unzipSync()`, the target entry is replaced in an in-memory map, and the archive is rewritten with `fflate.zipSync()` + `Bun.write()`. - - `.tar`, `.tar.gz`, and `.tgz` archives are read with `Bun.Archive`, existing entries are copied into an object map, the target entry is replaced, and `Bun.Archive.write()` rewrites the archive. + - `.zip` archives are read with `fflate.unzipSync()`, the target entry is replaced in an in-memory map, and the complete archive is reconstructed with `fflate.zipSync()` and published through the same guarded sibling-temp atomic writer as plain files. + - `.tar`, `.tar.gz`, and `.tgz` archives are read with `Bun.Archive`, existing entries are copied into an object map, the target entry is replaced, and `Bun.Archive.bytes()` reconstructs the archive before atomic publication. + - The reconstructed ZIP/TAR bytes are published through `writeFileAtomically()`, which creates parents only after trust-boundary validation. A failed reconstruction or publication leaves an existing archive byte-identical and removes the owned staging file. - `invalidateFsScanAfterWrite()` runs on the archive file path. 4. If the path is not treated as an archive, `execute()` calls `#resolveSqliteWritePath()`. That uses `parseSqlitePathCandidates()` and `isSqliteFile()` from `packages/coding-agent/src/tools/sqlite-reader.ts`. Existing non-SQLite files suppress the SQLite path interpretation. 5. SQLite writes call `enforcePlanModeWrite(..., { op: "update" })`, then `#writeSqliteRow()`. @@ -91,7 +91,7 @@ content: "hello\n" - Supported archive suffixes come from `parseArchivePathCandidates()`: `.tar`, `.tar.gz`, `.tgz`, `.zip`. - The inner path is normalized to `/`, strips empty and `.` segments, rejects `..`, and rejects directory targets ending in `/`. - Rewrites the whole archive file after replacing one entry. -- Creates the parent directory for the archive file if needed. +- Creates the parent directory only inside the guarded atomic publication path after destination trust validation. Example: @@ -146,8 +146,8 @@ Destination symlinks are followed: the referent is replaced and the link is pres ## Side Effects - Filesystem - Creates or overwrites plain files. - - Rewrites entire archive files when writing an archive entry. - - Creates parent directories for plain files and archive files. + - Reconstructs and atomically publishes entire archive files when writing an archive entry; failed publication preserves an existing archive and cleans owned staging residue. + - Creates parent directories for plain files and archive files only after the destination boundary has been validated. - Mutates existing SQLite databases; never creates a new SQLite DB. - Subprocesses / native bindings - Uses Bun SQLite bindings via `bun:sqlite`. @@ -184,7 +184,7 @@ Destination symlinks are followed: the referent is replaced and the link is pres ## Notes - Archive path detection runs before SQLite detection. A path that matches an archive selector is never treated as SQLite. - SQLite detection declines when an existing file with a `.sqlite` / `.db` suffix is present but does not have SQLite magic bytes; then the path falls back to a plain file write. -- ZIP entry content is encoded with `new TextEncoder().encode(content)` in `#writeArchiveEntry()`. Non-ZIP archive writes pass the string directly to `Bun.Archive.write()`. +- ZIP entry content is encoded with `new TextEncoder().encode(content)` in `#writeArchiveEntry()`. Non-ZIP archive entries are reconstructed with `Bun.Archive.bytes()` and both formats publish through `writeFileAtomically()`. - The prompt forbids two common anti-patterns: using `write` for routine edits that should use `edit`, and creating `*.md` / `README` files unless explicitly requested. It also forbids emojis unless requested. - Plain file writes report byte count using `cleanContent.length`, which is UTF-16 code units in JS, not an on-disk byte measurement. - `stripWriteContent()` only removes hashline prefixes when the session’s file display mode has `hashLines` enabled; otherwise content is written unchanged. diff --git a/packages/coding-agent/src/tools/atomic-file-write.ts b/packages/coding-agent/src/tools/atomic-file-write.ts index 40b0e00298..9a8cc56800 100644 --- a/packages/coding-agent/src/tools/atomic-file-write.ts +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -120,6 +120,8 @@ export interface WriteFileAtomicallyOptions { platform?: NodeJS.Platform; /** Sleep seam used by deterministic retry tests. */ sleep?: (delayMs: number) => Promise; + /** Test seam invoked after the fallback handle is opened and before mutation. */ + beforeInPlaceMutation?: () => Promise; } function tempPathFor(dest: string): string { @@ -254,6 +256,36 @@ function sameFileIdentity(left: ExistingFileMetadata, right: ExistingFileMetadat ); } +function metadataFromStat(stat: { + mode: number; + uid: number; + gid: number; + nlink: number; + dev: number; + ino: number; +}): ExistingFileMetadata { + return { + mode: stat.mode, + uid: stat.uid, + gid: stat.gid, + nlink: stat.nlink, + dev: stat.dev, + ino: stat.ino, + }; +} + +async function readWholeFileAtPositionZero(handle: fs.FileHandle): Promise { + const stat = await handle.stat(); + const bytes = new Uint8Array(stat.size); + let read = 0; + while (read < bytes.byteLength) { + const result = await handle.read(bytes, read, bytes.byteLength - read, read); + if (result.bytesRead === 0) throw new Error(`in-place read stalled at ${read} of ${bytes.byteLength} bytes`); + read += result.bytesRead; + } + return bytes; +} + async function preserveExistingMetadata(tmp: string, existing: ExistingFileMetadata): Promise { const staged = await fs.stat(tmp); if (staged.uid !== existing.uid || staged.gid !== existing.gid) { @@ -329,6 +361,7 @@ async function replaceInPlaceAfterSharingViolation( tmp: string, platform: NodeJS.Platform, expectedExisting: ExistingFileMetadata, + beforeMutation?: () => Promise, ): Promise { if (platform !== "win32") throw new Error("in-place sharing fallback is Windows-only"); // This fallback mutates the destination by pathname instead of publishing a @@ -336,44 +369,66 @@ async function replaceInPlaceAfterSharingViolation( // file we were authorized to replace. Rename retries and their backoff give a // concurrent writer time to substitute a different inode; overwriting that // one in place would be an unauthorized mutation with no rollback source. - // `dest` is already the resolved referent, so lstat identity here is the same - // inode the pre-staging check authorized. - const current = (await resolvePublishPath(dest)).existing; - if (current === undefined || !sameFileIdentity(current, expectedExisting)) { - throw new FileWriteNotPublishedError( - dest, - new Error( - `destination '${dest}' was replaced before the in-place fallback; refusing to overwrite a different file`, - ), - { destUnchanged: true, publicationState: "not_published" }, - ); - } - const original = new Uint8Array(await Bun.file(dest).arrayBuffer()); + // Open the pathname first, then bind all reads and writes to that handle. A + // pathname read followed by a later open can read one inode and mutate its + // successor. The handle identity and pathname identity are both checked + // immediately before the first mutation; a race therefore fails closed and + // never writes the successor. const replacement = new Uint8Array(await Bun.file(tmp).arrayBuffer()); const handle = await fs.open(dest, "r+"); + let original: Uint8Array | undefined; let failure: unknown; let committed = false; + let mutationStarted = false; try { try { + const opened = metadataFromStat(await handle.stat()); + if (!sameFileIdentity(opened, expectedExisting)) { + throw new FileWriteNotPublishedError( + dest, + new Error(`destination '${dest}' changed before the in-place fallback was opened`), + { destUnchanged: true, publicationState: "not_published" }, + ); + } + original = await readWholeFileAtPositionZero(handle); + if (beforeMutation) await beforeMutation(); + const current = (await resolvePublishPath(dest)).existing; + const bound = metadataFromStat(await handle.stat()); + if ( + current === undefined || + !sameFileIdentity(current, expectedExisting) || + !sameFileIdentity(bound, expectedExisting) + ) { + throw new FileWriteNotPublishedError( + dest, + new Error( + `destination '${dest}' was replaced before the in-place fallback mutation; refusing to overwrite a different file`, + ), + { destUnchanged: true, publicationState: "not_published" }, + ); + } // Write at an explicit absolute position: handle.writeFile() appends from // the handle's current offset, so a retry or rollback after a partial // write would otherwise land mid-file and interleave bytes. + mutationStarted = true; await writeWholeFileAtPositionZero(handle, replacement); await handle.sync(); await handle.truncate(replacement.byteLength); await handle.sync(); committed = true; } catch (error) { - try { - await writeWholeFileAtPositionZero(handle, original); - await handle.truncate(original.byteLength); - await handle.sync(); - } catch (rollbackError) { - failure = new FileWriteNotPublishedError( - dest, - new AggregateError([error, rollbackError], "In-place write rollback failed."), - { destUnchanged: false, publicationState: "unknown" }, - ); + if (original !== undefined && mutationStarted) { + try { + await writeWholeFileAtPositionZero(handle, original); + await handle.truncate(original.byteLength); + await handle.sync(); + } catch (rollbackError) { + failure = new FileWriteNotPublishedError( + dest, + new AggregateError([error, rollbackError], "In-place write rollback failed."), + { destUnchanged: false, publicationState: "unknown" }, + ); + } } if (failure === undefined) failure = error; } @@ -497,7 +552,13 @@ export async function writeFileAtomically( await renameIntoPlace(tmp, publishPath, platform, sleep); } catch (error) { if (existing !== undefined && platform === "win32" && isWindowsSharingViolation(error)) { - await replaceInPlaceAfterSharingViolation(publishPath, tmp, platform, existing); + await replaceInPlaceAfterSharingViolation( + publishPath, + tmp, + platform, + existing, + options.beforeInPlaceMutation, + ); try { await fs.unlink(tmp); } catch (cleanupError) { diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index cb16d4d6bb..6a4907791b 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -1,6 +1,5 @@ import { Database } from "bun:sqlite"; import * as fs from "node:fs/promises"; -import * as path from "node:path"; import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@gajae-code/agent-core"; import type { Component } from "@gajae-code/tui"; import { Text } from "@gajae-code/tui"; @@ -250,11 +249,6 @@ export class WriteTool implements AgentTool> { const isZip = resolvedArchivePath.absolutePath.toLowerCase().endsWith(".zip"); - const parentDir = path.dirname(resolvedArchivePath.absolutePath); - if (parentDir && parentDir !== ".") { - await fs.mkdir(parentDir, { recursive: true }); - } - if (isZip) { const zipEntries: Record = {}; diff --git a/packages/coding-agent/test/file-tools-atomicity.test.ts b/packages/coding-agent/test/file-tools-atomicity.test.ts index 0003712563..297a72b631 100644 --- a/packages/coding-agent/test/file-tools-atomicity.test.ts +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -601,6 +601,28 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("does not mkdir an archive path before rejecting a boundary escape", async () => { + if (process.platform === "win32") return; + const sessionRoot = path.join(os.tmpdir(), "gjc-local", "archive-boundary-test"); + const outsideRoot = path.join(tmpDir, "archive-outside-root"); + const danglingArchive = path.join(outsideRoot, "attacker", "nested", "payload.zip"); + const link = path.join(sessionRoot, "archive-link.zip"); + await fs.mkdir(sessionRoot, { recursive: true }); + await fs.symlink(danglingArchive, link); + try { + await expect( + new WriteTool(createSession(tmpDir)).execute("archive-boundary", { + path: `${link}:payload.txt`, + content: "must not publish\n", + }), + ).rejects.toThrow(/outside trust boundary/); + expect(await Bun.file(danglingArchive).exists()).toBe(false); + await expect(fs.stat(outsideRoot)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + await fs.rm(sessionRoot, { recursive: true, force: true }); + } + }); + it("refuses the Windows in-place fallback when the destination inode was replaced", async () => { if (process.platform === "win32") return; const dest = path.join(tmpDir, "win-substituted.ts"); @@ -639,6 +661,34 @@ describe("file tool atomicity and read-after-write (#4734)", () => { } }); + it("fails closed when the pathname changes after fallback validation", async () => { + if (process.platform === "win32") return; + const dest = path.join(tmpDir, "win-post-open-race.ts"); + const successor = path.join(tmpDir, "win-post-open-successor.ts"); + await fs.writeFile(dest, "authorized-original\n"); + const realRename = fs.rename.bind(fs); + const rename = spyOn(fs, "rename").mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })); + let raced = false; + try { + await expect( + writeFileAtomically(dest, "ours\n", { + platform: "win32", + sleep: async () => {}, + beforeInPlaceMutation: async () => { + if (raced) return; + raced = true; + await fs.writeFile(successor, "successor-inode\n"); + await realRename(successor, dest); + }, + }), + ).rejects.toMatchObject({ destUnchanged: true, publicationState: "not_published" }); + expect(raced).toBe(true); + expect(await fs.readFile(dest, "utf8")).toBe("successor-inode\n"); + } finally { + rename.mockRestore(); + } + }); + it("refuses publication when the parent directory is replaced while staging", async () => { if (process.platform === "win32") return; const parent = path.join(tmpDir, "volatile-parent"); diff --git a/scripts/ci-dev-affected.test.ts b/scripts/ci-dev-affected.test.ts index 8d5e50a9f7..7eb49accc9 100644 --- a/scripts/ci-dev-affected.test.ts +++ b/scripts/ci-dev-affected.test.ts @@ -1366,6 +1366,11 @@ test("tab-worker graph changes always include install-methods and are Darwin rel expect(tasks.map(task => task.key)).toContain(`test:${testFile}`); } }); + test("never emits a runnable task for a nonexistent test path", () => { + const missing = "packages/coding-agent/test/write-acp-fs-missing.test.ts"; + const tasks = planTargetedTasks([missing], targetingPackages, [...testFiles, missing], true); + expect(tasks.map(task => task.key)).not.toContain(`test:${missing}`); + }); test("native path identity changes select the POSIX regression suite", () => { const tasks = targeted(["crates/pi-natives/src/path_identity.rs"]); expect(tasks.map(task => task.key)).toContain("test:packages/natives/test/path-identity-posix.test.ts"); diff --git a/scripts/ci-dev-affected.ts b/scripts/ci-dev-affected.ts index 85b2f338d9..091af3dee9 100755 --- a/scripts/ci-dev-affected.ts +++ b/scripts/ci-dev-affected.ts @@ -1,6 +1,7 @@ #!/usr/bin/env bun import { $ } from "bun"; +import * as fsSync from "node:fs"; import * as path from "node:path"; import * as fs from "node:fs/promises"; import { selectCanaryTests } from "./ci-risk-canary-manifest"; @@ -287,8 +288,8 @@ async function resolvePlannedTasks(paths: readonly string[]): Promise { const normalizedPaths = normalizeChangedPaths(paths); const packages = await getWorkspacePackages(); const legacy = resolvePlanMode() === "pr" - ? planTargetedTasks(normalizedPaths, packages, await gatherTestFiles()) - : planTasks(normalizedPaths, packages); + ? planTargetedTasks(normalizedPaths, packages, await gatherTestFiles(), true) + : planTasks(normalizedPaths, packages, true); if (normalizedPaths.length > 0 && normalizedPaths.every(isDocOrChangelogPath)) return legacy; return appendBuildTasks(legacy, normalizedPaths, packages, await loadBuildInventory()); } @@ -735,7 +736,11 @@ function readStringMap(value: unknown): Record | undefined { return Object.fromEntries(entries); } -export function planTasks(paths: readonly string[], packages: readonly WorkspacePackage[]): Task[] { +export function planTasks( + paths: readonly string[], + packages: readonly WorkspacePackage[], + validateTestPaths = false, +): Task[] { const tasks = new Map(); // Mirror of the docs-index gate in planTargetedTasks: docs/ is the source the // embedded index is generated from, so either side changing must run its gate. @@ -841,7 +846,12 @@ export function planTasks(paths: readonly string[], packages: readonly Workspace // Native builds are added once (native-linux-x64) only when a planned task needs // the addon at runtime; the dedicated job restores it from cache when no native // source changed, so PRs never rebuild native per shard. -export function planTargetedTasks(paths: readonly string[], packages: readonly WorkspacePackage[], testFiles: readonly string[]): Task[] { +export function planTargetedTasks( + paths: readonly string[], + packages: readonly WorkspacePackage[], + testFiles: readonly string[], + validateTestPaths = false, +): Task[] { const tasks = new Map(); const relevant = paths.filter(changedPath => !isDocOrChangelogPath(changedPath)); // A docs edit is cheap, but it is not free: docs/ is the source the embedded docs @@ -885,7 +895,7 @@ export function planTargetedTasks(paths: readonly string[], packages: readonly W add(tasks, "rust-check", "Rust check", ["bun", "run", "check:rs"]); add(tasks, "rust-test", "Rust tests", ["bun", "run", "test:rs"]); for (const testFile of behavioralTestsFor(changedPath)) { - addTestFileTask(tasks, testFile); + addTestFileTask(tasks, testFile, validateTestPaths); } continue; } @@ -916,10 +926,10 @@ export function planTargetedTasks(paths: readonly string[], packages: readonly W const mappedTests = mappedTestsFor(changedPath, packages, testFiles); for (const testFile of mappedTests) { - addTestFileTask(tasks, testFile); + addTestFileTask(tasks, testFile, validateTestPaths); } for (const testFile of behavioralTestsFor(changedPath)) { - addTestFileTask(tasks, testFile); + addTestFileTask(tasks, testFile, validateTestPaths); } if (isCodingAgentShardOneCoveragePath(changedPath)) { addCodingAgentTestShard(tasks, 1); @@ -978,7 +988,8 @@ export function planTargetedTasks(paths: readonly string[], packages: readonly W // Add a task that runs exactly one test file. Keyed as `test:` // so the matrix shard name stays small and directly traceable to the file. -function addTestFileTask(tasks: Map, testFile: string): void { +function addTestFileTask(tasks: Map, testFile: string, requireExisting = false): void { + if (requireExisting && !fsSync.existsSync(path.join(repoRoot, testFile))) return; add(tasks, `test:${testFile}`, `Test ${testFile}`, ["bun", "test", testFile]); } From e5ff519214fbdd3006c2661e7210ab34dee10e61 Mon Sep 17 00:00:00 2001 From: Yeachan Heo Date: Thu, 20 Aug 2026 19:57:08 +0000 Subject: [PATCH 13/13] test(ci): remove global shard-order assumption Verify provider safety-stop E2E inclusion exactly once and derive its shard from the harness index contract instead of pinning an order-sensitive global shard number. --- scripts/run-bun-test-files.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/run-bun-test-files.test.ts b/scripts/run-bun-test-files.test.ts index e10507db72..e9416d2394 100644 --- a/scripts/run-bun-test-files.test.ts +++ b/scripts/run-bun-test-files.test.ts @@ -70,7 +70,12 @@ describe("fresh-process test harness contracts", () => { const assignedShards = Array.from({ length: 8 }, (_, index) => index + 1).filter(shard => selectShard(files, { index: shard, total: 8 }).includes(regression), ); - expect(assignedShards).toEqual([8]); + expect(assignedShards).toHaveLength(1); + const regressionIndex = files.indexOf(regression); + expect(regressionIndex).toBeGreaterThanOrEqual(0); + const expectedShard = (regressionIndex % 8) + 1; + expect(assignedShards).toEqual([expectedShard]); + expect(selectShard(files, { index: expectedShard, total: 8 })).toContain(regression); }); test("keeps Bun shard assignment deterministic", () => {