[GavinLucas]

The hygiene check reports DFS-2016 in Unicode-DFS-2016 as a tracker key. Unicode-DFS-2016 is the SPDX name of the licence Unicode's data files are under. A Debian copyright file must spell it exactly when the binary links crates that carry Unicode tables, as quaffed's now does through Ruff. So every pull request touching that file fails the check.

What changes

• DFS-\d+ joins NOT_A_TRACKER_KEY, the list of external standards the check does not report, with a comment saying what it is.
• tests/test_internal_references.py loads the shipped script. It checks that the licence name and two existing exceptions are not reported, and that XY-22 and ABC-1 still are.

Tested

• All 87 tests pass locally.
• With the checker change stashed, the two Unicode-DFS-2016 cases fail and the other three pass.

Not done, or open

• A tracker project whose key starts DFS would now go unreported. The check matches only the DFS-2016 part, so it cannot see the Unicode- in front.
• quaffed picks this up only once the v2 tag moves to include it.

[github-actions]

Claude review: approve

Summary

The change adds DFS-\d+ to NOT_A_TRACKER_KEY so that the SPDX identifier Unicode-DFS-2016 in a Debian copyright file is no longer reported as a tracker key. It also adds tests/test_internal_references.py, which loads the shipped script and checks that some external standards pass and that real keys are still reported. The fix is correct and nothing here should block a merge.

I followed both regexes by hand for each test line:

• In Unicode-DFS-2016, TRACKER_KEY matches DFS-2016, because the hyphen before D is a word boundary. NOT_A_TRACKER_KEY.search("DFS-2016") then excludes it.
• GPL-3+, UTF-8, RFC-3629 and ISO-8859 are excluded the same way.
• Unicode-3.0 and Expat never match, because they have lowercase letters.
• QU-22 and ABC-1 match and are not excluded. They produce copyright:1: tracker key …, which is what the test expects, since relative_to(tmp_path) gives copyright.

The new comment gives a reason for the exception, not a recorded state, so it is fine. The statement that "the licence case fails without the change" is in the commit message, which is the right place for it.

Findings

1. Preference: the exception is wider than the evidence for it. actions/repo-hygiene/check-repo-hygiene.py:81-83

   • The comment above the list says each entry "is a real external standard". Here the standard is Unicode-DFS-<year>, but the exception covers any DFS-<n>.
   • Failure case: a repository using a Jira project with the prefix DFS would have DFS-123 silently ignored everywhere. That is the "way to quiet a genuine hit" the comment rules out.
   • NOT_A_TRACKER_KEY only sees the matched span (match.group(0) at line 409), so it cannot see the Unicode- before it. Narrowing the exception would mean checking the line just before match.start() for Unicode-, or adding a lookbehind to an exclusion step that runs over the whole line.
   • FLEX-\d+ and GPL-\d+ already accept the same kind of risk, so this can be a follow-up rather than a blocker.

2. Maintainability: the new test file contains text the check reports. tests/test_internal_references.py:39-40

   • QU-22 and ABC-1 are the exact shape the check exists to keep out of a public repository.
   • The script removes itself from the scan (SELF, lines 126-131, 215) so that this repository's own text "would otherwise read as leaks". The README (around line 246) also shows running the script from this clone.
   • This test file gets no such exclusion. Before this change, the script was the only tracked file with a key-shaped string the check would report; now there are two lines.
   • I can't tell from here whether QU is a real project prefix. If it is, the fixture is itself the kind of leak this check exists to stop (SK.9.1).
   • Suggested fix: build the keys at runtime, for example "QU" + "-22", or use a prefix that is clearly made up and assembled the same way.
   • Mitigating facts: no workflow runs repo-hygiene on this repository, and a self-run would exit 2 anyway because there is no AGENTS.md or detail directory.

3. Preference: the hygiene() loader is now in two places. tests/test_internal_references.py:14-20 copies tests/test_slack_workflows.py:261-267 exactly. A shared fixture in tests/conftest.py would keep them from drifting apart, for example if the script is moved or renamed.

