Skip to content

Commit e4b6147

Browse files
fix(scanner): comment-anchor TODO grep + skip test-file markers (Codex #2) (#15)
The TODO/FIXME/HACK/DOMAIN_QUESTION scan matched any occurrence of the marker words anywhere on a line, so string literals, regexes, and the scanner's own pattern produced phantom improvement tasks. It also surfaced markers living in test fixtures as if they were real work. - Anchor the grep pattern to a line-start comment opener (`^[ \t]*(//|\*|#)[ \t]*(TODO|FIXME|HACK|DOMAIN_QUESTION)[: ]`), so only genuine comment markers match — not bare string/regex occurrences. - Post-filter results through a new exported `isTestFile()` helper to drop markers that live in `__tests__/` dirs or `*.test`/`*.spec` files. - `GREP_EXCLUDE_DIRS` and `scanDeadCode` left unchanged on purpose: dead-code usage search still needs test files, so excluding them there would reintroduce false dead-code positives. Adds regression tests: test-file markers ignored, the grep pattern stays comment-anchored, and `isTestFile` classification. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent e5957a1 commit e4b6147

3 files changed

Lines changed: 71 additions & 1 deletion

File tree

packages/asil-improvement-loop/src/__tests__/scanner.test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, it } from 'vitest';
22
import {
33
normalizePath,
4+
isTestFile,
45
scanCodebase,
56
scanTestFailures,
67
scanTypeErrors,
@@ -172,6 +173,49 @@ describe('scanner', () => {
172173
const foo = tasks.find((t) => t.filePaths[0]?.includes('foo.ts'));
173174
expect(foo?.severity).toBe('medium');
174175
});
176+
177+
it('skips markers found in test files — they are fixtures, not tasks (precision fix)', async () => {
178+
const grepOut = [
179+
'./packages/foo/src/__tests__/foo.test.ts:1:// DOMAIN_QUESTION: fixture data',
180+
'./packages/foo/src/scanner.spec.ts:2:// TODO: spec fixture',
181+
'./packages/foo/src/real.ts:9:// TODO: a genuine task',
182+
].join('\n');
183+
const runner = mockRunner([{ match: () => true, stdout: grepOut }]);
184+
const tasks = await scanTodos('/repo', { runner, fs: mockFileReader() });
185+
// Only the non-test file yields a task.
186+
expect(tasks.length).toBe(1);
187+
expect(tasks[0]?.filePaths[0]).toBe('packages/foo/src/real.ts');
188+
});
189+
190+
it('greps with a comment-anchored marker pattern (no bare string-literal matches)', async () => {
191+
const seen: string[][] = [];
192+
const runner = mockRunner([
193+
{
194+
match: (cmd, args) => {
195+
if (cmd === 'grep') seen.push(args);
196+
return cmd === 'grep';
197+
},
198+
stdout: '',
199+
},
200+
]);
201+
await scanTodos('/repo', { runner, fs: mockFileReader() });
202+
const pattern = seen[0]?.find((a) => a.includes('DOMAIN_QUESTION')) ?? '';
203+
// The pattern must require a line-start comment opener before the
204+
// marker, so `'DOMAIN_QUESTION'` / `/DOMAIN_QUESTION:/` don't match.
205+
expect(pattern).toContain('(//|\\*|#)');
206+
expect(pattern.startsWith('^')).toBe(true);
207+
});
208+
});
209+
210+
describe('isTestFile', () => {
211+
it('matches __tests__ dirs and .test/.spec files; not plain source', () => {
212+
expect(isTestFile('packages/x/src/__tests__/a.ts')).toBe(true);
213+
expect(isTestFile('src/a.test.ts')).toBe(true);
214+
expect(isTestFile('src/a.spec.tsx')).toBe(true);
215+
expect(isTestFile('a.test.py')).toBe(false); // py uses test_ prefix, not .test.
216+
expect(isTestFile('src/a.ts')).toBe(false);
217+
expect(isTestFile('src/scanner.ts')).toBe(false);
218+
});
175219
});
176220

177221
describe('scanCoverageGaps', () => {

packages/asil-improvement-loop/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export {
2121
scanCoverageGaps,
2222
scanDeadCode,
2323
normalizePath,
24+
isTestFile,
2425
stableTaskId,
2526
toRepoRelative,
2627
type ScanResult,

packages/asil-improvement-loop/src/scanner.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,15 @@ export async function scanTodos(
157157
...GREP_EXCLUDE_DIRS,
158158
...includes,
159159
'-E',
160-
'(TODO|FIXME|HACK|DOMAIN_QUESTION)[: ]',
160+
// Require the marker to be the FIRST token of a line-start comment
161+
// (`//`, `#`, or jsdoc `*`). This is ASIL's own DOMAIN_QUESTION
162+
// convention and it eliminates the false positives a live grind
163+
// surfaced: marker strings inside string literals / regexes
164+
// (e.g. `'DOMAIN_QUESTION'`, `/DOMAIN_QUESTION:/`) and mid-comment
165+
// mentions are no longer matched — only genuine actionable markers.
166+
// Trade-off: trailing comments (`code; // TODO`) aren't matched,
167+
// consistent with how DOMAIN_QUESTION markers are already detected.
168+
'^[ \\t]*(//|\\*|#)[ \\t]*(TODO|FIXME|HACK|DOMAIN_QUESTION)[: ]',
161169
'.',
162170
],
163171
{ cwd: repoRoot },
@@ -173,6 +181,10 @@ export async function scanTodos(
173181
const [, file, ln, txt] = m;
174182
if (!file) continue;
175183
const key = normalizePath(file);
184+
// Skip test files — a TODO/FIXME/DOMAIN_QUESTION marker in a test is
185+
// almost always test DATA (fixtures exercising the scanner itself),
186+
// not an actionable task. (Live-grind precision finding.)
187+
if (isTestFile(key)) continue;
176188
const arr = byFile.get(key) ?? [];
177189
arr.push(`:${ln}${txt?.trim() ?? ''}`);
178190
byFile.set(key, arr);
@@ -341,6 +353,19 @@ function shortPath(p: string): string {
341353
return p.replace(/^.*\//, '');
342354
}
343355

356+
/**
357+
* True for files that are tests: anything under a `__tests__/` directory
358+
* or named `*.test.*` / `*.spec.*`. Used to keep TODO/FIXME markers that
359+
* are test FIXTURES (data exercising the scanner) out of the task queue.
360+
*/
361+
export function isTestFile(p: string): boolean {
362+
return (
363+
p.includes('/__tests__/') ||
364+
p.startsWith('__tests__/') ||
365+
/\.(test|spec)\.[cm]?[jt]sx?$/.test(p)
366+
);
367+
}
368+
344369
/**
345370
* Convert an absolute path under `repoRoot` to a repo-relative path.
346371
* Coverage tools (vitest's istanbul reporter, coverage.py) emit

0 commit comments

Comments
 (0)