Repository navigation
test(cli): pay the cloud-connection transform at module load, out of the clocked window - #17659
Merged
os-sales merged 1 commit intoSep 11, 2026
Merged
Conversation
…the clocked window
`optional-package.test.ts`'s end-to-end control — "resolves a real workspace
package rather than mistaking it for absent" — called
`loadOptionalPackage('@objectstack/cloud-connection')` inside vitest's default
5000ms `testTimeout`. With `packages/cloud-connection/dist` built it took
5005ms / 5043ms across two runs and timed out; the same import in plain node
from `packages/cli` takes 856-1003ms on the same box.
The gap is not import cost. Every workspace package here is a pnpm link whose
realpath carries no `/node_modules/` segment, so vitest's default
`server.deps.external` inlines it — `dist/` included — and the probe paid a
cold whole-package source-graph transform inside the clock.
Fix is the one `scripts/check-test-source-alias.mjs` prescribes in its
clocked-window rule and that five sibling `packages/cli` test files already
carry verbatim: a module-top side-effect import, so the transform is paid
during collection, which vitest clocks against nothing. The timeout is NOT
raised — the budget is the only thing currently reporting the cost.
The assertion is untouched: the `it()` still calls `loadOptionalPackage()` on
the real specifier and still asserts `state === 'loaded'`, so a genuinely
absent or broken package still fails it.
Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c
Co-authored-by: Claude <noreply@anthropic.com>
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): |
os-sales
marked this pull request as ready for review
September 11, 2026 09:11
os-sales
enabled auto-merge
September 11, 2026 09:11
os-sales
deleted the
claude/issue-17180-optional-package-probe-transform
branch
September 11, 2026 09:39
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 free
to 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 #17180
packages/cli/src/utils/optional-package.test.ts's end-to-end control — "resolves a real workspace package rather than mistaking it for absent" (theit()is at:108, the call at:112; the card cited:108and that still locates it) — calledloadOptionalPackage('@objectstack/cloud-connection')inside vitest's default 5000mstestTimeoutand paid a cold whole-package source-graph transform there.One line changed, plus the comment explaining it. The timeout is not raised: the budget is the only thing currently reporting the cost, and the probe answers "does this package resolve", so a 7x cost it never meant to incur is the defect rather than the budget.
The shape, for #16497's taker
A module-top side-effect import of the specifier (
import '@objectstack/cloud-connection';), leaving the dynamic call in the test body untouched, so the transform is paid during collection — which vitest clocks against nothing — and the clocked call becomes a module-registry lookup.That is the remedy
scripts/check-test-source-alias.mjsprescribes in its own clocked-window diagnostic, and it is already landed verbatim in five siblingpackages/clitest files —src/commands/doctor-ledger-read-failure.test.ts:101,src/commands/doctor-ledger-posture-independence.test.ts:77,src/commands/doctor-ledger-dir-authority.test.ts:63,test/serve-marketplace-offline-install.test.ts:39,test/serve-marketplace-offline-runtime-config.test.ts:69— three of them for this same specifier, each under the same[#10126]comment. This PR copies that idiom rather than inventing anything; abeforeAllwas not an option (the gate rules it out near:266— "paying it in a hook is still a bad idea").Measured, with
packages/cloud-connection/distBUILTThe card's repro is conditional on that dist existing; with it absent the test fails differently, so it was built first (
pnpm --filter '@objectstack/cloud-connection...' build). Per-test durations read from vitest's own JSON reporter, two runs each, same command both sides, only this diff differing.Test timed out in 5000msThe other five tests in the file sat at 3–15 ms throughout, before and after — they never touched the transform.
Reference points on the same (shared, contended) box: the same import in plain node from
packages/clitakes 856 / 918 / 1003 ms. So the after figure is not merely "under budget" — at ~5.5 ms it is two orders of magnitude below the node baseline, which is the signature of the load having moved out of the window entirely rather than having got cheaper. A third reading taken after the full cli dependency closure was built agrees: 5.21 ms / 5.57 ms.The control arm is still live
The value of this test is that it resolves the REAL package, so nothing here mocks the import away. Proven by ablation, not by argument: with the body's specifier swapped for a genuinely absent package and the preload left in place, the test fails:
'absent'is the point — the preload does not make the assertion pass vacuously; the assertion still discriminates on a real resolution. The mutation was proved on disk (occurrence counts 1 to 0, injected 0 to 1,git hash-objectchanged) and restored to theHEADblob withgit diff HEADempty. An earlier attempt at this ablation refused to run because its anchor matched twice — the second occurrence being prose in the new comment — and reported the reading void rather than measuring; that is the guard working, and the re-run used the unique statement as its anchor.One behaviour change, deliberate
In a worktree where
packages/cloud-connection/distis unbuilt, this file now fails at collection ("Test Files 1 failed / Tests no tests") instead of collecting and failing one test withstate: 'broken'. Measured by parking the dist and running. The error names the package and the unresolvable entry, so the unbuilt-artifact reading survives and is arguably more direct; it is also exactly how the five sibling files above already behave, so onepnpm --filter @objectstack/cloud-connection buildclears all six identically.Gate gap, filed not fixed — #17658
check-test-source-alias.mjsdocuments this exact hazard and CI was green on this file. Which of the two possibilities holds was measured: the gate covers this shape — the specifier is already inKNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli']— but the call escapes its predicate, becausemoduleLoadSites()text-scans for a literal specifier insideimport(...)/require(...)and here the specifier is a string argument to a helper that imports a variable. Two legs on the identical line: indirect spelling exits 0 and prints nothing, literal spelling exits 1 and prints the remedy used above. Details and the scope question in #17658. No widening here — the gate script is outside this PR's file surface.Acceptance notes
skip-changeset, decided by measurement rather than by "it's only a test":@objectstack/cliis published and itsfilesaredist,README.md,CHANGELOG.md. A symbol unique to the changed file gets 0 hits across those paths in a built tree; positive controlloadOptionalPackagegets 3 (dist/utils/optional-package.js, its.d.ts,dist/commands/doctor.js). Nothing published moves.--ranat 54 derived / 54 run / 0 unrun.pnpm check:test-source-aliasgreen.pnpm lintgreen repo-wide (the fulleslint . --no-inline-config, not a narrowed subset).pnpm --filter @objectstack/cli typecheckgreen. cliunittier green at 195 files / 2700 tests once the dependency closure and cli's own dist were built;integrationtier declared to CI, since the diff touches no integration-tier file and no spawn entry.PREREQUISITE NOT METon an unbuilt workspace, which each states in its own words is "NOT a pass: nothing was measured" (check:dual-build-cjs-loads,check:i18n,check:i18n-coverage,check:i18n-walk-parity,check:type-check-debt). They need a whole-workspace build and are recorded as NOT MEASURED, not as red.packages/cli's[#10126]preload comment is duplicated verbatim across what is now six files. Successor: none — no PR or person is queued on those files, and hoisting a comment into a shared constant would be worse than the repetition.🤖 Generated with Claude Code
https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c