fix(FN-1952): recover failed review tasks

This commit is contained in:
gsxdsm
2026-04-16 20:52:50 -07:00
parent 95ced20e40
commit a313037cca
15 changed files with 314 additions and 50 deletions

View File

@@ -0,0 +1,5 @@
---
"@gsxdsm/fusion": patch
---
Allow failed review tasks to retry from a fresh base and let engine agents read legacy Pi API keys while writing credentials to Fusion auth storage.

View File

@@ -452,13 +452,15 @@ describe("runDashboard — AuthStorage & ModelRegistry wiring", () => {
expect(AuthStorage.create).toHaveBeenCalledTimes(1);
});
it("creates ModelRegistry with the authStorage instance", async () => {
it("creates ModelRegistry with a merged auth storage reader", async () => {
const { ModelRegistry } = await import("@mariozechner/pi-coding-agent");
await runDashboard(0, {});
expect(ModelRegistry).toHaveBeenCalledTimes(1);
expect(ModelRegistry).toHaveBeenCalledWith(mockAuthStorage);
const registryAuthStorage = (ModelRegistry as ReturnType<typeof vi.fn>).mock.calls[0][0];
expect(registryAuthStorage).not.toBe(mockAuthStorage);
expect(registryAuthStorage.getApiKey).toBeTypeOf("function");
});
it("discovers extensions and registers extension providers", async () => {

View File

@@ -37,7 +37,7 @@ import {
} from "./task-lifecycle.js";
import { promptForPort } from "./port-prompt.js";
import { createReadOnlyProviderSettingsView } from "./provider-settings.js";
import { createReadOnlyAuthFileStorage, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { createReadOnlyAuthFileStorage, mergeAuthStorageReads, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { getFusionAuthPath, getLegacyAuthPaths } from "./auth-paths.js";
const DIAGNOSTIC_INTERVAL_MS = 30 * 60 * 1000; // 30 minutes
@@ -326,9 +326,10 @@ export async function runDaemon(opts: DaemonOptions = {}) {
const automationStore = cwdEngine.getAutomationStore();
const authStorage = AuthStorage.create(getFusionAuthPath());
const modelRegistry = new ModelRegistry(authStorage);
const legacyAuthStorage = createReadOnlyAuthFileStorage(getLegacyAuthPaths());
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(authStorage, modelRegistry, [legacyAuthStorage]);
const mergedAuthStorage = mergeAuthStorageReads(authStorage, [legacyAuthStorage]);
const modelRegistry = new ModelRegistry(mergedAuthStorage);
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(mergedAuthStorage, modelRegistry);
// PackageManager may be used for skills adapter even if extension loading fails
let packageManager: DefaultPackageManager | undefined;

View File

@@ -9,7 +9,7 @@ import {
} from "./task-lifecycle.js";
import { promptForPort } from "./port-prompt.js";
import { createReadOnlyProviderSettingsView, createProjectSettingsPersistence } from "./provider-settings.js";
import { createReadOnlyAuthFileStorage, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { createReadOnlyAuthFileStorage, mergeAuthStorageReads, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { getFusionAuthPath, getLegacyAuthPaths } from "./auth-paths.js";
// Re-export for backward compatibility with tests
@@ -363,9 +363,10 @@ export async function runDashboard(port: number, opts: { paused?: boolean; dev?:
// Passing these to createServer enables the dashboard's Authentication
// tab (login/logout) and Model selector.
const authStorage = AuthStorage.create(getFusionAuthPath());
const modelRegistry = new ModelRegistry(authStorage);
const legacyAuthStorage = createReadOnlyAuthFileStorage(getLegacyAuthPaths());
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(authStorage, modelRegistry, [legacyAuthStorage]);
const mergedAuthStorage = mergeAuthStorageReads(authStorage, [legacyAuthStorage]);
const modelRegistry = new ModelRegistry(mergedAuthStorage);
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(mergedAuthStorage, modelRegistry);
// PackageManager may be used for skills adapter even if extension loading fails
let packageManager: DefaultPackageManager | undefined;

View File

@@ -2,7 +2,7 @@ import { describe, expect, it, vi } from "vitest";
import { existsSync, mkdirSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createReadOnlyAuthFileStorage, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { createReadOnlyAuthFileStorage, mergeAuthStorageReads, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
function makeAuthStorage(credentials: Record<string, { type: string; key?: string }> = {}) {
return {
@@ -18,6 +18,8 @@ function makeAuthStorage(credentials: Record<string, { type: string; key?: strin
delete credentials[provider];
}),
get: vi.fn((provider: string) => credentials[provider]),
getAll: vi.fn(() => ({ ...credentials })),
list: vi.fn(() => Object.keys(credentials)),
getApiKey: vi.fn(async (provider: string) => credentials[provider]?.key),
} as any;
}
@@ -67,6 +69,23 @@ describe("wrapAuthStorageWithApiKeyProviders", () => {
expect(legacyAuth.reload).toHaveBeenCalledTimes(1);
});
it("creates an AuthStorage-compatible merged reader for ModelRegistry", async () => {
const fusionAuth = makeAuthStorage({
openrouter: { type: "api_key", key: "fusion-key" },
});
const legacyAuth = makeAuthStorage({
minimax: { type: "api_key", key: "legacy-minimax-key" },
});
const merged = mergeAuthStorageReads(fusionAuth, [legacyAuth]);
expect(await merged.getApiKey("openrouter")).toBe("fusion-key");
expect(await merged.getApiKey("minimax")).toBe("legacy-minimax-key");
expect(merged.get("minimax")).toEqual({ type: "api_key", key: "legacy-minimax-key" });
expect(merged.list()).toEqual(expect.arrayContaining(["openrouter", "minimax"]));
});
it("reads legacy auth JSON without creating missing files", async () => {
const tempDir = join(tmpdir(), `fusion-provider-auth-${process.pid}-${Date.now()}`);
const legacyAgentDir = join(tempDir, ".pi", "agent");

View File

@@ -25,6 +25,8 @@ interface ReadFallbackAuthStorage {
hasAuth(provider: string): boolean;
getApiKey(providerId: string): Promise<string | undefined>;
get(providerId: string): { type?: string; key?: string } | undefined;
getAll(): Record<string, { type?: string; key?: string }>;
list(): string[];
}
const BUILT_IN_API_KEY_PROVIDERS: Array<{ id: string; name: string }> = [
@@ -54,32 +56,21 @@ export function wrapAuthStorageWithApiKeyProviders(
modelRegistry: ModelRegistry,
readFallbackAuthStorages: ReadFallbackAuthStorage[] = [],
): DashboardAuthStorage {
const readAuthStorages = [authStorage, ...readFallbackAuthStorages];
const getCredential = (providerId: string) => {
for (const storage of readAuthStorages) {
const credential = storage.get(providerId);
if (credential) return credential;
}
return undefined;
};
const mergedAuthStorage = mergeAuthStorageReads(authStorage, readFallbackAuthStorages);
return {
reload: () => {
for (const storage of readAuthStorages) {
storage.reload();
}
},
reload: () => mergedAuthStorage.reload(),
getOAuthProviders: () =>
authStorage
mergedAuthStorage
.getOAuthProviders()
.map((provider) => ({ id: provider.id, name: provider.name })),
hasAuth: (provider) => readAuthStorages.some((storage) => storage.hasAuth(provider)),
hasAuth: (provider) => mergedAuthStorage.hasAuth(provider),
login: (providerId, callbacks) =>
authStorage.login(providerId as Parameters<AuthStorage["login"]>[0], callbacks),
logout: (provider) => authStorage.logout(provider),
mergedAuthStorage.login(providerId as Parameters<AuthStorage["login"]>[0], callbacks),
logout: (provider) => mergedAuthStorage.logout(provider),
getApiKeyProviders: () => {
const oauthProviderIds = new Set(
authStorage.getOAuthProviders().map((provider) => provider.id),
mergedAuthStorage.getOAuthProviders().map((provider) => provider.id),
);
const providers = new Map<string, string>();
@@ -102,26 +93,84 @@ export function wrapAuthStorageWithApiKeyProviders(
);
},
setApiKey: (providerId, apiKey) => {
authStorage.set(providerId, { type: "api_key", key: apiKey });
mergedAuthStorage.set(providerId, { type: "api_key", key: apiKey });
},
clearApiKey: (providerId) => {
authStorage.remove(providerId);
mergedAuthStorage.remove(providerId);
},
hasApiKey: (providerId) => {
const credential = getCredential(providerId);
const credential = mergedAuthStorage.get(providerId);
return credential?.type === "api_key" && !!credential.key;
},
getApiKey: async (providerId) => {
for (const storage of readAuthStorages) {
const apiKey = await storage.getApiKey(providerId);
if (apiKey) return apiKey;
}
return undefined;
},
get: getCredential,
getApiKey: (providerId) => mergedAuthStorage.getApiKey(providerId),
get: (providerId) => mergedAuthStorage.get(providerId),
};
}
export function mergeAuthStorageReads(
authStorage: AuthStorage,
readFallbackAuthStorages: ReadFallbackAuthStorage[] = [],
): AuthStorage {
const readAuthStorages = [authStorage, ...readFallbackAuthStorages];
const getCredential = (providerId: string) => {
for (const storage of readAuthStorages) {
const credential = storage.get(providerId);
if (credential) return credential;
}
return undefined;
};
return new Proxy(authStorage, {
get(target, prop, receiver) {
if (prop === "reload") {
return () => {
for (const storage of readAuthStorages) {
storage.reload();
}
};
}
if (prop === "get") {
return getCredential;
}
if (prop === "has") {
return (provider: string) => readAuthStorages.some((storage) => Boolean(storage.get(provider)));
}
if (prop === "hasAuth") {
return (provider: string) => readAuthStorages.some((storage) => storage.hasAuth(provider));
}
if (prop === "getAll") {
return () => ({
...readFallbackAuthStorages.reduce(
(merged, storage) => ({ ...merged, ...storage.getAll() }),
{} as Record<string, { type?: string; key?: string }>,
),
...target.getAll(),
});
}
if (prop === "list") {
return () => Array.from(new Set(readAuthStorages.flatMap((storage) => storage.list())));
}
if (prop === "getApiKey") {
return async (providerId: string) => {
for (const storage of readAuthStorages) {
const apiKey = await storage.getApiKey(providerId);
if (apiKey) return apiKey;
}
return undefined;
};
}
return Reflect.get(target, prop, receiver);
},
}) as AuthStorage;
}
export function createReadOnlyAuthFileStorage(authPaths: string[]): ReadFallbackAuthStorage {
let credentials: Record<string, { type?: string; key?: string }> = {};
@@ -149,6 +198,8 @@ export function createReadOnlyAuthFileStorage(authPaths: string[]): ReadFallback
reload,
hasAuth: (provider) => Boolean(credentials[provider]),
get: (provider) => credentials[provider],
getAll: () => ({ ...credentials }),
list: () => Object.keys(credentials),
getApiKey: async (provider) => {
const credential = credentials[provider];
return credential?.type === "api_key" ? credential.key : undefined;

View File

@@ -38,7 +38,7 @@ import {
} from "./task-lifecycle.js";
import { promptForPort } from "./port-prompt.js";
import { createReadOnlyProviderSettingsView, createProjectSettingsPersistence } from "./provider-settings.js";
import { createReadOnlyAuthFileStorage, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { createReadOnlyAuthFileStorage, mergeAuthStorageReads, wrapAuthStorageWithApiKeyProviders } from "./provider-auth.js";
import { getFusionAuthPath, getLegacyAuthPaths } from "./auth-paths.js";
const DIAGNOSTIC_INTERVAL_MS = 30 * 60 * 1000; // 30 minutes
@@ -385,9 +385,10 @@ export async function runServe(
const automationStore = cwdEngine.getAutomationStore();
const authStorage = AuthStorage.create(getFusionAuthPath());
const modelRegistry = new ModelRegistry(authStorage);
const legacyAuthStorage = createReadOnlyAuthFileStorage(getLegacyAuthPaths());
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(authStorage, modelRegistry, [legacyAuthStorage]);
const mergedAuthStorage = mergeAuthStorageReads(authStorage, [legacyAuthStorage]);
const modelRegistry = new ModelRegistry(mergedAuthStorage);
const dashboardAuthStorage = wrapAuthStorageWithApiKeyProviders(mergedAuthStorage, modelRegistry);
// PackageManager may be used for skills adapter even if extension loading fails
let packageManager: DefaultPackageManager | undefined;

View File

@@ -1735,7 +1735,16 @@ describe("runTaskRetry", () => {
await runTaskRetry("FN-001");
expect(mockGetTask).toHaveBeenCalledWith("FN-001");
expect(mockUpdateTask).toHaveBeenCalledWith("FN-001", { status: null, error: null });
expect(mockUpdateTask).toHaveBeenCalledWith("FN-001", {
status: null,
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
recoveryRetryCount: null,
nextRecoveryAt: null,
});
expect(mockMoveTask).toHaveBeenCalledWith("FN-001", "todo");
expect(mockLogEntry).toHaveBeenCalledWith("FN-001", "Retry requested from CLI", "Task reset to todo for retry");
@@ -1786,7 +1795,16 @@ describe("runTaskRetry", () => {
await runTaskRetry("FN-001");
expect(mockGetTask).toHaveBeenCalledWith("FN-001");
expect(mockUpdateTask).toHaveBeenCalledWith("FN-001", { status: null, error: null });
expect(mockUpdateTask).toHaveBeenCalledWith("FN-001", {
status: null,
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
recoveryRetryCount: null,
nextRecoveryAt: null,
});
expect(mockMoveTask).toHaveBeenCalledWith("FN-001", "todo");
expect(mockLogEntry).toHaveBeenCalledWith("FN-001", "Retry requested from CLI", "Task reset to todo for retry");

View File

@@ -639,8 +639,17 @@ export async function runTaskRetry(id: string, projectName?: string) {
throw new Error(`Task ${id} is not in a retryable state (status: ${task.status || 'none'})`);
}
// Clear failure state
await store.updateTask(id, { status: null, error: null });
// Clear failure state and stale branch refs so retry can choose a fresh base.
await store.updateTask(id, {
status: null,
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
recoveryRetryCount: null,
nextRecoveryAt: null,
});
// Move to todo column
await store.moveTask(id, 'todo');

View File

@@ -1464,6 +1464,8 @@ describe("POST /tasks/:id/retry", () => {
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
stuckKillCount: 0,
recoveryRetryCount: null,
nextRecoveryAt: null,
@@ -1500,6 +1502,8 @@ describe("POST /tasks/:id/retry", () => {
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
stuckKillCount: 0,
recoveryRetryCount: null,
nextRecoveryAt: null,
@@ -1524,6 +1528,8 @@ describe("POST /tasks/:id/retry", () => {
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
stuckKillCount: 0,
recoveryRetryCount: null,
nextRecoveryAt: null,
@@ -1571,6 +1577,8 @@ describe("POST /tasks/:id/retry", () => {
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
stuckKillCount: 0,
recoveryRetryCount: null,
nextRecoveryAt: null,

View File

@@ -3263,6 +3263,8 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
error: null,
worktree: null,
branch: null,
baseBranch: null,
baseCommitSha: null,
stuckKillCount: 0,
recoveryRetryCount: null,
nextRecoveryAt: null,

View File

@@ -0,0 +1,52 @@
import { afterEach, beforeEach, describe, expect, it } from "vitest";
import { mkdirSync, writeFileSync, existsSync } from "node:fs";
import { mkdtemp } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createFusionAuthStorage, getFusionAuthPath } from "./auth-storage.js";
describe("createFusionAuthStorage", () => {
const originalHome = process.env.HOME;
let homeDir: string;
beforeEach(async () => {
homeDir = await mkdtemp(join(tmpdir(), "fusion-engine-auth-"));
process.env.HOME = homeDir;
});
afterEach(() => {
if (originalHome === undefined) {
delete process.env.HOME;
} else {
process.env.HOME = originalHome;
}
});
it("writes to Fusion auth and reads legacy Pi auth as fallback", async () => {
const legacyAgentDir = join(homeDir, ".pi", "agent");
mkdirSync(legacyAgentDir, { recursive: true });
writeFileSync(
join(legacyAgentDir, "auth.json"),
JSON.stringify({
openrouter: { type: "api_key", key: "legacy-openrouter-key" },
minimax: { type: "api_key", key: "legacy-minimax-key" },
}),
);
const authStorage = createFusionAuthStorage();
authStorage.set("openrouter", { type: "api_key", key: "fusion-openrouter-key" });
expect(await authStorage.getApiKey("openrouter")).toBe("fusion-openrouter-key");
expect(await authStorage.getApiKey("minimax")).toBe("legacy-minimax-key");
expect(authStorage.get("minimax")).toEqual({ type: "api_key", key: "legacy-minimax-key" });
expect(existsSync(getFusionAuthPath(homeDir))).toBe(true);
});
it("does not create missing legacy Pi auth files", async () => {
const authStorage = createFusionAuthStorage();
expect(await authStorage.getApiKey("openrouter")).toBeUndefined();
expect(existsSync(join(homeDir, ".pi", "agent", "auth.json"))).toBe(false);
expect(existsSync(join(homeDir, ".pi", "auth.json"))).toBe(false);
});
});

View File

@@ -0,0 +1,94 @@
import { existsSync, readFileSync } from "node:fs";
import { homedir } from "node:os";
import { join } from "node:path";
import { AuthStorage } from "@mariozechner/pi-coding-agent";
type StoredCredential = { type?: string; key?: string };
function getHomeDir(): string {
return process.env.HOME || process.env.USERPROFILE || homedir();
}
export function getFusionAuthPath(home = getHomeDir()): string {
return join(home, ".fusion", "agent", "auth.json");
}
function getLegacyAuthPaths(home = getHomeDir()): string[] {
return [
join(home, ".pi", "agent", "auth.json"),
join(home, ".pi", "auth.json"),
];
}
function readLegacyCredentials(authPaths = getLegacyAuthPaths()): Record<string, StoredCredential> {
const credentials: Record<string, StoredCredential> = {};
for (const authPath of authPaths) {
if (!existsSync(authPath)) {
continue;
}
try {
const parsed = JSON.parse(readFileSync(authPath, "utf-8")) as Record<string, StoredCredential>;
for (const [provider, credential] of Object.entries(parsed)) {
credentials[provider] ??= credential;
}
} catch {
// Ignore invalid legacy auth files and continue with other candidates.
}
}
return credentials;
}
function resolveStoredApiKey(key: string | undefined): string | undefined {
if (!key) return undefined;
return process.env[key] ?? key;
}
export function createFusionAuthStorage(): AuthStorage {
const primary = AuthStorage.create(getFusionAuthPath());
let legacyCredentials = readLegacyCredentials();
return new Proxy(primary, {
get(target, prop, receiver) {
if (prop === "reload") {
return () => {
target.reload();
legacyCredentials = readLegacyCredentials();
};
}
if (prop === "get") {
return (provider: string) => target.get(provider) ?? legacyCredentials[provider];
}
if (prop === "has") {
return (provider: string) => target.has(provider) || provider in legacyCredentials;
}
if (prop === "hasAuth") {
return (provider: string) => target.hasAuth(provider) || Boolean(legacyCredentials[provider]);
}
if (prop === "getAll") {
return () => ({ ...legacyCredentials, ...target.getAll() });
}
if (prop === "list") {
return () => Array.from(new Set([...Object.keys(legacyCredentials), ...target.list()]));
}
if (prop === "getApiKey") {
return async (provider: string) => {
const primaryKey = await target.getApiKey(provider);
if (primaryKey) return primaryKey;
const credential = legacyCredentials[provider];
return credential?.type === "api_key" ? resolveStoredApiKey(credential.key) : undefined;
};
}
return Reflect.get(target, prop, receiver);
},
}) as AuthStorage;
}

View File

@@ -13,7 +13,7 @@ import { Type, type Static } from "@mariozechner/pi-ai";
import { createKbAgent, describeModel, promptWithFallback, compactSessionContext } from "./pi.js";
import { buildSessionSkillContext } from "./session-skill-context.js";
import { reviewStep, type ReviewVerdict } from "./reviewer.js";
import { AuthStorage, ModelRegistry, SessionManager, getAgentDir, type ToolDefinition, type AgentSession } from "@mariozechner/pi-coding-agent";
import { ModelRegistry, SessionManager, getAgentDir, type ToolDefinition, type AgentSession } from "@mariozechner/pi-coding-agent";
import { PRIORITY_EXECUTE, type AgentSemaphore } from "./concurrency.js";
import { isRegisteredGitWorktree, isUsableTaskWorktree, type WorktreePool } from "./worktree-pool.js";
import { AgentLogger } from "./agent-logger.js";
@@ -42,6 +42,7 @@ import {
createTaskLogTool as sharedCreateTaskLogTool,
} from "./agent-tools.js";
import { getTaskCompletionBlockerForStore } from "./task-completion.js";
import { createFusionAuthStorage } from "./auth-storage.js";
// Re-export for backward compatibility (tests import from executor.ts)
export { summarizeToolArgs } from "./agent-logger.js";
@@ -390,7 +391,7 @@ export class TaskExecutor {
private get modelRegistry(): InstanceType<typeof ModelRegistry> {
if (!this._modelRegistry) {
const authStorage = AuthStorage.create();
const authStorage = createFusionAuthStorage();
this._modelRegistry = new ModelRegistry(authStorage, join(getAgentDir(), "models.json"));
this._modelRegistry.refresh();
}

View File

@@ -1,7 +1,7 @@
/**
* Shared pi SDK setup for fn engine agents.
*
* Uses the user's existing pi auth (API keys / OAuth from ~/.pi/agent/auth.json).
* Uses Fusion auth for writes and legacy pi auth as a read-only fallback.
* Provides factory functions for creating triage and executor agent sessions.
*/
@@ -13,7 +13,6 @@ import { join, relative, isAbsolute, resolve } from "node:path";
const execAsync = promisify(exec);
import {
AuthStorage,
createAgentSession,
createCodingTools,
createExtensionRuntime,
@@ -34,6 +33,7 @@ import {
type SkillSelectionContext,
} from "./skill-resolver.js";
import { isContextLimitError } from "./context-limit-detector.js";
import { createFusionAuthStorage } from "./auth-storage.js";
export interface AgentResult {
session: AgentSession;
@@ -474,7 +474,7 @@ export function wrapToolsWithBoundary(
*/
export async function createKbAgent(options: AgentOptions): Promise<AgentResult> {
console.error(`[pi] createKbAgent called (cwd=${options.cwd}, tools=${options.tools}, provider=${options.defaultProvider}, model=${options.defaultModelId})`);
const authStorage = AuthStorage.create();
const authStorage = createFusionAuthStorage();
const modelRegistry = new ModelRegistry(authStorage, join(getAgentDir(), "models.json"));
await registerExtensionProviders(options.cwd, modelRegistry);