fix(hermes): surface silent nonzero exit failures (#10107)
## Thinking Path - Followed a silent nonzero Hermes exit from child-process result parsing through heartbeat run, runtime, task-session, and agent finalization. - Found two gaps: the adapter could return `errorMessage: null` for a numeric nonzero exit, and heartbeat later reused the nullable adapter field instead of its normalized fallback. - Kept timeout, signal-cancellation, and specific parsed diagnostics authoritative. ## Linked Issue(s) / Bug Report Related to #9751 (stderr classification) and #9519 (exit-zero finalization), but this is a separate failure mode. Reproduction: run Hermes with a child result equivalent to `exitCode: 1`, `timedOut: false`, and no parsed diagnostic. The heartbeat row derives `Adapter failed`, while runtime/task-session/agent finalization can persist null diagnostics. ## What Changed - Give silent numeric nonzero Hermes exits a stable fallback such as `Hermes exited with code 1`. - Preserve specific parsed errors and timeout/signal semantics. - Reuse the normalized persisted run error for recovered runtime state, task-session `lastError`, and agent `errorReason`. - Add adapter-level and embedded-Postgres regressions. ## Verification - Hermes adapter `execute.onspawn.test.ts` — 7 passed. - Focused heartbeat normalized-error regression — 1 passed (91 skipped). - `pnpm --filter @paperclipai/hermes-paperclip-adapter typecheck` — passed. - `pnpm --filter @paperclipai/server typecheck` — passed. - `git diff --check origin/master...HEAD` — passed. Independent review also ran the full recovery file: the changed regression passed; one unrelated pre-existing timing-sensitive test timed out. ## Risks / Rollout Notes Low risk. Fallback text is used only when a numeric nonzero exit has no better diagnostic. Existing timeout, signal, and parsed-error precedence remains unchanged. ## Model Used OpenAI Codex `gpt-5.6-sol` with repository inspection, test execution, and independent read-only review. ## 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 (not applicable: internal diagnostics only) - [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 - [ ] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: cucurigoo <cucurigoo@users.noreply.github.com>
This commit is contained in:
parent
8a5c0615f9
commit
2f1c0e011e
|
|
@ -119,6 +119,74 @@ describe("hermes-local adapter onSpawn forwarding", () => {
|
|||
expect(opts.onSpawn).toBeDefined();
|
||||
});
|
||||
|
||||
it("preserves a specific stderr diagnostic for a nonzero exit", async () => {
|
||||
vi.mocked(serverUtils.runChildProcess).mockResolvedValueOnce({
|
||||
exitCode: 1,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
stdout: "",
|
||||
stderr: "Error: provider unavailable\n",
|
||||
pid: null,
|
||||
startedAt: null,
|
||||
});
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const result = await execute(ctx as any);
|
||||
|
||||
expect(result.errorMessage).toBe("Error: provider unavailable");
|
||||
});
|
||||
|
||||
it("reports the exit code when a nonzero exit has no diagnostic", async () => {
|
||||
vi.mocked(serverUtils.runChildProcess).mockResolvedValueOnce({
|
||||
exitCode: 130,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
pid: null,
|
||||
startedAt: null,
|
||||
});
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const result = await execute(ctx as any);
|
||||
|
||||
expect(result.errorMessage).toBe("Hermes exited with code 130");
|
||||
});
|
||||
|
||||
it("leaves timeout diagnostics to the heartbeat timeout path", async () => {
|
||||
vi.mocked(serverUtils.runChildProcess).mockResolvedValueOnce({
|
||||
exitCode: 143,
|
||||
signal: "SIGTERM",
|
||||
timedOut: true,
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
pid: null,
|
||||
startedAt: null,
|
||||
});
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const result = await execute(ctx as any);
|
||||
|
||||
expect(result.errorMessage).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not label signal cancellation as a silent nonzero exit", async () => {
|
||||
vi.mocked(serverUtils.runChildProcess).mockResolvedValueOnce({
|
||||
exitCode: null,
|
||||
signal: "SIGTERM",
|
||||
timedOut: false,
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
pid: null,
|
||||
startedAt: null,
|
||||
});
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const result = await execute(ctx as any);
|
||||
|
||||
expect(result.errorMessage).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not inherit PAPERCLIP_API_KEY without a harness token", async () => {
|
||||
const previousApiKey = process.env.PAPERCLIP_API_KEY;
|
||||
process.env.PAPERCLIP_API_KEY = "parent-process-key";
|
||||
|
|
|
|||
|
|
@ -563,6 +563,8 @@ export async function execute(
|
|||
|
||||
if (parsed.errorMessage) {
|
||||
executionResult.errorMessage = parsed.errorMessage;
|
||||
} else if (!result.timedOut && typeof result.exitCode === "number" && result.exitCode !== 0) {
|
||||
executionResult.errorMessage = `Hermes exited with code ${result.exitCode}`;
|
||||
}
|
||||
|
||||
if (parsed.usage) {
|
||||
|
|
|
|||
|
|
@ -1202,6 +1202,40 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
|||
return { companyId, agentId, runId, wakeupRequestId, issueId };
|
||||
}
|
||||
|
||||
it("persists the normalized failure when an adapter omits its diagnostic", async () => {
|
||||
mockAdapterExecute.mockResolvedValueOnce({
|
||||
exitCode: 1,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
errorMessage: null,
|
||||
provider: "test",
|
||||
model: "test-model",
|
||||
});
|
||||
|
||||
const { agentId, runId } = await seedQueuedIssueRunFixture();
|
||||
const heartbeat = heartbeatService(db);
|
||||
|
||||
await heartbeat.resumeQueuedRuns();
|
||||
await waitForRunToSettle(heartbeat, runId);
|
||||
await heartbeat.waitForRunExecutionDrain(runId);
|
||||
|
||||
const run = await heartbeat.getRun(runId);
|
||||
const runtime = await db
|
||||
.select({ lastError: agentRuntimeState.lastError })
|
||||
.from(agentRuntimeState)
|
||||
.where(eq(agentRuntimeState.agentId, agentId))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
const agent = await db
|
||||
.select({ status: agents.status, errorReason: agents.errorReason })
|
||||
.from(agents)
|
||||
.where(eq(agents.id, agentId))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
|
||||
expect(run).toMatchObject({ status: "failed", error: "Adapter failed" });
|
||||
expect(runtime?.lastError).toBe("Adapter failed");
|
||||
expect(agent).toEqual({ status: "error", errorReason: "Adapter failed" });
|
||||
});
|
||||
|
||||
it("keeps a local run active when the recorded pid is still alive", async () => {
|
||||
const child = spawnAliveProcess();
|
||||
childProcesses.add(child);
|
||||
|
|
|
|||
|
|
@ -13395,7 +13395,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
|||
sessionId: session.legacySessionId,
|
||||
lastRunId: run.id,
|
||||
lastRunStatus: run.status,
|
||||
lastError: result.errorMessage ?? null,
|
||||
lastError: run.error ?? null,
|
||||
totalInputTokens: sql`${agentRuntimeState.totalInputTokens} + ${inputTokens}`,
|
||||
totalOutputTokens: sql`${agentRuntimeState.totalOutputTokens} + ${outputTokens}`,
|
||||
totalCachedInputTokens: sql`${agentRuntimeState.totalCachedInputTokens} + ${cachedInputTokens}`,
|
||||
|
|
@ -16037,7 +16037,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
|||
),
|
||||
sessionDisplayId: nextSessionState.displayId,
|
||||
lastRunId: finalizedRun.id,
|
||||
lastError: outcome === "succeeded" ? null : (adapterResult.errorMessage ?? "run_failed"),
|
||||
lastError: runErrorMessage,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
@ -16045,7 +16045,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
|||
await finalizeAgentStatus(
|
||||
agent.id,
|
||||
outcome,
|
||||
outcome === "succeeded" ? null : (adapterResult.errorMessage ?? null),
|
||||
runErrorMessage,
|
||||
{
|
||||
keepIdleOnFailure:
|
||||
outcome === "failed" &&
|
||||
|
|
|
|||
Loading…
Reference in New Issue