[DX-793] [DX-938] Standardize JSON output with envelope format, fail() error handling, and format helpers#157
[DX-793] [DX-938] Standardize JSON output with envelope format, fail() error handling, and format helpers#157umair-ably wants to merge 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR refactors JSON output handling to use envelope-based formatting via Changes
Sequence Diagram(s)The conditions for generating sequence diagrams are not met. The changes primarily involve refactoring JSON output formatting across many command files rather than introducing new feature interactions or significantly modifying control flow between 3+ distinct components. Most changes are localized to individual command implementations and follow a consistent pattern (formatting function replacement). Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR systematically refactors JSON output across 80+ files with similar but not identical patterns. While most changes follow the same transformation (replacing Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes Ably CLI’s machine-readable output by making --json emit compact NDJSON-friendly records and introducing a consistent JSON envelope (type, command, success?) across commands, events, logs, and errors.
Changes:
- Add a typed JSON envelope builder (
buildJsonRecord) and a command-level wrapper (formatJsonRecord) so--jsonis compact and--pretty-jsonremains pretty/colorized. - Migrate many commands from ad-hoc
formatJsonOutput()payloads toformatJsonRecord()and rename conflicting payload fields (e.g.,type→eventType/logType). - Add NDJSON parsing helpers for tests and update/extend unit tests to assert non-empty JSON output and envelope shape.
Reviewed changes
Copilot reviewed 115 out of 115 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/utils/output.test.ts | Adds unit tests for buildJsonRecord envelope rules. |
| test/unit/services/stats-display.test.ts | Updates stats display JSON test to assert envelope fields. |
| test/unit/helpers/ndjson.test.ts | Adds tests for NDJSON/log-line parsing helpers. |
| test/unit/commands/spaces/locks/subscribe.test.ts | Adds JSON envelope assertions for initial locks snapshot output. |
| test/unit/commands/spaces/locks/get.test.ts | Adds JSON envelope assertions for lock get output. |
| test/unit/commands/spaces/locks/get-all.test.ts | Adds JSON envelope assertions for lock list output. |
| test/unit/commands/spaces/locations/subscribe.test.ts | Adds JSON envelope assertions for initial locations snapshot output. |
| test/unit/commands/spaces/locations/get-all.test.ts | Adds JSON envelope assertions for location list output. |
| test/unit/commands/spaces/cursors/subscribe.test.ts | Adds JSON envelope assertions for cursor update events. |
| test/unit/commands/spaces/cursors/set.test.ts | Switches JSON assertions to parse structured output. |
| test/unit/commands/spaces/cursors/get-all.test.ts | Adds JSON envelope assertions for cursor list output. |
| test/unit/commands/rooms/typing/subscribe.test.ts | Uses parseLogLines to assert event envelopes in captured logs. |
| test/unit/commands/rooms/reactions/subscribe.test.ts | Uses parseLogLines to assert event envelopes in captured logs. |
| test/unit/commands/rooms/presence/subscribe.test.ts | Uses parseLogLines; asserts eventType and envelope fields. |
| test/unit/commands/rooms/presence/enter.test.ts | Uses parseLogLines; asserts eventType and envelope fields. |
| test/unit/commands/rooms/occupancy/subscribe.test.ts | Uses parseLogLines; asserts envelope + eventType for snapshots. |
| test/unit/commands/rooms/messages/reactions/subscribe.test.ts | Uses parseLogLines; asserts event envelopes for reaction summaries. |
| test/unit/commands/rooms/messages.test.ts | Adds/updates tests to assert result/event envelopes for --json flows. |
| test/unit/commands/queues/list.test.ts | Updates error-path assertions to parse and validate error envelope. |
| test/unit/commands/logs/subscribe.test.ts | Adds JSON envelope assertions for log event streaming. |
| test/unit/commands/logs/push/subscribe.test.ts | Adds JSON envelope assertions for push log events. |
| test/unit/commands/logs/connection-lifecycle/subscribe.test.ts | Adds JSON envelope assertions for connection lifecycle log events. |
| test/unit/commands/logs/channel-lifecycle/subscribe.test.ts | Adds JSON envelope assertions for channel lifecycle log events. |
| test/unit/commands/channels/subscribe.test.ts | Adds JSON envelope assertions for channel message events. |
| test/unit/commands/channels/occupancy/subscribe.test.ts | Adds JSON envelope assertions for occupancy events. |
| test/unit/commands/auth/issue-jwt-token.test.ts | Updates assertions to avoid colliding with envelope type field. |
| test/unit/base/base-command-enhanced.test.ts | Adds tests for compact --json output and formatJsonRecord wrapping. |
| test/unit/base/auth-info-display.test.ts | Updates test helper to match shouldHideAccountInfo() signature change. |
| test/helpers/ndjson.ts | Introduces shared NDJSON/log parsing helpers for tests. |
| src/utils/output.ts | Adds JsonRecordType and buildJsonRecord() envelope builder. |
| src/stats-base-command.ts | Passes command ID/pretty-json to stats display; suppresses non-JSON unsubscribe text. |
| src/services/stats-display.ts | Emits enveloped JSON records for stats; supports pretty printing in JSON mode. |
| src/commands/support/ask.ts | Switches JSON output to formatJsonRecord("result", ...). |
| src/commands/spaces/members/subscribe.ts | Migrates JSON outputs to envelope; renames event payload type → eventType; standardizes jsonError usage. |
| src/commands/spaces/members/enter.ts | Migrates JSON outputs to envelope; renames event payload type → eventType; standardizes jsonError usage. |
| src/commands/spaces/locks/subscribe.ts | Migrates JSON outputs to envelope; renames event payload type → eventType; standardizes jsonError usage. |
| src/commands/spaces/locks/get.ts | Migrates JSON outputs to envelope for lock get paths. |
| src/commands/spaces/locks/get-all.ts | Migrates JSON outputs to envelope for lock list paths. |
| src/commands/spaces/locks/acquire.ts | Migrates JSON outputs to envelope for acquire + error paths. |
| src/commands/spaces/locations/subscribe.ts | Migrates JSON outputs to envelope; renames event payload type → eventType; standardizes jsonError usage. |
| src/commands/spaces/locations/set.ts | Migrates JSON outputs to envelope for set + event paths; standardizes jsonError usage. |
| src/commands/spaces/locations/get-all.ts | Migrates JSON outputs to envelope for list paths; standardizes jsonError usage. |
| src/commands/spaces/list.ts | Migrates JSON outputs to envelope for list paths. |
| src/commands/spaces/cursors/subscribe.ts | Migrates JSON outputs to envelope; renames event payload type → eventType; standardizes jsonError usage. |
| src/commands/spaces/cursors/set.ts | Migrates JSON outputs to envelope; standardizes jsonError usage. |
| src/commands/spaces/cursors/get-all.ts | Migrates JSON outputs to envelope for connection + list paths. |
| src/commands/rooms/typing/subscribe.ts | Migrates JSON event output to formatJsonRecord("event", ...). |
| src/commands/rooms/reactions/subscribe.ts | Migrates JSON event output to formatJsonRecord("event", ...). |
| src/commands/rooms/reactions/send.ts | Migrates JSON result/error output to envelope. |
| src/commands/rooms/presence/subscribe.ts | Renames payload type → eventType; migrates JSON event output to envelope. |
| src/commands/rooms/presence/enter.ts | Renames payload type → eventType; migrates JSON event output to envelope. |
| src/commands/rooms/occupancy/subscribe.ts | Renames payload type → eventType; migrates JSON event output to envelope. |
| src/commands/rooms/occupancy/get.ts | Migrates JSON result output to envelope. |
| src/commands/rooms/messages/subscribe.ts | Migrates JSON event output to envelope; standardizes jsonError usage. |
| src/commands/rooms/messages/send.ts | Migrates JSON result/error output to envelope; standardizes jsonError usage. |
| src/commands/rooms/messages/reactions/subscribe.ts | Renames payload type → eventType; migrates JSON event output to envelope. |
| src/commands/rooms/messages/reactions/send.ts | Migrates JSON result/error output to envelope; removes now-unneeded result interface. |
| src/commands/rooms/messages/reactions/remove.ts | Migrates JSON result output to envelope; removes now-unneeded result interface. |
| src/commands/rooms/messages/history.ts | Migrates JSON progress/result output to envelope. |
| src/commands/rooms/list.ts | Migrates JSON list output to envelope. |
| src/commands/queues/list.ts | Migrates JSON list/error output to envelope. |
| src/commands/queues/delete.ts | Migrates JSON result/error output to envelope. |
| src/commands/queues/create.ts | Migrates JSON result output to envelope. |
| src/commands/logs/subscribe.ts | Renames payload type → logType; migrates JSON event output to envelope. |
| src/commands/logs/push/subscribe.ts | Migrates JSON event output to envelope. |
| src/commands/logs/push/history.ts | Migrates JSON result/error output to envelope. |
| src/commands/logs/history.ts | Migrates JSON result/error output to envelope. |
| src/commands/logs/connection-lifecycle/subscribe.ts | Migrates JSON event output to envelope. |
| src/commands/logs/connection-lifecycle/history.ts | Migrates JSON result/error output to envelope. |
| src/commands/logs/channel-lifecycle/subscribe.ts | Migrates JSON event output to envelope. |
| src/commands/integrations/update.ts | Migrates JSON result output to envelope; uses JSON.stringify for human-readable target printing. |
| src/commands/integrations/list.ts | Migrates JSON result output to envelope; uses JSON.stringify for human-readable target printing. |
| src/commands/integrations/get.ts | Migrates JSON result output to envelope; uses JSON.stringify for human-readable target printing. |
| src/commands/integrations/delete.ts | Migrates JSON result/error output to envelope. |
| src/commands/integrations/create.ts | Migrates JSON result output to envelope; uses JSON.stringify for human-readable target printing. |
| src/commands/connections/test.ts | Migrates JSON result output to envelope. |
| src/commands/config/show.ts | Migrates JSON output to envelope for config show paths. |
| src/commands/config/path.ts | Migrates JSON result output to envelope. |
| src/commands/channels/subscribe.ts | Migrates JSON event/error output to envelope. |
| src/commands/channels/publish.ts | Migrates JSON result/error output to envelope. |
| src/commands/channels/presence/subscribe.ts | Migrates JSON event output to envelope. |
| src/commands/channels/presence/enter.ts | Migrates JSON event/result/error output to envelope. |
| src/commands/channels/occupancy/subscribe.ts | Migrates JSON event output to envelope. |
| src/commands/channels/occupancy/get.ts | Migrates JSON result/error output to envelope. |
| src/commands/channels/list.ts | Migrates JSON result/error output to envelope. |
| src/commands/channels/history.ts | Migrates JSON result/error output to envelope. |
| src/commands/channels/batch-publish.ts | Migrates JSON result/error output to envelope; uses JSON.stringify for human-readable response printing. |
| src/commands/bench/subscriber.ts | Migrates JSON result output to envelope. |
| src/commands/bench/publisher.ts | Migrates JSON result output to envelope. |
| src/commands/auth/revoke-token.ts | Migrates JSON result/error output to envelope. |
| src/commands/auth/keys/revoke.ts | Migrates JSON result/error output to envelope. |
| src/commands/auth/keys/list.ts | Migrates JSON result/error output to envelope. |
| src/commands/auth/keys/get.ts | Migrates JSON result/error output to envelope. |
| src/commands/auth/keys/current.ts | Migrates JSON result output to envelope. |
| src/commands/auth/keys/create.ts | Migrates JSON result/error output to envelope. |
| src/commands/auth/issue-jwt-token.ts | Migrates JSON result output to envelope; renames token type field to avoid collision. |
| src/commands/auth/issue-ably-token.ts | Migrates JSON result output to envelope. |
| src/commands/apps/update.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/set-apns-p12.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/list.ts | Migrates JSON list output to envelope. |
| src/commands/apps/delete.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/current.ts | Migrates JSON result output to envelope. |
| src/commands/apps/create.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/channel-rules/update.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/channel-rules/list.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/channel-rules/delete.ts | Migrates JSON result/error output to envelope. |
| src/commands/apps/channel-rules/create.ts | Migrates JSON result/error output to envelope. |
| src/commands/accounts/switch.ts | Migrates JSON result/error output to envelope. |
| src/commands/accounts/logout.ts | Migrates JSON result/error output to envelope. |
| src/commands/accounts/login.ts | Migrates JSON result/error output to envelope. |
| src/commands/accounts/list.ts | Migrates JSON result/error output to envelope. |
| src/commands/accounts/current.ts | Migrates JSON result output to envelope in multiple modes/paths. |
| src/base-command.ts | Implements compact --json, adds formatJsonRecord, updates verbose JSON logs, and changes account info hiding signature. |
| docs/Project-Structure.md | Documents new NDJSON helper in test helpers. |
| AGENTS.md | Updates contributor guidance to require formatJsonRecord JSON envelope output. |
Comments suppressed due to low confidence (2)
src/commands/config/show.ts:35
- In JSON mode when the config file is missing, the code outputs
formatJsonRecord("result", { error: ... })and then exits with code 1. Becauseresultenvelopes default tosuccess: true, this produces a contradictory record (type: result+success: true+error: ...) for a failing command. Use anerrorenvelope instead (e.g.formatJsonRecord("error", ...)orjsonError(...)) sosuccess:falseandtype:"error"match the exit code.
src/commands/accounts/current.ts:191 - In the web-CLI error path, the JSON output uses
formatJsonRecord("result", { error: ... }). Sinceresultrecords default tosuccess: true, this yields an inconsistent shape for a failed operation (type: result+success: true+error). Consider emitting anerrorrecord (and likely exiting non-zero) so consumers can reliably treattype:"error"/success:falseas failure.
} catch (error) {
// If we can't get account details, show an error message
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonRecord(
"result",
{
error: errorMessage(error),
mode: "web-cli",
},
flags,
),
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
src/commands/spaces/cursors/set.ts (1)
136-160:⚠️ Potential issue | 🟠 MajorKeep the
--data.positionvalidation JSON-safe.The earlier parse failures now return JSON envelopes, but the invalid-position branch still goes through
this.error(). Under--json, that leaks human-readable output and breaks the NDJSON contract for malformed cursor payloads.As per coding guidelines, "Wrap all human-readable output (progress, success, listening) in `if (!this.shouldOutputJson(flags))` to prevent polluting `--json` output".🐛 Proposed fix
if ( !cursorData.position || typeof (cursorData.position as Record<string, unknown>).x !== "number" || typeof (cursorData.position as Record<string, unknown>).y !== "number" ) { - this.error( - 'Invalid cursor position in --data. Expected format: {"position":{"x":number,"y":number}}', - ); + const errorMsg = + 'Invalid cursor position in --data. Expected format: {"position":{"x":number,"y":number}}'; + if (this.shouldOutputJson(flags)) { + this.jsonError({ error: errorMsg }, flags); + } else { + this.error(errorMsg); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/set.ts` around lines 136 - 160, The invalid cursor position branch uses this.error(...) which emits human-readable output even when --json/NDJSON is requested; update the validation handling around cursorData.position so that if (this.shouldOutputJson(flags)) you call this.jsonError({ error: 'Invalid cursor position ...' }, flags) and otherwise emit the human-readable this.error(...) (or wrap the this.error call in if (!this.shouldOutputJson(flags))). Reference cursorData, shouldOutputJson(flags), jsonError, and the existing invalid cursor position message when making the change.src/commands/spaces/locks/subscribe.ts (1)
95-117:⚠️ Potential issue | 🟠 MajorEmit an empty
resultrecord when there are no locks.In JSON mode this branch produces no output at all for a successful empty snapshot. That makes
ably spaces locks subscribe --jsonindistinguishable from a stalled command for machine consumers, especially with--duration 0.🐛 Proposed fix
- if (locks.length === 0) { - if (!this.shouldOutputJson(flags)) { - this.log( - chalk.yellow("No locks are currently active in this space."), - ); - } - } else if (this.shouldOutputJson(flags)) { + if (this.shouldOutputJson(flags)) { this.log( this.formatJsonRecord( "result", { locks: locks.map((lock) => ({ @@ flags, ), ); + } else if (locks.length === 0) { + this.log( + chalk.yellow("No locks are currently active in this space."), + ); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/subscribe.ts` around lines 95 - 117, When in JSON mode the code currently emits nothing for an empty snapshot (locks.length === 0), making `ably spaces locks subscribe --json` produce no output; change the empty-branch to also emit a JSON "result" record by calling this.formatJsonRecord("result", { locks: [], spaceName, status: "connected" }, flags) (i.e., when locks.length === 0 and this.shouldOutputJson(flags) is true), so consumers always receive a deterministically empty result; update the block around locks, this.shouldOutputJson(flags), and this.formatJsonRecord usages in subscribe.ts accordingly.src/commands/apps/set-apns-p12.ts (1)
62-66:⚠️ Potential issue | 🔴 CriticalGuard the progress log in JSON mode.
Line 62 emits human-readable progress text before the JSON envelope on Line 79, so
ably apps set-apns-p12 ... --jsonwill write non-JSON data to stdout and break machine parsing.Proposed fix
- this.log( - formatProgress( - `Uploading APNS P12 certificate for app ${formatResource(args.id)}`, - ), - ); + if (!this.shouldOutputJson(flags)) { + this.log( + formatProgress( + `Uploading APNS P12 certificate for app ${formatResource(args.id)}`, + ), + ); + }As per coding guidelines, 'Wrap all human-readable output (progress, success, listening) in
if (!this.shouldOutputJson(flags))to prevent polluting--jsonoutput'.Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/apps/set-apns-p12.ts` around lines 62 - 66, Wrap the human-readable progress and success logs so they are suppressed when JSON output is requested: guard the this.log(formatProgress(...)) call that uses formatProgress and formatResource with if (!this.shouldOutputJson(flags)) and do the same for the subsequent human-readable logs around lines that refer to the APNS upload success; use the command's shouldOutputJson(flags) check to ensure only the JSON envelope is written when --json is passed.src/commands/apps/current.ts (1)
188-205:⚠️ Potential issue | 🟠 MajorDon't emit a successful
resultfrom the error fallback.This branch only runs when
controlApi.getApp(appId)throws, but JSON mode still emits a"result"envelope. That hides the failure from machine consumers and reports success for a degraded response. Emit an"error"envelope here, or add an explicit partial-failure signal if this fallback is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/apps/current.ts` around lines 188 - 205, The JSON fallback currently emits a successful "result" envelope when controlApi.getApp(appId) fails; change this to emit an "error" envelope (or an explicit partial-failure indicator) so machine consumers see the failure. Locate the block using this.shouldOutputJson(flags) and this.formatJsonRecord(...) where appId and keyId are used, and replace the "result" type with "error" (and include an appropriate error object/message from the caught failure or a clear partial-failure description) while preserving flags formatting.src/commands/support/ask.ts (1)
59-64:⚠️ Potential issue | 🟠 MajorGuard the warning/error text when
--jsonis enabled.The
"No previous conversation found..."warning and the fallback"Failed to get a response..."message still write plain text to stdout in JSON mode. That will corrupt the NDJSON stream before/alongside the envelope record. This catch path should stay onhandleCommandError(...), and any warning should either be suppressed or emitted as a JSON log/event record instead.As per coding guidelines, "Wrap all human-readable output (progress, success, listening) in
if (!this.shouldOutputJson(flags))to prevent polluting--jsonoutput" and "Usethis.handleCommandError()in catch blocks for consistent error handling and JSON error emission".Also applies to: 75-85, 139-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/support/ask.ts` around lines 59 - 64, Guard all human-readable console output paths in ask command by wrapping direct this.log/chalk calls (e.g., the "No previous conversation found..." message and the "Failed to get a response..." fallback) in if (!this.shouldOutputJson(flags)) so they are suppressed in JSON mode; in catch blocks replace raw logging with this.handleCommandError(err, flags) to emit structured JSON errors/events instead of plain text; ensure spinner.stop() remains but any user-facing messages are either suppressed under JSON or emitted via handleCommandError so NDJSON output is not corrupted.src/commands/auth/issue-jwt-token.ts (1)
74-83:⚠️ Potential issue | 🟠 MajorKeep validation failures on the same JSON error path.
--jsonnow returns a"result"record here, but the bad API key / bad capability JSON branches and the catch block still exit viathis.error(...). Those cases won't emit an"error"envelope, so the output format changes between success and failure.As per coding guidelines, "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission".Also applies to: 125-141, 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth/issue-jwt-token.ts` around lines 74 - 83, Replace direct calls to this.error(...) in validation branches with calls to this.handleCommandError(...) so failures emit the consistent JSON error envelope; specifically, when keyId/keySecret are missing (the branch using this.error("Invalid API key format...")) and when capability JSON parsing fails (the catch that currently does this.error(`Invalid capability JSON: ${errorMessage(error)}`)), construct an Error with the same message (or wrap the original error) and pass it to this.handleCommandError. Apply the same change to the other validation locations noted (around the blocks handling flags.capability and the API key checks at the ranges corresponding to lines 125-141 and 155-156) so all validation and parse failures use this.handleCommandError instead of this.error.src/commands/accounts/current.ts (1)
179-191:⚠️ Potential issue | 🟠 MajorError response should use
"error"type orjsonError(), not"result".The error handling in web CLI mode wraps the error in a
"result"envelope, but according to the PR's envelope semantics, errors should use the"error"type (which includessuccess: false). Using"result"for error responses breaks the contract that allows consumers to reliably distinguish success from failure by checking thetypefield.🐛 Proposed fix
} catch (error) { // If we can't get account details, show an error message if (this.shouldOutputJson(flags)) { - this.log( - this.formatJsonRecord( - "result", - { - error: errorMessage(error), - mode: "web-cli", - }, - flags, - ), - ); + this.jsonError( + { + error: errorMessage(error), + mode: "web-cli", + }, + flags, + ); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/accounts/current.ts` around lines 179 - 191, The error handling in the catch block currently emits an envelope with type "result" via formatJsonRecord, which violates the envelope semantics for failures; update the catch to emit an error envelope instead (either call the jsonError() helper with the error message and mode "web-cli", or call formatJsonRecord with type "error" and include success: false and the error details) so that shouldOutputJson(flags) produces a proper error response; locate the catch in the command's execution flow where shouldOutputJson, formatJsonRecord, and errorMessage(error) are used and replace the "result"-typed response with jsonError(...) or an "error" envelope.src/commands/spaces/members/subscribe.ts (1)
83-100:⚠️ Potential issue | 🟠 MajorEmit the initial
resulteven when the space is empty.In JSON mode,
members.length === 0currently logs nothing, so consumers never get the readiness snapshot for an empty space. Please always emit the initialresultenvelope withmembers: [].Suggested fix
- if (members.length === 0) { - if (!this.shouldOutputJson(flags)) { - this.log( - chalk.yellow("No members are currently present in this space."), - ); - } - } else if (this.shouldOutputJson(flags)) { + if (this.shouldOutputJson(flags)) { this.log( this.formatJsonRecord( "result", { members: initialMembers, spaceName, status: "connected", }, flags, ), ); + } else if (members.length === 0) { + this.log( + chalk.yellow("No members are currently present in this space."), + ); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/members/subscribe.ts` around lines 83 - 100, The code currently skips emitting the initial JSON "result" when members.length === 0, so JSON consumers never receive the readiness snapshot; update the conditional in the subscribe flow to always call this.formatJsonRecord(...) and this.log(...) with { members: initialMembers, spaceName, status: "connected" } when this.shouldOutputJson(flags) is true (even inside the members.length === 0 branch) — locate the block around the members.length check and the methods shouldOutputJson, formatJsonRecord, and variables initialMembers/spaceName and ensure the JSON emission happens unconditionally for JSON mode while preserving the existing non-JSON yellow log for empty spaces.src/commands/integrations/update.ts (1)
44-48:⚠️ Potential issue | 🟠 Major
--statusis accepted but never sent toupdateRule().
statusis declared and shown in the examples, butupdatePayloadexplicitly excludes it and never copiesflags.status. As written,ably integrations update ... --status disabledsilently leaves the rule status unchanged.Suggested fix
- const updatePayload: Partial<Omit<PartialRuleData, "status">> = { + const updatePayload: Partial<PartialRuleData> = { + ...(flags.status && { + status: flags.status as PartialRuleData["status"], + }), ...(flags["request-mode"] && { requestMode: flags["request-mode"] }), ...(flags.source && { source: JSON.parse(flags.source) }), ...(flags.target && {Also applies to: 80-90, 105-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/integrations/update.ts` around lines 44 - 48, The flags.status value is parsed but never forwarded to the update call; modify the code that builds updatePayload (and the branch near where updateRule(ruleId, updatePayload) is called) to include status when provided — e.g., if (flags.status) updatePayload.status = flags.status — so that the status string ("enabled"/"disabled") is sent to updateRule; ensure you add this conditional assignment in the same function that creates updatePayload and before calling updateRule.src/commands/spaces/cursors/get-all.ts (1)
80-91:⚠️ Potential issue | 🟠 MajorDon't emit two successful
resultrecords for oneget-allinvocation.Lines 80-91 emit a
resultas soon as the connection is up, and Lines 236-252 emit anotherresultwith the actual cursor payload. For a one-shot command, that makes the stream ambiguous because consumers can treat the first record as the final answer. Make the connection notice alog/eventrecord or suppress it in JSON mode.Also applies to: 236-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/get-all.ts` around lines 80 - 91, The code currently emits a JSON "result" record when the realtime connection is established (using this.formatJsonRecord with "result" around the this.realtimeClient!.connection.id / spaceName payload) and then emits another "result" later with the actual cursor payload; change the first emission so it does not produce a final "result" in JSON mode — either emit it as a non-final "log" or "event" JSON record (e.g. use "log" instead of "result") or skip emitting anything when this.shouldOutputJson(flags) is true, ensuring only the cursor payload (the later this.formatJsonRecord call) uses "result"; apply the same adjustment to the analogous emission at the later block (lines ~236-252) so the command emits exactly one "result" for the final payload.src/commands/accounts/login.ts (1)
87-113:⚠️ Potential issue | 🟠 Major
--jsonstill falls back to interactive stdout output.If
tokenis omitted, this path still opens a browser and writes prompt/fallback text to stdout. That breaks the NDJSON contract and makesably accounts login --jsonunsafe for machine consumption. Please either require a token in JSON mode or move every prompt/browser fallback to stderr and skip browser launch. As per coding guidelines "Wrap all human-readable output (progress, success, listening) inif (!this.shouldOutputJson(flags))to prevent polluting--jsonoutput".Also applies to: 401-409, 472-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/accounts/login.ts` around lines 87 - 113, The current interactive fallback in accounts login leaks human-readable output and opens a browser even when --json is set; update the logic in the token acquisition block so that if this.shouldOutputJson(flags) is true and args.token is not provided the command fails fast with a JSON-safe error (i.e., throw or return an error instead of prompting), and ensure all calls that emit human-readable text or launch browsers (this.log messages around formatProgress and the this.openBrowser(obtainTokenPath) call and the fallback `Please visit` message) are executed only when !this.shouldOutputJson(flags); also apply the same guards to the similar interactive blocks referenced (the blocks around lines handling 401-409 and 472-483) and use promptForToken() only when interactive output is allowed.src/commands/integrations/create.ts (1)
173-175:⚠️ Potential issue | 🟡 MinorMissing JSON error handling in catch block.
The catch block at line 173-175 uses
this.error()directly without checkingshouldOutputJson(flags). This means JSON-mode users will receive a thrown error rather than a JSON error envelope, which is inconsistent with other commands in this PR.As per coding guidelines: "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission."🛠️ Proposed fix to add JSON error handling
} catch (error) { - this.error(`Error creating integration: ${errorMessage(error)}`); + if (this.shouldOutputJson(flags)) { + this.jsonError( + { + error: `Error creating integration: ${errorMessage(error)}`, + }, + flags, + ); + return; + } + this.error(`Error creating integration: ${errorMessage(error)}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/integrations/create.ts` around lines 173 - 175, The catch block currently calls this.error(`Error creating integration: ${errorMessage(error)}`) directly; replace that direct throw with a call to this.handleCommandError(error, flags) so JSON-mode consumers get a proper JSON error envelope and non-JSON behavior remains consistent—locate the try/catch in createIntegration command (the catch that references errorMessage(error)) and switch to using this.handleCommandError(error, flags) (or, if needed, pass the extracted message alongside the original error) and remove the direct this.error invocation; also ensure shouldOutputJson(flags) is not used directly here since handleCommandError handles JSON branching.
🧹 Nitpick comments (14)
src/commands/spaces/locks/acquire.ts (1)
143-153: Route lock-acquire failures throughhandleCommandError().This is still a catch block, so emitting JSON directly here bypasses the shared failure path and makes this command an exception to the standardized error handling.
As per coding guidelines, "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/acquire.ts` around lines 143 - 153, The catch block manually emits JSON and calls this.error; instead route failures through the shared handler by replacing the manual error flow with a call to this.handleCommandError(error, { flags, context: { lockId }, logEvent: { name: "lock", action: "acquireFailed", message: `Failed to acquire lock: ${errorMessage(error)}` } }); keep the existing logCliEvent usage only if handleCommandError does not already log; otherwise remove the manual this.logCliEvent/jsonError/this.error branches so all error output and JSON emission for the acquire lock code path go through handleCommandError.src/commands/queues/delete.ts (1)
112-118: Use the shared command error path here too.This catch block sidesteps the centralized handler the PR is standardizing, so delete failures can drift from the shared JSON/error/exit behavior.
As per coding guidelines, "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/queues/delete.ts` around lines 112 - 118, Replace the manual catch logic that creates errorMsg and branches on this.shouldOutputJson()/this.jsonError()/this.error() with a call to the centralized handler: call this.handleCommandError(error, flags) (passing the original error object and flags) inside the catch. Remove the errorMsg construction and the conditional json/error paths; keep references to errorMessage(), shouldOutputJson(), jsonError(), and error() only for locating the block and ensure the raw error is forwarded to handleCommandError.src/commands/spaces/locks/subscribe.ts (1)
216-223: PreferhandleCommandError()in this catch block too.This is one of the touched error paths, but it still hand-rolls JSON vs human output instead of using the centralized helper the PR is introducing. Routing through
this.handleCommandError(error, flags, "lock", { spaceName })will keep this command aligned with the new envelope/error contract.As per coding guidelines, "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/subscribe.ts` around lines 216 - 223, The catch block currently builds an errorMsg, logs via this.logCliEvent and conditionally calls this.jsonError/console output; replace that manual handling by invoking the centralized helper this.handleCommandError(error, flags, "lock", { spaceName }) so the command follows the new envelope/error contract — remove the manual this.logCliEvent and this.jsonError/shouldOutputJson branching in this catch, and ensure you pass the existing spaceName variable into the metadata argument.src/commands/config/show.ts (1)
26-34: Consider using "error" type for the missing config file case.When the config file doesn't exist, this outputs a "result" envelope containing an error field. Since this is an error condition (followed by
this.exit(1)), consider usingformatJsonRecord("error", ...)instead for consistency with the envelope type semantics wheretype: "error"indicates failure.However, if the intent is to have a uniform "result" envelope with success/error info embedded in the payload, this is acceptable.
💡 Optional: Use "error" type for error condition
if (this.shouldOutputJson(flags)) { this.log( this.formatJsonRecord( - "result", + "error", { error: "Config file does not exist", path: configPath }, flags, ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config/show.ts` around lines 26 - 34, The JSON output for the missing config file is using formatJsonRecord("result", ...) but this is an error path (it then calls this.exit(1)); change the envelope type to "error" by calling formatJsonRecord("error", { error: "Config file does not exist", path: configPath }, flags) so the output semantics match the failure; update the call near shouldOutputJson(...) / this.exit(1) and ensure any downstream consumers expect type: "error".src/commands/apps/channel-rules/list.ts (1)
108-120: Consider usinghandleCommandError()for consistent error handling.The catch block manually handles JSON vs non-JSON error output. As per coding guidelines, commands should use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission, which centralizes this logic.♻️ Proposed refactor
} catch (error) { - if (this.shouldOutputJson(flags)) { - this.jsonError( - { - appId, - error: errorMessage(error), - status: "error", - }, - flags, - ); - } else { - this.error(`Error listing channel rules: ${errorMessage(error)}`); - } + this.handleCommandError(error, flags, "channel-rules", { appId }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/apps/channel-rules/list.ts` around lines 108 - 120, The catch block in the list command manually inspects shouldOutputJson() and calls jsonError()/this.error() with errorMessage(), which duplicates centralized logic; replace that manual handling by calling this.handleCommandError(error, flags) inside the catch so JSON vs non-JSON output and formatting are handled consistently; update the catch in src/commands/apps/channel-rules/list.ts (the block currently using shouldOutputJson(), jsonError(), errorMessage(), and this.error()) to invoke this.handleCommandError(...) instead.src/commands/integrations/get.ts (1)
69-71: Consider adding JSON error handling for consistency.The catch block uses
this.error()directly, which outputs a non-JSON error message even when--jsonflag is used. This breaks the JSON stream for machine consumers. As per coding guidelines, usethis.handleCommandError()in catch blocks for consistent error handling and JSON error emission.♻️ Proposed refactor
} catch (error) { - this.error(`Error getting integration rule: ${errorMessage(error)}`); + this.handleCommandError(error, flags, "integrations", { ruleId: args.ruleId }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/integrations/get.ts` around lines 69 - 71, Replace the direct call to this.error(...) in the catch block of the get integration command with this.handleCommandError(...) so errors are emitted in the CLI's JSON-aware format; specifically, in the catch of the function in src/commands/integrations/get.ts (where it currently calls this.error(`Error getting integration rule: ${errorMessage(error)}`)), call this.handleCommandError(error, 'Error getting integration rule') (or at minimum this.handleCommandError(error)) and remove the direct this.error(...) usage so JSON consumers receive the standardized error payload.src/commands/auth/keys/list.ts (1)
118-129: Consider usinghandleCommandError()for consistent error handling.Similar to other commands in this PR, the error handling could use
handleCommandError()for consistency across the codebase. This aligns with the coding guidelines recommendation for catch blocks.As per coding guidelines: "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission."♻️ Suggested refactor
} catch (error) { - if (this.shouldOutputJson(flags)) { - this.jsonError( - { - appId, - error: errorMessage(error), - }, - flags, - ); - } else { - this.error(`Error listing keys: ${errorMessage(error)}`); - } + this.handleCommandError(error, flags, "keys", { appId }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth/keys/list.ts` around lines 118 - 129, Replace the manual catch logic that calls this.jsonError/this.error and errorMessage with a single call to the shared handler: use this.handleCommandError(error, flags, { appId }) inside the catch so the command uses the centralized error flow (handleCommandError) and preserves the appId metadata for JSON output; remove the conditional on this.shouldOutputJson and the direct calls to this.jsonError and this.error in the catch block.src/commands/integrations/delete.ts (1)
112-118: Consider usinghandleCommandError()for consistent error handling.The coding guidelines recommend using
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission. The current implementation manually checksshouldOutputJsonand callsjsonError/this.error()separately.As per coding guidelines: "Use
this.handleCommandError()in catch blocks for consistent error handling and JSON error emission."♻️ Suggested refactor
} catch (error) { - const errorMsg = `Error deleting integration: ${errorMessage(error)}`; - if (this.shouldOutputJson(flags)) { - this.jsonError({ error: errorMsg }, flags); - } else { - this.error(errorMsg); - } + this.handleCommandError(error, flags, "integration", { + integrationId: args.integrationId, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/integrations/delete.ts` around lines 112 - 118, The catch block currently builds errorMsg and manually branches on shouldOutputJson/ jsonError/ this.error; replace that manual handling by calling this.handleCommandError(error, flags) so the command uses the centralized error path; remove the errorMsg/stringification logic in the catch and pass the original error (or the error thrown) to this.handleCommandError (preserving errorMessage usage elsewhere if needed) to ensure consistent JSON emission and logging.src/commands/rooms/messages/history.ts (1)
164-170: History output should use[index] timestampformat per guidelines.The coding guidelines specify that history commands should use
${chalk.dim([${index + 1}])} ${formatTimestamp(timestamp)}ordering format for history output.As per coding guidelines for
src/commands/**/history*.{ts,tsx}: "Use[index] timestampordering format:${chalk.dim([${index + 1}])} ${formatTimestamp(timestamp)}for history output."♻️ Suggested format change
- for (const message of messagesInOrder) { + for (const [index, message] of messagesInOrder.entries()) { // Format message with timestamp, author and content const timestamp = formatMessageTimestamp(message.timestamp); const author = message.clientId || "Unknown"; this.log( - `${formatTimestamp(timestamp)} ${chalk.blue(`${author}:`)} ${message.text}`, + `${chalk.dim(`[${index + 1}]`)} ${formatTimestamp(timestamp)} ${chalk.blue(`${author}:`)} ${message.text}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/messages/history.ts` around lines 164 - 170, The history output loop currently logs `${formatTimestamp(timestamp)} ${chalk.blue(`${author}:`)} ${message.text}`; change it to include the 1-based index prefix and use the required ordering by prepending `${chalk.dim(\`[${index + 1}]\`)} ${formatTimestamp(timestamp)}` before the rest of the message. Update the loop over messagesInOrder in the same block (where formatMessageTimestamp, formatTimestamp and this.log are used) to provide an index (e.g., use a for loop with an index or entries()) and log `${chalk.dim(\`[${index + 1}]\`)} ${formatTimestamp(timestamp)} ${chalk.blue(`${author}:`)} ${message.text}` so it follows the `${chalk.dim(`[${index + 1}]`)} ${formatTimestamp(timestamp)}` guideline.src/commands/spaces/locations/get-all.ts (1)
272-285: Use the centralized error helper in both catches.The success path now follows the shared record formatter, but these catch blocks still special-case
jsonError()/this.error()locally. Routing both throughthis.handleCommandError()keeps error envelopes and exit handling aligned with the rest of this PR. As per coding guidelines "Usethis.handleCommandError()in catch blocks for consistent error handling and JSON error emission".Also applies to: 286-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locations/get-all.ts` around lines 272 - 285, The catch blocks currently call this.jsonError(...) / this.error(...) directly and use errorMessage(error); replace those calls with a single call to this.handleCommandError(error, flags) so errors go through the centralized handler (preserving JSON envelope and exit behavior). Locate the catches that reference this.shouldOutputJson, this.jsonError, this.error and errorMessage(error) (both occurrences around the get-all flow) and swap them to call this.handleCommandError(error, flags) instead, removing the special-case local JSON/error handling.test/helpers/ndjson.ts (1)
9-15: Consider adding error handling for malformed JSON lines.
parseNdjsonLineswill throw if any line contains invalid JSON, whileparseLogLinessilently skips invalid lines. This inconsistency could cause test failures that are hard to diagnose when stdout contains unexpected non-JSON content (e.g., warning messages, progress indicators).♻️ Proposed fix to add resilient parsing
export function parseNdjsonLines(stdout: string): Record<string, unknown>[] { - return stdout + const results: Record<string, unknown>[] = []; + for (const line of stdout .trim() .split("\n") - .filter(Boolean) - .map((line) => JSON.parse(line)); + .filter(Boolean)) { + try { + results.push(JSON.parse(line)); + } catch { + // skip non-JSON lines (warnings, progress messages, etc.) + } + } + return results; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/ndjson.ts` around lines 9 - 15, parseNdjsonLines currently throws on the first malformed JSON line; change it to be resilient like parseLogLines by attempting to JSON.parse each non-empty line inside a try/catch and skipping (or optionally collecting) lines that fail to parse so one bad stdout line doesn't break tests; update the implementation of parseNdjsonLines to iterate lines, try JSON.parse(line) in a try block, push successfully parsed objects to the result array and ignore parse errors (or optionally log them), preserving the function signature and behavior for valid lines.src/commands/spaces/locks/get.ts (1)
56-60: Minor semantic consideration: "Lock not found" as a result vs error.The "Lock not found" case uses
formatJsonRecord("result", { error: "Lock not found", lockId }, ...). Having anerrorfield inside a"result"envelope may be semantically confusing to consumers—they might expect errors only in"error"type envelopes.Consider using
{ lock: null, lockId }to indicate a successful lookup that found nothing, or usethis.jsonError(...)if this should be treated as an error condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locks/get.ts` around lines 56 - 60, The current "Lock not found" response uses this.formatJsonRecord("result", { error: "Lock not found", lockId }, ...), which mixes an error field into a "result" envelope; change it to return a clear "no-result" payload by using this.formatJsonRecord("result", { lock: null, lockId }, flags) to indicate a successful lookup with no lock found, or if this should be treated as an actual error, replace the call with this.jsonError("Lock not found", { lockId }) so errors go through the error envelope; update the code in the block that currently calls this.formatJsonRecord with { error: ... } accordingly.src/commands/spaces/locations/subscribe.ts (1)
183-187: Consider removing redundantstatusfield from error payloads.The
status: "error"field in thejsonErrorpayload is redundant since thejsonErrormethod already wraps the payload with atype: "error"envelope andsuccess: false. This applies to error calls at lines 185, 269, and 296.Suggested change
if (this.shouldOutputJson(flags)) { this.jsonError( - { error: errorMsg, spaceName, status: "error" }, + { error: errorMsg, spaceName }, flags, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locations/subscribe.ts` around lines 183 - 187, Remove the redundant status property from the error payloads that are passed to this.jsonError: locate the calls where this.shouldOutputJson(flags) is true and this.jsonError(...) is invoked (the call sites referencing spaceName and errorMsg in subscribe command), and drop the explicit status: "error" key from the payload objects so the envelope produced by this.jsonError (type: "error" / success: false) is relied upon instead; specifically update the jsonError calls in the subscribe handler (the invocations next to this.shouldOutputJson and any other jsonError usages in the same function) to send only the error details (e.g., { error: errorMsg, spaceName }) without the status field.src/base-command.ts (1)
1004-1013: Minor:dataspread could override base envelope fields.The
...dataspread at the end oflogEntryallows caller-provided fields to override the base fields (component,event,message,timestamp). If this is intentional to allow flexibility, consider adding a brief comment. If not, spreaddatafirst to let base fields take precedence.💡 Option to prevent override (if unintentional)
const logEntry = { + ...data, component, event, message, timestamp: new Date().toISOString(), - ...data, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/base-command.ts` around lines 1004 - 1013, The current JSON envelope creation (in the isJsonMode branch building logEntry used by this.formatJsonRecord and this.log) spreads ...data last which lets caller-provided fields override base fields (component, event, message, timestamp); change the object construction to spread ...data first (e.g., { ...data, component, event, message, timestamp }) so the base envelope fields take precedence, or if overriding is intentional add a brief comment above the logEntry construction documenting that behavior; update references around logEntry, isJsonMode, formatJsonRecord, and flags accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 01e55c6c-ddba-4ef0-91ed-d732f61c4251
📒 Files selected for processing (115)
AGENTS.mddocs/Project-Structure.mdsrc/base-command.tssrc/commands/accounts/current.tssrc/commands/accounts/list.tssrc/commands/accounts/login.tssrc/commands/accounts/logout.tssrc/commands/accounts/switch.tssrc/commands/apps/channel-rules/create.tssrc/commands/apps/channel-rules/delete.tssrc/commands/apps/channel-rules/list.tssrc/commands/apps/channel-rules/update.tssrc/commands/apps/create.tssrc/commands/apps/current.tssrc/commands/apps/delete.tssrc/commands/apps/list.tssrc/commands/apps/set-apns-p12.tssrc/commands/apps/update.tssrc/commands/auth/issue-ably-token.tssrc/commands/auth/issue-jwt-token.tssrc/commands/auth/keys/create.tssrc/commands/auth/keys/current.tssrc/commands/auth/keys/get.tssrc/commands/auth/keys/list.tssrc/commands/auth/keys/revoke.tssrc/commands/auth/revoke-token.tssrc/commands/bench/publisher.tssrc/commands/bench/subscriber.tssrc/commands/channels/batch-publish.tssrc/commands/channels/history.tssrc/commands/channels/list.tssrc/commands/channels/occupancy/get.tssrc/commands/channels/occupancy/subscribe.tssrc/commands/channels/presence/enter.tssrc/commands/channels/presence/subscribe.tssrc/commands/channels/publish.tssrc/commands/channels/subscribe.tssrc/commands/config/path.tssrc/commands/config/show.tssrc/commands/connections/test.tssrc/commands/integrations/create.tssrc/commands/integrations/delete.tssrc/commands/integrations/get.tssrc/commands/integrations/list.tssrc/commands/integrations/update.tssrc/commands/logs/channel-lifecycle/subscribe.tssrc/commands/logs/connection-lifecycle/history.tssrc/commands/logs/connection-lifecycle/subscribe.tssrc/commands/logs/history.tssrc/commands/logs/push/history.tssrc/commands/logs/push/subscribe.tssrc/commands/logs/subscribe.tssrc/commands/queues/create.tssrc/commands/queues/delete.tssrc/commands/queues/list.tssrc/commands/rooms/list.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/reactions/remove.tssrc/commands/rooms/messages/reactions/send.tssrc/commands/rooms/messages/reactions/subscribe.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/messages/subscribe.tssrc/commands/rooms/occupancy/get.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/presence/subscribe.tssrc/commands/rooms/reactions/send.tssrc/commands/rooms/reactions/subscribe.tssrc/commands/rooms/typing/subscribe.tssrc/commands/spaces/cursors/get-all.tssrc/commands/spaces/cursors/set.tssrc/commands/spaces/cursors/subscribe.tssrc/commands/spaces/list.tssrc/commands/spaces/locations/get-all.tssrc/commands/spaces/locations/set.tssrc/commands/spaces/locations/subscribe.tssrc/commands/spaces/locks/acquire.tssrc/commands/spaces/locks/get-all.tssrc/commands/spaces/locks/get.tssrc/commands/spaces/locks/subscribe.tssrc/commands/spaces/members/enter.tssrc/commands/spaces/members/subscribe.tssrc/commands/support/ask.tssrc/services/stats-display.tssrc/stats-base-command.tssrc/utils/output.tstest/helpers/ndjson.tstest/unit/base/auth-info-display.test.tstest/unit/base/base-command-enhanced.test.tstest/unit/commands/auth/issue-jwt-token.test.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/logs/channel-lifecycle/subscribe.test.tstest/unit/commands/logs/connection-lifecycle/subscribe.test.tstest/unit/commands/logs/push/subscribe.test.tstest/unit/commands/logs/subscribe.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/rooms/messages.test.tstest/unit/commands/rooms/messages/reactions/subscribe.test.tstest/unit/commands/rooms/occupancy/subscribe.test.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/unit/commands/rooms/reactions/subscribe.test.tstest/unit/commands/rooms/typing/subscribe.test.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/spaces/cursors/set.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/helpers/ndjson.test.tstest/unit/services/stats-display.test.tstest/unit/utils/output.test.ts
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
d8e1d0c to
0570e52
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
9a9c1e6 to
d5d2da8
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
e72157f to
36f4f4a
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
36f4f4a to
2a96237
Compare
umair-ably
changed the title
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
2a96237 to
e222ee6
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
e222ee6 to
b1e4f35
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
b1e4f35 to
301cd21
Compare
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
301cd21 to
b569d4f
Compare
…dd --json examples Migrate 14 placeholder/index files from extending Command to BaseTopicCommand for consistency with other topic commands. Add missing --json examples to 5 leaf commands.
umair-ably
force-pushed
the
dx-793-json-envelope
branch
from
b569d4f to
bd92ea3
Compare
umair-ably
changed the title
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
PR looks good overall, almost all of the review comments are well addressed 👍
Found very few actionable items =>
- Missing
eventTypeon reaction summary events - Spaces
Locksubscribe vs get field inconsistency
Will share review doc for the same 👍
Review Strategy
This PR has 7 commits organized for efficient review:
Add buildJsonRecord utility and CommandError classAdd JSON envelope and fail() to base classes and servicesfail(),logJsonResult,logJsonEvent)Migrate commands to JSON envelope, fail(), and format helpersAdd NDJSON test helpers and update unit testsFix e2e tests for JSON envelope formatUpdate documentation and skillsMigrate placeholder topic commands to BaseTopicCommandBaseTopicCommand, plus 22 missing--jsonexamples and 3 other small fixesRecommended order: Commits 1 → 2 → spot-check a few files in 3 → 6 → 4 → 5 → 7
Problem
The
--jsonand--pretty-jsonflags had several issues that made output difficult to consume programmatically:--jsonalways pretty-printed —formatJsonOutput()used 2-space indent even for--json, producing multi-line output incompatible with line-based streaming.logCliEvent()records interspersed with data records, with no way to filter.success: true, some didn't. Error formats varied.--jsonwas active, breakingjqpipelines.this.error()directly with inconsistent patterns. Easy to forgetreturn;after error calls, leading to code continuing after errors.Breaking Changes
If you are parsing
--jsonoutput programmatically, note these changes:Output format
--jsonis now compact single-line (was pretty-printed with 2-space indent). Each record is one line, compatible with NDJSON/JSON Lines.--pretty-jsonis unchanged.Envelope wrapper
type("result","event","error","log") andcommandfields. Existing data fields are preserved but wrapped in this envelope.Renamed fields
To avoid collision with the envelope's
typefield, several data fields have been renamed:auth:issue-jwt-tokentype: "jwt"tokenType: "jwt"spaces:cursors:subscribetype: "cursor_update"eventType: "cursor_update"spaces:members:subscribetype: "member_update"eventType: "member_update"spaces:locks:subscribetype: "lock_event"eventType: "lock_event"spaces:locations:subscribetype: "location_update"/"locations_snapshot"eventType: "location_update"/"locations_snapshot"rooms:*:subscribetype: "initialSnapshot"eventType: "initialSnapshot"type: <event_action>eventType: <event_action>Removed fields
success: true/success: falsefields have been removed from individual command data. The envelope now managessuccessautomatically (truefor"result"type,falsefor"error"type).Changes
1. Compact
--jsonoutput (NDJSON for streaming)--jsonnow produces compact single-line output, compatible with NDJSON/JSON Lines.--pretty-jsonis unchanged.--json--pretty-json2. Flat envelope with
type+commanddiscriminatorEvery JSON record includes:
type—"result","event","error", or"log"command— the oclif command ID (e.g.,"channels:subscribe")success— boolean, present only on"result"and"error"types3.
fail()error handling withCommandErrorAll commands now use
this.fail(error, flags, component)as the single error funnel. It returnsnever, so TypeScript enforces no code runs after it — eliminating the "forgottenreturn;" bug class. Errors are wrapped inCommandErrorwhich preserves Ably error codes and HTTP status codes.4. Fixed non-JSON text leaking into
--jsonmodeHuman-readable messages (e.g., stats unsubscribe) are now suppressed when JSON output is active.
5. Improved test coverage
jsonLines.length > 0before validating content.test/helpers/ndjson.tswithparseNdjsonLines(),parseLogLines(), andfilterByType()helpers.6. Migrated placeholder topic commands to BaseTopicCommand and added --json examples
18 placeholder/topic files in
rooms/,spaces/, andconfig/that extendedCommandorAblyBaseCommanddirectly have been migrated toBaseTopicCommandfor consistency with all other topic commands. Added missing--jsonexamples to 22 leaf commands.Output Examples
One-shot result (
ably channels list --json):{"type":"result","command":"channels:list","success":true,"channels":[{"name":"my-channel"}],"total":1}Streaming events (
ably channels subscribe test --json):{"type":"event","command":"channels:subscribe","channel":"test","data":"hello","timestamp":"2025-04-11T12:00:01.000Z"} {"type":"event","command":"channels:subscribe","channel":"test","data":"world","timestamp":"2025-04-11T12:00:02.000Z"}Error (
ably channels publish nonexistent hello --json):{"type":"error","command":"channels:publish","success":false,"error":"Channel not found"}Verbose log (
ably channels subscribe test --json --verbose):{"type":"log","command":"channels:subscribe","component":"subscribe","event":"channelAttached","message":"Attached to channel test"} {"type":"event","command":"channels:subscribe","channel":"test","data":"hello","timestamp":"2025-04-11T12:00:01.000Z"}Industry Precedent
--jsonbehavior--format json→ compact single-linetype,action,statusfields--format JSONfor machine-readableReferences: Docker events, CLI JSON guide, NDJSON spec
Key Methods
buildJsonRecord(type, command, data){type, command, success?, ...data}envelopethis.logJsonResult(data, flags)"result"envelope to stdoutthis.logJsonEvent(data, flags)"event"envelope to stdoutthis.fail(error, flags, component)neverCommandErrorlogCliEvent(flags, component, event, message)type: "log"envelope (verbose only)Migration Scope
buildJsonRecord()utility,CommandErrorclass,fail()method returningneverlogJsonResult/logJsonEventhelpers onAblyBaseCommand, updatedControlBaseCommand,ChatBaseCommand,SpacesBaseCommand,StatsBaseCommandfail(),logJsonResult/logJsonEvent, and format helpersCommand/AblyBaseCommandtoBaseTopicCommand--jsonexampleslogToStderrinstead ofconsole.log, includes envelope fieldstest/helpers/ndjson.tswithcaptureJsonLogs(),parseNdjsonLines(),filterByType()