From 665fcea763da763e21defc539f48c13062abcbac Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 18:31:30 +0000 Subject: [PATCH 1/2] fix(mcp): lazy on-demand provider activation with checkpoint persistence Replaces the eager activateAllProviders() approach with a model-driven lazy activation model: - Providers connect only when the model first accesses them, not at turn start. No wasted listTools() calls for unused providers. - searchMcpTools({ provider }) and callMcpTool('mcp__provider__*') both auto-activate the named provider on demand. Auth prompts fire normally if credentials are missing or expired. - Activated providers are persisted in the turn checkpoint under activeMcpProviderNames and restored at the start of future turns, so providers stay live across multi-turn conversations without re-activating on every request. - The prompt block advertises configured but inactive providers (name + description) with zero network cost, so the model knows what is available before deciding to activate. - syncResumeState() now also captures activeMcpProviderNamesForResume so checkpoint sync happens correctly after lazy activation inside tools. - loadSkill continues to work and still activates its plugin provider. Co-Authored-By: claude-opus-4-5 --- packages/junior/src/chat/mcp/tool-manager.ts | 55 ++++++------ packages/junior/src/chat/prompt.ts | 34 ++++++- packages/junior/src/chat/respond.ts | 37 ++++++-- .../src/chat/services/turn-checkpoint.ts | 8 ++ .../src/chat/state/turn-session-store.ts | 18 ++++ packages/junior/src/chat/tools/index.ts | 6 +- .../src/chat/tools/skill/call-mcp-tool.ts | 18 +++- .../src/chat/tools/skill/mcp-tool-summary.ts | 6 ++ .../src/chat/tools/skill/search-mcp-tools.ts | 13 +-- packages/junior/src/chat/tools/types.ts | 3 +- .../tests/unit/mcp/tool-manager.test.ts | 76 ++++++++-------- .../unit/services/turn-checkpoint.test.ts | 8 ++ .../tests/unit/tools/call-mcp-tool.test.ts | 29 +++--- .../tests/unit/tools/search-mcp-tools.test.ts | 89 ++++++++----------- specs/plugin-runtime.md | 12 +-- 15 files changed, 257 insertions(+), 155 deletions(-) diff --git a/packages/junior/src/chat/mcp/tool-manager.ts b/packages/junior/src/chat/mcp/tool-manager.ts index fada1ca4..681b8417 100644 --- a/packages/junior/src/chat/mcp/tool-manager.ts +++ b/packages/junior/src/chat/mcp/tool-manager.ts @@ -168,8 +168,6 @@ export interface ManagedMcpToolDescriptor { provider: string; } -type ActiveMcpSkillScope = Pick; - type ActiveMcpSkill = Pick; export interface ManagedMcpTool extends ManagedMcpToolDescriptor { @@ -200,6 +198,32 @@ export class McpToolManager { ); } + /** Return all active provider names, for checkpoint persistence. */ + getActiveProviderNames(): string[] { + return this.getActiveProviders(); + } + + /** Return all configured MCP providers with active/inactive state. + * Never connects to any MCP server. */ + getAvailableProviderCatalog(): Array<{ + provider: string; + description: string; + active: boolean; + }> { + return [...this.pluginsByProvider.entries()] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([provider, plugin]) => ({ + provider, + description: plugin.manifest.description, + active: this.activeProviders.has(provider), + })); + } + + /** Return true when the provider is configured (active or not). */ + hasConfiguredProvider(provider: string): boolean { + return this.pluginsByProvider.has(provider); + } + async activateForSkill(skill: ActiveMcpSkill): Promise { if (!skill.pluginProvider) { return false; @@ -262,11 +286,11 @@ export class McpToolManager { } } + /** Return descriptors for all active MCP provider tools, optionally filtered by provider. */ getActiveToolCatalog( - skills: ActiveMcpSkillScope[], options: { provider?: string } = {}, ): ManagedMcpToolDescriptor[] { - return this.getResolvedActiveTools(skills, options).map((tool) => + return this.getResolvedActiveTools(options).map((tool) => this.toToolDescriptor(tool), ); } @@ -415,9 +439,8 @@ export class McpToolManager { return true; } - /** Return all active ManagedMcpTool objects for the given skill scope. */ + /** Return all active ManagedMcpTool objects, optionally filtered by provider. */ getResolvedActiveTools( - skills: ActiveMcpSkillScope[], options: { provider?: string } = {}, ): ManagedMcpTool[] { const resolved: ManagedMcpTool[] = []; @@ -427,30 +450,12 @@ export class McpToolManager { continue; } - resolved.push(...this.resolveProviderTools(provider, skills)); + resolved.push(...(this.toolsByProvider.get(provider) ?? [])); } return resolved; } - private resolveProviderTools( - provider: string, - skills: ActiveMcpSkillScope[], - ): ManagedMcpTool[] { - const providerTools = this.toolsByProvider.get(provider) ?? []; - if (providerTools.length === 0) { - return []; - } - - const relevantSkills = skills.filter( - (skill) => skill.pluginProvider === provider, - ); - if (relevantSkills.length === 0) { - return []; - } - return providerTools; - } - private toToolDescriptor(tool: ManagedMcpTool): ManagedMcpToolDescriptor { return { name: tool.name, diff --git a/packages/junior/src/chat/prompt.ts b/packages/junior/src/chat/prompt.ts index d869562b..b6123600 100644 --- a/packages/junior/src/chat/prompt.ts +++ b/packages/junior/src/chat/prompt.ts @@ -16,7 +16,10 @@ import { } from "@/chat/sandbox/paths"; import type { ThreadArtifactsState } from "@/chat/state/artifacts"; import type { Skill, SkillMetadata, SkillInvocation } from "@/chat/skills"; -import type { ActiveMcpCatalogSummary } from "@/chat/tools/skill/mcp-tool-summary"; +import type { + ActiveMcpCatalogSummary, + AvailableMcpProviderSummary, +} from "@/chat/tools/skill/mcp-tool-summary"; import { escapeXml } from "@/chat/xml"; const DEFAULT_SOUL = "You are Junior, a practical and concise assistant."; @@ -252,6 +255,25 @@ function formatActiveMcpCatalogsForPrompt( return lines.join("\n"); } +function formatAvailableMcpProvidersForPrompt( + providers: AvailableMcpProviderSummary[], +): string | null { + const inactive = providers.filter((p) => !p.active); + if (inactive.length === 0) { + return null; + } + const lines = [ + "These MCP providers are configured but not yet connected. Supply the provider name to `searchMcpTools` to connect and list its tools.", + ]; + for (const p of inactive) { + lines.push(" "); + lines.push(` ${escapeXml(p.provider)}`); + lines.push(` ${escapeXml(p.description)}`); + lines.push(" "); + } + return lines.join("\n"); +} + interface ToolPromptContext { name: string; promptGuidelines?: string[]; @@ -538,6 +560,7 @@ function buildCapabilitiesSection(params: { availableSkills: SkillMetadata[]; activeSkills: Skill[]; activeMcpCatalogs: ActiveMcpCatalogSummary[]; + availableMcpProviders: AvailableMcpProviderSummary[]; invocation: SkillInvocation | null; toolGuidance?: ToolPromptContext[]; }): string | null { @@ -562,6 +585,13 @@ function buildCapabilitiesSection(params: { blocks.push(renderTagBlock("active-mcp-catalogs", activeCatalogs)); } + const availableProviders = formatAvailableMcpProvidersForPrompt( + params.availableMcpProviders, + ); + if (availableProviders) { + blocks.push(renderTagBlock("available-mcp-providers", availableProviders)); + } + const toolGuidance = formatToolGuidanceForPrompt(params.toolGuidance ?? []); if (toolGuidance) { blocks.push(renderTagBlock("tool-guidance", toolGuidance)); @@ -578,6 +608,7 @@ type TurnContextPromptInput = { availableSkills: SkillMetadata[]; activeSkills: Skill[]; activeMcpCatalogs?: ActiveMcpCatalogSummary[]; + availableMcpProviders?: AvailableMcpProviderSummary[]; toolGuidance?: ToolPromptContext[]; runtime?: { conversationId?: string; @@ -628,6 +659,7 @@ export function buildTurnContextPrompt(params: TurnContextPromptInput): string { availableSkills: params.availableSkills, activeSkills: params.activeSkills, activeMcpCatalogs: params.activeMcpCatalogs ?? [], + availableMcpProviders: params.availableMcpProviders ?? [], invocation: params.invocation, toolGuidance: params.toolGuidance ?? [], }), diff --git a/packages/junior/src/chat/respond.ts b/packages/junior/src/chat/respond.ts index 85549f71..e9587169 100644 --- a/packages/junior/src/chat/respond.ts +++ b/packages/junior/src/chat/respond.ts @@ -345,6 +345,7 @@ export async function generateAssistantReply( let lastKnownSandboxDependencyProfileHash: string | undefined = context.sandbox?.sandboxDependencyProfileHash; let loadedSkillNamesForResume: string[] = []; + let activeMcpProviderNamesForResume: string[] = []; let mcpToolManager: McpToolManager | undefined; let sandboxExecutor: SandboxExecutor | undefined; let timedOut = false; @@ -682,6 +683,8 @@ export async function generateAssistantReply( pluginAuth.getPendingPause() ?? mcpAuth.getPendingPause(); const syncResumeState = () => { loadedSkillNamesForResume = activeSkills.map((skill) => skill.name); + activeMcpProviderNamesForResume = + turnMcpToolManager.getActiveProviderNames(); }; setTags({ conversationId: spanContext.conversationId, @@ -747,12 +750,9 @@ export async function generateAssistantReply( ) { return undefined; } - const availableToolCount = turnMcpToolManager.getActiveToolCatalog( - activeSkills, - { - provider: effective.pluginProvider, - }, - ).length; + const availableToolCount = turnMcpToolManager.getActiveToolCatalog({ + provider: effective.pluginProvider, + }).length; return { mcp_provider: effective.pluginProvider, available_tool_count: availableToolCount, @@ -769,8 +769,8 @@ export async function generateAssistantReply( userText: userInput, artifactState: context.artifactState, configuration: configurationValues, - getActiveSkills: () => activeSkills, mcpToolManager: turnMcpToolManager, + onProviderActivated: syncResumeState, sandbox, advisor: { config: botConfig.advisor, @@ -790,7 +790,18 @@ export async function generateAssistantReply( promptSnippet: definition.promptSnippet, })); - syncResumeState(); + // ── MCP provider activation ────────────────────────────────────── + // Restore previously activated MCP providers from the checkpoint so + // providers connected in a prior turn are available immediately. + for (const provider of existingCheckpoint?.activeMcpProviderNames ?? []) { + await turnMcpToolManager.activateProvider(provider); + syncResumeState(); + if (mcpAuth.getPendingPause()) { + timeoutResumeMessages = existingCheckpoint?.piMessages ?? []; + throw mcpAuth.getPendingPause()!; + } + } + // Activate MCP for checkpoint-preloaded skills (backward compat with loadSkill). for (const skill of activeSkills) { await turnMcpToolManager.activateForSkill(skill); syncResumeState(); @@ -803,13 +814,16 @@ export async function generateAssistantReply( // ── Prompt context ─────────────────────────────────────────────── const activeMcpCatalogs = toActiveMcpCatalogSummaries( - turnMcpToolManager.getActiveToolCatalog(activeSkills), + turnMcpToolManager.getActiveToolCatalog(), ); + const availableMcpProviders = + turnMcpToolManager.getAvailableProviderCatalog(); baseInstructions = buildSystemPrompt(); const turnContextPrompt = buildTurnContextPrompt({ availableSkills, activeSkills, activeMcpCatalogs, + availableMcpProviders, toolGuidance, runtime: { conversationId: spanContext.conversationId, @@ -912,6 +926,7 @@ export async function generateAssistantReply( sliceId: currentSliceId, messages, loadedSkillNames: loadedSkillNamesForResume, + activeMcpProviderNames: activeMcpProviderNamesForResume, logContext: checkpointLogContext, }); }; @@ -1131,6 +1146,8 @@ export async function generateAssistantReply( sliceId: currentSliceId, allMessages: agent.state.messages, loadedSkillNames: activeSkills.map((skill) => skill.name), + activeMcpProviderNames: + turnMcpToolManager?.getActiveProviderNames() ?? [], logContext: checkpointLogContext, }); } @@ -1167,6 +1184,7 @@ export async function generateAssistantReply( currentUsage: turnUsage, messages: timeoutResumeMessages, loadedSkillNames: loadedSkillNamesForResume, + activeMcpProviderNames: activeMcpProviderNamesForResume, errorMessage: error instanceof Error ? error.message : String(error), logContext: checkpointLogContext, }); @@ -1204,6 +1222,7 @@ export async function generateAssistantReply( currentUsage: turnUsage, messages: timeoutResumeMessages, loadedSkillNames: loadedSkillNamesForResume, + activeMcpProviderNames: activeMcpProviderNamesForResume, errorMessage: error.message, logContext: checkpointLogContext, }); diff --git a/packages/junior/src/chat/services/turn-checkpoint.ts b/packages/junior/src/chat/services/turn-checkpoint.ts index 236d25d6..a1b2d122 100644 --- a/packages/junior/src/chat/services/turn-checkpoint.ts +++ b/packages/junior/src/chat/services/turn-checkpoint.ts @@ -112,6 +112,7 @@ export async function persistRunningCheckpoint(args: { sliceId: number; messages: PiMessage[]; loadedSkillNames: string[]; + activeMcpProviderNames: string[]; logContext: CheckpointLogContext; }): Promise { if (args.messages.length === 0 || !isContinuableBoundary(args.messages)) { @@ -132,6 +133,7 @@ export async function persistRunningCheckpoint(args: { state: "running", piMessages: args.messages, loadedSkillNames: args.loadedSkillNames, + activeMcpProviderNames: args.activeMcpProviderNames, }); } catch (checkpointError) { logCheckpointError( @@ -155,6 +157,7 @@ export async function persistCompletedCheckpoint(args: { sliceId: number; allMessages: PiMessage[]; loadedSkillNames: string[]; + activeMcpProviderNames: string[]; logContext: CheckpointLogContext; }): Promise { try { @@ -177,6 +180,7 @@ export async function persistCompletedCheckpoint(args: { state: "completed", piMessages: args.allMessages, loadedSkillNames: args.loadedSkillNames, + activeMcpProviderNames: args.activeMcpProviderNames, }); } catch (checkpointError) { logCheckpointError( @@ -203,6 +207,7 @@ export async function persistAuthPauseCheckpoint(args: { currentUsage?: AgentTurnUsage; messages: PiMessage[]; loadedSkillNames: string[]; + activeMcpProviderNames: string[]; errorMessage: string; logContext: CheckpointLogContext; }): Promise { @@ -232,6 +237,7 @@ export async function persistAuthPauseCheckpoint(args: { state: "awaiting_resume", piMessages, loadedSkillNames: args.loadedSkillNames, + activeMcpProviderNames: args.activeMcpProviderNames, resumeReason: "auth", resumedFromSliceId: args.currentSliceId, errorMessage: args.errorMessage, @@ -263,6 +269,7 @@ export async function persistTimeoutCheckpoint(args: { currentUsage?: AgentTurnUsage; messages: PiMessage[]; loadedSkillNames: string[]; + activeMcpProviderNames: string[]; errorMessage: string; logContext: CheckpointLogContext; }): Promise { @@ -293,6 +300,7 @@ export async function persistTimeoutCheckpoint(args: { state: "awaiting_resume", piMessages, loadedSkillNames: args.loadedSkillNames, + activeMcpProviderNames: args.activeMcpProviderNames, resumeReason: "timeout", resumedFromSliceId: args.currentSliceId, errorMessage: args.errorMessage, diff --git a/packages/junior/src/chat/state/turn-session-store.ts b/packages/junior/src/chat/state/turn-session-store.ts index e853d688..5be94445 100644 --- a/packages/junior/src/chat/state/turn-session-store.ts +++ b/packages/junior/src/chat/state/turn-session-store.ts @@ -27,6 +27,7 @@ export interface AgentTurnSessionCheckpoint { cumulativeUsage?: AgentTurnUsage; errorMessage?: string; loadedSkillNames?: string[]; + activeMcpProviderNames?: string[]; piMessages: PiMessage[]; resumeReason?: AgentTurnResumeReason; resumedFromSliceId?: number; @@ -162,6 +163,13 @@ function parseAgentTurnSessionRecord(value: unknown): ), } : {}), + ...(Array.isArray(parsed.activeMcpProviderNames) + ? { + activeMcpProviderNames: parsed.activeMcpProviderNames.filter( + (value): value is string => typeof value === "string", + ), + } + : {}), ...(parsed.resumeReason === "timeout" || parsed.resumeReason === "auth" ? { resumeReason: parsed.resumeReason } : {}), @@ -237,6 +245,7 @@ export async function upsertAgentTurnSessionCheckpoint(args: { state: AgentTurnSessionStatus; piMessages: PiMessage[]; loadedSkillNames?: string[]; + activeMcpProviderNames?: string[]; resumeReason?: AgentTurnResumeReason; errorMessage?: string; resumedFromSliceId?: number; @@ -283,6 +292,13 @@ export async function upsertAgentTurnSessionCheckpoint(args: { ), } : {}), + ...(Array.isArray(args.activeMcpProviderNames) + ? { + activeMcpProviderNames: args.activeMcpProviderNames.filter( + (value): value is string => typeof value === "string", + ), + } + : {}), ...(args.resumeReason ? { resumeReason: args.resumeReason } : {}), ...(args.errorMessage ? { errorMessage: args.errorMessage } : {}), ...(typeof args.resumedFromSliceId === "number" @@ -329,6 +345,7 @@ export async function supersedeAgentTurnSessionCheckpoint(args: { cumulativeDurationMs: existing.cumulativeDurationMs, cumulativeUsage: existing.cumulativeUsage, loadedSkillNames: existing.loadedSkillNames, + activeMcpProviderNames: existing.activeMcpProviderNames, resumeReason: existing.resumeReason, resumedFromSliceId: existing.resumedFromSliceId, errorMessage: args.errorMessage ?? existing.errorMessage, @@ -366,6 +383,7 @@ export async function failAgentTurnSessionCheckpoint(args: { cumulativeDurationMs: existing.cumulativeDurationMs, cumulativeUsage: existing.cumulativeUsage, loadedSkillNames: existing.loadedSkillNames, + activeMcpProviderNames: existing.activeMcpProviderNames, resumeReason: existing.resumeReason, resumedFromSliceId: existing.resumedFromSliceId, errorMessage: args.errorMessage ?? existing.errorMessage, diff --git a/packages/junior/src/chat/tools/index.ts b/packages/junior/src/chat/tools/index.ts index 25aea6c9..579a0894 100644 --- a/packages/junior/src/chat/tools/index.ts +++ b/packages/junior/src/chat/tools/index.ts @@ -120,14 +120,14 @@ export function createTools( tools.advisor = createAdvisorTool(context.advisor); } - if (context.mcpToolManager && context.getActiveSkills) { + if (context.mcpToolManager) { tools.searchMcpTools = createSearchMcpToolsTool( context.mcpToolManager, - context.getActiveSkills, + context.onProviderActivated, ); tools.callMcpTool = createCallMcpToolTool( context.mcpToolManager, - context.getActiveSkills, + context.onProviderActivated, ); } diff --git a/packages/junior/src/chat/tools/skill/call-mcp-tool.ts b/packages/junior/src/chat/tools/skill/call-mcp-tool.ts index ea51bbed..1c0f8b62 100644 --- a/packages/junior/src/chat/tools/skill/call-mcp-tool.ts +++ b/packages/junior/src/chat/tools/skill/call-mcp-tool.ts @@ -1,8 +1,13 @@ import { Type } from "@sinclair/typebox"; import type { McpToolManager } from "@/chat/mcp/tool-manager"; -import type { Skill } from "@/chat/skills"; import { tool } from "@/chat/tools/definition"; +/** Extract provider name from canonical MCP tool name (mcp____). */ +function parseMcpProviderFromToolName(toolName: string): string | undefined { + const match = /^mcp__([^_][^_]*)__/.exec(toolName); + return match?.[1]; +} + function resolveMcpArguments( input: Record, ): Record { @@ -32,11 +37,11 @@ function resolveMcpArguments( /** Create the stable dispatcher for active MCP provider tools. */ export function createCallMcpToolTool( mcpToolManager: McpToolManager, - getActiveSkills: () => Skill[], + onProviderActivated?: () => void, ) { return tool({ description: - "Call an active MCP tool by exact tool_name. Use loadSkill to activate the provider, then searchMcpTools to discover tool names and schemas; copy required provider fields into arguments. Do not call with only tool_name unless the discovered tool has no arguments. Authorization is handled by the runtime when required.", + "Call an active MCP tool by exact tool_name. Use searchMcpTools to discover tool names and schemas; copy required provider fields into arguments. Do not call with only tool_name unless the discovered tool has no arguments. Authorization is handled by the runtime when required.", inputSchema: Type.Object( { tool_name: Type.String({ @@ -54,8 +59,13 @@ export function createCallMcpToolTool( ), execute: async (input) => { const { tool_name } = input; + const provider = parseMcpProviderFromToolName(tool_name); + if (provider && mcpToolManager.hasConfiguredProvider(provider)) { + await mcpToolManager.activateProvider(provider); + onProviderActivated?.(); + } const mcpTool = mcpToolManager - .getResolvedActiveTools(getActiveSkills()) + .getResolvedActiveTools() .find((candidate) => candidate.name === tool_name); if (!mcpTool) { throw new Error(`MCP tool is not active for this turn: ${tool_name}`); diff --git a/packages/junior/src/chat/tools/skill/mcp-tool-summary.ts b/packages/junior/src/chat/tools/skill/mcp-tool-summary.ts index cc2b8246..0c4db00c 100644 --- a/packages/junior/src/chat/tools/skill/mcp-tool-summary.ts +++ b/packages/junior/src/chat/tools/skill/mcp-tool-summary.ts @@ -17,6 +17,12 @@ export interface ExposedToolSummary { annotations?: Record; } +export interface AvailableMcpProviderSummary { + provider: string; + description: string; + active: boolean; +} + export interface ActiveMcpCatalogSummary { provider: string; available_tool_count: number; diff --git a/packages/junior/src/chat/tools/skill/search-mcp-tools.ts b/packages/junior/src/chat/tools/skill/search-mcp-tools.ts index 18646f75..01ae0b9e 100644 --- a/packages/junior/src/chat/tools/skill/search-mcp-tools.ts +++ b/packages/junior/src/chat/tools/skill/search-mcp-tools.ts @@ -3,7 +3,6 @@ import type { ManagedMcpToolDescriptor, McpToolManager, } from "@/chat/mcp/tool-manager"; -import type { Skill } from "@/chat/skills"; import { tool } from "@/chat/tools/definition"; import { toExposedToolSummary } from "@/chat/tools/skill/mcp-tool-summary"; @@ -111,11 +110,11 @@ function searchMcpCatalog( /** Create the progressive MCP catalog search tool used before callMcpTool. */ export function createSearchMcpToolsTool( mcpToolManager: McpToolManager, - getActiveSkills: () => Skill[], + onProviderActivated?: () => void, ) { return tool({ description: - "List or search active MCP tools and return full descriptors, including input/output schemas and annotations. Use after loadSkill when choosing a provider tool or when callMcpTool arguments are unclear.", + "List or search active MCP tools and return full descriptors, including input/output schemas and annotations. When provider is supplied and not yet active, Junior connects to it on demand. Use when choosing a provider tool or when callMcpTool arguments are unclear.", inputSchema: Type.Object( { query: Type.Optional( @@ -128,7 +127,8 @@ export function createSearchMcpToolsTool( provider: Type.Optional( Type.String({ minLength: 1, - description: "Optional provider name to list or search within.", + description: + "Optional provider name to list or search within. If configured but not yet connected, Junior activates it on demand.", }), ), max_results: Type.Optional( @@ -142,8 +142,11 @@ export function createSearchMcpToolsTool( { additionalProperties: false }, ), execute: async ({ query, provider, max_results }) => { + if (provider && mcpToolManager.hasConfiguredProvider(provider)) { + await mcpToolManager.activateProvider(provider); + onProviderActivated?.(); + } const catalog = mcpToolManager.getActiveToolCatalog( - getActiveSkills(), provider ? { provider } : {}, ); const maxResults = max_results ?? DEFAULT_MAX_RESULTS; diff --git a/packages/junior/src/chat/tools/types.ts b/packages/junior/src/chat/tools/types.ts index e9aed3be..5796732c 100644 --- a/packages/junior/src/chat/tools/types.ts +++ b/packages/junior/src/chat/tools/types.ts @@ -57,8 +57,9 @@ export interface ToolRuntimeContext { userText?: string; artifactState?: ThreadArtifactsState; configuration?: Record; - getActiveSkills?: () => Skill[]; mcpToolManager?: McpToolManager; + /** Called after a provider is lazily activated so the turn checkpoint captures the new state. */ + onProviderActivated?: () => void; sandbox: SandboxWorkspace; } diff --git a/packages/junior/tests/unit/mcp/tool-manager.test.ts b/packages/junior/tests/unit/mcp/tool-manager.test.ts index 9cc07ec1..e02c7465 100644 --- a/packages/junior/tests/unit/mcp/tool-manager.test.ts +++ b/packages/junior/tests/unit/mcp/tool-manager.test.ts @@ -126,7 +126,6 @@ describe("McpToolManager", () => { it("activates plugin-scoped MCP tools once with collision-safe names", async () => { const plugin = buildPlugin(); const manager = new McpToolManager([plugin]); - const activeSkills = [{ name: "demo-skill", pluginProvider: "demo" }]; expect( await manager.activateForSkill({ @@ -134,17 +133,22 @@ describe("McpToolManager", () => { pluginProvider: undefined, }), ).toBe(false); - expect(await manager.activateForSkill(activeSkills[0]!)).toBe(true); + expect( + await manager.activateForSkill({ + name: "demo-skill", + pluginProvider: "demo", + }), + ).toBe(true); expect(await manager.activateProvider("demo")).toBe(false); expect(manager.getActiveProviders()).toEqual(["demo"]); - const tools = manager.getActiveToolCatalog(activeSkills); + const tools = manager.getActiveToolCatalog(); expect(tools).toHaveLength(1); expect(tools[0]?.name).toBe("mcp__demo__ping"); expect(tools[0]?.rawName).toBe("ping"); expect(tools[0]?.description).toBe("[demo] Ping the remote MCP server"); - const resolvedTools = manager.getResolvedActiveTools(activeSkills); + const resolvedTools = manager.getResolvedActiveTools(); expect(resolvedTools).toHaveLength(1); const result = await resolvedTools[0]!.execute({ query: "hello" }); @@ -168,16 +172,15 @@ describe("McpToolManager", () => { expect(clientOptions).not.toContainEqual( expect.objectContaining({ sessionId: expect.any(String) }), ); - expect(manager.getActiveToolCatalog(activeSkills)).toEqual([]); + expect(manager.getActiveToolCatalog()).toEqual([]); }); it("annotates MCP tool spans with the MCP method name", async () => { const plugin = buildPlugin(); const manager = new McpToolManager([plugin]); - const activeSkills = [{ name: "demo-skill", pluginProvider: "demo" }]; await manager.activateProvider("demo"); - const resolvedTools = manager.getResolvedActiveTools(activeSkills); + const resolvedTools = manager.getResolvedActiveTools(); await expect( resolvedTools[0]!.execute({ query: "hello" }), ).resolves.toMatchObject({ @@ -195,7 +198,6 @@ describe("McpToolManager", () => { it("logs expected MCP tool errors with semantic context", async () => { const plugin = buildPlugin(); const manager = new McpToolManager([plugin]); - const activeSkills = [{ name: "demo-skill", pluginProvider: "demo" }]; await manager.activateProvider("demo"); callToolMock.mockResolvedValueOnce({ content: [ @@ -207,7 +209,7 @@ describe("McpToolManager", () => { isError: true, }); - const resolvedTools = manager.getResolvedActiveTools(activeSkills); + const resolvedTools = manager.getResolvedActiveTools(); await expect(resolvedTools[0]!.execute({})).rejects.toThrow( "expected object, received undefined", ); @@ -232,13 +234,12 @@ describe("McpToolManager", () => { const manager = new McpToolManager([plugin], { onAuthorizationRequired: onAuthorizationRequiredMock, }); - const activeSkills = [{ name: "demo-skill", pluginProvider: "demo" }]; await manager.activateProvider("demo"); callToolMock.mockRejectedValueOnce( new McpAuthorizationRequiredError("demo", "Auth required"), ); - const resolvedTools = manager.getResolvedActiveTools(activeSkills); + const resolvedTools = manager.getResolvedActiveTools(); await expect(resolvedTools[0]!.execute({})).rejects.toBeInstanceOf( McpAuthorizationRequiredError, ); @@ -258,13 +259,12 @@ describe("McpToolManager", () => { const manager = new McpToolManager([plugin], { onAuthorizationRequired: onAuthorizationRequiredMock, }); - const activeSkills = [{ name: "demo-skill", pluginProvider: "demo" }]; await manager.activateProvider("demo"); callToolMock.mockRejectedValueOnce( new McpAuthorizationRequiredError("demo", "Auth required"), ); - const resolvedTools = manager.getResolvedActiveTools(activeSkills); + const resolvedTools = manager.getResolvedActiveTools(); await expect(resolvedTools[0]!.execute({})).resolves.toEqual({ content: [{ type: "text", text: "Authorization pending." }], details: { @@ -372,12 +372,7 @@ describe("McpToolManager", () => { expect(closeMock).toHaveBeenNthCalledWith(1, alphaPlugin); expect(closeMock).toHaveBeenNthCalledWith(2, betaPlugin); expect(manager.getActiveProviders()).toEqual([]); - expect( - manager.getActiveToolCatalog([ - { pluginProvider: "alpha" }, - { pluginProvider: "beta" }, - ]), - ).toEqual([]); + expect(manager.getActiveToolCatalog()).toEqual([]); }); it("filters MCP tools to the provider allowlist", async () => { @@ -408,14 +403,13 @@ describe("McpToolManager", () => { const manager = new McpToolManager([plugin]); await manager.activateProvider("notion"); - expect( - manager - .getActiveToolCatalog([{ pluginProvider: "notion" }]) - .map((tool) => tool.name), - ).toEqual(["mcp__notion__notion-search", "mcp__notion__notion-fetch"]); + expect(manager.getActiveToolCatalog().map((tool) => tool.name)).toEqual([ + "mcp__notion__notion-search", + "mcp__notion__notion-fetch", + ]); }); - it("exposes the provider tool catalog once a plugin skill is active", async () => { + it("exposes the provider tool catalog once a provider is active, without requiring a skill", async () => { const plugin = buildPlugin("notion"); listToolsMock.mockResolvedValue([ { @@ -439,24 +433,15 @@ describe("McpToolManager", () => { ]); const manager = new McpToolManager([plugin]); - const activeSkills = [ - { - name: "notion", - pluginProvider: "notion", - }, - ]; - - await manager.activateForSkill(activeSkills[0]!); + await manager.activateProvider("notion"); - expect( - manager.getActiveToolCatalog(activeSkills).map((tool) => tool.name), - ).toEqual([ + expect(manager.getActiveToolCatalog().map((tool) => tool.name)).toEqual([ "mcp__notion__notion-search", "mcp__notion__notion-fetch", "mcp__notion__notion-create-pages", ]); const createPagesTool = manager - .getResolvedActiveTools(activeSkills) + .getResolvedActiveTools() .find((t) => t.name === "mcp__notion__notion-create-pages"); await expect(createPagesTool!.execute({})).resolves.toMatchObject({ details: { @@ -466,6 +451,23 @@ describe("McpToolManager", () => { }); }); + it("getAvailableProviderCatalog returns all configured providers without connecting", async () => { + const notionPlugin = buildPlugin("notion"); + const linearPlugin = buildPlugin("linear"); + const manager = new McpToolManager([notionPlugin, linearPlugin]); + + const catalog = manager.getAvailableProviderCatalog(); + expect(catalog).toHaveLength(2); + expect(catalog.map((p) => p.provider)).toEqual(["linear", "notion"]); + expect(catalog.every((p) => !p.active)).toBe(true); + expect(listToolsMock).not.toHaveBeenCalled(); + + await manager.activateProvider("notion"); + const after = manager.getAvailableProviderCatalog(); + expect(after.find((p) => p.provider === "notion")?.active).toBe(true); + expect(after.find((p) => p.provider === "linear")?.active).toBe(false); + }); + it("fails activation when an allowlisted MCP tool is missing", async () => { const plugin = buildPlugin("notion", { allowedTools: ["notion-search", "notion-fetch"], diff --git a/packages/junior/tests/unit/services/turn-checkpoint.test.ts b/packages/junior/tests/unit/services/turn-checkpoint.test.ts index 37574dd2..ca4f0d3f 100644 --- a/packages/junior/tests/unit/services/turn-checkpoint.test.ts +++ b/packages/junior/tests/unit/services/turn-checkpoint.test.ts @@ -77,6 +77,7 @@ describe("persistAuthPauseCheckpoint", () => { currentSliceId: 1, messages: [], loadedSkillNames: ["demo-skill"], + activeMcpProviderNames: [], errorMessage: "plugin auth pause", logContext: { modelId: "test-model", @@ -130,6 +131,7 @@ describe("persistAuthPauseCheckpoint", () => { cachedInputTokens: 2, }, messages: [], + activeMcpProviderNames: [], loadedSkillNames: [], errorMessage: "timed out again", logContext: { @@ -170,6 +172,7 @@ describe("persistAuthPauseCheckpoint", () => { conversationId: "conversation-1", sessionId: "turn-1", sliceId: 1, + activeMcpProviderNames: [], allMessages: [ { role: "user", @@ -242,6 +245,7 @@ describe("persistAuthPauseCheckpoint", () => { sliceId: 1, messages: userBoundary, loadedSkillNames: [], + activeMcpProviderNames: [], logContext: { modelId: "test-model", }, @@ -253,6 +257,7 @@ describe("persistAuthPauseCheckpoint", () => { sliceId: 1, messages: unsafeAssistantBoundary, loadedSkillNames: [], + activeMcpProviderNames: [], logContext: { modelId: "test-model", }, @@ -273,6 +278,7 @@ describe("persistAuthPauseCheckpoint", () => { sliceId: 1, messages: toolResultBoundary, loadedSkillNames: ["demo-skill"], + activeMcpProviderNames: [], logContext: { modelId: "test-model", }, @@ -308,6 +314,7 @@ describe("persistAuthPauseCheckpoint", () => { sliceId: 1, messages, loadedSkillNames: ["demo-skill"], + activeMcpProviderNames: [], logContext: { modelId: "test-model", }, @@ -319,6 +326,7 @@ describe("persistAuthPauseCheckpoint", () => { currentSliceId: 1, messages: [], loadedSkillNames: ["demo-skill"], + activeMcpProviderNames: [], errorMessage: "provider stream interrupted", logContext: { modelId: "test-model", diff --git a/packages/junior/tests/unit/tools/call-mcp-tool.test.ts b/packages/junior/tests/unit/tools/call-mcp-tool.test.ts index 4794601a..1ccbcdfe 100644 --- a/packages/junior/tests/unit/tools/call-mcp-tool.test.ts +++ b/packages/junior/tests/unit/tools/call-mcp-tool.test.ts @@ -1,16 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import type { McpToolManager } from "@/chat/mcp/tool-manager"; -import type { Skill } from "@/chat/skills"; import { createCallMcpToolTool } from "@/chat/tools/skill/call-mcp-tool"; -const activeSkill: Skill = { - name: "demo", - description: "Demo skill", - skillPath: "/tmp/demo", - pluginProvider: "demo", - body: "instructions", -}; - describe("callMcpTool", () => { it("executes an active MCP tool by disclosed tool_name", async () => { const execute = vi.fn(async () => ({ @@ -18,6 +9,8 @@ describe("callMcpTool", () => { details: { ok: true }, })); const manager = { + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), getResolvedActiveTools: vi.fn(() => [ { name: "mcp__demo__ping", @@ -29,7 +22,7 @@ describe("callMcpTool", () => { }, ]), } as unknown as McpToolManager; - const callMcpTool = createCallMcpToolTool(manager, () => [activeSkill]); + const callMcpTool = createCallMcpToolTool(manager); await expect( callMcpTool.execute!( @@ -48,6 +41,8 @@ describe("callMcpTool", () => { it("rejects top-level MCP arguments instead of silently dropping them", async () => { const manager = { + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), getResolvedActiveTools: vi.fn(() => [ { name: "mcp__demo__ping", @@ -59,7 +54,7 @@ describe("callMcpTool", () => { }, ]), } as unknown as McpToolManager; - const callMcpTool = createCallMcpToolTool(manager, () => [activeSkill]); + const callMcpTool = createCallMcpToolTool(manager); await expect( callMcpTool.execute!( @@ -80,6 +75,8 @@ describe("callMcpTool", () => { details: { ok: true }, })); const manager = { + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), getResolvedActiveTools: vi.fn(() => [ { name: "mcp__demo__ping", @@ -91,7 +88,7 @@ describe("callMcpTool", () => { }, ]), } as unknown as McpToolManager; - const callMcpTool = createCallMcpToolTool(manager, () => [activeSkill]); + const callMcpTool = createCallMcpToolTool(manager); await expect( callMcpTool.execute!( @@ -110,6 +107,8 @@ describe("callMcpTool", () => { it("rejects non-object nested MCP arguments", async () => { const manager = { + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), getResolvedActiveTools: vi.fn(() => [ { name: "mcp__demo__ping", @@ -121,7 +120,7 @@ describe("callMcpTool", () => { }, ]), } as unknown as McpToolManager; - const callMcpTool = createCallMcpToolTool(manager, () => [activeSkill]); + const callMcpTool = createCallMcpToolTool(manager); await expect( callMcpTool.execute!( @@ -136,9 +135,11 @@ describe("callMcpTool", () => { it("rejects tools that are not active for the turn", async () => { const manager = { + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), getResolvedActiveTools: vi.fn(() => []), } as unknown as McpToolManager; - const callMcpTool = createCallMcpToolTool(manager, () => [activeSkill]); + const callMcpTool = createCallMcpToolTool(manager); await expect( callMcpTool.execute!( diff --git a/packages/junior/tests/unit/tools/search-mcp-tools.test.ts b/packages/junior/tests/unit/tools/search-mcp-tools.test.ts index d61c3c54..fc6be537 100644 --- a/packages/junior/tests/unit/tools/search-mcp-tools.test.ts +++ b/packages/junior/tests/unit/tools/search-mcp-tools.test.ts @@ -1,66 +1,55 @@ import { describe, expect, it, vi } from "vitest"; import type { McpToolManager } from "@/chat/mcp/tool-manager"; -import type { Skill } from "@/chat/skills"; import { createSearchMcpToolsTool } from "@/chat/tools/skill/search-mcp-tools"; -const activeSkill: Skill = { - name: "demo", - description: "Demo skill", - skillPath: "/tmp/demo", - pluginProvider: "demo", - body: "instructions", -}; - describe("searchMcpTools", () => { function buildManager() { return { - getActiveToolCatalog: vi.fn( - (_skills: Skill[], _options?: { provider?: string }) => [ - { - name: "mcp__demo__create_issue", - rawName: "create_issue", - provider: "demo", - description: "Create an issue", - parameters: { - type: "object", - properties: { - title: { type: "string", description: "Issue title" }, - labels: { - type: "array", - items: { type: "string" }, - description: "Issue labels", - }, - metadata: { - type: "object", - description: "Issue metadata", - }, + hasConfiguredProvider: vi.fn(() => false), + activateProvider: vi.fn(async () => true), + getActiveToolCatalog: vi.fn((_options?: { provider?: string }) => [ + { + name: "mcp__demo__create_issue", + rawName: "create_issue", + provider: "demo", + description: "Create an issue", + parameters: { + type: "object", + properties: { + title: { type: "string", description: "Issue title" }, + labels: { + type: "array", + items: { type: "string" }, + description: "Issue labels", + }, + metadata: { + type: "object", + description: "Issue metadata", }, - required: ["title"], - }, - outputSchema: { - type: "object", - properties: { id: { type: "string" } }, - required: ["id"], }, - annotations: { destructiveHint: true }, + required: ["title"], }, - { - name: "mcp__demo__list_projects", - rawName: "list_projects", - provider: "demo", - description: "List projects", - parameters: { type: "object", properties: {} }, + outputSchema: { + type: "object", + properties: { id: { type: "string" } }, + required: ["id"], }, - ], - ), + annotations: { destructiveHint: true }, + }, + { + name: "mcp__demo__list_projects", + rawName: "list_projects", + provider: "demo", + description: "List projects", + parameters: { type: "object", properties: {} }, + }, + ]), } as unknown as McpToolManager; } it("returns focused MCP descriptors with input and output schemas", async () => { const manager = buildManager(); - const searchMcpTools = createSearchMcpToolsTool(manager, () => [ - activeSkill, - ]); + const searchMcpTools = createSearchMcpToolsTool(manager); const result = (await searchMcpTools.execute!( { query: "issue title", max_results: 1 }, @@ -108,9 +97,7 @@ describe("searchMcpTools", () => { it("lists active provider tools without a query", async () => { const manager = buildManager(); - const searchMcpTools = createSearchMcpToolsTool(manager, () => [ - activeSkill, - ]); + const searchMcpTools = createSearchMcpToolsTool(manager); const result = (await searchMcpTools.execute!( { provider: "demo", max_results: 10 }, @@ -140,7 +127,7 @@ describe("searchMcpTools", () => { }, ], }); - expect(manager.getActiveToolCatalog).toHaveBeenCalledWith([activeSkill], { + expect(manager.getActiveToolCatalog).toHaveBeenCalledWith({ provider: "demo", }); }); diff --git a/specs/plugin-runtime.md b/specs/plugin-runtime.md index 0a81b101..39d8cbe8 100644 --- a/specs/plugin-runtime.md +++ b/specs/plugin-runtime.md @@ -75,11 +75,13 @@ Install-wide config defaults use `createApp({ configDefaults })` and must refere ## MCP Activation - MCP tools are not sandbox dependencies and are not registered globally at startup. -- Runtime activates a plugin's MCP tools only after a skill owned by that plugin is loaded in the current turn. -- Explicit `/skill` invocations preload the skill first. +- At turn setup, the runtime restores only providers that were active in the previous turn (from checkpoint). New providers connect lazily when the model first accesses them. +- Calling `searchMcpTools({ provider })` or `callMcpTool` for a configured but inactive provider triggers connection and `listTools` on demand, surfacing the auth flow if credentials are missing or expired. +- Explicit `/skill` invocations also activate the skill's associated MCP provider. - Remote MCP catalogs are authoritative only after connection and `listTools`. - Mid-turn `loadSkill` updates the host-managed MCP registry, but Junior does not mutate the Pi native tool list during the turn. -- Stable native tools `searchMcpTools` and `callMcpTool` search and execute active-provider MCP tools. +- `searchMcpTools` and `callMcpTool` search and execute active-provider tools. Both lazily connect a provider when given one that is configured but not active. +- The prompt `` block shows configured-but-inactive providers cheaply (name + description, no network). - `loadSkill` returns provider/count metadata, not full tool descriptors. - `searchMcpTools` returns focused descriptors including canonical `tool_name`, upstream `mcp_tool_name`, schemas, and annotations. - Preloaded and resumed skills disclose provider/count summaries in ``. @@ -126,8 +128,8 @@ Hook contexts expose narrow capabilities rather than raw Junior internals. Trust - Registry load order is deterministic. - Manifest validation fails before partial registration. - Plugin-backed skill loading rejects forged plugin metadata. -- MCP tools activate only after same-plugin skill load. -- `searchMcpTools` and `callMcpTool` cannot reach inactive provider tools. +- No MCP connections are made at turn start unless restoring checkpoint providers or activating skills. +- `searchMcpTools` and `callMcpTool` cannot reach tools from providers that are not configured or failed activation. ## Related Specs From 1c87b4d4a440ef1fb4a00d0c62583ff6d906fabb Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 19:13:03 +0000 Subject: [PATCH 2/2] ref(mcp): simplify provider name parser and drop redundant wrapper parseMcpProviderFromToolName: replace regex with indexOf('__') split. The regex failed for provider names containing a single underscore; the string approach is also more readable and explicit about the delimiter. getActiveProviderNames: remove the thin wrapper around getActiveProviders(). Both methods returned the same value; inlining the call site removes unnecessary public API surface. Co-authored-by: David Cramer --- packages/junior/src/chat/mcp/tool-manager.ts | 5 ----- packages/junior/src/chat/respond.ts | 4 ++-- packages/junior/src/chat/tools/skill/call-mcp-tool.ts | 9 ++++++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/junior/src/chat/mcp/tool-manager.ts b/packages/junior/src/chat/mcp/tool-manager.ts index 681b8417..f56ccc36 100644 --- a/packages/junior/src/chat/mcp/tool-manager.ts +++ b/packages/junior/src/chat/mcp/tool-manager.ts @@ -198,11 +198,6 @@ export class McpToolManager { ); } - /** Return all active provider names, for checkpoint persistence. */ - getActiveProviderNames(): string[] { - return this.getActiveProviders(); - } - /** Return all configured MCP providers with active/inactive state. * Never connects to any MCP server. */ getAvailableProviderCatalog(): Array<{ diff --git a/packages/junior/src/chat/respond.ts b/packages/junior/src/chat/respond.ts index e9587169..34306af7 100644 --- a/packages/junior/src/chat/respond.ts +++ b/packages/junior/src/chat/respond.ts @@ -684,7 +684,7 @@ export async function generateAssistantReply( const syncResumeState = () => { loadedSkillNamesForResume = activeSkills.map((skill) => skill.name); activeMcpProviderNamesForResume = - turnMcpToolManager.getActiveProviderNames(); + turnMcpToolManager.getActiveProviders(); }; setTags({ conversationId: spanContext.conversationId, @@ -1147,7 +1147,7 @@ export async function generateAssistantReply( allMessages: agent.state.messages, loadedSkillNames: activeSkills.map((skill) => skill.name), activeMcpProviderNames: - turnMcpToolManager?.getActiveProviderNames() ?? [], + turnMcpToolManager?.getActiveProviders() ?? [], logContext: checkpointLogContext, }); } diff --git a/packages/junior/src/chat/tools/skill/call-mcp-tool.ts b/packages/junior/src/chat/tools/skill/call-mcp-tool.ts index 1c0f8b62..a77fee17 100644 --- a/packages/junior/src/chat/tools/skill/call-mcp-tool.ts +++ b/packages/junior/src/chat/tools/skill/call-mcp-tool.ts @@ -2,10 +2,13 @@ import { Type } from "@sinclair/typebox"; import type { McpToolManager } from "@/chat/mcp/tool-manager"; import { tool } from "@/chat/tools/definition"; -/** Extract provider name from canonical MCP tool name (mcp____). */ +/** Extract the provider name from a canonical MCP tool name (mcp____). + * Uses the double-underscore delimiter; handles provider names that contain single underscores. */ function parseMcpProviderFromToolName(toolName: string): string | undefined { - const match = /^mcp__([^_][^_]*)__/.exec(toolName); - return match?.[1]; + if (!toolName.startsWith("mcp__")) return undefined; + const afterPrefix = toolName.slice("mcp__".length); + const delimIdx = afterPrefix.indexOf("__"); + return delimIdx > 0 ? afterPrefix.slice(0, delimIdx) : undefined; } function resolveMcpArguments(