Browse Source

fix(app): prevent terminal recovery loops (#25710)

Luke Parker 5 months ago
parent
commit
a366128a93

+ 14 - 8
packages/app/src/components/terminal.tsx

@@ -480,15 +480,21 @@ export const Terminal = (props: TerminalProps) => {
           })
           })
 
 
       const connectToken = async () => {
       const connectToken = async () => {
-        const result = await client.pty.connectToken(
-          { ptyID: id },
-          {
-            throwOnError: false,
-            headers: { "x-opencode-ticket": "1" },
-          },
-        )
+        const result = await client.pty
+          .connectToken(
+            { ptyID: id },
+            {
+              throwOnError: false,
+              headers: { "x-opencode-ticket": "1" },
+            },
+          )
+          .catch((err: unknown) => {
+            if (err instanceof Error && err.message.includes("Request is not supported")) return
+            throw err
+          })
+        if (!result) return
         if (result.response.status === 200 && result.data?.ticket) return result.data.ticket
         if (result.response.status === 200 && result.data?.ticket) return result.data.ticket
-        if ((result.response.status === 404 || result.response.status === 405) && password) return
+        if (result.response.status === 404 || result.response.status === 405) return
         if (result.response.status === 403)
         if (result.response.status === 403)
           throw new Error("PTY connect ticket rejected by origin or CSRF checks. Check the server CORS config.")
           throw new Error("PTY connect ticket rejected by origin or CSRF checks. Check the server CORS config.")
         throw new Error(`PTY connect ticket failed with ${result.response.status}`)
         throw new Error(`PTY connect ticket failed with ${result.response.status}`)

+ 41 - 1
packages/app/src/context/terminal.test.ts

@@ -1,6 +1,9 @@
 import { beforeAll, describe, expect, mock, test } from "bun:test"
 import { beforeAll, describe, expect, mock, test } from "bun:test"
 
 
-let getWorkspaceTerminalCacheKey: (dir: string) => string
+type ServerKey = Parameters<typeof import("./terminal").getTerminalServerScope>[1]
+
+let getWorkspaceTerminalCacheKey: (dir: string, scope?: string) => string
+let getTerminalServerScope: typeof import("./terminal").getTerminalServerScope
 let getLegacyTerminalStorageKeys: (dir: string, legacySessionID?: string) => string[]
 let getLegacyTerminalStorageKeys: (dir: string, legacySessionID?: string) => string[]
 let migrateTerminalState: (value: unknown) => unknown
 let migrateTerminalState: (value: unknown) => unknown
 
 
@@ -17,6 +20,7 @@ beforeAll(async () => {
   }))
   }))
   const mod = await import("./terminal")
   const mod = await import("./terminal")
   getWorkspaceTerminalCacheKey = mod.getWorkspaceTerminalCacheKey
   getWorkspaceTerminalCacheKey = mod.getWorkspaceTerminalCacheKey
+  getTerminalServerScope = mod.getTerminalServerScope
   getLegacyTerminalStorageKeys = mod.getLegacyTerminalStorageKeys
   getLegacyTerminalStorageKeys = mod.getLegacyTerminalStorageKeys
   migrateTerminalState = mod.migrateTerminalState
   migrateTerminalState = mod.migrateTerminalState
 })
 })
