From 9f0492e69fc263502568cec6bfbb5db7d4c19642 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 00:22:48 -0700 Subject: [PATCH] fix(review): address PR #1713 review findings - base-commit-capture.ts: shell-quote integration branch with a real single-quoted POSIX literal instead of JSON.stringify (not shell-safe). - TaskCard.tsx: memo compares full workspaceWorktrees values, not just key sets, so a same-key worktreePath/branch change re-renders. - TaskDetailModal.tsx: gate/render workspace summary off hydrated workingTask. - worktree-acquisition.ts: null the singular worktree/branch columns in the workspaceWorktrees write so isWorkspaceTask stays true; wrap non-fatal post-acquire observability so logEntry/audit can't re-escalate to fatal. - agent-tools.ts: register sub-repo worktree via onAcquired unconditionally (idempotent) so a resumed/already-acquired path is tracked after restart. - executor.ts: DB liveness fallback also checks task.workspaceWorktrees paths. - executor-workspace.test.ts: root non-git assertion runs in fx.rootDir ("."). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dashboard/app/components/TaskCard.tsx | 8 ++- .../app/components/TaskDetailModal.tsx | 6 +- .../src/__tests__/executor-workspace.test.ts | 5 +- packages/engine/src/agent-tools.ts | 12 ++-- packages/engine/src/base-commit-capture.ts | 17 +++-- packages/engine/src/executor.ts | 12 +++- packages/engine/src/worktree-acquisition.ts | 62 +++++++++++++++---- 7 files changed, 96 insertions(+), 26 deletions(-) diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index 14d93678e8..6400b3b02e 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -631,8 +631,12 @@ function areTaskCardPropsEqual(previous: TaskCardProps, next: TaskCardProps): bo // F7 — compare the sorted key SETS, not just the count: a same-count repo swap (one // repo released, a different one acquired) keeps the count but must still re-render, // otherwise the placeholder shows a stale repo set. - JSON.stringify(Object.keys(previousTask.workspaceWorktrees ?? {}).sort()) === - JSON.stringify(Object.keys(nextTask.workspaceWorktrees ?? {}).sort()) && + // FNXC:Workspace 2026-06-22-09:00: compare full VALUES, not only the key set. A + // pool-reclaim re-acquire keeps the same repo key but produces a different + // worktreePath/branch; a key-set-only check would leave the card showing stale path + // text. Whole-map JSON compare covers keys and values at negligible cost for small N. + JSON.stringify(previousTask.workspaceWorktrees ?? null) === + JSON.stringify(nextTask.workspaceWorktrees ?? null) && previousTask.branch === nextTask.branch && previousTask.baseBranch === nextTask.baseBranch && previousTask.breakIntoSubtasks === nextTask.breakIntoSubtasks && diff --git a/packages/dashboard/app/components/TaskDetailModal.tsx b/packages/dashboard/app/components/TaskDetailModal.tsx index 6432ed838f..6b275babff 100644 --- a/packages/dashboard/app/components/TaskDetailModal.tsx +++ b/packages/dashboard/app/components/TaskDetailModal.tsx @@ -3069,7 +3069,11 @@ export function TaskDetailContent({ {/* FNXC:Workspace 2026-06-21-00:00: workspace tasks have no singular task.worktree/task.branch; surface their acquired per-sub-repo worktrees as a flat read-only list so the detail view isn't blank (U3/KTD5). */} - {isWorkspaceTask(task) && } + {/* FNXC:Workspace 2026-06-22-09:00: gate/render off the hydrated + workingTask, not the sparse task row. workspaceWorktrees is only + present in fetched detail, so keying off task renders blank on the + optimistic-open path before the detail fetch resolves. */} + {isWorkspaceTask(workingTask) && } )} {task.status === "failed" && task.error && ( diff --git a/packages/engine/src/__tests__/executor-workspace.test.ts b/packages/engine/src/__tests__/executor-workspace.test.ts index 330915e966..7b0033bb54 100644 --- a/packages/engine/src/__tests__/executor-workspace.test.ts +++ b/packages/engine/src/__tests__/executor-workspace.test.ts @@ -48,8 +48,9 @@ describeIfGit("workspace fixture", () => { it("builds a non-git root with two real git sub-repos and a resolvable workspace config", async () => { fx = await createWorkspaceFixture(); - // Root is NOT a git repo. - expect(() => fx.git("..", "git rev-parse --git-dir")).toThrow(); + // Root is NOT a git repo. Use "." so the check runs in fx.rootDir itself, not + // its parent (".." would resolve to the tmpdir and could pass spuriously). + expect(() => fx.git(".", "git rev-parse --git-dir")).toThrow(); // Each sub-repo is a real git repo with a commit on main. expect(fx.git("repo-a", "git rev-parse --abbrev-ref HEAD")).toBe("main"); expect(fx.git("repo-b", "git rev-list --count HEAD")).toBe("1"); diff --git a/packages/engine/src/agent-tools.ts b/packages/engine/src/agent-tools.ts index a6fae43a06..2dbb3afc50 100644 --- a/packages/engine/src/agent-tools.ts +++ b/packages/engine/src/agent-tools.ts @@ -3669,10 +3669,14 @@ export function createAcquireRepoWorktreeTool(opts: { isError: true, }; } - // FNXC:Workspace 2026-06-21-22:30: F2 — register a freshly-acquired sub-repo worktree in the executor's activeWorktrees Set (skip the already-acquired short-circuit; that path was registered on its original fresh acquire). - if (!result.alreadyAcquired) { - onAcquired?.(result.worktreePath); - } + // FNXC:Workspace 2026-06-21-22:30: F2 — register a freshly-acquired sub-repo worktree in the executor's activeWorktrees Set (KTD2) so owner/liveness checks see live per-repo worktrees, not just the browse-only root. + // FNXC:Workspace 2026-06-22-09:00: register UNCONDITIONALLY, including the + // already-acquired short-circuit. After an executor restart activeWorktrees is an + // empty Map; a resumed workspace task with pre-existing task.workspaceWorktrees hits + // the alreadyAcquired path, so skipping onAcquired left the sub-repo path unregistered + // in-memory and conflict/liveness checks missed it. Set.add is idempotent, so re-firing + // on a fresh acquire is a harmless no-op. + onAcquired?.(result.worktreePath); await store.logEntry( task.id, result.alreadyAcquired diff --git a/packages/engine/src/base-commit-capture.ts b/packages/engine/src/base-commit-capture.ts index 4d9e778774..4862226f88 100644 --- a/packages/engine/src/base-commit-capture.ts +++ b/packages/engine/src/base-commit-capture.ts @@ -39,10 +39,19 @@ export async function resolveCapturedBaseCommitSha( integrationBranch: string = "main", ): Promise { const branch = integrationBranch.trim() || "main"; - // Shell-quote defensively; integration branch names are normalized upstream - // but may carry slashes (e.g. "release/2026-06") that are valid in refs. - const localRef = JSON.stringify(branch); - const originRef = JSON.stringify(`origin/${branch}`); + /* + FNXC:Workspace 2026-06-22-09:00: + Shell-quote with a real single-quoted POSIX literal, NOT JSON.stringify. A + JSON double-quoted string still lets bash expand `$(...)`, backticks, and `$VAR` + inside it; JSON.stringify is not a shell-quoting function. Git ref names can't + legally contain backticks so there's no live injection path today, but + single-quoting is the idiomatic safe form and stays correct if a caller ever + passes a less-constrained string. A single quote inside the value is escaped as + the standard `'\''` close-reopen sequence. + */ + const shellSingleQuote = (value: string): string => `'${value.replace(/'/g, "'\\''")}'`; + const localRef = shellSingleQuote(branch); + const originRef = shellSingleQuote(`origin/${branch}`); let baseCommitSha: string | undefined; try { const { stdout } = await execAsync( diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index a5e9063a5b..2ee4a765d7 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -14554,10 +14554,18 @@ You have access to the file system to review changes.${verdictBlock}`; const tasks = await this.store.listTasks({ slim: true, includeArchived: false }); for (const t of tasks) { if (t.id === requestingTaskId) continue; - if (t.worktree !== worktreePath) continue; if (t.column !== "in-progress") continue; if (t.paused === true) continue; - return t.id; + if (t.worktree === worktreePath) return t.id; + // FNXC:Workspace 2026-06-22-09:00: workspace tasks hold their worktrees in + // task.workspaceWorktrees, not the singular task.worktree column. The DB liveness + // fallback must check those per-sub-repo paths too — otherwise a conflict against a + // sub-repo worktree owned by an in-progress workspace task is missed, especially + // before its in-memory activeWorktrees entry is (re)registered after restart. + const wsEntries = t.workspaceWorktrees; + if (wsEntries && Object.values(wsEntries).some((entry) => entry.worktreePath === worktreePath)) { + return t.id; + } } } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); diff --git a/packages/engine/src/worktree-acquisition.ts b/packages/engine/src/worktree-acquisition.ts index 352ef4a036..a59c4e54b4 100644 --- a/packages/engine/src/worktree-acquisition.ts +++ b/packages/engine/src/worktree-acquisition.ts @@ -643,6 +643,24 @@ export async function acquireWorkspaceRepoWorktree( const repoAbsPath = join(workspaceRootDir, repoRelPath); + /* + FNXC:Workspace 2026-06-22-09:00: + Run best-effort observability (task log + audit) for the NON-FATAL post-acquire + steps without letting their own awaited writes escape. logEntry/audit can throw + (DB hiccup, audit sink failure); an unsuppressed throw inside a non-fatal catch + would re-escalate guard/base-capture failures into fatal acquisition errors that + strand the already-created worktree. Mirrors the busy-path swallow above. + */ + const safeObserve = async (fn: () => Promise): Promise => { + try { + await fn(); + } catch (obsErr) { + logger?.warn( + `${task.id}: workspace acquisition observability failed (suppressed): ${obsErr instanceof Error ? obsErr.message : String(obsErr)}`, + ); + } + }; + /* FNXC:Workspace 2026-06-21-20:10: Same-sub-repo exclusivity (KTD4): register the sub-repo absolute path in the @@ -748,11 +766,17 @@ export async function acquireWorkspaceRepoWorktree( // FNXC:Workspace 2026-06-21-22:30: F3 — identity-guard install is non-fatal; worktree is usable without it. const message = guardErr instanceof Error ? guardErr.message : String(guardErr); logger?.warn(`${task.id}: identity-guard install failed for sub-repo ${repoRelPath} (non-fatal): ${message}`); - await store.logEntry(task.id, `Workspace sub-repo identity-guard install failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); - await audit?.git({ - type: "worktree:workspace-repo-acquire-failed", - target: repoAbsPath, - metadata: { repoRelPath, taskId: task.id, error: message, stage: "identity-guard" }, + // FNXC:Workspace 2026-06-22-09:00: the observability writes (store.logEntry / audit.git) + // are themselves awaited and can throw; an unwrapped throw here would escape the catch + // and re-escalate this deliberately NON-FATAL step into a fatal acquisition error, + // stranding the already-created worktree. Suppress observability failures via safeObserve. + await safeObserve(async () => { + await store.logEntry(task.id, `Workspace sub-repo identity-guard install failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); + await audit?.git({ + type: "worktree:workspace-repo-acquire-failed", + target: repoAbsPath, + metadata: { repoRelPath, taskId: task.id, error: message, stage: "identity-guard" }, + }); }); } @@ -779,11 +803,15 @@ export async function acquireWorkspaceRepoWorktree( // FNXC:Workspace 2026-06-21-22:30: F3 — base-SHA capture is non-fatal; an undefined baseCommitSha is an accepted state. const message = baseErr instanceof Error ? baseErr.message : String(baseErr); logger?.warn(`${task.id}: base-SHA capture failed for sub-repo ${repoRelPath} (non-fatal): ${message}`); - await store.logEntry(task.id, `Workspace sub-repo base-SHA capture failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); - await audit?.git({ - type: "worktree:workspace-repo-acquire-failed", - target: repoAbsPath, - metadata: { repoRelPath, taskId: task.id, error: message, stage: "base-sha-capture" }, + // FNXC:Workspace 2026-06-22-09:00: same non-fatal contract as the identity-guard catch — + // the awaited observability writes must not re-escalate a non-fatal base-capture failure. + await safeObserve(async () => { + await store.logEntry(task.id, `Workspace sub-repo base-SHA capture failed for ${repoRelPath} (non-fatal): ${message}`, undefined, runContext); + await audit?.git({ + type: "worktree:workspace-repo-acquire-failed", + target: repoAbsPath, + metadata: { repoRelPath, taskId: task.id, error: message, stage: "base-sha-capture" }, + }); }); } @@ -802,7 +830,19 @@ export async function acquireWorkspaceRepoWorktree( ...(latest.workspaceWorktrees ?? {}), [repoRelPath]: { worktreePath: result.worktreePath, branch: result.branch, baseCommitSha }, }; - await store.updateTask(task.id, { workspaceWorktrees: updated }); + /* + FNXC:Workspace 2026-06-22-09:00: + F10 — reset the singular worktree/branch columns to null in the SAME write that + persists workspaceWorktrees. The single-repo `acquireTaskWorktree` above wrote + `task.worktree`/`task.branch` (the sub-repo path/branch) to the real task row; + clearing the in-memory copy passed in only stops the NEXT sub-repo from resuming + into this one's worktree — the DB row stays polluted. A non-null `task.worktree` + makes `isWorkspaceTask(task)` return false (its first guard), so the dashboard + stops rendering WorkspaceWorktreesSummary and instead shows the sub-repo branch in + the standard chip — the blank/wrong-card state U10 prevents. Nulling them here + keeps `task.worktree` null for the workspace task's whole lifetime. + */ + await store.updateTask(task.id, { workspaceWorktrees: updated, worktree: null, branch: null }); return { worktreePath: result.worktreePath, branch: result.branch, baseCommitSha, alreadyAcquired: false }; } catch (err) {