[PR #269] [MERGED] fix(public-repo-hygiene): anchor TICKET_RE on explicit boundaries and rebuild the ticket allowlist #272

New issue

Closed

opened 2026-10-26 22:37:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:58 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/269
Author: @mattmillerai
Created: 9/8/2026
Status: ✅ Merged
Merged: 9/8/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8701-ticket-re-underscore-boundaries

📝 Commits (2)

53d367f fix(public-repo-hygiene): anchor TICKET_RE on explicit boundaries and drop the unmatchable allowlist entries
16b30f6 fix(public-repo-hygiene): bound the ticket body to ASCII digits and drop the prefix-cleared allowlist entries (BE-8701)

📊 Changes

6 files changed (+195 additions, -38 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+3 -2)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+55 -22)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+131 -9)
📝 .github/workflows/public-repo-hygiene.yml (+4 -3)
📝 README.md (+1 -1)
📝 docs/callers/public-repo-hygiene.md (+1 -1)

📄 Description

\b refuses to fire next to _, because _ is a word character. That made
category 1 of the public-repo hygiene checker miss the most ordinary spellings
a ticket id is actually written in — BE-1234_design, feature/BE-1234_fix,
_BE-1234_, FOO_BE-1234 — on both the contents surface and the tracked-path
surface. This replaces \b with explicit lookarounds and rebuilds the
category-1 allowlist around what the shape can actually match.

What changed

TICKET_RE — \b[A-Z]{2,6}-\d{2,6}\b → (?<![A-Za-z0-9])[A-Z]{2,6}-\d{2,6}(?![A-Za-z0-9]).

Three properties of the excluded class are load-bearing and each is commented
in place, because each is a way a later "tidy-up" silently undoes this:

_ is deliberately absent. Putting it back ([A-Za-z0-9_]) makes the
assertion fail next to an underscore again, which is semantically identical
to \b — it would fix nothing while looking like a fix.
- is deliberately absent too. Adding it would stop AES-128 matching
inside AES-128-CBC, and would break the whole TICKET_ALLOWED_PREFIXES
carve-out, which depends on CVE-2021-44228 presenting here as CVE-2021.
The class is ASCII-only, so a non-ASCII letter is a boundary. \b treats
a Cyrillic homoglyph as a word character, so gluing one to the front of an id
was a way past the check.

The new pattern is a strict superset of the old: \b fires only when the
neighbour is outside \w = [A-Za-z0-9_] (plus Unicode word chars), and every
such character is also outside [A-Za-z0-9]. So nothing that matched before
stops matching.

TICKET_ALLOWLIST — removed UTF-8, OAUTH-2, IPV-4, IPV-6,
X-25519. The shape needs 2–6 letters and 2–6 digits, so none of the five
could ever fire; they read as coverage while being unreachable. Added the two
matchable near-neighbours that have no public namespace behind them:
AES-192, SHA-224.

Docs — the module docstring now states that CVE ids clear by pattern, not
by list; the stale UTF-8 allowlist example is corrected in the checker README,
the reusable workflow's ticket_allowlist input comment, and the caller doc.

Judgment calls

The CVE carve-out was already implemented, and better than what was
specified. The plan called for a new CVE_PREFIX_RE skip inside the
category-1 loop. TICKET_ALLOWED_PREFIXES (CVE, CWE, PEP, RFC, ISO,
UTF) landed on main after that plan was written and already clears CVE ids
by prefix, with no year to rot. Adding a second, narrower mechanism for the same
tokens would be a second place to forget. Kept the prefix mechanism; added the
behavioural coverage the plan asked for instead
(test_a_cve_never_expires_and_never_shadows_a_real_id).

UTF-16, UTF-32, RFC-1123, ISO-8859, ISO-27001 were NOT added as
exact entries, for the same reason — their namespaces (UTF, RFC, ISO)
already clear them. Re-listing them would be redundancy that drifts. Verified
directly rather than assumed:

