[plan][runtime][java] Add CEL Action condition filtering

[rosemarYuan]

Linked issue: #754

Stacked PR — this PR is based on #756 . Please review the [incremental diff], not the full diff against main.

Purpose of change

This is the second PR in the four-PR stack tracked by #754:

1. [api][java][python] Introduce EventType constants and unify Action trigger entry #756 — API changes (under review)
2. This PR — Java CEL runtime
3. Python CEL runtime (planned)
4. Documentation (planned)

It adds the Java-side CEL runtime for @Action trigger conditions, enabling actions to filter events by field-level CEL expressions (e.g. event.tokenCount > 100).

Key components

• ParsedCondition.classify: parses each trigger condition via CEL parser; bare identifier → TypeMatch, anything else → CelExpression.
• CelMacroPolicy: custom has() macro (has(x) → has(attributes.x)); whitelist rejects exists/filter/map etc.
• CelExpressionFacade: Parse → Validate → Check → Compile pipeline with process-wide LRU program cache and AST resource limits.
• CelConditionEvaluator: pre-compiles programs at open time; builds activation map from event; evaluates with configurable failure policy.
• ActionRouter: two-phase routing — actionsByEvent HashMap fast path for TypeMatch, CEL slow path with lazy activation and LinkedHashSet deduplication.
• Operator integration: ActionExecutionOperator / EventRouter delegate to ActionRouter.

Tests

• Plan layer: ParsedConditionTest, ActionParsedConditionsTest, ActionConstructionFailureTest
• Runtime layer: CelExpressionFacadeTest, CelConditionEvaluatorTest, ActionRouterTest, CelResourceLimitsTest
• Serialization: ActionJsonSerializerTest, ActionJsonDeserializerTest (including legacy listen_event_types fallback)
• Cross-language: cel_conformance_cases.yaml, disallowed_macros.yaml + .github/workflows/cel-conformance.yml

API

No new public API beyond what was introduced in #756. This PR adds runtime internals only.

Documentation

• doc-needed
• doc-not-needed (documentation will be covered in PR4)
• doc-included

[weiqingy]

Thanks for taking this on — the serialization discipline stands out: keeping the compiled Programs off the Java-serialization path (router built on the TaskManager) and rebuilding the transient actionsWithCel in both readObject and the constructors are the easy-to-miss details that quietly break on failover. The macro-whitelist-via-CALL-node approach is a neat, well-tested call too. A few questions inline, plus one CI issue worth a look before merge.

1. The dist uber-jar now shades dev.cel and its transitive graph (antlr4-runtime, re2j, protobuf-java). Is the LICENSE/NOTICE update for those tracked somewhere (release cut, or PR4), or expected in this PR? Just so it doesn't fall through the Apache release gate.

[weiqingy]

The reserved-keyword parity step reads symbols that don't exist at head, so it verifies nothing today and will mis-fire once PR3 lands. java_file points at CelReserved.java, but the reserved set lives in CelMacroPolicy.RESERVED_IDENTIFIERS; the set\.add("...") regex matches nothing (the set is built with Set.of(...) + set.addAll(CEL_STANDARD_MACROS)), so java_names comes out empty; and the DISALLOWED_MACROS branch keys off a constant that isn't there (CEL_STANDARD_MACROS / ALLOWED_MACROS are). Line 111 also lists AstRewriterTest in -Dtest, which has no class at head. These read like leftovers from the pre-refactor design (a CelReserved class, a DISALLOWED_MACROS set, an AST rewriter) that got folded into CelMacroPolicy without the workflow following along.

It's harmless right now because the step exit 0s while cel_reserved.py is absent — but the moment PR3 adds the Python side, this compares an empty Java set against the Python frozenset: either a spurious failure (Java only: []) or, if both happen to be empty, a false OK: 0 reserved identifiers aligned that silently passes a check guaranteeing nothing. Since the cross-language parity guard is the whole point of the workflow, would it be worth fixing it here — point the parser at CelMacroPolicy.java, extract RESERVED_IDENTIFIERS from the Set.of(...) block plus CEL_STANDARD_MACROS, drop AstRewriterTest — rather than carrying it forward broken? A guard that fails when java_names is empty would also keep a future refactor from silently re-breaking it.

