mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-23 02:45:38 -06:00
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: <bool>} 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 <grynn@users.noreply.github.com>
This commit is contained in:
@@ -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<typeof validateJsonSchemaValue>[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<unknown> {
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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 },
|
||||
};
|
||||
},
|
||||
};
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
|
||||
@@ -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", {
|
||||
|
||||
Reference in New Issue
Block a user