cmd/corral/ledger.go at 441da802c8, reviewed by claude-code, verified by codex.
`corral ledger push` performs no chain verification, so a ledger directory with an edited placed entry — one that `corral ledger append` and `corral ledger checkpoint` both refuse, and that `corral ledger verify` reports as broken — is pushed to the warehouse as the record, tampered bytes and all.
#!/bin/sh
set -e
export GOFLAGS=-mod=mod
cat > internal/auditpush/zz_coldreview_pushnoverify_test.go <<'EOF'
package auditpush
import (
"bytes"
"compress/gzip"
"os"
"path/filepath"
"testing"
)
// The push door does not verify the chain the other doors refuse.
func TestColdReviewLedgerPushDoesNotVerifyTheChain(t *testing.T) {
dir := t.TempDir()
for _, c := range []string{"c0000000", "c1111111"} {
if _, err := AppendLedgerEntry(dir, LedgerEntry{Bundle: Bundle{
Scan: ScanRow{Repo: "acme/one", Commit: c, ScanID: 1},
Files: []Row{{Repo: "acme/one", ScanID: 1, Path: "a.py"}},
}}, nil); err != nil {
t.Fatalf("fixture append %s: %v", c, err)
}
}
names, err := ledgerFileNames(dir)
if err != nil || len(names) != 2 {
t.Fatalf("fixture: ledgerFileNames = %v, %v", names, err)
}
// Edit a PLACED entry's bytes: the repo the record names.
p := filepath.Join(dir, ScansSubdir, names[0])
raw, err := readMaybeGzip(p)
if err != nil {
t.Fatal(err)
}
tampered := bytes.ReplaceAll(raw, []byte("acme/one"), []byte("acme/OWNED"))
if bytes.Equal(tampered, raw) {
t.Fatal("fixture: nothing was replaced")
}
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
if _, err := zw.Write(tampered); err != nil {
t.Fatal(err)
}
if err := zw.Close(); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(p, buf.Bytes(), 0o644); err != nil {
t.Fatal(err)
}
checks, err := VerifyLedgerDir(dir, nil)
if err != nil {
t.Fatal(err)
}
broken := 0
for _, c := range checks {
t.Logf("VERIFY %s problem=%q", c.File, c.Problem)
if c.Problem != "" {
broken++
}
}
if broken == 0 {
t.Fatal("fixture did not break the chain; nothing to demonstrate")
}
// The two writing doors refuse this directory.
if _, aerr := AppendLedgerEntry(dir, LedgerEntry{Bundle: Bundle{Scan: ScanRow{Repo: "acme/one", Commit: "c2222222", ScanID: 1}}}, nil); aerr == nil {
t.Fatal("precondition lost: append accepted a broken chain")
} else {
t.Logf("APPEND REFUSES: %v", aerr)
}
if _, _, cerr := WriteCheckpoint(dir, nil); cerr == nil {
t.Fatal("precondition lost: checkpoint accepted a broken chain")
} else {
t.Logf("CHECKPOINT REFUSES: %v", cerr)
}
// The push door (cmd/corral/ledger.go:241 calls exactly this) does not look.
target := filepath.Join(t.TempDir(), "warehouse.duckdb")
plan, perr := PushLedgerDir(dir, target, false, true)
if perr != nil {
t.Fatalf("DEFECT NOT SHOWN: push refused the broken chain: %v", perr)
}
t.Logf("PUSH ACCEPTS: err=%v would push scans=%d files=%d (skipped=%d retracted=%d)", perr, plan.Scans, plan.Files, plan.Skipped, plan.Retracted)
if plan.Scans != 2 {
t.Fatalf("expected both scan entries planned for push, got %d", plan.Scans)
}
// Best effort: the real write, and what the warehouse then says the repo was.
if _, werr := PushLedgerDir(dir, target, false, false); werr != nil {
t.Logf("(real push errored; the plan above is the demonstration): %v", werr)
return
}
db, derr := openWarehouseReadOnly(target)
if derr != nil {
t.Logf("(reopen: %v)", derr)
return
}
defer db.Close()
rows, qerr := db.Query(`SELECT repo, COALESCE(entry_hash, '') FROM corral_scans ORDER BY repo`)
if qerr != nil {
t.Logf("(query: %v)", qerr)
return
}
defer rows.Close()
for rows.Next() {
var repo, h string
if serr := rows.Scan(&repo, &h); serr == nil {
t.Logf("WAREHOUSE ROW: repo=%q entry_hash=%.12s", repo, h)
}
}
}
EOF
go test ./internal/auditpush -run TestColdReviewLedgerPushDoesNotVerifyTheChain -count=1 -v -timeout 55s=== RUN TestColdReviewLedgerPushDoesNotVerifyTheChain
zz_coldreview_pushnoverify_test.go:55: VERIFY 20260908T191629Z-c0000000-e390dbabed8d.json.gz problem="entry bytes do not match its hash — edited after it was written"
zz_coldreview_pushnoverify_test.go:55: VERIFY 20260908T191629Z-c1111111-d13466acda83.json.gz problem=""
zz_coldreview_pushnoverify_test.go:68: APPEND REFUSES: auditpush: /tmp/TestColdReviewLedgerPushDoesNotVerifyTheChain3206655042/001 does not verify (20260908T191629Z-c0000000-e390dbabed8d.json.gz: entry bytes do not match its hash — edited after it was written) — refusing to append, which would sign this chain as the new entry's history
zz_coldreview_pushnoverify_test.go:73: CHECKPOINT REFUSES: auditpush: /tmp/TestColdReviewLedgerPushDoesNotVerifyTheChain3206655042/001 does not verify (20260908T191629Z-c0000000-e390dbabed8d.json.gz: entry bytes do not match its hash — edited after it was written) — refusing to prune, because a checkpoint over a broken chain deletes the evidence and leaves a genesis that verifies clean
zz_coldreview_pushnoverify_test.go:82: PUSH ACCEPTS: err=<nil> would push scans=2 files=2 (skipped=0 retracted=0)
zz_coldreview_pushnoverify_test.go:107: WAREHOUSE ROW: repo="acme/OWNED" entry_hash=5db57bd2d512
zz_coldreview_pushnoverify_test.go:107: WAREHOUSE ROW: repo="acme/one" entry_hash=d274443aa6f2
--- PASS: TestColdReviewLedgerPushDoesNotVerifyTheChain (0.08s)
PASS
ok github.com/pdbethke/corralai/internal/auditpush 0.086sI traced runLedgerPush into PushLedgerDir. PushLedgerDir calls ReadLedgerDir and then plans or inserts entries without calling VerifyLedgerDir or otherwise validating each stored hash against its bytes. The supplied reproduction also exercises the exact tampered input claimed and demonstrates that append and checkpoint reject it while push accepts it.
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R1
Retracting a scan the warehouse has ALREADY received is a silent no-op: PushLedgerDir tests `have[e.Hash]` (already pushed) before it tests liveness and never deletes or flags the existing row, so `corral ledger push` reports it under "already there, skipped" and prints no "retracted scan(s) left out" line — contradicting this file's own help, which says a retracted scan "stops being the record — the view … skip it".
I traced the push loop and warehouse hash collection. For a previously pushed entry, have[e.Hash] is true, so PushLedgerDir increments Skipped and continues before consulting live[e.Hash]. Retraction and checkpoint entries themselves are classified NotPushable, and there is no warehouse deletion or retraction-marker write, so a later retraction cannot remove the existing scan row.
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R2
--expect-head, declared here as "the hash this chain's newest entry must have" and described as the only anchor that can catch end-truncation, is compared with strings.HasPrefix and has no minimum length or format check, so `--expect-head a` passes on roughly one chain in sixteen and a truncated or mistyped anchor silently reports "nothing was removed from the end".
I inspected runVerifyLedger. After trimming whitespace, any nonempty expectHead is accepted as an anchor and checked with strings.HasPrefix(got.Hash, head). There is no full-hash, hexadecimal, or minimum-length validation, so a one-character prefix can produce the claimed success message.
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R3
runLedger passes the ENTIRE argv to wantsHelp, which matches "-h", "--help" or "help" in any position, so `corral ledger retract <dir> <hash> --reason help` prints usage and exits 0 without writing a retraction — a destructive-intent command reporting success for work it did not do; runScans, runReview and runCriticScore all guard against exactly this (criticscore.go:153-157 names it as a hazard) by passing args[:1] or answering help first.
I followed the exact argv named in the finding. runLedger calls wantsHelp(args) before verb dispatch, and wantsHelp returns true when any element is exactly "help". Therefore the value of --reason in `ledger retract <dir> <hash> --reason help` causes top-level usage and return code 0 before runLedgerRetract can write anything. The comparison with runScans and runReview is also accurate because they restrict the help check to args[:1].
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R4
The "never re-linked in place" guard compares filepath.Abs paths, which clean but do not resolve symlinks, so `corral ledger append <dir>/scans-symlink/entry.json.gz <dir>` (or any symlinked route to a placed entry) escapes the check and re-links a placed entry into its own directory.
I examined both the guard and AppendLedgerEntry. filepath.Abs and filepath.Rel perform lexical normalization but do not resolve symlinks. A source path lexically outside `<dir>/scans` but resolving to an entry there therefore bypasses the guard; ReadLedgerEntry reads the placed entry through that alias and AppendLedgerEntry can place a newly linked copy in the same ledger.
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R5
The retract success line reads "retracted <hash> in <name>" where `name` is the new entry's FILE NAME, not the directory the sentence implies, and it echoes the operator's unresolved prefix rather than the hash WriteRetraction actually resolved and recorded — so the confirmation does not show what was retracted.
The claim overstates the target-identification problem. WriteRetraction resolves the supplied target to exactly one full entry hash and rejects unknown, ambiguous, and shorter-than-12-character prefixes. On success, runLedgerRetract prints `%.12s` of the supplied target; whether the operator supplied the full hash or a longer prefix, those 12 characters are the same unique prefix that WriteRetraction proved identifies the recorded target. Thus the confirmation does show what was retracted, rather than echoing an unresolved target. The separate wording complaint remains narrower and cosmetic: the value after "in" is indeed the newly written retraction entry's filename, not the directory.
ref 4401a6edc6066fbe0609f7df115b73663ee51f67c6a1614e24d5bf1a9acce890#R6
Recorded so that "found nothing here" can be told apart from "never looked here".
This file is the CLI surface of the thing the project calls THE RECORD, and its five verbs do not enforce the same rule. `append` and `checkpoint` both call VerifyLedgerDir and refuse a chain that does not verify — with long comments explaining that pruning or extending a broken chain launders it. `push`, which is the verb that carries the record OUT of the directory and into the warehouse everyone else reads, looks at nothing (finding 1, reproduced: the same directory that `append` refuses and `checkpoint` refuses is planned for push without a murmur, and the tampered repo string rides along). That is the exact shape the repo's own comments say it keeps getting caught by — a rule at one door and not the other — and the door left open is the outward-facing one. Finding 2 is the same subsystem from the other end: a retraction only removes a scan from a warehouse that has not already received it, because the already-there check runs before the retracted check and nothing ever deletes; so `corral ledger retract` after a push leaves the row standing and the verb prints "0 retracted scan(s) left out" while doing so — which directly contradicts this file's own help text at lines 63-65. Finding 3 weakens the one control the file advertises as the anchor against end-truncation: --expect-head is a prefix compare, so `--expect-head a` passes on a sixteenth of all chains, and the flag's own help calls it "the hash this chain's newest entry must have". Finding 4 is small but is a known hazard elsewhere in this same binary that this file walked into: runLedger passes the WHOLE argv to wantsHelp, where runScans, runReview and runCriticScore deliberately do not. Findings 5 and 6 are cosmetic-to-minor. I did not find a defect in flagsFirst, in the relative-directory fix on the in-place append guard, or in the verb dispatch itself.
R1–R5 stand after tracing the relevant control flow and stated inputs. R1 is directly supported by PushLedgerDir reading entries without VerifyLedgerDir. R2's already-present check precedes liveness and no deletion path exists. R3 uses an unrestricted prefix comparison. R4 scans the complete ledger argv for the literal word "help". R5 compares lexical absolute paths without resolving symlinks. R6 is narrower than stated: although the message awkwardly uses the new entry filename after "in", the displayed target is not unresolved or ambiguous because WriteRetraction accepts prefixes only when they are at least 12 characters and uniquely resolve to one entry, and the confirmation prints exactly that unique 12-character identifier.