Skip to content

refactor(advisor): remove rollout compatibility #9645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cv merged 7 commits into from
Aug 20, 2026
Merged

refactor(advisor): remove rollout compatibility #9645

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b4f8e71
refactor(advisor): remove rollout compatibility
cv Aug 19, 2026
ebb44fd
Merge branch 'main' into codex/pr-advisor-post-rollout-cleanup
cv Aug 19, 2026
cd44271
fix(advisor): tighten review evidence contracts
cv Aug 19, 2026
79275de
merge: resolve conflicts with main
github-actions[bot] Aug 19, 2026
1c556ed
merge: resolve conflicts with main
github-actions[bot] Aug 19, 2026
014d4c6
fix(advisor): restore architecture finding guidance
cv Aug 19, 2026
d02f774
Merge branch 'main' into codex/pr-advisor-post-rollout-cleanup
cv Aug 20, 2026
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
1 change: 0 additions & 1 deletion .github/workflows/pr-review-advisor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ jobs:
PR_REVIEW_ADVISOR_COMMENT_TITLE: PR Review Advisor
PR_REVIEW_ADVISOR_COMMENT_LABEL: PR review advisor
PR_REVIEW_ADVISOR_WORKFLOW_NAME: "PR Review / Advisor"
PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: "false"
SANDBOX_NAME: ${{ matrix.advisor.sandbox_name }}
# Only executable code from this checkout may run in the analysis job.
ADVISOR_DIR: ${{ github.workspace }}/advisor
Expand Down
9 changes: 4 additions & 5 deletions test/code-change-considerations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ import { tmpdir } from "node:os";
import path from "node:path";
import { afterEach, describe, expect, it, vi } from "vitest";

import { preparePromptArtifacts } from "../tools/pr-review-advisor/analyze.mts";
import { artifactPaths } from "../tools/pr-review-advisor/artifacts.mts";
import {
artifactPaths,
buildSystemPrompt,
preparePromptArtifacts,
readTrustedCodeChangeConsiderations,
} from "../tools/pr-review-advisor/analyze.mts";
import { loadAdvisorSchema, metadata } from "./helpers/pr-review-advisor-test-fixtures";
} from "../tools/pr-review-advisor/trusted-guidance.mts";
import { metadata } from "./helpers/pr-review-advisor-test-fixtures";

const ROOT = path.resolve(import.meta.dirname, "..");
const RESOURCE_PATH = path.join(
Expand Down Expand Up @@ -112,7 +112,6 @@ describe("shared code change considerations", () => {
artifacts: artifactPaths(outDir),
metadata: reviewMetadata,
diff: "",
schema: loadAdvisorSchema(),
}),
).toThrow("Code change considerations malformed");
expect(
Expand Down
27 changes: 16 additions & 11 deletions test/helpers/pr-review-advisor-test-fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ import fs from "node:fs";
import path from "node:path";

import { buildRiskPlan } from "../../tools/advisors/risk-plan.mts";
import { normalizeReviewResult } from "../../tools/pr-review-advisor/analyze.mts";
import type { ReviewAdvisorResult, ReviewMetadata } from "../../tools/pr-review-advisor/analyze.mts";

export const ROOT = path.resolve(import.meta.dirname, "../..");

export type ReviewMetadata = Parameters<typeof normalizeReviewResult>[1];

export function metadata(overrides: Partial<ReviewMetadata> = {}): ReviewMetadata {
const deterministic = {
diffStat: "1 file changed",
Expand All @@ -28,8 +26,7 @@ export function metadata(overrides: Partial<ReviewMetadata> = {}): ReviewMetadat
candidateExistingCoverage: [],
},
simplificationSignals: [],
previousAdvisorReview: null,
workflowSignals: [],
workflowSignals: [],
localizedPatchSignals: [],
driftEvidence: [],
github: null,
Expand All @@ -49,13 +46,15 @@ export function loadAdvisorSchema(): Record<string, unknown> {
return JSON.parse(fs.readFileSync(schemaPath, "utf-8")) as Record<string, unknown>;
}

export function validResult(overrides = {}) {
export function validResult(
overrides: Record<string, unknown> = {},
): ReviewAdvisorResult {
return {
version: 1,
baseRef: "wrong",
headRef: "wrong",
headSha: "wrong",
changedFiles: [],
baseRef: "origin/main",
headRef: "HEAD",
headSha: "abc123def456",
changedFiles: ["tools/pr-review-advisor/analyze.mts"],
summary: {
recommendation: "merge_after_fixes",
confidence: "high",
Expand All @@ -77,6 +76,11 @@ export function validResult(overrides = {}) {
evidence: "advisor scripts are invoked from ADVISOR_DIR",
},
],
terminologyReview: {
status: "clear",
decisions: [],
noChangesReason: "No semantic terminology candidates were selected.",
},
acceptanceCoverage: [
{
clause: "post a sticky advisory comment",
Expand Down Expand Up @@ -115,6 +119,7 @@ export function validResult(overrides = {}) {
},
targets: {
relevantChangedFiles: [],
changedCredentialFreeTests: [],
required: [],
optional: [],
noTargetE2eReason: "No E2E target impact.",
Expand All @@ -132,5 +137,5 @@ export function validResult(overrides = {}) {
requiresHumanReview: true,
},
...overrides,
};
} as ReviewAdvisorResult;
}
186 changes: 4 additions & 182 deletions test/pr-review-advisor-context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,15 @@ import { tmpdir } from "node:os";
import path from "node:path";
import { afterEach, describe, expect, it, vi } from "vitest";
import { githubGraphql, upsertStickyComment } from "../tools/advisors/github.mts";
import { buildPromptTurns } from "../tools/pr-review-advisor/analyze.mts";
import {
buildPromptTurns,
buildSystemPrompt,
classifyTestDepth,
collectStaticTestInventory,
collectTrustedPreviousAdvisorReview,
detectLocalizedPatchSignals,
detectSimplificationSignals,
extractIssueRefs,
extractPreviousAdvisorReview,
} from "../tools/pr-review-advisor/analyze.mts";
} from "../tools/pr-review-advisor/deterministic-context.mts";
import { extractIssueRefs } from "../tools/pr-review-advisor/github-context.mts";
import { buildSystemPrompt } from "../tools/pr-review-advisor/trusted-guidance.mts";
import { loadAdvisorSchema, metadata, ROOT } from "./helpers/pr-review-advisor-test-fixtures.ts";

describe("PR review advisor", () => {
Expand Down Expand Up @@ -79,7 +77,6 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
});

it("materializes the two-turn PR review contract (#6446)", () => {
const schema = loadAdvisorSchema();
const reviewMetadata = metadata();
reviewMetadata.deterministic.github = {
repo: "NVIDIA/NemoClaw",
Expand All @@ -93,7 +90,6 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
const turns = buildPromptTurns({
metadata: reviewMetadata,
diff: poisonedDiff,
schema,
});

expect(turns).toHaveLength(2);
Expand Down Expand Up @@ -308,180 +304,6 @@ diff --git a/test/example.test.ts b/test/example.test.ts
expect(signals[0]?.reviewRule).toContain("invalid state");
});

it("parses previous advisor metadata from trusted hidden sticky-comment fields", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\nbody",
},
],
new Set(["1"]),
);

expect(previous).toMatchObject({ headSha: "abc1234" });
});

it("keeps parallel advisor previous-review markers isolated", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ndefault",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->\n<!-- head_sha: def5678; recommendation: merge_after_fixes; run_id: 100; run_attempt: 1; comment_id: 2 -->\nnemotron",
},
],
new Set(["1", "2"]),
{ marker: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->" },
);

