fix(review): address PR #1714 review findings

- base-commit-capture: POSIX single-quote integration branch refs instead of
  JSON.stringify (double quotes are subject to $-expansion in the shell)
- executor: add per-repo no_commits guard to the workspace verifyWorktreeInvariants
  branch (parity with the singular path), gated by the same task-wide no-commit
  eligibility
- executor: reviewWorkspacePerRepo failure message now states the per-repo verdict
  list is partial (evaluation stops at first failure)
- worktree-acquisition: defensively wrap non-fatal/outer-catch logEntry/audit so a
  logging throw cannot promote a non-fatal error to fatal or mask the original error
- docs/plans: add code-fence language tags and fix MD028 blank-line-in-blockquote

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-22 00:21:49 -07:00
parent 14114f5f51
commit f4a9c65509
5 changed files with 117 additions and 26 deletions

View File

@@ -155,7 +155,7 @@ The surface-enumeration spine (FN-5893). Every row is a single-worktree / `cwd:r
Additive only — no migration to existing single-repo tasks:
```
```ts
Task.workspaceWorktrees: Record<repoRelPath, {
worktreePath: string;
branch: string;

View File

@@ -23,7 +23,7 @@ It also installs the **R7 workspace merge-boundary guard** at every merge entry
Merge is dispatched at `packages/engine/src/project-engine.ts:2275-2282`:
```
```ts
const mergerMode = normalizeMergerMode(settings.merger?.mode); // defaults to "ai"
return mergerMode === "ai"
? runAiMerge(store, cwd, taskId, mergeOptionsWithSettings)
@@ -61,7 +61,7 @@ Before claiming low blast radius, grep test fixtures, CI configs, and seeded/def
## Implementation Units
> **Units `U1–U4` below are local to this plan** (they decompose master-plan U0); they are **not** the master plan's `U1–U10`. U4 (audit) may run in parallel with U1–U3.
>
> **Standing requirements:** `FNXC:Workspace <yyyy-MM-dd-hh:mm>` dated comments at each non-obvious decision point (dispatch collapse, the R7 guard, the deprecation warning). A `.changeset/*.md` (`@runfusion/fusion: minor`). Respect the merge gate (`pnpm lint`, typecheck, `pnpm build`, `pnpm test:gate`) and FN-5048 (narrow seams, fake timers, no real polling / mock-the-world). **Base branch (decided):** branch off the **foundation** (`pr-1710` / `feat/workspace-multi-repo` head) — the R7 guard (U3) reads `task.workspaceWorktrees`, which the foundation adds and `main` lacks. Do **not** commit onto `pr-1710` directly; use a new branch and open a **stacked PR targeting `feat/workspace-multi-repo`** so the diff is only U0's changes.
### U1. Collapse the engine dispatch and route the two direct callers to `runAiMerge`

View File

@@ -39,10 +39,16 @@ 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-00:00:
// Shell-quote with POSIX single quotes, NOT JSON.stringify. JSON.stringify wraps
// in double quotes, under which the shell expands `$VAR`/backticks — a branch like
// `release/$2.0` would expand `$2` to a positional. Admin-configured integration
// branch names are not guaranteed to exclude `$`, and `$` is valid in git refs, so
// double-quoting is an injection/correctness risk. Single-quoting (with the embedded
// `'` → `'\''` escape) is literal and safe for slashes (e.g. "release/2026-06") too.
const shellQuote = (s: string): string => `'${s.replace(/'/g, "'\\''")}'`;
const localRef = shellQuote(branch);
const originRef = shellQuote(`origin/${branch}`);
let baseCommitSha: string | undefined;
try {
const { stdout } = await execAsync(

View File

@@ -10570,6 +10570,26 @@ export class TaskExecutor {
// Phase A returned a flat {ok:true} stub here (no root worktree to verify against the non-git root). Phase B iterates every `task.workspaceWorktrees` entry, asserting (a) the sub-repo worktree's git toplevel matches the recorded repo.worktreePath and (b) its HEAD is on the recorded `fusion/<id>` branch (repo.branch). The result union is PRESERVED EXACTLY — `{ok:true} | {ok:false; reason:'wrong_toplevel'|'wrong_branch'|'no_commits'; observed; expected}` — because the :10889 consumer switches on `reason` to drive requeue/handoff (:10894-10936). We ADD an optional `repo` field to the failure shape (purely additive; the consumer only reads reason/observed/expected) and return the FIRST failing repo. A zero-acquire workspace task (empty map) verifies vacuously → {ok:true}, matching Phase A so fn_task_done does not requeue it.
if (this.workspaceConfig) {
const workspaceWorktrees = task.workspaceWorktrees ?? {};
// FNXC:Workspace 2026-06-22-00:00: KTD2 — resolve the SAME task-wide no-commit eligibility the singular path
// uses (getNoCommitEligibilityReason / no-op-completion sentinel / prompt-derived), once, before the per-repo
// loop. When eligible (Plan-Only, verified no-op, etc.) the per-repo no_commits guard below is skipped so an
// intentionally commit-free workspace task is not blocked from completion.
const workspacePromptContent = (task as Task & { prompt?: unknown }).prompt;
const workspacePromptEligibility = evaluatePromptDerivedNoCommitEligibility(
task,
typeof workspacePromptContent === "string" ? workspacePromptContent : "",
);
const workspaceNoCommitEligibilityReason =
getNoCommitEligibilityReason(task) ??
(options?.noOpCompletion
? options.noOpCompletionReason ?? "verified no-op/duplicate completion sentinel"
: null) ??
(workspacePromptEligibility.eligible
? workspacePromptEligibility.reason ?? "prompt-derived no-commit eligibility"
: null);
if (workspaceNoCommitEligibilityReason) {
executorLog.log(`${task.id}: workspace fn_task_done no_commits guard skipped (${workspaceNoCommitEligibilityReason})`);
}
// FNXC:Workspace 2026-06-21-15:00: F6 — iterate sorted repo keys so the FIRST failing repo
// returned here is deterministic across runs/rehydrate (the value is surfaced to the operator).
for (const repoRel of Object.keys(workspaceWorktrees).sort()) {
@@ -10647,6 +10667,48 @@ export class TaskExecutor {
expected: expectedBranch,
};
}
// FNXC:Workspace 2026-06-22-00:00: KTD2 — per-repo no_commits guard (parity with the singular path at :10821).
// Phase B originally returned {ok:true} after the toplevel/branch checks, so a workspace task could call
// fn_task_done having committed NOTHING in any sub-repo (scope-leak sees zero touched files, branch names match)
// and still advance to in-review. Enforce the same `git rev-list --count <base>..HEAD > 0` invariant per repo,
// gated by the SAME task-wide no-commit eligibility below so Plan-Only / no-op-sentinel tasks stay exempt.
// The first sub-repo with zero commits fails with reason:'no_commits' (consumer-stable union).
if (!workspaceNoCommitEligibilityReason) {
const repoBaseRef = await this.resolveDiffBaseRef(repo.worktreePath, repo.baseCommitSha);
if (repoBaseRef) {
try {
const { stdout } = await execAsync(`git rev-list --count ${repoBaseRef}..HEAD`, {
cwd: repo.worktreePath,
encoding: "utf-8",
timeout: 10_000,
maxBuffer: 1024 * 1024,
});
const trimmedCount = stdout.trim();
if (trimmedCount) {
const count = Number.parseInt(trimmedCount, 10);
if (!Number.isFinite(count) || count <= 0) {
return {
ok: false,
reason: "no_commits",
repo: repoRel,
observed: Number.isFinite(count) ? String(count) : trimmedCount,
expected: "> 0",
};
}
}
} catch (error) {
return {
ok: false,
reason: "no_commits",
repo: repoRel,
observed: error instanceof Error ? error.message : String(error),
expected: `git rev-list --count ${repoBaseRef}..HEAD > 0`,
};
}
} else {
executorLog.warn(`${task.id}: unable to resolve diff base for ${repoRel} no_commits guard; skipping for this sub-repo`);
}
}
}
return { ok: true };
}
@@ -12593,7 +12655,10 @@ ${failureFeedback}
// verdict→edge mapping is identical to single-cwd), with the full repo-tagged review body.
return {
verdict: firstFailing.result.verdict,
review: `Workspace review failed in sub-repo \`${firstFailing.repo}\` (verdict ${firstFailing.result.verdict}). Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`,
// FNXC:Workspace 2026-06-22-00:00: the conjunction BREAKS on the first non-APPROVE repo,
// so reviewSections holds only the repos evaluated up to (and including) the failure — not
// every sub-repo. Label it honestly so operators don't read a partial list as exhaustive.
review: `Workspace review failed in sub-repo \`${firstFailing.repo}\` (verdict ${firstFailing.result.verdict}). Per-repo verdicts (evaluation stopped at first failure; later repos not reviewed):\n\n${reviewSections.join("\n\n")}`,
summary: `${firstFailing.repo}: ${firstFailing.result.verdict} — ${summarySections.join(" | ")}`,
};
}

View File

@@ -746,14 +746,21 @@ export async function acquireWorkspaceRepoWorktree(
});
} catch (guardErr) {
// FNXC:Workspace 2026-06-21-22:30: F3 — identity-guard install is non-fatal; worktree is usable without it.
// FNXC:Workspace 2026-06-22-00:00: the non-fatal logEntry/audit are themselves best-effort — if either throws
// (e.g. a DB write hiccup) it must NOT promote this non-fatal guard failure into a fatal acquisition failure.
// Swallow logging errors so acquisition continues (matching the F6 busy-path defensive wrap above).
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" },
});
try {
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" },
});
} catch {
// best-effort observability only — keep the (non-fatal) guard failure non-fatal
}
}
/*
@@ -777,14 +784,20 @@ export async function acquireWorkspaceRepoWorktree(
baseCommitSha = await resolveCapturedBaseCommitSha(result.worktreePath, logger, integrationBranch);
} catch (baseErr) {
// FNXC:Workspace 2026-06-21-22:30: F3 — base-SHA capture is non-fatal; an undefined baseCommitSha is an accepted state.
// FNXC:Workspace 2026-06-22-00:00: guard the best-effort logEntry/audit so a logging throw cannot promote this
// non-fatal capture failure into a fatal acquisition failure (parity with the F6 busy-path defensive wrap).
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" },
});
try {
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" },
});
} catch {
// best-effort observability only — keep the (non-fatal) capture failure non-fatal
}
}
/*
@@ -814,14 +827,21 @@ export async function acquireWorkspaceRepoWorktree(
sub-repo.
*/
if (!(err instanceof WorkspaceRepoAcquireBusyError)) {
// FNXC:Workspace 2026-06-22-00:00: wrap the failure logEntry/audit so a throw here cannot replace the ORIGINAL
// acquisition `err` the caller must observe — losing it would mask the real cause and the re-throw below would
// surface a logging error instead. Best-effort observability; `err` is always re-thrown.
const message = err instanceof Error ? err.message : String(err);
logger?.error?.(`${task.id}: workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`);
await store.logEntry(task.id, `Workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`, undefined, runContext);
await audit?.git({
type: "worktree:workspace-repo-acquire-failed",
target: repoAbsPath,
metadata: { repoRelPath, taskId: task.id, error: message },
});
try {
await store.logEntry(task.id, `Workspace sub-repo acquisition failed for ${repoRelPath}: ${message}`, undefined, runContext);
await audit?.git({
type: "worktree:workspace-repo-acquire-failed",
target: repoAbsPath,
metadata: { repoRelPath, taskId: task.id, error: message },
});
} catch {
// best-effort observability only — ensure the original acquisition error propagates
}
}
throw err;
} finally {