From ea3f4e20dde0457505e9043674d8c00b54e29a7e Mon Sep 17 00:00:00 2001 From: Peter D Bethke Date: Sun, 13 Sep 2026 07:19:24 -0400 Subject: [PATCH 1/2] =?UTF-8?q?gate:=20one=20policy=20per=20env=20var=20?= =?UTF-8?q?=E2=80=94=20remove=20the=20ambiguity=20instead=20of=20policing?= =?UTF-8?q?=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three guards were written against a ';' inside cmd= and a cold reviewer defeated all three, each time letting a truncated, WEAKER command post a wrongful success — the worst thing this package can do: round two split on ';' before isolating cmd=, so "go vet ./... ; go test ./..." was accepted as "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" The fourth answer is not a fourth guard. CORRALAI_GATE_POLICIES is retired and each policy gets its own CORRALAI_GATE_POLICY_, mirroring the existing CORRALAI_AGENT_ convention. The OPERATING SYSTEM supplies the separator, so a ';' in a command cannot collide with anything. The ambiguity is gone rather than policed, and the headline test is now the INVERSE of the ones it replaces: a semicolon in a command must survive verbatim. Also fixed, pre-existing and found in round four: CheckCmd was a []string produced by strings.Fields and rejoined with spaces at two call sites, which destroyed newlines — "true # comment\nfalse" collapsed onto one line and the failing step vanished behind the comment — and would have mangled any quoted argument containing a space. It is one string from the parser to the jail now. WHAT THIS COSTS, recorded because it is not free: - The retired variable is REFUSED, not parsed and not silently ignored. Silently ignoring it would turn a configured gate into no gate at all. Supporting both formats would mean two parsers for one rule, which is the defect class that has produced fifteen findings in this package. - One variable was easier to paste into a CI config than N. - Policy.CheckCmd []string -> string breaks brain Options.GatePolicies callers. - Ordering is now across variables, so ParsePolicyEnv SORTS BY NAME. Ranging a map and taking what comes is the exact non-determinism that produced two findings here in a day; policies decide which check runs against a pull request and may not arrive in a different order on different runs. - No expressiveness is lost. Two things improve: a malformed policy is isolated to its own variable, so degrade-never-block gets easier, and commands with newlines or quoting now work. Four negative controls, all firing. The harness gained a third invalidity check: a -run pattern that matches NO TESTS now reports as invalid rather than as a pass. It reported the semicolon control vacuous when the pattern was simply misspelled — the third time in this session my control harness, rather than the control, was the broken part. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V --- ROADMAP.md | 2 +- cmd/corral/main.go | 2 +- docs/cli/corral-wrangler.md | 2 +- docs/cli/corral.md | 2 +- internal/brain/gate.go | 6 +- internal/brain/gate_test.go | 2 +- internal/brain/identity.go | 2 +- internal/gate/coldreview3_test.go | 92 +---- internal/gate/coldreview_test.go | 138 +------ internal/gate/config.go | 372 +++++++++--------- internal/gate/config_test.go | 256 +++++++----- internal/gate/poller_test.go | 10 +- internal/gate/runner.go | 6 +- internal/gate/runner_test.go | 2 +- internal/gate/types.go | 15 +- internal/wranglerd/daemon.go | 20 +- .../content/docs/docs/cli/corral-wrangler.md | 2 +- site/src/content/docs/docs/cli/corral.md | 2 +- 18 files changed, 399 insertions(+), 534 deletions(-) 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/main.go b/cmd/corral/main.go index c2b82a98..6a93afa2 100644 --- a/cmd/corral/main.go +++ b/cmd/corral/main.go @@ -57,7 +57,7 @@ // 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 ./..."; +// CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; // 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; // timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; diff --git a/docs/cli/corral-wrangler.md b/docs/cli/corral-wrangler.md index e112e615..ca6df490 100644 --- a/docs/cli/corral-wrangler.md +++ b/docs/cli/corral-wrangler.md @@ -89,7 +89,7 @@ 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 ./..."; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; 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; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; diff --git a/docs/cli/corral.md b/docs/cli/corral.md index 1fa78136..43fa073e 100644 --- a/docs/cli/corral.md +++ b/docs/cli/corral.md @@ -999,7 +999,7 @@ 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 ./..."; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; 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; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; 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..8721c6bf 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"), }) 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..ca6df490 100644 --- a/site/src/content/docs/docs/cli/corral-wrangler.md +++ b/site/src/content/docs/docs/cli/corral-wrangler.md @@ -89,7 +89,7 @@ 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 ./..."; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; 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; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; diff --git a/site/src/content/docs/docs/cli/corral.md b/site/src/content/docs/docs/cli/corral.md index 1fa78136..43fa073e 100644 --- a/site/src/content/docs/docs/cli/corral.md +++ b/site/src/content/docs/docs/cli/corral.md @@ -999,7 +999,7 @@ 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 ./..."; +CORRALAI_GATE_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; 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; timeout= is seconds, defaults to gate.DefaultGateTimeout (600s) when omitted; From 5042197657be439246efeb3c002a0ef1636e1277 Mon Sep 17 00:00:00 2001 From: Peter D Bethke Date: Sun, 13 Sep 2026 07:43:06 -0400 Subject: [PATCH 2/2] gate: update corral-wrangler's help too, and regenerate the CLI docs from the binaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI reference is GENERATED from each binary's real -h output, and CI checks it for drift. I updated `corral`'s env-var help and not `corral-wrangler`'s, then hand-edited the generated markdown to match — so the docs disagreed with the binary that produces them, and the gate failed. A rule applied at one door and not its sibling, in the commit whose entire subject is rules applied at one door and not their siblings. The gate caught it, which is the system working: this is exactly the drift it exists to stop. Both binaries now carry the SAME text from one source, and the generated docs come from scripts/gen-cli-docs.sh rather than from me editing the output. `gen-cli-docs.sh --check` passes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V --- cmd/corral-wrangler/main.go | 11 +++++++---- cmd/corral/main.go | 11 +++++++---- docs/cli/corral-wrangler.md | 11 +++++++---- docs/cli/corral.md | 11 +++++++---- internal/brain/gate_test.go | 2 +- site/src/content/docs/docs/cli/corral-wrangler.md | 11 +++++++---- site/src/content/docs/docs/cli/corral.md | 11 +++++++---- 7 files changed, 43 insertions(+), 25 deletions(-) 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 6a93afa2..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_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; -// 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 ca6df490..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_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; - 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 43fa073e..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_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; - 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_test.go b/internal/brain/gate_test.go index 8721c6bf..70c3e33b 100644 --- a/internal/brain/gate_test.go +++ b/internal/brain/gate_test.go @@ -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/site/src/content/docs/docs/cli/corral-wrangler.md b/site/src/content/docs/docs/cli/corral-wrangler.md index ca6df490..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_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; - 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 43fa073e..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_POLICY_ repo merge gate: ONE policy per variable, "repo=owner/name,base=main,net=false,timeout=600,cmd=go test ./..." — cmd= is last and takes the rest of the value verbatim, semicolons and newlines included; - 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);