4. Note, nothing to change: the tests cover part of the exception list. The commit message says "Tests now cover that list", but the parametrised cases cover 5 of the 13 exceptions (DFS, GPL, UTF, RFC, ISO). That is enough for this change. I'm noting it only so that no one reads the test file as covering the whole list.

What I checked and what I didn't

Checked:

• The full diff and the surrounding code in check_no_internal_references, the self-exclusion, and the instruction and detail-layer checks.
• The shared-behaviour job in premerge.yaml, which runs tests/ and so picks up the new file.
• The README's local-run section.
• New and edited lines for recorded state; there is none.

Not checked:

• I couldn't run pytest or the script here because the commands were not approved. The results above come from following the regexes by hand, not from a test run.
• The repository has no CLAUDE.md or AGENTS.md, so there were no instruction files to review against.

Declined

None.

---

full review of 1ed475332618..cf2dfd91acfc. Run.

[github-actions]

Claude review: approve

Summary

The change adds DFS-\d+ to NOT_A_TRACKER_KEY (actions/repo-hygiene/check-repo-hygiene.py:81-83). With it, the SPDX name Unicode-DFS-2016 in a Debian copyright file is no longer reported as a tracker key. It also adds tests/test_internal_references.py, which loads the shipped script and checks two things: that some external standards are not reported, and that real keys still are. The fix is correct and I found nothing that should block a merge.

Since the last review, the branch was rewritten into one commit. The only change to the content is that the fixture key QU-22 is now XY-22.

I followed both regexes by hand against the scan loop at check-repo-hygiene.py:407-411:

• Unicode-DFS-2016: TRACKER_KEY matches DFS-2016, because the hyphen before D is a word boundary. NOT_A_TRACKER_KEY.search on that span then excludes it.
• GPL-3+, UTF-8, RFC-3629, ISO-8859: these are excluded the same way. For GPL-3+, the \b falls between 3 and +, and the (?!\.\d) lookahead does not stop the match.
• Unicode-3.0 and Expat: neither matches, because the lowercase letters stop \b[A-Z]{2,10} from starting a match.
• XY-22 and ABC-1: both match and neither is excluded. Each produces copyright:1: tracker key …, which is what the test expects, because relative_to(tmp_path) gives copyright.

The new comment gives a reason (the SPDX name must be spelled exactly), not a recorded state. The note that "the licence case fails without the change" is in the commit message, which is the right place for it.

Findings

1. Preference: the exception is wider than the evidence for it. actions/repo-hygiene/check-repo-hygiene.py:81-83

   • The comment says each entry "is a real external standard". The evidence here is the name Unicode-DFS-<year>, but the pattern exempts every DFS-<n>.
   • Failure case: a repository whose Jira project uses the prefix DFS would have DFS-123 ignored everywhere, with no warning. That is the "way to quiet a genuine hit" the comment rules out.
   • Why it is not a one-line fix: NOT_A_TRACKER_KEY only sees the matched span (match.group(0), line 409), so it cannot see the Unicode- before it. Narrowing the exception means checking line[:match.start()] for a trailing Unicode-, or adding an exclusion step that runs over the whole line.
   • FLEX-\d+ and GPL-\d+ already accept the same kind of risk, so this can be a follow-up.
   • Carried over from the last review; the code is unchanged.

2. Maintainability: the test file contains text the check reports. tests/test_internal_references.py:39-40

   • The script leaves itself out of the scan (SELF, lines 131 and 215), because its own text would otherwise read as leaks. This test file gets no such exclusion.
   • XY-22 and ABC-1 are exactly the shape the check exists to keep out of a public repository. A search over the repository shows that these two lines are now the only key-shaped text it would report outside the script.
   • Renaming QU-22 to XY-22 answers the narrower question from last time, which was whether QU is a real project prefix. It does not stop the line from being reported, and a two-letter prefix can never be shown not to be real.
   • Suggested fix: build the keys at runtime, for example "XY" + "-22". The test then still checks the same behaviour, but the file has nothing the check would report.
   • Mitigating facts: README line 286 says the hygiene check does not run on this repository. A self-run (README line 246) would exit 2 anyway, because the instruction layer is missing.
   • Partly addressed since the last review.

