diff --git a/packages/coding-agent/src/step/mcp-environment.ts b/packages/coding-agent/src/step/mcp-environment.ts new file mode 100644 index 00000000..c177387e --- /dev/null +++ b/packages/coding-agent/src/step/mcp-environment.ts @@ -0,0 +1,39 @@ +/** + * Environment resolution shared by the MCP runtime and the plugin doctor. + * + * It lives apart from `./mcp.ts` because `mcp.ts` imports `./plugins.ts`, so a + * plugin-side import of the runtime resolver would close a module cycle. Both + * sides must answer "does this server have a credential?" the same way: a + * doctor with its own rule reports a plugin as broken while the server it + * describes starts fine. + */ + +import { readStoredCredential } from "../core/auth-storage.ts"; +import { getStepAuthPath } from "./auth.ts"; + +/** + * The variables a Step login can supply on its own, so callers can tell a user + * whose only gap is `/login` apart from one who has to configure a variable + * StepCode knows nothing about. + */ +export const STEP_LOGIN_SUPPLIED_ENV: readonly string[] = ["STEPFUN_API_KEY"]; + +/** Resolve the environment passed to a plugin server, including Step login fallback. */ +export function resolveStepMcpEnvironment( + declared: Record | undefined, + input: { env?: NodeJS.ProcessEnv; authPath?: string } = {}, +): Record { + const resolved: Record = {}; + for (const [key, value] of Object.entries(input.env ?? process.env)) if (value !== undefined) resolved[key] = value; + Object.assign(resolved, declared ?? {}); + if (!resolved.STEPFUN_API_KEY?.trim()) { + const credential = readStoredCredential("step", input.authPath ?? getStepAuthPath()); + if (credential?.type === "oauth" && typeof credential.access === "string" && credential.access.trim()) { + resolved.STEPFUN_API_KEY = credential.access; + } + if (credential?.type === "api_key" && typeof credential.key === "string" && credential.key.trim()) { + resolved.STEPFUN_API_KEY = credential.key; + } + } + return resolved; +} diff --git a/packages/coding-agent/src/step/mcp.ts b/packages/coding-agent/src/step/mcp.ts index 2acbb7fc..dda3b840 100644 --- a/packages/coding-agent/src/step/mcp.ts +++ b/packages/coding-agent/src/step/mcp.ts @@ -6,11 +6,10 @@ import { StreamableHTTPClientTransport, StreamableHTTPError } from "@modelcontex import { CallToolResultSchema, type Tool as McpTool } from "@modelcontextprotocol/sdk/types.js"; import type { AgentToolResult } from "@step-harness/agent-core"; import { type TSchema, Type } from "typebox"; -import { readStoredCredential } from "../core/auth-storage.ts"; import type { ExtensionAPI, ExtensionFactory } from "../core/extensions/types.ts"; import { theme } from "../theme/theme.ts"; -import { getStepAuthPath } from "./auth.ts"; import { readGlobalStepConfig } from "./config-toml.ts"; +import { resolveStepMcpEnvironment } from "./mcp-environment.ts"; import { createStoredMcpOAuthProvider, hasStoredMcpOAuthCredential } from "./mcp-oauth.ts"; import { defaultStepPluginsDir, @@ -23,6 +22,8 @@ import { } from "./plugins.ts"; import { STEPCODE_VERSION } from "./version.ts"; +export { resolveStepMcpEnvironment } from "./mcp-environment.ts"; + const MCP_STARTUP_TIMEOUT_SEC = 30; const MCP_CALL_TIMEOUT_SEC = 300; const CLIENT_INFO = { name: "step-harness", version: STEPCODE_VERSION.value } as const; @@ -409,26 +410,6 @@ function isMissingExecutable(error: unknown): boolean { return error instanceof Error && /\bENOENT\b/u.test(error.message); } -/** Resolve the environment passed to a plugin server, including Step login fallback. */ -export function resolveStepMcpEnvironment( - declared: Record | undefined, - input: { env?: NodeJS.ProcessEnv; authPath?: string } = {}, -): Record { - const resolved: Record = {}; - for (const [key, value] of Object.entries(input.env ?? process.env)) if (value !== undefined) resolved[key] = value; - Object.assign(resolved, declared ?? {}); - if (!resolved.STEPFUN_API_KEY?.trim()) { - const credential = readStoredCredential("step", input.authPath ?? getStepAuthPath()); - if (credential?.type === "oauth" && typeof credential.access === "string" && credential.access.trim()) { - resolved.STEPFUN_API_KEY = credential.access; - } - if (credential?.type === "api_key" && typeof credential.key === "string" && credential.key.trim()) { - resolved.STEPFUN_API_KEY = credential.key; - } - } - return resolved; -} - interface McpCallResult { content?: Array<{ type?: string; text?: string; data?: string; mimeType?: string }>; structuredContent?: unknown; diff --git a/packages/coding-agent/src/step/plugins.ts b/packages/coding-agent/src/step/plugins.ts index 0e4cff09..4ed67f0b 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -16,6 +16,7 @@ import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; import type { ExtensionAPI, ExtensionCommandContext } from "../core/extensions/types.ts"; import { resolveStepConfigDir } from "./environment.ts"; +import { resolveStepMcpEnvironment, STEP_LOGIN_SUPPLIED_ENV } from "./mcp-environment.ts"; import { resolveStepStorageRoot } from "./storage-root.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "./telemetry.ts"; @@ -605,7 +606,10 @@ export async function uninstallPlugin(pluginsDir: string, name: string): Promise } /** Read MCP declarations without starting a process. */ -export async function diagnoseStepPlugin(pluginDir: string): Promise { +export async function diagnoseStepPlugin( + pluginDir: string, + options: { env?: NodeJS.ProcessEnv; authPath?: string } = {}, +): Promise { const read = await readStepPluginManifest(pluginDir); if (read.errors.length > 0) return { mcpServers: [], warnings: [...read.errors] }; if (!read.manifest) return { mcpServers: [], warnings: [`No ${STEP_PLUGIN_MANIFEST_FILE} found in ${pluginDir}.`] }; @@ -640,15 +644,58 @@ export async function diagnoseStepPlugin(pluginDir: string): Promise !process.env[name]?.trim()); - if (missingEnvironment.length > 0) { - warnings.push( - `Plugin provisioning has no shell value for ${missingEnvironment.join(", ")}; a Step login credential can supply it at runtime.`, + // Judged against the environment the matching servers are actually spawned + // with: `connectStepMcpServer` layers the process environment, the server's + // own declared `env`, and the Step login credential. Checking `process.env` + // alone reported every logged-in user as missing a variable they were never + // expected to export by hand. Only an inline `mcpServers` record can start a + // server — discovery skips a string declaration path — so that is the only + // shape whose declared `env` can satisfy a requirement. + const requiredEnvironment = read.manifest.provision?.requiresEnv ?? []; + if (requiredEnvironment.length > 0) { + const candidates = provisionedServerEnvironments(read.manifest).map((declared) => + resolveStepMcpEnvironment(declared, options), + ); + const missingEnvironment = requiredEnvironment.filter((name) => + candidates.every((candidate) => !candidate[name]?.trim()), ); + // A Step login only ever supplies its own credential, so pointing at + // `/login` for an unrelated variable would send the user nowhere. + const missingLogin = missingEnvironment.filter((name) => STEP_LOGIN_SUPPLIED_ENV.includes(name)); + const missingOther = missingEnvironment.filter((name) => !STEP_LOGIN_SUPPLIED_ENV.includes(name)); + if (missingLogin.length > 0) { + warnings.push( + `Plugin provisioning has no value for ${missingLogin.join(", ")}; run /login or export it before using this plugin.`, + ); + } + if (missingOther.length > 0) { + warnings.push( + `Plugin provisioning has no value for ${missingOther.join(", ")}; export it or declare it in the plugin's mcpServers env before using this plugin.`, + ); + } } return { mcpServers, warnings }; } +/** + * The declared environments of the servers a manifest's provisioning installs, + * matched on the provisioned command. Returns a single `undefined` when no + * server matches, so the caller still judges the requirement against the + * process environment and the login fallback. + */ +function provisionedServerEnvironments(manifest: StepPluginManifest): Array | undefined> { + const provisionCommand = manifest.provision?.command; + if (!provisionCommand || !isRecord(manifest.mcpServers)) return [undefined]; + const declared = Object.values(manifest.mcpServers).flatMap((declaration) => { + if (!isRecord(declaration) || declaration.command !== provisionCommand) return []; + if (!isRecord(declaration.env)) return [undefined]; + const env: Record = {}; + for (const [key, value] of Object.entries(declaration.env)) if (typeof value === "string") env[key] = value; + return [env]; + }); + return declared.length > 0 ? declared : [undefined]; +} + export async function listInstalledStepPlugins( input: { userDir?: string; projectDir?: string } = {}, ): Promise<{ plugins: InstalledStepPlugin[]; warnings: string[] }> { diff --git a/packages/coding-agent/test/step-plugins.test.ts b/packages/coding-agent/test/step-plugins.test.ts index 185c9cbf..8f267680 100644 --- a/packages/coding-agent/test/step-plugins.test.ts +++ b/packages/coding-agent/test/step-plugins.test.ts @@ -130,6 +130,79 @@ describe("Step plugin marketplace facade", () => { expect(afterUninstall.plugins.map((plugin) => plugin.id)).not.toContain("steppage"); }); + test("treats a Step login credential as satisfying a provisioned environment requirement", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-requires-env-")); + roots.push(root); + const pluginDir = join(root, "steppage"); + await mkdir(pluginDir, { recursive: true }); + await writeFile( + join(pluginDir, "step.plugin.json"), + JSON.stringify({ + id: "steppage", + provision: { + command: "steppage-mcp", + installer: "https://example.invalid/i.sh", + requiresEnv: ["STEPFUN_API_KEY"], + }, + }), + ); + const authPath = join(root, "auth.json"); + await writeFile( + authPath, + JSON.stringify({ step: { type: "oauth", access: "login-key", refresh: "r", expires: 0 } }), + ); + + // A logged-in user exports nothing by hand: the credential on disk is what + // the server is spawned with, so the doctor must not report it as missing. + const loggedIn = await diagnoseStepPlugin(pluginDir, { env: {}, authPath }); + expect(loggedIn.warnings.join(" ")).not.toContain("STEPFUN_API_KEY"); + + // With neither a shell value nor a credential the warning is real advice. + const loggedOut = await diagnoseStepPlugin(pluginDir, { env: {}, authPath: join(root, "absent.json") }); + expect(loggedOut.warnings.join(" ")).toContain("STEPFUN_API_KEY"); + expect(loggedOut.warnings.join(" ")).toContain("/login"); + }); + + test("accepts a requirement satisfied by the provisioned server's own declared env", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-declared-env-")); + roots.push(root); + const pluginDir = join(root, "declared"); + await mkdir(pluginDir, { recursive: true }); + await writeFile( + join(pluginDir, "step.plugin.json"), + JSON.stringify({ + id: "declared", + mcpServers: { declared: { command: "steppage-mcp", env: { STEPFUN_API_KEY: "declared-key" } } }, + provision: { command: "steppage-mcp", requiresEnv: ["STEPFUN_API_KEY"] }, + }), + ); + + // The runtime layers the server's declared env over the process env, so a + // manifest that carries its own key needs neither a shell value nor a login. + const diagnostics = await diagnoseStepPlugin(pluginDir, { env: {}, authPath: join(root, "absent.json") }); + expect(diagnostics.warnings.join(" ")).not.toContain("STEPFUN_API_KEY"); + }); + + test("points a non-login variable at configuration rather than /login", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-other-env-")); + roots.push(root); + const pluginDir = join(root, "other"); + await mkdir(pluginDir, { recursive: true }); + await writeFile( + join(pluginDir, "step.plugin.json"), + JSON.stringify({ + id: "other", + mcpServers: { other: { command: "other-mcp" } }, + provision: { command: "other-mcp", requiresEnv: ["GITHUB_TOKEN"] }, + }), + ); + + // A Step login cannot supply someone else's token, so it must not be the advice. + const diagnostics = await diagnoseStepPlugin(pluginDir, { env: {}, authPath: join(root, "absent.json") }); + expect(diagnostics.warnings.join(" ")).toContain("GITHUB_TOKEN"); + expect(diagnostics.warnings.join(" ")).not.toContain("/login"); + }); + test("does not overwrite a Claude-style plugin manifest", async () => { const root = await mkdtemp(join(tmpdir(), "step-plugins-claude-")); roots.push(root);