fix(test-infra): the SDUI registration pins attribute a key to its source spelling, not to whichever spelling was built - #7793
Merged
Conversation
…urce spelling, not to whichever spelling was built `derivePinnedKeys` attributes each derived key to the FIRST module it read the key from, and a `sideEffects` array names every registrar TWICE — once as `src/x.tsx`, once as `dist/x.js`. With no read order of its own, the winner was decided by the array's literal order in `package.json` and by whether `dist/` happened to be on disk. `packages/app-shell/dist` is gitignored, so the same commit answered the source spelling on an unbuilt checkout and the published one on a built one — a permanent red in the file that holds the pins that matter. The `dist` preference is not load-bearing and was never expressed anywhere: the verdict reads `keys` (a union deduplicated by key) and the chunk counts; the `sources` map enters it only through `.has()`, and is otherwise the diagnostic column. Measured on a fully built tree: 16 keys from 32 modules before and after, identical per-key chunk counts, gate exit 0 both ways. So the read is ordered source-first, using the package's own derived spelling map rather than a hardcoded `dist` test, and a package whose spelling map cannot be derived throws instead of quietly reverting to array order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sam
marked this pull request as ready for review
September 5, 2026 20:50
This was referenced Sep 5, 2026
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
2 participants
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 #6893
scripts/__tests__/check-sdui-registration-pins.test.tshad a verdict that was a function of untracked local state:packages/app-shell/distis gitignored (git ls-filesreturns 0 entries) andderivePinnedKeyswalks the filesystem withfs.existsSync, so the same commit passed on an unbuilt checkout and failed on a built one — a permanent red sitting in the same file as the pins that actually matter.1. The load-bearing measurement — is the
distpreference deliberate?It is not, and it is not expressed anywhere in the gate. The measurement, not taste, selects the repair.
Static half.
derivePinnedKeysattributes a key to the first module it read the key from (if (!sources.has(key)),check-sdui-registration-pins.mjs:119). Nothing in the gate says "prefer dist". The preference was a by-product of two facts with nothing to say about it:packages/app-shell/package.jsonlists all fifteen./dist/*.jsentries before all fifteen./src/*entries — the array names every registrar in both spellings, which ischeck-side-effects-array.mjs's own rule ("the array names EXACTLY: entry forms + every module ... in BOTH its source and its published spelling");dist/is on disk.The
sourcesmap is read at exactly four sites, and only one of them can reach a verdict::187theRULED_CONTROLSfloorsources.has(key)— membership only:201--listoutput:233per-key report row:234dropped-key error messageThe verdict itself is a function of
keys(a union deduplicated by key),unreadable,modulesRead, the chunk list and the sentinel. Reordering the read cannot move a union.Empirical half — the real gate, run end to end on a fully built tree (39 package
dist/dirs plusapps/console/dist/assets, 518 chunks), pre-fix binary recovered withgit show HEAD:...so both arms are real runs of real code:Only the source column moved, e.g.
mcp:connect-agentfrompackages/app-shell/dist/console/connect/ConnectAgentWidget.jstopackages/app-shell/src/console/connect/ConnectAgentWidget.tsx.modulesReadstayed at 32, so the published spelling is still read — this is a reordering, not a filter.⇒ Option 1 (source-first), per the triage ruling of 2026-09-04. Option 2 was never in play (a conditional skip stops the case running on exactly the machines where it fires); option 3 is the branch the ruling reserved for a load-bearing
distpreference, and the measurement says there is none.srcRootcomes from the package's ownderiveSpellingMap— derived from the manifest and round-trip-checked — rather than a hardcodeddisttest, so this file does not grow a second answer to "which prefix is the source one". A package whose spelling map cannot be derived throws rather than quietly reverting to array order;check-side-effects-array.mjsowns that condition and already reports it as exit 2 (ci.yml:341), so such a workspace is red there too.2. Does CI hit this? No — and the reason is step order, measured, not assumed.
The card left this unmeasured and one seat had read it one way. Read out of
.github/workflows/ci.yml(read-only; PR #7789 is editing that file):testjob — "Test (shard N/4)",ci.yml:611. This is the job that ownsscripts/__tests__/. Its steps, in order:pnpm install --frozen-lockfilepnpm test --shard=N/4pnpm test:dist(shard 1 only, after the tests)There is no build step before
pnpm test. The push lanetest-coverage(ci.yml:789) has the same shape: install at 821-822, tests at 830-831, no build.The only two build invocations anywhere in
ci.ymlareci.yml:1182(pnpm --filter @object-ui/console exec vite build, inside thee2ejob at 1104) andci.yml:1503(pnpm turbo run build --filter='@object-ui/site', inside thedocsjob at 1371). Both are different jobs, i.e. different runners with different filesystems, so neither can putpackages/app-shell/diston thetestjob's disk.pnpm test:distat 720-722 isturbo run test:dist --filter=@object-ui/componentsand runs after the shard'spnpm test, so it cannot affect it either.Confirming the install leg empirically rather than by reading:
pnpm installin a fresh worktree of this branch left 0 of 42 package directories with adist/.⇒ CI never reproduced this. The failure was invisible to CI and visible only on developer and agent machines — which is exactly why it survived, and why it was cheap in the way the card describes. The real gate,
pnpm check:sdui-registration-pins, runs in the other direction:performance-budget.yml:393-394, after "Build packages" (:177-179) and "Build Console" (:181-182), so it has always seen thedistspelling. That is the run the measurement above reproduces, and its verdict is unchanged.3. Verification — every reading names its build state
Command, once per state, from the repo root:
pnpm exec vitest run --maxWorkers=2 scripts/__tests__/. Post-fix readings are at04754509aon a clean tree.pnpm install; 0 of 42 package dirs withdist/distturbo run build --filter='./packages/*'(39 tasks) +--filter @object-ui/console build;packages/app-shell/distpresentpackages/plugin-gantt/distmoved aside (a non-app-shellpackage)check-readme-exports.test.ts×2check-sdui-registration-pinsappears 0 times in the failure log)The pre-fix red, on a real tree, byte-identical to the card:
The two reds still in the half-built row are not this diff's, and neither was touched:
check-readme-exports.test.ts×2 — that is finding(scripts): check-readme-exports.test.ts reds on a HALF-built tree — and another gate’s own printed build command is what produces one #7460's file and finding(scripts): check-readme-exports.test.ts reds on a HALF-built tree — and another gate’s own printed build command is what produces one #7460's documented half-built behaviour. It is red in that state before and after this diff, identically. Nothing here touches it, and making that row green is not this card's job.check-doc-links.test.ts×1 —Error: Test timed out in 15000ms, not an assertion failure. Re-run alone it passes 121/121. It is the shared-box flake AGENTS.md describes (an unbounded module load inside a bounded window); this diff changes two files it does not read.4. Lit control
Deliberately inverting the source-first partition to published-first, on the fully built tree, at
04754509a:1fa07fdeacedf8e9359f4c64d1842c92282adaae== on-disk before, both non-empty (an empty hash is read as failure, not as "nothing to compare").ba8756d0eba35bc1275b3825528117774e28d1d6— different, and the removed text greps to 0 while the injected text greps to 1. Mutation confirmed on disk before anything was read.'../check-sdui-registration-pins.mjs'by relative path, so the module under test is that source file and never resolves through a packageexportsmap or adist/.It lit, and each case named itself:
Restored under
trap '...' EXIT INT TERMwithgit checkout HEAD -- ABSOLUTE_PATH(absolute, resolved fromgit rev-parse --show-toplevel), and the restoration is proven by state, not by an exit code:5. Why the new cases use a fixture
The real-workspace assertion is the card's own symptom, and it is kept. But on an unbuilt checkout it passes for the wrong reason —
dist/simply is not there — so on its own it would still be a case whose meaning depends on the runner. The two new cases run against a fixture that owns its build state (fixture(chunks, { built: true })writes the published spelling too), so they are the same assertion over both build states and they keep asserting the preference on machines where nothing has been built. A third case pins the throw, and a fourth pins that the reordering is a permutation ofpkg.declared, so a partition that dropped an entry could not shrink the derived key set unnoticed.6. Gates
Run at
04754509aon a clean tree, exit code captured before any pipe:node scripts/check-changeset-presence.mjs✅ No source or published contract of a released package changed in this range, so no changeset is owed.(2 file(s) changed, 0 of them published source of a package the release covers, 0 changeset(s) added) ⇒ no changeset in this PRpnpm check:side-effects-arraypnpm check:sdui-registration-pinsHEAD:scripts/check-sdui-registration-pins.mjs=1fa07fdea; the consoledist/was removed afterwards to make the unbuilt row, so it was not re-run at that point)pnpm check:control-bytespnpm check:esm-specifiers/check:entry-guard/check:self-importnode scripts/check-governed-queue-guard.mjs --teston both paths✅ NOT GOVERNED⇒ ordinary PR routepnpm type-check:scriptstsc --listFilesnames both changed files, so this is a measurement rather than a green over an empty programeslinton both changed files,--no-inline-config --format jsonThe repo-wide lint scan was not run here; it is CI's, and this is a declared narrowing with its three legs: the two files are in eslint's own scope (they were linted, not reported as ignored); the count of 2 is read from
--format json, not guessed; andeslint.config.jsconfigures noprojectServiceand noparserOptions.project, so type-aware linting is off and this diff cannot move a verdict on a file it did not touch.Scope
Two files, both under
scripts/. Nopackages/*/srcfile was touched, so the Clause-② void condition did not fire..github/workflows/ci.ymlwas read only, never written.scripts/__tests__/check-readme-exports.test.ts(#7460) was not touched.🤖 Generated with Claude Code
https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Generated by Claude Code