[PR #275] [MERGED] feat(groom): optional `environment` input on the three token-minting jobs #276

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12540-groom-environment-input

📝 Commits (3)

df9650d feat(groom): optional environment input on the three token-minting jobs
bef39ed fix(groom): gate the environment binding on bot_app_id and harden its guard
519036e docs(groom): stop asserting the untested missing-environment-secret case

📊 Changes

4 files changed (+282 additions, -1 deletions)

View changed files

➕ .github/groom/tests/test_environment_binding.py (+156 -0)
📝 .github/workflows/groom.yml (+76 -0)
📝 README.md (+1 -1)
📝 docs/callers/groom.md (+49 -0)

📄 Description

ELI-5

groom.yml mints a bot App token in three of its jobs, and today the private key that lets it do that has to be a repository secret — readable by a workflow on any branch. This adds an optional environment input so a caller can say "put those three jobs inside my bot-main environment", and then keep the key as an environment secret behind a deployment-branch policy instead. Leave it unset (the default) and absolutely nothing changes.

What changed

.github/workflows/groom.yml — new optional input environment (type: string, required: false, default: ''), declared next to bot_app_id, and bound as environment: ${{ inputs.environment }} on exactly three jobs: build_select, file and build_pr. Those are the only three that read secrets.BOT_APP_PRIVATE_KEY. The header's caller-pattern comment gains a commented environment: bot-main line.
.github/groom/tests/test_environment_binding.py (new) — a guard for the two properties that regress silently: the default really is '', and the binding set is exactly the token-minting set. See "Beyond the plan" below.
docs/callers/groom.md — an environment row in the inputs table plus a "Scoping the bot key to an environment" section covering the same-name requirement and how the caller passes the value.
README.md — one clause on the groom.yml catalog row.

Review round (second commit)

The binding is now conditional on bot_app_id: ${{ inputs.bot_app_id != '' && inputs.environment || '' }} on all three jobs. build_select and file mint the token under if: inputs.bot_app_id != '', so with no App configured there is no credential for an environment to guard, and binding one anyway parked those jobs behind protection rules for nothing — a denying rule would have dropped the run's findings after the audit was already billed. build_pr mints unconditionally and keeps the identical expression for uniformity; it is reachable in builder mode only, where build_select already rejects an empty bot_app_id, and the guard now asserts that dependency. This is a deliberate narrowing, not a pure no-op: setting environment with no bot_app_id previously bound anyway.
The guard is shape-based rather than literal. It matched the exact string ${{ secrets.BOT_APP_PRIVATE_KEY }} and ^ environment: at exactly four spaces, so a new credentialed job spelling the secret ${{secrets.…}} or aliasing it through a job-level env: fell out of the minting set and the bound set at once while every assertEqual stayed green. Now secrets\s*\.\s*BOT_APP_PRIVATE_KEY and ^\s+environment\s*:, scanned over the jobs: section so the loosened indentation does not also match the input declaration.
The constraints the input silently carried are now documented in the input description, the header caller-pattern comment, the README row and docs/callers/groom.md: dedicated environment only (binding injects all of its secrets into jobs including the patch-applying build_pr); deployment-branch policies only (a required-reviewer or wait-timer rule pauses build_select, and cancel-in-progress: false then parks every later tick behind it — build_pr raises one deployment per finding); create the environment and its policy first (a referenced-but-missing environment is auto-created with no rules, so a typo fails open); and cover every branch groom dispatches from, since a denied deployment loses the findings yet still advances the cadence clock.
An ordered migration sequence + the final caller mapping, with deleting the repository-level key as an explicit step 5 — the one that actually ends the exposure — and an inline comment explaining that BOT_APP_PRIVATE_KEY resolving to '' afterwards is the supported end state rather than a break.

The security boundary this deliberately does not cross

audit_find, audit_verify and build run a model over untrusted repository content and get no binding. Putting one of them inside a credentialed environment would hand an environment's secrets to the job that reads attacker-influenceable code — the exact separation groom's split-job topology exists to hold. gate gets none either (it mints no token). No deployment: false, keeping parity with the directly-gated jobs on the consumer side.

Why an empty environment name is safe for existing callers

An empty environment.name binds no environment. I verified that claim against a live public precedent rather than taking it on faith: fluent/fluent-bit's .github/workflows/call-build-images.yaml declares environment as required: false with no default and binds environment: ${{ inputs.environment }} on call-build-images-meta; its caller .github/workflows/pr-package-tests.yaml invokes that reusable and never passes the input. Run 29556928666 (pr-package-tests.yaml, 2026-07-17) shows all four instances of that job concluding success. Both files and the run were read back through the GitHub API for this PR, not quoted from the design note.

On top of that, no existing caller can pass the input — it did not exist until this commit — so the pin-bump PRs that bump-groom-callers.yml opens after this merges need no other change.

Beyond the plan

The plan asked only that the existing suites keep passing. I added test_environment_binding.py because the two failure modes here are invisible in review of a 3000-line workflow: a hardcoded environment name would break every caller at startup with no local signal, and an environment: line added to an agent job is one green line in a diff. It pins the input's default: '', that the minting set is exactly {build_select, file, build_pr}, that no agent job binds anything, that no other job does either, and that every binding is the input expression rather than a literal. Text-matched rather than parsed, matching test_interval.py's existing literal-pin idiom (this repo is stdlib-only — no PyYAML). I mutation-tested it: binding audit_verify, hardcoding bot-main, dropping the binding from file, and changing the default each turn it red.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 379 tests, OK (1 skipped); 374 before this change, +5 new.
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' + python3 .github/workflow-pins/check_workflow_pins.py — 316 tests OK; lint OK, 11 workflows checked.
actionlint .github/workflows/groom.yml — 16 findings, byte-identical in count and kind to origin/main's copy of the same file (all pre-existing job.workflow_sha false positives actionlint does not know about). No new finding.
zizmor --persona=regular .github/workflows/groom.yml — 8 findings (5 suppressed), same as origin/main's copy. No new finding.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed, 2 pre-existing warnings (AGENTS.md line count, no CODEOWNERS), untouched by this change.
The four suite-level guards were also re-run after the final wording edits, not only before them.

Residual

Nothing is gated by merging this. This is the reusable-workflow half only. The consumer-side change — creating the environment, moving the key into it as an environment secret, and passing environment: from the caller — is a separate follow-up, and until it lands the repository-level bot key is still what every run reads. That follow-up is what would let the repository secret be deleted.
No groom run was executed with the input set or unset. The empty-binding no-op is evidenced by a third-party public repo's completed run (above) and by static assertion, not by a groom run of our own — triggering one bills a full model audit, so it is unexercised by cost policy rather than by inability. The first real proof will be the next scheduled run on an already-enrolled caller after the pin bump; a reviewer who wants it earlier can workflow_dispatch a dry_run: true groom on a caller pinned at this branch.
The behavior of a non-empty environment is untested here for the same reason. In particular, if a caller sets environment: on an environment that carries required reviewers, all three credentialed jobs will wait for manual approval — the issue-filing job included, on the default finds-only path. That is inherent to environment protection rules and not something this input can opt out of. It is now called out — in the input description, the README row and the caller docs — together with the consequence review surfaced that is worse than the wait: a paused build_select holds the groom-<repo> concurrency group under cancel-in-progress: false, so every later daily tick queues behind it and is cancelled, stopping groom for that repo with no failure and no log. Documented rather than enforced: a reusable workflow cannot see the caller's environment protection rules.
Same-name secret substitution is documented, not asserted. The mechanism itself is no longer an inference: GitHub's Reuse workflows guide states it outright — "If you include environment in the reusable workflow at the job level, the environment secret will be used, and not the secret passed from the caller workflow." That quote is now carried in the input description and the caller docs. What remains unasserted is the name match: a differently-named environment secret fails silently, leaving the passed value in place. Nothing in this repo can detect that misconfiguration on the caller's behalf; the docs are the only guard.
Two related tickets could not be read. The parent epic and the originating investigation ticket that this change was written from were named to me but their bodies were not fetched and are not reachable from this environment, so anything they specify beyond what is implemented above is unverified against the source. The plan I worked from was reproduced in full, and I did not deviate from it apart from the added test.
The duplicate check behind this work was reported incomplete on the ticketing side (part of the prior-art search was truncated). The GitHub-side half was complete, and I confirmed the one open PR touching groom.yml (#261, fix(groom): bound finder exploration and spend) edits only the audit_find job — disjoint from every hunk here.

Provenance

Authored by: agent-work loop
Verified: groom scripts unittest: 380 passed, 0 failed (1 skipped); workflow-pins unittest: 316 passed, 0 failed; check_workflow_pins.py: OK, 11 workflows, 0 violations; actionlint on groom.yml: 16 findings, count and kind identical to origin/main's copy (all pre-existing job.workflow_sha false positives); zizmor --persona=regular on groom.yml: 8 findings (5 suppressed), identical to base; check_agents_md.py: passed with 2 pre-existing warnings. The hardened guard was differentially mutation-tested: a new credentialed job spelling the key ${{secrets.BOT_APP_PRIVATE_KEY}} through a job-level env: and binding nothing passes the previous test and fails the current one; binding an agent job at four and six spaces, hardcoding an environment name, dropping a binding, changing the default, dropping the bot_app_id guard, ungating the file mint step, and removing build_select's builder/bot_app_id validation each turn it red.
Deviations: none against the plan. Two additions beyond it: test_environment_binding.py (described under "Beyond the plan") and the review round above, whose one behaviour change — dropping the binding when bot_app_id is unset — narrows the input relative to the original plan and is flagged as such.

_{🔄 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/275 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/8/2026 **Status:** ✅ Merged **Merged:** 9/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12540-groom-environment-input` --- ### 📝 Commits (3) - [`df9650d`](https://github.com/Comfy-Org/github-workflows/commit/df9650dc1b8c300f2a9d4e72d115753f45170837) feat(groom): optional `environment` input on the three token-minting jobs - [`bef39ed`](https://github.com/Comfy-Org/github-workflows/commit/bef39ed88c591c3db0895febdcc38190aca646f0) fix(groom): gate the environment binding on bot_app_id and harden its guard - [`519036e`](https://github.com/Comfy-Org/github-workflows/commit/519036ebfd4b9d87d28c494eec8eb4fbf141b893) docs(groom): stop asserting the untested missing-environment-secret case ### 📊 Changes **4 files changed** (+282 additions, -1 deletions) <details> <summary>View changed files</summary> ➕ `.github/groom/tests/test_environment_binding.py` (+156 -0) 📝 `.github/workflows/groom.yml` (+76 -0) 📝 `README.md` (+1 -1) 📝 `docs/callers/groom.md` (+49 -0) </details> ### 📄 Description ## ELI-5 `groom.yml` mints a bot App token in three of its jobs, and today the private key that lets it do that has to be a **repository** secret — readable by a workflow on any branch. This adds an optional `environment` input so a caller can say "put those three jobs inside my `bot-main` environment", and then keep the key as an **environment** secret behind a deployment-branch policy instead. Leave it unset (the default) and absolutely nothing changes. ## What changed - **`.github/workflows/groom.yml`** — new optional input `environment` (`type: string`, `required: false`, `default: ''`), declared next to `bot_app_id`, and bound as `environment: ${{ inputs.environment }}` on exactly three jobs: `build_select`, `file` and `build_pr`. Those are the only three that read `secrets.BOT_APP_PRIVATE_KEY`. The header's caller-pattern comment gains a commented `environment: bot-main` line. - **`.github/groom/tests/test_environment_binding.py`** (new) — a guard for the two properties that regress silently: the default really is `''`, and the binding set is exactly the token-minting set. See "Beyond the plan" below. - **`docs/callers/groom.md`** — an `environment` row in the inputs table plus a "Scoping the bot key to an environment" section covering the same-name requirement and how the caller passes the value. - **`README.md`** — one clause on the `groom.yml` catalog row. ### Review round (second commit) - **The binding is now conditional on `bot_app_id`**: `${{ inputs.bot_app_id != '' && inputs.environment || '' }}` on all three jobs. `build_select` and `file` mint the token under `if: inputs.bot_app_id != ''`, so with no App configured there is no credential for an environment to guard, and binding one anyway parked those jobs behind protection rules for nothing — a denying rule would have dropped the run's findings after the audit was already billed. `build_pr` mints unconditionally and keeps the identical expression for uniformity; it is reachable in builder mode only, where `build_select` already rejects an empty `bot_app_id`, and the guard now asserts that dependency. **This is a deliberate narrowing**, not a pure no-op: setting `environment` with no `bot_app_id` previously bound anyway. - **The guard is shape-based rather than literal.** It matched the exact string `${{ secrets.BOT_APP_PRIVATE_KEY }}` and `^ environment:` at exactly four spaces, so a new credentialed job spelling the secret `${{secrets.…}}` or aliasing it through a job-level `env:` fell out of the minting set *and* the bound set at once while every `assertEqual` stayed green. Now `secrets\s*\.\s*BOT_APP_PRIVATE_KEY` and `^\s+environment\s*:`, scanned over the `jobs:` section so the loosened indentation does not also match the input declaration. - **The constraints the input silently carried are now documented** in the input description, the header caller-pattern comment, the README row and `docs/callers/groom.md`: dedicated environment only (binding injects *all* of its secrets into jobs including the patch-applying `build_pr`); deployment-branch policies only (a required-reviewer or wait-timer rule pauses `build_select`, and `cancel-in-progress: false` then parks every later tick behind it — `build_pr` raises one deployment *per finding*); create the environment and its policy first (a referenced-but-missing environment is auto-created with no rules, so a typo fails **open**); and cover every branch groom dispatches from, since a denied deployment loses the findings yet still advances the cadence clock. - **An ordered migration sequence + the final caller mapping**, with deleting the repository-level key as an explicit step 5 — the one that actually ends the exposure — and an inline comment explaining that `BOT_APP_PRIVATE_KEY` resolving to `''` afterwards is the supported end state rather than a break. ## The security boundary this deliberately does not cross `audit_find`, `audit_verify` and `build` run a model over untrusted repository content and get **no** binding. Putting one of them inside a credentialed environment would hand an environment's secrets to the job that reads attacker-influenceable code — the exact separation groom's split-job topology exists to hold. `gate` gets none either (it mints no token). No `deployment: false`, keeping parity with the directly-gated jobs on the consumer side. ## Why an empty environment name is safe for existing callers An empty `environment.name` binds no environment. I verified that claim against a live public precedent rather than taking it on faith: `fluent/fluent-bit`'s `.github/workflows/call-build-images.yaml` declares `environment` as `required: false` with **no default** and binds `environment: ${{ inputs.environment }}` on `call-build-images-meta`; its caller `.github/workflows/pr-package-tests.yaml` invokes that reusable and never passes the input. Run `29556928666` (`pr-package-tests.yaml`, 2026-07-17) shows all four instances of that job concluding `success`. Both files and the run were read back through the GitHub API for this PR, not quoted from the design note. On top of that, no existing caller *can* pass the input — it did not exist until this commit — so the pin-bump PRs that `bump-groom-callers.yml` opens after this merges need no other change. ## Beyond the plan The plan asked only that the existing suites keep passing. I added `test_environment_binding.py` because the two failure modes here are invisible in review of a 3000-line workflow: a hardcoded environment name would break every caller at startup with no local signal, and an `environment:` line added to an agent job is one green line in a diff. It pins the input's `default: ''`, that the minting set is exactly `{build_select, file, build_pr}`, that no agent job binds anything, that no *other* job does either, and that every binding is the input expression rather than a literal. Text-matched rather than parsed, matching `test_interval.py`'s existing literal-pin idiom (this repo is stdlib-only — no PyYAML). I mutation-tested it: binding `audit_verify`, hardcoding `bot-main`, dropping the binding from `file`, and changing the default each turn it red. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — 379 tests, OK (1 skipped); 374 before this change, +5 new. - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` + `python3 .github/workflow-pins/check_workflow_pins.py` — 316 tests OK; lint OK, 11 workflows checked. - `actionlint .github/workflows/groom.yml` — 16 findings, byte-identical in count and kind to `origin/main`'s copy of the same file (all pre-existing `job.workflow_sha` false positives actionlint does not know about). No new finding. - `zizmor --persona=regular .github/workflows/groom.yml` — 8 findings (5 suppressed), same as `origin/main`'s copy. No new finding. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed, 2 pre-existing warnings (AGENTS.md line count, no CODEOWNERS), untouched by this change. - The four suite-level guards were also re-run *after* the final wording edits, not only before them. ## Residual - **Nothing is gated by merging this.** This is the reusable-workflow half only. The consumer-side change — creating the environment, moving the key into it as an environment secret, and passing `environment:` from the caller — is a separate follow-up, and until it lands the repository-level bot key is still what every run reads. That follow-up is what would let the repository secret be deleted. - **No groom run was executed with the input set or unset.** The empty-binding no-op is evidenced by a third-party public repo's completed run (above) and by static assertion, not by a groom run of our own — triggering one bills a full model audit, so it is unexercised by cost policy rather than by inability. The first real proof will be the next scheduled run on an already-enrolled caller after the pin bump; a reviewer who wants it earlier can `workflow_dispatch` a `dry_run: true` groom on a caller pinned at this branch. - **The behavior of a *non-empty* environment is untested here** for the same reason. In particular, if a caller sets `environment:` on an environment that carries **required reviewers**, all three credentialed jobs will wait for manual approval — the issue-filing job included, on the default finds-only path. That is inherent to environment protection rules and not something this input can opt out of. It is **now called out** — in the input description, the README row and the caller docs — together with the consequence review surfaced that is worse than the wait: a paused `build_select` holds the `groom-<repo>` concurrency group under `cancel-in-progress: false`, so every later daily tick queues behind it and is cancelled, stopping groom for that repo with no failure and no log. Documented rather than enforced: a reusable workflow cannot see the caller's environment protection rules. - **Same-name secret substitution is documented, not asserted.** The mechanism itself is no longer an inference: GitHub's [Reuse workflows](https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows#using-inputs-and-secrets-in-a-reusable-workflow) guide states it outright — *"If you include `environment` in the reusable workflow at the job level, the environment secret will be used, and not the secret passed from the caller workflow."* That quote is now carried in the input description and the caller docs. What remains unasserted is the **name match**: a differently-named environment secret fails *silently*, leaving the passed value in place. Nothing in this repo can detect that misconfiguration on the caller's behalf; the docs are the only guard. - **Two related tickets could not be read.** The parent epic and the originating investigation ticket that this change was written from were named to me but their bodies were not fetched and are not reachable from this environment, so anything they specify beyond what is implemented above is unverified against the source. The plan I worked from was reproduced in full, and I did not deviate from it apart from the added test. - **The duplicate check behind this work was reported incomplete** on the ticketing side (part of the prior-art search was truncated). The GitHub-side half was complete, and I confirmed the one open PR touching `groom.yml` (#261, `fix(groom): bound finder exploration and spend`) edits only the `audit_find` job — disjoint from every hunk here. ## Provenance - **Authored by:** agent-work loop - **Verified:** groom scripts `unittest`: 380 passed, 0 failed (1 skipped); workflow-pins `unittest`: 316 passed, 0 failed; `check_workflow_pins.py`: OK, 11 workflows, 0 violations; `actionlint` on groom.yml: 16 findings, count and kind identical to `origin/main`'s copy (all pre-existing `job.workflow_sha` false positives); `zizmor --persona=regular` on groom.yml: 8 findings (5 suppressed), identical to base; `check_agents_md.py`: passed with 2 pre-existing warnings. The hardened guard was differentially mutation-tested: a new credentialed job spelling the key `${{secrets.BOT_APP_PRIVATE_KEY}}` through a job-level `env:` and binding nothing passes the previous test and fails the current one; binding an agent job at four *and* six spaces, hardcoding an environment name, dropping a binding, changing the default, dropping the `bot_app_id` guard, ungating the `file` mint step, and removing `build_select`'s builder/`bot_app_id` validation each turn it red. - **Deviations:** none against the plan. Two additions beyond it: `test_environment_binding.py` (described under "Beyond the plan") and the review round above, whose one behaviour change — dropping the binding when `bot_app_id` is unset — narrows the input relative to the original plan and is flagged as such. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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