From 4715a8bda550719983d7f43853bb50df95f5af6d Mon Sep 17 00:00:00 2001 From: 7Sageer <7sageer@djwcb.cn> Date: Wed, 19 Aug 2026 18:53:31 +0800 Subject: [PATCH] refactor(agent-core-v2): strip comments from the MCP management plane files --- .../src/app/mcpConfig/configLoader.ts | 27 ------ .../src/app/mcpConfig/configStore.ts | 17 ---- .../src/app/mcpConfig/oauthService.ts | 17 ---- .../src/app/mcpManagement/errors.ts | 4 - .../src/app/mcpManagement/flag.ts | 5 - .../src/app/mcpManagement/mcpManagement.ts | 24 ----- .../app/mcpManagement/mcpManagementService.ts | 71 -------------- .../src/app/mcpRegistry/mcpRegistry.ts | 19 ---- .../src/app/mcpRegistry/mcpRegistryService.ts | 21 ---- .../src/app/plugin/pluginService.ts | 4 - .../agent-core-v2/src/mcpCore/configView.ts | 10 -- .../src/mcpCore/oauth/provider.ts | 17 ---- .../src/mcpCore/oauth/service.ts | 55 ----------- .../workspaceMcp/workspaceMcpService.ts | 6 -- .../workspaceMcpConfigService.ts | 7 -- .../test/app/mcpConfig/configStore.test.ts | 11 --- .../app/mcpManagement/mcpManagement.test.ts | 65 ------------- .../test/app/mcpRegistry/mcpRegistry.test.ts | 20 ---- .../test/app/plugin/pluginService.test.ts | 2 - .../test/mcpCore/oauth/service.test.ts | 96 +------------------ .../workspaceInstanceManager.test.ts | 4 - .../workspaceMcp/workspaceMcp.test.ts | 38 -------- .../workspaceMcpConfig.test.ts | 2 - .../kap-server/src/protocol/error-codes.ts | 1 - packages/kap-server/src/routes/v2/mcp.ts | 75 --------------- packages/kap-server/test/v2Mcp.test.ts | 28 ------ 26 files changed, 2 insertions(+), 644 deletions(-) diff --git a/packages/agent-core-v2/src/app/mcpConfig/configLoader.ts b/packages/agent-core-v2/src/app/mcpConfig/configLoader.ts index 30aee0736..69a43ee2e 100644 --- a/packages/agent-core-v2/src/app/mcpConfig/configLoader.ts +++ b/packages/agent-core-v2/src/app/mcpConfig/configLoader.ts @@ -1,21 +1,3 @@ -/** - * `mcpConfig` domain — MCP JSON config discovery and loading. - * - * Resolves the three MCP config files for a cwd (user `mcp.json` under the - * kimi home, project-root `.mcp.json` — the root discovered through the - * `git` domain's work-tree probe — and `.kimi-code/mcp.json` under the cwd) - * and loads them with user < project-root < project precedence, normalizing - * relative stdio `cwd` entries against the project-root file's directory. - * `includeProject: false` skips the two project-level files and loads the - * user file only — the workspace-trust gate: the project files ship with - * the checkout, so an untrusted workspace must never see them. - * {@link loadMcpServersDetailed} additionally reports the defining-file - * origin of every effective entry, for management surfaces that show where - * a server came from. All filesystem access goes through the os - * `IHostFileSystem`, supplied by the caller. Pure functions — no scoped - * state. - */ - import { dirname, isAbsolute, join, normalize, resolve } from 'pathe'; import { resolveKimiHome } from '#/app/bootstrap/bootstrap'; @@ -90,8 +72,6 @@ export async function loadMcpServersDetailed( [paths.projectRoot, projectRoot], [paths.project, project], ]); - // Null-prototype accumulators: a server literally named `__proto__` would - // otherwise hit the prototype setter and silently vanish from the merge. const servers: Record = Object.create(null); const origins: Record = Object.create(null); for (const [path, layer] of layers) { @@ -154,13 +134,6 @@ async function readMcpJson( } } -/** - * Parse the file's server map entry-by-entry instead of through a single - * `z.record()`: a record parse rebuilds its output with property assignment, - * which routes a literal `__proto__` server key through the prototype setter - * and silently drops it. Per-entry parsing over the JSON own-keys keeps every - * declared server. - */ function parseMcpJsonServers(data: unknown): Record { if (!isRecord(data)) { throw new Error('expected a JSON object'); diff --git a/packages/agent-core-v2/src/app/mcpConfig/configStore.ts b/packages/agent-core-v2/src/app/mcpConfig/configStore.ts index bae9b0e20..97c19607e 100644 --- a/packages/agent-core-v2/src/app/mcpConfig/configStore.ts +++ b/packages/agent-core-v2/src/app/mcpConfig/configStore.ts @@ -1,20 +1,3 @@ -/** - * `mcpConfig` domain — `IMcpConfigStore`, the App-scope write plane for the - * user-level MCP server catalog. - * - * Owns the user `mcp.json` (`/mcp.json`): `list` / `get` reads and - * `add` / `update` / `remove` mutations, persisted as bytes through the - * `storage` filesystem byte store (`IFileSystemStorageService`) at the - * home-root scope (`''`) with atomic replacement. The on-disk format is a - * port of the v1 `GlobalMcpConfigStore` — two-space JSON with a trailing - * newline that preserves unknown top-level keys — so both engines emit - * byte-identical files; `path` (resolved through the bootstrap home - * resolution) is the origin identity shown by management surfaces, not the - * persistence locator. Server entries are validated one by one against the - * `mcpCore` `McpServerConfigSchema`, and every successful mutation fires - * `onDidWrite`. Bound at App scope. - */ - import { join } from 'pathe'; import { createDecorator, type ServiceIdentifier } from '#/_base/di/instantiation'; diff --git a/packages/agent-core-v2/src/app/mcpConfig/oauthService.ts b/packages/agent-core-v2/src/app/mcpConfig/oauthService.ts index ce933c80f..581159393 100644 --- a/packages/agent-core-v2/src/app/mcpConfig/oauthService.ts +++ b/packages/agent-core-v2/src/app/mcpConfig/oauthService.ts @@ -1,20 +1,3 @@ -/** - * `mcpConfig` domain — `IMcpOAuthService`, the App-scope shared MCP OAuth - * orchestrator. - * - * One process-wide `McpOAuthService` (the `mcpCore` mechanism class) over the - * shared `IMcpOAuthStore` credential persistence: every workspace handler and - * session overlay attaches its providers instead of building per-handler - * services, so credential events, single-flight refreshes, and proactive - * refresh timers are process-global and N handlers sharing one server cannot - * interfere. The constructor starts the proactive-refresh sweep from the - * persisted credential meta sidecars. The client name announced on OAuth - * dynamic registration is the identity snapshot's slug, consulted per - * provider so an identity configured after construction still applies. - * Disposing the App scope shuts the service down (timers, in-flight flows, - * providers). Bound at App scope. - */ - import { createDecorator, type ServiceIdentifier } from '#/_base/di/instantiation'; import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; import { ILogService } from '#/_base/log/log'; diff --git a/packages/agent-core-v2/src/app/mcpManagement/errors.ts b/packages/agent-core-v2/src/app/mcpManagement/errors.ts index f67bca268..903045d0c 100644 --- a/packages/agent-core-v2/src/app/mcpManagement/errors.ts +++ b/packages/agent-core-v2/src/app/mcpManagement/errors.ts @@ -1,7 +1,3 @@ -/** - * `mcpManagement` domain — error codes. - */ - import { registerErrorDomain, type ErrorDomain } from '#/_base/errors/codes'; export const McpManagementErrors = { diff --git a/packages/agent-core-v2/src/app/mcpManagement/flag.ts b/packages/agent-core-v2/src/app/mcpManagement/flag.ts index 8900055af..f355594a8 100644 --- a/packages/agent-core-v2/src/app/mcpManagement/flag.ts +++ b/packages/agent-core-v2/src/app/mcpManagement/flag.ts @@ -1,8 +1,3 @@ -/** - * `mcpManagement` domain — feature flag for the experimental MCP management - * plane. - */ - import { type FlagDefinitionInput, registerFlagDefinition } from '#/app/flag/flagRegistry'; export const mcpManagementFlag: FlagDefinitionInput = { diff --git a/packages/agent-core-v2/src/app/mcpManagement/mcpManagement.ts b/packages/agent-core-v2/src/app/mcpManagement/mcpManagement.ts index 514a45305..8cd80a28f 100644 --- a/packages/agent-core-v2/src/app/mcpManagement/mcpManagement.ts +++ b/packages/agent-core-v2/src/app/mcpManagement/mcpManagement.ts @@ -1,27 +1,3 @@ -/** - * `mcpManagement` domain — `IMcpManagementService` contract. - * - * The unified MCP management plane over the `mcpRegistry` read view: - * - * - Write plane: CRUD on the user-level `mcp.json` guarded by the registry - * (read-only plugin / project-layer entries reject mutations), plus a - * connection-test probe that accepts either an inline server config or a - * registry-resolved name. Mutations land in the user-level file only — - * live sessions pick them up through the store's change event and the - * workspace config watch. - * - Inspection: the locator-addressed catalog with redacted configs, a - * per-server auth-status surface (offline by default, `verify` probes), - * and a batched real-connection inspection; runtime names shared by - * enabled entries are reported `unavailable` instead of guessed. - * - OAuth: locator-addressed begin/complete/cancel/reset over the shared - * `mcpConfig` OAuth orchestrator, with flow handles keyed by flowId and - * ambiguity rejection for shared runtime names. - * - * The plane is unreleased: the edge exposure (server routes, client - * facades) gates on the `mcp_management` flag; the engine service itself - * stays ungated so in-process hosts can delegate to it. Bound at App scope. - */ - import { createDecorator, type ServiceIdentifier } from '#/_base/di/instantiation'; import type { McpServerConfig } from '#/mcpCore/config-schema'; diff --git a/packages/agent-core-v2/src/app/mcpManagement/mcpManagementService.ts b/packages/agent-core-v2/src/app/mcpManagement/mcpManagementService.ts index 7553dc58d..79ee7b936 100644 --- a/packages/agent-core-v2/src/app/mcpManagement/mcpManagementService.ts +++ b/packages/agent-core-v2/src/app/mcpManagement/mcpManagementService.ts @@ -1,30 +1,3 @@ -/** - * `mcpManagement` domain — `IMcpManagementService` implementation. - * - * Orchestrates the write plane: every mutation normalizes the server name - * once (the store trims names, so the read-only guard, the persisted key, - * and the workspace reconciliation must all agree), checks the `mcpRegistry` - * read view for a read-only collision (an enabled plugin entry or a - * project-layer entry rejects; a disabled plugin descriptor is a dead - * shadow and never blocks), then writes the user-level file through the - * `mcpConfig` store — its change event and the workspace config watch drive - * the live-session reconciliation from there, so this service holds no - * session knowledge. The connection test runs a throwaway - * `McpConnectionManager` probe against the shared `mcpConfig` OAuth - * orchestrator, feeding the manager the `[mcp]` section tunables from - * `config` and the client name from `identity`; probing a stdio server - * materializes the probe cwd's workspace through the runtime binding (the - * same path any out-of-workspace connect takes) — note this registers the - * cwd in the persisted workspace directory, an accepted side effect of - * testing an arbitrary stdio server. The - * inspection batches that probe over every OAuth candidate in one manager. - * Locator-addressed OAuth operations run through the shared orchestrator - * with flow handles tracked by flowId, and refuse to act on a runtime name - * shared by enabled entries — the credential identity would be ambiguous. - * Reads assemble the management view with read-only entries redacted to - * key lists. Bound at App scope. - */ - import { randomUUID } from 'node:crypto'; import { normalize } from 'pathe'; @@ -76,7 +49,6 @@ import { type McpServerTestTarget, } from './mcpManagement'; -/** Default wait for the browser callback of a management-plane OAuth flow. */ const DEFAULT_AUTH_TIMEOUT_MS = 15 * 60_000; export class McpManagementService extends Disposable implements IMcpManagementService { @@ -110,10 +82,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe const name = normalizeServerName(server.name); const existing = await this.guardLookup(name); if (existing !== undefined && !(existing.source === 'global' && existing.mutable)) { - // A same-named plugin / project-layer entry already exists; writing a - // user-level shadow would silently change precedence, so reject. A - // mutable user-level duplicate falls through to the store's own - // "already exists" error. throwReadOnlyMcpServer(existing); } await this.store.add({ ...server, name }); @@ -124,7 +92,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe const name = normalizeServerName(server.name); const existing = await this.guardLookup(name); if (existing === undefined) { - // Preserve the store's not-found error (and its config validation). await this.store.update({ ...server, name }); } else { throwReadOnlyMcpServer(existing); @@ -192,9 +159,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe 'Pass an MCP server name or an inline server config', ); } - // A name-only probe is only meaningful when one enabled entry owns the - // runtime name; under a collision the UI cannot tell which server Test - // acts on, so reject like the auth paths do. const matches = (await this.registry.list({ cwd })).filter((entry) => entry.name === name); if (matches.length === 0) { throw new Error2(ErrorCodes.MCP_SERVER_NOT_FOUND, `MCP server "${name}" was not found`); @@ -206,10 +170,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe `MCP runtime name "${name}" is shared by multiple enabled servers`, ); } - // Probe the entry the runtime would actually run: the sole enabled match - // owns the name (an enabled plugin outranks the file layers, which list - // first). When every match is disabled, fall back to the first entry so - // the probe reports it as disabled. const entry = enabled[0] ?? matches[0]!; return { name: entry.name, ...entry.config }; } @@ -248,7 +208,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe } } - async listAuthStatuses(query: McpAuthStatusQuery = {}): Promise { const entries = await this.registry.list({ cwd: query.cwd }); const verify = query.verify === true; @@ -273,13 +232,9 @@ export class McpManagementService extends Disposable implements IMcpManagementSe } async resolveServerByName(name: string): Promise { - // get() first, preserving its not-found error for unknown names. await this.registry.get(name); const catalog = await this.serverDescriptors(); const matches = catalog.filter((candidate) => candidate.runtimeName === name); - // The sole enabled owner wins over disabled shadows (matching the runtime - // and the connection-test path); ambiguity is then judged among the - // remaining enabled entries. const descriptor = matches.find((candidate) => candidate.enabled) ?? matches[0]!; this.requireUnambiguousRuntimeName(catalog, descriptor); return descriptor.locator; @@ -333,8 +288,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe async resetServerAuth(locator: McpServerLocator): Promise { const server = await this.resolveServer(locator); const config = requireRemoteMcpConfig(server.runtimeName, server.config); - // The invalidation event propagates into live sessions via the shared - // OAuth service's event stream. await this.oauth.invalidate(server.runtimeName, config.url); } @@ -384,20 +337,14 @@ export class McpManagementService extends Disposable implements IMcpManagementSe verify: boolean, ): Promise { const server = entry.config; - // A disabled server never participates in OAuth; keep the historical - // classification instead of reporting oauth-required or probing it. if (server.enabled === false) return 'not-applicable'; if (server.transport === 'stdio') return 'not-applicable'; if (server.bearerTokenEnvVar !== undefined) return 'bearer-token'; - // Keep status classification aligned with the existing connection manager: - // unmarked static headers are not treated as OAuth credentials. if (server.headers !== undefined && server.auth !== 'oauth') return 'not-applicable'; if (server.transport !== 'http' && server.auth !== 'oauth') return 'not-applicable'; const tokens = await this.oauth.tokenState(entry.name, server.url); const offline = (): McpServerAuthState => { if (tokens.hasTokens) { - // An expired grant with a refresh token recovers on the next connect; - // without one the credential is dead and must be re-created. return !tokens.expired || tokens.hasRefreshToken ? 'oauth-authorized' : 'oauth-expired'; } return server.auth === 'oauth' ? 'oauth-required' : 'not-applicable'; @@ -406,22 +353,16 @@ export class McpManagementService extends Disposable implements IMcpManagementSe const probe = async (): Promise => this.withProbe({ name: entry.name, ...server }, cwd, (manager) => { const status = manager.get(entry.name)?.status; - // A clean connect only proves OAuth-authorized when a grant exists; - // a server that never challenges is simply not applicable. if (status === 'connected') return tokens.hasTokens ? 'oauth-authorized' : 'not-applicable'; if (status === 'needs-auth') return tokens.hasTokens ? 'oauth-expired' : 'oauth-required'; return offline(); }); if (verify) { - // Online verification: a real connection probe settles states the - // offline view cannot distinguish (revoked grant, dead refresh token). return probe(); } if (tokens.hasTokens) return offline(); if (server.auth === 'oauth') return 'oauth-required'; - // Unpinned auth with no stored grant: probe once to detect whether the - // server challenges at all. return this.withProbe({ name: entry.name, ...server }, cwd, (manager) => manager.get(entry.name)?.status === 'needs-auth' ? 'oauth-required' : 'not-applicable', ); @@ -439,7 +380,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe ): Promise { const runtimeNameCounts = new Map(); for (const server of new Map(catalog.map((item) => [item.serverId, item])).values()) { - // Disabled entries cannot hold a connection, so they cannot collide. if (!server.enabled) continue; runtimeNameCounts.set(server.runtimeName, (runtimeNameCounts.get(server.runtimeName) ?? 0) + 1); } @@ -513,11 +453,6 @@ export class McpManagementService extends Disposable implements IMcpManagementSe function throwReadOnlyMcpServer(entry: McpRegistryEntry): void { if (entry.source === 'global' && entry.mutable) return; - // A disabled plugin descriptor is absent from the runtime target, so a - // user-level entry of this name becomes the effective one the moment it - // is written — never block mutations on a dead shadow. (Disabled project - // entries still shadow the user file at runtime, so they keep their - // read-only rejection.) if (entry.source === 'plugin' && entry.config.enabled === false) return; const reason = entry.source === 'plugin' @@ -529,7 +464,6 @@ function throwReadOnlyMcpServer(entry: McpRegistryEntry): void { ); } -/** Flatten a registry entry into the managed view of the unified plane. */ function toManagedServer(entry: McpRegistryEntry): McpManagedServer { return { name: entry.name, @@ -584,7 +518,6 @@ export function describeMcpServerLocator(locator: McpServerLocator): string { return `${locator.pluginId}/${locator.serverName}`; } -/** Inspection-time descriptor: the wire shape but with the full config. */ type McpServerRuntimeDescriptor = Omit & { readonly config: McpServerConfig; }; @@ -628,10 +561,6 @@ function selectServerDescriptors( }); } -/** - * States decidable without connecting: anything pinned (stdio, bearer token, - * static non-OAuth headers) or disabled never enters the OAuth probe. - */ function configuredMcpAuthState( server: McpServerRuntimeDescriptor, ): McpServerAuthState | undefined { diff --git a/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistry.ts b/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistry.ts index 00e63f6eb..1c63e66f0 100644 --- a/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistry.ts +++ b/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistry.ts @@ -1,22 +1,3 @@ -/** - * `mcpRegistry` domain — `IMcpRegistryService` contract. - * - * The unified read view over every MCP server source the management plane - * knows about: the layered config files (`global` — the user-level - * `mcp.json` plus, when a `cwd` is supplied, the project-root `.mcp.json` - * and project-local `.kimi-code/mcp.json`) and plugin manifests (`plugin`, - * the final effective config after the plugin contributor's transforms; - * read-only, config ownership lives in the manifest). Only user-level - * entries are `mutable` through the management API (writes keep landing in - * the user-level file). A runtime-name collision keeps both entries — the - * management plane must show the collision instead of hiding one side — - * while {@link IMcpRegistryService.resolveRuntimeTarget} picks the entry a - * live session should actually run (an enabled plugin entry wins over the - * file layers; a disabled plugin descriptor is treated as absent). Caller - * (SDK-injected) entries are session-scoped and never appear here. Bound at - * App scope. - */ - import { createDecorator, type ServiceIdentifier } from '#/_base/di/instantiation'; import type { McpServerConfig } from '#/mcpCore/config-schema'; diff --git a/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistryService.ts b/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistryService.ts index db54e6413..b3053bbd4 100644 --- a/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistryService.ts +++ b/packages/agent-core-v2/src/app/mcpRegistry/mcpRegistryService.ts @@ -1,18 +1,3 @@ -/** - * `mcpRegistry` domain — `IMcpRegistryService` implementation. - * - * Assembles the unified read view per query — config files and the plugin - * install state are the sources of truth, so nothing here needs - * invalidation: `global` entries come from the user-level store - * (`mcpConfig`) alone, or from the layered files loaded through the - * `mcpConfig` config loader when a `cwd` is supplied (rooted at the - * `bootstrap` home dir); `plugin` entries come - * from the `plugin` domain's full descriptor list (disabled plugins - * included, managed env already merged). Reads go through the os - * `IHostFileSystem`; resolution errors (e.g. a malformed project file) - * propagate instead of reading as "not configured". Bound at App scope. - */ - import { LifecycleScope } from '#/app/scopes'; import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; @@ -67,18 +52,12 @@ export class McpRegistryService implements IMcpRegistryService { config, source: 'global', origin, - // Only entries whose effective definition lives in the user-level - // file can be mutated through the management API — writing a - // project-shadowed name would never change what sessions run. mutable: origin === this.store.path, }); } } for (const entry of await this.plugins.mcpServerEntries()) { - // A plugin entry whose runtime name collides with a global one is kept, - // not dropped: the management plane must show the collision (the app - // inspection surfaces it as `unavailable`) instead of hiding one side. out.push({ name: entry.name, config: entry.config, diff --git a/packages/agent-core-v2/src/app/plugin/pluginService.ts b/packages/agent-core-v2/src/app/plugin/pluginService.ts index 7a80ffe09..65f51df9e 100644 --- a/packages/agent-core-v2/src/app/plugin/pluginService.ts +++ b/packages/agent-core-v2/src/app/plugin/pluginService.ts @@ -192,10 +192,6 @@ export class PluginService extends Service implements IPluginService { } mcpServerEntries(): Promise { - // Management-plane read: a corrupt plugin state must fail loudly here - // instead of degrading to an empty list — a management mutation guarded - // on this view could otherwise shadow a read-only plugin server while - // the plugin contributions are unknown. return this.runManagementRead(async () => { const entries = this.manager.mcpServerEntries(); if (!entries.some((entry) => entry.config.transport === 'stdio')) { diff --git a/packages/agent-core-v2/src/mcpCore/configView.ts b/packages/agent-core-v2/src/mcpCore/configView.ts index 035003bc1..0c35e5f5e 100644 --- a/packages/agent-core-v2/src/mcpCore/configView.ts +++ b/packages/agent-core-v2/src/mcpCore/configView.ts @@ -1,13 +1,3 @@ -/** - * `mcpCore` domain — wire-facing view of an MCP server's effective config. - * - * The literal values of secret-bearing fields — stdio `env` and remote - * `headers` — are replaced by their sorted key lists: they may carry API keys - * or Authorization tokens, and status/list payloads (session MCP entries, - * the app-level inspection surface) must never disclose them to SDK - * consumers. Internal reconciliation keeps using the full `McpServerConfig`. - */ - import type { McpServerConfig } from './config-schema'; export type McpServerConfigView = diff --git a/packages/agent-core-v2/src/mcpCore/oauth/provider.ts b/packages/agent-core-v2/src/mcpCore/oauth/provider.ts index aaaf26976..70736bf06 100644 --- a/packages/agent-core-v2/src/mcpCore/oauth/provider.ts +++ b/packages/agent-core-v2/src/mcpCore/oauth/provider.ts @@ -23,8 +23,6 @@ const CLIENT_SUFFIX = '-client.json'; const DISCOVERY_SUFFIX = '-discovery.json'; /** Sidecar `-meta.json` suffix; the service scans these on startup. */ export const META_SUFFIX = '-meta.json'; -// Used only when the SDK probes auth during normal transport startup and no -// callback listener is active. Interactive login overrides it with a real URL. const PASSIVE_REDIRECT_URI = 'http://127.0.0.1:3118/callback'; export interface StoredMcpOAuthTokens extends OAuthTokens { @@ -84,9 +82,6 @@ export class McpOAuthClientProvider implements OAuthClientProvider { key: this.storeKey, read: async () => this.store.read(tokensFile), write: async (tokens) => { - // Single choke point for every durable token write (explicit saves and - // refresh grants committed by the fetch interceptor alike): keep the - // incoming stamp when present, stamp otherwise. const incoming = tokens as StoredMcpOAuthTokens; await this.store.write(tokensFile, { ...incoming, @@ -156,8 +151,6 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } async saveClientInformation(info: OAuthClientInformationMixed): Promise { - // Persist first, then mirror into the cache: a failed write must not - // leave the cache claiming a registration the disk does not have. await this.store.write(`${this.storeKey}${CLIENT_SUFFIX}`, info); this.clientCache = info; } @@ -167,12 +160,6 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { - // Hand the SDK's token object to the transaction untouched: when the - // grant rode createOAuthFetch, the transaction already persisted and - // recorded exactly this payload, so a matching save consumes the - // recorded effect instead of writing again — re-writing here could - // resurrect credentials cleared between the fetch and this callback. - // The durable `obtained_at` stamp is applied by the write callback. await this.tokenTransaction.save(tokens); const meta: McpOAuthStoreMeta = { serverName: this.serverName, serverUrl: this.serverUrl }; await this.store.write(`${this.storeKey}${META_SUFFIX}`, meta); @@ -242,10 +229,6 @@ export class McpOAuthClientProvider implements OAuthClientProvider { await this.clearCredentials('discovery'); this._codeVerifier = undefined; } - // The SDK-driven invalidation actually dropped the durable grant, so - // broadcast it like a user-driven reset: sessions sharing this credential - // flip to needs-auth now instead of keeping doomed connections until - // they each hit their own 401. this.onCredentialsInvalidated?.(scope); } diff --git a/packages/agent-core-v2/src/mcpCore/oauth/service.ts b/packages/agent-core-v2/src/mcpCore/oauth/service.ts index a83e00fac..39dc4fa92 100644 --- a/packages/agent-core-v2/src/mcpCore/oauth/service.ts +++ b/packages/agent-core-v2/src/mcpCore/oauth/service.ts @@ -54,15 +54,9 @@ export interface BeginAuthorizationResult { cancel(): Promise; } -/** - * The single underlying interactive flow shared by every handle that - * `beginAuthorization` hands out for the same credential store key. - */ interface SharedAuthorizationFlow { readonly authorizationUrl: URL; - /** Starts the wait-for-callback + code exchange on first call; later calls share the outcome. */ readonly startCompletion: BeginAuthorizationResult['complete']; - /** Tears down the callback listener and flow state; invoked by the initiating handle only. */ readonly cancelUnderlying: () => Promise; } @@ -96,9 +90,7 @@ export interface McpOAuthTokenState { readonly expired: boolean; } -/** Refresh this far ahead of the absolute expiry. */ const REFRESH_AHEAD_MS = 120_000; -/** `setTimeout` cannot schedule beyond 2^31-1 ms; later saves/sweeps re-arm. */ const MAX_TIMER_DELAY_MS = 0x7fffffff; export class McpOAuthService extends Disposable { @@ -261,7 +253,6 @@ export class McpOAuthService extends Disposable { const storeKey = mcpOAuthStoreKey(serverName, serverUrl); const inFlight = this.activeAuthorizations.get(storeKey); if (inFlight !== undefined) { - // A begin-phase failure (e.g. AlreadyAuthorizedError) propagates here. const flow = await inFlight; let detached = false; return { @@ -281,16 +272,12 @@ export class McpOAuthService extends Disposable { }; } - // Reserve the slot before the first await, so a concurrent call for the - // same credential (a `clientLabel` variant included — the key is the - // same store key) joins this flow instead of racing a second one. const started = this.startAuthorizationFlow(serverName, serverUrl, options); this.activeAuthorizations.set(storeKey, started); let flow: SharedAuthorizationFlow; try { flow = await started; } catch (error) { - // Begin-phase failures leave no active flow behind. this.activeAuthorizations.delete(storeKey); throw error; } @@ -332,9 +319,6 @@ export class McpOAuthService extends Disposable { provider.setRedirectUrl(new URL(callbackServer.redirectUri)); await provider.ready; - // See invalidateStaleRegistration: a reused registration whose redirect - // URIs no longer cover this flow's random-port callback would be rejected - // at the authorization endpoint with an error only the browser ever sees. await provider.invalidateStaleRegistration(callbackServer.redirectUri); let authorizationUrl: URL | undefined; @@ -344,8 +328,6 @@ export class McpOAuthService extends Disposable { fetchFn: provider.createOAuthFetch(), }); if (result !== 'REDIRECT') { - // Tokens already valid (e.g. unexpired refresh, or a grant written - // by another process). Tell needs-auth sessions to pick them up. await callbackServer.close(); this.emit({ type: 'tokens-saved', @@ -374,9 +356,6 @@ export class McpOAuthService extends Disposable { if (settled) return; settled = true; this.activeAuthorizations.delete(storeKey); - // Release the provider's flow state before the first await: as soon as - // the map entry is gone a new flow may begin on the same provider, and - // a late resetFlow would clobber its redirect URL / PKCE state. provider.resetFlow(); await callbackServer.close().catch(() => undefined); }; @@ -481,16 +460,8 @@ export class McpOAuthService extends Disposable { } private async refreshNow(serverName: string, serverUrl: string | URL): Promise { - // An interactive authorization for this credential owns the shared - // provider's PKCE/redirect state right now; resetting it here would break - // the user's in-flight browser flow. The flow produces fresh tokens on - // completion, and the transport 401 path remains the backstop if it - // fails — so skip rather than race it. if (this.activeAuthorizations.has(mcpOAuthStoreKey(serverName, serverUrl))) return; const state = await this.tokenState(serverName, serverUrl); - // The await above opened a window: an interactive flow that began while - // the token state was being read owns the provider's flow state now, so - // re-check before resetFlow would clobber it. if (this.activeAuthorizations.has(mcpOAuthStoreKey(serverName, serverUrl))) return; if (!state.hasTokens || !state.hasRefreshToken) { throw new Error2( @@ -501,14 +472,6 @@ export class McpOAuthService extends Disposable { const provider = this.getProvider(serverName, serverUrl); provider.resetFlow(); try { - // The SDK refreshes whenever a refresh token exists, without checking - // the access-token expiry — exactly what a proactive refresh wants. A - // rejected refresh token falls through to the interactive branch and - // comes back as REDIRECT, which this non-interactive path treats as - // failure. The token request must ride the provider's fetch wrapper: - // OAuthTokenTransaction serializes grants per credential, so without it - // a slower response carrying an older rotating refresh token could be - // persisted over a newer grant written by a concurrent 401 refresh. const result = await auth(provider as OAuthClientProvider, { serverUrl, fetchFn: provider.createOAuthFetch(), @@ -529,25 +492,15 @@ export class McpOAuthService extends Disposable { const storeKey = mcpOAuthStoreKey(serverName, canonicalUrl); this.cancelScheduledRefresh(serverName, canonicalUrl); const now = Date.now(); - // Already-expired grants are never refreshed proactively: the grant may - // belong to a server nobody connects to anymore, so firing a network - // refresh on boot/save would be wasted work. The connect path (the - // transport's 401-driven refresh) remains the backstop for live servers. if (expiresAt <= now) return; const delay = expiresAt - now - REFRESH_AHEAD_MS; let timer: NodeJS.Timeout; if (delay > MAX_TIMER_DELAY_MS) { - // setTimeout cannot schedule beyond 2^31-1 ms. Arm the maximum and - // recompute on firing, so far-future grants are rescheduled instead of - // never being refreshed proactively. timer = setTimeout(() => { this.refreshTimers.delete(storeKey); this.scheduleRefresh(serverName, canonicalUrl, expiresAt); }, MAX_TIMER_DELAY_MS); } else { - // delay <= 0 means the grant is already inside the ahead-of-expiry - // window but still valid — refresh immediately. Refresh is - // single-flight per credential, so duplicate triggers are safe. timer = setTimeout( () => { this.refreshTimers.delete(storeKey); @@ -579,7 +532,6 @@ export class McpOAuthService extends Disposable { try { listener(event); } catch { - // Listener faults must not break credential persistence. } } } @@ -596,19 +548,12 @@ export class AlreadyAuthorizedError extends Error2 { } } -/** - * Read and validate one `-meta.json` sidecar. The store's `read` only - * guarantees parseable JSON, so the shape is checked field by field; a - * malformed sidecar is skipped with a warning instead of aborting the - * startup sweep. - */ async function readStoreMeta( store: McpOAuthStore, key: string, log: Logger, ): Promise { const raw: unknown = await store.read(key); - // undefined: the file vanished between list and read, or held corrupt JSON. if (raw === undefined) return undefined; if (typeof raw !== 'object' || raw === null) { log.warn('ignoring malformed MCP OAuth meta file', { file: key }); diff --git a/packages/agent-core-v2/src/workspace/workspaceMcp/workspaceMcpService.ts b/packages/agent-core-v2/src/workspace/workspaceMcp/workspaceMcpService.ts index d1c6727e9..70fac70c5 100644 --- a/packages/agent-core-v2/src/workspace/workspaceMcp/workspaceMcpService.ts +++ b/packages/agent-core-v2/src/workspace/workspaceMcp/workspaceMcpService.ts @@ -172,19 +172,14 @@ export class WorkspaceMcpService extends Disposable implements IWorkspaceMcpServ manager: McpConnectionManager, event: McpOAuthEvent, ): Promise { - // Client/verifier/discovery invalidations are flow-local; only token-level - // changes move connections. if (event.type === 'tokens-invalidated' && event.scope !== 'tokens' && event.scope !== 'all') { return; } const entry = manager.get(event.serverName); if (entry === undefined) return; - // The credential is keyed by name + canonical URL: if this manager's - // entry points at a different URL now, the event is not about it. const serverUrl = manager.getRemoteServerUrl(event.serverName); if (serverUrl === undefined || canonicalMcpOAuthResource(serverUrl) !== event.serverUrl) return; if (event.type === 'tokens-invalidated') { - // Drop the cached provider so the reconnect starts from clean state. this.oauthService.forgetProvider(event.serverName, event.serverUrl); } if (entry.status === 'disabled' || entry.status === 'removed') return; @@ -209,7 +204,6 @@ export class WorkspaceMcpService extends Disposable implements IWorkspaceMcpServ ) { return; } - // A failed proactive refresh only matters to a live connection. if (event.type === 'refresh-failed' && entry.status !== 'connected') return; await manager.reconnectAndJoin(event.serverName); } diff --git a/packages/agent-core-v2/src/workspace/workspaceMcpConfig/workspaceMcpConfigService.ts b/packages/agent-core-v2/src/workspace/workspaceMcpConfig/workspaceMcpConfigService.ts index 773277218..85f86b7d5 100644 --- a/packages/agent-core-v2/src/workspace/workspaceMcpConfig/workspaceMcpConfigService.ts +++ b/packages/agent-core-v2/src/workspace/workspaceMcpConfig/workspaceMcpConfigService.ts @@ -68,8 +68,6 @@ export class WorkspaceMcpConfigService extends Disposable implements IWorkspaceM ); this._register( mcpConfigStore.onDidWrite(() => { - // A management-plane write is already durable here, so skip the - // watch debounce and reload immediately. void this.reloadFileServers().catch((error) => { this.log.warn(`mcp config reload after management write failed: ${String(error)}`); }); @@ -114,9 +112,6 @@ export class WorkspaceMcpConfigService extends Disposable implements IWorkspaceM } private merged(): Record { - // An enabled plugin entry wins over the file layers, matching the - // management plane's runtime resolution; when the plugin entry vanishes - // (disable / remove) the same-named file entry takes back over. return { ...Object.fromEntries(this.fileServers), ...Object.fromEntries(this.pluginServers) }; } @@ -184,8 +179,6 @@ export class WorkspaceMcpConfigService extends Disposable implements IWorkspaceM private publishIfChanged(): void { const next = this.merged(); - // Null-prototype accumulator: a server literally named `__proto__` would - // otherwise hit the prototype setter and silently vanish from the diff. const upsert: Record = Object.create(null); const remove: string[] = []; for (const [name, config] of Object.entries(next)) { diff --git a/packages/agent-core-v2/test/app/mcpConfig/configStore.test.ts b/packages/agent-core-v2/test/app/mcpConfig/configStore.test.ts index 7a3dc431e..394a647ad 100644 --- a/packages/agent-core-v2/test/app/mcpConfig/configStore.test.ts +++ b/packages/agent-core-v2/test/app/mcpConfig/configStore.test.ts @@ -1,14 +1,3 @@ -/** - * Scenario: user-level mcp.json write plane over the storage byte store. - * - * Resolves `IMcpConfigStore` through the DI test harness with the in-memory - * storage backend and drives CRUD round-trips, v1-compatible byte output - * (two-space indent, trailing newline, unknown top-level keys preserved), - * name normalization, read/validation failures, `__proto__` safety, and - * `onDidWrite` firing. Run with `pnpm --filter @moonshot-ai/agent-core-v2 - * exec vitest run test/app/mcpConfig/configStore.test.ts`. - */ - import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { DisposableStore } from '#/_base/di/lifecycle'; diff --git a/packages/agent-core-v2/test/app/mcpManagement/mcpManagement.test.ts b/packages/agent-core-v2/test/app/mcpManagement/mcpManagement.test.ts index 5cdbf1fa2..347ad6c88 100644 --- a/packages/agent-core-v2/test/app/mcpManagement/mcpManagement.test.ts +++ b/packages/agent-core-v2/test/app/mcpManagement/mcpManagement.test.ts @@ -1,28 +1,3 @@ -/** - * Scenario: the MCP management write plane — CRUD round-trips through the - * real store and registry, read-only collision guards (enabled plugin entries - * reject, disabled plugin descriptors never block), guard strictness under a - * degraded read view (a plugin listing failure or a corrupt user mcp.json - * aborts mutations without persisting), redacted read-only views, - * project-layer read-only visibility under a cwd query, and the - * connection-test probe (inline http — success and unreachable-server - * failure, inline stdio with workspace materialization, name resolution and - * its ambiguity rejection), the - * auth-status surface (offline grant classification, `verify` probes), the - * locator-addressed inspection catalog with its batched probe, and the - * locator-addressed OAuth operations (begin/complete/cancel/reset, flowId - * bookkeeping, active-flow cancel, complete timeout, runtime-name ambiguity - * rejection). The `mcp_management` flag - * gates the edge exposure only; the engine service itself is deliberately - * ungated. - * - * Exercises the real `McpManagementService` + `McpRegistryService` + - * `IMcpConfigStore` (in-memory storage backend) against a stubbed - * `IPluginService` and in-process MCP fixture / OAuth servers. Run: - * `pnpm --filter @moonshot-ai/agent-core-v2 exec vitest run - * test/app/mcpManagement/mcpManagement.test.ts`. - */ - import { mkdtempSync } from 'node:fs'; import { mkdir, rm, writeFile } from 'node:fs/promises'; import { createServer as createHttpServer, type Server as HttpServer } from 'node:http'; @@ -76,7 +51,6 @@ function stdioServer(name: string, command = 'npx'): GlobalMcpServerConfig { return { name, transport: 'stdio', command }; } -/** Byte-level locator of the user-level file inside the storage backend. */ const CONFIG_SCOPE = ''; const CONFIG_KEY = 'mcp.json'; @@ -161,12 +135,6 @@ describe('McpManagementService', () => { return server; } - /** - * An OAuth-gated endpoint: every request gets a 401 Bearer challenge, and - * the token endpoint rejects refresh grants with invalid_grant (a dead - * stored grant). Mirrors the needs-auth fixtures of - * `test/mcpCore/connection-manager.test.ts`. - */ async function startGatedServer(): Promise<{ origin: string; url: string }> { const httpServer: HttpServer = createHttpServer((req, res) => { if (req.method === 'POST' && req.url === '/token') { @@ -191,12 +159,6 @@ describe('McpManagementService', () => { return { origin: `http://127.0.0.1:${port}`, url: `http://127.0.0.1:${port}/mcp` }; } - /** - * A minimal OAuth authorization server for the interactive flow: DCR at - * `/register` and a token endpoint answering the authorization_code grant. - * Discovery is seeded straight into the provider, so the authorization - * redirect never leaves the process. - */ async function startInteractiveAuthServer(): Promise<{ origin: string }> { const httpServer: HttpServer = createHttpServer((req, res) => { if (req.method !== 'POST' || (req.url !== '/register' && req.url !== '/token')) { @@ -231,7 +193,6 @@ describe('McpManagementService', () => { return { origin: `http://127.0.0.1:${port}` }; } - /** Seeding goes through a provider whose `ready` settled — earlier writes are clobbered by its initial load. */ async function seedDiscovery(name: string, url: string, authServerOrigin: string): Promise { const provider = oauth.getProvider(name, url); await provider.ready; @@ -271,7 +232,6 @@ describe('McpManagementService', () => { await provider.saveTokens({ token_type: 'Bearer', ...tokens }); } - /** Play the browser: hit the flow's localhost callback listener with a code and the carried state. */ async function deliverAuthCallback(authorizationUrl: string): Promise { const url = new URL(authorizationUrl); const redirectUri = url.searchParams.get('redirect_uri'); @@ -403,8 +363,6 @@ describe('McpManagementService', () => { transport: 'http', url: 'https://example.com/user', }); - // The collision stays visible: the fresh user-level entry lists side by - // side with the read-only disabled descriptor. const matches = added.filter((entry) => entry.name === 'plugin-demo:docs'); expect(matches).toHaveLength(2); expect(matches[0]).toMatchObject({ source: 'global', mutable: true }); @@ -435,9 +393,6 @@ describe('McpManagementService', () => { const before = await readStoreBytes(); pluginError = new Error2(ErrorCodes.PLUGIN_LOAD_FAILED, 'plugin state corrupt'); - // Only a genuine not-found reads as "no collision": a degraded read - // view must abort the write, because a mutation guarded on it could - // shadow a read-only plugin server while contributions are unknown. await expect(management.addServer(stdioServer('beta'))).rejects.toMatchObject({ code: ErrorCodes.PLUGIN_LOAD_FAILED, }); @@ -557,8 +512,6 @@ describe('McpManagementService', () => { }, 20000); it('reports a clean failure for an unreachable inline http server', async () => { - // 127.0.0.1:1 refuses the connection immediately, so the probe settles - // as a failure long before its startup timeout. const result = await management.testServer({ server: { name: 'down', @@ -622,8 +575,6 @@ describe('McpManagementService', () => { serverName: 'api', }, ]; - // The management plane rejects this write (read-only collision), so the - // collision is seeded straight into the store — as an on-disk edit would. await store.add({ name: 'plugin-demo:api', transport: 'http', @@ -646,8 +597,6 @@ describe('McpManagementService', () => { serverName: 'api', }, ]; - // The disabled file entry lists before the plugin in registry order, but - // the enabled plugin is what a live session would actually run. await store.add({ name: 'plugin-demo:api', transport: 'http', @@ -721,8 +670,6 @@ describe('McpManagementService', () => { expires_in: 3600, }); - // An expired grant with a refresh token recovers on the next connect; - // without one the credential is dead and must be re-created. await expect(management.listAuthStatuses()).resolves.toEqual([ { name: 'stale', authStatus: 'oauth-expired' }, { name: 'refreshable', authStatus: 'oauth-authorized' }, @@ -756,9 +703,6 @@ describe('McpManagementService', () => { auth: 'oauth', }); - // `plain` is unpinned with no grant, so even the offline path probes it - // once to detect a challenge (the fixture never challenges). The - // oauth-marked entry short-circuits to oauth-required without a probe. await expect(management.listAuthStatuses()).resolves.toEqual([ { name: 'plain', authStatus: 'not-applicable' }, { name: 'challenged', authStatus: 'oauth-required' }, @@ -822,7 +766,6 @@ describe('McpManagementService', () => { 'plugin:demo:api', ]); - // Probed and connected without a grant: simply not applicable. expect(byId.get('global:plain')).toMatchObject({ locator: { source: 'global', name: 'plain' }, runtimeName: 'plain', @@ -851,7 +794,6 @@ describe('McpManagementService', () => { editable: false, authStatus: 'not-applicable', }); - // Inspection configs are the redacted wire view for every entry. expect(plugin?.config).toMatchObject({ headerKeys: ['X-Key'] }); expect(plugin?.config).not.toHaveProperty('headers'); expect(JSON.stringify(plugin?.config)).not.toContain('secret'); @@ -879,7 +821,6 @@ describe('McpManagementService', () => { error: 'MCP runtime name "plugin-demo:api" is not unique', }); - // Both sides of the collision stay visible in the catalog. const all = await management.inspectServers(); expect(all.filter((server) => server.runtimeName === 'plugin-demo:api')).toHaveLength(2); }, 20000); @@ -1076,7 +1017,6 @@ describe('McpManagementService', () => { refresh_token: 'good-refresh', }); - // The stored grant refreshes fine, so begin never surfaces a browser URL. await expect( management.beginServerAuth({ source: 'global', name: 'oauthable' }), ).resolves.toEqual({ status: 'already-authorized' }); @@ -1134,8 +1074,6 @@ describe('McpManagementService', () => { await management.cancelServerAuth({ flowId: begun.flowId }); - // The flow is gone from the ledger: completing its flowId now rejects - // like any unknown flow. await expect( management.completeServerAuth({ flowId: begun.flowId, timeoutMs: 1000 }), ).rejects.toMatchObject({ @@ -1160,8 +1098,6 @@ describe('McpManagementService', () => { throw new Error(`expected authorization-required, got ${begun.status}`); } - // No callback is delivered: the wait must fail after the handle's - // timeout instead of hanging on the default 15-minute one. await expect( management.completeServerAuth({ flowId: begun.flowId, timeoutMs: 200 }), ).rejects.toThrow(/OAuth callback timed out/); @@ -1214,7 +1150,6 @@ describe('McpManagementService', () => { }, ]; - // Reset is a no-network invalidate and works for plugin servers. await expect( management.resetServerAuth({ source: 'plugin', pluginId: 'demo', serverName: 'api' }), ).resolves.toBeUndefined(); diff --git a/packages/agent-core-v2/test/app/mcpRegistry/mcpRegistry.test.ts b/packages/agent-core-v2/test/app/mcpRegistry/mcpRegistry.test.ts index a7cbb17ee..312e3764a 100644 --- a/packages/agent-core-v2/test/app/mcpRegistry/mcpRegistry.test.ts +++ b/packages/agent-core-v2/test/app/mcpRegistry/mcpRegistry.test.ts @@ -1,17 +1,3 @@ -/** - * Scenario: the unified MCP registry read view — user-level listing without a - * cwd, the three-layer file merge with origins/mutability when a cwd is given, - * read-only plugin entries kept side by side on runtime-name collisions, - * runtime-target resolution priority, plugin load failure propagation, and - * structural config equality. - * - * Exercises the real `McpRegistryService` over the real `IMcpConfigStore` - * (in-memory storage backend), a stubbed `IPluginService`, and real temp - * config files read through the node-local `IHostFileSystem`. Run: - * `pnpm --filter @moonshot-ai/agent-core-v2 exec vitest run - * test/app/mcpRegistry/mcpRegistry.test.ts`. - */ - import { mkdtempSync } from 'node:fs'; import { mkdir, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; @@ -98,8 +84,6 @@ describe('McpRegistryService', () => { async function makeProject(): Promise<{ project: string; sub: string }> { const project = mkdtempSync(join(tmpdir(), 'kimi-mcp-registry-proj-')); tempDirs.push(project); - // An empty `.git` directory is enough for the work-tree probe to anchor - // the project-root layer here instead of walking further up. await mkdir(join(project, '.git'), { recursive: true }); const sub = join(project, 'pkg'); await mkdir(sub, { recursive: true }); @@ -162,14 +146,11 @@ describe('McpRegistryService', () => { 'userOnly', ]); - // Later layers override; the origin follows the winning definition and - // only the user-level winner stays mutable. expect(byName.get('shared')).toMatchObject({ source: 'global', mutable: false, origin: join(project, '.mcp.json'), }); - // Repo-root stdio cwd resolves against the repo root. expect(byName.get('shared')?.config).toEqual({ transport: 'stdio', command: 'repo-version', @@ -294,7 +275,6 @@ describe('McpRegistryService', () => { config: { command: 'user-version' }, }); - // With the file layer gone too, the name no longer resolves at all. await store.remove('plugin-demo:api'); await expect(registry.resolveRuntimeTarget('plugin-demo:api')).resolves.toBeUndefined(); }); diff --git a/packages/agent-core-v2/test/app/plugin/pluginService.test.ts b/packages/agent-core-v2/test/app/plugin/pluginService.test.ts index 1d92516af..617148269 100644 --- a/packages/agent-core-v2/test/app/plugin/pluginService.test.ts +++ b/packages/agent-core-v2/test/app/plugin/pluginService.test.ts @@ -180,8 +180,6 @@ describe('PluginService (plugin boundary)', () => { try { const svc = host.app.accessor.get(IPluginService); await expect(svc.enabledMcpServers()).resolves.toEqual({}); - // The management-plane descriptor list fails loudly instead: a - // mutation guarded on it must not run with plugin state unknown. const failure = await svc.mcpServerEntries().catch((error: unknown) => error); expect(failure).toMatchObject({ code: 'plugin.load_failed' }); } finally { diff --git a/packages/agent-core-v2/test/mcpCore/oauth/service.test.ts b/packages/agent-core-v2/test/mcpCore/oauth/service.test.ts index 70354342d..e83bcf4e1 100644 --- a/packages/agent-core-v2/test/mcpCore/oauth/service.test.ts +++ b/packages/agent-core-v2/test/mcpCore/oauth/service.test.ts @@ -1,19 +1,3 @@ -/** - * Scenario: the shared McpOAuthService stamps token writes with `obtained_at`, - * exposes the offline token state, emits credential events, runs token - * refreshes single-flight per credential, serializes interactive flows per - * credential, and schedules/shuts down proactive refreshes — over the async - * `McpOAuthStore` port (memory stub). Ported from v1's - * `test/mcp/oauth-service.test.ts`. Run with - * `pnpm --filter @moonshot-ai/agent-core-v2 exec vitest run test/mcpCore/oauth/service.test.ts`. - * - * Note: the scheduling/shutdown describes drive the refresh timers with - * `vi.useFakeTimers()` — a deliberate exception to the no-fake-timers rule: - * the behavior under test IS the timer semantics (a `MAX_TIMER_DELAY_MS` - * re-arm would take ~25 days of wall clock), and the service exposes no - * clock seam. The v1 blueprint suite drives them the same way. - */ - import { createServer as createHttpServer, type Server as HttpServer } from 'node:http'; import type { AddressInfo as HttpAddress } from 'node:net'; @@ -58,16 +42,10 @@ afterEach(async () => { } }); -/** The memory store's `list(prefix)` is prefix-matching, so meta sidecars are filtered by suffix. */ async function listMetaKeys(store: McpOAuthStore): Promise { return (await store.list()).filter((key) => key.endsWith(META_SUFFIX)); } -/** - * The provider mirrors client/discovery state into in-memory caches on - * construction (`ready`); seeding before that load settles is clobbered by - * it, so every seed goes through a provider whose `ready` has resolved. - */ async function readyProvider(fixture: Fixture): Promise { const provider = fixture.service.getProvider(SERVER_NAME, SERVER_URL); await provider.ready; @@ -79,13 +57,6 @@ interface FakeAuthServer { readonly counts: { register: number; exchange: number; refresh: number }; } -/** - * Minimal OAuth authorization server: DCR at `/register` (echoes the client - * metadata back with a client_id) and a token endpoint that answers both - * `authorization_code` and `refresh_token` grants with a fresh access token. - * Discovery and the authorization redirect never touch the network — tests - * seed discovery state and drive the localhost callback listener directly. - */ async function startFakeAuthServer( options: { readonly rejectRefreshToken?: boolean } = {}, ): Promise { @@ -140,7 +111,6 @@ async function startFakeAuthServer( return { url: `http://127.0.0.1:${port}`, counts }; } -/** Discovery state + registered client metadata matching a fake auth server. */ function authServerState(authServerUrl: string) { return { discovery: { @@ -165,10 +135,6 @@ function authServerState(authServerUrl: string) { }; } -/** - * Play the browser: hit the flow's localhost callback listener with a code - * and the `state` carried by the authorization URL. - */ async function deliverCallback(flow: BeginAuthorizationResult): Promise { const redirectUri = flow.authorizationUrl.searchParams.get('redirect_uri'); const state = flow.authorizationUrl.searchParams.get('state'); @@ -373,8 +339,6 @@ describe('McpOAuthService single-flight refresh', () => { token_type: 'Bearer', }); - // The refresh's /token request must go through OAuthTokenTransaction so - // it serializes against concurrent 401-driven refreshes from transports. const fetchSpy = vi.spyOn(provider, 'createOAuthFetch'); await fixture.service.refresh(SERVER_NAME, SERVER_URL); expect(fetchSpy).toHaveBeenCalled(); @@ -394,11 +358,6 @@ describe('McpOAuthService single-flight refresh', () => { token_type: 'Bearer', }); - // The dead refresh token is rejected with invalid_grant, so the SDK - // invalidates the 'tokens' scope and the durable grant is dropped. That - // must broadcast the invalidation like a user-driven reset, or sessions - // sharing the credential keep their doomed connections until their own - // 401s. await expect(fixture.service.refresh(SERVER_NAME, SERVER_URL)).rejects.toThrow( /requires an interactive login/, ); @@ -415,7 +374,6 @@ describe('McpOAuthService single-flight refresh', () => { const fixture = makeFixture(); cleanups.push(() => fixture.service.dispose()); - // The token endpoint returns a rotating refresh grant. const grant = { access_token: 'rotated-access', refresh_token: 'rotated-refresh', @@ -455,20 +413,15 @@ describe('McpOAuthService single-flight refresh', () => { token_type: 'Bearer', }); - // The SDK's grant request rides the transaction fetch, which persists and - // records the exact payload… const res = await provider.createOAuthFetch()(`${authServerUrl}/token`, { method: 'POST', body: new URLSearchParams({ grant_type: 'refresh_token', refresh_token: 'seed-refresh' }), }); const granted = (await res.json()) as Parameters[0]; - // …but before the SDK's saveTokens lands, the credential is reset. await provider.clearCredentials('all'); expect(await provider.tokens()).toBeUndefined(); - // The matching save is consumed as already-recorded instead of writing - // the cleared grant back to disk. await provider.saveTokens(granted); expect(await provider.tokens()).toBeUndefined(); }, 15000); @@ -483,7 +436,6 @@ describe('McpOAuthService interactive flow serialization', () => { await provider.saveDiscoveryState(authServerState(authServer.url).discovery); const first = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); - // A clientLabel variant maps to the same store key, so it joins too. const second = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL, { clientLabel: 'other-client', }); @@ -492,7 +444,6 @@ describe('McpOAuthService interactive flow serialization', () => { const firstComplete = first.complete({ timeoutMs: 10_000 }); await deliverCallback(first); await firstComplete; - // The joiner shares the settled outcome; the exchange ran exactly once. await second.complete(); expect(authServer.counts.exchange).toBe(1); expect((await fixture.service.tokenState(SERVER_NAME, SERVER_URL)).hasTokens).toBe(true); @@ -504,9 +455,6 @@ describe('McpOAuthService interactive flow serialization', () => { const authServer = await startFakeAuthServer({ rejectRefreshToken: true }); const provider = await readyProvider(fixture); await provider.saveDiscoveryState(authServerState(authServer.url).discovery); - // A dead-but-present grant keeps the credential refreshable, so a - // proactive/manual refresh would normally proceed — and would hit the - // same shared provider the interactive flow lives on. await provider.saveTokens({ access_token: 'stale-access-token', refresh_token: 'stale-refresh-token', @@ -516,8 +464,6 @@ describe('McpOAuthService interactive flow serialization', () => { const flow = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); const complete = flow.complete({ timeoutMs: 10_000 }); - // Refresh must skip while the flow is active instead of resetting the - // shared provider's PKCE/state out from under the browser callback. await expect(fixture.service.refresh(SERVER_NAME, SERVER_URL)).resolves.toBeUndefined(); await deliverCallback(flow); await complete; @@ -526,9 +472,6 @@ describe('McpOAuthService interactive flow serialization', () => { }, 15000); it('skips a refresh whose token read straddles the start of an interactive flow', async () => { - // Gate one read of the tokens file so the refresh's `tokenState()` await - // stays open while an interactive flow begins — the exact window the - // second `activeAuthorizations` check in refreshNow exists for. const memory = createMemoryMcpOAuthStore(); let releaseTokensRead: () => void = () => undefined; const tokensReadGate = new Promise((resolve) => { @@ -543,7 +486,7 @@ describe('McpOAuthService interactive flow serialization', () => { ...memory, async read(key: string): Promise { if (gateArmed && key.endsWith('-tokens.json')) { - gateArmed = false; // hold exactly one read + gateArmed = false; signalReadHeld(); await tokensReadGate; } @@ -552,14 +495,11 @@ describe('McpOAuthService interactive flow serialization', () => { }; const fixture = makeFixture(store); cleanups.push(() => fixture.service.dispose()); - // Runs before dispose (LIFO): unblocks a parked refresh on a failure path. cleanups.push(() => releaseTokensRead()); const authServer = await startFakeAuthServer({ rejectRefreshToken: true }); const provider = await readyProvider(fixture); await provider.saveDiscoveryState(authServerState(authServer.url).discovery); await provider.saveClientInformation(authServerState(authServer.url).client); - // A dead-but-present grant keeps the credential refreshable, so the - // refresh below would normally proceed to the token endpoint. await provider.saveTokens({ access_token: 'stale-access-token', refresh_token: 'stale-refresh-token', @@ -567,27 +507,18 @@ describe('McpOAuthService interactive flow serialization', () => { expires_in: 3600, }); - // The refresh passes the first activeAuthorizations check and parks - // inside the token-state read. gateArmed = true; const refresh = fixture.service.refresh(SERVER_NAME, SERVER_URL); await tokensReadHeld; - // An interactive flow begins in that window and takes over the shared - // provider's flow state. (Its own dead-grant refresh attempt is the one - // /token hit counted here.) const flow = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); const complete = flow.complete({ timeoutMs: 10_000 }); expect(authServer.counts.refresh).toBe(1); - // Releasing the read must not let the refresh race the flow: the re-check - // sees the active authorization, so no resetFlow and no second /token - // request — the refresh settles quietly. releaseTokensRead(); await expect(refresh).resolves.toBeUndefined(); expect(authServer.counts.refresh).toBe(1); - // The interactive flow is intact: the callback completes the exchange. await deliverCallback(flow); await complete; expect(authServer.counts.exchange).toBe(1); @@ -604,7 +535,6 @@ describe('McpOAuthService interactive flow serialization', () => { const first = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); const second = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); - // A joiner's cancel only detaches itself; the underlying flow survives. await second.cancel(); await expect(second.complete()).rejects.toThrow(/already completed or cancelled/); @@ -626,8 +556,6 @@ describe('McpOAuthService interactive flow serialization', () => { await first.cancel(); await expect(second.complete()).rejects.toThrow(/already completed or cancelled/); - // The credential is free again: a new begin starts a fresh flow with a - // new callback listener (hence a new redirect URI) and completes cleanly. const third = await fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL); expect(third.authorizationUrl.toString()).not.toBe(first.authorizationUrl.toString()); const thirdComplete = third.complete({ timeoutMs: 10_000 }); @@ -649,13 +577,9 @@ describe('McpOAuthService interactive flow serialization', () => { token_type: 'Bearer', }); - // The stored grant refreshes fine, so begin falls into the - // AlreadyAuthorizedError path instead of surfacing a URL. await expect( fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL), ).rejects.toBeInstanceOf(AlreadyAuthorizedError); - // A stale map entry would make the retry join a dead flow instead of - // failing the same way. await expect( fixture.service.beginAuthorization(SERVER_NAME, SERVER_URL), ).rejects.toBeInstanceOf(AlreadyAuthorizedError); @@ -665,9 +589,6 @@ describe('McpOAuthService interactive flow serialization', () => { describe('McpOAuthService sweepProactiveRefresh resilience', () => { it('skips malformed meta sidecars and still schedules the valid credential', async () => { - // The memory store cannot hold unparseable JSON, so v1's corrupt file is - // simulated by a key that `list()` surfaces but `read()` yields undefined - // for (the same observation v1's JsonFileStore produced for corrupt JSON). const memory = createMemoryMcpOAuthStore(); const store: McpOAuthStore = { ...memory, @@ -680,9 +601,6 @@ describe('McpOAuthService sweepProactiveRefresh resilience', () => { cleanups.push(() => fixture.service.dispose()); const authServer = await startFakeAuthServer(); - // A valid credential written straight to the store (simulating a previous - // process), expiring inside the proactive window so the sweep schedules - // an immediate refresh. const state = authServerState(authServer.url); const storeKey = mcpOAuthStoreKey(SERVER_NAME, SERVER_URL); await fixture.store.write(`${storeKey}-discovery.json`, state.discovery); @@ -699,8 +617,6 @@ describe('McpOAuthService sweepProactiveRefresh resilience', () => { serverUrl: SERVER_URL, } satisfies McpOAuthStoreMeta); - // Sidecars that parse as JSON but have the wrong shape, plus one whose - // read yields undefined (the corrupt-JSON case). await fixture.store.write('broken-empty-meta.json', {}); await fixture.store.write('broken-types-meta.json', { serverName: 1, serverUrl: 42 }); await fixture.store.write('broken-url-meta.json', { serverName: 'x', serverUrl: 'not a url' }); @@ -724,8 +640,6 @@ describe('McpOAuthService proactive refresh scheduling', () => { const state = authServerState(authServer.url); await provider.saveDiscoveryState(state.discovery); await provider.saveClientInformation(state.client); - // expires_in 60s < REFRESH_AHEAD_MS (120s): still valid, but already - // inside the proactive window, so the save hook must refresh immediately. await provider.saveTokens({ access_token: 'stale-access-token', refresh_token: 'stale-refresh-token', @@ -744,12 +658,11 @@ describe('McpOAuthService proactive refresh scheduling', () => { }); cleanups.push(() => fixture.service.dispose()); vi.useFakeTimers(); - const maxTimerDelayMs = 0x7fffffff; // mirrors MAX_TIMER_DELAY_MS in the service + const maxTimerDelayMs = 0x7fffffff; const refreshSpy = vi .spyOn(fixture.service, 'refresh') .mockRejectedValue(new Error('refresh unavailable in test')); - // ~25 days of validity: expiresAt - REFRESH_AHEAD_MS exceeds 2^31-1 ms. await fixture.service.getProvider(SERVER_NAME, SERVER_URL).saveTokens({ access_token: 'a', refresh_token: 'r', @@ -758,8 +671,6 @@ describe('McpOAuthService proactive refresh scheduling', () => { }); const expiresAt = (await fixture.service.tokenState(SERVER_NAME, SERVER_URL)).expiresAt!; - // The far-future grant is armed at the maximum timer delay; firing that - // timer re-computes the schedule instead of dropping the grant. await vi.advanceTimersByTimeAsync(maxTimerDelayMs); expect(refreshSpy).not.toHaveBeenCalled(); @@ -806,7 +717,6 @@ describe('McpOAuthService shutdown', () => { await fixture.service.shutdown(); - // The flow's callback listener is gone; completing is no longer possible. await expect(flow.complete()).rejects.toThrow(/already completed or cancelled/); }, 15000); @@ -817,14 +727,12 @@ describe('McpOAuthService shutdown', () => { await fixture.service.shutdown(); - // Listeners are cleared: later credential events go nowhere. const eventCount = fixture.events.length; await fixture.service .getProvider(SERVER_NAME, SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer', expires_in: 3600 }); expect(fixture.events).toHaveLength(eventCount); - // Cached providers were dropped. expect(fixture.service.getProvider(SERVER_NAME, SERVER_URL)).not.toBe(providerBefore); }); diff --git a/packages/agent-core-v2/test/workspace/workspaceInstance/workspaceInstanceManager.test.ts b/packages/agent-core-v2/test/workspace/workspaceInstance/workspaceInstanceManager.test.ts index 9fdcfff11..c51d62b2a 100644 --- a/packages/agent-core-v2/test/workspace/workspaceInstance/workspaceInstanceManager.test.ts +++ b/packages/agent-core-v2/test/workspace/workspaceInstance/workspaceInstanceManager.test.ts @@ -134,10 +134,6 @@ function manager( update: async () => undefined, delete: async () => {}, }; - // Positional mirror of the WorkspaceInstanceManager constructor signature — - // adding or removing a constructor parameter shifts every slot here and the - // mismatch fails silently (a stub landing on the wrong dep), so keep the - // slot count and indices in sync with the signature. const args: unknown[] = [ {}, { scope: () => 'sessions' }, diff --git a/packages/agent-core-v2/test/workspace/workspaceMcp/workspaceMcp.test.ts b/packages/agent-core-v2/test/workspace/workspaceMcp/workspaceMcp.test.ts index 069b95fc1..b510fbcf0 100644 --- a/packages/agent-core-v2/test/workspace/workspaceMcp/workspaceMcp.test.ts +++ b/packages/agent-core-v2/test/workspace/workspaceMcp/workspaceMcp.test.ts @@ -461,14 +461,6 @@ describe('WorkspaceMcpService', () => { ); }); - /** - * Intentional seam: fabricate the manager's view of one entry instead of - * running a real connection. The credential-event handler reads the - * manager only through `get` / `getRemoteServerUrl` and acts only through - * `reconnectAndJoin` / `reconnectAfterCurrent` / `onStatusChange`, so - * stubbing those prototype methods stands in for any real entry in this - * status while keeping these tests off the network/process boundary. - */ function mockManagerEntry( status: McpServerStatus, url: string = SERVER_URL, @@ -485,12 +477,6 @@ describe('WorkspaceMcpService', () => { .mockResolvedValue(undefined); } - /** - * A token endpoint that 500s every refresh. The SDK maps a 5xx to a - * ServerError (not invalid_grant), so the service reports - * `refresh-failed` WITHOUT invalidating the stored grant — the cleanest - * way to attribute what follows to the refresh-failed event alone. - */ async function startRefreshFailingServer(): Promise<{ origin: string; counts: { refresh: number }; @@ -511,7 +497,6 @@ describe('WorkspaceMcpService', () => { return { origin: `http://127.0.0.1:${port}`, counts }; } - /** Discovery + registered client for the fake auth server; tokens are saved by the test itself. */ async function seedOAuthServerState(authServerOrigin: string): Promise { const provider = oauthService.getProvider('notion', SERVER_URL); await provider.ready; @@ -559,10 +544,6 @@ describe('WorkspaceMcpService', () => { await oauthService .getProvider('notion', SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer' }); - // Negative wait, not wall-clock work: the handler's decision path has - // no await before the reconnect call, so the outcome is already decided - // when saveTokens returns; 20ms only drains the surrounding promise - // chain. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); @@ -593,9 +574,6 @@ describe('WorkspaceMcpService', () => { await oauthService .getProvider('notion', SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer' }); - // Same negative-wait rationale as the connected-entry case above: the - // handler's synchronous part already ran when the event fired, so 20ms - // only drains the microtask/promise chain. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); @@ -621,13 +599,9 @@ describe('WorkspaceMcpService', () => { await oauthService .getProvider('notion', SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer' }); - // The handler parked on the status wait instead of reconnecting - // mid-connect (same drain rationale as the connected-entry case). await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAfterCurrent).not.toHaveBeenCalled(); - // The initial connect settling (any non-pending status) releases the - // deferral. notifyStatus?.({ name: 'notion', transport: 'http', status: 'connected', toolCount: 0 }); await vi.waitFor(() => { expect(reconnectAfterCurrent).toHaveBeenCalledWith('notion'); @@ -644,7 +618,6 @@ describe('WorkspaceMcpService', () => { const provider = oauthService.getProvider('notion', SERVER_URL); await provider.ready; await provider.clearCredentials('client'); - // Same drain rationale as the connected-entry case above. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); @@ -661,7 +634,6 @@ describe('WorkspaceMcpService', () => { const provider = oauthService.getProvider('notion', SERVER_URL); await provider.ready; await provider.clearCredentials('discovery'); - // Same drain rationale as the connected-entry case above. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); @@ -675,9 +647,6 @@ describe('WorkspaceMcpService', () => { manager = service.connectionManager(); await service.ready; - // expires_in inside the proactive window arms an immediate refresh. The - // tokens-saved event fires while no entry is mocked — the manager - // lookup misses — so only the later refresh-failed reaches the entry. await oauthService.getProvider('notion', SERVER_URL).saveTokens({ access_token: 'stale-access-token', refresh_token: 'stale-refresh-token', @@ -701,8 +670,6 @@ describe('WorkspaceMcpService', () => { manager = service.connectionManager(); await service.ready; - // Same trick as the connected case: tokens-saved misses the unmocked - // entry; only refresh-failed reaches it. await oauthService.getProvider('notion', SERVER_URL).saveTokens({ access_token: 'stale-access-token', refresh_token: 'stale-refresh-token', @@ -711,9 +678,6 @@ describe('WorkspaceMcpService', () => { }); const reconnectAndJoin = mockManagerEntry('needs-auth'); - // Wait until the failure was definitely reported, then drain: the - // handler's decision for a needs-auth entry runs synchronously off the - // event. await vi.waitFor(() => { expect(events.some((event) => event.type === 'refresh-failed')).toBe(true); }); @@ -730,7 +694,6 @@ describe('WorkspaceMcpService', () => { await oauthService .getProvider('notion', SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer' }); - // Same drain rationale as the connected-entry case above. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); @@ -745,7 +708,6 @@ describe('WorkspaceMcpService', () => { await oauthService .getProvider('notion', SERVER_URL) .saveTokens({ access_token: 'a', token_type: 'Bearer' }); - // Same drain rationale as the connected-entry case above. await new Promise((resolve) => setTimeout(resolve, 20)); expect(reconnectAndJoin).not.toHaveBeenCalled(); diff --git a/packages/agent-core-v2/test/workspace/workspaceMcpConfig/workspaceMcpConfig.test.ts b/packages/agent-core-v2/test/workspace/workspaceMcpConfig/workspaceMcpConfig.test.ts index fe39cefa5..a44265842 100644 --- a/packages/agent-core-v2/test/workspace/workspaceMcpConfig/workspaceMcpConfig.test.ts +++ b/packages/agent-core-v2/test/workspace/workspaceMcpConfig/workspaceMcpConfig.test.ts @@ -233,8 +233,6 @@ describe('WorkspaceMcpConfigService', () => { await writeProjectConfig({}); watchFires.get(cwd)?.fire({ path: file, action: 'modified', kind: 'file' }); - // The plugin entry already owned the runtime name, so the merged view - // does not change when its file-layer shadow vanishes. await new Promise((resolvePromise) => setTimeout(resolvePromise, 500)); expect(changes).toEqual([]); expect(service.servers()).toEqual({ shared: stdioConfig('plugin-version') }); diff --git a/packages/kap-server/src/protocol/error-codes.ts b/packages/kap-server/src/protocol/error-codes.ts index 7272e1e8d..7807de66a 100644 --- a/packages/kap-server/src/protocol/error-codes.ts +++ b/packages/kap-server/src/protocol/error-codes.ts @@ -61,7 +61,6 @@ export const ErrorCode = { CAPABILITY_UNSUPPORTED: 40925, RUNTIME_UNAVAILABLE: 40926, PROMPT_ID_CONFLICT: 40927, - /** MCP 管理面未启用(mcp_management flag 关闭),同 40923 的 flag-未开先例 */ MCP_MANAGEMENT_DISABLED: 40928, APPROVAL_EXPIRED: 41001, diff --git a/packages/kap-server/src/routes/v2/mcp.ts b/packages/kap-server/src/routes/v2/mcp.ts index 9dfa04ebf..221fa359b 100644 --- a/packages/kap-server/src/routes/v2/mcp.ts +++ b/packages/kap-server/src/routes/v2/mcp.ts @@ -1,43 +1,3 @@ -/** - * `/api/v2/mcp` — the unified MCP management plane. - * - * Thin REST edge over the App-scope `IMcpManagementService` (agent-core-v2 - * `mcpManagement` domain): CRUD on the user-level `mcp.json`, a connection - * test probe, the locator-addressed inspection catalog, the auth-status - * surface, and the locator-addressed OAuth flow operations. - * - * The whole plane is gated by the `mcp_management` experimental flag: every - * route runs a preHandler gate that answers the `40928 - * mcp.management_disabled` envelope while the flag is off (the engine service - * itself stays ungated — only the edge hides it). The gate awaits - * `IConfigService.ready` before reading the flag so a config-enabled flag is - * honored from the very first request (the same startup race the - * `/api/v1/meta` flags projection guards against), and the check runs per - * request so a config-flipped flag takes effect without a reboot. - * - * Wire conventions follow `/api/v2/sessions`: the `{ code, msg, data, - * request_id }` envelope carries the business outcome — `40001` for invalid - * params/body (zod issues ride `details`) and for the engine's - * `request.invalid` / `config.invalid` rejections, `40408` for an unknown - * server name (`mcp.server_not_found`), `40928` while the plane is disabled — - * and the HTTP status only reports transport-level outcomes. - * - * REST shape notes: - * - CRUD lives on `/mcp/servers[/{name}]`. `PUT` takes the config body - * WITHOUT `name` (the path owns the identity) and the handler reattaches - * it; `POST` takes the named config (`GlobalMcpServerConfig`) verbatim. - * - Unlike the config files, the wire requires an explicit `transport` - * discriminant (the engine's `McpServerConfigSchema` preprocess that - * infers it from `command`/`url` is a file-format convenience, not part of - * the API contract — same strictness as klient's `mcpServerConfigSchema`). - * - Non-CRUD operations use colon actions (`/mcp/servers::test`, - * `/mcp/auth::begin`, …) declared with a doubled colon so find-my-way - * serves the literal colon on the wire (same convention as - * `/workspace/fs::search` in v1). - * - `verify` on `/mcp/auth-statuses` is a string query param - * (`?verify=true`) mapped onto the engine's boolean flag. - */ - import { ErrorCodes, IConfigService, @@ -93,15 +53,10 @@ interface V2McpRouteHost { ): unknown; } -// --------------------------------------------------------------------------- -// Request contract -// --------------------------------------------------------------------------- - const serverNameSchema = z.string().min(1); const serverNameParamSchema = z.object({ name: serverNameSchema }); -/** `?cwd=` joins the project layers into the resolution (engine `McpRegistryQuery`). */ const serverScopedQuerySchema = z.object({ cwd: z.string().min(1).optional() }); const authStatusesQuerySchema = z.object({ @@ -109,14 +64,12 @@ const authStatusesQuerySchema = z.object({ verify: z.enum(['true', 'false']).optional(), }); -/** `GlobalMcpServerConfig` — a named full config (POST body, inline test target). */ const globalMcpServerConfigSchema = z.discriminatedUnion('transport', [ McpServerStdioConfigSchema.extend({ name: serverNameSchema }), McpServerHttpConfigSchema.extend({ name: serverNameSchema }), McpServerSseConfigSchema.extend({ name: serverNameSchema }), ]); -/** `McpServerConfig` — PUT body; the path `{name}` owns the identity. */ const mcpServerConfigBodySchema = z.discriminatedUnion('transport', [ McpServerStdioConfigSchema, McpServerHttpConfigSchema, @@ -149,10 +102,6 @@ const authCompleteBodySchema = z.object({ const authCancelBodySchema = z.object({ flowId: z.string().min(1) }); -// --------------------------------------------------------------------------- -// Response contract (OpenAPI documentation; serialization is pass-through) -// --------------------------------------------------------------------------- - const mcpServerSourceSchema = z.enum(['global', 'plugin', 'caller']); const mcpServerAuthStateSchema = z.enum([ @@ -164,12 +113,6 @@ const mcpServerAuthStateSchema = z.enum([ 'unavailable', ]); -/** - * Managed/inspected server config on the wire: mutable entries carry the full - * config (edit UIs prefill from it); read-only entries are redacted — `env` / - * `headers` values never cross, only the sorted key lists (`envKeys` / - * `headerKeys`). One schema covers both shapes. - */ const mcpServerConfigDataSchema = z.union([ McpServerStdioConfigSchema.extend({ envKeys: z.array(z.string()).optional() }), McpServerHttpConfigSchema.extend({ headerKeys: z.array(z.string()).optional() }), @@ -218,31 +161,18 @@ const mcpServerAuthBeginResultSchema = z.union([ z.object({ status: z.literal('already-authorized') }), ]); -/** `40001 validation.failed` carries the offending fields (REST.md §1.4). */ const detailsSchema = z.array(z.object({ path: z.string(), message: z.string() })); -/** Errors every route in this file can return. */ const baseErrorSchemas = { [ErrorCode.VALIDATION_FAILED]: { detailsSchema }, [ErrorCode.MCP_MANAGEMENT_DISABLED]: {}, }; -/** Plus `40408` — routes that address one server by name / locator. */ const namedServerErrorSchemas = { ...baseErrorSchemas, [ErrorCode.MCP_SERVER_NOT_FOUND]: {}, }; -// --------------------------------------------------------------------------- -// Error mapping -// --------------------------------------------------------------------------- - -/** - * Map the engine's coded rejections onto the wire envelope: an unknown server - * is `40408`, a rejected request/config is `40001` (the v1 `transport/errors.ts` - * precedent for both codes), a disabled plane is `40928`. Anything else - * rethrows into the catch-all `50001` hook. - */ function sendMappedError( reply: { send(payload: unknown): unknown }, requestId: string, @@ -267,14 +197,9 @@ function sendMappedError( throw err; } -// --------------------------------------------------------------------------- -// Routes -// --------------------------------------------------------------------------- - export function registerV2McpRoutes(app: V2McpRouteHost, core: Scope): void { const management = (): IMcpManagementService => core.accessor.get(IMcpManagementService); - // The flag gate shared by every route in this file (see the header). const gate = ( req: { id: string }, reply: { send(payload: unknown): unknown }, diff --git a/packages/kap-server/test/v2Mcp.test.ts b/packages/kap-server/test/v2Mcp.test.ts index 4da0552b3..6f21bcc0d 100644 --- a/packages/kap-server/test/v2Mcp.test.ts +++ b/packages/kap-server/test/v2Mcp.test.ts @@ -1,13 +1,3 @@ -/** - * Scenario: `/api/v2/mcp` — the unified MCP management plane. - * Responsibilities: the `mcp_management` flag gate (off → every route answers - * the `40928 mcp.management_disabled` envelope without touching the service; - * on → the full surface), the envelope wire shape of every route, and the - * domain-code → wire-code mapping (`mcp.server_not_found` → 40408, - * `request.invalid` / `config.invalid` → 40001). - * Wiring: real kap-server; `IMcpManagementService` stubbed via DI seeds. - * Run: `pnpm --filter @moonshot-ai/kap-server exec vitest run test/v2Mcp.test.ts`. - */ import { mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -28,7 +18,6 @@ import { type RunningServer, startServer } from '../src/start'; import { authedFetch } from './helpers/auth'; import { TEST_HOST_IDENTITY } from './helpers/hostIdentity'; -/** The shared REST envelope: business outcome in `code`, payload in `data`. */ interface EnvelopeWire { code: number; msg: string; @@ -45,7 +34,6 @@ const STDIO_A: GlobalMcpServerConfig = { env: { TOKEN: 'secret' }, }; -/** Recording stub: user-level servers held in a Map, every call logged. */ interface McpStub { readonly service: IMcpManagementService; readonly calls: string[]; @@ -174,9 +162,6 @@ describe('server /api/v2/mcp', () => { let base: string; beforeEach(() => { - // Neutralize flag env vars leaking from the developer shell (same pattern - // as meta.test.ts): the per-flag env must be fully ABSENT for the - // flag-off baseline, and is pinned per describe below. vi.stubEnv('KIMI_CODE_EXPERIMENTAL_FLAG', '0'); }); @@ -265,8 +250,6 @@ describe('server /api/v2/mcp', () => { const added = await call('POST', '/api/v2/mcp/servers', STDIO_A); expect(added.status).toBe(200); expect(added.body.code).toBe(0); - // Mutable (user-level) entries carry the FULL config — edit UIs prefill - // from it, so `env` values are present here by design. expect(added.body.data).toEqual([ { name: 'a', @@ -292,8 +275,6 @@ describe('server /api/v2/mcp', () => { }); expect(updated.body.code).toBe(0); expect(updated.body.data).toHaveLength(1); - // The path owns the identity: the body carried no `name`, the route - // reattached the path param before delegating. expect(stub.state.lastUpdate).toEqual({ transport: 'stdio', command: 'run-b', name: 'a' }); const removed = await call('DELETE', '/api/v2/mcp/servers/a'); @@ -322,24 +303,19 @@ describe('server /api/v2/mcp', () => { const stub = makeMcpStub(); await boot(stub); - // Missing the `transport` discriminant. const badAdd = await call('POST', '/api/v2/mcp/servers', { name: 'a', command: 'run-a' }); expect(badAdd.body.code).toBe(40001); expect(Array.isArray(badAdd.body.details)).toBe(true); - // Locator missing the server name. const badBegin = await call('POST', '/api/v2/mcp/auth:begin', { source: 'global' }); expect(badBegin.body.code).toBe(40001); - // The service never saw either request. expect(stub.calls).toEqual([]); }); it('maps the engine request.invalid rejection to 40001', async () => { const stub = makeMcpStub(); await boot(stub); - // Zod-valid (both fields optional) but rejected by the engine: a test - // target needs a name or an inline server. const res = await call('POST', '/api/v2/mcp/servers:test', {}); expect(res.body.code).toBe(40001); expect(res.body.data).toBeNull(); @@ -348,8 +324,6 @@ describe('server /api/v2/mcp', () => { it('maps the engine config.invalid rejection to 40001', async () => { const stub = makeMcpStub(); - // A zod-valid body whose engine-side write then fails the config layer - // (e.g. a corrupt user mcp.json) surfaces as config.invalid. stub.service.addServer = async () => { throw new Error2( ErrorCodes.CONFIG_INVALID, @@ -366,8 +340,6 @@ describe('server /api/v2/mcp', () => { it('maps a delete rejected with mcp.server_not_found to 40408', async () => { const stub = makeMcpStub(); - // The engine's removeServer no-ops on unknown names today; drive the - // route's documented 40408 leg with the domain error directly. stub.service.removeServer = async (name) => { throw new Error2(ErrorCodes.MCP_SERVER_NOT_FOUND, `MCP server "${name}" was not found`); };