feat(server): make DefaultExecutionEventBus cross-runtime compatible #100

lingkaix · 2025-08-07T11:57:43Z

Description

The original DefaultExecutionEventBus implementation was bound to Node.js. I refactored it to be cross-runtime compatible. So that it works on Node.js, Bun, Deno, serverless runtimes like Cloudflare Worker, and even Browsers.

Follow the CONTRIBUTING Guide.
Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
- Important Prefixes for release-please:
  - fix: which represents bug fixes, and correlates to a SemVer patch.
  - feat: represents a new feature, and correlates to a SemVer minor.
  - feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
Ensure the tests and linter pass
Appropriate docs were updated (if necessary)

… runtimes

gemini-code-assist

Summary of Changes

Hello @lingkaix, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've refactored the DefaultExecutionEventBus to ensure it can operate seamlessly across various JavaScript runtimes, including Node.js, Bun, Deno, serverless environments like Cloudflare Workers, and even browsers. This change addresses its previous dependency on Node.js's EventEmitter, making the event bus truly universal.

Highlights

Cross-Runtime Compatibility: The DefaultExecutionEventBus has been refactored to remove its reliance on Node.js's EventEmitter, making it usable in diverse JavaScript environments.
Custom Event Handling: A custom event listener mechanism has been implemented within DefaultExecutionEventBus using Map and Set to manage event listeners, replacing the inherited EventEmitter functionality.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

lingkaix · 2025-08-07T11:59:16Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors DefaultExecutionEventBus to be cross-runtime compatible by removing its dependency on Node.js's EventEmitter. The new implementation is self-contained and uses standard Map and Set objects to manage event listeners. The change is well-implemented, but I found a critical issue in the finished method's implementation which passes an unsafe and incorrect object to listeners, subtly changing behavior from the previous implementation. My review includes a suggestion to fix this to align with the previous behavior and ensure type safety.

gemini-code-assist · 2025-08-07T11:59:24Z

src/server/events/execution_event_bus.ts

+          // For finished event, we don't pass an event object
+          listener({} as AgentExecutionEvent);


The implementation of the finished event is unsafe and its behavior has subtly changed. The listener is called with {} as AgentExecutionEvent, which is not a valid AgentExecutionEvent and will cause runtime errors for listeners that inspect the event object. The kind property, present on all types within AgentExecutionEvent, will be undefined.

The previous EventEmitter implementation would call the listener with no arguments (i.e., undefined for the first parameter). Calling with an empty object {} changes this behavior, which could break listeners that check for the event's existence (e.g., if (event)).

To maintain previous behavior and avoid unsafe type casting, it's better to call the listener without arguments. This requires a cast to any to bypass the strict type signature, which also highlights a potential design issue in the ExecutionEventBus interface where finished listeners are expected to receive an AgentExecutionEvent.

Suggested change

// For finished event, we don't pass an event object

listener({} as AgentExecutionEvent);

// For finished event, we don't pass an event object, matching previous EventEmitter behavior.

(listener as any)();

As suggested, lets make the event attribute as optional.

gemini-code-assist

Code Review

This pull request refactors DefaultExecutionEventBus to remove its dependency on the Node.js events module, making it compatible with various JavaScript runtimes like Deno, Bun, and browsers. The new implementation is self-contained and uses a Map and Set to manage event listeners.

The refactoring is well-done, but I've identified a type safety issue in the handling of the finished event. The current implementation passes an empty object cast as an AgentExecutionEvent, which violates the type contract and could lead to runtime errors. I've provided suggestions to make the event handling more robust and type-safe by adjusting the EventListener type and how parameterless events are dispatched.

gemini-code-assist · 2025-08-07T12:00:55Z

src/server/events/execution_event_bus.ts

-    | Task
-    | TaskStatusUpdateEvent
-    | TaskArtifactUpdateEvent;
+export type EventListener = (event: AgentExecutionEvent) => void;


To safely handle events that don't have a payload, like the finished event, the EventListener type should be updated to make the event parameter optional. This prevents type-safety issues when dispatching parameter-less events and makes the API more robust.

Suggested change

export type EventListener = (event: AgentExecutionEvent) => void;

export type EventListener = (event?: AgentExecutionEvent) => void;

gemini-code-assist · 2025-08-07T12:00:55Z

src/server/events/execution_event_bus.ts

+          // For finished event, we don't pass an event object
+          listener({} as AgentExecutionEvent);


