Conversation
Declarative plugins could contribute MCP servers, but nothing carried
their skills or commands into a session: the resource loader reads
agent/project resource directories that plugins never write to. A
marketplace plugin installed successfully and then had no effect.
Bridge the two with the existing `resources_discover` channel, which
already supports skill and prompt paths and re-merges them on reload.
Project plugin roots are gated on project trust, matching MCP discovery,
so an untrusted checkout cannot inject skill instructions.
Alongside that, MCP discovery dropped several plugin shapes silently:
- A remote server declaring `url` with no `command` was skipped, even
though the global-config path and the normalizer both accept a url.
- `mcpServers` naming a sibling `.mcp.json` (the Claude layout) was read
as an unsupported string, so every such plugin lost its servers.
- The `headers` spelling was not accepted, and its `${VAR:-fallback}`
values were never expanded. Expansion now applies to plugin manifests
only: a `config.toml` `http_headers` map stays literal.
- `step mcp login` only read `config.toml`, so it could not resolve the
`<pluginId>__<serverName>` name its own failure message printed. It now
accepts the bare and published spellings, and stores credentials under
the resolved name so login and the connection agree on the key.
The marketplace UI also grew a searchable plugin list, in-place clone
progress (with a cancel path), and lands on the new marketplace's plugins
after a successful add.
|
Thanks for this — closing Q2 of #196 is exactly what we asked for, and routing One thing this doesn't reach, which we'd like to understand whether it's The path today
// mcp.ts:365
const discovered: DiscoveredServer = {
name,
declaration: applyPluginHeaderAliases(normalizeDeclaration(value), value),
};
if (parsed.manifest.provision) discovered.provision = parsed.manifest.provision;
// mcp.ts:480
if (typeof value.cwd === "string" && value.cwd.trim()) declaration.cwd = value.cwd.trim();and the transport reads its working directory from exactly that field: // mcp.ts:530
cwd: input.declaration.cwd,So a manifest without Measured, end to endPlugin installed at
The middle row is the one worth calling out: the obvious author-side workaround, All three runs exited 0, with nothing about the server on stdout, stderr, or ScopeAll six MCP-form plugins in the "mcpServers": { "<id>": { "command": "node", "args": ["server/index.mjs"] } }Relative, and no This predates the branch: If it is worth closingA string // mcp.ts:434
const resolved = path.resolve(pluginDir, declared);
if (!isInside(pluginDir, resolved)) return undefined;So the inline form is the odd one out, and closing that gap is a small /**
* Anchor a stdio server's working directory to the plugin root.
*
* `resolveDeclaredServers` already resolves a string `mcpServers` path against
* the plugin directory; an inline declaration has no path to resolve, so it
* ends up with no anchor at all. A manifest may still name an explicit `cwd` —
* an absolute one is the author's own choice, a relative one is read against
* the plugin root rather than the process directory, and one that escapes the
* plugin falls back to the root instead of reaching the spawn verbatim.
*
* Deliberately kept out of `normalizeDeclaration`: that function is shared with
* the global `config.toml` `mcp_servers` path (`mcp.ts:306`), where a relative
* `cwd` means the process directory, not a plugin.
*/
function resolvePluginServerCwd(pluginDir: string, declaration: ServerDeclaration): ServerDeclaration {
// Only a stdio server is spawned in a working directory.
if (typeof declaration.command !== "string") return declaration;
const declared = typeof declaration.cwd === "string" ? declaration.cwd.trim() : "";
if (path.isAbsolute(declared)) return declaration;
const resolved = declared ? path.resolve(pluginDir, declared) : pluginDir;
return { ...declaration, cwd: isInside(pluginDir, resolved) ? resolved : pluginDir };
}and at the construction site: declaration: resolvePluginServerCwd(
pluginDir,
applyPluginHeaderAliases(normalizeDeclaration(value), value),
),Two details this shape avoids: it runs after Not a merge blocker — the main thing we'd like to know is whether the current |
|
@uos1231234 Two corrections to my comment above, both found by auditing my own change afterwards. 1. "fails completely silent" was too strong. I verified the spawn path more closely: — while 2. It reads: const relative = path.relative(path.resolve(root), path.resolve(candidate));
return relative === "" || (!relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative));When I first wrote that no caller could reach it, since So it is reachable, not latent. That is a one-line fix ( Happy to split that into a separate PR if you would rather shape it on its own — I have no strong view on whether the two belong in one branch, and the |
A plugin declares its server beside its own manifest, so a relative entry in `args` is relative to the plugin directory. Nothing told the transport that: `normalizeDeclaration` only sets `cwd` when the manifest names one, and `StdioClientTransport` reads its working directory from exactly that field, so an omitted `cwd` left the child inheriting the `step` process's own working directory. Measured end to end against a real `step` process, with a probe server that writes a file as its first statement: a manifest with no `cwd` never starts the server, and neither does one naming `"."`, while an absolute path to the plugin root does. All three runs exit 0 with nothing about the server on stdout, stderr, or the `--mode json` event stream. This anchors an inline stdio declaration to the plugin root, which is the anchor stepfun-ai#204's `resolveDeclaredServers` applies to a string `mcpServers` path. A `cwd` that escapes the plugin is refused rather than rewritten, so the server is dropped instead of starting somewhere the manifest did not name. The global `config.toml` `mcp_servers` path shares `normalizeDeclaration` and is left alone, since a relative `cwd` there means the process directory. Overlaps stepfun-ai#204 at the same construction site; if that lands first, the wrapper belongs around its `applyPluginHeaderAliases` result, and must run after that call, which returns the same object on one branch and a new one on the other.
A plugin declares its server beside its own manifest, so a relative entry in `args` is relative to the plugin directory. Nothing told the transport that: `normalizeDeclaration` only sets `cwd` when the manifest names one, and `StdioClientTransport` reads its working directory from exactly that field, so an omitted `cwd` left the child inheriting the `step` process''s own working directory. Measured end to end against a real `step` process, with a probe server that writes a file as its first statement: a manifest with no `cwd` never starts the server, and neither does one naming `.`, while an absolute path to the plugin root does. All three runs exit 0 with nothing about the server on stdout, stderr, or the `--mode json` event stream. This anchors an inline stdio declaration to the plugin root, which is the anchor stepfun-ai#204''s `resolveDeclaredServers` applies to a string `mcpServers` path. A `cwd` that escapes the plugin is refused rather than rewritten, so the server is dropped instead of starting somewhere the manifest did not name. It leans on `isContained` from ./plugins.ts, hardened in the previous commit: a manifest `cwd` arrives unnormalised, so this is the first caller to depend on that check rejecting an escape rather than on an upstream filter. The global `config.toml` `mcp_servers` path shares `normalizeDeclaration` and is left alone, since a relative `cwd` there means the process directory. Overlaps stepfun-ai#204 at the same construction site; if that lands first, the wrapper belongs around its `applyPluginHeaderAliases` result, and must run after that call, which returns the same object on one branch and a new one on the other.
A plugin declares its server beside its own manifest, so a relative entry in `args` is relative to the plugin directory. Nothing told the transport that: `normalizeDeclaration` only sets `cwd` when the manifest names one, and `StdioClientTransport` reads its working directory from exactly that field, so an omitted `cwd` left the child inheriting the `step` process's own working directory. Measured end to end against a real `step` process, with a probe server that writes a file as its first statement: a manifest with no `cwd` never starts the server, and neither does one naming `"."`, while an absolute path to the plugin root does. All three runs exit 0 with nothing about the server on stdout, stderr, or the `--mode json` event stream. This anchors an inline stdio declaration to the plugin root, which is the anchor stepfun-ai#204's `resolveDeclaredServers` applies to a string `mcpServers` path. A `cwd` that escapes the plugin is refused rather than rewritten, so the server is dropped instead of starting somewhere the manifest did not name. The global `config.toml` `mcp_servers` path shares `normalizeDeclaration` and is left alone, since a relative `cwd` there means the process directory. The two commits are one branch on purpose. A manifest `cwd` arrives unnormalised — `normalizeDeclaration` only trims it — so this is the first caller that exercises `isContained`'s escape path rather than relying on an upstream filter in front of it. Anchoring on the version hardened in the previous commit is what makes the refusal correct; the two halves are only sound together. Overlaps stepfun-ai#204 at the same construction site; the call site carries a comment with the exact form to use if it lands first.
|
@MelodyVAR @Uking-xxx — the change is ready but this repository restricts pull requests to collaborators, so both the UI and the API refuse it ( Reviewable here (two commits, second depends on first): main...uos1231234:fix/plugin-mcp-server-cwd 1.
That is reachable — 2. The working-directory issue from my previous comment. It leans on the hardened The construction site is one line below the one #204 rewrites, and carries a comment with the exact form to use if #204 lands first, including why the wrapper has to sit outside To apply it, if that is easier than reviewing the branch: curl -L -o cwd.patch \
https://github.com/stepfun-ai/Step-Code/compare/main...uos1231234:fix/plugin-mcp-server-cwd.patch
git am --3way cwd.patchI verified that exact sequence against a clean Validation — Happy to have any of it reworked, rebased, or split — and if you would rather open the permission so this can go through as a proper PR, that works too. Your call. |
2 participants
Why
Two things a user does not expect to be broken:
Both trace to the same gap: a plugin's contributions were discovered only partially.
Plugin skills and commands were never loaded
Nothing carried a plugin's
skills/commandsinto a session. MCP is the onecontribution that reads the plugin root directly; the resource loader reads
~/.stepcode/agent/{skills,prompts}and<cwd>/.stepcode/..., which pluginsnever write to. The two trees had no bridge, so
plugins.tsparsed and validatedthose fields and then nothing consumed them.
This uses the existing
resources_discoverchannel, which already accepts skilland prompt paths and already re-merges them on reload — so a plugin installed
during a session takes effect on the next resource reload rather than requiring a
restart. Project plugin roots are gated on project trust, matching MCP discovery:
an untrusted checkout must not inject skill instructions or prompt templates.
Four silent drops on the MCP path
urlwith nocommandwas skipped. The globalconfig path and
normalizeDeclarationboth accept a url, so the same serverworked from
config.tomlbut not from a plugin.mcpServersnaming a sibling.mcp.json— the Claude plugin layout — was readas an unsupported string.
readStepPluginManifestfills that field in onpurpose for exactly this shape, and discovery then discarded it.
headersspelling was not accepted, and its${VAR:-fallback}values werenever expanded. This is the context7 plugin's shape. Expansion is scoped to
plugin manifests: a
config.tomlhttp_headersmap stays literal, so noexisting configuration changes meaning.
step mcp loginread onlyconfig.tomland so could not resolve the<pluginId>__<serverName>name its own failure message printed. It now acceptsthe bare and published spellings (
step mcp login context7resolvescontext7__context7), refuses an ambiguous bare name rather than picking one,and stores credentials under the resolved name — storing the user's input
would write a key the connection never reads.
Marketplace UI
root and a project root is scanned alongside the global one, so each built-in
plugin was listed once per root.
All Plugins/Marketplaces, and drop the built-in rowfrom the source list (it can be neither updated nor removed).
handing the terminal back to the editor for the length of the clone. The clone
is cancellable, and a successful add lands on the new marketplace's plugins.
name@marketplaceforinstall, and name the marketplaces that docarry a plugin when the qualifier does not match.
Verification
packages/coding-agent: 3713 passed, 0 failedapps/cli: 629 passed / 84 filespackages/tui: 953 passed, 0 failedpnpm run check(biome, typecheck, all guard scripts): passessystem-prompt listing), the trust gate, the
.mcp.jsonindirection, headerinterpolation, and the credential-key agreement between login and connect.
./test.shwas not run to completion: it bootstrapspnpm@9.15.9into anisolated home whose npm userconfig is empty, so it reaches for
registry.npmjs.org, which is unreachable from this network (the environment'snpm registry is an internal mirror). The suites above are the same three the
script runs. Worth fixing separately.