ci: reject @mentions in pull request descriptions - #7496
Conversation
|
🕓 Ready for review — 3 ahead in queue (commit 9cde0fa) |
There was a problem hiding this comment.
Nack
No new bash code please.
Firstly, it is full of bashism (set -euo pipefail) which is not part of 'sh' standard, 2ndly it is difficult to write to make it safe, reliable and avoid any injections.
Consider calling dedicated python script instead if really need.
|
Addressed knst's nack in e076f3e. Replaced the inline bash/
Workflow now checks out the trusted base branch only ( with the untrusted PR body only via |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head addresses knst's older-SHA request by replacing the inline Bash implementation with a dedicated Python helper, passing the untrusted PR body through the environment, and adding 18 passing unit tests. Two blocking issues remain: renaming the job removes an existing required-check context, and the mention regex misses punctuation-adjacent mentions while rejecting some valid email addresses. The implementation rewrite and executable-mode follow-up should also be squashed into the original feature commit to keep the history coherent and bisectable.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/semantic-pull-request.yml`:
- [BLOCKING] .github/workflows/semantic-pull-request.yml:12: Preserve the existing required-check name
`Validate PR title` is currently a required check for this repository; GitHub's status-check metadata for PR #7496 marks that exact context as required. Changing the job display name makes future workflow runs emit `Validate PR title and description` instead, leaving the configured `Validate PR title` requirement permanently pending and blocking subsequent PRs from merging. Keep the existing job name; the new description-validation step can run within it, while the top-level workflow name can still describe the expanded behavior.
In `.github/workflows/check_pr_description_mentions.py`:
- [BLOCKING] .github/workflows/check_pr_description_mentions.py:30-32: Detect complete emails instead of suppressing punctuation-adjacent mentions
The negative lookbehind treats `.`, `+`, and `-` as email-local characters without establishing that the surrounding text is an email. GitHub renders `Thanks.@knst` and `cc +@knst` as user mentions, but `find_mentions()` returns no matches for either, allowing notifications that this check is intended to prevent. Conversely, valid dot-atom addresses such as `dev!@example.com` are not rendered by GitHub as mentions but are rejected here as `@example`. Detect complete email spans separately from mention candidates and add tests for punctuation-adjacent mentions and valid email local-part characters.
In `<commit:e076f3e>`:
- [SUGGESTION] <commit:e076f3e>:1: Squash the implementation rewrite and mode fix into the feature commit
Commit e076f3e1da2 replaces the inline Bash implementation introduced by 14715262e19 with the final Python implementation and tests, so these are successive revisions of one unshipped feature rather than independent logical changes. Commit 67a1e412ac5 only fixes the executable modes of the files added by e076f3e1da2, meaning that intermediate commit does not satisfy `lint-files` independently. Squash both follow-ups into 14715262e19 so the permanent history introduces the final tested implementation with correct modes in one coherent, bisectable commit.
Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
Flag GitHub @username mentions in PR bodies so they are not copied into merge commits and re-notify people on merge, rebase, or backport. Use a dedicated Python helper with unit tests, preserve the existing required-check name, and allow complete email addresses.
67a1e41 to
9bc7ed5
Compare
|
Addressed the current review findings in
Validation: 23 focused unit tests, Python bytecode compilation, Flake8 (excluding the repository-ignored E501 rule), YAML parsing, and |
|
The
This PR changes only the PR-description workflow and its Python helper/tests. It does not touch the failing functional test, connection helper, networking, or |
| from unittest import mock | ||
|
|
||
|
|
||
| SCRIPT_PATH = Path(__file__).with_name("check_pr_description_mentions.py") |
There was a problem hiding this comment.
do we have now tests for CI jobs? that's something new.
There was a problem hiding this comment.
lol; that's funny. @thepastaclaw please simplify. We don't need tests for this.
There was a problem hiding this comment.
Removed in b49d9db — dropped .github/workflows/test_check_pr_description_mentions.py and left just the helper + workflow step.
There was a problem hiding this comment.
Good callout. I dropped the dedicated unit-test file in b49d9db so this stays a simple helper + workflow step only.
|
The
This PR changes only No branch change or CI retrigger was made. Maintainers can rerun the lane if desired. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request adds a Python CLI that validates pull request descriptions. It accepts input from Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant BaseBranchCheckout
participant MentionValidator
GitHubActions->>BaseBranchCheckout: Checkout base branch
GitHubActions->>MentionValidator: Pass PR_BODY and invoke validator
MentionValidator->>GitHubActions: Return validation exit status
🚥 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: 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/workflows/check_pr_description_mentions.py:
- Around line 130-134: Update the exception handling around read_body(args) to
also catch UnicodeDecodeError, preserving the existing controlled error message
and return code for invalid UTF-8 body files alongside OSError.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 8103045b-0f5f-4b16-8050-4f19042d6b51
📒 Files selected for processing (2)
.github/workflows/check_pr_description_mentions.py.github/workflows/semantic-pull-request.yml
Reviewers asked to keep this check simple; the helper itself is enough without a dedicated CI unit-test file.
b49d9db to
9cde0fa
Compare
4 participants
Issue being fixed or feature implemented
PR descriptions that include GitHub username mentions get copied into merge commits. After that, those people get notified again whenever the PR is merged, rebased, or backported. See #7493 for the motivating case ("Thanks knst" / similar mentions in a release PR description).
This change makes CI fail when a PR description contains those mentions.
What was done?
Extended the existing semantic PR title workflow (
.github/workflows/semantic-pull-request.yml) so it also validates the PR description:amannn/action-semantic-pull-request@v6) unchangedgithub.event.pull_request.bodyvia env onpull_request_target(opened/edited/synchronize), writes it to a file, and fails on GitHub-style@usernamementionsHow Has This Been Tested?
git diff --check upstream/develop...HEADcleanThanks @knst.→ fail@PastaPastaPasta please look→ failcontact dev@example.com→ passno mentions here/ empty body / bare@/@@diff markers → passBreaking Changes
None. Existing PRs with
@usernamementions in their description will fail this check on the nextedited/synchronizeevent until the mentions are removed.Checklist: