[PR #102] [MERGED] fix(ci-groom): replace unreplaced REPLACE_AT_MERGE placeholder with a real main SHA pin (BE-5548) #112

New issue

Closed

opened 2026-10-26 22:36:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/102
Author: @mattmillerai
Created: 8/1/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-5548-ci-groom-pin

📝 Commits (2)

895242e fix(ci-groom): replace unreplaced REPLACE_AT_MERGE placeholder with a real main SHA pin (BE-5548)
c7ea3ac docs(ci-groom): correct the stale ORDERING note and record the pin's provenance

📊 Changes

1 file changed (+27 additions, -13 deletions)

View changed files

📝 .github/workflows/ci-groom.yml (+27 -13)

📄 Description

ELI-5

The daily "groom" janitor job in this repo was pointed at a signpost that read REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA instead of an actual address. GitHub couldn't find that address, so the job fell over before it even started — every single scheduled run for the last week. This PR writes the real address in (twice, because the job needs it in two places and they have to match).

Problem

.github/workflows/ci-groom.yml on main still carried the literal placeholder from the BE-4004 PR in both pins — the uses: ref (line 95) and the workflows_ref: input (line 102). The "replace at merge" step never happened, so the caller referenced a nonexistent ref and failed at startup. The 6 most recent ci-groom.yml runs (2026-07-27 → 2026-08-01, all schedule) are failure.

This is the exact failure mode AGENTS.md calls out under "Enrolling a caller is TWO steps" — and it names this very file: "This repo did it to its own ci-groom.yml."

Change

Both occurrences replaced with bcde90f4c5b0267d32583dc1d339ac8731c0da9a (the main tip at implementation time), annotated with the repo's # main @ <short> convention — the same form ci-assign-reviewers.yml uses. The stale ⚠️ BUMP-AT-MERGE instruction block is replaced with the standard pin note; it was an instruction to do exactly what this PR does, and leaving it would describe a placeholder that no longer exists.

Verification

grep -c REPLACE_AT_MERGE .github/workflows/ci-groom.yml → 0; no REPLACE_AT_MERGE or BUMP-AT-MERGE reference remains anywhere in the repo.
The two SHAs are byte-identical and 40-hex (grep -oE '[0-9a-f]{40}' yields one unique value), and git merge-base --is-ancestor bcde90f4… origin/main confirms the commit is on main.
The reusable at that SHA actually accepts what the caller sends. groom.yml@bcde90f declares every input passed here (bot_app_id, workflows_ref, dry_run, interval_days, cadence, volume_gate) and both secrets (ANTHROPIC_API_KEY required, BOT_APP_PRIVATE_KEY optional); the caller's permissions: grant matches the union groom.yml documents as required. builder is left at its false default, so no contents: write is needed.
The bumper can now move this file. Ran bump-callers.sh's actual rewrite rules against the edited file: GW_USES resolves to groom.yml alone (so GW_ONLY_OURS=1, no attribution guard), and both 40-hex pins and both # main @ annotations rewrite together in one pass. That is the real fix — see "Why the bumper never did this itself" below.
python3 -c "import yaml; yaml.safe_load(...)" parses the file.
Full local suite green: groom 170 tests, cursor-review 40, agents-md-integrity 18, bash .github/bump-callers/tests/test_bump_callers.sh 123/123, shellcheck -x clean, check_agents_md.py --root . passed (1 pre-existing warning: no CODEOWNERS, unrelated to this change).

Why the bumper never did this itself (roster check — step 3 of the ticket)

The ticket's hypothesis was that ci-groom.yml was missing from the GROOM_CALLERS roster. It is not. GROOM_CALLERS is readable and already contains an entry for this repo pointing at .github/workflows/ci-groom.yml — so no gh variable set was needed and none was made. (Roster contents are deliberately kept in repo variables because this repo is public; nothing beyond this repo's own self-entry is disclosed here or in the logs.)

