fix: harden remote loop launches against stale branches, clock skew, and silent errors

Author: Forge

docs/loop-system.md

@@ -187,7 +187,7 @@ graph TD
     G --> H[Branch preserved]
 ```
 
-When a workspace carries a SHA pin (`extra.startRef`, set by remote loop launches), the new branch is created from that exact commit instead of the clone's current `HEAD`. If the commit is not present locally, the adapter fetches the sync ref (`extra.syncRef`, default `refs/forge/<loopName>`) from the configured git remote first, and fails with a descriptive error when the SHA still cannot be resolved. Existing branches always win — the pin is ignored when the loop branch already exists. On final teardown the sync ref is deleted from the shared git remote. See [Configuration → Remotes](configuration.md#remotes).
+When a workspace carries a SHA pin (`extra.startRef`, set by remote loop launches), the new branch is created from that exact commit instead of the clone's current `HEAD`. If the commit is not present locally, the adapter fetches the sync ref (`extra.syncRef`, default `refs/forge/<loopName>`) from the configured git remote first, and fails with a descriptive error when the SHA still cannot be resolved. If the loop branch already exists, its tip must match the pinned SHA — a leftover same-named branch at a different commit fails creation with an actionable error instead of silently running old code (unpinned workspaces still reuse existing branches). On final teardown the sync ref is deleted from the shared git remote. See [Configuration → Remotes](configuration.md#remotes).
 
 Benefits of worktree isolation:
 - Isolation from ongoing development

src/hooks/forge-session-attach.ts

@@ -143,15 +143,28 @@ async function attachForgeSession(
       projectDirectory,
     )
 
-    if (action.action === 'keep' && action.reason !== 'running' && action.reason !== 'pending-attach') {
+    // A fresh TUI-owned launch (remote or local) whose name collides with a
+    // terminal loop row classifies as keep/pending-start (the classifier must
+    // keep restart-in-flight workspaces). Silently skipping here would leave a
+    // zombie session that runs the first prompt but never loops. Proceed to
+    // attachLoopToSession instead so its conflict handling fails loudly
+    // (toast + workspace removal). Restart-created workspaces carry no
+    // forgeLoop config and still skip below.
+    const isTerminalNameConflict =
+      action.action === 'keep' &&
+      action.reason === 'pending-start' &&
+      cfg?.initialPromptOwner === 'tui' &&
+      isPendingAttachWorkspace(classifyEntry)
+
+    if (action.action === 'keep' && action.reason !== 'running' && action.reason !== 'pending-attach' && !isTerminalNameConflict) {
       // bad config or wrong-project — toast and bail, do not attach
       deps.logger.log(
         `[forge-session-attach] skip session=${sessionId} workspace=${workspaceId} reason=${action.reason}`,
       )
       return
     }
 
-    if ((action.action === 'remove-fully' && action.reason === 'missing-row') || (action.action === 'keep' && action.reason === 'pending-attach')) {
+    if ((action.action === 'remove-fully' && action.reason === 'missing-row') || (action.action === 'keep' && action.reason === 'pending-attach') || isTerminalNameConflict) {
       // Fresh attach (no loop row yet) - proceed to attach
       if (!cfg) {
         if (action.action === 'remove-fully') {

src/utils/git-service.ts

@@ -20,6 +20,8 @@ export interface GitService {
   revParseGitCommonDir(cwd: string): GitResult
   revParseGitPath(cwd: string, path: string): GitResult
   revParseHead(cwd: string): GitResult
+  /** Resolve any ref (branch, tag, SHA) to a full commit SHA. */
+  revParseRef(cwd: string, ref: string): GitResult
   commitExists(cwd: string, sha: string): boolean
   push(cwd: string, remote: string, refspec: string, force: boolean): GitResult
   fetchRef(cwd: string, remote: string, ref: string): GitResult
@@ -84,6 +86,10 @@ export function createGitService(): GitService {
       return runGit(['rev-parse', 'HEAD'], cwd)
     },
 
+    revParseRef(cwd: string, ref: string): GitResult {
+      return runGit(['rev-parse', '--verify', `${ref}^{commit}`], cwd)
+    },
+
     commitExists(cwd: string, sha: string): boolean {
       return runGit(['cat-file', '-e', `${sha}^{commit}`], cwd).ok
     },

src/utils/tui-client.ts

@@ -276,7 +276,7 @@ export interface LaunchTuiLoopOptions {
 
 export async function launchTuiLoop(
   opts: LaunchTuiLoopOptions,
-): Promise<{ sessionId: string; loopName: string; worktreeDir?: string; workspaceId: string } | { error: string } | null> {
+): Promise<{ sessionId: string; loopName: string; worktreeDir?: string; workspaceId: string } | { error: string }> {
   const debug = opts.debug ?? tuiDebug
 
   const committedError = getWorktreeProjectPreconditionError(opts.projectId)
@@ -360,7 +360,7 @@ export async function launchTuiLoop(
     } catch (err) {
       debug(`launchTuiLoop: promptAsync failed session=${session.id} workspace=${workspace.id} error=${err instanceof Error ? err.message : String(err)}`)
       await opts.client.workspace.remove({ id: workspace.id }).catch(() => undefined)
-      return null
+      return { error: `Failed to send initial loop prompt: ${err instanceof Error ? err.message : String(err)}` }
     }
     debug(`launchTuiLoop: promptAsync ok session=${session.id} workspace=${workspace.id}`)
 
@@ -376,7 +376,7 @@ export async function launchTuiLoop(
     }
   } catch (err) {
     debug(`launchTuiLoop: post-create flow failed error=${err instanceof Error ? err.message : String(err)}`)
-    return null
+    return { error: `Loop launch failed: ${err instanceof Error ? err.message : String(err)}` }
   }
 }
 
@@ -501,7 +501,7 @@ export async function connectForgeProject(
           allowDirectories: allowExternalDirectories,
           onLaunched: (sid, wid) => selectTuiSession(api, client, sid, wid),
           debug: tuiDebug,
-        }) ?? null
+        })
       }
 
       return null

src/utils/tui-remote-launch.ts

@@ -152,10 +152,11 @@ export async function executeRemoteLoop(
     debug,
   })
 
-  if (!launchResult || 'error' in launchResult) {
-    const errMsg = launchResult && 'error' in launchResult ? launchResult.error : 'Failed to launch remote loop'
-    debug(`remote-launch: launchTuiLoop FAILED: ${errMsg}`)
-    return { error: errMsg }
+  if ('error' in launchResult) {
+    debug(`remote-launch: launchTuiLoop FAILED: ${launchResult.error}`)
+    const cleanup = git.push(req.localDirectory, remote.gitRemote, `:${syncRef}`, false)
+    debug(`remote-launch: sync ref cleanup ${cleanup.ok ? 'ok' : `failed: ${cleanup.stderr.trim() || 'unknown error'}`}`)
+    return { error: launchResult.error }
   }
 
   debug(`remote-launch: launched loop="${launchResult.loopName}" session=${launchResult.sessionId} on "${remote.name}"`)

src/workspace/forge-adapter.ts

@@ -167,6 +167,33 @@ export function createForgeWorkspaceAdapter(deps: ForgeAdapterDeps): WorkspaceAd
     })
   }
 
+  /**
+   * Re-stamp launcher-provided attach timestamps with this server's clock.
+   *
+   * Remote launches stamp `workspaceCreatedAt` and
+   * `forgeLoop.pendingAttachStartedAt` on the launching machine, but the
+   * attach/pending-start grace windows are evaluated against this server's
+   * clock (classify-stale.ts). Clock skew between the two machines beyond the
+   * grace window would otherwise expire a fresh workspace immediately.
+   * `configure` runs exactly once at creation on the owning server, so it is
+   * the single normalization point.
+   */
+  function restampAttachTimestamps(extra: unknown): unknown {
+    if (typeof extra !== 'object' || extra === null) return extra
+    const now = Date.now()
+    const record = extra as Record<string, unknown>
+    const result: Record<string, unknown> = { ...record, workspaceCreatedAt: now }
+    const forgeLoop = record.forgeLoop
+    if (
+      typeof forgeLoop === 'object' &&
+      forgeLoop !== null &&
+      typeof (forgeLoop as Record<string, unknown>).pendingAttachStartedAt === 'number'
+    ) {
+      result.forgeLoop = { ...(forgeLoop as Record<string, unknown>), pendingAttachStartedAt: now }
+    }
+    return result
+  }
+
   function deriveSyncPin(info: WorkspaceInfo, loopName: string): { startRef: string; syncRef: string; gitRemote: string } | null {
     const extra = (info.extra ?? {}) as Record<string, unknown>
     const startRef = typeof extra.startRef === 'string' && extra.startRef.length > 0 ? extra.startRef : null
@@ -226,6 +253,7 @@ export function createForgeWorkspaceAdapter(deps: ForgeAdapterDeps): WorkspaceAd
         name: loopName,
         branch: forgeBranchName(loopName),
         directory: forgeWorktreeDir(dataDir, loopName),
+        extra: restampAttachTimestamps(info.extra),
       }
     },
     async create(info) {
@@ -256,6 +284,22 @@ export function createForgeWorkspaceAdapter(deps: ForgeAdapterDeps): WorkspaceAd
       // Detect orphan state from a prior failed run: branch may exist without a live worktree.
       const branchExists = git.branchExists(projectDir, info.branch)
 
+      // A pinned launch must run exactly the pushed SHA. Reusing a leftover
+      // same-named branch at a different tip would silently run old code, so
+      // fail with an actionable error instead.
+      if (pin && branchExists) {
+        const tipRes = git.revParseRef(projectDir, `refs/heads/${info.branch}`)
+        const tip = tipRes.ok ? tipRes.stdout.trim() : ''
+        const pinnedRes = git.revParseRef(projectDir, pin.startRef)
+        const pinned = pinnedRes.ok ? pinnedRes.stdout.trim() : pin.startRef
+        if (tip !== pinned) {
+          throw new Error(
+            `forge workspace adapter: branch ${info.branch} already exists at ${tip ? tip.substring(0, 7) : 'unknown'} ` +
+            `but this launch pinned ${pinned.substring(0, 7)}; delete the stale branch or use a different loop name`,
+          )
+        }
+      }
+
       // Only pass startPoint when creating a new branch; existing branches always win.
       const startPoint = pin && !branchExists ? pin.startRef : undefined
 

test/helpers/fake-git.ts

@@ -16,6 +16,7 @@ export function createFakeGitService(overrides?: Partial<GitService>): GitServic
     revParseGitCommonDir: vi.fn<[string], GitResult>(() => ({ ...defaultOk })),
     revParseGitPath: vi.fn<[string, string], GitResult>(() => ({ ...defaultOk })),
     revParseHead: vi.fn<[string], GitResult>(() => ({ ...defaultOk })),
+    revParseRef: vi.fn<[string, string], GitResult>(() => ({ ...defaultOk })),
     commitExists: vi.fn<[string, string], boolean>(() => false),
     push: vi.fn<[string, string, string, boolean], GitResult>(() => ({ ...defaultOk })),
     fetchRef: vi.fn<[string, string, string], GitResult>(() => ({ ...defaultOk })),

test/hooks/forge-session-attach.test.ts

@@ -391,6 +391,61 @@ describe('createForgeSessionAttachHook', () => {
     expect(mockAttachLoop).not.toHaveBeenCalled()
   })
 
+  test('fresh TUI launch colliding with restartable terminal row fails loudly instead of leaving a zombie session', async () => {
+    const workspaceRemove = vi.fn().mockResolvedValue(undefined)
+    const tuiPublish = vi.fn().mockResolvedValue(undefined)
+    const loopsRepoGet = vi.fn().mockReturnValue({ projectId: 'proj_1', loopName: 'collide-loop', status: 'cancelled' })
+    mockAttachLoop.mockResolvedValueOnce({ ok: false, code: 'conflict', message: 'Loop collide-loop is terminal' })
+    const deps = buildHookDeps({
+      workspaceRemove,
+      tuiPublish,
+      loopsRepoGet,
+      sessionGet: vi.fn().mockResolvedValue({
+        id: 'sess_collide',
+        workspaceID: 'ws_collide',
+        directory: '/tmp/wt/collide',
+        projectID: 'proj_1',
+      }),
+      workspaceList: vi.fn().mockResolvedValue([
+        {
+          id: 'ws_collide',
+          type: 'forge',
+          directory: '/tmp/wt/collide',
+          extra: {
+            loopName: 'collide-loop',
+            projectDirectory: '/tmp/wt/collide',
+            workspaceCreatedAt: Date.now(),
+            forgeLoop: {
+              title: 'Collide Loop',
+              planSource: 'inline',
+              planText: '# Plan',
+              initialPromptOwner: 'tui',
+              pendingAttachStartedAt: Date.now(),
+            },
+          },
+        },
+      ]),
+    })
+
+    const handler = createForgeSessionMessageAttachHook(deps as any)
+
+    await handler({ sessionID: 'sess_collide' })
+
+    expect(mockAttachLoop).toHaveBeenCalledTimes(1)
+    expect(workspaceRemove).toHaveBeenCalledWith({ id: 'ws_collide' })
+    expect(deps.execDeps.pendingTeardowns.set).toHaveBeenCalledWith(
+      'collide-loop',
+      expect.objectContaining({ doRemoveWorktree: false }),
+    )
+    expect(tuiPublish).toHaveBeenCalledWith(expect.objectContaining({
+      body: expect.objectContaining({
+        properties: expect.objectContaining({
+          message: expect.stringContaining('terminal status'),
+        }),
+      }),
+    }))
+  })
+
   test('inline planSource resolves planText inline', async () => {
     const deps = buildHookDeps({
       workspaceList: vi.fn().mockResolvedValue([

test/tui-client.loop-error.test.ts

@@ -120,7 +120,7 @@ describe('plan.execute(loop) workspace.create failure', () => {
     }
   })
 
-  test('returns null for non-create failures in the post-create flow', async () => {
+  test('returns error detail for non-create failures in the post-create flow', async () => {
     const api = createMockApi()
 
     // Mock workspace.status to report the workspace as connected so awaitWorkspaceConnected resolves quickly
@@ -148,8 +148,12 @@ describe('plan.execute(loop) workspace.create failure', () => {
       plan: '# Test\n\nPost-create failure test.',
     })
 
-    // Post-create failures should still return null (generic)
-    expect(result).toBeNull()
+    // Post-create failures surface the underlying cause instead of a generic null
+    expect(result).not.toBeNull()
+    expect(result).toHaveProperty('error')
+    if (result && 'error' in result) {
+      expect(result.error).toContain('session create failed')
+    }
   }, 10000)
 
   test('returns committed-project error for global project before any workspace side effects', async () => {

test/utils/tui-client-warp-flow.test.ts

@@ -297,7 +297,7 @@ describe('TUI warp flow for plan.execute mode=loop', () => {
     expect(result).toEqual({ error: 'Failed to create worktree workspace: no data returned' })
   })
 
-  test('failure: session.create returns error → returns null', async () => {
+  test('failure: session.create returns error → returns {error} with cause', async () => {
     mockApi.client.session.create.mockResolvedValueOnce({ error: new Error('session create fail') })
 
     const client = await connectForgeProject(mockApi, DIRECTORY)
@@ -311,7 +311,7 @@ describe('TUI warp flow for plan.execute mode=loop', () => {
       },
     )
 
-    expect(result).toBeNull()
+    expect(result).toEqual({ error: expect.stringContaining('session create fail') })
     // workspace.create should still have been called but downstream after session.create shouldn't
     expect(mockApi.client.experimental.workspace.create).toHaveBeenCalled()
     expect(mockApi.client.tui.selectSession).not.toHaveBeenCalled()