weiqingy commented on PR #756: URL: https://github.com/apache/flink-agents/pull/756#issuecomment-4637523361
Thanks for the quick turnaround — went through all four threads against the head commit and they're fully resolved. Both legacy-key fallbacks now have a focused test that feeds the old `listen_event_types` key and asserts it surfaces as `getTriggerConditions()` / `trigger_conditions`, so the branch can't be silently dropped in a later cleanup. The `EventType` tests now assert value-equality against each source `EVENT_TYPE` on both sides, which actually guards the single-source-of-truth mapping. The `__init__` fix is the one worth calling out — catching that the custom `__init__` dropped the legacy key before `model_validator(mode="before")` ran turned a coverage gap into a real correctness fix. Nice find. CI is green across the board. One forward-looking question before I'm fully comfortable — not a blocker for this PR: The `getListenEventTypes()` / `listen_event_types` accessor is documented as the future filtering seam — its javadoc in `plan/.../actions/Action.java` says a later change overrides it to "filter out non-type entries" once CEL expressions land. But the `actionsByEvent` / `actions_by_event` routing maps are built through that accessor at only one of three sites: - `AgentPlan.java:207` (user `@Action` methods) iterates the raw `triggerConditions` list directly. - `AgentPlan.java:217` (`addBuiltAction`, built-ins) goes through `action.getListenEventTypes()`. - `agent_plan.py:148` (Python) iterates the raw `action.trigger_conditions` directly. Today the three are equivalent, since every trigger entry is a plain event-type name. The forward-looking concern: once the accessor starts filtering, the two raw-list sites — which are exactly the ones handling user-supplied conditions — would register CEL expression strings like `"type == EventType.ChatResponseEvent && retryCount > 0"` as literal keys in the routing map, which is what the accessor exists to strip. The built-in path that does go through the accessor never carries CEL, so the seam as drawn protects the one path that doesn't need it and bypasses the two that do. I'm confident the three sites diverge today; less sure whether it bites, since that depends on the unmerged CEL PR's design. Would it make sense to route all three through the accessor now — or, equally, defer the accessor and read the raw field everywhere until the CEL PR needs it? Either way, the three sites agreeing seems like the property worth locking in before CEL builds on top. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
