[PR #271] [MERGED] feat(cursor-review-auto-label): optional bot_app_id + BOT_APP_PRIVATE_KEY pair #273

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:37:58 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12530-auto-label-bot-app-id

📝 Commits (2)

ed87601 feat(cursor-review-auto-label): optional bot_app_id + BOT_APP_PRIVATE_KEY pair
8e3660e fix(cursor-review-auto-label): make the App-pair selection fail closed

📊 Changes

3 files changed (+150 additions, -15 deletions)

View changed files

📝 .github/workflows/cursor-review-auto-label.yml (+109 -10)
📝 README.md (+1 -1)
📝 docs/callers/cursor-review-auto-label.md (+40 -4)

📄 Description

ELI-5

The auto-label workflow always minted its GitHub App token from one hardcoded
app (vars.APP_ID, the write-broad CLOUD_CODE_BOT). This adds an optional
second way in: set a bot_app_id input and pass the matching
BOT_APP_PRIVATE_KEY, and it mints from that narrower app instead. Say
nothing and everything behaves exactly as it does today.

What changed

All in .github/workflows/cursor-review-auto-label.yml (plus its caller doc):

New optional input bot_app_id (after runs_on), default ''. App IDs
aren't secret, so it's an input, not a secret — same shape as
cursor-review.yml and pr-size.yml already use in this repo.
New optional secret BOT_APP_PRIVATE_KEY, and
CLOUD_CODE_BOT_PRIVATE_KEY drops to required: false (re-described as
required only when bot_app_id is unset).
New preflight step, Refuse an incomplete or mixed App pair, ahead of
the mint. It enforces exactly one complete pair — every other shape exits
1 with an explicit ::error::: an id with no key, a key with no id, the
mix (bot_app_id alongside a CLOUD_CODE_BOT_PRIVATE_KEY mapping),
neither key, and an empty vars.APP_ID on the default path.
Two mutually exclusive mint steps gated on if: inputs.bot_app_id != ''
/ == '', each reading one pair with no || on the key side, and each
requesting permission-pull-requests: write — the only permission
gh pr edit --add-label needs.
Header comment documents both modes and carries a second example caller.
docs/callers/cursor-review-auto-label.md: prerequisites table marked
"Required unless bot_app_id is set", rows added for the new input and
secret, narrow-app caller snippet under "Caller", and the "Why the App token
is mandatory" section reworded (an App mint is still mandatory; only which
App is now optional). The root README.md catalog row got the same
one-clause sync, since it stated the old requirement as unconditional.

Why `vars.APP_ID` is resolved at the step, not as an input `default:`

vars in a reusable workflow resolves in the caller's context. Reading it
at the step is exactly where the old line read it, so the resolution path for
existing callers is untouched. A vars default is documented as legal but
neither sibling reusable here uses one.

Chesterton's Fence — the two lines this changes

Both required: true and the vars.APP_ID hardcode date to the workflow's
creation commit c84f75d (2026-06-02), and its own comment states the intent:
"the label must be applied by a GitHub App so it triggers the downstream review
run". That intent is some App credential being present, not that specific
app — the single-app hardcode was the original design, not a guard added to fix
a later bug. The preflight preserves the intent exactly: a run with no usable
credential still fails, loudly.

What genuinely changes is where it fails. Before: omitting the secrets:
mapping was a startup failure with zero jobs. Now: the run starts and fails at
step 1 with a message naming the missing key. Mapping a secret that happens to
be unset (the quieter case the caller docs call out) previously reached
create-github-app-token and hard-failed there with a generic error; it now
fails at the preflight with an actionable one. Neither case ever produced a
green run, before or after.

The credential selection, and why it fails closed

The first cut selected the key with one expression:

private-key: ${{ inputs.bot_app_id != '' && secrets.BOT_APP_PRIVATE_KEY || secrets.CLOUD_CODE_BOT_PRIVATE_KEY }}

GitHub's &&/|| return operands, not booleans, so a set bot_app_id with an
empty BOT_APP_PRIVATE_KEY evaluates the left side to '' and falls through to
the legacy key — silently minting from the write-broad app a caller explicitly
asked to avoid. The preflight made that unreachable, but the invariant then
lived in a different step, one if:/continue-on-error/reorder away from
being void. Four of six panel reviewers flagged exactly that.

