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..9aca5993 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,304 @@ 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("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. + 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..db6b9854 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"; @@ -11,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, @@ -27,7 +30,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 +275,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 +352,23 @@ 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: anchorPluginServerCwd( + pluginDir, + applyPluginHeaderAliases(normalizeDeclaration(value), value), + ), }; if (parsed.manifest.provision) discovered.provision = parsed.manifest.provision; result.push(discovered); @@ -307,6 +378,117 @@ 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() : ""; + 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; +} + +/** + * 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 (!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 + // 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 +506,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 +648,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 +713,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/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 4ed67f0b..10024ae2 100644 --- a/packages/coding-agent/src/step/plugins.ts +++ b/packages/coding-agent/src/step/plugins.ts @@ -14,9 +14,11 @@ 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 { isPathContained } from "./path-containment.ts"; import { resolveStepStorageRoot } from "./storage-root.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "./telemetry.ts"; @@ -196,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(); @@ -404,11 +401,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 +447,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() @@ -453,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.`, ); @@ -465,6 +476,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, @@ -496,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.`); @@ -597,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}.`); @@ -617,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}.`); @@ -696,6 +710,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 (!isPathContained(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[] }> { @@ -763,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"); @@ -907,7 +988,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(); @@ -945,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: [ @@ -960,16 +1050,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 +1076,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; @@ -994,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")); @@ -1034,6 +1169,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 +1227,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 +1324,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 +1368,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 +1411,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 +1459,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 +1541,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 +1703,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..1f20d411 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"; @@ -62,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); @@ -77,6 +98,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 +578,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; }