Skip to content

Commit a98de8b

Browse files
authored
feat(coderabbit-config): validate .coderabbit.yaml so a broken config fails its own PR (BE-7757) (#179)
* feat(coderabbit-config): validate .coderabbit.yaml so a broken config fails its own PR (BE-7757) CodeRabbit rejects an invalid .coderabbit.yaml WHOLE — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter, path instruction and WIP-skip rule in it goes silently inert. And it validates the BASE branch, not the PR head, so the PR that breaks the file goes green and the breakage first surfaces on the NEXT PR, blamed on a change that did not cause it. Nothing in CI caught this; the same field on the same repo was fixed twice in six days by two independent tickets. Adds coderabbit-config-validate.yml (reusable, workflows_ref REQUIRED, checker loaded from this repo at the pinned ref), its checker + VENDORED schema under .github/coderabbit-config/, the test workflow, the bump fleet entrypoint, and a weekly refresh job that opens a PR when the vendored schema drifts upstream. Severity mirrors CodeRabbit's own behaviour rather than a schema library's: parse / maxLength / type / enum errors FAIL; an unknown key WARNS (CodeRabbit 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. The schema is vendored, never fetched on the validation path: a live fetch makes every consumer's CI depend on a third party, and an upstream tightening would redden the fleet with no change here. * fix(coderabbit-config): close the review panel's findings on the validator (BE-7757) Ten findings from the cursor-review panel, addressed in place. Checker: - Containment now resolves symlinks on BOTH sides (`realpath`, not `abspath`). A config committed as a symlink out of the tree cleared the old textual guard and was read anyway, and a YAML error quotes the target into a public log. - A path that exists but is not a regular file (directory, dangling symlink) is exit 2, not a silent pass. - `.coderabbit.yml` under the default name is validated with a warning instead of reported "absent — pass". CodeRabbit honours both spellings, so that was a permanently green check over a config nobody looked at. - `config_file` and `--root` are escaped in the banner and result lines like every other print; a newline in that workflow input could emit `::stop-commands::` and suppress the annotations of a failing check. - Bound the PR-controlled input (512 KiB) and the reported findings (100). Schema drift: - Diff the UNION of old and new caps. The old walk skipped a cap upstream ADDS to a previously uncapped property — the single most config-invalidating drift there is — and never looked at one it removes; both then printed "No property or `maxLength` changes", which is false. - Walk `anyOf`/`oneOf`/`allOf`, `patternProperties`, schema-valued `additionalProperties`, list-form `items` and `$defs`. The vendored schema already uses two of those shapes, so drift inside them was invisible. - An unexpected exception exits 2, not Python's default 1 — 1 means DRIFTED to the caller, which force-resets a shared branch on it. Workflows: - `python3 -P -m pip`: that step's working directory is the untrusted caller checkout, and `-m` puts it first on `sys.path`, so a PR adding `pip/__main__.py` ran its own code before the pinned checker and could rewrite it. - The refresh's invariant suite no longer aborts the run before the PR exists. It asserts exact facts (the 14-cap set, tone_instructions == 250), so any real upstream cap change failed it — and the later steps' implicit `success()` then opened no PR at all, silently dropping the drift class this prioritizes. The verdict now rides in the PR body and reddens the run at the end. - Verify drift.md before the force-push, so the shared branch never moves with nothing to explain it. Docs scoped to reality: unknown-key detection only fires where the schema CLOSES an object, so a typo inside `reviews` is not caught. Widening it is follow-up work.
1 parent 6446399 commit a98de8b

17 files changed

Lines changed: 4966 additions & 4 deletions

‎.github/bump-callers/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ forward automatically instead of silently drifting commits behind.
2929
|---|---|---|---|
3030
| [`bump-cursor-review-callers.yml`](../workflows/bump-cursor-review-callers.yml) | `cursor-review.yml`, `cursor-review/**` or `scripts/check-pr-size/**` (minus its `*_test.go`, which no caller executes) | `CURSOR_REVIEW_CALLERS` | non-empty (hard-fails if empty) |
3131
| [`bump-agents-md-callers.yml`](../workflows/bump-agents-md-callers.yml) | `agents-md-integrity.yml` or `agents-md-integrity/**` | `AGENTS_MD_CALLERS` | empty `[]` (grows as callers land) |
32+
| [`bump-coderabbit-config-callers.yml`](../workflows/bump-coderabbit-config-callers.yml) | `coderabbit-config-validate.yml` or `coderabbit-config/**` | `CODERABBIT_CONFIG_CALLERS` | empty `[]` (grows as callers land) |
3233
| [`bump-pr-size-callers.yml`](../workflows/bump-pr-size-callers.yml) | `pr-size.yml` or `scripts/check-pr-size/**` (minus its `*_test.go`, which no caller executes) | `PR_SIZE_CALLERS` | empty `[]` (grows as callers land) |
3334
| [`bump-pr-risk-callers.yml`](../workflows/bump-pr-risk-callers.yml) | `pr-risk.yml` or `scripts/pr-risk/**` (minus its `tests/` and `README.md`, which no caller executes) | `PR_RISK_CALLERS` | non-empty (hard-fails if empty) |
3435
| [`bump-pr-derisk-callers.yml`](../workflows/bump-pr-derisk-callers.yml) | `pr-derisk.yml`, `scripts/pr-derisk/**` **or `scripts/pr-risk/**`** (minus both `tests/` and `README.md`) — a `/derisk` run executes the pr-risk grader as well as the planner, so a grader change is consumer-visible on this fleet too | `PR_RISK_CALLERS` **(shared — filtered to `ci-pr-derisk.yml`)** | **may select nothing** (no callers enrolled yet — flip `ALLOW_EMPTY` to `false` with the first enrolment) |
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
# coderabbit-config
2+
3+
The checker behind [`coderabbit-config-validate.yml`](../workflows/coderabbit-config-validate.yml),
4+
plus the vendored schema it validates against. Consumer repos carry only a thin
5+
caller; everything here is loaded at run time from the SHA a caller pins, so a PR
6+
can never rewrite the check that judges it — nor swap the schema it is graded
7+
against.
8+
9+
## What it catches, and why CI has to
10+
11+
CodeRabbit rejects an invalid `.coderabbit.yaml` **whole**. Not the offending
12+
field — the entire file. The review then runs on org-wide UI defaults, so every
13+
reviewer instruction, path filter, path instruction and WIP-skip rule in the file
14+
goes silently inert. The file still reads fine; only the validator sees the loss.
15+
16+
And the feedback arrives one PR late, on the wrong diff: CodeRabbit validates the
17+
config on the **base** branch, not the PR head. The PR that breaks the file goes
18+
green; the breakage surfaces on the *next* PR, attributed to a change that did not
19+
cause it. That displacement is what makes this a machine check rather than a
20+
convention — the same field on the same repo was fixed twice in six days by two
21+
independent tickets, which is the signature of a failure nothing detects.
22+
23+
## Severity, and why it is split
24+
25+
The split mirrors what CodeRabbit itself does with each class of problem, not what
26+
a schema library calls "invalid":
27+
28+
| Class | Severity | Because |
29+
|---|---|---|
30+
| YAML parse error | error | CodeRabbit cannot read the file at all. |
31+
| `maxLength` violation | error | File-rejecting. The schema carries **14** per-field caps, 50 → 20,000 chars. |
32+
| type / enum error | error | File-rejecting. |
33+
| unknown / additional property | **warning** | CodeRabbit *strips* an unrecognized key rather than rejecting the file. Only where the schema closes the object — see below. |
34+
| config path that is not a regular file, or > 512 KiB | exit 2 | "I could not check" must never look like a pass. A path resolving outside the repo root (symlinks resolved) is refused the same way. |
35+
36+
`strict_unknown_keys: true` escalates the last row to an error for a repo that has
37+
cleaned up and wants to stay clean.
38+
39+
**How far the unknown-key row reaches.** It fires where the schema CLOSES an
40+
object — the document root, `knowledge_base.mcp`, and the two tool objects that
41+
declare `additionalProperties: false`. Most nested objects (`reviews`, `chat`,
42+
`knowledge_base`, `code_generation`, the individual tool configs) are open in
43+
upstream's schema, so a typo *inside* one — `reviews.profil`, or
44+
`golangci-lint.enabld` — produces no finding, with or without
45+
`strict_unknown_keys`. That is a real gap and not a claim this checker makes:
46+
what it catches is a key in the wrong PLACE, which is the failure that actually
47+
recurred here. Widening it means cross-checking parsed keys against the schema
48+
index rather than waiting for jsonschema to complain.
49+
50+
A stripped key is not a cosmetic problem. The recurring instance is a `tools:`
51+
block written at the document root instead of under `reviews:` — the schema root
52+
is `additionalProperties: false`, so the whole block is dropped and every setting
53+
in it reverts to the schema default, which is often the *opposite* of what was
54+
written. `reviews.tools.golangci-lint.enabled` defaults to `true`, so a root-level
55+
`enabled: false` runs the linter it was written to disable. Three org repos
56+
carried exactly that shape when this landed.
57+
58+
## Why file size is the wrong invariant
59+
60+
The obvious cheap check — "fail if `.coderabbit.yaml` gets too big" — would gate
61+
nothing. There is no whole-document size cap in the schema, only the 14 per-field
62+
ones, and size is uncorrelated with validity: the invalid config that started this
63+
was 2,198 bytes while a valid one next to it was 4,826. Across the org sweep there
64+
were **zero** `maxLength` violations and three misplaced-key repos, so a size check
65+
would have found none of the real problems and none of the historical one.
66+
67+
## Why the schema is vendored
68+
69+
`schema.v2.json` is a committed copy of
70+
`https://coderabbit.ai/integrations/schema.v2.json`. Validation never fetches it.
71+
72+
- A live fetch makes every consumer's CI depend on a third-party endpoint.
73+
- An upstream tightening — a `maxLength` lowered, a property removed — would turn
74+
CI red across every enrolled repo with no change on our side and no PR to point
75+
at.
76+
77+
[`refresh-coderabbit-schema.yml`](../workflows/refresh-coderabbit-schema.yml)
78+
covers the cost of vendoring: weekly it fetches upstream, compares semantically
79+
(so a whitespace-only reserialization is not "drift"), and opens a reviewable PR
80+
with a summary that leads on tightened caps — the one drift class that can
81+
retroactively invalidate a config nobody touched. Merging it trips
82+
`bump-coderabbit-config-callers.yml`, which rolls the pinned caller fleet forward.
83+
84+
**Any fetch of that URL must use `curl -fsSL`.** It 301-redirects, and a bare
85+
`curl` writes a ~167-byte HTML redirect stub. Both `check_coderabbit_config.py`
86+
and `schema_drift.py` refuse a file that is not a JSON Schema object rather than
87+
treating it as "no drift" or "everything valid" — a check that cannot fail is
88+
worse than no check.
89+
90+
## The dependency
91+
92+
This repo is otherwise stdlib-only, deliberately. This directory is the one
93+
exception and neither half has a stdlib answer: there is no YAML parser in the
94+
standard library, and the point of the check is to reproduce the verdict
95+
CodeRabbit's own Draft 2020-12 validator reaches — a hand-written subset validator
96+
would agree with the real one until it didn't, and the day it diverged the check
97+
would either gate nothing or fail a valid config.
98+
99+
So the dependency is pinned hard instead: exact versions with sha256 hashes in
100+
[`requirements.txt`](requirements.txt), installed with `--require-hashes`, and
101+
watched by Dependabot (`pip` ecosystem in `.github/dependabot.yml`) so the pins do
102+
not rot.
103+
104+
## Files
105+
106+
| File | What it is |
107+
|---|---|
108+
| `check_coderabbit_config.py` | The validator. `--root`, `--config`, `--schema`, `--strict-unknown-keys`. Exit 0 pass / 1 invalid / 2 could-not-run. |
109+
| `schema.v2.json` | The vendored schema. Never edit by hand — let the refresh job propose the bump. |
110+
| `schema_drift.py` | Semantic comparison + Markdown drift summary for the refresh PR. Exit 0 no-drift / 1 drifted / 2 unusable input. |
111+
| `requirements.txt` | The hash-pinned PyYAML + jsonschema set. |
112+
| `tests/` | Unit tests, run by `test-coderabbit-config-validate.yml`. |
113+
114+
## Run it locally
115+
116+
```bash
117+
python3 -m venv /tmp/crvenv
118+
/tmp/crvenv/bin/pip install --require-hashes --only-binary=:all: \
119+
-r .github/coderabbit-config/requirements.txt # linux/cp312 wheels
120+
/tmp/crvenv/bin/python .github/coderabbit-config/check_coderabbit_config.py --root .
121+
/tmp/crvenv/bin/python -m unittest discover -s .github/coderabbit-config/tests -p 'test_*.py' -v
122+
```
123+
124+
The hashes pin the cp312 / manylinux x86_64 wheels CI installs, so on another
125+
platform install the same two versions without `--require-hashes` instead.

0 commit comments

Comments
 (0)