[PR #188] [MERGED] feat(coderabbit-config): flag unknown keys inside schema-open objects (BE-7786) #198

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-7786-coderabbit-config-unknown-keys

📝 Commits (3)

4b011a4 feat(coderabbit-config): flag unknown keys inside schema-open objects (BE-7786)
72aa76d fix(coderabbit-config): cap findings after fan-out, name keys by source spelling, bound list traversal (BE-7786)
350dd93 docs(coderabbit-config): document the partial-scan severity row; correct a stale test comment (BE-7786)

📊 Changes

6 files changed (+823 additions, -44 deletions)

View changed files

📝 .github/coderabbit-config/README.md (+45 -11)
📝 .github/coderabbit-config/check_coderabbit_config.py (+300 -23)
📝 .github/coderabbit-config/tests/test_check_coderabbit_config.py (+395 -2)
📝 .github/workflows/test-coderabbit-config-validate.yml (+30 -0)
📝 AGENTS.md (+2 -2)
📝 docs/callers/coderabbit-config-validate.md (+51 -6)

📄 Description

ELI-5

CodeRabbit throws away any key in .coderabbit.yaml it doesn't recognize. The checker only told you about that in five places — the handful of spots where the schema explicitly says "nothing else allowed here". Everywhere else it said nothing, because the JSON Schema library it leans on had no rule to trip. So reviews: {profil: chill} — one letter off, and every setting under it silently inert — came back clean, in both normal and strict mode.

This makes the check look at the other 103 objects too. It reports the same thing, the same way, in the same words; it just no longer waits for the schema to spell out a rule it can restate.

The rule, and why it is drawn this way

Walking the parsed config alongside the schema, an object is treated as closed by omission only when it declares a properties block and none of additionalProperties, unevaluatedProperties, patternProperties, anyOf, oneOf, allOf, $ref, $dynamicRef, if, dependentSchemas, propertyNames, not. Each of those is a way an object can legitimately hold a name its properties does not list, so reading any of them as "closed" would invent a warning on a valid config — the failure mode that matters most here, because this check gates other repos' CI. Today's schema uses exactly one of the twelve, so eleven entries are pure future-proofing against a vendored-schema bump.

The walk descends only through matched properties and through items; never into a combinator branch, an additionalProperties schema, or the value under a key it just reported (that key has no subschema, so nothing beneath it is judgeable). Where the document and the schema disagree about shape it stays silent — that's a type error and jsonschema already describes it, in better words than a second complaint would.

No double-reporting, by construction rather than by de-duplication. jsonschema fires only where the keyword is present; the walk only where it is absent. Complements, not overlapping sets. test_the_two_halves_never_report_the_same_key exercises both at once on one config so a future refactor that merged them fails here instead of double-annotating someone's PR.

That last claim is stated as a fact about this vendored schema, not as a property of the two conditions — they are complements at a single node, but jsonschema evaluates a document path against every applicable subschema, so a node this walk judges closed whose $ref target or combinator branch carried additionalProperties: false would be reported by both halves. OpennessTest pins the two premises that rule it out today: the schema carries no $ref, and no node declaring properties also carries a combinator (so such a node counts as OPEN here and is never judged closed).

Findings from both halves go through one builder, so severity (warning, or error under strict_unknown_keys), message text, path, YAML line and the "did you mean" hint are identical by construction. They share MAX_FINDINGS, and the truncation note fires on the combined total — including the case where a single additionalProperties: false error fans out into more findings than the cap, which the islice on iter_errors does not bound. The walk carries its own budget, charged per key inspected and per node visited: the document is PR-controlled, and while PyYAML builds an anchored mapping once, an aliased one is walkable as a far larger tree, including through lists, which name no key at all. A walk that stops early — on the budget or the depth cap — reports that as a finding at the unknown-key severity, so it is a warning by default and a failure under strict_unknown_keys: a scan that could not rule out unknown keys must not hand back a green check to a repo that asked for unknown keys to fail.

Verification, against the artifacts the ticket names

The reported miss, reproduced first. On origin/main, reviews: {profil: chill}, reviews.tools.golangci-lint.enabld, and reviews.tools.github-checks.timeout_ms each returned zero findings in default and strict mode. After the change each returns exactly one, at the right path and the right line.

Acceptance, verbatim. A config containing only reviews.tools.github-checks.timeout_ms warns and exits 0 by default; exits 1 under --strict-unknown-keys.

Suites. coderabbit-config 72 tests pass (45 pre-existing, unchanged — none asserted the old miss — plus 27 added across the original change and the review round). All 8 existing CLI smoke steps from the test workflow, executed verbatim, pass; a 9th is added for this class. Also green: workflow-pins (78 + the lint itself, 10 workflows clean), agents-md-integrity (46), cursor-review (194), groom (365), refresh-reviewers (48), shellcheck on the bumper scripts.

Sized against the real corpus, not a fixture. I swept 194 non-archived org repos read-only; 20 carry a .coderabbit.yaml. Across all 20, findings go from 1 → 15. Every one of the 14 new findings is the same key, reviews.tools.github-checks.timeout_ms — i.e. zero false positives on real-world configs, which is the number that matters for a check other repos' CI depends on. All 20 still exit 0 in default mode, so enrolling this turns nothing red; 15 would exit 1 under strict_unknown_keys, which is opt-in and is exactly what the new rollout note in the caller doc warns about. Repo names are withheld — most are private and this repo is public; of the six public ones, ComfyUI, comfy-python-sdk, comfy-api-proxy, comfy-typescript-sdk and ComfyUI_prev carry the stale key.

The "stale key" claim, falsified rather than assumed. I fetched the live upstream schema and checked it directly: reviews.tools.github-checks names only enabled, declares no additionalProperties, and upstream today has the same 5-explicit / 103-by-omission split as the vendored copy. The repo's own schema_drift.py against live upstream reports no property or maxLength changes — the drift is cosmetic and touches nothing this walk's behaviour depends on. (Moving the vendored pin is refresh-coderabbit-schema.yml's job, not this PR's.)

Judgment calls

The shared finding builder changes one pre-existing behaviour, deliberately. A non-string YAML key is now named by its source spelling, read off the composed key node, on both paths. Previously such a key rendered through _format_path as a list index ([1] for 1: x) and the line lookup compared an int against the composed node's string value and returned no line. An intermediate str() coercion fixed the list-index half but not the rest: safe_load resolves plain true/on/yes to True, null to None and 0x10 to 16, so those keys were annotated reviews.True / reviews.None / reviews.16 — paths naming a key that appears nowhere in the file — and still lost the line, because the lookup then compared "True" against the node's "true". Matching key nodes tag-aware (constructing them the way safe_load did, which also keeps a quoted "true" from matching the boolean) fixes the name and the line together. str() survives only as the fallback for a key whose node cannot be found, e.g. an aliased or merge-keyed mapping.

The "did you mean" hint stays silent rather than guessing. It was written for the specific-name case, where the one other place the schema uses a name is almost certainly the intended home — the root-level tools: block. Keys found by the by-omission walk are dominated by generic names (enabled sits at ~72 places in the schema, plus scope, path, instructions), for which the alphabetically-first three are three wrong homes. The shortlist now prefers candidates under the offending key's own parent prefix and suppresses the hint past three remaining candidates; the motivating tools: → reviews.tools case is pinned by a test so the suppression cannot swallow it.

Two new tests pin schema facts (5 open / 103 closed-by-omission, and that only additionalProperties is used today). They will fail on a schema refresh that changes either. That is intentional and matches the existing SchemaTest convention in this file — a refresh that redefines what this check means should say so out loud in its own PR.

Bound by key-inspections and node-visits, not one or the other. A node-only budget is the obvious choice and the wrong one: one visit to a mapping with 100k keys costs 100k, so a node cap alone leaves the work superlinear in an adversarial document. But a key-only budget leaves list traversal outside the bound entirely — elements recurse without naming a key, and an alias-amplified nested list can charge 1 for an outer entry while thousands of element visits below it are free. Charging both is what makes the bound cover the aliased-list shape. The "did not see the whole document" flag is set where the walk actually bails rather than derived from the leftover budget afterwards, so an exact landing on the budget is a complete walk and a depth cutoff is a partial one; stopping on the findings cap is neither, and is reported as truncation instead. The depth cap remains belt-and-braces — descent follows the schema, which is finite and shallow, and $ref is an opener this walk never resolves.

Residual, and what stays open

The 14 org repos carrying reviews.tools.github-checks.timeout_ms are not cleaned by this PR. Each is a one-line edit in a different repository; this PR is scoped to one repo. They warn (exit 0) once enrolled, and the caller doc now tells an operator to clear them before switching on strict_unknown_keys.
How many enrolled callers this affects, I cannot state. The CODERABBIT_CONFIG_CALLERS roster is a repo secret by design and is not readable from here. The org-wide numbers above are the upper bound on impact, not the enrolled count.
AGENTS.md is 273 lines, over the 200-line ceiling its own standard enforces. Pre-existing on main (273 there too) and untouched by this change — my edit is deliberately net-zero on line count. This repo does not self-enroll agents-md-integrity.yml, so nothing catches it in CI. Out of scope to fix here, but it is a real finding on a file this change touches.
Inherited, still-unverified premise. That CodeRabbit strips an unknown key rather than rejecting the file is the severity split's foundation and comes from the original runtime observation, not from anything I observed. Same disclosure the check shipped with; if it turns out to be file-rejecting, strict_unknown_keys is the switch and the default should flip. This change widens where that premise applies, so it widens the blast radius of it being wrong.
Unexercised artifacts. The ticket references a prior read-only investigation and its findings comment; neither is reachable from this sandbox, so I worked from the description reproduced in my brief and re-derived its numbers independently (the 5/103 split and the stale-key corpus above are my own measurements, and they agree with it — except on repo count, where the ticket says six and I measure fourteen today).

Refs BE-7786 — the ticket stays open: the stale keys it names as evidence remain in the org's repos, unfixed by this PR, and the enrolled-caller count is unverifiable from here.

Provenance

Authored by: agent-work loop
Verified: re-run on the review-round commit — coderabbit-config unittest 72 passed / 0 failed, and 72 again on the tree merged with main (so no semantic conflict with the base); the test workflow's CLI smoke steps executed verbatim, all pass; agents-md-integrity 46 passed; cursor-review 194 passed; groom 365 passed (1 skipped); each of the eight panel findings reproduced against the pre-fix checker before being fixed and re-run after; worst-case key-lookup timing measured against the pre-fix build (no regression). Carried from the authoring run and not re-run here, since this round touches no workflow or shell file: workflow-pins 78 passed + check_workflow_pins.py clean (10 workflows); refresh-reviewers 48 passed; shellcheck -x on bump-callers.sh + preflight.sh clean; checker run read-only over 20 real org .coderabbit.yaml files (all exit 0 default; 1 → 15 findings, all 14 new ones the same genuine stale key)
Deviations: none on the ticket's acceptance criteria; the residuals above are out-of-scope items, not skipped criteria. The review round changed three behaviours the original body described differently — the bounded-walk report escalates under strict mode instead of staying a note, non-string keys are named by source spelling instead of str(), and the walk budget covers node visits as well as keys — and the prose above has been corrected to match rather than left describing the first round.

_{🔄 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/188 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7786-coderabbit-config-unknown-keys` --- ### 📝 Commits (3) - [`4b011a4`](https://github.com/Comfy-Org/github-workflows/commit/4b011a4ffc61e6f15a1a0cfac08ce07f393b414a) feat(coderabbit-config): flag unknown keys inside schema-open objects (BE-7786) - [`72aa76d`](https://github.com/Comfy-Org/github-workflows/commit/72aa76d3931a403a50ed1d9e99acf95df9035738) fix(coderabbit-config): cap findings after fan-out, name keys by source spelling, bound list traversal (BE-7786) - [`350dd93`](https://github.com/Comfy-Org/github-workflows/commit/350dd9315eb77eb30448a040f8ceb05cef30a48b) docs(coderabbit-config): document the partial-scan severity row; correct a stale test comment (BE-7786) ### 📊 Changes **6 files changed** (+823 additions, -44 deletions) <details> <summary>View changed files</summary> 📝 `.github/coderabbit-config/README.md` (+45 -11) 📝 `.github/coderabbit-config/check_coderabbit_config.py` (+300 -23) 📝 `.github/coderabbit-config/tests/test_check_coderabbit_config.py` (+395 -2) 📝 `.github/workflows/test-coderabbit-config-validate.yml` (+30 -0) 📝 `AGENTS.md` (+2 -2) 📝 `docs/callers/coderabbit-config-validate.md` (+51 -6) </details> ### 📄 Description ## ELI-5 CodeRabbit throws away any key in `.coderabbit.yaml` it doesn't recognize. The checker only *told* you about that in five places — the handful of spots where the schema explicitly says "nothing else allowed here". Everywhere else it said nothing, because the JSON Schema library it leans on had no rule to trip. So `reviews: {profil: chill}` — one letter off, and every setting under it silently inert — came back clean, in both normal and strict mode. This makes the check look at the other 103 objects too. It reports the same thing, the same way, in the same words; it just no longer waits for the schema to spell out a rule it can restate. ## The rule, and why it is drawn this way Walking the parsed config alongside the schema, an object is treated as **closed by omission** only when it declares a `properties` block and **none** of `additionalProperties`, `unevaluatedProperties`, `patternProperties`, `anyOf`, `oneOf`, `allOf`, `$ref`, `$dynamicRef`, `if`, `dependentSchemas`, `propertyNames`, `not`. Each of those is a way an object can legitimately hold a name its `properties` does not list, so reading any of them as "closed" would invent a warning on a valid config — the failure mode that matters most here, because this check gates other repos' CI. Today's schema uses exactly one of the twelve, so eleven entries are pure future-proofing against a vendored-schema bump. The walk descends only through matched properties and through `items`; never into a combinator branch, an `additionalProperties` schema, or the value under a key it just reported (that key has no subschema, so nothing beneath it is judgeable). Where the document and the schema disagree about shape it stays silent — that's a type error and jsonschema already describes it, in better words than a second complaint would. **No double-reporting, by construction rather than by de-duplication.** jsonschema fires only where the keyword is present; the walk only where it is absent. Complements, not overlapping sets. `test_the_two_halves_never_report_the_same_key` exercises both at once on one config so a future refactor that merged them fails here instead of double-annotating someone's PR. That last claim is stated as a fact about **this** vendored schema, not as a property of the two conditions — they are complements at a *single* node, but jsonschema evaluates a document path against every applicable subschema, so a node this walk judges closed whose `$ref` target or combinator branch carried `additionalProperties: false` would be reported by both halves. `OpennessTest` pins the two premises that rule it out today: the schema carries no `$ref`, and no node declaring `properties` also carries a combinator (so such a node counts as OPEN here and is never judged closed). Findings from both halves go through **one** builder, so severity (`warning`, or `error` under `strict_unknown_keys`), message text, path, YAML line and the "did you mean" hint are identical by construction. They share `MAX_FINDINGS`, and the truncation note fires on the combined total — including the case where a *single* `additionalProperties: false` error fans out into more findings than the cap, which the `islice` on `iter_errors` does not bound. The walk carries its own budget, charged per key inspected **and** per node visited: the document is PR-controlled, and while PyYAML builds an anchored mapping once, an aliased one is *walkable* as a far larger tree, including through lists, which name no key at all. A walk that stops early — on the budget or the depth cap — reports that as a finding at the unknown-key severity, so it is a warning by default and a **failure** under `strict_unknown_keys`: a scan that could not rule out unknown keys must not hand back a green check to a repo that asked for unknown keys to fail. ## Verification, against the artifacts the ticket names **The reported miss, reproduced first.** On `origin/main`, `reviews: {profil: chill}`, `reviews.tools.golangci-lint.enabld`, and `reviews.tools.github-checks.timeout_ms` each returned **zero** findings in default *and* strict mode. After the change each returns exactly one, at the right path and the right line. **Acceptance, verbatim.** A config containing only `reviews.tools.github-checks.timeout_ms` warns and exits **0** by default; exits **1** under `--strict-unknown-keys`. **Suites.** coderabbit-config 72 tests pass (45 pre-existing, unchanged — none asserted the old miss — plus 27 added across the original change and the review round). All 8 existing CLI smoke steps from the test workflow, executed verbatim, pass; a 9th is added for this class. Also green: workflow-pins (78 + the lint itself, 10 workflows clean), agents-md-integrity (46), cursor-review (194), groom (365), refresh-reviewers (48), `shellcheck` on the bumper scripts. **Sized against the real corpus, not a fixture.** I swept **194** non-archived org repos read-only; **20** carry a `.coderabbit.yaml`. Across all 20, findings go from **1 → 15**. Every one of the 14 new findings is the *same* key, `reviews.tools.github-checks.timeout_ms` — i.e. **zero false positives on real-world configs**, which is the number that matters for a check other repos' CI depends on. All 20 still exit **0** in default mode, so enrolling this turns nothing red; 15 would exit 1 under `strict_unknown_keys`, which is opt-in and is exactly what the new rollout note in the caller doc warns about. Repo names are withheld — most are private and this repo is public; of the six public ones, `ComfyUI`, `comfy-python-sdk`, `comfy-api-proxy`, `comfy-typescript-sdk` and `ComfyUI_prev` carry the stale key. **The "stale key" claim, falsified rather than assumed.** I fetched the live upstream schema and checked it directly: `reviews.tools.github-checks` names only `enabled`, declares no `additionalProperties`, and upstream today has the same 5-explicit / 103-by-omission split as the vendored copy. The repo's own `schema_drift.py` against live upstream reports **no property or `maxLength` changes** — the drift is cosmetic and touches nothing this walk's behaviour depends on. (Moving the vendored pin is `refresh-coderabbit-schema.yml`'s job, not this PR's.) ## Judgment calls **The shared finding builder changes one pre-existing behaviour, deliberately.** A non-string YAML key is now named by its **source spelling**, read off the composed key node, on *both* paths. Previously such a key rendered through `_format_path` as a list index (`[1]` for `1: x`) and the line lookup compared an int against the composed node's string value and returned no line. An intermediate `str()` coercion fixed the list-index half but not the rest: `safe_load` resolves plain `true`/`on`/`yes` to `True`, `null` to `None` and `0x10` to `16`, so those keys were annotated `reviews.True` / `reviews.None` / `reviews.16` — paths naming a key that appears nowhere in the file — and still lost the line, because the lookup then compared `"True"` against the node's `"true"`. Matching key nodes tag-aware (constructing them the way `safe_load` did, which also keeps a *quoted* `"true"` from matching the boolean) fixes the name and the line together. `str()` survives only as the fallback for a key whose node cannot be found, e.g. an aliased or merge-keyed mapping. **The "did you mean" hint stays silent rather than guessing.** It was written for the specific-name case, where the one other place the schema uses a name is almost certainly the intended home — the root-level `tools:` block. Keys found by the by-omission walk are dominated by *generic* names (`enabled` sits at ~72 places in the schema, plus `scope`, `path`, `instructions`), for which the alphabetically-first three are three wrong homes. The shortlist now prefers candidates under the offending key's own parent prefix and suppresses the hint past three remaining candidates; the motivating `tools:` → `reviews.tools` case is pinned by a test so the suppression cannot swallow it. **Two new tests pin schema facts (5 open / 103 closed-by-omission, and that only `additionalProperties` is used today).** They will fail on a schema refresh that changes either. That is intentional and matches the existing `SchemaTest` convention in this file — a refresh that redefines what this check *means* should say so out loud in its own PR. **Bound by key-inspections *and* node-visits, not one or the other.** A node-only budget is the obvious choice and the wrong one: one visit to a mapping with 100k keys costs 100k, so a node cap alone leaves the work superlinear in an adversarial document. But a key-only budget leaves list traversal outside the bound entirely — elements recurse without naming a key, and an alias-amplified nested list can charge 1 for an outer entry while thousands of element visits below it are free. Charging both is what makes the bound cover the aliased-list shape. The "did not see the whole document" flag is set where the walk actually bails rather than derived from the leftover budget afterwards, so an exact landing on the budget is a *complete* walk and a depth cutoff is a *partial* one; stopping on the findings cap is neither, and is reported as truncation instead. The depth cap remains belt-and-braces — descent follows the schema, which is finite and shallow, and `$ref` is an opener this walk never resolves. ## Residual, and what stays open - **The 14 org repos carrying `reviews.tools.github-checks.timeout_ms` are not cleaned by this PR.** Each is a one-line edit in a different repository; this PR is scoped to one repo. They warn (exit 0) once enrolled, and the caller doc now tells an operator to clear them *before* switching on `strict_unknown_keys`. - **How many enrolled callers this affects, I cannot state.** The `CODERABBIT_CONFIG_CALLERS` roster is a repo secret by design and is not readable from here. The org-wide numbers above are the upper bound on impact, not the enrolled count. - **`AGENTS.md` is 273 lines, over the 200-line ceiling its own standard enforces.** Pre-existing on `main` (273 there too) and untouched by this change — my edit is deliberately net-zero on line count. This repo does not self-enroll `agents-md-integrity.yml`, so nothing catches it in CI. Out of scope to fix here, but it is a real finding on a file this change touches. - **Inherited, still-unverified premise.** That CodeRabbit *strips* an unknown key rather than rejecting the file is the severity split's foundation and comes from the original runtime observation, not from anything I observed. Same disclosure the check shipped with; if it turns out to be file-rejecting, `strict_unknown_keys` is the switch and the default should flip. This change widens *where* that premise applies, so it widens the blast radius of it being wrong. - **Unexercised artifacts.** The ticket references a prior read-only investigation and its findings comment; neither is reachable from this sandbox, so I worked from the description reproduced in my brief and re-derived its numbers independently (the 5/103 split and the stale-key corpus above are my own measurements, and they agree with it — except on repo count, where the ticket says six and I measure fourteen today). Refs BE-7786 — the ticket stays open: the stale keys it names as evidence remain in the org's repos, unfixed by this PR, and the enrolled-caller count is unverifiable from here. ## Provenance - **Authored by:** agent-work loop - **Verified:** re-run on the review-round commit — coderabbit-config unittest 72 passed / 0 failed, and 72 again on the tree merged with `main` (so no semantic conflict with the base); the test workflow's CLI smoke steps executed verbatim, all pass; agents-md-integrity 46 passed; cursor-review 194 passed; groom 365 passed (1 skipped); each of the eight panel findings reproduced against the pre-fix checker before being fixed and re-run after; worst-case key-lookup timing measured against the pre-fix build (no regression). Carried from the authoring run and not re-run here, since this round touches no workflow or shell file: workflow-pins 78 passed + `check_workflow_pins.py` clean (10 workflows); refresh-reviewers 48 passed; `shellcheck -x` on `bump-callers.sh` + `preflight.sh` clean; checker run read-only over 20 real org `.coderabbit.yaml` files (all exit 0 default; 1 → 15 findings, all 14 new ones the same genuine stale key) - **Deviations:** none on the ticket's acceptance criteria; the residuals above are out-of-scope items, not skipped criteria. The review round changed three behaviours the original body described differently — the bounded-walk report escalates under strict mode instead of staying a note, non-string keys are named by source spelling instead of `str()`, and the walk budget covers node visits as well as keys — and the prose above has been corrected to match rather than left describing the first round. --- <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:16 +00:00