It is now two mutually exclusive steps gated on bot_app_id, each reading a
single pair. There is no fallback operand to fall through to; the mint fails
closed by its own shape. The one remaining || is
steps.app-token-narrow.outputs.token || steps.app-token-default.outputs.token,
which selects between two same-purpose outputs of which exactly one ever exists
— not a credential fallback, and neither mint is continue-on-error, so a
failed mint fails the job before the label step.

app-id: ${{ inputs.bot_app_id || vars.APP_ID }} is likewise gone; each step
names its own app id literally.

Verification

Re-run on the review-fix commit, in a clean worktree:

actionlint .github/workflows/cursor-review-auto-label.yml — 0 findings
(actionlint shellchecks run: blocks, so the expanded preflight script is
covered).
zizmor --persona=regular .github/workflows/cursor-review-auto-label.yml —
no findings (1 suppressed). The github-app "blanket installation
permissions" finding CodeRabbit surfaced is resolved by the two
permission-pull-requests: write inputs.
python3 -m unittest discover -s .github/workflow-pins/tests — 316 tests,
OK; python3 .github/workflow-pins/check_workflow_pins.py — OK, 11
workflows, every ref checkout guarded. (test-workflow-pins.yml is the one CI
job path-filtered onto .github/workflows/**, so it is what this diff trips.)
bash .github/bump-callers/tests/test_bump_callers.sh — 437 passed, 0
failed (this file is referenced by name in that suite).
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed
(2 pre-existing warnings: AGENTS.md at 155 lines, no CODEOWNERS).
yaml.safe_load — parses; inputs still
[review_label, skip_label, runs_on, bot_app_id], both secrets still
required: false, steps now
[Refuse an incomplete or mixed App pair, Generate GitHub App token (narrow App), Generate GitHub App token (default CLOUD_CODE_BOT), Apply label ...].
permission-pull-requests: write sufficing for a label write is not assumed
from docs alone: pr-area-label.yml's label-pr job already applies a PR
label in this org holding exactly contents: read + pull-requests: write.

Residual

The five existing callers were never exercised. They live in private repos
this environment holds no access to, so "the default path still works" is
established by reading the rendered expressions, not by observing a run. The
first real proof will be the pin-bump PRs bump-auto-label-callers.yml opens
after this merges — somebody should confirm one auto-label run goes green
rather than assuming it.
secrets: inherit is now load-bearing against the mix check. A caller that
uses secrets: inherit and happens to hold a repo/org secret literally named
BOT_APP_PRIVATE_KEY will now hard-fail where it previously ran. That name is
introduced by this PR, so no current caller can be in that state, but a repo
that later adds the secret for another purpose would trip it. The alternative —
softening the invariant to "at least one" — is the fail-open the panel's High
finding was about, so the loud failure was taken deliberately; the ::error::
names secrets: inherit explicitly.
permission-pull-requests: write narrows the DEFAULT path too, not just the
new one. If a caller's CLOUD_CODE_BOT installation lacked that permission the
mint now fails where it used to succeed — but the subsequent
gh pr edit --add-label would have 403'd anyway, so the run was already red;
it now fails earlier and more legibly.
A caller that omits the secrets: block entirely used to fail at startup
consuming no runner; it now starts a job and fails at the preflight. Unchanged
from the first commit, restated because it is a real change to a failure mode.
The blocked follow-up this unblocks is not done here. Wiring the narrow app
into the comfy-cloud-mcp-server pull_request caller is a change in that
repo and out of scope; this PR only makes it expressible.
Commit messages omit the usual (BE-####) suffix AGENTS.md asks for,
because this repo is public and internal tracking references must stay out of
world-readable text. The branch name carries the autolink instead.

Provenance

Authored by: agent-work loop
Verified: actionlint (auto-label file): 0 findings; zizmor --persona=regular (auto-label file): no findings, 1 suppressed; test_bump_callers.sh: 437 passed, 0 failed; workflow-pins unittest: 316 tests OK + check_workflow_pins.py OK; check_agents_md.py: passed (2 pre-existing warnings); yaml.safe_load: parses
Deviations: none on the ticket's six acceptance items. Beyond them: the root README.md catalog-row sync, rewording the doc's "Why the App token is mandatory" paragraph, and the review-fix commit 8e3660e (fail-closed mint split, three added preflight arms, token permission narrowing) — the last is a response to the review panel + CodeRabbit, not to the ticket. See ## Residual for what remains unexercised.

_{🔄 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/271 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/8/2026 **Status:** ✅ Merged **Merged:** 9/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12530-auto-label-bot-app-id` --- ### 📝 Commits (2) - [`ed87601`](https://github.com/Comfy-Org/github-workflows/commit/ed8760103f98d3849695b2f6b61509f02c514efb) feat(cursor-review-auto-label): optional bot_app_id + BOT_APP_PRIVATE_KEY pair - [`8e3660e`](https://github.com/Comfy-Org/github-workflows/commit/8e3660e0d73fee637cc83dd2a92c75ef10453719) fix(cursor-review-auto-label): make the App-pair selection fail closed ### 📊 Changes **3 files changed** (+150 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/cursor-review-auto-label.yml` (+109 -10) 📝 `README.md` (+1 -1) 📝 `docs/callers/cursor-review-auto-label.md` (+40 -4) </details> ### 📄 Description ## ELI-5 The auto-label workflow always minted its GitHub App token from one hardcoded app (`vars.APP_ID`, the write-broad CLOUD_CODE_BOT). This adds an optional second way in: set a `bot_app_id` input and pass the matching `BOT_APP_PRIVATE_KEY`, and it mints from that narrower app instead. Say nothing and everything behaves exactly as it does today. ## What changed All in `.github/workflows/cursor-review-auto-label.yml` (plus its caller doc): 1. **New optional input `bot_app_id`** (after `runs_on`), default `''`. App IDs aren't secret, so it's an input, not a secret — same shape as `cursor-review.yml` and `pr-size.yml` already use in this repo. 2. **New optional secret `BOT_APP_PRIVATE_KEY`**, and `CLOUD_CODE_BOT_PRIVATE_KEY` drops to `required: false` (re-described as required only when `bot_app_id` is unset). 3. **New preflight step, `Refuse an incomplete or mixed App pair`**, ahead of the mint. It enforces *exactly one complete pair* — every other shape exits 1 with an explicit `::error::`: an id with no key, a key with no id, the **mix** (`bot_app_id` alongside a `CLOUD_CODE_BOT_PRIVATE_KEY` mapping), neither key, and an empty `vars.APP_ID` on the default path. 4. **Two mutually exclusive mint steps** gated on `if: inputs.bot_app_id != ''` / `== ''`, each reading one pair with no `||` on the key side, and each requesting `permission-pull-requests: write` — the only permission `gh pr edit --add-label` needs. 5. **Header comment** documents both modes and carries a second example caller. 6. **`docs/callers/cursor-review-auto-label.md`**: prerequisites table marked "Required unless `bot_app_id` is set", rows added for the new input and secret, narrow-app caller snippet under "Caller", and the "Why the App token is mandatory" section reworded (an App mint is still mandatory; only *which* App is now optional). The root `README.md` catalog row got the same one-clause sync, since it stated the old requirement as unconditional. ## Why `vars.APP_ID` is resolved at the step, not as an input `default:` `vars` in a reusable workflow resolves in the **caller's** context. Reading it at the step is exactly where the old line read it, so the resolution path for existing callers is untouched. A `vars` default is documented as legal but neither sibling reusable here uses one. ## Chesterton's Fence — the two lines this changes Both `required: true` and the `vars.APP_ID` hardcode date to the workflow's creation commit `c84f75d` (2026-06-02), and its own comment states the intent: "the label must be applied by a GitHub App so it triggers the downstream review run". That intent is *some* App credential being present, not *that specific* app — the single-app hardcode was the original design, not a guard added to fix a later bug. The preflight preserves the intent exactly: a run with no usable credential still fails, loudly. What genuinely changes is **where** it fails. Before: omitting the `secrets:` mapping was a startup failure with zero jobs. Now: the run starts and fails at step 1 with a message naming the missing key. Mapping a secret that happens to be *unset* (the quieter case the caller docs call out) previously reached `create-github-app-token` and hard-failed there with a generic error; it now fails at the preflight with an actionable one. Neither case ever produced a green run, before or after. ## The credential selection, and why it fails closed The first cut selected the key with one expression: ``` private-key: ${{ inputs.bot_app_id != '' && secrets.BOT_APP_PRIVATE_KEY || secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} ``` GitHub's `&&`/`||` return operands, not booleans, so a set `bot_app_id` with an empty `BOT_APP_PRIVATE_KEY` evaluates the left side to `''` and falls through to the legacy key — silently minting from the write-broad app a caller explicitly asked to avoid. The preflight made that unreachable, but the invariant then lived in a *different step*, one `if:`/`continue-on-error`/reorder away from being void. Four of six panel reviewers flagged exactly that. It is now two mutually exclusive steps gated on `bot_app_id`, each reading a single pair. There is no fallback operand to fall through to; the mint fails closed by its own shape. The one remaining `||` is `steps.app-token-narrow.outputs.token || steps.app-token-default.outputs.token`, which selects between two same-purpose outputs of which exactly one ever exists — not a credential fallback, and neither mint is `continue-on-error`, so a failed mint fails the job before the label step. `app-id: ${{ inputs.bot_app_id || vars.APP_ID }}` is likewise gone; each step names its own app id literally. ## Verification Re-run on the review-fix commit, in a clean worktree: - `actionlint .github/workflows/cursor-review-auto-label.yml` — **0 findings** (actionlint shellchecks `run:` blocks, so the expanded preflight script is covered). - `zizmor --persona=regular .github/workflows/cursor-review-auto-label.yml` — **no findings** (1 suppressed). The `github-app` "blanket installation permissions" finding CodeRabbit surfaced is resolved by the two `permission-pull-requests: write` inputs. - `python3 -m unittest discover -s .github/workflow-pins/tests` — **316 tests, OK**; `python3 .github/workflow-pins/check_workflow_pins.py` — **OK**, 11 workflows, every ref checkout guarded. (`test-workflow-pins.yml` is the one CI job path-filtered onto `.github/workflows/**`, so it is what this diff trips.) - `bash .github/bump-callers/tests/test_bump_callers.sh` — **437 passed, 0 failed** (this file is referenced by name in that suite). - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — **passed** (2 pre-existing warnings: AGENTS.md at 155 lines, no CODEOWNERS). - `yaml.safe_load` — parses; inputs still `[review_label, skip_label, runs_on, bot_app_id]`, both secrets still `required: false`, steps now `[Refuse an incomplete or mixed App pair, Generate GitHub App token (narrow App), Generate GitHub App token (default CLOUD_CODE_BOT), Apply label ...]`. - `permission-pull-requests: write` sufficing for a *label* write is not assumed from docs alone: `pr-area-label.yml`'s `label-pr` job already applies a PR label in this org holding exactly `contents: read` + `pull-requests: write`. ## Residual - **The five existing callers were never exercised.** They live in private repos this environment holds no access to, so "the default path still works" is established by reading the rendered expressions, not by observing a run. The first real proof will be the pin-bump PRs `bump-auto-label-callers.yml` opens after this merges — somebody should confirm one auto-label run goes green rather than assuming it. - **`secrets: inherit` is now load-bearing against the mix check.** A caller that uses `secrets: inherit` *and* happens to hold a repo/org secret literally named `BOT_APP_PRIVATE_KEY` will now hard-fail where it previously ran. That name is introduced by this PR, so no current caller can be in that state, but a repo that later adds the secret for another purpose would trip it. The alternative — softening the invariant to "at least one" — is the fail-open the panel's High finding was about, so the loud failure was taken deliberately; the `::error::` names `secrets: inherit` explicitly. - **`permission-pull-requests: write` narrows the DEFAULT path too**, not just the new one. If a caller's CLOUD_CODE_BOT installation lacked that permission the mint now fails where it used to succeed — but the subsequent `gh pr edit --add-label` would have 403'd anyway, so the run was already red; it now fails earlier and more legibly. - **A caller that omits the `secrets:` block entirely** used to fail at startup consuming no runner; it now starts a job and fails at the preflight. Unchanged from the first commit, restated because it is a real change to a failure mode. - **The blocked follow-up this unblocks is not done here.** Wiring the narrow app into the `comfy-cloud-mcp-server` `pull_request` caller is a change in that repo and out of scope; this PR only makes it expressible. - **Commit messages omit the usual `(BE-####)` suffix** `AGENTS.md` asks for, because this repo is public and internal tracking references must stay out of world-readable text. The branch name carries the autolink instead. ## Provenance - **Authored by:** agent-work loop - **Verified:** actionlint (auto-label file): 0 findings; zizmor --persona=regular (auto-label file): no findings, 1 suppressed; test_bump_callers.sh: 437 passed, 0 failed; workflow-pins unittest: 316 tests OK + check_workflow_pins.py OK; check_agents_md.py: passed (2 pre-existing warnings); yaml.safe_load: parses - **Deviations:** none on the ticket's six acceptance items. Beyond them: the root `README.md` catalog-row sync, rewording the doc's "Why the App token is mandatory" paragraph, and the review-fix commit `8e3660e` (fail-closed mint split, three added preflight arms, token permission narrowing) — the last is a response to the review panel + CodeRabbit, not to the ticket. See `## Residual` for what remains unexercised. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:58 +00:00