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); + }); + });