fix: native FIXABLE raises only on gate and risk review classes (GST-987)

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

bin/gstack-review-dispatch

@@ -20,7 +20,7 @@ import * as fs from 'node:fs';
 import * as path from 'node:path';
 import { EXIT, exitTableHelp } from '../lib/headless-artifacts';
 import { BRIEF_ROLES, BriefError, briefFile, checkReviewedTree, checkoutRoot, checkoutTree, promptHash, renderBrief, sectionTexts, withWorkspace, type BriefRole, type Slots, type TreeCheck } from '../lib/review-brief';
-import { cleanMarkers, collect, mayBeUnavailable, parseFindings, readJsonl, readLedger, readerKey, recordJudgment, recordSent, appendEvent, verify, type JudgedLine, type MergedLine, type VerifyRow } from '../lib/review-ledger';
+import { cleanMarkers, collect, fixableRaises, mayBeUnavailable, parseFindings, readJsonl, readLedger, readerKey, recordJudgment, recordSent, appendEvent, verify, type JudgedLine, type MergedLine, type VerifyRow } from '../lib/review-ledger';
 import { buildManifest, chunkCount, chunkOf, manifestPath, markReader, readManifest, renderStage, writeManifest, STAGES, type Extra, type Manifest, type PassReader, type ReaderState, type Stage } from '../lib/review-pass';
 import type { Dispatch } from '../lib/review-depth';
 import { runScriptSync } from '../lib/script-invocation';
@@ -329,9 +329,9 @@ function main(): number {
       process.stdout.write(`COLLECT_UNAVAILABLE: reader=${reader} sha256=${r.sha} reason=${JSON.stringify(why)} (missing outside coverage: unavailable, never clean)\n`);
       return EXIT.ok;
     }
-    const r = collect(ledger, reader, output, merged, path.resolve(outFile!), unavailable, tree);
+    const mf = flag('--manifest');
+    const r = collect(ledger, reader, output, merged, path.resolve(outFile!), unavailable, tree, fixableRaises(mf ? readManifest(mf).plan_lines : undefined));
     if (tree && !tree.ok) {
-      const mf = flag('--manifest');
       if (mf) { const m = readManifest(mf); markReader(m, reader, 'failed', { output: outFile, error: `reviewed another tree: ${tree.reported}` }); writeManifest(mf, m); }
       process.stdout.write(wrongTree(reader, tree));
       return EXIT.fail;

changelog.d/gst-987-fixable-by-class.md

@@ -0,0 +1,5 @@
+**/ship's finding collector lets the native reviewer's own Severity field decide outside gate and risk reviews.**
+
+### Fixed
+
+- On a review whose class is not gate and that carries no risk reason (security, secrets, auth, money, user data), a native adversarial finding marked FIXABLE with its own `Severity: Medium` or `Severity: Low` now collects at that severity instead of CRITICAL. Gate and risk reviews, depth errors and any collect without a pass manifest keep collecting FIXABLE as CRITICAL, and a FIXABLE finding with no Severity field still collects as CRITICAL. (GST-987)

docs/review-policy.md

@@ -530,7 +530,10 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
    second line for the same field starts another: `Severity` sets its label (a label leading the finding stays its floor),
    `Blocks: yes` makes it CRITICAL and `Blocks: no` never lowers it; an inline
    `Blocks: yes` is CRITICAL too, and with no finding above it is one. The native
-   adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). An unnumbered, unlabeled bullet with no file:line
+   adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167) on the gate class, on any `risk:` or `error:`
+   depth reason, and whenever collect has no pass manifest to read the plan from. On any other class (collect
+   `--manifest` reads `REVIEW_CLASS:` and `REVIEW_DEPTH: … reasons=`), a native finding's own `Severity` field decides and
+   FIXABLE beside it raises nothing; a native finding with no `Severity` field still collects as CRITICAL (GST-987). An unnumbered, unlabeled bullet with no file:line
    under a paragraph finding whose Severity or Blocks line has not come yet is that finding's detail, so the field line after it
    lands on the finding; a numbered item is its own finding.
    A negated severity word still collects at its label ("found nothing CRITICAL", "No P0/P1 issues"), so a reader's

