fix(ci): ignore HTML comments when validating linked issues - #1791
fix(ci): ignore HTML comments when validating linked issues#1791ardelperal wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughPR issue-reference validation now ignores HTML comments, extracts visible closing references through reusable helpers, updates both workflow checks, documents the template placeholder, and adds regression tests. ChangesPR issue-reference validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PRBody
participant GitHubActions
participant IssueReferenceParser
PRBody->>GitHubActions: provide PR body
GitHubActions->>IssueReferenceParser: extract visible issue references
IssueReferenceParser-->>GitHubActions: return issue number array
GitHubActions->>GitHubActions: validate references and approval state
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/scripts/test-validate-pr-issue-references.js:
- Around line 10-48: Extend the tests for extractLinkedIssueNumbers with a
visible Fixes `#N` reference, asserting its issue number is extracted. Add
malformed-reference cases for text such as “discloses `#42`” and “Closes `#42abc`”,
asserting these do not produce false-positive issue numbers.
In @.github/scripts/validate-pr-issue-references.js:
- Around line 5-6: Update CLOSING_REFERENCE_PATTERN to enforce standalone
closing keywords and issue numbers: require a non-word boundary before closes,
fixes, or resolves so embedded prose such as “discloses” does not match, and
require a non-word boundary after the digits so suffixes such as “#42abc” are
rejected. Preserve case-insensitive matching and support the existing optional
whitespace before the issue number.
In @.github/workflows/pr-check.yml:
- Around line 49-57: Update both issue-reference steps at
.github/workflows/pr-check.yml lines 49-57 and 77-86 to avoid requiring
validate-pr-issue-references.js from the PR checkout. Load the parser from a
trusted base revision or use a pinned inline implementation, ensuring neither
step executes PR-controlled code with repository credentials.
- Around line 49-50: Add persist-credentials: false to both actions/checkout
steps in .github/workflows/pr-check.yml at lines 49-50 and 77-78, leaving the
existing checkout references unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12cc6cf8-adb2-4244-8576-a56c64c0954d
📒 Files selected for processing (4)
.github/PULL_REQUEST_TEMPLATE.md.github/scripts/test-validate-pr-issue-references.js.github/scripts/validate-pr-issue-references.js.github/workflows/pr-check.yml
| - name: Check out repository | ||
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow snippets =="
sed -n '35,90p' .github/workflows/pr-check.yml | cat -n
echo "== checkout usages in workflows =="
rg -n "actions/checkout|persist-credentials|git (commit|push|pull|fetch|apply|checkout|stash|rebase|diff)" .github/workflows || true
echo "== all checkout uses =="
rg -n "uses:\s*actions/checkout@" .github/workflows || trueRepository: Gentleman-Programming/gentle-ai
Length of output: 5369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path(".github/workflows/pr-check.yml")
text = p.read_text()
# Simple line-number extraction of actions/checkout entries in YAML steps.
for m in re.finditer(r"^(\s*)- name: .*?\n(?:\s+\S+.*\n)*?\s+uses:\s*actions/checkout@([a-f0-9]{40})(?:\s+#.*\n)?(?:\s+(\S+):\s*(.*?)(?:\n|$))?", text, re.M):
# Count lines by counting newlines before this match.
lineno = text.count("\n", 0, m.start()) + 1
indent_len = len(m.group(1))
yaml_block_indent = 6 # action step list item indent
uses_lineno = lineno + text.count("\n", len(m.group(0)), m.end()) - 1
persist_line = None
for i, line in enumerate(m.group(0).splitlines()):
stripped = line.strip()
if stripped.startswith("persist-credentials:"):
persist_line = True
break
print(f"checkout at approx lines {lineno}: uses line ~{uses_lineno}, persist-credentials present: {persist_line}, full block:\n" + "\n".join(m.group(0).splitlines()[:12]))
PYRepository: Gentleman-Programming/gentle-ai
Length of output: 161
🌐 Web query:
actions/checkout v5 persist-credentials default true README
💡 Result:
In the actions/checkout v5 action, the persist-credentials input defaults to true [1][2][3]. When persist-credentials is set to true (the default), the action configures the provided authentication token or SSH key into the local git configuration [4][2][5]. This allows subsequent git commands (such as git fetch or git push) run within the same job to be automatically authenticated [4][6][5]. The action includes a post-job cleanup step to remove these credentials [4][2][5]. If you wish to opt-out of this behavior—for example, to prevent the credentials from being persisted in the local git config—you can explicitly set persist-credentials to false in your workflow file [4][2][5].
Citations:
- 1: https://git.woni.link/actions/checkout/src/branch/releases/v5/action.yml
- 2: https://feicode.com/actions/checkout/src/tag/v5.1.0/README.md
- 3: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 4: https://github.com/actions/checkout
- 5: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 6: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
Disable persisted checkout credentials in both jobs.
These checkouts only read repository files, so prevent actions/checkout from writing GITHUB_TOKEN into local Git config:
.github/workflows/pr-check.yml#L50.github/workflows/pr-check.yml#L78
Add persist-credentials: false to both checkout steps.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 49-50: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 1 file
.github/workflows/pr-check.yml#L49-L50(this comment).github/workflows/pr-check.yml#L77-L78
🤖 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 @.github/workflows/pr-check.yml around lines 49 - 50, Add
persist-credentials: false to both actions/checkout steps in
.github/workflows/pr-check.yml at lines 49-50 and 77-78, leaving the existing
checkout references unchanged.
Source: Linters/SAST tools
… with credentials Removes actions/checkout from the check-issue-reference and check-issue-approved steps. The parser logic now lives inline in the actions/github-script block; the JS file is kept for tests only. This eliminates the attack surface identified by CodeRabbit: a malicious PR could replace validate-pr-issue-references.js in its own branch, and since actions/checkout fetched the PR HEAD, that file would run with repo credentials. By inlining the parser there is no file to tamperize. CodeRabbit review id: 9dd0ad68-8efd-410c-be6c-c66a0226eed3
Tightens CLOSING_REFERENCE_PATTERN to reject embedded prose and malformed references. The new pattern requires either start-of-string or a non-word-character boundary before the closing keyword, preventing matches against words like "discloses" or "encloses". It also enforces a word boundary after the digit so that trailing characters like "abc" in "#42abc" are rejected. Same logic mirrored inline in the workflow step; JS file is the canonical source-of-truth used by tests. CodeRabbit review id: 9dd0ad68-8efd-410c-be6c-c66a0226eed3
Three new test rows exercising Fixes keyword, embedded prose, and malformed trailing characters. These cover the boundary conditions added by the standalone-keyword regex tightening. CodeRabbit review id: 9dd0ad68-8efd-410c-be6c-c66a0226eed3
fbb8cc7 to
64557d7
Compare
|
All four CodeRabbit findings from PR #1791 are now addressed in three new commits on this branch:
Local validation: |
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 @.github/scripts/validate-pr-issue-references.js:
- Around line 14-16: Update stripHtmlComments in
.github/scripts/validate-pr-issue-references.js to remove HTML comments only
outside fenced code blocks, preserving literal <!-- Closes `#42` --> text inside
fences; apply the same fence-aware behavior at .github/workflows/pr-check.yml
lines 55-57 and 95-97. Add a regression case in
.github/scripts/test-validate-pr-issue-references.js lines 44-48 covering a
fenced comment-looking reference.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c367105-0534-4f01-b2b6-04baaecddc09
📒 Files selected for processing (4)
.github/PULL_REQUEST_TEMPLATE.md.github/scripts/test-validate-pr-issue-references.js.github/scripts/validate-pr-issue-references.js.github/workflows/pr-check.yml
| function stripHtmlComments(markdown) { | ||
| return markdown.replace(HTML_COMMENT_PATTERN, ''); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve literal comment syntax inside fenced code blocks.
The parser treats fenced closing references as visible, but stripHtmlComments removes <!-- Closes #42 --> inside a fence, so CI rejects that otherwise-visible reference. Make comment stripping Markdown-context-aware, or stop treating fenced references as valid.
.github/scripts/validate-pr-issue-references.js#L14-L16: preserve comment-looking literals while inside fenced code..github/scripts/test-validate-pr-issue-references.js#L44-L48: add a fenced<!-- ClosesInstalacion en debian 13 #42-->regression case..github/workflows/pr-check.yml#L55-L57: apply the same fence-aware behavior..github/workflows/pr-check.yml#L95-L97: apply the same fence-aware behavior.
📍 Affects 3 files
.github/scripts/validate-pr-issue-references.js#L14-L16(this comment).github/scripts/test-validate-pr-issue-references.js#L44-L48.github/workflows/pr-check.yml#L55-L57.github/workflows/pr-check.yml#L95-L97
🤖 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 @.github/scripts/validate-pr-issue-references.js around lines 14 - 16, Update
stripHtmlComments in .github/scripts/validate-pr-issue-references.js to remove
HTML comments only outside fenced code blocks, preserving literal <!-- Closes
`#42` --> text inside fences; apply the same fence-aware behavior at
.github/workflows/pr-check.yml lines 55-57 and 95-97. Add a regression case in
.github/scripts/test-validate-pr-issue-references.js lines 44-48 covering a
fenced comment-looking reference.
…ences Tighten CLOSING_REFERENCE_PATTERN with a negative lookahead (?![A-Za-z0-9_]) so a closing reference like \Closes #42abc\ is rejected as malformed, while preserving the valid forms \Closes Gentleman-Programming#1770\ and \Fixes Gentleman-Programming#123\. The previous regex used \\b\ as the trailing delimiter, which in JavaScript only matches between a word and a non-word character. A trailing letter sequence like \�bc\ satisfies that, so \#42abc\ extracted \42\ as a valid issue number. The status:approved gate then validated the non-existent or unapproved Gentleman-Programming#42 against the live API and failed, which is exactly the regression the previous commit introduced. The new lookahead explicitly rejects any alphanumeric or underscore character after the digits, so trailing letters are part of the same token and the reference is dropped. Test row 3 is updated: the original \Closes #42abc\\nFixes Gentleman-Programming#99-extra\ case used a fake issue number (Gentleman-Programming#99) and broke the status:approved gate in CI. The replacement uses Gentleman-Programming#1770 (the real linked issue) and asserts that malformed neighbours are filtered while the valid reference survives.
b42db0a to
15038ae
Compare
|
Hey @Alan-TheGentleman, this is ready for review when you have a moment. Status:
No new commits since the last CodeRabbit review. Happy to iterate if anything else surfaces. |
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Please make fenced-code behavior consistent. The parser treats references inside fenced code as visible, but blanket HTML-comment stripping removes <!-- Closes #42 --> inside those fences; both inline workflow copies share the same inconsistency and no regression covers it. Define the intended rule once, apply it to the script and workflow copies, add the fenced-comment test, restore the exact PR template, rebase onto current main, and rerun CI.
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Implementation review is blocked. Linked issue #1770 is currently status:needs-info and must regain maintainer approval after the requested clarification before this PR can receive implementation review. Please complete that issue work first, then request review again.
|
Closing this one in favor of maintainer PR #2511, which has been merged with the complete fix for issue #1770. The HTML-comment stripping, regex tightening, and regression coverage all landed there, so keeping both open would mean duplicate work on the same root cause. Thanks for the careful iteration on this. The detailed PR body, the four corrective commits, and the security-first inline-parser approach made it very easy to see what needed to land. That work directly shaped the final merged fix. |
2 participants
🔗 Linked Issue
Closes #1770
🏷️ PR Type
type:bug- Bug fix (non-breaking change that fixes an issue). The GitHub label is pending maintainer action.📝 Summary
This PR applies three corrective commits on top of the original HTML-comment-stripping fix (commit
7b5cba29):fix(ci): inline pr-issue-reference parser to avoid PR-controlled code with credentials(73a696f7) — removesactions/checkoutfrom the two issue-reference validation steps. The parser logic now lives inline in theactions/github-scriptblock; the JS file is retained for tests only. This closes the Critical security finding from CodeRabbit.**
fix(ci): require standalone closing keywords with word boundaries** (7dd4080) — tightens the regex to/(^|[^A-Za-z0-9_])(?:closes|fixes|resolves)\s+#(\d+)(?![A-Za-z0-9_])/gi. A non-word-character boundary is required before the closing keyword so prose likediscloses Instalacion en debian 13 #42does not match. A non-word-character boundary is required after the digits so malformed references with trailing letters likeCloses #42abc` are rejected. This closes the Major regex finding from CodeRabbit.test(ci): cover visible Fixes and malformed reference cases(64557d72) — adds regression test rows for a visibleFixes #1770reference, embedded prosediscloses #42returning empty, and malformedCloses #42abcreturning empty. This closes the Trivial test-coverage finding from CodeRabbit.All four CodeRabbit findings from PR #1791 are addressed. Local test suite: 10 of 10 passing.
📂 Changes
Counts from
git diff --numstat origin/main...HEAD. Branch:fix/ci-html-comment-validation. New head:15038ae2..github/scripts/validate-pr-issue-references.js.github/scripts/test-validate-pr-issue-references.jsnode:testcases: six original + four new..github/workflows/pr-check.ymlactions/checkoutsteps; parser inlined into eachactions/github-scriptstep..github/PULL_REQUEST_TEMPLATE.mdTotal: 151 additions, 9 deletions, 4 files — well below the 400-line review budget.
🧪 Test Plan
All commands run locally on Windows in Standard Mode.
Test suite via
node:test, ten cases, all passing:Fixesreference against the real linked issue.discloses(prose, not a closing verb).node --test .github/scripts/test-validate-pr-issue-references.jsAutomated Checks
Closes #1770.status:approved.gh pr edit --add-label type:bugreturns 403 for the contributor.✅ Contributor Checklist
status:approvedtype:*label added — pending maintainer; do not read the box above as an API label claimsize:exceptionadded — not requested and not requirednode --test10 of 10)go build/go vetregressions — N/A, no Go changesCo-Authored-Bytrailersoriginfix/ci-html-comment-validationorigin/main💬 Notes for Reviewers
Dependency diagram:
Security note on commit
73a696f7: bothactions/checkoutsteps were removed from the workflow. The parser logic now lives entirely inline inactions/github-script, so there is no PR-controlled JavaScript file executed with repository credentials. This eliminates the attack surface identified in the Critical CodeRabbit finding.The regex commit
15038ae2tightens the trailing boundary: a negative lookahead(?![A-Za-z0-9_])rejects malformed references with trailing letters, while still accepting the valid forms. This prevents the status:approved gate from validating fake issue numbers that the previous\bboundary would have accepted (e.g.Closes #42abcextracted as42).The original commit (
7b5cba29) addressed the HTML-comment-stripping bug. The four corrective commits (73a696f7,7dd40801,64557d72,15038ae2) address all four CodeRabbit review findings plus the CI regression surfaced by them.Pending maintainer actions
type:buglabel to this PR to satisfyCheck PR Has type:* Label.