Add support for context.waitUntil #179
Conversation
WalkthroughAdds a waitUntil batching handler with OpenTelemetry spans for background work, wires it into AgentContext and router flow (marks responses pending when tasks remain), exposes isIdle via new /_idle endpoints, adds markSessionCompleted API, plus assorted comment/formatting and test parameter renames. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router
participant Handler
participant AgentCtx as AgentContext
participant Waiter as AgentContextWaitUntilHandler
participant OTel
participant API as SessionAPI
note right of Waiter #E8F5E9: enqueue tasks, start spans
Client->>Router: HTTP request
Router->>Handler: invoke handler(ctx with waitUntil)
Handler->>AgentCtx: ctx.waitUntil(() => background task)
AgentCtx->>Waiter: enqueue task (increment running)
Handler-->>Router: return response
alt Waiter.hasPending()
Router-->>Client: Response + x-agentuity-session-pending header
else
Router-->>Client: Response
end
Router->>Waiter: waitUntilAll(logger, sessionId)
loop for each queued task
Waiter->>OTel: startSpan("waitUntil")
Waiter->>Handler: execute task within span context
Handler-->>Waiter: resolve / reject
Waiter->>OTel: setStatus / recordException
Waiter->>OTel: endSpan
end
Waiter->>API: markSessionCompleted(sessionId, duration)
API-->>Waiter: 202 Accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/node.ts (1)
159-186: Missing return after 404 on '/welcome/' can double‑write responseIf no content is found, the handler writes 404 and continues, risking headers/body being sent again in the main path.
res.writeHead(404); res.end(); + return;
🧹 Nitpick comments (13)
src/io/email.ts (1)
471-527: Avoid async Promise executor; simplify with awaited callback wrapperUsing
new Promise(async ...)is an anti‑pattern and can mask errors. Refactor to await a promise‑wrappedbuildcallback instead.- // biome-ignore lint/suspicious/noAsyncPromiseExecutor: needed for complex async email operations - return new Promise<string>(async (resolve, reject) => { - try { - let attachments: Attachment[] = []; - if (reply.attachments) { - attachments = await Promise.all( - reply.attachments.map(async (attachment) => { - const resp = await fromDataType(attachment.data); - return { - filename: attachment.filename, - content: await resp.data.buffer(), - contentType: resp.data.contentType, - contentDisposition: - attachment.contentDisposition ?? ('attachment' as const), - }; - }) - ); - } - const mail = new MailComposer({ - inReplyTo: this.messageId() ?? undefined, - references: this.messageId() ?? undefined, - date: new Date(), - from: { - name: from?.name ?? context.agent.name, - address: from?.email ?? this.toEmail() ?? '', - }, - to: { - name: this.fromName() ?? undefined, - address: this.fromEmail() ?? undefined, - } as Address, - subject: this.makeReplySubject(reply.subject), - text: reply.text, - html: reply.html, - attachments, - }); - const newemail = mail.compile(); - newemail.build(async (err, message) => { - if (err) { - reject(err); - } else { - try { - await context.email.sendReply( - context.agent.id, - message.toString(), - authToken, - newemail.messageId() - ); - resolve(newemail.messageId()); - } catch (ex) { - reject(ex); - } - } - }); - } catch (ex) { - reject(ex); - } - }); + // Build message and send without async Promise executor + let attachments: Attachment[] = []; + if (reply.attachments) { + attachments = await Promise.all( + reply.attachments.map(async (attachment) => { + const resp = await fromDataType(attachment.data); + return { + filename: attachment.filename, + content: await resp.data.buffer(), + contentType: resp.data.contentType, + contentDisposition: attachment.contentDisposition ?? 'attachment', + }; + }) + ); + } + const mail = new MailComposer({ + inReplyTo: this.messageId() ?? undefined, + references: this.messageId() ?? undefined, + date: new Date(), + from: { + name: from?.name ?? context.agent.name, + address: from?.email ?? this.toEmail() ?? '', + }, + to: { + name: this.fromName() ?? undefined, + address: this.fromEmail() ?? undefined, + } as Address, + subject: this.makeReplySubject(reply.subject), + text: reply.text, + html: reply.html, + attachments, + }); + const newemail = mail.compile(); + const message = await new Promise<Buffer>((resolve, reject) => { + newemail.build((err, msg) => { + if (err) { + reject(err); + return; + } + resolve(msg); + }); + }); + await context.email.sendReply( + context.agent.id, + message.toString(), + authToken, + newemail.messageId() + ); + return newemail.messageId();src/io/teams/index.ts (3)
12-15: Prefer unknown over any for payload shapeTypes align with guideline; payload is still dynamic.
type parseConfigResult = { config: Record<string, string>; - // biome-ignore lint/suspicious/noExplicitAny: Teams payload can contain various data types - justPayload: Record<string, any>; + justPayload: Record<string, unknown>; mode: Mode; };
17-21: Use unknown for incoming payload paramKeeps flexibility without opting out of type safety.
-const parseConfig = ( - // biome-ignore lint/suspicious/noExplicitAny: Teams payload structure is dynamic - payload: any, +const parseConfig = ( + // Teams payload structure is dynamic + payload: unknown, metadata: JsonObject ): parseConfigResult => {
25-27: Narrow internal var to Record<string, unknown>Still permissive, avoids
any.- // biome-ignore lint/suspicious/noExplicitAny: payload content varies by Teams event type - let justPayload: Record<string, any>; + // payload content varies by Teams event type + let justPayload: Record<string, unknown>;src/server/util.ts (1)
348-353: Redundant deletion of metadata.triggerYou already delete
metadata.triggerinside the conditional; the extra delete is unnecessary.if ('trigger' in metadata) { trigger = metadata.trigger as TriggerType; delete metadata.trigger; } - delete metadata.trigger;src/io/teams/AgentuityTeamsAdapter.ts (4)
62-64: Replaceas anywith the constructor’s parameter typeAvoid
any. Cast to the constructor param type to satisfy TS without disabling lint.- } as any); + } as ConstructorParameters< + typeof ConfigurationBotFrameworkAuthentication + >[0]);
85-96: Preferunknownand a typed mock request; verify headers sourceUse
unknownfor payload and a minimal request type instead ofany. Also, please verifymetadata.metadatavsmetadata.headersis correct for cloud vs dev.- // biome-ignore lint/suspicious/noExplicitAny: Teams payload structure varies - const teamsPayload = (await this.req.data.json()) as any; - // biome-ignore lint/suspicious/noExplicitAny: mock Restify request object - const mockRestifyReq: any = { + const teamsPayload: unknown = await this.req.data.json(); + const mockRestifyReq: { + method: 'POST'; + body: unknown; + headers: Record<string, string>; + } = { method: 'POST', body: teamsPayload, - headers: - this.mode === 'cloud' - ? this.req.metadata.metadata - : this.req.metadata.headers, + headers: + this.mode === 'cloud' + ? (this.req.metadata.metadata as Record<string, string>) + : (this.req.metadata.headers as Record<string, string>), };
97-107: Strongly type the mock Restify responseDrop
anyin favor of a minimal typed shape that matches what CloudAdapter uses.- // biome-ignore lint/suspicious/noExplicitAny: mock Restify response object - const mockRestifyRes: any = { + const mockRestifyRes: { + status: (code: number) => { send: (body: unknown) => void }; + end: () => void; + header: () => void; + send: (body?: unknown) => void; + } = { status: (_code: number) => { return { send: (_body: unknown) => {}, }; }, end: () => {}, header: () => {}, send: (_body?: unknown) => {}, };
112-114: Type the TurnContext instead ofanyUse the SDK’s TurnContext type.
- // biome-ignore lint/suspicious/noExplicitAny: Bot Framework TurnContext typing - async (context: any) => { + async (context: TurnContext) => {Add this import at the top of the file:
import type { TurnContext } from 'botbuilder';src/server/server.ts (1)
53-55: Avoidanyfor dynamic import; useunknown+ type guardsRefactor to
unknownand narrow where needed.- // biome-ignore lint/suspicious/noExplicitAny: dynamic module loading requires any - let mod: any; + let mod: unknown;Then narrow below using safe guards (outside this hunk):
const imported = mod as Record<string, unknown>; let thehandler: AgentHandler | undefined; let thewelcome: AgentWelcome | undefined; const maybeDefault = (imported as { default?: unknown }).default; if (typeof maybeDefault === 'function') { thehandler = maybeDefault as AgentHandler; } else { for (const key of Object.keys(imported)) { const val = imported[key]; if (key !== 'default' && typeof val === 'function') { thehandler = val as AgentHandler; break; } } } for (const key of Object.keys(imported)) { const val = imported[key]; if (key === 'welcome' && typeof val === 'function') { thewelcome = val as AgentWelcome; break; } }test/io/email.test.ts (1)
2-2: Nit: drop trailing comma in importMinor tidiness to match import style.
-import { parseEmail, } from '../../src/io/email'; +import { parseEmail } from '../../src/io/email';src/types.ts (1)
393-401: Single VectorSearchParams declaration confirmed — clean up stray diff markersOne exported interface found at src/types.ts:361. The "-export..." and "+export..." lines at src/types.ts:394–395 are leftover diff markers inside the JSDoc (not active duplicate declarations); remove them to avoid confusion.
src/router/context.ts (1)
5-6: Tighten types and immutability
- Store only async tasks to simplify consumption.
- Mark tracer as readonly.
- private promises: (() => void | Promise<void>)[]; - private tracer: Tracer; + private promises: Array<() => Promise<void>>; + private readonly tracer: Tracer;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.changeset/ten-ghosts-wash.md(1 hunks)src/autostart/index.ts(1 hunks)src/io/email.ts(1 hunks)src/io/teams/AgentuityTeamsActivityHandler.ts(1 hunks)src/io/teams/AgentuityTeamsAdapter.ts(4 hunks)src/io/teams/index.ts(1 hunks)src/otel/index.ts(2 hunks)src/otel/logger.ts(1 hunks)src/router/context.ts(1 hunks)src/router/router.ts(4 hunks)src/server/node.ts(2 hunks)src/server/server.ts(1 hunks)src/server/util.ts(2 hunks)src/types.ts(3 hunks)test/apis/keyvalue-compression.test.ts(2 hunks)test/apis/objectstore.test.ts(5 hunks)test/apis/vector.test.ts(11 hunks)test/io/email.test.ts(1 hunks)test/logger/console.test.ts(1 hunks)test/mocks/opentelemetry.ts(1 hunks)test/router/request.test.ts(1 hunks)test/server/context.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/server/{server,bun,node,agents}.ts
📄 CodeRabbit inference engine (AGENT.md)
Server components live in src/server/ as server.ts, bun.ts, node.ts, and agents.ts
Files:
src/server/server.tssrc/server/node.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/server/server.tssrc/io/teams/AgentuityTeamsActivityHandler.tssrc/io/teams/index.tssrc/otel/logger.tstest/mocks/opentelemetry.tstest/logger/console.test.tssrc/io/teams/AgentuityTeamsAdapter.tssrc/io/email.tssrc/router/context.tssrc/types.tssrc/server/node.tstest/server/context.test.tstest/apis/vector.test.tstest/router/request.test.tssrc/server/util.tstest/apis/objectstore.test.tssrc/otel/index.tstest/apis/keyvalue-compression.test.tssrc/autostart/index.tssrc/router/router.tstest/io/email.test.ts
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/teams/AgentuityTeamsActivityHandler.tssrc/io/teams/index.tssrc/io/teams/AgentuityTeamsAdapter.tssrc/io/email.ts
src/{logger,otel}/**
📄 CodeRabbit inference engine (AGENT.md)
Infrastructure code goes under src/logger/ and src/otel/ (OpenTelemetry)
Files:
src/otel/logger.tssrc/otel/index.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/mocks/opentelemetry.tstest/logger/console.test.tstest/server/context.test.tstest/apis/vector.test.tstest/router/request.test.tstest/apis/objectstore.test.tstest/apis/keyvalue-compression.test.tstest/io/email.test.ts
src/router/**
📄 CodeRabbit inference engine (AGENT.md)
Router code (request/response handling, streaming) resides in src/router/
Files:
src/router/context.tssrc/router/router.ts
🧠 Learnings (3)
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to {src,test}/**/!(*.d).ts : Use ESM import/export syntax; avoid CommonJS require/module.exports
Applied to files:
test/server/context.test.ts
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/server/{server,bun,node,agents}.ts : Server components live in src/server/ as server.ts, bun.ts, node.ts, and agents.ts
Applied to files:
test/server/context.test.ts
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/router/** : Router code (request/response handling, streaming) resides in src/router/
Applied to files:
src/router/router.ts
🧬 Code graph analysis (3)
src/io/teams/index.ts (2)
src/router/request.ts (1)
metadata(55-57)src/types.ts (1)
JsonObject(216-218)
src/otel/index.ts (2)
src/otel/logger.ts (1)
error(105-120)src/logger/console.ts (1)
error(153-162)
src/router/router.ts (1)
src/router/context.ts (1)
AgentContextWaitUntilHandler(4-55)
🔇 Additional comments (21)
test/logger/console.test.ts (1)
29-37: Underscore‑prefix for unused error param is fineNo behavior change; keeps linter quiet in tests.
src/io/teams/AgentuityTeamsActivityHandler.ts (1)
10-12: Comment tweak acknowledgedThe lint‑ignore rationale is clear and appropriate for prototype inspection.
src/otel/logger.ts (1)
234-236: Global console patch comment is fineNo functional changes; keep as‑is.
src/autostart/index.ts (1)
56-60: YAML parse note is acceptableClarifies the
anyusage with a lint ignore; no behavior change.src/server/node.ts (2)
35-36: Field reorder is fineNo functional impact.
103-106: Local var init style change is fineNo behavior change.
src/server/util.ts (2)
17-28: Safe stringify param rename is fineBehavior unchanged; handles circular refs.
30-39: Safe parse catch param rename is fineNo behavior change.
.changeset/ten-ghosts-wash.md (1)
1-6: Changeset looks goodClear, concise summary for a patch release.
test/server/context.test.ts (1)
1-1: LGTM on import cleanupRemoving unused
mocktightens the test surface.test/mocks/opentelemetry.ts (1)
27-29: Good underscore‑prefix for unused paramsConforms to linting without changing behavior.
src/otel/index.ts (1)
249-253: Non-blocking: structured warn looks goodNice fallback when Traceloop isn’t available.
test/router/request.test.ts (1)
4-4: LGTM on unused type removalType imports are lean and accurate.
test/apis/keyvalue-compression.test.ts (2)
30-42: LGTM — unused param renames in GET mock are fineNo behavior change; aligns with underscore convention across tests.
68-72: LGTM — OpenTelemetry context.with mock signature renameConsistent with test-suite wide updates.
test/apis/vector.test.ts (1)
9-18: LGTM — underscore-prefixed unused params across tracer and API mocksConsistent, no functional change.
Also applies to: 29-31, 68-71, 91-94, 120-123, 146-149, 172-176, 214-216, 239-241, 265-267
test/apis/objectstore.test.ts (1)
10-19: LGTM — unused param renames in tracer, OTEL, headers, and POST mocksMatches suite-wide convention; behavior unchanged.
Also applies to: 28-29, 31-33, 50-51, 256-260, 281-285
src/types.ts (2)
722-726: LGTM — WaitUntilCallback public type looks goodSignature matches the implementation pattern (capture ctx at enqueue time).
799-805: LGTM — AgentContext.waitUntil added with clear docsAPI shape aligns with the handler and router wiring.
src/router/router.ts (2)
290-293: LGTM — per-request AgentContextWaitUntilHandler instantiationScoped correctly to the request; no shared state across requests.
323-324: LGTM — waitUntil wired into AgentContextBound method preserves instance/context; matches the public type.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/io/email.ts (1)
182-204: Add an explicit timeout when fetching attachmentsFound: send supports a request.timeout (it sets signal: AbortSignal.timeout(request.timeout)) but applies no default; src/io/email.ts currently calls send({ url: this._url, method: 'GET' }, true) with no timeout (src/io/email.ts:203). Pass a reasonable timeout (e.g., 10000–30000 ms). Example: send({ url: this._url, method: 'GET', timeout: 15000 }, true)
🧹 Nitpick comments (6)
src/io/email.ts (2)
97-125: Fail-closed on DNS lookup errors to harden SSRF guardIf DNS resolution throws, we should treat the host as unsafe and block, rather than returning false and attempting the fetch anyway.
- } catch { - return false; - } + } catch { + // Fail-closed: treat resolution failures as unsafe + return true; + }
49-69: IPv6 range checks look good; consider library-based parsing for robustnessString-prefix checks for ranges like fe80::/10 work but are brittle. Consider a small IP parsing lib (e.g., ipaddr.js) for clearer, safer range checks if adding more CIDRs later.
src/router/router.ts (1)
267-295: Validate sessionId choice; prefer stable runId for correlationYou set
const sessionId = \sess_${span.spanContext().traceId}`. Confirm the backend expects this format. If the system’s notion of a session is alreadyrunId, usingrunId` directly may be safer for end-to-end correlation.- const sessionId = `sess_${span.spanContext().traceId}`; + const sessionId = runId;src/server/bun.ts (1)
113-120: Idle endpoint LGTM; consider explicit headersRoute works as intended. Optionally add
Content-Type: text/plainandCache-Control: no-storeto avoid caches lying about idleness.src/router/context.ts (2)
16-16: Tighten the task queue type.These are always async wrappers; make the type Promise-returning for clarity and safer usage.
- private promises: (() => void | Promise<void>)[]; + private promises: Array<() => Promise<void>>;
30-30: Use standard comment style (avoid triple-slash in code).Triple-slash is reserved for TS directives; use //.
- this.started = Date.now(); /// this first execution marks the start time + this.started = Date.now(); // this first execution marks the start time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/apis/keyvalue.ts(1 hunks)src/apis/session.ts(1 hunks)src/io/email.ts(11 hunks)src/io/slack.ts(2 hunks)src/logger/console.ts(1 hunks)src/otel/logger.ts(2 hunks)src/router/context.ts(1 hunks)src/router/data.ts(1 hunks)src/router/router.ts(5 hunks)src/server/agents.ts(3 hunks)src/server/bun.ts(2 hunks)src/server/node.ts(4 hunks)test/apis/api.test.ts(1 hunks)test/apis/keyvalue.test.ts(3 hunks)test/io/email.test.ts(19 hunks)test/server/context.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/apis/keyvalue.ts
- src/server/agents.ts
- src/logger/console.ts
- src/router/data.ts
- test/apis/keyvalue.test.ts
- src/io/slack.ts
- test/apis/api.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/server/context.test.ts
- src/otel/logger.ts
- src/server/node.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/server/{server,bun,node,agents}.ts
📄 CodeRabbit inference engine (AGENT.md)
Server components live in src/server/ as server.ts, bun.ts, node.ts, and agents.ts
Files:
src/server/bun.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/server/bun.tssrc/apis/session.tssrc/router/router.tssrc/router/context.tssrc/io/email.tstest/io/email.test.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/session.ts
src/router/**
📄 CodeRabbit inference engine (AGENT.md)
Router code (request/response handling, streaming) resides in src/router/
Files:
src/router/router.tssrc/router/context.ts
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/io/email.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/router/** : Router code (request/response handling, streaming) resides in src/router/
Applied to files:
src/router/router.ts
🧬 Code graph analysis (5)
src/server/bun.ts (1)
src/router/context.ts (1)
isIdle(11-13)
src/apis/session.ts (1)
src/apis/api.ts (1)
POST(191-206)
src/router/router.ts (1)
src/router/context.ts (1)
AgentContextWaitUntilHandler(15-65)
src/router/context.ts (2)
src/logger/logger.ts (1)
Logger(4-44)src/apis/session.ts (1)
markSessionCompleted(6-18)
test/io/email.test.ts (1)
src/io/email.ts (1)
attachments(374-446)
🔇 Additional comments (9)
test/io/email.test.ts (4)
2-2: LGTM: import cleanupImporting only
parseEmailmatches actual usage and keeps tests focused.
36-39: Good edge-case coverage for Slack-style filenamesThis ensures non-URL filenames don’t cause hard failures.
365-374: Nice coverage of private IPv4 and special addresses (incl. 0.0.0.0)Covers the expected blocks aligned with the code’s checks.
402-410: Good IPv6 blocklist tests (incl. ::ffff mapped IPv4)The tests align with the blocking logic in isBlockedIPv6.
src/router/router.ts (1)
399-416: Fix race/unhandled-promise: snapshot pending before firing waitUntilAll; don’t chain on success onlyCurrent code runs
waitUntilAllinside a.then(...)and checkshasPending()afterwards. If pending tasks finish quickly (or the handler rejects), the pending flag can be lost andwaitUntilAllmay not run.Apply:
- ).then((r) => { - contextHandler.waitUntilAll(logger, sessionId); - return r; - }); + ); ... - if (response) { - if (contextHandler.hasPending()) { + const hadPending = contextHandler.hasPending(); + void contextHandler.waitUntilAll(logger, sessionId); + if (response && hadPending) { if (response instanceof Response) { response.headers.set('x-agentuity-session-pending', 'true'); } else { if (!response.metadata) { response.metadata = {}; } response.metadata['session-pending'] = 'true'; // let the upstream know that we are still processing } - } - } + }Optional: use a boolean
trueforsession-pendingif no consumers rely on the string form.src/apis/session.ts (1)
10-18: Confirm API expectations: content type and acceptable success codes
- send() already sets "Content-Type: application/json" by default (src/apis/api.ts) — no header needed at the call site.
- markSessionCompleted only treats 202 as success; send() treats 200/201/202 as successful. If the backend can return 200 or 204, broaden the check (or confirm backend will always return 202) (src/apis/session.ts).
src/router/context.ts (3)
10-13: Idle check ignores queued tasks.isIdle() only reflects currently-running tasks, not queued ones. If tasks are enqueued but waitUntilAll hasn’t started, /_idle could incorrectly report idle. Confirm desired semantics; if “idle” should include no queued work, we should track queue length (or increment/decrement a separate queued counter on waitUntil/poll).
25-44: Don’t swallow task errors; type-safe catch and rethrow.Normalize sync/async with Promise.resolve, catch as unknown, record exception with message, and rethrow so waitUntilAll can observe failures.
- public waitUntil(promise: () => void | Promise<void>): void { + public waitUntil(task: () => void | Promise<void>): void { const currentContext = context.active(); this.promises.push(async () => { running++; if (this.started === undefined) { - this.started = Date.now(); /// this first execution marks the start time + this.started = Date.now(); // first execution marks the start time } const span = this.tracer.startSpan('waitUntil', {}, currentContext); const spanContext = trace.setSpan(currentContext, span); try { - await context.with(spanContext, async () => await promise()); + await context.with(spanContext, () => Promise.resolve(task())); span.setStatus({ code: SpanStatusCode.OK }); - } catch (ex) { - span.recordException(ex as Error); - span.setStatus({ code: SpanStatusCode.ERROR }); + } catch (ex: unknown) { + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: err.message + }); + throw err; } finally { span.end(); } }); }
50-64: Snapshot queue; instrument waitUntilAll; don’t drop newly queued tasks; report failures.Clearing the array in finally can drop tasks added during execution and miscount running. Also, errors inside tasks are swallowed, so overall status is always OK and failures aren’t surfaced.
public async waitUntilAll(logger: Logger, sessionId: string): Promise<void> { - if (this.promises.length === 0) { - return; - } - try { - await Promise.all(this.promises.map((p) => p())); - const duration = Date.now() - (this.started as number); - await markSessionCompleted(sessionId, duration); - } catch (ex) { - logger.error('error sending session completed', ex); - } finally { - running -= this.promises.length; - this.promises.length = 0; - } + if (this.promises.length === 0) return; + const currentContext = context.active(); + const span = this.tracer.startSpan('waitUntilAll', {}, currentContext); + const tasks = this.promises.splice(0, this.promises.length); + span.setAttribute('count', tasks.length); + const spanContext = trace.setSpan(currentContext, span); + try { + await context.with(spanContext, async () => { + const results = await Promise.allSettled(tasks.map((p) => p())); + const failed = results.filter((r) => r.status === 'rejected').length; + if (failed > 0) { + span.setAttribute('errors', failed); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: `${failed} waitUntil task(s) failed` + }); + } else { + span.setStatus({ code: SpanStatusCode.OK }); + } + }); + const started = this.started ?? Date.now(); + const duration = Date.now() - started; + await markSessionCompleted(sessionId, duration); + } catch (ex: unknown) { + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ code: SpanStatusCode.ERROR, message: err.message }); + logger.error('error sending session completed', err); + } finally { + running -= tasks.length; + span.end(); + } }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/router/context.ts (1)
10-13: Optional: Clarify idle semantics or decrement per-task in wrapper.If isIdle should reflect active work rather than “pending array entries,” move running-- to each wrapper’s finally and stop adjusting it in waitUntilAll. This reports idle as soon as all tasks complete even if the queue hasn’t been cleared yet.
If you want, I can provide a small diff for this variant.
test/router/context.test.ts (4)
128-160: Test names claim “re-throw” but no assertion checks it.Either assert that waitUntilAll throws or rename to reflect current behavior (errors recorded, not thrown). Right now the name misleads.
Example adjustment:
-it('should re-throw errors from direct promises', async () => { +it('records errors from direct promises (no throw)', async () => {
33-47: Assert session completion is sent on success.You mock markSessionCompleted but never assert it was called. Add an expectation in a success test to verify integration.
Minimal change:
await handler.waitUntilAll(console, 'test-session'); expect(mockTracer.startSpan).toHaveBeenCalledWith('waitUntil', {}, expect.any(Object)); expect(mockSpan.setStatus).toHaveBeenCalledWith({ code: SpanStatusCode.OK }); expect(mockSpan.end).toHaveBeenCalled(); +// verify session completion +const { markSessionCompleted } = await import('../../src/apis/session'); +expect(markSessionCompleted).toHaveBeenCalledWith('test-session', expect.any(Number));
89-94: Add a test for tasks enqueued during waitUntilAll; prevent drops.To guard against regressions in queue snapshotting, add a test that pushes a task while waitUntilAll is running and verify it’s not lost (either executed in this round if intended, or remains pending for a subsequent round).
I can draft this test if you confirm the intended behavior (execute now vs. defer to next waitUntilAll).
13-27: Prefer stricter types for tracer/span mocks.Using any hides type issues. Consider Partial and a minimal Span interface to keep tests type-safe.
Example:
import type { Tracer, Span } from '@opentelemetry/api'; let mockSpan: Pick<Span, 'setStatus' | 'recordException' | 'end'>; let mockTracer: Pick<Tracer, 'startSpan'>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/otel/index.ts(3 hunks)src/router/context.ts(1 hunks)src/types.ts(3 hunks)test/router/context.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types.ts
- src/otel/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/router/context.test.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
test/router/context.test.tssrc/router/context.ts
src/router/**
📄 CodeRabbit inference engine (AGENT.md)
Router code (request/response handling, streaming) resides in src/router/
Files:
src/router/context.ts
🧠 Learnings (1)
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/index.ts : src/index.ts must be the entry point and export server, logger, types, and APIs
Applied to files:
src/router/context.ts
🧬 Code graph analysis (2)
test/router/context.test.ts (1)
src/router/context.ts (1)
AgentContextWaitUntilHandler(15-71)
src/router/context.ts (2)
src/logger/logger.ts (1)
Logger(4-44)src/apis/session.ts (1)
markSessionCompleted(6-18)
🔇 Additional comments (3)
src/router/context.ts (3)
25-49: Run the task inside context.with; don’t invoke it beforehand; improve error typing.Currently the callback is invoked before entering the span context, so nested spans won’t be linked. Also cast to Error is unsafe; normalize unknown to Error and include status message.
Apply:
- public waitUntil(promise: Promise<void> | (() => void | Promise<void>)): void { + public waitUntil(promise: Promise<void> | (() => void | Promise<void>)): void { const currentContext = context.active(); this.promises.push(async () => { running++; if (this.started === undefined) { this.started = Date.now(); /// this first execution marks the start time } const span = this.tracer.startSpan('waitUntil', {}, currentContext); const spanContext = trace.setSpan(currentContext, span); try { - const resolvedPromise = typeof promise === 'function' ? promise() : promise; - await context.with( - spanContext, - async () => await Promise.resolve(resolvedPromise) - ); - span.setStatus({ code: SpanStatusCode.OK }); - } catch (ex: unknown) { - span.recordException(ex as Error); - span.setStatus({ code: SpanStatusCode.ERROR }); - throw ex; + await context.with(spanContext, () => + Promise.resolve( + typeof promise === 'function' ? (promise as () => void | Promise<void>)() : promise + ) + ); + span.setStatus({ code: SpanStatusCode.OK }); + } catch (ex: unknown) { + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ code: SpanStatusCode.ERROR, message: err.message }); + throw err; } finally { span.end(); } // NOTE: we only decrement when the promise is removed from the array in waitUntilAll }); }
56-70: Critical: waitUntilAll drops tasks added during execution; lacks span and miscounts running.
- Clearing the live array after awaiting can drop tasks enqueued mid-flight.
- Use a snapshot via splice, run tasks with allSettled, set span status/attributes, and decrement running using the snapshot size.
- Reset started after marking completion to avoid inflating duration on subsequent calls.
Apply:
- public async waitUntilAll(logger: Logger, sessionId: string): Promise<void> { - if (this.promises.length === 0) { - return; - } - try { - await Promise.all(this.promises.map((p) => p())); - const duration = Date.now() - (this.started as number); - await markSessionCompleted(sessionId, duration); - } catch (ex) { - logger.error('error sending session completed', ex); - } finally { - running -= this.promises.length; - this.promises.length = 0; - } - } + public async waitUntilAll(logger: Logger, sessionId: string): Promise<void> { + if (this.promises.length === 0) return; + const currentContext = context.active(); + const span = this.tracer.startSpan('waitUntilAll', {}, currentContext); + const tasks = this.promises.splice(0, this.promises.length); + span.setAttribute('count', tasks.length); + const spanContext = trace.setSpan(currentContext, span); + try { + const results = await context.with(spanContext, () => + Promise.allSettled(tasks.map((p) => p())) + ); + const failed = results.filter((r) => r.status === 'rejected').length; + if (failed > 0) { + span.setAttribute('errors', failed); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: `${failed} waitUntil task(s) failed`, + }); + } else { + span.setStatus({ code: SpanStatusCode.OK }); + } + if (this.started !== undefined) { + const duration = Date.now() - this.started; + await markSessionCompleted(sessionId, duration); + this.started = undefined; + } + } catch (ex: unknown) { + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ code: SpanStatusCode.ERROR, message: err.message }); + logger.error('error in waitUntilAll', err); + } finally { + span.end(); + running -= tasks.length; + } + }
8-8: Retain existing Logger import
src/logger/index.tsre-exportsLogger, soimport type { Logger } from '../logger';is already correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/router/context.ts (2)
16-16: Tighten promises array typeWe always push async wrappers, so the type can be simplified to functions returning Promise.
- private promises: (() => void | Promise<void>)[]; + private promises: Array<() => Promise<void>>;
43-46: Normalize and rethrow errors; include message in span statusAvoid
as Error. Normalize unknown to Error so telemetry gets a message, then rethrow the normalized error.- } catch (ex: unknown) { - span.recordException(ex as Error); - span.setStatus({ code: SpanStatusCode.ERROR }); - throw ex; + } catch (ex: unknown) { + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: err.message + }); + throw err;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/router/context.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/router/**
📄 CodeRabbit inference engine (AGENT.md)
Router code (request/response handling, streaming) resides in src/router/
Files:
src/router/context.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/router/context.ts
🧠 Learnings (1)
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/index.ts : src/index.ts must be the entry point and export server, logger, types, and APIs
Applied to files:
src/router/context.ts
🧬 Code graph analysis (1)
src/router/context.ts (2)
src/logger/logger.ts (1)
Logger(4-44)src/apis/session.ts (1)
markSessionCompleted(6-18)
🔇 Additional comments (1)
src/router/context.ts (1)
7-8: Fix incorrect Logger import path (likely build break)Logger is defined in src/logger/logger.ts and there’s no barrel export. Importing from '../logger' will fail.
-import type { Logger } from '../logger'; +import type { Logger } from '../logger/logger';To confirm, run:
#!/bin/bash rg -n -C2 -t ts -P 'export\s+(interface|type)\s+Logger\b' | sed -n '1,200p'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/router/context.ts (2)
10-13: Global state management could lead to cross-request interference.The global
runningcounter is shared across all request handlers, which could cause issues in high-concurrency scenarios where multiple requests are processed simultaneously.Consider encapsulating the running state within a request-scoped context or using a WeakMap keyed by request/session:
-let running = 0; +const runningBySession = new Map<string, number>(); + export function isIdle(): boolean { - return running === 0; + // Check if all sessions are idle + for (const count of runningBySession.values()) { + if (count > 0) return false; + } + return true; +} + +export function isSessionIdle(sessionId: string): boolean { + return (runningBySession.get(sessionId) ?? 0) === 0; }Then update the handler to track per-session:
// In constructor, accept sessionId public constructor(tracer: Tracer, private sessionId: string) { // ... existing code } // Update increment/decrement to use sessionId runningBySession.set(this.sessionId, (runningBySession.get(this.sessionId) ?? 0) + 1);
27-59: Consider validating Promise-like objects for better developer experience.The function accepts both
Promise<void>and a function, but doesn't validate that the function returns a valid Promise-like object.Add runtime validation to help catch developer errors early:
await context.with(spanContext, async () => { const resolvedPromise = typeof promise === 'function' ? promise() : promise; + // Validate that we have a thenable object + if (resolvedPromise && typeof resolvedPromise.then !== 'function') { + throw new TypeError('waitUntil expects a Promise or a function returning a Promise'); + } return await Promise.resolve(resolvedPromise); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/router/context.ts(1 hunks)test/router/context.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/router/context.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/router/**
📄 CodeRabbit inference engine (AGENT.md)
Router code (request/response handling, streaming) resides in src/router/
Files:
src/router/context.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/router/context.ts
🧠 Learnings (1)
📚 Learning: 2025-08-25T13:53:20.040Z
Learnt from: CR
PR: agentuity/sdk-js#0
File: AGENT.md:0-0
Timestamp: 2025-08-25T13:53:20.040Z
Learning: Applies to src/index.ts : src/index.ts must be the entry point and export server, logger, types, and APIs
Applied to files:
src/router/context.ts
🧬 Code graph analysis (1)
src/router/context.ts (2)
src/logger/logger.ts (1)
Logger(4-44)src/apis/session.ts (1)
markSessionCompleted(6-18)
🔇 Additional comments (2)
src/router/context.ts (2)
8-8: Barrel export re-exports Logger, current import is valid
src/logger/index.tsre-exportsLoggerfromlogger.ts, soimport type { Logger } from '../logger';requires no change.Likely an incorrect or invalid review comment.
65-84: Critical:waitUntilAllcan drop tasks added during execution and has incorrect error handling.Multiple issues:
- Tasks added while
waitUntilAllis running are lost whenthis.promises.length = 0executes- Individual task errors only log but don't affect telemetry status
markSessionCompletedis skipped if any task failsSnapshot the queue and use
Promise.allSettledto handle individual failures properly:public async waitUntilAll(logger: Logger, sessionId: string): Promise<void> { if (this.hasCalledWaitUntilAll) { throw new Error('waitUntilAll can only be called once per instance'); } this.hasCalledWaitUntilAll = true; if (this.promises.length === 0) { return; } + + // Snapshot current tasks to avoid losing any added during execution + const tasks = this.promises.splice(0, this.promises.length); + const currentContext = context.active(); + const span = this.tracer.startSpan('waitUntilAll', {}, currentContext); + span.setAttribute('count', tasks.length); + const spanContext = trace.setSpan(currentContext, span); + try { - await Promise.all(this.promises.map((p) => p())); - const duration = Date.now() - (this.started as number); - await markSessionCompleted(sessionId, duration); - } catch (ex) { - logger.error('error sending session completed', ex); + await context.with(spanContext, async () => { + const results = await Promise.allSettled(tasks.map((p) => p())); + const failed = results.filter((r) => r.status === 'rejected'); + + if (failed.length > 0) { + span.setAttribute('errors', failed.length); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: `${failed.length} waitUntil task(s) failed` + }); + // Log individual failures + failed.forEach((result, index) => { + if (result.status === 'rejected') { + logger.error(`waitUntil task failed`, result.reason); + } + }); + } else { + span.setStatus({ code: SpanStatusCode.OK }); + } + }); + } catch (ex: unknown) { + // This should rarely happen as allSettled doesn't throw + const err = ex instanceof Error ? ex : new Error(String(ex)); + span.recordException(err); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: err.message + }); + logger.error('Unexpected error in waitUntilAll', ex); } finally { - running -= this.promises.length; - this.promises.length = 0; + span.end(); + + // Always attempt to mark session completed + const duration = Date.now() - (this.started as number); + try { + await markSessionCompleted(sessionId, duration); + } catch (ex: unknown) { + logger.error('error sending session completed', ex); + } + + // Reset state + this.started = undefined; } }
| this.promises.push(async () => { | ||
| running++; | ||
| if (this.started === undefined) { | ||
| this.started = Date.now(); /// this first execution marks the start time | ||
| } | ||
| const span = this.tracer.startSpan('waitUntil', {}, currentContext); | ||
| const spanContext = trace.setSpan(currentContext, span); | ||
| try { | ||
| await context.with(spanContext, async () => { | ||
| const resolvedPromise = | ||
| typeof promise === 'function' ? promise() : promise; | ||
| return await Promise.resolve(resolvedPromise); | ||
| }); | ||
| span.setStatus({ code: SpanStatusCode.OK }); | ||
| } catch (ex: unknown) { | ||
| span.recordException(ex as Error); | ||
| span.setStatus({ code: SpanStatusCode.ERROR }); | ||
| throw ex; | ||
| } finally { | ||
| span.end(); | ||
| } | ||
| // NOTE: we only decrement when the promise is removed from the array in waitUntilAll | ||
| }); |
There was a problem hiding this comment.
Critical: running counter increments but never decrements on individual task failures.
The running counter increments inside each task wrapper but only decrements in bulk during waitUntilAll. If a task throws before waitUntilAll is called, the counter remains permanently elevated, breaking the isIdle() function.
Move the increment/decrement logic to ensure proper cleanup:
const currentContext = context.active();
this.promises.push(async () => {
- running++;
if (this.started === undefined) {
this.started = Date.now(); /// this first execution marks the start time
}
const span = this.tracer.startSpan('waitUntil', {}, currentContext);
const spanContext = trace.setSpan(currentContext, span);
+ running++;
try {
await context.with(spanContext, async () => {
const resolvedPromise =
typeof promise === 'function' ? promise() : promise;
return await Promise.resolve(resolvedPromise);
});
span.setStatus({ code: SpanStatusCode.OK });
} catch (ex: unknown) {
span.recordException(ex as Error);
span.setStatus({ code: SpanStatusCode.ERROR });
throw ex;
} finally {
span.end();
+ running--;
}
- // NOTE: we only decrement when the promise is removed from the array in waitUntilAll
});And update waitUntilAll to not double-decrement:
} finally {
- running -= this.promises.length;
this.promises.length = 0;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/router/context.ts around lines 36 to 58, the running counter is
incremented inside the task wrapper but only decremented later in waitUntilAll,
causing running to stay inflated if a task throws before waitUntilAll; move
running++ to just before executing the promise and add a running-- in the
wrapper's finally block so every task always decrements itself (ensure started
initialization remains in place), then update waitUntilAll to stop decrementing
per-task (remove the per-item running-- there or only decrement for tasks that
never handled it) to avoid double-decrementing.
There was a problem hiding this comment.
we don't want to decrement any of the running promises even if they complete until all of them finish in waitUntilAll
There was a problem hiding this comment.
@jhaynie, I understand - you want the running counter to represent work that hasn't been fully processed as a batch, not individual task completion. This maintains the collective nature of the wait-until pattern where the session isn't truly done until waitUntilAll processes everything together.
However, there's still the issue that if waitUntilAll is never called (due to an error in the main request flow), the running counter will remain elevated permanently. Perhaps we could address this with a cleanup mechanism or timeout, but I'll respect your current design choice for the batched completion semantics.
/| /|
( :v: )
|(_)|
-"---"-
*thinking*
✏ Learnings added
Learnt from: jhaynie
PR: agentuity/sdk-js#179
File: src/router/context.ts:36-58
Timestamp: 2025-09-24T18:33:34.707Z
Learning: In the AgentContextWaitUntilHandler class in src/router/context.ts, the running counter should only be decremented in waitUntilAll, not when individual promises complete. The design intent is that running represents work that hasn't been fully processed as a batch, maintaining the collective nature of the wait-until pattern where a session isn't done until all deferred work is processed together.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| span.recordException(ex as Error); | ||
| span.setStatus({ code: SpanStatusCode.ERROR }); |
There was a problem hiding this comment.
Type narrowing needed for exception recording.
The span.recordException expects an Error object, but you're casting unknown to Error without proper type checking.
} catch (ex: unknown) {
- span.recordException(ex as Error);
+ const err = ex instanceof Error ? ex : new Error(String(ex));
+ span.recordException(err);
span.setStatus({ code: SpanStatusCode.ERROR });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| span.recordException(ex as Error); | |
| span.setStatus({ code: SpanStatusCode.ERROR }); | |
| } catch (ex: unknown) { | |
| const err = ex instanceof Error ? ex : new Error(String(ex)); | |
| span.recordException(err); | |
| span.setStatus({ code: SpanStatusCode.ERROR }); | |
| } |
🤖 Prompt for AI Agents
In src/router/context.ts around lines 51-52, the code casts an unknown exception
to Error when calling span.recordException; instead, narrow the type first:
check if ex instanceof Error and call span.recordException(ex) with the real
Error, otherwise wrap the value into an Error (e.g., new Error(String(ex))) and
pass that to span.recordException so the span always receives a valid Error
object; keep the span.setStatus({ code: SpanStatusCode.ERROR }) after recording.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores