Skip to content

Two serve.ts comments claim the capability loop loads "host copy first", but it bare-imports — the CLI's copy always wins #10909

Description

@os-elon

Filed unassigned from the domain:cli lane while implementing #10769. Recording only.

The false claim

packages/cli/src/commands/serve.ts carries the phrase twice, at :37 and :44 (measured on the #10769 branch):

Values only (no plugin class): the capability loop dynamic-imports SmsServicePlugin itself, host copy first.

What the loop actually does

serve.ts:2987 and :3052:

constmod: any=awaitimport(/* webpackIgnore: true */spec.pkg);
...
constexMod: any=awaitimport(/* webpackIgnore: true */ex.pkg);

Bare import(). Node ESM resolves that against the CLI's own realpath, so the CLI's copy always wins and the host's copy is never consulted. "Host copy first" describes a behaviour the code does not have.

Why it matters beyond the comment

This is the same shape that made #10769's defect invisible for two shipped incidents: a locally reassuring sentence that tells the next reader the resolution question is already handled. #10769's own card calls out the sibling case (serve.ts's "declared above, before the auth block" note) for exactly this reason; that one was fixed there, these two were not, because they claim something about a different load site.

Note the class is currently latent rather than live: every package in Serve.CAPABILITY_PROVIDERS (21 of them — service-automation, service-analytics, plugin-audit, service-cache, service-storage, service-queue, service-job, service-messaging, the three trigger-*, service-realtime, mcp, service-package, plugin-email, service-sms, plugin-sharing, plugin-pinyin-search, plugin-reports, plugin-approvals, service-settings) is declared by packages/cli, so bare resolution finds all of them today. That is why #10769's widened sweep does not flag these sites: its rule is "a package the CLI does not declare must be host-anchored", and these are all declared.

Two ways to close it, and they are not equivalent

  1. Correct the comments to say the capability loop takes the CLI's copy. Cheap, honest, closes the misleading-reassurance half only.
  2. Make the comments true — route the capability loop through importFromHost, so an app that declares its own version of a capability provider gets its copy. This is a real resolution-policy change: it decides that the app's declared version outranks the CLI's bundled one for ~21 packages, with the module-duplication and version-skew consequences that implies. It is plausibly the right long-term contract (it is what importFromHost means everywhere else in the file) but it is a decision, not a repair, and it was deliberately left out of [finding] serve.ts's host-anchored importer is a mid-function const, so any app-declared optional package loaded ABOVE it silently falls back to CLI-anchored resolution — this has now shipped twice #10769 to keep that PR to the class it was dispatched for.

A grader should pick one; shipping neither leaves a comment that reads as a guarantee.

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions