Skip to content

test(evaluate): pin the CLI's OPA/Rego contract - #1073

Merged
mbevc1 merged 4 commits into
mainfrom
20260803_opa_testing
Aug 4, 2026
Merged

test(evaluate): pin the CLI's OPA/Rego contract#1073
mbevc1 merged 4 commits into
mainfrom
20260803_opa_testing

Conversation

@mbevc1

Copy link
Copy Markdown
Contributor

Policies passed to kosli evaluate are written by users, so a change in Rego parsing, safety checking, or the built-in surface is a breaking change for us even when our own Go code still compiles. The OPA 1.18.2 -> 1.19.0 bump in #1071 is the case in point: stricter safety checking for := invalidates a class of previously working customer policies, and nothing in the suite would have caught it.

Add the coverage that would have. These are characterisation tests split by layer: internal/evaluate owns Rego semantics, cmd/kosli proves only that diagnostics and verdicts reach the terminal.

internal/evaluate/opa_contract_test.go:

  • a realistic default-deny policy end to end, with --params-driven required attestations against trail-shaped input
  • the built-in surface real policies reach for (regex.match, semver.compare, time.parse_rfc3339_ns, every, some ... in, walk)
  • Rego v1 as the default parser version
  • the 1.19.0 := strictness change, paired with an over-strictness guard so a release that breaks correct := usage also goes red
  • unsafe-var reporting, and a broken violations rule failing before any verdict is produced

cmd/kosli:

  • the same boundary through evaluate input
  • TestPolicyFixturesStillCompile, a corpus-wide canary over testdata/policies that checks every fixture against each future bump

Two sharp edges are pinned as characterisation, not endorsement: an undefined allow errors instead of denying, and with --params omitted data.params is undefined, so object.get(data.params, k, fallback) yields undefined rather than the fallback.

Checklist

@mbevc1mbevc1 added enhancement New feature or request testing labels Aug 3, 2026
@claude

claudeBot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 2m 15s —— View job


Review: pin the CLI's OPA/Rego contract

  • Gather context (read changed files + production Evaluate)
  • Static analysis (go build ./..., go vet, go mod tidy)
  • Code review
  • Post summary

Static analysis

CheckResult
go build ./...✅ clean
go vet ./internal/evaluate/... ./cmd/kosli/...✅ clean
go mod tidy + git diff --exit-code go.mod go.sum✅ no changes

