From 8e012dc98c3c4bd53d64ac4f072d4a9f23729db0 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Mon, 22 Jun 2026 11:44:25 -0700 Subject: [PATCH 1/3] feat(skills/merge-gate): add PR Review Advisor as a hard gate in check-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 --- .../nemoclaw-maintainer-day/MERGE-GATE.md | 10 +-- .../scripts/check-gates.ts | 74 ++++++++++++++++++- .../nemoclaw-maintainer-day/scripts/triage.ts | 6 +- 3 files changed, 79 insertions(+), 11 deletions(-) diff --git a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md index 46ab27b43b8..5e0a3cbff1e 100644 --- a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md +++ b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md @@ -9,7 +9,7 @@ For the full priority list see [PR-REVIEW-PRIORITIES.md](PR-REVIEW-PRIORITIES.md 1. **CI green** — all required checks in `statusCheckRollup`. 2. **No conflicts** — `mergeStateStatus` clean. 3. **No major CodeRabbit** — ignore style nits; block on correctness/security bugs. -4. **No unresolved actionable PR Review Advisor findings** — correctness, security, acceptance, and test-depth findings block until addressed or explicitly judged false-positive. +4. **PR Review Advisor not blocked** — `check-gates.ts` now checks this automatically; `allPass` will be false if the latest advisor comment has `recommendation: blocked`. Correctness, security, acceptance, and test-depth findings block until addressed or explicitly judged false-positive. 5. **Risky code tested** — see [RISKY-AREAS.md](RISKY-AREAS.md). Confirm tests exist (added or pre-existing). ## Step 1: Run the Gate Checker @@ -18,7 +18,7 @@ For the full priority list see [PR-REVIEW-PRIORITIES.md](PR-REVIEW-PRIORITIES.md node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts ``` -This checks the deterministic gates programmatically and returns structured JSON with `allPass` and per-gate `pass`/`details`. PR Review Advisor follow-up remains a manual review step; use [PR CI and Automated Review Follow-Up](../_shared/pr-follow-up.md) for the shared triage loop. +This checks all gates programmatically and returns structured JSON with `allPass` and per-gate `pass`/`details`, including the PR Review Advisor status. Use [PR CI and Automated Review Follow-Up](../_shared/pr-follow-up.md) for the shared triage loop when individual findings need investigation. ## Step 2: Interpret Results @@ -29,16 +29,16 @@ The script handles the deterministic checks. You handle judgment calls: - **CI failing but narrow:** Follow the salvage workflow in [SALVAGE-PR.md](SALVAGE-PR.md). - **CI pending:** Wait and re-check. Do not approve while checks are still running. - **CodeRabbit:** Script flags unresolved major/critical threads. Review the `snippet` to confirm it's a real issue vs style nit. If doubt, leave unapproved. -- **PR Review Advisor:** Read the latest sticky advisor comment and apply [PR CI and Automated Review Follow-Up](../_shared/pr-follow-up.md). Valid correctness, security, acceptance-coverage, and test-depth findings block approval unless explicitly judged false-positive. +- **PR Review Advisor blocked:** `gates.prAdvisor.pass` will be false and `allPass` false. Read the full advisor comment on the PR, apply [PR CI and Automated Review Follow-Up](../_shared/pr-follow-up.md), and do not approve until the required findings are addressed or explicitly judged false-positive by a maintainer. - **Tests:** If `riskyCodeTested.pass` is false, follow [TEST-GAPS.md](TEST-GAPS.md). ## Step 3: Approve or Report -**Approve only when:** `allPass` is true, `mergeStateStatus` is not DIRTY, and the latest PR Review Advisor comment has no unresolved actionable findings. Approving a PR with conflicts is wasted effort — the rebase will invalidate the approval. +**Approve only when:** `allPass` is true and `mergeStateStatus` is not DIRTY. `allPass` now includes the PR Review Advisor gate, so a blocked advisor comment alone prevents approval. Approving a PR with conflicts is wasted effort — the rebase will invalidate the approval. The correct sequence for a conflicted PR: **salvage (rebase) → CI green → approve → report ready for merge.** -**All pass + no conflicts + no actionable PR Review Advisor findings:** Approve and summarize why. +**All pass + no conflicts:** Approve and summarize why. **Any fail:** diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts index 1fd5c207b30..aaf5abcf97d 100644 --- a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts @@ -4,7 +4,7 @@ /** * Deterministic merge-gate checker for a single NemoClaw PR. * - * Checks all 4 required gates and outputs structured JSON. + * Checks all 5 required gates and outputs structured JSON. * Claude uses the output to decide: approve, route to salvage, or report blockers. * * Usage: node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts [--repo OWNER/REPO] @@ -36,6 +36,11 @@ interface CodeRabbitThread { resolved: boolean; } +interface PrAdvisorGateResult extends GateResult { + recommendation?: string; + openRequired?: number; +} + interface GateOutput { pr: number; url: string; @@ -50,6 +55,7 @@ interface GateOutput { conflicts: GateResult & { mergeStateStatus?: string }; coderabbit: GateResult & { unresolvedThreads?: CodeRabbitThread[] }; riskyCodeTested: GateResult & { riskyFiles?: string[]; hasTests?: boolean }; + prAdvisor: PrAdvisorGateResult; }; } @@ -260,7 +266,66 @@ function checkCodeRabbit( } // --------------------------------------------------------------------------- -// Gate 4: Risky code has tests +// Gate 4: PR Review Advisor not blocked +// --------------------------------------------------------------------------- + +// The PRA bot embeds machine-readable metadata in an HTML comment: +// +// +const PRA_META_RE = /recommendation:\s*([a-z_]+)/i; +const PRA_REQUIRED_RE = /\*\*Open items:\*\*[^|]*?(\d+)\s+required/; + +function checkPrAdvisor(repo: string, number: number): PrAdvisorGateResult { + const raw = run("gh", ["api", `repos/${repo}/issues/${number}/comments`, "--paginate"]); + + if (!raw) { + // run() returns "" on API failure — fail closed, same as CodeRabbit gate + return { pass: false, details: "Could not fetch PR comments (API error — fail-closed)" }; + } + + 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"), + ); + 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 ?? ""; + + const metaMatch = PRA_META_RE.exec(body); + if (!metaMatch) { + return { + pass: true, + details: "PR Review Advisor comment found but no recommendation metadata", + }; + } + + const recommendation = metaMatch[1].toLowerCase(); + if (recommendation !== "blocked") { + return { pass: true, details: `PR Review Advisor: ${recommendation}`, recommendation }; + } + + const requiredMatch = PRA_REQUIRED_RE.exec(body); + const openRequired = requiredMatch ? parseInt(requiredMatch[1], 10) : undefined; + + return { + pass: false, + details: `PR Review Advisor: blocked${openRequired !== undefined ? ` (${openRequired} required item(s))` : ""}`, + recommendation, + openRequired, + }; +} + +// --------------------------------------------------------------------------- +// Gate 5: Risky code has tests // --------------------------------------------------------------------------- function checkRiskyCodeTested( @@ -329,13 +394,14 @@ function main(): void { const conflicts = checkConflicts(prData.mergeStateStatus); const coderabbit = checkCodeRabbit(repo, prNumber); const riskyCodeTested = checkRiskyCodeTested(prData.files ?? []); + const prAdvisor = checkPrAdvisor(repo, prNumber); const output: GateOutput = { pr: prNumber, url: prData.url, title: prData.title, - allPass: ci.pass && conflicts.pass && coderabbit.pass && riskyCodeTested.pass, - gates: { ci, conflicts, coderabbit, riskyCodeTested }, + allPass: ci.pass && conflicts.pass && coderabbit.pass && riskyCodeTested.pass && prAdvisor.pass, + gates: { ci, conflicts, coderabbit, riskyCodeTested, prAdvisor }, }; console.log(JSON.stringify(output, null, 2)); diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/triage.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/triage.ts index bacf672ad06..27c81171c0b 100644 --- a/.agents/skills/nemoclaw-maintainer-day/scripts/triage.ts +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/triage.ts @@ -268,8 +268,10 @@ function classifyPr(pr: PrData): ClassifiedPr { const blocked = mergeState === "BLOCKED"; if (blocked && !hasConflict) reasons.push("merge-blocked"); - // Simple CodeRabbit heuristic: check labels for major findings - // (Full CodeRabbit check is in check-gates.ts via GraphQL) + // CodeRabbit and PR Review Advisor are not checked here — fetching per-PR + // comments/threads for every open PR would make triage too slow. Both are + // checked as hard gates in check-gates.ts. A PR may appear as merge-now + // here but still fail the gate — always run check-gates.ts before approving. const coderabbitMajor = false; // conservative — gate checker does the real check // Classify into buckets From a5326ea66b1d52af4f64aaf89ea2760e9be0fea6 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Mon, 22 Jun 2026 12:49:22 -0700 Subject: [PATCH 2/3] fix(skills/merge-gate): address PRA-3/4/5/6 in check-gates advisor gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../scripts/check-gates.ts | 73 ++----- .../scripts/pra-gate.ts | 146 +++++++++++++ test/skills/check-gates-pra.test.ts | 192 ++++++++++++++++++ 3 files changed, 360 insertions(+), 51 deletions(-) create mode 100644 .agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts create mode 100644 test/skills/check-gates-pra.test.ts diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts index aaf5abcf97d..6889a785d02 100644 --- a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts @@ -19,6 +19,12 @@ import { REQUIRED_CHECK_NAMES, type StatusCheck, } from "./shared.ts"; +import { + parsePraCommentNdjson, + selectLatestTrustedPraComment, + evalPraComment, + type PrAdvisorGateResult, +} from "./pra-gate.ts"; // --------------------------------------------------------------------------- // Types @@ -36,11 +42,6 @@ interface CodeRabbitThread { resolved: boolean; } -interface PrAdvisorGateResult extends GateResult { - recommendation?: string; - openRequired?: number; -} - interface GateOutput { pr: number; url: string; @@ -269,59 +270,28 @@ function checkCodeRabbit( // Gate 4: PR Review Advisor not blocked // --------------------------------------------------------------------------- -// The PRA bot embeds machine-readable metadata in an HTML comment: -// -// -const PRA_META_RE = /recommendation:\s*([a-z_]+)/i; -const PRA_REQUIRED_RE = /\*\*Open items:\*\*[^|]*?(\d+)\s+required/; - -function checkPrAdvisor(repo: string, number: number): PrAdvisorGateResult { - const raw = run("gh", ["api", `repos/${repo}/issues/${number}/comments`, "--paginate"]); +function checkPrAdvisor(repo: string, number: number, headSha: string): PrAdvisorGateResult { + // --jq ".[]" emits one JSON object per line (NDJSON) — deterministic across pages + const raw = run("gh", [ + "api", + `repos/${repo}/issues/${number}/comments`, + "--paginate", + "--jq", + ".[]", + ]); if (!raw) { - // run() returns "" on API failure — fail closed, same as CodeRabbit gate return { pass: false, details: "Could not fetch PR comments (API error — fail-closed)" }; } - 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 allComments = parsePraCommentNdjson(raw); + const latest = selectLatestTrustedPraComment(allComments); - const praComments = allComments.filter((c) => - (c.body ?? "").includes("nemoclaw-pr-review-advisor"), - ); - if (praComments.length === 0) { + if (!latest) { 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 ?? ""; - - const metaMatch = PRA_META_RE.exec(body); - if (!metaMatch) { - return { - pass: true, - details: "PR Review Advisor comment found but no recommendation metadata", - }; - } - - const recommendation = metaMatch[1].toLowerCase(); - if (recommendation !== "blocked") { - return { pass: true, details: `PR Review Advisor: ${recommendation}`, recommendation }; - } - - const requiredMatch = PRA_REQUIRED_RE.exec(body); - const openRequired = requiredMatch ? parseInt(requiredMatch[1], 10) : undefined; - - return { - pass: false, - details: `PR Review Advisor: blocked${openRequired !== undefined ? ` (${openRequired} required item(s))` : ""}`, - recommendation, - openRequired, - }; + return evalPraComment(latest, headSha); } // --------------------------------------------------------------------------- @@ -375,7 +345,7 @@ function main(): void { "--repo", repo, "--json", - "number,title,url,files,statusCheckRollup,mergeStateStatus", + "number,title,url,files,statusCheckRollup,mergeStateStatus,headRefOid", ]) as { number: number; title: string; @@ -383,6 +353,7 @@ function main(): void { files: Array<{ path: string; status: string }>; statusCheckRollup: StatusCheck[]; mergeStateStatus: string; + headRefOid: string; } | null; if (!prData) { @@ -394,7 +365,7 @@ function main(): void { const conflicts = checkConflicts(prData.mergeStateStatus); const coderabbit = checkCodeRabbit(repo, prNumber); const riskyCodeTested = checkRiskyCodeTested(prData.files ?? []); - const prAdvisor = checkPrAdvisor(repo, prNumber); + const prAdvisor = checkPrAdvisor(repo, prNumber, prData.headRefOid ?? ""); const output: GateOutput = { pr: prNumber, diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts new file mode 100644 index 00000000000..66a78e6cc2e --- /dev/null +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts @@ -0,0 +1,146 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Pure PR Review Advisor gate logic — no shell calls, fully unit-testable. + * + * Exported and used by check-gates.ts. Separated so tests can exercise the + * parsing and provenance validation without mocking `gh`. + */ + +// --------------------------------------------------------------------------- +// Types +// --------------------------------------------------------------------------- + +export interface PraComment { + id: number; + user?: { login?: string }; + body?: string; +} + +export interface PraMeta { + headSha: string; + recommendation: string; + runId: number; + runAttempt: number; + commentId: number; +} + +export interface PrAdvisorGateResult { + pass: boolean; + details: string; + recommendation?: string; + openRequired?: number; +} + +// --------------------------------------------------------------------------- +// Constants +// --------------------------------------------------------------------------- + +// Explicit allowlist: only these recommendation values mean "OK to merge". +// Anything else — including unknown values — fails the gate. +export const PRA_PASS_RECOMMENDATIONS = new Set(["approved", "merge_as_is"]); + +// Full metadata line: all five fields must be present for a trusted comment. +const PRA_FULL_META_RE = + /head_sha:\s*([0-9a-f]+);\s*recommendation:\s*([a-z_]+);\s*run_id:\s*(\d+);\s*run_attempt:\s*(\d+);\s*comment_id:\s*(\d+)/i; + +const PRA_REQUIRED_RE = /\*\*Open items:\*\*[^|]*?(\d+)\s+required/; + +// --------------------------------------------------------------------------- +// Pure functions +// --------------------------------------------------------------------------- + +/** + * Parse the embedded HTML metadata from a PRA comment body. + * Returns null when metadata is absent or any required field is missing. + */ +export function parsePraMeta(body: string): PraMeta | null { + const m = PRA_FULL_META_RE.exec(body); + if (!m) return null; + return { + headSha: m[1].toLowerCase(), + recommendation: m[2].toLowerCase(), + runId: parseInt(m[3], 10), + runAttempt: parseInt(m[4], 10), + commentId: parseInt(m[5], 10), + }; +} + +/** + * Evaluate a single PRA comment against the current PR head SHA. + * Validates provenance (comment_id and head_sha) before trusting the + * recommendation so a spoofed or stale comment cannot bypass the gate. + */ +export function evalPraComment(comment: PraComment, headSha: string): PrAdvisorGateResult { + const body = comment.body ?? ""; + const meta = parsePraMeta(body); + + if (!meta) { + return { + pass: false, + details: "PR Review Advisor marker present but metadata incomplete — fail-closed", + }; + } + + if (meta.commentId !== comment.id) { + return { + pass: false, + details: "PR Review Advisor comment_id mismatch — fail-closed (possible spoof)", + }; + } + + const normalizedHead = headSha.toLowerCase(); + if (meta.headSha !== normalizedHead) { + return { + pass: false, + details: `PR Review Advisor is stale (sha ${meta.headSha.slice(0, 7)} ≠ head ${normalizedHead.slice(0, 7)}) — re-run CI`, + }; + } + + const rec = meta.recommendation; + if (PRA_PASS_RECOMMENDATIONS.has(rec)) { + return { pass: true, details: `PR Review Advisor: ${rec}`, recommendation: rec }; + } + + const requiredMatch = PRA_REQUIRED_RE.exec(body); + const openRequired = requiredMatch ? parseInt(requiredMatch[1], 10) : undefined; + + return { + pass: false, + details: `PR Review Advisor: ${rec}${openRequired !== undefined ? ` (${openRequired} required item(s))` : ""}`, + recommendation: rec, + openRequired, + }; +} + +/** + * Parse NDJSON output from `gh api --paginate --jq ".[]"`. + * Each line is one JSON comment object; malformed lines are skipped. + */ +export function parsePraCommentNdjson(raw: string): PraComment[] { + const comments: PraComment[] = []; + for (const line of raw.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + try { + comments.push(JSON.parse(trimmed) as PraComment); + } catch { + // skip malformed lines + } + } + return comments; +} + +/** + * Return the latest github-actions[bot] comment that contains the PRA marker. + * Only github-actions[bot] is trusted; user-posted comments are ignored. + */ +export function selectLatestTrustedPraComment(comments: PraComment[]): PraComment | null { + const trusted = comments.filter( + (c) => + c.user?.login === "github-actions[bot]" && + (c.body ?? "").includes("nemoclaw-pr-review-advisor"), + ); + return trusted.length > 0 ? trusted[trusted.length - 1] : null; +} diff --git a/test/skills/check-gates-pra.test.ts b/test/skills/check-gates-pra.test.ts new file mode 100644 index 00000000000..1e273ffc5e7 --- /dev/null +++ b/test/skills/check-gates-pra.test.ts @@ -0,0 +1,192 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + evalPraComment, + parsePraCommentNdjson, + parsePraMeta, + PRA_PASS_RECOMMENDATIONS, + selectLatestTrustedPraComment, +} from "../../.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts"; + +const HEAD = "8e012dc98c3c4bd53d64ac4f072d4a9f23729db0"; + +function makeBody( + overrides: Partial<{ headSha: string; recommendation: string; commentId: number }>, +): string { + const headSha = overrides.headSha ?? HEAD; + const recommendation = overrides.recommendation ?? "blocked"; + const commentId = overrides.commentId ?? 42; + return [ + "", + ``, + "## PR Review Advisor", + "**Open items:** 2 required · 1 warning", + ].join("\n"); +} + +function makeComment(overrides: Partial<{ id: number; login: string; body: string }> = {}) { + return { + id: overrides.id ?? 42, + user: { login: overrides.login ?? "github-actions[bot]" }, + body: overrides.body ?? makeBody({}), + }; +} + +// --------------------------------------------------------------------------- +// parsePraMeta +// --------------------------------------------------------------------------- + +describe("parsePraMeta", () => { + it("parses all five fields from a well-formed body", () => { + const body = makeBody({ headSha: HEAD, recommendation: "blocked", commentId: 99 }); + const meta = parsePraMeta(body); + expect(meta).not.toBeNull(); + expect(meta?.headSha).toBe(HEAD.toLowerCase()); + expect(meta?.recommendation).toBe("blocked"); + expect(meta?.commentId).toBe(99); + }); + + it("returns null when metadata line is absent", () => { + expect(parsePraMeta("\nsome body text")).toBeNull(); + }); + + it("returns null when any field is missing", () => { + expect(parsePraMeta("")).toBeNull(); + }); + + it("normalises headSha to lowercase", () => { + const body = makeBody({ headSha: HEAD.toUpperCase() }); + expect(parsePraMeta(body)?.headSha).toBe(HEAD.toLowerCase()); + }); +}); + +// --------------------------------------------------------------------------- +// parsePraCommentNdjson +// --------------------------------------------------------------------------- + +describe("parsePraCommentNdjson", () => { + it("parses multiple NDJSON lines", () => { + const lines = [ + JSON.stringify({ id: 1, user: { login: "alice" }, body: "hello" }), + JSON.stringify({ id: 2, user: { login: "bob" }, body: "world" }), + ].join("\n"); + const comments = parsePraCommentNdjson(lines); + expect(comments).toHaveLength(2); + expect(comments[0].id).toBe(1); + expect(comments[1].id).toBe(2); + }); + + it("skips blank lines and malformed JSON", () => { + const raw = `${JSON.stringify({ id: 1 })}\n\nnot json\n${JSON.stringify({ id: 2 })}`; + expect(parsePraCommentNdjson(raw)).toHaveLength(2); + }); + + it("returns empty array for empty input", () => { + expect(parsePraCommentNdjson("")).toHaveLength(0); + }); +}); + +// --------------------------------------------------------------------------- +// selectLatestTrustedPraComment +// --------------------------------------------------------------------------- + +describe("selectLatestTrustedPraComment", () => { + it("returns the last github-actions[bot] comment with the PRA marker", () => { + const comments = [makeComment({ id: 1 }), makeComment({ id: 2 })]; + expect(selectLatestTrustedPraComment(comments)?.id).toBe(2); + }); + + it("ignores comments from non-bot users", () => { + const comments = [ + makeComment({ id: 1, login: "alice" }), + makeComment({ id: 2, login: "github-actions[bot]" }), + makeComment({ id: 3, login: "malicious-user" }), + ]; + expect(selectLatestTrustedPraComment(comments)?.id).toBe(2); + }); + + it("ignores bot comments without the PRA marker", () => { + const comments = [ + { id: 1, user: { login: "github-actions[bot]" }, body: "some other bot comment" }, + ]; + expect(selectLatestTrustedPraComment(comments)).toBeNull(); + }); + + it("returns null when no trusted comments exist", () => { + expect(selectLatestTrustedPraComment([])).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// evalPraComment — provenance checks +// --------------------------------------------------------------------------- + +describe("evalPraComment — provenance", () => { + it("fails closed when metadata is incomplete", () => { + const comment = makeComment({ body: "\nno metadata" }); + const result = evalPraComment(comment, HEAD); + expect(result.pass).toBe(false); + expect(result.details).toMatch(/incomplete/i); + }); + + it("fails closed when comment_id does not match actual comment id", () => { + const comment = makeComment({ id: 99, body: makeBody({ commentId: 1 }) }); + const result = evalPraComment(comment, HEAD); + expect(result.pass).toBe(false); + expect(result.details).toMatch(/mismatch/i); + }); + + it("fails closed when head_sha is stale", () => { + const staleHead = "a".repeat(40); + const comment = makeComment({ body: makeBody({ headSha: staleHead }) }); + const result = evalPraComment(comment, HEAD); + expect(result.pass).toBe(false); + expect(result.details).toMatch(/stale/i); + }); +}); + +// --------------------------------------------------------------------------- +// evalPraComment — recommendation values +// --------------------------------------------------------------------------- + +describe("evalPraComment — recommendations", () => { + for (const rec of PRA_PASS_RECOMMENDATIONS) { + it(`passes for recommendation="${rec}"`, () => { + const comment = makeComment({ body: makeBody({ recommendation: rec }) }); + const result = evalPraComment(comment, HEAD); + expect(result.pass).toBe(true); + expect(result.recommendation).toBe(rec); + }); + } + + it("fails for recommendation=blocked", () => { + const comment = makeComment(); + const result = evalPraComment(comment, HEAD); + expect(result.pass).toBe(false); + expect(result.recommendation).toBe("blocked"); + }); + + it("fails for recommendation=merge_after_fixes", () => { + const comment = makeComment({ body: makeBody({ recommendation: "merge_after_fixes" }) }); + expect(evalPraComment(comment, HEAD).pass).toBe(false); + }); + + it("fails for recommendation=needs_rework", () => { + const comment = makeComment({ body: makeBody({ recommendation: "needs_rework" }) }); + expect(evalPraComment(comment, HEAD).pass).toBe(false); + }); + + it("fails for unknown recommendation values", () => { + const comment = makeComment({ body: makeBody({ recommendation: "some_future_state" }) }); + expect(evalPraComment(comment, HEAD).pass).toBe(false); + }); + + it("extracts openRequired from the Open items line", () => { + const comment = makeComment(); + const result = evalPraComment(comment, HEAD); + expect(result.openRequired).toBe(2); + }); +}); From b363569962f599ed098e2114486e9d821a981ae6 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Mon, 22 Jun 2026 16:49:49 -0700 Subject: [PATCH 3/3] fix(skills/merge-gate): validate advisor Actions run provenance in PRA 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 --- .../nemoclaw-maintainer-day/MERGE-GATE.md | 2 +- .../scripts/check-gates.ts | 27 ++++++ .../scripts/pra-gate.ts | 47 +++++++++- test/skills/check-gates-pra.test.ts | 85 +++++++++++++++++++ 4 files changed, 159 insertions(+), 2 deletions(-) diff --git a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md index 5e0a3cbff1e..a0b264844d4 100644 --- a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md +++ b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md @@ -9,7 +9,7 @@ For the full priority list see [PR-REVIEW-PRIORITIES.md](PR-REVIEW-PRIORITIES.md 1. **CI green** — all required checks in `statusCheckRollup`. 2. **No conflicts** — `mergeStateStatus` clean. 3. **No major CodeRabbit** — ignore style nits; block on correctness/security bugs. -4. **PR Review Advisor not blocked** — `check-gates.ts` now checks this automatically; `allPass` will be false if the latest advisor comment has `recommendation: blocked`. Correctness, security, acceptance, and test-depth findings block until addressed or explicitly judged false-positive. +4. **PR Review Advisor: merge_as_is** — `check-gates.ts` checks this automatically. The gate passes only when the latest advisor comment has `recommendation: merge_as_is`. All other recommendation values — including `blocked`, `needs_rework`, `merge_after_fixes`, `superseded`, `info_only`, and any unknown value — fail the gate. The referenced Actions run is validated (name, event, head SHA, run attempt, timestamp) before the recommendation is trusted. Correctness, security, acceptance, and test-depth findings block until addressed or explicitly judged false-positive by a maintainer. 5. **Risky code tested** — see [RISKY-AREAS.md](RISKY-AREAS.md). Confirm tests exist (added or pre-existing). ## Step 1: Run the Gate Checker diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts index 6889a785d02..fb762bbc896 100644 --- a/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts @@ -21,8 +21,11 @@ import { } from "./shared.ts"; import { parsePraCommentNdjson, + parsePraMeta, selectLatestTrustedPraComment, evalPraComment, + validateAdvisorRun, + type PraRun, type PrAdvisorGateResult, } from "./pra-gate.ts"; @@ -291,6 +294,30 @@ function checkPrAdvisor(repo: string, number: number, headSha: string): PrAdviso return { pass: true, details: "No PR Review Advisor comment found" }; } + // Validate the referenced Actions run before trusting the recommendation. + // github-actions[bot] is a shared identity across all workflows in the repo. + // A different workflow posting a comment with the same marker format would + // pass comment_id/head_sha checks without this step. + const meta = parsePraMeta(latest.body ?? ""); + if (meta) { + const runRaw = run("gh", ["api", `repos/${repo}/actions/runs/${meta.runId}`]); + if (!runRaw) { + return { pass: false, details: "Could not validate advisor run (API error — fail-closed)" }; + } + let runData: PraRun; + try { + runData = JSON.parse(runRaw) as PraRun; + } catch { + return { pass: false, details: "Could not parse advisor run response — fail-closed" }; + } + if (!validateAdvisorRun(runData, meta, latest.updated_at ?? "")) { + return { + pass: false, + details: "PR Review Advisor run provenance check failed — fail-closed", + }; + } + } + return evalPraComment(latest, headSha); } diff --git a/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts b/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts index 66a78e6cc2e..01cd37f3d11 100644 --- a/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts +++ b/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts @@ -16,6 +16,17 @@ export interface PraComment { id: number; user?: { login?: string }; body?: string; + updated_at?: string; +} + +export interface PraRun { + name?: string; + head_sha?: string; + event?: string; + run_attempt?: number; + run_started_at?: string; + created_at?: string; + updated_at?: string; } export interface PraMeta { @@ -39,7 +50,9 @@ export interface PrAdvisorGateResult { // Explicit allowlist: only these recommendation values mean "OK to merge". // Anything else — including unknown values — fails the gate. -export const PRA_PASS_RECOMMENDATIONS = new Set(["approved", "merge_as_is"]); +// Source: SUMMARY_RECOMMENDATIONS in tools/pr-review-advisor/analyze.mts. +// "approved" is not a valid advisor recommendation; only "merge_as_is" is. +export const PRA_PASS_RECOMMENDATIONS = new Set(["merge_as_is"]); // Full metadata line: all five fields must be present for a trusted comment. const PRA_FULL_META_RE = @@ -144,3 +157,35 @@ export function selectLatestTrustedPraComment(comments: PraComment[]): PraCommen ); return trusted.length > 0 ? trusted[trusted.length - 1] : null; } + +// --------------------------------------------------------------------------- +// Run provenance +// --------------------------------------------------------------------------- + +function isTimestampWithin(value: string, start: string, end: string): boolean { + const t = Date.parse(value); + const s = Date.parse(start); + const e = Date.parse(end); + if (![t, s, e].every(Number.isFinite)) return false; + return t >= s && t <= e; +} + +/** + * Verify that a GitHub Actions run corresponds to the trusted PR Review / Advisor + * workflow for this PR head. Mirrors isTrustedAdvisorRun() in + * tools/pr-review-advisor/analyze.mts. + * + * Pure function — the caller is responsible for fetching the run data. + */ +export function validateAdvisorRun(run: PraRun, meta: PraMeta, commentUpdatedAt: string): boolean { + const startedAt = run.run_started_at ?? run.created_at; + const endedAt = run.updated_at; + if (!startedAt || !endedAt) return false; + return ( + run.name === "PR Review / Advisor" && + run.event === "pull_request" && + (run.head_sha ?? "").toLowerCase() === meta.headSha && + (run.run_attempt ?? -1) === meta.runAttempt && + isTimestampWithin(commentUpdatedAt, startedAt, endedAt) + ); +} diff --git a/test/skills/check-gates-pra.test.ts b/test/skills/check-gates-pra.test.ts index 1e273ffc5e7..e6af25493df 100644 --- a/test/skills/check-gates-pra.test.ts +++ b/test/skills/check-gates-pra.test.ts @@ -9,6 +9,9 @@ import { parsePraMeta, PRA_PASS_RECOMMENDATIONS, selectLatestTrustedPraComment, + validateAdvisorRun, + type PraMeta, + type PraRun, } from "../../.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts"; const HEAD = "8e012dc98c3c4bd53d64ac4f072d4a9f23729db0"; @@ -190,3 +193,85 @@ describe("evalPraComment — recommendations", () => { expect(result.openRequired).toBe(2); }); }); + +// --------------------------------------------------------------------------- +// validateAdvisorRun +// --------------------------------------------------------------------------- + +const RUN_START = "2026-01-01T00:00:00Z"; +const RUN_END = "2026-01-01T01:00:00Z"; +const COMMENT_TIME = "2026-01-01T00:30:00Z"; + +function makeRun(overrides: Partial = {}): PraRun { + return { + name: "PR Review / Advisor", + head_sha: HEAD, + event: "pull_request", + run_attempt: 1, + run_started_at: RUN_START, + updated_at: RUN_END, + ...overrides, + }; +} + +function makeMeta(overrides: Partial = {}): PraMeta { + return { + headSha: HEAD.toLowerCase(), + recommendation: "blocked", + runId: 1, + runAttempt: 1, + commentId: 42, + ...overrides, + }; +} + +describe("validateAdvisorRun", () => { + it("passes when all fields match and timestamp is within window", () => { + expect(validateAdvisorRun(makeRun(), makeMeta(), COMMENT_TIME)).toBe(true); + }); + + it("fails when run name is not PR Review / Advisor", () => { + expect(validateAdvisorRun(makeRun({ name: "Other Workflow" }), makeMeta(), COMMENT_TIME)).toBe( + false, + ); + }); + + it("fails when event is not pull_request", () => { + expect(validateAdvisorRun(makeRun({ event: "push" }), makeMeta(), COMMENT_TIME)).toBe(false); + }); + + it("fails when head_sha mismatches", () => { + expect( + validateAdvisorRun(makeRun({ head_sha: "b".repeat(40) }), makeMeta(), COMMENT_TIME), + ).toBe(false); + }); + + it("fails when run_attempt mismatches", () => { + expect(validateAdvisorRun(makeRun({ run_attempt: 2 }), makeMeta(), COMMENT_TIME)).toBe(false); + }); + + it("fails when comment timestamp is before run start", () => { + expect(validateAdvisorRun(makeRun(), makeMeta(), "2025-12-31T23:59:59Z")).toBe(false); + }); + + it("fails when comment timestamp is after run end", () => { + expect(validateAdvisorRun(makeRun(), makeMeta(), "2026-01-01T02:00:00Z")).toBe(false); + }); + + it("fails when run_started_at and created_at are both absent", () => { + const run = makeRun({ run_started_at: undefined, created_at: undefined }); + expect(validateAdvisorRun(run, makeMeta(), COMMENT_TIME)).toBe(false); + }); + + it("falls back to created_at when run_started_at is absent", () => { + const run = makeRun({ run_started_at: undefined, created_at: RUN_START }); + expect(validateAdvisorRun(run, makeMeta(), COMMENT_TIME)).toBe(true); + }); + + it("rejects github-actions bot PRA metadata unless the run is PR Review Advisor for the same head and attempt", () => { + // Simulates a different workflow posting a marker comment with valid comment_id and head_sha + // but a non-Advisor workflow name — run validation must reject it. + const spoofedRun = makeRun({ name: "CI / Build", event: "push" }); + expect(validateAdvisorRun(spoofedRun, makeMeta(), COMMENT_TIME)).toBe(false); + }); +});