fix(ship): defer the paragraph-split fix; collect is main's, tests lock in every label (GST-435)

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

changelog.d/gst-435-collect-labels.md

@@ -1,13 +1,12 @@
-**/ship's review collection keeps a list item's title when its label first appears in a later paragraph, and no reviewer wording can hide a blocker.** In HARN-15's ship2 run the native adversarial voice wrote "4. **The host lock is not atomic** (`qm-stack.ts:66-76`)", two detail bullets, and then an indented "Fix: … FIXABLE. Severity: Low." paragraph. `collect` stored the finding as the Fix paragraph alone, so the lead could not tell which finding it was. What reviewers see is unchanged, and every brief renders byte for byte as before.
-
-### Fixed
-- **A list item whose label first appears in an indented later paragraph collects as that item.** The finding now starts with the item's first line. A labeled item's own labeled later paragraph is still its own finding, and an indented JSON finding or label heading under the item is read as before.
+**/ship's review collection keeps every label a reviewer writes, and new tests lock that in.** GST-435 tried three ways to stop a reader's own closing summary ("I found three INFORMATIONAL issues and no CRITICAL ones", "Neither finding is CRITICAL.") from collecting as a CRITICAL finding. Each version lost a real blocker in review, for example "The token leak in log.ts:3 blocks the merge. Nothing else is CRITICAL." A fix that kept a list item's title when its label first shows up in a later paragraph also put labels on the wrong finding in review. Collection is unchanged from before, and the tests now hold it there. What reviewers see is unchanged too, and every brief renders byte for byte as before.
 
 ### Known
-- **A reader's own closing summary still collects at its label.** Lines like "I found three INFORMATIONAL issues and no CRITICAL ones", "found nothing CRITICAL" and GEVA-77's "Neither finding is CRITICAL." collect as CRITICAL, and `judge` won't let the lead drop them, so the lead folds each one. Three drafts of a rule that dropped such labels each lost a real blocker in review (for example "The token leak in log.ts:3 blocks the merge. Nothing else is CRITICAL."), so the drop was removed. A follow-up will tag these lines `summary_candidate` and let `judge` dismiss a tagged line on record. "Severity: Low (not High)" collects as High, the same as before.
+- **A reader's own closing summary still collects at its label.** Lines like "I found three INFORMATIONAL issues and no CRITICAL ones", "found nothing CRITICAL" and GEVA-77's "Neither finding is CRITICAL." collect as CRITICAL, and `judge` won't let the lead drop them, so the lead folds each one. A follow-up will tag these lines `summary_candidate` and let `judge` dismiss a tagged line on record.
+- **A list item whose label first appears in an indented later paragraph still collects as that paragraph alone.** In HARN-15's ship2 run, native item 4 ("The host lock is not atomic") collected as its "Fix: …" paragraph without its title. The fix is deferred to the same follow-up.
+- "Severity: Low (not High)" collects as High, as before.
 
 ### Tests
-- `test/review-dispatch.test.ts`: every line the three review rounds found losing its label (exception clauses, negated subjects, "else" and "other", a blocker named in an earlier sentence), fix-check 5's table rows and INFORMATIONAL cases, and harness-evals #152's native FIXABLE findings collect at their label; the HARN-15 and GEVA-77 summaries are listed as known phantoms that keep their label; the ship2 native reply collects 6 findings with item 4's title. Fixtures under `test/fixtures/review-ledger/` (already registered to that test).
+- `test/review-dispatch.test.ts`: every line the GST-435 review rounds found losing or lowering a label collects at its label, including exception clauses, negated subjects, "else" and "other", and a blocker named in an earlier sentence. So do fix-check 5's table rows and harness-evals #152's native FIXABLE findings. The HARN-15 and GEVA-77 summaries are listed as known phantoms that keep their label. An indented JSON finding, an indented label heading and a later `Blocks: yes` under a list item keep their label on the right finding. Fixtures under `test/fixtures/review-ledger/` (already registered to that test).
 
 ### Docs
-- `docs/review-policy.md` (collect) describes the later-paragraph rule and the closing-summary phantom.
+- `docs/review-policy.md` (collect) says a closing summary collects at its label and the lead folds it.

docs/review-policy.md

