[PR #216] [MERGED] fix(public-repo-hygiene): stop reading huggingface.co model paths as GitHub repo references (BE-8910) #223

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/216
Author: @mattmillerai
Created: 8/24/2026
Status: ✅ Merged
Merged: 8/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8910-hf-host-aware-repo-refs

📝 Commits (4)

e413739 fix(public-repo-hygiene): stop reading huggingface.co model paths as GitHub repo references (BE-8910)
ba11694 fix(public-repo-hygiene): require the non-GitHub host to be the URL authority (BE-8910)
12782bd fix(public-repo-hygiene): close three fail-open holes in the non-GitHub host skip (BE-8910)
034a009 Merge origin/main into matt/be-8910-hf-host-aware-repo-refs

📊 Changes

5 files changed (+438 additions, -2 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+15 -2)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+146 -0)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+185 -0)
📝 .github/workflows/test-public-repo-hygiene.yml (+65 -0)
📝 docs/callers/public-repo-hygiene.md (+27 -0)

📄 Description

Read this first — the ground shifted under this PR. #217 merged to main on 2026-08-24 at 19:05 UTC with a parallel, independently-developed implementation of this PR's shape 1. That implementation is better than the one this branch carried, and it is already shipped. This branch has been rebuilt on top of it: it no longer contains a host-skip implementation at all. What is left is the one thing #217 does not do.

ELI-5

Comfy-Org/something is not automatically a GitHub repo — Comfy-Org publishes model weights on Hugging Face under the same org name, and the leak checker was reading those model names as references to private GitHub repos. #217 fixed that for the URL spelling: https://huggingface.co/Comfy-Org/<model> now clears, because the host sits right in front of the name.

It does not fix the markdown link spelling, where the same name appears twice on the line:

