Skip to content

fix(sdk): wrap thrown error bodies in real Error instances - #1139

Merged
Astro-Han merged 1 commit into
devfrom
claude/sdk-wrap-client-error
Jun 3, 2026
Merged

fix(sdk): wrap thrown error bodies in real Error instances#1139
Astro-Han merged 1 commit into
devfrom
claude/sdk-wrap-client-error

Conversation

@Astro-Han

Copy link
Copy Markdown
Owner

What

The v2 SDK client's error interceptor only wrapped empty / unparseable
response bodies into a real Error. Non-empty structured bodies — the common
opencode 4xx NamedError shape, e.g. { data: { message } } — passed straight
through, so the { throwOnError: true } path threw a bare POJO.

Every heavy SDK consumer in PawWork uses throwOnError: true (ACP agent.ts /
session.ts, the app session hooks, CLI run.ts, github-copilot plugin), and
those surfaces format errors with instanceof Error / .message. A thrown POJO
therefore surfaced as [object Object] or a blank message instead of the
server's actual error text.

This extracts the interceptor into an exported wrapClientError that converts
non-empty bodies into real Errors on the throw path, preserving the original
parsed body and status under .cause.

Behavior

  • throwOnError path: structured POJO → Error with message from
    data.messagemessagename → a METHOD url -> status description.
    Non-empty string bodies are wrapped too. .cause = { body, status }.
  • result-tuple path (result.error): non-empty bodies returned unchanged,
    so existing field reads (result.error.name, JSON.stringify, …) stay
    byte-for-byte identical.
  • empty / network failures: wrapped unconditionally on both paths with the
    same descriptive message as before (kept ASCII -> and exact format), now with
    .cause attached. This avoids regressing result.error message quality, e.g.
    settings-worktrees errorText() which would otherwise degrade to "{}".
  • Existing Error instances pass through untouched.

Why this shape

Ports the idea from upstream anomalyco/opencode 11363170ca
("fix(sdk): wrap thrown error bodies in Error"), adapted to PawWork's diverged
SDK:

  • Scoped to the active v2 client (packages/sdk/js/src/v2/client.ts) — the
    surface app/run/acp actually use. The helper is inlined and exported from
    that one file rather than upstream's separate error-interceptor.ts module.
  • Empty-body wrapping kept unconditional (upstream gates it on
    throwOnError); PawWork's prior interceptor wrapped empty bodies on both
    paths and a result.error consumer depends on the descriptive message.

The v1 src/client.ts (plugin package) currently registers no error
interceptor; wiring wrapClientError there is a clean, separable follow-up and
is intentionally left out to keep this a single-file change.

Thanks to upstream (@kitlangton) for the original fix.

Test

packages/sdk/js/test/v2-client-error-interceptor.test.ts:

  • Red → green: a { data: { message } } body thrown via
    createOpencodeClient(...).global.health({ throwOnError: true }) now rejects
    with a real Error carrying the extracted message (was the raw POJO).
  • Pure-function branch coverage of wrapClientError (message/name fallbacks,
    string bodies, result-tuple passthrough, empty + network descriptions,
    Error passthrough).
  • End-to-end coverage through createOpencodeClient's real generated-client
    error pipeline (structured throw, raw result-tuple, empty body).

Verification

  • bun test (SDK package): 16 pass, 0 fail.
  • bun run typecheck (SDK package): clean.
  • SDK package is outside the eslint lint:ci scope; no lint gate applies.

The v2 client's error interceptor only wrapped empty/unparseable bodies.
Non-empty structured bodies (opencode 4xx NamedError responses, e.g.
`{ data: { message } }`) passed straight through, so the `throwOnError`
path threw a bare POJO. Downstream formatters that read `instanceof Error`
or `.message` (CLI `run`, ACP, plugins, TUI) then surfaced `[object Object]`
or an empty message instead of the server's error text.

Extract the interceptor into an exported `wrapClientError` and convert
non-empty bodies into real `Error`s on the `throwOnError` path, preserving
the original parsed body and status under `.cause`. The result-tuple path
(`result.error`) still returns the raw body untouched, so field-level reads
stay byte-for-byte identical, and empty/network failures keep their existing
descriptive message on both paths.

