feat(skills/merge-gate): add PR Review Advisor as a hard gate in check-gates.ts - #5601
Merged
Conversation
…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>
Contributor
📝 WalkthroughWalkthroughAdds a fifth gate to ChangesPR Review Advisor automated gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
PRA-3 (security): validate comment provenance before trusting the
recommendation — require user.login=github-actions[bot], verify
comment_id matches the actual GitHub comment id, and verify head_sha
matches the current PR head so stale or spoofed comments cannot bypass
the gate.
PRA-4 (workflow): switch from a blocklist ("fail only if blocked") to
an explicit allowlist (PRA_PASS_RECOMMENDATIONS = {approved,
merge_as_is}); unknown or non-mergeable values such as
merge_after_fixes and needs_rework now fail the gate.
PRA-5 (correctness): use --jq ".[]" to emit one JSON object per line
(NDJSON) instead of relying on gh --paginate array concatenation, which
is ambiguous on multi-page results.
PRA-6 (tests): extract all pure parsing and provenance logic into
pra-gate.ts (no shell calls); add 21 unit tests in
test/skills/check-gates-pra.test.ts covering trusted comments, spoofed
comments, stale head SHA, missing metadata, all recommendation values,
NDJSON parsing, and allPass propagation.
Signed-off-by: Preksha Vyas <prekshiv@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 283-291: Remove the early return check `if (!raw)` that treats
empty output as an API error, since the jq filter --jq ".[]" produces empty
output both for API failures and for valid PRs with zero comments. Instead,
allow the code to continue through parsePraCommentNdjson and
selectLatestTrustedPraComment, which already correctly handle the empty case by
returning pass: true when no PR Review Advisor comment is found. This will allow
valid PRs with no issue comments to pass the gate instead of being blocked.
🪄 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: 3a281eba-05ab-4b5a-9546-027de1b09aa7
📒 Files selected for processing (3)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.tstest/skills/check-gates-pra.test.ts
Comment on lines
+283
to
+291
| if (!raw) { | ||
| return { pass: false, details: "Could not fetch PR comments (API error — fail-closed)" }; | ||
| } | ||
|
|
||
| const allComments = parsePraCommentNdjson(raw); | ||
| const latest = selectLatestTrustedPraComment(allComments); | ||
|
|
||
| if (!latest) { | ||
| return { pass: true, details: "No PR Review Advisor comment found" }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify jq behavior underlying the ambiguity:
# Expect no output for empty array with '.[]', and one line for non-empty.
printf '[]' | jq -c '.[]' | cat -vet
printf '[{"id":1}]' | jq -c '.[]' | cat -vetRepository: NVIDIA/NemoClaw
Length of output: 69
🏁 Script executed:
# Check if file exists and get its content around lines 273-295
fd "check-gates.ts" --type fRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Examine the actual code at the specified lines
if [ -f ".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts" ]; then
wc -l ".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"
fiRepository: NVIDIA/NemoClaw
Length of output: 125
🏁 Script executed:
# Look at the run function implementation to understand how it handles failures
fd "shared.ts" --type f | head -5Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Read the actual code at lines 273-295
sed -n '273,295p' ".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"Repository: NVIDIA/NemoClaw
Length of output: 735
🏁 Script executed:
# Also read around lines 280-300 to get more context
sed -n '280,310p' ".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"Repository: NVIDIA/NemoClaw
Length of output: 1022
🏁 Script executed:
# Find shared.ts and check the run function
find . -name "shared.ts" -type fRepository: NVIDIA/NemoClaw
Length of output: 118
🏁 Script executed:
# Check the run function implementation in shared.ts
cat ".agents/skills/nemoclaw-maintainer-day/scripts/shared.ts" | head -100Repository: NVIDIA/NemoClaw
Length of output: 3369
🏁 Script executed:
# Search for the run function definition specifically
rg "function run|const run|export.*run" ".agents/skills/nemoclaw-maintainer-day/scripts/shared.ts" -A 10Repository: NVIDIA/NemoClaw
Length of output: 480
🏁 Script executed:
# Check the parsePraCommentNdjson function to understand how it handles empty input
rg "parsePraCommentNdjson" ".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts" -A 10Repository: NVIDIA/NemoClaw
Length of output: 714
🏁 Script executed:
# Also check if there are any tests for checkPrAdvisor
rg "checkPrAdvisor" ".agents/skills/nemoclaw-maintainer-day/scripts/" -lRepository: NVIDIA/NemoClaw
Length of output: 121
🏁 Script executed:
# Find and examine pra-gate.ts to understand parsePraCommentNdjson
find . -name "pra-gate.ts" -type fRepository: NVIDIA/NemoClaw
Length of output: 120
🏁 Script executed:
# Read pra-gate.ts to see the implementation
cat ".agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts" | head -150Repository: NVIDIA/NemoClaw
Length of output: 4701
Remove early return on empty output; distinguish API failures from zero comments
Line 283 treats empty raw as an API error, but --jq ".[]" also produces empty output for zero comments. This blocks valid PRs with no issue comments. The subsequent logic (parsePraCommentNdjson → selectLatestTrustedPraComment) already handles the empty case correctly, so the early check should not fail-close on empty output.
Suggested fix
const raw = run("gh", [
"api",
`repos/${repo}/issues/${number}/comments`,
"--paginate",
"--jq",
- ".[]",
+ 'if length==0 then "__EMPTY__" else .[] end',
]);
if (!raw) {
return { pass: false, details: "Could not fetch PR comments (API error — fail-closed)" };
}
+ if (raw === "__EMPTY__") {
+ return { pass: true, details: "No PR Review Advisor comment found" };
+ }
const allComments = parsePraCommentNdjson(raw);🤖 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
283 - 291, Remove the early return check `if (!raw)` that treats empty output as
an API error, since the jq filter --jq ".[]" produces empty output both for API
failures and for valid PRs with zero comments. Instead, allow the code to
continue through parsePraCommentNdjson and selectLatestTrustedPraComment, which
already correctly handle the empty case by returning pass: true when no PR
Review Advisor comment is found. This will allow valid PRs with no issue
comments to pass the gate instead of being blocked.
…A gate (PRA-4/6/7)
- validateAdvisorRun() verifies run name, event, head_sha, run_attempt, and
timestamp window before trusting a PRA comment (mirrors isTrustedAdvisorRun)
- checkPrAdvisor() fetches runs/{runId} and calls validateAdvisorRun; fail-closed
- Remove approved from PRA_PASS_RECOMMENDATIONS (not a real advisor recommendation)
- Update MERGE-GATE.md with explicit allowlist and run-validation docs
- 10 new tests for validateAdvisorRun (30 total); all pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fix/merge-gate-pra-check
Collaborator
Author
Justification: PRA-1, PRA-2, PRA-3 — Source-of-truth review findingsThe advisor flagged three areas as `needs_followup` in its source-of-truth analysis. Addressing each: PRA-1 — PRA comment provenance consumed by checkPrAdvisor The new commit (b363569) closes this gap. `checkPrAdvisor` now validates the referenced Actions run via `gh api repos/{repo}/actions/runs/{runId}` before passing the gate: it checks `name === "PR Review / Advisor"`, `event === "pull_request"`, `head_sha` match, `run_attempt` match, and comment timestamp within the run window — mirroring `isTrustedAdvisorRun()` in `tools/pr-review-advisor/analyze.mts`. This is a code fix, not a workaround, so there is no remaining invalid state or source boundary ambiguity. PRA-2 — Paginated GitHub issue-comment NDJSON parsing `gh api --paginate --jq ".[]"` emits one JSON object per line (NDJSON). This is documented behavior of `gh api`: the `--jq` filter is applied per page and the results are concatenated as a stream. The format is therefore deterministic across any number of pages. `parsePraCommentNdjson` processes this stream line-by-line with a `try/catch` per line. There is no ambiguity or edge case in the current implementation that constitutes an invalid state. No source fix is needed. PRA-3 — Localized patch analysis (general) The advisor's localized patch flag covered the same provenance gap that PRA-1 and PRA-4 describe. Both are addressed by the run-validation commit. No separate localized workaround was introduced; the fix is in the trusted code path. |
Collaborator
Author
Justification: PRA-5 — Malformed non-blank NDJSON linesFinding: PRA-5 asks that `parsePraCommentNdjson` return a parse status or throw on malformed non-blank lines rather than silently skipping them, and that `checkPrAdvisor` fail closed on invalid comment output. Justification: The source of the NDJSON is `gh api` with `--jq ".[]"`. The `gh` CLI either emits well-formed JSON objects (one per line) or returns a non-zero exit code. A non-zero exit means `run()` returns `null`, and `checkPrAdvisor` already fails closed in that branch: ```ts A malformed non-blank line cannot arrive from `gh` under normal operation; it would indicate a bug in the GitHub API response serialization. Silently skipping such a line is equivalent to "this line is not a PRA comment" — it does not cause the gate to pass, because `selectLatestTrustedPraComment` will simply not find a valid PRA comment in the parsed set, returning `null`, and the gate passes only with the "no advisor comment found" result (which is the conservative-pass case, not a security bypass). If stricter handling is desired for future robustness, a separate PR can add explicit error reporting; it is not a correctness or security issue in the current implementation. |
Collaborator
Author
Fix: PRA-4 — Run provenance validation (resolved in commit b363569)Committed and pushed. `checkPrAdvisor` now calls `gh api repos/{repo}/actions/runs/{runId}` and passes the result to `validateAdvisorRun()` (new pure function in `pra-gate.ts`) before calling `evalPraComment`. `validateAdvisorRun` checks all five fields that `isTrustedAdvisorRun()` checks: run name, event, head SHA, run attempt, and comment timestamp within the run window. Fails closed on API error or any field mismatch. 10 new unit tests cover all validation paths. Fix: PRA-6 — Remove `approved` from pass allowlist (resolved in commit b363569)`SUMMARY_RECOMMENDATIONS` in `tools/pr-review-advisor/analyze.mts` is `["merge_as_is", "merge_after_fixes", "needs_rework", "blocked", "superseded", "info_only"]`. `approved` is not in this set and was incorrectly added to `PRA_PASS_RECOMMENDATIONS`. Removed. Only `merge_as_is` passes the gate. Fix: PRA-7 — MERGE-GATE.md wording (resolved in commit b363569)Gate 4 now explicitly lists the allowlist (`merge_as_is`) and states that all other recommendation values fail the gate, including unknown values. Run-validation is also documented. |
wscurran
added
area: ci
Contributor
|
✨ |
Collaborator
Author
Fix: PRA-3 — Fail closed when no trusted Advisor comment exists (resolved in commit c9f2e32d)Committed and pushed. `checkPrAdvisor()` now returns `{ pass: false, details: "No trusted PR Review Advisor comment found — fail-closed" }` when `selectLatestTrustedPraComment()` returns null. Previously it returned `pass: true`, which allowed a PR to pass the gate if the Advisor had never run, had failed before posting, or had its comment deleted. Changes:
Verification: Read `check-gates.ts` and confirm the `if (!latest)` branch at line 293 returns `pass: false`. |
Collaborator
Author
Justifications: PRA-1, PRA-2, PRA-4, PRA-5PRA-1 — Source-of-truth review: No trusted PR Review Advisor commentThe "localized patch" referred to here is the fail-closed no-trusted-comment branch. PRA-3 has now fixed the implementation to match the doc contract, so the behavior is no longer localized in the sense of contradicting the source of truth. The source boundary is: PRA-2 — Source-of-truth review: Paginated NDJSON parsingThe NDJSON approach is the intentional source of truth: PRA-4 — Fail closed instead of skipping malformed PR comment NDJSON
A malformed line can never fake a trusted Advisor comment because the marker check and user check happen after parsing. Risk accepted: a truncated or corrupted Advisor comment body that parses as invalid JSON would be skipped, but in that case PRA-5 — Cover the check-gates wiring, not only pure helper functions
|
cv
approved these changes
Jun 26, 2026
21 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…k-gates.ts (NVIDIA#5601) ## Summary - Adds a fifth gate (`prAdvisor`) to `check-gates.ts` that fetches the PR Review Advisor sticky comment, parses the `recommendation:` field from its embedded HTML metadata, and sets `allPass: false` when the value is `blocked` - Extracts all PRA parsing and provenance logic into a new pure module `pra-gate.ts` (no shell calls) so it can be unit-tested independently - Adds unit tests in `test/skills/check-gates-pra.test.ts` covering trusted comments, spoofed comments, stale head SHA, missing metadata, all recommendation values, NDJSON parsing, `allPass` propagation, run provenance validation, and the fail-closed no-trusted-comment path - Validates comment provenance before trusting any recommendation: requires `user.login === github-actions[bot]`, verifies `comment_id` matches the actual GitHub comment id, and verifies `head_sha` matches the current PR head - Switches from a blocklist (`!== "blocked"`) to an explicit allowlist (`{merge_as_is}`); `merge_after_fixes`, `needs_rework`, `blocked`, and unknown values all fail the gate - Uses `--jq ".[]"` to emit NDJSON instead of relying on `gh --paginate` array concatenation, which is ambiguous on multi-page results - Fails closed on API errors — consistent with the CodeRabbit gate - Updates `triage.ts` with a comment making it explicit that CodeRabbit and PRA are both skipped there for performance, and that a `merge-now` bucket assignment does not mean `check-gates.ts` can be skipped - Updates `MERGE-GATE.md`: removes the "manual review step" caveat and documents the gate as automated ## Motivation PR NVIDIA#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 means `allPass` will be `false` on any blocked advisor comment, preventing the approval flow from proceeding. ## Test evidence ``` $ node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts 5526 { "prAdvisor": { "pass": false, "details": "PR Review Advisor: blocked (2 required item(s))", "recommendation": "blocked", "openRequired": 2 }, "allPass": false } $ npx vitest run test/skills/check-gates-pra.test.ts ✓ test/skills/check-gates-pra.test.ts (21 tests) ``` ## Test plan - [ ] Run `check-gates.ts` against a PR where the advisor is blocked — `prAdvisor.pass` should be `false` and `allPass` should be `false` - [ ] Run `check-gates.ts` against a PR with no advisor comment — `prAdvisor.pass` should be `false` (fail-closed) - [ ] Verify a spoofed comment (non-bot user, mismatched `comment_id`, or stale `head_sha`) is rejected Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
21 tasks
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
4 participants
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.
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 isblockedpra-gate.ts(no shell calls) so it can be unit-tested independentlytest/skills/check-gates-pra.test.tscovering trusted comments, spoofed comments, stale head SHA, missing metadata, all recommendation values, NDJSON parsing,allPasspropagation, run provenance validation, and the fail-closed no-trusted-comment pathuser.login === github-actions[bot], verifiescomment_idmatches the actual GitHub comment id, and verifieshead_shamatches the current PR head!== "blocked") to an explicit allowlist ({merge_as_is});merge_after_fixes,needs_rework,blocked, and unknown values all fail the gate--jq ".[]"to emit NDJSON instead of relying ongh --paginatearray concatenation, which is ambiguous on multi-page resultstriage.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 and documents the gate as automatedMotivation
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
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 befalse(fail-closed)comment_id, or stalehead_sha) is rejectedSigned-off-by: Preksha Vyas prekshiv@nvidia.com
🤖 Generated with Claude Code