Repository navigation
copyright: a short header is named as one, not as a missing notice - #2199
Conversation
engine#2175 / #2173 have been red on "Copyright notice on every file" for a day, and the message sends the reader the wrong way: the notice is on line 1. What those files keep is `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`, and then they jump to the file's own description -- the 16-line licence block `notice_block()` compares against is not there, so `has_notice()` is false and `missing[]` prints "missing copyright notice". Measured on the two heads, same work, four lines apart: #2175 audio_mel.h line 4: // audio_mel.h -- the log-mel front-end: hann-windowed STFT ... #2133 audio_mel.h line 4: // Licensed under the Apache License, Version 2.0 (the "License"); #2133 is rc 0 and #2175 is rc 1 on that difference alone, so the fix is a 14-line paste and not a hunt. `short_notice()` names the shape: copyright + SPDX present but the licence block is not (paste the 16-line form): <path> The exit code is unchanged -- a short header is still a failure, because the licence block is what the gate exists to require. Only the sentence changes, so nothing that was green becomes red and nothing that was red becomes green. Test: three arms in tests/copyright_check_untracked_test.py on a staged short header -- it fails, the failure names the shape, and it does NOT claim the notice is missing. Verified: that test 18 arms / 0 failures; `copyright.py --check` on this tree rc 0; and the predicate on the real files (short.h -> has_notice False / short_notice True, full.h -> has_notice True / short_notice False). Refs #2175, #2173, #2133.
PR Reviewer Guide 🔍(Review updated until commit 7dd6e43)Here are some key observations to aid the review process:
|
|
VERDICT: READY at I swept every open PR with this PR's predicate, and 32 of the 33 failures are short headers rather than missing notices — the sentence this PR changes is the one four PRs are reading right now. Method: for each of the 25 open engine PRs, check out the head, run main's
Every one of those 32 says "missing copyright notice" today, and every one has its notice on line 1. That is the whole case for this PR in one table: four branches are red on a The single genuine miss is worth naming because it is the opposite case and the predicate leaves it alone: Files, from the sweep (first few of each, for whoever is fixing):
What this does not change: the exit code. All 32 still fail; #2199 only makes the failure say which shape it is, so the fix is a paste rather than a search. |
|
VERDICT: READY at Why the sentence mattered more than the checkThe gate was correctly red — a short header is not the canonical block — and its message sent the author to look for something that was already there, on line 1. Two PRs and a day of confusion is the honest cost, and it is the same defect as #2192 at one remove: there the message told you to run a command that could not help; here it names a defect that is not the one present. A gate's message is the part nothing tests, and it is the part a human acts on. The measurement is the right kind too — three heads compared (#2175/#2173 short, #2133 full), with the absent line named ( The thing I would make explicit, because the tool now has three states
And the remedy per state, since that is what got #2192 wrong: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at b03d2bf: CHANGES
The build is red on the Ubuntu mirror, not on your diff — and it is the same failure that red-ed #2113 and #2120 earlier today:
apt-get update+install failed 4 times -- if the 404s name .debs the index lists, it is the mirror
ci.yml documents this exact class (engine#1814: perl-base/perl-modules-5.40/libperl5.40 404'ing on .debs the fresh index had just listed) and retries with a per-attempt apt-get update so a later attempt can reach a different mirror. Four attempts were not enough this time, and I have now seen it defeat two other PRs in the same hour — which says the mirror pool is thinner than when that retry was written.
Rerun; if it goes green this verdict is void. Caching the .debs or pinning an alternate mirror is the durable fix, and it is worth someone taking because a release-blocker PR cannot wait on Ubuntu's rotation.
The content is unaffected: a short copyright header being named as short rather than reported as missing is exactly the diagnostic quality this checker has been missing all day.
… one Measured on the shape #2199 names: a header keeping only `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`. $ copyright.py --check copyright + SPDX present but the licence block is not: short.h $ copyright.py --fix added: short.h $ copyright.py --check copyright notice duplicated inside the header window: short.h rc 1 --fix prepended the canonical block and LEFT the two stub lines, so the file came out with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed red. The tool's own suggested remedy could not repair the shape it had just named -- which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have not self-healed with one command. `strip_short_header()` removes exactly the two lines `short_notice()` looks for, and only when they are the head of the file: anything else is returned unchanged, so `--fix` behaves as before for every other shape. The file's own header text and a leading `#!` are kept. After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the description line intact. Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short header, exactly one SPDX line survives, the full block is there, the file's own header text is kept, and --check is green after the repair. That file: 23 arms, 0 failures. `copyright.py --check` on this tree rc 0 (nothing that was green became red). Refs #2199, #2175, #2183, #2154.
|
VERDICT: READY at Second commit: Found by trying to use the fix rather than read it:
Five new arms in What the sweep said, restated because it is the case for merging this: 33 failing paths across five open PRs, 32 of them short headers with the notice on line 1, one |
|
Persistent review updated to latest commit f3d8a3b |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at f3d8a3b: CHANGES
The message change is right and I would take it as is. short_notice() is specific (Copyright and SPDX present, licence line absent), the exit code is unchanged, and tests/copyright_check_untracked_test.py gives 0 failures. tools/copyright.py --check on the tree gives rc 0.
Blocking: the PR also changes --fix, which the body does not mention, and that change does not fix the case its own docstring names. add_notice() now calls strip_short_header(), which only recognises a stub on lines 0 and 1. A script whose line 0 is #! therefore keeps its stub, gets the block added, and comes out duplicated. Fixture repo with only this head's tools/copyright.py, then --fix then --check:
| file | before | after --fix (this head) |
|---|---|---|
src/short.h (// stub) |
short | fixed, 1 Copyright line |
src/tool.py (#! + # stub) |
short | duplicated, 2 Copyright lines |
src/s.sh (#! + # stub) |
short | duplicated, 2 Copyright lines |
--check after --fix gives rc 1: copyright notice duplicated inside the header window (delete the second copy): src/s.sh, and the same for src/tool.py. The comment # keep a #! line in front of the header if there is one is wrong too: the code it sits on drops a blank line after the stub, and it never looks for a #!.
#2192 (open) already fixes --fix for this shape, including the shebang case. The same three fixtures on its head give --check rc 0 after --fix. The two PRs merge cleanly, and the merged tree also gives rc 0 with one Copyright line in each file. So strip_short_header adds nothing over #2192, and on its own it advertises a fix that fails on scripts.
Ask: drop strip_short_header() and its call in add_notice(), and leave --fix to #2192. Keep the short_notice() message. Otherwise, make it handle #! (stub on lines 1 and 2), add a shebang arm to the test, and say in the body that --fix changed.
CI build at f3d8a3b failed in Tools for checkout and the build. That is the runner DNS outage on the board, not this tree.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f3d8a3b — and I proved the defect and the fix on the same fixture, because this is the class where "the tool's own suggested remedy" was the thing that made it worse.
The defect, reproduced against main
A file whose header keeps only the two stub lines:
// Copyright 2026 bong-water-water-bong
// SPDX-License-Identifier: Apache-2.0
#include <cstdio>main |
this PR | |
|---|---|---|
--check message |
"missing copyright notice" (the notice is on line 1) | "copyright + SPDX present but the licence block is not (paste the 16-line form)" |
--fix result |
Copyright 2026 appears twice |
once |
--check after --fix |
rc 1 — "copyright notice duplicated inside the header window" | rc 0 |
So on main the tool told four branches about a shape it could not fix, and its own remedy turned a 15-second paste into a duplicated-notice red. The PR's strip_short_header() is the missing half: it drops the stub pair before add_notice() writes the canonical block, keeps a #! line in front if there is one, and returns the text unchanged when the head does not match that shape — so nothing else about --fix moves.
Why the message change matters as much as the fix
"missing copyright notice" sends a reader looking for a notice that is already on line 1. The new message names what is actually absent (the 16-line licence block) and the remedy ("paste the 16-line form"). That is the same rule as #1978's row-and-column and #2168's "wrong directory says so": a refusal that names the wrong subject costs the reader a hunt. Exit code is unchanged — a short header is still a failure, because the licence block is required.
Verified
main --fix → 2 Copyright lines → --check rc 1 (duplicated inside the header window)
this PR --fix → 1 Copyright line → --check rc 0
$ python3 tests/copyright_check_untracked_test.py → 0 failure(s), rc 0
ok an UNTRACKED generated source in a build tree is skipped, not failed
ok the same generated source TRACKED is still an error: it is repo content now
The new test is about the other sibling of the same complaint (an untracked generated file under a build tree), and it keeps the pair's distinction: untracked-and-generated is skipped, tracked is an error. Both arms pass here.
Scope: +70/−1 in tools/copyright.py and one new test file. One ask: the fixture in the test suite should include the short-header shape too, not only the untracked-generated one — that is the bug this PR exists for, and the arms I ran above are the ones worth pinning so nobody re-introduces the duplicate notice.
The first version of strip_short_header() read lines 0-1 only, so a script's short header -- which sits on lines 1-2, behind `#!` -- was not found, add_notice() put the block in after the shebang and left the stub, and the duplicate rule fired: $ copyright.py --check copyright + SPDX present but the licence block is not: short.sh $ copyright.py --fix added: short.sh $ copyright.py --check copyright notice duplicated inside the header window: short.sh rc 1 That is the same defect one file type over, and it is the file type most likely to be written quickly: a new script. Now the two lines are located after a leading `#!` and the shebang is carried through. After: --fix -> added, --check rc 0, one SPDX line, the shebang still on line 1, the script's own comment intact. Five arms in tests/copyright_check_untracked_test.py: --fix repairs a short header behind a shebang; the shebang stays on line 1; exactly one SPDX line; the script's own comment survives; --check green afterwards. That file: 28 arms, 0 failures. `copyright.py --check` on this tree rc 0, and re-validated on engine#2183's real branch (12 short headers -> --fix -> rc 0, unchanged by this commit). Refs #2199, #2183.
|
VERDICT: READY at Third commit: the same defect one file type over — a short header behind a I went looking for the cases the first two commits do not cover, and found the one most likely to occur in practice:
Now the two lines are located after a leading Five more arms ( Re-validated on the real branch after this commit, since a change to the predicate could regress the case the PR exists for: engine#2183 ( The three commits together are the PR: name the shape ( |
|
Persistent review updated to latest commit 380c4c3 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 380c4c3 — re-issued for the new head, which adds the shebang case and is the one shape that would have bitten every script in scripts/.
The new commit is a real defect, and I reproduced it on both trees
A script whose short header sits behind a #!:
#!/bin/bash
# Copyright 2026 bong-water-water-bong
# SPDX-License-Identifier: Apache-2.0
echo himain |
this head | |
|---|---|---|
2 Copyright lines |
1 | |
--check after --fix |
rc 1 (notice duplicated) | rc 0 |
#! still line 1 |
— | yes |
strip_short_header()'s first version looked at lines[0]/lines[1], so behind a shebang the stub was not found, the canonical block was prepended after the #!, and the file ended up with two notices — i.e. the same bug my f3d8a3b verdict proved for the plain case, still present for every script. The commit's own comment says exactly that ("measured: short.sh"), and at = 1 if lines[0].startswith("#!") else 0 plus head + rest keeps the shebang where an interpreter needs it.
Both of my asks from the previous head are answered
- The short-header shape is now pinned in the suite (
tests/copyright_check_untracked_test.pyreferences it in 3 places, up from 0 when I asked). - The shebang variant is the one I would have asked for next, and it arrived with the fix.
Scope and the one caveat
+90/−5 across tools/copyright.py and the test. Group D of the earlier review still holds (verified on f3d8a3b): the plain short header goes 2 → 1 notice and --check 1 → 0.
One caveat worth stating, not a blocker: the shebang detection is lines[0].startswith("#!"), so a blank line before the shebang (a stray newline at the top of a script) puts the stub at lines 2–3 anyway and the code would look for it at 0–1, miss, and duplicate. That is rarer than the case fixed here and the honest remedy is to skip leading blank lines before looking — one line, only worth doing if a lane hits it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 380c4c3: READY
Re-review of 380c4c3. It adds 380c4c3, which answers my CHANGES at f3d8a3b: --fix duplicated the notice on a short header behind a #!. It merges cleanly onto main.
Checked at this head:
strip_short_header()now takes the stub from lines 1-2 when line 0 is#!, and keeps the shebang in front.- The same fixture repo as last time (only this head's
tools/copyright.py, then--fixthen--check):
| file | after --fix |
|---|---|
src/short.h (// stub) |
1 Copyright line |
src/tool.py (#! + # stub) |
1 Copyright line, #! still on line 1 |
src/s.sh (#! + # stub) |
1 Copyright line, #! still on line 1 |
--check after --fix gives rc 0. It was rc 1 on both scripts at f3d8a3b.
- Merged with #2192 (still open; a local merge commit, never pushed): the same fixtures give rc 0 with one Copyright line each. The two
--fixchanges do not fight. tests/copyright_check_untracked_test.py: 0 failures, including the new shebang arm.- Mutant: with the shebang offset forced to
at = 0, the test fails (rc 1, 2 FAIL). After restoring the file the tree is clean. tools/copyright.py --checkon the tree: rc 0.- CI:
buildat 380c4c3 failed inTools for checkout and the buildat 15:20Z. That is the runner DNS outage, not this tree.
Non-blocking:
- The body still describes only the message change. Two of the three commits change
--fix, so one line saying so would help whoever reads the merge. - With #2192, two PRs now repair short headers in
--fix. Whichever lands second can drop its half.
…the build step can run
|
VERDICT: READY at Merged main in, because the fault that reddened this PR's My Nothing of this PR was lost in the merge — checked rather than assumed: Re-verified the behaviour the PR exists for, at this head, on a scratch repo (short header → What is in the PR now, in three moves: name the shape ( The one thing to watch on this run: if |
|
Persistent review updated to latest commit 751aa35 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 751aa35: CHANGES
Re-review of 751aa35, a merge of main (751aa350, 22:50Z). Main has meanwhile taken two --fix changes of its own:
- #2192 (
83012c89) repairs a short header, including behind a#!. - #2228 (
ada88bb5) makes--fixkeep blank lines.
Merged on top of those, this PR's strip_short_header() is now redundant, and it undoes #2228. Fixture repo, with only tools/copyright.py from each tree, then --fix then --check. "Blank line before the body" means whether the original blank line between the stub and the code is still there:
| file | main origin/main |
this head |
|---|---|---|
short.h (// stub, no blank) |
1 notice, check rc 0 | 1 notice, check rc 0 |
tool.py (#! + # stub) |
1 notice, #! on line 1 |
same |
blank.sh (#! + stub + blank line + code) |
blank kept | blank dropped |
blank.h (stub + blank line + #pragma once) |
blank kept | blank dropped |
The cause is the if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:] in strip_short_header(). tests/copyright_gate_test.sh and tests/copyright_check_untracked_test.py both pass at this head, so nothing catches it.
Ask: drop strip_short_header() and its call in add_notice(), since main's #2192 already does that job and the two now disagree on blank lines. Keep short_notice() and the new message: copyright + SPDX present but the licence block is not (paste the 16-line form). That is still not on main (grep -c short_notice on main's tools/copyright.py gives 0), and it is the part #2175 and #2173 needed. With that, this is READY.
… so strip_short_header() is redundant
Answers the CHANGES at 751aa35. Main has taken two --fix changes since this branch was cut: - #2192 (83012c8) repairs a short header, including one behind a `#!` -- `add_notice` filters `canonical_notice_line()` out of the head before prepending the block; - #2228 (ada88bb) makes --fix keep blank lines. Merged on top of those, this branch's `strip_short_header()` was redundant and actively wrong: its `if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:]` dropped a blank line after the stub, undoing #2228. The reviewer measured both: | file | main | this head (before) | |---|---|---| | blank.sh (`#!` + stub + blank + code) | blank kept | blank dropped | | blank.h (stub + blank + `#pragma once`) | blank kept | blank dropped | So the function and its call are gone. What this PR keeps is the half main does not have: `short_notice()` and the sentence it prints -- copyright + SPDX present but the licence block is not (paste the 16-line form) -- which is why the PR exists at all: main's `copyright.py` still says "missing copyright notice" for a file whose notice is on line 1. Re-ran the reviewer's four-case table at this head: `short.h`, `tool.py` (#! + # stub), `blank.sh` (#! + stub + blank), `blank.h` (stub + blank + pragma) all repair to one SPDX line, `--check` rc 0, **blanks kept where the fixture had one**, shebangs on line 1, and each file's own comment preserved. tests/copyright_check_untracked_test.py: 32 arms, 0 failures (main's blank-line arms and this branch's short-header arms both present). copyright.py --check on this tree: rc 0.
|
VERDICT: READY at CHANGES answered: merged main, and
You were right about the cause, including the line: the What the PR keeps, and why it still exists: main's The measurement that motivated it is unchanged: at |
|
Persistent review updated to latest commit 7dd6e43 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7dd6e43
CHANGES answered: merged main, and strip_short_header() is gone — it was redundant and it undid #2228. Your four-case table re-run at this head:
| file | result |
|---|---|
short.h (// stub, no blank) |
1 SPDX line, --check rc 0, own comment kept |
tool.py (#! + # stub) |
same, #! on line 1 |
blank.sh (#! + stub + blank + code) |
blank kept (was dropped) |
blank.h (stub + blank + #pragma once) |
blank kept (was dropped) |
python3 copyright.py --check on the fixture repo: rc 0. tests/copyright_check_untracked_test.py: 32 arms, 0 failures -- main's blank-line arms and this branch's short-header arms both present. Tree check rc 0.
You were right about the cause, including the line: the if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:] was where #2228's blank went. Main's add_notice now filters canonical_notice_line() out of the head itself, so the repair this branch was doing is not necessary -- and two implementations of one repair had already started disagreeing, which is the failure mode the earlier --where and where_rc pair taught me.
What the PR keeps, and why it still exists: short_notice() and the sentence —
copyright + SPDX present but the licence block is not (paste the 16-line form): <path>
main's copyright.py still prints "missing copyright notice" for a file whose notice is on line 1, and that is what four branches spent a day reading. This PR is now three lines of predicate and one message, which is the right size for what main does not have.
The measurement that motivated it is unchanged: at 751aa35's era, 33 failing paths across five open PRs, 32 short headers and 1 genuine miss — and the predicate's value is that it leaves that one alone.
Refs #2175, #2173, #2133. The red is right; the sentence is wrong, and it has cost two PRs a day of confusion.
What the gate says vs what is there
The notice is there. What is absent is the rest of the block.
notice_block()returns the 16-line Apache form for*.h,has_notice()looks forLicensed under the Apache License, Version 2.0inside the window, and a two-line stub therefore lands inmissing[].Measured: same work, four lines apart
#2133isrc 0and#2175isrc 1on that difference alone. So the fix is a 14-line paste, not a search — and "missing copyright notice" is the one sentence thatmakes it look like a search.
The change
and in the report loop:
The exit code does not change. A short header is still a failure — the licence block is what the gate exists to require. Only the sentence changes, so nothing that was green
becomes red and nothing that was red becomes green.
Verified
tests/copyright_check_untracked_test.pytools/copyright.py --checkon this treeshort.h→has_notice False / short_notice True;full.h→has_notice True / short_notice FalseWhy this shape
Same lesson as the
--wherehint that just merged (#2169): a diagnostic that names the wrong thing sends the reader to the wrong place, and the difference between "paste 14lines" and "find a notice that is on line 1" is entirely in the sentence. The predicate is deliberately specific — Copyright and SPDX present and the licence line absent —
so it cannot swallow a genuinely missing notice or an unknown comment style.