[PR #212] [MERGED] fix(public-repo-hygiene): allowlist six verified-public Comfy-Org repos (BE-8855) #220

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8855-public-repo-hygiene-allowlist

📝 Commits (2)

6c4251e fix(public-repo-hygiene): allowlist six verified-public Comfy-Org repos (BE-8855)
a5f6305 fix(public-repo-hygiene): casefold the allowlist duplicate check, document the sort (BE-8855)

📊 Changes

3 files changed (+161 additions, -12 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+15 -2)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+38 -10)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+108 -0)

📄 Description

ELI-5

The hygiene checker only knows a handful of Comfy-Org repos are public, and it treats everything else as a possible leak. Six repos that really are public were missing from that list, so every mention of them was being reported as a leak. This adds the six, sorts the list alphabetically so the next person can actually scan it, and adds a test that keeps it sorted and duplicate-free.

What changed

Adds six repos to PUBLIC_COMFY_ORG_REPOS in .github/public-repo-hygiene/check_public_repo_hygiene.py: workflow_templates, ComfyUI-Manager, comfy-skills, CustomNodeComfyMath, cookiecutter-comfy-extension, ComfyUI-test-framework. Each carries a one-line comment recording where the reference came from, per the README runbook.
Re-sorts the whole literal into case-insensitive alphabetical order. Three earlier additions had accreted at the bottom in arrival order, and the two ComfyUI* entries were already out of order relative to each other.
Adds AllowlistSourceOrderTest, which parses the module's own AST and asserts both allowlist literals (repos and teams) are sorted and duplicate-free, and that the literal it read is the one the module actually exports.
Documents the ordering rule in the "Adding a repo to the allowlist" runbook in .github/public-repo-hygiene/README.md.

Purely additive to the allowlist. It loosens nothing that default-deny was protecting: the list contains public names only by design, which is the property that makes it safe to host in a public repo in the first place.

Why the AST, not a runtime assertion

A frozenset has no order at run time, so the source text is the only place the ordering exists — and it is exactly the view a human reads to answer "is <name> already in here?" before appending a duplicate to the bottom. An exact textual duplicate is also invisible to every other assertion in the suite, because the set literal collapses {"a", "a"} to {"a"} before any test can see it. The AST is the one view that still has both copies.

Both halves were falsified rather than assumed: inserting an out-of-order entry failed test_repo_allowlist_is_sorted_and_duplicate_free on the ordering assertion, and inserting an in-order exact duplicate failed it on the duplicate assertion. The tree was restored between each.

Verification

Each of the six re-verified PUBLIC at implementation time (2026-08-23) rather than trusting the list I was handed, by two independent paths:

repo	`gh repo view --json visibility`	unauthenticated `api.github.com/repos/Comfy-Org/<name>`
`workflow_templates`	PUBLIC	200
`ComfyUI-Manager`	PUBLIC	200
`comfy-skills`	PUBLIC	200
`CustomNodeComfyMath`	PUBLIC	200
`cookiecutter-comfy-extension`	PUBLIC	200
`ComfyUI-test-framework`	PUBLIC	200

The unauthenticated check is the one that matters for a default-deny control: gh repo view runs as an org member, so it will happily print metadata for a private repo. An anonymous 404 would mean the repo is not visible to the public. All six returned 200.

End-to-end against a real caller: a throwaway shallow clone of Comfy-Org/comfy-cli at 9f42171 (untouched, read-only, deleted after).

Before: Result: 252 internal-only reference(s) found. — 58 repo-reference findings, 194 ticket-id findings.
After: Result: 223 internal-only reference(s) found. — 29 repo-reference findings, 194 ticket-id findings.

Exactly the −29 predicted, split 21 / 3 / 2 / 1 / 1 / 1 across the six, matching the counts this change was sized against. No other finding moved, and the exit code is still 1 — this does not turn that repo's build green, and is not meant to.

The remainder this does NOT fix (223 findings)

Same sweep pointed at the half that is out of scope here, so the number is on the record rather than left to be discovered after merge:

194 are ticket-shaped ids in that repo's own source and tests. Scrubbing them is that repo's work, not the allowlist's.
19 are model-repository names read out of workflow JSON fixtures and a preflight table, where the reference is a huggingface.co/Comfy-Org/<model> URL that the checker's matcher reads as a GitHub repo path. That is checker matching logic and is deliberately not touched here — allowlisting a Hugging Face namespace into a GitHub default-deny list would be the wrong fix.
10 are references to Comfy-Org repos that are genuinely not public. Those are real findings; they belong to the consuming repo, and they are not named here because this PR body is world-readable.

Caller pins

Left to bump-public-repo-hygiene-callers.yml, and I checked that this is safe rather than assuming it. The change lands in .github/public-repo-hygiene/check_public_repo_hygiene.py, which is inside that fleet's paths: filter and outside both of its exclusions (tests/**, README.md) — so the fleet fires on merge. The README/docs half of this PR is excluded from the filter, which is correct: it is not something a pinned caller executes.

