diff --git a/bin/gstack-approvals b/bin/gstack-approvals index 79bd5e21e..bd8c6b4b3 100755 --- a/bin/gstack-approvals +++ b/bin/gstack-approvals @@ -7,6 +7,8 @@ * [--date YYYY-MM-DD] [--source ] [--branch ] [--json] * gstack-approvals import --autoplan [--by ] [--branch | --stack ] [--json] * gstack-approvals render [--no-heading] [--full] [--branch ] + * gstack-approvals rule --finding --head --reason --words --by [--json] + * gstack-approvals rulings [--branch ] * gstack-approvals list [--json] [--branch ] * gstack-approvals sync [--branch ] * gstack-approvals path [--branch ] @@ -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 ] [--outcome approved|rejected|pending|standing] [--date YYYY-MM-DD] [--source ] import --autoplan [--by ] [--stack ] --stack writes the same rows to every branch of a stack; each branch is checked first, and one unknown branch writes none + rule --finding --head --reason + --words --by + 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 )", 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; bools: Set; error?: string } { const flags: Record = {}; @@ -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 and --reason '); + 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 '); + 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`); diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 4a4509f78..04369c030 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -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')), diff --git a/changelog.d/capy-gst-1537-ruled-notes.md b/changelog.d/capy-gst-1537-ruled-notes.md new file mode 100644 index 000000000..e4039d8c4 --- /dev/null +++ b/changelog.d/capy-gst-1537-ruled-notes.md @@ -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 --head --reason --words --by `: 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) diff --git a/docs/review-policy.md b/docs/review-policy.md index f4eac9b17..d8bf3e620 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -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 diff --git a/docs/ship-policy.md b/docs/ship-policy.md index e82521484..9e2d9b26f 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -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. + +**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 '' --head \ + --reason '' --words '' --by '' +``` + +The row is an approved `gstack-approvals` row with scope `ruled_note ` +and ask `ruled_note at : `, 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: at +ruled by at