Repository navigation
fix(release): validate distributed source instead of lockfile dependencies - #5375
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
Independent agent review. Reviewed at fe2171f15af09dfd5eafc6cad84076ea8f0a283c. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.
No P0–P2 findings.
This deletes 304 lines of tests from a release-compliance script, which is the one place where "the tests went with the responsibility" deserves to be checked rather than accepted, so that is where I spent the review.
The deleted tests all pin the removed responsibility
Every removed test names lockfile traversal in its own title — nested package lockfiles, lock provenance, lock links, lock closure, lockfile versions, npm shrinkwrap, nested package managers, and using a generated notice as dependency license authority. None of them pins a property the script still claims to enforce.
The compliance-critical property survives, on a better basis
rejects Category X and unknown nested package manifest licenses is retained, and the check it covers now reads manifests actually present in the archive rather than lockfile metadata:
if (manifest.license) validateReleaseLicense(manifest.license, entry);
else if (manifest.private !== true) {
throw new Error(`Cannot safely classify package manifest without a license: ${entry}`);
}For a source release that is the more appropriate authority — what ships is what is in the archive — and it is fail-closed in both directions: an unknown license is rejected by the allowlist, and a non-private manifest with no license at all is rejected outright. The A?GPL|LGPL-\d pattern is not the only gate; a bare GPL with no version would miss the pattern and then be rejected as unknown, so the allowlist backstops it.
rejects ZIP payloads hidden behind an allowed source prefix and rejects unknown binary paths even when their bytes are valid UTF-8 are also retained, so the payload checks are intact.
One clarification worth making explicit, because "remove that dependency-license traversal" can be read more broadly than it is: package-lock.json is still required in the archive and still read — for version consistency (packageLock.packages?.['']?.version) and as a required root file. What went away is its use as a license authority, not lockfile handling as such.
Ablation, and a correction to my own first reading
Neutering categoryXLicensePattern so it can never match turns rejects Category X and unknown nested package manifest licenses red. Exactly one test, not two.
I say that precisely because my first run looked like two. creates reproducible candidates from committed files only also failed — but it fails identically on the unmodified tree, with spawnSync gpg ENOENT, because it signs a real candidate and this machine has no GPG. It was failing for that reason in both states, so attributing it to the ablation would have overstated what the ablation showed.
On the head that moved while I was reviewing
I had finished against 31ad4058a when the branch advanced, so I re-checked rather than publishing a stale conclusion. The increment is one file and a net −23 lines: sourceImageFormat is gone, and the non-text rejection message loses its (PNG)-style suffix while keeping the full entry path. The throw itself is unchanged, so nothing I verified depended on it — but I re-ran everything at the new head instead of assuming that, and the ablation result is identical there (18 pass ablated versus 19 restored: one additional failure, the Category X test).
Evidence
asf-source-release: 19 pass, 1 fail, the single failure being the GPG one above. ci-workflow-policy: 44/44.
Not covered by me
Anything requiring GPG, which is the real candidate-signing path — I did not install it, so that test and whatever it would have exercised are unverified here rather than passing. The description's own figures (92 ASF checks, 82 CI planner/policy tests, the notice/closure tests, and the temporary LGPL experiment against both notice generators) are the author's evidence, not mine.
I also did not run the hosted candidate workflow, which the description states was not run either, and I did not evaluate the release's ASF compliance as a matter of policy — only whether the script's checks do what they claim.
This remains a draft source-release repair by the description's own framing.
Code review, CI status and merge readiness are separate. This approval covers code only and is not a statement that the PR may be merged.
Summary
Source archive creation rejects the website's external LGPL build tools because it treats every lockfile entry as distributed content. Remove that dependency-license traversal and its Desktop-notice overrides; check the package manifests and payloads actually in the archive. Installation consistency and actual shipped-dependency licensing remain with npm ci and the existing Desktop/CLI notice generators in the candidate workflow.
Also recognize Astro, readable diffs and the commit hook; record five image inputs and the attributed xterm patch's control bytes; escape the attachment fixture's ZIP bytes. Ordinary CI now creates and validates the real committed source archive on every run, including documentation and asset changes.
Remove the image-format sniffer used only to decorate rejection messages. Errors retain the full file path; compiled-payload rejection and provenance decisions are unchanged.
Refs #2974
Verification
The source verifier no longer attempts to approve external dependencies from lock metadata, validate nested lock schemas/links, or use a Desktop inventory as license authority for unrelated packages. Their tests were removed with those responsibilities. External-tool usage/provenance still requires release review, and convenience artifacts remain subject to their shipped-dependency checks. No new dependency allowlist or graph implementation is retained.
Not run: full repository tests or hosted Ubuntu candidate workflow. No signing, staging or voting. This remains a draft source-release repair.
AI use
Tool(s) and scope: Codex investigated, implemented, simplified and verified the changes.
Checklist
Does this PR entail a change in behavior?