Add trace ID extraction and appending to error messages - #1664
Conversation
🦋 Changeset detectedLatest commit: d2e48e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit d2e48e4. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 1917333. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.45.1-claude-trace-id-error-messages-cbzqwl.0.tgzCLI ( npm install ./e2b-cli-2.17.2-claude-trace-id-error-messages-cbzqwl.0.tgzPython SDK ( pip install ./e2b-2.45.1+claude.trace.id.error.messages.cbzqwl-py3-none-any.whl |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mishushakov
left a comment
mishushakov
left a comment
There was a problem hiding this comment.
reviewed, please remove duplicate method in JS SDK and you can modify the default Error classes to accept a trace field in the constructor that will append it to the message, which is cleaner than relying on helpers side-effects
|
Addressed the review in 0adc234:
Also added CLI test cases for the AWS raw-value fallback and header priority. Generated by Claude Code |
|
@djeebus Second pass — I re-reviewed after 0adc234 and verified the build locally. All five of my earlier comments are addressed; details plus a few independent findings below. My earlier comments
A. The feature is inert against production today, and 2 of its 3 header paths likely never fireYou flagged this in the description and asked someone to verify which header a real failed response returns. I checked against live prod:
No trace header on any path. And Suggestion: land B. JS error-class arity is now inconsistent, and it's a live trap
apiErrorFromCode(500, 'boom', AuthenticationError, 'STACK', 'realtrace')
// typechecks clean; message === "500: boom (trace ID: STACK)"No current call site hits it — Related: six call sites now read C. Unrelated change bundled in
D.
|
|
Second-pass items addressed in b7eeae2:
A is @djeebus's call, so I've left the GCP/AWS branches in place for now and updated the PR description with your production findings (no trace header on any live path, Build Packages failure on 0adc234 was the transient 503 you diagnosed; the new push re-runs CI. Generated by Claude Code |
|
@djeebus Consolidating Finding A into one self-contained write-up, since it's the last open call on this PR and the surrounding decisions have now shifted what's at stake. Everything you need to decide is below — no need to dig back through the thread. What this PR does, and where Finding A sitsThe PR reads a trace ID off the headers of a failed HTTP response and appends The rest of the review is settled — error classes now take a trailing options object in JS / keyword-only Finding A is the one thing left: do the GCP and AWS branches belong in the shipped surface at all? The evidence: none of the three headers comes back from any live E2B endpoint
No Why the two edge branches are structurally unlikely, not just currently unused
So Why this now matters more than when I first raised itWhen the parser was internal, shipping unused branches was just dead weight — delete it later, nobody notices. Now that That's the crux: publishing it freezes it. Worth getting right in this PR rather than the next one. What trimming to
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2da2723. Configure here.
There was a problem hiding this comment.
TASTE.md review
The shape of this change is right where TASTE points. The move from (message, stackTrace?, traceId?) to a trailing options object is the golden rule applied correctly, ErrorOpts/StackTraceErrorOpts use the Opts suffix and are named and exported rather than inlined, traceId/trace_id mirror across JS and Python, Id is cased as a word while prose keeps "trace ID" all-caps, JSDoc uses @param/@returns/@example against Python's :param:/:return:, and both the JS extractTraceId and the Python extract_trace_id return absence rather than throwing.
I verified the change locally: tsc --noEmit and ty check are clean, and the 58 JS plus 23 Python unit tests covering the new paths pass.
Six inline comments below. Two are worth acting on before merge (the dropped stack trace for 401/429, and the changeset severity), the rest are consistency nits.
One gap that has no diff line to attach to
Nine control-plane methods in packages/js-sdk/src/sandbox/sandboxApi.ts short-circuit 404 before handleApiError ever runs:
if (res.error?.code === 404) {
throw new SandboxNotFoundError(`Sandbox ${sandboxId} not found`)
}
const err = handleApiError(res)So getInfo, setTimeout, getMetrics, pause/resume and friends never pick up a trace ID, even though res.response.headers is in scope one line above. The same holds for the nine sync and nine async sites in e2b/sandbox_sync/sandbox_api.py and e2b/sandbox_async/sandbox_api.py.
This matters more than a normal coverage gap because it is exactly the example the PR description leads with:
await Sandbox.connect('already-dead-sandbox-id')
// 404: sandbox not found (trace ID: 105445aa7843bc8bf206b12000100000)As written, that call produces no trace ID. Deciding the 404 class at the call site is what TASTE asks for, but the trace ID is orthogonal to which class you pick — it can be attached at each of those sites ({ traceId: extractTraceId(res.response.headers) }) without changing the centralized status mapping. It is a mechanical change across the three files, and it would move the feature from "HTTP errors other than sandbox-not-found" to "the failure users actually hit".
Non-blocking notes
- The
(trace ID: …)suffix is now written out in three places:formatMessageinjs-sdk/src/errors.ts,format_message_with_trace_idine2b/exceptions.py, andthrowE2BRequestErrorincli/src/utils/errors.ts. The two SDK copies are unavoidable, but the CLI already importsextractTraceIdfrome2b, so its copy of the message shape can drift from the SDK's without anything noticing. Relatedly, the CLI'sE2BRequestErrorinterpolates the ID into the message but does not expose atraceIdfield the way the SDK errors now do. - TASTE asks that error messages say what to do, not just what failed.
(trace ID: abc123)is a bare fact — a reader who has not seen the changeset does not know it is meant to be handed to E2B support. Worth considering whether the suffix should carry that instruction, weighed against how much longer it makes every message. - The PR description notes that envd connect-RPC errors are not covered because
connectrpc.ConnectErrorexposes no response metadata in Python and the JS side is kept symmetric. That is the right call under the parity rule — better to have both surfaces miss it than to have JS-only trace IDs on RPC errors.
Sent by Cursor Automation: SDK complies with TASTE.md
| return new AuthenticationError( | ||
| content ? `${message} - ${content}` : message | ||
| content ? `${message} - ${content}` : message, | ||
| { traceId: opts?.traceId } |
There was a problem hiding this comment.
The 401 and 429 branches silently drop opts.stackTrace.
This is the pattern TASTE warns about in the dual-method section: forward the whole options surface you were handed rather than destructuring the one field you happen to care about, because the fields you don't name disappear without a sound.
It has a real effect now that it didn't have before this PR. AuthenticationError and RateLimitError both accept and apply opts.stackTrace as of this change, and getFileUploadLink calls handleApiError(fileUploadLinkRes, FileUploadError, { stackTrace }). So an expired API key during a template build produces an error whose stack points into the SDK instead of at the user's builder call — which is the thing stackTrace exists to prevent.
Both branches can just pass opts through:
if (code === 401) {
const message = 'Unauthorized, please check your credentials.'
return new AuthenticationError(
content ? `${message} - ${content}` : message,
opts
)
}
if (code === 429) {
const message = 'Rate limit exceeded, please try again later'
return new RateLimitError(content ? `${message} - ${content}` : message, opts)
}The Python side has the mirror of this: api_exception_from_code returns AuthenticationException(text, trace_id=trace_id) and RateLimitException(text, trace_id=trace_id) without the .with_traceback(stack_trace) that the default branch applies. Worth fixing in both so they stay in step.
| * } | ||
| * ``` | ||
| */ | ||
| export function extractTraceId(headers?: Headers | null): string | undefined { |
There was a problem hiding this comment.
Headers | null in a public signature.
TASTE is explicit that absence is undefined and never null, and that where an API hands back null you normalize at the boundary so the union never reaches a consumer. extractTraceId is exported from index.ts, so this union is part of the published surface.
Nothing in the SDK passes null today — api/index.ts and envd/api.ts pass response.response.headers, and the CLI passes res.response?.headers, which is Headers | undefined. The only caller is tests/traceId.test.ts:14, which asserts extractTraceId(null) works. So headers?: Headers costs nothing but that one assertion.
There is also a small parity gap in the same signature: JS makes the parameter optional (extractTraceId() is legal), while Python declares headers: Optional[Mapping[str, str]] with no default, so extract_trace_id() is a TypeError. Adding = None on the Python side lines the two up.
| @@ -0,0 +1,47 @@ | |||
| --- | |||
| "e2b": patch | |||
There was a problem hiding this comment.
patch understates this — the JS error constructors changed shape.
The changeset itself spells out the migration two paragraphs down: new SandboxError(message, stackTrace) becomes new SandboxError(message, { stackTrace }). Every error class in errors.ts is exported from the package entry point, e2b is at 2.39.0, and the old positional call now silently lands a string in the opts slot — opts?.traceId and opts?.stackTrace both come back undefined, so a caller who was re-pointing a stack trace loses it with no type error at the boundary and no runtime signal.
The same is true in the other direction for VolumeError, which used to accept anything Error accepts and now rejects stackTrace outright.
A changeset that documents a migration path isn't a patch. Either bump the severity so the release notes carry the break, or keep the old form working — constructor(message?: string, opts?: string | StackTraceErrorOpts) with the string form marked @deprecated and a "use { stackTrace } instead" note would match how TASTE says to retire an identifier.
Python is fine here in practice: SandboxException.__init__ narrows to one positional argument, but I scanned every construction of the affected classes across the SDK and no internal call site passes more than one, so only third-party code doing SandboxException(a, b) would break.
| import { createApiLogger } from '../logs' | ||
| import { | ||
| SandboxError, | ||
| ErrorOpts, |
There was a problem hiding this comment.
ErrorOpts is a type in a value import.
Two lines above, this file uses import type { components, paths } from './schema.gen', and js-sdk/tsconfig.json sets isolatedModules: true, which is exactly the setting that makes the value/type distinction load-bearing rather than cosmetic. TASTE calls out the same split on the export side ("runtime values use export and type-only names use export type — never mixed") and index.ts follows it in this PR with export type { ErrorOpts, StackTraceErrorOpts }.
A separate import type { ErrorOpts } from '../errors' keeps this consistent. Same applies to StackTraceErrorOpts in api/index.ts and ErrorOpts in sandbox/filesystem/index.ts.
| ) -> Exception: ... | ||
|
|
||
|
|
||
| def format_message_with_trace_id(message: str, trace_id: Optional[str] = None) -> str: |
There was a problem hiding this comment.
Should be _format_message_with_trace_id.
e2b.exceptions is an importable public module and this name has no underscore, so it reads as supported API even though it isn't in __all__. Its JS counterpart, formatMessage in errors.ts, is module-private and not exported — the two surfaces should agree on what's reachable.
The siblings already use the convention: _DEFAULT_API_ERROR_MAP in envd/api.py, _API_KEY_PATTERN and _API_KEY_EXAMPLE in api/__init__.py, _AWS_ROOT_PATTERN in the new trace_id.py right in this PR.
(ExceptionFactory above it is a different case — it appears in the public signatures of handle_api_exception and format_envd_api_exception, so it does need to be nameable.)
| * Context for errors that can point somewhere other than where they were | ||
| * constructed. | ||
| */ | ||
| export interface StackTraceErrorOpts extends ErrorOpts { |
There was a problem hiding this comment.
Nit on the name: StackTraceErrorOpts parses as "options for a StackTraceError", by analogy with SandboxConnectOpts for Sandbox.connect or FilesystemWriteOpts for a filesystem write. There is no StackTraceError; what this actually means is "ErrorOpts, plus a stack trace".
Something like TracedErrorOpts or ErrorOptsWithStackTrace reads closer to the split you're drawing. Both are exported from index.ts, so the name is the whole story for anyone reading the type list.
The split itself is right — making VolumeError reject stackTrace at the type level instead of accepting and ignoring it is the correct fix for what the previous commit left behind.
|
@devin-ai-integration please check |
When a failed API or envd response carries a trace header (X-Trace-ID, or the GCP X-Cloud-Trace-Context / AWS X-Amzn-Trace-Id edge headers), the error message now ends with "(trace ID: ...)" so users can include the ID when reporting a failure, and it is readable off the error itself as `error.traceId` / `exception.trace_id`. The header parser is exported for callers that handle an E2B response themselves: `extractTraceId` in JS, `extract_trace_id` in Python. JS error classes take their constructor context as a trailing options object. Only the classes actually handed a stack trace accept one — InvalidArgumentError, TemplateError, BuildError, FileUploadError, plus AuthenticationError/RateLimitError, which a 401 or 429 during a template file upload can produce; the rest take `ErrorOpts` (traceId only), keeping the narrowing from #1732. Not covered: envd RPC (connect) paths, because Python's ConnectError exposes no response metadata and the JS side is kept symmetric. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f463f67 to
8c5d68b
Compare
| @@ -94,18 +128,27 @@ export class SandboxNotFoundError extends NotFoundError { | |||
| * Thrown when authentication fails. | |||
| */ | |||
| export class AuthenticationError extends Error { | |||
There was a problem hiding this comment.
this error has no callers that pass a stack trace ?
| export class RateLimitError extends SandboxError { | ||
| constructor(message: string) { | ||
| super(message) | ||
| constructor(message: string, opts?: ErrorOptsWithStackTrace) { |
There was a problem hiding this comment.
this error has no callers that pass stack trace ?
AuthenticationError and RateLimitError take ErrorOptsWithStackTrace because getFileUploadLink is a real caller: it forwards the builder's captured frame into handleApiError, which routes 401 and 429 through apiErrorFromCode. Cover it end to end against a mocked upload-link response so the reason those two classes differ from the rest of the #1732 narrowing is visible in a test rather than only in review. Also spell out in the changeset that traceId lives on one root per domain (T-57), so readers narrow to the right one instead of assuming SandboxError covers every call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audited every construction of an exported error class. Only InvalidArgumentError, TemplateError, BuildError, and FileUploadError are ever handed a stack trace, so those four keep ErrorOptsWithStackTrace and the rest take ErrorOpts. AuthenticationError and RateLimitError were the odd pair: nothing constructs them with a frame directly, they only inherited one because apiErrorFromCode swaps in a different class for 401/429 and forwarded the opts it was given. A bad key or a rate limit is a property of the request rather than of the builder step that made it, so both branches now pass the trace ID alone and the two classes no longer advertise a parameter they never get. Same on the Python side, where 401/429 stop calling with_traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VolumeNotFoundError, VolumePathNotFoundError, and SecretNotFoundError are thrown at the call site before handleApiError sees the response — 404 meaning is decided per call site (T-60) — so they were the one group of errors the feature missed. Each raise site now reads the header itself. 11 sites in JS (volume/index.ts, secret.ts) and 11 in each Python flavor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
This change adds trace ID extraction from HTTP response headers and appends them to error messages across the SDK and CLI. When API or envd requests fail, the error message now includes the trace ID (e.g.,
(trace ID: abc123)) so users can report it to E2B support for correlation with server-side traces.Usage examples
The trace ID shows up in existing error messages when the failed response carries a trace header, and is readable off the error itself.
JS SDK
Python SDK (sync and async)
CLI
Key Changes
Trace ID parsing:
packages/js-sdk/src/traceId.ts(single implementation, exported asextractTraceIdfrom thee2bpackage and reused by the CLI) ande2b/trace_id.py(exported asextract_trace_idfor JS/Python parity):X-Trace-ID(direct),X-Cloud-Trace-Context(GCP edge),X-Amzn-Trace-Id(AWS edge)Root=1-<8 hex>-<24 hex>to the 32-hex form the server logs asedge_trace_idError classes take the trace ID as constructor context and append it to the message: a trailing options object in JS (
new SandboxError(message, { traceId }), uniform across every class) and a keyword-only argument in Python (SandboxException(message, trace_id=...)). Two option types are exported:ErrorOpts(traceId) andErrorOptsWithStackTrace(addsstackTrace). Only the four classes actually handed a stack trace take the latter —InvalidArgumentError,TemplateError,BuildError,FileUploadError— so refactor(js-sdk): drop unused stackTrace params from error constructors, reparent volume not-found errors #1732's narrowing is preserved and passingstackTraceto any other class is a type error rather than silently ignored.apiErrorFromCodeswaps inAuthenticationError/RateLimitErrorfor 401/429 and deliberately does not forward the caller's frame to them: a bad key or a rate limit is a property of the request, not of the builder step that made it.apiErrorFromCode/api_exception_from_codeand the envd error maps thread both through.Breaking (JS, hence the
minorchangeset): the options object replaces the positionalstackTraceparameter on the four classes that had one, sonew TemplateError(message, stackTrace)becomesnew TemplateError(message, { stackTrace })(likewiseInvalidArgumentError,BuildError,FileUploadError). TypeScript callers get a compile error on the old form; plain-JS callers silently lose the stack trace. Python is unaffected — its exceptions already took a single positional argument.Direct 404 raises:
VolumeNotFoundError/VolumePathNotFoundError/SecretNotFoundError(and their Python counterparts) are thrown at the call site beforehandleApiErrorsees the response — per T-60, 404 meaning is decided per call site — so each of those raise sites reads the header itself. 11 sites in JS (volume/index.ts,secret.ts) and 11 in each Python flavor (volume_sync/volume_async,secret_sync/secret_async).Wiring:
handleApiError(JS),handle_api_exception(Python),handleEnvdApiError/handle_envd_api_exception/ahandle_envd_api_exception(envd HTTP), and the CLI'shandleE2BRequestErrorread the response headers and pass the extracted ID into the error constructors.Not covered — envd RPC (connect) paths:
sandbox.commands/sandbox.filesRPC errors do not carry the trace ID, because Python'sconnectrpc.ConnectErrorexposes no response metadata, and the JS side is kept symmetric. This is where most in-sandbox failures surface, so the feature currently reaches HTTP API and envd HTTP (file transfer) errors only.Test coverage: header extraction and normalization edge cases, priority ordering, case-insensitivity, missing-header fallbacks, custom
errorClass/error_mappass-through, and CLI parsing — in JS, Python (sync + async), and the CLI.Note on the server side
Verified against production (2026-08-13): no failed
api.e2b.devor sandbox-edge response currently returns any trace header, so this change is inert until the server emits one. The API handlers already compute the trace ID (c.Set("traceID", traceID)in infra) — a one-linec.Header("X-Trace-ID", traceID)there would activate the primary path. The GCP/AWS edge-header branches are speculative (those are request-side conventions); whether to keep them or trim toX-Trace-ID-only once the infra change lands is an open reviewer question for @djeebus.https://claude.ai/code/session_0147b6bTi2gq4Yvm7VrLrxLD