Push notification support #126
Conversation
Implement Stage 1 of push notification support with full device registration management using the Ably SDK's push.admin.deviceRegistrations API. Commands added: - push devices save: Register devices (Android/FCM, iOS/APNs, Web Push) - push devices get: Retrieve device details by ID - push devices list: List devices with filters (clientId, state, limit) - push devices remove: Remove single device by ID - push devices remove-where: Bulk remove by clientId/deviceId Features: - Full support for all three platforms: Android (FCM), iOS (APNs), browser (Web Push) - Web Push requires --target-url, --p256dh-key, and --auth-secret flags - JSON output mode with --json flag - Confirmation prompts for destructive operations (--force to skip) - Token redaction in get command for security Includes 14 E2E tests hitting real Ably sandbox endpoints with fake device tokens (verified to work for testing purposes).
Implement Stage 2 of push notification support with full channel subscription management using the Ably SDK's push.admin.channelSubscriptions API. Commands added: - push channels save: Subscribe device or client to a push channel - push channels list: List subscriptions for a channel with filters - push channels list-channels: List all channels with push subscriptions - push channels remove: Remove single subscription - push channels remove-where: Bulk remove subscriptions by filter Features: - Subscribe by device ID or client ID (not both) - Filter subscriptions by deviceId or clientId - JSON output mode with --json flag - Confirmation prompts for destructive operations (--force to skip) - Helpful error messages for push-not-enabled (40100) errors Note: Full integration tests require a push-enabled channel namespace which is not available in the standard test environment. Validation tests verify CLI command structure and error handling. Part of #124
Add push notification publishing commands: - `push publish` - Publish notification to device or client - Supports --device-id or --client-id targeting - Simple flags: --title, --body, --sound, --icon, --badge - Custom data via --data, full control via --payload - Platform-specific overrides: --apns, --fcm, --web - `push batch-publish` - Publish up to 10,000 notifications - Accepts JSON array via --payload (file, inline, or stdin) - Reports success/failure counts per notification
Implements Stage 4 of push notification support with commands to manage
Apple Push Notification service (APNs) credentials:
- `push config show` - Display current push notification configuration
- `push config set-apns` - Configure APNs with P12 certificate or P8 token auth
- `push config clear-apns` - Remove APNs configuration
Key implementation details:
- P12 certificate upload uses multipart/form-data with correct endpoint
- Support for both certificate-based (.p12) and token-based (.p8) auth
- Token auth requires key-id, team-id, and bundle-id flags
- Sandbox/production environment determined by certificate type (not flag)
Also fixes:
- Control API uploadApnsP12 now uses FormData/Blob for proper multipart upload
- apps:set-apns-p12 updated to use Buffer instead of base64 string
- Updated unit tests to use correct /v1/apps/{id}/pkcs12 endpoint
Implements Stage 5 of push notification support with commands to manage
Firebase Cloud Messaging (FCM) credentials:
- `push config set-fcm` - Configure FCM using a service account JSON file
- Validates file exists and contains valid JSON
- Validates type="service_account" and project_id fields
- Displays project ID and client email on success
- `push config clear-fcm` - Remove FCM configuration
- Confirms before clearing (skip with --force)
- Detects and reports if FCM is not configured
Uses Control API PATCH /apps/{id} with fcmServiceAccount field.
Progress messages suppressed in JSON output mode.
Note: Ably API validates credentials with Google/Firebase before
persisting - test credentials will be accepted but not stored.
Push fixtures useful for real CLI testing
- Remove duplicate confirm() method from clear-apns.ts and clear-fcm.ts - Use existing interactiveHelper.confirm() from base class instead - Fix publish.ts and batch-publish.ts to exit(1) after JSON error output - Ensures consistent error handling with proper exit codes across push commands
Add comprehensive unit tests for: - push:devices:save (11 tests) - push:devices:get (7 tests) - push:devices:list (8 tests) - push:devices:remove (5 tests) - push:devices:remove-where (5 tests) Tests mock the Ably SDK and verify command behavior, flag validation, and error handling. Also adds removeWhere and listChannels mock methods to mock-ably-rest.ts helper. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive unit tests for: - push:channels:save (9 tests) - push:channels:list (8 tests) - push:channels:remove (8 tests) - push:channels:remove-where (6 tests) - push:channels:list-channels (5 tests) Tests mock the Ably SDK and verify command behavior, flag validation, and error handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive unit tests for: - push:publish (15 tests) - single notification publishing - push:batch-publish (13 tests) - batch notification publishing Tests cover: - Device and client ID targeting - Notification payload construction - Platform-specific overrides (APNs, FCM, Web) - JSON payload parsing - Validation of required flags - Error handling for API failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive unit tests for:
- push:config:set-apns - P12 certificate and P8 token-based auth (13 tests)
- push:config:clear-apns - clearing APNs configuration (7 tests)
- push:config:show - displaying push notification config status (10 tests)
Tests use nock to mock Control API endpoints (/v1/me, /v1/accounts/{id}/apps)
which are called by ControlBaseCommand before app-specific operations.
Note: set-fcm and clear-fcm tests already existed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces a comprehensive push notification management system to the Ably CLI, adding new command groups for batch publishing, device registrations, channel subscriptions, and configuration management. The API signature for uploading APNS P12 certificates changes from base64 string to Buffer, and the underlying App interface expands with new authentication and configuration fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Update APNs token-based auth to use correct field names per Control API: - apnsPrivateKey → apnsSigningKey - applePushKeyId → apnsSigningKeyId - applePushTeamId → apnsIssuerKey - applePushBundleId → apnsTopicHeader - Add apnsAuthType: "token" to request payload Update config show to use new response fields: - Use apnsAuthType to determine APNs config status and auth type - Use fcmProjectId to determine FCM config status - Use apnsUseSandboxEndpoint with fallback to legacy field Add App interface fields: apnsAuthType, apnsUseSandboxEndpoint, fcmProjectId Ref: ably/website@8d632061 (feature/legacy-dashboard-quick-wins) Note: Tests pass against mocks; E2E tests pending API deployment to staging
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/apps/set-apns-p12.ts (1)
33-37: Flag behavior inconsistency creates misleading UX.The
--use-for-sandboxflag is still accepted and displayed in output (lines 73-77), but it's no longer passed to the API (line 61-63). This creates a confusing experience:
- User provides
--use-for-sandbox- CLI shows "Environment: Sandbox"
- But the API ignores this - certificate type determines environment
This means the output could be incorrect and misleading. For example, if a user provides a production certificate with
--use-for-sandbox, the output says "Sandbox" but the API will use it for production.Options:
- Remove the flag entirely (breaking change, but clearer)
- Update output to clarify: "Certificate type determines environment (not the flag)"
- Fetch the actual environment from the API response and display that
Also note: The flag is still in the examples (line 21) which should be updated accordingly.
Also applies to: 68-78
🤖 Fix all issues with AI Agents
In @src/commands/push/channels/list-channels.ts:
- Around line 79-92: The JSON error path in listChannelsCommand (check the
this.shouldOutputJson(flags) branch that logs this.formatJsonOutput({...})) logs
the error but doesn't terminate with a non-zero exit; add a call to this.exit(1)
immediately after the this.log(...) in that branch so the process returns a
failure code when --json is used, mirroring the pattern used in push/publish.ts
and push/batch-publish.ts.
In @src/commands/push/channels/remove-where.ts:
- Around line 114-133: The JSON-path in the catch block for the remove-where
command logs the error but doesn't terminate with a non-zero exit code; after
calling this.log(this.formatJsonOutput(...)) when this.shouldOutputJson(flags)
is true, call this.exit(1) (or process.exit(1) if this.exit isn’t available) so
the command exits with a non-zero status; locate the catch block that builds
errorMessage/errorCode and uses this.shouldOutputJson(flags) and
this.formatJsonOutput to add the exit call immediately after logging the JSON
error.
In @src/commands/push/devices/list.ts:
- Around line 136-148: The JSON error path in the devices list command uses
this.log(this.formatJsonOutput(...)) but doesn't terminate with a non-zero exit,
so update the branch inside shouldOutputJson(flags) to either call
this.jsonError(...) passing the same structured payload (error: errorMessage,
success: false) or keep the log call and immediately invoke this.exit(1) after
it; locate the logic around shouldOutputJson, formatJsonOutput and the current
error handling to apply one of these two fixes so JSON-mode failures return a
non-zero exit code.
In @src/commands/push/devices/remove.ts:
- Around line 77-101: The JSON error branch in the catch of the remove device
command logs the error via this.formatJsonOutput when
this.shouldOutputJson(flags) is true but does not exit with a non-zero code,
causing inconsistent exit behavior versus the non-JSON branch that calls
this.error(); modify the catch so after logging the JSON error (using
this.log(this.formatJsonOutput(...))) you invoke this.error(...) or otherwise
call process.exit(1) to ensure the command exits with code 1 on error; locate
the catch block around the remove device handler where errorMessage/errorCode
are computed and add the exit behavior for the JSON branch while preserving the
existing 40400-specific message semantics.
In @test/e2e/push/publish-e2e.test.ts:
- Around line 73-76: The test failure handler is being registered too late; move
the setupTestFailureHandler() call from the afterEach block into a beforeEach so
it registers before each test runs. Specifically, remove
setupTestFailureHandler() from the afterEach that currently contains
setupTestFailureHandler() and await cleanupTrackedResources(), add a
beforeEach(() => setupTestFailureHandler()) (or await if it returns a promise)
and leave await cleanupTrackedResources() in afterEach to continue cleanup after
each test. Ensure you reference setupTestFailureHandler, beforeEach, and
afterEach when applying the change.
🧹 Nitpick comments (20)
src/commands/push/batch-publish.ts (1)
7-240: Batch publish logic is sound; consider tightening result typing for maintainabilityThe overall flow (payload parsing from string/file/stdin, validation, 1–10k bound checks, REST call, and JSON vs human-readable summarization) looks correct and in line with the new push batch-publish semantics.
The only improvement I’d suggest is around typing and result processing:
response.itemsandr.errorare treated asRecord<string, unknown>, which forces a lot of loose casting and property access (error.message,error.code) that can be awkward under strict TS/ESLint.- Defining a dedicated result type (e.g.
type BatchPublishResultItem = { error?: { message?: string; code?: string | number } }) and castingresponse.itemsonce would make filters and logging safer and clearer.- As a small nit, you could compute
successful/failedin a singlereducepass overresults, but that’s purely optional.Functionally this is fine; the above is a readability/typing refinement you can defer.
README.md (1)
837-3921: Push command documentation is thorough; address markdownlint fenced‑code language warningsThe new
ably pushand related sections (config/devices/channels/publish/batch-publish) are well-documented and match the command surfaces and flag semantics added in this PR.However, markdownlint is flagging a number of fenced code blocks without a language (MD040), and several of those fences are in newly added/updated sections (for example around the
ably push,ably push batch-publish, andably configcommand docs where blocks start with plaininstead ofsh or similar). This will keep lint failing.I’d recommend updating the newly introduced fences to include a language (e.g.
```sh-sessionor```bashfor CLI examples,```jsonwhere appropriate). If you’re generating this via oclif’s README tooling, it may be worth adjusting the generator config so future updates also include languages.src/commands/push/publish.ts (1)
187-206: File path detection heuristic may misidentify valid JSON.The check
!payload.trim().startsWith("{") && !payload.trim().startsWith("[")will incorrectly treat a JSON string starting with whitespace-prefixed content or other valid JSON (like a quoted string"..."or a number) as a file path.🔎 Suggested improvement
Consider checking file existence first or using a more explicit flag like
--payload-file:private parsePayload(payload: string): Record<string, unknown> { let jsonString = payload; // Check if it's a file path - if (!payload.trim().startsWith("{") && !payload.trim().startsWith("[")) { + // Try parsing as JSON first; if it fails and looks like a path, try file + try { + return JSON.parse(payload) as Record<string, unknown>; + } catch { const filePath = path.resolve(payload); - if (!fs.existsSync(filePath)) { - throw new Error(`File not found: ${filePath}`); + throw new Error(`Invalid JSON format or file not found: ${filePath}`); } - jsonString = fs.readFileSync(filePath, "utf8"); + try { + return JSON.parse(jsonString) as Record<string, unknown>; + } catch { + throw new Error("Invalid JSON format in payload file"); + } } - - try { - return JSON.parse(jsonString) as Record<string, unknown>; - } catch { - throw new Error("Invalid JSON format in --payload"); - } }test/unit/commands/push/channels/remove-where.test.ts (1)
92-112: Consider adding mutual exclusivity validation test.The validation tests cover missing
--channeland missing filter criteria. Per the pattern inpublish.tswhich enforces mutual exclusivity of--device-idand--client-id, consider adding a test to verify the command rejects both being specified simultaneously (if the command implements this validation).it("should reject when both --device-id and --client-id are provided", async () => { const { error } = await runCommand( [ "push:channels:remove-where", "--channel", "alerts", "--device-id", "device-1", "--client-id", "client-1", "--force", ], import.meta.url, ); expect(error).toBeDefined(); expect(error!.message).toMatch(/only one.*device-id.*client-id/i); });test/unit/commands/push/config/set-apns.test.ts (1)
121-128: JSON extraction approach is brittle.The regex-based JSON extraction (
stdout.match(/\{[\s\S]*\}/)) could break if stdout contains multiple JSON objects or nested structures. While the comment acknowledges mixed output, consider:
- Parsing only the last line of stdout if JSON is always printed last
- Using a more specific marker/delimiter for JSON output
- Validating that only one JSON object is matched
🔎 More robust JSON extraction approach
- // Extract JSON from output (command may log progress messages) - const jsonMatch = stdout.match(/\{[\s\S]*\}/); - expect(jsonMatch).not.toBeNull(); - const output = JSON.parse(jsonMatch![0]); + // Extract JSON from output (command may log progress messages) + // JSON output should be on the last line + const lines = stdout.trim().split('\n'); + const jsonLine = lines[lines.length - 1]; + const output = JSON.parse(jsonLine);src/commands/push/config/set-fcm.ts (1)
44-62: Type assertion unnecessary for required flag.The
flags["service-account"]is already typed asstringsince it's markedrequired: true. Theas stringcast on line 46 is redundant.🔎 Proposed fix
// Read and validate service account JSON - const serviceAccountPath = path.resolve( - flags["service-account"] as string, - ); + const serviceAccountPath = path.resolve(flags["service-account"]);test/e2e/push/publish-e2e.test.ts (1)
26-52: Test device cleanup could leave orphaned registration onbeforeAllfailure.If the device registration in
beforeAllsucceeds but subsequent code fails, the device won't be cleaned up becauseafterAllonly runs afterbeforeAllcompletes. Consider wrapping the registration in a try-catch that stores the device ID only on success.This is a minor reliability concern for E2E test cleanup. The current approach is acceptable since the fake token and unique ID minimize collision risk.
src/commands/push/devices/list.ts (1)
124-130: Clarify pagination guidance for users.The pagination hint says "Use --limit to show more" but doesn't explain that there's a hard 1000-item cap (Line 62) or how to retrieve additional pages beyond that limit. Users might expect
--limit 2000to work or need guidance on pagination tokens.Consider enhancing the message:
🔎 Suggested message improvement
if (devicesPage.hasNext()) { this.log( chalk.yellow( - `Showing first ${devices.length} devices. Use --limit to show more.`, + `Showing first ${devices.length} devices. Use --limit (max 1000) to retrieve more results. Pagination beyond 1000 items requires multiple API calls.`, ), ); }src/services/control-api.ts (1)
489-543: Refactor duplicated error handling logic.The error handling in
uploadApnsP12(lines 521-542) duplicates the logic from the privaterequestmethod (lines 570-611). This creates a maintenance burden—any future improvements to error messages or parsing would need to be applied in both places.🔎 Proposed refactor to extract common error handling
Consider extracting the error response parsing into a shared private method:
private async parseErrorResponse(response: Response): Promise<string> { const responseBody = await response.text(); let responseData: unknown = responseBody; try { responseData = JSON.parse(responseBody); } catch { /* Ignore parsing errors */ } let errorMessage = `API request failed (${response.status} ${response.statusText})`; if ( typeof responseData === "object" && responseData !== null && "message" in responseData && typeof responseData.message === "string" ) { errorMessage += `: ${responseData.message}`; } else if ( typeof responseData === "string" && responseData.length < 100 ) { errorMessage += `: ${responseData}`; } return errorMessage; }Then use it in both
uploadApnsP12and therequestmethod:if (!response.ok) { - const responseBody = await response.text(); - let responseData: unknown = responseBody; - try { - responseData = JSON.parse(responseBody); - } catch { - /* Ignore parsing errors */ - } - - let errorMessage = `API request failed (${response.status} ${response.statusText})`; - if ( - typeof responseData === "object" && - responseData !== null && - "message" in responseData && - typeof responseData.message === "string" - ) { - errorMessage += `: ${responseData.message}`; - } + const errorMessage = await this.parseErrorResponse(response); throw new Error(errorMessage); }src/commands/push/config/show.ts (1)
44-45: Consider simplifying null/undefined checks.The pattern
!== null && !== undefinedis verbose. Since bothnullandundefinedare falsy in the context of checking if configuration exists, you could simplify these checks.🔎 Suggested simplification
apns: { configured: - app.apnsAuthType !== null && app.apnsAuthType !== undefined, + app.apnsAuthType != null, authType: app.apnsAuthType || null,Or, if you want to be explicit about the configuration check:
- app.apnsAuthType !== null && app.apnsAuthType !== undefined, + Boolean(app.apnsAuthType),Note: Using
!= null(loose equality) checks for bothnullandundefined, which is one of the few acceptable uses of loose equality in TypeScript.Also applies to: 67-68, 99-100
src/commands/push/channels/list-channels.ts (1)
77-77: Type assertion could be more precise.The type assertion
(error as { code?: number }).codeworks but doesn't provide type safety. Consider using a type guard or checking the property existence first.🔎 Suggested improvement
const errorMessage = error instanceof Error ? error.message : String(error); - const errorCode = (error as { code?: number }).code; + const errorCode = + error && typeof error === 'object' && 'code' in error && typeof error.code === 'number' + ? error.code + : undefined;Or define a helper type guard:
function hasErrorCode(error: unknown): error is { code: number } { return error != null && typeof error === 'object' && 'code' in error && typeof error.code === 'number'; } const errorCode = hasErrorCode(error) ? error.code : undefined;src/commands/push/config/clear-apns.ts (1)
78-86: Consider stronger typing for the app update payload.The type cast to
Record<string, unknown>on line 85 bypasses TypeScript's type checking and could mask potential issues with the updateApp API signature.🔎 Suggested refactor to use proper typing
- await api.updateApp(appId, { - apnsCertificate: null, - apnsPrivateKey: null, - applePushKeyId: null, - applePushTeamId: null, - applePushBundleId: null, - apnsUsesSandboxCert: false, - } as Record<string, unknown>); + await api.updateApp(appId, { + apnsCertificate: null, + apnsPrivateKey: null, + applePushKeyId: null, + applePushTeamId: null, + applePushBundleId: null, + apnsUsesSandboxCert: false, + });If TypeScript complains, this indicates the
updateAppmethod's type signature may need adjustment to accept nullable fields, which would be more correct than suppressing the type check.src/commands/push/devices/save.ts (2)
164-183: Consider more robust file path detection.The file path detection on line 168 using
!data.trim().startsWith("{")could incorrectly treat a filename starting with{as inline JSON. While unlikely in practice, a more explicit check would be safer.🔎 More robust file detection approach
private parseDeviceData(data: string): Ably.DeviceDetails { - let jsonString = data; - - // Check if it's a file path - if (!data.trim().startsWith("{")) { + let jsonString: string; + + // Try to parse as JSON first; if it fails and looks like a path, try reading as file + const trimmed = data.trim(); + if (trimmed.startsWith("{") || trimmed.startsWith("[")) { + // Likely inline JSON + try { + return JSON.parse(trimmed) as Ably.DeviceDetails; + } catch { + throw new Error("Invalid JSON format in --data"); + } + } else { + // Treat as file path const filePath = path.resolve(data); if (!fs.existsSync(filePath)) { throw new Error(`File not found: ${filePath}`); } jsonString = fs.readFileSync(filePath, "utf8"); + try { + return JSON.parse(jsonString) as Ably.DeviceDetails; + } catch { + throw new Error(`Invalid JSON format in file: ${filePath}`); + } } - - try { - return JSON.parse(jsonString) as Ably.DeviceDetails; - } catch { - throw new Error("Invalid JSON format in --data"); - } }This approach tries to parse JSON first, and only treats the input as a file path if it doesn't look like JSON, reducing ambiguity.
240-246: Enhance error message with parsing details.When JSON parsing fails for metadata (line 243), the error message could be more helpful by including the underlying parse error, which would aid debugging for users.
🔎 Include parse error in message
if (flags.metadata) { try { device.metadata = JSON.parse(flags.metadata as string); - } catch { - throw new Error("Invalid JSON format in --metadata"); + } catch (error) { + const parseError = error instanceof Error ? error.message : String(error); + throw new Error(`Invalid JSON format in --metadata: ${parseError}`); } }Similarly, consider enhancing the error messages in
parseDeviceData(lines 180-182) to include parse errors.src/commands/push/channels/remove.ts (1)
116-139: Missing exit after JSON error output.In the error handler, when outputting JSON errors (lines 122-131), the code continues to the
elseblock (lines 132-138) which callsthis.error(). Sincethis.error()exits the process, this won't cause issues in practice, but the logic suggests JSON output should return early to avoid the else branch entirely.🔎 Suggested improvement for clarity
if (this.shouldOutputJson(flags)) { this.log( this.formatJsonOutput( { error: errorMessage, code: errorCode, success: false, }, flags, ), ); + return; } else { if (errorCode === 40400) { this.error(`Subscription not found`); } else { this.error(`Error removing subscription: ${errorMessage}`); } }src/commands/push/channels/save.ts (1)
100-111: Missing early return after JSON error output.Similar to the remove command, when outputting JSON errors (lines 101-110), the code continues to the
elseblock which callsthis.error(). While this works becausethis.error()exits, adding an early return after JSON output improves code clarity and prevents accidental fall-through if the error method changes.🔎 Suggested improvement for clarity
if (this.shouldOutputJson(flags)) { this.log( this.formatJsonOutput( { error: errorMessage, code: errorCode, success: false, }, flags, ), ); + return; } else { if (errorCode === 40100) {src/commands/push/channels/list.ts (2)
100-106: Consider handling subscriptions with neither deviceId nor clientId.The table output logic assumes each subscription has either
deviceIdorclientId(lines 101-105). If the API returns a subscription with neither field populated, no row would be displayed, potentially confusing users.🔎 Suggested defensive handling
// List subscriptions for (const sub of subscriptions) { if (sub.deviceId) { this.log(`${"device".padEnd(12)}${sub.deviceId}`); } else if (sub.clientId) { this.log(`${"client".padEnd(12)}${sub.clientId}`); + } else { + this.log(`${"unknown".padEnd(12)}${chalk.dim("(no ID)")}`); } }
113-124: Missing early return after JSON error output.When outputting JSON errors, add an early return to prevent fall-through to the else branch (similar pattern seen in other commands).
🔎 Suggested improvement
if (this.shouldOutputJson(flags)) { this.log( this.formatJsonOutput( { error: errorMessage, code: errorCode, success: false, }, flags, ), ); + return; } else { this.error(`Error listing subscriptions: ${errorMessage}`); }src/commands/push/devices/get.ts (2)
107-113: Token redaction could expose short tokens.The redaction logic only applies when
token.length > 8(line 110). For tokens with 8 or fewer characters, no redaction occurs and they could be fully exposed. While very short tokens are unlikely in production, consider applying redaction for all non-empty tokens or logging a warning for unexpectedly short tokens.🔎 Suggested improvement for comprehensive redaction
const recipient = device.push.recipient as Record<string, unknown>; const token = recipient.registrationToken || recipient.deviceToken; - if (typeof token === "string" && token.length > 8) { + if (typeof token === "string" && token.length > 0) { + if (token.length <= 8) { + const redacted = "***"; + this.log(` ${chalk.dim("Token:")} ${redacted} (redacted)`); + } else { const redacted = `${token.slice(0, 4)}...${token.slice(-4)}`; this.log(` ${chalk.dim("Token:")} ${redacted} (redacted)`); + } } }
129-140: Missing early return after JSON error output.Add an early return after JSON error output for consistency with error handling patterns.
🔎 Suggested improvement
if (this.shouldOutputJson(flags)) { this.log( this.formatJsonOutput( { error: errorMessage, code: errorCode, success: false, }, flags, ), ); + return; } else { if (errorCode === 40400) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
test/fixtures/push/test-apns-cert.pemis excluded by!**/*.pemtest/fixtures/push/test-apns-private-key.pemis excluded by!**/*.pem
📒 Files selected for processing (49)
README.mdsrc/commands/apps/set-apns-p12.tssrc/commands/push/batch-publish.tssrc/commands/push/channels/index.tssrc/commands/push/channels/list-channels.tssrc/commands/push/channels/list.tssrc/commands/push/channels/remove-where.tssrc/commands/push/channels/remove.tssrc/commands/push/channels/save.tssrc/commands/push/config/clear-apns.tssrc/commands/push/config/clear-fcm.tssrc/commands/push/config/index.tssrc/commands/push/config/set-apns.tssrc/commands/push/config/set-fcm.tssrc/commands/push/config/show.tssrc/commands/push/devices/get.tssrc/commands/push/devices/index.tssrc/commands/push/devices/list.tssrc/commands/push/devices/remove-where.tssrc/commands/push/devices/remove.tssrc/commands/push/devices/save.tssrc/commands/push/index.tssrc/commands/push/publish.tssrc/services/control-api.tstest/e2e/push/channels-e2e.test.tstest/e2e/push/devices-e2e.test.tstest/e2e/push/publish-e2e.test.tstest/fixtures/push/README.mdtest/fixtures/push/test-apns-key.p8test/fixtures/push/test-fcm-service-account.jsontest/helpers/mock-ably-rest.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/push/batch-publish.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/channels/list.test.tstest/unit/commands/push/channels/remove-where.test.tstest/unit/commands/push/channels/remove.test.tstest/unit/commands/push/channels/save.test.tstest/unit/commands/push/config/clear-apns.test.tstest/unit/commands/push/config/clear-fcm.test.tstest/unit/commands/push/config/set-apns.test.tstest/unit/commands/push/config/set-fcm.test.tstest/unit/commands/push/config/show.test.tstest/unit/commands/push/devices/get.test.tstest/unit/commands/push/devices/list.test.tstest/unit/commands/push/devices/remove-where.test.tstest/unit/commands/push/devices/remove.test.tstest/unit/commands/push/devices/save.test.tstest/unit/commands/push/publish.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
**/*.test.ts: When testing, mock the Ably SDK properly by stubbing the constructor (e.g., sinon.stub(Ably, 'Rest').returns(mockClient)).
Always ensure resources are properly closed/cleaned up in tests (e.g., call await client.close() and sinon.restore() in afterEach).
Files:
test/unit/commands/push/devices/get.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/push/channels/save.test.tstest/unit/commands/push/channels/remove-where.test.tstest/unit/commands/push/devices/list.test.tstest/unit/commands/push/config/set-fcm.test.tstest/unit/commands/push/config/set-apns.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/config/show.test.tstest/unit/commands/push/devices/remove.test.tstest/unit/commands/push/devices/save.test.tstest/unit/commands/push/channels/list.test.tstest/e2e/push/devices-e2e.test.tstest/e2e/push/publish-e2e.test.tstest/unit/commands/push/config/clear-fcm.test.tstest/unit/commands/push/batch-publish.test.tstest/unit/commands/push/publish.test.tstest/unit/commands/push/devices/remove-where.test.tstest/e2e/push/channels-e2e.test.tstest/unit/commands/push/channels/remove.test.tstest/unit/commands/push/config/clear-apns.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
test/unit/commands/push/devices/get.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/push/channels/save.test.tssrc/commands/push/channels/remove-where.tstest/unit/commands/push/channels/remove-where.test.tstest/unit/commands/push/devices/list.test.tssrc/commands/push/config/clear-apns.tssrc/commands/push/config/index.tstest/unit/commands/push/config/set-fcm.test.tstest/unit/commands/push/config/set-apns.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/config/show.test.tstest/unit/commands/push/devices/remove.test.tstest/unit/commands/push/devices/save.test.tssrc/commands/push/batch-publish.tstest/unit/commands/push/channels/list.test.tssrc/commands/push/channels/save.tssrc/commands/push/channels/list.tssrc/commands/push/channels/index.tstest/e2e/push/devices-e2e.test.tstest/e2e/push/publish-e2e.test.tssrc/commands/push/devices/get.tssrc/commands/push/config/set-fcm.tssrc/commands/push/devices/index.tssrc/commands/push/devices/remove.tssrc/commands/push/devices/list.tssrc/commands/push/devices/remove-where.tssrc/commands/push/channels/list-channels.tstest/unit/commands/push/config/clear-fcm.test.tstest/unit/commands/push/batch-publish.test.tstest/unit/commands/push/publish.test.tssrc/commands/push/config/set-apns.tssrc/commands/push/config/show.tssrc/commands/push/devices/save.tstest/unit/commands/push/devices/remove-where.test.tssrc/commands/push/config/clear-fcm.tssrc/commands/push/publish.tstest/helpers/mock-ably-rest.tssrc/commands/push/index.tstest/e2e/push/channels-e2e.test.tstest/unit/commands/push/channels/remove.test.tssrc/services/control-api.tssrc/commands/push/channels/remove.tssrc/commands/apps/set-apns-p12.tstest/unit/commands/push/config/clear-apns.test.ts
{test/**/*.{ts,js},**/*.{test,spec}.{ts,js}}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
When new features are added or changes made, tests must be updated or added, and it is your responsibility to ensure the tests pass before deeming your work complete.
Files:
test/unit/commands/push/devices/get.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/push/channels/save.test.tstest/unit/commands/push/channels/remove-where.test.tstest/unit/commands/push/devices/list.test.tstest/unit/commands/push/config/set-fcm.test.tstest/unit/commands/push/config/set-apns.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/config/show.test.tstest/unit/commands/push/devices/remove.test.tstest/unit/commands/push/devices/save.test.tstest/unit/commands/push/channels/list.test.tstest/e2e/push/devices-e2e.test.tstest/e2e/push/publish-e2e.test.tstest/unit/commands/push/config/clear-fcm.test.tstest/unit/commands/push/batch-publish.test.tstest/unit/commands/push/publish.test.tstest/unit/commands/push/devices/remove-where.test.tstest/helpers/mock-ably-rest.tstest/e2e/push/channels-e2e.test.tstest/unit/commands/push/channels/remove.test.tstest/unit/commands/push/config/clear-apns.test.ts
src/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
src/commands/**/*.ts: Commands should follow the oclif structure: export default class MyCommand extends Command { ... }
Command should support auth via the standard auth helper (import { getAuth } from '../../helpers/auth') and use it in the run method.
Use try/catch for error handling in commands, and provide user-friendly error messages (e.g., handle error.code === 40100 for authentication failures).
Do not use direct HTTP requests (e.g., fetch) for data plane APIs; use the Ably SDK instead.
Do not use hardcoded endpoint URLs; use configuration values (e.g., flags.controlHost, config.controlHost) when constructing URLs.
Do not use non-standard command structures (e.g., export default class { async execute(args) { ... } }); always use the oclif Command class.
Files:
src/commands/push/channels/remove-where.tssrc/commands/push/config/clear-apns.tssrc/commands/push/config/index.tssrc/commands/push/batch-publish.tssrc/commands/push/channels/save.tssrc/commands/push/channels/list.tssrc/commands/push/channels/index.tssrc/commands/push/devices/get.tssrc/commands/push/config/set-fcm.tssrc/commands/push/devices/index.tssrc/commands/push/devices/remove.tssrc/commands/push/devices/list.tssrc/commands/push/devices/remove-where.tssrc/commands/push/channels/list-channels.tssrc/commands/push/config/set-apns.tssrc/commands/push/config/show.tssrc/commands/push/devices/save.tssrc/commands/push/config/clear-fcm.tssrc/commands/push/publish.tssrc/commands/push/index.tssrc/commands/push/channels/remove.tssrc/commands/apps/set-apns-p12.ts
src/commands/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Follow oclif framework best practices as described in the oclif documentation.
Files:
src/commands/push/channels/remove-where.tssrc/commands/push/config/clear-apns.tssrc/commands/push/config/index.tssrc/commands/push/batch-publish.tssrc/commands/push/channels/save.tssrc/commands/push/channels/list.tssrc/commands/push/channels/index.tssrc/commands/push/devices/get.tssrc/commands/push/config/set-fcm.tssrc/commands/push/devices/index.tssrc/commands/push/devices/remove.tssrc/commands/push/devices/list.tssrc/commands/push/devices/remove-where.tssrc/commands/push/channels/list-channels.tssrc/commands/push/config/set-apns.tssrc/commands/push/config/show.tssrc/commands/push/devices/save.tssrc/commands/push/config/clear-fcm.tssrc/commands/push/publish.tssrc/commands/push/index.tssrc/commands/push/channels/remove.tssrc/commands/apps/set-apns-p12.ts
{README.md,docs/**,.cursor/rules/**}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Update all relevant documentation, including README.md, files within the docs/ directory, and files within the .cursor/rules/ directory if the changes impact requirements, structure, testing procedures, development guidelines, Ably usage, or project structure.
Files:
README.md
🧬 Code graph analysis (17)
test/unit/commands/push/devices/get.test.ts (4)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-ably-rest.ts (1)
getMockAblyRest(247-252)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/channels/save.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/channels/remove-where.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/devices/list.test.ts (4)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-ably-rest.ts (1)
getMockAblyRest(247-252)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/channels/list-channels.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/config/show.test.ts (4)
test/helpers/mock-config-manager.ts (1)
getMockConfigManager(476-483)test/root-hooks.ts (1)
afterEach(13-61)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/devices/remove.test.ts (4)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-ably-rest.ts (1)
getMockAblyRest(247-252)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/devices/save.test.ts (4)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-ably-rest.ts (1)
getMockAblyRest(247-252)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/channels/list.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/e2e/push/devices-e2e.test.ts (3)
test/helpers/e2e-test-helper.ts (7)
SHOULD_SKIP_E2E(18-19)forceExit(637-639)createAblyClient(68-97)resetTestTracking(800-803)setupTestFailureHandler(791-795)cleanupTrackedResources(645-669)E2E_API_KEY(17-17)test/root-hooks.ts (3)
afterAll(63-69)beforeEach(8-11)afterEach(13-61)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/e2e/push/publish-e2e.test.ts (3)
test/helpers/e2e-test-helper.ts (7)
SHOULD_SKIP_E2E(18-19)forceExit(637-639)createAblyClient(68-97)resetTestTracking(800-803)setupTestFailureHandler(791-795)cleanupTrackedResources(645-669)E2E_API_KEY(17-17)test/root-hooks.ts (3)
afterAll(63-69)beforeEach(8-11)afterEach(13-61)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/push/config/clear-fcm.test.ts (4)
test/helpers/mock-config-manager.ts (1)
getMockConfigManager(476-483)test/root-hooks.ts (1)
afterEach(13-61)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/batch-publish.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/publish.test.ts (3)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/push/devices/remove-where.test.ts (4)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-ably-rest.ts (1)
getMockAblyRest(247-252)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
src/services/control-api.ts (1)
src/utils/version.ts (1)
getCliVersion(13-15)
test/unit/commands/push/config/clear-apns.test.ts (4)
test/helpers/mock-config-manager.ts (1)
getMockConfigManager(476-483)test/root-hooks.ts (1)
afterEach(13-61)test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
🪛 Checkov (3.2.334)
test/fixtures/push/test-fcm-service-account.json
[medium] 5-6: Private Key
(CKV_SECRET_13)
🪛 Gitleaks (8.30.0)
test/unit/commands/push/config/set-apns.test.ts
[high] 51-51: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 markdownlint-cli2 (0.18.1)
README.md
841-841: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2391-2391: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2428-2428: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2465-2465: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2749-2749: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3431-3431: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3461-3461: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3497-3497: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3520-3520: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3562-3562: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3598-3598: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3638-3638: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3676-3676: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3712-3712: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3737-3737: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3774-3774: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3811-3811: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3855-3855: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3891-3891: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3927-3927: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3954-3954: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3992-3992: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4037-4037: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4078-4078: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4114-4114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4173-4173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4228-4228: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4254-4254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4295-4295: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4335-4335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4371-4371: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4400-4400: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4441-4441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4464-4464: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4521-4521: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4542-4542: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4587-4587: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4728-4728: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: auth-tests
- GitHub Check: ui-tests
- GitHub Check: session-tests
- GitHub Check: e2e-cli
- GitHub Check: Cursor Bugbot
- GitHub Check: test
| if (this.shouldOutputJson(flags)) { | ||
| this.log( | ||
| this.formatJsonOutput( | ||
| { | ||
| error: errorMessage, | ||
| code: errorCode, | ||
| success: false, | ||
| }, | ||
| flags, | ||
| ), | ||
| ); | ||
| } else { | ||
| this.error(`Error listing channels: ${errorMessage}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if this pattern is consistent across push commands
rg -n 'shouldOutputJson' --type=ts -g 'src/commands/push/**/*.ts' -A10 -B2 | head -100Repository: ably/ably-cli
Length of output: 170
🏁 Script executed:
# Also look at the specific file mentioned for comparison
cat -n src/commands/push/devices/list.ts | grep -A15 -B5 "shouldOutputJson"Repository: ably/ably-cli
Length of output: 1546
🏁 Script executed:
# Search for how this.error() is used in the codebase to understand the pattern
rg -n 'this\.error\(' --type=ts -g 'src/commands/**/*.ts' -B3 | head -80Repository: ably/ably-cli
Length of output: 4561
🏁 Script executed:
# Search for error handling in JSON mode across all push commands
rg -n 'this\.shouldOutputJson' --type=ts -g 'src/commands/push/**/*.ts' -A15 | grep -E "(shouldOutputJson|this\.error|this\.exit)" | head -60Repository: ably/ably-cli
Length of output: 4921
🏁 Script executed:
# Look at push/publish.ts to see how it handles errors in JSON mode
sed -n '160,185p' src/commands/push/publish.tsRepository: ably/ably-cli
Length of output: 754
🏁 Script executed:
# Check if there's a pattern in channels commands for list-channels
ls -la src/commands/push/channels/Repository: ably/ably-cli
Length of output: 530
Add this.exit(1) after logging JSON errors to ensure proper exit code.
When --json is used and an error occurs, the command logs JSON but doesn't call this.exit(1), causing the process to exit with code 0 instead of a non-zero error code. This pattern is inconsistent: push/publish.ts and push/batch-publish.ts correctly call this.exit(1) after logging JSON errors, but many other push commands (including list-channels.ts, devices/list.ts, channels/list.ts, remove commands, etc.) omit this call. Align all push commands to follow the pattern used in publish.ts by adding this.exit(1) in the JSON error path.
🤖 Prompt for AI Agents
In @src/commands/push/channels/list-channels.ts around lines 79 - 92, The JSON
error path in listChannelsCommand (check the this.shouldOutputJson(flags) branch
that logs this.formatJsonOutput({...})) logs the error but doesn't terminate
with a non-zero exit; add a call to this.exit(1) immediately after the
this.log(...) in that branch so the process returns a failure code when --json
is used, mirroring the pattern used in push/publish.ts and
push/batch-publish.ts.
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| const errorCode = (error as { code?: number }).code; | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.log( | ||
| this.formatJsonOutput( | ||
| { | ||
| error: errorMessage, | ||
| code: errorCode, | ||
| success: false, | ||
| }, | ||
| flags, | ||
| ), | ||
| ); | ||
| } else { | ||
| this.error(`Error removing subscriptions: ${errorMessage}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing non-zero exit code in JSON error output.
When an error occurs in JSON mode, the command logs the error but doesn't exit with a non-zero code. This could cause scripts relying on exit codes to incorrectly assume success.
🔎 Proposed fix
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{
error: errorMessage,
code: errorCode,
success: false,
},
flags,
),
);
+ this.exit(1);
} else {
this.error(`Error removing subscriptions: ${errorMessage}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| const errorMessage = | |
| error instanceof Error ? error.message : String(error); | |
| const errorCode = (error as { code?: number }).code; | |
| if (this.shouldOutputJson(flags)) { | |
| this.log( | |
| this.formatJsonOutput( | |
| { | |
| error: errorMessage, | |
| code: errorCode, | |
| success: false, | |
| }, | |
| flags, | |
| ), | |
| ); | |
| } else { | |
| this.error(`Error removing subscriptions: ${errorMessage}`); | |
| } | |
| } | |
| } catch (error) { | |
| const errorMessage = | |
| error instanceof Error ? error.message : String(error); | |
| const errorCode = (error as { code?: number }).code; | |
| if (this.shouldOutputJson(flags)) { | |
| this.log( | |
| this.formatJsonOutput( | |
| { | |
| error: errorMessage, | |
| code: errorCode, | |
| success: false, | |
| }, | |
| flags, | |
| ), | |
| ); | |
| this.exit(1); | |
| } else { | |
| this.error(`Error removing subscriptions: ${errorMessage}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/commands/push/channels/remove-where.ts around lines 114 - 133, The
JSON-path in the catch block for the remove-where command logs the error but
doesn't terminate with a non-zero exit code; after calling
this.log(this.formatJsonOutput(...)) when this.shouldOutputJson(flags) is true,
call this.exit(1) (or process.exit(1) if this.exit isn’t available) so the
command exits with a non-zero status; locate the catch block that builds
errorMessage/errorCode and uses this.shouldOutputJson(flags) and
this.formatJsonOutput to add the exit call immediately after logging the JSON
error.
| if (this.shouldOutputJson(flags)) { | ||
| this.log( | ||
| this.formatJsonOutput( | ||
| { | ||
| error: errorMessage, | ||
| success: false, | ||
| }, | ||
| flags, | ||
| ), | ||
| ); | ||
| } else { | ||
| this.error(`Error listing devices: ${errorMessage}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for JSON error output patterns in other commands
rg -n -A5 -B5 'shouldOutputJson.*flags.*error' --type=ts -g 'src/commands/**/*.ts'Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
cat -n src/commands/push/devices/list.ts | head -160 | tail -40Repository: ably/ably-cli
Length of output: 1325
🏁 Script executed:
#!/bin/bash
# Search for error handling patterns in commands
rg -n 'this\.error\(' --type=ts -g 'src/commands/**/*.ts' | head -20Repository: ably/ably-cli
Length of output: 1735
🏁 Script executed:
cat -n src/commands/push/devices/list.ts | sed -n '120,160p'Repository: ably/ably-cli
Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Search for commands that check shouldOutputJson to understand error handling patterns
rg -n -B10 'shouldOutputJson' --type=ts -g 'src/commands/**/*.ts' | grep -A10 -B10 'catch\|error'Repository: ably/ably-cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
# Search specifically for error handling in try-catch blocks where JSON output might be relevant
rg -n -A15 'catch\s*\(' --type=ts -g 'src/commands/**/*.ts' | head -100Repository: ably/ably-cli
Length of output: 6321
🏁 Script executed:
#!/bin/bash
# Check if there's a base Command class that defines shouldOutputJson and how it should be used
find . -name '*.ts' -type f | xargs grep -l 'shouldOutputJson' | grep -v node_modules | head -5Repository: ably/ably-cli
Length of output: 212
🏁 Script executed:
cat -n src/base-command.ts | head -100Repository: ably/ably-cli
Length of output: 4024
🏁 Script executed:
#!/bin/bash
# Search for jsonError method in base command
rg -n 'jsonError' --type=ts -g 'src/**/*.ts' | head -20Repository: ably/ably-cli
Length of output: 1669
🏁 Script executed:
cat -n src/commands/integrations/list.ts | sed -n '110,130p'Repository: ably/ably-cli
Length of output: 692
🏁 Script executed:
cat -n src/commands/push/devices/list.ts | sed -n '132,150p'Repository: ably/ably-cli
Length of output: 628
🏁 Script executed:
#!/bin/bash
# Search for jsonError method definition in base command
rg -n 'jsonError' --type=ts -A10 'src/base-command.ts' | head -50Repository: ably/ably-cli
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Search for jsonError in interactive-base-command as well
rg -n 'jsonError\(' --type=ts -A5 'src/interactive-base-command.ts'Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search more broadly for where jsonError is defined
find ./src -name '*.ts' -type f | xargs grep -l 'jsonError.*=' | head -10Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check another command to confirm the pattern - push/publish.ts
cat -n src/commands/push/publish.ts | sed -n '158,180p'Repository: ably/ably-cli
Length of output: 864
🏁 Script executed:
#!/bin/bash
# Let's also check push/batch-publish.ts to see the pattern
cat -n src/commands/push/batch-publish.ts | sed -n '134,160p'Repository: ably/ably-cli
Length of output: 943
Call this.exit(1) when logging JSON errors or use this.jsonError() helper instead.
When --json is used and an error occurs, the command logs JSON output via this.log() but doesn't exit with a non-zero code. This will cause automated tooling relying on exit codes to incorrectly treat the error as success.
Other commands in the codebase follow one of two patterns:
- Use
this.jsonError()helper (e.g.,src/commands/integrations/list.ts), which automatically exits with code 1 - Use
this.log()followed by explicitthis.exit(1)(e.g.,src/commands/push/publish.ts,src/commands/push/batch-publish.ts)
Current code (lines 136-148)
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{
error: errorMessage,
success: false,
},
flags,
),
);
} else {
this.error(`Error listing devices: ${errorMessage}`);
}
Either replace with this.jsonError() or add this.exit(1) after the JSON log.
🤖 Prompt for AI Agents
In @src/commands/push/devices/list.ts around lines 136 - 148, The JSON error
path in the devices list command uses this.log(this.formatJsonOutput(...)) but
doesn't terminate with a non-zero exit, so update the branch inside
shouldOutputJson(flags) to either call this.jsonError(...) passing the same
structured payload (error: errorMessage, success: false) or keep the log call
and immediately invoke this.exit(1) after it; locate the logic around
shouldOutputJson, formatJsonOutput and the current error handling to apply one
of these two fixes so JSON-mode failures return a non-zero exit code.
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| const errorCode = (error as { code?: number }).code; | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.log( | ||
| this.formatJsonOutput( | ||
| { | ||
| error: errorMessage, | ||
| code: errorCode, | ||
| success: false, | ||
| }, | ||
| flags, | ||
| ), | ||
| ); | ||
| } else { | ||
| if (errorCode === 40400) { | ||
| this.error(`Device not found: ${args.deviceId}`); | ||
| } else { | ||
| this.error(`Error removing device: ${errorMessage}`); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find all push commands to see error handling patterns
fd --type f 'remove\.ts$' src/commands/ | head -20Repository: ably/ably-cli
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Check other push commands for error handling patterns
find src/commands/push -name "*.ts" -type f | head -10Repository: ably/ably-cli
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Search for error handling in other commands that use JSON output
rg "shouldOutputJson" src/commands/ -A 10 -B 2 | head -100Repository: ably/ably-cli
Length of output: 5953
🏁 Script executed:
#!/bin/bash
# Look at the specific file to understand the context better
wc -l src/commands/push/devices/remove.tsRepository: ably/ably-cli
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the entire remove.ts file to understand full context
cat -n src/commands/push/devices/remove.tsRepository: ably/ably-cli
Length of output: 3599
🏁 Script executed:
#!/bin/bash
# Look for test files for the remove command
find . -path "*/test*" -o -path "*/spec*" | grep -i "remove" | grep -i "device" | head -10Repository: ably/ably-cli
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for test files more broadly
fd "remove\.test\." src/commands/push/devices/ 2>/dev/null || fd "remove\.spec\." src/commands/push/devices/ 2>/dev/null || find . -type f -name "*remove*.test.*" | grep push | grep deviceRepository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if test directory exists and find push device tests
find . -type d -name "__tests__" -o -type d -name "test" -o -type d -name "tests" | head -5Repository: ably/ably-cli
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Look for test files in common locations
ls -la src/commands/push/devices/ | grep -E "\.(test|spec)"Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search test directory for device remove tests
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | grep -i device | head -10Repository: ably/ably-cli
Length of output: 336
🏁 Script executed:
#!/bin/bash
# Check the base-command to see how error handling is intended
wc -l src/base-command.tsRepository: ably/ably-cli
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Look at relevant parts of base-command
head -150 src/base-command.tsRepository: ably/ably-cli
Length of output: 5269
🏁 Script executed:
#!/bin/bash
# Check the test file for remove.ts to see how exit codes are tested
cat -n test/unit/commands/push/devices/remove.test.ts | head -150Repository: ably/ably-cli
Length of output: 3299
🏁 Script executed:
#!/bin/bash
# Look at the full test file
wc -l test/unit/commands/push/devices/remove.test.tsRepository: ably/ably-cli
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check if tests verify exit codes for JSON error scenarios
rg "exit" test/unit/commands/push/devices/remove.test.ts -B 3 -A 3Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check how error testing is done in the test file
rg "json.*error|error.*json" test/unit/commands/push/devices/remove.test.ts -B 5 -A 5Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# View the entire test file
cat -n test/unit/commands/push/devices/remove.test.tsRepository: ably/ably-cli
Length of output: 3299
🏁 Script executed:
#!/bin/bash
# Check if other command tests check JSON error scenarios
rg "json.*error|error.*json|--json" test/unit/commands/push/ -l | head -5Repository: ably/ably-cli
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Look at save.ts test to see if it tests JSON error handling
cat -n test/unit/commands/push/devices/save.test.ts | grep -A 20 "json"Repository: ably/ably-cli
Length of output: 1412
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that verify exit codes with JSON output
rg "exit\(" test/unit/commands/ -B 3 -A 3 | head -50Repository: ably/ably-cli
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Look for how other commands handle error exit codes in JSON mode
rg "this\.exit\(" src/commands/ -B 5 -A 2 | head -80Repository: ably/ably-cli
Length of output: 2608
🏁 Script executed:
#!/bin/bash
# Check interactive-base-command for error method behavior
rg "error\(" src/interactive-base-command.ts -B 3 -A 5Repository: ably/ably-cli
Length of output: 1028
Add exit(1) in JSON error handling to match error exit codes in non-JSON mode.
The command inconsistently handles errors between JSON and non-JSON output modes. In JSON mode (lines 82-92), the code logs the error but exits with code 0 due to implicit return. The non-JSON path correctly calls this.error() which exits with code 1. This pattern is already established in other push commands (publish.ts, batch-publish.ts).
🔎 Required fix
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{
error: errorMessage,
code: errorCode,
success: false,
},
flags,
),
);
+ this.exit(1);
} else {🤖 Prompt for AI Agents
In @src/commands/push/devices/remove.ts around lines 77 - 101, The JSON error
branch in the catch of the remove device command logs the error via
this.formatJsonOutput when this.shouldOutputJson(flags) is true but does not
exit with a non-zero code, causing inconsistent exit behavior versus the
non-JSON branch that calls this.error(); modify the catch so after logging the
JSON error (using this.log(this.formatJsonOutput(...))) you invoke
this.error(...) or otherwise call process.exit(1) to ensure the command exits
with code 1 on error; locate the catch block around the remove device handler
where errorMessage/errorCode are computed and add the exit behavior for the JSON
branch while preserving the existing 40400-specific message semantics.
| afterEach(async () => { | ||
| setupTestFailureHandler(); | ||
| await cleanupTrackedResources(); | ||
| }); |
There was a problem hiding this comment.
setupTestFailureHandler should be called in beforeEach, not afterEach.
The setupTestFailureHandler function registers a handler for test failures using onTestFailed. Calling it in afterEach means the handler is registered after the test has already run, which defeats its purpose. It should be called in beforeEach to capture failures during test execution.
🔎 Proposed fix
beforeEach(() => {
resetTestTracking();
+ setupTestFailureHandler();
});
afterEach(async () => {
- setupTestFailureHandler();
await cleanupTrackedResources();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(async () => { | |
| setupTestFailureHandler(); | |
| await cleanupTrackedResources(); | |
| }); | |
| beforeEach(() => { | |
| resetTestTracking(); | |
| setupTestFailureHandler(); | |
| }); | |
| afterEach(async () => { | |
| await cleanupTrackedResources(); | |
| }); |
🤖 Prompt for AI Agents
In @test/e2e/push/publish-e2e.test.ts around lines 73 - 76, The test failure
handler is being registered too late; move the setupTestFailureHandler() call
from the afterEach block into a beforeEach so it registers before each test
runs. Specifically, remove setupTestFailureHandler() from the afterEach that
currently contains setupTestFailureHandler() and await
cleanupTrackedResources(), add a beforeEach(() => setupTestFailureHandler()) (or
await if it returns a promise) and leave await cleanupTrackedResources() in
afterEach to continue cleanup after each test. Ensure you reference
setupTestFailureHandler, beforeEach, and afterEach when applying the change.
Fixes three issues identified in PR review comments: 1. Remove --use-for-sandbox flag from set-apns-p12 command (PR #126 comment: flag had no API effect, showed misleading output) - Sandbox vs Production is now determined by certificate type - Removed flag definition, examples, and conditional output 2. Fix clear-apns to detect token-based auth configuration (PR #126 comment: detection used old fields, missed token auth) - Now uses apnsAuthType field for detection (consistent with show.ts) - Clears both token-based and certificate-based auth fields 3. Add this.exit(1) for JSON mode errors in push commands (PR #126 comment: JSON errors exited with code 0) - Fixed: devices/get, list, save, remove, remove-where - Fixed: channels/list, list-channels, save, remove, remove-where 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add --use-sandbox flag to APNs authentication in `ably push config set-apns`. Users must specify the environment for both token and certificate auth (defaults to production). - Token auth (.p8): apnsUseSandboxEndpoint sent with every request - Certificate auth (.p12): apnsUseSandboxEndpoint sent with every request Users are responsible for matching the endpoint with their certificate type (development cert = sandbox, distribution cert = production). Also removes deprecated `ably apps set-apns-p12` command - all push configuration should be under `ably push` namespace. Ref: PDR-088, WEB-3698
- Skip confirmation prompts in clear-apns and clear-fcm when --json flag is used, preventing automation hangs - Add null check in batch-publish validateBatchItems() before accessing item.recipient to handle malformed input arrays
| this.error(`Certificate file not found: ${certificatePath}`); | ||
| } | ||
|
|
||
| this.log(`Uploading APNs P12 certificate for app ${appId}...`); |
There was a problem hiding this comment.
JSON output corrupted by unconditional logging in set-apns
Medium Severity
The progress messages at lines 111 and 161 are logged unconditionally with this.log() before JSON output is produced. When users run the command with --json flag, these text messages will be prepended to the JSON output, breaking machine-readable parsing. Compare with set-fcm.ts (lines 78-82) which correctly wraps the progress message in if (!this.shouldOutputJson(flags)).
Additional Locations (1)
There was a problem hiding this comment.
This is a known CLI issue that we're going to address
…flicts - clear-fcm: Output proper JSON when FCM is not configured (was outputting colored text which corrupted JSON output) - clear-apns: Suppress progress message in JSON mode to avoid corrupting JSON output - publish: Validate that notification flags (--sound, --icon, --badge, --data, --collapse-key, --ttl, --apns, --fcm, --web) are not used with --payload, preventing silent flag ignorance 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… APNs detection Issue 1: Flag naming conflict - Renamed --client-id to --recipient-client-id in all push commands to avoid collision with the global --client-id authentication flag - Affected commands: publish, channels/save, channels/remove, channels/remove-where, channels/list, devices/save, devices/list, devices/remove-where - Updated all corresponding unit tests Issue 2: APNs detection ignores legacy configuration - Expanded APNs configuration detection to check both new (apnsAuthType) and legacy (apnsUsesSandboxCert) fields - Apps configured before Control API update may have apnsUsesSandboxCert set without apnsAuthType - Affected commands: clear-apns, show
AndyTWF
left a comment
AndyTWF
left a comment
There was a problem hiding this comment.
Few bits - mostly using utility functions etc
| const errorCode = (error as { code?: number }).code; | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.log( |
There was a problem hiding this comment.
We have this.jsonError which handles the formatting and throwing of errors in a way oclif likes
|
|
||
| static override examples = [ | ||
| // List all subscriptions for a channel | ||
| "$ ably push channels list --channel alerts", |
There was a problem hiding this comment.
I would have expected to see ably push channels list return a list of channels with push subscriptions, and then another one like ably push channels get that returns the details for one channel
| // Confirm deletion unless --force is used | ||
| if (!flags.force && !this.shouldOutputJson(flags)) { | ||
| const { default: inquirer } = await import("inquirer"); | ||
| const { confirmed } = await inquirer.prompt([ |
There was a problem hiding this comment.
We have a standardised confirmation util now in prompt-confirmation.ts so would be good to use/upgrade that
src/commands/push/channels/remove.ts
Outdated
| // Confirm deletion unless --force is used | ||
| if (!flags.force && !this.shouldOutputJson(flags)) { | ||
| const { default: inquirer } = await import("inquirer"); | ||
| const { confirmed } = await inquirer.prompt([ |
There was a problem hiding this comment.
Ditto re confirmation utils (I wont comment on future examples)
|
|
||
| // Confirm unless --force is used or in JSON mode (JSON mode should not prompt) | ||
| if (!flags.force && !this.shouldOutputJson(flags)) { | ||
| const confirmed = await this.interactiveHelper.confirm( |
There was a problem hiding this comment.
Looks like we have 3 different ways of confirming (2 in this PR):
- The existing prompt-confirmation helper in utils
- Some commands in this PR import inquirer
- This command uses interactiveHelper
Can we pick one?
| // Require at least one filter criterion | ||
| if (!flags["recipient-client-id"] && !flags["device-id"]) { | ||
| if (this.shouldOutputJson(flags)) { | ||
| this.log( |
There was a problem hiding this comment.
Using this.jsonError here will solve the issue that cursor found
src/commands/push/batch-publish.ts
Outdated
| const response = await rest.request( | ||
| "POST", | ||
| "/push/batch/publish", | ||
| 2, // API version |
test/e2e/push/channels-e2e.test.ts
Outdated
| }); | ||
|
|
||
| describe("push channels list-channels", () => { | ||
| it.skipIf(SHOULD_SKIP_E2E)( |
There was a problem hiding this comment.
vitest supports describe.skipIf, which we can do just once on the outermost describe
| // Note: Tests that require push-enabled channels are not included here | ||
| // because the test environment doesn't have push configured for channels. | ||
| // The validation tests above verify the CLI command structure is correct. | ||
| // Full integration tests would require a push-enabled namespace configuration. |
There was a problem hiding this comment.
It should be quite trivial to use the control API in a test to create the prerequisite namespace? We should definitely have this tested
| describe("push:channels:list-channels command", () => { | ||
| beforeEach(() => { | ||
| const mock = getMockAblyRest(); | ||
| mock.push.admin.channelSubscriptions.listChannels.mockReset(); |
There was a problem hiding this comment.
No need to do this cleanup on a per-file basis, for unit tests they're cleared up in an afterEach in test/unit/setup.ts
|
Thanks for all the great feedback @AndyTWF 👍 |
Add "Discover Patterns Before Implementing" section to help AI agents and contributors find and follow existing codebase conventions before implementing common functionality. This addresses review feedback where existing utilities (jsonError, promptForConfirmation) were not used because their existence wasn't discovered before implementation.
- Use this.jsonError() instead of manual JSON error formatting - Use promptForConfirmation utility instead of inquirer imports - Change batch-publish API version from 2 to 4 - Use describe.skipIf in E2E tests instead of it.skipIf on each test - Remove unnecessary mockReset() from unit test beforeEach hooks
|
@AndyTWF I was honestly quite annoyed that there were so many obvious problems with this PR that I created. I decided to revisit my coding/agent rules and prompts, and then reflect on the rules for this repo too, as I think these mistakes are obvious (thanks for pointing them out, I should have known about the obvious problems 😢) and should be avoid for future contributors too. So I've add c3e5099 to CLAUDE.md, which I hope will help, and then addressed your feedback. Shout if you think I've missed anything in the "lessons" for CLAUDE or otherwise. |
Whilst working on adding the new push notification dashboards, I realised users are quite limited by what they can do with push today because:
So I decided to just go ahead and add push support, and follow the new naming convention from @m-hulbert in
PDR-087: Updated CLI command hierarchy, the new patterns / testing implemented by @AndyTWF in this repo. I think that's a big win for completeness of this CLI, and it will materially improve how users can use push in the new dashboards with the CLI, along with the new UI I have added (see recording of new dashboards with push notification inspector & testing tool mentioned in this Slack post).
Note that whilst this does work, the APNS and FCM APIs are broken right now, so I have a fix at https://github.com/ably/website/commit/8d632061042074c35098821075bbc2fa2b03e4cc which I am hoping will make it upstream in the coming week or so. It is part of a much larger set of improvements, including the new push dashboard overhaul work in these commits.
Here is the brief I used to execute on this work:
Executive Summary
This proposal defines a comprehensive set of CLI commands for managing and testing push notifications in Ably. The commands are designed to be consistent with existing CLI patterns and align with the new command hierarchy defined in PDR-087.
Investigation: Why Only APNs and Not FCM?
Current State
The CLI currently only supports setting APNs credentials via
ably apps set-apns-p12, which uses the Control API endpointPOST /apps/{id}/pkcs12.Root Cause
The FCM configuration was simply never implemented, despite the Control API fully supporting it.
The Control API supports both APNs and FCM configuration through the app PATCH endpoint with these fields:
apnsCertificate,apnsPrivateKey,apnsUseSandboxEndpointfcmServiceAccount,fcmProjectIdThe CLI implemented a dedicated P12 upload command but never added equivalent FCM commands. This appears to be an oversight rather than a technical limitation.
Recommendation
Add FCM configuration support alongside the APNs commands in the new
ably pushcommand group.Command Hierarchy
Following PDR-087's decision to move
ably apps set-apns-p12toably push set-apns-p12, the full push command hierarchy will be:Naming Convention Note
The CLI uses shortened namespace names (
devicesandchannels) rather than the full SDK names (deviceRegistrationsandchannelSubscriptions) for brevity. Typingably push device-registrations remove-whererepeatedly would be cumbersome for users. The help text for each command will reference the full SDK method name (e.g., "Maps to push.admin.deviceRegistrations.save()") to maintain clarity for developers familiar with the SDK.Additionally, per PDR-087, push logs remain under
ably logs push:Detailed Command Specifications
1. Configuration Commands (
ably push config)ably push config showShow the current push notification configuration status for an app.
Flags:
--app- App ID or name (uses current app if not specified)--json- Output as JSON--pretty-json- Output as formatted JSONOutput (text):
ably push config set-apnsConfigure Apple Push Notification service credentials. Supports both certificate-based (.p12) and token-based (.p8) authentication.
Flags:
--app- App ID or name (uses current app if not specified)--certificate- Path to .p12 certificate file--password- Password for the .p12 certificate--sandbox- Use APNs sandbox environment (default: production)--key-file- Path to .p8 private key file (token auth)--key-id- Key ID from Apple Developer (token auth)--team-id- Team ID from Apple Developer (token auth)--topic- App bundle identifier (token auth)--json- Output as JSONExamples:
ably push config set-fcmConfigure Firebase Cloud Messaging credentials.
Flags:
--app- App ID or name (uses current app if not specified)--service-account- Path to Firebase service account JSON file--json- Output as JSONExamples:
ably push config clear-apnsRemove APNs configuration from an app.
Flags:
--app- App ID or name--force- Skip confirmation prompt--json- Output as JSONably push config clear-fcmRemove FCM configuration from an app.
Flags:
--app- App ID or name--force- Skip confirmation prompt--json- Output as JSON2. Device Registration Commands (
ably push devices)Maps to SDK:
push.admin.deviceRegistrationsably push devices saveRegister a new device or update an existing device registration.
Flags:
--app- App ID or name--id- Device identifier (required unless using --data)--platform- Platform:ios,android, orbrowser(required unless using --data)--form-factor- Form factor:phone,tablet,desktop,tv,watch,car,embedded--client-id- Optional client identifier to associate with the device--transport-type- Push transport:apns,fcm, orweb--device-token- Device token for APNs/FCM (or registration token for FCM)--metadata- Optional metadata as JSON string--data- Full device details as JSON string or file path--json- Output as JSONExamples:
ably push devices getGet detailed information about a specific device.
Args:
DEVICE_ID- The device identifier (required)Flags:
--app- App ID or name--json- Output as JSON--pretty-json- Output as formatted JSONOutput (text):
ably push devices listList registered devices for the current app.
Flags:
--app- App ID or name--client-id- Filter by client ID--device-id- Filter by device ID--state- Filter by state:ACTIVE,FAILING, orFAILED--limit- Maximum number of results (default: 100, max: 1000)--json- Output as JSON--pretty-json- Output as formatted JSONOutput (text):
ably push devices removeRemove a single device registration.
Args:
DEVICE_ID- The device identifier (required)Flags:
--app- App ID or name--force- Skip confirmation prompt--json- Output as JSONably push devices remove-whereRemove devices matching filter criteria.
Flags:
--app- App ID or name--client-id- Remove all devices for this client--device-id- Remove specific device by ID--force- Skip confirmation prompt--json- Output as JSONNote: At least one filter (
--client-idor--device-id) must be specified.3. Channel Subscription Commands (
ably push channels)Maps to SDK:
push.admin.channelSubscriptionsably push channels saveSubscribe a device or client to a push-enabled channel.
Flags:
--app- App ID or name--channel- Channel name to subscribe to (required)--device-id- Device ID to subscribe--client-id- Client ID to subscribe (subscribes all client's devices)--json- Output as JSONNote: Either
--device-idor--client-idmust be specified.Examples:
ably push channels listList push channel subscriptions.
Flags:
--app- App ID or name--channel- Filter by channel name (required)--client-id- Filter by client ID--device-id- Filter by device ID--limit- Maximum number of results (default: 100, max: 1000)--json- Output as JSONOutput (text):
ably push channels list-channelsList all channels that have at least one push subscription.
Flags:
--app- App ID or name--limit- Maximum number of results (default: 100, max: 1000)--json- Output as JSONOutput (text):
ably push channels removeRemove a single subscription.
Flags:
--app- App ID or name--channel- Channel to unsubscribe from (required)--device-id- Device ID to unsubscribe--client-id- Client ID to unsubscribe--force- Skip confirmation prompt--json- Output as JSONNote: Either
--device-idor--client-idmust be specified.ably push channels remove-whereRemove subscriptions matching filter criteria.
Flags:
--app- App ID or name--channel- Channel to unsubscribe from (required)--device-id- Filter by device ID--client-id- Filter by client ID--force- Skip confirmation prompt--json- Output as JSONNote: Channel is required. At least one additional filter (
--device-idor--client-id) is recommended to avoid removing all subscriptions for a channel.4. Publish Commands (
ably push publish)ably push publishPublish a push notification directly to device(s) or client(s).
Flags:
--app- App ID or name--device-id- Target device ID--client-id- Target client ID (all their devices)--title- Notification title--body- Notification body--sound- Notification sound (default, or filename)--icon- Notification icon (Android/Web)--badge- Badge count (iOS)--data- Custom data payload (JSON string)--payload- Full notification payload (JSON string or file path)--collapse-key- Collapse key for notification grouping--ttl- Time-to-live in seconds--apns- APNs-specific overrides (JSON)--fcm- FCM-specific overrides (JSON)--web- Web Push-specific overrides (JSON)--json- Output as JSONExamples:
ably push batch-publishPublish push notifications to multiple recipients in a single request.
Flags:
--app- App ID or name--payload- Batch payload (JSON array, file path, or-for stdin) - required--json- Output as JSONBatch Payload Format:
[ { "recipient": { "deviceId": "abc123" }, "payload": { "notification": { "title": "Hello", "body": "World" } } }, { "recipient": { "clientId": "user-456" }, "payload": { "notification": { "title": "Hi", "body": "There" }, "data": { "key": "value" } } } ]Limits:
Output (text):
API Mapping
Device Registrations (
push.admin.deviceRegistrations)push devices savesave()/push/deviceRegistrationspush devices getget()/push/deviceRegistrations/{deviceId}push devices listlist()/push/deviceRegistrationspush devices removeremove()/push/deviceRegistrations/{deviceId}push devices remove-whereremoveWhere()/push/deviceRegistrationsChannel Subscriptions (
push.admin.channelSubscriptions)push channels savesave()/push/channelSubscriptionspush channels listlist()/push/channelSubscriptionspush channels list-channelslistChannels()/push/channelspush channels removeremove()/push/channelSubscriptionspush channels remove-whereremoveWhere()/push/channelSubscriptionsPublishing (
push.admin)push publishpublish()/push/publishpush batch-publish/push/batch/publishConfiguration (Control API)
push config set-apns(p12)/apps/{id}/pkcs12push config set-apns(pem/p8)/apps/{id}push config set-fcm/apps/{id}push config clear-apns/apps/{id}push config clear-fcm/apps/{id}push config show/apps/{id}Note
Adds end-to-end push notification management to the CLI and cleans up legacy APNs handling.
ably pushsuite:config(show/set-apns/set-fcm/clear-apns/clear-fcm),devices(save/get/list/remove/remove-where),channels(save/list/list-channels/remove/remove-where),publish, andbatch-publishapps set-apns-p12command and example; replaced withpush config set-apnsAPPID,INTEGRATIONID,QUEUEID); addedably config path/show; clarified defaults and added flags (e.g.,--sequence-numbers,--show-others, duration flags) across several commandsWritten by Cursor Bugbot for commit 73ce711. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Documentation
[APPID],[INTEGRATIONID]).✏️ Tip: You can customize this high-level summary in your review settings.