ci: verify the built packages in the PR gate - #72
Conversation
- resolves every exports entry the way a consumer would (package self-reference from inside the package dir), naming what broke - asserts peers survive as imports in the built js or d.ts and that nothing undeclared is imported; a type-only peer counts via the declarations, caught red-first by the suite - per-entry 64 kB budget and a JSDoc-survival check on the declarations, the two defects a hand-run version of this check once found - discovers packages by exports map + build script and reports skips with their reason, so canton-connect joins automatically when the #47..#63 train merges (verified against refactor/70 in a worktree)
- job 2 runs check:shipped after build and knip, where dist exists - job 3 runs test:scripts (18 specs) before the workspace suites - root scripts slot into the existing pipeline order
- README's scripts table gains the test:scripts and check:shipped rows and the CI paragraph names the new steps - CLAUDE.md gains the Shipped output row, the CI row names the steps, and the validation checklist reproduces the full gate again
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
This PR closes #59 by adding a committed CI check that verifies what each package actually ships to consumers, rather than only checking src. A new Node script (scripts/check-shipped.mjs) walks every qualifying workspace package's exports map and, per entry, verifies: the target resolves via a real consumer-style self-reference, peer dependencies survive as import specifiers (not inlined), no undeclared bare imports leak in, a 64 kB per-entry budget holds, and JSDoc survives into the shipped .d.ts. It ships with an 18-spec node:test suite and is wired into the existing PR gate without renaming any jobs (preserving branch-protection required checks).
Changes:
- New
scripts/check-shipped.mjs(discovery + per-entry resolution/size/peer/JSDoc checks) with a red-firstcheck-shipped.test.mjssuite covering the exported helpers and per-failure-mode fixtures. - CI wiring:
check:shippedruns in job 2 afterbuild/knip(wheredistexists);test:scriptsruns in job 3 before the workspace suites. - Docs: README scripts table + CI paragraph and
CLAUDE.md(new "Shipped output" row and validation checklist) updated to describe the new steps.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/check-shipped.mjs |
New checker: workspace discovery, exports normalization, consumer-style resolution, size budget, peer/undeclared-import scan, JSDoc survival. |
scripts/check-shipped.test.mjs |
18 node:test specs: unit coverage for helpers + fixture-based integration per failure mode. |
package.json |
Adds test:scripts and check:shipped root scripts. |
.github/workflows/pr.yml |
Runs check:shipped in the build job and test:scripts in the test job. |
README.md |
Documents the two new scripts and updates the CI paragraph. |
CLAUDE.md |
Adds the "Shipped output" stack row and reproduces the full gate in the validation checklist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- the parser scanned to EOF and survived only because overrides/allowBuilds are mappings; a future top-level list would have read as package dirs, and a dropped dir shows in neither checked nor skipped - extracted pure as workspaceDirsOf and pinned by two specs, one feeding a representative file with comments, overrides, and a trailing list - raised by Copilot review on #72
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/check-shipped.mjs:287
- The direct-run guard uses
import.meta.main, which only exists in Node.js ≥ 24.2.0. The repo pins Node via.nvmrc(24) andengines(>=24), both of which permit 24.0.x/24.1.x. On those versionsimport.meta.mainisundefined, so runningnode scripts/check-shipped.mjs(e.g.pnpm check:shippedlocally) silently becomes a no-op and exits 0 without checking anything — the opposite of what this gate intends, and a failure the imported-in-tests path cannot catch. CI is safe today only becauseactions/setup-noderesolves.nvmrcto the newest 24.x. A version-agnostic entry check avoids the footgun. Optional.
if (import.meta.main) {
main()
}
|
|
||
| return readdirSync(distPath, { recursive: true, withFileTypes: true }) | ||
| .filter( | ||
| (dirent) => dirent.isFile() && (dirent.name.endsWith('.js') || dirent.name.endsWith('.d.ts')), |
There was a problem hiding this comment.
[High severity] distFilesOf scans .js and .d.ts together and pools the specifiers per package, so a peer surviving in the declarations satisfies the check for the runtime bundle. Both packages import react in their .d.ts (ReactNode, Ref); canton-connect likewise for dapp-sdk and core-types.
Verified against the real build: removed the react import from dist/index.js, replaced it with a bundled-copy shim, left dist/index.d.ts untouched. Result failures: [], and the report still printed externals: react.
The sabotage run in the PR body passed only because it rewrote every react import including the declarations, which a bundler regression would not do. This is acceptance criterion 2, and it is not enforced for the packages it covers.
| const importPath = join(pkgDir, importTarget) | ||
| if (existsSync(importPath)) { | ||
| const budgetKb = budgetOverrides[`${pkg} ${entry}`] ?? DEFAULT_BUDGET_KB | ||
| const sizeKb = statSync(importPath).size / 1024 |
There was a problem hiding this comment.
[High severity] The budget stats only the entry target, so it cannot backstop the peer check. tsdown already emits multi-file output for canton-connect, so a bundled dependency landing in a sibling chunk is invisible.
Verified against the real build: entry left at 10.0 kB, a 186 kB sibling chunk added and imported from it. Result failures: [], reported size 10.0 kB. Combined with distFilesOf pooling .d.ts specifiers, a bundled React ships with the gate fully green.
| export const bareImportsOf = (source) => { | ||
| const specifiers = new Set() | ||
| const patterns = [ | ||
| /^\s*(?:import|export)[^'"]*?from\s+['"]([^'"]+)['"]/gm, |
There was a problem hiding this comment.
[High severity] Both patterns are line-anchored and require whitespace after from, so the "no undeclared imports" guarantee is unsound. Verified misses: import{useState}from"react" (minified), await import('lodash'), require('lodash'), and any import that is not first on its line.
Two consequences: an undeclared package pulled in by a dynamic import passes silently, and if either build ever minifies, every required-peer assertion at line 212 flips to a false failure. Current output is unminified with no dynamic imports, so the branch is green today.
|
|
||
| // Every shipped condition must point at a file that exists ("development" ships nothing). | ||
| for (const [condition, target] of Object.entries(targets)) { | ||
| if (condition === 'development') { |
There was a problem hiding this comment.
[High severity] Skipping development means the check never validates the one condition that is broken in both packages it covers. Both declare "development": "./src/index.ts" first in their exports map, and files: ["dist"]. npm pack --dry-run on the built canton-dappbooster ships README.md, dist/index.d.ts, dist/index.js, package.json. No src/.
A consumer whose resolver enables the development condition resolves a file the tarball does not contain, and Vite enables it in dev (DEFAULT_CLIENT_CONDITIONS carries the development|production marker, verified in the installed Vite 8.2.0). The check reads the working tree, where src/ exists, so it cannot see this. Both packages are private: true today.
| } | ||
|
|
||
| const importTarget = targets.import ?? targets.default | ||
| if (typeof importTarget !== 'string') { |
There was a problem hiding this comment.
[Medium severity] targets.import is not necessarily a string. The nested shape {"import": {"types": "...", "default": "..."}} is standard and emitted by several bundlers, and it lands here as an object, failing with has no "import" or "default" condition.
Same for targets.types at line 178: nested, it is not a string, so docBlocks is undefined and the JSDoc assertion silently stops running. Neither package uses that shape today.
| @@ -0,0 +1,287 @@ | |||
| // Verifies what a consumer actually installs, per package: every exports entry resolves to a | |||
There was a problem hiding this comment.
[Medium severity] Neither new file is linted or formatted. biome.json's files.includes lists only canton-connect/**, canton-dappbooster/**, canton-theme/**, dapp/frontend/** and canton-barebones/**, so scripts/ falls outside it: pnpm lint on this branch reports Checked 144 files and touches neither of the 539 added lines. CLAUDE.md states lint and formatting are centralized in the root biome.json.
Note also that including scripts/ would surface check-shipped.mjs against the root useFilenamingConvention rule, which allows only camelCase and PascalCase.
| // Wholesale-stripped docs shipped once; declarations with exports but zero doc blocks fail. | ||
| const typesTarget = targets.types | ||
| const docBlocks = | ||
| typeof typesTarget === 'string' && existsSync(join(pkgDir, typesTarget)) |
There was a problem hiding this comment.
[Low severity] The JSDoc assertion only runs when types is present and points at an existing file. If a build stops emitting the types condition altogether, docBlocks is undefined, no failure is raised, and the run prints no d.ts as ordinary output. Declarations dropping out of the exports map is one of the regressions this check exists to catch.
| } | ||
|
|
||
| const keys = Object.keys(exportsField) | ||
| if (keys.every((key) => key.startsWith('.'))) { |
There was a problem hiding this comment.
[Low severity] every on an empty array returns true, so exports: {} passes through as zero entries. Combined with shouldCheck at line 39 accepting it, such a package counts toward checkedCount.packages, contributes no entries, and runs no per-entry assertion. The no package with an exports map and a build script was found guard does not fire.
|
|
||
| // Returns failure strings, each naming the entry and the problem; empty means the package ships. | ||
| // `entries` carries what was verified, so the caller can show its work. | ||
| export const checkPackage = (pkgDir, manifest, budgetOverrides = {}) => { |
There was a problem hiding this comment.
[Low severity] budgetOverrides is never passed. main calls checkPackage(join(root, dir), manifest) and no spec exercises the third parameter, so the 64 kB budget cannot be raised per entry without editing the script. Knip does not flag unused parameters.
| const resolved = resolveAsConsumer(pkgDir, specifier) | ||
| if (resolved === undefined) { | ||
| fail(entry, `"${specifier}" does not resolve from a consumer's import`) | ||
| } else if (!resolved.endsWith(importTarget.replace('./', '/'))) { |
There was a problem hiding this comment.
[Low severity] The assertion is a suffix match against the resolved file URL, so it cannot distinguish targets that share a tail: ./index.js and ./dist/index.js both satisfy a resolution ending in /index.js. It confirms the resolved path ends with the target, not that it is the target.
|
[Low severity] Two items in the PR description do not match the branch, neither anchored to a line:
|
Summary
Closes #59
Every existing gate runs against
src; nothing committed checked what a consumer actuallyinstalls. This adds the check, with its own red-first test suite: a committed script walks each
qualifying package's
exportsmap and fails CI when a built entry is broken, naming the entry andthe problem.
Changes
scripts/check-shipped.mjs(+18-spec node:test suite): walks each qualifying package'sexportsmap and verifies, per entry: resolution as a real consumer (package self-reference),peers surviving as import specifiers in the built
.jsor.d.ts(a type-only peer counts), noundeclared imports, a 64 kB per-entry budget, and JSDoc surviving into the declarations. Output
shows its work: per-entry target/size/doc-blocks, the externals, and every skipped package with
its reason.
check:shippedafter build and knip, wheredistexists; job 3 runstest:scriptsbefore the workspace suites. No job renames, so branch protection's requiredchecks are untouched.
test:scriptsandcheck:shippedrows and the CIparagraph names the new steps; CLAUDE.md gains a "Shipped output" row and its validation
checklist reproduces the full gate again.
Acceptance criteria
From #59:
A committed check covers the root entry and every declared subpath and fails when one does
not resolve. Deviation from the letter ("imports"): resolution plus static analysis, not
executing the bundle —
canton-connect's dist cannot execute outside a browser (the SDKgraph registers custom elements at import; recorded in feat(connect): add in-page wallet selection and remember the session wallet #63), so runtime verification stays
the harness's job.
Peers are not inlined: every required peer must survive as an import specifier, nothing
undeclared may be imported, and the issue's suggested size budget backs it up.
Runs in the PR gate, after
build.Covers
canton-connectandcanton-dappbooster: discovery is "has anexportsmap and abuildscript", so onmaintoday that'scanton-dappbooster, andcanton-connectjoinsautomatically when the feat: adopt the dapp-sdk facade in canton-connect #47…feat(connect): add in-page wallet selection and remember the session wallet #63 train merges — verified in a worktree at
refactor/70-provider-leaf-extraction(the current tip of the stack):Failure output names the entry point and what was wrong (transcript below).
Test plan
Automated tests
18 node:test specs behind
pnpm test:scripts, run as its own CI step: unit coverage for theexports normalization, specifier mapping and scanning, and the discovery rule, plus
fixture-package integration per failure mode. Red-first evidence: the type-only-peer spec was
written against the
.js-only scan and failed (a required types-only peer read as "inlined")before the fix extended the scan to the shipped declarations.
Full local gate green: lint, typecheck, build, workspace tests, script tests, knip,
check:shipped, lockfile untouched.Manual verification
Four sabotage runs against the real
canton-dappboosterbuild, each failing with a named entry:"import" points at ./dist/nope.js, which does not existpeer "react" never appears as an importexports symbols but carries no JSDoc78.3 kB, over the 64 kB budgetRestored build: green. On this branch the check reports
canton-dappboosterand lists every otherworkspace package as skipped with its reason (
no build script/no exports map) — nothing issilently omitted.
Breaking changes
None.
Checklist
Screenshots
None.