Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR fixes an MAU (monthly active user) counting problem where every CLI command minted a fresh random Changes
Review Notes
|
Review: feat(mau): resolve one stable client identity per processThis PR replaces the per-run random Breaking change: JWT tokens without
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues remain in token validation, auth flag handling, benchmark identity consistency, and concurrent default persistence.
Review effort: Lite
Findings: 4
Open (5)
What changed in this PR
This PR gives the CLI a stable client identity across commands, authentication, configuration, and benchmark processes.
Changes:
- Adds identity precedence, validation, and persisted defaults.
- Applies identities to clients, banners, JWTs, and Ably tokens.
- Adds benchmark suffixing, tests, and documentation updates.
| File | Summary |
|---|---|
test/unit/setup.ts |
Resets identity state between tests. |
test/unit/data/env-vars.test.ts |
Updates environment metadata tests. |
test/unit/commands/env.test.ts |
Updates environment command expectations. |
test/unit/base/client-identity.test.ts |
Tests identity resolution behavior. |
test/unit/base/base-command.test.ts |
Updates base-command mocks. |
test/helpers/mock-config-manager.ts |
Adds mock identity support. |
src/services/config-manager.ts |
Persists generated default identities. |
src/services/client-identity.ts |
Resolves and validates client identities. |
src/data/env-vars.ts |
Documents ABLY_CLIENT_ID. |
src/commands/bench/subscriber.ts |
Enables benchmark identity suffixing. |
src/commands/bench/publisher.ts |
Enables benchmark identity suffixing. |
src/commands/auth/issue-jwt-token.ts |
Propagates resolved identity into JWTs. |
src/commands/auth/issue-ably-token.ts |
Propagates resolved identity into Ably tokens. |
src/base-command.ts |
Integrates identity resolution, token handling, and banners. |
docs/Project-Structure.md |
Documents the new service. |
docs/Environment-Variables/General-Usage.md |
Documents the new environment variable. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1633
to
+1637
| const clientId = | ||
| identity.clientId && | ||
| this.suffixClientIdPerProcess && | ||
| identity.source !== "flag" | ||
| ? `${identity.clientId}-${randomUUID().slice(0, 8)}` |
Comment on lines
90
to
95
| tokenParams.clientId = flags["client-id"]; | ||
| } | ||
| } else { | ||
| // Generate a default client ID | ||
| tokenParams.clientId = `ably-cli-${randomUUID().slice(0, 8)}`; | ||
| // Default to the identity the CLI itself acts as | ||
| tokenParams.clientId = this.resolveClientId(flags); | ||
| } |
Comment on lines
+114
to
+118
| clientId = flags["client-id"]; | ||
| } | ||
| } else { | ||
| // Generate a default client ID | ||
| const defaultClientId = `ably-cli-${randomUUID().slice(0, 8)}`; | ||
| jwtPayload["x-ably-clientId"] = defaultClientId; | ||
| clientId = defaultClientId; | ||
| // Default to the identity the CLI itself acts as | ||
| clientId = this.resolveClientId(flags) ?? null; |
Comment on lines
+358
to
+362
| const existing = this.config.client?.defaultId; | ||
| if (existing) return existing; | ||
|
|
||
| const defaultId = generateDefaultClientId(); | ||
| this.config.client = { ...this.config.client, defaultId }; |
ttypic
force-pushed
the
integration/mau-stable-client-id
branch
from
October 5, 2026 18:56
7af5ce9 to
f99dbc9
Compare
umair-ably
reviewed
Oct 8, 2026
umair-ably
left a comment
umair-ably
left a comment
Contributor
There was a problem hiding this comment.
some claude assisted comments for this one:
- A config file that can't be written now breaks every data-plane command — config-manager.ts getDefaultClientId
- On first use, getDefaultClientId calls saveConfig(), which throws Failed to save Ably config if the write fails.
- resolveClientId only catches InvalidClientIdError and re-throws everything else.
- So a CI container or Docker image with a read-only home directory or ABLY_CLI_CONFIG_DIR, and only ABLY_API_KEY set, worked before this PR and now fails on every command. The banner swallows the error; getClientOptions doesn't.
- Fix: if the write fails, keep the generated ID in memory for this process and optionally logWarning once.
- Race on the first write — same method
- When two CLI processes start on a fresh config (parallel CI steps, several bench processes), each generates its own default, and the last write wins. The other process runs under an ID that never gets saved.
- saveConfig also writes the whole in-memory snapshot, so it can overwrite a token refresh another process just saved. That's the exact problem reloadConfig was added to prevent.
- Fix: call reloadConfig(), re-check client.defaultId, then write.
- The bench banner shows a different client ID from the one used
- bannerClientId calls resolveClientIdentity directly, so it ignores suffixClientIdPerProcess.
- bench publisher and bench subscriber print Client=ably-cli-xxxx but actually connect as ably-cli-xxxx-<8 hex>.
- Fix: skip the banner field when the command suffixes the ID, or note that it's suffixed per process.
- The --client-id description is stale — src/flags.ts:78 (not in the diff)
- It still says Use "none" to explicitly set no client ID, which this PR deprecates.
- Fix: drop the "none" sentence and mention that the default is a stable ID per install (or ABLY_CLIENT_ID). After pnpm prepare this also updates the generated docs and README.
- The deprecation message assumes "none" came from the flag
- "Omit the value to use your default client ID" reads oddly when "none" was set via ABLY_CLIENT_ID or the config.
- Fix: use SOURCE_NAMES[identity.source] in the message, as the validation errors already do.
Base automatically changed from
integration/mau-new-packages
to
integration/mau
October 12, 2026 11:38
Every client the CLI built used to mint a random `ably-cli-<uuid8>`, so a 30-command session acted as 30 different clients and left random names in message history and presence. The CLI now resolves its client ID once per process with the precedence --client-id > ABLY_CLIENT_ID > `client.id` in the config > an install default generated once and persisted in the config. Every client, every interactive command and every minted token share it. - Reject "" (which silently fell back to a random ID) and "*" with messages naming where the value came from; deprecate "none". - Under token auth the identity is the token's and is never overridden; a JWT without an `x-ably-clientId` claim is rejected up front. - Show the resolved client ID in the "Using:" banner. - bench suffixes its default identity per process so concurrent bench processes stay under the per-client-ID connection cap. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ttypic
force-pushed
the
integration/mau-stable-client-id
branch
from
October 12, 2026 11:38
f99dbc9 to
e0e9c9c
Compare
- Resolve the acting identity once per command, bench suffix
included, so the banner shows the ID the clients connect as
- Route explicit --client-id on the token commands through the
shared resolver: "none" now warns as deprecated, and "*" is
allowed so wildcard tokens can still be minted. With an explicit
--client-id the issuing client acts as the token's identity, so
the default identity is never resolved
- Under a wildcard ("*") JWT, act as --client-id, ABLY_CLIENT_ID or
the default client ID, since the token lets its holder choose
- Keep the generated default ID in memory if the config can't be
written, instead of failing every command
- Name the client.id config setting correctly in errors, say which
source a deprecated "none" came from, and update the --client-id
descriptions
Tests cover the banner matching the suffixed ID, wildcard tokens
when minting and connecting, source names in messages, and the
default ID's first-write and read-only-config cases.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Author
|
@umair-ably fixed 1,3,4,5. For 2 I think race is something not very harmful and expected. We can come up with very sophisticated locking mechanism, but I think it will be over engineering |
This branch was successfully deployed
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.
Every client the CLI built used to mint a random
ably-cli-<uuid8>, so a30-command session acted as 30 different clients and left random names
in message history and presence.
The CLI now resolves its client ID once per process with the precedence
--client-id > ABLY_CLIENT_ID >
client.idin the config > an installdefault generated once and persisted in the config. Every client, every
interactive command and every minted token share it.
messages naming where the value came from; deprecate "none".
a JWT without an
x-ably-clientIdclaim is rejected up front.processes stay under the per-client-ID connection cap.