Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: 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
Expand All @@ -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 <pr-number>
```

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

Expand All @@ -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:**

Expand Down
74 changes: 69 additions & 5 deletions .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <pr-number> [--repo OWNER/REPO]
Expand All @@ -19,6 +19,15 @@ import {
REQUIRED_CHECK_NAMES,
type StatusCheck,
} from "./shared.ts";
import {
parsePraCommentNdjson,
parsePraMeta,
selectLatestTrustedPraComment,
evalPraComment,
validateAdvisorRun,
type PraRun,
type PrAdvisorGateResult,
} from "./pra-gate.ts";

// ---------------------------------------------------------------------------
// Types
Expand Down Expand Up @@ -50,6 +59,7 @@ interface GateOutput {
conflicts: GateResult & { mergeStateStatus?: string };
coderabbit: GateResult & { unresolvedThreads?: CodeRabbitThread[] };
riskyCodeTested: GateResult & { riskyFiles?: string[]; hasTests?: boolean };
prAdvisor: PrAdvisorGateResult;
};
}

Expand Down Expand Up @@ -260,7 +270,59 @@ function checkCodeRabbit(
}

// ---------------------------------------------------------------------------
// Gate 4: Risky code has tests
// Gate 4: PR Review Advisor not blocked
// ---------------------------------------------------------------------------

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) {
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" };
Comment on lines +286 to +294

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -vet

Repository: 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 f

Repository: 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"
fi

Repository: 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 -5

Repository: 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 f

Repository: 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 -100

Repository: 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 10

Repository: 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 10

Repository: 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/" -l

Repository: NVIDIA/NemoClaw

Length of output: 121


🏁 Script executed:

# Find and examine pra-gate.ts to understand parsePraCommentNdjson
find . -name "pra-gate.ts" -type f

Repository: 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 -150

Repository: 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.

}

// 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);
}

// ---------------------------------------------------------------------------
// Gate 5: Risky code has tests
// ---------------------------------------------------------------------------

function checkRiskyCodeTested(
Expand Down Expand Up @@ -310,14 +372,15 @@ function main(): void {
"--repo",
repo,
"--json",
"number,title,url,files,statusCheckRollup,mergeStateStatus",
"number,title,url,files,statusCheckRollup,mergeStateStatus,headRefOid",
]) as {
number: number;
title: string;
url: string;
files: Array<{ path: string; status: string }>;
statusCheckRollup: StatusCheck[];
mergeStateStatus: string;
headRefOid: string;
} | null;

if (!prData) {
Expand All @@ -329,13 +392,14 @@ function main(): void {
const conflicts = checkConflicts(prData.mergeStateStatus);
const coderabbit = checkCodeRabbit(repo, prNumber);
const riskyCodeTested = checkRiskyCodeTested(prData.files ?? []);
const prAdvisor = checkPrAdvisor(repo, prNumber, prData.headRefOid ?? "");

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));
Expand Down
191 changes: 191 additions & 0 deletions .agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
// 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;
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 {
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.
// 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 =
/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;
}

// ---------------------------------------------------------------------------
// 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)
);
}
6 changes: 4 additions & 2 deletions .agents/skills/nemoclaw-maintainer-day/scripts/triage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading