From c0b6f212c9982e9a751a68a0bd8cc1942248ee98 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 8 Jun 2026 10:23:01 -0700 Subject: [PATCH] fix(FN-5994): address verification review feedback Harden command normalization edge cases and abort loop compaction on timeout before falling through to stuck-task requeue. Fusion-Task-Id: FN-5994 --- .../__tests__/executor-step-session.test.ts | 3 ++ .../run-verification-command.test.ts | 31 +++++++++++++++++-- packages/engine/src/executor.ts | 9 ++++++ packages/engine/src/run-verification-tool.ts | 22 ++++++++++--- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index d4e9a49dbe..ba2a4987b7 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/packages/engine/src/__tests__/executor-step-session.test.ts @@ -3457,10 +3457,12 @@ describe("TaskExecutor loop recovery", () => { const compactRetVal = overrides && "compactResult" in overrides ? overrides.compactResult : defaultResult; const compact = vi.fn(async () => compactRetVal); const steer = vi.fn(async () => {}); + const abort = vi.fn(async () => {}); return { prompt: vi.fn(async () => {}), dispose: vi.fn(), + abort, subscribe: vi.fn(), setThinkingLevel: vi.fn(), steer, @@ -3598,6 +3600,7 @@ describe("TaskExecutor loop recovery", () => { await vi.advanceTimersByTimeAsync(60000); await expect(resultPromise).resolves.toBe(false); + expect(mockSession.abort).toHaveBeenCalled(); expect(store.logEntry).toHaveBeenCalledWith( "FN-001", expect.stringContaining("Context compaction timed out"), diff --git a/packages/engine/src/__tests__/run-verification-command.test.ts b/packages/engine/src/__tests__/run-verification-command.test.ts index 8a7c94fe3b..ceeb8ba9c0 100644 --- a/packages/engine/src/__tests__/run-verification-command.test.ts +++ b/packages/engine/src/__tests__/run-verification-command.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { fileURLToPath } from "node:url"; import { runVerificationCommand, normalizeVerificationCommand, type RunVerificationOptions } from "../run-verification-tool.js"; // Some tests use platform-appropriate shell syntax. On Windows, sh-style @@ -24,7 +24,7 @@ const itPosix = onPosix ? it : it.skip; // so we fall back to os.tmpdir() which is always C:\Users\…\Temp there. describe("runVerificationCommand", { timeout: 30000 }, () => { const tempDir = onPosix ? "/tmp" : tmpdir(); - const workspaceRoot = join(process.cwd(), "../.."); + const workspaceRoot = fileURLToPath(new URL("../../../../", import.meta.url)); describe("command normalization", () => { it("rewrites package test -- --run filters to direct vitest with package-relative files", () => { @@ -54,6 +54,33 @@ describe("runVerificationCommand", { timeout: 30000 }, () => { const command = "pnpm --filter @fusion/dashboard test"; expect(normalizeVerificationCommand(command, workspaceRoot)).toEqual({ command, warnings: [] }); }); + + it("leaves commands with unterminated shell quotes unchanged", () => { + const command = "pnpm --filter @fusion/dashboard test -- --run 'src/__tests__/routes-tasks.test.ts"; + expect(normalizeVerificationCommand(command, workspaceRoot)).toEqual({ command, warnings: [] }); + }); + + it("preserves pnpm global flags that precede --filter", () => { + const result = normalizeVerificationCommand( + "pnpm -w --filter @fusion/dashboard test -- --run packages/dashboard/src/__tests__/routes-tasks.test.ts", + workspaceRoot, + ); + + expect(result.command).toBe( + "pnpm -w --filter @fusion/dashboard exec vitest run src/__tests__/routes-tasks.test.ts --silent=passed-only --reporter=dot", + ); + }); + + it("verifies the CLI package directory through package.json before rewriting", () => { + const result = normalizeVerificationCommand( + "pnpm --filter @runfusion/fusion test -- --run packages/cli/src/__tests__/cli.test.ts", + workspaceRoot, + ); + + expect(result.command).toBe( + "pnpm --filter @runfusion/fusion exec vitest run src/__tests__/cli.test.ts --silent=passed-only --reporter=dot", + ); + }); }); describe("basic command execution", () => { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index d636a4a464..982b694951 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -13421,6 +13421,14 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit let compactionTimedOut = false; let compactionTimer: ReturnType | undefined; + const abortActiveSession = () => { + const sessionWithAbort = activeEntry.session as unknown as { abort?: () => Promise }; + if (typeof sessionWithAbort.abort === "function") { + void sessionWithAbort.abort().catch((err: unknown) => { + executorLog.warn(`${taskId} loop compaction abort after timeout failed: ${err instanceof Error ? err.message : String(err)}`); + }); + } + }; let compactResult: Awaited> | null; try { compactResult = await Promise.race([ @@ -13428,6 +13436,7 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit new Promise((resolve) => { compactionTimer = setTimeout(() => { compactionTimedOut = true; + abortActiveSession(); resolve(null); }, LOOP_COMPACTION_TIMEOUT_MS); }), diff --git a/packages/engine/src/run-verification-tool.ts b/packages/engine/src/run-verification-tool.ts index fb0fce9194..b12ab69e12 100644 --- a/packages/engine/src/run-verification-tool.ts +++ b/packages/engine/src/run-verification-tool.ts @@ -36,7 +36,9 @@ const DEFAULT_TIMEOUT_PACKAGE_SEC = 300; const DEFAULT_TIMEOUT_WORKSPACE_SEC = 900; const MAX_TIMEOUT_SEC = 1800; -function shellSplit(input: string): string[] { +const packageDirCache = new Map(); + +function shellSplit(input: string): string[] | null { const tokens: string[] = []; let current = ""; let quote: "'" | "\"" | null = null; @@ -75,6 +77,7 @@ function shellSplit(input: string): string[] { } if (escaped) current += "\\"; + if (quote !== null) return null; if (current.length > 0) tokens.push(current); return tokens; } @@ -85,7 +88,8 @@ function shellQuote(value: string): string { } function findWorkspacePackageDir(rootDir: string, packageName: string): string | null { - if (packageName === "@runfusion/fusion") return "packages/cli"; + const cacheKey = `${rootDir}\0${packageName}`; + if (packageDirCache.has(cacheKey)) return packageDirCache.get(cacheKey) ?? null; const lastSegment = packageName.split("/").pop(); const candidates = [ @@ -97,7 +101,10 @@ function findWorkspacePackageDir(rootDir: string, packageName: string): string | if (!candidate) continue; try { const pkg = JSON.parse(readFileSync(join(rootDir, candidate, "package.json"), "utf8")) as { name?: string }; - if (pkg.name === packageName) return candidate; + if (pkg.name === packageName) { + packageDirCache.set(cacheKey, candidate); + return candidate; + } } catch { // Keep looking. } @@ -123,13 +130,17 @@ function findWorkspacePackageDir(rootDir: string, packageName: string): string | const child = join(current.dir, entry.name); try { const pkg = JSON.parse(readFileSync(join(rootDir, child, "package.json"), "utf8")) as { name?: string }; - if (pkg.name === packageName) return child; + if (pkg.name === packageName) { + packageDirCache.set(cacheKey, child); + return child; + } } catch { if (current.depth < 3) queue.push({ dir: child, depth: current.depth + 1 }); } } } + packageDirCache.set(cacheKey, null); return null; } @@ -152,6 +163,7 @@ function toPackageRelativeFilter(token: string, rootDir: string, packageDir: str export function normalizeVerificationCommand(command: string, rootDir: string): { command: string; warnings: string[] } { const tokens = shellSplit(command); const warnings: string[] = []; + if (!tokens) return { command, warnings }; if (tokens[0] !== "pnpm") return { command, warnings }; const filterIndex = tokens.findIndex((token) => token === "--filter" || token === "-F"); @@ -182,8 +194,10 @@ export function normalizeVerificationCommand(command: string, rootDir: string): const hasReporter = vitestArgs.some((token) => token === "--reporter" || token.startsWith("--reporter=")); const hasSilent = vitestArgs.some((token) => token === "--silent" || token.startsWith("--silent=")); + const pnpmGlobalFlags = tokens.slice(1, filterIndex); const normalizedTokens = [ "pnpm", + ...pnpmGlobalFlags, "--filter", packageName, "exec",