Expose messageId and usage endpoints#642
Conversation
|
Someone is attempting to deploy a commit to the Shaunak's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds message ID tracking across the agent response pipeline and introduces new billing/usage query endpoints. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/agent/DextoAgent.ts (1)
841-848:⚠️ Potential issue | 🔴 CriticalOmit
messageIdwhen it is absent.With
exactOptionalPropertyTypes: true, optional properties likeGenerateResponse.messageId?: stringcannot accept explicitundefinedin object literals. The return statement always includes the key even whenresponseEvent.messageIdis undefined, causing TS2375.Suggested fix
return { content: responseEvent.content, reasoning: responseEvent.reasoning, usage: usage as import('./types.js').TokenUsage, toolCalls, sessionId, - messageId: responseEvent.messageId, + ...(responseEvent.messageId !== undefined && { + messageId: responseEvent.messageId, + }), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/DextoAgent.ts` around lines 841 - 848, The return object in DextoAgent.ts always includes messageId even when responseEvent.messageId is undefined which violates exactOptionalPropertyTypes on GenerateResponse.messageId?: string; change the return to conditionally include the property only when responseEvent.messageId is defined (e.g., use a conditional spread that adds { messageId: responseEvent.messageId } only when responseEvent.messageId != null) so the key is omitted when absent; locate the return in the function that constructs the response (references: responseEvent.messageId, GenerateResponse.messageId) and apply the conditional property inclusion there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/events/index.ts`:
- Around line 373-374: Session-level event type for "llm:response" is missing
the new stable assistant messageId emitted by StreamProcessor via
SessionEventBus; update the session event map to mirror this by adding
messageId?: string to the SessionEventMap['llm:response'] / the session-scoped
"llm:response" type definition so session consumers receive the same field
StreamProcessor emits and keep the session contract in sync with the agent
contract.
In `@packages/server/src/hono/routes/sessions.ts`:
- Around line 221-227: Replace the ad-hoc 404 response schema (z.object({ error:
z.string() })) in the sessions route definitions with the shared error envelope
exported by the error middleware (use the middleware's exported schema, e.g.,
errorEnvelopeSchema / errorResponseSchema) and import that symbol from the
middleware module; update all occurrences referenced in this review (the two 404
response docs in sessions.ts and the ad-hoc 404 bodies returned at the
endpoints) so the OpenAPI/route docs and runtime responses use the same shared
error envelope shape.
- Around line 539-542: The session metadata lookup using
agent.getSessionMetadata(sessionId) can throw for stale/nonexistent IDs; wrap
that call in a try/catch in the /sessions/{sessionId}/usage handler (around the
existing agent.getSessionMetadata call) and on error return the same 404 JSON
response (ctx.json({ error: `Session not found: ${sessionId}` }, 404)); keep the
current undefined check but ensure thrown errors are caught and handled the same
way so a thrown lookup doesn't produce a 500.
---
Outside diff comments:
In `@packages/core/src/agent/DextoAgent.ts`:
- Around line 841-848: The return object in DextoAgent.ts always includes
messageId even when responseEvent.messageId is undefined which violates
exactOptionalPropertyTypes on GenerateResponse.messageId?: string; change the
return to conditionally include the property only when responseEvent.messageId
is defined (e.g., use a conditional spread that adds { messageId:
responseEvent.messageId } only when responseEvent.messageId != null) so the key
is omitted when absent; locate the return in the function that constructs the
response (references: responseEvent.messageId, GenerateResponse.messageId) and
apply the conditional property inclusion there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f56ed93-72e5-4d05-9932-da9bc3d73f4c
📒 Files selected for processing (8)
packages/core/src/agent/DextoAgent.tspackages/core/src/agent/types.tspackages/core/src/events/index.tspackages/core/src/llm/executor/stream-processor.tspackages/server/src/events/a2a-sse-subscriber.tspackages/server/src/hono/routes/messages.tspackages/server/src/hono/routes/sessions.tspackages/server/src/hono/schemas/responses.ts
| /** Stable assistant message id for idempotency/billing */ | ||
| messageId?: string; |
There was a problem hiding this comment.
Mirror messageId into the session-scoped llm:response type.
StreamProcessor now emits messageId on SessionEventBus, but SessionEventMap['llm:response'] still omits it. That leaves the session contract out of sync with the agent contract and hides the new field from session-level consumers.
Suggested fix
// SessionEventMap
'llm:response': {
content: string;
reasoning?: string;
provider?: LLMProvider;
model?: string;
/** Reasoning tuning variant used for this call, when the provider exposes it. */
reasoningVariant?: ReasoningVariant;
/** Reasoning budget tokens used for this call, when the provider exposes it. */
reasoningBudgetTokens?: number;
tokenUsage?: {
inputTokens?: number;
outputTokens?: number;
reasoningTokens?: number;
totalTokens?: number;
cacheReadTokens?: number;
cacheWriteTokens?: number;
};
+ /** Stable assistant message id for idempotency/billing */
+ messageId?: string;
/** Estimated input tokens before LLM call (for analytics/calibration) */
estimatedInputTokens?: number;
/** Finish reason: 'tool-calls' means more steps coming, others indicate completion */
finishReason?: LLMFinishReason;
};📝 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.
| /** Stable assistant message id for idempotency/billing */ | |
| messageId?: string; | |
| 'llm:response': { | |
| content: string; | |
| reasoning?: string; | |
| provider?: LLMProvider; | |
| model?: string; | |
| /** Reasoning tuning variant used for this call, when the provider exposes it. */ | |
| reasoningVariant?: ReasoningVariant; | |
| /** Reasoning budget tokens used for this call, when the provider exposes it. */ | |
| reasoningBudgetTokens?: number; | |
| tokenUsage?: { | |
| inputTokens?: number; | |
| outputTokens?: number; | |
| reasoningTokens?: number; | |
| totalTokens?: number; | |
| cacheReadTokens?: number; | |
| cacheWriteTokens?: number; | |
| }; | |
| /** Stable assistant message id for idempotency/billing */ | |
| messageId?: string; | |
| /** Estimated input tokens before LLM call (for analytics/calibration) */ | |
| estimatedInputTokens?: number; | |
| /** Finish reason: 'tool-calls' means more steps coming, others indicate completion */ | |
| finishReason?: LLMFinishReason; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/events/index.ts` around lines 373 - 374, Session-level
event type for "llm:response" is missing the new stable assistant messageId
emitted by StreamProcessor via SessionEventBus; update the session event map to
mirror this by adding messageId?: string to the SessionEventMap['llm:response']
/ the session-scoped "llm:response" type definition so session consumers receive
the same field StreamProcessor emits and keep the session contract in sync with
the agent contract.
| 404: { | ||
| description: 'Session not found', | ||
| content: { | ||
| 'application/json': { | ||
| schema: z.object({ error: z.string().describe('Error message') }).strict(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Use the shared error envelope for these 404s.
Lines 225/263 document { error: string }, and Lines 541/569 return the same ad-hoc body. That introduces a second error contract for the sessions API and forces clients to special-case these endpoints.
Based on learnings, error responses must follow the standard middleware format from packages/server/src/hono/middleware/error.ts, and 404 responses should document that structure in the route definition.
Also applies to: 259-264, 541-542, 569-572
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/hono/routes/sessions.ts` around lines 221 - 227, Replace
the ad-hoc 404 response schema (z.object({ error: z.string() })) in the sessions
route definitions with the shared error envelope exported by the error
middleware (use the middleware's exported schema, e.g., errorEnvelopeSchema /
errorResponseSchema) and import that symbol from the middleware module; update
all occurrences referenced in this review (the two 404 response docs in
sessions.ts and the ad-hoc 404 bodies returned at the endpoints) so the
OpenAPI/route docs and runtime responses use the same shared error envelope
shape.
| const metadata = await agent.getSessionMetadata(sessionId); | ||
| if (!metadata) { | ||
| return ctx.json({ error: `Session not found: ${sessionId}` }, 404); | ||
| } |
There was a problem hiding this comment.
Catch thrown metadata lookups before returning 404.
Line 539 assumes agent.getSessionMetadata(sessionId) returns undefined for a missing session, but this file already wraps the same lookup in try/catch when stale session IDs are possible. If the lookup throws here, /sessions/{sessionId}/usage returns 500 instead of the documented 404.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/hono/routes/sessions.ts` around lines 539 - 542, The
session metadata lookup using agent.getSessionMetadata(sessionId) can throw for
stale/nonexistent IDs; wrap that call in a try/catch in the
/sessions/{sessionId}/usage handler (around the existing
agent.getSessionMetadata call) and on error return the same 404 JSON response
(ctx.json({ error: `Session not found: ${sessionId}` }, 404)); keep the current
undefined check but ensure thrown errors are caught and handled the same way so
a thrown lookup doesn't produce a 500.
1 participant
PR create by dexto cloud agent.
Summary by CodeRabbit
Release Notes
New Features
API Changes