luigidemasi commented on code in PR #27714:
URL: https://github.com/apache/camel/pull/27714#discussion_r4246758674
##########
components/camel-ai/camel-ai-tool/src/test/java/org/apache/camel/component/ai/tool/AiToolAuthorizationDeniedEventTest.java:
##########
@@ -199,6 +231,75 @@ public void notify(CamelEvent event) {
AiToolResult result = AiToolExecutor.execute(spec, Map.of(), exchange);
assertThat(result).isInstanceOf(AiToolResult.AuthorizationDenied.class);
+ // the healthy @BeforeEach notifier still received the event despite
the other one throwing
+ assertThat(events).hasSize(1);
Review Comment:
**P2 — assert receipt by a subscriber registered after the failing one**
The healthy collector is registered first in `@BeforeEach` (lines 119–129);
this test adds the failing notifier afterward. Both have the same ordering, so
the healthy collector already receives its event before the failure occurs.
`events.hasSize(1)` therefore also passes when delivery aborts at the failing
notifier. The prior whole-loop `catch(Throwable)` would preserve the refusal
assertion as well.
Please register a fresh healthy subscriber after the failing notifier and
assert that subscriber's own collection contains the event, while retaining the
`AuthorizationDenied` assertion. Also cover `isEnabled()` throwing alongside
`notify()` throwing.
The rationale is to make the test detect the behavior this helper fixes: one
failing subscriber must not prevent subsequent subscribers from seeing an
audit/monitoring event. The current implementation does isolate those two
callbacks; the issue here is that this test would not catch losing that
behavior.
_AI-assisted review generated by Codex on behalf of Luigi De Masi
(@luigidemasi)._
##########
core/camel-support/src/main/java/org/apache/camel/support/EventHelper.java:
##########
@@ -1574,6 +1576,49 @@ public static boolean
notifyExchangeAsyncProcessingStartedEvent(CamelContext con
return answer;
}
+ /**
+ * Notifies an {@link AiEvent} to the started event notifiers, following
the same path as every other event
+ * category: it loops the started notifiers, skips those that are disabled
or have {@code ignoreAiEvents} set,
+ * creates the event lazily (via {@code eventSupplier}) only when some
notifier wants it, and delivers through
+ * {@code doNotifyEvent} so one failing notifier does not stop delivery to
the others.
+ * <p>
+ * AI events are their own category: {@code ignoreExchangeEvents} does
<em>not</em> suppress them even though an
+ * {@code AiEvent} is an exchange event. A notifier opts out with {@code
setIgnoreAiEvents(true)}.
+ *
+ * @since 4.24
+ */
+ public static boolean notifyAiEvent(CamelContext context,
Supplier<AiEvent> eventSupplier) {
+ ManagementStrategy management = context != null ?
context.getManagementStrategy() : null;
+ if (management == null) {
+ return false;
+ }
+
+ List<EventNotifier> notifiers = management.getStartedEventNotifiers();
+ if (notifiers == null || notifiers.isEmpty()) {
+ return false;
+ }
+
+ boolean answer = false;
+ CamelEvent event = null;
+ // optimise for loop using index access to avoid creating iterator
object
+ for (int i = 0; i < notifiers.size(); i++) {
+ EventNotifier notifier = notifiers.get(i);
+ if (notifier.isDisabled() || notifier.isIgnoreAiEvents()) {
Review Comment:
**P2 — isolate notifier eligibility failures before removing the producer's
protection**
`isDisabled()` and `isIgnoreAiEvents()` are overridable notifier callbacks,
but these calls are outside `doNotifyEvent`'s `Throwable` guards. This PR also
removes [AiToolExecutor's outer notification
guard](https://github.com/apache/camel/blob/131b9fe60539dea808b5318de8ce8a6a76e2ddbd/components/camel-ai/camel-ai-tool/src/main/java/org/apache/camel/component/ai/tool/AiToolExecutor.java#L201).
A concrete fault case is a custom notifier registered after context startup
whose `isDisabled()` throws `AssertionError`.
[Registration](https://github.com/apache/camel/blob/131b9fe60539dea808b5318de8ce8a6a76e2ddbd/core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultManagementStrategy.java#L83)
does not call that getter. When the tool's authorization policy denies access,
publication reaches this check before `AuthorizationDenied` is constructed; the
error escapes instead of returning the refusal, and later subscribers are
skipped.
Please isolate these eligibility checks per notifier too: catch the failure,
log it, skip that notifier and continue. Add coverage with the failing notifier
followed by a healthy subscriber, asserting both `AuthorizationDenied` and
delivery to the later subscriber. Restoring only an outer catch would protect
the refusal but still lose subsequent delivery.
This requires a failing custom notifier; the stock flag getters are simple
Boolean reads. It is a regression in the producer's existing best-effort
notification guarantee, not an authorization bypass. The tool remains denied.
This scenario is source-traced, not runtime-tested.
_AI-assisted review generated by Codex on behalf of Luigi De Masi
(@luigidemasi)._
--
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]
