From a7b87145c3f6f76b1915a3fd6f16ffc104bf3dc4 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:16:50 +0000 Subject: [PATCH] docs, tests and changelog for review_exclude_paths (GST-508) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-508-review-exclude-paths.md | 15 + docs/review-policy.md | 19 ++ docs/ship-policy.md | 122 ++++++++ docs/troubleshooting.md | 66 +++++ test/review-exclude.test.ts | 277 ++++++++++++++++++ .../ship/gst-508-review-exclude-paths.json | 4 + 6 files changed, 503 insertions(+) create mode 100644 changelog.d/gst-508-review-exclude-paths.md create mode 100644 test/review-exclude.test.ts create mode 100644 test/size-ledger/ship/gst-508-review-exclude-paths.json diff --git a/changelog.d/gst-508-review-exclude-paths.md b/changelog.d/gst-508-review-exclude-paths.md new file mode 100644 index 000000000..7f055b7db --- /dev/null +++ b/changelog.d/gst-508-review-exclude-paths.md @@ -0,0 +1,15 @@ +**/ship can leave a results PR's receipt files out of the review, and it stops instead of handing a reader a diff it cannot read.** A results PR in harness-evals or gbrain's eval lanes can carry thousands of receipt files. One HARN-1 results PR spent its outside-voice budget reading receipts and the pass died anyway (GST-507, GST-508). The repository owner can now list those folders in `.gstack/ship-policy.json`, and every reviewer reads the code without them. + +### Added +- **`review_exclude_paths` in `.gstack/ship-policy.json`** (`["receipts/**"]`, with `min_gstack_version` at least the release that adds it). It is read from the base branch only, so a branch cannot exclude its own files; a branch that adds the key gets a normal review and `REVIEW_EXCLUDE: none (head sets review_exclude_paths; …)`. Patterns are git glob pathspecs, root-anchored, and git itself lists the matches. +- **Only content is excluded**: data files, docs, `.log` files and files git reports as binary. Code, tests, executables, gate paths, agent prose, config, `.gstack/` and `.github/` files, gate-tagged files and files with a credential line stay in every reader's diff, each with a reason code. A data file a code or test file names by its full path stays too. A basename, a directory reference or a data manifest that lists receipts does not keep anything. +- **`gstack-review-plan` prints `REVIEW_EXCLUDE:`** with the counts, the excluded files by kind, the kept files and a digest. Every reader's `DIFF_CMD:`, the fixture command the security specialist and the Red Team use, and the outside voice's diff leave the files out; kept files come back through `DIFF_KEEP:`. Every brief says what was left out. The kept-file command is bounded at 24,000 argument bytes, and a pattern over that bound is not applied and the plan says why. +- **Excluded lines no longer trigger the numeric-claim scan or the targeted money, gate and auth classes**, and they count as docs for the review depth. +- **The receipt records `review_exclude`.** `gstack-review-dispatch verify` prints `REVIEW_EXCLUDE_RECORD: digest=` from the briefs the readers got, and `gstack-ship-receipt write --review-exclude ` must match the digest HEAD has under the base policy (`RECEIPT_INVALID field=review_exclude`, nothing written). Carry and the queue compare the digest, not the base commit, so a base merge that leaves the policy alone still carries. +- **A review ceiling.** `gstack-review-dispatch pass`, `stage` and `render` read each reader's diff once before writing anything. Over 2,000,000 bytes (about 500K tokens) is `REVIEW_DIFF_TOO_LARGE` (exit 3) with a fix for where the policy stands and pasteable JSON; a `git diff` that fails or runs past 120 seconds is `REVIEW_DIFF_UNREADABLE` (exit 1) with the reason, the timeout and a redacted stderr log. + +### Changed +- A repository without the key plans, briefs and diffs byte for byte as before. The one change it can see is the ceiling: a reader's diff over 2,000,000 bytes now stops the pass instead of being read in part. + +### Tests +- `test/review-exclude.test.ts`: what is excluded and kept (binary files under `config/`, a file named only by a data manifest or by its basename, a file a test names by full path), every brief's diff commands run against git, the numeric scan and targeted classes, the base policy read (no file, a missing blob, a bad commit, an invalid key), the argument-byte bound, both ceiling lines, and the receipt write, carry and queue checks. diff --git a/docs/review-policy.md b/docs/review-policy.md index 0986de1de..7b7c21bc3 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -338,6 +338,24 @@ Over the last 100 non-release commits on main (8663c3d2..8be7f213), money hit which hit 43 of 100; those paths already get full depth, so the gate class was narrowed to the gate scan's tagged hunks. + +## What readers read under `review_exclude_paths` + +A repository can list content folders its reviewers leave out, on its base +branch only ([ship-policy.md](ship-policy.md#review-exclude-paths)). Under it, +every reader (specialists, the Red Team, the native and outside voices, the +targeted reader) reads the same selection: its `DIFF_CMD:` with one +`:(top,exclude,glob)` item per applied pattern, the fixture command with the +same items, and the `DIFF_KEEP:` command for the kept files under those +patterns. The outside voice's embedded diff is that selection. Each brief says +how many files and bytes were left out and that the reader must not report on +their absence. Excluded paths count as docs for the depth and never trigger the +numeric-claim scan or a targeted class; kept files keep their own class. + +Independently of the key, no reader is handed more than 2,000,000 bytes of +diff: `gstack-review-dispatch` stops with `REVIEW_DIFF_TOO_LARGE` before it writes +anything, instead of a review of the part a reader happened to see. + ## What the receipt records `gstack-review-scope summary --since ` reads this invocation's @@ -532,6 +550,7 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: - `scripts/resolvers/review-army.ts`: the floor and `NO FINDINGS` in every specialist and Red Team prompt (rendered into `review/sections/review-army.md` and `ship/sections/review-army.md`), and the lead judgment's "On record" line that turns a [ruled class](#rulings-below-the-floor) into a note; in `/ship` every specialist and Red Team prompt it renders carries its diff base as a literal sha the parent writes in (`DIFF_BASE=`: the merge base on pass 1, `RANGE_BASE` from pass 2 on) beside its plan's `DIFF_CMD` excludes, because a reviewer is its own process and never sees the parent's shell variables; the fan-out batches above. - `ship/sections/review-army.md` Step 9: the plan's `REVIEW_DEPTH`, `GATE_SCAN` and `--depth-floor` on every run, the depth fields of the review-log row, passes, `RANGE_BASE`, classify, defer, render, the cap check before any edit, the Step 11 late-defer row, scratch files under `.gstack/tmp/`, the cap decisions; `ship/sections/adversarial.md` Step 11: the outside voice reads the same range and scopes queued fixes the same way. +- `lib/review-exclude.ts`: the base policy's `review_exclude_paths` (matched set, kept reasons, the kept-file argument bound, the digest the receipt, carry and the queue compare) and the 2,000,000-byte reader ceiling (`test/review-exclude.test.ts`). - `lib/review-depth.ts`, `bin/gstack-review-plan`: the diff class, the depth (`depthFor`, `DEPTH_RULES`, `depthOfCommitInput`), each dispatch's `exclude` pathspecs and `DIFF_CMD`, the transition table, the numeric-claim scan and each role's effort tier (`test/review-depth.test.ts`, `test/review-plan-cli.test.ts`). - `lib/review-collect.ts`: `collectCommit` (git objects only, from any checkout) and `collectLive` (the working tree), per-path lines and the base commit's registry (`test/review-collect.test.ts`). - `lib/review-scope.ts`, `bin/gstack-review-scope`: `classify`, `defer`, `render`, `summary` (with `fix_checks`, `dispatches`, `est_input_tokens`); refusals are `REVIEW_SCOPE_INPUT` and `REVIEW_SCOPE_RANGE` ([troubleshooting.md](troubleshooting.md#review-scope-input)). diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 2a28abc1a..1f87e5ddc 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -58,6 +58,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `gate_command` | command or `null` | `null` | The repo's full pre-merge gate. `gstack-ship-queue gate-ahead` and `ready` print it for the caller to run (in the background or in CI); gstack never executes it. Needs `min_gstack_version`. | | `full_e2e` | `true` \| `false` | `false` | `true`: every merge needs a passing full-gate result bound to a tree equal to the merging one modulo stamps; `gstack-ship-queue ready`, `stamp` and `verify` refuse without one (`QUEUE_GATE_MISSING`): a gate-ahead record, or the lane's `--gate-exit 0 --gate-log ` from its run on the gated head. Needs `min_gstack_version`. | | `review_scope` | `frozen` \| `diff-plus-blocking` \| `all` | `frozen` | The finding scope of `/ship`'s review loop; see [review_scope](#review-scope). `frozen` fixes the diff on pass 1, files blocking findings in unchanged code as issues, and later passes read only the range since the previous pass. `diff-plus-blocking` is the same but fixes a blocking finding in unchanged code in this PR instead of filing it. `all` is the earlier loop: every finding is fixed on every pass. Needs `min_gstack_version` when not `frozen`. | +| `review_exclude_paths` | git glob pathspecs | `[]` | Content files (data, docs, binary, `.log`) `/ship`'s reviewers leave out; read from the base only, with code, gate, config, executable and named-by-code files kept. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | | `min_gstack_version` | version or `null` | `null` | The oldest gstack that may read the file; see the compatibility rule above. | | `docs` | `{ caps: [{ path, max_bytes }] }` or `null` | `null` | Documentation files with a byte cap. `/ship` Step 14.5 runs `gstack-ship-receipt docs-caps --base ` (the caps come from `origin/`'s policy, never the branch's copy; `--policy ` is for a repository bootstrapping its policy) before the verification gate and prints one `DOCS_CAP: / ok\|over` line per cap; any `over` (an absent file counts) exits 1 and blocks the ship until the file is trimmed or the cap raised on the base branch (`DOCS_AUDIT_BLOCKED`). At most 64 caps, each path contained. See [the ship receipt](#ship-receipt). Needs `min_gstack_version`. | | `train` | `{ gate_workflow, free_workflow, full_gate_check, record_artifact, max_size, fill_hours, screen_selection, trial_reservation_usd, land? }`, `{ gates, land, max_size, fill_hours }` or `null` | `null` | Turns on [merge trains](#run-a-merge-train), read from the base revision only. Every field is required and an unknown field is `POLICY_INVALID`: `max_size` 1 to 6, `fill_hours` above 0 and at most 24, `screen_selection` `gstack-paid` \| `none`, `trial_reservation_usd` above 0 and at most 100. A block with `gates` ([any repository](#train-gates)) takes `max_size` 1 to 5 and needs `min_gstack_version` at least `TRAIN_GATES_KEY_VERSION`. `null` keeps the per-PR queue exactly as it is. Needs `min_gstack_version`. | @@ -1094,6 +1095,127 @@ is not a `pass` claim. Text you write under the heading after that block, and a `## Approvals` example inside a code fence, are still checked. Data PRs (evals, measurements, sweeps) run it before READY so the body's numbers are the run's. + +## `review_exclude_paths`: what reviewers leave out + +Since the release that added it (`REVIEW_EXCLUDE_KEY_VERSION` in +`lib/ship-policy.ts`). A results PR can carry thousands of receipt files that +no reviewer needs to read. `review_exclude_paths` lists git glob pathspecs +whose content files `/ship`'s reviewers leave out: + +```json +{ "review_exclude_paths": ["receipts/**"], "min_gstack_version": "1.92.0.0" } +``` + +**Read from the base only.** `gstack-review-plan` reads the policy committed on +the base commit it already resolved (`origin/`, or ``), never +the branch's copy, so a branch cannot exclude its own files. A branch that adds +the key gets `REVIEW_EXCLUDE: none (head sets review_exclude_paths; it applies +after it lands on )` and a normal review. A base policy that does not +validate stops the plan (`POLICY_INVALID`), and a base git cannot read stops it +(`POLICY_SOURCE_UNAVAILABLE`); neither turns exclusion off quietly. + +**Git's glob dialect.** Each pattern is a `:(top,glob)` pathspec: root-anchored +and case-sensitive. The matched set comes from git itself (`git diff --raw -z`, +plus `git ls-files --others -z` for untracked files), so the plan, the receipt +and what readers see are one set. + +| Pattern | gitignore would match | `review_exclude_paths` matches | +|---|---|---| +| `receipts/**` | everything under `receipts/` at any depth | everything under the top-level `receipts/` | +| `receipts/*` | files directly in `receipts/` | files directly in `receipts/` only (`*` never crosses `/`) | +| `data/` | any `data/` directory at any depth | nothing: write `data/**` (and `**/data/**` for any depth) | + +A pattern that matches no changed file is listed as `unmatched=` on the plan line. + +**What is excluded, and what is kept.** A matched file is excluded only when it +is content: a data file (JSON, JSONL, CSV, TSV, images), a docs-class file, a +`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers +would only see `Binary files differ`). A binary file is content even under +`config/` or in a skill directory; only the first four reasons below keep it. +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) | +| `executable` | its old or new mode is executable | +| `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) | +| `gate-tagged` | the gate scan tagged one of its hunks | +| `code` | it is code or a test, not content | +| `secret-line` | an added line reads a credential (the targeted pass's secret rule) | +| `named-by-code` | a code or test file that stays in review (not docs, a data file, a `.log` or binary, and not itself excluded) names its full repository path, at the head or the merge base | +| `consumer-scan-unavailable` | the `git grep` for those names failed, so every candidate is kept | + +Only a full path counts as a name. A basename such as `result.json` would keep +every receipt in the tree, and a directory reference such as `receipts/` (the +harness that writes the receipts names it) keeps nothing: the plan prints an +informational `REVIEW_EXCLUDE_REFERENCE:` line naming the code that reads the +folder. A data file never keeps another file: a results manifest such as +`docs/receipts-manifest.json` lists every receipt by full path, and it is +content, not a program that reads them. The name scan is one uncapped `git grep -F -f` over the candidates' +paths, not the collector's capped consumer scan. + +**What readers run.** Every reader's `DIFF_CMD:` gains one +`':(top,exclude,glob)'` item per applied pattern, and the fixture +command the security specialist, the Red Team and the native adversarial pass +read through gets the same items. A git exclude beats any positive pathspec, so +kept files under an applied pattern come back through a second command, +`DIFF_KEEP: git diff -- ':(top,literal)' …`, which every +brief names. That command is bounded by its argument bytes (24,000 across all +patterns, under Windows' command-line limit): a pattern whose kept files would +pass the bound is not applied, and the plan line says so as +`skipped=:kept=:bytes=`. Every brief also gets one line: +`Excluded by base policy @ (selection ): files, bytes +on this branch matching . They are not in your diff; do not report on +their absence.` + +**Depth, numbers and the targeted pass.** Excluded paths count as docs for the +review depth everywhere it is computed (`gstack-review-plan`, the receipt +writer, carry and the queue), and their lines never trigger the numeric-claim +scan or the targeted money, gate, auth and secret classes. Defects inside a +correctly excluded data file are coverage the owner gives up by policy. + +**The plan line.** `REVIEW_EXCLUDE: patterns=/ excluded= +bytes= kept= content=:,… source=@ digest=` +(`content=` counts the excluded files by kind: `binary`, `data`, `docs`, `log`), then up to 20 +`REVIEW_EXCLUDE_KEPT: ` lines (`--json` lists every one). A +repository with no key anywhere prints none of these lines, and its plan, briefs +and diff commands are byte for byte what they were. + +**The receipt, carry and the queue.** Each rendered brief records the digest it +names in the review ledger, and `gstack-review-dispatch verify` prints +`REVIEW_EXCLUDE_RECORD: digest=` when every reader's newest brief names the +same one (a mixed set makes it `VERIFY: dirty`). `gstack-ship-receipt write +--review-exclude `, with that digest, records `review_exclude` (source, patterns, counts, the gate-tagged kept +paths and three hashes). The `digest` hashes the declared patterns and the +excluded and kept lists; `source` is provenance only. Write, carry and the queue +each recompute the digest at the current base and compare it, so a base merge +or a moved base that leaves the patterns and the lists unchanged still carries +and binds. A different digest means the reviewers read another selection: +write refuses with `RECEIPT_INVALID field=review_exclude`, carry with +`RECEIPT_CARRY_REFUSED review_exclude changed`, and the queue with +`RECEIPT_INVALID field=review_exclude expected= recorded=`. The +fix is to rerun the review, never a receipt write alone. + +**Adopting it.** (1) Open a PR on the base that adds the patterns and +`min_gstack_version` at least `REVIEW_EXCLUDE_KEY_VERSION`, and ship it like any +policy change. (2) Once it merges, bring the base into the results branch (a +crew needs its coordinator's word for that merge) and rerun `/ship`; the plan +prints `REVIEW_EXCLUDE:`. (3) Check the kept list: a file you expected to be +excluded names its reason. + + +**The review ceiling.** Independently of the key, `gstack-review-dispatch` reads +each reviewer's diff once before it writes a manifest or a brief (`pass`, `stage` +and `render`). A diff over 2,000,000 bytes (about 500K tokens) stops with +[`REVIEW_DIFF_TOO_LARGE`](troubleshooting.md#review-diff-too-large), exit 3, and a +fix chosen by where the policy stands; a `git diff` that fails or runs past +120 seconds stops with +[`REVIEW_DIFF_UNREADABLE`](troubleshooting.md#review-diff-unreadable), exit 1. +There is no override flag: the recovery is a policy PR on the base, or a split. + ## `review_scope`: how the review loop converges diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9cf9db55c..6e797ca84 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3079,6 +3079,72 @@ checked-out HEAD: the receipt binds the tree the review read. **Fix.** Regenerate it with `gstack-ship-receipt write`; never hand-edit the block. For the `--head` refusal, check out that commit and write there. + +**`field=review_exclude`.** `gstack-ship-receipt write` and the queue's receipt check also +use this code when the receipt's review_exclude digest (or the +`--review-exclude ` write was given) is not the digest HEAD has under the +base policy's [`review_exclude_paths`](ship-policy.md#review-exclude-paths). The line +names `expected=` (now), `recorded=` (what the review read, `none` when it read +without exclusion) and `source=` (the base commit). The reviewers read another +selection than the one the head now has, so a receipt write alone cannot clear +it: rerun `/ship`'s review on the head (`gstack-review-dispatch pass --base `), +run `gstack-review-dispatch verify --ledger `, which prints +`REVIEW_EXCLUDE_RECORD: digest=` from the briefs the readers got, then +`gstack-ship-receipt write --pr --review-exclude --body `. + + +### `REVIEW_DIFF_TOO_LARGE` + +**Meaning.** One reviewer's diff is over the 2,000,000-byte review ceiling +(about 500K tokens). `gstack-review-dispatch pass`, `stage` and `render` read +each reader's diff once before writing anything, and stop (exit 3, nothing +written) rather than hand a reader more than it can read. The line names the +reader, its bytes, the five largest top-level folders (`top=:`) and +the base policy commit. + +**Fix.** The line prints the fix for where the base policy stands: + +- **No key:** copy the printed JSON (`{"review_exclude_paths":[…],"min_gstack_version":"…"}`) + into `.gstack/ship-policy.json` on the base as its own PR. A lane cannot land on + the base itself: ask the repo owner or the coordinator. Once it merges, bring + the base into the branch and rerun once; if it is still over, split the PR. +- **The branch declares the key but the base does not:** only the base copy + applies; land that hunk on the base first, as above. +- **A pattern was not applied** (`skipped=` on the `REVIEW_EXCLUDE:` line): its kept + files need more than 24,000 argument bytes. Narrow the pattern, move the kept + files out of it, or split the PR. +- **Still over after exclusion:** add the printed folders to the base policy, or + split the PR. + +There is no override flag. See [the review ceiling](ship-policy.md#review-diff-ceiling). + + +### `REVIEW_DIFF_UNREADABLE` + +**Meaning.** git could not read a reviewer's diff, so the pass stopped (exit 1) +before writing a manifest or a brief. `reason=timeout` means `git diff` ran past +`timeout_ms` (120,000 ms; fixed, no flag); `reason=git-exit` gives git's exit code. +The line carries the diff base, git's first stderr line with remote URLs +replaced by ``, and `log=`, a file under the run directory holding git's whole +stderr, redacted the same way. It is also printed when git cannot list the +files `review_exclude_paths` matches. + +**Fix.** By reason: a timeout, rerun once unchanged, then split the PR or +exclude its largest data folders on the base; git exit 128 (a missing object or +an unfetched base), `git fetch origin ` and rerun once; anything else, stop +and ask a person with the log file. + + +### `REVIEW_EXCLUDE_REFERENCE` + +**Meaning.** Informational, never a stop. A kept code file names a folder that +`review_exclude_paths` excludes (for example the harness that writes +`receipts/`). A folder reference does not keep files; only a full file path +does, so readers see that code without the files it reads. + +**Fix.** Nothing for the review itself. If the code's correctness depends on the +excluded files, narrow the pattern on the base so they stay in the diff. + ### `EXECUTION_UNKNOWN` diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts new file mode 100644 index 000000000..337045a51 --- /dev/null +++ b/test/review-exclude.test.ts @@ -0,0 +1,277 @@ +/** + * review_exclude_paths and the reviewer diff ceiling (GST-508, docs/ship-policy.md#review-exclude-paths): what the base + * policy excludes and keeps, what every brief's diff commands read, the receipt binding, the base-policy read, the + * argument-byte bound and the REVIEW_DIFF_TOO_LARGE / REVIEW_DIFF_UNREADABLE lines. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { EXIT } from '../lib/headless-artifacts'; +import { KEEP_ARG_MAX_BYTES, readSelection, tooLargeLine, unreadableLine } from '../lib/review-exclude'; +import { extractReceipt } from '../lib/ship-receipt'; +import { bindShippedReceipt } from '../lib/ship-receipt-depth'; +import { REVIEW_EXCLUDE_KEY_VERSION, isUnavailable, loadPolicyAt } from '../lib/ship-policy'; +import { cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); +const policyText = (patterns: string[]) => JSON.stringify({ review_exclude_paths: patterns, min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n'; +const GZ = Buffer.from([0x1f, 0x8b, 0, 1, 2, 3, 0, 0]); +const EXCLUDED = ['receipts/a/result.json', 'receipts/a/trial.gz', 'receipts/config/blob.bin', 'receipts/c/result.json', 'receipts/e/result.json', 'receipts/notes.md']; +const KEPT = ['receipts/b/result.json', 'receipts/d/fixture.json', 'receipts/run.sh']; + +/** A results branch: receipts/** on the base policy, receipts of every kind on the branch. */ +function census(patterns: string[] | null = ['receipts/**']): FixtureRepo { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: patterns ? { '.gstack/ship-policy.json': policyText(patterns) } : {} }); + repos.push(repo); + for (const [rel, bytes] of [['receipts/a/trial.gz', GZ], ['receipts/config/blob.bin', GZ]] as const) { + fs.mkdirSync(path.dirname(path.join(repo.work, rel)), { recursive: true }); + fs.writeFileSync(path.join(repo.work, rel), bytes); + } + write(repo.work, 'receipts/run.sh', '#!/bin/sh\necho run\n'); + fs.chmodSync(path.join(repo.work, 'receipts/run.sh'), 0o755); + commit(repo, { + 'receipts/a/result.json': '{"cost_usd": 12.5, "pass_rate": "97%"}\n', + 'receipts/b/result.json': '{"ok": true}\n', + 'receipts/c/result.json': '{"ok": true}\n', + 'receipts/d/fixture.json': '{"ok": true}\n', + 'receipts/e/result.json': '{"ok": true}\n', + 'receipts/notes.md': '# notes\n', + 'docs/receipts-manifest.json': JSON.stringify(['receipts/c/result.json', 'receipts/a/trial.gz']) + '\n', + 'lib/reader.js': "exports.p = 'receipts/b/result.json';\nexports.q = 'result.json';\n", + 'test/d.test.js': "const f = 'receipts/d/fixture.json';\n", + }); + return repo; +} +const plan = (repo: FixtureRepo, extra: string[] = []) => runBin(repo, 'gstack-review-plan', ['--base', 'main', '--gate-tagged', '0', ...extra]); +const mergeBase = (repo: FixtureRepo) => repo.git('merge-base', 'origin/main', 'HEAD'); +/** The paths one diff command from a plan line or a brief reads (run as --name-only in the fixture). */ +function pathsRead(repo: FixtureRepo, cmd: string, diffBase: string): string[] { + const named = cmd.replace(/^git diff( --stat| --name-status)?/, 'git diff --name-only').replace('', '"$DIFF_BASE"'); + const r = spawnSync('bash', ['-c', named], { cwd: repo.work, encoding: 'utf8', timeout: 30_000, env: { ...process.env, ...repo.env, DIFF_BASE: diffBase } }); + expect({ cmd: named, status: r.status, err: r.stderr }).toMatchObject({ cmd: named, status: 0 }); + return r.stdout.split('\n').filter(Boolean); +} +/** Rewrite the base: commit on main, push, and merge it into the feature branch. */ +function onBase(repo: FixtureRepo, files: Record) { + repo.git('checkout', '-q', 'main'); + commit(repo, files, 'base change'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feature'); + repo.git('fetch', '-q', 'origin'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); +} + +describe('what review_exclude_paths excludes and keeps', () => { + // Value: protects=a results PR's receipts leave the reviewers' diff while code, executables and files a program names stay; + // fails_when=a binary file under config/ is kept, a file named only by a data manifest or by its basename is kept, or a + // file a test names by full path is excluded; why_new=GST-508 adds the selection; seam=bin/gstack-review-plan on a real + // repository and git running the printed commands; no_claim=references by relative path or directory (REVIEW_EXCLUDE_REFERENCE) + test('binary, data and docs files are excluded; only a code or test file naming the full path keeps one', () => { + const repo = census(); + const p = JSON.parse(plan(repo, ['--json']).out); + expect(p.review_exclude.kept_files).toEqual([ + { path: 'receipts/b/result.json', reason: 'named-by-code' }, + { path: 'receipts/d/fixture.json', reason: 'named-by-code' }, + { path: 'receipts/run.sh', reason: 'executable' }, + ]); + expect(p.review_exclude).toMatchObject({ state: 'active', excluded: EXCLUDED.length, content: { binary: 2, data: 3, docs: 1, log: 0 } }); + const text = plan(repo).out; + expect(text).toContain(`REVIEW_EXCLUDE: patterns=1/1 excluded=${EXCLUDED.length} `); + expect(text).toContain(' content=binary:2,data:3,docs:1 '); + const mb = mergeBase(repo); + const cmds = text.split('\n').filter(l => /^DIFF_(CMD|KEEP): /.test(l)).map(l => l.replace(/^DIFF_(CMD|KEEP): \S+ /, '')); + expect(cmds.length).toBeGreaterThan(2); + const read = new Set(cmds.flatMap(c => pathsRead(repo, c, mb))); + for (const f of EXCLUDED) expect(read.has(f)).toBe(false); + for (const f of KEPT) expect(read.has(f)).toBe(true); + }, 120_000); + + // Value: protects=the Red Team, the security specialist, the adversarial pass and the outside voice read the same selection + // as the plan, including through the fixture-handling command; fails_when=any brief's git diff command (or the embedded + // outside-voice diff) shows an excluded file, or a kept file is unreachable from a brief; why_new=task 8 (the fixture + // command had no exclude slot); seam=gstack-review-dispatch pass/stage/render and git; no_claim=what a model does with the brief + test('every brief\'s git diff commands leave excluded files out and reach the kept ones', () => { + const repo = census(); + const out = path.join(repo.root, 'pass'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', out, '--pass', 'p1', '--', '--gate-tagged', '0']).status).toBe(EXIT.ok); + expect(runBin(repo, 'gstack-review-dispatch', ['stage', '--manifest', path.join(out, 'pass-manifest.json'), '--stage', 'voices']).status).toBe(EXIT.ok); + const m = JSON.parse(fs.readFileSync(path.join(out, 'pass-manifest.json'), 'utf8')); + const reader = m.readers.find((r: any) => r.role === 'adversarial-native'); + const mb = mergeBase(repo); + const slots = ['--base', 'main', '--diff-base', mb, '--exclude', reader.exclude, '--keep', ['--', ...reader.keep.map((k: string) => `'${k}'`)].join(' '), '--disclosure', m.disclosure]; + for (const role of [['specialist', '--name', 'security'], ['red-team', '--specialists', '1'], ['outside-voice']]) { + const r = runBin(repo, 'gstack-review-dispatch', ['render', '--role', ...role, '--out', path.join(out, 'briefs'), ...slots]); + expect(r.status).toBe(EXIT.ok); + } + const briefs = fs.readdirSync(path.join(out, 'briefs')).filter(f => f.endsWith('.md')).map(f => fs.readFileSync(path.join(out, 'briefs', f), 'utf8')); + expect(briefs.length).toBe(5); + const cmds = [...new Set(briefs.flatMap(b => [...b.matchAll(/`[^`\n]*?(git diff [^`\n]*)`/g)].map(x => x[1]!)))]; + expect(cmds.some(c => c.includes("'*fixture*'"))).toBe(true); + const read = new Set(cmds.flatMap(c => pathsRead(repo, c, mb))); + for (const f of EXCLUDED) expect(read.has(f)).toBe(false); + for (const f of KEPT) expect(read.has(f)).toBe(true); + const outside = briefs.find(b => b.includes('## Branch diff'))!; + expect(outside).toContain(`git diff "$DIFF_BASE" -- . ${m.readers[0].policy_exclude} and git diff "$DIFF_BASE" -- '${reader.keep[0]}'`); + for (const f of EXCLUDED) expect(outside).not.toContain(`diff --git a/${f}`); + expect(outside).toContain('diff --git a/receipts/b/result.json'); + for (const b of briefs) expect(b).toContain(`(selection ${m.review_exclude.digest})`); + }, 180_000); + + // Value: protects=excluded receipts cannot dispatch the outside voice or the targeted money reader on lines no reader sees, + // and a credential line inside a matched file keeps it; fails_when=scanNumbers or targetedClasses read excluded lines, or + // a secret-bearing data file is excluded; why_new=task 9; seam=bin/gstack-review-plan; no_claim=pregate's credential scan + test('excluded lines feed neither the numeric scan nor the targeted classes; a credential line keeps its file', () => { + const files = { 'receipts/r.json': '{"cost_usd": 12.5, "price_usd": 3, "pass_rate": "97%", "latency_ms": 180}\n', 'docs/bench.md': '# bench\n', 'receipts/s.json': '{"cmd": "curl -H $OPENAI_API_KEY"}\n' }; + const control = makeFixtureRepo({ prefix: 'gstack-exclude-' }); repos.push(control); + commit(control, files); + const with_ = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['receipts/**']) } }); repos.push(with_); + commit(with_, files); + const c = JSON.parse(plan(control, ['--json']).out); + const w = JSON.parse(plan(with_, ['--json']).out); + expect(c.numbers.publishes).toBe(true); + expect(c.targeted.classes).toEqual(['money', 'secret']); + expect(w.review_exclude.kept_files).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); + expect(w.numbers.publishes).toBe(false); + expect(w.targeted.classes).toEqual(['secret']); + }, 120_000); +}); + +describe('the base policy read', () => { + // Value: protects=a base the plan cannot read never turns exclusion off quietly; fails_when=a missing blob or a bad commit + // reads as no policy, or an invalid base key plans without exclusion; why_new=task 6; seam=loadPolicyAt and the plan CLI; + // no_claim=the head copy's validation (gstack-ship-policy validate) + test('no file reads as defaults; an unreadable object or commit is unavailable; an invalid base key refuses the plan', () => { + const none = census(null); + const sha = none.git('rev-parse', 'origin/main'); + const absent = loadPolicyAt(none.work, sha, 'origin/main'); + expect(isUnavailable(absent)).toBe(false); + expect(!isUnavailable(absent) && absent.policy.review_exclude_paths).toEqual([]); + expect(isUnavailable(loadPolicyAt(none.work, 'f'.repeat(40), 'origin/main'))).toBe(true); + + const gone = census(); + const blob = gone.git('rev-parse', 'origin/main:.gstack/ship-policy.json'); + fs.rmSync(path.join(gone.work, '.git/objects', blob.slice(0, 2), blob.slice(2))); + const unread = loadPolicyAt(gone.work, gone.git('rev-parse', 'origin/main'), 'origin/main'); + expect(isUnavailable(unread)).toBe(true); + const r = plan(gone); + expect(r.status).toBe(EXIT.refused); + expect(r.err).toContain('POLICY_SOURCE_UNAVAILABLE'); + + const bad = census([':(icase)receipts/**']); + const b = plan(bad); + expect(b.status).toBe(EXIT.refused); + expect(b.err).toContain('review_exclude_paths[0]'); + expect(b.err).toContain('(POLICY_INVALID)'); + expect(b.out).not.toContain('DISPATCH:'); + }, 120_000); +}); + +describe('the kept-file command is bounded by argument bytes', () => { + // Value: protects=the DIFF_KEEP command stays under the Windows command line however many kept files a pattern has, and a + // disjoint small pattern still applies; fails_when=a pattern whose kept files pass KEEP_ARG_MAX_BYTES is applied, or the plan + // does not say why it was not; why_new=task 10; seam=bin/gstack-review-plan; no_claim=a real Windows launch + test('a pattern whose kept files pass the bound is not applied and the plan says why; a disjoint small pattern applies', () => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['big/**', 'small/**']) } }); repos.push(repo); + const long = 'x'.repeat(180); + for (let k = 0; k < 140; k++) { + write(repo.work, `big/${long}-${k}.sh`, '#!/bin/sh\n'); + fs.chmodSync(path.join(repo.work, `big/${long}-${k}.sh`), 0o755); + } + commit(repo, { 'big/data.json': '{}\n', 'small/data.json': '{}\n', 'lib/a.js': 'exports.a = 1;\n' }); + const text = plan(repo).out; + expect(text).toMatch(new RegExp(`REVIEW_EXCLUDE: patterns=1/2 .* skipped=big/\\*\\*:kept=140:bytes=\\d+ \\(kept files over ${KEEP_ARG_MAX_BYTES} argument bytes: pattern not applied\\)`)); + const cmd = text.split('\n').find(l => l.startsWith('DIFF_CMD: '))!; + expect(cmd).toContain("':(top,exclude,glob)small/**'"); + expect(cmd).not.toContain('big/**'); + expect(text).not.toContain('DIFF_KEEP:'); + }, 120_000); +}); + +describe('the review ceiling lines', () => { + const d = { bytes: 3_500_000, byDir: [['results', 3_000_000], ['src', 500_000]] as Array<[string, number]> }; + // Value: protects=a REVIEW_DIFF_TOO_LARGE stop names the one fix that applies, with JSON a person can paste; fails_when=the + // fix ignores where the policy stands or prints a placeholder version; why_new=task 4; seam=tooLargeLine; no_claim=the bytes git reports + test('the fix branches on no key, head-only, a skipped pattern and still over, with pasteable JSON at the key version', () => { + const json = (line: string) => JSON.parse(line.match(/land (\{.*?\}) in /)![1]!); + const none = tooLargeLine('ctx', 'batched', d, { state: 'none', base: 'main', source: 'origin/main@abc' }).line; + expect(json(none)).toEqual({ review_exclude_paths: ['results/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }); + expect(none).toContain('then bring main into this branch and rerun once; if it is still over, split the PR'); + const head = tooLargeLine('ctx', 'batched', d, { state: 'head-only', base: 'main', source: 'origin/main@abc', headPatterns: ['results/**'] }).line; + expect(head).toContain('this branch declares review_exclude_paths, but only the copy on main applies'); + const skipped = tooLargeLine('ctx', 'batched', d, { state: 'active', base: 'main', source: 'origin/main@abc', record: { patterns: ['results/**'], skipped: [{ pattern: 'results/**', kept: 900, bytes: 40_000 }], excluded: 0 } }).line; + expect(skipped).toContain('pattern results/** was not applied: its 900 kept files need 40000 argument bytes'); + const over = tooLargeLine('ctx', 'batched', d, { state: 'active', base: 'main', source: 'origin/main@abc', floor: '1.300.0.0', record: { patterns: ['receipts/**'], skipped: [], excluded: 12 } }).line; + expect(over).toContain('still over after excluding 12 files'); + expect(json(over)).toEqual({ review_exclude_paths: ['receipts/**', 'results/**'], min_gstack_version: '1.300.0.0' }); + expect(over).toEndWith('nothing was written (REVIEW_DIFF_TOO_LARGE)'); + }); + + // Value: protects=a REVIEW_DIFF_UNREADABLE stop says timeout or git exit, the timeout, and keeps full stderr in a file with + // remote URLs redacted; fails_when=a credential-bearing remote URL reaches the line or the log, or the fix ignores the + // reason; why_new=task 5; seam=readSelection and unreadableLine; no_claim=a real 120-second timeout + test('a git exit and a timeout name their reason, the timeout and a redacted log file', () => { + const repo = census(); + const r = readSelection(repo.work, 'f'.repeat(40), []); + expect(r).toMatchObject({ ok: false, reason: 'git-exit', exit: 128 }); + if (r.ok) return; + const logDir = path.join(repo.root, 'run'); + const line = unreadableLine('ctx', 'specialist/security', { ...r, stderr: `fatal: unable to access ${['https://bot', 'tok_fixture'].join(':')}${'@'}example.invalid/o/r.git/\nmore\n` }, 'f'.repeat(40), 'main', logDir); + expect(line).toContain('reason=git-exit exit=128 timeout_ms=120000'); + expect(line).toContain('fix: git fetch origin main, then rerun once'); + expect(line).not.toContain('tok_fixture'); + const log = fs.readFileSync(path.join(logDir, 'diff-specialist_security.stderr'), 'utf8'); + expect(log).toContain(''); + expect(log).not.toContain('tok_fixture'); + const slow = unreadableLine('ctx', 'batched', { ok: false, reason: 'timeout', exit: null, stderr: '', timeoutMs: 120_000 }, 'f'.repeat(40), 'main', logDir); + expect(slow).toContain('reason=timeout exit=none timeout_ms=120000'); + expect(slow).toContain('rerun once unchanged; if it times out again, split the PR'); + }, 60_000); +}); + +describe('the receipt binds the selection the reviewers read', () => { + const receiptIn = (body: string) => extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; + const writeReceipt = (repo: FixtureRepo, body: string, extra: string[]) => + runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body, ...extra]); + // Value: protects=a receipt names the selection the rendered briefs read, a base merge that leaves the policy alone still + // carries, and a policy change on the base (adding or removing a pattern) needs a new review, not a write; fails_when=write + // records a digest the briefs never named, carry compares the base sha, or a removed exclusion clears by writing the old + // digest; why_new=tasks 2 and 7; seam=gstack-review-dispatch verify, gstack-ship-receipt write/carry, bindShippedReceipt; + // no_claim=the merge train's own refusal wording + test('verify names the briefs\' digest; write binds it; a neutral base merge carries; a policy edit refuses write and carry', () => { + const repo = census(); + const out = path.join(repo.root, 'pass'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', out, '--pass', 'p1', '--', '--gate-tagged', '0']).status).toBe(EXIT.ok); + expect(runBin(repo, 'gstack-review-dispatch', ['stage', '--manifest', path.join(out, 'pass-manifest.json'), '--stage', 'voices']).status).toBe(EXIT.ok); + const v = runBin(repo, 'gstack-review-dispatch', ['verify', '--ledger', path.join(out, 'review-ledger.jsonl')]); + const digest = v.out.match(/^REVIEW_EXCLUDE_RECORD: digest=([0-9a-f]{64}) /m)?.[1]; + expect(digest).toBe(JSON.parse(plan(repo, ['--json']).out).review_exclude.digest); + const body = path.join(repo.root, 'body.md'); + const missing = writeReceipt(repo, body, []); + expect(missing.status).toBe(EXIT.fail); + expect(missing.err).toContain(`field=review_exclude expected=${digest} recorded=none source=origin/main@`); + expect(missing.err).toContain('gstack-review-dispatch verify --ledger '); + expect(fs.existsSync(body)).toBe(false); + expect(writeReceipt(repo, body, ['--review-exclude', digest!]).status).toBe(EXIT.ok); + expect(receiptIn(body).review_exclude).toMatchObject({ digest, excluded: EXCLUDED.length, kept: KEPT.length }); + + onBase(repo, { 'docs/other.md': '# other\n' }); + const carried = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(carried.status).toBe(EXIT.ok); + expect(receiptIn(body).review_exclude!.digest).toBe(digest!); + + onBase(repo, { '.gstack/ship-policy.json': policyText(['receipts/**', 'results/**']) }); + const refused = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(refused.status).toBe(EXIT.refused); + expect(refused.err).toContain(`review_exclude changed: recorded=${digest}`); + const head = repo.git('rev-parse', 'HEAD'); + const queue = bindShippedReceipt(repo.work, 'origin/main', fs.readFileSync(body, 'utf8').replace(/head: \S+/, `head: ${head}`), undefined, [head]); + expect(queue.ok).toBe(false); + + onBase(repo, { '.gstack/ship-policy.json': JSON.stringify({}) + '\n' }); + const removed = writeReceipt(repo, path.join(repo.root, 'body2.md'), ['--review-exclude', digest!]); + expect(removed.status).toBe(EXIT.fail); + expect(removed.err).toContain(`field=review_exclude expected=none recorded=${digest}`); + }, 240_000); +}); diff --git a/test/size-ledger/ship/gst-508-review-exclude-paths.json b/test/size-ledger/ship/gst-508-review-exclude-paths.json new file mode 100644 index 000000000..1e80a96c5 --- /dev/null +++ b/test/size-ledger/ship/gst-508-review-exclude-paths.json @@ -0,0 +1,4 @@ +{ + "bytes": 486, + "why": "GST-508: review_exclude_paths slots on the fixture and name-status commands, and the --review-exclude receipt step" +}