From 792b799e165bae8db2b25a75b6ba2f7d317f6238 Mon Sep 17 00:00:00 2001 From: Miroslav Drbal Date: Thu, 30 Apr 2026 14:57:53 +0200 Subject: [PATCH] test(coding-agent/mcp): defend getter-based tool descriptions against signature regression Built-in tools whose prompt-rendered metadata depends on settings (`TaskTool`, `SearchToolBm25Tool`, `EditTool`) expose `description`/ `label` via getters that re-evaluate on every access. The skip optimization in `#applyActiveToolsByName` is correctness-safe for these because `#computeAppliedToolSignature` reads `tool.description` live each call, so a settings flip mutates the rendered string and differs the signature on the next refresh. This contract was implicit; a future refactor that caches per-tool description strings would silently break it. Defending it explicitly: - Added a regression test that wires a getter onto a CustomTool's `description`, verifies `refreshMCPTools` skips while the underlying state is unchanged, then mutates the state (without changing tool object identity) and verifies the rebuild fires. - Expanded the `#computeAppliedToolSignature` docstring to document the getter-based coverage path and the SDK-init-time closure constants in `sdk.ts` that genuinely cannot change at runtime (`repeatToolDescriptions`, `eagerTasks`, `intentField`, `mcpDiscoveryEnabled`, `secretsEnabled`). Triggered by a review question on whether the skip breaks settings- based prompt changes. It does not, but the property is non-obvious. --- .../coding-agent/src/session/agent-session.ts | 18 +++++-- .../agent-session-tool-rebuild-skip.test.ts | 52 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 93416b98d..fc8e42435 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -2309,10 +2309,20 @@ export class AgentSession { * embeds these in the appended prompt under "## MCP Server Instructions". * A server upgrade can change instructions while keeping tools identical. * - * Inputs NOT covered: tool input schemas, memory instructions read from disk, - * and any other ambient state the rebuild closure reads. Callers must explicitly - * call `refreshBaseSystemPrompt()` after such side-effecting changes; see e.g. - * the memory hooks and `#syncEditToolModeAfterModelChange`. + * Settings-driven tool metadata is covered automatically: built-in tools that + * depend on settings expose `description`/`label` via getters (see `TaskTool`, + * `SearchToolBm25Tool`, `EditTool`), and the signature reads them live on every + * call - so a settings flip that mutates the rendered string differs the signature + * the next time `#applyActiveToolsByName` runs. Do not refactor `describeTool` to + * cache per-tool strings without preserving this property. + * + * Inputs NOT covered: tool input schemas; memory instructions read from disk; + * and SDK-init-time closure constants in `sdk.ts` (`repeatToolDescriptions`, + * `eagerTasks`, `intentField`, `mcpDiscoveryEnabled`, `secretsEnabled`). The + * closure-captured ones cannot change at runtime regardless of skip behavior. + * For everything else, callers must explicitly call `refreshBaseSystemPrompt()` + * after side-effecting changes; see e.g. the memory hooks and + * `#syncEditToolModeAfterModelChange`. */ #computeAppliedToolSignature(toolNames: string[], tools: AgentTool[]): string { // Order-preserving join: any reorder must produce a different signature so diff --git a/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts b/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts index afa4f5aaf..541eb6169 100644 --- a/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts +++ b/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts @@ -334,4 +334,56 @@ describe("AgentSession refreshMCPTools rebuild skipping", () => { expect(rebuildCount).toBe(3); }); + it("rebuilds when a tool's getter-based description reflects new settings state", async () => { + // Built-in tools whose prompt-rendered metadata depends on settings expose + // `description` via getters that re-evaluate on every access (TaskTool reads + // task.disabledAgents/maxConcurrency/isolation.mode/simple/async.enabled, + // SearchToolBm25Tool reads the discoverable MCP tool count, EditTool resolves + // through the current edit-mode definition). The signature reads `tool.description` + // live each call, so a settings flip that mutates the rendered string MUST differ + // the signature on the next `#applyActiveToolsByName`. Defending this contract + // against a future refactor that caches per-tool description strings. + let rebuildCount = 0; + const { session } = newSession( + async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }, + // Discovery-on so the dynamic tool participates in the registry segment too, + // matching the production pattern where `TaskTool` and friends are always + // reachable via the registry. + { mcpDiscoveryEnabled: true }, + ); + + // Reuse the initially-active MCP name so the tool stays in the active list + // across refreshes - we want to defend the path where `tool.description` is read + // for the active descriptionSegment, not just the registrySegment. + const settingState = { disabled: "none" }; + const dynamicTool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "placeholder"); + Object.defineProperty(dynamicTool, "description", { + get: () => `dynamic disabled=${settingState.disabled}`, + enumerable: true, + configurable: true, + }); + + await session.refreshMCPTools([dynamicTool]); + const baseline = rebuildCount; + expect(baseline).toBeGreaterThanOrEqual(1); + + // Same underlying state, same tool object identity: skip. + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline); + + // Mutate the settings-backed state. The tool object identity does not change, + // but its `description` getter now returns a new string. The signature must + // pick this up live (no per-tool caching) and force a rebuild. + settingState.disabled = "plan,explore"; + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline + 1); + + // Same state again: skip. + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline + 1); + }); + });