lib/review-ledger.ts

@@ -102,8 +102,10 @@ export interface MergedLine { id: string; reader: string; label: string; stated?
  * starts the next finding, except an unlabeled, unnumbered bullet with no file:line under a paragraph finding whose fields have not
  * come yet, which is its detail and joins its text (GSTA-196: 4 findings with detail bullets collected as 8). An inline
  * `Blocks: yes` is CRITICAL too. The native voice's FIXABLE collects as CRITICAL; other labels stay as written for lead judgment. When in doubt the parser merges rather than drops: a lost blocker costs the gate.
+ * With fixableRaises false (a review outside the gate class with no risk reason, GST-987) a native finding's own Severity field decides
+ * and FIXABLE beside it raises nothing; a native finding with no Severity field still collects FIXABLE as CRITICAL.
  */
-export function parseFindings(reader: string, output: string): Array<{ label: string; raw: string }> {
+export function parseFindings(reader: string, output: string, fixableRaises = true): Array<{ label: string; raw: string }> {
   type Finding = { label: string; raw: string; fields: Set<string>; list: boolean; lead?: string };
   const out: Finding[] = [];
   let section: string | null = null;
@@ -116,8 +118,8 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
   const strongestOf = (labels: Array<string | undefined>) => labels.reduce<string | undefined>((a, b) => (a === undefined ? b : b === undefined ? a : stronger(a, b)), undefined);
   const strongest = (t: string) => strongestOf([...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)));
   const fieldOf = (t: string) => { const ls = [...t.matchAll(SEVERITY_FIELD)].flatMap(m => (m[1] ?? m[2]!).match(FIELD_LABELS)!); return ls.length === 1 ? ls[0] : undefined; };
-  const critical = (t: string) => BLOCKS_YES.test(t) || (role === 'adversarial-native' && /(?<![A-Za-z0-9])FIXABLE(?![A-Za-z0-9])/.test(t));
-  const leadOf = (t: string) => { const l = LEAD_LABEL.exec(t)?.[1]; return l === undefined ? undefined : named(l); };
+  const critical = (t: string) => BLOCKS_YES.test(t) || (fixableRaises && role === 'adversarial-native' && /(?<![A-Za-z0-9])FIXABLE(?![A-Za-z0-9])/.test(t));
+  const leadOf = (t: string) => { const l = LEAD_LABEL.exec(t)?.[1]; return l === undefined || !fixableRaises ? l : named(l); };
   const labelOf = (t: string) => {
     const field = fieldOf(t);
     if (field === undefined) return strongest(t);
@@ -179,6 +181,18 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
   return out.map(({ label, raw }) => ({ label, raw }));
 }
 
+/**
+ * Whether the native voice's FIXABLE raises a finding with its own Severity field to CRITICAL (GST-987): true on the gate class, on any
+ * risk reason (security, secret, auth, money, user data), on a depth error, and whenever the plan's REVIEW_CLASS or REVIEW_DEPTH
+ * reasons cannot be read.
+ */
+export function fixableRaises(planLines?: readonly string[]): boolean {
+  const cls = planLines?.map(l => /^REVIEW_CLASS: (\S+)/.exec(l)?.[1]).find(Boolean);
+  const reasons = planLines?.map(l => /^REVIEW_DEPTH: .*?\breasons=(\S+)/.exec(l)?.[1]).find(Boolean);
+  if (!cls || !reasons) return true;
+  return cls === 'gate' || reasons.split(',').some(r => /^(?:risk|error):/.test(r));
+}
+
 /**
  * The clean marker each reader's own brief prescribes for a review that found nothing: `NO FINDINGS` alone on a line for the
  * specialists, the batched specialist and the targeted reader (ship/sections/review-army.md, the specialist brief; review/specialists/*.md),
@@ -201,11 +215,11 @@ const hasCleanMarker = (reader: string, output: string) => output.replace(/\r\n/
  * review: false. With unavailable (the outside voice only) the output is its failure text, recorded as missing coverage. With a
  * failed tree check (GST-482: the reply reviewed another checkout, or names none) nothing is merged and the output is no review.
  */
-export function collect(ledger: string, reader: string, output: string, mergedFile: string, outputFile?: string, unavailable?: string, tree?: TreeCheck): { sha: string; lines: MergedLine[]; review: boolean } {
+export function collect(ledger: string, reader: string, output: string, mergedFile: string, outputFile?: string, unavailable?: string, tree?: TreeCheck, raises = true): { sha: string; lines: MergedLine[]; review: boolean } {
   const sha = promptHash(output);
   const prior = readLedger(ledger).filter(e => e.event === 'output' && e.reader === reader).length;
   const wrongTree = !unavailable && tree !== undefined && !tree.ok;
-  const lines = unavailable || wrongTree ? [] : parseFindings(reader, output).map((f, i) => {
+  const lines = unavailable || wrongTree ? [] : parseFindings(reader, output, raises).map((f, i) => {
     const s = stated(f.raw);
     return { id: `${reader}#${prior ? `${prior + 1}.` : ''}${i + 1}`, reader, label: f.label, ...(s ? { stated: s } : {}), raw: f.raw, output_sha256: sha };
   });

test/fixtures/review-ledger/gsta-141-native.md

@@ -0,0 +1,6 @@
+# garrytan/gstack-evals#141: native adversarial findings #4 and #6, head a752186 (tree 0ac70e872724875710ccc47f9c93b7b30a58604a)
+Copied verbatim from /home/user/.capy/work/ship-141/run/p2/out/adversarial-native.txt
+
+4. **driver.ts:352, 561 and 618 (the C1 ruling): skill-eval sessions now end with background Agents still running and leave no structured record.** run-host.ts:352 passes no wait. Claude Code 2.1.314 runs Agent calls in the background by default. So a skill-eval `--host claude` session ends and is graded on its "agents launched, waiting" turn, which is the original GST-734 bug. Its record also has no `session_end`, only a line in cli-stderr.log. The CREW rule asks for a per-arm count of sessions that ended with tasks pending, and these records can't give that count. Grades for skills that hand work to Agents may be deflated with nothing marking them. Old cached sessions do not mix in, because adapterSha256 hashes driver.ts. The fix: keep emitting `session_end` (cause `tasks_pending`, `waited_ms` 0) when no wait is set, and leave the hold opt-in. That reverses the C1 test's `"session_end" in r.outcome === false` assertion, so the C1 owner needs to agree. FIXABLE. Severity: Medium. Blocks: no
+
+6. **driver.ts:404-407: the timeout branch for an idle session sets `overdue = true` without checking `stop`.** Here is the case. A background agent's refused request sets a stop while the main agent is idle, such as `cap_stop`, or an infra failure does. The timer then fires during abort teardown. The record ends up with a `cap_stop` or infra termination but `session_end.cause` "deadline". The `watch` interval already checks `stop`. Fix: `else if (idleSince !== null && !stop)`. FIXABLE. Severity: Low. Blocks: no

test/review-fixable-class.test.ts

@@ -0,0 +1,51 @@
+import { describe, expect, test } from 'bun:test';
+import { spawnSync } from 'node:child_process';
+import * as fs from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+import { fixableRaises, parseFindings } from '../lib/review-ledger';
+
+const ROOT = path.resolve(import.meta.dir, '..');
+const NATIVE = fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/gsta-141-native.md'), 'utf8').split('\n').filter(l => /^\d+\. /.test(l)).join('\n\n');
+const plan = (cls: string, reasons: string) => [`REVIEW_CLASS: ${cls} (x)`, `REVIEW_DEPTH: full runtime_lines=9 rules=r reasons=${reasons} codes=none`];
+const CODE = plan('code', 'runtime=9');
+
+describe('native FIXABLE by review class (GST-987)', () => {
+  // Value: protects=outside the gate class with no risk reason a native finding's own Severity field decides, so gstack-evals #141's
+  //   eval-driver findings (FIXABLE. Severity: Medium/Low. Blocks: no) collect as Medium and Low, while gate, risk and unreadable plans
+  //   keep normalizer v2; fails_when=FIXABLE raises a fielded finding on a code-class plan, or a gate, risk, error or missing plan stops
+  //   raising it; why_new=GEVA-2 reported #141 collecting both as CRITICAL; seam=fixableRaises, parseFindings and collect --manifest;
+  //   no_claim=which class a diff belongs to (lib/review-depth.ts decides)
+  test('#141 native #4 and #6 collect at their own Severity on a code-class plan', () => {
+    expect(parseFindings('adversarial-native', NATIVE, fixableRaises(CODE)).map(f => f.label)).toEqual(['Medium', 'Low']);
+    expect(parseFindings('adversarial-native', NATIVE).map(f => f.label)).toEqual(['CRITICAL', 'CRITICAL']);
+  });
+  test('gate class, any risk reason, a depth error or an unreadable plan keeps normalizer v2', () => {
+    for (const lines of [plan('gate', 'gate-class,runtime=9'), plan('code', 'risk:money,runtime=9'), plan('code', 'risk:data,runtime=9'), plan('code', 'error:DIFF,runtime=9'), plan('code', ''), [CODE[0]!], [], undefined])
+      expect(fixableRaises(lines)).toBe(true);
+    expect(fixableRaises(CODE)).toBe(false);
+    expect(fixableRaises(plan('docs', 'runtime=0'))).toBe(false);
+  });
+  test('on a code-class plan FIXABLE without a Severity field still collects CRITICAL, and Blocks: yes still raises', () => {
+    expect(parseFindings('adversarial-native', '1. Race in a.ts:3. FIXABLE', false).map(f => f.label)).toEqual(['CRITICAL']);
+    expect(parseFindings('adversarial-native', 'FIXABLE: race in a.ts:3\nSeverity: Medium\nBlocks: no', false).map(f => f.label)).toEqual(['Medium']);
+    expect(parseFindings('adversarial-native', 'FIXABLE: race in a.ts:3\nSeverity: Medium\nBlocks: yes', false).map(f => f.label)).toEqual(['CRITICAL']);
+  });
+  test('collect reads the class from --manifest and defaults to the gate class without one', () => {
+    const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gst987-'));
+    const out = path.join(dir, 'native.txt');
+    fs.writeFileSync(out, NATIVE + '\n');
+    const manifest = path.join(dir, 'pass-manifest.json');
+    fs.writeFileSync(manifest, JSON.stringify({ version: 1, plan_lines: CODE, readers: [] }));
+    const labels = (extra: string[]) => {
+      const ledger = path.join(dir, `${extra.length}.jsonl`);
+      const merged = path.join(dir, `${extra.length}.merged.jsonl`);
+      const r = spawnSync(path.join(ROOT, 'bin/gstack-review-dispatch'), ['collect', '--ledger', ledger, '--role', 'adversarial-native', '--output', out, '--merged', merged, ...extra], { cwd: ROOT, encoding: 'utf8', timeout: 60_000 });
+      expect(r.stderr).toBe('');
+      return fs.readFileSync(merged, 'utf8').trim().split('\n').map(l => JSON.parse(l).label);
+    };
+    expect(labels(['--manifest', manifest])).toEqual(['Medium', 'Low']);
+    expect(labels([])).toEqual(['CRITICAL', 'CRITICAL']);
+    fs.rmSync(dir, { recursive: true, force: true });
+  });
+});

test/review-ledger-invariant.test.ts

@@ -11,6 +11,7 @@ const FIELD = new RegExp(`(?:(?:^|\\| )(?:[-*+>]\\s+|\\d+[.)]\\s+)?\\**\\s*Sever
 const LEAD = new RegExp(`(?:^|\\| )[^\\p{L}\\p{N}]*(?:\\d+[.)]\\s*[^\\p{L}\\p{N}]*)?(?:\\w[\\w./-]*:\\d+(?:[-–:,]\\s*\\d+)*\\S*\\s+[^\\p{L}\\p{N}]*)?(?:${LABELS})(?![a-z])`, 'u');
 const HEADING = new RegExp(`^(?:#{1,6}\\s*|\\*\\*)(?:${LABELS})\\b`, 'm');
 const LABEL = new RegExp(`${LABELS}`, 'gi');
+const LEAD_NO_FIXABLE = new RegExp(LEAD.source.replace('|FIXABLE|INVESTIGATE', ''), 'u');
 
 describe('parseFindings is never weaker than main (GST-976 invariant)', () => {
   // Value: protects=no input the suite pins, no reader fixture and no Severity field shape collects below the label main (82e9d7ec)
@@ -18,8 +19,10 @@ describe('parseFindings is never weaker than main (GST-976 invariant)', () => {
   //   leading label, a label heading, Blocks: yes or the native voice's FIXABLE raises it; fails_when=a field chain (High/Critical) keeps
   //   its weaker label (b3ddd6c) or a field whose clause runs on stops counting and a body P0 raises the finding (f3810d4);
   //   why_new=three GST-976 review rounds each found a shape the row tables missed; seam=parseFindings; no_claim=that main's label was
-  //   right (it is the floor), or coverage of stored private reader outputs (checked outside the repo)
-  test('every pinned input, reader fixture and generated field shape', () => {
+  //   right (it is the floor), or coverage of stored private reader outputs (checked outside the repo). GST-987: the gate class (and every
+  //   risk class) keeps this rule; outside it the native voice's FIXABLE no longer counts as a raise, so a fielded finding collects at its
+  //   field unless a lead, a label heading or Blocks: yes raises it
+  test.each([['gate', true], ['code', false]] as const)('every pinned input, reader fixture and generated field shape (%s class)', (_cls, raises) => {
     const { entries, shapes } = JSON.parse(fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/main-labels.json'), 'utf8'));
     const { bodies, fields: tails, joins, readers, generated } = shapes;
     for (const [b, f, j, r, main] of generated) entries.push({ reader: readers[r], output: bodies[b] + joins[j] + tails[f], main });
@@ -27,14 +30,14 @@ describe('parseFindings is never weaker than main (GST-976 invariant)', () => {
     let findings = 0;
     for (const e of entries as Array<{ reader: string; output?: string; fixture?: string; main: string[] }>) {
       const output = e.output ?? fs.readFileSync(path.join(ROOT, 'test/fixtures', e.fixture!), 'utf8');
-      const got = parseFindings(e.reader, output);
+      const got = parseFindings(e.reader, output, raises);
       if (!e.main.length) continue;
       if (got.length !== e.main.length) { bad.push(`count ${got.length}/${e.main.length} ${JSON.stringify(output.slice(0, 80))}`); continue; }
       got.forEach((f, i) => {
         findings++;
         const fields = [...f.raw.matchAll(FIELD)].map(m => (m[1] ?? m[2])!);
         const single = fields.length === 1 && fields[0]!.match(LABEL)!.length === 1 ? fields[0]!.match(LABEL)![0] : undefined;
-        const raised = /Blocks:?\**:?\s*\**\s*yes/i.test(f.raw) || (e.reader.startsWith('adversarial-native') && /FIXABLE/.test(f.raw)) || LEAD.test(f.raw) || HEADING.test(output);
+        const raised = /Blocks:?\**:?\s*\**\s*yes/i.test(f.raw) || (raises && e.reader.startsWith('adversarial-native') && /FIXABLE/.test(f.raw)) || (raises ? LEAD : LEAD_NO_FIXABLE).test(f.raw) || HEADING.test(output);
         const ok = single === undefined ? (rank(f.label) ?? 0) >= (rank(e.main[i]) ?? 0)
           : (rank(f.label) ?? 0) >= (rank(single) ?? 0) && (raised || (rank(f.label) ?? 0) === (rank(single) ?? 0));
         if (!ok) bad.push(`${e.reader} main=${e.main[i]} got=${f.label} field=${single ?? '-'} ${JSON.stringify(f.raw.slice(0, 100))}`);