UTF-16     matches=True  exact_allow=False  prefix_allow=True
UTF-32     matches=True  exact_allow=False  prefix_allow=True
RFC-1123   matches=True  exact_allow=False  prefix_allow=True
ISO-8859   matches=True  exact_allow=False  prefix_allow=True
ISO-27001  matches=True  exact_allow=False  prefix_allow=True

test_matchable_near_neighbours_are_cleared asserts all seven tokens are clean
end-to-end and deliberately does not assert which route cleared them, so the
test survives either implementation.

Two tests corrected rather than extended. The plan said to extend
test_shape_boundaries with the underscore shapes; that test pins an exact
expected list (['BE-12', 'ABCDEF-123456']) which the plan also required be
left alone, so the new shapes went into three sibling tests instead —
underscore-matches, hyphen-still-a-boundary, glued-alphanumeric-still-not.
Separately, test_allowlisted_acronyms_in_a_path_are_clean used src/UTF-8/ to
prove the built-in allowlist reaches the path surface — but UTF-8 is
shape-proof, so it was passing via the regex, not via the allowlist it stood
for. Switched to SHA-256.

The workflow YAML was touched, against the plan's "no change to the workflow
YAML". The change is one header comment naming UTF-8 as a built-in
allowlist entry, which this PR removes. No input, step, permission or default
moved, and bump-public-repo-hygiene-callers.yml already triggers on the
checker path this PR edits, so no new bump is provoked. Leaving a documented
example that is provably no longer in the list seemed the worse outcome.

Corpus sweep — including the half this does not fix

Old regex vs new, over tracked paths and contents, netting out the exact and
prefix allowlists:

repo	new-only `(token, file)` pairs
`Comfy-Org/github-workflows` (this repo, 1,363 findings at baseline)	0 outside the two files this PR edits
`Comfy-Org/ComfyUI`	0
`Comfy-Org/ComfyUI_frontend`	0

So on three real public trees the loosened boundary introduces zero new
findings — it adds no false-positive load — and the shapes it newly catches
(BE-1234_design and friends) are absent from all three, which is why they were
never noticed. They are pinned by unit test instead. The 4-finding delta in this
repo's own self-scan (1,359 → 1,363) is entirely the new fixture strings this PR
adds to its own test file and code comments.

Verification of the negative claims

Every claim of the form "this entry can never match" was checked empirically
rather than reasoned about:

UTF-8     old=False new=False
OAUTH-2   old=False new=False
IPV-4     old=False new=False
IPV-6     old=False new=False
X-25519   old=False new=False

test_every_allowlist_entry_can_actually_match pins this as an invariant, so
the dead-entry trap cannot reopen when someone adds the next acronym.

Residual

Not fixed here, and worth knowing before this workflow acquires callers:

