root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(review): address PR #1713 review findings

Browse Source 
- 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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-22 00:22:48 -07:00
parent d5fa8654f7
commit 9f0492e69f
7 changed files with 96 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
packages/dashboard/app/components/TaskCard.tsx
View File

@@ -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 &&

6
packages/dashboard/app/components/TaskDetailModal.tsx
View File

@@ -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) && <WorkspaceWorktreesSummary task={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) && <WorkspaceWorktreesSummary task={workingTask} />}
</>
)}
{task.status === "failed" && task.error && (

5
packages/engine/src/__tests__/executor-workspace.test.ts
View File

@@ -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");

12
packages/engine/src/agent-tools.ts
View File

@@ -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

17
packages/engine/src/base-commit-capture.ts
View File

@@ -39,10 +39,19 @@ export async function resolveCapturedBaseCommitSha(
integrationBranch: string = "main",
): Promise<string | undefined> {
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(

12
packages/engine/src/executor.ts
View File

@@ -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);

62
packages/engine/src/worktree-acquisition.ts
View File

@@ -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<void>): Promise<void> => {
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) {