mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix(native): explain failed accepted results on the run (#19091)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A native run ends when the server projects the accepted result onto the run row. > - Operators read `errorCode` and `error` on a failed run to learn why it failed. > - Three native writers can set a run to `failed` without these fields. #19063 repairs the finalizer retry-exhaustion writer. > - The two result-projection writers still set `failed` with no cause when the provider failure is not classified. > - This pull request makes both projection writers record a cause when they project `failed`. > - The benefit is that, with #19063, no native writer can leave a failed run unexplained. ## Linked Issues or Issue Description Refs #19063. **What happened?** A staging soak found 11 failed native runs with `errorCode` null and `error` null. In each run the provider turn succeeded. Workspace copy-back then failed three times, and the finalizer retry-exhaustion path set the run to `failed`. #19063 fixes that writer. An audit of every `heartbeat_runs` status writer found two more writers with the same defect. `finalizeNativeRun` and `projectCommittedRun` project an accepted result with `runTerminalState: "failed"`. They write the error fields only when a classified provider failure exists. **Expected behavior** A run with status `failed` always has an `errorCode` and an `error`. A cause that is already recorded stays. **Steps to reproduce** 1. Accept a native result with `runTerminalState: "failed"` and no `turn.failed` event. 2. Call `finalizeNativeRun` with `projectRunStatus: true`, or project the committed run through `projectCommittedRun`. 3. Read the run. On master its status is `failed`, and its `errorCode` and `error` are null. ## What Changed - Add `explainFailedRun` in `server/src/services/native-runtime/native-run-failure-explanation.ts`. It builds SQL SET expressions. They keep a non-empty existing `errorCode` or `error`. Otherwise they write `native_run_failed` and a readable message. By default the expressions leave a run that is already `cancelled` unchanged, as the status writers do. - `finalizeNativeRun` uses the helper when it projects `failed`. The existing provider-failure and recovered-failure values remain the preferred causes. - `projectCommittedRun` uses the helper for a `failed` accepted result. This writer sets status unconditionally, so it does not use the cancelled branch. - Writer audit: all legacy and heartbeat callers of `setRunStatus*` that pass `failed` already set both fields. The other direct native writers also set both fields. ## Verification - Red, with the source change stashed: 3 new cases failed with `errorCode: null`. These were the finalizer projection, recovery projection with no cause, and recovery projection with an empty error. The control case with a recorded cause passed. - Green: `npx vitest run src/__tests__/native-run-projection-work.test.ts src/services/native-runtime/native-run-finalizer-telemetry.test.ts src/__tests__/native-status-arbiter-corpus.test.ts` (225 passed). - Wider run: all `server/src/__tests__/native-*.test.ts` and `server/src/services/native-runtime/*.test.ts` (7416 passed). Two failures come from the local environment. `plugin-runner-probe` needs the Rust runner source. `native-ingress-bootstrap-retry` needs the controller-host OpenCode proxy. - This change was not exercised on staging. It changes only fields that are written at terminal projection. ## Risks - Low risk. The change affects only rows that are projected to `failed`. A recorded cause is never replaced. Succeeded and cancelled projections are unchanged. - The staging failures need #19063 too. This PR alone does not change their writer. - The cause behind the staging copy-back failures is still unknown. The sandbox command reports the redacted `remote_workspace_operation_failed`, which also appears in a separate signature. This PR does not change that diagnostic. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, through Claude Code with tool use (shell, file edits, browser reads of staging run records). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
4 files changed
+79
-8
No files matched your search
@@ -15,6 +15,7 @@ import { agentWakeupRequests, agents, companies, completionContracts, createDb,
|
||||
import { createDeliveryWorkCoordinator } from "../services/delivery-work-coordinator.js";
|
||||
import { nativeRunProjectionWork } from "../services/native-runtime/native-run-projection-work.js";
|
||||
import { projectCommittedRun } from "../services/native-runtime/native-run-projection.js";
|
||||
import { NATIVE_RUN_FAILED_UNEXPLAINED } from "../services/native-runtime/native-run-failure-explanation.js";
|
||||
import { notifyNativeFinalizationWork } from "../services/native-finalization-notifications.js";
|
||||
import { finalizeNativeRun, recordNativeFinalizationFailure } from "../services/native-runtime/native-run-finalizer.js";
|
||||
import * as statusCommitter from "../services/native-runtime/status-decision-committer.js";
|
||||
@@ -430,6 +431,25 @@ describe("committed native run projection lifecycle", () => {
|
||||
expect((await read(binding.runId)).executionStatusDeliveryId).toBe(settled.executionStatusDeliveryId);
|
||||
expect(emitAgentTaskRun).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
it("explains a failed accepted result when the finalizer projects the run", async () => {
|
||||
const binding = await accepted("failed");
|
||||
await finalizeNativeRun({ db, runId: binding.runId, workspaceFinalizeStatus: "succeeded", projectRunStatus: true });
|
||||
const run = await read(binding.runId);
|
||||
expect(run).toMatchObject({ status: "failed", errorCode: NATIVE_RUN_FAILED_UNEXPLAINED.errorCode, error: NATIVE_RUN_FAILED_UNEXPLAINED.error });
|
||||
});
|
||||
it.each([
|
||||
["no recorded cause", null, null, NATIVE_RUN_FAILED_UNEXPLAINED.errorCode, NATIVE_RUN_FAILED_UNEXPLAINED.error],
|
||||
["an empty error", "adapter_failed", " ", "adapter_failed", NATIVE_RUN_FAILED_UNEXPLAINED.error],
|
||||
["a recorded cause", "adapter_failed", "Adapter exited 1", "adapter_failed", "Adapter exited 1"],
|
||||
])("explains a failed accepted result in committed recovery projection with %s", async (_label, errorCode, error, expectedCode, expectedError) => {
|
||||
const binding = await accepted("failed");
|
||||
await finalizeNativeRun({ db, runId: binding.runId, workspaceFinalizeStatus: "succeeded" });
|
||||
await db.update(heartbeatRuns).set({ errorCode, error }).where(eq(heartbeatRuns.id, binding.runId));
|
||||
const run = await read(binding.runId);
|
||||
expect(run.status).toBe("running");
|
||||
await projectCommittedRun({ db, run, coordinator: await state(binding.runId) });
|
||||
expect(await read(binding.runId)).toMatchObject({ status: "failed", errorCode: expectedCode, error: expectedError });
|
||||
});
|
||||
it("ignores routine company-only finalization hints and still repairs a committed run", async () => {
|
||||
await start(); await flush(); await quiet();
|
||||
const previous = db.$client.options.debug;
|
||||
|
||||
@@ -0,0 +1,36 @@
|
||||
import { sql, type SQL } from "drizzle-orm";
|
||||
import { heartbeatRuns } from "@paperclipai/db";
|
||||
|
||||
/** Used when a native run reaches `failed` and no writer recorded a more specific cause. */
|
||||
export const NATIVE_RUN_FAILED_UNEXPLAINED = {
|
||||
errorCode: "native_run_failed",
|
||||
error: "The native run ended in a failed state without a classified provider error. Inspect the run events for the last recorded step.",
|
||||
} as const;
|
||||
|
||||
export type NativeRunFailureExplanation = { errorCode: string; error: string };
|
||||
|
||||
/**
|
||||
* A failed run must always say why. These SET expressions keep an explanation
|
||||
* already on the row, and otherwise record `fallback`. A run that is already
|
||||
* cancelled keeps its own fields, because the status writers preserve a
|
||||
* concurrent cancellation. In an UPDATE, column references read the row before
|
||||
* the update, which matches the status `case` expressions of the callers.
|
||||
*
|
||||
* `current` overrides the expression used for the existing value, for writers
|
||||
* that already compute a projected value for that field. A writer that sets
|
||||
* `failed` unconditionally passes `preserveCancelled: false`.
|
||||
*/
|
||||
export function explainFailedRun(
|
||||
fallback: NativeRunFailureExplanation,
|
||||
current: { errorCode?: SQL; error?: SQL; preserveCancelled?: boolean } = {},
|
||||
) {
|
||||
const errorCode = current.errorCode ?? sql`${heartbeatRuns.errorCode}`;
|
||||
const error = current.error ?? sql`${heartbeatRuns.error}`;
|
||||
const explainedCode = sql`coalesce(nullif(btrim(${errorCode}), ''), ${fallback.errorCode})`;
|
||||
const explainedError = sql`coalesce(nullif(btrim(${error}), ''), ${fallback.error})`;
|
||||
if (current.preserveCancelled === false) return { errorCode: explainedCode, error: explainedError };
|
||||
return {
|
||||
errorCode: sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.errorCode} else ${explainedCode} end`,
|
||||
error: sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.error} else ${explainedError} end`,
|
||||
};
|
||||
}
|
||||
@@ -26,6 +26,7 @@ import { hasPendingNativeChildCompletion } from "./native-child-completion-deliv
|
||||
import { settleExternalConversation } from "../slack-conversation-lifecycle.js";
|
||||
import { executionFailureRetryCount } from "../execution-recovery-attempt.js";
|
||||
import { readPersistedNativeFailureEvidence } from "./native-provider-failure-evidence.js";
|
||||
import { explainFailedRun, NATIVE_RUN_FAILED_UNEXPLAINED } from "./native-run-failure-explanation.js";
|
||||
import { dismissAutomaticCompletionReviews } from "./automatic-completion-reviews.js";
|
||||
import { findSupersededExternalReviewRequests } from "./superseded-external-review-requests.js";
|
||||
import { getNativeReviewAssignment, readNativeReviewAssignmentContext } from "./native-review-participant.js";
|
||||
@@ -1629,12 +1630,20 @@ export async function finalizeNativeRun(input: {
|
||||
|| ${JSON.stringify(finalizationMetadata)}::jsonb
|
||||
|| ${clearExecutionFailure ? sql`case when ${heartbeatRuns.status} = 'cancelled' then '{}'::jsonb
|
||||
else ${recoveredExecutionFailureMetadata()} end` : sql`'{}'::jsonb`}`;
|
||||
const projectedError = sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.error}
|
||||
else ${providerFailure ? sql`coalesce(${heartbeatRuns.error}, ${providerFailure.errorMessage})`
|
||||
: clearExecutionFailure ? sql`null` : heartbeatRuns.error} end`;
|
||||
const projectedErrorCode = sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.errorCode}
|
||||
else ${providerFailure ? sql`coalesce(${heartbeatRuns.errorCode}, ${providerFailure.errorCode})`
|
||||
: clearExecutionFailure ? sql`null` : heartbeatRuns.errorCode} end`;
|
||||
const nextError = providerFailure ? sql`coalesce(${heartbeatRuns.error}, ${providerFailure.errorMessage})`
|
||||
: clearExecutionFailure ? sql`null` : sql`${heartbeatRuns.error}`;
|
||||
const nextErrorCode = providerFailure ? sql`coalesce(${heartbeatRuns.errorCode}, ${providerFailure.errorCode})`
|
||||
: clearExecutionFailure ? sql`null` : sql`${heartbeatRuns.errorCode}`;
|
||||
// A run projected to failed always records why, even when the provider
|
||||
// reported a failed terminal without a classified failure.
|
||||
const projectsFailedStatus = input.projectRunStatus && projectedStatus === "failed";
|
||||
const explained = projectsFailedStatus
|
||||
? explainFailedRun(NATIVE_RUN_FAILED_UNEXPLAINED, { error: nextError, errorCode: nextErrorCode })
|
||||
: null;
|
||||
const projectedError = explained?.error ?? sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.error}
|
||||
else ${nextError} end`;
|
||||
const projectedErrorCode = explained?.errorCode ?? sql`case when ${heartbeatRuns.status} = 'cancelled' then ${heartbeatRuns.errorCode}
|
||||
else ${nextErrorCode} end`;
|
||||
const projectionChanged = or(
|
||||
sql`${heartbeatRuns.nativePhase} is distinct from ${finalizationPhase}`,
|
||||
sql`${heartbeatRuns.resultJson} is distinct from ${projectedResult}`,
|
||||
@@ -1659,7 +1668,7 @@ export async function finalizeNativeRun(input: {
|
||||
: {}),
|
||||
nativePhase: finalizationPhase,
|
||||
nativePhaseUpdatedAt: now,
|
||||
...(clearExecutionFailure || providerFailure ? { error: projectedError, errorCode: projectedErrorCode } : {}),
|
||||
...(clearExecutionFailure || providerFailure || projectsFailedStatus ? { error: projectedError, errorCode: projectedErrorCode } : {}),
|
||||
resultJson: projectedResult,
|
||||
updatedAt: now,
|
||||
})
|
||||
|
||||
@@ -18,6 +18,7 @@ import { notifyChatRunMilestones } from "../chat-run-milestone-notifications.js"
|
||||
import { emitAgentTaskRun } from "../agent-task-run-telemetry.js";
|
||||
import { isNewTerminalRunFailure, reportRunFailure } from "../run-failure-report.js";
|
||||
import { nativeRunnerOwnershipNotHeldCondition } from "./native-runner-ownership.js";
|
||||
import { explainFailedRun, NATIVE_RUN_FAILED_UNEXPLAINED } from "./native-run-failure-explanation.js";
|
||||
|
||||
|
||||
export function projectNativeTerminalRunStatus(
|
||||
@@ -192,7 +193,12 @@ export async function projectCommittedRun(input: {
|
||||
|| ${recoveredExecutionFailureMetadata()}
|
||||
|| jsonb_build_object('finalizationPhase', 'committed', 'failureCode', null, 'originalFailureCode', null, 'nextAttemptAt', null)`,
|
||||
}
|
||||
: { resultJson: sql`coalesce(${heartbeatRuns.resultJson}, '{}'::jsonb) || jsonb_build_object('finalizationPhase', 'committed', 'failureCode', null, 'originalFailureCode', null, 'nextAttemptAt', null)` }),
|
||||
: {
|
||||
// A failed accepted result keeps any recorded cause, and otherwise
|
||||
// names the unexplained failure.
|
||||
...(terminalState === "failed" ? explainFailedRun(NATIVE_RUN_FAILED_UNEXPLAINED, { preserveCancelled: false }) : {}),
|
||||
resultJson: sql`coalesce(${heartbeatRuns.resultJson}, '{}'::jsonb) || jsonb_build_object('finalizationPhase', 'committed', 'failureCode', null, 'originalFailureCode', null, 'nextAttemptAt', null)`,
|
||||
}),
|
||||
updatedAt: now,
|
||||
})
|
||||
.where(needsRepair)
|
||||
|
||||
Reference in new issue
Block a user