Conversation
- Add docs/Client-Identity.md: server/device classification by auth mode, the client ID precedence, values the CLI refuses, target vs identity --client-id, and the multi-user demo pattern (two terminals with --client-id for a demo, a client-scoped token to reproduce device behaviour exactly). - Rewrite the --client-id help, the token --client-id help and the ABLY_API_KEY / ABLY_TOKEN env-var text, which presented `--client-id none` as a normal option and described per-run random client IDs. - Correct the rule in AGENTS.md, the ably-new-command skill and the src/flags.ts comment that client identity is irrelevant for read-only commands: under MAU billing reads carry the session's client ID and are counted; they just have no reason to act as someone else. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR corrects a subtle but important misconception in docs, code comments, and flag descriptions: the claim that client identity is "irrelevant" for read-only commands. In reality, every CLI command carries a client ID (and Ably counts it under MAU billing) — read-only commands simply have no reason to act as a different client, so Changes
Review Notes
|
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: docs(mau): document client identity and correct the read-only rule
Overview: Pure documentation PR — adds a new docs/Client-Identity.md, updates skill files to correct the rationale for omitting clientIdFlag from read-only commands, and sharpens flag/env-var descriptions throughout. No functional logic changes. I checked all factual claims against the implementation.
Verified accurate
- JWT rejection before connecting (line 1731-1736 of
base-command.ts) — confirmed viathis.fail()call "none"is deprecated but still functional with a warning (line 1652-1656 ofbase-command.ts) — confirmed- Empty string
""and"*"are rejected viaInvalidClientIdError— confirmed insrc/services/client-identity.ts - Classification table (API key → server; JWT with
x-ably-clientType: server→ server; other tokens → device) — matchesably-client-factory.ts
One concern worth addressing
docs/Client-Identity.md line 73 — self-contradictory MAU claim
Both run under the server classification, so they are not counted or capped as devices. Ably counts client IDs wherever they appear, including in message payloads, so each simulated name may still register as an MAU.
The first sentence correctly states server-classified traffic is not counted as device MAU. The second sentence says the same names "may still register as an MAU," which directly contradicts that. More specifically, the phrase "including in message payloads" implies Ably scans message content to count MAU — that's not how Ably billing works. MAU is connection-based, not payload-scanning.
If the intent is to say "server-classified connections still appear in Ably's analytics with those client IDs" that's a different (and narrower) claim. If the intent is to warn users that server classification isn't a billing escape hatch, the rationale needs to be grounded in the actual billing mechanic. As written, it could cause readers to incorrectly believe they're accumulating MAU charges when they're not, or to make unnecessary architectural changes.
Suggested fix: either drop the second sentence (the first already covers the important point), or replace it with a specific, accurate statement about what Ably does track for server-classified traffic.
Minor
The DXRFC-029 reference in the opening paragraph is an internal document external contributors can't look up. Fine to leave as context for rationale, just worth knowing it will be opaque to OSS contributors.
Otherwise the doc is well-structured, the code-behaviour alignment is accurate, and the skill/AGENTS updates correctly capture the new rationale.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Documentation and help text still contain inconsistencies around deprecated none, command-local client IDs, and native token identity handling.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Documents client identity and MAU classification while correcting CLI help, environment-variable guidance, and read-only command instructions.
Changes:
- Adds client identity and classification documentation.
- Updates client-ID and token help text.
- Corrects identity guidance across project documentation and skills.
| File | Description |
|---|---|
src/flags.ts |
Updates client-ID guidance. |
src/data/env-vars.ts |
Corrects authentication identity guidance. |
src/commands/auth/issue-jwt-token.ts |
Updates token help. |
src/commands/auth/issue-ably-token.ts |
Updates token help. |
docs/Environment-Variables/General-Usage.md |
Links client identity documentation. |
docs/Client-Identity.md |
Adds identity and MAU documentation. |
AGENTS.md |
Corrects read-only identity guidance. |
.claude/skills/ably-new-command/SKILL.md |
Updates command-generation guidance. |
.claude/skills/ably-new-command/references/patterns.md |
Updates get-command guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `ABLY_TOKEN` holding a JWT with `x-ably-clientType: server` | `@ably/pubsub-server` | Server | | ||
| | `ABLY_TOKEN` holding any other JWT, or a native Ably token | `@ably/pubsub-device` | Device | | ||
|
|
||
| The device package has no HTTP client, so commands that use Ably's HTTP API (`channels publish`, `channels history`, `push ...` and others) fail under a device-classified token, pointing you at an API key or a server-scoped token. Realtime commands such as `channels subscribe` work on either side. |
| Under API key auth, the client ID is resolved once per process, from the first of: | ||
|
|
||
| 1. `--client-id <id>`, on commands that offer it | ||
| 2. `ABLY_CLIENT_ID` | ||
| 3. `client.id` in the config file (`~/.ably/config`) | ||
| 4. A default generated once per install (`ably-cli-<8 hex chars>`) and saved as `client.defaultId` in the config | ||
|
|
||
| Every client the command builds, every command in an interactive session, and every token minted by `ably auth issue-ably-token` / `issue-jwt-token` without `--client-id` uses that one ID. Commands without a `--client-id` flag still act as it. `ably bench` commands append a per-process suffix to the default, so that many concurrent bench processes stay under the per-client-ID connection cap. |
| Values the CLI refuses: | ||
|
|
||
| - `""` — an empty client ID. It used to fall back silently to a random ID. | ||
| - `"*"` — the wildcard. The CLI acts as, and issues tokens to, one concrete client ID. | ||
| - `"none"` — deprecated. It still acts with no client ID, with a warning, but apps that require identified clients reject that traffic. |
| - `"*"` — the wildcard. The CLI acts as, and issues tokens to, one concrete client ID. | ||
| - `"none"` — deprecated. It still acts with no client ID, with a warning, but apps that require identified clients reject that traffic. | ||
|
|
||
| Under token auth the client ID is the token's and is never overridden; `--client-id` is ignored with a warning. A JWT without an `x-ably-clientId` claim is rejected before connecting. |
Add docs/Client-Identity.md: server/device classification by auth
mode, the client ID precedence, values the CLI refuses, target vs
identity --client-id, and the multi-user demo pattern (two terminals
with --client-id for a demo, a client-scoped token to reproduce device
behaviour exactly).
ABLY_API_KEY / ABLY_TOKEN env-var text, which presented
--client-id noneas a normal option and described per-run randomclient IDs.
src/flags.ts comment that client identity is irrelevant for read-only
commands: under MAU billing reads carry the session's client ID and
are counted; they just have no reason to act as someone else.