diff --git a/src/signals/slop.ts b/src/signals/slop.ts index 8e79267625..2678afc9db 100644 --- a/src/signals/slop.ts +++ b/src/signals/slop.ts @@ -25,6 +25,7 @@ export type SlopAssessment = { export const SLOP_WEIGHTS = { missingTestEvidence: 30, + trivialWhitespaceChurn: 25, } as const; export const SLOP_RUBRIC_MARKDOWN = [ @@ -37,14 +38,25 @@ export const SLOP_RUBRIC_MARKDOWN = [ "", "Current deterministic signals:", "- missing test evidence", + "- trivial / whitespace-only churn", ].join("\n"); +const MIN_CHURN_LINES = 40; +const MAX_SOURCE_LINE_SHARE = 0.15; + export function buildSlopAssessment(input: SlopAssessmentInput): SlopAssessment { const findings: SignalFinding[] = []; const missingTestEvidenceFinding = buildMissingTestEvidenceFinding(input); + const trivialChurnFinding = buildTrivialWhitespaceChurnFinding(input); if (missingTestEvidenceFinding) findings.push(missingTestEvidenceFinding); + if (trivialChurnFinding) findings.push(trivialChurnFinding); - const slopRisk = clamp(missingTestEvidenceFinding ? SLOP_WEIGHTS.missingTestEvidence : 0, 0, 100); + const slopRisk = clamp( + (missingTestEvidenceFinding ? SLOP_WEIGHTS.missingTestEvidence : 0) + + (trivialChurnFinding ? SLOP_WEIGHTS.trivialWhitespaceChurn : 0), + 0, + 100, + ); return { slopRisk, @@ -83,6 +95,62 @@ export function buildMissingTestEvidenceFinding(input: SlopAssessmentInput): Sig }; } +export function buildTrivialWhitespaceChurnFinding(input: SlopAssessmentInput): SignalFinding | null { + const changedFiles = input.changedFiles ?? []; + const lineTotals = summarizeChangedLines(changedFiles); + if (lineTotals.changedLineCount < MIN_CHURN_LINES) return null; + if (lineTotals.sourceLineCount === 0) { + return buildTrivialChurnFinding(lineTotals.changedLineCount, lineTotals.nonCodeLineCount); + } + const sourceShare = lineTotals.sourceLineCount / lineTotals.changedLineCount; + if (sourceShare > MAX_SOURCE_LINE_SHARE) return null; + return buildTrivialChurnFinding(lineTotals.changedLineCount, lineTotals.nonCodeLineCount); +} + +function summarizeChangedLines(changedFiles: SlopChangedFile[]): { + changedLineCount: number; + sourceLineCount: number; + testLineCount: number; + nonCodeLineCount: number; +} { + const changedLineCount = changedFiles.reduce( + (sum, file) => sum + nonNegative(file.additions) + nonNegative(file.deletions), + 0, + ); + const sourceLineCount = changedFiles + .filter((file) => isCodeFile(file.path)) + .reduce((sum, file) => sum + nonNegative(file.additions) + nonNegative(file.deletions), 0); + const testLineCount = changedFiles + .filter((file) => isTestFile(file.path)) + .reduce((sum, file) => sum + nonNegative(file.additions) + nonNegative(file.deletions), 0); + const nonCodeLineCount = Math.max(0, changedLineCount - sourceLineCount - testLineCount); + return { changedLineCount, sourceLineCount, testLineCount, nonCodeLineCount }; +} + +function buildTrivialChurnFinding(changedLineCount: number, nonCodeLineCount: number): SignalFinding { + const detail = ensurePublicSafeText( + `The diff churns ${changedLineCount} line(s) with only ${Math.max(0, changedLineCount - nonCodeLineCount)} substantive source line(s) touched.`, + "The diff shows high churn with minimal substantive source changes.", + ); + const action = ensurePublicSafeText( + "Reduce whitespace-only or formatting-only churn and keep the diff focused on substantive changes.", + "Reduce formatting-only churn and keep the diff focused on substantive changes.", + ); + + return { + code: "trivial_whitespace_churn", + title: "Diff looks like trivial or whitespace-only churn", + severity: "warning", + detail, + action, + publicText: detail, + }; +} + +function nonNegative(value: number | undefined): number { + return Number.isFinite(value) && (value ?? 0) > 0 ? Math.trunc(value as number) : 0; +} + function ensurePublicSafeText(text: string, fallback: string): string { return isFocusManifestPublicSafe(text) ? text : fallback; } diff --git a/test/unit/slop.test.ts b/test/unit/slop.test.ts index 294394577a..8b592150e5 100644 --- a/test/unit/slop.test.ts +++ b/test/unit/slop.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { buildMissingTestEvidenceFinding, buildSlopAssessment, + buildTrivialWhitespaceChurnFinding, SLOP_RUBRIC_MARKDOWN, SLOP_WEIGHTS, } from "../../src/signals/slop"; @@ -13,6 +14,7 @@ describe("buildSlopAssessment", () => { it("exports rubric bands and a deterministic assessment shell", () => { expect(SLOP_RUBRIC_MARKDOWN).toContain("clean"); expect(SLOP_RUBRIC_MARKDOWN).toContain("missing test evidence"); + expect(SLOP_RUBRIC_MARKDOWN).toContain("trivial / whitespace-only churn"); const clean = buildSlopAssessment({}); expect(clean).toEqual({ slopRisk: 0, band: "clean", findings: [] }); @@ -35,6 +37,27 @@ describe("buildSlopAssessment", () => { expect(JSON.stringify(result)).not.toMatch(FORBIDDEN_PUBLIC_TERMS); }); + it("raises trivial-churn slop for high-churn diffs with minimal source lines", () => { + const result = buildSlopAssessment({ + changedFiles: [ + { path: "README.md", additions: 30, deletions: 20 }, + { path: "docs/guide.md", additions: 25, deletions: 15 }, + { path: "src/widget.ts", additions: 2, deletions: 1 }, + { path: "test/unit/widget.test.ts", additions: 4, deletions: 0 }, + ], + }); + + expect(result.slopRisk).toBe(SLOP_WEIGHTS.trivialWhitespaceChurn); + expect(result.band).toBe("elevated"); + expect(result.findings).toEqual([ + expect.objectContaining({ + code: "trivial_whitespace_churn", + severity: "warning", + }), + ]); + expect(JSON.stringify(result)).not.toMatch(FORBIDDEN_PUBLIC_TERMS); + }); + it("does not raise missing-test-evidence when changed test files are present", () => { expect( buildSlopAssessment({ @@ -55,13 +78,43 @@ describe("buildSlopAssessment", () => { ).toEqual({ slopRisk: 0, band: "clean", findings: [] }); }); - it("ignores docs-only diffs without code files", () => { + it("does not raise trivial-churn when substantive source edits dominate", () => { + expect( + buildSlopAssessment({ + changedFiles: [ + { path: "src/registry/sync.ts", additions: 80, deletions: 20 }, + { path: "test/unit/registry-sync.test.ts", additions: 40, deletions: 5 }, + ], + }), + ).toEqual({ slopRisk: 0, band: "clean", findings: [] }); + }); + + it("does not raise trivial-churn for small diffs below the churn threshold", () => { + expect( + buildSlopAssessment({ + changedFiles: [{ path: "README.md", additions: 10, deletions: 8 }], + }), + ).toEqual({ slopRisk: 0, band: "clean", findings: [] }); + }); + + it("ignores docs-only diffs without code files for missing-test-evidence", () => { expect( buildSlopAssessment({ - changedFiles: [{ path: "README.md", additions: 40, deletions: 0 }], + changedFiles: [{ path: "README.md", additions: 10, deletions: 0 }], }), ).toEqual({ slopRisk: 0, band: "clean", findings: [] }); }); + + it("raises trivial-churn for non-code-only high-churn diffs", () => { + expect( + buildSlopAssessment({ + changedFiles: [ + { path: "README.md", additions: 25, deletions: 20 }, + { path: "docs/guide.md", additions: 20, deletions: 15 }, + ], + }).findings.map((finding) => finding.code), + ).toEqual(["trivial_whitespace_churn"]); + }); }); describe("buildMissingTestEvidenceFinding", () => { @@ -77,3 +130,20 @@ describe("buildMissingTestEvidenceFinding", () => { expect(JSON.stringify(finding)).not.toMatch(FORBIDDEN_PUBLIC_TERMS); }); }); + +describe("buildTrivialWhitespaceChurnFinding", () => { + it("keeps public reason strings sanitized", () => { + const finding = buildTrivialWhitespaceChurnFinding({ + changedFiles: [ + { path: "README.md", additions: 30, deletions: 20 }, + { path: "docs/guide.md", additions: 25, deletions: 15 }, + ], + }); + + expect(finding).toMatchObject({ + code: "trivial_whitespace_churn", + publicText: expect.any(String), + }); + expect(JSON.stringify(finding)).not.toMatch(FORBIDDEN_PUBLIC_TERMS); + }); +});