feat(skills/merge-gate): add PR Review Advisor as a hard gate in check-gates.ts#5597
feat(skills/merge-gate): add PR Review Advisor as a hard gate in check-gates.ts#5597prekshivyas wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughA new "Gate 4: PR Review Advisor not blocked" is added to the merge gate system. ChangesPR Review Advisor Merge Gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1).agents/skills/nemoclaw-maintainer-day/MERGE-GATE.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Comment |
f2e3ae2 to
390f488
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md:
- Line 32: The documentation in MERGE-GATE.md at line 32 references a link that
should be available in gates.prAdvisor.details, but the check-gates.ts script
does not actually emit any URL in that field. Either update the check-gates.ts
script to include the actual comment URL when populating gates.prAdvisor.details
output, or remove the "(linked in `gates.prAdvisor.details`)" phrase from the
MERGE-GATE.md documentation to match the current implementation. Ensure that
what the documentation promises matches what the code actually delivers.
In @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 275-283: The praComments filter on line 275-283 only checks if the
comment body contains the text "nemoclaw-pr-review-advisor" without validating
that the comment was actually posted by the PR Review Advisor bot account
itself. This allows any user to spoof a bot recommendation by posting a comment
with that marker text. Modify the filter condition to also check that the
comment author is the actual PR Review Advisor bot account (by verifying the
user or author property of the comment object), ensuring that only authentic bot
comments are accepted for gate validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc3aad8e-0754-4671-9905-1bb84c922129
📒 Files selected for processing (2)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
| const praComments = allComments.filter((c) => | ||
| (c.body ?? "").includes("nemoclaw-pr-review-advisor"), | ||
| ); | ||
| if (praComments.length === 0) { | ||
| return { pass: true, details: "No PR Review Advisor comment found" }; | ||
| } | ||
|
|
||
| // Use the last PRA comment (most recent re-run) | ||
| const body = praComments[praComments.length - 1].body ?? ""; |
There was a problem hiding this comment.
Gate is spoofable because PRA detection trusts marker text without author validation.
Line 275-283 accepts any latest comment containing nemoclaw-pr-review-advisor. A non-bot commenter can post that marker with recommendation: approved and override a prior blocked bot comment.
Suggested fix
- let allComments: Array<{ body?: string }>;
+ let allComments: Array<{ body?: string; user?: { login?: string; type?: string } }>;
@@
- const praComments = allComments.filter((c) =>
- (c.body ?? "").includes("nemoclaw-pr-review-advisor"),
- );
+ const PRA_BOT_LOGIN = "nemoclaw-pr-review-advisor[bot]";
+ const praComments = allComments.filter((c) =>
+ (c.body ?? "").includes("nemoclaw-pr-review-advisor") &&
+ c.user?.login === PRA_BOT_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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts around lines
275 - 283, The praComments filter on line 275-283 only checks if the comment
body contains the text "nemoclaw-pr-review-advisor" without validating that the
comment was actually posted by the PR Review Advisor bot account itself. This
allows any user to spoof a bot recommendation by posting a comment with that
marker text. Modify the filter condition to also check that the comment author
is the actual PR Review Advisor bot account (by verifying the user or author
property of the comment object), ensuring that only authentic bot comments are
accepted for gate validation.
…k-gates.ts Previously the gate checker only checked CI, conflicts, CodeRabbit, and risky-code coverage. The PR Review Advisor status was documented as a manual review step, which allowed advisor-blocked PRs to slip through. Add a fifth gate that fetches the PRA sticky comment, parses the `recommendation:` metadata from its embedded HTML comment, and fails `allPass` when `recommendation: blocked`. Update MERGE-GATE.md to reflect that the gate is now automated. Signed-off-by: Preksha Vyas <prekshiv@nvidia.com>
390f488 to
8e012dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 286-295: After parsing the JSON in the try block, add a type check
to verify that allComments is actually an array before calling the filter
method. If the parsed JSON is not an array (such as when GitHub returns an error
object), return the fail-closed response with an appropriate error message
instead of allowing the code to proceed to the filter call, which would throw an
error on a non-array value. Add this validation immediately after the JSON.parse
assignment and before the filter call on allComments.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e95009b3-e6ca-4959-85b5-0999ab0afe74
📒 Files selected for processing (3)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/triage.ts
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/nemoclaw-maintainer-day/scripts/triage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
| let allComments: Array<{ body?: string }>; | ||
| try { | ||
| allComments = JSON.parse(raw) as Array<{ body?: string }>; | ||
| } catch { | ||
| return { pass: false, details: "Could not parse PR comments (invalid JSON — fail-closed)" }; | ||
| } | ||
|
|
||
| const praComments = allComments.filter((c) => | ||
| (c.body ?? "").includes("nemoclaw-pr-review-advisor"), | ||
| ); |
There was a problem hiding this comment.
Handle non-array API payloads to avoid a hard crash.
Line 286-295 assumes parsed JSON is always an array. If GitHub returns a JSON error object (valid JSON), allComments.filter(...) throws and bypasses fail-closed behavior.
Proposed fix
- let allComments: Array<{ body?: string }>;
+ let allComments: Array<{ body?: string; user?: { login?: string } }>;
try {
- allComments = JSON.parse(raw) as Array<{ body?: string }>;
+ const parsed = JSON.parse(raw) as unknown;
+ if (!Array.isArray(parsed)) {
+ return { pass: false, details: "Could not parse PR comments (unexpected JSON shape — fail-closed)" };
+ }
+ allComments = parsed as Array<{ body?: string; user?: { login?: string } }>;
} catch {
return { pass: false, details: "Could not parse PR comments (invalid JSON — fail-closed)" };
}📝 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.
| let allComments: Array<{ body?: string }>; | |
| try { | |
| allComments = JSON.parse(raw) as Array<{ body?: string }>; | |
| } catch { | |
| return { pass: false, details: "Could not parse PR comments (invalid JSON — fail-closed)" }; | |
| } | |
| const praComments = allComments.filter((c) => | |
| (c.body ?? "").includes("nemoclaw-pr-review-advisor"), | |
| ); | |
| let allComments: Array<{ body?: string; user?: { login?: string } }>; | |
| try { | |
| const parsed = JSON.parse(raw) as unknown; | |
| if (!Array.isArray(parsed)) { | |
| return { pass: false, details: "Could not parse PR comments (unexpected JSON shape — fail-closed)" }; | |
| } | |
| allComments = parsed as Array<{ body?: string; user?: { login?: string } }>; | |
| } catch { | |
| return { pass: false, details: "Could not parse PR comments (invalid JSON — fail-closed)" }; | |
| } | |
| const praComments = allComments.filter((c) => | |
| (c.body ?? "").includes("nemoclaw-pr-review-advisor"), | |
| ); |
🤖 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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts around lines
286 - 295, After parsing the JSON in the try block, add a type check to verify
that allComments is actually an array before calling the filter method. If the
parsed JSON is not an array (such as when GitHub returns an error object),
return the fail-closed response with an appropriate error message instead of
allowing the code to proceed to the filter call, which would throw an error on a
non-array value. Add this validation immediately after the JSON.parse assignment
and before the filter call on allComments.
|
Closing in favour of a PR directly from the main repo branch. |
2 participants
Summary
prAdvisor) tocheck-gates.tsthat fetches the PR Review Advisor sticky comment, parses therecommendation:field from its embedded HTML metadata, and setsallPass: falsewhen the value isblockedtriage.tswith a comment making it explicit that CodeRabbit and PRA are both skipped there for performance, and that amerge-nowbucket assignment does not meancheck-gates.tscan be skippedMERGE-GATE.md: removes the "manual review step" caveat, documents the gate as automated, and removes a wrong claim that the advisor link appears ingates.prAdvisor.detailsMotivation
PR #5526 was approved despite the PR Review Advisor posting a Blocked status with two required fixes. The advisor check was documented as "manual" in
MERGE-GATE.md, making it easy to skip. Making it a programmatic gate meansallPasswill befalseon any blocked advisor comment, preventing the approval flow from proceeding.Test evidence
A PR with no PRA comment returns
"pass": true, "details": "No PR Review Advisor comment found".Test plan
check-gates.tsagainst a PR where the advisor is blocked —prAdvisor.passshould befalseandallPassshould befalsecheck-gates.tsagainst a PR with no advisor comment —prAdvisor.passshould betruecheck-gates.tsagainst a PR where the advisor isapproved—prAdvisor.passshould betrueSigned-off-by: Preksha Vyas prekshiv@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit