[PR #192] [MERGED] fix(coderabbit-config): rank findings under the cap, index key nodes once per mapping (BE-7786) #200

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-7786-coderabbit-config-cap-ranking

📝 Commits (3)

9e7d55c fix(coderabbit-config): rank findings under the cap, index key nodes once per mapping (BE-7786)
e257047 fix(coderabbit-config): keep the walk budget bounding WORK, expand merges transitively (BE-7786)
b88e7f2 fix(coderabbit-config): bound the merge expansion, key the descent on the node (BE-7786)

📊 Changes

4 files changed (+757 additions, -82 deletions)

View changed files

📝 .github/coderabbit-config/README.md (+9 -6)
📝 .github/coderabbit-config/check_coderabbit_config.py (+356 -69)
📝 .github/coderabbit-config/tests/test_check_coderabbit_config.py (+382 -2)
📝 docs/callers/coderabbit-config-validate.md (+10 -5)

📄 Description

Follow-up to #188, which merged while its round-2 review was still running. Two of the eight findings that round raised are regressions in #188's own review-round fix, and both are now live on main — they affect every consumer repo running coderabbit-config-validate.yml.

The two that matter

A file-rejecting error could be capped away, flipping the exit code to 0. #188 re-applied MAX_FINDINGS by slicing the finding list, which is in error-sort order — and the root additionalProperties error has an empty absolute_path, so it sorts first and fans out to one finding per unknown key. A config with ~150 unknown root keys plus one maxLength violation therefore sliced the file-rejecting error away, leaving 100 warnings and exit 0 on a config CodeRabbit rejects whole. Findings now carry a rank and _apply_cap drops the lowest first, so a file-rejecting error is never crowded out by warnings that happened to sort ahead of it. The truncation note could not have covered this: a note does not reach the exit code — the same argument #188 used to turn the bounded-walk report into a finding.

Verified end to end: that input now exits 1, with the maxLength finding present among the 100.

The cap bounded the output but not the work. _extra_keys returns every extra key in the mapping, and each finding rescanned all of parent.value, constructing every candidate key node on the way. k00000: 1 is ten bytes, so ~50k unknown root keys fit inside MAX_CONFIG_BYTES at ~1e9 constructor invocations — the job burns runner minutes to its timeout instead of printing 100 annotations. #188 made this materially worse: it replaced a cheap string compare with a SafeConstructor + construct_object(deep=True) per candidate. The fan-out now stops at the cap, and the resolved-key → node map is built once per mapping and cached on the composed node. 20k unknown keys: 1.6s.

The rest

The incomplete-scan report was appended after both truncation sites, so a run that filled the cap emitted MAX_FINDINGS + 1 findings alongside a note saying only the first MAX_FINDINGS are reported. It goes through the same cap now, ranked above unknown keys so it is not the one dropped.
_suggest_home compared _format_path's [0] against _index_schema_paths' [], so for a key under a list element neither the parent preference nor the self-exclusion could ever match. Both vocabularies are normalized before comparing.
Two key shapes still fell back to str(), reproducing the "path naming a key that appears nowhere in the file" defect #188 set out to fix: a .nan: key (NaN is not equal to itself, so it missed its own index entry) and a merge-keyed mapping (safe_load flattens <<: *anchor, but the composed node keeps only <<). The index normalizes NaN and expands merge sources, with explicit keys keeping YAML's own precedence over merged ones, and a cycle guard for &x [*x].
bounded was set on entry to walk, so spending the last budget unit on a known key whose value is a scalar (reviews.profile: chill) flagged the trailing visit as a partial walk — a fully walked document reported as stopped early, which is a hard error under strict_unknown_keys. A node with nothing to inspect now returns before the bail-out.
The cutoff message said "after MAX_WALK_KEYS keys" while the counter also charges node visits; and the docs claimed the five explicitly-closed objects were exempt from the vendored-schema staleness window. They are not — a key upstream adds at the root is missing from the vendored properties list and the vendored additionalProperties: false rejects it just the same. Both corrected.

Round 3 — the review panel's findings on the fixes above

All six threads on this PR are addressed in e257047.

The budget stopped bounding the work. Exempting a leaf visit from the bail-out FLAG also exempted it from the BUDGET, and the list branch recurses once per element with no budget check of its own — so MAX_WALK_KEYS bounded non-empty containers, not walk() calls. A 1.6 KB aliased document over a nested-items schema took 1.01 s and reported bounded=False ("I saw the whole thing"); it is 0.03 s now. Only the flag is gated on there being something to inspect; the visit is charged either way, which is the invariant test_list_traversal_is_charged_against_the_walk_budget was written to hold. (The panel's 1e9 figure overstates it for this schema, which has no nested items — the vendored-schema worst case measures 1.71 s. The bound is still worth restoring: refresh-coderabbit-schema.yml moves the schema automatically and the walk is written to be schema-independent.)
Merges are transitive; the expansion was not. PyYAML's flatten_mapping recurses before splicing, so a: &a {x: 1} / b: &b {<<: *a} / reviews: {<<: *b} puts x under reviews in the loaded document while the index got a literal "<<" — the str() fallback with no line number, one level deeper. _merge_entries now recurses, with a merged mapping's own keys ahead of what it merged in.
The descent was merge-unaware too. _descend compared raw key text, so an ancestor arriving through a merge ended the walk at <<: the same defect one level up. The index now carries the value node and _descend routes through it — which also stops on:/true: being read as a list index (a bool IS an int in Python).
The islice runs a stage before ranking. Unranked FIFO over iter_errors, so it could in principle drop a file-rejecting error before _apply_cap can protect it. Not reachable against this schema — checked, and for a different reason than the panel gave: maxItems does not stop element validation (150 bad linked_repositories really do yield 150 errors), but any such document also trips maxItems: 20, which is itself file-rejecting. The real premise is the bound: only a literal false yields a stripped-keyword error, and every self-closing object is either unmultiplied or under a maxItems-bounded array (4 + 20 = 24). Now pinned as test_a_rejecting_error_cannot_be_crowded_out_before_ranking, which computes that bound from the schema.
The truncation note still described the survivors as a prefix after ranking made them not one, and _index_key's summary line named a None sentinel the function never returns. Both corrected.

Round 4 — the panel's findings on the round-3 fixes

All six threads addressed in b88e7f2.

The transitive merge expansion had no depth bound. A <<: chain of ~1500 links is ~37 KB, far inside MAX_CONFIG_BYTES; safe_load flattens it fine while this expansion, costing more frames per link, blew the recursion limit — and RecursionError is not a yaml.YAMLError, so it escaped validate and killed an advisory check with a traceback. Bounded at _MAX_MERGE_DEPTH, degrading to the documented str() fallback. PyYAML's own flatten_mapping can lose the same race first (anchors in a lazily-constructed sequence, so reviews flattens before them) — that path was already unguarded on main, and now raises ConfigError and exits 2, like an oversized config: "I could not check this" must not read as "this is fine".
_descend typed the path PART instead of the node, so an integer mapping key went down the sequence branch and died there. Reachable today: reviews.mutually_exclusive_groups takes arbitrary property names, so 1: is legal — reviews.mutually_exclusive_groups[1] with no line became reviews.mutually_exclusive_groups.1 at line 4. _format_path is given the document too, so it renders an integer as [n] only when the container really is a sequence.
The index was first-wins throughout, but construct_mapping assigns per pair, so safe_load keeps the LAST. A duplicate key annotated its earlier twin's line; two <<: keys in one mapping resolved to the earlier one while the document kept the later. Explicit keys assign unconditionally now, and merge groups are consumed in reverse.
The premise test modelled maxItems as a multiplicity bound, which it is not — jsonschema keeps validating elements past it. Reformulated to assert the four facts that actually hold, including that maxItems precedes items in key order so its rejecting error is emitted ahead of the element errors. Mutation-tested: all four clauses fail when broken; the old form was silent on all four.
_apply_cap's docstring claimed a document-order tie-break; the halves are concatenated, so it is emission order. Corrected, with why the walk's overflow finding is deliberately the first dropped.
Declined with a reply: every NaN key collapsing into one _NAN_KEY slot. Distinguishing .nan: from .NaN: would need positional matching between the loaded dict and the composed node — the constructed float cannot be bridged by identity or by equality, which is why the sentinel exists. Cosmetic, on pathological input.

Verification

99 tests, up from 72 — every case above is pinned by a test named for the defect, including the two High ones (test_a_file_rejecting_error_is_never_crowded_out_by_unknown_keys, test_the_unknown_key_fan_out_stops_working_at_the_cap, test_the_key_index_is_built_once_per_mapping), the round-3 High (test_scalar_elements_are_charged_against_the_walk_budget) and the round-4 Mediums (test_a_merge_chain_the_loader_itself_cannot_build_exits_two, test_an_integer_mapping_key_is_not_read_as_a_list_index). Every behavioural round-3 and round-4 test was re-run against the pre-fix checker and fails there. The test workflow's CLI smoke steps were re-run verbatim, plus an end-to-end assertion that the flood-plus-maxLength input exits 1. Also green: agents-md-integrity (46), cursor-review (194), groom (365, 1 skipped), refresh-reviewers (48), workflow-pins (78 + lint).

Refs BE-7786.

Provenance

Authored by: agent-work loop
Verified: coderabbit-config unittest 99 passed / 0 failed against the pinned jsonschema==4.26.0 + PyYAML==6.0.3, on this branch level with current main (0 behind); the test workflow's 11 CLI smoke steps executed verbatim, all pass; every behavioural round-3 and round-4 test re-run against the pre-fix checker and confirmed failing there (5 of 5 for round 4); the reformulated schema-premise test mutation-tested against four broken-schema variants, all four caught; walk cost measured 1.01 s → 0.03 s on the nested-items shape, 512 KiB worst cases 1.74–1.83 s, 50k-key index build 0.03 s; the merge-chain recursion window measured (1500 links crash before the fix, 900 do not) and both crash paths — this file's and PyYAML's own — confirmed handled; agents-md-integrity 46 passed; cursor-review 194 passed; groom 365 passed (1 skipped); refresh-reviewers 48 passed; workflow-pins 78 passed + lint clean
Deviations: three, each stated on its thread. (1) The merge-tag filter asked for in _resolved_key_index's merged-entries loop lives in _merge_entries instead — the single point that expands merges — so merged cannot carry a << entry by construction rather than by an unreachable branch. (2) _apply_cap's emission-order tie-break is documented rather than changed: the walk's overflow finding exists to set truncated, and tagging walk findings with document positions would cost the stable per-half ordering the concatenation buys. (3) The NaN-key collapse is declined, not deferred — cosmetic on pathological input, below the follow-up bar. Separately: AGENTS.md is 273 lines and over the checker's 200-line ceiling — pre-existing on main, untouched here, and path-filtered so it does not gate this PR.

_{🔄 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/192 **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-cap-ranking` --- ### 📝 Commits (3) - [`9e7d55c`](https://github.com/Comfy-Org/github-workflows/commit/9e7d55c3fd0eff0733559d01c285b13c70992a08) fix(coderabbit-config): rank findings under the cap, index key nodes once per mapping (BE-7786) - [`e257047`](https://github.com/Comfy-Org/github-workflows/commit/e257047604ba2fe1a65031b2d52833fee53a129a) fix(coderabbit-config): keep the walk budget bounding WORK, expand merges transitively (BE-7786) - [`b88e7f2`](https://github.com/Comfy-Org/github-workflows/commit/b88e7f21b5bacd531a096acbc1417bcbc444e467) fix(coderabbit-config): bound the merge expansion, key the descent on the node (BE-7786) ### 📊 Changes **4 files changed** (+757 additions, -82 deletions) <details> <summary>View changed files</summary> 📝 `.github/coderabbit-config/README.md` (+9 -6) 📝 `.github/coderabbit-config/check_coderabbit_config.py` (+356 -69) 📝 `.github/coderabbit-config/tests/test_check_coderabbit_config.py` (+382 -2) 📝 `docs/callers/coderabbit-config-validate.md` (+10 -5) </details> ### 📄 Description Follow-up to #188, which merged while its round-2 review was still running. Two of the eight findings that round raised are **regressions in #188's own review-round fix**, and both are now live on `main` — they affect every consumer repo running `coderabbit-config-validate.yml`. ## The two that matter **A file-rejecting error could be capped away, flipping the exit code to 0.** #188 re-applied `MAX_FINDINGS` by slicing the finding list, which is in error-sort order — and the root `additionalProperties` error has an *empty* `absolute_path`, so it sorts first and fans out to one finding per unknown key. A config with ~150 unknown root keys plus one `maxLength` violation therefore sliced the file-rejecting error away, leaving 100 warnings and **exit 0 on a config CodeRabbit rejects whole**. Findings now carry a rank and `_apply_cap` drops the lowest first, so a file-rejecting error is never crowded out by warnings that happened to sort ahead of it. The truncation note could not have covered this: a note does not reach the exit code — the same argument #188 used to turn the bounded-walk report into a finding. Verified end to end: that input now exits **1**, with the `maxLength` finding present among the 100. **The cap bounded the output but not the work.** `_extra_keys` returns every extra key in the mapping, and each finding rescanned all of `parent.value`, constructing every candidate key node on the way. `k00000: 1` is ten bytes, so ~50k unknown root keys fit inside `MAX_CONFIG_BYTES` at ~1e9 constructor invocations — the job burns runner minutes to its timeout instead of printing 100 annotations. #188 made this materially worse: it replaced a cheap string compare with a `SafeConstructor` + `construct_object(deep=True)` per candidate. The fan-out now stops at the cap, and the resolved-key → node map is built **once per mapping** and cached on the composed node. 20k unknown keys: 1.6s. ## The rest - The incomplete-scan report was appended *after* both truncation sites, so a run that filled the cap emitted `MAX_FINDINGS + 1` findings alongside a note saying only the first `MAX_FINDINGS` are reported. It goes through the same cap now, ranked above unknown keys so it is not the one dropped. - `_suggest_home` compared `_format_path`'s `[0]` against `_index_schema_paths`' `[]`, so for a key under a list element neither the parent preference nor the self-exclusion could ever match. Both vocabularies are normalized before comparing. - Two key shapes still fell back to `str()`, reproducing the "path naming a key that appears nowhere in the file" defect #188 set out to fix: a `.nan:` key (NaN is not equal to itself, so it missed its own index entry) and a merge-keyed mapping (`safe_load` flattens `<<: *anchor`, but the composed node keeps only `<<`). The index normalizes NaN and expands merge sources, with explicit keys keeping YAML's own precedence over merged ones, and a cycle guard for `&x [*x]`. - `bounded` was set on *entry* to `walk`, so spending the last budget unit on a known key whose value is a scalar (`reviews.profile: chill`) flagged the trailing visit as a partial walk — a fully walked document reported as stopped early, which is a **hard error** under `strict_unknown_keys`. A node with nothing to inspect now returns before the bail-out. - The cutoff message said "after `MAX_WALK_KEYS` keys" while the counter also charges node visits; and the docs claimed the five explicitly-closed objects were exempt from the vendored-schema staleness window. They are not — a key upstream adds at the root is missing from the vendored `properties` list and the vendored `additionalProperties: false` rejects it just the same. Both corrected. ## Round 3 — the review panel's findings on the fixes above All six threads on this PR are addressed in `e257047`. - **The budget stopped bounding the work.** Exempting a leaf visit from the bail-out FLAG also exempted it from the BUDGET, and the list branch recurses once per element with no budget check of its own — so `MAX_WALK_KEYS` bounded non-empty containers, not `walk()` calls. A 1.6 KB aliased document over a nested-`items` schema took **1.01 s and reported `bounded=False`** ("I saw the whole thing"); it is **0.03 s** now. Only the flag is gated on there being something to inspect; the visit is charged either way, which is the invariant `test_list_traversal_is_charged_against_the_walk_budget` was written to hold. (The panel's 1e9 figure overstates it for *this* schema, which has no nested `items` — the vendored-schema worst case measures 1.71 s. The bound is still worth restoring: `refresh-coderabbit-schema.yml` moves the schema automatically and the walk is written to be schema-independent.) - **Merges are transitive; the expansion was not.** PyYAML's `flatten_mapping` recurses before splicing, so `a: &a {x: 1}` / `b: &b {<<: *a}` / `reviews: {<<: *b}` puts `x` under `reviews` in the loaded document while the index got a literal `"<<"` — the `str()` fallback with no line number, one level deeper. `_merge_entries` now recurses, with a merged mapping's own keys ahead of what it merged in. - **The descent was merge-unaware too.** `_descend` compared raw key text, so an ancestor arriving through a merge ended the walk at `<<`: the same defect one level up. The index now carries the value node and `_descend` routes through it — which also stops `on:`/`true:` being read as a list index (a bool IS an int in Python). - **The `islice` runs a stage before ranking.** Unranked FIFO over `iter_errors`, so it could in principle drop a file-rejecting error before `_apply_cap` can protect it. Not reachable against this schema — checked, and for a different reason than the panel gave: `maxItems` does *not* stop element validation (150 bad `linked_repositories` really do yield 150 errors), but any such document also trips `maxItems: 20`, which is itself file-rejecting. The real premise is the bound: only a literal `false` yields a stripped-keyword error, and every self-closing object is either unmultiplied or under a `maxItems`-bounded array (4 + 20 = 24). Now pinned as `test_a_rejecting_error_cannot_be_crowded_out_before_ranking`, which computes that bound from the schema. - The truncation note still described the survivors as a prefix after ranking made them not one, and `_index_key`'s summary line named a `None` sentinel the function never returns. Both corrected. ## Round 4 — the panel's findings on the round-3 fixes All six threads addressed in `b88e7f2`. - **The transitive merge expansion had no depth bound.** A `<<:` chain of ~1500 links is ~37 KB, far inside `MAX_CONFIG_BYTES`; `safe_load` flattens it fine while this expansion, costing more frames per link, blew the recursion limit — and `RecursionError` is not a `yaml.YAMLError`, so it escaped `validate` and killed an advisory check with a traceback. Bounded at `_MAX_MERGE_DEPTH`, degrading to the documented `str()` fallback. **PyYAML's own `flatten_mapping` can lose the same race first** (anchors in a lazily-constructed sequence, so `reviews` flattens before them) — that path was already unguarded on `main`, and now raises `ConfigError` and exits **2**, like an oversized config: "I could not check this" must not read as "this is fine". - **`_descend` typed the path PART instead of the node**, so an integer mapping key went down the sequence branch and died there. Reachable today: `reviews.mutually_exclusive_groups` takes arbitrary property names, so `1:` is legal — `reviews.mutually_exclusive_groups[1]` with no line became `reviews.mutually_exclusive_groups.1` at line 4. `_format_path` is given the document too, so it renders an integer as `[n]` only when the container really is a sequence. - **The index was first-wins throughout, but `construct_mapping` assigns per pair, so `safe_load` keeps the LAST.** A duplicate key annotated its earlier twin's line; two `<<:` keys in one mapping resolved to the earlier one while the document kept the later. Explicit keys assign unconditionally now, and merge groups are consumed in reverse. - **The premise test modelled `maxItems` as a multiplicity bound, which it is not** — jsonschema keeps validating elements past it. Reformulated to assert the four facts that actually hold, including that `maxItems` precedes `items` in key order so its *rejecting* error is emitted ahead of the element errors. Mutation-tested: all four clauses fail when broken; the old form was silent on all four. - `_apply_cap`'s docstring claimed a document-order tie-break; the halves are concatenated, so it is emission order. Corrected, with why the walk's overflow finding is deliberately the first dropped. - Declined with a reply: every NaN key collapsing into one `_NAN_KEY` slot. Distinguishing `.nan:` from `.NaN:` would need positional matching between the loaded dict and the composed node — the constructed float cannot be bridged by identity or by equality, which is why the sentinel exists. Cosmetic, on pathological input. ## Verification 99 tests, up from 72 — every case above is pinned by a test named for the defect, including the two High ones (`test_a_file_rejecting_error_is_never_crowded_out_by_unknown_keys`, `test_the_unknown_key_fan_out_stops_working_at_the_cap`, `test_the_key_index_is_built_once_per_mapping`), the round-3 High (`test_scalar_elements_are_charged_against_the_walk_budget`) and the round-4 Mediums (`test_a_merge_chain_the_loader_itself_cannot_build_exits_two`, `test_an_integer_mapping_key_is_not_read_as_a_list_index`). Every behavioural round-3 and round-4 test was re-run against the pre-fix checker and fails there. The test workflow's CLI smoke steps were re-run verbatim, plus an end-to-end assertion that the flood-plus-`maxLength` input exits 1. Also green: agents-md-integrity (46), cursor-review (194), groom (365, 1 skipped), refresh-reviewers (48), workflow-pins (78 + lint). Refs BE-7786. ## Provenance - **Authored by:** agent-work loop - **Verified:** coderabbit-config unittest 99 passed / 0 failed against the pinned `jsonschema==4.26.0` + `PyYAML==6.0.3`, on this branch level with current `main` (0 behind); the test workflow's 11 CLI smoke steps executed verbatim, all pass; every behavioural round-3 and round-4 test re-run against the pre-fix checker and confirmed failing there (5 of 5 for round 4); the reformulated schema-premise test mutation-tested against four broken-schema variants, all four caught; walk cost measured 1.01 s → 0.03 s on the nested-`items` shape, 512 KiB worst cases 1.74–1.83 s, 50k-key index build 0.03 s; the merge-chain recursion window measured (1500 links crash before the fix, 900 do not) and both crash paths — this file's and PyYAML's own — confirmed handled; agents-md-integrity 46 passed; cursor-review 194 passed; groom 365 passed (1 skipped); refresh-reviewers 48 passed; workflow-pins 78 passed + lint clean - **Deviations:** three, each stated on its thread. (1) The merge-tag filter asked for in `_resolved_key_index`'s merged-entries loop lives in `_merge_entries` instead — the single point that expands merges — so `merged` cannot carry a `<<` entry by construction rather than by an unreachable branch. (2) `_apply_cap`'s emission-order tie-break is documented rather than changed: the walk's overflow finding exists to set `truncated`, and tagging walk findings with document positions would cost the stable per-half ordering the concatenation buys. (3) The NaN-key collapse is declined, not deferred — cosmetic on pathological input, below the follow-up bar. Separately: `AGENTS.md` is 273 lines and over the checker's 200-line ceiling — pre-existing on `main`, untouched here, and path-filtered so it does not gate this PR. --- <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:17 +00:00