Refactor agent module into reusable package#1354
Conversation
|
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:
📝 WalkthroughWalkthroughMoves Claude SDK execution out of the streams server into a new workspace package Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop
participant AgentPkg as `@superset/agent` (executeAgent)
participant ClaudeSDK as Claude SDK
participant StreamsProxy as Streams Proxy
participant Permissions
Desktop->>AgentPkg: executeAgent(params: {sessionId, prompt, env, onChunk, onPermissionRequest, signal})
AgentPkg->>AgentPkg: init converter, sessionStore, AbortController
AgentPkg->>ClaudeSDK: query({...model, env, resume, permissions hook...})
ClaudeSDK-->>AgentPkg: stream messages / system init
AgentPkg->>AgentPkg: convert messages → chunks
AgentPkg->>Desktop: onEvent(chunk_sent / session_initialized / completed)
AgentPkg->>StreamsProxy: POST /v1/sessions/{id}/chunks (via onChunk)
StreamsProxy-->>StreamsProxy: persist chunk to durable stream
AgentPkg->>Permissions: onPermissionRequest -> await user decision
Permissions-->>AgentPkg: approve/deny (resolvePendingPermission)
AgentPkg->>ClaudeSDK: continue or stop based on permissions or signal
AgentPkg-->>Desktop: executeAgent result (success/error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/streams/src/routes/messages.ts (1)
4-4:⚠️ Potential issue | 🟡 Minor
regenerateRequestSchemaimport is now unused.The regenerate endpoint was removed but its schema is still imported. Remove it to keep imports clean.
Proposed fix
-import { regenerateRequestSchema, stopGenerationRequestSchema } from "../types"; +import { stopGenerationRequestSchema } from "../types";packages/agent/src/session-store.ts (1)
78-96:⚠️ Potential issue | 🟡 Minor
setClaudeSessionIdsilently skips persistence ifinitSessionStorehasn't been called.
persistSessions()returns early whensessionsDir/sessionsFileare null (Line 46), so callingsetClaudeSessionIdbeforeinitSessionStorewill update the in-memory map but silently lose data on restart. Consider either:
- Throwing/logging a warning if the store isn't initialized, or
- Documenting this as an intentional "memory-only" fallback.
🤖 Fix all issues with AI agents
In `@apps/streams/package.json`:
- Line 16: Remove the unused dependency "@anthropic-ai/claude-agent-sdk" from
apps/streams/package.json: open package.json, delete the dependency entry for
"@anthropic-ai/claude-agent-sdk" (currently on line with the version "^0.2.37"),
then run the appropriate package manager command (npm/yarn/pnpm) to update
lockfile and node_modules so the dependency is fully removed.
In `@docs/realign-streams-architecture.md`:
- Around line 177-188: Update the example package.json to use deterministic,
valid semver ranges instead of non-deterministic/invalid values: replace
"@anthropic-ai/claude-agent-sdk": "latest" with a concrete compatible range
(e.g., "^0.2.37") and replace "zod": "^3.x.x" with a valid semver that matches
the workspace (e.g., "zod": "^4.3.5" or at minimum a valid "^3.0.0" if
intentionally on v3); ensure these changes are applied to the dependencies block
in the example so readers copying it get valid, reproducible versions.
- Around line 154-156: The docs and README reference an outdated callback
name/signature `canUseTool?: (toolUse: ToolUseBlock) => Promise<boolean>`;
update those references to match the implemented API by replacing occurrences of
`canUseTool` with `onPermissionRequest` and updating the signature to
`onPermissionRequest?: (params: PermissionRequestParams) =>
Promise<PermissionResult>` (matching ExecuteAgentParams), and adjust any example
payload names (`toolUse` → the appropriate `PermissionRequestParams` fields) and
expected return handling to use `PermissionResult`; ensure both
docs/realign-streams-architecture.md and packages/agent/README.md mirror the
types defined in ExecuteAgentParams and mention the PermissionRequestParams and
PermissionResult types.
In `@packages/agent/README.md`:
- Around line 14-32: The README usage example uses the outdated callback
canUseTool; update the example to call onPermissionRequest (matching the
ExecuteAgentParams type) and return a PermissionResult instead of a boolean,
ensuring the function signature matches the type definition (e.g., accepts the
same tool request object and returns a PermissionResult). Replace the canUseTool
parameter and its implementation in the example with onPermissionRequest, adjust
the returned value to a valid PermissionResult, and keep other params like
executeAgent and onChunk unchanged.
In `@packages/agent/src/agent-executor.ts`:
- Around line 79-91: The abort logic fails when an external AbortSignal is
already aborted because the "abort" event won't fire; update the setup in
agent-executor.ts so after creating abortController it checks if the incoming
signal exists and has signal.aborted === true and, if so, immediately call
abortController.abort(), then still attach the signal.addEventListener("abort",
...) with { once: true } to handle future aborts; reference the abortController
and signal variables when making this change.
In `@packages/agent/src/index.ts`:
- Line 2: The package is missing re-exports for types needed by consumers
implementing callbacks; update the re-exports in the module
(packages/agent/src/index.ts) to also export PermissionRequestParams,
PermissionResult, AgentEvent, and PermissionMode from "./types" so consumers
implementing onPermissionRequest and onEvent can import the correct types;
locate the existing export line that re-exports
ExecuteAgentParams/ExecuteAgentResult and add the four missing type names to
that export list.
In `@packages/agent/src/types.ts`:
- Around line 68-70: The PermissionResult union's "allow" variant currently
requires updatedInput; change it so updatedInput is optional by updating the
type definition for PermissionResult (the union that includes { behavior:
"allow"; updatedInput: Record<string, unknown> }) to make updatedInput?:
Record<string, unknown>, so callers can return an allow without providing the
original input. Ensure only the "allow" branch is modified and keep the "deny"
branch ({ behavior: "deny"; message: string }) unchanged.
- Around line 72-77: The ExecuteAgentResult interface currently requires
messageId and runId even on failure; update the interface (ExecuteAgentResult)
to make messageId and runId optional (messageId?: string; runId?: string) so
callers don’t need to fabricate values when success is false, and scan for
usages of ExecuteAgentResult to handle undefined messageId/runId where error
paths may not provide them.
🧹 Nitpick comments (8)
apps/streams/src/handlers/invoke-agent.ts (1)
6-16: Dead code:invokeAgentRequestSchemaandInvokeAgentRequestare unused.These were only used by the removed
handleInvokeAgent. Clean them up.Proposed fix
-const invokeAgentRequestSchema = z.object({ - agent: agentSpecSchema, - messages: z.array( - z.object({ - role: z.string(), - content: z.string(), - }), - ), -}); - -type InvokeAgentRequest = z.infer<typeof invokeAgentRequestSchema>; - // handleInvokeAgent removed - agent invocation is now handled by desktopdocs/realign-streams-architecture.md (1)
22-22: Add language specifiers to fenced code blocks.Lines 22, 119, and 360 use fenced code blocks without a language identifier (flagged by markdownlint MD040). Use
textor an appropriate language identifier.Also applies to: 119-119, 360-360
packages/agent/src/types.ts (1)
79-83:AgentEventerror variant usesErrorobject — may not serialize across IPC.The desktop app communicates between main and renderer via Electron IPC, which uses structured cloning.
Errorobjects lose their stack and custom properties during serialization. Consider using a plain object or string for the error field, or at minimum document that consumers should serialize before transmitting.apps/streams/src/routes/chunks.ts (2)
49-57:chunk as neversuppresses type safety;writeChunkcalled with 7 positional args.Two concerns here:
as neversilences the type checker entirely. The Zod schema inferschunkasRecord<string, unknown>, whilewriteChunkexpectsStreamChunk. A narrower cast (chunk as StreamChunk) would preserve some checking, or better yet, validate against the actualStreamChunkshape inchunkBodySchema.Seven positional arguments make this call fragile and hard to read. The coding guideline says to use object parameters for functions with 2+ parameters. Although
writeChunkis defined onAIDBSessionProtocol, consider refactoring that signature to accept an object — this call site shows why. As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments."Suggested narrower cast
- chunk as never, + chunk as StreamChunk,(with
import type { StreamChunk } from "../types"added at the top)
85-90:generations/startgenerates amessageIdbut doesn't associate it with the session.The endpoint returns a
messageIdviacrypto.randomUUID()but doesn't persist or track it anywhere (the tracking code is commented out). A caller could ignore this ID and write chunks with anymessageId, making this endpoint effectively a no-op UUID generator. If tracking is deferred intentionally, a brief comment or TODO explaining the plan would help future maintainers.packages/agent/src/session-store.ts (1)
45-57: Synchronous file I/O on everysetClaudeSessionIdcall.
writeFileSyncblocks the event loop each time a session is set. If agent execution callssetClaudeSessionIdon a hot path (e.g., per-message), this could introduce latency. Consider debouncing persistence or switching to async writes (writeFilefromnode:fs/promises).packages/agent/src/agent-executor.ts (2)
122-136: Unsafe cast and unchecked field access on SDK messages.Line 122 casts the message to
Record<string, unknown>, then Lines 125-126 access.type,.subtype, and.session_idwithout validation. If the SDK ever changes its message shape, this will silently misbehave rather than fail fast. As per coding guidelines, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks."A lightweight guard (e.g., checking
typeof msg.session_id === "string") on Line 126 instead of the bareas stringcast would be more defensive.
93-116: Hardcoded beta type cast couples this code to a specific SDK version.Line 114 casts
betastoArray<"context-1m-2025-08-07">, which will need updating whenever the SDK adds or changes beta identifiers. Consider typingbetasasstring[]at the SDK boundary if the SDK accepts it, or centralizing supported beta values as a constant.
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "^0.2.19", | ||
| "@anthropic-ai/claude-agent-sdk": "^0.2.37", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if `@anthropic-ai/claude-agent-sdk` is still imported/used in apps/streams/src
rg -n "@anthropic-ai/claude-agent-sdk" --type=ts --type=js -g '!node_modules' apps/streams/src/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Also check the entire apps/streams directory more broadly for any references
rg -n "@anthropic-ai/claude-agent-sdk" -g '!node_modules' apps/streams/ 2>/dev/null | grep -v "package.json\|package-lock.json"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check what the current package.json looks like to understand the context
cat -n apps/streams/package.json | head -30Repository: superset-sh/superset
Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Check for any type imports or indirect references to the dependency
rg -n "claude-agent-sdk\|from.*agent\|import.*agent" --type=ts --type=js -g '!node_modules' apps/streams/src/ 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if any files in apps/streams reference it, including comments/config
rg -n "claude-agent\|agent-sdk" --type=ts --type=js --type=json -g '!node_modules' apps/streams/ 2>/dev/null | grep -v "package-lock.json"Repository: superset-sh/superset
Length of output: 46
Remove unused @anthropic-ai/claude-agent-sdk dependency.
The streams server no longer imports or uses @anthropic-ai/claude-agent-sdk—agent logic has been moved elsewhere. Remove this dependency from apps/streams/package.json line 16 to keep the dependency surface clean.
🤖 Prompt for AI Agents
In `@apps/streams/package.json` at line 16, Remove the unused dependency
"@anthropic-ai/claude-agent-sdk" from apps/streams/package.json: open
package.json, delete the dependency entry for "@anthropic-ai/claude-agent-sdk"
(currently on line with the version "^0.2.37"), then run the appropriate package
manager command (npm/yarn/pnpm) to update lockfile and node_modules so the
dependency is fully removed.
| onChunk: (chunk: StreamChunk) => Promise<void>; | ||
| canUseTool?: (toolUse: ToolUseBlock) => Promise<boolean>; | ||
| onEvent?: (event: RuntimeEvent) => void; |
There was a problem hiding this comment.
Inconsistency: canUseTool callback doesn't match the implemented types.
The doc references canUseTool?: (toolUse: ToolUseBlock) => Promise<boolean> but the actual ExecuteAgentParams in packages/agent/src/types.ts (Line 55) defines onPermissionRequest?: (params: PermissionRequestParams) => Promise<PermissionResult> — different name, different signature, different return type. The README (packages/agent/README.md Line 27) also uses the stale canUseTool name.
Update the doc and README to reflect the implemented API.
🤖 Prompt for AI Agents
In `@docs/realign-streams-architecture.md` around lines 154 - 156, The docs and
README reference an outdated callback name/signature `canUseTool?: (toolUse:
ToolUseBlock) => Promise<boolean>`; update those references to match the
implemented API by replacing occurrences of `canUseTool` with
`onPermissionRequest` and updating the signature to `onPermissionRequest?:
(params: PermissionRequestParams) => Promise<PermissionResult>` (matching
ExecuteAgentParams), and adjust any example payload names (`toolUse` → the
appropriate `PermissionRequestParams` fields) and expected return handling to
use `PermissionResult`; ensure both docs/realign-streams-architecture.md and
packages/agent/README.md mirror the types defined in ExecuteAgentParams and
mention the PermissionRequestParams and PermissionResult types.
| ```json | ||
| { | ||
| "name": "@superset/agent", | ||
| "version": "0.0.1", | ||
| "private": true, | ||
| "main": "./src/index.ts", | ||
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "latest", | ||
| "zod": "^3.x.x" | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Invalid version specifiers in the example package.json.
"@anthropic-ai/claude-agent-sdk": "latest"— usinglatestis non-deterministic and can break builds. Pin to a concrete range (e.g.,^0.2.37as used in the actual desktop/streams packages)."zod": "^3.x.x"— not a valid semver range. Should be^3.0.0or match the actual workspace version (^4.3.5perapps/desktop/package.json).
Since this is a documentation example, readers may copy it verbatim.
🤖 Prompt for AI Agents
In `@docs/realign-streams-architecture.md` around lines 177 - 188, Update the
example package.json to use deterministic, valid semver ranges instead of
non-deterministic/invalid values: replace "@anthropic-ai/claude-agent-sdk":
"latest" with a concrete compatible range (e.g., "^0.2.37") and replace "zod":
"^3.x.x" with a valid semver that matches the workspace (e.g., "zod": "^4.3.5"
or at minimum a valid "^3.0.0" if intentionally on v3); ensure these changes are
applied to the dependencies block in the example so readers copying it get
valid, reproducible versions.
| ```typescript | ||
| import { executeAgent } from "@superset/agent"; | ||
|
|
||
| await executeAgent({ | ||
| sessionId: "session-123", | ||
| prompt: "Create a new React component", | ||
| cwd: "/path/to/project", | ||
| env: { | ||
| ANTHROPIC_API_KEY: "...", | ||
| }, | ||
| onChunk: async (chunk) => { | ||
| // Send chunk to streams server or handle locally | ||
| }, | ||
| canUseTool: async (toolUse) => { | ||
| // Handle permission approval | ||
| return true; | ||
| }, | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Usage example uses stale callback name.
The example uses canUseTool (Line 27) but the actual ExecuteAgentParams type defines onPermissionRequest with a different signature and return type (PermissionResult instead of boolean). This will mislead consumers of the package.
Suggested update to match the actual API
onChunk: async (chunk) => {
// Send chunk to streams server or handle locally
},
- canUseTool: async (toolUse) => {
- // Handle permission approval
- return true;
+ onPermissionRequest: async ({ toolUseId, toolName, input, signal }) => {
+ // Handle permission approval
+ return { behavior: "allow" };
},🤖 Prompt for AI Agents
In `@packages/agent/README.md` around lines 14 - 32, The README usage example uses
the outdated callback canUseTool; update the example to call onPermissionRequest
(matching the ExecuteAgentParams type) and return a PermissionResult instead of
a boolean, ensuring the function signature matches the type definition (e.g.,
accepts the same tool request object and returns a PermissionResult). Replace
the canUseTool parameter and its implementation in the example with
onPermissionRequest, adjust the returned value to a valid PermissionResult, and
keep other params like executeAgent and onChunk unchanged.
| // Setup abort controller | ||
| const abortController = new AbortController(); | ||
|
|
||
| // Connect external signal to internal abort controller | ||
| if (signal) { | ||
| signal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| abortController.abort(); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
If the external signal is already aborted, the internal AbortController is never triggered.
When signal.aborted is already true at the time the listener is attached, the "abort" event won't fire and abortController.abort() is never called. The SDK query would proceed normally despite the caller's intent to cancel.
Proposed fix
if (signal) {
+ if (signal.aborted) {
+ abortController.abort();
+ }
signal.addEventListener(
"abort",
() => {
abortController.abort();
},
{ once: true },
);
}📝 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.
| // Setup abort controller | |
| const abortController = new AbortController(); | |
| // Connect external signal to internal abort controller | |
| if (signal) { | |
| signal.addEventListener( | |
| "abort", | |
| () => { | |
| abortController.abort(); | |
| }, | |
| { once: true }, | |
| ); | |
| } | |
| // Setup abort controller | |
| const abortController = new AbortController(); | |
| // Connect external signal to internal abort controller | |
| if (signal) { | |
| if (signal.aborted) { | |
| abortController.abort(); | |
| } | |
| signal.addEventListener( | |
| "abort", | |
| () => { | |
| abortController.abort(); | |
| }, | |
| { once: true }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@packages/agent/src/agent-executor.ts` around lines 79 - 91, The abort logic
fails when an external AbortSignal is already aborted because the "abort" event
won't fire; update the setup in agent-executor.ts so after creating
abortController it checks if the incoming signal exists and has signal.aborted
=== true and, if so, immediately call abortController.abort(), then still attach
the signal.addEventListener("abort", ...) with { once: true } to handle future
aborts; reference the abortController and signal variables when making this
change.
| @@ -0,0 +1,13 @@ | |||
| export { executeAgent } from "./agent-executor"; | |||
| export type { ExecuteAgentParams, ExecuteAgentResult } from "./types"; | |||
There was a problem hiding this comment.
Missing type exports that consumers need for callback implementations.
Only ExecuteAgentParams and ExecuteAgentResult are re-exported from types.ts, but consumers implementing onPermissionRequest and onEvent callbacks will need PermissionRequestParams, PermissionResult, AgentEvent, and PermissionMode.
Proposed fix
-export type { ExecuteAgentParams, ExecuteAgentResult } from "./types";
+export type {
+ ExecuteAgentParams,
+ ExecuteAgentResult,
+ PermissionRequestParams,
+ PermissionResult,
+ AgentEvent,
+ PermissionMode,
+} from "./types";🤖 Prompt for AI Agents
In `@packages/agent/src/index.ts` at line 2, The package is missing re-exports for
types needed by consumers implementing callbacks; update the re-exports in the
module (packages/agent/src/index.ts) to also export PermissionRequestParams,
PermissionResult, AgentEvent, and PermissionMode from "./types" so consumers
implementing onPermissionRequest and onEvent can import the correct types;
locate the existing export line that re-exports
ExecuteAgentParams/ExecuteAgentResult and add the four missing type names to
that export list.
| export type PermissionResult = | ||
| | { behavior: "allow"; updatedInput: Record<string, unknown> } | ||
| | { behavior: "deny"; message: string }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
updatedInput should be optional on the "allow" variant.
When approving a tool use without modifications, callers shouldn't need to pass the original input back. Making it required adds unnecessary friction.
Proposed fix
export type PermissionResult =
- | { behavior: "allow"; updatedInput: Record<string, unknown> }
+ | { behavior: "allow"; updatedInput?: Record<string, unknown> }
| { behavior: "deny"; message: string };📝 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.
| export type PermissionResult = | |
| | { behavior: "allow"; updatedInput: Record<string, unknown> } | |
| | { behavior: "deny"; message: string }; | |
| export type PermissionResult = | |
| | { behavior: "allow"; updatedInput?: Record<string, unknown> } | |
| | { behavior: "deny"; message: string }; |
🤖 Prompt for AI Agents
In `@packages/agent/src/types.ts` around lines 68 - 70, The PermissionResult
union's "allow" variant currently requires updatedInput; change it so
updatedInput is optional by updating the type definition for PermissionResult
(the union that includes { behavior: "allow"; updatedInput: Record<string,
unknown> }) to make updatedInput?: Record<string, unknown>, so callers can
return an allow without providing the original input. Ensure only the "allow"
branch is modified and keep the "deny" branch ({ behavior: "deny"; message:
string }) unchanged.
| export interface ExecuteAgentResult { | ||
| success: boolean; | ||
| error?: string; | ||
| messageId: string; | ||
| runId: string; | ||
| } |
There was a problem hiding this comment.
messageId and runId should be optional on failure.
When success is false, there may be no messageId or runId (e.g., if the error occurs before the SDK session is established). Making them required forces callers to fabricate dummy values on error paths.
Proposed fix
export interface ExecuteAgentResult {
success: boolean;
error?: string;
- messageId: string;
- runId: string;
+ messageId?: string;
+ runId?: string;
}🤖 Prompt for AI Agents
In `@packages/agent/src/types.ts` around lines 72 - 77, The ExecuteAgentResult
interface currently requires messageId and runId even on failure; update the
interface (ExecuteAgentResult) to make messageId and runId optional (messageId?:
string; runId?: string) so callers don’t need to fabricate values when success
is false, and scan for usages of ExecuteAgentResult to handle undefined
messageId/runId where error paths may not provide them.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`:
- Around line 272-281: The onPermissionRequest handler currently auto-approves
every request (and permissionMode defaults to "bypassPermissions"); change this
to a safe default by rejecting or requiring opt-in: update onPermissionRequest
to check a session-level or config flag (e.g., a new allowAutoApprove boolean on
the session or constructor) and only return { behavior: "allow" } when that flag
is explicitly true, otherwise return { behavior: "deny" } or only allow for a
curated list of read-only tool names (check params.toolName/toolUseId) before
allowing; also adjust the default for permissionMode to a restrictive setting
unless the opt-in flag is set so auto-approval cannot be bypassed
unintentionally.
- Around line 218-220: runAgent currently creates a new AbortController and
assigns it to session.abortController without checking for an existing run, so
concurrent calls for the same sessionId can overwrite the controller and prevent
aborting the first run; update runAgent to guard by checking the session state
(e.g., a boolean like session.isRunning or existence of session.abortController)
and either reject the second call with an error or first abort the previous run
(call session.abortController.abort() and await/reset any related state) before
creating a new AbortController, ensuring you update session.abortController and
session.isRunning (or equivalent) consistently around the lifecycle of the agent
run.
- Around line 259-270: The onChunk handler currently fires a fetch to
`${PROXY_URL}/v1/sessions/${sessionId}/chunks` but ignores the response; update
the onChunk implementation to check the fetch response (response.ok) and handle
non-OK responses by logging the error (include status and body/error text) and
propagating failure so the agent can be aborted; specifically, after the fetch
in onChunk, await response.text() or json() on error, console.error (or the
module's logger) with the sessionId/messageId and response details, and throw or
return a rejected promise so upstream can detect the failure — additionally
implement a simple retry or failure counter inside onChunk (e.g., increment a
local failedChunkCount and, after N failures, throw to force agent abort) to
avoid silently dropping repeated chunk POST failures.
- Line 239: The code currently casts (await startRes.json()) as { messageId:
string } and assumes messageId is present; change this to validate the external
response from startRes.json() before using it (e.g., with a small Zod schema or
an explicit type guard) in the session manager logic that reads messageId so you
never proceed with undefined IDs when calling the chunk POST path; if validation
fails, handle it explicitly (throw or return a controlled error/rollback and log
via the same logger) and reference the startRes variable and the messageId usage
in the session-manager.ts flow to locate the spot to add the validation and
error handling.
- Around line 295-313: The finish POST to
`${PROXY_URL}/v1/sessions/${sessionId}/generations/finish` must always run even
on errors or aborts; move the fetch call that posts the empty body (currently
inside the try after executeAgent) into the finally block (where
session.abortController is cleared) so the proxy can't remain in a "generating"
state, and remove the duplicate call inside the try (or keep only the
finally-call if you prefer a single guaranteed notification). Update references
to sessionId, PROXY_URL, executeAgent, and session.abortController accordingly.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts (2)
435-444:updateSessionMetauses positional parameters.This method takes
(sessionId, patch)as positional args, while all other methods in this class consistently use a single object parameter. This violates the coding guideline for functions with 2+ parameters.Proposed fix
- async updateSessionMeta( - sessionId: string, - patch: { + async updateSessionMeta({ + sessionId, + patch, + }: { + sessionId: string; + patch: { title?: string; messagePreview?: string; providerSessionId?: string; - }, + }; }): Promise<void> { await this.store.update(sessionId, patch); }Note: callers of this method will need to be updated accordingly.
As per coding guidelines: "Use object parameters for functions with 2 or more parameters instead of positional arguments."
57-64: Constructor performs side effects (filesystem initialization).
initSessionStore(dataDir)initializes on-disk storage during construction. If this call fails (e.g., permission error), the constructor throws and the module-level singleton inindex.tswill crash the app at import time. Consider wrapping this in a try/catch with a meaningful error message, or deferring initialization to a dedicatedinit()method.
| // Create abort controller for this generation | ||
| const abortController = new AbortController(); | ||
| session.abortController = abortController; |
There was a problem hiding this comment.
No guard against concurrent runAgent calls for the same session.
If runAgent is called twice for the same sessionId before the first completes, the second call overwrites session.abortController on line 220, making it impossible to abort the first run. Consider checking if an agent is already running and either rejecting the call or aborting the previous one first.
Suggested guard
// Create abort controller for this generation
+ if (session.abortController) {
+ console.warn(`[chat/session] Aborting previous agent run for ${sessionId}`);
+ session.abortController.abort();
+ }
const abortController = new AbortController();
session.abortController = abortController;📝 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.
| // Create abort controller for this generation | |
| const abortController = new AbortController(); | |
| session.abortController = abortController; | |
| // Create abort controller for this generation | |
| if (session.abortController) { | |
| console.warn(`[chat/session] Aborting previous agent run for ${sessionId}`); | |
| session.abortController.abort(); | |
| } | |
| const abortController = new AbortController(); | |
| session.abortController = abortController; |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 218 - 220, runAgent currently creates a new AbortController and
assigns it to session.abortController without checking for an existing run, so
concurrent calls for the same sessionId can overwrite the controller and prevent
aborting the first run; update runAgent to guard by checking the session state
(e.g., a boolean like session.isRunning or existence of session.abortController)
and either reject the second call with an error or first abort the previous run
(call session.abortController.abort() and await/reset any related state) before
creating a new AbortController, ensuring you update session.abortController and
session.isRunning (or equivalent) consistently around the lifecycle of the agent
run.
| throw new Error(`Failed to start generation: ${startRes.status}`); | ||
| } | ||
|
|
||
| const { messageId } = (await startRes.json()) as { messageId: string }; |
There was a problem hiding this comment.
Unvalidated response from external proxy.
The fetch result is cast with as { messageId: string } without any validation. Per coding guidelines, external API data should be treated as untrusted. If the proxy returns an unexpected shape, messageId will be undefined and subsequent chunk POSTs will silently use a bad ID.
Suggested: validate with Zod or a guard
- const { messageId } = (await startRes.json()) as { messageId: string };
+ const startBody = await startRes.json();
+ const messageId = typeof startBody?.messageId === "string"
+ ? startBody.messageId
+ : (() => { throw new Error("Invalid start generation response: missing messageId"); })();Or use a small Zod schema if preferred.
As per coding guidelines: "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks."
📝 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.
| const { messageId } = (await startRes.json()) as { messageId: string }; | |
| const startBody = await startRes.json(); | |
| const messageId = typeof startBody?.messageId === "string" | |
| ? startBody.messageId | |
| : (() => { throw new Error("Invalid start generation response: missing messageId"); })(); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
at line 239, The code currently casts (await startRes.json()) as { messageId:
string } and assumes messageId is present; change this to validate the external
response from startRes.json() before using it (e.g., with a small Zod schema or
an explicit type guard) in the session manager logic that reads messageId so you
never proceed with undefined IDs when calling the chunk POST path; if validation
fails, handle it explicitly (throw or return a controlled error/rollback and log
via the same logger) and reference the startRes variable and the messageId usage
in the session-manager.ts flow to locate the spot to add the validation and
error handling.
| onChunk: async (chunk: StreamChunk) => { | ||
| await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/chunks`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({ | ||
| messageId, | ||
| actorId: "claude", | ||
| role: "assistant", | ||
| chunk, | ||
| }), | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Chunk POST failures are silently ignored.
onChunk fires a fetch but never checks response.ok. If the proxy rejects or drops a chunk, the agent continues unaware and the user sees a gap in the streamed output with no error signal.
At minimum, log failures. Ideally, abort the agent on repeated failures.
Proposed fix — check response
onChunk: async (chunk: StreamChunk) => {
- await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/chunks`, {
+ const res = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/chunks`, {
method: "POST",
headers,
body: JSON.stringify({
messageId,
actorId: "claude",
role: "assistant",
chunk,
}),
});
+ if (!res.ok) {
+ console.error(
+ `[chat/session] Chunk POST failed: ${res.status} ${res.statusText}`,
+ );
+ }
},📝 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.
| onChunk: async (chunk: StreamChunk) => { | |
| await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/chunks`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify({ | |
| messageId, | |
| actorId: "claude", | |
| role: "assistant", | |
| chunk, | |
| }), | |
| }); | |
| }, | |
| onChunk: async (chunk: StreamChunk) => { | |
| const res = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/chunks`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify({ | |
| messageId, | |
| actorId: "claude", | |
| role: "assistant", | |
| chunk, | |
| }), | |
| }); | |
| if (!res.ok) { | |
| console.error( | |
| `[chat/session] Chunk POST failed: ${res.status} ${res.statusText}`, | |
| ); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 259 - 270, The onChunk handler currently fires a fetch to
`${PROXY_URL}/v1/sessions/${sessionId}/chunks` but ignores the response; update
the onChunk implementation to check the fetch response (response.ok) and handle
non-OK responses by logging the error (include status and body/error text) and
propagating failure so the agent can be aborted; specifically, after the fetch
in onChunk, await response.text() or json() on error, console.error (or the
module's logger) with the sessionId/messageId and response details, and throw or
return a rejected promise so upstream can detect the failure — additionally
implement a simple retry or failure counter inside onChunk (e.g., increment a
local failedChunkCount and, after N failures, throw to force agent abort) to
avoid silently dropping repeated chunk POST failures.
| // Handle permissions (TODO: implement via tRPC events) | ||
| onPermissionRequest: async (params) => { | ||
| console.log( | ||
| "[chat/session] Permission request:", | ||
| params.toolName, | ||
| params.toolUseId, | ||
| ); | ||
| // For now, auto-approve | ||
| return { behavior: "allow", updatedInput: {} }; | ||
| }, |
There was a problem hiding this comment.
Auto-approving all permission requests is a security gap.
The onPermissionRequest handler unconditionally returns { behavior: "allow" }, combined with the default permissionMode of "bypassPermissions" on line 254. This means the agent can execute any tool — file writes, shell commands, etc. — without user consent.
The TODO on line 272 acknowledges this, but shipping it as-is is risky. At a minimum, consider gating this behind an explicit opt-in flag or restricting auto-approve to read-only tools until the tRPC permission flow is implemented.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 272 - 281, The onPermissionRequest handler currently auto-approves
every request (and permissionMode defaults to "bypassPermissions"); change this
to a safe default by rejecting or requiring opt-in: update onPermissionRequest
to check a session-level or config flag (e.g., a new allowAutoApprove boolean on
the session or constructor) and only return { behavior: "allow" } when that flag
is explicitly true, otherwise return { behavior: "deny" } or only allow for a
curated list of read-only tool names (check params.toolName/toolUseId) before
allowing; also adjust the default for permissionMode to a restrictive setting
unless the opt-in flag is set so auto-approval cannot be bypassed
unintentionally.
| // Finish generation | ||
| await fetch( | ||
| `${PROXY_URL}/v1/sessions/${sessionId}/generations/finish`, | ||
| { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({}), | ||
| }, | ||
| ); | ||
|
|
||
| console.log(`[chat/session] Agent completed for ${sessionId}`); | ||
| } catch (error) { | ||
| if ((error as Error).name !== "AbortError") { | ||
| console.error(`[chat/session] Agent execution failed:`, error); | ||
| } | ||
| throw error; | ||
| } finally { | ||
| session.abortController = undefined; | ||
| } |
There was a problem hiding this comment.
Generation not finished on error — proxy left in dangling state.
If executeAgent throws (or is aborted), the finish POST on lines 296–303 is skipped because it's inside the try block after executeAgent. The proxy will remain in a "generating" state for this session, likely blocking future generations.
Move the finish call into the finally block so it always fires.
Proposed fix
} catch (error) {
if ((error as Error).name !== "AbortError") {
console.error(`[chat/session] Agent execution failed:`, error);
}
throw error;
} finally {
+ // Always finish generation so proxy doesn't stay in "generating" state
+ try {
+ await fetch(
+ `${PROXY_URL}/v1/sessions/${sessionId}/generations/finish`,
+ {
+ method: "POST",
+ headers: buildProxyHeaders(),
+ body: JSON.stringify({}),
+ },
+ );
+ } catch (finishErr) {
+ console.error(`[chat/session] Failed to finish generation:`, finishErr);
+ }
session.abortController = undefined;
}And remove the duplicate finish call from lines 296–303 inside the try block (or keep it there too if you want early notification on success — but at minimum the finally version is needed).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 295 - 313, The finish POST to
`${PROXY_URL}/v1/sessions/${sessionId}/generations/finish` must always run even
on errors or aborts; move the fetch call that posts the empty body (currently
inside the try after executeAgent) into the finally block (where
session.abortController is cleared) so the proxy can't remain in a "generating"
state, and remove the duplicate call inside the try (or keep only the
finally-call if you prefer a single guaranteed notification). Update references
to sessionId, PROXY_URL, executeAgent, and session.abortController accordingly.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/package.json`:
- Line 38: Remove the redundant direct dependencies
"@anthropic-ai/claude-agent-sdk" and "@tanstack/ai" from the dependencies block
in the desktop package.json so they are resolved transitively via the existing
"@superset/agent" dependency; ensure no direct imports of these packages exist
in the desktop source (verify under src/) and then regenerate the lockfile
(npm/yarn install) to update the resolved dependency tree.
In `@packages/agent/package.json`:
- Line 18: The package.json references a non-existent version for
"@anthropic-ai/claude-agent-sdk" (currently "^0.2.38"); change that dependency
version to the published "^0.2.37" so npm install succeeds. Locate the
"@anthropic-ai/claude-agent-sdk" entry in package.json and update its version
string from "^0.2.38" to "^0.2.37".
🧹 Nitpick comments (1)
apps/streams/src/env.ts (1)
6-6: Consider adding port range validation.
z.coerce.number()accepts any number including floats, negatives, and values above 65535. The same applies toSTREAMS_INTERNAL_PORTon line 7, so this is a pre-existing pattern — but since you're touching this line, it's a good opportunity to tighten it.♻️ Suggested tightening
- STREAMS_PORT: z.coerce.number(), - STREAMS_INTERNAL_PORT: z.coerce.number(), + STREAMS_PORT: z.coerce.number().int().min(1).max(65535), + STREAMS_INTERNAL_PORT: z.coerce.number().int().min(1).max(65535),
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "^0.2.19", | ||
| "@anthropic-ai/claude-agent-sdk": "^0.2.38", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if desktop code directly imports from these packages
echo "=== Direct imports of `@anthropic-ai/claude-agent-sdk` in desktop ==="
rg --type ts --type tsx -l '@anthropic-ai/claude-agent-sdk' apps/desktop/src/ 2>/dev/null | head -20 || echo "No direct imports found"
echo ""
echo "=== Direct imports of `@tanstack/ai` in desktop ==="
rg --type ts --type tsx -l '@tanstack/ai' apps/desktop/src/ 2>/dev/null | head -20 || echo "No direct imports found"
echo ""
echo "=== Check `@superset/agent` package.json for dependencies ==="
if [ -f "apps/desktop/@superset/agent/package.json" ]; then
cat "apps/desktop/@superset/agent/package.json" | head -50
elif [ -f "packages/superset-agent/package.json" ]; then
cat "packages/superset-agent/package.json" | head -50
elif [ -f "packages/agent/package.json" ]; then
cat "packages/agent/package.json" | head -50
else
find . -path "*/node_modules" -prune -o -name "package.json" -type f -print | xargs grep -l '"name".*"@superset/agent"' 2>/dev/null | head -5
fiRepository: superset-sh/superset
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Get full context of apps/desktop/package.json around the flagged lines
echo "=== apps/desktop/package.json (lines 30-70) ==="
sed -n '30,70p' apps/desktop/package.jsonRepository: superset-sh/superset
Length of output: 1587
Remove redundant direct dependencies on @anthropic-ai/claude-agent-sdk and @tanstack/ai.
Desktop already depends on @superset/agent, which declares both packages with identical versions. No direct imports of these packages were found in apps/desktop/src/, meaning they resolve entirely through the agent package. Removing them here (lines 38 and 62) will resolve the packages transitively via @superset/agent and eliminates the risk of version drift between the two package.json files.
Also applies to: 40-40, 62-62
🤖 Prompt for AI Agents
In `@apps/desktop/package.json` at line 38, Remove the redundant direct
dependencies "@anthropic-ai/claude-agent-sdk" and "@tanstack/ai" from the
dependencies block in the desktop package.json so they are resolved transitively
via the existing "@superset/agent" dependency; ensure no direct imports of these
packages exist in the desktop source (verify under src/) and then regenerate the
lockfile (npm/yarn install) to update the resolved dependency tree.
| "test": "bun test" | ||
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "^0.2.38", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@anthropic-ai/claude-agent-sdk npm version 0.2.38
💡 Result:
@anthropic-ai/claude-agent-sdk does not appear to have an npm release 0.2.38; the latest version publicly indexed via jsDelivr is 0.2.37. [1]
Version 0.2.38 does exist for the legacy package @anthropic-ai/claude-code (the predecessor that was renamed to @anthropic-ai/claude-agent-sdk). [2]
Install commands
- Agent SDK:
npm i@anthropic-ai/claude-agent-sdk@0.2.37[1] - Legacy Claude Code SDK:
npm i@anthropic-ai/claude-code@0.2.38[2]
Sources: [1] [2]
Update @anthropic-ai/claude-agent-sdk to version 0.2.37 — version 0.2.38 does not exist for this package.
Version 0.2.38 is not published for @anthropic-ai/claude-agent-sdk. The latest available version is 0.2.37. Version 0.2.38 only exists for the legacy @anthropic-ai/claude-code package (the predecessor that was renamed). This will cause npm install to fail. Update the version to 0.2.37.
🤖 Prompt for AI Agents
In `@packages/agent/package.json` at line 18, The package.json references a
non-existent version for "@anthropic-ai/claude-agent-sdk" (currently "^0.2.38");
change that dependency version to the published "^0.2.37" so npm install
succeeds. Locate the "@anthropic-ai/claude-agent-sdk" entry in package.json and
update its version string from "^0.2.38" to "^0.2.37".
Kitenite
force-pushed
the
kitenite/refactor-agent-out
branch
from
6dcb86c to
952f516
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/ai-chat/index.ts`:
- Around line 229-237: The sendMessage mutation currently awaits
chatSessionManager.startAgent which in turn awaits executeAgent and can block
for minutes; change sendMessage to fire-and-forget the agent run so the mutation
returns immediately: call startAgent without awaiting (e.g., void
chatSessionManager.startAgent({...})) and attach a .catch handler to propagate
errors via the existing event/stream mechanism (use the same channel consumed by
streamEvents) so progress and failures are delivered to clients rather than
blocking the tRPC mutation; update sendMessage to return { success: true }
immediately after starting the agent.
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`:
- Around line 243-249: The fetch to
`${PROXY_URL}/v1/sessions/${sessionId}/generations/start` (using headers and an
empty JSON body) must validate the response before proceeding: after awaiting
fetch, check response.ok and if false read response.text() (or response.json()
when appropriate) and throw or return an error so the surrounding try/catch can
abort generation; update the code around that call (the start request in
session-manager.ts) to log the status and response body and stop further
processing when the proxy rejects the start request.
- Around line 293-299: The runtime dynamic import of createPermissionRequest
inside the onPermissionRequest block should be removed and replaced with the
statically imported symbol; add createPermissionRequest to the existing static
import list for "@superset/agent" at the top of the file, then update the
onPermissionRequest implementation to call createPermissionRequest({ toolUseId:
params.toolUseId, signal: params.signal }) directly instead of using await
import(...).
🧹 Nitpick comments (2)
apps/streams/src/routes/messages.ts (1)
18-34: Pre-existing: Zod validation errors return 500 instead of 400.If
stopGenerationRequestSchema.parse()throws aZodErroron malformed input, the catch block returns a generic 500. Consider distinguishing validation errors to return 400 (bad request). Also, theconsole.erroron Line 26 doesn't follow the[domain/operation]prefix pattern.♻️ Suggested improvement
app.post("/:sessionId/stop", async (c) => { const sessionId = c.req.param("sessionId"); try { const rawBody = await c.req.json(); const body = stopGenerationRequestSchema.parse(rawBody); await protocol.stopGeneration(sessionId, body.messageId ?? null); return new Response(null, { status: 204 }); } catch (error) { + if (error instanceof z.ZodError) { + return c.json({ error: "Invalid request body", details: error.errors }, 400); + } - console.error("Failed to stop generation:", error); + console.error("[messages/stop] Failed to stop generation:", error); return c.json( { error: "Failed to stop generation", details: (error as Error).message, }, 500, ); } });As per coding guidelines: "Use prefixed console logging with consistent context pattern: [domain/operation] message" and "Use appropriate TRPCError codes consistently: … BAD_REQUEST (invalid input)". While this is Hono (not tRPC), the same HTTP semantics apply.
packages/agent/src/types.ts (1)
81-85: Consider enriching thecompletedevent with result data.The
completedvariant carries no payload. Including theExecuteAgentResult(or at leastmessageId/runId) would letonEventsubscribers observe outcomes without needing a separate channel.- | { type: "completed" }; + | { type: "completed"; result: ExecuteAgentResult };This is optional — skip if
onEventis intentionally limited to lifecycle signals.
| sendMessage: publicProcedure | ||
| .input(z.object({ sessionId: z.string(), text: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| await chatSessionManager.startAgent({ | ||
| sessionId: input.sessionId, | ||
| prompt: input.text, | ||
| }); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
sendMessage mutation blocks until the entire agent run completes — likely causing client-side timeouts or frozen UI.
startAgent awaits executeAgent, which can run for minutes (tool calls, thinking, multi-turn). The tRPC mutation won't resolve until the agent finishes, so the caller is stuck waiting. Consider firing the agent without awaiting, returning immediately, and relying on the existing streamEvents subscription for progress/completion signals.
Proposed fix — fire and forget with error propagation via events
sendMessage: publicProcedure
.input(z.object({ sessionId: z.string(), text: z.string() }))
.mutation(async ({ input }) => {
- await chatSessionManager.startAgent({
+ // Fire agent execution without blocking the mutation response.
+ // Progress and errors are delivered via the streamEvents subscription.
+ chatSessionManager.startAgent({
sessionId: input.sessionId,
prompt: input.text,
+ }).catch((err) => {
+ console.error(`[ai-chat/sendMessage] Agent failed:`, err);
});
return { success: true };
}),📝 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.
| sendMessage: publicProcedure | |
| .input(z.object({ sessionId: z.string(), text: z.string() })) | |
| .mutation(async ({ input }) => { | |
| await chatSessionManager.startAgent({ | |
| sessionId: input.sessionId, | |
| prompt: input.text, | |
| }); | |
| return { success: true }; | |
| }), | |
| sendMessage: publicProcedure | |
| .input(z.object({ sessionId: z.string(), text: z.string() })) | |
| .mutation(async ({ input }) => { | |
| // Fire agent execution without blocking the mutation response. | |
| // Progress and errors are delivered via the streamEvents subscription. | |
| chatSessionManager.startAgent({ | |
| sessionId: input.sessionId, | |
| prompt: input.text, | |
| }).catch((err) => { | |
| console.error(`[ai-chat/sendMessage] Agent failed:`, err); | |
| }); | |
| return { success: true }; | |
| }), |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/ai-chat/index.ts` around lines 229 - 237,
The sendMessage mutation currently awaits chatSessionManager.startAgent which in
turn awaits executeAgent and can block for minutes; change sendMessage to
fire-and-forget the agent run so the mutation returns immediately: call
startAgent without awaiting (e.g., void chatSessionManager.startAgent({...}))
and attach a .catch handler to propagate errors via the existing event/stream
mechanism (use the same channel consumed by streamEvents) so progress and
failures are delivered to clients rather than blocking the tRPC mutation; update
sendMessage to return { success: true } immediately after starting the agent.
| try { | ||
| // Signal generation start to proxy | ||
| await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/generations/start`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({}), | ||
| }); |
There was a problem hiding this comment.
Generation start response is not validated.
The fetch to /generations/start doesn't check response.ok. If the proxy rejects the request (e.g., 4xx/5xx), the agent proceeds to execute anyway against a session the proxy doesn't consider "generating," leading to chunks posted into an unexpected state.
Proposed fix
// Signal generation start to proxy
- await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/generations/start`, {
+ const startRes = await fetch(`${PROXY_URL}/v1/sessions/${sessionId}/generations/start`, {
method: "POST",
headers,
body: JSON.stringify({}),
});
+ if (!startRes.ok) {
+ throw new Error(
+ `POST /generations/start failed: ${startRes.status} ${startRes.statusText}`,
+ );
+ }🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 243 - 249, The fetch to
`${PROXY_URL}/v1/sessions/${sessionId}/generations/start` (using headers and an
empty JSON body) must validate the response before proceeding: after awaiting
fetch, check response.ok and if false read response.text() (or response.json()
when appropriate) and throw or return an error so the surrounding try/catch can
abort generation; update the code around that call (the start request in
session-manager.ts) to log the status and response body and stop further
processing when the proxy rejects the start request.
| // Use the agent package's built-in permission system | ||
| const { createPermissionRequest } = await import("@superset/agent"); | ||
| return createPermissionRequest({ | ||
| toolUseId: params.toolUseId, | ||
| signal: params.signal, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unnecessary dynamic import — createPermissionRequest is available from the same package already statically imported.
Line 294 uses await import("@superset/agent") at runtime, but @superset/agent is already statically imported at the top of the file (lines 2–7). Add createPermissionRequest to the existing static import instead.
Proposed fix
At the top of the file:
import {
executeAgent,
getClaudeSessionId,
type PermissionRequestParams,
resolvePendingPermission,
+ createPermissionRequest,
} from "@superset/agent";In onPermissionRequest:
onPermissionRequest: async (params: PermissionRequestParams) => {
this.emit("event", {
type: "permission_request",
sessionId,
toolUseId: params.toolUseId,
toolName: params.toolName,
input: params.input,
} satisfies PermissionRequestEvent);
- const { createPermissionRequest } = await import("@superset/agent");
return createPermissionRequest({
toolUseId: params.toolUseId,
signal: params.signal,
});
},📝 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.
| // Use the agent package's built-in permission system | |
| const { createPermissionRequest } = await import("@superset/agent"); | |
| return createPermissionRequest({ | |
| toolUseId: params.toolUseId, | |
| signal: params.signal, | |
| }); | |
| }, | |
| // Use the agent package's built-in permission system | |
| return createPermissionRequest({ | |
| toolUseId: params.toolUseId, | |
| signal: params.signal, | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/lib/trpc/routers/ai-chat/utils/session-manager/session-manager.ts`
around lines 293 - 299, The runtime dynamic import of createPermissionRequest
inside the onPermissionRequest block should be removed and replaced with the
statically imported symbol; add createPermissionRequest to the existing static
import list for "@superset/agent" at the top of the file, then update the
onPermissionRequest implementation to call createPermissionRequest({ toolUseId:
params.toolUseId, signal: params.signal }) directly instead of using await
import(...).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/streams/fly.toml`:
- Line 12: Replace the DATA_DIR variable in fly.toml with the expected
STREAMS_DATA_DIR key so the env schema in env.ts (which reads STREAMS_DATA_DIR)
matches the fly config; specifically change the environment entry currently
named DATA_DIR to STREAMS_DATA_DIR so the server will validate and use the
mounted data directory.
| PORT = "8080" | ||
| STREAMS_PORT = "8080" | ||
| NODE_ENV = "production" | ||
| DATA_DIR = "/data" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what env var name the streams env schema expects for the data directory
rg -n 'DATA_DIR' apps/streams/src/env.tsRepository: superset-sh/superset
Length of output: 104
Update DATA_DIR to STREAMS_DATA_DIR in fly.toml.
The environment schema in env.ts (line 9) expects STREAMS_DATA_DIR, but fly.toml sets DATA_DIR. This mismatch will cause the environment validation to fail and the server won't pick up the data directory mount path. Change line 12 to:
STREAMS_DATA_DIR = "/data"
🤖 Prompt for AI Agents
In `@apps/streams/fly.toml` at line 12, Replace the DATA_DIR variable in fly.toml
with the expected STREAMS_DATA_DIR key so the env schema in env.ts (which reads
STREAMS_DATA_DIR) matches the fly config; specifically change the environment
entry currently named DATA_DIR to STREAMS_DATA_DIR so the server will validate
and use the mounted data directory.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Refactor
Chores