3. Preference: the hygiene() loader is now in two places. tests/test_internal_references.py:14-20 is an exact copy of tests/test_slack_workflows.py:261-267. A shared fixture or helper in tests/conftest.py would mean a move or rename of the script only has to be updated once.

   • Carried over from the last review; unchanged.

4. Note, nothing to change: the tests cover part of the exception list. The commit message says "Tests now cover that list". The parametrised cases cover 5 of the 13 exceptions (DFS, GPL, UTF, RFC, ISO). That is enough for this change; I'm noting it only so no one reads the test file as covering the whole list.

What I checked and what I didn't

Checked:

• The full diff and the surrounding code: the scan in check_no_internal_references, the SELF exclusion and the regex definitions.
• The README's local-run section and its statement that the check does not run here.
• premerge.yaml, whose test job imports the hygiene script.
• A search of the repository for key-shaped strings outside the script.
• Every new and edited line for recorded state; there is none.

Not checked:

• I could not run pytest, or the old script against the licence line, because those commands were not approved. The results above come from following the regexes by hand, not from a test run.
• The repository has no CLAUDE.md or AGENTS.md, so there were no instruction files to review against.

Declined

None. No replies declined any earlier finding.

---

full review of 1ed475332618..570ae6389839. Run.

[github-actions]

Claude review: approve

Resolved

• Finding 1, the DFS-\d+ exception was too wide (check-repo-hygiene.py:80-88, :415-417). DFS-\d+ is gone from NOT_A_TRACKER_KEY. The new PART_OF_A_STANDARD = \bUnicode-$ is checked against line[:match.start()]. I traced it by hand:

  • License: Unicode-DFS-2016: the text before the match is License: Unicode-, so it matches and the line is excluded.
  • Fixes DFS-2016: no match, so it is reported.
  • MyUnicode-DFS-2016: there is no word boundary before U, so the check does not match and the key is reported.

  The tests cover the last two cases.

• Finding 2, the test file held key-shaped text (tests/test_internal_references.py:29-33). The keys are now built at run time ("XY" + "-22" and so on), and the module docstring says why.

• Finding 3, the hygiene() loader was in two places. It now lives once in tests/conftest.py as load_hygiene. Both test files use it.

Still open

• Finding 4, a note rather than a change: the tests cover only some of the entries in NOT_A_TRACKER_KEY.

Declined

None.

New

1. Preference: import importlib.util is now unused in tests/test_slack_workflows.py:9. The only use of it moved to conftest.py.
2. Preference: hygiene() in tests/test_slack_workflows.py:263 is now a one-line wrapper around load_hygiene(). The call sites could use load_hygiene directly. Neither point blocks a merge.
3. Note, nothing to change: from conftest import load_hygiene works under pytest's default rootdir and sys.path handling, because tests/ has no __init__.py. If the tests are ever made a package, the import will break.

I found no recorded-state claims in the new or edited comments. They state rules and reasons.

Checked:

• The full delta, including the regex logic traced by hand against the scan loop.
• The test parametrisation.
• The leftover imports.

Not checked: I could not run pytest, because the command was not approved. The results above come from reading the code and tracing the regexes by hand.

---

delta review of 570ae6389839..0e67a2233cea. Run.

[GavinLucas]

On the review of 570ae63, everything is in 4d94630:

1. Narrowed. DFS-\d+ is no longer a standard on the list. The check now reads the text just before a match, and lets it through only after Unicode- (PART_OF_A_STANDARD). So a project whose keys start with DFS is still reported, and tests cover DFS-2016 alone and after MyUnicode-.
2. Fixed. The fixture keys are assembled at run time, so the file holds none for the check to find.
3. Fixed. The loader is now in tests/conftest.py, and the Slack tests use it too.
4. Fixed. The commit message now says the tests cover the licence name and three of the existing exceptions, not the whole list.

All 89 tests pass locally. With the checker change stashed, the two licence cases fail and the other five pass.

[github-actions]

