Uh oh!
There was an error while loading. Please reload this page.
fix(cli): resolve an app-declared config plugin from the app, not the CLI - #10948
Conversation
… CLI (#10908) `plugins: [...]` in the served app's own `objectstack.config.ts` is the documented extension path, and its string entries are the most app-owned specifiers in `serve.ts` — yet they were loaded with a bare `import()`, which Node ESM resolves against the CLI's realpath. A plugin the APP declares was therefore only loadable where it happened to be hoisted somewhere the CLI could see it: green in a dev checkout, missing on a real distribution layout. Same class as cloud#1013 and #10645, on the surface users are told to use. `Serve.importConfigPlugin` now lets the DECLARATION pick the resolver, which is what #4719 says should decide. Handing every specifier to `importFromHost` would not have been a superset — measured, twice over: * a relative specifier would re-base from this file's directory to `@objectstack/types/dist/`, because the host importer's pass-through re-enters `import()` from inside that package; * an UNDECLARED bare name would too, and that one takes working deployments away: from an app that declares nothing, `@objectstack/plugin-auth` and `@objectstack/plugin-audit` resolve from the CLI and fail through the host importer under a pnpm-isolated layout. So only the declared case moves; the other two branches keep the exact resolution they had. Nothing changes about which plugins are ACCEPTED — the #4719 declaration gate is untouched and no undeclared package gains a way in. The missing-plugin diagnostic now nests the #4719 remedy, and is pinned as a chosen text rather than left to drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
…ared plugin (#10908) The first draft of this file could not tell the two apart: its fixture package was invisible to the CLI, so the declared branch and the undeclared fallback both converged on the app's copy and an ablation that disabled the declaration check stayed green. Declaring a package the CLI ALSO resolves (`chalk`) is what makes "which copy wins" observable, which is the resolution-policy question the card is actually about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
…ir questions (#10908) Both non-declared branches exist because of a measurement, and each measurement turned into a card: #10943 (the host importer's fallback resolves from `@objectstack/types`, contradicting its own docblock) and #10944 (a relative `plugins:` entry resolves against the CLI's directory, so it cannot work). Naming them here is what stops the next reader re-deriving the branch or deleting it as redundant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
📓 Docs Drift CheckThis PR changes 1 package(s): 16 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c990f268e6f98ed207c379831ff1d4166ba0bc22 && git checkout c990f268e6f98ed207c379831ff1d4166ba0bc22
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9d101d28424fcdb067277709bbbb9097a3bc2fd9 74bab7ccfefbd69896cd85499ee1fe7ece7c5635 && git checkout -B drift-repro 9d101d28424fcdb067277709bbbb9097a3bc2fd9 && git merge --no-ff 74bab7ccfefbd69896cd85499ee1fe7ece7c5635
node scripts/docs-audit/affected-docs.mjs --json 9d101d28424fcdb067277709bbbb9097a3bc2fd9
|
⛔ merge queue 构建失败 — 先分诊,再决定要不要重排队列构建 32525740548 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集), 失败的 job(日志抽取,best effort):
跨 PR 相同签名(24h,按失败测试文件聚合):
历史信号:
分诊清单:
Generated by Claude Code · merge-queue-triage workflow (#4859) |
Uh oh!
There was an error while loading. Please reload this page.
… scans (#10514) Two `packages/cli` scans read `serve.ts` raw, with no comment/code separator at all, so prose counted as code -- the FABRICATES direction `scripts/js-comment-mask.mjs`'s own header calls the worse one. Neither scan was wrong today, but both were one ordinary comment away from being wrong: - `serve-email-config-parity.contract.test.ts`'s `keysReadFromConfigEmail()` matched `cfgEmail\.\w+` over raw source, so a comment naming an undeclared key would have fabricated a false "undeclared key" red, and -- the worse direction -- a comment naming a declared-but-unread key would have silently restored the exact `DECLARED_BUT_UNREAD` exemption this file's own docblock says was deleted for good after #5447/#5470. - `serve-multi-node-cap-advisory.pin.test.ts`'s four shape assertions matched regexes over raw source, so a reverted call could hide behind a trailing comment quoting the shape it replaced. Both now scan `maskComments(SERVE_SOURCE)` instead. `interfaceFields()`'s own narrower, brace-matched strip is untouched -- out of scope per the issue. Re-verified per the card's release condition: on origin/main at branch point (047ac86), the raw / naive-stripped / masked key sets for subject 1 and the raw / masked match verdicts for subject 2's four assertions are all identical -- no verdict moved by #10453/#10919/#10948/#10956's changes to serve.ts since the card was written. `CROSS_PACKAGE_TEST_INPUTS['@objectstack/cli']` and the turbo.json `@objectstack/cli#test` input already carry `scripts/js-comment-mask.mjs`/`.d.mts` (landed with #10453/PR #10513), so no registration work was needed -- just the two-site routing. Adds vacuity-proof tests demonstrating the FABRICATES shapes on synthetic sources: a comment containing the exact pattern each scan keys on is shown producing the wrong verdict when read raw, and the right one once masked. Fresh answer to the open question #10909's dev never returned: yes, editing comment prose in serve.ts could move both scans' results pre-fix (demonstrated above); post-fix it cannot. Side effect caught by `check:cross-package-test-inputs`: adding the `maskComments` import gave `serve-email-config-parity.contract.test.ts` its first real escaping read, which newly exposed a PRE-EXISTING prose mention of `packages/spec/src/system/email-config.test.ts` (a doc cross-reference, never actually read) to the gate's literal collector. Reworded to name it without a single quoted repo-relative literal, following the precedent already set in this directory's `serve-multi-node-cap-advisory.pin.test.ts` for the identical gate. Fixes#10514
Fixes#10908
plugins: [...]in the served app's ownobjectstack.config.tsis the documented way to extend a deployment, and its string entries are the most app-owned specifiers inserve.ts. They were loaded with a bareimport(), which Node ESM resolves against the CLI's realpath — so a plugin package the app itself declares was only loadable where it happened to be hoisted somewhere the CLI could see it. True in a dev checkout, false on a real distribution layout. Same mechanism as cloud#1013 and #10645, on the surface users are explicitly told to use.Triage assumption 1 was tested, and it is FALSE as stated
Triage marked its assumptions "to be tested, not inherited", and said to report a fork rather than force one. Assumption 1 was that
createHostImporteris a strict superset of a bareimport()and thatawait importFromHost(plugin)is therefore the whole repair. Measured on a real fixture, it is not a superset — in two independent ways, both of which would have taken working deployments away.1. A relative specifier changes base. The host importer's pass-through hands the specifier to an
import()that physically lives in@objectstack/types, and ESM resolves a relative specifier against the module containing the call:2. An undeclared bare name changes base the same way — and that one bites. The fallback is documented as "the importing package's own resolution", but it is the same
import()inside@objectstack/types, which under a pnpm-isolated layout sees only that package's own dependencies. From a temp app root that declares nothing:An app that writes
plugins: ['@objectstack/plugin-auth']without declaring it — a spelling this repo's own fixtures use — boots today and would have stopped booting.What this PR does instead
The declaration picks the resolver, which is what #4719 says should decide. Three branches, and only one of them moves:
file://URL,node:builtinNothing changes about which plugins are accepted — only where a declared one resolves from. The #4719 declaration gate is untouched and no undeclared package gains a way in that it did not already have; #10765's refusal pin stays green.
The two non-declared branches are not defensive padding: each exists because of one of the measurements above, and each measurement is now its own card — #10943 (the host importer's fallback contradicts its own docblock) and #10944 (a relative entry cannot resolve to anything an app owns). Both are named in the code so the branches are not re-derived or deleted as redundant.
The diagnostic is pinned, not drifted
Triage assumption 2 required the changed error text be a chosen behaviour. The
Failed to import plugin 'name'wrapper moved into the helper so the composed string is testable, and it now nests the #4719 remedy — "Declare it in that app's package.json" for a package the app never declared, or the install-problem text for one it declares but has not installed. Both are asserted.The source sweep
#10769's
UNRESOLVABLE_BARE_IMPORTSentry for this specifier is replaced, not removed. Two bareimport()sites remain in the helper, but both are now reached only after the declaration has been consulted, so neither can carry a package name the app declares — which is exactly the justification that list asks for. The vacuity guard and its floors are untouched.Verification
Run on
74bab7ccfe, the head this PR ships:packages/cli—Test Files 5 passed (5),Tests 40 passed (40), covering the new pins plusserve-cluster-host-resolution,serve-host-config,serve-optional-plugin-intentand theserve-organizations-host-resolutione2e.pnpm --filter @objectstack/cli typecheck— exit 0.scripts/pm/dispatch-gates.mjs, all exit 0:check-nul-bytes(OK (scanned 6313 text file(s) ... no raw ASCII control bytes)),check-engine-double-contract(OK — 376 pinned, 133 in the DEBT ledger, 2 exempt),check-cross-package-test-inputs(OK: 13 package(s) read outside themselves, all declared),check-ci-filter-parity,check-plugin-teardown-shape,check-changeset-no-major,check-empty-changeset,check:where-matcher,check:query-options-erasure,check:type-check-coverage,check-affected-docs.Reverse-verification. The subject is read from source (the test imports
./serve.jsinside the same package, so vitest loadsserve.tsdirectly — nodist/hop, hence no rebuild leg; the mutation reaching the assertions is itself the proof). Disabling the declaration check turns the app-copy pin red (1 failed | 8 passed) and restoring it returns9 passed. The first draft of that test could not detect this: its fixture package was invisible to the CLI, so both branches converged on the app's copy and the ablation stayed green. Declaring a package the CLI also resolves (chalk) is what makes "which copy wins" observable, and that is the pin that now bites.Out of scope, deliberately
#10909 is untouched. Its two comments still claim the capability loop loads "host copy first" while it bare-imports at
spec.pkgandex.pkg; this PR changes neither the comments nor those loads, so they remain exactly as false as before. One warning for whoever takes that card: those providers are all CLI-declared, so swapping them ontoimportFromHostnaively would hand every one of them to the fallback measured above and break them under a pnpm-isolated layout. #10943 has the numbers.Generated by Claude Code