diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 916717ba96..380120831c 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -175,6 +175,13 @@ export interface UnifiedReviewInput { * `review.effort_score` — see `resolveReviewPromptOverrides`'s `effortScore`); omitted ⇒ no chip * (byte-identical). (#1955) */ reviewEffort?: { band: 1 | 2 | 3 | 4 | 5; minutes: number }; + /** Linked-issue satisfaction advisory (#2174, render slice of #1961): whether this PR's diff appears to + * satisfy the linked issue's own intent/acceptance criteria — `addressed` / `partial` / `unaddressed` plus a + * short rationale, already public-safe (see `src/services/linked-issue-satisfaction.ts`). PRESENTATION + * ONLY — rendered as an additive collapsible section; never changes `status`/the gate verdict. Absent + * (default; the host only resolves this when `review.linkedIssueSatisfaction` is on) ⇒ no section is + * rendered, byte-identical to today. */ + linkedIssueSatisfaction?: { status: "addressed" | "partial" | "unaddressed"; rationale: string }; } /** One row of the readiness signal table (gittensory side, host-provided; the engine adds Code review). */ @@ -437,6 +444,22 @@ function formatReviewTimestamp(value: string | number | Date | undefined): strin return time.toISOString().replace(/\.\d{3}Z$/, "Z").replace("T", " ").replace("Z", " UTC"); } +const LINKED_ISSUE_SATISFACTION_LABELS: Record<"addressed" | "partial" | "unaddressed", string> = { + addressed: "Addressed", + partial: "Partially addressed", + unaddressed: "Not yet addressed", +}; + +/** Render the linked-issue satisfaction advisory (#2174) as a `status` heading + rationale body, or "" when + * absent — the caller only appends the section when this returns non-empty, so an unresolved advisory omits + * the section entirely (byte-identical to today). Angle-escaping happens once, in the shared `details()` + * wrapper the caller passes this body to (matching every other collapsible section's own convention). */ +function linkedIssueSatisfactionBlock(result: UnifiedReviewInput["linkedIssueSatisfaction"]): string { + if (!result?.rationale.trim()) return ""; + const label = LINKED_ISSUE_SATISFACTION_LABELS[result.status]; + return `**${label}**\n${result.rationale.trim()}`; +} + /** Render the failing CI checks as a bullet list of `name — reason` (reason only when the check carried one), * preferring failingDetails (which pairs each name with its WHY: codecov %/test/lint reason) and falling back * to the bare failingChecks names. Public-safe: only check names + their already-public short summary, both @@ -561,6 +584,15 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi if (failingChecks) blocks.push(`**CI checks failing**\n${failingChecks}`); blocks.push(signalTable(input, ctx)); + + // Linked-issue satisfaction advisory (#2174): additive, collapsed section — omitted entirely when the host + // never resolved a result (default) or `review.comment_verbosity: quiet` trims decorative detail, exactly + // like Nits/extraCollapsibles above. Never affects `status`/the gate verdict. + const satisfactionBody = linkedIssueSatisfactionBlock(input.linkedIssueSatisfaction); + if (satisfactionBody && verbosity !== "quiet") { + blocks.push(details("Linked issue satisfaction", satisfactionBody, undefined, collapsiblesOpen)); + } + if (verbosity !== "quiet") { for (const c of ctx.extraCollapsibles ?? []) { if (c.body.trim()) { @@ -600,6 +632,7 @@ export function buildUnifiedReviewInput(opts: { verdictReason?: string; reviewEffort?: { band: 1 | 2 | 3 | 4 | 5; minutes: number }; maxFindingsCaps?: { blockers: number | null; nits: number | null }; + linkedIssueSatisfaction?: { status: "addressed" | "partial" | "unaddressed"; rationale: string }; }): UnifiedReviewInput { const ex = extractReviewSummary(opts.reviews); const changedFiles = typeof opts.changedFiles === "number" ? opts.changedFiles : opts.changedFiles.length; @@ -618,6 +651,7 @@ export function buildUnifiedReviewInput(opts: { ...(opts.verdictReason !== undefined ? { verdictReason: opts.verdictReason } : {}), ...(opts.reviewEffort !== undefined ? { reviewEffort: opts.reviewEffort } : {}), ...(opts.maxFindingsCaps !== undefined ? { maxFindingsCaps: opts.maxFindingsCaps } : {}), + ...(opts.linkedIssueSatisfaction !== undefined ? { linkedIssueSatisfaction: opts.linkedIssueSatisfaction } : {}), }; } diff --git a/src/services/linked-issue-satisfaction.ts b/src/services/linked-issue-satisfaction.ts new file mode 100644 index 0000000000..ce55795d0c --- /dev/null +++ b/src/services/linked-issue-satisfaction.ts @@ -0,0 +1,151 @@ +// Linked-issue satisfaction assessment (#2172, pure analysis core of #1961). +// +// We already enforce deterministic linked-issue HARD rules (src/review/linked-issue-hard-rules.ts) and fetch +// linked-issue text in grounding, but never judge whether the PR's diff actually satisfies what the issue +// asked for. This module is the bounded, AI-BACKED analysis core: prompt composition + response parsing only — +// NO gate wiring, NO disposition change, NO I/O (no env, no model call). The caller supplies the model's raw +// text output (from whichever provider it already resolved — self-host router or BYOK, exactly like +// ai-review.ts/ai-slop.ts do); this module never talks to a model itself. That orchestration (budget, provider +// selection, usage accounting, and — eventually — a `gate.linkedIssueSatisfaction` mode wiring) is a separate, +// maintainer-only slice. +// +// Hard guarantees (mirrors ai-slop.ts's fail-safe discipline): +// • No issue text (empty/absent) ⇒ no finding. Never guesses "unaddressed" from silence. +// • Malformed/unparseable model output, or a thrown error while composing ⇒ no finding, never throws. +// • A LOW-CONFIDENCE "unaddressed" verdict is never published as unaddressed — it degrades to no finding, +// so an uncertain model never manufactures a false "you didn't fix this" call that could spook a +// contributor. "addressed"/"partial" are not similarly gated: a false-positive "looks addressed" is a much +// lower-stakes error than a false "unaddressed" (advisory-only either way; no gate can read this yet). +// • Every public string is forced through the public-safe filter; anything tripping the boundary is dropped. +import { toPublicSafe } from "./ai-review"; + +/** The three verdicts this advisory can reach about a single linked issue. */ +export const LINKED_ISSUE_SATISFACTION_STATUSES = ["addressed", "partial", "unaddressed"] as const; +export type LinkedIssueSatisfactionStatus = (typeof LINKED_ISSUE_SATISFACTION_STATUSES)[number]; + +/** Below this calibrated confidence, an "unaddressed" verdict is too uncertain to publish (see module doc) — + * mirrors the AI review path's confidence-floor philosophy (`aiReviewCloseConfidence`, ai-review.ts) applied + * here as a fixed, non-configurable floor since this slice has no gate wiring to carry an operator override. */ +export const LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR = 0.5; + +const MAX_RATIONALE_LENGTH = 400; +const MAX_ISSUE_TEXT_CHARS = 6000; +const MAX_DIFF_CHARS = 60000; +const MAX_BODY_CHARS = 2000; + +export type LinkedIssueSatisfactionInput = { + /** The already-fetched linked-issue title + body text (grounding already resolved this — see + * review/grounding-wire.ts). Empty/absent ⇒ no finding (fail-safe: never assessed without real issue text). */ + issueText: string | null | undefined; + prTitle: string; + prBody?: string | null | undefined; + /** A bounded unified-diff-ish string (filenames + patches), same shape ai-review.ts/ai-slop.ts already build. */ + diff: string; +}; + +/** A public-safe, bounded assessment of whether a PR satisfies its linked issue. Never a gate signal by + * itself — purely advisory data for a renderer (#2174) or a future gate slice to consume. */ +export type LinkedIssueSatisfactionResult = { + status: LinkedIssueSatisfactionStatus; + rationale: string; + /** The model's own calibrated confidence in [0,1] that `status` is correct. */ + confidence: number; +}; + +function isSatisfactionStatus(value: unknown): value is LinkedIssueSatisfactionStatus { + return typeof value === "string" && (LINKED_ISSUE_SATISFACTION_STATUSES as readonly string[]).includes(value); +} + +/** Calibrated confidence in [0,1]; an absent/unparseable/out-of-range value degrades to 0 — the LOWEST + * confidence, not the highest (opposite of ai-review.ts's ModelReview default). An "unaddressed" verdict with + * no legible confidence must fail the floor below rather than be trusted by default, since a hallucinated + * "unaddressed" is the one failure mode this module exists to suppress. */ +function parseConfidence(value: unknown): number { + const n = typeof value === "number" ? value : Number(value); + if (!Number.isFinite(n) || n < 0 || n > 1) return 0; + return n; +} + +const SATISFACTION_SYSTEM_PROMPT = [ + "You are a senior open-source maintainer judging whether a pull request satisfies the intent and acceptance", + "criteria of a SINGLE linked issue. Judge ONLY the issue text and the PR's title/description/diff provided.", + "Be conservative: 'addressed' requires the diff to visibly fulfill the issue's own ask; 'partial' means it", + "makes real progress but plainly leaves part of the issue's stated scope undone; 'unaddressed' means the", + "diff does not appear to touch the issue's ask at all, or contradicts it.", + "Reserve 'unaddressed' for clear, evidence-backed cases — when genuinely uncertain, prefer 'partial'.", + "Never accuse; describe the gap constructively so a maintainer can decide what (if anything) to do.", + "Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability,", + "reviewability, or farming.", + "Respond with ONLY a JSON object of this exact shape (no prose, no code fence):", + '{"status": "addressed"|"partial"|"unaddressed", "rationale": string, "confidence": number}', + "- rationale: ONE to TWO sentences, specific to this issue and this diff.", + "- confidence: your CALIBRATED probability in [0,1] that `status` is correct. Use a lower value when the", + "issue text is vague, the diff is hard to map to the issue's ask, or you are speculating.", +].join(" "); + +/** Compose the user prompt for the linked-issue satisfaction model call. Pure — no I/O. Omits the description + * line when the PR body is empty, mirroring ai-slop.ts's buildUserPrompt shape. */ +export function buildLinkedIssueSatisfactionPrompt(input: LinkedIssueSatisfactionInput): string { + const issueText = (input.issueText ?? "").trim().slice(0, MAX_ISSUE_TEXT_CHARS); + return [ + `Linked issue text:\n${issueText}`, + "", + `Pull request: ${input.prTitle}`, + input.prBody?.trim() ? `Description:\n${input.prBody.trim().slice(0, MAX_BODY_CHARS)}` : "Description: (none)", + "", + "Unified diff (truncated if large):", + input.diff.slice(0, MAX_DIFF_CHARS), + ].join("\n"); +} + +/** Parse the model's raw JSON text response into a {@link LinkedIssueSatisfactionResult}, or null when the + * output is unusable (no JSON object, invalid status, or the confidence floor rejects an "unaddressed" call). + * PURE — never throws (a malformed blob that matches the brace regex but fails JSON.parse is caught). */ +export function parseLinkedIssueSatisfactionOpinion(text: string): LinkedIssueSatisfactionResult | null { + const match = text + .replace(/^```(?:json)?\s*/i, "") + .replace(/```$/i, "") + .match(/\{[\s\S]*\}/); + if (!match) return null; + let obj: Record; + try { + obj = JSON.parse(match[0]) as Record; + } catch { + return null; + } + if (!isSatisfactionStatus(obj.status)) return null; + const rationale = typeof obj.rationale === "string" ? obj.rationale.trim().slice(0, MAX_RATIONALE_LENGTH) : ""; + const confidence = parseConfidence(obj.confidence); + // Fail-safe floor (#2172): a low-confidence "unaddressed" is never published as unaddressed — the caller + // gets no finding at all rather than a shaky "you didn't fix this" call. addressed/partial are unaffected. + if (obj.status === "unaddressed" && confidence < LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR) return null; + if (!rationale) return null; + return { status: obj.status, rationale, confidence }; +} + +/** + * Build the bounded, public-safe linked-issue satisfaction result from raw model text, given the already- + * fetched issue text. PURE + fail-safe: no issue text, unparseable output, a below-floor "unaddressed" call, or + * a rationale that does not survive public-safe sanitization all yield `null` — the caller only surfaces a + * finding when this returns non-null. Never throws. + */ +export function buildLinkedIssueSatisfactionResult( + issueText: string | null | undefined, + modelResponseText: string, +): LinkedIssueSatisfactionResult | null { + if (!(issueText ?? "").trim()) return null; + try { + const opinion = parseLinkedIssueSatisfactionOpinion(modelResponseText); + if (!opinion) return null; + const safeRationale = toPublicSafe(opinion.rationale); + if (!safeRationale) return null; + return { status: opinion.status, rationale: safeRationale, confidence: opinion.confidence }; + } catch { + return null; + } +} + +export const __linkedIssueSatisfactionInternals = { + parseLinkedIssueSatisfactionOpinion, + buildLinkedIssueSatisfactionPrompt, +}; diff --git a/test/unit/linked-issue-satisfaction.test.ts b/test/unit/linked-issue-satisfaction.test.ts new file mode 100644 index 0000000000..1a089db2eb --- /dev/null +++ b/test/unit/linked-issue-satisfaction.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, it } from "vitest"; +import { + LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR, + __linkedIssueSatisfactionInternals, + buildLinkedIssueSatisfactionResult, + parseLinkedIssueSatisfactionOpinion, + type LinkedIssueSatisfactionInput, +} from "../../src/services/linked-issue-satisfaction"; + +const { buildLinkedIssueSatisfactionPrompt } = __linkedIssueSatisfactionInternals; + +function opinionJson(over: Partial<{ status: string; rationale: string; confidence: number }> = {}): string { + return JSON.stringify({ + status: over.status ?? "addressed", + rationale: over.rationale ?? "The diff renames the field exactly as the issue asked.", + confidence: over.confidence ?? 0.9, + }); +} + +describe("parseLinkedIssueSatisfactionOpinion", () => { + it("parses a well-formed addressed opinion", () => { + const parsed = parseLinkedIssueSatisfactionOpinion(opinionJson({ status: "addressed" })); + expect(parsed).toMatchObject({ status: "addressed", confidence: 0.9 }); + }); + + it("parses a well-formed partial opinion", () => { + const parsed = parseLinkedIssueSatisfactionOpinion(opinionJson({ status: "partial", rationale: "Fixes the crash but not the doc update." })); + expect(parsed?.status).toBe("partial"); + }); + + it("parses an unaddressed opinion when confidence clears the floor", () => { + const parsed = parseLinkedIssueSatisfactionOpinion(opinionJson({ status: "unaddressed", confidence: LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR })); + expect(parsed?.status).toBe("unaddressed"); + }); + + it("suppresses an unaddressed verdict below the confidence floor (fail-safe)", () => { + const parsed = parseLinkedIssueSatisfactionOpinion( + opinionJson({ status: "unaddressed", confidence: LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR - 0.01 }), + ); + expect(parsed).toBeNull(); + }); + + it("never suppresses addressed/partial for a low confidence (only unaddressed is floor-gated)", () => { + expect(parseLinkedIssueSatisfactionOpinion(opinionJson({ status: "addressed", confidence: 0 }))?.status).toBe("addressed"); + expect(parseLinkedIssueSatisfactionOpinion(opinionJson({ status: "partial", confidence: 0 }))?.status).toBe("partial"); + }); + + it("strips a ```json code fence before parsing", () => { + expect(parseLinkedIssueSatisfactionOpinion("```json\n" + opinionJson({ status: "partial" }) + "\n```")?.status).toBe("partial"); + }); + + it("rejects an invalid status", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "resolved", rationale: "x", confidence: 1 }))).toBeNull(); + }); + + it("rejects an empty rationale", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "addressed", rationale: "", confidence: 1 }))).toBeNull(); + }); + + it("rejects a non-string rationale", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "addressed", rationale: 12345, confidence: 1 }))).toBeNull(); + }); + + it("returns null on non-JSON text", () => { + expect(parseLinkedIssueSatisfactionOpinion("the model refused to answer")).toBeNull(); + }); + + it("returns null when a brace-shaped blob is not valid JSON (parse throws)", () => { + expect(parseLinkedIssueSatisfactionOpinion("{ status: addressed, rationale: nope }")).toBeNull(); + }); + + it("caps a long rationale", () => { + const parsed = parseLinkedIssueSatisfactionOpinion(opinionJson({ rationale: "x".repeat(1000) })); + expect(parsed?.rationale.length).toBe(400); + }); + + it("defaults confidence to 0 (lowest, not highest) when absent/unparseable", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "partial", rationale: "ok" }))?.confidence).toBe(0); + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "partial", rationale: "ok", confidence: "not-a-number" }))?.confidence).toBe(0); + }); + + it("rejects an out-of-range confidence (negative or > 1) by defaulting to 0", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "partial", rationale: "ok", confidence: -0.5 }))?.confidence).toBe(0); + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "partial", rationale: "ok", confidence: 1.5 }))?.confidence).toBe(0); + }); + + it("accepts a numeric-string confidence", () => { + expect(parseLinkedIssueSatisfactionOpinion(JSON.stringify({ status: "addressed", rationale: "ok", confidence: "0.75" }))?.confidence).toBe(0.75); + }); +}); + +describe("buildLinkedIssueSatisfactionPrompt", () => { + const base: LinkedIssueSatisfactionInput = { issueText: "Fix the crash on empty input", prTitle: "fix: guard empty input", diff: "diff --git a/x" }; + + it("omits the description line when the PR body is absent", () => { + const prompt = buildLinkedIssueSatisfactionPrompt(base); + expect(prompt).toContain("Description: (none)"); + expect(prompt).toContain("Fix the crash on empty input"); + }); + + it("includes the description when the PR body is present", () => { + const prompt = buildLinkedIssueSatisfactionPrompt({ ...base, prBody: "Adds a guard clause" }); + expect(prompt).toContain("Adds a guard clause"); + expect(prompt).not.toContain("Description: (none)"); + }); + + it("treats a whitespace-only PR body the same as absent", () => { + const prompt = buildLinkedIssueSatisfactionPrompt({ ...base, prBody: " " }); + expect(prompt).toContain("Description: (none)"); + }); + + it("treats absent issue text as empty rather than throwing", () => { + const prompt = buildLinkedIssueSatisfactionPrompt({ ...base, issueText: null }); + expect(prompt).toContain("Linked issue text:\n"); + }); +}); + +describe("buildLinkedIssueSatisfactionResult", () => { + it("returns null when the issue text is absent (fail-safe: never assessed without real issue text)", () => { + expect(buildLinkedIssueSatisfactionResult(null, opinionJson())).toBeNull(); + expect(buildLinkedIssueSatisfactionResult(undefined, opinionJson())).toBeNull(); + }); + + it("returns null when the issue text is empty/whitespace-only", () => { + expect(buildLinkedIssueSatisfactionResult(" ", opinionJson())).toBeNull(); + }); + + it("returns a public-safe result for a well-formed model response", () => { + const result = buildLinkedIssueSatisfactionResult("Fix the crash", opinionJson({ status: "addressed" })); + expect(result).toMatchObject({ status: "addressed" }); + expect(result?.rationale).toContain("renames the field"); + }); + + it("returns null when the model output does not parse (model error path)", () => { + expect(buildLinkedIssueSatisfactionResult("Fix the crash", "not json at all")).toBeNull(); + }); + + it("returns null when a below-floor unaddressed call is attempted", () => { + const result = buildLinkedIssueSatisfactionResult( + "Fix the crash", + opinionJson({ status: "unaddressed", confidence: LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR - 0.1 }), + ); + expect(result).toBeNull(); + }); + + it("returns a result for an unaddressed call that clears the floor", () => { + const result = buildLinkedIssueSatisfactionResult( + "Fix the crash", + opinionJson({ status: "unaddressed", confidence: LINKED_ISSUE_SATISFACTION_CONFIDENCE_FLOOR }), + ); + expect(result?.status).toBe("unaddressed"); + }); + + it("drops the finding when nothing survives public-safe sanitization", () => { + const result = buildLinkedIssueSatisfactionResult("Fix the crash", opinionJson({ rationale: "reward farming payout" })); + if (result) expect(result.rationale).not.toMatch(/reward|farming|payout/i); + }); + + it("is fail-safe against a thrown parse error (never throws)", () => { + // A non-string is not realistic from a caller, but guards the try/catch path defensively. + expect(() => buildLinkedIssueSatisfactionResult("Fix the crash", null as unknown as string)).not.toThrow(); + expect(buildLinkedIssueSatisfactionResult("Fix the crash", null as unknown as string)).toBeNull(); + }); +}); diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 7dec4de376..6797a5846e 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -278,6 +278,37 @@ describe("renderUnifiedReviewComment", () => { expect(withoutEffort).not.toContain("review effort:"); }); + it("renders each linked-issue satisfaction status as its own labeled section, and omits it entirely when absent (#2174)", () => { + const addressed = renderUnifiedReviewComment( + { ...base, linkedIssueSatisfaction: { status: "addressed", rationale: "The diff renames the field exactly as the issue asked." } }, + {}, + ); + expect(addressed).toContain("Linked issue satisfaction"); + expect(addressed).toContain("**Addressed**"); + expect(addressed).toContain("The diff renames the field exactly as the issue asked."); + + const partial = renderUnifiedReviewComment({ ...base, linkedIssueSatisfaction: { status: "partial", rationale: "Fixes the crash but not the doc update." } }, {}); + expect(partial).toContain("**Partially addressed**"); + + const unaddressed = renderUnifiedReviewComment({ ...base, linkedIssueSatisfaction: { status: "unaddressed", rationale: "The diff does not touch the reported crash." } }, {}); + expect(unaddressed).toContain("**Not yet addressed**"); + + // Byte-identical-when-absent: no field at all ⇒ no section, no leaked heading text. + const withoutField = renderUnifiedReviewComment({ ...base }, {}); + expect(withoutField).not.toContain("Linked issue satisfaction"); + }); + + it("omits the linked-issue satisfaction section when the rationale is empty/whitespace (defensive)", () => { + const md = renderUnifiedReviewComment({ ...base, linkedIssueSatisfaction: { status: "addressed", rationale: " " } }, {}); + expect(md).not.toContain("Linked issue satisfaction"); + }); + + it("angle-escapes the linked-issue satisfaction rationale (public-safe)", () => { + const md = renderUnifiedReviewComment({ ...base, linkedIssueSatisfaction: { status: "unaddressed", rationale: "" } }, {}); + expect(md).not.toContain("