(I couldn't execute go test in this sandbox — approval declined — but I verified the assertions against the fixtures and production code by hand; see below.)

What this PR does well

This is a genuinely thoughtful characterisation suite, and it earns its +667/-0:

  • The layer split is the right call.internal/evaluate owns Rego semantics; cmd/kosli proves only that a diagnostic and a verdict reach the terminal (evaluateInput_test.go:151-203). No behavioural duplication across layers.
  • //go:embed kills the drift risk. The realistic policy is a single fixture (internal/evaluate/testdata/policies/realistic-compliance.rego), embedded here and pointed at by the CLI test via relative path — a rename/delete now breaks the build rather than silently forking two copies. This resolves the earlier duplication thread cleanly.
  • The 1.19.0 regression is pinned from both sides.TestOPAContract_AssignmentRHSSafetyIsStrict locks the strictness change, and TestOPAContract_SafeAssignmentFromInputStillCompiles is the over-strictness guard so a future release that breaks correct:= also goes red. That pairing is exactly what would have caught chore(deps): bump the go-dependencies group with 10 updates #1071.
  • Sharp edges are pinned as characterisation, not endorsement, each with a "this is the test to invert when we fix it" note: undefined allow errors instead of denying (:271), data.params undefined without --params (:412), non-string violations silently dropped (:313).
  • Security property is documented, not hidden.TestOPAContract_PolicyCanReachTheNetwork (:351) makes the http.send exfiltration surface explicit and testable — the honest way to carry a known risk until the ast.Capabilities allowlist lands.
  • All four earlier review threads (embed the fixture, mirror the deliberatelyBroken skip-set, version-stamp both canary failures) are addressed by the follow-up commits.

Minor observations (non-blocking)

  1. Cross-package relative path.evaluateInput_test.go:184,190 reach ../../internal/evaluate/testdata/policies/realistic-compliance.rego. It's well-justified in the comment and runs fine from the package dir, but it's the one spot that would break silently on a testdata reshuffle (the embed on the other side would still compile). The comment already flags the trade-off, so this is just noting it out loud.
  2. TestOPAContract_UnsafeVarIsReportedWithFileAndLine and TestOPAContract_AssignmentRHSSafetyIsStrict both drive the same x := y shape — deliberate (one pins file:line, one pins the safety message text), just flagging the overlap so a future refactor doesn't collapse them and lose one dimension.
  3. The http.send follow-up still has no home. The invert-this-assertion note at :358 is good, but without a tracked issue number it relies on a grep to be found again. Worth filing the ast.Capabilities allowlist ticket and linking it in that comment. Open a follow-up →

Nothing here blocks. This is exactly the kind of contract coverage that turns a customer-facing dependency-bump regression into a red CI run. Nice work.
20260803_opa_testing

Comment threadinternal/evaluate/opa_contract_test.go
Comment threadinternal/evaluate/opa_contract_test.go Outdated
Comment threadinternal/evaluate/opa_contract_test.go
Comment threadinternal/evaluate/opa_contract_test.go Outdated
Comment threadinternal/evaluate/opa_contract_test.go
Policies passed to `kosli evaluate` are written by users, so a change in
Rego parsing, safety checking, or the built-in surface is a breaking
change for us even when our own Go code still compiles. The OPA
1.18.2 -> 1.19.0 bump in #1071 is the case in point: stricter safety
checking for `:=` invalidates a class of previously working customer
policies, and nothing in the suite would have caught it.
Add the coverage that would have. These are characterisation tests split
by layer: internal/evaluate owns Rego semantics, cmd/kosli proves only
that diagnostics and verdicts reach the terminal.
internal/evaluate/opa_contract_test.go:
- a realistic default-deny policy end to end, with --params-driven
required attestations against trail-shaped input
- the built-in surface real policies reach for (regex.match,
semver.compare, time.parse_rfc3339_ns, every, some ... in, walk)
- Rego v1 as the default parser version
- the 1.19.0 `:=` strictness change, paired with an over-strictness
guard so a release that breaks correct `:=` usage also goes red
- unsafe-var reporting, and a broken `violations` rule failing before
any verdict is produced
cmd/kosli:
- the same boundary through `evaluate input`
- TestPolicyFixturesStillCompile, a corpus-wide canary over
testdata/policies that checks every fixture against each future bump
Two sharp edges are pinned as characterisation, not endorsement: an
undefined `allow` errors instead of denying, and with `--params` omitted
`data.params` is undefined, so object.get(data.params, k, fallback)
yields undefined rather than the fallback.
The policy existed twice — a const in the internal test and a
near-identical .rego under cmd/kosli/testdata. Both copies passed
independently, so drift would have produced two layers disagreeing about
what the policy means with nothing going red.
Keep one copy, in internal/evaluate/testdata, and consume it from both
layers: the internal test embeds it with //go:embed, and the CLI test
points --policy at the same file. That direction is with the grain of
the dependency graph — cmd/kosli already imports internal/evaluate in
production — and embedding means a rename or deletion breaks the build
rather than one test.
Verified by mutating the fixture's default `required` list: the CLI's
ALLOWED case flips to DENIED, confirming both layers read the one file.
`internal/evaluate` gains its own canary over its testdata, mirroring the
cmd/kosli one, so each package covers the fixtures it owns.
@mbevc1
mbevc1force-pushed the 20260803_opa_testing branch from 2690dca to a1587ddCompareAugust 3, 2026 14:47
internal/evaluate's fixture canary had no exclusion set, unlike the
cmd/kosli copy. It passes today because this package's testdata holds
only valid fixtures, but internal/evaluate is exactly where an
intentionally uncompilable fixture would naturally land, and that would
have turned the canary red spuriously.
The set is empty for now — its value is being the obvious home for such
a fixture, and keeping the two mirrors structurally identical.
The cmd/kosli mirror names the OPA version in its parse and compile
failure messages; this copy did not. An upgrade-triggered failure is the
whole reason these canaries exist, so the message that names the version
that broke the fixture should read the same from either package.
Verified both messages render: a malformed fixture reports "no longer
parses under OPA 1.19.0", and one that parses but fails safety checking
reports "no longer compiles under OPA 1.19.0".
@mbevc1
mbevc1force-pushed the 20260803_opa_testing branch from a1587dd to da76d4bCompareAugust 3, 2026 14:48
@mbevc1
mbevc1 enabled auto-merge (squash) August 3, 2026 14:48
@mbevc1
mbevc1 merged commit dec2d34 into mainAug 4, 2026
20 checks passed
@mbevc1
mbevc1 deleted the 20260803_opa_testing branch August 4, 2026 08:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or requesttesting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mbevc1@dangrondahl