Skip to content

Commit 833e56a

Browse files
Huyvux12Huyvux12
committed
fix(coverage-changed): address maintainer feedback on the stamp gate
Refusal now fires before the waiver when the report has no stamp (run-level test pins it). The excluded-path tally is restored for stamp-excluded files an include root covers, so new logic under an exclude entry still shows in the job summary. Both gates share one outside-root scan via a predicate in stamp.ts, with no schema change. The COVERAGE_CONFIG fixture is shared between the model and run suites, and the CONTRIBUTING wording now names both the no-verdict and no-report messages.
1 parent 253a1d6 commit 833e56a

8 files changed

Lines changed: 140 additions & 66 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -300,12 +300,14 @@ guess from standing in for a measurement of code you changed.
300300

301301
The changed-line gate (`check:coverage-changed`) reads the same stamp: with no stamp, a stamp from
302302
another commit or working tree, a narrowed run, or an lcov report whose records name files outside this
303-
checkout, it prints `no trustworthy verdict` and names the reason instead of a PASS/FAIL table. The fix
304-
is the same — re-run `pnpm test:coverage` on this commit — and the refusal fires before the
305-
`coverage-waiver` label is consulted, because a waiver skips a failure rather than blessing a report
306-
with no numbers to print. Which files owe coverage comes from the stamp's include/exclude, so a file the
307-
config excludes is out of scope by declaration, while a file the stamp instruments that lcov omits is
308-
still tallied as excluded-path lines.
303+
checkout, it prints `no trustworthy verdict` and names the reason instead of a PASS/FAIL table — once
304+
there is an lcov report to read; with no report at all it prints `no lcov report` and asks for a
305+
coverage run first. The fix is the same — re-run `pnpm test:coverage` on this commit — and the refusal
306+
fires before the `coverage-waiver` label is consulted, because a waiver skips a failure rather than
307+
blessing a report with no numbers to print. Which files owe coverage comes from the stamp's
308+
include/exclude: a file the config excludes is out of scope by declaration, but when an include root
309+
covers it its added lines are still tallied as excluded-path lines, as is a file the stamp instruments
310+
that lcov omits.
309311

310312
## Test app and Maestro compatibility
311313

‎scripts/coverage-changed/fixtures.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import type { CoverageConfig } from '../coverage-crap/stamp.ts';
2+
3+
/**
4+
* The stamp's include/exclude for these suites, mirroring the `test.coverage`
5+
* include/exclude in `vitest.config.ts` for the paths the tests exercise:
6+
* both coverage roots, test files out, `src/sdk/**` out. One copy, shared by
7+
* the model and run suites, so a config change cannot leave them disagreeing.
8+
*/
9+
export const COVERAGE_CONFIG: CoverageConfig = {
10+
include: ['src/**/*.ts', 'packages/*/src/**/*.ts'],
11+
exclude: [
12+
'**/*.test.ts',
13+
'**/__tests__/**',
14+
'**/*-types.ts',
15+
'**/types.ts',
16+
'src/sdk/**',
17+
'src/bin.ts',
18+
],
19+
};

‎scripts/coverage-changed/model.test.ts‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ import {
77
ignoredLineSet,
88
parseLcov,
99
} from './model.ts';
10-
import { isInstrumentedPath, type CoverageConfig } from '../coverage-crap/stamp.ts';
10+
import { isInstrumentedPath } from '../coverage-crap/stamp.ts';
11+
import { COVERAGE_CONFIG } from './fixtures.ts';
1112

