Repository navigation
fix: retain third-party license notices in distributions - #4070
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
I reviewed 7410a5b and found no problems that must change in the code. Not blocking, and fine to take or leave: sort the dependencies by Three open threads from cubic-dev-ai still apply: the missing Checks are still running. Smoke Tests is pending, and it runs I did not build or run anything. I did not observe the plugin's ordering, its cwd exclusion, or its use of Before merge, Smoke Tests must pass. Then add the d.mts declaration, use one output-directory constant in The newer head c803382 deduplicates the license texts in the notice plugin and its test, which covers the last optional note above. It does not touch |
|
The code at c803382 looks good apart from the two open review threads below, which still need a fix or an answer. All 35 checks pass, including check:tooling, which runs the build and check:package, the route this diff changes. There are no conflicts. Not blocking, and you can take or leave these: sort a copy of The Cubic threads on the missing d.mts declaration (#4070 (comment)) and the duplicated dist/src path (#4070 (comment)) still apply, and one lower-priority thread on entryCount also still applies. Please fix or answer them before merge. I did not run the build or the tests myself. |
c803382 to
b90d5ec
Compare
|
Addressed the three Cubic threads and the inexpensive follow-ups from the latest thymikee review.
Regression evidence against rebased pre-fix
The declaration-generation test confirms emitted Rebased the three existing commits onto Validation at b90d5ec:
|
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/__tests__/bundled-license-notices.test.ts">
<violation number="1" location="scripts/__tests__/bundled-license-notices.test.ts:163">
P3: This test never checks that the license text reaches the notice file. Add an assertion for the fixture's LICENSE text so a dropped license body fails here.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 3 new · 2 still open · 6 no longer raised 5 finding(s): 1 minor, 4 nit. Verdict: approve. 🟡 Minor (1)
🔵 Nit (4)
Reply under an inline comment with Earlier reviewsReview of
|
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:6 |
🆕 bundledLicenseNotices keeps source in one closure shared by the JS and DTS builds of one config |
scripts/lib/cli-entrypoint.ts:65 |
🆕 runEntrypoint turns any stdout/stderr flush error into exit 1, including a nonzero code main chose |
🔵 Nit (6)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:43 |
🆕 runEntrypoint's stdout diagnostic is dropped silently when stderr cannot accept it |
scripts/lib/bundled-license-notices.ts:67 |
The jpeg-js special case is hard-coded inline in the notice renderer |
scripts/lib/bundled-license-notices.ts:75 |
🆕 jpeg-js version coupling is one-way: bumping only the repo copy never fails until upstream version changes |
scripts/lib/bundled-license-notices.ts:88 |
The renderer's license-section layout is written and parsed in two places with no shared owner |
scripts/lib/bundled-license-notices.ts:90 |
🆕 renderNotices trusts plugin-reported author/license text, so a multi-line field breaks the attribution grammar the pack gate reads |
scripts/size-report-package.mjs:189 |
🆕 Missing-notices error says "npm pack" even when a different tree was checked |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 53539d31 · 03.11.2026 22:46 UTC
Apex Sentinel
Verdict: approve
Since the last review: 2 new · 1 still open · 7 no longer raised
3 finding(s): 3 nit. Verdict: approve.
🔵 Nit (3)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:74 |
renderNotices hard-codes jpeg-js and its notice-file first-line format |
scripts/size-report-package.mjs:108 |
🆕 readAttribution can read an attribution line out of a license body and count it as a real attribution |
scripts/lib/cli-entrypoint.ts:63 |
🆕 runEntrypoint names one stream's error after the other stream's drain |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 44b59309 · 03.11.2026 21:46 UTC
Apex Sentinel
Verdict: approve
Since the last review: 8 new · 0 still open · 5 no longer raised
8 finding(s): 8 nit. Verdict: approve.
🔵 Nit (8)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:28 |
🆕 The notices file name is duplicated between the bundler's emit and the publish gate's constant |
scripts/lib/bundled-license-notices.ts:42 |
🆕 Version is trimmed to validate but emitted raw, producing an attribution line the pack gate cannot parse |
scripts/lib/bundled-license-notices.ts:75 |
🆕 jpeg-js version coupling between the shipped LICENSES file and a version range is implicit |
scripts/lib/bundled-license-notices.ts:78 |
🆕 jpeg-js source-notice path and version pattern are hard-coded at their point of use |
scripts/lib/cli-entrypoint.ts:3 |
🆕 isClosedPipe narrows with error.code after a in check instead of a cast |
scripts/lib/cli-entrypoint.ts:12 |
🆕 drainOutput leaves a live 'error' listener and a pending empty write on a stream that already finished |
scripts/lib/cli-entrypoint.ts:73 |
🆕 A write-error diagnostic written after the drain timeout is discarded by the immediate process.exit |
scripts/size-report-package.mjs:103 |
🆕 readAttribution cannot match an attribution line whose license expression contains a parenthesis, so the gate reports 0 attributions and blocks the release |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of a5c849d6 · 03.11.2026 20:48 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 2 still open · 4 no longer raised
5 finding(s): 5 nit. Verdict: approve.
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/check-package.ts:104 |
🆕 check-package reads only packed.filename from parseNpmPackOutput, so the new aggregate-size validation never runs in the publish gate |
scripts/lib/bundled-license-notices.ts:18 |
Notice emission depends on rollup-plugin-license internals under a caret range |
scripts/lib/bundled-license-notices.ts:94 |
🆕 renderNotices output has no trailing newline, unlike the zero-dependency placeholder |
scripts/lib/cli-entrypoint.ts:79 |
🆕 A throwing stderr write on the main() failure path escapes as an unhandled rejection |
scripts/size-report-package.mjs:104 |
assertBundledLicenseNotices hard-codes 'npm pack' in the manifest failure, ignoring the caller's context |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 0fe4cbdd · 03.11.2026 17:20 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 3 still open · 2 no longer raised
6 finding(s): 1 minor, 5 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:18 |
🆕 Build silently depends on rollup-plugin-license 3.7.1 internals: synchronous thirdParty.output and hook object shape |
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:28 |
Duplicated 'THIRD_PARTY_NOTICES.txt' file name in emitter and pack assertion |
scripts/lib/bundled-license-notices.ts:53 |
renderNotices reports a missing version as "missing its license or license text" |
scripts/lib/bundled-license-notices.ts:60 |
Curated jpeg-js notice check is a hard-coded special case that cannot satisfy multipleVersions: true |
scripts/lib/cli-entrypoint.ts:56 |
🆕 Shadowed error in the reportStdoutFailure catch discards the original failure |
scripts/size-report-package.mjs:68 |
🆕 Pack gate passes on the "no third-party dependencies are bundled" placeholder |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 63a8a9a1 · 03.11.2026 17:07 UTC
Apex Sentinel
Verdict: approve
Since the last review: 5 new · 0 still open · 4 no longer raised
5 finding(s): 5 nit. Verdict: approve.
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:12 |
🆕 drainOutput leaves its 'error' listener attached and never cancels the in-flight empty write |
scripts/lib/bundled-license-notices.ts:18 |
🆕 dts rebuild rewrites THIRD_PARTY_NOTICES.txt with a byte-identical file every build |
scripts/lib/bundled-license-notices.ts:29 |
🆕 The notices file name 'THIRD_PARTY_NOTICES.txt' is hard-coded on line 29 here, and the publish gate asserts on the … |
scripts/lib/bundled-license-notices.ts:58 |
🆕 renderNotices rejects a missing license or text but not a missing version |
scripts/lib/bundled-license-notices.ts:63 |
🆕 jpeg-js attribution rules are hard-coded inline in renderNotices |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 4e6e8d95 · 03.11.2026 16:42 UTC
Apex Sentinel
Verdict: approve
Since the last review: 2 new · 2 still open · 4 no longer raised
4 finding(s): 1 minor, 3 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/size-report-package.mjs:169 |
🆕 parseNpmPackOutput validates file entries but not the size fields the size report consumes |
🔵 Nit (3)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:61 |
🆕 jpeg-js version gate only checks the first line, so a bumped jpeg-js whose notice text is stale still passes |
scripts/lib/cli-entrypoint.ts:34 |
runEntrypoint keeps a permanent 'error' listener on process.stdout/stderr that can mask the drain failure with a later one |
scripts/lib/cli-entrypoint.ts:46 |
stdout flush failure diagnostic is silently dropped when stderr is also failing |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 4348d35b · 03.11.2026 16:14 UTC
Apex Sentinel
Verdict: approve
Since the last review: 2 new · 4 still open · 8 no longer raised
6 finding(s): 1 minor, 5 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:7 |
Sticky stream.errored turns a mid-run transient output error into a failed exit code |
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/size-report-package.mjs:68 |
🆕 The emitted asset name lives in two hard-coded places with nothing linking them: bundled-license-notices.ts:28 emits … |
scripts/lib/bundled-license-notices.ts:19 |
generateBundle override discards rollup-plugin-license's hook-object ordering metadata |
scripts/lib/bundled-license-notices.ts:60 |
bundled-license-notices.ts:60-73 puts one curated package's rules inside the generic renderer: the literal package na… |
scripts/lib/bundled-license-notices.ts:61 |
🆕 jpeg-js license pairing is duplicated by hand between the fixture file and its own guard |
scripts/lib/cli-entrypoint.ts:36 |
Permanent no-op 'error' listeners on process.stdout/stderr mask stream errors if main() never settles |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 4cad2690 · 03.11.2026 15:19 UTC
Apex Sentinel
Verdict: approve
Since the last review: 9 new · 3 still open · 3 no longer raised
12 finding(s): 2 minor, 10 nit. Verdict: approve.
🟡 Minor (2)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:13 |
🆕 exitAfterOutput can hang instead of exiting when the write callback never settles |
scripts/lib/cli-entrypoint.ts:33 |
The new drain turns a real write error (EIO, ENOSPC on a redirected file, EBADF) into exit code 1 without saying so: … |
🔵 Nit (10)
| Where | Finding |
|---|---|
scripts/check-package.ts:245 |
🆕 assertBundledLicenseNotices is called twice on the same entries in check-package.ts |
scripts/lib/bundled-license-notices.ts:17 |
🆕 Comment about the plugin's synchronous callback sits on the wrong statement |
scripts/lib/bundled-license-notices.ts:19 |
Wrapper around rollup-plugin-license silently drops its hook metadata and can collide on the emitted asset name |
scripts/lib/bundled-license-notices.ts:48 |
🆕 Dependency id sort recomputes its key on every comparison instead of a precomputed sort key |
scripts/lib/bundled-license-notices.ts:61 |
🆕 jpeg-js source-file notices are not checked against the bundled sources, so attribution can be silently incomplete or stale |
scripts/lib/bundled-license-notices.ts:63 |
jpeg-js source-notice special case is hard-coded inside the shared renderer |
scripts/lib/cli-entrypoint.ts:1 |
🆕 runEntrypoint treats every ERR_STREAM_DESTROYED as a benign closed pipe, hiding real post-destroy write errors |
scripts/lib/cli-entrypoint.ts:10 |
🆕 Comment line removed from runEntrypoint separated the doc from the function it documents |
scripts/size-report-package.d.mts:5 |
🆕 assertBundledLicenseNotices is stated in the .d.mts but absent from the .d.mts-linked runtime's own type contract — the declared API is exported, the type file declares it without asserting the linked runtime actually provides it |
scripts/size-report-package.d.mts:17 |
🆕 parseNpmPackOutput became exported (i.e. public to check-package.ts) and now returns { ...pack, files } — every… |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 02ff19e5 · 03.11.2026 12:43 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 3 still open · 6 no longer raised
6 finding(s): 1 minor, 5 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:16 |
Drained output write errors are now silent — runEntrypoint's prefix line is unreachable for them |
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:18 |
🆕 Notice emitter depends on rollup-plugin-license's internal hook shape and hook order |
scripts/lib/bundled-license-notices.ts:25 |
THIRD_PARTY_NOTICES filename duplicated between plugin and pack guard |
scripts/lib/bundled-license-notices.ts:54 |
jpeg-js extra-notice handling is hard-coded in a generic loop |
scripts/lib/bundled-license-notices.ts:58 |
🆕 jpeg-js source-file notice version guard passes on a blank first line when the file starts with CRLF |
scripts/size-report-package.mjs:68 |
🆕 size-report base-measurement set is fragile: assertBundledLicenseNotices was added to the size-report-package.mjs module, not just the policy function |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 74a7b5b4 · 03.11.2026 10:25 UTC
Apex Sentinel
Verdict: approve
Since the last review: 5 new · 4 still open · 1 no longer raised
9 finding(s): 2 minor, 7 nit. Verdict: approve.
🟡 Minor (2)
| Where | Finding |
|---|---|
scripts/check-package.ts:95 |
🆕 check-package.ts reads npm's raw size field without the validation the same file's owner applies elsewhere |
scripts/lib/cli-entrypoint.ts:5 |
Flush swallows every stream error, not just the closed-pipe ones |
🔵 Nit (7)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:5 |
🆕 The exit flush resolves immediately on an empty write, so it does not prove the piped output was handed off |
scripts/check-package.ts:34 |
check-package.ts imports the assert and its type from a size-report module whose shipped-imports guard forbids |
scripts/lib/bundled-license-notices.ts:5 |
🆕 bundledLicenseNotices has no return type and no doc comment |
scripts/lib/bundled-license-notices.ts:14 |
🆕 Notice file has no trailing newline except in the no-dependencies case |
scripts/lib/bundled-license-notices.ts:18 |
THIRD_PARTY_NOTICES.txt path is spelled out in four places with no shared owner |
scripts/lib/bundled-license-notices.ts:40 |
🆕 The license check only hashes the first line of LICENSES/jpeg-js.txt, so a stale body or a missing Apache text still ships |
scripts/lib/bundled-license-notices.ts:40 |
jpeg-js is special-cased with hard-coded names and paths inside the generic notice plugin |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of 05a76ca8 · 03.11.2026 09:44 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 2 still open · 2 no longer raised
5 finding(s): 1 minor, 4 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/lib/cli-entrypoint.ts:5 |
🆕 runEntrypoint: a failed flush now rejects unhandled instead of reporting one prefixed line |
🔵 Nit (4)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:13 |
The notices filename and its packed path are duplicated across two owners |
scripts/size-report-package.mjs:68 |
New pack gate is satisfied by the "No third-party dependencies are bundled." placeholder |
scripts/check-package.ts:103 |
🆕 packTarball now asserts publish contents as a side effect, duplicating the check in assertPublishPackageContents |
scripts/lib/bundled-license-notices.ts:38 |
🆕 The jpeg-js source-notice rule is hardcoded inside the generic notices plugin |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of fbd18153 · 03.11.2026 08:39 UTC
Apex Sentinel
Verdict: approve
Since the last review: 2 new · 2 still open · 3 no longer raised
4 finding(s): 1 minor, 3 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:15 |
Empty bundle writes a non-empty notices file, so the pack gate accepts a build with zero notices |
🔵 Nit (3)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:12 |
Notices filename and path are spelled in three places that must drift together |
scripts/lib/bundled-license-notices.ts:32 |
🆕 Missing-license error reports an unknown version and hides which of the two checks failed |
scripts/lib/bundled-license-notices.ts:40 |
🆕 Missing LICENSES/ files surface as a raw ENOENT instead of the actionable refresh message |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
Review of b90d5ec0 · 03.11.2026 08:06 UTC
Apex Sentinel
Verdict: approve
5 finding(s): 5 nit. Verdict: approve.
🔵 Nit (5)
| Where | Finding |
|---|---|
scripts/lib/bundled-license-notices.ts:15 |
The "no dependencies bundled" placeholder passes the non-empty notices gate |
scripts/lib/bundled-license-notices.ts:26 |
Notice ordering uses localeCompare, so the generated file is not byte-stable across ICU locales |
scripts/lib/bundled-license-notices.ts:37 |
jpeg-js notice file hardcodes version 0.4.4 with nothing tying it to the installed dependency |
scripts/lib/bundled-license-notices.ts:37 |
jpeg-js is hard-coded inside the generic license plugin |
scripts/size-report-package.mjs:69 |
The notices path is spelled out in three places |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on b90d5ec. The verdict and the full list are in the summary comment.
|
Pushed fbd1815 for the final bot round.
Coverage diagnosis: the successful exact-base main step took 11m27s. The b90d5ec attempts failed after 10m47s / 9m06s, before the 30-minute timeout. The one retry reports Fresh-head CI/bot reviews are pending. Later nits receive replies without another push. No labels or merge. |
There was a problem hiding this comment.
Apex Sentinel: 2 new findings on fbd1815. The verdict and the full list are in the summary comment.
|
Coverage follow-up at 05a76ca: Confirmed reporting defect: The license tests now invoke tsdown in a child process. Native bundler/compiler state stays outside the instrumented Vitest worker; failed builds propagate their nonzero status and stderr. The existing minification, exclusion, deduplication, declarations, and jpeg-js mismatch assertions remain. Reproductions used Validation at this exact head:
Cold diff review completed; no independent reviewer delegated from this implementation child. The disposable checkout was removed. Fresh hosted Coverage is still required. No CI reruns, labels, or merge. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 2 new findings on 05a76ca. The verdict and the full list are in the summary comment.
|
Follow-up at 74a7b5b:
Slow-test investigation: the complete 05a76ca Coverage job log has no
All seven tests passed. That deliberately partial coverage invocation exited 1 on repository-wide coverage floors, not a failed or slow test. The CI timing hypothesis remains unconfirmed. No slow-test budget, pin, or allowlist changed. Cold diff review completed; no independent reviewer delegated. The experiment checkout was removed. One push; no old-job rerun, labels, or merge. Fresh hosted Coverage/reviews remain pending. |
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on 74a7b5b. The verdict and the full list are in the summary comment.
|
The code at 74a7b5b has no blocking issue, but merge readiness still needs complete Linux Coverage evidence and a successful Smoke Tests finish. Since the earlier review, the shared pack-entry type, output-directory consistency, fixture count, deterministic ordering, supplement version checks, and closed-stdout handling are fixed. No conflicts are known. Coverage is reported as FAILURE, but the supplied excerpt ends during test output without a terminal diagnostic or suite summary, so its cause remains unassigned. Its route runs through the gate runner, output/exit forwarding, and test:coverage:ci across all non-fuzz-worker projects, including both new unit-core suites, then fuzz-worker; this overlaps the change. Smoke Tests is IN_PROGRESS and also covers affected build/package checks and registered gates. Earlier macOS passes and unchanged-base success do not establish an unrelated failure. Before merge, obtain the complete exact-head Linux Coverage outcome, diagnose its first failing diagnostic on that route, fix the responsible code and rerun; Smoke Tests must also finish successfully. Not blocking: could both application and validation entrypoints call the existing exitAfterFlush owner, after extending it for the demonstrated real-pipe cases while preserving bounded completion and the regression assertions, then delete the nested exitAfterOutput implementation and keep error-prefix formatting in runEntrypoint; this consolidation suggestion can be taken or left, and the answered jpeg-js table and independent package-path questions stay closed. The non-EPIPE policy thread still applies as an answered policy tradeoff, and two lower-priority style threads still apply. The remaining threads can be resolved: shared entry type now uses NpmPackEntry; output destinations use one outDir; fixture count matches its entries; body assertions separately cover full license and NOTICE text; empty notice is valid when no third-party code survives and retained dependencies fail closed; ordering uses code-unit comparisons with regression coverage; version drift is rejected; supplement table adds no demonstrated correctness benefit; independent paths support standalone copying and fail closed on drift; entry normalization has no evidenced malformed producer or false rejection; missing-file errors identify the file and fail the build; unavailable stdout has guards and real closed-pipe assertions; standalone imports do not prevent independent execution; flush completion waits behind prior writes and has exact 4-MiB length and tail assertions, with no demonstrated current failure; supplement body loss is not demonstrated because complete tracked texts are read and version drift is rejected. I read the code and the check output; I did not build, pack or run tests, so the notice file contents and test counts are unverified. No live device proof is required for these build, test, and validation changes. |
|
Fixed in The failed Coverage job had one actual test failure: Coverage starts without Evidence:
Cold diff review completed; fresh hosted checks and bot review remain required. No CI rerun, labels, or merge. |
5c64e00 to
02ff19e
Compare
|
Rebased onto main
Fresh validation: frozen install, build, full-repository format, and 23 focused tests passed. Earlier red/green and Linux/macOS coverage evidence retains its original SHA attribution. Fresh hosted checks and reviews are pending. No merge or labels. |
`underFakeTimers` advanced the clock 10 ms per awaited step, and each step is a real event-loop turn. A fill-verification sample crosses a real loopback socket to the fake helper session, so the turns it needed burned fake time: the 150 ms sample interval came out at 230-250 ms, and the 1500 ms deadline was reached with a sample still owed. The late-value test then saw seven captures instead of eight and reported a mismatch, at three in ten runs on an unloaded host. One millisecond per step keeps a socket reply inside its own interval: the interval is met at 150-160 ms and the test's sample count is no longer a function of host speed. The budgets and every assertion stay as they were.
`rollup-plugin-license` types `Dependency.version` as optional and reports `null` for a module whose `package.json` has none, which is what an unresolvable or path-linked module looks like to it. `renderNotices` tested only the license and its text, so an otherwise fully licensed dependency was emitted as `included@null (MIT): [1]` — a shipped notice attributing code to a release that never existed — and a dependency missing both fields was reported as a license problem, pointing at a LICENSE file instead of at the version-less module. The version is now checked first, so every later diagnostic can name the package the way the notice file does. No shipping package is affected: the current build resolves a version for all 27 bundled dependencies.
`assertBundledLicenseNotices` read the `npm pack` manifest, which can only say that `dist/src/THIRD_PARTY_NOTICES.txt` exists and how many bytes it is. The bundler writes a 41-byte "No third-party dependencies are bundled." notice when it reports zero dependencies, and a dependency that carries neither a version nor a license identifier is reported as none — with its code still bundled. Either way the publish gate cleared a tarball shipping unattributed code, which is the failure this PR exists to prevent. The published CLI always bundles, so its notices must carry at least one `name@version (license)` attribution and one referenced license text. The gate now reads those bytes: from the tree it just packed, and again from the clean-installed tree a user receives. A project that genuinely bundles nothing still gets the generic notice — that policy is the published package's, not the bundler's — and a caller holding only the manifest keeps the manifest-only check, which is what the size lane's base measurement of an older tree needs.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Published Exact-head gate Red→green against published head ( Clock repairs, candid scope. Android: No push-side source edits were made after review approval. Independent Apex review approved |
The published-notices gate counted the two kinds of line a notices file holds: at least one `name@version (license)` attribution and at least one `[n]` marker. A file of bare headings satisfies both counts and carries no license terms at all, so `pkg@1.0.0 (MIT): [1]` over an empty `[1]` cleared the gate, as did a reference to an id the file never writes. The gate now reads the sections: every reference between a package's colon and its optional `; Author:` clause must name a numbered section with text in it, and the failure names the attributions that do not. Deduplicated and multi-reference files, author clauses, and license bodies with blank lines inside them are the shapes the real build writes, so they stay accepted, and a tree that genuinely bundles nothing still gets the generic notice.
The generic notice for a bundle with no third-party code ended in a newline and the populated one did not, so the file a user reads ran its last license block against the shell prompt. Every other text file the build writes ends one line, and the notice is 49 kB of text people read with `cat`. One newline after the joined sections, and the existing populated fixture now pins that the file ends in exactly one.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 44b5930. The verdict and the full list are in the summary comment.
The published-notices gate matched the license field with `[^)]+`, so an attribution whose license is a grouped SPDX expression — `renderNotices` wraps whatever the plugin reports, and the plugin passes a parenthesized `license` field through, producing `pkg@1.0.0 ((MIT OR Apache-2.0)): [1]` — was not recognized at all. A notices file holding only such a line reported zero attributions, so the gate rejected bytes a healthy build produced; mixed with a valid line, the unparseable one was skipped, so a grouped attribution with a dangling or missing reference cleared the gate unseen. The field now ends at the first `):` boundary, the only place a nested expression can close, and the reference list stops at the renderer's optional `; Author: ` clause. That also drops a disclosed phantom: a package whose author field carried `[7]` read that footnote as a second license reference, failing valid bytes that referenced only `[1]` (and passing a file whose sole "reference" was such a footnote). The new cases run the real license plugin over a fixture tree, so they assert on bytes `renderNotices` emits, not on hand-written shapes: a grouped expression and an author footnote each pass the pack gate, and a grouped line whose reference is dangling or absent still fails naming it.
|
Evidence at Bytes, not hand-written shapes: two new tests build a fixture tree with the repo's own fixture build ( What the renderer writes for supported input:
Old reader (
New reader: both real-bytes files pass; the two mixed files above now fail naming the grouped attribution ( Counts: focused Scope kept honest: the license field is read as opaque producer text, not audited for SPDX validity. |
The pack gate scanned every line of THIRD_PARTY_NOTICES.txt for an attribution, but the renderer emits the attributions as one block before the first numbered section and copies each section's license or NOTICE text verbatim after it. Prose there belongs to the upstream package: a contact line or a pointer to "section [9]" shaped like an attribution made the gate reject bytes the bundler legitimately wrote, and one such line let a file that attributes nobody clear the gate that exists to catch exactly that. Scope attribution parsing to the header block the producer writes; the section scan, the reference checks and the reject directions stay as they were.
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on df46a35. The verdict and the full list are in the summary comment.
|
At df46a35, the earlier review's notice checks are stronger, but two issues remain. CI reports 36 passing checks. There are no conflicts. The package gate can pass when a bundle contains a valid licensed dependency and another dependency with neither license nor version: the plugin omits the latter from its report, so it is never validated. Published code can therefore lack required notices. The build must independently enumerate every bundled third-party module and require attributable metadata and license text for each; please cover a mixed valid and omitted dependency through build and publish validation. Could early stdout errors leave the script hanging when stderr is backpressured and main remains pending? This code is unchanged from the earlier review, and its drain deadline starts only after main settles. One output-completion owner should bound diagnostics in both paths; please cover pending main with an unread stderr pipe. Could the notice-format reader move beside its renderer, with build-time module enumeration enforcing completeness before formatting? Extracting a dependency-free format owner, including it in the standalone size-script copy contract, and replacing the existing parser would let the gate call that contract while retaining package-path policy. Two lower-priority threads still apply: format ownership and multiline metadata. The other twelve threads no longer apply at this head. Before merge, please close the bundled-dependency completeness gap, bound early diagnostics, and consolidate the notice-format parser with its owner. I did not run tests or reproduce the reported timings and artifact sizes. No live device run is needed for these build and package changes. |
The build now walks every module rendered into the bundle and attributes each third-party one to its package itself, replacing rollup-plugin-license. That plugin dropped a package whose manifest named neither a version nor a license, so a bundle beside a licensed neighbor shipped a partial notices file the pack gate accepted. A module with no package.json under node_modules now stops the build too. The real artifact is byte-identical (48,996 bytes, 27 attributions over 23 sections). The notices format moves into scripts/size-report-package-notices.mjs: one dependency-free owner of the renderer and reader. The pack gate calls its reader and keeps the package-path policy; the size lane's base step copies size-report-package*.mjs so the measured revision's format travels with its policy. Multiline license and author text renders on one attribution line. runEntrypoint routes both output paths through settleOutput. An early stdout-failure diagnostic still unflushed at its deadline exits with the retained failure while main is pending, since Node cannot close stdio and a stalled stderr write would otherwise hold the process forever. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The empty-bundle notice is internal to the renderer, and the hand-written size-report-package-notices.d.mts joins the existing size-report-package.d.mts Fallow entry: its consumers resolve it through tsc, not an import edge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 50cc9f9. The verdict and the full list are in the summary comment.
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Response to the review at df46a35, now at head Red/green method: I put the new tests on top of the production files from
Apex Sentinel opened four threads at |
|
At 50cc9f9, the code looks good, and the three earlier findings are fixed. The build now lists every bundled module itself, so a dependency the license plugin skips can no longer get past the gate. Early output diagnostics are bounded while main is still running. The notices format has one owner that both the renderer and the package gate use, with attribution whitespace normalized and only consumed exports kept. All 36 checks pass, including the build, package and script routes this touches. The review threads are already resolved, and no conflicts are known. |
thymikee
added
the
ready-for-human
Summary
Keeps bundled dependency license and NOTICE text in the npm package, including jpeg-js's source-file notices. Identical texts are written once. A bundled package missing its license, its license text or its version stops the build. The notices are written next to the bundle output.
scripts/lib/bundled-license-notices.tsgoes through every module rendered into each output chunk and maps each third-party module to the nearest namedpackage.json. This replacesrollup-plugin-license, which silently skipped any package whose manifest had neither a version nor a license. A module undernode_moduleswith nopackage.jsonalso stops the build. Root and private workspace packages count as this project's own code. The real artifact is byte-identical to the plugin's: 48,996 bytes, 27 attributions over 23 sections.scripts/size-report-package-notices.mjsimports nothing and holds both the renderer and the reader. The pack gate (scripts/size-report-package.mjs) calls the reader and keeps the package-path policy and the rule that the published CLI must attribute somebody. The Bundle Size lane's base step copiesscripts/size-report-package*.mjs, so a measured revision's format goes with its own policy. A test enforces that copy contract. Multiline license or author text is written on a single attribution line.name@version (license): [n]line must point at a numbered section that has text. Only the header block before the first[n]heading is read for attributions. A grouped SPDX license expression is accepted, and an; Author:clause cannot stand in for a reference. The empty-bundle output and manifest-only callers are unchanged.runEntrypointoutput now goes throughsettleOutput. When stdout fails early, the diagnostic gets the same deadline as final output. If that diagnostic is still unflushed at its deadline while main is pending, the script exits with the failure it kept. Node cannot close stdio, so otherwise a stalled stderr write would hold the process open forever.underFakeTimerssteps 1 ms per event-loop turn, and Apple's queued readiness no longer spends fake time.Validation
50cc9f9f76:check:affected --run --head 50cc9f9f76— 41 passed / 25 skipped,exactCommit=true,checkoutChanged=false(receiptrun-WsFLhG). The earlier push96505cea47failed only Fallow (unused exports on the new format module), which50cc9f9f76fixes.df46a350dd(7 failed / 77 passed) and pass at head (84 passed).pnpm buildgives adist/src/THIRD_PARTY_NOTICES.txtthatcmpmatches byte for byte against the plugin-built file atdf46a350dd, and the pack gate accepts it.🤖 Generated with Claude Code