mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-07-10 01:39:25 +00:00
fix(agent-core-v2): restore native append for Write append mode
- add IHostFileSystem.appendText backed by fs.appendFile (O_APPEND) - route WriteTool append through it instead of read-then-rewrite, so existing content is never read, truncated, or clobbered by concurrent writers and a crash mid-append can only lose the new bytes - update typed host-fs test fakes and WriteTool append assertions
This commit is contained in:
parent
989ca43ff7
commit
c6b6c30bd4
8 changed files with 45 additions and 29 deletions
|
|
@ -5,7 +5,7 @@
|
|||
* Bound at App scope.
|
||||
*/
|
||||
|
||||
import { open, readFile, readdir, stat, mkdir, rm, writeFile } from 'node:fs/promises';
|
||||
import { appendFile, open, readFile, readdir, stat, mkdir, rm, writeFile } from 'node:fs/promises';
|
||||
|
||||
import { InstantiationType } from '#/_base/di/extensions';
|
||||
import { LifecycleScope, registerScopedService } from '#/_base/di/scope';
|
||||
|
|
@ -52,6 +52,10 @@ export class HostFileSystem implements IHostFileSystem {
|
|||
await writeFile(path, data, 'utf8');
|
||||
}
|
||||
|
||||
async appendText(path: string, data: string): Promise<void> {
|
||||
await appendFile(path, data, 'utf8');
|
||||
}
|
||||
|
||||
async readBytes(path: string, n?: number): Promise<Uint8Array> {
|
||||
if (n === undefined) {
|
||||
const buf = await readFile(path);
|
||||
|
|
|
|||
|
|
@ -6,9 +6,9 @@
|
|||
* (mirroring `mkdir(parents=True, exist_ok=True)`). Path access policy is
|
||||
* resolved before any filesystem I/O.
|
||||
*
|
||||
* `IHostFileSystem.writeText` has no mode flag: overwrite maps to a direct
|
||||
* write, while append reads the existing content first (treating a missing
|
||||
* file as empty) and writes the concatenation back.
|
||||
* Append uses `IHostFileSystem.appendText` (a native `O_APPEND`-style append),
|
||||
* so existing content is never read or rewritten — keeping appends atomic with
|
||||
* respect to concurrent writers and safe against mid-write crashes.
|
||||
*
|
||||
* Write access flows through the os `hostFs` domain (`IHostFileSystem`); path
|
||||
* semantics (home expansion, path class) come from the `hostEnvironment`
|
||||
|
|
@ -107,14 +107,7 @@ export class WriteTool implements BuiltinTool<WriteInput> {
|
|||
try {
|
||||
const mode = args.mode ?? 'overwrite';
|
||||
if (mode === 'append') {
|
||||
let existing = '';
|
||||
try {
|
||||
existing = await this.fs.readText(safePath);
|
||||
} catch (error) {
|
||||
const code = (error as { code?: unknown } | null)?.code;
|
||||
if (code !== 'ENOENT') throw error;
|
||||
}
|
||||
await this.fs.writeText(safePath, existing + args.content);
|
||||
await this.fs.appendText(safePath, args.content);
|
||||
} else {
|
||||
await this.fs.writeText(safePath, args.content);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -34,6 +34,15 @@ export interface IHostFileSystem {
|
|||
options?: { encoding?: BufferEncoding; errors?: TextDecodeErrors },
|
||||
): Promise<string>;
|
||||
writeText(path: string, data: string): Promise<void>;
|
||||
/**
|
||||
* Append UTF-8 `data` to the end of `path`, creating the file if it does not
|
||||
* exist. Maps to a native append (POSIX `O_APPEND` / `fs.appendFile`): it
|
||||
* never reads or truncates existing content, so concurrent readers never see
|
||||
* a partially-rewritten file and a crash mid-write can lose only the new
|
||||
* bytes, never the prior contents. Prefer this over a read-then-rewrite for
|
||||
* log-style appends.
|
||||
*/
|
||||
appendText(path: string, data: string): Promise<void>;
|
||||
/**
|
||||
* Read bytes from `path`. When `n` is given, reads at most the first `n`
|
||||
* bytes (a ranged/prefix read); otherwise reads the whole file. The ranged
|
||||
|
|
|
|||
|
|
@ -6,10 +6,9 @@
|
|||
* minimal fake `IHostFileSystem` inline so the tool can be exercised without
|
||||
* the composition root.
|
||||
*
|
||||
* The v1 append path used `kaos.writeText(path, data, { mode: 'a' })`. v2's
|
||||
* `IHostFileSystem.writeText` has no mode flag, so append is implemented as
|
||||
* `readText` (treating a missing file as empty) followed by `writeText` of the
|
||||
* concatenation. The append-call assertions below reflect that mechanic.
|
||||
* Append is routed through `IHostFileSystem.appendText` (a native append), so
|
||||
* the tool no longer reads the existing file. The append-call assertions below
|
||||
* reflect that single-call mechanic.
|
||||
*/
|
||||
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
|
|
@ -51,6 +50,8 @@ interface WriteFsOptions {
|
|||
readText?: (path: string) => Promise<string>;
|
||||
/** Override writeText. Default no-op. */
|
||||
writeText?: (path: string, data: string) => Promise<void>;
|
||||
/** Override appendText. Default no-op. */
|
||||
appendText?: (path: string, data: string) => Promise<void>;
|
||||
/** Override stat. Default reports an existing directory. */
|
||||
stat?: (path: string) => Promise<HostFileStat>;
|
||||
/** Override mkdir. Default no-op. */
|
||||
|
|
@ -72,12 +73,13 @@ function createWriteFs(options: WriteFsOptions = {}) {
|
|||
}),
|
||||
);
|
||||
const writeText = vi.fn(options.writeText ?? (async () => {}));
|
||||
const appendText = vi.fn(options.appendText ?? (async () => {}));
|
||||
const stat = vi.fn(
|
||||
options.stat ?? (async () => ({ isFile: false, isDirectory: true, size: 0 })),
|
||||
);
|
||||
const mkdir = vi.fn(options.mkdir ?? (async () => {}));
|
||||
const fs = { cwd: '/', readText, writeText, stat, mkdir } as unknown as IHostFileSystem;
|
||||
return { fs, readText, writeText, stat, mkdir };
|
||||
const fs = { cwd: '/', readText, writeText, appendText, stat, mkdir } as unknown as IHostFileSystem;
|
||||
return { fs, readText, writeText, appendText, stat, mkdir };
|
||||
}
|
||||
|
||||
function makeTool(options: WriteFsOptions = {}, workspace = PERMISSIVE_WORKSPACE) {
|
||||
|
|
@ -218,8 +220,8 @@ describe('WriteTool', () => {
|
|||
expect(result.output).toContain('Wrote 5 bytes');
|
||||
});
|
||||
|
||||
it('appends content by reading existing bytes then writing the concatenation', async () => {
|
||||
const { tool, readText, writeText } = makeTool({ readText: async () => 'old' });
|
||||
it('appends content through appendText without reading existing bytes', async () => {
|
||||
const { tool, readText, writeText, appendText } = makeTool();
|
||||
|
||||
const result = await execute(tool, {
|
||||
path: '/tmp/existing.txt',
|
||||
|
|
@ -227,8 +229,9 @@ describe('WriteTool', () => {
|
|||
mode: 'append',
|
||||
});
|
||||
|
||||
expect(readText).toHaveBeenCalledWith('/tmp/existing.txt');
|
||||
expect(writeText).toHaveBeenCalledWith('/tmp/existing.txt', 'old\nhello');
|
||||
expect(appendText).toHaveBeenCalledWith('/tmp/existing.txt', '\nhello');
|
||||
expect(readText).not.toHaveBeenCalled();
|
||||
expect(writeText).not.toHaveBeenCalled();
|
||||
expect(result.output).toContain('Appended 6 bytes');
|
||||
});
|
||||
|
||||
|
|
@ -272,11 +275,11 @@ describe('WriteTool', () => {
|
|||
const expectedBytes = Buffer.byteLength(content, 'utf8');
|
||||
expect(expectedBytes).toBe(5);
|
||||
|
||||
const { tool, writeText } = makeTool({ readText: async () => 'prefix' });
|
||||
const { tool, appendText } = makeTool();
|
||||
|
||||
const result = await execute(tool, { path: '/tmp/menu.txt', content, mode: 'append' });
|
||||
|
||||
expect(writeText).toHaveBeenCalledWith('/tmp/menu.txt', 'prefixcafé');
|
||||
expect(appendText).toHaveBeenCalledWith('/tmp/menu.txt', 'café');
|
||||
expect(result.output).toContain('Appended 5 bytes');
|
||||
});
|
||||
|
||||
|
|
@ -414,9 +417,9 @@ describe('WriteTool', () => {
|
|||
});
|
||||
|
||||
it('appending to a nonexistent file creates it with just the appended bytes', async () => {
|
||||
// Append mode on a missing path returns success and creates the file:
|
||||
// readText rejects with ENOENT, so existing content is treated as empty.
|
||||
const { tool, readText, writeText } = makeTool();
|
||||
// Native append (fs.appendFile) creates the file when it is missing, so
|
||||
// append mode on a new path succeeds and writes exactly the appended bytes.
|
||||
const { tool, readText, appendText } = makeTool();
|
||||
|
||||
const result = await execute(tool, {
|
||||
path: '/tmp/new-append.txt',
|
||||
|
|
@ -426,8 +429,8 @@ describe('WriteTool', () => {
|
|||
|
||||
expect(result.isError).toBeFalsy();
|
||||
expect(toolContentString(result).toLowerCase()).toContain('appended');
|
||||
expect(readText).toHaveBeenCalledWith('/tmp/new-append.txt');
|
||||
expect(writeText).toHaveBeenCalledWith('/tmp/new-append.txt', 'New content');
|
||||
expect(appendText).toHaveBeenCalledWith('/tmp/new-append.txt', 'New content');
|
||||
expect(readText).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('allows absolute writes to a sibling dir that merely shares the work-dir prefix', async () => {
|
||||
|
|
|
|||
|
|
@ -432,6 +432,7 @@ function isFullHostFs(input: unknown): boolean {
|
|||
const keys: readonly (keyof IHostFileSystem)[] = [
|
||||
'readText',
|
||||
'writeText',
|
||||
'appendText',
|
||||
'readBytes',
|
||||
'writeBytes',
|
||||
'readLines',
|
||||
|
|
|
|||
|
|
@ -66,6 +66,7 @@ function fakeFs(files: Record<string, string>): IHostFileSystem {
|
|||
return c;
|
||||
},
|
||||
writeText: async () => {},
|
||||
appendText: async () => {},
|
||||
readBytes: async (p, n) => {
|
||||
const c = fileMap.get(p);
|
||||
if (c === undefined) throw enoent(p);
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ export function createFakeHostFs(overrides: Partial<IHostFileSystem> = {}): IHos
|
|||
_serviceBrand: undefined,
|
||||
readText: () => notImplemented('FakeHostFs.readText'),
|
||||
writeText: () => notImplemented('FakeHostFs.writeText'),
|
||||
appendText: () => notImplemented('FakeHostFs.appendText'),
|
||||
readBytes: () => notImplemented('FakeHostFs.readBytes'),
|
||||
writeBytes: () => notImplemented('FakeHostFs.writeBytes'),
|
||||
readLines: () => notImplemented('FakeHostFs.readLines'),
|
||||
|
|
|
|||
|
|
@ -76,6 +76,10 @@ class MemoryHostFs implements IHostFileSystem {
|
|||
this.files.set(path, data);
|
||||
}
|
||||
|
||||
async appendText(path: string, data: string): Promise<void> {
|
||||
this.files.set(path, (this.files.get(path) ?? '') + data);
|
||||
}
|
||||
|
||||
pauseNextWrite(): { readonly started: Promise<void>; readonly release: () => void } {
|
||||
let started!: () => void;
|
||||
let release!: () => void;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue