Fix-forward PR 2306 (doc-gate config errors): 2 config-error paths still exit 1 - #2312
Merged
Conversation
An unparseable or missing doc-gate config previously raised an unhandled traceback that exited 1 -- identical to a real documentation-drift violation, so a typo in docs/doc-gate.toml was indistinguishable from a missing changelog. Catch tomllib.TOMLDecodeError and OSError in main(), print a clear error to stderr, and exit 2 instead.
…figs Pr #2306 left two config-error paths that still exited 1 instead of the config-error code: - Structurally invalid config (valid TOML, wrong shape, e.g. rules is a string) passed load_config and died later in the rule loop with an AttributeError. Add _validate_config to check the shape after parsing. - Non-UTF-8 config bytes raised UnicodeDecodeError inside tomllib.load (which decodes before parsing), uncaught by the TOMLDecodeError-only handler. Catch that alongside the other config errors. Also move EXIT_CONFIG_ERROR from 2 to 3: exit code 2 is argparse's own usage-error exit, so a typo'd flag was indistinguishable from a broken config. Now 1 (violation), 2 (argparse), and 3 (config error) are all mutually distinct. Supersedes exec/tsk-thh54d (pr #2306).
📝 WalkthroughWalkthroughThe document gate now uses named exit codes, validates the structure of parsed TOML configuration, reports configuration failures, and preserves distinct codes for violations and invalid CLI usage. Tests cover malformed, missing, unreadable, and structurally invalid configurations. ChangesDocument gate configuration errors
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TOMLConfiguration
participant ConfigValidator
participant stderr
CLI->>TOMLConfiguration: Load configuration
TOMLConfiguration-->>CLI: Parsed data or loading error
CLI->>ConfigValidator: Validate parsed structure
ConfigValidator-->>CLI: Valid data or validation error
CLI->>stderr: Report configuration failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
PR Summary by QodoDoc-gate: validate config shape and use distinct exit code for config errors
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Owner
Author
|
nemotron-super review VERDICT: Approved Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_doc_gate.py`:
- Around line 105-121: Extend configuration validation before evaluate_rules and
check_referenced_paths to validate each rule’s consumed fields, including
when_changed and on_modify, using the expected types and rejecting invalid
scalar values; also validate every referenced_paths_scan entry as a string. Keep
existing outer-container checks and raise clear ValueError messages identifying
the invalid rule index or scan entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25077cfe-7b2b-4580-9ca2-0219076a18d7
📒 Files selected for processing (2)
scripts/check_doc_gate.pytests/test_doc_gate.py
Comment on lines
+105
to
+121
| rules = config.get("rules", []) | ||
| if not isinstance(rules, list): | ||
| raise ValueError("'rules' must be a list of tables") | ||
| for i, rule in enumerate(rules): | ||
| if not isinstance(rule, dict): | ||
| raise ValueError(f"rules[{i}] must be a table") | ||
| gate = config.get("gate", {}) | ||
| if not isinstance(gate, dict): | ||
| raise ValueError("'gate' must be a table") | ||
| if "trailer" in gate and not isinstance(gate["trailer"], str): | ||
| raise ValueError("'gate.trailer' must be a string") | ||
| invariants = config.get("invariants", {}) | ||
| if not isinstance(invariants, dict): | ||
| raise ValueError("'invariants' must be a table") | ||
| scan = invariants.get("referenced_paths_scan", []) | ||
| if not isinstance(scan, list): | ||
| raise ValueError("'invariants.referenced_paths_scan' must be a list") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline scripts/check_doc_gate.py --lang python --items all --type function
sed -n '90,250p' scripts/check_doc_gate.py
rg -n -C 4 'def _match_any|def check_referenced_paths|when_changed|require_doc|on_modify|referenced_paths_scan' \
scripts/check_doc_gate.py tests/test_doc_gate.pyRepository: jaylfc/taOS
Length of output: 16464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file outline/digest =="
wc -l tests/test_doc_gate.py
rg -n 'when_changed|require_doc|on_modify|referenced_paths_scan|_validate_config|valueerror|config error|exception|raises' tests/test_doc_gate.py scripts/check_doc_gate.py
sed -n '1,120p' tests/test_doc_gate.py
sed -n '120,360p' tests/test_doc_gate.py
echo "== behavioral type probe =="
python3 - <<'PY'
from pathlib import Path
import tomllib
import sys
sys.path.insert(0, str(Path(__file__).parent))
from scripts.check_doc_gate import _glob_match, extract_path_tokens, evaluate_rules, check_referenced_paths, _match_any, _validate_config
cases = {
"rule when_changed scalar": {"rules": [{"name": "r", "when_changed": 7, "require_doc": []}]},
"rule require_doc scalar": {"rules": [{"name": "r", "when_changed": [], "require_doc": 7}]},
"rule on_modify string": {"rules": [{"name": "r", "when_changed": [], "require_doc": [], "on_modify": "yes"}]},
"rule hint number": {"rules": [{"name": "r", "when_changed": [], "require_doc": [], "hint": 7}]},
"scan entry number": {"invariants": {"referenced_paths_scan": [7]}},
}
for case, config in cases.items():
print(f"\n{case}")
try:
_validate_config(config)
print("_validate_config: passed")
except Exception as e:
print(f"_validate_config: {type(e).__name__}: {e}")
continue
if case.startswith("rule"):
try:
out = evaluate_rules([("A", "desktop/src/apps/Foo.tsx")], ["msg"], config)
print(f"evaluate_rules: passed returned {out!r}")
except Exception as e:
print(f"evaluate_rules: {type(e).__name__}: {e}")
else:
try:
out = check_referenced_paths(Path(".checkdoc", encoding="utf-8"), ["README.md"], config)
print(f"check_referenced_paths: passed returned {out!r}")
except Exception as e:
print(f"check_referenced_paths: {type(e).__name__}: {e}")
for val in (7, "a/b", "a/b"):
for f in (_glob_match, extract_path_tokens, _match_any):
print(f"{f.__name__}({val!r}) -> ", end="")
try:
if f is _match_any:
print(f"{'True' if f('a/b', [val]) else 'False'}")
else:
print(f"{f(val)!r}")
except Exception as e:
print(f"{type(e).__name__}: {e}")
PYRepository: jaylfc/taOS
Length of output: 19025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
def _glob_match(path, pattern):
"""Reduced version of scripts/check_doc_gate.py._glob_match for type behavior."""
regex_parts = []
i = 0
length = len(pattern)
while i < length:
char = pattern[i]
if char == "*":
if i + 1 < length and pattern[i + 1] == "*":
if regex_parts and regex_parts[-1] == "/" and i + 2 == length:
regex_parts[-1] = "(?:/.*)?"
else:
regex_parts.append(".*")
i += 2
else:
regex_parts.append("[^/]*")
i += 1
elif char == "?":
regex_parts.append("[^/]")
i += 1
else:
regex_parts.append(re.escape(char))
i += 1
return re.fullmatch("".join(regex_parts), path) is not None
def _is_test_path(path):
"""Reduced version of scripts/check_doc_gate.py._is_test_path for type behavior."""
base = path.rsplit("/", 1)[-1]
if "/__tests__/" in path:
return True
if base.startswith("test_") and base.endswith(".py"):
return True
return base.endswith((".test.tsx", ".test.ts", ".test.jsx", ".test.js", ".spec.tsx", ".spec.rs", ".spec.ts", ".spec.jsx"))
def extract_path_tokens(text):
"""Reduced version of scripts/check_doc_gate.py.extract_path_tokens for type behavior."""
pos = 0
while pos < len(text):
if text[pos] == "`":
start = pos + 1
pos_end = text.find("`", pos + 1)
if pos_end == -1:
break
token = text[start:pos_end]
if not any(c in token for c in "*?"):
yield token
pos = pos_end + 1
else:
pos += 1
values = [7, None, {}, [], True, "a/b"]
for f in (_glob_match, _match_any := lambda path, patterns: any(_glob_match(path, pat) for pat in patterns), _is_test_path, extract_path_tokens):
for v in values:
try:
args = ("a/b", [v]) if f is _match_any else (v,) if f in (_is_test_path, extract_path_tokens) else ()
result = f(*args)
print(f"{f.__name__}({v!r}) -> returned {result!r}")
except Exception as e:
print(f"{f.__name__}({v!r}) -> {type(e).__name__}: {e}")
PYRepository: jaylfc/taOS
Length of output: 2072
Validate nested rule and scan patterns before dispatch.
scripts/check_doc_gate.py only validates outer containers, so valid TOML with scalar or missing nested values such as rules[0].when_changed = 7, rules[0].on_modify = "yes", or referenced_paths_scan = [7] can pass configuration validation and crash during rule matching or the invariant scan. Reject invalid consumed field types before evaluate_rules and check_referenced_paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check_doc_gate.py` around lines 105 - 121, Extend configuration
validation before evaluate_rules and check_referenced_paths to validate each
rule’s consumed fields, including when_changed and on_modify, using the expected
types and rejecting invalid scalar values; also validate every
referenced_paths_scan entry as a string. Keep existing outer-container checks
and raise clear ValueError messages identifying the invalid rule index or scan
entry.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 57.2K · Output: 9.3K · Cached: 132K |
Code Review by Qodo
1. Scan entry type unchecked
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+105
to
+108
| rules = config.get("rules", []) | ||
| if not isinstance(rules, list): | ||
| raise ValueError("'rules' must be a list of tables") | ||
| for i, rule in enumerate(rules): |
There was a problem hiding this comment.
1. Incomplete config type validation 🐞 Bug ≡ Correctness
_validate_config() only checks that 'rules' is a list of tables, but it does not validate per-rule field types (e.g., when_changed/require_doc must be list[str], on_modify must be bool). Valid TOML with wrong field types can still raise unhandled exceptions (e.g., TypeError in _match_any) or silently mis-evaluate rules, causing the tool to exit 1 instead of EXIT_CONFIG_ERROR.
Agent Prompt
### Issue description
`_validate_config()` currently validates only the container types (e.g. `rules` is a list and each `rule` is a dict), but downstream code assumes specific types for fields inside each rule. Malformed-but-parseable TOML (e.g. `when_changed = 123` or `on_modify = "false"`) can still crash later or behave incorrectly, which defeats the intent of surfacing config problems as `EXIT_CONFIG_ERROR`.
### Issue Context
- `evaluate_rules()` treats `when_changed`/`require_doc` as iterables of glob strings and `on_modify` as a boolean.
- `_match_any()` iterates `patterns`; if a non-iterable sneaks through (e.g. int), it will raise a `TypeError` at runtime.
- Strings are iterable, so wrong types can also silently mis-evaluate rules rather than crashing.
### Fix Focus Areas
- scripts/check_doc_gate.py[96-122]
- scripts/check_doc_gate.py[175-176]
- scripts/check_doc_gate.py[222-248]
### Implementation notes
- Extend `_validate_config()` to validate, for each rule dict:
- `name` is `str` (optional but if present must be `str`)
- `when_changed` is `list[str]` (required; or at least if present, must be `list[str]`)
- `require_doc` is `list[str]` (required; or at least if present, must be `list[str]`)
- `hint` is `str` (if present)
- `on_modify` is `bool` (if present)
- Also validate `invariants.referenced_paths_scan` elements are `str` (not just that it is a list).
- Keep raising `ValueError` so the existing `except (..., ValueError)` path continues to return `EXIT_CONFIG_ERROR`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+119
to
+121
| scan = invariants.get("referenced_paths_scan", []) | ||
| if not isinstance(scan, list): | ||
| raise ValueError("'invariants.referenced_paths_scan' must be a list") |
There was a problem hiding this comment.
1. Scan entry type unchecked 🐞 Bug ☼ Reliability
_validate_config() only verifies that invariants.referenced_paths_scan is a list, so a TOML list containing non-strings will pass validation but later crash check_referenced_paths() at `repo_root / rel` with a TypeError. This produces an unhandled traceback and exit code 1 instead of EXIT_CONFIG_ERROR.
Agent Prompt
### Issue description
`_validate_config()` validates `invariants.referenced_paths_scan` is a list, but does not validate the element types. If the list contains a non-string TOML value (e.g. an integer), `check_referenced_paths()` later crashes at `repo_root / rel` with `TypeError`, bypassing the new config-error handling and exiting as a generic failure.
### Issue Context
This PR’s intent is to ensure config errors return `EXIT_CONFIG_ERROR` (3) rather than crashing and exiting 1 (violation). A malformed-but-parseable `referenced_paths_scan` list still violates that intent.
### Fix Focus Areas
- scripts/check_doc_gate.py[116-137]
- tests/test_doc_gate.py[299-369]
### Suggested fix
1. In `_validate_config()`, after confirming `scan` is a list, validate `all(isinstance(x, str) for x in scan)` and raise `ValueError` with a clear message (e.g. `"'invariants.referenced_paths_scan' entries must be strings"`) on the first invalid entry.
2. Add a regression test under `TestConfigErrorExitCode` with TOML like:
```toml
[invariants]
referenced_paths_scan = [123]
```
and assert `dg.main([...]) == dg.EXIT_CONFIG_ERROR` and stderr contains `"config error"`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 654a82b |
Owner
Author
Review: the fix is correct. The one red check is not yours.CI has now run properly — this PR had zero workflow runs until 00:05Z because it was opened during the GitHub Actions outage and its Result: 17 success, 1 failure. The single failure is
The substance is right, verified rather than skimmed:
I have not merged it: When #2310 lands, this should go green unchanged. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Owner
Author
|
recreate-cla |
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CARD TITLE (intent, not commit subject): Fix-forward PR 2306 (doc-gate config errors): 2 config-error paths still exit 1
Autonomous build of board card tsk-aerar5.
Pr #2306 left two config-error paths that still exited 1 instead of the
config-error code:
Structurally invalid config (valid TOML, wrong shape, e.g. rules is a
string) passed load_config and died later in the rule loop with an
AttributeError. Add _validate_config to check the shape after parsing.
Non-UTF-8 config bytes raised UnicodeDecodeError inside tomllib.load
(which decodes before parsing), uncaught by the TOMLDecodeError-only
handler. Catch that alongside the other config errors.
Also move EXIT_CONFIG_ERROR from 2 to 3: exit code 2 is argparse's own
usage-error exit, so a typo'd flag was indistinguishable from a broken
config. Now 1 (violation), 2 (argparse), and 3 (config error) are all
mutually distinct.
Supersedes exec/tsk-thh54d (pr #2306).
Files:
scripts/check_doc_gate.py | 48 +++++++++++++++++++++++++++++--
tests/test_doc_gate.py | 72 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 117 insertions(+), 3 deletions(-)
Summary by CodeRabbit
Bug Fixes
Tests