fix(review): find the prior round with code, not a prose recipe

lib/craft-log-run.mjs

@@ -21,14 +21,15 @@
 //   finalize --dir <d>                    complete record; folds in that run's checkpoints
 //   recover                               unfinalized checkpoint dirs → partial records (no stdin)
 //   from-journal <transcriptDir>          rebuild a record from a workflow transcript (no stdin)
+//   prior-round --branch <b> [--project <p>]  newest prior review round for that branch (no stdin)
 //
 // Run: node lib/craft-log-run.mjs <command> [flags]
 import fs from 'node:fs'
 import os from 'node:os'
 import path from 'node:path'
 import { execFileSync } from 'node:child_process'
 import { fileURLToPath } from 'node:url'
-import { indexProjection, summarizeFindings, reviewVerdict } from './run-record.mjs'
+import { indexProjection, summarizeFindings, reviewVerdict, selectPriorRounds } from './run-record.mjs'
 
 export const DEFAULT_STORE = path.join(os.homedir(), '.craft', 'runs')
 const PARTIAL_DIR = '.partial'
@@ -37,11 +38,12 @@ const PARTIAL_DIR = '.partial'
 // Everything here used to be a shell snippet in a prompt. It is ordinary IO with one rule: a field
 // that cannot be resolved becomes null/'' and NEVER fails the run. Losing `dirty` is a blemish;
 // losing the record because git was unhappy is the failure this whole file exists to prevent.
