[PR #231] [MERGED] feat(public-repo-hygiene): scan the tracked path string, not just file contents #237

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9399-hygiene-scan-tracked-path

📝 Commits (2)

0a2178f feat(public-repo-hygiene): scan the tracked path string, not just file contents
8dccd72 fix(public-repo-hygiene): gate URL-syntax suppressors off the path surface, cap path findings, read surrogates as name (BE-9399)

📊 Changes

6 files changed (+782 additions, -221 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+60 -3)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+353 -215)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+342 -0)
📝 .github/workflows/public-repo-hygiene.yml (+3 -2)
📝 README.md (+1 -1)
📝 docs/callers/public-repo-hygiene.md (+23 -0)

📄 Description

ELI-5

The leak checker reads what is inside your files. It never read the file names. So a public repo could contain a directory literally called docs/Comfy-Org/<a-private-repo>/, with a completely empty file in it, and the check went green — while GitHub happily showed that private repo's name to anyone browsing the repository. This PR makes the checker read the tracked path as well as the contents, using the exact same matcher so every allowlist and every false-positive suppressor applies to both.

What changed

_file_findings's per-line body is factored out into a shared _line_findings(location, line, ticket_allowlist, owner_span), and run_checks now runs it over the tracked path itself — right after the exclusion check, before check_file. Deliberately one matcher, not two: TICKET_ALLOWLIST / TICKET_ALLOWED_PREFIXES, the --ticket-allow extension, the model-host and markdown-link-label suppressors, the homoglyph handling and the .git/period strips all reach the new surface for free, and there is no second place to forget an allowlist entry.

Details worth knowing at review time:

Every non-excluded entry gets its path scanned, including the ones check_file declines (binary, non-UTF-8, submodule gitlink, LFS stub, FIFO, unreadable). The path is published in the tree whatever the entry type. It does not make such an entry count as SCANNED: — that number is files read as text.
owner_span is always None on a path. A path string is not a CODEOWNERS owner line, even the path of a CODEOWNERS file. I passed the precomputed span rather than an is_codeowners flag because deriving the span also needs the line number (the BOM-at-offset-0 rule), and a line number means nothing on a path — owner_span=None reads as the fact it is, where lineno=1 would have been a lie in the signature. Same behaviour either way.
exclude_paths: covers the new surface, because the existing _is_excluded continue sits above the path scan. An excluded path is not scanned as a path either, and still reports its skipped-file count so the hole stays named in the log.
Findings are labelled <path> (tracked path): rather than <path>:<lineno>: — "rename the file" and "edit the file" are different fixes. Merged into the same per-run MAX_FINDINGS_TOTAL cap.
No regex/boundary change. / is not in REPO_REF_RE's left lookbehind, so docs/Comfy-Org/x/y.md matches while aComfy-Org/x does not; \b fires at both / and -, so notes/TEAM-1234/plan.md and TEAM-1234-notes.md both match. Same token rules as content, verified empirically rather than assumed.
One behaviour change outside the plan, and why. _emit now lists findings before the zero-coverage verdict. Until this PR, findings implied scanned >= 1; now a repo of nothing but binaries can leak in its own file listing while scanned stays 0, and the old ordering returned exit 2 having printed the finding nowhere — sending the operator after a configuration problem instead of the leak. Exit codes are byte-for-byte unchanged (still 2 there); only the output is added. Pinned by test_a_path_finding_is_listed_even_when_nothing_was_scanned.

Rollout is the ordinary one: callers pin workflows_ref by SHA, so the new surface reaches them through the normal bump-public-repo-hygiene-callers.yml flow.

False-positive risk — measured, not asserted

Every tracked path of nine Comfy-Org public repos, fed through the real _line_findings:

repo	tracked paths	path findings
github-workflows	63	0
ComfyUI	957	0
ComfyUI_frontend	4150	0
comfy-cli	317	0
comfy-api-proxy	47	0
litegraph.js	171	0
comfy-typescript-sdk	60	0
comfy-python-sdk	58	0
docs	5592	0
total	11,415	0

Enabling this reddens no caller CI. This repo's own working tree (176 tracked paths, and it is deliberately not self-enrolled) also produces 0 path findings. Overhead is ~4 µs per path (47 ms across 11,187 paths), against a content scan that reads up to 5 MiB per file.

And the portion this does not cover, swept with the same tool: the checker reads the work tree, so a path that once existed and was renamed or deleted stays in every clone and is still not examined — nor are branch and tag names. Across the same nine repos that is 3,337 history-only paths, which likewise produce 0 would-be findings today. Real gap, empirically empty, now written down as a known limitation rather than left implicit. (Caveat: measured over the refs present in the local clones, which may not carry every remote branch.)

Verification

