internal/gate at 382df06437, reviewed by antigravity, verified by codex.
In ParsePolicies, the semicolon truncation check is bypassed when the command fragment after ';' contains an '=' character, silently accepting the truncated weaker policy and discarding the remaining command into bad.
cat << 'EOF' > internal/gate/repro_semicolon_test.go
package gate
import (
"fmt"
"strings"
"testing"
)
func TestReproSemicolonWithEqualsInCmd(t *testing.T) {
pol, bad := ParsePolicies("repo=o/r,cmd=go vet ./... ; go test -tags=integration ./...")
if len(pol) == 0 {
t.Fatalf("defect not present: policy was rejected")
}
cmd := strings.Join(pol[0].CheckCmd, " ")
if strings.Contains(cmd, "go test") {
t.Fatalf("defect not present: second command was preserved: %q", cmd)
}
fmt.Printf("DEFECT DEMONSTRATED: accepted truncated policy CheckCmd=%v (bad=%v)\n", pol[0].CheckCmd, bad)
}
EOF
go test -v -run TestReproSemicolonWithEqualsInCmd ./internal/gate=== RUN TestReproSemicolonWithEqualsInCmd DEFECT DEMONSTRATED: accepted truncated policy CheckCmd=[go vet ./...] (bad=[go test -tags=integration ./...]) --- PASS: TestReproSemicolonWithEqualsInCmd (0.00s) PASS ok github.com/pdbethke/corralai/internal/gate 0.005s
I checked whether rejecting the second fragment also retracts the first policy. For the exact input, internal/gate/config.go:82 skips that retraction because '-tags=integration' contains '='. The first fragment is accepted at line 186 with CheckCmd ['go','vet','./...']; the second lacks repo and cmd fields and enters bad at lines 182–184. That rejection leaves the truncated policy intact.
ref 659ec89015180093f0fbd8ca1668d1d8f482737751e0f473623eb21c9c99222f#R1
In ParsePolicies, strayFieldAfterCmd checks only for exact comma-delimited strings without whitespace tolerance, allowing trailing policy fields with spaces (e.g. ', base=release') to be absorbed into the command and silently converting the policy to gate all branches.
cat << 'EOF' > internal/gate/repro_stray_field_test.go
package gate
import (
"fmt"
"testing"
)
func TestReproStrayFieldWithSpaceSwallowed(t *testing.T) {
pol, bad := ParsePolicies("repo=o/r,cmd=make test, base=release")
if len(bad) > 0 || len(pol) == 0 {
t.Fatalf("defect not present: policy was rejected as bad (%v)", bad)
}
if len(pol[0].Base) != 0 {
t.Fatalf("defect not present: Base was properly set to %v", pol[0].Base)
}
fmt.Printf("DEFECT DEMONSTRATED: policy with ', base=release' accepted silently with Base=nil (gates all branches), CheckCmd=%v\n", pol[0].CheckCmd)
}
EOF
go test -v -run TestReproStrayFieldWithSpaceSwallowed ./internal/gate=== RUN TestReproStrayFieldWithSpaceSwallowed DEFECT DEMONSTRATED: policy with ', base=release' accepted silently with Base=nil (gates all branches), CheckCmd=[make test, base=release] --- PASS: TestReproStrayFieldWithSpaceSwallowed (0.00s) PASS ok github.com/pdbethke/corralai/internal/gate 0.005s
I checked whitespace normalization and downstream branch selection. internal/gate/config.go:49 matches only ',base=', so ', base=release' survives. Lines 104–125 capture it as command text and tokenize it into ['make','test,','base=release']; the base assignment at lines 146–148 only processes the preceding header. Base consequently remains empty, and internal/gate/poller.go:49–55 explicitly interprets that as all bases. The malformed command need not succeed for the claimed policy-widening defect to exist.
ref 659ec89015180093f0fbd8ca1668d1d8f482737751e0f473623eb21c9c99222f#R2
OpenStore fails to migrate existing gate_runs tables to include context in the primary key, causing multiple policy runs on the same commit head to overwrite each other via INSERT OR REPLACE and trigger an infinite execution loop on every poller tick.
cat << 'EOF' > internal/gate/repro_migration_test.go
package gate
import (
"database/sql"
"fmt"
"path/filepath"
"testing"
"time"
_ "github.com/marcboeker/go-duckdb/v2"
)
func TestReproStoreMigrationFailsToWidenPrimaryKey(t *testing.T) {
dbPath := filepath.Join(t.TempDir(), "legacy_gate.db")
rawDB, err := sql.Open("duckdb", dbPath)
if err != nil {
t.Fatal(err)
}
_, err = rawDB.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,
ran_at TIMESTAMP NOT NULL,
PRIMARY KEY (repo, head_sha)
)`)
if err != nil {
t.Fatal(err)
}
_ = rawDB.Close()
store, err := OpenStore(dbPath)
if err != nil {
t.Fatal(err)
}
defer store.Close()
err = store.Save(Run{
Repo: "o/r",
HeadSHA: "commit1",
Context: "corral/lint",
PR: 1,
Passed: true,
StatusPosted: true,
RecordID: 101,
RanAt: time.Unix(1000, 0),
})
if err != nil {
t.Fatal(err)
}
err = store.Save(Run{
Repo: "o/r",
HeadSHA: "commit1",
Context: "corral/test",
PR: 1,
Passed: true,
StatusPosted: true,
RecordID: 102,
RanAt: time.Unix(2000, 0),
})
if err != nil {
t.Fatal(err)
}
runLint, ok, err := store.GetByHead("o/r", "commit1", "corral/lint")
if err != nil {
t.Fatal(err)
}
if !ok {
fmt.Printf("DEFECT DEMONSTRATED: legacy table migration did not widen primary key; second context overwritten first context: GetByHead(corral/lint) ok=false (run=%+v)\n", runLint)
return
}
t.Fatalf("defect not present: GetByHead found both contexts cleanly (ok=%v, run=%+v)", ok, runLint)
}
EOF
go test -v -run TestReproStoreMigrationFailsToWidenPrimaryKey ./internal/gate=== RUN TestReproStoreMigrationFailsToWidenPrimaryKey
DEFECT DEMONSTRATED: legacy table migration did not widen primary key; second context overwritten first context: GetByHead(corral/lint) ok=false (run={Repo: HeadSHA: PR:0 Passed:false Context: StatusPosted:false RecordID:0 RanAt:0001-01-01 00:00:00 +0000 UTC})
--- PASS: TestReproStoreMigrationFailsToWidenPrimaryKey (0.03s)
PASS
ok github.com/pdbethke/corralai/internal/gate 0.037sI checked whether migration changes the legacy constraint or polling compensates for it. internal/gate/store.go:28 uses CREATE TABLE IF NOT EXISTS, and lines 55–65 only add and backfill columns, preserving the legacy (repo, head_sha) primary key. Save uses INSERT OR REPLACE at line 88, while GetByHead requires the requested context at lines 128–130. With two applicable contexts on the same open head, each save replaces the other's row, and internal/gate/poller.go:65–79 runs the missing context again. This sustains repeated execution across ticks while both policies and the head remain applicable.
ref 659ec89015180093f0fbd8ca1668d1d8f482737751e0f473623eb21c9c99222f#R3
Runner.Run passes an unpopulated Policy.Context ("") directly to StatusPoster.SetCommitStatus instead of defaulting to "corral/gate", causing commit status to post under the forge default context while the dedupe store records "corral/gate".
cat << 'EOF' > internal/gate/repro_runner_context_test.go
package gate
import (
"context"
"fmt"
"path/filepath"
"testing"
"time"
)
type capturingStatusPoster struct {
contexts []string
}
func (c *capturingStatusPoster) SetCommitStatus(ctx context.Context, repoURL, sha, statusCtx, state, targetURL, description string) error {
c.contexts = append(c.contexts, statusCtx)
return nil
}
func TestReproRunnerPostsEmptyContext(t *testing.T) {
store, err := OpenStore(filepath.Join(t.TempDir(), "gate.db"))
if err != nil {
t.Fatal(err)
}
defer store.Close()
status := &capturingStatusPoster{}
r := &Runner{
Checkout: &fakeCheckouter{},
Jail: &fakeJail{exitCode: 0, output: "ok"},
Certify: &fakeCertifier{recordID: 1},
Status: status,
Store: store,
RecordURL: func(repo, sha string) string { return "http://target" },
Now: func() time.Time { return time.Unix(0, 0) },
}
pol := Policy{Repo: "o/r", CheckCmd: []string{"true"}}
err = r.Run(context.Background(), "https://github.com/o/r", pol, PRRef{Number: 1, HeadSHA: "sha1"})
if err != nil {
t.Fatal(err)
}
hasEmptyContext := false
for _, ctx := range status.contexts {
if ctx == "" {
hasEmptyContext = true
break
}
}
if !hasEmptyContext {
t.Fatalf("defect not present: all posted contexts were non-empty: %v", status.contexts)
}
run, ok, err := store.GetByHead("o/r", "sha1", "corral/gate")
if err != nil || !ok {
t.Fatalf("store lookup failed: ok=%v err=%v", ok, err)
}
fmt.Printf("DEFECT DEMONSTRATED: Runner.Run posted commit status with context %q while store recorded under context %q\n",
status.contexts, run.Context)
}
EOF
go test -v -run TestReproRunnerPostsEmptyContext ./internal/gate=== RUN TestReproRunnerPostsEmptyContext DEFECT DEMONSTRATED: Runner.Run posted commit status with context ["" ""] while store recorded under context "corral/gate" --- PASS: TestReproRunnerPostsEmptyContext (0.02s) PASS ok github.com/pdbethke/corralai/internal/gate 0.026s
I traced both status calls and store normalization, including the production adapter. internal/gate/runner.go:78 and :139 pass p.Context unchanged; internal/gate/store.go:83–85 normalizes an empty context to 'corral/gate'. internal/repo/repo.go:182–187 forwards the empty string, and internal/repo/provider.go:437–439 serializes it unchanged. The supplied fake proves the argument/store mismatch, although it does not establish how a live forge handles an explicitly empty context. No normalization in the traced posting path refutes the underlying defect.
ref 659ec89015180093f0fbd8ca1668d1d8f482737751e0f473623eb21c9c99222f#R4
Runner.Run calculates the jail timeout as time.Duration(p.TimeoutS) * time.Second without bounds checking, allowing large TimeoutS values to overflow int64 into negative durations that cause the sandbox to fall back to its 60-second default.
cat << 'EOF' > internal/gate/repro_runner_timeout_test.go
package gate
import (
"context"
"fmt"
"path/filepath"
"testing"
"time"
)
func TestReproRunnerTimeoutOverflowsToNegative(t *testing.T) {
store, err := OpenStore(filepath.Join(t.TempDir(), "gate.db"))
if err != nil {
t.Fatal(err)
}
defer store.Close()
jail := &fakeJail{exitCode: 0, output: "ok"}
r := &Runner{
Checkout: &fakeCheckouter{},
Jail: jail,
Certify: &fakeCertifier{recordID: 1},
Status: &fakeStatusPoster{},
Store: store,
RecordURL: func(repo, sha string) string { return "http://target" },
Now: func() time.Time { return time.Unix(0, 0) },
}
pol := Policy{Repo: "o/r", Context: "corral/gate", CheckCmd: []string{"true"}, TimeoutS: 1 << 62}
err = r.Run(context.Background(), "https://github.com/o/r", pol, PRRef{Number: 1, HeadSHA: "sha1"})
if err != nil {
t.Fatal(err)
}
if jail.lastTimeout >= 0 {
t.Fatalf("defect not present: jail timeout was not negative: %v", jail.lastTimeout)
}
fmt.Printf("DEFECT DEMONSTRATED: Runner computed negative jail timeout %v from TimeoutS=%d, triggering sandbox 60s fallback\n",
jail.lastTimeout, pol.TimeoutS)
}
EOF
go test -v -run TestReproRunnerTimeoutOverflowsToNegative ./internal/gate=== RUN TestReproRunnerTimeoutOverflowsToNegative
repro_runner_timeout_test.go:36: defect not present: jail timeout was not negative: 0s
--- FAIL: TestReproRunnerTimeoutOverflowsToNegative (0.02s)
FAIL
FAIL github.com/pdbethke/corralai/internal/gate 0.025s
FAILI checked the failed script's exact arithmetic and traced the effective timeout to the sandbox. On a 64-bit target, TimeoutS=1<<62 multiplied by 1,000,000,000 wraps to zero, explaining the failed negative-duration assertion. This does not refute the timeout defect: internal/gate/runner.go:101–105 passes that zero without correction; internal/brain/gate.go:62 forwards it to RunGuarded; internal/sandbox/sandbox.go:290–291 delegates to Run, whose lines 190–191 replace any nonpositive timeout with 60 seconds. The parser's bound does not protect programmatically constructed policies. The script's sign expectation is wrong, but its exact input still reaches the claimed restrictive fallback.
ref 659ec89015180093f0fbd8ca1668d1d8f482737751e0f473623eb21c9c99222f#R5
Recorded so that "found nothing here" can be told apart from "never looked here".
The internal/gate package attempts to implement a secure, fail-closed merge gate and status-checking pipeline, but the recent review fixes introduced fragile heuristics that fail on common inputs and left structural doors unguarded. Specifically, the fix for R2 in Finding 1 relies on a naive check (!strings.Contains(entry, "=")) that is completely bypassed whenever a secondary command uses flag syntax such as -tags=... or environment assignments, quietly accepting truncated commands and executing weaker policies. Finding 2 demonstrates that the fix for R8 (strayFieldAfterCmd) checks only for exact comma-prefixed strings without whitespace tolerance, silently absorbing trailing policy fields like ', base=release' into the command and dangerously widening the policy to all base branches. Finding 3 reveals a critical database migration flaw: OpenStore assumes that leaving pre-existing DuckDB tables with their legacy (repo, head_sha) primary key is harmless, but INSERT OR REPLACE on the narrower key causes multi-context policies on the same commit to overwrite each other, causing GetByHead to miss rows and trapping the poller in an infinite re-execution loop on every tick. Finally, Findings 4 and 5 demonstrate that defensive invariants were applied only to the parsing layer in config.go and omitted from runner.go, allowing programmatic policies to post status under an empty forge context while recording 'corral/gate' in the store (Finding 4) and to overflow int64 durations into negative timeouts that trigger the sandbox's restrictive 60-second fallback (Finding 5).
R1–R4 stand after tracing the relevant parsing, storage, polling, and status-posting paths. R5 also stands: its script incorrectly expects a negative duration for an input that overflows to zero, but zero triggers the same 60-second fallback. I did not execute tests or modify the repository.