feat(opencode-plugin): address Cubic PR #4 findings

packages/opencode-plugin/.opencode/plugin/daytona/core/logger.ts

@@ -31,24 +31,28 @@ class Logger {
     } catch (err) {
       // Directory may already exist, ignore
     }
-    // Trim log file if it exceeds 3MB (keep last 1MB)
+    // Trim by byte length (not characters) so the 1MB target holds for non-ASCII logs
     try {
       const stats = statSync(this.logFile)
       const maxSize = 3 * 1024 * 1024
       const keepSize = 1024 * 1024
       if (stats.size > maxSize) {
-        const content = readFileSync(this.logFile, 'utf8')
-        const trimmed = content.slice(-keepSize)
+        const buffer = readFileSync(this.logFile)
+        const trimmed = buffer.subarray(buffer.length - keepSize)
         // Drop partial first line so we don't start mid-log
         const firstNewline = trimmed.indexOf('\n')
-        writeFileSync(this.logFile, firstNewline >= 0 ? trimmed.slice(firstNewline + 1) : trimmed)
+        writeFileSync(this.logFile, firstNewline >= 0 ? trimmed.subarray(firstNewline + 1) : trimmed)
       }
     } catch (err) {
       // File may not exist yet, ignore
     }
     const timestamp = new Date().toISOString()
     const logEntry = `[${timestamp}] [${level}] ${message}\n`
