From 4625966cbe2e415bcb89d52005be8c702c970baa Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Thu, 24 Sep 2026 16:53:19 +0800 Subject: [PATCH 1/2] fix: judge plugin requiresEnv against the runtime environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `diagnoseStepPlugin` checked `process.env` for the variables a plugin's provision block declares, while the server it describes is spawned with `resolveStepMcpEnvironment`, which falls back to the Step login credential for STEPFUN_API_KEY. Every logged-in user therefore saw the built-in StepPage plugin warn about a missing STEPFUN_API_KEY that the runtime supplies on its own, and the warning told them a credential "can supply it at runtime" rather than what to do. Move `resolveStepMcpEnvironment` into `step/mcp-environment.ts` so the doctor and the runtime share one resolution — `mcp.ts` imports `plugins.ts`, so plugins cannot import the resolver from `mcp.ts` without a cycle — inject env/authPath into `diagnoseStepPlugin`, and reword the remaining warning to point at `/login`. --- .../coding-agent/src/step/mcp-environment.ts | 32 ++++++++++++++++++ packages/coding-agent/src/step/mcp.ts | 25 ++------------ packages/coding-agent/src/step/plugins.ts | 24 ++++++++++---- .../coding-agent/test/step-plugins.test.ts | 33 +++++++++++++++++++ 4 files changed, 86 insertions(+), 28 deletions(-) create mode 100644 packages/coding-agent/src/step/mcp-environment.ts 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..3f1bb4ed --- /dev/null +++ b/packages/coding-agent/src/step/mcp-environment.ts @@ -0,0 +1,32 @@ +/** + * 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"; + +/** 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..c05f252d 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 } 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,11 +644,19 @@ 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 server will actually be spawned with, + // not the bare shell: a Step login supplies STEPFUN_API_KEY at spawn time, so + // checking `process.env` alone reported every logged-in user as missing a + // variable they were never expected to export by hand. + const requiredEnvironment = read.manifest.provision?.requiresEnv ?? []; + if (requiredEnvironment.length > 0) { + const runtimeEnvironment = resolveStepMcpEnvironment(undefined, options); + const missingEnvironment = requiredEnvironment.filter((name) => !runtimeEnvironment[name]?.trim()); + if (missingEnvironment.length > 0) { + warnings.push( + `Plugin provisioning has no value for ${missingEnvironment.join(", ")}; run /login or export it before using this plugin.`, + ); + } } return { mcpServers, warnings }; } diff --git a/packages/coding-agent/test/step-plugins.test.ts b/packages/coding-agent/test/step-plugins.test.ts index 185c9cbf..c831665e 100644 --- a/packages/coding-agent/test/step-plugins.test.ts +++ b/packages/coding-agent/test/step-plugins.test.ts @@ -130,6 +130,39 @@ 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("does not overwrite a Claude-style plugin manifest", async () => { const root = await mkdtemp(join(tmpdir(), "step-plugins-claude-")); roots.push(root); From e4434e2a6d78aedbd758bd92b33f5395cc23889b Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:07:41 +0800 Subject: [PATCH 2/2] fix: match plugin requiresEnv to each provisioned server's environment Review follow-up on the previous commit, which compared `requiresEnv` against the login fallback but still passed no declared environment. `connectStepMcpServer` layers three sources: the process environment, the server's own declared `env`, and the Step login credential. The doctor consulted only the first and third, so a plugin that carried its key in `mcpServers..env` was still reported as missing it. Match the servers whose command is the provisioned one and judge the requirement against each of their resolved environments. Only an inline `mcpServers` record can start a server, since discovery skips a string declaration path, so that is the only shape consulted. A Step login also only ever supplies its own credential, so `/login` is no longer offered for an unrelated variable; those are pointed at exporting it or declaring it in the manifest. `STEP_LOGIN_SUPPLIED_ENV` keeps that list next to the resolver that honours it rather than hardcoded in the message. --- .../coding-agent/src/step/mcp-environment.ts | 7 +++ packages/coding-agent/src/step/plugins.ts | 53 +++++++++++++++---- .../coding-agent/test/step-plugins.test.ts | 40 ++++++++++++++ 3 files changed, 91 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/step/mcp-environment.ts b/packages/coding-agent/src/step/mcp-environment.ts index 3f1bb4ed..c177387e 100644 --- a/packages/coding-agent/src/step/mcp-environment.ts +++ b/packages/coding-agent/src/step/mcp-environment.ts @@ -11,6 +11,13 @@ 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, diff --git a/packages/coding-agent/src/step/plugins.ts b/packages/coding-agent/src/step/plugins.ts index c05f252d..4ed67f0b 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -16,7 +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 } from "./mcp-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"; @@ -644,23 +644,58 @@ export async function diagnoseStepPlugin( } if (read.manifest.entry) warnings.push("Executable plugin entries are recorded but not loaded by the Step marketplace facade."); - // Judged against the environment the server will actually be spawned with, - // not the bare shell: a Step login supplies STEPFUN_API_KEY at spawn time, so - // checking `process.env` alone reported every logged-in user as missing a - // variable they were never expected to export by hand. + // 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 runtimeEnvironment = resolveStepMcpEnvironment(undefined, options); - const missingEnvironment = requiredEnvironment.filter((name) => !runtimeEnvironment[name]?.trim()); - if (missingEnvironment.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 ${missingEnvironment.join(", ")}; run /login or export it before using this plugin.`, + `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 c831665e..8f267680 100644 --- a/packages/coding-agent/test/step-plugins.test.ts +++ b/packages/coding-agent/test/step-plugins.test.ts @@ -163,6 +163,46 @@ describe("Step plugin marketplace facade", () => { 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);