[rosemarYuan]

Thanks for flagging this — I hadn't realized the deps shaded into the uber-jar needed LICENSE entries. You're right that this PR is where dev.cel enters the uber-jar, so this is the right place to handle it. Pushed a commit adding a bundled-dependency section to LICENSE listing the four transitives (dev.cel, antlr4-runtime, re2j, protobuf-java; Apache 2.0 / BSD-3-Clause) and appending the upstream copyright lines to NOTICE.

On cel-conformance.yml — my miss, I forgot to update the workflow after the refactor, and the issues you called out all hold. I fixed them locally first, then moved the fix commit over to the Python implementation PR. Rationale: the cross-language parity check only becomes meaningful once cel_reserved.py lands, so landing the fix together with the Python side makes for a cleaner story and lets us verify both ends in one go. The Python PR will go out after this Java PR is reviewed — looking forward to your eyes on that one too.

[weiqingy]

Thanks — the LICENSE/NOTICE additions cover it: dev.cel:cel/dev.cel:protobuf under Apache 2.0, re2j/antlr4-runtime under BSD-3-Clause, plus the guava/checker-qual/tree-sitter transitives, with the individual license files alongside. The definitive check is the shaded dist jar at release, but the set lines up with dev.cel's closure.

And agreed on removing the workflow until the Python side lands — that's cleaner than carrying a parity check that can't run yet, and it sidesteps the empty-set false-pass. I'll keep an eye out for the rebuilt check on the Python PR: that the regex pulls a non-empty RESERVED_IDENTIFIERS out of CelMacroPolicy, and that an empty java_names fails loudly rather than aligning two empty sets. Quick turnaround, appreciated.

[weiqingy]

This parser is built without the resource-limit CelOptions that CelExpressionFacade.CEL_PARSER uses (maxParseRecursionDepth(32), maxExpressionCodePointSize(8192)), so the "same dialect as the runtime facade parser" note isn't quite accurate — the parse limits differ. The practical effect is small since classify() runs at plan-build time on the author's own annotation strings, but it does mean a deep expression parses fine here and only trips the cap later at toProgram. Could we apply the same maxParseRecursionDepth(32) / maxExpressionCodePointSize(8192) caps here too? ParsedCondition is in plan and the facade's CEL_OPTIONS is private to runtime, so a shared parser would cross the module boundary — but duplicating the two caps would line the limits up with the "same dialect" doc and surface a too-deep expression once, early.

[rosemarYuan]

You're right — the doc claim and the code had drifted, and I did miss this during the implementation.

I adopted the local fix you suggested: duplicating the two numeric caps in the plan-side parser is lighter than crossing the plan/runtime module boundary for a shared CelOptions singleton. With this change, a too-deep or too-long expression now fails at classify() during job submission, which matches the runtime facade behavior claimed in the documentation.

[weiqingy]

Confirmed — the duplicated caps line up with CelExpressionFacade.CEL_OPTIONS (maxParseRecursionDepth(32) / maxExpressionCodePointSize(8192)), so a too-deep or too-long expression now trips at classify() as the doc claims.

[weiqingy]

open() always constructs the evaluator with the default WARN_AND_SKIP policy, and nothing else (config, plan field, ctor arg) can select FAIL, so production always silently drops an action on a CEL runtime error or non-boolean result, with only a WARN log as signal. FAIL is fully implemented and tested but reachable only from tests. Two things I wanted to check rather than assume: is silent-skip the intended only production behavior here (fail-open vs. fail-closed is a real choice — a compilable expression that throws on every event would just quietly stop firing), and is FAIL meant to become a config knob, or is it speculative for now? If it's not going to be selectable, dropping it would avoid the test-only code path; if it is, the wiring looks like it's missing.

[rosemarYuan]

Good catch — this was intended to be configurable, but the final wiring step was missing, so FAIL was only reachable from tests.

The intended default is still WARN_AND_SKIP, since that is the safer streaming behavior: a single bad event or unexpected CEL evaluation result should not fail the whole job. That said, strict-semantics pipelines do need a fail-closed option, so FAIL should be reachable in production rather than remaining a test-only path.

I added a follow-up commit to wire this through properly:

