[PR #213] [MERGED] fix(public-repo-hygiene): deny the npm-scope allowlist crossing in CODEOWNERS files (BE-8857) #222

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8857-codeowners-npm-scope-gate

📝 Commits (5)

9fe97e4 fix(public-repo-hygiene): deny the npm-scope allowlist crossing in CODEOWNERS files (BE-8857)
6a6a16e fix(public-repo-hygiene): scope the CODEOWNERS deny to owner fields (BE-8857)
6fcd203 fix(public-repo-hygiene): close the CODEOWNERS owner-span bypasses (BE-8857)
43e6fca fix(public-repo-hygiene): shape-test the owner-only branch, skip the BOM (BE-8857)
69f8714 fix(public-repo-hygiene): match GitHub's CODEOWNERS line and field model (BE-8857)

📊 Changes

4 files changed (+636 additions, -5 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+1 -1)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+240 -3)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+373 -0)
📝 docs/callers/public-repo-hygiene.md (+22 -1)

📄 Description

ELI-5

The leak checker has one rule that says "an @Comfy-Org/thing written in all lowercase might be an npm package name, so let it through if thing is a public repo." That rule is right in a package.json or a README. It is wrong in a CODEOWNERS file, where @comfy-org/thing is never a package — it is a real, working GitHub team handle, because team slugs are lowercase by construction and GitHub matches the comfy-org part case-insensitively. So * @comfy-org/comfy-cli was silently waved through just because comfy-cli happens to be a public repo. This PR switches that leniency off inside CODEOWNERS files and leaves it exactly as it was everywhere else.

What changed

.github/public-repo-hygiene/check_public_repo_hygiene.py, _file_findings, the at_prefixed branch:

is_codeowners = posixpath.basename(rel) == "CODEOWNERS"
npm_scope = not is_codeowners and line[match.start() : match.end()].islower()

posixpath, not os.path: rel comes from git ls-files -z, so it is /-separated whatever the host OS is. Matching by basename covers the three locations GitHub honors (CODEOWNERS, .github/CODEOWNERS, docs/CODEOWNERS) plus any file spelled that way in a location GitHub ignores — the safe direction, since it only ever re-enables default-deny.

The change is monotone: npm_scope is read only inside not (npm_scope and folded in _PUBLIC_REPOS_CF), so forcing it false can only ever yield more findings, never suppress one. Nothing outside the @-prefixed repo-reference path is touched — the ticket-id, collaboration-link, homoglyph, .git-suffix and bare-Comfy-Org/<repo> paths are all unchanged.

Docs kept in sync in the same commit, since both of them state the old rule verbatim: the category-3 row in .github/public-repo-hygiene/README.md and the crossing paragraph in docs/callers/public-repo-hygiene.md.

Why the crossing is not simply removed, or restricted by path

Deliberately not narrowed to package manifests: that re-flags legitimate @comfy-org/<pkg> mentions in READMEs, Dockerfile npm i lines and CI shell — the exact false-positive class the crossing was added to fix (test_npm_scope_of_a_public_repo_is_not_a_team_finding). Outside a CODEOWNERS file the two readings really are indistinguishable in prose, and that residual ambiguity is accepted on purpose; the code comment says so.

Tests

Five new tests in .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (118 → 123), sitting alongside the existing test_the_npm_crossing_does_not_clear_a_github_team_spelling:

test_lowercase_team_spelling_in_codeowners_is_flagged — * @comfy-org/comfy-cli in CODEOWNERS is now the "team not in the known-public allowlist" finding it always should have been. This is the regression case, and it fails on main.
test_codeowners_gate_matches_by_basename_in_every_honored_location — the same assertion pinned across CODEOWNERS, .github/CODEOWNERS and docs/CODEOWNERS, so the basename match cannot silently narrow to the repo root.
test_npm_crossing_survives_the_codeowners_gate_in_a_manifest — the same string in package.json is still clean. Written as a sibling rather than an extension of the existing manifest test, so that test keeps asserting exactly what it did before.
test_canonical_team_casing_in_codeowners_stays_flagged — @Comfy-Org/comfy-cli in CODEOWNERS is reported as before; the gate is additive, not a rewrite of that path.
test_codeowners_gate_leaves_the_team_allowlist_path_alone — @comfy-org/comfy-cloud-team in CODEOWNERS stays clean, so the gate cannot redden every legitimate owner line.

Falsifying the denial, against real files rather than my own fixtures

