chore(hygiene): clear the remaining public-repo-hygiene findings - #768
Conversation
…ings Both were prose provenance notes that named a private repo by its full org/name path. The unqualified names they describe already appear elsewhere in the same files and are not what the hygiene check denies, so the fact survives and the path does not.
…iables comfy-cli is public, and this workflow named its private source repo in eight places plus hardcoded the file's path inside that repo, which says more about the service's internals than the repo name does. Both now come from CQL_NO_GPU_SOURCE_REPO and CQL_NO_GPU_SOURCE_PATH, already set on this repo. Neither is echoed anywhere: Actions logs on a public repo are world-readable. The unset case is checked beside the token check and fails loudly, because an empty expansion would otherwise produce a 404 that reads like the file moved rather than like a missing prerequisite.
fixtures/gallery/ holds templates copied verbatim from an upstream repo, with a documented curl loop in its README that re-downloads them. Their markdown labels model links with the weights' own owner/name, which reaches the checker as a bare owner/name and is rightly read as a GitHub repo path. Editing them is not an option: a refresh reverts it and breaks the verbatim contract the fixtures exist to keep. The exclusion is reported with its skipped-file count in the run log.
dosubot
Bot
added
the
size:XS
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 128 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 4 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…xclusion The upstream checker now resolves a markdown link label against its own target, so the gallery fixtures' model links no longer read as GitHub repo paths. The exclusion those two findings justified is a coverage hole with nothing left to cover, so it goes and the fixtures are scanned again.
main landed #785, an independent fix for the same red check, while this branch was open. The two disagree about method, so the merge picks one per file rather than taking either side wholesale. Docstrings (engine.py, knowledge.py): take main's wording. Same intent, already reviewed and landed; this branch's rewording was redundant. public-repo-hygiene.yml: keep this branch's pin bump (3b2c8ca) and main's HALO-03 ticket allowlist, and drop main's whole exclude_paths block. Every entry in it was a checker false positive — Hugging Face model URLs and HF `owner/name` repos reading as unlisted GitHub repos — and all of them are fixed upstream at the new pin. The last entry, refresh-cql-catalogs.yml, was exempting the one real finding; it is unnecessary now that the file names no private repo. Verified by running the pinned checker against the merged tree: no internal-only references found, with no exclusions. refresh-cql-catalogs.yml: the private source coordinates move to SECRETS, not repo variables. Actions prints each step's resolved `env:` block into the run log, and on a public repo those logs are world-readable, so a variable is republished on every run — confirmed in this repo's own hygiene log, which prints `TICKET_ALLOWLIST: HAILUO-03` verbatim. That would have defeated the point of moving them out of the file. Also from review: - Validate both coordinates before they are spliced into a REST path. They are admin-mutable without code review, and a `?` or `#` would end the path component and silently drop the `?ref=main` pin. - `if ! SHA=$(gh api ...)` so the error can actually print under `set -e`, and reject the literal `null` that `--jq` emits for a payload with no `sha` (it is non-empty, so it sailed past the `-z` test into `git/blobs/null`). - Record the source blob SHA in the auto-PR body, so a reviewer can tell a real upstream change from a source quietly re-pointed via the secrets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Rebuilt on current This merge partly reverses #785. That PR landed today and fixed the same red check by a different method — an Pre-merge admin step. Not merging this; leaving it for review. |
skishore23
left a comment
skishore23
left a comment
There was a problem hiding this comment.
Approving — the diff is strictly better than main: coordinates gone from the file, zero exclude_paths, the 3b2c8ca pin is a full-SHA ancestor of the reusable's main (exactly the two HF false-positive fixes past 609f972), the guard rejects every hostile shape I threw at it (URL form, .., %2F, @, ?, #, newline, %00…), gh api's error output carries no request path so a 404/401 can't leak the coordinates, and the pinned checker on the head tree with no exclusions reports no internal-only references found (492 files). actionlint clean.
One thing to act on before anyone provisions the new secrets — the fetch this PR hardens is dead code. comfy_cli/cql/data/no_gpu_nodes.json and its parser were deleted from main in #474 (e36fdd33); the only remaining references to no_gpu_nodes in the tree are the 25 in this workflow. Consequences at the head: git diff --quiet origin/main -- …/no_gpu_nodes.json exits 0 for a path tracked on neither side, so drift is never detected for it, but whenever either public catalog drifts the cp + git add block commits the private file into the public repo as a new tracked file with no consumer, and the auto-PR body's "loaded at runtime via importlib.resources" line is false for it. Nothing regresses today (the job already fails at step 1 daily, and none of CQL_NO_GPU_SOURCE_* / CLOUD_REPO_READ_TOKEN are provisioned as secrets), so not a blocker — but the right hygiene fix is to delete the fetch, the guard, CLOUD_REPO_READ_TOKEN, and the two leftover repo variables (which still exist), rather than hide the coordinates. Happy to see that folded in here or as an immediate follow-up; please don't create the secrets.
Minor: the blob-SHA "provenance" in the PR body is the hash of the bytes the same PR commits (git hash-object reproduces it), so it can't distinguish an upstream change from a re-pointed secret — fine to keep, just not the guarantee the comment describes.
|
agent-fleet merger — held (G12). An approval on this PR reads as an unresolved ask: «please don't create the secrets.» If that is not blocking, the fastest clear is a re-approval. Clears when: EVERY reviewer whose approval body carries an ask re-approves with a body that has none — a reviewer clears only their OWN body, so a different reviewer approving on top clears nothing; or a human with triage rights adds Replying here does not clear the hold on its own — the merger reads approvals and labels, not comment text. This notice is posted once per head; it will not repeat. |
2 participants
Summary
Everything comfy-cli needs for
public-repo-hygieneto pass at full strength, in one PR. Supersedes #767.mainfixed the same red check independently while this was open (#785), by a different method: it told the checker to skip the offending paths. This branch removes the reasons instead, so the merge keeps the check green and keeps it a default-deny control.refresh-cql-catalogs.ymlinto secrets3b2c8caexclude_paths:dropped entirely (was 4 entries onmain)main's equivalent wording, already landed, winsDetails
The coordinates are secrets, not repo variables
refresh-cql-catalogs.ymlnamed a private repo in eight places and hardcoded the file's path inside it. The path is the part worth noting: it describes a private service's internal layout, and the hygiene checker never caught it because it only matches<owner>/<name>.Both now come from
CQL_NO_GPU_SOURCE_REPOandCQL_NO_GPU_SOURCE_PATH. An earlier revision of this PR made them repo variables; review was right that that does not work, and it is now fixed. Actions prints each step's resolvedenv:block into the run log, and on a public repo those logs are world-readable — so a variable is republished on every run. That is not theoretical: this repo's own hygiene job printsTICKET_ALLOWLIST: HAILUO-03verbatim in its public log. Secrets are masked in that same block, so secrets it is.Three things a reviewer should look at:
repos//contents/would 404 in a way that reads like the file moved rather than like a missing prerequisite.CQL_NO_GPU_SOURCE_REPOmust be a bareowner/name;CQL_NO_GPU_SOURCE_PATHmust be a repo-relative path with no leading/, no.., and no?or#— either of those last two would end the path component and silently drop the?ref=mainbranch pin.Two smaller review fixes in the same step:
if ! SHA=$(gh api ...)so the error can actually print (underset -ea bare assignment aborts first), and rejecting the literalnullthat--jqemits for a 200 with nosha— it is non-empty, so it sailed past the-ztest intogit/blobs/null. The auto-PR body now also records the source blob SHA, so a reviewer of a generated data PR can tell a real upstream change from a source quietly re-pointed by editing the secrets.No exclusions at all
maincarries anexclude_paths:block with four entries. Three are Hugging Face artifacts — model URLs and HFowner/namemodel repos — that the checker read as unlisted GitHub repos; those are fixed in the checker itself as of the pin here. The fourth exemptedrefresh-cql-catalogs.yml, which is unnecessary once that file names nothing private.So the whole block goes. An exclusion suppresses the scan for a path forever, including files not yet written, and
exclude_pathsis a caller-supplied input — which is the same shape of hole this workflow's own header says the out-of-tree refactor closed. Zero entries is the only setting where that concern does not apply.Testing
Ran the pinned checker (
3b2c8ca) against the merged tree, with no exclusions:no internal-only references found, 492 files scanned. Confirmed both ticket-allowlist entries are still load-bearing (dropping either reproduces findings).Guard step exercised directly against 12 inputs — valid, URL form, extra path segments, empty, leading slash,
?,#,.., trailing slash, embedded newline, space, single-segment path — each lands on the intended branch. Both workflow files parse as YAML.ruff checkandruff format --diffclean at the CI-pinned 0.15.15.pytest tests/comfy_cli: 5973 passed, 14 skipped, 1 failed —test_from_workflow_refuses_a_workflow_nested_past_the_parser_limit, which short-circuits onbuild_not_signed_inon an unauthenticated dev machine. Pre-existing and unrelated; this PR changes no Python.Before merge
CQL_NO_GPU_SOURCE_REPOandCQL_NO_GPU_SOURCE_PATHcurrently exist on this repo as variables. They need to be re-created as secrets (Settings → Secrets and variables → Actions → Secrets), after which the variables can be deleted.If that is missed,
refresh-cql-catalogsfails loudly at its guard step with the setup instructions — the contract that file already documents. It cannot regress anything in the meantime: that scheduled job has been failing daily onmainsince before this PR, at its very first step, because theCLOUD_CODE_BOTapp credentials are not provisioned either (latest run).ELI5
A check runs on every PR here looking for mentions of private, internal things that should not appear in a public repo. It had been failing since the day it was turned on, which is the worst state for a check to be in: people learn to scroll past a red mark, and a real problem then looks exactly like the noise.
Most of what it flagged was not a real problem — mostly download links for public AI models that the checker misread as private code. That is fixed in the shared checker, and this PR picks up the fix.
The genuine one was a scheduled job that copies a file out of a private repo and spelled out both that repo's name and the exact folder path inside it. Both moved into repository settings. Review caught that the first attempt put them somewhere that gets printed into the build log, which is public — so they are stored as secrets instead, which the log hides.
The last piece is about how the check gets to green.
maingot there by handing the checker a list of files to skip. This PR removes the list, because a skip list is permanent and quietly covers files nobody has written yet. Nothing is skipped now; there is simply nothing left to find.Provenance
Authored by: agent-work loop
Verified: Pinned hygiene checker run against the merged tree with zero exclusions (
no internal-only references found, 492 files); ticket-allowlist entries confirmed still needed; guard shape-checks exercised against 12 inputs; both workflow YAMLs parse;ruff check+ruff format --diffclean at CI-pinned 0.15.15;pytest tests/comfy_cli5973 passed / 1 pre-existing unauthenticated-machine failure. Theenv:-block leak that motivated the vars→secrets switch was confirmed against this repo's own public run log rather than assumed.Deviations: The Contents-API 1 MB ceiling on the blob-SHA lookup (raised in review) is documented in place rather than fixed — it is pre-existing, and the catalog is three orders of magnitude below the limit. Provisioning the two secrets is left to a repo admin; this PR cannot do it.