python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py' — 178 passed, 0 failed (1 skipped, pre-existing; 165 → 178, +13 new). The pre-existing 165 pass unchanged, which is the zero-new-false-positive claim.
Mutation check on the new tests: with the path scan neutered, 7 of the 13 go red (the other 6 are clean-path negatives that pass either way by construction).
Every run: step of .github/workflows/test-public-repo-hygiene.yml extracted and executed locally — all 13 smoke steps pass, exit 0.
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, 0 exempt.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (1 pre-existing CODEOWNERS warning).
bash .github/bump-callers/tests/test_paths_contract.sh — 70 passed, 0 failed.
Neighbouring Python suites re-run to catch cross-area breakage: agents-md-integrity 46, cursor-review 305, groom 365, refresh-reviewers 48, workflow-pins 272 — all OK.

New tests (13): a private repo name in a path fails; an allowlisted repo name in a path passes; ticket-shaped directory component and filename prefix both fail; built-in and caller-extended acronyms in a path pass; the (tracked path) vs :<lineno> labels distinguish the two surfaces when a name appears in both; aComfy-Org/x does not flag (left-boundary pin); an internal-marker host in a path flags; the npm-scope and model-host suppressors carry over; the nested-model-host over-flag is pinned; exclude_paths: suppresses and still counts; a path finding fires on a binary entry and on a dangling symlink whose body is never read; and the zero-coverage output case above.

Residual

.github/lint/README.md KNOWN LIMITATION 8 is not updated. That file does not exist on main — it arrives with PR #224, which is still open. Its limitation-8 sentence "neither checker scans the tracked path itself" becomes stale the moment this merges, and #224's branch will need that sentence corrected on rebase. I could not edit it here without touching another open PR's branch, so it is called out rather than done. Whichever of the two lands second owns the fix.
A model-host mirror path over-flags (root or nested, since review round 1). models/hf.co/Comfy-Org/<model>/config.json is reported while the same mirror at the tree root is suppressed: MODEL_HOST_PREFIX_RE's left anchor rejects a preceding /, and every segment of a path has one. This is the over-flag direction (a leak guard should be wrong that way), one exclude_paths: entry clears it, and the shape occurs zero times across the 11,415 paths measured — so it is pinned by a test and documented as a known limitation rather than narrowed. Narrowing it would mean loosening a suppressor, which fails open; that is a deliberate non-goal here and would want its own change with its own evidence.
Repository history and refs remain unscanned, per the 3,337-path sweep above. Scrubbing a name out of the tree does not scrub it out of the history that still carries it. Out of scope for a checker whose contract is git ls-files, but it is the honest remaining hole and is now stated in both READMEs.
Caller repos are not bumped by this PR. The new surface reaches them only when bump-public-repo-hygiene-callers.yml moves their pinned SHA. Nothing here does that, and no caller CI changes until it happens.
Unexercised artifact: the originating investigation's findings write-up is not reachable from this environment, so the 11,415-path figure was re-derived independently here rather than taken on trust — it reproduces exactly, which is the closest thing to exercising it that was available.

Provenance

Authored by: agent-work loop
Verified: public-repo-hygiene unittest 184 passed / 0 failed (1 skipped) after review round 1 (178 → 184); check_workflow_pins.py OK (11 workflows); check_agents_md.py passed (1 pre-existing warning); local path-surface re-sweep over every public clone on the host, 0 findings on public repos
Deviations: _line_findings takes a precomputed owner_span rather than an is_codeowners flag plus a line number (same behaviour, no fake lineno=1 on a path — rationale above); _emit output ordering changed so a finding is never swallowed by the zero-coverage verdict (exit codes unchanged); .github/lint/README.md left alone because PR #224 has not merged. Review round 1 (cursor-review panel, 9 threads): the two URL-syntax suppressors (model-host prefix, markdown link label) are now gated OFF the path surface via url_suppressors=False — a path has no authority, so they failed open there; a vendored mirror therefore over-flags at the root as well as nested. A surrogateescape code point (Cs) now continues a name in _nonascii_tail. Path findings are capped and PARTIAL-counted through _path_findings. Documented rather than changed: Comfy-Org/ComfyUI.md over-flags, lowercase ticket ids in paths miss, category-2 hosts need a following /, exit 2 may carry findings, and exclude_paths: costs contents coverage.