@@ -25,6 +29,42 @@ describe("getWorkspaceTerminalCacheKey", () => {
   test("uses workspace-only directory cache key", () => {
   test("uses workspace-only directory cache key", () => {
     expect(getWorkspaceTerminalCacheKey("/repo")).toBe("/repo:__workspace__")
     expect(getWorkspaceTerminalCacheKey("/repo")).toBe("/repo:__workspace__")
   })
   })
+
+  test("can include a server scope", () => {
+    expect(getWorkspaceTerminalCacheKey("/repo", "wsl:Debian")).toBe("wsl:Debian:/repo:__workspace__")
+  })
+})
+
+describe("getTerminalServerScope", () => {
+  test("preserves local server keys", () => {
+    expect(
+      getTerminalServerScope(
+        { type: "sidecar", variant: "base", http: { url: "http://127.0.0.1:4096" } },
+        "sidecar" as ServerKey,
+      ),
+    ).toBeUndefined()
+    expect(
+      getTerminalServerScope(
+        { type: "http", http: { url: "http://localhost:4096" } },
+        "http://localhost:4096" as ServerKey,
+      ),
+    ).toBeUndefined()
+    expect(
+      getTerminalServerScope({ type: "http", http: { url: "http://[::1]:4096" } }, "http://[::1]:4096" as ServerKey),
+    ).toBeUndefined()
+  })
+
+  test("scopes non-local server keys", () => {
+    expect(
+      getTerminalServerScope(
+        { type: "sidecar", variant: "wsl", distro: "Debian", http: { url: "http://127.0.0.1:4096" } },
+        "wsl:Debian" as ServerKey,
+      ),
+    ).toBe("wsl:Debian" as ServerKey)
+    expect(
+      getTerminalServerScope({ type: "http", http: { url: "https://example.com" } }, "https://example.com" as ServerKey),
+    ).toBe("https://example.com" as ServerKey)
+  })
 })
 })
 
 
 describe("getLegacyTerminalStorageKeys", () => {
 describe("getLegacyTerminalStorageKeys", () => {

+ 47 - 15
packages/app/src/context/terminal.tsx

@@ -4,6 +4,7 @@ import { batch, createEffect, createMemo, createRoot, on, onCleanup } from "soli
 import { useParams } from "@solidjs/router"
 import { useParams } from "@solidjs/router"
 import { useSDK } from "./sdk"
 import { useSDK } from "./sdk"
 import type { Platform } from "./platform"
 import type { Platform } from "./platform"
+import { ServerConnection, useServer } from "./server"
 import { defaultTitle, titleNumber } from "./terminal-title"
 import { defaultTitle, titleNumber } from "./terminal-title"
 import { Persist, persisted, removePersisted } from "@/utils/persist"
 import { Persist, persisted, removePersisted } from "@/utils/persist"
 
 
@@ -82,10 +83,26 @@ export function migrateTerminalState(value: unknown) {
   }
   }
 }
 }
 
 
-export function getWorkspaceTerminalCacheKey(dir: string) {
+export function getWorkspaceTerminalCacheKey(dir: string, scope?: string) {
+  if (scope) return `${scope}:${dir}:${WORKSPACE_KEY}`
   return `${dir}:${WORKSPACE_KEY}`
   return `${dir}:${WORKSPACE_KEY}`
 }
 }
 
 
+export function getTerminalServerScope(conn: ServerConnection.Any | undefined, key: ServerConnection.Key) {
+  if (!conn) return
+  if (conn.type === "sidecar" && conn.variant === "base") return
+  if (conn.type === "http") {
+    try {
+      const url = new URL(conn.http.url)
+      if (url.hostname === "localhost" || url.hostname === "127.0.0.1" || url.hostname === "::1" || url.hostname === "[::1]")
+        return
+    } catch {
+      return key
+    }
+  }
+  return key
+}
+
 export function getLegacyTerminalStorageKeys(dir: string, legacySessionID?: string) {
 export function getLegacyTerminalStorageKeys(dir: string, legacySessionID?: string) {
   if (!legacySessionID) return [`${dir}/terminal.v1`]
   if (!legacySessionID) return [`${dir}/terminal.v1`]
   return [`${dir}/terminal/${legacySessionID}.v1`, `${dir}/terminal.v1`]
   return [`${dir}/terminal/${legacySessionID}.v1`, `${dir}/terminal.v1`]
@@ -110,15 +127,21 @@ const trimTerminal = (pty: LocalPTY) => {
   }
   }
 }
 }
 
 
