Repository navigation
fix(link-checker): exclude fenced code blocks from anchor extraction - #1436
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the link checker incorrectly identified anchors within fenced code blocks. By implementing a fence-skipping mechanism similar to the one used for link collection, the tool now ensures that only valid, rendered Markdown headings and HTML anchors are indexed, preventing dead fragment links caused by code comments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the anchor extraction logic in tools/check_all_links.py to ignore headings and HTML anchors located inside fenced code blocks, and adds a corresponding unit test. The review feedback correctly identifies that the proposed implementation uses a simple boolean toggle which fails to handle nested code blocks with varying backtick counts, and provides a robust code suggestion to track the specific backtick count of the active outer fence.
| lines: List[str] = [] | ||
| in_fence = False | ||
| for line in content.splitlines(): | ||
| stripped = line.strip() | ||
| if stripped.startswith("```"): | ||
| in_fence = not in_fence | ||
| continue | ||
| if not in_fence: | ||
| lines.append(line) | ||
| unfenced_content = "\n".join(lines) |
There was a problem hiding this comment.
The current code fence detection logic uses a simple boolean toggle (in_fence = not in_fence) whenever a line starts with "```". This approach fails to correctly handle nested code blocks (e.g., a markdown code block showing another code block inside it using different backtick counts). In such cases, the inner code block's fences will toggle in_fence back to False, causing headings or HTML anchors inside the nested block to be incorrectly extracted as valid anchors.
To resolve this, we can track the specific backtick count of the active outer fence and only close it when a matching fence of at least that length is encountered.
| lines: List[str] = [] | |
| in_fence = False | |
| for line in content.splitlines(): | |
| stripped = line.strip() | |
| if stripped.startswith("```"): | |
| in_fence = not in_fence | |
| continue | |
| if not in_fence: | |
| lines.append(line) | |
| unfenced_content = "\n".join(lines) | |
| lines: List[str] = [] | |
| fence_backticks = 0 | |
| for line in content.splitlines(): | |
| stripped = line.strip() | |
| if fence_backticks == 0: | |
| backticks = len(stripped) - len(stripped.lstrip(chr(96))) | |
| if backticks >= 3 and chr(96) not in stripped[backticks:]: | |
| fence_backticks = backticks | |
| continue | |
| else: | |
| backticks = len(stripped) - len(stripped.lstrip(chr(96))) | |
| if backticks >= fence_backticks and chr(96) not in stripped[backticks:]: | |
| fence_backticks = 0 | |
| continue | |
| if fence_backticks == 0: | |
| lines.append(line) | |
| unfenced_content = "\n".join(lines) |
There was a problem hiding this comment.
Updated in 537cc86 to track the outer fence backtick length and support nested code blocks.
537cc86 to
e347c49
Compare
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed e347c49da213d071911faf58527d5edd5aa6436b and the earlier nested-fence feedback. All 19 tests in python3 -m unittest tools.test_check_all_links pass, and ordinary fences, shorter nested fences, and longer whitespace-only closing fences pass additional offline checks.
One correctness gap remains in the closing-fence condition at tools/check_all_links.py:190: checking that the suffix contains no backtick also accepts a suffix such as python. A closing fence may only have trailing spaces, per the GFM specification.
This reproduces on the reviewed head:
from tools.check_all_links import extract_anchors_from_content
content = "```markdown\n```python\n# Hidden\n```\n# Visible\n"
print(extract_anchors_from_content(content))
# Actual: {'hidden'}
# Expected: {'visible'}The python line is literal content inside the outer fence. The current condition closes the fence there, indexes # Hidden, then treats the real closer as a new opener and drops # Visible. This both accepts a dead anchor and newly rejects a valid section anchor; current main still includes visible for this input.
Please require an empty/whitespace-only suffix when closing an active fence and add a regression covering this case. This is a different boundary from the shorter nested fences covered by the existing test. Validation here was offline; no external URL or API calls were exercised.
|
@BichengWang Good catch on the GFM closing fence spec. Updated the closing condition to require an empty/whitespace-only suffix so inner fences carrying an info string are treated as literal content rather than premature closers, and added the reproduction as a regression test in est_check_all_links.py. All tests passing! |
|
Thanks @mohmedmm, nice fix and thanks @BichengWang for the closing-fence review; confirmed the current head handles that case. Verified: the #1435 reproducer now fails as it should (In-page anchor '#install-dependencies' not found), 20/20 tests pass and the two new ones fail on main, and a full internal audit of the repo gives identical results before and after (the code-comment "anchors" this removes aren't linked anywhere). One thing to fix: the PR changes the mode of tools/check_all_links.py from 100755 to 100644. That's unrelated to the fix, so please restore it to keep the diff focused (git update-index --chmod=+x tools/check_all_links.py). One suggestion: after the nesting fix, anchor extraction uses a stricter fence parser than link extraction, which still uses the simple in_fence = not in_fence toggle in two places. A single shared helper used by all three would keep headings and links on the same rules (and fix nested fences for links too). Ideally in this PR since you're already in this code, but a follow-up is fine if you prefer to keep this one minimal. Happy to re-review once the mode is restored. |
|
@kkorpal Restored 100755 mode for tools/check_all_links.py, and unified fence parsing across anchors and both link extraction paths with a single ilter_unfenced_lines helper as suggested. All 21 tests passing! |
Anchor extraction matched headings directly against raw document text without filtering code fences, causing code comments starting with
#to register as valid section anchors and passing dead fragment links. This adopts the same fence-skipping behavior used during link collection so only rendered Markdown headings and HTML anchors are indexed.Fixes #1435