From eccb933071473467220e678b5112264f5f6477cf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 22 Aug 2026 15:50:34 -0700 Subject: [PATCH 1/2] ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy Replaces this repo's own scripts/check_public_repo_hygiene.py with a thin caller for the reusable workflow in Comfy-Org/github-workflows, which now owns both the checker and the known-public allowlist. The reason is not deduplication, though that is real -- this repo and its sibling SDK carried the same policy in two languages, and a one-line allowlist fix had to be made twice. It is that the old job ran the checker out of the PR's OWN checkout: - uses: actions/checkout@... # the PR merge ref - run: Python scripts/check_public_repo_hygiene.py so a PR could widen the allowlist, or disable the scan outright, and go green. A guard a change can edit is not a guard against that change. The reusable loads the checker from the pinned workflows_ref commit instead, and the allowlist is deliberately NOT an input, so a PR here cannot reach either through this workflow's inputs. PARITY PROVEN before deleting anything, not after. Run against this tree, the in-tree checker reported clean and the shared one reported exactly one finding: 'TEAM-1234' in scripts/check_public_repo_hygiene.py's own doc comment -- the example ticket format, in the file this commit deletes. With that file gone the shared checker is clean too, so the verdicts agree on everything that survives. Note what the pin does and does not buy, per the reusable's own header: it guarantees the checker comes from the commit the uses: line resolved to. It cannot govern which uses: line runs, because a pull_request caller executes its workflow file from the PR head. The control for that is a branch-protection rule requiring non-author review of .github/workflows/, which is worth having here if this is ever made a required check. --- .github/workflows/ci.yml | 17 --- .github/workflows/public-repo-hygiene.yml | 29 ++++ scripts/check_public_repo_hygiene.py | 178 ---------------------- 3 files changed, 29 insertions(+), 195 deletions(-) create mode 100644 .github/workflows/public-repo-hygiene.yml delete mode 100644 scripts/check_public_repo_hygiene.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b0d0458..96946b9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,20 +93,3 @@ jobs: - name: Verify distribution metadata (publish dry run) run: twine check dist/* - # NEW -- regression guard for the internal-reference leak this repo already - # had once (see scripts/check_public_repo_hygiene.py for what it looks for - # and why). Public repo, so this stays a permanent gate, not a one-time fix. - public-repo-hygiene: - name: public-repo-hygiene - runs-on: ubuntu-latest - steps: - - name: Check out repository - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 - with: - python-version: '3.12' - - - name: Scan for internal-only references - run: python3 scripts/check_public_repo_hygiene.py diff --git a/.github/workflows/public-repo-hygiene.yml b/.github/workflows/public-repo-hygiene.yml new file mode 100644 index 0000000..906e41e --- /dev/null +++ b/.github/workflows/public-repo-hygiene.yml @@ -0,0 +1,29 @@ +# Description: Thin caller for the shared public-repo-hygiene reusable workflow +# in Comfy-Org/github-workflows, which owns the checker AND its known-public +# allowlist. This replaces the in-tree copy this repo used to carry. +# +# Why the checker is not in this repo any more: the old job ran +# `scripts/check_public_repo_hygiene.py` straight out of the PR's own checkout, +# so a PR could widen the allowlist — or disable the scan — and go green. The +# reusable loads the checker from the pinned `workflows_ref` commit instead, so +# a PR here cannot reach it through this workflow's inputs. The allowlist is +# deliberately NOT an input, for the same reason. +# +# The pin is kept fresh by bump-public-repo-hygiene-callers.yml once this repo is +# enrolled in the PUBLIC_REPO_HYGIENE_CALLERS roster. +name: Public Repo Hygiene + +on: + pull_request: + push: + branches: [main] + +jobs: + hygiene: + permissions: + contents: read + uses: Comfy-Org/github-workflows/.github/workflows/public-repo-hygiene.yml@aad06972e20732d6a9167552c5b3247c4f5bac58 # github-workflows main (aad0697) + with: + # REQUIRED, and must equal the `uses:` SHA above — the reusable asserts + # they match, which is what pins the checker to the reviewed commit. + workflows_ref: aad06972e20732d6a9167552c5b3247c4f5bac58 diff --git a/scripts/check_public_repo_hygiene.py b/scripts/check_public_repo_hygiene.py deleted file mode 100644 index 42eb47e..0000000 --- a/scripts/check_public_repo_hygiene.py +++ /dev/null @@ -1,178 +0,0 @@ -#!/usr/bin/env python3 -"""Fail CI if the tree contains markers that only make sense in an internal -(private) context — this repo is public. - -This is a lightweight regression guard, not a secrets scanner: it looks for -categories of internal-only references (ticket-style IDs, internal -collaboration-tool links, and repo names outside the known-public set), not -credentials. It intentionally uses small, explicit allow/deny lists instead -of a single clever regex, so a false positive is a one-line list edit instead -of a mystery. - -Run: python3 scripts/check_public_repo_hygiene.py -""" - -from __future__ import annotations - -import re -import subprocess -import sys -from pathlib import Path - -ROOT = Path(__file__).resolve().parent.parent - -# Files this script itself doesn't need to scan (its own source, lockfiles, -# and generated/vendored output that isn't hand-authored). -EXCLUDE_PATHS = { - Path("scripts/check_public_repo_hygiene.py"), -} -EXCLUDE_DIR_PREFIXES = ( - "src/comfy_low/models/", # generated from the vendored spec -) - -# --- Category 1: ticket-shaped identifiers (TEAM-1234) ------------------- -# Generic shape rather than a guessed list of real internal team keys, so we -# don't need to encode (and thus disclose) an internal naming scheme here. -# Catches false positives on common tech acronyms via an explicit allowlist -# below -- extend that list, not the regex, when a legitimate term trips it. -TICKET_RE = re.compile(r"\b[A-Z]{2,6}-\d{2,6}\b") -TICKET_ALLOWLIST = { - "UTF-8", - "ISO-8601", - "SHA-256", - "SHA-384", - "SHA-512", - "AES-128", - "AES-256", - "RFC-2119", - "RFC-7231", - "RFC-3339", - "OAUTH-2", - "IPV-4", - "IPV-6", - "X-25519", - "WIN-32", - "WIN-64", -} - -# --- Category 2: internal collaboration-tool links/markers ---------------- -INTERNAL_MARKER_RES = [ - re.compile(r"notion\.(so|site)/", re.IGNORECASE), - re.compile(r"slack\.com/(archives|client)/", re.IGNORECASE), - re.compile(r"\bapp\.slack\.com\b", re.IGNORECASE), - re.compile(r"docs\.google\.com/", re.IGNORECASE), - re.compile(r"drive\.google\.com/", re.IGNORECASE), - re.compile(r"app\.datadoghq\.com/", re.IGNORECASE), - re.compile(r"\bposthog\.com/project/", re.IGNORECASE), - re.compile(r"\blinear\.app/", re.IGNORECASE), - re.compile(r"\bincident-\d+\b", re.IGNORECASE), -] - -# --- Category 3: references to Comfy-Org repos outside the known-public set -# Default-deny: only these are known to be public. Anything else under -# `Comfy-Org/` gets flagged so a maintainer can either scrub it or add -# it here once confirmed public. (No private repo names are listed here on -# purpose -- the point of default-deny is that we never need to.) -PUBLIC_COMFY_ORG_REPOS = { - "comfy-api-proxy", - "comfy-cla", - "comfy-cli", - "comfy-cloud-mcp-server", - "Comfy-Desktop", - "comfy-python-sdk", - "comfy-swift-sdk", - "comfy-typescript-sdk", - "ComfyUI_frontend", - "ComfyUI", -} -# CODEOWNERS team handles (`@Comfy-Org/`) are inherently public on a -# public repo -- GitHub renders the CODEOWNERS owners to anyone who can see the -# repo, so listing them here is not a leak. These mirror the sibling repos' -# CODEOWNERS (e.g. comfy-api-proxy). An `@Comfy-Org/` handle NOT in this -# set is still flagged, so a genuinely-internal team reference surfaces. -PUBLIC_COMFY_ORG_TEAMS = { - "comfy-cloud-team", - "core-engine-team", -} -REPO_REF_RE = re.compile(r"Comfy-Org/([A-Za-z0-9_.-]+)") - - -def _tracked_files() -> list[Path]: - out = subprocess.run( - ["git", "ls-files"], cwd=ROOT, check=True, capture_output=True, text=True - ).stdout - return [Path(p) for p in out.splitlines() if p] - - -def _is_excluded(rel: Path) -> bool: - if rel in EXCLUDE_PATHS: - return True - return any(str(rel).startswith(prefix) for prefix in EXCLUDE_DIR_PREFIXES) - - -def _check_file(rel: Path) -> list[str]: - findings: list[str] = [] - abs_path = ROOT / rel - try: - text = abs_path.read_text(encoding="utf-8") - except (UnicodeDecodeError, OSError): - return findings # binary or unreadable; not in scope - - for lineno, line in enumerate(text.splitlines(), start=1): - for match in TICKET_RE.finditer(line): - if match.group(0).upper() not in TICKET_ALLOWLIST: - findings.append(f"{rel}:{lineno}: possible internal ticket ID: {match.group(0)!r}") - - for pattern in INTERNAL_MARKER_RES: - if pattern.search(line): - findings.append( - f"{rel}:{lineno}: internal collaboration-tool marker: {line.strip()!r}" - ) - - for match in REPO_REF_RE.finditer(line): - name = match.group(1) - # A leading `@` makes this a CODEOWNERS team handle, not a repo ref. - if match.start() > 0 and line[match.start() - 1] == "@": - if name not in PUBLIC_COMFY_ORG_TEAMS: - findings.append( - f"{rel}:{lineno}: reference to @Comfy-Org/{name}, a team not in the " - "known-public allowlist (scripts/check_public_repo_hygiene.py) -- " - "confirm it's public and add it, or remove the reference" - ) - continue - if name not in PUBLIC_COMFY_ORG_REPOS: - findings.append( - f"{rel}:{lineno}: reference to Comfy-Org/{name}, which is not in the " - "known-public allowlist (scripts/check_public_repo_hygiene.py) -- " - "confirm it's public and add it, or remove the reference" - ) - - return findings - - -def main() -> int: - all_findings: list[str] = [] - for rel in _tracked_files(): - if _is_excluded(rel): - continue - all_findings.extend(_check_file(rel)) - - if all_findings: - print( - "ERROR: possible internal-only references found in this public repo:\n", file=sys.stderr - ) - for finding in all_findings: - print(f" {finding}", file=sys.stderr) - print( - "\nIf this is a genuine false positive, extend the allowlist in " - "scripts/check_public_repo_hygiene.py with a comment explaining why.", - file=sys.stderr, - ) - return 1 - - print("OK: no internal-only references found") - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) From 1d17d959d370df510dd3207a2461328ab8a0df4a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 27 Aug 2026 12:14:06 -0700 Subject: [PATCH 2/2] docs: qualify what the workflows_ref pin guarantees, and name the new check context The AGENTS.md rewrite claimed the pin meant "a PR here cannot reach it through this workflow's inputs" and stopped there. True, but a reader takes it as "a PR cannot subvert the hygiene gate", which is a stronger claim than the pin supports: `workflows_ref` only has to equal the SHA the caller's own `uses:` line selected, and a `pull_request` run executes that workflow file from the PR head. A PR that rewrites both values runs a different reusable entirely and its equality check proves nothing. The pinned reusable says exactly this in its own header ("WHERE THAT GUARANTEE STOPS"); the doc that survives in this repo did not. So state the limit, then state the control that actually covers it here, because it is a branch-protection setting and not a file anyone can grep for: main requires an approving code-owner review and dismisses stale approvals on every push, and CODEOWNERS owning `*` is what pulls .github/workflows/ under that rule. Also note it is not enforced for administrators, and that CODEOWNERS alone would enforce nothing -- it names reviewers and blocks nothing until a rule requires their approval. That distinction is the whole failure mode: an adopter who reads "CODEOWNERS" as sufficient leaves the `uses:` line editable by the very PR it is meant to gate. Also record the status-check context the moved job now reports, `hygiene / public-repo-hygiene`, in both AGENTS.md and CONTRIBUTING.md. It changed because the job left ci.yml for its own workflow file, and the old single-segment name is not reproducible for a reusable-workflow call. --- AGENTS.md | 21 +++++++++++++++++++++ CONTRIBUTING.md | 3 ++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 852740d..35bf1df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,6 +108,27 @@ that let a PR widen the allowlist or disable the scan and still go green. The reusable loads the checker from a pinned `workflows_ref` commit instead, so a PR here cannot reach it through this workflow's inputs. +**What the pin does and does not buy.** `workflows_ref` binds the checker to the +commit the caller's own `uses:` line selected — the reusable asserts the two +match, and that is all it asserts. A `pull_request` run executes the workflow +file from the PR head, so a PR that rewrites *both* the `uses:` SHA and +`workflows_ref` (or replaces the caller job outright) still satisfies that +equality check while running a checker of its own choosing. That is true of +every reusable workflow on GitHub, not a quirk of this one. + +The control for it is out of band, and it is a branch-protection setting rather +than a file in this repo: `main` requires an approving **code-owner** review and +dismisses stale approvals on every new push, so a change under +`.github/workflows/` cannot land on its author's say-so. +[`.github/CODEOWNERS`](.github/CODEOWNERS) owns `*`, which is what puts the +workflow directory under that requirement — but CODEOWNERS *alone* enforces +nothing, it only names reviewers. The rule is not absolute either: it is not +enforced for administrators. Relax the branch-protection setting and the pin's +guarantee relaxes with it, silently. + +The job runs from its own workflow file, not `ci.yml`, so the status-check +context it reports is `hygiene / public-repo-hygiene`. + The checker scans every git-tracked file (except `src/comfy_low/models/`) and fails on three categories: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b17b079..a2aa703 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -59,7 +59,8 @@ CI runs two more jobs beyond the four above: - **`public-repo-hygiene`** — a thin caller into the shared reusable workflow in `Comfy-Org/github-workflows`, which scans for internal-only references. This is a public repo; the check is a permanent gate, not a one-time - cleanup. See AGENTS.md for details. + cleanup. It lives in its own workflow file rather than `ci.yml`, so it + reports as `hygiene / public-repo-hygiene`. See AGENTS.md for details. ## The codegen trap: `src/comfy_low/models/_generated.py`