mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 00:03:15 +01:00
review: classify the PR type and always check repo conventions (#356)
The checklist now opens with a PR-type line (application, CI/infra, scripts/tooling, docs-only or mixed). Application and mixed diffs run every category; other types always run five categories, including the new Repo Conventions & Platform Fit, and report the rest as not applicable in one line instead of skipping silently. No-loss replay: GSTA-224 H1 preregistered no-loss run (run id gsta224-h1-20261031, Opus 5.5, 81 calls, $9.53) against main's checklist with a no-gstack arm; every preregistered bar passed. The review/checklist.md pin in test/review-packets-removed.test.ts moves to a32963ef with replay 'GSTA-224 H1'. Refs GSTA-224 Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed
1 parent
92a9350a32
commit
3d795e18e3
4 files changed
+162
-3
No files matched your search
@@ -0,0 +1,130 @@
|
||||
/**
|
||||
* /review's PR-type step (GSTA-224 H1, issue #356). The checklist names the PR type from the changed paths, keeps every
|
||||
* category on application and mixed diffs, always runs five categories on CI/infra, scripts/tooling and docs-only diffs,
|
||||
* and reports the rest in one not-applicable line. The classifier below is built from the patterns the checklist itself
|
||||
* lists, so a reworded rule that drops a pattern fails here before a reviewer reads it.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
|
||||
const checklist = fs.readFileSync(path.join(import.meta.dir, '..', 'review', 'checklist.md'), 'utf8').replace(/\r\n/g, '\n');
|
||||
|
||||
function between(text: string, start: string, end: string): string {
|
||||
const i = text.indexOf(start);
|
||||
expect(i).toBeGreaterThanOrEqual(0);
|
||||
const j = text.indexOf(end, i + start.length);
|
||||
expect(j).toBeGreaterThan(i);
|
||||
return text.slice(i, j);
|
||||
}
|
||||
|
||||
const block = () => between(checklist, '**PR type', '**Two-pass review:**');
|
||||
const TYPES = ['docs-only', 'CI/infra', 'scripts/tooling', 'application', 'mixed'] as const;
|
||||
type PrType = typeof TYPES[number];
|
||||
|
||||
function rule(type: PrType): string {
|
||||
const line = block().split('\n').find(l => l.startsWith('- **' + type + ':**'));
|
||||
expect(line).toBeDefined();
|
||||
return line!;
|
||||
}
|
||||
const ticks = (line: string) => [...line.matchAll(/\x60([^\x60]+)\x60/g)].map(m => m[1]!);
|
||||
const globRe = (glob: string) => new RegExp('^' + glob.replace(/[.+^\${}()|[\]\\]/g, '\\$&').replace(/\*\*/g, '\u0000').replace(/\*/g, '[^/]*').replace(/\u0000/g, '.*') + '$');
|
||||
const base = (p: string) => p.slice(p.lastIndexOf('/') + 1);
|
||||
|
||||
const MANIFESTS = ['package.json', 'bun.lock', 'bun.lockb', 'package-lock.json', 'yarn.lock', 'pnpm-lock.yaml', 'Cargo.toml', 'Cargo.lock', 'go.mod', 'go.sum', 'Gemfile', 'Gemfile.lock', 'pyproject.toml', 'uv.lock'];
|
||||
const LINT_CONFIG = /^(\.eslintrc.*|eslint\.config\..*|\.prettierrc.*|prettier\.config\..*|biome\.json|\.shellcheckrc|\.editorconfig)$/;
|
||||
|
||||
function classifier() {
|
||||
const ci = ticks(rule('CI/infra')).map(globRe);
|
||||
const scriptRes = ticks(rule('scripts/tooling')).map(globRe);
|
||||
const app = rule('application');
|
||||
const appDirs = ticks(app).filter(t => t.endsWith('/'));
|
||||
const appGlobs = ticks(app).filter(t => t.startsWith('*')).map(globRe);
|
||||
expect(appDirs).toEqual(['bin/', 'scripts/', 'tools/']);
|
||||
expect(app).toContain('package manifests and lockfiles');
|
||||
expect(app).toContain('skill Markdown');
|
||||
expect(rule('docs-only')).toContain('\x60docs/\x60');
|
||||
const isScript = (p: string) => scriptRes.some(r => r.test(base(p)));
|
||||
return (p: string): Exclude<PrType, 'mixed'> => {
|
||||
if (appDirs.some(d => p.startsWith(d)) && !isScript(p)) return 'application';
|
||||
if (MANIFESTS.includes(base(p)) || appGlobs.some(r => r.test(base(p))) || base(p) === 'SKILL.md') return 'application';
|
||||
if (/\.(md|txt)$/.test(p) && (p.startsWith('docs/') || /^(README|CHANGELOG)/.test(base(p)))) return 'docs-only';
|
||||
if (ci.some(r => r.test(p) || r.test(base(p))) || /(^|\/)(docker-)?compose\.ya?ml$|\.tf$/.test(p)) return 'CI/infra';
|
||||
if (isScript(p) || LINT_CONFIG.test(base(p))) return 'scripts/tooling';
|
||||
return 'application';
|
||||
};
|
||||
}
|
||||
|
||||
function prType(paths: string[]): PrType {
|
||||
const one = classifier();
|
||||
const types = new Set(paths.map(one));
|
||||
return types.size > 1 ? 'mixed' : [...types][0]!;
|
||||
}
|
||||
|
||||
describe('review checklist PR type (GSTA-224 H1)', () => {
|
||||
test('the PR type is decided first and names the five types in order', () => {
|
||||
const b = block();
|
||||
expect(checklist.indexOf('**PR type')).toBeLessThan(checklist.indexOf('## Review Categories'));
|
||||
expect(b).toContain('\x60PR type: <type>\x60');
|
||||
let at = -1;
|
||||
for (const type of TYPES) {
|
||||
const i = b.indexOf('- **' + type + ':**');
|
||||
expect(i).toBeGreaterThan(at);
|
||||
at = i;
|
||||
}
|
||||
expect(rule('application')).toContain('When in doubt, application.');
|
||||
});
|
||||
|
||||
test.each(([
|
||||
['a .tmpl change', ['review/SKILL.md.tmpl', 'review/SKILL.md'], 'application'],
|
||||
['a bin/ TypeScript change', ['bin/gstack-design-detect.ts'], 'application'],
|
||||
['an extensionless bin/ executable', ['bin/gstack-approval-classes'], 'application'],
|
||||
['scripts/ TypeScript', ['scripts/skill-bin-drift.ts'], 'application'],
|
||||
['package.json + bun.lock', ['package.json', 'bun.lock'], 'application'],
|
||||
['a workflow-only change', ['.github/workflows/free-tests.yml'], 'CI/infra'],
|
||||
['a Dockerfile', ['Dockerfile.ci'], 'CI/infra'],
|
||||
['a .bat add at the root', ['build.bat'], 'scripts/tooling'],
|
||||
['a .bat add under scripts/', ['scripts/build.bat'], 'scripts/tooling'],
|
||||
['a shell script and the Makefile', ['scripts/release.sh', 'Makefile'], 'scripts/tooling'],
|
||||
['docs only', ['docs/skills.md', 'README.md'], 'docs-only'],
|
||||
['docs plus a checklist prompt', ['docs/skills.md', 'review/checklist.md'], 'mixed'],
|
||||
['a workflow plus application code', ['.github/workflows/ci.yml', 'lib/review-brief.ts'], 'mixed'],
|
||||
['a workflow plus a .bat script', ['.github/workflows/ci.yml', 'build.bat'], 'mixed'],
|
||||
['an unplaceable path', ['assets/logo.svg'], 'application'],
|
||||
] as Array<[string, string[], PrType]>).map(([name, paths, expected]) => [name + ' is ' + expected, paths, expected] as const))('%s', (_name, paths, expected) => {
|
||||
expect(prType(paths)).toBe(expected);
|
||||
});
|
||||
|
||||
test('application and mixed diffs run everything; other types always run five categories and report the rest in one line', () => {
|
||||
const scope = block().slice(block().indexOf('On application and mixed diffs'));
|
||||
expect(scope).toContain('On application and mixed diffs, run every category below exactly as written; nothing is skipped.');
|
||||
const always = /always run (.+?), and report/.exec(scope);
|
||||
expect(always).not.toBeNull();
|
||||
const names = always![1]!.split(/, | and (?=Repo)/);
|
||||
expect(names).toEqual(['Gate Integrity', 'Shell Injection', 'Completeness Gaps', 'Distribution & CI/CD Pipeline', 'Repo Conventions & Platform Fit']);
|
||||
const headings = checklist.split('\n').filter(l => l.startsWith('#### ')).map(l => l.slice(5));
|
||||
for (const name of names) expect(headings.some(h => h.startsWith(name))).toBe(true);
|
||||
expect(scope).toContain('once, in one line: \x60Not applicable (PR type: <type>): <categories>\x60');
|
||||
expect(scope).toContain('never counts as passed');
|
||||
});
|
||||
|
||||
test('Repo Conventions & Platform Fit needs a concrete consequence and Distribution gains the script checks', () => {
|
||||
const conventions = between(checklist, '#### Repo Conventions & Platform Fit', '\n---');
|
||||
expect(conventions).toContain('concrete consequence');
|
||||
expect(conventions).toContain('a \x60.bat\x60 script in a repo whose scripts are bash plus a Makefile is a finding');
|
||||
expect(conventions).toContain('platforms the CI runners and docs target');
|
||||
expect(conventions).toContain('repeats no existing tool');
|
||||
const distribution = between(checklist, '#### Distribution & CI/CD Pipeline', '#### Repo Conventions');
|
||||
for (const token of ['\x60set -euo pipefail\x60', 'quoted variables', "CI runners' OS", 'release tag or commit SHA', 'pinned version']) expect(distribution).toContain(token);
|
||||
expect(distribution).toContain('except for the script and pinning checks above');
|
||||
expect(between(checklist, '## Severity Classification', '## Fix-First Heuristic')).toContain('└─ Repo Conventions & Platform Fit');
|
||||
});
|
||||
|
||||
test('the addition stays near 25 lines', () => {
|
||||
const conventions = between(checklist, '#### Repo Conventions & Platform Fit', '\n---');
|
||||
const scriptChecks = checklist.split('\n').filter(l => l.startsWith('- Scripts CI or a build relies on:') || l.startsWith('- Pinned versions:'));
|
||||
expect(scriptChecks).toHaveLength(2);
|
||||
const lines = (block() + conventions).trimEnd().split('\n').length + scriptChecks.length + 1;
|
||||
expect(lines).toBeLessThanOrEqual(25);
|
||||
});
|
||||
});
|
||||
@@ -56,6 +56,7 @@ const NO_LOSS_REPLAYS = {
|
||||
'G1': "GSTA-167 variant G Arm G1: live no-loss replay of both prose voices with part 1's prompts, 2 passes, normalizer v2, paired rule against pooled C' (parts 1 and 3; specialists at medium as in C and C2)",
|
||||
'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)",
|
||||
'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged",
|
||||
'GSTA-224 H1': 'GSTA-224 H1 preregistered no-loss run: the PR-type checklist against main\'s checklist and a no-gstack arm (Opus 5.5, 81 calls), 5 behavior cases, 10 Round 7 application blockers and 2 clean controls; every bar passed (handoffs/GSTA-224/h1-noloss-results.md)',
|
||||
} as const;
|
||||
type Replay = keyof typeof NO_LOSS_REPLAYS;
|
||||
const REVIEWER_BRIEFS: Record<string, { sha: string; replay: Replay }> = {
|
||||
@@ -63,7 +64,7 @@ const REVIEWER_BRIEFS: Record<string, { sha: string; replay: Replay }> = {
|
||||
'ship/sections/adversarial.md': { sha: 'd776ee5bc6e0c13930f8f4de05bad2f1ed9e52bc9fc5df80ae3ba7680455b2bb', replay: 'GST-257' },
|
||||
'review/sections/review-army.md': { sha: '78329b64640260cd8831ca81a9189f3f8dcade9c406431631d3f2cd798cd3967', replay: '#3638' },
|
||||
'review/sections/adversarial.md': { sha: 'e6962fa25b78f362654f2954f79683e524d2ebd452d1494f8a2b099888e8e255', replay: 'G1' },
|
||||
'review/checklist.md': { sha: '607788afeb2c83f2dff1dccd58d212616ec72393d28cda2fa18690c1a83df699', replay: '#3638' },
|
||||
'review/checklist.md': { sha: 'a32963ef38d9d3613c58c805e8517cd2e593c63769b30dd7d40e2592fa48fc78', replay: 'GSTA-224 H1' },
|
||||
'review/design-checklist.md': { sha: 'b9205f88313cc64902c0b13e9685722ab8138d3dcc58916154f47274abda7d83', replay: '#3638' },
|
||||
'review/gate-integrity.md': { sha: 'f570db344c9a01df633bcaa281d6017f4f12420fd3e0aa3078c1ce06e0df8332', replay: '#3638' },
|
||||
'review/specialists/api-contract.md': { sha: '263d23ac119dd601d315c191dbdbd503d47c00264c9c0ca81959559ca11d4e95', replay: '#3638' },
|
||||
|
||||
Reference in new issue
Block a user