fix: [Examples] - Create a typescript example for openai-agents#171
Open
theRangeCoder wants to merge 6 commits into
Open
Conversation
qualifire-dev#72) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new TypeScript example that wraps an OpenAI Agent in an A2A-compatible Express server: Shirtify agent with two Zod-validated tools, an OpenAIAgentExecutor that streams responses and manages per-context history/cancellation, plus package/tsconfig and server wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Express as Express Server
participant Executor as OpenAIAgentExecutor
participant EventBus as ExecutionEventBus
participant OpenAI as OpenAI Agent
Client->>Express: POST /execute (user message)
Express->>Executor: execute(requestContext, EventBus)
Executor->>EventBus: publish(submitted)
Executor->>Executor: dedupe & append message to history
Executor->>Executor: convert history -> OpenAI input items
Executor->>OpenAI: run(agent, messages) [streaming]
loop on delta chunks
OpenAI-->>Executor: response.output_text.delta
Executor->>EventBus: publish(working with delta)
end
Executor->>Executor: finalize response & save agent message
Executor->>EventBus: publish(completed)
Executor-->>Express: execution finished
Express-->>Client: HTTP response (task update)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
examples/js/openai-agents-example/src/agent.ts (1)
62-73: Consider adding email format validation.The
z.string()without email format validation. While this works for an example, adding.email()would make the validation more robust.Proposed enhancement
const sendEmailTool = tool({ name: 'send_email', description: 'Send an email to a customer', parameters: z.object({ - email: z.string().describe('Email address of the recipient'), + email: z.string().email().describe('Email address of the recipient'), subject: z.string().describe('Email subject'), body: z.string().describe('Email body'), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agent.ts` around lines 62 - 73, The sendEmailTool's parameters currently use z.string() for the email field without format validation; update the parameters schema in sendEmailTool (the call to tool and its parameters z.object) to use z.string().email() (or z.string().email({ message: 'Invalid email' }) for custom message) for the email property so invalid emails are rejected at validation time while leaving subject and body as z.string().examples/js/openai-agents-example/package.json (1)
24-27: Consider removing unusedts-nodedependency.The
devscript usestsx(vianpx tsx), notts-node. Ifts-nodeisn't used elsewhere, it can be removed to reduce dependencies.Proposed fix
"devDependencies": { "@types/express": "^5.0.3", - "ts-node": "^10.9.2", "typescript": "^5.8.3" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/package.json` around lines 24 - 27, The package.json currently lists "ts-node" under devDependencies but the dev script uses "npx tsx" so "ts-node" appears unused; remove the "ts-node" entry from devDependencies (in the same package.json where "typescript" and "@types/express" live) to reduce unnecessary dependencies and update package-lock/yarn.lock by running the appropriate package manager install afterwards.examples/js/openai-agents-example/src/agentExecutor.ts (2)
7-8: Module-levelcontextsMap creates shared state across executor instances.The
contextsMap is declared at module level, meaning allOpenAIAgentExecutorinstances share the same conversation history. This could cause issues if multiple executors are instantiated. Consider making it an instance property.Proposed fix
-// Store for conversation contexts -const contexts = new Map<string, Message[]>(); export class OpenAIAgentExecutor implements AgentExecutor { private cancelledTasks = new Set<string>(); private agent: any; + private contexts = new Map<string, Message[]>(); constructor(agent: any) { this.agent = agent; }Then update all references from
contextstothis.contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` around lines 7 - 8, The module-level Map named contexts creates shared state across all executors; move its declaration into the OpenAIAgentExecutor class as an instance property (e.g., this.contexts = new Map<string, Message[]>() in the constructor) and update every usage of contexts in methods like any method referencing contexts to use this.contexts instead so each OpenAIAgentExecutor has its own conversation history.
12-16: Consider stronger typing foragentparameter.Using
anyfor the agent parameter loses type safety. If theAgenttype is exported from@openai/agents, consider using it.Proposed improvement
+import { Agent, run } from '@openai/agents'; -import { run } from '@openai/agents'; export class OpenAIAgentExecutor implements AgentExecutor { private cancelledTasks = new Set<string>(); - private agent: any; + private agent: Agent; - constructor(agent: any) { + constructor(agent: Agent) { this.agent = agent; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` around lines 12 - 16, The class uses any for the agent field and constructor (private agent: any; constructor(agent: any)) which loses type safety; replace any with the appropriate exported Agent type (or a tailored interface) from `@openai/agents` and update the field and constructor signatures to use that type (e.g., private agent: Agent; constructor(agent: Agent)) and add the import for Agent so callers and IDEs get correct typing for methods/properties on the agent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/js/openai-agents-example/package.json`:
- Line 6: The package.json "main" entry currently points to "index.js" but
TypeScript builds into the dist/ directory; update the "main" field in
package.json so it references "dist/index.js" (change the value of the "main"
key) to match the compiled output and ensure imports/loaders find the built
module.
- Line 19: Update the pinned dependency "@openai/agents" in package.json from
^0.0.14 to the current 0.8.x range (e.g., "^0.8.0") and then change the event
type check that currently looks for event.type === 'raw_response_event' to the
new API event.type === 'raw_model_stream_event' (locate the check using the
event variable and related agent streaming/response handler). Ensure any tests
or handlers that branch on the old string are updated to the new string and run
the project to verify compatibility with the 0.8.x API.
In `@examples/js/openai-agents-example/src/agent.ts`:
- Line 22: Fix the typo in the instruction string by adding the missing "to":
change the sentence "You are not allowed give discounts to customers." to "You
are not allowed to give discounts to customers." in
examples/js/openai-agents-example/src/agent.ts (update the exact string literal
used for the agent/system prompt).
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Line 11: cancelledTasks is accumulating task IDs and never cleared; after you
publish a final status for a task, remove its ID from the cancelledTasks Set to
prevent a memory leak. Locate the places in agentExecutor.ts that publish final
statuses (the publishStatus / publish call sites that pass final: true in the
success, failure, and cancellation branches) and add
cancelledTasks.delete(taskId) immediately after the final publish; apply the
same cleanup in every branch where final: true is published so the Set is pruned
when tasks complete, error, or are cancelled.
- Around line 193-196: Replace the unsafe cast "(stream as any).finalOutput" by
importing the correct stream type from `@openai/agents` that exposes the
finalOutput property (referencing the stream variable and finalOutput) and use
that type for the stream variable so TypeScript knows finalOutput exists; also
confirm from the SDK whether finalOutput is a Promise—if it's a direct property
remove the unnecessary await, otherwise await its Promise—update the import and
the variable declaration/type accordingly to eliminate the any cast.
In `@examples/js/openai-agents-example/src/index.ts`:
- Around line 58-67: The shutdown function calls server.close() then immediately
process.exit(0), which can terminate the process before the HTTP server finishes
draining; update the shutdown implementation (the shutdown function) to wait for
server.close to complete before exiting by either promisifying/awaiting
server.close (or using the provided close callback) and only calling
process.exit(0) after successful close, ensure you handle and log any close
error and add a short timeout fallback to force-exit if close hangs.
- Around line 51-54: The console.log messages when starting the server hardcode
"localhost" while getAgentCard() uses process.env.HOST; fix this by reading the
host into a local variable (e.g., const HOST = process.env.HOST || 'localhost')
and use that HOST along with PORT when calling expressApp.listen and in the
three console.log lines (the server start message, Agent Card URL, and stop
hint) so the logged URLs match the configured host.
---
Nitpick comments:
In `@examples/js/openai-agents-example/package.json`:
- Around line 24-27: The package.json currently lists "ts-node" under
devDependencies but the dev script uses "npx tsx" so "ts-node" appears unused;
remove the "ts-node" entry from devDependencies (in the same package.json where
"typescript" and "@types/express" live) to reduce unnecessary dependencies and
update package-lock/yarn.lock by running the appropriate package manager install
afterwards.
In `@examples/js/openai-agents-example/src/agent.ts`:
- Around line 62-73: The sendEmailTool's parameters currently use z.string() for
the email field without format validation; update the parameters schema in
sendEmailTool (the call to tool and its parameters z.object) to use
z.string().email() (or z.string().email({ message: 'Invalid email' }) for custom
message) for the email property so invalid emails are rejected at validation
time while leaving subject and body as z.string().
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 7-8: The module-level Map named contexts creates shared state
across all executors; move its declaration into the OpenAIAgentExecutor class as
an instance property (e.g., this.contexts = new Map<string, Message[]>() in the
constructor) and update every usage of contexts in methods like any method
referencing contexts to use this.contexts instead so each OpenAIAgentExecutor
has its own conversation history.
- Around line 12-16: The class uses any for the agent field and constructor
(private agent: any; constructor(agent: any)) which loses type safety; replace
any with the appropriate exported Agent type (or a tailored interface) from
`@openai/agents` and update the field and constructor signatures to use that type
(e.g., private agent: Agent; constructor(agent: Agent)) and add the import for
Agent so callers and IDEs get correct typing for methods/properties on the
agent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d50ec2ce-78f3-41e8-971f-12805e87ec2b
📒 Files selected for processing (5)
examples/js/openai-agents-example/package.jsonexamples/js/openai-agents-example/src/agent.tsexamples/js/openai-agents-example/src/agentExecutor.tsexamples/js/openai-agents-example/src/index.tsexamples/js/openai-agents-example/tsconfig.json
- package.json: fix main entry to dist/index.js, bump @openai/agents to ^0.8.0, remove unused ts-node dev dependency
- agent.ts: fix typo in instructions ("not allowed give" → "not allowed to give"), add .email() validation to sendEmailTool
- agentExecutor.ts: move contexts Map from module-level to instance property, type agent as Agent instead of any, import RunResultStreaming and type stream variable, update event type raw_response_event → raw_model_stream_event, remove await from stream.finalOutput (direct property), add cancelledTasks.delete(taskId) after every final status publish to prevent memory leak
- index.ts: extract HOST from env var and use in listen() and console.log, move process.exit(0) into server.close() callback with 5 s force-exit timeout fallback
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/js/openai-agents-example/src/agentExecutor.ts (1)
10-10: Consider boundingcontextsgrowth to avoid unbounded memory.
this.contextsand per-context history only grow. For a long-running server, add trimming and/or inactivity eviction (TTL/LRU) to keep memory stable.Also applies to: 77-82, 208-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` at line 10, The contexts Map (this.contexts) can grow without bound; add bounded growth and eviction: introduce configurable limits (e.g., maxContexts and maxMessagesPerContext) and enforce them whenever you create or append to an entry in this.contexts (trim Message[] to maxMessagesPerContext) and evict least-recently-used or oldest entries when maxContexts is exceeded; additionally add an inactivity TTL and a periodic cleanup job to remove expired context entries; update all places that mutate this.contexts (the code paths that create/append context arrays) to touch the LRU/lastAccess metadata so eviction/TTL works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 84-93: The current guard uses messages.length to detect empty
conversation but messages is a 1:1 map of historyForAgent so empty text contents
slip through; update the logic around the messages construction (the messages
variable built from historyForAgent and TextPart filtering) to first filter out
entries whose content would be empty (i.e., after mapping/collecting text parts)
and then recheck length (or check the filtered array) before hitting the failure
branch; locate the messages creation and the subsequent if (messages.length ===
0) block and ensure you drop items with empty content rather than relying on
array length from historyForAgent.
---
Nitpick comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Line 10: The contexts Map (this.contexts) can grow without bound; add bounded
growth and eviction: introduce configurable limits (e.g., maxContexts and
maxMessagesPerContext) and enforce them whenever you create or append to an
entry in this.contexts (trim Message[] to maxMessagesPerContext) and evict
least-recently-used or oldest entries when maxContexts is exceeded; additionally
add an inactivity TTL and a periodic cleanup job to remove expired context
entries; update all places that mutate this.contexts (the code paths that
create/append context arrays) to touch the LRU/lastAccess metadata so
eviction/TTL works correctly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 52031b91-718d-4f5b-a464-b05d1062c268
📒 Files selected for processing (4)
examples/js/openai-agents-example/package.jsonexamples/js/openai-agents-example/src/agent.tsexamples/js/openai-agents-example/src/agentExecutor.tsexamples/js/openai-agents-example/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- examples/js/openai-agents-example/src/agent.ts
- examples/js/openai-agents-example/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/js/openai-agents-example/src/index.ts
- Filter messages with empty content after map to correctly detect non-text inputs (messages.length was always equal to historyForAgent.length) - Add bounded growth and LRU eviction to this.contexts: - MAX_CONTEXTS = 500, MAX_MESSAGES_PER_CONTEXT = 100, CONTEXT_TTL_MS = 30 min - touchContext() trims history and evicts oldest entry when at capacity - evictExpiredContexts() runs every 5 min via unref'd setInterval - contextLastAccess updated on every read and write Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/js/openai-agents-example/src/agentExecutor.ts (1)
187-187: Remove theas anycast to improve type safety at the SDK boundary.The
messagesvariable is already constructed as an array of objects withroleandcontentfields, which matches theAgentInputItem[]type expected by therun()function from@openai/agents. The cast toanyis unnecessary and prevents TypeScript from catching potential type mismatches during compilation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` at line 187, Remove the unnecessary "as any" cast on the call to run(this.agent, messages as any): ensure messages is typed to the expected AgentInputItem[] (or the specific input type expected by run) and pass it directly to run so TypeScript can validate the structure; update the messages declaration/type or add an appropriate import of AgentInputItem if needed, then change the call to run(this.agent, messages) and keep the RunResultStreaming assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 277-293: The catch block in the OpenAIAgentExecutor that builds
errorUpdate assumes error.message exists; update the handler (in the try/catch
around task processing where taskId, contextId and errorUpdate are created) to
extract a safe error string by checking if error is an instance of Error and
using error.message, otherwise fallback to String(error) or
JSON.stringify(error) (with try/catch) so the parts text becomes a meaningful
message for thrown strings/objects; ensure the updated text assignment replaces
`Agent error: ${error.message}` with this robust extraction logic.
- Around line 191-209: The loop only stops consuming the stream but doesn't
abort the upstream model request; create an AbortController for this run (e.g.,
controller = new AbortController()) and pass controller.signal into
run(this.agent, messages, { signal: controller.signal }) so the SDK can
terminate the request, then when detecting cancellation in the for-await (where
cancelledTasks.has(taskId) is checked) call controller.abort() before publishing
the canceled update and returning; ensure the controller is scoped/stored per
task/run so the correct controller is aborted for the matching taskId.
---
Nitpick comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Line 187: Remove the unnecessary "as any" cast on the call to run(this.agent,
messages as any): ensure messages is typed to the expected AgentInputItem[] (or
the specific input type expected by run) and pass it directly to run so
TypeScript can validate the structure; update the messages declaration/type or
add an appropriate import of AgentInputItem if needed, then change the call to
run(this.agent, messages) and keep the RunResultStreaming assignment unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57c804ed-e337-402d-b415-dc1e17306397
📒 Files selected for processing (1)
examples/js/openai-agents-example/src/agentExecutor.ts
- Import AgentInputItem from @openai/agents and type messages as AgentInputItem[] with explicit role cast; removes the messages as any cast so TypeScript validates the structure at compile time - Add AbortController per run and pass signal to run(); call controller.abort() on mid-stream cancellation so the upstream model request is terminated, not just the event consumer loop (prevents wasted API tokens) - Change catch (error: any) to catch (error: unknown) and extract the message via error instanceof Error ? error.message : String(error) so thrown strings and plain objects produce meaningful output rather than undefined Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 187-188: The run() call is missing streaming configuration and an
await; change the invocation so you await run(...) with the option { stream:
true } to get a StreamedRunResult (update the call that creates controller and
stream where run(this.agent, messages, ...) is used), use the awaited result for
the for-await loop, and ensure you await stream.completed before reading
stream.finalOutput to guarantee finalOutput is available.
- Line 3: Import the provided guard isOpenAIResponsesRawModelStreamEvent from
'@openai/agents' and replace the current ad-hoc check that inspects (event.data
as any).type; use isOpenAIResponsesRawModelStreamEvent(event) to narrow the
event, then verify event.data.type === 'model' and event.data.event.type ===
'response.output_text.delta' before publishing the intermediate "working" update
(i.e., update the streaming handler that currently checks (event.data as
any).type to use this guard and the nested checks instead).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 39f862f2-1f24-425c-ad17-64ef74211135
📒 Files selected for processing (1)
examples/js/openai-agents-example/src/agentExecutor.ts
- Add await and { stream: true } to run() call so it returns a
StreamedRunResult instead of an unresolved Promise; without this the
for-await loop would fail silently
- Import isOpenAIResponsesRawModelStreamEvent from @openai/agents and replace
ad-hoc (event.data as any) checks; the correct nested shape for Responses API
events is event.data.type === 'model' then event.data.event.type ===
'response.output_text.delta' and delta lives at event.data.event.delta —
the old path never matched, so intermediate working updates were never sent
- Add await stream.completed before reading stream.finalOutput to guarantee
finalOutput is set before it is accessed
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/js/openai-agents-example/src/agentExecutor.ts (1)
65-71:⚠️ Potential issue | 🟠 MajorCancellation is not immediate because
cancelTaskcannot abort the active stream.At Line 69, cancellation is only recorded in a
Set. The actual abort happens later at Line 205, but only when thefor awaitloop gets another event. If no next event arrives promptly, the run can keep consuming tokens after cancellation.Proposed fix
export class OpenAIAgentExecutor implements AgentExecutor { private cancelledTasks = new Set<string>(); + private activeControllers = new Map<string, AbortController>(); @@ public cancelTask = async ( taskId: string, eventBus: ExecutionEventBus, ): Promise<void> => { this.cancelledTasks.add(taskId); + this.activeControllers.get(taskId)?.abort(); // The execute loop is responsible for publishing the final state }; @@ - const controller = new AbortController(); + const controller = new AbortController(); + this.activeControllers.set(taskId, controller); const stream = await run(this.agent, messages, { stream: true, signal: controller.signal, }); @@ - eventBus.publish(finalUpdate); - this.cancelledTasks.delete(taskId); + eventBus.publish(finalUpdate); + this.cancelledTasks.delete(taskId); @@ } catch (error: unknown) { @@ eventBus.publish(errorUpdate); this.cancelledTasks.delete(taskId); + } finally { + this.activeControllers.delete(taskId); } } }#!/bin/bash # Verify cancellation wiring and whether cancelTask can directly abort active runs. rg -n -C3 "cancelTask|AbortController|activeControllers|controller\\.abort\\(|for await \\(const event of stream\\)" examples/js/openai-agents-example/src/agentExecutor.tsAlso applies to: 192-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` around lines 65 - 71, cancelTask currently only marks taskId in cancelledTasks and cannot abort an in-progress token stream; update cancelTask to look up the AbortController for that task (the activeControllers map used where the stream is created and consumed in the for await (const event of stream) loop), call controller.abort() immediately, add the taskId to cancelledTasks, and perform the same cleanup/publish-final-state steps the execute loop does (remove controller from activeControllers and publish the cancellation event to eventBus) so cancellation is immediate and the stream stops consuming tokens.
🧹 Nitpick comments (1)
examples/js/openai-agents-example/src/agentExecutor.ts (1)
21-27: Consider adding explicit timer teardown for executor lifecycle hygiene.
cleanupTimeris started in the constructor and never cleared. Adispose()/close()method (called by server shutdown/tests) would prevent lingering intervals in non-prod flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js/openai-agents-example/src/agentExecutor.ts` around lines 21 - 27, The constructor starts a recurring cleanupTimer (set in the constructor and unref'd) but never clears it; add a public dispose() (or close()) method on AgentExecutor (or the class containing constructor/cleanupTimer) that calls clearInterval(this.cleanupTimer) and sets this.cleanupTimer = undefined (or null) and ensure any callers (shutdown/tests) call dispose(); also update any relevant tests or server shutdown hook to call dispose(), and consider making dispose idempotent and calling it from a finalizer if needed; refer to cleanupTimer, constructor, and evictExpiredContexts to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 104-123: Move the pre-run cancellation gate so you check
cancellation before creating/publishing the initial "working" status;
specifically, before you construct workingStatusUpdate and call
eventBus.publish(workingStatusUpdate) (the block that uses uuidv4(), taskId and
contextId), call the existing pre-run cancellation check (the same check used
later around the pre-run cancellation logic) and abort/publish "canceled" if
needed; apply the same reorder to the other similar block mentioned (the 171-189
region) so no task ever emits "working" before its cancellation state is
verified.
---
Duplicate comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 65-71: cancelTask currently only marks taskId in cancelledTasks
and cannot abort an in-progress token stream; update cancelTask to look up the
AbortController for that task (the activeControllers map used where the stream
is created and consumed in the for await (const event of stream) loop), call
controller.abort() immediately, add the taskId to cancelledTasks, and perform
the same cleanup/publish-final-state steps the execute loop does (remove
controller from activeControllers and publish the cancellation event to
eventBus) so cancellation is immediate and the stream stops consuming tokens.
---
Nitpick comments:
In `@examples/js/openai-agents-example/src/agentExecutor.ts`:
- Around line 21-27: The constructor starts a recurring cleanupTimer (set in the
constructor and unref'd) but never clears it; add a public dispose() (or
close()) method on AgentExecutor (or the class containing
constructor/cleanupTimer) that calls clearInterval(this.cleanupTimer) and sets
this.cleanupTimer = undefined (or null) and ensure any callers (shutdown/tests)
call dispose(); also update any relevant tests or server shutdown hook to call
dispose(), and consider making dispose idempotent and calling it from a
finalizer if needed; refer to cleanupTimer, constructor, and
evictExpiredContexts to locate the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 660b1b79-6431-4f8d-9b5a-da46d2ec9f0d
📒 Files selected for processing (1)
examples/js/openai-agents-example/src/agentExecutor.ts
- Add activeControllers Map; cancelTask() now calls controller.abort() immediately so the upstream model request is terminated without waiting for the next stream event - activeControllers.set(taskId, controller) registered before run(); finally block calls activeControllers.delete(taskId) on every exit path - Move pre-run cancellation check to before the "working" status publish so a cancelled task never briefly appears as working then cancelled - Add public dispose() that clears the cleanup setInterval; call it from index.ts shutdown inside server.close() callback for clean executor teardown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
Author
|
Hey @drorIvry @yuval-qf — this PR is ready for a final review and merge. Five rounds of CodeRabbit feedback have been addressed across the implementation. Here's a summary of what was hardened beyond the initial working example: Correctness
Cancellation
Memory safety
Type safety
Lifecycle
All CodeRabbit actionable comments (Critical/Major) are resolved. Remaining open threads are nitpicks that have been resolved via |
Contributor
|
Review rate limit: 8/8 reviews remaining, available now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
TypeScript example for an agent built with OpenAI's Agents SDK (
@openai/agents), wrapped in A2A.Why
Identified via issue #72 in qualifire-dev/rogue.
Fix
Added
examples/js/openai-agents-example/following the exact structure of the existinglanggraph-js-exampleandvercel-ai-example:src/agent.ts— Shirtify t-shirt store agent defined with@openai/agents(Agent,tool, zod schemas)src/agentExecutor.ts—OpenAIAgentExecutorimplementing the A2AAgentExecutorinterface; streamsresponse.output_text.deltaevents as intermediate A2A status updatessrc/index.ts— Express server wiring upDefaultRequestHandler,InMemoryTaskStore, and the executorpackage.json— same scripts (dev,build,start) and package manager as sibling examplestsconfig.json— identical to sibling examplesTesting
langgraph-js-exampleandvercel-ai-exampleCloses #72