[CEL-1660] Clean dist/ before build; guard against stale artifacts; add release tooling - #17
Conversation
…dd release tooling Fixes the bug found after PR #15 merged: `npm run build` (= `tsc`) never cleaned `dist/` first, and `prepublishOnly` ran only `build`. A stale local dist/ from before a source deletion survives tsc's incremental emit (it only overwrites/adds files for what's still in src/, never removes what isn't) and `index.js` no longer referencing a deleted file doesn't stop `files: ["dist", "!dist/canonical"]` from shipping it anyway, still reachable by a deep import. This is a general class, not a one-off: ANY file deletion from src/ survives into the tarball indefinitely unless dist/ is rebuilt from clean, and no existing gate catches it. Three changes: 1. `npm run clean` (node -e fs.rmSync, no new dependency, cross-platform) + `npm run build` now runs it first. `prepublishOnly` therefore can no longer ship a stale artifact through the normal `npm publish` path. 2. `scripts/verify-dist-no-orphans.mjs` + `npm run verify-dist` — a second, independent guard for anyone who builds outside the normal path (a bare `tsc`, a manually-touched dist/). Walks dist/, maps each .js/.d.ts back to its src/ .ts/.tsx (and .json files vendored via tsc's resolveJsonModule back to their src/ counterpart), and fails loudly listing every orphan if any file has none. Verified working: added a throwaway src/temp-orphan-probe.ts, built, deleted the source without rebuilding (reproducing the exact bug), confirmed the guard fails and names the two orphaned files, then rebuilt (which cleans dist/) to restore a clean, orphan-free state — nothing left over in this commit. Wired into `prepublishOnly` and `check-exports`. 3. New Makefile mirroring @cellarnode/ui's release-install/release-patch/ release-minor/release-major shape (install -> build -> npm version -> npm publish -> commit -> tag), git tag prefix `v` matching this repo's own existing tag (`v0.1.0`, not `ui-v...`). NOT run — this session has no npm publish auth for this package (a separate parked PR already hit ENEEDAUTH attempting exactly that). These targets make the eventual human-run release a single command instead of a hand-run sequence. Finding surfaced, not fixed here (out of this PR's three-item scope): `npx @arethetypeswrong/cli --pack .` exits non-zero on this package TODAY, independent of anything in this PR - verified on a clean checkout of main before touching anything. No CI job wires `check-exports` in, so nobody's noticed it fails outside a exit-code-masking pipe. `make build` and `release-*` therefore chain the new `verify-dist` guard directly, NOT the full `check-exports` (which still runs publint + arethetypeswrong for manual/informational use) - chaining the latter would make every release fail for a pre-existing, unrelated reason. Commented in the Makefile. Gates (direct exit codes, re-run after the final commit): `make build` (typecheck + test + compile-clean-first + verify-dist) = 0.
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 4/5
- In
scripts/verify-dist-no-orphans.mjs,walk(DIST)returns an empty list for an emptydist/, so the guard can report success after a clean or aborted build and allow incomplete artifacts through; fail validation whendist/contains no files. - The changes in
scripts/verify-dist-no-orphans.mjsdo not appear to cover the stated CEL-1660 requirements, including repointing 21 consumer call sites and removinguseBeverageLabelMapfrom the package; confirm the ticket scope or include the missing implementation before relying on this PR as its completion.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/verify-dist-no-orphans.mjs">
<violation number="1" location="scripts/verify-dist-no-orphans.mjs:3">
P3: This PR is filed under Linear CEL-1660 but does not implement that ticket's requirements: repoint 21 consumer call sites onto useReferenceData/useBeverageClassifications, delete useBeverageLabelMap from the package, bump consumers to a caret range, and add direct hook test coverage are all absent (the Vue/angular label-map hooks remain in src/). The diff is valid build-hygiene work (clean-first dist plus a stale-artifact guard) but is out of scope for CEL-1660; retitle/re-scope the PR or split it so the ticket association is accurate.</violation>
<violation number="2" location="scripts/verify-dist-no-orphans.mjs:92">
P3: On an empty or near-empty `dist/` the guard reports success. `walk(DIST)` returns `[]` when `dist/` exists with no files (e.g. after `make clean`, after a build that aborted between `clean` and `tsc`, or after any failure path that leaves `dist/` present but unpopulated), so `orphans.length === 0` and the script prints "OK — 0 dist artifact(s) checked" and exits 0. A stale or never-produced build therefore passes the very gate meant to fail loudly on a bad tree. Note the inconsistency: a *missing* `dist/` fails (lines 24-26) but an *empty* one succeeds. Since the script only ever counts files that already exist in `dist/`, it cannot distinguish "clean build" from "no build happened". Fail when `checked === 0` so an empty/partial tree is never reported as OK.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| process.exit(1); | ||
| } | ||
|
|
||
| console.log(`verify-dist-no-orphans: OK — ${checked} dist artifact(s) checked, 0 orphans.`); |
There was a problem hiding this comment.
P3: On an empty or near-empty dist/ the guard reports success. walk(DIST) returns [] when dist/ exists with no files (e.g. after make clean, after a build that aborted between clean and tsc, or after any failure path that leaves dist/ present but unpopulated), so orphans.length === 0 and the script prints "OK — 0 dist artifact(s) checked" and exits 0. A stale or never-produced build therefore passes the very gate meant to fail loudly on a bad tree. Note the inconsistency: a missing dist/ fails (lines 24-26) but an empty one succeeds. Since the script only ever counts files that already exist in dist/, it cannot distinguish "clean build" from "no build happened". Fail when checked === 0 so an empty/partial tree is never reported as OK.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify-dist-no-orphans.mjs, line 92:
<comment>On an empty or near-empty `dist/` the guard reports success. `walk(DIST)` returns `[]` when `dist/` exists with no files (e.g. after `make clean`, after a build that aborted between `clean` and `tsc`, or after any failure path that leaves `dist/` present but unpopulated), so `orphans.length === 0` and the script prints "OK — 0 dist artifact(s) checked" and exits 0. A stale or never-produced build therefore passes the very gate meant to fail loudly on a bad tree. Note the inconsistency: a *missing* `dist/` fails (lines 24-26) but an *empty* one succeeds. Since the script only ever counts files that already exist in `dist/`, it cannot distinguish "clean build" from "no build happened". Fail when `checked === 0` so an empty/partial tree is never reported as OK.</comment>
<file context>
@@ -0,0 +1,92 @@
+ process.exit(1);
+}
+
+console.log(`verify-dist-no-orphans: OK — ${checked} dist artifact(s) checked, 0 orphans.`);
</file context>
| console.log(`verify-dist-no-orphans: OK — ${checked} dist artifact(s) checked, 0 orphans.`); | |
| if (checked === 0) { | |
| console.error(`verify-dist-no-orphans: no artifacts found in "${DIST}/" — dist is empty. Run \`npm run build\` first.`); | |
| process.exit(1); | |
| } | |
| console.log(`verify-dist-no-orphans: OK — ${checked} dist artifact(s) checked, 0 orphans.`); |
| @@ -0,0 +1,92 @@ | |||
| #!/usr/bin/env node | |||
| /** | |||
| * CEL-1660 build-hygiene fix — `npm run build` did not clean `dist/` first, | |||
There was a problem hiding this comment.
P3: This PR is filed under Linear CEL-1660 but does not implement that ticket's requirements: repoint 21 consumer call sites onto useReferenceData/useBeverageClassifications, delete useBeverageLabelMap from the package, bump consumers to a caret range, and add direct hook test coverage are all absent (the Vue/angular label-map hooks remain in src/). The diff is valid build-hygiene work (clean-first dist plus a stale-artifact guard) but is out of scope for CEL-1660; retitle/re-scope the PR or split it so the ticket association is accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/verify-dist-no-orphans.mjs, line 3:
<comment>This PR is filed under Linear CEL-1660 but does not implement that ticket's requirements: repoint 21 consumer call sites onto useReferenceData/useBeverageClassifications, delete useBeverageLabelMap from the package, bump consumers to a caret range, and add direct hook test coverage are all absent (the Vue/angular label-map hooks remain in src/). The diff is valid build-hygiene work (clean-first dist plus a stale-artifact guard) but is out of scope for CEL-1660; retitle/re-scope the PR or split it so the ticket association is accurate.</comment>
<file context>
@@ -0,0 +1,92 @@
+#!/usr/bin/env node
+/**
+ * CEL-1660 build-hygiene fix — `npm run build` did not clean `dist/` first,
+ * and `prepublishOnly` ran only `build`. After the PR #15 removal merged,
+ * `dist/react/` still contained all four deleted `use-beverage-label-map.*`
</file context>
Summary
Fixes the build-hygiene bug found after PR #15 merged:
npm run build(tsc) never cleaneddist/first, andprepublishOnlyran onlybuild. A stale localdist/react/from before theuse-beverage-label-map.*deletion survived —tsc's incremental emit only adds/overwrites what's still insrc/, it never removes what isn't there anymore.index.jsno longer referencing the deleted file meant nothing resolved through the package entry point andcheck-exportsstayed green, butfiles: ["dist", "!dist/canonical"]meant a publish from that tree would ship the "removed" hook anyway, still reachable by a deep import (@cellarnode/beverage-utils/react/use-beverage-label-map). This is the general class, not a one-off: any file deletion fromsrc/survives into the tarball indefinitely unlessdist/is rebuilt from clean, and no existing gate catches it.Kept to the three things asked, nothing else:
1. Clean-first build
Cross-platform via Node's own
fsAPI (no new dependency — the repo had norimraf/similar already, checked first rather than assumed).prepublishOnlynow can't ship a stale artifact through the normalnpm publishpath, since it runsbuild.2. Regression guard:
scripts/verify-dist-no-orphans.mjs(npm run verify-dist)Second, independent guard for anyone who builds outside the normal path (a bare
tsc, a manually-toucheddist/). Walksdist/, maps each.js/.d.tsback to itssrc/.ts/.tsxcounterpart (and.jsonfiles vendored viatsc'sresolveJsonModule, e.g.dist/canonical/reference-data.json, back to theirsrc/counterpart), and fails loudly — naming every orphan — if any dist file has no corresponding source.Proved it works: added a throwaway
src/temp-orphan-probe.ts, built, deleted the source file without rebuilding (reproducing the exact bug), ranverify-dist— it correctly failed, listing both orphaned artifacts (dist/temp-orphan-probe.js,.d.ts). Rebuilt (which cleansdist/first) to restore a clean, orphan-free state. Nothing left over in this commit — confirmed viagit status --porcelainbefore staging.Wired into
prepublishOnlyandcheck-exports.3. Release tooling — new
MakefileMirrors
@cellarnode/ui'srelease-install/release-patch/release-minor/release-majorshape (install → build →npm version→npm publish→ commit → tag). Git tag prefix isv(matching this repo's own existing tag,v0.1.0— notui-v..., that's ui's own convention for a different reason).Not run. This session has no
npm publishauth for this package — a separately parked PR already hitENEEDAUTHattempting exactly that (per team-lead, deliberately not inspecting.npmrcsince that means reading credentials). These targets exist so the eventual human-run release is one command instead of a hand-run sequence, not so an agent can run them.Finding surfaced, not fixed here (out of this PR's three-item scope)
npx @arethetypeswrong/cli --pack .exits non-zero on this package today, independent of anything in this PR — verified on a clean checkout ofmainbefore touching anything (git stash listempty,git status --porcelainclean, ran it directly with no pipe masking the exit code:exit=1). No CI job wirescheck-exportsin at all, so nobody's noticed it fails outside a pipe that swallows the exit code — including me, on PR #15: I reported "check-exports exit=0" there, which was wrong, caught by... | tail -N; echo $?capturingtail's exit rather than the actual command's. Correcting that here.Because of this,
make buildandrelease-*chain the newverify-distguard directly, not the fullcheck-exportsscript (which still runspublint+arethetypeswrongfor manual/informational use, unchanged) — chaining the latter into the release path would make every release fail for a pre-existing, unrelated reason (no CJS-compatible fallback for the./react/./vue/./angularsubpath exports). Commented inline in the Makefile. This is a real gap worth a separate ticket if the team wants node10/CJS consumers supported, but it's not this PR's problem to solve.Gates (direct exit codes, no masking pipe this time)
make build(typecheck + test + compile-clean-first + verify-dist) = 0. Re-run and confirmed clean after the final commit.Do not merge. Linear untouched.
Summary by cubic
Fixes the stale-
dist/bug where deletedsrc/files could still ship in the npm tarball via deep imports, sincetscnever removes output for sources that no longer exist.npm run buildnow cleansdist/first, and a new guard catches stale artifacts for anyone building outside the normal path.Bug Fixes
cleanscript removesdist/via Node'sfs.rmSync(cross-platform, no new dependency);buildruns it beforetsc.verify-distscript fails loudly, naming every orphan, when anydist/file has no matchingsrc/source; wired intoprepublishOnlyandcheck-exports.New Features
Makefileaddsrelease-install/release-patch/release-minor/release-majortargets mirroring@cellarnode/ui, withvtag prefix.verify-distdirectly rather thancheck-exports, because@arethetypeswrong/cliexits non-zero on this package today — a pre-existing issue unrelated to this PR.Written for commit 80c3243. Summary will update on new commits.