diff --git a/changelog.d/gst-972-followup-review-depth.md b/changelog.d/gst-972-followup-review-depth.md new file mode 100644 index 000000000..1091c552d --- /dev/null +++ b/changelog.d/gst-972-followup-review-depth.md @@ -0,0 +1,8 @@ +**/ship now gives full review to edits of the code that hides files from reviewers or picks the diff base, and real review to harness pin edits (GST-972 follow-up).** Both were slipping through with less review than they need. + +### Fixed +- **Gate paths:** `lib/review-exclude.ts` decides which files reviewers never see, and `lib/default-base.ts` picks the base every diff is cut from. An edit to either was planned as plain code at light depth, with no gate class. Both are now gate paths, so an edit gets full depth, with the Red Team as its own reader plus Step 11's adversarial and outside voices. `review_exclude_paths` also can't hide them from reviewers anymore. +- **Harness pins:** files under `scripts/harness-pins/` repeat their harness file's path, so a pin like `scripts/harness-pins/test/helpers/e2e-gate.ts.json` matched the `test/` rule and planned as tests. Since the one-file-per-pin split (#3903), a harness helper edit with its pin got no reviewers at all, where the old shared `scripts/harness-version.json` made it code class. Pins outside `test/` were worse. No code names a pin file, so the unused-data rule planned them as docs. Pins now always classify as code, so the batched reader with the testing and security checklists and Step 11's adversarial pass run on them again. `review_exclude_paths` can't hide pins from reviewers anymore either; they're kept like other config files. + +### Tests +- `test/review-depth.test.ts`: both modules plan gate class at full depth; harness pins classify as code and never as data, while review pin fixtures stay tests; a helper edit with its pin plans the code-class reviewers. A new no-loss check (replay GST-972) runs the GST-967 history grid on the changed paths and their stand-ins, which reproduce main's plans byte for byte: the branch only raises class and depth and drops no reviewer. `lib/review-depth.ts` is repinned under that replay. diff --git a/docs/review-policy.md b/docs/review-policy.md index 7446e4c92..edc5ce5f2 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -247,7 +247,8 @@ adds one fix-check after the first and never a second. **Classes** are unchanged. A mixed diff takes its most restrictive class: **docs** (markdown no agent or gate reads), **tests** (test files and fixtures), **code** (everything else, including every `SKILL.md` and template, markdown in a skill -directory, `*/sections/**`, `scripts/resolvers/**`, rule docs, checklists and bins) and +directory, `*/sections/**`, `scripts/resolvers/**`, rule docs, checklists, bins and the +harness pins under `scripts/harness-pins/`, whose paths repeat a `test/` segment) and **gate** (code plus auth or migration scope, a hunk the gate scan tags, a deleted code or test path, CI workflows, credential, state-root and the paths below). @@ -262,7 +263,8 @@ the working tree with untracked files; a data file named only by another data fi follows that file, four files deep. Precedence: agent prose (`.gstack/**`), a test or fixture path, a gate path, a config file (`.github/**`, anything under a `config/` directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, -`manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named +`manifest.json`, the other tool manifests `lib/review-depth.ts` lists, and the harness +pins under `scripts/harness-pins/`, which code reads by path and never names) and a named consumer each keep the path code, and YAML and TOML are always code. An SVG counts as data only when its whole text at the head is a drawing (GST-911): SVG drawing elements and RDF metadata, CSS made of declarations only (no `@` rule and no @@ -372,7 +374,8 @@ and lockfiles stay runtime. To adopt it, commit an entry like this on the base b **Gate paths for the machinery itself.** An edit to `lib/review-depth.ts`, `lib/review-collect.ts`, `bin/gstack-review-plan`, `lib/review-scope.ts`, -`lib/review-targeted.ts`, `lib/review-risk.ts`, `review/specialists/targeted.md`, `lib/ship-queue*.ts`, `lib/regen.ts`, `.gstack/generated.json`, +`lib/review-targeted.ts`, `lib/review-risk.ts`, `lib/review-exclude.ts` (which files reviewers never see), +`lib/default-base.ts` (the base every diff is cut from), `review/specialists/targeted.md`, `lib/ship-queue*.ts`, `lib/regen.ts`, `.gstack/generated.json`, `ship/sections/review-army.md.tmpl`, `ship/sections/adversarial.md.tmpl` or `scripts/resolvers/review-army.ts` is gate class: changing how much review runs gets the most review. `rules=` (`DEPTH_RULES`) hashes the deciding constants and patterns, diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 8ed65142f..5c0eedd2b 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1392,11 +1392,11 @@ Everything else stays in every reader's diff with one reason code: | Reason | Kept because | |---|---| | `policy-dir` | it is under `.gstack/` or `.github/` | -| `gate-path` | it is a gate path (lib/review-depth.ts) | +| `gate-path` | it is a gate path (lib/review-depth.ts), such as `lib/review-exclude.ts` or `lib/default-base.ts` | | `executable` | its old or new mode is executable | | `link` | its old or new mode is a symlink or a submodule commit | | `agent-prose` | it is a skill, section, resolver, template or rule file | -| `config` | it is a data-shaped config file (`package.json`, `tsconfig*.json`, `config/`, dotfiles) | +| `config` | it is a data-shaped config file (`package.json`, `tsconfig*.json`, `config/`, dotfiles, `scripts/harness-pins/`) | | `gate-tagged` | the gate scan tagged one of its hunks | | `code` | it is code or a test, not content; a binary file outside the content extensions above, or one with executable magic, counts as code | | `secret-line` | an added line reads a credential (the targeted pass's secret rule); for a file git calls binary only because of its attributes, the added lines come from `git diff --text`, which `.gitattributes` cannot change (readers' own diffs still follow the attributes and show the kept file as binary) | diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 5ee31870d..93446ee82 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -52,10 +52,12 @@ const TEST_PATH = [/(^|\/)(tests?|spec|__tests__)\/|(^|\/)tests?\.py$|(^|\/)test const DOC_PATH = /\.(md|mdx|markdown|rst|txt|adoc)$/i; /** Release metadata a version stamp writes: never a reason for review depth on its own. */ const RELEASE_METADATA = [/^CHANGELOG\.md$/, /^VERSION$/, /^agents-digest\//]; +/** The harness pins scripts/bump-harness-version.ts writes per harness file: code, though their paths repeat a test/ segment. */ +const HARNESS_PINS = /^scripts\/harness-pins\//; /** Gate paths, the review-depth machinery included: an edit to how much review runs gets the most review. */ const GATE_PATH = [ /(^|\/)\.github\/workflows\//, /(^|\/)(secrets?|credentials?|keychain|token[s]?)([._/-]|$)/i, /state-root/, /(^|\/)(pregate|gate|ship-receipt|ship-policy)[^/]*\.(ts|sh|json)$/, /(^|\/)bin\/gstack-(pregate|gate|ship-receipt|verify-gate|ci-gate)$/, - /^lib\/review-(depth|collect|scope|targeted|risk)\.ts$/, /^review\/specialists\/targeted\.md$/, /^bin\/gstack-review-plan$/, /^lib\/ship-queue[^/]*\.ts$/, /^lib\/regen\.ts$/, /^\.gstack\/generated\.json$/, /^ship\/sections\/(review-army|adversarial)\.md\.tmpl$/, /^scripts\/resolvers\/review-army\.ts$/, + /^lib\/review-(depth|collect|scope|targeted|risk|exclude)\.ts$/, /^lib\/default-base\.ts$/, /^review\/specialists\/targeted\.md$/, /^bin\/gstack-review-plan$/, /^lib\/ship-queue[^/]*\.ts$/, /^lib\/regen\.ts$/, /^\.gstack\/generated\.json$/, /^ship\/sections\/(review-army|adversarial)\.md\.tmpl$/, /^scripts\/resolvers\/review-army\.ts$/, ]; /** The path rules lib/review-exclude.ts keeps a matched file for: a gate path, agent prose, or a data-shaped config file. */ @@ -69,7 +71,7 @@ export const BINARY_DATA_PATH = /\.(sqlite3?|db|duckdb|parquet|arrow|feather|avr export const DATA_PATH = /\.(json|jsonl|ndjson|csv|tsv|png|jpe?g|gif|webp|bmp|ico|avif)$/i; /** Data-shaped files tools read by convention, with no code naming them: always code. */ const DATA_CONFIG = [ - /(^|\/)\.github\//, /(^|\/)config\//, /(^|\/)\.[^/]+$/, /(^|\/)[^/]+\.config\.[^/]+$/, + /(^|\/)\.github\//, /(^|\/)config\//, /(^|\/)\.[^/]+$/, /(^|\/)[^/]+\.config\.[^/]+$/, HARNESS_PINS, /(^|\/)(package|package-lock|npm-shrinkwrap|composer|deno|jsr|tsconfig[^/]*|jsconfig[^/]*|manifest|app|vercel|netlify|firebase|renovate|turbo|nx|lerna|biome|angular|nest-cli|lint-staged|components|pyrightconfig|cdk|serverless|appsettings[^/]*)\.json$/i, ]; @@ -176,7 +178,7 @@ export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ */ export function classifyPath(p: string, root?: string | readonly string[]): PathClass { if (RELEASE_METADATA.some(r => r.test(p))) return 'docs'; - if (AGENT_PROSE.some(r => r.test(p))) return 'code'; + if (AGENT_PROSE.some(r => r.test(p)) || HARNESS_PINS.test(p)) return 'code'; if (TEST_PATH.some(r => r.test(p))) return 'tests'; if (!DOC_PATH.test(p)) return 'code'; const top = p.includes('/') ? p.split('/')[0]! : ''; diff --git a/test/fixtures/review-pins/files/lib/review-depth.ts.json b/test/fixtures/review-pins/files/lib/review-depth.ts.json index d7396b3f1..c3a34a382 100644 --- a/test/fixtures/review-pins/files/lib/review-depth.ts.json +++ b/test/fixtures/review-pins/files/lib/review-depth.ts.json @@ -1 +1 @@ -{"sha":"d860aa6e2881c395ee2a64426518f42d79c03e1e67752574d9e39a2019200d9c","replay":"GSTA-258-L7+GST-961+962"} +{"sha":"4685626e3f25b8264a4c68181de593b7d923a676428beb4dd34fe42bb9fb4073","replay":"GSTA-258-L7+GST-961+962+GST-972"} diff --git a/test/fixtures/review-pins/replays/GST-972.json b/test/fixtures/review-pins/replays/GST-972.json new file mode 100644 index 000000000..48bd60a5d --- /dev/null +++ b/test/fixtures/review-pins/replays/GST-972.json @@ -0,0 +1 @@ +{"replay":"GST-972","justification":"GST-972 follow-up no-loss check (test/review-depth.test.ts, 'GST-972 follow-up no-loss check'): lib/review-exclude.ts and lib/default-base.ts become gate paths and scripts/harness-pins/** classifies as code and never as unconsumed data. On the GST-967 history grid over the four changed cases (1,152 planReview cases), stand-in paths reproduce main's plans at 1ccab0d byte for byte, and the branch only raises class and depth (code/light to gate/full for the two modules, tests/light and docs/light to code/light for the pins), drops no reader main dispatched and lowers no effort except the 168 checklist slots of light's batched reader, which carried the Red Team prompt at full effort and move to full depth's batched reader at the specialist effort while the Red Team keeps full effort as its own dispatch. Over the last 100 first-parent main commits to 1ccab0d (288 histories each, 28,800 plans, collectCommit with scope {} and gate scan 0), the branch plans exactly what main planned: the three commits touching these paths (a1da77e, 8d276f9, 14d7414) were already gate/full"} diff --git a/test/fixtures/review-pins/replays/GSTA-258-C+GST-972.json b/test/fixtures/review-pins/replays/GSTA-258-C+GST-972.json new file mode 100644 index 000000000..8fa0ee12b --- /dev/null +++ b/test/fixtures/review-pins/replays/GSTA-258-C+GST-972.json @@ -0,0 +1 @@ +{"replay":"GSTA-258-C+GST-972","justification":"merge of main bf2cab34c into #3959 under a rule 19 grant: lib/review-depth.ts is main's GSTA-258-C+GST-940 bytes plus exactly the GST-972 lines (the merged file minus main equals the branch minus its merge base 1ccab0d8b, line for line, with no conflicted line in the file), so GSTA-258-C's planner no-loss check (with GST-940's verdict no-loss check, replay GSTA-258-C+GST-940) and the GST-972 follow-up no-loss check (replay GST-972) both hold; on the merged tree the 'GST-972 follow-up no-loss check' in test/review-depth.test.ts and the GSTA-258-C planner tests in test/review-depth.test.ts and test/review-depth-lean.test.ts pass again, rerun on the merge with main at bf2cab34c (GStack Minder drive handoffs/GSTA-202/evidence-GST-972/followup-merge/)"} diff --git a/test/fixtures/review-pins/replays/GSTA-258-L7+GST-961+962+GST-972.json b/test/fixtures/review-pins/replays/GSTA-258-L7+GST-961+962+GST-972.json new file mode 100644 index 000000000..f2d167f57 --- /dev/null +++ b/test/fixtures/review-pins/replays/GSTA-258-L7+GST-961+962+GST-972.json @@ -0,0 +1 @@ +{"replay":"GSTA-258-L7+GST-961+962+GST-972","justification":"rule 19 merge of main c07eddb (with #3942's GSTA-258-L7+GST-961+962 repin) into #3959 (GST-972): git merged lib/review-depth.ts with no conflicted line; the merged file minus main is exactly the GST-972 lines (HARNESS_PINS, the review-exclude and default-base gate paths, the classifyPath pins rule), so this repin relies on the GST-972 follow-up no-loss check (replay GST-972) and the GSTA-258-L7+GST-961+962 justification, neither rerun on the merged file; their focused tests (test/review-depth.test.ts, test/review-depth-lean.test.ts, test/review-packets-removed.test.ts) pass on the merged tree (GSTA-268)"} diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index befbe0e2f..69ef9dda0 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -3,7 +3,7 @@ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { classifyDiff, classifyPath, dataCandidate, depthFor, gateScanNeeded, depthOfCommitInput, diffCommand, nextStep, planReview, reviewPlanLine, scanNumbers, DEPTH_RULES, FULL_PASS_CAP, SCOPED_RANGE_LINES, type DepthError, type PlanInput } from '../lib/review-depth'; +import { classifyDiff, classifyPath, dataCandidate, depthFor, gateScanNeeded, depthOfCommitInput, diffCommand, isGatePath, nextStep, planReview, reviewPlanLine, scanNumbers, DEPTH_RULES, ROLE_EFFORT, FULL_PASS_CAP, SCOPED_RANGE_LINES, type DepthError, type PlanInput } from '../lib/review-depth'; import { summarizeReviewLoop } from '../lib/review-scope'; import { sectionTexts } from '../lib/review-brief'; import { createHash } from 'node:crypto'; @@ -190,6 +190,81 @@ describe('GST-967 dispatch-superset no-loss check', () => { }); }); +describe('GST-972 follow-up: review-exclude and default-base are gate paths; harness pins are code', () => { + // Value: protects=full review on edits to what reviewers never see (review-exclude) and to the diff base (default-base), and code-class review on harness pin edits; fails_when=either module plans light depth with no gate class, or a scripts/harness-pins file classifies as tests or as unconsumed data; + // why_new=GST-972 item 4 and the GST-1084 pin split (#3903) dropped both to light or test-only plans; seam=classifyPath, classifyDiff, planReview; no_claim=what the reviewers find + test.each(['lib/review-exclude.ts', 'lib/default-base.ts'])('%s is a gate path: gate class, full depth', p => { + expect(isGatePath(p)).toBe(true); + const c = classifyDiff({ paths: [p], gateTagged: 0, perPath: [{ path: p, lines: 1, binary: false }] }); + expect([c.class, depthFor({ cls: c.class, runtimeLines: c.runtimeLines ?? 0, errors: c.errors, risk: c.risk })]).toEqual(['gate', 'full']); + }); + test('a harness pin is code, never test or data; a review pin fixture stays tests', () => { + expect(['scripts/harness-pins/test/helpers/e2e-gate.ts.json', 'scripts/harness-pins/scripts/lib/paid-plan.ts.json'].map(p => [classifyPath(p), dataCandidate(p)])).toEqual([['code', false], ['code', false]]); + expect(classifyPath('test/fixtures/review-pins/files/lib/review-depth.ts.json')).toBe('tests'); + }); + test('a harness helper edit with its pin plans the code-class reviewers, even when no code names the pin', () => { + const paths = ['test/helpers/e2e-gate.ts', 'scripts/harness-pins/test/helpers/e2e-gate.ts.json']; + const p = planReview(base({ paths, scope: { SCOPE_TESTS: 'true' }, diffLines: 20, consumed: [] })); + expect([p.class, roles(p)]).toEqual(['code', ['specialist:batched:full', 'adversarial-native:adversarial:full']]); + expect(batch(p)).toEqual(expect.arrayContaining(['testing', 'security'])); + }); +}); + +/** + * The GST-972 follow-up no-loss check (crew rule 2026-10-30; replay 'GST-972' in test/fixtures/review-pins/replays/GST-972.json). + * On the GST-967 history grid, each changed case runs on its real paths and on a stand-in (a path main and the branch both + * classify the way main classified the real one). Main's planReview at 1ccab0d (v1.94.24.0) on the real paths hashes to + * MAIN_972_PLANS, and so does the branch's on the stand-ins, so the stand-in plans are main's. On the real paths the branch only + * raises class and depth and keeps every reader main dispatched, at the same effort or higher, with one exception: light's + * batched reader, which carries the Red Team prompt at full effort, becomes full depth's batched reader at the specialist + * effort, while the Red Team keeps full effort as its own dispatch. + */ +const MAIN_972_PLANS = '798e56faf8a44c182b77e1638b9a80c554a5d727bb56a84786b1c11507b31030'; +describe('GST-972 follow-up no-loss check', () => { + // Value: protects=the no-loss rule for review-depth changes: the GST-972 follow-up drops no reviewer main dispatched and lowers no class, depth or effort beyond the named Red Team split; fails_when=a stand-in stops matching main's plans, a reader disappears, a class or depth falls, or an effort falls anywhere else; + // why_new=GST-972 follow-up; seam=planReview; no_claim=what the added readers find (they only add coverage) + test('the changed cases only raise class and depth and keep every reader main dispatched', () => { + const sha = (x: unknown) => createHash('sha256').update(JSON.stringify(x)).digest('hex'); + const cases: Record]> = { + 'review-exclude': [['lib/review-exclude.ts'], ['lib/plain-module.ts'], { SCOPE_BACKEND: 'true' }], + 'default-base': [['lib/default-base.ts'], ['lib/plain-module.ts'], { SCOPE_BACKEND: 'true' }], + 'helper+pin': [['test/helpers/e2e-gate.ts', 'scripts/harness-pins/test/helpers/e2e-gate.ts.json'], ['test/helpers/e2e-gate.ts', 'test/helpers/e2e-gate.ts.json'], { SCOPE_TESTS: 'true' }], + pin: [['scripts/harness-pins/scripts/lib/paid-plan.ts.json'], ['scripts/lib/paid-plan.ts.json'], {}], + }; + const grid = (pick: 0 | 1) => { + const rows: { name: string; fullPasses: number; last: string; bits: number; adversarialRan: boolean; class: string; depth: string; step: string; dispatches: { role: string; name: string; effort: string; checklists: string[] }[] }[] = []; + for (const [name, c] of Object.entries(cases)) for (const fullPasses of [0, 1, 2]) for (const last of ['none', 'full', 'fix-check'] as const) for (let bits = 0; bits < 16; bits++) for (const adversarialRan of [false, true]) { + const history = { fullPasses, last, fixedBlocking: !!(bits & 1), fixedBelow: !!(bits & 2), blockingOpen: !!(bits & 4), touchedGate: !!(bits & 8) }; + const p = planReview({ paths: c[pick], scope: c[2], gateTagged: 0, diffLines: 40, consumed: [], numbers: none, history, adversarialRan }); + rows.push({ name, fullPasses, last, bits, adversarialRan, class: p.class, depth: p.depth, step: p.next.step, dispatches: p.dispatches.map(d => ({ role: d.role, name: d.name, effort: d.effort, checklists: d.checklists ?? [] })) }); + } + return rows; + }; + const main = grid(1); + expect(sha(main)).toBe(MAIN_972_PLANS); + const rank: Record = { docs: 0, tests: 1, code: 2, gate: 3, light: 0, standard: 1, full: 2, medium: 1, host: 2 }; + const readers = (r: (typeof main)[number]) => new Map(r.dispatches.flatMap(d => d.role === 'specialist' ? d.checklists.map(c => [c === 'red-team' ? 'red-team' : `checklist:${c}`, d.effort] as const) : [[d.role, d.effort] as const])); + const moves: Record = {}; + let split = 0; + grid(0).forEach((b, i) => { + const m = main[i]!; + const move = `${m.name}: ${m.class}/${m.depth} -> ${b.class}/${b.depth}`; + moves[move] = (moves[move] ?? 0) + 1; + expect([rank[b.class]! >= rank[m.class]!, rank[b.depth]! >= rank[m.depth]!]).toEqual([true, true]); + const mr = readers(m), br = readers(b); + for (const [reader, effort] of mr) { + expect([move, reader, br.has(reader)]).toEqual([move, reader, true]); + if (rank[br.get(reader)!]! >= rank[effort]!) continue; + expect([reader.startsWith('checklist:'), m.depth, effort, br.get(reader), mr.get('red-team'), br.get('red-team')]).toEqual([true, 'light', 'full', ROLE_EFFORT.specialist, 'full', 'full']); + split++; + } + }); + expect([main.length, split, moves]).toEqual([1152, 168, { + 'review-exclude: code/light -> gate/full': 288, 'default-base: code/light -> gate/full': 288, 'helper+pin: tests/light -> code/light': 288, 'pin: docs/light -> code/light': 288, + }]); + }); +}); + describe('scanNumbers', () => { test('finds claims and fingerprints them', () => { const a = scanNumbers({ 'docs/bench.md': ['Median dropped 48% to 1.2s', 'no numbers here'] });