Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
142 changes: 140 additions & 2 deletions review/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
*/
import {
defineAgent,
encodeSegment,
readJsonFile,
resolveMountRoot,
type IntegrationClientOptions,
type WorkforceCtx
} from '@agentworkforce/runtime';
import { githubClient, slackClient } from '@relayfile/relay-helpers';
Expand All @@ -23,6 +27,30 @@ interface Pr {
url: string;
author: string; // github login of whoever opened the PR
headSha?: string;
state?: string;
merged?: boolean;
labels?: unknown;
}

/** The materialized PR record at `…/pulls/{n}/meta.json`. Read for the
* authoritative author/labels/state — the webhook payload doesn't carry them
* on every trigger (check_run.completed has neither). Read defensively: the
* shape is the github adapter's projection and fields may be absent. */
interface PrMeta {
state?: string; // 'open' | 'closed'
merged?: boolean;
// The materialized meta.json has carried `author` both as a bare login
// string and as an object — accept either so the allowlist isn't silently
// bypassed by a shape mismatch.
author?: string | { login?: string };
labels?: unknown; // validated as Array<{ name?: string }> at read time
[key: string]: unknown;
}
Comment on lines +39 to 48

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: The PR metadata author shape is modeled incorrectly, so the authoritative author from meta.json is never read. In this codebase the metadata author is a string login (as used by repo-hygiene), but here it is typed as an object with login; that makes meta?.author?.login resolve to undefined and forces fallback to payload-derived author values, which can be wrong on check_run.completed. Update the meta contract to match the real schema and read the login from that field directly. [api mismatch]

Severity Level: Major ⚠️
- ❌ CI-triggered PR reviews skipped for allowlisted human authors.
- ⚠️ REVIEW_AUTHORS ignores authoritative author from PR metadata.
Steps of Reproduction ✅
1. Configure the pr-reviewer to use an author allowlist by setting `REVIEW_AUTHORS`
(consumed via `input()` at `review/agent.ts:52-55` and used in `reviewAuthorAllowlist()`
at `review/agent.ts:155-159`).

2. Note that `shouldSkipReview()` at `review/agent.ts:101-132` reads PR metadata via
`loadPrMeta()` (`review/agent.ts:135-146`) and expects `PrMeta.author` to be an object
with `login` (`review/agent.ts:36-42`) while the same `meta.json` endpoint is typed as
`author?: string` in `GithubPrMeta` at `repo-hygiene/agent.ts:28-35`.

3. Open a PR in a repo where CI emits `check_run.completed` events; when such an event
arrives, the handler in `review/agent.ts:50-92` calls `readPr()`
(`review/agent.ts:262-282`), which sets `pr.author` from the webhook payload
(`p?.pull_request?.user?.login ?? p?.sender?.login ?? 'unknown'`).

4. For `check_run.completed` payloads, `p.pull_request` is often absent and
`p.sender.login` is typically the app (for example, `github-actions[bot]`), so `pr.author`
is not the human PR opener, while the `meta.json` `author` field (typed as `string` in
`GithubPrMeta` at `repo-hygiene/agent.ts:28-35`) contains the correct opener login.

5. When `shouldSkipReview()` runs for that event, `loadPrMeta()` successfully returns a
`PrMeta` whose runtime `author` value is a string, making `meta?.author?.login`
(`review/agent.ts:126`) evaluate to `undefined`; the expression `(meta?.author?.login ??
pr.author)` therefore falls back to `pr.author` (the app login), causing the allowlist
check `!allow.has(author)` (`review/agent.ts:127`) to skip the review even though the
correct human author in `meta.json` would have passed `REVIEW_AUTHORS`.

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:** 36:42
**Comment:**
	*Api Mismatch: The PR metadata author shape is modeled incorrectly, so the authoritative author from `meta.json` is never read. In this codebase the metadata author is a string login (as used by `repo-hygiene`), but here it is typed as an object with `login`; that makes `meta?.author?.login` resolve to `undefined` and forces fallback to payload-derived author values, which can be wrong on `check_run.completed`. Update the meta contract to match the real schema and read the login from that field directly.

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


const DEFAULT_SKIP_LABEL = 'no-agent-relay-review';

function vfsClient(): IntegrationClientOptions {
return { relayfileMountRoot: resolveMountRoot({}) };
}

