Nest ErrorInfo fields under error key in JSON output#244
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Overall this is a clean, well-tested breaking change with a clear rationale. The implementation is consistent and thorough — all call sites updated, 12 new tests, good coverage of edge cases.
One correctness concern worth fixing
In src/errors/command-error.ts, toJsonData() spreads this.context after setting the error object:
return {
error: errorObj,
...this.context, // if context has an 'error' key, errorObj is silently replaced
};If any call site ever passes context with an error key, the structured errorObj is silently replaced with no type error. No current call site does this, but context is a free-form Record<string, unknown>. Fix by spreading context first so error always wins:
return {
...this.context,
error: errorObj,
};Minor observation (not blocking)
The cast (jsonData.error as Record<string, unknown>).hint = friendlyHint in base-command.ts:1591 is safe because toJsonData() always returns error as an object, but TypeScript cannot verify it — the cast is load-bearing.
Everything else looks good
extractErrorInfo()correctly handles the Ably ErrorInfo duck-type pattern and excludes string-typed codes (e.g. Node.jsENOENT)- Human-readable output paths in all three commands still use
errorMessage(error)as a string — no regression - Context fields (
appId,channel,queueName,errorCode,helpUrl) correctly remain at the top level of the envelope hintcorrectly moves insideerror— it is error metadata, not top-level context- Test coverage is thorough and the new
extractErrorInfodescribe block is well-structured
|
Ran local check, minor missing usage found
// CURRENT (inconsistent)
this.logJsonResult(
{
account: {
error: errorMessage(error), // plain string
},
mode: "web-cli",
},
flags,
);Fix: Import and use // At top of file, change import:
import { errorMessage, extractErrorInfo } from "../../utils/errors.js";
// At line 230:
this.logJsonResult(
{
account: {
error: extractErrorInfo(error), // now an object: { message, code?, statusCode? }
},
mode: "web-cli",
},
flags,
); |
60a3cb8 to
e3e79a2
Compare
this is a non-fatal "partial result with an error note," not a fail() error envelope... so not what this PR is addressing |
e3e79a2 to
b4880ef
Compare
sacOO7
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback
LGTM
|
You can also check if we need to update |
| * Returns an object matching the Ably ErrorInfo shape: { message, code?, statusCode? }. | ||
| * Suitable for embedding in JSON output as an `error` field. | ||
| */ | ||
| export function extractErrorInfo(error: unknown): { |
There was a problem hiding this comment.
Should we not concretely define ErrorInfo here (or even just re-export it from ably-js)?
| serial?: string; | ||
| success: boolean; | ||
| error?: string; | ||
| error?: { |
There was a problem hiding this comment.
Ditto re definining ErrorInfo as a type (even if it's just re-exporting ably-js)
Summary
In JSON mode, error fields (
message,code,statusCode) were flattened at the top level of the envelope alongsidetype,command, andsuccess. This was inconsistent with how errors are represented across the Ably ecosystem — the SDKs, the REST API, and ErrorInfo all nest error details under a singleerrorkey.This PR nests ErrorInfo fields under an
errorobject so the CLI is consistent:Before:
{"type":"error","command":"channels:publish","success":false,"error":"Unauthorized","code":40160,"statusCode":401,"hint":"Run ably auth keys list...","appId":"abc"}After:
{"type":"error","command":"channels:publish","success":false,"error":{"message":"Unauthorized","code":40160,"statusCode":401,"hint":"Run ably auth keys list..."},"appId":"abc"}Context fields (e.g.
appId,availableAccounts,channel) remain at the top level — they're metadata about the error situation, not part of the error itself.Changes
CommandError.toJsonData()— nestsmessage,code,statusCodeundererrorkey; context fields spread at top levelextractErrorInfo()(new) — extracts structured ErrorInfo (message,code?,statusCode?) from any error value via duck-typing, used by per-item errors in batch operationschannels publish,rooms messages send, andconnections testnow emit structurederrorobjects instead of plain strings, preserving Ably error codes when presentfail()hint added inside theerrorobject rather than at the top levelextractErrorInfo, JSON serialization (no undefined values leak), and full envelope structure verificationTest plan
pnpm preparesucceedspnpm exec eslint .— 0 errorspnpm test:unit— 2209 tests pass (12 new)undefinedvalues appear in serialized JSON outputerror🤖 Generated with Claude Code