1213
test('parseLcov reads DA hits and BRDA taken, treating "-" as not taken', () => {
1314
const lcov = [
@@ -33,21 +34,6 @@ test('parseLcov reads DA hits and BRDA taken, treating "-" as not taken', () =>
3334
assert.equal(index.get('src/b.ts')?.lineHits.get(10), 1);
3435
});
3536

36-
// The stamp's include/exclude, mirroring `test.coverage` in `vitest.config.ts`
37-
// for the paths these tests exercise: both coverage roots, test files and the
38-
// config's named exclusions out.
39-
const COVERAGE_CONFIG: CoverageConfig = {
40-
include: ['src/**/*.ts', 'packages/*/src/**/*.ts'],
41-
exclude: [
42-
'**/*.test.ts',
43-
'**/__tests__/**',
44-
'**/*-types.ts',
45-
'**/types.ts',
46-
'src/sdk/**',
47-
'src/bin.ts',
48-
],
49-
};
50-
5137
test('source/test classification matches the verified stamp selection', () => {
5238
assert.equal(isInstrumentedPath('src/core/x.ts', COVERAGE_CONFIG), true);
5339
assert.equal(isInstrumentedPath('src/core/x.test.ts', COVERAGE_CONFIG), false);
@@ -222,14 +208,28 @@ test('instrumented source absent from lcov counts as excluded-path code lines',
222208
assert.deepEqual(result.excluded.files[0]?.lines, [1]);
223209
});
224210

225-
test('a path the stamp excludes is out of scope, not tallied', () => {
211+
test('a path the stamp excludes is tallied as excluded-path, not gated', () => {
226212
const result = computeChangedCoverage({
227213
diffs: [diff('src/sdk/thing.ts', [1, 2, 3])],
228214
coverage,
229215
stamp: COVERAGE_CONFIG,
230216
fileLines: () => ['const kept = 1;', '', '// a comment'],
231217
});
232218
assert.equal(result.totalLines, 0);
219+
assert.equal(result.excluded.totalLines, 1);
220+
assert.equal(result.excluded.files[0]?.reason, 'excluded-path');
221+
assert.deepEqual(result.excluded.files[0]?.lines, [1]);
222+
assert.equal(result.passed, true);
223+
});
224+
225+
test('a stamp-excluded test file or a path outside every include root is not tallied', () => {
226+
const result = computeChangedCoverage({
227+
diffs: [diff('src/sdk/thing.test.ts', [1, 2, 3]), diff('README.md', [1, 2])],
228+
coverage,
229+
stamp: COVERAGE_CONFIG,
230+
fileLines: () => ['const kept = 1;', '', '// a comment'],
231+
});
232+
assert.equal(result.totalLines, 0);
233233
assert.equal(result.excluded.totalLines, 0);
234234
assert.deepEqual(result.excluded.files, []);
235235
assert.equal(result.passed, true);

‎scripts/coverage-changed/model.ts‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@
99
// gate reads from the same stamp. A changed instrumented file ABSENT from
1010
// lcov was dropped by the run — reported (non-gating) so the gap cannot
1111
// silently absorb new logic. A changed file the stamp excludes is out of scope
12-
// by declaration and owes nothing, not even a tally entry.
12+
// by declaration and owes no gating line — but when an include root covers it,
13+
// its code-like added lines still show in the non-gating excluded tally, so
14+
// new logic under an exclude entry cannot hide from the job summary either.
1315

1416
import type { ChangedFileDiff } from '../lib/repo-git.ts';
1517
import { normalizePath } from '../lib/repo-path.ts';
16-
import { isInstrumentedPath, type CoverageConfig } from '../coverage-crap/stamp.ts';
18+
import { isIncludedPath, isInstrumentedPath, type CoverageConfig } from '../coverage-crap/stamp.ts';
1719

1820
// Single source of truth for the gate. Changed-line coverage below this
1921
// percentage fails the Coverage CI job (unless waived).
@@ -205,30 +207,49 @@ function sum<T>(items: readonly T[], pick: (item: T) => number): number {
205207
return items.reduce((acc, item) => acc + pick(item), 0);
206208
}
207209

210+
// Test code never counts toward the gate or the excluded tally, which exists
211+
// to surface hidden product logic, not test code.
212+
function isTestFile(path: string): boolean {
213+
return /\.test\.ts$/.test(path) || /(^|\/)__tests__\//.test(path);
214+
}
215+
216+
// Code-like added lines of a file the run did not measure, as the non-gating
217+
// excluded tally entry: a gap the summary must show, never a denominator line.
218+
function excludedPathReport(
219+
diff: ChangedFileDiff,
220+
lines: readonly string[] | null,
221+
): ExcludedFileReport | null {
222+
const excluded = diff.added.filter((n) => isCodeLike(lines?.[n - 1]));
223+
return excluded.length > 0 ? { path: diff.path, reason: 'excluded-path', lines: excluded } : null;
224+
}
225+
208226
export function computeChangedCoverage(input: ComputeInput): ChangedCoverageResult {
209227
const files: FileCoverageReport[] = [];
210228
const excludedFiles: ExcludedFileReport[] = [];
211229

212230
for (const diff of input.diffs) {
213231
// A non-blob side (symlink, gitlink) is not source: its working-tree entry can
214232
// resolve to a directory, which no content read may attempt.
215-
if (
216-
diff.deleted ||
217-
!diff.regularFile ||
218-
diff.added.length === 0 ||
219-
!isInstrumentedPath(diff.path, input.stamp)
220-
)
233+
if (diff.deleted || !diff.regularFile || diff.added.length === 0) continue;
234+
235+
if (!isInstrumentedPath(diff.path, input.stamp)) {
236+
// Out of scope by declaration — but a file an include root covers still
237+
// shows its added lines in the non-gating excluded tally, the way the
238+
// gate did before the stamp owned selection. A test file or a path
239+
// outside every include root owes not even that.
240+
if (isIncludedPath(diff.path, input.stamp) && !isTestFile(diff.path)) {
241+
const report = excludedPathReport(diff, input.fileLines(diff.path));
242+
if (report) excludedFiles.push(report);
243+
}
221244
continue;
245+
}
222246

223247
const cov = input.coverage.get(diff.path);
224248
if (!cov) {
225249
// Instrumented source absent from the lcov report: the run recorded no
226250
// counters for it. Count its code-like added lines so the gap is visible.
227-
const lines = input.fileLines(diff.path);
228-
const excluded = diff.added.filter((n) => isCodeLike(lines?.[n - 1]));
229-
if (excluded.length > 0) {
230-
excludedFiles.push({ path: diff.path, reason: 'excluded-path', lines: excluded });
231-
}
251+
const report = excludedPathReport(diff, input.fileLines(diff.path));
252+
if (report) excludedFiles.push(report);
232253
continue;
233254
}
234255

‎scripts/coverage-changed/run.test.ts‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,8 @@ import os from 'node:os';
55
import path from 'node:path';
66
import { afterEach, beforeEach, test } from 'node:test';
77
import { run } from './run.ts';
8-
import {
9-
beginCoverageRun,
10-
finishCoverageRun,
11-
type CoverageConfig,
12-
type RunSelection,
13-
} from '../coverage-crap/stamp.ts';
8+
import { beginCoverageRun, finishCoverageRun, type RunSelection } from '../coverage-crap/stamp.ts';
9+
import { COVERAGE_CONFIG } from './fixtures.ts';
1410
import { beginStderrCapture, beginStdoutCapture } from '@agent-device/test-kit';
1511

1612
let repo: string;
@@ -29,23 +25,6 @@ function writeLcov(records: string): void {
2925
write('coverage/lcov.info', records);
3026
}
3127

32-
/**
33-
* What the fixture's coverage pass was told to instrument. Mirrors the
34-
* `test.coverage` include/exclude in `vitest.config.ts` for the paths these
35-
* tests exercise: both coverage roots, test files out, `src/sdk/**` out.
36-
*/
37-
const COVERAGE_CONFIG: CoverageConfig = {
38-
include: ['src/**/*.ts', 'packages/*/src/**/*.ts'],
39-
exclude: [
40-
'**/*.test.ts',
41-
'**/__tests__/**',
42-
'**/*-types.ts',
43-
'**/types.ts',
44-
'src/sdk/**',
45-
'src/bin.ts',
46-
],
47-
};
48-
4928
/** What `pnpm test:coverage` leaves beside its reports: the stamp naming the run that wrote them. */
5029
function writeStamp(runSelection: RunSelection = 'full'): void {
5130
const outcome = finishCoverageRun(
@@ -252,6 +231,27 @@ test('refuses a report with no stamp beside it instead of scoring it', () => {
252231
assert.doesNotMatch(out, /Changed-line coverage gate:/);
253232
});
254233

234+
test('refusal fires before the waiver when the report has no stamp', () => {
235+
write('src/feature.ts', 'export const covered = 1;\nexport const uncovered = 2;\n');
236+
git('add', '-A');
237+
git('commit', '-q', '-m', 'feature');
238+
writeLcov('SF:src/feature.ts\nDA:1,3\nDA:2,0\nend_of_record\n');
239+
// No stamp beside the report: the numbers are unattributable, so the waiver
240+
// has nothing to waive — it skips a failure, it does not bless a report.
241+
const previous = process.env.AGENT_DEVICE_COVERAGE_WAIVER;
242+
process.env.AGENT_DEVICE_COVERAGE_WAIVER = 'true';
243+
try {
244+
const { code, out } = capture(() => run(['--base', 'main'], repo));
245+
assert.equal(code, 1);
246+
assert.match(out, /no trustworthy verdict/);
247+
assert.match(out, /carries no coverage-run-stamp\.json/);
248+
assert.doesNotMatch(out, /Changed-line coverage gate:/);
249+
} finally {
250+
if (previous === undefined) delete process.env.AGENT_DEVICE_COVERAGE_WAIVER;
251+
else process.env.AGENT_DEVICE_COVERAGE_WAIVER = previous;
252+
}
253+
});
254+
255255
test('refuses a report whose stamp names an earlier commit', () => {
256256
write('src/feature.ts', 'export const covered = 1;\nexport const uncovered = 2;\n');
257257
git('add', '-A');
@@ -305,10 +305,12 @@ test('refuses a report written in another checkout instead of passing it as clea
305305
git('commit', '-q', '-m', 'feature');
306306
// Two checkouts of one commit share HEAD and bytes, so no stamp can tell them apart: the lcov
307307
// paths themselves must name the foreign root. A map like this joins none of this checkout's
308-
// files, which without the check reads as "no file was measured" and passes 0/0.
308+
// files, which without the check reads as "no file was measured" and passes 0/0. The stamp here
309+
// is valid for this tree, so the foreign paths are the only defect the run can see.
309310
const foreign = fs.mkdtempSync(path.join(os.tmpdir(), 'cov-changed-foreign-'));
310311
try {
311312
writeLcov(`SF:${foreign}/src/feature.ts\nDA:1,3\nDA:2,0\nend_of_record\n`);
313+
writeStamp();
312314
const { code, out } = capture(() => run(['--base', 'main'], repo));
313315
assert.equal(code, 1);
314316
assert.match(out, /no trustworthy verdict/);
@@ -319,17 +321,19 @@ test('refuses a report written in another checkout instead of passing it as clea
319321
}
320322
});
321323

322-
test('a file the stamp excludes is out of scope, not tallied as excluded', () => {
324+
test('a file the stamp excludes is tallied as excluded, not gated', () => {
323325
write('src/sdk/thing.ts', 'export const kept = 1;\n');
324326
git('add', '-A');
325327
git('commit', '-q', '-m', 'sdk change');
326328
// No record for it in lcov: the run never measured it, by declaration. The
327-
// gate owes nothing for it — neither a denominator line nor a tally entry.
329+
// gate owes nothing for it — but the added line still shows in the
330+
// non-gating excluded tally, so new logic under an exclude entry is visible.
328331
writeLcov('SF:src/base.ts\nDA:1,1\nend_of_record\n');
329332
writeStamp();
330333
const { code, out } = capture(() => run(['--base', 'main'], repo));
331334
assert.equal(code, 0);
332335
assert.match(out, /Changed-line coverage gate: PASS/);
333336
assert.match(out, /0\/0 \(n\/a\)/);
334-
assert.match(out, /Changed executable lines excluded \(non-gating\) \| 0 \|/);
337+
assert.match(out, /Changed executable lines excluded \(non-gating\) \| 1 \|/);
338+
assert.match(out, /excluded `src\/sdk\/thing\.ts` \[excluded-path\]: 1/);
335339
});

‎scripts/coverage-changed/run.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ import { runCmdSync } from '@agent-device/host-kit/command';
2727
import { parseScriptArgs } from '../lib/cli-args.ts';
2828
import { parseUnifiedDiff, readChangedLinesDiff } from '../lib/repo-git.ts';
2929
import { computeChangedCoverage, parseLcov, type ChangedCoverageResult } from './model.ts';
30-
import { describeStampMismatch, verifyCoverageStamp } from '../coverage-crap/stamp.ts';
30+
import {
31+
describeStampMismatch,
32+
findOutsideRootPath,
33+
verifyCoverageStamp,
34+
} from '../coverage-crap/stamp.ts';
3135

3236
const USAGE = 'Usage: pnpm check:coverage-changed [--base <ref>]\n';
3337
const COVERAGE_DIR = 'coverage';
@@ -86,7 +90,7 @@ export function run(argv: readonly string[], cwd?: string): number {
8690
// outside this one and can join none of its files. That must not read as "no
8791
// file was measured", and the stamp cannot catch it: two checkouts of one
8892
// commit carry the same HEAD and the same bytes.
89-
const outside = [...coverage.keys()].find((file) => file.startsWith('../'));
93+
const outside = findOutsideRootPath(coverage.keys());
9094
if (outside !== undefined) {
9195
return refuse(
9296
`${lcovPath} records files outside this checkout, for example ${outside}. ` +

‎scripts/coverage-crap/run.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import {
4040
import {
4141
declaresWholeFileIgnore,
4242
describeStampMismatch,
43+
findOutsideRootPath,
4344
isInstrumentedPath,
4445
verifyCoverageStamp,
4546
type CoverageConfig,
@@ -161,7 +162,7 @@ function gatherCrapInputs(
161162
// Vitest writes the map's keys absolute, so a report from another root relativizes outside this one
162163
// and can join none of its files. That must not read as "no file was measured", and the stamp cannot
163164
// catch it: two checkouts of one commit carry the same HEAD and the same bytes.
164-
const outside = [...mapPaths].find((file) => file.startsWith('../'));
165+
const outside = findOutsideRootPath(mapPaths);
165166
if (outside !== undefined) {
166167
return {
167168
refusal:

‎scripts/coverage-crap/stamp.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,29 @@ export function isInstrumentedPath(relativePath: string, config: CoverageConfig)
181181
);
182182
}
183183

184+
/**
185+
* Whether the resolved config's include covers `relativePath`, whatever the
186+
* exclude says: the run was told to measure it, even when an exclude entry
187+
* then took it back out.
188+
*/
189+
export function isIncludedPath(relativePath: string, config: CoverageConfig): boolean {
190+
const candidate = withHiddenVisible(relativePath.split('\\').join('/'));
191+
return config.include.some((pattern) => path.matchesGlob(candidate, withHiddenVisible(pattern)));
192+
}
193+
194+
/**
195+
* The first path in `paths` that names a file outside this checkout, if any.
196+
* A report records its paths absolute, so one written in another root
197+
* relativizes outside this one — and the stamp cannot catch it, because two
198+
* checkouts of one commit carry the same HEAD and the same bytes.
199+
*/
200+
export function findOutsideRootPath(paths: Iterable<string>): string | undefined {
201+
for (const file of paths) {
202+
if (file.startsWith('../')) return file;
203+
}
204+
return undefined;
205+
}
206+
184207
// A whole-file ignore comment drops a source from every Istanbul map, so its missing record is not a gap
185208
// and no verdict may send the reader to re-run for it. The keyword set and its position are the
186209
// provider's own: ast-v8-to-istanbul reads `istanbul`, `c8`, `v8`, and `node:coverage` as one keyword,

0 commit comments

Comments
 (0)