ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy - #68
ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy#68mattmillerai wants to merge 3 commits into
Conversation
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.
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 116 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe local public-repository hygiene job and checker are removed. A separate workflow delegates these checks to a pinned reusable workflow on pull requests and pushes to ChangesPublic repository hygiene
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change replaces the repository-local hygiene check with a shared workflow and removes the duplicated in-tree checker; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…andoff main added the pre-rename name ComfyPythonSDK to the in-tree hygiene allowlist, but this branch deletes that script in favor of the shared Comfy-Org/github-workflows reusable, whose org-wide allowlist (pinned at aad0697) doesn't carry that entry. Rephrase the CHANGELOG's rename note so it no longer forms a Comfy-Org/ComfyPythonSDK reference, verified clean against the pinned checker script.
…giene # Conflicts: # .github/workflows/ci.yml # scripts/check_public_repo_hygiene.py
1 participant
ELI-5
This repo had its own script for catching internal-only references (Linear IDs, Datadog links, private repo names) before they get published. The problem: the script lived in the repo it was guarding, so a PR could edit the script — or just delete the check — and still go green. This swaps it for a shared one that a PR here can't reach.
What changes
.github/workflows/public-repo-hygiene.yml— a thin caller for the reusable workflow inComfy-Org/github-workflowsscripts/check_public_repo_hygiene.py(184 lines)public-repo-hygienejob fromci.ymlWhy, and it isn't really deduplication
Deduplication is real — this repo and its sibling SDK carried the same three-category policy in two languages, and a one-line allowlist fix recently had to be made twice. But the actual reason is tamper-resistance. The old job was:
So a PR could add a private repo name to the in-tree allowlist and then leak it, green. Or delete the job. A guard a change can edit is not a guard against that change. The reusable loads the checker from the pinned
workflows_refcommit, and the known-public allowlist is deliberately not an input — an allowlist a caller can pass is one a PR in the caller repo can widen.Parity was proven before deleting anything
Run against this tree, the in-tree checker reported clean, and the shared checker reported exactly one finding:
That is the example ticket format inside the old script's own doc comment — in the file this PR deletes. With it gone, the shared checker is clean too. So the two verdicts agree on everything that survives the change, and the one difference is explained rather than waved past.
What the pin does and doesn't buy
Worth stating plainly, because it is easy to overclaim and the reusable's own header is careful about it: the pin guarantees the checker comes from the commit the
uses:line resolved to. It cannot govern whichuses:line runs — apull_requestcaller executes its workflow file from the PR head, so a PR that rewrites both theuses:line andworkflows_refruns a different reusable entirely. That is true of every reusable workflow on GitHub.The control for it is out of band: a branch-protection rule or ruleset requiring non-author review of
.github/workflows/. Worth adding here if this ever becomes a required status check. CODEOWNERS alone is not that control — it requests reviewers and blocks nothing.So the honest claim for this PR is the narrower one: a PR here can no longer reach the checker or the allowlist through this workflow's inputs.
Next
Once merged, this repo gets enrolled in the
PUBLIC_REPO_HYGIENE_CALLERSroster so the bump fleet keeps the pin fresh. Without enrolment the pin never moves, which is its own quiet failure mode.Summary by CodeRabbit