• promoted EvaluationFailurePolicy to the API module as CelEvaluationFailurePolicy, following the existing LoggerType pattern;
• exposed CEL_EVALUATION_FAILURE_POLICY in AgentConfigOptions, with WARN_AND_SKIP as the default;
• updated ActionRouter.open() to read the policy from the plan config when constructing the evaluator.

With this change, both WARN_AND_SKIP and FAIL are now selectable in production. Default behavior is unchanged for existing users — anyone not setting celEvaluationFailurePolicy in plan config still gets WARN_AND_SKIP.

[weiqingy]

The wiring looks right — getConfig() is always initialized (every AgentPlan constructor sets a default AgentConfiguration), so open() won't NPE, and the enum round-trips through config the same way EVENT_LOGGER_TYPE/LoggerType does.

One gap on the path you just added, though: nothing tests it end-to-end. The only FAIL test (CelConditionEvaluatorTest.testFailPolicyThrowsOnEvaluationError) constructs the evaluator directly, so it never exercises CEL_EVALUATION_FAILURE_POLICY → ActionRouter.open() → evaluator — and all the ActionRouterTest cases run on the default policy. A regression in the wiring (wrong option read, default flipped, open() dropping the value) would stay green. Would it be worth a small ActionRouterTest case — build a plan with a CEL action, getConfig().set(AgentConfigOptions.CEL_EVALUATION_FAILURE_POLICY, FAIL), then assert route() throws IllegalStateException on an event whose condition errors? That would lock down the path this commit is here to enable.

[rosemarYuan]

Thanks for the review suggestion. Good catch — that error path was not covered before.

I added two paired cases in ActionRouterTest, both using the same erroring expression (attributes.nonexistent > 3) and the same event:

• route_failPolicyFromConfig_throwsOnConditionEvaluationError: sets CEL_EVALUATION_FAILURE_POLICY=FAIL via getConfig().set(...) and verifies that route() throws IllegalStateException.
• route_defaultWarnAndSkip_swallowsConditionEvaluationError: leaves the policy unset and verifies that route() returns an empty result.

The pairing is intentional. A standalone FAIL -> throw test could still pass if the default policy were accidentally changed, the option key were misread, or the configured value were dropped. By keeping the expression and event identical and varying only the policy source, the tests make those regression modes visible.

[weiqingy]

Confirmed — the paired cases close the gap. route_failPolicyFromConfig_throwsOnConditionEvaluationError walks exactly the path this commit enables (getConfig().set(FAIL) → open() reading CEL_EVALUATION_FAILURE_POLICY → evaluator), and pinning hasMessageContaining("CEL condition evaluation failed") keys on the eval-error branch specifically rather than the non-boolean message, so it won't silently match the wrong throw site. Holding the expression and event fixed while varying only the policy source is what makes the default-flip / misread-key / dropped-value regressions each surface in one case.

Nothing further from me — the changes look good. I'll leave the final call to the maintainers.

[wenjin272]

Thanks for taking this on @rosemarYuan.

I left some comments regarding certain class names and JavaDoc. Additionally, I believe we are missing end-to-end (E2E) tests.

[wenjin272]

I don't see any python implementations related to _MAX_NORMALIZE_DEPTH. What means mirrors Python's _MAX_NORMALIZE_DEPTH?

[wenjin272]

I believe users cannot fully understand the purpose of this configuration item from its name. CEL is an implementation-level detail, and API-layer users do not need to understand how conditions are evaluated. It could be renamed to action.trigger-condition.evaluate-failure-tolerance-strategy or some one like this.

[wenjin272]

I feel it's unnecessary to have a separate file, simply implement it as an inner class of AgentConfigOptions.

[wenjin272]

An IllegalArgumentException is thrown here, but it is not caught anywhere. This behavior is inconsistent with the advertised functionality of WARN_AND_SKIP.

[wenjin272]

All fields are included in the normalization process. Is it not possible to normalize only the attributes involved in conditional calculations here?

[rosemarYuan]

Good point. I agree that normalizing only the attributes referenced by the condition would be a useful optimization, but I don't think this is the right time to introduce it yet, mainly because the current Python CEL engine has limited support for doing this lazily.

