fix(pr-reviewer): resolve PR author from sender login#31
Merged
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
codeant-ai
Bot
added
the
size:L
📝 WalkthroughWalkthroughThe PR refactors the PR review agent's VFS client to use new *2-aliased fs/promises imports with enhanced error handling and path validation, updates the relay integration to use these new helpers, and extends PR author resolution with a ChangesPR Review Agent VFS Refactoring and Author Resolution
Possibly related PRs
Suggested labels
Poem
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| number, | ||
| url: prRef?.html_url ?? `https://github.com/${owner}/${repo}/pull/${number}`, | ||
| author: p?.pull_request?.user?.login ?? 'unknown', | ||
| author: p?.pull_request?.user?.login ?? (p?.pull_request ? p?.sender?.login : undefined) ?? 'unknown', |
There was a problem hiding this comment.
Suggestion: Falling back to sender.login for every payload that contains pull_request can misidentify the PR author on review/comment events, where sender is often the reviewer/commenter rather than the opener. If PR metadata is temporarily unavailable, the allowlist check can be evaluated against the wrong user, causing incorrect authorization decisions. Restrict this fallback to event types where sender is guaranteed to be the PR opener, or leave the author unresolved otherwise. [security]
Severity Level: Major ⚠️
- ❌ REVIEW_AUTHORS gate can evaluate against reviewer identity.
- ⚠️ pr-reviewer may run on disallowed PR authors.
- ⚠️ Some allowed authors' PRs may be incorrectly skipped.
- ⚠️ Slack notifications can attribute reviews to wrong user.Steps of Reproduction ✅
1. Configure the review agent with an author allowlist, e.g. set `REVIEW_AUTHORS=alice` as
described in `review/persona.ts:10-15` ("Only review and auto-fix PRs opened by these
GitHub logins").
2. Observe that the review agent is triggered on `pull_request_review_comment.created`
events via the `triggers.github` configuration in `review/agent.ts:60-68`, and that the
handler passes `event.payload` into `readPr()` at `review/agent.ts:23-25`.
3. In `readPr()` (`review/agent.ts:41-74`), note that the PR author is derived as `author:
p?.pull_request?.user?.login ?? (p?.pull_request ? p?.sender?.login : undefined) ??
'unknown'` (line 328 in the PR diff), and that comments earlier in the file
(`review/agent.ts:20-23`) explicitly state the webhook shape is an adapter projection
whose fields "may be absent", meaning `pull_request.user.login` can legitimately be
missing on some PR-shaped events.
4. Consider a `pull_request_review_comment.created` webhook where, due to adapter shape
drift or partial projection (acknowledged by the "fields may be absent" comment),
`p.pull_request` is present but `p.pull_request.user.login` is missing while
`p.sender.login` is set to the reviewer/commenter (not the PR opener). In this case, the
`author` field on the `Pr` object returned by `readPr()` will be set to the reviewer's
login via the fallback.
5. When the handler processes this event in the "Everything else is a reason to
(re)review" branch (`review/agent.ts:23-33`), it calls `shouldSkipReview(ctx, pr)`
(`review/agent.ts:42-80). That function computes `author = resolveAuthorLogin(meta, pr)`
(`review/agent.ts:36-38`), which falls back to `pr.author`, and then runs
`reviewAuthorAllowlistDecision(allow, author)` (`review/agent.ts:69-76`).
6. If `loadPrMeta(pr)` (`review/agent.ts:41-51`) fails to return an `author` (e.g.
meta.json missing or missing `author`, which is explicitly modeled as optional in `PrMeta`
at `review/agent.ts:24-31`), the allowlist decision will use the misidentified `author`
(the reviewer from `sender.login`) instead of the true PR opener. With `REVIEW_AUTHORS`
containing the reviewer but not the actual author, the gate incorrectly passes and
pr-reviewer runs on a PR that should have been skipped, or conversely may skip a PR if the
commenter is not in `REVIEW_AUTHORS` while the true author is.
7. This same misidentified `pr.author` will also be used for Slack notifications in
`review/agent.ts:15-23`, where `who` is rendered as the "PR opener" link using
`pr.author`, causing notifications to attribute the review to the reviewer/commenter
rather than the actual PR author when this fallback path is taken.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** review/agent.ts
**Line:** 328:328
**Comment:**
*Security: Falling back to `sender.login` for every payload that contains `pull_request` can misidentify the PR author on review/comment events, where `sender` is often the reviewer/commenter rather than the opener. If PR metadata is temporarily unavailable, the allowlist check can be evaluated against the wrong user, causing incorrect authorization decisions. Restrict this fallback to event types where `sender` is guaranteed to be the PR opener, or leave the author unresolved otherwise.
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
|
CodeAnt AI finished reviewing your PR. |
![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
🧹 Nitpick comments (1)
review/agent.ts (1)
314-328: ⚡ Quick winClarify/limit
sender.loginfallback, but the specific review/comment misclassification is unlikelyGitHub’s webhook payload docs for
pull_request_review.submittedandpull_request_review_comment.createdincludepull_request.user.login, soreadPr()should resolve the PR author fromp.pull_request.user.loginand not hit thesender.loginfallback for those events.shouldSkipReview()also prefers the live/pulls/{n}/meta.jsonauthor, further reducing risk.If you want stronger defense against payload/meta shape drift, keep the
sender.loginfallback only for the cases wherepull_request.user.loginis truly unavailable (e.g., PR-shaped payloads missing opener user), and add tests intests/review-agent.test.mjsthat explicitly assert the author for review/comment triggers comes frompull_request.user.login(notsender.login).🤖 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 `@review/agent.ts` around lines 314 - 328, The current author fallback uses sender.login which can misattribute PR author for review/comment events; in readPr() update the returned author field (and any places using it) to take p?.pull_request?.user?.login as the definitive source, only falling back to p?.sender?.login when a pull_request object exists but its user.login is missing (i.e., p?.pull_request && !p?.pull_request?.user?.login ? p?.sender?.login : undefined), and leave shouldSkipReview() unchanged; also add unit tests in tests/review-agent.test.mjs asserting that for pull_request_review.submitted and pull_request_review_comment.created payloads the author is p.pull_request.user.login and not sender.login.
🤖 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 `@review/.workforce/build/pr-reviewer/agent.bundle.mjs`:
- Around line 71-73: The path-escape check in readJsonFile is too broad because
using relative.startsWith("..") rejects names like "..foo"; update the check to
match the more precise logic used in readJsonFile2 (i.e., detect exactly the
parent-dir token or parent-dir followed by path separator). Replace the current
condition that uses relative.startsWith("..") with the equivalent of (relative
=== ".." || relative.startsWith(`..${path.sep}`)), keeping the existing
path.isAbsolute check and same error thrown (refer to the relayPath/relative
variables and the readJsonFile/readJsonFile2 functions to locate the code).
---
Nitpick comments:
In `@review/agent.ts`:
- Around line 314-328: The current author fallback uses sender.login which can
misattribute PR author for review/comment events; in readPr() update the
returned author field (and any places using it) to take
p?.pull_request?.user?.login as the definitive source, only falling back to
p?.sender?.login when a pull_request object exists but its user.login is missing
(i.e., p?.pull_request && !p?.pull_request?.user?.login ? p?.sender?.login :
undefined), and leave shouldSkipReview() unchanged; also add unit tests in
tests/review-agent.test.mjs asserting that for pull_request_review.submitted and
pull_request_review_comment.created payloads the author is
p.pull_request.user.login and not sender.login.
🪄 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 Plus
Run ID: 8e5196cc-4f99-4f04-a837-170a911a3e78
📒 Files selected for processing (3)
review/.workforce/build/pr-reviewer/agent.bundle.mjsreview/agent.tstests/review-agent.test.mjs
Comment on lines
+71
to
73
| if (relative.startsWith("..") || path.isAbsolute(relative)) { | ||
| throw new Error(`Relayfile path escapes mount root: ${relayPath}`); | ||
| } |
There was a problem hiding this comment.
Path escape detection is overly broad and inconsistent with the *2 variant.
relative.startsWith("..") incorrectly rejects valid filenames like ..foo or ...hidden that happen to start with two dots but are not traversal attempts. The *2 variant at line 119 correctly checks relative === ".." || relative.startsWith(\..${path2.sep}`)`.
This inconsistency means readJsonFile (non-*2) may reject valid paths that readJsonFile2 would accept.
🐛 Proposed fix
- if (relative.startsWith("..") || path.isAbsolute(relative)) {
+ if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (relative.startsWith("..") || path.isAbsolute(relative)) { | |
| throw new Error(`Relayfile path escapes mount root: ${relayPath}`); | |
| } | |
| if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { | |
| throw new Error(`Relayfile path escapes mount root: ${relayPath}`); | |
| } |
🤖 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 `@review/.workforce/build/pr-reviewer/agent.bundle.mjs` around lines 71 - 73,
The path-escape check in readJsonFile is too broad because using
relative.startsWith("..") rejects names like "..foo"; update the check to match
the more precise logic used in readJsonFile2 (i.e., detect exactly the
parent-dir token or parent-dir followed by path separator). Replace the current
condition that uses relative.startsWith("..") with the equivalent of (relative
=== ".." || relative.startsWith(`..${path.sep}`)), keeping the existing
path.isAbsolute check and same error thrown (refer to the relayPath/relative
variables and the readJsonFile/readJsonFile2 functions to locate the code).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="review/agent.ts">
<violation number="1" location="review/agent.ts:328">
P1: Author fallback to `sender.login` is too broad and can misclassify reviewer/commenter as PR author on review events, weakening REVIEW_AUTHORS enforcement.</violation>
</file>
<file name="review/.workforce/build/pr-reviewer/agent.bundle.mjs">
<violation number="1" location="review/.workforce/build/pr-reviewer/agent.bundle.mjs:71">
P2: Use an exact parent-directory traversal check here; `startsWith("..")` also blocks valid names like `..foo` that are not escaping the mount root.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| number, | ||
| url: prRef?.html_url ?? `https://github.com/${owner}/${repo}/pull/${number}`, | ||
| author: p?.pull_request?.user?.login ?? 'unknown', | ||
| author: p?.pull_request?.user?.login ?? (p?.pull_request ? p?.sender?.login : undefined) ?? 'unknown', |
There was a problem hiding this comment.
P1: Author fallback to sender.login is too broad and can misclassify reviewer/commenter as PR author on review events, weakening REVIEW_AUTHORS enforcement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At review/agent.ts, line 328:
<comment>Author fallback to `sender.login` is too broad and can misclassify reviewer/commenter as PR author on review events, weakening REVIEW_AUTHORS enforcement.</comment>
<file context>
@@ -324,7 +325,7 @@ export function readPr(payload: unknown): Pr | undefined {
number,
url: prRef?.html_url ?? `https://github.com/${owner}/${repo}/pull/${number}`,
- author: p?.pull_request?.user?.login ?? 'unknown',
+ author: p?.pull_request?.user?.login ?? (p?.pull_request ? p?.sender?.login : undefined) ?? 'unknown',
...(headSha ? { headSha } : {}),
...(p?.pull_request?.state ? { state: p.pull_request.state } : {}),
</file context>
Suggested change
| author: p?.pull_request?.user?.login ?? (p?.pull_request ? p?.sender?.login : undefined) ?? 'unknown', | |
| author: p?.pull_request?.user?.login ?? (p?.pull_request && !(p as { review?: unknown } | null)?.review && !(p as { comment?: unknown } | null)?.comment ? p?.sender?.login : undefined) ?? 'unknown', |
| const absolute = path.resolve(root, normalized); | ||
| const relative = path.relative(root, absolute); | ||
| if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { | ||
| if (relative.startsWith("..") || path.isAbsolute(relative)) { |
There was a problem hiding this comment.
P2: Use an exact parent-directory traversal check here; startsWith("..") also blocks valid names like ..foo that are not escaping the mount root.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At review/.workforce/build/pr-reviewer/agent.bundle.mjs, line 71:
<comment>Use an exact parent-directory traversal check here; `startsWith("..")` also blocks valid names like `..foo` that are not escaping the mount root.</comment>
<file context>
@@ -40,7 +68,7 @@ function toAbsolutePath(client, relayPath) {
const absolute = path.resolve(root, normalized);
const relative = path.relative(root, absolute);
- if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) {
+ if (relative.startsWith("..") || path.isAbsolute(relative)) {
throw new Error(`Relayfile path escapes mount root: ${relayPath}`);
}
</file context>
Suggested change
| if (relative.startsWith("..") || path.isAbsolute(relative)) { | |
| if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { |
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.
User description
Restore pr-reviewer firing for allowlisted PR authors by falling back to sender.login when pull_request.user.login has not materialized yet. This keeps the REVIEW_AUTHORS gate fail-closed for resolved authors, marks unresolved skips as notify-worthy, and leaves the Slack-notify policy change for a separate PR.
Summary by cubic
Fixes PR author resolution in the reviewer agent by falling back to
sender.loginwhenpull_request.user.loginis missing, ensuring allowlisted authors still trigger reviews. Unresolved-author skips now setnotify: trueto flag attention.payload.sender.loginas a fallback author for PR-shaped events.REVIEW_AUTHORSis set and author is unresolved, return a skip withnotify: true.Written for commit a740e56. Summary will update on new commits.
CodeAnt-AI Description
Resolve PR author from the webhook sender when opener info is missing
What Changed
Impact
✅ Fewer missed reviews for allowed authors✅ Clearer follow-up on unresolved PR authors✅ More reliable PR review routing💡 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.