The fleet is live, not a no-op: its roster secret exists and was last set 2026-08-22, and the one caller I can see from outside is currently pinned at 4df9bf6 — the exact current tip of main — on both of its two pin sites (uses: and workflows_ref:). So a fleet that bumps zero callers is not the state here.

Judgment calls

Re-sorting existing entries is a bigger diff than the six additions. It is required by the "alphabetically placed with the existing entries" criterion — placing six entries alphabetically into a list that is not itself sorted is not a well-defined operation — and by the new sortedness test, which cannot pass over the pre-existing order. It is provably content-neutral: parsing the literal before and after gives 13 → 19 entries, six added, zero removed, and no pre-existing duplicates. That assertion is now permanent, not just something I ran once.
Sort key is str.casefold, not the default. A case-sensitive sort would require ComfyUI to sort before comfy-cli, which reads as unsorted to a human and does not match how membership is tested (which is casefolded).
The team allowlist is covered too. Only the repo allowlist was asked for; PUBLIC_COMFY_ORG_TEAMS already happened to be sorted, so extending the same assertion to it was free and stops the identical drift one line down.

Unexercised artifacts

The parent ticket's full triage and the two sibling tickets referenced from the description are Linear issues; their bodies were not fetched into this run and are not reachable from here, so I worked from the description and the numbers I could reproduce myself rather than from their content. The 252 → 223 reproduction above is the substitute: every count this change was sized against was re-derived locally instead of taken on trust.
The comfy-cli PRs named as the provenance of the original report were not opened or read. Nothing in this change depends on them.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py': 118 tests, OK (1 skipped, pre-existing). Negative-control runs on a deliberately unsorted literal and a deliberately duplicated entry: each failed the intended assertion, tree restored after. check_public_repo_hygiene.py --root <throwaway comfy-cli clone>: 252 findings before, 223 after, exit 1 both times.
Deviations: none — all four acceptance criteria met. The fourth (caller pins) is satisfied by deferral to the bump fleet, with the evidence above that the fleet is live and in-scope for this path.

