mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-09 18:28:15 +01:00
Stop the focused-test tag from keeping run data in review (GST-2006) (#4195)
* Stop the focused-test tag from keeping run data in review (GST-2006)
RH-1's skip, focus and removed-declaration checks now read only files a test runner can execute: source files by extension, extensionless scripts, test-folder or project-glob files that are not data, and files renamed from one of those. English like "don't fit (" in a results .jsonl no longer tags the file and keeps it in the reviewers' diff under review_exclude_paths (harness-evals#333).
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* Keep RH-1 on every file except data and logs
Review found that the code-extension allowlist dropped RH-1 from an HTML browser suite, unlisted languages, and a test-floor file renamed to data. RH-1 now skips only .json, .jsonl, .ndjson, .csv, .tsv, .log and .out files that were not renamed from another kind of file.
Review-Finding: p1v/outside-voice#1 reader=outside-voice
Review-Finding: p1v/outside-voice#2 reader=outside-voice
Review-Finding: p1v/targeted#1 reader=targeted
Review-Finding: p1v/adversarial-native#2.1 reader=adversarial-native
Review-Finding: p1v/adversarial-native#2.2 reader=adversarial-native
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
---------
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
68b1d056a4
commit
350d4196c3
6 files changed
+148
-8
No files matched your search
@@ -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.
|
||||
@@ -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__/<name>.snap` or `<name>.snap` whose conventional owner (`<name>` 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 |
|
||||
|
||||
@@ -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);
|
||||
|
||||
+18
-6
@@ -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<string>;
|
||||
}
|
||||
|
||||
/** 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');
|
||||
|
||||
@@ -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, ['<script>', 'describe.skip("security regression", function () {});', '</script>'])))[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'] });
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
Reference in new issue
Block a user