Repository navigation
Conversation
…de_session MCP results
…cy, consolidate navigation tools - create_workspace now requires projectId instead of deriving from active workspace - start_claude_session and start_claude_subagent now require workspaceId - switch_workspace is now UI-only (navigateToWorkspace instead of setActive) - Remove navigate_to_workspace (redundant with switch_workspace) - Remove getActiveWorkspaceId from ToolContext
📝 WalkthroughWalkthroughThis PR refactors tool parameter handling by replacing implicit context-based workspace derivation with explicit parameter passing. It removes the navigate-to-workspace tool entirely, adds required Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/start-claude-subagent.ts`:
- Around line 10-12: The execute function in start-claude-subagent.ts currently
has the signature async function execute(params: z.infer<typeof schema>):
Promise<CommandResult> which violates the ToolDefinition contract used
elsewhere; update the signature to accept the ToolContext as a second parameter
(e.g., async function execute(params: z.infer<typeof schema>, _ctx:
ToolContext): Promise<CommandResult>) so it matches other tools
(create-worktree, delete-workspace, switch-workspace) and keeps type
consistency; ensure you import or reference ToolContext if needed and prefix the
parameter with an underscore if unused.
🧹 Nitpick comments (5)
packages/mcp/src/tools/devices/start-claude-session/start-claude-session.ts (1)
84-94: Consider leveraging the ZodinputSchemainstead of manual validation with unsafe casts.The
as stringcasts on lines 89–91 bypass TypeScript's type checking — if a non-string value slips through, the cast won't catch it at runtime. Since the MCP SDK already validates against the ZodinputSchema(which declares all three fields asz.string()),validateArgsis likely redundant. If you want a belt-and-suspenders approach, consider parsing with a Zod schema instead:♻️ Suggested refactor
+const argsSchema = z.object({ + deviceId: z.string(), + taskId: z.string(), + workspaceId: z.string(), +}); + -function validateArgs(args: Record<string, unknown>): { - deviceId: string; - taskId: string; - workspaceId: string; -} | null { - const deviceId = args.deviceId as string; - const taskId = args.taskId as string; - const workspaceId = args.workspaceId as string; - if (!deviceId || !taskId || !workspaceId) return null; - return { deviceId, taskId, workspaceId }; +function validateArgs(args: Record<string, unknown>) { + const result = argsSchema.safeParse(args); + return result.success ? result.data : null; }packages/mcp/src/tools/devices/create-workspace/create-workspace.ts (2)
40-44: Type assertions (as string) bypass compile-time safety — pre-existing pattern.These casts assume the MCP SDK passes validated args. If it doesn't,
args.projectIdcould beundefinedat runtime despite theas stringassertion. The downstream!projectIdguard mitigates this, but a safer approach would be to parseargsthrough a Zod schema at the top of the handler (e.g., a combined input schema.parse(args)), which would give both runtime validation and proper typing in one step. Not blocking since this follows the existingdeviceIdpattern.
46-58: Schema and runtime validation are subtly inconsistent for empty strings.
z.string()in the input schema (lines 27-30) permits"", but the runtime guards!deviceId/!projectIdreject it. If the MCP SDK validates against the schema first, an empty string would pass schema validation but then hit these error branches.Consider adding
.min(1)to thedeviceIdandprojectIdZod declarations so the two layers agree. This is a pre-existing inconsistency fordeviceId, and the newprojectIdcheck just follows the same pattern, so not blocking.Proposed fix to align schema with runtime checks
- deviceId: z.string().describe("Target device ID"), - projectId: z - .string() - .describe("Project ID to create workspaces in (use list_projects to find)"), + deviceId: z.string().min(1).describe("Target device ID"), + projectId: z + .string() + .min(1) + .describe("Project ID to create workspaces in (use list_projects to find)"),apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/create-worktree.ts (1)
16-19: Consider adding.min(1)toprojectIdto reject empty strings.
z.string()accepts empty strings. SinceprojectIdis a required identifier passed tocreateWorktree.mutateAsync, an empty string would likely cause a downstream failure with a less helpful error message. The same applies to theworkspaceIdfields added in other tools in this PR.Suggested fix
const schema = z.object({ - projectId: z.string(), + projectId: z.string().min(1), workspaces: z.array(workspaceInputSchema).min(1).max(5), });apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/start-claude-session.ts (1)
30-30: Stale step number in comment.The comment reads
// 3. Append command to pending terminal setup— the preceding numbered steps were likely removed during this refactor, making the "3." misleading.Suggested fix
- // 3. Append command to pending terminal setup + // Append command to pending terminal setup
| async function execute( | ||
| params: z.infer<typeof schema>, | ||
| ctx: ToolContext, | ||
| ): Promise<CommandResult> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the ToolDefinition interface
find . -name 'types.ts' -path '*/useCommandWatcher/*' -o -name 'types.ts' -path '*/tools/*' | head -20Repository: superset-sh/superset
Length of output: 174
🏁 Script executed:
# Search for ToolDefinition type definition
rg -n 'interface ToolDefinition|type ToolDefinition' --type=tsRepository: superset-sh/superset
Length of output: 230
🏁 Script executed:
# Check how execute is called in the command watcher
rg -n 'tool\.execute\|\.execute\(' --type=ts -B2 -A3 | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# List all tool files to see what tools exist
fd '\.ts$' -path '*/useCommandWatcher/tools/*'Repository: superset-sh/superset
Length of output: 232
🏁 Script executed:
# Read the ToolDefinition interface
cat -n apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/types.ts | head -100Repository: superset-sh/superset
Length of output: 2293
🏁 Script executed:
# Find all tool files
fd -g '*.ts' apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/Repository: superset-sh/superset
Length of output: 1364
🏁 Script executed:
# Search for where execute function is called/invoked
rg -n 'execute\(' --type=ts apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/ -B2 -A2Repository: superset-sh/superset
Length of output: 7040
Add ctx parameter to match the ToolDefinition interface and other tools.
The ToolDefinition interface requires execute to accept (params: z.infer<T>, ctx: ToolContext), but start-claude-subagent.ts omits the ctx parameter entirely. All other tools (create-worktree, delete-workspace, switch-workspace, etc.) include ctx, even when unused (prefixed with _). Add _ctx: ToolContext as the second parameter to maintain type contract consistency:
async function execute(
params: z.infer<typeof schema>,
_ctx: ToolContext,
): Promise<CommandResult> {🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/components/AgentHooks/hooks/useCommandWatcher/tools/start-claude-subagent.ts`
around lines 10 - 12, The execute function in start-claude-subagent.ts currently
has the signature async function execute(params: z.infer<typeof schema>):
Promise<CommandResult> which violates the ToolDefinition contract used
elsewhere; update the signature to accept the ToolContext as a second parameter
(e.g., async function execute(params: z.infer<typeof schema>, _ctx:
ToolContext): Promise<CommandResult>) so it matches other tools
(create-worktree, delete-workspace, switch-workspace) and keeps type
consistency; ensure you import or reference ToolContext if needed and prefix the
parameter with an underscore if unused.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Summary
create_workspacenow requiresprojectIdinstead of deriving it from active workspacestart_claude_sessionandstart_claude_subagentnow requireworkspaceIdswitch_workspaceis now UI-only (usesnavigateToWorkspaceinstead ofsetActive)navigate_to_workspacetool (redundant withswitch_workspace)getActiveWorkspaceIdfromToolContext— no tool depends on implicit active workspace statecreate_workspaceandstart_claude_sessionnow returnworktreePathin resultsTest plan
create_workspaceMCP tool works with explicitprojectIdstart_claude_sessionMCP tool works with explicitworkspaceIdstart_claude_subagentMCP tool works with explicitworkspaceIdswitch_workspacenavigates UI without callingsetActivenavigate_to_workspaceis no longer registered as an MCP toolSummary by CodeRabbit
Release Notes
Removals
Changes