This repo's own self-scan is RED and stays red — 1,359 findings before,
1,363 after, exit 1. That is pre-existing and expected: github-workflows is
not enrolled in its own public-repo-hygiene workflow (there is no
ci-public-repo-hygiene.yml), and its own conventions put ticket ids in
commit messages, comments and test fixtures throughout. The plan listed
"self-scan stays green" as an acceptance criterion; that criterion is not
met and was never true on main. Deciding whether to enrol this repo (and
with what exclude_paths / ticket_allowlist) is a separate call, not
something this PR should make.
No caller repo was exercised. The plan asked that this land before any
repo enrols, and no caller exists yet, so the change is verified against this
repo's unit suite plus the three-repo sweep above — never against a live
consumer run of the reusable workflow.
The redundant ISO-8601, RFC-2119, RFC-7231, RFC-3339 entries were
left in place. Each is now cleared twice — once exactly, once by its
namespace prefix. They are harmless and the anti-vacuity test passes on them,
but they are the same "second place to maintain" shape this PR argues against
for UTF-16. Deliberately out of scope.
The --scan-text PR-title/body surface is unaffected, verified:
CAT_TICKET is not in SURFACE_CATEGORIES, so category 1 does not run there
at all and the boundary change cannot move that surface's verdict.
Prior work referenced by the plan could not be read. The plan cites an
investigation and a review-thread comment held in a tracker this session has
no access to; only the plan text reproduced in the task brief was available.
The line numbers it quotes are relative to an older revision of the checker,
which has since gained the prefix carve-out, the tracked-path surface and the
PR-text surface — the two divergences documented under "Judgment calls" above
follow from that.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/public-repo-hygiene/tests: 202 run, 201 passed, 1 skipped, 0 failed (195 before this PR); python3 -m unittest discover -s .github/agents-md-integrity/tests: 46 passed, 0 failed; check_agents_md.py --root .: passed with 2 pre-existing warnings; check_public_repo_hygiene.py --root .: 1,359 findings before → 1,363 after, delta accounted for line by line (see corpus sweep); old-vs-new regex sweep over ComfyUI and ComfyUI_frontend: 0 new findings each; yaml.safe_load on the edited workflow: parses.
Deviations: the CVE carve-out was implemented as specified only in effect, not in form (the prefix mechanism already on main is used instead of the proposed CVE_PREFIX_RE); five of the seven proposed allowlist additions were omitted as already-covered-by-prefix and asserted behaviourally instead; the underscore cases went into sibling tests rather than into test_shape_boundaries, whose pinned expectation the plan required be left unchanged; one comment line in the reusable workflow YAML was corrected despite the plan's "no change to the workflow YAML" — all four are argued under "Judgment calls" above. The "self-scan stays green" acceptance criterion is not met and was already false on main; see "Residual".

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/269 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/8/2026 **Status:** ✅ Merged **Merged:** 9/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8701-ticket-re-underscore-boundaries` --- ### 📝 Commits (2) - [`53d367f`](https://github.com/Comfy-Org/github-workflows/commit/53d367fd3faac0985ed293d1faaaabe54e485a22) fix(public-repo-hygiene): anchor TICKET_RE on explicit boundaries and drop the unmatchable allowlist entries - [`16b30f6`](https://github.com/Comfy-Org/github-workflows/commit/16b30f6642086ee10b514bf2f53e5b516b5d5adc) fix(public-repo-hygiene): bound the ticket body to ASCII digits and drop the prefix-cleared allowlist entries (BE-8701) ### 📊 Changes **6 files changed** (+195 additions, -38 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+3 -2) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+55 -22) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+131 -9) 📝 `.github/workflows/public-repo-hygiene.yml` (+4 -3) 📝 `README.md` (+1 -1) 📝 `docs/callers/public-repo-hygiene.md` (+1 -1) </details> ### 📄 Description `\b` refuses to fire next to `_`, because `_` is a word character. That made category 1 of the public-repo hygiene checker miss the most ordinary spellings a ticket id is actually written in — `BE-1234_design`, `feature/BE-1234_fix`, `_BE-1234_`, `FOO_BE-1234` — on both the contents surface and the tracked-path surface. This replaces `\b` with explicit lookarounds and rebuilds the category-1 allowlist around what the shape can actually match. ## What changed **`TICKET_RE`** — `\b[A-Z]{2,6}-\d{2,6}\b` → `(?<![A-Za-z0-9])[A-Z]{2,6}-\d{2,6}(?![A-Za-z0-9])`. Three properties of the excluded class are load-bearing and each is commented in place, because each is a way a later "tidy-up" silently undoes this: - `_` is deliberately **absent**. Putting it back (`[A-Za-z0-9_]`) makes the assertion fail next to an underscore again, which is semantically identical to `\b` — it would fix nothing while looking like a fix. - `-` is deliberately **absent** too. Adding it would stop `AES-128` matching inside `AES-128-CBC`, and would break the whole `TICKET_ALLOWED_PREFIXES` carve-out, which depends on `CVE-2021-44228` presenting here as `CVE-2021`. - The class is **ASCII-only**, so a non-ASCII letter is a boundary. `\b` treats a Cyrillic homoglyph as a word character, so gluing one to the front of an id was a way past the check. The new pattern is a strict **superset** of the old: `\b` fires only when the neighbour is outside `\w` = `[A-Za-z0-9_]` (plus Unicode word chars), and every such character is also outside `[A-Za-z0-9]`. So nothing that matched before stops matching. **`TICKET_ALLOWLIST`** — removed `UTF-8`, `OAUTH-2`, `IPV-4`, `IPV-6`, `X-25519`. The shape needs 2–6 letters **and** 2–6 digits, so none of the five could ever fire; they read as coverage while being unreachable. Added the two matchable near-neighbours that have no public namespace behind them: `AES-192`, `SHA-224`. **Docs** — the module docstring now states that CVE ids clear by *pattern*, not by list; the stale `UTF-8` allowlist example is corrected in the checker README, the reusable workflow's `ticket_allowlist` input comment, and the caller doc. ## Judgment calls **The CVE carve-out was already implemented, and better than what was specified.** The plan called for a new `CVE_PREFIX_RE` skip inside the category-1 loop. `TICKET_ALLOWED_PREFIXES` (`CVE`, `CWE`, `PEP`, `RFC`, `ISO`, `UTF`) landed on `main` after that plan was written and already clears CVE ids by prefix, with no year to rot. Adding a second, narrower mechanism for the same tokens would be a second place to forget. Kept the prefix mechanism; added the behavioural coverage the plan asked for instead (`test_a_cve_never_expires_and_never_shadows_a_real_id`). **`UTF-16`, `UTF-32`, `RFC-1123`, `ISO-8859`, `ISO-27001` were NOT added as exact entries**, for the same reason — their namespaces (`UTF`, `RFC`, `ISO`) already clear them. Re-listing them would be redundancy that drifts. Verified directly rather than assumed: ``` UTF-16 matches=True exact_allow=False prefix_allow=True UTF-32 matches=True exact_allow=False prefix_allow=True RFC-1123 matches=True exact_allow=False prefix_allow=True ISO-8859 matches=True exact_allow=False prefix_allow=True ISO-27001 matches=True exact_allow=False prefix_allow=True ``` `test_matchable_near_neighbours_are_cleared` asserts all seven tokens are clean end-to-end and deliberately does **not** assert which route cleared them, so the test survives either implementation. **Two tests corrected rather than extended.** The plan said to extend `test_shape_boundaries` with the underscore shapes; that test pins an exact expected list (`['BE-12', 'ABCDEF-123456']`) which the plan also required be left alone, so the new shapes went into three sibling tests instead — underscore-matches, hyphen-still-a-boundary, glued-alphanumeric-still-not. Separately, `test_allowlisted_acronyms_in_a_path_are_clean` used `src/UTF-8/` to prove the built-in allowlist reaches the path surface — but `UTF-8` is shape-proof, so it was passing via the regex, not via the allowlist it stood for. Switched to `SHA-256`. **The workflow YAML was touched, against the plan's "no change to the workflow YAML".** The change is one header **comment** naming `UTF-8` as a built-in allowlist entry, which this PR removes. No input, step, permission or default moved, and `bump-public-repo-hygiene-callers.yml` already triggers on the checker path this PR edits, so no new bump is provoked. Leaving a documented example that is provably no longer in the list seemed the worse outcome. ## Corpus sweep — including the half this does not fix Old regex vs new, over tracked **paths and contents**, netting out the exact and prefix allowlists: | repo | new-only `(token, file)` pairs | |---|---| | `Comfy-Org/github-workflows` (this repo, 1,363 findings at baseline) | **0** outside the two files this PR edits | | `Comfy-Org/ComfyUI` | **0** | | `Comfy-Org/ComfyUI_frontend` | **0** | So on three real public trees the loosened boundary introduces **zero** new findings — it adds no false-positive load — and the shapes it newly catches (`BE-1234_design` and friends) are absent from all three, which is why they were never noticed. They are pinned by unit test instead. The 4-finding delta in this repo's own self-scan (1,359 → 1,363) is entirely the new fixture strings this PR adds to its own test file and code comments. ## Verification of the negative claims Every claim of the form "this entry can never match" was checked empirically rather than reasoned about: ``` UTF-8 old=False new=False OAUTH-2 old=False new=False IPV-4 old=False new=False IPV-6 old=False new=False X-25519 old=False new=False ``` `test_every_allowlist_entry_can_actually_match` pins this as an invariant, so the dead-entry trap cannot reopen when someone adds the next acronym. ## Residual Not fixed here, and worth knowing before this workflow acquires callers: - **This repo's own self-scan is RED and stays red** — 1,359 findings before, 1,363 after, exit 1. That is pre-existing and expected: `github-workflows` is not enrolled in its own `public-repo-hygiene` workflow (there is no `ci-public-repo-hygiene.yml`), and its own conventions put ticket ids in commit messages, comments and test fixtures throughout. The plan listed "self-scan stays green" as an acceptance criterion; that criterion is **not met and was never true on `main`**. Deciding whether to enrol this repo (and with what `exclude_paths` / `ticket_allowlist`) is a separate call, not something this PR should make. - **No caller repo was exercised.** The plan asked that this land before any repo enrols, and no caller exists yet, so the change is verified against this repo's unit suite plus the three-repo sweep above — never against a live consumer run of the reusable workflow. - **The redundant `ISO-8601`, `RFC-2119`, `RFC-7231`, `RFC-3339` entries were left in place.** Each is now cleared twice — once exactly, once by its namespace prefix. They are harmless and the anti-vacuity test passes on them, but they are the same "second place to maintain" shape this PR argues against for `UTF-16`. Deliberately out of scope. - **The `--scan-text` PR-title/body surface is unaffected**, verified: `CAT_TICKET` is not in `SURFACE_CATEGORIES`, so category 1 does not run there at all and the boundary change cannot move that surface's verdict. - **Prior work referenced by the plan could not be read.** The plan cites an investigation and a review-thread comment held in a tracker this session has no access to; only the plan text reproduced in the task brief was available. The line numbers it quotes are relative to an older revision of the checker, which has since gained the prefix carve-out, the tracked-path surface and the PR-text surface — the two divergences documented under "Judgment calls" above follow from that. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/public-repo-hygiene/tests`: 202 run, 201 passed, 1 skipped, 0 failed (195 before this PR); `python3 -m unittest discover -s .github/agents-md-integrity/tests`: 46 passed, 0 failed; `check_agents_md.py --root .`: passed with 2 pre-existing warnings; `check_public_repo_hygiene.py --root .`: 1,359 findings before → 1,363 after, delta accounted for line by line (see corpus sweep); old-vs-new regex sweep over `ComfyUI` and `ComfyUI_frontend`: 0 new findings each; `yaml.safe_load` on the edited workflow: parses. - **Deviations:** the CVE carve-out was implemented as specified only in effect, not in form (the prefix mechanism already on `main` is used instead of the proposed `CVE_PREFIX_RE`); five of the seven proposed allowlist additions were omitted as already-covered-by-prefix and asserted behaviourally instead; the underscore cases went into sibling tests rather than into `test_shape_boundaries`, whose pinned expectation the plan required be left unchanged; one comment line in the reusable workflow YAML was corrected despite the plan's "no change to the workflow YAML" — all four are argued under "Judgment calls" above. The "self-scan stays green" acceptance criterion is not met and was already false on `main`; see "Residual". --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:58 +00:00