diff --git a/.github/coderabbit-config/README.md b/.github/coderabbit-config/README.md index b87ef115..96cc07d4 100644 --- a/.github/coderabbit-config/README.md +++ b/.github/coderabbit-config/README.md @@ -40,11 +40,12 @@ cleaned up and wants to stay clean. only the property names it lists — which upstream expresses two different ways, and both are checked: -* **explicitly**, with `additionalProperties: false`. Five objects: the document - root, `knowledge_base.mcp`, `knowledge_base.linked_repositories[]`, and the - `htmlhint` / `stylelint` tool configs. jsonschema reports these. +* **explicitly**, with `additionalProperties: false`. Seven objects: the document + root, `reviews.auto_approvals` and its `overrides[]` items, `knowledge_base.mcp`, + `knowledge_base.linked_repositories[]`, and the `htmlhint` / `stylelint` tool + configs. jsonschema reports these. * **by omission** — a `properties` block and no statement at all about anything - else. **103** objects, including `reviews`, `chat`, `knowledge_base`, + else. **105** objects, including `reviews`, `chat`, `knowledge_base`, `code_generation` and every individual tool config. jsonschema has no keyword to fire on here, so a schema-walk in `_walk_unknown_keys` reports them. This is what catches a typo *inside* an object — `reviews.profil`, @@ -81,7 +82,7 @@ pin moves. Warn-only mode absorbs that as a warning; a caller running 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 +would; by-omission is merely the larger surface (105 objects against 7). See the caller doc's rollout section. A stripped key is not a cosmetic problem. The recurring instance is a `tools:` diff --git a/.github/coderabbit-config/check_coderabbit_config.py b/.github/coderabbit-config/check_coderabbit_config.py index 042b33a1..9ea7851f 100644 --- a/.github/coderabbit-config/check_coderabbit_config.py +++ b/.github/coderabbit-config/check_coderabbit_config.py @@ -28,9 +28,9 @@ class of problem rather than what a schema library calls "invalid": failure for a repo that has cleaned up and wants to stay clean. An unknown key is reported wherever the schema object it sits in accepts only the -names it lists — whether it says so with `additionalProperties: false` (five +names it lists — whether it says so with `additionalProperties: false` (seven objects, which jsonschema reports) or simply by naming `properties` and nothing -else (103 more, which `_walk_unknown_keys` reports). A node that opts OUT of that +else (105 more, which `_walk_unknown_keys` reports). A node that opts OUT of that — any of `_OPENER_KEYWORDS` — is left alone in both halves. A repo with no `.coderabbit.yaml` passes, and says so in the log — not every @@ -110,10 +110,11 @@ class of problem rather than what a schema library calls "invalid": STRIPPED_KEYWORDS = frozenset({"additionalProperties", "unevaluatedProperties"}) # Keywords that leave a schema object OPEN to keys its own `properties` block does -# not name. Upstream's schema closes only five objects with an explicit -# `additionalProperties: false` (the document root, the `htmlhint` and `stylelint` -# tool objects, `knowledge_base.mcp`, and `knowledge_base.linked_repositories[]`), -# while 103 more declare `properties` and say nothing at all about the rest. A +# not name. Upstream's schema closes only seven objects with an explicit +# `additionalProperties: false` (the document root, `reviews.auto_approvals` and +# its `overrides[]` items, the `htmlhint` and `stylelint` tool objects, +# `knowledge_base.mcp`, and `knowledge_base.linked_repositories[]`), while 105 +# more declare `properties` and say nothing at all about the rest. A # typo inside one of those — `reviews.profil`, `golangci-lint.enabld` — is stripped # by CodeRabbit exactly like a root-level one, but jsonschema has nothing to # complain about, so `iter_errors` alone reports it nowhere. `_walk_unknown_keys` @@ -307,6 +308,35 @@ def _suggest_home(key, schema_index, offending_path): return f" Did you mean {shown}?" +# Keys under which a schema node is evaluated as a BRANCH: a failure there +# surfaces as the combinator's own (rejecting) error, never as a bare +# `additionalProperties` one, so opening a closed object inside one would change +# which documents the branch accepts. `_without_stripping` leaves them as found. +_BRANCH_KEYWORDS = frozenset({"anyOf", "oneOf", "allOf", "not", "if", "then", "else"}) + + +def _without_stripping(node): + """A copy of the schema whose only errors are the file-rejecting ones. + + Drops every `additionalProperties: false` / `unevaluatedProperties: false` + outside a combinator — what CodeRabbit does to a document is strip the keys + those name and then validate the rest, and removing the keyword is exactly + that model: it raises no error of its own and gates nothing else, so every + other keyword yields the errors it yielded before. Schema-valued forms are + kept; they validate the extra keys rather than strip them. + """ + if isinstance(node, list): + return [_without_stripping(item) for item in node] + if not isinstance(node, dict): + return node + out = {} + for key, value in node.items(): + if key in STRIPPED_KEYWORDS and value is False: + continue + out[key] = value if key in _BRANCH_KEYWORDS else _without_stripping(value) + return out + + def _apply_cap(tagged): """Trim `[(rank, finding)]` to MAX_FINDINGS. Returns (findings, was_capped). @@ -625,8 +655,8 @@ def _walk_unknown_keys(data, schema, node, schema_index, severity, limit): """Unknown keys inside objects the schema closes BY OMISSION. jsonschema reports an unknown key only where the schema says `additionalProperties` - / `unevaluatedProperties` — five objects in upstream's schema. This walks the - parsed document alongside the schema and reports the other 103: a node that + / `unevaluatedProperties` — seven objects in upstream's schema. This walks the + parsed document alongside the schema and reports the other 105: a node that declares a dict-valued `properties` and NONE of `_OPENER_KEYWORDS` accepts exactly the names it lists, so any other key in the document is stripped. @@ -831,6 +861,22 @@ def _order(error): truncated = len(raw_errors) > MAX_FINDINGS del raw_errors[MAX_FINDINGS:] + # The `islice` is unranked FIFO in schema order, so it can delete a + # file-rejecting error before `_apply_cap` ever ranks it: an unbounded array + # of closed objects (`reviews.auto_approvals.overrides[]` declares no + # `maxItems`) raises one stripped error per element, all emitted ahead of a + # `maxLength` violation in a later property — 100 warnings, exit 0, on a file + # CodeRabbit rejects whole. So when the cap was hit, the rejecting errors are + # re-collected from a second pass that cannot raise anything else, bounded + # the same way, and replace whatever subset of them the first pass kept. + if truncated: + rejecting = Draft202012Validator(_without_stripping(schema)) + raw_errors = [e for e in raw_errors if e.validator in STRIPPED_KEYWORDS] + [ + e + for e in islice(rejecting.iter_errors(data), MAX_FINDINGS + 1) + if e.validator not in STRIPPED_KEYWORDS + ] + # `islice` above bounds the ERROR count, not the finding count: one # `additionalProperties: false` error reports a whole mapping and `_extra_keys` # splits it into one finding per key, so a single error can fan out past the @@ -886,7 +932,7 @@ def _order(error): ) ) - # The other 103 objects: closed by OMISSION, so jsonschema said nothing about + # The other 105 objects: closed by OMISSION, so jsonschema said nothing about # them. Appended after the jsonschema findings rather than merged into their # sort, which keeps both halves in document order within themselves and the # whole list stable run to run. One past `remaining` so an overflow is diff --git a/.github/coderabbit-config/schema.v2.json b/.github/coderabbit-config/schema.v2.json index ed3bf269..6a1c997a 100644 --- a/.github/coderabbit-config/schema.v2.json +++ b/.github/coderabbit-config/schema.v2.json @@ -242,14 +242,88 @@ "type": "string", "enum": ["quiet", "chill", "assertive"] }, + "auto_approvals": { + "default": {}, + "description": "Automatic approvals | Control automatic approval, optional risk limits, and overrides for teams or paths. Limits and disabled overrides apply to approve and resolve commands too. Missing, failed, disabled or stale required assessments withhold approval.", + "type": "object", + "properties": { + "enabled": { + "default": false, + "description": "Automatic approvals | Request changes for blocking findings. Automatically approve when required threads are resolved, the latest commit is reviewed, pre-merge checks pass, and configured risk limits are satisfied. In GitLab, required discussions must be resolved.", + "type": "boolean" + }, + "max_merge_risk": { + "description": "Maximum merge risk for approval | Highest inclusive merge risk that permits approval. Omitted limits can be inherited; merge risk is unrestricted only when no configuration source sets a limit. A configured limit requires an assessment covering the current commit.", + "type": "string", + "enum": ["minimal", "low", "moderate", "high", "critical"] + }, + "allow_author_approval": { + "default": true, + "description": "Allow author approval commands | Allow pull request authors to use top-level @coderabbitai approve and @coderabbitai resolve commands on their own pull requests. When disabled, another reviewer must run them. The pull request's branch configuration cannot re-enable this permission. Commands still honor approval risk limits. This does not require human review or change automatic approvals after normal review requirements pass.", + "type": "boolean" + }, + "max_security_risk": { + "description": "Maximum security risk for approval | Highest inclusive security architecture risk that permits approval. Security risk assessments require the Advanced plan or above. Omitted limits can be inherited; security risk is unrestricted only when no configuration source sets a limit. This is separate from Security finding severity. A configured limit requires an assessment covering the current commit.", + "type": "string", + "enum": ["minimal", "low", "moderate", "high", "critical"] + }, + "overrides": { + "description": "Overrides | Disable approvals or override risk limits by author team, changed paths, or both. When teams and paths are set, both must match. A matching file applies the override to the whole pull request. Any matching disabled override withholds approval. Otherwise, the strictest explicitly configured value wins for each limit; omitted values inherit the global limit. At most 15 distinct teams may be checked. Unavailable required team membership or changed-file evidence withholds approval.", + "type": "array", + "items": { + "type": "object", + "properties": { + "enabled": { + "description": "Automatic approvals | Set to false to withhold approval for matching pull requests, regardless of risk. Omit to use the global setting. Any matching disabled override wins. Review comments and requests for changes continue normally; overrides cannot turn on a globally disabled workflow.", + "type": "boolean", + "const": false + }, + "teams": { + "description": "Teams | Apply this override when the pull request author belongs to any listed GitHub team. Use organization/team-slug.", + "minItems": 1, + "maxItems": 15, + "type": "array", + "items": { + "type": "string", + "pattern": "^[A-Za-z0-9][A-Za-z0-9-]*\\/[A-Za-z0-9][A-Za-z0-9-]*$" + } + }, + "paths": { + "description": "Paths | Apply this override when any changed file matches these repository-relative globs. Uses the same glob syntax as path_filters, including ! exclusions. Renames match both old and new paths. When teams and paths are set, both must match. Omit to match all paths.", + "minItems": 1, + "type": "array", + "items": { + "type": "string", + "minLength": 1 + } + }, + "max_merge_risk": { + "description": "Maximum merge risk for approval | Override the global merge-risk limit for matching pull requests. Omit to inherit the global limit.", + "type": "string", + "enum": ["minimal", "low", "moderate", "high", "critical"] + }, + "max_security_risk": { + "description": "Maximum security risk for approval | Override the global security-risk limit for matching pull requests. Omit to inherit the global limit. Security risk assessments require the Advanced plan or above.", + "type": "string", + "enum": ["minimal", "low", "moderate", "high", "critical"] + } + }, + "additionalProperties": false + } + } + }, + "additionalProperties": false + }, "request_changes_workflow": { - "default": false, - "description": "Automatically approve when CodeRabbit’s comments are resolved, the latest commit has been reviewed, and no pre-merge checks are failing. Note: In GitLab, all discussions must be resolved.", + "description": "Legacy alias for reviews.auto_approvals.enabled.", + "deprecated": true, + "x-undocumented": true, "type": "boolean" }, "allow_author_approval": { - "default": true, - "description": "Allow Author Approval | Controls whether pull request authors can use `@coderabbitai approve` and `@coderabbitai resolve` on their own pull requests. When disabled, anyone other than the author can still use them, provided CodeRabbit can identify both the commenter and the author. The pull request’s branch configuration cannot re-enable this setting.", + "description": "Legacy alias for reviews.auto_approvals.allow_author_approval.", + "deprecated": true, + "x-undocumented": true, "type": "boolean" }, "high_level_summary": { @@ -287,6 +361,11 @@ "description": "Post review status messages (e.g., when a review is skipped) in the walkthrough summary comment.", "type": "boolean" }, + "review_info": { + "default": true, + "description": "Post review information such as the configuration used, reviewed commits, and skipped files.", + "type": "boolean" + }, "review_details": { "default": false, "description": "Post review details (ignored files, extra context used, suppressed comments, etc.).", @@ -619,13 +698,13 @@ } }, "autofix": { - "description": "Autofix | Configure autofix behavior.", + "description": "Fix comments | Configure the Fix comments finishing touch.", "default": {}, "type": "object", "properties": { "enabled": { "default": true, - "description": "Autofix | Enable the autofix finishing touch (trigger via the 🪄 Autofix checkboxes under review comments or the `@coderabbitai autofix` command). When disabled, the autofix checkboxes are hidden, the command is removed from help, and CodeRabbit declines `@coderabbitai autofix` requests.", + "description": "Fix comments | Enable the Fix comments finishing touch (trigger via the 🪄 Fix comments checkbox under review comments or the `@coderabbitai fix comments` command). When disabled, the Fix comments checkboxes are hidden, the command is removed from help, and CodeRabbit declines one-off repair requests using `@coderabbitai fix comments` or the compatibility aliases `fix-comments`, `fix`, `autofix`, `auto-fix`, and `auto fix`, with or without `stacked pr`.", "type": "boolean" } } @@ -886,7 +965,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Ruff | Ruff is a Python linter and code formatter. | Enable Ruff integration. | v0.16.8", + "description": "Enable Ruff | Ruff is a Python linter and code formatter. | Enable Ruff integration. | v0.16.10", "type": "boolean" }, "config_file": { @@ -981,7 +1060,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Biome | Biome is a fast formatter, linter, and analyzer for web projects. | Enable Biome integration. | v2.5.14", + "description": "Enable Biome | Biome is a fast formatter, linter, and analyzer for web projects. | Enable Biome integration. | v2.5.15", "type": "boolean" } } @@ -1021,7 +1100,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable PHPStan | PHPStan requires [config file](https://phpstan.org/config-reference#config-file) in your repository root. Please ensure that this file contains the `paths:` parameter. | v2.2.14", + "description": "Enable PHPStan | PHPStan requires [config file](https://phpstan.org/config-reference#config-file) in your repository root. Please ensure that this file contains the `paths:` parameter. | v2.3.0", "type": "boolean" }, "level": { @@ -1052,7 +1131,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable PHPMD | PHPMD is a tool to find potential problems in PHP code. | v2.15.0", + "description": "Enable PHPMD | PHPMD is a tool to find potential problems in PHP code. | v3.0.0", "type": "boolean" } } @@ -1076,7 +1155,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable golangci-lint | golangci-lint is a fast linters runner for Go. | Enable golangci-lint integration. | v2.13.2", + "description": "Enable golangci-lint | golangci-lint is a fast linters runner for Go. | Enable golangci-lint integration. | v2.14.0", "type": "boolean" }, "config_file": { @@ -1104,7 +1183,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Betterleaks | Betterleaks is a secret scanner (an improved version of Gitleaks). | Enable Betterleaks integration. | v1.8.1", + "description": "Enable Betterleaks | Betterleaks is a secret scanner (an improved version of Gitleaks). | Enable Betterleaks integration. | v1.9.0", "type": "boolean" } } @@ -1128,7 +1207,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Checkov | Checkov is a static code analysis tool for infrastructure-as-code files. | v3.3.19", + "description": "Enable Checkov | Checkov is a static code analysis tool for infrastructure-as-code files. | v3.3.25", "type": "boolean" } } @@ -1261,7 +1340,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Regal | Regal is a linter and language server for Rego. | v0.42.0", + "description": "Enable Regal | Regal is a linter and language server for Rego. | v0.43.0", "type": "boolean" } } @@ -1297,7 +1376,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable PMD | PMD is an extensible multilanguage static code analyzer. It’s mainly concerned with Java. | v7.27.0", + "description": "Enable PMD | PMD is an extensible multilanguage static code analyzer. It’s mainly concerned with Java. | v7.28.0", "type": "boolean" }, "config_file": { @@ -1337,7 +1416,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Vale | Vale checks prose against repository-defined editorial style rules. It runs only when a supported root Vale configuration is present. | v3.22.0", + "description": "Enable Vale | Vale checks prose against repository-defined editorial style rules. It runs only when a supported root Vale configuration is present. | v3.24.0", "type": "boolean" } } @@ -1373,7 +1452,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Semgrep | Semgrep is a static analysis tool designed to scan code for security vulnerabilities and code quality issues. | Enable Semgrep integration. | v1.177.0", + "description": "Enable Semgrep | Semgrep is a static analysis tool designed to scan code for security vulnerabilities and code quality issues. | Enable Semgrep integration. | v1.179.0", "type": "boolean" }, "config_file": { @@ -1389,7 +1468,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable CircleCI | CircleCI tool is a static checker for CircleCI config files. | v1.0.50961", + "description": "Enable CircleCI | CircleCI tool is a static checker for CircleCI config files. | v1.2.0", "type": "boolean" } } @@ -1413,7 +1492,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable SQLFluff | SQLFluff is an open source, dialect-flexible and configurable SQL linter. | v4.3.0", + "description": "Enable SQLFluff | SQLFluff is an open source, dialect-flexible and configurable SQL linter. | v4.4.0", "type": "boolean" }, "config_file": { @@ -1429,7 +1508,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Squawk for Postgres migration linting | Detects unsafe schema changes that can cause downtime or blocking locks | v2.65.0", + "description": "Enable Squawk for Postgres migration linting | Detects unsafe schema changes that can cause downtime or blocking locks | v2.67.0", "type": "boolean" } } @@ -1441,7 +1520,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Trivy for security scanning of IaC files (Terraform, Kubernetes, Docker, etc.) | v0.74.0", + "description": "Enable Trivy for security scanning of IaC files (Terraform, Kubernetes, Docker, etc.) | v0.75.0", "type": "boolean" } } @@ -1465,7 +1544,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Pylint | Pylint is a Python static code analysis tool. | v4.0.8", + "description": "Enable Pylint | Pylint is a Python static code analysis tool. | v4.1.2", "type": "boolean" } } @@ -1477,7 +1556,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Oxlint | Oxlint is a JavaScript/TypeScript linter for OXC written in Rust. | v1.85.0", + "description": "Enable Oxlint | Oxlint is a JavaScript/TypeScript linter for OXC written in Rust. | v1.87.0", "type": "boolean" } } @@ -1489,7 +1568,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable Shopify Theme Check | A linter for Shopify themes that helps you follow Shopify theme & Liquid best practices | cli 4.8.0 | theme 3.58.2", + "description": "Enable Shopify Theme Check | A linter for Shopify themes that helps you follow Shopify theme & Liquid best practices | cli 4.8.5 | theme 3.58.2", "type": "boolean" } } @@ -1507,13 +1586,13 @@ } }, "brakeman": { - "description": "Brakeman is a static analysis security vulnerability scanner for Ruby on Rails applications. | v8.0.6", + "description": "Brakeman is a static analysis security vulnerability scanner for Ruby on Rails applications. | v8.1.0", "default": {}, "type": "object", "properties": { "enabled": { "default": true, - "description": "Enable Brakeman | Brakeman is a static analysis security vulnerability scanner for Ruby on Rails applications. | v8.0.6", + "description": "Enable Brakeman | Brakeman is a static analysis security vulnerability scanner for Ruby on Rails applications. | v8.1.0", "type": "boolean" } } @@ -1587,7 +1666,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable oasdiff | oasdiff detects breaking changes between OpenAPI specifications. | v1.32.1", + "description": "Enable oasdiff | oasdiff detects breaking changes between OpenAPI specifications. | v1.33.0", "type": "boolean" } } @@ -1599,7 +1678,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable React Doctor | React Doctor scans React codebases for security, performance, correctness, and accessibility issues. | v0.9.14", + "description": "Enable React Doctor | React Doctor scans React codebases for security, performance, correctness, and accessibility issues. | v0.9.17", "type": "boolean" } } @@ -1659,7 +1738,7 @@ "properties": { "enabled": { "default": true, - "description": "Enable SkillSpector | SkillSpector is a security scanner for AI agent skills. It detects vulnerabilities, malicious patterns, and security risks in SKILL.md manifests and MCP configurations. | v2.11.2", + "description": "Enable SkillSpector | SkillSpector is a security scanner for AI agent skills. It detects vulnerabilities, malicious patterns, and security risks in SKILL.md manifests and MCP configurations. | v2.12.0", "type": "boolean" } } @@ -1771,7 +1850,7 @@ }, "filePatterns": { "default": [], - "description": "File Patterns | Patterns for your coding guideline documents. CodeRabbit scans these files to learn your standards and apply them during reviews. Multiple files supported; file names are case-sensitive. An entry may also reference a same-organization repository the CodeRabbit installation can read, as repo:path or owner/repo:path (e.g. engineering-standards:frontend/react.md, globs supported); such files are read at that repository's default branch. Defaults include: (**/.cursorrules, .github/copilot-instructions.md, **/CLAUDE.md, **/GEMINI.md, **/.cursor/rules/*, **/.windsurfrules, **/.clinerules/*, **/.rules/*, **/AGENT.md, **/AGENTS.md, **/REVIEW.md).", + "description": "File Patterns | Patterns for your coding guideline documents. CodeRabbit scans these files to learn your standards and apply them during reviews. Multiple files supported; file names are case-sensitive. An entry may also reference another repository the CodeRabbit installation can read, as repo:path or owner/repo:path (e.g. engineering-standards:frontend/react.md, globs supported); such files are read at that repository's default branch. A bare repo name uses the reviewed repository's owner. On GitHub.com, owner/repo:path may reference another organization when the CodeRabbit App is installed there with access to the source repository. Every cross-organization source requires author and review-actor read access under the existing identity rules, even when named by trusted target configuration. Other providers require the same organization. Defaults include: (**/.cursorrules, .github/copilot-instructions.md, **/CLAUDE.md, **/.claude/rules/**/*.md, **/GEMINI.md, **/.cursor/rules/*, **/.windsurfrules, **/.clinerules/*, **/.rules/*, **/AGENT.md, **/AGENTS.md, **/REVIEW.md).", "type": "array", "items": { "anyOf": [ diff --git a/.github/coderabbit-config/tests/test_check_coderabbit_config.py b/.github/coderabbit-config/tests/test_check_coderabbit_config.py index 1532225f..d1511271 100644 --- a/.github/coderabbit-config/tests/test_check_coderabbit_config.py +++ b/.github/coderabbit-config/tests/test_check_coderabbit_config.py @@ -11,7 +11,7 @@ a `golangci-lint: enabled: false` into the schema default of `true`; * a `reviews.tools.github-checks.timeout_ms` that upstream has since removed — live in six org repos at the time of writing, and the whole reason the - unknown-key check cannot stop at the five objects the schema closes + unknown-key check cannot stop at the seven objects the schema closes explicitly (see `ClosedByOmissionTest`). Every test validates against the REAL vendored schema rather than a toy one, so @@ -429,7 +429,7 @@ def test_unreadable_schema_exits_two(self): class ClosedByOmissionTest(unittest.TestCase): """Unknown keys inside objects the schema closes by saying nothing. - Upstream declares `additionalProperties: false` on five objects and leaves 103 + Upstream declares `additionalProperties: false` on seven objects and leaves 105 more with a `properties` block and no statement about anything else. CodeRabbit strips an unrecognized key in either kind, so both must be reported — and the two halves must stay disjoint, since one fires only where the keyword is @@ -723,6 +723,44 @@ def test_a_file_rejecting_error_is_never_crowded_out_by_unknown_keys(self): self.assertEqual(len(rejecting), 1, msg="the maxLength error was capped away") self.assertIn("error", severities(findings)) + def test_a_rejecting_error_is_not_crowded_out_before_ranking_either(self): + # The stage BEFORE `_apply_cap`: `islice(iter_errors(...))` is FIFO in + # schema order, and `auto_approvals.overrides[]` is an array of closed + # objects with no `maxItems`, so 150 elements each carrying one unknown + # key emit 150 stripped errors ahead of the `maxLength` violation in the + # later `path_instructions`. Without the rejecting-only second pass the + # islice keeps none of it and the file exits 0. + overrides = "".join(" - enabled: false\n bogus: 1\n" for _ in range(150)) + text = ( + "reviews:\n auto_approvals:\n overrides:\n" + overrides + + " path_instructions:\n - path: x\n instructions: " + + ("y" * 20_001) + "\n" + ) + findings, notes = checker.validate(text, SCHEMA) + self.assertEqual(len(findings), checker.MAX_FINDINGS) + self.assertTrue(any("were dropped" in n for n in notes), msg=f"{notes}") + rejecting = [f for f in findings if "characters" in f[3]] + self.assertEqual(len(rejecting), 1, msg="the maxLength error was sliced away") + self.assertIn("path_instructions", rejecting[0][1]) + self.assertIn("error", severities(findings)) + + def test_the_rejecting_only_schema_opens_nothing_inside_a_branch(self): + # Opening a closed object inside `anyOf` would change which documents + # that branch accepts, i.e. hide a rejecting error rather than isolate it. + schema = { + "type": "object", + "additionalProperties": False, + "properties": { + "a": {"type": "object", "additionalProperties": False, "properties": {}}, + "b": {"anyOf": [{"type": "object", "additionalProperties": False}]}, + }, + } + opened = checker._without_stripping(schema) + self.assertNotIn("additionalProperties", opened) + self.assertNotIn("additionalProperties", opened["properties"]["a"]) + self.assertIs(opened["properties"]["b"]["anyOf"][0]["additionalProperties"], False) + self.assertIs(schema["additionalProperties"], False, msg="the input was mutated") + def test_the_unknown_key_fan_out_stops_working_at_the_cap(self): # The cap bounds the OUTPUT; this bounds the WORK. `k00000: 1` is ten # bytes, so ~50k unknown root keys fit inside MAX_CONFIG_BYTES, and @@ -1058,10 +1096,15 @@ def test_the_two_halves_partition_the_objects_that_declare_properties(self): # Not an arbitrary snapshot: this is the size of the gap being closed. # `omitted` grows by one per object the upstream schema adds without an # opener -- 103 at #179, 104 at #215, 105 once #250 vendored the `vale` - # tool. `explicit` holding at 5 is the load-bearing half: it says the - # refresh introduced no NEW opener keyword, so the walk still means what - # the docstring above claims. Bump these deliberately, never to go green. - self.assertEqual(len(explicit), 5) + # tool. `explicit` is the load-bearing half: every member must be CLOSED + # with `additionalProperties: false` (asserted below), so growth there + # means more explicitly closed objects, never a new way to open one. It went + # 5 -> 7 when upstream added `reviews.auto_approvals` and its + # `overrides[]` items, both closed that way. Bump these deliberately, + # never to go green. + self.assertEqual(len(explicit), 7) + for one in explicit: + self.assertIs(one.get("additionalProperties"), False, msg=f"{list(one)}") self.assertEqual(len(omitted), 105) def test_every_explicitly_open_object_uses_additional_properties(self): @@ -1094,11 +1137,15 @@ def test_a_rejecting_error_cannot_be_crowded_out_before_ranking(self): order, so a file-rejecting error arriving past the cap is deleted before ranking ever sees it — the same exit-0-on-a-whole-file-rejection the rank exists to prevent. It needs MAX_FINDINGS *stripped* errors ahead of it - (one rejecting error among them and the exit code is 1 regardless), and - against THIS schema that cannot be assembled. Four facts make it so, each - asserted below rather than summarized in a number — `maxItems` is NOT one - of them as a multiplicity bound, because jsonschema keeps validating - elements past it (a 150-element array really does raise 150 of them). + (one rejecting error among them and the exit code is 1 regardless). + Until `reviews.auto_approvals.overrides` that could not be assembled + against THIS schema; that array is the one exception, and the checker now + re-collects rejecting errors in a second pass whenever the cap is hit (see + `CapAndBudgetTest`). The facts below are still asserted rather than + summarized in a number, so a refresh that adds ANOTHER unbounded source + names it here — `maxItems` is NOT one of them as a multiplicity bound, + because jsonschema keeps validating elements past it (a 150-element + array really does raise 150 of them). """ stripped_false, unbounded_multipliers, arrays_over_closed = [], [], [] @@ -1154,7 +1201,16 @@ def walk(node, path, multiplied): # exceeding it raises a REJECTING `maxItems` error, and `iter_errors` # walks a node's keywords in schema key order, so that error is emitted # ahead of the element errors it would otherwise be buried under. - for path, array in [x for x in arrays_over_closed if isinstance(x, tuple)]: + # Except the arrays listed here, which declare no `maxItems` at all and + # are what the checker's rejecting-only second pass exists for. + uncapped = {"$.properties.reviews.properties.auto_approvals.properties.overrides"} + arrays = [x for x in arrays_over_closed if isinstance(x, tuple)] + self.assertEqual( + {path for path, array in arrays if "maxItems" not in array}, uncapped + ) + for path, array in arrays: + if path in uncapped: + continue keys = list(array.keys()) self.assertIn("maxItems", keys, msg=path) self.assertLess(keys.index("maxItems"), keys.index("items"), msg=path)