feat(pr-reviewer): fail closed on unresolved authors#27
Conversation
|
CodeAnt AI is reviewing your PR. |
📝 WalkthroughWalkthroughThis PR adds author-based filtering to the PR reviewer agent via a fail-closed allowlist decision, introduces a test runner to validate the logic, bundles the agent with explicit trigger configuration and client-based messaging, and updates the persona and runner to support the new agent specification. ChangesPR Reviewer Agent Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PR reviewer agent's author allowlist logic to fail closed when configured but the author cannot be resolved confidently. This logic is extracted into a new exported function reviewAuthorAllowlistDecision and unit-tested in tests/review-agent.test.mjs. Additionally, a custom test runner script scripts/test.mjs has been introduced to build TypeScript sources with esbuild and run Node.js tests, and a new test:review script has been added to package.json. The agent's persona configuration and runner have also been updated to support the new defineAgent structure and additional inputs (REVIEW_AUTHORS, SKIP_LABELS). There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| import { build } from 'esbuild'; |
There was a problem hiding this comment.
Suggestion: This script imports esbuild directly but esbuild is not declared in this package's direct dependencies/devDependencies. The test runner can fail with ERR_MODULE_NOT_FOUND in clean or non-hoisting installs (or after dependency graph changes), so add esbuild as an explicit devDependency to make this command reliable. [import error]
Severity Level: Major ⚠️
- ❌ `npm run test:review` can fail with module not found.
- ⚠️ Focused reviewer tests become unreliable across install environments.
- ⚠️ CI pipelines depending on `test:review` may intermittently break.Steps of Reproduction ✅
1. Install this package in isolation using only its declared dependencies from
`package.json` at `/workspace/agents/package.json:14-23`, which do NOT list `esbuild`
under `dependencies` or `devDependencies`.
2. Run `npm run test:review` (script defined in `package.json:8-12` as `node
scripts/test.mjs tests/review-agent.test.mjs`), which executes the test runner at
`/workspace/agents/scripts/test.mjs:1-94`.
3. Node evaluates `scripts/test.mjs` and reaches the import `import { build } from
'esbuild';` at `scripts/test.mjs:6`, requiring that `esbuild` be resolvable from the root
`node_modules` based on this package's own dependency graph.
4. In an environment where `esbuild` is not hoisted to the root (e.g., non-hoisting
installs, or after `agentworkforce` stops depending on `@agentworkforce/deploy` which
currently pulls in `esbuild` transitively as shown in `package-lock.json:8-19`), the
module `esbuild` is absent from this package's direct dependency tree, causing
`ERR_MODULE_NOT_FOUND` on startup and making `npm run test:review` fail.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:** scripts/test.mjs
**Line:** 6:6
**Comment:**
*Import Error: This script imports `esbuild` directly but `esbuild` is not declared in this package's direct dependencies/devDependencies. The test runner can fail with `ERR_MODULE_NOT_FOUND` in clean or non-hoisting installs (or after dependency graph changes), so add `esbuild` as an explicit devDependency to make this command reliable.
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 (!author || author === 'unknown') { | ||
| return { reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved' }; | ||
| } | ||
| if (!allow.has(author)) { | ||
| return { reason: `author @${author} is not in REVIEW_AUTHORS` }; | ||
| } |
There was a problem hiding this comment.
Suggestion: The allowlist decision treats any non-empty author as confidently resolved, but in check_run.completed flows author can come from sender.login (often a bot) when PR meta lookup fails, which is not the PR opener. That causes valid allowlisted PRs to be skipped as "not in REVIEW_AUTHORS" instead of being classified as unresolved. Update the decision/input so non-authoritative fallback identities are treated as unresolved when REVIEW_AUTHORS is set. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Allowlisted PRs skipped on failing check_run.completed events.
- ⚠️ Reviews silently omitted despite author being explicitly allowlisted.Steps of Reproduction ✅
1. Configure the pr-reviewer persona with `REVIEW_AUTHORS` containing the real PR opener
but not the CI bot, e.g. `REVIEW_AUTHORS=alice` and no entry for `github-actions`
(allowlist is parsed in `review/agent.ts:112-115` by `reviewAuthorAllowlist`).
2. Open a pull request authored by `alice` in a repo where this agent is installed so that
the PR is materialized and later receives CI check runs (PR parsing happens via `readPr`
in `review/agent.ts:2-35` which sets `author`).
3. Trigger a failing CI check that emits a `check_run.completed` webhook with a
non-success conclusion (so `ciFailed` in `review/agent.ts:49-51` returns true) where the
payload has `check_run.pull_requests[0]` populated but no `pull_request.user`, and the VFS
`meta.json` either fails to load or does not include an `author` field (so `loadPrMeta` in
`review/agent.ts:91-101` returns `undefined` or a meta without `author`).
4. When this `check_run.completed` event is delivered, the handler in
`review/agent.ts:10-39` calls `readPr`, which sets `pr.author` to
`p?.pull_request?.user?.login ?? p?.sender?.login ?? 'unknown'` (see
`review/agent.ts:25-31`); for `check_run.completed` without `pull_request.user`, this
becomes the CI bot login from `sender.login`. `shouldSkipReview` in
`review/agent.ts:48-80` then calls `resolveAuthorLogin(meta, pr)` (lines 86-88), which
falls back to `pr.author` because `meta?.author` is missing, and passes that bot login
into `reviewAuthorAllowlistDecision` at `review/agent.ts:117-121`. Since `allow.size > 0`
and `author` is a non-empty, non-`'unknown'` bot login, the code path at
`review/agent.ts:5-9` returns `{ reason: 'author @github-actions is not in REVIEW_AUTHORS'
}` instead of treating the author as unresolved. `shouldSkipReview` returns this skip
reason, so the handler logs `pr-reviewer skipped` and does not call `reviewAndFix`, even
though the real PR opener (`alice`) is allowlisted. This matches the reported issue that
non-authoritative fallback identities (e.g. CI bot `sender.login` in
`check_run.completed`) are treated as real authors, causing allowlisted PRs to be skipped
rather than classified as unresolved.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:** 183:188
**Comment:**
*Incorrect Condition Logic: The allowlist decision treats any non-empty `author` as confidently resolved, but in `check_run.completed` flows `author` can come from `sender.login` (often a bot) when PR meta lookup fails, which is not the PR opener. That causes valid allowlisted PRs to be skipped as "not in REVIEW_AUTHORS" instead of being classified as unresolved. Update the decision/input so non-authoritative fallback identities are treated as unresolved when `REVIEW_AUTHORS` is set.
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: 3
🤖 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 1000-1003: The resolveAuthorLogin function currently falls back to
pr.author which can be the check-run sender; change it to only use pr.author
when the payload actually represents a pull_request (e.g., when
pr.pull_request?.user?.login exists) so stale/missing meta.json won't treat a
check_run sender as the PR opener—update resolveAuthorLogin to derive from
meta.author first, and only if absent and pr.pull_request?.user?.login is
present use pr.author (otherwise leave unresolved/empty) so REVIEW_AUTHORS fails
closed for non-pull_request payloads.
In `@review/agent.ts`:
- Around line 128-137: The current logic claims to "fail open" when meta.json is
missing but resolveAuthorLogin falls back to 'unknown', causing
reviewAuthorAllowlistDecision to treat it as a non-author and skip; fix by
making the author resolution explicitly indicate missing meta (e.g., return
null/undefined instead of 'unknown') or update reviewAuthorAllowlistDecision to
treat a missing-meta sentinel as "fail open" (do not skip and do not notify),
and update the gate comment accordingly; specifically adjust resolveAuthorLogin
(used in review/agent.ts) and/or reviewAuthorAllowlistDecision to detect meta
absence from readPr and avoid silent skips for unknown author when meta.json
wasn't loaded.
In `@scripts/test.mjs`:
- Around line 6-30: The script scripts/test.mjs imports esbuild via "import {
build } from 'esbuild'" but esbuild is not declared in package.json; add
"esbuild" to package.json devDependencies (exact version matching
package-lock.json or a suitable pinned/dev version) and run npm install to
update lockfile so clean/pruned installs and npm run test:review won't fail.
🪄 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: 3aeb990b-40b3-4b7a-9f89-f7f100537b3a
📒 Files selected for processing (7)
package.jsonreview/.workforce/build/pr-reviewer/agent.bundle.mjsreview/.workforce/build/pr-reviewer/persona.jsonreview/.workforce/build/pr-reviewer/runner.mjsreview/agent.tsscripts/test.mjstests/review-agent.test.mjs
| // 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. | ||
| // Fail closed when configured: if the author can't be resolved confidently, | ||
| // skip instead of risking a review on the wrong PR author. | ||
| 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` }; | ||
| } | ||
| const author = resolveAuthorLogin(meta, pr); | ||
| const allowlistSkip = reviewAuthorAllowlistDecision(allow, author); | ||
| if (allowlistSkip) { | ||
| return allowlistSkip; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where pulls/{n}/meta.json is written/materialized and whether it precedes check_run.completed handling.
rg -nP 'meta\.json' -C3
rg -nP 'check_run\.completed|check_run' -C3 --type=tsRepository: AgentWorkforce/agents
Length of output: 8617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="review/agent.ts"
# Show implementations + nearby constants/comments
rg -n "function (resolveAuthorLogin|reviewAuthorAllowlistDecision|loadPrMeta)\b" "$FILE"
rg -n "'unknown'|unknown" "$FILE"
rg -n "REVIEW_AUTHORS" "$FILE"
rg -n "allowlist" "$FILE"
# Print focused line ranges (adjust if rg shows different line numbers)
python3 - <<'PY'
import re,sys,subprocess,os,math,json
FILE="review/agent.ts"
# parse rg output to get line numbers for key functions
keys=["resolveAuthorLogin","reviewAuthorAllowlistDecision","loadPrMeta","readPr"]
for k in keys:
out=subprocess.check_output(["rg","-n",f"function {k}\\b",FILE],text=True).strip().splitlines()
if not out:
continue
ln=int(out[0].split(":")[0])
lo=max(1,ln-20); hi=ln+60
print(f"\n### {k} around lines {lo}-{hi}")
subprocess.run(["sed","-n",f"{lo},{hi}p",FILE],check=False)
PYRepository: AgentWorkforce/agents
Length of output: 15197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="review/agent.ts"
# Show full shouldSkipReview body and caller usage
rg -n "function shouldSkipReview|shouldSkipReview\\(" "$FILE"
echo "---- shouldSkipReview (80-170) ----"
sed -n '90,180p' "$FILE"
echo "---- caller handling skip result (170-260?) ----"
rg -n "notifySkip\\(|notify\\b|skip\\b.*notify" "$FILE"
sed -n '60,120p' "$FILE"Repository: AgentWorkforce/agents
Length of output: 7485
Fix author allowlist “fail open” comment / behavior when meta.json is missing (esp. check_run.completed).
shouldSkipReview applies REVIEW_AUTHORS fail closed: if meta.json can’t be loaded, resolveAuthorLogin(meta, pr) falls back to readPr’s author (pull_request.user.login or sender.login or 'unknown'), and reviewAuthorAllowlistDecision skips when author === 'unknown' or not in the allowlist (silent: no notify). That can suppress re-reviews on check_run.completed when the payload doesn’t identify the real PR author.
The gate comment at review/agent.ts lines 104-106 (“those fail open when meta is missing”) is inaccurate for the author dimension; update the comment or adjust the fallback/decision to truly fail open when meta.json is unavailable.
🤖 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 128 - 137, The current logic claims to "fail
open" when meta.json is missing but resolveAuthorLogin falls back to 'unknown',
causing reviewAuthorAllowlistDecision to treat it as a non-author and skip; fix
by making the author resolution explicitly indicate missing meta (e.g., return
null/undefined instead of 'unknown') or update reviewAuthorAllowlistDecision to
treat a missing-meta sentinel as "fail open" (do not skip and do not notify),
and update the gate comment accordingly; specifically adjust resolveAuthorLogin
(used in review/agent.ts) and/or reviewAuthorAllowlistDecision to detect meta
absence from readPr and avoid silent skips for unknown author when meta.json
wasn't loaded.
|
Reviewed PR #27 locally and made one focused hardening change: exported and tested the pure author/label normalization helpers used by the new review skip gate, so meta author shapes and defensive label parsing are now covered. Regenerated the checked-in Local verification passed:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 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="scripts/test.mjs">
<violation number="1" location="scripts/test.mjs:6">
P2: New test runner imports `esbuild` but the package is not declared in dependencies/devDependencies, making test execution environment-dependent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| import { build } from 'esbuild'; |
There was a problem hiding this comment.
P2: New test runner imports esbuild but the package is not declared in dependencies/devDependencies, making test execution environment-dependent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/test.mjs, line 6:
<comment>New test runner imports `esbuild` but the package is not declared in dependencies/devDependencies, making test execution environment-dependent.</comment>
<file context>
@@ -0,0 +1,94 @@
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+import { build } from 'esbuild';
+
+const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
</file context>
|
Reviewed PR #27 and fixed the issues I found. Changed:
Validated locally:
|
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #27 and fixed the issues I found. Changed:
Validated locally:
|
1 participant
User description
Summary\n- Fail closed when REVIEW_AUTHORS is configured but the PR author cannot be resolved.\n- Keep unset allowlists open, and keep the non-member skip branch intact.\n- Restage the pr-reviewer bundle, runner, and persona artifacts so deployment uses the updated behavior.\n\n## Validation\n- npm run test:review\n- 4/4 focused reviewer tests passing\n\n## Notes\n- Bundle drift is confined to the restaged reviewer patch set and generated deployment artifacts.\n- Repo-wide npm test is still blocked by the separate linear-tsc lane, which is intentionally out of scope here.
Summary by cubic
Fail closed when
REVIEW_AUTHORSis set but the PR author can’t be resolved, preventing unintended reviews. Also updates the persona/runner and adds a smallesbuild-based test harness.New Features
REVIEW_AUTHORS; if unset, review all authors.REVIEW_AUTHORSandSKIP_LABELS(defaults to "no-agent-relay-review"), makeSLACK_CHANNELoptional, declaregithub/slackintegrations, setcloud: true.defineAgentand forwards triggers/schedules/watch.Refactors
pr-reviewerbundle and deployment artifacts.scripts/test.mjs,npm run test:review, focused allowlist tests, and dev dependency onesbuildfor building test targets.Written for commit 19491bc. Summary will update on new commits.
CodeAnt-AI Description
Skip reviews when the PR author cannot be confirmed
What Changed
REVIEW_AUTHORSis set and the PR author cannot be resolved, the reviewer now skips the PR instead of proceeding.REVIEW_AUTHORSare skipped with a clear reason.REVIEW_AUTHORSin its settings, makes the Slack channel optional, and keeps its GitHub/Slack connections declared for deployment.Impact
✅ Fewer unintended PR reviews✅ Clearer skip reasons for author-based review rules✅ Safer review gating when author data is missing💡 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.