Casting an empty object {} to AgentExecutionEvent is not type-safe and can lead to runtime errors in consumers of this event bus, as it lacks the kind discriminator property. The finished event is a signal and should not carry a payload. In conjunction with making the event parameter optional in EventListener, this should be changed to call the listener without any arguments.

Suggested change

// For finished event, we don't pass an event object

listener({} as AgentExecutionEvent);

// For finished event, we don't pass an event object

listener();

gemini-code-assist · 2025-08-07T12:00:55Z

src/server/events/execution_event_bus.ts

+  }
+
+  once(eventName: "event" | "finished", listener: EventListener): this {
+    const onceWrapper = (event: AgentExecutionEvent) => {


To align with the proposed change to EventListener (making the event parameter optional), the onceWrapper function should also handle an optional event argument to maintain type consistency across the event bus implementation.

Suggested change

const onceWrapper = (event: AgentExecutionEvent) => {

const onceWrapper = (event?: AgentExecutionEvent) => {

lingkaix · 2025-08-07T12:25:14Z

The suggestion from Gemini maybe alright. But we need to modify ExecutionEventBus to be like:
export interface ExecutionEventBus { publish(event: AgentExecutionEvent): void; on(eventName: "event", listener: (event: AgentExecutionEvent) => void): this; on(eventName: "finished", listener: () => void): this; off(eventName: "event", listener: (event: AgentExecutionEvent) => void): this; off(eventName: "finished", listener: () => void): this; once(eventName: "event", listener: (event: AgentExecutionEvent) => void): this; once(eventName: "finished", listener: () => void): this; removeAllListeners(eventName?: "event" | "finished"): this; finished(): void; }. Does this make sense?

Also, I appreciate if anyone would like to discuss and clarify:

the scoop and usage of 'on', 'off' and 'once'
better error handling
maybe use interaction check to avoid copying listeners for better performance

Thank you!

swapydapy · 2025-08-25T21:48:23Z

Thanks for raising this PR. Will review it this week.

swapydapy · 2025-09-03T20:41:38Z

src/server/events/execution_event_bus.ts

+  publish(event: AgentExecutionEvent): void;
+  on(eventName: "event" | "finished", listener: EventListener): this;
+  off(eventName: "event" | "finished", listener: EventListener): this;
+  once(eventName: "event" | "finished", listener: EventListener): this;


Let's remove this once method. I don't this is used.

swapydapy · 2025-09-03T20:41:58Z

src/server/events/execution_event_bus.ts

-    | Task
-    | TaskStatusUpdateEvent
-    | TaskArtifactUpdateEvent;
+export type EventListener = (event: AgentExecutionEvent) => void;


swapydapy · 2025-09-03T20:45:42Z

src/server/events/execution_event_bus.ts

+          // For finished event, we don't pass an event object
+          listener({} as AgentExecutionEvent);


As suggested, lets make the event attribute as optional.

swapydapy · 2025-09-03T20:46:57Z

src/server/events/execution_event_bus.ts

+  }
+
+  publish(event: AgentExecutionEvent): void {
+    const listeners = this.eventListeners.get("event");


the following code is copied across both publish & finished. Can we just encapsulate this as _emit().

simon@a1 added 2 commits August 7, 2025 11:17

feat(server): make DefaultExecutionEventBus to be compatible multiple…

b56eef0

… runtimes

refactor(server): unified EventListener type

12ba26f

gemini-code-assist bot reviewed Aug 7, 2025

View reviewed changes

Merge branch 'main' into main

fb612a4

swapydapy requested changes Sep 3, 2025

View reviewed changes

		// For finished event, we don't pass an event object
		listener({} as AgentExecutionEvent);

	export type EventListener = (event: AgentExecutionEvent) => void;
	export type EventListener = (event?: AgentExecutionEvent) => void;

	const onceWrapper = (event: AgentExecutionEvent) => {
	const onceWrapper = (event?: AgentExecutionEvent) => {

feat(server): make DefaultExecutionEventBus cross-runtime compatible #100

Are you sure you want to change the base?

feat(server): make DefaultExecutionEventBus cross-runtime compatible #100

Uh oh!

Conversation

lingkaix commented Aug 7, 2025

Description

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

lingkaix commented Aug 7, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

lingkaix commented Aug 7, 2025

Uh oh!

swapydapy commented Aug 25, 2025

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

swapydapy Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants