From 1557fc5a47ab7e20a562fa63ab89928a3b77f40f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 17:57:00 -0700 Subject: [PATCH] =?UTF-8?q?fix(FN-branch-group):=20repair=20CI=20failures?= =?UTF-8?q?=20=E2=80=94=20execFile=20mock=20compatibility,=20TaskCard=20na?= =?UTF-8?q?rrowing,=20e2e=20memo=20race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolve execFile lazily via namespace import in coordinator/merger/ task-lifecycle so the repo's exec-only child_process test mocks load again (10+ engine suites failed at import); dashboard.test.ts mock gains execFile so the argv-based git probes hit the mock instead of spawning real git - TaskCard: capture optional branchContext.groupId into a const (narrowing doesn't survive into the onClick closure; app tsconfig caught it in CI) - planning e2e: bounded poll past the 2.5s listTasks startup memo that served a pre-landing snapshot on fast CI runs --- .../cli/src/commands/__tests__/dashboard.test.ts | 11 +++++++++++ packages/cli/src/commands/task-lifecycle.ts | 10 ++++++++-- packages/dashboard/app/components/TaskCard.tsx | 13 ++++++++----- .../branch-group-single-pr-e2e.test.ts | 12 ++++++++++-- packages/engine/src/group-merge-coordinator.ts | 9 +++++++-- packages/engine/src/merger.ts | 10 ++++++++-- 6 files changed, 52 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/__tests__/dashboard.test.ts b/packages/cli/src/commands/__tests__/dashboard.test.ts index b535317fa4..34ff54754d 100644 --- a/packages/cli/src/commands/__tests__/dashboard.test.ts +++ b/packages/cli/src/commands/__tests__/dashboard.test.ts @@ -275,10 +275,21 @@ const { vi.mock("node:child_process", async (importOriginal) => { const original = await importOriginal(); + // execFile mirrors exec's success-callback contract: the new argv-based git + // probes (pushTaskBranchToOrigin / gitCommandSucceeds) must hit the mock, not + // spawn real git against this test's fake cwds. + const mockExecFile = ((_file: string, _args?: unknown, optsOrCb?: unknown, cbMaybe?: unknown) => { + const callback = [optsOrCb, cbMaybe, _args].find((v) => typeof v === "function") as + | ((err: null, stdout: string, stderr: string) => void) + | undefined; + if (callback) callback(null, "", ""); + return { pid: 12346, stdout: null, stderr: null, on: vi.fn(), once: vi.fn(), kill: vi.fn() }; + }) as unknown as typeof original.execFile; return { ...original, exec: mockExec, execSync: mockExecSync, + execFile: mockExecFile, }; }); diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index 3011f868e1..be0004c70e 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -13,10 +13,16 @@ * - Full PR lifecycle orchestration (create → status check → merge) */ -import { exec, execFile } from "node:child_process"; +import { exec } from "node:child_process"; +import * as childProcess from "node:child_process"; import { promisify } from "node:util"; const execAsync = promisify(exec); -const execFileAsync = promisify(execFile); +// `execFile` is resolved lazily through the namespace import so test mocks that +// only stub `exec`/`execSync` (the repo's established node:child_process mock +// convention) can still load this module; `execFile` is only required when a +// code path actually shells out. +const execFileAsync: (file: string, args: string[], opts?: import("node:child_process").ExecFileOptions) => Promise<{ stdout: string; stderr: string }> = (file, args, opts) => + (promisify(childProcess.execFile) as (f: string, a: string[], o?: object) => Promise<{ stdout: string; stderr: string }>)(file, args, opts); import type { TaskStore } from "@fusion/core"; import { resolveTaskMergeTarget, getCurrentRepo, isBranchGroupMemberLanded } from "@fusion/core"; import type { Settings, TaskDetail, PrInfo, MergeResult, BranchGroup, BranchGroupPrState, Task } from "@fusion/core"; diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index a2a0e9932a..0285831585 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -1929,19 +1929,22 @@ function TaskCardComponent({ )} {task.branchContext?.groupId && (() => { const { branchContext } = task; - if (!branchContext?.groupId) return null; + // Capture into a const: narrowing on the optional groupId does not + // survive into the onClick closure below. + const groupId = branchContext?.groupId; + if (!branchContext || !groupId) return null; return ( { if (!onOpenGroupModal) return; event.stopPropagation(); - onOpenGroupModal(branchContext.groupId); + onOpenGroupModal(groupId); }} > @@ -1950,7 +1953,7 @@ function TaskCardComponent({ {branchContext.assignmentMode === "shared" && branchMetadata.branch ? branchMetadata.branch - : branchContext.groupId} + : groupId} ); diff --git a/packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts b/packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts index fb2dbc6c91..7a0b617006 100644 --- a/packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts @@ -189,8 +189,16 @@ describe("U8 end-to-end: single managed group PR (planning + mission)", () => { expect((await aiMergeTask(store, rootDir, second.id, { syncGroupPr })).merged).toBe(true); await store.updateTask(second.id, { column: "done" } as any); - // Completion gate now satisfied (canonical predicate). - const members = (await store.listTasksByBranchGroup(group.id)) as Task[]; + // Completion gate now satisfied (canonical predicate). listTasks carries + // a 2.5s startup memo that can serve a pre-landing snapshot on fast CI + // runs — poll past it (bounded) so this and the promote gate below read + // fresh member state through the real listTasksByBranchGroup path. + let members: Task[] = []; + for (let attempt = 0; attempt < 20; attempt += 1) { + members = (await store.listTasksByBranchGroup(group.id)) as Task[]; + if (evaluateBranchGroupCompletion({ members, group }).complete) break; + await new Promise((resolve) => setTimeout(resolve, 250)); + } expect(evaluateBranchGroupCompletion({ members, group }).complete).toBe(true); // Promote → EXACTLY ONE PR via createGroupPr; persisted open. diff --git a/packages/engine/src/group-merge-coordinator.ts b/packages/engine/src/group-merge-coordinator.ts index bcd00189f4..28bfd9dda6 100644 --- a/packages/engine/src/group-merge-coordinator.ts +++ b/packages/engine/src/group-merge-coordinator.ts @@ -1,4 +1,4 @@ -import { execFile } from "node:child_process"; +import * as childProcess from "node:child_process"; import { promisify } from "node:util"; import type { BranchGroup, BranchGroupPrState, MergeTargetResolution, Settings, Task, TaskStore } from "@fusion/core"; @@ -8,7 +8,12 @@ import { resolveIntegrationBranch } from "./integration-branch.js"; // argv-based git invocation: arguments are passed as an array (no shell), so // branch names like `foo$(touch /tmp/x)` can never trigger command substitution. // Defense-in-depth alongside store-level validateBranchGroupBranchName. -const execFileAsync = promisify(execFile); +// `execFile` is resolved lazily through the namespace import so test mocks that +// only stub `exec`/`execSync` (the repo's established node:child_process mock +// convention) can still load this module; `execFile` is only required when a +// code path actually shells out. +const execFileAsync: (file: string, args: string[], opts?: import("node:child_process").ExecFileOptions) => Promise<{ stdout: string; stderr: string }> = (file, args, opts) => + (promisify(childProcess.execFile) as (f: string, a: string[], o?: object) => Promise<{ stdout: string; stderr: string }>)(file, args, opts); export interface BranchGroupMergeRouting { branchGroup: BranchGroup; diff --git a/packages/engine/src/merger.ts b/packages/engine/src/merger.ts index 2d4b624b2c..49ab13c6de 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -1,11 +1,17 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { execSync, exec, execFile } from "node:child_process"; +import { execSync, exec } from "node:child_process"; +import * as childProcess from "node:child_process"; import { promisify } from "node:util"; import { IDENTITY_GUARD_BYPASS_ENV } from "./worktree-hooks.js"; // Internal git plumbing intentionally bypasses sandbox backends. const execAsync = promisify(exec); -const execFileAsync = promisify(execFile); +// `execFile` is resolved lazily through the namespace import so test mocks that +// only stub `exec`/`execSync` (the repo's established node:child_process mock +// convention) can still load this module; `execFile` is only required when a +// code path actually shells out. +const execFileAsync: (file: string, args: string[], opts?: import("node:child_process").ExecFileOptions) => Promise<{ stdout: string; stderr: string }> = (file, args, opts) => + (promisify(childProcess.execFile) as (f: string, a: string[], o?: object) => Promise<{ stdout: string; stderr: string }>)(file, args, opts); /** * Env for merger-driven `git commit` calls so the identity-guard pre-commit