_{🔄 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/231 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/26/2026 **Status:** ✅ Merged **Merged:** 8/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9399-hygiene-scan-tracked-path` --- ### 📝 Commits (2) - [`0a2178f`](https://github.com/Comfy-Org/github-workflows/commit/0a2178f6cfe611a88d0e2117af03064cbbce8c31) feat(public-repo-hygiene): scan the tracked path string, not just file contents - [`8dccd72`](https://github.com/Comfy-Org/github-workflows/commit/8dccd72cc9d92412a8170f7c98cf6c1365fd71ef) fix(public-repo-hygiene): gate URL-syntax suppressors off the path surface, cap path findings, read surrogates as name (BE-9399) ### 📊 Changes **6 files changed** (+782 additions, -221 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+60 -3) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+353 -215) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+342 -0) 📝 `.github/workflows/public-repo-hygiene.yml` (+3 -2) 📝 `README.md` (+1 -1) 📝 `docs/callers/public-repo-hygiene.md` (+23 -0) </details> ### 📄 Description ## ELI-5 The leak checker reads what is *inside* your files. It never read the file *names*. So a public repo could contain a directory literally called `docs/Comfy-Org/<a-private-repo>/`, with a completely empty file in it, and the check went green — while GitHub happily showed that private repo's name to anyone browsing the repository. This PR makes the checker read the tracked path as well as the contents, using the exact same matcher so every allowlist and every false-positive suppressor applies to both. ## What changed `_file_findings`'s per-line body is factored out into a shared `_line_findings(location, line, ticket_allowlist, owner_span)`, and `run_checks` now runs it over the tracked path itself — right after the exclusion check, before `check_file`. Deliberately **one matcher, not two**: `TICKET_ALLOWLIST` / `TICKET_ALLOWED_PREFIXES`, the `--ticket-allow` extension, the model-host and markdown-link-label suppressors, the homoglyph handling and the `.git`/period strips all reach the new surface for free, and there is no second place to forget an allowlist entry. Details worth knowing at review time: - **Every non-excluded entry gets its path scanned**, including the ones `check_file` declines (binary, non-UTF-8, submodule gitlink, LFS stub, FIFO, unreadable). The path is published in the tree whatever the entry type. It does **not** make such an entry count as `SCANNED:` — that number is files read as *text*. - **`owner_span` is always `None` on a path.** A path string is not a CODEOWNERS owner line, even the path *of* a CODEOWNERS file. I passed the precomputed span rather than an `is_codeowners` flag because deriving the span also needs the line *number* (the BOM-at-offset-0 rule), and a line number means nothing on a path — `owner_span=None` reads as the fact it is, where `lineno=1` would have been a lie in the signature. Same behaviour either way. - **`exclude_paths:` covers the new surface**, because the existing `_is_excluded` `continue` sits above the path scan. An excluded path is not scanned as a path either, and still reports its skipped-file count so the hole stays named in the log. - **Findings are labelled `<path> (tracked path):`** rather than `<path>:<lineno>:` — "rename the file" and "edit the file" are different fixes. Merged into the same per-run `MAX_FINDINGS_TOTAL` cap. - **No regex/boundary change.** `/` is not in `REPO_REF_RE`'s left lookbehind, so `docs/Comfy-Org/x/y.md` matches while `aComfy-Org/x` does not; `\b` fires at both `/` and `-`, so `notes/TEAM-1234/plan.md` and `TEAM-1234-notes.md` both match. Same token rules as content, verified empirically rather than assumed. - **One behaviour change outside the plan, and why.** `_emit` now lists findings *before* the zero-coverage verdict. Until this PR, findings implied `scanned >= 1`; now a repo of nothing but binaries can leak in its own file listing while `scanned` stays `0`, and the old ordering returned exit 2 having printed the finding nowhere — sending the operator after a configuration problem instead of the leak. Exit codes are byte-for-byte unchanged (still 2 there); only the output is added. Pinned by `test_a_path_finding_is_listed_even_when_nothing_was_scanned`. Rollout is the ordinary one: callers pin `workflows_ref` by SHA, so the new surface reaches them through the normal `bump-public-repo-hygiene-callers.yml` flow. ## False-positive risk — measured, not asserted Every tracked path of nine Comfy-Org public repos, fed through the real `_line_findings`: | repo | tracked paths | path findings | |---|---:|---:| | github-workflows | 63 | 0 | | ComfyUI | 957 | 0 | | ComfyUI_frontend | 4150 | 0 | | comfy-cli | 317 | 0 | | comfy-api-proxy | 47 | 0 | | litegraph.js | 171 | 0 | | comfy-typescript-sdk | 60 | 0 | | comfy-python-sdk | 58 | 0 | | docs | 5592 | 0 | | **total** | **11,415** | **0** | Enabling this reddens no caller CI. This repo's own working tree (176 tracked paths, and it is deliberately not self-enrolled) also produces 0 path findings. Overhead is ~4 µs per path (47 ms across 11,187 paths), against a content scan that reads up to 5 MiB per file. **And the portion this does *not* cover, swept with the same tool:** the checker reads the work tree, so a path that once existed and was renamed or deleted stays in every clone and is still not examined — nor are branch and tag names. Across the same nine repos that is **3,337 history-only paths, which likewise produce 0 would-be findings** today. Real gap, empirically empty, now written down as a known limitation rather than left implicit. (Caveat: measured over the refs present in the local clones, which may not carry every remote branch.) ## Verification - `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'` — **178 passed, 0 failed** (1 skipped, pre-existing; 165 → 178, +13 new). The pre-existing 165 pass **unchanged**, which is the zero-new-false-positive claim. - Mutation check on the new tests: with the path scan neutered, 7 of the 13 go red (the other 6 are clean-path negatives that pass either way by construction). - Every `run:` step of `.github/workflows/test-public-repo-hygiene.yml` extracted and executed locally — all 13 smoke steps pass, exit 0. - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, 0 exempt. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (1 pre-existing CODEOWNERS warning). - `bash .github/bump-callers/tests/test_paths_contract.sh` — 70 passed, 0 failed. - Neighbouring Python suites re-run to catch cross-area breakage: agents-md-integrity 46, cursor-review 305, groom 365, refresh-reviewers 48, workflow-pins 272 — all OK. New tests (13): a private repo name in a path fails; an allowlisted repo name in a path passes; ticket-shaped directory component *and* filename prefix both fail; built-in and caller-extended acronyms in a path pass; the `(tracked path)` vs `:<lineno>` labels distinguish the two surfaces when a name appears in both; `aComfy-Org/x` does not flag (left-boundary pin); an internal-marker host in a path flags; the npm-scope and model-host suppressors carry over; the nested-model-host over-flag is pinned; `exclude_paths:` suppresses and still counts; a path finding fires on a binary entry and on a dangling symlink whose body is never read; and the zero-coverage output case above. ## Residual - **`.github/lint/README.md` KNOWN LIMITATION 8 is not updated.** That file does not exist on `main` — it arrives with PR #224, which is still **open**. Its limitation-8 sentence "neither checker scans the tracked path itself" becomes stale the moment this merges, and #224's branch will need that sentence corrected on rebase. I could not edit it here without touching another open PR's branch, so it is called out rather than done. Whichever of the two lands second owns the fix. - **A model-host mirror path over-flags (root or nested, since review round 1).** `models/hf.co/Comfy-Org/<model>/config.json` is reported while the same mirror at the tree root is suppressed: `MODEL_HOST_PREFIX_RE`'s left anchor rejects a preceding `/`, and every segment of a path has one. This is the over-flag direction (a leak guard should be wrong that way), one `exclude_paths:` entry clears it, and the shape occurs zero times across the 11,415 paths measured — so it is pinned by a test and documented as a known limitation rather than narrowed. Narrowing it would mean loosening a *suppressor*, which fails open; that is a deliberate non-goal here and would want its own change with its own evidence. - **Repository history and refs remain unscanned**, per the 3,337-path sweep above. Scrubbing a name out of the tree does not scrub it out of the history that still carries it. Out of scope for a checker whose contract is `git ls-files`, but it is the honest remaining hole and is now stated in both READMEs. - **Caller repos are not bumped by this PR.** The new surface reaches them only when `bump-public-repo-hygiene-callers.yml` moves their pinned SHA. Nothing here does that, and no caller CI changes until it happens. - **Unexercised artifact:** the originating investigation's findings write-up is not reachable from this environment, so the 11,415-path figure was re-derived independently here rather than taken on trust — it reproduces exactly, which is the closest thing to exercising it that was available. ## Provenance - **Authored by:** agent-work loop - **Verified:** public-repo-hygiene unittest 184 passed / 0 failed (1 skipped) after review round 1 (178 → 184); check_workflow_pins.py OK (11 workflows); check_agents_md.py passed (1 pre-existing warning); local path-surface re-sweep over every public clone on the host, 0 findings on public repos - **Deviations:** `_line_findings` takes a precomputed `owner_span` rather than an `is_codeowners` flag plus a line number (same behaviour, no fake `lineno=1` on a path — rationale above); `_emit` output ordering changed so a finding is never swallowed by the zero-coverage verdict (exit codes unchanged); `.github/lint/README.md` left alone because PR #224 has not merged. Review round 1 (cursor-review panel, 9 threads): the two URL-syntax suppressors (model-host prefix, markdown link label) are now gated OFF the path surface via `url_suppressors=False` — a path has no authority, so they failed open there; a vendored mirror therefore over-flags at the root as well as nested. A surrogateescape code point (`Cs`) now continues a name in `_nonascii_tail`. Path findings are capped and `PARTIAL`-counted through `_path_findings`. Documented rather than changed: `Comfy-Org/ComfyUI.md` over-flags, lowercase ticket ids in paths miss, category-2 hosts need a following `/`, exit 2 may carry findings, and `exclude_paths:` costs contents coverage. --- <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:40 +00:00