Let an owner's ruling clear new blockers that an extra review round found, so /ship can finish (GST-2251) (#4260)

* Let an owner's ruling clear the new blockers a granted fix-check found (GST-2251)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Name every finding that still holds a fix-check open, with the full rule command (GST-2251)

Review-Finding: p1/adversarial-native#1 reader=adversarial-native
Review-Finding: p1/red-team#1 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Describe the wider FIX_CHECK_OPEN list in the release note (GST-2251)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Keep a skipped blocking finding open until the owner rules it, even beside a ruled CRITICAL (GST-2251)

A ruled open CRITICAL could clear a fix-check row while an unruled non-CRITICAL finding logged skipped with blocking:true stayed unruled. Rulings now clear a row only when one covers every unresolved or blocking finding, and FIX_CHECK_OPEN names the rest.

Review-Finding: captain-audit/Q1 reader=outside-voice
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>
This commit is contained in:
Garry Tanandcapy-ai[bot] authored and GitHub committed 2026-11-10 01:43:01 +00:00
1 parent 56c4e873b2
commit 46d51496b4
7 files changed
+155 -14

No files matched your search

@@ -0,0 +1,4 @@
---
important: blocker
---
- **An owner's ruling now clears the new blockers a granted fix-check found, so /ship can finish with them listed as open notes.** When a granted fix-check finds a new CRITICAL, it logs the finding `open` and stops for the owner. The owner can rule it a disclosed note with `gstack-approvals rule`, but `gstack-ship-release finish` rejected the fix-check as unfinished before it looked at any ruling. It also ignored rulings on `open` findings, so the stop could only end with another fix-check (harness-evals#361, GST-2251). Now, once an approved ruling covers every finding that still holds it open, the fix-check counts at the tree it read, and each ruling is printed and listed in the PR body and receipt. A fix-check with a finding still unruled prints `FIX_CHECK_OPEN:`, which names every finding that still holds it open, including below-floor ones, and the full `gstack-approvals rule` command to run. A finding the lane only marks as accepted in its own review-log row still stays open, and so does a ruling at a head outside the fix-check's range. Edits made after the fix-check still route as post-review changes.
+2 -1
View File
@@ -730,7 +730,8 @@ reads the newest `review-fix-check` row from `gstack-review-read`, chained back
through earlier rows whose commit is the next row's `range_base`
([chained fix-checks](ship-policy.md#fix-check-chain), GST-1627). When every link
is a completed clean read on committed content (freshness `CURRENT` against its own
tree) whose commit is on the branch, the oldest link's `range_base` is a branch commit
tree; a fix-check that stopped on findings the owner has since ruled notes reads
clean, [ruled notes](ship-policy.md#ruled-notes)) whose commit is on the branch, the oldest link's `range_base` is a branch commit
holding the reviewed tree, and for each link an approved `review_loop_grant` in
`gstack-approvals` names a stop head inside its `range_base..commit`, each code or test path inside a link's `range_base..commit`
that is unchanged since, or whose change against the base is still the one the
+11 -2
View File
@@ -1683,13 +1683,22 @@ row is ignored. Step 16's `finish` reads the branch's rulings
(`lib/ruled-notes.ts`) and prints each as `RULED_NOTE: <fingerprint> at <head12>
ruled by <who> at <time>: <reason> (approval <id>)`. A ruling closes a finding on
a `review-fix-check` row finish counts (the newest, or an earlier link of its
[chain](#fix-check-chain); action `skipped`, `noted` or `deferred`, not a
[chain](#fix-check-chain); action `open`, `skipped`, `noted` or `deferred`, not a
gate finding) when its head lies inside that row's `range_base..commit`, the same
test a `review_loop_grant` passes. So the order of ruling and fix-check doesn't
matter, and a ruling made at an earlier /ship's head closes nothing: its
`RULED_NOTE:` line ends `(outside this fix-check's range; not applied)`. A granted
fix-check whose other findings are all below the floor then reads CURRENT and
counts. A skipped CRITICAL with no ruling stays open. Nothing ruled is hidden:
counts. That includes the usual order, where the fix-check finds a new CRITICAL,
logs it `open` with `converged:false` and stops for the owner, and the owner rules
afterwards: the row's `incomplete` binding still names the one tree it read, so
once every blocking finding on it is ruled it binds at that tree as a clean read
(harness-evals#361, GST-2251). A CRITICAL with no ruling stays open, and while one
does, finish prints `FIX_CHECK_OPEN: <fingerprints> on the fix-check at <commit12>`
naming the `gstack-approvals rule` command for each
([troubleshooting](troubleshooting.md#fix-check-open)). A review-loop exception is
not a ruling: it accepts edits after a fix-check, never the fix-check's own
findings. Nothing ruled is hidden:
`gstack-approvals rulings` prints one `Ruled note: …` line per ruling for the
Pre-Landing Review and the PR body, `gstack-ship-receipt write` records them as
`ruled_notes` (each with its `head`) and refuses a `--body` that does not carry each
+23
View File
@@ -6813,6 +6813,29 @@ PR whose older receipt lost its parent branch, or a depth, targeted or exception
refusal, run /ship on the new head. When `--head` is not the checked-out HEAD,
check it out and carry there.
<a id="fix-check-open"></a>
### `FIX_CHECK_OPEN`
**Meaning.** `gstack-ship-release finish` did not count a granted fix-check
because it still has findings no owner has ruled on that still hold it open. The line lists
their fingerprints and the fix-check's commit. A fix-check that found a new
CRITICAL logs it `open` with `converged:false`, so `FIX_CHECK: not counted`
reads `missing start capture or incomplete/nonconverged pass` until the
rulings exist. A `--review-loop-exception` does not help here: it accepts
edits made after `fix_check.head` (`REVIEW_LOOP_EXCEPTION: accepted none`
when there are none), never the fix-check's own findings (harness-evals#361).
**Fix.** When the owner accepts the findings as disclosed notes, the owner
records one ruling per listed fingerprint, outside the lane's /ship run, at
the printed commit or any head inside the fix-check's range:
`gstack-approvals rule --finding <fingerprint> --head <commit> --reason '<why>'
--words '<owner's exact words>' --by '<owner>'`. Then run finish again. It
prints each `RULED_NOTE:`, and the PR body and receipt list every `Ruled
note:` line ([ship-policy.md](ship-policy.md#ruled-notes)). A ruling at a head
outside the range is listed as `not applied`, and a path edited after the
fix-check still routes by the post-review rules. When the owner wants the
findings fixed instead, fix them and ask for a grant for one more fix-check.
<a id="review-loop-exception-invalid"></a>
### `REVIEW_LOOP_EXCEPTION_INVALID`
+16 -8
View File
@@ -335,22 +335,27 @@ export function bindReview(rec: Record<string, any>, token: string, env = proces
return rec;
}
const unresolvedFinding = (f: unknown) => record(f) && (f.action === 'open' ||
export const unresolvedFinding = (f: unknown) => record(f) && (f.action === 'open' ||
((f.action === 'deferred' || f.action === 'noted') && (f.blocking === true || isCriticalFinding(f))) ||
(!f.gate && isCriticalFinding(f) && isOpenByAction(f)));
const RULABLE = new Set(['skipped', 'noted', 'deferred']);
export const RULABLE = new Set(['open', 'skipped', 'noted', 'deferred']);
/** What a ruling must cover before rulings can clear a row: every unresolved finding and every one the reader marked blocking, whatever its action. */
export const holdsOpen = (f: unknown) => !record(f) || unresolvedFinding(f) || f.blocking === true;
/**
* GST-1537: the claimed row with each finding the owner ruled a disclosed note (`ruled`: fingerprints from approved
* `gstack-approvals rule` rows, lib/ruled-notes.ts, never from the row itself) taken out, and recorded clean when the
* rest leave nothing unresolved and the findings account for every claimed issue and critical; otherwise unchanged.
* rest leave nothing unresolved and the findings account for every claimed issue and critical; otherwise unchanged. An open
* finding is rulable too: a fix-check that finds a CRITICAL stops for the owner (review-army decision 3.5 sends it to decision 2)
* with the finding open and the row converged:false, bound `incomplete` on unchanged content, and the ruling is the owner's
* answer to that stop, so a row the rulings clear binds at the tree it read (GST-2251, harness-evals#361).
*/
function withoutRuledNotes(rec: Record<string, any>, ruled: ReadonlySet<string>): Record<string, any> {
const isRuled = (f: unknown) => record(f) && !f.gate && RULABLE.has(f.action) && typeof f.fingerprint === 'string' && ruled.has(f.fingerprint);
if (!ruled.size || rec.status_source === 'derived' || rec.completed !== true || !Array.isArray(rec.findings) ||
!rec.findings.some(isRuled) || rec.findings.length < Number(rec.issues_found ?? 0) ||
rec.findings.filter(f => record(f) && isCriticalFinding(f)).length < Number(rec.critical ?? 0) ||
rec.findings.some(f => !isRuled(f) && (!record(f) || unresolvedFinding(f)))) return rec;
rec.findings.some(f => !isRuled(f) && holdsOpen(f))) return rec;
return { ...rec, status: 'clean', issues_found: 0, critical: 0, informational: 0, findings: rec.findings.filter(f => !isRuled(f)) };
}
@@ -363,17 +368,20 @@ export function reviewFreshness(rec: Record<string, any>, currentWtree: string,
return { status: 'STALE', reason: `content changed during review: the tree moved from ${short(binding.start_wtree)} to ${short(binding.end_wtree)} while ${rec.skill} ran (an edit, commit or merge landed before its row was logged), so the record cannot bind. Repair: freeze the checkout until every reviewer returns, then rerun the steps 11.5 names on the frozen tree; when the edit was your own fix, log it as a fix so gstack-review-plan picks the repeat depth (docs/troubleshooting.md#review-content-changed)` };
}
if (rec.scope === 'fix-range') return { status: 'UNVERIFIED', reason: 'fix-range pass; only a later full pass can clear review' };
if (binding?.state !== 'verified' || rec.completed !== true || rec.converged !== true ||
!rec.wtree || binding.start_wtree !== rec.wtree || binding.end_wtree !== rec.wtree) {
const cleared = withoutRuledNotes(rec, ruled);
const ruledStop = cleared !== rec && binding?.state === 'incomplete' && rec.converged === false;
const wtree = ruledStop ? binding.end_wtree : rec.wtree;
if ((binding?.state !== 'verified' && !ruledStop) || rec.completed !== true || (rec.converged !== true && !ruledStop) ||
!wtree || binding.start_wtree !== wtree || binding.end_wtree !== wtree) {
return { status: 'UNVERIFIED', reason: 'missing start capture or incomplete/nonconverged pass' };
}
if (!currentWtree || currentWtree === 'unknown' || rec.wtree !== currentWtree) {
if (!currentWtree || currentWtree === 'unknown' || wtree !== currentWtree) {
return { status: 'STALE', reason: 'working-tree content differs from reviewed content' };
}
// B7: a derived row carries findings_open; a resolved critical finding no
// longer counts against it. Claimed rows keep the conservative reading of
// issues_found, and the legacy `critical` and Codex counters stay as they were.
rec = withoutRuledNotes(rec, ruled);
rec = cleared;
const open = typeof rec.findings_open === 'number' ? rec.findings_open : rec.issues_found;
if (rec.status !== 'clean' || open > 0 || rec.critical > 0 ||
(rec.skill === 'codex-review' && rec.findings > (rec.findings_fixed ?? 0)) ||
+6 -3
View File
@@ -21,7 +21,7 @@ import { locate, readRecord } from './approval-record';
import { mechanicalPaths, writeMechanicalRecord } from './ship-mechanical';
import { uidSidecars } from './ship-sidecar';
import type { MechanicalConstant } from './ship-policy-mechanical';
import { reviewFreshness } from './review-evidence';
import { holdsOpen, reviewFreshness, RULABLE } from './review-evidence';
import { isCriticalFinding, isOpenByAction } from './headless-artifacts';
import { fingerprintPath, normalizePath } from './review-scope';
import { forbidSpecs } from './review-fetch';
@@ -295,9 +295,10 @@ function fixChecked(cwd: string, o: { reviewed: string; now: string; baseRef: st
}
const ruledOn = (l: FixLink) => new Set(rulings.filter(n => n.links.includes(l)).map(n => n.finding));
const why = (l: FixLink) => {
const fresh = reviewFreshness(l.row, l.row.wtree, ruledOn(l));
const read = l.row.wtree ?? l.row.review_binding?.end_wtree;
const fresh = reviewFreshness(l.row, read, ruledOn(l));
return fresh?.status !== 'CURRENT' ? fresh?.reason ?? 'not a review row'
: l.row.tree !== l.row.wtree || !onBranch(l.commit) ? 'the row was logged on content no branch commit holds'
: l.row.tree !== read || !onBranch(l.commit) ? 'the row was logged on content no branch commit holds'
: !onBranch(l.from) ? 'the row names no range_base commit on this branch' : '';
};
const at = (l: FixLink) => (chain.length > 1 ? `; fix-check at ${l.commit.slice(0, 12)}, link ${chain.indexOf(l) + 1} of ${chain.length}` : '');
@@ -321,6 +322,8 @@ function fixChecked(cwd: string, o: { reviewed: string; now: string; baseRef: st
: uncovered ? `a finding on ${uncovered.p} that no later clean fix-check's range touched${at(uncovered.d)}`
: ungranted ? `no approved review_loop_grant names a stop head inside its range_base..commit${at(ungranted)}` : '';
if (notCounted) r.add(`FIX_CHECK: not counted (${notCounted})`);
const rulable = (l: FixLink) => (Array.isArray(l.row.findings) ? l.row.findings : []).filter((f: any) => !!f && !f.gate && RULABLE.has(f.action) && holdsOpen(f) && typeof f.fingerprint === 'string' && !ruledOn(l).has(f.fingerprint)).map((f: any) => f.fingerprint as string);
for (const l of notCounted ? chain : []) if (rulable(l).length) r.add(`FIX_CHECK_OPEN: ${rulable(l).join(',')} on the fix-check at ${l.commit.slice(0, 12)}: the owner rules each one a disclosed note with gstack-approvals rule --finding <fingerprint> --head ${l.commit.slice(0, 12)} --reason '<why>' --words '<owner\'s exact words>' --by '<owner>'; a review-loop exception accepts edits after a fix-check, never its findings (docs/troubleshooting.md#fix-check-open)`);
if (!chain.length || notCounted) return { counted: false, paths: [] };
const read: ((p: string) => boolean)[] = [];
for (const [k, l] of chain.entries()) {
+93
View File
@@ -399,3 +399,96 @@ describe('ruledNotes reads only approved owner rulings (GST-1537)', () => {
expect(ruledNoteText(notes[0]!)).toBe(`${RACE} at 0123456789ab ruled by qm minder (CAPY-1) at 2026-11-05T01:02:03.000Z: the lock already allows it (approval ${notes[0]!.approval})`);
});
});
// harness-evals#361 (GST-2251): round 2 stopped, the owner granted one scoped fix-check, it read the fix and found two new
// CRITICALs, so it logged converged:false (review-army decision 3.5 sends a CRITICAL to decision 2). The owner accepted both
// as disclosed notes. finish refused the row as nonconverged before reading any ruling, and the lane's review-loop exception,
// which accepts edits after a fix-check, accepted nothing, so nothing pointed at the ruling the docs name.
const NEW_A = 'src/x.ts:1:unbounded-retry';
const NEW_B = 'src/x.ts:1:lost-ack';
const HE361 = {
completed: true, converged: false, status: 'issues_found', issues_found: 2, critical: 2, informational: 0,
findings: [
{ fingerprint: NEW_A, severity: 'CRITICAL', action: 'open', locus: 'fix-range', blocking: true, raised_by: 'fix-check' },
{ fingerprint: NEW_B, severity: 'CRITICAL', action: 'open', locus: 'diff', blocking: true, raised_by: 'targeted' },
],
};
const NONCONVERGED = 'FIX_CHECK: not counted (missing start capture or incomplete/nonconverged pass)';
const openLine = (findings: string[], head: string) => `FIX_CHECK_OPEN: ${findings.join(',')} on the fix-check at ${head.slice(0, 12)}: the owner rules each one a disclosed note with gstack-approvals rule --finding <fingerprint> --head ${head.slice(0, 12)} --reason '<why>' --words '<owner\'s exact words>' --by '<owner>'; a review-loop exception accepts edits after a fix-check, never its findings (docs/troubleshooting.md#fix-check-open)`;
const exception = (head: string) => ['--review-loop-exception', JSON.stringify({ accepted_by: 'harness minder (HARN-1)', date: '2026-11-09', via: 'a-9628d51bef2d', reason: 'both ship as open notes', fix_check: { head, findings: 2 } })];
const finishLoop = (f: ShipFx, reviewed: string, post: string, head: string) => f.run(RELEASE, ['finish', '--base', 'main', '--reviewed-tree', reviewed, '--docs-post', post, '--lane', LANE, ...exception(head)]);
// Value: protects=an owner's rulings close a granted fix-check's own new CRITICALs, so finish counts it at its head and lists
// every ruling (harness-evals#361); fails_when=finish refuses the fix-check as nonconverged after both rulings, or binds with
// one finding unruled, or never names the ruling the owner needs; seam=finish's route reading the review log and the approval
// record; no_claim=Step 11.5's full-pass binding
describe('a granted fix-check that found new CRITICALs the owner accepted (harness-evals#361, GST-2251)', () => {
test('#361: finish names the open findings, then binds at the fix-check head once the owner rules both', () => {
const { f, passHead, fixHead, reviewed, post, grant } = grantedFixCheck(HE361);
const before = finishLoop(f, reviewed, post, fixHead);
expect([before.status, lineOf(before.stdout, 'FIX_CHECK'), lineOf(before.stdout, 'FIX_CHECK_OPEN')]).toEqual([3, NONCONVERGED, openLine([NEW_A, NEW_B], fixHead)]);
expect(lineOf(before.stdout, 'REVIEW_LOOP_EXCEPTION')).toStartWith(`REVIEW_LOOP_EXCEPTION: accepted none (changed since fix-check ${fixHead.slice(0, 12)}`);
for (const finding of [NEW_A, NEW_B]) expect(rule(f, finding, fixHead, 'harness minder (HARN-1)').status).toBe(0);
const r = finishLoop(f, reviewed, post, fixHead);
expect(lineOf(r.stdout, 'FIX_CHECK'), r.stdout).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 ${grant})`);
expect(lineOf(r.stdout, 'FIX_CHECK_OPEN')).toBe('');
expect(r.stdout.split('\n').filter(l => l.startsWith('RULED_NOTE: '))).toEqual([NEW_A, NEW_B].map(x => expect.stringMatching(new RegExp(`^RULED_NOTE: ${x} at ${fixHead.slice(0, 12)} ruled by harness minder \\(HARN-1\\) at .*\\(approval a-[0-9a-f]{12}\\)$`))));
expect(r.status, r.stdout).toBe(0);
const listed = run(f, 'gstack-approvals', ['rulings']).stdout.trim().split('\n');
expect(listed).toEqual([NEW_A, NEW_B].map(x => expect.stringMatching(new RegExp(`^Ruled note: ${x} at ${fixHead.slice(0, 12)} ruled by harness minder`))));
});
test('one ruling of two leaves the fix-check uncounted and names the finding still open', () => {
const { f, fixHead, reviewed, post } = grantedFixCheck(HE361);
expect(rule(f, NEW_A, fixHead).status).toBe(0);
const r = finishLoop(f, reviewed, post, fixHead);
expect([r.status, lineOf(r.stdout, 'FIX_CHECK'), lineOf(r.stdout, 'FIX_CHECK_OPEN')]).toEqual([3, NONCONVERGED, openLine([NEW_B], fixHead)]);
});
test('the lane\'s own claim rules nothing, and a ruling at a head outside the fix-check\'s range closes nothing', () => {
const claimed = { ...HE361, findings: HE361.findings.map(x => ({ ...x, ruled: 'note', ruled_by: 'harness minder (HARN-1)', approval: 'a-9628d51bef2d' })) };
const { f, fixHead, reviewed, post } = grantedFixCheck(claimed);
expect(lineOf(finishLoop(f, reviewed, post, fixHead).stdout, 'FIX_CHECK')).toBe(NONCONVERGED);
const mergeBase = f.git('merge-base', 'main', 'HEAD').stdout.trim();
for (const finding of [NEW_A, NEW_B]) expect(rule(f, finding, mergeBase).status).toBe(0);
const r = finishLoop(f, reviewed, post, fixHead);
expect([r.status, lineOf(r.stdout, 'FIX_CHECK'), lineOf(r.stdout, 'FIX_CHECK_OPEN')]).toEqual([3, NONCONVERGED, openLine([NEW_A, NEW_B], fixHead)]);
expect(r.stdout.split('\n').filter(l => l.startsWith('RULED_NOTE: ')).every(l => l.endsWith(" (outside this fix-check's range; not applied)"))).toBe(true);
});
test('an open INFORMATIONAL beside the two ruled CRITICALs keeps the fix-check uncounted and is named, then binds once ruled', () => {
const NEW_C = 'src/x.ts:1:retry-log';
const row = { ...HE361, issues_found: 3, critical: 2, informational: 1, findings: [...HE361.findings, { fingerprint: NEW_C, severity: 'INFORMATIONAL', action: 'open', locus: 'diff', blocking: false, raised_by: 'fix-check' }] };
const { f, fixHead, reviewed, post } = grantedFixCheck(row);
for (const finding of [NEW_A, NEW_B]) expect(rule(f, finding, fixHead).status).toBe(0);
const before = finishLoop(f, reviewed, post, fixHead);
expect([before.status, lineOf(before.stdout, 'FIX_CHECK'), lineOf(before.stdout, 'FIX_CHECK_OPEN')]).toEqual([3, NONCONVERGED, openLine([NEW_C], fixHead)]);
expect(rule(f, NEW_C, fixHead).status).toBe(0);
const r = finishLoop(f, reviewed, post, fixHead);
expect(lineOf(r.stdout, 'FIX_CHECK'), r.stdout).toStartWith(`FIX_CHECK: reviewed src/x.ts,test/x.test.ts (fix-check at ${fixHead.slice(0, 12)}`);
expect(lineOf(r.stdout, 'FIX_CHECK_OPEN')).toBe('');
expect(r.status, r.stdout).toBe(0);
});
// Value: protects=nothing binds without an owner row: a skipped non-CRITICAL finding the reader marked blocking stays open
// beside a ruled open CRITICAL; fails_when=the ruled CRITICAL clears the row while the skipped blocker has no ruling;
// seam=withoutRuledNotes and FIX_CHECK_OPEN
test('a skipped blocking finding with no ruling keeps the row unbound beside a ruled open CRITICAL, and is named', () => {
const SKIPPED = 'src/x.ts:1:skipped-blocker';
const row = { ...HE361, issues_found: 2, critical: 1, informational: 1, findings: [HE361.findings[0], { fingerprint: SKIPPED, severity: 'INFORMATIONAL', action: 'skipped', locus: 'fix-range', blocking: true, raised_by: 'fix-check' }] };
const { f, fixHead, reviewed, post } = grantedFixCheck(row);
expect(rule(f, NEW_A, fixHead).status).toBe(0);
const r = finishLoop(f, reviewed, post, fixHead);
expect([r.status, lineOf(r.stdout, 'FIX_CHECK'), lineOf(r.stdout, 'FIX_CHECK_OPEN')]).toEqual([3, NONCONVERGED, openLine([SKIPPED], fixHead)]);
});
test('an edit after the fix-check head still routes as behavior without an exception that accepts it', () => {
const { f, fixHead, reviewed, post } = grantedFixCheck(HE361);
for (const finding of [NEW_A, NEW_B]) expect(rule(f, finding, fixHead).status).toBe(0);
f.write({ 'src/x.ts': 'export const x = 4;\n' }); f.commit('edit after the fix-check');
const r = finish(f, reviewed, post);
expect(r.status).toBe(3);
expect(lineOf(r.stdout, 'FIX_CHECK')).toStartWith('FIX_CHECK: reviewed test/x.test.ts (fix-check at');
expect(r.stdout).toContain('STOP: behavior, tests or build inputs changed since review (src/x.ts)');
});
});