Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions test/pr-review-advisor-comment-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,8 @@ describe("PR review advisor comment CLI", () => {
expect(comment).toContain("severity counts match");
expect(comment).not.toContain("do not publish this summary");
expect(comment).not.toContain("do not publish this finding");
expect(comment).not.toContain("Why no E2E coverage is recommended");
expect(comment).not.toContain("Why no selector is recommended");
expect(comment).toContain("<summary>1 optional E2E recommendation</summary>");
expect(comment.match(/<code>docs-validation<\/code>/gu)).toHaveLength(1);

const partialComment = buildComment({
summary: "# ignored\n",
Expand Down
2 changes: 1 addition & 1 deletion test/pr-review-advisor-security-boundaries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ describe("PR review advisor security boundaries", () => {

const comment = buildComment({ summary: renderSummary(result), result });
expect(comment).toContain("<code>publisher-exact-head-proof</code>");
expect(comment).toContain("Selected as a trusted exact-head credential-free E2E job.");
expect(comment.match(/<code>publisher-exact-head-proof<\/code>/gu)).toHaveLength(1);
});

it("drops malformed or mismatched exact-head selector evidence", () => {
Expand Down
39 changes: 14 additions & 25 deletions test/pr-review-advisor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,17 +274,12 @@ describe("PR review advisor", () => {

const comment = buildComment({ summary: renderSummary(result), result });
expect(comment).toContain("### E2E guidance");
expect(comment).toContain(
"Advisory only: coverage and selector recommendations are non-authoritative",
);
expect(comment).toContain(
"E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output",
);
expect(comment).toContain("Advisory only. E2E / PR Gate selects and runs jobs independently.");
expect(comment).toContain("<code>upgrade-stale-sandbox</code>");
expect(comment).toContain("**Recommended coverage:**");
expect(comment).toContain("**Recommended selectors:**");
expect(comment).not.toContain("**Required coverage:**");
expect(comment).not.toContain("**Required selectors:**");
expect(comment).toContain("**Recommended E2E:**");
expect(comment.match(/<code>upgrade-stale-sandbox<\/code>/gu)).toHaveLength(1);
expect(comment).not.toContain("Recommended coverage");
expect(comment).not.toContain("Recommended selectors");
expect(comment).not.toContain("rm -rf");
});

Expand Down Expand Up @@ -323,7 +318,7 @@ describe("PR review advisor", () => {
expect([required.coverage.optionalTests, required.targets.optional]).toEqual([[], []]);
});

it("renders the reasons for recommended E2E coverage", () => {
it("renders each E2E recommendation once", () => {
const result = normalizeReviewResult(
validResult({
e2e: {
Expand Down Expand Up @@ -353,26 +348,16 @@ describe("PR review advisor", () => {
);

const comment = buildComment({ summary: renderSummary(result), result });
expect(comment).toContain("**Recommended coverage:** <code>security-posture</code>");
expect(comment).toContain(
"- <code>security-posture</code> — Selected from the trusted checked-in E2E coverage inventory.",
);
expect(comment).toContain("**Recommended selectors:** <code>security-posture</code>");
expect(comment).not.toContain("Why no selector is recommended");
expect(comment).toContain("**Recommended E2E:** <code>security-posture</code>");
expect(comment.match(/<code>security-posture<\/code>/gu)).toHaveLength(1);

const noE2eResult = normalizeReviewResult(validResult(), metadata());
const noE2eComment = buildComment({
summary: renderSummary(noE2eResult),
result: noE2eResult,
});
expect(noE2eComment).toContain(
"**Why no E2E coverage is recommended:** No deterministic or trusted-inventory E2E coverage was selected.",
);
expect(noE2eComment).toContain(
"**Why no selector is recommended:** No trusted E2E selector was selected.",
);
expect(noE2eComment).not.toContain("Why no E2E is required");
expect(noE2eComment).not.toContain("Why no selector is required");
expect(noE2eComment).toContain("**Recommended E2E:** _None_");
expect(noE2eComment).not.toContain("Why no");
});

it("sanitizes malformed enum values and preserves deterministic fallback gates", () => {
Expand Down Expand Up @@ -1128,6 +1113,10 @@ diff --git a/test/example.test.ts b/test/example.test.ts
expect(summary).toContain("Blocking findings for maintainer adjudication");
expect(summary).toContain("## Warnings");
expect(summary).toContain("## Suggestions (optional)");
expect(summary).toContain("## Recommended E2E");
expect(summary).toContain("## Optional E2E");
expect(summary).not.toContain("E2E coverage");
expect(summary).not.toContain("E2E selectors");
expect(summary).not.toContain("Test follow-ups");
expect(summary).not.toContain("comment builder test");
expect(summary).not.toContain("🛠️");
Expand Down
13 changes: 7 additions & 6 deletions tools/pr-review-advisor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ visibly incomplete, but their final-result artifact preserves any open canonical
before the failure so later runs and reviewers do not lose substantive review history.

The workflow is advisory and must not be configured as an E2E-required status check. Its combined
comment includes deterministic-plan-backed E2E guidance and trusted-code-authored reasons for recommended coverage, but
does not dispatch or report pass/fail for E2E jobs. Model availability must not become the authority
comment lists trusted E2E recommendations, but does not dispatch or report pass/fail for E2E jobs.
Model availability must not become the authority
for whether a pull request can merge.
For PRs from this repository, the PR E2E controller separately rebuilds the plan from GitHub's
changed-file list and dispatches every selected job after `CI / Pull Request` completes. `E2E / PR
Expand All @@ -82,7 +82,7 @@ Authors and coding agents should follow the shared [PR CI and Automated Review F
- The analysis job is limited to `NVIDIA/NemoClaw`, has read-only GitHub permissions, and is the only job that receives the model secret.
- The analyzer collects deterministic GitHub context before model work, then removes GitHub tokens from the process environment.
After registering the model credential in the in-memory SDK auth store, it also removes that credential from the process environment before model turns begin.
- The separate publisher has pull-request write permission, but receives neither the model secret nor the untrusted PR worktree. It accepts only the bounded primary artifact from the same workflow run and rechecks the live PR head and base before commenting. Before rendering E2E guidance, it independently allowlists coverage IDs and exact selector tuples, ignores artifact-authored E2E prose, and supplies canonical reasons. A newly added credential-free test can extend the job allowlist only through trusted-normalizer evidence bound to the same head SHA, changed-file path, and basename-derived selector ID.
- The separate publisher has pull-request write permission, but receives neither the model secret nor the untrusted PR worktree. It accepts only the bounded primary artifact from the same workflow run and rechecks the live PR head and base before commenting. Before rendering E2E guidance, it independently allowlists coverage IDs and exact selector tuples and ignores artifact-authored E2E prose. A newly added credential-free test can extend the job allowlist only through trusted-normalizer evidence bound to the same head SHA, changed-file path, and basename-derived selector ID.
- Sticky publication updates only a marker-bearing comment owned by `github-actions[bot]`; a user-authored marker cannot claim the update target.
The rendered comment preserves its hidden identity metadata while enforcing a 60 KiB UTF-8 limit, and publication errors remain visible in the publisher logs.
- The workflow posts advisory comments only; it does not approve, request changes, merge, push, label, or dispatch E2E.
Expand Down Expand Up @@ -154,15 +154,16 @@ comment and future reporting work. Findings include probe-shaped fields for impa
hints, and missing regression-test guidance so agents know what to check rather than treating findings
as generic commentary. Every source-of-truth review item includes a `findingId`: unresolved items
reference their covering open ledger finding, while satisfied and not-applicable items use `null`.
Every result also includes nested `e2e.coverage` and `e2e.targets` guidance. The trusted normalizer
Every result also includes nested `e2e.coverage` and `e2e.targets` guidance. The fields stay
separate in JSON, but comments and summaries combine their IDs into one `Recommended E2E` list and
one optional list. Each ID appears once. The trusted normalizer
restores deterministic requirements before model selections, retains only allowlisted coverage IDs
and exact supported selector tuples, and replaces model-authored reasons with canonical trusted
reasons. It discards free-form E2E domains, new-test recommendations, and no-selection explanations.
For a changed credential-free test, the normalizer also records structured exact-head evidence only
after the trusted module-tag parser accepts the source; model-provided evidence is overwritten. The
trusted publisher independently repeats the ID and tuple checks, verifies that evidence against the
result head and changed-file identity, and derives its own reasons instead of rendering E2E prose
from the artifact.
result head and changed-file identity, and renders only trusted IDs.
The compatibility schema retains `requiredTests` and `targets.required`, but those names describe
the normalized advisory tier, not merge requirements. Rendered comments label them as recommended;
the independent PR E2E controller does not consume advisor output.
Expand Down
53 changes: 15 additions & 38 deletions tools/pr-review-advisor/analyze.mts
Original file line number Diff line number Diff line change
Expand Up @@ -2626,56 +2626,33 @@ export function renderSummary(result: ReviewAdvisorResult): string {
}

function appendE2eSummary(lines: string[], e2e: CombinedE2eResult): void {
lines.push("## Required E2E coverage");
if (e2e.coverage.requiredTests.length === 0) {
lines.push(`- _None._${e2e.coverage.noE2eReason ? ` ${e2e.coverage.noE2eReason}` : ""}`);
} else {
for (const test of e2e.coverage.requiredTests.slice(0, 20)) {
lines.push(`- **${test.id}**: ${test.reason}`);
}
}
lines.push("");
lines.push("## Optional E2E coverage");
if (e2e.coverage.optionalTests.length === 0) {
lines.push("- _None._");
} else {
for (const test of e2e.coverage.optionalTests.slice(0, 20)) {
lines.push(`- **${test.id}**: ${test.reason}`);
}
}
lines.push("");
lines.push("## New E2E recommendations");
if (e2e.coverage.newE2eRecommendations.length === 0) {
const required = combinedE2eIds(e2e.targets.required, e2e.coverage.requiredTests);
const optional = combinedE2eIds(e2e.targets.optional, e2e.coverage.optionalTests);

lines.push("## Recommended E2E");
if (required.length === 0) {
lines.push("- _None._");
} else {
for (const recommendation of e2e.coverage.newE2eRecommendations.slice(0, 20)) {
lines.push(`- **${recommendation.domain}**: ${recommendation.reason}`);
lines.push(` - Suggested test: ${recommendation.suggestedTest}`);
}
}
lines.push("");
lines.push("## Required E2E selectors");
if (e2e.targets.required.length === 0) {
lines.push(
`- _None._${e2e.targets.noTargetE2eReason ? ` ${e2e.targets.noTargetE2eReason}` : ""}`,
);
} else {
for (const recommendation of e2e.targets.required.slice(0, 20)) {
lines.push(`- **${recommendation.id}**: ${recommendation.reason}`);
for (const id of required.slice(0, 20)) {
lines.push(`- **${id}**`);
}
}
lines.push("");
lines.push("## Optional E2E selectors");
if (e2e.targets.optional.length === 0) {
lines.push("## Optional E2E");
if (optional.length === 0) {
lines.push("- _None._");
} else {
for (const recommendation of e2e.targets.optional.slice(0, 20)) {
lines.push(`- **${recommendation.id}**: ${recommendation.reason}`);
for (const id of optional.slice(0, 20)) {
lines.push(`- **${id}**`);
}
}
lines.push("");
}

function combinedE2eIds(targets: Array<{ id: string }>, coverage: Array<{ id: string }>): string[] {
return [...new Set([...targets.map(({ id }) => id), ...coverage.map(({ id }) => id)])];
}

export function renderDetailedReview(result: ReviewAdvisorResult): string {
const lines = renderSummary(result).trimEnd().split("\n");
lines.push("");
Expand Down
95 changes: 25 additions & 70 deletions tools/pr-review-advisor/comment.mts
Original file line number Diff line number Diff line change
Expand Up @@ -488,104 +488,63 @@ function renderE2eDetails(result?: ReviewAdvisorResult): string {

const inventory = commentE2eInventory();
const exactHeadCredentialFreeJobIds = trustedExactHeadCredentialFreeJobIds(result);
const requiredCoverage = trustedCoverageItems(coverage?.requiredTests, inventory);
const optionalCoverage = trustedCoverageItems(coverage?.optionalTests, inventory);
const newRecommendations: NonNullable<
NonNullable<ReviewAdvisorResult["e2e"]>["coverage"]
>["newE2eRecommendations"] = [];
const requiredTargets = trustedTargetItems(
const requiredCoverage = trustedCoverageIds(coverage?.requiredTests, inventory);
const optionalCoverage = trustedCoverageIds(coverage?.optionalTests, inventory);
const requiredTargets = trustedTargetIds(
targets?.required,
true,
inventory,
exactHeadCredentialFreeJobIds,
);
const optionalTargets = trustedTargetItems(
const optionalTargets = trustedTargetIds(
targets?.optional,
false,
inventory,
exactHeadCredentialFreeJobIds,
);
const noE2eReason = "No deterministic or trusted-inventory E2E coverage was selected.";
const noTargetE2eReason = "No trusted E2E selector was selected.";
const requiredE2e = uniqueE2eIds([...requiredTargets, ...requiredCoverage]);
const optionalE2e = uniqueE2eIds([...optionalTargets, ...optionalCoverage]);
const lines = [
"",
"### E2E guidance",
"_Advisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output._",
"_Advisory only. E2E / PR Gate selects and runs jobs independently._",
"",
];

lines.push(
`**Recommended coverage:** ${renderE2eIds(requiredCoverage) || "_None_"}`,
`**Recommended selectors:** ${renderE2eIds(requiredTargets) || "_None_"}`,
);
if (requiredCoverage.length > 0) {
lines.push("");
for (const item of requiredCoverage.slice(0, E2E_RENDER_LIMIT)) {
const id = escapeLocationHtml(item.id || "E2E test");
const reason = item.reason ? ` — ${escapeCommentText(item.reason)}` : "";
lines.push(`- <code>${id}</code>${reason}`);
}
}
if (requiredTargets.length > 0) {
lines.push("");
for (const item of requiredTargets.slice(0, E2E_RENDER_LIMIT)) {
const id = escapeLocationHtml(item.id || "E2E target");
const reason = item.reason ? ` — ${escapeCommentText(item.reason)}` : "";
lines.push(`- <code>${id}</code>${reason}`);
}
}
lines.push(`**Recommended E2E:** ${renderE2eIds(requiredE2e) || "_None_"}`);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

if (optionalCoverage.length > 0 || optionalTargets.length > 0 || newRecommendations.length > 0) {
if (optionalE2e.length > 0) {
lines.push(
"",
"<details>",
`<summary>${compactCount(optionalCoverage.length, "optional coverage item")} · ${compactCount(optionalTargets.length, "optional selector")} · ${compactCount(newRecommendations.length, "new-test recommendation")}</summary>`,
`<summary>${compactCount(optionalE2e.length, "optional E2E recommendation")}</summary>`,
"",
);
for (const item of optionalCoverage.slice(0, E2E_RENDER_LIMIT)) {
lines.push(
`- Optional coverage <code>${escapeLocationHtml(item.id || "unnamed")}</code>${item.reason ? ` — ${escapeCommentText(item.reason)}` : ""}`,
);
}
for (const item of optionalTargets.slice(0, E2E_RENDER_LIMIT)) {
lines.push(
`- Optional selector <code>${escapeLocationHtml(item.id || "unnamed")}</code>${item.reason ? ` — ${escapeCommentText(item.reason)}` : ""}`,
);
}
for (const item of newRecommendations.slice(0, E2E_RENDER_LIMIT)) {
const name = item.suggestedTest || item.domain || "E2E test";
lines.push(
`- New test: ${escapeCommentText(name)}${item.reason ? ` — ${escapeCommentText(item.reason)}` : ""}`,
);
for (const id of optionalE2e.slice(0, E2E_RENDER_LIMIT)) {
lines.push(`- <code>${escapeLocationHtml(id)}</code>`);
}
lines.push("", "</details>");
}

if (requiredCoverage.length === 0 && optionalCoverage.length === 0 && noE2eReason) {
lines.push("", `**Why no E2E coverage is recommended:** ${escapeCommentText(noE2eReason)}`);
}
if (requiredTargets.length === 0 && optionalTargets.length === 0 && noTargetE2eReason) {
lines.push("", `**Why no selector is recommended:** ${escapeCommentText(noTargetE2eReason)}`);
}
lines.push("");
return `${lines.join("\n")}\n`;
}

function trustedCoverageItems(
function trustedCoverageIds(
items: Array<{ id?: string; reason?: string }> | undefined,
inventory: TrustedE2eRecommendationInventory,
): Array<{ id: string; reason: string }> {
): string[] {
const allowedIds = new Set([...inventory.allowedJobIds, ...inventory.liveSupportedTargetIds]);
const seen = new Set<string>();
return (items ?? []).flatMap((item) => {
const id = item.id;
if (!id || !allowedIds.has(id) || seen.has(id)) return [];
seen.add(id);
return [{ id, reason: "Selected from the trusted checked-in E2E coverage inventory." }];
return [id];
});
}

function trustedTargetItems(
function trustedTargetIds(
items:
| Array<{
id?: string;
Expand All @@ -598,7 +557,7 @@ function trustedTargetItems(
required: boolean,
inventory: TrustedE2eRecommendationInventory,
exactHeadCredentialFreeJobIds: ReadonlySet<string>,
): Array<{ id: string; reason: string }> {
): string[] {
const allowedJobs = new Set(inventory.allowedJobIds);
const allowedTargets = new Set(inventory.liveSupportedTargetIds);
const seen = new Set<string>();
Expand All @@ -613,18 +572,14 @@ function trustedTargetItems(
const key = `${selectorType}:${id}`;
if (!trustedTuple || seen.has(key)) return [];
seen.add(key);
const reason =
selectorType === "all"
? "Selected as the trusted full E2E fan-out selector."
: selectorType === "job"
? exactHeadCredentialFreeJobIds.has(id) && !allowedJobs.has(id)
? "Selected as a trusted exact-head credential-free E2E job."
: "Selected as a trusted checked-in E2E job."
: "Selected as a trusted live-supported E2E target.";
return [{ id, reason }];
return [id];
});
}

function uniqueE2eIds(ids: string[]): string[] {
return [...new Set(ids)];
}

function trustedExactHeadCredentialFreeJobIds(result?: ReviewAdvisorResult): Set<string> {
const ids = new Set<string>();
const headSha = result?.headSha;
Expand All @@ -651,10 +606,10 @@ function commentE2eInventory(): TrustedE2eRecommendationInventory {
return cachedE2eInventory;
}

function renderE2eIds(items: Array<{ id?: string }>): string {
return items
function renderE2eIds(ids: string[]): string {
return ids
.slice(0, E2E_RENDER_LIMIT)
.map((item) => `<code>${escapeLocationHtml(item.id || "unnamed")}</code>`)
.map((id) => `<code>${escapeLocationHtml(id)}</code>`)
.join(", ");
}

Expand Down