From 548b381f5f89801d0f6055334d95a1a45ba7023c Mon Sep 17 00:00:00 2001 From: ultrahighsuper Date: Fri, 17 Jul 2026 02:26:26 +0900 Subject: [PATCH] fix(mcp): honor free-text tests evidence in loopover_check_test_evidence (#6618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loopover_check_test_evidence` was documented as modeled on `checkSlopRisk`, but its shape only had `changedPaths`/`testFiles` and its handler called `classifyTestCoverage(allPaths)` directly, never consulting `hasLocalTestEvidence`. So a caller whose only evidence is free-text (e.g. "ran `go test ./...` locally, no new file") got an "absent" verdict from this tool, even though `loopover_check_slop_risk` and `loopover_suggest_boundary_tests` correctly credit the exact same evidence via the shared `hasLocalTestEvidence` helper. - Add an optional `tests` field to `checkTestEvidenceShape`, same bounds as the sibling shapes (`z.array(z.string().max(400)).max(2000).optional()`). - Import `hasLocalTestEvidence` and, in the handler, override an otherwise- "absent" classification to "adequate" (with testFileCount >= 1) only when `hasLocalTestEvidence({ tests, testFiles })` is true, plus a distinct guidance line noting the evidence came from the free-text field. - The override applies ONLY above "absent": weak/adequate/strong path-based classifications are returned unchanged, so the tool never becomes more lenient than the path signal once real test-file evidence exists. Adds three test cases to test/unit/mcp-check-test-evidence.test.ts: free-text-only evidence lifts absent→adequate with distinct guidance; an empty `tests: []` stays absent; and a weak path classification stays weak (override does not fire above absent). Closes #6618 --- src/mcp/server.ts | 19 +++++++++-- test/unit/mcp-check-test-evidence.test.ts | 40 +++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 7665f6a0e6..2367d87aa3 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -155,7 +155,7 @@ import { buildTestGenSpec, type LocalWriteActionSpec, } from "./local-write-tools"; -import { classifyTestCoverage, isCodeFile, isTestPath, TEST_FRAMEWORKS } from "../signals/test-evidence"; +import { classifyTestCoverage, hasLocalTestEvidence, isCodeFile, isTestPath, TEST_FRAMEWORKS } from "../signals/test-evidence"; import { applyStepResult, buildPlanDag, nextReadySteps, planProgress, validatePlanDag, type PlanDag } from "../services/plan-dag"; import { buildFocusManifestValidation } from "../services/focus-manifest-validation"; import { isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness } from "../settings/agent-execution"; @@ -1182,6 +1182,7 @@ const checkImprovementPotentialOutputSchema = { const checkTestEvidenceShape = { changedPaths: z.array(z.string().min(1).max(400)).max(2000), testFiles: z.array(z.string().min(1).max(400)).max(2000).optional(), + tests: z.array(z.string().max(400)).max(2000).optional(), }; const checkTestEvidenceOutputSchema = { @@ -3640,12 +3641,24 @@ export class LoopoverMcp { private async checkTestEvidence(input: z.infer>): Promise { await this.enforceToolRateLimit("loopover_check_test_evidence"); const allPaths = [...input.changedPaths, ...(input.testFiles ?? [])]; - const classification = classifyTestCoverage(allPaths); const codeFileCount = input.changedPaths.filter(isCodeFile).length; - const testFileCount = allPaths.filter(isTestPath).length; + let classification = classifyTestCoverage(allPaths); + let testFileCount = allPaths.filter(isTestPath).length; + // Credit free-text `tests` evidence (e.g. "ran `go test ./...` locally, no new file") the same way the + // sibling tools loopover_check_slop_risk / loopover_suggest_boundary_tests already do via + // hasLocalTestEvidence. Only ever LIFT an otherwise-"absent" verdict -- never make this more lenient than + // the path-based signal once real test-file evidence (weak/adequate/strong) already exists. + const creditedByFreeTextTests = + classification === "absent" && hasLocalTestEvidence({ tests: input.tests, testFiles: input.testFiles }); + if (creditedByFreeTextTests) { + classification = "adequate"; + testFileCount = Math.max(testFileCount, 1); + } const guidance: string[] = []; if (codeFileCount === 0) { guidance.push("No hand-authored code files changed, so the missing-test-evidence signal does not apply (e.g. a docs- or config-only change)."); + } else if (creditedByFreeTextTests) { + guidance.push("No test file was detected among the changed paths, but the free-text `tests` evidence you supplied is credited as test evidence (the same way check_slop_risk and suggest_boundary_tests treat it)."); } else if (classification === "absent") { guidance.push("Changed code files carry no test evidence — add or update a test that exercises the change before opening the PR."); } else if (classification === "strong") { diff --git a/test/unit/mcp-check-test-evidence.test.ts b/test/unit/mcp-check-test-evidence.test.ts index 8be09b8023..34fa920a15 100644 --- a/test/unit/mcp-check-test-evidence.test.ts +++ b/test/unit/mcp-check-test-evidence.test.ts @@ -63,4 +63,44 @@ describe("MCP loopover_check_test_evidence (#2235)", () => { expect(data.codeFileCount).toBe(0); expect(data.guidance.join(" ")).toMatch(/does not apply/i); }); + + it("credits free-text tests evidence when no test file is present, lifting absent to adequate (#6618)", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_check_test_evidence", + arguments: { changedPaths: ["src/a.ts", "src/b.ts"], tests: ["ran `go test ./internal/entity` locally, no new file"] }, + }); + const data = result.structuredContent as Result; + expect(data.classification).toBe("adequate"); // lifted from the path-based "absent" + expect(data.testFileCount).toBeGreaterThanOrEqual(1); + expect(data.guidance.join(" ")).toMatch(/free-text `tests`/i); // distinct wording, not the path-derived lines + expect(data.guidance.join(" ")).not.toMatch(/looks strong/i); + }); + + it("does not credit an empty tests array — classification stays absent (#6618)", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_check_test_evidence", + arguments: { changedPaths: ["src/a.ts", "src/b.ts"], tests: [] }, + }); + const data = result.structuredContent as Result; + expect(data.classification).toBe("absent"); + expect(data.testFileCount).toBe(0); + expect(data.guidance.join(" ")).toMatch(/no test evidence/i); + }); + + it("does not apply the override above absent — a weak path classification stays weak (#6618)", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_check_test_evidence", + arguments: { + changedPaths: ["src/a.ts", "src/b.ts", "src/c.ts", "src/d.ts", "src/e.ts"], + testFiles: ["test/a.test.ts"], + tests: ["ran the full suite locally"], + }, + }); + const data = result.structuredContent as Result; + expect(data.classification).toBe("weak"); // real path evidence already present → override must not fire + expect(data.guidance.join(" ")).toMatch(/weak/i); + }); });