-export function clearWorkspaceTerminals(dir: string, sessionIDs?: string[], platform?: Platform) {
-  const key = getWorkspaceTerminalCacheKey(dir)
+export function clearWorkspaceTerminals(
+  dir: string,
+  sessionIDs?: string[],
+  platform?: Platform,
+  scope?: string,
+) {
+  const key = getWorkspaceTerminalCacheKey(dir, scope)
   for (const cache of caches) {
   for (const cache of caches) {
     const entry = cache.get(key)
     const entry = cache.get(key)
     entry?.value.clear()
     entry?.value.clear()
   }
   }
 
 
-  void removePersisted(Persist.workspace(dir, "terminal"), platform)
+  void removePersisted(Persist.workspace(dir, scope ? `terminal:${scope}` : "terminal"), platform)
 
 
+  if (scope) return
   const legacy = new Set(getLegacyTerminalStorageKeys(dir))
   const legacy = new Set(getLegacyTerminalStorageKeys(dir))
   for (const id of sessionIDs ?? []) {
   for (const id of sessionIDs ?? []) {
     for (const key of getLegacyTerminalStorageKeys(dir, id)) {
     for (const key of getLegacyTerminalStorageKeys(dir, id)) {
@@ -130,12 +153,17 @@ export function clearWorkspaceTerminals(dir: string, sessionIDs?: string[], plat
   }
   }
 }
 }
 
 
-function createWorkspaceTerminalSession(sdk: ReturnType<typeof useSDK>, dir: string, legacySessionID?: string) {
-  const legacy = getLegacyTerminalStorageKeys(dir, legacySessionID)
+function createWorkspaceTerminalSession(
+  sdk: ReturnType<typeof useSDK>,
+  dir: string,
+  legacySessionID?: string,
+  scope?: string,
+) {
+  const legacy = scope ? [] : getLegacyTerminalStorageKeys(dir, legacySessionID)
 
 
   const [store, setStore, _, ready] = persisted(
   const [store, setStore, _, ready] = persisted(
     {
     {
-      ...Persist.workspace(dir, "terminal", legacy),
+      ...Persist.workspace(dir, scope ? `terminal:${scope}` : "terminal", legacy),
       migrate: migrateTerminalState,
       migrate: migrateTerminalState,
     },
     },
     createStore<{
     createStore<{
@@ -357,8 +385,12 @@ export const { use: useTerminal, provider: TerminalProvider } = createSimpleCont
   gate: false,
   gate: false,
   init: () => {
   init: () => {
     const sdk = useSDK()
     const sdk = useSDK()
+    const server = useServer()
     const params = useParams()
     const params = useParams()
     const cache = new Map<string, TerminalCacheEntry>()
     const cache = new Map<string, TerminalCacheEntry>()
+    const scope = createMemo(() => {
+      return getTerminalServerScope(server.current, server.key)
+    })
 
 
     caches.add(cache)
     caches.add(cache)
     onCleanup(() => caches.delete(cache))
     onCleanup(() => caches.delete(cache))
@@ -382,9 +414,9 @@ export const { use: useTerminal, provider: TerminalProvider } = createSimpleCont
       }
       }
     }
     }
 
 
-    const loadWorkspace = (dir: string, legacySessionID?: string) => {
+    const loadWorkspace = (dir: string, legacySessionID: string | undefined, serverScope: string | undefined) => {
       // Terminals are workspace-scoped so tabs persist while switching sessions in the same directory.
       // Terminals are workspace-scoped so tabs persist while switching sessions in the same directory.
-      const key = getWorkspaceTerminalCacheKey(dir)
+      const key = getWorkspaceTerminalCacheKey(dir, serverScope)
       const existing = cache.get(key)
       const existing = cache.get(key)
       if (existing) {
       if (existing) {
         cache.delete(key)
         cache.delete(key)
@@ -393,7 +425,7 @@ export const { use: useTerminal, provider: TerminalProvider } = createSimpleCont
       }
       }
 
 
       const entry = createRoot((dispose) => ({
       const entry = createRoot((dispose) => ({
-        value: createWorkspaceTerminalSession(sdk, dir, legacySessionID),
+        value: createWorkspaceTerminalSession(sdk, dir, legacySessionID, serverScope),
         dispose,
         dispose,
       }))
       }))
 
 
@@ -402,16 +434,16 @@ export const { use: useTerminal, provider: TerminalProvider } = createSimpleCont
       return entry.value
       return entry.value
     }
     }
 
 
-    const workspace = createMemo(() => loadWorkspace(params.dir!, params.id))
+    const workspace = createMemo(() => loadWorkspace(params.dir!, params.id, scope()))
 
 
     createEffect(
     createEffect(
       on(
       on(
-        () => ({ dir: params.dir, id: params.id }),
+        () => ({ dir: params.dir, id: params.id, scope: scope() }),
         (next, prev) => {
         (next, prev) => {
           if (!prev?.dir) return
           if (!prev?.dir) return
-          if (next.dir === prev.dir && next.id === prev.id) return
-          if (next.dir === prev.dir && next.id) return
-          loadWorkspace(prev.dir, prev.id).trimAll()
+          if (next.dir === prev.dir && next.id === prev.id && next.scope === prev.scope) return
+          if (next.dir === prev.dir && next.id && next.scope === prev.scope) return
+          loadWorkspace(prev.dir, prev.id, prev.scope).trimAll()
         },
         },
         { defer: true },
         { defer: true },
       ),
       ),

+ 2 - 1
packages/app/src/pages/layout.tsx

@@ -35,7 +35,7 @@ import type { DragEvent } from "@thisbeyond/solid-dnd"
 import { useProviders } from "@/hooks/use-providers"
 import { useProviders } from "@/hooks/use-providers"
 import { showToast, Toast, toaster } from "@opencode-ai/ui/toast"
 import { showToast, Toast, toaster } from "@opencode-ai/ui/toast"
 import { useGlobalSDK } from "@/context/global-sdk"
 import { useGlobalSDK } from "@/context/global-sdk"
-import { clearWorkspaceTerminals } from "@/context/terminal"
+import { clearWorkspaceTerminals, getTerminalServerScope } from "@/context/terminal"
 import { dropSessionCaches, pickSessionCacheEvictions } from "@/context/global-sync/session-cache"
 import { dropSessionCaches, pickSessionCacheEvictions } from "@/context/global-sync/session-cache"
 import {
 import {
   clearSessionPrefetchInflight,
   clearSessionPrefetchInflight,
@@ -1557,6 +1557,7 @@ export default function Layout(props: ParentProps) {
       directory,
       directory,
       sessions.map((s) => s.id),
       sessions.map((s) => s.id),
       platform,
       platform,
+      getTerminalServerScope(server.current, server.key),
     )
     )
     await globalSDK.client.instance.dispose({ directory }).catch(() => undefined)
     await globalSDK.client.instance.dispose({ directory }).catch(() => undefined)
 
 

+ 18 - 2
packages/app/src/pages/session/terminal-panel.tsx

@@ -37,6 +37,7 @@ export function TerminalPanel() {
   const [store, setStore] = createStore({
   const [store, setStore] = createStore({
     autoCreated: false,
     autoCreated: false,
     activeDraggable: undefined as string | undefined,
     activeDraggable: undefined as string | undefined,
+    recovered: {} as Record<string, boolean>,
     view: typeof window === "undefined" ? 1000 : (window.visualViewport?.height ?? window.innerHeight),
     view: typeof window === "undefined" ? 1000 : (window.visualViewport?.height ?? window.innerHeight),
   })
   })
 
 
@@ -145,6 +146,21 @@ export function TerminalPanel() {
   const all = terminal.all
   const all = terminal.all
   const ids = createMemo(() => all().map((pty) => pty.id))
   const ids = createMemo(() => all().map((pty) => pty.id))
 
 
+  const recoverTerminal = (key: string, id: string, clone: (id: string) => Promise<void>) => {
+    if (store.recovered[key]) return
+    setStore("recovered", key, true)
+    void clone(id)
+  }
+
+  const terminalRecoveryKey = (pty: { id: string; title: string; titleNumber: number }) => {
+    return String(pty.titleNumber || pty.title || pty.id)
+  }
+
+  const markTerminalConnected = (key: string, id: string, trim: (id: string) => void) => {
+    setStore("recovered", key, false)
+    trim(id)
+  }
+
   const handleTerminalDragStart = (event: unknown) => {
   const handleTerminalDragStart = (event: unknown) => {
     const id = getDraggableId(event)
     const id = getDraggableId(event)
     if (!id) return
     if (!id) return
@@ -280,9 +296,9 @@ export function TerminalPanel() {
                             <Terminal
                             <Terminal
                               pty={pty()}
                               pty={pty()}
                               autoFocus={opened()}
                               autoFocus={opened()}
-                              onConnect={() => ops.trim(id)}
+                              onConnect={() => markTerminalConnected(terminalRecoveryKey(pty()), id, ops.trim)}
                               onCleanup={ops.update}
                               onCleanup={ops.update}
-                              onConnectError={() => ops.clone(id)}
+                              onConnectError={() => recoverTerminal(terminalRecoveryKey(pty()), id, ops.clone)}
                             />
                             />
                           </div>
                           </div>
                         )}
                         )}