Skip to content

fix: retain third-party license notices in distributions - #4070

Open
thymikee wants to merge 3 commits into
mainfrom
fix/third-party-license-notices
Open

thymikee wants to merge 3 commits into
mainfrom
fix/third-party-license-notices

Conversation

@thymikee

@thymikee thymikee commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Summary

Published bundles include third-party code but omitted its license texts. Generate dist/src/THIRD_PARTY_NOTICES.txt from modules retained in each build, including upstream NOTICE files and jpeg-js's source-level notices. Externalized or tree-shaken dependencies drop out automatically. Identical texts are shared, while distinct copyrights remain separate.

Retain the Apache-2.0 license and provenance for vendored Maestro fixtures and the Gradle wrapper. Require a non-empty generated notice file in the actual npm tarball.

13 files changed. Packaging and development compliance only; no CLI, Node.js, or device behavior changes.

Validation

Current head: c80338229d.

  • Missing-notice regression, same test command with the new test added to the base:

    base 0f7e668007 head 7410a5be4b
    Fails: missing expected exception Passes: missing/empty notices rejected
  • Focused license-generation and package-content tests: 8 passed, including shared-text deduplication, distinct copyrights, minification, external/tree-shaken dependencies, missing license text, and jpeg-js source notices.

  • pnpm build generated the license report for the real bundle.

  • Full checks passed at 7410a5be4b. pnpm check:affected --base 7410a5be4b --run --head c80338229d also passed all runnable checks with exactCommit: true.

  • Size versus 7410a5be4b: notices 92,631 → 47,966 bytes; packed tarball 1,543,992 → 1,541,891 bytes. All 27 packages and 57 distinct copyright lines retained.

  • Independent review of the initial change and the compact formatter: no actionable findings.

  • GitHub CI pending; no device runs required for this packaging-only change.

@vercel

vercel Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
agent-device-website Ready Ready Preview Oct 29, 2026 2:43pm UTC

Request Review

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.59 MB 5.64 MB +48.0 kB
Package (unpacked) 5.59 MB 5.64 MB +48.0 kB
Package (download) 1.70 MB 1.70 MB +7.1 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 31.3 ms 30.0 ms -1.4 ms
CLI --help 60.7 ms 60.1 ms -0.6 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 13 files

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__/fixtures/size-report-npm-pack.json">

<violation number="1" location="scripts/__tests__/fixtures/size-report-npm-pack.json:5">
P3: This fixture's `entryCount` is still 20, but the `files` array now has 21 entries after adding `THIRD_PARTY_NOTICES.txt`. Update it to 21 so the fixture stays self-consistent.</violation>
</file>

<file name="scripts/size-report-package.mjs">

<violation number="1" location="scripts/size-report-package.mjs:68">
P2: `assertBundledLicenseNotices` is exported here but not declared in `scripts/size-report-package.d.mts`. `check-package.ts` imports it, so TypeScript will not see the export. Add the declaration to the `.d.mts` file.</violation>
</file>

<file name="tsdown.config.ts">

<violation number="1" location="tsdown.config.ts:203">
P2: The notice directory `./dist/src` is hardcoded here while `outDir: 'dist/src'` is set separately above. Define one constant (for example `const outDir = 'dist/src'`) and use it for both `outDir` and the notices path, so the notices cannot drift from the bundle output.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

};
}

export function assertBundledLicenseNotices(entries) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: assertBundledLicenseNotices is exported here but not declared in scripts/size-report-package.d.mts. check-package.ts imports it, so TypeScript will not see the export. Add the declaration to the .d.mts file.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At scripts/size-report-package.mjs, line 68:

<comment>`assertBundledLicenseNotices` is exported here but not declared in `scripts/size-report-package.d.mts`. `check-package.ts` imports it, so TypeScript will not see the export. Add the declaration to the `.d.mts` file.</comment>

<file context>
@@ -65,7 +65,16 @@ export function collectNpmPack(root) {
   };
 }
 