export default defineAgent({
Expand Down Expand Up @@ -54,6 +82,12 @@ export default defineAgent({
// Everything else is a reason to (re)review and push fixes.
const pr = readPr(event.payload);
if (pr) {
const skip = await shouldSkipReview(ctx, pr);
if (skip) {
ctx.log?.('info', 'pr-reviewer skipped', { owner: pr.owner, repo: pr.repo, number: pr.number, reason: skip.reason });
if (skip.notify) await notifySkip(ctx, pr, skip.reason);
return;
}
await reviewAndFix(ctx, pr);
} else if (event.type === 'check_run.completed') {
// GitHub sometimes emits check_run.completed with pull_requests: [] for
Expand All @@ -64,6 +98,99 @@ export default defineAgent({
}
});

// ── review gate ─────────────────────────────────────────────────────────────
// Decide whether to (re)review/fix this PR at all. Returns a skip reason, or
// null to proceed. Three gates, in order: already-merged, a disabling label,
// and an author allowlist. Prefer the live PR meta.json, but fall back to
// fields that are present on pull_request webhook payloads; check_run.completed
// payloads do not carry enough detail, so those fail open when meta is missing.
async function shouldSkipReview(ctx: WorkforceCtx, pr: Pr): Promise<{ reason: string; notify?: boolean } | null> {
const meta = await loadPrMeta(pr);

// Already merged/closed by the time we got here — don't post a stale review
// on a finished PR. This is the cheap, agent-side half of the merge-race;
// preserving the unpushed fixes via a recovery PR needs the cloud-side work
// tracked in AgentWorkforce/cloud#1659 / #1660.
const state = (meta?.state ?? pr.state ?? '').trim().toLowerCase();
if (meta?.merged === true || pr.merged === true || state === 'closed') {
return { reason: 'PR is already merged/closed', notify: true };
}

// A disabling label turns the reviewer off entirely for this PR. `labels` is
// validated here (not just type-asserted) since meta.json shape can drift.
const skipLabels = skipLabelSet(ctx);
const prLabels = labelNames(Array.isArray(meta?.labels) ? meta.labels : pr.labels);
const hit = prLabels.find((name) => skipLabels.has(name));
if (hit) {
return { reason: `PR carries the "${hit}" label` };
}

// Author allowlist: when REVIEW_AUTHORS is set, only review/fix PRs opened by
// those logins (e.g. "only my own PRs"). Unset → review every author.
// Fail open: if we couldn't determine a confident author (meta read failed
// and the payload had none), don't block — the gate only excludes a known
// author that isn't allowed.
const allow = reviewAuthorAllowlist(ctx);
if (allow.size > 0) {
const author = resolveAuthorLogin(meta, pr);
if (author && author !== 'unknown' && !allow.has(author)) {
return { reason: `author @${author} is not in REVIEW_AUTHORS` };
}
Comment on lines +133 to +138

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: The author allowlist gate is not actually fail-open when metadata loading fails: it still enforces REVIEW_AUTHORS against fallback payload author values (often sender/unknown for non-PR payload shapes), which can incorrectly skip eligible PRs during transient meta read failures. Only enforce this gate when a reliable PR opener is available from metadata, or explicitly bypass the gate when metadata is unavailable. [logic error]

Severity Level: Major ⚠️
- ❌ Transient meta read failures can block allowed PR reviews.
- ⚠️ REVIEW_AUTHORS behaves fail-closed on metadata errors.
Steps of Reproduction ✅
1. Enable an author allowlist for pr-reviewer by setting `REVIEW_AUTHORS` so that
`reviewAuthorAllowlist()` at `review/agent.ts:155-159` returns a non-empty `Set`,
activating the gate in `shouldSkipReview()` at `review/agent.ts:124-130`.

2. Observe that `loadPrMeta()` at `review/agent.ts:135-146` reads `meta.json` via
`readJsonFile<PrMeta>()` and catches all errors, returning `undefined` on any failure
(e.g., transient VFS or adapter issues).

3. When a GitHub event (for example, `check_run.completed` listed in `triggers` at
`review/agent.ts:55-60`) arrives during such a transient failure, `loadPrMeta()` returns
`undefined`, so `meta` is falsy in `shouldSkipReview()` (`review/agent.ts:101-132`): the
merged/closed and label gates effectively no-op, but the allowlist gate still executes.

4. In this meta-failure case, the allowlist gate computes `author` as
`(meta?.author?.login ?? pr.author).trim().toLowerCase()` at `review/agent.ts:126`, which
reduces to `pr.author` because `meta` is `undefined`; `pr.author` comes from `readPr()`
(`review/agent.ts:262-282`) and, for `check_run.completed`, is often the app or `sender`
login (e.g., `github-actions[bot]`) or `'unknown'`, not the actual PR opener.

5. Because `REVIEW_AUTHORS` is defined in terms of human opener logins, the
`allow.has(author)` check at `review/agent.ts:127` fails when `author` is this fallback
value, causing `shouldSkipReview()` to return a skip reason and the handler at
`review/agent.ts:79-85` to skip `reviewAndFix()` despite the PR having an allowlisted
opener; this violates the intended best-effort/fail-open behavior when metadata loading is
unreliable.

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:** 124:129
**Comment:**
	*Logic Error: The author allowlist gate is not actually fail-open when metadata loading fails: it still enforces `REVIEW_AUTHORS` against fallback payload author values (often `sender`/`unknown` for non-PR payload shapes), which can incorrectly skip eligible PRs during transient meta read failures. Only enforce this gate when a reliable PR opener is available from metadata, or explicitly bypass the gate when metadata is unavailable.

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

}

return null;
}

/** Lowercased PR author login, preferring the authoritative meta.json (string
* or `{ login }`) and falling back to the webhook payload. Returns '' when no
* login can be determined. */
function resolveAuthorLogin(meta: PrMeta | undefined, pr: Pr): string {
const fromMeta = typeof meta?.author === 'string' ? meta.author : meta?.author?.login;
return (fromMeta ?? pr.author ?? '').trim().toLowerCase();
}

async function loadPrMeta(pr: Pr): Promise<PrMeta | undefined> {
try {
return await readJsonFile<PrMeta>(
vfsClient(),
'github',
'getPr',
`/github/repos/${encodeSegment(pr.owner)}/${encodeSegment(pr.repo)}/pulls/${pr.number}/meta.json`
);
} catch {
return undefined;
}
}

/** Lowercased label names that disable the reviewer. Defaults to
* "no-agent-relay-review" when SKIP_LABELS is unset. */
function skipLabelSet(ctx: WorkforceCtx): Set<string> {
const raw = input(ctx, 'SKIP_LABELS') ?? DEFAULT_SKIP_LABEL;
return new Set(raw.split(',').map((s) => s.trim().toLowerCase()).filter(Boolean));
}

/** Lowercased github logins allowed to be reviewed/fixed. Empty = everyone. */
function reviewAuthorAllowlist(ctx: WorkforceCtx): Set<string> {
const raw = input(ctx, 'REVIEW_AUTHORS') ?? '';
return new Set(raw.split(',').map((s) => s.trim().toLowerCase()).filter(Boolean));
}

function labelNames(labels: unknown): string[] {
if (!Array.isArray(labels)) return [];
return labels
.map((l) => (l && typeof (l as { name?: unknown }).name === 'string' ? (l as { name: string }).name.trim().toLowerCase() : ''))
.filter(Boolean);
}

async function notifySkip(ctx: WorkforceCtx, pr: Pr, reason: string): Promise<void> {
const channel = input(ctx, 'SLACK_CHANNEL');
if (!channel) return;
await slackClient().post(
channel,
`:information_source: pr-reviewer skipped PR #${pr.number} in *${pr.owner}/${pr.repo}* — ${reason}: ${pr.url}`
);
}

async function reviewAndFix(ctx: WorkforceCtx, pr: Pr): Promise<void> {
const run = await ctx.harness.run({
cwd: ctx.sandbox.cwd,
Expand Down Expand Up @@ -159,7 +286,15 @@ async function mergePr(ctx: WorkforceCtx, pr: Pr): Promise<void> {
function readPr(payload: unknown): Pr | undefined {
const p = payload as {
number?: number;
pull_request?: { number?: number; html_url?: string; user?: { login?: string }; head?: { sha?: string } };
pull_request?: {
number?: number;
html_url?: string;
user?: { login?: string };
head?: { sha?: string };
state?: string;
merged?: boolean;
labels?: unknown;
};
check_run?: { pull_requests?: Array<{ number?: number; html_url?: string; head_sha?: string }> };
repository?: { name?: string; owner?: { login?: string } };
sender?: { login?: string };
Expand All @@ -177,7 +312,10 @@ function readPr(payload: unknown): Pr | undefined {
number,
url: prRef?.html_url ?? `https://github.com/${owner}/${repo}/pull/${number}`,
author: p?.pull_request?.user?.login ?? p?.sender?.login ?? 'unknown',
...(headSha ? { headSha } : {})
...(headSha ? { headSha } : {}),
...(p?.pull_request?.state ? { state: p.pull_request.state } : {}),
...(typeof p?.pull_request?.merged === 'boolean' ? { merged: p.pull_request.merged } : {}),
...(p?.pull_request?.labels !== undefined ? { labels: p.pull_request.labels } : {})
};
}
function isApproval(payload: unknown): boolean {
Expand Down
11 changes: 11 additions & 0 deletions review/persona.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ export default definePersona({
env: 'APPROVERS',
optional: true,
picker: { provider: 'github', resource: 'users' }
},
REVIEW_AUTHORS: {
description: 'Only review and auto-fix PRs opened by these GitHub logins (comma-separated). If unset, every author is reviewed.',
env: 'REVIEW_AUTHORS',
optional: true,
picker: { provider: 'github', resource: 'users' }
},
SKIP_LABELS: {
description: 'PR labels that disable the reviewer entirely (comma-separated). Defaults to "no-agent-relay-review".',
env: 'SKIP_LABELS',
optional: true
}
},

Expand Down