_{🔄 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/212 **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-8855-public-repo-hygiene-allowlist` --- ### 📝 Commits (2) - [`6c4251e`](https://github.com/Comfy-Org/github-workflows/commit/6c4251ec2d4a44df8876d23e155e11e603e22df7) fix(public-repo-hygiene): allowlist six verified-public Comfy-Org repos (BE-8855) - [`a5f6305`](https://github.com/Comfy-Org/github-workflows/commit/a5f6305e09950216e24f8e8848f1960cc71380cc) fix(public-repo-hygiene): casefold the allowlist duplicate check, document the sort (BE-8855) ### 📊 Changes **3 files changed** (+161 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+15 -2) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+38 -10) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+108 -0) </details> ### 📄 Description ## ELI-5 The hygiene checker only knows a handful of Comfy-Org repos are public, and it treats everything else as a possible leak. Six repos that really are public were missing from that list, so every mention of them was being reported as a leak. This adds the six, sorts the list alphabetically so the next person can actually scan it, and adds a test that keeps it sorted and duplicate-free. ## What changed - Adds six repos to `PUBLIC_COMFY_ORG_REPOS` in `.github/public-repo-hygiene/check_public_repo_hygiene.py`: `workflow_templates`, `ComfyUI-Manager`, `comfy-skills`, `CustomNodeComfyMath`, `cookiecutter-comfy-extension`, `ComfyUI-test-framework`. Each carries a one-line comment recording where the reference came from, per the README runbook. - Re-sorts the whole literal into case-insensitive alphabetical order. Three earlier additions had accreted at the bottom in arrival order, and the two `ComfyUI*` entries were already out of order relative to each other. - Adds `AllowlistSourceOrderTest`, which parses the module's own AST and asserts both allowlist literals (repos and teams) are sorted and duplicate-free, and that the literal it read is the one the module actually exports. - Documents the ordering rule in the "Adding a repo to the allowlist" runbook in `.github/public-repo-hygiene/README.md`. Purely additive to the allowlist. It loosens nothing that default-deny was protecting: the list contains public names only by design, which is the property that makes it safe to host in a public repo in the first place. ## Why the AST, not a runtime assertion A `frozenset` has no order at run time, so the source text is the only place the ordering exists — and it is exactly the view a human reads to answer "is `<name>` already in here?" before appending a duplicate to the bottom. An exact textual duplicate is also invisible to every other assertion in the suite, because the set literal collapses `{"a", "a"}` to `{"a"}` before any test can see it. The AST is the one view that still has both copies. Both halves were falsified rather than assumed: inserting an out-of-order entry failed `test_repo_allowlist_is_sorted_and_duplicate_free` on the ordering assertion, and inserting an in-order exact duplicate failed it on the duplicate assertion. The tree was restored between each. ## Verification Each of the six re-verified PUBLIC at implementation time (2026-08-23) rather than trusting the list I was handed, by two independent paths: | repo | `gh repo view --json visibility` | unauthenticated `api.github.com/repos/Comfy-Org/<name>` | | -- | -- | -- | | `workflow_templates` | PUBLIC | 200 | | `ComfyUI-Manager` | PUBLIC | 200 | | `comfy-skills` | PUBLIC | 200 | | `CustomNodeComfyMath` | PUBLIC | 200 | | `cookiecutter-comfy-extension` | PUBLIC | 200 | | `ComfyUI-test-framework` | PUBLIC | 200 | The unauthenticated check is the one that matters for a default-deny control: `gh repo view` runs as an org member, so it will happily print metadata for a private repo. An anonymous `404` would mean the repo is not visible to the public. All six returned `200`. End-to-end against a real caller: a throwaway shallow clone of `Comfy-Org/comfy-cli` at `9f42171` (untouched, read-only, deleted after). - Before: `Result: 252 internal-only reference(s) found.` — 58 repo-reference findings, 194 ticket-id findings. - After: `Result: 223 internal-only reference(s) found.` — 29 repo-reference findings, 194 ticket-id findings. Exactly the −29 predicted, split 21 / 3 / 2 / 1 / 1 / 1 across the six, matching the counts this change was sized against. No other finding moved, and the exit code is still `1` — this does not turn that repo's build green, and is not meant to. ## The remainder this does NOT fix (223 findings) Same sweep pointed at the half that is out of scope here, so the number is on the record rather than left to be discovered after merge: - **194** are ticket-shaped ids in that repo's own source and tests. Scrubbing them is that repo's work, not the allowlist's. - **19** are model-repository names read out of workflow JSON fixtures and a preflight table, where the reference is a `huggingface.co/Comfy-Org/<model>` URL that the checker's matcher reads as a GitHub repo path. That is checker matching logic and is deliberately not touched here — allowlisting a Hugging Face namespace into a GitHub default-deny list would be the wrong fix. - **10** are references to Comfy-Org repos that are genuinely not public. Those are real findings; they belong to the consuming repo, and they are not named here because this PR body is world-readable. ## Caller pins Left to `bump-public-repo-hygiene-callers.yml`, and I checked that this is safe rather than assuming it. The change lands in `.github/public-repo-hygiene/check_public_repo_hygiene.py`, which is inside that fleet's `paths:` filter and outside both of its exclusions (`tests/**`, `README.md`) — so the fleet fires on merge. The README/docs half of this PR is excluded from the filter, which is correct: it is not something a pinned caller executes. The fleet is live, not a no-op: its roster secret exists and was last set 2026-08-22, and the one caller I can see from outside is currently pinned at `4df9bf6` — the exact current tip of `main` — on both of its two pin sites (`uses:` and `workflows_ref:`). So a fleet that bumps zero callers is not the state here. ## Judgment calls - **Re-sorting existing entries is a bigger diff than the six additions.** It is required by the "alphabetically placed with the existing entries" criterion — placing six entries alphabetically into a list that is not itself sorted is not a well-defined operation — and by the new sortedness test, which cannot pass over the pre-existing order. It is provably content-neutral: parsing the literal before and after gives 13 → 19 entries, six added, **zero removed**, and no pre-existing duplicates. That assertion is now permanent, not just something I ran once. - **Sort key is `str.casefold`, not the default.** A case-sensitive sort would require `ComfyUI` to sort before `comfy-cli`, which reads as unsorted to a human and does not match how membership is tested (which is casefolded). - **The team allowlist is covered too.** Only the repo allowlist was asked for; `PUBLIC_COMFY_ORG_TEAMS` already happened to be sorted, so extending the same assertion to it was free and stops the identical drift one line down. ## Unexercised artifacts - The parent ticket's full triage and the two sibling tickets referenced from the description are Linear issues; their bodies were not fetched into this run and are not reachable from here, so I worked from the description and the numbers I could reproduce myself rather than from their content. The 252 → 223 reproduction above is the substitute: every count this change was sized against was re-derived locally instead of taken on trust. - The comfy-cli PRs named as the provenance of the original report were not opened or read. Nothing in this change depends on them. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'`: 118 tests, OK (1 skipped, pre-existing). Negative-control runs on a deliberately unsorted literal and a deliberately duplicated entry: each failed the intended assertion, tree restored after. `check_public_repo_hygiene.py --root <throwaway comfy-cli clone>`: 252 findings before, 223 after, exit 1 both times. - **Deviations:** none — all four acceptance criteria met. The fourth (caller pins) is satisfied by deferral to the bump fleet, with the evidence above that the fleet is live and in-scope for this path. --- <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:29 +00:00