diff --git a/.changeset/fix-atomic-write-symlink.md b/.changeset/fix-atomic-write-symlink.md new file mode 100644 index 000000000..001cc9acd --- /dev/null +++ b/.changeset/fix-atomic-write-symlink.md @@ -0,0 +1,7 @@ +--- +"@moonshot-ai/agent-core": patch +--- + +fix: preserve symlinks in atomicWrite to prevent config.toml symlink breakage + +When config.toml is a symlink (e.g., pointing to iCloud), atomicWrite's rename() was replacing the symlink itself instead of writing through it. This fix resolves symlinks before rename, preserving the symlink. diff --git a/packages/agent-core/src/config/toml.ts b/packages/agent-core/src/config/toml.ts index 172e97cfc..e0af1cbd0 100644 --- a/packages/agent-core/src/config/toml.ts +++ b/packages/agent-core/src/config/toml.ts @@ -450,7 +450,7 @@ export async function writeConfigFile(filePath: string, config: KimiConfig): Pro // stripEnvModelConfig / the getConfig -> setConfig round-trip). const validated = validateConfig(stripEnvModelConfig(config)); await mkdir(dirname(filePath), { recursive: true, mode: 0o700 }); - await atomicWrite(filePath, `${stringifyToml(configToTomlData(validated))}\n`); + await atomicWrite(filePath, `${stringifyToml(configToTomlData(validated))}\n`, { followSymlinks: true }); } export function configToTomlData(config: KimiConfig): Record { diff --git a/packages/agent-core/src/utils/fs.ts b/packages/agent-core/src/utils/fs.ts index 7d4566aa1..ae8d9bab0 100644 --- a/packages/agent-core/src/utils/fs.ts +++ b/packages/agent-core/src/utils/fs.ts @@ -17,7 +17,7 @@ import { randomBytes } from 'node:crypto'; import { closeSync, fsyncSync, openSync } from 'node:fs'; import * as nodeFs from 'node:fs'; -import { open, rename, unlink } from 'node:fs/promises'; +import { open, rename, unlink, lstat, realpath } from 'node:fs/promises'; import { dirname } from 'pathe'; /** @@ -62,12 +62,30 @@ export function syncDirSync(dirPath: string): void { * failure *after* the rename (i.e. in the parent-directory fsync) is * surfaced to the caller — the content is already in place, but * durability is not guaranteed. + * + * @param options.followSymlinks — if true and `filePath` is a symlink, + * resolve to the real path before rename, preserving the symlink. + * Default false (replaces the symlink itself). Opt-in because following + * symlinks can break path-traversal boundaries in guarded stores. */ export async function writeFileAtomicDurable( filePath: string, content: string | Uint8Array, + options?: { followSymlinks?: boolean }, ): Promise { - const tmpPath = filePath + '.tmp'; + let writeTarget = filePath; + if (options?.followSymlinks) { + try { + const stat = await lstat(filePath); + if (stat.isSymbolicLink()) { + writeTarget = await realpath(filePath); + } + } catch { + // lstat/realpath failure — fall back to original path. + } + } + + const tmpPath = writeTarget + '.tmp'; let renamed = false; try { const fh = await open(tmpPath, 'w'); @@ -80,15 +98,15 @@ export async function writeFileAtomicDurable( // Windows pre-unlink for MoveFileEx parity. if (process.platform === 'win32') { try { - await unlink(filePath); + await unlink(writeTarget); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code !== 'ENOENT') throw error; } } - await rename(tmpPath, filePath); + await rename(tmpPath, writeTarget); renamed = true; - await syncDir(dirname(filePath)); + await syncDir(dirname(writeTarget)); } finally { if (!renamed) { // Best-effort cleanup of the `.tmp` file if we never got to the @@ -143,16 +161,33 @@ function syncFd(fd: number): Promise { * * @param filePath — absolute or relative path to the target file. * @param content — string or binary payload to write. + * @param options.followSymlinks — if true and `filePath` is a symlink, + * resolve to the real path before rename, preserving the symlink. + * Default false (replaces the symlink itself). Opt-in because following + * symlinks can break path-traversal boundaries in guarded stores. * @param _syncOverride — test seam: override the fsync implementation for * fault injection. Production callers must never supply this. */ export async function atomicWrite( filePath: string, content: string | Uint8Array, + options?: { followSymlinks?: boolean }, _syncOverride?: (fd: number) => Promise, ): Promise { + let writeTarget = filePath; + if (options?.followSymlinks) { + try { + const stat = await lstat(filePath); + if (stat.isSymbolicLink()) { + writeTarget = await realpath(filePath); + } + } catch { + // lstat/realpath failure — fall back to original path. + } + } + const hex = randomBytes(4).toString('hex'); - const tmpPath = `${filePath}.tmp.${process.pid}.${hex}`; + const tmpPath = `${writeTarget}.tmp.${process.pid}.${hex}`; let renamed = false; try { const fh = await open(tmpPath, 'w'); @@ -167,13 +202,13 @@ export async function atomicWrite( // before the rename turns this into the POSIX-style "replace" case. if (process.platform === 'win32') { try { - await unlink(filePath); + await unlink(writeTarget); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code !== 'ENOENT') throw error; } } - await rename(tmpPath, filePath); + await rename(tmpPath, writeTarget); renamed = true; } finally { if (!renamed) { diff --git a/packages/agent-core/test/utils/atomic-write.test.ts b/packages/agent-core/test/utils/atomic-write.test.ts new file mode 100644 index 000000000..d60ed9bfd --- /dev/null +++ b/packages/agent-core/test/utils/atomic-write.test.ts @@ -0,0 +1,159 @@ +/** + * Tests for atomicWrite — specifically symlink preservation. + * + * The core invariant: when followSymlinks is true and `filePath` is a + * symlink, atomicWrite should preserve the symlink (write to the resolved + * target), not replace it. Default behavior (followSymlinks false) replaces + * the symlink itself. + */ + +import { lstat, mkdir, readFile, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'pathe'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { atomicWrite } from '../../src/utils/fs'; + +let rootDir: string; + +beforeEach(async () => { + rootDir = join( + tmpdir(), + `kimi-atomic-write-${Date.now()}-${Math.random().toString(36).slice(2)}`, + ); + await mkdir(rootDir, { recursive: true }); +}); + +afterEach(async () => { + await rm(rootDir, { recursive: true, force: true }); +}); + +describe('atomicWrite', () => { + it('writes content to a regular file', async () => { + const filePath = join(rootDir, 'regular.txt'); + await atomicWrite(filePath, 'hello world'); + + const content = await readFile(filePath, 'utf-8'); + expect(content).toBe('hello world'); + }); + + it('overwrites existing regular file', async () => { + const filePath = join(rootDir, 'existing.txt'); + await writeFile(filePath, 'old content'); + + await atomicWrite(filePath, 'new content'); + + const content = await readFile(filePath, 'utf-8'); + expect(content).toBe('new content'); + }); + + it('default behavior: replaces symlink with regular file', async () => { + const targetDir = join(rootDir, 'target'); + await mkdir(targetDir); + const realFile = join(targetDir, 'config.toml'); + await writeFile(realFile, 'original'); + + const symlinkPath = join(rootDir, 'config.toml'); + await symlink(realFile, symlinkPath); + + // Without followSymlinks, symlink is replaced + await atomicWrite(symlinkPath, 'new content'); + + const stat = await lstat(symlinkPath); + expect(stat.isSymbolicLink()).toBe(false); + + const content = await readFile(symlinkPath, 'utf-8'); + expect(content).toBe('new content'); + + // Original target unchanged + const originalContent = await readFile(realFile, 'utf-8'); + expect(originalContent).toBe('original'); + }); + + it('followSymlinks: preserves symlink and writes to target', async () => { + const targetDir = join(rootDir, 'real-target'); + await mkdir(targetDir); + const realFile = join(targetDir, 'config.toml'); + await writeFile(realFile, 'original content'); + + const symlinkPath = join(rootDir, 'config.toml'); + await symlink(realFile, symlinkPath); + + // Verify symlink exists + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); + + // Write with followSymlinks + await atomicWrite(symlinkPath, 'updated content', { followSymlinks: true }); + + // Symlink should still exist + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); + + // Real file should have new content + expect(await readFile(realFile, 'utf-8')).toBe('updated content'); + + // Reading via symlink should also show new content + expect(await readFile(symlinkPath, 'utf-8')).toBe('updated content'); + }); + + it('followSymlinks: replaces broken symlink when target does not exist', async () => { + const targetDir = join(rootDir, 'target-dir'); + await mkdir(targetDir); + const realFile = join(targetDir, 'new-file.toml'); + const symlinkPath = join(rootDir, 'link.toml'); + await symlink(realFile, symlinkPath); + + // Verify it's a broken symlink + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); + + // realpath fails on broken symlink, falls back to original path + await atomicWrite(symlinkPath, 'new content', { followSymlinks: true }); + + // Symlink is replaced by regular file (fallback behavior) + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(false); + + expect(await readFile(symlinkPath, 'utf-8')).toBe('new content'); + }); + + it('handles non-existent file (no symlink)', async () => { + const filePath = join(rootDir, 'new-file.txt'); + await atomicWrite(filePath, 'brand new'); + + const content = await readFile(filePath, 'utf-8'); + expect(content).toBe('brand new'); + }); + + it('followSymlinks: preserves symlink through multiple writes', async () => { + const targetDir = join(rootDir, 'target'); + await mkdir(targetDir); + const realFile = join(targetDir, 'config.toml'); + await writeFile(realFile, 'v1'); + + const symlinkPath = join(rootDir, 'config.toml'); + await symlink(realFile, symlinkPath); + + await atomicWrite(symlinkPath, 'v2', { followSymlinks: true }); + await atomicWrite(symlinkPath, 'v3', { followSymlinks: true }); + await atomicWrite(symlinkPath, 'v4', { followSymlinks: true }); + + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); + expect(await readFile(realFile, 'utf-8')).toBe('v4'); + }); + + it('followSymlinks: handles circular symlink gracefully', async () => { + // Create circular symlinks: a -> b -> a + const linkA = join(rootDir, 'link-a.toml'); + const linkB = join(rootDir, 'link-b.toml'); + await symlink(linkB, linkA); + await symlink(linkA, linkB); + + // realpath should fail on circular symlink, fall back to original path + await expect( + atomicWrite(linkA, 'content', { followSymlinks: true }), + ).resolves.not.toThrow(); + + // Fallback: symlink is replaced by regular file + expect((await lstat(linkA)).isSymbolicLink()).toBe(false); + expect(await readFile(linkA, 'utf-8')).toBe('content'); + }); +});