check_doc_gate invariants: file:line citations and cross-repo paths fail as nonexistent - #2307
check_doc_gate invariants: file:line citations and cross-repo paths fail as nonexistent#2307jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe documentation gate now excludes closing Markdown brackets from path tokens and removes trailing line references before validation. Tests cover file citations with line suffixes and external Markdown link URLs. ChangesDocumentation path parsing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 QodoFix doc-gate invariant parsing for file:line citations and markdown links
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
nemotron-super review VERDICT: LGTM Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # table), which would otherwise falsely flag deploy-time paths that never | ||
| # exist in the repo itself. | ||
| _TOKEN_RE = re.compile(r"(?<![\w/])(?:scripts|tinyagentos|docs|desktop)/[^\s`\"'|]+") | ||
| _TOKEN_RE = re.compile(r"(?<![\w/])(?:scripts|tinyagentos|docs|desktop)/[^\s`\"'|)\]]+") |
There was a problem hiding this comment.
WARNING: Paths containing ) or ] in filenames will be truncated
The new _TOKEN_RE exclusion of ) and ] prevents markdown-link syntax from being consumed, but it also means any legitimate repo path containing these characters (e.g. docs/foo(bar).md) will be cut off at the first occurrence. This causes the existence check to look for a truncated, nonexistent path and report a false positive.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 69.6K · Output: 23.6K · Cached: 502.1K |
Code Review by Qodo
1. Line strip misses punctuation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) | ||
| while token and token[-1] in _TRAILING_PUNCT: | ||
| token = token[:-1] |
There was a problem hiding this comment.
1. Line strip misses punctuation 🐞 Bug ≡ Correctness
In _clean_token(), the file:line suffix regex is applied before trimming _TRAILING_PUNCT, so tokens like scripts/foo.sh:123, or scripts/foo.sh:123. keep :123 after punctuation is removed and can be reported as nonexistent paths. This can still break check_referenced_paths() for common prose citations that aren’t wrapped in backticks/brackets.
Agent Prompt
## Issue description
`_clean_token()` strips `:line` / `:line-range` suffixes before removing trailing punctuation. When a citation ends with punctuation (e.g. `scripts/foo.sh:123,`), the `re.sub(...$)` does not match (because of the comma/period), then punctuation is removed, leaving `scripts/foo.sh:123` which later fails existence checks.
## Issue Context
The PR is explicitly hardening doc-gate invariants for file:line citations; the current ordering means only the “no trailing punctuation” form is reliably handled.
## Fix Focus Areas
- scripts/check_doc_gate.py[56-70]
- tests/test_doc_gate.py[178-201]
## Suggested fix
- Move the `re.sub(r":[0-9]+...$", "", token)` to run **after** the `_TRAILING_PUNCT` trimming loop, or run it both before and after (second pass after punctuation trimming).
- Add regression tests covering at least:
- `See scripts/foo.sh:123, for details.`
- `See scripts/foo.sh:123. for details.`
asserting `failures == []` when `scripts/foo.sh` exists.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Good call taking the dedicated line-citation pattern over splitting on the first colon. Splitting would also truncate any path that legitimately contains one, and The strip is in the wrong place in
Every one of those leaks a suffix into the existence check and fails. "See Worth naming why CI is green on it: Two-part fix, both in
_LINE_SUFFIX_RE = re.compile(r"(?::[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*)+$")
...
while token and token[-1] in _TRAILING_PUNCT:
token = token[:-1]
token = _LINE_SUFFIX_RE.sub("", token)Verified against all of the above plus Please extend the test to the four failing shapes rather than only the bare one, and confirm each fails on the current commit before the reorder. A test that has only ever been seen passing does not tell us the reorder did anything. Unrelated to the above, flagging rather than blocking: |
jaylfc
added
the
lead-blocked
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
🧹 Nitpick comments (2)
tests/test_doc_gate.py (2)
189-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the external URL test detect accidental URL extraction.
The local label and the URL both end with
docs/agent-manual.md. If the URL path is incorrectly consumed, the extracted path still exists locally, so the test passes. Use a distinct nonexistent URL target, such asdocs/external-only.md, while keepingdocs/agent-manual.mdas the local label.🤖 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 `@tests/test_doc_gate.py` around lines 189 - 201, The test_markdown_link_url_not_consumed_as_path test must use a distinct nonexistent external URL target while retaining docs/agent-manual.md as the local link label. Change the URL portion to reference a path such as docs/external-only.md so accidental URL extraction produces a detectable failure.
178-187: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression cases for punctuation and repeated suffixes.
This test covers only
scripts/foo.sh:123without trailing punctuation. Add cases such as:123.,:10-12,, and:12:34. These cases would currently fail normalization.🤖 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 `@tests/test_doc_gate.py` around lines 178 - 187, Add regression coverage to test_file_line_citation_is_ignored for file:line references followed by punctuation and repeated suffixes, including scripts/foo.sh:123., scripts/foo.sh:10-12,, and scripts/foo.sh:12:34. Assert each citation is normalized to the existing file and produces no failures through dg.check_referenced_paths.
🤖 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 63-65: Update the token-normalization logic in
scripts/check_doc_gate.py to remove trailing punctuation before applying the
line-citation regex. Extend that regex to strip repeated colon-number suffixes,
including forms such as :12:34, while preserving existing range and
comma-separated citation handling.
---
Nitpick comments:
In `@tests/test_doc_gate.py`:
- Around line 189-201: The test_markdown_link_url_not_consumed_as_path test must
use a distinct nonexistent external URL target while retaining
docs/agent-manual.md as the local link label. Change the URL portion to
reference a path such as docs/external-only.md so accidental URL extraction
produces a detectable failure.
- Around line 178-187: Add regression coverage to
test_file_line_citation_is_ignored for file:line references followed by
punctuation and repeated suffixes, including scripts/foo.sh:123.,
scripts/foo.sh:10-12,, and scripts/foo.sh:12:34. Assert each citation is
normalized to the existing file and produces no failures through
dg.check_referenced_paths.
🪄 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: 99c9fa57-8491-495a-b4ce-4d0b704eecf5
📒 Files selected for processing (2)
scripts/check_doc_gate.pytests/test_doc_gate.py
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) | ||
| while token and token[-1] in _TRAILING_PUNCT: | ||
| token = token[:-1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip punctuation before removing line citations.
The regex runs before trailing punctuation is removed. Therefore, tokens such as scripts/foo.sh:123. or scripts/foo.sh:10-12, retain their line suffix after the punctuation loop and fail the existence check. The regex also does not remove repeated suffixes such as scripts/foo.sh:12:34.
Move the punctuation cleanup before the regex and extend the regex to support repeated :\d+ components.
Proposed fix
- token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token)
while token and token[-1] in _TRAILING_PUNCT:
token = token[:-1]
+ token = re.sub(
+ r":[0-9]+(?::[0-9]+)*(?:-[0-9]+)?(?:,[0-9]+(?::[0-9]+)*(?:-[0-9]+)?)*$",
+ "",
+ token,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) | |
| while token and token[-1] in _TRAILING_PUNCT: | |
| token = token[:-1] | |
| while token and token[-1] in _TRAILING_PUNCT: | |
| token = token[:-1] | |
| token = re.sub( | |
| r":[0-9]+(?::[0-9]+)*(?:-[0-9]+)?(?:,[0-9]+(?::[0-9]+)*(?:-[0-9]+)?)*$", | |
| "", | |
| token, | |
| ) |
🤖 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 63 - 65, Update the
token-normalization logic in scripts/check_doc_gate.py to remove trailing
punctuation before applying the line-citation regex. Extend that regex to strip
repeated colon-number suffixes, including forms such as :12:34, while preserving
existing range and comma-separated citation handling.
1 participant
CARD TITLE (intent, not commit subject): check_doc_gate invariants: file:line citations and cross-repo paths fail as nonexistent
Autonomous build of board card tsk-zcnklp.
Files:
scripts/check_doc_gate.py | 3 ++-
tests/test_doc_gate.py | 24 ++++++++++++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
Summary by CodeRabbit
Bug Fixes
path:123or line ranges.Tests