Today buildTriggerVariables normalizes the full attributes tree for each event, regardless of which variables the condition actually reads. On the Java side, this could be optimized with CEL Java's Program.eval(CelVariableResolver): we could use a memoized resolver and normalize only the referenced root/subtree on demand, while still preserving the current output > root > input precedence.

The Python side is the limiting factor. celpy, the more mature community-driven implementation we use today, loads values before evaluation: it eagerly scans the input values and builds the NameContainer / CEL value tree up front, so a lazy mapping would still be enumerated during loading.

There is a possible future path through cel-expr-python, the new official Google-backed Python implementation released this year. Its underlying cel-cpp runtime has a value-provider mechanism that could support lazy attribute binding. But the current Python binding still loads eagerly, requires Python >= 3.11 while we still support 3.10, and is pre-1.0 (0.1.3) without API/stability guarantees. I’ve also opened an upstream issue to ask whether Python 3.10 support could be added.

So for this PR, I’d keep the behavior simple and consistent: activation is eagerly built, and the cross-language contract remains behavioral rather than mechanism-level. We can revisit lazy normalization once the Python engine story is more stable.

[wenjin272]

Activation is a concept specific to CEL. People unfamiliar with CEL may not understand the purpose of this function based on its name alone. I suggest renaming it to mapEventToTriggerConditionVariables or something like this.

Our naming and Javadoc directly expose CEL in many places. I believe that even in modules primarily intended for other developers, such as the plan/runtime module, we should strive to encapsulate CEL concepts within the expression evaluation internals. For example, rename CelConditionEvaluator to ConditionEvaluator, and ActionWithCels to ActionWithConditions.

[wenjin272]

dev.cel:common:0.12.0
dev.cel:compiler:0.12.0
dev.cel:runtime:0.12.0
dev.cel:v1alpha1:0.12.0

[wenjin272]

It appears that parsedConditions is designed to distinguish whether each condition is a TypeMatch or a CelExpression. The cache is stored within the Action to avoid repeated runtime checks.

1. I suggest declaring this field as final.
2. The current Javadoc reflects an earlier implementation where the CEL AST was stored. However, due to Kryo serialization issues, this was changed to store a String instead, rendering the documentation outdated. In my experience, such comments are often auto-generated by coding agents. It is unnecessary to document development pitfalls like Kryo serialization problems; instead, please simply describe the field’s role in the final solution.

[wenjin272]

I suggest renaming it to ConditionExpressionCompiler, as the term "Facade" is rarely used and its meaning is unclear.

I also suggest changing the Javadoc to: Compiles trigger condition expressions into cached executable programs

[wenjin272]

The current classifier effectively restricts direct event-type triggers to CEL identifiers. That may be an acceptable beta API constraint, but it should be made explicit rather
than surfacing as a CEL parse side effect.

For example, @Action("com.example.OrderEvent") or @Action("order-created") used to look like natural event-type strings, but now they are parsed as condition expressions /
invalid CEL instead of being accepted as direct type triggers. If direct event-type triggers are intended to share the same string slot as condition expressions, please document
that event type names used in @Action(...) must be valid condition identifiers and cannot contain ., -, :, etc. It would also be better to validate this explicitly at plan
construction time and return a trigger-condition error message, instead of an Invalid CEL expression error.

[wenjin272]

Why does the name mix camelCase and snake_case?

[wenjin272]

Why add an e2e directory to the runtime module?

[rosemarYuan]

Thanks for the review. I updated the trigger-entry contract to make the type-name vs condition-expression behavior explicit.

The key issue is that the expression parser treats . as field selection and - as subtraction. So event type names or attribute keys containing those characters cannot be written as plain identifiers without ambiguity.

The updated contract is:

• For event type trigger entries:

  • standard type names containing only letters, digits, and _ can still be used directly, e.g. @Action("userEvent8");
  • complex type names containing ., -, etc. should be quoted as a string literal, e.g. @Action("'com.ios.orderEvent'");
  • in expressions, compare against the type string normally, e.g. @Action("type == 'com.ios.orderEvent' && score > 10");
  • name-like entries such as a.b.event or order-created now fail at registration time with guidance to quote them, instead of being parsed incorrectly and silently never matching.

