From 2f1c0e011eb0e53152a4392d46a9ba41200585d2 Mon Sep 17 00:00:00 2001 From: Constantine Date: Thu, 13 Aug 2026 02:09:35 +0300 Subject: [PATCH] fix(hermes): surface silent nonzero exit failures (#10107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- .../hermes/src/server/execute.onspawn.test.ts | 68 +++++++++++++++++++ .../adapters/hermes/src/server/execute.ts | 2 + .../heartbeat-process-recovery.test.ts | 34 ++++++++++ server/src/services/heartbeat.ts | 6 +- 4 files changed, 107 insertions(+), 3 deletions(-) diff --git a/packages/adapters/hermes/src/server/execute.onspawn.test.ts b/packages/adapters/hermes/src/server/execute.onspawn.test.ts index 23aef939b5..a919ddd236 100644 --- a/packages/adapters/hermes/src/server/execute.onspawn.test.ts +++ b/packages/adapters/hermes/src/server/execute.onspawn.test.ts @@ -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"; diff --git a/packages/adapters/hermes/src/server/execute.ts b/packages/adapters/hermes/src/server/execute.ts index b476ae214e..3b4c5d4bc4 100644 --- a/packages/adapters/hermes/src/server/execute.ts +++ b/packages/adapters/hermes/src/server/execute.ts @@ -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) { diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 84ba03a1b5..4826ad0021 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -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); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index ff40d2f4d2..a1f53697e6 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -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" &&