Ports the idea from upstream anomalyco/opencode 11363170ca, adapted to
PawWork's diverged SDK: scoped to the active v2 client only, with the helper
inlined (one file) rather than a shared module, and empty-body wrapping kept
unconditional to avoid regressing `result.error` message quality.

Test: red->green in v2-client-error-interceptor.test.ts — a structured
`{ data: { message } }` body now throws a real Error with the extracted
message; pure-function branch coverage plus end-to-end coverage through
`createOpencodeClient`'s real gen error pipeline.
@Astro-Han Astro-Han added the bug Something isn't working label Jun 3, 2026
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@Astro-Han has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minute and 25 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b6d45e64-77b2-4b12-8899-3a47a9028c68

📥 Commits

Reviewing files that changed from the base of the PR and between 5c99f9f and b8470db.

📒 Files selected for processing (2)
  • packages/sdk/js/src/v2/client.ts
  • packages/sdk/js/test/v2-client-error-interceptor.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/sdk-wrap-client-error

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new wrapClientError utility function in the JS SDK client to properly wrap non-2xx error bodies into standard Error objects. This ensures downstream formatters receive a meaningful error message instead of raw objects, while still preserving the original structured body and status under the cause property. The client's error interceptor has been updated to use this utility, and comprehensive unit tests have been added to verify various error scenarios. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@github-actions github-actions Bot added harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority labels Jun 3, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@Astro-Han
Astro-Han merged commit e8d4251 into dev Jun 3, 2026
35 of 36 checks passed
@Astro-Han
Astro-Han deleted the claude/sdk-wrap-client-error branch June 3, 2026 12:09
Astro-Han added a commit that referenced this pull request Jun 3, 2026
Restructure the right-side Status panel from 2 sections (Progress + Sources) into 4 (Progress -> Workspace/Git -> Changed files/Artifact -> Sources), fold the standalone Files tab into the panel as a compact changed-files list, and move the titlebar worktree badge into the Git section.

Change boundary (packages/app, packages/ui):
- Status panel: new Git section (diff stats -> Review, branch row, worktree indicator with tooltip + open-directory) and Artifact section (changed files with hover/focus-visible open + reveal); Git section hidden outside a git repo. Data from sync.data.vcs (branch), sessionInfo().executionContext.activeWorktree (worktree), aggregateFiles(turn_change_aggregate) (diff stats).
- Files tab removed from RightPanelStaticTab / RIGHT_PANEL_TAB_META / command palette / keybind; legacy persisted "files" coerced to "status" via coerceLegacySidePanelTab + migrateLegacyRightPanelTab.
- Titlebar PawworkWorktreeBadge removed (worktree info now lives in the panel Git section).
- New `changes` icon registered (packages/ui).

Verification:
- typecheck clean; full app unit suite 1753 pass / 0 fail.
- Visual: status-summary-panel snap (4 sections + rest->hover open/reveal) plus a dev:desktop walk in the real Electron host (dark theme, zh locale, live sidecar) - Git section populated from real VCS (+N/-N, branch main), Artifact section listing a real session file (sample-test.md, +76).
- Codex adversarial review P1 findings fixed; 5 review threads resolved.

Brought current with dev: one conflict in context/layout.tsx - dev had extracted the inline layout helpers into context/layout-state.ts / layout-projects.ts (#1056 slice work). Took dev's refactored layout.tsx and ported this branch's defaultSidePanelTab `| "files"` widening into layout-state.ts. Final state 0 behind dev; also merged #1135/#1136/#1137/#1139 cleanly (no file overlap).

Deferred: dev:desktop worktree-indicator tooltip + non-git-hide are state-conditional and were not exercised live (hover open/reveal covered by the snap). session-side-panel.test.tsx isolation failure is the pre-existing mock.module warmup flake (its @/context/command mock omits matchKeybind/parseKeybind that terminal.tsx imports transitively); green in the full suite, tracked under the #1084 mock.module cleanup.

Relates #1056.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant