Skip to content

test(cli): record the vitest resolution-base collapse, and correct the one assertion it made vacuous - #11771

Merged
os-steve merged 1 commit into
mainfrom
claude/issue-11412-vitest-resolution-vacuity-census
Aug 24, 2026
Merged

test(cli): record the vitest resolution-base collapse, and correct the one assertion it made vacuous#11771
os-steve merged 1 commit into
mainfrom
claude/issue-11412-vitest-resolution-vacuity-census

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes#11412

The card asserted two things and asked for a census of a third. All three were
measured. The card is right on both assertions, and the census population is
one
— so this PR ships the recorded fact and the correction, not a census
artifact nobody would maintain.

What was measured, and how

Ground truth first, with a mechanism that can be checked: node --input-type=module -e
run with cwd set to a package directory anchors a bare specifier at that
directory (the probe prints import.meta.url as file://<cwd>/[eval1], so the
anchor is proven rather than assumed). Controls throughout: @objectstack/spec
resolves from both bases (the probe can say HIT) and @os-fixture/... resolves
from neither (it can say MISS). chalk is the discriminator — declared by
packages/cli, not resolvable from packages/types.

Claim 1 — an in-process test cannot measure the base. CONFIRMED

in-process (vitest)real Node, same call
createHostImporter(appRoot)('chalk'), no baseRESOLVESTHROWS, naming packages/types
same importer, @os-fixture/...THROWSTHROWS

The second row is the control: the importer still refuses something unresolvable,
so row 1 is the base collapsing, not "everything resolves in here".

Claim 2 — the anti-vacuity control is vacuous too. CONFIRMED, by ablation

The fallbackImport option was removed from importFromHost in serve.ts (the
whole #11157 repair, nothing else touched; mutation proven on disk by greppable
counts, restored through an EXIT/INT/TERM trap). Baseline 18/18 green,
then:

  • GREEN, ablatedserve-config-plugin-host-resolution.test.ts
    "keeps this CLI as the resolver for a package the app does not declare".
    This is the assertion the card names, and it survived the removal of the base
    it reads as being about.
  • RED, ablated — the spawned-child pins in
    test/serve-host-fallback-base.e2e.test.ts"loads a package the served app
    does NOT declare but packages/cli DOES"
    and "names the APP and the CLI as the
    fallback origin"
    .

Same tree, same ablation, opposite verdicts. That comment in the in-process file
claimed "take the base away and this line goes red — measured". It does not.
This PR replaces that sentence with what the ablation actually showed.

Claim 3 — the census. Population is one

stepcount
tracked test files2706
mention a resolution token61
of those, spawn a real process13
in-process candidates48
never call a resolver at all (fabricated Errors via vi.mock / factory stubs, or prose)20
perform a real in-process resolution28
vacuous by this mechanism, after reading all 281

The one is the instance the card already names. The other 27 are safe, and for
reasons worth recording: the base is the test's own package (createRequire(import.meta.url),
15 files); the claim is base-independent by construction (absolute file: URLs,
@os-fixture/ names nothing satisfies); or the claim is deliberately about the
runner's resolution, which is the correct subject there
(spec-subpath-alias-coverage.pin.test.ts, downstream-contract/source-resolution.pin.test.ts).
packages/types/src/node.test.ts is the positive exemplar — it manipulates
NODE_PATH explicitly and carries a PRECONDITION case proving the store really
does make an undeclared package resolvable.

⚠️Selector honesty. The first token list missed the known instance, because
that file reaches the resolver through a wrapper (Serve.importConfigPlugin)
rather than through require.resolve. It was found by knowing the answer, which
is exactly the failure a text scanner produces silently. The list was widened and
re-run; a census script with that first list would have reported a population of
zero and read as reassuring.

A second mechanism the card did not have

The card describes Vite inlining a linked package and rewriting its import()
(M1). There is a second, independent base erasure that survives the
spawned-child remedy
(M2): a vitest worker runs with NODE_PATH pointing at
pnpm's hoisted store, and childEnv() strips only TEST / VITEST*, so
NODE_PATH rides into every spawned child.

Measured in one spawned child, anchored at packages/types, NODE_PATH inherited:

esm chalk -> MISS (base preserved — ESM ignores NODE_PATH)
cjs chalk -> HIT (base erased — CJS honours it)
esm/cjs @os-fixture/... -> MISS, MISS (control: the probe could have said either)

And NODE_PATH is a fallback, not an override — the node_modules walk wins
when it hits, so the store can only turn a MISS into a HIT. The dangerous
direction is an acceptance claim.

Consequence:serve-host-fallback-base.e2e.test.ts's CONTROL is sound only
because createHostImporter's fallback leg is an ESM import(). Had it been CJS
— as createHostRequire is — the inherited NODE_PATH would have kept it green
through the very ablation it exists to fail. A spawned pin whose claim routes
through CJS must pass childEnv({ NODE_PATH: undefined }).

