From 212e03219da9b24d12e9f429053998309fa10a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Wed, 22 Apr 2026 15:48:00 -0400 Subject: [PATCH] fix(artifacts): classify projection filesystem errors --- src/main/agent-runtime/artifact-projector.ts | 122 +++++++++++++++---- src/main/agent-runtime/hook-handlers.ts | 1 + src/main/agent-runtime/runtime-bridge.ts | 3 +- tests/unit/artifact-projector-errors.test.ts | 86 +++++++++++++ 4 files changed, 190 insertions(+), 22 deletions(-) create mode 100644 tests/unit/artifact-projector-errors.test.ts diff --git a/src/main/agent-runtime/artifact-projector.ts b/src/main/agent-runtime/artifact-projector.ts index 32f8f1d7..ae1f9a31 100644 --- a/src/main/agent-runtime/artifact-projector.ts +++ b/src/main/agent-runtime/artifact-projector.ts @@ -4,9 +4,10 @@ // modules. If 2026-05-20 passes without decomposition, a follow-up session // MUST revisit and either decompose or justify the ceiling exception. import { isAbsolute, relative, resolve } from "node:path"; -import { execFileSync } from "node:child_process"; +import { execFileSync, spawnSync } from "node:child_process"; import { readdirSync, statSync } from "node:fs"; import type { AppDatabase } from "@main/database"; +import { logAppError, type Logger } from "../errors/index.js"; /** * Filesystem artifact protocol for pi tool invocations. @@ -84,6 +85,7 @@ export type ArtifactProjectorRunContext = { taskId: string | null; workspaceId: string | null; workspacePath: string | null; + logger?: Logger; /** * mtime snapshot captured at run start for workspaces that are NOT * a git repo. Diff-against-baseline is how `projectBashArtifacts` @@ -114,6 +116,28 @@ const BASELINE_SKIP_DIR_NAMES: ReadonlySet = new Set([ ".pnpm-store", ]); +const DEFAULT_ARTIFACT_LOGGER: Logger = console; + +function logArtifactFsError( + logger: Logger | undefined, + error: unknown, + code: string, + message: string, + context: Record, +): void { + logAppError( + logger ?? DEFAULT_ARTIFACT_LOGGER, + error, + { + kind: "recoverable", + code, + message, + context, + }, + "warn", + ); +} + /** * Capture a run-start mtime baseline only for workspaces that need * one. Git workspaces return null here because `git status` already @@ -130,12 +154,22 @@ const BASELINE_SKIP_DIR_NAMES: ReadonlySet = new Set([ * still covers every path that was captured while the operator sees * the cap signal. */ -export function captureBaselineIfRequired(workspacePath: string | null): WorkspaceBaseline | null { +export function captureBaselineIfRequired( + workspacePath: string | null, + logger?: Logger, +): WorkspaceBaseline | null { if (!workspacePath) return null; if (isGitRepo(workspacePath)) return null; try { if (!statSync(workspacePath).isDirectory()) return null; - } catch { + } catch (error) { + logArtifactFsError( + logger, + error, + "artifact.fs.workspace_stat_failed", + "Failed to stat workspace before artifact baseline capture.", + { workspacePath }, + ); return null; } const mtimeByRelPath = new Map(); @@ -151,7 +185,14 @@ export function captureBaselineIfRequired(workspacePath: string | null): Workspa let entries: { name: string; isDirectory: () => boolean; isSymbolicLink: () => boolean }[]; try { entries = readdirSync(dir, { withFileTypes: true }); - } catch { + } catch (error) { + logArtifactFsError( + logger, + error, + "artifact.fs.baseline_readdir_failed", + "Failed to read workspace directory during artifact baseline capture.", + { workspacePath, dir }, + ); continue; } for (const entry of entries) { @@ -168,7 +209,14 @@ export function captureBaselineIfRequired(workspacePath: string | null): Workspa const rel = relative(workspacePath, abs); if (rel.length === 0) continue; mtimeByRelPath.set(rel, stat.mtimeMs); - } catch { + } catch (error) { + logArtifactFsError( + logger, + error, + "artifact.fs.baseline_stat_failed", + "Failed to stat workspace file during artifact baseline capture.", + { workspacePath, absPath: abs }, + ); continue; } if (mtimeByRelPath.size >= MAX_BASELINE_ENTRIES) { @@ -366,7 +414,11 @@ function retireDeletedArtifacts( * emit a `baseline_truncated` outcome so operators see the degraded * signal. */ -function diffAgainstBaseline(workspacePath: string, baseline: WorkspaceBaseline): GitChangeSet { +function diffAgainstBaseline( + workspacePath: string, + baseline: WorkspaceBaseline, + logger: Logger | undefined, +): GitChangeSet { const changed: string[] = []; const seenRels = new Set(); const stack: string[] = [workspacePath]; @@ -376,7 +428,14 @@ function diffAgainstBaseline(workspacePath: string, baseline: WorkspaceBaseline) let entries: { name: string; isDirectory: () => boolean; isSymbolicLink: () => boolean }[]; try { entries = readdirSync(dir, { withFileTypes: true }); - } catch { + } catch (error) { + logArtifactFsError( + logger, + error, + "artifact.fs.diff_readdir_failed", + "Failed to read workspace directory during artifact diff.", + { workspacePath, dir }, + ); continue; } for (const entry of entries) { @@ -392,7 +451,14 @@ function diffAgainstBaseline(workspacePath: string, baseline: WorkspaceBaseline) const stat = statSync(abs); if (!stat.isFile()) continue; mtimeMs = stat.mtimeMs; - } catch { + } catch (error) { + logArtifactFsError( + logger, + error, + "artifact.fs.diff_stat_failed", + "Failed to stat workspace file during artifact diff.", + { workspacePath, absPath: abs }, + ); continue; } const rel = relative(workspacePath, abs); @@ -458,12 +524,19 @@ function collectChangedPaths(ctx: ArtifactProjectorRunContext): ChangeSet { try { const gitSet = listChangedRelPaths(ctx.workspacePath); return { source: "git", changed: gitSet.changed, deleted: gitSet.deleted }; - } catch { + } catch (error) { + logArtifactFsError( + ctx.logger, + error, + "artifact.fs.git_status_failed", + "Failed to read git status during artifact projection.", + { workspacePath: ctx.workspacePath, runId: ctx.runId }, + ); return { source: "none", changed: [], deleted: [], skipReason: "git_status_failed" }; } } if (ctx.workspaceBaseline) { - const diff = diffAgainstBaseline(ctx.workspacePath, ctx.workspaceBaseline); + const diff = diffAgainstBaseline(ctx.workspacePath, ctx.workspaceBaseline, ctx.logger); return { source: "baseline", changed: diff.changed, @@ -510,16 +583,12 @@ function projectBashArtifacts( * itself would accept the directory, which is the semantics we want. */ function isGitRepo(workspacePath: string): boolean { - try { - execFileSync("git", ["rev-parse", "--is-inside-work-tree"], { - cwd: workspacePath, - timeout: 2_000, - stdio: ["ignore", "ignore", "ignore"], - }); - return true; - } catch { - return false; - } + const result = spawnSync("git", ["rev-parse", "--is-inside-work-tree"], { + cwd: workspacePath, + timeout: 2_000, + stdio: ["ignore", "ignore", "ignore"], + }); + return result.status === 0; } interface GitChangeSet { @@ -597,7 +666,18 @@ function upsertFilesystemArtifact( let stat: ReturnType; try { stat = statSync(absPath); - } catch { + } catch (error) { + logArtifactFsError( + ctx.logger, + error, + "artifact.fs.path_stat_failed", + "Failed to stat filesystem artifact before projection.", + { + workspacePath, + absPath, + runId: ctx.runId, + }, + ); return { kind: "skipped", reason: "path_missing" }; } if (!stat.isFile()) { diff --git a/src/main/agent-runtime/hook-handlers.ts b/src/main/agent-runtime/hook-handlers.ts index e1c0c36b..99e94af2 100644 --- a/src/main/agent-runtime/hook-handlers.ts +++ b/src/main/agent-runtime/hook-handlers.ts @@ -162,6 +162,7 @@ export function handleAfterToolCall( taskId: ctx.taskId, workspaceId: ctx.workspaceId, workspacePath: ctx.workspacePath, + logger: deps.logger, workspaceBaseline: ctx.workspaceBaseline, }; try { diff --git a/src/main/agent-runtime/runtime-bridge.ts b/src/main/agent-runtime/runtime-bridge.ts index a18138e7..9f1e6ae6 100644 --- a/src/main/agent-runtime/runtime-bridge.ts +++ b/src/main/agent-runtime/runtime-bridge.ts @@ -195,6 +195,7 @@ export class RuntimeBridge { taskId: ctx.taskId, workspaceId: ctx.workspaceId, workspacePath: ctx.workspacePath, + logger: this.deps.logger, workspaceBaseline: ctx.workspaceBaseline, }); const inserted = outcomes.filter((o) => o.kind === "inserted").length; @@ -643,7 +644,7 @@ export class RuntimeBridge { // `git status` is the source of truth there, and paying for an // O(tree) scan on every run just to throw the result away is a // hot-path tax the scheduler should not be paying. - const baseline = captureBaselineIfRequired(payload.workspacePath); + const baseline = captureBaselineIfRequired(payload.workspacePath, this.deps.logger); this.registerActiveRun({ runId, agentId: payload.agentId, diff --git a/tests/unit/artifact-projector-errors.test.ts b/tests/unit/artifact-projector-errors.test.ts new file mode 100644 index 00000000..ce44e032 --- /dev/null +++ b/tests/unit/artifact-projector-errors.test.ts @@ -0,0 +1,86 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { AppDatabase } from "../../src/main/database"; +import { + captureBaselineIfRequired, + projectToolExecutionToWorkProduct, + type ArtifactProjectorRunContext, +} from "../../src/main/agent-runtime/artifact-projector"; + +const tempDirs: string[] = []; + +async function createTempDir(prefix: string): Promise { + const dir = await mkdtemp(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function createLogger() { + return { + warn: vi.fn(), + error: vi.fn(), + }; +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +describe("artifact projector error taxonomy", () => { + it("classifies unreadable workspace baseline capture failures", async () => { + const logger = createLogger(); + const parent = await createTempDir("agentcompany-missing-workspace-parent-"); + const workspacePath = join(parent, "missing-workspace"); + + const baseline = captureBaselineIfRequired(workspacePath, logger); + + expect(baseline).toBeNull(); + expect(logger.warn).toHaveBeenCalledWith( + "[artifact.fs.workspace_stat_failed] Failed to stat workspace before artifact baseline capture.", + { workspacePath }, + expect.stringContaining("cause="), + ); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it("classifies missing filesystem artifact projection failures", async () => { + const logger = createLogger(); + const workspacePath = await createTempDir("agentcompany-artifact-errors-"); + const absPath = join(workspacePath, "missing.md"); + const ctx: ArtifactProjectorRunContext = { + runId: "run-artifact-error", + agentId: "agent-artifact-error", + companyId: "company-artifact-error", + taskId: "task-artifact-error", + workspaceId: "workspace-artifact-error", + workspacePath, + logger, + workspaceBaseline: null, + }; + + const outcomes = projectToolExecutionToWorkProduct( + {} as AppDatabase, + ctx, + { + toolName: "write", + args: { path: "missing.md" }, + result: null, + isError: false, + }, + ); + + expect(outcomes).toEqual([{ kind: "skipped", reason: "path_missing" }]); + expect(logger.warn).toHaveBeenCalledWith( + "[artifact.fs.path_stat_failed] Failed to stat filesystem artifact before projection.", + { + workspacePath, + absPath, + runId: "run-artifact-error", + }, + expect.stringContaining("cause="), + ); + expect(logger.error).not.toHaveBeenCalled(); + }); +});