From 453ed92dbffbeb65ddc9c8ab888880108920f046 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 23:02:46 -0700 Subject: [PATCH] =?UTF-8?q?fix(review):=20Phase=20B=20workspace=20hardenin?= =?UTF-8?q?g=20=E2=80=94=20fail-closed=20scope=20guard,=20review=20conjunc?= =?UTF-8?q?tion,=20.changeset=20carve-out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ce-code-review (4 personas) on Phase B. No P0; the review conjunction was confirmed safe (no false-done — empty map and per-repo throws both route to UNAVAILABLE, which blocks). Applied: P1: the fn_task_done scope-leak guard now fails CLOSED in workspace mode — a per-repo capture throw blocks completion ("refusing as a precaution") instead of the outer .catch returning {blocked:false} and letting an incomplete check pass. A scoped task that acquired ZERO sub-repo worktrees is now blocked rather than silently passing scope enforcement. P2: reviewWorkspacePerRepo breaks on the first non-APPROVE repo so a later repo's throw can't discard an already-determined REVISE (callers were seeing UNAVAILABLE instead). captureWorkspaceModifiedFiles isolates each per-repo capture in try/catch so one repo's throw can't skip the modifiedFiles write. The .changeset always-allowed carve-out is honored in workspace mode: the scope-leak branch now filters repo-LOCAL paths via the (previously dead) workspace-paths.ts deriveRepoScopeSubset helper through the same filter as the singular path, so a sub-repo .changeset/* no longer falsely blocks fn_task_done. All four per-repo loops iterate sorted keys for deterministic offending-repo reporting; the dead repoRel callback param and the duplicate path-normalizer are removed. Verified safe (no change): the reviewer semaphore releases on throw (try/finally), and per-repo reviewers inherit the task abort via session disposal. Deferred to Phase C: extracting a workspace-executor.ts module (before the merge loop lands). Gate green: typecheck, lint, build, test:gate (649+58). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../executor-workspace-taskdone.test.ts | 71 ++++++++ .../src/__tests__/reviewer-workspace.test.ts | 40 ++++- packages/engine/src/executor.ts | 151 +++++++++++++----- packages/engine/src/workspace-paths.ts | 12 +- 4 files changed, 227 insertions(+), 47 deletions(-) diff --git a/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts index 9f24aaa75c..8d62639a6d 100644 --- a/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts +++ b/packages/engine/src/__tests__/executor-workspace-taskdone.test.ts @@ -164,6 +164,77 @@ describeIfGit("U2 KTD4 — per-repo scope-leak guard in fn_task_done", () => { const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); expect(result.blocked).toBe(false); }); + + // FNXC:Workspace 2026-06-21-15:00: F5 — per-repo `.changeset/` carve-out honored in workspace mode. + // A legit sub-repo changeset (`repo-a/.changeset/x.md`) must NOT be flagged off-scope: the always-allowed + // filter now runs against the repo-LOCAL remainder (`.changeset/x.md`), so the carve-out matches. Before + // the fix the file was prefixed BEFORE filtering, the `.changeset/` startsWith never matched, and + // fn_task_done was wrongly REFUSED. + it("F5: a sub-repo `.changeset/` file is NOT flagged off-scope (always-allowed honored)", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + // A per-repo changeset OUTSIDE the declared `repo-a/src/**` scope — only the always-allowed + // carve-out can keep this from being a leak. + mkdirSync(path.join(a.worktreePath, ".changeset"), { recursive: true }); + writeFileSync(path.join(a.worktreePath, ".changeset", "tidy-foo.md"), "---\n'@x': patch\n---\n", "utf-8"); + execSync("git add .changeset/tidy-foo.md", { cwd: a.worktreePath, stdio: "pipe" }); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(false); + }); + + // FNXC:Workspace 2026-06-21-15:00: F2 — scoped task that acquired ZERO sub-repo worktrees is blocked. + // declaredScope is non-empty but `workspaceWorktrees` is empty → scope cannot be verified at all. The + // guard must refuse fn_task_done rather than silently aggregating zero off-scope files and passing. + it("F2: scoped task with zero acquired worktrees → blocked (cannot verify scope)", async () => { + fx = await createWorkspaceFixture(); + const store = createStore(["repo-a/src/**"]); + const executor = workspaceExecutor(fx, store); + const task = makeTask({ branch: BRANCH, workspaceWorktrees: {} }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("acquired no sub-repo worktrees"); + }); + + // FNXC:Workspace 2026-06-21-15:00: F1 — fail CLOSED on a mid-loop capture throw. + // If one repo's capture throws (scope is UNVERIFIED for that repo), the guard must BLOCK naming the + // repo — not let the outer `.catch()` fail open and proceed with an incomplete scope check. + it("F1: a mid-loop capture throw → blocked (fail-closed), names the repo", async () => { + fx = await createWorkspaceFixture(); + const a = addRepoWorktree(fx, "repo-a", "src/a.ts"); + const b = addRepoWorktree(fx, "repo-b", "src/b.ts"); + const store = createStore(["repo-a/src/**", "repo-b/src/**"]); + const executor = workspaceExecutor(fx, store); + // Narrow seam: force the per-repo uncommitted capture to throw for repo-a's worktree only. + const realCapture = (executor as any).captureUncommittedModifiedFiles.bind(executor); + vi.spyOn(executor as any, "captureUncommittedModifiedFiles").mockImplementation(async (wt: unknown) => { + if (wt === a.worktreePath) throw new Error("simulated capture failure"); + return realCapture(wt as string); + }); + const task = makeTask({ + branch: BRANCH, + workspaceWorktrees: { + "repo-a": { worktreePath: a.worktreePath, branch: BRANCH, baseCommitSha: a.baseCommitSha }, + "repo-b": { worktreePath: b.worktreePath, branch: BRANCH, baseCommitSha: b.baseCommitSha }, + }, + }); + + const result = await (executor as any).evaluateTaskDoneScopeLeak(task, fx.rootDir, PROMPT, SETTINGS); + expect(result.blocked).toBe(true); + expect(result.message).toContain("repo-a"); + expect(result.message).toContain("refusing fn_task_done"); + }); }); describeIfGit("U2 KTD4 — per-repo worktree-invariant verify in fn_task_done", () => { diff --git a/packages/engine/src/__tests__/reviewer-workspace.test.ts b/packages/engine/src/__tests__/reviewer-workspace.test.ts index cab774f3aa..4f5306d190 100644 --- a/packages/engine/src/__tests__/reviewer-workspace.test.ts +++ b/packages/engine/src/__tests__/reviewer-workspace.test.ts @@ -96,13 +96,17 @@ afterEach(() => { }); describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared loop both call sites use)", () => { + // FNXC:Workspace 2026-06-21-15:00: F7 — the per-repo callback is single-arg `(cwd)` now; tests map + // cwd→repo themselves (the loop no longer passes repoRel through to runForCwd). + const repoOfCwd = (cwd: string): string => (cwd === WT_A ? "repo-a" : cwd === WT_B ? "repo-b" : cwd); + it("conjunction: two repos both APPROVE → aggregate APPROVE, one reviewer pass per repo cwd", async () => { const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); const executor = workspaceExecutor(makeStore(task)); const seen: string[] = []; - const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string, repo: string) => { + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { seen.push(cwd); - return { verdict: "APPROVE", review: `clean in ${repo}`, summary: `clean ${repo}` }; + return { verdict: "APPROVE", review: `clean in ${repoOfCwd(cwd)}`, summary: `clean ${repoOfCwd(cwd)}` }; }); expect(seen).toEqual([WT_A, WT_B]); // one pass per sub-repo cwd, never ROOT expect(result.verdict).toBe("APPROVE"); @@ -113,7 +117,8 @@ describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared l it("conjunction: one repo REVISE → aggregate REVISE, tagged with the failing repo", async () => { const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); const executor = workspaceExecutor(makeStore(task)); - const result = await (executor as any).reviewWorkspacePerRepo(task, async (_cwd: string, repo: string) => { + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + const repo = repoOfCwd(cwd); return repo === "repo-b" ? { verdict: "REVISE", review: `bug in ${repo}`, summary: `revise ${repo}` } : { verdict: "APPROVE", review: `clean ${repo}`, summary: `clean ${repo}` }; @@ -124,6 +129,35 @@ describe("U2 KTD3 — reviewWorkspacePerRepo conjunction + tagging (the shared l expect(result.summary).toMatch(/^repo-b:/); }); + // FNXC:Workspace 2026-06-21-15:00: F3 — break on the FIRST non-APPROVE repo. + it("F3: repo-a APPROVE + repo-b REVISE (no throw) → aggregate REVISE tagged repo-b", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + const repo = repoOfCwd(cwd); + return repo === "repo-a" + ? { verdict: "APPROVE", review: "clean repo-a", summary: "clean a" } + : { verdict: "REVISE", review: "bug repo-b", summary: "revise b" }; + }); + expect(result.verdict).toBe("REVISE"); + expect(result.summary).toMatch(/^repo-b:/); + }); + + it("F3: repo-a REVISE + repo-b throws → REVISE preserved (break before repo-b; NOT masked to UNAVAILABLE)", async () => { + const task = makeTask({ workspaceWorktrees: TWO_REPO_WORKTREES }); + const executor = workspaceExecutor(makeStore(task)); + const seen: string[] = []; + const result = await (executor as any).reviewWorkspacePerRepo(task, async (cwd: string) => { + seen.push(cwd); + if (cwd === WT_B) throw new Error("repo-b reviewer blew up"); + return { verdict: "REVISE", review: "bug repo-a", summary: "revise a" }; + }); + // repo-a recorded the first non-APPROVE and the loop BROKE, so repo-b's reviewer is never invoked. + expect(seen).toEqual([WT_A]); + expect(result.verdict).toBe("REVISE"); + expect(result.summary).toMatch(/^repo-a:/); + }); + it("zero-acquire workspace task → UNAVAILABLE (caller routes; no fabricated APPROVE)", async () => { const task = makeTask({ workspaceWorktrees: {} }); const executor = workspaceExecutor(makeStore(task)); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 74d0f45a2b..be43f645c9 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -82,6 +82,12 @@ import { resolveSandboxBackend } from "./sandbox/index.js"; import type { SandboxBackend } from "./sandbox/types.js"; import { ModelRegistry, SessionManager, type ToolDefinition, type AgentSession } from "@earendil-works/pi-coding-agent"; import { PRIORITY_EXECUTE, type AgentSemaphore } from "./concurrency.js"; +// FNXC:Workspace 2026-06-21-15:00: F5/F8 — wire in the previously dead workspace-path helpers. +// `normalizeRepoRelPath` is the single shared scope-path normalizer (F8); `deriveRepoScopeSubset` +// maps the task's repo-prefixed declared File Scope to a repo-LOCAL subset so the per-repo scope-leak +// filter reuses the SAME always-allowed/scope-match surface as the non-workspace path (F5). One-way +// executor→workspace-paths edge (workspace-paths imports nothing). +import { deriveRepoScopeSubset, normalizeRepoRelPath } from "./workspace-paths.js"; import { RemovalReason, classifyTaskWorktree, describeRegisteredWorktrees, detectNestedWorktreeRoot, getRegisteredWorktreePaths, isGitRepository, isInsideWorktreesDir, isRegisteredGitWorktree, removeWorktree, type WorktreePool } from "./worktree-pool.js"; import { attemptBranchAutocorrect } from "./branch-autocorrect.js"; import { ActiveSessionWorktreeRemovalError } from "./worktree-backend.js"; @@ -592,13 +598,14 @@ export interface WorkflowRevisionFeedbackPartition { const WORKFLOW_SCRIPT_OUTPUT_MAX_CHARS = 4_000; const WORKFLOW_FEEDBACK_PATH_REGEX = /`([^`\n]+)`|(?` 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 ?? {}; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { + // 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()) { + const repo = workspaceWorktrees[repoRel]; const expectedBranch = repo.branch || canonicalFusionBranchName(task.id); // Skip git checks if the worktree dir is gone (mirrors the singular FN-009 carve-out below): completion does not require a live worktree on disk. if (!existsSync(repo.worktreePath)) { @@ -10873,29 +10883,74 @@ export class TaskExecutor { // against `worktreePath`. In workspace mode `worktreePath` is the browse-only non-git workspace // root, so both silently return [] (git failures swallowed) and the uncommitted-in-scope block // never fires — a workspace task could complete with off-scope changes in any sub-repo. So we - // ITERATE every acquired sub-repo (cwd = repo.worktreePath, base = repo.baseCommitSha), - // repo-prefix each repo's touched files (`/`) so they compare against the task's - // repo-prefixed declared File Scope, and block on the FIRST repo carrying off-scope changes — - // naming the repo. The task-level preamble above (scopeOverride / declaredScope / enforcementMode) - // is shared and runs once. Return shape is preserved: `{blocked:false} | {blocked:true; message}`. + // ITERATE every acquired sub-repo (cwd = repo.worktreePath, base = repo.baseCommitSha) and block + // on the FIRST repo carrying off-scope changes — naming the repo. The task-level preamble above + // (scopeOverride / declaredScope / enforcementMode) is shared and runs once. Return shape is + // preserved: `{blocked:false} | {blocked:true; message}`. + // + // FNXC:Workspace 2026-06-21-15:00: F1/F2/F5/F6 hardening of the per-repo scope-leak guard. + // F5 (false-block fix + dead-code wiring + single filter surface): we previously repo-prefixed each + // touched file (`${repoRel}/${file}`) BEFORE filtering, so `isAlwaysAllowedScopeLeakPath`'s + // `startsWith(".changeset/")` carve-out never matched a sub-repo changeset (`repo-a/.changeset/x.md`) + // and a legit per-repo changeset was wrongly flagged off-scope → fn_task_done wrongly REFUSED. Now we + // derive each repo's repo-LOCAL declared-scope subset (`deriveRepoScopeSubset`) and run the SAME + // `workflowPathMatchesDeclaredScope` + `isAlwaysAllowedScopeLeakPath` filter the non-workspace path + // uses against the repo-LOCAL touched file — one filter surface, not two. This wires in the formerly + // dead `deriveRepoScopeSubset`/`splitRepoScopedPath` helpers. + // F1 (fail CLOSED on throw): each repo iteration is wrapped in its own try/catch (like the + // attribution-audit loop). A thrown capture/diff error in workspace mode surfaces as a BLOCK naming + // the repo instead of bubbling to the outer `.catch()` that fails OPEN — an incomplete scope check + // must never let fn_task_done proceed. + // F2 (scoped-but-zero-acquire): a scoped task that acquired NO sub-repo worktrees aggregates zero + // off-scope files and would silently pass; we block it (scope is declared but unverifiable). + // F6 (deterministic ordering): iterate sorted repo keys so the reported offending repo is stable + // across runs/rehydrate. let touchedFiles: string[]; let offendingRepo: string | undefined; if (this.workspaceConfig) { const workspaceWorktrees = task.workspaceWorktrees ?? {}; + const repoKeys = Object.keys(workspaceWorktrees).sort(); + // F2: declaredScope is non-empty here (the `declaredScope.length === 0` early-return above + // handled the unscoped case). A scoped task that acquired no sub-repo worktrees cannot have its + // scope verified at all — refuse rather than silently passing scope enforcement. + if (repoKeys.length === 0) { + const message = "workspace task declares File Scope but acquired no sub-repo worktrees — cannot verify scope"; + executorLog.warn(`${task.id}: [scope-leak] ${message}`); + await this.store.logEntry(task.id, `[scope-leak] ${message}`, undefined, this.getRunContextFor(task.id)); + return { blocked: true, message }; + } const aggregatedOffScope: string[] = []; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { - const [repoUncommitted, repoCommitted] = await Promise.all([ - this.captureUncommittedModifiedFiles(repo.worktreePath), - this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, "scope-leak-guard"), - ]); - const repoTouched = [...new Set([...repoUncommitted, ...repoCommitted])].map((f) => `${repoRel}/${f}`); - const repoOffScope = repoTouched - .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope)) - .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)); - if (repoOffScope.length > 0) { - // First offending repo wins (mirrors verifyWorktreeInvariants' first-failing-repo return). - if (!offendingRepo) offendingRepo = repoRel; - aggregatedOffScope.push(...repoOffScope); + for (const repoRel of repoKeys) { + const repo = workspaceWorktrees[repoRel]; + try { + const [repoUncommitted, repoCommitted] = await Promise.all([ + this.captureUncommittedModifiedFiles(repo.worktreePath), + this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, "scope-leak-guard"), + ]); + // Repo-LOCAL touched files (no `${repoRel}/` prefix) so the always-allowed `.changeset/` + // carve-out and the scope match operate as the reviewer/cwd=repo sees them (F5). + const repoTouched = [...new Set([...repoUncommitted, ...repoCommitted])]; + // Repo-LOCAL declared-scope subset for THIS repo (prefix stripped). Same filter as the + // non-workspace branch below — one surface. + const repoScopeSubset = deriveRepoScopeSubset(declaredScope, repoRel); + const repoOffScope = repoTouched + .filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, repoScopeSubset)) + .filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath)) + // Re-prefix the surviving off-scope files for the operator-facing message/attribution. + .map((filePath) => `${repoRel}/${filePath}`); + if (repoOffScope.length > 0) { + // First offending repo wins (mirrors verifyWorktreeInvariants' first-failing-repo return). + if (!offendingRepo) offendingRepo = repoRel; + aggregatedOffScope.push(...repoOffScope); + } + } catch (repoErr: unknown) { + // F1: fail CLOSED. A capture/diff throw means scope is UNVERIFIED for this repo; refuse + // fn_task_done as a precaution rather than letting the outer `.catch()` fail open. + const errMessage = repoErr instanceof Error ? repoErr.message : String(repoErr); + const message = `workspace scope-leak guard failed to evaluate (${repoRel}/${errMessage}) — refusing fn_task_done as a precaution`; + executorLog.warn(`${task.id}: [scope-leak] ${message}`); + await this.store.logEntry(task.id, `[scope-leak] ${message}`, undefined, this.getRunContextFor(task.id)); + return { blocked: true, message }; } } touchedFiles = aggregatedOffScope; @@ -12455,11 +12510,21 @@ ${failureFeedback} source = "post-session", ): Promise { const workspaceWorktrees = task.workspaceWorktrees ?? {}; + // FNXC:Workspace 2026-06-21-15:00: F4/F6 — per-repo error isolation + deterministic ordering. + // F4: an unexpected throw from one repo's `captureModifiedFiles` must NOT escape and skip the + // downstream `updateTask({modifiedFiles})` write — that would leave `task.modifiedFiles` empty and + // blind the merge file audit. Wrap each per-repo call (log + continue), mirroring the post-session + // branch-attribution loop. F6: iterate sorted repo keys so aggregation order is stable across runs. const aggregated: string[] = []; - for (const [repoRel, repo] of Object.entries(workspaceWorktrees)) { - const repoFiles = await this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source); - for (const file of repoFiles) { - aggregated.push(`${repoRel}/${file}`); + for (const repoRel of Object.keys(workspaceWorktrees).sort()) { + const repo = workspaceWorktrees[repoRel]; + try { + const repoFiles = await this.captureModifiedFiles(repo.worktreePath, repo.baseCommitSha, task.id, audit, source); + for (const file of repoFiles) { + aggregated.push(`${repoRel}/${file}`); + } + } catch (repoErr: unknown) { + executorLog.warn(`${task.id}: per-repo modified-file capture failed for ${repoRel}: ${repoErr instanceof Error ? repoErr.message : String(repoErr)}`); } } return aggregated; @@ -12483,12 +12548,18 @@ ${failureFeedback} * UNAVAILABLE retry) is unchanged. */ private async reviewWorkspacePerRepo( + // FNXC:Workspace 2026-06-21-15:00: F7 — drop the dead `repoRel` callback param. + // Both call sites bind `(cwd) => runForCwd(cwd)` and discard the second arg, so the type wrongly + // implied repo identity is observable inside `runForCwd`. Removed until a real consumer needs it + // (Phase C). The loop below still tags findings with `repoRel` from its own iteration key. task: Task, - invokeForCwd: (cwd: string, repoRel: string) => Promise, + invokeForCwd: (cwd: string) => Promise, ): Promise { const workspaceWorktrees = task.workspaceWorktrees ?? {}; - const entries = Object.entries(workspaceWorktrees); - if (entries.length === 0) { + // FNXC:Workspace 2026-06-21-15:00: F6 — sort repo keys so the reported FIRST failing repo is + // deterministic across runs/rehydrate. + const repoKeys = Object.keys(workspaceWorktrees).sort(); + if (repoKeys.length === 0) { // No acquired worktree — surface UNAVAILABLE so the caller routes it rather than // fabricating an authoritative APPROVE for an un-reviewable workspace task. return { @@ -12501,13 +12572,19 @@ ${failureFeedback} const reviewSections: string[] = []; const summarySections: string[] = []; let firstFailing: { repo: string; result: ReviewResult } | undefined; - for (const [repoRel, repo] of entries) { - const result = await invokeForCwd(repo.worktreePath, repoRel); + for (const repoRel of repoKeys) { + const repo = workspaceWorktrees[repoRel]; + const result = await invokeForCwd(repo.worktreePath); // Tag every per-repo finding with its sub-repo so downstream readers attribute it correctly. reviewSections.push(`### [${repoRel}] ${result.verdict}\n${result.review}`); summarySections.push(`[${repoRel}] ${result.verdict}: ${result.summary}`); - if (result.verdict !== "APPROVE" && !firstFailing) { + if (result.verdict !== "APPROVE") { + // FNXC:Workspace 2026-06-21-15:00: F3 — BREAK on the first non-APPROVE repo. + // The contract is "the FIRST non-APPROVE repo's verdict becomes the aggregate". Without the + // break, a LATER repo's reviewer throwing would discard this already-determined REVISE/RETHINK + // and the caller would see UNAVAILABLE — masking the real verdict. Stop at the first failure. firstFailing = { repo: repoRel, result }; + break; } } @@ -12524,8 +12601,8 @@ ${failureFeedback} // Every sub-repo approved → the task is reviewed (conjunction satisfied). return { verdict: "APPROVE", - review: `All ${entries.length} sub-repo(s) approved. Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, - summary: `APPROVE across ${entries.length} sub-repo(s): ${summarySections.join(" | ")}`, + review: `All ${repoKeys.length} sub-repo(s) approved. Per-repo verdicts:\n\n${reviewSections.join("\n\n")}`, + summary: `APPROVE across ${repoKeys.length} sub-repo(s): ${summarySections.join(" | ")}`, }; } diff --git a/packages/engine/src/workspace-paths.ts b/packages/engine/src/workspace-paths.ts index 308c559341..299dbfe357 100644 --- a/packages/engine/src/workspace-paths.ts +++ b/packages/engine/src/workspace-paths.ts @@ -10,13 +10,11 @@ Matching rule: canonicalize the path to forward-slash relative segments, then pi /** Sentinel returned when a path does not belong to any configured sub-repo. */ export const UNSCOPED_REPO = "unscoped" as const; -/** - * Normalize a workspace-relative path token to forward-slash form with no leading - * `./`, no leading/trailing slashes, and collapsed duplicate slashes. Mirrors the - * executor's `normalizeWorkflowScopePath` shape so File-Scope tokens and modified - * files compare consistently, but kept local to avoid an executor import cycle. - */ -function normalizeRepoRelPath(value: string): string { +/* +FNXC:Workspace 2026-06-21-15:00: +F8 — single normalize helper. The executor previously kept its own `normalizeWorkflowScopePath` that was a near-duplicate of this function, differing only in leading-slash stripping (`/^\/+/` here vs none there) and trailing-slash greediness (`/\/+$/` here vs `/\/$/` there). Two slightly-different normalizers meant an absolute or trailing-slash-laden path could derive a different scope key in the two code paths. We promote THIS (more aggressive: strips leading slash + collapses repeated trailing slashes) to the single exported normalizer and have the executor import it for scope-path normalization, so workspace and non-workspace scope matching canonicalize identically. workspace-paths.ts stays dependency-light (imports nothing), so executor→workspace-paths is a one-way, acyclic edge. +*/ +export function normalizeRepoRelPath(value: string): string { return value .trim() .replace(/\\/g, "/")