diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 4b414c851a..dfeafa2053 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -4735,22 +4735,6 @@ const IMPROVEMENT_BAND_LABELS: Record = { * (see the module comment above for why a full collapsible isn't wired up in this PR: at most one * deterministic finding can fire today, since REES's complexity/duplication analyzers and a parsed Codecov * number have no caller yet -- see `signals/improvement.ts`'s own header comment). */ -function improvementEvidenceText( - band: ImprovementBand, - safeFindings: SignalFinding[], - safeValueAssessment: { magnitude: ImprovementMagnitude; rationale: string } | undefined, -): string { - const findingSentences = safeFindings.map((finding) => finding.publicText ?? finding.detail); - const deterministicPart = - band === "insufficient-signal" - ? "Nothing measurable for the structural-improvement analyzers on this PR (e.g. no code files changed)." - : findingSentences.length > 0 - ? findingSentences.slice(0, 2).join(" ") + (findingSentences.length > 2 ? ` (+${findingSentences.length - 2} more.)` : "") - : "No structural-improvement signals were detected for this PR."; - const valuePart = safeValueAssessment ? ` LLM value judgment: ${safeValueAssessment.magnitude} — ${safeValueAssessment.rationale}` : ""; - return `${deterministicPart}${valuePart}`; -} - /** The risk × value quadrant label (#4745): crosses the existing `SlopBand` (risk axis, `src/signals/slop.ts`) * with the deterministic `ImprovementBand` (value axis, #4742) into one compact string, e.g. * `"risk: low · value: moderate"` -- exactly the issue's own example wording. Both band types are closed @@ -4789,22 +4773,23 @@ function buildImprovementSignalRow( slopBand?: SlopBand | undefined, ): PublicPrPanelSignalRow | null { if (!assessment) return null; - const safeFindings = assessment.findings.filter( - (finding) => !containsPrivatePublicTerm([finding.title, finding.detail, finding.publicText].filter(Boolean).join(" ")), - ); - const safeValueAssessment = valueAssessment && !containsPrivatePublicTerm(valueAssessment.rationale) ? valueAssessment : undefined; - const evidence = improvementEvidenceText(assessment.band, safeFindings, safeValueAssessment); - // #4745: prefixes the risk × value quadrant onto the SAME Evidence cell instead of a new row/column -- - // `assessment.band` is always defined here (the row already bailed out above when `assessment` is absent), - // so this only ever needs `slopBand` to produce the full quadrant; absent slopBand (slop wasn't computed - // this pass) leaves the evidence text exactly as it was before this PR, never a fabricated risk reading. + // #5101: the Evidence cell is a quick rating, not a paragraph. The risk × value quadrant (#4745) IS the + // score; it is tagged with only the LLM's one-word magnitude when present. The raw finding sentences and the + // full LLM rationale the old cell concatenated — a wall of text by construction — are intentionally dropped + // per maintainer feedback ("a quick, clean, simple score/rating ... not a paragraph"). Only closed-enum band + // names + the magnitude enum are interpolated, so the cell is public-safe with no free-text leak surface — + // the `containsPrivatePublicTerm` filtering the old free-text evidence needed is no longer required here. const quadrant = formatRiskValueQuadrant(slopBand, assessment.band); + const magnitudeTag = valueAssessment ? `LLM: ${valueAssessment.magnitude}` : undefined; + // Quadrant is absent only when slop wasn't computed this pass; fall back to the value band alone so the cell + // is never empty, still never fabricating a risk reading. + const rating = [quadrant ?? `value: ${assessment.band}`, magnitudeTag].filter(Boolean).join(" · "); return { key: "improvementSignal", cells: [ "Improvement", IMPROVEMENT_BAND_LABELS[assessment.band], - quadrant ? `${quadrant} — ${evidence}` : evidence, + rating, "Advisory only — never blocks merge.", ], }; diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 8021746dac..c691528c9b 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -3016,7 +3016,7 @@ describe("queue processors", () => { // unified renderer's table only surfaces the first 3 of each row's 4 cells (Label/Result/Evidence, not // Action) — same as the adjacent "Gate result" row, which also never shows its own 4th cell here — so this // asserts against the 3 columns this renderer actually prints, not the row's full cells array. - expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | No structural-improvement signals were detected for this PR. |"); + expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | value: none |"); // Public-safe regardless: no internal trust/economics fields leak through this new row either. expect(postedBody).not.toMatch(/wallet|hotkey|coldkey|reward|trust score/i); } finally { @@ -3191,11 +3191,10 @@ describe("queue processors", () => { expect(calls.comments).toBeGreaterThan(0); expect(postedBody).toContain(""); - // The quadrant prefix ("risk: low · value: none") threaded from the REAL slopBand computed this pass - // (missingTestEvidence only, slopRisk 15 -> band "low") ahead of the SAME evidence text #4744 already - // asserts verbatim -- proving processors.ts's new hoisted slopBand reaches the rendered comment, not - // just computed and discarded. - expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | risk: low · value: none — No structural-improvement signals were detected for this PR. |"); + // The quadrant rating ("risk: low · value: none") threaded from the REAL slopBand computed this pass + // (missingTestEvidence only, slopRisk 15 -> band "low") IS the concise Evidence cell (#5101) -- proving + // processors.ts's hoisted slopBand reaches the rendered comment, not just computed and discarded. + expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | risk: low · value: none |"); // Public-safe regardless: no internal trust/economics fields leak through the new quadrant clause either. expect(postedBody).not.toMatch(/wallet|hotkey|coldkey|reward|trust score/i); } finally { diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index e4a8d5152a..833cc49a20 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -930,11 +930,11 @@ describe("signal coverage edge cases", () => { expect(panel.rows).toHaveLength(7); }); - it("renders the deterministic band as a static template label when only the deterministic tier is available", () => { + it("renders a concise value rating (#5101) and never dumps the finding sentences into the cell", () => { const comment = buildPublicPrIntelligenceComment({ ...improvementBaseArgs, improvementSignal: minorAssessment, env: {} }); - expect(comment).toContain("| Improvement | ✅ Minor |"); - expect(comment).toContain("Code changes are accompanied by test evidence."); - expect(comment).not.toContain("Value judgment"); + expect(comment).toContain("| Improvement | ✅ Minor | value: minor |"); + // #5101: the raw finding sentence is intentionally no longer rendered — the cell is a quick rating. + expect(comment).not.toContain("Code changes are accompanied by test evidence."); expect(comment).not.toContain("LLM value judgment"); const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: minorAssessment }); @@ -943,12 +943,12 @@ describe("signal coverage edge cases", () => { expect(row.cells).toEqual([ "Improvement", "✅ Minor", - "Code changes are accompanied by test evidence.", + "value: minor", "Advisory only — never blocks merge.", ]); }); - it("renders the LLM tier's magnitude + rationale alongside the deterministic band when both tiers are available", () => { + it("tags the concise rating with the LLM's one-word magnitude (not the full rationale) when both tiers are available (#5101)", () => { const noneAssessment = { improvementScore: 0, band: "none" as const, findings: [] }; const valueAssessment = { magnitude: "significant" as const, rationale: "This removes a whole class of retry bugs." }; const comment = buildPublicPrIntelligenceComment({ @@ -959,21 +959,26 @@ describe("signal coverage edge cases", () => { }); // The Result cell reflects the DETERMINISTIC band ("none"), never the LLM magnitude -- the two tiers are // deliberately never blended into one number/label (epic #4737 design constraint 1). - expect(comment).toContain("| Improvement | ℹ️ None detected |"); - expect(comment).toContain("LLM value judgment: significant — This removes a whole class of retry bugs."); + expect(comment).toContain("| Improvement | ℹ️ None detected | value: none · LLM: significant |"); + // #5101: only the one-word magnitude, never the full rationale paragraph, is surfaced. + expect(comment).toContain("· LLM: significant"); + expect(comment).not.toContain("This removes a whole class of retry bugs."); + expect(comment).not.toContain("LLM value judgment"); const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: noneAssessment, valueAssessment }); const row = panel.rows.find((r) => r.key === "improvementSignal")!; expect(row.cells[1]).toBe("ℹ️ None detected"); - expect(row.cells[2]).toBe("No structural-improvement signals were detected for this PR. LLM value judgment: significant — This removes a whole class of retry bugs."); + expect(row.cells[2]).toBe("value: none · LLM: significant"); }); - it("renders the insufficient-signal band and caps inline findings at 2 with a '+N more' summary beyond that", () => { + it("renders insufficient-signal as a concise value rating and drops all finding sentences, however many (#5101)", () => { const insufficientAssessment = { improvementScore: 0, band: "insufficient-signal" as const, findings: [] }; const insufficientPanel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: insufficientAssessment }); const insufficientRow = insufficientPanel.rows.find((r) => r.key === "improvementSignal")!; expect(insufficientRow.cells[1]).toBe("ℹ️ Insufficient signal"); - expect(insufficientRow.cells[2]).toContain("Nothing measurable"); + expect(insufficientRow.cells[2]).toBe("value: insufficient-signal"); + // #5101: the old "Nothing measurable ..." paragraph is gone. + expect(insufficientRow.cells[2]).not.toContain("Nothing measurable"); const manyFindingsAssessment = { improvementScore: 100, @@ -987,12 +992,10 @@ describe("signal coverage edge cases", () => { }; const manyFindingsPanel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: manyFindingsAssessment }); const manyFindingsRow = manyFindingsPanel.rows.find((r) => r.key === "improvementSignal")!; - // Only the first two finding sentences render inline (none of the four fixtures above set `publicText`, so - // this also exercises the `finding.publicText ?? finding.detail` fallback); the remaining two are - // summarized by count rather than dumped inline (mirrors the "Nits"-style non-inline-dump convention). - expect(manyFindingsRow.cells[2]).toBe( - "2 function(s) have lower cyclomatic complexity after this pull request. 1 previously-duplicated code block(s) were consolidated or removed by this pull request. (+2 more.)", - ); + // #5101: no matter how many findings, the cell is the concise value rating — finding sentences are never + // concatenated inline (the whole point of this issue's simplification). + expect(manyFindingsRow.cells[2]).toBe("value: significant"); + expect(manyFindingsRow.cells[2]).not.toContain("cyclomatic complexity"); }); it("hides the row via review.fields.improvementSignal: false, exactly like its seven siblings", () => { @@ -1037,9 +1040,10 @@ describe("signal coverage edge cases", () => { const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: unsafeAssessment, valueAssessment: unsafeValueAssessment }); const row = panel.rows.find((r) => r.key === "improvementSignal")!; expect(JSON.stringify(row)).not.toMatch(forbidden); - // With the one (unsafe) finding filtered out and the (unsafe) valueAssessment dropped, the deterministic - // "no signals" fallback text is what's left -- never an empty cell. - expect(row.cells[2]).toBe("No structural-improvement signals were detected for this PR."); + // #5101 makes this structurally leak-proof: the cell renders only closed-enum band/magnitude names, never + // the free-text finding detail or LLM rationale, so no sanitizer pass is even needed. The unsafe finding + // sentence and unsafe rationale simply never reach the cell. + expect(row.cells[2]).toBe("value: moderate · LLM: significant"); }); }); @@ -1087,21 +1091,22 @@ describe("signal coverage edge cases", () => { }; const quadrantAssessment = { improvementScore: 10, band: "minor" as const, findings: [] }; - it("prefixes the quadrant label onto the Improvement row's Evidence cell when slopBand is threaded alongside improvementSignal", () => { + it("uses the quadrant label as the Improvement row's Evidence rating when slopBand is threaded (#5101)", () => { const panel = buildPublicPrPanelSignalRows({ ...quadrantBaseArgs, improvementSignal: quadrantAssessment, slopBand: "low" }); const row = panel.rows.find((r) => r.key === "improvementSignal")!; - expect(row.cells[2]).toBe("risk: low · value: minor — No structural-improvement signals were detected for this PR."); + // #5101: the quadrant IS the rating now — no trailing finding/rationale paragraph. + expect(row.cells[2]).toBe("risk: low · value: minor"); // The Result cell (the deterministic band label) is untouched by the quadrant -- the two tiers never blend. expect(row.cells[1]).toBe("✅ Minor"); const comment = buildPublicPrIntelligenceComment({ ...quadrantBaseArgs, improvementSignal: quadrantAssessment, slopBand: "low", env: {} }); - expect(comment).toContain("risk: low · value: minor — No structural-improvement signals were detected for this PR."); + expect(comment).toContain("| Improvement | ✅ Minor | risk: low · value: minor |"); }); - it("leaves the Evidence cell exactly as #4744 shipped it when slopBand is omitted -- byte-identical for every caller that hasn't threaded it yet", () => { + it("falls back to the value band alone when slopBand is omitted -- never a fabricated risk reading (#5101)", () => { const panel = buildPublicPrPanelSignalRows({ ...quadrantBaseArgs, improvementSignal: quadrantAssessment }); const row = panel.rows.find((r) => r.key === "improvementSignal")!; - expect(row.cells[2]).toBe("No structural-improvement signals were detected for this PR."); + expect(row.cells[2]).toBe("value: minor"); expect(row.cells[2]).not.toContain("risk:"); });