The actual reason the pin never moved is narrower and worth recording: bump-callers.sh anchors its rewrite to s/[0-9a-f]{40}/${NEW_SHA}/g. The literal REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA is not 40-hex, so no bump could ever match it — the caller was enrolled and the bumper was running, but the placeholder was structurally invisible to it. Enrollment was never the gap; a hand-bump that never happened was. After this PR the pins are 40-hex and the fleet takes over automatically.

Judgment calls

Pinned to main tip (bcde90f), not to this PR's own squash SHA. The original comment asked for the latter, but that only made sense while the interval gate lived in that PR; it has been on main for months. Pinning to a commit that exists today is what makes the ref resolvable the moment this merges — a self-referential pin would require a second hand-bump and reproduce the bug.
Removed the ⚠️ BUMP-AT-MERGE comment block. It described a placeholder that no longer exists ("the placeholder below fails LOUD… until bumped"); keeping it would be a confident lie about the file's state. Nothing in the repo greps for that marker (verified).

Not done in this PR

Ticket step 4 — dispatching ci-groom.yml with dry_run=true after merge to confirm the run gets past startup — is not done and cannot be done from this PR, because the workflow that runs is the one on the default branch, so the check is only meaningful post-merge. Suggested one-liner for whoever merges:

gh workflow run ci-groom.yml -R Comfy-Org/github-workflows -f dry_run=true

A workflow_dispatch bypasses the interval gate, and dry_run: true also bypasses the volume gate (volume_gate: ${{ github.event.inputs.dry_run != 'true' }}), so it exercises the full audit without filing issues. Otherwise the next daily 09:17 UTC tick will show it. This PR's own CI does not cover it: ci-groom.yml is schedule/dispatch-only, so it never runs on a PR.

