From d02ec436da678d0efc519bd78d0e85dd30bf35eb Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Thu, 30 Jul 2026 05:21:39 +0800 Subject: [PATCH] fix(signals): unify the test-evidence code-path predicate as isTestableCodePath buildMissingTestEvidenceFinding exempts generated/vendored/minified output (source extensions, but nobody hand-writes tests for regenerated code) via !PADDING_CATEGORIES.has(classifyChangedFile(path)). Two consumers claimed to mirror it and used bare isCodeFile instead: - improvement.ts's hasCodeFileToEvaluate returned true for a codegen-only diff, so buildMissingTestEvidenceFinding's null (no testable code) was read as "test evidence present" and a zero-test PR of only api/service.pb.go scored added_test_evidence and banded `minor` instead of `insufficient-signal`. - contributor-open-pr-monitor.ts's missingTestsFromFiles flagged a vendored-only diff missing_tests, telling the contributor to test vendored code the gate-side exempts. Extract the rule once as isTestableCodePath(path) in the engine slop module, re-export it through the src shim, and adopt it in both consumers (removing the two hand-copied isCodeFile filters). No change to PADDING_CATEGORIES or any replacement/threshold. Closes #9696 --- packages/loopover-engine/src/index.ts | 1 + packages/loopover-engine/src/signals/slop.ts | 12 +++++++- packages/loopover-engine/test/slop.test.ts | 28 +++++++++++++++++++ src/signals/contributor-open-pr-monitor.ts | 12 ++++---- src/signals/improvement.ts | 8 ++++-- src/signals/slop.ts | 1 + test/unit/contributor-open-pr-monitor.test.ts | 19 ++++++++++++- test/unit/improvement.test.ts | 11 ++++++++ 8 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 packages/loopover-engine/test/slop.test.ts diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 1824c8466d..8172f57715 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -465,6 +465,7 @@ export { buildSlopAssessment, buildTrivialWhitespaceChurnFinding, hasClearNoIssueRationale, + isTestableCodePath, type SlopAssessment, type SlopAssessmentInput, type SlopBand, diff --git a/packages/loopover-engine/src/signals/slop.ts b/packages/loopover-engine/src/signals/slop.ts index f740d910d0..d5d7141c0f 100644 --- a/packages/loopover-engine/src/signals/slop.ts +++ b/packages/loopover-engine/src/signals/slop.ts @@ -110,6 +110,16 @@ const PADDING_DOMINANCE_SHARE = 0.5; // (where it exempts codegen-only diffs from the missing-test-evidence signal). const PADDING_CATEGORIES = new Set(["minified", "generated", "vendored"]); +/** The canonical "this path is code that ought to carry tests" predicate: a source file that is NOT + * mechanically-produced padding (generated/vendored/minified output carries source extensions — e.g. + * protoc's `.pb.go` stubs — but nobody hand-writes tests for it). Exported as the single source of truth so + * every test-evidence consumer (buildMissingTestEvidenceFinding here, plus the improvement and + * contributor-open-pr-monitor signals in src/) applies the SAME rule instead of a hand-copied `isCodeFile` + * that disagrees at exactly the codegen boundary (#9696). */ +export function isTestableCodePath(path: string): boolean { + return isCodeFile(path) && !PADDING_CATEGORIES.has(classifyChangedFile(path)); +} + export function buildSlopAssessment(input: SlopAssessmentInput): SlopAssessment { const findings: AdvisoryFinding[] = []; const trivialChurnFinding = buildTrivialWhitespaceChurnFinding(input); @@ -282,7 +292,7 @@ export function buildMissingTestEvidenceFinding(input: SlopAssessmentInput): Adv // so passes the plain isCodeFile check, but nobody hand-writes tests for mechanically regenerated code. // Mirror buildNonSubstantivePaddingFinding's classifyChangedFile-based exemption so this signal can't // fire on a codegen-only diff. - const codePaths = changedPaths.filter((path) => isCodeFile(path) && !PADDING_CATEGORIES.has(classifyChangedFile(path))); + const codePaths = changedPaths.filter(isTestableCodePath); if (codePaths.length === 0) return null; // A changed test FILE only counts as real test evidence when it carries substantive content. An empty or diff --git a/packages/loopover-engine/test/slop.test.ts b/packages/loopover-engine/test/slop.test.ts new file mode 100644 index 0000000000..126d6a4317 --- /dev/null +++ b/packages/loopover-engine/test/slop.test.ts @@ -0,0 +1,28 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { buildMissingTestEvidenceFinding, isTestableCodePath } from "../dist/index.js"; + +// #9696: isTestableCodePath is the single source-of-truth "code that ought to carry tests" predicate — a +// source file that is NOT mechanically-produced padding — and buildMissingTestEvidenceFinding filters on it, +// so a codegen-only diff never trips the missing-test-evidence signal. + +test("isTestableCodePath is true for hand-authored source and false for padding or non-code", () => { + assert.equal(isTestableCodePath("src/app/service.ts"), true); + // Generated/vendored/minified output carries source extensions but is not hand-authored, so it is exempt. + assert.equal(isTestableCodePath("api/service.pb.go"), false); // generated + assert.equal(isTestableCodePath("vendor/dep/util.go"), false); // vendored + assert.equal(isTestableCodePath("dist/app.min.js"), false); // minified + assert.equal(isTestableCodePath("README.md"), false); // not a code file at all +}); + +test("buildMissingTestEvidenceFinding exempts a codegen-only diff but still fires on real code without tests", () => { + // A diff of only generated output has no testable code → the missing-test-evidence signal must not fire. + assert.equal( + buildMissingTestEvidenceFinding({ changedFiles: [{ path: "api/service.pb.go", additions: 500, deletions: 0 }], tests: [], testFiles: [] }), + null, + ); + // Real hand-authored code with zero tests → the finding fires. + const finding = buildMissingTestEvidenceFinding({ changedFiles: [{ path: "src/app/service.ts", additions: 120, deletions: 0 }], tests: [], testFiles: [] }); + assert.ok(finding && finding.code === "missing_test_evidence", "expected a missing-test-evidence finding for uncovered hand-authored code"); +}); diff --git a/src/signals/contributor-open-pr-monitor.ts b/src/signals/contributor-open-pr-monitor.ts index daae8f676f..4fd8cb3d37 100644 --- a/src/signals/contributor-open-pr-monitor.ts +++ b/src/signals/contributor-open-pr-monitor.ts @@ -12,7 +12,7 @@ import type { CheckSummaryRecord, PullRequestFileRecord, PullRequestRecord, Pull import { nowIso } from "../utils/json"; import { buildRoleContext } from "./engine"; import { isFailingCheckSummary } from "./check-summary"; -import { isCodeFile } from "./local-branch"; +import { isTestableCodePath } from "./slop"; import { isTestPath } from "./test-evidence"; export type OpenPrWorkClassification = @@ -263,10 +263,12 @@ function normalizeTitle(title: string): string { function missingTestsFromFiles(files: PullRequestFileRecord[]): boolean { if (files.length === 0) return false; - // "Code" is genuine source (isCodeFile), not merely "anything that isn't a test": a docs-, lockfile-, or - // config-only PR has no code to cover and must not be flagged missing_tests. Mirrors the isCodeFile code-side - // used by slop.ts's buildMissingTestEvidenceFinding and the local-branch/local-scorer source predicates. - const codeFiles = files.filter((file) => file.path && isCodeFile(file.path)); + // "Code" is genuine source that ought to carry tests, not merely "anything that isn't a test": a docs-, + // lockfile-, or config-only PR — or one of only generated/vendored/minified output — has no code to cover + // and must not be flagged missing_tests. Uses slop.ts's shared isTestableCodePath so this signal applies the + // SAME codegen exemption buildMissingTestEvidenceFinding does, instead of a bare isCodeFile that would flag a + // vendored-only diff the gate-side deliberately exempts (#9696). + const codeFiles = files.filter((file) => file.path && isTestableCodePath(file.path)); const testFiles = files.filter((file) => file.path && isTestPath(file.path)); return codeFiles.length > 0 && testFiles.length === 0; } diff --git a/src/signals/improvement.ts b/src/signals/improvement.ts index d861c38521..f936b6782e 100644 --- a/src/signals/improvement.ts +++ b/src/signals/improvement.ts @@ -26,8 +26,7 @@ // live source for complexityDeltas, duplicationDeltas, or patchCoverageDeltaPercent — all three are honest // gaps, not yet wired by design (a later sub-issue's job), and this module must degrade cleanly when they're // absent (see "insufficient signal" below) rather than fabricate a neutral score. -import { buildMissingTestEvidenceFinding, clamp, type SlopChangedFile } from "./slop"; -import { isCodeFile } from "./path-matchers"; +import { buildMissingTestEvidenceFinding, clamp, isTestableCodePath, type SlopChangedFile } from "./slop"; import type { SignalFinding } from "./engine"; export type ImprovementBand = "insufficient-signal" | "none" | "minor" | "moderate" | "significant"; @@ -161,7 +160,10 @@ function finitePatchCoverageDelta(value: number | undefined): number | undefined } function hasCodeFileToEvaluate(changedFiles: SlopChangedFile[] | undefined): boolean { - return (changedFiles ?? []).some((file) => Boolean(file.path) && isCodeFile(file.path)); + // isTestableCodePath, not bare isCodeFile: a diff of only generated/vendored/minified output (e.g. + // `api/service.pb.go`) is NOT code that needs tests, so it must not disambiguate the null from + // buildMissingTestEvidenceFinding into a positive "test evidence present" finding (#9696). + return (changedFiles ?? []).some((file) => Boolean(file.path) && isTestableCodePath(file.path)); } // Fires when at least one function's complexity genuinely dropped (a negative delta) after this PR. Mixed diff --git a/src/signals/slop.ts b/src/signals/slop.ts index 55903dbbb7..d8557f5c0c 100644 --- a/src/signals/slop.ts +++ b/src/signals/slop.ts @@ -21,6 +21,7 @@ export { buildTrivialWhitespaceChurnFinding, clamp, hasClearNoIssueRationale, + isTestableCodePath, slopBandFor, type SlopAssessment, type SlopAssessmentInput, diff --git a/test/unit/contributor-open-pr-monitor.test.ts b/test/unit/contributor-open-pr-monitor.test.ts index b0f6102f3e..ef936252e8 100644 --- a/test/unit/contributor-open-pr-monitor.test.ts +++ b/test/unit/contributor-open-pr-monitor.test.ts @@ -7,7 +7,7 @@ import { mapPendingClassToWorkClassification, } from "../../src/signals/contributor-open-pr-monitor"; import { classifyOpenPullRequest } from "../../src/scoring/pending-pr-scenarios"; -import type { PullRequestRecord, PullRequestReviewRecord } from "../../src/types"; +import type { PullRequestFileRecord, PullRequestRecord, PullRequestReviewRecord } from "../../src/types"; import type { RoleContext } from "../../src/signals/engine"; import { createTestEnv } from "../helpers/d1"; @@ -63,6 +63,23 @@ function approvedReview(pullNumber: number): PullRequestReviewRecord { } describe("contributor open PR monitor", () => { + it("regression (#9696): missingTestsFromFiles exempts a generated/vendored-only diff, matching the gate-side isTestableCodePath", () => { + const { missingTestsFromFiles } = __contributorOpenPrMonitorInternals; + // A vendored-only diff with no tests must NOT be flagged missing_tests — the gate-side signal deliberately + // exempts exactly this case, so telling the contributor to add tests for vendored code was a false negative. + expect(missingTestsFromFiles([{ path: "vendor/dep/util.go", additions: 200, deletions: 0 }] as PullRequestFileRecord[])).toBe(false); + expect(missingTestsFromFiles([{ path: "api/service.pb.go", additions: 500, deletions: 0 }] as PullRequestFileRecord[])).toBe(false); + // Real hand-authored code with no test file is still flagged. + expect(missingTestsFromFiles([{ path: "src/app/service.ts", additions: 50, deletions: 0 }] as PullRequestFileRecord[])).toBe(true); + // Real code WITH a substantive test file is not flagged. + expect( + missingTestsFromFiles([ + { path: "src/app/service.ts", additions: 50, deletions: 0 }, + { path: "test/unit/service.test.ts", additions: 30, deletions: 0 }, + ] as PullRequestFileRecord[]), + ).toBe(false); + }); + it("maps issue #36 classifications from cached review/check metadata", () => { const approved = classifyOpenPullRequest({ pr: pr({ number: 1 }), diff --git a/test/unit/improvement.test.ts b/test/unit/improvement.test.ts index 9355e2d80d..793d3b80be 100644 --- a/test/unit/improvement.test.ts +++ b/test/unit/improvement.test.ts @@ -119,6 +119,17 @@ describe("buildStructuralImprovementAssessment", () => { expect(result.findings).toEqual([expect.objectContaining({ code: "added_test_evidence", severity: "info" })]); }); + it("regression (#9696): a codegen-only diff (no tests) does NOT emit a false added_test_evidence signal", () => { + // api/service.pb.go passes bare isCodeFile (a `.go` extension) but is generated output nobody hand-tests. + // Before the fix, hasCodeFileToEvaluate used bare isCodeFile, so buildMissingTestEvidenceFinding's null + // (no testable code) was read as "test evidence present" and the band became `minor` for a zero-test PR. + const result = buildStructuralImprovementAssessment({ + changedFiles: [{ path: "api/service.pb.go", additions: 500, deletions: 0 }], + }); + expect(result.findings.map((finding) => finding.code)).not.toContain("added_test_evidence"); + expect(result.band).toBe("insufficient-signal"); + }); + it("stacks both corroborating signals (30) to a band that still sits below a single structural signal (35)", () => { const result = buildStructuralImprovementAssessment({ patchCoverageDeltaPercent: 5,