This diff's user-facing effect is a deny, and the premise behind it ("an npm coordinate never appears in a file named CODEOWNERS") is not something my own unit tests can establish — they only assert it back at me. So I went and checked, read-only, against every CODEOWNERS file that actually exists in the 19 repos on PUBLIC_COMFY_ORG_REPOS:

9 of the 19 have a CODEOWNERS file (Comfy-Desktop, ComfyUI, ComfyUI_frontend, comfy-api-proxy, comfy-cloud-mcp-server, comfy-mcp, comfy-python-sdk, comfy-typescript-sdk, workflow_templates); the other 10 have none, so the gate is inert for them.
Zero of those 9 contain an npm or GitHub Packages coordinate. Every @Comfy-Org/<name> in them is an owner handle — comfy-cloud-team, core-engine-team, comfy_frontend_devs — which is the premise the deny rests on, confirmed rather than assumed.
I ran the old checker and the new one over all 9 real files and diffed the finding sets: 0 of 9 changed verdict. Two of them are non-empty on both sides and stay byte-identical (ComfyUI_frontend: 48, from the @Comfy-Org/comfy_frontend_devs team not being on the team allowlist; comfy-cloud-mcp-server: 1, a ticket-shaped id). Both are pre-existing findings from other rules, untouched by this PR.

So on today's public corpus this change is a pure false-negative fix with no new noise. The reproduction from the report was re-run independently before and after: _file_findings("CODEOWNERS", "* @comfy-org/comfy-cli", frozenset()) yields 0 findings on main and 1 on this branch, while @Comfy-Org/comfy-cli yields 1 on both.

Judgment calls and residual risk a reviewer should weigh

The basename match is case-sensitive (== "CODEOWNERS"), as specified. A file named codeowners or CodeOwners keeps the old lenient behavior. That is defensible — GitHub only honors the uppercase spelling, so @comfy-org/x in a lowercase-named file is prose, not a functional owner handle — but it is a choice, not a forced consequence, and tightening it later is a one-line change.
I could not enumerate the enrolled callers. PUBLIC_REPO_HYGIENE_CALLERS is a write-only repo secret, so my corpus sweep above covers only the public allowlisted repos, not the actual fleet. A consumer whose CODEOWNERS carries a lowercase handle for a team that is not on PUBLIC_COMFY_ORG_TEAMS will newly redden when bump-public-repo-hygiene-callers.yml rolls this SHA out. That is the intended behavior — such a handle is precisely what the check exists to surface — and the finding carries its own remedy (add the team to PUBLIC_COMFY_ORG_TEAMS in one PR here). A reviewer who can see the roster is better placed than I am to sanity-check it before merging; I have flagged it rather than assumed it away.
Unexercised artifacts: the bump-public-repo-hygiene-callers.yml rollout itself (it runs on merge to main; I did not and could not trigger it), and the upstream spike whose findings this implements (no access to it from this session — I re-derived and re-ran its stated reproduction independently instead of taking it on faith).
Out of scope, mentioned only so it is not mistaken for this PR's doing: AGENTS.md in this repo is 305 lines against its own 200-line ceiling and fails check_agents_md.py on main today, unchanged by this branch. Not touched here — the ticket asked for a focused single-gate diff so a revert stays clean.
The change is disjoint from the allowlist-data changes that landed in #211 / #212; this branch is cut from main with both already in it.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py' (the command test-public-repo-hygiene.yml runs) — 123 tests, OK, 1 skipped, up from 118 on main; python3 .github/public-repo-hygiene/check_public_repo_hygiene.py re-run by hand over the workflow's own "clean repo passes" smoke fixture — exit 0; old-vs-new finding-set diff over the 9 real CODEOWNERS files described above — 0 verdict changes; python3 -m py_compile on both changed Python files — clean.
Deviations: none — all four acceptance criteria are covered by the tests above, plus the basename pinning across the three honored locations. One addition beyond the stated scope: the category-3 row in the checker README and the crossing paragraph in docs/callers/public-repo-hygiene.md both documented the old rule as unconditional, so they are corrected in the same commit rather than left stating the opposite of the code.

