diff --git a/bin/gstack-review-scope b/bin/gstack-review-scope index 1a8a7b53c..5eab9312c 100755 --- a/bin/gstack-review-scope +++ b/bin/gstack-review-scope @@ -3,17 +3,15 @@ * gstack-review-scope — the finding scope of /ship's review loop (docs/ship-policy.md#review-scope). * * gstack-review-scope classify --base --pass [--range-base ] [--policy frozen|diff-plus-blocking|all] [--findings |-] [--json] - * gstack-review-scope defer --findings [--pr ] [--repo ] [--branch ] [--out ] [--dry-run] [--json] + * gstack-review-scope defer --findings [--out ] [--json] * gstack-review-scope render --deferred [--findings ] * gstack-review-scope summary --since (--run | --ledger ... | --no-ledger) [--out ] [--json] * * `classify` reads a JSON array of findings (the Step 9.3 result) and stamps * each with `locus` (diff | fix-range | outside), `kind` (defect | test-gap | * advisory), `blocking` and `scope` (fix | defer | note); the loop fixes only - * `fix`, files `defer` as issues and writes `note` into the PR body. - * `defer` files one GitHub issue per deferred finding through gh, reusing an - * open issue that already carries the finding's marker; without gh the finding - * stays deferred as "unfiled" with the reason. `render` prints the PR-body + * `fix` and writes `defer` and `note` into the PR body. `defer` records each + * deferred finding for the PR and files no issue. `render` prints the PR-body * sections. `summary` reads this branch's review log for the invocation's * passes and prints the REVIEW_LOOP line the ship receipt carries. * Logic: lib/review-scope.ts. @@ -27,10 +25,9 @@ import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { resultLineWithDocs } from '../lib/result-codes'; import { runScriptSync } from '../lib/script-invocation'; import { isUnavailable, loadPolicyFromBase, policyUnavailableLine, resolveBaseBranch } from '../lib/ship-policy'; -import { EXIT as POST_EXIT, runPost } from '../lib/gstack-post'; import { - ledgerPass, parseFixTrailers, parseHunks, renderDeferredIssue, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, - ALREADY_FILED_REASON, DEFERRED_MARKER, findingIdentity, findingWhere, REVIEW_SCOPE_POLICIES, type DeferredRecord, type ReviewScopePolicy, type ScopeFinding, type ScopedFinding, + deferredRecord, ledgerPass, parseFixTrailers, parseHunks, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, + REVIEW_SCOPE_POLICIES, type DeferredRecord, type ReviewScopePolicy, type ScopeFinding, type ScopedFinding, } from '../lib/review-scope'; import { ledgerBlockers, type LedgerBlocker } from '../lib/review-ledger'; @@ -39,9 +36,9 @@ const HELP = `gstack-review-scope — finding scope for /ship's review loop (doc classify --base --pass [--range-base ] [--policy frozen|diff-plus-blocking|all] [--findings |-] stamp each finding with locus, kind, blocking and scope (fix | defer | note); prints REVIEW_SCOPE: … and the classified JSON array (--json: JSON only) - defer --findings [--pr ] [--repo ] [--branch ] [--out ] [--dry-run] - file one issue per scope=defer finding through gh (reuses an open issue carrying the - finding's marker); prints DEFERRED: lines and the deferred records + defer --findings [--out ] + record each scope=defer finding for the PR body (no issue is filed); + prints DEFERRED: lines and the deferred records render --deferred [--findings ] the PR-body sections: ## Deferred findings (references, never closing keywords) and ## Review notes (scope=note findings) @@ -55,7 +52,7 @@ const HELP = `gstack-review-scope — finding scope for /ship's review loop (doc prints REVIEW_SCOPE_UNBOUND: : (unbound=N). An unreadable ledger: REVIEW_SCOPE_LEDGER Defaults: --policy from the base branch's .gstack/ship-policy.json review_scope (frozen); an unresolvable base refuses - (POLICY_SOURCE_UNAVAILABLE) unless --policy is passed; --findings - reads stdin; --branch the current branch. + (POLICY_SOURCE_UNAVAILABLE) unless --policy is passed; --findings - reads stdin. ${exitTableHelp()}`; function flag(args: string[], name: string): string | undefined { @@ -98,57 +95,6 @@ function diffHunkLines(from: string, cwd: string): { ok: boolean; out: string; e return { ok: true, out: keep.join('\n'), err: '' }; } finally { fs.rmSync(dir, { recursive: true, force: true }); } } -function gh(args: string[], cwd: string): { ok: boolean; out: string; err: string } { - const r = spawnSync('gh', args, { cwd, encoding: 'utf8', timeout: 120_000, maxBuffer: 16 * 1024 * 1024 }); - if (r.error) return { ok: false, out: '', err: (r.error as NodeJS.ErrnoException).code === 'ENOENT' ? 'gh is not installed' : r.error.message }; - return { ok: r.status === 0, out: r.stdout ?? '', err: (r.stderr ?? '').trim() }; -} -/** - * File one deferred issue through the publication boundary the PR body uses - * (lib/gstack-post: the exact title and body bytes are scanned for secrets and - * private values before anything reaches gh). Reviewer text is untrusted here: a - * security finding may quote the value it found. A HIGH finding refuses, a MEDIUM - * one needs a human and is never confirmed unattended; both leave the finding - * deferred and unfiled with the reason in the record. - */ -function publishIssue(repoRoot: string, repo: string | undefined, title: string, body: string): { ok: boolean; out: string; err: string } { - const dir = fs.mkdtempSync(path.join(process.env.TMPDIR ?? '/tmp', 'gstack-deferred-')); - const titleFile = path.join(dir, 'title.txt'); - const bodyFile = path.join(dir, 'issue.md'); - fs.writeFileSync(titleFile, title, { mode: 0o600 }); - fs.writeFileSync(bodyFile, body, { mode: 0o600 }); - let out = ''; - let err = ''; - try { - const code = runPost(['issue-create', '--title-file', titleFile, '--body-file', bodyFile], { - run(cmd, args, input) { - // --repo names the destination: the remote is that GitHub repository and every gh call targets it. - // `gh repo view` takes the repository as its positional argument and rejects --repo, so the visibility read - // (public-strict scan or not) names it that way; every other gh command takes --repo. - if (repo && cmd === 'git' && args[0] === 'remote') return { status: 0, stdout: `https://github.com/${repo}\n`, stderr: '' }; - const argv = cmd === 'gh' && repo ? (args[0] === 'repo' && args[1] === 'view' ? [...args.slice(0, 2), repo, ...args.slice(2)] : [...args, '--repo', repo]) : args; - const r = spawnSync(cmd, argv, { cwd: repoRoot, input: input ?? '', encoding: 'utf8', timeout: 120_000, maxBuffer: 16 * 1024 * 1024 }); - if (r.error) return { status: 1, stdout: '', stderr: (r.error as NodeJS.ErrnoException).code === 'ENOENT' ? `${cmd} is not installed` : r.error.message }; - return { status: r.status ?? 1, stdout: r.stdout ?? '', stderr: r.stderr ?? '' }; - }, - readFile: p => fs.readFileSync(p, 'utf8'), - out: t => { out += t; }, - err: t => { err += t; }, - }, path.join(import.meta.dir, 'gstack-config')); - if (code === POST_EXIT.posted) return { ok: true, out, err }; - const rules = [...new Set([...out.matchAll(/RULE: (\S+)/g)].map(m => m[1]))].join(', '); - const reason = code === POST_EXIT.refused ? `publication scan refused: HIGH ${rules || 'finding'}; remove the value from the finding text` - : code === POST_EXIT.confirm ? `publication scan: MEDIUM ${rules || 'finding'} needs a human before this text is posted` - : `gh issue create failed: ${(err.trim().split('\n').find(l => l.trim()) ?? 'unknown').slice(0, 160)}`; - return { ok: false, out, err: reason }; - } finally { - fs.rmSync(dir, { recursive: true, force: true }); - } -} - -/** One `gh issue list` page; a page exactly this full may hide older markers, so the run says so and the records carry it. */ -const DEFERRED_SEARCH_LIMIT = 200; - function readFindings(spec: string | undefined, what: string): ScopeFinding[] { const text = spec === undefined || spec === '-' ? fs.readFileSync(0, 'utf8') : fs.readFileSync(spec, 'utf8'); const parsed = JSON.parse(text.trim() || '[]'); @@ -206,51 +152,10 @@ function cmdClassify(rest: string[], json: boolean): number { } function cmdDefer(rest: string[], json: boolean): number { - const repoRoot = git(['rev-parse', '--show-toplevel'], process.cwd()).out.trim() || process.cwd(); let findings: ScopedFinding[]; try { findings = readFindings(flag(rest, '--findings'), 'classified findings') as ScopedFinding[]; } catch (e: any) { return refuse('defer', 'REVIEW_SCOPE_INPUT', e.message); } - const toDefer = findings.filter(f => f.scope === 'defer'); - const prFlag = flag(rest, '--pr'); - if (prFlag !== undefined && !/^[1-9]\d*$/.test(prFlag)) return refuse('defer', 'REVIEW_SCOPE_INPUT', `--pr must be a PR number, got ${JSON.stringify(prFlag.slice(0, 40))}`); - const repo = flag(rest, '--repo'); - const repoArgs = repo ? ['--repo', repo] : []; - const ctx = { - repo, pr: prFlag ? Number(prFlag) : undefined, - branch: flag(rest, '--branch') ?? (git(['rev-parse', '--abbrev-ref', 'HEAD'], repoRoot).out.trim() || undefined), - head: git(['rev-parse', 'HEAD'], repoRoot).out.trim() || undefined, - }; - const dryRun = rest.includes('--dry-run'); - // One search for every marker, before any create: a failed search files nothing, so a rate-limited rerun cannot duplicate issues. - let filed: Array<{ number: number; url: string; body: string }> | null = null; - let searchError = ''; - let truncated = false; - if (!dryRun && toDefer.length) { - const existing = gh(['issue', 'list', ...repoArgs, '--state', 'open', '--search', `"${DEFERRED_MARKER}" in:body`, '--json', 'number,url,body', '--limit', String(DEFERRED_SEARCH_LIMIT)], repoRoot); - if (existing.ok) { try { filed = JSON.parse(existing.out); } catch { searchError = 'gh issue list returned no JSON'; } } - else searchError = `gh issue list failed: ${(existing.err || 'unknown').split('\n')[0]!.slice(0, 160)}`; - if (!Array.isArray(filed)) filed = null; - // An exactly-full page is read as truncated: an older issue with this finding's marker may sit past it, so a new issue may duplicate one. - truncated = (filed?.length ?? 0) >= DEFERRED_SEARCH_LIMIT; - if (truncated) process.stderr.write(`DEFERRED_SEARCH: truncated at ${DEFERRED_SEARCH_LIMIT} open issues; an existing issue for a finding may not have been found\n`); - } - const records: DeferredRecord[] = []; - for (const f of toDefer) { - const { title, body, marker } = renderDeferredIssue(f, ctx); - const record: DeferredRecord = { fingerprint: findingIdentity(f), path: findingWhere(f), severity: String(f.severity ?? 'unrated'), summary: String(f.summary ?? f.problem ?? f.category ?? ''), issue: null }; - records.push(record); - if (dryRun) { record.reason = 'dry run'; continue; } - if (!filed) { record.reason = searchError; continue; } - const hit = filed.find(i => typeof i.body === 'string' && i.body.includes(marker)); - if (hit) { record.issue = hit.number; record.url = hit.url; record.reason = ALREADY_FILED_REASON; continue; } - const created = publishIssue(repoRoot, repo, title, body); - if (created.ok) { - const url = created.out.split('\n').map(l => l.trim()).filter(l => /\/issues\/\d+$/.test(l)).pop() ?? ''; - const n = Number(/\/issues\/(\d+)$/.exec(url)?.[1]); - if (Number.isInteger(n)) { record.issue = n; record.url = url; if (truncated) record.reason = `filed; the marker search was truncated at ${DEFERRED_SEARCH_LIMIT}, so an older issue may duplicate it`; } else record.reason = `gh returned no issue url: ${url.slice(0, 120)}`; - } else record.reason = created.err; - } - // Unfiled is the documented degraded path, not a failure: the finding stays deferred and the PR body names the reason. - for (const r of records) process.stderr.write(`DEFERRED: ${r.issue ? `#${r.issue}` : `unfiled (${r.reason ?? 'no gh'})`} ${r.severity} ${r.path}\n`); + const records: DeferredRecord[] = findings.filter(f => f.scope === 'defer').map(deferredRecord); + for (const r of records) process.stderr.write(`DEFERRED: ${r.severity} ${r.path}\n`); if (!records.length) process.stderr.write('DEFERRED: none\n'); const out = flag(rest, '--out'); if (out) fs.writeFileSync(out, JSON.stringify(records, null, 2) + '\n'); diff --git a/captain/templates/READY.md b/captain/templates/READY.md index f07b082cb..ff4f5002f 100644 --- a/captain/templates/READY.md +++ b/captain/templates/READY.md @@ -39,7 +39,7 @@ The lane's own lines, after the block: pin: # the full head SHA, restated screen choice: <--no-screen: cases ran focused and passed | --screen: > flaky: > -review passes: (deferred: ) +review passes: (deferred: ) Unverified: Head frozen from now on; a push drops this PR from any train and I will READY again. ``` diff --git a/capy/skill/gstack/factory.md b/capy/skill/gstack/factory.md index 35b426a7f..8e30e1d9f 100644 --- a/capy/skill/gstack/factory.md +++ b/capy/skill/gstack/factory.md @@ -4,8 +4,8 @@ Your crew's CREW.md and recorded approvals decide who may merge, update a base, 1. **Plan, then a yes.** New functionality starts with /autoplan sized to the work; the build waits for "do it". [SKILL.md: Plan, approve, ship] 2. **Someone else merges.** Run /ship to a receipt on your final pushed head, send one READY quoting gstack's verify output verbatim, and stop. The merge owner merges on green and assigns versions; docs-only PRs run only docs checks. [crew/docs/captain.md: The READY contract] -3. **Block only on the floor.** A finding blocks if it would fail a test, break production, or lose data or secrets; the rest goes in the PR body or next wave. [docs/review-policy.md] -4. **Fix waves.** When main goes red, hold /ship while one owner finds the root cause; bring main in once, as your rules allow. Fold deferred findings and test follow-ups into one PR. [docs/triage.md] +3. **Block only on the floor.** A finding blocks if it would fail a test, break production, or lose data or secrets; deferred findings stay on the PR and never become issues. [docs/review-policy.md] +4. **Fix waves.** When main goes red, hold /ship while one owner finds the root cause; bring main in once, as your rules allow. Follow-ups get one issue with an owner and one PR. [docs/triage.md] 5. **Severe first, same gates.** A severe user-facing bug jumps the queue with a short plan; receipt, checks and merge owner stay. 6. **Trade findings, not small talk.** Another crew's finding goes to its Captain, through yours, with evidence. 7. **Measure before you act.** Run the real system on the merged tree you ship; a real run outranks a reviewed plan. Each number names its run, commit or uncapped query; say what you did not verify. diff --git a/changelog.d/keep-deferred-findings-on-the-pr.md b/changelog.d/keep-deferred-findings-on-the-pr.md new file mode 100644 index 000000000..db21f8574 --- /dev/null +++ b/changelog.d/keep-deferred-findings-on-the-pr.md @@ -0,0 +1 @@ +- **/ship no longer files deferred review findings as issues.** A blocking finding in code the PR did not change (`review_scope: frozen`) now stays on the PR: `gstack-review-scope defer` records it for the PR body's `## Deferred findings` section and never calls `gh`, so it opens no GitHub issue and needs no marker search. The `--pr`, `--repo`, `--branch` and `--dry-run` options are gone, and a deferred record no longer carries `issue`, `url` or `reason`. When the reviewer stated its own severity or Blocks field, the deferred line in the PR body shows it next to the label, as the issue body used to. /ship's Step 9.4, docs/review-policy.md, docs/ship-policy.md, the READY template, crew/docs/captain.md and the Capy factory page now say the same thing: deferred findings stay on the PR, and follow-up work someone will actually do gets one issue with a named owner. The pre-existing test failure option in /ship that assigns an issue to the person who broke the test is unchanged, since that issue has an owner. diff --git a/crew/docs/captain.md b/crew/docs/captain.md index 496f5b1aa..14c85070f 100644 --- a/crew/docs/captain.md +++ b/crew/docs/captain.md @@ -219,6 +219,9 @@ Issues show what the crew is really doing, so keep them true: - Before archiving a thread, close or move to Backlog every issue assigned to it. - On each wake, leave every issue you touched accurate: its status, assignee and links. - An unassigned Todo that nobody will start goes to Backlog. +- Deferred findings from review or /ship never become issues: they stay on the PR's + findings or its `## Deferred findings` section, and any still open close when the PR + merges. Follow-up work someone will actually do gets one issue with a named owner. - A daily sweep closes, re-assigns or backlogs any open issue outside Backlog that nobody has touched for 2 days and whose assignee is archived, or idle with no open PR. - The sweep skips an issue assigned to a person (an ask waiting on the owner, say) and a @@ -638,7 +641,7 @@ only the run the lane named. ## The lessons loop Every report ends with one Lesson. The controller reads lessons before chat, and each -one becomes a rule in CREW.md, a line in AGENTS.md, or a tool fix filed as an issue; +one becomes a rule in CREW.md, a line in AGENTS.md, or a tool fix as one issue with an owner; nothing stays a lesson for long. Rules carry the thread that produced them, so a new lane can read the incident behind the rule. diff --git a/docs/review-policy.md b/docs/review-policy.md index 17722401b..e4a7511e9 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -78,12 +78,9 @@ incomplete coverage, which blocks until the reviewer is rerun. a hunk the PR changed, with three lines of context; `outside`: anything else), `kind`, `blocking` and `scope`. Findings in the diff are `fix`. A blocking finding in code the PR did not change is `defer`: `gstack-review-scope defer` - files it as a GitHub issue (one per finding, reused across reruns through an - `` marker; the issue text is - scanned like the PR body before anything is posted, so a finding that quotes a - secret is `unfiled (publication scan refused: …)`; `unfiled ()` when - `gh` is unavailable or its search fails; every unfiled case still exits 0) - and the PR body lists it under `## Deferred findings`. A finding's `cause` + records it for the PR and files no issue, and the PR body lists it under + `## Deferred findings`. Deferred findings never become issues; work someone + will actually do gets one issue with a named owner. A finding's `cause` (the changed line behind a defect reported elsewhere, a deleted file included) places it in the diff. Below-floor findings outside the diff are `note`, listed under `## Review notes`. @@ -688,7 +685,7 @@ puts in the receipt: | `REVIEW_SCOPE_VOIDED:` (stderr, not a receipt field) | an abandoned pass (GBEV-3): a `pN` ledger never judged that no row names, replaced by a later `pM` ledger (M > N) that was judged or is named. Its findings are left out of every count above instead of counting as unbound and unrecorded. A fix-check ledger and the last pass are never voided | Findings bind by id first (GST-1168). A row finding that came from a dispatched reader carries `ledger_id` (`/`, such as `p2/adversarial-native#3`) and pairs with that ledger entry whatever its path:line says, so a native finding with no path:line, or a row logged after its fix moved the line, still pairs. Each fix commit names what it fixed in a `Review-Finding: reader=` trailer, and a trailer counts the finding as fixed. A late defer re-logs its finding under the same id, so it counts once. Rows that record their head dedupe by kind, pass, head and finding ids, so a logger command run twice counts once. Rows and findings without an id keep the path:line pairing above. The native reader's own verdict line ("all FIXABLE, and none blocks", with no file:line, list mark, lead label or Severity/Blocks field) collects at FIXABLE, not CRITICAL; the findings it summarizes collect on their own lines. -| `review_loop.deferred` | blocking findings in unchanged code filed as issues (a Step 11 late defer, logged as a `review-late-defer` row so it never displaces the newest `review` pass, counts here and is not a pass) | +| `review_loop.deferred` | blocking findings in unchanged code, kept on the PR under `## Deferred findings` (a Step 11 late defer, logged as a `review-late-defer` row so it never displaces the newest `review` pass, counts here and is not a pass) | | `review_loop.noted` | below-floor findings listed in the PR body | | `review_loop.fixed` | findings fixed in this PR | | `review_loop.scope` | the `review_scope` the loop ran under | @@ -803,11 +800,11 @@ difference passed a scoped review. | Value | Pass 1 | Blocking finding in unchanged code | Pass 2+ | Cap | |---|---|---|---|---| -| `frozen` (default) | the diff | filed as an issue, listed as deferred | the range since the previous pass, blocking only | 2 rounds (a fix-check is one) | +| `frozen` (default) | the diff | listed as deferred in the PR body | the range since the previous pass, blocking only | 2 rounds (a fix-check is one) | | `diff-plus-blocking` | the diff | fixed in this PR | the range since the previous pass, blocking only | 3 fix cycles (Step 9.4 runs the review plan's two-round cap only under `frozen`) | | `all` | everything | fixed in this PR | the whole diff, every finding | 3 fix cycles | -`diff-plus-blocking` is the fallback for a repository where deferred issues pile +`diff-plus-blocking` is the fallback for a repository where deferred findings pile up; `all` is the loop as it ran before the key existed. A value other than `frozen` needs `min_gstack_version` at or past the release that added the key (`REVIEW_SCOPE_KEY_VERSION` in `lib/ship-policy-keys.ts`) so an older gstack refuses @@ -935,7 +932,7 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: real blockers named in an exception clause ("No CRITICAL issues except the token leak in log.ts:3"); GSTA-167 reverted that. Each prose finding's line also records the Severity and Blocks its own text states as `stated`, beside the collected label and never in its place; - a classified finding that carries `stated` shows it in its deferred-review issue next to the label ("CRITICAL (the + a classified finding that carries `stated` shows it in its `## Deferred findings` line next to the label ("CRITICAL (the reviewer wrote Severity: Low, Blocks: no)"). /ship's Step 9.4 does not copy `stated` into its findings file yet: that instruction lives in a reviewer brief that changes only after a no-loss replay. A CRITICAL whose own text writes the field `Blocks: no` (colon required, and no `Blocks: yes`) is not counted as a blocker by the ledger count, nor by the diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 4dde8af03..9248b3c1c 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -72,7 +72,7 @@ branch that changes the policy. | `migrations` | `{ dir, id }` or `null` | `null` | The migration directory and a regex with exactly one capture group for the number in a file name (`"^v(\\d+)-"`); `gstack-pregate regate` reports `MIGRATION_COLLISION`. | | `gate_command` | command or `null` | `null` | The repo's full pre-merge gate. `gstack-ship-queue gate-ahead` and `ready` print it for the caller to run (in the background or in CI); gstack never executes it. Needs `min_gstack_version`. | | `full_e2e` | `true` \| `false` | `false` | `true`: every merge needs a passing full-gate result bound to a tree equal to the merging one modulo stamps; `gstack-ship-queue ready`, `stamp` and `verify` refuse without one (`QUEUE_GATE_MISSING`): a gate-ahead record, or the lane's `--gate-exit 0 --gate-log ` from its run on the gated head. Needs `min_gstack_version`. | -| `review_scope` | `frozen` \| `diff-plus-blocking` \| `all` | `frozen` | The finding scope of `/ship`'s review loop; see [review_scope](#review-scope). `frozen` fixes the diff on pass 1, files blocking findings in unchanged code as issues, and later passes read only the range since the previous pass. `diff-plus-blocking` is the same but fixes a blocking finding in unchanged code in this PR instead of filing it. `all` is the earlier loop: every finding is fixed on every pass. Needs `min_gstack_version` when not `frozen`. | +| `review_scope` | `frozen` \| `diff-plus-blocking` \| `all` | `frozen` | The finding scope of `/ship`'s review loop; see [review_scope](#review-scope). `frozen` fixes the diff on pass 1, lists blocking findings in unchanged code as deferred in the PR body (never as issues), and later passes read only the range since the previous pass. `diff-plus-blocking` is the same but fixes a blocking finding in unchanged code in this PR instead of deferring it. `all` is the earlier loop: every finding is fixed on every pass. Needs `min_gstack_version` when not `frozen`. | | `review_exclude_paths` | git glob pathspecs | `[]` | Content files `/ship`'s reviewers leave out: data, docs, `.log`, and binaries under a content extension (images, fonts, archives and the like) that do not start with executable magic. Read from the base only. Code (native libraries and every other binary outside the content extensions included), gate paths, agent prose, config, executables, links, `.gstack/` and `.github/` files, gate-tagged, secret-line and named-by-code files stay in review. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | | `review` | `{ panel?, reader_model?, platform?, risk_paths? }` | `{ all, lane, off, [] }` | Who does `/ship`'s review, read from the base only; see [Who reads](review-policy.md#who-reads-r65-the-base-policys-review-key). `panel`: `all` (today's panel on every class) \| `risk` \| `off` (the opt-in one-reader shapes); `reader_model`: `lane`, `cross-family` or a model id; `platform`: `off` \| `on` (a docs or tests diff with no numeric, results or prereg claims whose PR carries capy-ai[bot]'s review at the exact head skips the readers); `risk_paths`: globs of code paths that count as risk class `policy`. Introduced at 1.94.30.0; needs `min_gstack_version` when set to a non-default value. | | `readers` | `{ forbid_fetch?, forbid_paths?, brief_note? }` | `{ [], [], null }` | What `/ship`'s readers may not reach beyond the fetch rule every brief carries; see [readers](#readers). Read from the base; the head may add `forbid_fetch` entries, never remove one. Needs `min_gstack_version`. | @@ -1621,12 +1621,9 @@ construction: security or data-loss defect. Everything else (an advisory, an informational item, a test gap, a wording note) is below the floor. Pass 1 fixes findings in the diff (`scope: fix`). A blocking finding in code the PR did not change is - `scope: defer`: `gstack-review-scope defer` files it as a GitHub issue (reusing - an open issue that carries the same `` - marker; the title and body go through the same publication scan as the PR - body, so a finding that quotes a credential stays `unfiled (publication scan - refused: …)`; `unfiled ()` without `gh`) and `render` writes it under - `## Deferred findings` in the PR body. Below-floor findings outside the diff, + `scope: defer`: `gstack-review-scope defer` records it for the PR, files no + issue, and `render` writes it under `## Deferred findings` in the PR body, which + goes through the PR body's publication scan. Below-floor findings outside the diff, every below-floor finding from pass 2 on, and every below-floor finding from Step 11's voices or a fix-check (`raised_by` `adversarial-native`, `outside-voice` or `fix-check`, even classified at `--pass 1`) are @@ -1727,7 +1724,7 @@ The floor and the pass structure are stated once, in [review-policy.md](review-policy.md); this section documents the knob. `review_scope: diff-plus-blocking` is the documented fallback for a repository -where deferred issues pile up: the same passes and range as `frozen`, but a +where deferred findings pile up: the same passes and range as `frozen`, but a blocking finding in code the PR did not change is `scope: fix` in this PR instead of `defer`, and the loop stops at Step 9.4's third fixing cycle (`CYCLES >= 3`): only `frozen` runs the review plan's two-round cap. `review_scope: all` is the opt-back to the earlier loop: diff --git a/docs/unattended.md b/docs/unattended.md index 1246cf76d..c39f807c2 100644 --- a/docs/unattended.md +++ b/docs/unattended.md @@ -174,10 +174,10 @@ The receipt's `review_loop` object, written from `gstack-review-scope summary (Step 9 full passes this ship ran, 1 or 2; up to 3 under `review_scope: diff-plus-blocking` or `all`), `blocking_found` (every blocker the readers found, rows and dispatch ledgers both, [review-policy.md](review-policy.md#what-the-receipt-records)), `deferred` -(blocking findings in unchanged code filed as issues, Step 11's late defers -included), `noted`, `fixed` and `scope`. The +(blocking findings in unchanged code, kept on the PR and never filed as +issues, Step 11's late defers included), `noted`, `fixed` and `scope`. The summary line a parent reads prints it as `passes= blocking_found= -deferred=`; the PR body lists the deferred issues under `## Deferred +deferred=`; the PR body lists the deferred findings under `## Deferred findings` and the below-floor items under `## Review notes`. A pass that finds nothing is `NO FINDINGS` from every reviewer and a pass with no edits, which is the loop's exit, never a failed review ([review-policy.md](review-policy.md)). diff --git a/lib/review-scope.ts b/lib/review-scope.ts index 2243978c9..45182e86e 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -6,12 +6,13 @@ * new neighbours in pre-existing code, fixed them, and the fixes drew another * pass until the cycle cap. Under `review_scope: frozen` (the default) the * scope is fixed after pass 1: findings in the diff are fixed, findings in - * code the PR did not change are filed as issues (deferred), and later passes - * read only the range since the previous pass and fix only blocking findings. + * code the PR did not change are deferred and stay on the PR, never filed as + * issues, and later passes read only the range since the previous pass and fix + * only blocking findings. * Everything below the blocking floor is a named note in the PR body and never * triggers a pass. `review_scope: all` keeps the old behaviour. * - * Pure functions here; bin/gstack-review-scope runs git and gh. + * Pure functions here; bin/gstack-review-scope runs git. */ import * as path from 'node:path'; import { SPECIALISTS } from './review-depth'; @@ -234,9 +235,8 @@ export function scopeLine(policy: ReviewScopePolicy, pass: number, scoped: reado } // --------------------------------------------------------------------------- -// deferral: one issue per blocking finding in code the PR did not change +// deferral: a blocking finding in code the PR did not change stays on the PR // --------------------------------------------------------------------------- -export const DEFERRED_MARKER = 'gstack-deferred-finding'; const oneLine = (s: unknown) => String(s ?? '').replace(/[\x00-\x1f\x7f\u2028\u2029]+/g, ' ').trim(); export function findingIdentity(f: ScopeFinding): string { @@ -250,43 +250,20 @@ function findingText(f: ScopeFinding): string { return oneLine(f.summary ?? f.problem ?? f.category ?? 'review finding'); } -export interface DeferContext { repo?: string; pr?: number; branch?: string; head?: string } +export interface DeferredRecord { fingerprint: string; path: string; severity: string; stated?: string; summary: string } -/** Issue title and body for a deferred finding; the marker keeps a rerun from filing it twice. */ -export function renderDeferredIssue(f: ScopedFinding, ctx: DeferContext): { title: string; body: string; marker: string } { - const text = findingText(f); - const title = `Deferred review finding: ${findingWhere(f)} — ${text}`.slice(0, 120); - const origin = ctx.pr ? `${ctx.repo ? `${ctx.repo}#${ctx.pr}` : `PR #${ctx.pr}`}` : ctx.branch ? `branch \`${ctx.branch}\`` : 'a /ship run'; - const marker = ``; +/** The PR's record of a deferred finding: `severity` stays the collected label, `stated` carries what the reviewer wrote beside it. */ +export function deferredRecord(f: ScopedFinding): DeferredRecord { const said = [typeof f.stated?.severity === 'string' ? `Severity: ${oneLine(f.stated.severity)}` : '', typeof f.stated?.blocks === 'string' ? `Blocks: ${oneLine(f.stated.blocks)}` : ''].filter(Boolean); - const body = [ - `Filed by /ship's pre-landing review of ${origin}${ctx.head ? ` (head ${ctx.head.slice(0, 12)})` : ''}. The finding is in code that PR did not change, so it was deferred here instead of being fixed there (review scope frozen after pass 1).`, - '', - `- **Severity:** ${oneLine(f.severity ?? 'unrated')}${said.length ? ` (the reviewer wrote ${said.join(', ')})` : ''}`, - `- **Where:** \`${findingWhere(f)}\``, - `- **Finding:** ${text}`, - ...(f.fix ? [`- **Suggested fix:** ${oneLine(f.fix)}`] : []), - ...(f.specialist ? [`- **Found by:** ${oneLine(f.specialist)}`] : []), - '', - marker, - '', - ].join('\n'); - return { title, body, marker }; + return { fingerprint: findingIdentity(f), path: findingWhere(f), severity: String(f.severity ?? 'unrated'), ...(said.length ? { stated: said.join(', ') } : {}), summary: String(f.summary ?? f.problem ?? f.category ?? '') }; } -export interface DeferredRecord { fingerprint: string; path: string; severity: string; summary: string; issue: number | null; url?: string; reason?: string } -/** `defer`'s reason when the marker search found the finding's issue already open; the section renders such a record as a plain `#N`. */ -export const ALREADY_FILED_REASON = 'already filed'; - -/** The PR-body section for deferred findings: references, never closing keywords, so a merge closes nothing. Only `frozen` defers. */ +/** The PR-body section for deferred findings, which stay on the PR: no issue is filed and no closing keyword is written. Only `frozen` defers. */ export function renderDeferredSection(deferred: readonly DeferredRecord[], noted: readonly ScopeFinding[] = []): string { const out: string[] = []; if (deferred.length) { - out.push('## Deferred findings', '', 'Blocking findings in code this PR did not change (review scope: frozen); filed as issues and not closed by this PR.', ''); - for (const d of deferred) { - const ref = d.issue ? `#${d.issue}${d.reason && d.reason !== ALREADY_FILED_REASON ? ` (${oneLine(d.reason)})` : ''}` : `unfiled${d.reason ? ` (${oneLine(d.reason)})` : ''}`; - out.push(`- Deferred: ${ref} — ${oneLine(d.severity)} \`${oneLine(d.path)}\` ${oneLine(d.summary)}`); - } + out.push('## Deferred findings', '', 'Blocking findings in code this PR did not change (review scope: frozen). They stay on this PR and are not filed as issues.', ''); + for (const d of deferred) out.push(`- Deferred: ${oneLine(d.severity)}${d.stated ? ` (the reviewer wrote ${oneLine(d.stated)})` : ''} \`${oneLine(d.path)}\` ${oneLine(d.summary)}`); out.push(''); } if (noted.length) { diff --git a/lib/ship-policy.ts b/lib/ship-policy.ts index aa9eec53d..c2e6d98fa 100644 --- a/lib/ship-policy.ts +++ b/lib/ship-policy.ts @@ -35,7 +35,7 @@ export const SHIP_POLICY_SCHEMA = 'https://github.com/garrytan/gstack/docs/ship- export type BumpPolicy = 'patch' | 'ask' | 'never'; export type QueueMode = 'claim-at-open' | 'stamp-at-merge'; export type HistoryPolicy = 'merge-only' | 'squash-ok'; -/** `frozen`: /ship fixes the diff in pass 1, defers unchanged code to issues and reviews only the range after; `all`: every finding is fixed now. */ +/** `frozen`: /ship fixes the diff in pass 1, defers unchanged code to the PR body and reviews only the range after; `all`: every finding is fixed now. */ export type ReviewScope = 'frozen' | 'diff-plus-blocking' | 'all'; export interface ShipPolicy { @@ -755,7 +755,7 @@ export function shipConsequences(policy: ShipPolicy, base: string): string[] { ? 'review_scope all: Step 9.4 fixes every finding on every pass (gstack-review-scope classify --policy all)' : policy.review_scope === 'diff-plus-blocking' ? 'review_scope diff-plus-blocking: Step 9.4 runs `gstack-review-scope classify --policy diff-plus-blocking` each pass; pass 1 fixes the diff and every blocking finding, in unchanged code too, later passes read only the range since the previous pass and fix only blocking findings; notes go to the PR body' - : 'review_scope frozen: Step 9.4 runs `gstack-review-scope classify` each pass; pass 1 fixes the diff and defers blocking findings in unchanged code to issues, later passes read only the range since the previous pass and fix only blocking findings; notes go to the PR body'); + : 'review_scope frozen: Step 9.4 runs `gstack-review-scope classify` each pass; pass 1 fixes the diff and defers blocking findings in unchanged code to the PR body, later passes read only the range since the previous pass and fix only blocking findings; notes go to the PR body'); if (policy.review_exclude_paths.length) out.push(`review_exclude_paths ${policy.review_exclude_paths.join(' ')}: Step 9's reviewers leave out the data, docs, binary and .log files these git globs match on this branch; gstack-review-plan prints REVIEW_EXCLUDE: with the counts and the kept files, and every brief names what was left out (docs/ship-policy.md#review-exclude-paths)`); if (policy.preregistration_shas.length) out.push(`preregistration: \`gstack-evidence ancestor --from-policy --base ${base}\` must print only ANCESTOR: ok before push; the ship receipt records the result`); if (policy.release_tool) out.push(`release_tool: \`${policy.release_tool}\` runs only inside gstack-restamp (staging worktree, outputs confined to release_outputs)`); diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index 52688b4f7..5c06bf264 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -745,12 +745,9 @@ but missing dispatched output still blocks continuation, even with a QA exceptio copied from the hunk, a test name or a commit message. 3.5. **Defer and note the rest.** Run - `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --pr --out ` - (omit `--pr` before a PR exists). It files one issue per `scope: defer` finding, - reusing an open issue that carries the finding's marker, scanning the issue text - as the PR body is scanned, and prints `DEFERRED:` lines; `unfiled ()` - keeps the finding deferred when gh cannot file it or the scan refuses the text, - and the command still exits 0: the PR body carries the reason. + `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --out `. + It prints a `DEFERRED:` line per `scope: defer` finding and files no issue: a + deferred finding stays on this PR, and you never file one as an issue either. Then `gstack-review-scope render --deferred --findings ` prints the `## Deferred findings` and `## Review notes` sections: save them for Step 19's PR body. Deferred and noted findings are never edited. @@ -758,7 +755,7 @@ but missing dispatched output still blocks continuation, even with a QA exceptio queued adversarial finding after this pass's items 5–6 were logged, append one row under its own skill (a tokenless `review` row would read UNVERIFIED): `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review-late-defer","via":"ship","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` - with `findings` holding only the deferred records (`action: "deferred"`, `issue`, + with `findings` holding only the deferred records (`action: "deferred"`, `locus`, `kind`, `blocking`, `ledger_id` as in item 6) and no `--finish` token. Log it in the program that decides the defer, with that moment's full ISO 8601 TIMESTAMP, never at Step 19. @@ -810,7 +807,7 @@ but missing dispatched output still blocks continuation, even with a QA exceptio - `findings`: checklist, specialist, exploratory QA and queued Steps 10–11 records with `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION: `"auto-fixed"`, `"fixed"` (approved), `"skipped"` (explicit Skip), - `"deferred"` (with `issue`, the filed number or `null`) or `"noted"`. Copy + `"deferred"` or `"noted"`. Copy `locus`, `kind`, `blocking`, `raised_by` and `ledger_id` from the classified output onto every finding. Merge revalidated invocation decisions by identity and advisory/defect kind; preserve `advisory`, `evidence_paths` and `helper_target`. diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 22a09b94b..d7e7f6ec6 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -155,12 +155,9 @@ but missing dispatched output still blocks continuation, even with a QA exceptio copied from the hunk, a test name or a commit message. 3.5. **Defer and note the rest.** Run - `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --pr --out ` - (omit `--pr` before a PR exists). It files one issue per `scope: defer` finding, - reusing an open issue that carries the finding's marker, scanning the issue text - as the PR body is scanned, and prints `DEFERRED:` lines; `unfiled ()` - keeps the finding deferred when gh cannot file it or the scan refuses the text, - and the command still exits 0: the PR body carries the reason. + `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --out `. + It prints a `DEFERRED:` line per `scope: defer` finding and files no issue: a + deferred finding stays on this PR, and you never file one as an issue either. Then `gstack-review-scope render --deferred --findings ` prints the `## Deferred findings` and `## Review notes` sections: save them for Step 19's PR body. Deferred and noted findings are never edited. @@ -168,7 +165,7 @@ but missing dispatched output still blocks continuation, even with a QA exceptio queued adversarial finding after this pass's items 5–6 were logged, append one row under its own skill (a tokenless `review` row would read UNVERIFIED): `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review-late-defer","via":"ship","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` - with `findings` holding only the deferred records (`action: "deferred"`, `issue`, + with `findings` holding only the deferred records (`action: "deferred"`, `locus`, `kind`, `blocking`, `ledger_id` as in item 6) and no `--finish` token. Log it in the program that decides the defer, with that moment's full ISO 8601 TIMESTAMP, never at Step 19. @@ -220,7 +217,7 @@ but missing dispatched output still blocks continuation, even with a QA exceptio - `findings`: checklist, specialist, exploratory QA and queued Steps 10–11 records with `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION: `"auto-fixed"`, `"fixed"` (approved), `"skipped"` (explicit Skip), - `"deferred"` (with `issue`, the filed number or `null`) or `"noted"`. Copy + `"deferred"` or `"noted"`. Copy `locus`, `kind`, `blocking`, `raised_by` and `ledger_id` from the classified output onto every finding. Merge revalidated invocation decisions by identity and advisory/defect kind; preserve `advisory`, `evidence_paths` and `helper_target`. diff --git a/test/fixtures/review-pins/files/ship/sections/review-army.md.json b/test/fixtures/review-pins/files/ship/sections/review-army.md.json index 3e599194d..99d8f1ea2 100644 --- a/test/fixtures/review-pins/files/ship/sections/review-army.md.json +++ b/test/fixtures/review-pins/files/ship/sections/review-army.md.json @@ -1 +1 @@ -{"sha":"49dfe3133576b05ee2c0c071efa1e24e04c2c57deed5d7bda11f54a431114af2","replay":"GST-1821"} +{"sha":"346dfea9dcc2e28cd0c9dbfb9f1207798a93a3cecbd913d12e16557a32b0f3fc","replay":"GST-2073"} diff --git a/test/fixtures/review-pins/replays/GST-2073.json b/test/fixtures/review-pins/replays/GST-2073.json new file mode 100644 index 000000000..a3fd64abf --- /dev/null +++ b/test/fixtures/review-pins/replays/GST-2073.json @@ -0,0 +1 @@ +{"replay":"GST-2073","justification":"GST-2073 brief-identity no-loss check (the brief-body test below): every reviewer brief lib/review-brief.ts cuts from ship/sections/review-army.md is byte-identical to its pinned body, so no reviewer sees a changed byte; only the lead's Step 9.4 instructions outside the cut spans changed: item 3.5 now runs `gstack-review-scope defer` without --pr and says a deferred finding stays on the PR and is never filed as an issue, and the review-log records for deferred findings no longer carry an `issue` field. ship/sections/adversarial.md and every review/ file are unchanged"} diff --git a/test/helpers/ship-frozen-scope-actor.ts b/test/helpers/ship-frozen-scope-actor.ts index 19a2a8d28..2ae0b58e7 100644 --- a/test/helpers/ship-frozen-scope-actor.ts +++ b/test/helpers/ship-frozen-scope-actor.ts @@ -168,7 +168,7 @@ Supported interactions, declared before launch: - Edit or Write: ${invoice} only, and only to apply a fix for a finding the classified output scopes \`fix\`. ${payments} is never written in this fixture. Write also accepts ${draft}, the item-6 review record of the current pass: the JSON object the gstack-review-log example takes (skill, timestamp, status, issues_found, critical, informational, quality_score 10.0, specialists {}, findings with fingerprint, severity, action and the copied locus, kind and blocking, commit, via "ship", completed, converged, cycles, pass, review_scope, range_base). The record's commit is the short HEAD: ${head.slice(0, 7)} now, and the sha the commit adapter prints after it commits a fix; no git command is needed for it. - Bash accepts exactly these single commands, without chaining, redirection or backgrounding. Each is the bound adapter for the generated helper command it names and does not decide which item or decision applies: ${commands.classify} runs gstack-review-scope classify for the current pass against this pass's .gstack/tmp/review-findings-p$PASS.json (--range-base is bound from pass 2 on); it writes .gstack/tmp/review-classified-p$PASS.json and prints the REVIEW_SCOPE line and the classified JSON. - ${commands.defer} runs gstack-review-scope defer on the current pass's classified file; it writes .gstack/tmp/review-deferred.json and prints the DEFERRED lines and the rendered PR-body sections (gh is absent here, so a deferred finding stays unfiled with its reason). + ${commands.defer} runs gstack-review-scope defer on the current pass's classified file; it writes .gstack/tmp/review-deferred.json and prints the DEFERRED lines and the rendered PR-body sections (a deferred finding stays on the PR; no issue is filed). ${commands.persist} passes ${draft} to the real gstack-review-log with this pass's REVIEW_START token (--finish); the pass-1 token was captured before this prompt, and the repeat adapter captures the next one. ${commands.commit} runs item 4's commit of the fixed file (git add invoice.ts && git commit -m "fix: pre-landing review fixes"). ${commands.repeat} takes decision 3: it runs the next pass's Steps 5 through 9.3 on the range since the previous pass, writes that pass's .gstack/tmp/review-findings-p$PASS.json and prints the result with the new REVIEW_START captured. @@ -221,7 +221,7 @@ export async function shipFrozenScopeFailures(fixture: ReturnType) => identity(f) === FROZEN_SCOPE_FIX_IDENTITY); const deferred = (first?.findings ?? []).find((f: Record) => identity(f) === FROZEN_SCOPE_DEFER_IDENTITY); check(['fixed', 'auto-fixed'].includes(fixed?.action) && fixed?.blocking === true && fixed?.locus === 'diff', 'pass 1 did not record the in-diff defect as fixed'); - check(deferred?.action === 'deferred' && deferred?.issue === null && deferred?.blocking === true && deferred?.locus === 'outside', 'pass 1 did not record the pre-existing defect as deferred'); + check(deferred?.action === 'deferred' && deferred?.blocking === true && deferred?.locus === 'outside', 'pass 1 did not record the pre-existing defect as deferred'); check(first?.review_scope === 'frozen' && second?.review_scope === 'frozen', 'rows do not carry the frozen policy'); check(typeof second?.range_base === 'string' && second.range_base.length >= 7 && evidence.head.startsWith(second.range_base), 'pass 2 did not record the pass-1 head as its range base'); check(!(second?.findings ?? []).some((f: Record) => ['fixed', 'auto-fixed'].includes(f?.action)) && Number(second?.issues_found) === 0, 'pass 2 recorded fixes or open defects on an empty range review'); @@ -327,7 +327,7 @@ if (import.meta.main && process.argv[2] === '--fixture') { } else if (action === 'defer') { if (!fs.existsSync(classified)) throw new Error(`Classify pass ${pass} first`); const deferred = file('review-deferred.json'); - const run = bin('gstack-review-scope', ['defer', '--findings', classified, '--out', deferred, '--dry-run']); + const run = bin('gstack-review-scope', ['defer', '--findings', classified, '--out', deferred]); output = [run.stderr.trim(), run.stdout.trim()].filter(Boolean).join('\n'); } else if (action === 'persist') { if (!fs.existsSync(config.draft)) throw new Error('Write the review record first'); diff --git a/test/review-loop-budget-wording.test.ts b/test/review-loop-budget-wording.test.ts index 0f1682630..aef3bed3e 100644 --- a/test/review-loop-budget-wording.test.ts +++ b/test/review-loop-budget-wording.test.ts @@ -26,7 +26,7 @@ describe('review-loop budget wording', () => { expect(army).toContain('cut that growth before anything else'); expect(army).toContain('(`git diff $RANGE_BASE` from pass 2 on)'); expect(army).toContain('The gate scan (2.5) and the QA probes keep the whole candidate'); - expect(army).toContain('and the command still exits 0: the PR body carries the reason'); + expect(army).toContain('files no issue: a deferred finding stays on this PR'); }); test('the checklist glossary maps the three words onto the blocking floor and repeats the stop rules', () => { diff --git a/test/review-scope-followups.test.ts b/test/review-scope-followups.test.ts index d2ff1aed5..9ff75400b 100644 --- a/test/review-scope-followups.test.ts +++ b/test/review-scope-followups.test.ts @@ -1,22 +1,13 @@ /** * PR 2 of the /ship + /review convergence lane: the review-scope follow-ups * from PR 1's review notes. A Step 11 late defer is a review-log row that - * counts under deferred and is not a pass; the deferred-issue search names its - * page size and reads an exactly-full page as truncated; `gh repo view` takes - * the repository positionally, so the visibility read works under --repo and a - * MEDIUM finding on a public repository stays unfiled until a human confirms. + * counts under deferred and is not a pass. */ -import { afterEach, describe, expect, test } from 'bun:test'; +import { describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { EXIT } from '../lib/headless-artifacts'; -import { ALREADY_FILED_REASON, LATE_DEFER_SKILL, renderDeferredSection, reviewLoopLine, summarizeReviewLoop } from '../lib/review-scope'; +import { LATE_DEFER_SKILL, reviewLoopLine, summarizeReviewLoop } from '../lib/review-scope'; import { reviewFreshness } from '../lib/review-evidence'; -import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; - -const repos: FixtureRepo[] = []; -const fixture = () => { const r = makeFixtureRepo({ prefix: 'review-scope-2-' }); repos.push(r); return r; }; -afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); describe('late defers from Step 11', () => { test('a review-late-defer row adds its deferred findings, is not counted as a pass, and is invisible to review freshness', () => { @@ -39,72 +30,6 @@ describe('late defers from Step 11', () => { }); }); -describe('deferred-issue search page and the repository argument gh accepts', () => { - const finding = (fingerprint: string, summary: string) => { - const [p, line, category] = fingerprint.split(':'); - return { fingerprint, path: p, line: Number(line), severity: 'CRITICAL', category, summary, locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }; - }; - const shim = (repo: FixtureRepo, visibility: string, issues: number) => { - const dir = path.join(repo.root, 'gh-shim'); - fs.mkdirSync(dir); - const log = path.join(dir, 'calls.log'); - const page = JSON.stringify(Array.from({ length: issues }, (_, i) => ({ number: 100 + i, url: `https://example.test/acme/app/issues/${100 + i}`, body: `` }))); - fs.writeFileSync(path.join(dir, 'gh'), [ - '#!/bin/sh', `echo "$*" >> "${log}"`, - `if [ "$1" = repo ] && [ "$2" = view ]; then [ "$3" = acme/app ] || { echo "gh: --repo not accepted by repo view: $*" >&2; exit 1; }; echo "${visibility}"; exit 0; fi`, - `if [ "$1" = issue ] && [ "$2" = list ]; then cat <<'EOF'\n${page}\nEOF\nexit 0; fi`, - 'if [ "$1" = issue ] && [ "$2" = create ]; then echo "https://example.test/acme/app/issues/900"; exit 0; fi', - 'echo "unsupported $*" >&2; exit 1', '', - ].join('\n'), { mode: 0o755 }); - return { dir, log }; - }; - - // The gh stand-in is a sh script on a colon-joined PATH, which CreateProcess cannot run on Windows. - test.skipIf(process.platform === 'win32')('an exactly-full search page is reported truncated and recorded on the issue it files; a short page is not', () => { - const repo = fixture(); - const classified = path.join(repo.root, 'classified.json'); - fs.writeFileSync(classified, JSON.stringify([finding('lib/legacy.ts:7:data-loss', 'Unbounded delete')])); - const full = shim(repo, 'PRIVATE', 200); - const out = path.join(repo.root, 'deferred.json'); - const r = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--repo', 'acme/app', '--out', out, '--json'], { PATH: `${full.dir}:${process.env.PATH}` }); - expect(r.status).toBe(EXIT.ok); - expect(r.err).toContain('DEFERRED_SEARCH: truncated at 200 open issues; an existing issue for a finding may not have been found'); - const records = JSON.parse(r.out); - expect(records[0]).toMatchObject({ issue: 900, reason: 'filed; the marker search was truncated at 200, so an older issue may duplicate it' }); - expect(fs.readFileSync(full.log, 'utf8')).toContain('--limit 200'); - expect(renderDeferredSection(records)).toContain('- Deferred: #900 (filed; the marker search was truncated at 200, so an older issue may duplicate it) — CRITICAL `lib/legacy.ts:7` Unbounded delete'); - expect(renderDeferredSection([{ ...records[0], issue: 9, reason: ALREADY_FILED_REASON }])).toContain('- Deferred: #9 — CRITICAL `lib/legacy.ts:7` Unbounded delete'); - const repo2 = fixture(); - const short = shim(repo2, 'PRIVATE', 199); - const r2 = runBin(repo2, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--repo', 'acme/app', '--json'], { PATH: `${short.dir}:${process.env.PATH}` }); - expect(r2.status).toBe(EXIT.ok); - expect(r2.err).not.toContain('DEFERRED_SEARCH'); - expect(JSON.parse(r2.out)[0]).toEqual({ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete', issue: 900, url: 'https://example.test/acme/app/issues/900' }); - }); - - test.skipIf(process.platform === 'win32')('with --repo the visibility read passes the repository positionally, so a MEDIUM finding on a public repository waits for a human and stays unfiled', () => { - const repo = fixture(); - const classified = path.join(repo.root, 'classified.json'); - fs.writeFileSync(classified, JSON.stringify([ - finding('lib/mail.ts:3:pii', 'Notification goes to jane.roe@acme-corp.io on every retry'), - finding('lib/legacy.ts:7:data-loss', 'Unbounded delete'), - ])); - const pub = shim(repo, 'PUBLIC', 0); - const r = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--repo', 'acme/app', '--json'], { PATH: `${pub.dir}:${process.env.PATH}` }); - expect(r.status).toBe(EXIT.ok); - const records = JSON.parse(r.out); - expect(records[0].issue).toBeNull(); - expect(records[0].reason).toMatch(/^publication scan: MEDIUM pii\.email needs a human before this text is posted$/); - expect(records[1]).toMatchObject({ issue: 900 }); - const calls = fs.readFileSync(pub.log, 'utf8'); - expect(calls).toContain('repo view acme/app --json visibility --jq .visibility'); - expect(calls).not.toMatch(/^repo view .*--repo/m); - expect(calls.match(/^issue create /gm)?.length).toBe(1); - expect(calls).not.toContain('jane.roe@acme-corp.io'); - expect(r.err).toContain('DEFERRED: unfiled (publication scan: MEDIUM pii.email needs a human before this text is posted) CRITICAL lib/mail.ts:3'); - }); -}); - describe('checklist rules with ids (docs/review-policy.md)', () => { const checklist = fs.readFileSync(path.resolve(import.meta.dir, '../review/checklist.md'), 'utf8'); const policy = fs.readFileSync(path.resolve(import.meta.dir, '../docs/review-policy.md'), 'utf8'); diff --git a/test/review-scope.test.ts b/test/review-scope.test.ts index 1bf9e53a5..932c3d562 100644 --- a/test/review-scope.test.ts +++ b/test/review-scope.test.ts @@ -3,7 +3,7 @@ * bin/gstack-review-scope). Five Captains hit a loop that re-read the whole * diff each pass and fixed neighbours in pre-existing code until the cycle * cap. Under `review_scope: frozen` pass 1 fixes the diff and defers blocking - * findings in unchanged code to issues; pass 2+ fixes only blocking findings + * findings in unchanged code to the PR body; pass 2+ fixes only blocking findings * in the range since the previous pass (or the diff) and notes the rest. */ import { afterEach, describe, expect, test } from 'bun:test'; @@ -12,7 +12,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; import { - isBlocking, kindOf, locusOf, mapStateMatrixOutput, parseHunks, raisedBy, renderDeferredIssue, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, + deferredRecord, isBlocking, kindOf, locusOf, mapStateMatrixOutput, parseHunks, raisedBy, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, UNTESTED_CELL, type ScopedFinding, type ScopeFinding, } from '../lib/review-scope'; import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; @@ -166,34 +166,42 @@ describe('scope decisions', () => { describe('deferral rendering', () => { const finding: ScopedFinding = { fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts', line: 7, severity: 'CRITICAL', category: 'data-loss', summary: 'Unbounded delete\nwithout a where clause', fix: 'add the tenant filter', specialist: 'security', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }; - test('a deferred issue shows the severity and Blocks the reviewer stated beside the collected label (GST-766)', () => { - const issue = renderDeferredIssue({ ...finding, stated: { severity: 'Low', blocks: 'no' } }, { repo: 'acme/app', pr: 42 }); - expect(issue.body).toContain('- **Severity:** CRITICAL (the reviewer wrote Severity: Low, Blocks: no)'); - expect(renderDeferredIssue({ ...finding, stated: { blocks: 'yes\n- **Injected:** x' } }, {}).body).toContain('- **Severity:** CRITICAL (the reviewer wrote Blocks: yes - **Injected:** x)'); - expect(renderDeferredIssue(finding, {}).body).toContain('- **Severity:** CRITICAL\n'); + test('a deferred record keeps the collected label as its severity and the reviewer\'s stated severity and Blocks beside it (GST-766)', () => { + expect(deferredRecord({ ...finding, stated: { severity: 'Low', blocks: 'no' } })).toMatchObject({ severity: 'CRITICAL', stated: 'Severity: Low, Blocks: no' }); + expect(deferredRecord({ ...finding, stated: { blocks: 'yes\n- **Injected:** x' } })).toMatchObject({ severity: 'CRITICAL', stated: 'Blocks: yes - **Injected:** x' }); + expect(deferredRecord(finding)).toEqual({ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete\nwithout a where clause' }); }); - test('the issue names the origin PR, the finding and a marker that keeps a rerun from filing twice; the body section references and never closes', () => { - const issue = renderDeferredIssue(finding, { repo: 'acme/app', pr: 42, branch: 'feat/x', head: 'a'.repeat(40) }); - expect(issue.title).toBe('Deferred review finding: lib/legacy.ts:7 — Unbounded delete without a where clause'); - expect(issue.body).toContain('acme/app#42 (head aaaaaaaaaaaa)'); - expect(issue.body).toContain('- **Severity:** CRITICAL'); - expect(issue.body).toContain('- **Suggested fix:** add the tenant filter'); - expect(issue.body).toContain('- **Found by:** security'); - expect(issue.marker).toBe(''); - expect(issue.body.trimEnd().endsWith(issue.marker)).toBe(true); + test('deferred findings stay on the PR: the body section lists them, says no issue was filed, and never closes anything (R85)', () => { const section = renderDeferredSection( - [{ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete', issue: 77 }, - { fingerprint: 'lib/old.ts:1:auth', path: 'lib/old.ts:1', severity: 'CRITICAL', summary: 'Missing owner check', issue: null, reason: 'gh is not installed' }], + [{ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete' }, + { fingerprint: 'lib/old.ts:1:auth', path: 'lib/old.ts:1', severity: 'CRITICAL', stated: 'Blocks: no', summary: 'Missing owner\ncheck' }], [{ path: 'src/auth.ts', line: 42, severity: 'INFORMATIONAL', summary: 'rename b', advisory: true }]); expect(section).toContain('## Deferred findings'); - expect(section).toContain('- Deferred: #77 — CRITICAL `lib/legacy.ts:7` Unbounded delete'); - expect(section).toContain('- Deferred: unfiled (gh is not installed) — CRITICAL `lib/old.ts:1` Missing owner check'); + expect(section).toContain('not filed as issues'); + expect(section).toContain('- Deferred: CRITICAL `lib/legacy.ts:7` Unbounded delete'); + expect(section).toContain('- Deferred: CRITICAL (the reviewer wrote Blocks: no) `lib/old.ts:1` Missing owner check'); + expect(section).not.toMatch(/#\d|unfiled/); expect(section).toContain('## Review notes'); expect(section).toContain('- INFORMATIONAL [ADVISORY] `src/auth.ts:42` rename b'); expect(section).not.toMatch(/\b(fixes|closes|resolves)\b/i); expect(renderDeferredSection([], [])).toBe(''); }); + + test('no gstack instruction files a deferred review or /ship finding as an issue (R85)', () => { + const files = ['ship/sections/review-army.md.tmpl', 'ship/sections/pr-body.md.tmpl', 'captain/templates/READY.md', 'docs/review-policy.md', 'docs/ship-policy.md', 'docs/unattended.md', 'crew/docs/captain.md', 'capy/skill/gstack/factory.md', 'lib/ship-policy.ts', 'lib/review-scope.ts', 'bin/gstack-review-scope']; + const filing = /\b(fil(e|es|ed|ing)|defers?)\b[^.;:,]{0,80}\b(as|to) (a |an |one )?(github )?issues?\b|\bone issue per\b|\bdeferred[- ]issues?\b|gstack-deferred-finding/gi; + const negated = (text: string, at: number) => /\b(never|not|no)\b[^.;:,]*$/i.test(text.slice(Math.max(0, at - 40), at)); + const hits = files.flatMap(f => fs.readFileSync(path.join(ROOT, f), 'utf8').split(/\n\s*\n/).map(para => para.replace(/\s+/g, ' ')) + .filter(para => /defer/i.test(para)) + .flatMap(para => [...para.matchAll(filing)].filter(m => !negated(para, m.index!)).map(m => `${f}: ${m[0]}`))); + expect(hits).toEqual([]); + for (const f of ['capy/skill/gstack/factory.md', 'crew/docs/captain.md']) { + const text = fs.readFileSync(path.join(ROOT, f), 'utf8').replace(/\s+/g, ' '); + expect(text, f).toMatch(/[Dd]eferred findings[^.]*never (become|filed as) issues/); + expect(text, f).toMatch(/one issue with (a named |an )owner/); + } + }); }); describe('loop summary', () => { @@ -321,81 +329,32 @@ describe('gstack-review-scope (bin)', () => { expect(JSON.parse(r.out).findings.map((x: ScopedFinding) => `${x.path}=${x.locus}/${x.scope}`)).toEqual(['src/café.ts=diff/fix', 'src/naïve.ts=diff/fix']); }); - test('defer without gh keeps the finding deferred as unfiled and still exits 0 (the PR body carries the reason), searches once before creating, and files nothing when the search fails; --dry-run files nothing', () => { + test('defer keeps each deferred finding on the PR and never runs gh, so no issue is filed (R85)', () => { const repo = fixture(); + const shim = path.join(repo.root, 'gh-shim'); + fs.mkdirSync(shim); + const log = path.join(shim, 'calls.log'); + fs.writeFileSync(path.join(shim, 'gh'), `#!/bin/sh\necho "$*" >> "${log}"\necho https://example.test/acme/app/issues/10\n`, { mode: 0o755 }); const classified = path.join(repo.root, 'classified.json'); fs.writeFileSync(classified, JSON.stringify([ { fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts', line: 7, severity: 'CRITICAL', category: 'data-loss', summary: 'Unbounded delete', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }, { fingerprint: 'src/auth.ts:42:naming', path: 'src/auth.ts', line: 42, severity: 'INFORMATIONAL', category: 'naming', summary: 'rename b', locus: 'diff', kind: 'defect', blocking: false, scope: 'note' }, { fingerprint: 'src/auth.ts:11:security', path: 'src/auth.ts', line: 11, severity: 'CRITICAL', category: 'security', summary: 'fixed', locus: 'diff', kind: 'defect', blocking: true, scope: 'fix' }, ])); - const dry = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--dry-run', '--json']); - expect(dry.status).toBe(EXIT.ok); - expect(JSON.parse(dry.out)).toEqual([{ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete', issue: null, reason: 'dry run' }]); - const noGh = path.join(repo.root, 'nogh'); - fs.mkdirSync(noGh); - fs.writeFileSync(path.join(noGh, 'gh'), '#!/bin/sh\necho "gh: not logged in" >&2; exit 4\n', { mode: 0o755 }); const out = path.join(repo.root, 'deferred.json'); - const unfiled = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--out', out], { PATH: `${noGh}:${process.env.PATH}` }); - expect(unfiled.status).toBe(EXIT.ok); - expect(unfiled.err).toMatch(/^DEFERRED: unfiled \(gh issue list failed: gh: not logged in\) CRITICAL lib\/legacy\.ts:7$/m); - expect(unfiled.out).toContain('- Deferred: unfiled (gh issue list failed: gh: not logged in) — CRITICAL `lib/legacy.ts:7` Unbounded delete'); - // the search failed, so nothing was created: a rate-limited rerun cannot duplicate the issue the marker exists to find - const listFails = path.join(repo.root, 'listfails'); - fs.mkdirSync(listFails); - const created = path.join(listFails, 'created'); - fs.writeFileSync(path.join(listFails, 'gh'), `#!/bin/sh\nif [ "$1" = issue ] && [ "$2" = list ]; then echo "gh: HTTP 403 rate limit" >&2; exit 1; fi\nif [ "$1" = issue ] && [ "$2" = create ]; then touch "${created}"; echo https://example.test/o/r/issues/1; exit 0; fi\nexit 1\n`, { mode: 0o755 }); - const rateLimited = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--json'], { PATH: `${listFails}:${process.env.PATH}` }); - expect(rateLimited.status).toBe(EXIT.ok); - expect(JSON.parse(rateLimited.out)[0]).toMatchObject({ issue: null, reason: 'gh issue list failed: gh: HTTP 403 rate limit' }); - expect(fs.existsSync(created)).toBe(false); + const r = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--out', out, '--json'], { PATH: `${shim}:${process.env.PATH}` }); + expect(r.status, r.err).toBe(EXIT.ok); + expect(JSON.parse(r.out)).toEqual([{ fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete' }]); + expect(r.err).toMatch(/^DEFERRED: CRITICAL lib\/legacy\.ts:7$/m); + expect(fs.existsSync(log)).toBe(false); const rendered = runBin(repo, 'gstack-review-scope', ['render', '--deferred', out, '--findings', classified]); expect(rendered.status).toBe(EXIT.ok); - expect(rendered.out).toContain('## Deferred findings'); + expect(rendered.out).toContain('- Deferred: CRITICAL `lib/legacy.ts:7` Unbounded delete'); expect(rendered.out).toContain('## Review notes'); expect(rendered.out).toContain('- INFORMATIONAL `src/auth.ts:42` rename b'); expect(rendered.out).not.toContain('src/auth.ts:11'); }); - test('defer through gh creates one issue per deferred finding and reuses an open issue that already carries the marker', () => { - const repo = fixture(); - const shim = path.join(repo.root, 'gh-shim'); - fs.mkdirSync(shim); - const log = path.join(shim, 'calls.log'); - fs.writeFileSync(path.join(shim, 'gh'), [ - '#!/bin/sh', `echo "$*" >> "${log}"`, - 'if [ "$1" = issue ] && [ "$2" = list ]; then', - ` echo '[{"number":9,"url":"https://example.test/acme/app/issues/9","body":"x y"},{"number":3,"url":"https://example.test/acme/app/issues/3","body":""}]'; exit 0; fi`, - 'if [ "$1" = issue ] && [ "$2" = create ]; then echo "https://example.test/acme/app/issues/10"; exit 0; fi', - 'echo "unsupported $*" >&2; exit 1', '', - ].join('\n'), { mode: 0o755 }); - const classified = path.join(repo.root, 'classified.json'); - fs.writeFileSync(classified, JSON.stringify([ - { fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts', line: 7, severity: 'CRITICAL', category: 'data-loss', summary: 'Unbounded delete', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }, - { fingerprint: 'lib/old.ts:1:auth', path: 'lib/old.ts', line: 1, severity: 'CRITICAL', category: 'auth', summary: 'Missing owner check', fix: 'check owner', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }, - // Reviewer text is untrusted: a security finding that quotes the credential it found must not reach a public issue. - { fingerprint: 'lib/keys.ts:3:security', path: 'lib/keys.ts', line: 3, severity: 'CRITICAL', category: 'security', summary: 'Hardcoded token ghp_' + 'Qm4zR7tK2wX9pL5vN8sD1fG6hJ3yB0cA7eU2', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }, - ])); - const r = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--repo', 'acme/app', '--json'], { PATH: `${shim}:${process.env.PATH}` }); - expect(r.status).toBe(EXIT.ok); - const records = JSON.parse(r.out); - expect(records.slice(0, 2)).toEqual([ - { fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts:7', severity: 'CRITICAL', summary: 'Unbounded delete', issue: 9, url: 'https://example.test/acme/app/issues/9', reason: 'already filed' }, - { fingerprint: 'lib/old.ts:1:auth', path: 'lib/old.ts:1', severity: 'CRITICAL', summary: 'Missing owner check', issue: 10, url: 'https://example.test/acme/app/issues/10' }, - ]); - expect(records[2].issue).toBeNull(); - expect(records[2].reason).toMatch(/^publication scan refused: HIGH /); - expect(r.err).toContain('DEFERRED: #9 CRITICAL lib/legacy.ts:7'); - expect(r.err).toContain('DEFERRED: #10 CRITICAL lib/old.ts:1'); - expect(r.err).toContain('DEFERRED: unfiled (publication scan refused: HIGH '); - const calls = fs.readFileSync(log, 'utf8'); - expect(calls.match(/^issue create /gm)?.length).toBe(1); - expect(calls.match(/^issue list /gm)?.length).toBe(1); - expect(calls).toContain('--search "gstack-deferred-finding" in:body'); - expect(calls).toContain('issue create --title=Deferred review finding: lib/old.ts:1 — Missing owner check --body-file=- --repo acme/app'); - expect(calls).not.toContain('Qm4zR7tK2wX9pL5vN8sD1fG6hJ3yB0cA7eU2'); - }); - // Generated by /ship Step 7 (coverage audit of the harness lane E diff). // Value: protects=summary reads this branch's review log through gstack-review-read and prints the REVIEW_LOOP counters that // `gstack-ship-receipt write --review-loop` consumes, counting only this invocation's ship-via review passes and tolerating a diff --git a/test/ship-frozen-scope-actor.test.ts b/test/ship-frozen-scope-actor.test.ts index 5c65d6ea9..7c14793ed 100644 --- a/test/ship-frozen-scope-actor.test.ts +++ b/test/ship-frozen-scope-actor.test.ts @@ -48,9 +48,9 @@ async function act(f: Fixture, options: { fix?: boolean; passes?: 1 | 2 } = {}) fs.writeFileSync(f.invoice, SAFE); } expect(await hook(f, 'Bash', { command: f.commands.defer })).toBeUndefined(); - expect(sh(f, f.commands.defer!)).toContain('DEFERRED: unfiled (dry run) CRITICAL payments.ts:2'); + expect(sh(f, f.commands.defer!)).toContain('DEFERRED: CRITICAL payments.ts:2'); const findings = classified.findings.map((x: any) => ({ fingerprint: x.fingerprint, severity: x.severity, action: x.scope === 'fix' ? (fix ? 'auto-fixed' : 'skipped') : 'deferred', - ...(x.scope === 'defer' ? { issue: null } : {}), locus: x.locus, kind: x.kind, blocking: x.blocking })); + locus: x.locus, kind: x.kind, blocking: x.blocking })); const row = (extra: Record) => ({ skill: 'review', timestamp: new Date().toISOString(), status: 'issues_found', issues_found: 0, critical: 0, informational: 0, quality_score: 10.0, specialists: {}, findings, commit: sh(f, 'git rev-parse --short HEAD').trim(), via: 'ship', completed: true, converged: false, cycles: 1, pass: 1, review_scope: 'frozen', range_base: null, ...extra }); diff --git a/test/size-ledger/ship/keep-deferred-findings-on-the-pr.json b/test/size-ledger/ship/keep-deferred-findings-on-the-pr.json new file mode 100644 index 000000000..ab7c827ee --- /dev/null +++ b/test/size-ledger/ship/keep-deferred-findings-on-the-pr.json @@ -0,0 +1,4 @@ +{ + "bytes": -304, + "why": "Step 9.4 item 3.5: deferred findings stay on the PR; gstack-review-scope defer files no issue" +}