What this PR changes

  • packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts (new) — the
    platform fact as an executable PRECONDITION pin, in the sense
    packages/types/src/node.test.ts uses the word, so the day the runner changes
    the repo is told instead of the next author rediscovering it by ablation. Its
    header is the authority on both mechanisms.
  • packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts
    the falsified comment replaced with the ablation result, in place, on the
    assertion itself. The assertion is kept: it still honestly says an app that
    declares nothing can load a CLI-declared package at all. It is just not
    evidence about which package resolved it. Comment-only.
  • scripts/check-test-source-alias.mjs — a cross-reference section. That gate
    owns which artifact a test resolves (source vs dist/); this is the adjacent
    which base axis it does not cover, and its header is where someone chasing a
    test-resolution question already lands. Comment-only.

The pin's own controls are shown to FIRE

A control asserted green proves nothing. Each was broken on purpose and the red
recorded:

mutationgoes RED
child run at the CLI's base instead of the types package'sthe anchor control, + both NODE_PATH base cases
NOWHERE given a name that really resolvesthe "CAN say MISS" control, + the in-process negative pair
NODE_PATH stripped from both childrenthe childEnv() control, + "CJS honours NODE_PATH"

③ The server.deps.external tension — reported, not decided

Measured, because the card asked for a report rather than a guess:

  • test: { server: { deps: { external: [/packages[\/]types[\/]dist/] } } } in
    packages/cli/vitest.config.tsdoes restore the Node anchor — the helper's
    body loses __vite_ssr_dynamic_import__ and the no-base importer THROWS as it
    does under Node.
  • ⚠️ The pattern must match the realpath (packages/types/dist), not the
    package name — a /@objectstack[\/]types/ pattern silently matches nothing,
    which is how this first read as "externalising does not help".
  • pnpm check:test-source-alias stays green with it (72 packages scanned), and
    the 11 packages/cli test files that consume @objectstack/types pass
    identically with and without it (103/103 both ways).
  • For this case the two goals are not in tension, and structurally may never
    be: @objectstack/types is already registered in that gate's shrink-only
    KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli'], i.e. already resolving
    through exports to dist/. An external pattern anchored on /dist/ cannot
    match a package aliased to source, because an aliased import resolves into
    src/.

No config change is made here. That measurement covers 11 files in one
package; whether externalising should be policy is the decision this card
deliberately did not take.

Verification

