Sync the builder with the sibling packages - #36
Conversation
Bring the plugin to the form the sibling builders share: the optional filter argument alongside the doc comment already in place. This repo keeps calling it with no argument, so the logged output is unchanged. Co-authored-by: Claude <noreply@anthropic.com>
The target name was written out twice -- once as the rule in builder/Makefile, once as the argument that calls it -- so the two spellings could drift apart. Copying the declaration line keeps them identical by construction. The value is a target name in builder/, which is why it is not a path relative to this directory. Co-authored-by: Claude <noreply@anthropic.com>
npm reports an engines mismatch as a warning and still exits 0, so a dependency that cannot run on a line the package claims to support passes CI unnoticed. --engine-strict turns that into an error, and it reaches transitive dependencies as well. Co-authored-by: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The recipe branched on whether a package had test-only peers to declare: some copies carried the cp line, some did not. Shipping the file everywhere -- empty where nothing is needed -- puts every unpack on the same two lines, and states that this lane needs nothing rather than leaving it to be inferred from a missing file. Adding a peer later is then a line of JSON instead of a line of JSON and a line of make. Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
There was a problem hiding this comment.
🟡 Changes recommended
The changes touch the release/CI packing pipeline (Makefile shell logic and --engine-strict engine enforcement), which warrants human confirmation of matrix behavior before approval despite no defects being found.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR syncs the builder/ tooling with sibling packages, bringing three small build-infrastructure changes into alignment. It touches only build/CI scaffolding (a rollup logging plugin and the pack-lane Makefile/manifest), with no impact on the library's runtime code or public API.
Changes:
show-files.tsgains an optional filter argument (test?: {test: (path: string) => boolean}); all in-repo callers invoke it with no argument, so logged output is unchanged.- The builder test-bundle target name is extracted into the
BUILDER_TEST_DSTvariable so the two Makefiles can't drift, and the pack lane gains its own (empty)package.jsonplus--engine-stricton the tarball install to turn engine mismatches into hard errors.
File summaries
| File | Description |
|---|---|
builder/show-files.ts |
Adds an optional RegExp-like filter to the rollup import-logging plugin; early-returns when a module is filtered out. Backward compatible with existing zero-arg callers. |
builder/pack/package.json |
New minimal manifest ({"devDependencies": {}}) copied into a bare lane by unpack to host test-only peers without self-referencing the package. |
builder/pack/Makefile |
Introduces BUILDER_TEST_DST to mirror builder/Makefile, conditionally seeds package.json in the bare lane, and adds --engine-strict to the lane install. |
I verified: the BUILDER_TEST_DST target (./tests/bundled.mjs) exists in builder/Makefile:108 and resolves correctly under $(MAKE) -C ..; the cd ../.. && [ -e package.json ] || cp ... recipe copies into the repo root correctly under both checkout and bare-lane conditions; and --engine-strict is safe against the declared engines (node >=18.8.0) across the CI matrix (18–26).
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three builder changes the sibling packages already carry.
show-files.tstakes the shared form: the optional filter argument alongside the doc comment already in place. This repo calls it with no argument, so the logged output is unchanged.builder/Makefile, the argument inbuilder/pack/Makefile— so the two spellings could drift. Copying the declaration line keeps them identical by construction.--engine-stricton the lane install. npm reports an engines mismatch as a warning and still exits 0, so a dependency that cannot run on a supported line would pass CI unnoticed. The flag makes it an error, and it reaches transitive dependencies too.unpackon the same two lines, and adding a peer later is a line of JSON rather than a line of JSON and a line of make.Verified
tsc --noEmit, format/imports checks — green.pack→bundled→test-dist(36/36 plus the IIFE check), thenunpack→teston the installed copy, leavinggit statusuntouched.The lane runs fewer suites than the checkout because the compat and benchmark suites stay source-only, which is unchanged here.
🤖 Generated with Claude Code