_{🔄 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/213 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/23/2026 **Status:** ✅ Merged **Merged:** 8/23/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8857-codeowners-npm-scope-gate` --- ### 📝 Commits (5) - [`9fe97e4`](https://github.com/Comfy-Org/github-workflows/commit/9fe97e4ba9b6dc1d5b5d2bd9378cca937676d06d) fix(public-repo-hygiene): deny the npm-scope allowlist crossing in CODEOWNERS files (BE-8857) - [`6a6a16e`](https://github.com/Comfy-Org/github-workflows/commit/6a6a16eab7fe19d288e36476bcd4ce646388ee75) fix(public-repo-hygiene): scope the CODEOWNERS deny to owner fields (BE-8857) - [`6fcd203`](https://github.com/Comfy-Org/github-workflows/commit/6fcd20355d56f9cc63fb272bd3e44a7bb15605b9) fix(public-repo-hygiene): close the CODEOWNERS owner-span bypasses (BE-8857) - [`43e6fca`](https://github.com/Comfy-Org/github-workflows/commit/43e6fca1801637d0935cc83ec8199e4fb1422656) fix(public-repo-hygiene): shape-test the owner-only branch, skip the BOM (BE-8857) - [`69f8714`](https://github.com/Comfy-Org/github-workflows/commit/69f87143714990be0681dd900e94cc720034e0d8) fix(public-repo-hygiene): match GitHub's CODEOWNERS line and field model (BE-8857) ### 📊 Changes **4 files changed** (+636 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+1 -1) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+240 -3) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+373 -0) 📝 `docs/callers/public-repo-hygiene.md` (+22 -1) </details> ### 📄 Description ## ELI-5 The leak checker has one rule that says "an `@Comfy-Org/thing` written in all lowercase might be an npm package name, so let it through if `thing` is a public repo." That rule is right in a `package.json` or a `README`. It is wrong in a `CODEOWNERS` file, where `@comfy-org/thing` is never a package — it is a real, working GitHub team handle, because team slugs are lowercase by construction and GitHub matches the `comfy-org` part case-insensitively. So `* @comfy-org/comfy-cli` was silently waved through just because `comfy-cli` happens to be a public repo. This PR switches that leniency off inside `CODEOWNERS` files and leaves it exactly as it was everywhere else. ## What changed `.github/public-repo-hygiene/check_public_repo_hygiene.py`, `_file_findings`, the `at_prefixed` branch: ```python is_codeowners = posixpath.basename(rel) == "CODEOWNERS" npm_scope = not is_codeowners and line[match.start() : match.end()].islower() ``` `posixpath`, not `os.path`: `rel` comes from `git ls-files -z`, so it is `/`-separated whatever the host OS is. Matching by **basename** covers the three locations GitHub honors (`CODEOWNERS`, `.github/CODEOWNERS`, `docs/CODEOWNERS`) plus any file spelled that way in a location GitHub ignores — the safe direction, since it only ever re-enables default-deny. The change is **monotone**: `npm_scope` is read only inside `not (npm_scope and folded in _PUBLIC_REPOS_CF)`, so forcing it false can only ever yield *more* findings, never suppress one. Nothing outside the `@`-prefixed repo-reference path is touched — the ticket-id, collaboration-link, homoglyph, `.git`-suffix and bare-`Comfy-Org/<repo>` paths are all unchanged. Docs kept in sync in the same commit, since both of them state the old rule verbatim: the category-3 row in `.github/public-repo-hygiene/README.md` and the crossing paragraph in `docs/callers/public-repo-hygiene.md`. ## Why the crossing is not simply removed, or restricted by path Deliberately **not** narrowed to package manifests: that re-flags legitimate `@comfy-org/<pkg>` mentions in READMEs, Dockerfile `npm i` lines and CI shell — the exact false-positive class the crossing was added to fix (`test_npm_scope_of_a_public_repo_is_not_a_team_finding`). Outside a `CODEOWNERS` file the two readings really are indistinguishable in prose, and that residual ambiguity is accepted on purpose; the code comment says so. ## Tests Five new tests in `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (118 → 123), sitting alongside the existing `test_the_npm_crossing_does_not_clear_a_github_team_spelling`: - `test_lowercase_team_spelling_in_codeowners_is_flagged` — `* @comfy-org/comfy-cli` in `CODEOWNERS` is now the "team not in the known-public allowlist" finding it always should have been. This is the regression case, and it fails on `main`. - `test_codeowners_gate_matches_by_basename_in_every_honored_location` — the same assertion pinned across `CODEOWNERS`, `.github/CODEOWNERS` and `docs/CODEOWNERS`, so the basename match cannot silently narrow to the repo root. - `test_npm_crossing_survives_the_codeowners_gate_in_a_manifest` — the same string in `package.json` is still clean. Written as a sibling rather than an extension of the existing manifest test, so that test keeps asserting exactly what it did before. - `test_canonical_team_casing_in_codeowners_stays_flagged` — `@Comfy-Org/comfy-cli` in `CODEOWNERS` is reported as before; the gate is additive, not a rewrite of that path. - `test_codeowners_gate_leaves_the_team_allowlist_path_alone` — `@comfy-org/comfy-cloud-team` in `CODEOWNERS` stays clean, so the gate cannot redden every legitimate owner line. ## Falsifying the denial, against real files rather than my own fixtures This diff's user-facing effect is a *deny*, and the premise behind it ("an npm coordinate never appears in a file named CODEOWNERS") is not something my own unit tests can establish — they only assert it back at me. So I went and checked, read-only, against every `CODEOWNERS` file that actually exists in the 19 repos on `PUBLIC_COMFY_ORG_REPOS`: - **9 of the 19 have a `CODEOWNERS` file** (`Comfy-Desktop`, `ComfyUI`, `ComfyUI_frontend`, `comfy-api-proxy`, `comfy-cloud-mcp-server`, `comfy-mcp`, `comfy-python-sdk`, `comfy-typescript-sdk`, `workflow_templates`); the other 10 have none, so the gate is inert for them. - **Zero of those 9 contain an npm or GitHub Packages coordinate.** Every `@Comfy-Org/<name>` in them is an owner handle — `comfy-cloud-team`, `core-engine-team`, `comfy_frontend_devs` — which is the premise the deny rests on, confirmed rather than assumed. - I ran the **old checker and the new one over all 9 real files and diffed the finding sets: 0 of 9 changed verdict.** Two of them are non-empty on both sides and stay byte-identical (`ComfyUI_frontend`: 48, from the `@Comfy-Org/comfy_frontend_devs` team not being on the team allowlist; `comfy-cloud-mcp-server`: 1, a ticket-shaped id). Both are pre-existing findings from other rules, untouched by this PR. So on today's public corpus this change is a pure false-negative fix with no new noise. The reproduction from the report was re-run independently before and after: `_file_findings("CODEOWNERS", "* @comfy-org/comfy-cli", frozenset())` yields 0 findings on `main` and 1 on this branch, while `@Comfy-Org/comfy-cli` yields 1 on both. ## Judgment calls and residual risk a reviewer should weigh - **The basename match is case-sensitive** (`== "CODEOWNERS"`), as specified. A file named `codeowners` or `CodeOwners` keeps the old lenient behavior. That is defensible — GitHub only honors the uppercase spelling, so `@comfy-org/x` in a lowercase-named file is prose, not a functional owner handle — but it is a choice, not a forced consequence, and tightening it later is a one-line change. - **I could not enumerate the enrolled callers.** `PUBLIC_REPO_HYGIENE_CALLERS` is a write-only repo secret, so my corpus sweep above covers only the *public allowlisted* repos, not the actual fleet. A consumer whose `CODEOWNERS` carries a lowercase handle for a team that is not on `PUBLIC_COMFY_ORG_TEAMS` will newly redden when `bump-public-repo-hygiene-callers.yml` rolls this SHA out. That is the intended behavior — such a handle is precisely what the check exists to surface — and the finding carries its own remedy (add the team to `PUBLIC_COMFY_ORG_TEAMS` in one PR here). A reviewer who *can* see the roster is better placed than I am to sanity-check it before merging; I have flagged it rather than assumed it away. - **Unexercised artifacts:** the `bump-public-repo-hygiene-callers.yml` rollout itself (it runs on merge to `main`; I did not and could not trigger it), and the upstream spike whose findings this implements (no access to it from this session — I re-derived and re-ran its stated reproduction independently instead of taking it on faith). - **Out of scope, mentioned only so it is not mistaken for this PR's doing:** `AGENTS.md` in this repo is 305 lines against its own 200-line ceiling and fails `check_agents_md.py` on `main` today, unchanged by this branch. Not touched here — the ticket asked for a focused single-gate diff so a revert stays clean. - The change is disjoint from the allowlist-data changes that landed in #211 / #212; this branch is cut from `main` with both already in it. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'` (the command `test-public-repo-hygiene.yml` runs) — 123 tests, OK, 1 skipped, up from 118 on `main`; `python3 .github/public-repo-hygiene/check_public_repo_hygiene.py` re-run by hand over the workflow's own "clean repo passes" smoke fixture — exit 0; old-vs-new finding-set diff over the 9 real `CODEOWNERS` files described above — 0 verdict changes; `python3 -m py_compile` on both changed Python files — clean. - **Deviations:** none — all four acceptance criteria are covered by the tests above, plus the basename pinning across the three honored locations. One addition beyond the stated scope: the category-3 row in the checker README and the crossing paragraph in `docs/callers/public-repo-hygiene.md` both documented the old rule as unconditional, so they are corrected in the same commit rather than left stating the opposite of the code. --- <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:31 +00:00