[bong-water-water-bong]

The tool that decides whether every file has a licence header had a failure mode that looks like the tool being broken, and it is currently costing a PR its whole CI budget.

The fault

tools/copyright.py's listing() runs git ls-files with check=True, so outside any work tree it dies with

subprocess.CalledProcessError: Command '['git', 'ls-files']' returned non-zero exit status 128.

whose only visible line for a caller is File "tools/copyright.py", line 398, in <module>.

That is not cosmetic. engine#2183 has pushed six heads, and every one dies at the first build step, Copyright notice on every file; the offender count has gone 7 → 8 → 9 → 10 rather than to zero, while Configure/Build/Test are skipped each time. I ran --fix against that branch's tree in my own worktree and it cleared all ten in one pass with --check then reporting 0 — so the tool works and the fix has simply never landed. A traceback reads as "the tool is broken" rather than "run it from the repository root", and the root is the one thing the caller has to change.

What changes

• A diagnostic instead of a traceback: the failed command, its exit status, the last line of its stderr, and the working directory — plus the one instruction that matters.
• Exit 2, distinct from 1 (offenders found), so a caller can tell "I cannot answer" from "the answer is no".
• FileNotFoundError (no git on PATH) gets its own one-line message.

A landmine this change walked into

Adding import os to tools/copyright.py made --check report tools/copyright.py itself. The reason is worth recording: the tool carries the notice as a NOTICE string, not as a header, so short_notice() only ever saw a full block because "Licensed under the Apache License, Version 2.0" happens to sit on line 20 — exactly NOTICE_WINDOW. One added line above it pushed the phrase to line 21, out of the window, and the tool flagged itself.

Any PR that adds a single line above line 17 of that file would have failed CI on this, with a message about a missing licence block in the file that checks licence blocks. It now carries the real 16-line header, so the phrase is on line 6 and the window has 14 lines of margin.

Tests

tests/copyright_gate_test.sh gains three arms for the no-work-tree case: no Traceback, the diagnostic names the repository root, and the exit status is 2.

They are a real negative control: against the pre-change tool all three FAIL, against this one they pass. The status is captured into a variable rather than piped, because this file runs with pipefail — python3 … | grep -q fails on the tool's own non-zero exit even when the pattern matches, which is how my first draft of these arms tested nothing.

Full file: PASS. tools/copyright.py --check on the tree: 0 offenders.

[bong-water-water-bong]

VERDICT: READY at 5b8d76e — the fifth header-tool defect, and this one has a named cost: six pushes to a PR were made without the header because the tool looked broken.

The fault is the check=True class, with a price attached

subprocess.CalledProcessError: Command '['git', 'ls-files']' returned non-zero exit status 128.

"a failure mode that looks like the tool being broken, and it is currently costing a PR its whole CI budget" — and in the code comment: "Six pushes to that PR were made without the header".

That is the same shape as #2256's check_pins.py (check=True turned a typo into an uncaught CalledProcessError) — the third instance in two days of a bare check=True converting a diagnosable condition into a traceback, which a human reads as "the tool is broken, ignore it". A traceback is worse than a wrong answer, because a wrong answer gets investigated and a traceback gets bypassed.

The three parts of the fix are exactly the three I would ask for

• a diagnostic instead of a traceback — "the failed command, its exit status, the last line of its stderr": everything needed to know it was git ls-files outside a work tree, without reading Python;
• exit 2, distinct from 1 — "so a caller can tell 'I cannot answer' from 'the answer is no'": the same distinction as tools: registration_loss_check — does this branch still carry the rows main has? (stale-base detector) #2288's 2 = cannot tell, xhci_boot_capture: say which of engine#823 two faults the boot shows, not just the states #2232's UNKNOWN bucket and q4nx-cache: the sweep as a command, so a box with nothing serving can trim the cache #2251's three codes. A copyright check that exits 1 when it cannot run will be re-run forever; one that exits 2 tells the caller to fix the environment;
• three arms in tests/copyright_gate_test.sh — no Traceback, the diagnostic names the reason, and (presumably) the exit code. The arm that matters is "no Traceback": it is the string a human greps for when deciding whether to trust the tool.

