From df9650dc1b8c300f2a9d4e72d115753f45170837 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 11:36:43 -0700 Subject: [PATCH 1/3] feat(groom): optional `environment` input on the three token-minting jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Give `groom.yml` an optional `environment` input and bind it on `build_select`, `file` and `build_pr` — the only jobs that read `secrets.BOT_APP_PRIVATE_KEY`. A caller can then hold the bot App key as an ENVIRONMENT secret behind a deployment-branch policy instead of a repository secret every branch can read. The default is `''`, which binds no environment, so every existing caller is unaffected — none of them pass the input and none can. The agent jobs (`audit_find`, `audit_verify`, `build`) deliberately do NOT bind it: they run a model over untrusted repository content and must stay outside any credentialed environment. That boundary is now pinned by a unit guard (`test_environment_binding.py`) rather than by a comment, along with the empty default and the "never a hardcoded environment name" property. Docs: caller-pattern comment in the workflow header, an inputs row plus a "Scoping the bot key to an environment" section in docs/callers/groom.md, and a clause on the README catalog row. --- .../groom/tests/test_environment_binding.py | 104 ++++++++++++++++++ .github/workflows/groom.yml | 39 +++++++ README.md | 2 +- docs/callers/groom.md | 10 ++ 4 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 .github/groom/tests/test_environment_binding.py diff --git a/.github/groom/tests/test_environment_binding.py b/.github/groom/tests/test_environment_binding.py new file mode 100644 index 00000000..476fa54e --- /dev/null +++ b/.github/groom/tests/test_environment_binding.py @@ -0,0 +1,104 @@ +"""The optional `environment` input, and the boundary it must never cross. + +`groom.yml` lets a caller bind a GitHub environment so the bot App key can be an +ENVIRONMENT secret behind a deployment-branch policy instead of a repository +secret every branch can read. Two properties make that safe, and both are +invisible in review once the file is 3000 lines long: + + * the default is `''` — an empty environment name binds nothing, so the eight + existing callers, none of which pass the input, keep today's behavior; and + * only the jobs that MINT the bot token bind it. The finder / verifier / + builder jobs run a model over untrusted repository content, and putting one + of those inside a credentialed environment is exactly the boundary the + split-job topology exists to hold. + +Both regress silently: a hardcoded environment name breaks every caller at +startup with no local signal, and an `environment:` added to an agent job is one +green line in a diff. Asserted as text rather than parsed — PyYAML is not stdlib +and this repo is stdlib-only (same reasoning as test_interval.py's literal pins). +""" + +import os +import re +import unittest + +BINDING = "environment: ${{ inputs.environment }}" +SECRET = "${{ secrets.BOT_APP_PRIVATE_KEY }}" +# The jobs that run an agent over untrusted repo content. Named explicitly, not +# derived, so deleting the binding from a credentialed job cannot silently +# shrink this set too. +AGENT_JOBS = ("audit_find", "audit_verify", "build") + + +def _workflow_text(): + wf = os.path.join(os.path.dirname(__file__), "..", "..", "workflows", "groom.yml") + with open(wf, encoding="utf-8") as f: + return f.read() + + +def _job_blocks(text): + """Map job name -> that job's block, from `jobs:` to EOF.""" + body = text.split("\njobs:\n", 1)[1] + blocks = {} + for block in re.split(r"(?m)^ (?=[A-Za-z_][A-Za-z0-9_-]*:\s*$)", body): + name = block.split(":", 1)[0].strip() + if name and re.fullmatch(r"[A-Za-z_][A-Za-z0-9_-]*", name): + blocks[name] = block + return blocks + + +class EnvironmentInputTest(unittest.TestCase): + def setUp(self): + self.text = _workflow_text() + self.jobs = _job_blocks(self.text) + + def test_the_input_is_optional_and_defaults_to_empty(self): + # The whole no-op-for-existing-callers claim rests on this default. + decl = re.search( + r"(?ms)^ environment:\n(.*?)(?=^ [A-Za-z_])", self.text + ) + self.assertIsNotNone(decl, "no `environment:` input declared in groom.yml") + body = decl.group(1) + self.assertIn("type: string", body) + self.assertIn("required: false", body) + self.assertRegex(body, r"(?m)^ default: ''$") + + def test_every_job_that_mints_the_bot_token_binds_the_environment(self): + minting = sorted(n for n, b in self.jobs.items() if SECRET in b) + self.assertEqual( + minting, ["build_pr", "build_select", "file"], + "the set of jobs reading the bot App key changed — re-check the binding", + ) + for name in minting: + with self.subTest(job=name): + self.assertIn(BINDING, self.jobs[name]) + + def test_no_agent_job_sits_inside_a_credentialed_environment(self): + # The security boundary: a job that reads untrusted repo content with a + # model must never be able to reach an environment's secrets. + for name in AGENT_JOBS: + with self.subTest(job=name): + self.assertIn(name, self.jobs, "agent job missing from groom.yml") + self.assertNotRegex( + self.jobs[name], r"(?m)^ environment:", + "an agent job must not bind a GitHub environment", + ) + + def test_only_the_minting_jobs_bind_anything_at_all(self): + # The converse of the two above, so a binding added to a NEW uncredentialed + # job (a future gate, a summary job) is caught rather than assumed benign. + bound = sorted( + n for n, b in self.jobs.items() if re.search(r"(?m)^ environment:", b) + ) + self.assertEqual(bound, ["build_pr", "build_select", "file"]) + + def test_the_binding_is_the_input_and_never_a_hardcoded_name(self): + # A literal name here would bind an environment that does not exist in a + # caller's repo, failing every existing caller at startup. + for line in re.findall(r"(?m)^ environment:.*$", self.text): + with self.subTest(line=line): + self.assertEqual(line.strip(), BINDING) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 56577c10..1f924ab3 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -114,6 +114,13 @@ name: Groom (reusable) # with: # # Post/act issues as cloud-code-bot instead of github-actions[bot]. # bot_app_id: ${{ vars.APP_ID }} +# # OPTIONAL: bind a GitHub environment on the three credentialed jobs +# # (build_select / file / build_pr) so the bot App key can live as an +# # ENVIRONMENT secret with a deployment-branch policy instead of a +# # repository secret. The environment must hold a secret named exactly +# # BOT_APP_PRIVATE_KEY. It is an ordinary `with:` string input — pass +# # the name (or a `vars.` expression), never a secret. +# # environment: bot-main # # No workflows_ref: the briefs + ledger auto-load from the same commit # # this `uses:` pin resolves to (job.workflow_sha — the runner v2.334.0+ # # accessor, not the empty `github.job_workflow_sha`). Set it only to @@ -393,6 +400,23 @@ on: type: string required: false default: '' + environment: + description: >- + GitHub environment (in the CALLER repo) that the three credentialed + jobs (build_select, file, build_pr) bind. Lets a caller keep + BOT_APP_PRIVATE_KEY as an ENVIRONMENT secret with a main-only + deployment-branch policy instead of a repository secret readable by + every branch. Empty (the default) binds no environment — existing + callers are unaffected. Because the caller's `secrets:` mapping is + evaluated in the CALLER job (which cannot itself carry + `environment:`), the environment must hold a secret named exactly + BOT_APP_PRIVATE_KEY; GitHub substitutes it for the passed value when + this job binds the environment. Never applied to the agent jobs — + they read untrusted repo content and must not sit inside a + credentialed environment. + type: string + required: false + default: '' builder: description: >- Opt-in AUTO-BUILDER ("split C", BE-4003). When true, the top @@ -1975,6 +1999,11 @@ jobs: needs: [gate, audit_verify] if: needs.gate.outputs.should_run == 'true' && needs.audit_verify.outputs.have_findings == 'true' runs-on: ubuntu-latest + # Optional caller-side gate on the bot App key: '' (the default) binds no + # environment, so existing callers are unaffected. Bound ONLY by the jobs + # that mint the bot token — never by the agent jobs, which run a model over + # untrusted repo content and must stay outside a credentialed environment. + environment: ${{ inputs.environment }} timeout-minutes: 15 permissions: contents: read @@ -2195,6 +2224,11 @@ jobs: needs: [gate, build_select] if: needs.gate.outputs.should_run == 'true' && needs.build_select.outputs.have_file == 'true' runs-on: ubuntu-latest + # Optional caller-side gate on the bot App key: '' (the default) binds no + # environment, so existing callers are unaffected. Bound ONLY by the jobs + # that mint the bot token — never by the agent jobs, which run a model over + # untrusted repo content and must stay outside a credentialed environment. + environment: ${{ inputs.environment }} timeout-minutes: 15 permissions: contents: read @@ -2936,6 +2970,11 @@ jobs: # never uploaded its result simply fails its own build_pr cell (fail-fast off). if: ${{ !cancelled() && needs.gate.outputs.should_run == 'true' && needs.build_select.outputs.have_build == 'true' }} runs-on: ubuntu-latest + # Optional caller-side gate on the bot App key: '' (the default) binds no + # environment, so existing callers are unaffected. Bound ONLY by the jobs + # that mint the bot token — never by the agent jobs, which run a model over + # untrusted repo content and must stay outside a credentialed environment. + environment: ${{ inputs.environment }} # 15 -> 20. Headroom only, and deliberately the LAST line of defence rather # than the fix: the 2026-08-04 cancellation was a hung fetch that produced # zero bytes, so a bigger budget alone would just have bought a longer hang. diff --git a/README.md b/README.md index 46dd46a5..9ba48ee3 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ complete, copy-pasteable caller. | [`pr-derisk.yml`](.github/workflows/pr-derisk.yml) | **On-demand de-risk split plan (`/derisk`, beta)** — **off by default** (`enabled: false`; `vars.DERISK_CONFIG` on the caller repo outranks it in both directions, so `{"enabled": false}` is a no-PR kill switch). Someone with write access comments `/derisk` on a pull request; the workflow re-grades that PR with the pr-risk grader, makes **one** model call for a semantic partition of the diff into a chain of 2–5 **sequential** PRs, and posts **one sticky advisory comment**: a verdict line and a chain table above the fold, the plan, the chain-landing rules and the floor math inside collapsed `
`. **Every floor shown is computed by `grade-pr-risk.sh --stdin`** over a synthetic record built from each step's files — the model proposes *which files go together* and nothing else, and any tier it writes is discarded before rendering. The partition must cover the changed-file set **exactly**; a missing or duplicated path is rejected, re-prompted **once**, then falls back. **Nothing is gated, routed, merged, labelled or filed** — filing tickets from a plan is a later rung behind its own command. Honest by construction: when no step lands below the PR's **path floor** the verdict reads "N smaller single-concern R3s, same lane", never a fake lane win, because the verdict is arithmetic over grader output rather than the model's prose — and the comparison is against the path floor rather than the headline tier precisely because a split only moves the path axis, so a fork PR (provenance R3, path floor R0) is told the truth instead of a reduction no partition can deliver. An over-budget diff, an unvalidatable partition and an API failure each post an explaining comment — a `/derisk` that quietly does nothing is never an outcome. Safe as a comment command because an `issue_comment` workflow runs from the caller's **default branch** (a PR cannot edit the workflow serving it), the commenter is gated on `author_association` (`allowed_associations`, default `OWNER,MEMBER,COLLABORATOR` — narrow it, never widen), and **no PR code is ever checked out**: the diff is read over the API and handed to a model as text. `workflows_ref` is **required** and its shape + ancestry are enforced by the guard byte-identical to `pr-risk.yml`'s (see that row for the full rationale and its two residuals); a test fails the build if the copies drift. The calling job needs `contents: read` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`, and the `anthropic_api_key` secret. | [pr-derisk.md](docs/callers/pr-derisk.md) | | [`pr-area-label.yml`](.github/workflows/pr-area-label.yml) | **Agentic PR area labeling** — classifies each PR into exactly one `area:*` label, and (on push to the consumer's default branch) syncs the repo's `area:*` labels to the taxonomy. The taxonomy is the consumer's own `.github/area-labels.yml` (`repo_context` + `labels[]` with `name`/`color`/`description`/optional `guidance`); the shared workflow carries nothing repo-specific. An LLM makes the domain-vs-path judgement a static `paths:` map can't, but with **no tools and no token**: PR title/body/paths/labels go to the Anthropic Messages API as data inside `` tags (diff excluded), the reply is enum-constrained by a JSON schema to the taxonomy's own names, and a deterministic step applies it with targeted `area:*` add/remove ops (never a full-set PUT, so concurrent non-area edits survive). The taxonomy is read from the PR's **base ref** — a PR can't rewrite the rules that classify it — and validated (unique `area:[a-z0-9-]+` names, every label resolves to a non-blank routing guide) before it drives a write; everything fails soft rather than failing the check. Classifier logic lives in [`scripts/area-label/`](scripts/area-label), loaded from the pinned `workflows_ref` (validated to a full 40-hex SHA before checkout). Fork/Dependabot PRs are skipped by construction (no writable token, no secret). Labels ride the plain `GITHUB_TOKEN` (no `labeled`-trigger cascade). The calling job needs `contents: read` + `issues: write` + `pull-requests: write`; the `ANTHROPIC_API_KEY` secret is provided org-wide (unset ⇒ `label-pr` fails soft, sync still works). | [pr-area-label.md](docs/callers/pr-area-label.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | -| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | +| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set); the optional `environment` input binds a GitHub environment on just the three token-minting jobs (`build_select` / `file` / `build_pr`, never the agent jobs) so that key can be an environment secret behind a deployment-branch policy rather than a repository secret — empty (the default) binds nothing and is byte-for-byte today's behavior, and the environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`. **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | | [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `SHA-256`/`AES-192` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`), and for each one the scan covers its CONTENTS, its tracked PATH and (for a symlink) its target string — the same matcher and allowlists on all three, so a private repo name in a directory component is a finding just as it is in a line of prose; binaries are skipped for their contents but still have their path scanned; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | | [`coderabbit-config-validate.yml`](.github/workflows/coderabbit-config-validate.yml) | Validates the caller repo's `.coderabbit.yaml` against CodeRabbit's config schema, **on the PR that breaks it**. CodeRabbit rejects an invalid config *whole* — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter and path instruction in it goes silently inert — and it validates the **base** branch, not the PR head, so the breakage first surfaces on the *next* PR, blamed on a change that did not cause it. Severity mirrors CodeRabbit's own behaviour: a YAML parse error, a `maxLength` violation or a type/enum error **fails**; an unknown/additional property **warns** (CodeRabbit strips those rather than rejecting the file, so the config loads while everything under the key silently does nothing) unless `strict_unknown_keys: true`. A repo with no `.coderabbit.yaml` passes, and says so. The schema is **vendored** at [`.github/coderabbit-config/`](.github/coderabbit-config) — no network fetch on the validation path, so an upstream schema change cannot redden the fleet without a reviewed PR here; `refresh-coderabbit-schema.yml` opens that PR weekly when upstream drifts. Setup: [docs/callers/coderabbit-config-validate.md](docs/callers/coderabbit-config-validate.md). Pin `workflows_ref` to the same ref as `uses:`; no secrets required. | diff --git a/docs/callers/groom.md b/docs/callers/groom.md index 9c4d0fce..01303f56 100644 --- a/docs/callers/groom.md +++ b/docs/callers/groom.md @@ -116,10 +116,20 @@ The ones that matter: | `scope_label` / `scope_desc` | `whole-repo` | Cosmetic labels for the scope in issue bodies. | | `workflows_ref` | `''` | **Leaving it unset is safe.** Alone among these workflows groom does not *require* it — it defaults to `''` and each asset checkout falls back to `${{ job.workflow_sha }}`, the commit your `uses:` pin resolved to, so the briefs, `ledger.py` and `interval.py` always match the logic running them with nothing to keep in sync. Set it only to test briefs from a branch. Before BE-8077 that fallback was spelled `github.job_workflow_sha` and silently loaded the assets from this repo's default branch — see the footgun below. | | `bot_app_id` | `''` | File as your App rather than `github-actions[bot]`. | +| `environment` | `''` | Bind a GitHub environment (in YOUR repo) on the three jobs that mint the bot App token — `build_select`, `file`, `build_pr` — so `BOT_APP_PRIVATE_KEY` can be an environment secret behind a deployment-branch policy instead of a repository secret every branch can read. Empty (the default) binds nothing. See "Scoping the bot key to an environment" below. | | `builder` | `false` | Opt into PR-writing — see below. | | `max_prs` | `'5'` | Only with `builder: true`. Typed **string**, deliberately. | | `pr_size_limit` | `400` | Only with `builder: true`. Caps a built PR's diff. | +## Scoping the bot key to an environment + +By default `BOT_APP_PRIVATE_KEY` is a **repository** secret, which any workflow on any branch of your repo can read. Set `environment: bot-main` (any environment name) and the three credentialed jobs — `build_select`, `file` and `build_pr`, the only ones that mint the bot App token — bind that environment, so you can hold the key as an **environment** secret with a `main`-only deployment-branch policy instead. The agent jobs (`audit_find`, `audit_verify`, `build`) deliberately never bind it: they run a model over untrusted repo content and must stay outside any credentialed environment. + +Two things to get right: + +- **The environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`.** Your caller's `secrets:` mapping is evaluated in the *caller* job, which cannot itself carry `environment:`, so the value you pass through is whatever the caller could see. GitHub substitutes the environment's same-named secret for it once the nested job binds the environment — that name match is the whole mechanism, so a differently-named environment secret silently leaves the passed (or empty) value in place. +- **Pass the environment as a plain string** under `with:`, e.g. `environment: bot-main`. It is an ordinary `type: string` input (a `${{ vars.* }}` expression works too) — it is *not* a secret and must not be routed through `secrets:`. Leaving it unset is the existing behavior, unchanged. + ## Opt-in auto-builder With `builder: true`, the top `max_prs` CONFIRMED, non-security findings become From bef39ed88c591c3db0895febdcc38190aca646f0 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 18:15:29 -0700 Subject: [PATCH 2/3] fix(groom): gate the environment binding on bot_app_id and harden its guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the optional `environment` input. - Bind only when `bot_app_id` is set. `build_select` and `file` mint the bot token under `if: inputs.bot_app_id != ''`, so without an App there is no credential for an environment to guard; binding one anyway parked those jobs behind protection rules for nothing, and a denying rule would drop the run's findings after the audit had already been billed. `build_pr` keeps the same expression for uniformity — it is reachable in builder mode only, where `build_select` already rejects an empty `bot_app_id`. - Make the guard 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:` dropped out of BOTH the minting set and the bound set at once while every assertEqual stayed green — demonstrated: the previous test passes on that mutation, this one fails on it. - Document the constraints the input silently carries: use a DEDICATED environment (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 the `cancel-in-progress: false` concurrency group then parks every later tick behind it); 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 `interval_days` cadence clock. - Spell out the migration sequence and the final caller `secrets:` mapping, with deleting the repository-level key as an explicit ordered step — the one that actually ends the exposure. Cite GitHub's documented precedence for the substitution the feature rests on ("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"). --- .../groom/tests/test_environment_binding.py | 76 ++++++++++++++++--- .github/workflows/groom.yml | 59 +++++++++++--- README.md | 2 +- docs/callers/groom.md | 45 +++++++++-- 4 files changed, 153 insertions(+), 29 deletions(-) diff --git a/.github/groom/tests/test_environment_binding.py b/.github/groom/tests/test_environment_binding.py index 476fa54e..4fe23e14 100644 --- a/.github/groom/tests/test_environment_binding.py +++ b/.github/groom/tests/test_environment_binding.py @@ -16,18 +16,32 @@ startup with no local signal, and an `environment:` added to an agent job is one green line in a diff. Asserted as text rather than parsed — PyYAML is not stdlib and this repo is stdlib-only (same reasoning as test_interval.py's literal pins). + +The two matchers below are deliberately SHAPE-based rather than exact strings. +An earlier draft tested for the literal `${{ secrets.BOT_APP_PRIVATE_KEY }}` and +for `^ environment:` at exactly four spaces, which meant a benign rewrite — +`${{secrets.BOT_APP_PRIVATE_KEY}}` with the spaces closed up, the key aliased +through a job-level `env:`, or a job re-indented — silently dropped the job out +of BOTH the minting set and the bound set at once, leaving every assertEqual +green while the property they exist to protect was gone. """ import os import re import unittest -BINDING = "environment: ${{ inputs.environment }}" -SECRET = "${{ secrets.BOT_APP_PRIVATE_KEY }}" +BINDING = "environment: ${{ inputs.bot_app_id != '' && inputs.environment || '' }}" +# Any reference to the bot App key, however the expression is spaced or wrapped, +# and wherever in the job it appears (a step's `with:`, or a job-level `env:`). +SECRET_RE = re.compile(r"secrets\s*\.\s*BOT_APP_PRIVATE_KEY") +# A job-level `environment:` key at ANY indentation — YAML does not require the +# two-space-per-level style this file happens to use. +ENV_KEY_RE = re.compile(r"(?m)^\s+environment\s*:") # The jobs that run an agent over untrusted repo content. Named explicitly, not # derived, so deleting the binding from a credentialed job cannot silently # shrink this set too. AGENT_JOBS = ("audit_find", "audit_verify", "build") +MINTING_JOBS = ["build_pr", "build_select", "file"] def _workflow_text(): @@ -36,9 +50,15 @@ def _workflow_text(): return f.read() +def _jobs_section(text): + """Everything from `jobs:` to EOF — excludes the `on:` input declaration, + which is also spelled `environment:` and would otherwise match ENV_KEY_RE.""" + return text.split("\njobs:\n", 1)[1] + + def _job_blocks(text): """Map job name -> that job's block, from `jobs:` to EOF.""" - body = text.split("\njobs:\n", 1)[1] + body = _jobs_section(text) blocks = {} for block in re.split(r"(?m)^ (?=[A-Za-z_][A-Za-z0-9_-]*:\s*$)", body): name = block.split(":", 1)[0].strip() @@ -64,9 +84,9 @@ def test_the_input_is_optional_and_defaults_to_empty(self): self.assertRegex(body, r"(?m)^ default: ''$") def test_every_job_that_mints_the_bot_token_binds_the_environment(self): - minting = sorted(n for n, b in self.jobs.items() if SECRET in b) + minting = sorted(n for n, b in self.jobs.items() if SECRET_RE.search(b)) self.assertEqual( - minting, ["build_pr", "build_select", "file"], + minting, MINTING_JOBS, "the set of jobs reading the bot App key changed — re-check the binding", ) for name in minting: @@ -79,26 +99,58 @@ def test_no_agent_job_sits_inside_a_credentialed_environment(self): for name in AGENT_JOBS: with self.subTest(job=name): self.assertIn(name, self.jobs, "agent job missing from groom.yml") - self.assertNotRegex( - self.jobs[name], r"(?m)^ environment:", + self.assertIsNone( + ENV_KEY_RE.search(self.jobs[name]), "an agent job must not bind a GitHub environment", ) def test_only_the_minting_jobs_bind_anything_at_all(self): # The converse of the two above, so a binding added to a NEW uncredentialed # job (a future gate, a summary job) is caught rather than assumed benign. - bound = sorted( - n for n, b in self.jobs.items() if re.search(r"(?m)^ environment:", b) - ) - self.assertEqual(bound, ["build_pr", "build_select", "file"]) + bound = sorted(n for n, b in self.jobs.items() if ENV_KEY_RE.search(b)) + self.assertEqual(bound, MINTING_JOBS) def test_the_binding_is_the_input_and_never_a_hardcoded_name(self): # A literal name here would bind an environment that does not exist in a # caller's repo, failing every existing caller at startup. - for line in re.findall(r"(?m)^ environment:.*$", self.text): + lines = re.findall(r"(?m)^\s+environment\s*:.*$", _jobs_section(self.text)) + self.assertEqual(len(lines), len(MINTING_JOBS)) + for line in lines: with self.subTest(line=line): self.assertEqual(line.strip(), BINDING) + def test_the_binding_is_dropped_when_no_bot_app_is_configured(self): + # With no App configured there is no credential for an environment to + # guard, and binding one anyway would park the job behind protection + # rules for nothing — a denying rule would then drop the run's findings + # after the audit had already been billed. The BINDING constant above + # already pins the guarded expression on all three jobs; this test pins + # the invariant that makes dropping it correct. + # + # `build_select` and `file` mint OPTIONALLY, so their mint step carries + # the same `bot_app_id != ''` condition the binding does. + for name in ("build_select", "file"): + with self.subTest(job=name): + mint = re.search( + r"(?ms)^ - name: Mint bot-identity token.*?(?=^ - name: )", + self.jobs[name], + ) + self.assertIsNotNone(mint, "no mint step found") + self.assertRegex( + mint.group(0), r"if: \$\{\{ inputs\.bot_app_id != '' \}\}" + ) + # `build_pr` mints UNCONDITIONALLY, which is safe only because it is + # reachable in builder mode alone and `build_select` hard-fails a + # `builder: true` run that set no bot_app_id. If that validation ever + # goes away, build_pr's unguarded mint becomes reachable with an empty + # key and the dropped binding stops being a no-op. + self.assertIn( + 'if [ "$BUILDER" = "true" ] && [ -z "$BOT_APP_ID" ]; then', + self.jobs["build_select"], + "build_select no longer rejects builder:true without bot_app_id — " + "build_pr's unconditional mint depends on that check", + ) + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 1f924ab3..40b890d3 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -119,7 +119,12 @@ name: Groom (reusable) # # ENVIRONMENT secret with a deployment-branch policy instead of a # # repository secret. The environment must hold a secret named exactly # # BOT_APP_PRIVATE_KEY. It is an ordinary `with:` string input — pass -# # the name (or a `vars.` expression), never a secret. +# # the name (or a `vars.` expression), never a secret. Create it (and +# # its branch policy) BEFORE setting this — a missing environment is +# # auto-created with no rules, so a typo fails open. Use a DEDICATED +# # environment holding only this key, and branch policies only: a +# # required-reviewer or wait-timer rule stalls groom permanently. +# # See docs/callers/groom.md, "Scoping the bot key to an environment". # # environment: bot-main # # No workflows_ref: the briefs + ledger auto-load from the same commit # # this `uses:` pin resolves to (job.workflow_sha — the runner v2.334.0+ @@ -407,13 +412,31 @@ on: BOT_APP_PRIVATE_KEY as an ENVIRONMENT secret with a main-only deployment-branch policy instead of a repository secret readable by every branch. Empty (the default) binds no environment — existing - callers are unaffected. Because the caller's `secrets:` mapping is - evaluated in the CALLER job (which cannot itself carry - `environment:`), the environment must hold a secret named exactly - BOT_APP_PRIVATE_KEY; GitHub substitutes it for the passed value when - this job binds the environment. Never applied to the agent jobs — - they read untrusted repo content and must not sit inside a - credentialed environment. + callers are unaffected, and the binding is also dropped when + bot_app_id is unset, since there is then no token to guard. Because + the caller's `secrets:` mapping is evaluated in the CALLER job (which + cannot itself carry `environment:`), the environment must hold a + secret named exactly BOT_APP_PRIVATE_KEY — GitHub then uses the + environment's secret instead of the passed value ("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" — docs.github.com, Reuse workflows). Never applied + to the agent jobs — they read untrusted repo content and must not sit + inside a credentialed environment. THREE constraints on what you + point this at, all covered in docs/callers/groom.md: (1) use a + DEDICATED environment holding only this key — binding is + all-or-nothing, so every secret and variable in the environment is + injected into these jobs, build_pr included, which applies a + model-authored patch; (2) DEPLOYMENT-BRANCH POLICIES ONLY — a + required-reviewer or wait-timer rule PAUSES build_select, and this + workflow's concurrency group (cancel-in-progress: false) then parks + every later tick behind it, stopping groom for that repo with no + failure and no log; build_pr is additionally a max-parallel-1 matrix, + so it raises ONE deployment PER finding, not one per run; and (3) + CREATE THE ENVIRONMENT AND ITS POLICY FIRST — GitHub creates a + referenced-but-missing environment on demand with no rules and no + secrets, so a typo'd name fails OPEN: a fully green run, still on the + repository-level key, with the gate you think you have absent. type: string required: false default: '' @@ -2003,7 +2026,12 @@ jobs: # environment, so existing callers are unaffected. Bound ONLY by the jobs # that mint the bot token — never by the agent jobs, which run a model over # untrusted repo content and must stay outside a credentialed environment. - environment: ${{ inputs.environment }} + # Also '' when bot_app_id is unset: the mint step below is itself gated on + # `bot_app_id != ''`, so with no App configured there is no credential for + # an environment to guard, and binding one anyway would put this job behind + # protection rules for nothing — a denying rule would then drop the run's + # findings after the audit had already been billed. + environment: ${{ inputs.bot_app_id != '' && inputs.environment || '' }} timeout-minutes: 15 permissions: contents: read @@ -2228,7 +2256,12 @@ jobs: # environment, so existing callers are unaffected. Bound ONLY by the jobs # that mint the bot token — never by the agent jobs, which run a model over # untrusted repo content and must stay outside a credentialed environment. - environment: ${{ inputs.environment }} + # Also '' when bot_app_id is unset: the mint step below is itself gated on + # `bot_app_id != ''`, so with no App configured there is no credential for + # an environment to guard, and binding one anyway would put this job behind + # protection rules for nothing — a denying rule would then drop the run's + # findings after the audit had already been billed. + environment: ${{ inputs.bot_app_id != '' && inputs.environment || '' }} timeout-minutes: 15 permissions: contents: read @@ -2974,7 +3007,11 @@ jobs: # environment, so existing callers are unaffected. Bound ONLY by the jobs # that mint the bot token — never by the agent jobs, which run a model over # untrusted repo content and must stay outside a credentialed environment. - environment: ${{ inputs.environment }} + # Also '' when bot_app_id is unset — kept identical to the other two + # bindings, though it cannot fire here: build_pr is reachable only in + # builder mode, and build_select hard-fails a `builder: true` run that set + # no bot_app_id, so the mint step below can afford to be unconditional. + environment: ${{ inputs.bot_app_id != '' && inputs.environment || '' }} # 15 -> 20. Headroom only, and deliberately the LAST line of defence rather # than the fix: the 2026-08-04 cancellation was a hung fetch that produced # zero bytes, so a bigger budget alone would just have bought a longer hang. diff --git a/README.md b/README.md index 9ba48ee3..31284720 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ complete, copy-pasteable caller. | [`pr-derisk.yml`](.github/workflows/pr-derisk.yml) | **On-demand de-risk split plan (`/derisk`, beta)** — **off by default** (`enabled: false`; `vars.DERISK_CONFIG` on the caller repo outranks it in both directions, so `{"enabled": false}` is a no-PR kill switch). Someone with write access comments `/derisk` on a pull request; the workflow re-grades that PR with the pr-risk grader, makes **one** model call for a semantic partition of the diff into a chain of 2–5 **sequential** PRs, and posts **one sticky advisory comment**: a verdict line and a chain table above the fold, the plan, the chain-landing rules and the floor math inside collapsed `
`. **Every floor shown is computed by `grade-pr-risk.sh --stdin`** over a synthetic record built from each step's files — the model proposes *which files go together* and nothing else, and any tier it writes is discarded before rendering. The partition must cover the changed-file set **exactly**; a missing or duplicated path is rejected, re-prompted **once**, then falls back. **Nothing is gated, routed, merged, labelled or filed** — filing tickets from a plan is a later rung behind its own command. Honest by construction: when no step lands below the PR's **path floor** the verdict reads "N smaller single-concern R3s, same lane", never a fake lane win, because the verdict is arithmetic over grader output rather than the model's prose — and the comparison is against the path floor rather than the headline tier precisely because a split only moves the path axis, so a fork PR (provenance R3, path floor R0) is told the truth instead of a reduction no partition can deliver. An over-budget diff, an unvalidatable partition and an API failure each post an explaining comment — a `/derisk` that quietly does nothing is never an outcome. Safe as a comment command because an `issue_comment` workflow runs from the caller's **default branch** (a PR cannot edit the workflow serving it), the commenter is gated on `author_association` (`allowed_associations`, default `OWNER,MEMBER,COLLABORATOR` — narrow it, never widen), and **no PR code is ever checked out**: the diff is read over the API and handed to a model as text. `workflows_ref` is **required** and its shape + ancestry are enforced by the guard byte-identical to `pr-risk.yml`'s (see that row for the full rationale and its two residuals); a test fails the build if the copies drift. The calling job needs `contents: read` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`, and the `anthropic_api_key` secret. | [pr-derisk.md](docs/callers/pr-derisk.md) | | [`pr-area-label.yml`](.github/workflows/pr-area-label.yml) | **Agentic PR area labeling** — classifies each PR into exactly one `area:*` label, and (on push to the consumer's default branch) syncs the repo's `area:*` labels to the taxonomy. The taxonomy is the consumer's own `.github/area-labels.yml` (`repo_context` + `labels[]` with `name`/`color`/`description`/optional `guidance`); the shared workflow carries nothing repo-specific. An LLM makes the domain-vs-path judgement a static `paths:` map can't, but with **no tools and no token**: PR title/body/paths/labels go to the Anthropic Messages API as data inside `` tags (diff excluded), the reply is enum-constrained by a JSON schema to the taxonomy's own names, and a deterministic step applies it with targeted `area:*` add/remove ops (never a full-set PUT, so concurrent non-area edits survive). The taxonomy is read from the PR's **base ref** — a PR can't rewrite the rules that classify it — and validated (unique `area:[a-z0-9-]+` names, every label resolves to a non-blank routing guide) before it drives a write; everything fails soft rather than failing the check. Classifier logic lives in [`scripts/area-label/`](scripts/area-label), loaded from the pinned `workflows_ref` (validated to a full 40-hex SHA before checkout). Fork/Dependabot PRs are skipped by construction (no writable token, no secret). Labels ride the plain `GITHUB_TOKEN` (no `labeled`-trigger cascade). The calling job needs `contents: read` + `issues: write` + `pull-requests: write`; the `ANTHROPIC_API_KEY` secret is provided org-wide (unset ⇒ `label-pr` fails soft, sync still works). | [pr-area-label.md](docs/callers/pr-area-label.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | -| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set); the optional `environment` input binds a GitHub environment on just the three token-minting jobs (`build_select` / `file` / `build_pr`, never the agent jobs) so that key can be an environment secret behind a deployment-branch policy rather than a repository secret — empty (the default) binds nothing and is byte-for-byte today's behavior, and the environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`. **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | +| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set); the optional `environment` input binds a GitHub environment on just the three token-minting jobs (`build_select` / `file` / `build_pr`, never the agent jobs) so that key can be an environment secret behind a deployment-branch policy rather than a repository secret — empty (the default) binds nothing and is byte-for-byte today's behavior, as is any value while `bot_app_id` is unset. The environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY` (GitHub then uses the environment's copy rather than the passed value), must be a **dedicated** environment (binding injects *all* of its secrets into jobs that include the patch-applying `build_pr`), must already exist (a referenced-but-missing environment is auto-created with no rules, so a typo fails open), and must carry **deployment-branch policies only** — a required-reviewer or wait-timer rule pauses `build_select` and the `cancel-in-progress: false` concurrency group then parks every later tick behind it. **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | | [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `SHA-256`/`AES-192` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`), and for each one the scan covers its CONTENTS, its tracked PATH and (for a symlink) its target string — the same matcher and allowlists on all three, so a private repo name in a directory component is a finding just as it is in a line of prose; binaries are skipped for their contents but still have their path scanned; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | | [`coderabbit-config-validate.yml`](.github/workflows/coderabbit-config-validate.yml) | Validates the caller repo's `.coderabbit.yaml` against CodeRabbit's config schema, **on the PR that breaks it**. CodeRabbit rejects an invalid config *whole* — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter and path instruction in it goes silently inert — and it validates the **base** branch, not the PR head, so the breakage first surfaces on the *next* PR, blamed on a change that did not cause it. Severity mirrors CodeRabbit's own behaviour: a YAML parse error, a `maxLength` violation or a type/enum error **fails**; an unknown/additional property **warns** (CodeRabbit strips those rather than rejecting the file, so the config loads while everything under the key silently does nothing) unless `strict_unknown_keys: true`. A repo with no `.coderabbit.yaml` passes, and says so. The schema is **vendored** at [`.github/coderabbit-config/`](.github/coderabbit-config) — no network fetch on the validation path, so an upstream schema change cannot redden the fleet without a reviewed PR here; `refresh-coderabbit-schema.yml` opens that PR weekly when upstream drifts. Setup: [docs/callers/coderabbit-config-validate.md](docs/callers/coderabbit-config-validate.md). Pin `workflows_ref` to the same ref as `uses:`; no secrets required. | diff --git a/docs/callers/groom.md b/docs/callers/groom.md index 01303f56..6d5cab82 100644 --- a/docs/callers/groom.md +++ b/docs/callers/groom.md @@ -116,19 +116,54 @@ The ones that matter: | `scope_label` / `scope_desc` | `whole-repo` | Cosmetic labels for the scope in issue bodies. | | `workflows_ref` | `''` | **Leaving it unset is safe.** Alone among these workflows groom does not *require* it — it defaults to `''` and each asset checkout falls back to `${{ job.workflow_sha }}`, the commit your `uses:` pin resolved to, so the briefs, `ledger.py` and `interval.py` always match the logic running them with nothing to keep in sync. Set it only to test briefs from a branch. Before BE-8077 that fallback was spelled `github.job_workflow_sha` and silently loaded the assets from this repo's default branch — see the footgun below. | | `bot_app_id` | `''` | File as your App rather than `github-actions[bot]`. | -| `environment` | `''` | Bind a GitHub environment (in YOUR repo) on the three jobs that mint the bot App token — `build_select`, `file`, `build_pr` — so `BOT_APP_PRIVATE_KEY` can be an environment secret behind a deployment-branch policy instead of a repository secret every branch can read. Empty (the default) binds nothing. See "Scoping the bot key to an environment" below. | +| `environment` | `''` | Bind a GitHub environment (in YOUR repo) on the three jobs that mint the bot App token — `build_select`, `file`, `build_pr` — so `BOT_APP_PRIVATE_KEY` can be an environment secret behind a deployment-branch policy instead of a repository secret every branch can read. Empty (the default) binds nothing, and so does any value while `bot_app_id` is unset. Deployment-branch policies only, and the environment must exist before you set this. See "Scoping the bot key to an environment" below. | | `builder` | `false` | Opt into PR-writing — see below. | | `max_prs` | `'5'` | Only with `builder: true`. Typed **string**, deliberately. | | `pr_size_limit` | `400` | Only with `builder: true`. Caps a built PR's diff. | ## Scoping the bot key to an environment -By default `BOT_APP_PRIVATE_KEY` is a **repository** secret, which any workflow on any branch of your repo can read. Set `environment: bot-main` (any environment name) and the three credentialed jobs — `build_select`, `file` and `build_pr`, the only ones that mint the bot App token — bind that environment, so you can hold the key as an **environment** secret with a `main`-only deployment-branch policy instead. The agent jobs (`audit_find`, `audit_verify`, `build`) deliberately never bind it: they run a model over untrusted repo content and must stay outside any credentialed environment. +By default `BOT_APP_PRIVATE_KEY` is a **repository** secret, which any workflow on any branch of your repo can read. Set `environment: bot-main` and the three credentialed jobs — `build_select`, `file` and `build_pr`, the only ones that mint the bot App token — bind that environment, so you can hold the key as an **environment** secret with a `main`-only deployment-branch policy instead. The agent jobs (`audit_find`, `audit_verify`, `build`) deliberately never bind it: they run a model over untrusted repo content and must stay outside any credentialed environment. Nothing binds while `bot_app_id` is unset either — with no App there is no token for an environment to guard. -Two things to get right: +### Why the environment's secret wins -- **The environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`.** Your caller's `secrets:` mapping is evaluated in the *caller* job, which cannot itself carry `environment:`, so the value you pass through is whatever the caller could see. GitHub substitutes the environment's same-named secret for it once the nested job binds the environment — that name match is the whole mechanism, so a differently-named environment secret silently leaves the passed (or empty) value in place. -- **Pass the environment as a plain string** under `with:`, e.g. `environment: bot-main`. It is an ordinary `type: string` input (a `${{ vars.* }}` expression works too) — it is *not* a secret and must not be routed through `secrets:`. Leaving it unset is the existing behavior, unchanged. +Your caller's `secrets:` mapping is evaluated in the *caller* job, which cannot itself carry `environment:`, so what you pass through is whatever the caller could see. The substitution happens on the other side, and it is documented behaviour rather than 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 warns: + +> Environment secrets cannot be passed from the caller workflow as `on.workflow_call` does not support the `environment` keyword. 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 name match is the whole mechanism: **the environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`.** A differently-named environment secret does not error — it silently leaves the passed (or empty) value in place, and you get a green run that never used the environment copy. + +### Migration sequence + +Do these in order. Steps 1–3 are additive and reversible; **step 5 is the one that actually removes the exposure**, and doing it before step 4 breaks groom. + +1. **Create the environment and its deployment-branch policy first.** Settings → Environments → New environment (`bot-main`), then restrict deployment branches to your default branch. Do not skip this: GitHub creates a referenced-but-missing environment **on demand, with no rules and no secrets**, so a typo'd or not-yet-created name fails *open* — the jobs still run, still receive the repository-level key, and the gate you think you have does not exist. There is no error to notice. +2. **Add the App key to that environment** as an environment secret named exactly `BOT_APP_PRIVATE_KEY`. +3. **Set the input** on your caller: `environment: bot-main` under `with:`. It is an ordinary `type: string` input (a `${{ vars.* }}` expression works too) — it is *not* a secret and must not be routed through `secrets:`. +4. **Validate one real run.** Confirm the run shows a deployment to `bot-main` on `build_select` and `file`, and that issues were filed under the bot. Until this passes, keep the repository secret — it is still what the run reads if anything above is misconfigured. +5. **Only then delete the repository-level secret** (`CLOUD_CODE_BOT_PRIVATE_KEY` in the Comfy setup). This is the step that ends the "readable from every branch" exposure. Until you do it, the key is exactly as reachable as before, no matter what the environment says. + +**Your caller's `secrets:` mapping does not change**, and step 5 does not break it. The final form is still: + +```yaml + with: + bot_app_id: ${{ vars.APP_ID }} + environment: bot-main + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + # After step 5 this resolves to '' — the repository secret is gone. That + # is fine and expected: the three credentialed jobs bind `bot-main` and + # use ITS `BOT_APP_PRIVATE_KEY` instead of the value passed here. Keep the + # line: the input is declared `required: false`, but dropping it makes the + # pre-migration and mid-migration states fail instead of degrading. + BOT_APP_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} +``` + +### What to point it at + +- **Use a DEDICATED environment holding only this key.** Binding is all-or-nothing: *every* secret and variable in the environment is injected into these three jobs — including `build_pr`, which applies a model-authored patch and pushes it as the bot. Pointing this at a pre-existing environment that also holds, say, deploy credentials silently widens what those jobs can reach. +- **Deployment-branch policies only — no required reviewers, no wait timer.** A *pausing* rule suspends `build_select`, which both `file` and `build_pr` depend on. This workflow runs under `concurrency: groom-` with `cancel-in-progress: false`, so a run parked awaiting approval (up to 30 days) holds the group and every later daily tick queues behind it and is cancelled — groom stops for that repo with no failure and no log. `build_pr` makes it worse: it is a `max-parallel: 1` matrix over up to `max_prs` findings, so it raises one deployment **per finding**, not one per run — up to five sequential approvals, and a cell denied after earlier cells have pushed leaves a half-filed run. +- **Make the branch policy cover every branch groom actually runs from.** A denied deployment is not free: the finder and verifier have already been billed by then, their findings are lost, and the run still counts as the last real one for the `interval_days` cadence gate (which anchors on the finder having spent, not on the run having filed anything), so the next `interval_days` of scheduled ticks no-op. If you `workflow_dispatch` groom from feature branches, a default-branch-only policy will silently eat those runs. ## Opt-in auto-builder From 519036ebfd4b9d87d28c494eec8eb4fbf141b893 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 18:20:53 -0700 Subject: [PATCH 3/3] docs(groom): stop asserting the untested missing-environment-secret case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review follow-ups. - The guide claimed a differently-named (or absent) environment secret "silently leaves the passed value in place". The documented precedence sentence settles which secret wins when BOTH exist; it does not settle the absent case, and we have not tested it — the competing reading is that the job is left with no usable key and minting fails outright. Say which reading we assume, say why (the silent one is the one that costs you something), and say plainly that it is unconfirmed, rather than stating it as fact. Same correction applied to step 1's "fails open" framing, which rested on it. - Step 4 no longer treats a green run as proof — a run still reading the repository key is also green — and points at the deployment appearing on `build_select` / `file` as the actual signal. Step 5 describes the repository secret as a rollback path rather than a safety net. - README: "finds only — no commits, no PRs, never merges" was unqualified while the same row goes on to document `builder: true` opening PRs. Now "finds only by default", with the auto-builder named as the exception that still never merges. --- README.md | 2 +- docs/callers/groom.md | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 31284720..338d3a09 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ complete, copy-pasteable caller. | [`pr-derisk.yml`](.github/workflows/pr-derisk.yml) | **On-demand de-risk split plan (`/derisk`, beta)** — **off by default** (`enabled: false`; `vars.DERISK_CONFIG` on the caller repo outranks it in both directions, so `{"enabled": false}` is a no-PR kill switch). Someone with write access comments `/derisk` on a pull request; the workflow re-grades that PR with the pr-risk grader, makes **one** model call for a semantic partition of the diff into a chain of 2–5 **sequential** PRs, and posts **one sticky advisory comment**: a verdict line and a chain table above the fold, the plan, the chain-landing rules and the floor math inside collapsed `
`. **Every floor shown is computed by `grade-pr-risk.sh --stdin`** over a synthetic record built from each step's files — the model proposes *which files go together* and nothing else, and any tier it writes is discarded before rendering. The partition must cover the changed-file set **exactly**; a missing or duplicated path is rejected, re-prompted **once**, then falls back. **Nothing is gated, routed, merged, labelled or filed** — filing tickets from a plan is a later rung behind its own command. Honest by construction: when no step lands below the PR's **path floor** the verdict reads "N smaller single-concern R3s, same lane", never a fake lane win, because the verdict is arithmetic over grader output rather than the model's prose — and the comparison is against the path floor rather than the headline tier precisely because a split only moves the path axis, so a fork PR (provenance R3, path floor R0) is told the truth instead of a reduction no partition can deliver. An over-budget diff, an unvalidatable partition and an API failure each post an explaining comment — a `/derisk` that quietly does nothing is never an outcome. Safe as a comment command because an `issue_comment` workflow runs from the caller's **default branch** (a PR cannot edit the workflow serving it), the commenter is gated on `author_association` (`allowed_associations`, default `OWNER,MEMBER,COLLABORATOR` — narrow it, never widen), and **no PR code is ever checked out**: the diff is read over the API and handed to a model as text. `workflows_ref` is **required** and its shape + ancestry are enforced by the guard byte-identical to `pr-risk.yml`'s (see that row for the full rationale and its two residuals); a test fails the build if the copies drift. The calling job needs `contents: read` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`, and the `anthropic_api_key` secret. | [pr-derisk.md](docs/callers/pr-derisk.md) | | [`pr-area-label.yml`](.github/workflows/pr-area-label.yml) | **Agentic PR area labeling** — classifies each PR into exactly one `area:*` label, and (on push to the consumer's default branch) syncs the repo's `area:*` labels to the taxonomy. The taxonomy is the consumer's own `.github/area-labels.yml` (`repo_context` + `labels[]` with `name`/`color`/`description`/optional `guidance`); the shared workflow carries nothing repo-specific. An LLM makes the domain-vs-path judgement a static `paths:` map can't, but with **no tools and no token**: PR title/body/paths/labels go to the Anthropic Messages API as data inside `` tags (diff excluded), the reply is enum-constrained by a JSON schema to the taxonomy's own names, and a deterministic step applies it with targeted `area:*` add/remove ops (never a full-set PUT, so concurrent non-area edits survive). The taxonomy is read from the PR's **base ref** — a PR can't rewrite the rules that classify it — and validated (unique `area:[a-z0-9-]+` names, every label resolves to a non-blank routing guide) before it drives a write; everything fails soft rather than failing the check. Classifier logic lives in [`scripts/area-label/`](scripts/area-label), loaded from the pinned `workflows_ref` (validated to a full 40-hex SHA before checkout). Fork/Dependabot PRs are skipped by construction (no writable token, no secret). Labels ride the plain `GITHUB_TOKEN` (no `labeled`-trigger cascade). The calling job needs `contents: read` + `issues: write` + `pull-requests: write`; the `ANTHROPIC_API_KEY` secret is provided org-wide (unset ⇒ `label-pr` fails soft, sync still works). | [pr-area-label.md](docs/callers/pr-area-label.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | -| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set); the optional `environment` input binds a GitHub environment on just the three token-minting jobs (`build_select` / `file` / `build_pr`, never the agent jobs) so that key can be an environment secret behind a deployment-branch policy rather than a repository secret — empty (the default) binds nothing and is byte-for-byte today's behavior, as is any value while `bot_app_id` is unset. The environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY` (GitHub then uses the environment's copy rather than the passed value), must be a **dedicated** environment (binding injects *all* of its secrets into jobs that include the patch-applying `build_pr`), must already exist (a referenced-but-missing environment is auto-created with no rules, so a typo fails open), and must carry **deployment-branch policies only** — a required-reviewer or wait-timer rule pauses `build_select` and the `cancel-in-progress: false` concurrency group then parks every later tick behind it. **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | +| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only by default — no commits, no PRs; the opt-in auto-builder below is the one exception, and even it never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set); the optional `environment` input binds a GitHub environment on just the three token-minting jobs (`build_select` / `file` / `build_pr`, never the agent jobs) so that key can be an environment secret behind a deployment-branch policy rather than a repository secret — empty (the default) binds nothing and is byte-for-byte today's behavior, as is any value while `bot_app_id` is unset. The environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY` (GitHub then uses the environment's copy rather than the passed value), must be a **dedicated** environment (binding injects *all* of its secrets into jobs that include the patch-applying `build_pr`), must already exist (a referenced-but-missing environment is auto-created with no rules, so a typo fails open), and must carry **deployment-branch policies only** — a required-reviewer or wait-timer rule pauses `build_select` and the `cancel-in-progress: false` concurrency group then parks every later tick behind it. **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. **Path scoping** (`path`, BE-4757): scope a run to ONE directory (`path: services/api`) instead of the whole repo — empty (the default) is today's whole-repo behavior, byte-for-byte, so existing callers are unaffected. It CONSTRAINS rather than instructs (unlike `scope_desc`, which is prompt prose): the path is validated (absolute / `..`-component / escaping paths rejected) and contained against the checkout before any agent runs, the finder is handed the concrete in-scope file list, and findings whose evidence lies entirely outside the directory are dropped with the count logged. The checkout stays **full** on purpose — a refactor in `services/api` legitimately references `common/`. The cadence clock is **per scope** (the finder job is renamed `Audit — finder (scoped: )`, and the interval gate counts only prior runs of the same scope), so a scoped run leaves the next scheduled whole-repo tick due while a permanently scoped caller still gets a real `interval_days` cadence of its own; and the dedup signature ignores `path`, so a scoped run and a whole-repo run suppress each other's duplicates. Expose it as a `workflow_dispatch` input in the caller for on-demand scoped runs, or pin one in `with:` for a permanently scoped monorepo caller. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | | [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `SHA-256`/`AES-192` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`), and for each one the scan covers its CONTENTS, its tracked PATH and (for a symlink) its target string — the same matcher and allowlists on all three, so a private repo name in a directory component is a finding just as it is in a line of prose; binaries are skipped for their contents but still have their path scanned; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | | [`coderabbit-config-validate.yml`](.github/workflows/coderabbit-config-validate.yml) | Validates the caller repo's `.coderabbit.yaml` against CodeRabbit's config schema, **on the PR that breaks it**. CodeRabbit rejects an invalid config *whole* — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter and path instruction in it goes silently inert — and it validates the **base** branch, not the PR head, so the breakage first surfaces on the *next* PR, blamed on a change that did not cause it. Severity mirrors CodeRabbit's own behaviour: a YAML parse error, a `maxLength` violation or a type/enum error **fails**; an unknown/additional property **warns** (CodeRabbit strips those rather than rejecting the file, so the config loads while everything under the key silently does nothing) unless `strict_unknown_keys: true`. A repo with no `.coderabbit.yaml` passes, and says so. The schema is **vendored** at [`.github/coderabbit-config/`](.github/coderabbit-config) — no network fetch on the validation path, so an upstream schema change cannot redden the fleet without a reviewed PR here; `refresh-coderabbit-schema.yml` opens that PR weekly when upstream drifts. Setup: [docs/callers/coderabbit-config-validate.md](docs/callers/coderabbit-config-validate.md). Pin `workflows_ref` to the same ref as `uses:`; no secrets required. | diff --git a/docs/callers/groom.md b/docs/callers/groom.md index 6d5cab82..af0b62e3 100644 --- a/docs/callers/groom.md +++ b/docs/callers/groom.md @@ -131,17 +131,21 @@ Your caller's `secrets:` mapping is evaluated in the *caller* job, which cannot > Environment secrets cannot be passed from the caller workflow as `on.workflow_call` does not support the `environment` keyword. 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 name match is the whole mechanism: **the environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`.** A differently-named environment secret does not error — it silently leaves the passed (or empty) value in place, and you get a green run that never used the environment copy. +That name match is the whole mechanism: **the environment must hold a secret named exactly `BOT_APP_PRIVATE_KEY`.** + +What happens when it does *not* — because the name is misspelled, or because a typo'd `environment:` auto-created an empty one — is the one thing here we have **not** confirmed against a run. GitHub's precedence model is a merge in which the most specific level wins, which implies the caller-passed value simply stays in place: a green run that quietly never used the environment copy. The competing reading is that binding the environment leaves the job with no usable key and token minting fails outright. The documented sentence above settles which secret wins when *both* exist; it does not settle the absent case, and we have not tested it. + +Assume the silent one. It is the reading that costs you something — a loud mint failure tells you immediately, whereas a green run that still reads the repository key looks exactly like success. Step 4 below is what catches it either way, and it is why step 5 comes last. ### Migration sequence Do these in order. Steps 1–3 are additive and reversible; **step 5 is the one that actually removes the exposure**, and doing it before step 4 breaks groom. -1. **Create the environment and its deployment-branch policy first.** Settings → Environments → New environment (`bot-main`), then restrict deployment branches to your default branch. Do not skip this: GitHub creates a referenced-but-missing environment **on demand, with no rules and no secrets**, so a typo'd or not-yet-created name fails *open* — the jobs still run, still receive the repository-level key, and the gate you think you have does not exist. There is no error to notice. +1. **Create the environment and its deployment-branch policy first.** Settings → Environments → New environment (`bot-main`), then restrict deployment branches to your default branch. Do not skip this: GitHub creates a referenced-but-missing environment **on demand, with no rules and no secrets**, so a typo'd or not-yet-created name gives you a run with no gate on it at all — and, on the reading above, no error to notice either. Check the environment name against Settings → Environments rather than trusting a green run. 2. **Add the App key to that environment** as an environment secret named exactly `BOT_APP_PRIVATE_KEY`. 3. **Set the input** on your caller: `environment: bot-main` under `with:`. It is an ordinary `type: string` input (a `${{ vars.* }}` expression works too) — it is *not* a secret and must not be routed through `secrets:`. -4. **Validate one real run.** Confirm the run shows a deployment to `bot-main` on `build_select` and `file`, and that issues were filed under the bot. Until this passes, keep the repository secret — it is still what the run reads if anything above is misconfigured. -5. **Only then delete the repository-level secret** (`CLOUD_CODE_BOT_PRIVATE_KEY` in the Comfy setup). This is the step that ends the "readable from every branch" exposure. Until you do it, the key is exactly as reachable as before, no matter what the environment says. +4. **Validate one real run.** Confirm the run shows a deployment to `bot-main` on `build_select` and `file`, and that issues were filed under the bot. Do not treat green alone as proof — a run that is still reading the repository key is also green. The deployment appearing on those two jobs is the signal that the binding took effect. Keep the repository secret until this passes. +5. **Only then delete the repository-level secret** (`CLOUD_CODE_BOT_PRIVATE_KEY` in the Comfy setup). This is the step that ends the "readable from every branch" exposure — until you do it, the key is exactly as reachable as before, no matter what the environment says. Deleting it is also what makes step 4's check meaningful in retrospect: from here on, a working run *cannot* be one that fell back. Treat the repository secret as a **rollback path** rather than a safety net — if the environment-backed run misbehaves, re-adding it restores the previous state. **Your caller's `secrets:` mapping does not change**, and step 5 does not break it. The final form is still: