Detecting the format symbols in description.#42
Conversation
# Conflicts: # .github/workflows/remove-adept-to-close-on-issue-close.yml
WalkthroughThe PR introduces markdown content sanitization to prevent formatting symbols from breaking GitHub issue templates. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The base branch was changed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/helpers.py (1)
64-67: Type hint doesn't reflect nullable input/return.
sanitize_markdownaccepts and returnsNone(the early-return branch on falsytext), but the signature is declared astext: str→str. Static type checkers will flag the call sites that passalert.metadata.rule_description or rule_idexpressions where the left operand may legitimately beNone, and callers forwarding the return value into template values. Please widen the annotation to reflect reality.🔧 Proposed fix
-def sanitize_markdown(text: str) -> str: - """Escape block-level Markdown so text renders as plain content in an issue body.""" - if not text: - return text +def sanitize_markdown(text: str | None) -> str | None: + """Escape block-level Markdown so text renders as plain content in an issue body.""" + if not text: + return text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/helpers.py` around lines 64 - 67, The type annotations for sanitize_markdown are incorrect because the function accepts and returns None; update the signature to accept Optional[str] and return Optional[str] and add the necessary typing import (Optional) so callers and static type checkers see the nullable contract; keep the function behavior the same (early return when text is falsy) and update any local references to the parameter type if needed.tests/core/test_helpers.py (1)
30-31: Assert order: expected first.-def test_passthrough_unchanged(text: str | None) -> None: - assert sanitize_markdown(text) == text +def test_passthrough_unchanged(text: str | None) -> None: + assert text == sanitize_markdown(text)As per coding guidelines:
tests/**/*.py: Use assert pattern with expected first:assert expected == actual.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_helpers.py` around lines 30 - 31, The test assertion in test_passthrough_unchanged uses actual first; change it to follow the expected-first pattern by asserting the expected value (text) is equal to the actual result of sanitize_markdown(text). Update the assertion in the test_passthrough_unchanged function to use assert text == sanitize_markdown(text) so expected comes first and actual second.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/helpers.py`:
- Around line 23-30: The sanitizer in src/core/helpers.py currently escapes ATX
headings, blockquotes, HRs and table pipes but misses fenced code blocks and
setext H1 underlines; add regex patterns (e.g., _FENCE_RE =
re.compile(r"^(`{3,}|~{3,})", re.MULTILINE) and _SETEXT_RE =
re.compile(r"^(=+)\s*$", re.MULTILINE)) and apply them in the same sanitization
sequence where _HEADING_RE/_BLOCKQUOTE_RE/_HR_RE/_TABLE_RE are used (the
substitutions that write into the sanitized variable), and add a unit test case
exercising a fenced code block and an = underline so CHILD_BODY_TEMPLATE
headings aren’t swallowed.
In `@tests/security/issues/test_builder.py`:
- Line 326: The loop variable `l` in the generator expressions inside the
assertions (e.g., assert not any(l.strip().startswith("## Black") for l in
body.split("\n"))) is ambiguous and triggers ruff E741; rename it to a clearer
identifier such as `line` (or `ln`/`line_str`) in both occurrences (the one
shown and the similar one at the other assertion) so the expressions read like
any(line.strip().startswith(... ) for line in body.split("\n")) to resolve the
lint error.
---
Nitpick comments:
In `@src/core/helpers.py`:
- Around line 64-67: The type annotations for sanitize_markdown are incorrect
because the function accepts and returns None; update the signature to accept
Optional[str] and return Optional[str] and add the necessary typing import
(Optional) so callers and static type checkers see the nullable contract; keep
the function behavior the same (early return when text is falsy) and update any
local references to the parameter type if needed.
In `@tests/core/test_helpers.py`:
- Around line 30-31: The test assertion in test_passthrough_unchanged uses
actual first; change it to follow the expected-first pattern by asserting the
expected value (text) is equal to the actual result of sanitize_markdown(text).
Update the assertion in the test_passthrough_unchanged function to use assert
text == sanitize_markdown(text) so expected comes first and actual second.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bb6d73f-3675-4964-bfd0-aab2e4d5a047
📒 Files selected for processing (5)
.github/copilot-instructions.mdsrc/core/helpers.pysrc/security/issues/builder.pytests/core/test_helpers.pytests/security/issues/test_builder.py
Overview
This pull request introduces a new utility function to sanitize block-level Markdown, ensuring that text rendered in issue bodies does not accidentally trigger Markdown formatting. It applies this sanitization to security issue generation, particularly for fields that may contain user-supplied or external Markdown content. The changes also include comprehensive tests for the new helper.
Release Notes
Related
Closes #27
Summary by CodeRabbit
Bug Fixes