ci: add a verify-pack job that loads every exports target from the tarball - #427
ci: add a verify-pack job that loads every exports target from the tarball#427ianwremmel-ai-agent wants to merge 7 commits into
Conversation
Merge order: this needs #435 first. The two Merging #435 will not re-run this PR's checks on its own — #435 touches only Annotation triage. Of 21 distinct annotations, 18 are dismissed as unrelated to this diff:
The three that remain actionable are the Correction to my earlier note. I had called the Anything a package imports but does not declare falls through to the workspace's own |
|
View your CI Pipeline Execution ↗ for commit 8e2d038
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #427 +/- ##
=======================================
Coverage 80.41% 80.41%
=======================================
Files 23 23
Lines 730 730
Branches 182 182
=======================================
Hits 587 587
Misses 140 140
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Review triage, both passes. Acted on in 50cbe7b:
Verified on a synthetic workspace that each of the first three now fails where it previously reported Dismissed, with reasons:
|
|
CI status on 50cbe7b. Everything passes except two jobs, and neither is fixable inside this PR.
Neither package has a
This diff adds one workflow job and one Two things I deliberately did not do, both needing a call from you:
Happy to take either on as a separate ticket if you want it. |
…rball publint and attw read the exports map statically, so a condition that resolves to a file which is present but does not execute passes both. scripts/verify-pack.mjs packs each of the 32 packages under packages/@code-like-a-carpenter, extracts the tarball, and require()s the require target and import()s the import target of every exports entry out of the extracted directory. types and bin targets get an existence check. Loading from the tarball rather than the workspace is the point: a workspace symlink resolves files npm pack may not have included, which is how cli-core and cli-plugin-example came to declare a bin pointing at a cli.mjs that does not exist. npm pack needs a version, which these packages do not carry until multi-semantic-release supplies one at publish time, so each package is staged outside the workspace with a synthetic version before packing. Each extracted package gets a node_modules holding its declared dependencies, so a sibling resolves to its own extracted tarball rather than to the workspace source. Anything undeclared falls through to the workspace's node_modules; policing dependency declarations is tool-deps' job.
…lent passes
Two adversarial review passes found four ways the check reported ok for a
package it had not actually verified.
A target that calls process.exit() during evaluation ended the child, so
every target queued behind it in the same process was skipped and the zero
exit code read as success. A CLI whose "." export runs itself is exactly
that shape, which is why the previous version needed an allowlist to skip
@code-like-a-carpenter/cli. Each loadable target now gets its own child,
and success is a marker the child writes once the module has evaluated
rather than the exit code. A module that exits while evaluating still
executed, so the marker is written from an exit handler; only a module that
throws suppresses it. The allowlist is gone and cli's entry point is now
really loaded.
checkFor fell through to an existence check for any condition that was not
literally require or import, so a flat {types, default} map -- or a bare
string target -- got the publint-grade check this script exists to improve
on. It now picks a loader from the extension and the package's type field.
Subpath patterns are reported as unsupported instead of failing as a
missing file.
A sibling that failed to pack still got a symlink, and node walks past a
dangling link to the workspace's node_modules, where the scope is symlinked
back to the unpacked source. The dependent package then reported ok having
been verified against the source tree. Dependents of a failed package now
fail, and a link is only created when its target exists.
Nothing bounded the children. A module that left the event loop alive hung
the run to the job timeout -- CI already logged an orphaned node process.
Loading now stops the child as soon as the module evaluates, and every
spawn has a 60s kill.
Also: enumerate packages/*/* rather than one scope, skip directories with
no manifest, stop dependency resolution at the workspace root, do not treat
optionalDependencies as mandatory, catch errors from linking, guard the npm
pack JSON parse, decode child output per stream, fix a no-op sort that left
package order filesystem-dependent, move setup inside the try that cleans
up the temp tree, and annotate each failing target rather than only the
summary line.
Packing and loading now run concurrently, which takes the full check from
58s to 9s.
50cbe7b to
9e6f4ba
Compare
A cross-model review pass found three paths that reported a broken
package as `ok`.
The loader wrote its evaluated-marker from an `exit` handler whenever
loading had started without throwing, and the parent treated that marker
alone as a pass. A module calling `process.exit(1)` while evaluating, or
leaving a top-level `await` unsettled (node exits 13), produced the
marker and passed. The marker now also requires a zero exit code. A
module that evaluates and exits cleanly, including a CLI that runs
itself on import, still passes.
Fallback arrays in an `exports` map collected no targets at all, so
`{".": ["./missing.mjs"]}` passed vacuously. They are reported as
unsupported, which fails, rather than skipped.
A `package.json` that exists but does not parse hit the same `catch` as
a missing one and was dropped from the run, letting the job report that
every package it did look at was fine. A malformed manifest now aborts.
Staging flattened the scope into the directory name, mapping `@a/b_c`
and `@a_b/c` onto one directory. It keeps the scope as a directory now,
as the extract root already did.
Adversarial review of the checker found four gaps, each a case where a package that is actually broken is reported as loading cleanly. - A target that resolves outside the extracted directory is now a failure. `path.resolve(pkg.dir, target)` happily accepts an absolute path or one that climbs out with `..`, which lands back in the workspace source — the one place this job exists not to read. - JSON under an explicit `require` condition is now required rather than existence-checked. A file that is present but does not parse is the exact shape the job is for. JSON under any other condition stays an existence check: `import()` of JSON needs a type attribute, so loading every package's `"./package.json"` target would fail for the missing attribute alone. - Two directories claiming one package name is now an error. The map is keyed by name, so the second silently replaced the first and the run reported every package it did look at as passing. - The comments claimed the marker proves the module "has evaluated". It proves evaluation was entered and did not throw; a module that exits partway through still writes it. That is the deliberate price of writing it from an `exit` handler, so the comments now say so.
|
Codex review round, triaged. Eleven findings; four were real and are fixed in 96ccda8. Fixed
Each is covered by a synthetic workspace I ran the script against: a package whose target escapes the extract dir, one whose Dismissed
One caveat on process. Two adversarial passes are supposed to run on a non-Claude model. The spec-aware pass ran on Codex and produced everything above. The spec-blind pass hit a Codex usage limit mid-run and produced nothing. I pushed anyway because the change is four small, individually tested edits to a script that is not on |
…exity rule ESLint's `complexity` rule caps a function at 10; `checkFor` reached 12 and `checkPackage` 13 in 96ccda8, which is what failed the lint job. Both are decomposed rather than exempted. `checkFor` hands its extension and `type` fallback to a new `loaderFor`; `checkPackage` hands one target to `checkTarget` and the outcome of one child load to `loadProblem`, leaving it to collect what those return. No behavior change. A differential test over the 968 reachable combinations of conditions, target extension, `unsupported`, package `type`, exit code, stdout shape and timeout flag produces identical results before and after.
Two more adversarial review rounds on Codex, one spec-aware and one
spec-blind. Four findings were real, each a way a broken target or a broken
checkout is reported as clean.
- A module that fails on a later turn now fails. The child exited the instant
the loader returned, so a top-level `Promise.reject()` or a `setImmediate`
that throws never reached the event loop and the target passed. The child
now installs `unhandledRejection` and `uncaughtException` handlers and
defers its exit by one `setImmediate` — one turn, enough to see what is
already queued, and still bounded, so a module holding a socket open cannot
hang the job.
- Finding no packages is now an error. An empty discovery printed "All 0
packages load from their tarball" and exited 0, so a moved packages
directory or a broken checkout would have turned the job green.
- Only ENOENT means "not a package" when reading a manifest. Every other
error — a permission problem, an I/O error — was swallowed as a stray
directory, dropping a real package from the run while the job still passed.
- A non-string `bin` entry is reported rather than dropped, matching what
`collectTargets` already does with the export shapes it cannot check.
Verified against a synthetic workspace: packages whose entry point calls
`Promise.reject()` or `setImmediate(() => { throw })` passed before and fail
now; one that calls `process.exit(0)` mid-evaluation still passes, which is
the documented tradeoff of writing the marker from an `exit` handler. The
empty workspace exited 0 before and exits 1 now. Against the real 32
packages the result is unchanged — the same 30 pass and the same two fail on
the `bin` that #435 removes — so nothing here is a false positive.
Dismissed, with the evidence that settles each: no package declares
`peerDependencies`, `bundledDependencies`, or a `prepack`/`prepare` script;
every `bin` and `types` value is a string; the only JSON target is the
unconditional `"./package.json"`, never under an `import` condition; and the
only conditions in use are `types`, `default`, `require` and `import`, so
custom-condition resolution does not arise. Static `exports`-map validity —
a target that does not start with `./`, subpath and condition keys mixed —
stays publint's job under CLC-1134. A module that calls `process.exit(0)`
while evaluating still passes by design.
|
The spec-blind pass I owed has now run, and I re-ran the spec-aware one against the current diff. Both on Codex. Fourteen findings between them; four were real and are fixed in 8e2d038. The lint failure from 96ccda8 is fixed in 29b3e9e. Lint Not Prettier — two Fixed
Verified on a synthetic workspace: entry points calling Dismissed, with the evidence that settles each No package declares Static Still blocked on #435. The two |
|
Resumed pass, nothing to change. The two blockers are both still open and neither is in this diff. #435 has not landed. It is out of draft and mergeable, waiting on review. I confirmed its diff removes exactly the two fields
What unblocks this PR: review of #435, and whichever of #436/#437 lands the Parking this as blocked on you rather than re-checking a state only you can change. |
publintandattwread theexportsmap statically, so a condition that resolves to a file which is present but does not execute passes both of them. Nothing in this repo loads a packed tarball, so a package can ship an entry point that resolves and still throws for the first consumer who imports it.This adds
scripts/verify-pack.mjsand averify-packCI job that runs it. For each of the 32 packages underpackages/@code-like-a-carpenter/*it packs the package, extracts the tarball, and out of the extracted directoryrequire()s therequiretarget andimport()s theimporttarget of everyexportsentry.typesandbintargets get an existence check.Loading from the tarball rather than the workspace is the point: a workspace symlink resolves files
npm packmay not have included.Ticket: https://linear.app/code-like-a-carpenter/issue/CLC-1135/add-a-verify-pack-ci-job-that-loads-every-exports-target-from-the
It is red on main today
Two packages already fail, which is the job doing its job:
Neither package has a
cli.mjsat all. That is what CLC-1131 fixes, so this cannot go green until CLC-1131 lands.Scope note
CLC-1135 cites
tool-tool's CJS build as the motivating example. This job does not catch it. CLC-1133 records that the module still loads underrequire()and only a guarded branch is broken, so no load-time check reaches it. This job catches the class of bug one level out: a condition whose file is missing from the tarball, or present and non-loading.