+export function assertBundledLicenseNotices(entries) {
+  if (
+    !entries.some((entry) => entry.path === 'dist/src/THIRD_PARTY_NOTICES.txt' && entry.size > 0)
</file context>

Comment thread tsdown.config.ts
dropAmbientDeclarationImports,
bundledLicenseNotices(
import.meta.dirname,
fileURLToPath(new URL('./dist/src', import.meta.url)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The notice directory ./dist/src is hardcoded here while outDir: 'dist/src' is set separately above. Define one constant (for example const outDir = 'dist/src') and use it for both outDir and the notices path, so the notices cannot drift from the bundle output.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At tsdown.config.ts, line 203:

<comment>The notice directory `./dist/src` is hardcoded here while `outDir: 'dist/src'` is set separately above. Define one constant (for example `const outDir = 'dist/src'`) and use it for both `outDir` and the notices path, so the notices cannot drift from the bundle output.</comment>

<file context>
@@ -195,7 +196,13 @@ export default defineConfig({
+    dropAmbientDeclarationImports,
+    bundledLicenseNotices(
+      import.meta.dirname,
+      fileURLToPath(new URL('./dist/src', import.meta.url)),
+    ),
+  ],
</file context>

"unpackedSize": 2319,
"files": [
{
"path": "dist/src/THIRD_PARTY_NOTICES.txt",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This fixture's entryCount is still 20, but the files array now has 21 entries after adding THIRD_PARTY_NOTICES.txt. Update it to 21 so the fixture stays self-consistent.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At scripts/__tests__/fixtures/size-report-npm-pack.json, line 5:

<comment>This fixture's `entryCount` is still 20, but the `files` array now has 21 entries after adding `THIRD_PARTY_NOTICES.txt`. Update it to 21 so the fixture stays self-consistent.</comment>

<file context>
@@ -1,6 +1,10 @@
   "unpackedSize": 2319,
   "files": [
+    {
+      "path": "dist/src/THIRD_PARTY_NOTICES.txt",
+      "size": 100
+    },
</file context>

@thymikee

Copy link
Copy Markdown
Member Author

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 ${name}@${version} in the notice template (scripts/lib/bundled-license-notices.ts#L13) so the file stays identical if chunks render in a different order; drop private:true from the fixture root package.json in the test so the absence check exercises the cwd exclusion that production relies on; and optionally print identical license texts once after the name@version list, keeping one entry per distinct text, since the size report shows +92.7 kB unpacked and +9.2 kB download.

Three open threads from cubic-dev-ai still apply: the missing assertBundledLicenseNotices declaration in the d.mts file (#4070 (comment)) and the duplicated dist/src output path in tsdown.config.ts (#4070 (comment)). One lower-priority thread also still applies, and none of the threads can be resolved yet.

Checks are still running. Smoke Tests is pending, and it runs pnpm check:package (action.yml#L100), which builds with the new plugin and runs the new gate on the real npm pack output. A failure there would likely be related to this change. Coverage passes now, and there are no conflicts.

I did not build or run anything. I did not observe the plugin's ordering, its cwd exclusion, or its use of renderedLength. I also did not check whether the dts pass in tsdown re-renders and overwrites THIRD_PARTY_NOTICES.txt with a partial list. The non-empty tarball assert would not catch that. Please confirm the packed notice file lists all bundled dependencies, including anything pulled in by @agent-device/testmu and @agent-device/proxy.

Before merge, Smoke Tests must pass. Then add the d.mts declaration, use one output-directory constant in tsdown.config.ts, and set entryCount to 21 in size-report-npm-pack.json.

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 size-report-package.d.mts, tsdown.config.ts or size-report-npm-pack.json, so the three threads still apply there. The next review will cover c803382.

@thymikee

Copy link
Copy Markdown
Member Author

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 dependencies by name@version before the loop in scripts/lib/bundled-license-notices.ts, so chunk order cannot renumber the [n] ids (I am not sure the order varies between builds); drop private: true from the fixture root package.json in the test, so the doesNotMatch check exercises the self exclusion and not only the private-package filter; and consider adding author to the compact line, or confirm that every bundled license text has a Copyright line, because the old output also carried the author.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant