diff --git a/docs/tools/read.md b/docs/tools/read.md index 5d0da01f22..6cec1b0a7a 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. + - 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: - image metadata / inline image diff --git a/docs/tools/write.md b/docs/tools/write.md index 18b5aa97b0..28d7d498ac 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 @@ -52,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()`. @@ -66,15 +67,17 @@ 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. +- 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: @@ -88,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: @@ -130,11 +133,21 @@ 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. - - Rewrites entire archive files when writing an archive entry. - - Creates parent directories for archive files only. + - 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`. @@ -171,7 +184,7 @@ content: "" ## 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/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 71965e8251..aee66a7aff 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,9 @@ - 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 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. - 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..2358414edd 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 { 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"; @@ -745,11 +746,11 @@ export async function writethroughNoop( _batch?: LspWritethroughBatchRequest, _getDeferred?: (dst: string) => WritethroughDeferredHandle | undefined, ): Promise { - if (file) { + if (file !== undefined) { await file.write(content); - } else { - await Bun.write(dst, content); + return undefined; } + await writeFileAtomically(dst, content); return undefined; } @@ -921,7 +922,25 @@ async function runLspWritethrough( const { lspServers, customLinterServers } = splitServers(servers); let finalContent = content; - const writeContent = async (value: string) => (file ? file.write(value) : Bun.write(dst, value)); + let publishedContent = false; + const writeContent = async (value: string) => { + try { + if (file !== undefined) await file.write(value); + else await writeFileAtomically(dst, value); + publishedContent = true; + } catch (error) { + if (error instanceof FileWriteNotPublishedError) { + if (publishedContent) { + throw new FileWriteNotPublishedError(dst, error.cause, { + destUnchanged: false, + publicationState: "published", + }); + } + throw error; + } + throw error; + } + }; const getWritePromise = once(() => writeContent(finalContent)); const useCustomFormatter = enableFormat && customLinterServers.length > 0; @@ -986,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/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 c3722d5a09..5785548774 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -490,6 +490,74 @@ 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 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"); + addPath(directPath); + + 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) { + 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[] { + 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 paths = collectFileMutationPaths(block.name, block.arguments).filter(path => path.length > 0); + if (paths.length > 0) callsById.set(block.id, paths); + } + } + 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 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; } /** Escape XML-ish metacharacters and flatten newlines so state text cannot break compaction prompt framing. */ @@ -11867,6 +11935,7 @@ export class AgentSession { activeSkills: [], queuedMessages: false, lastAssistantStopReason: undefined, + recentFileMutations: [], }; try { const goalState = this.getGoalModeState(); @@ -11921,6 +11990,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 +12019,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..9a8cc56800 --- /dev/null +++ b/packages/coding-agent/src/tools/atomic-file-write.ts @@ -0,0 +1,594 @@ +/** + * 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 + * 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. + * + * Destination symlinks are followed: the referent is replaced, the link stays. + * 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 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 + * 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 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"; + +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; + uid: number; + gid: number; + nlink: number; + dev: number; + ino: number; +} + +interface ResolvedPublishPath { + publishPath: string; + existing?: ExistingFileMetadata; +} + +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; 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; + } + } +} + +export function isFileWritePermissionError(error: unknown): boolean { + return isEacces(error) || hasFsCode(error, "EPERM") || hasFsCode(error, "EROFS"); +} + +export function formatFileWriteError(error: unknown, dest: string, options: { destUnchanged?: boolean } = {}): string { + if (error instanceof FileWriteNotPublishedError && options.destUnchanged !== false) return error.message; + if (isEisdir(error)) { + return `Cannot write '${dest}': path is a directory.`; + } + if (isFileWritePermissionError(error)) { + const code = isFsError(error) ? error.code : "EPERM"; + 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); +} + +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; + /** Platform override used by deterministic retry tests. */ + 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 { + 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`); +} + +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; +} + +async function resolvePublishPath(dest: string, depth = 0): Promise { + if (depth > 40) { + throw new Error(`ELOOP: too many symbolic links, write '${dest}'`); + } + try { + 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); + } + 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 }; + } +} + +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 (error) { + if (!isEnoent(error)) throw error; + 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. + * + * 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); + 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}'`); + } +} + +/** + * 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 + * 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(); +} + +function sameFileIdentity(left: ExistingFileMetadata, right: ExistingFileMetadata): boolean { + 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 + ); +} + +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) { + 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}'.`); + } +} + +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. + */ +/** + * 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, + 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 + // 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. + // 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) { + 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; + } + } 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 + * different file. + */ +async function assertPublishTargetStillIntended( + dest: string, + publishPath: string, + trustBoundary: string | undefined, + expectedExisting: ExistingFileMetadata | undefined, + 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 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`, + ); + } + 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); + } +} + +export async function writeFileAtomically( + dest: string, + content: string | Uint8Array, + options: WriteFileAtomicallyOptions = {}, +): 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; + // 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 }); + // 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( + `Cannot atomically replace hard-linked file '${dest}': replacement would split its link group.`, + ); + } + if (existing !== undefined) { + await assertExistingTargetWritable(publishPath); + } + let lastError: unknown; + for (let attempt = 0; attempt < TEMP_CREATE_ATTEMPTS; attempt++) { + const tmp = tempPathFor(publishPath); + let owned = false; + try { + const handle = await fs.open(tmp, "wx", existing?.mode ?? DEFAULT_FILE_MODE); + owned = true; + try { + await handle.writeFile(content); + await handle.sync(); + } finally { + await handle.close(); + } + if (existing !== undefined) { + await preserveExistingMetadata(tmp, existing); + } + 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. + try { + await renameIntoPlace(tmp, publishPath, platform, sleep); + } catch (error) { + if (existing !== undefined && platform === "win32" && isWindowsSharingViolation(error)) { + await replaceInPlaceAfterSharingViolation( + publishPath, + tmp, + platform, + existing, + options.beforeInPlaceMutation, + ); + 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) { + lastError = error; + // 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 cleanupOwnedTemp(tmp, error); + 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 9bb2cd131e..c680239e27 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -1429,18 +1429,28 @@ 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 { - const code = +function isExplicitTransportUnavailable(error: unknown): boolean { + const directCode = typeof error === "object" && error !== null && "code" in error ? (error as { code?: unknown }).code : undefined; - // 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); + 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; + return code === "transport_unavailable" || code === "bridge_unavailable"; +} + +function isClientAuthorityDenial(error: unknown): boolean { + return !isExplicitTransportUnavailable(error); } export class ReadTool implements AgentTool { @@ -2395,6 +2405,53 @@ 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; + 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 }, + 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; @@ -2402,10 +2459,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; @@ -2416,7 +2480,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; } } @@ -2663,6 +2728,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..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"; @@ -17,6 +16,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, writeFileAtomically } from "./atomic-file-write"; import { assertEditableFile } from "./auto-generated-guard"; import { type ConflictEntry, @@ -249,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 = {}; @@ -275,9 +270,9 @@ export class WriteTool implements AgentTool = {}; @@ -304,9 +299,11 @@ 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("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("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("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"); 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..297a72b631 --- /dev/null +++ b/packages/coding-agent/test/file-tools-atomicity.test.ts @@ -0,0 +1,770 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import type { PathLike, StatOptions } from "node:fs"; +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 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; + } + return realOpen(target, flags); + }); + 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 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; + } + return realOpen(target, flags); + }); + 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 (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); + 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("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 = { + capabilities: { readTextFile: true }, + readTextFile: async () => { + const error = new Error("EPERM: Operation not permitted") as Error & { code: string }; + error.code = "EPERM"; + throw error; + }, + }; + 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 () => { + 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(); + }); + + 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; + 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; + } + 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"); + 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(); + } + }); + + 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("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("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"); + 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 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"); + 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("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 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(); + } + }); + + 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("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"); + 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 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"); + expect((await fs.readdir(path.dirname(dest))).some(name => name.endsWith(".tmp"))).toBe(false); + } finally { + original.mockRestore(); + } + }); + + 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"); + 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("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"); + 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")) { + 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 { + 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"); + 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("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("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"); + 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("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"); + 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()); + 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"); + 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/); + }); +}); diff --git a/packages/coding-agent/test/read-acp-fs.test.ts b/packages/coding-agent/test/read-acp-fs.test.ts index bd3f2b861f..0335cf2aa3 100644 --- a/packages/coding-agent/test/read-acp-fs.test.ts +++ b/packages/coding-agent/test/read-acp-fs.test.ts @@ -88,6 +88,65 @@ 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("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 da4681aac2..b2d57b63c5 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -1,8 +1,11 @@ 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"; +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,80 @@ describe("createLspWritethrough batching", () => { expect(loadConfigSpy).toHaveBeenCalledTimes(1); 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("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 = { + 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); + }); + + 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/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. * diff --git a/scripts/ci-dev-affected.test.ts b/scripts/ci-dev-affected.test.ts index d38b270d84..7eb49accc9 100644 --- a/scripts/ci-dev-affected.test.ts +++ b/scripts/ci-dev-affected.test.ts @@ -1353,6 +1353,24 @@ 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("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 29d8392fda..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"; @@ -65,6 +66,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", @@ -282,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()); } @@ -730,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. @@ -836,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 @@ -880,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; } @@ -911,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); @@ -973,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]); } 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", () => {