Skip to content

test(cli): pay the cloud-connection transform at module load, out of the clocked window - #17659

Merged
os-sales merged 1 commit into
mainfrom
claude/issue-17180-optional-package-probe-transform
Sep 11, 2026
Merged

os-sales merged 1 commit into
mainfrom
claude/issue-17180-optional-package-probe-transform

Conversation

@os-sales

@os-sales os-sales commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

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" (the it() is at :108, the call at :112; the card cited :108 and that still locates it) — called loadOptionalPackage('@objectstack/cloud-connection') inside vitest's default 5000ms testTimeout and 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.mjs prescribes in its own clocked-window diagnostic, and it is already landed verbatim in five sibling packages/cli test 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; a beforeAll was 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/dist BUILT

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

run 1 run 2 result
before 5005.22 ms 5042.80 ms FAILED — Test timed out in 5000ms
after 5.74 ms 5.46 ms passed

The 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/cli takes 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:

AssertionError: expected 'absent' to be 'loaded'
 Tests  1 failed | 5 passed (6)

'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-object changed) and restored to the HEAD blob with git diff HEAD empty. 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/dist is unbuilt, this file now fails at collection ("Test Files 1 failed / Tests no tests") instead of collecting and failing one test with state: '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 one pnpm --filter @objectstack/cloud-connection build clears all six identically.

Gate gap, filed not fixed — #17658

check-test-source-alias.mjs documents 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 in KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli'] — but the call escapes its predicate, because moduleLoadSites() text-scans for a literal specifier inside import(...)/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/cli is published and its files are dist, README.md, CHANGELOG.md. A symbol unique to the changed file gets 0 hits across those paths in a built tree; positive control loadOptionalPackage gets 3 (dist/utils/optional-package.js, its .d.ts, dist/commands/doctor.js). Nothing published moves.
  • 54 gate families derived from the final one-path diff and all 54 run, reconciled with --ran at 54 derived / 54 run / 0 unrun. pnpm check:test-source-alias green. pnpm lint green repo-wide (the full eslint . --no-inline-config, not a narrowed subset). pnpm --filter @objectstack/cli typecheck green. cli unit tier green at 195 files / 2700 tests once the dependency closure and cli's own dist were built; integration tier declared to CI, since the diff touches no integration-tier file and no spawn entry.
  • Five of the 54 gates exit 3 = PREREQUISITE NOT MET on 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.
  • Noted, not filed: 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

…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>
@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.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json d46deba195233f3d72652975c9b52fa3b3ec4e4b → packageMentionDocs.

@github-actions github-actions Bot added the tests label Sep 11, 2026
@claude claude Bot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 11, 2026
@os-sales
os-sales marked this pull request as ready for review September 11, 2026 09:11
@os-sales
os-sales enabled auto-merge September 11, 2026 09:11
@os-sales
os-sales added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 8955995 Sep 11, 2026
45 checks passed
@os-sales
os-sales deleted the claude/issue-17180-optional-package-probe-transform branch September 11, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

2 participants