mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 10:40:32 +01:00
* fix(retro): keep retro history in the state root, shared across worktrees (#3749) /retro saved snapshots under .context/retros inside the working tree, so a run in a linked worktree saw none of the main checkout's history, reported "First retro recorded", and wrote a snapshot that git worktree remove deletes. gstack-retro-metrics --history [--next <date>] now resolves the history directory as <state root>/projects/<slug>/retros (the slug is keyed on the git common dir, so every worktree shares it), moves any legacy .context/retros from this worktree and the main checkout into it, lists prior snapshots and prints the next snapshot path. Step 12/13 and the gbrain prior-retros query use it. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(gate-diff): read a bare base as origin/<base> in gate-diff and diff-scope (#3650) gstack-gate-diff and gstack-diff-scope resolved a bare base such as main to the local branch whenever it shared history with HEAD. A local main that is behind origin/main (a linked worktree, or a Capy clone, where local main never moves) then set the merge base, so /ship's gate scan and specialist scope also read commits already upstream. Both now prefer origin/<base> when it resolves, the base /ship fetches and merges against. Fixes #3650 Co-authored-by: exGeni <135786749+exGeni@users.noreply.github.com> Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(retro): make history migration safe against symlinks and concurrent runs (#3749 review) Review pass 1 findings: a .context/retros that resolves outside its worktree (or to the shared history itself) is left alone, so migration can no longer delete the history it points at; moves are no-clobber with retry; a copy identical to any same-date snapshot collapses; RETRO_FILE is reserved as {} so overlapping runs in two worktrees never share a path; --next must be a YYYY-MM-DD date; a missing option value no longer loops forever. Step 12 names RETRO_HISTORY_ERROR instead of reading it as a stale helper. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): fragment for the #3749 retro history fix Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (diff-scope reads slash-named bases through origin too) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(retro): trim Step 12/13 prose back under the parity skeleton cap Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: Step 11 review fixes (a fork branch, a colliding tag or another remote never replaces the base) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(retro): keep today="<today>" in Step 12 so the judge excerpt keeps both date inputs Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): describe the fork, tag and remote guards (#3650) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(retro): compare #3749 history paths with forward slashes so the Windows shard passes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review): review-plan and gate-diff plan the same with a stale local main or a shadowing tag (GST-883, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): gate-diff and review-plan resolve a branch base to origin/<base> by full ref name; review-plan scans the merge base its own diff used (GST-883) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): GST-883 fragment Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review-risk): hostile-input time bounds for every risk pattern; the R51 rm and rmSync rules fail (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review-risk): the R51 rm and rmSync rules run in linear time (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review-packets-removed): repin lib/review-risk.ts on the GST-898 verdict-identity replay Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review-risk): padding cannot push recursive: true out of the rmSync window; repin on the GST-898 no-loss replay Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): GST-898 fragment wording Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review): planner misreads: SVG chart as runtime, session data as auth, bare base in diff-scope and receipt depth (GST-911, GST-884, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): a drawing-only SVG is data and a scripted one is security; session data files are not auth; diff-scope and the receipt depth read origin/<base> by full ref name (GST-911, GST-884) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): the SVG drawing check refuses CSS escapes and reads zero-padded character references; the security class reads a file's changed SVG lines joined (GST-911 review pass 1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review): a stray quote cannot hide an external href in a drawing SVG (GST-911 review pass 2, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): the SVG drawing check reads every name= without consuming its value, so a stray quote cannot hide an external href (GST-911 review pass 2) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review): CSS in a drawing SVG holds only declarations and url(#id): image-set(), image(), element() keep it code (GST-911 Step 11, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): a drawing SVG's CSS holds declarations only, no @ rule and no function but url(#id), which closes image-set(), image(), element() and every other CSS load (GST-911 Step 11) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review): a style element's CSS is read past comments and CDATA, unclosed style tags are refused in linear time, and every drawing verdict holds at 1 MB and 8 MB (GST-911, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): walk a style element's CSS with indexOf, dropping comments and reading CDATA, so no hidden </style> ends it early and unclosed style tags refuse in linear time (GST-911) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(review-policy): the linear-time claim covers R51's rm and rmSync rules (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(review-policy): state the rmSync padding rule exactly (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review-depth): linear url() refusal in svgDrawing (wave review-2, red-team CRITICAL) SVG_REFUSED's url() entry backtracked quadratically on url( followed by a long whitespace run (80k spaces: 820 ms; 1 MB: over 230 s), and svgDrawing runs in collectLive and in collectCommit, which ship-queue ready, train build and land call with no timeout. The rewritten entry refuses exactly what #3838's pattern refused. Repin lib/review-depth.ts under no-loss replay GST-911-redos (verdict identity: 60,000 seeded strings, the edges and every SVG blob in history, zero differences). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (wave review-2, pass 1) - bin/gstack-diff-scope: _resolve_base takes a related local <base> before another remote's twin, the order resolveBase uses, so the two tools pick the same base for a bare name when origin/<base> is unrelated. - lib/review-depth.ts: a drawing's DOCTYPE names no document but W3C's SVG DTD; SYSTEM and other PUBLIC identifiers keep the SVG code. Repin lib/review-depth.ts under no-loss replay GST-911-redos (the DOCTYPE change only keeps more SVGs code, so it drops no reviewer). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: freeze the exported SVG_REFUSED list (wave review-2, Step 11) Exported only so test/review-risk-redos.test.ts can reach the url() entry; a frozen readonly copy keeps any importer from changing verdicts while DEPTH_RULES still reports the original rules. Repin lib/review-depth.ts under no-loss replay GST-911-redos (no pattern changed). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Revert "fix: freeze the exported SVG_REFUSED list (wave review-2, Step 11)" The fix-check (wave review-2) showed Object.freeze is shallow: each RegExp can still be changed in place, so the freeze did not close adversarial-native#1, and it moved the tree past the one the Step 11 voices reviewed. The finding ships as an open note (its reviewer rated it Low, not blocking; no production caller mutates the list). Restores the lib/review-depth.ts pin under no-loss replay GST-911-redos. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): wave review-2 entry for the integrator fixes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(review-policy): a drawing's DOCTYPE names no outside document (wave review-2 docs audit) 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> Co-authored-by: exGeni <135786749+exGeni@users.noreply.github.com>
631 lines
48 KiB
Markdown
631 lines
48 KiB
Markdown
# Review policy: how gstack's reviews converge
|
|
|
|
This is the canonical statement of what a gstack review may block on, how the
|
|
`/ship` review loop converges, and what the ship receipt records about it. The
|
|
skills (`/review`, `/ship`) and the tools (`gstack-review-scope`,
|
|
`gstack-ship-receipt`) implement it; a team's own rules should point here
|
|
instead of restating it. A one-line crew rule is enough:
|
|
|
|
> Review convergence: gstack review policy (docs/review-policy.md), scope knob default.
|
|
|
|
## The blocking floor
|
|
|
|
A finding is **blocking** when it is a CRITICAL defect: it would fail a test,
|
|
break production, or lose data or secrets. Everything else is below the floor:
|
|
an advisory, an informational item, a test finding (category `testing`: a gap,
|
|
a weak or flaky test), a wording or naming note, a style or simplification
|
|
suggestion. Below-floor findings are reported, never fixed after pass 1 of a
|
|
ship, and never earn another review pass.
|
|
|
|
Three words cover every finding, and the loop budgets by them: a **blocker** is
|
|
the intended case gone wrong (the floor above); a **should** is a real bug a
|
|
caller will hit inside the intended case (fixed when it is in the diff on pass
|
|
1, deferred when it is not); a **nit** is the rest (a named note). Two stop
|
|
rules follow: if a pass's fixes grew the diff past what the request needs, cut
|
|
that growth first; and the cap approves nothing, so a standing blocker is
|
|
reported, never waved through.
|
|
|
|
Before a merged finding counts, the parent applies lead judgment
|
|
(`review/sections/review-army.md`, `ship/sections/review-army.md`, stage 4 of
|
|
the merge): a reviewer whose whole return is nits found nothing; a defect names
|
|
the call site or input that reaches it, or it is a note; structure for a case
|
|
nothing takes is advisory; "I would have done it differently" is not a finding;
|
|
a finding that assumes a rule the repository documents otherwise is dropped with
|
|
the pointer. Independent agreement between reviewers is the strongest signal.
|
|
|
|
Every reviewer prompt gstack dispatches (the core checklist, each specialist,
|
|
the Red Team, the outside voice) carries this definition: CRITICAL means the
|
|
floor above; everything else is INFORMATIONAL. Data loss, a trust-boundary
|
|
bypass and uncapped spend stay forced CRITICAL (the sideways sweep rule); no
|
|
lane, specialist, dedup or saved decision downgrades them.
|
|
|
|
<a id="rulings-below-the-floor"></a>
|
|
### Rulings below the floor
|
|
|
|
The floor is a test, and these classes have been put to it, each more than
|
|
once, and failed it (#3485 from the /ship of #3484, #3504 from the /ship of
|
|
#3487). A finding in one of them is a **note** from the first pass on, named
|
|
by its ruling in the PR body's `## Review notes`; it never earns a fix round
|
|
or another review pass, and the lead judgment stage drops it from the defect
|
|
count with the pointer. The floor itself does not move, so a finding that also
|
|
loses data, crosses a trust boundary or spends without a cap stays CRITICAL
|
|
whatever class it resembles.
|
|
|
|
| Ruling | What it looks like | Why it is below the floor |
|
|
|---|---|---|
|
|
| **own-artifact hardening** | A check missing between two things the repository's own tooling writes: a screen verdict not compared with the receipt it belongs to; a receipt schema that does not declare a key the writer records; a policy loader that defaults when the base branch's own file is invalid; a size read that follows a symlink inside the repository. | The only writer is the repository's maintainers through its tools; no caller inside the intended case reaches the gap, and no test fails. A writer outside that trust boundary makes it a bypass, and CRITICAL. |
|
|
| **unwired or unread** | A helper with no caller, a field written and never read, a duplicated helper, a header comment naming a step that does not run it, a key-version constant that moved before any release carried the old value. | Nothing takes the path, so nothing breaks; it is dead code to delete or wire in its own change. |
|
|
| **bounded repeat work** | A re-dispatch on every call, a redundant screen after main moves, an orphaned scratch branch, a refusal that could come one step earlier in the sequence, a settle that could wait one more check. | The operation completes, nothing is lost, and the cost is one more bounded run; uncapped spend is the sideways sweep's CRITICAL, not this class. |
|
|
| **conservative refusal** | Stale state scoped wider than its cause: a RED verdict that keeps refusing a head after main moved, a pending result that keeps refusing after it settled. | A refusal costs a rerun and never passes something wrong; a wrong pass would be the floor. |
|
|
| **artifact presentation** | A string from an artifact printed verbatim into a parsed line (whitespace, control characters); a classifier that reads a non-zero exit code beside a non-RED verdict as RED. | Display and classification of the repository's own output; it forges no parsed field. |
|
|
| **test gap** | A state-matrix cell without a test, a coverage percentage, a weak or flaky test, a fixture that shares the coordinator's environment. | Category `testing`, below the floor by definition above; a gap is work for the test audit, not a defect in the diff. Precedence for state machines: the specialist still reports each untested cell CRITICAL so `gstack-state-matrix check` proves the matrix complete, and `gstack-state-matrix findings` maps it to INFORMATIONAL per matrix cell before the merge; a wrong row or collapsed kinds stays CRITICAL, and a cell with a second finding keeps both. |
|
|
|
|
A reviewer that wants a class promoted argues it against the floor with a
|
|
reaching input, in the finding, and the owner rules; the table records the
|
|
rulings, it does not close them.
|
|
|
|
## `NO FINDINGS` is a result
|
|
|
|
A reviewer that finds nothing answers `NO FINDINGS` and nothing else. That is a
|
|
completed, clean result, never a failed review and never a reason to invent an
|
|
item to show work. Missing or unusable reviewer output is the opposite:
|
|
incomplete coverage, which blocks until the reviewer is rerun.
|
|
|
|
## How `/ship` converges (`review_scope: frozen`, the default)
|
|
|
|
1. **Pass 1 reviews the whole diff** against the merge base. Every finding goes
|
|
through `gstack-review-scope classify`, which stamps it with `locus` (`diff`:
|
|
a hunk the PR changed, with three lines of context; `outside`: anything else),
|
|
`kind`, `blocking` and `scope`. Findings in the diff are `fix`. A blocking
|
|
finding in code the PR did not change is `defer`: `gstack-review-scope defer`
|
|
files it as a GitHub issue (one per finding, reused across reruns through an
|
|
`<!-- gstack-deferred-finding: <fingerprint> -->` marker; the issue text is
|
|
scanned like the PR body before anything is posted, so a finding that quotes a
|
|
secret is `unfiled (publication scan refused: …)`; `unfiled (<reason>)` when
|
|
`gh` is unavailable or its search fails; every unfiled case still exits 0)
|
|
and the PR body lists it under `## Deferred findings`. A finding's `cause`
|
|
(the changed line behind a defect reported elsewhere, a deleted file included)
|
|
places it in the diff.
|
|
Below-floor findings outside the diff are `note`, listed under `## Review notes`.
|
|
2. **What comes next is the transition table below**, not "any fix earns
|
|
another pass". A full pass after pass 1 reviews only the range since the
|
|
previous pass: each pass saves its head, the next diffs `RANGE_BASE..HEAD`
|
|
for every reviewer (the gate scan and QA probes keep the whole candidate) and
|
|
classifies with `--range-base`. Only a blocking finding in that range or in
|
|
the diff is `fix`; every below-floor finding is a `note`.
|
|
3. **Two rounds is the cap** (R51): a full pass, a scoped pass
|
|
([below](#scoped-pass)) or a fix-check is a round. After round 2 the run
|
|
finishes with its open notes listed in the PR body and receipt, or stops for
|
|
the owner when a blocking finding is open, or was fixed in round 2 and is
|
|
unverified, or a round-2 fix edited a gate-class path, naming it (path,
|
|
severity, problem). A third round is the owner's
|
|
grant of one scoped fix-check (recorded in `review_loop_exception`), never the
|
|
loop's. The cap approves nothing.
|
|
|
|
<a id="transitions"></a>
|
|
### Transitions
|
|
|
|
`gstack-review-plan` (`lib/review-depth.ts` `nextStep`) owns this table, and
|
|
/ship asks it after every step:
|
|
|
|
| The step that just finished | Next |
|
|
|---|---|
|
|
| none | full pass 1 on the whole diff |
|
|
| a full pass with no edits | done |
|
|
| a full pass that fixed a blocking finding, below the cap, on a diff below full depth with a fix range of at most 150 runtime lines | scoped pass on the range |
|
|
| a full pass that fixed a blocking finding, below the cap (full depth, a larger range, or a range or depth it could not read) | full pass on the range |
|
|
| a full pass whose fix touched a gate-class path, below the cap | full pass on the range |
|
|
| a full pass that fixed only below-floor findings | one fix-check (round 2) |
|
|
| a light pass 1 whose below-floor fixes change no runtime lines | done |
|
|
| round 2 (a full pass or a fix-check) that fixed a blocking finding or edited a gate-class path | stop: name it for the owner, who may grant one scoped fix-check |
|
|
| round 2 (a full pass or a fix-check) that fixed only below-floor findings | done: notes in the PR body |
|
|
| a full pass that leaves a blocking finding open (fixed or not) | stop: name it for the owner |
|
|
| a fix-check that found nothing blocking | done |
|
|
| a fix-check that found a blocking finding | stop: name it for the owner |
|
|
|
|
A **fix-check** is one reviewer reading the touched hunks, the fix list, the
|
|
test results and the callers of every changed symbol, at full effort. It is
|
|
logged as a `review-fix-check` row, so it never counts as a pass. No army, no
|
|
Red Team and no outside voice run on it.
|
|
|
|
<a id="scoped-pass"></a>
|
|
A **scoped pass** is pass 2 after a blocking fix when pass 1 and the diff both
|
|
ran below full depth (no gate class, no depth error, no `--depth-floor full`).
|
|
A full pass 1 always gets a full pass 2: `--pass1-depth` carries pass 1's
|
|
`REVIEW_DEPTH`, and an absent flag counts as full. The fix range
|
|
`RANGE_BASE..HEAD` must have at most `SCOPED_RANGE_LINES` (150) runtime lines, counted
|
|
as the depth counts them (`gstack-review-plan --range-base $PASS_HEAD` prints
|
|
`FIX_RANGE:`). It dispatches one fix-check on the range, the Red Team on the
|
|
range, the native adversarial pass when the range changes runtime lines, every
|
|
force-included specialist and the outside voice when published numbers changed.
|
|
Step 11's voices that have not run yet stay listed, as on a full pass 2. It
|
|
writes a full-pass `review` row with `"scope":"scoped"`, so it counts toward
|
|
the cap, and a blocker it leaves open stops for the owner as before. A fix that
|
|
touched a gate-class path (read from the range with its own scope and gate
|
|
scan), a larger range, an empty range, or a
|
|
range, classifier or depth it cannot read keeps the full pass 2. The 150-line figure is a starting value from
|
|
a 24-PR backtest (GSTA-152 D6), to revisit once classified findings carry
|
|
`raised_by`.
|
|
|
|
The same floor and the same `NO FINDINGS` rule apply to `/review` on its own;
|
|
its re-review loop keeps its three-cycle fix cap (`CYCLES`), and a pass that
|
|
applies no edits is its exit too.
|
|
|
|
<a id="review-depth"></a>
|
|
## Review depth by diff class
|
|
|
|
`gstack-review-plan --base <base>` classifies the diff, picks a **depth** and prints
|
|
the reviewers the next step dispatches: `REVIEW_CLASS:`, `REVIEW_DEPTH: <d>
|
|
runtime_lines=<n> rules=<hash> reasons=<…> codes=<…>`, `GATE_SCAN: tagged=<n>|unavailable
|
|
head=<sha>`, one `DEPTH_ERROR:` per input that failed closed, one `DISPATCH:` per reviewer
|
|
with its effort and a shell-quoted `DIFF_CMD:` (plus `DIFF_EXCLUDE: <name> none` when it
|
|
reads everything), one `SKIP:` per reviewer left out, and `--json` with the same fields.
|
|
A plan that fails closed to full exits 0; a diff git cannot list exits 1.
|
|
|
|
**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
|
|
**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).
|
|
|
|
**Runtime lines** are the changed lines of code-class paths, minus outputs the base
|
|
commit's `.gstack/generated.json` registers. Docs, tests and release metadata are never
|
|
runtime; a `.tmpl` edit plus its regenerated `SKILL.md` counts the template once.
|
|
|
|
**Data files no code names are docs.** A changed or deleted JSON, JSONL, NDJSON, CSV,
|
|
TSV or raster image (PNG, JPEG, GIF, WebP, BMP, ICO, AVIF) counts as docs when no
|
|
non-docs file names its basename at the head, at the merge base or, for live /ship, in
|
|
the working tree with untracked files; a data file named only by another data file
|
|
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
|
|
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
|
|
function but a same-file `url(#id)`, which closes `@import`, `image-set()`,
|
|
`image()`, `element()` and every other CSS load), no backslash escape, every
|
|
`href` a same-file `#` reference or an inline raster image, no event handler and no
|
|
entity declaration, and no DOCTYPE but a bare `<!DOCTYPE svg>` or W3C's own SVG DTD, read after character references are decoded; anything else, a
|
|
script or `foreignObject` included, keeps it code. More than
|
|
2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed`
|
|
and fails the depth closed. Only `classifyDiff` applies the rule (`lib/review-collect.ts`
|
|
finds the consumers, `gstack-ship-preflight`'s `RUNTIME_LINES` reads the same
|
|
function); every path-only caller (the receipt's gate-scan decision, the Step 16
|
|
post-review route, /autoplan's docs-only scope) keeps data files code. A numeric claim
|
|
in a data file still schedules the outside voice.
|
|
|
|
| Depth | When | Pass 1 dispatches |
|
|
|---|---|---|
|
|
| **light**, runtime 0 | docs, tests, eval write-ups, or only registered outputs | one reader: the core checklist; the outside voice instead reads a diff that publishes numbers |
|
|
| **light**, runtime above 0 | code with no risk class, at any size (R51: size never raises the depth) | core, one **batched** specialist (the batched checklists below plus the Red Team prompt in one context), the native adversarial pass (kept: backtest inconclusive, GSTA-147 D5) and the outside voice on numbers |
|
|
| **standard** | `--depth-floor standard`, or a light diff raised by a fixed blocker or a CRITICAL | one batched specialist at `medium` and the Red Team at 200 runtime lines (or on a CRITICAL or a raise); the batched reader leaves docs and registered outputs out of its diff; adversarial and outside voice read everything |
|
|
| **full** | gate class, a [risk class](#risk-classes), or any classifier error | one batched specialist on the whole diff, the Red Team always, no hit-rate gating |
|
|
|
|
<a id="risk-classes"></a>
|
|
**Risk classes (R51).** `lib/review-risk.ts` reads a code diff's paths and its
|
|
added and deleted lines; any class makes the depth full (`risk:<class>` in
|
|
`reasons=`, `review_depth.risk` in the receipt). money, auth and secret are the
|
|
[targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code)'s matchers.
|
|
**money** also reads metering and charge words in a path or on a line (meter,
|
|
metering, metered, charge, charged, billable, billed, credits) and three
|
|
pairs: usage beside a token or cost word, proxy beside billing, and a client
|
|
beside a cost or charge word, so a CLI's usage text and a plain proxy stay out
|
|
(GST-850, from gbrain-evals #289, whose metering proxy and billing clients read
|
|
`required=none`). **security** is a trust-boundary path (trust, redact, sanitize, neutralize,
|
|
consent, escape) or line (`shell: true`, `exec(` and `execSync(`, `eval`, `new Function(`,
|
|
raw HTML sinks, a shell run through argv, recursive deletes), any call to an
|
|
outbound HTTP client (`fetch`, `http(s).get` or `.request`, axios, got, undici,
|
|
`requests`, `httpx`, urllib, with no exemption for a literal URL), `new URL` from
|
|
input, redirect toggles, proxy settings and host allowlists, archive extraction
|
|
and path traversal (`tar.x`, adm-zip, yauzl, extract-zip, `extractall`, `tar -x`,
|
|
`unzip`, `path.join` or `resolve` on request or entry input, a `'..'` check added
|
|
or removed), and any changed line that names the `child_process` or
|
|
`node:child_process` module, in any import or require spelling. A changed SVG
|
|
line that can run code (a script or `foreignObject` element, an event handler, a
|
|
`javascript:` URL) is security too (GST-911). The GST-850
|
|
rules are coarse on purpose and parse no import clause or call arguments;
|
|
`test/review-risk-linear.test.ts` holds each of them under 100 ms on
|
|
200,000-character hostile lines, its anchor followed by a long run of the
|
|
characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` call with more than 500 characters before the first `)` after it counts as security on its own. **data** is user data: a path naming pii, gdpr, ccpa, privacy,
|
|
personal, account, customer, cookie, persistence, storage or database, or a line
|
|
that drops a table or column, deletes rows or bulk-deletes records. Auth scope,
|
|
migrations and deleted code paths were already gate class. `RISK_RULES` hashes
|
|
the patterns and `DEPTH_RULES` includes it.
|
|
|
|
**UI text** stays code (light unless a risk class applies). A docs path for UI
|
|
copy and locale values was built as an allowlist in GST-850 and dropped for
|
|
review cost: its first review round found a new bypass class in each version,
|
|
the same pattern as three rounds of R51. Chart SVGs use the drawing allowlist
|
|
above, which reads the whole file rather than changed lines. When a diff
|
|
publishes numbers, the batched specialist's diff keeps the write-up's text files
|
|
instead of leaving docs out.
|
|
|
|
**Batched checklists (the lean ensemble, GSTA-167 part 2).** One specialist
|
|
subagent reads Testing and Security on every code or gate diff, plus
|
|
State-machines, Data-migration, API-contract and Design when scope asks (a
|
|
forced checklist always joins). Maintainability, Performance and Simplification
|
|
are read only when forced (`SKIP:` names them). The Red Team reads the batched
|
|
reader's merged findings. A free re-score of saved C and D findings lost 0 of
|
|
the 29 blockers (handoffs/GSTA-167/lean-ensemble-rescore.md); the live round 5
|
|
replay passed (L∘G1 found 24 of 29 blockers against the control's 21,
|
|
handoffs/GSTA-167/noloss5-results.md).
|
|
|
|
A fixed blocking finding, a CRITICAL or an open blocker raises a light diff to
|
|
standard for the next step (`step=standard`): its specialists read at least 50 lines
|
|
and the Red Team runs, so a raise never dispatches less than light did. When the
|
|
next step is a scoped pass, the raise applies to the scoped set only. A fix-check
|
|
is the same at every depth.
|
|
|
|
**Fail closed.** Each of these makes the depth full and is listed in `errors[]` with
|
|
the input it could not read, a sanitized cause and the repair: a `gstack-diff-scope`
|
|
error, a gate scan that is unavailable, a base registry that cannot be read or is
|
|
invalid (`REGEN_REGISTRY_INVALID` or `REGEN_CONTAINMENT` in `codes=`), a numstat git
|
|
could not produce, and a binary code-class path (it is never counted as 0 lines).
|
|
|
|
**The registry rule.** Only the base commit's registry counts (the merge base's
|
|
`origin/<base>` commit for live /ship and the current base ref's commit for the queue: `origin/<base>` in `ready`, `<remote>/<base>` in `train build`), and only
|
|
entries with a `command`, which pregate's regen check proves for that tree. An entry
|
|
added on the branch never excludes anything; `command: null` entries, vendored code
|
|
and lockfiles stay runtime. To adopt it, commit an entry like this on the base branch:
|
|
|
|
```json
|
|
{
|
|
"$schema": "https://github.com/garrytan/gstack/docs/pregate.md#generated-v1",
|
|
"entries": [
|
|
{ "id": "skill-docs", "command": "bun run gen:skill-docs --host all", "outputs": ["*/SKILL.md", "*/sections/*.md"], "inputs": ["*/SKILL.md.tmpl", "*/sections/*.tmpl"] }
|
|
]
|
|
}
|
|
```
|
|
|
|
**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`,
|
|
`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,
|
|
not file bytes, so a comment edit keeps it.
|
|
|
|
**`--depth-floor standard|full`** (a /ship prompt flag, passed to every plan run)
|
|
raises the depth and never lowers it.
|
|
|
|
**Numbers keep the outside voice.** At light depth the outside voice runs only when
|
|
the diff publishes numeric claims: `gstack-review-plan` scans the added lines of
|
|
prose and data files for percentages, money, multipliers, durations, token or credit
|
|
counts and score, rate or speed claims (`NUMBERS: yes`). The claims' fingerprint
|
|
(`fingerprint=`) is passed back as `--reviewed-numbers`; when a later fix changes a
|
|
claim, only the outside voice reruns, on the numbers. It always reads the whole diff.
|
|
|
|
**The adversarial and outside voices run once per invocation** (Step 11, on the
|
|
final tree), as the last full-pass plan lists them; after they ran, decision 2
|
|
passes `--adversarial-ran` so a repeat lists them no more. Later full passes rerun the
|
|
specialists on the range only. Step 11.5 binds a planned skip through the Step 9
|
|
record's `CURRENT` freshness instead of a native record.
|
|
|
|
**Effort.** The core checklist, the Red Team, the native adversarial pass and
|
|
fix-checks run at `full` effort (the lane's reasoning); specialists run at
|
|
`medium` (GSTA-167 part 3: the no-loss controls ran them there), except light's
|
|
batched specialist, which carries the Red Team prompt and stays `full`. The
|
|
outside voice and the native adversarial pass carry the floor sentence and end
|
|
each finding with `Severity:` and `Blocks: yes|no`; a finding is CRITICAL when
|
|
its label rule says so (native FIXABLE, outside Critical or High) or it says
|
|
`Blocks: yes`, and `Blocks: no` never lowers one (`gstack-review-dispatch collect`). Work that cannot block (the Step 7 coverage audit, the Step 8
|
|
plan-completion audit, the Step 14.5 docs audit, decision-trail reviewers,
|
|
watchers) runs at `light`: the same model at a lower reasoning effort, never a
|
|
cheaper model family. The outside voice keeps its own provider and effort
|
|
(`codex_effort`). A host that cannot set a subagent's effort runs it at its
|
|
default. `gstack-config set review_effort_tiers off` (or `--no-tiers`) runs
|
|
the specialists, audits and watchers at the host default too
|
|
([model-policy.md](model-policy.md#reasoning-effort)).
|
|
|
|
### Behavior changes
|
|
|
|
- Pass 2 after a blocking fix on a diff below full depth, with a fix range of at
|
|
most 150 runtime lines, is a [scoped pass](#scoped-pass): a fix-check and the Red
|
|
Team on the range, plus the native adversarial pass (on the range once Step 11 has
|
|
run, otherwise on the whole branch), instead of the specialist army.
|
|
- A tests-only diff (runtime 0) no longer dispatches the Testing specialist: the
|
|
core checklist and the gate scan review it.
|
|
- Every depth dispatches one batched specialist instead of one reader per
|
|
checklist; Maintainability, Performance and Simplification run only when forced.
|
|
- Light no longer runs a separate Red Team: the batched specialist carries its
|
|
prompt. At runtime 0 light runs no adversarial pass either.
|
|
- Standard reads thresholds on runtime lines, not the whole diff, so a test-heavy
|
|
diff with few runtime lines dispatches fewer specialists.
|
|
- R51: size never raises the depth; non-risky code is light at any size, and a risk
|
|
class makes it full. Two rounds is the cap, a fix-check
|
|
included: a blocker fixed in round 2 stops for the owner instead of earning a
|
|
fix-check, and a light pass 1 whose below-floor fixes change no runtime lines is done.
|
|
- A JSON, CSV, TSV or raster data file no code names is docs, not runtime (GSTA-167
|
|
part 4a). A binary image no code names is no longer a `binary_code_path` error
|
|
that forces full depth; one that code names still is.
|
|
|
|
## Targeted pass on money, gate, auth and secret code
|
|
|
|
Some code is costly to get wrong: code that spends money, code that decides
|
|
whether a check passes, code that decides who gets in, and code that handles
|
|
secrets. When a diff touches one of these, `gstack-review-plan` dispatches one
|
|
more reader, `targeted`, at every depth, light included. It reads the whole
|
|
diff, fixtures and tests included, with the blocks of
|
|
`review/specialists/targeted.md` for the classes that hit, and it runs in Step
|
|
11 beside the native adversarial pass and the outside voice (GST-257).
|
|
|
|
`lib/review-targeted.ts` decides the classes from added and deleted lines, so
|
|
removing a cap, a check or a redaction counts. Docs and tests paths never count,
|
|
and matching is token-bounded: `costume` never reads as cost.
|
|
|
|
| Class | Signal |
|
|
|---|---|
|
|
| money | a path segment naming cost, spend, budget, ledger, billing, price, fee or invoice; or a changed line with an identifier ending in `_usd` or `Usd`, any `spend_cap` spelling, or a call to a paid provider `lib/pricing.ts` prices |
|
|
| gate | a hunk the gate scan tagged: a relaxed threshold, a skipped test, a loosened check |
|
|
| auth | `SCOPE_AUTH` from `gstack-diff-scope`, on a code path with an auth, session, jwt, permission or role token in its name; a data file named for sessions (`recovered-sessions.jsonl`) outside a config location does not set `SCOPE_AUTH` |
|
|
| secret | a secrets, credentials, keychain or token path, or a changed line that reads a key-named environment variable (`OPENAI_API_KEY`, `GH_TOKEN`) or a credential store |
|
|
|
|
An input the classifier cannot read (the diff, `gstack-diff-scope`, the gate
|
|
scan) fails closed to all four classes. `TARGETED_RULES` hashes the patterns.
|
|
The classifier and the brief are gate paths, so an edit that weakens them gets
|
|
full review.
|
|
|
|
The plan prints `TARGETED: classes=<money,gate,auth,secret|none> rules=<hash>
|
|
brief=<sha256 of the brief> targeted_patch_id=<id>` and, when a class hit,
|
|
`DISPATCH: targeted targeted effort=full`. The targeted reader always reads the whole diff: `render --role targeted`
|
|
refuses a `--diff-base` other than the merge base, so on pass 2 and later it
|
|
gets the merge base, not `RANGE_BASE`. After a fix, pass
|
|
`--targeted-ran <targeted_patch_id>` from the line the reader ran on: the same
|
|
committed patch skips the reader, and a changed one, uncommitted edits or
|
|
`none` dispatch it again. `targeted_patch_id` is the patch-id of the whole
|
|
committed diff, tests included, so a test-only fix that adds a gate hunk
|
|
dispatches it again; it is not the receipt's `product_patch_id`.
|
|
|
|
The brief states the blocking floor in one sentence, and its money block cites
|
|
the checklist's Forced-CRITICAL rule on uncapped spend, so a money finding can
|
|
block without new floor words. The reader is read-only, and `NO FINDINGS` is a
|
|
result. `gstack-review-dispatch render --role targeted --classes <classes>`
|
|
renders its brief; `verify --targeted-out <file>` writes the record that
|
|
`gstack-ship-receipt write --review-targeted @<file>` puts in the receipt.
|
|
`gstack-ship-queue ready` and `train build` print
|
|
`REVIEW_TARGETED: required=<classes|none> receipt=<classes|none|legacy>
|
|
verdict=ok|missing` and refuse nothing yet; enforcement comes in a later
|
|
release.
|
|
|
|
Over the last 100 non-release commits on main (8663c3d2..8be7f213), money hit
|
|
21, gate 18, auth 3 and secret 8. The gate class first used the gate paths,
|
|
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 the receipt records
|
|
|
|
`gstack-review-scope summary --since <REVIEW_SINCE> --run <run dir>` reads this invocation's
|
|
review-log rows and the dispatch ledgers under the run directory (`--ledger <f>` names one;
|
|
`--no-ledger` when no reader ran through `gstack-review-dispatch`) and writes the counters `gstack-ship-receipt write --review-loop`
|
|
puts in the receipt:
|
|
|
|
| Field | Meaning |
|
|
|---|---|
|
|
| `review_loop.passes` | full review passes this ship ran (1 or 2; up to 3 under `review_scope: all`) |
|
|
| `review_loop.blocking_found` | every blocker found (GST-951): the rows' blocking findings (classified, or an unclassified CRITICAL), plus each ledger CRITICAL (as collected or judged) that no row finding from the same reviewer at the same path:line accounts for (each row finding pairs with one per ledger), so an adversarial, outside-voice or not-yet-classified blocker counts and one whose newest classification is below the floor does not. An unreadable ledger, merged file or judged set, or a judged set changed since `judge`, is `REVIEW_SCOPE_LEDGER`, never a count |
|
|
| `review_loop.unrecorded` | blockers found that no row gives a disposition (fixed, deferred, noted, skipped, a gate answer); absent when 0 |
|
|
| `review_loop.ledgers_read` | dispatch ledgers the count read; absent under `--no-ledger` |
|
|
| `review_loop.deferred` | blocking findings in unchanged code filed as issues (a Step 11 late defer, logged as a `review-late-defer` row so it never displaces the newest `review` pass, counts here and is not a pass) |
|
|
| `review_loop.noted` | below-floor findings listed in the PR body |
|
|
| `review_loop.fixed` | findings fixed in this PR |
|
|
| `review_loop.scope` | the `review_scope` the loop ran under |
|
|
| `review_loop.fix_checks` | fix-checks this ship ran (never passes) |
|
|
| `review_loop.rounds` | passes plus fix-checks (R51 caps it at 2; the queue's `REVIEW_DEPTH:` line prints it) |
|
|
| `review_loop.dispatches` | reviewer dispatches by role: `specialist`, `red-team`, `adversarial-native`, `outside-voice`, `fix-check`, `targeted` |
|
|
| `review_loop.est_input_tokens` | prompt plus diff bytes the dispatches read, divided by 4: a relative estimate for comparing runs, not a bill |
|
|
| `review_targeted` | the [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code): the `classes` it covered, `rules`, `brief_sha`, the receipt's own `product_patch_id`, `status` (`done` only with its output collected and the committed diff it was rendered on still the one at verify, else `failed`; absent when write got no record, which the queue reports as `legacy`), `findings` and `raised_by`; a carry keeps it while the product patch is unchanged |
|
|
|
|
Each review row may carry `dispatches: [{role, effort, prompt_bytes,
|
|
diff_bytes, status}]`, and each finding `raised_by` (the reviewer that raised it,
|
|
stamped by `gstack-review-scope classify`; `unknown` when untagged, absent on older rows); `summary` adds them up (older rows fall back to the
|
|
specialist stats they record). The summary line appends `fix_checks=`,
|
|
`dispatches=` and `est_input_tokens=` only when they were recorded, so an old
|
|
loop prints the old line.
|
|
|
|
Beside the counters, `write` records `review_depth` (the depth, runtime lines,
|
|
rules hash, base and registry commits, the gate scan bound to the head and the risk classes) and
|
|
the queue refuses a receipt below the depth its head needs
|
|
([ship-policy.md#ship-receipt](ship-policy.md#ship-receipt)). A stop the owner
|
|
accepted after a fix-check edit goes in `review_loop_exception`, never in prose
|
|
under `REVIEW_LOOP:`.
|
|
|
|
The receipt's summary line prints `passes=<n> blocking_found=<k> deferred=<m>`,
|
|
which is what a scorecard tracks (target: a median of 1 pass per PR). A
|
|
receipt carried across a base merge or a test-only fix
|
|
([ship-policy.md#receipt-carry](ship-policy.md#receipt-carry)) keeps these
|
|
counters, because the reviewed product diff is unchanged or its only
|
|
difference passed a scoped review.
|
|
|
|
## The scope knob
|
|
|
|
`.gstack/ship-policy.json` `review_scope` ([ship-policy.md#review-scope](ship-policy.md#review-scope)):
|
|
|
|
| Value | Pass 1 | Blocking finding in unchanged code | Pass 2+ | Cap |
|
|
|---|---|---|---|---|
|
|
| `frozen` (default) | the diff | filed as an issue, listed as deferred | the range since the previous pass, blocking only | 2 rounds (a fix-check is one) |
|
|
| `diff-plus-blocking` | the diff | fixed in this PR | the range since the previous pass, blocking only | 2 rounds (a fix-check is one) |
|
|
| `all` | everything | fixed in this PR | the whole diff, every finding | 3 fix cycles |
|
|
|
|
`diff-plus-blocking` is the fallback for a repository where deferred issues pile
|
|
up; `all` is the loop as it ran before the key existed. A value other than
|
|
`frozen` needs `min_gstack_version` at or past the release that added the key
|
|
(`REVIEW_SCOPE_KEY_VERSION` in `lib/ship-policy.ts`) so an older gstack refuses
|
|
the file by name instead of ignoring the key.
|
|
|
|
## Reviewer fan-out
|
|
|
|
The review army launches every selected specialist in one message so they run
|
|
in parallel; a host that caps running subagents makes that impossible past the
|
|
cap. In `/ship` the knob is a prompt flag, `--review-fanout auto|serial|parallel`
|
|
(default `auto`), read where the force flags (`--all-specialists`) are (`/review`
|
|
on its own keeps the single parallel batch: its generated SKILL.md sits at its
|
|
size pin, and a refused launch there is a failed reader, never dropped coverage):
|
|
|
|
| Value | Batches |
|
|
|---|---|
|
|
| `auto` | when the host reports fewer free subagent slots than selected specialists, batches of the free-slot count (at least one), each launched in one message, the next only after every reader of the previous one finished or is confirmed stopped; a host that reports no slot count runs one parallel batch |
|
|
| `serial` | batches of one |
|
|
| `parallel` | one batch regardless of slots; a refused launch is a failed reader, never silently dropped coverage |
|
|
|
|
Capy caps a thread at 8 running subagents, and waiting or idle ones hold a
|
|
slot, so under `auto` a nine-specialist review on an otherwise idle Capy
|
|
thread runs as one batch of 8 and one of 1 (`Fan-out: 9 specialists in 2
|
|
batches of 8`). Batching changes the order, never the set: every selected
|
|
specialist runs before the merge stage, and the Red Team's activation reads
|
|
the merged result as before.
|
|
|
|
## Two checklist rules with ids
|
|
|
|
Two findings `review/checklist.md` names by rule id so a PR body, a review log
|
|
and a later reader agree on what was flagged:
|
|
|
|
- `prereg-with-results` (Pass 1, Gate Integrity, ASK): one diff adds or changes
|
|
an eval's preregistration (bars, arms, decoys, stop rules, a plan file) and
|
|
also adds that eval's results, scores or verdict. The bar could have been set
|
|
after the outcome, so the pairing is the finding. The repair is a split: the
|
|
preregistration lands first and the results PR cites its merged commit.
|
|
- `test-keyboard-count-loop` (Pass 2, informational): a browser-driver test
|
|
reaches an element by a counted loop of key presses and asserts on whatever
|
|
has focus, so the count encodes today's DOM order. Locate the target by role,
|
|
label or test id; a counted loop stays only when the count is the behavior
|
|
under test.
|
|
|
|
<a id="porting-a-contributors-work"></a>
|
|
## Porting a contributor's work
|
|
|
|
A ported branch keeps the contributor's commits as theirs and adds the
|
|
porter's fixes on top, each crediting the original authors.
|
|
`gstack-coauthor-trailers [<range> | --base <ref>] [--message <file>]
|
|
[--exclude <email>]...` lists every distinct author of the range's commits
|
|
(default `origin/<base>..HEAD`) other than the checkout's own committer
|
|
identity, in first-appearance order, as `Co-authored-by: Name <email>` lines;
|
|
`--message <file>` adds the ones the commit message file lacks to its trailer
|
|
block (`git interpret-trailers`, pinned placement, so an existing co-author
|
|
keeps attribution), matched by
|
|
email case-folded, so running it twice changes nothing. The `COAUTHORS: <n> in
|
|
<range>` line on stderr is the receipt; `--json` prints `{ range, authors,
|
|
trailers, appended }`.
|
|
|
|
## Verbatim dispatch
|
|
|
|
A driven `/ship` (ship/DRIVER.md) never lets the model write a reviewer's brief
|
|
or merge a reviewer's findings. The no-loss replays showed why: with briefs the
|
|
parent filled in by hand, only 37 of 112 dispatched prompts matched the rendered
|
|
ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
|
|
|
|
1. `render --role <role> [--name <n>] --out <dir>` cuts the brief out of
|
|
`ship/sections/review-army.md` or `ship/sections/adversarial.md` (the fix-check
|
|
from `scripts/resolvers/fix-check.ts`) and fills only declared slots: the
|
|
literal diff base, the plan's `DIFF_CMD` excludes, stack, test framework,
|
|
learnings, checklist paths, the Red Team's merged findings and specialist count,
|
|
and the outside voice's branch diff. A section that lost an anchor or placeholder
|
|
refuses (`BRIEF_RENDER_FAILED`) instead of rendering something else, and so does
|
|
a slot the section leaves unfilled (every reader but fix-check and the outside voice).
|
|
That check runs before the findings and learnings go in, and they reach the reader
|
|
as written, so a finding that quotes `origin/<base>` neither fails the render nor
|
|
gets filled (GST-797).
|
|
2. The dispatch program passes that file's content unchanged as the reader's
|
|
prompt and runs `sent` on what it passed: a hash that differs from the rendered
|
|
one is `BRIEF_MISMATCH`, and that reader's review does not count.
|
|
3. `collect` hashes each reader's raw output and appends its findings to
|
|
`merged.jsonl` with the label as written (a JSON line's `severity`, or the most
|
|
severe of a prose line's Critical, High, Medium, Low, P0-P3, FIXABLE or INVESTIGATE
|
|
wherever they stand, headings and table rows included; a top-level list item under a
|
|
label-only heading takes the stronger of the two). A `Severity:` or `Blocks:` line
|
|
continues the finding above it until a list item, heading, bold paragraph opener or a
|
|
second line for the same field starts another: `Severity` raises its label,
|
|
`Blocks: yes` makes it CRITICAL and `Blocks: no` never lowers it; an inline
|
|
`Blocks: yes` is CRITICAL too, and with no finding above it is one. The native
|
|
adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). An unnumbered, unlabeled bullet with no file:line
|
|
under a paragraph finding whose Severity or Blocks line has not come yet is that finding's detail, so the field line after it
|
|
lands on the finding; a numbered item is its own finding.
|
|
A negated severity word still collects at its label ("found nothing CRITICAL", "No P0/P1 issues"), so a reader's
|
|
closing summary is a known phantom: it stays at that label in the judged set, lead judgment can fold it only into a
|
|
finding at least as severe, and it counts toward what blocks. GST-766 skipped negated labels and lost
|
|
real blockers named in an exception clause ("No CRITICAL issues except the token leak in log.ts:3"); GSTA-167
|
|
reverted that. Each prose finding's line also
|
|
records the Severity and Blocks its own text states as `stated`, beside the collected label and never in its place;
|
|
a classified finding that carries `stated` shows it in its deferred-review issue next to the label ("CRITICAL (the
|
|
reviewer wrote Severity: Low, Blocks: no)"). /ship's Step 9.4 does not copy `stated` into its findings file yet: that
|
|
instruction lives in a reviewer brief that changes only after a no-loss replay. What blocks is unchanged.
|
|
A phantom finding costs a fold; a lost one costs the gate.
|
|
A reply with no finding counts as a review only with the clean marker its own brief asks for: `NO FINDINGS` alone on a
|
|
line for the specialists, the batched specialist, the targeted reader, the Red Team and fix-check, the `Recommendation:`
|
|
line for the outside voice, either for the native adversarial voice. Without one (a refusal, an empty reply, a reader
|
|
that never saw the diff) collect records `review: false`, prints `COLLECT_NOT_REVIEWED` and exits 1.
|
|
4. Lead judgment writes `judged.jsonl`; `judge` refuses a set that drops a merged
|
|
finding, lowers its severity, folds it into a less severe duplicate, or adds or
|
|
promotes one without a reason (`JUDGMENT_REJECTED`). A merged file missing any id
|
|
the ledger collected is a drop. Each addition and promotion is logged with its reason.
|
|
5. `verify --ledger <f>` replays all of it from the run's ledger: rendered against
|
|
dispatched hashes per render (each render is one dispatch of its reader, so a
|
|
two-pass ledger checks pass-1 sends against pass-1 briefs; a later render prints as
|
|
`<reader>@2`), a collected output that is a review for every sent reader (`NOT_COLLECTED`
|
|
otherwise, and the output row `NOT_REVIEWED`; the marker is checked again in the output file when it is unchanged, so an
|
|
older ledger meets the same rule; a non-review the same dispatch later replaced with a review, by re-sending the brief, is `VOIDED`
|
|
and counted in `voided=N`). The outside voice may fail without blocking: `collect --unavailable '<reason>'` records its failure text
|
|
as missing coverage (`COLLECT_UNAVAILABLE`); an empty reason or an output that holds findings is refused (exit 2), so a partial
|
|
answer is collected normally and loses nothing. Verify prints its brief and output `UNAVAILABLE` and `unavailable=N` on the
|
|
verdict line, and its coverage is logged `unavailable` in the review-log row; any other reader is refused (exit 2), because its
|
|
missing coverage leaves the review incomplete. A mismatched send re-sent
|
|
byte-exact before anything was collected is void and counted (`voided=N` on the
|
|
verdict line); one that produced an output stays `MISMATCH`. Then output hashes
|
|
against the merged lines they produced, and the judgment rerun against the
|
|
recorded judged set, covering every collected id. A ledger from a run that kept no
|
|
prompts prints `VERIFY: no brief records` (exit 3), never clean.
|
|
6. `pass --base <b> --out <dir> --pass <id> -- <plan flags>` runs one pass from a
|
|
staged manifest (`<dir>/pass-manifest.json`): audit readers marked first, then
|
|
the specialists with the other audit readers, then the Red Team, then Step 11's
|
|
voices. A QA extra is refused before any reader is created: Step 9.2.1's probes
|
|
are the parent's. `--diff-base` sets every reader's base, a scoped pass's Red
|
|
Team included; the targeted reader keeps the merge base. Each reviewer keeps its plan entry and is rendered by the same code as
|
|
`render`, once its inputs exist; the Red Team's findings file is the specialists'
|
|
`merged.jsonl` lines byte for byte. A brief leaves the bin in indexed chunks of at
|
|
most 65,536 characters (`chunk`), so no tool output line carries a whole brief and a
|
|
host's line or output cap cannot cut one. Each reader's intent is recorded before its
|
|
subagent is created and its path right after (`mark`), so a resumed pass finds a
|
|
create whose result was lost by its title and never starts a reader twice; a
|
|
missing or doubled title stops the pass. On Capy the pass runs as one call to the
|
|
checked-in program `lib/capy-review-pass.js`, served by `program` and loaded
|
|
unchanged by the gstack Capy skill. A full repeat pass takes `--diff-base` (RANGE_BASE; a scoped pass, none), a
|
|
fix-check plan renders its reader from `--fix-list`, `--hunks`, `--tests` and `--callers`,
|
|
`--audit-only` runs just the `--extras` readers (Step 14.5's docs audit), and the fork
|
|
point is found against `origin/<base>` first, as preflight does.
|
|
7. Every brief, audit readers' included, opens with a fingerprint step and ends with a
|
|
workspace paragraph naming the checkout the bins run in (GST-482): a reader on a shared
|
|
machine starts in the machine's working directory, so it is told to `cd` there on every
|
|
command, to give every file tool an absolute path under it, and to start its reply with
|
|
the `REVIEWED_TREE: <HEAD^{tree}> <toplevel>` line the step's command outputs there. The
|
|
step comes first because a reader following a findings-only format skipped the request
|
|
when it only closed the brief (GST-897). The section's render sits between the two,
|
|
byte for byte. `collect` and `mark --state collected`
|
|
compare that line with the fingerprint recorded at render: a missing or foreign line is
|
|
`COLLECT_WRONG_TREE`, a later commit in the same checkout `COLLECT_TREE_MOVED`; nothing
|
|
is merged, the reader becomes `failed`, and `verify` reports `NOT_COLLECTED`. An
|
|
outside voice's tree failure is recorded `unavailable` only when its reply holds no finding. The line catches a reader that
|
|
worked elsewhere by accident; it is not proof of reading, since diff text can predict it. A `printed:` label
|
|
before the line (plain, bold or inside its backticks) is still read, and its tree and toplevel are still compared
|
|
exactly (GST-776). Capy cannot start a subagent in a chosen directory, so before a stage takes a start token or sends
|
|
its first reader, the Capy pass program compares the checkout with the one shared readers start in (the thread's
|
|
working directory) and stops when that is another checkout or cannot be read: run /ship in the thread's own checkout.
|
|
A start directory outside any checkout, such as a multi-repo workspace root, holds no tree to review by mistake and
|
|
dispatches. Step 11's voices take the `adversarial-review` start
|
|
token by default, and a `logStart` list without it stops before anything runs.
|
|
|
|
## Where this is enforced
|
|
|
|
- `lib/review-brief.ts`, `lib/review-ledger.ts`, `lib/review-pass.ts`, `lib/capy-review-pass.js`, `bin/gstack-review-dispatch`: [verbatim dispatch](#verbatim-dispatch) (`test/review-dispatch.test.ts`, `test/review-pass.test.ts`).
|
|
- `lib/ship-parts.ts`, `bin/gstack-ship-parts`: the section text each /ship phase uses, returned once per run inside the phase program before it; `next --resend` sends an unreceived batch again (`test/ship-parts.test.ts`).
|
|
|
|
- `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=<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-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`, `unrecorded`, `ledgers_read`); refusals are `REVIEW_SCOPE_INPUT`, `REVIEW_SCOPE_RANGE` and `REVIEW_SCOPE_LEDGER` ([troubleshooting.md](troubleshooting.md#review-scope-input)).
|
|
- `lib/ship-policy.ts`: the `review_scope` key and its `ship: review_scope …` consequence line.
|
|
- `review/checklist.md`: `prereg-with-results` under Gate Integrity and `test-keyboard-count-loop` under Pass 2; `bin/gstack-coauthor-trailers` for the contributor-credit trailers of a port.
|
|
- `lib/ship-receipt-prose.ts`, `gstack-ship-receipt read --check-prose`: the body's quoted counters against the receipt (`RECEIPT_PROSE_MISMATCH`, [ship-policy.md](ship-policy.md#ship-receipt)).
|
|
- `lib/ship-receipt.ts`, `lib/headless-artifacts.ts`: the `review_loop` receipt object, its schema and the summary line.
|