[PR #66] [MERGED] ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy #68

New issue

Closed

opened 2026-10-31 22:04:16 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:16 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/66
Author: @mattmillerai
Created: 8/22/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: main ← Head: chore/adopt-shared-hygiene

📝 Commits (8)

90f0e78 ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy
d4b1239 Merge origin/main into chore/adopt-shared-hygiene
93ef054 docs: describe the shared hygiene checker instead of the deleted in-tree one
b1bd4fe Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene
858a89e Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene
a3a8ef7 Merge origin/main, resolve ci.yml conflict by keeping public-repo-hygiene job removed
6b1e5a1 ci: re-export the shared hygiene result under the required check name
11dd694 Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene

📊 Changes

5 files changed (+125 additions, -247 deletions)

View changed files

📝 .github/workflows/ci.yml (+0 -21)
➕ .github/workflows/public-repo-hygiene.yml (+56 -0)
📝 AGENTS.md (+48 -22)
📝 CONTRIBUTING.md (+21 -10)
➖ scripts/check-public-repo-hygiene.mjs (+0 -194)

📄 Description

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

Adds .github/workflows/public-repo-hygiene.yml — a thin caller for the reusable workflow in Comfy-Org/github-workflows, plus a one-step public-repo-hygiene gate job that re-exports the caller's result under the exact check name main's branch protection requires (see below)
Deletes scripts/check-public-repo-hygiene.mjs (193 lines)
Removes the public-repo-hygiene job from ci.yml
Updates AGENTS.md and CONTRIBUTING.md to describe the shared checker, where allowlist and false-positive fixes now land, and how to reproduce the check locally

Why, 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:

- uses: actions/checkout@...   # the PR merge ref
- run: <the checker, from the PR's own tree>

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_ref commit, 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:

`scripts/check-public-repo-hygiene.mjs`: possible internal ticket ID: 'TEAM-1234'

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 which uses: line runs — a pull_request caller executes its workflow file from the PR head, so a PR that rewrites both the uses: line and workflows_ref runs 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/. That is worth adding here, and the conditional this section used to hang it on is already met — public-repo-hygiene is in main's required_status_checks.contexts today. 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.

Keeping the required check name

main's branch protection requires a status check named exactly public-repo-hygiene, and GitHub matches required contexts by exact string. A reusable-workflow call cannot produce that name — a called job is always reported as <caller job> / <called job>, so the caller above surfaces only as hygiene / public-repo-hygiene. Confirmed empirically via the check-runs API against the pre-fix head a3a8ef7: hygiene / public-repo-hygiene was present and a bare public-repo-hygiene was not.

Left alone that is not cosmetic. The required context would sit permanently unreported — blocking this PR, and then every future PR against main.

So the workflow also defines a one-step public-repo-hygiene job that passes iff the shared job succeeded. That keeps the existing branch-protection config working untouched, with no admin edit needed. if: always() on it is load-bearing: without it a failing hygiene would leave the gate skipped, and branch protection counts a skipped required check as satisfied — the failure would sail through the gate that exists to catch it.

Once merged, this repo gets enrolled in the PUBLIC_REPO_HYGIENE_CALLERS roster so the bump fleet keeps the pin fresh. Without enrolment the pin never moves, which is its own quiet failure mode.

Provenance

Authored by: agent-work loop

Verified: pnpm lint, pnpm format:check, pnpm typecheck, pnpm test (642 passed, 5 skipped), pnpm check:spec-drift and pnpm build all pass locally on the branch tip 6b1e5a1. The missing required check name was confirmed empirically against the check-runs API and main's branch-protection required_status_checks.contexts, not inferred. The gate job has since run: on 6b1e5a1 the check-runs API now lists a bare public-repo-hygiene as success alongside hygiene / public-repo-hygiene, so all four required contexts (Test (node 22), Test (node 24), build-check (publish dry run), public-repo-hygiene) report green and the gate is confirmed working rather than assumed. origin/main (tip 4491e4b, a vendored Router spec sync) has since been merged into the branch and pushed as 11dd694; on that head pnpm lint, pnpm format:check, pnpm typecheck, pnpm check:spec-drift, pnpm test (642 passed, 5 skipped) and pnpm build all pass locally, and CI on it is green for every required context — Test (node 22), Test (node 24), build-check (publish dry run), public-repo-hygiene — plus hygiene / public-repo-hygiene.

Deviations: sdk-parity (python surface freshness) is red on this branch, but it is pre-existing drift on main itself (identical RouterRunResult extractor failure on main's own tip), tracked by #156, with the fix open as #167 (whose own sdk-parity run passes). It is unrelated to this PR's diff and is not a required status check, so it is deliberately not fixed here — re-confirmed on main's own tip 4491e4b, where the identical sdk-parity job is the sole failure.

Summary by CodeRabbit

Chores
- Moved public repository hygiene checks to a centralized, pinned workflow.
- Checks now run automatically for pull requests and pushes to the main branch.
- Replaced the repository-local hygiene checker and previous CI job.
- Centralized ownership of checker rules and allowlists while retaining pinned revisions.
Documentation
- Updated contributor guidance for centralized checks, configuration, exceptions, and local reproduction.
- Documented false-positive handling and ticket-pattern exceptions.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/66 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/22/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `chore/adopt-shared-hygiene` --- ### 📝 Commits (8) - [`90f0e78`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/90f0e78d364257fbd023915bc47508a7674afa03) ci: adopt the shared public-repo-hygiene workflow, drop the in-tree copy - [`d4b1239`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/d4b1239f51b170ca2d3ee9a147fb268683c8f0be) Merge origin/main into chore/adopt-shared-hygiene - [`93ef054`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/93ef054ab568bfd1aec2308660ec5e8da83dd8d8) docs: describe the shared hygiene checker instead of the deleted in-tree one - [`b1bd4fe`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/b1bd4fe450d0b2440d04d3562b7754424b8a6bb7) Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene - [`858a89e`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/858a89e520bbbc78be1e0dcd8b88d990ffcec985) Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene - [`a3a8ef7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/a3a8ef7763d4820fee4c8f1972d90ee19a4c3de8) Merge origin/main, resolve ci.yml conflict by keeping public-repo-hygiene job removed - [`6b1e5a1`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/6b1e5a1be3f9ba3bb4d432bba31a65eb3dc1d1cb) ci: re-export the shared hygiene result under the required check name - [`11dd694`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/11dd6949e3cc65f6994635cbd4f6a45025978fe2) Merge remote-tracking branch 'origin/main' into chore/adopt-shared-hygiene ### 📊 Changes **5 files changed** (+125 additions, -247 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+0 -21) ➕ `.github/workflows/public-repo-hygiene.yml` (+56 -0) 📝 `AGENTS.md` (+48 -22) 📝 `CONTRIBUTING.md` (+21 -10) ➖ `scripts/check-public-repo-hygiene.mjs` (+0 -194) </details> ### 📄 Description ## 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 - **Adds** `.github/workflows/public-repo-hygiene.yml` — a thin caller for the reusable workflow in `Comfy-Org/github-workflows`, plus a one-step `public-repo-hygiene` gate job that re-exports the caller's result under the exact check name `main`'s branch protection requires (see [below](#keeping-the-required-check-name)) - **Deletes** `scripts/check-public-repo-hygiene.mjs` (193 lines) - **Removes** the `public-repo-hygiene` job from `ci.yml` - **Updates** `AGENTS.md` and `CONTRIBUTING.md` to describe the shared checker, where allowlist and false-positive fixes now land, and how to reproduce the check locally ## Why, 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: ```yaml - uses: actions/checkout@... # the PR merge ref - run: <the checker, from the PR's own tree> ``` 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_ref` commit, 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: ``` `scripts/check-public-repo-hygiene.mjs`: possible internal ticket ID: 'TEAM-1234' ``` 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 which `uses:` line runs — a `pull_request` caller executes its workflow file from the PR head, so a PR that rewrites both the `uses:` line and `workflows_ref` runs 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/`. That is worth adding here, and the conditional this section used to hang it on is already met — `public-repo-hygiene` is in `main`'s `required_status_checks.contexts` today. 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**. ## Keeping the required check name `main`'s branch protection requires a status check named exactly `public-repo-hygiene`, and GitHub matches required contexts by exact string. A reusable-workflow call cannot produce that name — a called job is always reported as `<caller job> / <called job>`, so the caller above surfaces only as `hygiene / public-repo-hygiene`. Confirmed empirically via the check-runs API against the pre-fix head `a3a8ef7`: `hygiene / public-repo-hygiene` was present and a bare `public-repo-hygiene` was not. Left alone that is not cosmetic. The required context would sit permanently unreported — blocking this PR, and then every future PR against `main`. So the workflow also defines a one-step `public-repo-hygiene` job that passes iff the shared job succeeded. That keeps the existing branch-protection config working untouched, with no admin edit needed. `if: always()` on it is load-bearing: without it a failing `hygiene` would leave the gate *skipped*, and branch protection counts a skipped required check as satisfied — the failure would sail through the gate that exists to catch it. ## Next Once merged, this repo gets enrolled in the `PUBLIC_REPO_HYGIENE_CALLERS` roster so the bump fleet keeps the pin fresh. Without enrolment the pin never moves, which is its own quiet failure mode. ## Provenance **Authored by:** agent-work loop **Verified:** `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm test` (642 passed, 5 skipped), `pnpm check:spec-drift` and `pnpm build` all pass locally on the branch tip `6b1e5a1`. The missing required check name was confirmed empirically against the check-runs API and `main`'s branch-protection `required_status_checks.contexts`, not inferred. The gate job has since run: on `6b1e5a1` the check-runs API now lists a bare `public-repo-hygiene` as **success** alongside `hygiene / public-repo-hygiene`, so all four required contexts (`Test (node 22)`, `Test (node 24)`, `build-check (publish dry run)`, `public-repo-hygiene`) report green and the gate is confirmed working rather than assumed. `origin/main` (tip `4491e4b`, a vendored Router spec sync) has since been merged into the branch and pushed as `11dd694`; on that head `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm check:spec-drift`, `pnpm test` (642 passed, 5 skipped) and `pnpm build` all pass locally, and CI on it is green for every required context — `Test (node 22)`, `Test (node 24)`, `build-check (publish dry run)`, `public-repo-hygiene` — plus `hygiene / public-repo-hygiene`. **Deviations:** `sdk-parity (python surface freshness)` is red on this branch, but it is pre-existing drift on `main` itself (identical `RouterRunResult` extractor failure on main's own tip), tracked by #156, with the fix open as #167 (whose own `sdk-parity` run passes). It is unrelated to this PR's diff and is not a required status check, so it is deliberately not fixed here — re-confirmed on `main`'s own tip `4491e4b`, where the identical `sdk-parity` job is the sole failure.  ## Summary by CodeRabbit * **Chores** * Moved public repository hygiene checks to a centralized, pinned workflow. * Checks now run automatically for pull requests and pushes to the main branch. * Replaced the repository-local hygiene checker and previous CI job. * Centralized ownership of checker rules and allowlists while retaining pinned revisions. * **Documentation** * Updated contributor guidance for centralized checks, configuration, exceptions, and local reproduction. * Documented false-positive handling and ticket-pattern exceptions.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:16 +00:00