fix: detach all memory tools when enabling memfs (#900)
Co-authored-by: Letta <noreply@letta.com>
This commit is contained in:
@@ -233,6 +233,7 @@ import {
|
||||
import {
|
||||
isFileEditTool,
|
||||
isFileWriteTool,
|
||||
isMemoryTool,
|
||||
isPatchTool,
|
||||
isShellTool,
|
||||
} from "./helpers/toolNameMapping";
|
||||
@@ -2693,8 +2694,7 @@ export default function App({
|
||||
for (const line of buffersRef.current.byId.values()) {
|
||||
if (line.kind !== "tool_call") continue;
|
||||
if (!line.toolCallId || !line.name) continue;
|
||||
if (line.name !== "memory" && line.name !== "memory_apply_patch")
|
||||
continue;
|
||||
if (!isMemoryTool(line.name)) continue;
|
||||
if (memorySyncProcessedToolCallsRef.current.has(line.toolCallId))
|
||||
continue;
|
||||
newToolCallIds.push(line.toolCallId);
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
*/
|
||||
|
||||
import { isInteractiveApprovalTool } from "../../tools/interactivePolicy";
|
||||
import { MEMORY_TOOL_NAMES } from "../../tools/toolset";
|
||||
|
||||
/**
|
||||
* Maps internal tool names to user-friendly display names.
|
||||
@@ -141,7 +142,7 @@ export function alwaysRequiresUserInput(name: string): boolean {
|
||||
* Checks if a tool is a memory tool (server-side memory management)
|
||||
*/
|
||||
export function isMemoryTool(name: string): boolean {
|
||||
return name === "memory" || name === "memory_apply_patch";
|
||||
return MEMORY_TOOL_NAMES.has(name);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
17
src/tests/cli/toolNameMapping.test.ts
Normal file
17
src/tests/cli/toolNameMapping.test.ts
Normal file
@@ -0,0 +1,17 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { isMemoryTool } from "../../cli/helpers/toolNameMapping";
|
||||
|
||||
describe("toolNameMapping.isMemoryTool", () => {
|
||||
test("recognizes all supported memory tool names", () => {
|
||||
expect(isMemoryTool("memory")).toBe(true);
|
||||
expect(isMemoryTool("memory_apply_patch")).toBe(true);
|
||||
expect(isMemoryTool("memory_insert")).toBe(true);
|
||||
expect(isMemoryTool("memory_replace")).toBe(true);
|
||||
expect(isMemoryTool("memory_rethink")).toBe(true);
|
||||
});
|
||||
|
||||
test("returns false for non-memory tools", () => {
|
||||
expect(isMemoryTool("bash")).toBe(false);
|
||||
expect(isMemoryTool("web_search")).toBe(false);
|
||||
});
|
||||
});
|
||||
75
src/tests/tools/toolset-memfs-detach.test.ts
Normal file
75
src/tests/tools/toolset-memfs-detach.test.ts
Normal file
@@ -0,0 +1,75 @@
|
||||
// Tests for detaching server-side memory tools when enabling memfs
|
||||
|
||||
import { beforeEach, describe, expect, mock, test } from "bun:test";
|
||||
|
||||
// Mock getClient before importing the module under test
|
||||
|
||||
const detachMock = mock((_toolId: string, _opts: { agent_id: string }) =>
|
||||
Promise.resolve({}),
|
||||
);
|
||||
const retrieveMock = mock((_agentId: string, _opts?: Record<string, unknown>) =>
|
||||
Promise.resolve({
|
||||
tools: [
|
||||
{ name: "memory", id: "tool-memory" },
|
||||
{ name: "memory_apply_patch", id: "tool-memory-apply" },
|
||||
{ name: "memory_insert", id: "tool-memory-insert" },
|
||||
{ name: "memory_replace", id: "tool-memory-replace" },
|
||||
{ name: "memory_rethink", id: "tool-memory-rethink" },
|
||||
{ name: "web_search", id: "tool-web-search" },
|
||||
// No id should be ignored
|
||||
{ name: "memory_replace" },
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
const mockGetClient = mock(() =>
|
||||
Promise.resolve({
|
||||
agents: {
|
||||
retrieve: retrieveMock,
|
||||
tools: {
|
||||
detach: detachMock,
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
mock.module("../../agent/client", () => ({
|
||||
getClient: mockGetClient,
|
||||
}));
|
||||
|
||||
const { detachMemoryTools } = await import("../../tools/toolset");
|
||||
|
||||
describe("detachMemoryTools", () => {
|
||||
beforeEach(() => {
|
||||
detachMock.mockClear();
|
||||
retrieveMock.mockClear();
|
||||
mockGetClient.mockClear();
|
||||
});
|
||||
|
||||
test("detaches all known memory tool variants", async () => {
|
||||
const detached = await detachMemoryTools("agent-123");
|
||||
expect(detached).toBe(true);
|
||||
|
||||
const detachedToolIds = detachMock.mock.calls.map((call) => call[0]);
|
||||
expect(detachedToolIds).toEqual([
|
||||
"tool-memory",
|
||||
"tool-memory-apply",
|
||||
"tool-memory-insert",
|
||||
"tool-memory-replace",
|
||||
"tool-memory-rethink",
|
||||
]);
|
||||
|
||||
// Ensure we did not detach unrelated tools
|
||||
expect(detachedToolIds.includes("tool-web-search")).toBe(false);
|
||||
});
|
||||
|
||||
test("returns false when no memory tools are attached", async () => {
|
||||
retrieveMock.mockResolvedValueOnce({
|
||||
tools: [{ name: "web_search", id: "tool-web-search" }],
|
||||
});
|
||||
|
||||
const detached = await detachMemoryTools("agent-123");
|
||||
expect(detached).toBe(false);
|
||||
expect(detachMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -15,6 +15,16 @@ import {
|
||||
const CODEX_TOOLS = OPENAI_PASCAL_TOOLS;
|
||||
const GEMINI_TOOLS = GEMINI_PASCAL_TOOLS;
|
||||
|
||||
// Server-side memory tool names that can mutate memory blocks.
|
||||
// When memfs is enabled, we detach ALL of these from the agent.
|
||||
export const MEMORY_TOOL_NAMES = new Set([
|
||||
"memory",
|
||||
"memory_apply_patch",
|
||||
"memory_insert",
|
||||
"memory_replace",
|
||||
"memory_rethink",
|
||||
]);
|
||||
|
||||
// Toolset type including snake_case variants
|
||||
export type ToolsetName =
|
||||
| "codex"
|
||||
@@ -130,7 +140,7 @@ export async function detachMemoryTools(agentId: string): Promise<boolean> {
|
||||
|
||||
let detachedAny = false;
|
||||
for (const tool of currentTools) {
|
||||
if (tool.name === "memory" || tool.name === "memory_apply_patch") {
|
||||
if (tool.name && MEMORY_TOOL_NAMES.has(tool.name)) {
|
||||
if (tool.id) {
|
||||
await client.agents.tools.detach(tool.id, { agent_id: agentId });
|
||||
detachedAny = true;
|
||||
|
||||
Reference in New Issue
Block a user