Uh oh!
There was an error while loading. Please reload this page.
test(cli): the serve host-resolution sweep reports what it cannot resolve - #12535
Merged
Conversation
…olve An unresolvable specifier carried no package name, so it fell OUT of the judged population rather than into it: the sweep did not report the load as unknowable, it reported nothing and kept passing over the remainder. That is how the @objectstack/organizations load left this sweep during #11614, and teaching the resolver one more spelling only moves the boundary rather than making a crossing audible. An unresolved specifier is now a failure naming the file, the line, the callee and the specifier text, for every callee, unless the site is declared out of the sweep with its reason. Declarations are kept per callee so a bare import() can never inherit a host-anchored load's excuse, and a declaration matching no live site fails too. Three scanner repairs the loud path needed to be true: - `function importFromHost(` was read as a load site, adding a phantom, permanently unresolvable member to the population. - The whole argument list was read as the specifier, so the two-argument `importFromHost(pluginSpecifier, root)` was unresolvable for a reason unrelated to its specifier. - `resolveIdentifier` searched the whole file for the FIRST `const <name> =`, so the parameter `pkg` in `loadOptionalServicePlugin` resolved against a binding 1160 lines below it. The search is now confined to the source above the call and takes the nearest preceding binding. A blank specifier now counts as no specifier, and the sweep's own RED path is exercised over synthetic sources so it is observed on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
Whitespace only; no assertion, table entry or message text changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
This was referenced Aug 26, 2026
os-litant
marked this pull request as ready for review
August 26, 2026 09:26
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes#12162
The sweep in
packages/cli/src/commands/serve-cluster-host-resolution.test.tsasks whether every app-declarable optional load inserve.tsis host-anchored. A specifier it could not resolve carried no package name, so it fell out of the judged population rather than into it: the sweep did not report the load as unknowable, it reported nothing at all and went on passing over the remainder. Silence was the guard's own failure mode.Per the dispatch ruling the answer is fail loudly, not one more spelling. An unresolved specifier is now a failure naming the file, the line, the callee and the specifier text, for every callee.
The finding that changes the card
The card credits the hand-maintained four-package list with catching the
@objectstack/organizationsloss during #11614. Measured on currentmain, that list would not catch it again — and was already green for the wrong reason.resolveIdentifiersearched the whole file for the firstconstbinding of the identifier. The load atserve.ts:3217isimportFromHost(pkg)insideloadOptionalServicePlugin, wherepkgis a function parameter (declared at :3212). The resolver bound it to aconst pkg = Serve.ORGANIZATIONS_RUNTIME_PKGat :4377 — 1160 lines below the call, in an unrelated string helper. So the sweep reported an organizations load that does not exist at that line.Replaying the #11614 shape on today's tree (
const organizationsPkgwritten aslet organizationsPkg, the first spelling the card lists as vanishing) with the old resolver:The phantom at :3217 stood in for the real load at :2841. Every guard stayed green. ⇒ A whole-file first-match resolver can bind a name to a declaration that is not in scope at all, and the backstop the card credits was already defeated. That is more general than the gap this card was filed for.
The search is now confined to the source above the call and takes the nearest preceding binding — a
constonly exists below itself, the same temporal-dead-zone fact #10769 pinned forimportFromHost. Two app-declarable loads that the phantom was hiding (@objectstack/service-ai,@objectstack/service-ai-studio, passed as literals one frame above :3217) are now declared out of the sweep explicitly instead of accidentally.The premise was measured, and it is FALSE
Ruling: the swept region contains no dynamic load whose specifier is legitimately unresolvable. Measured against
origin/main3dafd8c9c: 8 of 43 load sites are unresolvable, and every one legitimately so — a served-artifact path, a loop over two CLI-declared packages, twoCAPABILITY_PROVIDERSmembers, the host importer's own fallback base, the app's config-plugin specifier, and a generic helper's parameter. A bare hard failure would be 8 false positives.So "fail loudly" is expressed as a declared boundary rather than an absent one, and the full census is written into the file (line, callee, argument, reason) so the next reader inherits it rather than the number 8. Two properties are what separate this from an allowlist, and both are enforced:
import()can never inherit a host-anchored load's excuse. RewritingimportFromHost(pkg)toimport(pkg)finds no entry and goes red.Three scanner repairs the loud path needed in order to be true, each removing a false positive rather than adding an excuse:
function importFromHost(was read as a load siteimportFromHost(pluginSpecifier, root)was unresolvable for a reason unrelated to its specifierresolveIdentifiersearched the whole fileA blank specifier (a template whose prefix is empty yields the empty string, which is falsy everywhere downstream) now counts as no specifier, so it is reported rather than dropped.
Proof the loud failure fires
Planted an unknown spelling in
serve.ts, ran the sweep, restored, and proved the restore by blob hash rather than exit code:serve.tsis unchanged in this PR — the diff is one test file. No build was involved in that ablation and none was needed: the sweep readsserve.tsandpackage.jsonfrom source withreadFileSyncat test time, so nothing resolves through adist/.The failure path is also exercised permanently, over synthetic sources, by nine new tests — a
let, a member of something other thanServe., a binding below its use, a same-named binding in another scope, a blank template prefix, a two-argument host load, and a comma inside a literal. A guard whose red path is never run is the same silence one layer up, which is this card's whole subject.The count floor at 20 — the answer, measured
The card's sharpest general point is that a guard which COUNTS cannot detect a specific member disappearing. True, and it is no longer asked to: that job now belongs to the loud check, by name, per site. But the floor is not redundant, and the split is the inverse of what the card assumed. Three ablations:
const→let)So the floor covers the literal half of "wrong but present" and the named list covers the identifier half. Both are kept; the floor's comment and failure message are rewritten to say what it actually guards, because its old justification ("every load now looks unknowable, which would empty the sweep below without failing it") is no longer true of this file.
Verification
At
dbb6f3343(the branch head):pnpm --filter @objectstack/cli exec vitest run src/commands/serve-cluster-host-resolution.test.ts—Test Files 1 passed (1) · Tests 22 passed (22)(13 before).pnpm --filter @objectstack/cli exec tsc --noEmit— exit 0, 0error TS.--listFilesconfirms the edited test file is in tsc's program, so this is not a vacuous reading.packages/clisuite:Tests 2119 passed | 5 skipped. Fourtest/*.e2e.test.tsfiles failed only becausepackages/cli/distwas absent (serve exited 2 before "Server is ready"); after building the CLI they areTest Files 4 passed (4) · Tests 8 passed (8).npx eslint . --no-inline-config --format jsonover the WHOLE repo: 5204 files linted, 0 errors, 0 warnings. Not a narrowed run.scripts/pm/dispatch-gates.mjsfor this diff: green, includingcheck:nul-bytes,check:test-source-alias,check:type-source-resolution,check:cross-package-test-inputsandcheck:i18n-coverage(OK (12 config(s), 602 baselined untranslated string(s), none new)).No changeset: the diff is one test file and publishes nothing. Nothing in PR #12149 changes — that repair is correct on its own terms and is why this gap was visible at all.
Generated by Claude Code