mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-08 01:38:10 +01:00
ship finish: an owner-ruled note closes its finding, and every ruling is listed (GST-1537) (#4050)
* ship finish: an owner-ruled note closes its finding, and every ruling is listed (GST-1537) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(windows): exclude ship-finish-ruled-notes from the Windows lanes like its ship-bin-fixture siblings (GST-1537) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * ship: keep the pinned Step 9 brief unchanged; the PR body's Pre-Landing Review lists ruled notes (GST-1537) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p1/specialist-batched#1 reader=state-machines Review-Finding: p1/red-team#2 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p2/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: bind each ruled note to the stop head it was ruled at (GST-1537) Review-Finding: p2/red-team#1 reader=red-team Review-Finding: s11/outside-voice#1 reader=outside-voice Review-Finding: p1/specialist-batched#2 reader=testing 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
1 parent
eaf1701da2
commit
cf21a7fbef
16 files changed
+379
-16
No files matched your search
+33
-4
@@ -7,6 +7,8 @@
|
||||
* [--date YYYY-MM-DD] [--source <ref>] [--branch <name>] [--json]
|
||||
* gstack-approvals import --autoplan <run dir> [--by <whose words>] [--branch <name> | --stack <b1,b2,...>] [--json]
|
||||
* gstack-approvals render [--no-heading] [--full] [--branch <name>]
|
||||
* gstack-approvals rule --finding <fingerprint> --head <stop head> --reason <why> --words <exact words> --by <who ruled> [--json]
|
||||
* gstack-approvals rulings [--branch <name>]
|
||||
* gstack-approvals list [--json] [--branch <name>]
|
||||
* gstack-approvals sync [--branch <name>]
|
||||
* gstack-approvals path [--branch <name>]
|
||||
@@ -18,9 +20,11 @@
|
||||
* `render` prints the `## Approvals` section /ship puts in the PR body. Logic:
|
||||
* lib/approval-record.ts.
|
||||
*/
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import * as path from 'node:path';
|
||||
import { EXIT, exitTableHelp } from '../lib/headless-artifacts';
|
||||
import { appendRows, branchExists, buildRow, importAutoplan, locate, readRecord, renderMode, renderRecord, repoVisibility, syncRecord, type RecordError, type StoreLocation } from '../lib/approval-record';
|
||||
import { GSTACK_ACTOR, openShipRun, RULED_NOTE_SCOPE, ruledNoteRow, ruledNotes, ruledNoteText } from '../lib/ruled-notes';
|
||||
import { resultLine } from '../lib/result-codes';
|
||||
|
||||
const HELP = `gstack-approvals — the per-thread approval record (capy/skill/gstack/captain.md)
|
||||
@@ -29,6 +33,12 @@ const HELP = `gstack-approvals — the per-thread approval record (capy/skill/gs
|
||||
[--pending <text>] [--outcome approved|rejected|pending|standing] [--date YYYY-MM-DD] [--source <ref>]
|
||||
import --autoplan <run dir> [--by <whose words, e.g. Garry>] [--stack <b1,b2,...>]
|
||||
--stack writes the same rows to every branch of a stack; each branch is checked first, and one unknown branch writes none
|
||||
rule --finding <fingerprint> --head <the stop head the owner ruled at> --reason <why it ships as a note, one line>
|
||||
--words <the owner's exact words> --by <who ruled>
|
||||
the owner's ruling that a review finding ships as a disclosed note (docs/ship-policy.md#ruled-notes); finish
|
||||
counts that finding closed on a fix-check whose range_base..commit holds the head, and the receipt and PR body
|
||||
list it; refused inside this checkout's open /ship run
|
||||
rulings the "Ruled note: …" lines the Pre-Landing Review and the PR body carry
|
||||
render [--no-heading] [--full] the ## Approvals section for a PR body or report; a public repository, or one whose
|
||||
visibility gh cannot read, gets only "Approved: yes|no (record <id>)", because a PR description keeps
|
||||
its edit history; --full prints the rows when gh cannot read the visibility and you know the repo is private
|
||||
@@ -43,7 +53,7 @@ Example:
|
||||
|
||||
${exitTableHelp()}`;
|
||||
|
||||
const VALUE_FLAGS = new Set(['--ask', '--words', '--by', '--scope', '--pending', '--outcome', '--date', '--source', '--branch', '--autoplan', '--stack']);
|
||||
const VALUE_FLAGS = new Set(['--ask', '--words', '--by', '--scope', '--pending', '--outcome', '--date', '--source', '--branch', '--autoplan', '--stack', '--finding', '--reason', '--head']);
|
||||
|
||||
function parse(args: string[]): { flags: Record<string, string>; bools: Set<string>; error?: string } {
|
||||
const flags: Record<string, string> = {};
|
||||
@@ -111,9 +121,23 @@ function main(argv: string[]): number {
|
||||
const json = bools.has('--json');
|
||||
const recorded = (where: StoreLocation) => `${where.file}${where.mirror ? ` mirror=${where.mirror}` : ''}`;
|
||||
|
||||
if (cmd === 'add' || cmd === 'import') {
|
||||
if (cmd === 'add' || cmd === 'import' || cmd === 'rule') {
|
||||
let rows;
|
||||
if (cmd === 'add') {
|
||||
if (cmd === 'rule') {
|
||||
const invalid = (message: string) => fail(cmd, { code: 'APPROVAL_RECORD_INVALID', message });
|
||||
const finding = (flags['--finding'] ?? '').trim(), reason = (flags['--reason'] ?? '').trim();
|
||||
if (!finding || /\s/.test(finding) || !reason) return invalid('rule needs --finding <the finding\'s fingerprint, no spaces> and --reason <why it ships as a note>');
|
||||
if (/[\x00-\x1f\x7f]/.test(reason)) return invalid('--reason is one line: no newline or control character');
|
||||
const head = spawnSync('git', ['rev-parse', '--verify', '--quiet', `${flags['--head'] ?? ''}^{commit}`], { encoding: 'utf8', timeout: 10_000 });
|
||||
if (!flags['--head'] || head.status !== 0) return invalid('rule needs --head <the stop head the owner ruled at, a commit in this repository>');
|
||||
if (GSTACK_ACTOR.test((flags['--by'] ?? '').trim())) return invalid('--by names gstack; a ruling is the owner\'s, so --by names who ruled');
|
||||
const open = openShipRun(process.cwd());
|
||||
if (open) return invalid(`/ship run ${open} is open on this checkout: a ruling is the owner's, never the lane's own runner's; record it from the owner's session`);
|
||||
const built = buildRow({ ...ruledNoteRow(finding, head.stdout.trim(), reason), words: flags['--words'] ?? '', by: flags['--by'] ?? '', date: flags['--date'] }, loc.branch);
|
||||
if ('code' in built) return fail(cmd, built);
|
||||
rows = [built];
|
||||
} else if (cmd === 'add') {
|
||||
if (RULED_NOTE_SCOPE.test(flags['--scope'] ?? '')) return fail(cmd, { code: 'APPROVAL_RECORD_INVALID', message: 'a ruled_note scope is recorded with gstack-approvals rule' });
|
||||
const built = buildRow({ ask: flags['--ask'] ?? '', words: flags['--words'] ?? '', by: flags['--by'] ?? '', scope: flags['--scope'] ?? '', pending: flags['--pending'], outcome: flags['--outcome'], date: flags['--date'], source: flags['--source'] }, loc.branch);
|
||||
if ('code' in built) return fail(cmd, built);
|
||||
rows = [built];
|
||||
@@ -126,7 +150,7 @@ function main(argv: string[]): number {
|
||||
const res = appendRows(loc, rows);
|
||||
if ('code' in res) return fail(cmd, res);
|
||||
if (json) process.stdout.write(JSON.stringify({ added: res.added.map(r => r.id), skipped: res.skipped, file: loc.file, mirror: loc.mirror ?? null }) + '\n');
|
||||
else if (cmd === 'add') process.stdout.write(`APPROVAL_RECORDED: ${rows[0]!.id}${res.skipped ? ' (already present)' : ''} ${recorded(loc)}\n`);
|
||||
else if (cmd !== 'import') process.stdout.write(`APPROVAL_RECORDED: ${rows[0]!.id}${res.skipped ? ' (already present)' : ''} ${recorded(loc)}\n`);
|
||||
else process.stdout.write(`APPROVAL_IMPORTED: n=${res.added.length} skipped=${res.skipped} ${recorded(loc)}\n`);
|
||||
if (res.mirrorError) return fail(cmd, res.mirrorError);
|
||||
return EXIT.ok;
|
||||
@@ -145,6 +169,11 @@ function main(argv: string[]): number {
|
||||
if (read.state === 'unavailable') return fail(cmd, { code: 'APPROVAL_RECORD_UNAVAILABLE', message: `no record at ${loc.file} and no mirror` });
|
||||
return EXIT.ok;
|
||||
}
|
||||
if (cmd === 'rulings') {
|
||||
const notes = ruledNotes(readRecord(loc).rows);
|
||||
process.stdout.write(notes.length ? notes.map(n => `Ruled note: ${ruledNoteText(n)}\n`).join('') : 'RULED_NOTES: none\n');
|
||||
return EXIT.ok;
|
||||
}
|
||||
if (cmd === 'sync') {
|
||||
const s = syncRecord(loc);
|
||||
process.stdout.write(`APPROVAL_SYNCED: rows=${s.rows} ${recorded(loc)}\n`);
|
||||
|
||||
@@ -56,6 +56,7 @@ import { RISK_CLASSES, type RiskClass } from '../lib/review-risk';
|
||||
import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose';
|
||||
import { resolveStateRoot } from '../lib/state-root';
|
||||
import { readPause, receiptBaseRed } from '../lib/base-red';
|
||||
import { readRuledNotes, ruledNoteText } from '../lib/ruled-notes';
|
||||
import { isRegistryUnavailable, loadRegistryFromBase } from '../lib/regen';
|
||||
|
||||
const HELP = `gstack-ship-receipt — the \`\`\`gstack-ship-receipt\`\`\` block (docs/ship-policy.md#ship-receipt)
|
||||
@@ -260,6 +261,9 @@ function cmdWrite(rest: string[], json: boolean): number {
|
||||
const line = reviewLoopExceptionLine(loopException.exception);
|
||||
if (bodyFile && !bodyText.split(/\r?\n/).some(l => l.trim() === line)) return refuse(`${bodyFile} does not carry the exception line verbatim: ${JSON.stringify(line)}`);
|
||||
}
|
||||
const ruledNotes = readRuledNotes(repoRoot);
|
||||
const unlisted = bodyFile ? ruledNotes.map(n => `Ruled note: ${ruledNoteText(n)}`).find(l => !bodyText.split(/\r?\n/).some(b => b.trim() === l)) : undefined;
|
||||
if (unlisted) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${bodyFile} does not list ruled note ${JSON.stringify(unlisted)} (gstack-approvals rulings prints each line the body carries); nothing written`) + '\n'); return EXIT.fail; }
|
||||
const declared = (flag(rest, '--declare-risk') ?? '').split(',').map(s => s.trim()).filter(Boolean);
|
||||
if (rest.includes('--declare-risk') && (!declared.length || declared.some(c => !(RISK_CLASSES as readonly string[]).includes(c)))) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `--declare-risk ${flag(rest, '--declare-risk') ?? ''}: each class is one of ${RISK_CLASSES.join(', ')}; nothing written`) + '\n'); return EXIT.fail; }
|
||||
const review = receiptReviewAtHead(repoRoot, base, path.dirname(Bun.fileURLToPath(import.meta.url)), headSha ?? head, declared as RiskClass[]);
|
||||
@@ -315,7 +319,7 @@ function cmdWrite(rest: string[], json: boolean): number {
|
||||
policy: flag(rest, '--policy') ?? tree?.policy, queue_mode: flag(rest, '--queue-mode'), preregistration: flag(rest, '--preregistration'), history: flag(rest, '--history'),
|
||||
...pregate, issues: issuesFile ? receiptIssuesFrom(issuesFile) : undefined, approval: approval.kind === 'ok' ? approval.approval : undefined,
|
||||
review_loop: reviewLoop, ...(docs?.ok ? { docs: docs.docs } : {}), ...product,
|
||||
gstack_version: installedGstackVersion(), review_depth: reviewDepth, review_loop_exception: loopException?.ok ? loopException.exception : undefined, review_targeted: targeted?.ok ? targeted.targeted : undefined,
|
||||
gstack_version: installedGstackVersion(), review_depth: reviewDepth, review_loop_exception: loopException?.ok ? loopException.exception : undefined, ruled_notes: ruledNotes.length ? ruledNotes : undefined, review_targeted: targeted?.ok ? targeted.targeted : undefined,
|
||||
base_red: pause ? receiptBaseRed(pause) : undefined,
|
||||
review_exclude: receiptExclude(review.exclude),
|
||||
no_stop: noStopArg(flag(rest, '--no-stop')),
|
||||
|
||||
@@ -0,0 +1 @@
|
||||
- **A finding the owner rules a disclosed note no longer keeps a granted fix-check from counting, and every ruling is listed.** On qm#3122 at 532cc5c the owner ruled two CRITICALs notes, not fixes. The runner logged them as `skipped`, `finish` counted a skipped CRITICAL as open, so the granted fix-check row (its own findings all below the floor) never read CURRENT and `finish` stopped with "behavior changed since review". The owner now records each ruling with `gstack-approvals rule --finding <fingerprint> --head <stop head> --reason <why> --words <exact words> --by <who ruled>`: an approved approval row with the ruler, the head, the reason and the time. `rule` refuses a head that is not a commit, a multi-line reason, a `--by` naming gstack and any ruling inside the checkout's open /ship run, and `add` refuses the ruling scope. `finish` reads rulings from the approval record only (a `ruled` field or `action: "ruled"` on the lane's own review-log row is ignored), treats a ruled finding as closed when the ruling's head lies inside the fix-check's range, whichever came first, and prints each as `RULED_NOTE:`; a ruling at an earlier head is listed as not applied. A skipped CRITICAL with no ruling stays open. `gstack-approvals rulings` prints the `Ruled note: …` lines for the Pre-Landing Review and the PR body; the receipt records them as `ruled_notes`, `gstack-ship-receipt write --body` refuses a body missing one, and the READY block prints each. See docs/ship-policy.md#ruled-notes. (GST-1537)
|
||||
@@ -95,7 +95,8 @@ incomplete coverage, which blocks until the reviewer is rerun.
|
||||
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
|
||||
finishes with its open notes listed in the PR body and receipt (an owner-ruled
|
||||
note is closed and listed: [ruled notes](ship-policy.md#ruled-notes)), 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). The cap limits fix rounds, not Step 11's voices: on gate
|
||||
|
||||
@@ -1608,6 +1608,41 @@ construction:
|
||||
the run and names the finding (path, severity, problem) for the owner instead
|
||||
of a third round; the owner may grant one scoped fix-check.
|
||||
|
||||
<a id="ruled-notes"></a>
|
||||
**Ruled notes (GST-1537).** R51's other answer to an open finding is the owner's
|
||||
ruling that it ships as a disclosed note, with the note listed. The owner records
|
||||
each ruling, one finding per row, through the approval record, outside the lane's
|
||||
/ship run, naming the stop head it ruled at (the commit a `review_loop_grant` names):
|
||||
|
||||
```bash
|
||||
gstack-approvals rule --finding '<the finding's fingerprint>' --head <stop head> \
|
||||
--reason '<why it ships as a note, one line>' --words '<the owner's exact words>' --by '<who ruled>'
|
||||
```
|
||||
|
||||
The row is an approved `gstack-approvals` row with scope `ruled_note <fingerprint> <head>`
|
||||
and ask `ruled_note <fingerprint> at <head>: <reason>`, with the head's full sha, so
|
||||
two findings ruled with the same reason in one reply get two rows, and a ruling at
|
||||
a later head is a new row. Its `by` is the ruler and its `recorded_at` the time.
|
||||
`rule` refuses a `--head` that is not a commit in the repository, a reason with a
|
||||
newline or control character, a `--by` naming gstack, and any ruling while this
|
||||
checkout holds an open /ship run's marker (the lane's own runner). `add` refuses a
|
||||
`ruled_note` scope, so `rule` is the only path. A review-log field never makes a
|
||||
ruling: `ruled`, `action: "ruled"` or any other claim the lane writes on its own
|
||||
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
|
||||
the newest `review-fix-check` row (action `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:
|
||||
`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
|
||||
line word for word, and the READY block prints each as `RULED_NOTE:`.
|
||||
|
||||
`gstack-review-scope summary --since <REVIEW_SINCE> --run <run dir>` reads the branch's review-log
|
||||
rows for this invocation and its dispatch ledgers, and writes the counters the ship receipt carries
|
||||
(`gstack-ship-receipt write … --review-loop @<file>`): `review_loop: { passes,
|
||||
|
||||
@@ -123,6 +123,8 @@ export interface ShipReceipt {
|
||||
review_depth?: { depth: 'platform' | 'light' | 'standard' | 'full'; class: 'docs' | 'tests' | 'code' | 'gate'; runtime_lines: number; rules: string; base: string; registry: string | null; gate_scan: { tagged: number | null; head: string }; reasons: string[]; risk?: string[]; mode?: 'lean' | 'panel' | 'platform'; platform_review?: { id: string; head: string; reviewer: string }; declared?: string[]; detected?: string[] };
|
||||
/** The owner's acceptance of a stop after a fix-check edit under frozen scope (#3559); the PR body repeats it word for word. */
|
||||
review_loop_exception?: { accepted_by: string; date: string; via: string; reason: string; fix_check?: { head: string; findings: number; files?: string[] }; second_pass?: { head: string; deferred: string[] } };
|
||||
/** The owner's rulings that a finding ships as a disclosed note (gstack-approvals rule, lib/ruled-notes.ts); the PR body lists each one word for word (GST-1537). */
|
||||
ruled_notes?: Array<{ finding: string; head: string; by: string; reason: string; at: string; approval: string }>;
|
||||
/** The targeted pass on money, gate, auth and secret code (lib/review-targeted.ts), bound to the receipt's product_patch_id; `done` only with its output collected. */
|
||||
review_targeted?: { classes: string[]; rules: string; brief_sha: string; product_patch_id: string; status: 'done' | 'failed' | 'not-run'; findings: number; raised_by: string };
|
||||
/** Step 5 failures gstack-base-red proved are the base's own (lib/base-red.ts): a conditional receipt the queue refuses until /ship --resume rewrites it without this block. */
|
||||
@@ -267,6 +269,7 @@ export const ARTIFACT_SCHEMAS = {
|
||||
review_targeted: { type: 'object', required: ['classes', 'rules', 'brief_sha', 'product_patch_id', 'status', 'findings', 'raised_by'], properties: { classes: { type: 'array', items: { enum: ['money', 'gate', 'auth', 'secret'] } }, rules: str, brief_sha: str, product_patch_id: str, status: { enum: ['done', 'failed', 'not-run'] }, findings: { type: 'integer', minimum: 0 }, raised_by: str } },
|
||||
review_exclude: { type: 'object', required: ['source', 'patterns', 'applied', 'excluded', 'excluded_bytes', 'kept', 'gate_tagged', 'patterns_sha256', 'list_sha256', 'digest'], properties: { source: str, patterns: strList, applied: strList, excluded: { type: 'integer', minimum: 0 }, excluded_bytes: { type: 'integer', minimum: 0 }, kept: { type: 'integer', minimum: 0 }, gate_tagged: strList, patterns_sha256: str, list_sha256: str, digest: str } },
|
||||
review_loop_exception: { type: 'object', required: ['accepted_by', 'date', 'via', 'reason'], properties: { accepted_by: str, date: str, via: str, reason: str, fix_check: { type: 'object', required: ['head', 'findings'], properties: { head: str, findings: { type: 'integer', minimum: 0 }, files: strList } }, second_pass: { type: 'object', required: ['head', 'deferred'], properties: { head: str, deferred: strList } } } },
|
||||
ruled_notes: { type: 'array', items: { type: 'object', required: ['finding', 'head', 'by', 'reason', 'at', 'approval'], properties: { finding: str, head: str, by: str, reason: str, at: str, approval: str } } },
|
||||
no_stop: { type: 'array', items: { type: 'object', required: ['class', 'reason', 'evidence'], properties: { class: { enum: [...NO_STOP_CLASSES] }, reason: str, evidence: str } } },
|
||||
issues: { type: 'object', properties: { fixes: { type: 'array', items: { type: 'integer' } }, refs: { type: 'array', items: { type: 'integer' } }, flakes: { type: 'array', items: { type: 'object', required: ['issue', 'run'], properties: { issue: { type: 'integer' }, run: str } } }, related: { type: 'array', items: { type: 'object', required: ['pr', 'author', 'closes'], properties: { pr: { type: 'integer' }, repo: str, author: str, closes: { type: 'array', items: { type: 'integer' } }, supersedes: { type: 'boolean' }, credit: { type: 'boolean' } } } } } },
|
||||
},
|
||||
|
||||
+22
-4
@@ -335,7 +335,26 @@ export function bindReview(rec: Record<string, any>, token: string, env = proces
|
||||
return rec;
|
||||
}
|
||||
|
||||
export function reviewFreshness(rec: Record<string, any>, currentWtree: string): { status: string; reason: string } | undefined {
|
||||
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']);
|
||||
|
||||
/**
|
||||
* 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.
|
||||
*/
|
||||
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;
|
||||
return { ...rec, status: 'clean', issues_found: 0, critical: 0, informational: 0, findings: rec.findings.filter(f => !isRuled(f)) };
|
||||
}
|
||||
|
||||
export function reviewFreshness(rec: Record<string, any>, currentWtree: string, ruled: ReadonlySet<string> = new Set()): { status: string; reason: string } | undefined {
|
||||
if (!DIFF_REVIEWS.has(rec.skill)) return;
|
||||
if (rec.skill === 'ship') return { status: 'UNVERIFIED', reason: 'ship telemetry is not a review pass' };
|
||||
const binding = rec.review_binding;
|
||||
@@ -354,12 +373,11 @@ export function reviewFreshness(rec: Record<string, any>, currentWtree: string):
|
||||
// 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);
|
||||
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)) ||
|
||||
(Array.isArray(rec.findings) && rec.findings.some(f => record(f) && (f.action === 'open' ||
|
||||
((f.action === 'deferred' || f.action === 'noted') && (f.blocking === true || isCriticalFinding(f))) ||
|
||||
(!f.gate && isCriticalFinding(f) && isOpenByAction(f)))))) {
|
||||
(Array.isArray(rec.findings) && rec.findings.some(unresolvedFinding))) {
|
||||
return { status: 'UNVERIFIED', reason: 'review has unresolved findings or did not finish clean' };
|
||||
}
|
||||
return { status: 'CURRENT', reason: 'completed clean pass on unchanged content' };
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
/**
|
||||
* Owner-ruled notes (GST-1537, docs/ship-policy.md#ruled-notes): the owner's ruling that a review finding ships as a
|
||||
* disclosed note instead of a fix. A ruling is one approved gstack-approvals row (`gstack-approvals rule`), with scope
|
||||
* `ruled_note <finding fingerprint> <head>` and ask `ruled_note <finding fingerprint> at <head>: <reason>`, where head is the
|
||||
* full sha of the stop head the owner ruled at; the row's `by` is the ruler and its `recorded_at` the time. A review-log field
|
||||
* never makes one, so the lane that logged a finding cannot close it.
|
||||
*/
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import { locate, readRecord, type ApprovalRow, type Env } from './approval-record';
|
||||
|
||||
export interface RuledNote { finding: string; head: string; by: string; reason: string; at: string; approval: string }
|
||||
|
||||
const SCOPE = /^ruled_note (\S+) ([0-9a-f]{40})$/;
|
||||
const ASK = /^ruled_note (\S+) at ([0-9a-f]{40}): (\S(?:.*\S)?)$/;
|
||||
/** A ruler gstack names for itself (the no-stop default's `gstack no-stop default (K1)`) is no owner. */
|
||||
export const GSTACK_ACTOR = /^gstack\b/i;
|
||||
export const RULED_NOTE_SCOPE = /^\s*ruled_note\b/;
|
||||
|
||||
export const ruledNoteRow = (finding: string, head: string, reason: string) => ({ scope: `ruled_note ${finding} ${head}`, ask: `ruled_note ${finding} at ${head}: ${reason}` });
|
||||
|
||||
/** The approved ruling rows, one per finding and head (the newest wins). */
|
||||
export function ruledNotes(rows: readonly ApprovalRow[]): RuledNote[] {
|
||||
const byFinding = new Map<string, RuledNote>();
|
||||
for (const r of rows) {
|
||||
const scope = SCOPE.exec(r.scope), ask = ASK.exec(r.ask);
|
||||
if (r.outcome !== 'approved' || !scope || !ask || ask[1] !== scope[1] || ask[2] !== scope[2] || GSTACK_ACTOR.test(r.by)) continue;
|
||||
const finding = scope[1]!, head = scope[2]!, key = `${finding} ${head}`;
|
||||
byFinding.delete(key);
|
||||
byFinding.set(key, { finding, head, by: r.by, reason: ask[3]!, at: r.recorded_at, approval: r.id });
|
||||
}
|
||||
return [...byFinding.values()];
|
||||
}
|
||||
|
||||
/** The rulings in the checked-out branch's approval record (and its mirror). */
|
||||
export function readRuledNotes(cwd: string, env: Env = process.env): RuledNote[] {
|
||||
const loc = locate({ cwd, env });
|
||||
return 'code' in loc ? [] : ruledNotes(readRecord(loc).rows);
|
||||
}
|
||||
|
||||
/** The one-line form finish prints after `RULED_NOTE: ` and the PR body carries after `Ruled note: `. */
|
||||
export const ruledNoteText = (n: RuledNote) => `${n.finding} at ${n.head.slice(0, 12)} ruled by ${n.by} at ${n.at}: ${n.reason} (approval ${n.approval})`;
|
||||
|
||||
/** The /ship run open on this checkout (its marker in the git dir, which finish removes), if any. */
|
||||
export function openShipRun(cwd: string): string | undefined {
|
||||
const p = spawnSync('git', ['rev-parse', '--git-path', 'gstack-ship-run'], { cwd, encoding: 'utf8', timeout: 10_000 });
|
||||
if (p.status !== 0 || !p.stdout.trim()) return undefined;
|
||||
try { return fs.readFileSync(path.resolve(cwd, p.stdout.trim()), 'utf8').trim() || undefined; } catch { return undefined; }
|
||||
}
|
||||
@@ -9,6 +9,7 @@ import { APPROVALS_FOOTER_END, APPROVALS_TABLE_HEADER } from './approval-record'
|
||||
import { RECEIPT_FENCE } from './ship-receipt';
|
||||
import { parseReviewLoopException, reviewLoopExceptionLine } from './ship-receipt-depth';
|
||||
import { docsExceptionLine, parseReceiptDocs } from './ship-receipt-docs';
|
||||
import { ruledNoteText, type RuledNote } from './ruled-notes';
|
||||
|
||||
export interface ProseClaim { field: string; prose: number; receipt: number }
|
||||
export interface ProseCheck { checked: number; mismatches: ProseClaim[] }
|
||||
@@ -95,7 +96,7 @@ function withoutApprovalsBlocks(body: string): string {
|
||||
|
||||
/**
|
||||
* The body without the exception lines the receipt's own exceptions require: the `Review-loop exception: …` line
|
||||
* (GST-977) and the `Documentation exception: …` line (GST-983). write and the receipt check want each word for word,
|
||||
* (GST-977), the `Documentation exception: …` line (GST-983) and each `Ruled note: …` line (GST-1537). write and the receipt check want each word for word,
|
||||
* and each quotes the owner's reason (`outside 1 and 2 skip detector`), not this PR's counters. Only a line whose
|
||||
* trimmed text is a rendered line, the test write applies, is left out; the same words inside a longer line stay
|
||||
* prose. A marker line takes its place so the text on either side never joins into a claim.
|
||||
@@ -107,6 +108,7 @@ function withoutExceptionLines(body: string, receipt: Record<string, unknown>):
|
||||
const docs = receipt.docs === undefined ? undefined : parseReceiptDocs(receipt.docs);
|
||||
const docsLine = docs?.ok ? docsExceptionLine(docs.docs) : null;
|
||||
if (docsLine) lines.add(docsLine);
|
||||
if (Array.isArray(receipt.ruled_notes)) for (const n of receipt.ruled_notes) lines.add(`Ruled note: ${ruledNoteText(n as RuledNote)}`);
|
||||
if (!lines.size) return body;
|
||||
return body.split(/(?<=\n)/).map(l => (lines.has(l.trim()) ? '(exception line)\n' : l)).join('');
|
||||
}
|
||||
|
||||
+3
-1
@@ -12,6 +12,7 @@ import { approvalSummary } from './approval-citations';
|
||||
import { FRAGMENT_README } from './changelog-fragments';
|
||||
import { renderIssueLines } from './issue-links';
|
||||
import { matchesAny } from './ship-policy';
|
||||
import { ruledNoteText } from './ruled-notes';
|
||||
import { stampFileKind, versionStampOnly } from './pregate/stamp-only';
|
||||
import { stampOnlyLine, type BranchSide } from './stamp-only';
|
||||
|
||||
@@ -20,7 +21,7 @@ const KEY_ORDER: Array<keyof ShipReceipt | 'docs'> = [
|
||||
'schema_version', 'pr', 'head', 'version', 'base', 'gated_tree', 'predecessor', 'tree', 'gate_reuse', 'gate', 'ci', 'ci_carried', 'spend_usd',
|
||||
'session_kind', 'artifacts_consumed', 'run', 'thread', 'policy', 'queue_mode', 'preregistration', 'history',
|
||||
'head_tree', 'product_patch_id', 'gstack_version', 'review_depth', 'pregate', 'requires_remote', 'issues', 'approval', 'docs', 'review_loop',
|
||||
'review_loop_exception', 'review_targeted', 'review_exclude', 'carry', 'base_red', 'flaky_exception', 'no_stop',
|
||||
'review_loop_exception', 'ruled_notes', 'review_targeted', 'review_exclude', 'carry', 'base_red', 'flaky_exception', 'no_stop',
|
||||
];
|
||||
|
||||
/** Keys in the fixed order so identifiers lead; undefined values are dropped. */
|
||||
@@ -173,6 +174,7 @@ export function readyTailLines(r: ShipReceipt | undefined): string[] {
|
||||
const legacy = !!r && !r.review_depth;
|
||||
return [
|
||||
loop ? `REVIEW_LOOP: passes=${loop.passes} blocking_found=${loop.blocking_found} deferred=${loop.deferred} noted=${loop.noted ?? 0} fixed=${loop.fixed ?? 0}${opt('unrecorded')}${opt('unbound')}${opt('fix_checks')}` : 'REVIEW_LOOP: none in the receipt',
|
||||
...(r?.ruled_notes ?? []).map(n => `RULED_NOTE: ${oneLine(ruledNoteText(n))}`),
|
||||
'TRIAL_MERGES: <one line per open sibling pin that shares a file: its git merge-tree result and the resolution, or "none share a file">',
|
||||
'Attention:',
|
||||
legacy ? 'reviewed by <model on another family> (risk class unknown: the receipt records no review_depth; rule 27 cross-family trail read)'
|
||||
|
||||
+9
-4
@@ -19,6 +19,7 @@ import { baseHolds, boundedPaths, comparePath, contextPacket, gateEditedPaths, i
|
||||
import { heldBy } from './ship-receipt';
|
||||
import { locate, readRecord } from './approval-record';
|
||||
import { reviewFreshness } from './review-evidence';
|
||||
import { readRuledNotes, ruledNoteText } from './ruled-notes';
|
||||
import { parseReviewLoopException, type ReviewLoopException } from './ship-receipt-depth';
|
||||
import { parseReceiptDocs, type ReceiptDocs } from './ship-receipt-docs';
|
||||
import { resolveStateRoot } from './state-root';
|
||||
@@ -197,8 +198,9 @@ function harnessRepin(cwd: string, from: string, to: string, p: string): boolean
|
||||
* 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 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.
|
||||
* head inside range_base..commit: the owner's granted fix-check after Step 11 is a review (GST-1130). A finding on that row the
|
||||
* owner ruled a disclosed note (an approved gstack-approvals rule row whose stop head lies inside the row's range_base..commit, never the row's own claim) is closed, and every ruling
|
||||
* prints as RULED_NOTE (GST-1537). Authored docs keep the stage 2.2 recheck.
|
||||
*/
|
||||
type FixCheckException = ReviewLoopException & { fix_check: NonNullable<ReviewLoopException['fix_check']> };
|
||||
type Routed = Record<'behavior' | 'docs' | 'metadata' | 'tests' | 'base' | 'context' | 'bounded' | 'accepted' | 'fix_checked', string[]>;
|
||||
@@ -281,11 +283,14 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: Review
|
||||
const commitOf = (c: unknown) => typeof c === 'string' && /^[0-9a-f]{7,64}$/.test(c) ? git(cwd, ['rev-parse', '--verify', '--quiet', `${c}^{commit}`]).stdout.trim() : '';
|
||||
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 inFixRange = (c: string) => !!fixFrom && !!fixCommit && ancestor(fixFrom, c) && ancestor(c, fixCommit);
|
||||
const rulings = readRuledNotes(cwd).map(n => ({ ...n, applied: !!fixRow && inFixRange(n.head) }));
|
||||
for (const n of rulings) r.add(`RULED_NOTE: ${ruledNoteText(n)}${fixRow && !n.applied ? ' (outside this fix-check\'s range; not applied)' : ''}`);
|
||||
const fresh = fixRow && reviewFreshness(fixRow, fixRow.wtree, new Set(rulings.filter(n => n.applied).map(n => n.finding)));
|
||||
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); }));
|
||||
&& (`${g.ask} ${g.scope}`.match(/\b[0-9a-f]{7,40}\b/g) ?? []).some(s => { const c = commitOf(s); return !!c && inFixRange(c); }));
|
||||
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'
|
||||
|
||||
@@ -403,6 +403,10 @@ export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }>
|
||||
file: 'test/ship-finish-stack-parent.test.ts',
|
||||
reason: 'drives bin/gstack-ship-release, gstack-docs-candidate and gstack-wtree (shebangs) through test/helpers/ship-bin-fixture.ts like test/ship-rereview-triggers.test.ts; windows-latest fails every case at the first bin spawn (windows-scoped-shard, run 41950236362)',
|
||||
},
|
||||
{
|
||||
file: 'test/ship-finish-ruled-notes.test.ts',
|
||||
reason: 'drives bin/gstack-ship-release, gstack-approvals, gstack-docs-candidate and gstack-wtree (shebangs) through test/helpers/ship-bin-fixture.ts like test/ship-rereview-triggers.test.ts; windows-latest fails every case at the first bin spawn (windows-scoped-shard, run 42139467391)',
|
||||
},
|
||||
];
|
||||
|
||||
// Force-include overrides: files a WINDOWS_FRAGILE_PATTERNS regex excludes for
|
||||
|
||||
@@ -102,6 +102,8 @@ ZERO-RUN lane never reaches this body unresolved.>
|
||||
|
||||
## Pre-Landing Review
|
||||
<findings from Step 9 code review, or "No issues found."; then Step 9.4's `REVIEW_LOOP:` line.>
|
||||
<Each `Ruled note: …` line `gstack-approvals rulings` prints, verbatim: a finding the owner ruled a disclosed note,
|
||||
with its ruler and reason. `gstack-ship-receipt write --body` refuses a body missing one.>
|
||||
<Gate edits: Step 9's latest `Gate edits:` line verbatim — `none detected (...)`,
|
||||
`N listed, M read, K findings — J kept, R restored, O open`, `partial (...)` or
|
||||
`UNAVAILABLE — ...`; never `none detected` for partial or UNAVAILABLE. Under it, each
|
||||
@@ -232,6 +234,9 @@ or a body missing the exception line, is `DOCS_AUDIT_BLOCKED` with nothing writt
|
||||
When the owner accepted a stop after a fix-check edit, add
|
||||
`--review-loop-exception @<file>` (`{accepted_by, date, via, reason, fix_check: {head, findings}}`)
|
||||
and put its printed `Review-loop exception:` line in the body; never a prose line under `REVIEW_LOOP:`.
|
||||
A finding the owner rules a disclosed note instead of a fix is closed only by the owner's
|
||||
`gstack-approvals rule`, recorded outside this run, never by this run or a review-log field
|
||||
(docs/ship-policy.md#ruled-notes); the receipt lists each one as `ruled_notes`.
|
||||
When `gstack-review-dispatch verify` printed `REVIEW_EXCLUDE_RECORD: digest=<d>`, add `--review-exclude <d>`
|
||||
and Step 14.5's `--docs-post <audit-id>-post.json` and `--docs-pre <its Prepare snapshot>`; a digest that no longer matches HEAD is
|
||||
`RECEIPT_INVALID field=review_exclude`: rerun the review, not the write, unless only the docs audit's
|
||||
|
||||
@@ -93,6 +93,8 @@ ZERO-RUN lane never reaches this body unresolved.>
|
||||
|
||||
## Pre-Landing Review
|
||||
<findings from Step 9 code review, or "No issues found."; then Step 9.4's `REVIEW_LOOP:` line.>
|
||||
<Each `Ruled note: …` line `gstack-approvals rulings` prints, verbatim: a finding the owner ruled a disclosed note,
|
||||
with its ruler and reason. `gstack-ship-receipt write --body` refuses a body missing one.>
|
||||
<Gate edits: Step 9's latest `Gate edits:` line verbatim — `none detected (...)`,
|
||||
`N listed, M read, K findings — J kept, R restored, O open`, `partial (...)` or
|
||||
`UNAVAILABLE — ...`; never `none detected` for partial or UNAVAILABLE. Under it, each
|
||||
@@ -215,6 +217,9 @@ or a body missing the exception line, is `DOCS_AUDIT_BLOCKED` with nothing writt
|
||||
When the owner accepted a stop after a fix-check edit, add
|
||||
`--review-loop-exception @<file>` (`{accepted_by, date, via, reason, fix_check: {head, findings}}`)
|
||||
and put its printed `Review-loop exception:` line in the body; never a prose line under `REVIEW_LOOP:`.
|
||||
A finding the owner rules a disclosed note instead of a fix is closed only by the owner's
|
||||
`gstack-approvals rule`, recorded outside this run, never by this run or a review-log field
|
||||
(docs/ship-policy.md#ruled-notes); the receipt lists each one as `ruled_notes`.
|
||||
When `gstack-review-dispatch verify` printed `REVIEW_EXCLUDE_RECORD: digest=<d>`, add `--review-exclude <d>`
|
||||
and Step 14.5's `--docs-post <audit-id>-post.json` and `--docs-pre <its Prepare snapshot>`; a digest that no longer matches HEAD is
|
||||
`RECEIPT_INVALID field=review_exclude`: rerun the review, not the write, unless only the docs audit's
|
||||
|
||||
@@ -0,0 +1,195 @@
|
||||
/**
|
||||
* Owner-ruled notes (GST-1537, CAPY-1 qm#3122 at 532cc5c): a CRITICAL the owner rules a disclosed note is closed for
|
||||
* finish once the ruling is recorded through gstack-approvals, and only then. The lane's own review-log row can say
|
||||
* whatever it likes about a ruling; finish reads rulings from the approval record alone, and the receipt and PR body list
|
||||
* every one with its ruler and reason.
|
||||
*/
|
||||
import { afterEach, describe, expect, setDefaultTimeout, test } from 'bun:test';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import { EXIT } from '../lib/headless-artifacts';
|
||||
import { buildRow } from '../lib/approval-record';
|
||||
import { ruledNotes, ruledNoteText } from '../lib/ruled-notes';
|
||||
import { extractReceipt } from '../lib/ship-receipt';
|
||||
import { lineOf, RELEASE, shipFixture, type ShipFx } from './helpers/ship-bin-fixture';
|
||||
|
||||
setDefaultTimeout(90_000);
|
||||
const BIN = path.dirname(RELEASE);
|
||||
const LANE = 'tests=printf " 3 pass\\n 0 fail\\nRan 3 tests across 1 file. [5.00ms]\\n"';
|
||||
const ships: ShipFx[] = [];
|
||||
afterEach(() => { while (ships.length) ships.pop()!.cleanup(); });
|
||||
|
||||
const RACE = 'src/x.ts:1:edit-share-race';
|
||||
const AMBIENT = 'src/x.ts:1:ambient-turns';
|
||||
const TEST_V1 = "import { expect, test } from 'bun:test';\ntest('a', () => { expect(2).toBe(2); });\n";
|
||||
const TEST_V2 = "import { expect, test } from 'bun:test';\ntest('a', () => { expect(2).toBe(2); }, 1_000);\n";
|
||||
/** The qm#3122 fix-check row: two outside-voice CRITICALs the owner ruled notes (logged skipped) and the fix-check's own two below-floor notes. */
|
||||
const QM3122 = {
|
||||
completed: true, converged: true, status: 'issues_found', issues_found: 4, critical: 2, informational: 2,
|
||||
findings: [
|
||||
{ fingerprint: RACE, severity: 'CRITICAL', action: 'skipped', raised_by: 'outside-voice', blocking: true },
|
||||
{ fingerprint: AMBIENT, severity: 'CRITICAL', action: 'skipped', raised_by: 'outside-voice', blocking: true },
|
||||
{ fingerprint: 'src/x.ts:1:naming', severity: 'INFORMATIONAL', action: 'noted', raised_by: 'fix-check', blocking: false },
|
||||
{ fingerprint: 'test/x.test.ts:2:coverage', severity: 'INFORMATIONAL', action: 'noted', raised_by: 'fix-check', blocking: false },
|
||||
],
|
||||
};
|
||||
const NOT_CLEAN = 'FIX_CHECK: not counted (review has unresolved findings or did not finish clean)';
|
||||
|
||||
const run = (f: ShipFx, bin: string, args: string[]) => f.run(path.join(BIN, bin), args, { GSTACK_APPROVALS_MIRROR: '0' });
|
||||
const finish = (f: ShipFx, reviewed: string, post: string) => f.run(RELEASE, ['finish', '--base', 'main', '--reviewed-tree', reviewed, '--docs-post', post, '--lane', LANE]);
|
||||
const rule = (f: ShipFx, finding: string, head: string, by = 'qm minder (CAPY-1)') => run(f, 'gstack-approvals', ['rule', '--finding', finding, '--head', head, '--reason', `${finding.split(':')[2]} ships as a disclosed note`, '--words', 'both are notes, not fixes', '--by', by, '--json']);
|
||||
const logFixCheck = (f: ShipFx, row: Record<string, unknown>) => {
|
||||
const start = run(f, 'gstack-review-log', ['--start', 'review-fix-check']);
|
||||
expect(start.status).toBe(0);
|
||||
const logged = run(f, 'gstack-review-log', [JSON.stringify({ skill: 'review-fix-check', timestamp: '2026-11-05T00:00:00Z', via: 'ship', review_scope: 'frozen', ...row }), '--finish', start.stdout.trim()]);
|
||||
expect(logged.status, logged.stderr).toBe(0);
|
||||
};
|
||||
/** Round 2 stopped at passHead, the owner granted one scoped fix-check, the fix landed, and the fix-check read it. */
|
||||
function grantedFixCheck(row: Record<string, unknown>, beforeLog?: (f: ShipFx, passHead: string) => void) {
|
||||
const f = shipFixture({ 'src/x.ts': 'export const x = 1;\n' }, { 'src/x.ts': 'export const x = 2;\n', 'test/x.test.ts': TEST_V1 });
|
||||
ships.push(f);
|
||||
const passHead = f.git('rev-parse', 'HEAD').stdout.trim();
|
||||
const reviewed = f.run(path.join(BIN, 'gstack-wtree'), []).stdout.trim();
|
||||
f.write({ 'src/x.ts': 'export const x = 3;\n', 'test/x.test.ts': TEST_V2 }); f.commit('fix the round-2 blocker');
|
||||
const fixHead = f.git('rev-parse', 'HEAD').stdout.trim();
|
||||
const post = path.join(f.root, 'post.json');
|
||||
expect(f.run(path.join(BIN, 'gstack-docs-candidate'), ['snapshot', '--out', post, '--audit-id', 'a1', '--mode', 'edit', '--base', 'main', '--docs', 'README.md']).status).toBe(0);
|
||||
const grant = run(f, 'gstack-approvals', ['add', '--ask', `Owner grant: one scoped fix-check after round 2 (review_loop_grant ${passHead.slice(0, 12)})`, '--words', 'one scoped fix-check', '--by', 'qm minder (CAPY-1)', '--scope', 'this /ship invocation', '--json']);
|
||||
expect(grant.status).toBe(0);
|
||||
beforeLog?.(f, passHead);
|
||||
logFixCheck(f, { range_base: passHead, ...row });
|
||||
return { f, passHead, fixHead, reviewed, post, grant: JSON.parse(grant.stdout).added[0] as string };
|
||||
}
|
||||
|
||||
// Value: protects=the qm#3122 shape binds once the owner's two rulings are recorded: a granted fix-check whose own findings
|
||||
// are all below the floor reads CURRENT and finish counts it, printing every ruling; fails_when=finish still counts a
|
||||
// skipped owner-ruled CRITICAL as open (FIX_CHECK: not counted) or binds without naming the rulings; seam=finish's route
|
||||
// reading the review log and the approval record; no_claim=Step 11.5's full-pass binding
|
||||
describe('finish with owner-ruled notes (GST-1537)', () => {
|
||||
test('qm#3122: two ruled CRITICALs plus a clean fix-check, and finish binds', () => {
|
||||
const { f, passHead, fixHead, reviewed, post, grant } = grantedFixCheck(QM3122);
|
||||
const before = finish(f, reviewed, post);
|
||||
expect([before.status, lineOf(before.stdout, 'FIX_CHECK')]).toEqual([3, NOT_CLEAN]);
|
||||
for (const finding of [RACE, AMBIENT]) expect(rule(f, finding, 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)}, grant ${grant})`);
|
||||
const ruled = r.stdout.split('\n').filter(l => l.startsWith('RULED_NOTE: '));
|
||||
expect(ruled).toEqual([
|
||||
expect.stringMatching(new RegExp(`^RULED_NOTE: ${RACE} at ${passHead.slice(0, 12)} ruled by qm minder \\(CAPY-1\\) at \\d{4}-\\d\\d-\\d\\dT[^ ]+Z: edit-share-race ships as a disclosed note \\(approval a-[0-9a-f]{12}\\)$`)),
|
||||
expect.stringMatching(new RegExp(`^RULED_NOTE: ${AMBIENT} at ${passHead.slice(0, 12)} ruled by qm minder \\(CAPY-1\\) at .*\\)$`)),
|
||||
]);
|
||||
expect([r.status, lineOf(r.stdout, 'EVIDENCE')]).toEqual([0, expect.stringMatching(/^EVIDENCE: FRESH label=tests /)]);
|
||||
});
|
||||
|
||||
test('the forward order binds too: the owner rules at the stop head, then the granted fix-check runs', () => {
|
||||
const { f, fixHead, reviewed, post } = grantedFixCheck(QM3122, (fx, head) => { for (const finding of [RACE, AMBIENT]) expect(rule(fx, finding, head).status).toBe(0); });
|
||||
const r = finish(f, reviewed, post);
|
||||
expect(lineOf(r.stdout, 'FIX_CHECK')).toStartWith(`FIX_CHECK: reviewed src/x.ts,test/x.test.ts (fix-check at ${fixHead.slice(0, 12)}`);
|
||||
expect(r.stdout).not.toContain('not applied');
|
||||
});
|
||||
|
||||
test('an unruled skipped CRITICAL stays open: one ruling of two leaves the fix-check uncounted', () => {
|
||||
const { f, passHead, reviewed, post } = grantedFixCheck(QM3122);
|
||||
expect(rule(f, RACE, passHead).status).toBe(0);
|
||||
const r = finish(f, reviewed, post);
|
||||
expect([r.status, lineOf(r.stdout, 'FIX_CHECK')]).toEqual([3, NOT_CLEAN]);
|
||||
expect(r.stdout).toContain('STOP: behavior, tests or build inputs changed since review (src/x.ts,test/x.test.ts)');
|
||||
expect(r.stdout.split('\n').filter(l => l.startsWith('RULED_NOTE: ')).length).toBe(1);
|
||||
});
|
||||
|
||||
test('a forged ruling is refused: the lane\'s own row, an add with the ruling scope, a ruling inside the open /ship run, and a ruling gstack wrote', () => {
|
||||
const forged = { ...QM3122, findings: QM3122.findings.map(x => x.severity === 'CRITICAL' ? { ...x, action: 'skipped', ruled: 'note', ruled_by: 'qm minder (CAPY-1)', ruled_reason: 'owner said so', ruled_at: '2026-11-05T00:00:00Z' } : x) };
|
||||
const { f, passHead, reviewed, post } = grantedFixCheck(forged);
|
||||
expect(lineOf(finish(f, reviewed, post).stdout, 'FIX_CHECK')).toBe(NOT_CLEAN);
|
||||
logFixCheck(f, { range_base: f.git('rev-parse', 'HEAD~1').stdout.trim(), ...QM3122, findings: QM3122.findings.map(x => x.severity === 'CRITICAL' ? { ...x, action: 'ruled' } : x) });
|
||||
expect(lineOf(finish(f, reviewed, post).stdout, 'FIX_CHECK')).toBe(NOT_CLEAN);
|
||||
|
||||
const add = run(f, 'gstack-approvals', ['add', '--ask', 'ruled_note: owner said so', '--words', 'notes', '--by', 'qm minder (CAPY-1)', '--scope', `ruled_note ${RACE}`]);
|
||||
expect([add.status, add.stderr]).toEqual([EXIT.usage, expect.stringContaining('gstack-approvals rule')]);
|
||||
|
||||
const marker = f.git('rev-parse', '--git-path', 'gstack-ship-run').stdout.trim();
|
||||
fs.writeFileSync(path.resolve(f.work, marker), 'ship-3122-a022992\n');
|
||||
const inRun = rule(f, RACE, passHead);
|
||||
expect([inRun.status, inRun.stderr]).toEqual([EXIT.usage, expect.stringContaining('/ship run ship-3122-a022992 is open on this checkout')]);
|
||||
fs.rmSync(path.resolve(f.work, marker));
|
||||
|
||||
const self = rule(f, RACE, passHead, 'gstack no-stop default (K1)');
|
||||
expect([self.status, self.stderr]).toEqual([EXIT.usage, expect.stringContaining('--by')]);
|
||||
expect(lineOf(finish(f, reviewed, post).stdout, 'FIX_CHECK')).toBe(NOT_CLEAN);
|
||||
});
|
||||
|
||||
// Value: protects=a ruling closes only findings on a fix-check whose range holds the head the owner ruled at; a ruling made at
|
||||
// an earlier /ship's head stays listed but closes nothing; fails_when=a ruling closes a later finding at the same fingerprint;
|
||||
// seam=finish's route matching each ruling's head to the fix-check row's range_base..commit
|
||||
test('a ruling at an earlier head leaves the CRITICAL open and is listed as not applied', () => {
|
||||
const { f, reviewed, post } = grantedFixCheck(QM3122);
|
||||
const mergeBase = f.git('merge-base', 'main', 'HEAD').stdout.trim();
|
||||
for (const finding of [RACE, AMBIENT]) expect(rule(f, finding, mergeBase).status).toBe(0);
|
||||
const r = finish(f, reviewed, post);
|
||||
expect([r.status, lineOf(r.stdout, 'FIX_CHECK')]).toEqual([3, NOT_CLEAN]);
|
||||
expect(r.stdout.split('\n').filter(l => l.startsWith('RULED_NOTE: ')).map(l => l.endsWith(" (outside this fix-check's range; not applied)"))).toEqual([true, true]);
|
||||
});
|
||||
|
||||
test('rule refuses a missing --head, a head that is not a commit and a multi-line reason', () => {
|
||||
const { f, passHead } = grantedFixCheck(QM3122);
|
||||
const tree = f.git('rev-parse', 'HEAD^{tree}').stdout.trim();
|
||||
for (const args of [['--finding', RACE], ['--finding', RACE, '--head', '0123456789abcdef'], ['--finding', RACE, '--head', tree]]) {
|
||||
const r = run(f, 'gstack-approvals', ['rule', ...args, '--reason', 'a note', '--words', 'notes', '--by', 'qm minder (CAPY-1)']);
|
||||
expect([r.status, r.stderr]).toEqual([EXIT.usage, expect.stringContaining('--head')]);
|
||||
}
|
||||
const lines = run(f, 'gstack-approvals', ['rule', '--finding', RACE, '--head', passHead, '--reason', 'first line\nsecond line', '--words', 'notes', '--by', 'qm minder (CAPY-1)']);
|
||||
expect([lines.status, lines.stderr]).toEqual([EXIT.usage, expect.stringContaining('one line')]);
|
||||
expect(run(f, 'gstack-approvals', ['rulings']).stdout).toBe('RULED_NOTES: none\n');
|
||||
});
|
||||
|
||||
// Value: protects=one owner reply ruling two findings with the same reason records both; fails_when=the second ruling
|
||||
// hashes to the first row's id and is skipped as already present, leaving its finding open; seam=gstack-approvals rule
|
||||
test('two rulings with the same reason, words and ruler both record', () => {
|
||||
const { f, passHead } = grantedFixCheck(QM3122);
|
||||
const same = (finding: string) => run(f, 'gstack-approvals', ['rule', '--finding', finding, '--head', passHead, '--reason', 'both ship as disclosed notes', '--words', 'both are notes, not fixes', '--by', 'qm minder (CAPY-1)', '--json']);
|
||||
const [a, b] = [same(RACE), same(AMBIENT)];
|
||||
expect([a.status, b.status, JSON.parse(b.stdout).skipped]).toEqual([0, 0, 0]);
|
||||
expect(run(f, 'gstack-approvals', ['rulings']).stdout.trim().split('\n')).toEqual([expect.stringMatching(new RegExp(`^Ruled note: ${RACE} at `)), expect.stringMatching(new RegExp(`^Ruled note: ${AMBIENT} at `))]);
|
||||
});
|
||||
|
||||
// Value: protects=nothing ruled is hidden (R73): the receipt records every ruling and write refuses a PR body that does not
|
||||
// list each one word for word; fails_when=write drops the rulings or accepts a body without them; seam=gstack-ship-receipt write
|
||||
test('the receipt records every ruling, and write refuses a body that does not list each one', () => {
|
||||
const { f, passHead } = grantedFixCheck(QM3122);
|
||||
for (const finding of [RACE, AMBIENT]) expect(rule(f, finding, passHead).status).toBe(0);
|
||||
const listed = run(f, 'gstack-approvals', ['rulings']);
|
||||
expect(listed.status).toBe(0);
|
||||
const lines = listed.stdout.trim().split('\n');
|
||||
expect(lines).toEqual([expect.stringMatching(new RegExp(`^Ruled note: ${RACE} at ${passHead.slice(0, 12)} ruled by `)), expect.stringMatching(new RegExp(`^Ruled note: ${AMBIENT} at ${passHead.slice(0, 12)} ruled by `))]);
|
||||
const body = path.join(f.root, 'body.md');
|
||||
const write = () => f.run(path.join(BIN, 'gstack-ship-receipt'), ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body], { GSTACK_APPROVALS_MIRROR: '0' });
|
||||
fs.writeFileSync(body, `## Pre-Landing Review\n\n${lines[0]}\n`);
|
||||
const short = write();
|
||||
expect([short.status, short.stderr]).toEqual([EXIT.fail, expect.stringContaining(`does not list ruled note ${JSON.stringify(lines[1])}`)]);
|
||||
fs.writeFileSync(body, `## Pre-Landing Review\n\n${lines.join('\n')}\n`);
|
||||
const ok = write();
|
||||
expect(ok.status, ok.stderr).toBe(EXIT.ok);
|
||||
const receipt = extractReceipt(ok.stdout);
|
||||
expect(receipt.errors).toEqual([]);
|
||||
expect(receipt.receipt?.ruled_notes?.map(n => n.finding)).toEqual([RACE, AMBIENT]);
|
||||
expect(receipt.receipt?.ruled_notes?.[0]).toMatchObject({ head: passHead, by: 'qm minder (CAPY-1)', reason: 'edit-share-race ships as a disclosed note', approval: expect.stringMatching(/^a-/), at: expect.stringMatching(/Z$/) });
|
||||
});
|
||||
});
|
||||
|
||||
describe('ruledNotes reads only approved owner rulings (GST-1537)', () => {
|
||||
const HEAD = '0123456789abcdef0123456789abcdef01234567';
|
||||
const row = (finding: string, o: Partial<{ ask: string; scope: string; by: string; outcome: string }> = {}) => {
|
||||
const r = buildRow({ ask: `ruled_note ${finding} at ${HEAD}: the lock already allows it`, words: 'a note', by: 'qm minder (CAPY-1)', scope: `ruled_note ${finding} ${HEAD}`, ...o }, 'feat', new Date('2026-11-05T01:02:03Z'));
|
||||
if ('code' in r) throw new Error(r.message);
|
||||
return r;
|
||||
};
|
||||
test('an approved ruling row counts; pending, rejected, mismatched, headless and gstack-written rows do not', () => {
|
||||
for (const excluded of [row(AMBIENT, { outcome: 'pending' }), row(AMBIENT, { outcome: 'rejected' }), row(AMBIENT, { ask: 'the lock already allows it' }),
|
||||
row(AMBIENT, { ask: `ruled_note ${RACE} at ${HEAD}: the lock already allows it` }), row(AMBIENT, { scope: `ruled_note ${AMBIENT}` }), row(AMBIENT, { by: 'gstack no-stop default (K1)' })]) {
|
||||
expect(ruledNotes([excluded])).toEqual([]);
|
||||
}
|
||||
const notes = ruledNotes([row(RACE)]);
|
||||
expect(notes).toEqual([{ finding: RACE, head: HEAD, by: 'qm minder (CAPY-1)', reason: 'the lock already allows it', at: '2026-11-05T01:02:03.000Z', approval: expect.stringMatching(/^a-[0-9a-f]{12}$/) }]);
|
||||
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})`);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,4 @@
|
||||
{
|
||||
"bytes": 471,
|
||||
"why": "GST-1537: the PR body's Pre-Landing Review lists each owner-ruled note and the receipt step names gstack-approvals rule as the only path"
|
||||
}
|
||||
Reference in new issue
Block a user