@@ -478,8 +478,6 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
    adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). 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.
-   An indented later paragraph that gives an unlabeled list item its first label (`Fix: ... FIXABLE. Severity: Low.`) collects
-   as that item, its first line included.
    A phantom finding costs a fold; a lost one costs the gate.
    A reply with no finding counts as a review only with the clean marker its own brief asks for: `NO FINDINGS` alone on a
    line for the specialists, the batched specialist, the targeted reader, the Red Team and fix-check, the `Recommendation:`

lib/review-ledger.ts

@@ -73,8 +73,7 @@ export interface MergedLine { id: string; reader: string; label: string; raw: st
  * `Blocks: yes` makes it CRITICAL, `Blocks: no` never lowers it; with no finding open it is one, carrying the paragraph's subject.
  * A list item, a heading, a bold or post-blank paragraph at the margin, or a line naming a file:line after the open finding's fields
  * 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 indented later
- * paragraph that first labels an unlabeled list item collects as that item, its first line included (GST-435). An inline
+ * 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.
  */
 export function parseFindings(reader: string, output: string): Array<{ label: string; raw: string }> {
@@ -84,8 +83,6 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
   let current: Finding | null = null;
   let head = '';
   let gap = false;
-  let item: string | null = null;
-  let itemFinding: Finding | null = null;
   const role = reader.split('/')[0];
   const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l);
   const strongest = (t: string) => [...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)).reduce<string | undefined>((a, b) => (a === undefined ? b : stronger(a, b)), undefined);
@@ -113,10 +110,8 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
       else current = null;
       continue;
     }
-    if (!indented && !LIST_ITEM.test(t)) { item = null; itemFinding = null; }
     const heading = LABEL_HEADING.exec(t);
     if (heading) { section = named(heading[1]!); current = null; head = ''; continue; }
-    if (indented && item && !itemFinding && !LIST_ITEM.test(t) && !t.startsWith('{') && (label || blocks)) { current = itemFinding = push(blocks ? 'CRITICAL' : label!, `${item} | ${t}`); continue; }
     if (!indented && HEADING.test(t)) { section = null; current = null; }
     if (t.startsWith('{')) {
       try {
@@ -126,16 +121,14 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
     }
     const detail = listItem && /^[-*+]\s/.test(t) && !locus && !label && !section && !!current && !current.list && !current.fields.size;
     if (detail && !blocks) { current!.raw += ` | ${t}`; continue; }
-    if (listItem && !detail) { item = t; itemFinding = null; }
     const fresh = (listItem && !detail) || (!indented && !detail && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus);
     if (fresh) { current = null; head = t; }
     const inherited = section && listItem ? section : undefined;
     const found = label && inherited ? stronger(inherited, label) : label ?? inherited;
-    if (found) { current = push(blocks ? 'CRITICAL' : found, t); if (!indented) head = t; if (item === t) itemFinding = current; continue; }
+    if (found) { current = push(blocks ? 'CRITICAL' : found, t); if (!indented) head = t; continue; }
     if (blocks && !t.startsWith('{')) {
       if (current) { current.label = 'CRITICAL'; current.raw += ` | ${t}`; continue; }
       current = push('CRITICAL', head && head !== t ? `${head} | ${t}` : t);
-      if (item === t) itemFinding = current;
       continue;
     }
     if (!fresh && !current && (!LIST_ITEM.test(head) || locus)) head = t;

test/review-dispatch.test.ts

@@ -482,27 +482,25 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
     expect(parseFindings('outside-voice', 'Severity: Low (not High)').map(f => f.label)).toEqual(['High']);
   });
 
