From f034f006a05cd0ddd8a9a3ae50ceca0e638f7272 Mon Sep 17 00:00:00 2001 From: e11734937-beep Date: Mon, 6 Jul 2026 01:13:52 +0200 Subject: [PATCH] feat(enrichment): add changed-line coverage-delta analyzer Adds a coverageDelta REES analyzer that flags added lines a PR introduces which the repo's own latest successful CI coverage report records as executed zero times, parsed from the head commit's GitHub Actions coverage artifact (lcov / Istanbul coverage-final.json / Cobertura XML) and intersected with the patch's added lines. Additive and fail-safe: any missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Self-contained in review-enrichment/. Closes #1516 --- .env.example | 4 +- apps/gittensory-ui/src/lib/rees-analyzers.ts | 26 ++ review-enrichment/analyzer-metadata.json | 28 ++ .../src/analyzers/coverage-delta.ts | 384 ++++++++++++++++++ review-enrichment/src/analyzers/registry.ts | 46 +++ review-enrichment/src/render.ts | 1 + review-enrichment/src/types.ts | 12 + .../test/analyzer-registry.test.ts | 1 + review-enrichment/test/coverage-delta.test.ts | 267 ++++++++++++ src/review/enrichment-analyzer-names.ts | 1 + 10 files changed, 769 insertions(+), 1 deletion(-) create mode 100644 review-enrichment/src/analyzers/coverage-delta.ts create mode 100644 review-enrichment/test/coverage-delta.test.ts diff --git a/.env.example b/.env.example index d64398bd7b..38ee163ad2 100644 --- a/.env.example +++ b/.env.example @@ -70,6 +70,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y # i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence +# coverageDelta # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency @@ -84,7 +85,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology # todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting # errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak -# deprecatedDep,revertRecurrence +# deprecatedDep,revertRecurrence,coverageDelta # deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot @@ -92,6 +93,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y # i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence +# coverageDelta # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 7d5074ca41..e5bd1f7589 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1295,6 +1295,32 @@ export const REES_ANALYZERS = [ "Conservative: only a message-confirmed revert commit whose removed range overlaps an added range is reported (one finding per file); line-range overlap is a heuristic across history, and lockfiles/generated/binary paths are skipped. Fail-safe on missing token/invalid slug/fetch error or an aborted signal.", }, }, + { + name: "coverageDelta", + title: "Coverage gaps on changed lines", + category: "quality", + cost: "github-heavy", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + maxRunsProbed: 5, + maxFilesReported: 15, + maxLinesPerFile: 20, + }, + docs: { + summary: + "Flags added lines in a PR that the project's own latest successful CI coverage report records as never executed — measured test gaps on exactly the touched lines, not a guess about whether tests look present.", + looksAt: + "The PR's added new-file line numbers, intersected with the zero-hit lines parsed from the coverage artifact (lcov, Istanbul coverage-final.json, or Cobertura XML) of the head commit's most recent successful workflow run.", + reports: + "Each changed file and the specific added line numbers with no test coverage — never file contents.", + network: + "Calls the GitHub Actions runs and artifacts APIs and downloads one coverage artifact zip, each bounded by fixed fanout and byte caps. Requires GitHub token forwarding.", + notes: + "Conservative and fail-safe: only an added line the report explicitly marks zero-hit is flagged, so a missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Bounded by run, file, and line caps.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 86b48de311..08a1bc0cbe 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1455,6 +1455,34 @@ "network": "Calls the GitHub commits API per probed file and the single-commit API per revert commit, both bounded by fixed fanout caps. Requires GitHub token forwarding for private repos.", "notes": "Conservative: only a message-confirmed revert commit whose removed range overlaps an added range is reported (one finding per file); line-range overlap is a heuristic across history, and lockfiles/generated/binary paths are skipped. Fail-safe on missing token/invalid slug/fetch error or an aborted signal." } + }, + { + "name": "coverageDelta", + "title": "Coverage gaps on changed lines", + "category": "quality", + "cost": "github-heavy", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "maxRunsProbed": 5, + "maxFilesReported": 15, + "maxLinesPerFile": 20 + }, + "docs": { + "summary": "Flags added lines in a PR that the project's own latest successful CI coverage report records as never executed — measured test gaps on exactly the touched lines, not a guess about whether tests look present.", + "looksAt": "The PR's added new-file line numbers, intersected with the zero-hit lines parsed from the coverage artifact (lcov, Istanbul coverage-final.json, or Cobertura XML) of the head commit's most recent successful workflow run.", + "reports": "Each changed file and the specific added line numbers with no test coverage — never file contents.", + "network": "Calls the GitHub Actions runs and artifacts APIs and downloads one coverage artifact zip, each bounded by fixed fanout and byte caps. Requires GitHub token forwarding.", + "notes": "Conservative and fail-safe: only an added line the report explicitly marks zero-hit is flagged, so a missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Bounded by run, file, and line caps." + } } ] } diff --git a/review-enrichment/src/analyzers/coverage-delta.ts b/review-enrichment/src/analyzers/coverage-delta.ts new file mode 100644 index 0000000000..a46b2d833c --- /dev/null +++ b/review-enrichment/src/analyzers/coverage-delta.ts @@ -0,0 +1,384 @@ +// Coverage-delta analyzer (#1516, part of #1499). For the lines a PR ADDS, reads the project's OWN latest +// successful CI coverage report — pulled from the GitHub Actions artifact of the PR head commit — and flags the +// added lines that report records as executed zero times: real measured test gaps on exactly the touched lines, +// not a heuristic about whether tests "look" present. This is heavy/external analysis the no-checkout +// `claude --print` reviewer cannot do; the REES returns it as a brief block the engine splices into the review +// (additive + fail-safe). Distinct from the test-ratio analyzer (added test vs source line volume) and the +// flaky-test analyzer: this intersects the PR's added new-file lines with the coverage report's zero-hit lines. +// Reports only file + line numbers, never code. Any missing token/artifact/parse fails safe to [] — so a fetch +// error can never manufacture a false "untested" finding. +import { inflateRawSync } from "node:zlib"; +import type { + AnalyzerDiagnostics, + CoverageDeltaFinding, + EnrichRequest, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; + +const GITHUB_API = "https://api.github.com"; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const MAX_RUNS_PROBED = 5; // recent successful runs to search for a coverage artifact +const MAX_ARTIFACT_BYTES = 8 * 1024 * 1024; // skip an artifact zip larger than this (bounded download) +const MAX_ENTRY_BYTES = 4 * 1024 * 1024; // skip a single uncompressed zip entry larger than this +const MAX_FILES_REPORTED = 15; // cap findings so the brief stays bounded +const MAX_LINES_PER_FILE = 20; // cap reported uncovered lines per file +// Artifact names that plausibly hold a coverage report; matched case-insensitively against the artifact name. +const COVERAGE_ARTIFACT_RE = /cover|lcov/i; + +/** file path -> the 1-based line numbers the coverage report records as executed zero times. */ +type CoverageMap = Map>; + +type CoverageKind = "lcov" | "istanbul" | "cobertura"; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +/** The slice of a GitHub Actions workflow-run this analyzer reads. */ +interface WorkflowRun { + id?: number; + conclusion?: string | null; + created_at?: string; +} + +/** The slice of a GitHub Actions artifact this analyzer reads. */ +interface Artifact { + id?: number; + name?: string; + size_in_bytes?: number; +} + +/** A decompressed zip entry: its archive name and raw bytes. */ +export interface ZipEntry { + name: string; + data: Buffer; +} + +function addUncovered(map: CoverageMap, file: string, line: number): void { + let set = map.get(file); + if (!set) { + set = new Set(); + map.set(file, set); + } + set.add(line); +} + +/** The 1-based new-file line numbers a unified-diff patch ADDS. Operates on GitHub's per-file `patch` (which has + * no `+++`/`---` headers). Pure — returns [] for an empty patch or one with no valid hunk header. */ +export function addedLineNumbers(patch: string): number[] { + const lines: number[] = []; + if (!patch) return lines; + let newLine = 0; + let active = false; + for (const line of patch.split("\n")) { + if (line.startsWith("@@")) { + const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (!header) { + active = false; + continue; + } + newLine = Number(header[1]); + active = true; + continue; + } + if (!active) continue; + if (line.startsWith("\\")) continue; // "\ No newline at end of file" + const marker = line[0]; + if (marker === "+") { + lines.push(newLine); + newLine += 1; + } else if (marker !== "-") { + newLine += 1; // context line advances the new-file counter; a "-" removal does not + } + } + return lines; +} + +/** Parse an LCOV report into file -> zero-hit line numbers (`DA:,` rows whose hit count is 0). Pure. */ +export function parseLcov(content: string): CoverageMap { + const map: CoverageMap = new Map(); + let file = ""; + for (const raw of content.split("\n")) { + const line = raw.trim(); + if (line.startsWith("SF:")) { + file = line.slice(3); + } else if (line === "end_of_record") { + file = ""; + } else if (file && line.startsWith("DA:")) { + const [num, hits] = line.slice(3).split(","); + const n = Number(num); + if (Number.isInteger(n) && n > 0 && Number(hits) === 0) addUncovered(map, file, n); + } + } + return map; +} + +/** Parse an Istanbul/nyc `coverage-final.json` into file -> zero-hit line numbers. Each file entry maps statement + * ids to hit counts (`s`) and statement ids to source ranges (`statementMap`); a zero-hit statement is uncovered + * at its start line. Pure — malformed JSON or an unexpected shape yields an empty map. */ +export function parseIstanbulJson(content: string): CoverageMap { + const map: CoverageMap = new Map(); + let data: unknown; + try { + data = JSON.parse(content); + } catch { + return map; + } + if (typeof data !== "object" || data === null) return map; + for (const [file, cov] of Object.entries(data as Record)) { + if (typeof cov !== "object" || cov === null) continue; + const { s, statementMap } = cov as { + s?: Record; + statementMap?: Record; + }; + if (!s || !statementMap) continue; + for (const [id, hits] of Object.entries(s)) { + if (hits !== 0) continue; + const start = statementMap[id]?.start?.line; + if (typeof start === "number" && Number.isInteger(start) && start > 0) { + addUncovered(map, file, start); + } + } + } + return map; +} + +/** Parse a Cobertura XML report into file -> zero-hit line numbers. Scans line-by-line (no backtracking regex) + * for `` scope and `` rows. Pure. */ +export function parseCoberturaXml(content: string): CoverageMap { + const map: CoverageMap = new Map(); + let file = ""; + for (const raw of content.split("\n")) { + if (raw.includes("")) { + file = ""; + } else if (file && raw.includes("= Math.max(0, buf.length - 65558); i--) { + if (buf.readUInt32LE(i) === 0x06054b50) { + eocd = i; + break; + } + } + if (eocd < 0) return entries; + const cdSize = buf.readUInt32LE(eocd + 12); + const cdStart = buf.readUInt32LE(eocd + 16); + if (cdStart + cdSize > buf.length) return entries; + let pos = cdStart; + while (pos + 46 <= cdStart + cdSize) { + if (buf.readUInt32LE(pos) !== 0x02014b50) break; // central-directory file header + const method = buf.readUInt16LE(pos + 10); + const compressedSize = buf.readUInt32LE(pos + 20); + const nameLen = buf.readUInt16LE(pos + 28); + const extraLen = buf.readUInt16LE(pos + 30); + const commentLen = buf.readUInt16LE(pos + 32); + const localOffset = buf.readUInt32LE(pos + 42); + const name = buf.toString("utf8", pos + 46, pos + 46 + nameLen); + pos += 46 + nameLen + extraLen + commentLen; + if (localOffset + 30 > buf.length) continue; + const localNameLen = buf.readUInt16LE(localOffset + 26); + const localExtraLen = buf.readUInt16LE(localOffset + 28); + const dataStart = localOffset + 30 + localNameLen + localExtraLen; + if (dataStart + compressedSize > buf.length) continue; + const chunk = buf.subarray(dataStart, dataStart + compressedSize); + if (method === 0) { + if (chunk.length > MAX_ENTRY_BYTES) continue; + entries.push({ name, data: chunk }); + } else if (method === 8) { + try { + entries.push({ name, data: inflateRawSync(chunk, { maxOutputLength: MAX_ENTRY_BYTES }) }); + } catch { + continue; // corrupt deflate stream or output over the cap -> skip this entry + } + } + } + return entries; +} + +/** The coverage format implied by a file name, or null when it is not a recognized report. Pure. */ +export function coverageFileKind(name: string): CoverageKind | null { + const base = (name.split("/").pop() ?? "").toLowerCase(); + if (base === "lcov.info" || base.endsWith(".lcov")) return "lcov"; + if (base === "coverage-final.json") return "istanbul"; + if (base === "cobertura.xml" || base === "coverage.xml") return "cobertura"; + return null; +} + +function parseCoverage(kind: CoverageKind, content: string): CoverageMap { + if (kind === "lcov") return parseLcov(content); + if (kind === "istanbul") return parseIstanbulJson(content); + return parseCoberturaXml(content); +} + +/** True when a coverage report path refers to the PR file — exact, or the report path ends with `/` + * (coverage tools commonly emit workspace-absolute paths). Pure. */ +export function pathMatches(coveragePath: string, prFile: string): boolean { + const c = coveragePath.replace(/\\/g, "/"); + const p = prFile.replace(/\\/g, "/"); + return c === p || c.endsWith(`/${p}`); +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; +} + +/** Fetch + parse JSON with the shared bounded-fetch guard rails; returns the parsed body or null on any + * error/non-200 so the caller degrades that one lookup rather than throwing. */ +async function fetchGithubJson( + url: string, + headers: Record, + fetchFn: typeof fetch, + category: string, + options: ScanOptions, +): Promise { + const fetchOptions = { + endpointCategory: category, + headers, + signal: options.signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "coverage-delta", + subcall: category, + maxBytes: 1024 * 1024, + maxCallsPerCategory: MAX_RUNS_PROBED + 2, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok ? response.data : null; +} + +/** Download an artifact zip via the injected fetch, bounded by a declared and actual byte cap. Returns the bytes + * or null on any error/non-ok/oversize. GitHub redirects the zip endpoint to a signed URL; fetch follows it. */ +async function fetchArtifactZip( + url: string, + headers: Record, + fetchFn: typeof fetch, + signal?: AbortSignal, +): Promise { + try { + const response = await fetchFn(url, { headers, signal }); + if (!response.ok) return null; + const declared = Number(response.headers.get("content-length") ?? ""); + if (Number.isFinite(declared) && declared > MAX_ARTIFACT_BYTES) return null; + const bytes = await response.arrayBuffer(); + if (bytes.byteLength > MAX_ARTIFACT_BYTES) return null; + return Buffer.from(bytes); + } catch { + return null; + } +} + +/** Analyzer entrypoint: PR added lines -> latest successful CI run for headSha -> coverage artifact -> parsed + * zero-hit lines -> intersection with the added lines. Fail-safe: any missing token/head-sha/invalid slug/failed + * fetch/unparseable artifact/aborted signal degrades to []. */ +export async function scanCoverageDelta( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, headSha, githubToken, files = [] } = req; + if (!githubToken || !headSha) return []; + const parts = repoFullName.split("/"); + if (parts.length !== 2) return []; + const [owner, repo] = parts; + if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + // Index the PR's added new-file lines per file before any network round-trip; nothing to check if none. + const changed = new Map(); + for (const file of files) { + const added = addedLineNumbers(file.patch ?? ""); + if (added.length > 0) changed.set(file.path, added); + } + if (changed.size === 0) return []; + + const headers = githubHeaders(githubToken); + const base = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`; + + const runsBody = await fetchGithubJson<{ workflow_runs?: WorkflowRun[] }>( + `${base}/actions/runs?head_sha=${encodeURIComponent(headSha)}&status=success&per_page=20`, + headers, + fetchFn, + "github-actions-runs", + options, + ); + const runs = (runsBody?.workflow_runs ?? []) + .filter((run): run is WorkflowRun & { id: number } => run.conclusion === "success" && typeof run.id === "number") + .sort((a, b) => (b.created_at ?? "").localeCompare(a.created_at ?? "")) + .slice(0, MAX_RUNS_PROBED); + if (runs.length === 0) return []; + + let coverage: CoverageMap | null = null; + for (const run of runs) { + if (options.signal?.aborted || coverage) break; + const artifactsBody = await fetchGithubJson<{ artifacts?: Artifact[] }>( + `${base}/actions/runs/${run.id}/artifacts`, + headers, + fetchFn, + "github-actions-artifacts", + options, + ); + const artifact = (artifactsBody?.artifacts ?? []) + .filter( + (a): a is Artifact & { id: number; name: string } => + typeof a.id === "number" && + typeof a.name === "string" && + COVERAGE_ARTIFACT_RE.test(a.name) && + (a.size_in_bytes ?? 0) <= MAX_ARTIFACT_BYTES, + ) + .sort((a, b) => (a.size_in_bytes ?? 0) - (b.size_in_bytes ?? 0))[0]; + if (!artifact) continue; + const zip = await fetchArtifactZip(`${base}/actions/artifacts/${artifact.id}/zip`, headers, fetchFn, options.signal); + if (!zip) continue; + for (const entry of readZipEntries(zip)) { + const kind = coverageFileKind(entry.name); + if (!kind) continue; + const parsed = parseCoverage(kind, entry.data.toString("utf8")); + if (parsed.size > 0) { + coverage = parsed; + break; + } + } + } + if (!coverage) return []; + + const findings: CoverageDeltaFinding[] = []; + for (const [file, addedLines] of changed) { + if (findings.length >= MAX_FILES_REPORTED) break; + let match: Set | undefined; + for (const [coveragePath, zeroHit] of coverage) { + if (pathMatches(coveragePath, file)) { + match = zeroHit; + break; + } + } + if (!match) continue; + const zero = match; + const uncoveredLines = addedLines.filter((line) => zero.has(line)).slice(0, MAX_LINES_PER_FILE); + if (uncoveredLines.length > 0) findings.push({ file, uncoveredLines }); + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index e26dea04e1..330a8ea2d1 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -50,6 +50,7 @@ import { scanFlakyTest } from "./flaky-test.js"; import { scanApiBreak } from "./api-break.js"; import { scanDeprecatedDependencies } from "./deprecated-dep.js"; import { scanRevertRecurrence } from "./revert-recurrence.js"; +import { scanCoverageDelta } from "./coverage-delta.js"; import type { AnalyzerDescriptor, AnalyzerFn, @@ -1489,6 +1490,51 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanRevertRecurrence(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "coverageDelta", + title: "Coverage gaps on changed lines", + category: "quality", + cost: "github-heavy", + defaultEnabled: true, + requires: ["files", "github-token", "head-sha"], + limits: { + maxRunsProbed: 5, + maxFilesReported: 15, + maxLinesPerFile: 20, + }, + docs: { + summary: + "Flags added lines in a PR that the project's own latest successful CI coverage report records as never executed — measured test gaps on exactly the touched lines, not a guess about whether tests look present.", + looksAt: + "The PR's added new-file line numbers, intersected with the zero-hit lines parsed from the coverage artifact (lcov, Istanbul coverage-final.json, or Cobertura XML) of the head commit's most recent successful workflow run.", + reports: + "Each changed file and the specific added line numbers with no test coverage — never file contents.", + network: + "Calls the GitHub Actions runs and artifacts APIs and downloads one coverage artifact zip, each bounded by fixed fanout and byte caps. Requires GitHub token forwarding.", + notes: + "Conservative and fail-safe: only an added line the report explicitly marks zero-hit is flagged, so a missing token, absent artifact, unparseable report, or fetch error yields no finding rather than a false one. Bounded by run, file, and line caps.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = [ + "### Coverage gaps on changed lines (added lines the project's own CI coverage marks as untested)", + ]; + for (const item of findings) { + const shown = item.uncoveredLines.slice(0, 8); + const more = + item.uncoveredLines.length > shown.length + ? ` (+${item.uncoveredLines.length - shown.length} more)` + : ""; + const label = item.uncoveredLines.length === 1 ? "line" : "lines"; + lines.push( + `- ${helpers.safeCodeSpan(item.file)} — uncovered added ${label} ${helpers.safeCodeSpan(shown.join(", "))}${more}`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanCoverageDelta(req, fetch, { signal, analysis, diagnostics }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 66a4dada67..022b0fd3cb 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -501,6 +501,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("apiBreak", findings.apiBreak)); lines.push(...renderDescriptorSection("deprecatedDep", findings.deprecatedDep)); lines.push(...renderDescriptorSection("revertRecurrence", findings.revertRecurrence)); + lines.push(...renderDescriptorSection("coverageDelta", findings.coverageDelta)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 743dc842fd..9a877404da 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -629,6 +629,17 @@ export interface RevertRecurrenceFinding { revertedPr?: number; } +/** Added new-file lines in a changed file that the project's OWN latest successful CI coverage report marks as + * executed zero times — real measured test gaps on exactly the lines this PR introduces, not a heuristic about + * whether tests "look" present (#1516, part of #1499). Derived from the GitHub Actions coverage artifact of the + * PR head commit (lcov / Istanbul `coverage-final.json` / Cobertura XML), intersected with the patch's added + * line numbers. Reports the file and the uncovered line numbers only — never file contents. */ +export interface CoverageDeltaFinding { + file: string; + /** 1-based new-file line numbers among the PR's added lines the coverage report records as never executed. */ + uncoveredLines: number[]; +} + export interface BriefFindings { dependency?: DependencyFinding[]; dependencyDiff?: DependencyDiffFinding[]; @@ -682,6 +693,7 @@ export interface BriefFindings { apiBreak?: ApiBreakFinding[]; deprecatedDep?: DeprecatedDependencyFinding[]; revertRecurrence?: RevertRecurrenceFinding[]; + coverageDelta?: CoverageDeltaFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 26b5a558c5..9edf4b1408 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -62,6 +62,7 @@ const EXPECTED_ANALYZERS = [ "apiBreak", "deprecatedDep", "revertRecurrence", + "coverageDelta", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/coverage-delta.test.ts b/review-enrichment/test/coverage-delta.test.ts new file mode 100644 index 0000000000..7be40dd48b --- /dev/null +++ b/review-enrichment/test/coverage-delta.test.ts @@ -0,0 +1,267 @@ +// Units for the coverage-delta analyzer (#1516). Own file (not enrichment.test.ts) so concurrent analyzer PRs +// don't collide. All network is mocked; a real deflated zip is built in-test from node:zlib. Runs against dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { deflateRawSync } from "node:zlib"; +import { + addedLineNumbers, + parseLcov, + parseIstanbulJson, + parseCoberturaXml, + readZipEntries, + coverageFileKind, + pathMatches, + scanCoverageDelta, +} from "../dist/analyzers/coverage-delta.js"; +import { renderBrief } from "../dist/render.js"; +import { resetExternalFetchCircuitBreakerForTest } from "../dist/external-fetch.js"; + +const jsonResponse = (body, code = 200) => new Response(JSON.stringify(body), { status: code }); + +// A PR patch that adds new-file lines 11-13 to src/a.ts (12 is a context line, not added). +const PR_PATCH = "@@ -10,3 +10,5 @@\n ctx1\n+add11\n keepctx\n+add13\n+add14\n ctx2"; +const LCOV = "SF:src/a.ts\nDA:11,0\nDA:13,5\nDA:14,0\nend_of_record\n"; + +// Build a single-entry ZIP (central directory + EOCD) around `data` under `name`, deflated (method 8). +function buildZip(name, data, { store = false } = {}) { + const nameBuf = Buffer.from(name, "utf8"); + const raw = Buffer.from(data, "utf8"); + const body = store ? raw : deflateRawSync(raw); + const method = store ? 0 : 8; + const local = Buffer.alloc(30); + local.writeUInt32LE(0x04034b50, 0); + local.writeUInt16LE(method, 8); + local.writeUInt32LE(body.length, 18); + local.writeUInt32LE(raw.length, 22); + local.writeUInt16LE(nameBuf.length, 26); + const localOffset = 0; + const localRecord = Buffer.concat([local, nameBuf, body]); + const central = Buffer.alloc(46); + central.writeUInt32LE(0x02014b50, 0); + central.writeUInt16LE(method, 10); + central.writeUInt32LE(body.length, 20); + central.writeUInt32LE(raw.length, 24); + central.writeUInt16LE(nameBuf.length, 28); + central.writeUInt32LE(localOffset, 42); + const centralRecord = Buffer.concat([central, nameBuf]); + const cdStart = localRecord.length; + const eocd = Buffer.alloc(22); + eocd.writeUInt32LE(0x06054b50, 0); + eocd.writeUInt16LE(1, 8); + eocd.writeUInt16LE(1, 10); + eocd.writeUInt32LE(centralRecord.length, 12); + eocd.writeUInt32LE(cdStart, 16); + return Buffer.concat([localRecord, centralRecord, eocd]); +} + +const req = (files, over = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + headSha: "sha123", + githubToken: "ghp_test", + files, + ...over, +}); + +// Route by URL: runs list, per-run artifacts list, and the artifact zip download. +const routed = ({ runs, artifacts, zip }) => async (url) => { + if (url.includes("/actions/runs?")) return jsonResponse({ workflow_runs: runs }); + if (url.endsWith("/artifacts")) return jsonResponse({ artifacts }); + if (url.includes("/artifacts/") && url.endsWith("/zip")) { + return new Response(zip, { status: 200 }); + } + return jsonResponse({}, 404); +}; + +const RUNS = [{ id: 7, conclusion: "success", created_at: "2026-06-01T00:00:00Z" }]; +const ARTIFACTS = [{ id: 99, name: "coverage-report", size_in_bytes: 1000 }]; + +test("addedLineNumbers: added new-file lines only, skipping context and removals", () => { + assert.deepEqual(addedLineNumbers(PR_PATCH), [11, 13, 14]); + assert.deepEqual(addedLineNumbers(""), []); + assert.deepEqual(addedLineNumbers("no hunk\n+x"), []); // no @@ header ⇒ inactive + assert.deepEqual(addedLineNumbers("@@ bad @@\n+x"), []); // header regex fails ⇒ inactive + // Multiple hunks; "\ No newline" marker is skipped and does not advance the counter. + assert.deepEqual(addedLineNumbers("@@ -1,0 +1,1 @@\n+a\n@@ -5,1 +5,2 @@\n c\n+b\n\\ No newline"), [1, 6]); +}); + +test("parseLcov: DA rows with zero hits become uncovered lines", () => { + const map = parseLcov(LCOV); + assert.deepEqual([...(map.get("src/a.ts") ?? [])], [11, 14]); + assert.equal(parseLcov("SF:x\nDA:bad,0\nDA:0,0\nend_of_record").get("x")?.size ?? 0, 0); + assert.equal(parseLcov("DA:5,0").size, 0); // no SF: scope ⇒ ignored +}); + +test("parseIstanbulJson: zero-hit statements map to their start line; bad shapes yield empty", () => { + const json = JSON.stringify({ + "src/a.ts": { + s: { "0": 1, "1": 0, "2": 0 }, + statementMap: { "0": { start: { line: 11 } }, "1": { start: { line: 13 } }, "2": { start: { line: 14 } } }, + }, + "src/skip.ts": { s: { "0": 3 } }, // no statementMap ⇒ skipped + }); + const map = parseIstanbulJson(json); + assert.deepEqual([...(map.get("src/a.ts") ?? [])], [13, 14]); + assert.equal(map.has("src/skip.ts"), false); + assert.equal(parseIstanbulJson("not json").size, 0); + assert.equal(parseIstanbulJson("[]").size, 0); // array ⇒ no file entries with s/statementMap + assert.equal(parseIstanbulJson("null").size, 0); +}); + +test("parseCoberturaXml: zero-hit lines within a class scope become uncovered", () => { + const xml = [ + '', + '', + '', + '', + "", + '', // outside any class scope ⇒ ignored + ].join("\n"); + assert.deepEqual([...(parseCoberturaXml(xml).get("src/a.ts") ?? [])], [11, 14]); +}); + +test("readZipEntries: reads a deflated and a stored entry; rejects a non-zip buffer", () => { + const deflated = readZipEntries(buildZip("lcov.info", LCOV)); + assert.equal(deflated[0]?.name, "lcov.info"); + assert.equal(deflated[0]?.data.toString("utf8"), LCOV); + const stored = readZipEntries(buildZip("lcov.info", LCOV, { store: true })); + assert.equal(stored[0]?.data.toString("utf8"), LCOV); + assert.deepEqual(readZipEntries(Buffer.from("not a zip file at all")), []); + assert.deepEqual(readZipEntries(Buffer.alloc(4)), []); // shorter than the EOCD record +}); + +test("coverageFileKind: recognizes lcov / istanbul / cobertura names, else null", () => { + assert.equal(coverageFileKind("out/lcov.info"), "lcov"); + assert.equal(coverageFileKind("report.lcov"), "lcov"); + assert.equal(coverageFileKind("coverage/coverage-final.json"), "istanbul"); + assert.equal(coverageFileKind("cobertura.xml"), "cobertura"); + assert.equal(coverageFileKind("coverage.xml"), "cobertura"); + assert.equal(coverageFileKind("README.md"), null); +}); + +test("pathMatches: exact or workspace-absolute suffix match", () => { + assert.equal(pathMatches("src/a.ts", "src/a.ts"), true); + assert.equal(pathMatches("/home/runner/work/repo/repo/src/a.ts", "src/a.ts"), true); + assert.equal(pathMatches("other/a.ts", "src/a.ts"), false); +}); + +test("scanCoverageDelta: flags added lines the coverage report marks uncovered (lcov via deflated zip)", async () => { + const findings = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("lcov.info", LCOV) }), + ); + assert.deepEqual(findings, [{ file: "src/a.ts", uncoveredLines: [11, 14] }]); +}); + +test("scanCoverageDelta: an istanbul coverage-final.json artifact is parsed the same way", async () => { + const json = JSON.stringify({ + "src/a.ts": { + s: { "0": 0, "1": 2, "2": 0 }, + statementMap: { "0": { start: { line: 11 } }, "1": { start: { line: 13 } }, "2": { start: { line: 14 } } }, + }, + }); + const findings = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("coverage-final.json", json) }), + ); + assert.deepEqual(findings, [{ file: "src/a.ts", uncoveredLines: [11, 14] }]); +}); + +test("scanCoverageDelta: fully covered added lines produce no finding", async () => { + const findings = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("lcov.info", "SF:src/a.ts\nDA:11,2\nDA:13,1\nDA:14,3\nend_of_record") }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCoverageDelta: requires a github token, a head sha, and a single valid repo slug", async () => { + const call = routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("lcov.info", LCOV) }); + const files = [{ path: "src/a.ts", patch: PR_PATCH }]; + assert.deepEqual(await scanCoverageDelta(req(files, { githubToken: undefined }), call), []); + assert.deepEqual(await scanCoverageDelta(req(files, { headSha: undefined }), call), []); + assert.deepEqual(await scanCoverageDelta(req(files, { repoFullName: "octo/repo/extra" }), call), []); + assert.deepEqual(await scanCoverageDelta(req(files, { repoFullName: "bad slug/x!" }), call), []); +}); + +test("scanCoverageDelta: a PR that adds no lines never touches the network", async () => { + let called = false; + const out = await scanCoverageDelta(req([{ path: "src/a.ts", patch: "@@ -5,2 +5,0 @@\n-gone1\n-gone2" }]), async () => { + called = true; + return jsonResponse({}); + }); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanCoverageDelta: no successful run, or no coverage artifact, yields no finding", async () => { + const files = [{ path: "src/a.ts", patch: PR_PATCH }]; + const noRuns = await scanCoverageDelta(req(files), routed({ runs: [], artifacts: ARTIFACTS, zip: Buffer.alloc(0) })); + assert.deepEqual(noRuns, []); + const noArtifact = await scanCoverageDelta( + req(files), + routed({ runs: RUNS, artifacts: [{ id: 1, name: "build-logs", size_in_bytes: 10 }], zip: Buffer.alloc(0) }), + ); + assert.deepEqual(noArtifact, []); +}); + +test("scanCoverageDelta: an oversized artifact is skipped before download", async () => { + const findings = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: [{ id: 99, name: "coverage", size_in_bytes: 50 * 1024 * 1024 }], zip: buildZip("lcov.info", LCOV) }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCoverageDelta: a coverage report for a different file is not matched", async () => { + const findings = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("lcov.info", "SF:src/other.ts\nDA:11,0\nend_of_record") }), + ); + assert.deepEqual(findings, []); +}); + +test("scanCoverageDelta: fails safe on a non-ok runs fetch or a throwing fetch", async () => { + resetExternalFetchCircuitBreakerForTest(); + const files = [{ path: "src/a.ts", patch: PR_PATCH }]; + assert.deepEqual(await scanCoverageDelta(req(files), async () => jsonResponse({}, 500)), []); + resetExternalFetchCircuitBreakerForTest(); + assert.deepEqual( + await scanCoverageDelta(req(files), async () => { + throw new Error("network"); + }), + [], + ); +}); + +test("scanCoverageDelta: fails safe when the zip download errors", async () => { + resetExternalFetchCircuitBreakerForTest(); + const out = await scanCoverageDelta(req([{ path: "src/a.ts", patch: PR_PATCH }]), async (url) => { + if (url.includes("/actions/runs?")) return jsonResponse({ workflow_runs: RUNS }); + if (url.endsWith("/artifacts")) return jsonResponse({ artifacts: ARTIFACTS }); + return new Response("nope", { status: 500 }); + }); + assert.deepEqual(out, []); +}); + +test("scanCoverageDelta: stops on an already-aborted signal without a finding", async () => { + const out = await scanCoverageDelta( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed({ runs: RUNS, artifacts: ARTIFACTS, zip: buildZip("lcov.info", LCOV) }), + { signal: AbortSignal.abort() }, + ); + assert.deepEqual(out, []); +}); + +test("renderBrief emits a public-safe coverage-delta block", () => { + const { promptSection } = renderBrief({ + coverageDelta: [{ file: "src/a.ts", uncoveredLines: [11, 14] }], + }); + assert.match(promptSection, /Coverage gaps on changed lines/); + assert.match(promptSection, /src\/a\.ts/); + assert.match(promptSection, /11, 14/); +}); + +test("renderBrief omits the coverage-delta block when there are no findings", () => { + assert.equal(renderBrief({ coverageDelta: [] }).promptSection, ""); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index ffdb9b8608..0760f1b218 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -56,6 +56,7 @@ export const REES_ANALYZER_NAMES = [ "apiBreak", "deprecatedDep", "revertRecurrence", + "coverageDelta", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number];