internal/review at 5ea687f383, reviewed by claude-code, verified by codex.
A reproduction that never ran because the harness failed (worktree gone, timeout, sh missing) is recorded exactly like a script that ran and disagreed, and OutcomeOf then reports the reviewer's claim as checked-and-did-not-hold "by execution" — charging an infrastructure failure to the reviewer in `models rank`, crediting the audited party's change as having held, and crediting any hand-waving CODE-READ refutation as correct.
set -eu
export GOPROXY=off
cat > internal/review/zz_cold_f1_test.go <<'GOEOF'
package review
import (
"context"
"errors"
"testing"
)
type coldErrRep struct{}
func (coldErrRep) Run(context.Context, string) (string, int, error) {
return "", 0, errors.New("git worktree at 5ea687f3: no such file or directory")
}
func TestColdHarnessFailureIsChargedToTheReviewerAsExecution(t *testing.T) {
r := &Review{Findings: []Finding{{ID: "R1", Claim: "the gate is skipped on the push path", Declared: TierReproduced, Tier: TierReproduced, Script: "echo evidence; exit 0"}}}
Reproduce(context.Background(), coldErrRep{}, r)
f := r.Findings[0]
t.Logf("the harness failed, nothing ran. after Reproduce: tier=%s declared=%s demoted=%q", f.Tier, f.Declared, f.Demoted)
t.Logf("ExitCode == nil is %v — review.go:51 says nil means the script NEVER RAN", f.ExitCode == nil)
o := OutcomeOf(f, nil)
t.Logf("OutcomeOf = Known=%v Held=%v By=%q <-- the record says execution decided it", o.Known, o.Held, o.By)
g := Grade(Finding{Declared: f.Declared, Tier: f.Tier, Refutation: &Refutation{Model: "v", Verdict: VerdictRefuted, Tier: TierCodeRead, Argument: "I grepped and found nothing"}}, nil)
t.Logf("Grade with a CODE-READ refutation (which verifier.go says moves nothing) = %+v", g)
t.Logf("=> models rank: reviewer checked=1 held=0; verifier calls=1 correct=1; committer's change 'held'")
if f.ExitCode != nil {
t.Fatalf("not demonstrated: a script did run")
}
if !o.Known || o.Held || o.By != "execution" {
t.Fatalf("not demonstrated: OutcomeOf = %+v", o)
}
if !g.ReviewerChecked || g.ReviewerHeld || !g.VerifierCorrect {
t.Fatalf("not demonstrated: Grade = %+v", g)
}
t.Log("DEFECT: nothing executed, yet the record grades the reviewer wrong and the verifier right 'by execution'")
}
GOEOF
go test ./internal/review/ -run TestColdHarnessFailureIsChargedToTheReviewerAsExecution -v -count=1=== RUN TestColdHarnessFailureIsChargedToTheReviewerAsExecution
zz_cold_f1_test.go:19: the harness failed, nothing ran. after Reproduce: tier=CODE-READ declared=REPRODUCED demoted="the script could not run: git worktree at 5ea687f3: no such file or directory"
zz_cold_f1_test.go:20: ExitCode == nil is true — review.go:51 says nil means the script NEVER RAN
zz_cold_f1_test.go:22: OutcomeOf = Known=true Held=false By="execution" <-- the record says execution decided it
zz_cold_f1_test.go:24: Grade with a CODE-READ refutation (which verifier.go says moves nothing) = {ReviewerChecked:true ReviewerHeld:false VerifierCalled:true VerifierCorrect:true}
zz_cold_f1_test.go:25: => models rank: reviewer checked=1 held=0; verifier calls=1 correct=1; committer's change 'held'
zz_cold_f1_test.go:35: DEFECT: nothing executed, yet the record grades the reviewer wrong and the verifier right 'by execution'
--- PASS: TestColdHarnessFailureIsChargedToTheReviewerAsExecution (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/review 0.002sI traced Reproduce through OutcomeOf and Grade and could not refute the named case. Reproduce leaves ExitCode nil when Reproducer.Run returns an error and demotes Tier, while OutcomeOf uses only Declared and Tier, consequently returning Known=true, Held=false, By="execution". Grade then scores both seats from that outcome without consulting ExitCode.
confirmed by pdbethke — real, reproduced: a reproduction the harness could not run is recorded as a script that ran and disagreed, and charged to the reviewer
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R1
Verify runs a verifier's REPRODUCED refutation script against findings of ANY tier and records its exit 0, but OutcomeOf only ever consults findings declared REPRODUCED — so a refutation that executed and demonstrated a CODE-READ claim false is paid for, run, recorded, and then discarded as "no outcome", grading nobody and leaving the claim listed as undecided in `corral brief`; the comment at outcome.go:37-39 that justifies this ("only a claim that was declared REPRODUCED was ever put to the tree") is false.
set -eu
export GOPROXY=off
cat > internal/review/zz_cold_f2_test.go <<'GOEOF'
package review
import (
"context"
"testing"
)
type coldOKRep struct{ ran int }
func (r *coldOKRep) Run(_ context.Context, script string) (string, int, error) {
r.ran++
return "the check the finding calls missing fires at internal/x.go:12\n", 0, nil
}
func TestColdReproducedRefutationOfACodeReadFindingIsExecutedThenDiscarded(t *testing.T) {
r := &Review{Findings: []Finding{{ID: "R1", Claim: "the check is missing on the push path", Declared: TierCodeRead, Tier: TierCodeRead}}}
rep := &coldOKRep{}
refs := map[string]Refutation{"R1": {Model: "verifier-m", Verdict: VerdictRefuted, Declared: TierReproduced, Tier: TierReproduced, Argument: "here is the check, firing", Script: "go test ./... -run TheCheck"}}
Verify(context.Background(), rep, r, "verifier-m", "the review is wrong", refs)
f := r.Findings[0]
x := f.Refutation
t.Logf("Verify ran %d script(s) against a finding declared %s — outcome.go:37 says only REPRODUCED claims are put to the tree", rep.ran, f.Declared)
t.Logf("refutation recorded: tier=%s exit=%v stdout=%q", x.Tier, *x.ExitCode, x.Stdout)
o := OutcomeOf(f, nil)
g := Grade(f, nil)
t.Logf("OutcomeOf = %+v <-- Known=false: the execution that just happened is not an outcome", o)
t.Logf("Grade = %+v <-- the verifier that PROVED the claim false is graded on nothing", g)
t.Logf("=> corral brief counts this claim Undecided; models rank gets no observation for either seat")
if rep.ran != 1 || x.Tier != TierReproduced || x.ExitCode == nil || *x.ExitCode != 0 {
t.Fatalf("not demonstrated: the refutation did not execute cleanly: ran=%d %+v", rep.ran, x)
}
if o.Known || (g != Graded{}) {
t.Fatalf("not demonstrated: the execution was counted: %+v / %+v", o, g)
}
t.Log("DEFECT: a reproduction ran and exited 0, and the record treats the finding as never put to the tree")
}
GOEOF
go test ./internal/review/ -run TestColdReproducedRefutationOfACodeReadFindingIsExecutedThenDiscarded -v -count=1=== RUN TestColdReproducedRefutationOfACodeReadFindingIsExecutedThenDiscarded
zz_cold_f2_test.go:22: Verify ran 1 script(s) against a finding declared CODE-READ — outcome.go:37 says only REPRODUCED claims are put to the tree
zz_cold_f2_test.go:23: refutation recorded: tier=REPRODUCED exit=0 stdout="the check the finding calls missing fires at internal/x.go:12\n"
zz_cold_f2_test.go:26: OutcomeOf = {Known:false Held:false By:} <-- Known=false: the execution that just happened is not an outcome
zz_cold_f2_test.go:27: Grade = {ReviewerChecked:false ReviewerHeld:false VerifierCalled:false VerifierCorrect:false} <-- the verifier that PROVED the claim false is graded on nothing
zz_cold_f2_test.go:28: => corral brief counts this claim Undecided; models rank gets no observation for either seat
zz_cold_f2_test.go:35: DEFECT: a reproduction ran and exited 0, and the record treats the finding as never put to the tree
--- PASS: TestColdReproducedRefutationOfACodeReadFindingIsExecutedThenDiscarded (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/review 0.002sI checked both Verify and OutcomeOf. Verify executes every REFUTED refutation declared REPRODUCED regardless of the finding's declared tier, but it only demotes the finding when the finding currently has TierReproduced. OutcomeOf returns an unknown outcome for findings not originally declared REPRODUCED, so the successful refutation execution in the named CODE-READ case is indeed excluded from grading.
confirmed by pdbethke — real, reproduced: a verifier's reproduced refutation of a CODE-READ claim is run and recorded and then never consulted by OutcomeOf
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R2
extractJSON returns the first *parseable* {...} in the reply rather than the review object, and Parse never checks that the object is a review — so any valid JSON in the reviewer's prose before the payload makes Parse succeed with zero findings, zero sound items and an empty opinion, and the run records (and ledgers) a review that found nothing when the reviewer found things.
set -eu
export GOPROXY=off
cat > internal/review/zz_cold_f3_test.go <<'GOEOF'
package review
import "testing"
func TestColdParseTakesTheFirstValidObjectNotTheReview(t *testing.T) {
prose := "Before writing this I looked for ledger rows shaped like {\"tier\":\"REPRODUCED\"} to see what had held.\n\n"
payload := `{"opinion":"the router trusts its callers","findings":[{"claim":"the gate is skipped on the push path","tier":"REPRODUCED","file":"x.go","line":3,"severity":"high","script":"exit 0"},{"claim":"the cache key is blind to the model","tier":"CODE-READ","file":"y.go","line":9,"severity":"medium"}],"sound":["the parser","the walker","the tier normaliser"]}`
reply := prose + payload
opinion, findings, sound, err := Parse(reply)
t.Logf("the reply carries: 1 opinion, 2 findings (one REPRODUCED, high), 3 sound items")
t.Logf("extractJSON returned: %s", extractJSON(reply))
t.Logf("Parse returned: err=%v opinion=%q findings=%d sound=%d", err, opinion, len(findings), len(sound))
t.Logf("what would be recorded, coverage note: %q", CoverageNote(Review{Findings: findings, Sound: sound}))
if err != nil {
t.Fatalf("not demonstrated: Parse errored (a loud failure, not a silent empty review): %v", err)
}
if len(findings) != 0 || len(sound) != 0 || opinion != "" {
t.Fatalf("not demonstrated: Parse found the payload: %d findings", len(findings))
}
t.Log("DEFECT: a high-severity REPRODUCED finding is silently dropped and the review is recorded as a blanket approval")
}
GOEOF
go test ./internal/review/ -run TestColdParseTakesTheFirstValidObjectNotTheReview -v -count=1=== RUN TestColdParseTakesTheFirstValidObjectNotTheReview
zz_cold_f3_test.go:10: the reply carries: 1 opinion, 2 findings (one REPRODUCED, high), 3 sound items
zz_cold_f3_test.go:11: extractJSON returned: {"tier":"REPRODUCED"}
zz_cold_f3_test.go:12: Parse returned: err=<nil> opinion="" findings=0 sound=0
zz_cold_f3_test.go:13: what would be recorded, coverage note: "unknown: no findings and 0 item(s) checked and found sound (fewer than 3) — a blanket approval, not a review; nothing here is evidence the scope is sound"
zz_cold_f3_test.go:20: DEFECT: a high-severity REPRODUCED finding is silently dropped and the review is recorded as a blanket approval
--- PASS: TestColdParseTakesTheFirstValidObjectNotTheReview (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/review 0.002sI traced extractJSON and Parse on the exact input shape. Outside a json fence, extractJSON returns the first balanced substring that json.Valid accepts. Parse unmarshals into a struct whose fields are all optional, so {"tier":"REPRODUCED"} unmarshals successfully as an empty review and prevents consideration of the later actual payload.
confirmed by pdbethke — real, reproduced: Parse accepts any parseable object in the prose as the review; a zero-findings review results from a stray JSON literal
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R3
LoadScope never checks that the scope stays inside the repository, so a scope containing ".." reads files outside the checkout and Brief puts their contents into the turn sent to the model provider (and their paths into the ledger entry's files_shown).
set -eu
export GOPROXY=off
cat > internal/review/zz_cold_f4_test.go <<'GOEOF'
package review
import (
"os"
"path/filepath"
"strings"
"testing"
)
func TestColdLoadScopeReadsOutsideTheRepository(t *testing.T) {
dir := t.TempDir()
root := filepath.Join(dir, "repo")
if err := os.MkdirAll(filepath.Join(root, "internal"), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(root, "internal", "a.go"), []byte("package a\n"), 0o600); err != nil {
t.Fatal(err)
}
secret := filepath.Join(dir, "id_ed25519")
if err := os.WriteFile(secret, []byte("-----BEGIN PRIVATE KEY-----\nNOT-IN-THE-REPOSITORY\n"), 0o600); err != nil {
t.Fatal(err)
}
sc, err := LoadScope(root, "../id_ed25519", 200000)
if err != nil {
t.Fatalf("not demonstrated: LoadScope refused the escape: %v", err)
}
t.Logf("repository root: %s", root)
t.Logf("the file is OUTSIDE it: %s", secret)
t.Logf("LoadScope Files = %v (these go on the entry as files_shown)", sc.Files)
brief := Brief("repo", "5ea687f3", "../id_ed25519", sc)
t.Logf("the turn sent to the model provider:\n%s", brief)
if !strings.Contains(brief, "NOT-IN-THE-REPOSITORY") {
t.Fatalf("not demonstrated: the content did not reach the brief")
}
t.Log("DEFECT: a scope outside the repository is read and shipped to the reviewer seat")
}
GOEOF
go test ./internal/review/ -run TestColdLoadScopeReadsOutsideTheRepository -v -count=1=== RUN TestColdLoadScopeReadsOutsideTheRepository
zz_cold_f4_test.go:27: repository root: /tmp/TestColdLoadScopeReadsOutsideTheRepository294733575/001/repo
zz_cold_f4_test.go:28: the file is OUTSIDE it: /tmp/TestColdLoadScopeReadsOutsideTheRepository294733575/001/id_ed25519
zz_cold_f4_test.go:29: LoadScope Files = [../id_ed25519] (these go on the entry as files_shown)
zz_cold_f4_test.go:31: the turn sent to the model provider:
Repository: repo
Commit: 5ea687f3
Scope: ../id_ed25519
You are shown 1 file(s), 50 bytes.
===== ../id_ed25519 =====
-----BEGIN PRIVATE KEY-----
NOT-IN-THE-REPOSITORY
zz_cold_f4_test.go:35: DEFECT: a scope outside the repository is read and shipped to the reviewer seat
--- PASS: TestColdLoadScopeReadsOutsideTheRepository (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/review 0.003sI checked LoadScope and its CLI caller. filepath.Clean preserves a leading .., filepath.Join(root, scope) resolves it outside root, and neither LoadScope nor cmd/corral/review.go performs a containment check. For a non-agentic reviewer, Brief receives the resulting Scope contents and includes them verbatim in the provider turn, exactly as claimed.
confirmed by pdbethke — real, reproduced: LoadScope does not confine the scope to the checkout; a scope with .. reads outside it and the bytes go to the provider
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R4
Plan attributes every finding of a covering review to every scope it covers without looking at the finding's File, so a parent review's held finding about internal/other makes internal/review report Held>0 and the reason "changed since its review, with findings that held — a fix batch nobody has re-attacked" for a scope no finding ever named.
I followed Plan's aggregation. Once covers(r.Scope, scope) succeeds, every r.Findings element is passed to OutcomeOf and counted in that scope's Held, Fell, or Open totals. Finding.File is never consulted, so a finding about a sibling beneath a covering parent review can produce the stated Held count and fix-batch reason for the candidate scope.
confirmed by pdbethke — real, read: Plan counts every finding of a covering review toward every scope it covers, ignoring the finding's file
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R5
CoverageNote's blanket-approval rule is defeated by any finding at all: a review with one HYPOTHESIS finding and an empty sound list gets no coverage note, so "nothing here is evidence the scope is sound" is withheld from exactly the review that most needs it — one that executed nothing and listed nothing it checked.
The claim is broader than the documented rule. internal/review/review.go:121-128 explicitly defines a blanket approval as a review with no findings and fewer than MinSound sound items. A review containing a HYPOTHESIS finding is not a blanket approval: it records a concrete examined claim, remains visibly HYPOTHESIS through Counts, and receives no known outcome from OutcomeOf. Thus the rule is not "defeated"; the proposed treatment of thin reviews containing findings would be a different coverage policy.
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R6
ParseRefutations keys refutations by the verifier's own id string with no check against the review's finding ids, so a verifier that answers "1" or "r1" instead of "R1", or answers the same id twice, has its verdicts silently dropped or overwritten — and the caller's only guard (cmd/corral/review.go:263) fires solely when the map is completely empty, so a partial mismatch is recorded as a verified review with missing verdicts.
I inspected ParseRefutations, Verify, and the caller guard. ParseRefutations trims but does not canonicalize or validate IDs and assigns directly into a map, so case mismatches remain unmatched and duplicate IDs overwrite earlier entries. Verify silently skips absent exact keys, while cmd/corral/review.go only emits its no-verdict warning when len(refs)==0, leaving partial mismatches unreported.
confirmed by pdbethke — real, read: refutation ids are neither canonicalised nor checked against the review's ids; a verifier answering r1 or 1 is silently dropped
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R7
tail() cuts on a byte boundary, so a reproduction whose output exceeds 4000 bytes and ends in multi-byte text is recorded with a truncated rune — invalid UTF-8 in the Stdout that the entry and the --attest statement carry as the run's evidence.
The in-memory truncation issue is real, but the claimed external representation is false. tail at internal/review/review.go:271-275 can split UTF-8. The ledger, however, serializes Review through Go's encoding/json, which replaces invalid UTF-8 in string values with the Unicode replacement rune and therefore emits valid UTF-8 JSON. More decisively, internal/certify/review_statement.go's Reproduction has no stdout field at all: Reproductions stores only OutputSHA256, and BuildReviewAttestation carries that reproduction structure. The --attest statement therefore cannot carry invalid Stdout as claimed.
ref 61dc210a39fd77be5806ba1c4e079686cdc4de11a99d18e616bfb3801605ef1d#R8
Recorded so that "found nothing here" can be told apart from "never looked here".
This package is the part of corral that decides who was right, so its arithmetic is the record's arithmetic — and three of the four defects below are places where the record says something it cannot know. The load-bearing one is F1: `Reproduce` demotes a finding when the *harness* failed ("the script could not run") exactly as it demotes a script that ran and disagreed, and `OutcomeOf` then reports Known/Held=false with By="execution" — a sentence that is false, because nothing executed. The struct itself documents the discriminator (`ExitCode` nil "when it never ran", review.go:51) and `OutcomeOf` never looks at it. Downstream that is not cosmetic: `models rank` counts it as a reviewer claim checked-and-failed, `committerRankEvidence` credits the audited party with a change that "held", `corral brief` files it under DidNotHold, and a verifier that wrote one hand-waving REFUTED line is graded correct for it (Grade, outcome.go:69). F2 is the mirror image and contradicts a comment in the same file: outcome.go:37-39 asserts "only a claim that was declared REPRODUCED was ever put to the tree", but `Verify` gates on nothing about the finding — it runs a verifier's REPRODUCED refutation script against CODE-READ and HYPOTHESIS findings too, records the exit 0, and then `OutcomeOf` throws that execution away as unknown. A real measurement computed and discarded, and the seat that did the hardest work in the loop is graded on none of it. F3 is a re-run of the bug review.go:322-325 congratulates itself on fixing: `extractJSON` still takes the FIRST match, only now the first *parseable* one, with no check that the object is a review — so a reviewer that writes any valid JSON in its prose is recorded as having found nothing and listed nothing, silently, with no error, and the coverage note then calls the reviewer's own review a blanket approval. F4 is smaller and operator-triggered but it is the one that leaves the machine: `LoadScope` never checks that the scope stays inside the repository, so `--scope ../id_ed25519` reads a file outside the checkout and `Brief` puts its contents in the turn sent to the model provider. The tests in this package are unusually good at pinning the demote-only rule (they nail every branch of `Reproduce` and `Verify`), which is exactly why the defects are all one layer out from where the tests look: at the seam between what execution did and what the record says execution did.
R1–R5 and R7 stand: the supplied reproductions and the inspected control flow support the stated record-integrity failures. R6 is refuted because it misstates the deliberately narrow blanket-approval rule and treats a recorded finding as if it were no review content. R8 is refuted as broader than the implementation: tail can create an invalid in-memory string, but the ledger’s JSON encoder sanitizes invalid UTF-8, while the attestation carries only a SHA-256 digest of stdout, not stdout itself.