Merge pull request #249 from ably/jsonFixes

Clean up JSON output: logging helpers, stderr routing, and lifecycle signals

Author: umair-ably

.claude/skills/ably-codebase-review/SKILL.md

@@ -94,14 +94,14 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
 
 ### Agent 3: Output Formatting Sweep
 
-**Goal:** Verify all human output uses the correct format helpers and is JSON-guarded.
+**Goal:** Verify all human output uses the correct format helpers and correct output method (stderr for status, stdout for data).
 
 **Method (grep/read — text patterns):**
 1. **Grep** for `chalk\.cyan\(` in command files — should use `formatResource()` instead
 2. **Grep** for `formatProgress(` and check matches for manual `...` appended
 3. **Grep** for `formatSuccess(` and read the lines to check they end with `.`
-4. **Grep** for `shouldOutputJson` to find all JSON-aware commands
-5. **Read** command files and look for unguarded `this.log()` calls (not inside `if (!this.shouldOutputJson(flags))`)
+4. **Grep** for `this\.log(formatProgress\|this\.log(formatSuccess\|this\.log(formatListening\|this\.log(formatWarning` and `this\.logToStderr(formatProgress\|this\.logToStderr(formatSuccess\|this\.logToStderr(formatListening\|this\.logToStderr(formatWarning` — these must use the base command helpers instead: `this.logProgress(msg, flags)`, `this.logSuccessMessage(msg, flags)`, `this.logListening(msg, flags)`, `this.logHolding(msg, flags)`, `this.logWarning(msg, flags)`. In non-JSON mode all helpers emit to stderr. In JSON mode: `logProgress` and `logSuccessMessage` are **silent** (no-ops), while `logListening` (status: "listening"), `logHolding` (status: "holding"), and `logWarning` (status: "warning") emit structured JSON on stdout. `logSuccessMessage` should be inside the `else` block after `logJsonResult`. Also check these helpers are NOT inside `shouldOutputJson` guards — they don't need them.
+5. **Grep** for `shouldOutputJson` to find all JSON-aware commands and verify data output is properly branched
 6. **Grep** for quoted resource names patterns like `"${` or `'${` near `channel`, `name`, `app` variables — should use `formatResource()`
 
 **Method (grep — structured output format):**
@@ -155,15 +155,15 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
 4. Cross-reference: every leaf command should appear in both the `logJsonResult`/`logJsonEvent` list and the `shouldOutputJson` list
 5. **Read** streaming commands to verify they use `logJsonEvent`, one-shot commands use `logJsonResult`
 6. **Read** each `logJsonResult`/`logJsonEvent` call and verify data is nested under a domain key — singular for events/single items (e.g., `{message: ...}`, `{cursor: ...}`), plural for collections (e.g., `{cursors: [...]}`, `{rules: [...]}`). Top-level envelope fields are `type`, `command`, `success` only. Metadata like `total`, `timestamp`, `appId` may sit alongside the domain key.
-7. **Check** hold commands (set, enter, acquire) emit `logJsonStatus("holding", ...)` after `logJsonResult` — this signals to JSON consumers that the command is alive and waiting for Ctrl+C / `--duration`
+7. **Check** hold commands (set, enter, acquire) emit `this.logHolding(...)` after `logJsonResult` — this emits `status: "holding"` in JSON mode, signaling to consumers that the command is alive and waiting for Ctrl+C / `--duration`. For passive subscribe commands, check for `this.logListening(...)` instead (emits `status: "listening"`)
 
 **Reasoning guidance:**
 - Commands that ONLY have human output (no JSON path) are deviations
 - Direct `formatJsonRecord` usage in command files should use `logJsonResult`/`logJsonEvent` instead
 - Topic index commands (showing help) don't need JSON output
 - Data spread at the top level without a domain key is a deviation — nest under a singular or plural domain noun
 - Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) alongside the domain key are acceptable — they describe the result, not the domain objects