• For attribute access:

  • normal nested keys can still use dot access, e.g. a.b.c;
  • keys that themselves contain . or - should use index syntax, e.g. score_name['batch-3.12'].region > 10;
  • key existence for such names can use in, e.g. 'batch-3.12' in score_name.

I also reviewed the test coverage, filled the previously uncovered paths, and added a simple end-to-end test for trigger conditions.

Besides that, I fixed several details raised in the comments, including making the public naming/Javadocs use the user-facing “trigger condition” terminology, centralizing failure handling under the new failure-policy config, and updating the dist NOTICE/LICENSE entries for the bundled dependencies.

[rosemarYuan]

For Object normalizedAttrs = normalizeValue(event.getAttributes(), 0);, I think here have two question.

The first one is the large-number failure. That part has been fixed: when a number larger than int64 enters condition evaluation, we now convert it to double instead of failing during activation building.

The second one is reducing the amount of data passed into condition evaluation. I agree that we should avoid normalizing every field when the condition only needs a small subset. There are two possible directions here.

1. Shrink the map we build for condition evaluation. At plan/open time, we can analyze each condition and collect the attribute paths it may read. At runtime, we build the input map only from the union of fields needed by the candidate conditions for that event, and normalize only those selected values. If a condition cannot be narrowed safely, we can fall back to the current full-map behavior.
2. Lazy attribute access from the expression engine itself. Java already has the needed capability through CEL Java’s variable resolver path, so values can be normalized only when the expression actually reads them. The problem is Python: the current community implementation does not provide an equivalent lazy access path. The newer Google-backed Python implementation is promising because it wraps the official C++ runtime, but the Python binding does not expose this capability yet, and it currently requires Python 3.11 while we still support Python 3.10.

So my plan is to implement the first approach now: partial map construction with conservative fallback to full normalization. Later, once the Google CEL Python implementation supports Python 3.10 and exposes the required lazy access capability, we can consider migrating to the second approach.

@wenjin272 does this direction sound reasonable?

[wenjin272]

After a thorough review, I believe the current PR has issues in API design, architectural organization, and specific function implementation details. I have provided detailed comments on these issues.

[wenjin272]

Comment 1: simplify the variable-building paths

I think the full-build / partial-build fallback structure is adding more complexity than the trigger condition semantics require.

If we want to keep the referenced-key optimization, the runtime could have a single main path:

1. collect the referenced attribute keys from the precompiled condition metadata,
2. build the CEL activation only for those keys,
3. add the framework-owned variables such as type, EventType, and the event-id fallback.

The current fallback paths mostly exist because the full build supports extra implicit behavior (attributes as a full namespace, JSON-looking strings being parsed, and input/
output subkeys being flattened). If those semantics are not part of the intended public condition DSL, we should not preserve them through fallback logic.

Concretely, buildTriggerVariablesForConditions can stay as the main entry point, with maybe one helper for collecting referenced keys. buildTriggerVariables /
buildPartialTriggerVariables do not need to be separate public-style paths.

Comment 2: avoid exposing attributes as a special framework variable

I find the special attributes variable confusing here.

Event.attributes is already the user payload. For action conditions, the natural user-facing model should be that keys inside event.getAttributes() are exposed as condition
variables, e.g. score > 80, not that users need to know about the Java Event object shape and write attributes.score.

Supporting both score and attributes.score creates a second access model and forces special handling in extractMergedKeys() (attributes as a whole-map sentinel). It also
makes attributes a reserved name, which can conflict with a real user attribute named attributes.

I suggest not injecting a framework-owned full attributes map into the CEL activation. Treat attributes as just another user attribute key if the user provided one, and only
reserve explicit framework variables such as type and EventType.

Comment 3: remove JSON-string parsing from normalization

I do not think normalizeValue should parse JSON-looking strings.

The numeric normalization makes sense because CEL expects stable numeric types, but parsing arbitrary strings that look like JSON changes the user payload type implicitly. For
example, a string value like "{"region":15}" stops being a string and becomes a map only because of its shape. That makes condition behavior harder to reason about and creates
special cases in the referenced-key optimization.

If input or output should be structured, they should be provided as Map values. If they are strings, they should remain strings. With that rule, the JSON-string fallback in
buildPartialTriggerVariables is no longer needed.

Comment 4: fail fast on missing referenced-key metadata

