Skip to content

[finding] MCPServerPlugin's own docblock still teaches the deprecated stdio trigger — plugin.ts:112-122 disagrees with plugin.ts:234-239 twelve lines below it #14473

New issue
New issue
Closed
Closed
[finding] MCPServerPlugin's own docblock still teaches the deprecated stdio trigger — plugin.ts:112-122 disagrees with plugin.ts:234-239 twelve lines below it#14473
Labels
domain:cliRetired 2026-10-22, alias of domain:server (cli surface); in-flight cards keep it until they landpriority:p3

Description

@os-litant
os-litant
opened on Sep 2, 2026

Out-of-scope by-product of the skills optimization flight on #14305. It is the source of a defect that already shipped once: the flight's MCP section was written from this docblock and inherited its error, caught in contract review round 1 and fixed in PR #14463 at 5dabfd2a. Filed unassigned. A code comment disagreeing with its own code — no spec .describe() twin is owed.

The drift

packages/mcp/src/plugin.ts:112-122, the class docblock:

 * 2. **start** — … Starts the long-lived transport (stdio) only when
 *    `autoStart` is enabled or `OS_MCP_SERVER_ENABLED` is explicitly `true` — …
 *
 * Environment Variables:
 * - `OS_MCP_SERVER_ENABLED` — HTTP surface default-on; `false` disables it,
 *   explicit `true` additionally auto-starts the stdio transport

packages/mcp/src/plugin.ts:226-239, the code it documents:

    // ── stdio auto-start decision (opt-in, its OWN switch) ──
    // … it stays opt-in via a SEPARATE switch
    // (`OS_MCP_STDIO_ENABLED` / the `autoStart` option), never the HTTP var.
    const stdio = resolveMcpStdioAutoStart();
    const shouldStart = this.options.autoStart || stdio.enabled;
    if (stdio.viaDeprecatedAlias && !this.options.autoStart) {
      ctx.logger.warn(
        '[MCP] Starting the stdio transport via OS_MCP_SERVER_ENABLED=true is DEPRECATED — …
         Use OS_MCP_STDIO_ENABLED=true (or the plugin `autoStart` option)…',
      );
    }

resolveMcpStdioAutoStart() (packages/types/src/env.ts:333-345) reads
OS_MCP_STDIO_ENABLED first and returns it clean; OS_MCP_SERVER_ENABLED=true falls through
to the legacy branch and returns viaDeprecatedAlias: true. So the docblock names only the
deprecated trigger, never the canonical one, and presents the deprecated behaviour as the
design. The env.ts docblock (:307-328) has the split right — this one was not updated with
it.

Why it is worth a card rather than a drive-by

The blast radius is measured, not hypothetical. This docblock is what a reader (human or AI)
lands on from MCPServerPlugin, and it is demonstrably what the published
skills/objectstack-ai package was written from: an author following it sets
OS_MCP_SERVER_ENABLED=true, gets a working transport plus a deprecation warning at every
boot, and has no way from this file to learn the right spelling. One catch happened to be in
review; the next may not be.

Suggested fix

Four lines of comment in packages/mcp/src/plugin.ts:

  • Step 2: "…only when autoStart is enabled or OS_MCP_STDIO_ENABLED is truthy".
  • Environment Variables: keep OS_MCP_SERVER_ENABLED as the HTTP-surface gate only; add
    OS_MCP_STDIO_ENABLED as the stdio switch; mark the legacy OS_MCP_SERVER_ENABLED=true
    stdio trigger deprecated, matching the warning string 100 lines below.

Worth checking whether the same pre-split wording survives in packages/mcp/README.md or the
docs tree; this audit only measured the plugin docblock. Related, closed, not a duplicate:
#9579 (packages/mcp/README.md documenting runtime methods that do not exist) is the same
defect class on a neighbouring file, and its close-out did not cover this span.

Dedupe: one targeted search_issues over this repo (repo-scoped REST answers 403 for this
seat), returning a non-empty result set, so the session's search is not in the silent-zero
mode. Nothing open covers this.

Activity

  1. os-litant
    added
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Sep 2, 2026
  2. huangyiirene
    added
    pm:queueReady for the PM dispatch loop
    domain:cliRetired 2026-10-22, alias of domain:server (cli surface); in-flight cards keep it until they land
    priority:p3
    and removed
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Sep 2, 2026

  3. huangyiirene commented on Sep 2, 2026

    @huangyiirene
    huangyiirene
    on Sep 2, 2026 – with Claude
    Collaborator

    Triage — graded, finding cleared. Verified at origin/main ed44512.

    Confirmed, line for line

    • packages/mcp/src/plugin.ts:112-118 — step 2 says the transport starts "only when autoStart is enabled or OS_MCP_SERVER_ENABLED is explicitly true", and the Environment Variables block says explicit true "additionally auto-starts the stdio transport". Neither names OS_MCP_STDIO_ENABLED at all.
    • packages/mcp/src/plugin.ts:226-239 — the code reads resolveMcpStdioAutoStart() and warns that OS_MCP_SERVER_ENABLED=true is DEPRECATED, telling the operator to use OS_MCP_STDIO_ENABLED.
    • packages/types/src/env.ts:333-345 — canonical switch first, legacy OS_MCP_SERVER_ENABLED === 'true' second with viaDeprecatedAlias: true. The env.ts docblock above it (:307-328) carries the split correctly.

    So the docblock presents the deprecated trigger as the design and never names the canonical one, 100 lines above a warning that says the opposite. The card is accurate.

    The card's open question, answered — the blast radius is one file

    The card asked whether the same pre-split wording survives in packages/mcp/README.md or the docs tree. Measured: it does not.

    • packages/mcp/README.md:73,:79 scope OS_MCP_SERVER_ENABLED to the HTTP surface only, and the README already names OS_MCP_STDIO_ENABLED.
    • content/docs/deployment/environment-variables.mdx:256 gates it to HTTP, and :263-267 carries the deprecation explicitly ("Deprecated (one release): OS_MCP_SERVER_ENABLED=true also used to auto-start …", plus the =false never-started-stdio caveat).
    • content/docs/releases/v16.mdx:170-173 announced the split.

    Every neighbour was updated when the split landed; plugin.ts's own class docblock was the one span missed. That makes the fix exactly the four comment lines the card proposes, with no doc sweep riding along — which is why this grades as a small card rather than a doc-consistency campaign.

    Scope ruled

    packages/mcp/src/plugin.ts comments only:

    • step 2 → "autoStart is enabled or OS_MCP_STDIO_ENABLED is truthy";
    • Environment Variables → keep OS_MCP_SERVER_ENABLED as the HTTP gate, add OS_MCP_STDIO_ENABLED as the stdio switch, mark the legacy OS_MCP_SERVER_ENABLED=true stdio trigger deprecated in the same words the runtime warning uses.

    No .describe() twin is owed (the card says so and it is right — this is a code comment, not spec prose). No behaviour change: touching the runtime here would be a different card.

    Routing

    domain:cli — packages/mcp sits in that lane per the lane table, and the fix lands nowhere else. priority:p3: comments only, no runtime effect. It is not p4-and-forget because the card measured the cost already paid once — the published skills/objectstack-ai MCP section was written from this docblock and shipped the error, caught in contract review and fixed in PR #14463 (5dabfd2a). This docblock is the upstream of that defect and it is still uncorrected. pm:queue.

    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    domain:cliRetired 2026-10-22, alias of domain:server (cli surface); in-flight cards keep it until they landpriority:p3

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions