Replies: 1 comment
|
一条你今天就能加的护栏(不等官方)既然你是从自己的环境构建那张 env map 的宿主,在交给 harness 之前把未定义项滤掉就能完全避开这条: const env = Object.fromEntries(
Object.entries(rawEnv).filter(([, v]) => v !== undefined)
) as Record<string, string>这里有个语义选择要你自己定:一个尚未铸造的 per-user key,你是希望
两者对 MCP server 的行为可能不同——有些 server 会把"变量存在但为空"当成显式的空配置,有些只检查存在性。你那个"首次登录才铸造"的场景大概率要前者。 对你说的"每个新用户都恰好踩一次、而所有开发机都复现不了"——这条护栏正好治那个:它不依赖变量何时被铸造。 但我同意真正该修的是另外两件
第 2 条我觉得单独拎出来提最有价值:它不是 MCP 特有的,是整个配置层的诊断质量问题。 边界我们不改 利益相关:我维护 pi2dsh,跟这条回复无关。 |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
A small schema detail with a large blast radius for anything embedding the harness. Present in 0.1.0-rc.8.
What happens
packages/mcp/mcp-client/src/index.tsdeclares:A host that builds that map from its own environment — the ordinary case — produces
undefinedfor a variable that is not set yet.undefinedis not aString, so config validation rejects the entry.The consequence is out of all proportion to the cause. Validation runs before anything mounts, and the mount is transactional, so the result is not a disabled MCP server. It is a dead harness: no listener on the port, so an embedded browser view shows a refused connection.
Measured on a clean Windows 11 VM, running the vendored harness directly:
plugin tree failed to load— 2 of 2 runsThe painful version of this is a variable that is legitimately absent for the first few minutes of a product's life — a per-user key minted at first sign-in, say. Every new user hits it exactly once, and no developer machine reproduces it, because they all already have the value.
The diagnostic points away from the cause
Configis az.union, and the union resolver collects each branch'sValidationErrorinto a local array, discards it, and throws only:JSON.stringifyomits keys whose value isundefined. So the one key that caused the failure is the only key missing from the message. The branch error that knew the path ($.env.FOO missing required value) is thrown away in favour of the one renderer that structurally cannot show it.Suggested fix
Treat an unset value as "do not forward this variable", which is the natural reading of an env map:
Schemastery then accepts the entry and drops the unset key itself, so nothing downstream needs a filter and the child environment never sees it. A present-but-wrong-typed value is still rejected, which is correct. This also matches
scrubbedParentEnvinpackages/subprocess/subprocess/src/index.ts, which already skipsvalue !== undefinedwhen building a child environment — so the same rule would apply at both ends.headerson the streamable-http transport has the identical shape and the same argument applies.Separately, appending the collected branch messages to the union error would make every config failure name its path, not just this one. They are already gathered; only the throw discards them.
A related expectation the API sets
failOnStartupError: z.boolean().default(false)reasonably reads as "a broken entry degrades rather than taking the product down", and that reading is why hosts do not defend against the above. It cannot hold for a validation failure: the flag governs an entry that fails to START, while a config error is raised before that entry mounts and is unconditionally fatal to the tree.That may well be the right design — an all-or-nothing mount is defensible, and the code is explicit that it is transactional. The narrower problem is that the flag's name and its default promise something they cannot deliver. Documenting that boundary would be the cheapest honest fix; letting a host mark a config layer non-essential would be the more useful one.
We have applied the schema change locally and it behaves as described. Happy to share the patch — noting we cannot open a pull request, per CONTRIBUTING.
All reactions