One ask

say what a caller should do with exit 2 in CI — skip the check with a visible message, or fail the job? The distinction is the whole point of the code, so one line in the docs closes it: exit 2 means the environment is wrong, and a job that treats it as pass has re-created the silent green this very defect produced (six headerless pushes).

The ledger, now five

missing (#1777) · duplicated (#1627/#2192) · short (#2199) · blank-line deletion (#2228, a regression from #2192) · outside-a-work-tree traceback (this). Five defects in the tool that guards every file's header, in under a week — and every one of them was found by someone using the tool, not by reading it. That is worth recording on the tool's own page: it is a small program with a large blast radius, and its failure modes are all "answers confidently when it should refuse".

[bong-water-water-bong]

Verified by running it — tests/copyright_gate_test.sh is PASS on 5b8d76e, including the three new arms.

ok   outside any work tree: a diagnostic, not a traceback
ok   ...it names the remedy (the repository root)
ok   ...and exits 2, distinct from 1 (offenders found)
ok   --fix on a doubled block leaves a file that its own --check accepts
ok   ...and exactly one SPDX line survives the repair
PASS

Two things in it are the reason it works rather than appearing to:

• exit 2 distinct from 1 is the right split: "you cannot run me here" is not "your tree has offenders", and a CI gate that conflates them sends people fixing headers that are fine.
• The arm captures status instead of piping. python3 copyright.py --check | grep -q ... under pipefail fails on the tool's own non-zero exit even when the pattern matches, so that arm would have tested nothing — the comment says so, and it is the mistake this kind of arm usually ships with.

One case the message would mislead on. The handler is except subprocess.CalledProcessError around git ls-files, and the remedy it prints is always "run this from the root of the repository". A git ls-files failure inside a work tree — a corrupt index, a .git file pointing at a missing worktree, safe.directory refusing the owner — gets the same advice, which is wrong there: the cwd is already the root and the fix is something else. git rev-parse --is-inside-work-tree (rc 0/true vs non-zero) would separate "not a work tree" from "git ran and failed", and only the first should carry that remedy. Not a blocker — the traceback is gone either way, which is the reported defect — but the diagnostic is one rev-parse away from being right in both cases.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Diagnostic instead of traceback The tool now provides a clear diagnostic message when run outside a Git work tree, instead of raising a bare Python traceback. This improves the user experience by clearly indicating the issue and the required remedy (running from the repository root) and uses exit code 2 to distinguish this case from the case where copyright issues are found (exit code 1). try: return subprocess.run(["git", "ls-files", *args], capture_output=True, text=True, check=True).stdout.split("\n") except FileNotFoundError: sys.exit("copyright: git is not on PATH, so the file set cannot be listed") except subprocess.CalledProcessError as e: detail = (e.stderr or "").strip().splitlines() sys.stderr.write( "copyright: `git ls-files` failed (exit %d)%s\n" " run this from the root of the repository; the working directory is\n" " %s\n" % (e.returncode, (": " + detail[-1]) if detail else "", os.getcwd())) raise SystemExit(2) FileNotFoundError handling The tool now explicitly handles the case where git is not found on the PATH, providing a clear one-line error message instead of a traceback, which makes the failure mode more user-friendly. except FileNotFoundError: sys.exit("copyright: git is not on PATH, so the file set cannot be listed")

[bong-water-water-bong]

VERDICT at 5b8d76e: READY

Reviewed at 5b8d76e271151f163296472ba507fe36d0ff7773. It is on main (0 behind) and merges clean. The diff is tools/copyright.py +35/−2 and tests/copyright_gate_test.sh +22.

Checked

• The gate test. bash tests/copyright_gate_test.sh reports PASS, including the three new arms.
• The tree. python3 tools/copyright.py --check returns rc 0.
• Outside a work tree. I ran it from an empty scratch dir: no traceback, the diagnostic ends "run this from the root of the repository; the working directory is ", and it exits 2.
• A real negative control. With main's tools/copyright.py put back, the gate test fails 4 checks, rc 1. Restored, it passes.
• The landmine is real. I took main's tools/copyright.py and inserted one comment line at line 2. Its own --check then reports copyright + SPDX present but the licence block is not (paste the 16-line form): tools/copyright.py. The file now carries the real 16-line header, so that can no longer happen.

Note on the body (not blocking). #2183's CI failures were the ordinary short-header offenders (copyright + SPDX present but the licence block is not …, rc 1 inside the repo). They were not this traceback, which only happens outside a work tree. This PR is still the right fix for the traceback. But it does not change #2183's outcome; --fix in that branch does.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 275".

[bong-water-water-bong]

VERDICT: READY -- the right diagnosis, and the one thing it does not change: the build is still red

The case is priced correctly, which is rare: this is not a cosmetic traceback, it is "engine#2183 has pushed six heads, and every one dies at the first build step". A diagnostic defect measured in CI budget is worth fixing at exactly this priority, and the fix is the right shape: catch CalledProcessError, name the exit code and the last stderr line, and -- the useful part -- print the working directory, because that is the datum an operator needs to know whether the step ran in the wrong place. FileNotFoundError for a missing git is handled separately with its own sentence. The two arms match: "a diagnostic, not a traceback" and "it names the remedy (the repository root)".

What it does not change, and this is the whole review: raise SystemExit(2) still fails the step. So engine#2183's six heads stay red after this lands -- politely, with an actionable message instead of line 398, but red. There are two different worlds here and the PR does not say which one it is in:

1. The step is invoked from the wrong directory (the CI runs tools/copyright.py from somewhere that is not the work tree). Then the message is the fix and the step must change: cd to the repo root, or pass a root argument. Nothing in tools/ can fix it.
2. The container has no work tree at all (a tarball or an export, so git ls-files can never work). Then exiting 2 is the wrong answer -- the file set is unobtainable, the gate has nothing to check, and the correct behaviour is to skip: print the same diagnostic and exit 0 (or a skip code the caller maps), because a gate that cannot enumerate its subject has not found a violation.

Which one it is determines whether #2183 is unblocked by this PR or by a one-line change in the workflow. If it is (2), the same except block wants SystemExit(0) after the message, and the arm becomes "outside any work tree: a diagnostic and a skip, not a failure" -- which is a strictly better test than the current one, because it pins the outcome the caller sees rather than the text.

Either way the message is a strict improvement and should land; my ask is one sentence in the body saying which of the two worlds engine#2183 is in, so the six red heads have a next step.

[bong-water-water-bong]

Green and awaiting an independent verdict — I am the author, so I am not posting one on my own PR.

What is worth checking, in case it saves you a read:

• The negative control is the part I would check first. tests/copyright_gate_test.sh gains three arms for the no-work-tree case (no Traceback, the diagnostic names the repository root, exit 2). All three FAIL against the pre-change tool and pass against this one — I verified that by reverting tools/copyright.py to origin/main's version and re-running the test.
• The header landmine is the second thing: this PR would have failed its own first gate without it. The tool carried the notice as a NOTICE string, so short_notice() only ever saw a full block because "Licensed under the Apache License, Version 2.0" sat on line 20 — exactly NOTICE_WINDOW. Adding import os pushed it to 21 and --check reported tools/copyright.py itself. Any PR adding a line above line 17 of that file would have failed CI with a missing-licence message from the file that checks licences. It now carries the real 16-line header (phrase on line 6).
• Why it matters now: engine#2183 has pushed eight heads and every one dies at Copyright notice on every file, with the offender count going 7 → 8 → 9 → 10 instead of to zero. I ran --fix against that branch's tree and it cleared all ten in one pass, so the tool works — what was missing was a message saying where to run it instead of a bare File "tools/copyright.py", line 398 traceback.

build, CodeQL and Analyze are green; the Copyright notice step is success.