internal/gate at 459add6e04, reviewed by claude-code, verified by antigravity.
Runner.Run posts "success" even when Store.Save fails (only logged at runner.go:107-109), contradicting its documented fail-closed invariant. No dedupe row is written, so every poll tick re-runs the jail, re-certifies (appending a new signed record each time) and re-posts success forever, and the status target_url /api/gate/run returns 404.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_save_test.go <<'EOF'
package gate
import (
"context"
"testing"
)
func TestColdReviewSaveFailStillPostsSuccess(t *testing.T) {
checkout := &fakeCheckouter{}
jail := &fakeJail{exitCode: 0, output: "ok"}
cert := &fakeCertifier{recordID: 42}
status := &fakeStatusPoster{}
r := newTestRunner(t, checkout, jail, cert, status)
// make every write fail (as disk-full / a locked write would) while reads keep working
if _, err := r.Store.db.Exec(`DROP TABLE gate_runs`); err != nil { t.Fatal(err) }
if _, err := r.Store.db.Exec(`CREATE TABLE gate_runs (repo VARCHAR NOT NULL, head_sha VARCHAR NOT NULL, pr INTEGER NOT NULL, passed BOOLEAN NOT NULL, record_id BIGINT NOT NULL CHECK (record_id < 0), ran_at TIMESTAMP NOT NULL, PRIMARY KEY (repo, head_sha))`); err != nil { t.Fatal(err) }
if err := r.Store.Save(Run{Repo: "x", HeadSHA: "y", RecordID: 1}); err == nil { t.Fatal("setup: Save should fail") }
p := &Poller{Policies: []Policy{testPolicy()}, List: &fakeLister{prs: []PRRef{testPR()}}, Store: r.Store, Run: r.Run}
_ = p.Tick(context.Background())
_ = p.Tick(context.Background())
_ = p.Tick(context.Background())
_, stored, lerr := r.Store.GetBySHA("o/r", "deadbeef")
succ := 0
for _, s := range status.states { if s == "success" { succ++ } }
t.Logf("EVIDENCE: statuses=%v success_posts=%d certify_calls=%d jail_runs=%d dedupe_row_stored=%v lookup_err=%v", status.states, succ, cert.calls, jail.calls, stored, lerr)
if succ == 0 || stored || lerr != nil || cert.calls != 3 { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewSaveFailStillPostsSuccess$' -count=1 -v=== RUN TestColdReviewSaveFailStillPostsSuccess
2026/09/12 12:03:39 gate: save dedupe o/r@deadbeef: gate: save: Constraint Error: CHECK constraint failed on table gate_runs with expression CHECK((record_id < 0))
2026/09/12 12:03:39 gate: save dedupe o/r@deadbeef: gate: save: Constraint Error: CHECK constraint failed on table gate_runs with expression CHECK((record_id < 0))
2026/09/12 12:03:39 gate: save dedupe o/r@deadbeef: gate: save: Constraint Error: CHECK constraint failed on table gate_runs with expression CHECK((record_id < 0))
zz_coldreview_save_test.go:25: EVIDENCE: statuses=[pending success pending success pending success] success_posts=3 certify_calls=3 jail_runs=3 dedupe_row_stored=false lookup_err=<nil>
--- PASS: TestColdReviewSaveFailStillPostsSuccess (0.02s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.029sInspected Runner.Run at internal/gate/runner.go:106-115. When r.Store.Save returns an error at line 107, the error is merely logged via log.Printf and control flow falls through directly to evaluating state := 'failure'; if passed { state = 'success' }; return r.Status.SetCommitStatus(...). If the jail exited 0, 'success' is posted despite the persistence failure, directly contradicting the documented fail-closed invariant at runner.go:57-60 that 'Run posts commit status "success" ONLY when the jail check truly executed and exited 0 AND every prior step (checkout, sign, store) succeeded.' Furthermore, because Store.Save failed to record the row, subsequent poller ticks find ok=false from GetBySHA, re-running the jail, appending duplicate signatures in the build store, and re-posting success indefinitely while /api/gate/run returns 404.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R1
ParsePolicies splits on ';' before it isolates cmd=, so a shell command such as "go vet ./... ; go test ./..." (run via sh -c) is truncated to the weaker "go vet ./..." and that policy is ACCEPTED. Only the orphaned tail is reported as bad. This is the 'truncated cmd manufactures a green gate' path the file's own comment says it prevents.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_semi_test.go <<'EOF'
package gate
import (
"strings"
"testing"
)
func TestColdReviewSemicolonTruncatesCmd(t *testing.T) {
raw := "repo=o/r,base=main,cmd=go vet ./... ; go test ./..."
pols, bad := ParsePolicies(raw)
got := ""
if len(pols) == 1 { got = strings.Join(pols[0].CheckCmd, " ") }
t.Logf("EVIDENCE: operator cmd=%q -> accepted policies=%d with CheckCmd=%q; bad=%q", "go vet ./... ; go test ./...", len(pols), got, bad)
if len(pols) != 1 || got != "go vet ./..." { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewSemicolonTruncatesCmd$' -count=1 -v=== RUN TestColdReviewSemicolonTruncatesCmd
zz_coldreview_semi_test.go:13: EVIDENCE: operator cmd="go vet ./... ; go test ./..." -> accepted policies=1 with CheckCmd="go vet ./..."; bad=["go test ./..."]
--- PASS: TestColdReviewSemicolonTruncatesCmd (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.005sInspected ParsePolicies at internal/gate/config.go:40-71. ParsePolicies splits raw on ';' at line 40 before inspecting or isolating ',cmd='. For an input like 'repo=o/r,base=main,cmd=go vet ./... ; go test ./...', strings.Split divides the string at the semicolon. The first entry has repo=o/r and cmd=go vet ./..., which satisfies both repoSeen and cmdSeen and is accepted into policies as a Policy with CheckCmd=['go', 'vet', './...']. The second segment (' go test ./...') lacks repo= and cmd=, so it is placed into bad. Because the jail executes commands using sh -c inside bwrap, the accepted policy executes only the weaker truncated prefix command without running the test suite, allowing a green gate status to be posted on a partial check.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R2
Dedupe is keyed on (repo, head_sha) only. With two policies for the same repo and different contexts (e.g. corral/lint and corral/test), the first policy's run stores the row, and the Poller skips the second policy for every head, so its check never runs and its status context is never posted; if that context is a required check, the PR is blocked indefinitely.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_twopol_test.go <<'EOF'
package gate
import (
"context"
"path/filepath"
"strings"
"testing"
"time"
)
type crCtxStatus struct{ posted []string }
func (c *crCtxStatus) SetCommitStatus(ctx context.Context, repoURL, sha, sctx, state, target, desc string) error {
c.posted = append(c.posted, sctx+"="+state)
return nil
}
func TestColdReviewSecondPolicySameRepoNeverGated(t *testing.T) {
store, err := OpenStore(filepath.Join(t.TempDir(), "g.db"))
if err != nil { t.Fatal(err) }
defer store.Close()
st := &crCtxStatus{}
jail := &fakeJail{exitCode: 0, output: "ok"}
r := &Runner{Checkout: &fakeCheckouter{}, Jail: jail, Certify: &fakeCertifier{recordID: 1}, Status: st, Store: store,
RecordURL: func(a, b string) string { return "" }, Now: func() time.Time { return time.Unix(1000, 0) }}
pols, bad := ParsePolicies("repo=o/r,base=main,context=corral/lint,cmd=make lint;repo=o/r,base=main,context=corral/test,cmd=make test")
if len(bad) != 0 || len(pols) != 2 { t.Fatalf("setup: pols=%v bad=%v", pols, bad) }
p := &Poller{Policies: pols, List: &fakeLister{prs: []PRRef{{Number: 1, HeadSHA: "abc", Base: "main"}}}, Store: store, Run: r.Run}
_ = p.Tick(context.Background())
_ = p.Tick(context.Background())
t.Logf("EVIDENCE: accepted policies %q and %q; jail runs=%d; statuses posted=%v", pols[0].Context, pols[1].Context, jail.calls, st.posted)
for _, s := range st.posted {
if strings.HasPrefix(s, "corral/test=") { t.Fatal("second policy was gated; defect not demonstrated") }
}
if jail.calls != 1 { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewSecondPolicySameRepoNeverGated$' -count=1 -v=== RUN TestColdReviewSecondPolicySameRepoNeverGated
zz_coldreview_twopol_test.go:31: EVIDENCE: accepted policies "corral/lint" and "corral/test"; jail runs=1; statuses posted=[corral/lint=pending corral/lint=success]
--- PASS: TestColdReviewSecondPolicySameRepoNeverGated (0.03s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.031sInspected internal/gate/store.go:28-36 and internal/gate/poller.go:48-74. In store.go, the gate_runs schema defines PRIMARY KEY (repo, head_sha) and GetBySHA queries strictly by repo and head_sha without taking Policy.Context into account. In poller.go, Tick iterates over each declared Policy, querying p.Store.GetBySHA(pol.Repo, pr.HeadSHA) for each PR head. When multiple policies target the same repo with different contexts (e.g. corral/lint and corral/test), the first policy executes and writes a dedupe row. On the second policy's pass for that head SHA, GetBySHA returns ok=true, hitting if ok { continue } and skipping execution. The second policy's check is never run and its status context is never posted to the forge.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R3
The dedupe row is saved BEFORE the final commit status is posted, both in Run and in FailClosed. If that post fails (forge 5xx, rate limit), Run returns the error, the Poller logs it, and every later tick skips the SHA as already gated. The verdict is never delivered to the forge, and the check stays pending or missing until someone pushes a new head.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_status_test.go <<'EOF'
package gate
import (
"context"
"errors"
"testing"
)
func TestColdReviewStatusFailureNeverRetried(t *testing.T) {
status := &fakeStatusPoster{err: errors.New("github: 502 bad gateway")}
cert := &fakeCertifier{recordID: 7}
jail := &fakeJail{exitCode: 0, output: "ok"}
r := newTestRunner(t, &fakeCheckouter{}, jail, cert, status)
p := &Poller{Policies: []Policy{testPolicy()}, List: &fakeLister{prs: []PRRef{testPR()}}, Store: r.Store, Run: r.Run}
_ = p.Tick(context.Background())
during := len(status.states)
status.err = nil // forge recovers
_ = p.Tick(context.Background())
_ = p.Tick(context.Background())
run, ok, _ := r.Store.GetBySHA("o/r", "deadbeef")
t.Logf("EVIDENCE: status attempts during outage=%v (all failed); attempts after forge recovered=%d; jail runs=%d; store row present=%v passed=%v", status.states[:during], len(status.states)-during, jail.calls, ok, run.Passed)
if !ok || len(status.states) != during || jail.calls != 1 { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewStatusFailureNeverRetried$' -count=1 -v=== RUN TestColdReviewStatusFailureNeverRetried
2026/09/12 12:03:43 gate: poller: run o/r#7@deadbeef: github: 502 bad gateway
zz_coldreview_status_test.go:21: EVIDENCE: status attempts during outage=[pending success] (all failed); attempts after forge recovered=0; jail runs=1; store row present=true passed=true
--- PASS: TestColdReviewStatusFailureNeverRetried (0.02s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.027sInspected Runner.Run (runner.go:107-114) and FailClosed (failclosed.go:17-20). In both functions, Store.Save is invoked and committed to DuckDB prior to calling Status.SetCommitStatus. If SetCommitStatus fails due to a forge outage, rate limit, or transient network error, Run returns the error to Poller.Tick (poller.go:69-71), which logs it and continues. On all subsequent poller intervals, p.Store.GetBySHA returns ok=true because the dedupe row was already saved, causing Tick to skip the head. No retry of the commit status post is ever attempted, leaving the commit check in pending or unset indefinitely on the forge.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R4
Scenario: the brain shuts down mid-run and cancels ctx. RunGuarded returns a 'cancelled' error, Run takes fail(), FailClosed stores Passed=false permanently (Save does not use ctx), and then the status post with the cancelled ctx fails. The forge keeps the earlier "pending" indefinitely, and after restart the poller skips the head as already gated. Any transient checkout failure is likewise made permanent.
Inspected context cancellation propagation across internal/gate/runner.go:94-97, internal/gate/failclosed.go:16-21, and internal/sandbox/sandbox.go:257-266. When ctx is cancelled during execution (such as on daemon shutdown), sandbox.RunGuarded returns an error ('cancelled before the run could be graded'). Runner.Run branches to r.fail, which delegates to FailClosed. FailClosed calls store.Save, which executes s.db.Exec synchronously without context checking, permanently recording Passed=false in gate_runs. Next, status.SetCommitStatus is called with the cancelled ctx and fails immediately, leaving the forge with the earlier 'pending' status. On restart, Poller.Tick queries GetBySHA, finds ok=true, and skips the head, permanently stranding the PR. Similarly, a transient checkout network error in CheckoutPR triggers r.fail and is permanently stored as Passed=false without retry.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R5
Runner.Run does not check that p.CheckCmd is non-empty; only ParsePolicies enforces it. A Policy built programmatically, for example via brain Options.GatePolicies, with no CheckCmd sends "" to the jail. The jail runs sh -c on rlimitPrelude+"", which exits 0, and "success" is posted for a check that ran nothing.
Inspected Runner.Run at internal/gate/runner.go:94 and StartGate at internal/brain/gate.go:81-133. Runner.Run does not validate that len(p.CheckCmd) > 0 before calling r.Jail.Run(ctx, strings.Join(p.CheckCmd, ' '), ...). If a policy is configured programmatically (such as via Options.GatePolicies without going through ParsePolicies) with an empty or nil CheckCmd, an empty string is passed to the jail. In production with bwrapIsolator (internal/sandbox/isolator_linux.go:162), the executed command is sh -c rlimitPrelude + '', which runs 'ulimit -u 1024 2>/dev/null; ulimit -f 4194304 2>/dev/null; ' and exits 0. Certify accepts the empty command with exit 0, and Runner.Run posts 'success' for an execution that evaluated no checks.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R6
A large timeout= is accepted by strconv.Atoi, and time.Duration(TimeoutS)*time.Second then silently overflows to a negative duration. The sandbox turns any deadline <=0 into its 60s default, which is the very failure DefaultGateTimeout's comment says permanently blocks merge; no error is reported.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_timeout_test.go <<'EOF'
package gate
import (
"context"
"testing"
)
func TestColdReviewTimeoutOverflow(t *testing.T) {
pols, bad := ParsePolicies("repo=o/r,base=main,timeout=10000000000,cmd=make test")
if len(pols) != 1 { t.Fatalf("setup: %v %v", pols, bad) }
jail := &fakeJail{exitCode: 1}
r := newTestRunner(t, &fakeCheckouter{}, jail, &fakeCertifier{recordID: 1}, &fakeStatusPoster{})
_ = r.Run(context.Background(), "https://github.com/o/r", pols[0], testPR())
t.Logf("EVIDENCE: TimeoutS=%d accepted with bad=%q; jail deadline passed=%v (sandbox maps <=0 to its 60s default)", pols[0].TimeoutS, bad, jail.lastTimeout)
if jail.lastTimeout > 0 { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewTimeoutOverflow$' -count=1 -v=== RUN TestColdReviewTimeoutOverflow
zz_coldreview_timeout_test.go:14: EVIDENCE: TimeoutS=10000000000 accepted with bad=[]; jail deadline passed=-2346317h47m53.709551616s (sandbox maps <=0 to its 60s default)
--- PASS: TestColdReviewTimeoutOverflow (0.02s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.022sInspected ParsePolicies (internal/gate/config.go:98-101), Runner.Run (internal/gate/runner.go:90-93), and sandbox.Run (internal/sandbox/sandbox.go:190-192). In config.go, strconv.Atoi parses any valid integer into int without range checking. On 64-bit platforms where int is int64, passing a value such as timeout=10000000000 yields TimeoutS=10000000000. In Runner.Run, time.Duration(p.TimeoutS) * time.Second multiplies 10^10 by 10^9 nanoseconds (10^19), exceeding math.MaxInt64 (~9.22e18) and overflowing to a negative duration (-8446744073709551616 ns). In sandbox.Run, if opts.Timeout <= 0 { opts.Timeout = 60 * time.Second } clamps this negative duration to the 60-second default without reporting an error, bypassing DefaultGateTimeout (600s).
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R7
A field placed after cmd= is silently swallowed into the command rather than reported as bad, despite the doc's promise that cmd parsing 'fails loudly'. "repo=o/r,cmd=make test,base=release" is accepted with Base=nil, which gates ALL bases, and CheckCmd [make test,base=release], with nothing in bad.
#!/bin/sh
set -e
cat > internal/gate/zz_coldreview_after_cmd_test.go <<'EOF'
package gate
import "testing"
func TestColdReviewFieldAfterCmdSwallowed(t *testing.T) {
pols, bad := ParsePolicies("repo=o/r,cmd=make test,base=release")
t.Logf("EVIDENCE: policies=%+v bad=%q", pols, bad)
if len(bad) != 0 || len(pols) != 1 || pols[0].Base != nil || len(pols[0].CheckCmd) != 2 || pols[0].CheckCmd[1] != "test,base=release" { t.Fatal("defect not demonstrated") }
}
EOF
go test ./internal/gate/ -run '^TestColdReviewFieldAfterCmdSwallowed$' -count=1 -v=== RUN TestColdReviewFieldAfterCmdSwallowed
zz_coldreview_after_cmd_test.go:7: EVIDENCE: policies=[{Repo:o/r Base:[] Context:corral/gate CheckCmd:[make test,base=release] AllowNet:false TimeoutS:0}] bad=[]
--- PASS: TestColdReviewFieldAfterCmdSwallowed (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.007sInspected ParsePolicies at internal/gate/config.go:46-72. When strings.Index finds ',cmd=', head is set to entry[:idx] and cmdVal takes entry[idx+len(',cmd='):]. The key-value parsing loop processes only head, while strings.Fields(cmdVal) processes the command tail. When given an input such as 'repo=o/r,cmd=make test,base=release', the trailing ',base=release' is never parsed as a configuration key; pol.Base remains nil (defaulting to matching all base branches), CheckCmd becomes ['make', 'test,base=release'], and bad is empty. Despite the header comment specifying that 'cmd= MUST be the LAST field in an entry', trailing fields are swallowed into the command rather than rejected as malformed.
ref 7f7b9576c7618ecda8af409c0fb981e88a0e851d9961e61f39b694b374befde3#R8
Recorded so that "found nothing here" can be told apart from "never looked here".
internal/gate is fail-closed on the paths its own tests cover: a checkout, jail or signing error never posts "success", and sandbox.RunGuarded makes sure a timeout or a cancelled run can never look like exit 0. The defects sit where the tests don't go, and they follow the same one-door shape. (1) The Runner's doc says it posts "success" only when checkout, sign AND store all succeeded. The store step is only logged, though. A failed Save still posts success and leaves no dedupe row, so the poller re-runs the check and appends a new signed record every tick, indefinitely. (2) cmd parsing takes pains to keep commas so a command is never silently truncated into a weaker one. The earlier split on ';' does exactly that truncation, and the weaker policy is accepted, not refused. (3) Dedupe is keyed on (repo, head_sha) without the policy or its context. Two policies for the same repo, which the config doc invites and which configurable contexts imply, get only the first one run. The second commit-status context is never posted. (4) In both the pass path and FailClosed, the store row is written before the final status post. A failed post (forge outage, rate limit, the context cancelled at shutdown) is therefore never retried, and the forge keeps whatever state it last received, often "pending", indefinitely. (5) A transient or shutdown-caused error is stored as a permanent Passed=false. Combined with a status post that fails on the cancelled context, this strands a PR head in pending. (6) The Runner itself does not check that CheckCmd is non-empty; only ParsePolicies enforces it. (7) and (8) are low-severity parser defects: a timeout overflow falls back to the sandbox's 60s default, and fields placed after cmd= are silently swallowed rather than reported. Findings 1–4, 7 and 8 come with scripts. Each script writes a test next to the code, inside package gate, reuses the package's existing fakes, and exits 0 only if the defect shows.
The review is exceptionally thorough, technically precise, and verified against the implementation. Findings R1 and R4 correctly identify ordering and error-handling bugs between Store.Save and Status.SetCommitStatus where failed saves still post success, and failed status posts are permanently suppressed from retry by premature dedupe writes. Findings R2 and R8 accurately identify parsing defects in ParsePolicies caused by top-level semicolon splitting before command isolation and greedy tail capture without trailing-field validation. Finding R3 correctly demonstrates that keying dedupe solely on (repo, head_sha) prevents multi-policy execution across distinct contexts for the same repository. Findings R5 and R6 correctly trace failure-mode edge cases where context cancellation causes permanent false-negative dedupe entries while stranding PR status in pending, and where an empty CheckCmd bypasses validation to post a green status via ulimit prelude execution. Finding R7 demonstrates real integer overflow in time.Duration nanosecond calculation when TimeoutS exceeds ~9.22e9 seconds, causing fallback to the 60s sandbox default. None of the 8 findings could be refuted.