refactor(coverage): give the changed-lines diff one owner and pass it on stdin

package.json

@@ -150,7 +150,7 @@
     "check:gate-manifest": "node --experimental-strip-types scripts/gate/check.ts",
     "check:gate-manifest:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/gate/*.test.ts",
     "check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts",
-    "check:coverage-changed:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts",
+    "check:coverage-changed:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/lib/repo-git.test.ts scripts/lib/repo-path.test.ts scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts",
     "check:coverage-crap": "node --experimental-strip-types scripts/coverage-crap/run.ts",
     "check:coverage-crap:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-crap/model.test.ts scripts/coverage-crap/patches.test.ts scripts/coverage-crap/run.test.ts",
     "check:layering": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/layering/*.test.ts && node --experimental-strip-types scripts/layering/check.ts",

scripts/coverage-changed/model.test.ts

@@ -1,24 +1,15 @@
 import assert from 'node:assert/strict';
 import { test } from 'node:test';
+import { type ChangedFileDiff, parseUnifiedDiff } from '../lib/repo-git.ts';
 import {
   CHANGED_LINE_COVERAGE_THRESHOLD,
   computeChangedCoverage,
   ignoredLineSet,
   isIncludableSource,
   isTestFile,
-  normalizePath,
   parseLcov,
-  parseUnifiedDiff,
-  type ChangedFileDiff,
 } from './model.ts';
 
-test('normalizePath strips root, b/ and ./ prefixes and posixifies', () => {
-  assert.equal(normalizePath('/repo/src/a.ts', '/repo'), 'src/a.ts');
-  assert.equal(normalizePath('b/src/a.ts'), 'src/a.ts');
-  assert.equal(normalizePath('./src/a.ts'), 'src/a.ts');
-  assert.equal(normalizePath(String.raw`src\a.ts`), 'src/a.ts');
-});
-
 test('parseLcov reads DA hits and BRDA taken, treating "-" as not taken', () => {
   const lcov = [
     'SF:/repo/src/a.ts',
@@ -43,46 +34,6 @@ test('parseLcov reads DA hits and BRDA taken, treating "-" as not taken', () =>
   assert.equal(index.get('src/b.ts')?.lineHits.get(10), 1);
 });
 
-test('parseUnifiedDiff numbers added lines per hunk and ignores deleted lines', () => {
-  const diff = [
-    'diff --git a/src/a.ts b/src/a.ts',
-    '--- a/src/a.ts',
-    '+++ b/src/a.ts',
-    '@@ -1,0 +1,2 @@',
-    '+const a = 1;',
-    '+const b = 2;',
-    '@@ -10,1 +12,1 @@',
-    '-old line',
-    '+new line',
-  ].join('\n');
-  const [file] = parseUnifiedDiff(diff);
-  assert.equal(file.path, 'src/a.ts');
-  assert.equal(file.deleted, false);
-  assert.deepEqual(file.added, [1, 2, 12]);
-});
-
-test('parseUnifiedDiff marks deleted files and keeps rename destinations', () => {
-  const diff = [
-    'diff --git a/src/gone.ts b/src/gone.ts',
-    '--- a/src/gone.ts',
-    '+++ /dev/null',
-    '@@ -1,1 +0,0 @@',
-    '-was here',
-    'diff --git a/src/old.ts b/src/new.ts',
-    'rename from src/old.ts',
-    'rename to src/new.ts',
-    '--- a/src/old.ts',
-    '+++ b/src/new.ts',
-    '@@ -3,0 +4,1 @@',
-    '+added on new path',
-  ].join('\n');
-  const files = parseUnifiedDiff(diff);
-  const gone = files.find((f) => f.path === 'src/gone.ts');
-  assert.equal(gone?.deleted, true);
-  const renamed = files.find((f) => f.path === 'src/new.ts');
-  assert.deepEqual(renamed?.added, [4]);
-});
-
 test('source/test classification matches the coverage include glob', () => {
   assert.equal(isIncludableSource('src/core/x.ts'), true);
   assert.equal(isIncludableSource('src/core/x.test.ts'), false);

scripts/coverage-changed/model.ts

@@ -10,7 +10,8 @@
 // lcov was dropped by an exclude glob — reported (non-gating) so exclusions
 // cannot silently absorb new logic.
 
-import { repoRelativePath } from '../lib/repo-path.ts';
+import type { ChangedFileDiff } from '../lib/repo-git.ts';
+import { normalizePath } from '../lib/repo-path.ts';
 
 // Single source of truth for the gate. Changed-line coverage below this
 // percentage fails the Coverage CI job (unless waived).
@@ -27,24 +28,6 @@ export type LcovFile = {
 
 export type CoverageIndex = ReadonlyMap<string, LcovFile>;
 
-export type ChangedFileDiff = {
-  readonly path: string;
-  // New-side line numbers of '+' lines, ascending and de-duplicated.
-  readonly added: readonly number[];
-  // `+++ /dev/null`: the file was deleted, so it contributes no changed lines.
-  readonly deleted: boolean;
-};
-
-// Normalize an lcov/diff path to a repo-relative posix path, then drop the `a/`-style prefixes git
-// writes. Relativization itself is shared with the coverage-map keys (scripts/lib/repo-path.ts): a
-// key from another checkout must relativize identically here and there, or the two never join.
-export function normalizePath(raw: string, rootDir?: string): string {
-  let p = repoRelativePath(raw, rootDir);
-  if (p.startsWith('b/')) p = p.slice(2);
-  if (p.startsWith('./')) p = p.slice(2);
-  return p;
-}
-
 function parseDaRecord(body: string): readonly [number, number] | null {
   const [lineNo, hits] = body.split(',');
   const n = Number(lineNo);
@@ -90,63 +73,6 @@ export function parseLcov(text: string, rootDir?: string): CoverageIndex {
   return index;
 }
 
-type DiffSection = { path: string; added: Set<number>; deleted: boolean; cursor: number };
-
-function newStartOfHunk(line: string): number | null {
-  const match = /@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line);
-  return match ? Number(match[1]) : null;
-}
-
-function pathFromDiffHeader(line: string): string {
-  const header = /^diff --git a\/.+ b\/(.+)$/.exec(line);
-  return header ? normalizePath(header[1]!) : '';
-}
-
-// Fold one line of a file's diff body into its section. Renames+edits report
-// their added lines under the destination (`+++ b/...`); pure renames and
-// deletions add nothing. `--unified=0` means no context lines in practice.
-function foldDiffBodyLine(section: DiffSection, line: string): void {
-  if (line.startsWith('+++ ')) {
-    const target = line.slice(4).trim();
-    if (target === '/dev/null') section.deleted = true;
-    else section.path = normalizePath(target);
-  } else if (line.startsWith('@@')) {
-    const start = newStartOfHunk(line);
-    if (start !== null) section.cursor = start;
-  } else if (line.startsWith('+') && !line.startsWith('+++')) {
-    section.added.add(section.cursor);
-    section.cursor += 1;
-  } else if (line.startsWith('-') || line.startsWith('\\')) {
-    // Removed line or "\ No newline": occupies no new-side number.
-  } else {
-    section.cursor += 1;
-  }
-}
-
-export function parseUnifiedDiff(diff: string): ChangedFileDiff[] {
-  const files: ChangedFileDiff[] = [];
-  let section: DiffSection | null = null;
-  const flush = (): void => {
-    if (section && section.path) {
-      files.push({
-        path: section.path,
-        added: [...section.added].sort((a, b) => a - b),
-        deleted: section.deleted,
-      });
-    }
-  };
-  for (const line of diff.split('\n')) {
-    if (line.startsWith('diff --git ')) {
-      flush();
-      section = { path: pathFromDiffHeader(line), added: new Set(), deleted: false, cursor: 0 };
-    } else if (section) {
-      foldDiffBodyLine(section, line);
-    }
-  }
-  flush();
-  return files;
-}
-
 // Vitest coverage `include` spans two roots: `src/**/*.ts` and
 // `packages/*/src/**/*.ts`. Test files are excluded there and carry no product
 // logic, so they never count toward the gate or the excluded tally (which

scripts/coverage-changed/run.ts

@@ -16,13 +16,8 @@ import path from 'node:path';
 import { pathToFileURL } from 'node:url';
 import { runCmdSync } from '@agent-device/host-kit/command';
 import { parseScriptArgs } from '../lib/cli-args.ts';
-import { readChangedLinesDiff } from '../lib/repo-git.ts';
-import {
-  computeChangedCoverage,
-  parseLcov,
-  parseUnifiedDiff,
-  type ChangedCoverageResult,
-} from './model.ts';
+import { parseUnifiedDiff, readChangedLinesDiff } from '../lib/repo-git.ts';
+import { computeChangedCoverage, parseLcov, type ChangedCoverageResult } from './model.ts';
 
 const USAGE = 'Usage: pnpm check:coverage-changed [--base <ref>]\n';
 const LCOV_PATH = 'coverage/lcov.info';

scripts/coverage-crap/model.ts

@@ -1,7 +1,7 @@
 // Pure pieces of the coverage-based CRAP verdict: which Istanbul file records fallow can trust,
 // whether the two reports of one coverage run agree about what it measured, and how the scored
 // functions are rendered. run.ts owns the git, fallow, config and file I/O.
-import type { ChangedFileDiff } from '../coverage-changed/model.ts';
+import type { ChangedFileDiff } from '../lib/repo-git.ts';
 import { repoRelativePath } from '../lib/repo-path.ts';
 
 type IstanbulCounters = Readonly<{
@@ -185,7 +185,7 @@ export type TouchedFile = Readonly<{ path: string; addedLines: number }>;
 
 /**
  * The files whose lines this change edits. A pure rename, delete, or mode change adds no lines, so
- * it contributes none — which is also exactly the set of files fallow's `--diff-file` scoping can
+ * it contributes none — which is also exactly the set of files fallow's `--diff-stdin` scoping can
  * charge a finding to, since that filter indexes the same added lines.
  */
 export function readTouchedFiles(diffs: readonly ChangedFileDiff[]): TouchedFile[] {

scripts/coverage-crap/run.test.ts

@@ -218,19 +218,27 @@ test('editing a function that already scores too high blocks, even though the sc
 test("the score of an untouched file is not this change's verdict", () => {
   // Half of this is the exit code: the map holds `src/debt.ts` at CRAP 42 and the run is still green,
   // which only happens if fallow filtered its findings by this diff rather than scoring the repo.
-  // The other half is that diff itself, since a flag that silently went missing would score
-  // everything and a diff built from the wrong range would look just as clean.
+  // The diff reaches fallow on stdin and never through the checkout, so seed the artifact an
+  // earlier release wrote with a scope charging the untouched debt file: a gate that read that
+  // file again would report `src/debt.ts:1` at 42 and exit 1, and one that rewrote it would leave
+  // different content behind. Scoping on stdin does neither.
   writeCoverage({ 'src/debt.ts': {}, 'src/clean.ts': { hits: 4 } });
   write('src/clean.ts', `${TRIVIAL}export const also_trivial = 2;\n`);
   commit('clean edit, debt left alone');
+  const staleDiff = [
+    'diff --git a/src/debt.ts b/src/debt.ts',
+    '--- a/src/debt.ts',
+    '+++ b/src/debt.ts',
+    '@@ -1,0 +1,1 @@',
+    '+export const planted = 1;',
+  ].join('\n');
+  write('coverage/fallow-crap.diff', staleDiff);
 
   const { code, out } = check(['--base', HEADS[0]!]);
 
   assert.equal(code, 0);
   assert.doesNotMatch(out, /src\/debt\.ts:1/);
-  const scoped = fs.readFileSync(path.join(repo, 'coverage/fallow-crap.diff'), 'utf8');
-  assert.match(scoped, /^\+\+\+ b\/src\/clean\.ts$/m);
-  assert.doesNotMatch(scoped, /^\+\+\+ b\/src\/debt\.ts$/m);
+  assert.equal(fs.readFileSync(path.join(repo, 'coverage/fallow-crap.diff'), 'utf8'), staleDiff);
 });
 
 test('a pure rename does not touch the functions that moved with the file', () => {
@@ -261,7 +269,7 @@ test('a rename that also edits the body owes the edited lines', () => {
 });
 
 test('deleting lines from a debt function does not charge the function that survives them', () => {
-  // Measured, not assumed: fallow's `--diff-file` index holds added lines only, so a hunk that is
+  // Measured, not assumed: fallow's diff-scoping index holds added lines only, so a hunk that is
   // nothing but removals contributes no line to intersect a function's span with, and this run
   // agrees rather than overriding it — the same reason the whole-file and pure-rename exemptions
   // exist. The residual is deliberate and not free: a deletion can even raise a surviving

scripts/coverage-crap/run.ts

@@ -16,8 +16,8 @@ import path from 'node:path';
 import { pathToFileURL } from 'node:url';
 import { runCmdSync } from '@agent-device/host-kit/command';
 import { parseScriptArgs } from '../lib/cli-args.ts';
-import { parseLcov, parseUnifiedDiff } from '../coverage-changed/model.ts';
-import { readChangedLinesDiff } from '../lib/repo-git.ts';
+import { parseLcov } from '../coverage-changed/model.ts';
+import { parseUnifiedDiff, readChangedLinesDiff } from '../lib/repo-git.ts';
 import { repoRelativePath } from '../lib/repo-path.ts';
 import {
   coverageMapPaths,
@@ -42,7 +42,6 @@ const USAGE = 'Usage: pnpm check:coverage-crap [--base <ref> | --all]\n';
 const COVERAGE_PATH = 'coverage/coverage-istanbul.json';
 const LCOV_PATH = 'coverage/lcov.info';
 const SANITIZED_PATH = 'coverage/fallow-crap-coverage.json';
-const DIFF_PATH = 'coverage/fallow-crap.diff';
 const FALLOW_CONFIG = 'fallow-crap.json';
 const CRAP_THRESHOLD = 30;
 const FALLOW_MAX_BUFFER_BYTES = 64 * 1024 * 1024;
@@ -77,15 +76,13 @@ function refuse(reason: string): number {
   return 1;
 }
 
-/**
- * The diff this verdict is scoped to, written where fallow reads it. Without it there is nothing to
- * scope to, and a `fallow-crap.diff` left by an earlier run would silently supply one, so a failure
- * here is fatal rather than a scoring of the wrong lines.
- */
-function readScopedDiff(root: string, base: string): readonly TouchedFile[] {
+/** The diff this verdict is scoped to, as fallow receives it on stdin. */
+function readScopedDiff(
+  root: string,
+  base: string,
+): { diff: string; touched: readonly TouchedFile[] } {
   const diff = readChangedLinesDiff(root, base);
-  fs.writeFileSync(path.resolve(root, DIFF_PATH), diff);
-  return readTouchedFiles(parseUnifiedDiff(diff));
+  return { diff, touched: readTouchedFiles(parseUnifiedDiff(diff)) };
 }
 
 /**
@@ -106,6 +103,8 @@ type CrapInputs = Readonly<{
   mapPaths: ReadonlySet<string>;
   /** Every file the run listed in lcov, or null when it wrote nothing usable. */
   enumeratedPaths: ReadonlySet<string> | null;
+  /** The base...HEAD diff fallow scores against, on stdin; empty in `all` scope. */
+  diff: string;
   touched: readonly TouchedFile[];
   /** Records the sanitizer refused, as repo-relative paths. */
   dropped: readonly string[];
@@ -134,10 +133,11 @@ function gatherCrapInputs(
     return { refusal: `${coveragePath}: ${describe(error)}` };
   }
 
+  let diff = '';
   let touched: readonly TouchedFile[] = [];
   if (scope === 'touched') {
     try {
-      touched = readScopedDiff(root, base);
+      ({ diff, touched } = readScopedDiff(root, base));
     } catch (error: unknown) {
       return { refusal: `the diff against ${base} failed: ${describe(error)}` };
     }
@@ -152,13 +152,18 @@ function gatherCrapInputs(
   return {
     mapPaths: coverageMapPaths(map, root),
     enumeratedPaths: readEnumeratedPaths(root),
+    diff,
     touched,
     dropped: dropped.map((file) => repoRelativePath(file, root)),
   };
 }
 
 /** Scores the sanitized map, scoped to this diff in `touched` scope. */
-function runFallowHealth(root: string, scope: CrapScope): { stdout: string } | { failure: string } {
+function runFallowHealth(
+  root: string,
+  scope: CrapScope,
+  diff: string,
+): { stdout: string } | { failure: string } {
   const fallow = runCmdSync(
     process.execPath,
     [
@@ -171,13 +176,18 @@ function runFallowHealth(root: string, scope: CrapScope): { stdout: string } | {
       String(CRAP_THRESHOLD),
       '--coverage',
       SANITIZED_PATH,
-      ...(scope === 'touched' ? ['--diff-file', DIFF_PATH] : []),
+      ...(scope === 'touched' ? ['--diff-stdin'] : []),
       '--report-only',
       '--format',
       'json',
       '--quiet',
     ],
-    { cwd: root, allowFailure: true, maxBuffer: FALLOW_MAX_BUFFER_BYTES },
+    {
+      cwd: root,
+      allowFailure: true,
+      maxBuffer: FALLOW_MAX_BUFFER_BYTES,
+      ...(scope === 'touched' ? { stdin: diff } : {}),
+    },
   );
   return fallow.exitCode === 0
     ? { stdout: fallow.stdout }
@@ -201,7 +211,7 @@ function readExemptions(root: string): CrapExemptions {
 
 /** Scores what was gathered, prints the verdict, and returns the process exit code. */
 function reportCrapVerdict(root: string, scope: CrapScope, inputs: CrapInputs): number {
-  const health = runFallowHealth(root, scope);
+  const health = runFallowHealth(root, scope, inputs.diff);
   if ('failure' in health) {
     process.stderr.write(`check:coverage-crap: ${health.failure}\n`);
     return 1;