[Comfy-Org/Qwen-Image_ComfyUI](https://huggingface.co/Comfy-Org/Qwen-Image_ComfyUI/tree/main)

The target has a host in front of it and clears. The label is a bare token preceded by [, so no offset the host scan produces can reach it, and it stays a finding — as unfixable from the caller's side as the URL was, for exactly the same two reasons (the name cannot go on a GitHub allowlist because there is no GitHub repo of that name, and the link is the product content telling a user where to download the weights). This PR clears the label, and only when the link target is a model-host URL naming the same repo.

Measured effect — this is the whole delta

Swept comfy-org/comfy-cli at e64435a with the caller's own ticket_allowlist: HAILUO-03:

checker	category-3 findings
`main` before `#217`	29
`main` today (`#217`)	12
this branch	10

The two that clear are exactly the label shape, both on tests/comfy_cli/fixtures/gallery/02_qwen_Image_edit_subgraphed.json:117 — Comfy-Org/Qwen-Image_ComfyUI and Comfy-Org/Qwen-Image-Edit_ComfyUI. A full-output diff of the two runs shows no other line moved, and categories 1 and 2 are untouched.

The remaining 10 are Comfy-Org/cloud (8), Comfy-Org/cql (1) and Comfy-Org/comfy-knowledge (1) — genuine references to non-public GitHub repos, correctly flagged, and out of scope here: they need scrubbing in comfy-cli, not a checker change.

What changed

.github/public-repo-hygiene/check_public_repo_hygiene.py — one new skip, gated on both ends, and nothing else in the verdict path:

_labels_non_github_link runs the same host test as the line-level scan (MODEL_HOST_PREFIX_RE, main's) against the link destination rather than against the line. It clears the label only when a model-host prefix ends exactly where the target's own reference starts, and the two names match.
_md_link_destination reads that destination under a length bound and reports whether the bound cut it.

Also updated: the checker README, docs/callers/public-repo-hygiene.md, and the CI smoke fixtures in test-public-repo-hygiene.yml. #217 touched none of those three, so the smoke layer had no model-host coverage at all before this PR — the clean-repo fixture and nine check_one cases now cover both shapes end to end.

What this branch DELETED to get here

The merge commit (034a009) removes this branch's own host machinery wholesale — _NON_GITHUB_HOST_PREFIX_RE, _NON_GITHUB_HOST_AUTHORITY_L, _URL_TOKEN_CHARS, _HOST_PREFIX_LOOKBACK, NON_GITHUB_ORG_NAMESPACE_HOSTS, _NON_GITHUB_HOST_PATH_PREFIXES and _is_non_github_host_prefixed — and keeps main's. That is not deference; #217's version is genuinely ahead on three counts this branch never had: the collections/ route, an api/ that requires a resource segment (Hugging Face routes no /api/<owner>/), and (?-i:…)-scoped route segments so /MODELS/ does not clear. Both branches independently landed on the same central insight — that a suppressor cannot reuse a detector's anchor, since the looseness that over-flags safely in one fails open in the other.

16 tests were dropped with that machinery, each a duplicate of coverage #217 landed in RepoReferenceCategoryTest (host spellings, routes, ports, lookalikes, the IDN neighbour, path-segment authority). 12 remain, all of them the label shape and the ways clearing it could become a bypass.

The three fail-open holes the review panel found, carried forward

The cursor-review panel ran against e413739, one commit behind the authority anchor, so two of its eight findings were already closed and one was stale in its stated mechanism. Three were real, all of them reproduced by probe before anything was touched, and all three apply unchanged to the label skip:

The target was compared on a PARTIAL read. REPO_REF_RE's name class is ASCII, so a target naming <private>‐model (U+2010) or <private>%2Dother read as <private>, compared equal to the label and silenced it — the prefix-vs-full-name hole _nonascii_tail closed on the other side, reached through this door. The target's name must now span a whole path segment.
The destination bound truncated rather than declined, so a URL whose first 256 characters happened to end at a name boundary compared equal too. Declining an over-long destination is not the fix — real Hugging Face file URLs pass the bound routinely (…/resolve/main/split_files/diffusion_models/<file>.safetensors), and refusing them re-opens the false positives this skip exists to close. The truncation is detected, and only the comparison it can corrupt is refused.
The skip only looked forward, so a bare token in prose merely followed by ](<matching URL> was read as link text with no [ ever opening a label. The reference now has to open the label — the shape the docstring, the README and the caller doc all describe, and the shape every instance in the swept corpus is written in.

The panel's fourth real finding, the non-ASCII neighbour (https://éhuggingface.co/… silently clearing, which this PR's body and README had both wrongly claimed was a bare-spelling-only residual), needs no carrying: main's _AUTHORITY_L is a positive delimiter class, so that spelling fails it already and #217 pins it with a test.

Two panel findings were declined rather than fixed, with reasoning on their threads: the .git-on-the-target suggestion rests on a premise that is inverted (the .git strip happens after the label skip, so both sides are compared on equal terms), and the path-segment/authority finding was already closed a commit earlier.

Residual risk a reviewer should weigh

The clearing condition is a suppressor on a default-deny leak guard, so every widening is the dangerous direction. Both gates exist for that reason: the name must match, and the host must be the destination's authority. Every way I could think of to reach the skip without a real model-host link is pinned as a test that still expects a finding.
A destination exactly at the length bound reads as truncated when it is not, so a label over one of those over-flags. One length, safe direction, documented in the helper.
Reference-style markdown links are not handled. [Comfy-Org/<model>][ref] with the URL defined elsewhere is still a finding; no fixture in the swept corpus uses that shape, and clearing it would mean resolving link definitions across lines, which a line-oriented scan cannot do.
A label with leading text ([see Comfy-Org/x](…)) is now a finding, because the reference has to open the label. Every label-shape instance in the swept corpus is written [Comfy-Org/<name>](, and the sweep is byte-identical across that tightening — but it is a real narrowing, not a free one.
Unmeasured beyond one caller. No repo other than comfy-org/comfy-cli was swept; the rest of the enrolled fleet is unmeasured for this change.

Unexercised artifacts

The source ticket cites a parent tracking issue and three sibling issues by identifier. Those live in an issue tracker this run has no access to, so their bodies and comments were never read. Their in-repo traces were read. #217's own review threads were not read either — only its merged code and commit messages.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py' — 165 tests, OK, 1 skipped; cursor-review 194 OK; agents-md-integrity 46 OK; groom 365 OK, 1 skipped; bash .github/bump-callers/tests/test_bump_callers.sh — 437 passed, 0 failed; shellcheck -x on the bump-callers scripts — clean; python3 -m py_compile on both changed Python files — clean; before/after full-output sweep of comfy-org/comfy-cli at e64435a — main 12 category-3 findings, this branch 10, no other category moved, and byte-identical to this branch's own pre-merge output; every bypass the review panel raised reproduced by probe against the pre-fix code and re-probed against the merged tree; CI smoke fixtures replicated by hand — clean fixture exit 0 (including the no-scheme spellings and an over-long-but-complete destination), all nine dirty cases exit 1 with the expected message; gh repo view (5 × not found) and the Hugging Face model API (5 × 200) run against the five names.
Deviations: the branch was rebuilt on main after #217 shipped a parallel implementation of shape 1; this PR's own host machinery was deleted rather than reconciled, because #217's is ahead on three narrowings and is already merged. 16 tests were dropped as duplicates of coverage #217 landed. Two review-panel findings were declined rather than fixed, with reasoning left on their threads. Beyond the stated scope, the checker README, the caller-facing doc and the CI smoke fixtures are updated in the same branch, because #217 updated none of them and leaving them describing the pre-#217 rule would have made them state the opposite of the code. One thing this PR does NOT fix and did not touch: this repo's own AGENTS.md is 305 lines against the 200-line hard ceiling its own agents-md-integrity checker enforces — pre-existing on main, ungated here because this repo has no ci-agents-md-integrity.yml caller.

_{🔄 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/216 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8910-hf-host-aware-repo-refs` --- ### 📝 Commits (4) - [`e413739`](https://github.com/Comfy-Org/github-workflows/commit/e413739c0951cd80f3fc827d0a15edaadca50d47) fix(public-repo-hygiene): stop reading huggingface.co model paths as GitHub repo references (BE-8910) - [`ba11694`](https://github.com/Comfy-Org/github-workflows/commit/ba116947a33d2388b2bdb85883200e633c68782f) fix(public-repo-hygiene): require the non-GitHub host to be the URL authority (BE-8910) - [`12782bd`](https://github.com/Comfy-Org/github-workflows/commit/12782bd22f84a0a78932abe3744a971d0ee4af76) fix(public-repo-hygiene): close three fail-open holes in the non-GitHub host skip (BE-8910) - [`034a009`](https://github.com/Comfy-Org/github-workflows/commit/034a009993458d4f27bf8be6b0591b601dd82e38) Merge origin/main into matt/be-8910-hf-host-aware-repo-refs ### 📊 Changes **5 files changed** (+438 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+15 -2) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+146 -0) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+185 -0) 📝 `.github/workflows/test-public-repo-hygiene.yml` (+65 -0) 📝 `docs/callers/public-repo-hygiene.md` (+27 -0) </details> ### 📄 Description > **Read this first — the ground shifted under this PR.** `#217` merged to `main` on 2026-08-24 at 19:05 UTC with a parallel, independently-developed implementation of this PR's shape 1. That implementation is better than the one this branch carried, and it is already shipped. This branch has been rebuilt on top of it: **it no longer contains a host-skip implementation at all.** What is left is the one thing `#217` does not do. ## ELI-5 `Comfy-Org/something` is not automatically a GitHub repo — Comfy-Org publishes model weights on Hugging Face under the same org name, and the leak checker was reading those model names as references to private GitHub repos. `#217` fixed that for the **URL** spelling: `https://huggingface.co/Comfy-Org/<model>` now clears, because the host sits right in front of the name. It does not fix the **markdown link** spelling, where the same name appears twice on the line: ```markdown [Comfy-Org/Qwen-Image_ComfyUI](https://huggingface.co/Comfy-Org/Qwen-Image_ComfyUI/tree/main) ``` The *target* has a host in front of it and clears. The *label* is a bare token preceded by `[`, so no offset the host scan produces can reach it, and it stays a finding — as unfixable from the caller's side as the URL was, for exactly the same two reasons (the name cannot go on a GitHub allowlist because there is no GitHub repo of that name, and the link is the product content telling a user where to download the weights). This PR clears the label, and only when the link target is a model-host URL naming the **same** repo. ## Measured effect — this is the whole delta Swept `comfy-org/comfy-cli` at `e64435a` with the caller's own `ticket_allowlist: HAILUO-03`: | checker | category-3 findings | |---|---| | `main` before `#217` | 29 | | `main` today (`#217`) | **12** | | this branch | **10** | The two that clear are exactly the label shape, both on `tests/comfy_cli/fixtures/gallery/02_qwen_Image_edit_subgraphed.json:117` — `Comfy-Org/Qwen-Image_ComfyUI` and `Comfy-Org/Qwen-Image-Edit_ComfyUI`. A full-output diff of the two runs shows no other line moved, and categories 1 and 2 are untouched. The remaining 10 are `Comfy-Org/cloud` (8), `Comfy-Org/cql` (1) and `Comfy-Org/comfy-knowledge` (1) — genuine references to non-public GitHub repos, correctly flagged, and out of scope here: they need scrubbing in comfy-cli, not a checker change. ## What changed `.github/public-repo-hygiene/check_public_repo_hygiene.py` — one new skip, gated on both ends, and nothing else in the verdict path: - `_labels_non_github_link` runs the **same** host test as the line-level scan (`MODEL_HOST_PREFIX_RE`, `main`'s) against the link **destination** rather than against the line. It clears the label only when a model-host prefix ends exactly where the target's own reference starts, **and** the two names match. - `_md_link_destination` reads that destination under a length bound and reports whether the bound **cut** it. Also updated: the checker README, `docs/callers/public-repo-hygiene.md`, and the CI smoke fixtures in `test-public-repo-hygiene.yml`. `#217` touched none of those three, so the smoke layer had **no** model-host coverage at all before this PR — the clean-repo fixture and nine `check_one` cases now cover both shapes end to end. ## What this branch DELETED to get here The merge commit (`034a009`) removes this branch's own host machinery wholesale — `_NON_GITHUB_HOST_PREFIX_RE`, `_NON_GITHUB_HOST_AUTHORITY_L`, `_URL_TOKEN_CHARS`, `_HOST_PREFIX_LOOKBACK`, `NON_GITHUB_ORG_NAMESPACE_HOSTS`, `_NON_GITHUB_HOST_PATH_PREFIXES` and `_is_non_github_host_prefixed` — and keeps `main`'s. That is not deference; `#217`'s version is genuinely ahead on three counts this branch never had: the `collections/` route, an `api/` that requires a resource segment (Hugging Face routes no `/api/<owner>/`), and `(?-i:…)`-scoped route segments so `/MODELS/` does not clear. Both branches independently landed on the same central insight — that a *suppressor* cannot reuse a *detector's* anchor, since the looseness that over-flags safely in one fails open in the other. 16 tests were dropped with that machinery, each a duplicate of coverage `#217` landed in `RepoReferenceCategoryTest` (host spellings, routes, ports, lookalikes, the IDN neighbour, path-segment authority). 12 remain, all of them the label shape and the ways clearing it could become a bypass. ## The three fail-open holes the review panel found, carried forward The cursor-review panel ran against `e413739`, one commit behind the authority anchor, so two of its eight findings were already closed and one was stale in its stated mechanism. Three were real, all of them reproduced by probe before anything was touched, and all three apply unchanged to the label skip: 1. **The target was compared on a PARTIAL read.** `REPO_REF_RE`'s name class is ASCII, so a target naming `<private>‐model` (U+2010) or `<private>%2Dother` read as `<private>`, compared equal to the label and silenced it — the prefix-vs-full-name hole `_nonascii_tail` closed on the other side, reached through this door. The target's name must now span a **whole path segment**. 2. **The destination bound truncated rather than declined**, so a URL whose first 256 characters happened to end at a name boundary compared equal too. Declining an over-long destination is *not* the fix — real Hugging Face file URLs pass the bound routinely (`…/resolve/main/split_files/diffusion_models/<file>.safetensors`), and refusing them re-opens the false positives this skip exists to close. The truncation is **detected**, and only the comparison it can corrupt is refused. 3. **The skip only looked forward**, so a bare token in prose merely followed by `](<matching URL>` was read as link text with no `[` ever opening a label. The reference now has to **open** the label — the shape the docstring, the README and the caller doc all describe, and the shape every instance in the swept corpus is written in. The panel's fourth real finding, the non-ASCII neighbour (`https://éhuggingface.co/…` silently clearing, which this PR's body and README had both wrongly claimed was a bare-spelling-only residual), needs no carrying: `main`'s `_AUTHORITY_L` is a positive delimiter class, so that spelling fails it already and `#217` pins it with a test. Two panel findings were declined rather than fixed, with reasoning on their threads: the `.git`-on-the-target suggestion rests on a premise that is inverted (the `.git` strip happens *after* the label skip, so both sides are compared on equal terms), and the path-segment/authority finding was already closed a commit earlier. ## Residual risk a reviewer should weigh - **The clearing condition is a suppressor on a default-deny leak guard**, so every widening is the dangerous direction. Both gates exist for that reason: the name must match, and the host must be the destination's authority. Every way I could think of to reach the skip without a real model-host link is pinned as a test that still expects a finding. - **A destination exactly at the length bound reads as truncated when it is not**, so a label over one of those over-flags. One length, safe direction, documented in the helper. - **Reference-style markdown links are not handled.** `[Comfy-Org/<model>][ref]` with the URL defined elsewhere is still a finding; no fixture in the swept corpus uses that shape, and clearing it would mean resolving link definitions across lines, which a line-oriented scan cannot do. - **A label with leading text** (`[see Comfy-Org/x](…)`) is now a finding, because the reference has to open the label. Every label-shape instance in the swept corpus is written `[Comfy-Org/<name>](`, and the sweep is byte-identical across that tightening — but it is a real narrowing, not a free one. - **Unmeasured beyond one caller.** No repo other than `comfy-org/comfy-cli` was swept; the rest of the enrolled fleet is unmeasured for this change. ## Unexercised artifacts The source ticket cites a parent tracking issue and three sibling issues by identifier. Those live in an issue tracker this run has no access to, so their bodies and comments were never read. Their in-repo traces were read. `#217`'s own review threads were not read either — only its merged code and commit messages. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'` — 165 tests, OK, 1 skipped; cursor-review 194 OK; agents-md-integrity 46 OK; groom 365 OK, 1 skipped; `bash .github/bump-callers/tests/test_bump_callers.sh` — 437 passed, 0 failed; `shellcheck -x` on the bump-callers scripts — clean; `python3 -m py_compile` on both changed Python files — clean; before/after full-output sweep of `comfy-org/comfy-cli` at `e64435a` — `main` 12 category-3 findings, this branch 10, no other category moved, and byte-identical to this branch's own pre-merge output; every bypass the review panel raised reproduced by probe against the pre-fix code and re-probed against the merged tree; CI smoke fixtures replicated by hand — clean fixture exit 0 (including the no-scheme spellings and an over-long-but-complete destination), all nine dirty cases exit 1 with the expected message; `gh repo view` (5 × not found) and the Hugging Face model API (5 × 200) run against the five names. - **Deviations:** the branch was rebuilt on `main` after `#217` shipped a parallel implementation of shape 1; this PR's own host machinery was deleted rather than reconciled, because `#217`'s is ahead on three narrowings and is already merged. 16 tests were dropped as duplicates of coverage `#217` landed. Two review-panel findings were declined rather than fixed, with reasoning left on their threads. Beyond the stated scope, the checker README, the caller-facing doc and the CI smoke fixtures are updated in the same branch, because `#217` updated none of them and leaving them describing the pre-`#217` rule would have made them state the opposite of the code. One thing this PR does NOT fix and did not touch: this repo's own `AGENTS.md` is 305 lines against the 200-line hard ceiling its own `agents-md-integrity` checker enforces — pre-existing on `main`, ungated here because this repo has no `ci-agents-md-integrity.yml` caller. --- <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