Skip to content

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

Merged
gurinderu merged 7 commits into
mainfrom
fix/deterministic-prior-round
Aug 31, 2026
Merged

gurinderu merged 7 commits into
mainfrom
fix/deterministic-prior-round

Conversation

@gurinderu

Copy link
Copy Markdown
Owner

What this changes

Re-review round detection — finding the previous review run for a branch — was
done by handing a haiku agent a prose recipe: read ~/.craft/runs/index.jsonl,
pick the newest matching line, shell out for ancestry, rebuild a file path from a
timestamp, read that JSON, return selected fields. Meanwhile the same file already
contained a deterministic selectPriorRound that was never called — dead code.

This is the same class of defect the header of lib/craft-log-run.mjs says was
already removed from the write path ("a model was formatting bytes it had no
business formatting"). The read path never got the same treatment.

craft-log-run.mjs grows a prior-round subcommand that owns the whole decision:
tolerant index read, selection via the existing pure selectPriorRound, ancestry
via git, detail-record load, one line of JSON out — and on any failure a clean
"no prior round" with exit 0, so losing it degrades the review instead of aborting
it. The sandbox has no filesystem access, so an agent still transports the result;
it no longer decides anything.

Why it mattered

Measured on the real run store before this change:

  • index rows exist with project: "." — a legacy bug where the reviewed repo was
    never resolved to an absolute path. The prose project=pwd match failed them
    silently.
  • index.jsonl contains two blocks of pretty-printed multi-line JSON. A strict
    reader chokes; a "best-effort" model reports "no prior round found".

Either way the re-review chain broke without a word and the round restarted from a
blank ledger.

Two review rounds on this branch found five issues, all addressed here:

  • a prior round could be taken from an unrelated repository via the "."
    candidate, and the search stopped at the first rejection instead of falling
    through to an older valid round;
  • the tests asserted around the two behaviours the change exists for rather than
    through them;
  • the output still round-tripped through a strict schema, so one older ledger entry
    missing a key lost the round anyway — the subcommand now normalises every entry
    to the schema shape;
  • path.resolve ran in the transport agent's process, against the wrong base;
  • the no-"." invariant was enforced at the CLI rather than in the library, so a
    direct call reinstated it. Now enforced where it is stated, with a test that
    fails if the guard is removed.

Fixing the first of those surfaced a false premise: records were not already
stored with an absolute project path (writeRecord kept --project verbatim and
the checkpoint/finalize call sites passed .). Dropping the "." candidate alone
would have broken re-review for the normal case, so project resolution is fixed at
those call sites too.

Scope — what this does NOT close

The primary defect in re-review is finding identity: fingerprint() hashes
model-authored text (file + symbol + ruleId + shingle(title)) and matchesPrior
gates on a 0.6 title-overlap threshold plus a symbol veto. Measured across two
rounds of one branch: 82 ledger entries each, 59 findings in files shared with the
prior round, 1 matching fingerprint, 2 recognised by matchesPrior, and
every entry still disposition: open. That is untouched here and needs its own
change.

Verification

node --test 169 pass / 0 fail; check-workflows 6/6 parse; check-skills and
check-evals clean.

Round detection handed a haiku agent a recipe: read index.jsonl, pick the
newest matching line, shell out for ancestry, rebuild a path from a
timestamp, read the JSON. The same file already had a deterministic
selectPriorRound — dead code, never called. Meanwhile the real store has
rows written with project="." and two blocks of pretty-printed JSON inside
the JSONL; the prose match failed the first silently and a strict reader
chokes on the second. Either way the re-review chain broke without a word
and the round restarted from a blank ledger.

craft-log-run.mjs grows a `prior-round` subcommand that owns the whole
decision: tolerant index read (malformed lines skipped), selection via the
existing pure selectPriorRound, ancestry via git, detail record load, one
line of JSON out — and on any failure a clean "no prior round" with exit 0,
so losing it degrades the review instead of aborting it. The model stays
only as transport, as it already is on the write path.

Removes the duplicated selectPriorRound from review.js.
…op at the first rejection

The read path matched a third candidate spelling, ".", against index rows a
legacy relative --project had written. Such a row names no repository: on a
common branch name the newest hit could be another repo's round entirely. It is
gone. Records are now filed under a resolved absolute path (the CLI resolves
--project, and the three logger call sites cd into the reviewed repo first), so
the unattributable rows simply age out.

The loop also committed to the single newest hit and gave up if its head failed
the ancestry check or its detail file would not parse — reinstating in code the
silent chain-break the previous commit removed. Candidates are now walked
newest-first and a rejection continues the search.

The ledger is normalized to the workflow's strict LEDGER_ITEM shape before it is
printed: missing required keys filled with their natural empties, unknown keys
dropped. One old entry missing one key used to invalidate the whole structured
output and null out the round we had just located. The script owns the shape.

Tests now drive the paths this change exists for: a "." row, a non-ancestor
head, a missing detail record, and the fallback to an older valid round.
…ot the CLI

findPriorRound's own comment forbids matching legacy index rows written with
project="." — they belong to no repository, and matching one hands this branch
a round from an unrelated repo. The guarantee held only because the CLI resolved
--project first; a direct library call with a relative project put '.' straight
back into the candidate set. Filter candidates to absolute paths, so the
invariant is enforced where it is stated.
The end-to-end test only covers the CLI, which resolves --project to an
absolute path before calling in. The path that actually regressed is a
direct findPriorRound call with a relative project; assert it never
matches a legacy project="." index row.
…never it

Every rejection on the read path collapsed into one {found:false}: no store,
no index, no candidate rows, a rebased-away head, an unreadable record. The
defect this command exists to remove is a silent chain-break, so it must not
add one — after the absolute-path key, the first re-review of every existing
branch restarts from a blank ledger, and that has to be visible.

- each miss carries a `reason` (no-branch, no-store, no-index,
  no-candidate-rows, unattributable-rows-only, git-unavailable,
  row-without-head, ancestry-rejected, detail-unreadable, partial-only)
- `ledgerCount` rides alongside the ledger so the workflow can tell a
  truncated transport from a genuinely short round
- a `partial: true` record — a run that DIED — is skipped and the search
  walks on to an older complete round
- the head returned is the head whose ancestry was verified, not rec.head
…d was missed

The located round still leaves the sandbox as a haiku's structured output, and
nothing cross-checked what arrived. ledgerDegraded fired only on an entirely
empty ledger, so an 82-entry round truncated to 20 in transit was
indistinguishable from a genuine 20-entry one — the payload is the whole point
of the command.

The loader now prints an authoritative ledgerCount; ledgerTruncated compares it
against the array that arrived and folds a mismatch into ledgerDegraded, which
already forces the full base...HEAD re-scan, and it is logged. A record from
before the count existed has none, so the check is skipped rather than
mis-firing. The `reason` for a missing round is logged too.
It became a one-line wrapper over selectPriorRounds with no production caller —
only tests. The defect this branch exists to fix was code written, tested and
never called; leaving a smaller instance behind is the same mistake. The tests
now exercise selectPriorRounds directly, including the newest-first ordering
the fallback walk depends on.

Also: the prior-round tests build their own git repo instead of borrowing the
ambient checkout (outside one they errored rather than failed), a partial
record is pinned as never selectable, and the CLI contract the PR body sells —
one line of JSON, exit 0, repo-root key from a subdirectory, a failure case
that still exits 0 — is exercised as a real subprocess.
@gurinderu
gurinderu merged commit 50d6fb4 into main Aug 31, 2026
1 check passed
@gurinderu
gurinderu deleted the fix/deterministic-prior-round branch September 2, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant