diff --git a/ROADMAP.md b/ROADMAP.md index 34a42fcc..719c2fa6 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -212,7 +212,7 @@ carries the product now. and posts `corral/gate = pass|fail` — a status branch protection **requires**, so a red or missing verdict blocks the merge. Fail-closed (a `success` is only ever posted on a real exit-0), and the gated SHA is provably the merged SHA. v1 is GitHub + - opt-in (`CORRALAI_GATE_POLICIES`). + opt-in (one `CORRALAI_GATE_POLICY_` per repo). - **The control gate.** The merge gate runs the *repo's own* check; the control gate runs the **control owner's independently-vetted tests** against each PR head — the person accountable for code they didn't write sets the bar, and the author can't diff --git a/cmd/corral-wrangler/main.go b/cmd/corral-wrangler/main.go index 108285c7..9ad20c83 100644 --- a/cmd/corral-wrangler/main.go +++ b/cmd/corral-wrangler/main.go @@ -61,11 +61,14 @@ // from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) // CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); // `corral certify verify` checks the same default unless --rekor-url overrides it -// CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; -// cmd= MUST be the last field — everything after it is the command verbatim (commas -// allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +// CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; +// cmd= MUST be last — everything after it is the command VERBATIM to the end of the +// value: commas, semicolons, quotes and newlines included, so nothing can truncate it +// into a weaker command that exits 0 and posts a wrongful success; // timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; -// empty => the repo gate is OFF (no poller starts); GitHub-only for v1 +// no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. +// Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with +// commands containing ";" // CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) // CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) // CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below); diff --git a/cmd/corral/main.go b/cmd/corral/main.go index c2b82a98..53450e26 100644 --- a/cmd/corral/main.go +++ b/cmd/corral/main.go @@ -57,11 +57,14 @@ // from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) // CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); // `corral certify verify` checks the same default unless --rekor-url overrides it -// CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; -// cmd= MUST be the last field — everything after it is the command verbatim (commas -// allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +// CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; +// cmd= MUST be last — everything after it is the command VERBATIM to the end of the +// value: commas, semicolons, quotes and newlines included, so nothing can truncate it +// into a weaker command that exits 0 and posts a wrongful success; // timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; -// empty => the repo gate is OFF (no poller starts); GitHub-only for v1 +// no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. +// Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with +// commands containing ";" // CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) // CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) // CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below); diff --git a/docs/cli/corral-wrangler.md b/docs/cli/corral-wrangler.md index e112e615..e1327904 100644 --- a/docs/cli/corral-wrangler.md +++ b/docs/cli/corral-wrangler.md @@ -89,11 +89,14 @@ CORRALAI_BRAIN_TOKEN `corral certify`'s bearer token to authenticate to a from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); `corral certify verify` checks the same default unless --rekor-url overrides it -CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; - cmd= MUST be the last field — everything after it is the command verbatim (commas - allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; + cmd= MUST be last — everything after it is the command VERBATIM to the end of the + value: commas, semicolons, quotes and newlines included, so nothing can truncate it + into a weaker command that exits 0 and posts a wrongful success; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; - empty => the repo gate is OFF (no poller starts); GitHub-only for v1 + no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. + Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with + commands containing ";" CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below); diff --git a/docs/cli/corral.md b/docs/cli/corral.md index 1fa78136..0c20e83f 100644 --- a/docs/cli/corral.md +++ b/docs/cli/corral.md @@ -999,11 +999,14 @@ CORRALAI_BRAIN_TOKEN `corral certify`'s bearer token to authenticate to a from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); `corral certify verify` checks the same default unless --rekor-url overrides it -CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; - cmd= MUST be the last field — everything after it is the command verbatim (commas - allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; + cmd= MUST be last — everything after it is the command VERBATIM to the end of the + value: commas, semicolons, quotes and newlines included, so nothing can truncate it + into a weaker command that exits 0 and posts a wrongful success; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; - empty => the repo gate is OFF (no poller starts); GitHub-only for v1 + no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. + Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with + commands containing ";" CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below); diff --git a/internal/brain/gate.go b/internal/brain/gate.go index bef8dabf..a18e4a2c 100644 --- a/internal/brain/gate.go +++ b/internal/brain/gate.go @@ -71,7 +71,7 @@ func (j jailAdapter) Run(ctx context.Context, command, workspace string, network // // opts.GatePolicies == nil/empty is the feature's OFF switch: StartGate is // a complete no-op (nil, nil) — zero behavior change for a brain that -// doesn't set CORRALAI_GATE_POLICIES. +// sets no CORRALAI_GATE_POLICY_. // // opts.GateBackend == nil DISABLES gating even when policies ARE // configured: this is the fail-closed contract carried up from @@ -83,11 +83,11 @@ func StartGate(ctx context.Context, opts Options) (*gate.Store, error) { return nil, nil } if opts.GateBackend == nil { - log.Printf("gate: DISABLED — CORRALAI_GATE_POLICIES is set (%d polic(ies)) but no sandbox isolation backend is available; refusing to run PR checks unsandboxed (set CORRALAI_GATE_EXEC_BACKEND)", len(opts.GatePolicies)) + log.Printf("gate: DISABLED — %d gate polic(ies) are configured but no sandbox isolation backend is available; refusing to run PR checks unsandboxed (set CORRALAI_GATE_EXEC_BACKEND)", len(opts.GatePolicies)) return nil, nil } if opts.Repo == nil { - log.Printf("gate: DISABLED — CORRALAI_GATE_POLICIES is set but no repo.Engine is configured (Options.Repo is nil)") + log.Printf("gate: DISABLED — a gate policy is configured but no repo.Engine is configured (Options.Repo is nil)") return nil, nil } diff --git a/internal/brain/gate_test.go b/internal/brain/gate_test.go index 99b35b7e..70c3e33b 100644 --- a/internal/brain/gate_test.go +++ b/internal/brain/gate_test.go @@ -186,7 +186,7 @@ func TestGateRunHandlerUnknownSHAIs404(t *testing.T) { // startup failure). func TestStartGateNilBackendDisablesGating(t *testing.T) { store, err := StartGate(context.Background(), Options{ - GatePolicies: []gate.Policy{{Repo: "o/r", CheckCmd: []string{"true"}}}, + GatePolicies: []gate.Policy{{Repo: "o/r", CheckCmd: "true"}}, GateBackend: nil, GateDB: filepath.Join(t.TempDir(), "gate.db"), }) @@ -198,7 +198,7 @@ func TestStartGateNilBackendDisablesGating(t *testing.T) { } } -// TestStartGateEmptyPoliciesIsOff: no CORRALAI_GATE_POLICIES configured +// TestStartGateEmptyPoliciesIsOff: no CORRALAI_GATE_POLICY_ configured // means the feature is off — StartGate must be a complete no-op. func TestStartGateEmptyPoliciesIsOff(t *testing.T) { store, err := StartGate(context.Background(), Options{}) diff --git a/internal/brain/identity.go b/internal/brain/identity.go index fe8657f1..7a633da1 100644 --- a/internal/brain/identity.go +++ b/internal/brain/identity.go @@ -286,7 +286,7 @@ type Options struct { Witness transparency.Witness // GatePolicies declares which repos/branches the merge gate (repo-gate - // control plane) covers — parsed from CORRALAI_GATE_POLICIES. Empty/nil + // control plane) covers — parsed from CORRALAI_GATE_POLICY_. Empty/nil // is the feature's off switch: StartGate is then a complete no-op and // no poller ever starts (zero behavior change for existing brains). GatePolicies []gate.Policy diff --git a/internal/gate/coldreview3_test.go b/internal/gate/coldreview3_test.go index eae1c0cb..26facfdb 100644 --- a/internal/gate/coldreview3_test.go +++ b/internal/gate/coldreview3_test.go @@ -7,7 +7,6 @@ import ( "database/sql" "path/filepath" "slices" - "strings" "testing" "time" ) @@ -22,93 +21,6 @@ import ( // where it supplied them, because a test written from my paraphrase of a // finding is a test written by the person who missed it. -// TestSemicolonGuardIsNotDefeatedByAnEqualsSign is round three's R1. -// -// MY BUG: the round-two guard asked `!strings.Contains(entry, "=")` to decide -// whether a fragment was a continuation of a truncated command. Any ordinary -// second command defeats that — "go test -tags=integration ./..." contains an -// '=' — so the fragment read as a policy entry, the truncation went unnoticed -// again, and the weaker command was accepted. I guarded a real case and left -// the common one open. -// -// The rule is now keyed on repo=, the field a policy REQUIRES. -func TestSemicolonGuardIsNotDefeatedByAnEqualsSign(t *testing.T) { - for _, tail := range []string{ - "go test -tags=integration ./...", // the reviewer's own input - "GOFLAGS=-mod=mod go test ./...", - "make check VAR=1", - "go test ./...", // no '=' at all: the case round two did catch - } { - t.Run(tail, func(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,cmd=go vet ./... ; " + tail) - if len(pol) != 0 { - t.Errorf("accepted %d policy/policies with command %q — the operator wrote two steps and this gate runs one, then posts success", - len(pol), strings.Join(pol[0].CheckCmd, " ")) - } - if len(bad) == 0 { - t.Fatal("the truncation was silent") - } - }) - } -} - -// TestTwoRealEntriesSharingARepoStillParse is the control for R1's fix. The -// guard drops the PRECEDING policy when it sees a continuation, and it used to -// identify that policy with `strings.Contains(frags[i-1], policies[n-1].Repo)` -// — another guess, which misfires when two entries share a repo. It is keyed -// on the fragment's index now, and legitimate multi-entry values must survive. -func TestTwoRealEntriesSharingARepoStillParse(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,context=corral/lint,cmd=golangci-lint run;repo=o/r,context=corral/test,cmd=go test ./...") - if len(pol) != 2 { - t.Fatalf("policies = %d, want 2 (bad=%v) — two contexts on one repo is the documented multi-policy shape", len(pol), bad) - } - if pol[0].Context == pol[1].Context { - t.Errorf("both policies came back with context %q", pol[0].Context) - } - if got := strings.Join(pol[1].CheckCmd, " "); got != "go test ./..." { - t.Errorf("second command = %q, want it intact", got) - } -} - -// TestStrayFieldAfterCmdToleratesWhitespace is round three's R2. -// -// MY BUG: strayFieldAfterCmd matched the single spelling ","+f+"=" exactly. An -// operator writing the spaced form — "cmd=make test, base=release", which is at -// least as natural — slipped through, the field was absorbed into the command, -// and Base stayed nil: a policy gating EVERY base branch rather than the one -// named. One spelling guarded, the other left open, in the fix for a finding -// about exactly that. -func TestStrayFieldAfterCmdToleratesWhitespace(t *testing.T) { - for _, entry := range []string{ - "repo=o/r,cmd=make test, base=release", // the reviewer's own input - "repo=o/r,cmd=make test ,base=release", - "repo=o/r,cmd=make test , base = release", - "repo=o/r,cmd=make test,\tcontext=corral/other", - "repo=o/r,cmd=make test, timeout=30", - } { - t.Run(entry, func(t *testing.T) { - pol, bad := ParsePolicies(entry) - if len(pol) != 0 { - t.Errorf("accepted a policy whose Base is %v and command is %q — a WIDER policy than was written, silently", - pol[0].Base, strings.Join(pol[0].CheckCmd, " ")) - } - if len(bad) == 0 { - t.Fatal("a policy field after cmd= was swallowed with nothing reported") - } - }) - } -} - -// TestACommandContainingACommaEqualsPairStillParses is the control: the fix -// must not start refusing commands that legitimately contain ",=" where -// the word is not a policy field. -func TestACommandContainingACommaEqualsPairStillParses(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,cmd=go test -ldflags=-X main.v=1,other=2 ./...") - if len(pol) != 1 { - t.Fatalf("policies = %d, want 1 (bad=%v) — 'other' is not a policy field and must not trip the guard", len(pol), bad) - } -} - // TestLegacyKeyIsWidenedNotLeftAlone is round three's R3, and the one I am // least comfortable about, because I shipped it on the strength of a sentence // I wrote asserting it was safe. @@ -212,7 +124,7 @@ func TestRunnerPostsAndRecordsTheSameContext(t *testing.T) { } // A Policy with NO Context, as brain Options.GatePolicies may build it. if err := r.Run(context.Background(), "http://forge/o/r", - Policy{Repo: "o/r", CheckCmd: []string{"true"}}, PRRef{Number: 1, HeadSHA: "abc"}); err != nil { + Policy{Repo: "o/r", CheckCmd: "true"}, PRRef{Number: 1, HeadSHA: "abc"}); err != nil { t.Fatal(err) } @@ -273,7 +185,7 @@ func TestTheRunnerActuallyUsesTheBoundedTimeout(t *testing.T) { Now: func() time.Time { return time.Unix(0, 0) }, } _ = r.Run(context.Background(), "http://forge/o/r", - Policy{Repo: "o/r", Context: "corral/gate", CheckCmd: []string{"true"}, TimeoutS: 9223372036854775807}, + Policy{Repo: "o/r", Context: "corral/gate", CheckCmd: "true", TimeoutS: 9223372036854775807}, PRRef{Number: 1, HeadSHA: "abc"}) if jail.lastTimeout <= 0 { diff --git a/internal/gate/coldreview_test.go b/internal/gate/coldreview_test.go index cf47994f..e4021c14 100644 --- a/internal/gate/coldreview_test.go +++ b/internal/gate/coldreview_test.go @@ -6,7 +6,6 @@ import ( "context" "path/filepath" "slices" - "strings" "testing" "time" ) @@ -20,72 +19,6 @@ import ( // --- FAMILY ONE: the gate reports a result for a check it did not run --- -// TestSemicolonInCmdNeverYieldsAWeakerPolicy is R2, the high-severity one. -// -// THE DEFECT: ParsePolicies split the raw value on ';' BEFORE isolating cmd=. -// A shell command containing a semicolon — "cmd=go vet ./... ; go test ./..." -// under sh -c — was therefore torn in half: the first fragment parsed as a -// COMPLETE, VALID policy carrying only "go vet ./...", and just the orphaned -// tail landed in bad. The operator declared two steps, the gate ran one, and -// posted success. The file's own doc comment says cmd parsing exists to stop -// exactly this, having guarded the comma and not the semicolon. -func TestSemicolonInCmdNeverYieldsAWeakerPolicy(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,cmd=go vet ./... ; go test ./...") - - for _, p := range pol { - cmd := strings.Join(p.CheckCmd, " ") - if !strings.Contains(cmd, "go test") { - t.Errorf("accepted a policy whose command is %q — the operator wrote two steps and this runs one, then posts success", cmd) - } - } - if len(pol) != 0 { - t.Errorf("accepted %d policy/policies from an entry whose command was truncated; it must be refused, not repaired by guesswork", len(pol)) - } - if len(bad) == 0 { - t.Fatal("nothing reported in bad — the truncation was silent, which is the whole defect") - } - joined := strings.Join(bad, " | ") - if !strings.Contains(joined, ";") { - t.Errorf("bad = %q never mentions the ';' that caused this, so the operator cannot act on it", joined) - } -} - -// TestCmdWithCommasStillWorks is the companion control: the comma case the -// file always handled must keep working, or this fix traded one truncation for -// a refusal of good policies. -func TestCmdWithCommasStillWorks(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,base=main,cmd=go test -run A,B ./...") - if len(pol) != 1 { - t.Fatalf("policies = %d, want 1 (bad=%v) — a command containing commas is legal and documented", len(pol), bad) - } - if got := strings.Join(pol[0].CheckCmd, " "); got != "go test -run A,B ./..." { - t.Errorf("CheckCmd = %q, want the command verbatim", got) - } -} - -// TestFieldAfterCmdIsReportedNotSwallowed is R8. -// -// THE DEFECT: cmd= takes the rest of the entry verbatim, so a policy field -// written after it was absorbed into the command. "cmd=make test,base=release" -// produced CheckCmd ["make","test,base=release"] AND Base nil — a policy that -// gates EVERY base rather than the one named, silently, with nothing in bad. -// A wider policy than the operator wrote is the same class of wrong as a -// weaker command. -func TestFieldAfterCmdIsReportedNotSwallowed(t *testing.T) { - for _, field := range []string{"base=release", "context=corral/other", "net=true", "timeout=30", "repo=other/repo"} { - t.Run(field, func(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,cmd=make test," + field) - if len(pol) != 0 { - t.Errorf("accepted a policy with %q after cmd=: CheckCmd=%v Base=%v — the field was swallowed and the policy is not what was written", - field, pol[0].CheckCmd, pol[0].Base) - } - if len(bad) == 0 { - t.Fatalf("%q after cmd= was accepted silently; the doc promises cmd parsing fails loudly", field) - } - }) - } -} - // TestRunnerRefusesAPolicyWithNoCommand is R6. // // THE DEFECT: ParsePolicies refuses an entry with no cmd=, but a Policy built @@ -96,12 +29,12 @@ func TestFieldAfterCmdIsReportedNotSwallowed(t *testing.T) { func TestRunnerRefusesAPolicyWithNoCommand(t *testing.T) { for _, tc := range []struct { name string - cmd []string + cmd string }{ - {"nil", nil}, - {"empty slice", []string{}}, - {"one empty string", []string{""}}, - {"whitespace only", []string{" ", "\t"}}, + {"empty", ""}, + {"spaces", " "}, + {"a tab", "\t"}, + {"a newline", "\n"}, } { t.Run(tc.name, func(t *testing.T) { store, err := OpenStore(filepath.Join(t.TempDir(), "g.db")) @@ -132,50 +65,6 @@ func TestRunnerRefusesAPolicyWithNoCommand(t *testing.T) { } } -// TestTimeoutIsBoundedRatherThanOverflowing is R7. -// -// THE DEFECT: a huge timeout= parsed fine with strconv.Atoi, and then -// time.Duration(n)*time.Second overflowed int64 into a NEGATIVE duration. The -// sandbox turns any deadline <= 0 into its own 60s default — which is the -// precise outcome DefaultGateTimeout's comment says "permanently blocks merge -// on any real-world command" — and nothing was reported. -func TestTimeoutIsBoundedRatherThanOverflowing(t *testing.T) { - for _, tc := range []struct { - name string - value string - want bool // accepted? - }{ - {"overflowing", "9223372036854775807", false}, - {"absurd but parseable", "999999999999", false}, - {"negative", "-1", false}, - {"not a number", "soon", false}, - {"the maximum", "86400", true}, - {"an ordinary ten minutes", "600", true}, - } { - t.Run(tc.name, func(t *testing.T) { - pol, bad := ParsePolicies("repo=o/r,timeout=" + tc.value + ",cmd=go test ./...") - if tc.want { - if len(pol) != 1 { - t.Fatalf("a legitimate timeout=%s was refused (bad=%v)", tc.value, bad) - } - // The effective duration must be positive, or the sandbox - // silently substitutes its own 60s and we are back to the bug. - if d := time.Duration(pol[0].TimeoutS) * time.Second; d <= 0 { - t.Errorf("timeout=%s yields a non-positive duration %v", tc.value, d) - } - return - } - if len(pol) != 0 { - d := time.Duration(pol[0].TimeoutS) * time.Second - t.Errorf("accepted timeout=%s, giving duration %v — a value <= 0 becomes the sandbox's 60s default and blocks merges", tc.value, d) - } - if len(bad) == 0 { - t.Errorf("timeout=%s was rejected silently, with nothing for the operator to read", tc.value) - } - }) - } -} - // --- FAMILY TWO: the verdict was computed and not delivered --- // TestStoreFailureNeverPostsSuccess is R1. @@ -203,7 +92,7 @@ func TestStoreFailureNeverPostsSuccess(t *testing.T) { Now: func() time.Time { return time.Unix(0, 0) }, } _ = r.Run(context.Background(), "http://forge/o/r", - Policy{Repo: "o/r", Context: "corral/gate", CheckCmd: []string{"true"}}, PRRef{Number: 1, HeadSHA: "abc"}) + Policy{Repo: "o/r", Context: "corral/gate", CheckCmd: "true"}, PRRef{Number: 1, HeadSHA: "abc"}) if slices.Contains(status.states, "success") { t.Fatal("posted success while the run could not be recorded — the documented fail-closed invariant says otherwise, and the poller would re-certify this head forever") @@ -228,8 +117,8 @@ func TestDedupeIsPerStatusContext(t *testing.T) { ran := map[string]int{} p := &Poller{ Policies: []Policy{ - {Repo: "o/r", Base: []string{"main"}, Context: "corral/lint", CheckCmd: []string{"lint"}}, - {Repo: "o/r", Base: []string{"main"}, Context: "corral/test", CheckCmd: []string{"test"}}, + {Repo: "o/r", Base: []string{"main"}, Context: "corral/lint", CheckCmd: "lint"}, + {Repo: "o/r", Base: []string{"main"}, Context: "corral/test", CheckCmd: "test"}, }, List: &fakeLister{prs: []PRRef{{Number: 1, HeadSHA: "abc", Base: "main"}}}, Store: store, @@ -285,7 +174,7 @@ func TestAnUndeliveredVerdictIsRetried(t *testing.T) { runs := 0 p := &Poller{ - Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}}, + Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}}, List: &fakeLister{prs: []PRRef{{Number: 1, HeadSHA: "abc", Base: "main"}}}, Store: store, Run: func(ctx context.Context, repoURL string, pol Policy, pr PRRef) error { @@ -338,3 +227,12 @@ func TestMarkPostedIsNotVacuous(t *testing.T) { t.Error("a row appeared under a context that was never saved") } } + +// The semicolon and stray-field tests that stood here policed an ambiguity +// that NO LONGER EXISTS: policies now live one per CORRALAI_GATE_POLICY_ +// variable, so the entry separator is the operating system's and a ';' inside +// a command cannot collide with it. Three guards were written against that +// collision and a cold reviewer defeated all three (rounds two, three and +// four). The fourth answer was to remove the ambiguity rather than police it. +// The replacement tests are in config_test.go, and they assert the inverse: +// a command containing ';' must now survive VERBATIM. diff --git a/internal/gate/config.go b/internal/gate/config.go index fc3d7f0a..44bb081a 100644 --- a/internal/gate/config.go +++ b/internal/gate/config.go @@ -4,68 +4,53 @@ package gate import ( "regexp" + "sort" "strconv" "strings" ) -// ParsePolicies parses CORRALAI_GATE_POLICIES: semicolon-separated policy -// entries, each a comma-separated list of key=value pairs — -// "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./...". An -// empty raw string yields (nil, nil): the merge-gate feature's off switch. A -// malformed entry (missing the required repo= or cmd=) is skipped and -// reported in bad rather than aborting the whole parse — one bad entry in -// an operator's env var must not silently disable every other repo's gate -// (degrade-never-block, same directive as the poller). +// PolicyEnvPrefix is where a merge-gate policy lives: ONE policy per +// environment variable, named CORRALAI_GATE_POLICY_, mirroring the +// CORRALAI_AGENT_ convention the review seats already use. // -// cmd= MUST be the LAST field in an entry. Everything from "cmd=" to the end -// of the entry is the command VERBATIM — commas included, never -// comma-split — so a check like "go test -run A,B ./..." isn't silently -// truncated to "go test -run A" (a truncated cmd is a WEAKER command that -// could exit 0 and post a wrongful "success"; this is the one -// operator-reachable path that could manufacture a green gate, so cmd -// parsing fails loudly rather than guessing). An entry with no cmd= at all -// is malformed (reported in bad), never silently accepted with an -// empty/default command. +// WHY ONE VARIABLE PER POLICY. The previous format packed every policy into a +// single ";"-separated CORRALAI_GATE_POLICIES, and a command is allowed to +// contain a ";" — so the entry separator and the command's own syntax +// collided. Three separate guards were written against that collision and a +// cold reviewer defeated all three: // -// base= may repeat within an entry (space or "|"-joined isn't supported — -// only the last base= wins per entry today; multi-base policies are -// expressed as multiple semicolon-separated entries sharing a repo). An -// omitted base= means "all bases" (Policy.Base == nil). An omitted -// context defaults to "corral/gate". An omitted net= defaults to false -// (no network — fail-closed default, matching the runner's own posture). -// An omitted (or non-numeric) timeout= leaves Policy.TimeoutS at 0, which -// the runner turns into DefaultGateTimeout. +// round two split on ';' before isolating cmd=, so "go vet ./... ; go test +// ./..." was accepted as the WEAKER "go vet ./..." +// round three keyed the guard on the absence of '=', defeated by any command +// containing a flag like -tags=integration +// round four keyed it on the absence of "repo=", defeated by a command +// containing "repo=" incidentally: "true; env repo=x false" +// +// Every one of those let a truncated, weaker command post a wrongful success, +// which is the worst thing this package can do. The fourth attempt is not +// another guard. Giving each policy its own variable means the OPERATING +// SYSTEM supplies the separator, and a ";" inside a command can no longer +// collide with anything. The ambiguity is removed rather than policed. +const PolicyEnvPrefix = "CORRALAI_GATE_POLICY_" + +// LegacyPolicyEnv is the retired single-variable format. It is not parsed — +// it is REFUSED, loudly, by ParsePolicyEnv. Supporting both would mean two +// parsers for one rule, and a rule living at two doors is the defect this +// package has produced fifteen findings' worth of; see PolicyEnvPrefix. +const LegacyPolicyEnv = "CORRALAI_GATE_POLICIES" + // maxGateTimeoutS bounds timeout= well below the point where // time.Duration(n)*time.Second overflows int64 (~292 years), while leaving // room for any real check: 24 hours. const maxGateTimeoutS = 24 * 60 * 60 -// policyFields are the keys ParsePolicies understands. strayFieldAfterCmd -// derives its check from this list rather than repeating it, so a key added -// here is covered without a second edit. +// policyFields are the keys a policy understands. strayFieldAfterCmd derives +// its check from this list rather than repeating it, so a key added here is +// covered without a second edit. var policyFields = []string{"repo", "base", "context", "net", "timeout"} -// strayFieldAfterCmd returns the name of a known policy field that appears -// after cmd= (where it would be swallowed into the command verbatim), or "". -// -// It tolerates whitespace on BOTH sides of the comma and around the '='. The -// first version matched the single spelling ","+f+"=" exactly, so the spaced -// form an operator is at least as likely to write — "cmd=make test, base=release" -// — sailed through and silently widened the policy to every base branch, which -// is the very defect the check was added for. One spelling guarded, the other -// left open. (Cold review round three, 2026-09-12, R2 — reproduced, high, -// against my own fix for the round-two R8.) -func strayFieldAfterCmd(cmdVal string) string { - for _, f := range policyFields { - if strayFieldRE[f].MatchString(cmdVal) { - return f - } - } - return "" -} - -// strayFieldRE is derived from policyFields, so a key added there is covered -// without a second edit — the rule this repo keeps relearning. +// strayFieldRE is derived from policyFields and tolerates whitespace on both +// sides of the comma and the '='. var strayFieldRE = func() map[string]*regexp.Regexp { m := make(map[string]*regexp.Regexp, len(policyFields)) for _, f := range policyFields { @@ -74,160 +59,171 @@ var strayFieldRE = func() map[string]*regexp.Regexp { return m }() -func ParsePolicies(raw string) (policies []Policy, bad []string) { - raw = strings.TrimSpace(raw) - if raw == "" { - return nil, nil +// ParsePolicyEnv reads every CORRALAI_GATE_POLICY_ out of environ (the +// os.Environ() form, "KEY=VALUE") and returns the policies in a DETERMINISTIC +// order — sorted by variable name. +// +// The sort is not cosmetic. Ranging a map and taking what comes is the exact +// non-determinism that produced two findings in this repository within a day +// (an arbitrary Rekor entry chosen on a UUID miss, and the same again in the +// logger). Policies decide which check runs against a pull request; they are +// not allowed to arrive in a different order on different runs. +// +// A malformed policy is reported in bad and SKIPPED, never fatal: one bad +// variable must not silently disable every other repo's gate +// (degrade-never-block, the same directive the poller follows). Because each +// policy now has its own variable, a bad one is isolated by construction — +// under the old format a single stray character could take its neighbours +// with it. +func ParsePolicyEnv(environ []string) (policies []Policy, bad []string) { + if legacy := envValue(environ, LegacyPolicyEnv); legacy != "" { + bad = append(bad, LegacyPolicyEnv+" is no longer supported and was IGNORED — its ';' separator collided with commands containing ';', which silently ran a weaker check and posted success. Set one "+PolicyEnvPrefix+" per policy instead; each value is the same text between the old ';' separators") } - // THE SEMICOLON IS THE SAME DEFECT THE COMMA NOTE ABOVE GUARDS AGAINST. - // cmd= runs to the end of its ENTRY, and entries are ';'-separated, so a - // command containing ';' — "cmd=go vet ./... ; go test ./..." under sh -c - // — was split here BEFORE cmd= was isolated: the first half was accepted - // as a complete policy carrying the WEAKER command, and only the orphaned - // tail was reported. That is precisely the "truncated cmd manufactures a - // green gate" path this file claims to prevent, arriving through the other - // delimiter. (Cold review 2026-09-12, R2 — reproduced, high.) - // - // It is refused rather than repaired. Re-joining the fragments would be - // guessing: an operator who simply forgot repo= on a second entry would - // have it silently welded onto the previous command. cmd parsing fails - // loudly, so both fragments are reported and NEITHER policy is accepted. - frags := strings.Split(raw, ";") - // prevPolicy is the index in policies produced by the PREVIOUS fragment, - // or -1. Tracked by position so a truncated entry is identified exactly. - prevPolicy := -1 - for i, entry := range frags { - entry = strings.TrimSpace(entry) - if entry == "" { + + type named struct{ name, val string } + var found []named + for _, kv := range environ { + key, val, ok := strings.Cut(kv, "=") + if !ok || !strings.HasPrefix(key, PolicyEnvPrefix) { continue } - // A fragment with no key=value at all cannot be a policy entry; if the - // one before it declared a cmd=, this is that command's missing tail. - // The test for "this fragment is a continuation, not a policy" is - // whether it declares repo= — the field a policy REQUIRES — and not - // whether it contains an '=' anywhere. - // - // The first version of this guard asked `!strings.Contains(entry, "=")`, - // which any ordinary second command defeats: "go test -tags=integration - // ./..." contains '=', so the fragment looked like a policy entry, the - // truncation went unnoticed, and the weaker command was accepted again. - // A guard keyed on an INCIDENTAL character instead of the declared - // schema is the same mistake as enumerating where a property holds. - // (Cold review round three, 2026-09-12, R1 — reproduced, high, against - // my own fix for the round-two R2.) - if i > 0 && !strings.Contains(entry, "repo=") && strings.Contains(frags[i-1], "cmd=") { - bad = append(bad, entry+" (a ';' inside cmd= truncated the previous entry's command — the entry before this one was NOT applied; remove the ';' or express the steps as one command)") - // Drop the truncated policy, identified by INDEX rather than by - // matching its repo string: `strings.Contains(frags[i-1], repo)` - // was another guess, and it failed whenever two entries shared a - // repo or a repo name appeared inside a command. - if prevPolicy >= 0 && prevPolicy == len(policies)-1 { - bad = append(bad, strings.TrimSpace(frags[i-1])+" (command truncated at ';')") - policies = policies[:prevPolicy] - } - prevPolicy = -1 + name := strings.TrimPrefix(key, PolicyEnvPrefix) + if name == "" { + bad = append(bad, key+" has no name after the prefix") continue } + found = append(found, named{name, val}) + } + sort.Slice(found, func(i, j int) bool { return found[i].name < found[j].name }) - // cmd= must be the last field: split the entry at "cmd=" so the - // tail (the command) is captured verbatim, commas and all, instead - // of being torn apart by the generic comma-split below. - var head, cmdVal string - cmdSeen := false - switch { - case strings.HasPrefix(entry, "cmd="): - cmdVal = entry[len("cmd="):] - cmdSeen = true - case strings.Contains(entry, ",cmd="): - idx := strings.Index(entry, ",cmd=") - head = entry[:idx] - cmdVal = entry[idx+len(",cmd="):] - cmdSeen = true + for _, f := range found { + pol, reason := ParsePolicy(f.val) + if reason != "" { + bad = append(bad, PolicyEnvPrefix+f.name+": "+reason) + continue } - cmdVal = strings.TrimSpace(cmdVal) + policies = append(policies, pol) + } + return policies, bad +} - pol := Policy{Context: "corral/gate"} - var repoSeen bool - var badField string - if cmdSeen { - // A POLICY FIELD PLACED AFTER cmd= was swallowed into the command - // rather than reported: "repo=o/r,cmd=make test,base=release" was - // accepted with CheckCmd ["make","test,base=release"] AND Base nil, - // which gates every base instead of one — a wider policy than the - // operator wrote, silently, with nothing in bad. The doc says cmd= - // must be LAST and that parsing fails loudly; now it does. - // (Cold review 2026-09-12, R8.) - if stray := strayFieldAfterCmd(cmdVal); stray != "" { - bad = append(bad, entry+" ("+stray+"= appears after cmd=, which takes the rest of the entry verbatim; move it before cmd=)") - prevPolicy = -1 - continue - } - if fields := strings.Fields(cmdVal); len(fields) > 0 { - pol.CheckCmd = fields - } else { - cmdSeen = false // "cmd=" with an empty/whitespace-only tail is not a real command - } +// envValue returns the value of key in an os.Environ()-shaped slice, or "". +func envValue(environ []string, key string) string { + for _, kv := range environ { + if k, v, ok := strings.Cut(kv, "="); ok && k == key { + return v } + } + return "" +} + +// ParsePolicy parses ONE policy value: a comma-separated list of key=value +// pairs — "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." +// — returning a non-empty reason when it is malformed. +// +// cmd= MUST BE LAST, and everything after it is the command VERBATIM to the +// end of the value: commas, semicolons, quotes, newlines and all. There is no +// entry separator left to collide with, so the command needs no escaping and +// corral needs no heuristic to find its end. +// +// A field written after cmd= would be swallowed into the command — which once +// produced a policy gating EVERY base branch when the operator had named one — +// so it is reported rather than absorbed. An empty or whitespace-only command +// is malformed: it would reach the jail as `sh -c ""`, exit 0, and post a +// success for a check that ran nothing. +// +// base= may repeat; only the last wins. An omitted base= means "all bases" +// (Policy.Base == nil). An omitted context= is defaulted at the door that ACTS +// on the policy (Policy.normalized), never here, so the forge and the store +// can never disagree about which check spoke. An omitted net= defaults to +// false — no network, matching the runner's fail-closed posture. +func ParsePolicy(raw string) (Policy, string) { + raw = strings.TrimSpace(raw) + if raw == "" { + return Policy{}, "empty" + } - for _, kv := range strings.Split(head, ",") { - kv = strings.TrimSpace(kv) - if kv == "" { - continue + // Split the value at cmd= so the command is captured whole. + var head, cmdVal string + cmdSeen := false + switch { + case strings.HasPrefix(raw, "cmd="): + cmdVal, cmdSeen = raw[len("cmd="):], true + case strings.Contains(raw, ",cmd="): + i := strings.Index(raw, ",cmd=") + head, cmdVal, cmdSeen = raw[:i], raw[i+len(",cmd="):], true + } + if !cmdSeen { + return Policy{}, "no cmd= (a policy with no command would report a result for a check that never ran)" + } + if stray := strayFieldAfterCmd(cmdVal); stray != "" { + return Policy{}, stray + "= appears after cmd=, which takes the rest of the value verbatim; move it before cmd=" + } + // TrimSpace, NOT strings.Fields. Splitting the command into fields and + // rejoining them with spaces destroyed newlines and would have mangled + // quoted arguments: "true # comment\nfalse" collapsed onto one line and + // the failing step vanished behind the comment. The command is one string + // from here to the jail. + cmd := strings.TrimSpace(cmdVal) + if cmd == "" { + return Policy{}, "cmd= is empty" + } + + pol := Policy{CheckCmd: cmd} + var repoSeen bool + for _, kv := range strings.Split(head, ",") { + kv = strings.TrimSpace(kv) + if kv == "" { + continue + } + key, val, ok := strings.Cut(kv, "=") + if !ok { + return Policy{}, "field " + strconv.Quote(kv) + " is not key=value" + } + key, val = strings.TrimSpace(key), strings.TrimSpace(val) + switch key { + case "repo": + pol.Repo = val + repoSeen = val != "" + case "base": + if val != "" { + pol.Base = []string{val} } - key, val, ok := strings.Cut(kv, "=") - if !ok { - continue + case "context": + if val != "" { + pol.Context = val } - key = strings.TrimSpace(key) - val = strings.TrimSpace(val) - switch key { - case "repo": - pol.Repo = val - repoSeen = val != "" - case "base": - if val != "" { - pol.Base = []string{val} - } - case "context": - if val != "" { - pol.Context = val - } - case "net": - pol.AllowNet = val == "true" || val == "1" - case "timeout": - // A huge timeout= parsed fine and then OVERFLOWED in - // time.Duration(n)*time.Second to a negative duration, which - // the sandbox turns into its own 60s default — the exact - // outcome DefaultGateTimeout's comment says blocks merges, - // reached silently. Bound it and say so. - // (Cold review 2026-09-12, R7.) - if n, err := strconv.Atoi(val); err == nil { - switch { - case n < 0: - badField = "timeout=" + val + " is negative" - case n > maxGateTimeoutS: - badField = "timeout=" + val + "s exceeds the maximum " + strconv.Itoa(maxGateTimeoutS) + "s" - default: - pol.TimeoutS = n - } - } else if val != "" { - badField = "timeout=" + val + " is not a number" - } + case "net": + pol.AllowNet = val == "true" || val == "1" + case "timeout": + n, err := strconv.Atoi(val) + switch { + case err != nil: + return Policy{}, "timeout=" + val + " is not a number" + case n < 0: + return Policy{}, "timeout=" + val + " is negative" + case n > maxGateTimeoutS: + return Policy{}, "timeout=" + val + "s exceeds the maximum " + strconv.Itoa(maxGateTimeoutS) + "s" + default: + pol.TimeoutS = n } + default: + return Policy{}, "unknown field " + strconv.Quote(key) + " (known: " + strings.Join(policyFields, ", ") + ", cmd)" } + } + if !repoSeen { + return Policy{}, "no repo=" + } + return pol, "" +} - if badField != "" { - bad = append(bad, entry+" ("+badField+")") - prevPolicy = -1 - continue - } - if !repoSeen || !cmdSeen { - bad = append(bad, entry) - prevPolicy = -1 - continue +// strayFieldAfterCmd returns the name of a known policy field that appears +// after cmd= (where it would be swallowed into the command verbatim), or "". +func strayFieldAfterCmd(cmdVal string) string { + for _, f := range policyFields { + if strayFieldRE[f].MatchString(cmdVal) { + return f } - prevPolicy = len(policies) - policies = append(policies, pol) } - return policies, bad + return "" } diff --git a/internal/gate/config_test.go b/internal/gate/config_test.go index 57e9cb41..176eef78 100644 --- a/internal/gate/config_test.go +++ b/internal/gate/config_test.go @@ -3,143 +3,189 @@ package gate import ( - "reflect" "strings" "testing" ) -func TestParsePoliciesSingleEntry(t *testing.T) { - pols, bad := ParsePolicies("repo=owner/name,base=main,net=false,cmd=go test ./...") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) - } - want := []Policy{{ - Repo: "owner/name", - Base: []string{"main"}, - Context: "corral/gate", - CheckCmd: []string{"go", "test", "./..."}, - AllowNet: false, - }} - if !reflect.DeepEqual(pols, want) { - t.Fatalf("got %+v, want %+v", pols, want) - } -} +// These tests cover the policy format adopted on 2026-09-13: ONE policy per +// CORRALAI_GATE_POLICY_ variable, replacing the single ";"-separated +// CORRALAI_GATE_POLICIES. The change was made because three successive guards +// against ";"-in-a-command were each defeated by a cold reviewer, every time +// letting a truncated, weaker command post a wrongful success. +// +// So the headline test is the inverse of the ones it replaces: a semicolon in +// a command must now survive VERBATIM, because nothing is competing for it. -func TestParsePoliciesMultipleEntriesSemicolonSeparated(t *testing.T) { - pols, bad := ParsePolicies("repo=o/a,base=main,cmd=make test;repo=o/b,base=main,net=true,cmd=make check") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) - } - if len(pols) != 2 { - t.Fatalf("got %d policies, want 2: %+v", len(pols), pols) - } - if pols[0].Repo != "o/a" || pols[0].AllowNet { - t.Fatalf("policy 0 wrong: %+v", pols[0]) - } - if pols[1].Repo != "o/b" || !pols[1].AllowNet { - t.Fatalf("policy 1 wrong: %+v", pols[1]) +func TestACommandMayContainASemicolon(t *testing.T) { + for _, cmd := range []string{ + "go vet ./... ; go test ./...", + "true; env repo=x false", // round four's defeat of guard three + "go test -tags=integration ./...", // round three's defeat of guard two + "make a; make b; make c", + "sh -c 'a; b'", + } { + t.Run(cmd, func(t *testing.T) { + pol, reason := ParsePolicy("repo=o/r,cmd=" + cmd) + if reason != "" { + t.Fatalf("refused a legitimate command: %s", reason) + } + if pol.CheckCmd != cmd { + t.Errorf("CheckCmd = %q, want the command verbatim %q — a truncated command is a WEAKER command that can exit 0 and post success", pol.CheckCmd, cmd) + } + }) } } -func TestParsePoliciesEmptyStringYieldsNoPolicies(t *testing.T) { - pols, bad := ParsePolicies("") - if len(pols) != 0 || len(bad) != 0 { - t.Fatalf("got pols=%v bad=%v, want both empty (feature off)", pols, bad) +// TestACommandKeepsItsNewlinesAndQuoting is round four's gate R2, which was +// pre-existing: the command was strings.Fields-split and rejoined with spaces +// at two call sites, so "true # comment\nfalse" collapsed onto one line and +// the failing step vanished behind the comment. Quoted arguments containing +// spaces would have been mangled the same way. +func TestACommandKeepsItsNewlinesAndQuoting(t *testing.T) { + for _, cmd := range []string{ + "true # comment\nfalse", + "go test -run 'A B' ./...", + "a\nb\nc", + `echo "two spaces"`, + } { + pol, reason := ParsePolicy("repo=o/r,cmd=" + cmd) + if reason != "" { + t.Fatalf("refused %q: %s", cmd, reason) + } + if pol.CheckCmd != cmd { + t.Errorf("CheckCmd = %q, want %q — rejoining a split command drops newlines, and a commented-out failing step passes", pol.CheckCmd, cmd) + } } } -func TestParsePoliciesMissingRepoIsBadNotFatal(t *testing.T) { - pols, bad := ParsePolicies("base=main,cmd=true;repo=o/ok,base=main,cmd=true") - if len(pols) != 1 || pols[0].Repo != "o/ok" { - t.Fatalf("expected the one well-formed entry to survive: %+v", pols) +func TestParsePolicyFields(t *testing.T) { + pol, reason := ParsePolicy("repo=o/r,base=main,context=corral/lint,net=true,timeout=900,cmd=go test ./...") + if reason != "" { + t.Fatalf("reason = %q, want none", reason) } - if len(bad) != 1 { - t.Fatalf("expected the malformed entry reported, got %v", bad) + if pol.Repo != "o/r" || len(pol.Base) != 1 || pol.Base[0] != "main" { + t.Errorf("repo/base = %q/%v", pol.Repo, pol.Base) } -} - -func TestParsePoliciesMissingCmdIsBadNotFatal(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,base=main") - if len(pols) != 0 { - t.Fatalf("expected no policies from an entry with no cmd, got %+v", pols) + if pol.Context != "corral/lint" || !pol.AllowNet || pol.TimeoutS != 900 { + t.Errorf("context/net/timeout = %q/%v/%d", pol.Context, pol.AllowNet, pol.TimeoutS) } - if len(bad) != 1 { - t.Fatalf("expected the malformed entry reported, got %v", bad) + if pol.CheckCmd != "go test ./..." { + t.Errorf("CheckCmd = %q", pol.CheckCmd) } } -func TestParsePoliciesDefaultsBaseAndContext(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,cmd=true") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) +// TestParsePolicyDefaults: context is deliberately NOT defaulted here. It is +// defaulted by Policy.normalized at the door that acts on the policy, because +// defaulting it in two places is how the forge and the store came to disagree +// about which check had spoken. +func TestParsePolicyDefaults(t *testing.T) { + pol, reason := ParsePolicy("repo=o/r,cmd=make test") + if reason != "" { + t.Fatalf("reason = %q", reason) + } + if pol.Base != nil { + t.Errorf("Base = %v, want nil (all bases)", pol.Base) } - if len(pols) != 1 { - t.Fatalf("got %d policies, want 1", len(pols)) + if pol.AllowNet { + t.Error("AllowNet defaulted true — the fail-closed default is no network") } - if len(pols[0].Base) != 0 { - t.Fatalf("expected no base restriction (all bases) when base= is omitted, got %v", pols[0].Base) + if pol.TimeoutS != 0 { + t.Errorf("TimeoutS = %d, want 0 so the runner applies DefaultGateTimeout", pol.TimeoutS) } - if pols[0].Context != "corral/gate" { - t.Fatalf("expected default context 'corral/gate', got %q", pols[0].Context) + if got := pol.normalized().Context; got != DefaultStatusContext { + t.Errorf("normalized context = %q, want %q", got, DefaultStatusContext) } } -// TestParsePoliciesTimeoutParsesToSeconds: an explicit timeout= field parses -// into Policy.TimeoutS as an int number of seconds. -func TestParsePoliciesTimeoutParsesToSeconds(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,base=main,timeout=120,cmd=true") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) - } - if len(pols) != 1 || pols[0].TimeoutS != 120 { - t.Fatalf("got %+v, want TimeoutS=120", pols) +func TestParsePolicyRefusals(t *testing.T) { + for _, tc := range []struct{ name, raw, wantSubstr string }{ + {"empty", "", "empty"}, + {"no repo", "cmd=go test ./...", "repo"}, + {"no cmd", "repo=o/r,base=main", "cmd"}, + {"empty cmd", "repo=o/r,cmd= ", "empty"}, + {"field after cmd", "repo=o/r,cmd=make test,base=release", "after cmd="}, + {"field after cmd, spaced", "repo=o/r,cmd=make test, base=release", "after cmd="}, + {"unknown field", "repo=o/r,mode=fast,cmd=make test", "unknown field"}, + {"timeout not a number", "repo=o/r,timeout=soon,cmd=make test", "not a number"}, + {"timeout negative", "repo=o/r,timeout=-1,cmd=make test", "negative"}, + {"timeout overflows", "repo=o/r,timeout=9223372036854775807,cmd=make test", "exceeds"}, + {"not key=value", "repo=o/r,justaword,cmd=make test", "not key=value"}, + } { + t.Run(tc.name, func(t *testing.T) { + pol, reason := ParsePolicy(tc.raw) + if reason == "" { + t.Fatalf("accepted %q, giving CheckCmd %q", tc.raw, pol.CheckCmd) + } + if !strings.Contains(reason, tc.wantSubstr) { + t.Errorf("reason %q does not mention %q, so the operator cannot act on it", reason, tc.wantSubstr) + } + }) } } -// TestParsePoliciesOmittedTimeoutIsZero: no timeout= field means TimeoutS -// stays 0 — the runner is the one that turns 0 into DefaultGateTimeout, not -// the parser. -func TestParsePoliciesOmittedTimeoutIsZero(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,cmd=true") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) +// TestParsePolicyEnvIsDeterministic: policies decide which check runs against +// a pull request, so they may not arrive in a different order on different +// runs. Ranging a map and taking what comes is the exact non-determinism that +// produced two findings in this repository within a day. +func TestParsePolicyEnvIsDeterministic(t *testing.T) { + env := []string{ + PolicyEnvPrefix + "ZEBRA=repo=o/z,cmd=z", + "PATH=/usr/bin", + PolicyEnvPrefix + "ALPHA=repo=o/a,cmd=a", + PolicyEnvPrefix + "MIDDLE=repo=o/m,cmd=m", + } + for i := 0; i < 20; i++ { + pol, bad := ParsePolicyEnv(env) + if len(bad) != 0 { + t.Fatalf("bad = %v", bad) + } + if len(pol) != 3 { + t.Fatalf("policies = %d, want 3", len(pol)) + } + if pol[0].Repo != "o/a" || pol[1].Repo != "o/m" || pol[2].Repo != "o/z" { + t.Fatalf("order = %s, %s, %s — want sorted by variable name", pol[0].Repo, pol[1].Repo, pol[2].Repo) + } } - if len(pols) != 1 || pols[0].TimeoutS != 0 { - t.Fatalf("got %+v, want TimeoutS=0", pols) +} + +// TestOneBadPolicyDoesNotTakeTheOthersDown — degrade, never block. Under the +// old single-variable format a stray character could take a neighbour with it; +// now each policy is isolated by construction. +func TestOneBadPolicyDoesNotTakeTheOthersDown(t *testing.T) { + pol, bad := ParsePolicyEnv([]string{ + PolicyEnvPrefix + "GOOD=repo=o/r,cmd=go test ./...", + PolicyEnvPrefix + "BROKEN=cmd=no repo here", + }) + if len(pol) != 1 || pol[0].Repo != "o/r" { + t.Fatalf("policies = %v, want the one good policy to survive", pol) + } + if len(bad) != 1 || !strings.Contains(bad[0], "BROKEN") { + t.Errorf("bad = %v, want it to name the offending variable", bad) } } -// TestParsePoliciesCmdWithCommaIsPreservedVerbatim is the fix-#2 regression: -// a cmd containing a comma (e.g. "go test -run A,B ./...") must NOT be -// silently truncated at the first comma — that would run a weaker command -// than the operator declared and could manufacture a wrongful "success". -// cmd= must be the LAST field in an entry; everything after "cmd=" is the -// command verbatim, commas included. -func TestParsePoliciesCmdWithCommaIsPreservedVerbatim(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,base=main,cmd=go test -run A,B ./...") - if len(bad) != 0 { - t.Fatalf("unexpected bad entries: %v", bad) - } - if len(pols) != 1 { - t.Fatalf("got %d policies, want 1: %+v", len(pols), pols) - } - got := strings.Join(pols[0].CheckCmd, " ") - want := "go test -run A,B ./..." - if got != want { - t.Fatalf("cmd = %q, want %q (comma must survive, never truncated)", got, want) +// TestLegacyVariableIsRefusedLoudly: the retired format is not parsed and not +// silently ignored either. Silently ignoring it would turn a configured gate +// into no gate at all, which is the failure mode this package exists to +// prevent. +func TestLegacyVariableIsRefusedLoudly(t *testing.T) { + pol, bad := ParsePolicyEnv([]string{ + LegacyPolicyEnv + "=repo=o/r,cmd=go test ./...", + }) + if len(pol) != 0 { + t.Fatalf("parsed %d policies from the retired variable — it must not be honored", len(pol)) + } + if len(bad) == 0 { + t.Fatal("the retired variable was ignored SILENTLY; an operator would believe their gate was running") + } + if !strings.Contains(bad[0], PolicyEnvPrefix) { + t.Errorf("the message %q does not name the replacement", bad[0]) } } -// TestParsePoliciesCmdMustBeLastEntryHasNoCmdFieldIsBad: an entry with no -// cmd= at all is rejected loudly (bad), never silently accepted with an -// empty/default command — a missing check must never look like a passing -// gate. -func TestParsePoliciesCmdMustBeLastEntryHasNoCmdFieldIsBad(t *testing.T) { - pols, bad := ParsePolicies("repo=o/r,base=main,net=true") - if len(pols) != 0 { - t.Fatalf("expected no policies from an entry with no cmd=, got %+v", pols) - } - if len(bad) != 1 { - t.Fatalf("expected the malformed (cmd-less) entry reported, got %v", bad) +func TestNoPolicyVariablesMeansFeatureOff(t *testing.T) { + pol, bad := ParsePolicyEnv([]string{"PATH=/usr/bin", "HOME=/root"}) + if len(pol) != 0 || len(bad) != 0 { + t.Errorf("policies=%v bad=%v, want both empty (the off switch)", pol, bad) } } diff --git a/internal/gate/poller_test.go b/internal/gate/poller_test.go index 919b6a49..30988faa 100644 --- a/internal/gate/poller_test.go +++ b/internal/gate/poller_test.go @@ -35,7 +35,7 @@ func TestPollerGatesNewHeadOnce(t *testing.T) { var runs int p := &Poller{ - Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}}, + Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}}, List: &fakeLister{prs: []PRRef{{Number: 1, HeadSHA: "abc", Base: "main"}}}, Store: store, // The fake stands in for a runner that ran AND delivered its status, @@ -72,8 +72,8 @@ func TestPollerRunsEachNewHeadAcrossPolicies(t *testing.T) { var ran []string p := &Poller{ Policies: []Policy{ - {Repo: "o/a", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}, - {Repo: "o/b", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}, + {Repo: "o/a", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}, + {Repo: "o/b", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}, }, // Route by policy repo via a small dispatcher list, since Poller.List // is a single PRLister shared across policies in production (the @@ -127,7 +127,7 @@ func TestPollerListErrorLoggedNeverCrashes(t *testing.T) { var runs int p := &Poller{ - Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}}, + Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}}, List: &fakeLister{err: errors.New("forge unavailable")}, Store: store, Run: func(ctx context.Context, repoURL string, pol Policy, pr PRRef) error { @@ -152,7 +152,7 @@ func TestPollerLoopHonorsCancellation(t *testing.T) { t.Cleanup(func() { _ = store.Close() }) p := &Poller{ - Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"true"}}}, + Policies: []Policy{{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "true"}}, List: &fakeLister{}, Store: store, Run: func(ctx context.Context, repoURL string, pol Policy, pr PRRef) error { return nil }, diff --git a/internal/gate/runner.go b/internal/gate/runner.go index f2b39827..990c1c73 100644 --- a/internal/gate/runner.go +++ b/internal/gate/runner.go @@ -98,7 +98,7 @@ func (r *Runner) Run(ctx context.Context, repoURL string, p Policy, pr PRRef) er // a question nobody asked. The rule belongs at the door that ACTS on the // policy, not only at the one that parses it. // (Cold review 2026-09-12, R6.) - if len(p.CheckCmd) == 0 || strings.TrimSpace(strings.Join(p.CheckCmd, " ")) == "" { + if strings.TrimSpace(p.CheckCmd) == "" { return r.fail(ctx, repoURL, p, pr, target, "error", "policy has no check command — refusing to report a result for a check that would run nothing") } @@ -113,14 +113,14 @@ func (r *Runner) Run(ctx context.Context, repoURL string, p Policy, pr PRRef) er } timeout := p.effectiveTimeout() - exit, output, runErr := r.Jail.Run(ctx, strings.Join(p.CheckCmd, " "), dest, p.AllowNet, timeout) + exit, output, runErr := r.Jail.Run(ctx, p.CheckCmd, dest, p.AllowNet, timeout) if runErr != nil { return r.fail(ctx, repoURL, p, pr, target, "error", "jail: "+runErr.Error()) } sum := sha256.Sum256([]byte(output)) digest := "sha256:" + hex.EncodeToString(sum[:]) - recordID, _, certErr := r.Certify.Certify(ctx, p.Repo, pr.HeadSHA, strings.Join(p.CheckCmd, " "), exit, digest) + recordID, _, certErr := r.Certify.Certify(ctx, p.Repo, pr.HeadSHA, p.CheckCmd, exit, digest) if certErr != nil { return r.fail(ctx, repoURL, p, pr, target, "error", "sign: "+certErr.Error()) } diff --git a/internal/gate/runner_test.go b/internal/gate/runner_test.go index 57cc2b1f..1ffe938b 100644 --- a/internal/gate/runner_test.go +++ b/internal/gate/runner_test.go @@ -69,7 +69,7 @@ func (f *fakeStatusPoster) SetCommitStatus(ctx context.Context, repoURL, sha, co } func testPolicy() Policy { - return Policy{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: []string{"go", "test", "./..."}, AllowNet: false} + return Policy{Repo: "o/r", Base: []string{"main"}, Context: "corral/gate", CheckCmd: "go test ./...", AllowNet: false} } func testPR() PRRef { diff --git a/internal/gate/types.go b/internal/gate/types.go index 48b34978..23644acc 100644 --- a/internal/gate/types.go +++ b/internal/gate/types.go @@ -21,10 +21,17 @@ const DefaultGateTimeout = 600 * time.Second // it. Later tasks (the poller, the runner) consume this; Task 2 only // defines the shape. type Policy struct { - Repo string - Base []string - Context string - CheckCmd []string + Repo string + Base []string + Context string + // CheckCmd is the command as ONE STRING, handed to the jail's `sh -c` + // unchanged. It was once a []string produced by strings.Fields and + // rejoined with spaces at two call sites, which destroyed newlines — so + // "true # comment\nfalse" collapsed onto one line and the failing step + // disappeared behind the comment — and would have mangled any quoted + // argument containing a space. Splitting a shell command and hoping the + // rejoin is faithful is not a thing to do. + CheckCmd string AllowNet bool // TimeoutS is the jail deadline in seconds. 0 (unset) means "use // DefaultGateTimeout" — see Runner.Run, which computes the effective diff --git a/internal/wranglerd/daemon.go b/internal/wranglerd/daemon.go index 78446adc..85657c5d 100644 --- a/internal/wranglerd/daemon.go +++ b/internal/wranglerd/daemon.go @@ -658,14 +658,20 @@ func Run(version string) { log.Printf("verify-gate: NO isolation backend (%v); gated completion falls back to worker-reported executions — set CORRALAI_GATE_EXEC_BACKEND", gerr) } - // Repo gate (merge gate): CORRALAI_GATE_POLICIES declares which repos - // get an independent, jailed check run against every new open-PR head. - // Empty var => feature off (ParsePolicies returns nil, nil). Malformed - // entries are logged and skipped — one bad entry must not take down - // every other repo's gate (degrade-never-block). - gatePolicies, badGatePolicies := gate.ParsePolicies(os.Getenv("CORRALAI_GATE_POLICIES")) + // Repo gate (merge gate): one CORRALAI_GATE_POLICY_ per repo that + // gets an independent, jailed check run against every new open-PR head. + // No such variable => feature off. A malformed policy is logged and + // skipped — one bad variable must not take down every other repo's gate + // (degrade-never-block) — and each policy having its own variable means a + // bad one is isolated by construction. + // + // The retired CORRALAI_GATE_POLICIES is reported here, not parsed: a + // ';' inside a command collided with its ';' separator and silently ran a + // weaker check. See gate.PolicyEnvPrefix for the three guards that failed + // before the format was changed instead. + gatePolicies, badGatePolicies := gate.ParsePolicyEnv(os.Environ()) for _, bad := range badGatePolicies { - log.Printf("gate: malformed CORRALAI_GATE_POLICIES entry (skipped): %q", bad) + log.Printf("gate: policy skipped: %s", bad) } gateDB := env("CORRALAI_GATE_DB", filepath.Join(home, ".claude", "corralai_gate.duckdb")) diff --git a/site/src/content/docs/docs/cli/corral-wrangler.md b/site/src/content/docs/docs/cli/corral-wrangler.md index e112e615..e1327904 100644 --- a/site/src/content/docs/docs/cli/corral-wrangler.md +++ b/site/src/content/docs/docs/cli/corral-wrangler.md @@ -89,11 +89,14 @@ CORRALAI_BRAIN_TOKEN `corral certify`'s bearer token to authenticate to a from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); `corral certify verify` checks the same default unless --rekor-url overrides it -CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; - cmd= MUST be the last field — everything after it is the command verbatim (commas - allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; + cmd= MUST be last — everything after it is the command VERBATIM to the end of the + value: commas, semicolons, quotes and newlines included, so nothing can truncate it + into a weaker command that exits 0 and posts a wrongful success; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; - empty => the repo gate is OFF (no poller starts); GitHub-only for v1 + no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. + Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with + commands containing ";" CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below); diff --git a/site/src/content/docs/docs/cli/corral.md b/site/src/content/docs/docs/cli/corral.md index 1fa78136..0c20e83f 100644 --- a/site/src/content/docs/docs/cli/corral.md +++ b/site/src/content/docs/docs/cli/corral.md @@ -999,11 +999,14 @@ CORRALAI_BRAIN_TOKEN `corral certify`'s bearer token to authenticate to a from CORRALAI_BRAIN_KEY above (that's an Ed25519 IDENTITY SEED, not a bearer token — do not reuse it) CORRALAI_REKOR_URL Sigstore Rekor instance report_build anchors signed build attestations to (default https://rekor.sigstore.dev); `corral certify verify` checks the same default unless --rekor-url overrides it -CORRALAI_GATE_POLICIES repo merge gate: ";"-separated policies "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; - cmd= MUST be the last field — everything after it is the command verbatim (commas - allowed, never split) so "cmd=go test -run A,B ./..." isn't silently truncated; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..."; + cmd= MUST be last — everything after it is the command VERBATIM to the end of the + value: commas, semicolons, quotes and newlines included, so nothing can truncate it + into a weaker command that exits 0 and posts a wrongful success; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; - empty => the repo gate is OFF (no poller starts); GitHub-only for v1 + no such variable => the repo gate is OFF (no poller starts); GitHub-only for v1. + Replaces CORRALAI_GATE_POLICIES, which is REFUSED: its ";" separator collided with + commands containing ";" CORRALAI_GATE_DB repo gate dedupe/index store DuckDB path (default ~/.claude/corralai_gate.duckdb) CORRALAI_GATE_POLL_SECONDS how often (seconds) the repo gate polls covered repos for new PR heads (default 120) CORRALAI_GATE_EXEC_BACKEND / _EXEC_UNSAFE_HOST same jail backend used by the independent verify-gate (see below);