[PR #207] [MERGED] fix(public-repo-hygiene): case-insensitive matching and sentence-period stripping (BE-8697) #213

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-8654-public-repo-hygiene ← Head: matt/be-8697-hygiene-case-and-period

📝 Commits (2)

27ccd2e fix(public-repo-hygiene): case-insensitive matching and sentence-period stripping (BE-8697)
c1abd5f fix(public-repo-hygiene): close the four case/period edges the panel found (BE-8697)

📊 Changes

5 files changed (+275 additions, -41 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+6 -13)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+73 -11)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+169 -9)
📝 .github/workflows/test-public-repo-hygiene.yml (+15 -3)
📝 docs/callers/public-repo-hygiene.md (+12 -5)

📄 Description

STACKED — merging lands on matt/be-8654-public-repo-hygiene (owned by Matt Miller, PR #206), NOT main. Do not merge this before #206. #206's acceptance criterion is an 18/18 findings-parity proof against the two per-repo SDK scripts, and merging this into its branch first would invalidate that proof. Once #206 merges, GitHub retargets this PR to main — see "Merge order" below for the one recovery case.

ELI-5

The leak guard has a rule: "any Comfy-Org/<repo> name that isn't on the known-public list is a finding." Two bugs made that rule both too loose and too noisy. Too loose: GitHub treats owner names case-insensitively, so writing comfy-org/some-private-thing in lowercase reached the same repository but sailed straight past the check — a one-keystroke bypass of the whole category. Too noisy: the repo-name pattern allows dots, so ordinary English like "…built on Comfy-Org/ComfyUI." was read as a repo literally named ComfyUI. and flagged. This fixes both, plus the matching allowlist half so a differently-cased spelling of a public name isn't reported as a leak.

What changed

REPO_REF_RE is now re.IGNORECASE. Whole-pattern is equivalent to org-segment-only here: the capture class [A-Za-z0-9_.-] is already case-complete, so group(1) comes back exactly as written either way.
A sentence-final . is stripped before the team/repo fork. It has to be before the fork, not in the repo branch: the confirmed false positive (@Comfy-Org/Comfy-Cloud-Team.) lives in the team branch, which the existing .git strip never reaches. The .git strip stays where it is and now runs on the already-period-stripped name, so …/Foo.git. resolves too. A name that is nothing but the period (Comfy-Org/.) references no repo and is skipped rather than flagged.
Allowlist membership is compared casefolded, via two derived module-level views. PUBLIC_COMFY_ORG_REPOS / PUBLIC_COMFY_ORG_TEAMS stay the human-edited source lists in canonical GitHub spelling, and findings still quote the name as matched.
Tests, the checker README, the caller guide, and the end-to-end smoke fixtures follow.

Why each fix cannot weaken the control

Case-insensitive matching widens the match — strictly more detection, never less.
The allowlist contains public names only, by design, under any casing — so casefolded membership can only clear names GitHub already resolves to an allowlisted public repo or team. Verified below.
A GitHub slug can never end in ., so a stripped trailing period is always sentence punctuation. Verified below.

Corroborating precedent inside this repo: bump-callers.sh already casefolds the org segment for exactly reason 1 (.github/bump-callers/bump-callers.sh:265, .github/bump-callers/README.md:663) and already rejects an all-dots name for reason 3 (bump-callers.sh:224). This brings the hygiene checker in line with a sibling component that settled the same question first.

Falsification of the premises (not just unit tests)

Unit tests I author only assert my own premise, so each premise was checked against GitHub directly, read-only:

Owner names really are case-insensitive. GET /repos/comfy-org/ComfyUI and GET /repos/COMFY-ORG/comfyui both return full_name: Comfy-Org/ComfyUI — i.e. the lowercase spelling was reaching the real repository while the checker ignored it.
A dot-terminated slug names nothing. GET /repos/Comfy-Org/ComfyUI. and GET /repos/Comfy-Org/. both 404, so stripping a trailing dot cannot suppress a reference to a real repository — including the Comfy-Org/. skip, which is the only place this diff removes a finding rather than adding one.
The team casing is the same team. GET /orgs/Comfy-Org/teams/Comfy-Cloud-Team and .../comfy-cloud-team both return slug comfy-cloud-team — so casefolded team membership clears the CODEOWNERS display spelling of an already-allowlisted public team, not a different team.

Corpus sweep — both halves

Old and new checkers run over this repo's own 500+ tracked files (this repo is deliberately not self-enrolled, so it is a stress corpus, not a clean one). Repo-reference findings: 179 → 173.

16 locations newly flagged — every one a genuine lowercase-org reference that previously bypassed the category entirely (in bump-callers/, agents-md-integrity/, and the hygiene fixtures themselves).
20 locations cleared — all sentence-final-period false positives, e.g. .github/dependabot.yml:1 ("…for Comfy-Org/github-workflows.") and .github/workflows/pr-risk.yml:396.
The half this PR does NOT fix, measured with the same sweep: the scan stays line-oriented. On this tree that leaves 1 dangling comfy-org/ at end-of-line and 15 lines ending on a wrapped Comfy-/Comfy-Org token, none of which any line-oriented matcher can see. That limitation is unchanged, and it is now the only bullet left in the checker README's "Known limitations".

Detection parity with the SDK scripts ends here — deliberately

This intentionally ends findings parity with the two per-repo scripts the centralization replaced (comfy-python-sdk scripts/check_public_repo_hygiene.py, comfy-typescript-sdk scripts/check-public-repo-hygiene.mjs). Parity existed only to prove the centralization changed nothing; once #206 lands, that proof has served its purpose. Those SDK copies are not patched here — they are deleted at adoption. There are currently zero callers of public-repo-hygiene.yml (gh search code --owner Comfy-Org 'public-repo-hygiene.yml' returns no hits), which is exactly why these fixes should land before anyone enrols.

Merge order

delete_branch_on_merge is on for this repo. If #206's branch is deleted on merge, GitHub closes this PR instead of retargeting it, and a closed PR with a deleted base can be neither reopened nor retargeted. The branch itself survives, so the recovery is to open a fresh PR: gh pr create --base main --head matt/be-8697-hygiene-case-and-period (if #206 squash-merges, git merge origin/main into this branch first so the three-dot diff shows only the net change).

Judgment calls

This PR is stacked on an unmerged #206, which the ticket asked to wait for. The ticket's stated hazard is pushing these commits into #206 and invalidating its parity proof; a stacked PR on a separate branch does not do that, and standing fleet policy is to stack on a blocker that has buildable branch code rather than idle. The banner above and the merge-order note are what keep the hazard visible. If you would rather this simply wait, closing it and re-opening after #206 costs nothing.
Finding messages still print the canonical Comfy-Org/ prefix even when the source said comfy-org/. Only the name is echoed as matched (pre-existing behaviour — the org segment was always a literal in the message). A reader grepping for the exact lowercase string will not find it, but every finding carries file:line, which is the primary locator. Changing it would churn every message format for marginal gain.
No AI attribution trailer on the commit, unlike #206's commits — that is this loop's standing convention, not a comment on the base branch's.

Unexercised artifacts

The ticket's provenance references a Linear spike issue whose body I hold no access to; the ticket text is the whole of what I was given. Nothing in it was needed beyond what is verified above.
The two SDK scripts are in other repositories and are deliberately untouched, so no parity re-run was performed against them — ending that parity is the point of this change.

Verification

python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py' — 63 passed, 0 failed, 1 skipped (the skip is pre-existing: APFS rejects the non-UTF-8 filename that test builds; it is Linux-only by design). Foundation branch was 54.
All 9 smoke steps from test-public-repo-hygiene.yml executed locally, including the tamper-resistance and reserved-path steps — all pass.
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, none with a default, every ref checkout guarded; its 216 unit tests pass.
test_team_allowlist_does_not_leak_into_repo_allowlist still passes: the team/repo namespace separation survives casefolding (Comfy-Org/comfy-cloud-team without an @ is still a repo finding).

Refs BE-8697 — deliberately not Closes: this PR is stacked on an unmerged #206 and cannot reach main until that lands, and the ticket's own precondition ("#206 must be MERGED first") is therefore not yet satisfied. The ticket should stay open until this is on the default branch.

Provenance

Authored by: agent-work loop
Verified: public-repo-hygiene unittest suite: 63 passed, 0 failed, 1 skipped (pre-existing macOS filesystem skip); all 9 test-public-repo-hygiene.yml smoke steps run locally, all pass; workflow-pins lint OK (11 workflows) + its 216 unit tests pass; old-vs-new corpus sweep over this repo's tracked files: 179 → 173 repo-reference findings, 16 newly flagged, 20 cleared
Deviations: the ticket's precondition was to wait for #206 to merge; this is stacked on #206's branch instead, per standing stack-don't-wait policy, with the merge-order hazard called out above. No other acceptance criterion was skipped.

_{🔄 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/207 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/22/2026 **Status:** ✅ Merged **Merged:** 8/22/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-8654-public-repo-hygiene` ← **Head:** `matt/be-8697-hygiene-case-and-period` --- ### 📝 Commits (2) - [`27ccd2e`](https://github.com/Comfy-Org/github-workflows/commit/27ccd2e886bb55413996f622ab8a97956467fa12) fix(public-repo-hygiene): case-insensitive matching and sentence-period stripping (BE-8697) - [`c1abd5f`](https://github.com/Comfy-Org/github-workflows/commit/c1abd5f0c257beae8881ff1e7b7780284dc18363) fix(public-repo-hygiene): close the four case/period edges the panel found (BE-8697) ### 📊 Changes **5 files changed** (+275 additions, -41 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+6 -13) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+73 -11) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+169 -9) 📝 `.github/workflows/test-public-repo-hygiene.yml` (+15 -3) 📝 `docs/callers/public-repo-hygiene.md` (+12 -5) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-8654-public-repo-hygiene` (owned by Matt Miller, PR #206), NOT `main`.** Do **not** merge this before #206. #206's acceptance criterion is an 18/18 findings-parity proof against the two per-repo SDK scripts, and merging this into its branch first would invalidate that proof. Once #206 merges, GitHub retargets this PR to `main` — see "Merge order" below for the one recovery case. ## ELI-5 The leak guard has a rule: "any `Comfy-Org/<repo>` name that isn't on the known-public list is a finding." Two bugs made that rule both too loose and too noisy. Too loose: GitHub treats owner names case-insensitively, so writing `comfy-org/some-private-thing` in lowercase reached the same repository but sailed straight past the check — a one-keystroke bypass of the whole category. Too noisy: the repo-name pattern allows dots, so ordinary English like "…built on Comfy-Org/ComfyUI." was read as a repo literally named `ComfyUI.` and flagged. This fixes both, plus the matching allowlist half so a differently-cased spelling of a *public* name isn't reported as a leak. ## What changed - **`REPO_REF_RE` is now `re.IGNORECASE`.** Whole-pattern is equivalent to org-segment-only here: the capture class `[A-Za-z0-9_.-]` is already case-complete, so `group(1)` comes back exactly as written either way. - **A sentence-final `.` is stripped before the team/repo fork.** It has to be before the fork, not in the repo branch: the confirmed false positive (`@Comfy-Org/Comfy-Cloud-Team.`) lives in the *team* branch, which the existing `.git` strip never reaches. The `.git` strip stays where it is and now runs on the already-period-stripped name, so `…/Foo.git.` resolves too. A name that is nothing but the period (`Comfy-Org/.`) references no repo and is skipped rather than flagged. - **Allowlist membership is compared casefolded**, via two derived module-level views. `PUBLIC_COMFY_ORG_REPOS` / `PUBLIC_COMFY_ORG_TEAMS` stay the human-edited source lists in canonical GitHub spelling, and findings still quote the name as matched. - Tests, the checker README, the caller guide, and the end-to-end smoke fixtures follow. ## Why each fix cannot weaken the control 1. Case-insensitive matching **widens** the match — strictly more detection, never less. 2. The allowlist contains public names only, by design, under any casing — so casefolded membership can only clear names GitHub already resolves to an allowlisted public repo or team. Verified below. 3. A GitHub slug can never end in `.`, so a stripped trailing period is always sentence punctuation. Verified below. Corroborating precedent inside this repo: `bump-callers.sh` already casefolds the org segment for exactly reason 1 (`.github/bump-callers/bump-callers.sh:265`, `.github/bump-callers/README.md:663`) and already rejects an all-dots name for reason 3 (`bump-callers.sh:224`). This brings the hygiene checker in line with a sibling component that settled the same question first. ## Falsification of the premises (not just unit tests) Unit tests I author only assert my own premise, so each premise was checked against GitHub directly, read-only: - **Owner names really are case-insensitive.** `GET /repos/comfy-org/ComfyUI` and `GET /repos/COMFY-ORG/comfyui` both return `full_name: Comfy-Org/ComfyUI` — i.e. the lowercase spelling was reaching the real repository while the checker ignored it. - **A dot-terminated slug names nothing.** `GET /repos/Comfy-Org/ComfyUI.` and `GET /repos/Comfy-Org/.` both 404, so stripping a trailing dot cannot suppress a reference to a real repository — including the `Comfy-Org/.` skip, which is the only place this diff *removes* a finding rather than adding one. - **The team casing is the same team.** `GET /orgs/Comfy-Org/teams/Comfy-Cloud-Team` and `.../comfy-cloud-team` both return slug `comfy-cloud-team` — so casefolded team membership clears the CODEOWNERS display spelling of an already-allowlisted public team, not a different team. ## Corpus sweep — both halves Old and new checkers run over this repo's own 500+ tracked files (this repo is deliberately not self-enrolled, so it is a stress corpus, not a clean one). Repo-reference findings: **179 → 173**. - **16 locations newly flagged** — every one a genuine lowercase-org reference that previously bypassed the category entirely (in `bump-callers/`, `agents-md-integrity/`, and the hygiene fixtures themselves). - **20 locations cleared** — all sentence-final-period false positives, e.g. `.github/dependabot.yml:1` ("…for Comfy-Org/github-workflows.") and `.github/workflows/pr-risk.yml:396`. - **The half this PR does NOT fix**, measured with the same sweep: the scan stays line-oriented. On this tree that leaves **1** dangling `comfy-org/` at end-of-line and **15** lines ending on a wrapped `Comfy-`/`Comfy-Org` token, none of which any line-oriented matcher can see. That limitation is unchanged, and it is now the only bullet left in the checker README's "Known limitations". ## Detection parity with the SDK scripts ends here — deliberately This intentionally ends findings parity with the two per-repo scripts the centralization replaced (`comfy-python-sdk` `scripts/check_public_repo_hygiene.py`, `comfy-typescript-sdk` `scripts/check-public-repo-hygiene.mjs`). Parity existed only to prove the centralization changed nothing; once #206 lands, that proof has served its purpose. Those SDK copies are **not** patched here — they are deleted at adoption. There are currently **zero** callers of `public-repo-hygiene.yml` (`gh search code --owner Comfy-Org 'public-repo-hygiene.yml'` returns no hits), which is exactly why these fixes should land before anyone enrols. ## Merge order `delete_branch_on_merge` is on for this repo. If #206's branch is deleted on merge, GitHub **closes** this PR instead of retargeting it, and a closed PR with a deleted base can be neither reopened nor retargeted. The branch itself survives, so the recovery is to open a fresh PR: `gh pr create --base main --head matt/be-8697-hygiene-case-and-period` (if #206 squash-merges, `git merge origin/main` into this branch first so the three-dot diff shows only the net change). ## Judgment calls - **This PR is stacked on an unmerged #206, which the ticket asked to wait for.** The ticket's stated hazard is pushing these commits *into* #206 and invalidating its parity proof; a stacked PR on a separate branch does not do that, and standing fleet policy is to stack on a blocker that has buildable branch code rather than idle. The banner above and the merge-order note are what keep the hazard visible. If you would rather this simply wait, closing it and re-opening after #206 costs nothing. - **Finding messages still print the canonical `Comfy-Org/` prefix** even when the source said `comfy-org/`. Only the *name* is echoed as matched (pre-existing behaviour — the org segment was always a literal in the message). A reader grepping for the exact lowercase string will not find it, but every finding carries `file:line`, which is the primary locator. Changing it would churn every message format for marginal gain. - **No AI attribution trailer on the commit**, unlike #206's commits — that is this loop's standing convention, not a comment on the base branch's. ## Unexercised artifacts - The ticket's provenance references a Linear spike issue whose body I hold no access to; the ticket text is the whole of what I was given. Nothing in it was needed beyond what is verified above. - The two SDK scripts are in other repositories and are deliberately untouched, so no parity re-run was performed against them — ending that parity is the point of this change. ## Verification - `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'` — **63 passed, 0 failed, 1 skipped** (the skip is pre-existing: APFS rejects the non-UTF-8 filename that test builds; it is Linux-only by design). Foundation branch was 54. - All **9** smoke steps from `test-public-repo-hygiene.yml` executed locally, including the tamper-resistance and reserved-path steps — all pass. - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, none with a default, every ref checkout guarded; its 216 unit tests pass. - `test_team_allowlist_does_not_leak_into_repo_allowlist` still passes: the team/repo namespace separation survives casefolding (`Comfy-Org/comfy-cloud-team` without an `@` is still a repo finding). Refs BE-8697 — deliberately not `Closes`: this PR is stacked on an unmerged #206 and cannot reach `main` until that lands, and the ticket's own precondition ("#206 must be MERGED first") is therefore not yet satisfied. The ticket should stay open until this is on the default branch. ## Provenance - **Authored by:** agent-work loop - **Verified:** public-repo-hygiene unittest suite: 63 passed, 0 failed, 1 skipped (pre-existing macOS filesystem skip); all 9 test-public-repo-hygiene.yml smoke steps run locally, all pass; workflow-pins lint OK (11 workflows) + its 216 unit tests pass; old-vs-new corpus sweep over this repo's tracked files: 179 → 173 repo-reference findings, 16 newly flagged, 20 cleared - **Deviations:** the ticket's precondition was to wait for #206 to merge; this is stacked on #206's branch instead, per standing stack-don't-wait policy, with the merge-order hazard called out above. No other acceptance criterion was skipped. --- <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:26 +00:00