-function git(args, cwd) {
+function git(args, cwd, { probe = false } = {}) {
   try {
-    return execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim()
+    const out = execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim()
+    return probe ? { ok: true, out } : out
   } catch {
-    return ''
+    return probe ? { ok: false, out: '' } : ''
   }
 }
 
@@ -56,6 +58,17 @@ function craftRoot() {
   return path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..')
 }
 
+// The stable identity of the repository being reviewed. A run launched from a subdirectory, through
+// a symlinked or differently-spelled path, or from a linked worktree would otherwise key its record
+// to a DIFFERENT string than the next run — and the round chain breaks exactly the way a `.` key
+// broke it. `--show-toplevel` collapses all of those onto one key. Not a git repo (or no git at
+// all) → the resolved path, which is still better than a relative one.
+export function repoKey(p = process.cwd()) {
+  const abs = path.resolve(p)
+  const top = git(['rev-parse', '--show-toplevel'], abs)
+  return top ? path.resolve(top) : abs
+}
+
 export function computedFields(project = process.cwd(), now = new Date()) {
   return {
     ts: stamp(now),
@@ -297,6 +310,122 @@ agents add toolsRun. A record with \`partial: true\` did NOT finish — never av
 `)
 }
 
+// ---- prior round (READ path) -----------------------------------------------------------------
+// The mirror image of `write`: the re-review round used to be located by handing a model a prose
+// recipe (read the index, pick the newest match, check ancestry, rebuild the path, read the JSON).
+// That is a chain of DECISIONS, and it silently returned "no prior round" whenever any link failed.
+// It is all done here, deterministically; the model only carries the bytes.
+export const PRIOR_ROUND_NONE = { found: false, round: 0, head: '', ledger: [], ledgerCount: 0, priorFindings: 0, reason: '' }
+
+// Every "no prior round" carries WHY. The defect this command exists to remove is a silent
+// chain-break, so the read path must never collapse "no store yet", "this branch has never been
+// reviewed", "the head was rebased away" and "the record file is gone" into one indistinguishable
+// {found:false} — the workflow logs this string so a broken chain is visible in the transcript.
+function noPriorRound(reason) {
+  return { ...PRIOR_ROUND_NONE, reason }
+}
+
+// The exact shape the workflow's strict LEDGER_ITEM schema accepts, with each key's natural empty.
+// Normalizing here is the point: an older persisted ledger entry missing one required key would
+// invalidate the whole structured output downstream and null out the round we just located. The
+// script owns the shape; the model only transports it.
+const LEDGER_ITEM_SHAPE = {
+  fp: '', file: '', line: 0, symbol: '', severity: '', tier: '',
+  disposition: '', source: '', ruleId: '', title: '', why: '',
+}
+
+function normalizeLedgerItem(item) {
+  const src = (item && typeof item === 'object') ? item : {}
+  const out = {}
+  for (const [k, empty] of Object.entries(LEDGER_ITEM_SHAPE)) {
+    const v = src[k]
+    if (typeof empty === 'number') {
+      const n = Number(v)
+      out[k] = Number.isFinite(n) ? Math.trunc(n) : 0
+    } else {
+      out[k] = typeof v === 'string' ? v : (v == null ? '' : String(v))
+    }
+  }
+  // `sources` is optional in the schema but carries the strict-mode escalation — keep it when it is
+  // present and well-formed, drop it otherwise rather than emitting a value the schema rejects.
+  if (Array.isArray(src.sources)) out.sources = src.sources.filter(x => typeof x === 'string')
+  return out                                          // unknown keys are dropped by construction
+}
+
+function normalizeLedger(ledger) {
+  return (Array.isArray(ledger) ? ledger : []).map(normalizeLedgerItem)
+}
+
+export function findPriorRound({ store = DEFAULT_STORE, project = process.cwd(), branch = '' } = {}) {
+  if (!branch) return noPriorRound('no-branch')
+  const indexFile = path.join(store, 'index.jsonl')
+  if (!fs.existsSync(store)) return noPriorRound('no-store')
+  if (!fs.existsSync(indexFile)) return noPriorRound('no-index')
+  const entries = readJsonl(indexFile)   // malformed lines are skipped, not thrown
+  // Rows exist under both the resolved absolute path and whatever string the caller passed. NOT under
+  // `.`: legacy rows written with project="." are not attributable to any repository, and matching
+  // them can hand this branch a round from an unrelated repo — worse than missing the round.
+  const cwd = path.resolve(project)
+  // The invariant is enforced HERE, not at the CLI: a relative candidate (`.` above all) is dropped
+  // and only its resolved absolute form is searched, so a direct library call cannot reach the rows
+  // the comment above forbids.
+  const candidates = [...new Set([project, cwd])].filter(p => path.isAbsolute(p))
+  const rows = candidates
+    .flatMap(p => selectPriorRounds(entries, { project: p, branch }))
+    .sort((a, b) => String(b.ts).localeCompare(String(a.ts)))
+  if (!rows.length) {
+    // Distinguish "never reviewed on this branch" from "reviewed, but every row is keyed to a
+    // string that names no repository" — the latter is the population this branch deliberately
+    // stops matching, and the FIRST re-review of every pre-existing branch lands here.
+    const unattributable = entries.some(e =>
+      e && e.kind === 'workflow' && e.name === 'review' && e.branch === branch &&
+      typeof e.project === 'string' && !path.isAbsolute(e.project))
+    return noPriorRound(unattributable ? 'unattributable-rows-only' : 'no-candidate-rows')
+  }
+  // A repo we cannot interrogate is not the same as a rejected candidate: without git, ancestry is
+  // unknowable and EVERY row below would be dropped for the wrong reason.
+  if (!git(['rev-parse', '--git-dir'], cwd, { probe: true }).ok) return noPriorRound('git-unavailable')
+  // Newest FIRST, and a rejection continues the search: a candidate whose head is no longer on this
+  // history (rebase/force-push) or whose detail record is unreadable must not blank a valid older
+  // round — that is the silent chain-break this command exists to remove.
+  let rejected = ''
+  for (const cand of rows) {
+    if (!cand.head) { rejected ||= 'row-without-head'; continue }
+    const mb = git(['merge-base', '--is-ancestor', String(cand.head), 'HEAD'], cwd, { probe: true })
+    if (!mb.ok) { rejected ||= 'ancestry-rejected'; continue }
+    const file = path.join(store, recordFilename({ ts: cand.ts, kind: cand.kind, name: cand.name }))
+    let rec
+    try {
+      rec = JSON.parse(fs.readFileSync(file, 'utf8'))
+    } catch {
+      rejected ||= 'detail-unreadable'
+      continue
+    }
+    // A `partial: true` record is a run that DIED — its ledger is whatever the last checkpoint
+    // happened to hold, not the round's conclusion. The store's own README says never to average
+    // one in; carrying one as the prior round would silently truncate the ledger chain. Keep
+    // walking: an older complete round is a far better base than a half-written newer one.
+    if (rec.partial) { rejected ||= 'partial-only'; continue }
+    const ledger = normalizeLedger(rec.ledger)
+    return {
+      found: true,
+      round: Number(rec.round || cand.round || 0) || 0,
+      // The head we RETURN is the head whose ancestry we just verified. Preferring `rec.head` here
+      // would hand `git diff <head>...HEAD` a value nothing checked — they agree today only because
+      // one write produces both.
+      head: String(cand.head),
+      ledger,
+      // Authoritative count, computed here. The ledger crosses an agent boundary as structured
+      // output on its way to the workflow; without a count printed alongside it, a truncated array
+      // is indistinguishable from a genuinely short round. The workflow asserts the two agree.
+      ledgerCount: ledger.length,
+      priorFindings: Number(rec.findings?.total ?? cand.findingsTotal ?? 0) || 0,
+      reason: '',
+    }
+  }
+  return noPriorRound(rejected || 'no-candidate-rows')
+}
+
 // ---- CLI -------------------------------------------------------------------------------------
 function readStdin() {
   try {
@@ -319,7 +448,12 @@ if (invokedDirectly) {
   // `project` keys the record to the repo that was REVIEWED. It defaults to cwd because that is
   // right for a live run, but from-journal is typically run from somewhere else entirely — and a
   // record filed against the wrong project silently splits that project's history in two.
-  const project = flag(argv, 'project') || process.cwd()
+  // Resolved to the REPOSITORY ROOT, not merely to an absolute path: a row keyed "." is not
+  // attributable to any repository (and the read path deliberately refuses to match those), and a
+  // row keyed to a subdirectory of the repo is invisible to a run launched from the root. Both the
+  // write path (checkpoint/finalize) and the read path (prior-round) go through this one line, so
+  // records written before and after keep matching.
+  const project = repoKey(flag(argv, 'project') || process.cwd())
   const parseStdin = () => {
     const raw = readStdin()
     if (!raw.trim()) throw new Error('no record on stdin')
@@ -351,8 +485,18 @@ if (invokedDirectly) {
       const rec = recordFromJournal(dir, { name: flag(argv, 'name') || 'review' })
       const { file } = writeRecord(rec, { store, project })
       console.log(`wrote ${file} (${rec.agentsStarted} agent(s), ${rec.agentsLost} lost, ${rec.findings.total} candidate finding(s))`)
+    } else if (cmd === 'prior-round') {
+      // Losing the prior round must DEGRADE the review, never abort it: any failure below is a
+      // clean "no prior round" on stdout with exit 0, never the FAILED line the other commands use.
+      let out = PRIOR_ROUND_NONE
+      try {
+        out = findPriorRound({ store, project, branch: flag(argv, 'branch') || '' })
+      } catch {
+        out = PRIOR_ROUND_NONE
+      }
+      console.log(JSON.stringify(out))
     } else {
-      console.error('usage: craft-log-run.mjs write|checkpoint|finalize|recover|from-journal')
+      console.error('usage: craft-log-run.mjs write|checkpoint|finalize|recover|from-journal|prior-round')
       process.exit(2)
     }
   } catch (e) {

lib/run-record.mjs

@@ -122,14 +122,17 @@ export function indexProjection(r) {
   }
 }
 
-// Pick the newest prior `review` run for this project+branch from the loaded index.jsonl entries.
-// ts strings are UTC and lexically sortable (YYYY-MM-DDTHH-MM-SSZ), so a string max is chronological.
-export function selectPriorRound(indexEntries, { project, branch }) {
-  let best = null
+// All prior `review` runs for this project+branch from the loaded index.jsonl entries, NEWEST FIRST.
+// ts strings are UTC and lexically sortable (YYYY-MM-DDTHH-MM-SSZ), so a string sort is chronological.
+// Callers walk this list: a candidate can be rejected downstream (head no longer an ancestor after a
+// rebase, unreadable detail record) and the next-newest must still be reachable.
+export function selectPriorRounds(indexEntries, { project, branch }) {
+  const hits = []
   for (const e of (Array.isArray(indexEntries) ? indexEntries : [])) {
     if (!e || e.kind !== 'workflow' || e.name !== 'review') continue
     if (e.project !== project || e.branch !== branch || !e.branch) continue
-    if (!best || String(e.ts) > String(best.ts)) best = e
+    hits.push(e)
   }
-  return best
+  return hits.sort((a, b) => String(b.ts).localeCompare(String(a.ts)))
 }
+