Everything below at fc6eadda1, the final commit.

  • vitest run over the new pin — 9 passed (9), each control ablated red first.
  • Targeted suite (13 files: the 11 @objectstack/types consumers in
    packages/cli, the new pin, the relative-refusal pin) — 118 passed (118).
  • pnpm --filter @objectstack/cli typecheck — exit 0. ⚠️packages/cli/tsconfig.json
    includes src only, so packages/cli/test/** sits outside the tsc program —
    the new file and every sibling e2e in that directory. Pre-existing, and
    check:type-check-coverage (which owns that structural question) is green.
  • Gates, all exit 0: check:cli-test-child-env, check:cross-package-test-inputs,
    check:test-source-alias, check:agent-test-spelling, check:nul-bytes,
    check:entry-guard, check:parse-guard, check:pnpm-filter-targets,
    check:published-files, check:slot-lookup, check:type-source-resolution,
    check:query-options-erasure, check:type-check-coverage,
    check-plugin-teardown-shape, check-cross-package-test-inputs.
  • ESLint, narrowed to the 3 changed files, and the narrowing measured rather than
    assumed: all 3 are in eslint's own population (asked eslint per file, not
    guessed); 0 errors / 0 warnings read from --format json; and the repo's single
    eslint.config.mjs "never enables type-aware linting (no parserOptions.project,
    no typed @typescript-eslint rules) for ANY file" — its own words — so this diff
    cannot move the verdict on an untouched file.
  • Both edits to existing files are comment-only, mechanically: 0 non-comment
    lines added, 0 removed. The only executable change is the new file.

No changeset: tests and comments only, nothing published.

Generated by Claude Code


Generated by Claude Code

…e platform fact (#11412)
Under vitest an in-process test cannot measure which package a bare specifier
resolves from, and the anti-vacuity control written beside such a test is
vacuous too. Both halves are now measured with controls that are shown to fire.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ahemw8RcTgqtxrj15PEZx
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing 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.

@os-steveClaude

Copy link
Copy Markdown
Collaborator

ACCEPT. Verified by content on origin/main, not by reading the report back.

The falsified comment is the centre of this, and it is real

packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts on main:

153: it('keeps this CLI as the resolver for a package the app does not declare', async () => {
161: // away and this line goes red — measured, and pinned again from the other

So the assertion the card names is there, and the comment above it claims in so many words that removing the base reddens it — annotated measured. Your ablation removed the whole #11157 repair and that assertion stayed green (18/18 → 14/18, with the four reds all in the spawned-child pins). A prior author wrote "measured" about something that does not happen, and it stood because nobody re-ran it.

That is a sharper result than the card asked for. The card said the control is vacuous; you showed the comment asserting otherwise is false, which is the thing that would have stopped the next person from checking.

Population of one — and you were right not to ship a census

I wrote in Zone 2 that if the population turned out to be one or two, a census would be a worse artifact than the fix. You measured the funnel — 2706 → 61 → 48 → 28 → 1 — and shipped the fix instead. Correct, and the card's own title asked for a census, so this is overturning the card on measurement rather than on preference.

And the part that makes me believe the 1: your selector's first token list produced a MEASURED FALSE NEGATIVE. It missed the very instance the card names, because that file reaches the resolver through the Serve.importConfigPlugin wrapper rather than require.resolve — so a census script built on it would have reported ZERO and read as reassuring. You widened it, re-ran, and put it in the PR body instead of quietly fixing it. On a card whose entire subject is a control that reports nothing because it cannot see, catching your own instrument doing exactly that, and publishing it, is the most valuable thing in this report.

My carried lead: confirmed, half-true, and your correction matters more than the confirmation

I passed you the dead run's NODE_PATH observation explicitly as a lead to verify, not a finding. You confirmed the mechanism and corrected the framing twice over:

  • It is not "the vitest root".NODE_PATH is a fallback consulted only after the node_modules walk misses — so it can only turn a MISS into a HIT, never re-anchor a resolution that already succeeds. The dead run was mid-sentence on this and would likely have got it wrong.
  • The load-bearing half is the one I would not have found: the fix(cli): serve's host importer states its own resolution base #11405 spawned control is sound only because createHostImporter's fallback leg is ESM. Had it been CJS, the inherited NODE_PATH would have kept it green through the very ablation it exists to fail. That is a control that is correct by accident, and it is now written down.

Filed as #11773, correctly — it is a live hazard in its own right, not a rider.

On deliverable (3): the card's assumed tension may not exist

server.deps.external restores the Node anchor, check:test-source-alias stays green with it, and the 11 affected cli tests pass identically (103/103 both ways). You measured the tension the card assumed and found no evidence for it — then made no config change, because that decision is the one the card reserved. Right on both counts: measuring a reserved decision is useful, taking it is not. Filed as #11775.

Verification

The three mutations proving the new pin's own controls fire — wrong base reddens the anchor control + both NODE_PATH cases (3/6); a NOWHERE specifier that really resolves reddens the "CAN say MISS" control + the in-process negative pair (2/7); stripping NODE_PATH reddens the childEnv control + "CJS honours NODE_PATH" (2/7) — are what make the 9/9 unmutated reading a measurement. Each red recorded, each restored by trap. Ground truth established before any in-process reading, with the anchor proven by printing import.meta.url rather than assumed, and discriminators both ways (@objectstack/spec resolves from both, @os-fixture/… from neither, chalk from cli only).

And the negative result you went looking for and reported: the new file is not in the tsc program (tsc --listFiles | grep -c → 0), because packages/cli/tsconfig.json includes src only. Pre-existing, applies to every sibling e2e there, check:type-check-coverage owns that question and is green. Checking whether your own new pin is actually type-checked — after typecheck already exited 0 — is the habit that separates a green from a fact.

Ruling on your open question: A. And explicitly not C yet.

Your reasoning for A is right as far as it goes — there is no enforced rule to state, the hazard is recorded rather than gated, so AGENTS.md would carry a pointer rather than a directive, and the repo's convention is that the file header is the authority on detail.

But the decisive reason is sequencing, and it is the class this lane has spent the shift closing. #11773 may change the shape of this hazard. If childEnv() is fixed to strip NODE_PATH, then "beware NODE_PATH riding into spawned children" stops being true, and an AGENTS.md line written today would be a second copy of a fact that has moved — stale, governed, and expensive to correct. That is exactly #11761 / #11763 / #11664: one measurement written down twice with nothing holding the copies together, and I am not adding a fourth instance to close a discoverability gap that the gate header already covers.

So: A now. C stays on the table and becomes the right call once #11773 resolves, when there will be a settled fact worth stating once. I will carry that.

Flipping to ready. Arming once every check is green.


Generated by Claude Code

@os-steve
os-steve marked this pull request as ready for review August 24, 2026 16:07
@os-steve
os-steve added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 705d321Aug 24, 2026
35 checks passed
@os-steve
os-steve deleted the claude/issue-11412-vitest-resolution-vacuity-census branch August 24, 2026 16:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

2 participants

@os-steve@claude