Claude review: approve

This is a full review of 1ed4753..4d94630, which is the single commit 4d94630. The repository has no CLAUDE.md or AGENTS.md. I reviewed against the README, in particular its recorded-state rule under "The Claude review" and its "Changing anything here" section.

Findings

1. Maintainability: the new comment says only the licence name is let through, but the code lets through any key after Unicode-.

   • Where:
     • actions/repo-hygiene/check-repo-hygiene.py:84-88. The comment says "Only that name is let through".
     • tests/test_internal_references.py:31, which says the same.
   • What the code does: PART_OF_A_STANDARD (\bUnicode-$) checks only the text before the match. It never looks at the matched key. Tracing Unicode-XY-22 against the loop at :413-418:
     • TRACKER_KEY matches XY-22 at offset 8.
     • line[:8] is Unicode-, so the key is not reported.
   • What this means: Any key written straight after Unicode- is let through. The DFS-prefix case the comment cares about is still covered: a bare DFS-2016, or one after MyUnicode-, is reported, and the tests check both. In practice it also lets through Unicode-DFS-2015, which is the other real SPDX name.
   • Fix, either of:
     • Reword both comments to say "only after Unicode-", which is also what the commit message says.
     • Tighten the code so that it also requires span to match DFS-\d+.
   • This is not a bug in practice, but the comment states a guarantee the code does not give.

2. Preference: import importlib.util is now unused in tests/test_slack_workflows.py:9. Its only use moved into tests/conftest.py. (This was raised in the previous round and is still there.)

3. Preference: hygiene() at tests/test_slack_workflows.py:263-264 is now a one-line wrapper around load_hygiene(). Its two call sites at :289 and :299 could call load_hygiene() directly. (Raised before; still there.)

4. Note, nothing to change: from conftest import load_hygiene works only because tests/ has no __init__.py, which puts tests/ on sys.path under pytest's default rootdir-relative import mode. If tests/ is ever made a package, the import will break. (Carried forward.)

5. Note: the tests cover four of the twelve entries in NOT_A_TRACKER_KEY: GPL, UTF, RFC and ISO. That is enough for this change. The entries have no direct tests of their own. (Carried forward.)

6. Note on the commit message only: it says the tests check "three of the existing exceptions". The third licence-case line checks four: UTF-8, RFC-3629 and ISO-8859, plus GPL-3 on the second line. Nothing in the tree depends on this.

Recorded state: none of the new or edited comments and docstrings make recorded-state claims. They state rules and reasons: why DFS-\d+ is not in the list, and why the test keys are built at run time. The "two licence cases fail without the change" record is in the commit message, where it belongs.

Checked:

• The whole diff, including how PART_OF_A_STANDARD interacts with TRACKER_KEY and NOT_A_TRACKER_KEY in the scan loop.
• Every parametrised test case, traced by hand:
  • GPL-3+: GPL-3 matches and is excused by NOT_A_TRACKER_KEY.
  • Unicode-3.0: has no upper-case prefix, so it does not match.
  • MyUnicode-DFS-2016: has no word boundary before U, so it is reported.
  • Every test expects an exact copyright:1: tracker key … finding.
• That findings() passes the arguments in the order check_no_internal_references(root, tracked, files) expects, and that relative comes out as copyright.
• That test_action_pins.py has no third copy of the loader.

Not checked: I could not run pytest or a Python snippet, because neither command was approved. Everything above comes from reading the code and tracing the regexes by hand.

Declined

None.

---

full review of 1ed475332618..4d94630ac85b. Run.

[github-actions]

Claude review: approve

Claude review: approve

This is a delta review of 4d94630..2991357, the single commit 2991357. It removes the unused import importlib.util and the one-line hygiene() wrapper from tests/test_slack_workflows.py. Its two call sites now call load_hygiene() directly. I read the diff and grepped the file. I did not run pytest.

Resolved

• Previous finding 2 (preference): the unused import importlib.util is gone.
• Previous finding 3 (preference): hygiene() is gone, and both call sites use load_hygiene() directly.

Still open

