From 4b7bd393200f24a518ec28becb66817fdb00336b Mon Sep 17 00:00:00 2001 From: hkkin <17541011217@163.com> Date: Mon, 15 Jun 2026 02:39:32 +0800 Subject: [PATCH 1/3] fix: preserve symlinks in atomicWrite by resolving to real path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When config.toml is a symlink (e.g., pointing to iCloud), atomicWrite's rename() was replacing the symlink itself instead of writing through it. Root cause: fs.rename() replaces the directory entry, not the target. When the target is a symlink, the symlink gets overwritten. Fix: Before rename, resolve symlinks with lstat + realpath. The tmp file is created in the real path's parent directory, and rename targets the real path, preserving the symlink. Fallback behavior unchanged: if lstat or realpath fails (broken symlink, non-existent file), fall back to original path — same behavior as before. Fixes: symlinked config.toml gets replaced with regular file after adding/removing providers. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/agent-core/src/utils/fs.ts | 25 +++- .../test/utils/atomic-write.test.ts | 137 ++++++++++++++++++ 2 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 packages/agent-core/test/utils/atomic-write.test.ts diff --git a/packages/agent-core/src/utils/fs.ts b/packages/agent-core/src/utils/fs.ts index 7d4566aa1..d91f38837 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'; /** @@ -141,6 +141,9 @@ function syncFd(fd: number): Promise { * Atomically write `content` to `filePath`. If the target already exists * it is replaced; if it does not exist it is created. * + * If `filePath` is a symlink, the write targets the resolved real path so + * the symlink itself is preserved (not replaced by the rename). + * * @param filePath — absolute or relative path to the target file. * @param content — string or binary payload to write. * @param _syncOverride — test seam: override the fsync implementation for @@ -151,8 +154,22 @@ export async function atomicWrite( content: string | Uint8Array, _syncOverride?: (fd: number) => Promise, ): Promise { + // Resolve symlinks so rename() doesn't replace the symlink itself. + // If lstat or realpath fails (file doesn't exist, broken symlink, etc.), + // fall back to the original path — same behavior as before. + let writeTarget = filePath; + try { + const stat = await lstat(filePath); + if (stat.isSymbolicLink()) { + writeTarget = await realpath(filePath); + } + } catch { + // ENOENT = file doesn't exist yet; realpath failure = broken symlink. + // Fall back to original path — rename will create/replace as before. + } + 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 +184,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..45d76ff84 --- /dev/null +++ b/packages/agent-core/test/utils/atomic-write.test.ts @@ -0,0 +1,137 @@ +/** + * Tests for atomicWrite — specifically symlink preservation. + * + * The core invariant: if `filePath` is a symlink, atomicWrite should + * preserve the symlink (write to the resolved target), not replace it. + */ + +import { mkdir, readFile, rm, stat, 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('preserves symlink and writes to target', async () => { + // Create a real file in a subdirectory (simulating iCloud target) + const targetDir = join(rootDir, 'real-target'); + await mkdir(targetDir); + const realFile = join(targetDir, 'config.toml'); + await writeFile(realFile, 'original content'); + + // Create a symlink to the real file + const symlinkPath = join(rootDir, 'config.toml'); + await symlink(realFile, symlinkPath); + + // Verify symlink exists and points to real file + const symlinkStat = await stat(symlinkPath, { bigint: false }); + const lstatResult = await (await import('node:fs/promises')).lstat(symlinkPath); + expect(lstatResult.isSymbolicLink()).toBe(true); + + // Write via atomicWrite using the symlink path + await atomicWrite(symlinkPath, 'updated content'); + + // Symlink should still exist + const afterLstat = await (await import('node:fs/promises')).lstat(symlinkPath); + expect(afterLstat.isSymbolicLink()).toBe(true); + + // Real file should have new content + const realContent = await readFile(realFile, 'utf-8'); + expect(realContent).toBe('updated content'); + + // Reading via symlink should also show new content + const symlinkContent = await readFile(symlinkPath, 'utf-8'); + expect(symlinkContent).toBe('updated content'); + }); + + it('replaces broken symlink when target does not exist', async () => { + // Create symlink to non-existent file (broken symlink) + 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 + const lstatBefore = await (await import('node:fs/promises')).lstat(symlinkPath); + expect(lstatBefore.isSymbolicLink()).toBe(true); + + // realpath fails on broken symlink, falls back to original path + // which replaces the symlink with a regular file + await atomicWrite(symlinkPath, 'new content'); + + // Symlink is replaced by regular file (fallback behavior) + const lstatAfter = await (await import('node:fs/promises')).lstat(symlinkPath); + expect(lstatAfter.isSymbolicLink()).toBe(false); + + // File should have content + const content = await readFile(symlinkPath, 'utf-8'); + expect(content).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('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); + + // Multiple writes + await atomicWrite(symlinkPath, 'v2'); + await atomicWrite(symlinkPath, 'v3'); + await atomicWrite(symlinkPath, 'v4'); + + // Symlink should still exist + const lstatResult = await (await import('node:fs/promises')).lstat(symlinkPath); + expect(lstatResult.isSymbolicLink()).toBe(true); + + // Final content + const content = await readFile(realFile, 'utf-8'); + expect(content).toBe('v4'); + }); +}); From b87a9737b51ca985e5c999990cec2506b3ba37f3 Mon Sep 17 00:00:00 2001 From: hkkin <17541011217@163.com> Date: Mon, 15 Jun 2026 02:58:56 +0800 Subject: [PATCH 2/3] chore: add changeset for atomicWrite symlink fix --- .changeset/fix-atomic-write-symlink.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/fix-atomic-write-symlink.md 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. From 671d523291dc129c3da60d9825ad8677652b5f39 Mon Sep 17 00:00:00 2001 From: hkkin <17541011217@163.com> Date: Mon, 15 Jun 2026 03:15:50 +0800 Subject: [PATCH 3/3] fix: make symlink following opt-in for atomicWrite Address Codex review: following symlinks by default can break path- traversal boundaries in guarded stores (e.g., per-id-json-store). Changes: - atomicWrite: add options.followSymlinks parameter (default false) - writeFileAtomicDurable: add same opt-in parameter - writeConfigFile: pass followSymlinks: true (config.toml is a user-controlled fixed path, not a dynamic store) - Tests: verify default behavior replaces symlink, opt-in preserves it, add circular symlink test This preserves the security boundary for tasks/cron stores while fixing the symlink issue for config.toml. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/agent-core/src/config/toml.ts | 2 +- packages/agent-core/src/utils/fs.ts | 52 ++++++--- .../test/utils/atomic-write.test.ts | 104 +++++++++++------- 3 files changed, 99 insertions(+), 59 deletions(-) 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 d91f38837..ae8d9bab0 100644 --- a/packages/agent-core/src/utils/fs.ts +++ b/packages/agent-core/src/utils/fs.ts @@ -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 @@ -141,31 +159,31 @@ function syncFd(fd: number): Promise { * Atomically write `content` to `filePath`. If the target already exists * it is replaced; if it does not exist it is created. * - * If `filePath` is a symlink, the write targets the resolved real path so - * the symlink itself is preserved (not replaced by the rename). - * * @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 { - // Resolve symlinks so rename() doesn't replace the symlink itself. - // If lstat or realpath fails (file doesn't exist, broken symlink, etc.), - // fall back to the original path — same behavior as before. let writeTarget = filePath; - try { - const stat = await lstat(filePath); - if (stat.isSymbolicLink()) { - writeTarget = await realpath(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. } - } catch { - // ENOENT = file doesn't exist yet; realpath failure = broken symlink. - // Fall back to original path — rename will create/replace as before. } const hex = randomBytes(4).toString('hex'); diff --git a/packages/agent-core/test/utils/atomic-write.test.ts b/packages/agent-core/test/utils/atomic-write.test.ts index 45d76ff84..d60ed9bfd 100644 --- a/packages/agent-core/test/utils/atomic-write.test.ts +++ b/packages/agent-core/test/utils/atomic-write.test.ts @@ -1,11 +1,13 @@ /** * Tests for atomicWrite — specifically symlink preservation. * - * The core invariant: if `filePath` is a symlink, atomicWrite should - * preserve the symlink (write to the resolved target), not replace it. + * 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 { mkdir, readFile, rm, stat, symlink, writeFile } from 'node:fs/promises'; +import { lstat, mkdir, readFile, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'pathe'; @@ -46,40 +48,55 @@ describe('atomicWrite', () => { expect(content).toBe('new content'); }); - it('preserves symlink and writes to target', async () => { - // Create a real file in a subdirectory (simulating iCloud target) + 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'); - // Create a symlink to the real file const symlinkPath = join(rootDir, 'config.toml'); await symlink(realFile, symlinkPath); - // Verify symlink exists and points to real file - const symlinkStat = await stat(symlinkPath, { bigint: false }); - const lstatResult = await (await import('node:fs/promises')).lstat(symlinkPath); - expect(lstatResult.isSymbolicLink()).toBe(true); + // Verify symlink exists + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); - // Write via atomicWrite using the symlink path - await atomicWrite(symlinkPath, 'updated content'); + // Write with followSymlinks + await atomicWrite(symlinkPath, 'updated content', { followSymlinks: true }); // Symlink should still exist - const afterLstat = await (await import('node:fs/promises')).lstat(symlinkPath); - expect(afterLstat.isSymbolicLink()).toBe(true); + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); // Real file should have new content - const realContent = await readFile(realFile, 'utf-8'); - expect(realContent).toBe('updated content'); + expect(await readFile(realFile, 'utf-8')).toBe('updated content'); // Reading via symlink should also show new content - const symlinkContent = await readFile(symlinkPath, 'utf-8'); - expect(symlinkContent).toBe('updated content'); + expect(await readFile(symlinkPath, 'utf-8')).toBe('updated content'); }); - it('replaces broken symlink when target does not exist', async () => { - // Create symlink to non-existent file (broken symlink) + 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'); @@ -87,23 +104,17 @@ describe('atomicWrite', () => { await symlink(realFile, symlinkPath); // Verify it's a broken symlink - const lstatBefore = await (await import('node:fs/promises')).lstat(symlinkPath); - expect(lstatBefore.isSymbolicLink()).toBe(true); + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); // realpath fails on broken symlink, falls back to original path - // which replaces the symlink with a regular file - await atomicWrite(symlinkPath, 'new content'); + await atomicWrite(symlinkPath, 'new content', { followSymlinks: true }); // Symlink is replaced by regular file (fallback behavior) - const lstatAfter = await (await import('node:fs/promises')).lstat(symlinkPath); - expect(lstatAfter.isSymbolicLink()).toBe(false); + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(false); - // File should have content - const content = await readFile(symlinkPath, 'utf-8'); - expect(content).toBe('new content'); + 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'); @@ -112,7 +123,7 @@ describe('atomicWrite', () => { expect(content).toBe('brand new'); }); - it('preserves symlink through multiple writes', async () => { + it('followSymlinks: preserves symlink through multiple writes', async () => { const targetDir = join(rootDir, 'target'); await mkdir(targetDir); const realFile = join(targetDir, 'config.toml'); @@ -121,17 +132,28 @@ describe('atomicWrite', () => { const symlinkPath = join(rootDir, 'config.toml'); await symlink(realFile, symlinkPath); - // Multiple writes - await atomicWrite(symlinkPath, 'v2'); - await atomicWrite(symlinkPath, 'v3'); - await atomicWrite(symlinkPath, 'v4'); + await atomicWrite(symlinkPath, 'v2', { followSymlinks: true }); + await atomicWrite(symlinkPath, 'v3', { followSymlinks: true }); + await atomicWrite(symlinkPath, 'v4', { followSymlinks: true }); - // Symlink should still exist - const lstatResult = await (await import('node:fs/promises')).lstat(symlinkPath); - expect(lstatResult.isSymbolicLink()).toBe(true); + expect((await lstat(symlinkPath)).isSymbolicLink()).toBe(true); + expect(await readFile(realFile, 'utf-8')).toBe('v4'); + }); - // Final content - const content = await readFile(realFile, 'utf-8'); - expect(content).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'); }); });