diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 3b9c293..87adde0 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -30,6 +30,27 @@ forward automatically instead of silently drifting commits behind. | [`bump-assign-reviewers-callers.yml`](../workflows/bump-assign-reviewers-callers.yml) | `assign-reviewers.yml` | `ASSIGN_REVIEWERS_CALLERS` | empty `[]` (grows as callers land) | | [`bump-groom-callers.yml`](../workflows/bump-groom-callers.yml) | `groom.yml` or `groom/**` | `GROOM_CALLERS` | empty `[]` (grows as callers land) | | [`bump-auto-label-callers.yml`](../workflows/bump-auto-label-callers.yml) | `cursor-review-auto-label.yml` | `AUTO_LABEL_CALLERS` | non-empty (hard-fails if empty) | +| [`bump-detect-unreviewed-merge-callers.yml`](../workflows/bump-detect-unreviewed-merge-callers.yml) | `detect-unreviewed-merge.yml` | `DETECT_UNREVIEWED_MERGE_CALLERS` | **not yet seeded** — hard-fails until it is (see below) | + +### Why the detect-unreviewed-merge roster is not seeded yet + +Its 12 live callers are known and correctly wired (each pins a full 40-hex SHA +against this repo's path, i.e. exactly what the rewrite moves). It is unseeded +anyway, on purpose. + +Every roster reaches `bump-callers.sh` through the step's `env:` block, and +Actions prints that block — values and all — before the script's `::add-mask::` +can run. This repo is public, so each seeded fleet already publishes its roster +in a world-readable log. That is the known gap documented in every `bump-*` +header. The difference here is that two of this fleet's callers are non-public +repos that appear in **no** already-seeded roster, so seeding would publish two +names that are not out yet — and a public log entry cannot be unpublished. + +So the trade is: seed now and take an irreversible disclosure, or leave it +unseeded and take a red run. The red run is reversible and is already the +designed behaviour for an empty roster, so it wins. Seed the variable as the +immediate follow-on to the masking fix — never as a way to turn that red run +green. ### Reusables with no fleet — deliberate, not an oversight @@ -37,7 +58,6 @@ forward automatically instead of silently drifting commits behind. |---|---|---| | `stale.yml` | 0 | Nothing to bump. Add a fleet when the first caller lands. | | `assign-prs-to-author.yml` | 0 | Same. | -| `detect-unreviewed-merge.yml` | ~12 | **A real gap.** Its pins are bumped by hand. Deferred deliberately, not missed. | A reusable that has callers but no fleet is the trap this whole directory exists to prevent: the pins simply never move, so consumers drift behind indefinitely diff --git a/.github/workflows/bump-detect-unreviewed-merge-callers.yml b/.github/workflows/bump-detect-unreviewed-merge-callers.yml new file mode 100644 index 0000000..749b33a --- /dev/null +++ b/.github/workflows/bump-detect-unreviewed-merge-callers.yml @@ -0,0 +1,270 @@ +name: Bump detect-unreviewed-merge callers + +# When detect-unreviewed-merge.yml is updated on main, open a SHA-bump PR in +# every repo that pins a caller against it. PRs are opened by Cloud Code Bot so +# they are easy to filter and merge. +# +# This is a thin entrypoint over the shared bumper at +# .github/bump-callers/bump-callers.sh — the same script drives the +# cursor-review, agents-md-integrity, pr-size, assign-reviewers, auto-label and +# groom caller fleets. Keeping ONE implementation is deliberate: a forked copy +# is how other shared machinery in the org has drifted. The entrypoints differ +# only in their path-filter trigger and the parameters passed below. (They stay +# separate rather than a single matrix because their triggers differ — a +# detect-unreviewed-merge.yml change must not spuriously bump the other fleets' +# callers, and vice versa.) +# +# Unlike cursor-review/groom/pr-size, this reusable loads NOTHING at run time — +# no `workflows_ref:` input, no prompt/script directory — so the workflow file +# itself is the whole surface a caller pins, and the path filter has a single +# entry. +# +# The caller list is NOT hardcoded here. This repo is PUBLIC (workflow file and +# Actions run logs are both publicly viewable), and most callers are private, so +# their names must never appear in this file or its logs. The list lives in the +# repo-level Actions variable `DETECT_UNREVIEWED_MERGE_CALLERS` (config, not a +# credential — a variable, not a secret, since secrets are write-only via the +# API) as a JSON array of {"repo","file","label"} objects, same shape as the +# other fleets' variables. `bump-callers.sh` `::add-mask::`es every repo name +# before IT echoes anything. +# KNOWN GAP (shared by every bump-* entrypoint, tracked separately): passing the +# roster through this step's `env:` means Actions prints the raw value in the +# step's env dump before the script — and its masking — ever runs. Closing it +# means reading the variable at run time (`gh variable get`) and masking it +# first, which needs a token permission this fleet does not mint today; it is a +# fleet-wide change, not one this entrypoint can make alone. Until then, treat +# the roster as visible in the public run log. +# +# CONSEQUENCE — this fleet ships DELIBERATELY UNSEEDED. Every other seeded fleet +# already publishes its roster this way, so their names are out; this fleet's +# callers include two non-public repos that no existing roster contains, and +# seeding before the masking fix lands would publish them on the very first run. +# A public log entry cannot be unpublished; a red run can. So the variable stays +# unset until the masking fix lands, and the resulting hard-fail is the +# intentional nag (see ALLOW_EMPTY below) rather than an oversight. Seed it — +# `gh variable set` per the update flow below — as the immediate follow-on to +# that fix, NOT as a way to turn the red run green. +# +# ALLOW_EMPTY is set to "false" below — already `bump-callers.sh`'s default, but +# stated explicitly (as `bump-auto-label-callers.yml` does) so the property +# survives a future flip of that default. This fleet has a known, non-empty set +# of LIVE CALLERS, so an empty run is never "this fleet has no members" and must +# never be a silent no-op: that would leave every caller un-bumped with nobody +# noticing, exactly the drift this directory exists to prevent. +# +# READ THIS BEFORE ACTING ON A RED RUN. An empty roster right now is the EXPECTED +# PRE-SEED STATE described above, not a clobbered variable. Until the masking fix +# lands, the correct response to that red run is to leave it red — do NOT "fix" +# it by seeding the variable, which is precisely the irreversible disclosure this +# header exists to prevent. Once masking is in place, seeding is the follow-on +# and the run goes green on its own. (After that point, and only after it, an +# empty roster does mean the variable was clobbered and should be restored.) +# Either way the answer is never to flip ALLOW_EMPTY on. +# +# Update flow — adding/removing a caller needs NO public commit: +# gh variable set DETECT_UNREVIEWED_MERGE_CALLERS --repo Comfy-Org/github-workflows \ +# --body "$(jq -c . callers.json)" +# Keep the canonical callers.json in a PRIVATE infra/ops repo so variable edits +# have a reviewed source of truth; the org audit log records each edit. (The +# specific home repo is intentionally not named here — this file is public.) + +on: + workflow_dispatch: {} # allow on-demand runs (e.g. to re-bump callers) + push: + branches: [main] + paths: + - .github/workflows/detect-unreviewed-merge.yml + +permissions: + contents: read + +# Serialize runs of this fleet. The bumper pushes to a STABLE branch +# (ci/bump-detect-unreviewed-merge) shared across runs, so two overlapping runs +# (a rapid second main push, or a push racing a manual re-run) would force-reset +# that branch and race the PR update — an older run finishing last could leave +# the committed diff pinned to a stale SHA. cancel-in-progress: false lets the +# running bump finish; GitHub keeps only the newest pending run, so the latest +# SHA always wins (BE-3882). +# Edge, shared by all seven bump-* entrypoints: "newest pending" is by ENQUEUE +# time, not by commit date, so manually re-running a superseded run while a +# newer push run is still pending evicts that newer run — the re-run then finds +# WATCHED changed and correctly no-ops, and nobody pins the newer SHA. It is +# self-healing (the next change to WATCHED bumps everything forward) and +# recoverable now (`workflow_dispatch` from main pins the current tip), so it is +# documented rather than guarded against here: the guard belongs in the shared +# staleness script, not forked into one of seven copies. +concurrency: + group: bump-detect-unreviewed-merge-callers + cancel-in-progress: false + +jobs: + bump: + # Guard against a workflow_dispatch run from a non-main ref: github.sha is + # the tip of whatever ref was selected, and the bumper force-resets a stable + # shared branch — a manual run from an older ref would pin every caller to a + # stale SHA (and run this job's script, with the App token minted below, + # from an unreviewed commit). The push trigger is already main-only, so this + # only ever skips stray manual runs. + if: github.ref == 'refs/heads/main' + runs-on: ubuntu-latest + # The job is a bounded loop of `git`/`gh` network calls; nothing here should + # take minutes. Without this a hung call would hold a runner for the default + # 6 hours (and, via the serializing concurrency group above, block every + # later bump behind it). + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Generate Cloud Code Bot token + uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1.12.0 — pinned: see dependabot.yml ignore (v2+ breaks cross-repo `owner:` token scoping) + id: token + with: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} + # `owner:` with no `repositories:` is deliberate and cannot be narrowed: + # the caller list is a runtime variable (see the header), so the repos + # this token must reach are not knowable at authoring time — and naming + # them here would leak private caller names into a public file. + # + # The PERMISSIONS can be narrowed without naming anything, so they are: + # unset, the minted token carries every permission the app holds on every + # repo it is installed on, far past what this bumper needs. It commits + # the rewritten caller file via the Git Data API (contents) and opens or + # updates the bump PR (pull-requests); `gh pr create --label` stamps the + # caller's label through the issues API (issues) — same trio, and the + # same reasoning, as the auto-label and groom fleets' entrypoints. + owner: Comfy-Org + permission-contents: write + permission-pull-requests: write + permission-issues: write + + - name: Bump SHA in caller repos + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + NEW_SHA: ${{ github.sha }} + VAR_NAME: DETECT_UNREVIEWED_MERGE_CALLERS + TAG: detect-unreviewed-merge + WORKFLOW_FILE: detect-unreviewed-merge.yml + # Redundant with bump-callers.sh's own default, deliberately — see the + # header: this fleet has a known non-empty set of live callers, so an + # empty roster is never "this fleet has no members" and must fail + # loudly regardless of what that default becomes. RIGHT NOW that + # failure is EXPECTED — the roster ships unseeded until the masking + # fix lands (header, "READ THIS BEFORE ACTING ON A RED RUN"). Do NOT + # respond to it by seeding the variable; only after masking is in + # place does an empty roster mean it was clobbered. + ALLOW_EMPTY: "false" + # JSON array of {"repo","file","label"} — see the header comment for the + # update flow. Kept in a variable (not the file) so private caller names + # never land in this public repo. NOTE: this env binding is itself the + # known log-exposure gap documented in the header — Actions dumps the + # step env before the script's `::add-mask::` runs. + CALLERS_JSON: ${{ vars.DETECT_UNREVIEWED_MERGE_CALLERS }} + run: | + # The main-only ref guard above cannot catch a manual RE-RUN of an + # older main run: github.ref is refs/heads/main but github.sha is + # that run's original (now stale) commit, and the bumper would + # force-repin every caller to it. So establish the current main tip + # first; the guard below decides whether this run is genuinely stale. + # Don't pipe into `cut`: the pipeline would report cut's status, so a + # failed ls-remote (network blip, remote hiccup) yields an EMPTY + # main_tip that then compares unequal to github.sha and silently + # no-ops the whole fleet bump as if the run were stale. A lookup we + # couldn't perform is not evidence of staleness — fail loudly. + # `--refs` plus an exact-refname match, not just the first line: git + # matches ref patterns at component boundaries, so a branch literally + # named `foo/refs/heads/main` also matches this pattern and could be + # the line a bare `%%\t*` parse consumes. + if ! ls_remote=$(git ls-remote --refs origin refs/heads/main); then + echo "::error::Could not look up the current main tip (git ls-remote failed)" + exit 1 + fi + main_tip=$(awk '$2 == "refs/heads/main" { print $1; exit }' <<<"$ls_remote") + if [[ -z "$main_tip" ]]; then + echo "::error::git ls-remote returned no SHA for refs/heads/main" + exit 1 + fi + WATCHED=.github/workflows/detect-unreviewed-merge.yml + if [[ "$main_tip" != "$GITHUB_SHA" ]]; then + # main has moved on. That alone does NOT make this run stale: the + # push trigger is path-filtered to WATCHED, so an unrelated commit + # landing in the seconds between this run's trigger and this check + # starts NO run of its own. Skipping on a bare SHA mismatch would + # discard the only run for this change and leave every caller + # frozen — the exact pin-drift this fleet exists to prevent. + # What actually distinguishes a stale re-run is that WATCHED has + # CHANGED since: then a later commit did touch the path and does + # have its own run, which will pin the newer content. + if ! git fetch --depth=1 origin main; then + echo "::error::Could not fetch the current main tip to compare $WATCHED" + exit 1 + fi + # Prove FETCH_HEAD resolves to a real commit BEFORE reading blobs + # out of it. `git rev-parse --verify --quiet` returns empty both for + # "that path is absent from this tree" and for "this revision could + # not be resolved at all" (a partial fetch, an unexpected FETCH_HEAD + # state). Without this guard the second case is indistinguishable + # from deletion and would exit 0 as "decommissioned" — the same + # "a lookup we couldn't perform is not evidence" anti-pattern the + # ls-remote guard above rejects, but silently no-opping the whole + # fleet. With it, an empty tip_blob genuinely means absent-from-tree. + if ! main_tip=$(git rev-parse --verify --quiet "FETCH_HEAD^{commit}"); then + echo "::error::Fetched main but FETCH_HEAD does not resolve to a commit — cannot compare $WATCHED" + exit 1 + fi + tip_blob=$(git rev-parse --verify --quiet "FETCH_HEAD:$WATCHED" || true) + # HEAD is this run's own checkout, so it must resolve. An empty + # here_blob means $WATCHED is absent at github.sha, which is the + # deletion-commit case the final guard handles — don't let it fall + # into the "changed since" branch below and be reported as a stale + # re-run, which would be a misleading log for a real decommission. + if ! here_blob=$(git rev-parse --verify --quiet "HEAD:$WATCHED"); then + echo "::warning::$WATCHED is absent at this run's own commit $GITHUB_SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + exit 0 + fi + if [[ -z "$tip_blob" ]]; then + # ::warning:: not a bare echo: if the reusable was deleted while + # live callers still pin it, they all hard-fail at startup and a + # silently-green run here is the fleet's only chance to say so. + echo "::warning::$WATCHED no longer exists on main ($main_tip) — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + exit 0 + fi + if [[ "$tip_blob" != "$here_blob" ]]; then + echo "github.sha $GITHUB_SHA is behind main ($main_tip) and $WATCHED changed since — stale run/re-run; the newer commit has its own run. Nothing to bump" + exit 0 + fi + # Pin callers to the VERIFIED TIP, not to this run's stale + # github.sha. We have just proved $WATCHED is byte-identical at + # both, so the tip is the same reusable content at a commit that is + # actually current — pinning the older SHA would hand every caller a + # non-tip commit (and, on a land-then-revert, re-pin them backwards). + # + # COUPLED TO THE PATH FILTER — this is only sound because the `paths:` + # trigger has exactly ONE entry, so a single-blob comparison covers + # everything a caller pins at this SHA. If you ever widen that filter + # (an asset directory, a `workflows_ref` surface, as cursor-review / + # groom / pr-size already have), this comparison silently + # under-verifies: callers would be pinned to a tip whose other + # relevant content was never compared. Widen the comparison to every + # filtered path in the same change, or drop this re-point. + echo "main moved to $main_tip since $GITHUB_SHA, but $WATCHED is unchanged — this run is still the only one for that change; pinning callers to $main_tip and proceeding" + # `export` explicitly: the env: binding already marks it exported, + # but the script is a separate process and this must not depend on + # that inherited attribute surviving a reassignment. + export NEW_SHA="$main_tip" + fi + # The push path filter also matches a commit that DELETES the + # reusable workflow; bumping callers to a SHA where it is gone would + # break every caller. Deletion means decommissioning — no-op. + # Test "$WATCHED", not a second copy of the literal path: two literals + # drift apart on a rename, and the stale one would name a file that + # never exists, making this test always true and the whole fleet a + # permanent silent no-op. + if [[ ! -f "$WATCHED" ]]; then + echo "::warning::$WATCHED absent at this SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + exit 0 + fi + bash .github/bump-callers/bump-callers.sh diff --git a/.github/workflows/test-bump-callers.yml b/.github/workflows/test-bump-callers.yml index 387c489..31a5de3 100644 --- a/.github/workflows/test-bump-callers.yml +++ b/.github/workflows/test-bump-callers.yml @@ -3,8 +3,8 @@ name: Test bump-callers script # Runs the functional tests + shellcheck for the shared caller-bump script # (.github/bump-callers/bump-callers.sh). That one script drives the SHA-bump # fan-out for the cursor-review, cursor-review-auto-label, agents-md-integrity, -# pr-size, pr-risk, assign-reviewers AND groom caller fleets, so a regression here -# silently breaks every consumer +# pr-size, pr-risk, assign-reviewers, groom AND detect-unreviewed-merge caller fleets, +# so a regression here silently breaks every consumer # repo's pin auto-bump (or, worse, leaks a private caller name into this public # repo's run logs) — cheap to guard with a unit run on change. Mirrors # test-cursor-review-scripts.yml / test-agents-md-integrity.yml. @@ -20,6 +20,7 @@ on: - '.github/workflows/bump-assign-reviewers-callers.yml' - '.github/workflows/bump-groom-callers.yml' - '.github/workflows/bump-auto-label-callers.yml' + - '.github/workflows/bump-detect-unreviewed-merge-callers.yml' - '.github/workflows/test-bump-callers.yml' push: branches: [main] @@ -32,6 +33,7 @@ on: - '.github/workflows/bump-assign-reviewers-callers.yml' - '.github/workflows/bump-groom-callers.yml' - '.github/workflows/bump-auto-label-callers.yml' + - '.github/workflows/bump-detect-unreviewed-merge-callers.yml' - '.github/workflows/test-bump-callers.yml' permissions: diff --git a/AGENTS.md b/AGENTS.md index 3ca434d..568bc36 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,12 +104,15 @@ tests — run the matching command above for whatever you touched. - `bump-cursor-review-callers.yml` / `bump-auto-label-callers.yml` / `bump-agents-md-callers.yml` / `bump-pr-size-callers.yml` / `bump-pr-risk-callers.yml` / `bump-assign-reviewers-callers.yml` / - `bump-groom-callers.yml` — thin entrypoints over `bump-callers.sh` that fan SHA - bumps out to consumers. A groom or pr-risk caller pins TWICE (`uses:` + - `workflows_ref:`); the shared rewrite moves both, so never hand-bump one alone. - `stale.yml` and `assign-prs-to-author.yml` have no fleet because they have no - callers; `detect-unreviewed-merge.yml` has ~12 callers and no fleet — a known, - deferred gap, so its pins move by hand. + `bump-groom-callers.yml` / `bump-detect-unreviewed-merge-callers.yml` — thin + entrypoints over `bump-callers.sh` that fan SHA bumps out to consumers. A groom + or pr-risk caller pins TWICE (`uses:` + `workflows_ref:`); the shared rewrite + moves both, so never hand-bump one alone. `stale.yml` and + `assign-prs-to-author.yml` have no fleet because they have no callers; + `detect-unreviewed-merge.yml`'s fleet is + `bump-detect-unreviewed-merge-callers.yml`; its `DETECT_UNREVIEWED_MERGE_CALLERS` + roster is deliberately UNSEEDED (so it hard-fails) until the run-log masking + gap is closed — see the bump-callers README. - `bump-cursor-cli-pin.yml` — weekly PR moving `CURSOR_CLI_VERSION` / `CURSOR_CLI_SHA256` in `cursor-review.yml` (BE-5870). Not a caller bumper: merging it trips `bump-cursor-review-callers.yml`'s path filter, which rolls @@ -122,11 +125,11 @@ tests — run the matching command above for whatever you touched. - **Public repo — never leak private caller names.** Consumer repo lists live in repo **variables** — one per fleet (`CURSOR_REVIEW_CALLERS`, `AUTO_LABEL_CALLERS`, `AGENTS_MD_CALLERS`, `PR_SIZE_CALLERS`, - `PR_RISK_CALLERS`, `ASSIGN_REVIEWERS_CALLERS`, `GROOM_CALLERS`; the - bump-callers README table is canonical) — never hardcoded in a workflow file or - printed to run logs (logs are public). The bumper masks names it processes. - Keep private repo paths/detail out of workflow files, commit messages, and PR - text. + `PR_RISK_CALLERS`, `ASSIGN_REVIEWERS_CALLERS`, `GROOM_CALLERS`, + `DETECT_UNREVIEWED_MERGE_CALLERS`; the bump-callers README table is canonical) + — never hardcoded in a workflow file or printed to run logs (logs are public). + The bumper masks names it processes. Keep private repo paths/detail out of + workflow files, commit messages, and PR text. - **Pin everything by full commit SHA**, with a trailing `# v1` comment — both the `uses:` in callers and every third-party action here. Bare `@v1` fails the pin-validation (`pinact`, `zizmor`) that consumer CI runs. See README "Pinning".