Conversation
- applyException refuses copyrightByOperand on a single-identifier expression, where render.ts prints no per-operand credit. - applyException refuses a credit naming a holder the pinned license text does not state (whitespace-collapsed, holders split on "; "). - The exception remedy template tells the reader when to record copyrightByOperand. - render.ts: correct the comment on the product preamble - downstream CI does run this repository's verify:third-party-notices scripts. - notices-policy.json: re-sign npm:posthog-node for its per-operand credits (tj_couch@sil.org, 2026-09-28) and name each operand's holders in its reason; separate its Apache-2.0 notices with "; "; record @posthog/core as (Apache-2.0 AND MIT) so the license-distribution table counts the pair once. - Regenerated THIRD-PARTY-NOTICES.md and its lock (cold build, Linux). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tjcouch-sil
left a comment
There was a problem hiding this comment.
Thanks for picking these up! Five inline comments below. One more thing, about me being the recorded reviewer on npm:posthog-node:
I'm happy to be the recorded reviewer. The Apache-2.0 credit (PostHog/Hiberly; Mixpanel) is right, and the ; separator fix is right. There's one thing I'd like us to settle first, though: which credits we list.
The situation. The two posthog entries now follow different rules:
npm:posthog-node(notices-policy.json:135) lists all five MIT notices in its LICENSE: Sentry 2012, Sentry 2017, Meta, Expo and AgentCat. Only one of those, Sentry 2012 (getsentry/sentry-javascript), matches code posthog-node ships. Its three Sentry-derived files aresrc/extensions/error-tracking/autocapture.ts,modifiers/module.node.tsandmodifiers/context-lines.node.ts. The other four come from code in PostHog'spackages/react-nativeandpackages/mcp.npm:@posthog/core(notices-policy.json:147-148) lists trimmed credits: only the holders whose code the package ships today (PostHog 2022, TraceKit and Sentry 2012 under MIT; LiosK under Apache-2.0).
Why I'd rather have the complete list everywhere. An exception is looked up by package name, and version is "provenance, not part of the key" (types.ts:105). Only a change to the LICENSE's hash forces a re-review. PostHog's LICENSE is shared across their whole repo, so if a later posthog-node or @posthog/core starts shipping code from Expo, Meta, Sentry-RN or AgentCat, the file won't change, since it already lists them. The exception would keep passing, and a trimmed credit would quietly under-credit. Credits taken from the file as-is don't have that problem, and listing a holder whose code we don't currently ship costs nothing; the whole LICENSE file is printed verbatim anyway.
My suggestion: keep posthog-node's credits untrimmed, and expand @posthog/core's to every notice its LICENSE gives under each operand. MIT: PostHog 2022; TraceKit; Sentry 2012; Sentry 2017; Meta; Expo; AgentCat (lines 229, 256, 291, 318, 345, 372, 399). Apache-2.0: PostHog/Hiberly; Mixpanel; LiosK (lines 1, 3, 280).
Going further, a few options I'm unsure about:
- A completeness gate: every
Copyrightline in the pinned file must appear in some operand's credit. This would enforce the complete-list rule the same way the new check enforces "holder is in the file". I lean toward this one. - Discovering the headers programmatically: scan a package's files for "derived from " headers and map them to LICENSE sections. This is probably too specific to PostHog's conventions to be worth it.
- Or maybe this isn't as big a deal as I'm making it, since the full LICENSE text is always printed and the credit lines only decide which holder is named beside each canonical text.
Matt, you're welcome to push back on any of this. I'd like your take on which way to go.
AI-assisted review (Claude Code).
…ry one Addresses TJ's review of #2872. - applyException refuses a credit that is not a whole notice the pinned license text states. A plain substring test over the whitespace-collapsed file passed truncated notices, bare holder names, lines of license prose, and two notices run together across the blank line between them. - Where credits are recorded, every notice the pinned text states must be credited to some operand. An exception is keyed by name, and a monorepo LICENSE names every holder whatever a package ships today, so a trimmed credit would silently under-credit a later version under an unchanged text. Exceptions that record no credits are not checked. - @posthog/core's credits now list all ten notices in its LICENSE. - Regenerated THIRD-PARTY-NOTICES.md and its lock; the diff is exactly @posthog/core's two credit lines. - report.test.ts: moved the stacked-grant remedy test above the comment that explains the conjunction test below it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, TJ. I've taken the complete-list rule and the completeness gate.
(AI-assisted, with my guidance) |
tjcouch-sil
left a comment
There was a problem hiding this comment.
Looks good, thanks Matt! One non-blocking follow-up inline.
| // trimmed to today's code would silently under-credit a later version whose license text, and so | ||
| // hash, is unchanged. An exception recording no credits has every row credited to the file's | ||
| // first notice, and nothing here to check. | ||
| const uncredited = creditEntries.length |
There was a problem hiding this comment.
The completeness gate skips any exception that records no credits, and two existing AND-license exceptions fall through that gap with the wrong holder printed:
- chroma-js
(BSD-3-Clause AND Apache-2.0): the Apache-2.0 grant covers ColorBrewer's color tables,Copyright (c) 2002 Cynthia Brewer, Mark Harrower, and The Pennsylvania State University.(node_modules/chroma-js/LICENSE:35-36). ButTHIRD-PARTY-NOTICES.md:8627credits the Apache-2.0 text toGregor Aisch. - lucide-react
(ISC AND MIT): the MIT grant covers the Feather-derived icons,Copyright (c) 2013-present Cole Bemis(node_modules/lucide-react/LICENSE:23-25). ButTHIRD-PARTY-NOTICES.md:8776credits the MIT text toLucide Icons and Contributors.
Neither is new to this PR, and the full LICENSE files are still printed. It's the same mis-credit copyrightByOperand exists to fix, though, and nothing flags it.
Suggestion:
- Add
copyrightByOperandto both entries. - Require the field on any AND-license exception whose pinned file states two or more notices, so the completeness gate covers these too. Running this commit's
statedNoticesover the current exceptions, only chroma-js and lucide-react would be affected; pako has an AND license but a single notice.
Happy for this to land here or in a follow-up, whichever you prefer.
AI-assisted review (Claude Code).
tjcouch-sil
left a comment
There was a problem hiding this comment.
Two more small ones inline, both of which you'd hit when adding credits for chroma-js (my earlier comment on line 781).
AI-assisted review (Claude Code).
| if (uncredited.length) | ||
| return blocked( | ||
| `the reviewed exception for ${key}@${version} credits no operand with ` + | ||
| `${uncredited.map(([notice]) => `"${notice}"`).join(', ')}, which its pinned license text ` + |
There was a problem hiding this comment.
Letting a credit stop at any line break keeps a trailing All rights reserved. optional, but it also accepts a holder list cut in half. chroma-js's Apache-2.0 notice wraps across two lines (node_modules/chroma-js/LICENSE:35-36):
Copyright (c) 2002 Cynthia Brewer, Mark Harrower,
and The Pennsylvania State University.
"Copyright (c) 2002 Cynthia Brewer, Mark Harrower," passes as a whole notice, and Penn State drops out of the printed credit.
This message steers a reviewer straight into it. It quotes only the notice's first line (([notice]) => takes forms[0]), so if chroma-js gets credits, the text you'd copy out of this error is the truncated one.
Suggestion:
- Quote the whole notice here (the last form,
forms[forms.length - 1]). - Accept a shorter form only when what it leaves off is a trailing
All rights reserved.-style line, not part of the holder.
| * since a single-identifier row prints no per-operand credit. Each value is one or more notices | ||
| * copied whole from the pinned license file and separated by `; `. `applyException` refuses a | ||
| * credit that is not a whole notice the file states, and refuses credits that leave any notice in | ||
| * the file credited to no operand - an exception outlives the code any one version ships. An |
There was a problem hiding this comment.
The last sentence here ("An operand with no key keeps the package's own notice") describes a partial map, but the completeness gate (policy.ts:781) doesn't allow one. It requires every notice in the file to be credited to a keyed operand, and it doesn't count the fallback notice an unkeyed operand prints.
chroma-js is the natural case. Gregor Aisch's notice is already right for BSD-3-Clause, so you'd record only:
{ "Apache-2.0": "Copyright (c) 2002 Cynthia Brewer, Mark Harrower, and The Pennsylvania State University." }The gate refuses that with credits no operand with "Copyright (c) 2011-2025, Gregor Aisch", so you have to key BSD-3-Clause as well.
Keying every operand is fine by me, and it's the more explicit record. I'd just replace that sentence with "every operand needs a key" so the docs match the gate. The other option is to count the file's first notice as credited whenever some operand has no key.
Summary
Follow-up to #2871, which was merged as-is to unblock the Paratext 10 build. This PR addresses the six (all low-severity) findings from its review.
Why review this
Not part of current epic. #2871 added
Exception.copyrightByOperand, a hand-typed copyright notice that the notices document prints verbatim. Nothing checks that notice against the license file, and two shapes the gate accepts never print it. This PR closes those gaps while the feature is fresh, and fixes a small rendering inconsistency #2871 introduced.Changes
applyExceptionrefusescopyrightByOperandon a single-identifierspdx.render.tsonly prints per-operand credits for a multi-operand expression (spdxIdsOf(...).length > 1). An exception is always pinned to a file, so a single-id credit cleared the gate and then appeared nowhere.applyExceptionrefuses a credit that is not a whole notice the pinned license text states.classifynow passes the pinned file's text alongside its hash. Each credit is split on;, and each part must be one of the file's notices, starting at the line that opens it and stopping only at a line break within its paragraph. A notice is a line opening withCopyright,COPYRIGHT,©or(c)in a notice shape (seeNOTICE_START); Apache'sCopyright [yyyy]appendix, its(c) You must retainclause andCOPYRIGHT AND PERMISSION NOTICE-style headings are not notices. The hash pins the file, not what was typed from it; a plain substring test passed truncated notices, bare holder names, license prose and two notices run together.report.ts) now tells the reader when to recordcopyrightByOperand. The line is unconditional, because the stacked-grant case typically arrives declared as one license and identified as nothing, so the template cannot see the conjunction coming.render.tscomment on the product preamble no longer says a downstream product's CI "does not run this repository's scripts". Paratext 10's CI runsverify:third-party-noticesandverify:third-party-notices:shipping-setin its clone, so those scripts are a contract with it.notices-policy.json:npm:posthog-nodeis re-signed for its per-operand credits:tj_couch@sil.org, 2026-09-28. Itsreasonnow names which holders grant each operand. The text hash is unchanged. Every credited holder matches a line of the pinned LICENSE word for word (lines 1 and 3 for Apache-2.0; 227, 254, 281, 308 and 335 for MIT). @tjcouch-sil confirmed he is happy to be the recorded reviewer.;, like every other multi-notice credit.npm:@posthog/coreis recorded as(Apache-2.0 AND MIT), matchingposthog-node, so the license-distribution table counts the pair once instead of as two one-package rows.npm:@posthog/core's credits list every notice in its LICENSE: MIT gets PostHog 2022, TraceKit, Sentry 2012, Sentry 2017, Meta, Expo and AgentCat; Apache-2.0 gets PostHog/Hiberly, Mixpanel and LiosK. Itsreasonsays why a notice for code this version does not ship is still credited.types.tsdoc, the notices README Governance bullet andexceptionsNoteare updated to state the new rules.THIRD-PARTY-NOTICES.mdand its lock (Linux, cold cache). The diff is exactly the merged distribution row,@posthog/core's license column and two credit lines, and theposthog-nodecredit separator.Downstream impact
This changes rendered text for
@posthog/coreandposthog-node, which paratext-10-studio also ships. Studio's committed notices will need regenerating after this merges, or itsverify-noticesstep fails. The studio overlay records nocopyrightByOperandentries, so the new gates do not block it.Testing
policy.test.ts(copyrightByOperand): single-id refusal; a credit that is not a whole notice (truncated, a fragment, license prose, two notices typed as one, a notice run past its blank line, two notices a blank line apart run together); a notice left uncredited; which lines count as notices (year-less holders, comment markers,©,COPYRIGHT (C)) and which do not (Apache's appendix and clause, headings, prose); a notice cut at a line break; an exception with no credits left unchecked. Each branch of the check and ofNOTICE_STARTwas mutated and seen to fail a test.report.test.ts: the remedy guidance line.npx vitest run .erb/scripts/third-party-notices/: 916 passed.@posthog/corecredits are refused against its real file.npm run typecheckandnpm run lintare clean.npm test: 6328 passed; the run reported one unhandled rejection fromsrc/renderer/services/web-view.service-shard.test.ts(a settle timeout), which passed 105/105 twice on its own.npm run build:third-party-noticesfrom a cold production build, thennpm run verify:third-party-notices: 311 packages verified against the lock.Risk Level
Low. The changes are scoped to the notices generator and its policy data. The new gates only narrow what an exception may record.
🤖 Generated with Claude Code
This change is