diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md index 74f296a3d8d..8eff0b342ba 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md @@ -65,9 +65,10 @@ Parses each PR body for `supersedes #N`, `replaces #N`, `closes in favor of #N`, ```bash scripts/collect-gates.sh scripts/check-coderabbit-threads.sh +node --experimental-strip-types --no-warnings ../nemoclaw-maintainer-day/scripts/check-gates.ts ``` -Six gates, all mandatory. See `checks/tier-0-gates.md` for the full list and interpretation. +Seven gates, all mandatory. Use the shared gate result's PR Advisor recommendation for the Advisor gate: it is `true` only when the trusted exact-head result both passes and reports `merge_as_is`; a missing recommendation is `false`. See `checks/tier-0-gates.md` for the full list and interpretation. ### Step 5: Tier 1 correctness @@ -79,6 +80,9 @@ Three checks, all LLM judgments. See `checks/tier-2-quality.md`. ### Step 7: Weighted score +- Build the Tier 0 eligibility set from exactly these seven boolean keys: `state_open`, `ci_green_latest_sha`, `mergeable`, `contributor_compliance`, `branch_protection`, `pr_advisor_merge_as_is`, and `coderabbit_threads_resolved`. +- Stop if any candidate omits a key, adds an unknown key, or supplies a non-boolean value. +- Only PRs for which all seven gates are `true` enter happy-path scoring. - Each pass = full points - Each yellow = half points - Each fail = zero @@ -87,11 +91,11 @@ Three checks, all LLM judgments. See `checks/tier-2-quality.md`. ### Step 8: Tier 3 ranking -Branch on whether any PR passes all Tier 0 gates. See `tiebreakers.md` for happy-path tiebreakers, degraded-mode distance-to-ready ranking, and the behavior-coverage matrix. +Derive the mode from the Tier 0 eligibility set rather than accepting a caller-provided mode. In happy mode, set `winner` only to a PR in that set and leave `closest_to_ready` null. In degraded mode, leave `winner` null and use `closest_to_ready` only for an open, contributor-compliant salvage candidate. See `tiebreakers.md` for happy-path tiebreakers, degraded-mode distance-to-ready ranking, and the behavior-coverage matrix. ### Step 9: Emit verdict -Use `templates/verdict.md`. Every judgment must carry evidence (file:line refs, diff snippets), reasoning chain, and the score it contributed. +Use `templates/verdict.md`. Pass the generated spec through `scripts/render-verdict.py`; do not render or recommend a merge if it exits nonzero. The renderer independently validates the seven gates, recomputes eligibility and mode, and rejects a `winner` outside the eligible set. Every judgment must carry evidence (file:line refs, diff snippets), reasoning chain, and the score it contributed. ## Reference files @@ -108,6 +112,7 @@ Use `templates/verdict.md`. Every judgment must carry evidence (file:line refs, - `scripts/find-candidates.sh` — PR discovery - `scripts/collect-gates.sh` — Tier 0 gate evaluation - `scripts/check-coderabbit-threads.sh` — GraphQL thread resolution +- `../nemoclaw-maintainer-day/scripts/check-gates.ts` — trusted PR Review Advisor result - `scripts/parse-supersession.sh` — body parsing for supersession refs - `scripts/render-verdict.py` — verdict scorecard renderer diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md index afa062dd7a1..1761834e0d3 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md @@ -3,7 +3,7 @@ # Tier 0 — Plumbing Gates -Mandatory prerequisites. Any gate failure means the PR cannot be merged in its current state. Six gates total. Run `scripts/collect-gates.sh ` to evaluate gates 1-5 mechanically; run `scripts/check-coderabbit-threads.sh ` for gate 6. +Mandatory prerequisites. Any gate failure means the PR cannot be merged in its current state. Seven gates total. Run `scripts/collect-gates.sh ` to evaluate gates 1-5 mechanically, use the shared maintainer-day gate result for gate 6, and run `scripts/check-coderabbit-threads.sh ` for gate 7. ## Contents @@ -12,7 +12,8 @@ Mandatory prerequisites. Any gate failure means the PR cannot be merged in its c - Gate 3: Mergeable, no conflicts - Gate 4: Contributor compliance satisfied - Gate 5: Branch protection satisfied -- Gate 6: Automated reviewer threads resolved +- Gate 6: PR Review Advisor recommends merge as-is +- Gate 7: Automated reviewer threads resolved ## Gate 1: PR state OPEN @@ -52,7 +53,13 @@ The PR body must include a valid contributor `Signed-off-by:` declaration, and e **Why defer:** Branch protection rules are the source of truth. Re-implementing the check in the skill would drift from repo policy. If your repo doesn't enforce CODEOWNERS via branch protection, set `codeowners_enforced_via_branch_protection: false` in `repo-policy.md` and add explicit team checks. -## Gate 6: Automated reviewer threads resolved +## Gate 6: PR Review Advisor recommends merge as-is + +The trusted, exact-head PR Review Advisor result must report `recommendation: merge_as_is`. Use `.gates.prAdvisor` from `nemoclaw-maintainer-day/scripts/check-gates.ts`, and set `pr_advisor_merge_as_is` to `true` only when both `pass` is `true` and `recommendation` is exactly `merge_as_is`. + +**Fail closed:** Missing, skipped, stale, malformed, untrusted, or non-`merge_as_is` Advisor results fail this gate. A successful CodeRabbit review or human approval does not substitute for the Advisor result. + +## Gate 7: Automated reviewer threads resolved All threads created by automated reviewers (e.g., CodeRabbit) must be in `resolved: true` state. **Zero unresolved threads is the bar.** diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh index 75897087afd..c1094194177 100755 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh @@ -4,7 +4,8 @@ # # Collect Tier 0 gate state for a PR and emit JSON for downstream scoring. # Covers gates 1-5 (state, CI on latest SHA, mergeable, contributor compliance, -# branch protection). Gate 6 (CodeRabbit threads) is handled by +# branch protection). Gate 6 (PR Review Advisor) comes from the shared +# maintainer-day gate result, and gate 7 (CodeRabbit threads) is handled by # check-coderabbit-threads.sh. # # Usage: collect-gates.sh [--repo OWNER/REPO] diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py index 5e3d303a5e2..c445d633416 100755 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py @@ -26,7 +26,8 @@ "supersession_edges": [{"superseder": 2851, "superseded": 2693}], "tiebreaker_fired": "smaller_diff", "winner": 2851, - "mode": "happy" // or "degraded" + "closest_to_ready": null, + "mode": "happy" // optional assertion; derived from Tier 0 gates } Usage: @@ -45,6 +46,94 @@ # Tier 2 weight per check (each pass = 1 point, yellow = 0.5, fail = 0). TIER_2_WEIGHT = 1.0 +TIER_0_GATES = ( + ("state_open", "State open"), + ("ci_green_latest_sha", "CI green on latest SHA"), + ("mergeable", "Mergeable"), + ("contributor_compliance", "Contributor compliance"), + ("branch_protection", "Branch protection"), + ("pr_advisor_merge_as_is", "PR Review Advisor: merge as-is"), + ("coderabbit_threads_resolved", "Automated-review threads resolved"), +) +TIER_0_KEYS = tuple(key for key, _label in TIER_0_GATES) +INVALID_SPEC_EXIT = 64 + + +class SpecValidationError(ValueError): + """Raised when a verdict spec cannot produce a safe recommendation.""" + + +def validate_spec(spec: Any) -> tuple[str, int | None]: + """Validate untrusted renderer input and derive the verdict mode.""" + if not isinstance(spec, dict): + raise SpecValidationError("top-level value must be an object") + + prs = spec.get("prs") + if not isinstance(prs, list) or not prs: + raise SpecValidationError("prs must be a non-empty array") + + pr_numbers: set[int] = set() + eligible_numbers: set[int] = set() + salvageable_numbers: set[int] = set() + for index, pr in enumerate(prs): + if not isinstance(pr, dict): + raise SpecValidationError(f"prs[{index}] must be an object") + number = pr.get("number") + if type(number) is not int: + raise SpecValidationError(f"prs[{index}].number must be an integer") + if number in pr_numbers: + raise SpecValidationError(f"duplicate PR number: {number}") + pr_numbers.add(number) + + gates = pr.get("tier_0") + if not isinstance(gates, dict): + raise SpecValidationError(f"PR #{number} tier_0 must be an object") + missing = [key for key in TIER_0_KEYS if key not in gates] + extra = sorted(set(gates) - set(TIER_0_KEYS)) + if missing: + raise SpecValidationError( + f"PR #{number} tier_0 is missing required gates: {', '.join(missing)}" + ) + if extra: + raise SpecValidationError(f"PR #{number} tier_0 has unknown gates: {', '.join(extra)}") + non_boolean = [key for key in TIER_0_KEYS if type(gates[key]) is not bool] + if non_boolean: + raise SpecValidationError( + f"PR #{number} tier_0 gates must be boolean: {', '.join(non_boolean)}" + ) + + if all(gates[key] for key in TIER_0_KEYS): + eligible_numbers.add(number) + if gates["state_open"] and gates["contributor_compliance"]: + salvageable_numbers.add(number) + + mode = "happy" if eligible_numbers else "degraded" + winner = spec.get("winner") + if winner is not None: + if type(winner) is not int or winner not in pr_numbers: + raise SpecValidationError("winner must reference a candidate PR number") + if winner not in eligible_numbers: + raise SpecValidationError(f"winner PR #{winner} did not pass every Tier 0 gate") + + closest_to_ready = spec.get("closest_to_ready") + if closest_to_ready is not None: + if type(closest_to_ready) is not int or closest_to_ready not in pr_numbers: + raise SpecValidationError("closest_to_ready must reference a candidate PR number") + if mode != "degraded": + raise SpecValidationError("closest_to_ready is only valid in degraded mode") + if closest_to_ready not in salvageable_numbers: + raise SpecValidationError( + f"closest_to_ready PR #{closest_to_ready} must be open and contributor-compliant" + ) + + supplied_mode = spec.get("mode") + if supplied_mode is not None and supplied_mode != mode: + raise SpecValidationError( + f"supplied mode {supplied_mode!r} contradicts derived mode {mode!r}" + ) + + return mode, closest_to_ready + def status_emoji(status: str) -> str: """Map a check status to a short label. Matches templates/verdict.md.""" @@ -77,18 +166,10 @@ def render_scorecard(prs: list[dict[str, Any]]) -> str: # Tier 0 rows.append(["**Tier 0 — gates**"] + [""] * len(prs)) - tier_0_keys = [ - "state_open", - "ci_green_latest_sha", - "mergeable", - "branch_protection", - "coderabbit_threads_resolved", - ] - for key in tier_0_keys: - label = key.replace("_", " ").capitalize() + for key, label in TIER_0_GATES: row = [label] for pr in prs: - row.append(status_emoji(pr.get("tier_0", {}).get(key, "fail"))) + row.append(status_emoji(pr["tier_0"][key])) rows.append(row) # Tier 1 @@ -176,13 +257,18 @@ def main() -> int: spec = json.load(sys.stdin) except json.JSONDecodeError as e: print(f"Invalid JSON spec on stdin: {e}", file=sys.stderr) - return 64 + return INVALID_SPEC_EXIT + + try: + mode, closest_to_ready = validate_spec(spec) + except SpecValidationError as e: + print(f"Invalid verdict spec: {e}", file=sys.stderr) + return INVALID_SPEC_EXIT issue = spec["issue"] criteria = spec.get("criteria", []) prs = spec.get("prs", []) winner = spec.get("winner") - mode = spec.get("mode", "happy") tiebreaker = spec.get("tiebreaker_fired") supersession = spec.get("supersession_edges", []) @@ -209,10 +295,10 @@ def main() -> int: print(f"\n### Verdict: MERGE PR #{winner}\n") else: print("\n### Verdict: Neither mergeable yet\n") - if winner is not None: - print(f"PR #{winner} is closer to ready.\n") + if closest_to_ready is not None: + print(f"PR #{closest_to_ready} is closer to ready.\n") else: - print("No PR is meaningfully closer; both need substantial salvage.\n") + print("No open, contributor-compliant PR is eligible for salvage.\n") print("Reasoning trace:") if supersession: diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md index 19bb4a8969e..667ade77fe3 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md @@ -21,8 +21,10 @@ Render the final scorecard with `scripts/render-verdict.py`. Below is the human- | State OPEN | pass | pass | | CI green (latest SHA) | pass | fail (stale) | | Mergeable | pass | pass | +| Contributor compliance | pass | pass | | Branch protection | pass | pass | -| CodeRabbit threads | pass | yellow (2 unresolved) | +| PR Review Advisor: merge as-is | pass | fail (changes requested) | +| Automated-review threads resolved | pass | fail (2 unresolved) | | **Tier 1 — correctness** | | | | Test exercises bug path | pass | pass | | Comment-as-spec coverage | pass | yellow (misses ask 3) | diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md index 9ac0847e805..1732c8bf207 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md @@ -15,6 +15,8 @@ Final decision logic. Two paths: happy mode when at least one PR passes all Tier Eliminate any PR failing Tier 0. Among survivors: +- Set `winner` only to a survivor and leave `closest_to_ready` null. + 1. Compute weighted score across Tiers 1-2. 2. Build the **behavior-coverage matrix** (see below). If matrix has a clear winner per criterion, that wins. 3. Apply tiebreakers in order. First tiebreaker that distinguishes the PRs picks the winner. @@ -43,6 +45,8 @@ Don't give up — pick the closest-to-ready and recommend salvage steps. - Tie → fewer trivial failures wins - Tie → higher Tier 1-2 weighted score wins (correctness beneath the broken plumbing) 3. Output: + - Leave `winner` null; it is reserved for a fully eligible merge recommendation + - Set `closest_to_ready` only to an open PR that passes contributor compliance; leave it null for a rejection-only verdict - Per-PR Tier 0 failure list - Per-PR Tier 1-2 scorecard (so the winner has objective merit beneath the gates) - Verdict: "Neither mergeable yet. PR A is closer — fix [substantive list]. PR B has [issues]." diff --git a/test/pr-comparator-render-verdict.test.ts b/test/pr-comparator-render-verdict.test.ts new file mode 100644 index 00000000000..eb034736e97 --- /dev/null +++ b/test/pr-comparator-render-verdict.test.ts @@ -0,0 +1,147 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +const renderer = path.join( + process.cwd(), + ".agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py", +); + +const passingGates = { + state_open: true, + ci_green_latest_sha: true, + mergeable: true, + contributor_compliance: true, + branch_protection: true, + pr_advisor_merge_as_is: true, + coderabbit_threads_resolved: true, +}; + +function specFor(tier0: Record, overrides: Record = {}) { + return { + issue: 9999, + criteria: [], + prs: [ + { + number: 123, + title: "candidate", + tier_0: tier0, + tier_1: {}, + tier_2: {}, + }, + ], + winner: null, + closest_to_ready: null, + ...overrides, + }; +} + +function render(spec: unknown) { + return spawnSync("python3", [renderer], { + cwd: process.cwd(), + encoding: "utf8", + input: JSON.stringify(spec), + }); +} + +describe("PR comparator verdict renderer", () => { + it("renders contributor compliance and merges only an eligible winner", () => { + const result = render(specFor(passingGates, { mode: "happy", winner: 123 })); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + expect(result.stdout).toContain("| Contributor compliance | pass |"); + expect(result.stdout).toContain("### Verdict: MERGE PR #123"); + }); + + it("rejects a supplied winner if contributor compliance failed", () => { + const result = render( + specFor({ ...passingGates, contributor_compliance: false }, { mode: "happy", winner: 123 }), + ); + + expect(result.status).toBe(64); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("winner PR #123 did not pass every Tier 0 gate"); + }); + + it("rejects a supplied winner unless the PR Advisor recommends merge as-is", () => { + const result = render( + specFor({ ...passingGates, pr_advisor_merge_as_is: false }, { mode: "happy", winner: 123 }), + ); + + expect(result.status).toBe(64); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("winner PR #123 did not pass every Tier 0 gate"); + }); + + it.each([ + ["missing", (({ pr_advisor_merge_as_is: _omitted, ...gates }) => gates)(passingGates)], + ["non-boolean", { ...passingGates, pr_advisor_merge_as_is: "merge_as_is" }], + ["unknown", { ...passingGates, invented_gate: true }], + ])("rejects %s Tier 0 gate data", (_label, gates) => { + const result = render(specFor(gates)); + + expect(result.status).toBe(64); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("Invalid verdict spec"); + }); + + it("rejects a supplied mode that contradicts derived eligibility", () => { + const result = render( + specFor( + { ...passingGates, ci_green_latest_sha: false }, + { mode: "happy", closest_to_ready: 123 }, + ), + ); + + expect(result.status).toBe(64); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("contradicts derived mode 'degraded'"); + }); + + it("uses closest_to_ready for an eligible degraded-mode salvage candidate", () => { + const result = render( + specFor( + { ...passingGates, ci_green_latest_sha: false }, + { mode: "degraded", closest_to_ready: 123 }, + ), + ); + + expect(result.status).toBe(0); + expect(result.stdout).toContain("### Verdict: Neither mergeable yet"); + expect(result.stdout).toContain("PR #123 is closer to ready."); + expect(result.stdout).not.toContain("MERGE PR"); + }); + + it("rejects a noncompliant degraded-mode salvage candidate", () => { + const result = render( + specFor( + { ...passingGates, contributor_compliance: false }, + { mode: "degraded", closest_to_ready: 123 }, + ), + ); + + expect(result.status).toBe(64); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("must be open and contributor-compliant"); + }); + + it("keeps the generation instructions aligned with renderer eligibility", () => { + const skill = fs.readFileSync( + path.join(process.cwd(), ".agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md"), + "utf8", + ); + + for (const gate of Object.keys(passingGates)) { + expect(skill).toContain(`\`${gate}\``); + } + expect(skill).toContain("set `winner` only to a PR in that set"); + expect(skill).toContain("use `closest_to_ready` only for an open, contributor-compliant"); + expect(skill).toContain("do not render or recommend a merge if it exits nonzero"); + }); +});