_{🔄 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/github-workflows/pull/102 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/1/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-5548-ci-groom-pin` --- ### 📝 Commits (2) - [`895242e`](https://github.com/Comfy-Org/github-workflows/commit/895242e802a07256dc037e2564bf78ea6829bc3d) fix(ci-groom): replace unreplaced REPLACE_AT_MERGE placeholder with a real main SHA pin (BE-5548) - [`c7ea3ac`](https://github.com/Comfy-Org/github-workflows/commit/c7ea3ac640ad2ea71f307f88e41aac2fc3d43001) docs(ci-groom): correct the stale ORDERING note and record the pin's provenance ### 📊 Changes **1 file changed** (+27 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci-groom.yml` (+27 -13) </details> ### 📄 Description ## ELI-5 The daily "groom" janitor job in this repo was pointed at a signpost that read `REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA` instead of an actual address. GitHub couldn't find that address, so the job fell over before it even started — every single scheduled run for the last week. This PR writes the real address in (twice, because the job needs it in two places and they have to match). ## Problem `.github/workflows/ci-groom.yml` on `main` still carried the literal placeholder from the BE-4004 PR in **both** pins — the `uses:` ref (line 95) and the `workflows_ref:` input (line 102). The "replace at merge" step never happened, so the caller referenced a nonexistent ref and failed at startup. The 6 most recent `ci-groom.yml` runs (2026-07-27 → 2026-08-01, all `schedule`) are `failure`. This is the exact failure mode AGENTS.md calls out under "Enrolling a caller is TWO steps" — and it names this very file: *"This repo did it to its own `ci-groom.yml`."* ## Change Both occurrences replaced with `bcde90f4c5b0267d32583dc1d339ac8731c0da9a` (the `main` tip at implementation time), annotated with the repo's `# main @ <short>` convention — the same form `ci-assign-reviewers.yml` uses. The stale `⚠️ BUMP-AT-MERGE` instruction block is replaced with the standard pin note; it was an instruction to do exactly what this PR does, and leaving it would describe a placeholder that no longer exists. ## Verification - `grep -c REPLACE_AT_MERGE .github/workflows/ci-groom.yml` → **0**; no `REPLACE_AT_MERGE` or `BUMP-AT-MERGE` reference remains anywhere in the repo. - The two SHAs are **byte-identical** and 40-hex (`grep -oE '[0-9a-f]{40}'` yields one unique value), and `git merge-base --is-ancestor bcde90f4… origin/main` confirms the commit is on `main`. - **The reusable at that SHA actually accepts what the caller sends.** `groom.yml@bcde90f` declares every input passed here (`bot_app_id`, `workflows_ref`, `dry_run`, `interval_days`, `cadence`, `volume_gate`) and both secrets (`ANTHROPIC_API_KEY` required, `BOT_APP_PRIVATE_KEY` optional); the caller's `permissions:` grant matches the union groom.yml documents as required. `builder` is left at its `false` default, so no `contents: write` is needed. - **The bumper can now move this file.** Ran `bump-callers.sh`'s actual rewrite rules against the edited file: `GW_USES` resolves to `groom.yml` alone (so `GW_ONLY_OURS=1`, no attribution guard), and both 40-hex pins **and** both `# main @` annotations rewrite together in one pass. That is the real fix — see "Why the bumper never did this itself" below. - `python3 -c "import yaml; yaml.safe_load(...)"` parses the file. - Full local suite green: groom 170 tests, cursor-review 40, agents-md-integrity 18, `bash .github/bump-callers/tests/test_bump_callers.sh` 123/123, `shellcheck -x` clean, `check_agents_md.py --root .` passed (1 pre-existing warning: no CODEOWNERS, unrelated to this change). ## Why the bumper never did this itself (roster check — step 3 of the ticket) The ticket's hypothesis was that `ci-groom.yml` was missing from the `GROOM_CALLERS` roster. **It is not.** `GROOM_CALLERS` is readable and already contains an entry for this repo pointing at `.github/workflows/ci-groom.yml` — so no `gh variable set` was needed and none was made. (Roster contents are deliberately kept in repo variables because this repo is public; nothing beyond this repo's own self-entry is disclosed here or in the logs.) The actual reason the pin never moved is narrower and worth recording: `bump-callers.sh` anchors its rewrite to `s/[0-9a-f]{40}/${NEW_SHA}/g`. The literal `REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA` is not 40-hex, so **no** bump could ever match it — the caller was enrolled and the bumper was running, but the placeholder was structurally invisible to it. Enrollment was never the gap; a hand-bump that never happened was. After this PR the pins are 40-hex and the fleet takes over automatically. ## Judgment calls - **Pinned to `main` tip (`bcde90f`), not to this PR's own squash SHA.** The original comment asked for the latter, but that only made sense while the interval gate lived *in that PR*; it has been on `main` for months. Pinning to a commit that exists today is what makes the ref resolvable the moment this merges — a self-referential pin would require a second hand-bump and reproduce the bug. - **Removed the `⚠️ BUMP-AT-MERGE` comment block.** It described a placeholder that no longer exists ("the placeholder below fails LOUD… until bumped"); keeping it would be a confident lie about the file's state. Nothing in the repo greps for that marker (verified). ## Not done in this PR **Ticket step 4 — dispatching `ci-groom.yml` with `dry_run=true` after merge to confirm the run gets past startup — is not done and cannot be done from this PR**, because the workflow that runs is the one on the default branch, so the check is only meaningful post-merge. Suggested one-liner for whoever merges: ``` gh workflow run ci-groom.yml -R Comfy-Org/github-workflows -f dry_run=true ``` A `workflow_dispatch` bypasses the interval gate, and `dry_run: true` also bypasses the volume gate (`volume_gate: ${{ github.event.inputs.dry_run != 'true' }}`), so it exercises the full audit without filing issues. Otherwise the next daily 09:17 UTC tick will show it. This PR's own CI does not cover it: `ci-groom.yml` is `schedule`/`dispatch`-only, so it never runs on a PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:22 +00:00