[PR #179] [MERGED] feat(coderabbit-config): validate .coderabbit.yaml so a broken config fails its own PR (BE-7757) #186

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-7757-coderabbit-config-validate

📝 Commits (2)

3f86cee feat(coderabbit-config): validate .coderabbit.yaml so a broken config fails its own PR (BE-7757)
959c374 fix(coderabbit-config): close the review panel's findings on the validator (BE-7757)

📊 Changes

17 files changed (+4966 additions, -4 deletions)

View changed files

📝 .github/bump-callers/README.md (+1 -0)
➕ .github/coderabbit-config/README.md (+125 -0)
➕ .github/coderabbit-config/check_coderabbit_config.py (+642 -0)
➕ .github/coderabbit-config/requirements.txt (+53 -0)
➕ .github/coderabbit-config/schema.v2.json (+2249 -0)
➕ .github/coderabbit-config/schema_drift.py (+274 -0)
➕ .github/coderabbit-config/tests/test_check_coderabbit_config.py (+398 -0)
➕ .github/coderabbit-config/tests/test_schema_drift.py (+217 -0)
📝 .github/dependabot.yml (+26 -0)
➕ .github/workflows/bump-coderabbit-config-callers.yml (+161 -0)
➕ .github/workflows/coderabbit-config-validate.yml (+202 -0)
➕ .github/workflows/refresh-coderabbit-schema.yml (+279 -0)
➕ .github/workflows/test-coderabbit-config-validate.yml (+190 -0)
📝 AGENTS.md (+32 -4)
📝 README.md (+1 -0)
📝 docs/callers/README.md (+2 -0)
➕ docs/callers/coderabbit-config-validate.md (+114 -0)

📄 Description

ELI-5

CodeRabbit reads a .coderabbit.yaml in your repo to know how to review your PRs. If that file breaks the rules of its own format, CodeRabbit throws away the whole file and reviews with generic defaults instead — every instruction you wrote silently stops mattering, and nothing looks different. Worse, it checks the file on the base branch, so the PR that broke it goes green and the damage shows up on the next person's PR, blamed on them.

This adds a shared CI check that validates the file on the PR that changes it, so the breakage lands on the right diff, with an annotation on the right line.

What's here

File
`.github/workflows/coderabbit-config-validate.yml`	The reusable `workflow_call` check. `workflows_ref` REQUIRED, no default; checker loaded from this repo at the pinned ref, never from the caller's checkout.
`.github/coderabbit-config/`	`check_coderabbit_config.py`, the vendored `schema.v2.json`, `schema_drift.py`, hash-pinned `requirements.txt`, tests, README.
`.github/workflows/test-coderabbit-config-validate.yml`	35 unit tests + 6 end-to-end CLI smoke tests.
`.github/workflows/bump-coderabbit-config-callers.yml`	The SHA-bump fleet, on the shared `preflight.sh` + `bump-callers.sh`. Roster is a secret (`CODERABBIT_CONFIG_CALLERS`), seeded empty.
`.github/workflows/refresh-coderabbit-schema.yml`	Weekly: fetch upstream, compare semantically, open a reviewable PR on drift.
docs	`docs/callers/coderabbit-config-validate.md`, README catalog row, AGENTS.md, dependabot `pip` entry.

Severity is split to mirror what CodeRabbit itself does, not what a schema library calls invalid: a YAML parse error, a maxLength violation or a type/enum error fails (those are what it rejects the whole file for); an unknown/additional property warns (it strips those, so the config loads while everything under the key does nothing) unless strict_unknown_keys: true. A repo with no config passes, and says so — "no config here" must never read the same as "config validated clean" in a log.

Verification

Repo suites — all green locally: the new coderabbit-config suite (35 tests), the 6 CLI smoke steps extracted from the test workflow and executed verbatim, check_workflow_pins.py (8 reusables, no workflows_ref default, every ref checkout guarded) + its 78 tests, test_paths_contract.sh (58 checks — the guard that this fleet's paths: filter exactly matches the WATCHED/WATCHED_ASSETS it hands preflight.sh), test_bump_callers.sh, shellcheck, and the agents-md / cursor-review / groom / refresh-reviewers / workflow-pins suites.

Against the artifacts the ticket names. I swept 18 org repos read-only for a .coderabbit.yaml; 11 carry one. Running this checker over all 11 reproduces the ticket's Evidence table exactly:

	Default	`strict_unknown_keys: true`
The 3 configs the ticket reports as invalid	warn (exit 0)	fail (exit 1)
The other 8	clean	clean

Each of the three is reported with the offending root key named, the correct source line, and a Did you mean \reviews.tools`?hint. ZeromaxLength` violations org-wide — which is the second half of the ticket's point, and why a file-size check would have gated nothing. Repo names are redacted here: most are private and this repo is public.

The half I am not fixing: the 3 invalid configs stay invalid — they are out of scope by the ticket's own design (one child ticket per repo, so each lands as its own PR in its own repo). And the check is currently wired to 0 of 11 repos: this PR ships the machinery, the fleet makes the rollout a follow-up.

The install path is exercised for real: pip install --require-hashes --only-binary=:all: resolved cleanly for linux/cp312 in a dry run, including the typing-extensions transitive that a 3.13+ resolver silently drops. The same install runs in the test workflow, so a wrong hash set fails on the PR that broke it rather than in every consumer's CI.

Judgment calls

The vendored schema, and the one dependency. This repo is stdlib-only on purpose, and this directory breaks that. Neither half has a stdlib answer: there is no YAML parser in the standard library, and the point of the check is to reproduce the verdict CodeRabbit's own Draft 2020-12 validator reaches — a hand-written subset validator would agree with the real one until it didn't, and the day it diverged the check would either gate nothing or fail a valid config. So the dependency is accepted and pinned hard: exact versions + sha256, --require-hashes, and a Dependabot pip entry so the pins don't rot the way an unwatched inline pip install x==1.2.3 does. AGENTS.md now records the exception rather than still claiming stdlib-only.

Not extending agents-md-integrity.yml — a sibling workflow, per the ticket. Folding this in would push a Python dependency and a new failure mode onto every one of its callers, for a check about a different class of file.

Failure modes fail loudly, in the right direction. Exit 2 (could-not-run) is distinct from exit 1 (config invalid), because "I could not check" must never look like "it's fine". Both the checker and the drift detector refuse a file that isn't a JSON Schema object rather than reporting "no drift" / "all valid" — a check that cannot fail is worse than no check. That is the concrete redirect-stub case the ticket names, and it's covered by a smoke test.

Path containment + injection. config_file is guarded against an absolute or ../-escaping path and against an empty value (which would otherwise join to the repo root and report a clean pass over a repo nobody looked at). Every annotation is workflow-command-escaped — the strings come from a PR-author-controlled file, and YAML permits a newline in a quoted key.

Unmet criteria and unexercised artifacts

The comfy-conventions caller is NOT in this PR. That acceptance criterion needs a commit in a different repository; I am scoped to one repo and one PR. It is a two-line caller plus a roster entry — docs/callers/coderabbit-config-validate.md is the copy-pasteable version. The fleet entrypoint also needs one workflow_dispatch run from main after merge (landing a fleet touches no path its own filter watches, so it fires no run of its own).
The severity split rests on a premise I could not re-verify from CI. That CodeRabbit strips an unknown key rather than rejecting the file comes from the ticket's runtime observation. The circumstantial evidence is consistent — three repos with a root-level tools: block are reviewed normally rather than defaulted — but I did not observe CodeRabbit's loader directly. If it turns out to be file-rejecting, strict_unknown_keys is the switch, and the default should flip.
Sub-issues BE-7759 and BE-7761 were named to me but not fetched, and their bodies are unreachable from this sandbox. I worked from the parent ticket's description of them (the per-repo cleanups, deliberately out of scope here).
Not covered: duplicate YAML keys. PyYAML silently keeps the last one; whether CodeRabbit's parser errors instead is untested. Worth its own look if someone hits it.

Refs BE-7757 — the ticket stays open: the caller wiring criterion is unmet above, and the three invalid configs it reports are still invalid, tracked on its children.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/179 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/18/2026 **Status:** ✅ Merged **Merged:** 8/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7757-coderabbit-config-validate` --- ### 📝 Commits (2) - [`3f86cee`](https://github.com/Comfy-Org/github-workflows/commit/3f86cee375c0d5d2bec1920f52a6945edcc0ba3d) feat(coderabbit-config): validate .coderabbit.yaml so a broken config fails its own PR (BE-7757) - [`959c374`](https://github.com/Comfy-Org/github-workflows/commit/959c37433d254495d6a93dee01a267e2cb2bb36b) fix(coderabbit-config): close the review panel's findings on the validator (BE-7757) ### 📊 Changes **17 files changed** (+4966 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+1 -0) ➕ `.github/coderabbit-config/README.md` (+125 -0) ➕ `.github/coderabbit-config/check_coderabbit_config.py` (+642 -0) ➕ `.github/coderabbit-config/requirements.txt` (+53 -0) ➕ `.github/coderabbit-config/schema.v2.json` (+2249 -0) ➕ `.github/coderabbit-config/schema_drift.py` (+274 -0) ➕ `.github/coderabbit-config/tests/test_check_coderabbit_config.py` (+398 -0) ➕ `.github/coderabbit-config/tests/test_schema_drift.py` (+217 -0) 📝 `.github/dependabot.yml` (+26 -0) ➕ `.github/workflows/bump-coderabbit-config-callers.yml` (+161 -0) ➕ `.github/workflows/coderabbit-config-validate.yml` (+202 -0) ➕ `.github/workflows/refresh-coderabbit-schema.yml` (+279 -0) ➕ `.github/workflows/test-coderabbit-config-validate.yml` (+190 -0) 📝 `AGENTS.md` (+32 -4) 📝 `README.md` (+1 -0) 📝 `docs/callers/README.md` (+2 -0) ➕ `docs/callers/coderabbit-config-validate.md` (+114 -0) </details> ### 📄 Description ## ELI-5 CodeRabbit reads a `.coderabbit.yaml` in your repo to know how to review your PRs. If that file breaks the rules of its own format, CodeRabbit throws away **the whole file** and reviews with generic defaults instead — every instruction you wrote silently stops mattering, and nothing looks different. Worse, it checks the file on the *base* branch, so the PR that broke it goes green and the damage shows up on the *next* person's PR, blamed on them. This adds a shared CI check that validates the file **on the PR that changes it**, so the breakage lands on the right diff, with an annotation on the right line. ## What's here | File | | |---|---| | `.github/workflows/coderabbit-config-validate.yml` | The reusable `workflow_call` check. `workflows_ref` REQUIRED, no default; checker loaded from this repo at the pinned ref, never from the caller's checkout. | | `.github/coderabbit-config/` | `check_coderabbit_config.py`, the **vendored** `schema.v2.json`, `schema_drift.py`, hash-pinned `requirements.txt`, tests, README. | | `.github/workflows/test-coderabbit-config-validate.yml` | 35 unit tests + 6 end-to-end CLI smoke tests. | | `.github/workflows/bump-coderabbit-config-callers.yml` | The SHA-bump fleet, on the shared `preflight.sh` + `bump-callers.sh`. Roster is a **secret** (`CODERABBIT_CONFIG_CALLERS`), seeded empty. | | `.github/workflows/refresh-coderabbit-schema.yml` | Weekly: fetch upstream, compare **semantically**, open a reviewable PR on drift. | | docs | `docs/callers/coderabbit-config-validate.md`, README catalog row, AGENTS.md, dependabot `pip` entry. | **Severity is split to mirror what CodeRabbit itself does**, not what a schema library calls invalid: a YAML parse error, a `maxLength` violation or a type/enum error **fails** (those are what it rejects the whole file for); an unknown/additional property **warns** (it strips those, so the config loads while everything under the key does nothing) unless `strict_unknown_keys: true`. A repo with no config **passes, and says so** — "no config here" must never read the same as "config validated clean" in a log. ## Verification **Repo suites** — all green locally: the new `coderabbit-config` suite (35 tests), the 6 CLI smoke steps extracted from the test workflow and executed verbatim, `check_workflow_pins.py` (8 reusables, no `workflows_ref` default, every ref checkout guarded) + its 78 tests, `test_paths_contract.sh` (58 checks — the guard that this fleet's `paths:` filter exactly matches the `WATCHED`/`WATCHED_ASSETS` it hands `preflight.sh`), `test_bump_callers.sh`, `shellcheck`, and the agents-md / cursor-review / groom / refresh-reviewers / workflow-pins suites. **Against the artifacts the ticket names.** I swept **18** org repos read-only for a `.coderabbit.yaml`; **11** carry one. Running this checker over all 11 reproduces the ticket's Evidence table exactly: | | Default | `strict_unknown_keys: true` | |---|---|---| | The 3 configs the ticket reports as invalid | warn (exit 0) | **fail (exit 1)** | | The other 8 | clean | clean | Each of the three is reported with the offending root key named, the correct source line, and a `Did you mean \`reviews.tools\`?` hint. Zero `maxLength` violations org-wide — which is the second half of the ticket's point, and why a file-size check would have gated nothing. Repo names are **redacted here**: most are private and this repo is public. **The half I am not fixing:** the 3 invalid configs stay invalid — they are out of scope by the ticket's own design (one child ticket per repo, so each lands as its own PR in its own repo). And the check is currently wired to **0 of 11** repos: this PR ships the machinery, the fleet makes the rollout a follow-up. **The install path** is exercised for real: `pip install --require-hashes --only-binary=:all:` resolved cleanly for linux/cp312 in a dry run, including the `typing-extensions` transitive that a 3.13+ resolver silently drops. The same install runs in the test workflow, so a wrong hash set fails on the PR that broke it rather than in every consumer's CI. ## Judgment calls **The vendored schema, and the one dependency.** This repo is stdlib-only on purpose, and this directory breaks that. Neither half has a stdlib answer: there is no YAML parser in the standard library, and the *point* of the check is to reproduce the verdict CodeRabbit's own Draft 2020-12 validator reaches — a hand-written subset validator would agree with the real one until it didn't, and the day it diverged the check would either gate nothing or fail a valid config. So the dependency is accepted and pinned hard: exact versions + sha256, `--require-hashes`, and a Dependabot `pip` entry so the pins don't rot the way an unwatched inline `pip install x==1.2.3` does. AGENTS.md now records the exception rather than still claiming stdlib-only. **Not extending `agents-md-integrity.yml`** — a sibling workflow, per the ticket. Folding this in would push a Python dependency and a new failure mode onto every one of its callers, for a check about a different class of file. **Failure modes fail loudly, in the right direction.** Exit 2 (could-not-run) is distinct from exit 1 (config invalid), because "I could not check" must never look like "it's fine". Both the checker and the drift detector refuse a file that isn't a JSON Schema object rather than reporting "no drift" / "all valid" — a check that cannot fail is worse than no check. That is the concrete redirect-stub case the ticket names, and it's covered by a smoke test. **Path containment + injection.** `config_file` is guarded against an absolute or `../`-escaping path and against an empty value (which would otherwise join to the repo root and report a clean pass over a repo nobody looked at). Every annotation is workflow-command-escaped — the strings come from a PR-author-controlled file, and YAML permits a newline in a quoted key. ## Unmet criteria and unexercised artifacts - **The `comfy-conventions` caller is NOT in this PR.** That acceptance criterion needs a commit in a *different* repository; I am scoped to one repo and one PR. It is a two-line caller plus a roster entry — `docs/callers/coderabbit-config-validate.md` is the copy-pasteable version. The fleet entrypoint also needs one `workflow_dispatch` run from `main` after merge (landing a fleet touches no path its own filter watches, so it fires no run of its own). - **The severity split rests on a premise I could not re-verify from CI.** That CodeRabbit *strips* an unknown key rather than rejecting the file comes from the ticket's runtime observation. The circumstantial evidence is consistent — three repos with a root-level `tools:` block are reviewed normally rather than defaulted — but I did not observe CodeRabbit's loader directly. If it turns out to be file-rejecting, `strict_unknown_keys` is the switch, and the default should flip. - **Sub-issues BE-7759 and BE-7761 were named to me but not fetched**, and their bodies are unreachable from this sandbox. I worked from the parent ticket's description of them (the per-repo cleanups, deliberately out of scope here). - **Not covered:** duplicate YAML keys. PyYAML silently keeps the last one; whether CodeRabbit's parser errors instead is untested. Worth its own look if someone hits it. Refs BE-7757 — the ticket stays open: the caller wiring criterion is unmet above, and the three invalid configs it reports are still invalid, tracked on its children. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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