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

* fix(coderabbit-config): rank findings under the cap, index key nodes once per mapping (BE-7786)

Round-2 review findings. The first two are regressions in the round-1
fix itself, not in the original change.

* HIGH — the round-1 cap truncated 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. 150 unknown root
  keys plus one `maxLength` violation therefore sliced the file-rejecting
  error away and left 100 warnings: 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. The compensating
  note could not have covered this — a note does not reach the exit code,
  the same argument that turned the bounded-walk report into a finding.

* HIGH — 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 candidate keys. ~50k unknown root keys
  fit inside MAX_CONFIG_BYTES and cost ~1e9 constructions, so the job
  burned to its timeout instead of printing 100 annotations. 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 keys: 1.6s.

* The incomplete-scan report was appended after both truncation sites, so
  a run that filled the cap emitted MAX_FINDINGS + 1 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 neither the parent preference nor the
  self-exclusion could ever match for a key under a list element. Both
  vocabularies are normalized before the comparison.

* Two keys still fell back to `str()`, reproducing the defect round 1
  fixed: a `.nan:` key (NaN is not equal to itself, so it missed its own
  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 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` and its `additionalProperties: false` rejects it
  just the same. Both corrected.

87 tests (up from 72); every case above is pinned.

* fix(coderabbit-config): keep the walk budget bounding WORK, expand merges transitively (BE-7786)

Round-3 review findings on this PR's own fixes.

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` stopped bounding `walk()` calls and started
bounding only non-empty containers. A 1.6 KB aliased document over a
nested-`items` schema took 1.0 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 what
`test_list_traversal_is_charged_against_the_walk_budget` was written to hold.

`_merge_entries` stopped at one level while PyYAML's `flatten_mapping` is
transitive, so `a: &a {x: 1}` / `b: &b {<<: *a}` / `reviews: {<<: *b}` put
`x` under `reviews` in the loaded document and a literal `"<<"` in the index
— the `str()` fallback with no line number, one level deeper. And `_descend`
compared raw key text, so an ancestor arriving through a merge ended the
descent at `<<`: same defect one level up. The index now carries the value
node too and `_descend` routes through it, which also stops `on:`/`true:`
being read as a list index (a bool IS an int).

The truncation note still said "only the first N are reported" after ranking
made the survivors not a prefix, and `_index_key`'s summary line named a
`None` sentinel the function never returns.

`OpennessTest` gains the premise behind the one gap left open by design: the
`islice` over `iter_errors` is unranked FIFO and runs a stage before
`_apply_cap`, so it could in principle drop a file-rejecting error — it
cannot against this schema because every self-closing object is either
unmultiplied or under a `maxItems`-bounded array, and a refresh that changes
that now fails here.

93 tests, up from 87.

* fix(coderabbit-config): bound the merge expansion, key the descent on the node (BE-7786)

Round-4 review findings.

The transitive merge expansion had no depth bound. A `<<:` chain of ~1500
links is ~37 KB — far inside MAX_CONFIG_BYTES — and `safe_load` flattens it
fine while this expansion, costing more frames per link, blew the recursion
limit; `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 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 and a rejecting error below it lost its line and rendered as
`...[1]` — a list position the file does not have. `_descend` now dispatches
on the node and `_format_path` is given the document so it can tell an
integer key from a list index.

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, and where a mapping carries two `<<:` keys the earlier one won
here and the later one won in the document. Explicit keys now assign
unconditionally and merge groups are consumed in reverse.

`_apply_cap`'s docstring claimed a document-order tie-break; the two halves
are concatenated, so it is emission order — corrected, with why the walk's
overflow finding is deliberately the first dropped.

`test_a_rejecting_error_cannot_be_crowded_out_before_ranking` 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. Verified by
mutation: all four clauses fail when broken.

99 tests, up from 93.

Author: mattmillerai

.github/coderabbit-config/README.md

@@ -73,13 +73,16 @@ pins the two premises that rule it out today — the schema carries no `$ref`, a
 no node declaring `properties` also carries a combinator — so a schema refresh
 that breaks either fails there rather than double-annotating a real PR.
 
-**The by-omission half is only as current as the vendored schema.** It reads those
-103 objects as closed against the copy committed here, so a property upstream ADDS
-is an unknown key until `refresh-coderabbit-schema.yml` lands the bump and each
-caller's pin moves. Warn-only mode absorbs that as a warning; a caller running
+**The unknown-key check is only as current as the vendored schema.** Every object
+is judged against the copy committed here, so a property upstream ADDS is an
+unknown key until `refresh-coderabbit-schema.yml` lands the bump and each caller's
+pin moves. Warn-only mode absorbs that as a warning; a caller running
 `strict_unknown_keys: true` gets a hard failure on a key it was right to add. Same
-freshness, opposite sign, as catching a key upstream removed — see the caller doc's
-rollout section.
+freshness, opposite sign, as catching a key upstream removed. Neither half is
+exempt — a root-level addition is missing from the vendored `properties` list and
+the vendored `additionalProperties: false` rejects it just as the by-omission rule
+would; by-omission is merely the larger surface (103 objects against 5). See the
+caller doc's rollout section.
 
 A stripped key is not a cosmetic problem. The recurring instance is a `tools:`
 block written at the document root instead of under `reviews:` — the schema root