Skip to content

feat(evaluate): let evaluate policy take --policy more than once - #1247

Merged
jumboduck merged 5 commits into
mainfrom
feat/evaluate-policy-multiple-roots
Oct 5, 2026
Merged

jumboduck merged 5 commits into
mainfrom
feat/evaluate-policy-multiple-roots

Conversation

@pbeckham

@pbeckham pbeckham commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

kosli evaluate policy now takes --policy more than once, so a policy and a shared Rego library can be sent in one command from wherever each is kept. The help now says what a bundle carries, which of its files the evaluator loads, and which rules it evaluates and prints.

Why

A policy written against a shared library had to be copied into one directory with that library before it could be evaluated, and the copy drifts from the library's own repository. Authors also had no way to learn from the CLI which of their files decide the verdict: that a README travels but is not read, that data.yaml lands under data, or that allow must be defined while every entrypoint: true rule is returned beside it.

What changed

  • Every --policy joins one bundle, and each file is keyed relative to its own root, as opa eval -d one -d two reads them. Modules import each other by package, so their location does not matter, and a data file keeps the data path its own root gives it.
  • Two files the evaluator loads under one name are refused, naming both roots, because keeping either would choose which policy runs. For any other file, such as a second README.md, the first --policy wins and the copy left behind is named on stderr.
  • A directory no longer sends what sits in dot-directories, so a policy kept at a repository's root does not ship .git and hit the 100-file cap.
  • Every other file still travels, so notes stay with the policy. The same rules are meant to govern publishing a policy bundle later.

evaluate trail --server-side still takes one --policy; its behaviour is unchanged apart from the dot-directory rule.

Verification

New tests in evaluatePolicy_test.go pin down a policy plus a library file travelling as one evaluation, per-root keys including a nested data.yaml, refusal of a loaded clash for .rego, data.yaml, data.json and *.ergo.yml, first-wins with a stderr notice for README.md and *_test.rego, the caps counted across roots, dot-directories left out, and both help paragraphs. The existing bundle and cap tests still pass unchanged apart from the fixture README's wording. main.yml runs the full integration suite on push, which covers the evaluate trail and trails suites that share the bundle code.

🤖 Generated with Claude Code

pbeckham and others added 4 commits October 5, 2026 13:17
A policy kept at a repository's root sent its .git objects and CI config,
which the evaluator never loads and which run into the bundle's file cap.

Refs kosli-dev/server#6971

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A policy and a shared library such as ergo can now be sent in one command
without copying them into one directory. Every --policy joins one bundle,
each keyed relative to its own root, as opa eval -d does, so a data file
keeps its path under data. Two files the evaluator loads under one name
are refused, naming both; for any other file the first --policy wins and
the copy left behind is named on stderr.

Refs kosli-dev/server#6971, kosli-dev/server#7196

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…or loads

The help now names the files the evaluator reads (modules other than
tests, and the data filenames), where a data file lands under data, that
every other file travels unread, and how two --policy roots resolve a
name they share.

Refs kosli-dev/server#6971, kosli-dev/server#7196

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
allow is always evaluated and must be defined, and every rule the policy
package annotates as an entrypoint is evaluated and printed by name. The
help now says so, with how the policy package is found among libraries
and which annotations are refused.

Refs kosli-dev/server#6971

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added go Pull requests that update go code feat labels Oct 5, 2026
@claude

claude Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @pbeckham's task in 1m 1s —— View job


Review of commits since last pass (d7c82629..146f07f5)

  • Fetch previous review comments and diff
  • Check status of each previous finding
  • Review new diff for regressions
  • Post summary

Previous findings — both resolved in 146f07f5:

  1. Repeated --policy dir --policy dir — resolved. policyRoots now compares refs via filepath.Abs and silently drops an exact repeat (TestARepeatedRootIsSentOnce covers the plain, trailing-slash, and ../ spellings).
  2. Overlapping roots (--policy dir --policy dir/lib) — resolved. policyRoots now rejects one root nested inside another, in either order and for a file or directory, before anything is read: --policy %s is inside --policy %s, so its files would be sent twice (TestARootInsideAnotherIsRefused covers directory-before, directory-after, and file cases). The evaluatorLoads drift-coverage nit is also addressed with the new table-driven TestEvaluatorLoads.

New diff review: Only evaluateHelpers.go (+54) and evaluatePolicy_test.go (+61) changed. Walked through policyRoots/isWithin against the new tests — dedup and nesting checks are symmetric, use the already-imported path/filepath/strings packages (no new import issues), and error messages reference the original --policy spelling rather than the resolved absolute path, which is correct for user-facing output. No bugs or regressions found in the new code.

Note: I wasn't able to run go build/go test in this sandboxed session (command approval unavailable here), so this is a manual code review rather than a verified test run — the existing test suite (TestARepeatedRootIsSentOnce, TestARootInsideAnotherIsRefused, TestEvaluatorLoads) looks correct by inspection but wasn't executed.

No new inline comments — all touched findings resolved, nothing new flagged.

Comment thread cmd/kosli/evaluateHelpers.go Outdated
Comment thread cmd/kosli/evaluateHelpers.go
…other

A root given twice, however spelled, clashed with itself, and a root
inside another sent each of its files under two names, which the evaluator
reads as two modules declaring the same rules. Pins evaluatorLoads with a
table test, since it copies the evaluator's loader, and corrects the
policyBundleKey comment that still said every entry is parsed as a module.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pbeckham

pbeckham commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

On issue 1 of the review summary: the policyBundleKey comment is corrected in 146f07f. A single file that the evaluator would not load is still accepted on purpose. Under the bundle rule, --policy ctrl.rego --policy README.md is how a note travels with a policy. A bundle with no module is not a silent no-op either, because the evaluator fails it with "the bundle holds no policy module".

@jumboduck
jumboduck merged commit b2d3686 into main Oct 5, 2026
23 checks passed
@jumboduck
jumboduck deleted the feat/evaluate-policy-multiple-roots branch October 5, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants