Garry Tanandcapy-ai[bot]
576839cb8c
* feat(ship): review loop converges (frozen finding scope, 3-pass cap, NO FINDINGS first-class) and receipts carry across conflict-only merges
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* test(ship): keep the Step 9 first-entry and Fix-First phrasing the skip/requeue contract asserts; fill the review record's pass, review_scope and range_base placeholders in the caller template test; narrow a test type
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* fix: pre-landing review fixes (pass 1) and the review-loop budget wording
Two blocking findings from /ship's own pass 1, both reproduced in the QA smoke before the fix:
gstack-ship-receipt carry hashed the product diff with git patch-id --stable, which strips
whitespace, so a space moved in a reviewed shell line carried as "no product change"; and it
classed package.json and every lockfile as release metadata, so a dependency swap carried too.
The patch-id is now --verbatim, package.json and VERSION are metadata only while the change is a
version stamp (lib/pregate/stamp-only.ts), lockfiles are product unless the policy names them,
the test globs name code extensions, a dirty working tree refuses before anything is compared,
and a receipt carried twice keeps the reviewed head as carry.from.
gstack-review-scope: an unresolvable base is POLICY_SOURCE_UNAVAILABLE instead of a silent
working-tree read; --range-base is validated; git runs with core.quotePath=false; range_base is
null on pass 1; a blocking finding with no path is fix; category "testing" is a test finding
below the floor; defer searches once, files nothing on a failed search and exits 0 unfiled;
summary refuses zero matching passes and the receipt write is chained to it with &&; a finding
the diff causes elsewhere names the changed line in `cause` and classify follows it.
Review-loop budget wording (ideas from tstack's sol-loop and pstack's interrogate, rewritten;
NOTICE.md): the blocker / should / nit glossary and two stop rules in Step 9.4, the checklist
and docs/review-policy.md; a lead-judgment stage in the finding merge both /review and /ship
render. The core read diffs $RANGE_BASE from pass 2 on. Caps and pins re-measured with reasons.
Step 7 coverage audit: four tests added in existing files, the --review-loop case moved to its
own test (the shared test tripped bun's 5 s default).
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* fix: pin the renumbered review-army stages and point the wording test at product code
CI at c3e27fc failed three free-tier tests, all this branch's own: the
workflow-required-steps fixture and shared-libs-rendering still pinned the
pre-Lead-judgment stage numbers, and test-of-test-ratchet counted
review-loop-budget-wording.test.ts as test-only code because it read only
generated markdown. The fixture and rendering test now pin stages 1-8, and the
wording test renders both armies through generateReviewArmy and reads the
authored ship template as well as the generated sections.
Pass-2 maintainability notes folded in: docs/troubleshooting.md names the two
new RECEIPT_CARRY_REFUSED causes (dirty tree, unreadable status) and the two new
REVIEW_SCOPE_INPUT causes (non-hex --range-base, summary with no matching
pass); gstack-review-scope's help states that classify reads the base branch's
policy and refuses an unresolvable base unless --policy is passed.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* fix: date a review-log row from its binding start when it has no timestamp
The Step 9.4 template asks the writer to fill `timestamp`; the logger stamps
`review_binding.started_at` but never `timestamp`. `gstack-review-scope
summary` dropped such a row silently, so this PR's own pass 2 was missing from
REVIEW_LOOP (passes=1 for a two-pass loop). A row now dates from `timestamp`,
else from its binding start; a row with neither is still not counted.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* fix: pass-4 review fixes (carry stamp rule, deleted-file locus, scanned defer, diff prefixes, row time)
Five findings at the blocking floor from the Step 11 outside voice (gpt-6.1-sol)
and the native adversarial pass, fixed as a fourth pass approved by GStack
Minder under the three-pass policy:
- carry: a path the policy names under stamp_files, stamp_paths or
release_outputs is metadata only while its change is a stamp (versionStampOnly
for a manifest kind, stampOnlyText otherwise), the rule the tree receipt
applies; a logic change in a stamped source file is product. Every
metadata-listed path is read at both heads and a non-stamp change drops its
globs from the product exclusion.
- review-scope: parseHunks keeps a deleted file as a whole changed location, so
a cause anchored in a removed export is the diff's to fix, not deferred.
- defer: issues are filed through lib/gstack-post's publication scan; a HIGH
finding in the title or body refuses and a MEDIUM one needs a human, both
recorded as unfiled with the reason; --repo names the destination.
- classify: both diffs pin --src-prefix=a/ --dst-prefix=b/ --no-textconv, so
diff.mnemonicPrefix cannot send every finding outside.
- summary: a row dates from review_binding.started_at before the writer-typed
timestamp, so a date-only value cannot drop a real pass.
Docs: ship-policy.md#receipt-carry, review-policy.md, the Step 9.4 template
and the CHANGELOG gate-integrity note record the rule and the pass.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* test(review-army): assert the Red Team branch the run took in the N+1 case
Under the blocking floor the Performance specialist rates a 12-line N+1
INFORMATIONAL, so the conditional Red Team does not activate; the case
encoded the old severity (N+1 is CRITICAL, a Red Team always follows). It
now asserts the branch the run took: a Red Team that ran must follow the
Performance specialist; no Red Team is correct only when the report's own
count says 0 critical and its Red Team line names that reason. The N+1
detection assertion is unchanged. The free contract test gains the
informational-not-activated and informational-claimed controls and the
budget test's synthetic report carries the count line.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* v1.91.256.0 stamp
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>
7.4 KiB
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.
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)
- Pass 1 reviews the whole diff against the merge base. Every finding goes
through
gstack-review-scope classify, which stamps it withlocus(diff: a hunk the PR changed, with three lines of context;outside: anything else),kind,blockingandscope. Findings in the diff arefix. A blocking finding in code the PR did not change isdefer:gstack-review-scope deferfiles 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 isunfiled (publication scan refused: …);unfiled (<reason>)whenghis unavailable or its search fails; every unfiled case still exits 0) and the PR body lists it under## Deferred findings. A finding'scause(the changed line behind a defect reported elsewhere, a deleted file included) places it in the diff. Below-floor findings outside the diff arenote, listed under## Review notes. - Pass 2 and later review only the range since the previous pass. Each pass
saves its head; the next pass diffs
RANGE_BASE..HEADfor 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 isfix; every below-floor finding is anote. A finding the diff causes in code it did not change names the changed line incause, and the locus follows it. - Three passes is the cap. A pass that applies no edits is the exit: the
PR ships with its deferred and noted items named in the body. A blocking
finding still in
fixscope on pass 3 stops the run and names the finding (path, severity, problem) for the owner; a fourth pass is the owner's call, never the loop's.
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.
What the receipt records
gstack-review-scope summary --since <REVIEW_SINCE> reads this invocation's
review-log rows and writes the counters gstack-ship-receipt write --review-loop
puts in the receipt:
| Field | Meaning |
|---|---|
review_loop.passes |
review passes this ship ran (1 to 3) |
review_loop.blocking_found |
distinct blocking findings across the passes |
review_loop.deferred |
blocking findings in unchanged code filed as issues |
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 |
The receipt's summary line prints passes=<n> blocking_found=<k> deferred=<m>,
which is what a scorecard tracks (target: a median of 2 passes per PR). A
receipt carried across a base merge or a test-only fix
(ship-policy.md#receipt-carry) keeps these
counters, because the reviewed product diff is unchanged.
The scope knob
.gstack/ship-policy.json 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 | 3 passes |
diff-plus-blocking |
the diff | fixed in this PR | the range since the previous pass, blocking only | 3 passes |
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.
Where this is enforced
scripts/resolvers/review-army.ts: the floor andNO FINDINGSin every specialist and Red Team prompt (rendered intoreview/sections/review-army.mdandship/sections/review-army.md).ship/sections/review-army.mdStep 9: passes,RANGE_BASE, classify, defer, render, the cap decisions;ship/sections/adversarial.mdStep 11: the outside voice reads the same range and scopes queued fixes the same way.lib/review-scope.ts,bin/gstack-review-scope:classify,defer,render,summary; refusals areREVIEW_SCOPE_INPUTandREVIEW_SCOPE_RANGE(troubleshooting.md).lib/ship-policy.ts: thereview_scopekey and itsship: review_scope …consequence line.lib/ship-receipt.ts,lib/headless-artifacts.ts: thereview_loopreceipt object, its schema and the summary line.