-- Hold commands missing `logJsonStatus` after `logJsonResult` are deviations — JSON consumers need the hold signal
+- Hold commands missing `logHolding` after `logJsonResult` are deviations — JSON consumers need the hold signal
 
 ### Agent 6: Test Pattern Sweep
 

.claude/skills/ably-new-command/SKILL.md

@@ -210,19 +210,15 @@ export default class TopicAction extends AblyBaseCommand {
 
 ### Output patterns
 
-The CLI has specific output helpers in `src/utils/output.ts`. All human-readable output must be wrapped in a JSON guard:
+The base command provides five logging helpers: `this.logProgress(msg, flags)`, `this.logSuccessMessage(msg, flags)`, `this.logListening(msg, flags)`, `this.logHolding(msg, flags)`, `this.logWarning(msg, flags)`. These do NOT need `shouldOutputJson` guards. In non-JSON mode they all emit formatted text on stderr. In JSON mode: `logProgress` and `logSuccessMessage` are **silent** (no-ops — the JSON result/event records already convey progress and success), while `logListening` (status: "listening"), `logHolding` (status: "holding"), and `logWarning` (status: "warning") emit structured JSON on stdout. `logSuccessMessage` should be placed inside the `else` block after `logJsonResult` for readability. Only human-readable **data output** (field labels, headings, record blocks) needs the `if/else` guard:
 
 ```typescript
-// JSON guard — all human output goes through this
-if (!this.shouldOutputJson(flags)) {
-  this.log(formatProgress("Attaching to channel: " + formatResource(channelName)));
-}
+// Progress/success/listening — no guard needed, helpers handle both modes
+this.logProgress("Attaching to channel: " + formatResource(channelName), flags);
 
 // After success:
-if (!this.shouldOutputJson(flags)) {
-  this.log(formatSuccess("Subscribed to channel: " + formatResource(channelName) + "."));
-  this.log(formatListening("Listening for messages."));
-}
+this.logSuccessMessage("Subscribed to channel: " + formatResource(channelName) + ".", flags);
+this.logListening("Listening for messages.", flags);
 
 // JSON output — nest data under a domain key, not spread at top level.
 // Envelope provides top-level: type, command, success.
@@ -276,7 +272,11 @@ Rules:
 - `formatHeading(text)` — bold, for record headings in lists
 - `formatIndex(n)` — dim bracketed number, for history ordering
 - Use `this.fail()` for all errors (see Error handling below), never `this.log(chalk.red(...))`
-- Never use `console.log` or `console.error` — always `this.log()` or `this.logToStderr()`
+- Never use `console.log` or `console.error` — always `this.log()` for data or the logging helpers for status messages
+- Use `this.logProgress(msg, flags)`, `this.logSuccessMessage(msg, flags)`, `this.logListening(msg, flags)`, `this.logHolding(msg, flags)`, `this.logWarning(msg, flags)` for status messages — no `shouldOutputJson` guard needed. In non-JSON mode all emit to stderr. In JSON mode: `logProgress` and `logSuccessMessage` are silent; `logListening` (status: "listening"), `logHolding` (status: "holding"), and `logWarning` (status: "warning") emit structured JSON on stdout
+- Do NOT use the old pattern `this.logToStderr(formatProgress/Success/Listening/Warning(...))` — use the helpers instead
+- `formatPaginationLog()` output still uses `this.logToStderr()` directly (not a helper yet)
+- `formatLabel()`, `formatHeading()`, `formatIndex()`, `formatTimestamp()`, `formatClientId()`, `formatEventType()` → `this.log()` inside the non-JSON branch
 
 ### JSON envelope — reserved keys
 
@@ -454,9 +454,10 @@ See the "Keeping Skills Up to Date" section in `CLAUDE.md` for the full list of
 - [ ] Correct base class (`AblyBaseCommand`, `ControlBaseCommand`, `ChatBaseCommand`, `SpacesBaseCommand`, or `StatsBaseCommand`)
 - [ ] Correct flag set (`productApiFlags` vs `ControlBaseCommand.globalFlags`)
 - [ ] `clientIdFlag` only if command needs client identity
-- [ ] All human output wrapped in `if (!this.shouldOutputJson(flags))`
-- [ ] Output helpers used correctly (`formatProgress`, `formatSuccess`, `formatWarning`, `formatListening`, `formatResource`, `formatTimestamp`, `formatClientId`, `formatEventType`, `formatLabel`, `formatHeading`, `formatIndex`)
-- [ ] `formatSuccess()` messages end with `.` (period)
+- [ ] Human data output wrapped in `if (!this.shouldOutputJson(flags))` — but `logProgress`, `logSuccessMessage`, `logListening`, `logWarning` helpers do NOT need guards
+- [ ] Status messages use base command helpers (`this.logProgress`, `this.logSuccessMessage`, `this.logListening`, `this.logWarning`) — NOT `this.logToStderr(formatProgress/Success/Listening/Warning(...))`
+- [ ] Output formatters used correctly for data display (`formatResource`, `formatTimestamp`, `formatClientId`, `formatEventType`, `formatLabel`, `formatHeading`, `formatIndex`)
+- [ ] `logSuccessMessage()` messages end with `.` (period)
 - [ ] Non-JSON data output uses multi-line labeled blocks (see `patterns.md` "Human-Readable Output Format"), not tables or custom grids
 - [ ] Non-JSON output exposes all available SDK fields (same data as JSON mode, omitting only null/empty values)
 - [ ] SDK types imported directly (`import type { CursorUpdate } from "@ably/spaces"`) — no local interface redefinitions of SDK types (display interfaces in `src/utils/` are fine)

.claude/skills/ably-new-command/references/patterns.md

@@ -44,15 +44,11 @@ async run(): Promise<void> {
   // Returns a cleanup function, but cleanup is handled automatically by base command.
   this.setupChannelStateLogging(channel, flags);
 
-  if (!this.shouldOutputJson(flags)) {
-    this.log(formatProgress("Attaching to channel: " + formatResource(args.channel)));
-  }
+  this.logProgress("Attaching to channel: " + formatResource(args.channel), flags);
 
   channel.once("attached", () => {
-    if (!this.shouldOutputJson(flags)) {
-      this.log(formatSuccess("Attached to channel: " + formatResource(args.channel) + "."));
-      this.log(formatListening("Listening for events."));
-    }
+    this.logSuccessMessage("Attached to channel: " + formatResource(args.channel) + ".", flags);
+    this.logListening("Listening for events.", flags);
   });
 
   let sequenceCounter = 0;
@@ -115,9 +111,7 @@ async run(): Promise<void> {
 
   const channel = rest.channels.get(args.channel);
 
-  if (!this.shouldOutputJson(flags)) {
-    this.log(formatProgress("Publishing to channel: " + formatResource(args.channel)));
-  }
+  this.logProgress("Publishing to channel: " + formatResource(args.channel), flags);
 
   try {
     const message: Partial<Ably.Message> = {
@@ -131,9 +125,9 @@ async run(): Promise<void> {
       // Nest data under a domain key. Don't use "success" as a data key
       // for batch summaries — it overrides the envelope's success field. Use "allSucceeded".
       this.logJsonResult({ message: { channel: args.channel, name: message.name, data: message.data } }, flags);
-    } else {
-      this.log(formatSuccess("Message published to channel: " + formatResource(args.channel) + "."));
     }
+
+    this.logSuccessMessage("Message published to channel: " + formatResource(args.channel) + ".", flags);
   } catch (error) {
     this.fail(error, flags, "publish", { channel: args.channel });
   }
@@ -179,17 +173,15 @@ async run(): Promise<void> {
     // NO room.attach() — update/delete/annotate are REST calls
     const room = await chatClient.rooms.get(args.room);
 
-    if (!this.shouldOutputJson(flags)) {
-      this.log(formatProgress("Updating message " + formatResource(args.serial) + " in room " + formatResource(args.room)));
-    }
+    this.logProgress("Updating message " + formatResource(args.serial) + " in room " + formatResource(args.room), flags);
 
     const result = await room.messages.update(args.serial, updateParams, details);
 
     if (this.shouldOutputJson(flags)) {
       this.logJsonResult({ room: args.room, serial: args.serial, versionSerial: result.version.serial }, flags);
-    } else {
-      this.log(formatSuccess(`Message ${formatResource(args.serial)} updated in room ${formatResource(args.room)}.`));
     }
+
+    this.logSuccessMessage(`Message ${formatResource(args.serial)} updated in room ${formatResource(args.room)}.`, flags);
   } catch (error) {
     this.fail(error, flags, "roomMessageUpdate", { room: args.room, serial: args.serial });
   }
@@ -227,15 +219,18 @@ async run(): Promise<void> {
     const { items: messages, hasMore, pagesConsumed } = await collectPaginatedResults(history, flags.limit);
 
     const paginationWarning = formatPaginationLog(pagesConsumed, messages.length);
-    if (paginationWarning && !this.shouldOutputJson(flags)) {
-      this.log(paginationWarning);
+    if (paginationWarning) {
+      this.logToStderr(paginationWarning);
     }
 
     if (this.shouldOutputJson(flags)) {
       // Plural domain key for collections, optional metadata alongside
       this.logJsonResult({ messages, hasMore, total: messages.length }, flags);
-    } else {
-      this.log(formatSuccess(`Found ${messages.length} messages.`));
+    }
+
+    this.logSuccessMessage(`Found ${messages.length} messages.`, flags);
+
+    if (!this.shouldOutputJson(flags)) {
       // Display each message using multi-line labeled blocks
 
       if (hasMore) {
@@ -328,9 +323,7 @@ async run(): Promise<void> {
     }
   }
 
-  if (!this.shouldOutputJson(flags)) {
-    this.log(formatProgress("Entering presence on channel: " + formatResource(args.channel)));
-  }
+  this.logProgress("Entering presence on channel: " + formatResource(args.channel), flags);
 
   // Optionally subscribe to other members' events before entering
   if (flags["show-others"]) {
@@ -342,10 +335,8 @@ async run(): Promise<void> {
 
   await channel.presence.enter(presenceData);
 
-  if (!this.shouldOutputJson(flags)) {
-    this.log(formatSuccess("Entered presence on channel: " + formatResource(args.channel) + "."));
-    this.log(formatListening("Present on channel."));
-  }
+  this.logSuccessMessage("Entered presence on channel: " + formatResource(args.channel) + ".", flags);
+  this.logListening("Present on channel.", flags);
 
   await this.waitAndTrackCleanup(flags, "presence", flags.duration);
 }
@@ -434,8 +425,8 @@ async run(): Promise<void> {
     const { items, hasMore, pagesConsumed } = await collectPaginatedResults(firstPage, flags.limit);
 
     const paginationWarning = formatPaginationLog(pagesConsumed, items.length);
-    if (paginationWarning && !this.shouldOutputJson(flags)) {
-      this.log(paginationWarning);
+    if (paginationWarning) {
+      this.logToStderr(paginationWarning);
     }
 
     if (this.shouldOutputJson(flags)) {
@@ -486,8 +477,11 @@ async run(): Promise<void> {
     if (this.shouldOutputJson(flags)) {
       // Singular domain key for single-item results
       this.logJsonResult({ resource: result }, flags);
-    } else {
-      this.log(formatSuccess("Resource created: " + formatResource(result.id) + "."));
+    }
+
+    this.logSuccessMessage("Resource created: " + formatResource(result.id) + ".", flags);
+
+    if (!this.shouldOutputJson(flags)) {
       // Display additional fields using formatLabel
       this.log(`${formatLabel("Status")} ${result.status}`);
       this.log(`${formatLabel("Created")} ${new Date(result.createdAt).toISOString()}`);
@@ -669,7 +663,7 @@ Commands must behave strictly according to their documented purpose — no unint
 
 **Set / enter / acquire commands** — hold state until Ctrl+C / `--duration`:
 - Enter space (manual: `enterSpace: false` + `space.enter()` + `markAsEntered()`), perform operation, output confirmation, then hold with `waitAndTrackCleanup`
-- Emit `formatListening("Holding <thing>.")` (human) and `logJsonStatus("holding", ...)` (JSON)
+- Emit `this.logHolding("Holding <thing>. Press Ctrl+C to exit.", flags)` — in non-JSON mode this shows dim text on stderr; in JSON mode it emits `status: "holding"`
 - **NOT subscribe** to other events — that is what subscribe commands are for
 
 **Side-effect rules:**
@@ -706,8 +700,7 @@ await this.space!.enter();
 this.markAsEntered();
 await this.space!.locations.set(location);
 // output result...
-this.log(formatListening("Holding location."));
-this.logJsonStatus("holding", "Holding location. Press Ctrl+C to exit.", flags);
+this.logHolding("Holding location. Press Ctrl+C to exit.", flags);
 await this.waitAndTrackCleanup(flags, "location", flags.duration);
 ```
 
@@ -752,22 +745,21 @@ this.logJsonResult({ channels: items, total, hasMore }, flags);       // channel
 
 Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) may sit alongside the collection key since they describe the result, not the domain objects.
 
