gate: one policy per env var — remove the ambiguity instead of policing it - #342
Merged
Merged
Conversation
…ng it
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_<NAME>, mirroring the existing
CORRALAI_AGENT_<NAME> 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V
…from the binaries 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three guards were written against a
;insidecmd=, and a cold reviewer defeated all three — each time letting a truncated, weaker command post a wrongful success:;before isolatingcmd=go vet ./... ; go test ./...ran only the vet=-tags=integrationrepo=true; env repo=x false— an incidentalrepo=The fourth answer is not a fourth guard.
CORRALAI_GATE_POLICIESis retired; each policy now gets its ownCORRALAI_GATE_POLICY_<NAME>, mirroring the existingCORRALAI_AGENT_<NAME>convention. The operating system supplies the separator, so a;in a command cannot collide with anything.The headline test is the inverse of the ones it replaces: a semicolon in a command must now survive verbatim.
Also fixed — pre-existing, found in round four:
CheckCmdwas a[]stringfromstrings.Fields, rejoined with spaces at two call sites. That destroyed newlines, sotrue # comment\nfalsecollapsed onto one line and the failing step vanished behind the comment; quoted arguments with spaces would have been mangled too. It is one string from parser to jail now.What this costs, since it isn't free
Policy.CheckCmd []string → stringbreaksbrain Options.GatePoliciescallers.ParsePolicyEnvsorts by name. Ranging a map and taking what comes is the exact non-determinism that produced two findings here in a day, and policies decide which check runs against a PR.Method
Four negative controls, all firing. The harness gained a third invalidity check: a
-runpattern matching no tests now reports invalid rather than passing. It called the semicolon control vacuous when the pattern was simply misspelled — the third time this session my control harness, rather than the control, was the broken part.Full suite green, vet clean, security gate OK, site builds.
🤖 Generated with Claude Code
https://claude.ai/code/session_011NgkB3eLPBhgfqU1XopR1V