mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-08 00:54:38 +02:00
fix: record safe sandbox restore failure diagnostics (#14064)
Record a bounded diagnostic for failed workspace and staged-asset restores. Preserve the original error, retry policy, and archive safety checks. Never copy raw provider messages, credentials, paths, or asset names into the log. Nested failures log once; safe fields survive throwing property getters. Verified 129 focused restore/Claude tests, typecheck/build, and green full PR CI. Greptile 5/5 with all review threads resolved. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
c3ddb288b1
commit
4ca404b49a
6 files changed
+210
-13
No files matched your search
@@ -193,6 +193,18 @@ the successor to the original failed run. Repair does not reset the automatic
|
||||
retry budget. Transient failures retain the existing
|
||||
bounded retry policy. Archive confinement remains required.
|
||||
|
||||
Sandbox restore tasks also write a `Workspace restore diagnostic` line to the
|
||||
run log on failure. `phase` is `workspace` or `asset`, so a failed staged-asset
|
||||
copy-back (such as credentials) can be distinguished from workspace restoration. The line
|
||||
contains only an allowlisted OS/transport `errorCode` (otherwise `unknown`), an
|
||||
optional numeric HTTP error status, and an optional bounded process exit code.
|
||||
Up to four nested causes are inspected. Messages, URLs, filesystem paths, asset
|
||||
names, credentials, and response bodies are excluded. Every failed outbound
|
||||
task emits its own diagnostic; nested repository failures are logged once by
|
||||
the enclosing workspace task. The original error and restore safety policy are
|
||||
unchanged. These lines stay in the instance run log and its configured durable
|
||||
storage, and are not new first-party telemetry events.
|
||||
|
||||
## Codex resume usage snapshot
|
||||
|
||||
The native runner retains a bounded local `harness.diagnostic` event with code
|
||||
|
||||
@@ -3862,6 +3862,34 @@ describe("sandbox managed runtime outbound coordinator", () => {
|
||||
]);
|
||||
}
|
||||
|
||||
it("records each failed restore phase without changing stable failure ordering", async () => {
|
||||
const { workspaceDir, remoteWorkspaceDir, dirOf } = await makeOutboundDirs(["home", "private-asset"]);
|
||||
const { control, gate } = makeOutboundControl();
|
||||
const client = makeGatedOutboundClient(true, gate);
|
||||
const workspaceError = Object.assign(new Error("private workspace path"), { code: "EACCES" });
|
||||
control.failWith("workspace", workspaceError);
|
||||
control.failWith("home", Object.assign(new Error("private credential"), { status: 404 }));
|
||||
control.failWith("private-asset", new Error("private asset error"));
|
||||
const prepared = await prepareSandboxManagedRuntime({
|
||||
spec: makeSpec(remoteWorkspaceDir), adapterKey: "codex", client, workspaceLocalDir: workspaceDir,
|
||||
assets: [
|
||||
makeControlledAsset("home", dirOf("home"), control, gate),
|
||||
makeControlledAsset("private-asset", dirOf("private-asset"), control, gate),
|
||||
],
|
||||
});
|
||||
const lines: string[] = [];
|
||||
await expect(prepared.restoreWorkspace((line) => { lines.push(line); })).rejects.toBe(workspaceError);
|
||||
const diagnostics = lines.filter((line) => line.includes("Workspace restore diagnostic:"));
|
||||
expect(diagnostics).toHaveLength(3);
|
||||
expect(diagnostics).toEqual(expect.arrayContaining([
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"workspace","errorCode":"EACCES"}\n',
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"asset","errorCode":"unknown","httpStatus":404}\n',
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"asset","errorCode":"unknown"}\n',
|
||||
]));
|
||||
expect(diagnostics.join("")).not.toContain("private");
|
||||
expect(control.settled).toEqual(expect.arrayContaining(["workspace", "home", "private-asset"]));
|
||||
});
|
||||
|
||||
it("with concurrency permitted, an asset restore starts while the workspace restore is held open", async () => {
|
||||
const { workspaceDir, remoteWorkspaceDir, dirOf } = await makeOutboundDirs(["home"]);
|
||||
const { control, gate } = makeOutboundControl();
|
||||
|
||||
@@ -45,6 +45,7 @@ import {
|
||||
type SyncOperationTask,
|
||||
} from "./sync-operation-schedule.js";
|
||||
import type { RuntimeSpanRunner } from "./acpx-engine/startup-timing.js";
|
||||
import { withWorkspaceRestoreDiagnostics } from "./workspace-restore-diagnostics.js";
|
||||
|
||||
const execFile = promisify(execFileCallback);
|
||||
const SANDBOX_WORKSPACE_HEAVY_DIR_NAMES = [
|
||||
@@ -1674,7 +1675,7 @@ export async function prepareSandboxManagedRuntime(input: {
|
||||
// tasks never share scratch state.
|
||||
if (syncWorkspace) {
|
||||
outboundTasks.push(() =>
|
||||
runStepSpan("restore.workspace", async () => {
|
||||
withWorkspaceRestoreDiagnostics("workspace", () => runStepSpan("restore.workspace", async () => {
|
||||
// Each repository owns its Git history and merge. The parent baseline also
|
||||
// records child files so restart recovery has their original merge inputs.
|
||||
for (const repository of repositories) {
|
||||
@@ -1900,7 +1901,7 @@ export async function prepareSandboxManagedRuntime(input: {
|
||||
}
|
||||
}
|
||||
});
|
||||
}),
|
||||
}), restoreSink),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1912,15 +1913,17 @@ export async function prepareSandboxManagedRuntime(input: {
|
||||
const assetRestore = asset.restore;
|
||||
const assetKey = asset.key;
|
||||
outboundTasks.push(() =>
|
||||
runStepSpan(`restore.asset.${assetKey}`, async () => {
|
||||
await withTempDir("paperclip-sandbox-restore-", async (tempDir) => {
|
||||
await assetRestore({
|
||||
assetDir: path.posix.join(runtimeRootDir, assetKey),
|
||||
readFile: async (remotePath) => toBuffer(await input.client.readFile(remotePath)),
|
||||
tempDir,
|
||||
withWorkspaceRestoreDiagnostics(
|
||||
"asset",
|
||||
() => runStepSpan(`restore.asset.${assetKey}`, async () => {
|
||||
await withTempDir("paperclip-sandbox-restore-", async (tempDir) => {
|
||||
await assetRestore({
|
||||
assetDir: path.posix.join(runtimeRootDir, assetKey),
|
||||
readFile: async (remotePath) => toBuffer(await input.client.readFile(remotePath)),
|
||||
tempDir,
|
||||
});
|
||||
});
|
||||
});
|
||||
}),
|
||||
}), restoreSink),
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { classifyWorkspaceRestoreFailure } from "./workspace-restore-merge.js";
|
||||
import { withWorkspaceRestoreDiagnostics } from "./workspace-restore-diagnostics.js";
|
||||
|
||||
describe("workspace restore diagnostics", () => {
|
||||
it("leaves successful restores silent", async () => {
|
||||
const sink = vi.fn();
|
||||
expect(await withWorkspaceRestoreDiagnostics("workspace", async () => 42, sink)).toBe(42);
|
||||
expect(sink).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("records bounded cause fields without copying messages or arbitrary codes", async () => {
|
||||
const sink = vi.fn();
|
||||
const error = Object.assign(new Error("private path and credential"), {
|
||||
code: "private-code", url: "https://private.invalid/secret", body: "private body",
|
||||
cause: Object.assign(new Error("private cause"), { code: "ECONNRESET", statusCode: 503, exitCode: 7 }),
|
||||
});
|
||||
await expect(withWorkspaceRestoreDiagnostics("asset", async () => { throw error; }, sink)).rejects.toBe(error);
|
||||
expect(sink).toHaveBeenCalledExactlyOnceWith(
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"asset","errorCode":"ECONNRESET","httpStatus":503,"exitCode":7}\n',
|
||||
);
|
||||
});
|
||||
|
||||
it.each([null, "private string", { code: "token", status: "401", exitCode: Infinity }, { status: 200, exitCode: -1 }])(
|
||||
"omits unrecognized diagnostic values (%j)", async (error) => {
|
||||
const sink = vi.fn();
|
||||
await expect(withWorkspaceRestoreDiagnostics("asset", async () => { throw error; }, sink)).rejects.toBe(error);
|
||||
expect(sink).toHaveBeenCalledExactlyOnceWith(
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"asset","errorCode":"unknown"}\n',
|
||||
);
|
||||
},
|
||||
);
|
||||
|
||||
it("bounds cause traversal even when it cycles", async () => {
|
||||
const error = Object.assign(new Error("private"), { cause: null as unknown, code: "ENOENT" });
|
||||
error.cause = error;
|
||||
const sink = vi.fn();
|
||||
await expect(withWorkspaceRestoreDiagnostics("asset", async () => { throw error; }, sink)).rejects.toBe(error);
|
||||
expect(sink.mock.calls[0]?.[0]).toContain('"errorCode":"ENOENT"');
|
||||
});
|
||||
|
||||
it.each(["EACCES", "WORKSPACE_RESTORE_UNSAFE_ARCHIVE"])("preserves %s when logging fails", async (code) => {
|
||||
const error = Object.assign(new Error("private"), { code });
|
||||
const classification = classifyWorkspaceRestoreFailure(error);
|
||||
await expect(withWorkspaceRestoreDiagnostics("workspace", async () => { throw error; }, async () => { throw new Error("sink failed"); }))
|
||||
.rejects.toBe(error);
|
||||
expect(classifyWorkspaceRestoreFailure(error)).toBe(classification);
|
||||
});
|
||||
|
||||
it("keeps the restore error even if a provider diagnostic getter throws", async () => {
|
||||
const error = Object.defineProperty(Object.assign(new Error("original"), {
|
||||
statusCode: 503, cause: { code: "ECONNRESET" },
|
||||
}), "code", { get() { throw new Error("getter failed"); } });
|
||||
const sink = vi.fn();
|
||||
await expect(withWorkspaceRestoreDiagnostics("asset", async () => { throw error; }, sink)).rejects.toBe(error);
|
||||
expect(sink).toHaveBeenCalledExactlyOnceWith(
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"asset","errorCode":"ECONNRESET","httpStatus":503}\n',
|
||||
);
|
||||
});
|
||||
|
||||
it("uses valid fallback statuses when preferred fields are invalid", async () => {
|
||||
const error = { status: "failed", statusCode: 503, exitCode: "failed", code: 7 };
|
||||
const sink = vi.fn();
|
||||
await expect(withWorkspaceRestoreDiagnostics("workspace", async () => { throw error; }, sink)).rejects.toBe(error);
|
||||
expect(sink).toHaveBeenCalledExactlyOnceWith(
|
||||
'[paperclip] Workspace restore diagnostic: {"phase":"workspace","errorCode":"unknown","httpStatus":503,"exitCode":7}\n',
|
||||
);
|
||||
});
|
||||
|
||||
it("logs nested failures once while preserving independent concurrent and later diagnostics", async () => {
|
||||
const error = Object.assign(new Error("nested failure"), { code: "ENOENT" });
|
||||
const sink = vi.fn();
|
||||
const fail = async () => { await Promise.resolve(); throw error; };
|
||||
const results = await Promise.allSettled([
|
||||
withWorkspaceRestoreDiagnostics("workspace", () => withWorkspaceRestoreDiagnostics("workspace", fail, sink), sink),
|
||||
withWorkspaceRestoreDiagnostics("asset", fail, sink),
|
||||
]);
|
||||
expect(results).toEqual([{ status: "rejected", reason: error }, { status: "rejected", reason: error }]);
|
||||
expect(sink).toHaveBeenCalledTimes(2);
|
||||
expect(sink.mock.calls.map(([line]) => JSON.parse(line.split(": ")[1]).phase).sort()).toEqual(["asset", "workspace"]);
|
||||
await expect(withWorkspaceRestoreDiagnostics("workspace", fail, sink)).rejects.toBe(error);
|
||||
expect(sink).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,68 @@
|
||||
import { AsyncLocalStorage } from "node:async_hooks";
|
||||
import type { RuntimeProgressSink } from "./runtime-progress.js";
|
||||
|
||||
type RestorePhase = "workspace" | "asset";
|
||||
const ERROR_CODES = new Set([
|
||||
"ENOENT", "EACCES", "EPERM", "ENOSPC", "EIO", "EXDEV", "ENOTDIR", "EISDIR",
|
||||
"ECONNRESET", "ECONNREFUSED", "ETIMEDOUT", "EPIPE", "ENOTFOUND", "EAI_AGAIN",
|
||||
"ABORT_ERR", "UND_ERR_CONNECT_TIMEOUT", "UND_ERR_SOCKET",
|
||||
]);
|
||||
const activeDiagnostic = new AsyncLocalStorage<boolean>();
|
||||
|
||||
function readField(value: Record<string, unknown>, key: string): unknown {
|
||||
try { return value[key]; } catch { return undefined; }
|
||||
}
|
||||
|
||||
function boundedInteger(value: unknown, minimum: number, maximum: number): number | undefined {
|
||||
return typeof value === "number" && Number.isInteger(value) && value >= minimum && value <= maximum
|
||||
? value : undefined;
|
||||
}
|
||||
|
||||
/** Only fixed codes and bounded numbers may enter the company-readable run log. */
|
||||
function diagnostic(error: unknown): { errorCode: string; httpStatus?: number; exitCode?: number } {
|
||||
const result: { errorCode: string; httpStatus?: number; exitCode?: number } = { errorCode: "unknown" };
|
||||
let current = error;
|
||||
// SDKs wrap transport errors in a cause. Bound traversal, including cycles.
|
||||
for (let depth = 0; depth < 4 && current && typeof current === "object"; depth++) {
|
||||
const value = current as Record<string, unknown>;
|
||||
const code = readField(value, "code");
|
||||
if (result.errorCode === "unknown" && typeof code === "string" && ERROR_CODES.has(code)) {
|
||||
result.errorCode = code;
|
||||
}
|
||||
const status = boundedInteger(readField(value, "status"), 400, 599)
|
||||
?? boundedInteger(readField(value, "statusCode"), 400, 599);
|
||||
if (result.httpStatus === undefined && status !== undefined) {
|
||||
result.httpStatus = status;
|
||||
}
|
||||
const exitCode = boundedInteger(readField(value, "exitCode"), 1, 255) ?? boundedInteger(code, 1, 255);
|
||||
if (result.exitCode === undefined && exitCode !== undefined) {
|
||||
result.exitCode = exitCode;
|
||||
}
|
||||
current = readField(value, "cause");
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
/** Add evidence without changing the thrown error, restore policy, or task ordering. */
|
||||
export async function withWorkspaceRestoreDiagnostics<T>(
|
||||
phase: RestorePhase,
|
||||
operation: () => Promise<T>,
|
||||
onProgress?: RuntimeProgressSink,
|
||||
): Promise<T> {
|
||||
// A nested repository restore propagates to its enclosing workspace task.
|
||||
// That task owns the diagnostic. Independent parallel tasks retain their own
|
||||
// async scopes, so two failed tasks still produce two diagnostic lines.
|
||||
if (activeDiagnostic.getStore()) return await operation();
|
||||
return await activeDiagnostic.run(true, async () => {
|
||||
try {
|
||||
return await operation();
|
||||
} catch (error) {
|
||||
try {
|
||||
await onProgress?.(`[paperclip] Workspace restore diagnostic: ${JSON.stringify({ phase, ...diagnostic(error) })}\n`);
|
||||
} catch {
|
||||
// A broken log sink must not replace a restore failure or relax its safety classification.
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
});
|
||||
}
|
||||
@@ -1044,14 +1044,16 @@ describe("claude_local ACP lane", () => {
|
||||
}),
|
||||
);
|
||||
|
||||
// Fail-open: the restore miss never changes the run's exit code or
|
||||
// status, and it surfaces as one allowlisted code — never the raw error.
|
||||
// Preserve the execution's exit code while reporting the restore failure.
|
||||
// Only a fixed diagnostic may contain the errno, never the raw error.
|
||||
expect(result.exitCode).toBe(0);
|
||||
expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_permission_denied");
|
||||
const allLogs = loggedLines.join("");
|
||||
expect(allLogs).not.toContain("SENTINEL-HOST-PATH-marker");
|
||||
expect(allLogs).not.toContain(localCwd);
|
||||
expect(allLogs).not.toContain("EACCES");
|
||||
const diagnostic = '[paperclip] Workspace restore diagnostic: {"phase":"workspace","errorCode":"EACCES"}\n';
|
||||
expect(loggedLines.filter((line) => line.includes("Workspace restore diagnostic:"))).toEqual([diagnostic]);
|
||||
expect(loggedLines.filter((line) => line !== diagnostic).join("")).not.toContain("EACCES");
|
||||
expect(allLogs).toContain("permission denied");
|
||||
} finally {
|
||||
await fs.chmod(localCwd, 0o700).catch(() => undefined);
|
||||
|
||||
Reference in new issue
Block a user