Address CodeRabbit review feedback for push commands

- Guard deprecation warning in set-apns-p12 for JSON mode
- Use formatWarning() and formatClientId() in push channels list
- Validate --data flag parses to a JSON object in push devices save
- Validate all JSON flags (--recipient, --payload, --data, --apns, --fcm, --web) parse to objects in push publish
- Let E2E test setup failures propagate instead of silently skipping

Author: umair-ably

src/chat-base-command.ts

@@ -3,7 +3,6 @@ import { ChatClient, Room, RoomStatus } from "@ably/chat";
 import { AblyBaseCommand } from "./base-command.js";
 import { productApiFlags } from "./flags.js";
 import { BaseFlags } from "./types/cli.js";
-import chalk from "chalk";
 
 import {
   formatSuccess,

src/commands/apps/set-apns-p12.ts

@@ -8,6 +8,7 @@ import {
   formatProgress,
   formatResource,
   formatSuccess,
+  formatWarning,
 } from "../../utils/output.js";
 
 export default class AppsSetApnsP12Command extends ControlBaseCommand {
@@ -49,9 +50,13 @@ export default class AppsSetApnsP12Command extends ControlBaseCommand {
   async run(): Promise<void> {
     const { args, flags } = await this.parse(AppsSetApnsP12Command);
 
-    this.warn(
-      'This command is deprecated. Use "ably push config set-apns" instead.',
-    );
+    if (!this.shouldOutputJson(flags)) {
+      this.logToStderr(
+        formatWarning(
+          'This command is deprecated. Use "ably push config set-apns" instead.',
+        ),
+      );
+    }
 
     // Display authentication information
     this.showAuthInfoIfNeeded(flags);

src/commands/push/channels/list.ts

@@ -4,13 +4,15 @@ import { AblyBaseCommand } from "../../../base-command.js";
 import { productApiFlags } from "../../../flags.js";
 import { BaseFlags } from "../../../types/cli.js";
 import {
+  formatClientId,
   formatCountLabel,
   formatHeading,
   formatLabel,
   formatLimitWarning,
   formatProgress,
   formatResource,
   formatSuccess,
+  formatWarning,
 } from "../../../utils/output.js";
 
 export default class PushChannelsList extends AblyBaseCommand {
@@ -71,7 +73,7 @@ export default class PushChannelsList extends AblyBaseCommand {
       }
 
       if (subscriptions.length === 0) {
-        this.log("No subscriptions found.");
+        this.log(formatWarning("No subscriptions found."));
         return;
       }
 
@@ -90,7 +92,9 @@ export default class PushChannelsList extends AblyBaseCommand {
         if (sub.deviceId)
           this.log(`  ${formatLabel("Device ID")} ${sub.deviceId}`);
         if (sub.clientId)
-          this.log(`  ${formatLabel("Client ID")} ${sub.clientId}`);
+          this.log(
+            `  ${formatLabel("Client ID")} ${formatClientId(sub.clientId)}`,
+          );
         this.log("");
       }
 

src/commands/push/devices/save.ts

@@ -153,7 +153,9 @@ export default class PushDevicesSave extends AblyBaseCommand {
           }
           recipient = {
             transportType,
-            deviceToken: flags["device-token"],
+            ...(transportType === "fcm"
+              ? { registrationToken: flags["device-token"] }
+              : { deviceToken: flags["device-token"] }),
           };
         }
 
@@ -225,14 +227,25 @@ export default class PushDevicesSave extends AblyBaseCommand {
       jsonString = fs.readFileSync(filePath, "utf8");
     }
 
+    let parsed: unknown;
     try {
-      return JSON.parse(jsonString) as Record<string, unknown>;
+      parsed = JSON.parse(jsonString);
     } catch {
       this.fail(
         "--data must be valid JSON or a path to a JSON file (prefixed with @)",
         flags as BaseFlags,
         "pushDeviceSave",
       );
     }
+
+    if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+      this.fail(
+        "--data must be a JSON object",
+        flags as BaseFlags,
+        "pushDeviceSave",
+      );
+    }
+
+    return parsed as Record<string, unknown>;
   }
 }

src/commands/push/publish.ts

@@ -93,10 +93,14 @@ export default class PushPublish extends AblyBaseCommand {
         recipient = { clientId: flags["client-id"] };
       } else {
         try {
-          recipient = JSON.parse(flags.recipient!) as Record<string, unknown>;
+          const parsed = JSON.parse(flags.recipient!);
+          if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+            throw new Error("not an object");
+          }
+          recipient = parsed as Record<string, unknown>;
         } catch {
           this.fail(
-            "--recipient must be valid JSON",
+            "--recipient must be a valid JSON object",
             flags as BaseFlags,
             "pushPublish",
           );
@@ -107,10 +111,14 @@ export default class PushPublish extends AblyBaseCommand {
       let payload: Record<string, unknown>;
       if (flags.payload) {
         try {
-          payload = JSON.parse(flags.payload) as Record<string, unknown>;
+          const parsed = JSON.parse(flags.payload);
+          if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+            throw new Error("not an object");
+          }
+          payload = parsed as Record<string, unknown>;
         } catch {
           this.fail(
-            "--payload must be valid JSON",
+            "--payload must be a valid JSON object",
             flags as BaseFlags,
             "pushPublish",
           );
@@ -133,46 +141,79 @@ export default class PushPublish extends AblyBaseCommand {
 
         if (flags.data) {
           try {
-            payload.data = JSON.parse(flags.data);
+            const parsed = JSON.parse(flags.data);
+            if (
+              !parsed ||
+              typeof parsed !== "object" ||
+              Array.isArray(parsed)
+            ) {
+              throw new Error("not an object");
+            }
+            payload.data = parsed;
           } catch {
             this.fail(
-              "--data must be valid JSON",
+              "--data must be a valid JSON object",
               flags as BaseFlags,
               "pushPublish",
             );
           }
         }
       }
 
+      // Validate that at least some payload content exists
+      if (
+        !payload!.notification &&
+        !payload!.data &&
+        Object.keys(payload!).length === 0
+      ) {
+        this.fail(
+          "No push payload provided. Use --payload, --title/--body, or --data to specify notification content",
+          flags as BaseFlags,
+          "pushPublish",
+        );
+      }
+
       // Add platform-specific overrides
       if (flags.apns) {
         try {
-          payload.apns = JSON.parse(flags.apns);
+          const parsed = JSON.parse(flags.apns);
+          if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+            throw new Error("not an object");
+          }
+          payload.apns = parsed;
         } catch {
           this.fail(
-            "--apns must be valid JSON",
+            "--apns must be a valid JSON object",
             flags as BaseFlags,
             "pushPublish",
           );
         }
       }
       if (flags.fcm) {
         try {
-          payload.fcm = JSON.parse(flags.fcm);
+          const parsed = JSON.parse(flags.fcm);
+          if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+            throw new Error("not an object");
+          }
+          payload.fcm = parsed;
         } catch {
           this.fail(
-            "--fcm must be valid JSON",
+            "--fcm must be a valid JSON object",
             flags as BaseFlags,
             "pushPublish",
           );
         }
       }
       if (flags.web) {
         try {
-          payload.web = JSON.parse(flags.web);
+          const parsed = JSON.parse(flags.web);
+          if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
+            throw new Error("not an object");
+          }
+          payload.web = parsed;
         } catch {
           this.fail(
-            "--web must be valid JSON",
+            "--web must be a valid JSON object",
             flags as BaseFlags,
             "pushPublish",
           );

test/e2e/connections/connections.test.ts

@@ -152,7 +152,7 @@ describe("Connections E2E Tests", () => {
     );
   });
 
-  describe("Live Connection Monitoring E2E", () => {
+  describe.skipIf(!process.env.E2E_ABLY_API_KEY)("Live Connection Monitoring E2E", () => {
     it(
       "should monitor live connections with real client lifecycle",
       { timeout: 180000 },
@@ -168,10 +168,7 @@ describe("Connections E2E Tests", () => {
         // Step 1: Start live connection log monitoring
         const monitorEnv = { ...process.env };
         delete monitorEnv.ABLY_CLI_TEST_MODE;
-        const apiKey = process.env.E2E_ABLY_API_KEY;
-        if (!apiKey) {
-          throw new Error("E2E_ABLY_API_KEY environment variable is required");
-        }
+        const apiKey = process.env.E2E_ABLY_API_KEY!;
 
         // Use connection-lifecycle command which uses the correct meta channel
         const connectionsMonitor = spawn(

test/e2e/push/push-config-e2e.test.ts

@@ -41,23 +41,19 @@ describe("Push Config E2E Tests", () => {
       logErrors: false,
     });
 
-    try {
-      // Create a dedicated test app for push config tests
-      const appName = `E2E Push Config Test ${Date.now()}`;
-      const createResult = await runCommand(
-        ["apps", "create", "--name", appName, "--json"],
-        {
-          env: { ABLY_ACCESS_TOKEN: accessToken },
-        },
-      );
+    // Create a dedicated test app for push config tests
+    // Let setup failures propagate — only missing credentials should skip
+    const appName = `E2E Push Config Test ${Date.now()}`;
+    const createResult = await runCommand(
+      ["apps", "create", "--name", appName, "--json"],
+      {
+        env: { ABLY_ACCESS_TOKEN: accessToken },
+      },
+    );
 
-      const result = JSON.parse(createResult.stdout);
-      testAppId = result.app.id;
-      console.log(`Created test app for push config: ${testAppId}`);
-    } catch (error) {
-      console.error("Failed to create test app:", error);
-      shouldSkip = true;
-    }
+    const result = JSON.parse(createResult.stdout);
+    testAppId = result.app.id;
+    console.log(`Created test app for push config: ${testAppId}`);
   });
 
   afterAll(async () => {

test/fixtures/push/test-fcm-service-account.json

@@ -2,7 +2,7 @@
   "type": "service_account",
   "project_id": "test-project-id",
   "private_key_id": "test-key-id",
-  "private_key": "-----BEGIN RSA PRIVATE KEY-----\nFAKE+KEY+FOR+TESTING\n-----END RSA PRIVATE KEY-----\n",
+  "private_key": "fake-key-for-testing",
   "client_email": "test@test-project-id.iam.gserviceaccount.com",
   "client_id": "123456789",
   "auth_uri": "https://accounts.google.com/o/oauth2/auth",

test/unit/commands/push/devices/save.test.ts

@@ -71,7 +71,7 @@ describe("push:devices:save command", () => {
         [
           "push:devices:save",
           "--data",
-          '{"id":"device-2","platform":"android","formFactor":"tablet","push":{"recipient":{"transportType":"fcm","deviceToken":"tok"}}}',
+          '{"id":"device-2","platform":"android","formFactor":"tablet","push":{"recipient":{"transportType":"fcm","registrationToken":"tok"}}}',
         ],
         import.meta.url,
       );