fix: gate review notes on canonical invocation; accept natural target-selector prompts - #8
Open
CodeDeficient wants to merge 7 commits into
Open
fix: gate review notes on canonical invocation; accept natural target-selector prompts#8CodeDeficient wants to merge 7 commits into
CodeDeficient wants to merge 7 commits into
Conversation
'Review commit <sha>', 'Review <sha>', 'Review PR #7', and 'Review branch <name>' are now accepted as target-only prompts for enforcement-grade review notes. Adds stripTargetSelectorPrefix() which strips known prefixes before checking the remaining text against target-only rules. Custom methodology prompts (e.g. 'Review commit abc1234 for correctness...') remain non-canonical because the remaining text after stripping contains prose, not a bare target. Adds 4 new tests for the missed natural prompt cases.
…argets extractPrNumber now matches both #7 and PR 7 / Review PR 7. isTargetOnlyPrompt rejects bare numeric strings like '7' before branch-name fallback, preventing ambiguous targets from resolving to HEAD. stripTargetSelectorPrefix no longer strips 'review pr ' prefix so PR 7 stays intact for extractPrNumber. Adds 8 new tests: - extractPrNumber: PR 7, Review PR 7, PR #7 - isCanonicalReviewInvocation: Review PR 7, PR 7 (true); bare '7', Review 7, Review branch 7 (false) - resolveTargetSha: PR 7, Review PR 7 resolve to PR head
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| /** Extract a PR number (#NNN or PR NNN) from a string. */ | ||
| export function extractPrNumber(text: string): string | null { | ||
| const match = text.match(/#(\d+)/) | ||
| const match = text.match(/#(\d+)/) || text.match(/(?:^|\s)PR\s+(\d+)\b/i) |
There was a problem hiding this comment.
Suggestion: Explicit hashless PR references can be misclassified as SHAs because extractSha runs before extractPrNumber in resolveTargetSha. For example, PR 1234567 matches the seven-digit SHA pattern, so if a local Git ref named 1234567 exists, the plugin attaches the note to that ref instead of resolving PR 1234567 through gh pr view. Ensure explicit PR N syntax takes precedence over SHA extraction. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ PR review notes can target an unrelated local numeric ref.
- ❌ `gh pr view` is bypassed for affected PR numbers.
- ⚠️ Enforcement metadata may be attached to the wrong commit.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugin/review-note.ts
**Line:** 17:17
**Comment:**
*Incorrect Condition Logic: Explicit hashless PR references can be misclassified as SHAs because `extractSha` runs before `extractPrNumber` in `resolveTargetSha`. For example, `PR 1234567` matches the seven-digit SHA pattern, so if a local Git ref named `1234567` exists, the plugin attaches the note to that ref instead of resolving PR 1234567 through `gh pr view`. Ensure explicit `PR N` syntax takes precedence over SHA extraction.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| if (/^\d+$/.test(target)) return false | ||
|
|
||
| if (target.length <= 100 && /^[\w\-.\\/]+$/.test(target)) return true |
There was a problem hiding this comment.
Suggestion: The new prefix stripping makes arbitrary one-line instructions such as Review security or Review dependency-updates canonical because the remainder matches this branch-name pattern. If that name is not a Git ref, resolveTargetSha falls back to HEAD, and the plugin attaches an enforcement-grade note despite the prompt not identifying a valid target. Restrict the accepted natural-language forms or prevent unresolved target selectors from falling back to HEAD for canonical reviewer invocations. [logic error]
Severity Level: Critical 🚨
- ❌ Unresolved prompts can annotate the current HEAD.
- ❌ Review enforcement may report a review for the wrong target.
- ⚠️ Branch-name validation does not verify Git ref existence.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugin/review-note.ts
**Line:** 62:62
**Comment:**
*Logic Error: The new prefix stripping makes arbitrary one-line instructions such as `Review security` or `Review dependency-updates` canonical because the remainder matches this branch-name pattern. If that name is not a Git ref, `resolveTargetSha` falls back to HEAD, and the plugin attaches an enforcement-grade note despite the prompt not identifying a valid target. Restrict the accepted natural-language forms or prevent unresolved target selectors from falling back to HEAD for canonical reviewer invocations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix…usion
extractSha now skips hex matches preceded by 'PR' (case-insensitive),
mirroring the existing (?<!#) protection for #-prefixed PR refs.
This prevents 'Review PR 1234567' from resolving to a commit whose
SHA prefix matches those digits instead of the PR head.
Uses a loop-based scan so subsequent non-PR-prefixed hex matches
are still found (e.g. 'PR 1234567 and abc1234' returns abc1234).
Adds 3 new tests:
- extractSha('PR 1234567') returns null
- extractSha('Review PR 1234567') returns null
- resolveTargetSha('Review PR 1234567') resolves to PR, not SHA
extractPrNumber now only matches PR NNN when it appears at the start of the string or after 'review ' prefix. Incidental prose like 'Reviewing PR 8 changes' or 'check PR 7' no longer triggers PR resolution. Regex changed from /(?:^|\s)PR\s+(\d+)\b/i to /^(?:review\s+)?PR\s+(\d+)\b/i with .trim() to handle leading spaces from the concatenated searchText in resolveTargetSha. Adds 5 new tests: - extractPrNumber rejects 'Reviewing PR 8 changes' - extractPrNumber rejects 'check PR 7' - extractPrNumber rejects 'Please review PR 8' - resolveTargetSha does not resolve incidental 'PR NNN' prose - resolveTargetSha resolves 'PR 7' from concatenated searchText with leading spaces (simulates real plugin flow)
resolveTargetShaFromArgs() checks prompt, then /review Input: prefix, then description individually before falling back to combined search. This prevents generic descriptions like 'code review' from masking target selectors like 'PR 7' in the prompt field. - Add resolveTargetShaFromArgs() with field-aware resolution - Plugin uses resolveTargetShaFromArgs(args, deps) instead of resolveTargetSha(searchText, deps) - 8 new tests covering prompt-first resolution, incidental prose rejection, /review Input: prefix, and mixed-field scenarios - 78 pass, 0 fail, 108 expect() calls
The PR-boundary check only looked for PR$ but missed pr-, pr_, pr/ in branch names like upgrade-pr-1234567. Broaden to pr[\W_]*$ so any non-alphanumeric separator between 'pr' and hex digits is caught. 4 new tests: pr-, pr_, pr/ rejection, prep non-rejection.
isCanonicalReviewInvocation() previously accepted any task with command: 'review' regardless of prompt content. Since the task tool allows callers to set command, a custom reviewer task could spoof command: 'review' with arbitrary multiline prompts and still attach enforcement notes. Fix: command-based canonical status now requires the prompt to start with 'Input:' — the shape produced by the real /review command (command/review.md: 'Input: '). 11 new tests: 5 accept (Input: with various targets), 5 reject (spoofed command with multiline/instructional/bare/empty/incidental prompts), 3 existing tests updated to include realistic prompts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists
The review methodology was moved into the agent system prompt (PR #7), but two gaps remained:
subagent_type: "reviewer"invocation.Review commit 55f02e9fwere rejected as non-canonical, causing the plugin to skip note attachment.Review PR 7(without#) was accepted as canonical but resolved to HEAD instead of the PR head.Design decisions
Decision: Plugin-enforced canonical invocation gate
The plugin now checks
isCanonicalReviewInvocation()before attaching notes. Only two paths produce enforcement-grade notes:/review <target>command pathCustom reviewer prompts (e.g.
Review commit abc1234 for correctness...) still run but do not get a review note. Prompt hardening inagent/reviewer.mdis defense-in-depth only.Decision: Accept natural target-selector prompts
Review commit <sha>,Review <sha>,Review PR #7,Review PR 7, andReview branch <name>are now accepted as target-only prompts. ThestripTargetSelectorPrefix()function strips known prefixes before checking the remaining text against target-only rules.Decision: Reject bare numeric targets
Bare numeric strings like
"7"are rejected as ambiguous — they are neither SHA length nor explicit PR syntax. Only explicit PR refs (#7,PR 7,PR #7) are accepted.What this enables
#prefixRisks and safeguards
Risk: Custom reviewer prompts may still run but produce no enforcement note
Mitigation: Documented in README. Plugin logs skipped note attachment at info level.
Risk:
Review PR 7could be confused with a branch nameMitigation:
extractPrNumbernow matches both#7andPR 7patterns, and the resolver correctly callsgh pr view 7instead of falling back to HEAD.Validation
CodeAnt-AI Description
Accept natural review targets while preventing ambiguous numeric targets
What Changed
Review commit <sha>,Review <sha>,Review PR 7, andReview branch <name>.PR 7orPR #7, and these references resolve to the pull request head.7,Review 7, andReview branch 7are rejected instead of being treated as branch names or the current revision.Impact
✅ Natural review prompts produce enforcement notes✅ PR numbers without hashes open the correct PR revision✅ Fewer ambiguous reviews against the current branch💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.