Skip to content

gate + transparency: ten cold-review findings, and the second door found while fixing one - #330

Merged
pdbethke merged 1 commit into
mainfrom
fix/transparency-cold-review
Sep 12, 2026
Merged

pdbethke merged 1 commit into
mainfrom
fix/transparency-cold-review

Conversation

@pdbethke

Copy link
Copy Markdown
Owner

Two cold-review rounds on 2026-09-12, both aimed at scopes the ledger had never seen, picked by claim surface rather than the planner's file-count heuristic. Every finding survived a different model's attempt to refute it.

internal/transparency — reviewer codex, verifier claude-code

R1 (reproduced, medium) — VerifyInclusion panicked instead of rejecting. {} unmarshals into a zero InclusionProof, and tle.GenerateTransparencyLogEntry dereferences the nil RootHash. No caller recovers — the only recover() in the tree is in internal/mission, and certverify.go:207 calls straight through. So a corrupt rekor field in a record took certify verify down rather than failing its rekor check. A crash is not fail-closed.

parseInclusionProof now requires the swagger model's own required fields — derived, not an enumeration of the three that happen to panic today.

R2 (code-read, medium) — the payload hash does not bind the envelope. Step 3 compared only the DSSE payload hash while the comment above it claimed the check confirms the entry "wraps THIS envelope." It did not: a replacement envelope signed by a different key over the same payload reused the original inclusion proof. Step 4 now compares the logged signatures.

Signature bytes and deliberately not envelopeHash: a hash over envelope JSON is byte-identity, so any re-serialization would refuse a perfectly good record. For an entry kind whose body shape we can't read, step 4 is skipped and the returned reason says so — weaker, never silently.

internal/gate — reviewer claude-code, verifier antigravity, 8 findings

False greens — the gate reporting a result for a check it did not run:

R2 (high) ; inside cmd= truncated the entry before cmd= was isolated, so "go vet ./... ; go test ./..." was accepted as the weaker "go vet ./...". The file guarded the comma and left the semicolon open.
R8 a policy field after cmd= was swallowed into the command, leaving Base nil — a wider policy than written, silently.
R6 an empty CheckCmd reached the jail as sh -c "", exited 0, posted success. The rule sat at the door that parses a policy, not the one that acts on it.
R7 a huge timeout= overflowed to a negative duration, which the sandbox turns into its 60s default — the exact outcome DefaultGateTimeout's own comment says blocks merges.

R2 is refused, not repaired: re-joining the fragments would weld a forgotten repo= onto the previous command. cmd parsing fails loudly, as its doc always claimed.

Undelivered verdicts: R1 (a failed Save was logged, success posted anyway, and with no dedupe row the poller re-certified that head forever), R3 (dedupe keyed on (repo, head_sha) alone, so a second policy never ran and never posted its context), R4/R5 (the row was written before the status post, so a failed post was never retried and a shutdown-cancelled run stood as a permanent refusal).

R4/R5 are fixed by recording whether the verdict reached the forge — not by guessing which errors are transient. Needed a schema migration; DuckDB refuses ADD COLUMN with constraints at parse time, so the columns are added nullable, backfilled, and every read COALESCEs.

The second door

Grepping every Save caller while fixing R3/R4 turned up internal/brain/controlgate.go saving a row with no context and no delivery mark. Left alone, my own fix would have made the control gate re-run its jail every tick, forever. One rule, two doors — the same shape as the last three rounds.

Two of my own mistakes, kept on the record

My first negative-control harness was vacuous. It counted --- FAIL lines, so a build failure produced zero and read exactly like a pass — which is how R2, the high-severity finding, appeared verified when nothing had run. Redone properly: all ten fail without their fix, and R2's control reproduces the truncation verbatim.

A test fixture of mine omitted the checkpoint every real Rekor proof carries (verified against corral's own entry, index 2759598612, where it's 220 bytes), which made a legitimate proof look refused. The control fired; the fixture was wrong, not the guard. The note stays in the test, because a control that fires is only useful if you then work out which side is broken.

Full suite green, go vet clean, scripts/check-security.sh OK, gofmt clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V

…door I found while fixing one

Two review rounds on 2026-09-12, both on scopes the ledger had never seen.
Every finding survived a different model's attempt to refute it.

internal/transparency — reviewer codex, verifier claude-code:

  R1 (reproduced) VerifyInclusion PANICKED on a malformed proof. `{}`
  unmarshals into a zero InclusionProof and tle.GenerateTransparencyLogEntry
  dereferences nil RootHash; no caller recovers, so a corrupt `rekor` field in
  a record took `certify verify` down instead of failing its rekor check. Now
  parseInclusionProof requires the swagger model's OWN required fields —
  derived, not an enumeration of the three that happen to panic today.

  R2 (code-read) Step 3 compared only the DSSE payload hash while the comment
  above it claimed the entry "wraps THIS envelope". A replacement envelope
  signed by a DIFFERENT key over the same payload reused the original
  inclusion proof. Step 4 now compares the logged signatures. Signature bytes,
  not envelopeHash: a hash over envelope JSON is byte-identity and any
  re-serialization would refuse a good record.

internal/gate — reviewer claude-code, verifier antigravity, 8 findings:

  FALSE GREENS — the gate reporting a result for a check it did not run:
  R2 (high) `;` inside cmd= truncated the entry BEFORE cmd= was isolated, so
    "go vet ./... ; go test ./..." was accepted as the weaker "go vet ./...".
    The file guarded the comma and left the semicolon open. Refused now, not
    repaired: re-joining would weld a forgotten repo= onto a command.
  R8 a policy field after cmd= was swallowed into the command, giving Base nil
    — a WIDER policy than written, silently.
  R6 an empty CheckCmd reached the jail as `sh -c ""`, exited 0, posted
    success. The rule was at the door that parses a policy, not the one that
    acts on it.
  R7 a huge timeout= overflowed to a negative duration, which the sandbox
    turns into its 60s default — the outcome DefaultGateTimeout's own comment
    says blocks merges.

  UNDELIVERED VERDICTS:
  R1 a failed Save was logged and ignored, success posted anyway, and with no
    dedupe row the poller re-certified that head forever.
  R3 dedupe keyed on (repo, head_sha) alone, so a second policy on the same
    repo never ran and never posted its context.
  R4/R5 the row was written before the status post, so a failed post was never
    retried and a shutdown-cancelled run stood as a permanent refusal. Fixed
    by recording whether the verdict REACHED the forge, not by guessing which
    errors are transient.

THE SECOND DOOR: grepping every Save caller while fixing R3/R4 found
internal/brain/controlgate.go saving a row with no context and no delivery
mark. Unfixed, my own fix would have made the control gate re-run its jail on
every tick forever. One rule, two doors — again.

Every fix has a negative control, and one of them mattered: my first control
harness counted "--- FAIL" lines, so a build failure read as a pass and R2 —
the high-severity finding — looked verified when nothing had run. Re-done
properly, all ten fail without their fix.

Also corrected: a test fixture of mine that omitted the checkpoint every real
Rekor proof carries, which made a legitimate proof look refused.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V
@pdbethke
pdbethke merged commit 382df06 into main Sep 12, 2026
5 checks passed
@pdbethke
pdbethke deleted the fix/transparency-cold-review branch September 12, 2026 16:30
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant