From c3ff97de0d1446e7242f28f590f062b080256423 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:06:50 +0000 Subject: [PATCH] fix(orb): report null mergeRate for a zero-sample slop band buildSlopOutcomeCalibration fabricated a 0% merge rate for a slop band with no resolved PRs, which for a discrimination table reads as "every PR in this band was closed" -- the strongest possible claim the table can make. Mirror overallMergeRate's existing null-for-empty treatment: SlopBandCalibration.mergeRate is now number | null, computeDiscriminates narrows sampled bands with a type guard instead of comparing against a nullable field, and the MCP CLI's shared n/a-below-sample renderer now actually exercises its null arm for a per-band rate. Widened the OpenAPI response shape and the UI card's type to match. --- apps/loopover-ui/public/openapi.json | 58 ++++++++++++++++++- .../app-panels/slop-band-calibration-card.tsx | 6 +- packages/loopover-mcp/bin/loopover-mcp.ts | 3 + src/openapi/schemas.ts | 19 +++++- src/services/outcome-calibration.ts | 14 ++++- test/unit/mcp-cli-maintain.test.ts | 13 +++++ test/unit/outcome-calibration.test.ts | 12 ++++ test/unit/support/mcp-cli-harness.ts | 5 +- 8 files changed, 122 insertions(+), 8 deletions(-) diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 84541855da..c7efb44afb 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -15272,7 +15272,63 @@ "nullable": true }, "slop": { - "nullable": true + "type": "object", + "properties": { + "totalResolved": { + "type": "number" + }, + "bands": { + "type": "array", + "items": { + "type": "object", + "properties": { + "band": { + "type": "string", + "enum": [ + "clean", + "low", + "elevated", + "high" + ] + }, + "sampleSize": { + "type": "number" + }, + "merged": { + "type": "number" + }, + "closed": { + "type": "number" + }, + "mergeRate": { + "type": "number", + "nullable": true + } + }, + "required": [ + "band", + "sampleSize", + "merged", + "closed", + "mergeRate" + ] + } + }, + "overallMergeRate": { + "type": "number", + "nullable": true + }, + "discriminates": { + "type": "boolean", + "nullable": true + } + }, + "required": [ + "totalResolved", + "bands", + "overallMergeRate", + "discriminates" + ] }, "recommendations": { "nullable": true diff --git a/apps/loopover-ui/src/components/site/app-panels/slop-band-calibration-card.tsx b/apps/loopover-ui/src/components/site/app-panels/slop-band-calibration-card.tsx index bb7bfbed32..b0e5334c2e 100644 --- a/apps/loopover-ui/src/components/site/app-panels/slop-band-calibration-card.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/slop-band-calibration-card.tsx @@ -12,7 +12,7 @@ export type SlopBandCalibration = { sampleSize: number; merged: number; closed: number; - mergeRate: number; + mergeRate: number | null; }; export type SlopOutcomeCalibration = { @@ -38,7 +38,9 @@ function discriminationPill(discriminates: boolean | null): { status: Status; la function BandRow({ row }: { row: SlopBandCalibration }) { const hasSamples = row.sampleSize > 0; - const pct = hasSamples ? Math.round(row.mergeRate * 100) : null; + // hasSamples (sampleSize > 0) already guarantees mergeRate is non-null server-side; the fallback here is + // unreachable and exists only to satisfy the wider (nullable) type. + const pct = hasSamples ? Math.round((row.mergeRate ?? 0) * 100) : null; return (
diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index f7f4d0b018..49248dedad 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -2897,6 +2897,9 @@ export async function maintainCli(args: readonly string[]) { const payload = await apiGet(`${repoBase}/outcome-calibration${query}`); const window = payload.windowDays ? `last ${payload.windowDays}d` : "all history"; const recommendations = payload.recommendations ?? {}; + // #9641: a zero-sample slop band reports mergeRate null server-side (never a fabricated 0, which for this + // discrimination table would read as "every PR in this band was closed") -- this shared null/undefined + // guard already renders that as "n/a (below sample)" rather than coercing it into "0%". const rate = (value: any) => (value === null || value === undefined ? "n/a (below sample)" : `${Math.round(value * 100)}%`); const lines = [ `Outcome calibration for ${repoFullName} (${window}): recommendations ${recommendations.positive ?? 0} positive, ${recommendations.negative ?? 0} negative, ${recommendations.pending ?? 0} pending (positive rate ${rate(recommendations.positiveRate)}).`, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 3b1c71cfbd..4023d63b4a 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -3290,7 +3290,24 @@ export const OutcomeCalibrationResponseSchema = z repoFullName: z.string().optional(), generatedAt: z.string().optional(), windowDays: z.number().nullable().optional(), - slop: z.unknown().optional(), + // #9641: a band with sampleSize 0 (no resolved PRs) reports mergeRate null -- never a fabricated 0, which + // for a discrimination table would read as "every PR in this band was closed". + slop: z + .object({ + totalResolved: z.number(), + bands: z.array( + z.object({ + band: z.enum(["clean", "low", "elevated", "high"]), + sampleSize: z.number(), + merged: z.number(), + closed: z.number(), + mergeRate: z.number().nullable(), + }), + ), + overallMergeRate: z.number().nullable(), + discriminates: z.boolean().nullable(), + }) + .optional(), recommendations: z.unknown().optional(), signals: z.array(z.string()).optional(), status: z.string().optional(), diff --git a/src/services/outcome-calibration.ts b/src/services/outcome-calibration.ts index 4f27139b28..e53793fbb5 100644 --- a/src/services/outcome-calibration.ts +++ b/src/services/outcome-calibration.ts @@ -26,7 +26,7 @@ const SLOP_BAND_ORDER: readonly SlopBand[] = ["clean", "low", "elevated", "high" // Below this per-band sample the merge rate is too noisy to judge discrimination. const MIN_BAND_SAMPLE = 5; -export type SlopBandCalibration = { band: SlopBand; sampleSize: number; merged: number; closed: number; mergeRate: number }; +export type SlopBandCalibration = { band: SlopBand; sampleSize: number; merged: number; closed: number; mergeRate: number | null }; export type SlopOutcomeCalibration = { totalResolved: number; @@ -107,7 +107,9 @@ export function buildSlopOutcomeCalibration(pullRequests: PullRequestRecord[], o const bands: SlopBandCalibration[] = SLOP_BAND_ORDER.map((band) => { const { merged, closed } = counts.get(band) ?? { merged: 0, closed: 0 }; const sampleSize = merged + closed; - return { band, sampleSize, merged, closed, mergeRate: sampleSize > 0 ? round(merged / sampleSize) : 0 }; + // Mirrors overallMergeRate below: a band nobody has data for reports null ("unknown"), never a fabricated + // 0 ("every PR in this band was closed"), the strongest possible discrimination claim (#9641). + return { band, sampleSize, merged, closed, mergeRate: sampleSize > 0 ? round(merged / sampleSize) : null }; }); return { totalResolved, @@ -117,8 +119,14 @@ export function buildSlopOutcomeCalibration(pullRequests: PullRequestRecord[], o }; } +// A sampled band's sampleSize (>= MIN_BAND_SAMPLE, so > 0) always yields a non-null mergeRate above; this +// predicate narrows the type so the comparison below doesn't need a runtime null check on an unreachable case. +function isSampledBand(band: SlopBandCalibration): band is SlopBandCalibration & { mergeRate: number } { + return band.sampleSize >= MIN_BAND_SAMPLE; +} + function computeDiscriminates(bands: SlopBandCalibration[]): boolean | null { - const sampled = bands.filter((band) => band.sampleSize >= MIN_BAND_SAMPLE); // already in severity order + const sampled = bands.filter(isSampledBand); // already in severity order if (sampled.length < 2) return null; // not enough signal to judge for (let index = 1; index < sampled.length; index += 1) { // A later (higher-severity) band merging MORE than an earlier one means the score is not discriminating. diff --git a/test/unit/mcp-cli-maintain.test.ts b/test/unit/mcp-cli-maintain.test.ts index 08abfb32eb..6ddeab6b59 100644 --- a/test/unit/mcp-cli-maintain.test.ts +++ b/test/unit/mcp-cli-maintain.test.ts @@ -89,6 +89,7 @@ beforeEach(() => { planIssuesBodies.length = 0; apiRequests.length = 0; fixtureOptions.repoDocRefresh = undefined; + fixtureOptions.outcomeCalibrationBands = undefined; }); async function captureStdout(fn: () => Promise): Promise { @@ -396,6 +397,18 @@ describe("loopover-mcp CLI — maintain (#784)", () => { expect(scoped).toMatch(/Outcome calibration for owner\/repo \(last 30d\)/); }); + // REGRESSION (#9641): a zero-sample band's mergeRate is null (not a fabricated 0), and the plain-text + // renderer must show that as "n/a", matching the sampled bands' own "n/a (below sample)" wording. + it("outcome-calibration renders a zero-sample band's null mergeRate as n/a, not 0%", async () => { + fixtureOptions.outcomeCalibrationBands = [ + { band: "clean", sampleSize: 12, merged: 9, closed: 3, mergeRate: 0.75 }, + { band: "high", sampleSize: 0, merged: 0, closed: 0, mergeRate: null }, + ]; + const out = await cli(["maintain", "outcome-calibration", "--repo", "owner/repo"]); + expect(out).toMatch(/high: n\/a \(below sample\) merge rate over 0 PR\(s\)/); + expect(out).not.toMatch(/high: 0% merge rate/); + }); + it("onboarding-pack mirrors the session-gated API payload and forwards refresh", async () => { const json = JSON.parse( await cli([ diff --git a/test/unit/outcome-calibration.test.ts b/test/unit/outcome-calibration.test.ts index aa19e24fb1..c8c3155d99 100644 --- a/test/unit/outcome-calibration.test.ts +++ b/test/unit/outcome-calibration.test.ts @@ -55,6 +55,18 @@ describe("buildSlopOutcomeCalibration", () => { expect(result.totalResolved).toBe(4); }); + // REGRESSION (#9641): a band with zero resolved PRs reports mergeRate null -- never a fabricated 0, which + // for a discrimination table would read as "every PR in this band was closed" -- and computeDiscriminates' + // verdict over the remaining sampled bands is unaffected by the null band. + it("reports a zero-sample band's mergeRate as null, leaving the verdict over the sampled bands unchanged", () => { + const result = buildSlopOutcomeCalibration([...band("clean", 6, 5, 0), ...band("low", 6, 4, 100), ...band("elevated", 6, 1, 200)]); + // "high" has no resolved PRs at all -- one zero-sample band alongside three sampled ones. + const high = result.bands.find((b) => b.band === "high")!; + expect(high.sampleSize).toBe(0); + expect(high.mergeRate).toBeNull(); + expect(result.discriminates).toBe(true); // clean 0.833 > low 0.667 > elevated 0.167 -- non-increasing + }); + it("excludes open PRs and PRs with no slop assessment", () => { const open: PullRequestRecord = { repoFullName: "owner/repo", number: 9, title: "open", state: "open", labels: [], linkedIssues: [], slopRisk: 70, slopBand: "high" }; const unassessed: PullRequestRecord = { repoFullName: "owner/repo", number: 10, title: "no slop", state: "closed", mergedAt: "2026-06-01T00:00:00.000Z", labels: [], linkedIssues: [] }; diff --git a/test/unit/support/mcp-cli-harness.ts b/test/unit/support/mcp-cli-harness.ts index 2e72b803b5..a858eb35ad 100644 --- a/test/unit/support/mcp-cli-harness.ts +++ b/test/unit/support/mcp-cli-harness.ts @@ -205,6 +205,9 @@ export async function startFixtureServer( onApiRequest?: (request: IncomingMessage) => void; /** #9300: captures DELETE /v1/repos/:owner/:repo/selftune/overrides body ({ confirm }). */ onClearSelftuneOverride?: (body: { confirm?: boolean }) => void; + /** #9641: overrides the outcome-calibration route's `slop` band list -- lets a test drive a zero-sample + * band (mergeRate: null) through the CLI's plain-text renderer without touching the other two bands. */ + outcomeCalibrationBands?: unknown[] | undefined; validateConfigWarnings?: string[]; openPrMonitor?: Record; prOutcomes?: Record; @@ -842,7 +845,7 @@ export async function startFixtureServer( repoFullName: "owner/repo", generatedAt: "2026-05-30T00:00:00.000Z", windowDays: windowDays ? Number(windowDays) : null, - slop: [ + slop: options.outcomeCalibrationBands ?? [ { band: "clean", sampleSize: 12, merged: 9, closed: 3, mergeRate: 0.75 }, { band: "high", sampleSize: 4, merged: 1, closed: 3, mergeRate: 0.25 }, ],