• Previous finding 1 (maintainability): the comments at actions/repo-hygiene/check-repo-hygiene.py:84-88 and tests/test_internal_references.py:31 say only the licence name is let through, but the code lets through any key written after Unicode-. This commit does not touch either file.
• Notes 4, 5 and 6 from the previous review are carried forward, and none needs a change:
  • The from conftest import load_hygiene import relies on tests/ having no __init__.py.
  • Only four of the twelve NOT_A_TRACKER_KEY entries have tests.
  • The earlier commit message says "three of the existing exceptions" where four are checked.

Declined
None.

New
None. A grep of tests/test_slack_workflows.py finds no remaining use of hygiene() or importlib. The new commit adds no comments or docs, so there is no recorded-state claim to check.

---

delta review of 4d94630ac85b..2991357410a4. Run.

[github-actions]

Claude review: approve

This delta covers 2991357..ccc6291. It is one commit that removes a blank line in tests/test_internal_references.py and one in tests/test_slack_workflows.py. Both blank lines separated from conftest import load_hygiene from the third-party imports above it. I read the diff only. I did not run ruff or pytest.

Resolved

• None. The previous review's findings were not about import grouping.

Still open

• Maintainability: the comments at actions/repo-hygiene/check-repo-hygiene.py:84-88 and tests/test_internal_references.py:31 say only the licence name is let through. The code lets through any key written after Unicode-. This commit does not touch either file.
• Notes carried forward, none needing a change:
  • The from conftest import load_hygiene import relies on tests/ having no __init__.py.
  • Only four of the twelve NOT_A_TRACKER_KEY entries have tests.
  • An earlier commit message says "three of the existing exceptions" where four are checked.

Declined

• None.

New

• None. Because conftest is not a package, ruff should class it with pytest and yaml, so merging the sections matches ruff's default isort output. The commit adds no comments or documentation, so there is no recorded-state claim to check.

---

delta review of 2991357410a4..ccc62914f53d. Run.

[GavinLucas]

On the full review of 4d94630:

1. Fixed in 1f6229c. The code now does what the comment says. The line up to the end of the key must end in Unicode-DFS- and a number, so a key after Unicode-, such as Unicode-XY-22, is reported, and a new test checks that. The test fails without the change. quaffed passes the check with this version.
   2 and 3. Fixed in 2991357. The unused import and the wrapper are gone.
   4 and 5. Noted, no change.
2. The tally in the earlier commit message is off by one, as you say. Nothing depends on it.

Separately, ccc6291 sorts the tests' imports as ruff's default isort rule does.

[github-actions]

Claude review: approve

Resolved

• The comment and code mismatch from the previous round is fixed. check-repo-hygiene.py:84-89 and tests/test_internal_references.py:31 say only the licence name is let through. PART_OF_A_STANDARD is now \bUnicode-DFS-\d+$ and is matched against line[: match.end()] (check-repo-hygiene.py:417). That text ends in Unicode-DFS-<digits> only when the key is the DFS-<digits> part of that exact name. A key after Unicode- such as Unicode-XY-22 is reported, and so is MyUnicode-DFS-2016, because \b fails inside the word.

Still open

• The from conftest import load_hygiene import relies on tests/ having no __init__.py. It is a note, not a change request.
• Only four of the twelve NOT_A_TRACKER_KEY entries have tests. It is a note, not a change request.
• An earlier commit message says "three of the existing exceptions" where four are checked. The author acknowledged this and nothing depends on it.

Declined

• None.

New

• None.
  • I traced the regex change by hand. It anchors with $ on the line up to the key's end, so it is correct. TRACKER_KEY takes the longest digit run, and its (?!\.\d) lookahead already excludes keys followed by a decimal point.
  • The new test case ("Unicode-", "XY-22") exercises the case the old code let through. The test loop wraps the pair into before + key, and the expected finding is the key alone. This matches the neighbouring cases.
  • The new comment lines record no state, only the rule and its reason.
  • I read the diff and the surrounding code. I did not run pytest or ruff.

---

delta review of ccc62914f53d..1f6229c8cc09. Run.