-    appendFileSync(this.logFile, logEntry)
+    try {
+      appendFileSync(this.logFile, logEntry)
+    } catch (err) {
+      // Best-effort logging: never let a write failure crash the caller
+    }
   }
 
   info(message: string): void {

packages/opencode-plugin/.opencode/plugin/daytona/core/project-data-storage.ts

@@ -26,20 +26,34 @@ export class ProjectDataStorage {
   }
 
   /**
-   * Get the file path for a project's session data
+   * Get the file path for a project's session data.
+   * encodeURIComponent gives a reversible, collision-free encoding that also
+   * strips path separators, so a projectId can't traverse outside storageDir
+   * and distinct ids can't collide onto the same file.
    */
   private getProjectFilePath(projectId: string): string {
-    return join(this.storageDir, `${projectId}.json`)
+    return join(this.storageDir, `${encodeURIComponent(projectId)}.json`)
   }
 
   /**
-   * List known project IDs from storage.
+   * List known project IDs from storage, decoded to the canonical form used by callers.
+   * Filenames that can't be decoded (e.g. hand-created files with invalid percent escapes)
+   * are skipped: exposing them would return an id that can't round-trip through
+   * getProjectFilePath, causing subsequent load/save/remove to silently target the wrong file.
    */
   private listProjectIds(): string[] {
     try {
-      return readdirSync(this.storageDir)
-        .filter((name) => name.endsWith('.json'))
-        .map((name) => name.slice(0, -'.json'.length))
+      const ids: string[] = []
+      for (const name of readdirSync(this.storageDir)) {
+        if (!name.endsWith('.json')) continue
+        const encoded = name.slice(0, -'.json'.length)
+        try {
+          ids.push(decodeURIComponent(encoded))
+        } catch {
+          logger.warn(`Skipping project data file with undecodable name: ${name}`)
+        }
+      }
+      return ids
     } catch (err) {
       logger.error(`Failed to list project data files: ${err}`)
       return []
@@ -86,20 +100,26 @@ export class ProjectDataStorage {
         sessions: {},
       }
 
-      // Remove from source first (best effort).
+      // Write the destination first and confirm it landed on disk, so a write
+      // failure can never delete the source before the copy is safely persisted.
+      destination.sessions[sessionId] = found
+      // Prefer the worktree for the project we're actually operating on.
+      destination.worktree = worktree
+      this.save(projectId, destination)
+
+      if (!this.load(projectId)?.sessions?.[sessionId]) {
+        logger.error(`Migration of session ${sessionId} to project ${projectId} did not persist; leaving source intact`)
+        return found
+      }
+
+      // Destination is safe; now remove from the source (best effort).
       try {
         delete otherData!.sessions[sessionId]
-        this.save(otherProjectId, otherData!.worktree, otherData!.sessions)
+        this.save(otherProjectId, otherData!)
       } catch (err) {
         logger.warn(`Failed to remove session ${sessionId} from project ${otherProjectId}: ${err}`)
       }
 
-      // Add to destination and persist.
-      destination.sessions[sessionId] = found
-      // Prefer the worktree for the project we're actually operating on.
-      destination.worktree = worktree
-      this.save(projectId, destination.worktree, destination.sessions)
-
       logger.info(`Migrated session ${sessionId} from project ${otherProjectId} to project ${projectId}`)
       return found
     }
@@ -108,21 +128,42 @@ export class ProjectDataStorage {
   }
 
   /**
-   * Save project session data to disk
+   * Read-only lookup of a session across all project files. Unlike getSession, this never
+   * migrates or writes, so it is safe to use on the delete path.
    */
-  save(projectId: string, worktree: string, sessions: Record<string, SessionInfo>): void {
-    const filePath = this.getProjectFilePath(projectId)
-    const projectData: ProjectSessionData = {
-      projectId,
-      worktree,
-      sessions,
+  findSession(sessionId: string): { projectId: string; worktree: string; session: SessionInfo } | undefined {
+    for (const projectId of this.listProjectIds()) {
+      const data = this.load(projectId)
+      const session = data?.sessions?.[sessionId]
+      if (session && data) {
+        // Return the filename-derived projectId (the value that maps back to the file we
+        // just loaded), NOT data.projectId. The delete cleanup path passes this value to
+        // removeSession → load → getProjectFilePath; if we returned the raw canonical id
+        // and it doesn't round-trip identically (e.g. legacy files with a different
+        // sanitization scheme), the cleanup would silently target a different file and
+        // leave the stale mapping behind.
+        return { projectId, worktree: data.worktree, session }
+      }
     }
+    return undefined
+  }
 
+  /**
+   * Save project session data to disk.
+   *
+   * `storageKey` identifies WHICH FILE on disk to write (round-trips through
+   * getProjectFilePath). `projectData.projectId` is the CANONICAL id written into
+   * the JSON body — kept separate so callers who reached a file via a filename-derived
+   * key (e.g. findSession → removeSession for legacy files) don't clobber the
+   * pre-existing canonical id with the storage key.
+   */
+  save(storageKey: string, projectData: ProjectSessionData): void {
+    const filePath = this.getProjectFilePath(storageKey)
     try {
       writeFileSync(filePath, JSON.stringify(projectData, null, 2))
-      logger.info(`Saved project data for ${projectId}`)
+      logger.info(`Saved project data for ${projectData.projectId}`)
     } catch (err) {
-      logger.error(`Failed to save project data for ${projectId}: ${err}`)
+      logger.error(`Failed to save project data for ${projectData.projectId} at ${filePath}: ${err}`)
     }
   }
 
@@ -173,7 +214,11 @@ export class ProjectDataStorage {
       }
     }
 
-    this.save(projectId, worktree, projectData.sessions)
+    // Refresh worktree from the current call (state that can change over time), but
+    // NEVER refresh projectData.projectId — that's identity, set at creation, and
+    // preserving it is the whole point of save()'s two-parameter API.
+    projectData.worktree = worktree
+    this.save(projectId, projectData)
   }
 
   /**
@@ -183,7 +228,7 @@ export class ProjectDataStorage {
     const projectData = this.load(projectId)
     if (projectData && projectData.sessions[sessionId]) {
       delete projectData.sessions[sessionId]
-      this.save(projectId, worktree, projectData.sessions)
+      this.save(projectId, projectData)
     }
   }
 }

packages/opencode-plugin/.opencode/plugin/daytona/core/session-manager.ts

@@ -8,7 +8,7 @@
  * Stores data per-project in ~/.local/share/opencode/storage/daytona/{projectId}.json
  */
 
-import { Daytona, type Sandbox } from '@daytona/sdk'
+import { Daytona, DaytonaNotFoundError, type Sandbox } from '@daytona/sdk'
 import { logger } from './logger'
 import type { SessionSandboxMap, SandboxInfo } from './types'
 import { SessionGitManager } from '../git/session-git-manager'
@@ -112,43 +112,58 @@ export class DaytonaSessionManager {
 
     // If we have a sandboxId but not a full sandbox object, reconnect to it
     if (this.isPartiallyInitialized(existing)) {
-      logger.info(`Reconnecting to existing sandbox: ${existing.id}`)
-      const daytona = new Daytona({ apiKey: this.apiKey })
-      const reconnectStart = Date.now()
-      logger.info(`Daytona get begin sandboxId=${existing.id}`)
-      const sandbox = await daytona.get(existing.id)
-      logger.info(`Daytona get done sandboxId=${existing.id} in ${Date.now() - reconnectStart}ms`)
-      logger.info(`Starting sandbox begin sandboxId=${sandbox.id}`)
-      await sandbox.start()
-      logger.info(`Starting sandbox done sandboxId=${sandbox.id} in ${Date.now() - reconnectStart}ms`)
-      this.sessionSandboxes.set(sessionId, sandbox)
-      // Preserve branch number if it exists for this sandbox
-      let branchNumber = this.dataStorage.getBranchNumberForSandbox(projectId, sandbox.id)
-      if (!branchNumber) {
-        try {
-          branchNumber = SessionGitManager.allocateAndReserveBranchNumber(worktree)
-        } catch {
-          // No local git repo (or git unavailable) shouldn't block sandbox usage.
-          branchNumber = undefined
+      try {
+        logger.info(`Reconnecting to existing sandbox: ${existing.id}`)
+        const daytona = new Daytona({ apiKey: this.apiKey })
+        const reconnectStart = Date.now()
+        logger.info(`Daytona get begin sandboxId=${existing.id}`)
+        const sandbox = await daytona.get(existing.id)
+        logger.info(`Daytona get done sandboxId=${existing.id} in ${Date.now() - reconnectStart}ms`)
+        logger.info(`Starting sandbox begin sandboxId=${sandbox.id}`)
+        await sandbox.start()
+        logger.info(`Starting sandbox done sandboxId=${sandbox.id} in ${Date.now() - reconnectStart}ms`)
+        this.sessionSandboxes.set(sessionId, sandbox)
+        // Preserve branch number if it exists for this sandbox
+        let branchNumber = this.dataStorage.getBranchNumberForSandbox(projectId, sandbox.id)
+        if (!branchNumber) {
+          try {
+            branchNumber = SessionGitManager.allocateAndReserveBranchNumber(worktree)
+          } catch {
+            // No local git repo (or git unavailable) shouldn't block sandbox usage.
+            branchNumber = undefined
+          }
         }
-      }
-      this.dataStorage.updateSession(projectId, worktree, sessionId, sandbox.id, branchNumber)
-      toast.show({
-        title: 'Sandbox connected',
-        message: `Connected to existing sandbox.`,
-        variant: 'info',
-      })
+        this.dataStorage.updateSession(projectId, worktree, sessionId, sandbox.id, branchNumber)
+        toast.show({
+          title: 'Sandbox connected',
+          message: `Connected to existing sandbox.`,
+          variant: 'info',
+        })
 
-      // Even if git syncing is disabled, ensure the project directory exists in the sandbox.
-      if (!branchNumber) {
-        try {
-          await new DaytonaSandboxGitManager(sandbox, this.repoPath).ensureDirectory()
-        } catch (err) {
-          logger.warn(`Failed to ensure sandbox project directory exists: ${err}`)
+        // Even if git syncing is disabled, ensure the project directory exists in the sandbox.
+        if (!branchNumber) {
+          try {
+            await new DaytonaSandboxGitManager(sandbox, this.repoPath).ensureDirectory()
+          } catch (err) {
+            logger.warn(`Failed to ensure sandbox project directory exists: ${err}`)
+          }
         }
-      }
 
-      return sandbox
+        return sandbox
+      } catch (err) {
+        // Only treat 404 as "sandbox is confirmed gone" — clear the mapping and fall through
+        // to create a replacement. For transient errors (network, auth, timeout, provisioning),
+        // preserve the mapping and propagate so the caller can retry later without losing the
+        // session→sandbox binding and its branchNumber.
+        if (err instanceof DaytonaNotFoundError) {
+          logger.error(`Sandbox ${existing.id} no longer exists; creating a replacement.`)
+          this.sessionSandboxes.delete(sessionId)
+          this.dataStorage.removeSession(projectId, worktree, sessionId)
+        } else {
+          logger.error(`Failed to reconnect to sandbox ${existing.id}: ${err}`)
+          throw err
+        }
+      }
     }
 
     // If not in cache/storage for this project, try to recover from other projects and migrate.
@@ -220,39 +235,59 @@ export class DaytonaSessionManager {
   /**
    * Delete the sandbox associated with the given session ID
    */
-  async deleteSandbox(sessionId: string, projectId: string): Promise<void> {
+  async deleteSandbox(sessionId: string, projectId: string): Promise<boolean> {
     let sandbox = this.sessionSandboxes.get(sessionId)
 
+    // Read-only lookup so deleting never migrates sessions or rewrites project metadata.
+    const stored = this.dataStorage.findSession(sessionId)
+
+    let sandboxGone = false
+
     // If not in cache, try to load from storage and reconnect
     if (!sandbox || this.isPartiallyInitialized(sandbox)) {
-      const storedWorktree = this.dataStorage.load(projectId)?.worktree ?? ''
-      const sessionInfo = this.dataStorage.getSession(projectId, storedWorktree, sessionId)
-      if (sessionInfo?.sandboxId) {
+      if (stored?.session.sandboxId) {
         const daytona = new Daytona({ apiKey: this.apiKey })
         try {
-          sandbox = await daytona.get(sessionInfo.sandboxId)
+          sandbox = await daytona.get(stored.session.sandboxId)
           this.sessionSandboxes.set(sessionId, sandbox)
         } catch (err) {
-          logger.error(`Failed to reconnect to sandbox ${sessionInfo.sandboxId}: ${err}`)
+          if (err instanceof DaytonaNotFoundError) {
+            sandboxGone = true
+            logger.warn(`Sandbox ${stored.session.sandboxId} no longer exists; clearing stale mapping.`)
+          } else {
+            // Non-404: we cannot confirm the sandbox is gone. Surface the error so the
+            // event handler shows a "Delete failed" toast instead of silently reporting
+            // success while leaving a running sandbox on the Daytona account.
+            logger.error(`Failed to reconnect to sandbox ${stored.session.sandboxId}: ${err}`)
+            throw err
+          }
         }
+      } else {
+        sandboxGone = true
       }
     }
 
     // Delete the sandbox if we have a fully initialized one
+    let deleted = false
     if (this.isFullyInitialized(sandbox)) {
       logger.info(`Removing sandbox for session: ${sessionId}`)
       await sandbox.delete()
-      this.sessionSandboxes.delete(sessionId)
-
-      // Remove from storage
-      const projectData = this.dataStorage.load(projectId)
-      if (projectData) {
-        this.dataStorage.removeSession(projectId, projectData.worktree, sessionId)
-      }
-
+      deleted = true
+      sandboxGone = true
       logger.info(`Sandbox deleted successfully.`)
     } else {
       logger.warn(`No sandbox found for session: ${sessionId}`)
     }
+
+    // Clear the local mapping when the sandbox is gone (deleted or already absent) so a
+    // stale entry can't cause repeated reconnect failures. Preserve it on transient errors.
+    if (sandboxGone) {
+      this.sessionSandboxes.delete(sessionId)
+      const cleanupProjectId = stored?.projectId ?? projectId
+      const cleanupWorktree = stored?.worktree ?? this.dataStorage.load(projectId)?.worktree ?? ''
+      this.dataStorage.removeSession(cleanupProjectId, cleanupWorktree, sessionId)
+    }
+
+    return deleted
   }
 }