smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): addressed review findings for runtime MCP support

Browse Source 
- Removed unsafe OAuth endpoint extraction from error message text
- Fixed PKCE verifier storage with typed #codeVerifier field
- Fixed refresh token fallback using access token as refresh token
- Enforced restrictive file permissions (0o700/0o600) for MCP configs
- Fixed wizard buildConfig() to respect user-chosen env var and header names
- Fixed reauth endpoint discovery for non-OAuth servers
- Stored original config on connection, resolved config only for transport
- Added runtime type validation for enabled/timeout in config loaders
- Converted all TS private keywords to ES # private fields
- Wrapped uncaught throws in /mcp add with try/catch error handling
- Replaced new Promise with Promise.withResolvers() pattern
- Sanitized TUI output with replaceTabs/truncateToWidth
- Enforced http/https URL validation in add wizard
- Fixed greedy /mcp prefix match in input controller
- Corrected config filename references in MCP guide
- Added server name validation to updateMCPServer
- Fixed timeout timer leak in stdio transport
This commit is contained in:
can1357
2026-02-10 14:31:19 +01:00
parent 7e4bedf18b
commit 4da2acbee4
11 changed files with 887 additions and 785 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/MCP_COMMAND_GUIDE.md
+1 -1
View File
@@ -301,7 +301,7 @@ Project-specific configuration (usually in project root).
3. **Secure sensitive data**
- Use OAuth when available
- Use shell commands for API keys: `!op read op://vault/key`
- Never commit `.mcp.json` files with plain API keys to version control
- Never commit `.omp/mcp.json` files with plain API keys to version control
4. **Name servers descriptively**
- Use purpose-based names: "github-tools", "docs-search"
packages/coding-agent/src/discovery/builtin.ts
+47 -2
View File
@@ -5,6 +5,7 @@
* .pi is an alias for backwards compatibility.
*/
import * as path from "node:path";
import { logger } from "@oh-my-pi/pi-utils";
import { registerProvider } from "../capability";
import { type ContextFile, contextFileCapability } from "../capability/context-file";
import { type Extension, type ExtensionManifest, extensionCapability } from "../capability/extension";
@@ -79,10 +80,54 @@ async function loadMCPServers(ctx: LoadContext): Promise<LoadResult<MCPServer>>
const expanded = expandEnvVarsDeep(data.mcpServers);
for (const [serverName, config] of Object.entries(expanded)) {
const serverConfig = config as Record<string, unknown>;
// Validate enabled: coerce string "true"/"false", warn on other types
let enabled: boolean | undefined;
if (serverConfig.enabled === undefined || serverConfig.enabled === null) {
enabled = undefined;
} else if (typeof serverConfig.enabled === "boolean") {
enabled = serverConfig.enabled;
} else if (typeof serverConfig.enabled === "string") {
const lower = serverConfig.enabled.toLowerCase();
if (lower === "false" || lower === "0") enabled = false;
else if (lower === "true" || lower === "1") enabled = true;
else {
logger.warn(`MCP server "${serverName}": invalid enabled value "${serverConfig.enabled}", ignoring`);
enabled = undefined;
}
} else {
logger.warn(`MCP server "${serverName}": invalid enabled type ${typeof serverConfig.enabled}, ignoring`);
enabled = undefined;
}
// Validate timeout: coerce numeric strings, warn on invalid
let timeout: number | undefined;
if (serverConfig.timeout === undefined || serverConfig.timeout === null) {
timeout = undefined;
} else if (typeof serverConfig.timeout === "number") {
if (Number.isFinite(serverConfig.timeout) && serverConfig.timeout > 0) {
timeout = serverConfig.timeout;
} else {
logger.warn(`MCP server "${serverName}": invalid timeout ${serverConfig.timeout}, ignoring`);
timeout = undefined;
}
} else if (typeof serverConfig.timeout === "string") {
const parsed = Number(serverConfig.timeout);
if (Number.isFinite(parsed) && parsed > 0) {
timeout = parsed;
} else {
logger.warn(`MCP server "${serverName}": invalid timeout "${serverConfig.timeout}", ignoring`);
timeout = undefined;
}
} else {
logger.warn(`MCP server "${serverName}": invalid timeout type ${typeof serverConfig.timeout}, ignoring`);
timeout = undefined;
}
result.push({
name: serverName,
enabled: serverConfig.enabled as boolean | undefined,
timeout: serverConfig.timeout as number | undefined,
enabled,
timeout,
command: serverConfig.command as string | undefined,
args: serverConfig.args as string[] | undefined,
env: serverConfig.env as Record<string, string> | undefined,
packages/coding-agent/src/discovery/mcp-json.ts
+26 -2
View File
@@ -7,6 +7,7 @@
* Priority: 5 (low, as this is a fallback after tool-specific providers)
*/
import * as path from "node:path";
import { logger } from "@oh-my-pi/pi-utils";
import { registerProvider } from "../capability";
import { readFile } from "../capability/fs";
import { type MCPServer, mcpCapability } from "../capability/mcp";
@@ -47,10 +48,33 @@ function transformMCPConfig(config: MCPConfigFile, source: SourceMeta): MCPServe
if (config.mcpServers) {
for (const [name, serverConfig] of Object.entries(config.mcpServers)) {
// Runtime type validation for user-controlled JSON values
let enabled: boolean | undefined;
if (serverConfig.enabled !== undefined) {
if (typeof serverConfig.enabled === "boolean") {
enabled = serverConfig.enabled;
} else {
logger.warn("MCP server has invalid 'enabled' value, ignoring", { name, value: serverConfig.enabled });
}
}
let timeout: number | undefined;
if (serverConfig.timeout !== undefined) {
if (
typeof serverConfig.timeout === "number" &&
Number.isFinite(serverConfig.timeout) &&
serverConfig.timeout > 0
) {
timeout = serverConfig.timeout;
} else {
logger.warn("MCP server has invalid 'timeout' value, ignoring", { name, value: serverConfig.timeout });
}
}
const server: MCPServer = {
name,
enabled: serverConfig.enabled,
timeout: serverConfig.timeout,
enabled,
timeout,
command: serverConfig.command,
args: serverConfig.args,
env: serverConfig.env,
packages/coding-agent/src/mcp/config-writer.ts
+13 -6
View File
@@ -1,11 +1,12 @@
/**
* MCP Configuration File Writer
*
* Utilities for reading/writing .mcp.json files at user or project level.
* Utilities for reading/writing .omp/mcp.json files at user or project level.
*/
import * as fs from "node:fs";
import { homedir } from "node:os";
import * as os from "node:os";
import * as path from "node:path";
import { isEnoent } from "@oh-my-pi/pi-utils";
import { validateServerConfig } from "./config";
import type { MCPConfigFile, MCPServerConfig } from "./types";
@@ -16,7 +17,7 @@ import type { MCPConfigFile, MCPServerConfig } from "./types";
*/
export function getMCPConfigPath(scope: "user" | "project", cwd: string): string {
if (scope === "user") {
return path.join(homedir(), ".omp", "mcp.json");
return path.join(os.homedir(), ".omp", "mcp.json");
}
return path.join(cwd, ".omp", "mcp.json");
}
@@ -31,7 +32,7 @@ export async function readMCPConfigFile(filePath: string): Promise<MCPConfigFile
const parsed = JSON.parse(content) as MCPConfigFile;
return parsed;
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
if (isEnoent(error)) {
// File doesn't exist, return empty config
return { mcpServers: {} };
}
@@ -46,12 +47,12 @@ export async function readMCPConfigFile(filePath: string): Promise<MCPConfigFile
export async function writeMCPConfigFile(filePath: string, config: MCPConfigFile): Promise<void> {
// Ensure parent directory exists
const dir = path.dirname(filePath);
await fs.promises.mkdir(dir, { recursive: true });
await fs.promises.mkdir(dir, { recursive: true, mode: 0o700 });
// Write to temp file first (atomic write)
const tmpPath = `${filePath}.tmp`;
const content = JSON.stringify(config, null, 2);
await fs.promises.writeFile(tmpPath, content, "utf-8");
await fs.promises.writeFile(tmpPath, content, { encoding: "utf-8", mode: 0o600 });
// Rename to final path (atomic on most systems)
await fs.promises.rename(tmpPath, filePath);
@@ -122,6 +123,12 @@ export async function addMCPServer(filePath: string, name: string, config: MCPSe
* @throws Error if validation fails
*/
export async function updateMCPServer(filePath: string, name: string, config: MCPServerConfig): Promise<void> {
// Validate server name
const nameError = validateServerName(name);
if (nameError) {
throw new Error(nameError);
}
// Validate the config
const errors = validateServerConfig(name, config);
if (errors.length > 0) {
packages/coding-agent/src/mcp/manager.ts
+10 -7
View File
@@ -83,7 +83,7 @@ export class MCPManager {
#pendingConnections = new Map<string, Promise<MCPServerConnection>>();
#pendingToolLoads = new Map<string, Promise<ToolLoadResult>>();
#sources = new Map<string, SourceMeta>();
private authStorage: AuthStorage | null = null;
#authStorage: AuthStorage | null = null;
constructor(
private cwd: string,
@@ -94,7 +94,7 @@ export class MCPManager {
* Set the auth storage for resolving OAuth credentials.
*/
setAuthStorage(authStorage: AuthStorage): void {
this.authStorage = authStorage;
this.#authStorage = authStorage;
}
/**
@@ -164,10 +164,13 @@ export class MCPManager {
}
// Resolve auth config before connecting
const resolvedConfig = await this.resolveAuthConfig(config);
const resolvedConfig = await this.#resolveAuthConfig(config);
const connectionPromise = connectToServer(name, resolvedConfig).then(
connection => {
// Store original config (without resolved tokens) to keep
// cache keys stable and avoid leaking rotating credentials.
connection.config = config;
if (sources[name]) {
connection._source = sources[name];
}
@@ -329,7 +332,7 @@ export class MCPManager {
* Resolve auth and shell-command substitutions in config before connecting.
*/
async prepareConfig(config: MCPServerConfig): Promise<MCPServerConfig> {
return this.resolveAuthConfig(config);
return this.#resolveAuthConfig(config);
}
/**
@@ -401,14 +404,14 @@ export class MCPManager {
/**
* Resolve OAuth credentials and shell commands in config.
*/
private async resolveAuthConfig(config: MCPServerConfig): Promise<MCPServerConfig> {
async #resolveAuthConfig(config: MCPServerConfig): Promise<MCPServerConfig> {
let resolved: MCPServerConfig = { ...config };
const auth = config.auth;
if (auth?.type === "oauth" && auth.credentialId && this.authStorage) {
if (auth?.type === "oauth" && auth.credentialId && this.#authStorage) {
const credentialId = auth.credentialId;
try {
const credential = this.authStorage.get(credentialId);
const credential = this.#authStorage.get(credentialId);
if (credential?.type === "oauth") {
if (resolved.type === "http" || resolved.type === "sse") {
resolved = {
packages/coding-agent/src/mcp/oauth-discovery.ts
-14
View File
@@ -166,20 +166,6 @@ export function extractOAuthEndpoints(error: Error): OAuthEndpoints | null {
};
}
// Try to extract URLs from error message
const urlPattern = /(https?:\/\/[^\s"'<>]+)/g;
const urls = errorMsg.match(urlPattern);
if (urls && urls.length >= 2) {
// Heuristic: First URL is likely auth, second is token
return {
authorizationUrl: urls[0],
tokenUrl: urls[1],
clientId: clientIdFromAuthUrl(urls[0]),
scopes: scopeFromAuthUrl(urls[0]),
};
}
return null;
}
packages/coding-agent/src/mcp/oauth-flow.ts
+29 -28
View File
@@ -29,20 +29,21 @@ export interface MCPOAuthConfig {
* Supports standard OAuth 2.0 authorization code flow with PKCE.
*/
export class MCPOAuthFlow extends OAuthCallbackFlow {
private resolvedClientId?: string;
private registeredClientSecret?: string;
#resolvedClientId?: string;
#registeredClientSecret?: string;
#codeVerifier?: string;
constructor(
private config: MCPOAuthConfig,
ctrl: OAuthController,
) {
super(ctrl, DEFAULT_PORT, CALLBACK_PATH);
this.resolvedClientId = this.resolveClientId(config);
this.#resolvedClientId = this.#resolveClientId(config);
}
async generateAuthUrl(state: string, redirectUri: string): Promise<{ url: string; instructions?: string }> {
if (!this.resolvedClientId) {
await this.tryRegisterClient(redirectUri);
if (!this.#resolvedClientId) {
await this.#tryRegisterClient(redirectUri);
}
const authUrl = new URL(this.config.authorizationUrl);
@@ -51,8 +52,8 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
if (!params.get("response_type")) {
params.set("response_type", "code");
}
if (this.resolvedClientId && !params.get("client_id")) {
params.set("client_id", this.resolvedClientId);
if (this.#resolvedClientId && !params.get("client_id")) {
params.set("client_id", this.#resolvedClientId);
}
if (this.config.scopes && !params.get("scope")) {
params.set("scope", this.config.scopes);
@@ -61,13 +62,13 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
params.set("state", state);
// Add PKCE challenge (some providers require it)
const codeVerifier = this.generateCodeVerifier();
const codeChallenge = await this.generateCodeChallenge(codeVerifier);
const codeVerifier = this.#generateCodeVerifier();
const codeChallenge = await this.#generateCodeChallenge(codeVerifier);
params.set("code_challenge", codeChallenge);
params.set("code_challenge_method", "S256");
// Store code verifier for token exchange
(this as any).codeVerifier = codeVerifier;
this.#codeVerifier = codeVerifier;
return { url: authUrl.toString() };
}
@@ -78,18 +79,18 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
code,
redirect_uri: redirectUri,
});
if (this.resolvedClientId) {
params.set("client_id", this.resolvedClientId);
if (this.#resolvedClientId) {
params.set("client_id", this.#resolvedClientId);
}
// Add code verifier for PKCE
const codeVerifier = (this as any).codeVerifier;
if (codeVerifier) {
params.set("code_verifier", codeVerifier);
if (this.#codeVerifier) {
params.set("code_verifier", this.#codeVerifier);
}
this.#codeVerifier = undefined;
// Add client secret if provided
const clientSecret = this.config.clientSecret ?? this.registeredClientSecret;
const clientSecret = this.config.clientSecret ?? this.#registeredClientSecret;
if (clientSecret) {
params.set("client_secret", clientSecret);
}
@@ -120,7 +121,7 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
return {
access: data.access_token,
refresh: data.refresh_token ?? data.access_token, // Fallback to access token if no refresh
refresh: data.refresh_token ?? "",
expires,
};
}
@@ -128,31 +129,31 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
/**
* Generate PKCE code verifier (random string).
*/
private generateCodeVerifier(): string {
#generateCodeVerifier(): string {
const bytes = new Uint8Array(32);
crypto.getRandomValues(bytes);
return this.base64UrlEncode(bytes);
return this.#base64UrlEncode(bytes);
}
/**
* Generate PKCE code challenge from verifier.
*/
private async generateCodeChallenge(verifier: string): Promise<string> {
async #generateCodeChallenge(verifier: string): Promise<string> {
const encoder = new TextEncoder();
const data = encoder.encode(verifier);
const hash = await crypto.subtle.digest("SHA-256", data);
return this.base64UrlEncode(new Uint8Array(hash));
return this.#base64UrlEncode(new Uint8Array(hash));
}
/**
* Base64 URL encode (without padding).
*/
private base64UrlEncode(bytes: Uint8Array): string {
#base64UrlEncode(bytes: Uint8Array): string {
const base64 = btoa(String.fromCharCode(...bytes));
return base64.replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, "");
}
private resolveClientId(config: MCPOAuthConfig): string | undefined {
#resolveClientId(config: MCPOAuthConfig): string | undefined {
const fromConfig = config.clientId?.trim();
if (fromConfig) return fromConfig;
@@ -166,8 +167,8 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
/**
* Try OAuth dynamic client registration when provider requires a client_id.
*/
private async tryRegisterClient(redirectUri: string): Promise<void> {
const registrationEndpoint = await this.resolveRegistrationEndpoint();
async #tryRegisterClient(redirectUri: string): Promise<void> {
const registrationEndpoint = await this.#resolveRegistrationEndpoint();
if (!registrationEndpoint) return;
try {
@@ -195,17 +196,17 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
};
if (data.client_id && data.client_id.trim() !== "") {
this.resolvedClientId = data.client_id;
this.#resolvedClientId = data.client_id;
}
if (data.client_secret && data.client_secret.trim() !== "") {
this.registeredClientSecret = data.client_secret;
this.#registeredClientSecret = data.client_secret;
}
} catch {
// Ignore registration failures and continue without client registration.
}
}
private async resolveRegistrationEndpoint(): Promise<string | null> {
async #resolveRegistrationEndpoint(): Promise<string | null> {
try {
const authorizationEndpoint = new URL(this.config.authorizationUrl);
const metadataUrl = new URL("/.well-known/oauth-authorization-server", authorizationEndpoint.origin);
packages/coding-agent/src/mcp/transports/stdio.ts
+28 -23
View File
@@ -161,30 +161,35 @@ export class StdioTransport implements MCPTransport {
const timeout = this.config.timeout ?? 30000;
return Promise.race([
new Promise<T>((resolve, reject) => {
this.#pendingRequests.set(id, {
resolve: resolve as (value: unknown) => void,
reject,
});
let timer: NodeJS.Timeout | undefined;
try {
return await Promise.race([
new Promise<T>((resolve, reject) => {
this.#pendingRequests.set(id, {
resolve: resolve as (value: unknown) => void,
reject,
});
const message = `${JSON.stringify(request)}\n`;
try {
// Bun's FileSink has write() method directly
this.#process!.stdin.write(message);
this.#process!.stdin.flush();
} catch (error: unknown) {
this.#pendingRequests.delete(id);
reject(error);
}
}),
new Promise<never>((_, reject) =>
setTimeout(() => {
this.#pendingRequests.delete(id);
reject(new Error(`Request timeout after ${timeout}ms`));
}, timeout),
),
]);
const message = `${JSON.stringify(request)}\n`;
try {
// Bun's FileSink has write() method directly
this.#process!.stdin.write(message);
this.#process!.stdin.flush();
} catch (error: unknown) {
this.#pendingRequests.delete(id);
reject(error);
}
}),
new Promise<never>((_, reject) => {
timer = setTimeout(() => {
this.#pendingRequests.delete(id);
reject(new Error(`Request timeout after ${timeout}ms`));
}, timeout);
}),
]);
} finally {
clearTimeout(timer);
}
}
async notify(method: string, params?: Record<string, unknown>): Promise<void> {
packages/coding-agent/src/modes/components/mcp-add-wizard.ts
+603 -589
View File
File diff suppressed because it is too large Load Diff
packages/coding-agent/src/modes/controllers/input-controller.ts
+1 -1
View File
@@ -334,7 +334,7 @@ export class InputController {
}
// Handle MCP server management commands
if (text.startsWith("/mcp")) {
if (text === "/mcp" || text.startsWith("/mcp ")) {
this.ctx.editor.addToHistory(text);
this.ctx.editor.setText("");
await this.ctx.handleMCPCommand(text);
packages/coding-agent/src/modes/controllers/mcp-command-controller.ts
+129 -112
View File
@@ -83,38 +83,38 @@ export class MCPCommandController {
const subcommand = parts[1]?.toLowerCase();
if (!subcommand || subcommand === "help") {
this.showHelp();
this.#showHelp();
return;
}
switch (subcommand) {
case "add":
await this.handleAdd(text);
await this.#handleAdd(text);
break;
case "list":
await this.handleList();
await this.#handleList();
break;
case "remove":
case "rm":
await this.handleRemove(text);
await this.#handleRemove(text);
break;
case "test":
await this.handleTest(parts[2]);
await this.#handleTest(parts[2]);
break;
case "reauth":
await this.handleReauth(parts[2]);
await this.#handleReauth(parts[2]);
break;
case "unauth":
await this.handleUnauth(parts[2]);
await this.#handleUnauth(parts[2]);
break;
case "enable":
await this.handleSetEnabled(parts[2], true);
await this.#handleSetEnabled(parts[2], true);
break;
case "disable":
await this.handleSetEnabled(parts[2], false);
await this.#handleSetEnabled(parts[2], false);
break;
case "reload":
await this.handleReload();
await this.#handleReload();
break;
default:
this.ctx.showError(`Unknown subcommand: ${subcommand}. Type /mcp help for usage.`);
@@ -124,7 +124,7 @@ export class MCPCommandController {
/**
* Show help text
*/
private showHelp(): void {
#showHelp(): void {
const helpText = [
"",
theme.bold("MCP Server Management"),
@@ -146,10 +146,10 @@ export class MCPCommandController {
"",
].join("\n");
this.showMessage(helpText);
this.#showMessage(helpText);
}
private parseAddCommand(text: string): MCPAddParsed {
#parseAddCommand(text: string): MCPAddParsed {
const prefixMatch = text.match(/^\/mcp\s+add\b\s*(.*)$/i);
const rest = prefixMatch?.[1]?.trim() ?? "";
if (!rest) {
@@ -265,8 +265,8 @@ export class MCPCommandController {
/**
* Handle /mcp add - Launch interactive wizard or quick-add from args
*/
private async handleAdd(text: string): Promise<void> {
const parsed = this.parseAddCommand(text);
async #handleAdd(text: string): Promise<void> {
const parsed = this.#parseAddCommand(text);
if (parsed.error) {
this.ctx.showError(parsed.error);
return;
@@ -278,10 +278,13 @@ export class MCPCommandController {
// matching wizard behavior. Command quick-add intentionally skips this.
if (!parsed.isCommandQuickAdd && (finalConfig.type === "http" || finalConfig.type === "sse")) {
try {
await this.handleTestConnection(finalConfig);
await this.#handleTestConnection(finalConfig);
} catch (error) {
if (parsed.hasAuthToken) {
throw error;
this.ctx.showError(
`Authentication failed for "${parsed.initialName}": ${error instanceof Error ? error.message : String(error)}`,
);
return;
}
const authResult = analyzeAuthError(error as Error);
if (authResult.requiresAuth) {
@@ -295,31 +298,39 @@ export class MCPCommandController {
}
if (!oauth) {
throw new Error(
this.ctx.showError(
`Authentication required for "${parsed.initialName}", but OAuth endpoints could not be discovered. ` +
`Use /mcp add ${parsed.initialName} (wizard) or configure auth manually.`,
);
return;
}
const credentialId = await this.handleOAuthFlow(
oauth.authorizationUrl,
oauth.tokenUrl,
oauth.clientId ?? "",
"",
oauth.scopes ?? "",
);
finalConfig = {
...finalConfig,
auth: {
type: "oauth",
credentialId,
},
};
try {
const credentialId = await this.#handleOAuthFlow(
oauth.authorizationUrl,
oauth.tokenUrl,
oauth.clientId ?? "",
"",
oauth.scopes ?? "",
);
finalConfig = {
...finalConfig,
auth: {
type: "oauth",
credentialId,
},
};
} catch (oauthError) {
this.ctx.showError(
`OAuth flow failed for "${parsed.initialName}": ${oauthError instanceof Error ? oauthError.message : String(oauthError)}`,
);
return;
}
}
}
}
await this.handleWizardComplete(parsed.initialName, finalConfig, parsed.scope);
await this.#handleWizardComplete(parsed.initialName, finalConfig, parsed.scope);
return;
}
@@ -334,17 +345,17 @@ export class MCPCommandController {
const wizard = new MCPAddWizard(
async (name: string, config: MCPServerConfig, scope: "user" | "project") => {
done();
await this.handleWizardComplete(name, config, scope);
await this.#handleWizardComplete(name, config, scope);
},
() => {
done();
this.handleWizardCancel();
this.#handleWizardCancel();
},
async (authUrl: string, tokenUrl: string, clientId: string, clientSecret: string, scopes: string) => {
return await this.handleOAuthFlow(authUrl, tokenUrl, clientId, clientSecret, scopes);
return await this.#handleOAuthFlow(authUrl, tokenUrl, clientId, clientSecret, scopes);
},
async (config: MCPServerConfig) => {
return await this.handleTestConnection(config);
return await this.#handleTestConnection(config);
},
() => {
this.ctx.ui.requestRender();
@@ -362,7 +373,7 @@ export class MCPCommandController {
/**
* Handle OAuth authentication flow for MCP server
*/
private async handleOAuthFlow(
async #handleOAuthFlow(
authUrl: string,
tokenUrl: string,
clientId: string,
@@ -495,12 +506,7 @@ export class MCPCommandController {
);
// Execute OAuth flow with 5 minute timeout
const credentials = await Promise.race([
flow.login(),
new Promise<never>((_, reject) =>
setTimeout(() => reject(new Error("OAuth flow timed out after 5 minutes")), 5 * 60 * 1000),
),
]);
const credentials = await withTimeout(flow.login(), 5 * 60 * 1000, "OAuth flow timed out after 5 minutes");
this.ctx.chatContainer.addChild(new Spacer(1));
this.ctx.chatContainer.addChild(new Text(theme.fg("success", "✓ Authorization completed in browser."), 1, 0));
@@ -541,7 +547,7 @@ export class MCPCommandController {
* Test connection to an MCP server.
* Throws an error if connection fails (used for auto-detection).
*/
private async handleTestConnection(config: MCPServerConfig): Promise<void> {
async #handleTestConnection(config: MCPServerConfig): Promise<void> {
// Create temporary connection using a test name
const testName = `test_${Date.now()}`;
let resolvedConfig: MCPServerConfig;
@@ -557,7 +563,7 @@ export class MCPCommandController {
await disconnectServer(connection);
}
private async findConfiguredServer(
async #findConfiguredServer(
name: string,
): Promise<{ filePath: string; scope: "user" | "project"; config: MCPServerConfig } | null> {
const cwd = process.cwd();
@@ -578,43 +584,54 @@ export class MCPCommandController {
return null;
}
private async removeManagedOAuthCredential(credentialId: string | undefined): Promise<void> {
async #removeManagedOAuthCredential(credentialId: string | undefined): Promise<void> {
if (!credentialId || !credentialId.startsWith("mcp_oauth_")) return;
await this.ctx.session.modelRegistry.authStorage.remove(credentialId);
}
private stripOAuthAuth(config: MCPServerConfig): MCPServerConfig {
#stripOAuthAuth(config: MCPServerConfig): MCPServerConfig {
const next = { ...config } as MCPServerConfig & { auth?: { type: "oauth" | "apikey"; credentialId?: string } };
delete next.auth;
return next;
}
private async resolveOAuthEndpointsFromServer(config: MCPServerConfig): Promise<{
async #resolveOAuthEndpointsFromServer(config: MCPServerConfig): Promise<{
authorizationUrl: string;
tokenUrl: string;
clientId?: string;
scopes?: string;
}> {
// First test if server actually needs auth by connecting without OAuth
let connectionSucceeded = false;
let connectionError: Error | undefined;
try {
await this.handleTestConnection(this.stripOAuthAuth(config));
throw new Error("Server connection succeeded without OAuth; reauthorization is not required.");
await this.#handleTestConnection(this.#stripOAuthAuth(config));
connectionSucceeded = true;
} catch (error) {
const authResult = analyzeAuthError(error as Error);
let oauth = authResult.authType === "oauth" ? (authResult.oauth ?? null) : null;
if (!oauth && (config.type === "http" || config.type === "sse") && config.url) {
oauth = await discoverOAuthEndpoints(config.url);
}
if (!oauth) {
throw new Error("Could not discover OAuth endpoints from server response.");
}
return oauth;
connectionError = error as Error;
}
// Server connected fine without auth — reauth is not needed
if (connectionSucceeded) {
throw new Error("Server connection succeeded without OAuth; reauthorization is not required.");
}
// Analyze the connection error to extract OAuth endpoints
const authResult = analyzeAuthError(connectionError!);
let oauth = authResult.authType === "oauth" ? (authResult.oauth ?? null) : null;
if (!oauth && (config.type === "http" || config.type === "sse") && config.url) {
oauth = await discoverOAuthEndpoints(config.url);
}
if (!oauth) {
throw new Error("Could not discover OAuth endpoints from server response.");
}
return oauth;
}
private async waitForServerConnectionWithAnimation(
async #waitForServerConnectionWithAnimation(
name: string,
options?: { suppressDisconnectedWarning?: boolean },
): Promise<"connected" | "connecting" | "disconnected"> {
@@ -662,7 +679,7 @@ export class MCPCommandController {
}
}
private async syncManagerConnection(name: string, config: MCPServerConfig): Promise<void> {
async #syncManagerConnection(name: string, config: MCPServerConfig): Promise<void> {
if (!this.ctx.mcpManager) return;
if (this.ctx.mcpManager.getConnectionStatus(name) !== "disconnected") return;
await this.ctx.mcpManager.connectServers({ [name]: config }, {});
@@ -671,7 +688,7 @@ export class MCPCommandController {
}
}
private async handleWizardComplete(name: string, config: MCPServerConfig, scope: "user" | "project"): Promise<void> {
async #handleWizardComplete(name: string, config: MCPServerConfig, scope: "user" | "project"): Promise<void> {
try {
// Determine file path
const cwd = process.cwd();
@@ -681,11 +698,11 @@ export class MCPCommandController {
await addMCPServer(filePath, name, config);
// Reload MCP manager
await this.reloadMCP();
await this.#reloadMCP();
const state =
config.enabled === false
? "disconnected"
: await this.waitForServerConnectionWithAnimation(name, { suppressDisconnectedWarning: true });
: await this.#waitForServerConnectionWithAnimation(name, { suppressDisconnectedWarning: true });
let isConnected = state === "connected";
const isConnecting = state === "connecting";
@@ -693,9 +710,9 @@ export class MCPCommandController {
// report as connected to avoid false-negative messaging.
if (!isConnected && !isConnecting && config.enabled !== false) {
try {
await this.handleTestConnection(config);
await this.#handleTestConnection(config);
isConnected = true;
await this.syncManagerConnection(name, config);
await this.#syncManagerConnection(name, config);
} catch {
// Keep disconnected status
}
@@ -721,7 +738,7 @@ export class MCPCommandController {
lines.push(theme.fg("muted", `Run ${theme.fg("accent", "/mcp list")} to see all configured servers.`));
lines.push("");
this.showMessage(lines.join("\n"));
this.#showMessage(lines.join("\n"));
} catch (error) {
const errorMsg = error instanceof Error ? error.message : String(error);
@@ -739,8 +756,8 @@ export class MCPCommandController {
}
}
private handleWizardCancel(): void {
this.showMessage(
#handleWizardCancel(): void {
this.#showMessage(
[
"",
theme.fg("muted", "Server creation cancelled."),
@@ -754,7 +771,7 @@ export class MCPCommandController {
/**
* Handle /mcp list - Show all configured servers
*/
private async handleList(): Promise<void> {
async #handleList(): Promise<void> {
try {
const cwd = process.cwd();
@@ -771,7 +788,7 @@ export class MCPCommandController {
const projectServers = Object.keys(projectConfig.mcpServers ?? {});
if (userServers.length === 0 && projectServers.length === 0) {
this.showMessage(
this.#showMessage(
[
"",
theme.fg("muted", "No MCP servers configured."),
@@ -831,7 +848,7 @@ export class MCPCommandController {
lines.push("");
}
this.showMessage(lines.join("\n"));
this.#showMessage(lines.join("\n"));
} catch (error) {
this.ctx.showError(`Failed to list servers: ${error instanceof Error ? error.message : String(error)}`);
}
@@ -840,7 +857,7 @@ export class MCPCommandController {
/**
* Handle /mcp remove <name> - Remove a server
*/
private async handleRemove(text: string): Promise<void> {
async #handleRemove(text: string): Promise<void> {
const match = text.match(/^\/mcp\s+(?:remove|rm)\b\s*(.*)$/i);
const rest = match?.[1]?.trim() ?? "";
const tokens = parseCommandArgs(rest);
@@ -895,9 +912,9 @@ export class MCPCommandController {
await removeMCPServer(filePath, name);
// Reload MCP manager
await this.reloadMCP();
await this.#reloadMCP();
this.showMessage(["", theme.fg("success", `✓ Removed server "${name}" from ${scope} config`), ""].join("\n"));
this.#showMessage(["", theme.fg("success", `✓ Removed server "${name}" from ${scope} config`), ""].join("\n"));
} catch (error) {
this.ctx.showError(`Failed to remove server: ${error instanceof Error ? error.message : String(error)}`);
}
@@ -906,7 +923,7 @@ export class MCPCommandController {
/**
* Handle /mcp test <name> - Test connection to a server
*/
private async handleTest(name: string | undefined): Promise<void> {
async #handleTest(name: string | undefined): Promise<void> {
if (!name) {
this.ctx.showError("Server name required. Usage: /mcp test <name>");
return;
@@ -936,7 +953,7 @@ export class MCPCommandController {
return;
}
this.showMessage(["", theme.fg("muted", `Testing connection to "${name}"...`), ""].join("\n"));
this.#showMessage(["", theme.fg("muted", `Testing connection to "${name}"...`), ""].join("\n"));
// Resolve auth config if needed
let resolvedConfig: MCPServerConfig;
@@ -973,8 +990,8 @@ export class MCPCommandController {
}
lines.push("");
await this.syncManagerConnection(name, config);
this.showMessage(lines.join("\n"));
await this.#syncManagerConnection(name, config);
this.#showMessage(lines.join("\n"));
} finally {
// Disconnect test connection
await disconnectServer(connection);
@@ -1000,21 +1017,21 @@ export class MCPCommandController {
}
}
private async handleSetEnabled(name: string | undefined, enabled: boolean): Promise<void> {
async #handleSetEnabled(name: string | undefined, enabled: boolean): Promise<void> {
if (!name) {
this.ctx.showError(`Server name required. Usage: /mcp ${enabled ? "enable" : "disable"} <name>`);
return;
}
try {
const found = await this.findConfiguredServer(name);
const found = await this.#findConfiguredServer(name);
if (!found) {
this.ctx.showError(`Server "${name}" not found.`);
return;
}
if ((found.config.enabled ?? true) === enabled) {
this.showMessage(
this.#showMessage(
["", theme.fg("muted", `Server "${name}" is already ${enabled ? "enabled" : "disabled"}.`), ""].join(
"\n",
),
@@ -1024,11 +1041,11 @@ export class MCPCommandController {
const updated: MCPServerConfig = { ...found.config, enabled };
await updateMCPServer(found.filePath, name, updated);
await this.reloadMCP();
await this.#reloadMCP();
let status = "";
if (enabled) {
const state = await this.waitForServerConnectionWithAnimation(name);
const state = await this.#waitForServerConnectionWithAnimation(name);
status =
state === "connected"
? theme.fg("success", "Connected")
@@ -1046,7 +1063,7 @@ export class MCPCommandController {
lines.push(` Status: ${status}`);
}
lines.push("");
this.showMessage(lines.join("\n"));
this.#showMessage(lines.join("\n"));
} catch (error) {
this.ctx.showError(
`Failed to ${enabled ? "enable" : "disable"} server: ${error instanceof Error ? error.message : String(error)}`,
@@ -1054,14 +1071,14 @@ export class MCPCommandController {
}
}
private async handleUnauth(name: string | undefined): Promise<void> {
async #handleUnauth(name: string | undefined): Promise<void> {
if (!name) {
this.ctx.showError("Server name required. Usage: /mcp unauth <name>");
return;
}
try {
const found = await this.findConfiguredServer(name);
const found = await this.#findConfiguredServer(name);
if (!found) {
this.ctx.showError(`Server "${name}" not found.`);
return;
@@ -1071,14 +1088,14 @@ export class MCPCommandController {
found.config as MCPServerConfig & { auth?: { type: "oauth" | "apikey"; credentialId?: string } }
).auth;
if (currentAuth?.type === "oauth") {
await this.removeManagedOAuthCredential(currentAuth.credentialId);
await this.#removeManagedOAuthCredential(currentAuth.credentialId);
}
const updated = this.stripOAuthAuth(found.config);
const updated = this.#stripOAuthAuth(found.config);
await updateMCPServer(found.filePath, name, updated);
await this.reloadMCP();
await this.#reloadMCP();
this.showMessage(
this.#showMessage(
["", theme.fg("success", `✓ Cleared auth for "${name}" (${found.scope} config)`), ""].join("\n"),
);
} catch (error) {
@@ -1086,14 +1103,14 @@ export class MCPCommandController {
}
}
private async handleReauth(name: string | undefined): Promise<void> {
async #handleReauth(name: string | undefined): Promise<void> {
if (!name) {
this.ctx.showError("Server name required. Usage: /mcp reauth <name>");
return;
}
try {
const found = await this.findConfiguredServer(name);
const found = await this.#findConfiguredServer(name);
if (!found) {
this.ctx.showError(`Server "${name}" not found.`);
return;
@@ -1108,15 +1125,15 @@ export class MCPCommandController {
found.config as MCPServerConfig & { auth?: { type: "oauth" | "apikey"; credentialId?: string } }
).auth;
if (currentAuth?.type === "oauth") {
await this.removeManagedOAuthCredential(currentAuth.credentialId);
await this.#removeManagedOAuthCredential(currentAuth.credentialId);
}
const baseConfig = this.stripOAuthAuth(found.config);
const oauth = await this.resolveOAuthEndpointsFromServer(baseConfig);
const baseConfig = this.#stripOAuthAuth(found.config);
const oauth = await this.#resolveOAuthEndpointsFromServer(baseConfig);
this.showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n"));
this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n"));
const credentialId = await this.handleOAuthFlow(
const credentialId = await this.#handleOAuthFlow(
oauth.authorizationUrl,
oauth.tokenUrl,
oauth.clientId ?? "",
@@ -1132,8 +1149,8 @@ export class MCPCommandController {
},
};
await updateMCPServer(found.filePath, name, updated);
await this.reloadMCP();
const state = await this.waitForServerConnectionWithAnimation(name);
await this.#reloadMCP();
const state = await this.#waitForServerConnectionWithAnimation(name);
const lines = [
"",
@@ -1148,18 +1165,18 @@ export class MCPCommandController {
}`,
"",
];
this.showMessage(lines.join("\n"));
this.#showMessage(lines.join("\n"));
} catch (error) {
this.ctx.showError(`Failed to reauthorize server: ${error instanceof Error ? error.message : String(error)}`);
}
}
private async handleReload(): Promise<void> {
async #handleReload(): Promise<void> {
try {
this.showMessage(["", theme.fg("muted", "Reloading MCP servers and runtime tools..."), ""].join("\n"));
await this.reloadMCP();
this.#showMessage(["", theme.fg("muted", "Reloading MCP servers and runtime tools..."), ""].join("\n"));
await this.#reloadMCP();
const connectedCount = this.ctx.mcpManager?.getConnectedServers().length ?? 0;
this.showMessage(
this.#showMessage(
["", theme.fg("success", "✓ MCP reload complete"), ` Connected servers: ${connectedCount}`, ""].join("\n"),
);
} catch (error) {
@@ -1170,7 +1187,7 @@ export class MCPCommandController {
/**
* Reload MCP manager with new configs
*/
private async reloadMCP(): Promise<void> {
async #reloadMCP(): Promise<void> {
if (!this.ctx.mcpManager) {
return;
}
@@ -1189,14 +1206,14 @@ export class MCPCommandController {
errorLines.push(` ${serverName}: ${error}`);
}
errorLines.push("");
this.showMessage(errorLines.join("\n"));
this.#showMessage(errorLines.join("\n"));
}
}
/**
* Show a message in the chat
*/
private showMessage(text: string): void {
#showMessage(text: string): void {
this.ctx.chatContainer.addChild(new Spacer(1));
this.ctx.chatContainer.addChild(new DynamicBorder());
this.ctx.chatContainer.addChild(new Text(text, 1, 1));