Fix error info loss in rest.request() commands and batch-publish#236
Fix error info loss in rest.request() commands and batch-publish#236umair-ably merged 9 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Walkthrough This PR fixes structured error information being lost in several commands that use Changes
Review Notes
|
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR fixes a genuine bug: structured Ably error info (codes, messages, HTTP status) was being stringified before reaching the fail() pipeline, stripping all useful context. The fix is clean and well-targeted.
What I checked
CommandError.fromHttpResponse()and the newfrom()plain-object branch- All 6 command changes
- Test coverage for new code paths
No issues found
CommandError.fromHttpResponse() uses response.errorCode || undefined to convert both 0 and null to undefined (Ably error code 0 = no code). Intentional, and tested.
batch-publish.ts: The local ErrorInfo interface defines error as a plain object with message: string, code: number. Passing it directly to fail() is safe — CommandError.from() now duck-types it via the new plain-object branch. The errorInfo.error || CommandError.fromHttpResponse(...) fallback is clean.
keystroke.ts: reason is typed Error | null | string | undefined. Passing it directly to fail() instead of pre-extracting reason.message is correct — CommandError.from() handles all four cases, and for ErrorInfo extends Error it extracts code and statusCode properly.
Test assertions: Tests checking error?.message contains the error code (e.g. "40101") are correct — fail() appends "Ably error code: 40101" to the human message before calling this.error().
Test placement: The new test in rooms/list.test.ts sits outside the describe("error handling") block, matching the pre-existing pattern in that file (line 146 does the same). Not introduced by this PR.
Overall
Implementation is correct. CommandError.from(), fromHttpResponse(), and the command changes compose cleanly. Tests cover key scenarios including the zero-errorCode edge case. Ready to merge.
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI error pipeline so commands preserve and surface structured Ably error information (error message, Ably error code, HTTP status) instead of losing it via pre-stringified messages.
Changes:
- Extend
CommandError.from()to extractmessage/code/statusCodefrom plain JSON-like error objects, and addCommandError.fromHttpResponse()forrest.request()error responses. - Update
channels list,rooms list,spaces list, andspaces getto usefromHttpResponse()instead of interpolating status codes into strings. - Update
channels batch-publishandrooms typing keystroketo pass structured errors intofail()rather than stringifying/decomposing them, plus add unit/command tests for these behaviors.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/errors/command-error.ts |
Adds plain-object duck-typing in from() and introduces fromHttpResponse() to preserve structured HTTP/Ably error fields. |
src/commands/spaces/list.ts |
Uses CommandError.fromHttpResponse() when rest.request() returns a non-200 response. |
src/commands/spaces/get.ts |
Uses CommandError.fromHttpResponse() on non-200 responses to preserve error details. |
src/commands/rooms/list.ts |
Uses CommandError.fromHttpResponse() rather than status-only string errors. |
src/commands/channels/list.ts |
Uses CommandError.fromHttpResponse() rather than status-only string errors. |
src/commands/channels/batch-publish.ts |
Stops interpolating/decomposing batch errors; passes structured error objects (or fromHttpResponse() fallback) into fail(). |
src/commands/rooms/typing/keystroke.ts |
Passes the structured attach failure reason into fail() instead of stringifying it. |
test/unit/errors/command-error.test.ts |
Adds unit tests for plain-object extraction and fromHttpResponse(). |
test/unit/commands/spaces/list.test.ts |
Adds command-level test ensuring errorMessage/errorCode surface from HTTP error responses. |
test/unit/commands/spaces/get.test.ts |
Adds command-level test ensuring errorMessage/errorCode surface from HTTP error responses. |
test/unit/commands/rooms/list.test.ts |
Adds command-level test ensuring errorMessage/errorCode surface from HTTP error responses. |
test/unit/commands/channels/list.test.ts |
Adds command-level test ensuring errorMessage/errorCode surface from HTTP error responses. |
test/unit/commands/channels/batch-publish.test.ts |
Adds tests ensuring batch-publish failure paths surface structured error messages (not [object Object]). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umair-ably
force-pushed
the
fixRestErrors
branch
from
beb32a6 to
7e84a3c
Compare
|
Existing bug, following commands skip
|
AndyTWF
left a comment
AndyTWF
left a comment
There was a problem hiding this comment.
Think I started reviewing before this got moved back to in draft, so will pause for now
| } else if (statusChange.current === RoomStatus.Failed) { | ||
| this.fail( | ||
| `Failed to attach to room ${roomName}: ${reasonMsg || "Unknown error"}`, | ||
| reason || `Failed to attach to room ${roomName}`, |
There was a problem hiding this comment.
This change doesn't seem directly related to the main PR purpose, but this gives a horrible output.
Using: Account=Ably Realtime (X) • App=Default App (X) • Key=Subscribe only (X)
✓ Connected to room: abc.
[AblySDK Error] Ably: Protocol.onNack(): serial = 0; count = 1; err = [_ErrorInfo: Channel denied access based on given capability; channelId = abc::$chat; statusCode=401; code=40160; see https://help.ably.io/error/40160 ]
/Users/andyford/Ably/cli/node_modules/.pnpm/@oclif+core@4.2.10/node_modules/@oclif/core/lib/errors/error.js:44
err = new cli_1.CLIError(input, options);
^
CLIError: Channel denied access based on given capability; channelId = abc::$chat
Run "ably auth keys list" to check your key's capabilities for this resource, or update them in the Ably dashboard.
Ably error code: 40160 (https://help.ably.io/error/40160)
at Object.error (/Users/andyford/Ably/cli/node_modules/.pnpm/@oclif+core@4.2.10/node_modules/@oclif/core/lib/errors/error.js:44:15)
at TypingKeystroke.error (/Users/andyford/Ably/cli/node_modules/.pnpm/@oclif+core@4.2.10/node_modules/@oclif/core/lib/command.js:210:23)
at TypingKeystroke.error (file:///Users/andyford/Ably/cli/dist/src/interactive-base-command.js:26:22)
at TypingKeystroke.fail (file:///Users/andyford/Ably/cli/dist/src/base-command.js:1216:14)
at file:///Users/andyford/Ably/cli/dist/src/commands/rooms/typing/keystroke.js:111:30
at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
code: undefined,
oclif: { exit: 2 },
skipOclifErrorHandling: undefined,
suggestions: undefined
}
Node.js v22.14.0
There was a problem hiding this comment.
ahh i've seen this before... i'll change this so the outer catch can handle the error properly
There was a problem hiding this comment.
fixed... now propagates up to the appropriate catch where it is handled properly... also, checked and handled a few other places where this would've happened.
Also the newest commit shows I've suppressed SDK logs, this is expected. Intention behind it is that not doing this basically shows our CLI errors and the base SDK error which often just repeat each other. The CLI's errors are nicer so keeping them. User's can still see both if they want in --verbose mode. We also already follow this same practice for Chat.
Keeping these as part of this PR (it was the intent of this PR that flagged this issue so makes sense to fix it here too).
There was a problem hiding this comment.
Cleaner, but the error is now:
✓ Connected to room: abc.
› Error: Channel denied access based on given capability; channelId = abc::$chat
So two things - we're saying "connected to room" when connection is established but not attachment (which is sorta misleading)
Also do we want to expose the underlying channel in the error message, or do something more room-friendly?
There was a problem hiding this comment.
sweet, that actually breaks a previously unwritten rule we had in the codebase in that the action-specific message already implies successful attachment.
I've done a sweep to make sure the timing of the success messages are correct too e.g. are we actually saying subscribed when we are subscribed. (most of these changes are in 5714d9e due to fixing rebase conflicts)
I've documented this rule now too.
Good shout on sanitising the channel names... I've done this at the base so it applies to spaces too
Docs + sanitising
7101cc6
There was a problem hiding this comment.
Review summary: All 19 files reviewed. No blocking issues found. The PR cleanly fixes error info loss in rest.request() commands and missing HTTP status checks. fromHttpResponse correctly handles errorCode=0 (maps to undefined), plain-object duck-typing in from() is correctly placed after instanceof Error check, all three new statusCode !== 200 guards are correctly structured. One minor note: the new test in rooms/list.test.ts is placed outside the existing error handling describe block (following a pre-existing pattern at line 146). Not a blocker. Approving.
|
test comment |
|
Walkthrough: This PR fixes structured error info being lost in commands using rest.request() or manually decomposed error objects. Instead of 'Failed to list spaces: 500', users now see the actual server error message, Ably error code, HTTP status code, and actionable hints. Three commands that silently swallowed API errors now surface failures properly. Changes: (1) Error Infrastructure - src/errors/command-error.ts: Added fromHttpResponse() factory for HttpPaginatedResponse; extended from() to duck-type plain objects with a message property. (2) Commands - Error Info Loss - channels/list, rooms/list, spaces/list, spaces/get: Replace string-interpolated status codes with fromHttpResponse(). (3) Commands - Error Info Loss - channels/batch-publish: Pass error objects directly to fail() instead of interpolated strings (3 paths). (4) Commands - Error Info Loss - rooms/typing/keystroke: Pass room.error directly to fail() instead of stringifying. (5) Commands - Missing Status Checks - channels/occupancy/get and spaces/occupancy/get: Add statusCode guard; previously returned all-zero metrics silently on API error. (6) Commands - Missing Status Checks - push/batch-publish: Add statusCode guard; previously reported success on API error. (7) Tests: 8 new tests for fromHttpResponse() and plain-object handling; 4 tests for rest.request() commands; 2 for batch-publish; 6 for occupancy/get. Review Notes: Behavioral change - occupancy get commands and push batch-publish now exit on API failure instead of returning misleading results. No new dependencies. All 19 modified files have test coverage. No migration considerations. |
umair-ably
force-pushed
the
fixRestErrors
branch
from
b29bd0a to
9ee4981
Compare
|
PLACEHOLDER - to be replaced |
There was a problem hiding this comment.
Review: Fix error info loss in rest.request() commands and batch-publish
This is a well-structured fix. The CommandError.fromHttpResponse() factory, the duck-typing extension to CommandError.from(), and the failurePromise pattern for setupRoomStatusHandler are all correct and cleanly implemented. Test coverage is solid.
Two issues worth flagging:
Bug: rooms/reactions/send.ts doesn't capture failurePromise
src/commands/rooms/reactions/send.ts:99 still calls setupRoomStatusHandler without capturing the return value:
// Before this PR (old behavior):
this.setupRoomStatusHandler(room, flags, { roomName }); // returned void
// After this PR (new behavior):
this.setupRoomStatusHandler(room, flags, { roomName }); // returns { failurePromise } — unhandledSince setupRoomStatusHandler now returns a Promise<never> that rejects on RoomStatus.Failed, not capturing it means any room failure during this command creates an unhandled promise rejection. Every other command in this PR was updated to use Promise.race — this one was missed.
For rooms/reactions/send.ts specifically (a one-shot command that exits immediately after sending), the practical risk is low: if the room fails during room.attach(), that call throws and the catch block handles it. But if the failure fires during the brief window between attach succeeding and the reactions.send() resolving, the failurePromise rejection goes unhandled, emitting a Node.js UnhandledPromiseRejection warning as the process exits.
The fix is straightforward — the command doesn't use waitAndTrackCleanup, so the failurePromise doesn't need to be raced; it just needs to be attached to the catch chain. One option:
const { failurePromise } = this.setupRoomStatusHandler(room, flags, { roomName });
// ... (existing code) ...
await room.reactions.send(...);
// No race needed — if failurePromise rejected, it would have already thrown above
// But attach the handler so it's not unhandled:
failurePromise.catch((e) => { /* swallow — caught by outer try/catch if it occurs during await */ });Or simply: since this is a one-shot command, room.attach() covers the failure case. You could also just not call setupRoomStatusHandler at all here since it's not a subscribe command and there's no success/listening message needed. That's a design call for the team.
Minor: New test in rooms/list.test.ts placed outside describe("error handling")
The new test at line 156 is at the top level of describe("rooms:list command"), not inside describe("error handling") where it belongs:
describe("rooms:list command", () => {
it("should handle non-200 response with error", ...) // top-level
it("should surface errorCode and errorMessage ...", ...) // ← new test, also top-level
describe("functionality", ...)
describe("error handling", ...) // this is where it should live
});
Doesn't affect test execution, just organization.
Everything else looks correct: the errorCode || undefined treating 0 as "no code" is intentional and explicitly tested; the keystroke.ts refactor from event-callback-driven to sequential is a clean improvement; the base-command.ts suppression of duplicate SDK error logs is the right call given that command-level error handling now propagates them properly.
umair-ably
force-pushed
the
fixRestErrors
branch
from
bc8a2d3 to
c93d7fb
Compare
umair-ably
force-pushed
the
fixRestErrors
branch
from
a5e4452 to
7101cc6
Compare
umair-ably
force-pushed
the
fixRestErrors
branch
from
e174cf1 to
487029a
Compare
umair-ably
force-pushed
the
fixRestErrors
branch
from
487029a to
6202b06
Compare
umair-ably
force-pushed
the
fixRestErrors
branch
from
6202b06 to
7a901db
Compare
umair-ably
force-pushed
the
fixRestErrors
branch
from
7a901db to
9cf3926
Compare
…batch-publish These three commands called rest.request() without checking statusCode, silently swallowing API errors (4xx/5xx) since the Ably SDK returns HTTP errors as part of the response rather than throwing: - channels occupancy get: returned all-zero metrics on error - spaces occupancy get: returned all-zero metrics on error - push batch-publish: reported success on error Add statusCode !== 200 guard with CommandError.fromHttpResponse() to all three, matching the pattern established by the other fixes in this PR. Add corresponding test coverage for HTTP error responses.
…rror logs setupRoomStatusHandler() called this.fail() from inside the onStatusChange callback. Since fail() throws a CLIError, this became an unhandled exception — Node dumped a full stack trace instead of showing a clean error message. Fix by having setupRoomStatusHandler return a failurePromise that rejects on RoomStatus.Failed. All 7 long-running room commands now race this against waitAndTrackCleanup so failures propagate cleanly to the outer try/catch. Refactored rooms typing keystroke from a custom onStatusChange callback to the standard sequential await pattern used by every other room command. Also suppress the Pub/Sub SDK's [AblySDK Error] stderr output in non-verbose mode — every realtime command already has its own error handling (setupConnectionStateLogging, setupChannelStateLogging, setupRoomStatusHandler) making the raw SDK output redundant. Still shown with --verbose, matching the Chat SDK's LogLevel.Silent default.
…placed error tests
…s from errors Remove "Connected to room" messages (fires on attach, not connection) and replace with action-specific messages after the actual operation completes. Fix success message timing for occupancy/reactions/message-reactions subscribe commands where the handler message fired before the subscribe call. Sanitize internal SDK channel suffixes (::$chat, ::$space, ::$cursors) from error messages in AblyBaseCommand.fail() so users see room/space names.
…ulted in messy stacktrace being printed
umair-ably
force-pushed
the
fixRestErrors
branch
from
9cf3926 to
f22cca6
Compare
umair-ably
dismissed
AndyTWF’s stale review
flagged in slack... unblocking this PR. Issues (if needed) can be addressed trivially in subsequent PRs
Summary / Intent
Several commands discard structured error information (Ably error codes, server error messages, HTTP status codes) by converting errors to strings before passing them to the
fail()error pipeline. This means users see unhelpful messages like"Failed to list spaces: 500"instead of the server's actual error message, Ably error code, and actionable hints.The
fail()→CommandError.from()pipeline is already designed to extract and display all of this — ErrorInfo codes, status codes, help URLs, and friendly hints like "Check your API key". But it only works when it receives structured error data, not pre-stringified messages.Root cause
The Ably SDK's
rest.request()returns anHttpPaginatedResponseon API errors (rather than throwing), which haserrorCode,errorMessage, andstatusCodefields. Four commands checkedstatusCode !== 200but only interpolated the status code into a string:this.fail(Failed to list spaces: ${response.statusCode}, ...). This bypassed all error extraction inCommandError.from().Similarly,
batch-publishmanually decomposed error objects intoerrMsg/errorCodestring variables and interpolated them into a message string, losing the structured data. Androoms/typing/keystrokestringifiedroom.error(an Ably ErrorInfo) before passing it tofail().Three additional commands had a worse variant of this bug: they didn't check
statusCodeat all afterrest.request(), silently swallowing API errors and returning misleading successful results.What changed
CommandErrorimprovements (src/errors/command-error.ts):from()now handles plain objects — added duck-typing for objects with a stringmessageproperty, extractingmessage,code, andstatusCode. Previously onlyErrorinstances were duck-typed; plain JSON error objects (like those in batch API response bodies) fell through toString(error). This makes it safe to pass parsed error objects directly tofail().fromHttpResponse()— a static factory that creates aCommandErrorfrom anHttpPaginatedResponse, extractingerrorCode,errorMessage, andstatusCode. Falls back to a generic message whenerrorMessageis empty. Parameter types acceptnumber | null | undefinedforerrorCodeandstring | null | undefinedforerrorMessageto match the actual SDK type.Command fixes (error info loss):
rest.request()commands (channels list,rooms list,spaces list,spaces get) — usefromHttpResponse()instead of string interpolation.channels batch-publish(3 error paths) — passes error objects directly tofail()instead of manually extracting fields into interpolated strings.rooms typing keystroke— passesroom.error(AblyErrorInfo extends Error) directly tofail()instead of stringifying it.Command fixes (missing status checks):
channels occupancy get— returned all-zero metrics on API error instead of failing. AddedstatusCode !== 200guard.spaces occupancy get— same issue, returned all-zero metrics silently. AddedstatusCode !== 200guard.push batch-publish— reported all notifications as published on API error. AddedstatusCode !== 200guard.What we didn't change
Validation messages that are intentionally plain strings (e.g., "Queue name cannot be empty", "Metadata must be a JSON object") — these don't have ErrorInfo to preserve.
Before/after
Failed to list spaces: 404No application found with id aaaaaa+Ably error code: 40400{"error":"Failed to list spaces: 404"}{"error":"No application found with id aaaaaa","code":40400,"statusCode":404}Failed to list channels: 401Batch publish failed: Invalid message format (40000)Invalid message format+Ably error code: 40000+ hint lookupchannels occupancy getwith invalid keypush batch-publishwith invalid keyTest plan
CommandError.fromHttpResponse()— extraction, fallback, zero errorCode, non-zero errorCodeCommandError.from()— message+code, message+code+statusCode, message only, object without messagerest.request()command) verifyingerrorCodeanderrorMessageare surfacedpnpm cliagainst real Ably API — confirmed error message, code, and status code all surface correctly in both human and JSON modes