From bda152e074d540487e65c1f972e3d03875c51fcf Mon Sep 17 00:00:00 2001 From: xuyunfang Date: Tue, 29 Sep 2026 21:08:19 +0800 Subject: [PATCH 1/3] fix(plugins): load plugin skills and commands, and fix MCP discovery Declarative plugins could contribute MCP servers, but nothing carried their skills or commands into a session: the resource loader reads agent/project resource directories that plugins never write to. A marketplace plugin installed successfully and then had no effect. Bridge the two with the existing `resources_discover` channel, which already supports skill and prompt paths and re-merges them on reload. Project plugin roots are gated on project trust, matching MCP discovery, so an untrusted checkout cannot inject skill instructions. Alongside that, MCP discovery dropped several plugin shapes silently: - A remote server declaring `url` with no `command` was skipped, even though the global-config path and the normalizer both accept a url. - `mcpServers` naming a sibling `.mcp.json` (the Claude layout) was read as an unsupported string, so every such plugin lost its servers. - The `headers` spelling was not accepted, and its `${VAR:-fallback}` values were never expanded. Expansion now applies to plugin manifests only: a `config.toml` `http_headers` map stays literal. - `step mcp login` only read `config.toml`, so it could not resolve the `__` name its own failure message printed. It now accepts the bare and published spellings, and stores credentials under the resolved name so login and the connection agree on the key. The marketplace UI also grew a searchable plugin list, in-place clone progress (with a cancel path), and lands on the new marketplace's plugins after a successful add. --- apps/cli/src/main.ts | 48 ++- apps/cli/src/ui/interactive-mode.ts | 2 + apps/cli/src/ui/startup-ui.ts | 2 + .../src/ui/view/dialogs/extension-input.ts | 27 +- .../src/ui/view/dialogs/extension-selector.ts | 77 +++- apps/cli/test/step-overlay-components.test.ts | 74 ++++ .../coding-agent/src/cli/project-trust.ts | 5 +- .../src/components/bordered-loader.ts | 5 + .../coding-agent/src/core/extensions/types.ts | 16 + packages/coding-agent/src/features/step.ts | 4 + packages/coding-agent/src/index.ts | 1 + .../src/modes/interactive-contract.ts | 1 + .../coding-agent/src/modes/rpc/rpc-mode.ts | 7 +- .../coding-agent/src/modes/rpc/rpc-types.ts | 1 + .../src/step/mcp-credential-key.test.ts | 68 ++++ .../coding-agent/src/step/mcp-startup.test.ts | 270 +++++++++++++- packages/coding-agent/src/step/mcp.ts | 193 +++++++++- packages/coding-agent/src/step/plugins.ts | 336 ++++++++++++++++-- packages/coding-agent/src/step/stdio-host.ts | 7 +- .../coding-agent/test/step-plugins.test.ts | 262 +++++++++++++- packages/tui/src/components/select-list.ts | 13 + 21 files changed, 1357 insertions(+), 62 deletions(-) create mode 100644 packages/coding-agent/src/step/mcp-credential-key.test.ts diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 19223261..b2d5899f 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -10,6 +10,7 @@ import "#bootstrap/environment"; import { join } from "node:path"; import { + ambiguousPluginServerNames, applyStepCodeConfigDefaults, buildStepSystemPromptAppendix, configureHttpDispatcher, @@ -50,6 +51,7 @@ import { resolveStepAgentDir, resolveStepConfigDir, resolveStepConfigRoot, + resolveStepMcpServer, resolveStepStorageRoot, restoreStdout, runFeedbackCommand, @@ -427,10 +429,44 @@ try { } } else if (subcommand === "login" || subcommand === "logout") { const name = args[1]; - const server = name ? servers[name] : undefined; - if (!name || !server) { + // A plugin's servers are named `__`, which is the + // name a start-failure message prints and the name `/mcp` shows. Reading + // only config.toml here meant those servers could not be logged into at + // all, even by copying the name out of the error. + // + // Keep the *resolved* name, not the user's input: credentials are stored + // under `|` and the runtime reads them under the published + // name, so logging in with a bare `context7` would write a key that the + // `context7__context7` connection never looks up — reporting success and + // then failing the same way on the next start. + const resolved = name && !servers[name] ? await resolveStepMcpServer(name) : undefined; + const serverName = resolved?.name ?? name; + const server = name ? (servers[name] ?? resolved?.declaration) : undefined; + // A bare name shared by several plugins resolves to nothing. Say so + // instead of claiming no such server exists, which sends the user + // hunting for a typo that is not there. + const ambiguous = name && !servers[name] && !resolved ? await ambiguousPluginServerNames(name) : []; + if (!name) { process.stderr.write("Usage: step mcp login|logout \n"); process.exitCode = 1; + } else if (!server) { + if (ambiguous.length > 0) { + process.stderr.write( + `Several plugins declare a server named '${name}': ${ambiguous.join(", ")}. ` + + "Use the qualified name.\n", + ); + } else { + // Name the sources so a typo is distinguishable from a server + // that exists only in a plugin, and point at /mcp for the latter. + process.stderr.write( + `No MCP server named '${name}'. It is not in config.toml` + + (servers && Object.keys(servers).length > 0 + ? ` (which has: ${Object.keys(servers).join(", ")})` + : "") + + ", and no plugin declares it. Run /mcp inside Step to see the names in use.\n", + ); + } + process.exitCode = 1; } else if (!server.url) { process.stderr.write( `"${name}" doesn't support OAuth login — it's only available for HTTP and SSE servers.\n`, @@ -438,7 +474,7 @@ try { process.exitCode = 1; } else if (subcommand === "login") { try { - await loginMcpServer(name, server.url, server.oauth, process.env); + await loginMcpServer(serverName!, server.url, server.oauth, process.env); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (message.includes("403") && name.toLowerCase().includes("figma")) { @@ -450,9 +486,9 @@ try { } } else { process.stdout.write( - logoutMcpServer(name, server.url, process.env) - ? `Logged out of MCP server '${name}'.\n` - : `No stored credentials for MCP server '${name}'.\n`, + logoutMcpServer(serverName!, server.url, process.env) + ? `Logged out of MCP server '${serverName}'.\n` + : `No stored credentials for MCP server '${serverName}'.\n`, ); } } else { diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index dd044edb..8a187c6f 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -2922,6 +2922,7 @@ export class InteractiveMode { timeout: opts?.timeout, onToggleToolsExpanded: () => this.toggleToolOutputExpansion(), presentation: this.presentation, + searchable: opts?.searchable, }, ); this.extensionSelector = selector; @@ -3003,6 +3004,7 @@ export class InteractiveMode { tui: this.ui, timeout: opts?.timeout, presentation: this.presentation, + examples: opts?.examples, }); this.extensionInput = input; unmount = this.mountExtensionDialog(input, opts); diff --git a/apps/cli/src/ui/startup-ui.ts b/apps/cli/src/ui/startup-ui.ts index 5b252de7..65dc4ec4 100644 --- a/apps/cli/src/ui/startup-ui.ts +++ b/apps/cli/src/ui/startup-ui.ts @@ -202,6 +202,7 @@ export async function showStartupInput( title: string, placeholder?: string, paths?: StartupTuiPathOptions, + examples?: readonly string[], ): Promise { const ui = await createStartupTui(settingsManager, paths); return new Promise((resolve) => { @@ -225,6 +226,7 @@ export async function showStartupInput( { tui: ui, presentation: STARTUP_PRESENTATION, + examples, }, ); ui.addChild(input); diff --git a/apps/cli/src/ui/view/dialogs/extension-input.ts b/apps/cli/src/ui/view/dialogs/extension-input.ts index 4952bf20..7879674c 100644 --- a/apps/cli/src/ui/view/dialogs/extension-input.ts +++ b/apps/cli/src/ui/view/dialogs/extension-input.ts @@ -3,7 +3,16 @@ */ import { DynamicBorder, keyHint, theme } from "@step-harness/coding-agent"; -import { Container, type Focusable, getKeybindings, Input, Spacer, Text, type TUI } from "@step-harness/pi-tui"; +import { + Container, + type Focusable, + getKeybindings, + Input, + Spacer, + Text, + type TUI, + truncateToWidth, +} from "@step-harness/pi-tui"; import { CountdownTimer } from "./countdown-timer.ts"; import { renderStepDialogFrame, splitStepDialogTitle } from "./step-dialog.ts"; @@ -11,6 +20,8 @@ export interface ExtensionInputOptions { tui?: TUI; timeout?: number; presentation?: "native" | "step"; + /** Reference lines listed under the title while the input is still empty. */ + examples?: readonly string[]; } export class ExtensionInputComponent extends Container implements Focusable { @@ -22,6 +33,7 @@ export class ExtensionInputComponent extends Container implements Focusable { private currentTitle: string; private countdown: CountdownTimer | undefined; private readonly placeholder: string | undefined; + private readonly examples: readonly string[]; private readonly presentation: "native" | "step"; // Focusable implementation - propagate to input for IME cursor positioning @@ -48,6 +60,7 @@ export class ExtensionInputComponent extends Container implements Focusable { this.baseTitle = title; this.currentTitle = title; this.placeholder = placeholder; + this.examples = opts?.examples ?? []; this.presentation = opts?.presentation ?? "native"; this.addChild(new DynamicBorder()); @@ -90,6 +103,17 @@ export class ExtensionInputComponent extends Container implements Focusable { } } + /** The examples block, listed under the title above the editable row. */ + private renderExamples(contentWidth: number): string[] { + if (this.examples.length === 0) return []; + const rows = [theme.fg("muted", "Examples:")]; + for (const example of this.examples) { + rows.push(theme.fg("dim", truncateToWidth(` · ${example}`, contentWidth, "…", false))); + } + rows.push(""); + return rows; + } + dispose(): void { this.countdown?.dispose(); } @@ -105,6 +129,7 @@ export class ExtensionInputComponent extends Container implements Focusable { if (heading.length > 0) rows.push(theme.fg("accent", theme.bold(`● ${heading}`))); for (const line of body) rows.push(theme.fg("muted", line)); if (rows.length > 0) rows.push(""); + for (const row of this.renderExamples(contentWidth)) rows.push(row); let inputLine = this.input.render(contentWidth)[0] ?? "> "; if (this.input.getValue().length === 0 && this.placeholder) { diff --git a/apps/cli/src/ui/view/dialogs/extension-selector.ts b/apps/cli/src/ui/view/dialogs/extension-selector.ts index 0f93d9cd..58a30532 100644 --- a/apps/cli/src/ui/view/dialogs/extension-selector.ts +++ b/apps/cli/src/ui/view/dialogs/extension-selector.ts @@ -6,7 +6,9 @@ import { DynamicBorder, keyHint, rawKeyHint, theme } from "@step-harness/coding-agent"; import { Container, + fuzzyFilter, getKeybindings, + Input, type SelectItem, SelectList, Spacer, @@ -22,10 +24,18 @@ export interface ExtensionSelectorOptions { timeout?: number; onToggleToolsExpanded?: () => void; presentation?: "native" | "step"; + /** + * Show a search row above the list and filter it as the user types. + * + * Use it for lists whose length is set by the environment rather than by the + * dialog — a marketplace can carry hundreds of plugins, and scrolling is not + * a way to find one. Movement keys still drive the list; everything else goes + * to the search row, so a query never has to be prefixed with a mode key. + */ + searchable?: boolean; } export class ExtensionSelectorComponent extends Container { - private readonly options: string[]; private selectedIndex = 0; private readonly selectList: SelectList; private onSelectCallback: (option: string) => void; @@ -36,6 +46,8 @@ export class ExtensionSelectorComponent extends Container { private countdown: CountdownTimer | undefined; private onToggleToolsExpanded: (() => void) | undefined; private readonly presentation: "native" | "step"; + private readonly searchInput: Input | undefined; + private readonly allItems: SelectItem[]; constructor( title: string, @@ -46,7 +58,6 @@ export class ExtensionSelectorComponent extends Container { ) { super(); - this.options = options; this.onSelectCallback = onSelect; this.onCancelCallback = onCancel; this.onToggleToolsExpanded = opts?.onToggleToolsExpanded; @@ -58,6 +69,7 @@ export class ExtensionSelectorComponent extends Container { value: option, label: option, })); + this.allItems = items; this.selectList = new SelectList(items, Math.max(1, Math.min(8, items.length)), { selectedPrefix: (text) => theme.fg("accent", text), selectedText: (text) => theme.fg("accent", text), @@ -68,7 +80,7 @@ export class ExtensionSelectorComponent extends Container { this.selectList.onSelect = (item) => this.onSelectCallback(item.value); this.selectList.onCancel = () => this.onCancelCallback(); this.selectList.onSelectionChange = (item) => { - const index = items.indexOf(item); + const index = this.visibleItems().indexOf(item); if (index >= 0) this.selectedIndex = index; }; @@ -79,6 +91,15 @@ export class ExtensionSelectorComponent extends Container { this.addChild(this.titleText); this.addChild(new Spacer(1)); + if (opts?.searchable) { + this.searchInput = new Input(); + // Enter in the search row means "take the highlighted plugin", the same + // as Enter anywhere else here; the list owns the actual selection. + this.searchInput.onSubmit = () => this.selectList.handleInput("\r"); + this.addChild(this.searchInput); + this.addChild(new Spacer(1)); + } + if (opts?.timeout && opts.timeout > 0 && opts.tui) { this.countdown = new CountdownTimer( opts.timeout, @@ -108,6 +129,12 @@ export class ExtensionSelectorComponent extends Container { this.addChild(new DynamicBorder()); } + /** The items the list is currently showing, which the search row re-filters. */ + private visibleItems(): SelectItem[] { + const query = this.searchInput?.getValue() ?? ""; + return query.trim() ? fuzzyFilter(this.allItems, query, (item) => item.value) : this.allItems; + } + handleInput(keyData: string): void { const kb = getKeybindings(); if (kb.matches(keyData, "app.tools.expand")) { @@ -115,11 +142,27 @@ export class ExtensionSelectorComponent extends Container { return; } + const searchInput = this.searchInput; + const drivesList = + kb.matches(keyData, "tui.select.up") || + kb.matches(keyData, "tui.select.down") || + kb.matches(keyData, "tui.select.pageUp") || + kb.matches(keyData, "tui.select.pageDown") || + kb.matches(keyData, "tui.select.confirm") || + kb.matches(keyData, "tui.select.cancel"); + + if (searchInput && !drivesList) { + searchInput.handleInput(keyData); + this.applySearch(); + return; + } + // Pi's SelectList owns regular movement/confirm/cancel. Step only keeps - // the product's non-circular boundary behavior for transient decisions. - if (this.presentation === "step") { + // the product's non-circular boundary behavior for transient decisions, and + // a search row owns the movement keys instead once one is present. + if (this.presentation === "step" && !searchInput) { const atFirst = this.selectedIndex === 0; - const atLast = this.selectedIndex === Math.max(0, this.options.length - 1); + const atLast = this.selectedIndex === Math.max(0, this.allItems.length - 1); if ((kb.matches(keyData, "tui.select.up") && atFirst) || (kb.matches(keyData, "tui.select.down") && atLast)) { return; } @@ -127,6 +170,20 @@ export class ExtensionSelectorComponent extends Container { this.selectList.handleInput(keyData); } + /** + * Push the current query onto the list. + * + * The selector drives the list through setItems rather than setFilter, so an + * empty match set has to be preserved deliberately: setItems with the full + * list would resurrect every option the query just excluded. + */ + private applySearch(): void { + const query = this.searchInput?.getValue() ?? ""; + this.selectList.setItems(this.visibleItems()); + if (query.trim() && this.visibleItems().length === 0) this.selectList.setEmpty(); + this.selectedIndex = 0; + } + override render(width: number): string[] { if (this.presentation !== "step") return super.render(width); @@ -138,6 +195,11 @@ export class ExtensionSelectorComponent extends Container { if (heading.length > 0) rows.push(theme.fg("accent", theme.bold(`● ${heading}`))); for (const line of body) rows.push(theme.fg("muted", line)); if (rows.length > 0) rows.push(""); + if (this.searchInput) { + rows.push(theme.fg("muted", "Search:")); + rows.push(...this.searchInput.render(contentWidth)); + rows.push(""); + } rows.push(...this.selectList.render(contentWidth)); rows.push(""); rows.push( @@ -148,7 +210,8 @@ export class ExtensionSelectorComponent extends Container { " " + keyHint("tui.select.confirm", "select") + " " + - keyHint("tui.select.cancel", "cancel"), + keyHint("tui.select.cancel", "cancel") + + (this.searchInput ? " type to search" : ""), ), contentWidth, ), diff --git a/apps/cli/test/step-overlay-components.test.ts b/apps/cli/test/step-overlay-components.test.ts index 2b0e9b33..51298a59 100644 --- a/apps/cli/test/step-overlay-components.test.ts +++ b/apps/cli/test/step-overlay-components.test.ts @@ -120,6 +120,80 @@ describe("Step transient presentation", () => { for (const row of rows) expect(visibleWidth(row)).toBe(48); }); + it("filters a searchable selector by typing and keeps the list in charge of Enter", () => { + initTheme("step-blue"); + const keybindings = new KeybindingsManager(); + setKeybindings(keybindings); + const selected: string[] = []; + const selector = new ExtensionSelectorComponent( + "All Plugins (3 available)", + ["skill-creator · official", "code-review · official", "plan-to-lark · plan-to-lark"], + (value) => selected.push(value), + () => undefined, + { presentation: "step", searchable: true }, + ); + + const plain = (): string => selector.render(80).map(stripTerminalSequences).join("\n"); + expect(plain()).toContain("Search:"); + + // Typing goes to the query, not to the list. + for (const char of "rev") selector.handleInput(char); + const filtered = plain(); + expect(filtered).toContain("code-review"); + expect(filtered).not.toContain("plan-to-lark"); + + // Enter still confirms the highlighted match. + selector.handleInput("\r"); + expect(selected).toEqual(["code-review · official"]); + + // A query that matches nothing shows the empty state instead of the + // options it excluded. + const empty = new ExtensionSelectorComponent( + "All Plugins", + ["alpha", "beta"], + () => undefined, + () => undefined, + { presentation: "step", searchable: true }, + ); + for (const char of "zzz") empty.handleInput(char); + const emptyRows = empty.render(80).map(stripTerminalSequences).join("\n"); + expect(emptyRows).toContain("No matching"); + expect(emptyRows).not.toContain("alpha"); + }); + + it("lists examples above the input row", () => { + initTheme("step-blue"); + const keybindings = new KeybindingsManager(); + setKeybindings(keybindings); + const input = new ExtensionInputComponent( + "Add Marketplace", + undefined, + () => undefined, + () => undefined, + { + presentation: "step", + examples: ["owner/repo (GitHub)", "./path/to/marketplace"], + }, + ); + + const before = input.render(60); + const plainBefore = before.map(stripTerminalSequences); + const examplesRow = plainBefore.findIndex((row) => row.includes("Examples:")); + const firstExampleRow = plainBefore.findIndex((row) => row.includes("owner/repo (GitHub)")); + expect(examplesRow).toBeGreaterThan(0); + expect(firstExampleRow).toBe(examplesRow + 1); + // The examples sit above the editable row, not below it. + const inputRow = plainBefore.findIndex((row) => row.includes("> ")); + expect(inputRow).toBeGreaterThan(firstExampleRow); + for (const row of before) expect(visibleWidth(row)).toBe(60); + + // The reference lines stay put while the value is edited. + input.handleInput("g"); + const after = input.render(60).map(stripTerminalSequences); + expect(after.some((row) => row.includes("Examples:"))).toBe(true); + expect(after.some((row) => row.includes("owner/repo (GitHub)"))).toBe(true); + }); + it("frames the extension editor while retaining native editing", () => { initTheme("step-blue"); const keybindings = new KeybindingsManager(); diff --git a/packages/coding-agent/src/cli/project-trust.ts b/packages/coding-agent/src/cli/project-trust.ts index c94a3555..e0eda3da 100644 --- a/packages/coding-agent/src/cli/project-trust.ts +++ b/packages/coding-agent/src/cli/project-trust.ts @@ -23,6 +23,7 @@ export interface ProjectTrustUiPrimitives { title: string, placeholder?: string, paths?: StartupTuiPathOptions, + examples?: readonly string[], ) => Promise; } @@ -80,7 +81,7 @@ export function createProjectTrustContext(options: { )) ?? false ); }, - input: async (title, placeholder) => { + input: async (title, placeholder, opts) => { if (!options.hasUI) { return undefined; } @@ -90,7 +91,7 @@ export function createProjectTrustContext(options: { if (!showStartupInput) { return undefined; } - return showStartupInput(options.settingsManager, title, placeholder, options.paths); + return showStartupInput(options.settingsManager, title, placeholder, options.paths, opts?.examples); }, notify: (message, type = "info") => { if (options.mode !== "interactive") { diff --git a/packages/coding-agent/src/components/bordered-loader.ts b/packages/coding-agent/src/components/bordered-loader.ts index baed2cb7..b609108b 100644 --- a/packages/coding-agent/src/components/bordered-loader.ts +++ b/packages/coding-agent/src/components/bordered-loader.ts @@ -45,6 +45,11 @@ export class BorderedLoader extends Container { return this.signalController?.signal ?? new AbortController().signal; } + /** Replace the spinner text in place, for operations that report stages. */ + setMessage(message: string): void { + this.loader.setMessage(message); + } + set onAbort(fn: (() => void) | undefined) { if (this.cancellable) { (this.loader as CancellableLoader).onAbort = fn; diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 3191f448..ad76bd3f 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -110,6 +110,22 @@ export interface ExtensionUIDialogOptions { * drift is visible because no output follows the dialog to fill the gap back in. */ overlay?: boolean; + /** + * Reference lines for an input dialog, rendered under the title and above the + * editable row. Use them to show the accepted spellings of an answer that has + * more than one (a git URL, an owner/repo pair, a local path), so the accepted + * forms are visible while typing instead of only in the placeholder that + * disappears on the first keystroke. Ignored by select, confirm and editor + * dialogs, and by hosts that render dialogs as plain transport. + */ + examples?: readonly string[]; + /** + * Add a search row to a select dialog and filter its options as the user + * types. Use it for lists whose length is set by the environment rather than + * by the dialog — a marketplace can carry hundreds of plugins, and scrolling + * is not a way to find one. Ignored by non-select dialogs. + */ + searchable?: boolean; } /** Placement for extension widgets. */ diff --git a/packages/coding-agent/src/features/step.ts b/packages/coding-agent/src/features/step.ts index 7a3a4357..bc8c1347 100644 --- a/packages/coding-agent/src/features/step.ts +++ b/packages/coding-agent/src/features/step.ts @@ -19,6 +19,7 @@ import { StepPermissionController, type StepPermissionControllerOptions, } from "../step/permissions.ts"; +import { createStepPluginResourcesExtension } from "../step/plugins.ts"; import type { StepSettingsManager } from "../step/settings-manager.ts"; import { recordStepSlashCommand, registerStepPiCommandAdapters } from "../step/slash-commands.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "../step/telemetry.ts"; @@ -110,6 +111,9 @@ export function createStepExtension(options: StepExtensionOptions = {}): Extensi // bridge is loaded as part of the Step product extension so ordinary Pi // sessions remain unchanged. createStepMcpExtension()(pi); + // The same plugins also contribute skills and commands, which the resource + // loader reads from agent/project directories rather than the plugin root. + createStepPluginResourcesExtension()(pi); registerStepPiCommandAdapters(pi, options.telemetry, options.stepSettings, options.feedbackIdentity); const builtInSlashCommands = new Set(BUILTIN_SLASH_COMMANDS.map((command) => command.name)); // Extension commands are wrapped at registration time below. Inputs that diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 5b9a78f4..8e933536 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -610,6 +610,7 @@ export { type StepLoginMethod, type StepLoginStatus, } from "./step/login-status.ts"; +export { ambiguousPluginServerNames, resolveStepMcpServer } from "./step/mcp.ts"; export { describeStepMcpImportOutcome, runStepMcpImportPrompt } from "./step/mcp-import-prompt.ts"; export { hasStoredMcpOAuthCredential, loginMcpServer, logoutMcpServer } from "./step/mcp-oauth.ts"; export { resolveStepLoginProfiles } from "./step/onboarding.ts"; diff --git a/packages/coding-agent/src/modes/interactive-contract.ts b/packages/coding-agent/src/modes/interactive-contract.ts index 0c2f5724..b4b0a460 100644 --- a/packages/coding-agent/src/modes/interactive-contract.ts +++ b/packages/coding-agent/src/modes/interactive-contract.ts @@ -183,5 +183,6 @@ export interface StartupUiHooks { title: string, placeholder?: string, paths?: StartupTuiPathOptions, + examples?: readonly string[], ): Promise; } diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index 64909b88..fd9aabca 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -145,8 +145,11 @@ export async function runRpcMode(runtimeHost: AgentSessionRuntimeHost): Promise< ), input: (title, placeholder, opts) => - createDialogPromise(opts, undefined, { method: "input", title, placeholder, timeout: opts?.timeout }, (r) => - "cancelled" in r && r.cancelled ? undefined : "value" in r ? r.value : undefined, + createDialogPromise( + opts, + undefined, + { method: "input", title, placeholder, examples: opts?.examples, timeout: opts?.timeout }, + (r) => ("cancelled" in r && r.cancelled ? undefined : "value" in r ? r.value : undefined), ), notify(message: string, type?: "info" | "warning" | "error"): void { diff --git a/packages/coding-agent/src/modes/rpc/rpc-types.ts b/packages/coding-agent/src/modes/rpc/rpc-types.ts index 1c2a9f00..1565bd4a 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-types.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-types.ts @@ -252,6 +252,7 @@ export type RpcExtensionUIRequest = method: "input"; title: string; placeholder?: string; + examples?: readonly string[]; timeout?: number; } | { type: "extension_ui_request"; id: string; method: "editor"; title: string; prefill?: string } diff --git a/packages/coding-agent/src/step/mcp-credential-key.test.ts b/packages/coding-agent/src/step/mcp-credential-key.test.ts new file mode 100644 index 00000000..f92c06a1 --- /dev/null +++ b/packages/coding-agent/src/step/mcp-credential-key.test.ts @@ -0,0 +1,68 @@ +/** + * The credential store is keyed by `|`, and the two sides must agree + * on `name`: `step mcp login` writes the key, the MCP connection reads it. A + * plugin's servers are published as `__`, so a login that + * stored the user's bare input would report success and then fail the same way + * on the next start. This file exists separately from `mcp-startup.test.ts` + * because that file mocks the credential reader out entirely. + */ + +import { mkdir, mkdtemp, realpath, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, expect, test, vi } from "vitest"; +import { resolveStepMcpServer } from "./mcp.ts"; +import { hasStoredMcpOAuthCredential } from "./mcp-oauth.ts"; + +const pluginMocks = vi.hoisted(() => ({ dirs: [] as string[], manifests: new Map() })); +vi.mock("./plugins.ts", () => ({ + defaultStepPluginsDir: () => "/unused-test-plugins", + listStepPluginDirectories: async () => pluginMocks.dirs, + readStepPluginManifest: async (dir: string) => ({ manifest: pluginMocks.manifests.get(dir), errors: [] }), + ensureBuiltinPluginsInstalled: async () => ({ installed: [], warnings: [] }), + provisionBuiltinPlugin: async () => undefined, +})); +vi.mock("./config-toml.ts", () => ({ readGlobalStepConfig: () => ({}) })); + +const cleanups: Array<() => Promise> = []; +afterEach(async () => { + for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); + pluginMocks.dirs = []; + pluginMocks.manifests.clear(); +}); + +test("login and the runtime agree on the credential key", async () => { + // Credentials sit beside config.toml: the parent of the agent directory. + const root = await realpath(await mkdtemp(join(await realpath(tmpdir()), "cred-"))); + cleanups.push(async () => rm(root, { recursive: true, force: true })); + const env = { ...process.env, STEP_CODING_AGENT_DIR: join(root, "agent") }; + await mkdir(join(root, "agent"), { recursive: true }); + + const dir = "/mock-plugins/context7"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "context7", + mcpServers: { context7: { url: "https://mcp.context7.com/mcp" } }, + }); + + // The CLI is handed this resolved name; passing the user's bare input instead + // would write a key the connection never reads. + const resolved = await resolveStepMcpServer("context7"); + expect(resolved?.name).toBe("context7__context7"); + + const url = "https://mcp.context7.com/mcp"; + const write = (key: string) => + writeFile( + join(root, ".credentials.json"), + JSON.stringify({ [key]: { serverName: key, serverUrl: url, tokens: { access_token: "t" } } }), + ); + + // The bare-name key is what a login using the user's input stores, and the + // runtime never looks it up. + await write(`context7|${url}`); + expect(hasStoredMcpOAuthCredential("context7__context7", url, env)).toBe(false); + + // The published key is what both sides use after the fix. + await write(`context7__context7|${url}`); + expect(hasStoredMcpOAuthCredential("context7__context7", url, env)).toBe(true); +}); diff --git a/packages/coding-agent/src/step/mcp-startup.test.ts b/packages/coding-agent/src/step/mcp-startup.test.ts index cdb6eaee..8e1361dd 100644 --- a/packages/coding-agent/src/step/mcp-startup.test.ts +++ b/packages/coding-agent/src/step/mcp-startup.test.ts @@ -1,17 +1,35 @@ +import { mkdir, mkdtemp, realpath, rm, writeFile } from "node:fs/promises"; import { createServer } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { setImmediate as yieldToEventLoop } from "node:timers/promises"; +import { StreamableHTTPError } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { afterEach, expect, test, vi } from "vitest"; import { createEventBus } from "../core/event-bus.ts"; import { createExtensionRuntime, loadExtensionFromFactory } from "../core/extensions/loader.ts"; import type { ExtensionMode } from "../core/extensions/types.ts"; import type { StepConfigDocument } from "./config-toml.ts"; -import { createStepMcpExtension, getStepMcpStatuses } from "./mcp.ts"; +import { + ambiguousPluginServerNames, + bareServerName, + createStepMcpExtension, + describeMcpStartFailure, + discoverStepMcpServers, + expandHeaderTemplate, + getStepMcpStatuses, + resolveStepMcpServer, +} from "./mcp.ts"; const config = vi.hoisted(() => ({ value: {} as StepConfigDocument })); vi.mock("./config-toml.ts", () => ({ readGlobalStepConfig: () => config.value })); +const pluginMocks = vi.hoisted(() => ({ + dirs: [] as string[], + manifests: new Map(), +})); vi.mock("./plugins.ts", () => ({ defaultStepPluginsDir: () => "/unused-test-plugins", - listStepPluginDirectories: async () => [], + listStepPluginDirectories: async () => pluginMocks.dirs, + readStepPluginManifest: async (dir: string) => ({ manifest: pluginMocks.manifests.get(dir), errors: [] }), ensureBuiltinPluginsInstalled: async () => ({ installed: [], warnings: [] }), provisionBuiltinPlugin: async () => undefined, })); @@ -19,6 +37,9 @@ vi.mock("./mcp-oauth.ts", () => ({ hasStoredMcpOAuthCredential: () => false })); const cleanups: Array<() => Promise> = []; afterEach(async () => { for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); + pluginMocks.dirs = []; + pluginMocks.manifests.clear(); + config.value = {}; }); async function slowServer(toolCount = 1) { @@ -70,6 +91,12 @@ async function slowServer(toolCount = 1) { return { url: `http://127.0.0.1:${address.port}/mcp`, release, requested: () => requested }; } +/** The literal `${VAR}` / `${VAR:-fallback}` text a plugin writes in a header value. */ +function placeholder(name: string, fallback?: string): string { + const body = fallback === undefined ? name : `${name}:-${fallback}`; + return `${"$"}${"{"}${body}${"}"}`; +} + async function setup(mode: ExtensionMode) { const runtime = createExtensionRuntime(); const extension = await loadExtensionFromFactory(createStepMcpExtension(), process.cwd(), createEventBus(), runtime); @@ -193,3 +220,242 @@ test("a missing header environment variable fails the server instead of sending ); expect(harness.extension.tools.size).toBe(0); }); + +test("discovers a remote plugin server declared with a url and no command", async () => { + const server = await slowServer(2); + const dir = "/mock-plugins/remote"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "remote", + mcpServers: { docs: { type: "http", url: server.url } }, + }); + // Gate on a url instead of a command: a plugin server with no command used to + // be skipped silently, so the same entry worked from config.toml but not here. + const harness = await setup("tui"); + await harness.start(); + server.release(); + await vi.waitFor(() => expect(harness.extension.tools.size).toBe(2)); + expect(getStepMcpStatuses()).toContainEqual({ name: "remote__docs", status: "connected", toolCount: 2 }); +}); + +test("carries Claude plugin headers, including environment interpolation", async () => { + process.env.STEP_TEST_HEADER_VALUE = "from-env"; + const seen: Array = []; + const received = createServer(async (req, res) => { + if (req.method !== "POST") { + // The transport probes with GET/HEAD; only the JSON-RPC POST carries a body. + res.writeHead(405).end(); + return; + } + seen.push(req.headers["x-from-env"] as string | undefined); + seen.push(req.headers.authorization as string | undefined); + const chunks: Buffer[] = []; + for await (const chunk of req) chunks.push(Buffer.from(chunk)); + const message = JSON.parse(Buffer.concat(chunks).toString()) as { id?: number; method: string }; + if (message.id === undefined) { + res.writeHead(202).end(); + return; + } + const result = + message.method === "initialize" + ? { + protocolVersion: "2025-03-26", + capabilities: { tools: {} }, + serverInfo: { name: "headers-test", version: "1" }, + } + : { tools: [] }; + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ jsonrpc: "2.0", id: message.id, result })); + }); + await new Promise((resolve) => received.listen(0, "127.0.0.1", resolve)); + const address = received.address(); + if (!address || typeof address === "string") throw new Error("Missing test port"); + cleanups.push(async () => { + received.closeAllConnections(); + await new Promise((resolve) => received.close(() => resolve())); + delete process.env.STEP_TEST_HEADER_VALUE; + }); + + const dir = "/mock-plugins/headers"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "headers", + mcpServers: { + docs: { + type: "http", + url: `http://127.0.0.1:${address.port}/mcp`, + // The Claude plugin spelling, with an unset variable behind a default. + // Built by concatenation so the literal placeholder text is the + // fixture rather than something a template literal would interpolate. + headers: { + Authorization: `Bearer ${placeholder("CONTEXT7_API_KEY", "")}`, + "X-From-Env": placeholder("STEP_TEST_HEADER_VALUE"), + }, + }, + }, + }); + const harness = await setup("tui"); + await harness.start(); + await vi.waitFor(() => expect(seen.length).toBeGreaterThan(0)); + expect(seen).toContain("from-env"); + // The unset key expands to nothing, so no Authorization header is sent at all + // rather than the malformed `Bearer ` an empty default would produce. + expect(seen).not.toContain("Bearer"); + expect(seen).toContain(undefined); +}); + +test("expands header templates and reports a variable with no fallback", () => { + process.env.STEP_TEST_PRESENT = "value"; + expect(expandHeaderTemplate("literal")).toBe("literal"); + expect(expandHeaderTemplate(placeholder("STEP_TEST_PRESENT"))).toBe("value"); + expect(expandHeaderTemplate(`Bearer ${placeholder("STEP_TEST_PRESENT")}`)).toBe("Bearer value"); + expect(expandHeaderTemplate(`Bearer ${placeholder("STEP_TEST_ABSENT", "anonymous")}`)).toBe("Bearer anonymous"); + // No fallback and no variable: the caller omits the header rather than + // sending the template text to the server. + expect(expandHeaderTemplate(`Bearer ${placeholder("STEP_TEST_ABSENT")}`)).toBeUndefined(); + // The empty default a plugin uses to mean "omit when unset". A plain + // interpolation would produce the malformed `Bearer ` instead. + expect(expandHeaderTemplate(`Bearer ${placeholder("STEP_TEST_ABSENT", "")}`)).toBeUndefined(); + expect(expandHeaderTemplate(`cost: ${"$$"}5 ${placeholder("STEP_TEST_PRESENT")}`)).toBe("cost: $5 value"); + delete process.env.STEP_TEST_PRESENT; +}); + +test("resolves a plugin whose mcpServers points at a sibling .mcp.json", async () => { + const server = await slowServer(1); + // The string form names a real file, so this needs a real directory on disk. + const root = await mkdtemp(join(await realpath(tmpdir()), "mcp-plugin-file-")); + cleanups.push(async () => rm(root, { recursive: true, force: true })); + const dir = join(root, "external"); + await mkdir(dir, { recursive: true }); + // The Claude layout: the manifest names the file rather than carrying the map. + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { id: "external", mcpServers: ".mcp.json" }); + await writeFile(join(dir, ".mcp.json"), JSON.stringify({ mcpServers: { docs: { type: "http", url: server.url } } })); + const harness = await setup("tui"); + await harness.start(); + server.release(); + await vi.waitFor(() => expect(harness.extension.tools.size).toBe(1)); + expect(getStepMcpStatuses()).toContainEqual({ name: "external__docs", status: "connected", toolCount: 1 }); +}); + +test("ignores an mcpServers path that escapes the plugin", async () => { + // Write a perfectly valid file outside the plugin, then point at it. + const root = await mkdtemp(join(await realpath(tmpdir()), "mcp-plugin-escape-")); + cleanups.push(async () => rm(root, { recursive: true, force: true })); + await writeFile(join(root, "outside.json"), JSON.stringify({ mcpServers: { evil: { command: "false" } } })); + const dir = join(root, "plugin"); + await mkdir(dir, { recursive: true }); + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { id: "escapes", mcpServers: "../outside.json" }); + + const harness = await setup("tui"); + await harness.start(); + // Nothing is read from outside the plugin directory. + expect(getStepMcpStatuses()).toEqual([]); + expect(harness.extension.tools.size).toBe(0); +}); + +test("resolves a plugin server by its bare name as well as its published one", async () => { + const dir = "/mock-plugins/named"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "context7", + mcpServers: { context7: { type: "http", url: "https://example.invalid/mcp" } }, + }); + + // `step mcp login context7` is what a user reaches for, and what the start + // failure now suggests; the qualifier is only there to disambiguate. + const bare = await resolveStepMcpServer("context7"); + expect(bare?.name).toBe("context7__context7"); + expect(bare?.declaration.url).toBe("https://example.invalid/mcp"); + + // The published spelling keeps working for callers that have it. + const published = await resolveStepMcpServer("context7__context7"); + expect(published?.declaration.url).toBe("https://example.invalid/mcp"); +}); + +test("a bare name still resolves when the plugin and server names differ", async () => { + const dir = "/mock-plugins/mismatched"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "claude-plugins-official", + mcpServers: { docs: { url: "https://example.invalid/docs" } }, + }); + const resolved = await resolveStepMcpServer("docs"); + expect(resolved?.name).toBe("claude-plugins-official__docs"); +}); + +test("an ambiguous bare name resolves to nothing instead of picking one", async () => { + const alpha = "/mock-plugins/alpha"; + const beta = "/mock-plugins/beta"; + pluginMocks.dirs = [alpha, beta]; + pluginMocks.manifests.set(alpha, { id: "alpha", mcpServers: { docs: { url: "https://alpha.invalid/mcp" } } }); + pluginMocks.manifests.set(beta, { id: "beta", mcpServers: { docs: { url: "https://beta.invalid/mcp" } } }); + + // Two plugins declare `docs`. Logging into whichever discovery reached first + // would be a silent coin flip, so the bare name is reported as ambiguous. + expect(await resolveStepMcpServer("docs")).toBeUndefined(); + expect(await ambiguousPluginServerNames("docs")).toEqual(["alpha__docs", "beta__docs"]); + + // The qualified spellings stay unambiguous. + expect((await resolveStepMcpServer("alpha__docs"))?.declaration.url).toBe("https://alpha.invalid/mcp"); + // A single match is not reported as ambiguous. + expect(await ambiguousPluginServerNames("nothing-here")).toEqual([]); +}); + +test("an unqualified name does not match a server whose own name ends with it", async () => { + const dir = "/mock-plugins/notasuffix"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { id: "other", mcpServers: { mydocs: { url: "https://example.invalid/x" } } }); + // `docs` must not match `other__mydocs`: the separator is what makes the + // trailing segment a name rather than an arbitrary suffix. + expect(await resolveStepMcpServer("docs")).toBeUndefined(); + expect((await resolveStepMcpServer("mydocs"))?.name).toBe("other__mydocs"); +}); + +test("prefers a config entry over a plugin server of the same name", async () => { + const dir = "/mock-plugins/shadowed"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "shared", + mcpServers: { docs: { url: "https://from-plugin.invalid/mcp" } }, + }); + config.value = { mcp_servers: { shared__docs: { url: "https://from-config.invalid/mcp" } } }; + + const resolved = await resolveStepMcpServer("shared__docs"); + expect(resolved?.declaration.url).toBe("https://from-config.invalid/mcp"); +}); + +test("reports an unknown name as undefined rather than throwing", async () => { + expect(await resolveStepMcpServer("nothing-here")).toBeUndefined(); +}); + +test("the auth hint prints the bare server name", () => { + // The 401 path is keyed on the transport's own error type, which is what a + // real unauthenticated remote server raises. + const hint = describeMcpStartFailure({ + name: "context7__context7", + command: "https://mcp.context7.com/mcp", + error: new StreamableHTTPError(401, "Authentication required."), + }); + expect(hint).toContain("step mcp login context7, then restart Step."); + + // An unrelated server keeps its own name unchanged. + expect(bareServerName("figma-mcp")).toBe("figma-mcp"); + expect(bareServerName("context7__context7")).toBe("context7"); + // Only the plugin qualifier is stripped. A declared name may itself contain + // the separator, and splitting at the last one would mangle it. + expect(bareServerName("acme__my__service")).toBe("my__service"); +}); + +test("a config.toml http_headers value is sent verbatim, not interpolated", async () => { + // The documented config surface is a literal string map. Expanding `${...}` + // here would silently rewrite values that already worked. + const literal = `Bearer ${placeholder("NOT_SET")}`; + config.value = { + mcp_servers: { literal: { url: "https://example.invalid/mcp", http_headers: { Authorization: literal } } }, + }; + const discovered = await discoverStepMcpServers(process.cwd(), false); + const found = discovered.find((server) => server.name === "literal"); + expect(found?.declaration.http_headers?.Authorization).toBe(literal); +}); diff --git a/packages/coding-agent/src/step/mcp.ts b/packages/coding-agent/src/step/mcp.ts index 4b0bef20..8b12b96e 100644 --- a/packages/coding-agent/src/step/mcp.ts +++ b/packages/coding-agent/src/step/mcp.ts @@ -1,3 +1,5 @@ +import { readFile } from "node:fs/promises"; +import path from "node:path"; import { setImmediate as yieldToEventLoop } from "node:timers/promises"; import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; @@ -27,7 +29,9 @@ 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; -const STEPPAGE_SERVER_NAME = "steppage__steppage"; +/** Joins a plugin id to the server name it declares, e.g. `context7__context7`. */ +const PLUGIN_SERVER_SEPARATOR = "__"; +const STEPPAGE_SERVER_NAME = `steppage${PLUGIN_SERVER_SEPARATOR}steppage`; const STEPPAGE_DEPLOY_TOOL_NAME = "page_deploy"; const STEPPAGE_MANAGEMENT_URL = "https://platform.stepfun.com/sites"; @@ -270,6 +274,65 @@ async function closeStepMcpServer(server: Pick 0 ? published.slice(separator + PLUGIN_SERVER_SEPARATOR.length) : published; +} + +/** + * Look up one configured MCP server by the name the runtime reports for it. + * + * `step mcp login|logout` is handed a name from a start-failure message, which is + * the *discovered* name. Config entries keep their own name, but a plugin's + * servers are published as `__` because the manifest nests + * them — so a login command that only read `config.toml` could never resolve the + * very name its own error message printed. + * + * Discovery is consulted only as a fallback: a config entry with the same name + * still wins, matching the precedence used everywhere else. + */ +export async function resolveStepMcpServer( + name: string, + options: { cwd?: string; projectTrusted?: boolean; env?: NodeJS.ProcessEnv } = {}, +): Promise { + const env = options.env ?? process.env; + const config = readGlobalStepConfig(env); + const entry = config.mcp_servers?.[name]; + if (isRecord(entry) && entry.enabled !== false) return { name, declaration: normalizeDeclaration(entry) }; + const discovered = await discoverStepMcpServers(options.cwd ?? process.cwd(), options.projectTrusted ?? false); + const exact = discovered.find((server) => server.name === name); + if (exact) return exact; + // A plugin's server is published as `__`, but the part + // that identifies it to the user is the server name the manifest declared. + // Accept that bare name too, so `step mcp login context7` works on the + // `context7__context7` server — the qualifier only disambiguates. + const qualified = `${PLUGIN_SERVER_SEPARATOR}${name}`; + const matches = discovered.filter((server) => server.name.endsWith(qualified)); + // Two plugins can declare a server of the same name. Picking whichever + // discovery reached first would log the user into an arbitrary one, so an + // ambiguous bare name resolves to nothing; the caller reports it as ambiguous. + return matches.length === 1 ? matches[0] : undefined; +} + +/** + * The published names of plugin servers whose declared name matches `name`. + * + * A caller needs this to tell "no such server" from "several plugins declare + * one": an ambiguous bare name must be reported, not resolved to a guess. Empty + * means nothing matched; two or more means the name needs qualifying. + */ +export async function ambiguousPluginServerNames(name: string): Promise { + const discovered = await discoverStepMcpServers(process.cwd(), false); + const qualified = `${PLUGIN_SERVER_SEPARATOR}${name}`; + const matches = discovered.filter((server) => server.name.endsWith(qualified)).map((server) => server.name); + return matches.length > 1 ? matches : []; +} + export async function discoverStepMcpServers(cwd: string, projectTrusted: boolean): Promise { const roots = [defaultStepPluginsDir(process.env)]; if (projectTrusted) roots.push(defaultStepPluginsDir(process.env, { cwd, project: true })); @@ -288,16 +351,20 @@ export async function discoverStepMcpServers(cwd: string, projectTrusted: boolea for (const pluginDir of await listStepPluginDirectories(root)) { const parsed = await readStepPluginManifest(pluginDir); if (!parsed.manifest) continue; - const declared = parsed.manifest?.mcpServers; - if (!declared || typeof declared === "string") continue; + const declared = await resolveDeclaredServers(pluginDir, parsed.manifest?.mcpServers); + if (!declared) continue; for (const [serverName, value] of Object.entries(declared)) { - if (!isRecord(value) || typeof value.command !== "string" || !value.command.trim()) continue; - const name = `${parsed.manifest.id}__${serverName}`; + // A declared server may be stdio (command) or remote (url). Requiring + // `command` here dropped remote servers silently, even though the + // global-config path and `normalizeDeclaration` both accept a url — + // so the same server worked from config.toml but not from a plugin. + if (!isRecord(value) || !hasTransport(value)) continue; + const name = `${parsed.manifest.id}${PLUGIN_SERVER_SEPARATOR}${serverName}`; if (seen.has(name)) continue; seen.add(name); const discovered: DiscoveredServer = { name, - declaration: normalizeDeclaration(value), + declaration: applyPluginHeaderAliases(normalizeDeclaration(value), value), }; if (parsed.manifest.provision) discovered.provision = parsed.manifest.provision; result.push(discovered); @@ -307,6 +374,104 @@ export async function discoverStepMcpServers(cwd: string, projectTrusted: boolea return result; } +/** True when a declaration names either transport: a stdio command or a url. */ +function hasTransport(value: Record): boolean { + const command = typeof value.command === "string" ? value.command.trim() : ""; + const url = typeof value.url === "string" ? value.url.trim() : ""; + return command.length > 0 || url.length > 0; +} + +/** + * Expand the environment interpolation Claude Code plugins use in header values: + * `${VAR}` inserts the variable, `${VAR:-fallback}` falls back when it is unset + * or empty, and `$$` escapes a literal dollar sign. + * + * Returns undefined when there is no value to send, so the caller omits the + * header entirely. That covers two cases a plugin uses interchangeably to mean + * "no credential configured": a variable with no fallback at all, and an empty + * fallback as in `Authorization: "Bearer ${CONTEXT7_API_KEY:-}"`. The second is + * not equivalent to sending `Bearer ` — that is a malformed credential, and a + * server rejecting it reads as a broken plugin rather than an unset key. + */ +export function expandHeaderTemplate(template: string): string | undefined { + const pattern = /\$\$|\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}/gu; + let missing = false; + const expanded = template.replace(pattern, (match, name: string | undefined, fallback: string | undefined) => { + if (match === "$$") return "$"; + const value = name === undefined ? undefined : process.env[name]; + if (value !== undefined && value !== "") return value; + if (fallback !== undefined && fallback !== "") return fallback; + // Unset with no usable fallback: the header has no value, and the literal + // template must never reach the server. + missing = true; + return ""; + }); + return missing ? undefined : expanded; +} + +/** True when `candidate` stays inside `root`, so a manifest cannot read elsewhere. */ +function isInside(root: string, candidate: string): boolean { + const relative = path.relative(path.resolve(root), path.resolve(candidate)); + return relative === "" || (!relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); +} + +/** + * Resolve a manifest's `mcpServers` into a server map. + * + * The field is either the map itself or a path to a file holding it. The path + * form is how Claude Code plugins keep MCP config in a sibling `.mcp.json`, and + * `readPluginManifestAtPath` fills it in automatically when it finds that file — + * so a plugin reaching this point with a string is the normal Claude layout, not + * a malformed one. Treating it as unsupported dropped every such plugin's + * servers without a word. + */ +async function resolveDeclaredServers( + pluginDir: string, + declared: unknown, +): Promise | undefined> { + if (isRecord(declared)) return declared; + if (typeof declared !== "string" || !declared.trim()) return undefined; + const resolved = path.resolve(pluginDir, declared); + if (!isInside(pluginDir, resolved)) return undefined; + try { + const raw = JSON.parse(await readFile(resolved, "utf8")) as unknown; + // The file may hold the map directly or wrap it under `mcpServers`, the + // shape `.mcp.json` itself uses. + if (isRecord(raw) && isRecord(raw.mcpServers)) return raw.mcpServers; + return isRecord(raw) ? raw : undefined; + } catch { + // A missing, unreadable, or non-file target (a directory rejects with + // EISDIR). Nothing to declare either way. + return undefined; + } +} + +/** + * Apply the Claude-plugin header spelling to an already-normalized declaration. + * + * Kept separate from {@link normalizeDeclaration} because the two entries have + * different contracts. A plugin's `.mcp.json` may write `headers` and interpolate + * the environment (`"Bearer ${API_KEY:-}"`), which is Claude's format. A + * `config.toml` entry documents `http_headers` as a literal string map, so its + * values are sent verbatim — expanding them here would silently rewrite or drop + * headers that already worked. + */ +function applyPluginHeaderAliases(declaration: ServerDeclaration, value: Record): ServerDeclaration { + if (!isRecord(value.headers)) return declaration; + const headers: Record = {}; + for (const [name, entry] of Object.entries(value.headers)) { + if (typeof entry !== "string") continue; + const expanded = expandHeaderTemplate(entry); + // A header whose variable is unset and has no fallback has no value to send, + // and the literal template must never reach the server. + if (expanded !== undefined) headers[name] = expanded; + } + // An explicit `http_headers` still wins: it is the more specific spelling, and + // a manifest that carries both should not have one silently discarded. + if (Object.keys(headers).length === 0 || declaration.http_headers !== undefined) return declaration; + return { ...declaration, http_headers: headers }; +} + function normalizeDeclaration(value: Record): ServerDeclaration { const declaration: ServerDeclaration = {}; if (typeof value.command === "string" && value.command.trim()) declaration.command = value.command.trim(); @@ -324,7 +489,11 @@ function normalizeDeclaration(value: Record): ServerDeclaration if (key === "startup_timeout_sec" && typeof value[key] === "number") declaration.startup_timeout_sec = value[key]; if (key === "tool_timeout_sec" && typeof value[key] === "number") declaration.tool_timeout_sec = value[key]; } - for (const key of ["http_headers", "env_http_headers"] as const) { + if (isRecord(value.http_headers)) + declaration.http_headers = Object.fromEntries( + Object.entries(value.http_headers).filter(([, v]) => typeof v === "string"), + ) as Record; + for (const key of ["env_http_headers"] as const) { if (isRecord(value[key])) declaration[key] = Object.fromEntries( Object.entries(value[key]).filter(([, v]) => typeof v === "string"), @@ -462,8 +631,12 @@ export function describeMcpStartFailure(input: { input.error instanceof UnauthorizedError || (input.error instanceof StreamableHTTPError && input.error.code === 401) ) { - const name = /^[\w.-]+$/u.test(input.name) ? input.name : `'${input.name.replace(/'/gu, "'\\''")}'`; - return `MCP server '${input.name}' could not start: ${detail}\nAuthenticate with: step mcp login ${name}, then restart Step.`; + // Print the bare server name, not the `__` form the + // runtime publishes: the qualifier is an implementation detail of how + // plugin servers are namespaced, and `login` accepts either spelling. + const bare = bareServerName(input.name); + const quoted = /^[\w.-]+$/u.test(bare) ? bare : `'${bare.replace(/'/gu, "'\\''")}'`; + return `MCP server '${input.name}' could not start: ${detail}\nAuthenticate with: step mcp login ${quoted}, then restart Step.`; } if (!isMissingExecutable(input.error)) return `MCP server '${input.name}' could not start: ${detail}`; const install = input.provision ? provisionInstallCommand(input.provision, input.env ?? process.env) : undefined; @@ -523,7 +696,7 @@ export function convertMcpCallResult(serverName: string, toolName: string, resul } function remoteToolName(server: Pick, remote: McpTool): string { - return `${server.name}__${sanitizeName(remote.name)}`; + return `${server.name}${PLUGIN_SERVER_SEPARATOR}${sanitizeName(remote.name)}`; } function createRemoteTool(server: ConnectedServer, remote: McpTool) { diff --git a/packages/coding-agent/src/step/plugins.ts b/packages/coding-agent/src/step/plugins.ts index 4ed67f0b..1bd996d6 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -14,7 +14,8 @@ import fs from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; -import type { ExtensionAPI, ExtensionCommandContext } from "../core/extensions/types.ts"; +import { BorderedLoader } from "../components/bordered-loader.ts"; +import type { ExtensionAPI, ExtensionCommandContext, ExtensionFactory } 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"; @@ -404,11 +405,21 @@ export async function listStepPluginDirectories(root: string): Promise } } -/** Discover installable entries from one or more local marketplace roots. */ +/** + * Discover installable entries from one or more local marketplace roots. + * + * The same marketplace checkout can be reachable from more than one root: the + * built-in marketplace is materialized under each root's `marketplaces` + * directory, and a project root is scanned alongside the global one. Roots are + * already ordered by precedence, so an entry is kept only the first time its + * name is seen and later duplicates are dropped — otherwise every built-in + * plugin would be listed once per root. + */ export async function listMarketplacePlugins( marketplaceRoots: readonly string[] = defaultMarketplaceRoots(), ): Promise { const entries: MarketplacePluginEntry[] = []; + const seenPluginNames = new Set(); const warnings: string[] = []; for (const root of marketplaceRoots) { const candidates = (await findMarketplaceManifest(root)) ? [root] : await listStepPluginDirectories(root); @@ -440,6 +451,10 @@ export async function listMarketplacePlugins( warnings.push(`Marketplace '${marketplaceName}' entry '${name}' is not a safe plugin name; skipped.`); continue; } + // Dedupe before probing the source: a shadowed entry is never listed, + // so warning about its missing checkout would describe a problem the + // user does not have while the visible copy works. + if (seenPluginNames.has(name)) continue; if (isRecord(value.source)) { const sourceKind = typeof value.source.source === "string" && value.source.source.trim() @@ -465,6 +480,9 @@ export async function listMarketplacePlugins( ); continue; } + // Precedence is "first root wins": a project-level marketplace of the + // same name shadows the global copy rather than adding a second entry. + seenPluginNames.add(name); entries.push({ name, description: typeof value.description === "string" ? value.description : undefined, @@ -696,6 +714,73 @@ function provisionedServerEnvironments(manifest: StepPluginManifest): Array 0 ? declared : [undefined]; } +/** + * Resolve the skill and command directories contributed by installed plugins. + * + * A plugin's non-MCP contributions live beside its manifest rather than in the + * agent's own resource directories, so nothing finds them by default: MCP is the + * one contribution that reads the plugin root directly. This is the bridge for + * the other two, returning paths that `ResourceLoader.extendResources` accepts. + * + * Declared entries win over convention. A manifest that names `skills` / + * `commands` is taken at its word (resolved against the plugin root, and skipped + * when it escapes or does not exist); otherwise the conventional `skills/` and + * `commands/` directories are used when present. This mirrors how + * `readPluginManifestAtPath` fills those fields in for Claude-style manifests. + */ +export async function discoverStepPluginResourcePaths(input: { + userDir?: string; + projectDir?: string; + /** Project plugins are read only when the project is trusted, as MCP discovery does. */ + projectTrusted?: boolean; +}): Promise<{ skillPaths: string[]; promptPaths: string[]; warnings: string[] }> { + const warnings: string[] = []; + const seenByKind = { skills: new Set(), commands: new Set() }; + const collected = { skills: [] as string[], commands: [] as string[] }; + // An untrusted project must not inject skill instructions or prompt templates + // into the session, so its plugin root is skipped entirely. The global root is + // always read: it is the user's own configuration, not the checkout's. + const roots = [ + ...(input.projectTrusted && input.projectDir ? [input.projectDir] : []), + input.userDir ?? defaultStepPluginsDir(), + ]; + + for (const root of roots) { + for (const pluginDir of await listStepPluginDirectories(root)) { + const read = await readStepPluginManifest(pluginDir); + if (!read.manifest) continue; + const manifest = read.manifest; + for (const key of ["skills", "commands"] as const) { + const declared = manifest[key]; + // A declared empty array means "this plugin contributes none": honor + // it instead of falling through to a stale conventional directory. + const candidates = + declared !== undefined ? declared : (await pathExists(path.join(pluginDir, key))) ? [key] : []; + for (const relative of candidates) { + const resolved = path.resolve(pluginDir, relative); + if (!isContained(pluginDir, resolved)) { + warnings.push( + `Plugin '${manifest.id}' declares ${key} '${relative}', which is outside the plugin; skipped.`, + ); + continue; + } + if (!(await pathExists(resolved))) { + warnings.push(`Plugin '${manifest.id}' declares ${key} '${relative}', which is missing; skipped.`); + continue; + } + // Dedupe within a kind, not across kinds: one directory can + // legitimately be both a skills root and a commands root. + const canonical = path.resolve(resolved); + if (seenByKind[key].has(canonical)) continue; + seenByKind[key].add(canonical); + collected[key].push(canonical); + } + } + } + } + return { skillPaths: collected.skills, promptPaths: collected.commands, warnings }; +} + export async function listInstalledStepPlugins( input: { userDir?: string; projectDir?: string } = {}, ): Promise<{ plugins: InstalledStepPlugin[]; warnings: string[] }> { @@ -907,7 +992,16 @@ export async function addMarketplaceSource(input: { source: string; marketplacesDir?: string; name?: string; + /** + * Called as the add moves through its stages. A git clone can run for a + * minute or more, and a caller that only hears back on completion has no way + * to show the user that anything is happening. + */ + onProgress?: (message: string) => void; + /** Aborts an in-flight clone when the caller's dialog is cancelled. */ + signal?: AbortSignal; }): Promise { + const report = input.onProgress ?? (() => {}); const source = input.source.trim(); if (!source) return { warnings: ["Marketplace source is empty."] }; const marketplacesDir = input.marketplacesDir ?? defaultStepMarketplacesDir(); @@ -960,16 +1054,17 @@ export async function addMarketplaceSource(input: { if (sourcePath) { const local = path.resolve(sourcePath); if (!(await pathExists(local))) return { warnings: [`Marketplace source does not exist: ${local}`] }; + report(`Copying ${local}`); await fs.cp(local, target, { recursive: true, errorOnExist: true, force: false }); } else { - await execFileAsync("git", ["clone", "--depth", "1", "--quiet", "--", cloneSource!, target], { - timeout: 120_000, - }); + report(`Cloning repository (timeout: 120s): ${cloneSource}`); + await cloneMarketplace(cloneSource!, target, report, input.signal); } } catch (error) { await fs.rm(target, { recursive: true, force: true }).catch(() => undefined); return { warnings: [`Could not add marketplace '${cloneName}': ${describe(error)}`] }; } + report("Reading marketplace manifest"); if (!(await findMarketplaceManifest(target))) { await fs.rm(target, { recursive: true, force: true }); return { warnings: [`${source} has no marketplace manifest; it was not added.`] }; @@ -985,6 +1080,50 @@ export async function addMarketplaceSource(input: { }; } +/** + * Clone a marketplace, forwarding git's progress lines as they arrive. + * + * `--progress` is what makes git emit percentage updates to stderr when it is + * not attached to a terminal; without it a large clone is silent until it + * finishes, which is indistinguishable from a hang. + */ +async function cloneMarketplace( + cloneSource: string, + target: string, + report: (message: string) => void, + signal?: AbortSignal, +): Promise { + await new Promise((resolve, reject) => { + const child = execFile( + "git", + ["clone", "--depth", "1", "--progress", "--", cloneSource, target], + { timeout: 120_000, ...(signal ? { signal } : {}) }, + (error) => (error ? reject(error) : resolve()), + ); + let pending = ""; + child.stderr?.on("data", (chunk: Buffer | string) => { + pending += chunk.toString(); + // Keep the trailing partial line: a chunk can end mid-update, and the + // rest arrives in the next one. + const parts = splitCloneProgress(pending); + pending = parts.pop() ?? ""; + for (const line of parts) report(`Cloning repository: ${line}`); + }); + }); +} + +/** + * Split accumulated git output into complete progress lines, keeping the last + * partial line as the final element. + * + * git redraws its progress with carriage returns instead of newlines, so + * splitting on newlines alone would hold the whole transfer in the buffer and + * leave the caller with a single update at the end. + */ +export function splitCloneProgress(pending: string): string[] { + return pending.split(/[\r\n]+/u).map((line) => line.trim()); +} + export async function removeMarketplaceSource(input: { name: string; marketplacesDir?: string; @@ -1034,6 +1173,43 @@ interface InteractivePluginOptions extends StepPluginCommandOptions { marketplaceRoots: readonly string[]; } +/** + * Contribute installed plugins' skills and commands to the session. + * + * MCP is discovered by reading the plugin root directly, but skills and commands + * are consumed by `ResourceLoader` from agent/project resource directories that + * plugins do not write to. This extension is the bridge: it answers + * `resources_discover` with the plugin-owned paths, which the loader already + * knows how to merge (`extendResources`) and re-merge on reload, so a plugin + * installed during a session takes effect on the next resource reload instead of + * requiring a restart. + * + * Failures are reported as warnings on the extension error channel rather than + * thrown: a broken plugin must not cost the session its other resources. + */ +export function createStepPluginResourcesExtension(): ExtensionFactory { + return (pi: ExtensionAPI): void => { + pi.on("resources_discover", async (_event, ctx) => { + const projectDir = defaultStepPluginsDir(process.env, { cwd: ctx.cwd, project: true }); + try { + const discovered = await discoverStepPluginResourcePaths({ + projectDir, + projectTrusted: ctx.isProjectTrusted(), + userDir: defaultStepPluginsDir(process.env), + }); + // Surface a malformed declaration once, on the channel the loader + // already uses for resource diagnostics, rather than throwing: the + // rest of the plugins' resources must still load. + for (const warning of discovered.warnings) ctx.ui?.notify?.(warning, "warning"); + return { skillPaths: discovered.skillPaths, promptPaths: discovered.promptPaths }; + } catch (error) { + ctx.ui?.notify?.(`Could not read plugin resources: ${describe(error)}`, "warning"); + return {}; + } + }); + }; +} + function reportPluginWarnings(say: PluginNotice, warnings: readonly string[]): void { if (warnings.length > 0) say(warnings.join("\n"), "warning"); } @@ -1055,18 +1231,18 @@ async function openInteractivePluginMenu( options: InteractivePluginOptions, say: PluginNotice, ): Promise { - // Materialize the built-in source before reading the source list so its - // count is stable in the top-level menu. + // Materialize the built-in source before listing so the available count is + // stable in the top-level menu. const available = await loadAvailablePlugins(options); - const [installed, sources] = await Promise.all([ - listInstalledStepPlugins({ userDir: options.pluginsDir, projectDir: projectPluginsDir(ctx) }), - listMarketplaceSources(options.marketplacesDir), - ]); + const installed = await listInstalledStepPlugins({ + userDir: options.pluginsDir, + projectDir: projectPluginsDir(ctx), + }); reportPluginWarnings(say, [...installed.warnings, ...available.builtinWarnings, ...available.warnings]); const choices = [ `Installed (${installed.plugins.length})`, - `Marketplace (${available.entries.length} available)`, - `Marketplaces (${sources.length})`, + `All Plugins (${available.entries.length} available)`, + "Marketplaces", ]; const selected = await ctx.ui.select("Plugins", choices); if (selected === choices[0]) await openInstalledPlugins(ctx, options, say); @@ -1152,7 +1328,11 @@ async function openMarketplacePlugins( const state = installedIds.has(entry.name) ? " · installed" : ""; return `${entry.name} · ${entry.marketplace}${state}`; }); - const selected = await ctx.ui.select(`Marketplace (${available.entries.length} available)`, labels); + const selected = await ctx.ui.select(`All Plugins (${available.entries.length} available)`, labels, { + // A marketplace can carry hundreds of plugins, so this list is the one + // place that needs a query rather than a scroll. + searchable: true, + }); if (!selected) return; const index = labels.indexOf(selected); const entry = index >= 0 ? available.entries[index] : undefined; @@ -1192,6 +1372,41 @@ async function openMarketplacePluginDetails( } } +/** + * Run `work` behind a spinner that shows the latest progress message. + * + * The marketplace add is the one `/plugin` action that can take minutes, and it + * used to hand the terminal back to the editor for its whole duration. The + * spinner keeps the dialog mounted and updates in place; hosts without a UI + * (print, RPC) simply run the work, since they have nothing to render into. + */ +async function runWithProgress( + ctx: ExtensionCommandContext, + work: (report: (message: string) => void, signal: AbortSignal) => Promise, +): Promise { + if (!ctx.hasUI || ctx.mode !== "tui") return work(() => undefined, new AbortController().signal); + const settled = await ctx.ui.custom<{ ok: true; value: T } | { ok: false; error: unknown }>( + (tui, theme, _keybindings, done) => { + // Cancellable: a clone can run for the full two-minute timeout, and a + // mistyped URL should not hold the command hostage with no way out. + const loader = new BorderedLoader(tui, theme, "Working...", { cancellable: true }); + const controller = new AbortController(); + loader.onAbort = () => controller.abort(); + void work((message) => loader.setMessage(message), controller.signal).then( + (value) => done({ ok: true, value }), + // The failure travels back as a value so the dialog always unmounts + // before the caller reports it; rejecting through the dialog would + // leave the spinner on screen next to the error. + (error: unknown) => done({ ok: false, error }), + ); + return loader; + }, + { hideFooter: true }, + ); + if (settled.ok) return settled.value; + throw settled.error; +} + async function openMarketplaceSources( ctx: ExtensionCommandContext, options: InteractivePluginOptions, @@ -1200,24 +1415,45 @@ async function openMarketplaceSources( const builtin = await ensureBuiltinMarketplace({ marketplacesDir: options.marketplacesDir }); const sources = await listMarketplaceSources(options.marketplacesDir); reportPluginWarnings(say, builtin.warnings); - const labels = sources.map((source) => - source.kind === "builtin" ? `${source.name} · built in` : `${source.name} · ${source.kind}`, - ); + // The built-in source ships with the CLI: it cannot be updated or removed, so + // listing it here would only add a row that does nothing. It stays out of the + // labels and the parallel `sources` slice so the index lookup stays aligned. + const manageable = sources.filter((source) => source.kind !== "builtin"); + const labels = manageable.map((source) => `${source.name} · ${source.kind}`); const addLabel = "Add marketplace"; - const selected = await ctx.ui.select(`Marketplaces (${sources.length})`, [...labels, addLabel]); + const selected = await ctx.ui.select("Marketplaces", [...labels, addLabel]); if (!selected) return; if (selected === addLabel) { - const source = await ctx.ui.input("Add marketplace", "git URL, owner/repo, or local path"); + const source = await ctx.ui.input("Add Marketplace\nEnter marketplace source:", undefined, { + examples: [ + "owner/repo (GitHub)", + "git@github.com:owner/repo.git (SSH)", + "https://example.com/marketplace.json", + "./path/to/marketplace", + ], + }); if (!source?.trim()) return; - const added = await addMarketplaceSource({ source, marketplacesDir: options.marketplacesDir }); + const added = await runWithProgress(ctx, (report, signal) => + addMarketplaceSource({ + source, + marketplacesDir: options.marketplacesDir, + onProgress: report, + signal, + }), + ); say( [...(added.source ? [`Added marketplace '${added.source.name}'.`] : []), ...added.warnings].join("\n"), added.source ? "info" : "warning", ); + // Land on the new marketplace's plugins instead of bouncing back to the + // source list: adding a source is only ever a step toward installing from it. + // A failed add keeps the user where they are so the warning stays adjacent + // to the list they can retry from. + if (added.source) await openMarketplacePlugins(ctx, options, say); return; } const index = labels.indexOf(selected); - const source = index >= 0 ? sources[index] : undefined; + const source = index >= 0 ? manageable[index] : undefined; if (source) await openMarketplaceSourceDetails(ctx, options, source, say); } @@ -1227,10 +1463,8 @@ async function openMarketplaceSourceDetails( source: MarketplaceSource, say: PluginNotice, ): Promise { - if (source.kind === "builtin") { - say(`${source.name} ships with StepCode and cannot be updated or removed.`); - return; - } + // `openMarketplaceSources` filters the built-in source out of the list, so + // only user-added marketplaces reach here — and both can be updated/removed. const details = `${source.origin ?? source.path}\nKind: ${source.kind}`; const selected = await ctx.ui.select(`${source.name}\n${details}`, ["Update", "Remove", "Back"]); if (selected === "Back") { @@ -1311,26 +1545,42 @@ export function registerStepPluginCommand(pi: ExtensionAPI, options: StepPluginC return; } case "install": { - const name = action[1]; - if (!name) { + // Accept the `name@marketplace` spelling that Claude Code uses + // alongside a bare name; the marketplace half only narrows the + // search, since a plugin name alone is already unambiguous here. + const requested = action[1]; + if (!requested) { say("Usage: /plugin install ", "warning"); return; } + const { name, marketplace } = parsePluginSpecifier(requested); await ensureBuiltinMarketplace({ marketplacesDir }); const available = await listMarketplacePlugins( options.marketplacesDir || options.storageRootDir ? [marketplacesDir] : defaultMarketplaceRoots(process.env, { includeProject: true }), ); - const entry = available.entries.find((candidate) => candidate.name === name); + const matches = available.entries.filter( + (candidate) => candidate.name === name && (!marketplace || candidate.marketplace === marketplace), + ); + const entry = matches[0]; if (!entry) { - say(`No marketplace entry named '${name}' is available locally.`, "warning"); + // Name the marketplaces that do carry this plugin: the bare name + // resolves across all of them, so a wrong marketplace half is + // the one failure the user cannot see from the message alone. + const elsewhere = available.entries.filter((candidate) => candidate.name === name); + const alternatives = + elsewhere.length > 0 + ? ` It is available from: ${[...new Set(elsewhere.map((candidate) => candidate.marketplace))].join(", ")}.` + : ""; + const wanted = marketplace ? `'${name}' from '${marketplace}'` : `'${name}'`; + say(`No marketplace entry named ${wanted} is available locally.${alternatives}`, "warning"); return; } const installed = await installMarketplacePlugin(entry, pluginsDir); say( [ - `Installed ${name} from ${entry.marketplace} to ${installed.installedPath}.`, + `Installed ${entry.name} from ${entry.marketplace} to ${installed.installedPath}.`, "Restart Step to start the plugin's MCP server.", ...installed.warnings, ].join("\n"), @@ -1457,6 +1707,32 @@ export function buildManifestFromMarketplaceEntry(entry: MarketplacePluginEntry) return manifest; } +/** + * Split the `name@marketplace` spelling into its two halves. + * + * The marketplace half is optional: a bare name is what `/plugin install` has + * always accepted, and Step resolves a name across every configured + * marketplace rather than requiring the qualifier. + */ +/** + * Split the `name@marketplace` spelling into its two halves. + * + * Only the last `@` can introduce the qualifier, and only when it is not the + * first character — a scoped npm-style name such as `@scope/plugin` has no + * qualifier at all. A name that itself contains `@` (`foo@bar@baz`) is + * ambiguous, so the whole specifier is taken as the name rather than silently + * resolving to `foo@bar` and reporting a marketplace miss the user cannot see. + */ +function parsePluginSpecifier(specifier: string): { name: string; marketplace?: string } { + const trimmed = specifier.trim(); + const separator = trimmed.lastIndexOf("@"); + if (separator <= 0) return { name: trimmed }; + const name = trimmed.slice(0, separator); + const marketplace = trimmed.slice(separator + 1); + if (!marketplace.trim() || name.includes("@")) return { name: trimmed }; + return { name, marketplace: marketplace.trim() }; +} + function deriveMarketplaceName(source: string): string | undefined { const trimmed = source.replace(/[\\/]+$/u, ""); const segment = trimmed diff --git a/packages/coding-agent/src/step/stdio-host.ts b/packages/coding-agent/src/step/stdio-host.ts index bd6e905a..410d0bd9 100644 --- a/packages/coding-agent/src/step/stdio-host.ts +++ b/packages/coding-agent/src/step/stdio-host.ts @@ -1181,7 +1181,12 @@ export class StepStdioHost { confirm: (title, message, opts) => dialog("user_dialog.request", { kind: "confirm", title, message }, false, opts), input: (title, placeholder, opts) => - dialog("user_dialog.request", { kind: "input", title, placeholder }, undefined, opts), + dialog( + "user_dialog.request", + { kind: "input", title, placeholder, examples: opts?.examples }, + undefined, + opts, + ), notify: (message, type) => this.#emitEvent("user_notification", { message, type }), onTerminalInput: () => () => {}, setStatus: (key, text) => this.#emitEvent("ui.status", { key, text }), diff --git a/packages/coding-agent/test/step-plugins.test.ts b/packages/coding-agent/test/step-plugins.test.ts index 8f267680..96e76b24 100644 --- a/packages/coding-agent/test/step-plugins.test.ts +++ b/packages/coding-agent/test/step-plugins.test.ts @@ -1,17 +1,19 @@ import { execFile } from "node:child_process"; -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { pathToFileURL } from "node:url"; import { promisify } from "node:util"; import { afterEach, describe, expect, test, vi } from "vitest"; import type { ExtensionAPI, ExtensionCommandContext, RegisteredCommand } from "../src/core/extensions/types.ts"; +import { loadSkillsFromDir } from "../src/core/skills.ts"; import { addMarketplaceSource, BUILTIN_MARKETPLACE_NAME, defaultStepMarketplacesDir, defaultStepPluginsDir, diagnoseStepPlugin, + discoverStepPluginResourcePaths, ensureBuiltinMarketplace, ensureBuiltinPluginsInstalled, installMarketplacePlugin, @@ -20,6 +22,7 @@ import { listMarketplaceSources, parseStepPluginManifest, registerStepPluginCommand, + splitCloneProgress, uninstallPlugin, updateMarketplaceSource, } from "../src/step/plugins.ts"; @@ -77,6 +80,241 @@ describe("Step plugin marketplace facade", () => { expect(listed.entries.every((entry) => entry.sourcePath.startsWith(result.path))).toBe(true); }); + test("lists each plugin once when the same marketplace is reachable from two roots", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-twice-")); + roots.push(root); + // The built-in marketplace is materialized under every root, and a project + // root is scanned alongside the global one, so the same checkout is + // reachable twice. The listing must not grow with the root count. + const globalRoot = join(root, "global", "marketplaces"); + const projectRoot = join(root, "project", "marketplaces"); + for (const marketplacesDir of [globalRoot, projectRoot]) { + await ensureBuiltinMarketplace({ marketplacesDir }); + } + await writeMarketplace(join(globalRoot, "plan-to-lark"), { + name: "plan-to-lark", + plugins: [{ name: "plan-to-lark", source: "plugins/plan-to-lark" }], + }); + + const listed = await listMarketplacePlugins([projectRoot, globalRoot]); + expect(listed.entries.map((entry) => entry.name)).toEqual(["playwright", "steppage", "plan-to-lark"]); + expect(listed.warnings).toEqual([]); + }); + + test("accepts the name@marketplace spelling and reports where a name does live", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-spec-")); + roots.push(root); + const marketplacesDir = join(root, "marketplaces"); + const pluginsDir = join(root, "plugins"); + await writeMarketplace(join(marketplacesDir, "official"), { + name: "official", + plugins: [{ name: "skill-creator", source: "plugins/skill-creator" }], + }); + + const commands = new Map(); + // The handler captures its dirs from registration, not from the context. + const registerCommand = vi.fn((name: string, command: Omit) => { + commands.set(name, { ...command, name, sourceInfo: {} as RegisteredCommand["sourceInfo"] }); + }); + registerStepPluginCommand({ registerCommand } as unknown as ExtensionAPI, { marketplacesDir, pluginsDir }); + const notify = vi.fn(); + const ctx = { cwd: root, ui: { notify } } as unknown as ExtensionCommandContext; + + // The qualifier selects the marketplace rather than forming part of the name. + await commands.get("plugin")!.handler("install skill-creator@official", ctx); + const installedManifest = join(pluginsDir, "skill-creator", "step.plugin.json"); + expect(JSON.parse(await readFile(installedManifest, "utf8"))).toMatchObject({ id: "skill-creator" }); + + // A wrong qualifier names the marketplaces that do carry the plugin. + notify.mockClear(); + await commands.get("plugin")!.handler("install skill-creator@elsewhere", ctx); + expect(String(notify.mock.calls[0]?.[0])).toContain("It is available from: official"); + }); + + test("streams clone progress so a long add is not silent", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-progress-")); + roots.push(root); + const marketplacesDir = join(root, "marketplaces"); + const origin = join(root, "origin"); + await writeMarketplace(origin, { name: "origin", plugins: [{ name: "one", source: "one" }] }); + await mkdir(join(origin, "one"), { recursive: true }); + await writeFile(join(origin, "one", "step.plugin.json"), JSON.stringify({ id: "one" })); + await execFileAsync("git", ["init", "--quiet", origin]); + await execFileAsync("git", ["-C", origin, "add", "-A"]); + await execFileAsync("git", [ + "-C", + origin, + "-c", + "user.email=test@example.invalid", + "-c", + "user.name=test", + "commit", + "--quiet", + "-m", + "seed", + ]); + + const seen: string[] = []; + const added = await addMarketplaceSource({ + source: pathToFileURL(origin).toString(), + marketplacesDir, + onProgress: (message) => seen.push(message), + }); + expect(added.warnings).toEqual([]); + // Progress starts before the clone so the spinner is never blank, and the + // manifest check reports after it. + expect(seen[0]).toContain("Cloning repository (timeout: 120s)"); + expect(seen.at(-1)).toContain("Reading marketplace manifest"); + // git separates progress updates with carriage returns rather than + // newlines, so the parser must split on both or the spinner text would + // never change after the first update. + // The trailing empty element is the unfinished line the caller keeps as + // its buffer, so complete updates are the ones before it. + expect(splitCloneProgress("Receiving objects: 10%\rReceiving objects: 90%\r")).toEqual([ + "Receiving objects: 10%", + "Receiving objects: 90%", + "", + ]); + }); + + test("prefers the first root when two roots offer the same plugin name", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-precedence-")); + roots.push(root); + const projectRoot = join(root, "project", "marketplaces"); + const globalRoot = join(root, "global", "marketplaces"); + await writeMarketplace(join(projectRoot, "local"), { + name: "local", + plugins: [{ name: "shared", source: "shared" }], + }); + await writeMarketplace(join(globalRoot, "remote"), { + name: "remote", + plugins: [{ name: "shared", source: "shared" }], + }); + + const listed = await listMarketplacePlugins([projectRoot, globalRoot]); + expect(listed.entries).toHaveLength(1); + expect(listed.entries[0]?.marketplace).toBe("local"); + }); + + test("contributes plugin skills and commands as resource paths", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-resources-")); + roots.push(root); + const pluginsDir = join(root, "plugins"); + + // A plugin whose manifest names its contributions explicitly. + const declared = join(pluginsDir, "declared"); + await mkdir(join(declared, "my-skills"), { recursive: true }); + await mkdir(join(declared, "my-commands"), { recursive: true }); + await writeFile( + join(declared, "step.plugin.json"), + JSON.stringify({ id: "declared", skills: ["my-skills"], commands: ["my-commands"] }), + ); + + // A plugin relying on the conventional directories. + const conventional = join(pluginsDir, "conventional"); + await mkdir(join(conventional, "skills", "one"), { recursive: true }); + await mkdir(join(conventional, "commands"), { recursive: true }); + await writeFile(join(conventional, "step.plugin.json"), JSON.stringify({ id: "conventional" })); + + // A plugin with no such contributions at all. + const bare = join(pluginsDir, "bare"); + await mkdir(bare, { recursive: true }); + await writeFile(join(bare, "step.plugin.json"), JSON.stringify({ id: "bare" })); + + const discovered = await discoverStepPluginResourcePaths({ userDir: pluginsDir }); + expect(discovered.warnings).toEqual([]); + expect(discovered.skillPaths.sort()).toEqual([join(declared, "my-skills"), join(conventional, "skills")].sort()); + expect(discovered.promptPaths.sort()).toEqual( + [join(declared, "my-commands"), join(conventional, "commands")].sort(), + ); + }); + + test("withholds project plugin resources unless the project is trusted", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-trust-")); + roots.push(root); + const projectDir = join(root, "project"); + const userDir = join(root, "user"); + await writeMarketplacePlaceholder(join(projectDir, "from-project"), "project-skill"); + await writeMarketplacePlaceholder(join(userDir, "from-user"), "user-skill"); + + // Untrusted: only the user's own plugins contribute. + const untrusted = await discoverStepPluginResourcePaths({ projectDir, userDir, projectTrusted: false }); + expect(untrusted.skillPaths).toEqual([join(userDir, "from-user", "skills")]); + + // Trusted: the project's plugins are read too. + const trusted = await discoverStepPluginResourcePaths({ projectDir, userDir, projectTrusted: true }); + expect(trusted.skillPaths).toEqual([ + join(projectDir, "from-project", "skills"), + join(userDir, "from-user", "skills"), + ]); + }); + + test("keeps a path that is both a skills root and a commands root", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-both-")); + roots.push(root); + const pluginsDir = join(root, "plugins"); + const plugin = join(pluginsDir, "both"); + // One directory serving both kinds must not be deduped across them. + await mkdir(join(plugin, "shared"), { recursive: true }); + await writeFile( + join(plugin, "step.plugin.json"), + JSON.stringify({ id: "both", skills: ["shared"], commands: ["shared"] }), + ); + + const discovered = await discoverStepPluginResourcePaths({ userDir: pluginsDir }); + expect(discovered.skillPaths).toEqual([join(plugin, "shared")]); + expect(discovered.promptPaths).toEqual([join(plugin, "shared")]); + }); + + test("honors an explicitly empty contribution list over the conventions", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-empty-")); + roots.push(root); + const pluginsDir = join(root, "plugins"); + const plugin = join(pluginsDir, "none"); + // A stale conventional directory must not resurrect what the manifest + // explicitly declared as none. + await mkdir(join(plugin, "skills", "stale"), { recursive: true }); + await writeFile(join(plugin, "step.plugin.json"), JSON.stringify({ id: "none", skills: [] })); + + const discovered = await discoverStepPluginResourcePaths({ userDir: pluginsDir }); + expect(discovered.skillPaths).toEqual([]); + }); + + test("skips a declared contribution that is missing", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-missing-")); + roots.push(root); + const pluginsDir = join(root, "plugins"); + const plugin = join(pluginsDir, "bad"); + await mkdir(plugin, { recursive: true }); + await writeFile(join(plugin, "step.plugin.json"), JSON.stringify({ id: "bad", skills: ["gone"] })); + + const discovered = await discoverStepPluginResourcePaths({ userDir: pluginsDir }); + expect(discovered.skillPaths).toEqual([]); + expect(discovered.warnings).toHaveLength(1); + expect(discovered.warnings[0]).toContain("missing"); + }); + + test("ships plugin skills through the resource loader", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "step-plugins-skill-")); + roots.push(root); + const pluginsDir = join(root, "plugins"); + // The conventional layout Claude Code plugins use, which the plugin's own + // manifest does not describe. + const skillDir = join(pluginsDir, "maker", "skills", "maker"); + await mkdir(skillDir, { recursive: true }); + await writeFile(join(pluginsDir, "maker", "step.plugin.json"), JSON.stringify({ id: "maker" })); + await writeFile( + join(skillDir, "SKILL.md"), + ["---", "name: maker", "description: Make things on request.", "---", "", "# Maker", ""].join("\n"), + ); + + const discovered = await discoverStepPluginResourcePaths({ userDir: pluginsDir }); + const loaded = loadSkillsFromDir({ dir: discovered.skillPaths[0]!, source: "plugin" }); + expect(loaded.diagnostics).toEqual([]); + expect(loaded.skills.map((skill) => skill.name)).toEqual(["maker"]); + expect(loaded.skills[0]?.description).toBe("Make things on request."); + }); + test("installs a manifest and reports MCP declarations without starting a process", async () => { const root = await mkdtemp(join(tmpdir(), "step-plugins-install-")); roots.push(root); @@ -322,6 +560,28 @@ describe("Step plugin marketplace facade", () => { }); }); +/** Write a marketplace checkout with one directory and manifest per declared plugin. */ +async function writeMarketplace( + marketplaceDir: string, + manifest: { name: string; plugins: Array<{ name: string; source: string }> }, +): Promise { + await mkdir(join(marketplaceDir, ".step-plugin"), { recursive: true }); + await writeFile(join(marketplaceDir, ".step-plugin", "marketplace.json"), JSON.stringify(manifest)); + for (const plugin of manifest.plugins) { + await mkdir(join(marketplaceDir, plugin.source), { recursive: true }); + await writeFile( + join(marketplaceDir, plugin.source, "step.plugin.json"), + JSON.stringify({ id: plugin.name, name: plugin.name }), + ); + } +} + +/** A plugin exposing one conventional `skills/` directory. */ +async function writeMarketplacePlaceholder(pluginDir: string, skillName: string): Promise { + await mkdir(join(pluginDir, "skills", skillName), { recursive: true }); + await writeFile(join(pluginDir, "step.plugin.json"), JSON.stringify({ id: pluginDir.split("/").pop() })); +} + async function writePlugin(root: string, directory: string, id: string, name: string): Promise { const pluginDir = join(root, directory); await mkdir(pluginDir, { recursive: true }); diff --git a/packages/tui/src/components/select-list.ts b/packages/tui/src/components/select-list.ts index 3af4cfe3..677847de 100644 --- a/packages/tui/src/components/select-list.ts +++ b/packages/tui/src/components/select-list.ts @@ -69,6 +69,19 @@ export class SelectList implements Component { this.selectedIndex = clamp(this.selectedIndex, 0, Math.max(0, items.length - 1)); } + /** + * Show the empty result state while keeping the current options. + * + * A caller that filters outside this component (fuzzy matching, remote + * search) needs to distinguish "no match" from "no items": setItems with an + * empty array would drop the options it still holds, and setItems with the + * full array would ignore the query. + */ + setEmpty(): void { + this.filteredItems = []; + this.selectedIndex = 0; + } + getSelectedIndex(): number { return this.selectedIndex; } From 0903f1f7721ecabc37478b01410f7e8371ff1ea6 Mon Sep 17 00:00:00 2001 From: xuyunfang Date: Wed, 30 Sep 2026 12:17:41 +0800 Subject: [PATCH 2/3] fix(plugins): reject a path in the root's direct parent as contained `path.relative` reports the root's direct parent as a bare "..", which does not start with ".." + sep, so both containment helpers (`isContained` in plugins.ts and `isInside` in mcp.ts) treated it as inside the root. This is reachable: `listMarketplacePlugins` resolves each entry's `source` from a third-party marketplace manifest, so `"source": ".."` listed the checkout's own parent, and installing it copied that directory into the plugin root. Both helpers now share one `isPathContained` that rejects "..". Reported by @uos1231234 in #204. --- packages/coding-agent/src/step/mcp.ts | 9 ++----- .../src/step/path-containment.test.ts | 20 ++++++++++++++++ .../coding-agent/src/step/path-containment.ts | 14 +++++++++++ packages/coding-agent/src/step/plugins.ts | 24 ++++++++----------- .../coding-agent/test/step-plugins.test.ts | 18 ++++++++++++++ 5 files changed, 64 insertions(+), 21 deletions(-) create mode 100644 packages/coding-agent/src/step/path-containment.test.ts create mode 100644 packages/coding-agent/src/step/path-containment.ts diff --git a/packages/coding-agent/src/step/mcp.ts b/packages/coding-agent/src/step/mcp.ts index 8b12b96e..c2cf54f8 100644 --- a/packages/coding-agent/src/step/mcp.ts +++ b/packages/coding-agent/src/step/mcp.ts @@ -13,6 +13,7 @@ import { readGlobalStepConfig } from "./config-toml.ts"; import { createMcpToolCaller, listAllMcpTools } from "./mcp-client.ts"; import { resolveStepMcpEnvironment } from "./mcp-environment.ts"; import { createStoredMcpOAuthProvider, hasStoredMcpOAuthCredential } from "./mcp-oauth.ts"; +import { isPathContained } from "./path-containment.ts"; import { defaultStepPluginsDir, ensureBuiltinPluginsInstalled, @@ -409,12 +410,6 @@ export function expandHeaderTemplate(template: string): string | undefined { return missing ? undefined : expanded; } -/** True when `candidate` stays inside `root`, so a manifest cannot read elsewhere. */ -function isInside(root: string, candidate: string): boolean { - const relative = path.relative(path.resolve(root), path.resolve(candidate)); - return relative === "" || (!relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); -} - /** * Resolve a manifest's `mcpServers` into a server map. * @@ -432,7 +427,7 @@ async function resolveDeclaredServers( if (isRecord(declared)) return declared; if (typeof declared !== "string" || !declared.trim()) return undefined; const resolved = path.resolve(pluginDir, declared); - if (!isInside(pluginDir, resolved)) return undefined; + if (!isPathContained(pluginDir, resolved)) return undefined; try { const raw = JSON.parse(await readFile(resolved, "utf8")) as unknown; // The file may hold the map directly or wrap it under `mcpServers`, the diff --git a/packages/coding-agent/src/step/path-containment.test.ts b/packages/coding-agent/src/step/path-containment.test.ts new file mode 100644 index 00000000..6b7687e8 --- /dev/null +++ b/packages/coding-agent/src/step/path-containment.test.ts @@ -0,0 +1,20 @@ +import path from "node:path"; +import { expect, test } from "vitest"; +import { isPathContained } from "./path-containment.ts"; + +const root = path.resolve("/tmp/step-root"); + +test.each([ + [".", true], + ["plugins/a", true], + ["..", false], + ["../x", false], + ["../step-root-sibling", false], + ["..hidden", true], +])("%s inside the root: %s", (relative, expected) => { + expect(isPathContained(root, path.resolve(root, relative))).toBe(expected); +}); + +test("an absolute path elsewhere is not contained", () => { + expect(isPathContained(root, path.resolve("/etc"))).toBe(false); +}); diff --git a/packages/coding-agent/src/step/path-containment.ts b/packages/coding-agent/src/step/path-containment.ts new file mode 100644 index 00000000..fe66c2e2 --- /dev/null +++ b/packages/coding-agent/src/step/path-containment.ts @@ -0,0 +1,14 @@ +import path from "node:path"; + +/** + * True when `candidate` is `root` or lies beneath it. + * + * `path.relative` reports the root's direct parent as a bare `".."`, which does + * not start with `".." + sep`; testing only the prefix therefore let a manifest + * value of `".."` through as if it were contained. + */ +export function isPathContained(root: string, candidate: string): boolean { + const relative = path.relative(path.resolve(root), path.resolve(candidate)); + if (relative === "") return true; + return relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative); +} diff --git a/packages/coding-agent/src/step/plugins.ts b/packages/coding-agent/src/step/plugins.ts index 1bd996d6..10024ae2 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -18,6 +18,7 @@ import { BorderedLoader } from "../components/bordered-loader.ts"; import type { ExtensionAPI, ExtensionCommandContext, ExtensionFactory } from "../core/extensions/types.ts"; import { resolveStepConfigDir } from "./environment.ts"; import { resolveStepMcpEnvironment, STEP_LOGIN_SUPPLIED_ENV } from "./mcp-environment.ts"; +import { isPathContained } from "./path-containment.ts"; import { resolveStepStorageRoot } from "./storage-root.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "./telemetry.ts"; @@ -197,11 +198,6 @@ function isSafeName(value: string): boolean { return SAFE_NAME.test(value) && value !== "." && value !== ".."; } -function isContained(root: string, candidate: string): boolean { - const relative = path.relative(path.resolve(root), path.resolve(candidate)); - return relative === "" || (!relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); -} - function normalizeRelativePath(value: unknown): string | undefined { if (typeof value !== "string" || value.trim() === "") return undefined; const normalized = value.trim(); @@ -468,7 +464,7 @@ export async function listMarketplacePlugins( ? value.source.trim() : path.join("plugins", name); const sourcePath = path.resolve(marketplaceDir, relative); - if (!isContained(marketplaceDir, sourcePath)) { + if (!isPathContained(marketplaceDir, sourcePath)) { warnings.push( `Marketplace '${marketplaceName}' entry '${name}' has a source outside the checkout; skipped.`, ); @@ -514,7 +510,7 @@ export async function installMarketplacePlugin( ): Promise<{ installedPath: string; warnings: string[]; diagnostics: StepPluginDiagnostics }> { if (!isSafeName(entry.name)) throw new Error(`'${entry.name}' is not a safe plugin name.`); const target = path.resolve(pluginsDir, entry.name); - if (!isContained(pluginsDir, target) || path.basename(target) !== entry.name) + if (!isPathContained(pluginsDir, target) || path.basename(target) !== entry.name) throw new Error(`'${entry.name}' is not an installed plugin name.`); if (await pathExists(target)) throw new Error(`Plugin '${entry.name}' is already installed at ${target}. Remove it first.`); @@ -615,7 +611,7 @@ export async function uninstallPlugin(pluginsDir: string, name: string): Promise if (!isSafeName(name.trim())) throw new Error(`'${name}' is not an installed plugin name.`); const root = path.resolve(pluginsDir); const target = path.resolve(root, name.trim()); - if (!isContained(root, target) || path.dirname(target) !== root) + if (!isPathContained(root, target) || path.dirname(target) !== root) throw new Error(`'${name}' is not an installed plugin name.`); const stat = await fs.lstat(target).catch(() => undefined); if (!stat?.isDirectory()) throw new Error(`Plugin '${name}' is not installed in ${root}.`); @@ -635,7 +631,7 @@ export async function diagnoseStepPlugin( const mcpServers: string[] = []; if (typeof read.manifest.mcpServers === "string") { const declarationPath = path.resolve(pluginDir, read.manifest.mcpServers); - if (!isContained(pluginDir, declarationPath)) { + if (!isPathContained(pluginDir, declarationPath)) { warnings.push(`MCP declaration ${read.manifest.mcpServers} escapes ${pluginDir}.`); } else if (!(await pathExists(declarationPath))) { warnings.push(`MCP declaration ${read.manifest.mcpServers} is missing from ${pluginDir}.`); @@ -758,7 +754,7 @@ export async function discoverStepPluginResourcePaths(input: { declared !== undefined ? declared : (await pathExists(path.join(pluginDir, key))) ? [key] : []; for (const relative of candidates) { const resolved = path.resolve(pluginDir, relative); - if (!isContained(pluginDir, resolved)) { + if (!isPathContained(pluginDir, resolved)) { warnings.push( `Plugin '${manifest.id}' declares ${key} '${relative}', which is outside the plugin; skipped.`, ); @@ -848,7 +844,7 @@ export async function ensureBuiltinMarketplace( await fs.rm(target, { recursive: true, force: true }); for (const [relative, contents] of Object.entries(BUILTIN_MARKETPLACE_FILES)) { const resolved = path.resolve(target, relative); - if (!isContained(target, resolved)) + if (!isPathContained(target, resolved)) return { path: target, warnings: [`Built-in marketplace entry ${relative} escapes its directory.`] }; await fs.mkdir(path.dirname(resolved), { recursive: true }); await fs.writeFile(resolved, contents, "utf8"); @@ -1039,8 +1035,8 @@ export async function addMarketplaceSource(input: { const resolvedTarget = path.resolve(target); if ( resolvedSource === resolvedTarget || - isContained(resolvedSource, resolvedTarget) || - isContained(resolvedTarget, resolvedSource) + isPathContained(resolvedSource, resolvedTarget) || + isPathContained(resolvedTarget, resolvedSource) ) { return { warnings: [ @@ -1133,7 +1129,7 @@ export async function removeMarketplaceSource(input: { return { warnings: [`Marketplace '${name}' cannot be removed.`] }; const root = input.marketplacesDir ?? defaultStepMarketplacesDir(); const target = path.resolve(root, name); - if (!isContained(root, target) || path.dirname(target) !== path.resolve(root)) + if (!isPathContained(root, target) || path.dirname(target) !== path.resolve(root)) return { warnings: [`Marketplace '${name}' is not a valid name.`] }; if (!(await pathExists(target))) return { warnings: [`No marketplace named '${name}' is configured.`] }; const isGitCheckout = await pathExists(path.join(target, ".git")); diff --git a/packages/coding-agent/test/step-plugins.test.ts b/packages/coding-agent/test/step-plugins.test.ts index 96e76b24..1f20d411 100644 --- a/packages/coding-agent/test/step-plugins.test.ts +++ b/packages/coding-agent/test/step-plugins.test.ts @@ -65,6 +65,24 @@ describe("Step plugin marketplace facade", () => { expect(result.warnings[0]).toEqual(expect.stringContaining("1 url")); }); + test("skips a marketplace entry whose source is the checkout's own parent", async () => { + const root = await mkdtemp(join(tmpdir(), "step-plugins-parent-")); + roots.push(root); + const checkout = join(root, "marketplaces", "hostile"); + await mkdir(join(checkout, ".step-plugin"), { recursive: true }); + await writeFile( + join(checkout, ".step-plugin", "marketplace.json"), + JSON.stringify({ name: "hostile", plugins: [{ name: "parent", source: ".." }] }), + ); + // Make the parent look like a valid plugin, so only the containment check + // stands between this entry and a copy of the whole directory. + await writeFile(join(root, "marketplaces", "step.plugin.json"), JSON.stringify({ id: "parent" })); + + const result = await listMarketplacePlugins([join(root, "marketplaces")]); + expect(result.entries).toEqual([]); + expect(result.warnings).toEqual([expect.stringContaining("has a source outside the checkout")]); + }); + test("materializes built-ins under the supplied Step marketplace root", async () => { const root = await mkdtemp(join(tmpdir(), "step-plugins-builtin-")); roots.push(root); From be4d5f4d8106cabab0450b80f804d14c1812ce42 Mon Sep 17 00:00:00 2001 From: xuyunfang Date: Wed, 30 Sep 2026 12:17:51 +0800 Subject: [PATCH 3/3] fix(step): run a plugin's stdio MCP server from the plugin root An inline stdio server in a plugin manifest was spawned with no working directory, so it inherited the directory `step` was launched from. A manifest like `{"command":"node","args":["server/index.mjs"]}` then could not find its own script, and `"cwd": "."` did not help because it was never anchored either. The string `.mcp.json` form already resolved against the plugin root; the inline form did not. A plugin stdio server now runs from the plugin root. A relative `cwd` is read against the plugin root, one that escapes the plugin falls back to the root, and an absolute `cwd` is kept. `config.toml` servers are unchanged: there a relative `cwd` still means the process directory. Reported by @uos1231234 in #204. --- .../coding-agent/src/step/mcp-startup.test.ts | 62 +++++++++++++++++++ packages/coding-agent/src/step/mcp.ts | 24 ++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/step/mcp-startup.test.ts b/packages/coding-agent/src/step/mcp-startup.test.ts index 8e1361dd..9aca5993 100644 --- a/packages/coding-agent/src/step/mcp-startup.test.ts +++ b/packages/coding-agent/src/step/mcp-startup.test.ts @@ -320,6 +320,68 @@ test("expands header templates and reports a variable with no fallback", () => { delete process.env.STEP_TEST_PRESENT; }); +test("a plugin stdio server runs from the plugin root unless it names an absolute cwd", async () => { + const dir = "/mock-plugins/local"; + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "local", + mcpServers: { + bare: { command: "node", args: ["server/index.mjs"] }, + dot: { command: "node", cwd: "." }, + nested: { command: "node", cwd: "server" }, + escapes: { command: "node", cwd: ".." }, + absolute: { command: "node", cwd: "/opt/elsewhere" }, + remote: { url: "https://example.test/mcp" }, + }, + }); + const discovered = await discoverStepMcpServers(process.cwd(), false); + const cwdOf = (server: string) => discovered.find((entry) => entry.name === `local__${server}`)?.declaration.cwd; + expect(cwdOf("bare")).toBe(dir); + expect(cwdOf("dot")).toBe(dir); + expect(cwdOf("nested")).toBe(join(dir, "server")); + expect(cwdOf("escapes")).toBe(dir); + expect(cwdOf("absolute")).toBe("/opt/elsewhere"); + // A remote server is never spawned, so it is given no working directory. + expect(cwdOf("remote")).toBeUndefined(); +}); + +test("a plugin stdio server with a relative script starts when step runs elsewhere", async () => { + const root = await mkdtemp(join(await realpath(tmpdir()), "mcp-plugin-cwd-")); + cleanups.push(async () => rm(root, { recursive: true, force: true })); + const dir = join(root, "plugin"); + await mkdir(join(dir, "server"), { recursive: true }); + // A minimal newline-delimited JSON-RPC server: enough for the handshake and one tool. + await writeFile( + join(dir, "server", "index.mjs"), + `import { createInterface } from "node:readline"; +const send = (message) => process.stdout.write(JSON.stringify({ jsonrpc: "2.0", ...message }) + "\\n"); +createInterface({ input: process.stdin }).on("line", (line) => { + const { id, method, params } = JSON.parse(line); + if (id === undefined) return; + if (method === "initialize") { + send({ id, result: { protocolVersion: params.protocolVersion, capabilities: { tools: {} }, serverInfo: { name: "probe", version: "1" } } }); + } else if (method === "tools/list") { + send({ id, result: { tools: [{ name: "where", inputSchema: { type: "object" } }] } }); + } else { + send({ id, error: { code: -32601, message: method } }); + } +}); +`, + ); + pluginMocks.dirs = [dir]; + pluginMocks.manifests.set(dir, { + id: "local", + mcpServers: { probe: { command: process.execPath, args: ["server/index.mjs"] } }, + }); + // The test process is not in the plugin directory, which is the whole point. + expect(process.cwd()).not.toBe(dir); + + const harness = await setup("tui"); + await harness.start(); + await vi.waitFor(() => expect(harness.extension.tools.size).toBe(1)); + expect(getStepMcpStatuses()).toContainEqual({ name: "local__probe", status: "connected", toolCount: 1 }); +}); + test("resolves a plugin whose mcpServers points at a sibling .mcp.json", async () => { const server = await slowServer(1); // The string form names a real file, so this needs a real directory on disk. diff --git a/packages/coding-agent/src/step/mcp.ts b/packages/coding-agent/src/step/mcp.ts index c2cf54f8..db6b9854 100644 --- a/packages/coding-agent/src/step/mcp.ts +++ b/packages/coding-agent/src/step/mcp.ts @@ -365,7 +365,10 @@ export async function discoverStepMcpServers(cwd: string, projectTrusted: boolea seen.add(name); const discovered: DiscoveredServer = { name, - declaration: applyPluginHeaderAliases(normalizeDeclaration(value), value), + declaration: anchorPluginServerCwd( + pluginDir, + applyPluginHeaderAliases(normalizeDeclaration(value), value), + ), }; if (parsed.manifest.provision) discovered.provision = parsed.manifest.provision; result.push(discovered); @@ -375,6 +378,25 @@ export async function discoverStepMcpServers(cwd: string, projectTrusted: boolea return result; } +/** + * Run a plugin's stdio server from the plugin root. + * + * Without this the child inherited the directory `step` was launched from, so a + * manifest such as `{"command":"node","args":["server/index.mjs"]}` could not + * find its own script. A relative `cwd` is read against the plugin root, and one + * that escapes the plugin falls back to the root; an absolute `cwd` is the + * author's explicit choice and is kept. This stays out of `normalizeDeclaration` + * because `config.toml` shares it, and there a relative `cwd` keeps meaning the + * process directory. + */ +function anchorPluginServerCwd(pluginDir: string, declaration: ServerDeclaration): ServerDeclaration { + if (typeof declaration.command !== "string") return declaration; + const declared = declaration.cwd ?? ""; + if (path.isAbsolute(declared)) return declaration; + const resolved = path.resolve(pluginDir, declared); + return { ...declaration, cwd: isPathContained(pluginDir, resolved) ? resolved : path.resolve(pluginDir) }; +} + /** True when a declaration names either transport: a stdio command or a url. */ function hasTransport(value: Record): boolean { const command = typeof value.command === "string" ? value.command.trim() : "";