check_doc_gate invariants: file:line citations and cross-repo paths fail as nonexistent - #2307
Conversation
📝 WalkthroughWalkthroughThe documentation gate excludes closing Markdown brackets from path tokens and removes trailing line references before validation. Tests cover line citations, punctuation, ranges, colon-containing filenames, 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 💡 1📝 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 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: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit cfcbb59)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cfcbb59)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 9a8eb41)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 71.5K · Output: 14.6K · Cached: 216.6K |
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
|
Lead fix-forward: rebased onto dev over the #2369 doc-gate rewrite (the conflict source). The two changes compose rather than collide — the token regex now carries BOTH #2369's hyphen lookbehind AND this PR's |
9a8eb41 to
cfcbb59
Compare
| _TOKEN_RE = re.compile(r"(?<![\w/-])(?:scripts|tinyagentos|docs|desktop)/[^\s`\"'|]+") | ||
| # never sees it. `)` and `]` are excluded from the token body so a markdown | ||
| # link's closing bracket ends the token instead of gluing the URL on. | ||
| _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 still be truncated
The new _TOKEN_RE exclusion of ) and ] prevents markdown-link URLs from being consumed, but it has the same effect as before: any legitimate repo path containing those characters (e.g. docs/foo(bar).md) will be cut off at the first occurrence. The existence check then looks for a truncated, nonexistent path and reports a false positive. There is no test or escape hatch for this case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for sep in ("#", "?", "::"): | ||
| if sep in token: | ||
| token = token.split(sep, 1)[0] | ||
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) |
There was a problem hiding this comment.
WARNING: Line-citation regex runs before trailing-punctuation strip — punctuation defeats the anchor
The re.sub(...$) is applied here before the _TRAILING_PUNCT while-loop on lines 78-79, so any trailing punctuation (,, .) prevents the end anchor from matching. After the loop strips the punctuation the :123 suffix is still present, so the existence check still fails. The new tests cover only the bare backticked form without trailing punctuation.
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) | |
| while token and token[-1] in _TRAILING_PUNCT: | |
| token = token = re.sub(r":[0-9]+(?::[0-9]+)*(?:-[0-9]+)?(?:,[0-9]+(?::[0-9]+)*(?:-[0-9]+)?)*$", "", token) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for sep in ("#", "?", "::"): | ||
| if sep in token: | ||
| token = token.split(sep, 1)[0] | ||
| token = re.sub(r":[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$", "", token) |
There was a problem hiding this comment.
WARNING: Repeated line-number suffixes (:12:34) are not fully stripped
_TRAILING_PUNCT includes :, so for a token like scripts/foo.sh:12:34 the punctuation loop strips the middle colon on its first pass, leaving scripts/foo.sh:1234. The anchored re.sub then fails to match and the dangling :1234 fails the existence check. Extend the regex with an outer (?:...)+ group to collapse all trailing line-reference suffixes in one pass.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…(lead block) An end-anchored suffix regex running before the punct loop is defeated by a sentence-ending stop, comma, or closing paren, and a single-pass group misses file:line:col. Reordered and wrapped in (?:...)+ per the block review; the four leaking shapes each proved red on the pre-reorder commit, and docs/weird:name.md stays preserved.
|
Block resolved (lead-completed, no worker on this conflicted branch). The suffix strip now runs AFTER the trailing-punct loop with the repeated group, exactly as the block prescribed. Red-first evidence — all four shapes measured on the pre-reorder commit: Post-reorder: all five parametrized shapes pass, |
jaylfc
removed
the
lead-blocked
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_doc_gate.py (1)
219-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the external-link regression test discriminating.
The external URL uses the same
docs/agent-manual.mdpath created locally. If the parser incorrectly consumes that URL path, the existence check still passes. Use a remote-only path or assert the exact output ofextract_path_tokens().Proposed test adjustment
readme.write_text( "See [docs/agent-manual.md]" - "(https://github.com/other/repo/blob/main/docs/agent-manual.md).\n" + "(https://github.com/other/repo/blob/main/docs/remote-only.md).\n" )🤖 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 219 - 231, Update test_markdown_link_url_not_consumed_as_path so the external URL references a path absent from tmp_path, or additionally assert extract_path_tokens() excludes the URL path. Keep the test verifying that check_referenced_paths returns no failures for the local documentation link.
🤖 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.
Outside diff comments:
In `@tests/test_doc_gate.py`:
- Around line 219-231: Update test_markdown_link_url_not_consumed_as_path so the
external URL references a path absent from tmp_path, or additionally assert
extract_path_tokens() excludes the URL path. Keep the test verifying that
check_referenced_paths returns no failures for the local documentation link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9aab635-c8d8-4885-bfaf-86f45c0fccb8
📒 Files selected for processing (2)
scripts/check_doc_gate.pytests/test_doc_gate.py
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
Tests