feat(desktop): SSH workspace support — remote terminal, image paste, connection management#1566
feat(desktop): SSH workspace support — remote terminal, image paste, connection management#1566yigitkonur wants to merge 8 commits into
Conversation
Make SSH session detach non-destructive by keeping the SSH channel alive instead of closing it on navigation away. Add session reuse in doCreateOrAttach() so returning from Settings reattaches to the existing live session rather than creating a fresh shell. Clear stale cold-restore state only when a live session is recovered, preventing restored-mode UI from incorrectly reappearing. Also adds all previously untracked remote-ssh module files to version control (were excluded by global lib/ gitignore rule).
Refactor the Changes panel to use a GitRunner abstraction that routes git commands over SSH for remote workspaces. The same parsers work for both local and remote — only the transport changes. Key fix: RemoteGitRunner now awaits ensureConnected() before exec, preventing the race where the Changes tab hit the runtime before the SSH connection was ready. Also add retry + retryDelay on the getBranches query so it recovers instead of getting stuck permanently. Passes workspaceId through the full component tree (ChangesView, CommitInput, FileDiffSection, FileViewerPane, etc.) so every git operation resolves the correct runner.
📝 WalkthroughWalkthroughIntroduces comprehensive SSH remote workspace support enabling users to connect to remote projects over SSH. Adds SSH connection management, remote workspace creation/routing, multi-service remote runtime (terminal, git, SFTP, clipboard), database schema updates, backend TRPC procedures, and frontend UI for SSH connections and remote workspace workflows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Desktop as Superset Desktop
participant WorkspaceRT as WorkspaceRuntime<br/>Registry
participant RemoteSSH as RemoteSSH<br/>WorkspaceRuntime
participant SSHPool as SSHConnection<br/>Pool
participant SSHConn as SSHConnection
participant Remote as Remote Host
User->>Desktop: Create Remote Workspace
Desktop->>WorkspaceRT: getForWorkspaceId(workspaceId)
WorkspaceRT->>WorkspaceRT: Query local DB for<br/>workspace type & SSH ID
alt Is Remote Workspace
WorkspaceRT->>SSHPool: getOrCreate(sshConfig)
SSHPool->>SSHPool: Check pool cache
alt Connection exists
SSHPool->>SSHPool: Clear idle timer
else New connection needed
SSHPool->>SSHConn: new SSHConnection(config)
SSHConn->>Remote: Connect via ssh2
Remote-->>SSHConn: Connected
SSHPool->>SSHPool: Cache connection
end
SSHPool-->>RemoteSSH: Return SSHConnection
RemoteSSH->>SSHConn: ensureConnected()
SSHConn->>Remote: Deploy clipboard proxy scripts
Remote-->>SSHConn: Scripts deployed
RemoteSSH-->>WorkspaceRT: RemoteSSHWorkspaceRuntime ready
else Is Local Workspace
WorkspaceRT-->>WorkspaceRT: Return LocalWorkspaceRuntime
end
WorkspaceRT-->>Desktop: WorkspaceRuntime instance
Desktop->>RemoteSSH: terminal.createOrAttach()
RemoteSSH->>SSHConn: openShell() or getTmux session
SSHConn->>Remote: Start terminal session
Remote-->>SSHConn: Shell/Tmux session active
SSHConn-->>RemoteSSH: ClientChannel
RemoteSSH-->>Desktop: SessionResult with terminal pane
User->>Desktop: Paste image to remote terminal
Desktop->>RemoteSSH: uploadClipboardImage(imageData)
RemoteSSH->>SSHConn: exec() SFTP upload
SSHConn->>Remote: Upload image to ~/.superset/clipboard/
Remote-->>SSHConn: Image stored
SSHConn-->>RemoteSSH: Remote path
RemoteSSH->>Desktop: Return clipboard image path
Desktop->>Desktop: Write shell command to terminal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
i know someone else is working on a similar PR, but i still wanted to send this because it has some critical fixes for me. you might not want to merge it right away, but there are definitely points worth checking. i spent a ton of time figuring out how to paste a remote image and finally got it working. been using it with my Claude Code setup. opening the PR since it's been working well for me locally. might not meet all the contribution guidelines, so feel free to close it. no hard feelings! |
This reverts commit 3aa0ee8.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsx (1)
55-58:⚠️ Potential issue | 🟠 Major
workspaceIdmissing fromgetBranches.invalidate— stale branch data after base-branch update for remote workspacesThe query at line 51 is keyed on
{ worktreePath, workspaceId }, but theonSuccessinvalidation on line 57 only passes{ worktreePath }. WhenworkspaceIdis a non-undefined string (i.e., any remote workspace), the tRPC/React Query serialized cache key won't match, so the branch list will remain stale after a successfulupdateBaseBranchmutation.🐛 Proposed fix
const updateBaseBranch = electronTrpc.changes.updateBaseBranch.useMutation({ onSuccess: () => { - utils.changes.getBranches.invalidate({ worktreePath }); + utils.changes.getBranches.invalidate({ worktreePath, workspaceId }); }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsx` around lines 55 - 58, The onSuccess invalidation for the updateBaseBranch mutation omits workspaceId, so utils.changes.getBranches.invalidate({ worktreePath }) doesn't match the query key used elsewhere (which is { worktreePath, workspaceId }) and leaves remote workspace branch lists stale; update the onSuccess handler in updateBaseBranch (the electronTrpc.changes.updateBaseBranch.useMutation call) to call utils.changes.getBranches.invalidate with both worktreePath and workspaceId (e.g., utils.changes.getBranches.invalidate({ worktreePath, workspaceId })) so the correct cache entry is invalidated after a successful mutation.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/InfiniteScrollView.tsx (1)
243-292:⚠️ Potential issue | 🟠 Major
workspaceIdis missing from thestagedandunstagedVirtualizedFileListinstances, breaking remote diff content loading for those sections.All other
VirtualizedFileList/ child consumers receiveworkspaceId(against-base at Line 199, CommitSection at Line 224, focus-modeFileDiffSectionat Line 174), but the staged (Lines 244–261) and unstaged (Lines 274–292) lists do not. SinceVirtualizedFileList→FileDiffSection→getFileContentsquery rely onworkspaceIdto route through the SSH connection, diffs in these two sections will fail to load for remote workspaces. Note that the staging/unstaging mutations at Lines 255 and 285 correctly includeworkspaceId, so only the diff viewer is affected.🐛 Proposed fix
{expandedCategories.staged && ( <VirtualizedFileList files={sortedStaged} category="staged" worktreePath={worktreePath} + workspaceId={workspaceId} collapsedFiles={collapsedFiles} onToggleFile={toggleFile} scrollElementRef={containerRef} onUnstage={(file) => unstageFileMutation.mutate({ worktreePath, filePath: file.path, workspaceId, }) } onDiscard={handleDiscard} isActioning={isActioning} /> )}{expandedCategories.unstaged && ( <VirtualizedFileList files={sortedUnstaged} category="unstaged" worktreePath={worktreePath} + workspaceId={workspaceId} collapsedFiles={collapsedFiles} onToggleFile={toggleFile} scrollElementRef={containerRef} onStage={(file) => stageFileMutation.mutate({ worktreePath, filePath: file.path, workspaceId, }) } onDiscard={handleDiscard} isActioning={isActioning} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/InfiniteScrollView.tsx` around lines 243 - 292, The staged and unstaged VirtualizedFileList instances are missing the workspaceId prop causing remote diff loading to fail; update both VirtualizedFileList usages (the ones rendering under expandedCategories.staged and expandedCategories.unstaged) to pass workspaceId alongside the existing props (files, category, worktreePath, collapsedFiles, onToggleFile, scrollElementRef, onStage/onUnstage, onDiscard, isActioning) so FileDiffSection/getFileContents receive the workspaceId for SSH routing.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx (1)
108-116:⚠️ Potential issue | 🟡 Minor
openFileInEditorconstructs a local path from a remoteworktreePath, silently failing for remote workspaces.When
workspaceIdis present (remote workspace),worktreePathis a remote path like/home/user/project. The mutationopenInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath })sends this remote path to the Electron main process which resolves it locally — the file doesn't exist and the action fails silently (or with a confusing error).Consider guarding the handler:
🛡️ Proposed fix
const handleOpenInEditor = useCallback( (e: React.MouseEvent) => { e.stopPropagation(); - if (worktreePath) { + if (worktreePath && !workspaceId) { const absolutePath = `${worktreePath}/${file.path}`; openInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath }); } }, - [worktreePath, file.path, openInEditorMutation], + [worktreePath, workspaceId, file.path, openInEditorMutation], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx` around lines 108 - 116, The handler handleOpenInEditor should avoid sending a remote path to openInEditorMutation for remote workspaces: check workspaceId (or an isRemoteWorkspace flag) and if present either block the action and surface a user-friendly message or route to a remote-open helper instead; only construct absolutePath and call openInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath }) when workspaceId is falsy (local), keeping e.stopPropagation() intact and referencing file.path, worktreePath, handleOpenInEditor and openInEditorMutation to locate the change.apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsx (1)
134-165:⚠️ Potential issue | 🟠 Major
projectTypeis always"local"— remote projects will be misidentified.Every
ProjectGroupentry is created withprojectType: "local"(line 142) regardless of whether the project is remote. TheWorkspaceItemtype does not carryprojectType, so there is currently no source for the correct value in this pipeline. Any downstream consumer ofProjectGroup.projectTypethat tries to distinguish remote projects will see incorrect data.The fix requires propagating project type through the data pipeline. Two viable approaches:
- Add
projectTypetoWorkspaceItemand source it fromgroup.project(in the open-workspaces loop) orallProjects[i](in the closed-worktrees loop), then readitem.projectTypewhen buildingprojectGroups.- Build a project-type lookup map from
allProjectsorgroupsbefore theprojectGroupsmemo and use it when settingprojectTypeon each new group entry.♻️ Sketch of approach 2
+ // Build a map of projectId → projectType from allProjects data + const projectTypeMap = useMemo(() => { + const map = new Map<string, string>(); + for (const p of allProjects) { + map.set(p.id, p.projectType ?? "local"); + } + return map; + }, [allProjects]); const projectGroups = useMemo<ProjectGroup[]>(() => { const groupsMap = new Map<string, ProjectGroup>(); for (const item of filteredItems) { if (!groupsMap.has(item.projectId)) { groupsMap.set(item.projectId, { projectId: item.projectId, projectName: item.projectName, - projectType: "local", + projectType: projectTypeMap.get(item.projectId) ?? "local", workspaces: [], }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsx` around lines 134 - 165, The ProjectGroup.projectType is hardcoded to "local" in the projectGroups useMemo causing remote projects to be misidentified; fix by building a lookup of projectId→projectType from the source project collection (e.g., allProjects or groups) before computing projectGroups, then when creating a new group inside the projectGroups useMemo set projectType = projectTypeLookup.get(item.projectId) (fallback to "local" if missing); ensure any references to WorkspaceItem/projectGroups now rely on that lookup so downstream consumers get the correct projectType.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (2)
141-145:⚠️ Potential issue | 🟠 MajorMissing
workspaceIdinuseGitChangesStatus— remote workspaces won't resolve the correct git runner.The hook is called without
workspaceId, but theidprop (workspace ID) is available. For remote workspaces, the backend'sresolveGitTargetneedsworkspaceIdto route git operations through the SSH runner. Without it,getBranchesandgetStatusqueries will fail or use a local runner for a remote path.🐛 Proposed fix
const { status: localChanges } = useGitChangesStatus({ worktreePath, + workspaceId: id, enabled: hasHovered && !!worktreePath, staleTime: GITHUB_STATUS_STALE_TIME, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 141 - 145, The hook invocation of useGitChangesStatus is missing the workspaceId, causing remote workspaces to route git ops to the wrong runner; update the call inside WorkspaceListItem to pass the workspace id (the id prop) as workspaceId (e.g., useGitChangesStatus({ worktreePath, workspaceId: id, enabled: hasHovered && !!worktreePath, staleTime: GITHUB_STATUS_STALE_TIME })) so resolveGitTarget on the backend can correctly select the SSH runner for remote paths.
278-296:⚠️ Potential issue | 🟠 Major
sshHostandsshUsernameare not passed toCollapsedWorkspaceItemfor remote workspaces.The
CollapsedWorkspaceItemcomponent acceptssshHostandsshUsernameprops to display SSH connection info in the tooltip, but they are not forwarded here. Remote workspaces in collapsed view will always show the generic "SSH" fallback instead ofusername@host.🐛 Proposed fix
<CollapsedWorkspaceItem id={id} name={name} branch={branch} type={type} isActive={isActive} isUnread={isUnread} workspaceStatus={workspaceStatus} itemRef={itemRef} showDeleteDialog={showDeleteDialog} setShowDeleteDialog={setShowDeleteDialog} onMouseEnter={handleMouseEnter} onClick={handleClick} onDeleteClick={handleDeleteClick} onCopyPath={handleCopyPath} + sshHost={sshHost} + sshUsername={sshUsername} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 278 - 296, CollapsedWorkspaceItem is missing sshHost and sshUsername props so remote workspaces in the collapsed view show the generic "SSH" fallback; pass the component's sshHost and sshUsername values into the CollapsedWorkspaceItem JSX (alongside id, name, branch, type, etc.) so the tooltip can render username@host for remote workspaces; update the CollapsedWorkspaceItem invocation in WorkspaceListItem to include sshHost={sshHost} and sshUsername={sshUsername} (and ensure those variables are in scope/props).apps/desktop/src/lib/trpc/routers/changes/staging.ts (1)
52-68:⚠️ Potential issue | 🟡 Minor
deleteFileswithgit clean -fon the remote path won't remove untracked directories.
git clean -f(without-d) only removes untracked files, not untracked directories. If untracked items include directories, they'll silently remain. The local path usingsecureFs.deletemay handle directories differently, creating an asymmetry between local and remote behavior.If directory cleanup is intended, add
-d:- await runner.raw(["clean", "-f", "--", ...filePaths]); + await runner.raw(["clean", "-fd", "--", ...filePaths]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/staging.ts` around lines 52 - 68, The remote branch of deleteFiles (function deleteFiles) uses runner.raw(["clean","-f",...filePaths]) which won't remove untracked directories; change the git clean invocation to include -d so directories are removed (e.g. runner.raw(["clean","-f","-d","--", ...filePaths])); keep the same guard for filePaths.length > 0 and preserve the local branch using secureFs.delete to maintain parity between remote and local behavior.
🟠 Major comments (19)
apps/desktop/scripts/setup-signing.sh-252-288 (1)
252-288:⚠️ Potential issue | 🟠 MajorUnquoted values in the heredoc will break
source "$ENV_FILE"for any standard Apple identity.A typical Developer ID certificate name is
Developer ID Application: Company Name (ABCDE12345)— it contains spaces. Written asCSC_NAME=Developer ID Application: Company Name (ABCDE12345)to the env file, whensourced underset -euo pipefailthe shell treatsDeveloper ID Application...as a command to execute afterCSC_NAME=Developer, immediately aborting with a "command not found" error. The same risk applies toAPPLE_API_KEYwhen the.p8path contains spaces.🐛 Proposed fix — double-quote all values in the heredoc
cat > "$ENV_FILE" <<EOF # ... # Signing identity (from Keychain) -CSC_NAME=${identity} +CSC_NAME="${identity}" # Apple Team ID -APPLE_TEAM_ID=${team_id} +APPLE_TEAM_ID="${team_id}" # Enable notarization (set to "true" for distribution builds) -APPLE_NOTARIZE=${notarize} +APPLE_NOTARIZE="${notarize}" EOFAnd for the conditional blocks appended afterwards:
# App Store Connect API Key (for notarization) -APPLE_API_KEY=${api_key} -APPLE_API_KEY_ID=${api_key_id} -APPLE_API_ISSUER=${api_issuer} +APPLE_API_KEY="${api_key}" +APPLE_API_KEY_ID="${api_key_id}" +APPLE_API_ISSUER="${api_issuer}"# Apple ID credentials (for notarization) -APPLE_ID=${apple_id} -APPLE_PASSWORD=${apple_password} +APPLE_ID="${apple_id}" +APPLE_PASSWORD="${apple_password}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/setup-signing.sh` around lines 252 - 288, The heredoc writing to $ENV_FILE in setup-signing.sh writes unquoted values (e.g., CSC_NAME, APPLE_API_KEY, APPLE_ID, APPLE_PASSWORD, APPLE_API_KEY_ID, APPLE_API_ISSUER) which breaks sourcing when values contain spaces; update the heredoc lines to emit quoted values (e.g., CSC_NAME="${identity}", APPLE_API_KEY="${api_key}", APPLE_API_KEY_ID="${api_key_id}", APPLE_API_ISSUER="${api_issuer}", APPLE_ID="${apple_id}", APPLE_PASSWORD="${apple_password}") in the main heredoc and in the conditional appended heredocs so sourcing the generated .env.signing is safe under set -euo pipefail.apps/desktop/src/lib/trpc/routers/projects/projects.ts-1258-1297 (1)
1258-1297:⚠️ Potential issue | 🟠 MajorWrap the project + workspace inserts in a transaction to prevent orphaned records.
The project row is inserted first; if the process crashes or an exception is thrown before the workspace insert completes, a project without a workspace is left in the database. On the next
createRemotecall the dedup loop finds the project but no active workspace and — before the fix proposed above — inserts yet another project row. Drizzle's better-sqlite3 adapter supports synchronous transactions directly.🔒 Proposed fix
-// Create new remote project -const project = localDb - .insert(projects) - .values({ - mainRepoPath: input.remotePath, - name: projectName, - color: getDefaultProjectColor(), - defaultBranch: "main", - projectType: "remote", - }) - .returning() - .get(); - -// Create remote workspace -const maxTabOrder = getMaxWorkspaceTabOrder(project.id); -const workspaceName = `${sshConn.name}:${input.remotePath.split("/").pop() || "remote"}`; - -const workspace = localDb - .insert(workspaces) - .values({ - projectId: project.id, - type: "remote", - branch: "remote", - name: workspaceName, - sshConnectionId: input.sshConnectionId, - remotePath: input.remotePath, - tabOrder: maxTabOrder + 1, - }) - .returning() - .get(); +const workspaceName = `${sshConn.name}:${input.remotePath.split("/").pop() || "remote"}`; + +const { project, workspace } = localDb.transaction((tx) => { + const p = + orphanedProject ?? + tx + .insert(projects) + .values({ + mainRepoPath: input.remotePath, + name: projectName, + color: getDefaultProjectColor(), + defaultBranch: "main", + projectType: "remote", + }) + .returning() + .get(); + + const maxTabOrder = getMaxWorkspaceTabOrder(p.id); + const w = tx + .insert(workspaces) + .values({ + projectId: p.id, + type: "remote", + branch: "remote", + name: workspaceName, + sshConnectionId: input.sshConnectionId, + remotePath: input.remotePath, + tabOrder: maxTabOrder + 1, + }) + .returning() + .get(); + + return { project: p, workspace: w }; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 1258 - 1297, Wrap the project and workspace insert sequence in a synchronous Drizzle transaction so both inserts succeed or both roll back; specifically, use the localDb transaction API to execute the block that calls localDb.insert(projects).values(...).returning().get() and then localDb.insert(workspaces).values(...).returning().get(), and only after the transaction completes call setLastActiveWorkspace(workspace.id), activateProject(project), and track(...); ensure the transaction returns both created rows (project and workspace) so you can use their ids after commit and avoid leaving orphaned project rows if the workspace insert fails.apps/desktop/src/lib/trpc/routers/projects/projects.ts-1219-1252 (1)
1219-1252:⚠️ Potential issue | 🟠 Major
createRemoteproduces duplicate projects after close + reopen.The
closeprocedure hard-deletes workspace rows while leaving the project record (settingtabOrder: null). WhencreateRemoteis called again for the same path and SSH connection, the outer loop finds the preserved project but finds no active workspace (existingWorkspaceis null because the row was deleted), so it falls through and inserts a new project row — creating a duplicate entry in the sidebar.The dedup loop must remember whether a project was found (even without an active workspace) and reuse it instead of inserting a new one:
🐛 Proposed fix
+let orphanedProject: (typeof allRemoteProjects)[0] | null = null; + for (const existingProject of allRemoteProjects) { const existingWorkspace = localDb .select() .from(workspaces) .where( and( eq(workspaces.projectId, existingProject.id), eq(workspaces.type, "remote"), eq(workspaces.sshConnectionId, input.sshConnectionId), isNull(workspaces.deletingAt), ), ) .get(); if (existingWorkspace) { touchWorkspace(existingWorkspace.id); setLastActiveWorkspace(existingWorkspace.id); activateProject(existingProject); return { project: existingProject, workspace: existingWorkspace }; } + + // Track the first project match without an active workspace for reuse below + if (!orphanedProject) orphanedProject = existingProject; } // Create new remote project (or reuse project whose workspace was deleted on close) const projectName = input.name || input.remotePath.split("/").pop() || "remote-project"; -const project = localDb - .insert(projects) - .values({ - mainRepoPath: input.remotePath, - name: projectName, - color: getDefaultProjectColor(), - defaultBranch: "main", - projectType: "remote", - }) - .returning() - .get(); +const project = + orphanedProject ?? + localDb + .insert(projects) + .values({ + mainRepoPath: input.remotePath, + name: projectName, + color: getDefaultProjectColor(), + defaultBranch: "main", + projectType: "remote", + }) + .returning() + .get();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 1219 - 1252, The dedup loop in createRemote currently only reuses an existing project when an active workspace exists, causing duplicate project rows when workspaces were hard-deleted; change the logic to record the found existingProject even if existingWorkspace is null and reuse that project instead of inserting a new one: inside the loop over allRemoteProjects capture the first matching existingProject (e.g., foundProject variable) and if existingWorkspace is null, create or rehydrate a new workspace tied to existingProject (or set its tabOrder/reactivate it) rather than creating a fresh project row; keep using helper functions touchWorkspace, setLastActiveWorkspace, and activateProject to finish activation for the reused project.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useClipboardImagePaste.ts-34-36 (1)
34-36:⚠️ Potential issue | 🟠 Major
quoteForShellInputonly escapes"and\—$and backticks are still expanded inside double quotes.If
remotePathever contains a$or backtick character, the shell will perform variable expansion or command substitution. Consider using single-quote escaping instead, which is immune to expansion:🔧 Suggested fix
-function quoteForShellInput(path: string): string { - return `"${path.replace(/(["\\])/g, "\\$1")}"`; -} +function quoteForShellInput(path: string): string { + return `'${path.replace(/'/g, "'\\''")}'`; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useClipboardImagePaste.ts` around lines 34 - 36, quoteForShellInput currently only escapes double quotes and backslashes, leaving $ and backticks vulnerable to shell expansion; update the function quoteForShellInput to use single-quote wrapping and escape internal single quotes (replace each ' with '\'' sequence) so the returned string is safe from variable expansion and command substitution when interpolated into shell commands, and adjust any callers (e.g., places that pass remotePath) to use the new behavior.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/sftp-service.ts-31-38 (1)
31-38:⚠️ Potential issue | 🟠 MajorCache the SFTP session in
SSHConnectioninstead of creating a new subsystem channel per call.Each method calls
this.connection.getSftp(), which does not cache the SFTP wrapper. ThegetSftp()method (line 162-170 inconnection.ts) invokesclient.sftp()directly on every call, creating a new SFTP subsystem channel for each operation. This adds unnecessary latency and resource overhead. Add a private SFTP cache field toSSHConnectionand reuse the existing session across multiple calls, releasing it only on connection closure or explicit disconnect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/sftp-service.ts` around lines 31 - 38, The SFTP methods (e.g., readFile) call this.connection.getSftp() which currently creates a new subsystem channel each time; update SSHConnection by adding a private cached SFTP field (e.g., private _sftp?: SFTPWrapper) and modify getSftp() to return the cached instance if present, creating and storing it on first call and only releasing it on explicit disconnect/close; ensure the cache is cleared when the connection closes (or on error) so subsequent calls recreate it, and keep method callers like sftp-service.ts/readFile unchanged except they will now reuse the cached SFTP session.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/ssh-key-resolver.ts-76-78 (1)
76-78: 🛠️ Refactor suggestion | 🟠 MajorUnnecessary dynamic
require("node:fs")—fsis already imported at the top.
existsSyncandreadFileSyncare imported from"node:fs"on line 12, butreaddirSync(line 77) andstatSync(line 94) use inlinerequire("node:fs")instead. Add them to the top-level import.♻️ Proposed fix
-import { existsSync, readFileSync } from "node:fs"; +import { existsSync, readFileSync, readdirSync, statSync } from "node:fs";Then replace the
requirecalls:- const { readdirSync } = require("node:fs"); - const entries = readdirSync(sshDir) as string[]; + const entries = readdirSync(sshDir, "utf-8");- const { statSync } = require("node:fs"); - const stat = statSync(fullPath); + const stat = statSync(fullPath);Also applies to: 93-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/ssh-key-resolver.ts` around lines 76 - 78, The file uses dynamic require("node:fs") for readdirSync and statSync inside ssh-key-resolver.ts even though existsSync and readFileSync are imported at the top; update the top-level import to include readdirSync and statSync, then replace the inline require(...) usages (the local readdirSync and statSync calls around the try block and lines ~93-95) to use the imported readdirSync and statSync identifiers instead and remove the dynamic require calls.apps/desktop/src/main/lib/workspace-runtime/registry.ts-141-184 (1)
141-184:⚠️ Potential issue | 🟠 MajorStale runtime reference returned to caller after async replacement.
getOrCreateRemoteRuntimereturns the runtime synchronously, thenensureRemoteRuntimeHealthyfires-and-forgets the connection. If the connection fails, the handler evicts and replaces the runtime in the cache (line 164), but the caller already holds a reference to the now-disposed runtime. Subsequent operations on that reference will fail silently or throw.This is a latent race: the first
getForWorkspaceIdcall returns runtime A, the background health check disposes A and inserts runtime B, but the caller still uses A.Consider either:
- Making
getForWorkspaceIdasync so it can await connection establishment before returning, or- Using a proxy/indirection layer so the cached entry can swap the underlying connection transparently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts` around lines 141 - 184, The problem is callers get a synchronous RemoteSSHWorkspaceRuntime (via getOrCreateRemoteRuntime) that can be evicted/replaced asynchronously by ensureRemoteRuntimeHealthy, leaving them holding a stale/ disposed instance; change getOrCreateRemoteRuntime to be async and await the runtime’s connection before returning: after creating or fetching the entry (touchRemoteRuntime / this.remoteRuntimes.get), call await runtime.ensureConnected() (or await replacement.ensureConnected() for newly created ones) and only then return the runtime; keep ensureRemoteRuntimeHealthy as a background watchdog but make it check current entry identity (this.remoteRuntimes.get(key)) before evicting, and update all callers of getOrCreateRemoteRuntime to await the new async signature so they never receive a disposed runtime.apps/desktop/src/main/lib/workspace-runtime/registry.ts-95-97 (1)
95-97: 🛠️ Refactor suggestion | 🟠 MajorBare
catch {}silently swallows all DB errors.This catches everything — schema mismatches, corrupted DB, permission errors — and falls through to the default local runtime with no logging. At minimum, log the error so operational issues are diagnosable.
🔧 Proposed fix
- } catch { - // Database not ready or workspace not found — fall through to default + } catch (err) { + console.warn("[Registry] Failed to resolve workspace runtime, falling back to local:", err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts` around lines 95 - 97, The bare catch block that swallows DB errors when attempting to load the workspace runtime should be changed to capture the error and log it before falling back to the default local runtime; replace `catch { ... }` with `catch (err) { /* use the existing logger (e.g. processLogger) or console.error */ processLogger?.error("Failed to read workspace runtime from DB, falling back to default runtime", err); }` so operational issues (schema, permission, corruption) are visible while keeping the existing fallback behavior.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection-pool.ts-30-44 (1)
30-44:⚠️ Potential issue | 🟠 MajorRef count leak when reconnection of an existing pooled connection fails.
If
existing.connection.connect()(line 40) throws, the refCount has already been incremented (line 32) but the error propagates without decrementing it. The caller gets an error, never callsrelease(), and the refCount is permanently inflated — the idle timer will never fire for this entry.🔧 Proposed fix: decrement on failure
if (existing) { existing.refCount++; if (existing.idleTimer) { clearTimeout(existing.idleTimer); existing.idleTimer = null; } // Reconnect if needed if (!existing.connection.isConnected) { - await existing.connection.connect(); + try { + await existing.connection.connect(); + } catch (err) { + existing.refCount = Math.max(0, existing.refCount - 1); + throw err; + } } return existing.connection; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection-pool.ts` around lines 30 - 44, The pool increments existing.refCount before attempting reconnect, but if existing.connection.connect() throws the refCount is never decremented; wrap the reconnect in a try/catch so that on any exception you decrement existing.refCount (and if it reaches 0 re-establish the idle cleanup timer the same way release() would) and then rethrow the error; alternatively increment refCount only after connect succeeds. Update the code around pool.get(key) / existing.refCount / existing.idleTimer and the reconnect call to ensure refCount is restored and idleTimer restored/started on failure.apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts-24-25 (1)
24-25:⚠️ Potential issue | 🟠 MajorRemove unused
_globalPool— dead code that contradicts design intent.
_globalPoolis instantiated at module scope (line 25) but never referenced anywhere in the file. The constructor creates a standaloneSSHConnectiondirectly (line 41) rather than requesting a connection from the pool. This contradicts the documented intent of using a "shared connection pool" and leavesSSHConnectionPoolunnecessarily imported and instantiated.Remove the unused pool and its import from line 15.
🔧 Proposed fix
-/** Shared connection pool across all RemoteSSHWorkspaceRuntime instances */ -const _globalPool = new SSHConnectionPool(); -And remove
SSHConnectionPoolfrom the import on line 15.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts` around lines 24 - 25, Remove the unused module-scope pool: delete the `_globalPool` instantiation and remove the `SSHConnectionPool` import so the file no longer creates an unused shared pool; update the `RemoteSSHWorkspaceRuntime` constructor (which currently instantiates `SSHConnection` directly) to remain unchanged (no pool usage) or, if pool behavior is desired, replace the direct `new SSHConnection(...)` call in the constructor with a call to the pool API — otherwise remove the `_globalPool` symbol and its import to eliminate dead code and the misleading "shared connection pool" comment.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts-279-281 (1)
279-281:⚠️ Potential issue | 🟠 Major
detachAllListenersalso removes theconnected/disconnectedhandlers wired in the constructor.
removeAllListeners()at line 280 strips the connection event handlers registered at lines 60–68, breaking reconnection-driven reattach and disconnect notification for the lifetime of this runtime instance.If this method is called for renderer detach (not full teardown), the runtime silently stops responding to SSH reconnection events.
Sketch: scope removal to pane-specific events only
detachAllListeners(): void { - this.removeAllListeners(); + // Only remove pane-specific event listeners, preserve connection handlers + for (const eventName of this.eventNames()) { + const name = String(eventName); + if (name.includes(':')) { + this.removeAllListeners(eventName); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts` around lines 279 - 281, detachAllListeners currently calls removeAllListeners which also removes the connection handlers registered in the constructor (the 'connected'/'disconnected' listeners) and breaks reconnection; change detachAllListeners so it only removes pane-specific listeners instead of all listeners — either remove specific event names (call removeListener/off for pane-related events) or track and remove non-connection handler references while preserving the constructor's 'connected' and 'disconnected' handlers; use the existing constructor handler names and the detachAllListeners function name to locate and update the logic.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/session.ts-252-269 (1)
252-269:⚠️ Potential issue | 🟠 MajorMissing
errorevent handler on the channel.
wireChannelhandlesdataandcloseevents but does not listen forerroron the channel. An unhandlederrorevent on a Node.js stream will crash the process (or at minimum trigger anuncaughtException). Theconnection.tsexecmethod does handle stream errors (line 171); this should too.Proposed fix
this.channel.stderr?.on("data", (data: Buffer) => { this.callbacks.onData(data.toString()); }); + + this.channel.on("error", (err: Error) => { + this._isAlive = false; + this.callbacks.onExit(1); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/session.ts` around lines 252 - 269, The wireChannel method is missing an "error" handler on this.channel which can cause unhandled exceptions; add this.channel.on("error", (err: Error) => { this._isAlive = false; this._isDetached = false; if (typeof this.callbacks.onError === "function") this.callbacks.onError(err); else this.callbacks.onExit?.(1); /* optionally log or pass err info */ }); to mirror how exec handles stream errors—ensure the handler references wireChannel, this.channel, _isAlive/_isDetached state, and uses callbacks.onError when available (fallback to callbacks.onExit with a non-zero code) so errors are propagated instead of crashing the process.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts-71-85 (1)
71-85:⚠️ Potential issue | 🟠 Major
initTmuxCapabilityis fire-and-forget —createOrAttachcan race ahead and create raw-shell sessions even when tmux is available.
initTmuxCapabilityis launched withvoidin the constructor (line 72), sotmuxAvailableremainsfalseuntil the async check completes. IfcreateOrAttachis called before then, sessions are created in raw-shell mode and won't benefit from tmux persistence/recovery. This is especially likely on first mount when registry warmup and session creation happen concurrently.Consider awaiting tmux detection lazily at the start of
doCreateOrAttach, or exposing areadypromise.Sketch: lazy-init with cached promise
+ private tmuxInitPromise: Promise<void> | null = null; constructor(connection: SSHConnection) { ... - void this.initTmuxCapability(); + this.tmuxInitPromise = this.initTmuxCapability(); } private async doCreateOrAttach(params: CreateSessionParams): Promise<SessionResult> { ... if (!this.connection.isConnected) { await this.connection.connect(); } + // Ensure tmux detection has completed before deciding session mode + if (this.tmuxInitPromise) { + await this.tmuxInitPromise; + this.tmuxInitPromise = null; + } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts` around lines 71 - 85, initTmuxCapability is currently fire-and-forget, causing createOrAttach/doCreateOrAttach to race and create raw-shell sessions before tmux detection finishes; fix by making the tmux check cancellable/cached and awaited: have the constructor set a cached promise (e.g., this.tmuxInitPromise = this.initTmuxCapability()) or make initTmuxCapability return a cached-ready promise, then at the start of doCreateOrAttach (or createOrAttach) await that promise (if not already resolved) before deciding on persistent/coldRestore behavior so tmuxAvailable is determined before session creation; ensure you still handle errors from isTmuxAvailable and preserve the existing fallback to raw-shell mode.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/session.ts-93-107 (1)
93-107:⚠️ Potential issue | 🟠 MajorEscape
sessionNamein shell writes to prevent potential command injection.Lines 97, 117, and 246 write
tmux attach-session -t ${sessionName}directly to the shell without escaping. WhilesessionNameis currently derived from internal identifiers,paneIdhas no documented format constraints. IfpaneIdever contains shell metacharacters (spaces, semicolons, backticks, etc.), this becomes an injection vector. Apply the defensive fix already established in this file for similar variables:Proposed fix
-this.channel.write(`tmux attach-session -t ${sessionName}\n`); +this.channel.write(`tmux attach-session -t ${escapeShellArg(sessionName)}\n`);Apply at lines 97, 117, and 246.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/session.ts` around lines 93 - 107, The tmux attach command writes sessionName directly into shell input and can be abused via shell metacharacters; update every this.channel.write that sends `tmux attach-session -t ${sessionName}` (the occurrences that also use paneId elsewhere) to pass sessionName through the file's existing shell-escaping helper (use the same escape/shellEscape utility used elsewhere in this module) before interpolation so the value is safely quoted/escaped; ensure you replace the raw template interpolation in all three write sites (the ones that write the attach-session command) with the escaped value and keep the trailing newline as before.apps/desktop/src/renderer/index.html-12-12 (2)
12-12:⚠️ Potential issue | 🟠 MajorRemove
'unsafe-eval'from CSP—it appears unnecessary and is a security risk.The comment justifies
'unsafe-eval'as "needed by dependencies," but the evidence doesn't support this:
- xterm ImageAddon (which is actively used) requires only
'wasm-unsafe-eval'for WebAssembly compilation, not'unsafe-eval'for JS evaluation.- Monaco Editor (0.55.1, actively used) does not inherently require
'unsafe-eval'. The codebase already uses the modern ESM approach with proper worker imports (editorWorker,cssWorker,htmlWorker, etc.), which works fine without'unsafe-eval'.- No
eval()calls found in the renderer source code.- Vite (not webpack eval-based devtool) is the bundler, so development source maps don't introduce eval usage.
The presence of both
'unsafe-eval'and'wasm-unsafe-eval'is redundant—'wasm-unsafe-eval'alone covers the ImageAddon's actual requirement. Remove'unsafe-eval'and update the comment to clarify that only'wasm-unsafe-eval'is needed for xterm ImageAddon's WebAssembly component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/index.html` at line 12, Remove the 'unsafe-eval' token from the Content-Security-Policy script-src directive in the index.html fragment that currently reads "script-src 'self' 'unsafe-eval' 'wasm-unsafe-eval' https://*.posthog.com"; keep 'wasm-unsafe-eval' (and 'self' and PostHog) to satisfy xterm ImageAddon's WebAssembly requirement, update the inline comment that justifies the CSP to state that only 'wasm-unsafe-eval' is required for the ImageAddon and that Monaco/Esm worker imports do not require 'unsafe-eval', and run a quick runtime check of the renderer to confirm no functionality breaks after removal.
12-12:⚠️ Potential issue | 🟠 Major
'unsafe-eval'in CSP is a significant security regression — identify and remove it.Adding
'unsafe-eval'toscript-srcallows arbitrary JavaScript strings to be executed viaeval(),new Function(),setTimeout(string), etc., substantially weakening Content Security Policy. This is distinct from and far broader than the already-present'wasm-unsafe-eval', which safely permits WebAssembly compilation without enabling JSeval.Key concerns:
'wasm-unsafe-eval'is already sufficient. The comment cites xterm ImageAddon, which requires WASM (SIXEL decoder). WebAssembly compilation in CSP requires only'wasm-unsafe-eval', not'unsafe-eval'. These are two separate directives;'wasm-unsafe-eval'does not grant JSeval, so there is no reason to add'unsafe-eval'for that feature.Vague and unauditable justification. The comment says "needed by dependencies" without naming which dependency, version, or whether a CSP-compatible alternative exists.
Most likely culprit is build tooling, not a runtime library. If this error appeared during development, it was likely triggered by a bundler configured to use eval-based sourcemaps (e.g., Webpack
eval-source-mapor Vite withbuild.sourcemap: true). Runtime libraries likessh2are Node.js-only and should never run in a browser renderer; if SSH functionality is needed, it belongs in a Node backend/proxy that communicates with the renderer over WebSocket.Elevated risk surface in this PR. SSH terminal output, remote file contents, and clipboard data from remote machines are attacker-influenced inputs that now flow into a renderer where
evalis permitted, raising the XSS-to-RCE escalation risk in an Electron context.Recommended path:
- Run a development build with
--inspect-brkor check browser console at startup to identify which script actually callseval()orFunction().- If it's a bundler/sourcemap artifact, reconfigure the build tool: Webpack → use
devtool: 'source-map'(non-eval) instead ofeval-source-map; Vite → usebuild.sourcemap: truewithbuild.minify: 'esbuild'or similar non-eval options.- If it's a runtime library, check whether a CSP-compatible build flag or configuration exists.
- Remove
'unsafe-eval'once the underlying cause is fixed.Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/index.html` at line 12, Remove the explicit "'unsafe-eval'" token from the Content-Security-Policy script-src entry in apps/desktop/src/renderer/index.html (leave 'wasm-unsafe-eval' if needed for xterm/ImageAddon), then root-cause the source of eval usage: run a dev build and check the console to find which script is calling eval/Function (likely a bundler sourcemap like Webpack's eval-source-map or Vite config), switch the bundler to a non-eval sourcemap option (e.g., Webpack devtool: 'source-map' or Vite build.sourcemap: true without eval), and only re-add CSP relaxations if a verified runtime dependency absolutely requires them and cannot be fixed.apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts-944-980 (1)
944-980:⚠️ Potential issue | 🟠 Major
createRemoteWorkspacelacks an idempotency check — duplicate workspaces are created on repeated calls.Every other procedure in this file guards against duplicates (
onConflictDoNothing, or an explicitSELECTbefore insert).createRemoteWorkspaceinserts unconditionally, so a double-click, network retry, or re-invocation produces multiple identical workspace rows pointing to the same SSH connection and remote path.🛡️ Proposed fix — check for an existing workspace first
+import { and, eq, isNull, not } from "drizzle-orm"; .mutation(async ({ input }) => { // ... project + sshConn validation ... + // Guard against duplicates + const existingWorkspace = localDb + .select() + .from(workspaces) + .where( + and( + eq(workspaces.projectId, input.projectId), + eq(workspaces.sshConnectionId, input.sshConnectionId), + eq(workspaces.remotePath, input.remotePath), + isNull(workspaces.deletingAt), + ), + ) + .get(); + + if (existingWorkspace) { + touchWorkspace(existingWorkspace.id); + setLastActiveWorkspace(existingWorkspace.id); + activateProject(project); + return { + workspace: existingWorkspace, + initialCommands: null, + worktreePath: input.remotePath, + projectId: project.id, + isInitializing: false, + wasExisting: true, + }; + } + const maxTabOrder = getMaxWorkspaceTabOrder(input.projectId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 944 - 980, The mutation unconditionally inserts a new workspaces row causing duplicates; before inserting (in the .mutation handler that references localDb, workspaces, input.projectId, input.sshConnectionId, and input.remotePath) check for an existing workspace matching projectId, sshConnectionId and remotePath and return that record if found, or alternatively perform the insert with an onConflictDoNothing (or equivalent) and then SELECT the row to return; ensure tabOrder calculation via getMaxWorkspaceTabOrder only runs when creating a new row and that the function returns the existing workspace instead of creating a duplicate.packages/local-db/drizzle/meta/0027_snapshot.json-1-6 (1)
1-6:⚠️ Potential issue | 🟠 MajorBoth
0027_snapshot.jsonand0028_snapshot.jsonshare identicalidandprevId— Drizzle migration chain is broken.
0027_snapshot.jsonhas"id": "f503ac09-..."and so does0028_snapshot.json. Drizzle uses snapshot UUIDs to establish and validate the migration chain:
0028_snapshot.jsonmust have a new uniqueidand itsprevIdmust equal0027_snapshot.json'sid.- The two files appear to be identical copies of the final schema (both already contain
ssh_connections.connection_status,ssh_connections.updated_at,workspaces.ssh_connection_id,workspaces.remote_path), whereas0027_snapshot.jsonshould reflect the schema state only after migration 0027 — i.e., without the columns added by migration 0028.This breaks
drizzle-kit generate, schema drift detection, and any future migration tooling that validates the snapshot chain.Action required: Regenerate both snapshots via
drizzle-kit generaterather than manually authoring them. Based on learnings, snapshot files underpackages/local-db/drizzle/meta/are auto-generated by Drizzle and must never be manually edited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/drizzle/meta/0027_snapshot.json` around lines 1 - 6, The two snapshot files 0027_snapshot.json and 0028_snapshot.json share identical "id"/"prevId" and 0027 contains schema changes from 0028, breaking the Drizzle migration chain; fix it by deleting the manually edited snapshots and re-generating correct snapshots with the Drizzle tool (run drizzle-kit generate) so 0027_snapshot.json reflects the schema state immediately after migration 0027 (i.e., without columns added by 0028), 0028_snapshot.json gets a new unique "id" and its "prevId" equals 0027's id, and commit only the auto-generated files (do not manually edit 0027_snapshot.json or 0028_snapshot.json).apps/desktop/src/lib/trpc/routers/changes/security/path-validation.ts-100-124 (1)
100-124:⚠️ Potential issue | 🟠 MajorBare
catch {}silently swallows DB errors, creating a security bypass path.If
assertRegisteredWorktreethrows a runtime exception (e.g., abetter-sqlite3error on DB failure), the catch discards it and execution falls through to the remote workspace check. If any row inworkspaceshappens to haveremotePathequal to the attacker-supplied path ANDtype === "remote", the function returns without error — effectively authorizing an unregistered local path as a remote workspace. This directly undermines the security boundary documented in this file's own threat model.Only
PathValidationError(the expected "not found" signal) should be caught; all other errors should propagate.🔒 Proposed fix
- try { - assertRegisteredWorktree(workspacePath); - return; - } catch { - // Not a local worktree/project — check remote workspaces - } + try { + assertRegisteredWorktree(workspacePath); + return; + } catch (err) { + if (!(err instanceof PathValidationError)) { + throw err; + } + // Not a local worktree/project — check remote workspaces + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/security/path-validation.ts` around lines 100 - 124, The current catch in assertRegisteredWorkspacePath silently swallows all exceptions from assertRegisteredWorktree, allowing DB/runtime errors to bypass local checks; change the try/catch so it only catches PathValidationError thrown by assertRegisteredWorktree and rethrows any other exception. Specifically, inside assertRegisteredWorkspacePath, replace the bare catch with a catch that inspects the caught error (or uses instanceof) and continues only when it is a PathValidationError; for any other error, rethrow so failures from the database layer (e.g., better-sqlite3) propagate instead of falling through to the remote workspaces lookup (localDb / workspaces).
| async worktreeAdd( | ||
| repoPath: string, | ||
| worktreePath: string, | ||
| branch: string, | ||
| baseBranch?: string, | ||
| ): Promise<void> { | ||
| const args = baseBranch | ||
| ? `git worktree add -b ${branch} ${escapeArg(worktreePath)} ${baseBranch}` | ||
| : `git worktree add ${escapeArg(worktreePath)} ${branch}`; | ||
|
|
||
| const result = await this.connection.exec(args, repoPath); | ||
| if (result.code !== 0) { | ||
| throw new Error(`git worktree add failed: ${result.stderr}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Command injection: branch and baseBranch are not escaped.
escapeArg is applied to worktreePath but branch and baseBranch are interpolated raw into the shell command string. A crafted branch name (e.g., containing ; rm -rf / or backticks) would execute arbitrary commands on the remote host via SSH.
🔒 Proposed fix: escape all user-supplied arguments
async worktreeAdd(
repoPath: string,
worktreePath: string,
branch: string,
baseBranch?: string,
): Promise<void> {
const args = baseBranch
- ? `git worktree add -b ${branch} ${escapeArg(worktreePath)} ${baseBranch}`
- : `git worktree add ${escapeArg(worktreePath)} ${branch}`;
+ ? `git worktree add -b ${escapeArg(branch)} ${escapeArg(worktreePath)} ${escapeArg(baseBranch)}`
+ : `git worktree add ${escapeArg(worktreePath)} ${escapeArg(branch)}`;
const result = await this.connection.exec(args, repoPath);📝 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 worktreeAdd( | |
| repoPath: string, | |
| worktreePath: string, | |
| branch: string, | |
| baseBranch?: string, | |
| ): Promise<void> { | |
| const args = baseBranch | |
| ? `git worktree add -b ${branch} ${escapeArg(worktreePath)} ${baseBranch}` | |
| : `git worktree add ${escapeArg(worktreePath)} ${branch}`; | |
| const result = await this.connection.exec(args, repoPath); | |
| if (result.code !== 0) { | |
| throw new Error(`git worktree add failed: ${result.stderr}`); | |
| } | |
| } | |
| async worktreeAdd( | |
| repoPath: string, | |
| worktreePath: string, | |
| branch: string, | |
| baseBranch?: string, | |
| ): Promise<void> { | |
| const args = baseBranch | |
| ? `git worktree add -b ${escapeArg(branch)} ${escapeArg(worktreePath)} ${escapeArg(baseBranch)}` | |
| : `git worktree add ${escapeArg(worktreePath)} ${escapeArg(branch)}`; | |
| const result = await this.connection.exec(args, repoPath); | |
| if (result.code !== 0) { | |
| throw new Error(`git worktree add failed: ${result.stderr}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/git-service.ts` around
lines 63 - 77, The worktreeAdd function builds a shell command by interpolating
branch and baseBranch raw, creating a command injection risk; update worktreeAdd
to escape branch and baseBranch using the existing escapeArg utility (same as
used for worktreePath) or otherwise construct the exec invocation with a safe
argv array so that branch and baseBranch are properly quoted/escaped before
calling this.connection.exec; ensure the conditional that builds args uses
escapeArg(branch) and escapeArg(baseBranch) (and keep escapeArg(worktreePath))
so no user-supplied value is injected unescaped.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx (1)
617-732: Significant duplication betweenRemoteWorkspaceFormandSshRemoteProjectForm.These two components share ~80% of their structure: the SSH connection
Select, the remote-pathInput, the optional name field, the empty-connections state, and the create button with spinner. Consider extracting the shared SSH-connection + remote-path form fields into a reusable component and passing in only the differing parts (mutation, labels, back button).Sketch of a shared component
// Shared form fields component function SshConnectionFields({ sshConnectionId, onSshConnectionChange, remotePath, onRemotePathChange, connections, pathLabel?: string, }: { /* ... */ }) { return ( <> <div className="space-y-1.5"> <Label className="text-xs text-muted-foreground">SSH Connection</Label> <Select value={sshConnectionId} onValueChange={onSshConnectionChange}> <SelectTrigger className="h-9 text-sm"> <SelectValue placeholder="Select connection..." /> </SelectTrigger> <SelectContent> {connections.map((conn) => ( <SelectItem key={conn.id} value={conn.id}> {conn.name} ({conn.username}@{conn.host}) </SelectItem> ))} </SelectContent> </Select> </div> <div className="space-y-1.5"> <Label className="text-xs text-muted-foreground">{pathLabel ?? "Remote Path"}</Label> <Input className="h-9 text-sm font-mono" placeholder="/home/user/project" value={remotePath} onChange={(e) => onRemotePathChange(e.target.value)} /> </div> </> ); }Also applies to: 734-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx` around lines 617 - 732, The RemoteWorkspaceForm and SshRemoteProjectForm duplicate the SSH connection select, remote-path input, optional name input, empty-connections UI, and create button; extract those shared bits into a new component (e.g., SshConnectionFields) that accepts props like sshConnectionId, onSshConnectionChange, remotePath, onRemotePathChange, connections, pathLabel and a render prop or callback for the action button; update RemoteWorkspaceForm (references: RemoteWorkspaceForm, createRemote, handleCreate, createRemote.isPending) and SshRemoteProjectForm to use SshConnectionFields, move the “no connections” UI into the shared component or surface it via a prop, and keep only the differing mutation logic, labels and navigation/back behavior in each parent component.apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts (1)
34-34:connectionis typed| nullbut is never actuallynullafter construction.The field is initialized to
nullat declaration, then immediately set tonew SSHConnection(config)in the constructor.dispose()doesn't null it, so the?.on line 79 is always a no-op. Simplify the declaration toprivate connection: SSHConnectionand remove the optional chain.♻️ Proposed refactor
- private connection: SSHConnection | null = null; + private connection!: SSHConnection;- this.connection?.disconnect(); + this.connection.disconnect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts` at line 34, Change the field declaration from a nullable SSHConnection to a non-nullable one: replace "private connection: SSHConnection | null = null" with "private connection: SSHConnection" and initialize it in the constructor where "new SSHConnection(config)" is already assigned; remove any optional chaining usages for "connection" (e.g., "connection?...." calls) and use direct access instead; ensure "dispose()" behavior remains the same (do not set connection to null) or, if you need to allow reinitialization, explicitly set it to null and keep the nullable type consistently across constructor and dispose; update all references to the "connection" field and any guards accordingly.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts (2)
75-84: Mutatingreadonly capabilitiesvia type-cast is a TypeScript anti-pattern.If
TerminalCapabilities.persistentis typedreadonly, the casts work around the type system rather than respecting it. Prefer initializingcapabilitiesas a plain mutable object internally and only exposing it through a getter or accepting that capabilities are not stable until the async init resolves.♻️ Proposed refactor
- readonly capabilities: TerminalCapabilities; + readonly capabilities: TerminalCapabilities = { + persistent: false, + coldRestore: false, + }; constructor(connection: SSHConnection) { // ... - // Will be updated after tmux availability check - this.capabilities = { - persistent: false, - coldRestore: false, - };private async initTmuxCapability(): Promise<void> { try { this.tmuxAvailable = await this.tmuxManager.isTmuxAvailable(); if (this.tmuxAvailable) { - (this.capabilities as { persistent: boolean }).persistent = true; - (this.capabilities as { coldRestore: boolean }).coldRestore = true; + // Cast needed to bypass readonly at initialization time + (this.capabilities as TerminalCapabilities & Record<string, boolean>).persistent = true; + (this.capabilities as TerminalCapabilities & Record<string, boolean>).coldRestore = true; }Or, more cleanly, declare the backing object as mutable and assign to
readonly capabilities:+ private _capabilities: { persistent: boolean; coldRestore: boolean } = { + persistent: false, + coldRestore: false, + }; + get capabilities(): TerminalCapabilities { return this._capabilities; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts` around lines 75 - 84, The code is mutating a readonly TerminalCapabilities via a type cast in initTmuxCapability; instead, make the backing capabilities mutable and update that, or expose a readonly view and leave the public property immutable: add a private mutable field (e.g., private _capabilities: MutableTerminalCapabilities) and replace direct casts/assignments in initTmuxCapability with updates to _capabilities, then have public get capabilities(): TerminalCapabilities return the readonly view (or a frozen/readonly copy); update references to use _capabilities when initializing tmuxAvailable from tmuxManager.isTmuxAvailable and setting persistent/coldRestore so you no longer bypass the type system with casts.
40-40:setMaxListeners(100)suppresses the leak warning rather than bounding the actual listener count.Each terminal pane subscription adds a
data:${paneId},exit:${paneId}, anddisconnect:${paneId}listener. Verify that subscriptions are properly removed when a pane is detached/killed; if so, the ceiling is the number of simultaneously open panes, which is likely well under 10. If pane listeners are not cleaned up, the higher limit hides a real leak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts` at line 40, The call to this.setMaxListeners(100) merely hides a potential leak — locate where the terminal pane event handlers are added (the code that registers "data:${paneId}", "exit:${paneId}", and "disconnect:${paneId}" listeners inside the TerminalRuntime/terminal-runtime class) and ensure those listeners are removed when a pane is detached or killed (use removeListener/off or unsubscribe in the pane detach/kill handler such as the pane teardown/cleanup path). After fixing listener cleanup, reduce the max listener ceiling to a sensible value (e.g., 10) or remove the setMaxListeners call entirely so real leaks surface; keep references to the exact symbols "data:${paneId}", "exit:${paneId}", "disconnect:${paneId}", and the pane detach/kill cleanup method to guide where to add removal logic.apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts (1)
21-29:SSHConnectionEventsis defined but provides no compile-time type safety.The interface documents the event signatures but isn't wired to
SSHConnection.on/emit. Callers (e.g.,terminal-runtime.ts) get no type checking on event payloads.♻️ Proposed refactor — declaration merging for typed events
+// Typed overloads via declaration merging +export declare interface SSHConnection { + on(event: "connected", listener: () => void): this; + on(event: "disconnected", listener: (error?: Error) => void): this; + on(event: "reconnecting", listener: (attempt: number) => void): this; + on(event: "error", listener: (error: Error) => void): this; + on(event: "stateChange", listener: (state: SSHConnectionState) => void): this; + emit(event: "connected"): boolean; + emit(event: "disconnected", error?: Error): boolean; + emit(event: "reconnecting", attempt: number): boolean; + emit(event: "error", error: Error): boolean; + emit(event: "stateChange", state: SSHConnectionState): boolean; +} + export class SSHConnection extends EventEmitter {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts` around lines 21 - 29, The SSHConnectionEvents interface isn't wired to the SSHConnection EventEmitter, so callers get no type safety; update the SSHConnection class to declare typed overloads for EventEmitter methods (on, once, addListener, off/removeListener, emit) using the SSHConnectionEvents keys and tuple payloads (e.g., on<K extends keyof SSHConnectionEvents>(event: K, listener: (...args: SSHConnectionEvents[K]) => void): this) so TypeScript checks event names and payload shapes when using SSHConnection.on/emit/etc.; keep the SSHConnectionEvents interface as the single source of truth and implement these overloads on the SSHConnection class to provide compile-time safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts`:
- Around line 36-38: The constructor in class RemoteSSHWorkspaceRuntime assigns
this.config = config but no class field config is declared and the value is
never used; remove the assignment this.config = config from the constructor to
fix the TS2339 error (or alternatively, if you actually need to store the host
config, add a class field declaration like private config: SSHHostConfig and
then keep the assignment); update only the constructor (and the class fields)
around RemoteSSHWorkspaceRuntime, constructor, and the getPoolKey/id
initialization to reflect the chosen approach.
- Around line 24-25: The _globalPool variable is dead code and contradicts the
comment: either remove the pool and its comment and the SSHConnectionPool
import, or wire the pool into RemoteSSHWorkspaceRuntime so instances use shared
connections; to fix quickly, delete the _globalPool declaration and its comment
plus the unused SSHConnectionPool import, or alternatively modify the
RemoteSSHWorkspaceRuntime constructor to obtain connections from _globalPool
(e.g., replace new SSHConnection(config) with a pool.get(config) call and ensure
pool.release/close usage) and keep the comment consistent.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts`:
- Around line 270-280: The reconnect loop can overwrite this.client after
disconnect() is called because it checks signal.aborted before but not after
createClient(); fix by capturing the newly created client in a local variable
(e.g., newClient = await this.createClient()) and after it resolves check
signal.aborted and/or current state (and whether this.client was nulled) before
assigning to this.client; if aborted or disconnected, immediately close
newClient (ensure you call its .end()/.dispose()/close method as appropriate)
and do not emit "connected" or set state, otherwise assign newClient to
this.client, setState({status: "connected"}) and emit("connected").
- Around line 63-64: connect() currently only checks for status === "connected"
allowing concurrent calls to both call createClient() and leak the first
connection; fix by adding a guard and coordination: introduce and use a single
in-flight promise (e.g., this._connectingPromise) or set this._state.status =
"connecting" immediately so subsequent calls await the same operation, have
connect() return the existing this._connectingPromise when present, and ensure
createClient() results are assigned to this.client only once (or any superseded
client is cleanly ended via .end()); update related reconnection logic to
respect "connecting" and "reconnecting" states so no concurrent createClient()
calls run.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts`:
- Around line 314-328: The reattachSessions() method currently calls
session.reattach(this.connection, 80, 24) with hardcoded cols/rows; update the
flow so RemoteSSHSession stores its last-known dimensions (e.g., properties like
lastCols/lastRows updated by RemoteSSHSession.resize() and
RemoteSSHTerminalRuntime.resize()), and reattachSessions() uses those stored
values when calling session.reattach(this.connection, cols, rows) (with a
sensible fallback to the current runtime size or defaults if unset); ensure the
sessions map entries (session) are used to read those per-session dimensions and
remove the hardcoded 80×24.
- Around line 59-73: The connection event handlers registered in the
RemoteSSHTerminalRuntime constructor (this.connection.on("disconnected", ...)
and this.connection.on("connected", ...)) are never removed, causing leaks; fix
by extracting those inline closures into instance-bound handler methods or
stored arrow properties (e.g., this.onConnectionDisconnected and
this.onConnectionConnected), register those named handlers with
this.connection.on in the constructor, and unregister them in cleanup() (or
detachAllListeners()/dispose()) via this.connection.off/removeListener for the
exact same handler references so the listeners are removed and won’t keep the
runtime alive or run after disposal; ensure reattachSessions() and
initTmuxCapability() usage remains unchanged.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`:
- Around line 153-158: The effect that auto-selects remote mode only sets
setMode("remote") when project?.projectType === "remote" but never resets it;
update the useEffect in NewWorkspaceModal to setMode("remote") when
project?.projectType === "remote" and otherwise reset mode to the default (e.g.,
setMode("new") or an empty/default value you use elsewhere) so switching from a
remote to a non-remote project activates the correct tab; modify the useEffect
that references project?.projectType and setMode accordingly.
- Around line 629-642: The project is failing because the
createSshConnectionsRouter import is missing—add a new router module that
exports createSshConnectionsRouter with a list procedure (used as
electronTrpc.sshConnections.list) that queries the sshConnections table (via the
same prisma/db client used elsewhere) and returns the results; implement it
using the existing trpc helpers (e.g., createTRPCRouter or router builder used
in other routers) and attach a public query named list, or if SSH connections
are not needed remove the import of createSshConnectionsRouter from the routers
index and all references to electronTrpc.sshConnections.list in the codebase
(e.g., NewWorkspaceModal) so the missing module is no longer required.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts`:
- Line 34: Change the field declaration from a nullable SSHConnection to a
non-nullable one: replace "private connection: SSHConnection | null = null" with
"private connection: SSHConnection" and initialize it in the constructor where
"new SSHConnection(config)" is already assigned; remove any optional chaining
usages for "connection" (e.g., "connection?...." calls) and use direct access
instead; ensure "dispose()" behavior remains the same (do not set connection to
null) or, if you need to allow reinitialization, explicitly set it to null and
keep the nullable type consistently across constructor and dispose; update all
references to the "connection" field and any guards accordingly.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts`:
- Around line 21-29: The SSHConnectionEvents interface isn't wired to the
SSHConnection EventEmitter, so callers get no type safety; update the
SSHConnection class to declare typed overloads for EventEmitter methods (on,
once, addListener, off/removeListener, emit) using the SSHConnectionEvents keys
and tuple payloads (e.g., on<K extends keyof SSHConnectionEvents>(event: K,
listener: (...args: SSHConnectionEvents[K]) => void): this) so TypeScript checks
event names and payload shapes when using SSHConnection.on/emit/etc.; keep the
SSHConnectionEvents interface as the single source of truth and implement these
overloads on the SSHConnection class to provide compile-time safety.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts`:
- Around line 75-84: The code is mutating a readonly TerminalCapabilities via a
type cast in initTmuxCapability; instead, make the backing capabilities mutable
and update that, or expose a readonly view and leave the public property
immutable: add a private mutable field (e.g., private _capabilities:
MutableTerminalCapabilities) and replace direct casts/assignments in
initTmuxCapability with updates to _capabilities, then have public get
capabilities(): TerminalCapabilities return the readonly view (or a
frozen/readonly copy); update references to use _capabilities when initializing
tmuxAvailable from tmuxManager.isTmuxAvailable and setting
persistent/coldRestore so you no longer bypass the type system with casts.
- Line 40: The call to this.setMaxListeners(100) merely hides a potential leak —
locate where the terminal pane event handlers are added (the code that registers
"data:${paneId}", "exit:${paneId}", and "disconnect:${paneId}" listeners inside
the TerminalRuntime/terminal-runtime class) and ensure those listeners are
removed when a pane is detached or killed (use removeListener/off or unsubscribe
in the pane detach/kill handler such as the pane teardown/cleanup path). After
fixing listener cleanup, reduce the max listener ceiling to a sensible value
(e.g., 10) or remove the setMaxListeners call entirely so real leaks surface;
keep references to the exact symbols "data:${paneId}", "exit:${paneId}",
"disconnect:${paneId}", and the pane detach/kill cleanup method to guide where
to add removal logic.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`:
- Around line 617-732: The RemoteWorkspaceForm and SshRemoteProjectForm
duplicate the SSH connection select, remote-path input, optional name input,
empty-connections UI, and create button; extract those shared bits into a new
component (e.g., SshConnectionFields) that accepts props like sshConnectionId,
onSshConnectionChange, remotePath, onRemotePathChange, connections, pathLabel
and a render prop or callback for the action button; update RemoteWorkspaceForm
(references: RemoteWorkspaceForm, createRemote, handleCreate,
createRemote.isPending) and SshRemoteProjectForm to use SshConnectionFields,
move the “no connections” UI into the shared component or surface it via a prop,
and keep only the differing mutation logic, labels and navigation/back behavior
in each parent component.
| /** Shared connection pool across all RemoteSSHWorkspaceRuntime instances */ | ||
| const _globalPool = new SSHConnectionPool(); |
There was a problem hiding this comment.
_globalPool is declared but never referenced — dead code contradicting the architecture comment.
The comment reads "Shared connection pool across all RemoteSSHWorkspaceRuntime instances" yet _globalPool is never used. The constructor creates a standalone new SSHConnection(config) per instance, bypassing the pool entirely. If the intent is per-instance isolation, remove the pool variable and its comment. If connection sharing was intended, the pool wiring is missing.
♻️ Proposed cleanup (per-instance isolation path)
-/** Shared connection pool across all RemoteSSHWorkspaceRuntime instances */
-const _globalPool = new SSHConnectionPool();
-
export class RemoteSSHWorkspaceRuntime implements WorkspaceRuntime {Also remove the unused SSHConnectionPool import:
import {
getPoolKey,
RemoteClipboardService,
RemoteGitService,
RemoteSSHTerminalRuntime,
SFTPService,
SSHConnection,
- SSHConnectionPool,
type SSHHostConfig,
} from "./remote-ssh/index";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts` around lines 24 -
25, The _globalPool variable is dead code and contradicts the comment: either
remove the pool and its comment and the SSHConnectionPool import, or wire the
pool into RemoteSSHWorkspaceRuntime so instances use shared connections; to fix
quickly, delete the _globalPool declaration and its comment plus the unused
SSHConnectionPool import, or alternatively modify the RemoteSSHWorkspaceRuntime
constructor to obtain connections from _globalPool (e.g., replace new
SSHConnection(config) with a pool.get(config) call and ensure pool.release/close
usage) and keep the comment consistent.
| constructor(config: SSHHostConfig) { | ||
| this.config = config; | ||
| this.id = `remote-ssh:${getPoolKey(config)}`; |
There was a problem hiding this comment.
this.config is never declared as a class field — TypeScript compile error.
config is not listed in the class field declarations (lines 28–34). Assigning this.config = config will fail with TS2339: Property 'config' does not exist on type 'RemoteSSHWorkspaceRuntime'. The stored value is also never read — config is consumed directly via the constructor parameter on line 38. The assignment should simply be removed.
🐛 Proposed fix
constructor(config: SSHHostConfig) {
- this.config = config;
this.id = `remote-ssh:${getPoolKey(config)}`;📝 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.
| constructor(config: SSHHostConfig) { | |
| this.config = config; | |
| this.id = `remote-ssh:${getPoolKey(config)}`; | |
| constructor(config: SSHHostConfig) { | |
| this.id = `remote-ssh:${getPoolKey(config)}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh.ts` around lines 36 -
38, The constructor in class RemoteSSHWorkspaceRuntime assigns this.config =
config but no class field config is declared and the value is never used; remove
the assignment this.config = config from the constructor to fix the TS2339 error
(or alternatively, if you actually need to store the host config, add a class
field declaration like private config: SSHHostConfig and then keep the
assignment); update only the constructor (and the class fields) around
RemoteSSHWorkspaceRuntime, constructor, and the getPoolKey/id initialization to
reflect the chosen approach.
| async connect(): Promise<void> { | ||
| if (this._state.status === "connected") return; |
There was a problem hiding this comment.
connect() has no guard for the "connecting" or "reconnecting" states — concurrent calls leak SSH connections.
If connect() is called twice concurrently (or while reconnection is in progress), both callers pass the status === "connected" check and each invokes createClient(), establishing two separate SSH connections. Only the last result is stored in this.client; the first is abandoned without ever calling .end().
🐛 Proposed fix
- async connect(): Promise<void> {
- if (this._state.status === "connected") return;
+ private connectingPromise: Promise<void> | null = null;
+
+ async connect(): Promise<void> {
+ if (this._state.status === "connected") return;
+ if (this.connectingPromise) return this.connectingPromise;
this.intentionalDisconnect = false;
this.setState({ status: "connecting" });
- try {
- this.client = await this.createClient();
- this.setState({ status: "connected" });
- this.emit("connected");
- } catch (err) {
- const error = err instanceof Error ? err : new Error(String(err));
- this.setState({ status: "disconnected", lastError: error.message });
- this.emit("error", error);
- throw error;
- }
+ this.connectingPromise = (async () => {
+ try {
+ this.client = await this.createClient();
+ this.setState({ status: "connected" });
+ this.emit("connected");
+ } catch (err) {
+ const error = err instanceof Error ? err : new Error(String(err));
+ this.setState({ status: "disconnected", lastError: error.message });
+ this.emit("error", error);
+ throw error;
+ } finally {
+ this.connectingPromise = null;
+ }
+ })();
+ return this.connectingPromise;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts` around
lines 63 - 64, connect() currently only checks for status === "connected"
allowing concurrent calls to both call createClient() and leak the first
connection; fix by adding a guard and coordination: introduce and use a single
in-flight promise (e.g., this._connectingPromise) or set this._state.status =
"connecting" immediately so subsequent calls await the same operation, have
connect() return the existing this._connectingPromise when present, and ensure
createClient() results are assigned to this.client only once (or any superseded
client is cleanly ended via .end()); update related reconnection logic to
respect "connecting" and "reconnecting" states so no concurrent createClient()
calls run.
| try { | ||
| await waitForReconnect(attempt, this.reconnectStrategy, signal); | ||
| if (signal.aborted) return; | ||
|
|
||
| this.client = await this.createClient(); | ||
| this.setState({ status: "connected" }); | ||
| this.emit("connected"); | ||
| return; | ||
| } catch { | ||
| // Continue to next attempt | ||
| } |
There was a problem hiding this comment.
Reconnect loop leaks the SSH connection established after disconnect() is called mid-createClient().
signal.aborted is checked at line 272 before createClient() is called, but not after it resolves. Because createClient() can block for up to 30 s (the readyTimeout), disconnect() can complete in that window — nulling this.client, aborting the signal, and setting state to "disconnected". When createClient() eventually resolves, it overwrites this.client, sets state to "connected", and emits "connected" despite the intentional disconnect. The newly established SSH client is never closed.
🐛 Proposed fix
try {
await waitForReconnect(attempt, this.reconnectStrategy, signal);
if (signal.aborted) return;
this.client = await this.createClient();
+ if (signal.aborted || this.intentionalDisconnect) {
+ this.client.end();
+ this.client = null;
+ return;
+ }
this.setState({ status: "connected" });
this.emit("connected");
return;
} catch {
// Continue to next attempt
}📝 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.
| try { | |
| await waitForReconnect(attempt, this.reconnectStrategy, signal); | |
| if (signal.aborted) return; | |
| this.client = await this.createClient(); | |
| this.setState({ status: "connected" }); | |
| this.emit("connected"); | |
| return; | |
| } catch { | |
| // Continue to next attempt | |
| } | |
| try { | |
| await waitForReconnect(attempt, this.reconnectStrategy, signal); | |
| if (signal.aborted) return; | |
| this.client = await this.createClient(); | |
| if (signal.aborted || this.intentionalDisconnect) { | |
| this.client.end(); | |
| this.client = null; | |
| return; | |
| } | |
| this.setState({ status: "connected" }); | |
| this.emit("connected"); | |
| return; | |
| } catch { | |
| // Continue to next attempt | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/connection.ts` around
lines 270 - 280, The reconnect loop can overwrite this.client after disconnect()
is called because it checks signal.aborted before but not after createClient();
fix by capturing the newly created client in a local variable (e.g., newClient =
await this.createClient()) and after it resolves check signal.aborted and/or
current state (and whether this.client was nulled) before assigning to
this.client; if aborted or disconnected, immediately close newClient (ensure you
call its .end()/.dispose()/close method as appropriate) and do not emit
"connected" or set state, otherwise assign newClient to this.client,
setState({status: "connected"}) and emit("connected").
| // Wire connection events | ||
| this.connection.on("disconnected", () => { | ||
| for (const [paneId] of this.sessions) { | ||
| this.emit(`disconnect:${paneId}`); | ||
| } | ||
| }); | ||
|
|
||
| this.connection.on("connected", () => { | ||
| // Reattach sessions after reconnection | ||
| void this.reattachSessions(); | ||
| }); | ||
|
|
||
| // Check tmux availability lazily | ||
| void this.initTmuxCapability(); | ||
| } |
There was a problem hiding this comment.
Connection event listeners are never removed — resource leak and dangling callbacks.
this.connection.on("disconnected", ...) and this.connection.on("connected", ...) are registered in the constructor but neither cleanup() nor detachAllListeners() removes them from this.connection. this.removeAllListeners() removes listeners from the terminal runtime itself, not from SSHConnection.
As a result:
- The closures over
thisprevent theRemoteSSHTerminalRuntimefrom being GC'd as long as theSSHConnectionlives. - After
dispose()→connection.disconnect(), the"disconnected"handler fires on a cleaned-up runtime, iterating now-empty sessions and emitting stale events.
🐛 Proposed fix
+ private onDisconnected = () => {
+ for (const [paneId] of this.sessions) {
+ this.emit(`disconnect:${paneId}`);
+ }
+ };
+
+ private onConnected = () => {
+ void this.reattachSessions();
+ };
+
constructor(connection: SSHConnection) {
// ...
- this.connection.on("disconnected", () => {
- for (const [paneId] of this.sessions) {
- this.emit(`disconnect:${paneId}`);
- }
- });
-
- this.connection.on("connected", () => {
- void this.reattachSessions();
- });
+ this.connection.on("disconnected", this.onDisconnected);
+ this.connection.on("connected", this.onConnected);
}Then in cleanup():
async cleanup(): Promise<void> {
+ this.connection.off("disconnected", this.onDisconnected);
+ this.connection.off("connected", this.onConnected);
await this.killAllRemoteSessions();
}📝 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.
| // Wire connection events | |
| this.connection.on("disconnected", () => { | |
| for (const [paneId] of this.sessions) { | |
| this.emit(`disconnect:${paneId}`); | |
| } | |
| }); | |
| this.connection.on("connected", () => { | |
| // Reattach sessions after reconnection | |
| void this.reattachSessions(); | |
| }); | |
| // Check tmux availability lazily | |
| void this.initTmuxCapability(); | |
| } | |
| private onDisconnected = () => { | |
| for (const [paneId] of this.sessions) { | |
| this.emit(`disconnect:${paneId}`); | |
| } | |
| }; | |
| private onConnected = () => { | |
| void this.reattachSessions(); | |
| }; | |
| constructor(connection: SSHConnection) { | |
| // Wire connection events | |
| this.connection.on("disconnected", this.onDisconnected); | |
| this.connection.on("connected", this.onConnected); | |
| // Check tmux availability lazily | |
| void this.initTmuxCapability(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts`
around lines 59 - 73, The connection event handlers registered in the
RemoteSSHTerminalRuntime constructor (this.connection.on("disconnected", ...)
and this.connection.on("connected", ...)) are never removed, causing leaks; fix
by extracting those inline closures into instance-bound handler methods or
stored arrow properties (e.g., this.onConnectionDisconnected and
this.onConnectionConnected), register those named handlers with
this.connection.on in the constructor, and unregister them in cleanup() (or
detachAllListeners()/dispose()) via this.connection.off/removeListener for the
exact same handler references so the listeners are removed and won’t keep the
runtime alive or run after disposal; ensure reattachSessions() and
initTmuxCapability() usage remains unchanged.
| private async reattachSessions(): Promise<void> { | ||
| if (!this.tmuxAvailable) return; | ||
|
|
||
| for (const [paneId, session] of this.sessions) { | ||
| try { | ||
| const reattached = await session.reattach(this.connection, 80, 24); | ||
| if (!reattached) { | ||
| this.emit(`exit:${paneId}`, 0); | ||
| this.sessions.delete(paneId); | ||
| } | ||
| } catch { | ||
| this.emit(`error:${paneId}`, new Error("Failed to reattach session")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded 80×24 in reattachSessions() restores sessions at the wrong terminal size.
After an SSH reconnect, every session is reattached with cols=80, rows=24 regardless of the actual terminal window. This causes wrapping artifacts and incorrect display until the user manually resizes.
The RemoteSSHSession already supports resize() and RemoteSSHTerminalRuntime.resize() is the right call-site for updates — consider storing the last-known dimensions per session in RemoteSSHSession and using them here.
♻️ Proposed fix sketch
In RemoteSSHSession, track dimensions:
+ private _cols = 80;
+ private _rows = 24;
resize(cols: number, rows: number): void {
+ this._cols = cols;
+ this._rows = rows;
// ...
}In reattachSessions():
- const reattached = await session.reattach(this.connection, 80, 24);
+ const reattached = await session.reattach(
+ this.connection,
+ session.cols,
+ session.rows,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/workspace-runtime/remote-ssh/terminal-runtime.ts`
around lines 314 - 328, The reattachSessions() method currently calls
session.reattach(this.connection, 80, 24) with hardcoded cols/rows; update the
flow so RemoteSSHSession stores its last-known dimensions (e.g., properties like
lastCols/lastRows updated by RemoteSSHSession.resize() and
RemoteSSHTerminalRuntime.resize()), and reattachSessions() uses those stored
values when calling session.reattach(this.connection, cols, rows) (with a
sensible fallback to the current runtime size or defaults if unset); ensure the
sessions map entries (session) are used to read those per-session dimensions and
remove the hardcoded 80×24.
| // Auto-select "remote" mode when a remote project is selected | ||
| useEffect(() => { | ||
| if (project?.projectType === "remote") { | ||
| setMode("remote"); | ||
| } | ||
| }, [project?.projectType]); |
There was a problem hiding this comment.
Mode is not reset when switching from a remote to a non-remote project.
When the user selects a remote project, mode is auto-set to "remote". If they then switch to a non-remote project, mode stays "remote" — the "New" and "Import" tabs reappear but the "Remote" tab remains active, showing the RemoteWorkspaceForm for a local project.
Consider also resetting mode when the project type is not "remote":
Proposed fix
useEffect(() => {
if (project?.projectType === "remote") {
setMode("remote");
+ } else if (mode === "remote") {
+ setMode("new");
}
-}, [project?.projectType]);
+}, [project?.projectType, mode]);📝 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.
| // Auto-select "remote" mode when a remote project is selected | |
| useEffect(() => { | |
| if (project?.projectType === "remote") { | |
| setMode("remote"); | |
| } | |
| }, [project?.projectType]); | |
| // Auto-select "remote" mode when a remote project is selected | |
| useEffect(() => { | |
| if (project?.projectType === "remote") { | |
| setMode("remote"); | |
| } else if (mode === "remote") { | |
| setMode("new"); | |
| } | |
| }, [project?.projectType, mode]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`
around lines 153 - 158, The effect that auto-selects remote mode only sets
setMode("remote") when project?.projectType === "remote" but never resets it;
update the useEffect in NewWorkspaceModal to setMode("remote") when
project?.projectType === "remote" and otherwise reset mode to the default (e.g.,
setMode("new") or an empty/default value you use elsewhere) so switching from a
remote to a non-remote project activates the correct tab; modify the useEffect
that references project?.projectType and setMode accordingly.
| const { data: connections = [] } = | ||
| electronTrpc.sshConnections.list.useQuery(); | ||
|
|
||
| const createRemote = | ||
| electronTrpc.workspaces.createRemoteWorkspace.useMutation({ | ||
| onSuccess: (data) => { | ||
| toast.success("Remote workspace created"); | ||
| onSuccess(); | ||
| navigateToWorkspace(data.workspace.id, navigate, { replace: true }); | ||
| }, | ||
| onError: (err) => { | ||
| toast.error(err.message); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tRPC router definitions in the codebase
echo "=== Searching for sshConnections router ==="
find . -type f -name "*.ts" -path "*/trpc/*" -o -name "*router*" -type f | head -20
echo ""
echo "=== Looking for sshConnections procedure ==="
rg "sshConnections" --type=ts -g "!node_modules" -B 2 -A 2 | head -40
echo ""
echo "=== Looking for createRemoteWorkspace ==="
rg "createRemoteWorkspace" --type=ts -g "!node_modules" -B 2 -A 2 | head -40
echo ""
echo "=== Looking for projects.createRemote ==="
rg "createRemote" --type=ts -g "!node_modules" -g "!*.tsx" -B 2 -A 2 | head -40
echo ""
echo "=== Check trpc router structure ==="
fd -t f "trpc" -E node_modules | head -30Repository: superset-sh/superset
Length of output: 6352
🏁 Script executed:
#!/bin/bash
# Search for sshConnections router definition
echo "=== Searching for sshConnections router file ==="
find apps/desktop/src/lib/trpc -type f -name "*.ts" | grep -i ssh
echo ""
echo "=== Searching for 'sshConnections' in router index files ==="
rg "sshConnections" apps/desktop/src/lib/trpc --type=ts -B 2 -A 2
echo ""
echo "=== Check if sshConnections is a router ==="
rg "sshConnections\s*:" --type=ts -g "!node_modules" | head -20
echo ""
echo "=== Look for 'list' procedure definitions ==="
rg "list.*publicProcedure|list\s*:\s*publicProcedure" apps/desktop/src/lib/trpc --type=ts -B 2 -A 5 | head -50Repository: superset-sh/superset
Length of output: 5971
🏁 Script executed:
#!/bin/bash
# Find the sshConnections router definition
echo "=== Search for createSshConnectionsRouter ==="
rg "createSshConnectionsRouter" apps/desktop/src/lib/trpc --type=ts -B 2 -A 5
echo ""
echo "=== Look for ssh-connections or sshConnections router file ==="
fd -t f "ssh" apps/desktop/src/lib/trpc --type=ts
echo ""
echo "=== Check the routers directory structure ==="
find apps/desktop/src/lib/trpc/routers -type dRepository: superset-sh/superset
Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Search for ssh-connections file
echo "=== Look for ssh-connections file or directory ==="
find apps/desktop/src/lib/trpc/routers -name "*ssh*" -type f
find apps/desktop/src/lib/trpc/routers -name "*ssh*" -type d
echo ""
echo "=== Check what files are directly in routers directory ==="
ls -la apps/desktop/src/lib/trpc/routers/ | grep -v "^d" | head -20
echo ""
echo "=== Search for 'ssh-connections' import errors or file ==="
rg "ssh-connections" apps/desktop/src/lib/trpc --type=ts
echo ""
echo "=== Check if the file might be indexed ==="
ls -la apps/desktop/src/lib/trpc/routers/ | head -30Repository: superset-sh/superset
Length of output: 2172
🏁 Script executed:
#!/bin/bash
# Check if ssh-connections router file exists or was supposed to be added
echo "=== Check git status for ssh-connections ==="
git status | grep -i ssh
echo ""
echo "=== Check git ls-files for ssh-connections ==="
git ls-files | grep -i ssh
echo ""
echo "=== Check if this is a new file in the PR ==="
git diff --name-only HEAD | grep -i ssh
echo ""
echo "=== Check git log for recent changes to index.ts ==="
git log --oneline -n 20 -- apps/desktop/src/lib/trpc/routers/index.ts
echo ""
echo "=== Verify the import path in index.ts ==="
grep -n "ssh-connections" apps/desktop/src/lib/trpc/routers/index.ts
</parameter>Repository: superset-sh/superset
Length of output: 1736
🏁 Script executed:
#!/bin/bash
# Check if ssh-connections router file is referenced anywhere else
echo "=== Search for createSshConnectionsRouter usage ==="
rg "createSshConnectionsRouter" apps/desktop/src --type=ts
echo ""
echo "=== Check git history for ssh-connections file ==="
git log --all --full-history -- "apps/desktop/src/lib/trpc/routers/ssh-connections*" | head -20
echo ""
echo "=== Check the revert commit ==="
git show 1b534ee3 --stat | head -40Repository: superset-sh/superset
Length of output: 2658
🏁 Script executed:
# Quick confirmation - check if build would fail
grep -n "import.*ssh-connections" apps/desktop/src/lib/trpc/routers/index.tsRepository: superset-sh/superset
Length of output: 130
The sshConnections tRPC router implementation is missing, causing a build failure.
This component depends on three tRPC procedures:
electronTrpc.workspaces.createRemoteWorkspace✓ defined inapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tselectronTrpc.projects.createRemote✓ defined inapps/desktop/src/lib/trpc/routers/projects/projects.tselectronTrpc.sshConnections.list✗ missing
The file apps/desktop/src/lib/trpc/routers/index.ts (line 21) imports createSshConnectionsRouter from ./ssh-connections, but this module does not exist. The import will cause a MODULE_NOT_FOUND error at build time.
Implement apps/desktop/src/lib/trpc/routers/ssh-connections/index.ts with a list procedure that queries the sshConnections table from the database, or remove the import and all references to electronTrpc.sshConnections if this router is not intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx`
around lines 629 - 642, The project is failing because the
createSshConnectionsRouter import is missing—add a new router module that
exports createSshConnectionsRouter with a list procedure (used as
electronTrpc.sshConnections.list) that queries the sshConnections table (via the
same prisma/db client used elsewhere) and returns the results; implement it
using the existing trpc helpers (e.g., createTRPCRouter or router builder used
in other routers) and attach a public query named list, or if SSH connections
are not needed remove the import of createSshConnectionsRouter from the routers
index and all references to electronTrpc.sshConnections.list in the codebase
(e.g., NewWorkspaceModal) so the missing module is no longer required.
|
exciting. i hope we merge something soon. thx for working on this |
|
had some issues, better to start from scratch for someone else |
2 participants
What this does
Adds SSH remote workspace support to the desktop app.
Working
Not included / needs more work
The Changes/Files tab wiring for remote workspaces is in this branch but still rough — needs more testing and review before it's ready. Happy to split that into a follow-up if preferred.
Tested on
macOS (Apple Silicon) → SSH to Ubuntu 22.04 over LAN
Schema
0027_add_project_type.sql0028_add_missing_ssh_columns.sqlSummary by CodeRabbit
New Features
Improvements