From eca5a1ec77319a21205ef6d20537964f1350d208 Mon Sep 17 00:00:00 2001 From: Jon Wiggins Date: Sun, 2 Aug 2026 17:30:00 -0600 Subject: [PATCH] fix(cost): accumulate cost on resume and use final result-event cost --- apps/api/e2e/repo-task.e2e.test.ts | 39 +++++++++++ apps/api/src/workers/task-worker.ts | 45 +++++++++++-- .../agent-adapters/src/claude-code.test.ts | 26 ++++++++ packages/agent-adapters/src/claude-code.ts | 21 +++++- packages/shared/src/index.ts | 1 + packages/shared/src/utils/cost.test.ts | 65 +++++++++++++++++++ packages/shared/src/utils/cost.ts | 59 +++++++++++++++++ 7 files changed, 250 insertions(+), 6 deletions(-) create mode 100644 packages/shared/src/utils/cost.test.ts create mode 100644 packages/shared/src/utils/cost.ts diff --git a/apps/api/e2e/repo-task.e2e.test.ts b/apps/api/e2e/repo-task.e2e.test.ts index ee31ffc4..b7f38414 100644 --- a/apps/api/e2e/repo-task.e2e.test.ts +++ b/apps/api/e2e/repo-task.e2e.test.ts @@ -260,4 +260,43 @@ describe("repo-task e2e", () => { const { body: logsBody } = await api<{ logs: LogRow[] }>(`/api/tasks/${taskId}/logs`); expect(logsBody.logs).toHaveLength(0); }); + + it("accumulates cost and tokens across a resume instead of overwriting (issue #541)", async () => { + // First run reports $0.05 and no PR → needs_attention, recording cost 0.05. + const taskId = await createTask("Resume accumulates cost [[mock:cost:0.05]]"); + const first = await waitForTaskState(taskId, [ + "needs_attention", + "completed", + "failed", + "pr_opened", + ]); + expect(first.state).toBe("needs_attention"); + expect(first.costUsd).toBe("0.05"); + expect(first.inputTokens).toBe(100); + expect(first.outputTokens).toBe(25); + + // Resume with a fresh $0.03 invocation. The resumed claude process reports + // only its OWN spend (0.03), so the persisted task total must ACCUMULATE to + // 0.05 + 0.03 = 0.08 — not be overwritten to 0.03 (the pre-fix undercount). + // The [[mock:cost:0.03]] directive leads the resume prompt, so it is the + // first match the fake sees even though the original 0.05 prompt is appended + // for context. + const { status } = await api(`/api/tasks/${taskId}/resume`, { + method: "POST", + body: JSON.stringify({ prompt: "Please continue [[mock:cost:0.03]]" }), + }); + expect(status).toBe(200); + + const resumed = await waitFor( + async () => { + const t = await getTask(taskId); + return t.costUsd === "0.08" ? t : null; + }, + { timeoutMs: 90_000, label: `task ${taskId} cost accumulates to 0.08` }, + ); + expect(resumed.costUsd).toBe("0.08"); + // Tokens accumulate the same way: 100+100 input, 25+25 output. + expect(resumed.inputTokens).toBe(200); + expect(resumed.outputTokens).toBe(50); + }); }); diff --git a/apps/api/src/workers/task-worker.ts b/apps/api/src/workers/task-worker.ts index fee75a35..570fc1be 100644 --- a/apps/api/src/workers/task-worker.ts +++ b/apps/api/src/workers/task-worker.ts @@ -13,6 +13,8 @@ import { parseRepoUrl, parsePrUrl, parseIntEnv, + addCostStrings, + addTokenCounts, } from "@optio/shared"; import { getAdapter } from "@optio/agent-adapters"; import { parseClaudeEvent } from "../services/agent-event-parser.js"; @@ -1086,11 +1088,46 @@ export function startTaskWorker() { await taskService.updateTaskResult(taskId, result.summary, result.error); - // Persist cost, token usage, and model data + // Persist cost, token usage, and model data. + // + // On a resume or force-restart, Claude runs as a FRESH process (either + // `claude --resume ` or a brand-new session on the existing + // branch). Its result reports only its OWN turns' total_cost_usd / token + // usage — it has no knowledge of what the prior run already spent. So the + // recorded value must ACCUMULATE (prior + this run), not overwrite. + // Overwriting is what caused issue #541: /api/analytics/costs sums + // tasks.cost_usd, so replacing the original cost with just the resumed + // invocation's spend undercounts total spend. + // + // A first run (no resume/restart signal) has no prior spend to preserve, + // so it writes its value directly — this also keeps "redo from scratch" + // semantics for a fresh run. Accumulating never double-counts here: each + // relaunch is a distinct process reporting only its own cost, so + // prior + current is always the true total. + // + // Continuation signals: `resumeSessionId` (/resume, --resume), a + // `restartFromBranch` fresh session on the existing PR (/force-restart, + // auto-resume), or a `resumePrompt` (set by every relaunch path — + // including message-resume where the stored session id may be absent). + // Any of the three means a prior run's cost is already recorded and must + // be preserved; only a genuine first run has none of them. + const isContinuation = !!(resumeSessionId || restartFromBranch || resumePrompt); const costFields: Record = {}; - if (result.costUsd != null) costFields.costUsd = String(result.costUsd); - if (result.inputTokens != null) costFields.inputTokens = result.inputTokens; - if (result.outputTokens != null) costFields.outputTokens = result.outputTokens; + if (result.costUsd != null) { + costFields.costUsd = isContinuation + ? addCostStrings(taskAfterExec.costUsd, result.costUsd) + : String(result.costUsd); + } + if (result.inputTokens != null) { + costFields.inputTokens = isContinuation + ? addTokenCounts(taskAfterExec.inputTokens, result.inputTokens) + : result.inputTokens; + } + if (result.outputTokens != null) { + costFields.outputTokens = isContinuation + ? addTokenCounts(taskAfterExec.outputTokens, result.outputTokens) + : result.outputTokens; + } if (result.model) costFields.modelUsed = result.model; if (Object.keys(costFields).length > 0) { await db.update(tasks).set(costFields).where(eq(tasks.id, taskId)); diff --git a/packages/agent-adapters/src/claude-code.test.ts b/packages/agent-adapters/src/claude-code.test.ts index 6c2dfd78..e43b9595 100644 --- a/packages/agent-adapters/src/claude-code.test.ts +++ b/packages/agent-adapters/src/claude-code.test.ts @@ -281,6 +281,32 @@ describe("ClaudeCodeAdapter", () => { expect(result.costUsd).toBe(0.0534); }); + it("uses the LAST result event's cost on a multi-turn run, not the first (issue #541)", () => { + // With --input-format stream-json each mid-task user message produces its + // own result event whose total_cost_usd is cumulative for the process. + // Taking the first match would drop everything after turn one. + const logs = [ + '{"type":"assistant","message":{"usage":{"input_tokens":100,"output_tokens":50}}}', + '{"type":"result","subtype":"success","is_error":false,"total_cost_usd":0.0534,"result":"turn 1 done"}', + '{"type":"assistant","message":{"usage":{"input_tokens":200,"output_tokens":75}}}', + '{"type":"result","subtype":"success","is_error":false,"total_cost_usd":0.1289,"result":"turn 2 done"}', + ].join("\n"); + const result = adapter.parseResult(0, logs); + expect(result.costUsd).toBe(0.1289); + }); + + it("falls back to the LAST regex cost match when result events are not JSON", () => { + // Truncated/garbled result lines still expose total_cost_usd in the raw + // text; the fallback must also prefer the last occurrence. + const logs = [ + 'noise "total_cost_usd": 0.01 more noise', + " total_cost_usd was 0.02 ...", + 'trailing "total_cost_usd": 0.0789 end', + ].join("\n"); + const result = adapter.parseResult(0, logs); + expect(result.costUsd).toBe(0.0789); + }); + it("extracts model from system init event", () => { const logs = [ '{"type":"system","subtype":"init","model":"claude-sonnet-4-20250514"}', diff --git a/packages/agent-adapters/src/claude-code.ts b/packages/agent-adapters/src/claude-code.ts index 9af16e49..e648ffcc 100644 --- a/packages/agent-adapters/src/claude-code.ts +++ b/packages/agent-adapters/src/claude-code.ts @@ -113,7 +113,17 @@ export class ClaudeCodeAdapter implements AgentAdapter { const prMatch = logs.match( /https:\/\/(?![\w.-]+\/api\/)[^\s"]+\/(?:pull\/\d+|-\/merge_requests\/\d+)/, ); - const costMatch = logs.match(/"total_cost_usd":\s*([\d.]+)/); + // Cost: take the LAST total_cost_usd, not the first. In stream-json mode a + // multi-turn run (mid-task user messages) emits a `result` event PER TURN, + // and each result's total_cost_usd is the CUMULATIVE cost of the process so + // far — so the final result event carries the full spend. Grabbing the first + // match would drop every turn after the first (issue #541). The authoritative + // value comes from the last result event (captured in the loop below); this + // global-regex last-match is only a fallback for logs whose result event + // didn't parse as JSON. + const costMatches = [...logs.matchAll(/"total_cost_usd":\s*([\d.]+)/g)]; + const costFromRegex = + costMatches.length > 0 ? parseFloat(costMatches[costMatches.length - 1][1]) : undefined; // Extract error, token usage, model, and result text from Claude's NDJSON events let totalInputTokens = 0; @@ -128,6 +138,8 @@ export class ClaudeCodeAdapter implements AgentAdapter { let lastResultIsError = false; let lastResultText: string | undefined; let lastResultSubtype: string | undefined; + // Cumulative cost reported by the most recent result event (last wins). + let lastResultCostUsd: number | undefined; // Synthetic assistant text Claude Code emits when an API call fails // ("API Error: ..."). Only used as a failure signal when the run never // produced a result event (i.e. the error was terminal, not recovered). @@ -175,6 +187,11 @@ export class ClaudeCodeAdapter implements AgentAdapter { lastResultSubtype = typeof event.subtype === "string" ? event.subtype : undefined; lastResultText = typeof event.result === "string" && event.result ? event.result : undefined; + // Each result event's total_cost_usd is cumulative for the process, + // so the last one seen holds the authoritative final cost. + if (typeof event.total_cost_usd === "number") { + lastResultCostUsd = event.total_cost_usd; + } } } catch { // Not JSON, skip @@ -230,7 +247,7 @@ export class ClaudeCodeAdapter implements AgentAdapter { return { success, prUrl: prMatch?.[0], - costUsd: costMatch ? parseFloat(costMatch[1]) : undefined, + costUsd: lastResultCostUsd ?? costFromRegex, inputTokens: totalInputTokens > 0 ? totalInputTokens : undefined, outputTokens: totalOutputTokens > 0 ? totalOutputTokens : undefined, model, diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 49a8fd53..08d48fbc 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -25,6 +25,7 @@ export * from "./types/git-platform.js"; export * from "./utils/parse-repo-url.js"; export * from "./utils/is-stalled.js"; export * from "./utils/parse-int-env.js"; +export * from "./utils/cost.js"; export * from "./optio-tools.js"; export * from "./types/pr-review.js"; export * from "./reconcile/types.js"; diff --git a/packages/shared/src/utils/cost.test.ts b/packages/shared/src/utils/cost.test.ts new file mode 100644 index 00000000..5f08651e --- /dev/null +++ b/packages/shared/src/utils/cost.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect } from "vitest"; +import { addCostStrings, addTokenCounts } from "./cost.js"; + +describe("addCostStrings", () => { + it("accumulates prior + current cost (the resume undercount fix)", () => { + // Original run cost $0.0534; resumed run reports only its own $0.0212. + // The task total must reflect both, not just the resumed invocation. + expect(addCostStrings("0.0534", "0.0212")).toBe("0.0746"); + }); + + it("treats a null prior cost as zero (first run)", () => { + expect(addCostStrings(null, "0.0123")).toBe("0.0123"); + expect(addCostStrings(undefined, "0.0123")).toBe("0.0123"); + }); + + it("treats an empty-string prior cost as zero", () => { + expect(addCostStrings("", "0.5")).toBe("0.5"); + }); + + it("returns the prior cost when the new cost is missing", () => { + expect(addCostStrings("0.42", null)).toBe("0.42"); + }); + + it("is decimal-safe: 0.1 + 0.2 does not drift to 0.30000000000000004", () => { + expect(addCostStrings("0.1", "0.2")).toBe("0.3"); + }); + + it("handles differing decimal precision without drift", () => { + expect(addCostStrings("0.001", "0.00012345")).toBe("0.00112345"); + }); + + it("accepts numbers as well as strings", () => { + expect(addCostStrings(0.0534, 0.0212)).toBe("0.0746"); + }); + + it("never emits scientific notation for tiny costs", () => { + const sum = addCostStrings("0.0000001", "0.0000002"); + expect(sum).not.toMatch(/e/i); + expect(Number(sum)).toBeCloseTo(0.0000003, 12); + }); + + it("treats non-numeric junk as zero rather than NaN", () => { + expect(addCostStrings("not-a-number", "0.25")).toBe("0.25"); + expect(addCostStrings("0.25", "junk")).toBe("0.25"); + }); + + it("accumulates repeatedly across multiple resumes", () => { + let total = addCostStrings(null, "0.05"); + total = addCostStrings(total, "0.03"); + total = addCostStrings(total, "0.02"); + expect(total).toBe("0.1"); + }); +}); + +describe("addTokenCounts", () => { + it("adds two token counts", () => { + expect(addTokenCounts(300, 125)).toBe(425); + }); + + it("treats nullish operands as zero", () => { + expect(addTokenCounts(null, 125)).toBe(125); + expect(addTokenCounts(300, undefined)).toBe(300); + expect(addTokenCounts(null, null)).toBe(0); + }); +}); diff --git a/packages/shared/src/utils/cost.ts b/packages/shared/src/utils/cost.ts new file mode 100644 index 00000000..2253a105 --- /dev/null +++ b/packages/shared/src/utils/cost.ts @@ -0,0 +1,59 @@ +/** + * Cost accounting helpers. + * + * `tasks.cost_usd` (and the equivalent columns on workflow_runs / pr_review_runs + * / persistent_agents) is stored as a **string** — see the schema comment + * "stored as string to avoid float precision issues". Analytics reads it back + * with `CAST(cost_usd AS NUMERIC)` and sums across rows, so the string must be a + * plain decimal literal (no scientific notation, no thousands separators). + */ + +/** Count the digits after the decimal point in a numeric string. */ +function decimalPlaces(s: string): number { + const m = /\.(\d+)/.exec(s); + return m ? m[1].length : 0; +} + +/** + * Decimal-safe addition of two cost values, returning a plain decimal string. + * + * Naive float addition drifts (`0.1 + 0.2 === 0.30000000000000004`), which would + * accumulate error every time a resumed run's cost is added to the prior total. + * To avoid that, both operands are scaled to integers at the finer of the two + * inputs' decimal precision (capped so the scale can't overflow), added exactly + * as integers, then rescaled. + * + * Nullish, empty, or non-numeric operands are treated as `0`, so this is safe to + * call with a task's current `costUsd` (which may be `null` on the first run). + */ +export function addCostStrings(a?: string | number | null, b?: string | number | null): string { + const sa = a == null ? "" : String(a); + const sb = b == null ? "" : String(b); + const na = Number(sa || 0); + const nb = Number(sb || 0); + const va = Number.isFinite(na) ? na : 0; + const vb = Number.isFinite(nb) ? nb : 0; + + // Cap precision at 12 decimals: more than enough for per-run USD costs and + // keeps 10 ** decimals well within safe-integer range. + const decimals = Math.min(12, Math.max(decimalPlaces(sa), decimalPlaces(sb))); + const scale = 10 ** decimals; + const sum = (Math.round(va * scale) + Math.round(vb * scale)) / scale; + + // Format with toFixed (never scientific notation, unlike String() for values + // below 1e-6) at the operands' precision, then trim trailing zeros so the + // result is a clean plain-decimal literal for CAST(... AS NUMERIC). + let out = sum.toFixed(decimals); + if (out.includes(".")) { + out = out.replace(/0+$/, "").replace(/\.$/, ""); + } + return out; +} + +/** + * Add two token counts, treating nullish operands as 0. Tokens are integers, so + * plain addition is exact — this just centralizes the null handling. + */ +export function addTokenCounts(a?: number | null, b?: number | null): number { + return (a ?? 0) + (b ?? 0); +}