Conversation
- Added LaunchOverlay component to display loading state while attaching to agents in the dashboard. - Removed unnecessary launching state from panels, simplifying the agent attachment logic. - Updated launchAgent and attachToAgent functions to handle synchronous checks for tmux sessions, improving performance and reliability. - Adjusted database initialization to use a default path from configuration instead of hardcoded values.
WalkthroughAdds new interactive CLI UI components (Init, Dashboard, Panels), workspace-scoped agent management (start, attach, stop-all), tmux-based launch/attach tooling, orchestrator/storage/type refinements (backfills, current workspace, options-based create, ProcessStatus), tests, docs (including new CLAUDE.md), and packaging/build adjustments for the CLI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as superset CLI
participant Init as Init Wizard
participant WSOrch as WorkspaceOrchestrator
participant EnvOrch as EnvironmentOrchestrator
participant DB as DB
User->>CLI: run "superset init"
CLI->>Init: launch wizard
Init->>Init: collect type/path/branch/name/agents
Init->>WSOrch: validate/create (envId, type, options)
WSOrch->>DB: read/write workspaces & state
EnvOrch->>DB: ensure/select environment
WSOrch-->>Init: created workspace id
Init->>User: show summary and exit
sequenceDiagram
participant User
participant Dashboard
participant Orchs as Orchestrators
participant DB as DB
participant Launcher
participant Tmux
User->>Dashboard: open
Dashboard->>Orchs: parallel load (workspaces, processes, current)
Orchs->>DB: fetch & backfill defaults
DB-->>Orchs: data
Orchs-->>Dashboard: DashboardData
Dashboard->>Dashboard: render + poll
User->>Dashboard: select agent + Enter
Dashboard->>Launcher: launchAgent(agent,{attach:true})
Launcher->>Tmux: check/create/attach session
alt session reachable
Tmux-->>Launcher: attach success
Launcher-->>Dashboard: attach result
else fail
Tmux-->>Launcher: error
Launcher-->>Orchs: mark agent ERROR
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageToolapps/cli/readme.md[style] ~43-~43: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE) 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (1)
178-179: Pre-existing bug: Undefined variable references.These lines reference
onTabSelectandonTabCreated, but those variables are not defined. Based on lines 18-19, the correct variable names arehandleTabSelectandhandleTabCreated. This would cause runtime errors when rendering the Kanban columns.Note: This bug pre-dates this PR (these lines were not changed), but should be addressed.
Apply this diff to fix the undefined references:
onTabSelect={handleTabSelect} - onTabCreated={onTabCreated} + onTabCreated={handleTabCreated}Apply this same pattern to all five KanbanColumn instances (lines 178-179, 189-190, 200-201, 211-212, 223-224).
Also applies to: 189-190, 200-201, 211-212, 223-224
apps/old-desktop/src/main/lib/desktop-stores.ts (1)
52-90: Don’t swallow initialization/migration failures; gate version writes on successinitializeAsync currently catches all errors, logs, and then resolves, so DesktopStores.initialize() appears successful even if storage dir setup or legacy migration fail. Additionally, DomainVersion/ui version writes run regardless of whether migration succeeded, which can incorrectly mark versions as initialized after a failed migration.
- private async initializeAsync(): Promise<void> { - try { - // Ensure storage directories exist first - await ensureDesktopStorageDirs(); - - // Then run migration if needed - await this.runMigrationIfNeeded(); - } catch (error) { - console.error("Failed to initialize Desktop stores:", error); - } - } + private async initializeAsync(): Promise<void> { + try { + // Ensure storage directories exist first + await ensureDesktopStorageDirs(); + + // Then run migration if needed + await this.runMigrationIfNeeded(); + } catch (error) { + console.error("Failed to initialize Desktop stores:", error); + throw error; // surface failures to callers of DesktopStores.initialize() + } + } @@ - private async runMigrationIfNeeded(): Promise<void> { - const migrator = new LegacyMigrator(); - if (migrator.shouldMigrate()) { - console.log("[DesktopStores] Running migration from legacy config..."); - const result = await migrator.migrate( - this.environmentOrchestrator, - this.workspaceOrchestrator, - this.uiStore, - false, - ); - if (result.success) { - console.log( - `[DesktopStores] Migration completed: ${result.migrated.workspaces} workspaces migrated`, - ); - } else { - console.error(`[DesktopStores] Migration failed: ${result.error}`); - } - } - - // Initialize versions if not set - if (DomainVersion.read() === 0) { - DomainVersion.write(); - } - if (this.uiStore.readUiVersion() === 0) { - this.uiStore.writeUiVersion(); - } - } + private async runMigrationIfNeeded(): Promise<void> { + const migrator = new LegacyMigrator(); + let migrationOk = true; + + if (migrator.shouldMigrate()) { + console.log("[DesktopStores] Running migration from legacy config..."); + const result = await migrator.migrate( + this.environmentOrchestrator, + this.workspaceOrchestrator, + this.uiStore, + false, + ); + migrationOk = result.success; + if (result.success) { + console.log( + `[DesktopStores] Migration completed: ${result.migrated.workspaces} workspaces migrated`, + ); + } else { + console.error(`[DesktopStores] Migration failed: ${result.error}`); + throw new Error(`Legacy migration failed: ${result.error}`); + } + } + + // Initialize versions if not set, but only after successful migration / no migration + if (migrationOk && DomainVersion.read() === 0) { + DomainVersion.write(); + } + if (migrationOk && this.uiStore.readUiVersion() === 0) { + this.uiStore.writeUiVersion(); + } + }apps/cli/src/types/workspace.ts (1)
28-50: WorkspaceOrchestrator interface return types should be async to match implementation
WorkspaceOrchestrator.updateandWorkspaceOrchestrator.deleteare typed as synchronous (=> void), but the implementation inapps/cli/src/lib/orchestrators/workspace-orchestrator.tsisasyncand returnsPromise<void>. That mismatch will causeimplements IWorkspaceOrchestratortype errors and/or misleading call sites.Consider updating the interface to reflect the async contract:
export interface WorkspaceOrchestrator { @@ - update: (id: string, workspace: Partial<Workspace>) => void; + update: (id: string, workspace: Partial<Workspace>) => Promise<void>; @@ - delete: (id: string) => void; + delete: (id: string) => Promise<void>;
🧹 Nitpick comments (22)
apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (1)
88-92: Consider aligningexpandedGroupTabswith Tree’s actual open stateRight now
expandedGroupTabsis only used to computeinitialOpenState, whileonToggleand theselectedTabIdeffect update the set but don’t directly control the arborist tree’s open state. If the intent is to auto‑expand the group that contains the selected tab, you may want to confirm howinitialOpenStateis treated byreact-arboristand, if needed, switch to a controlled open‑state pattern (or call the tree API) so visual expansion always matchesexpandedGroupTabs.Also applies to: 108-125, 441-477
apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
9-12: Centralizing the drag-and-drop manager looks goodUsing the shared
dragDropManagerfromrenderer/lib/dndfor the MosaicdragAndDropManagerprop keeps DnD behavior consistent across the app and avoids per-component manager instances. Import consolidation for the tab/workspace contexts is also clean and idiomatic.If you touch this file again, consider trimming unused values from
renderTile’s dependency array (workingDirectory,workspaceId,worktreeId,handleTabFocus) to reduce unnecessary re-creations of the callback.Also applies to: 241-249
apps/old-desktop/src/renderer/screens/main/utils.ts (1)
2-2: Good reuse offormatRelativeTime; confirm timestamp choiceImporting
formatRelativeTimefrom the AddTaskModal utils keeps relative-time formatting consistent across the UI, and using it fortask.lastUpdatedis reasonable.One thing to double‑check:
lastUpdatedis derived fromworktree.createdAt. If theWorktreetype ever gains anupdatedAt/lastActivityfield, that would likely be a better source for this value.Also applies to: 98-99
apps/old-desktop/src/main/lib/workspace-ipcs.ts (1)
778-807: Consider explicit “workspace not found” handling in new-architecture IPCsIn
workspace-activateandworkspace-rescan, the code assumesworkspaceOrch.get(input.workspaceId)returns a workspace and immediately readsworkspace.type. Ifgetcan returnnull/undefined, this will throw and be caught by the genericcatch, producing a less clear error.You might prefer a direct guard like you use in earlier handlers:
- const workspace = await workspaceOrch.get(input.workspaceId); - if (workspace.type !== "local") { + const workspace = await workspaceOrch.get(input.workspaceId); + if (!workspace) { + return { + success: false, + error: "Workspace not found", + }; + } + if (workspace.type !== "local") { return { success: false, error: "Only local workspaces are supported", }; }Applying the same pattern to both IPCs would make error responses more predictable.
Also applies to: 851-881
apps/cli/src/components/LaunchOverlay.tsx (1)
13-41: Hoist animation frame arrays outside the component for stability/perfThe spinnerFrames and breathingFrames arrays are recreated on every render, and the effect depends on spinnerFrames.length. This works, but you can simplify and avoid per-render allocations by moving both arrays to module-level constants and changing the effect dependency to an empty array. That also makes the intent (“run once”) clearer.
-export function LaunchOverlay({ agentType, sessionName }: LaunchOverlayProps) { - const [frame, setFrame] = React.useState(0); - - // Braille spinner frames for smooth animation - const spinnerFrames = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; - - // Breathing dots animation - const breathingFrames = [ - "●", - "●○", - "●○○", - "○●○", - "○○●", - "○●", - "●", - "", - ]; +const SPINNER_FRAMES = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"] as const; +const BREATHING_FRAMES = ["●", "●○", "●○○", "○●○", "○○●", "○●", "●", ""] as const; + +export function LaunchOverlay({ agentType, sessionName }: LaunchOverlayProps) { + const [frame, setFrame] = React.useState(0); @@ - React.useEffect(() => { - const interval = setInterval(() => { - setFrame((prev) => (prev + 1) % spinnerFrames.length); - }, 80); - - return () => clearInterval(interval); - }, [spinnerFrames.length]); - - const spinner = spinnerFrames[frame]; - const breathing = breathingFrames[frame % breathingFrames.length]; + React.useEffect(() => { + const interval = setInterval(() => { + setFrame((prev) => (prev + 1) % SPINNER_FRAMES.length); + }, 80); + return () => clearInterval(interval); + }, []); + + const spinner = SPINNER_FRAMES[frame]; + const breathing = BREATHING_FRAMES[frame % BREATHING_FRAMES.length];apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts (1)
107-158: Prefer using the per-test tempDir for workspace pathsThe updated create calls correctly use the new
{ path }options shape. To keep tests more isolated and avoid relying on global/tmp/*paths (and potential collisions across parallel runs), consider deriving these paths from the per-test tempDir you already create.- const workspace = await workspaceOrchestrator.create( - env.id, - WorkspaceType.LOCAL, - { path: "/tmp/test" }, - ); + const workspace = await workspaceOrchestrator.create( + env.id, + WorkspaceType.LOCAL, + { path: join(tempDir, "workspace") }, + ); @@ - const workspace = await workspaceOrchestrator.create( - env.id, - WorkspaceType.LOCAL, - { path: "/tmp/test" }, - ); + const workspace = await workspaceOrchestrator.create( + env.id, + WorkspaceType.LOCAL, + { path: join(tempDir, "workspace") }, + ); @@ - const ws1 = await workspaceOrchestrator.create( - env.id, - WorkspaceType.LOCAL, - { path: "/tmp/test1" }, - ); - const ws2 = await workspaceOrchestrator.create( - env.id, - WorkspaceType.LOCAL, - { path: "/tmp/test2" }, - ); + const ws1 = await workspaceOrchestrator.create( + env.id, + WorkspaceType.LOCAL, + { path: join(tempDir, "ws1") }, + ); + const ws2 = await workspaceOrchestrator.create( + env.id, + WorkspaceType.LOCAL, + { path: join(tempDir, "ws2") }, + );apps/cli/src/lib/storage/adapter.ts (1)
3-9: Narrow get/set to collection-like keys to avoid misuse on non-Record fieldsCollectionValue is a nice improvement. Right now, though, get/set accept any keyof Database, which means you can (typesafely) call get("state", "id") even though state isn’t really an id-keyed collection.
You can tighten this by introducing a CollectionKey type that only includes Record-like collections and using that in get/set (and optionally getCollection/updateCollection).
-// Helper type to extract value type from Record collections -type CollectionValue<K extends keyof Database> = Database[K] extends Record< - string, - infer V -> - ? V - : never; +// Helper types to work only with Record<string, V> collections +type CollectionKey = { + [K in keyof Database]: Database[K] extends Record<string, any> ? K : never; +}[keyof Database]; + +type CollectionValue<K extends CollectionKey> = Database[K] extends Record< + string, + infer V +> + ? V + : never; @@ - get<K extends keyof Database>( + get<K extends CollectionKey>( collection: K, id: string, ): Promise<CollectionValue<K> | undefined>; @@ - set<K extends keyof Database>( + set<K extends CollectionKey>( collection: K, id: string, value: CollectionValue<K>, ): Promise<void>;Also applies to: 42-54
apps/cli/src/lib/storage/lowdb-adapter.ts (1)
155-156: Consider extracting repeated casting pattern.The pattern of casting
this.db!.data[collection] as Record<string, unknown>is repeated inget,set,delete, andhasmethods. Consider extracting this into a private helper method to reduce duplication.Example refactor:
private getCollectionAsRecord<K extends keyof Database>( collection: K ): Record<string, unknown> { return this.db!.data[collection] as Record<string, unknown>; }Then use it as:
const coll = this.getCollectionAsRecord(collection);Also applies to: 167-168, 178-179, 189-190
apps/cli/src/lib/launch/config.ts (2)
6-10: Document that default commands assume PATH availability.The default launch commands (
"claude","codex","cursor") assume these executables are in the user's PATH. Consider documenting this requirement in the docstring or README, and potentially validating command availability before attempting to launch.
28-28: Document the environment variable override mechanism.The
SUPERSET_AGENT_LAUNCH_<TYPE>environment variable pattern (e.g.,SUPERSET_AGENT_LAUNCH_CLAUDE) should be documented in user-facing documentation so users know they can override default launch commands.Also applies to: 50-50
apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
103-193: Consider centralizing “outdated launchCommand/sessionName” checks and caching tmux availability
needsPersistandbackfillDefaultsduplicate the heuristics for when an agent’slaunchCommand/sessionNameare missing or outdated. Similarly,isTmuxInstalled()shells out on everytmuxSessionExistscall.You could:
- Extract a small helper like
isLaunchConfigOutdated(agent: Agent): booleanto share betweenneedsPersistandbackfillDefaults.- Cache the result of
isTmuxInstalled()in a module-level variable to avoid repeatedly runningwhich tmuxinsidelist()loops, while still keepingtmuxSessionExistssynchronous.apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
94-147:createoptions and auto-select behavior are coherent; consider using CloudWorkspace typeThe new
create(environmentId, type, options)signature and per-type validation (pathfor LOCAL,branchfor CLOUD) are clear, and auto-selecting the created workspace viause()is a good default.Since
CloudWorkspaceis now defined intypes/workspace.ts, you could import it here and replace theas anycast for the CLOUD branch with a properCloudWorkspacecast for stronger typing.apps/cli/src/commands/dashboard.tsx (2)
57-82: Parallel data load is good; consider resetting error state on retry
loadDashboardcorrectly fetches workspaces, processes, and current workspace in parallel and updates state. However, onceerroris set in the catch block, it’s never cleared on subsequent successful refreshes, so the UI will stay in the error branch even if later calls succeed.
[signal_strength]You can reset error at the start of
loadDashboard:const loadDashboard = React.useCallback(async () => { - try { + try { + setError(null); const db = getDb();
149-193: LaunchOverlay/launching state is currently unreachableThe
launchingandlaunchingAgentstate, plus theLaunchOverlaybranch, are never activated; the Enter handler exits the Ink app immediately and runslaunchAgentviasetTimeout, so the overlay path can’t be reached.Either:
- Remove
launching/launchingAgentstate and theLaunchOverlaybranch to simplify the component, or- If you want the overlay, set
launching/launchingAgentbefore callingexit()(or avoidexit()until after the overlay is shown) so the branch can actually render.Also applies to: 226-236
apps/cli/src/lib/launch/run.ts (1)
17-38: Centralize tmux/session handling and consider portability
isTmuxInstalled,tmuxSessionExists, and the repeatedsessionNamederivation appear in multiple places. This is fine functionally, but you could:
- Extract a small helper (e.g.,
getSessionName(agent)) to avoid any future drift betweenlaunchAgentandattachToAgent.- Optionally replace
which tmuxwith a directspawnSync("tmux", ["-V"])-style check if you want better portability to environments withoutwhich.These are non-blocking but would improve maintainability and resilience.
Also applies to: 69-72, 140-144
apps/cli/src/types/process.ts (1)
47-62: ProcessOrchestrator interface still models async methods as synchronous
update,stop, anddeleteare typed as returningvoid, while the concreteProcessOrchestratorimplementation isasyncand returnsPromise<void>.stopAllwas updated toPromise<number>, but the others remain out of sync.For callers typed against this interface, this can be misleading (e.g., no
awaitimplied, no way to know when mutations persist). Consider adjusting to:update: (id: string, process: Partial<Process>) => Promise<void>; stop: (id: string) => Promise<void>; delete: (id: string) => Promise<void>;to reflect actual behavior and avoid subtle type mismatches.
apps/cli/src/commands/init.tsx (1)
58-88: Duplicate path/branch checks ignore environment and silently swallow DB errorsThe duplicate detection for paths and branches is reasonable, but two behaviors are worth verifying:
- You treat duplicates globally across all environments (
orchestrator.list()with no environment filter). If workspaces are meant to be unique per environment rather than globally, this will reject some valid setups.- The
catch (err) { /* Ignore database errors during validation */ }blocks mean DB issues will be silently ignored and the flow will proceed as if there were no duplicates, only to fail later when creating the workspace.If you care about these cases, consider:
- Filtering by
environmentIdwhen checking duplicates, or explicitly documenting that path/branch must be globally unique.- At least logging DB validation errors to stderr so they're not completely invisible.
Also applies to: 100-135
apps/cli/src/commands/panels.tsx (1)
79-128: Index wrap-around logic assumes non-empty arrays but degrades gracefullyThe navigation logic wraps indices with
filteredAgents.length - 1anddata.workspaces.length - 1. When the corresponding arrays are empty, the index can become-1, which then yieldsundefinedforselectedWorkspace/selectedAgent.Because you guard usages (
selectedAgent && ...) and treat undefined as “no selection,” this is not a functional bug, but it is slightly awkward state. If you want to tighten it up, you could early-return in the navigation branches when the relevant list is empty, or clamp indices to0whenlength === 0.Also applies to: 186-194
apps/cli/src/commands/agent.tsx (4)
276-341: Workspace-scopedAgentStopAllbehavior looks correct, but consider exposing the resolved workspace id
AgentStopAllcorrectly:
- Resolves a target workspace (explicit
workspaceIdor current workspace viaWorkspaceOrchestrator.getCurrent()),- Filters to non-terminated
ProcessType.AGENTprocesses for that workspace, and- Stops them sequentially via
processOrchestrator.stop.On success you show the count, and when called with
workspaceIdyou display a truncated id. For consistency, you might also want to show the resolved workspace when using the current workspace path (e.g., render the id/name fromtargetWorkspaceId) so users know which context was affected.Also applies to: 351-361
482-503: Avoid re-importinglaunchAgentinside the per-agent loopInside
startAgentsyou perform a dynamic import of"../lib/launch/run"on every iteration:const agent = process as import("../types/process").Agent; const { launchAgent } = await import("../lib/launch/run"); const result = await launchAgent(agent, { attach: false });Because Node caches modules this won’t be disastrous, but it’s unnecessary work. You can:
- Hoist the dynamic import out of the loop (once per
startAgentscall), or- Prefer a static top-level import if circular dependencies aren’t a concern.
That keeps the loop focused on launching agents and simplifies reasoning about side effects.
430-474:AgentStartignores theonCompletecallback and relies solely on process exit
AgentStartacceptsonCompletebut never calls it; instead it usesexit()from Ink and asetTimeoutto exit the app after showing success. This is fine for direct CLI usage, but if you ever embedAgentStartwithin a higher-level Ink tree or tests that rely ononComplete, they’ll never be notified.If composability is desired, consider invoking
onCompletewhen:
- You transition to
StartStep.COMPLETE, or- Right before calling
exit().You can still keep the auto-exit behavior while giving callers a clean completion signal.
Also applies to: 533-551, 596-621
631-699:AgentAttachcorrectly resolves by id or sessionName, but also dropsonCompleteThe attach flow is solid:
- Tries
ProcessOrchestrator.get(id), then falls back to searching agents bysessionName.- Verifies the process is an agent before launching.
- Uses
launchAgent(agent, { attach: true })with a small delay after exiting Ink, then exits the Node process based on the result.Similar to
AgentStart, theonCompleteprop is accepted but not used. If you plan to embed this component, consider invokingonCompletebefore callingglobalThis.process.exit(...)or after a successful attach/return so callers can hook into completion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (75)
WARP.md(1 hunks)apps/cli/package.json(2 hunks)apps/cli/readme.md(1 hunks)apps/cli/src/cli.tsx(8 hunks)apps/cli/src/commands/agent.tsx(5 hunks)apps/cli/src/commands/dashboard.tsx(1 hunks)apps/cli/src/commands/index.ts(1 hunks)apps/cli/src/commands/init.tsx(1 hunks)apps/cli/src/commands/panels.tsx(1 hunks)apps/cli/src/commands/workspace.tsx(3 hunks)apps/cli/src/components/LaunchOverlay.tsx(1 hunks)apps/cli/src/lib/db.ts(1 hunks)apps/cli/src/lib/launch/config.ts(1 hunks)apps/cli/src/lib/launch/run.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/change-orchestrator.test.ts(8 hunks)apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts(3 hunks)apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts(16 hunks)apps/cli/src/lib/orchestrators/__tests__/workspace-orchestrator.test.ts(7 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(7 hunks)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts(3 hunks)apps/cli/src/lib/storage/__tests__/lowdb-adapter.test.ts(3 hunks)apps/cli/src/lib/storage/adapter.ts(2 hunks)apps/cli/src/lib/storage/lowdb-adapter.ts(5 hunks)apps/cli/src/lib/storage/types.ts(3 hunks)apps/cli/src/types/process.ts(2 hunks)apps/cli/src/types/workspace.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeNavigation.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.tsx(1 hunks)apps/old-desktop/src/main/index.ts(1 hunks)apps/old-desktop/src/main/lib/desktop-stores.ts(1 hunks)apps/old-desktop/src/main/lib/migration/migrator.ts(1 hunks)apps/old-desktop/src/main/lib/storage/index.ts(1 hunks)apps/old-desktop/src/main/lib/storage/lowdb-adapter.ts(1 hunks)apps/old-desktop/src/main/lib/storage/orchestrators/index.ts(1 hunks)apps/old-desktop/src/main/lib/types/cli-types.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/index.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/store.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/types.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/composer.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/index.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/types.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-ipcs.ts(1 hunks)apps/old-desktop/src/renderer/contexts/AppProviders.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/TabContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/TaskContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/WorktreeOperationsContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/MainScreen.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx(2 hunks)apps/old-desktop/src/renderer/screens/main/hooks/useTasks.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/utils.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/ui.ts(1 hunks)apps/website/src/app/components/HeroSection/HeroSection.tsx(1 hunks)apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx(1 hunks)apps/website/src/app/page.tsx(1 hunks)packages/ui/src/components/context-menu.tsx(1 hunks)packages/ui/src/components/dialog.tsx(1 hunks)packages/ui/src/components/dropdown-menu.tsx(1 hunks)packages/ui/src/components/resizable.tsx(1 hunks)packages/ui/src/components/select.tsx(1 hunks)packages/ui/src/components/separator.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
apps/cli/src/lib/db.ts (1)
apps/cli/src/lib/storage/lowdb-adapter.ts (1)
LowdbAdapter(25-198)
apps/cli/src/commands/workspace.tsx (3)
apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/types/workspace.ts (1)
WorkspaceOrchestrator(28-50)
apps/cli/src/lib/launch/config.ts (1)
apps/cli/src/types/process.ts (1)
Agent(41-45)
apps/cli/src/lib/storage/adapter.ts (1)
apps/cli/src/lib/storage/types.ts (1)
Database(15-25)
apps/cli/src/lib/launch/run.ts (2)
apps/cli/src/types/process.ts (1)
Agent(41-45)apps/cli/src/lib/launch/config.ts (1)
getLaunchCommand(19-42)
apps/cli/src/commands/init.tsx (3)
apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/types/workspace.ts (1)
WorkspaceOrchestrator(28-50)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (3)
apps/cli/src/types/process.ts (2)
Agent(41-45)Process(15-29)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/types/workspace.ts (1)
Workspace(6-16)
apps/cli/src/cli.tsx (7)
apps/cli/src/commands/init.tsx (1)
Init(37-397)apps/cli/src/commands/dashboard.tsx (1)
Dashboard(29-555)apps/cli/src/commands/panels.tsx (1)
Panels(28-486)apps/cli/src/commands/workspace.tsx (1)
WorkspaceUse(232-280)apps/cli/src/commands/agent.tsx (3)
AgentStart(430-624)AgentAttach(631-710)AgentStopAll(281-365)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)
apps/cli/src/commands/panels.tsx (4)
apps/cli/src/types/process.ts (3)
Process(15-29)ProcessOrchestrator(47-62)Agent(41-45)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-328)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-129)
apps/cli/src/lib/storage/lowdb-adapter.ts (1)
apps/cli/src/lib/storage/types.ts (3)
Database(15-25)SerializedDatabase(31-41)createEmptyDatabase(64-76)
apps/cli/src/lib/storage/types.ts (1)
apps/cli/src/lib/storage/index.ts (2)
createEmptyDatabase(5-5)SerializedDatabase(4-4)
apps/cli/src/types/workspace.ts (1)
apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)
apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (2)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
needsPersist(103-118)apps/cli/src/types/workspace.ts (2)
Workspace(6-16)LocalWorkspace(18-21)
apps/cli/src/commands/dashboard.tsx (7)
apps/cli/src/types/workspace.ts (2)
Workspace(6-16)WorkspaceOrchestrator(28-50)apps/cli/src/types/process.ts (3)
Process(15-29)Agent(41-45)ProcessOrchestrator(47-62)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-328)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-129)apps/cli/src/components/LaunchOverlay.tsx (1)
LaunchOverlay(13-70)
apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (1)
packages/api/src/index.ts (1)
RouterOutputs(9-9)
apps/cli/src/commands/agent.tsx (6)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (2)
stopAll(275-312)ProcessOrchestrator(50-328)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/types/process.ts (2)
ProcessOrchestrator(47-62)Process(15-29)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-129)
🔇 Additional comments (83)
apps/old-desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx (1)
3-3: Import reordering aligns with project conventions.The hook imports have been reordered to group tab-related hooks before workspace-related hooks, consistent with the broader refactoring pattern across the codebase. No functional changes.
apps/old-desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (1)
9-10: LGTM! Non-functional import reordering.The import statements have been reorganized without any functional impact on the component.
Also applies to: 13-14
apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx (1)
9-17: LGTM! Clean import organization.The import reordering logically groups related imports together (config imports followed by shader imports) without any functional changes.
packages/ui/src/components/dropdown-menu.tsx (1)
3-3: LGTM - Import ordering improvement.The icon imports have been moved to follow the type import, improving consistency with TypeScript conventions. This change has no functional impact.
packages/ui/src/components/dialog.tsx (1)
3-3: LGTM - Consistent import ordering.The icon import now follows the type import, aligning with the standardized import sequence across the UI package.
packages/ui/src/components/select.tsx (1)
4-4: LGTM - Type-only import addition.Adding the explicit type-only import for React is a good practice that can improve tree-shaking and makes the type-only usage explicit.
packages/ui/src/components/resizable.tsx (1)
2-2: LGTM - Import ordering refinement.Moving the icon import after the type import maintains consistency with the standardized import sequence.
packages/ui/src/components/separator.tsx (1)
2-2: LGTM - Correct type-only import.Converting to a type-only import is appropriate since React is only used for type annotations in this component, which can improve bundle size through better tree-shaking.
packages/ui/src/components/context-menu.tsx (1)
3-3: LGTM - Completes import standardization.This import reordering completes the consistent import sequencing pattern across all reviewed UI components. The changes improve code organization without affecting functionality.
apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (1)
6-6: LGTM – Minor stylistic changes with no functional impact.The import reordering and the conversion from
interfacetotypefor the empty props object are both cosmetic changes that don't affect functionality.Also applies to: 14-14
apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
21-21: dragDropManager wiring into<Tree>looks correctUsing the shared
dragDropManagerinstance viadndManager={dragDropManager}is the right pattern for react‑arborist DnD; the added import cleanly resolves that dependency with no other behavioral changes.Also applies to: 1067-1072
apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (1)
10-11: Imports and DnD wiring are consistent with the main WorktreeItemType‑only
NodeApi/TreeApiimports plus a value import forTree, and using the shareddragDropManagerinstance, match the pattern in the primaryWorktreeItemimplementation and should integrate cleanly with react‑arborist and your shared DnD setup.Also applies to: 20-24
apps/website/src/app/components/HeroSection/HeroSection.tsx (1)
1-3: LGTM!Import reordering is a safe, non-functional change that improves code organization.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.tsx (1)
1-8: LGTM!Import reordering maintains consistency without affecting functionality.
apps/old-desktop/src/renderer/contexts/WorktreeOperationsContext.tsx (1)
1-6: LGTM!Import reordering is consistent with the broader refactoring across context files.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeNavigation.tsx (1)
1-4: LGTM!Consolidating the AnimatedBackground import to a single location at the top eliminates duplication and improves maintainability.
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx (1)
1-5: LGTM!Import reordering aligns with the PR's broader organizational improvements.
apps/old-desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
1-5: LGTM!Type import reordering maintains consistency without affecting behavior.
apps/old-desktop/src/renderer/contexts/TabContext.tsx (1)
1-5: LGTM!Placing React imports after type imports follows a common convention and improves consistency.
WARP.md (1)
1-1: Review comment is based on incorrect file content observation.WARP.md actually contains
@AGENTS.md, notCLAUDE.mdas stated in the review. Both WARP.md and CLAUDE.md use the@prefix syntax to reference AGENTS.md (a guide document for the Superset monorepo). This appears to be intentional reference syntax rather than an error, placeholder, or misnamed content. Please re-examine the actual file content and revise your review if necessary.Likely an incorrect or invalid review comment.
apps/old-desktop/src/main/lib/storage/lowdb-adapter.ts (1)
6-6: Config-based collection path wiring is correctImporting and using
getDomainCollectionPathforinitCollectionkeeps collection file locations centralized in config, which is safer than hardcoding paths in the adapter and aligns with the rest of the storage setup.Also applies to: 27-33
apps/old-desktop/src/main/lib/workspace-ipcs.ts (1)
12-15: Window manager import placement is fineMoving
windowManagerinto the main import block and using it for the window‑scoped workspace IPCs keeps the module layout consistent and avoids duplicate imports without changing behavior.Also applies to: 207-231
apps/old-desktop/src/main/lib/ui-store/store.ts (1)
1-10: Atomic write helpers and fs imports are alignedBringing
closeSyncinto thenode:fsnamed imports matches its use inatomicWriteand ensures the temp file descriptor is always closed in thefinallyblock before the atomic rename. The rest of the import reordering is purely cosmetic.Also applies to: 52-68
apps/old-desktop/src/main/lib/ui-store/types.ts (1)
1-1: Type import reorder is harmlessReordering the
MosaicNode/Tabtype import fromshared/typesdoesn’t alter any of the exported UI store types and keeps the dependencies explicit.apps/old-desktop/src/main/lib/ui-store/index.ts (1)
1-2: Re-export ordering is fineExporting
./storebefore./typesis purely organizational and keeps the public surface unchanged for consumers importing from this module.apps/old-desktop/src/renderer/contexts/TaskContext.tsx (1)
5-8: Task context wiring remains correct after import reorderingReordering the type and context imports doesn’t change behavior, and
TaskProviderstill correctly passescurrentWorkspace,setSelectedWorktreeId,handleTabSelect, andhandleWorktreeCreatedWithResultintouseTasks, then exposes that via the context.Also applies to: 40-50
apps/cli/readme.md (1)
1-45: Excellent documentation expansion!The new documentation is comprehensive, well-organized, and provides clear guidance on:
- Quick start and development workflow
- Core concepts (Environment, Workspace, Agent, Change)
- Common command patterns with examples
- tmux integration details
- Storage schema and tips
This significantly improves the developer experience for the CLI.
apps/cli/package.json (3)
5-7: LGTM: Correct binary naming.The change from a string to an object with a named binary is the correct approach for npm package binaries.
36-36: LGTM: Dependency addition aligns with interactive features.The
ink-text-inputdependency supports the new interactive CLI components mentioned in the PR summary.
13-13: I need to gather more information to verify this change. Let me run additional verification scripts and search for documentation on the bun flag that was removed.Removal of
--packages=bundleflag is safe—it specifies default behavior.The
--packagesparameter controls whether package dependencies are included in the bundle, with "bundle" being the default value. Removing the redundant flag has no impact on dependency bundling or CLI portability; the CLI will continue bundling dependencies as expected. No other references to this flag were found in the codebase, and the build output behavior remains unchanged.apps/old-desktop/src/main/lib/storage/orchestrators/index.ts (1)
1-4: LGTM: Export reordering.This is a non-functional reordering of exports with no runtime impact.
apps/old-desktop/src/main/lib/storage/index.ts (1)
1-5: LGTM: Export reordering.This is a non-functional reordering of exports with no runtime impact.
apps/old-desktop/src/main/index.ts (1)
3-3: LGTM: Import reordering.This is a non-functional reordering of imports with no runtime impact.
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts (1)
3-3: LGTM: Import reordering.This is a non-functional reordering of imports with no runtime impact.
apps/cli/src/lib/orchestrators/__tests__/change-orchestrator.test.ts (1)
37-37: LGTM: Test updates align with new API.The tests have been correctly updated to pass workspace creation options as an object
{ path: string }instead of a primitive string, aligning with the WorkspaceOrchestrator.create API evolution that now accepts an options object.Also applies to: 59-59, 71-71, 76-76, 100-100, 127-127, 154-154, 173-173, 212-212
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts (1)
2-2: LGTM: Import reordering.This is a non-functional reordering of imports with no runtime impact.
apps/old-desktop/src/main/lib/workspace-composition/index.ts (1)
1-2: LGTM!The export reordering has no functional impact and the changes are safe.
apps/old-desktop/src/shared/ipc-channels/index.ts (1)
16-16: LGTM!The import reordering has no functional impact.
apps/old-desktop/src/main/lib/workspace-composition/types.ts (1)
3-3: LGTM!The import reordering has no functional impact.
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts (1)
3-3: LGTM!The import reordering has no functional impact.
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx (1)
13-13: LGTM!The import reordering has no functional impact.
apps/old-desktop/src/renderer/contexts/AppProviders.tsx (1)
5-8: LGTM!The import reordering has no functional impact.
apps/old-desktop/src/shared/ipc-channels/ui.ts (1)
5-5: LGTM!Using explicit type-only imports is good practice and has no functional impact.
apps/website/src/app/page.tsx (3)
4-5: LGTM!The new component imports are properly added.
Also applies to: 10-10
13-13: LGTM!The state management for the waitlist modal is correctly initialized.
20-28: LGTM!The component integration and modal state management are correctly implemented. The handlers properly control the modal visibility through state updates.
apps/old-desktop/src/main/lib/workspace-composition/composer.ts (1)
4-5: LGTM! Import organization improved.The addition of the
WorktreeUiMetadatatype import and reordering of imports improves code organization without affecting runtime behavior.apps/cli/src/lib/orchestrators/__tests__/workspace-orchestrator.test.ts (1)
35-37: LGTM! Tests properly updated for new API signature.All test calls to
WorkspaceOrchestrator.createhave been consistently updated to pass an options object{ path: "..." }instead of a direct path string, correctly reflecting the API change mentioned in the PR summary.Also applies to: 58-60, 81-86, 98-103, 114-116, 128-130, 138-140, 155-157
apps/cli/src/lib/db.ts (1)
7-7: LGTM! Cleaner separation of concerns.The refactoring properly delegates database path resolution to the
LowdbAdapterand storage config layer. The updated comment accurately reflects that the path defaults to~/.superset/cli/db.jsonvia the config system.Also applies to: 14-15
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts (1)
5-5: LGTM! Export reordering with no functional impact.The change only reorders the exported types and does not affect functionality or the public API surface.
apps/old-desktop/src/main/lib/migration/migrator.ts (1)
2-4: LGTM! Import organization improved.The import reordering improves code organization without any runtime impact. Grouping related imports together follows best practices.
Also applies to: 10-11
apps/old-desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
6-11: LGTM! Import organization improved.The reordering of context imports aligns with the centralized context module structure without affecting component behavior.
apps/cli/src/lib/storage/__tests__/lowdb-adapter.test.ts (1)
7-7: LGTM! Tests properly updated for new Process interface.The addition of
status: ProcessStatus.RUNNINGto test Process objects correctly reflects the newstatusfield in the Process interface mentioned in the PR summary.Also applies to: 127-127, 147-147
apps/old-desktop/src/renderer/screens/main/MainScreen.tsx (1)
1-1: LGTM! Import additions support new keyboard shortcut functionality.The expanded imports provide the necessary context hooks and utility functions to enable the keyboard shortcut handling implemented in the
useEffecthook (lines 88-134). The changes are additive and properly integrated.Also applies to: 3-12
apps/cli/src/commands/index.ts (1)
1-22: CLI command barrel updates look consistentThe added exports (AgentAttach/AgentStart, Dashboard, Init, Panels, WorkspaceUse) cleanly expand the public CLI surface without affecting existing commands. As long as the underlying modules define these symbols, this barrel looks good.
apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts (1)
2-2: LGTM: Export reordering only.This is a non-functional change that only affects the order of type exports.
apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts (3)
6-6: LGTM: ProcessStatus import added for type safety.Good addition of the ProcessStatus enum import to replace string literals in test assertions.
39-39: LGTM: Workspace creation updated to use options object.The updated API signature
create(environmentId, type, { path })is more extensible than passing the path directly and aligns with the broader API changes in the PR.Also applies to: 60-60, 81-81, 110-110, 131-131, 136-136, 154-154, 175-175, 207-207, 225-225, 247-247, 272-272, 297-297, 313-313
71-71: LGTM: Using ProcessStatus enum instead of string literals.This improves type safety and makes status checks more maintainable.
Also applies to: 236-236, 264-264
apps/cli/src/lib/storage/types.ts (2)
22-24: LGTM: State field added for current workspace tracking.The addition of
state.currentWorkspaceIdenables the app to persist which workspace is currently active, aligning with the workspace selection features introduced in this PR.Also applies to: 38-40
65-69: Default environment initialization is correct—all required fields are present.The
Environmentinterface contains only one required field:id: string. The default environment initialization at lines 65-69 provides this field withid: "default". No additional mandatory fields likecreatedAtorupdatedAtare defined in theEnvironmenttype, so the concern about missing required fields is not applicable here.apps/cli/src/commands/workspace.tsx (3)
140-140: LGTM: Updated to use options object.The change from passing
pathdirectly to{ path }aligns with the new workspace orchestrator API that accepts an options object for more flexibility.
165-171: Good UX: Clear feedback and next steps.The added guidance helps users understand that the workspace is now current and what to do next.
227-280: LGTM: Well-structured WorkspaceUse component.The component properly:
- Handles loading and error states
- Verifies workspace exists before setting it as current
- Updates
lastUsedAttimestamp viaorchestrator.use()- Provides clear user feedback
apps/cli/src/lib/storage/lowdb-adapter.ts (2)
47-58: LGTM: Ensures default database is written on first initialization.The logic to check if the database file exists and write the default structure if it doesn't is a good addition that ensures the database is properly initialized.
13-19: No changes required—the type duplication is intentional and correct.The
CollectionValuetype inadapter.tsis not exported, solowdb-adapter.tscannot import it. Each file correctly maintains its own scope-local definition. This is the proper design:adapter.tsuses the type in its interface definition, andlowdb-adapter.tsuses it independently in its class implementation. No consolidation is needed.apps/cli/src/lib/launch/config.ts (1)
19-42: LGTM: Clear priority hierarchy for launch command resolution.The function properly implements a clear precedence chain: agent-specific command → environment override → user config (TODO) → default.
apps/old-desktop/src/main/lib/types/cli-types.ts (3)
6-11: LGTM: FileDiff added to change exports.Good addition of
FileDiffto the public exports for change-related types.
20-25: LGTM: Expanded process-related exports.The addition of
Process,ProcessType, andTerminalto the type exports, along with the named export ofProcessType, properly exposes these types for desktop app usage.
27-31: LGTM: Workspace exports consolidated.Good organization to have all workspace-related exports in a dedicated block.
apps/old-desktop/src/renderer/contexts/index.ts (2)
3-6: LGTM: Additional context providers exported.Good consolidation of context exports in the barrel file, making these providers and hooks easier to import from a single location.
9-9: LGTM: WorktreeOperationsProvider added to exports.Good catch adding the provider alongside the hook for consistency with other context exports.
apps/cli/src/cli.tsx (5)
42-65: Newinit,dashboard, andpanelscommands are wired cleanlyThe new commands correctly render the corresponding Ink components and keep the CLI surface coherent. The split between non-interactive (env/workspace/agent/change) and interactive (init/dashboard/panels) flows is clear and consistent.
103-172:workspace usecommand integrates well with the orchestratorThe
workspace use <id>command cleanly delegates to theWorkspaceUseInk component and exits the process on completion, aligning with the new “current workspace” concept in the orchestrator.
201-218: Agentstartandattachcommands line up with the new lifecycle flowsThe
startcommand’s optionalworkspaceIdargument and theattachcommand’sid/session name argument match the behavior described forAgentStartandAgentAttach, and error/exit handling is delegated to those components appropriately.
271-284:stop-alldescription and option now match AgentStopAll’s semanticsUpdating the description and adding the
--workspaceoption to passworkspaceIdintoAgentStopAllcorrectly reflects the behavior of stopping agents scoped to a workspace or the current workspace when omitted.
335-381: Default root action provides a helpful, workspace-aware entrypointThe new root
program.actionthat shows a welcome message, current workspace (including path/branch), and a concise “getting started” command list is a solid UX improvement. Dynamic imports keep startup cheap when subcommands are used.apps/cli/src/types/workspace.ts (1)
6-16: Workspace metadata and CloudWorkspace typing look consistentAdding
name,description, timestamps,lastUsedAt, anddefaultAgents, plus introducingCloudWorkspacewith abranchfield, lines up with howWorkspaceOrchestratorcreates/backfills workspaces. This gives the CLI enough structure to drive the new interactive flows.Also applies to: 23-26
apps/cli/src/lib/orchestrators/process-orchestrator.ts (3)
19-45: tmux presence/session helpers are reasonable for a CLI contextUsing
isTmuxInstalled+tmuxSessionExistsbased onexecSynckeeps status checks simple and synchronous, which is acceptable for a terminal CLI tool, especially since errors are safely caught and mapped tofalse.
53-101: Backfill + tmux-based status sync inget/listis well-structuredThe flow of
backfillDefaults→syncAgentStatus→ conditionalstorage.setin bothgetandlistcleanly keeps persisted processes in sync with reality (status, timestamps, launchCommand, sessionName) without over-writing on every read.
246-312:stopandstopAllhandle tmux cleanup and status transitions appropriatelyBoth methods now kill tmux sessions when present, mark agents as STOPPED with
endedAt/updatedAt, andstopAllreturns a usefulstoppedCount. Errors fromtmux kill-sessionare swallowed safely so stale sessions don’t break the orchestrator.apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
17-49: Default backfilling and listing behavior matches the extended Workspace shapeUsing
backfillDefaultsinget/listto ensurecreatedAt,updatedAt, anddefaultAgentsare always populated (and persisting only fromgetwhen needed) nicely smooths migration from older DBs without excessive writes.apps/cli/src/types/process.ts (1)
8-13: Process/Agent type extensions look consistent and usefulAdding
ProcessStatus,pid,lastHeartbeat,launchCommand, and discriminatedtypeplussessionNameonAgentlines up well with the orchestrator logic and new CLI features. The shapes are clear and should make downstream status-driven logic and tmux integration straightforward.Also applies to: 19-19, 26-29, 31-45
| // Categorize agents by status | ||
| const runningAgents = filteredAgents.filter( | ||
| (a) => a.status === ProcessStatus.RUNNING || !a.endedAt, | ||
| ); | ||
| const idleAgents = filteredAgents.filter( | ||
| (a) => a.status === ProcessStatus.IDLE && !a.endedAt, | ||
| ); | ||
| const stoppedAgents = filteredAgents.filter((a) => a.endedAt); | ||
| const errorAgents = filteredAgents.filter( | ||
| (a) => a.status === ProcessStatus.ERROR, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Running/idle agent counts treat all non-ended agents as “running”
Both the global summary and per-workspace counts use predicates like:
const runningAgents = filteredAgents.filter(
(a) => a.status === ProcessStatus.RUNNING || !a.endedAt,
);and
const wsRunning = wsAgents.filter(
(a) => a.status === ProcessStatus.RUNNING || !a.endedAt,
);This classifies all agents without endedAt (including IDLE agents) as running, so idle agents are double-counted as both running and idle, and workspace “N running” labels are misleading.
Filter running agents strictly by status (optionally also checking !endedAt) and keep idle separate. For example:
- const runningAgents = filteredAgents.filter(
- (a) => a.status === ProcessStatus.RUNNING || !a.endedAt,
- );
- const idleAgents = filteredAgents.filter(
- (a) => a.status === ProcessStatus.IDLE && !a.endedAt,
- );
+ const runningAgents = filteredAgents.filter(
+ (a) => a.status === ProcessStatus.RUNNING && !a.endedAt,
+ );
+ const idleAgents = filteredAgents.filter(
+ (a) => a.status === ProcessStatus.IDLE && !a.endedAt,
+ );
@@
- const wsRunning = wsAgents.filter(
- (a) => a.status === ProcessStatus.RUNNING || !a.endedAt,
- );
+ const wsRunning = wsAgents.filter(
+ (a) => a.status === ProcessStatus.RUNNING && !a.endedAt,
+ );This will make the summary and per-workspace badges accurately reflect actual RUNNING vs IDLE states.
Also applies to: 409-445
| {workspaces.map((ws, index) => { | ||
| const isSelected = index === selectedWorkspaceIndex; | ||
| const isCurrent = ws.id === currentWorkspaceId; | ||
| const wsAgents = agents.filter((a) => a.workspaceId === ws.id); | ||
| const wsRunning = wsAgents.filter( | ||
| (a) => a.status === ProcessStatus.RUNNING, | ||
| ); | ||
|
|
||
| return ( | ||
| <Box key={ws.id} flexDirection="row" gap={1}> | ||
| <Text | ||
| bold={isSelected} | ||
| color={isSelected ? "cyan" : isCurrent ? "white" : "gray"} | ||
| > | ||
| {isSelected ? "▸" : " "} {wsRunning.length > 0 ? "●" : "○"}{" "} | ||
| {ws.name || ws.id.slice(0, 8)} | ||
| </Text> | ||
| </Box> | ||
| ); | ||
| })} | ||
| <Box marginTop={1}> | ||
| <Text dimColor>[W] New</Text> | ||
| </Box> |
There was a problem hiding this comment.
UI hints for [W] New and [N] New Agent don’t map to any key handling
The footer and panel bodies advertise:
[W] Newin the workspaces panel, and[N] New Agentplus[Enter] Openin the agents panel,
but useInput only handles 1/2/3, arrows/j/k, r, q, and Enter for opening. Pressing w or n currently has no effect.
Either wire up handlers for w/n to trigger the intended actions (e.g., init workspace or start-agent flow), or hide these hints until those actions are implemented to avoid confusing users.
Also applies to: 307-355, 460-482
🤖 Prompt for AI Agents
In apps/cli/src/commands/panels.tsx around lines 267-289 (and also apply same
fix to sections 307-355 and 460-482), the UI shows "[W] New" and "[N] New Agent"
hints but no input handlers for 'w'/'n' exist; update the useInput handler to
listen for both lowercase and uppercase 'w' and 'n' (and their corresponding key
names if using key.sequence/key.name), and call the existing new-workspace and
new-agent kickoff functions (or the functions that open those flows/modals) so
the hints are actionable; if those kickoff functions don’t exist yet, either
implement simple wrappers that set the same state used by the current create
flows or remove the hint text until the handlers are implemented.
| async use(id: string): Promise<void> { | ||
| // Verify workspace exists | ||
| await this.get(id); | ||
|
|
||
| // Update lastUsedAt | ||
| const workspace = await this.storage.get("workspaces", id); | ||
| if (workspace) { | ||
| workspace.lastUsedAt = new Date(); | ||
| workspace.updatedAt = new Date(); | ||
| await this.storage.set("workspaces", id, workspace); | ||
| } | ||
|
|
||
| // Set as current workspace | ||
| const db = await this.storage.read(); | ||
| db.state.currentWorkspaceId = id; | ||
| await this.storage.write(db); | ||
| } |
There was a problem hiding this comment.
use should defensively initialize db.state before writing currentWorkspaceId
use() reads const db = await this.storage.read(); and then writes db.state.currentWorkspaceId = id; without ensuring db.state exists. On a fresh or legacy DB with no state object, this will throw at runtime. This can happen on first workspace creation, since create() calls use() immediately.
A minimal fix mirroring getCurrent()’s defensive logic:
async use(id: string): Promise<void> {
@@
- // Set as current workspace
- const db = await this.storage.read();
- db.state.currentWorkspaceId = id;
- await this.storage.write(db);
+ // Set as current workspace
+ const db = await this.storage.read();
+ if (!db.state) {
+ db.state = {};
+ }
+ db.state.currentWorkspaceId = id;
+ await this.storage.write(db);
}📝 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.
| async use(id: string): Promise<void> { | |
| // Verify workspace exists | |
| await this.get(id); | |
| // Update lastUsedAt | |
| const workspace = await this.storage.get("workspaces", id); | |
| if (workspace) { | |
| workspace.lastUsedAt = new Date(); | |
| workspace.updatedAt = new Date(); | |
| await this.storage.set("workspaces", id, workspace); | |
| } | |
| // Set as current workspace | |
| const db = await this.storage.read(); | |
| db.state.currentWorkspaceId = id; | |
| await this.storage.write(db); | |
| } | |
| async use(id: string): Promise<void> { | |
| // Verify workspace exists | |
| await this.get(id); | |
| // Update lastUsedAt | |
| const workspace = await this.storage.get("workspaces", id); | |
| if (workspace) { | |
| workspace.lastUsedAt = new Date(); | |
| workspace.updatedAt = new Date(); | |
| await this.storage.set("workspaces", id, workspace); | |
| } | |
| // Set as current workspace | |
| const db = await this.storage.read(); | |
| if (!db.state) { | |
| db.state = {}; | |
| } | |
| db.state.currentWorkspaceId = id; | |
| await this.storage.write(db); | |
| } |
🤖 Prompt for AI Agents
In apps/cli/src/lib/orchestrators/workspace-orchestrator.ts around lines 150 to
166, the method sets db.state.currentWorkspaceId without ensuring db.state
exists; update use() to defensively initialize db.state (e.g., if (!db.state)
db.state = {}) before assigning currentWorkspaceId, then set
db.state.currentWorkspaceId = id and write the db back as before so writes never
throw on fresh/legacy DBs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/cli/src/commands/panels.tsx (1)
79-177:[W] New/[N] New Agenthints still have no key handlersThe left and middle panels render:
[W] Newunder workspaces, and[N] New Agentplus[Enter] Openunder agents,but
useInputonly handles1/2/3, arrows/j/k,r,q/Escape/Ctrl+C, and Enter for opening an agent. Pressingwornis still a no-op.Either:
- Add handlers for
input === "w"/"W"and"n"/"N"that kick off the corresponding flows (e.g., workspace init / agent start), or- Remove or hide these hints until those flows are implemented, to avoid misleading users.
This will keep the UI hints aligned with actual keybindings.
Also applies to: 263-295, 355-360, 465-487
apps/cli/src/commands/dashboard.tsx (1)
244-255: Running/idle counts still treat all non-ended agents as “running”
runningAgentsandwsRunningfilter witha.status === ProcessStatus.RUNNING || !a.endedAt, so IDLE agents (and any non-ended non-RUNNING states) are counted as running and can be double-counted in the summary.You can keep running/idle disjoint and more accurate by filtering strictly on
status(optionally still requiring!endedAt):- const runningAgents = filteredAgents.filter( - (a) => a.status === ProcessStatus.RUNNING || !a.endedAt, - ); - const idleAgents = filteredAgents.filter( - (a) => a.status === ProcessStatus.IDLE && !a.endedAt, - ); + const runningAgents = filteredAgents.filter( + (a) => a.status === ProcessStatus.RUNNING && !a.endedAt, + ); + const idleAgents = filteredAgents.filter( + (a) => a.status === ProcessStatus.IDLE && !a.endedAt, + ); @@ - const wsRunning = wsAgents.filter( - (a) => a.status === ProcessStatus.RUNNING || !a.endedAt, - ); + const wsRunning = wsAgents.filter( + (a) => a.status === ProcessStatus.RUNNING && !a.endedAt, + );This keeps “running” counts aligned with true
RUNNINGstate and avoids double-counting idle agents.Also applies to: 392-427
apps/cli/src/commands/agent.tsx (1)
476-523:AgentStartreports success even when no agents actually startedIn
startAgents, only successfullaunchAgentcalls push intocreated, but after the loop you always do:setStartedAgents(created); setStep(StartStep.COMPLETE);and the COMPLETE view renders:
<Text color="green"> ✓ Started {startedAgents.length} agent(s) successfully! </Text>If all
launchAgentcalls fail, this shows a green “✓ Started 0 agent(s) successfully!” message, which is misleading.Consider:
- If
created.length === 0, set an error state (or a dedicated failure step) and avoid the green success UI.- If
created.length < agents.length, surface partial success, e.g. “Started N agent(s); M failed — see logs above”.- Only transition to
StartStep.COMPLETEand show the green success message when at least one agent started.That will give users an accurate signal about whether anything is actually running.
Also applies to: 604-620
🧹 Nitpick comments (1)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
53-101: Synchronous tmux checks on everyget/listcan be heavy; tighten typing for agent-specific defaults
get/listnow callbackfillDefaults+syncAgentStatusfor every process, andsyncAgentStatusinvokestmuxSessionExists, which in turn runswhich tmux+tmux has-sessionsynchronously. With the dashboard/panels polling every 3 seconds, a larger number of agents will translate into frequent blocking subprocesses on the main CLI thread.Also,
needsPersistandbackfillDefaultsaccess agent-specific fields (agentType,sessionName,launchCommand) on a parameter typed asProcess, which doesn’t declare those properties, so this is fragile from a TypeScript perspective.Consider:
- Caching
isTmuxInstalled()at module level and optionally gatingsyncAgentStatusbehind a feature flag or a “lazy sync” path, so that simple list/get calls don’t always spawn tmux processes.- Narrowing
processexplicitly where you use agent-only fields, e.g.:if (process.type === ProcessType.AGENT) { const agent = process as Agent; // use agent.agentType, agent.sessionName, etc }or by changing the helper signatures to accept
Agentwhere appropriate, to keep the type surface honest and avoid surprises when evolving these types.Also applies to: 103-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/cli/src/commands/agent.tsx(5 hunks)apps/cli/src/commands/dashboard.tsx(1 hunks)apps/cli/src/commands/panels.tsx(1 hunks)apps/cli/src/lib/launch/run.ts(1 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/cli/src/lib/launch/run.ts (2)
apps/cli/src/types/process.ts (1)
Agent(41-45)apps/cli/src/lib/launch/config.ts (1)
getLaunchCommand(19-42)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (3)
apps/cli/src/types/process.ts (2)
Agent(41-45)Process(15-29)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/types/workspace.ts (1)
Workspace(6-16)
apps/cli/src/commands/dashboard.tsx (6)
apps/cli/src/types/workspace.ts (2)
Workspace(6-16)WorkspaceOrchestrator(28-50)apps/cli/src/types/process.ts (3)
Process(15-29)ProcessOrchestrator(47-62)Agent(41-45)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-325)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
apps/cli/src/commands/agent.tsx (6)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (2)
stopAll(272-309)ProcessOrchestrator(50-325)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/types/process.ts (2)
ProcessOrchestrator(47-62)Process(15-29)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
apps/cli/src/commands/panels.tsx (4)
apps/cli/src/types/process.ts (3)
Process(15-29)ProcessOrchestrator(47-62)Agent(41-45)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-325)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
🪛 GitHub Actions: CI
apps/cli/src/commands/agent.tsx
[error] 434-434: lint/suspicious/noExplicitAny FIXABLE. Unexpected any. Specify a different type.
[error] 476-476: lint/suspicious/noExplicitAny FIXABLE. Unexpected any. Specify a different type.
| export function AgentStart({ workspaceId, onComplete }: AgentStartProps) { | ||
| const { exit } = useApp(); | ||
| const [step, setStep] = React.useState(StartStep.LOADING); | ||
| const [error, setError] = React.useState<string | null>(null); | ||
| const [workspace, setWorkspace] = React.useState<any>(null); | ||
| const [selectedAgents, setSelectedAgents] = React.useState<AgentType[]>([]); | ||
| const [startedAgents, setStartedAgents] = React.useState<Process[]>([]); | ||
|
|
There was a problem hiding this comment.
Replace any with concrete workspace types to satisfy lint and improve safety
The workspace state and startAgents parameter are both typed as any, which is tripping noExplicitAny in CI and weakens type safety around workspace handling:
const [workspace, setWorkspace] = React.useState<any>(null);
…
const startAgents = async (ws: any, agents: AgentType[]) => {You can fix the lint errors and get better checking by using the Workspace type:
-import { getDefaultLaunchCommand } from "../lib/launch/config";
+import { getDefaultLaunchCommand } from "../lib/launch/config";
+import type { Workspace } from "../types/workspace";
@@
- const [workspace, setWorkspace] = React.useState<any>(null);
+ const [workspace, setWorkspace] = React.useState<Workspace | null>(null);
@@
- const startAgents = async (ws: any, agents: AgentType[]) => {
+ const startAgents = async (ws: Workspace, agents: AgentType[]) => {This should clear the noExplicitAny failures and make it harder to accidentally call startAgents with an incompatible object.
Also applies to: 476-531
🧰 Tools
🪛 GitHub Actions: CI
[error] 434-434: lint/suspicious/noExplicitAny FIXABLE. Unexpected any. Specify a different type.
🤖 Prompt for AI Agents
In apps/cli/src/commands/agent.tsx around lines 430-437 (and also apply the same
change to lines 476-531), the workspace state and startAgents parameter use the
explicit any type which triggers noExplicitAny and reduces type safety; change
the React state declaration to use the concrete Workspace type (e.g.
React.useState<Workspace | null>(null)) and update any function signatures like
startAgents to accept ws: Workspace (or Workspace | null if appropriate), import
the Workspace type from its module if not already imported, and update any
downstream usages/assignments to satisfy the new type (narrow checks for null
where needed) so lint passes and typing is stricter.
| import { exec, execSync, spawn } from "node:child_process"; | ||
| import { promisify } from "node:util"; | ||
| import type { Agent } from "../../types/process"; | ||
| import { getLaunchCommand } from "./config"; | ||
|
|
||
| const execAsync = promisify(exec); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Avoid interpolating command into a shell string; use spawn to prevent injection and quoting issues
launchAgent currently creates tmux sessions via:
await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`);Since command can come from SUPERSET_AGENT_LAUNCH_*, any embedded quotes or shell metacharacters (;, &&, etc.) will be interpreted by the shell, which is both fragile (easy to break) and a potential injection vector.
A safer approach is to avoid the shell altogether and use spawn with arguments:
-import { exec, execSync, spawn } from "node:child_process";
-import { promisify } from "node:util";
+import { execSync, spawn } from "node:child_process";
@@
-const execAsync = promisify(exec);
@@
- // Session doesn't exist - create it in detached mode
- try {
- await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`);
+ // Session doesn't exist - create it in detached mode
+ try {
+ await new Promise<void>((resolve, reject) => {
+ const child = spawn("tmux", ["new-session", "-d", "-s", sessionName, command], {
+ stdio: "ignore",
+ detached: false,
+ });
+
+ child.on("error", (error) => reject(error));
+ child.on("exit", (code) => {
+ if (code === 0) {
+ resolve();
+ } else {
+ reject(new Error(`tmux new-session exited with code ${code ?? "null"}`));
+ }
+ });
+ });This way command is passed as a single argument to tmux (no local shell parsing), making env-configured launch commands much more robust and removing the shell-injection vector.
Also applies to: 89-137
I'll verify the shell injection concern by examining the actual codebase.
🏁 Script executed:
#!/bin/bash
# Find and display the launchAgent function around lines 89-137
cat apps/cli/src/lib/launch/run.ts | head -150 | tail -70Length of output: 2112
🏁 Script executed:
#!/bin/bash
# Get full content of the run.ts file to understand context
wc -l apps/cli/src/lib/launch/run.tsLength of output: 34
🏁 Script executed:
#!/bin/bash
# Search for launchAgent function definition
rg -n "launchAgent" apps/cli/src/lib/launch/run.tsLength of output: 165
🏁 Script executed:
#!/bin/bash
# Search for SUPERSET_AGENT_LAUNCH to understand command source
rg -n "SUPERSET_AGENT_LAUNCH" apps/cli/src/Length of output: 536
🏁 Script executed:
#!/bin/bash
# Search for execAsync usage in the file
rg -n "execAsync" apps/cli/src/lib/launch/run.ts -A2 -B2Length of output: 366
🏁 Script executed:
#!/bin/bash
# Get full launchAgent function (lines 47 onwards)
sed -n '47,137p' apps/cli/src/lib/launch/run.tsLength of output: 2860
🏁 Script executed:
#!/bin/bash
# Get the getLaunchCommand function from config.ts
cat apps/cli/src/lib/launch/config.tsLength of output: 1613
🏁 Script executed:
#!/bin/bash
# Check if there are any other exec/execSync/spawn calls in run.ts
rg -n "exec|spawn" apps/cli/src/lib/launch/run.tsLength of output: 416
🏁 Script executed:
#!/bin/bash
# Get attachToAgent function and surrounding context (lines 175-210)
sed -n '140,210p' apps/cli/src/lib/launch/run.tsLength of output: 2125
🏁 Script executed:
#!/bin/bash
# Get context around lines 181, 184, 187 execSync calls
sed -n '175,200p' apps/cli/src/lib/launch/run.tsLength of output: 741
Fix shell injection vulnerability at line 91 by using spawn() instead of execAsync()
The vulnerability is confirmed. Line 91 interpolates the command variable (sourced from environment variable SUPERSET_AGENT_LAUNCH_*) directly into a shell string:
await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`);This allows shell metacharacters (;, &&, |, etc.) to be interpreted, creating both a fragility and injection vector.
The codebase already uses the safe spawn() pattern elsewhere (line 195 in attachToAgent), so the proposed fix using spawn() with an argument array is the correct approach:
-import { exec, execSync, spawn } from "node:child_process";
-import { promisify } from "node:util";
+import { execSync, spawn } from "node:child_process";
@@
-const execAsync = promisify(exec);
@@
- // Session doesn't exist - create it in detached mode
- try {
- await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`);
+ // Session doesn't exist - create it in detached mode
+ try {
+ await new Promise<void>((resolve, reject) => {
+ const child = spawn("tmux", ["new-session", "-d", "-s", sessionName, command], {
+ stdio: "ignore",
+ detached: false,
+ });
+
+ child.on("error", (error) => reject(error));
+ child.on("exit", (code) => {
+ if (code === 0) {
+ resolve();
+ } else {
+ reject(new Error(`tmux new-session exited with code ${code ?? "null"}`));
+ }
+ });
+ });🤖 Prompt for AI Agents
In apps/cli/src/lib/launch/run.ts (context shown lines 1-7; fix applies at line
~91), the code uses execAsync with a shell-interpolated string which allows
shell injection; replace the execAsync call await execAsync(`tmux new-session -d
-s "${sessionName}" "${command}"`) with a spawn-based invocation that does not
use a shell: call spawn('tmux', ['new-session', '-d', '-s', sessionName, '--',
...args], { stdio: 'inherit' }) where args is a safe array of the command and
its arguments (if getLaunchCommand returns a single string, split it into args
using a proper tokenizer or adapt getLaunchCommand to return an array), await
the child process completion (wrap in a Promise that resolves/rejects on
'exit'/'error') and do not set shell: true; this removes shell interpolation and
prevents injection.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/cli/src/commands/agent.tsx (2)
430-476: Replaceanyworkspace typing with concreteWorkspaceto fix lint and improve safetyLine 434 and the
startAgentssignature still useany:const [workspace, setWorkspace] = React.useState<any>(null); // Line 434 … const startAgents = async (ws: any, agents: AgentType[]) => { // Line 476This is exactly what CI is flagging (
lint/suspicious/noExplicitAny) and it loses type-safety even though you already have aWorkspacetype available via the orchestrators.A minimal fix:
- Import the
Workspacetype (adjust path to wherever it’s defined, e.g.../types/workspace).- Update the state and helper signature:
-import React from "react"; +import React from "react"; +import type { Workspace } from "../types/workspace"; // adjust path if needed -const [workspace, setWorkspace] = React.useState<any>(null); +const [workspace, setWorkspace] = React.useState<Workspace | null>(null); -const startAgents = async (ws: any, agents: AgentType[]) => { +const startAgents = async (ws: Workspace, agents: AgentType[]) => {
- Add a small null guard when invoking
startAgentsfromhandleAgentSelect:- if (item.value === "done") { - if (selectedAgents.length === 0) { - setError("Please select at least one agent to start"); - return; - } - setStep(StartStep.STARTING); - startAgents(workspace, selectedAgents); + if (item.value === "done") { + if (selectedAgents.length === 0) { + setError("Please select at least one agent to start"); + return; + } + if (!workspace) { + setError("Workspace not loaded. Please try again."); + return; + } + setStep(StartStep.STARTING); + startAgents(workspace, selectedAgents);This should resolve the lint error and make the flow safer by guaranteeing a real
Workspaceis passed intostartAgents. (Based on relevant orchestrator snippets.)Also applies to: 533-541
476-528: Don’t report a green “success” when zero agents actually startThe
startAgentshelper always transitions toStartStep.COMPLETEand the UI always shows:<Text color="green"> ✓ Started {startedAgents.length} agent(s) successfully! </Text>even when
created.length === 0(alllaunchAgentcalls failed), because only successful ones are pushed intocreatedbut you don’t branch on that value. Users will see a green “Started 0 agent(s) successfully!” and may assume everything is fine.Consider at least handling the “no agents started” case explicitly before setting COMPLETE:
- // Update workspace lastUsedAt and set as current if not already - await workspaceOrchestrator.use(ws.id); - - setStartedAgents(created); - setStep(StartStep.COMPLETE); + // Update workspace lastUsedAt and set as current if not already + await workspaceOrchestrator.use(ws.id); + + // If no agents actually started, surface a clear error instead of success + if (created.length === 0) { + setError( + "Failed to start any agents. Check the error output above for details.", + ); + return; + } + + setStartedAgents(created); + setStep(StartStep.COMPLETE);You could go further and distinguish partial success (e.g., “Started N agent(s); M failed — see logs above”), but even just avoiding the “0 agents successfully” green state would make the UX much less misleading. (Based on earlier review feedback.)
Also applies to: 596-620
🧹 Nitpick comments (4)
apps/cli/src/commands/panels.tsx (2)
104-131: Guard navigation when lists are empty to avoid negative indices.The wrap‑around logic uses
length - 1on bothdata.workspacesandfilteredAgents. When a list is empty, pressing ↑ in that panel will set the corresponding selected index to-1. The rest of the code happens to tolerate this, but it’s a brittle invariant and can lead to confusing selection state.Consider explicitly guarding on list emptiness before changing indices, e.g.:
- const agents = data.processes.filter((p) => p.type === ProcessType.AGENT); - const selectedWorkspace = data.workspaces[selectedWorkspaceIndex]; - const filteredAgents = selectedWorkspace + const agents = data.processes.filter((p) => p.type === ProcessType.AGENT); + const selectedWorkspace = data.workspaces[selectedWorkspaceIndex]; + const filteredAgents = selectedWorkspace ? agents.filter((a) => a.workspaceId === selectedWorkspace.id) : agents; + const hasWorkspaces = data.workspaces.length > 0; + const hasAgents = filteredAgents.length > 0; @@ - if (key.upArrow || input === "k") { - if (activePanel === "workspaces") { + if (key.upArrow || input === "k") { + if (activePanel === "workspaces" && hasWorkspaces) { setSelectedWorkspaceIndex((prev) => prev > 0 ? prev - 1 : data.workspaces.length - 1, ); - } else if (activePanel === "agents") { + } else if (activePanel === "agents" && hasAgents) { setSelectedAgentIndex((prev) => prev > 0 ? prev - 1 : filteredAgents.length - 1, ); } - } else if (key.downArrow || input === "j") { - if (activePanel === "workspaces") { + } else if (key.downArrow || input === "j") { + if (activePanel === "workspaces" && hasWorkspaces) { setSelectedWorkspaceIndex((prev) => prev < data.workspaces.length - 1 ? prev + 1 : 0, ); - } else if (activePanel === "agents") { + } else if (activePanel === "agents" && hasAgents) { setSelectedAgentIndex((prev) => prev < filteredAgents.length - 1 ? prev + 1 : 0, ); }
22-24:onCompleteprop is unused; either wire it into exit paths or drop it.
PanelsPropsexposesonComplete, but the component only destructures it as_onCompleteand never calls it. That makes the public API misleading and harder to integrate with higher‑level CLI flows.Consider either:
- Invoking
onCompletein success/exit paths instead of (or in addition to)process.exit, or- Removing the prop from this component until it’s needed.
Also applies to: 31-37
apps/cli/src/commands/agent.tsx (2)
276-361: Workspace-scoped stop logic is solid; consider minor UX cleanupThe new
AgentStopAllflow correctly:
- Resolves a target workspace (explicit
workspaceIdorgetCurrent()fallback),- Filters to running
ProcessType.AGENTprocesses for that workspace, and- Stops each via
processOrchestrator.stop, with a clear “no running agents” error whencount === 0.Two small polish points you might consider:
Success workspace label
In the success view (Lines 353-360), you only show the workspace whenworkspaceIdprop is provided, but not when the current workspace is auto-detected. If you want consistent feedback, you could tracktargetWorkspaceIdin state and always render the workspace identifier from that, regardless of whether it came from props orgetCurrent().Redundant
setLoading(false)
In thecount === 0branch you callsetLoading(false)before returning, and it will run again infinally. It’s harmless but can be simplified by relying on thefinallyblock.These are optional; the current implementation is functionally correct.
482-504: Hoist dynamiclaunchAgentimport outside the loop for clarity and tiny perf winInside
startAgentsyou dynamically importlaunchAgenton every iteration:for (const agentType of agents) { … const agent = process as import("../types/process").Agent; const { launchAgent } = await import("../lib/launch/run"); const result = await launchAgent(agent, { attach: false }); … }Since the module is cached after the first import, this works, but it’s slightly noisier and does redundant awaits. You can hoist the import once before the loop:
- const created: Process[] = []; - for (const agentType of agents) { + const { launchAgent } = await import("../lib/launch/run"); + + const created: Process[] = []; + for (const agentType of agents) { // Get default launch command for this agent type const launchCommand = getDefaultLaunchCommand(agentType); … - const agent = process as import("../types/process").Agent; - const { launchAgent } = await import("../lib/launch/run"); - const result = await launchAgent(agent, { attach: false }); + const agent = process as import("../types/process").Agent; + const result = await launchAgent(agent, { attach: false });Purely an optional cleanup; behavior stays the same.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cli/src/commands/agent.tsx(5 hunks)apps/cli/src/commands/dashboard.tsx(1 hunks)apps/cli/src/commands/panels.tsx(1 hunks)apps/cli/src/lib/launch/run.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cli/src/lib/launch/run.ts
- apps/cli/src/commands/dashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cli/src/commands/panels.tsx (4)
apps/cli/src/types/process.ts (3)
Process(15-29)ProcessOrchestrator(47-62)Agent(41-45)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-325)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
apps/cli/src/commands/agent.tsx (6)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (2)
stopAll(272-309)ProcessOrchestrator(50-325)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/types/process.ts (2)
ProcessOrchestrator(47-62)Process(15-29)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
🪛 GitHub Actions: CI
apps/cli/src/commands/agent.tsx
[error] 434-434: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
🔇 Additional comments (1)
apps/cli/src/commands/agent.tsx (1)
626-711: AgentAttach resolution and failure handling look robustThe attach flow is well thought through:
- Tries
get(id)first, then falls back to finding bysessionNameamong AGENT processes.- Validates the process type before attaching.
- Exits Ink before invoking
launchAgentso tmux can own stdin.- On failure, it best-effort updates the agent status to
STOPPEDwithendedAt, and surfaces clear console errors plus a non-zero exit code.No issues from my side here; this segment looks solid.
| useInput((input, key) => { | ||
| if (!data) return; | ||
|
|
||
| if (key.escape || input === "q" || (key.ctrl && input === "c")) { | ||
| exit(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Exit keys are ignored while data is null (loading/error state).
Because of if (!data) return;, Esc, q, and Ctrl+C do nothing until data is populated. If loadData fails and only error is set, the user may not be able to exit via these keys at all.
Move the exit-handling branch before the !data guard so quitting always works:
- useInput((input, key) => {
- if (!data) return;
-
- if (key.escape || input === "q" || (key.ctrl && input === "c")) {
- exit();
- return;
- }
+ useInput((input, key) => {
+ if (key.escape || input === "q" || (key.ctrl && input === "c")) {
+ exit();
+ return;
+ }
+
+ if (!data) return;🤖 Prompt for AI Agents
In apps/cli/src/commands/panels.tsx around lines 82 to 89, the current useInput
handler returns early when data is falsy, which prevents exit keys from working
during loading or error states; move the branch that checks for exit keys
(Escape, "q", Ctrl+C) to the top of the handler so those keys call exit()
immediately regardless of data, then keep the existing guard (if !data return)
after that to skip other input handling when data is not available.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/cli/src/commands/agent.tsx (1)
430-440: Fix the explicitanytypes to resolve pipeline failures and improve type safety.The
workspacestate (line 434) andwsvariable (line 447) use explicit or implicitanytypes, which triggernoExplicitAnyandnoImplicitAnyLetlint errors and weaken type safety.Import the
Workspacetype and apply these fixes:+import type { Workspace } from "../types/workspace"; export function AgentStart({ workspaceId, onComplete }: AgentStartProps) { const { exit } = useApp(); const [step, setStep] = React.useState(StartStep.LOADING); const [error, setError] = React.useState<string | null>(null); - const [workspace, setWorkspace] = React.useState<any>(null); + const [workspace, setWorkspace] = React.useState<Workspace | null>(null); const [selectedAgents, setSelectedAgents] = React.useState<AgentType[]>([]); const [startedAgents, setStartedAgents] = React.useState<Process[]>([]);const loadWorkspace = async () => { try { const db = getDb(); const workspaceOrchestrator = new WorkspaceOrchestrator(db); - let ws; + let ws: Workspace; if (workspaceId) { ws = await workspaceOrchestrator.get(workspaceId); } else {Also applies to: 447-447, 479-479
🧹 Nitpick comments (2)
apps/cli/src/commands/agent.tsx (2)
644-647: Refactor the type assertion for better type safety.Line 646 uses
(a as any).agentTypewhich bypasses type checking. SincestartedAgentsis typed asProcess[]and agents have theagentTypefield, cast to theAgenttype instead.+import type { Agent } from "../types/process"; {successCount > 0 && ( <Text dimColor color="green"> - Success: {startedAgents.map((a) => (a as any).agentType).join(", ")} + Success: {startedAgents.map((a) => (a as Agent).agentType).join(", ")} </Text> )}
738-741: Consider reusing the outer scope imports instead of re-importing.Lines 738-741 dynamically re-import
getDbandProcessOrchestratorinside the error handler, but these modules are already imported and available at the outer scope (lines 5, 7).Reuse the existing imports to simplify the error handler:
if (!result.success) { // Update agent status to STOPPED on failure try { - const db = (await import("../lib/db")).getDb(); - const { ProcessOrchestrator } = await import( - "../lib/orchestrators/process-orchestrator" - ); - const orchestrator = new ProcessOrchestrator(db); + const orchestrator = new ProcessOrchestrator(getDb()); await orchestrator.update(agent.id, { status: ProcessStatus.STOPPED, endedAt: new Date(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cli/src/commands/agent.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/commands/agent.tsx (6)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (2)
stopAll(272-309)ProcessOrchestrator(50-325)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/types/process.ts (2)
ProcessOrchestrator(47-62)Process(15-29)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/lib/launch/run.ts (1)
launchAgent(47-138)
🪛 GitHub Actions: CI
apps/cli/src/commands/agent.tsx
[error] 434-434: lint/suspicious/noExplicitAny: Unexpected any. Use a more specific type (e.g., Workspace or unknown).
[error] 447-447: lint/suspicious/noImplicitAnyLet: Variable 'ws' implicitly has any type. Provide a type annotation.
🔇 Additional comments (3)
apps/cli/src/commands/agent.tsx (3)
276-365: LGTM! Workspace-scoped agent stopping implemented correctly.The updated
AgentStopAllcomponent properly handles workspace-scoped operations with good fallback to the current workspace, clear error messages when no workspace is set or no agents are running, and informative success feedback including the stopped count.
536-553: Good fix! Proper error handling when all agents fail to start.The logic now correctly checks
created.length > 0before transitioning to theCOMPLETEstep and displays a detailed error message when all agents fail, addressing the issue from the previous review where success was reported even with zero started agents.
687-770: Excellent implementation of the agent attach flow with robust cleanup and error handling.The attach logic correctly handles:
- Lookup by ID or sessionName with clear error messaging
- Validation that the process is an agent
- Exiting Ink before tmux attachment to prevent stdin conflicts (critical)
- Deferring the actual attachment with
setImmediate- Updating agent status to
STOPPEDon attachment failure- Proper exit codes for success/failure paths
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
apps/cli/src/commands/agent.tsx (1)
418-477: Replaceanyusages in AgentStart withWorkspacefor type safety (and to fix lint)The CI warnings at Line 434 (
noExplicitAny) and Line 447 (noImplicitAnyLet) come from:const [workspace, setWorkspace] = React.useState<any>(null); … let ws; … const startAgents = async (ws: any, agents: AgentType[]) => {This also weakens type safety for workspace handling. You can fix both lint and typings by using the concrete
Workspacetype and adding a small guard:+import type { Workspace } from "../types/workspace"; @@ - const [workspace, setWorkspace] = React.useState<any>(null); + const [workspace, setWorkspace] = React.useState<Workspace | null>(null); @@ - let ws; + let ws: Workspace | null = null; @@ - const startAgents = async (ws: any, agents: AgentType[]) => { + const startAgents = async (ws: Workspace, agents: AgentType[]) => { @@ - if (item.value === "done") { - if (selectedAgents.length === 0) { + if (item.value === "done") { + if (selectedAgents.length === 0) { setError("Please select at least one agent to start"); return; + } + if (!workspace) { + setError("Workspace not loaded; please try again."); + return; } - setStep(StartStep.STARTING); - startAgents(workspace, selectedAgents); + setStep(StartStep.STARTING); + startAgents(workspace, selectedAgents);This keeps the runtime behaviour the same, clears the lint warnings, and makes it harder to accidentally call
startAgentswith an invalid object. Note this also addresses the earlier review feedback about avoidinganyhere.Also applies to: 479-557, 559-577, 583-672
apps/cli/src/lib/launch/run.ts (1)
1-7: Avoid interpolating env-derived commands into shell strings; usespawn/sanitization instead
createSessionWithRetryandcommandExistsare currently vulnerable to shell-quoting issues and potential injection because they interpolatecommand/binaryinto shell strings:const execAsync = promisify(exec); … await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`, { timeout: 10000 }); … execSync(`command -v ${binary}`, { stdio: "ignore" });Since
commandoriginates fromgetLaunchCommand(agent)and can be configured viaSUPERSET_AGENT_LAUNCH_*, a malicious or simply oddly formatted value (embedded quotes,;,&&, etc.) can break tmux startup or execute unintended shell fragments. This is effectively the same issue flagged in the earlier review.Consider tightening this in two places:
- Use
spawn(no shell) to create the tmux sessionPass the launch command as a single argument so tmux, not your local shell, interprets it:
-import { exec, execSync, spawn } from "node:child_process"; -import { promisify } from "node:util"; +import { execSync, spawn } from "node:child_process"; @@ -const execAsync = promisify(exec); @@ - await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`, { - timeout: 10000, - }); + await new Promise<void>((resolve, reject) => { + const child = spawn( + "tmux", + ["new-session", "-d", "-s", sessionName, command], + { stdio: "ignore" }, + ); + + child.on("error", reject); + child.on("exit", (code) => { + code === 0 + ? resolve() + : reject(new Error(`tmux new-session exited with code ${code ?? "null"}`)); + }); + });
- Harden
commandExistsagainst injectionEither avoid the shell entirely (e.g. by probing with
spawnSync) or at least reject unsafe binaries:-function commandExists(binary: string): boolean { - try { - execSync(`command -v ${binary}`, { stdio: "ignore" }); - return true; - } catch { - return false; - } -} +function commandExists(binary: string): boolean { + // Only allow simple command names to avoid shell metacharacters. + if (!/^[a-zA-Z0-9._-]+$/.test(binary)) { + return false; + } + try { + execSync(`command -v ${binary}`, { stdio: "ignore" }); + return true; + } catch { + return false; + } +}This preserves current behaviour while eliminating the obvious injection surface and making env-configured launch commands more robust.
Also applies to: 76-83, 89-137, 172-200, 232-239
apps/cli/src/commands/panels.tsx (1)
77-84: Exit keys are still gated bydatabeing non-null (cannot quit on loading/error)Because of
if (!data) return;at the top of the handler,Esc,q, andCtrl+Chere only run afterdatais populated. During loading or whenloadDatafails and onlyerroris set, the user cannot exit via these keys. This matches a prior comment and is still present.Reorder the checks so exit keys always work:
- useInput((input, key) => { - if (!data) return; - - if (key.escape || input === "q" || (key.ctrl && input === "c")) { - exit(); - return; - } + useInput((input, key) => { + if (key.escape || input === "q" || (key.ctrl && input === "c")) { + exit(); + return; + } + + if (!data) return;
🧹 Nitpick comments (6)
apps/cli/src/lib/persistence_docs/EXAMPLE.md (1)
117-124: stopAll docs now match agent-only behaviour; consider showing the returned countThe updated comments and example correctly reflect that
stopAllonly stops agents (not terminals) and thatendedAtis set on stopped agents. You may want to also demonstrate the numeric return value for completeness, e.g.:const stoppedCount = await processOrch.stopAll(); console.log(`Stopped ${stoppedCount} agent(s)`);This would document the updated signature more explicitly.
apps/cli/src/commands/agent.tsx (1)
276-341: AgentStopAll per-workspace logic is correct; tweak success message workspace labelThe per-workspace stop logic looks good:
- Resolves target workspace from
workspaceIdprop orgetCurrent(), with a clear error if neither is available.- Filters to running agents only (
!p.endedAt && p.type === ProcessType.AGENT) and callsstopper agent.- Returns an informative error if nothing was stopped.
One minor UX nit: in the success state you only show the workspace label when
workspaceIdprop was provided. If the workspace was inferred from the current workspace, that label won’t render even thoughtargetWorkspaceIdis known. Consider basing the label on the actual workspace used in the effect (e.g., by puttingtargetWorkspaceIdinto state) sosuperset agent stop-allwithout--workspacestill shows which workspace was affected.Also applies to: 351-361
apps/cli/src/commands/panels.tsx (2)
99-126: Selection indices can become negative or out of range when lists are emptyWhen
workspaces.lengthorfilteredAgents.lengthis0, the up/down handlers can set indices to-1or keep stale indices from previous data, leading to “no item highlighted” behavior. Rendering is safe, but UX is a bit odd.You can defensively clamp / early-return when the active list is empty:
// Navigation if (key.upArrow || input === "k") { - if (activePanel === "workspaces") { + if (activePanel === "workspaces") { + if (data.workspaces.length === 0) return; setSelectedWorkspaceIndex((prev) => prev > 0 ? prev - 1 : data.workspaces.length - 1, ); - } else if (activePanel === "agents") { + } else if (activePanel === "agents") { + if (filteredAgents.length === 0) return; setSelectedAgentIndex((prev) => prev > 0 ? prev - 1 : filteredAgents.length - 1, ); } } else if (key.downArrow || input === "j") { - if (activePanel === "workspaces") { + if (activePanel === "workspaces") { + if (data.workspaces.length === 0) return; setSelectedWorkspaceIndex((prev) => prev < data.workspaces.length - 1 ? prev + 1 : 0, ); - } else if (activePanel === "agents") { + } else if (activePanel === "agents") { + if (filteredAgents.length === 0) return; setSelectedAgentIndex((prev) => prev < filteredAgents.length - 1 ? prev + 1 : 0, ); }
26-35:onCompleteprop is unused – consider invoking it on exit
Panelsaccepts anonCompletecallback but renames it to_onCompleteand never calls it. If this matches other commands’ patterns (e.g., used in tests or higher-level orchestration), it’d be useful to call it when the panel flow finishes (on explicit exit or after a successful attach).Example:
-export function Panels({ onComplete: _onComplete }: PanelsProps) { +export function Panels({ onComplete }: PanelsProps) { @@ - const { exit } = useApp(); + const { exit } = useApp(); + + const finish = React.useCallback(() => { + onComplete?.(); + exit(); + }, [exit, onComplete]); @@ - if (key.escape || input === "q" || (key.ctrl && input === "c")) { - exit(); + if (key.escape || input === "q" || (key.ctrl && input === "c")) { + finish(); return; } @@ - // Exit Ink to stop useInput before tmux takes over stdin - exit(); + // Exit Ink to stop useInput before tmux takes over stdin + finish();Adjust attach flow as needed if you want
onCompleteto run only on “normal” exits.apps/cli/src/commands/init.tsx (2)
100-135: Normalize the branch string once to avoid subtle duplicate / storage mismatchesYou validate with
state.branch.trim()and use that for duplicate detection, but you pass rawstate.branchintocreate, which may still contain whitespace. That can cause situations where"main "passes validation, matches an existing"main"in the duplicate check, but still stores"main "in the new workspace.Consider normalizing once:
const handleBranchSubmit = async () => { - if (!state.branch.trim()) { + const branch = state.branch.trim(); + if (!branch) { @@ - const duplicate = workspaces.find( - (w) => "branch" in w && w.branch === state.branch.trim(), - ); + const duplicate = workspaces.find( + (w) => "branch" in w && w.branch === branch, + ); @@ - setState((s) => ({ - ...s, - error: undefined, - step: InitStep.INPUT_NAME, - })); + setState((s) => ({ + ...s, + branch, + error: undefined, + step: InitStep.INPUT_NAME, + }));Then
createWorkspacecan safely usestate.branchas already-normalized.Also applies to: 187-196
58-69: Explicitly decide whether empty path should mean CWD
handlePathSubmitmaps an emptystate.pathtoresolve(""), i.e., the current working directory. That may be intentional (default to CWD) or surprising given the prompt text.If you don’t want that behavior, add a simple guard:
const handlePathSubmit = async () => { - const expandedPath = state.path.replace(/^~/, process.env.HOME || "~"); + if (!state.path.trim()) { + setState((s) => ({ + ...s, + error: "Path cannot be empty", + })); + return; + } + + const expandedPath = state.path.replace(/^~/, process.env.HOME || "~");If defaulting to CWD is desired, consider mentioning it in the prompt text to make it clear to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/cli/readme.md(1 hunks)apps/cli/src/cli.tsx(8 hunks)apps/cli/src/commands/agent.tsx(5 hunks)apps/cli/src/commands/init.tsx(1 hunks)apps/cli/src/commands/panels.tsx(1 hunks)apps/cli/src/lib/launch/run.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts(14 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(7 hunks)apps/cli/src/lib/persistence_docs/EXAMPLE.md(1 hunks)apps/cli/src/lib/persistence_docs/SUMMARY.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/lib/persistence_docs/SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/lib/orchestrators/tests/process-orchestrator.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/cli/src/lib/launch/run.ts (2)
apps/cli/src/types/process.ts (1)
Agent(41-45)apps/cli/src/lib/launch/config.ts (1)
getLaunchCommand(19-42)
apps/cli/src/commands/agent.tsx (4)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (2)
stopAll(277-314)ProcessOrchestrator(50-330)apps/cli/src/types/process.ts (2)
ProcessOrchestrator(47-62)Process(15-29)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/lib/launch/run.ts (1)
launchAgent(210-263)
apps/cli/src/commands/init.tsx (3)
apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)apps/cli/src/types/workspace.ts (1)
WorkspaceOrchestrator(28-50)
apps/cli/src/lib/orchestrators/process-orchestrator.ts (3)
apps/cli/src/types/process.ts (2)
Agent(41-45)Process(15-29)apps/cli/src/lib/launch/config.ts (1)
getDefaultLaunchCommand(48-58)apps/cli/src/types/workspace.ts (1)
Workspace(6-16)
apps/cli/src/cli.tsx (7)
apps/cli/src/commands/init.tsx (1)
Init(37-401)apps/cli/src/commands/dashboard.tsx (1)
Dashboard(28-539)apps/cli/src/commands/panels.tsx (1)
Panels(26-420)apps/cli/src/commands/workspace.tsx (1)
WorkspaceUse(232-280)apps/cli/src/commands/agent.tsx (3)
AgentStart(430-675)AgentAttach(682-781)AgentStopAll(281-365)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-224)
apps/cli/src/commands/panels.tsx (4)
apps/cli/src/types/process.ts (3)
Process(15-29)ProcessOrchestrator(47-62)Agent(41-45)apps/cli/src/lib/db.ts (1)
getDb(9-18)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(50-330)apps/cli/src/lib/launch/run.ts (1)
launchAgent(210-263)
🪛 GitHub Actions: CI
apps/cli/src/commands/agent.tsx
[warning] 434-434: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[warning] 447-447: lint/suspicious/noImplicitAnyLet: This variable implicitly has the any type. Declare a type or initialize.
apps/cli/src/cli.tsx
[warning] 4-4: lint/correctness/noUnusedImports: Unused import React detected.
🪛 LanguageTool
apps/cli/readme.md
[style] ~43-~43: To form a complete sentence, be sure to include a subject.
Context: ...nment and state.currentWorkspaceId. - Can be overridden with `SUPERSET_CLI_DATA_D...
(MISSING_IT_THERE)
🔇 Additional comments (4)
apps/cli/readme.md (1)
1-48: Well-structured documentation for expanded CLI surface.The README provides clear organization with practical Quick start, Core concepts, Common commands, and operational sections. Commands are well-documented with concrete examples, and tmux/storage integration details are helpful for users.
apps/cli/src/cli.tsx (1)
42-65: CLI command wiring and default action look coherentThe added commands (
init,dashboard,panels,workspace use,agent start,agent attach,agent stop-all, andchangegroup) are wired consistently:
- Each command renders the corresponding Ink component with appropriate props.
- Mutating commands pass
onComplete={() => process.exit(0)}so the process terminates cleanly after UI completion.agent startandagent attachrely on their components to control exit, which matches their more interactive flows.- The default
program.actiongives a helpful overview and attempts to show the current workspace, while safely swallowing DB-init errors.No functional issues stand out here.
Also applies to: 66-178, 201-217, 271-284, 295-334, 335-381
apps/cli/src/commands/agent.tsx (1)
682-781: AgentAttach flow and failure handling look solidThe attach flow is robust:
- Resolves the target process by ID first, then falls back to sessionName lookup among agents.
- Validates that the located process is an agent before proceeding.
- Exits Ink before attaching, then calls
launchAgent(agent, { attach: true })insetImmediate, which avoids conflicting with Ink’s input handling.- On attach failure, attempts to mark the agent
STOPPEDwith anendedAttimestamp, logging a warning if persistence fails, and exits with a non-zero code.No obvious logic or lifecycle issues here; the approach is consistent with the tmux-based orchestration elsewhere.
apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
1-45: ProcessOrchestrator tmux integration and stopAll semantics look consistentThe new tmux-related helpers and backfill logic are coherent:
backfillDefaultssets sane defaults for status and timestamps and upgrades legacy agentlaunchCommand/sessionNamevalues.needsPersistandsyncAgentStatusgate persistence and keep agent.status in sync with tmux reality without over-writing healthy records.createnow generates deterministic IDs and session names and starts processes inIDLE, which aligns with the AgentStart flow.stopandstopAllcorrectly:
- kill tmux sessions for agents with a
sessionName, ignoring already-dead sessions;- set
endedAt/updatedAtand mark agents asSTOPPED;- and
stopAllreturns the count of agents stopped and deliberately ignores terminals.This matches the updated docs and CLI descriptions about stopping only agents, not terminals.
Also applies to: 53-94, 103-190, 192-222, 243-270, 272-314
| @@ -3,23 +3,29 @@ import { Command } from "commander"; | |||
| import { render } from "ink"; | |||
| import React from "react"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
Remove unused React import to satisfy lint
React is not referenced in this file; JSX elements are only passed to render. You can safely drop this import to clear the noUnusedImports warning:
-import React from "react";Assuming your TS config uses the modern JSX transform (jsx: "react-jsx"), this will still compile.
🏁 Script executed:
#!/bin/bash
# Verify React import and usage in cli.tsx
echo "=== Examining apps/cli/src/cli.tsx ==="
cat apps/cli/src/cli.tsx | head -20
echo ""
echo "=== Searching for 'React' references in file ==="
rg -n 'React' apps/cli/src/cli.tsx || echo "No React references found"
echo ""
echo "=== Checking for JSX syntax ==="
rg -n 'JSX|<[A-Z]|</' apps/cli/src/cli.tsx | head -10Length of output: 927
Remove unused React import from line 4
Verified: React is imported but never referenced in the file. The file uses JSX extensively (<Init />, <Dashboard />, <Panels />, etc.), but none of these reference the React object directly. With the modern JSX transform, this import is not required and can be safely removed:
-import React from "react";📝 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.
| import React from "react"; |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 4-4: lint/correctness/noUnusedImports: Unused import React detected.
🤖 Prompt for AI Agents
In apps/cli/src/cli.tsx around line 4, remove the unused import "import React
from \"react\";" because the file never references the React identifier and the
project uses the modern JSX transform; simply delete that import line and ensure
no other references to React exist so the file still compiles.
| const handleAgentsSelect = (item: { value: string }) => { | ||
| if (item.value === "done") { | ||
| createWorkspace(); | ||
| } else if (item.value === "skip") { | ||
| setState((s) => ({ ...s, defaultAgents: [] })); | ||
| createWorkspace(); | ||
| } else { | ||
| // Toggle agent selection | ||
| setState((s) => ({ | ||
| ...s, | ||
| defaultAgents: s.defaultAgents.includes(item.value as AgentType) | ||
| ? s.defaultAgents.filter((a) => a !== item.value) | ||
| : [...s.defaultAgents, item.value as AgentType], | ||
| })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Skip default agents path still uses previously selected agents
In handleAgentsSelect, for "skip" you clear defaultAgents via setState and then immediately call createWorkspace(). Because React state updates are async, createWorkspace still sees the old state.defaultAgents, so the created workspace will keep those agents instead of none.
} else if (item.value === "skip") {
setState((s) => ({ ...s, defaultAgents: [] }));
createWorkspace();
}But createWorkspace reads from the closed-over state:
const workspace = await orchestrator.create(
envId,
state.workspaceType!,
{
path: state.path || undefined,
branch: state.branch || undefined,
name: state.name || undefined,
defaultAgents: state.defaultAgents,
},
);Consider passing the effective config into createWorkspace, or calculating it inside createWorkspace from the latest state passed via functional update:
- const createWorkspace = async () => {
+ const createWorkspace = async (overrides?: Partial<Pick<InitState, "defaultAgents">>) => {
+ const effective = { ...state, ...overrides };
setState((s) => ({ ...s, step: InitStep.CREATING }));
@@
- const workspace = await orchestrator.create(
- envId,
- state.workspaceType!,
- {
- path: state.path || undefined,
- branch: state.branch || undefined,
- name: state.name || undefined,
- defaultAgents: state.defaultAgents,
- },
- );
+ const workspace = await orchestrator.create(
+ envId,
+ effective.workspaceType!,
+ {
+ path: effective.path || undefined,
+ branch: effective.branch || undefined,
+ name: effective.name || undefined,
+ defaultAgents: effective.defaultAgents,
+ },
+ );
@@
-} else if (item.value === "skip") {
- setState((s) => ({ ...s, defaultAgents: [] }));
- createWorkspace();
+} else if (item.value === "skip") {
+ createWorkspace({ defaultAgents: [] });Also applies to: 162-215
I'll help verify this review comment by examining the codebase. Let me investigate the code structure and the specific issue identified.
---
🏁 Script executed:
```shell
#!/bin/bash
# First, let's view the full context of the handleAgentsSelect function and createWorkspace
cat apps/cli/src/commands/init.tsx | head -250 | tail -120
Length of output: 2987
🏁 Script executed:
#!/bin/bash
# Search for all createWorkspace definitions and calls to understand the closure issue
rg -n "createWorkspace" apps/cli/src/commands/init.tsx -B2 -A5Length of output: 629
🏁 Script executed:
#!/bin/bash
# Check the exact lines around 145-160 and 162-215 for context
wc -l apps/cli/src/commands/init.tsxLength of output: 34
🏁 Script executed:
#!/bin/bash
# Look for setState calls followed by function calls that might have the same issue
rg -n "setState.*defaultAgents" apps/cli/src/commands/init.tsx -B1 -A3Length of output: 170
🏁 Script executed:
#!/bin/bash
# Get full view of the file to understand the structure
head -250 apps/cli/src/commands/init.tsxLength of output: 6021
I need to check if there are other similar patterns with the async state issue:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all setState calls in handleAgentsSelect and surrounding code
rg -n "setState" apps/cli/src/commands/init.tsx -B1 -A2
Length of output: 2210
🏁 Script executed:
#!/bin/bash
# Check if there are other handler functions that might have similar async issues
sed -n '45,250p' apps/cli/src/commands/init.tsx | grep -n "const handle\|setState\|createWorkspace\|async"Length of output: 738
🏁 Script executed:
#!/bin/bash
# Check the entire file for any async function calls immediately after setState
rg -n "setState.*\);\s*(await|createWorkspace|orchestrator\.|envOrchestrator\.)" apps/cli/src/commands/init.tsxLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look at handleBranchSubmit to understand the trim issue mentioned
sed -n '95,135p' apps/cli/src/commands/init.tsxLength of output: 843
Async state update causes skip to preserve previously selected agents
In handleAgentsSelect, when the user selects "skip", you call setState({ defaultAgents: [] }) followed immediately by createWorkspace(). Because React state updates are asynchronous, createWorkspace reads state.defaultAgents from the closure before the state change applies, so the workspace is created with the previously selected agents instead of an empty list.
} else if (item.value === "skip") {
setState((s) => ({ ...s, defaultAgents: [] }));
createWorkspace();
}Pass the effective value explicitly to createWorkspace:
- const createWorkspace = async () => {
+ const createWorkspace = async (overrides?: Partial<Pick<InitState, "defaultAgents">>) => {
+ const effective = { ...state, ...overrides };
setState((s) => ({ ...s, step: InitStep.CREATING }));
const workspace = await orchestrator.create(
envId,
effective.workspaceType!,
{
path: effective.path || undefined,
branch: effective.branch || undefined,
name: effective.name || undefined,
defaultAgents: effective.defaultAgents,
},
);
- } else if (item.value === "skip") {
- setState((s) => ({ ...s, defaultAgents: [] }));
- createWorkspace();
+ } else if (item.value === "skip") {
+ createWorkspace({ defaultAgents: [] });🤖 Prompt for AI Agents
In apps/cli/src/commands/init.tsx around lines 145 to 160, the code sets
defaultAgents to [] then immediately calls createWorkspace(), but because
setState is async the old agents may still be used; instead compute the intended
agents value and pass it explicitly to createWorkspace (for example determine
agents = item.value === "skip" ? [] : current/updated list) then call setState
with the new defaultAgents and call createWorkspace(agents) (or otherwise update
createWorkspace to accept the agents list) so the workspace is created with the
correct effective agents.
| const loadData = React.useCallback(async () => { | ||
| try { | ||
| const db = getDb(); | ||
| const workspaceOrchestrator = new WorkspaceOrchestrator(db); | ||
| const processOrchestrator = new ProcessOrchestrator(db); | ||
|
|
||
| const [workspaces, processes, currentWorkspace] = await Promise.all([ | ||
| workspaceOrchestrator.list(), | ||
| processOrchestrator.list(), | ||
| workspaceOrchestrator.getCurrent(), | ||
| ]); | ||
|
|
||
| setData({ | ||
| workspaces, | ||
| processes, | ||
| currentWorkspaceId: currentWorkspace?.id, | ||
| lastRefresh: new Date(), | ||
| }); | ||
|
|
||
| setLoading(false); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : "Unknown error"); | ||
| setLoading(false); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
Error state never cleared after a successful loadData refresh
On a transient failure, setError(...) is called, but on later successful refreshes error is never reset, so the component will keep rendering the error screen even with fresh data.
Consider clearing the error on success:
const loadData = React.useCallback(async () => {
try {
const db = getDb();
const workspaceOrchestrator = new WorkspaceOrchestrator(db);
const processOrchestrator = new ProcessOrchestrator(db);
@@
- setData({
+ setData({
workspaces,
processes,
currentWorkspaceId: currentWorkspace?.id,
lastRefresh: new Date(),
});
-
- setLoading(false);
+ setError(null);
+ setLoading(false);
} catch (err) {
setError(err instanceof Error ? err.message : "Unknown error");
setLoading(false);
}
}, []);🏁 Script executed:
#!/bin/bash
# Inspect the actual file to verify the loadData implementation
cat apps/cli/src/commands/panels.tsx | head -100Length of output: 2862
🏁 Script executed:
#!/bin/bash
# Search for all setError calls and error-related state in the file
rg -n 'setError|error' apps/cli/src/commands/panels.tsx -C2Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Check if error is initialized and how the component handles it on initial load
rg -n 'useState.*error|const.*error' apps/cli/src/commands/panels.tsx -C2Length of output: 344
🏁 Script executed:
#!/bin/bash
# Search for conditional rendering that might depend on error state
rg -n 'if.*error|error.*?' apps/cli/src/commands/panels.tsx -C1Length of output: 871
🏁 Script executed:
#!/bin/bash
# Get the complete render/return section to verify error handling in output
tail -n +175 apps/cli/src/commands/panels.tsx | head -30Length of output: 715
🏁 Script executed:
#!/bin/bash
# Search for any other places where error state might be reset
rg -n 'setError\(' apps/cli/src/commands/panels.tsxLength of output: 69
Error state never cleared after successful loadData refresh
On transient failure, setError() is called (line 60), but on subsequent successful refreshes the error is never reset. Since the render checks if (error) before rendering data (lines 180–181), the component will keep displaying the stale error screen even with fresh successful data.
The suggested fix is correct: add setError(null) after successful data update:
const loadData = React.useCallback(async () => {
try {
const db = getDb();
const workspaceOrchestrator = new WorkspaceOrchestrator(db);
const processOrchestrator = new ProcessOrchestrator(db);
const [workspaces, processes, currentWorkspace] = await Promise.all([
workspaceOrchestrator.list(),
processOrchestrator.list(),
workspaceOrchestrator.getCurrent(),
]);
setData({
workspaces,
processes,
currentWorkspaceId: currentWorkspace?.id,
lastRefresh: new Date(),
});
+ setError(null);
setLoading(false);
} catch (err) {
setError(err instanceof Error ? err.message : "Unknown error");
setLoading(false);
}
}, []);📝 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 loadData = React.useCallback(async () => { | |
| try { | |
| const db = getDb(); | |
| const workspaceOrchestrator = new WorkspaceOrchestrator(db); | |
| const processOrchestrator = new ProcessOrchestrator(db); | |
| const [workspaces, processes, currentWorkspace] = await Promise.all([ | |
| workspaceOrchestrator.list(), | |
| processOrchestrator.list(), | |
| workspaceOrchestrator.getCurrent(), | |
| ]); | |
| setData({ | |
| workspaces, | |
| processes, | |
| currentWorkspaceId: currentWorkspace?.id, | |
| lastRefresh: new Date(), | |
| }); | |
| setLoading(false); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : "Unknown error"); | |
| setLoading(false); | |
| } | |
| }, []); | |
| const loadData = React.useCallback(async () => { | |
| try { | |
| const db = getDb(); | |
| const workspaceOrchestrator = new WorkspaceOrchestrator(db); | |
| const processOrchestrator = new ProcessOrchestrator(db); | |
| const [workspaces, processes, currentWorkspace] = await Promise.all([ | |
| workspaceOrchestrator.list(), | |
| processOrchestrator.list(), | |
| workspaceOrchestrator.getCurrent(), | |
| ]); | |
| setData({ | |
| workspaces, | |
| processes, | |
| currentWorkspaceId: currentWorkspace?.id, | |
| lastRefresh: new Date(), | |
| }); | |
| setError(null); | |
| setLoading(false); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : "Unknown error"); | |
| setLoading(false); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In apps/cli/src/commands/panels.tsx around lines 39 to 63, loadData sets an
error on failure but never clears it on success, causing stale error UI to
persist; update the success path to call setError(null) after successfully
setting data (before or alongside setLoading(false)) so any previous error is
cleared when new data loads.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/cli/src/lib/launch/run.ts (1)
1-7: Critical shell injection vulnerability remains unaddressedThe past review identified a critical shell injection vulnerability at line 100 (previously line 91) where
commandandsessionNameare interpolated into a shell string:await execAsync(`tmux new-session -d -s "${sessionName}" "${command}"`, {This issue was marked as "✅ Addressed in commit 64fbac1" but the vulnerable code pattern is still present. The
commandvariable comes from environment variables (SUPERSET_AGENT_LAUNCH_*) and can contain shell metacharacters that will be interpreted by the shell.The fix suggested in the previous review should be applied immediately: replace
execAsyncwithspawnusing an argument array to prevent shell interpolation.Also applies to: 100-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cli/src/lib/launch/run.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/lib/launch/run.ts (2)
apps/cli/src/types/process.ts (1)
Agent(41-45)apps/cli/src/lib/launch/config.ts (1)
getLaunchCommand(19-42)
🔇 Additional comments (4)
apps/cli/src/lib/launch/run.ts (4)
8-12: LGTM!Clean interface definition for launch results.
17-24: LGTM!Safe implementation using a static command string with no interpolation.
210-267: LGTM with caveats!The launch logic is well-structured with appropriate validation, preflight checks, and clear error messages. The shell wrapper detection (lines 234-236) is a thoughtful addition to avoid false negatives.
However, note that
commandExists()called at line 238 has a critical shell injection vulnerability that must be addressed.
275-372: LGTM - demonstrates the correct spawn pattern!This function correctly uses
spawnwith an argument array at line 317:const child = spawn("tmux", ["attach", "-t", sessionName], { stdio: "inherit", detached: false, });This is the safe pattern that should be applied to the other tmux invocations in this file (lines 31, 45, 60, and especially line 100). No shell interpolation means no injection risk.
The retry logic and error handling are well-implemented, and the UI decoration provides good UX.
| function tmuxSessionExists(sessionName: string): boolean { | ||
| try { | ||
| execSync(`tmux has-session -t "${sessionName}" 2>/dev/null`, { | ||
| stdio: "ignore", | ||
| }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential shell injection in session name handling
Three utility functions interpolate sessionName directly into shell strings:
- Line 31:
tmux has-session -t "${sessionName}" - Line 45:
tmux kill-session -t "${sessionName}" - Line 60:
tmux list-panes -t "${sessionName}"
While sessionName is typically derived from agent.sessionName or generated as agent-${agent.id.slice(0,6)}, if agent.sessionName is user-controlled and contains shell metacharacters (e.g., "; rm -rf /;"), these could become injection vectors.
Consider refactoring to use spawn with argument arrays instead of shell string interpolation for defense in depth.
Also applies to: 43-51, 57-71
🤖 Prompt for AI Agents
In apps/cli/src/lib/launch/run.ts around lines 29-38 (and similarly 43-51,
57-71), the code interpolates sessionName into shell command strings which
allows shell injection; replace execSync usage with a non-shell child_process
call (spawnSync or execFileSync) passing the tmux subcommand and session name as
separate arguments (e.g., ["has-session", "-t", sessionName]) and set shell:
false / use execFileSync so the sessionName is not interpreted by a shell;
additionally validate or constrain sessionName to an allowed pattern (e.g.,
alphanumeric, dash, underscore, max length) before invoking the process and
preserve existing stdio/error handling and return value semantics.
| function commandExists(binary: string): boolean { | ||
| try { | ||
| execSync(`command -v ${binary}`, { stdio: "ignore" }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical shell injection in commandExists
Line 78 interpolates binary into a shell command without quotes:
execSync(`command -v ${binary}`, { stdio: "ignore" });Since binary ultimately comes from user-controlled environment variables (SUPERSET_AGENT_LAUNCH_*), an attacker could inject shell commands. For example, if SUPERSET_AGENT_LAUNCH_FOO="evil; rm -rf /", then binary becomes "evil;" and arbitrary code executes.
Apply this diff to fix:
- execSync(`command -v ${binary}`, { stdio: "ignore" });
+ execSync("command", ["-v", binary], { stdio: "ignore" });Or better yet, use spawn:
-function commandExists(binary: string): boolean {
- try {
- execSync(`command -v ${binary}`, { stdio: "ignore" });
- return true;
- } catch {
- return false;
- }
-}
+function commandExists(binary: string): boolean {
+ try {
+ // Use which as a safer alternative that accepts arguments
+ execSync(`which "${binary.replace(/"/g, '\\"')}"`, { stdio: "ignore" });
+ return true;
+ } catch {
+ return false;
+ }
+}Note: execSync doesn't accept an array as second parameter. Use which with escaped quotes as shown, or refactor to spawnSync.
🤖 Prompt for AI Agents
In apps/cli/src/lib/launch/run.ts around lines 76 to 83, commandExists currently
interpolates the user-controlled `binary` into a shell string causing a critical
shell-injection risk; replace the execSync call with a shell-argument-safe API
(e.g., use child_process.spawnSync or execFile/execFileSync) and call a
non-shell program like `which` (or `command` as an executable if available)
passing `binary` as an argument array so it is never interpreted by a shell;
return true when the child process exits with code 0 and false otherwise, and
optionally validate `binary` against a whitelist/allowed pattern before invoking
the child process.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.