The keySetCache == null / cache-miss fallback to full variable building seems too permissive.

In the normal ActionRouter lifecycle, expression conditions should be initialized through initPrograms(), and the program cache and referenced-key cache should be built
together. If referenced-key metadata is missing for a condition, that looks like a lifecycle/invariant violation rather than a case where we should silently switch to full
activation building.

Since this PR is adding referenced-key optimization, I think missing key metadata should fail fast with a clear error. That keeps the optimized path easier to reason about and
avoids keeping a second full-build behavior only as a defensive fallback.

[wenjin272]

Do we need initPrograms() as a separate lifecycle method? In the production path it is called immediately after constructing ConditionEvaluator, so these two steps seem to define one complete evaluator instance.

Keeping them separate leaves the object in a partially initialized state, forces programCache / keySetCache to be nullable, and requires extra defensive handling for missing cache state. It may be simpler to pass the expression conditions into the constructor or a factory method, build both caches there, and make them final.

[wenjin272]

Is the evaluate() / evaluateProgram() split needed? evaluateProgram() has only one caller and mainly contains the second half of the same evaluation flow. This makes the failure handling split across two methods: evaluate() handles CelEvaluationException, while evaluateProgram() handles non-boolean results.

I think this would be easier to read if the program lookup, evaluation, result type check, and failure-policy handling stayed in evaluate().

[wenjin272]

Why is it necessary to check and forcibly cast the return type of event.getAttributes() here? Isn't the return value of event.getAttributes() already a definite Map<String, Object>?

[wenjin272]

The input / output subkey promotion seems too broad here. This code promotes subkeys whenever the event attributes contain input or output maps, regardless of the actual event type.

If this is intended as a convenience for framework InputEvent / OutputEvent, it should probably be scoped to those event types only. For ordinary user events, input andoutput should remain normal attribute keys, and users can access nested values as input.foo / output.foo.

Please also update resolveMergedKey() consistently. It currently resolves every referenced key by checking output.<key> first, then the root attribute, then input.<key>, for all event types. If subkey promotion is limited to InputEvent / OutputEvent, this lookup order should only apply to the matching framework event type; otherwise a user event with an input or output map can still have its nested fields promoted in the optimized path.

[wenjin272]

/**
 * Returns the actions whose trigger conditions match the event.
 *
 * <p>Actions matched by exact event type are returned before actions matched by expression
 * conditions. Each action appears at most once.
 */

[wenjin272]

List<Action> expressionCandidates = new ArrayList<>();
for (Action action : agentPlan.getActionsWithExpressions()) {
    if (!typedHits.contains(action)) {
        expressionCandidates.add(action);
    }
}

if (expressionCandidates.isEmpty()) {
    return typedHits;
}

[wenjin272]

Can this be a plain ArrayList instead of a LinkedHashSet? actionsByEvent and actionsWithExpressions should not contain duplicate actions, expression candidates are already filtered with !typedHits.contains(a), and each candidate is added at most once because the inner loop breaks after the first matching expression.

Using a set here suggests there is another duplicate path to handle, but under the current plan invariants there should not be one.

[wenjin272]

Do we need the process-wide PROGRAM_CACHE in ConditionExpressionCompiler ConditionEvaluator already builds a per-evaluator programCache during initialization, so the route hot path does not recompile expressions.

The global cache only adds reuse across different evaluators/plans, but it also introduces mutable global state, LRU behavior, and test-only hooks (clearProgramCacheForTests, programCacheSizeForTests). Unless we have a clear cross-plan reuse requirement or performance evidence, it may be simpler to keep only the evaluator-level compiled program map and let toProgram() compile directly.

[wenjin272]

I wonder if we should split exact event-type triggers and expression triggers in the API instead of overloading one value / triggerConditions field for both.

Today the same string list can contain either event type names or boolean expressions, which forces ParsedCondition.classify() to guess the intent, introduces special rules for name-shaped entries and quoted event types, and makes the plan module depend on CEL parsing just to classify trigger entries.

A cleaner API could be something like value for exact event types and conditions for expression triggers. Then event types never need to be parsed as expressions, complex event type names do not need nested quoting like "'com.example.order-created'", and CEL-specific parsing can stay in the runtime expression compiler.