From dbaf3e93d36653f1b77a7c75ea5e4449c8d81351 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 11 Oct 2026 22:39:53 -0700 Subject: [PATCH 1/3] fix(agent): memoize toolSetDigest per tool-set identity toolSetDigest re-hashed the full tool array on every call. Cache the digest in a WeakMap keyed by array identity: the director owns stable tool arrays and swaps them wholesale (constructor, updateToolDefinitions) without in-place mutation, so the same reference always means the same content and a new reference (tools changed) recomputes. A WeakMap also avoids retaining arrays the director has dropped. Output is byte-identical for the same tool set. Add a spy test proving one sha256 per tool set across repeated digest calls, using withMockedModule from testkit/mock-module.ts. --- src/agent/director.ts | 14 +++++++- src/agent/tool-set-digest.test.ts | 53 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 src/agent/tool-set-digest.test.ts diff --git a/src/agent/director.ts b/src/agent/director.ts index b0dc8dcd9..20e210ba9 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -59,14 +59,26 @@ const logger = getLogger([LOG_NAMESPACE_ROOT, "agent", "director"]); // (mount, description edit, reorder) drops the next turn's cache hit. The // digest stays deterministic — it only moves when discovery grows the set. // Hashed: MCP descriptions are arbitrary server text, not log material. +// +// Memoized per array identity: the director owns stable tool arrays and swaps +// them wholesale (constructor, updateToolDefinitions) without in-place +// mutation, so the same reference always means the same content and a new +// reference (tools changed) misses the WeakMap and recomputes. A WeakMap also +// avoids retaining arrays the director has dropped. +const toolSetDigestCache = new WeakMap(); + export function toolSetDigest(tools: readonly ToolDefinition[]): string { + const cached = toolSetDigestCache.get(tools); + if (cached !== undefined) return cached; const shape = tools .map( (t) => `${t.name}:${t.description ?? ""}:${JSON.stringify(t.inputSchema ?? null)}`, ) .join("|"); - return createHash("sha256").update(shape).digest("hex").slice(0, 12); + const digest = createHash("sha256").update(shape).digest("hex").slice(0, 12); + toolSetDigestCache.set(tools, digest); + return digest; } function isInternalRecoveryAbort( diff --git a/src/agent/tool-set-digest.test.ts b/src/agent/tool-set-digest.test.ts new file mode 100644 index 000000000..addf54262 --- /dev/null +++ b/src/agent/tool-set-digest.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, test } from "bun:test"; +import { withMockedModule } from "../../testkit/mock-module.js"; + +// Spy on createHash to prove toolSetDigest memoizes: repeated digest calls +// with the same tool array must run the sha256 exactly once, and a new array +// (tools changed) must run it again. The mock delegates to the real +// implementation (captured before mocking) so behavior is unchanged; +// hashCalls only measures the delta around each assertion. +let hashCalls = 0; +await withMockedModule( + import.meta.resolve("node:crypto"), + (real) => ({ + ...real, + createHash: (algorithm: string) => { + hashCalls += 1; + return real.createHash(algorithm); + }, + }), +); + +// Dynamic import so director.ts resolves node:crypto through the mock above. +const { toolSetDigest } = await import("./director.js"); + +const base = { + name: "read_file", + description: "read a file", + inputSchema: { type: "object" }, +}; + +describe("toolSetDigest memoization", () => { + test("hashes once per tool set across repeated digest calls", () => { + const tools = [{ ...base }]; + hashCalls = 0; + const first = toolSetDigest(tools); + const second = toolSetDigest(tools); + const third = toolSetDigest(tools); + expect(second).toBe(first); + expect(third).toBe(first); + expect(hashCalls).toBe(1); + }); + + test("recomputes when a new tool set arrives", () => { + const tools = [{ ...base }]; + const changed = [{ ...base, description: "changed" }]; + hashCalls = 0; + const digest = toolSetDigest(tools); + expect(hashCalls).toBe(1); + const changedDigest = toolSetDigest(changed); + expect(hashCalls).toBe(2); + expect(changedDigest).not.toBe(toolSetDigest(tools)); + expect(digest).toBe(toolSetDigest(tools)); + }); +}); From 5357a1acffa1adfeeb4163129c2a2d5398ba63b3 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 11 Oct 2026 23:31:27 -0700 Subject: [PATCH 2/3] perf(agent): memoize toolSetDigest per director, not per module The digest is only ever consumed by ChatDirectorImpl.updateToolDefinitions, so a module-level WeakMap keyed by array identity is heavier than the call pattern needs. Move the memoization onto the director as a last-digest field: the incoming set is hashed once when installed and the previous set's digest is reused from the field on the next update. toolSetDigest becomes a pure function with identical digest values; the proof test now spies through updateToolDefinitions to show one hash per installed tool set per director. --- src/agent/director.ts | 31 +++++++++++++---------- src/agent/tool-set-digest.test.ts | 41 ++++++++++++++++++------------- 2 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/agent/director.ts b/src/agent/director.ts index 20e210ba9..4d14d2fd9 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -60,25 +60,19 @@ const logger = getLogger([LOG_NAMESPACE_ROOT, "agent", "director"]); // digest stays deterministic — it only moves when discovery grows the set. // Hashed: MCP descriptions are arbitrary server text, not log material. // -// Memoized per array identity: the director owns stable tool arrays and swaps -// them wholesale (constructor, updateToolDefinitions) without in-place -// mutation, so the same reference always means the same content and a new -// reference (tools changed) misses the WeakMap and recomputes. A WeakMap also -// avoids retaining arrays the director has dropped. -const toolSetDigestCache = new WeakMap(); - +// Pure by design: the memoization lives on the director (_toolSetDigest), the +// only production caller, so the module keeps no per-array state. Callers swap +// tool arrays wholesale (constructor, updateToolDefinitions) without in-place +// mutation; mutating an installed array in place would leave the director's +// cached digest stale. export function toolSetDigest(tools: readonly ToolDefinition[]): string { - const cached = toolSetDigestCache.get(tools); - if (cached !== undefined) return cached; const shape = tools .map( (t) => `${t.name}:${t.description ?? ""}:${JSON.stringify(t.inputSchema ?? null)}`, ) .join("|"); - const digest = createHash("sha256").update(shape).digest("hex").slice(0, 12); - toolSetDigestCache.set(tools, digest); - return digest; + return createHash("sha256").update(shape).digest("hex").slice(0, 12); } function isInternalRecoveryAbort( @@ -487,6 +481,13 @@ class ChatDirectorImpl extends DefaultDirector { private readonly askOperatorCalls = new Set(); private readonly _systemPrompt: string; private _toolDefinitions: ToolDefinition[]; + // Digest of the installed _toolDefinitions, stored when the array is set so + // updateToolDefinitions can compare an incoming set without re-hashing the + // current one. One slot per director — the digest is only consumed by the + // next update on this instance, never by shared/static context (all + // production call sites are here). Stale only if a caller mutates the + // installed array in place; callers swap arrays wholesale instead. + private _toolSetDigest: string | undefined; private inactivityTimeoutMs: number | undefined; private totalTimeoutMs: number | undefined; // Host-owned live object, attached via setWorkflowCoordinator — never a @@ -711,9 +712,13 @@ class ChatDirectorImpl extends DefaultDirector { } updateToolDefinitions(toolDefinitions: ToolDefinition[]): void { - const before = toolSetDigest(this._toolDefinitions); + // The stored digest covers the current set (installed by the constructor + // or a prior update); only the very first update has to hash it, and the + // fallback runs once per director. + const before = this._toolSetDigest ?? toolSetDigest(this._toolDefinitions); const after = toolSetDigest(toolDefinitions); this._toolDefinitions = toolDefinitions; + this._toolSetDigest = after; if (before === after) return; logger.debug`tool-set-changed count=${String(this._toolDefinitions.length)} before=${before} after=${after}`; } diff --git a/src/agent/tool-set-digest.test.ts b/src/agent/tool-set-digest.test.ts index addf54262..8d841076e 100644 --- a/src/agent/tool-set-digest.test.ts +++ b/src/agent/tool-set-digest.test.ts @@ -1,11 +1,12 @@ import { describe, expect, test } from "bun:test"; import { withMockedModule } from "../../testkit/mock-module.js"; -// Spy on createHash to prove toolSetDigest memoizes: repeated digest calls -// with the same tool array must run the sha256 exactly once, and a new array -// (tools changed) must run it again. The mock delegates to the real -// implementation (captured before mocking) so behavior is unchanged; -// hashCalls only measures the delta around each assertion. +// Spy on createHash to prove toolSetDigest memoizes per director: installing +// a tool set runs the sha256 exactly once for that set, and comparing a later +// update against the stored per-director digest must not re-hash the previous +// set. The mock delegates to the real implementation (captured before mocking) +// so behavior is unchanged; hashCalls only measures the delta around each +// assertion. let hashCalls = 0; await withMockedModule( import.meta.resolve("node:crypto"), @@ -19,7 +20,7 @@ await withMockedModule( ); // Dynamic import so director.ts resolves node:crypto through the mock above. -const { toolSetDigest } = await import("./director.js"); +const { createChatDirector } = await import("./director.js"); const base = { name: "read_file", @@ -28,26 +29,32 @@ const base = { }; describe("toolSetDigest memoization", () => { - test("hashes once per tool set across repeated digest calls", () => { + test("hashes each installed tool set once per director", () => { + const director = createChatDirector("system", [], {}); const tools = [{ ...base }]; + const changed = [{ ...base, description: "changed" }]; + + // Warm-up installs `tools`; its digest is stored on the director. + director.updateToolDefinitions(tools); hashCalls = 0; - const first = toolSetDigest(tools); - const second = toolSetDigest(tools); - const third = toolSetDigest(tools); - expect(second).toBe(first); - expect(third).toBe(first); + // The previous set's digest comes from the per-director field (no re-hash); + // only the incoming set is hashed. + director.updateToolDefinitions(changed); expect(hashCalls).toBe(1); }); test("recomputes when a new tool set arrives", () => { + const director = createChatDirector("system", [], {}); const tools = [{ ...base }]; const changed = [{ ...base, description: "changed" }]; + + director.updateToolDefinitions(tools); + director.updateToolDefinitions(changed); hashCalls = 0; - const digest = toolSetDigest(tools); + // A fresh reference (even with identical content) misses the stored slot + // and re-runs the hash; the digest gates the tool-set-changed log, so + // identical content still logs nothing. + director.updateToolDefinitions([{ ...base }]); expect(hashCalls).toBe(1); - const changedDigest = toolSetDigest(changed); - expect(hashCalls).toBe(2); - expect(changedDigest).not.toBe(toolSetDigest(tools)); - expect(digest).toBe(toolSetDigest(tools)); }); }); From 6a3c8ab5cf2d244cb1c70ca0aa30987c88539937 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 12 Oct 2026 16:31:53 -0700 Subject: [PATCH 3/3] refactor(agent): drop tool-set digest narration --- src/agent/director.ts | 15 --------------- src/agent/tool-set-digest.test.ts | 13 ------------- 2 files changed, 28 deletions(-) diff --git a/src/agent/director.ts b/src/agent/director.ts index 4d14d2fd9..6943c4172 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -59,12 +59,6 @@ const logger = getLogger([LOG_NAMESPACE_ROOT, "agent", "director"]); // (mount, description edit, reorder) drops the next turn's cache hit. The // digest stays deterministic — it only moves when discovery grows the set. // Hashed: MCP descriptions are arbitrary server text, not log material. -// -// Pure by design: the memoization lives on the director (_toolSetDigest), the -// only production caller, so the module keeps no per-array state. Callers swap -// tool arrays wholesale (constructor, updateToolDefinitions) without in-place -// mutation; mutating an installed array in place would leave the director's -// cached digest stale. export function toolSetDigest(tools: readonly ToolDefinition[]): string { const shape = tools .map( @@ -481,12 +475,6 @@ class ChatDirectorImpl extends DefaultDirector { private readonly askOperatorCalls = new Set(); private readonly _systemPrompt: string; private _toolDefinitions: ToolDefinition[]; - // Digest of the installed _toolDefinitions, stored when the array is set so - // updateToolDefinitions can compare an incoming set without re-hashing the - // current one. One slot per director — the digest is only consumed by the - // next update on this instance, never by shared/static context (all - // production call sites are here). Stale only if a caller mutates the - // installed array in place; callers swap arrays wholesale instead. private _toolSetDigest: string | undefined; private inactivityTimeoutMs: number | undefined; private totalTimeoutMs: number | undefined; @@ -712,9 +700,6 @@ class ChatDirectorImpl extends DefaultDirector { } updateToolDefinitions(toolDefinitions: ToolDefinition[]): void { - // The stored digest covers the current set (installed by the constructor - // or a prior update); only the very first update has to hash it, and the - // fallback runs once per director. const before = this._toolSetDigest ?? toolSetDigest(this._toolDefinitions); const after = toolSetDigest(toolDefinitions); this._toolDefinitions = toolDefinitions; diff --git a/src/agent/tool-set-digest.test.ts b/src/agent/tool-set-digest.test.ts index 8d841076e..cf616c4fa 100644 --- a/src/agent/tool-set-digest.test.ts +++ b/src/agent/tool-set-digest.test.ts @@ -1,12 +1,6 @@ import { describe, expect, test } from "bun:test"; import { withMockedModule } from "../../testkit/mock-module.js"; -// Spy on createHash to prove toolSetDigest memoizes per director: installing -// a tool set runs the sha256 exactly once for that set, and comparing a later -// update against the stored per-director digest must not re-hash the previous -// set. The mock delegates to the real implementation (captured before mocking) -// so behavior is unchanged; hashCalls only measures the delta around each -// assertion. let hashCalls = 0; await withMockedModule( import.meta.resolve("node:crypto"), @@ -19,7 +13,6 @@ await withMockedModule( }), ); -// Dynamic import so director.ts resolves node:crypto through the mock above. const { createChatDirector } = await import("./director.js"); const base = { @@ -34,11 +27,8 @@ describe("toolSetDigest memoization", () => { const tools = [{ ...base }]; const changed = [{ ...base, description: "changed" }]; - // Warm-up installs `tools`; its digest is stored on the director. director.updateToolDefinitions(tools); hashCalls = 0; - // The previous set's digest comes from the per-director field (no re-hash); - // only the incoming set is hashed. director.updateToolDefinitions(changed); expect(hashCalls).toBe(1); }); @@ -51,9 +41,6 @@ describe("toolSetDigest memoization", () => { director.updateToolDefinitions(tools); director.updateToolDefinitions(changed); hashCalls = 0; - // A fresh reference (even with identical content) misses the stored slot - // and re-runs the hash; the digest gates the tool-set-changed log, so - // identical content still logs nothing. director.updateToolDefinitions([{ ...base }]); expect(hashCalls).toBe(1); });