diff --git a/.loopover.yml.example b/.loopover.yml.example index d06ce6422e..05b93477c5 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -413,6 +413,14 @@ gate: # Number 0-1, or null. Default: null (engine uses 0.93). Config-as-code # only — no DB column or dashboard toggle; this can only be set here. closeConfidence: null + # #8962: 0-100 salvageability floor. When set, an AT/ABOVE-closeConfidence + # consensus/split defect on a PR whose deterministic salvageability score + # (fixable defect class + author's merged history here + in-flight + # iteration) clears this floor is HELD with fix-it guidance instead of + # one-shot-closed — "real defect, salvageable PR" is the residual close + # error class. Number 0-100, or null. Default: null (axis off; behavior + # identical to today). Config-as-code only. + salvageabilityMinScore: null # Disposition for a sub-closeConfidence-floor consensus/split defect (#4603). # one_shot — ignore the floor; always one-shot-close (today's # pre-#4603 behavior). Opt-in only. diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index b05be15b6e..9b71b72fba 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -9974,6 +9974,10 @@ "closeAuditHoldoutPct": { "type": "number", "nullable": true + }, + "aiReviewSalvageabilityMinScore": { + "type": "number", + "nullable": true } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 6b03c90bc5..416a733173 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -427,6 +427,14 @@ gate: # Number 0-1, or null. Default: null (engine uses 0.93). Config-as-code # only — no DB column or dashboard toggle; this can only be set here. closeConfidence: null + # #8962: 0-100 salvageability floor. When set, an AT/ABOVE-closeConfidence + # consensus/split defect on a PR whose deterministic salvageability score + # (fixable defect class + author's merged history here + in-flight + # iteration) clears this floor is HELD with fix-it guidance instead of + # one-shot-closed — "real defect, salvageable PR" is the residual close + # error class. Number 0-100, or null. Default: null (axis off; behavior + # identical to today). Config-as-code only. + salvageabilityMinScore: null # Disposition for a sub-closeConfidence-floor consensus/split defect (#4603). # one_shot — ignore the floor; always one-shot-close (today's # pre-#4603 behavior). Opt-in only. diff --git a/docs/decision-audit-rubric.md b/docs/decision-audit-rubric.md index a97e73c879..232448561d 100644 --- a/docs/decision-audit-rubric.md +++ b/docs/decision-audit-rubric.md @@ -34,6 +34,9 @@ would have made, given only what was knowable at decision time?** - `stale_signal` — decided on out-of-date CI/conflict/issue state. - `scope_misread` — misjudged linked-issue scope or requirements. - `policy_misapplied` — an enforcement rule fired on a case it should not cover. +- `salvageable_close` — the defect was real but the PR was salvageable (fixable class, responsive author); + closing denied a contribution a hold-with-guidance would have landed (#8962). Additive category — the + correct/incorrect question is unchanged, so rubric version 1 labels remain comparable. - `other` — anything else; describe in the adjudication notes if used. ## Workflow diff --git a/package-lock.json b/package-lock.json index 3f6a09daf9..837c5ba41e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4084,22 +4084,6 @@ "node": ">=8" } }, - "node_modules/@posthog/cli/node_modules/prettier": { - "version": "3.8.3", - "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.8.3.tgz", - "integrity": "sha512-7igPTM53cGHMW8xWuVTydi2KO233VFiTNyF5hLJqpilHfmn8C8gPf+PS7dUT64YcXFbiMGZxS9pCSxL/Dxm/Jw==", - "extraneous": true, - "license": "MIT", - "bin": { - "prettier": "bin/prettier.cjs" - }, - "engines": { - "node": ">=14" - }, - "funding": { - "url": "https://github.com/prettier/prettier?sponsor=1" - } - }, "node_modules/@posthog/core": { "version": "1.42.1", "resolved": "https://registry.npmjs.org/@posthog/core/-/core-1.42.1.tgz", @@ -21045,7 +21029,7 @@ }, "packages/loopover-engine": { "name": "@loopover/engine", - "version": "3.14.1", + "version": "3.15.0", "license": "AGPL-3.0-only", "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.3.205", diff --git a/packages/loopover-engine/package.json b/packages/loopover-engine/package.json index a30811752d..5c3d191887 100644 --- a/packages/loopover-engine/package.json +++ b/packages/loopover-engine/package.json @@ -1,6 +1,6 @@ { "name": "@loopover/engine", - "version": "3.14.1", + "version": "3.15.0", "license": "AGPL-3.0-only", "type": "module", "description": "Shared deterministic engine logic for the LoopOver review stack and loopover-miner.", diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index 392b1c5e89..d618638da0 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -122,6 +122,11 @@ export type FocusManifestGateConfig = { /** `gate.aiReview.closeConfidence` (#7): minimum calibrated AI-reviewer confidence (0-1) for an AI defect to BLOCK * under `aiReview.mode: block`. null (unset) ⇒ the gate's 0.93 default. Clamped to [0,1] at parse time. */ aiReviewCloseConfidence: number | null; + /** `gate.aiReview.salvageabilityMinScore` (#8962): 0-100 floor on the deterministic salvageability score + * at/above which an at-floor AI-judgment close is routed to hold-with-guidance instead ("real defect, + * salvageable PR" — the residual close-error class the 2026-07 decision audit identified). null (unset, + * the default) ⇒ the salvageability axis never changes a disposition. Manifest-only. */ + aiReviewSalvageabilityMinScore: number | null; /** `gate.aiReview.lowConfidenceDisposition` (#4603): disposition for a sub-`closeConfidence`-floor * `ai_consensus_defect`/`ai_review_split` finding. null (unset) ⇒ `hold_for_review` (the shipped default). * DB-backed (dashboard-settable too, via the `/ai-review` route); this overrides the stored value -- mirrors @@ -1319,6 +1324,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, + aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, @@ -1810,6 +1816,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu aiReviewModel: normalizeOptionalString(aiReviewRecord?.model, "gate.aiReview.model", warnings), aiReviewAllAuthors: normalizeOptionalBoolean(aiReviewRecord?.allAuthors, "gate.aiReview.allAuthors", warnings), aiReviewCloseConfidence: normalizeOptionalConfidence(aiReviewRecord?.closeConfidence, "gate.aiReview.closeConfidence", warnings), + aiReviewSalvageabilityMinScore: normalizeOptionalScore(aiReviewRecord?.salvageabilityMinScore, "gate.aiReview.salvageabilityMinScore", warnings), aiReviewLowConfidenceDisposition: normalizeOptionalEnum( aiReviewRecord?.lowConfidenceDisposition, "gate.aiReview.lowConfidenceDisposition", @@ -1874,6 +1881,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.aiReviewModel !== null || gate.aiReviewAllAuthors !== null || gate.aiReviewCloseConfidence !== null || + gate.aiReviewSalvageabilityMinScore !== null || gate.aiReviewLowConfidenceDisposition !== null || gate.aiReviewCombine !== null || gate.aiReviewOnMerge !== null || @@ -1941,6 +1949,7 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { gate.aiReviewModel !== null || gate.aiReviewAllAuthors !== null || gate.aiReviewCloseConfidence !== null || + gate.aiReviewSalvageabilityMinScore !== null || gate.aiReviewLowConfidenceDisposition !== null || gate.aiReviewCombine !== null || gate.aiReviewOnMerge !== null || @@ -1953,6 +1962,7 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { if (gate.aiReviewModel !== null) aiReview.model = gate.aiReviewModel; if (gate.aiReviewAllAuthors !== null) aiReview.allAuthors = gate.aiReviewAllAuthors; if (gate.aiReviewCloseConfidence !== null) aiReview.closeConfidence = gate.aiReviewCloseConfidence; + if (gate.aiReviewSalvageabilityMinScore !== null) aiReview.salvageabilityMinScore = gate.aiReviewSalvageabilityMinScore; if (gate.aiReviewLowConfidenceDisposition !== null) aiReview.lowConfidenceDisposition = gate.aiReviewLowConfidenceDisposition; if (gate.aiReviewCombine !== null) aiReview.combine = gate.aiReviewCombine; if (gate.aiReviewOnMerge !== null) aiReview.onMerge = gate.aiReviewOnMerge; diff --git a/packages/loopover-miner/expected-engine.version b/packages/loopover-miner/expected-engine.version index 49547fdca4..f02113fe87 100644 --- a/packages/loopover-miner/expected-engine.version +++ b/packages/loopover-miner/expected-engine.version @@ -1 +1 @@ -3.14.1 +3.15.0 diff --git a/scripts/check-docs-drift.ts b/scripts/check-docs-drift.ts index b5cf603661..51b043f8ce 100644 --- a/scripts/check-docs-drift.ts +++ b/scripts/check-docs-drift.ts @@ -175,6 +175,7 @@ export const SETTINGS_ALIAS_MANIFEST: AliasManifestRow[] = [ { field: "aiReviewModel", aliases: ["aiReview:"] }, { field: "aiReviewAllAuthors", aliases: ["allAuthors"] }, { field: "aiReviewCloseConfidence", aliases: ["closeConfidence"] }, + { field: "aiReviewSalvageabilityMinScore", aliases: ["salvageabilityMinScore"] }, { field: "aiReviewLowConfidenceDisposition", aliases: ["lowConfidenceDisposition"] }, { field: "aiReviewCombine", aliases: ["aiReview:"] }, { field: "aiReviewOnMerge", aliases: ["onMerge"] }, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index b75cc37d54..16ca01b590 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -785,6 +785,7 @@ export const RepositorySettingsSchema = z aiReviewAllAuthors: z.boolean(), aiReviewConfirmedContributorsOnly: z.boolean().nullable().optional(), aiReviewCloseConfidence: z.number().nullable().optional(), + aiReviewSalvageabilityMinScore: z.number().nullable().optional(), aiReviewLowConfidenceDisposition: z.enum(["one_shot", "hold_for_review", "advisory_only"]).nullable().optional(), aiReviewCombine: z.enum(["single", "consensus", "synthesis"]).nullable().optional(), aiReviewOnMerge: z.enum(["either", "both"]).nullable().optional(), diff --git a/src/queue/gate-checks.ts b/src/queue/gate-checks.ts index 4654b49eea..552eef792e 100644 --- a/src/queue/gate-checks.ts +++ b/src/queue/gate-checks.ts @@ -93,6 +93,7 @@ export function gateCheckPolicy( aiReviewCloseConfidence: settings.aiReviewCloseConfidence ?? (typeof aiReviewCloseConfidenceOverride === "number" ? aiReviewCloseConfidenceOverride : (aiReviewCloseConfidenceOverride?.value ?? null)), + aiReviewSalvageabilityMinScore: settings.aiReviewSalvageabilityMinScore ?? null, // #8849: provenance for the low-confidence hold copy — true ONLY when the floor in force actually came // from a live calibration (an explicit repo setting suppresses it). aiReviewCloseConfidenceCalibrated: diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 58aa2ac81b..70cc8736be 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -198,6 +198,7 @@ import { recordConfiguredGateBlockerSignals, recordGateScoreSignals, resolveAiReviewLowConfidenceHold, + resolveAiReviewSalvageableHold, } from "../rules/advisory"; import { hasValidationNote, isTestPath } from "../signals/test-evidence"; import { detectNotificationEvents } from "../notifications/events"; @@ -645,6 +646,7 @@ import { recordReversalSignals, } from "../review/outcomes-wire"; import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; +import { computeSalvageabilityForTarget } from "../review/salvageability-wire"; import { REVIEW_PROMPT_VERSION, REVIEW_SYSTEM_PROMPT } from "../services/ai-review"; import { resolveAutomaticCloseConfidence } from "../review/risk-control-wire"; import { maybeApplyCloseAuditHoldout } from "../review/close-audit-holdout"; @@ -3192,6 +3194,13 @@ async function runAgentMaintenancePlanAndExecute( // gate failed SOLELY on a sub-aiReviewCloseConfidence-floor ai_consensus_defect/ai_review_split finding under // the (default) hold_for_review disposition. See resolveAiReviewLowConfidenceHold's own doc comment. const aiReviewLowConfidenceHold = resolveAiReviewLowConfidenceHold(gate, settings); + // #8962 salvageability hold — the OTHER side of the floor: an at/above-floor AI-judgment close routed to + // hold-with-guidance when the deterministic salvageability score clears gate.aiReview.salvageabilityMinScore. + // Knob unset (the default) short-circuits before any IO; the low-confidence hold keeps precedence. + const aiReviewSalvageableHold = + aiReviewLowConfidenceHold === undefined && settings.aiReviewSalvageabilityMinScore != null + ? resolveAiReviewSalvageableHold(gate, settings, await computeSalvageabilityForTarget(env, repoFullName, pr.number, pr.authorLogin, gate)) + : undefined; const planned = planAgentMaintenanceActions( buildAgentMaintenancePlanInput({ gate, @@ -3210,7 +3219,7 @@ async function runAgentMaintenancePlanAndExecute( linkedIssueRulesConfig, migrationCollisionHold, unlinkedIssueMatchHold, - aiReviewLowConfidenceHold, + aiReviewLowConfidenceHold: aiReviewLowConfidenceHold ?? aiReviewSalvageableHold, unlinkedIssueMatchClose, liveMergeState, liveReviewDecision, @@ -3335,6 +3344,9 @@ async function runAgentMaintenancePlanAndExecute( // worse than recording nothing (the reviewDiagnostics ledger holds the per-run model identities). { const aiJudgment = gate.blockers.find((blocker) => AI_JUDGMENT_BLOCKER_CODES.has(blocker.code)); + // #8962: recomputed here (two cheap reads, only when an AI judgment shaped the decision) rather than + // threaded from the plan site — the record must carry the boundary evidence even when the knob is off. + const salvageability = aiJudgment !== undefined ? await computeSalvageabilityForTarget(env, repoFullName, pr.number, pr.authorLogin, gate) : null; const { record, recordDigest } = await buildDecisionRecord({ repoFullName, pullNumber: pr.number, @@ -3353,6 +3365,7 @@ async function runAgentMaintenancePlanAndExecute( modelId: null, promptDigest: aiJudgment !== undefined ? await contentDigest({ version: REVIEW_PROMPT_VERSION, template: REVIEW_SYSTEM_PROMPT }) : null, aiConfidence: aiJudgment?.confidence ?? null, + salvageability, }); await persistDecisionRecord(env, record, recordDigest); } diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index 64606af08c..57d9e466f9 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -14,7 +14,7 @@ import { errorMessage, nowIso } from "../utils/json"; /** Bump when the record's FIELD SET changes meaning — consumers compare records only within a version. */ -export const DECISION_RECORD_SCHEMA_VERSION = "2"; // v2 (#8834): + aiConfidence, model/prompt commitments live +export const DECISION_RECORD_SCHEMA_VERSION = "3"; // v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments /** * Canonical JSON: recursively key-sorted, no insignificant whitespace — the ONE serialization every digest @@ -76,18 +76,23 @@ export type DecisionRecord = { * defect / split), null when no AI judgment contributed. Persisted so every decision joins the * risk-control calibration set (#8835) with its confidence attached. */ aiConfidence: number | null; + /** #8962: the deterministic salvageability score + its named factors when an AI judgment shaped the + * decision — the second-axis evidence for auditing the close/hold boundary. null for rule-only decisions + * (and for reconstructed/backfilled records predating v3). */ + salvageability: { score: number; factors: string[] } | null; decidedAt: string; }; /** Assemble the record and its own content digest. PURE given pre-computed digests. Normalizes the * optional-shaped caller fields (undefined -> null) HERE so call sites carry no fallback arms of their own. */ export async function buildDecisionRecord( - input: Omit & { + input: Omit & { decidedAt?: string; gatePack?: string | null | undefined; ciState?: string | null | undefined; baseSha?: string | null | undefined; aiConfidence?: number | null | undefined; + salvageability?: { score: number; factors: string[] } | null | undefined; }, ): Promise<{ record: DecisionRecord; recordDigest: string }> { const record: DecisionRecord = { @@ -98,6 +103,7 @@ export async function buildDecisionRecord( ciState: input.ciState ?? null, baseSha: input.baseSha ?? null, aiConfidence: input.aiConfidence ?? null, + salvageability: input.salvageability ?? null, }; return { record, recordDigest: await contentDigest(record) }; } diff --git a/src/review/salvageability-wire.ts b/src/review/salvageability-wire.ts new file mode 100644 index 0000000000..1dc6ba47c6 --- /dev/null +++ b/src/review/salvageability-wire.ts @@ -0,0 +1,44 @@ +// Salvageability wire (#8962) — the two cheap decision-time lookups around src/review/salvageability.ts's +// pure score: the author's realized merged history in this repo and this PR's own review-cycle count. +// FAIL-OPEN by contract: any read error returns null, and a null score never changes a disposition — the +// salvageability axis must degrade to today's behavior, never block or unblock the gate on a DB blip. +import { computeSalvageability, type SalvageabilityScore } from "./salvageability"; +import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation } from "../rules/advisory"; +import { errorMessage } from "../utils/json"; + +export async function computeSalvageabilityForTarget( + env: Env, + repoFullName: string, + prNumber: number, + authorLogin: string | null | undefined, + gate: Pick, +): Promise { + const blocker = gate.blockers.find((finding) => AI_JUDGMENT_BLOCKER_CODES.has(finding.code)); + if (!blocker) return null; + try { + // Realized MERGED outcomes only (the #8840 lesson: open/cached PRs are not history). Same-repo scoped — + // salvageability is a statement about this repository's bar, not global reputation. + const merged = authorLogin + ? await env.DB.prepare( + `SELECT COUNT(*) AS n FROM pull_requests p + WHERE LOWER(p.author_login) = LOWER(?) AND p.repo_full_name = ? + AND EXISTS (SELECT 1 FROM review_audit ra + WHERE ra.event_type = 'pr_outcome' AND ra.decision = 'merged' + AND ra.target_id = p.repo_full_name || '#' || p.number)`, + ) + .bind(authorLogin, repoFullName) + .first<{ n: number }>() + : null; + const cycles = await env.DB.prepare(`SELECT COUNT(*) AS n FROM decision_records WHERE repo_full_name = ? AND pull_number = ?`) + .bind(repoFullName, prNumber) + .first<{ n: number }>(); + return computeSalvageability({ + findingText: `${blocker.title} ${blocker.detail}`, + authorPriorMergedCount: merged?.n ?? 0, + priorReviewCycles: cycles?.n ?? 0, + }); + } catch (error) { + console.warn(JSON.stringify({ event: "salvageability_read_error", target: `${repoFullName}#${prNumber}`, message: errorMessage(error).slice(0, 120) })); + return null; + } +} diff --git a/src/review/salvageability.ts b/src/review/salvageability.ts new file mode 100644 index 0000000000..b6088058e5 --- /dev/null +++ b/src/review/salvageability.ts @@ -0,0 +1,88 @@ +// Salvageability (#8962, epic #8828) — the second axis of the close decision, PURE. +// +// The 2026-07-26 decision audit proved defect-confidence alone cannot push close precision past ~96%: 28 +// contributor PRs held on REAL, high-confidence findings (0.55–0.97) later MERGED — the residual close-error +// class is "real defect, salvageable PR". Confidence measures whether the defect is real; salvageability +// measures whether closing is the right ACTION anyway. This module scores the second question from +// deterministic decision-time facts only — no AI call, every point traceable to a named factor. +// +// Consumed by resolveAiReviewSalvageableHold (src/rules/advisory.ts): when an AI-judgment close WOULD fire +// (confidence at/above the floor) but the score clears the repo's `gate.aiReview.salvageabilityMinScore` +// manifest knob, the close is routed into the existing held-for-review path with fix-it guidance instead. +// The knob is manifest-only and ABSENT by default — unset ⇒ this module never changes a disposition. + +/** How mechanically fixable the flagged defect class is, judged from the finding's own text. */ +export type DefectFixability = "mechanical" | "structural" | "unknown"; + +/** Textual signatures of defect classes an author routinely fixes in one push — drawn from the audited + * held-then-fixed-then-merged cohort (encoding artifacts, dead imports, stale generated artifacts, version + * mismatches, missing guards/coverage on otherwise-sound changes). */ +const MECHANICAL_PATTERN = + /\bmojibake\b|\bencoding\b|\bunused import\b|\bdead import\b|\bregenerat|\bstale (?:generated|artifact|bundle|contract)|\bversion (?:bump|mismatch|downgrade)|\bmissing (?:test|branch|unit) coverage\b|\bcodecov\/patch\b|\buncovered\b|\bprettier\b|\bformatting\b|\btrim\(\)|\bnull(?:ish)? (?:guard|check)\b|\bblank[- ](?:cell|netuid|stake)\b/i; + +/** Textual signatures of disqualifying-by-shape classes: the PR itself is the problem, not one fixable + * defect — fabricated evidence, scope violations, duplication, undisclosed bundling, policy breaches. */ +const STRUCTURAL_PATTERN = + /\bfabricat|\bmanufactur(?:es?|ed) (?:a |the )?(?:payload|state|test)|\bscope (?:creep|violation)\b|\bunrelated (?:change|feature|refactor|file|deletion)\b|\b(?:bundles?|bundled|bundling)\b[^.]{0,60}\bunrelated\b|\bunrelated\b[^.]{0,60}\b(?:bundles?|bundled|bundling)\b|\bundisclosed\b|\bduplicat|\bno (?:linked|eligible)[^.]{0,24}\bissue\b|\bissue-scope\b|\bdeletes? (?:working|existing)\b|\bout of scope\b|\bsingle-file\b|\bappend-only\b|\bspeculat/i; + +/** Classify the flagged defect's fixability from the finding text. Structural wins when both match — a + * mechanical-sounding detail inside a scope-violation finding does not make the PR salvageable. PURE. */ +export function classifyDefectFixability(findingText: string): DefectFixability { + if (STRUCTURAL_PATTERN.test(findingText)) return "structural"; + if (MECHANICAL_PATTERN.test(findingText)) return "mechanical"; + return "unknown"; +} + +export type SalvageabilityInput = { + /** The blocking AI-judgment finding's text (title + detail) — fixability is judged from it. */ + findingText: string; + /** The author's previously MERGED PRs in this repo (realized outcomes, not open PRs — the #8840 lesson). */ + authorPriorMergedCount: number; + /** Distinct review cycles this PR has already been through (decision records per head sha): an author who + * is iterating has demonstrated responsiveness on THIS change. */ + priorReviewCycles: number; +}; + +export type SalvageabilityScore = { + /** 0–100; compare against `gate.aiReview.salvageabilityMinScore`. */ + score: number; + /** The named factors behind every point — persisted with the decision so the boundary is auditable. */ + factors: string[]; + fixability: DefectFixability; +}; + +/** + * Deterministic salvageability score. Weights encode the audit's evidence, not tuning: proven-landing + * authors fix things (the held-then-merged cohort skewed heavily toward authors with merged history), a + * mechanical defect class is one push from green, and in-flight iteration is direct evidence of + * responsiveness. A structural defect class zeroes the fixability contribution BY DESIGN — no author + * reputation makes a scope violation or fabricated test salvageable-in-place. PURE and total. + */ +export function computeSalvageability(input: SalvageabilityInput): SalvageabilityScore { + const fixability = classifyDefectFixability(input.findingText); + const factors: string[] = []; + let score = 0; + if (fixability === "mechanical") { + score += 45; + factors.push("mechanical defect class (+45)"); + } else if (fixability === "unknown") { + score += 15; + factors.push("unclassified defect class (+15)"); + } else { + factors.push("structural defect class (+0)"); + } + if (input.authorPriorMergedCount >= 3) { + score += 40; + factors.push(`author has ${input.authorPriorMergedCount} merged PRs here (+40)`); + } else if (input.authorPriorMergedCount >= 1) { + score += 25; + factors.push(`author has ${input.authorPriorMergedCount} merged PR(s) here (+25)`); + } else { + factors.push("no merged history in this repo (+0)"); + } + if (input.priorReviewCycles >= 2) { + score += 15; + factors.push(`already iterating (${input.priorReviewCycles} review cycles) (+15)`); + } + return { score: Math.min(100, score), factors, fixability }; +} diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index c6cef3b101..49cd74fd0c 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -60,6 +60,9 @@ export type GateCheckPolicy = { /** #8849: true when the floor above came from a LIVE risk-control calibration (not a static setting or the * backtest loosening) — the low-confidence hold names its source so a held contributor sees why. */ aiReviewCloseConfidenceCalibrated?: boolean | undefined; + /** #8962: 0-100 salvageability floor (manifest-only, gate.aiReview.salvageabilityMinScore); null/absent = + * the salvageability axis is OFF and never changes a disposition. */ + aiReviewSalvageabilityMinScore?: number | null | undefined; /** Disposition for a sub-floor `ai_consensus_defect`/`ai_review_split` finding (#4603) — see the type's own doc * comment (`src/types.ts`) for the full semantics of `one_shot` / `hold_for_review` / `advisory_only`. * `null`/undefined ⇒ `hold_for_review` (the shipped default). Only `advisory_only` changes what @@ -250,6 +253,35 @@ export function resolveAiReviewLowConfidenceHold( }; } +/** + * #8962 — the salvageability hold, resolveAiReviewLowConfidenceHold's sibling for the OTHER side of the + * floor: an AI-judgment failure whose blockers all sit AT/ABOVE the close-confidence floor (a close WOULD + * fire) but whose deterministic salvageability score clears the repo's `gate.aiReview.salvageabilityMinScore` + * manifest knob ("real defect, salvageable PR" — the residual close-error class the 2026-07 decision audit + * identified). Routes the close into the same held-for-manual-review path, with fix-it guidance. Returns + * undefined (no hold) when the knob is unset (the default — behavior byte-identical), the failure isn't + * AI-judgment-only, any blocker sits below the floor (the low-confidence hold owns that case and must keep + * precedence), or the score is under the knob. PURE — the caller computes the score. + */ +export function resolveAiReviewSalvageableHold( + evaluation: GateCheckEvaluation, + policy: Pick, + salvageability: { score: number; factors: string[] } | null, +): { reason: string; comment: string } | undefined { + const minScore = policy.aiReviewSalvageabilityMinScore; + if (minScore == null || salvageability === null) return undefined; + if ((policy.aiReviewLowConfidenceDisposition ?? "hold_for_review") !== "hold_for_review") return undefined; + if (!isAiJudgmentOnlyFailure(evaluation)) return undefined; + const floor = policy.aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE; + if (evaluation.blockers.some((blocker) => (blocker.confidence ?? 1) < floor)) return undefined; + if (salvageability.score < minScore) return undefined; + return { + reason: `the flagged defect looks fixable and the author's record clears the salvageability floor (${salvageability.score} ≥ ${minScore}: ${salvageability.factors.join("; ")})`, + comment: + "An AI reviewer flagged a likely defect with high confidence, but this PR scored as salvageable (fixable defect class and/or a proven author track record), so it is HELD with guidance instead of closed: push a fix addressing the flagged defect and the next review cycle re-evaluates it. A maintainer can still close or merge manually at any time.", + }; +} + // DUPLICATE-ONLY blocker codes: findings whose own severity is always "warning" (advisory by nature — a // same-linked-issue overlap is a lead for a human, not proof of a defect) but that a per-repo gate-mode config // can still escalate into a hard blocker (`duplicate_pr_risk` under `duplicatePrGateMode: "block"`, its ONLY diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 586d79837b..73bfb63a7f 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -512,6 +512,7 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani if (gate.aiReviewModel !== null) effective.aiReviewModel = gate.aiReviewModel; if (gate.aiReviewAllAuthors !== null) effective.aiReviewAllAuthors = gate.aiReviewAllAuthors; if (gate.aiReviewCloseConfidence !== null) effective.aiReviewCloseConfidence = gate.aiReviewCloseConfidence; + if (gate.aiReviewSalvageabilityMinScore !== null) effective.aiReviewSalvageabilityMinScore = gate.aiReviewSalvageabilityMinScore; if (gate.aiReviewLowConfidenceDisposition !== null) effective.aiReviewLowConfidenceDisposition = gate.aiReviewLowConfidenceDisposition; // Dual-AI combine/onMerge/reviewers overrides (#2567) are projected onto `effective` unclamped here — they are // a REFINEMENT of the operator's AI_REVIEW_PLAN, not a replacement for it, so the actual operator-floor clamp diff --git a/src/types.ts b/src/types.ts index c62cd2de1c..e3ee654961 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1059,6 +1059,10 @@ export type RepositorySettings = { * ignores the floor entirely. Config-as-code only — set via `.loopover.yml gate.aiReview.closeConfidence` * (no dashboard/DB column); unset ⇒ the gate uses the 0.93 default. Clamped to [0,1] at parse time. */ aiReviewCloseConfidence?: number | null | undefined; + /** #8962: 0-100 salvageability floor at/above which an at-confidence-floor AI-judgment close is held with + * fix-it guidance instead ("real defect, salvageable PR"). Manifest-only (gate.aiReview.salvageabilityMinScore); + * absent/null ⇒ the salvageability axis never changes a disposition. */ + aiReviewSalvageabilityMinScore?: number | null | undefined; /** Disposition for a sub-floor `ai_consensus_defect`/`ai_review_split` finding (#4603) — see * {@link AiReviewLowConfidenceDisposition} for the full semantics of each value. Default `"hold_for_review"`. * Unlike {@link aiReviewCloseConfidence}, this IS DB-backed/dashboard-settable (via the `/ai-review` route, diff --git a/test/unit/backfill-decision-labels-core.test.ts b/test/unit/backfill-decision-labels-core.test.ts index 8aa566dea7..d4247604ce 100644 --- a/test/unit/backfill-decision-labels-core.test.ts +++ b/test/unit/backfill-decision-labels-core.test.ts @@ -115,7 +115,7 @@ describe("buildBundle", () => { expect(await contentDigest(record)).toBe(row.record_digest); expect(canonicalJson(record)).toBe(row.record_json); expect(record.configDigest).toBe("backfill:unavailable"); - expect(record.schemaVersion).toBe("2"); + expect(record.schemaVersion).toBe("3"); // v3 (#8962): + salvageability } expect(records[0]).toMatchObject({ id: "record:o/r#1@sha1", action: "close", created_at: "2026-07-01T00:00:00.000Z" }); expect(records[1]).toMatchObject({ action: "hold" }); diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index b972182d48..f91fa4b858 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -51,6 +51,7 @@ function recordInput(over: Partial = {}): Omit { aiReviewModel: "model:", aiReviewAllAuthors: "allAuthors:", aiReviewCloseConfidence: "closeConfidence:", + aiReviewSalvageabilityMinScore: "salvageabilityMinScore:", aiReviewLowConfidenceDisposition: "lowConfidenceDisposition:", aiReviewCombine: "combine:", aiReviewOnMerge: "onMerge:", @@ -955,7 +956,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, + gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null, amsReputationBridge: null }, @@ -1148,7 +1149,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); + expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); }); it("parses gate.mergeReadiness, round-trips it, and warns on a bad value (#822)", () => { @@ -1467,6 +1468,15 @@ describe("parseFocusManifest gate config", () => { expect(resolveEffectiveSettings({ aiReviewAllAuthors: true , closeOwnerAuthors: false} as unknown as RepositorySettings, noFlag).aiReviewAllAuthors).toBe(true); }); + it("#8962: parses gate.aiReview.salvageabilityMinScore (0-100 score), round-trips, projects onto effective settings", () => { + const m = parseFocusManifest({ gate: { aiReview: { salvageabilityMinScore: 60 } } }); + expect(m.gate.present).toBe(true); + expect(m.gate.aiReviewSalvageabilityMinScore).toBe(60); + expect((gateConfigToJson(m.gate) as { aiReview: { salvageabilityMinScore: number } }).aiReview.salvageabilityMinScore).toBe(60); + expect(parseFocusManifest({ gate: gateConfigToJson(m.gate) }).gate).toEqual(m.gate); + expect(parseFocusManifest({ gate: { aiReview: { salvageabilityMinScore: "high" } } }).warnings.some((w) => /salvageabilityMinScore/.test(w))).toBe(true); + }); + it("parses gate.aiReview.closeConfidence, clamps to [0,1], makes the gate present, round-trips + resolves it, and warns on a bad value (#7)", () => { // closeConfidence alone makes the gate present, serializes back under gate.aiReview.closeConfidence, and the // gate alias projects it onto effective settings. diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index c1eed863f9..e147e3b552 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -27,6 +27,9 @@ describe("gateCheckPolicy — #8176 global close-confidence default-override", ( expect(gateCheckPolicy(settings(), null, undefined, null, undefined, undefined, 0.9).aiReviewCloseConfidence).toBe(0.9); // Explicit per-repo setting: the override must NEVER displace it. expect(gateCheckPolicy(settings({ aiReviewCloseConfidence: 0.97 }), null, undefined, null, undefined, undefined, 0.9).aiReviewCloseConfidence).toBe(0.97); + // #8962: the salvageability floor passes through verbatim; absent stays null (axis off). + expect(gateCheckPolicy(settings({ aiReviewSalvageabilityMinScore: 60 }), null, undefined, null, undefined, undefined, 0.9).aiReviewSalvageabilityMinScore).toBe(60); + expect(gateCheckPolicy(settings(), null, undefined, null, undefined, undefined, 0.9).aiReviewSalvageabilityMinScore).toBeNull(); // Neither: null, so advisory.ts applies its shipped default. expect(gateCheckPolicy(settings(), null, undefined, null, undefined, undefined, null).aiReviewCloseConfidence).toBeNull(); // #8849: the provenance-carrying shape — a calibrated λ̂ sets the floor AND flags its source; an diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 0d53daa52a..b7a03f73ee 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -309,6 +309,70 @@ describe("queue processors", () => { expect(pr8?.state).toBe("open"); }); + it("#8962: an AT-floor defect on a salvageable PR is HELD with guidance instead of one-shot-closed; the v3 record carries the score", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); + await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { close: "auto" }, gatePack: "oss-anti-slop" }); + // The knob rides the MANIFEST (config-as-code only) — this exercises parse -> overlay -> policy end to end. + await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required", aiReviewMode: "block" }, gate: { aiReview: { salvageabilityMinScore: 60 } } }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 18, title: "Salvageable defect PR", state: "open", user: { login: "contributor" }, head: { sha: "d18" }, labels: [], body: "Closes #1" }); + await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 18, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = value.length;" } }); + // Salvageability evidence: the author has two realized MERGED outcomes in this repo (+25) and the + // flagged defect class is mechanical (+45) => score 70 >= the manifest floor of 60. + for (const n of [101, 102]) { + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: n, title: "landed", state: "closed", user: { login: "contributor" }, head: { sha: `m${n}` }, labels: [], body: "" }); + await env.DB.prepare("INSERT INTO review_audit (id, project, target_id, event_type, decision, created_at) VALUES (?, 'owner/agent-repo', ?, 'pr_outcome', 'merged', ?)") + .bind(`ra-${n}`, `owner/agent-repo#${n}`, new Date().toISOString()) + .run(); + } + const inputFingerprint = await cachedSubFloorDefectFingerprint("Salvageable defect PR"); + await putCachedAiReview(env, "owner/agent-repo", 18, "d18", "block", { + notes: "cached review", + reviewerCount: 2, + // 0.95 sits ABOVE the default 0.93 floor — without the salvageability axis this one-shot-closes. + findings: [{ code: "ai_consensus_defect", severity: "critical", title: "Unused import", detail: "unused import join from node:path is dead code.", confidence: 0.95 }], + metadata: { inputFingerprint }, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/18/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = value.length;" }]); + if (url.endsWith("/pulls/18") && init?.method === "PATCH") return Response.json({ number: 18, state: "closed" }); + if (url.endsWith("/pulls/18")) return Response.json({ number: 18, title: "Salvageable defect PR", state: "open", user: { login: "contributor" }, head: { sha: "d18" }, labels: [], body: "Closes #1" }); + if (url.includes("/commits/d18/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/d18/status")) return Response.json({ state: "success", statuses: [] }); + if (url.endsWith("/pulls/18/reviews") && init?.method === "POST") return Response.json({ id: 1 }); + if (url.endsWith("/pulls/18/reviews")) return Response.json([]); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + await sweepAndDrainPerPr(env, "owner/agent-repo"); + // The gate still failed on the AI-judgment blocker (merge stays blocked)... + const blocker = await env.DB.prepare("select blocker_codes_json from gate_outcomes where repo_full_name = ? and pull_number = ? order by rowid desc limit 1").bind("owner/agent-repo", 18).first<{ blocker_codes_json: string }>(); + expect(blocker?.blocker_codes_json).toContain("ai_consensus_defect"); + // ...but the salvageability hold suppressed the at-floor one-shot close. + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and detail like ?").bind("agent.action.close", "%#18%").first<{ n: number }>(); + expect(closeAudit?.n).toBe(0); + const pr18 = await getPullRequest(env, "owner/agent-repo", 18); + expect(pr18?.state).toBe("open"); + // The v3 decision record carries the boundary evidence. + const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = ? and pull_number = ?").bind("owner/agent-repo", 18).first<{ record_json: string }>(); + const parsedRecord = JSON.parse(record!.record_json) as { schemaVersion: string; salvageability: { score: number; factors: string[] } | null }; + expect(parsedRecord.schemaVersion).toBe("3"); + expect(parsedRecord.salvageability?.score).toBe(70); + expect(parsedRecord.salvageability?.factors.join(" ")).toContain("mechanical defect class"); + }); + it("#4603: the SAME sub-floor defect one-shot-closes when aiReviewLowConfidenceDisposition is explicitly one_shot", async () => { let aiCalls = 0; const env = createTestEnv({ @@ -368,7 +432,7 @@ describe("queue processors", () => { // decision record — the row every future calibration read keys on. const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = 'owner/agent-repo' and pull_number = 9 order by created_at desc limit 1").first<{ record_json: string }>(); const parsedRecord = JSON.parse(record!.record_json) as { aiConfidence: number | null; promptDigest: string | null; schemaVersion: string }; - expect(parsedRecord.schemaVersion).toBe("2"); + expect(parsedRecord.schemaVersion).toBe("3"); // v3 (#8962): + salvageability expect(parsedRecord.aiConfidence).toBe(0.3); // the cached sub-floor defect's calibrated confidence expect(typeof parsedRecord.promptDigest).toBe("string"); }); diff --git a/test/unit/salvageability.test.ts b/test/unit/salvageability.test.ts new file mode 100644 index 0000000000..2f4c8c843f --- /dev/null +++ b/test/unit/salvageability.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, it } from "vitest"; +import { classifyDefectFixability, computeSalvageability } from "../../src/review/salvageability"; +import { computeSalvageabilityForTarget } from "../../src/review/salvageability-wire"; +import { resolveAiReviewSalvageableHold, type GateCheckEvaluation } from "../../src/rules/advisory"; +import { createTestEnv } from "../helpers/d1"; + +// #8962: the second axis of the close decision. Contracts: deterministic classification (structural wins), +// every score point named in factors, knob-unset = never changes a disposition, wire fails OPEN. + +describe("classifyDefectFixability", () => { + it("mechanical classes: encoding, dead imports, stale artifacts, coverage, version mismatches", () => { + for (const text of [ + "renders mojibake for em dash", + "unused import join from node:path", + "stale generated contract bundle for this route", + "codecov/patch failing at 86% with zero test lines", + "version bump to 1.21.0 is a downgrade vs main", + "blank-cell guard missing on stake_tao", + ]) { + expect(classifyDefectFixability(text)).toBe("mechanical"); + } + }); + + it("structural classes: fabrication, scope, duplication, bundling, policy — and structural WINS over mechanical", () => { + for (const text of [ + "the test manufactures a payload the code never emits", + "bundles an unrelated feature into a migration PR", + "duplicates the existing module", + "no linked eligible issue is provided", + "deletes working code with no replacement", + ]) { + expect(classifyDefectFixability(text)).toBe("structural"); + } + expect(classifyDefectFixability("unused import inside an unrelated refactor bundled into this PR")).toBe("structural"); + }); + + it("unknown when neither taxonomy matches", () => { + expect(classifyDefectFixability("computes top_pair_share from the returned page only")).toBe("unknown"); + }); +}); + +describe("computeSalvageability", () => { + it("every point is named: mechanical + veteran author + iterating = 100, each factor listed", () => { + const s = computeSalvageability({ findingText: "unused import x", authorPriorMergedCount: 5, priorReviewCycles: 3 }); + expect(s.score).toBe(100); + expect(s.fixability).toBe("mechanical"); + expect(s.factors).toHaveLength(3); + expect(s.factors.join(" ")).toContain("+45"); + expect(s.factors.join(" ")).toContain("+40"); + expect(s.factors.join(" ")).toContain("+15"); + }); + + it("tier arms: unknown class +15; single merged PR +25; no history and no iteration contribute 0 with named zero-factors", () => { + const s = computeSalvageability({ findingText: "misreads the pagination shape", authorPriorMergedCount: 1, priorReviewCycles: 1 }); + expect(s.score).toBe(40); // 15 + 25 + const zero = computeSalvageability({ findingText: "fabricates a test payload", authorPriorMergedCount: 0, priorReviewCycles: 0 }); + expect(zero.score).toBe(0); + expect(zero.fixability).toBe("structural"); + expect(zero.factors).toEqual(["structural defect class (+0)", "no merged history in this repo (+0)"]); + }); +}); + +const aiEval = (confidence: number, code = "ai_consensus_defect"): GateCheckEvaluation => + ({ + enabled: true, + conclusion: "failure", + title: "t", + summary: "s", + blockers: [{ code, title: "defect", severity: "critical", detail: "d", confidence }], + warnings: [], + }) as never; + +describe("resolveAiReviewSalvageableHold", () => { + const salv = { score: 70, factors: ["mechanical defect class (+45)", "author has 3 merged PRs here (+25)"] }; + + it("fires ONLY when: knob set, score clears it, AI-judgment-only failure, every blocker at/above the floor", () => { + const hold = resolveAiReviewSalvageableHold(aiEval(0.95), { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 60 }, salv); + expect(hold).toBeDefined(); + expect(hold!.reason).toContain("70 ≥ 60"); + expect(hold!.reason).toContain("mechanical defect class"); + expect(hold!.comment).toContain("HELD with guidance"); + }); + + it("defaults: no configured floor uses the gate default, and a confidence-less blocker counts as certainty (at/above floor)", () => { + const noConfidence = aiEval(0.99); + delete (noConfidence.blockers[0] as { confidence?: number }).confidence; + const hold = resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv); + expect(hold).toBeDefined(); + }); + + it("knob unset (the default) or no score: never changes a disposition", () => { + expect(resolveAiReviewSalvageableHold(aiEval(0.95), { aiReviewCloseConfidence: 0.9 }, salv)).toBeUndefined(); + expect(resolveAiReviewSalvageableHold(aiEval(0.95), { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 60 }, null)).toBeUndefined(); + }); + + it("below-floor blockers belong to the low-confidence hold — this resolver stands down", () => { + expect(resolveAiReviewSalvageableHold(aiEval(0.7), { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined(); + }); + + it("stands down on: sub-threshold score, non-hold disposition, and mixed (non-AI) failures", () => { + expect(resolveAiReviewSalvageableHold(aiEval(0.95), { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 80 }, salv)).toBeUndefined(); + expect( + resolveAiReviewSalvageableHold(aiEval(0.95), { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 60, aiReviewLowConfidenceDisposition: "one_shot" }, salv), + ).toBeUndefined(); + const mixed = aiEval(0.95); + (mixed.blockers as unknown[]).push({ code: "secret_leak", title: "s", severity: "critical", detail: "d" }); + expect(resolveAiReviewSalvageableHold(mixed, { aiReviewCloseConfidence: 0.9, aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined(); + }); +}); + +describe("computeSalvageabilityForTarget (wire)", () => { + it("counts the author's realized MERGED outcomes and this PR's review cycles; no AI blocker = null (no IO)", async () => { + const env = createTestEnv(); + await env.DB.prepare(`INSERT INTO pull_requests (repo_full_name, number, author_login, title, state) VALUES ('o/r', 1, 'alice', 't', 'closed'), ('o/r', 2, 'alice', 't', 'closed'), ('o/r', 3, 'alice', 't', 'open'), ('o/r', 9, 'alice', 't', 'closed')`).run(); + for (const [target, decision] of [["o/r#1", "merged"], ["o/r#2", "merged"], ["o/r#9", "closed"]] as const) { + await env.DB.prepare(`INSERT INTO review_audit (id, project, target_id, event_type, decision, created_at) VALUES (?, 'o/r', ?, 'pr_outcome', ?, ?)`) + .bind(`ra-${target}`, target, decision, new Date().toISOString()) + .run(); + } + for (const sha of ["s1", "s2"]) { + await env.DB.prepare(`INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) VALUES (?, 'o/r', 3, ?, 'hold', 'r', 'd', '{}', ?)`) + .bind(`record:o/r#3@${sha}`, sha, new Date().toISOString()) + .run(); + } + const gate = aiEval(0.95); + const score = await computeSalvageabilityForTarget(env, "o/r", 3, "ALICE", gate); + expect(score).not.toBeNull(); + // 2 merged (not the closed one, not the open PR) => +25 tier; unknown class 'defect d' => +15; 2 cycles => +15. + expect(score!.score).toBe(55); + expect(score!.factors.join(" ")).toContain("2 merged"); + expect(await computeSalvageabilityForTarget(env, "o/r", 3, "alice", { blockers: [] })).toBeNull(); + }); + + it("fails OPEN on a read error and when the author is unknown the merged tier is zero", async () => { + const env = createTestEnv(); + const { vi } = await import("vitest"); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const broken = createTestEnv(); + vi.spyOn(broken.DB, "prepare").mockImplementation(() => { + throw new Error("db down"); + }); + expect(await computeSalvageabilityForTarget(broken, "o/r", 1, "alice", aiEval(0.9))).toBeNull(); + expect(warn).toHaveBeenCalled(); + const anon = await computeSalvageabilityForTarget(env, "o/r", 1, null, aiEval(0.9)); + expect(anon).not.toBeNull(); + expect(anon!.factors.join(" ")).toContain("no merged history"); + vi.restoreAllMocks(); + }); +});