From aebe360349aef1c6ec4e4e67c5e81050c4a07728 Mon Sep 17 00:00:00 2001 From: Vishal Doshi Date: Tue, 18 Aug 2026 03:46:02 +0530 Subject: [PATCH] fix(agents): memory-flush append-only write returns outputSchema-conforming details (#120404) * fix(agents): make memory-flush append-only write honor the write outputSchema The memory-flush append-only write wrapper inherits the base write tool (and its declared outputSchema) via spread, but returned {path, appendOnly} details. The code-mode bridge validates results against the declared schema and rejected the call AFTER the append side effect had landed (reported as the outputSchema variant in #120385). Return {changed: true, created: } via a best-effort pre-append existence probe; sandbox-bridged appends omit created rather than guess. Exports WriteToolOutputSchema for the new contract test. * fix(agents): memory-flush append-only write returns outputSchema-conforming details Address ClawSweeper review findings on #120404: P1 - Update the remaining append-only wrapper expectations. Three test sites still asserted the removed `{ path, appendOnly }` / `created` shapes and would have failed after the return expression changed: - src/agents/agent-tools.workspace-paths.test.ts (2 assertions) - src/agents/embedded-agent-runner/run/attempt.memory-flush-forwarding.test.ts - src/agents/agent-tools.memory-flush-append-write.test.ts All now assert `{ changed: true }` and are included in focused validation. P2 - Avoid reporting a non-authoritative created flag. Removed the pre-append existence probe entirely rather than narrowing it. The probe ran before the append, so another writer could create or remove the target in between while the result still claimed `created` - a TOCTOU guess the append path cannot make authoritatively. The inherited write outputSchema permits the bare `{ changed: true }` shape, so the wrapper now reports only what the call actually knows. This also deletes the sandbox/non-sandbox divergence the probe introduced. Net effect: -31 lines. The fix is now purely subtractive on the source side. Validation: 7 test files, 85 passed / 2 skipped; oxlint clean on all touched files. * test(agents): validate memory-flush schema via public tool * fix(agents): condense memory flush result contract comment * test(agents): use narrow write-tool barrel --------- Co-authored-by: Grynn --- ...nt-tools.memory-flush-append-write.test.ts | 87 +++++++++++++++++++ src/agents/agent-tools.read.ts | 7 +- .../agent-tools.workspace-paths.test.ts | 10 +-- .../attempt.memory-flush-forwarding.test.ts | 5 +- 4 files changed, 93 insertions(+), 16 deletions(-) create mode 100644 src/agents/agent-tools.memory-flush-append-write.test.ts diff --git a/src/agents/agent-tools.memory-flush-append-write.test.ts b/src/agents/agent-tools.memory-flush-append-write.test.ts new file mode 100644 index 000000000000..5447faf5a27d --- /dev/null +++ b/src/agents/agent-tools.memory-flush-append-write.test.ts @@ -0,0 +1,87 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { validateJsonSchemaValue } from "../plugins/schema-validator.js"; +import { wrapToolMemoryFlushAppendOnlyWrite } from "./agent-tools.read.js"; +import type { AnyAgentTool } from "./agent-tools.types.js"; +import { createWriteTool } from "./sessions/tools/index.js"; + +const RELATIVE_PATH = "memory/2026-08-08.md"; + +let declaredWriteOutputSchema: Parameters[0]["schema"]; + +function baseWriteTool(): AnyAgentTool { + return { + name: "write", + description: "Write a file.", + parameters: { type: "object", properties: {} }, + outputSchema: declaredWriteOutputSchema, + execute: async () => { + throw new Error("append-only wrapper should not delegate for append params"); + }, + } as unknown as AnyAgentTool; +} + +function validateAgainstDeclaredSchema(value: unknown) { + return validateJsonSchemaValue({ + schema: declaredWriteOutputSchema, + cacheKey: "test:memory-flush-write-output", + value, + cache: false, + }); +} + +describe("wrapToolMemoryFlushAppendOnlyWrite output contract", () => { + let root: string; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), "memory-flush-write-")); + // Mirror the catalog path: declared schemas are JSON-serialized before the + // bridge validates results against them. Read the schema from the public + // tool factory so production internals do not need a test-only export. + const writeTool = createWriteTool(root) as unknown as AnyAgentTool; + declaredWriteOutputSchema = structuredClone(writeTool.outputSchema) as unknown as Parameters< + typeof validateJsonSchemaValue + >[0]["schema"]; + }); + + afterEach(async () => { + await fs.rm(root, { recursive: true, force: true }); + }); + + async function runAppend(): Promise { + const wrapped = wrapToolMemoryFlushAppendOnlyWrite(baseWriteTool(), { + root, + relativePath: RELATIVE_PATH, + }); + const result = await wrapped.execute( + "call-1", + { path: RELATIVE_PATH, content: "hello" }, + new AbortController().signal, + undefined, + ); + return (result as { details?: unknown }).details; + } + + it("returns write-schema-conforming details when creating the memory file", async () => { + const details = await runAppend(); + expect(details).toEqual({ changed: true }); + expect(validateAgainstDeclaredSchema(details).ok).toBe(true); + }); + + it("returns write-schema-conforming details when appending to an existing file", async () => { + const absolute = path.join(root, RELATIVE_PATH); + await fs.mkdir(path.dirname(absolute), { recursive: true }); + await fs.writeFile(absolute, "seed\n", "utf-8"); + const details = await runAppend(); + expect(details).toEqual({ changed: true }); + expect(validateAgainstDeclaredSchema(details).ok).toBe(true); + expect(await fs.readFile(absolute, "utf-8")).toBe("seed\nhello"); + }); + + it("documents the pre-fix regression: append-only metadata violates the declared schema", () => { + const validation = validateAgainstDeclaredSchema({ path: RELATIVE_PATH, appendOnly: true }); + expect(validation.ok).toBe(false); + }); +}); diff --git a/src/agents/agent-tools.read.ts b/src/agents/agent-tools.read.ts index f417bd567a61..1c97e788a0c0 100644 --- a/src/agents/agent-tools.read.ts +++ b/src/agents/agent-tools.read.ts @@ -753,12 +753,11 @@ export function wrapToolMemoryFlushAppendOnlyWrite( sandbox: options.sandbox, signal, }); + // This wrapper inherits the write tool's output schema, so report only + // the authoritative `changed`; deriving `created` before append is racy. return { content: [{ type: "text", text: `Appended content to ${options.relativePath}.` }], - details: { - path: options.relativePath, - appendOnly: true, - }, + details: { changed: true }, }; }, }; diff --git a/src/agents/agent-tools.workspace-paths.test.ts b/src/agents/agent-tools.workspace-paths.test.ts index b26eaa2fe8cd..782ca76e825c 100644 --- a/src/agents/agent-tools.workspace-paths.test.ts +++ b/src/agents/agent-tools.workspace-paths.test.ts @@ -1062,10 +1062,7 @@ describe("FS tools with workspaceOnly=false", () => { expect(hasToolError(result)).toBe(false); expect(result).toStrictEqual({ content: [{ type: "text", text: "Appended content to memory/2026-03-07.md." }], - details: { - path: "memory/2026-03-07.md", - appendOnly: true, - }, + details: { changed: true }, }); await expect(fs.readFile(allowedAbsolutePath, "utf-8")).resolves.toBe("seed\nnew note"); }); @@ -1090,10 +1087,7 @@ describe("FS tools with workspaceOnly=false", () => { expect(hasToolError(result)).toBe(false); expect(result).toStrictEqual({ content: [{ type: "text", text: "Appended content to memory/2026-03-08.md." }], - details: { - path: "memory/2026-03-08.md", - appendOnly: true, - }, + details: { changed: true }, }); await expect(fs.readFile(allowedAbsolutePath, "utf-8")).resolves.toBe("new note"); }); diff --git a/src/agents/embedded-agent-runner/run/attempt.memory-flush-forwarding.test.ts b/src/agents/embedded-agent-runner/run/attempt.memory-flush-forwarding.test.ts index 2464e5ed76dc..1fd7dd9ade65 100644 --- a/src/agents/embedded-agent-runner/run/attempt.memory-flush-forwarding.test.ts +++ b/src/agents/embedded-agent-runner/run/attempt.memory-flush-forwarding.test.ts @@ -93,10 +93,7 @@ describe("runEmbeddedAttempt memory flush tool forwarding", () => { expect(result.content).toEqual([ { type: "text", text: `Appended content to ${MEMORY_RELATIVE_PATH}.` }, ]); - expect(result.details).toEqual({ - path: MEMORY_RELATIVE_PATH, - appendOnly: true, - }); + expect(result.details).toEqual({ changed: true }); await expect(fs.readFile(memoryFile, "utf-8")).resolves.toBe("seed\nnew durable note"); await expect( wrapped.execute("call-memory-flush-deny", {