mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-08 09:48:20 +01:00
fix(ship-release): count a fix-check row only under a recorded review_loop_grant (GST-1130 granted round)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
01593be831
commit
35ae01e428
4 files changed
+34
-9
No files matched your search
@@ -1,9 +1,9 @@
|
||||
**A granted fix-check's clean read now counts at Step 16.** On gstack-evals#168 the owner granted one scoped fix-check, the reader read `9b0c56c..7ea1907` and found nothing, and `gstack-ship-release finish` still stopped with `STOP: behavior, tests or build inputs changed since review (tests/panel-kit-v3-1.test.ts)`, sending the test file the fix-check had read back through Steps 5 to 11.5. The review-loop exception could not help, because it is the owner accepting unreviewed edits and it never accepts a gate-edited test. The fix-check row also read `UNVERIFIED`, because it was logged without its `completed` and `converged` fields (GST-1130, GEVA-218).
|
||||
|
||||
### Fixed
|
||||
- **`gstack-ship-release finish`** reads the newest `review-fix-check` row. When the row is a completed clean read on committed content and its `range_base` is a branch commit holding the reviewed tree, every code or test path inside that range that is unchanged since routes as `fix_checked=` on the `ROUTE:` line, with a `FIX_CHECK: reviewed <paths>` line. That includes a gate-edited test. Authored docs still get the plan-item recheck, and a path edited after the fix-check still stops. A row that does not qualify prints `FIX_CHECK: not counted (<reason>)` and changes nothing.
|
||||
- **`gstack-ship-release finish`** reads the newest `review-fix-check` row. When the row is a completed clean read on committed content its `range_base` is a branch commit holding the reviewed tree, and the owner's recorded `review_loop_grant` names a stop head inside that range, every code or test path inside that range that is unchanged since routes as `fix_checked=` on the `ROUTE:` line, with a `FIX_CHECK: reviewed <paths>` line. That includes a gate-edited test. Authored docs still get the plan-item recheck, and a path edited after the fix-check still stops. A row that does not qualify prints `FIX_CHECK: not counted (<reason>)` and changes nothing.
|
||||
- **`gstack-review-log`** refuses a `review-fix-check` row that does not carry `completed` and `converged` as true or false, and keeps the start token for the retry. The refusal names the missing fields, so the row is fixed when it is written instead of reading `UNVERIFIED` at Step 11.5.
|
||||
|
||||
### Tests
|
||||
- `test/ship-rereview-triggers.test.ts`: a fixture shaped like gstack-evals#168 (a round-2 pass, then a fix commit that edits the source and a gate-edited test) stops on main and passes finish once a clean fix-check row with its range base is logged through the real `gstack-review-log`. Negative controls: an edit after the fix-check still stops, and an incomplete row, a row with no range base, a range base that does not hold the reviewed tree, or a row logged on uncommitted content is not counted.
|
||||
- `test/ship-rereview-triggers.test.ts`: a fixture shaped like gstack-evals#168 (a round-2 pass, then a fix commit that edits the source and a gate-edited test) stops on main and passes finish once a clean fix-check row with its range base is logged through the real `gstack-review-log`. Negative controls: an edit after the fix-check still stops, and an ungranted row, a grant for another stop head, an incomplete row, a row with no range base, a range base that does not hold the reviewed tree, or a row logged on uncommitted content is not counted.
|
||||
- `test/review-log-findings.test.ts`: a `review-fix-check` row missing either field, or with a non-boolean value, is refused and keeps its start token; the complete row binds as verified.
|
||||
@@ -496,8 +496,9 @@ too large for the scoped review, and without the record nothing changes.
|
||||
A completed fix-check is a review, not an owner acceptance (GST-1130). Finish
|
||||
reads the newest `review-fix-check` row from `gstack-review-read`. When it is a
|
||||
completed clean read on committed content (freshness `CURRENT` against its own
|
||||
tree) whose commit is on the branch and whose `range_base` is a branch commit
|
||||
holding the reviewed tree, each code or test path inside `range_base..commit`
|
||||
tree) whose commit is on the branch, whose `range_base` is a branch commit
|
||||
holding the reviewed tree, and for which an approved `review_loop_grant` in
|
||||
`gstack-approvals` names a stop head inside `range_base..commit`, each code or test path inside `range_base..commit`
|
||||
that is unchanged since routes as `fix_checked=` in `ROUTE:` and is listed on a
|
||||
`FIX_CHECK: reviewed` line, a gate-edited test included. Authored docs keep the
|
||||
stage 2.2 recheck, and a path edited after the fix-check still routes by the
|
||||
|
||||
+11
-4
@@ -15,6 +15,7 @@ import { mergeChangelogs, normalizeChangelog } from './changelog-check';
|
||||
import { versionAtRef } from './restamp';
|
||||
import { classifyPath } from './review-depth';
|
||||
import { comparePath, contextPacket, gateEditedPaths, isTestCode, PACKET_SECTION_CAP, readScopedReview, touchedPaths, type PatchSides } from './ship-patch';
|
||||
import { locate, readRecord } from './approval-record';
|
||||
import { reviewFreshness } from './review-evidence';
|
||||
import { parseReviewLoopException, type ReviewLoopException } from './ship-receipt-depth';
|
||||
import { parseReceiptDocs, type ReceiptDocs } from './ship-receipt-docs';
|
||||
@@ -164,8 +165,9 @@ function commitRelease(cwd: string, release: string[], trailer: string | undefin
|
||||
/**
|
||||
* Step 16 stage 2's route. With --reviewed-base, a path whose branch patch is the reviewed one counts as base (lib/ship-patch.ts).
|
||||
* A code or test path the newest completed clean review-fix-check row read (inside its range_base..commit range, where
|
||||
* range_base holds the reviewed tree, and unchanged since) counts as fix_checked: a granted fix-check after Step 11 is a review
|
||||
* (GST-1130). Authored docs keep the stage 2.2 recheck.
|
||||
* range_base holds the reviewed tree, and unchanged since) counts as fix_checked when an approved review_loop_grant names a stop
|
||||
* head inside range_base..commit: the owner's granted fix-check after Step 11 is a review (GST-1130). Authored docs keep the
|
||||
* stage 2.2 recheck.
|
||||
*/
|
||||
type Routed = Record<'behavior' | 'docs' | 'metadata' | 'tests' | 'base' | 'context' | 'accepted' | 'fix_checked', string[]>;
|
||||
function route(cwd: string, base: string, reviewed: string, reviewedBase: string | undefined, release: string[], opts: { docsRechecked: boolean; contextReviewed?: string; loop?: ReviewLoopException }, r: Report): Routed | null {
|
||||
@@ -208,10 +210,15 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string
|
||||
const onBranch = (c: string) => !!c && git(cwd, ['merge-base', '--is-ancestor', c, 'HEAD']).status === 0;
|
||||
const fixCommit = commitOf(fixRow?.commit_full), fixFrom = commitOf(fixRow?.range_base);
|
||||
const fresh = fixRow && reviewFreshness(fixRow, fixRow.wtree);
|
||||
const ancestor = (a: string, d: string) => git(cwd, ['merge-base', '--is-ancestor', a, d]).status === 0;
|
||||
const grants = fixRow ? (loc => 'file' in loc ? readRecord(loc).rows : [])(locate({ cwd, env: process.env })) : [];
|
||||
const grant = grants.find(g => g.outcome === 'approved' && `${g.ask} ${g.scope}`.includes('review_loop_grant')
|
||||
&& (`${g.ask} ${g.scope}`.match(/\b[0-9a-f]{7,40}\b/g) ?? []).some(s => { const c = commitOf(s); return !!c && !!fixFrom && !!fixCommit && ancestor(fixFrom, c) && ancestor(c, fixCommit); }));
|
||||
const notCounted = !fixRow ? '' : fresh?.status !== 'CURRENT' ? fresh?.reason ?? 'not a review row'
|
||||
: fixRow.tree !== fixRow.wtree || !onBranch(fixCommit) ? 'the row was logged on content no branch commit holds'
|
||||
: !onBranch(fixFrom) ? 'the row names no range_base commit on this branch'
|
||||
: git(cwd, ['rev-parse', `${fixFrom}^{tree}`]).stdout.trim() !== reviewed ? `range_base ${fixFrom.slice(0, 12)} does not hold the reviewed tree` : '';
|
||||
: git(cwd, ['rev-parse', `${fixFrom}^{tree}`]).stdout.trim() !== reviewed ? `range_base ${fixFrom.slice(0, 12)} does not hold the reviewed tree`
|
||||
: !grant ? 'no approved review_loop_grant names a stop head inside its range_base..commit' : '';
|
||||
if (notCounted) r.add(`FIX_CHECK: not counted (${notCounted})`);
|
||||
const counted = !!fixRow && !notCounted;
|
||||
if (counted) {
|
||||
@@ -219,7 +226,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string
|
||||
if (!range || !after) { r.stop(`git could not diff the fix-check range ${fixFrom.slice(0, 12)}..${fixCommit.slice(0, 12)} against the reviewed and current trees: stage 2 is blocked`); return null; }
|
||||
by.fix_checked = [...by.behavior, ...by.tests, ...by.context].filter(p => range.has(p) && !after.has(p)).sort();
|
||||
for (const k of ['behavior', 'tests', 'context'] as const) by[k] = by[k].filter(p => !by.fix_checked.includes(p));
|
||||
r.add(`FIX_CHECK: reviewed ${by.fix_checked.length ? short(by.fix_checked) : 'none'} (fix-check at ${fixCommit.slice(0, 12)}, range from ${fixFrom.slice(0, 12)})`);
|
||||
r.add(`FIX_CHECK: reviewed ${by.fix_checked.length ? short(by.fix_checked) : 'none'} (fix-check at ${fixCommit.slice(0, 12)}, range from ${fixFrom.slice(0, 12)}, grant ${grant!.id})`);
|
||||
}
|
||||
const loop = opts.loop;
|
||||
if (loop) {
|
||||
|
||||
@@ -752,6 +752,11 @@ describe('finish after a completed fix-check (GST-1130, GEVA-218)', () => {
|
||||
expect(start.status).toBe(0);
|
||||
return f.run(path.join(BIN, 'gstack-review-log'), [JSON.stringify({ skill: 'review-fix-check', timestamp: '2026-11-02T00:00:00Z', via: 'ship', review_scope: 'frozen', status: 'clean', issues_found: 0, critical: 0, ...row }), '--finish', start.stdout.trim()]);
|
||||
};
|
||||
const grant = (f: ShipFx, stopHead: string) => {
|
||||
const a = f.run(path.join(BIN, 'gstack-approvals'), ['add', '--ask', `Owner grant: one scoped fix-check after round 2 (review_loop_grant ${stopHead.slice(0, 12)})`, '--words', 'one scoped fix-check after round 2', '--by', 'GStack Minder', '--scope', 'this /ship invocation', '--json'], { GSTACK_APPROVALS_MIRROR: '0' });
|
||||
expect(a.status).toBe(0);
|
||||
return JSON.parse(a.stdout).added[0] as string;
|
||||
};
|
||||
const afterFixCheck = () => {
|
||||
const f = fx({ 'src/x.ts': 'export const x = 1;\n' }, { 'src/x.ts': 'export const x = 2;\n', 'test/x.test.ts': TEST_V1 });
|
||||
const passHead = f.git('rev-parse', 'HEAD').stdout.trim();
|
||||
@@ -764,9 +769,10 @@ describe('finish after a completed fix-check (GST-1130, GEVA-218)', () => {
|
||||
const { f, passHead, fixHead, reviewed, post } = afterFixCheck();
|
||||
const plain = finish(f, reviewed, post);
|
||||
expect([plain.status, plain.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/x.ts,test/x.test.ts)')]).toEqual([3, true]);
|
||||
const id = grant(f, passHead);
|
||||
expect(logFixCheck(f, { completed: true, converged: true, range_base: passHead }).status).toBe(0);
|
||||
const r = finish(f, reviewed, post);
|
||||
expect(lineOf(r.stdout, 'FIX_CHECK')).toBe(`FIX_CHECK: reviewed src/x.ts,test/x.test.ts (fix-check at ${fixHead.slice(0, 12)}, range from ${passHead.slice(0, 12)})`);
|
||||
expect(lineOf(r.stdout, 'FIX_CHECK')).toBe(`FIX_CHECK: reviewed src/x.ts,test/x.test.ts (fix-check at ${fixHead.slice(0, 12)}, range from ${passHead.slice(0, 12)}, grant ${id})`);
|
||||
expect(lineOf(r.stdout, 'ROUTE')).toMatch(/ behavior=0 docs=0 metadata=0 tests=0 base=unknown context=0 fix_checked=2$/);
|
||||
expect([r.status, lineOf(r.stdout, 'EVIDENCE')]).toEqual([0, expect.stringMatching(/^EVIDENCE: FRESH label=tests /)]);
|
||||
f.write({ 'src/x.ts': 'export const x = 4;\n' }); f.commit('an edit after the fix-check');
|
||||
@@ -775,6 +781,17 @@ describe('finish after a completed fix-check (GST-1130, GEVA-218)', () => {
|
||||
expect([late.status, late.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/x.ts)')]).toEqual([3, true]);
|
||||
});
|
||||
|
||||
test('a clean fix-check row with no recorded review_loop_grant for a stop head in its range is not counted', () => {
|
||||
const { f, passHead, reviewed, post } = afterFixCheck();
|
||||
expect(logFixCheck(f, { completed: true, converged: true, range_base: passHead }).status).toBe(0);
|
||||
const ungranted = finish(f, reviewed, post);
|
||||
expect(lineOf(ungranted.stdout, 'FIX_CHECK')).toBe('FIX_CHECK: not counted (no approved review_loop_grant names a stop head inside its range_base..commit)');
|
||||
expect([ungranted.status, ungranted.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/x.ts,test/x.test.ts)')]).toEqual([3, true]);
|
||||
grant(f, mergeBase(f));
|
||||
const elsewhere = finish(f, reviewed, post);
|
||||
expect([elsewhere.status, lineOf(elsewhere.stdout, 'FIX_CHECK')]).toEqual([3, 'FIX_CHECK: not counted (no approved review_loop_grant names a stop head inside its range_base..commit)']);
|
||||
});
|
||||
|
||||
test('a fix-check row that did not finish clean, or names no range base, is not counted', () => {
|
||||
const { f, passHead, reviewed, post } = afterFixCheck();
|
||||
expect(logFixCheck(f, { completed: false, converged: false, range_base: passHead }).status).toBe(0);
|
||||
|
||||
Reference in new issue
Block a user