Skip to content

Nest ErrorInfo fields under error key in JSON output#244

Merged
umair-ably merged 4 commits intomainfrom
addErrorEnvelope
Mar 31, 2026
Merged

Nest ErrorInfo fields under error key in JSON output#244
umair-ably merged 4 commits intomainfrom
addErrorEnvelope

Conversation

@umair-ably
Copy link
Copy Markdown
Collaborator

Summary

In JSON mode, error fields (message, code, statusCode) were flattened at the top level of the envelope alongside type, command, and success. 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 single error key.

This PR nests ErrorInfo fields under an error object 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() — nests message, code, statusCode under error key; context fields spread at top level
  • extractErrorInfo() (new) — extracts structured ErrorInfo (message, code?, statusCode?) from any error value via duck-typing, used by per-item errors in batch operations
  • Per-item errors in channels publish, rooms messages send, and connections test now emit structured error objects instead of plain strings, preserving Ably error codes when present
  • fail() hint added inside the error object rather than at the top level
  • 12 new tests covering extractErrorInfo, JSON serialization (no undefined values leak), and full envelope structure verification

Test plan

  • pnpm prepare succeeds
  • pnpm exec eslint . — 0 errors
  • pnpm test:unit — 2209 tests pass (12 new)
  • Verified no undefined values appear in serialized JSON output
  • Verified context fields stay at top level, ErrorInfo fields nest under error

🤖 Generated with Claude Code

@umair-ably umair-ably requested a review from sacOO7 March 31, 2026 13:49
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Mar 31, 2026 4:51pm

Request Review

Copy link
Copy Markdown

@claude-code-ably-assistant claude-code-ably-assistant bot left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@claude-code-ably-assistant claude-code-ably-assistant bot left a comment

Choose a reason for hiding this comment

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

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.js ENOENT)
  • 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
  • hint correctly moves inside error — it is error metadata, not top-level context
  • Test coverage is thorough and the new extractErrorInfo describe block is well-structured

@sacOO7
Copy link
Copy Markdown
Contributor

sacOO7 commented Mar 31, 2026

Ran local check, minor missing usage found

accounts/current.ts uses flat string error in JSON result

// CURRENT (inconsistent)
this.logJsonResult(
  {
    account: {
      error: errorMessage(error),  // plain string
    },
    mode: "web-cli",
  },
  flags,
);

Fix: Import and use extractErrorInfo:

// 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,
);

@umair-ably
Copy link
Copy Markdown
Collaborator Author

Ran local check, minor missing usage found

accounts/current.ts uses flat string error in JSON result

// CURRENT (inconsistent)
this.logJsonResult(
  {
    account: {
      error: errorMessage(error),  // plain string
    },
    mode: "web-cli",
  },
  flags,
);

Fix: Import and use extractErrorInfo:

// 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,
);

this is a non-fatal "partial result with an error note," not a fail() error envelope... so not what this PR is addressing

Copy link
Copy Markdown
Contributor

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing feedback
LGTM

@sacOO7
Copy link
Copy Markdown
Contributor

sacOO7 commented Mar 31, 2026

You can also check if we need to update CLAUDE.md convention for this change

@umair-ably umair-ably merged commit d402f51 into main Mar 31, 2026
10 checks passed
@umair-ably umair-ably deleted the addErrorEnvelope branch March 31, 2026 17:02
* 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): {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we not concretely define ErrorInfo here (or even just re-export it from ably-js)?

serial?: string;
success: boolean;
error?: string;
error?: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto re definining ErrorInfo as a type (even if it's just re-exporting ably-js)

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants