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
18 changes: 8 additions & 10 deletions .github/workflows/pr-review-advisor.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ jobs:
if: ${{ github.repository == 'NVIDIA/NemoClaw' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'NVIDIA/NemoClaw') }}
runs-on: ubuntu-24.04
timeout-minutes: 40
continue-on-error: ${{ !matrix.advisor.publish_comment }}
strategy:
fail-fast: false
matrix:
Expand All @@ -63,17 +64,13 @@ jobs:
model: openai/openai/gpt-5.5
artifact_dir: pr-review-advisor
artifact_name: pr-review-advisor
comment_marker: "<!-- nemoclaw-pr-review-advisor -->"
comment_title: PR Review Advisor
comment_label: PR review advisor
publish_comment: true
- id: nemotron-ultra
label: Nemotron 3 Ultra
model: nvidia/nvidia/nemotron-3-ultra
artifact_dir: pr-review-advisor-nemotron-ultra
artifact_name: pr-review-advisor-nemotron-ultra
comment_marker: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->"
comment_title: PR Review Advisor (Nemotron Ultra)
comment_label: PR review advisor (Nemotron Ultra)
publish_comment: false
env:
# Pin the Pi SDK to a known-good version. Updates should go through
# normal dependency review so a compromised upstream release cannot run
Expand All @@ -89,10 +86,11 @@ jobs:
# so do not claim that this job waits for required checks to settle.
PR_REVIEW_ADVISOR_MODEL: ${{ matrix.advisor.model }}
PR_REVIEW_ADVISOR_ARTIFACT_DIR: ${{ matrix.advisor.artifact_dir }}
PR_REVIEW_ADVISOR_COMMENT_MARKER: ${{ matrix.advisor.comment_marker }}
PR_REVIEW_ADVISOR_COMMENT_TITLE: ${{ matrix.advisor.comment_title }}
PR_REVIEW_ADVISOR_COMMENT_LABEL: ${{ matrix.advisor.comment_label }}
PR_REVIEW_ADVISOR_COMMENT_MARKER: "<!-- nemoclaw-pr-review-advisor -->"
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: ${{ matrix.advisor.publish_comment }}
# Trusted implementation code is always checked out here. PR content is
# only read as inert analysis data under ADVISOR_WORKDIR.
ADVISOR_DIR: ${{ github.workspace }}/advisor
Expand Down Expand Up @@ -250,7 +248,7 @@ jobs:
fi

- name: Post PR review advisor comment
if: ${{ always() && github.event_name == 'pull_request' }}
if: ${{ always() && github.event_name == 'pull_request' && matrix.advisor.publish_comment }}
continue-on-error: true
env:
GH_TOKEN: ${{ secrets.PR_REVIEW_ADVISOR_GITHUB_TOKEN || github.token }}
Expand Down
5 changes: 2 additions & 3 deletions test/pr-review-advisor-ledger-tools.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ function reviewMetadata(): Parameters<typeof normalizeReviewResult>[1] {
simplificationSignals: [],
workflowSignals: [],
localizedPatchSignals: [],
monolithDeltas: [],
driftEvidence: [],
previousAdvisorReview: null,
github: null,
Expand Down Expand Up @@ -200,7 +199,7 @@ describe("PR review ledger tools", () => {
);

expect(result).toMatchObject({
summary: { confidence: "low", recommendation: "merge_after_fixes" },
summary: { confidence: "low", recommendation: "info_only" },
findings: [{ title: finding().title }],
reviewCompleteness: { requiresHumanReview: true },
});
Expand Down Expand Up @@ -690,7 +689,7 @@ describe("PR review ledger tools", () => {
withCanonicalReviewLedgerFindings(drifted, ledger.snapshot()).findings[0]?.severity,
).toBe("warning");
expect(withCanonicalReviewLedgerFindings(drifted, ledger.snapshot()).summary).toMatchObject({
recommendation: "merge_after_fixes",
recommendation: "merge_as_is",
topItem: "Refusal status is masked",
});
});
Expand Down
11 changes: 6 additions & 5 deletions test/pr-review-advisor-test-depth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe("PR review advisor deterministic test-depth floor", () => {
]);
});

it("keeps the complete floor and model guidance visible within shared caps (#6446)", () => {
it("keeps the complete floor as internal context within shared caps (#6446)", () => {
const deterministicTests = Array.from(
{ length: 13 },
(_value, index) => `Run deterministic E2E job ${index + 1}.`,
Expand All @@ -80,10 +80,11 @@ describe("PR review advisor deterministic test-depth floor", () => {
const summary = renderSummary(result);
const comment = buildComment({ summary, result });

expect(summary).toContain("Run deterministic E2E job 1.");
expect(summary).toContain("Add model-specific regression test 1.");
expect(comment).toContain("Run deterministic E2E job 1.");
expect(comment).toContain("Add model-specific regression test 1.");
expect(summary).not.toContain("Run deterministic E2E job 1.");
expect(summary).not.toContain("Add model-specific regression test 1.");
expect(comment).not.toContain("Run deterministic E2E job 1.");
expect(comment).not.toContain("Add model-specific regression test 1.");
expect(comment).toContain("No blocking advisor findings");
expect(testDepth.suggestedTests).toHaveLength(20);
expect(testDepth.suggestedTests).toEqual(expect.arrayContaining(deterministicTests));
});
Expand Down
1 change: 0 additions & 1 deletion test/pr-review-advisor-turns.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ function metadata(
previousAdvisorReview: null,
workflowSignals: [],
localizedPatchSignals: [],
monolithDeltas: [],
driftEvidence: [],
github: null,
},
Expand Down
103 changes: 97 additions & 6 deletions test/pr-review-advisor-workflow-boundary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,102 @@ fi
expect(validatePrReviewAdvisorWorkflowBoundary()).toEqual([]);
});

it("requires one advisor lane to publish the PR comment", () => {
const source = fs.readFileSync(
path.join(ROOT, ".github/workflows/pr-review-advisor.yaml"),
"utf8",
);
const cases = [
{
workflow: source.replace("publish_comment: true", "publish_comment: false"),
expected: "advisor matrix must publish exactly one PR comment",
},
{
workflow: source.replace("publish_comment: false", "publish_comment: true"),
expected: "advisor matrix must publish exactly one PR comment",
},
{
workflow: source.replace(" publish_comment: false\n", ""),
expected: "advisor matrix entry 2 missing boolean publish_comment",
},
];

for (const { workflow, expected } of cases) {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-publisher-"));
const workflowPath = path.join(tmp, "workflow.yaml");
fs.writeFileSync(workflowPath, workflow);
try {
expect(validatePrReviewAdvisorWorkflowBoundary(workflowPath)).toContain(expected);
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}
}
});

it("keeps comment publication gated to the publishing advisor lane", () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-publisher-"));
const workflowPath = path.join(tmp, "workflow.yaml");
const workflow = fs
.readFileSync(path.join(ROOT, ".github/workflows/pr-review-advisor.yaml"), "utf8")
.replace(" && matrix.advisor.publish_comment }}", " }}");
fs.writeFileSync(workflowPath, workflow);

try {
expect(validatePrReviewAdvisorWorkflowBoundary(workflowPath)).toContain(
"Post PR review advisor comment must run only for the publishing advisor lane",
);
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}
});

it("keeps failures non-blocking only for non-publishing advisor lanes", () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-publisher-"));
const workflowPath = path.join(tmp, "workflow.yaml");
const workflow = fs
.readFileSync(path.join(ROOT, ".github/workflows/pr-review-advisor.yaml"), "utf8")
.replace(
"continue-on-error: ${{ !matrix.advisor.publish_comment }}",
"continue-on-error: false",
);
fs.writeFileSync(workflowPath, workflow);

try {
expect(validatePrReviewAdvisorWorkflowBoundary(workflowPath)).toContain(
"review job failures must be non-blocking only for non-publishing advisor lanes",
);
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}
});

it("pins previous-review context to the publishing workflow", () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-publisher-"));
const workflowPath = path.join(tmp, "workflow.yaml");
const workflow = fs
.readFileSync(path.join(ROOT, ".github/workflows/pr-review-advisor.yaml"), "utf8")
.replace(
"PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: ${{ matrix.advisor.publish_comment }}",
"PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: true",
)
.replace(
'PR_REVIEW_ADVISOR_WORKFLOW_NAME: "PR Review / Advisor"',
'PR_REVIEW_ADVISOR_WORKFLOW_NAME: "Other Workflow"',
);
fs.writeFileSync(workflowPath, workflow);

try {
expect(validatePrReviewAdvisorWorkflowBoundary(workflowPath)).toEqual(
expect.arrayContaining([
"review job env.PR_REVIEW_ADVISOR_WORKFLOW_NAME must be PR Review / Advisor",
"review job env.PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW must be ${{ matrix.advisor.publish_comment }}",
]),
);
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}
});

it("rejects a workflow that masks an incomplete advisor analysis", () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-outcome-"));
const workflowPath = path.join(tmp, "workflow.yaml");
Expand Down Expand Up @@ -241,10 +337,6 @@ fi
const workflowPath = path.join(tmp, "workflow.yaml");
const workflow = fs
.readFileSync(path.join(ROOT, ".github", "workflows", "pr-review-advisor.yaml"), "utf-8")
.replace(
'comment_marker: "<!-- nemoclaw-pr-review-advisor-nemotron-ultra -->"',
'comment_marker: "<!-- nemoclaw-pr-review-advisor -->"',
)
.replace("artifact_dir: pr-review-advisor-nemotron-ultra", "artifact_dir: pr-review-advisor")
.replace(
"artifact_name: pr-review-advisor-nemotron-ultra",
Expand All @@ -261,7 +353,6 @@ fi
"advisor matrix field model must be unique: openai/openai/gpt-5.5",
"advisor matrix field artifact_dir must be unique: pr-review-advisor",
"advisor matrix field artifact_name must be unique: pr-review-advisor",
"advisor matrix field comment_marker must be unique: <!-- nemoclaw-pr-review-advisor -->",
"step 'Post PR review advisor comment' run script must include --title \"$PR_REVIEW_ADVISOR_COMMENT_TITLE\"",
]),
);
Expand Down Expand Up @@ -314,7 +405,7 @@ jobs:
"workflow must run on pull_request, not only trusted-target events",
"workflow must not run untrusted PR code under pull_request_target",
"workflow permissions.contents must be read",
"review job must not be globally continue-on-error",
"review job failures must be non-blocking only for non-publishing advisor lanes",
"PR checkout must use the pull request head SHA as inert analysis data",
"Run PR review advisor must receive PR_REVIEW_ADVISOR_API_KEY only from secrets.PR_REVIEW_ADVISOR_API_KEY",
"Run PR review advisor must not receive OPENAI_API_KEY",
Expand Down
Loading
Loading