fix: judge plugin requiresEnv against the runtime environment - #189
Merged
Merged
Conversation
`diagnoseStepPlugin` checked `process.env` for the variables a plugin's provision block declares, while the server it describes is spawned with `resolveStepMcpEnvironment`, which falls back to the Step login credential for STEPFUN_API_KEY. Every logged-in user therefore saw the built-in StepPage plugin warn about a missing STEPFUN_API_KEY that the runtime supplies on its own, and the warning told them a credential "can supply it at runtime" rather than what to do. Move `resolveStepMcpEnvironment` into `step/mcp-environment.ts` so the doctor and the runtime share one resolution — `mcp.ts` imports `plugins.ts`, so plugins cannot import the resolver from `mcp.ts` without a cycle — inject env/authPath into `diagnoseStepPlugin`, and reword the remaining warning to point at `/login`.
Review follow-up on the previous commit, which compared `requiresEnv` against the login fallback but still passed no declared environment. `connectStepMcpServer` layers three sources: the process environment, the server's own declared `env`, and the Step login credential. The doctor consulted only the first and third, so a plugin that carried its key in `mcpServers.<name>.env` was still reported as missing it. Match the servers whose command is the provisioned one and judge the requirement against each of their resolved environments. Only an inline `mcpServers` record can start a server, since discovery skips a string declaration path, so that is the only shape consulted. A Step login also only ever supplies its own credential, so `/login` is no longer offered for an unrelated variable; those are pointed at exporting it or declaring it in the manifest. `STEP_LOGIN_SUPPLIED_ENV` keeps that list next to the resolver that honours it rather than hardcoded in the message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
/pluginreported the pre-installed StepPage plugin as missingSTEPFUN_API_KEYfor every logged-in user, even though the plugin works fine.Two code paths disagreed about what "has a credential" means:
resolveStepMcpEnvironment) spawns the server withprocess.env→ manifestdeclaredenv → Step login credential fallback forSTEPFUN_API_KEY.diagnoseStepPlugin) checked the plugin'sprovision.requiresEnvagainst bareprocess.envonly.So a user who had run
/login— the normal case — was told a variable was missing that the runtime supplies on its own fromauth.json. The warning also only described the situation ("a Step login credential can supply it at runtime") instead of saying what to do about it.requiresEnvgates no behaviour, so this was pure noise in the Installed list.Change
resolveStepMcpEnvironmentintostep/mcp-environment.tsso the doctor and the runtime share one resolution. It needs its own module:mcp.tsimportsplugins.ts, soplugins.tscannot import the resolver frommcp.tswithout closing a module cycle.mcp.tsre-exports it, so existing consumers are unchanged.diagnoseStepPlugintakes{ env?, authPath? }and judgesrequiresEnvagainst the resolved runtime environment./login, so it only appears when neither a shell value nor a credential exists — and then says what to do.Tests
requiresEnvhad no coverage. Added both directions totest/step-plugins.test.ts:auth.json+ empty shell env → no warning/loginnpm run checkpasses (biome, layer-direction, public-boundary, tsgo);step-plugins.test.tsandmcp.test.tspass.