feat: register ext.Lists() in CEL environment#186
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR registers CEL list extensions by adding Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 162 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/criteria/cel_evaluator_test.go (1)
410-457: ⚡ Quick winAdd negative-path list-extension tests to protect safe-eval behavior (CWE-693 hardening).
Only happy paths are covered. Add failure-mode cases (e.g., invalid
slicebounds/types, invalidsortBykey expression) and assert the expectedEvaluateSafecontract (errvsresult.HasError()). This is where policy-eval degradations typically hide.As per coding guidelines, “Error paths SHOULD be tested, not just happy paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/criteria/cel_evaluator_test.go` around lines 410 - 457, Tests only cover happy paths for list extensions; add negative-path cases calling evaluator.EvaluateSafe to verify safe-eval behavior for invalid inputs (CWE-693). Add tests like: a) call `tags.slice(5,2)` and `tags.slice("a",2)` and assert either a non-nil err or result.HasError() per the EvaluateSafe contract; b) call `nodePools.sortBy(p, p.nonexistent)` and `nodePools.sortBy(p, 123)` and assert proper error/result.HasError(); c) call `someList.sort()` with mixed-type elements and assert failure; ensure each new test mirrors the existing style (use require.NoError/require.Error and require.True/False on result.HasError() as appropriate) and place them near the existing "slice returns sub-list" and "sortBy orders objects by field" tests so failures surface in the same area.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/conventions/cel.md`:
- Around line 24-33: Docs claim ext.Lists() is available but Sentinel's CEL
environment only registers ext.Strings(), causing list-based CEL expressions to
fail in hyperfleet-sentinel; fix by adding ext.Lists() registration to
Sentinel's CEL env initialization (the same place ext.Strings() is registered in
the Sentinel CEL env setup / decision env builder) to mirror
hyperfleet-adapter's registration in the cel evaluator, or if you prefer a docs
change instead, update the CEL conventions doc to explicitly state that
ext.Lists() is currently adapter-only until Sentinel's CEL env (where
ext.Strings() is registered) is updated; ensure the change references
ext.Lists() and ext.Strings() so reviewers can verify parity.
In `@internal/criteria/cel_evaluator_test.go`:
- Around line 395-458: Add subtests inside TestCELEvaluatorExtLists to cover
lists.range and depth-bounded flatten: call
evaluator.EvaluateSafe("lists.range(3)") and assert no error, no result error,
that the result is a []ref.Val of length 3 with values 0,1,2 (as types.Int); and
call evaluator.EvaluateSafe("nested.flatten(1)") (or "nested.flatten(2)" if you
prefer explicit depth) and assert no error, no result error, and that the
flattened result is a []ref.Val of the expected length/contents (e.g., length 4
with "a","b","c","d"); place these as new t.Run subtests within
TestCELEvaluatorExtLists near the other list-method tests and use the existing
evaluator variable and require/assert helpers already in the file.
In `@internal/criteria/cel_evaluator.go`:
- Line 72: The Adapter is enabling ext.Lists() (options = append(options,
ext.Lists())) which causes CEL environment drift versus Sentinel’s CEL builder;
to fix, ensure both evaluators use the same CEL options by either adding
ext.Lists() to the Sentinel CEL environment construction or gating Adapter’s use
of ext.Lists() behind a shared compatibility flag used by both components;
update the Sentinel CEL builder (the function that constructs its CEL options)
to include ext.Lists() or add a centralized feature flag that both Adapter and
Sentinel check before appending ext.Lists(), and run the CEL compatibility tests
to verify parity.
In `@test/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml`:
- Line 413: The expression spec.node_pools.map(p, p.name).slice(0, 1)[0] can
throw if spec.node_pools is empty; modify the CEL expression to guard against
empty lists by checking size() > 0 (e.g., use spec.node_pools.size() > 0 ?
spec.node_pools.map(p, p.name).slice(0,1)[0] : "<fallback>") or use a safe
ternary that returns a sensible default instead of indexing into an empty slice;
update the expression in the task config to perform this check and supply a
fallback value.
- Around line 23-27: Sentinel's CEL environment registers ext.Strings() but not
ext.Lists(), causing configs that use .distinct(), .sort(), .sortBy(), .slice(),
or .flatten() to fail; update the CEL environment initialization (the same
location that registers ext.Strings() in decision.go and payload/builder.go) to
also register ext.Lists(), rebuild the CEL environment to match ADR 0006's
shared expression engine expectation, and run unit/integration tests to validate
that expressions using distinct/sort/sortBy/slice/flatten no longer produce
undeclared reference errors.
---
Nitpick comments:
In `@internal/criteria/cel_evaluator_test.go`:
- Around line 410-457: Tests only cover happy paths for list extensions; add
negative-path cases calling evaluator.EvaluateSafe to verify safe-eval behavior
for invalid inputs (CWE-693). Add tests like: a) call `tags.slice(5,2)` and
`tags.slice("a",2)` and assert either a non-nil err or result.HasError() per the
EvaluateSafe contract; b) call `nodePools.sortBy(p, p.nonexistent)` and
`nodePools.sortBy(p, 123)` and assert proper error/result.HasError(); c) call
`someList.sort()` with mixed-type elements and assert failure; ensure each new
test mirrors the existing style (use require.NoError/require.Error and
require.True/False on result.HasError() as appropriate) and place them near the
existing "slice returns sub-list" and "sortBy orders objects by field" tests so
failures surface in the same area.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e0331684-2322-4b6a-98c9-8f6cfc04371b
📒 Files selected for processing (4)
docs/conventions/cel.mdinternal/criteria/cel_evaluator.gointernal/criteria/cel_evaluator_test.gotest/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| // Enable optional types for optional chaining syntax (e.g., a.?b.?c) | ||
| options = append(options, cel.OptionalTypes()) | ||
| options = append(options, ext.Strings()) | ||
| options = append(options, ext.Lists()) |
There was a problem hiding this comment.
Cross-component CEL drift risks policy mismatch (CWE-436/CWE-693).
Line 72 enables ext.Lists() in Adapter, but linked findings show Sentinel still building CEL env without ext.Lists(). That creates evaluator divergence where the same expression can pass compile/eval here and fail in Sentinel. For gate/policy paths, this is a major integration risk; align env options across components or gate rollout behind shared compatibility.
As per coding guidelines, “Validate changes against HyperFleet architecture standards from the linked architecture repository” and “Prioritize Critical and Major severity issues.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/criteria/cel_evaluator.go` at line 72, The Adapter is enabling
ext.Lists() (options = append(options, ext.Lists())) which causes CEL
environment drift versus Sentinel’s CEL builder; to fix, ensure both evaluators
use the same CEL options by either adding ext.Lists() to the Sentinel CEL
environment construction or gating Adapter’s use of ext.Lists() behind a shared
compatibility flag used by both components; update the Sentinel CEL builder (the
function that constructs its CEL options) to include ext.Lists() or add a
centralized feature flag that both Adapter and Sentinel check before appending
ext.Lists(), and run the CEL compatibility tests to verify parity.
Sources: Coding guidelines, Linked repositories
| # 16. distinct() — deduplicate list elements (ext.Lists) | ||
| # 17. sort() — sort list elements (ext.Lists) | ||
| # 18. slice() — extract a sub-list by index range (ext.Lists) | ||
| # 19. flatten() — collapse nested lists into one (ext.Lists) | ||
| # 20. sortBy() — sort objects by a derived key (ext.Lists) |
There was a problem hiding this comment.
CEL list extension version skew with hyperfleet-sentinel.
Sentinel's decision.go and payload/builder.go register ext.Strings() but not ext.Lists(). ADR 0006 defines CEL as a shared expression engine. Configs using .distinct(), .sort(), .sortBy(), .slice(), or .flatten() will fail in Sentinel with undeclared reference errors until its CEL environment is updated to match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml`
around lines 23 - 27, Sentinel's CEL environment registers ext.Strings() but not
ext.Lists(), causing configs that use .distinct(), .sort(), .sortBy(), .slice(),
or .flatten() to fail; update the CEL environment initialization (the same
location that registers ext.Strings() in decision.go and payload/builder.go) to
also register ext.Lists(), rebuild the CEL environment to match ADR 0006's
shared expression engine expectation, and run unit/integration tests to validate
that expressions using distinct/sort/sortBy/slice/flatten no longer produce
undeclared reference errors.
Source: Linked repositories
Adds ext.Lists() to the CEL evaluator alongside the existing ext.Strings(), enabling list manipulation functions in all CEL expression contexts (precondition captures, payload conditions, post-action gates). Functions available: distinct(), sort(), sortBy(), slice(), flatten(), lists.range() Payload expressions reference captured variables (uniqueTags, sortedTags, etc.) rather than raw API response fields, which are out of scope in the payload phase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@rh-amarin: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
Registers
ext.Lists()in the CEL evaluator environment, enabling list extension functions in precondition capture expressions, payload conditions, and post-action gates — mirroring howext.Strings()was added in #121.Functions added:
<list>.distinct()— remove duplicate elements<list>.sort()— sort comparable elements alphabetically / numerically<list>.sortBy(e, keyExpr)— sort objects by a derived key<list>.slice(start, end)— extract a sub-list by index range<list>.flatten()/flatten(depth)— collapse nested listslists.range(n)— generate[0, 1, …, n-1]Changes
internal/criteria/cel_evaluator.go: addext.Lists()tobuildCELOptions()internal/criteria/cel_evaluator_test.go:TestCELEvaluatorExtListswith 5 subtests (distinct,sort,slice,flatten,sortBy)test/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml: patterns 16–20 with capture and payload examples using real cluster API response datadocs/conventions/cel.md: List Extensions section documenting all 6 functions with examplesTest plan
make test— all unit tests pass including newTestCELEvaluatorExtListsmake lintadapter serve --dry-run-eventwith the updated cel-showcase config🤖 Generated with Claude Code