diff --git a/changelog.d/stop-focused-test-tag-keeping-run-data.md b/changelog.d/stop-focused-test-tag-keeping-run-data.md new file mode 100644 index 000000000..23f244387 --- /dev/null +++ b/changelog.d/stop-focused-test-tag-keeping-run-data.md @@ -0,0 +1 @@ +- **`/ship` no longer keeps a whole results file in review because a model reply says "fit (".** The gate scan's focused-or-skipped-test check (RH-1) read every non-prose file, so English like "don't fit (the gap never drops below one space)" in a committed `turns.jsonl` tagged the file and kept it in the reviewers' diff even under `review_exclude_paths`. On harness-evals#333 that put the reader's diff back at 5,120,181 bytes against the 2 MB ceiling and `/ship` stopped with `REVIEW_DIFF_TOO_LARGE` (GST-2006). RH-1 now skips only data and logs, which no test runner executes: JSON, JSONL, NDJSON, CSV, TSV, `.log` and `.out` files no longer carry RH-1 unless they were renamed from another kind of file. Every other file keeps RH-1, in a test folder or not, and every other gate tag still reads data files, and code under an excluded path with `fit(`, `fdescribe(`, `it.only(` or `describe.skip(` is still tagged and kept. diff --git a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md index 28a8563d9..4536cb3e6 100644 --- a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md +++ b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md @@ -158,7 +158,7 @@ v1 (see Review record). | id | what it tags | signal | |---|---|---| -| RH-1 | test removed or disabled | removed `test(`/`it(`/`def test_`/`func Test`/`@Test`; added `.skip`, `.only`, `xit`, `xdescribe`, `@pytest.mark.skip`, `#[ignore]`, `t.Skip(`, `@Disabled`, `@Ignore`, `XCTSkip`, `@tag :skip` | +| RH-1 | test removed or disabled | removed `test(`/`it(`/`def test_`/`func Test`/`@Test`; added `.skip`, `.only`, `xit`, `xdescribe`, `@pytest.mark.skip`, `#[ignore]`, `t.Skip(`, `@Disabled`, `@Ignore`, `XCTSkip`, `@tag :skip`; not in data, logs or run output (`.json`, `.jsonl`, `.ndjson`, `.csv`, `.tsv`, `.log`, `.out`) unless renamed from another file, so English there that quotes "fit (" carries no RH-1 | | RH-3 | snapshot changed without its owning test | `__snapshots__/.snap` or `.snap` whose conventional owner (`` test file) has no hunk in the candidate. `*.golden`, `testdata/` and project `Snapshot paths:` are tagged `RH-3?` (owner unresolved: listed and read, never inferred). No general source-ownership inference in v1 | | RH-4 | placeholder committed | added `todo!()`, `unimplemented!()`, `raise NotImplementedError`, `throw new Error("not implemented")`, `pass # TODO` in non-test code | | RH-12 | environment sniffing in product code | added `process.env.CI`, `NODE_ENV === 'test'`, `RAILS_ENV == 'test'`, `PYTEST_CURRENT_TEST`, `GITHUB_ACTIONS` outside test paths | diff --git a/lib/gate-diff/index.ts b/lib/gate-diff/index.ts index b03686eae..6357a526e 100644 --- a/lib/gate-diff/index.ts +++ b/lib/gate-diff/index.ts @@ -62,7 +62,7 @@ function entryFor(file: FileDiff, h: Hunk, floor: PathClass | null, allPaths: Re const path = file.status === 'deleted' ? file.oldPath : file.newPath; const old = h.lines.filter((l) => l.kind === 'del').map((l) => l.text.replace(/\r$/, '')); const added = h.lines.filter((l) => l.kind === 'add').map((l) => l.text.replace(/\r$/, '')); - const tagged = tagHunk({ header: h.header, lines: h.lines.map((l) => ({ kind: l.kind, text: l.text.replace(/\r$/, '') })) }, { floor, path, allPaths }); + const tagged = tagHunk({ header: h.header, lines: h.lines.map((l) => ({ kind: l.kind, text: l.text.replace(/\r$/, '') })) }, { floor, path, oldPath: file.oldPath, allPaths }); const admitted = floor !== null || tagged.tags.length > 0 || hunkHasRemovedTestDeclaration(old); if (!admitted) return null; const bytes = [...old, ...added].reduce((n, l) => n + Buffer.byteLength(l) + 1, 0); diff --git a/lib/gate-diff/tags.ts b/lib/gate-diff/tags.ts index f39e7c7e2..657897377 100644 --- a/lib/gate-diff/tags.ts +++ b/lib/gate-diff/tags.ts @@ -20,12 +20,22 @@ export const COVERAGE_TAGS = ['RH-1', 'RH-3', 'RH-4', 'RH-12', 'RH-13', 'RH-14', export interface TagContext { floor: PathClass | null; path: string; + /** The path before a rename or copy; RH-1 also reads a test renamed to data. */ + oldPath?: string; /** Every changed path in the candidate (new paths), for RH-3 owner lookup. */ allPaths: ReadonlySet; } /** Prose files (docs, prompt templates) carry no code idioms: only the path-based RH-3 applies. */ export const PROSE_FILE = /\.(?:md|mdx|markdown|txt|rst|adoc|asciidoc)(?:\.tmpl)?$/i; +/** + * Data, logs and run output: no test runner executes them, and they quote "fit (" as English, so RH-1's skip, focus and + * removed-declaration idioms skip them. Every other file keeps RH-1, as does data renamed from any other file. + */ +const DATA_FILE = /\.(?:json|jsonl|ndjson|csv|tsv|log|out)$/i; +function testCapable(ctx: TagContext): boolean { + return [ctx.path, ctx.oldPath ?? ctx.path].some((p) => !DATA_FILE.test(p)); +} /** Generated, vendored or minified files: unreadable as hunks, so floor inventory only. */ export const GENERATED_FILE = /(?:^|\/)(?:dist|build|vendor|node_modules|\.yarn)\/|\.min\.(?:js|css)$|(?:^|\/)[^/]*lock(?:\.json|\.yaml|b)?$/i; @@ -235,13 +245,15 @@ export function tagHunk(h: Hunk, ctx: TagContext): TagResult { } else if (ctx.floor === 'golden') tags.add('RH-3?'); if (PROSE_FILE.test(ctx.path) || GENERATED_FILE.test(ctx.path)) return { tags: [...tags], notes, citation: null }; - const removedDecls = dels.filter((l) => TEST_DECLARATION.test(l)).length; - const addedDecls = adds.filter((l) => TEST_DECLARATION.test(l)).length; - if (removedDecls > addedDecls) { - tags.add('RH-1'); - notes.push(`RH-1: ${removedDecls - addedDecls} test declaration(s) removed`); + if (testCapable(ctx)) { + const removedDecls = dels.filter((l) => TEST_DECLARATION.test(l)).length; + const addedDecls = adds.filter((l) => TEST_DECLARATION.test(l)).length; + if (removedDecls > addedDecls) { + tags.add('RH-1'); + notes.push(`RH-1: ${removedDecls - addedDecls} test declaration(s) removed`); + } + if (anyMatch(adds, SKIP_IDIOMS) || (ctx.floor === 'test' && anyMatch(adds, SKIP_IDIOMS_TEST_ONLY))) tags.add('RH-1'); } - if (anyMatch(adds, SKIP_IDIOMS) || (ctx.floor === 'test' && anyMatch(adds, SKIP_IDIOMS_TEST_ONLY))) tags.add('RH-1'); if (nonTest && anyMatch(adds, PLACEHOLDER_IDIOMS)) tags.add('RH-4'); if (nonTest && anyMatch(adds, ENV_SNIFF_IDIOMS)) tags.add('RH-12'); diff --git a/test/gate-diff.test.ts b/test/gate-diff.test.ts index 35c37bff4..ac7a91b35 100644 --- a/test/gate-diff.test.ts +++ b/test/gate-diff.test.ts @@ -330,3 +330,83 @@ describe('gate-diff classify: floor matcher and CLAUDE.md block', () => { expect(snapshotOwnerChanged('t/a.snap', new Set(['t/b.test.ts']))).toBe(false); }); }); + +// Value: protects=a focused or skipped test still tags RH-1 wherever a test runner could execute it (JS and TS test and +// source files, extensionless scripts, a project's own test globs, a data file renamed to code, a test renamed to data), while +// English "fit (" in counted run data no longer tags RH-1 and keeps a whole results file in review (harness-evals#333, +// GST-2006); fails_when=RH-1 drops on any code path above, RH-1 still tags a .jsonl, .json, .ndjson, .csv or .log reply, or +// another tag stops firing on a data file; why_new=RH-1 read every non-prose file; seam=scanCandidate over parsed git +// diffs; no_claim=languages whose skip idioms the table does not list +describe('RH-1 reads only files a test runner can execute (GST-2006)', () => { + const added = (p: string, lines: string[]) => + `diff --git a/${p} b/${p}\nnew file mode 100644\n--- /dev/null\n+++ b/${p}\n@@ -0,0 +1,${lines.length} @@\n${lines.map((l) => '+' + l).join('\n')}\n`; + const renamed = (from: string, to: string, ctx: string[], dels: string[], adds: string[]) => { + const old = ctx.length + dels.length; + const now = ctx.length + adds.length; + return `diff --git a/${from} b/${to}\nsimilarity index 70%\nrename from ${from}\nrename to ${to}\n--- a/${from}\n+++ b/${to}\n@@ -1,${old} +1,${now} @@\n${[...ctx.map((l) => ' ' + l), ...dels.map((l) => '-' + l), ...adds.map((l) => '+' + l)].join('\n')}\n`; + }; + const run = (diff: string, project = EMPTY_PROJECT_CONFIG) => + scanCandidate({ files: parseUnifiedDiff(diff), commits: [], project, specIsTestPath: false, candidate: 'c', artifact: '/tmp/a' }); + const tagsAt = (r: ScanResult) => Object.fromEntries(r.hunks.map((h) => [h.path, h.tags])); + const MARKERS = ["fit('adds', () => {});", "fdescribe('math', () => {});", "it.only('adds', () => {});", "describe.skip('math', () => {});"]; + const SCRIPT_EXTS = ['ts', 'tsx', 'js', 'mjs', 'cjs']; + /** The two model replies from harness-evals#333's main/turns.jsonl that tagged RH-1 at a229e258. */ + const REPLIES = [ + JSON.stringify({ turn: 14, role: 'assistant', text: "The columns don't fit (the gap never drops below one space), so I widened the table." }), + JSON.stringify({ turn: 15, role: 'assistant', text: 'Values past the limit should *not* fit (e.g. a 41-character name in a 40-character field).' }), + ]; + + test('every focus and skip marker still tags RH-1 in .ts, .tsx, .js, .mjs and .cjs test files and in non-test source', () => { + for (const ext of SCRIPT_EXTS) { + for (const marker of MARKERS) { + for (const p of [`test/math.test.${ext}`, `src/math.spec.${ext}`, `lib/math-helpers.${ext}`]) { + expect([p, marker, tagsAt(run(added(p, ["import { add } from './add';", marker])))[p]]).toEqual([p, marker, ['RH-1']]); + } + } + } + }); + + test('code under a results path, extensionless scripts and a project test glob still tag RH-1', () => { + for (const p of ['docs/benchmarks/2026-11-06-qm-s1-edition/main/score.ts', 'docs/benchmarks/2026-11-06-qm-s1-edition/main/trials/check.mjs', 'bin/run-suite']) { + expect([p, tagsAt(run(added(p, ["fit('scores', () => {});"])))[p]]).toEqual([p, ['RH-1']]); + } + const project = parseGateIntegrityBlock('## Gate Integrity\nExtra test paths: checks/**\n'); + expect(tagsAt(run(added('checks/login.check', ["describe.skip('login', () => {});"]), project))['checks/login.check']).toEqual(['RH-1']); + }); + + test('a .jsonl renamed to .ts and a test renamed to .jsonl still tag RH-1', () => { + const dir = 'docs/benchmarks/2026-11-06-qm-s1-edition/main'; + const toCode = run(renamed(`${dir}/turns.jsonl`, `${dir}/turns.ts`, ['{"turn":1}', '{"turn":2}', '{"turn":3}'], [], ["fit('turns', () => {});"])); + expect(tagsAt(toCode)).toEqual({ [`${dir}/turns.ts`]: ['RH-1'] }); + const toData = run(renamed('test/math.test.ts', `${dir}/math.jsonl`, ["import { add } from './add';", '{"turn":1}', '{"turn":2}'], ["test('adds', () => {", ' expect(add(1, 2)).toBe(3);', '});'], [])); + expect(tagsAt(toData)).toEqual({ [`${dir}/math.jsonl`]: ['RH-1'] }); + }); + + test('#333\'s English "fit (" in run data tags nothing; data and logs carry no RH-1, in a test folder too', () => { + const turns = 'docs/benchmarks/2026-11-06-qm-s1-edition/main/turns.jsonl'; + const r = run(added(turns, REPLIES)); + expect(r.hunks).toEqual([]); + expect(r.unmatchedPaths).toEqual([turns]); + for (const p of ['results/replies.json', 'results/replies.ndjson', 'results/replies.csv', 'results/run.log', 'results/run.out']) { + expect([p, run(added(p, ["it.only (a fit (b), fdescribe (c)", "test.skip('x')"])).hunks]).toEqual([p, []]); + } + const fixture = run(added('test/fixtures/replies.jsonl', REPLIES)); + expect(fixture.hunks.map((h) => [h.path, h.floor, h.tags])).toEqual([['test/fixtures/replies.jsonl', 'test', []]]); + }); + + test('every other tag still fires on a data file', () => { + const lines = ['{"cmd": "git push --no-verify"}', '{"code": "// eslint-disable-next-line"}']; + expect(tagsAt(run(added('results/replies.jsonl', lines)))['results/replies.jsonl']).toEqual(['RH-13', 'RH-14']); + }); + + test('an HTML browser suite, an unlisted language and a test-floor file renamed to data still tag RH-1', () => { + const html = 'pages/browser.html'; + expect(tagsAt(run(added(html, [''])))[html]).toEqual(['RH-1']); + const crystal = 'src/math_check.cr'; + expect(tagsAt(run(added(crystal, ['it.only("adds") { add(1, 2).should eq 3 }'])))[crystal]).toEqual(['RH-1']); + const project = parseGateIntegrityBlock('## Gate Integrity\nExtra test paths: checks/**\n'); + const decls = ["test('logs in', () => {", ' expect(1).toBe(1);', '});']; + expect(tagsAt(run(renamed('checks/login.check', 'results/login.jsonl', ['{"turn":1}', '{"turn":2}'], decls, []), project))).toEqual({ 'results/login.jsonl': ['RH-1'] }); + expect(tagsAt(run(renamed('test/api.tavern.yaml', 'results/api.json', ['{"turn":1}', '{"turn":2}'], decls, [])))).toEqual({ 'results/api.json': ['RH-1'] }); + }); +}); diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index 22e55abeb..959aab08f 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -1113,3 +1113,50 @@ describe('a branch cannot hide code under a base pattern', () => { }, 180_000); }); + +describe('the gate scan keeps code but not run data under a pattern (GST-2006)', () => { + // Value: protects=a results PR whose model replies say "fit (" excludes its run data and stays under the reader ceiling, while + // a focused or skipped test in code under the same pattern (a test file, a non-test module, a .jsonl renamed to .ts) is + // still gate-tagged and kept; fails_when=RH-1 keeps main/turns.jsonl as gate-tagged (harness-evals#333 at a229e258: + // reader diff 5,120,181 B, REVIEW_DIFF_TOO_LARGE) or a code file under the pattern loses its RH-1 keep; why_new=every other + // exclusion test passes --gate-tagged 0 or plants no idiom in data; seam=gstack-review-plan and gstack-review-dispatch pass + // running the real gate scan; no_claim=what reviewers do with the kept files + test('English "fit (" in turns.jsonl leaves the file excluded; code with focus and skip markers stays gate-tagged', () => { + const dir = 'docs/benchmarks/2026-11-06-qm-s1-edition/main'; + const legacy = Array.from({ length: 12 }, (_, n) => JSON.stringify({ turn: n, role: 'user', text: `question ${n}` })).join('\n') + '\n'; + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-rh1-', baseFiles: { '.gstack/ship-policy.json': policyText(['docs/benchmarks/**/main/**']), [`${dir}/legacy.jsonl`]: legacy } }); + repos.push(repo); + const replies = [ + "The columns don't fit (the gap never drops below one space), so I widened the table.", + 'Values past the limit should *not* fit (e.g. a 41-character name in a 40-character field).', + ]; + const filler = 'x'.repeat(1_000); + const turns = Array.from({ length: 2_600 }, (_, n) => JSON.stringify({ turn: n, role: 'assistant', text: `${replies[n % 2]} ${filler}` })).join('\n') + '\n'; + expect(Buffer.byteLength(turns)).toBeGreaterThan(2_500_000); + repo.git('mv', `${dir}/legacy.jsonl`, `${dir}/legacy.ts`); + commit(repo, { + [`${dir}/turns.jsonl`]: turns, + [`${dir}/trials/t1.json`]: '{"pass": true}\n', + [`${dir}/legacy.ts`]: legacy + "it.only('replays turn 3', () => {});\n", + [`${dir}/score.test.ts`]: "fit('scores turn 3', () => {});\n", + [`${dir}/report.mjs`]: "describe.skip('report', () => {});\n", + }); + const p = JSON.parse(runBin(repo, 'gstack-review-plan', ['--base', 'main', '--json']).out); + expect(p.review_exclude.kept_files).toEqual([ + { path: `${dir}/legacy.ts`, reason: 'gate-tagged' }, + { path: `${dir}/report.mjs`, reason: 'gate-tagged' }, + { path: `${dir}/score.test.ts`, reason: 'gate-tagged' }, + ]); + expect(p.review_exclude).toMatchObject({ state: 'active', excluded: 3, content: { binary: 0, data: 3, docs: 0, log: 0 } }); + const out = path.join(repo.root, 'pass'); + const pass = runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', out, '--pass', 'p1']); + expect({ status: pass.status, err: pass.err.split('\n').filter(l => l.includes('REVIEW_DIFF_TOO_LARGE')) }).toEqual({ status: EXIT.ok, err: [] }); + const reader = JSON.parse(fs.readFileSync(path.join(out, 'pass-manifest.json'), 'utf8')).readers[0]; + const mb = mergeBase(repo); + const read = spawnSync('bash', ['-c', `git diff "$DIFF_BASE" -- . ${reader.policy_exclude} && git diff "$DIFF_BASE" -- ${reader.keep.map((k: string) => `'${k}'`).join(' ')}`], { cwd: repo.work, encoding: 'utf8', timeout: 30_000, maxBuffer: 64 * 1024 * 1024, env: { ...process.env, ...repo.env, DIFF_BASE: mb } }); + expect(read.status).toBe(0); + expect(read.stdout).not.toContain(`b/${dir}/turns.jsonl`); + expect(read.stdout).toContain(`b/${dir}/score.test.ts`); + expect(Buffer.byteLength(read.stdout)).toBeLessThan(2_000_000); + }, 180_000); +});