From b2a48aba97922914f0d4f34f55b513cb04d93b70 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:05:43 -0700 Subject: [PATCH] feat(review): add an ordinal improvement/value judgment to the AI review schema Adds a `valueAssessment` field to `ModelReview` (`src/services/ai-review.ts`), populated by the SAME LLM call the reviewer already makes -- no new call, no new provider/model-selection surface. It is a genuinely different axis from `confidence`: `confidence` is calibrated defect-certainty ("how sure am I my own blockers are real"); `valueAssessment` instead asks "does this change plausibly move the codebase forward, given the diff and its stated intent." It is also not a risk judgment -- that stays the deterministic slop.ts tier, which this call never touches. The band is a small fixed ordinal (unclear/minor/moderate/significant), matching this repo's existing SlopBand convention, never a percentage. The system prompt only asks for it -- and the parser only looks for it -- when the caller resolves `input.improvementSignal` on, mirroring how inlineFindings/findingCategories/securityFocus are already caller-resolved in this file, so the disabled path spends zero extra prompt or output tokens and this file carries no new dependency on the `ConvergedFeatureKey` union. Sanitizer safety: the prompt explicitly steers the model toward "improvement/value/gain" wording and away from "score" and its sibling forbidden terms, since a sanitizer hit on AI-authored text drops the whole note rather than redacting the offending phrase. A rationale that still fails the check is dropped the same way (never surfaced, never partially redacted). New tests assert a representative set of rationale strings pass every independent public-comment sanitizer this repo relies on (isPublicSafeText, and both same-named sanitizePublicComment implementations), plus a negative control proving those assertions are meaningful. Dual-review combination: when both reviewers emit a valueAssessment, the more conservative (lower) of the two bands is surfaced, carrying that opinion's own rationale -- overclaiming a change's value is the riskier direction to err toward advisory-only. Depends on #4738 (adding `improvementSignal` as a real `ConvergedFeatureKey`) before any caller can resolve and pass this flag in production; that has not merged yet, so this PR uses a caller-resolved boolean field rather than an internal resolveConvergedFeature call, keeping it independently compilable and mergeable regardless of #4738's landing order. Implements #4743 (sub-issue of epic #4737) --- src/services/ai-review.ts | 143 ++++++++++++- test/unit/ai-review.test.ts | 393 ++++++++++++++++++++++++++++++++++++ 2 files changed, 532 insertions(+), 4 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 4377f24646..62c12d4884 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -319,6 +319,21 @@ export type GittensoryAiReviewInput = { * instruction is appended, so the prompt is byte-identical and the model emits no category. */ findingCategories?: boolean | undefined; + /** + * `improvementSignal` converged feature (#4743, LLM tier of epic #4737; config-as-code foundation in #4738) — + * when true, the reviewer is ALSO asked for an ordinal "does this change plausibly move the codebase forward" + * judgment (`valueAssessment` on `ModelReview`), a genuinely different axis from `confidence`/blockers (see + * `ModelReview.valueAssessment`'s doc comment). The CALLER resolves the feature (expected shape: + * `resolveConvergedFeature(env, manifest, "improvementSignal", repoFullName)`, #4738) and passes the resolved + * boolean here — mirroring `inlineFindings`/`findingCategories`/`securityFocus` above, all of which are + * caller-resolved rather than looked up internally, so a manifest already loaded once upstream for several + * flags is never re-fetched per-flag inside this module. (The one exception, `safety`, resolves internally via + * `convergedFeatureActive` because it is security-critical and has no upstream caller today; `improvementSignal` + * is a read-only advisory signal, not a security control, so it follows the majority pattern instead.) Absent/ + * false (the default, and the only reachable value until a caller starts resolving the feature) ⇒ no instruction + * is appended and the model is never asked — byte-identical prompt, zero extra output tokens spent. + */ + improvementSignal?: boolean | undefined; /** * This PR's changed file paths (#2558) — reused to splice a concise "changed code files with zero * test-path evidence" section into the user prompt via the engine's own deterministic classifier @@ -363,6 +378,11 @@ export type GittensoryAiReviewResult = estimatedNeurons: number; reviewerCount: number; inlineFindings: InlineFinding[]; + /** Combined improvement/value judgment (#4743), public-safe and ready to render. ALWAYS present (`null` + * when `input.improvementSignal` is falsy, when neither reviewer emitted a usable judgment, or when the + * only candidate(s) failed the public-safe check) — see {@link composeImprovementSignal} for how a dual + * review's two opinions combine into one. ADVISORY ONLY, never a gate input. */ + valueAssessment: { magnitude: ImprovementMagnitude; rationale: string } | null; reviewDiagnostics?: AiReviewDiagnostic[] | undefined; }; @@ -385,6 +405,15 @@ export type InlineFinding = { category?: FindingCategory | undefined; }; +/** + * Ordinal improvement/value band (#4743) — deliberately NOT a percentage or any fake-precise number, same + * house convention as `SlopBand` (`signals/slop.ts`: `clean/low/elevated/high`): a small named ordinal an LLM + * (or a human) can honestly stand behind. Ascending order least → most valuable: unclear < minor < moderate < + * significant. This is the LLM-JUDGED tier's axis only — the deterministic structural-improvement tier (sibling + * sub-issues of #4737) is a separate system with its own scoring. + */ +export type ImprovementMagnitude = "unclear" | "minor" | "moderate" | "significant"; + export type ModelReview = { assessment: string; // blockers = concrete must-fix defects in the diff (drive the consensus defect / gate); nits = non-blocking @@ -403,6 +432,20 @@ export type ModelReview = { // Line-anchored findings for inline PR review comments (#inline-comments). ALWAYS present (parseModelReview // sets []); populated only when the caller asked for them (input.inlineFindings) AND the model emitted any. inlineFindings: InlineFinding[]; + /** + * Ordinal improvement/value judgment (#4743) — a DIFFERENT axis from `confidence` above, not a rename of it. + * `confidence` is calibrated DEFECT-CERTAINTY: "how sure am I that MY OWN blockers are real." `valueAssessment` + * instead asks "does this change plausibly move the codebase forward, given the diff and its stated intent" — + * is it well-targeted and worth making. A defect-free change can still be low-value; a genuinely valuable change + * can still carry a real bug — the two axes are independent by design. This is also NOT a risk/safety judgment + * (that is the separate deterministic `signals/slop.ts` tier, which this call never touches, and which remains + * the ONLY thing allowed to gate). ADVISORY ONLY, same as `assessment`/`nits`/`suggestions` — never a gate input. + * Gated behind the `improvementSignal` converged feature: the prompt only asks for this field when the caller + * has resolved the feature on (`input.improvementSignal`, see its doc comment), so this is `undefined` both when + * the feature is off AND when it is on but the model omitted/mis-emitted the field — parseModelReview never + * fabricates a value or a fallback band. + */ + valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined; }; export type AiReviewDiagnostic = { @@ -698,12 +741,32 @@ export function parseModelReview(text: string): ModelReview | null { }) .slice(0, 20) : []; + // Fail-safe (#4743): a malformed/absent valueAssessment degrades to `undefined`, never a fabricated band — + // an invalid `magnitude` (not one of the 4 fixed literals) or a blank `rationale` drops the WHOLE field + // rather than keeping a half-valid judgment (mirrors toInlineFindings' item-level all-or-nothing discipline). + const toValueAssessment = ( + value: unknown, + ): { magnitude: ImprovementMagnitude; rationale: string } | undefined => { + if (!value || typeof value !== "object") return undefined; + const o = value as Record; + const magnitude = o.magnitude; + if ( + magnitude !== "unclear" && + magnitude !== "minor" && + magnitude !== "moderate" && + magnitude !== "significant" + ) + return undefined; + const rationale = typeof o.rationale === "string" ? o.rationale.trim() : ""; + return rationale ? { magnitude, rationale } : undefined; + }; const assessment = typeof obj.assessment === "string" ? obj.assessment.trim() : ""; const blockers = toList(obj.blockers); const nits = toList(obj.nits); const suggestions = toList(obj.suggestions); const inlineFindings = toInlineFindings(obj.inlineFindings); + const valueAssessment = toValueAssessment(obj.valueAssessment); // Calibrated reviewer confidence (#8): clamp the model's `confidence` to [0,1]; an absent/garbage value falls // back to 1.0 (parseReviewConfidence) so the gate degrades to the historical always-block behavior. const confidence = parseReviewConfidence(obj.confidence); @@ -715,7 +778,15 @@ export function parseModelReview(text: string): ModelReview | null { suggestions.length === 0 ) return null; - return { assessment, blockers, nits, suggestions, inlineFindings, confidence }; + return { + assessment, + blockers, + nits, + suggestions, + inlineFindings, + confidence, + ...(valueAssessment ? { valueAssessment } : {}), + }; } catch { return null; } @@ -860,10 +931,21 @@ const INLINE_FINDINGS_SUFFIX = const FINDING_CATEGORY_SUFFIX = ' Each inlineFindings item must ALSO include "category": one of exactly "security", "correctness", "performance", "maintainability", "tests", "style" — the KIND of issue, not its severity.'; +// `improvementSignal` converged feature (#4743, LLM tier of epic #4737) → an appended instruction asking for an +// ADDITIONAL, genuinely different axis: not "is this correct/safe" (blockers/nits/confidence above) and not "is +// this risky" (the separate deterministic signals/slop.ts tier, never touched by this call) but "does this change +// plausibly move the codebase forward." Absent/off (default) appends nothing (byte-identical prompt, zero extra +// output tokens). Deliberately steers the model toward "improvement"/"value"/"gain" wording and away from +// "score" and its sibling forbidden terms (#542) so the sanitizer is defended-in-depth rather than the only guard +// (see the public-safe test suite asserting representative rationale text never trips it). +const IMPROVEMENT_SIGNAL_SUFFIX = + '\n\nVALUE ASSESSMENT: ALSO include an additional top-level field "valueAssessment" in the SAME JSON object — an object of the shape {"magnitude": one of exactly "unclear", "minor", "moderate", or "significant", "rationale": ONE specific sentence}. This is a DIFFERENT question from everything above: does this change, as shown in the diff, plausibly move the codebase forward given its stated title, description, and intent — is it well-targeted and worth making? It is NOT your confidence that the change is bug-free (that is the separate "confidence" field above — a defect-free change can still be low-value, and a genuinely valuable change can still carry a real bug) and it is NOT a risk or safety judgment (a separate deterministic system handles that; do not hedge on risk here). You see only the unified diff, never the full pre-change files, so base this on the before/after hunk shape visible in the diff plus the stated intent — never claim to have compared whole files you cannot see. Use "unclear" when the diff is too small, too mechanical, or too disconnected from its stated intent to judge either way — never guess. Never use the word "score" (or reward, ranking, payout, wallet, hotkey, coldkey, trust, farming, or reviewability) to describe this judgment; describe it only in terms of improvement, value, or gain.'; + /** The effective reviewer SYSTEM prompt. Appends the grounding-discipline suffix when the caller supplied one * (flag GITTENSORY_REVIEW_GROUNDING on), the `review.profile` tone suffix when set, the `review.security_focus` - * prioritization suffix when on, then the inline-findings instruction when the caller asked for them; all absent - * (default) → the base prompt, byte-identical to today. */ + * prioritization suffix when on, then the inline-findings instruction when the caller asked for them, then the + * improvement-signal instruction when the caller resolved that feature on; all absent (default) → the base + * prompt, byte-identical to today. */ function buildSystemPrompt(input: GittensoryAiReviewInput): string { const groundingSuffix = input.grounding?.systemSuffix ?? ""; // Review-enrichment brief (#1472): the REES supplies a one-line discipline suffix ("treat a listed CVE/secret as @@ -884,7 +966,9 @@ function buildSystemPrompt(input: GittensoryAiReviewInput): string { const inlineSuffix = input.inlineFindings ? INLINE_FINDINGS_SUFFIX : ""; // review.finding_categories (#1958) only makes sense layered on top of inlineFindings itself being requested. const categorySuffix = input.inlineFindings && input.findingCategories ? FINDING_CATEGORY_SUFFIX : ""; - return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${securityFocusSuffix}${pathSuffix}${repoInstructionsSuffix}${inlineSuffix}${categorySuffix}`; + // improvementSignal (#4743): caller-resolved, exactly like inlineFindings/findingCategories above. + const improvementSignalSuffix = input.improvementSignal ? IMPROVEMENT_SIGNAL_SUFFIX : ""; + return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${securityFocusSuffix}${pathSuffix}${repoInstructionsSuffix}${inlineSuffix}${categorySuffix}${improvementSignalSuffix}`; } function buildRepoInstructionsSystemAppend(repoInstructions: string | null | undefined): string { @@ -1423,6 +1507,49 @@ export function composeInlineFindings(reviews: ModelReview[]): InlineFinding[] { return [...byLine.values()]; } +/** Ascending order for {@link ImprovementMagnitude} (#4743) — used ONLY to pick the more conservative (lower) + * of two dual-review opinions in {@link composeImprovementSignal}. Never itself surfaced, never a gate input. */ +const IMPROVEMENT_MAGNITUDE_ORDER: Record = { + unclear: 0, + minor: 1, + moderate: 2, + significant: 3, +}; + +/** + * Compose the public-safe, combined improvement/value judgment from one or two model reviews (#4743) — the + * ordinal-value counterpart of {@link composeAdvisoryNotes}. ADVISORY ONLY, never a gate input (see + * `signals/slop.ts` for the one deterministic system allowed to gate). + * + * Dual-review combination (documented behavior, #dual-ai-combiner): when BOTH reviewers emitted a + * `valueAssessment`, this takes the MORE CONSERVATIVE (lower) of the two magnitudes rather than averaging or + * surfacing both — consistent with this signal's "advisory, never overstate" posture: overclaiming a change's + * value is the riskier direction to err toward (it could nudge a maintainer to wave through something that + * is not actually well-targeted), while understating it costs nothing since a human still makes the final call. + * The rationale carried is always the ONE from whichever reviewer supplied the chosen (lower, or tied) band, so + * the cited reason matches the surfaced magnitude — never a blended sentence attributed to no one. A single + * opinion (one reviewer configured, `mode: "advisory"` which never runs a second opinion, or the other + * reviewer's call failing/omitting the field) is used as-is. Null when no reviewer emitted a usable judgment, or + * when the chosen one's rationale fails the public-safe check (dropped whole, never partially redacted — same + * fail-safe discipline as `consensusDefectOf`/`synthesizeDefect`). + */ +export function composeImprovementSignal( + reviews: ReadonlyArray, +): { magnitude: ImprovementMagnitude; rationale: string } | null { + const opinions = reviews + .map((review) => review.valueAssessment) + .filter((v): v is { magnitude: ImprovementMagnitude; rationale: string } => Boolean(v)); + if (opinions.length === 0) return null; + const chosen = opinions.reduce((lowest, candidate) => + IMPROVEMENT_MAGNITUDE_ORDER[candidate.magnitude] < IMPROVEMENT_MAGNITUDE_ORDER[lowest.magnitude] + ? candidate + : lowest, + ); + const rationale = toPublicSafe(chosen.rationale); + if (!rationale) return null; // unsafe rationale → drop the whole judgment, fail-safe (never a partial note) + return { magnitude: chosen.magnitude, rationale }; +} + /** A CONSENSUS defect = BOTH reviews independently name at least one concrete blocker (the severity-disciplined * reviewbot model: a lone blocker in a dual review is a split, not a hard block). Requiring two independent * models to AGREE is itself the precision mechanism; the calibrated confidence (#8) — a consensus is only as @@ -2181,6 +2308,12 @@ export async function runGittensoryAiReview( const inlineFindings = input.inlineFindings ? composeInlineFindings(reviewsForNotes) : []; + // Improvement/value judgment (#4743): only propagate model output when the resolved feature gate asked for it — + // same authorization discipline as inlineFindings above, and null (not computed) rather than a fallback band + // when the feature is off, so no extra work happens on the disabled path. + const valueAssessment = input.improvementSignal + ? composeImprovementSignal(reviewsForNotes) + : null; await record( env, @@ -2219,6 +2352,7 @@ export async function runGittensoryAiReview( estimatedNeurons, reviewerCount: Math.max(reviewsForNotes.length, fallbackNotes.length), inlineFindings, + valueAssessment, reviewDiagnostics, }; } @@ -2325,6 +2459,7 @@ export const __aiReviewInternals = { coerceAiText, composeAdvisoryNotes, composeInlineFindings, + composeImprovementSignal, consensusDefectOf, combineReviews, dualAiReviewersDisagree, diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 7f3eb9dcad..00603577c0 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -13,6 +13,9 @@ import { import { createTestEnv } from "../helpers/d1"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { inlineFindingCategory } from "../../src/review/inline-comments-select"; +import { isPublicSafeText } from "../../src/signals/redaction"; +import { sanitizePublicComment as sanitizePublicCommentQueueIntelligence } from "../../src/queue-intelligence"; +import { sanitizePublicComment as sanitizePublicCommentGithubCommands } from "../../src/github/commands"; const { parseModelReview, @@ -21,6 +24,7 @@ const { coerceAiText, composeAdvisoryNotes, composeInlineFindings, + composeImprovementSignal, consensusDefectOf, combineReviews, dualAiReviewersDisagree, @@ -56,6 +60,10 @@ type ModelReviewShape = { suggestions: string[]; inlineFindings: InlineFinding[]; confidence: number; + valueAssessment?: { + magnitude: "unclear" | "minor" | "moderate" | "significant"; + rationale: string; + }; }; const reviewWithFindings = ( inlineFindings: InlineFinding[], @@ -593,6 +601,69 @@ describe("review.security_focus shapes the reviewer system prompt (#review-secur }); }); +describe("review.improvement_signal shapes the reviewer system prompt (#4743)", () => { + const systemPromptOf = (run: ReturnType): string => + (run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> }) + ?.messages?.[0]?.content ?? ""; + const runImprovementSignal = async (improvementSignal: boolean | undefined) => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await runGittensoryAiReview(env, { ...baseInput, improvementSignal }); + return systemPromptOf(run); + }; + + it("true appends the VALUE ASSESSMENT instruction, naming the field and distinguishing it from confidence and from risk", async () => { + const system = await runImprovementSignal(true); + expect(system).toContain("VALUE ASSESSMENT"); + expect(system).toContain('"valueAssessment"'); + expect(system).toContain('"unclear"'); + expect(system).toContain('"significant"'); + // Explicitly distinguished from confidence (defect-certainty) and from risk (the separate slop.ts tier). + expect(system).toContain("NOT your confidence"); + expect(system).toContain("NOT a risk or safety judgment"); + // Steers the model away from the sanitizer's forbidden vocabulary and toward safe wording (#542). + expect(system).toContain('Never use the word "score"'); + expect(system).toContain("improvement, value, or gain"); + // Grounds the judgment in what the model actually receives (diff only, never full pre-change files). + expect(system).toContain("never claim to have compared whole files you cannot see"); + }); + + it("absent / false leaves the prompt byte-identical (no VALUE ASSESSMENT suffix, zero extra output tokens)", async () => { + const withFalse = await runImprovementSignal(false); + const withUndefined = await runImprovementSignal(undefined); + expect(withFalse).not.toContain("VALUE ASSESSMENT"); + expect(withUndefined).not.toContain("VALUE ASSESSMENT"); + expect(withFalse).toBe(withUndefined); + }); + + it("composes alongside every other suffix (inline findings, security focus, profile) without truncating them", async () => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await runGittensoryAiReview(env, { + ...baseInput, + improvementSignal: true, + inlineFindings: true, + securityFocus: true, + profile: "assertive", + }); + const system = systemPromptOf(run); + expect(system).toContain("VALUE ASSESSMENT"); + expect(system).toContain("INLINE FINDINGS"); + expect(system).toContain("SECURITY FOCUS"); + expect(system).toContain("ASSERTIVE"); + }); +}); + describe("runGittensoryAiReview block mode (consensus)", () => { function envWith(run: (model: string) => Promise) { return createTestEnv({ @@ -3435,6 +3506,328 @@ describe("pure helpers", () => { if (result.status === "ok") expect(result.inlineFindings).toEqual([]); }); + it("parseModelReview parses a well-formed valueAssessment for each of the 4 fixed magnitude bands (#4743)", () => { + for (const magnitude of ["unclear", "minor", "moderate", "significant"] as const) { + const json = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { + magnitude, + rationale: "This tightens an existing helper without changing its behavior.", + }, + }); + expect(parseModelReview(json)?.valueAssessment).toEqual({ + magnitude, + rationale: "This tightens an existing helper without changing its behavior.", + }); + } + }); + + it("parseModelReview drops an invalid/unrecognized magnitude — never fabricates a fallback band (#4743)", () => { + const json = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "huge", rationale: "This is a big improvement." }, + }); + expect(parseModelReview(json)?.valueAssessment).toBeUndefined(); + // The rest of the review still parses fine — an invalid valueAssessment drops ONLY that field. + expect(parseModelReview(json)?.assessment).toBe("ok"); + }); + + it("parseModelReview drops a valueAssessment with a blank or non-string rationale, keeping the rest of the review (#4743)", () => { + const blank = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "minor", rationale: " " }, + }); + expect(parseModelReview(blank)?.valueAssessment).toBeUndefined(); + expect(parseModelReview(blank)?.assessment).toBe("ok"); + + const nonStringRationale = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "minor", rationale: 42 }, + }); + expect(parseModelReview(nonStringRationale)?.valueAssessment).toBeUndefined(); + }); + + it("parseModelReview defaults valueAssessment to undefined when absent, non-object, or null (#4743)", () => { + const absent = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + }); + expect(parseModelReview(absent)?.valueAssessment).toBeUndefined(); + + const nonObject = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: "significant", + }); + expect(parseModelReview(nonObject)?.valueAssessment).toBeUndefined(); + + const nullValue = JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: null, + }); + expect(parseModelReview(nullValue)?.valueAssessment).toBeUndefined(); + }); + + describe("composeImprovementSignal (#4743, dual-review combination)", () => { + const withValue = ( + magnitude: "unclear" | "minor" | "moderate" | "significant", + rationale: string, + ): ModelReviewShape => ({ + assessment: "", + blockers: [], + nits: [], + suggestions: [], + inlineFindings: [], + confidence: 1, + valueAssessment: { magnitude, rationale }, + }); + const noValue = (): ModelReviewShape => ({ + assessment: "", + blockers: [], + nits: [], + suggestions: [], + inlineFindings: [], + confidence: 1, + }); + + it("returns null when no reviewer emitted a valueAssessment, and for an empty review list", () => { + expect(composeImprovementSignal([])).toBeNull(); + expect(composeImprovementSignal([noValue(), noValue()])).toBeNull(); + }); + + it("a single opinion (one reviewer, or the other lacks a valueAssessment) is used as-is, regardless of slot order", () => { + const solo = withValue( + "significant", + "This closes a real gap with a focused, well-tested change.", + ); + const expected = { + magnitude: "significant", + rationale: "This closes a real gap with a focused, well-tested change.", + }; + expect(composeImprovementSignal([solo])).toEqual(expected); + expect(composeImprovementSignal([solo, noValue()])).toEqual(expected); + expect(composeImprovementSignal([noValue(), solo])).toEqual(expected); + }); + + it("dual review: takes the MORE CONSERVATIVE (lower) of the two magnitudes, carrying THAT opinion's own rationale (documented #dual-ai-combiner behavior)", () => { + const bigger = withValue("significant", "Reviewer A sees a major improvement."); + const smaller = withValue("minor", "Reviewer B sees only a small, incremental gain."); + const expected = { + magnitude: "minor", + rationale: "Reviewer B sees only a small, incremental gain.", + }; + expect(composeImprovementSignal([bigger, smaller])).toEqual(expected); + // Order-independent: the lower magnitude wins regardless of which slot it occupies. + expect(composeImprovementSignal([smaller, bigger])).toEqual(expected); + }); + + it("dual review tie (equal magnitudes): keeps the first reviewer's rationale deterministically", () => { + const a = withValue("moderate", "Reviewer A's take."); + const b = withValue("moderate", "Reviewer B's take."); + expect(composeImprovementSignal([a, b])).toEqual({ + magnitude: "moderate", + rationale: "Reviewer A's take.", + }); + }); + + it("drops the whole judgment (fail-safe, never a partial/redacted note) when the chosen rationale is not public-safe", () => { + const unsafe = withValue("moderate", "This raises the trust score meaningfully."); + expect(composeImprovementSignal([unsafe])).toBeNull(); + // A dual review where the CONSERVATIVE (chosen) opinion is unsafe drops the whole judgment even though the + // other opinion alone would have been safe — never silently falls back to the other reviewer's band instead. + const safeButNotChosen = withValue("significant", "This is a well-targeted, valuable change."); + expect(composeImprovementSignal([safeButNotChosen, unsafe])).toBeNull(); + }); + }); + + it("runGittensoryAiReview surfaces the composed valueAssessment only when improvementSignal was resolved on (#4743)", async () => { + const json = JSON.stringify({ + assessment: "Looks fine.", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { + magnitude: "moderate", + rationale: "This consolidates duplicated logic into one helper.", + }, + }); + const run = vi.fn(async () => ({ response: json })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + const result = await runGittensoryAiReview(env, { + ...baseInput, + improvementSignal: true, + }); + expect(result.status).toBe("ok"); + if (result.status === "ok") + expect(result.valueAssessment).toEqual({ + magnitude: "moderate", + rationale: "This consolidates duplicated logic into one helper.", + }); + }); + + it("runGittensoryAiReview never surfaces a valueAssessment when improvementSignal is off, even if the model emitted one anyway (#4743)", async () => { + const json = JSON.stringify({ + assessment: "Looks fine.", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { + magnitude: "significant", + rationale: "Unsolicited but present in the model output.", + }, + }); + const runFor = async (improvementSignal: boolean | undefined) => { + const run = vi.fn(async () => ({ response: json })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + return runGittensoryAiReview(env, { ...baseInput, improvementSignal }); + }; + const withFalse = await runFor(false); + const withUndefined = await runFor(undefined); + expect(withFalse.status).toBe("ok"); + expect(withUndefined.status).toBe("ok"); + if (withFalse.status === "ok") expect(withFalse.valueAssessment).toBeNull(); + if (withUndefined.status === "ok") expect(withUndefined.valueAssessment).toBeNull(); + }); + + it("runGittensoryAiReview leaves valueAssessment null when improvementSignal is on but the model omitted the field", async () => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + const result = await runGittensoryAiReview(env, { + ...baseInput, + improvementSignal: true, + }); + expect(result.status).toBe("ok"); + if (result.status === "ok") expect(result.valueAssessment).toBeNull(); + }); + + it("runGittensoryAiReview dual-review (block mode) combines two valueAssessments into the more conservative band end-to-end (#4743, #dual-ai-combiner)", async () => { + const responseFor = (magnitude: string, rationale: string) => ({ + response: JSON.stringify({ + assessment: "ok", + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude, rationale }, + }), + }); + const run = vi.fn(async (model: string) => + model === BEST_REVIEW_MODELS[1] + ? responseFor("minor", "Secondary reviewer sees only a small gain.") + : responseFor("significant", "Primary reviewer sees a big win."), + ); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + const result = await runGittensoryAiReview(env, { + ...baseInput, + mode: "block", + improvementSignal: true, + }); + expect(result.status).toBe("ok"); + if (result.status === "ok") + expect(result.valueAssessment).toEqual({ + magnitude: "minor", + rationale: "Secondary reviewer sees only a small gain.", + }); + }); + + describe("valueAssessment rationale is sanitizer-safe by construction (#4743)", () => { + // Representative rationale strings a compliant model could plausibly emit for a range of PR shapes, per the + // VALUE ASSESSMENT prompt instructions (one specific sentence, "improvement/value/gain" framing, never + // "score" or its sibling forbidden terms). These must survive every independently-implemented public-comment + // sanitizer layer this repo relies on (#542) — a hit on any one silently drops the WHOLE note, not just the + // offending phrase (see `toPublicSafe`), so the prompt's own wording is the first line of defense, never the + // sanitizer alone. + const representativeRationales = [ + "This fixes a real null-dereference bug without touching unrelated code, a clear improvement.", + "This consolidates three near-duplicate helpers into one, reducing future maintenance burden.", + "This is a minor, low-risk documentation correction with limited value beyond readability.", + "This adds a complete, well-tested feature that directly addresses the linked issue's stated need.", + "The diff is too mechanical, a bulk rename, to judge its value from the shown hunks alone.", + "This is a routine dependency bump with modest value beyond staying current.", + "This adds meaningful test coverage for an existing gap, a solid but incremental gain.", + "This flips a single configuration default, a small but well-targeted improvement.", + ]; + + it("every representative rationale passes isPublicSafeText (src/signals/redaction.ts)", () => { + for (const rationale of representativeRationales) { + expect(isPublicSafeText(rationale)).toBe(true); + } + }); + + it("every representative rationale passes queue-intelligence.ts's sanitizePublicComment (throws on a hit — must not throw, and must return the text unchanged)", () => { + for (const rationale of representativeRationales) { + expect(() => sanitizePublicCommentQueueIntelligence(rationale)).not.toThrow(); + expect(sanitizePublicCommentQueueIntelligence(rationale)).toBe(rationale); + } + }); + + it("every representative rationale passes github/commands.ts's sanitizePublicComment unchanged (redacts matches in place — must not redact anything here)", () => { + for (const rationale of representativeRationales) { + expect(sanitizePublicCommentGithubCommands(rationale)).toBe(rationale); + } + }); + + it("composeImprovementSignal accepts every representative rationale end-to-end without dropping the judgment", () => { + for (const rationale of representativeRationales) { + const review: ModelReviewShape = { + assessment: "", + blockers: [], + nits: [], + suggestions: [], + inlineFindings: [], + confidence: 1, + valueAssessment: { magnitude: "moderate", rationale }, + }; + expect(composeImprovementSignal([review])).toEqual({ magnitude: "moderate", rationale }); + } + }); + + it("negative control: a rationale that ignores the prompt's guidance and uses forbidden vocabulary DOES trip every sanitizer (proves the assertions above are meaningful, not vacuous)", () => { + const unsafe = "This raises the trust score and improves the reward payout."; + expect(isPublicSafeText(unsafe)).toBe(false); + expect(() => sanitizePublicCommentQueueIntelligence(unsafe)).toThrow(); + expect(sanitizePublicCommentGithubCommands(unsafe)).not.toBe(unsafe); + }); + }); + it("composeAdvisoryNotes renders only the sections that have public-safe content", () => { const review = ( over: Partial<{