Repository navigation
ci(root): derive the packed workspace set instead of listing it - #835
Conversation
The scaffold's workspace legs packed a hand-written list of nine packages. nextly and plugin-sdk both depend on @nextlyhq/blocks-engine, which was not on it, so pnpm pack rewrote that workspace:* to a concrete version and the install resolved it FROM THE REGISTRY. Two consequences, and the second is the one that was invisible. On an ordinary pull request those legs tested the PUBLISHED blocks-engine while reporting on the workspace, so a change to it could break a scaffold and merge green. On the Changesets version pull request the bumped version does not exist yet, and the same gap fails outright with ETARGET -- making a check that can never pass on a release, which is how a merge policy of 'CI fully green' teaches people to wave red through. The list is now derived from the workspace: every non-private package under packages/, minus create-nextly-app, which is the scaffolder rather than a dependency of what it scaffolds. Nine packages were missing, not one -- blocks-engine, blocks-react, builder, plugin-page-builder, plugin-seo, admin-css and all three storage adapters. pin-workspace-packages.mjs already states this rule about its own names: a list is a second answer to a question the manifests answer, and the two agree only until someone adds a package. The workflow was breaking it one layer up. The count control is derived from the same list rather than a literal -ge 9, which is what let the gap hide: nine tarballs satisfied it while the tenth package was never packed. The derivation refuses rather than returning empty -- verified by exit code.
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 297985fdab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The dist assertion assumed every publishable package builds one. @nextlyhq/admin-css has no build script and publishes src and bin, so its tarball contains zero package/dist/ entries and the check exits 1 every time. Scaffold blog-code-first failed on exactly that. Same mistake as the hardcoded package list this step had just stopped making, one level down: a literal standing in for something the manifests already answer. The expectation is now read from each package's own files[], and negation entries are skipped because they subtract from a set rather than naming something that must be present. A package declaring no files[] refuses rather than passing unverified. Tarballs are matched to packages by the name inside them rather than by filename, since a scope becomes a dash and @nextlyhq/ui and nextlyhq-ui differ. Controls, on a real pnpm pack of admin-css: 5 entries under package/src against the derived expectation, 0 under package/dist against the old one.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Handover for whoever owns this workflow — posting here because it is the channel that reaches you reliably; I misaddressed a socket message and it went to the wrong lane. Your fix worked, measured on your merge. My follow-up for the second cause failed, and its own CI is the evidence. #839 fixed the remaining leg — Mechanism: my change added Four rounds, and rounds 2 to 4 were one defect: adding a root dependency to satisfy a peer cannot avoid changing the dependency boundary, and the scaffold legs exist to test that boundary. Optional adapters, then peers of packages the app never installs, then The replacement design is written up and it touches THIS file, which is why I am handing it over rather than starting: Shape: stop writing — the package is present at the exact version and the peer is still unmet. A registry keeps ranges semver, so they resolve as they would for a real user, and the scaffolded manifest keeps exactly what the generator wrote. The property to judge any fix on, and the thing most likely to be forgotten: only the two blog legs declare The design is unclaimed. Take it if you want it, or say so and I will pick it up once the founder decides on #839 — either way I will not touch this workflow without coordinating first. |
1 participant
What was wrong
The scaffold's
core: workspacelegs packed a hand-written list of nine packages.nextlyandplugin-sdkboth depend on@nextlyhq/blocks-engine, which was not on it — sopnpm packrewrote thatworkspace:*to a concrete version and the install resolved it from the registry.Nine packages were missing, not one:
blocks-engine,blocks-react,builder,plugin-page-builder,plugin-seo,admin-css, and all threestorage-*.The failure a reviewer can reproduce today
Workspace
nextlyinstalls beside registry@nextlyhq/blocks-engine@0.0.2-alpha.42, andnextlyimports a subpath that copy does not export:mainis self-consistent — the import and the export both arrived in2f3bb5767(#738). Only the packed/registry skew breaks it.Why most of the matrix stayed green — the separating property
Only the two blog legs declare
core: workspace. The fourblanklegs never mix registry and workspace packages at all, so a greenblankproves nothing about this class of defect. That is why a nine-package list drifted for as long as it did while the matrix looked healthy.The second, release-only symptom
On the Changesets Version PR the bumped version does not exist yet, so the same gap fails outright:
That is a check that can never pass on a release, on every release, forever — while the repository's merge policy is "CI fully green". It is currently sitting on #744, whose merge drains the changeset backlog. This removes that particular red; I am not claiming it is the only one on #744.
The fix
The packed set is derived from the workspace: every non-private package under
packages/, minuscreate-nextly-app, which is the scaffolder rather than a dependency of what it scaffolds. The same derived list drives the turbo build filter, the pack loop, and the count control.pin-workspace-packages.mjsalready states this rule about its own names:The workflow was breaking that rule one layer up.
Two literals removed, not one
The count control was
[ "$count" -ge 9 ]— that literal is what let the gap hide, since nine tarballs satisfied it while the tenth was never packed. It is now derived and asserts equality.The content control asserted
package/dist/on every tarball.@nextlyhq/admin-csshas no build script and publishessrcandbin, so that assertion failed it forever — the same mistake one level down, caught by Codex and by this PR's own matrix within one round. The expectation is now read from each manifest'sfiles[]; negation entries (ui's!dist/metafile-*.json) are skipped, and a package declaring nofiles[]refuses rather than passing unverified.Verification
blocks-engineincludednextly/plugin-sdkdepend onblocks-enginenextlyimports@nextlyhq/blocks-engine/formatblock-document.ts:56admin-csstarball vs derivedpackage/srcadmin-csstarball vs oldpackage/dist/The refusal control was re-run without a pipe:
tailhad been reporting its own exit status, which is the false-clean shape.claude/rules/reading-a-ci-verdict.mdwarns about.This PR is self-testing — it edits
scaffold-build.yml, which is in that workflow's ownpathstrigger, so the matrix runs against the change that alters it.Corroboration
The
blocks-engineskew was diagnosed independently by another session from a different failing leg, on run31865420109, reaching the same cause by a different route. The separating property above is theirs.A SECOND, independent cause — measured after merge, corrected
Scaffold blog-visual(pnpm) still fails whileblog-code-first(npm) now passes. That split is the evidence: this PR fixed cause 1; cause 2 is independent of it.The mechanism first recorded here was wrong and is corrected rather than quietly edited. This section originally said the pin script's
overrides"may not reach peer ranges". The opposite is true, and it was settled by running it (run31866038617) rather than by reading the diff:The install log is the line neither theory could be talked into:
The package is present at the exact version and the peer is still unmet. Under the original theory that line cannot occur. Both theories predicted the same failing import, so only execution separated them.
pin-workspace-packages.mjsnever mentionspeerDependenciesand does not need to — writing the override is enough. The follow-up therefore lives there (and possibly in the scaffolded manifest'speerDependencyRules), not in this workflow. Diagnosed and owned by another session; full write-up attasks/left-tasks/2026-08-15-1500-blog-scaffold-broken-by-a-hand-listed-pack-set.md.Publishing is unaffected:
npm view @nextlyhq/plugin-form-builder peerDependenciesshows a correctly rewritten0.0.2-alpha.42. Harness defect, not shipped.Scope
CI-only, so no changeset per the repo rule.