expect(previous).toMatchObject({
headSha: "def5678",
body: expect.stringContaining("nemotron"),
});
});

it("validates parallel advisor previous-review provenance with marker isolation", async () => {
vi.spyOn(globalThis, "fetch").mockImplementation(async (input: unknown) => {
const url = String(input);
const runId = url.split("/").at(-1);
return {
ok: true,
json: async () => ({
name: "PR Review / Advisor",
path: ".github/workflows/pr-review-advisor.yaml",
head_sha: runId === "100" ? "def5678" : "abc1234",
event: "pull_request",
run_attempt: 1,
run_started_at: "2026-01-01T00:00:00Z",
updated_at: "2026-01-01T00:10:00Z",
}),
} as Response;
});

const previous = await collectTrustedPreviousAdvisorReview(
"NVIDIA/NemoClaw",
"token",
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ndefault",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->\n<!-- head_sha: def5678; recommendation: merge_after_fixes; run_id: 100; run_attempt: 1; comment_id: 2 -->\nnemotron",
},
],
{ marker: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->" },
);

expect(previous).toMatchObject({
headSha: "def5678",
body: expect.stringContaining("nemotron"),
});
});

it("ignores spoofed previous advisor comments from untrusted authors", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ntrusted",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "random-user" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: deadbeef; recommendation: merge_after_fixes; run_id: 100; run_attempt: 1; comment_id: 2 -->\nspoof",
},
],
new Set(["1", "2"]),
);

expect(previous).toMatchObject({ headSha: "abc1234" });
});

it("ignores bot-authored marker comments without complete hidden advisor metadata", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ntrusted",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: deadbeef -->\nlegacy bot marker without complete hidden metadata",
},
],
new Set(["1", "2"]),
);

expect(previous).toMatchObject({ headSha: "abc1234" });
});

it("ignores complete bot-authored marker collisions without trusted run provenance", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ntrusted",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: deadbeef; recommendation: merge_after_fixes; run_id: 100; run_attempt: 1; comment_id: 2 -->\nspoof",
},
],
new Set(["1"]),
);

expect(previous).toMatchObject({ headSha: "abc1234" });
});

it("ignores bot-authored marker replays with copied trusted metadata", () => {
const previous = extractPreviousAdvisorReview(
[
{
id: 1,
updated_at: "2026-01-01T00:05:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\ntrusted",
},
{
id: 2,
updated_at: "2026-01-01T00:06:00Z",
user: { login: "github-actions[bot]" },
body: "<!-- nemoclaw-pr-review-advisor -->\n<!-- head_sha: abc1234; recommendation: merge_after_fixes; run_id: 99; run_attempt: 1; comment_id: 1 -->\nreplay",
},
],
new Set(["1"]),
);

expect(previous).toMatchObject({ body: expect.stringContaining("trusted") });
});

it("upserts sticky comments with created comment-scoped bodies", async () => {
const fetchMock = vi
.spyOn(globalThis, "fetch")
Expand Down
Loading
Loading