-### Hold status for long-running commands (logJsonStatus)
+### Hold status for long-running commands (logHolding)
 
-Long-running commands that hold state (e.g. `spaces members enter`, `spaces locations set`, `spaces locks acquire`, `spaces cursors set`) must emit a status line after the result so JSON consumers know the command is alive and waiting:
+Long-running commands that hold state (e.g. `spaces members enter`, `spaces locations set`, `spaces locks acquire`, `spaces cursors set`) must call `this.logHolding(...)` after the result so JSON consumers know the command is alive and waiting. For passive subscribe/stream commands, use `this.logListening(...)` instead.
 
 ```typescript
 // After the result output:
 if (this.shouldOutputJson(flags)) {
   this.logJsonResult({ member: formatMemberOutput(self!) }, flags);
 } else {
-  this.log(formatSuccess(`Entered space: ${formatResource(spaceName)}.`));
+  this.logSuccessMessage(`Entered space: ${formatResource(spaceName)}.`, flags);
   // ... labels ...
-  this.log(formatListening("Holding presence."));
 }
 
-// logJsonStatus has built-in shouldOutputJson guard — no outer if needed
-this.logJsonStatus("holding", "Holding presence. Press Ctrl+C to exit.", flags);
+// logHolding: in non-JSON mode emits dim text on stderr; in JSON mode emits status: "holding"
+this.logHolding("Holding presence. Press Ctrl+C to exit.", flags);
 
 await this.waitAndTrackCleanup(flags, "member", flags.duration);
 ```
@@ -778,6 +770,16 @@ This emits two NDJSON lines in `--json` mode:
 {"type":"status","command":"spaces:members:enter","status":"holding","message":"Holding presence. Press Ctrl+C to exit."}
 ```
 
+**Behavior matrix for logging helpers:**
+
+| Helper | Non-JSON | JSON |
+|--------|----------|------|
+| `logProgress` | stderr | silent (no-op) |
+| `logSuccessMessage` | stderr | silent (no-op) |
+| `logWarning` | stderr | stdout (status: "warning") |
+| `logListening` | stderr | stdout (status: "listening") |
+| `logHolding` | stderr | stdout (status: "holding") |
+
 ### Choosing the domain key name
 
 | Scenario | Key | Example |