diff --git a/test/pr-review-advisor-comment-cli.test.ts b/test/pr-review-advisor-comment-cli.test.ts index ef90e4a33cf..db937c8bd1e 100644 --- a/test/pr-review-advisor-comment-cli.test.ts +++ b/test/pr-review-advisor-comment-cli.test.ts @@ -176,6 +176,10 @@ describe("PR review advisor comment CLI", () => { partial: false, confidence: "high", counts: { blockers: 1, warnings: 1, suggestions: 1 }, + e2e: { + recommended: [{ id: "security-posture" }], + optional: [], + }, }); expect(completed.fingerprints?.findings).toMatch(/^[0-9a-f]{64}$/u); expect(completed.fingerprints?.e2e).toMatch(/^[0-9a-f]{64}$/u); @@ -280,7 +284,7 @@ describe("PR review advisor comment CLI", () => { } }); - it("renders sanitized model-lane status and structural disagreement only", () => { + it("renders sanitized model-lane status and visible E2E disagreements (#8016)", () => { const result = { version: 1, headSha: "a".repeat(40), @@ -316,7 +320,24 @@ describe("PR review advisor comment CLI", () => { summary: { confidence: "low", oneLine: "do not publish this summary" }, findings: [{ severity: "warning", title: "do not publish this finding" }], e2e: { - coverage: { requiredTests: [{ id: "security-posture" }], optionalTests: [] }, + coverage: { + requiredTests: [ + { + id: "full-e2e", + reason: "Cover the shipped startup chain. @team ", + }, + ], + optionalTests: [ + { + id: "full-e2e", + reason: "do not publish a duplicate selector", + }, + { + id: "not-allowlisted", + reason: "do not publish an unknown selector", + }, + ], + }, targets: { required: [], optional: [] }, }, }; @@ -343,8 +364,69 @@ 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).toContain( + "1 additional E2E selection from the second opinion", + ); + expect(comment).toContain( + "full-e2e: The completed second-opinion lane identified E2E coverage that the primary lane omitted.", + ); + expect(comment).not.toContain("Cover the shipped startup chain"); + expect(comment).not.toContain("do not publish a duplicate selector"); + expect(comment).not.toContain("not-allowlisted"); + expect(comment).not.toContain("do not publish an unknown selector"); + expect(comment).toContain( + "Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.", + ); expect(comment).toContain("1 optional E2E recommendation"); expect(comment.match(/vllm-docker-storage<\/code>/gu)).toHaveLength(1); + expect(comment.match(/full-e2e<\/code>/gu)).toHaveLength(1); + + const completedPartialComment = buildComment({ + summary: "# ignored\n", + result, + lanes: { + primary, + secondOpinion: { ...secondOpinion, partial: true }, + }, + }); + expect(completedPartialComment).not.toContain( + "additional E2E selection from the second opinion", + ); + expect(completedPartialComment).not.toContain("full-e2e"); + + const malformedSecondOpinionResult = { + ...secondOpinionResult, + e2e: { + coverage: { + requiredTests: [null, "invalid", { id: "full-e2e", reason: "valid coverage" }], + optionalTests: [], + }, + targets: { + required: [ + null, + "invalid", + { + id: "security-posture", + workflow: "e2e.yaml", + selectorType: "job", + required: true, + reason: "valid target", + }, + ], + optional: [], + }, + }, + }; + expect( + normalizeAdvisorLaneReport( + malformedSecondOpinionResult, + malformedSecondOpinionResult, + result.headSha, + ).e2e, + ).toEqual({ + recommended: [{ id: "security-posture" }, { id: "full-e2e" }], + optional: [], + }); const partialComment = buildComment({ summary: "# ignored\n", @@ -365,5 +447,6 @@ describe("PR review advisor comment CLI", () => { expect(partialComment).not.toContain("do not publish this provider failure"); expect(partialComment).not.toContain("do not publish this summary"); expect(partialComment).not.toContain("do not publish this finding"); + expect(partialComment).not.toContain("full-e2e"); }); }); diff --git a/test/pr-risk-plan.test.ts b/test/pr-risk-plan.test.ts index 4c6962bcbdd..c8eb969e9df 100644 --- a/test/pr-risk-plan.test.ts +++ b/test/pr-risk-plan.test.ts @@ -27,7 +27,7 @@ describe("deterministic PR risk plan", () => { const second = plan("src/lib/onboard.ts", "src/lib/state/registry.ts"); expect(first).toEqual(second); - expect(first.version).toBe(9); + expect(first.version).toBe(10); expect(first.headSha).toBe(HEAD_SHA); expect(first.planHash).toMatch(/^[a-f0-9]{64}$/u); expect(first.changedFiles).toEqual(["src/lib/onboard.ts", "src/lib/state/registry.ts"]); @@ -84,6 +84,39 @@ describe("deterministic PR risk plan", () => { expect(result.planHash).not.toBe(withoutFocusedSelection.planHash); }); + it("selects startup and auth E2E for managed startup delivery changes (#8016)", () => { + const changedFiles = [ + "scripts/lib/entrypoint-env-wrapper.sh", + "src/lib/onboard/managed-startup/agent-environment.ts", + "src/lib/onboard/sandbox-create-launch.ts", + ]; + const result = plan(...changedFiles); + const adjacentOnboardChange = plan("src/lib/onboard/provider-selection.ts"); + + expect(result.families).toContainEqual( + expect.objectContaining({ + id: "focused-e2e", + matchedFiles: changedFiles, + requiredJobs: [ + "device-auth-health", + "issue-4462-scope-upgrade-approval", + "openclaw-inference-switch", + ], + }), + ); + expect(riskPlanRequiredJobIds(result)).toEqual( + expect.arrayContaining([ + "device-auth-health", + "issue-4462-scope-upgrade-approval", + "openclaw-inference-switch", + ]), + ); + expect(riskPlanRequiredJobIds(adjacentOnboardChange)).toEqual([ + "onboard-repair", + "onboard-resume", + ]); + }); + it("leaves E2E support-only changes in the fast e2e-support project (#7921)", () => { const changedFiles = ["test/e2e/support/workflow-plan.test.ts"]; const focusedE2eJobs = focusedE2eJobsForChangedFiles(changedFiles); diff --git a/tools/advisors/risk-plan.mts b/tools/advisors/risk-plan.mts index 6eacfb7d51a..0f514f76ffe 100644 --- a/tools/advisors/risk-plan.mts +++ b/tools/advisors/risk-plan.mts @@ -3,7 +3,7 @@ import { createHash } from "node:crypto"; -export const RISK_PLAN_VERSION = 9 as const; +export const RISK_PLAN_VERSION = 10 as const; export const PR_E2E_TYPED_TARGET_IDS = [ "ubuntu-repo-cloud-langchain-deepagents-code", @@ -20,6 +20,11 @@ const POST_REBOOT_DELIVERY_RUNTIME_FILES = new Set([ "src/lib/onboard/docker-startup-command-agent.ts", "src/lib/onboard/sandbox-create-step.ts", ]); +const MANAGED_STARTUP_E2E_JOB_IDS = [ + "device-auth-health", + "issue-4462-scope-upgrade-approval", + "openclaw-inference-switch", +] as const; export type RiskTier = 0 | 1 | 2 | 3; export type RiskFamilyId = @@ -157,6 +162,22 @@ export function focusedPrE2eTargetsForChangedFiles( ]; } +export function focusedPrE2eJobsForChangedFiles( + changedFiles: readonly string[], +): TrustedFocusedE2eJob[] { + const matchedFiles = stableUnique( + changedFiles.filter( + (file) => + (file.startsWith("src/lib/onboard/managed-startup/") || + file === "src/lib/onboard/sandbox-create-launch.ts" || + file === "scripts/lib/entrypoint-env-wrapper.sh") && + isRuntimeRelevant(file), + ), + ); + if (matchedFiles.length === 0) return []; + return MANAGED_STARTUP_E2E_JOB_IDS.map((id) => ({ id, matchedFiles })); +} + export const RISK_RULES: readonly RiskRule[] = [ { id: "lifecycle-state", @@ -393,7 +414,10 @@ export function buildRiskPlan(options: { }): RiskPlan { const changedFiles = stableUnique(options.changedFiles); const runtimeFiles = changedFiles.filter(isRuntimeRelevant); - const focusedE2eJobs = normalizeFocusedE2eJobs(options.focusedE2eJobs ?? [], changedFiles); + const focusedE2eJobs = normalizeFocusedE2eJobs( + [...focusedPrE2eJobsForChangedFiles(changedFiles), ...(options.focusedE2eJobs ?? [])], + changedFiles, + ); const focusedLiveFiles = new Set(focusedE2eJobs.flatMap((selection) => selection.matchedFiles)); const staticFamilies: RiskPlanFamily[] = RISK_RULES.flatMap((rule) => { const matchedFiles = runtimeFiles.filter( diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index eff195bd1e0..8d78634409e 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -45,7 +45,15 @@ It intentionally does not report GitHub mergeability, branch protection, CI stat 14. Retries transient provider failures such as HTTP 429 within the same session using one bounded exponential-backoff layer. GPT waits 6s, 12s, 24s, and 48s; Nemotron waits 9s, 18s, 36s, and 72s so parallel lanes do not retry in lockstep. The workflow still publishes the primary comment and lane artifacts after an incomplete analysis. An incomplete primary review fails its outcome step; the artifact-only evaluation lane does not affect the workflow result. 15. Validates and repairs the draft synthesis in the final turn of the same session. If that turn fails or emits malformed output, the runner preserves a schema-valid canonical draft with a limitation; a post-validation ledger mismatch still fails closed. 16. Writes artifacts under the model-specific artifact directory in the writable runtime subtree, downloads them to the trusted host, and uploads them from the read-only analysis job. Example directories are `artifacts/pr-review-advisor/` and `artifacts/pr-review-advisor-nemotron-ultra/`. -17. Uses a separate publisher job with no model credential or untrusted worktree. It validates the primary artifact and live PR head/base, then posts or updates one combined sticky PR comment marked by ``. The evaluation lane does not publish another review. Previous sticky-comment ingestion is disabled for both lanes. +17. Uses a separate publisher job with no model credential or untrusted worktree. + It validates the primary artifact and live PR head/base. + It then posts or updates one combined sticky PR comment marked by ``. + The primary lane remains authoritative for the assessment and recommended E2E guidance. + When the completed second-opinion lane includes a trusted E2E selector that the primary lane omits, the publisher shows an optional disagreement. + The disagreement includes the selector and a publisher-authored coverage-gap reason in the same comment. + A missing, malformed, or incomplete second-opinion result cannot suppress the primary result. + The evaluation lane does not publish another review. + Previous sticky-comment ingestion is disabled for both lanes. The ordered stage array in `buildPromptTurns` is the source of truth for stage order, evidence, and prompt text. Runtime numbering and prompt artifact names derive from that array, so adding or @@ -100,6 +108,18 @@ Authors and coding agents should follow the shared [PR CI and Review Follow-Up]( job that the model omits or downgrades. The PR E2E controller separately dispatches every listed job without consuming the advisor's normalized result. +Risk plan version 10 maps runtime changes from these paths to the `focused-e2e` family: + +- `src/lib/onboard/managed-startup/**`. +- `src/lib/onboard/sandbox-create-launch.ts`. +- `scripts/lib/entrypoint-env-wrapper.sh`. + +Each match selects these focused E2E jobs: + +- `device-auth-health`. +- `issue-4462-scope-upgrade-approval`. +- `openclaw-inference-switch`. + ## Required secret Configure this repository secret for review analysis: @@ -170,6 +190,9 @@ reports how many more IDs exist. The trusted normalizer restores deterministic requirements before model selections, retains only allowlisted coverage IDs and supported selector tuples, and replaces model-authored reasons with trusted reasons. It discards free-form E2E domains, new-test recommendations, and no-selection explanations. +The publisher compares the completed lanes after this normalization. It lists trusted +second-opinion-only selectors with a publisher-authored coverage-gap reason as optional +disagreements without adding them to the primary lane's recommended E2E guidance. For a changed credential-free test, the normalizer also records structured 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 diff --git a/tools/pr-review-advisor/comment.mts b/tools/pr-review-advisor/comment.mts index 2da91855dbd..b4efdb2140b 100755 --- a/tools/pr-review-advisor/comment.mts +++ b/tools/pr-review-advisor/comment.mts @@ -120,12 +120,22 @@ type LaneFingerprints = { e2e: string; }; +type LaneE2eRecommendation = { + id: string; +}; + +type LaneE2eRecommendations = { + recommended: LaneE2eRecommendation[]; + optional: LaneE2eRecommendation[]; +}; + export type AdvisorLaneReport = { status: "completed" | "failed" | "skipped" | "unavailable"; partial: boolean; counts?: FindingCounts; confidence?: "low" | "medium" | "high"; fingerprints?: LaneFingerprints; + e2e?: LaneE2eRecommendations; }; export type AdvisorLaneReports = { @@ -415,9 +425,10 @@ function renderAdvisorLanes(lanes?: AdvisorLaneReports): string { ]; const comparison = renderLaneComparison(lanes.primary, lanes.secondOpinion); if (comparison) lines.push(`- **Model comparison:** ${comparison}`); + lines.push(...renderSecondOpinionE2eRecommendations(lanes.primary, lanes.secondOpinion)); lines.push( "", - "_Nemotron output stays in workflow artifacts and does not change the assessment above._", + "_Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate._", "", ); return `${lines.join("\n")}\n`; @@ -474,6 +485,54 @@ function renderLaneComparison( return `${findingComparison}; ${e2eComparison}; ${countComparison}.`; } +function renderSecondOpinionE2eRecommendations( + primary: AdvisorLaneReport, + secondOpinion: AdvisorLaneReport, +): string[] { + if ( + primary.status !== "completed" || + secondOpinion.status !== "completed" || + primary.partial || + secondOpinion.partial || + !primary.e2e || + !secondOpinion.e2e + ) { + return []; + } + + const primaryIds = new Set( + [...primary.e2e.recommended, ...primary.e2e.optional].map(({ id }) => id), + ); + const additionalIds = new Set(); + const additional = [...secondOpinion.e2e.recommended, ...secondOpinion.e2e.optional].filter( + ({ id }) => { + if (primaryIds.has(id) || additionalIds.has(id)) return false; + additionalIds.add(id); + return true; + }, + ); + if (additional.length === 0) return []; + + const lines = [ + "", + "
", + `${compactCount(additional.length, "additional E2E selection")} from the second opinion`, + "", + "_Advisory only. The primary lane did not select these E2E jobs or targets._", + "", + ]; + for (const recommendation of additional.slice(0, E2E_RENDER_LIMIT)) { + lines.push( + `- ${escapeLocationHtml(recommendation.id)}: The completed second-opinion lane identified E2E coverage that the primary lane omitted.`, + ); + } + if (additional.length > E2E_RENDER_LIMIT) { + lines.push(`- _${additional.length - E2E_RENDER_LIMIT} more._`); + } + lines.push("", "
"); + return lines; +} + function countDifference(difference: number, label: string): string { if (difference === 0) return `the same number of ${label}s`; const direction = difference > 0 ? "more" : "fewer"; @@ -536,29 +595,22 @@ function renderE2eDetails(result?: ReviewAdvisorResult): string { } function trustedCoverageIds( - items: Array<{ id?: string; reason?: string }> | undefined, + items: unknown[] | undefined, inventory: TrustedE2eRecommendationInventory, ): string[] { const allowedIds = new Set([...inventory.allowedJobIds, ...inventory.liveSupportedTargetIds]); const seen = new Set(); return (items ?? []).flatMap((item) => { + if (!isRecord(item)) return []; const id = item.id; - if (!id || !allowedIds.has(id) || seen.has(id)) return []; + if (typeof id !== "string" || !allowedIds.has(id) || seen.has(id)) return []; seen.add(id); return [id]; }); } function trustedTargetIds( - items: - | Array<{ - id?: string; - workflow?: string; - selectorType?: string; - required?: boolean; - reason?: string; - }> - | undefined, + items: unknown[] | undefined, required: boolean, inventory: TrustedE2eRecommendationInventory, changedCredentialFreeJobIds: ReadonlySet, @@ -567,9 +619,15 @@ function trustedTargetIds( const allowedTargets = new Set(inventory.liveSupportedTargetIds); const seen = new Set(); return (items ?? []).flatMap((item) => { + if (!isRecord(item)) return []; const id = item.id; const selectorType = item.selectorType; - if (!id || item.workflow !== inventory.workflow || item.required !== required) return []; + if ( + typeof id !== "string" || + item.workflow !== inventory.workflow || + item.required !== required + ) + return []; const trustedTuple = (selectorType === "all" && id === inventory.fanoutId) || (selectorType === "job" && (allowedJobs.has(id) || changedCredentialFreeJobIds.has(id))) || @@ -633,6 +691,7 @@ function trustedLaneStructure( counts: FindingCounts; confidence?: "low" | "medium" | "high"; fingerprints: LaneFingerprints; + e2e: LaneE2eRecommendations; } | undefined { if (!isRecord(value) || value.version !== 1 || !Array.isArray(value.findings)) return undefined; @@ -646,9 +705,11 @@ function trustedLaneStructure( } const summary = isRecord(value.summary) ? value.summary : undefined; const confidence = trustedLaneConfidence(summary?.confidence); + const e2e = trustedLaneE2eRecommendations(value as ReviewAdvisorResult); return { counts, ...(confidence ? { confidence } : {}), + e2e, fingerprints: { findings: opaqueFingerprint(normalizedFindingRecords(value.findings)), e2e: opaqueFingerprint(e2eDecisionSets(value.e2e)), @@ -667,6 +728,43 @@ function trustedLaneConfidence(value: unknown): "low" | "medium" | "high" | unde return value === "low" || value === "medium" || value === "high" ? value : undefined; } +function trustedLaneE2eRecommendations(result: ReviewAdvisorResult): LaneE2eRecommendations { + const inventory = commentE2eInventory(); + const changedCredentialFreeJobIds = trustedChangedCredentialFreeJobIds(result); + const coverage = result.e2e?.coverage; + const targets = result.e2e?.targets; + return { + recommended: trustedLaneE2eTier( + Array.isArray(coverage?.requiredTests) ? coverage.requiredTests : undefined, + Array.isArray(targets?.required) ? targets.required : undefined, + true, + inventory, + changedCredentialFreeJobIds, + ), + optional: trustedLaneE2eTier( + Array.isArray(coverage?.optionalTests) ? coverage.optionalTests : undefined, + Array.isArray(targets?.optional) ? targets.optional : undefined, + false, + inventory, + changedCredentialFreeJobIds, + ), + }; +} + +function trustedLaneE2eTier( + coverageItems: unknown[] | undefined, + targetItems: unknown[] | undefined, + required: boolean, + inventory: TrustedE2eRecommendationInventory, + changedCredentialFreeJobIds: ReadonlySet, +): LaneE2eRecommendation[] { + const ids = uniqueE2eIds([ + ...trustedTargetIds(targetItems, required, inventory, changedCredentialFreeJobIds), + ...trustedCoverageIds(coverageItems, inventory), + ]); + return ids.map((id) => ({ id })); +} + function e2eDecisionSets(value: unknown): Record { const e2e = isRecord(value) ? value : {}; const coverage = isRecord(e2e.coverage) ? e2e.coverage : {};