internal/certify/review_statement.go at 03ed0ba6f6, reviewed by claude-sonnet-5, verified by gemini-3.6-flash.
sha256Hex returns the empty string for empty input instead of the real SHA-256 digest of "", so ScriptSHA256/OutputSHA256/opinionSha256 fields for legitimately-empty content (e.g. CODE-READ findings with no script, empty stdout, empty opinion) do not match the hash a verifier computes from the raw bytes stored in the ledger, breaking the statement's core verification guarantee.
#!/bin/sh
set -e
TESTFILE="internal/certify/zz_review_statement_bug_test.go"
cleanup() { rm -f "$TESTFILE"; }
trap cleanup EXIT
cat > "$TESTFILE" <<'EOF'
package certify
import (
"crypto/sha256"
"encoding/hex"
"testing"
)
func TestSha256HexEmptyMismatch(t *testing.T) {
real := sha256.Sum256([]byte(""))
want := hex.EncodeToString(real[:])
got := sha256Hex("")
t.Logf("sha256Hex_empty=%s actual_sha256_empty=%s", got, want)
if got == want {
t.Fatalf("no mismatch: bug not present, sha256Hex correctly returned %s", got)
}
}
EOF
OUT=$(go test ./internal/certify/... -run TestSha256HexEmptyMismatch -v 2>&1)
STATUS=$?
echo "$OUT"
if [ $STATUS -ne 0 ]; then
echo "go test failed unexpectedly (bug may be absent or code broken)"
exit 1
fi
if echo "$OUT" | grep -q 'sha256Hex_empty= actual_sha256_empty=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'; then
echo "CONFIRMED: sha256Hex(\"\") returned empty string while real sha256(\"\") is e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
exit 0
fi
echo "Expected evidence pattern not found in test output"
exit 1=== RUN TestSha256HexEmptyMismatch
zz_review_statement_bug_test.go:13: sha256Hex_empty= actual_sha256_empty=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
--- PASS: TestSha256HexEmptyMismatch (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/certify 0.003s
CONFIRMED: sha256Hex("") returned empty string while real sha256("") is e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855sha256Hex returning "" for empty input is intentional so that Go's json:",omitempty" tag on ScriptSHA256 and OutputSHA256 omits those keys when no script or output exists (e.g. for CODE-READ findings). Returning a non-empty SHA-256 digest of "" would break omitempty and falsely claim a script/output was present. Furthermore, ReproductionsSHA256 recomputed during verification uses Reproductions(r) identically, producing an exact hash match between attestation generation and verification.
#!/bin/sh
set -e
TESTFILE="internal/certify/zz_verify_r1_test.go"
cleanup() { rm -f "$TESTFILE"; }
trap cleanup EXIT
cat > "$TESTFILE" <<'EOF'
package certify
import (
"encoding/json"
"testing"
"time"
"github.com/pdbethke/corralai/internal/review"
)
func TestSha256HexEmptyBehavior(t *testing.T) {
r := review.Review{
Repo: "github.com/example/repo",
Commit: "123456",
StartedAt: time.Now(),
Findings: []review.Finding{
{
ID: "R1",
Claim: "Code read finding with no script or output",
Declared: "CODE-READ",
Tier: "CODE-READ",
Script: "",
Stdout: "",
},
},
Opinion: "",
}
att := BuildReviewAttestation(r)
pred, ok := att["predicate"].(map[string]any)
if !ok {
t.Fatalf("expected predicate map")
}
claimedHash, _ := pred["reproductionsSha256"].(string)
recomputedHash := ReproductionsSHA256(r)
if claimedHash != recomputedHash {
t.Fatalf("hash mismatch: claimed %s != recomputed %s", claimedHash, recomputedHash)
}
reps := Reproductions(r)
data, err := json.Marshal(reps)
if err != nil {
t.Fatalf("marshal error: %v", err)
}
var raw []map[string]any
if err := json.Unmarshal(data, &raw); err != nil {
t.Fatalf("unmarshal error: %v", err)
}
if _, exists := raw[0]["scriptSha256"]; exists {
t.Fatalf("expected scriptSha256 to be omitted when script is empty")
}
if _, exists := raw[0]["outputSha256"]; exists {
t.Fatalf("expected outputSha256 to be omitted when stdout is empty")
}
}
EOF
go test ./internal/certify/... -run TestSha256HexEmptyBehavior -vrefuted by pdbethke — by design: an empty hash is what omitempty needs so a finding with no script carries no scriptSha256; the verifier's reproduction showed exactly that
ref 5e49d9f21ad8e4a87d9ed4a34a2fe0f9ffe3193cbcd95c98138c90e36af9b52e#R1
BuildReviewAttestation's doc comment describes an entryHash that lets the statement and its ledger entry name each other (statement first, entry second, entry carries statement's hash), but the function never accepts or emits any entryHash field in the returned predicate, so the documented two-step linkage is not implemented in this code.
We inspected BuildReviewAttestation and its doc comment. The doc comment describes an entryHash concept and its role in statement/entry linkage, but BuildReviewAttestation(r review.Review) neither accepts an entryHash parameter nor populates an entryHash field in the returned map.
confirmed by pdbethke — the doc comment describes an entryHash parameter that was designed and never added; the linkage runs the other way (the entry names the statement). comment fixed
ref 5e49d9f21ad8e4a87d9ed4a34a2fe0f9ffe3193cbcd95c98138c90e36af9b52e#R2
ReproductionsSHA256 silently discards the error from json.Marshal; if Reproductions(r) ever contained a value that fails to marshal (e.g. via a future field addition of an unsupported type), the function would return the SHA-256 of an empty byte slice rather than signaling failure, producing a valid-looking but meaningless hash in the attestation.
Reproductions(r) returns []Reproduction, which is a slice of structs containing exclusively strings, ints, pointers to ints, and inline structs of those same primitive types. In Go, json.Marshal on this concrete data type can never fail (no functions, channels, circular references, or unsupported types). Discarding the error return value is safe, and the finding relies entirely on a hypothetical future code modification that is not present.
refuted by pdbethke — json.Marshal cannot fail on a slice of structs of strings, ints and *int; the verifier's argument is correct
ref 5e49d9f21ad8e4a87d9ed4a34a2fe0f9ffe3193cbcd95c98138c90e36af9b52e#R3
Recorded so that "found nothing here" can be told apart from "never looked here".
The file's whole purpose is to produce a hash-bound attestation of a review's reproductions, so the correctness of the hashing is the entire point of the code. Finding 1 shows that the hashing helper silently breaks that guarantee for the empty-string case: it substitutes a placeholder instead of the real SHA-256 digest, so any consumer that independently hashes the raw bytes recorded in the ledger (an empty script for a CODE-READ/HYPOTHESIS finding, an empty stdout, or an empty opinion) will get a value that never matches the statement's claimed hash. That is a self-defeating bug in a file whose stated job is exactly this kind of hash equality. Finding 2 is a smaller but real gap: the doc comment on BuildReviewAttestation describes an entryHash mechanism that is supposed to let the statement and the ledger entry name each other, but no such field is ever produced by the function, so the two-step (statement-then-entry) linkage the comment promises does not exist in the code shown.
R1 is refuted with a test demonstrating that returning an empty string from sha256Hex for empty inputs is necessary for struct field omitempty tag behavior (omitting script/output hashes when no script was run) and that verifier hash recomputation matches perfectly. R2 stands as the doc comment references an entryHash parameter/field that does not exist in BuildReviewAttestation. R3 is refuted via code read because []Reproduction consists purely of primitive types that can never cause json.Marshal to fail.