-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(devin): read usage from ModelUsageStats and classify cloud failures #4419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,53 @@ import { getCachedCatalog } from "./devin/cloud-direct/catalog"; | |
| import { buildNonOpenAIToolCatalogNudgeForTools } from "./tool-catalog-nudge"; | ||
| import { DEVIN_DEFAULT_API_SERVER, resolveDevinApiServer } from "../oauth/devin"; | ||
|
|
||
| /** | ||
| * Combine two usage frames from one turn by keeping the larger count per field. | ||
| * | ||
| * Devin's counters are cumulative within a turn, so a frame that reports less | ||
| * than an earlier one is reporting a subset, not a correction. | ||
| */ | ||
| export function mergeDevinUsage(previous: OcxUsage, next: OcxUsage): OcxUsage { | ||
| const keys = [ | ||
| "inputTokens", "outputTokens", | ||
| "cachedInputTokens", "cacheReadInputTokens", "cacheCreationInputTokens", | ||
| "reasoningOutputTokens", | ||
| ] as const; | ||
| const merged: OcxUsage = { ...previous, ...next }; | ||
| for (const key of keys) { | ||
| const a = previous[key]; | ||
| const b = next[key]; | ||
| if (typeof a === "number" && typeof b === "number") merged[key] = Math.max(a, b); | ||
| else if (typeof a === "number" && b === undefined) merged[key] = a; | ||
| } | ||
| // totalTokens is derived, not merged. Taking the max of two totals alongside | ||
| // per-field maxima can leave total !== input + output, and the cost and log | ||
| // paths read the total. | ||
| const total = (merged.inputTokens ?? 0) + (merged.outputTokens ?? 0); | ||
| if (total > 0) merged.totalTokens = total; | ||
| return merged; | ||
| } | ||
|
|
||
| /** | ||
| * The wording `isClientClosedMessage` recognises. | ||
| * | ||
| * "Devin turn was aborted." matched nothing, so a cancelled turn fell through to | ||
| * the default inference and was logged as a 502 upstream failure rather than as | ||
| * the client hanging up. | ||
| */ | ||
| const DEVIN_CLIENT_CLOSED_MESSAGE = "client closed request"; | ||
|
|
||
| /** Map a cloud-direct failure onto the structured fields the error event carries. */ | ||
| export function devinErrorClassification(error: unknown): { status?: number; errorType?: string; retryable?: boolean } { | ||
| const status = error instanceof CloudChatError ? error.status : undefined; | ||
| if (status === undefined) return {}; | ||
| if (status === 401) return { status, errorType: "authentication_error", retryable: false }; | ||
| if (status === 403) return { status, errorType: "permission_error", retryable: false }; | ||
| if (status === 429) return { status, errorType: "rate_limit_error", retryable: true }; | ||
| if (status >= 500) return { status, retryable: true }; | ||
| return { status, retryable: false }; | ||
| } | ||
|
|
||
| export const DEVIN_API_SERVER = DEVIN_DEFAULT_API_SERVER; | ||
|
|
||
| const EFFORT_SUFFIXES = new Set(["low", "medium", "high", "xhigh", "max", "none", "1m", "max-1m", "none-1m", "fast"]); | ||
|
|
@@ -210,7 +257,7 @@ export function createDevinAdapter( | |
|
|
||
| async runTurn(parsed: OcxParsedRequest, incoming: IncomingMeta, emit: (event: AdapterEvent) => void) { | ||
| if (incoming.abortSignal?.aborted) { | ||
| emit({ type: "error", message: "Devin turn was aborted before start." }); | ||
| emit({ type: "error", message: DEVIN_CLIENT_CLOSED_MESSAGE, status: 499, retryable: false }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift Add
🤖 Prompt for AI Agents |
||
| return; | ||
| } | ||
| let apiKey: string; | ||
|
|
@@ -272,7 +319,7 @@ export function createDevinAdapter( | |
| // Say what happened instead, the way the other runTurn-only adapter | ||
| // does, and carry any usage already seen. | ||
| closeOpenTool(); | ||
| emit({ type: "error", message: "Devin turn was aborted.", ...(usage ? { usage } : {}) }); | ||
| emit({ type: "error", message: DEVIN_CLIENT_CLOSED_MESSAGE, status: 499, retryable: false, ...(usage ? { usage } : {}) }); | ||
| return; | ||
| } | ||
| if (event.kind === "text") { | ||
|
|
@@ -305,35 +352,46 @@ export function createDevinAdapter( | |
| } | ||
| if (event.kind === "usage") { | ||
| const total = event.totalTokens ?? ((event.promptTokens ?? 0) + (event.completionTokens ?? 0)); | ||
| usage = { | ||
| const next: OcxUsage = { | ||
| inputTokens: event.promptTokens ?? 0, | ||
| outputTokens: event.completionTokens ?? 0, | ||
| ...(total > 0 ? { totalTokens: total } : {}), | ||
| ...(event.cachedInputTokens !== undefined ? { cachedInputTokens: event.cachedInputTokens } : {}), | ||
| ...(event.cacheCreationInputTokens !== undefined ? { cacheCreationInputTokens: event.cacheCreationInputTokens } : {}), | ||
| ...(event.reasoningTokens !== undefined ? { reasoningOutputTokens: event.reasoningTokens } : {}), | ||
| }; | ||
| // Merge rather than replace. A turn can carry more than one usage | ||
| // frame, and the counters are cumulative, so a later partial frame | ||
| // that omits a field used to zero a count the earlier frame had | ||
| // already reported. | ||
| usage = usage ? mergeDevinUsage(usage, next) : next; | ||
| continue; | ||
| } | ||
| } | ||
| closeOpenTool(); | ||
| if (incoming.abortSignal?.aborted) { | ||
| emit({ type: "error", message: "Devin turn was aborted.", ...(usage ? { usage } : {}) }); | ||
| emit({ type: "error", message: DEVIN_CLIENT_CLOSED_MESSAGE, status: 499, retryable: false, ...(usage ? { usage } : {}) }); | ||
| } else { | ||
| emit({ type: "done", ...(usage ? { usage } : {}), ...(stopReason ? { stopReason } : {}) }); | ||
| } | ||
| } catch (error) { | ||
| closeOpenTool(); | ||
| if (incoming.abortSignal?.aborted) { | ||
| emit({ type: "error", message: "Devin turn was aborted.", ...(usage ? { usage } : {}) }); | ||
| emit({ type: "error", message: DEVIN_CLIENT_CLOSED_MESSAGE, status: 499, retryable: false, ...(usage ? { usage } : {}) }); | ||
| return; | ||
| } | ||
| const message = error instanceof CloudChatError | ||
| ? ("Devin cloud error" + (error.code ? " " + error.code : "") + ": " + error.message) | ||
| : error instanceof Error ? error.message : String(error); | ||
| // Usage that already arrived is still real; dropping it loses the | ||
| // accounting for a turn that did most of its work before failing. | ||
| emit({ type: "error", message, ...(usage ? { usage } : {}) }); | ||
| emit({ | ||
| type: "error", | ||
| message, | ||
| ...devinErrorClassification(error), | ||
| ...(error instanceof CloudChatError && error.code ? { code: error.code } : {}), | ||
| ...(usage ? { usage } : {}), | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -669,7 +669,30 @@ function buildGetChatMessageRequest(args: BuildArgs): Buffer { | |
| * any non-zero to 'tool_calls' for now (and let the caller fall back to | ||
| * 'stop' if no tool_call deltas were emitted). | ||
| */ | ||
| function* decodeChatFrame(proto: Buffer): Generator<CloudChatEvent> { | ||
| export function* decodeChatFrame(proto: Buffer): Generator<CloudChatEvent> { | ||
| // Field 7 is `ModelUsageStats`, the authoritative per-turn accounting, and | ||
| // field 28 is `response_dimension_groups` — the rows the IDE renders. The | ||
| // decoder below reads 28 because a capture happened to expose metric-looking | ||
| // strings there (`ResponseDimension.uid` is its field 5, which is what the | ||
| // entry walker treats as `metric_id`), and that works only when the service | ||
| // chose to render cache rows. Field 7 carries cache read and cache write | ||
| // unconditionally, which is why a cached Devin turn used to report a bare | ||
| // total with no cached subset. | ||
| // | ||
| // Both fields arrive in the same message and the adapter keeps the last usage | ||
| // event it sees, so this cannot be a plain "decode both": field 7 has to | ||
| // suppress field 28 within the message. It is yielded before the rest of the | ||
| // frame rather than after it, so a frame that also carries finish (field 5) | ||
| // still reports usage ahead of the turn's end, and the order does not depend | ||
| // on where the service happens to place the field. | ||
| let authoritativeUsage: CloudChatEvent | null = null; | ||
| for (const f of iterFields(proto)) { | ||
| if (f.num === 7 && f.wire === 2 && Buffer.isBuffer(f.value)) { | ||
| authoritativeUsage = decodeModelUsageStats(f.value as Buffer); | ||
| if (authoritativeUsage) break; | ||
| } | ||
| } | ||
| if (authoritativeUsage) yield authoritativeUsage; | ||
| for (const f of iterFields(proto)) { | ||
| if (f.num === 3 && f.wire === 2 && Buffer.isBuffer(f.value)) { | ||
| // Visible delta_text — what the user should SEE in the chat. | ||
|
|
@@ -740,6 +763,7 @@ function* decodeChatFrame(proto: Buffer): Generator<CloudChatEvent> { | |
| // else stays 'stop' for 0/2/4-9/12/13 | ||
| yield { kind: 'finish', reason }; | ||
| } else if (f.num === 28 && f.wire === 2 && Buffer.isBuffer(f.value)) { | ||
| if (authoritativeUsage) continue; | ||
| const usage = decodeUsageBlock(f.value as Buffer); | ||
| if (usage) yield usage; | ||
| } | ||
|
|
@@ -834,6 +858,68 @@ function decodeUsageBlock(buf: Buffer): CloudChatEvent | null { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * `exa.codeium_common_pb.ModelUsageStats` at GetChatMessageResponse field 7. | ||
| * | ||
| * ModelUsageStats { | ||
| * #2 input_tokens uint64 | ||
| * #3 output_tokens uint64 | ||
| * #4 cache_write_tokens uint64 | ||
| * #5 cache_read_tokens uint64 | ||
| * } | ||
| * | ||
| * Plain varints, so the field-28 entry walker — which descends a | ||
| * length-delimited sub-message and reads a fixed32 float — cannot read this at | ||
| * all. It needs its own decoder. | ||
| * | ||
| * Whether Cognition's `input_tokens` already includes the cached tokens is not | ||
| * settled. oh-my-pi sums all four into its total, which suggests exclusive, but | ||
| * that is their convention rather than a measurement of this field. Guessing | ||
| * wrong in the inclusive direction is the expensive mistake: `normalizeCostTokens` | ||
| * only rejects `read + write > input`, so an inflated input passes validation and | ||
| * bills cached tokens at the uncached rate. | ||
| * | ||
| * So the shape is derived from the frame instead of assumed. An input that | ||
| * already covers the cache is left alone; one that cannot possibly cover it is | ||
| * folded. Both branches agree on the case that motivated this — a 58k prompt | ||
| * that is 57k cache read and 1k fresh reads as 58k with a 57k cached subset — | ||
| * and neither can emit `read + write > input`. Replace the derivation with a | ||
| * fixed mapping once a live frame settles the question. | ||
| */ | ||
| export function decodeModelUsageStats(buf: Buffer): CloudChatEvent | null { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes the Devin adapter's usage and error contracts, but the commit updates none of the structure documents mapped to AGENTS.md reference: src/AGENTS.md:L10-L11 Useful? React with 👍 / 👎. |
||
| let wireInput: number | undefined; | ||
| let output: number | undefined; | ||
| let cacheWrite: number | undefined; | ||
| let cacheRead: number | undefined; | ||
| for (const f of iterFields(buf)) { | ||
| if (f.wire !== 0) continue; | ||
| const n = Number(f.value); | ||
| if (!Number.isFinite(n) || n < 0) continue; | ||
| if (f.num === 2) wireInput = n; | ||
| else if (f.num === 3) output = n; | ||
| else if (f.num === 4) cacheWrite = n; | ||
| else if (f.num === 5) cacheRead = n; | ||
| } | ||
| if (wireInput === undefined && output === undefined && cacheRead === undefined && cacheWrite === undefined) { | ||
| return null; | ||
| } | ||
| const read = cacheRead ?? 0; | ||
| const write = cacheWrite ?? 0; | ||
| const rawInput = wireInput ?? 0; | ||
| const promptTokens = rawInput >= read + write ? rawInput : rawInput + read + write; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Devin uses the exclusive shape and fresh input is at least as large as AGENTS.md reference: src/AGENTS.md:L17-L19 Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '--- target implementation ---'
sed -n '860,935p' src/adapters/devin/cloud-direct/chat.ts
printf '%s\n' '--- direct references ---'
rg -n -C 3 'decodeModelUsageStats|promptTokens|cacheRead|cacheWrite|rawInput' src tests 2>/dev/null | head -n 240
printf '%s\n' '--- relevant test file ---'
if [ -f tests/providers/devin-hardening.test.ts ]; then
sed -n '1,260p' tests/providers/devin-hardening.test.ts
else
printf '%s\n' 'tests/providers/devin-hardening.test.ts not present'
fi
printf '%s\n' '--- candidate Devin schemas/docs ---'
rg -n -i -C 3 'input_tokens|cache_read|cache_write|prompt_tokens|usage|cloud-direct|Devin' src tests docs README.md 2>/dev/null | head -n 320Repository: lidge-jun/opencodex Length of output: 50377 🤖 get_repo_knowledge executed:
Length of output: 17360 🏁 Script executed: #!/bin/bash
set -u
sed -n '860,935p' src/adapters/devin/cloud-direct/chat.ts
rg -n -C 3 'decodeModelUsageStats|promptTokens|cacheRead|cacheWrite|rawInput' src tests 2>/dev/null | head -n 240
if [ -f tests/providers/devin-hardening.test.ts ]; then
sed -n '1,260p' tests/providers/devin-hardening.test.ts
fi
rg -n -i -C 3 'input_tokens|cache_read|cache_write|prompt_tokens|usage|cloud-direct|Devin' src tests docs README.md 2>/dev/null | head -n 320Repository: lidge-jun/opencodex Length of output: 50377 🏁 Script executed: sed -n '860,935p' src/adapters/devin/cloud-direct/chat.ts
rg -n -C 3 'decodeModelUsageStats|promptTokens|cacheRead|cacheWrite|rawInput' src tests 2>/dev/null | head -n 240
if [ -f tests/providers/devin-hardening.test.ts ]; then
sed -n '1,260p' tests/providers/devin-hardening.test.ts
fi
rg -n -i -C 3 'input_tokens|cache_read|cache_write|prompt_tokens|usage|cloud-direct|Devin' src tests docs README.md 2>/dev/null | head -n 320Repository: lidge-jun/opencodex Length of output: 50377 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '--- usage-focused tests ---'
rg -n -C 8 'decodeModelUsageStats|ModelUsageStats|cache.*(read|write)|promptTokens|totalTokens' tests/providers/devin-hardening.test.ts
printf '%s\n' '--- internal usage contract ---'
sed -n '360,405p' src/types/request.ts
printf '%s\n' '--- Devin usage mapping ---'
rg -n -C 12 'mergeDevinUsage|cachedInputTokens|cacheCreationInputTokens|promptTokens' src/adapters/devin src/adapters/devin/cloud-direct/chat.ts
printf '%s\n' '--- cost normalization ---'
rg -n -C 12 'function normalizeCostTokens|normalizeCostTokens' srcRepository: lidge-jun/opencodex Length of output: 50375 🏁 Script executed: #!/bin/bash
set -u
rg -n -C 8 'decodeModelUsageStats|ModelUsageStats|cache.*(read|write)|promptTokens|totalTokens' tests/providers/devin-hardening.test.ts
sed -n '360,405p' src/types/request.ts
rg -n -C 12 'mergeDevinUsage|cachedInputTokens|cacheCreationInputTokens|promptTokens' src/adapters/devin src/adapters/devin/cloud-direct/chat.ts
rg -n -C 12 'function normalizeCostTokens|normalizeCostTokens' srcRepository: lidge-jun/opencodex Length of output: 50375 Use an explicit When field 🤖 Prompt for AI AgentsSources: Coding guidelines, Path instructions |
||
| const completionTokens = output ?? 0; | ||
| const total = promptTokens + completionTokens; | ||
| return { | ||
| kind: 'usage', | ||
| promptTokens, | ||
| completionTokens, | ||
| totalTokens: total > 0 ? total : undefined, | ||
| cachedInputTokens: cacheRead, | ||
| cacheCreationInputTokens: cacheWrite, | ||
| reasoningTokens: undefined, | ||
| }; | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------------------- | ||
| // Public API: streamChat | ||
| // ---------------------------------------------------------------------------- | ||
|
|
@@ -864,7 +950,18 @@ export interface CloudChatRequest { | |
| } | ||
|
|
||
| export class CloudChatError extends Error { | ||
| constructor(message: string, public readonly code?: string, public readonly traceId?: string) { | ||
| constructor( | ||
| message: string, | ||
| public readonly code?: string, | ||
| public readonly traceId?: string, | ||
| /** | ||
| * Upstream HTTP status, when the failure was a status line rather than a | ||
| * Connect trailer. Without it the adapter's message reaches | ||
| * `inferHttpStatusFromAdapterMessage`, which does not parse `HTTP 429`, so | ||
| * a live rate limit was classified 502 and core's failover never rotated. | ||
| */ | ||
| public readonly status?: number, | ||
| ) { | ||
| super(message); | ||
| this.name = 'CloudChatError'; | ||
| } | ||
|
|
@@ -987,7 +1084,12 @@ export async function* streamChatEvents(req: CloudChatRequest): AsyncGenerator<C | |
| // The body is not echoed into the message. This error reaches the adapter's | ||
| // error event and /api/logs, and a Connect error can quote the request that | ||
| // produced it - which is the request holding the api_key. | ||
| throw new CloudChatError(`GetChatMessage failed (HTTP ${resp.status})`, undefined); | ||
| // | ||
| // Only the HTTP status line is carried here. A Connect EOS trailer that | ||
| // reports resource_exhausted or unavailable still arrives without a status, | ||
| // so a cap delivered that way keeps the older message-inference path. | ||
| // Mapping trailer codes onto HTTP statuses is deliberately a follow-up. | ||
| throw new CloudChatError(`GetChatMessage failed (HTTP ${resp.status})`, undefined, undefined, resp.status); | ||
| } | ||
| if (!resp.body) { | ||
| throw new CloudChatError('GetChatMessage response had no body stream'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Cognition returns HTTP 507, 501, or another permanent 5xx, this catch-all marks the failure retryable even though the repository's central
isTransientUpstreamStatuspolicy deliberately excludes statuses such as 507. The bridge exposes this flag inresponse.failed, so clients can replay failures that the shared retry policy considers permanent; use that predicate rather thanstatus >= 500and retain the separate 429 handling.AGENTS.md reference: src/AGENTS.md:L17-L19
Useful? React with 👍 / 👎.