-  // Value: protects=a list item whose label first appears in an indented later paragraph ("Fix: ... FIXABLE. Severity: Low.") collects as
-  //   that item, title included (HARN-15 ship2 native item 4 "The host lock is not atomic" collected as a finding that began "Fix: create
-  //   the file", so judge could not tell which finding it was); fails_when=the item's finding loses its first line or the reply collects
-  //   more or fewer than 6; why_new=#3677's detail merge covered margin bullets only, not an item's later paragraphs; seam=parseFindings;
-  //   no_claim=a labeled item's labeled later paragraph (it stays its own finding, as above)
-  test('an indented later paragraph that first labels a list item collects as that item, title included', () => {
+  // Value: protects=item layouts keep their label on the right finding: an indented JSON finding keeps its JSON severity, an indented
+  //   label heading sets the section for the next item, and a later Blocks: yes raises the finding a Severity field line or JSON line
+  //   opened; fails_when=any of these collects a lower label or puts a label on another finding; why_new=GST-435's paragraph-split draft
+  //   broke each of these and was deferred; seam=parseFindings; no_claim=the split shape (known: HARN-15 ship2 native item 4 still
+  //   collects as its "Fix:" paragraph alone, without its title, until the follow-up)
+  test('item layouts keep their label on the right finding; the later-paragraph split is a known deferred shape', () => {
     const native = fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/harn15-ship2-p2-adversarial-native.txt'), 'utf8');
     const found = parseFindings('adversarial-native', native);
-    expect(found.map(f => [f.label, f.raw.slice(0, 3)])).toEqual([['CRITICAL', '1. '], ['CRITICAL', '2. '], ['CRITICAL', '3. '], ['CRITICAL', '4. '], ['CRITICAL', '5. '], ['Low', '6. ']]);
-    expect(found[3]!.raw).toStartWith('4. **The host lock is not atomic** (`qm-stack.ts:66-76`).');
-    expect(found[3]!.raw).toContain(' | Fix: create the file with `writeFileSync(path, pid, { flag: \'wx\' })`.');
-    expect(parseFindings('outside-voice', '1. **Race on the lock** (a.ts:3)\n   - the writer holds no lock\n\n   Fix: take it first. Severity: High. Blocks: no.').map(f => [f.label, f.raw]))
-      .toEqual([['High', '1. **Race on the lock** (a.ts:3) | Fix: take it first. Severity: High. Blocks: no.']]);
+    expect(found.map(f => f.label)).toEqual(['CRITICAL', 'CRITICAL', 'CRITICAL', 'CRITICAL', 'CRITICAL', 'Low']);
+    expect(found[3]!.raw).toStartWith('Fix: create the file with');
+    expect(parseFindings('outside-voice', '1. **Race on the lock** (a.ts:3)\n   - the writer holds no lock\n\n   Fix: take it first. Severity: High. Blocks: no.').map(f => f.label)).toEqual(['High']);
     expect(parseFindings('outside-voice', '1. Lock race (a.ts:3)\n\n   Fix: take it first. Blocks: yes\n2. Cache drift (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']);
     expect(parseFindings('red-team', '1. Unescaped query\n   {"severity":"critical","summary":"Low-level race in q.ts"}').map(f => [f.label, f.raw]))
       .toEqual([['critical', '{"severity":"critical","summary":"Low-level race in q.ts"}']]);
     expect(parseFindings('red-team', '1. Unescaped query\n   **Critical**\n2. Token leak in the logger').map(f => [f.label, f.raw])).toEqual([['Critical', '2. Token leak in the logger']]);
     expect(parseFindings('red-team', '1. Unescaped query\n   ## High\n2. Token leak in the logger').map(f => [f.label, f.raw])).toEqual([['High', '2. Token leak in the logger']]);
-    // Known: a Severity field line under an unlabeled item opens its own finding, so a later indented Blocks: yes line becomes a second
-    // finding (main merged them into one CRITICAL). No label is lost; the Low one is a phantom the lead folds.
-    expect(parseFindings('red-team', '1. Race in queue\n- Severity: Low\n   this one blocks. Blocks: yes').map(f => f.label)).toEqual(['Low', 'CRITICAL']);
+    expect(parseFindings('red-team', '1. Race in queue\n- Severity: Low\n   this one blocks. Blocks: yes').map(f => f.label)).toEqual(['CRITICAL']);
+    expect(parseFindings('red-team', '1. Unescaped query\n   {"severity":"low","summary":"race"}\n   this blocks the merge. Blocks: yes').map(f => [f.label, f.raw]))
+      .toEqual([['CRITICAL', '{"severity":"low","summary":"race"} | this blocks the merge. Blocks: yes']]);
   });
 
   // Value: protects=a non-review the same dispatch later answers with a review is voided (counted, VERIFY clean), and a failed outside