diff --git a/.github/public-repo-hygiene/README.md b/.github/public-repo-hygiene/README.md index 25b9145..b7a27b7 100644 --- a/.github/public-repo-hygiene/README.md +++ b/.github/public-repo-hygiene/README.md @@ -2,7 +2,8 @@ The checker behind [`public-repo-hygiene.yml`](../workflows/public-repo-hygiene.yml): a lightweight regression guard that fails CI when a **public** repo's tracked files carry -internal-only references. Setup guide for consumers: +internal-only references — in their contents, in their tracked **paths**, or in a symlink's +target string. Setup guide for consumers: [`docs/callers/public-repo-hygiene.md`](../../docs/callers/public-repo-hygiene.md). It is **not** a secrets scanner. It looks for three categories of internal-only *reference*, not @@ -30,6 +31,24 @@ and no further, and what is *derived* from those bytes is capped too — `MAX_FI a category-2 finding copies the matched line and the scanned repo controls how long that is. Hitting a cap adds a `::warning::` and never softens the verdict: the run still fails. +**Three surfaces, one matcher.** A tracked file publishes *three* strings, not one: its contents, +its symlink target string if it is a link, and its **path**. A tree containing +`docs/Comfy-Org//placeholder.md` names that repo to anyone who browses or clones +the repository even if every file in it is spotless, and until BE-9399 that tree passed clean +because only the first two were read. The path is now scanned as well, for **every** non-excluded +tracked entry — including the ones whose body the reader declines (binary, non-UTF-8, submodule +gitlink, FIFO, unreadable), since the path is published whatever the entry type. All three +surfaces go through the same `_line_findings`, so the same regexes, the same allowlists and the +same caller-side `ticket_allowlist:` / `exclude_paths:` knobs reach all of them; a second matcher +would be a second place to forget an allowlist entry. Paths come from `git ls-files -z` as +`/`-separated posix strings, and the token rules are the ones the contents get: `/` is not in the +repo-reference lookbehind, so `docs/Comfy-Org/x/y.md` matches while `aComfy-Org/x` does not, and +`\b` fires at both `/` and `-`, so `notes/TEAM-1234/plan.md` and `TEAM-1234-notes.md` both match. +A path finding is labelled ` (tracked path):` rather than `::`, because +"rename the file" and "edit the file" are different fixes. It does **not** make an entry count as +`SCANNED:` — that number is files read as *text*, and a path finding proves nothing about the +bytes inside. + Everything the scan declines to look at leaves a trace, because a guard that silently skips something is worse than no guard — the green run reads as coverage: @@ -226,8 +245,46 @@ category 3 host-aware, which removed the `huggingface.co/Comfy-Org/` fals because a hyphen is a non-word character). Recovering either costs an over-flag on a shape that really *is* a different registrable name (`app.slack.com-evil.com` → `com-evil.com`), so both are kept and pinned by `test_a_label_character_adjacent_to_the_host_is_a_known_miss`. -- **Only file *contents* are scanned, never file *paths*.** A `docs/-migration.md` or a - `notion-exports/` directory passes clean. +- **The two URL-syntax suppressors are switched off on the path surface, so a vendored + model mirror over-flags on its path.** `MODEL_HOST_PREFIX_RE` (a model-host authority in front + of the name) and `_labels_non_github_link` (a markdown link whose label names the same model + repo) read URL / markdown *syntax*, and a tracked path has neither: `hf.co/Comfy-Org//` is a + directory that happens to be called `hf.co`, not a different namespace, and inheriting the + suppressor would let a tree park a private name under such a directory and stay green. So + `hf.co/Comfy-Org//config.json` is reported on its path wherever it sits in the tree — + over-flagging is the safe direction for a leak guard. The allowlists, the ticket knobs, the + npm-scope crossing and the homoglyph handling are not syntax and reach both surfaces unchanged. + The shape did not occur once across the 11,415 tracked paths of nine Comfy-Org public repos; + pinned by `test_a_model_host_mirror_path_over_flags_wherever_it_sits`. +- **`exclude_paths:` is the caller-side remedy for a path over-flag, and it is not free.** An + excluded subtree is not scanned as a path *or* for its **contents** — excluding `models/` to + clear a vendored mirror stops every file beneath it being read. Prefer renaming, or the + narrowest entry that clears the finding (an exact file path rather than its directory). +- **An allowlisted repo name carrying a file extension over-flags.** `REPO_REF_RE`'s name class + admits `.` and only `.git` and a trailing period are stripped, so `docs/Comfy-Org/ComfyUI.md` + reads as the repo `ComfyUI.md`. Stripping an arbitrary extension would clear + `Comfy-Org/ComfyUI.internal` on the strength of its prefix (a repo name may legally carry a + dot), which is the fail-open direction, so it is not done; the directory form + `docs/Comfy-Org/ComfyUI/x.md` is clean. Pinned by + `test_an_allowlisted_name_with_a_file_extension_over_flags`. +- **Lowercase ticket ids in a path are a known miss.** `TICKET_RE` is case-sensitive on every + surface (see above), so kebab-cased `notes/be-1234/plan.md` passes while `notes/BE-1234/plan.md` + fires. Folding case for paths would turn `sha-256`, `iso-8601`, `rfc-2119` and every other + `-` component into a finding. Pinned by + `test_a_lowercase_ticket_id_in_a_path_is_a_known_miss`. +- **Most category-2 host patterns need a `/` after the host**, which a URL always has and a + path's *last* component never does: `docs/notion.so/x.md` fires (the host is a directory) + while `docs/notion.so.md`, `docs/app.slack.com-notes.md` and a `notion-exports/` directory + pass — the last because it is not a host at all. The path surface catches a host used as a + directory name, not a host-like fragment inside a file name. +- **Exit 2 can now carry findings.** The path is scanned even for entries whose body is never + read, so a repo of nothing but binaries can list path findings while `SCANNED:` is 0. Exit 2 + wins (the run proves nothing about the *contents*) and the findings are printed above the + verdict; read 2 as "not a pass", never as "no findings". +- **Paths and contents are scanned; the repository's *history* and its refs are not.** A path that + once existed and was renamed or deleted stays in every clone, as do branch and tag names, and + none of those is a tracked file. Scrubbing a name out of the tree does not scrub it out of the + history that still carries it. - **Git-LFS content, submodule contents and the far side of a symlink are not scanned** — each is counted under `NOT SCANNED:` (or, for a symlink, scanned as the link target *string* git actually stores) rather than silently treated as covered. See the coverage rules below. diff --git a/.github/public-repo-hygiene/check_public_repo_hygiene.py b/.github/public-repo-hygiene/check_public_repo_hygiene.py index 5b8c0ff..683a70f 100644 --- a/.github/public-repo-hygiene/check_public_repo_hygiene.py +++ b/.github/public-repo-hygiene/check_public_repo_hygiene.py @@ -8,6 +8,16 @@ of one clever regex, so a false positive is a one-line list edit instead of a mystery. +THREE SURFACES, ONE MATCHER (BE-9399). A tracked entry publishes more than its +bytes: its CONTENTS, its symlink TARGET STRING if it is a link, and its tracked +PATH are all visible to anyone who browses or clones a public repo. A tree +holding `docs/Comfy-Org//placeholder.md` names that repo even +if every file in it is spotless, so the path is scanned too — for every +non-excluded entry, including the ones whose body the reader declines (binary, +gitlink, FIFO, unreadable). All three go through `_line_findings`, never a +forked matcher, so the allowlists and the caller-tunable knobs apply +identically to each. + WHY IT LIVES HERE (BE-8654). It started as two copies — a Python one in the Python SDK and a JavaScript one in the TypeScript SDK — each run from the PR's OWN checkout. That had two defects. (1) A PR could weaken or delete the checker @@ -37,7 +47,12 @@ `working-tree-encoding` gitattribute that makes the work tree differ from what git stores, or a scan that ended up reading zero files). Exit 2 is never "clean" -- a guard that looked at nothing has to be as loud as one that found -something. +something. Since BE-9399 the two can co-occur: the tracked PATH is scanned even +for entries whose body is never read, so a zero-coverage run may still list +findings. Exit 2 wins there (the run still proves nothing about the CONTENTS), +and the findings are printed above the verdict rather than swallowed -- a +wrapper keying on the exit code alone should read 2 as "not a pass", never as +"no findings". Run locally: python3 .github/public-repo-hygiene/check_public_repo_hygiene.py --root . @@ -500,8 +515,15 @@ # numbers (N*), combining marks (M*), and the dash/connector punctuation that # supplies the homoglyphs -- U+2010 HYPHEN renders identically to `-` on # github.com. Quote and bracket categories are deliberately absent, so ordinary -# prose like `Comfy-Org/ComfyUI’s frontend` stays a clean reference. -_NAME_CONTINUING_CATEGORIES = frozenset({"Pd", "Pc"}) +# prose like `Comfy-Org/ComfyUI’s frontend` stays a clean reference. `Cs` +# (a lone surrogate) is here too: `tracked_files` decodes paths with +# `surrogateescape`, so a byte that is not valid UTF-8 inside a path component +# arrives as one, and it can only be sitting INSIDE the name -- treating it as +# a boundary would read `Comfy-Org/ComfyUI\xff-private` as the bare allowlisted +# `ComfyUI` and clear it, the prefix-vs-full-name hole `_nonascii_tail` exists +# to close. File CONTENTS never carry one (non-UTF-8 bodies are declined), so +# this only ever fires on the path surface. (BE-9399 review.) +_NAME_CONTINUING_CATEGORIES = frozenset({"Pd", "Pc", "Cs"}) # --- The model-host false positive's SECOND shape: a markdown link LABEL. # `MODEL_HOST_PREFIX_RE` above clears a reference the host sits in front of. @@ -1426,6 +1448,237 @@ def _codeowners_lines(text): ] +def _line_findings( + location, line, ticket_allowlist, owner_span, *, url_suppressors=True +): + """Yield LINE's findings, each prefixed with LOCATION. + + Shared by BOTH scanned surfaces rather than forked, so they can never drift + apart: a file's CONTENTS (`_file_findings`, LOCATION `:`) and + the tracked PATH string itself (`run_checks`, LOCATION + ` (tracked path)`). A public tree publishes + `docs/Comfy-Org//notes.md` exactly as loudly as it + publishes that name inside a file, so the same regexes, the same + allowlists and the same suppressors have to reach both -- a second matcher + would be a second place to forget an allowlist entry, and the + caller-tunable knobs would reach only one of them. (BE-9399.) + + OWNER_SPAN is `_codeowners_owner_span`'s answer for this line, or None when + the line has no CODEOWNERS owner fields -- which a tracked PATH always is, + even the path OF a CODEOWNERS file, because a path string is not an owner + line. Passed in rather than derived here because deriving it needs both the + file's CODEOWNERS-ness and the line NUMBER (a UTF-8 BOM is decoding residue + only at offset 0 of the file), neither of which means anything on a path. + + URL_SUPPRESSORS is False on the path surface. Two of the repo category's + false-positive suppressors read URL / markdown SYNTAX -- a model-host + authority in front of the name (`MODEL_HOST_PREFIX_RE`) and a markdown link + whose label names the same model repo (`_labels_non_github_link`) -- and + that syntax has no meaning in a tracked path: there is no authority in a + path, so `hf.co/Comfy-Org//x` is a directory that happens to be + called `hf.co`, not a different namespace. Inheriting them wholesale + would let a tree park a private name under an `hf.co/` directory and stay + green, which is the leak this surface was added to catch. The allowlists, + the ticket knobs, the npm-scope crossing and the homoglyph handling are + NOT syntax and reach both surfaces unchanged. (BE-9399 review.) + """ + for match in TICKET_RE.finditer(line): + token = match.group(0).upper() + if token in ticket_allowlist: + continue + # A PUBLIC identifier namespace clears by prefix, not by exact + # token (see TICKET_ALLOWED_PREFIXES): `CVE-2021-44228` presents + # here as `CVE-2021`, and the year makes an exact carve-out expire. + if token.split("-", 1)[0] in TICKET_ALLOWED_PREFIXES: + continue + yield ( + f"{location}: possible internal ticket ID: " + f"{match.group(0)!r}" + ) + + for pattern in INTERNAL_MARKER_RES: + if pattern.search(line): + yield ( + f"{location}: internal collaboration-tool marker: " + f"{_excerpt(line)!r}" + ) + # Offsets where a model-host URL prefix ENDS are exactly the offsets a + # `Comfy-Org/` match may START at and not be a github.com reference. + # Computed at most once per LINE for the same reason as `owner_span`: + # the alternative is searching backwards from every match, which is + # O(line) per match and so quadratic on the one `MAX_FILE_BYTES` line + # this has to survive. Deferred until a first `Comfy-Org/` match exists, + # because it is the one derived structure on this path with no cap -- + # on the 5 MiB single line `MAX_FILE_BYTES` deliberately admits, a line + # of repeated `hf.co/` yields ~870k end offsets and a set of ints that + # large is tens of MB of runner memory. Almost no line has a match, so + # the common case now allocates nothing at all; when one does, the scan + # still runs exactly once. + model_host_ends = None if url_suppressors else frozenset() + for match in REPO_REF_RE.finditer(line): + if model_host_ends is None: + model_host_ends = frozenset( + m.end() for m in MODEL_HOST_PREFIX_RE.finditer(line) + ) + # See `MODEL_HOST_PREFIX_RE`: a different namespace, not a leak. + # Checked before the homoglyph branch below, because that branch's + # remedy ("rewrite the name in ASCII") is wrong advice for a model + # repo whose name is not ours to rewrite. + if match.start() in model_host_ends: + continue + name = match.group(1) + # Characters the ASCII name class could not read are the REST of the + # name, not a boundary. Carry them into what is reported, and (below) + # never clear a name they appear in. (BE-8654 review.) + tail = _nonascii_tail(line, match.end()) + name += tail + # Strip a sentence-final period BEFORE the team/repo fork: a GitHub + # repo or team slug can never end in `.`, so a trailing one is + # always prose punctuation the `.`-permitting name class swallowed + # (BE-8697). It has to happen here rather than in the repo branch + # below, because the team branch needs it too -- `@Comfy-Org/ + # Comfy-Cloud-Team.` at the end of a sentence is the confirmed + # false positive that motivated this. `rstrip` also handles the + # ellipsis case, and a reference that is nothing BUT the period + # (`Comfy-Org/.`) names no repo at all, so it is not a finding. + name = name.rstrip(".") + if not name: + continue + at_prefixed = match.start() > 0 and line[match.start() - 1] == "@" + # The markdown-label shape (BE-8910). Gated on `not tail` because + # the comparison below is over what the ASCII class read, and on + # `not at_prefixed` because a team handle or an npm coordinate + # labelling a model link is not a spelling to clear -- both keep the + # skip to the shape the fixtures actually carry. + if ( + url_suppressors + and not tail + and not at_prefixed + and _labels_non_github_link( + line, match.start(), match.end(), name + ) + ): + continue + if tail: + # Never cleared, and never SILENTLY cleared either: casefold + # membership is untrustworthy in both directions over a name + # like this -- `comfy-typecript-sdk` folds ONTO an + # allowlisted name, and `comfyuiinternal` folds off the + # end of one. Reported with its own remedy, because "add it to + # the allowlist" is not the fix for a homoglyph. (BE-8654 + # review.) + yield ( + f"{location}: reference to " + f"{'@' if at_prefixed else ''}Comfy-Org/{_bounded(name)}, " + "whose name carries non-ASCII characters that can render " + "identically to ASCII ones on github.com (a homoglyph), so " + "it is NOT cleared against the known-public allowlist -- " + "rewrite the name in ASCII, remove the reference, or (if " + "the non-ASCII text is adjacent PROSE rather than part of " + "the name) put a separator between them" + ) + continue + # A leading `@` makes this a CODEOWNERS team handle, not a repo ref + # -- OR an npm / GitHub Packages scope, which is spelled exactly the + # same way and is required to be lowercase. Before BE-8697 made the + # org segment case-insensitive, the canonical `@comfy-org/` + # spelling in a `package.json` or lockfile did not match at all; + # now it does, so this branch has to admit BOTH readings or a + # dependency on a known-PUBLIC repo becomes "a team not in the + # known-public allowlist" with no caller-side escape (the repo + # allowlist is deliberately not a workflow input). + # + # EXCEPT on the OWNER FIELDS of a CODEOWNERS line, where only ONE + # of the two readings is possible: an `@`-prefixed owner is a + # handle and npm coordinates never appear there, so the repo + # crossing is denied and team-allowlist membership is required. + # WHICH files get this treatment is `_is_codeowners`: the three + # locations GitHub actually reads CODEOWNERS from (`rel` is a + # git-tracked path, `/`-separated whatever the host OS is), with + # the name matched case-insensitively. WHERE on a line it applies + # is `_codeowners_owner_span`, because a `#` comment and a scoped + # path pattern carry the same spelling without being handles. + # (BE-8857, + its review.) + # + # The reverse crossing stays forbidden on purpose -- a bare + # `Comfy-Org/` is unambiguously a repo path, since there is no + # syntax that writes a team without the `@`, so admitting team names + # there would weaken default-deny with no false positive to justify + # it. See test_team_allowlist_does_not_leak_into_repo_allowlist. + if at_prefixed: + folded = name.casefold() + # The crossing is NARROWED to a spelling that could actually BE + # an npm coordinate: those are required to be lowercase, so + # `@comfy-org/comfy-cli` crosses and the canonical GitHub team + # spelling `@Comfy-Org/comfy-cli` does not. Naming a team after + # the repo it owns is the commonest CODEOWNERS convention there + # is, so an unconditional crossing cleared exactly the likely + # collision -- a team handle that is not in the team allowlist, + # waved through because a public repo happens to share its name. + # (BE-8654 review.) That narrowing is not enough on its own: + # team slugs are lowercase BY CONSTRUCTION and GitHub resolves + # the org segment case-insensitively, so `@comfy-org/comfy-cli` + # in a CODEOWNERS file is a real, functional team handle that + # the lowercase test alone waved through. Hence the CODEOWNERS + # gate. Elsewhere the lowercase narrowing stands, because a + # lowercase mention in a README, a Dockerfile `npm i` line or a + # CI shell script genuinely could be an npm coordinate -- that + # residual ambiguity is ACCEPTED: in prose the two readings are + # indistinguishable, and re-denying there would re-open the + # false-positive class the crossing exists to fix (see + # test_npm_scope_of_a_public_repo_is_not_a_team_finding). + # (BE-8857.) + # Keyed on the OWNER FIELDS of a CODEOWNERS line, not on the + # file as a whole: a `#` comment and a scoped path pattern + # (`/packages/@comfy-org/comfy-cli/**`) legally carry the same + # spelling without being handles. (BE-8857 review.) + in_owner_field = owner_span is not None and ( + owner_span[0] <= match.start() < owner_span[1] + ) + npm_scope = ( + not in_owner_field + and line[match.start() : match.end()].islower() + ) + if folded not in _PUBLIC_TEAMS_CF and not ( + npm_scope and folded in _PUBLIC_REPOS_CF + ): + yield ( + f"{location}: reference to " + f"@Comfy-Org/{_bounded(name)}, a " + "team not in the known-public allowlist " + "(Comfy-Org/github-workflows " + ".github/public-repo-hygiene/" + "check_public_repo_hygiene.py) -- confirm it's public " + "and add it, or remove the reference" + ) + continue + # Strip a trailing `.git`: repository URLs (package.json + # `repository.url`, git remotes) conventionally end in `.git`, and + # `Foo.git` is still a reference to the public repo `Foo`. Matched + # case-insensitively like everything else on this path (BE-8697) -- + # a case-SENSITIVE strip here would leave `ComfyUI.GIT` carrying its + # suffix into a membership test that then misses `comfyui`. + repo = re.sub(r"\.git$", "", name, flags=re.IGNORECASE) + # `Comfy-Org/.git` reaches here with the whole name consumed: the + # period strip above left `.git` alone (no TRAILING dot) and the + # suffix strip took the rest. Like `Comfy-Org/.`, it names no repo, + # so there is nothing to report -- and reporting it would print the + # repo-less "reference to Comfy-Org/" the guard above exists to + # prevent. + if not repo: + continue + if repo.casefold() not in _PUBLIC_REPOS_CF: + yield ( + f"{location}: reference to " + f"Comfy-Org/{_bounded(repo)}, which is " + "not in the known-public allowlist " + "(Comfy-Org/github-workflows " + ".github/public-repo-hygiene/check_public_repo_hygiene.py)" + " -- confirm it's public and add it, or remove the " + "reference" + ) + + def _file_findings(rel, text, ticket_allowlist): """Yield one file's findings lazily, in report order. @@ -1442,207 +1695,48 @@ def _file_findings(rel, text, ticket_allowlist): # the long-standing behaviour the other two categories are pinned to. lines = _codeowners_lines(text) if is_codeowners else text.splitlines() for lineno, line in enumerate(lines, start=1): - for match in TICKET_RE.finditer(line): - token = match.group(0).upper() - if token in ticket_allowlist: - continue - # A PUBLIC identifier namespace clears by prefix, not by exact - # token (see TICKET_ALLOWED_PREFIXES): `CVE-2021-44228` presents - # here as `CVE-2021`, and the year makes an exact carve-out expire. - if token.split("-", 1)[0] in TICKET_ALLOWED_PREFIXES: - continue - yield ( - f"{rel}:{lineno}: possible internal ticket ID: " - f"{match.group(0)!r}" - ) - - for pattern in INTERNAL_MARKER_RES: - if pattern.search(line): - yield ( - f"{rel}:{lineno}: internal collaboration-tool marker: " - f"{_excerpt(line)!r}" - ) - # Per LINE, not per match: `_codeowners_owner_span` walks the line, so # recomputing it inside the match loop is quadratic on a single long # line -- the same shape `_excerpt` and `_bounded` guard against. owner_span = ( _codeowners_owner_span(line, lineno) if is_codeowners else None ) - # Offsets where a model-host URL prefix ENDS are exactly the offsets a - # `Comfy-Org/` match may START at and not be a github.com reference. - # Computed at most once per LINE for the same reason as `owner_span`: - # the alternative is searching backwards from every match, which is - # O(line) per match and so quadratic on the one `MAX_FILE_BYTES` line - # this has to survive. Deferred until a first `Comfy-Org/` match exists, - # because it is the one derived structure on this path with no cap -- - # on the 5 MiB single line `MAX_FILE_BYTES` deliberately admits, a line - # of repeated `hf.co/` yields ~870k end offsets and a set of ints that - # large is tens of MB of runner memory. Almost no line has a match, so - # the common case now allocates nothing at all; when one does, the scan - # still runs exactly once. - model_host_ends = None - for match in REPO_REF_RE.finditer(line): - if model_host_ends is None: - model_host_ends = frozenset( - m.end() for m in MODEL_HOST_PREFIX_RE.finditer(line) - ) - # See `MODEL_HOST_PREFIX_RE`: a different namespace, not a leak. - # Checked before the homoglyph branch below, because that branch's - # remedy ("rewrite the name in ASCII") is wrong advice for a model - # repo whose name is not ours to rewrite. - if match.start() in model_host_ends: - continue - name = match.group(1) - # Characters the ASCII name class could not read are the REST of the - # name, not a boundary. Carry them into what is reported, and (below) - # never clear a name they appear in. (BE-8654 review.) - tail = _nonascii_tail(line, match.end()) - name += tail - # Strip a sentence-final period BEFORE the team/repo fork: a GitHub - # repo or team slug can never end in `.`, so a trailing one is - # always prose punctuation the `.`-permitting name class swallowed - # (BE-8697). It has to happen here rather than in the repo branch - # below, because the team branch needs it too -- `@Comfy-Org/ - # Comfy-Cloud-Team.` at the end of a sentence is the confirmed - # false positive that motivated this. `rstrip` also handles the - # ellipsis case, and a reference that is nothing BUT the period - # (`Comfy-Org/.`) names no repo at all, so it is not a finding. - name = name.rstrip(".") - if not name: - continue - at_prefixed = match.start() > 0 and line[match.start() - 1] == "@" - # The markdown-label shape (BE-8910). Gated on `not tail` because - # the comparison below is over what the ASCII class read, and on - # `not at_prefixed` because a team handle or an npm coordinate - # labelling a model link is not a spelling to clear -- both keep the - # skip to the shape the fixtures actually carry. - if ( - not tail - and not at_prefixed - and _labels_non_github_link( - line, match.start(), match.end(), name - ) - ): - continue - if tail: - # Never cleared, and never SILENTLY cleared either: casefold - # membership is untrustworthy in both directions over a name - # like this -- `comfy-typecript-sdk` folds ONTO an - # allowlisted name, and `comfyuiinternal` folds off the - # end of one. Reported with its own remedy, because "add it to - # the allowlist" is not the fix for a homoglyph. (BE-8654 - # review.) - yield ( - f"{rel}:{lineno}: reference to " - f"{'@' if at_prefixed else ''}Comfy-Org/{_bounded(name)}, " - "whose name carries non-ASCII characters that can render " - "identically to ASCII ones on github.com (a homoglyph), so " - "it is NOT cleared against the known-public allowlist -- " - "rewrite the name in ASCII, remove the reference, or (if " - "the non-ASCII text is adjacent PROSE rather than part of " - "the name) put a separator between them" - ) - continue - # A leading `@` makes this a CODEOWNERS team handle, not a repo ref - # -- OR an npm / GitHub Packages scope, which is spelled exactly the - # same way and is required to be lowercase. Before BE-8697 made the - # org segment case-insensitive, the canonical `@comfy-org/` - # spelling in a `package.json` or lockfile did not match at all; - # now it does, so this branch has to admit BOTH readings or a - # dependency on a known-PUBLIC repo becomes "a team not in the - # known-public allowlist" with no caller-side escape (the repo - # allowlist is deliberately not a workflow input). - # - # EXCEPT on the OWNER FIELDS of a CODEOWNERS line, where only ONE - # of the two readings is possible: an `@`-prefixed owner is a - # handle and npm coordinates never appear there, so the repo - # crossing is denied and team-allowlist membership is required. - # WHICH files get this treatment is `_is_codeowners`: the three - # locations GitHub actually reads CODEOWNERS from (`rel` is a - # git-tracked path, `/`-separated whatever the host OS is), with - # the name matched case-insensitively. WHERE on a line it applies - # is `_codeowners_owner_span`, because a `#` comment and a scoped - # path pattern carry the same spelling without being handles. - # (BE-8857, + its review.) - # - # The reverse crossing stays forbidden on purpose -- a bare - # `Comfy-Org/` is unambiguously a repo path, since there is no - # syntax that writes a team without the `@`, so admitting team names - # there would weaken default-deny with no false positive to justify - # it. See test_team_allowlist_does_not_leak_into_repo_allowlist. - if at_prefixed: - folded = name.casefold() - # The crossing is NARROWED to a spelling that could actually BE - # an npm coordinate: those are required to be lowercase, so - # `@comfy-org/comfy-cli` crosses and the canonical GitHub team - # spelling `@Comfy-Org/comfy-cli` does not. Naming a team after - # the repo it owns is the commonest CODEOWNERS convention there - # is, so an unconditional crossing cleared exactly the likely - # collision -- a team handle that is not in the team allowlist, - # waved through because a public repo happens to share its name. - # (BE-8654 review.) That narrowing is not enough on its own: - # team slugs are lowercase BY CONSTRUCTION and GitHub resolves - # the org segment case-insensitively, so `@comfy-org/comfy-cli` - # in a CODEOWNERS file is a real, functional team handle that - # the lowercase test alone waved through. Hence the CODEOWNERS - # gate. Elsewhere the lowercase narrowing stands, because a - # lowercase mention in a README, a Dockerfile `npm i` line or a - # CI shell script genuinely could be an npm coordinate -- that - # residual ambiguity is ACCEPTED: in prose the two readings are - # indistinguishable, and re-denying there would re-open the - # false-positive class the crossing exists to fix (see - # test_npm_scope_of_a_public_repo_is_not_a_team_finding). - # (BE-8857.) - # Keyed on the OWNER FIELDS of a CODEOWNERS line, not on the - # file as a whole: a `#` comment and a scoped path pattern - # (`/packages/@comfy-org/comfy-cli/**`) legally carry the same - # spelling without being handles. (BE-8857 review.) - in_owner_field = owner_span is not None and ( - owner_span[0] <= match.start() < owner_span[1] - ) - npm_scope = ( - not in_owner_field - and line[match.start() : match.end()].islower() - ) - if folded not in _PUBLIC_TEAMS_CF and not ( - npm_scope and folded in _PUBLIC_REPOS_CF - ): - yield ( - f"{rel}:{lineno}: reference to " - f"@Comfy-Org/{_bounded(name)}, a " - "team not in the known-public allowlist " - "(Comfy-Org/github-workflows " - ".github/public-repo-hygiene/" - "check_public_repo_hygiene.py) -- confirm it's public " - "and add it, or remove the reference" - ) - continue - # Strip a trailing `.git`: repository URLs (package.json - # `repository.url`, git remotes) conventionally end in `.git`, and - # `Foo.git` is still a reference to the public repo `Foo`. Matched - # case-insensitively like everything else on this path (BE-8697) -- - # a case-SENSITIVE strip here would leave `ComfyUI.GIT` carrying its - # suffix into a membership test that then misses `comfyui`. - repo = re.sub(r"\.git$", "", name, flags=re.IGNORECASE) - # `Comfy-Org/.git` reaches here with the whole name consumed: the - # period strip above left `.git` alone (no TRAILING dot) and the - # suffix strip took the rest. Like `Comfy-Org/.`, it names no repo, - # so there is nothing to report -- and reporting it would print the - # repo-less "reference to Comfy-Org/" the guard above exists to - # prevent. - if not repo: - continue - if repo.casefold() not in _PUBLIC_REPOS_CF: - yield ( - f"{rel}:{lineno}: reference to " - f"Comfy-Org/{_bounded(repo)}, which is " - "not in the known-public allowlist " - "(Comfy-Org/github-workflows " - ".github/public-repo-hygiene/check_public_repo_hygiene.py)" - " -- confirm it's public and add it, or remove the " - "reference" - ) + yield from _line_findings( + f"{rel}:{lineno}", line, ticket_allowlist, owner_span + ) + + +def _path_findings(rel, ticket_allowlist): + """Return (findings, warnings, partial) for one tracked PATH string. + + The path-surface twin of `check_file`, with the same per-file findings cap + and the same `PARTIAL_FINDINGS` accounting: a path is scanned-repo + controlled too (git allows 4 KiB of `AA-12/` components), so a bare `list()` + here could both exceed the documented per-file cap and skip the `PARTIAL:` + line that says the enumeration was cut. (BE-9399 review.) + """ + findings = list( + itertools.islice( + _line_findings( + f"{rel} (tracked path)", + rel, + ticket_allowlist, + None, + url_suppressors=False, + ), + MAX_FINDINGS_PER_FILE + 1, + ) + ) + warnings, partial = [], [] + if len(findings) > MAX_FINDINGS_PER_FILE: + del findings[MAX_FINDINGS_PER_FILE:] + partial.append(PARTIAL_FINDINGS) + warnings.append( + f"its tracked path produced more than {MAX_FINDINGS_PER_FILE} " + f"findings; only the first {MAX_FINDINGS_PER_FILE} are listed. " + f"The path needs renaming wholesale -- the run still FAILS" + ) + return findings, warnings, partial def check_file(root, rel, ticket_allowlist): @@ -1743,10 +1837,45 @@ def run_checks(root, excludes=(), extra_ticket_allow=()): if hit is not None: counts[hit] += 1 continue - found, file_warnings, skip_kind, file_partial = check_file( - root, rel, ticket_allowlist + # The tracked PATH is a published string in its own right: a public + # tree's file listing shows `docs/Comfy-Org//x.md` to + # anyone, and before BE-9399 only the file's CONTENTS were read, so + # that tree passed clean. Scanned with the SAME `_line_findings` the + # contents get -- same regexes, same allowlists, same suppressors, and + # the same caller-side `--ticket-allow` / `--exclude` knobs (the + # `_is_excluded` `continue` above is what makes an `exclude_paths:` + # entry cover the path surface too, while still counting the file in + # the exclusion tally). + # + # Run for EVERY non-excluded entry, ahead of `check_file` and + # independent of it: the path is published whether or not the body is + # ever read, so an entry `check_file` declines (binary, submodule + # gitlink, FIFO, unreadable) still gets its path examined. It does not + # make such an entry count as `scanned` -- that number is files READ AS + # TEXT, and a path finding proves nothing about the bytes inside. + # + # `owner_span=None` unconditionally, even when the path IS a CODEOWNERS + # file: `_is_codeowners` decides how to read that file's LINES, and a + # path string is not an owner line. `url_suppressors=False` (inside + # `_path_findings`): a path has no URL authority and no markdown + # syntax, so the two suppressors that read those would fail OPEN here. + # + # `rel` comes from `git ls-files -z` decoded with `surrogateescape` + # (see `tracked_files`), so it is a `/`-separated posix string that may + # carry lone surrogates; `_nonascii_tail` treats one as part of the + # name (category `Cs` is in `_NAME_CONTINUING_CATEGORIES`), so a + # non-UTF-8 byte inside a name can never turn it into an allowlisted + # prefix of itself. + found, file_warnings, file_partial = _path_findings( + rel, ticket_allowlist + ) + content_found, content_warnings, skip_kind, content_partial = ( + check_file(root, rel, ticket_allowlist) ) - for kind in file_partial: + found += content_found + file_warnings += content_warnings + # A file counts ONCE per kind, whichever surface(s) earned it. + for kind in sorted(set(file_partial) | set(content_partial)): partial[kind] += 1 # Per-RUN cap on top of the per-file one, so a tree of many mid-sized # offenders cannot flood the log either. Scanning CONTINUES past it -- @@ -1859,6 +1988,29 @@ def _emit(result): print(f"WARN: {w}") print(f"::warning::public-repo-hygiene: {w}") + # Listed BEFORE the zero-coverage verdict below, because since BE-9399 the + # two can co-occur: the tracked PATH is scanned for every entry, including + # the ones whose body is never read, so a repo of nothing but binaries can + # leak in its own file listing while `scanned` stays 0. The exit code is + # still 2 there -- a run that read no text proves nothing about the + # contents -- but swallowing the one thing it DID find would send the + # operator hunting a configuration problem instead of the leak. + if result.findings: + print("\nERROR: possible internal-only references found in this public repo:\n") + for finding in result.findings: + escaped = _esc_cmd(finding) + print(f" {escaped}") + print(f"::error::public-repo-hygiene: {escaped}") + print( + "\nIf this is a genuine false positive, either add the acronym via " + "the workflow's `ticket_allowlist:` input, or -- for a Comfy-Org " + "repo you have CONFIRMED is public -- open a PR against " + "Comfy-Org/github-workflows adding it to PUBLIC_COMFY_ORG_REPOS in " + ".github/public-repo-hygiene/check_public_repo_hygiene.py. The repo " + "allowlist is org-wide and deliberately not editable from a caller " + "repo." + ) + if result.scanned == 0: # Green here would make the root-exclusion rejection one spelling away # from pointless: a caller that names every top-level directory in @@ -1873,20 +2025,6 @@ def _emit(result): print("\nResult: no internal-only references found.") return 0 - print("\nERROR: possible internal-only references found in this public repo:\n") - for finding in result.findings: - escaped = _esc_cmd(finding) - print(f" {escaped}") - print(f"::error::public-repo-hygiene: {escaped}") - print( - "\nIf this is a genuine false positive, either add the acronym via the " - "workflow's `ticket_allowlist:` input, or -- for a Comfy-Org repo you " - "have CONFIRMED is public -- open a PR against " - "Comfy-Org/github-workflows adding it to PUBLIC_COMFY_ORG_REPOS in " - ".github/public-repo-hygiene/check_public_repo_hygiene.py. The repo " - "allowlist is org-wide and deliberately not editable from a caller " - "repo." - ) print(f"\nResult: {len(result.findings)} internal-only reference(s) found.") return 1 diff --git a/.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py b/.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py index 8375de2..129dec8 100644 --- a/.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py +++ b/.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py @@ -16,6 +16,8 @@ import ast import codecs +import contextlib +import io import os import subprocess import sys @@ -1716,6 +1718,324 @@ def test_a_root_naming_a_file_is_a_config_error_not_a_traceback(self): self.assertIn("cannot run git in", str(caught.exception)) +class TrackedPathSurfaceTest(CheckerTestCase): + """The tracked PATH string is scanned, not just the file's contents. + + A public tree publishes its file listing: `docs/Comfy-Org//placeholder.md` names that repo to anyone who clones or browses the + repository, and until BE-9399 such a tree passed clean because only file + CONTENTS and symlink target strings were read. The path now goes through + the SAME `_line_findings` the contents do -- same regexes, same + allowlists, same suppressors, same caller-side knobs -- so the two surfaces + cannot drift apart. + + The symlink target string, the third surface, is pinned separately by + `test_symlink_target_string_is_scanned_but_never_followed`. + """ + + def test_private_repo_name_in_a_tracked_path_is_a_finding(self): + self.repo.write( + "docs/Comfy-Org/some-private-repo/placeholder.md", "clean\n" + ) + findings = self.findings() + self.assertEqual(len(findings), 1, findings) + self.assertIn("(tracked path)", findings[0]) + self.assertIn("some-private-repo", findings[0]) + + def test_the_location_label_distinguishes_the_two_surfaces(self): + # Same name in both places is TWO findings, and a reader has to be able + # to tell "rename the file" from "edit the file" apart. + self.repo.write( + "docs/Comfy-Org/some-private-repo/notes.md", + "See Comfy-Org/some-private-repo for context.\n", + ) + findings = self.findings() + self.assertEqual(len(findings), 2, findings) + path_finding = [f for f in findings if "(tracked path)" in f] + line_finding = [f for f in findings if "(tracked path)" not in f] + self.assertEqual(len(path_finding), 1, findings) + self.assertEqual(len(line_finding), 1, findings) + self.assertTrue( + path_finding[0].startswith( + "docs/Comfy-Org/some-private-repo/notes.md (tracked path): " + ), + path_finding[0], + ) + self.assertTrue( + line_finding[0].startswith( + "docs/Comfy-Org/some-private-repo/notes.md:1: " + ), + line_finding[0], + ) + + def test_an_allowlisted_repo_name_in_a_path_is_clean(self): + self.repo.write("docs/Comfy-Org/ComfyUI/x.md", "clean\n") + self.assertEqual(self.findings(), []) + + def test_a_ticket_shaped_path_component_is_a_finding(self): + # `TICKET_RE`'s `\b` fires at `/` and at `-`, so a directory component + # and a filename prefix both match -- the same token rules the content + # scan uses, no path-specific boundary. + self.repo.write("notes/BE-1234/plan.md", "clean\n") + self.repo.write("BE-5678-notes.md", "clean\n") + findings = sorted(self.findings()) + self.assertEqual(len(findings), 2, findings) + self.assertTrue(all("(tracked path)" in f for f in findings), findings) + self.assertIn("BE-5678", findings[0]) + self.assertIn("BE-1234", findings[1]) + + def test_allowlisted_acronyms_in_a_path_are_clean(self): + # Built-in allowlist and the caller-side `--ticket-allow` both reach + # the path surface, because there is only one matcher to reach. + self.repo.write("src/UTF-8/decode.py", "clean\n") + self.repo.write("src/GPU-100/kernel.py", "clean\n") + self.assertEqual( + self.findings(extra_ticket_allow=["GPU-100"]), [] + ) + + def test_a_name_glued_to_the_org_segment_is_not_a_path_finding(self): + # `REPO_REF_RE`'s left lookbehind is `[A-Za-z0-9_]`, which does NOT + # include `/` -- that is what lets a path component match at all. The + # pin is the other half: a letter immediately before `Comfy-Org` is a + # different name, on a path exactly as in prose. + self.repo.write("aComfy-Org/x.md", "clean\n") + self.repo.write("docs/aComfy-Org/y.md", "clean\n") + self.assertEqual(self.findings(), []) + + def test_an_internal_marker_in_a_path_is_a_finding(self): + self.repo.write("docs/notion.so/exported-page.md", "clean\n") + findings = self.findings() + self.assertEqual(len(findings), 1, findings) + self.assertIn("(tracked path)", findings[0]) + self.assertIn("internal collaboration-tool marker", findings[0]) + + def test_the_path_surface_inherits_the_non_syntax_suppressors(self): + # The allowlists, the ticket knobs and the npm/GitHub Packages scope + # crossing are not URL syntax, so they reach the path surface for the + # same reason the allowlists do: one matcher, not two. + self.repo.write( + "packages/@comfy-org/comfy-cli/package.json", "{}\n" + ) + self.assertEqual(self.findings(), []) + + def test_the_url_syntax_suppressors_do_not_reach_the_path_surface(self): + # `MODEL_HOST_PREFIX_RE` and `_labels_non_github_link` read URL / + # markdown SYNTAX -- an authority in front of the name, a link whose + # label names the same model repo. A tracked path has neither: there + # is no authority in a path, so `hf.co/Comfy-Org//` is a directory + # that happens to be called `hf.co`, not a different namespace, and + # inheriting the suppressor would let a tree park a private name + # under it and stay green (BE-9399 review). Both are gated off via + # `url_suppressors=False`; the same shapes in CONTENTS stay cleared. + self.repo.write( + "hf.co/Comfy-Org/some-private-repo/config.json", "{}\n" + ) + self.repo.write( + "[Comfy-Org/some-private-repo](https:/huggingface.co/" + "Comfy-Org/some-private-repo)/x.md", + "clean\n", + ) + findings = self.findings() + # Three, not two: the markdown-shaped path names the repo TWICE (label + # and destination) and neither copy is cleared on this surface. + self.assertEqual(len(findings), 3, findings) + self.assertTrue(all("(tracked path)" in f for f in findings), findings) + self.assertTrue( + all("some-private-repo" in f for f in findings), findings + ) + # Contents: the same two shapes are still suppressed (existing + # behaviour, re-pinned here so a later change to the gate cannot + # flip it the other way). + self.repo.write( + "clean/README.md", + "https://hf.co/Comfy-Org/some-model\n" + "[Comfy-Org/some-model](https://huggingface.co/Comfy-Org/some-model)\n", + ) + self.assertEqual( + [f for f in self.findings() if "README.md:" in f], [] + ) + + def test_a_model_host_mirror_path_over_flags_wherever_it_sits(self): + # KNOWN, ACCEPTED over-flag: a vendored model mirror is reported on + # its path at the tree root and under a directory alike, because the + # model-host suppressor is URL syntax and is switched off on the path + # surface (see the test above). Over-flagging is the safe direction + # for a leak guard; a repo that really vendors such a tree clears it + # with one `exclude_paths:` entry -- at the cost of that subtree's + # CONTENTS no longer being scanned either, which the docs now say. + # Measured across the 11,415 tracked paths of nine Comfy-Org public + # repos: zero occurrences of either shape. + self.repo.write("hf.co/Comfy-Org/some-model/config.json", "{}\n") + self.repo.write( + "models/hf.co/Comfy-Org/some-model/config.json", "{}\n" + ) + findings = self.findings() + self.assertEqual(len(findings), 2, findings) + self.assertTrue(all("(tracked path)" in f for f in findings), findings) + self.assertTrue( + all("Comfy-Org/some-model" in f for f in findings), findings + ) + self.assertEqual( + self.findings(excludes=["models/", "hf.co/"]), [] + ) + + def test_an_allowlisted_name_with_a_file_extension_over_flags(self): + # KNOWN, ACCEPTED over-flag, pinned so it is a decision and not an + # accident: `REPO_REF_RE`'s name class admits `.`, and only `.git` and + # a trailing period are stripped, so `docs/Comfy-Org/ComfyUI.md` + # reads as the repo `ComfyUI.md`, which is not allowlisted. Stripping + # an arbitrary extension would clear `Comfy-Org/ComfyUI.internal` on + # the strength of its prefix -- a private repo may legally carry a + # dot -- which is the fail-open direction. Rename the file or use + # `exclude_paths:`; the directory form `docs/Comfy-Org/ComfyUI/x.md` + # is clean (see test_an_allowlisted_repo_name_in_a_path_is_clean). + self.repo.write("docs/Comfy-Org/ComfyUI.md", "clean\n") + findings = self.findings() + self.assertEqual(len(findings), 1, findings) + self.assertIn("(tracked path)", findings[0]) + self.assertIn("Comfy-Org/ComfyUI.md", findings[0]) + + def test_a_lowercase_ticket_id_in_a_path_is_a_known_miss(self): + # `TICKET_RE` is case-sensitive on every surface: folding it for paths + # would turn `sha-256`, `iso-8601`, `rfc-2119` and every kebab-cased + # `-` component into a finding. Documented as a known + # miss rather than folded (BE-9399 review). + self.repo.write("notes/be-1234/plan.md", "clean\n") + self.assertEqual(self.findings(), []) + + def test_a_surrogate_inside_a_path_name_is_never_a_boundary(self): + # `tracked_files` decodes with `surrogateescape`, so a non-UTF-8 byte + # inside a path component arrives as a lone surrogate (category `Cs`). + # It has to READ AS PART OF THE NAME: as a boundary, + # `Comfy-Org/ComfyUI\xff-private` would be the bare allowlisted + # `ComfyUI` and clear -- the prefix-vs-full-name hole `_nonascii_tail` + # exists to close, reopened on the one surface that can carry such + # input (non-UTF-8 CONTENTS are declined outright). Driven through the + # helper directly: macOS refuses to create a file with an invalid-UTF-8 + # name, so the tree cannot be built portably. + rel = "docs/Comfy-Org/ComfyUI\udcff-private/x.md" + findings, _, _ = checker._path_findings(rel, checker.TICKET_ALLOWLIST) + self.assertEqual(len(findings), 1, findings) + self.assertIn("homoglyph", findings[0]) + self.assertIn("(tracked path)", findings[0]) + self.assertEqual( + checker._nonascii_tail("ComfyUI\udcff-private", 7), + "\udcff-private", + ) + # ...and the finding survives printing: `_esc_cmd` round-trips it. + self.assertIn("\\udcff", checker._esc_cmd(findings[0])) + + def test_path_findings_are_capped_and_counted_as_partial(self): + # A path is scanned-repo controlled like a line is (git allows ~4 KiB + # of `AA-12/` components), so it gets the same per-file cap and the + # same `PARTIAL_FINDINGS` accounting as the contents -- a bare + # `list()` would exceed the documented cap with no `PARTIAL:` line. + # Driven through the helper: the path is longer than PATH_MAX, so the + # tree cannot be built. + cap = checker.MAX_FINDINGS_PER_FILE + rel = "/".join(f"AA-{10 + i}" for i in range(cap + 5)) + "/x.md" + findings, warnings, partial = checker._path_findings( + rel, checker.TICKET_ALLOWLIST + ) + self.assertEqual(len(findings), cap, len(findings)) + self.assertEqual(partial, [checker.PARTIAL_FINDINGS]) + self.assertEqual(len(warnings), 1, warnings) + self.assertIn("tracked path", warnings[0]) + # Exactly AT the cap is not partial. + rel = "/".join(f"AA-{10 + i}" for i in range(cap)) + "/x.md" + findings, warnings, partial = checker._path_findings( + rel, checker.TICKET_ALLOWLIST + ) + self.assertEqual(len(findings), cap) + self.assertEqual((warnings, partial), ([], [])) + + def test_a_partial_path_file_is_counted_once_in_the_run(self): + # Both surfaces of one file can earn PARTIAL_FINDINGS; the run counts + # the FILE once per kind, not once per surface. + cap = checker.MAX_FINDINGS_PER_FILE + rel = "/".join(f"AA-{10 + i}" for i in range(cap + 5)) + "/x.md" + capped = ( + [f"{rel} (tracked path): finding"] * cap, + [f"{rel}: its tracked path produced too many"], + [checker.PARTIAL_FINDINGS], + ) + content = ( + [f"{rel}:1: finding"] * cap, + ["produced more than"], + None, + [checker.PARTIAL_FINDINGS], + ) + self.repo.write("ok.md", "clean\n") + with unittest.mock.patch.object( + checker, "tracked_files", return_value=["ok.md", rel] + ), unittest.mock.patch.object( + checker, "_work_tree_encoded", return_value=[] + ), unittest.mock.patch.object( + checker, + "_path_findings", + side_effect=lambda r, a: capped if r == rel else ([], [], []), + ), unittest.mock.patch.object( + checker, + "check_file", + side_effect=lambda root, r, a: ( + content if r == rel else ([], [], None, []) + ), + ): + result = checker.run_checks(self.repo.root) + self.assertEqual(result.partial, [(checker.PARTIAL_FINDINGS, 1)]) + self.assertEqual(len(result.findings), 2 * cap) + self.assertEqual(result.scanned, 2) + + def test_excluding_the_leaky_path_suppresses_it_and_still_counts(self): + # `exclude_paths:` is the caller's escape hatch for a false positive, + # and it has to cover the path surface too -- otherwise a repo that + # excluded a vendored tree would be reddened by that tree's own name + # with no way to clear it. The exclusion still reports its count, so + # the hole stays named in the log. + self.repo.write( + "vendor/Comfy-Org/some-private-repo/placeholder.md", "clean\n" + ) + self.repo.write("ok.md", "clean\n") + result = self.run_checks(excludes=["vendor/"]) + self.assertEqual(result.findings, []) + self.assertEqual(result.exclusions, [("vendor/", 1)]) + self.assertEqual(result.scanned, 1) + + def test_a_path_finding_fires_on_an_entry_whose_body_is_skipped(self): + # The path is published in the tree whether or not the bytes inside are + # ever read, so the path scan is independent of `check_file` -- it runs + # for a binary blob, and for every other entry the reader declines. + self.repo.write( + "assets/Comfy-Org/some-private-repo/logo.bin", + b"\x00\xff\xfe", + track=True, + ) + self.repo.write("ok.md", "clean\n") + result = self.run_checks() + self.assertEqual(len(result.findings), 1, result.findings) + self.assertIn("(tracked path)", result.findings[0]) + self.assertIn("some-private-repo", result.findings[0]) + # ...and it does NOT make the unread blob count as scanned: that number + # is files read as TEXT, and a path finding proves nothing about the + # bytes inside. + self.assertEqual(result.skipped, [("binary", 1)]) + self.assertEqual(result.scanned, 1) + + def test_a_path_finding_fires_on_a_dangling_symlink_entry(self): + # The other body-less entry shape, and the one that already had its + # target string scanned -- so this pins that the PATH is scanned as + # well as the target, not instead of it. + os.symlink( + "../nowhere", os.path.join(self.repo.root, "BE-4242.link") + ) + self.repo._git("add", "--", "BE-4242.link") + self.repo.write("ok.md", "clean\n") + findings = self.run_checks().findings + self.assertEqual(len(findings), 1, findings) + self.assertIn("(tracked path)", findings[0]) + self.assertIn("BE-4242", findings[0]) + + class CoverageReportingTest(CheckerTestCase): """Nothing the checker skips may be invisible in the log (BE-8654). @@ -2609,6 +2929,28 @@ def test_a_scan_that_read_nothing_exits_2_not_0(self): checker.main(["--root", self.repo.root, "--exclude", "docs/"]), 2 ) + def test_a_path_finding_is_listed_even_when_nothing_was_scanned(self): + # The two can co-occur since the path surface was added: a repo of + # nothing but binaries leaks in its own file listing while `scanned` + # stays 0. The exit code is still 2 -- a run that read no text proves + # nothing about the contents -- but the finding has to be PRINTED, or + # the operator reads "nothing was scanned" and goes hunting for a + # configuration problem instead of the leak. + self.repo.write( + "assets/Comfy-Org/some-private-repo/logo.bin", + b"\x00\xff\xfe", + track=True, + ) + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + code = checker.main(["--root", self.repo.root]) + out = buf.getvalue() + self.assertEqual(code, 2, out) + self.assertIn("some-private-repo", out) + self.assertIn("(tracked path)", out) + self.assertIn("SCANNED: 0 file(s)", out) + self.assertIn("nothing was scanned", out) + def test_multi_value_inputs_are_split(self): self.assertEqual( checker._split_values(["a/, b/\nc/", "", " ", "d/"]), diff --git a/.github/workflows/public-repo-hygiene.yml b/.github/workflows/public-repo-hygiene.yml index a50a362..298cdcb 100644 --- a/.github/workflows/public-repo-hygiene.yml +++ b/.github/workflows/public-repo-hygiene.yml @@ -8,8 +8,9 @@ name: Public Repo Hygiene (reusable) # `@Comfy-Org/` CODEOWNERS-handle case). It is a lightweight regression # guard, not a secrets scanner. It runs in the CALLER's context, so # `actions/checkout` checks out the calling repo and the scan operates on that -# repo's tracked files. Fails with a non-zero exit (and GitHub annotations) so -# it can be wired as a required status check. +# repo's tracked files — their CONTENTS, their tracked PATHS, and a symlink's +# target string, all three through the same matcher. Fails with a non-zero exit +# (and GitHub annotations) so it can be wired as a required status check. # # The check script is loaded from THIS repo (public, pinned via `workflows_ref`) # — never from the caller's checkout — so a PR can't rewrite the checker OR its diff --git a/README.md b/README.md index 24c0866..f48dc41 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ complete, copy-pasteable caller. | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | -| [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `UTF-8`/`SHA-256` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`); binaries are skipped; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | +| [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `UTF-8`/`SHA-256` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`), and for each one the scan covers its CONTENTS, its tracked PATH and (for a symlink) its target string — the same matcher and allowlists on all three, so a private repo name in a directory component is a finding just as it is in a line of prose; binaries are skipped for their contents but still have their path scanned; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | | [`coderabbit-config-validate.yml`](.github/workflows/coderabbit-config-validate.yml) | Validates the caller repo's `.coderabbit.yaml` against CodeRabbit's config schema, **on the PR that breaks it**. CodeRabbit rejects an invalid config *whole* — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter and path instruction in it goes silently inert — and it validates the **base** branch, not the PR head, so the breakage first surfaces on the *next* PR, blamed on a change that did not cause it. Severity mirrors CodeRabbit's own behaviour: a YAML parse error, a `maxLength` violation or a type/enum error **fails**; an unknown/additional property **warns** (CodeRabbit strips those rather than rejecting the file, so the config loads while everything under the key silently does nothing) unless `strict_unknown_keys: true`. A repo with no `.coderabbit.yaml` passes, and says so. The schema is **vendored** at [`.github/coderabbit-config/`](.github/coderabbit-config) — no network fetch on the validation path, so an upstream schema change cannot redden the fleet without a reviewed PR here; `refresh-coderabbit-schema.yml` opens that PR weekly when upstream drifts. Setup: [docs/callers/coderabbit-config-validate.md](docs/callers/coderabbit-config-validate.md). Pin `workflows_ref` to the same ref as `uses:`; no secrets required. | | [`cursor-review-catalog-drift.yml`](.github/workflows/cursor-review-catalog-drift.yml) | **Internal to this repo — not `workflow_call`able.** Weekly (Mon 06:17 UTC) + `workflow_dispatch` drift check between the `cursor-review.yml` model pins and Cursor's live `cursor-agent models` catalog. Reads the pins *out of* `cursor-review.yml` (panel heredoc + `judge_model` default) rather than duplicating them, then reports: a **delisted pin** (urgent — consumer PRs are about to fail the review preflight; this also fails the run), a **pin whose catalog line now says NO-ZDR** (also urgent, and the quieter failure — nothing breaks, private review diffs just keep flowing to a model that may retain them), **unpinned same-lab catalog ids** (a review-me list, never an auto-recommendation — "newest highest-reasoning ZDR-eligible" is a human call, and NO-ZDR markers are surfaced verbatim; rendered one row per model *family*, newest first, since ids that differ only by a reasoning/speed tier are not separate candidates), **catalog ids from families the panel pins nothing from** (a quieter collapsed catch-all, never urgent — a lab is an id's first `-`/`.`-separated token, so a lab the panel *does* pin can only surface there once it rebrands under a new prefix, e.g. OpenAI's `o` series alongside `gpt-*`), and a **`last checked` audit date** older than 30 days (or future-dated). Findings land in one sticky issue (`[cursor-review catalog drift]`, label `cursor-review-catalog-drift`, updated in place, and closed only by a run that finds *no* drift at all — rare in practice, since a live catalog always offers unpinned tiers, so the issue's own footer states whether that report needs action now or is advisory) with the raw catalog folded in. Least privilege by construction: the job that pipes Cursor's installer into bash holds only `contents: read` and hands its rendered report to a separate `issues: write` job via an artifact. Comparison logic + tests live in [`.github/cursor-review/catalog-drift.py`](.github/cursor-review/catalog-drift.py). Requires `CURSOR_API_KEY`. | — | | [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, and `workflows_ref` (required) must pin the generator to the same SHA as `uses:`. | — | diff --git a/docs/callers/public-repo-hygiene.md b/docs/callers/public-repo-hygiene.md index ad39140..5b68f4e 100644 --- a/docs/callers/public-repo-hygiene.md +++ b/docs/callers/public-repo-hygiene.md @@ -18,6 +18,9 @@ CI if it finds any: 3. **`Comfy-Org/` references outside a default-deny known-public allowlist** — plus the `@Comfy-Org/` CODEOWNERS-handle case, checked against a separate team allowlist. +Each category is applied to the file's **contents**, to its tracked **path**, and — for a symlink +— to its target string. See "Tracked paths are scanned too" below. + It is a lightweight regression guard, **not** a secrets scanner. Fails with a non-zero exit and GitHub annotations, so it wires in cleanly as a required status check. The checker lives in [`.github/public-repo-hygiene/`](../../.github/public-repo-hygiene). @@ -196,6 +199,26 @@ while the committed blob GitHub serves stays plainly readable, which would be a content the guard never looked at. Drop the attribute, or name those paths in `exclude_paths:` so the hole is counted in the log instead of hidden. +**Tracked paths are scanned too, not just file contents.** A tree containing +`docs/Comfy-Org//placeholder.md` or `notes/TEAM-1234/plan.md` publishes those +names to anyone who browses or clones the repo, however spotless the files themselves are — so +the path string goes through the same matcher, the same allowlists and the same knobs the +contents do. It runs for **every** non-excluded tracked entry, including the ones whose body is +never read (binary, non-UTF-8, submodule gitlink, LFS stub, unreadable): the path is published +whatever the entry type. A path finding is labelled ` (tracked path):` instead of +`::`, because the fix is to rename the file rather than edit it, and it does not +make an unread entry count towards `SCANNED:`. `exclude_paths:` covers this surface as well — an +excluded path is not scanned as a path either, and still reports its skipped-file count. Three +consequences worth knowing before you make the check required: a *vendored model mirror* +(`hf.co/Comfy-Org//…`, at the root or nested) is reported on its path, because the +model-host suppressor reads URL syntax that a path does not have; `exclude_paths:` clears that, +but an excluded subtree is not scanned for its **contents** either, so prefer renaming or the +narrowest entry that clears the finding; and scrubbing a name out of the **tree** does not scrub +it out of the **history**, which this checker does not read. Two shapes are documented rather than +handled: an allowlisted name with a file extension (`docs/Comfy-Org/ComfyUI.md`) over-flags, and +a lowercase ticket id in a path (`notes/be-1234/`) is a miss — the full list is in the checker +README's known limitations. + **Submodules are not scanned.** `git ls-files` lists a gitlink, and the workflow checks you out without `submodules:`, so the directory is empty here. Each is named in a `::warning::` and counted under `NOT SCANNED:`. A submodule's own files need their own hygiene run in their own repo.