Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPrioritizes environment-based authentication (ABLY_TOKEN, ABLY_API_KEY, ABLY_ACCESS_TOKEN), centralizes flag sets in a new flags module, updates base/feature command classes to use composed flag groups, adds endpoint storage to config manager, and adjusts numerous commands and tests to use environment-driven auth and new flags. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (flags/args)
participant ENV as Environment (ABLY_* vars)
participant Config as ConfigManager
participant Auth as Auth Helper (applyApiKeyAuth / resolve)
participant Ably as Ably Client
CLI->>Auth: run command (flags)
Auth->>ENV: check ABLY_TOKEN / ABLY_API_KEY / ABLY_ACCESS_TOKEN
alt env credential found
ENV-->>Auth: return token/apiKey
else no env credential
Auth->>Config: ensureAppAndKey / lookup stored apiKey/token
Config-->>Auth: account/app key or undefined
end
Auth->>Auth: validate/parse apiKey (parseApiKey) if needed
Auth->>Ably: build client options (endpoint from ENV or Config)
Ably-->>CLI: operate with resolved auth & options
(Note: rectangle coloring not rendered in sequence syntax; components represent the sequential auth resolution flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/unit/commands/queues/create.test.ts (1)
212-215:⚠️ Potential issue | 🟡 MinorAdd auth-header assertion for the
/appslookup call.In Lines 212–215, this mock does not validate
Authorization, so a regression on that request could slip through while the test still passes.Patch to assert Bearer token on the intermediate request
- nock("https://control.ably.net") - .get(`/v1/accounts/${accountId}/apps`) - .reply(200, [{ id: appId, accountId, name: "Test App" }]); + nock("https://control.ably.net", { + reqheaders: { + authorization: `Bearer ${customToken}`, + }, + }) + .get(`/v1/accounts/${accountId}/apps`) + .reply(200, [{ id: appId, accountId, name: "Test App" }]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/queues/create.test.ts` around lines 212 - 215, The nock for the apps lookup does not assert the Authorization header, so add a header assertion to the existing nock chain for the GET `/v1/accounts/${accountId}/apps` call (the nock that currently replies 200 with [{ id: appId, accountId, name: "Test App" }]); specifically, use nock's header matcher (e.g., .matchHeader('authorization' /* or 'Authorization' */, `Bearer ${token}`) or the test's actual token variable) so the mock validates the same bearer token used elsewhere in the test before replying.src/commands/auth/revoke-token.ts (1)
193-197:⚠️ Potential issue | 🟠 MajorDo not hardcode the revocation API hostname.
hostname: "rest.ably.io"ignores configured/account-specific endpoints, so this command can target the wrong environment after the endpoint/global-flags refactor.Suggested fix
- const response = await this.makeHttpRequest( + const response = await this.makeHttpRequest( + (this.getClientOptions(flags).restHost as string) || "rest.ably.io", keyName, secret, requestBody, flags.debug, );private makeHttpRequest( + hostname: string, keyName: string, secret: string, requestBody: Record<string, unknown>, debug: boolean, ): Promise<Record<string, unknown> | string | null> { @@ - hostname: "rest.ably.io", + hostname,As per coding guidelines, "Use configuration for endpoint URLs (controlHost) instead of hardcoding endpoint URLs, following the pattern: flags.controlHost || config.controlHost || 'control.ably.net'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/auth/revoke-token.ts` around lines 193 - 197, The request to revoke tokens currently hardcodes hostname: "rest.ably.io" in the HTTP options, which breaks account/endpoint overrides; update the request construction in revoke-token.ts (the object using hostname, method, path, port and path `/keys/${keyName}/revokeTokens`) to use the configured control host instead of a literal string by selecting flags.controlHost || config.controlHost || 'control.ably.net' (or the equivalent controlHost variable used elsewhere) and pass that value into the hostname field so the command respects configured endpoints.src/services/config-manager.ts (1)
361-367:⚠️ Potential issue | 🟡 MinorPotential data loss:
storeAccountdoes not preserve theendpointfield.The
storeAccountmethod preservesappsandcurrentAppIdfrom the existing account but does not preserveendpoint. IfstoreEndpointis called beforestoreAccount(e.g., during a re-login flow), the stored endpoint will be lost.🛡️ Proposed fix to preserve endpoint
// Create or update the account entry this.config.accounts[alias] = { accessToken, ...accountInfo, apps: this.config.accounts[alias]?.apps || {}, currentAppId: this.config.accounts[alias]?.currentAppId, + endpoint: this.config.accounts[alias]?.endpoint, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/config-manager.ts` around lines 361 - 367, The storeAccount method overwrites the account entry without preserving the existing endpoint; update the merge in storeAccount (the assignment to this.config.accounts[alias]) to include the previous endpoint when present (e.g., pull the existing account object from this.config.accounts[alias] and carry over .endpoint alongside apps and currentAppId) so storeEndpoint calls done earlier are not lost; reference the storeAccount method and the this.config.accounts[alias] merge to apply the change.
🧹 Nitpick comments (5)
test/unit/commands/queues/delete.test.ts (1)
9-12: Use env save/restore in teardown to avoid mutating process-wide baseline state.Line 11 unconditionally deletes
ABLY_ACCESS_TOKEN; restoring the prior value is safer when tests run in shared processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/queues/delete.test.ts` around lines 9 - 12, The afterEach teardown currently unconditionally deletes process.env.ABLY_ACCESS_TOKEN which mutates global state; change the afterEach in test/unit/commands/queues/delete.test.ts to save the original value (e.g. const prevAbly = process.env.ABLY_ACCESS_TOKEN) before tests run and restore it in afterEach (set process.env.ABLY_ACCESS_TOKEN = prevAbly or delete if it was undefined), keeping the nock.cleanAll() call; reference the existing afterEach and the environment key process.env.ABLY_ACCESS_TOKEN when making this change.test/unit/commands/auth/keys/create.test.ts (1)
18-21: Prefer restoring priorABLY_ACCESS_TOKENvalue inafterEach.Line 20 can unintentionally erase a pre-existing token for later tests. Save-and-restore keeps suite isolation stronger.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/create.test.ts` around lines 18 - 21, The afterEach currently deletes process.env.ABLY_ACCESS_TOKEN which can clobber a pre-existing token; instead capture the original value before tests (e.g., in a beforeEach or at module scope) and restore it in afterEach; update the afterEach that references process.env.ABLY_ACCESS_TOKEN to set it back to the saved value (or delete it only if it was originally undefined) so tests restore the prior environment state; look for the afterEach block and any related beforeEach where you can store the originalToken variable to implement this.test/unit/commands/queues/list.test.ts (1)
7-10: Restore originalABLY_ACCESS_TOKENvalue instead of always deleting it.If the runner starts with
ABLY_ACCESS_TOKENalready set, Line 9 permanently clears it for later suites. Prefer save-and-restore to avoid cross-test pollution.Suggested patch
-import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; import nock from "nock"; import { runCommand } from "@oclif/test"; import { getMockConfigManager } from "../../../helpers/mock-config-manager.js"; describe("queues:list command", () => { + let previousAccessToken: string | undefined; + + beforeEach(() => { + previousAccessToken = process.env.ABLY_ACCESS_TOKEN; + }); + afterEach(() => { nock.cleanAll(); - delete process.env.ABLY_ACCESS_TOKEN; + if (previousAccessToken === undefined) { + delete process.env.ABLY_ACCESS_TOKEN; + } else { + process.env.ABLY_ACCESS_TOKEN = previousAccessToken; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/queues/list.test.ts` around lines 7 - 10, The afterEach currently deletes process.env.ABLY_ACCESS_TOKEN which can clobber a pre-existing token; change tests to save the original before mutation (e.g. capture const originalAblyToken = process.env.ABLY_ACCESS_TOKEN at the top of the test file or in a beforeEach) and in afterEach restore it instead of unconditionally deleting (set process.env.ABLY_ACCESS_TOKEN = originalAblyToken or delete it when originalAblyToken === undefined); keep the existing nock.cleanAll() call intact and reference the afterEach cleanup block and the ABLY_ACCESS_TOKEN env var when making the change.src/commands/logs/connection-lifecycle/subscribe.ts (1)
12-17: Consider adding an environment variable example for consistency.Other commands in this PR (e.g.,
channels subscribe,channels presence subscribe) include an example showingABLY_API_KEY="YOUR_API_KEY"usage. Adding a similar example here would maintain consistency across the CLI documentation.📝 Suggested example addition
static override examples = [ "$ ably logs connection-lifecycle subscribe", "$ ably logs connection-lifecycle subscribe --json", "$ ably logs connection-lifecycle subscribe --pretty-json", "$ ably logs connection-lifecycle subscribe --duration 30", + '$ ABLY_API_KEY="YOUR_API_KEY" ably logs connection-lifecycle subscribe', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/logs/connection-lifecycle/subscribe.ts` around lines 12 - 17, Add a consistent environment-variable example to the static override examples array for this command by inserting an entry like 'ABLY_API_KEY="YOUR_API_KEY" $ ably logs connection-lifecycle subscribe' into the examples list (the static property named examples in this file/class), ensuring it follows the style and ordering used by other commands (e.g., channels subscribe) and keeping the existing variants (--json, --pretty-json, --duration) intact.test/unit/base/auth-info-display.test.ts (1)
191-212: LGTM! Token display test properly validates ABLY_TOKEN handling.The test verifies that auth info display shows token information when
ABLY_TOKENis present while hiding account info. The cleanup on line 200 is placed before assertions, which works but could be improved by using a try/finally pattern for more robust cleanup.💡 Optional: Use try/finally for more robust cleanup
it("should display app and auth info when ABLY_TOKEN env var is set", async function () { // Setup shouldHideAccountInfoStub.mockReturnValue(true); process.env.ABLY_TOKEN = "test-token-value"; - // Execute - await command.testDisplayAuthInfo({}); - - // Cleanup - delete process.env.ABLY_TOKEN; + try { + // Execute + await command.testDisplayAuthInfo({}); + } finally { + // Cleanup + delete process.env.ABLY_TOKEN; + } // Verify output includes token info but not account info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/base/auth-info-display.test.ts` around lines 191 - 212, The test sets process.env.ABLY_TOKEN and currently deletes it before assertions; change the test to ensure cleanup always runs by wrapping the execution and assertions in a try/finally so process.env.ABLY_TOKEN is deleted in the finally block; locate the test case referencing shouldHideAccountInfoStub, process.env.ABLY_TOKEN, command.testDisplayAuthInfo, and logStub and move the delete process.env.ABLY_TOKEN into the finally to guarantee environment cleanup even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/base-command.ts`:
- Around line 555-557: Restore the persisted/config-backed fallback when
building controlHost: instead of only using flags["control-host"] ||
process.env.ABLY_CONTROL_HOST, change the expression to prefer CLI flag, then
env var, then the persisted account/config value (e.g.
this.config.get('controlHost') or this.account?.controlHost — whichever API is
used in this class) and finally the default; update both call sites where
controlHost is constructed (the occurrences that use flags["control-host"] ||
process.env.ABLY_CONTROL_HOST) so the control-plane host respects the stored
account configuration.
---
Outside diff comments:
In `@src/commands/auth/revoke-token.ts`:
- Around line 193-197: The request to revoke tokens currently hardcodes
hostname: "rest.ably.io" in the HTTP options, which breaks account/endpoint
overrides; update the request construction in revoke-token.ts (the object using
hostname, method, path, port and path `/keys/${keyName}/revokeTokens`) to use
the configured control host instead of a literal string by selecting
flags.controlHost || config.controlHost || 'control.ably.net' (or the equivalent
controlHost variable used elsewhere) and pass that value into the hostname field
so the command respects configured endpoints.
In `@src/services/config-manager.ts`:
- Around line 361-367: The storeAccount method overwrites the account entry
without preserving the existing endpoint; update the merge in storeAccount (the
assignment to this.config.accounts[alias]) to include the previous endpoint when
present (e.g., pull the existing account object from this.config.accounts[alias]
and carry over .endpoint alongside apps and currentAppId) so storeEndpoint calls
done earlier are not lost; reference the storeAccount method and the
this.config.accounts[alias] merge to apply the change.
In `@test/unit/commands/queues/create.test.ts`:
- Around line 212-215: The nock for the apps lookup does not assert the
Authorization header, so add a header assertion to the existing nock chain for
the GET `/v1/accounts/${accountId}/apps` call (the nock that currently replies
200 with [{ id: appId, accountId, name: "Test App" }]); specifically, use nock's
header matcher (e.g., .matchHeader('authorization' /* or 'Authorization' */,
`Bearer ${token}`) or the test's actual token variable) so the mock validates
the same bearer token used elsewhere in the test before replying.
---
Nitpick comments:
In `@src/commands/logs/connection-lifecycle/subscribe.ts`:
- Around line 12-17: Add a consistent environment-variable example to the static
override examples array for this command by inserting an entry like
'ABLY_API_KEY="YOUR_API_KEY" $ ably logs connection-lifecycle subscribe' into
the examples list (the static property named examples in this file/class),
ensuring it follows the style and ordering used by other commands (e.g.,
channels subscribe) and keeping the existing variants (--json, --pretty-json,
--duration) intact.
In `@test/unit/base/auth-info-display.test.ts`:
- Around line 191-212: The test sets process.env.ABLY_TOKEN and currently
deletes it before assertions; change the test to ensure cleanup always runs by
wrapping the execution and assertions in a try/finally so process.env.ABLY_TOKEN
is deleted in the finally block; locate the test case referencing
shouldHideAccountInfoStub, process.env.ABLY_TOKEN, command.testDisplayAuthInfo,
and logStub and move the delete process.env.ABLY_TOKEN into the finally to
guarantee environment cleanup even if assertions fail.
In `@test/unit/commands/auth/keys/create.test.ts`:
- Around line 18-21: The afterEach currently deletes
process.env.ABLY_ACCESS_TOKEN which can clobber a pre-existing token; instead
capture the original value before tests (e.g., in a beforeEach or at module
scope) and restore it in afterEach; update the afterEach that references
process.env.ABLY_ACCESS_TOKEN to set it back to the saved value (or delete it
only if it was originally undefined) so tests restore the prior environment
state; look for the afterEach block and any related beforeEach where you can
store the originalToken variable to implement this.
In `@test/unit/commands/queues/delete.test.ts`:
- Around line 9-12: The afterEach teardown currently unconditionally deletes
process.env.ABLY_ACCESS_TOKEN which mutates global state; change the afterEach
in test/unit/commands/queues/delete.test.ts to save the original value (e.g.
const prevAbly = process.env.ABLY_ACCESS_TOKEN) before tests run and restore it
in afterEach (set process.env.ABLY_ACCESS_TOKEN = prevAbly or delete if it was
undefined), keeping the nock.cleanAll() call; reference the existing afterEach
and the environment key process.env.ABLY_ACCESS_TOKEN when making this change.
In `@test/unit/commands/queues/list.test.ts`:
- Around line 7-10: The afterEach currently deletes
process.env.ABLY_ACCESS_TOKEN which can clobber a pre-existing token; change
tests to save the original before mutation (e.g. capture const originalAblyToken
= process.env.ABLY_ACCESS_TOKEN at the top of the test file or in a beforeEach)
and in afterEach restore it instead of unconditionally deleting (set
process.env.ABLY_ACCESS_TOKEN = originalAblyToken or delete it when
originalAblyToken === undefined); keep the existing nock.cleanAll() call intact
and reference the afterEach cleanup block and the ABLY_ACCESS_TOKEN env var when
making the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (71)
src/base-command.tssrc/chat-base-command.tssrc/commands/accounts/login.tssrc/commands/accounts/switch.tssrc/commands/apps/create.tssrc/commands/apps/delete.tssrc/commands/apps/update.tssrc/commands/auth/issue-ably-token.tssrc/commands/auth/issue-jwt-token.tssrc/commands/auth/revoke-token.tssrc/commands/bench/publisher.tssrc/commands/bench/subscriber.tssrc/commands/channels/batch-publish.tssrc/commands/channels/history.tssrc/commands/channels/inspect.tssrc/commands/channels/list.tssrc/commands/channels/occupancy/get.tssrc/commands/channels/occupancy/subscribe.tssrc/commands/channels/presence/enter.tssrc/commands/channels/presence/subscribe.tssrc/commands/channels/publish.tssrc/commands/channels/subscribe.tssrc/commands/config/index.tssrc/commands/config/path.tssrc/commands/config/show.tssrc/commands/connections/test.tssrc/commands/logs/channel-lifecycle/subscribe.tssrc/commands/logs/connection-lifecycle/history.tssrc/commands/logs/connection-lifecycle/subscribe.tssrc/commands/logs/history.tssrc/commands/logs/push/history.tssrc/commands/logs/push/subscribe.tssrc/commands/logs/subscribe.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/reactions/remove.tssrc/commands/rooms/messages/reactions/send.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/messages/subscribe.tssrc/commands/rooms/occupancy/get.tssrc/commands/rooms/reactions/send.tssrc/commands/rooms/typing/keystroke.tssrc/commands/rooms/typing/subscribe.tssrc/commands/spaces/cursors/set.tssrc/commands/version.tssrc/control-base-command.tssrc/flags.tssrc/services/config-manager.tssrc/spaces-base-command.tssrc/types/cli.tstest/helpers/mock-config-manager.tstest/unit/base/auth-info-display.test.tstest/unit/base/base-command.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/apps/list.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/auth/keys/create.test.tstest/unit/commands/auth/revoke-token.test.tstest/unit/commands/bench/bench.test.tstest/unit/commands/bench/benchmarking.test.tstest/unit/commands/channels/batch-publish.test.tstest/unit/commands/channels/history.test.tstest/unit/commands/channels/list.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/interactive-autocomplete.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/spaces/spaces.test.ts
💤 Files with no reviewable changes (1)
- test/unit/commands/bench/bench.test.ts
There was a problem hiding this comment.
Pull request overview
Streamlines Ably CLI global flag handling by centralizing flag definitions and moving authentication inputs from CLI flags to environment variables and stored config (with per-account endpoint support).
Changes:
- Introduces
src/flags.tsto centralize shared flag sets (coreGlobalFlags,productApiFlags,controlApiFlags) and updates commands/base classes to use them. - Removes direct CLI flag auth overrides (e.g.,
--api-key,--access-token,--token) in favor of env vars (ABLY_API_KEY,ABLY_ACCESS_TOKEN,ABLY_TOKEN) + config resolution. - Adds per-account
endpointpersistence in config and updates unit tests + command examples accordingly.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/spaces/spaces.test.ts | Updates spaces tests to rely on config/env auth instead of --api-key. |
| test/unit/commands/queues/list.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/queues/delete.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/queues/create.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/interactive-autocomplete.test.ts | Updates autocomplete expectations to reflect the new global/base flag set (e.g., verbose/web-cli-help). |
| test/unit/commands/channels/subscribe.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/channels/presence/subscribe.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/channels/presence/enter.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/channels/list.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/channels/history.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/channels/batch-publish.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/bench/benchmarking.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/bench/bench.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/auth/revoke-token.test.ts | Removes --api-key usage; relies on config/env auth. |
| test/unit/commands/auth/keys/create.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/apps/update.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/apps/list.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var. |
| test/unit/commands/apps/delete.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/commands/apps/create.test.ts | Switches access-token test to ABLY_ACCESS_TOKEN env var and cleans env after tests. |
| test/unit/base/base-command.test.ts | Updates base-command tests for endpoint/auth behavior (env + config manager getEndpoint). |
| test/unit/base/auth-info-display.test.ts | Aligns “hide account info” behavior with env-var based auth (ABLY_TOKEN, ABLY_API_KEY, ABLY_ACCESS_TOKEN). |
| test/helpers/mock-config-manager.ts | Adds endpoint storage/retrieval to the mock ConfigManager. |
| src/types/cli.ts | Updates BaseFlags typing to reflect the streamlined/public flag surface. |
| src/spaces-base-command.ts | Uses productApiFlags as the global flag set for Spaces commands. |
| src/chat-base-command.ts | Uses productApiFlags as the global flag set for Chat commands. |
| src/flags.ts | New centralized flag definitions and composites for product vs control API commands. |
| src/base-command.ts | Refactors auth resolution to env-vars/config + introduces shared auth helper (applyApiKeyAuth) + endpoint resolution via env/config. |
| src/control-base-command.ts | Control commands now use controlApiFlags and resolve access token from env/config (no CLI override). |
| src/services/config-manager.ts | Adds per-account endpoint support (getEndpoint/storeEndpoint) to config manager + TOML persistence. |
| src/commands/version.ts | Uses coreGlobalFlags rather than the previous broader global flag set. |
| src/commands/config/show.ts | Uses coreGlobalFlags. |
| src/commands/config/path.ts | Uses coreGlobalFlags. |
| src/commands/config/index.ts | Uses coreGlobalFlags. |
| src/commands/logs/subscribe.ts | Uses productApiFlags for product API command flag surface. |
| src/commands/logs/history.ts | Uses productApiFlags for product API command flag surface. |
| src/commands/logs/push/subscribe.ts | Uses productApiFlags (and custom flags) for product API log subscription. |
| src/commands/logs/push/history.ts | Uses productApiFlags for product API command flag surface. |
| src/commands/logs/connection-lifecycle/subscribe.ts | Uses productApiFlags for product API command flag surface. |
| src/commands/logs/connection-lifecycle/history.ts | Uses productApiFlags for product API command flag surface. |
| src/commands/logs/channel-lifecycle/subscribe.ts | Uses productApiFlags (and custom flags) for product API log subscription. |
| src/commands/connections/test.ts | Uses productApiFlags. |
| src/commands/channels/subscribe.ts | Uses productApiFlags and updates examples to env-var auth. |
| src/commands/channels/publish.ts | Uses productApiFlags and updates examples to env-var auth. |
| src/commands/channels/presence/subscribe.ts | Uses productApiFlags + clientIdFlag and updates examples to env-var auth. |
| src/commands/channels/presence/enter.ts | Uses productApiFlags + clientIdFlag and updates examples to env-var auth. |
| src/commands/channels/occupancy/subscribe.ts | Uses productApiFlags and updates examples to env-var auth. |
| src/commands/channels/occupancy/get.ts | Uses productApiFlags and updates examples to env-var auth. |
| src/commands/channels/list.ts | Uses productApiFlags. |
| src/commands/channels/inspect.ts | Uses productApiFlags plus hidden control flags for mixed product/control behavior. |
| src/commands/channels/history.ts | Uses productApiFlags. |
| src/commands/channels/batch-publish.ts | Uses productApiFlags. |
| src/commands/bench/subscriber.ts | Uses productApiFlags. |
| src/commands/bench/publisher.ts | Uses productApiFlags. |
| src/commands/auth/revoke-token.ts | Uses productApiFlags. |
| src/commands/auth/issue-jwt-token.ts | Uses productApiFlags and updates examples to ABLY_TOKEN=... pattern. |
| src/commands/auth/issue-ably-token.ts | Uses productApiFlags and updates examples to ABLY_TOKEN=... pattern. |
| src/commands/apps/update.ts | Updates examples to ABLY_ACCESS_TOKEN=... pattern. |
| src/commands/apps/delete.ts | Updates examples to ABLY_ACCESS_TOKEN=... pattern. |
| src/commands/apps/create.ts | Updates examples to ABLY_ACCESS_TOKEN=... pattern. |
| src/commands/accounts/login.ts | Adds --endpoint support (stored in account config) and adopts new flag composition. |
| src/commands/accounts/switch.ts | Adds --endpoint support (stored in account config) and adopts new flag composition. |
| src/commands/spaces/cursors/set.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/typing/subscribe.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/typing/keystroke.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/reactions/send.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/occupancy/get.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/messages/subscribe.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/messages/send.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/messages/reactions/send.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/messages/reactions/remove.ts | Updates examples to ABLY_API_KEY=... pattern. |
| src/commands/rooms/messages/history.ts | Updates examples to ABLY_API_KEY=... pattern. |
Comments suppressed due to low confidence (2)
src/commands/logs/channel-lifecycle/subscribe.ts:24
productApiFlagsalready providesjson/pretty-jsonwith mutual exclusivity. Overridingjsonhere removes theexclusiveconstraint from the base flag definition, so--jsonand--pretty-jsoncan be supplied together. Either rely on the sharedjsonflag fromproductApiFlags, or add back theexclusive: ["pretty-json"]constraint in this override.
static override flags = {
...productApiFlags,
json: Flags.boolean({
default: false,
description: "Output results as JSON",
}),
src/commands/logs/push/subscribe.ts:23
productApiFlagsalready definesjson/pretty-jsonas mutually exclusive global flags. Re-definingjsonhere overrides that definition and drops theexclusiveconstraint, allowing--jsonand--pretty-jsonto be passed together. Consider removing this localjsonflag override, or re-adding theexclusive: ["pretty-json"]constraint so behavior stays consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
PR description isn't exactly true... the commit message is more accurate:
We already had env vars for access token and api key... I've introduced token since we've removed --token usage. We've not positioned this as the prefered way of auth. |
DX-786: Replaced monolithic globalFlags (16 flags on every command) with composable flag groups. Utility commands like `ably version` now only show --verbose/--json. Auth flags (--api-key, --access-token, --token) removed entirely — auth is via `ably login` or env vars (ABLY_API_KEY, ABLY_TOKEN, ABLY_ACCESS_TOKEN). Moved --endpoint to login/switch only, stored per-account in config. Removed --env and --host. Kept --client-id on presence commands.
umair-ably
force-pushed
the
DX-786-global-commands
branch
from
a3cc7fa to
07dc49c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 85 out of 86 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/commands/logs/push/subscribe.ts:23
productApiFlagsalready defines--jsonwith mutual exclusivity against--pretty-json. Redefiningjsonhere overrides that and allows both flags at once, which can lead to ambiguous output formatting. Consider removing this override and relying onproductApiFlags, or re-addingexclusive: ["pretty-json"]to the localjsonflag definition.
src/commands/logs/channel-lifecycle/subscribe.ts:24productApiFlagsalready defines--jsonas mutually exclusive with--pretty-json. This localjsonredefinition overrides that behavior, so users could pass both flags simultaneously. Consider removing the duplicatejsonflag or keeping the exclusivity (exclusive: ["pretty-json"]) when overriding.
static override flags = {
...productApiFlags,
json: Flags.boolean({
default: false,
description: "Output results as JSON",
}),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sacOO7
reviewed
Mar 5, 2026
| **Auth in tests — use environment variables, NEVER CLI flags:** | ||
| ```typescript | ||
| // ❌ WRONG — --api-key, --token, --access-token are NOT CLI flags | ||
| runCommand(["channels", "publish", "my-channel", "hello", "--api-key", key]); |
Contributor
There was a problem hiding this comment.
@coderabbitai what do you think about using runCommand(["channels", "publish", "my-channel", "hello", "--api-key", key]); vs
runCommand(["channels", "publish", "my-channel", "hello"], {
env: { ABLY_API_KEY: key },
});
Since ably-cli is mainly a cli tool, its easier to pass access-token directly right? Do you think both options should be provided
There was a problem hiding this comment.
@sacOO7, great question — this is essentially a security vs. usability tradeoff, and the PR has made a deliberate architectural call. Here's the reasoning behind each side:
Why flags (--api-key) feel natural for a CLI tool
- Discoverability — flags appear in
--helpoutput, making auth self-documenting - Explicit — no need to know about env var conventions upfront
- Easy for one-off commands —
ably channels publish my-channel hello --api-key app.key:secretis concise
Why the PR chose env vars only
- Security — CLI flags appear in shell history (
~/.bash_history,~/.zsh_history) and in the process list (ps aux), potentially leaking secrets - Cleaner flag surface — removing auth flags from every command reduces noise (previously 16 global flags on every command)
- CI/CD alignment — env vars are the standard way secrets are injected in pipelines
- Consistency —
ably loginstores credentials once; env vars override for scripting. There's no need for a third mechanism
Should both be supported?
Supporting both would directly conflict with the PR's stated goal:
"Authentication flags (
--api-key,--access-token,--token) are removed from command-level flags; auth is handled viaably loginor environment variables"
Adding --api-key back as a flag on individual commands would reintroduce the very problem this PR is solving — proliferating auth flags across all commands.
If discoverability is a concern, a middle ground worth considering (without reverting the design) would be to mention env vars in the --help output or in a dedicated ably auth --help section. But supporting both as equivalent CLI options would undermine the security and UX simplification goals of this PR.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/unit/commands/interactive-autocomplete.test.ts (1)
692-701: Misleading comment: verbose, json, and pretty-json are not fromproductApiFlags.Line 692's comment states "base flags from productApiFlags," but
json,pretty-json,verbose, andweb-cli-helpare typically from global/base flags, notproductApiFlags. The actualproductApiFlags(port, tlsPort, tls) are correctly identified in the section starting at line 698.📝 Suggested comment fix
- // Check for base flags from productApiFlags + // Check for base/global flags expect(batchPublish.flags).toHaveProperty("json"); expect(batchPublish.flags).toHaveProperty("pretty-json"); expect(batchPublish.flags).toHaveProperty("verbose"); expect(batchPublish.flags).toHaveProperty("web-cli-help"); - // Check for product API flags (hidden) + // Check for productApiFlags (port, tlsPort, tls) expect(batchPublish.flags).toHaveProperty("port"); expect(batchPublish.flags).toHaveProperty("tlsPort"); expect(batchPublish.flags).toHaveProperty("tls");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/interactive-autocomplete.test.ts` around lines 692 - 701, Update the misleading test comment: change the comment that reads "base flags from productApiFlags" to accurately state that json, pretty-json, verbose, and web-cli-help are base/global flags (not from productApiFlags), and keep a separate comment noting that productApiFlags are the hidden flags checked via batchPublish.flags for "port", "tlsPort", and "tls"; reference batchPublish.flags and productApiFlags in the updated comment to make the distinction clear.test/e2e/stats/stats.test.ts (1)
68-71: Consider deduplicating the repeated token env object.A shared constant would reduce repetition and make future auth-env changes safer.
♻️ Proposed refactor
const E2E_ACCESS_TOKEN = process.env.E2E_ABLY_ACCESS_TOKEN; +const ACCESS_TOKEN_ENV = { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! }; ... - const result = await runCommand(["stats", "account"], { - timeoutMs: 60000, - env: { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! }, - }); + const result = await runCommand(["stats", "account"], { + timeoutMs: 60000, + env: ACCESS_TOKEN_ENV, + }); ... - env: { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! }, + env: ACCESS_TOKEN_ENV,Also applies to: 91-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/stats/stats.test.ts` around lines 68 - 71, The test repeats the env object with ABLY_ACCESS_TOKEN and E2E_ACCESS_TOKEN! in multiple runCommand calls; extract a single shared constant (e.g., const authEnv = { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! }) near the top of the test file and replace the inline env: { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! } in each runCommand invocation (references: runCommand and E2E_ACCESS_TOKEN) so future auth-env changes are centralized and DRY.test/unit/commands/channels/list.test.ts (1)
70-70: Add a regression assertion that--api-keyis no longer exposed.You removed
--api-keyfrom invocations, but there isn’t a direct assertion guarding the new CLI surface.Suggested test addition
describe("flags", () => { + it("should not expose --api-key flag", async () => { + const { stdout } = await runCommand( + ["channels:list", "--help"], + import.meta.url, + ); + + expect(stdout).not.toContain("--api-key"); + }); + it("should accept --limit flag", async () => {As per coding guidelines "Update or add tests when new features or changes are made, ensuring all tests pass before deeming work complete."
Also applies to: 88-88, 99-99, 108-108, 117-117, 126-126, 139-139, 163-163, 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/list.test.ts` at line 70, Add a regression assertion in the channels:list unit tests to verify the CLI no longer exposes the deprecated --api-key flag: after calling runCommand(["channels:list"], import.meta.url) assert that stdout (or the command help/output captured) does not include the string "--api-key" (e.g., expect(stdout).not.toContain("--api-key")). Update the corresponding test cases that call runCommand (references: the test file test/unit/commands/channels/list.test.ts and the runCommand invocations around lines referenced in the review) so each invocation includes this negative assertion to guard the new CLI surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/base-command.ts`:
- Around line 588-594: The code currently constructs a truncatedToken from
process.env.ABLY_TOKEN (tokenFromEnv / truncatedToken) and prints it; remove any
creation or display of truncatedToken and stop exposing token fragments in auth
info output. Instead, change the auth-info branch that checks tokenFromEnv to
not include token material—use a non-sensitive indicator (e.g., "token: present"
or "token: redacted") or a boolean flag, and remove references to truncatedToken
and tokenFromEnv from any logging/return values in the same function/logic that
builds auth info.
In `@src/commands/channels/publish.ts`:
- Around line 38-39: Remove the clientIdFlag spread from the flags for the
channels publish command: locate the flags array/assignment where
...productApiFlags and ...clientIdFlag are spread (the channels publish command
definition) and delete the ...clientIdFlag entry so --client-id is not available
on publish; leave all other flags (e.g., productApiFlags) unchanged.
In `@src/services/config-manager.ts`:
- Around line 377-387: storeEndpoint writes accounts[alias].endpoint but
storeAccount later overwrites the entire account object and drops endpoint;
update storeAccount to preserve any existing endpoint (e.g., read const
existingEndpoint = this.config.accounts[alias]?.endpoint and include endpoint:
existingEndpoint when reconstructing the account) or merge the incoming account
data with the existing account instead of replacing it wholesale so stored
endpoints are not lost.
---
Nitpick comments:
In `@test/e2e/stats/stats.test.ts`:
- Around line 68-71: The test repeats the env object with ABLY_ACCESS_TOKEN and
E2E_ACCESS_TOKEN! in multiple runCommand calls; extract a single shared constant
(e.g., const authEnv = { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN! }) near the top of
the test file and replace the inline env: { ABLY_ACCESS_TOKEN: E2E_ACCESS_TOKEN!
} in each runCommand invocation (references: runCommand and E2E_ACCESS_TOKEN) so
future auth-env changes are centralized and DRY.
In `@test/unit/commands/channels/list.test.ts`:
- Line 70: Add a regression assertion in the channels:list unit tests to verify
the CLI no longer exposes the deprecated --api-key flag: after calling
runCommand(["channels:list"], import.meta.url) assert that stdout (or the
command help/output captured) does not include the string "--api-key" (e.g.,
expect(stdout).not.toContain("--api-key")). Update the corresponding test cases
that call runCommand (references: the test file
test/unit/commands/channels/list.test.ts and the runCommand invocations around
lines referenced in the review) so each invocation includes this negative
assertion to guard the new CLI surface.
In `@test/unit/commands/interactive-autocomplete.test.ts`:
- Around line 692-701: Update the misleading test comment: change the comment
that reads "base flags from productApiFlags" to accurately state that json,
pretty-json, verbose, and web-cli-help are base/global flags (not from
productApiFlags), and keep a separate comment noting that productApiFlags are
the hidden flags checked via batchPublish.flags for "port", "tlsPort", and
"tls"; reference batchPublish.flags and productApiFlags in the updated comment
to make the distinction clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d8dd185-b64b-4a20-8c13-605fce878e69
📒 Files selected for processing (86)
.claude/CLAUDE.mdREADME.mdsrc/base-command.tssrc/chat-base-command.tssrc/commands/accounts/login.tssrc/commands/accounts/switch.tssrc/commands/apps/create.tssrc/commands/apps/delete.tssrc/commands/apps/update.tssrc/commands/auth/issue-ably-token.tssrc/commands/auth/issue-jwt-token.tssrc/commands/auth/revoke-token.tssrc/commands/bench/publisher.tssrc/commands/bench/subscriber.tssrc/commands/channels/batch-publish.tssrc/commands/channels/history.tssrc/commands/channels/inspect.tssrc/commands/channels/list.tssrc/commands/channels/occupancy/get.tssrc/commands/channels/occupancy/subscribe.tssrc/commands/channels/presence/enter.tssrc/commands/channels/presence/subscribe.tssrc/commands/channels/publish.tssrc/commands/channels/subscribe.tssrc/commands/config/index.tssrc/commands/config/path.tssrc/commands/config/show.tssrc/commands/connections/test.tssrc/commands/logs/channel-lifecycle/subscribe.tssrc/commands/logs/connection-lifecycle/history.tssrc/commands/logs/connection-lifecycle/subscribe.tssrc/commands/logs/history.tssrc/commands/logs/push/history.tssrc/commands/logs/push/subscribe.tssrc/commands/logs/subscribe.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/reactions/remove.tssrc/commands/rooms/messages/reactions/send.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/messages/subscribe.tssrc/commands/rooms/occupancy/get.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/presence/subscribe.tssrc/commands/rooms/reactions/send.tssrc/commands/rooms/typing/keystroke.tssrc/commands/rooms/typing/subscribe.tssrc/commands/spaces/cursors/get-all.tssrc/commands/spaces/cursors/set.tssrc/commands/spaces/cursors/subscribe.tssrc/commands/spaces/locations/set.tssrc/commands/spaces/locations/subscribe.tssrc/commands/spaces/locks/acquire.tssrc/commands/spaces/locks/subscribe.tssrc/commands/spaces/members/enter.tssrc/commands/spaces/members/subscribe.tssrc/commands/version.tssrc/control-base-command.tssrc/flags.tssrc/services/config-manager.tssrc/spaces-base-command.tssrc/types/cli.tstest/e2e/bench/bench.test.tstest/e2e/connections/connections.test.tstest/e2e/stats/stats.test.tstest/helpers/mock-config-manager.tstest/unit/base/auth-info-display.test.tstest/unit/base/base-command.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/apps/list.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/auth/keys/create.test.tstest/unit/commands/auth/revoke-token.test.tstest/unit/commands/bench/bench.test.tstest/unit/commands/bench/benchmarking.test.tstest/unit/commands/channels/batch-publish.test.tstest/unit/commands/channels/history.test.tstest/unit/commands/channels/list.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/interactive-autocomplete.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/spaces/spaces.test.ts
💤 Files with no reviewable changes (1)
- test/unit/commands/bench/bench.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/commands/rooms/messages/reactions/send.ts
🚧 Files skipped from review as they are similar to previous changes (40)
- test/unit/commands/channels/presence/subscribe.test.ts
- src/commands/apps/delete.ts
- src/commands/logs/push/subscribe.ts
- test/unit/commands/queues/create.test.ts
- src/commands/rooms/messages/reactions/remove.ts
- src/commands/bench/subscriber.ts
- src/commands/channels/occupancy/subscribe.ts
- src/commands/rooms/reactions/send.ts
- src/commands/auth/revoke-token.ts
- test/unit/commands/bench/benchmarking.test.ts
- test/unit/commands/channels/subscribe.test.ts
- src/commands/logs/connection-lifecycle/subscribe.ts
- src/commands/spaces/cursors/set.ts
- src/commands/config/show.ts
- src/commands/channels/presence/subscribe.ts
- src/commands/apps/update.ts
- src/commands/connections/test.ts
- src/commands/logs/subscribe.ts
- src/commands/accounts/login.ts
- src/commands/channels/occupancy/get.ts
- test/unit/commands/apps/delete.test.ts
- src/commands/rooms/typing/keystroke.ts
- src/commands/rooms/occupancy/get.ts
- test/unit/commands/apps/update.test.ts
- src/commands/apps/create.ts
- src/commands/logs/push/history.ts
- test/unit/commands/apps/create.test.ts
- src/chat-base-command.ts
- test/unit/commands/auth/keys/create.test.ts
- src/commands/accounts/switch.ts
- src/commands/config/index.ts
- src/commands/rooms/messages/subscribe.ts
- test/unit/commands/queues/list.test.ts
- test/helpers/mock-config-manager.ts
- test/unit/commands/queues/delete.test.ts
- test/unit/commands/apps/list.test.ts
- src/commands/bench/publisher.ts
- src/commands/channels/list.ts
- src/commands/version.ts
- src/commands/rooms/messages/history.ts
sacOO7
reviewed
Mar 5, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
BTW, while checking the flow for ably login =>
- It opens the browser and prompts for an
access-token. - After entering the access token, there doesn’t seem to be a way to log out.
So, I had a few questions:
- Can we add
logoutcommand that revokes the Control APIaccess token? - Would it make sense to group both
loginandlogoutunder anauthcommand? For example:ably auth loginably auth logout
- If we decide to keep
loginandlogoutas top-level commands, perhapslogincould appear as the first command andlogoutas the last inably --help?
Contributor
Author
|
Contributor
|
I think overall |
sacOO7
approved these changes
Mar 5, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
LGMT ( PS. seems I missed out on login/logout)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests