Skip to content

fix(pr-reviewer): resolve PR author from sender login #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khaliqgant merged 1 commit into from
Jun 3, 2026
+146 −66
Merged

fix(pr-reviewer): resolve PR author from sender login #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 128 additions & 61 deletions review/.workforce/build/pr-reviewer/agent.bundle.mjs
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions review/agent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,12 @@ function reviewAuthorAllowlist(ctx: WorkforceCtx): Set<string> {
export function reviewAuthorAllowlistDecision(
allow: Set<string>,
author: string
): { reason: string } | null {
): { reason: string; notify?: boolean } | null {
if (allow.size === 0) {
return null;
}
if (!author || author === 'unknown') {
return { reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved' };
return { reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved', notify: true };
}
if (!allow.has(author)) {
return { reason: `author @${author} is not in REVIEW_AUTHORS` };
Expand Down Expand Up @@ -311,6 +311,7 @@ export function readPr(payload: unknown): Pr | undefined {
};
check_run?: { pull_requests?: Array<{ number?: number; html_url?: string; head_sha?: string }> };
repository?: { name?: string; owner?: { login?: string } };
sender?: { login?: string };
} | null;
const prRef = p?.pull_request ?? p?.check_run?.pull_requests?.[0];
const number = prRef?.number ?? p?.number;
Expand All @@ -324,7 +325,7 @@ export function readPr(payload: unknown): Pr | undefined {
repo,
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',

@codeant-ai codeant-ai Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
👍 | 👎

@cubic-dev-ai cubic-dev-ai Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',

...(headSha ? { headSha } : {}),
...(p?.pull_request?.state ? { state: p.pull_request.state } : {}),
...(typeof p?.pull_request?.merged === 'boolean' ? { merged: p.pull_request.merged } : {}),
Expand Down
16 changes: 14 additions & 2 deletions tests/review-agent.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ test('reviewAuthorAllowlistDecision skips authors not in the allowlist', () => {
test('reviewAuthorAllowlistDecision skips unresolved authors when configured', () => {
assert.deepEqual(
reviewAuthorAllowlistDecision(new Set(['khaliqgant']), ''),
{ reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved' },
{ reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved', notify: true },
);
assert.deepEqual(
reviewAuthorAllowlistDecision(new Set(['khaliqgant']), 'unknown'),
{ reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved' },
{ reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved', notify: true },
);
});

Expand Down Expand Up @@ -76,6 +76,18 @@ test('readPr uses the pull request opener as author when present', () => {
})?.author, 'WillWashburn');
});

test('readPr falls back to sender login for PR-shaped payloads when opener login is missing', () => {
assert.equal(readPr({
number: 27,
pull_request: {
number: 27,
html_url: 'https://github.com/AgentWorkforce/agents/pull/27',
},
repository: { name: 'agents', owner: { login: 'AgentWorkforce' } },
sender: { login: 'KhaliqGant' },
})?.author, 'KhaliqGant');
});

test('labelNames normalizes github label arrays defensively', () => {
assert.deepEqual(labelNames([
{ name: ' No-Agent-Relay-Review ' },
Expand Down