Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR replaces the monolithic Changes
Review Notes
|
There was a problem hiding this comment.
Summary
This PR migrates the CLI from the ably package to @ably/pubsub-server + @ably/pubsub-device + @ably/pubsub-core, introducing MAU client-side classification. The factory module is clean and well-tested. The Spaces dynamic-import hack is gone (a real reliability win). The bulk of the command-file changes are mechanical type swaps that look correct.
One real bug to fix before users hit it, and one minor edge-case gap to be aware of.
src/base-command.ts + src/commands/auth/issue-jwt-token.ts — Bug: error hint references a non-existent flag
Line 458 of base-command.ts:
"ably auth issue-jwt-token --client-type server --client-id <id>"
auth issue-jwt-token has no --client-type flag, and its JwtPayload interface does not include x-ably-clientType. A user who hits this error and tries to follow the hint will get Error: Unexpected argument: --client-type.
The test in test/unit/base/device-http-client.test.ts validates that the error message contains "--client-type server", which locks in the incorrect hint.
Fix: Either add --client-type server|device to auth issue-jwt-token (making the hint accurate — this seems like the intended end state) or change the error message to give actionable advice that works today (e.g., "Use an API key for server-side access, or issue a JWT token that includes the x-ably-clientType: server claim.").
src/base-command.ts — Minor gap: device-REST guard only covers ABLY_TOKEN
The early device-REST rejection (line 453–462) checks process.env.ABLY_TOKEN:
if (
clientType === "rest" &&
resolveClientSide({ token: process.env.ABLY_TOKEN }) === "device"
) {
this.fail("...helpful hint...", flags, "client");
}If ABLY_TOKEN is not set but the resolved clientOptions (from stored config or ABLY_API_KEY) contains a device-side JWT, the check passes, resolveClientSide(clientOptions) later returns "device", createPubSubHttpClient throws "A device-side client cannot be an HTTP client.", and the raw error propagates through the catch block to the user — no actionable hint.
In practice this scenario is unlikely (CLI config stores API keys, not device JWTs), so I'm not flagging it as a blocker. But it's worth noting: the check could be moved to after clientOptions is resolved to cover all auth sources consistently.
Everything else looks good
- Factory module (
src/services/ably-client-factory.ts): clean separation, pure functions, well-covered by the new unit tests.jwt.decode(no verify) is correct here — the backend enforces signature validity; the CLI only needs to read the claim for routing. - Spaces client creation: replacing the fragile
getSpacesConstructor()dynamic import withcreateSpacesClient()is a meaningful reliability improvement. connections/test.ts: callingcreatePubSubRealtimeClient(options)directly (withoutside) is correct — the factory derivessidefromoptionsand device-side realtime is allowed.test/unit/base/device-http-client.test.ts: the third test ("run for a token with the server claim") relies on the mock REST client; confirmed thatgetMockAblyRest()defaultshistorytomockResolvedValue(createMockPaginatedResult([])), so the test is sound.- Canary package versions: expected for the
integration/mautarget branch.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unsupported recovery command and E2E helper factory bypass remain unresolved.
Review effort: Lite
Findings: 1
What changed in this PR
This PR migrates the CLI from ably v2 to split Pub/Sub packages and centralizes server/device client construction for MAU classification.
Changes:
- Adds server/device client factories and device-side HTTP protection.
- Updates Chat, Spaces, commands, tests, and E2E helpers.
- Replaces dependencies and updates documentation.
| File | Summary |
|---|---|
test/unit/services/ably-client-factory.test.ts |
Tests client classification and factory construction. |
test/unit/base/device-http-client.test.ts |
Tests device-side HTTP rejection. |
test/unit/base/base-command-enhanced.test.ts |
Updates base-command coverage for the new client behavior. |
test/setup.ts |
Updates shared client mocks, types, and cleanup. |
test/helpers/mock-ably-realtime.ts |
Migrates realtime mocks to Pub/Sub packages. |
test/helpers/e2e-test-helper.ts |
Creates E2E clients; review note: a construction path bypasses the shared factory. |
test/helpers/ably-event-emitter.ts |
Updates event emitter integration types. |
test/e2e/push/publish-e2e.test.ts |
Migrates publish E2E client usage. |
test/e2e/push/devices-e2e.test.ts |
Migrates device E2E client usage. |
test/e2e/push/channels-e2e.test.ts |
Migrates channel push E2E client usage. |
test/e2e/channels/channels-e2e.test.ts |
Migrates channel E2E client usage. |
src/utils/output.ts |
Migrates SDK utility types. |
src/utils/message.ts |
Migrates message types. |
src/utils/history.ts |
Migrates history types. |
src/spaces-base-command.ts |
Constructs Spaces clients with the shared realtime client. |
src/services/ably-client-factory.ts |
Adds server/device client selection and construction. |
src/commands/spaces/occupancy/subscribe.ts |
Migrates Spaces occupancy client types. |
src/commands/logs/subscribe.ts |
Migrates log subscription types. |
src/commands/logs/push/subscribe.ts |
Migrates push log subscription types. |
src/commands/logs/connection-lifecycle/subscribe.ts |
Migrates connection lifecycle types. |
src/commands/logs/channel-lifecycle/subscribe.ts |
Migrates channel lifecycle types. |
src/commands/connections/test.ts |
Uses the realtime factory for connection tests. |
src/commands/channels/update.ts |
Migrates channel client types. |
src/commands/channels/subscribe.ts |
Migrates channel subscription types. |
src/commands/channels/publish.ts |
Migrates channel publish types. |
src/commands/channels/presence/subscribe.ts |
Migrates presence subscription types. |
src/commands/channels/presence/enter.ts |
Migrates presence entry types. |
src/commands/channels/occupancy/subscribe.ts |
Migrates occupancy subscription types. |
src/commands/channels/history.ts |
Migrates channel history types. |
src/commands/channels/get-message.ts |
Migrates message retrieval types. |
src/commands/channels/delete.ts |
Migrates channel deletion types. |
src/commands/channels/append.ts |
Migrates channel append types. |
src/commands/channels/annotations/subscribe.ts |
Migrates annotation subscription types. |
src/commands/channels/annotations/publish.ts |
Migrates annotation publishing types. |
src/commands/channels/annotations/get.ts |
Migrates annotation retrieval types. |
src/commands/channels/annotations/delete.ts |
Migrates annotation deletion types. |
src/commands/bench/subscriber.ts |
Migrates benchmark subscriber types. |
src/commands/bench/publisher.ts |
Migrates benchmark publisher types. |
src/commands/auth/issue-ably-token.ts |
Migrates token issuance SDK types. |
src/chat-base-command.ts |
Constructs Chat clients with the shared realtime client. |
src/base-command.ts |
Routes client creation through factories and rejects device HTTP clients; recovery references an unsupported JWT client-type option. |
pnpm-lock.yaml |
Updates the dependency lockfile. |
package.json |
Replaces the legacy Ably dependency with split packages. |
docs/Testing.md |
Updates testing and mock documentation. |
docs/Project-Structure.md |
Documents the client factory architecture. |
AGENTS.md |
Documents Pub/Sub package conventions. |
.claude/skills/ably-new-command/SKILL.md |
Updates command-development SDK guidance. |
.claude/skills/ably-new-command/references/patterns.md |
Updates SDK usage patterns. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resolveClientSide({ token: process.env.ABLY_TOKEN }) === "device" | ||
| ) { | ||
| this.fail( | ||
| `This command uses Ably's HTTP API, which only server-side clients can use, and the token in ABLY_TOKEN classifies the CLI as a device. Use an API key, or a server-scoped token: "ably auth issue-jwt-token --client-type server --client-id <id>".`, |
There was a problem hiding this comment.
implemented later in the stack anyway #461, I think we should keep it
Replace the `ably` dependency with `@ably/pubsub-server`, the split v3 package for servers, so every client the CLI builds declares itself a server: server traffic is exempt from MAU counting. The CLI always connects as a server, whatever it authenticates with. Types come from the same package, which re-exports the core's type surface. `createAblyRestClient()` / `createAblyRealtimeClient()` build clients with its `createHttpClient` / `createRealtimeClient`. `@ably/chat` and `@ably/spaces` move to their canaries built on the v3 core (chat 0.0.0-canary.20261001T1209.783b80e6, spaces 0.0.0-canary.20261001T1221.ca08adce), constructed with `createChatClient` and `createSpacesClient` around the CLI's own realtime client, so rooms and spaces traffic is server traffic too. Nothing depends on `ably` v2 any more. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3b0d890 to
9fed11c
Compare
Replace the
ablydependency with@ably/pubsub-server, the split v3package for servers, so every client the CLI builds declares itself a
server: server traffic is exempt from MAU counting. The CLI always
connects as a server, whatever it authenticates with. Types come from
the same package, which re-exports the core's type surface.
createAblyRestClient()/createAblyRealtimeClient()build clientswith its
createHttpClient/createRealtimeClient.@ably/chatand@ably/spacesmove to their canaries built on the v3core (chat 0.0.0-canary.20261001T1209.783b80e6, spaces
0.0.0-canary.20261001T1221.ca08adce), constructed with
createChatClientand
createSpacesClientaround the CLI's own realtime client, so roomsand spaces traffic is server traffic too. Nothing depends on
ablyv2any more.