Add proxy integration tests and missing UTS test coverage#2216
Add proxy integration tests and missing UTS test coverage#2216paddybyers wants to merge 22 commits intouts-integrationfrom
Conversation
Sandbox HTTP requests (app provisioning and deletion) now have a 30s AbortSignal timeout. Previously these had no timeout, so a stuck sandbox request would silently consume the entire mocha suite timeout. Teardown deletion is also wrapped in try/catch since it's best-effort cleanup — sandbox apps auto-expire. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Suite timeouts must accommodate the sum of all tests plus setup and teardown. 60s was too tight for suites with many tests under sandbox load, causing spurious failures when individual operations stalled. Individual operations already have their own bounded timeouts (30s for sandbox HTTP, 10-15s for connection waits), so the suite timeout now serves as a generous outer bound. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test was using client-side Date.now() to define time boundaries between "early" and "late" message batches. This failed reliably because publishes completed within the same millisecond, making the boundary meaningless. The test now retrieves server-assigned timestamps from the messages and derives the boundary from those. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move all unit tests under realtime/unit/ and rest/unit/ directories to mirror the specification repo's top-level unit vs integration breakdown. Update import paths and describe() names accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test proxy is a Go binary that sits between the SDK and the Ably sandbox, forwarding WebSocket and HTTP traffic while allowing programmable fault injection. Each test creates a proxy session with rules that match specific protocol messages and apply actions: suppress, replace, inject, delay, or refuse connections. The proxy also supports imperative actions (e.g. disconnect the current WebSocket) and exposes an event log for post-hoc verification of traffic patterns. Infrastructure: - helpers/proxy.ts: TypeScript client for the proxy's REST control API. Exports createProxySession(), ProxySession (addRules, triggerAction, getLog, close), and waitForProxy(). - helpers/run-proxy-tests.sh: Builds the Go proxy if needed, starts it, runs mocha against proxy/**/*.test.ts, and kills it on exit. - package.json: test:uts excludes proxy/ tests (they require the proxy binary); test:uts:proxy runs them via run-proxy-tests.sh. Tests (19 test cases across 5 files): - connection_open_failures (RTN14a/b/c/d/g): Fatal errors, token errors, timeouts, and connection refused during the opening handshake. - connection_resume (RTN15a/b/c6/c7/h1/h3): Disconnect/resume, resume preserving connectionId, failed resume with new id, token vs non-token DISCONNECTED errors. - heartbeat (RTN23a): Suppress server frames to starve heartbeats, verify idle timeout fires and SDK reconnects. - channel_faults (RTL4f/5f/13a/14): Attach timeout, detach timeout, unsolicited DETACHED triggering reattach, channel ERROR causing FAILED. - rest_faults (RSC10/RSC15m/REC2c2/RTL6): Token renewal on HTTP 401, 503 with fallback disabled, end-to-end publish+history passthrough. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 new proxy integration tests matching the UTS specs: connection_resume.test.ts (3 new tests): - Test 21 (RTN15j): Fatal ERROR mid-session → FAILED state + channels FAILED - Test 22 (RTN15g/g2): connectionStateTtl expiry prevents resume - Test 23 (RTN19a/a2): Unacked messages resent after successful resume channel_faults.test.ts (2 new tests): - Test 24 (RTL12): ATTACHED with resumed=false triggers channel UPDATE event - Test 25 (RTL3d): Both channels reattach after connection recovery auth_reauth.test.ts (1 new test): - Test 26 (RTN22/RTC8a): Server-initiated AUTH triggers re-authentication presence_reentry.test.ts (1 new test): - Test 27 (RTP17i/RTP17g): Presence auto re-enter on non-resumed ATTACHED Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New test files: - RTB1: backoff coefficient and jitter validation - RSA4a: token expiry with non-renewable tokens (FAILED state) - RSA4c/d/f: auth callback errors (timeout, 403, invalid type, oversize) - RTN16: connection recovery key, msgSerial, channelSerials - RTN20: network change events (skipped — browser-only in Node.js) - RSF1/RTF1: forwards compatibility for unknown fields/actions - RSH7: push channel subscriptions (unit + integration, integration skipped) Extended test files: - RTL22/MFI: message filter subscriptions (name, refTimeserial, clientId) - RTN7e: publish error matches connection.errorReason - CHD2/CHM2: all ChannelMetrics fields including objectPublishers/Subscribers - RTN16d/RTN16l: proxy-based two-phase recovery and recovery failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
proxy.ts now builds and spawns the Go proxy binary automatically via ensureProxy() — no need to start it externally before running tests. The proxy is built on first use (skipped if binary is up to date), polled via /health until ready, and killed on process exit. Removes the separate test:uts:proxy script; proxy tests now run as part of the standard test:uts suite. The test:uts:unit script excludes both proxy/ and integration/ directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new npm test script, reorganizes and renames many UTS unit tests, introduces a TypeScript proxy helper that downloads and controls a Go-based uts-proxy, increases many integration timeouts, tightens sandbox request timeouts and teardown, adjusts mock HTTP fallback/timeout behavior, and adds many new proxy-driven integration tests and REST/unit test suites. UTS Test & Proxy Changes
sequenceDiagram
participant Client as Client (Ably SDK)
participant ProxyCtrl as Proxy Control API
participant ProxyProc as uts-proxy process
participant Sandbox as Sandbox REST/API
participant GitHub as GitHub Releases
Note over ProxyHelper,GitHub: Proxy helper downloads/releases binary
ProxyHelper->>GitHub: request release tarball
GitHub-->>ProxyHelper: tarball
ProxyHelper->>ProxyProc: extract & spawn binary (chmod + exec)
ProxyProc->>Sandbox: forward proxied ws/http traffic
Client->>ProxyProc: connect / send frames (ws/http)
ProxyCtrl->>ProxyProc: /sessions, rules, actions (create/trigger/log)
Test->>ProxyCtrl: createProxySession / addRules / triggerAction
ProxyCtrl-->>Test: sessionId, events, logs
🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…uccess Tests the race condition where a slow request to a cached fallback host completes after fallbackRetryTimeout has expired. The late success must not re-establish the fallback as the preferred host. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
test/uts/realtime/unit/channels/channel_connection_state.test.ts (1)
689-689:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TypeScript compilation errors in mock connectionDetails.
The pipeline is failing because
respond_with_connected()receives incompleteconnectionDetailsobjects. TypeScript requires connectionKey, clientId, maxIdleInterval, maxMessageSize, and serverId properties.🔧 Proposed fix: Provide minimal required properties
For line 689:
conn.respond_with_connected({ - connectionDetails: { connectionStateTtl: 5000 }, + connectionDetails: { + connectionKey: 'test-key', + clientId: 'test-client', + maxIdleInterval: 15000, + maxMessageSize: 65536, + serverId: 'test-server', + connectionStateTtl: 5000 + }, });For line 758:
conn.respond_with_connected({ - connectionDetails: { connectionStateTtl: 5000 }, + connectionDetails: { + connectionKey: 'test-key', + clientId: 'test-client', + maxIdleInterval: 15000, + maxMessageSize: 65536, + serverId: 'test-server', + connectionStateTtl: 5000 + }, });Alternatively, if the mock framework supports it, consider adjusting the type signature to accept
Partial<ConnectionDetails>.Also applies to: 758-758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/unit/channels/channel_connection_state.test.ts` at line 689, The mock passed to respond_with_connected() uses an incomplete connectionDetails object; update the test's connectionDetails in channel_connection_state.test (the respond_with_connected() calls) to include the required ConnectionDetails properties: connectionKey, clientId, maxIdleInterval, maxMessageSize, and serverId (in addition to connectionStateTtl), or change the mock type to Partial<ConnectionDetails> if the framework supports it; ensure the updated objects satisfy TypeScript by providing minimal valid values for connectionKey, clientId, maxIdleInterval, maxMessageSize, and serverId wherever connectionDetails is constructed (e.g., the respond_with_connected calls around the failing cases).test/uts/rest/unit/request.test.ts (1)
434-448:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
expect.failinsidetryis swallowed by thecatch— the test always passes.If
client.request(...)succeeds (no throw), execution reachesexpect.fail(...). That throws a ChaiAssertionError, which is immediately caught by thecatch (error: any)block.expect(error).to.existthen evaluates theAssertionErroras truthy and the test passes — despite the SUT behaving incorrectly. The test gives false confidence.🛡️ Proposed fix: re-throw AssertionErrors so the failure surfaces correctly
- try { - await client.request('GET', '/test', 3, null as any, null as any, null as any); - expect.fail('Expected request to throw on connection refused'); - } catch (error: any) { - expect(error).to.exist; - } + let threwNetworkError = false; + try { + await client.request('GET', '/test', 3, null as any, null as any, null as any); + } catch (error: any) { + threwNetworkError = true; + expect(error).to.exist; + } + expect(threwNetworkError, 'Expected request to throw on connection refused').to.be.true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/unit/request.test.ts` around lines 434 - 448, The test's catch block is swallowing assertion failures from expect.fail because it catches every error; update the test around the call to client.request(...) so assertion errors are re-thrown: detect Chai/AssertionError (e.g. error.name === 'AssertionError' or instanceof Chai.AssertionError) inside the catch and rethrow it, otherwise assert that a non-assertion error exists; adjust the catch in the "Network error handling - connection refused" test that surrounds client.request to rethrow assertion errors and only run expect(error).to.exist for genuine runtime errors.test/uts/realtime/unit/connection/connection_failures.test.ts (1)
250-262:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTypeScript error TS2345:
erroris not a known property ofrespond_with_connected's parameter type — blocks all CI pipelines.Every CI job fails at this line because
respond_with_connected's expected type does not include anerrorfield. TheconnectionDetailssub-object already works around this withas any; applying the same cast to the entire argument is the minimal fix.🐛 Proposed fix — cast whole argument `as any`
- conn.respond_with_connected({ - connectionId: 'connection-2', - connectionDetails: { - connectionKey: 'key-2', - maxIdleInterval: 15000, - connectionStateTtl: 120000, - } as any, - error: { - code: 80008, - statusCode: 400, - message: 'Unable to recover connection', - }, - }); + conn.respond_with_connected({ + connectionId: 'connection-2', + connectionDetails: { + connectionKey: 'key-2', + maxIdleInterval: 15000, + connectionStateTtl: 120000, + }, + error: { + code: 80008, + statusCode: 400, + message: 'Unable to recover connection', + }, + } as any);Alternatively, if
respond_with_connectedinMockWebSocketis meant to support an optionalerrorfield (which the spec requires for failed-resume CONNECTED messages), widening its parameter type there is the cleaner long-term fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/unit/connection/connection_failures.test.ts` around lines 250 - 262, The call to respond_with_connected is passing an object with an unexpected error field which causes TS2345; fix by casting the entire argument to any (e.g., pass the object as any) so TypeScript accepts the extra error property, or alternatively update the MockWebSocket.respond_with_connected parameter type to include an optional error field if you prefer the long-term type-safe solution; target the call site in connection_failures.test.ts where respond_with_connected is invoked and/or the MockWebSocket.respond_with_connected signature.test/uts/realtime/unit/presence/realtime_presence_get.test.ts (1)
66-69:⚠️ Potential issue | 🔴 CriticalType the
resultparameter instead of refactoring to avoid the implicit-any error.The TS7006 error on lines 66 and 136 is real and occurs because
resultlacks an explicit type. However, the suggested refactoring is incorrect—it removes thereturn result;statement, which would causegetFutureto resolve toundefinedrather than thePresenceMessage[]array. This breaks the code on lines 86 and 169, wheremembersis immediately used (e.g.,members.length,members.map()).The correct fix is to type the parameter explicitly:
- const getFuture = channel.presence.get().then((result) => { + const getFuture = channel.presence.get().then((result: PresenceMessage[]) => { getResolved = true; return result; });Apply the same fix to the second occurrence (lines 136–139).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/unit/presence/realtime_presence_get.test.ts` around lines 66 - 69, The promise handler for channel.presence.get (assigned to getFuture) uses an untyped parameter `result`, causing TS7006; explicitly type that parameter as PresenceMessage[] in the .then callback for both occurrences (the one creating getFuture and the second similar .then around lines 136–139) so the handler remains `return result;` and getFuture resolves to the PresenceMessage[] array used later (e.g., members.length, members.map()). Ensure you update the parameter type in the .then callbacks for the functions that call channel.presence.get() without removing the returned result.test/uts/rest/unit/fallback.test.ts (1)
519-530:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
countBeforeis captured but never asserted — caching test is incomplete.The test verifies the hostname of the last captured request but never checks that the second
client.time()call made only one request (i.e., went straight to the cached fallback without retrying the primary). If the cache is broken and the SDK falls back to the same host by chance (~20% per run), the test silently passes. ThecountBeforevariable was clearly intended for this assertion.🛡️ Proposed fix
const countBefore = requestCount; await client.time(); + // Exactly one request should have been made — straight to cached fallback, no primary retry + expect(requestCount - countBefore).to.equal(1); + // The second request should use the cached fallback host const secondRequestHost = captured[captured.length - 1].url.hostname; expect(secondRequestHost).to.equal(fallbackHost);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/unit/fallback.test.ts` around lines 519 - 530, The test captures countBefore but never asserts it; update the caching test to assert that only one new request was made after the second client.time() call by comparing requestCount to countBefore + 1 (use the existing variables countBefore and requestCount), ensuring the SDK used the cached fallback and did not retry the primary; keep the existing hostname assertion (secondRequestHost === fallbackHost) and add the numeric assertion immediately after the second client.time() call.test/uts/rest/integration/push_admin.test.ts (1)
160-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TS2339:
deviceTokendoes not exist onPushRecipient
PushRecipientis a union type;deviceTokenonly exists on the APNS-specific variant. Accessing it directly on the union causes TS2339 across all 8 CI jobs. Castrecipienttoanyin the two assertions.🐛 Proposed fix
- expect(updated.push!.recipient!.deviceToken).to.equal('token-v2'); + expect((updated.push!.recipient! as any).deviceToken).to.equal('token-v2'); // Verify via get const retrieved = await client.push.admin.deviceRegistrations.get(deviceId); - expect(retrieved.push!.recipient!.deviceToken).to.equal('token-v2'); + expect((retrieved.push!.recipient! as any).deviceToken).to.equal('token-v2');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/integration/push_admin.test.ts` around lines 160 - 166, Type error TS2339 occurs because PushRecipient is a union and deviceToken exists only on the APNS variant; update the two assertions that access updated.push!.recipient!.deviceToken and retrieved.push!.recipient!.deviceToken by casting recipient to any (e.g., (updated.push!.recipient as any).deviceToken and (retrieved.push!.recipient as any).deviceToken) so the compiler accepts the property access while preserving the assertions that the token equals 'token-v2'.test/uts/rest/integration/mutable_messages.test.ts (1)
144-158:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TS18047:
pollUntilresults inferred asT | nullacross five tests
pollUntilusesnullas the "not ready" sentinel in callbacks, so TypeScript infers its return type asT | null. SincepollUntilthrows on timeout (never returns null on success), the results can be safely asserted non-null at the declaration site. The same pattern causes TS18047 for:
updatedMsg– lines 154–158deletedMsg– line 196versions– lines 237, 240annotations– lines 309, 312result– lines 366, 368🐛 Proposed fix pattern (apply to all five `pollUntil` calls)
- const updatedMsg = await pollUntil( + const updatedMsg = (await pollUntil( async () => { const msg = await channel.getMessage(serial); if (msg.action === 'message.update') return msg; return null; }, { interval: 500, timeout: 10000 }, - ); + ))!;Apply the same
(await pollUntil(...))!pattern todeletedMsg(line 187),versions(line 228),annotations(line 300), andresult(line 357).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/integration/mutable_messages.test.ts` around lines 144 - 158, The pollUntil callback returns T | null, causing TS18047 downstream; for each polling call (pollUntil(...)) where the test expects a non-null successful result, assert non-null at the declaration by adding a non-null assertion to the awaited value (e.g., change the assignments to use (await pollUntil(...))! ). Apply this to the variables updatedMsg, deletedMsg, versions, annotations, and result so their types are treated as the successful T rather than T | null while keeping pollUntil usage and callbacks unchanged.test/uts/rest/integration/pagination.test.ts (2)
53-59:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TS2554:
channel.history()requires one argumentThe Ably SDK's TypeScript types require a params object; calling
history()with zero arguments fails compilation. This occurs inpollUntilcallbacks at lines 55, 98, 148, 192, and 246.🐛 Proposed fix (apply at all five occurrences)
- const result = await channel.history(); + const result = await channel.history({});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/integration/pagination.test.ts` around lines 53 - 59, Calls to channel.history() inside the pollUntil callbacks are failing TypeScript because Ably's history method requires a params object; update each callback to call channel.history({}) instead of channel.history() (the occurrences are the pollUntil callbacks checking result.items.length === 15 at the five locations), i.e., replace channel.history() with channel.history({}) so the TypeScript signature is satisfied while preserving the existing result handling in those pollUntil functions.
104-121:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TS18047/TS2322/TS2345: null-safety errors from
PaginatedResult.next()
next()returnsPaginatedResult<T> | null. Three distinct errors stem from this:
- Lines 106 & 110 –
page2andpage3are typedPaginatedResult | null; chaining.next()/ accessing.itemson them is TS18047.- Line 115 – iterating
[page1, page2, page3]where elements can benullis TS18047.- Line 117 –
item.idisstring | undefined; pushing intostring[]is TS2345.- Line 156 – same
page2null pattern for.first().- Line 209 – reassigning
pageinside the while-loop withawait page.next()fails TS2322 (PaginatedResult | nullnot assignable toPaginatedResult).🐛 Proposed fixes
- const page2 = await page1.next(); - const page3 = await page2.next(); + const page2 = (await page1.next())!; + const page3 = (await page2.next())!;- allIds.push(item.id); + allIds.push(item.id!);At line 156:
- const page2 = await page1.next(); + const page2 = (await page1.next())!;At line 209:
- page = await page.next(); + page = (await page.next())!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/integration/pagination.test.ts` around lines 104 - 121, The failures come from treating PaginatedResult.next() as always returning a value and from item.id possibly being undefined; update calls to PaginatedResult.next() (page2 = await page1.next(), page3 = await page2.next(), the .first() call, and the while-loop reassignment) to explicitly null-check their results (e.g., if a next()/first() call returns null, throw or break the test) so subsequent .next()/.items access is on a non-null PaginatedResult; also narrow/validate item.id before using it (e.g., assert existence with expect(item.id).to.exist or skip items with undefined id) and in the while-loop assign the awaited next value to a new variable (e.g., const nextPage = await page.next()) then check nextPage !== null before assigning back to page to satisfy the PaginatedResult versus null type.test/uts/rest/unit/channel/publish.test.ts (1)
437-459:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing deviation skip guard in
RSL1ltest — inconsistent with other deviation tests in this file.Both
RSL1etests (lines 114–115 and 144–145) useif (!process.env.RUN_DEVIATIONS) this.skip()before exercising the deviation. TheRSL1ltest has the same inline deviation comment but no skip guard, so it will always run and may produce a confusing assertion failure ifRestChannel.publish()doesn't accept extra params.🛡️ Proposed fix
it('RSL1l - publish params as querystring', async function () { + // DEVIATION: see deviations.md + if (!process.env.RUN_DEVIATIONS) this.skip(); const captured: any[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/unit/channel/publish.test.ts` around lines 437 - 459, The RSL1l test lacks the deviation skip guard and should be skipped unless deviations are enabled; inside the 'RSL1l - publish params as querystring' it block (the test that calls Ably.Rest, client.channels.get and ch.publish), add the same guard used by the RSL1e tests: at the start of the test body insert "if (!process.env.RUN_DEVIATIONS) this.skip();" so the assertions about query params only run when RUN_DEVIATIONS is set.test/uts/realtime/unit/presence/presence_map.test.ts (1)
29-36:⚠️ Potential issue | 🔴 CriticalRemove the
0argument fromnew Logger()constructor call — it takes no parameters.The
Loggerconstructor insrc/common/lib/util/logger.tsis defined with zero parameters (constructor() { ... }). Passing0as an argument causesTS2554: Expected 0 arguments, but got 1and breaks the build.Proposed fix
function createMockPresence(): any { - const logger = new Logger(0); + const logger = new Logger(); return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/unit/presence/presence_map.test.ts` around lines 29 - 36, The test helper createMockPresence is instantiating Logger with an invalid argument; remove the numeric argument passed to new Logger() so it matches the Logger constructor signature (use new Logger() inside createMockPresence) to resolve TS2554; update the reference in createMockPresence to const logger = new Logger() and keep the rest of the returned mock unchanged.test/uts/rest/unit/auth/token_details.test.ts (1)
285-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
RUN_DEVIATIONSguard on a known-deviation assertion.The comment on line 287 explicitly labels this as a deviation ("ably-js may keep the stale token"), yet the test lacks the guard used by every other deviation test in the same file (lines 139, 172, 284, etc.). If the SDK retains the stale token,
expect(client.auth.tokenDetails).to.be.nullwill fail unconditionally in CI, not just in deviation-mode runs.🛠️ Proposed fix
// Spec (RSA16d): after failed renewal, tokenDetails MUST be null. // DEVIATION: ably-js may keep the stale token. See deviations.md. expect(callbackCount).to.equal(2); + if (!process.env.RUN_DEVIATIONS) this.skip(); expect(client.auth.tokenDetails).to.be.null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/unit/auth/token_details.test.ts` around lines 285 - 290, This test asserts client.auth.tokenDetails must be null after failed renewal but omits the RUN_DEVIATIONS guard used elsewhere; wrap the tokenDetails assertion in the same deviation check (using the RUN_DEVIATIONS flag) so the expectation on client.auth.tokenDetails is only enforced when deviations are not allowed (preserve the existing expect(callbackCount).to.equal(2) behavior and conditionally run expect(client.auth.tokenDetails).to.be.null under the RUN_DEVIATIONS guard).test/uts/realtime/unit/presence/local_presence_map.test.ts (1)
18-28:⚠️ Potential issue | 🔴 CriticalFix
Loggerconstructor call — blocks all CI pipelines.
new Logger(0)fails withTS2554: Expected 0 arguments, but got 1at line 28. TheLoggerconstructor insrc/common/lib/util/logger.tstakes no arguments. Remove the0argument.🐛 Proposed fix
function createMockPresence(): any { - const logger = new Logger(0); + const logger = new Logger(); return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/unit/presence/local_presence_map.test.ts` around lines 18 - 28, The test's mock presence is calling the Logger constructor with an unexpected argument; in createMockPresence replace the instantiation new Logger(0) with new Logger() so the Logger constructor (from Logger) is invoked with no arguments; ensure createMockPresence continues to supply presence.channel.name, presence._synthesizeLeaves and presence.syncComplete for PresenceMap's constructor.test/uts/realtime/integration/sandbox.ts (1)
9-9:⚠️ Potential issue | 🔴 CriticalBlocking CI failure: JSON module import cannot be resolved due to uninitialised git submodule.
All five CI pipelines fail with
TS2307: Cannot find module '../../../common/ably-common/test-resources/test-app-setup.json'. The root cause is that theably-commongit submodule attest/common/ably-commonis not initialised in the CI checkout, leaving the directory empty.The
resolveJsonModule: trueflag is already correctly set in the roottsconfig.jsonand coverstest/**/*, so TypeScript can resolve JSON imports once the submodule is initialised. The import path../../../common/ably-common/test-resources/test-app-setup.jsoncorrectly resolves totest/common/ably-common/test-resources/test-app-setup.jsonwhen the submodule is present.Ensure the CI pipeline initialises submodules during checkout (e.g.,
git submodule update --init --recursive).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/integration/sandbox.ts` at line 9, The CI failure is caused by an uninitialised git submodule so the JSON import in test/uts/realtime/integration/sandbox.ts (importing testAppSetup from '../../../common/ably-common/test-resources/test-app-setup.json') cannot be resolved; fix the pipeline checkout step to initialise submodules (for example by adding a command like git submodule update --init --recursive to your CI job before TypeScript compilation) so the test/common/ably-common directory is populated and resolveJsonModule can load the JSON.test/uts/rest/integration/history.test.ts (1)
48-69:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
TS18047: 'history' is possibly 'null'errors in RSL2a are also blocking CI.
pollUntilreturnsT | null, sohistoryis typed asPaginatedResult<Message> | null. All property accesses on lines 53–66 (history.items, etc.) are flagged asTS18047. Add a non-null assertion after theawaitto match the intent thatpollUntilthrows or resolves on timeout.🐛 Proposed fix for RSL2a (line 48)
- const history = await pollUntil(async () => { - const result = await channel.history(); - return result.items.length === 3 ? result : null; - }, { interval: 500, timeout: 10000 }); + const history = (await pollUntil(async () => { + const result = await channel.history({}); + return result.items.length === 3 ? result : null; + }, { interval: 500, timeout: 10000 }))!;Apply the same
(await pollUntil(...))!pattern to theRSL2b1andRSL2b2pollUntilusages if they also assign to a variable used downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/integration/history.test.ts` around lines 48 - 69, The variable history is typed as PaginatedResult<Message> | null because pollUntil returns T | null, causing TS18047; fix by asserting non-null on the awaited result where history is assigned (wrap the await pollUntil(...) with a non-null assertion operator, e.g. (await pollUntil(...))!) so subsequent accesses like history.items, history.items[0].name, and history.items[x].timestamp are valid; also apply the same (await pollUntil(...))! pattern to the other pollUntil usages referenced (RSL2b1 and RSL2b2) if they similarly assign to variables used later.
🧹 Nitpick comments (2)
test/uts/rest/unit/types/error_types.test.ts (1)
51-59: 💤 Low valueDuplicate
TI5label across two separate test cases.Both "TI5 - cause attribute" (Line 51) and "TI5 - nested error cause" (Line 126) are tagged
TI5. The nested-cause test is a separate scenario and should carry a distinct label (e.g.,TI5bor just drop the spec tag from the extra test) to avoid ambiguity in test reports.Also applies to: 126-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/unit/types/error_types.test.ts` around lines 51 - 59, The two tests using the same spec tag "TI5" ("TI5 - cause attribute" and "TI5 - nested error cause") must be disambiguated; locate the failing test description string "TI5 - nested error cause" in the unit test suite (the it(...) call that constructs nested ErrorInfo with a cause) and change its label to a unique tag such as "TI5b - nested error cause" (or remove the TI5 tag) so test reports no longer show duplicate identifiers; ensure the it() description is updated only (keep the test body, assertions and use of Ably.ErrorInfo.fromValues intact).test/uts/realtime/integration/proxy/connection_resume.test.ts (1)
25-49: ⚡ Quick winClean up the waiter listeners on the timeout path.
Both helpers only detach their listeners on success. If a wait times out, the stale listener stays registered and can bleed later state changes into subsequent assertions in this suite. A shared cleanup path for both resolve and reject will make these tests much less flaky.
Also applies to: 52-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/integration/proxy/connection_resume.test.ts` around lines 25 - 49, The waiters (e.g., waitForState) currently remove the listener only on success, leaving the listener registered if the timeout triggers; update waitForState (and the sibling helper at the other block) to always remove the listener and clear the timer on both resolve and reject—capture the listener and timer in scope and ensure you call client.connection.off(listener) and clearTimeout(timer) in the timeout path (and any early-return success path) or use a finally-style cleanup so the listener is detached regardless of outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/uts/realtime/integration/proxy/connection_open_failures.test.ts`:
- Around line 112-114: The assertions read client.connection.errorReason
properties without narrowing a nullable type; extract const err =
client.connection.errorReason, assert expect(err).to.not.be.null (or use
expect(err).to.exist), then access guarded properties as
expect(err!.code).to.equal(40005) and expect(err!.statusCode).to.equal(400);
apply the same pattern for the other occurrence that checks errorReason later in
the file.
In `@test/uts/realtime/integration/proxy/connection_resume.test.ts`:
- Around line 871-876: The assertion is comparing the proxy 'recover' query
param to the wrong value; update the test so the ws_connect log entry's
queryParams 'recover' is asserted against the recoveryKey created by
client1.connection.createRecoveryKey(), not originalConnectionKey. Locate the
check using wsConnects[0].queryParams and replace the expected value reference
from originalConnectionKey to recoveryKey (the variable produced earlier in the
test) so the test verifies the actual recovery payload passed to client2.
- Around line 309-311: Extract client.connection.errorReason into a local
variable and check it for null before accessing properties: replace direct
assertions like expect(client.connection.errorReason).to.not.be.null;
expect(client.connection.errorReason.code).to.equal(80008); with a guarded
pattern that assigns const err = client.connection.errorReason; assert err is
not null (or fail the test) and then assert err.code === 80008. Apply this
change for all occurrences referencing client.connection.errorReason (the blocks
around the assertions at the noted locations).
In `@test/uts/realtime/integration/proxy/rest_faults.test.ts`:
- Around line 229-231: The test accesses publishedMsg.data after using
history.items.find which can return undefined, causing TS18048; update the test
around the publishedMsg variable (the result of history.items.find) to narrow
the type before accessing .data — for example, add an explicit runtime/type
guard such as expect(publishedMsg).to.not.be.undefined or
expect(publishedMsg).to.exist and then either use a non-null assertion
(publishedMsg!.data) or reassign to a typed variable (const msg = publishedMsg
as { data: string }) before calling .data to satisfy TypeScript; ensure the
assertion and subsequent access both reference the publishedMsg identifier.
- Around line 101-103: The assertion is using positional indexing on
httpResponses which is brittle; instead filter responses by the same request
path used for httpRequests so you target the proxy's publish response only.
Update the httpResponses assignment (the const httpResponses = log.filter(...))
to include a path check (matching the same path string or variable used when
filtering httpRequests) and then assert on the first element of that filtered
array; reference the httpResponses variable and the existing log.filter call to
locate where to add the path predicate.
In `@test/uts/realtime/unit/connection/backoff_jitter.test.ts`:
- Around line 271-277: The test is passing an internal-only option
channelRetryTimeout into the Ably.Realtime constructor which is not part of the
public ClientOptions type, causing TS2345; update the Ably.Realtime
instantiation (the object passed to new Ably.Realtime in this test where
channelRetryTimeout and disconnectedRetryTimeout are set) to cast the options
object to any (e.g. via an as any cast) so the internal option is allowed,
leaving trackClient unchanged.
In `@test/uts/realtime/unit/presence/presence_sync.test.ts`:
- Around line 20-23: In createMockPresence(), remove the incorrect numeric
argument passed to the Logger constructor: change the instantiation in the
createMockPresence function from new Logger(0) to new Logger() so it matches the
Logger class signature (constructor()) and resolves the TS2554 error; update any
other test helpers in this file that call new Logger(...) to use no-argument
construction as well.
In `@test/uts/rest/integration/history.test.ts`:
- Around line 151-152: The 2 ms delay is too short to guarantee distinct server
timestamps causing intermittent test failures (when maxEarlyTs === minLateTs),
so increase the sleep in the test where the Promise/setTimeout is used to a much
larger value (e.g., 50–100 ms) so maxEarlyTs and minLateTs will almost always
differ; update the await new Promise((r) => setTimeout(r, 2)) call to use ~100
ms and keep the existing assertions that check timeBoundary, lateHistory, and
expect(lateHistory.items.length).to.be.at.least(1).
- Around line 158-162: Remove the explicit any[] annotation and use a non-null
assertion on the pollUntil result, and call channel.history with an empty
options object; specifically, change the polling block so you assign the result
without typing (let allMessages = await pollUntil(... )!) and inside the
callback call channel.history({}) instead of channel.history(), and apply the
same channel.history({}) change to the other calls to channel.history in this
file (the calls referenced around lines where history is used).
---
Outside diff comments:
In `@test/uts/realtime/integration/sandbox.ts`:
- Line 9: The CI failure is caused by an uninitialised git submodule so the JSON
import in test/uts/realtime/integration/sandbox.ts (importing testAppSetup from
'../../../common/ably-common/test-resources/test-app-setup.json') cannot be
resolved; fix the pipeline checkout step to initialise submodules (for example
by adding a command like git submodule update --init --recursive to your CI job
before TypeScript compilation) so the test/common/ably-common directory is
populated and resolveJsonModule can load the JSON.
In `@test/uts/realtime/unit/channels/channel_connection_state.test.ts`:
- Line 689: The mock passed to respond_with_connected() uses an incomplete
connectionDetails object; update the test's connectionDetails in
channel_connection_state.test (the respond_with_connected() calls) to include
the required ConnectionDetails properties: connectionKey, clientId,
maxIdleInterval, maxMessageSize, and serverId (in addition to
connectionStateTtl), or change the mock type to Partial<ConnectionDetails> if
the framework supports it; ensure the updated objects satisfy TypeScript by
providing minimal valid values for connectionKey, clientId, maxIdleInterval,
maxMessageSize, and serverId wherever connectionDetails is constructed (e.g.,
the respond_with_connected calls around the failing cases).
In `@test/uts/realtime/unit/connection/connection_failures.test.ts`:
- Around line 250-262: The call to respond_with_connected is passing an object
with an unexpected error field which causes TS2345; fix by casting the entire
argument to any (e.g., pass the object as any) so TypeScript accepts the extra
error property, or alternatively update the MockWebSocket.respond_with_connected
parameter type to include an optional error field if you prefer the long-term
type-safe solution; target the call site in connection_failures.test.ts where
respond_with_connected is invoked and/or the
MockWebSocket.respond_with_connected signature.
In `@test/uts/realtime/unit/presence/local_presence_map.test.ts`:
- Around line 18-28: The test's mock presence is calling the Logger constructor
with an unexpected argument; in createMockPresence replace the instantiation new
Logger(0) with new Logger() so the Logger constructor (from Logger) is invoked
with no arguments; ensure createMockPresence continues to supply
presence.channel.name, presence._synthesizeLeaves and presence.syncComplete for
PresenceMap's constructor.
In `@test/uts/realtime/unit/presence/presence_map.test.ts`:
- Around line 29-36: The test helper createMockPresence is instantiating Logger
with an invalid argument; remove the numeric argument passed to new Logger() so
it matches the Logger constructor signature (use new Logger() inside
createMockPresence) to resolve TS2554; update the reference in
createMockPresence to const logger = new Logger() and keep the rest of the
returned mock unchanged.
In `@test/uts/realtime/unit/presence/realtime_presence_get.test.ts`:
- Around line 66-69: The promise handler for channel.presence.get (assigned to
getFuture) uses an untyped parameter `result`, causing TS7006; explicitly type
that parameter as PresenceMessage[] in the .then callback for both occurrences
(the one creating getFuture and the second similar .then around lines 136–139)
so the handler remains `return result;` and getFuture resolves to the
PresenceMessage[] array used later (e.g., members.length, members.map()). Ensure
you update the parameter type in the .then callbacks for the functions that call
channel.presence.get() without removing the returned result.
In `@test/uts/rest/integration/history.test.ts`:
- Around line 48-69: The variable history is typed as PaginatedResult<Message> |
null because pollUntil returns T | null, causing TS18047; fix by asserting
non-null on the awaited result where history is assigned (wrap the await
pollUntil(...) with a non-null assertion operator, e.g. (await pollUntil(...))!)
so subsequent accesses like history.items, history.items[0].name, and
history.items[x].timestamp are valid; also apply the same (await
pollUntil(...))! pattern to the other pollUntil usages referenced (RSL2b1 and
RSL2b2) if they similarly assign to variables used later.
In `@test/uts/rest/integration/mutable_messages.test.ts`:
- Around line 144-158: The pollUntil callback returns T | null, causing TS18047
downstream; for each polling call (pollUntil(...)) where the test expects a
non-null successful result, assert non-null at the declaration by adding a
non-null assertion to the awaited value (e.g., change the assignments to use
(await pollUntil(...))! ). Apply this to the variables updatedMsg, deletedMsg,
versions, annotations, and result so their types are treated as the successful T
rather than T | null while keeping pollUntil usage and callbacks unchanged.
In `@test/uts/rest/integration/pagination.test.ts`:
- Around line 53-59: Calls to channel.history() inside the pollUntil callbacks
are failing TypeScript because Ably's history method requires a params object;
update each callback to call channel.history({}) instead of channel.history()
(the occurrences are the pollUntil callbacks checking result.items.length === 15
at the five locations), i.e., replace channel.history() with channel.history({})
so the TypeScript signature is satisfied while preserving the existing result
handling in those pollUntil functions.
- Around line 104-121: The failures come from treating PaginatedResult.next() as
always returning a value and from item.id possibly being undefined; update calls
to PaginatedResult.next() (page2 = await page1.next(), page3 = await
page2.next(), the .first() call, and the while-loop reassignment) to explicitly
null-check their results (e.g., if a next()/first() call returns null, throw or
break the test) so subsequent .next()/.items access is on a non-null
PaginatedResult; also narrow/validate item.id before using it (e.g., assert
existence with expect(item.id).to.exist or skip items with undefined id) and in
the while-loop assign the awaited next value to a new variable (e.g., const
nextPage = await page.next()) then check nextPage !== null before assigning back
to page to satisfy the PaginatedResult versus null type.
In `@test/uts/rest/integration/push_admin.test.ts`:
- Around line 160-166: Type error TS2339 occurs because PushRecipient is a union
and deviceToken exists only on the APNS variant; update the two assertions that
access updated.push!.recipient!.deviceToken and
retrieved.push!.recipient!.deviceToken by casting recipient to any (e.g.,
(updated.push!.recipient as any).deviceToken and (retrieved.push!.recipient as
any).deviceToken) so the compiler accepts the property access while preserving
the assertions that the token equals 'token-v2'.
In `@test/uts/rest/unit/auth/token_details.test.ts`:
- Around line 285-290: This test asserts client.auth.tokenDetails must be null
after failed renewal but omits the RUN_DEVIATIONS guard used elsewhere; wrap the
tokenDetails assertion in the same deviation check (using the RUN_DEVIATIONS
flag) so the expectation on client.auth.tokenDetails is only enforced when
deviations are not allowed (preserve the existing
expect(callbackCount).to.equal(2) behavior and conditionally run
expect(client.auth.tokenDetails).to.be.null under the RUN_DEVIATIONS guard).
In `@test/uts/rest/unit/channel/publish.test.ts`:
- Around line 437-459: The RSL1l test lacks the deviation skip guard and should
be skipped unless deviations are enabled; inside the 'RSL1l - publish params as
querystring' it block (the test that calls Ably.Rest, client.channels.get and
ch.publish), add the same guard used by the RSL1e tests: at the start of the
test body insert "if (!process.env.RUN_DEVIATIONS) this.skip();" so the
assertions about query params only run when RUN_DEVIATIONS is set.
In `@test/uts/rest/unit/fallback.test.ts`:
- Around line 519-530: The test captures countBefore but never asserts it;
update the caching test to assert that only one new request was made after the
second client.time() call by comparing requestCount to countBefore + 1 (use the
existing variables countBefore and requestCount), ensuring the SDK used the
cached fallback and did not retry the primary; keep the existing hostname
assertion (secondRequestHost === fallbackHost) and add the numeric assertion
immediately after the second client.time() call.
In `@test/uts/rest/unit/request.test.ts`:
- Around line 434-448: The test's catch block is swallowing assertion failures
from expect.fail because it catches every error; update the test around the call
to client.request(...) so assertion errors are re-thrown: detect
Chai/AssertionError (e.g. error.name === 'AssertionError' or instanceof
Chai.AssertionError) inside the catch and rethrow it, otherwise assert that a
non-assertion error exists; adjust the catch in the "Network error handling -
connection refused" test that surrounds client.request to rethrow assertion
errors and only run expect(error).to.exist for genuine runtime errors.
---
Nitpick comments:
In `@test/uts/realtime/integration/proxy/connection_resume.test.ts`:
- Around line 25-49: The waiters (e.g., waitForState) currently remove the
listener only on success, leaving the listener registered if the timeout
triggers; update waitForState (and the sibling helper at the other block) to
always remove the listener and clear the timer on both resolve and
reject—capture the listener and timer in scope and ensure you call
client.connection.off(listener) and clearTimeout(timer) in the timeout path (and
any early-return success path) or use a finally-style cleanup so the listener is
detached regardless of outcome.
In `@test/uts/rest/unit/types/error_types.test.ts`:
- Around line 51-59: The two tests using the same spec tag "TI5" ("TI5 - cause
attribute" and "TI5 - nested error cause") must be disambiguated; locate the
failing test description string "TI5 - nested error cause" in the unit test
suite (the it(...) call that constructs nested ErrorInfo with a cause) and
change its label to a unique tag such as "TI5b - nested error cause" (or remove
the TI5 tag) so test reports no longer show duplicate identifiers; ensure the
it() description is updated only (keep the test body, assertions and use of
Ably.ErrorInfo.fromValues intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a17bb75e-33e6-4c2a-af0c-a24d6cd9d8f7
📒 Files selected for processing (122)
package.jsontest/uts/deviations.mdtest/uts/realtime/integration/auth/token_renewal.test.tstest/uts/realtime/integration/delta_decoding.test.tstest/uts/realtime/integration/helpers/proxy.tstest/uts/realtime/integration/helpers/run-proxy-tests.shtest/uts/realtime/integration/mutable_messages.test.tstest/uts/realtime/integration/presence/presence_lifecycle.test.tstest/uts/realtime/integration/presence/presence_sync.test.tstest/uts/realtime/integration/proxy/auth_reauth.test.tstest/uts/realtime/integration/proxy/channel_faults.test.tstest/uts/realtime/integration/proxy/connection_open_failures.test.tstest/uts/realtime/integration/proxy/connection_resume.test.tstest/uts/realtime/integration/proxy/heartbeat.test.tstest/uts/realtime/integration/proxy/presence_reentry.test.tstest/uts/realtime/integration/proxy/rest_faults.test.tstest/uts/realtime/integration/sandbox.tstest/uts/realtime/unit/auth/auth_callback_errors.test.tstest/uts/realtime/unit/auth/connection_auth.test.tstest/uts/realtime/unit/auth/realtime_authorize.test.tstest/uts/realtime/unit/auth/token_expiry_non_renewable.test.tstest/uts/realtime/unit/channels/channel_additional_attached.test.tstest/uts/realtime/unit/channels/channel_annotations.test.tstest/uts/realtime/unit/channels/channel_attach.test.tstest/uts/realtime/unit/channels/channel_attributes.test.tstest/uts/realtime/unit/channels/channel_connection_state.test.tstest/uts/realtime/unit/channels/channel_delta_decoding.test.tstest/uts/realtime/unit/channels/channel_detach.test.tstest/uts/realtime/unit/channels/channel_error.test.tstest/uts/realtime/unit/channels/channel_get_message.test.tstest/uts/realtime/unit/channels/channel_history.test.tstest/uts/realtime/unit/channels/channel_message_versions.test.tstest/uts/realtime/unit/channels/channel_options.test.tstest/uts/realtime/unit/channels/channel_properties.test.tstest/uts/realtime/unit/channels/channel_publish.test.tstest/uts/realtime/unit/channels/channel_server_initiated_detach.test.tstest/uts/realtime/unit/channels/channel_state_events.test.tstest/uts/realtime/unit/channels/channel_subscribe.test.tstest/uts/realtime/unit/channels/channel_update_delete_message.test.tstest/uts/realtime/unit/channels/channel_when_state.test.tstest/uts/realtime/unit/channels/channels_collection.test.tstest/uts/realtime/unit/channels/message_field_population.test.tstest/uts/realtime/unit/client/client_options.test.tstest/uts/realtime/unit/client/realtime_client.test.tstest/uts/realtime/unit/client/realtime_request.test.tstest/uts/realtime/unit/client/realtime_stats.test.tstest/uts/realtime/unit/client/realtime_time.test.tstest/uts/realtime/unit/client/realtime_timeouts.test.tstest/uts/realtime/unit/connection/auto_connect.test.tstest/uts/realtime/unit/connection/backoff_jitter.test.tstest/uts/realtime/unit/connection/connection_failures.test.tstest/uts/realtime/unit/connection/connection_id_key.test.tstest/uts/realtime/unit/connection/connection_open_failures.test.tstest/uts/realtime/unit/connection/connection_ping.test.tstest/uts/realtime/unit/connection/connection_recovery.test.tstest/uts/realtime/unit/connection/error_reason.test.tstest/uts/realtime/unit/connection/fallback_hosts.test.tstest/uts/realtime/unit/connection/forwards_compatibility.test.tstest/uts/realtime/unit/connection/heartbeat.test.tstest/uts/realtime/unit/connection/network_change.test.tstest/uts/realtime/unit/connection/server_initiated_reauth.test.tstest/uts/realtime/unit/connection/update_events.test.tstest/uts/realtime/unit/connection/when_state.test.tstest/uts/realtime/unit/presence/local_presence_map.test.tstest/uts/realtime/unit/presence/presence_map.test.tstest/uts/realtime/unit/presence/presence_sync.test.tstest/uts/realtime/unit/presence/realtime_presence_channel_state.test.tstest/uts/realtime/unit/presence/realtime_presence_enter.test.tstest/uts/realtime/unit/presence/realtime_presence_get.test.tstest/uts/realtime/unit/presence/realtime_presence_history.test.tstest/uts/realtime/unit/presence/realtime_presence_reentry.test.tstest/uts/realtime/unit/presence/realtime_presence_subscribe.test.tstest/uts/realtime/unit/time.test.tstest/uts/rest/channel/rest_channel_attributes.test.tstest/uts/rest/integration/batch_presence.test.tstest/uts/rest/integration/history.test.tstest/uts/rest/integration/mutable_messages.test.tstest/uts/rest/integration/pagination.test.tstest/uts/rest/integration/presence.test.tstest/uts/rest/integration/push_admin.test.tstest/uts/rest/integration/push_channels.test.tstest/uts/rest/integration/revoke_tokens.test.tstest/uts/rest/unit/auth/auth_callback.test.tstest/uts/rest/unit/auth/auth_scheme.test.tstest/uts/rest/unit/auth/authorize.test.tstest/uts/rest/unit/auth/client_id.test.tstest/uts/rest/unit/auth/revoke_tokens.test.tstest/uts/rest/unit/auth/token_details.test.tstest/uts/rest/unit/auth/token_renewal.test.tstest/uts/rest/unit/auth/token_request_params.test.tstest/uts/rest/unit/batch_presence.test.tstest/uts/rest/unit/batch_publish.test.tstest/uts/rest/unit/channel/annotations.test.tstest/uts/rest/unit/channel/get_message.test.tstest/uts/rest/unit/channel/history.test.tstest/uts/rest/unit/channel/idempotency.test.tstest/uts/rest/unit/channel/message_versions.test.tstest/uts/rest/unit/channel/publish.test.tstest/uts/rest/unit/channel/publish_result.test.tstest/uts/rest/unit/channel/rest_channel_attributes.test.tstest/uts/rest/unit/channel/update_delete_message.test.tstest/uts/rest/unit/channels_collection.test.tstest/uts/rest/unit/encoding/message_encoding.test.tstest/uts/rest/unit/fallback.test.tstest/uts/rest/unit/logging.test.tstest/uts/rest/unit/presence/rest_presence.test.tstest/uts/rest/unit/push/push_admin_publish.test.tstest/uts/rest/unit/push/push_channel_subscriptions.test.tstest/uts/rest/unit/push/push_channels.test.tstest/uts/rest/unit/push/push_device_registrations.test.tstest/uts/rest/unit/request.test.tstest/uts/rest/unit/request_endpoint.test.tstest/uts/rest/unit/rest_client.test.tstest/uts/rest/unit/stats.test.tstest/uts/rest/unit/time.test.tstest/uts/rest/unit/types/error_types.test.tstest/uts/rest/unit/types/message_types.test.tstest/uts/rest/unit/types/mutable_message_types.test.tstest/uts/rest/unit/types/options_types.test.tstest/uts/rest/unit/types/paginated_result.test.tstest/uts/rest/unit/types/presence_message_types.test.tstest/uts/rest/unit/types/token_types.test.ts
💤 Files with no reviewable changes (1)
- test/uts/rest/channel/rest_channel_attributes.test.ts
| expect(client.connection.errorReason).to.not.be.null; | ||
| expect(client.connection.errorReason.code).to.equal(40005); | ||
| expect(client.connection.errorReason.statusCode).to.equal(400); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read lines around 112-114 and 340-342
fd -t f "connection_open_failures.test.ts" test/
# Get file size first
wc -l test/uts/realtime/integration/proxy/connection_open_failures.test.tsRepository: ably/ably-js
Length of output: 265
🏁 Script executed:
# Read lines 100-125 to see context around line 112-114
sed -n '100,125p' test/uts/realtime/integration/proxy/connection_open_failures.test.tsRepository: ably/ably-js
Length of output: 919
🏁 Script executed:
# Read lines 330-350 to see context around line 340-342
sed -n '330,350p' test/uts/realtime/integration/proxy/connection_open_failures.test.tsRepository: ably/ably-js
Length of output: 824
🏁 Script executed:
# Find and check tsconfig.json
find . -name "tsconfig.json" -type f | head -5Repository: ably/ably-js
Length of output: 253
🏁 Script executed:
# Find the connection type definition to understand errorReason type
fd -t f "connection" src/ | grep -E "\.(ts|d\.ts)$" | head -20Repository: ably/ably-js
Length of output: 295
🏁 Script executed:
# Search for errorReason type definition
rg "errorReason" --type typescript -A 2 -B 2 src/Repository: ably/ably-js
Length of output: 91
🏁 Script executed:
# Search for errorReason type definition with correct type flag
rg "errorReason" --type ts -A 2 -B 2 src/ | head -100Repository: ably/ably-js
Length of output: 7459
🏁 Script executed:
# Check the main connection class definition
cat -n src/common/lib/client/connection.ts | head -150Repository: ably/ably-js
Length of output: 3048
🏁 Script executed:
# Check tsconfig.json for strict null checking settings
cat -n tsconfig.json | head -50Repository: ably/ably-js
Length of output: 654
🏁 Script executed:
# Look for ErrorInfo type definition to understand the .code and .statusCode properties
rg "class ErrorInfo|interface ErrorInfo" --type ts -A 10 src/Repository: ably/ably-js
Length of output: 884
Extract errorReason to a local variable and guard it before accessing properties.
With strict: true enabled in tsconfig.json, errorReason: ErrorInfo | null requires explicit type narrowing before accessing .code and .statusCode. Chai's .to.not.be.null assertion does not narrow the TypeScript type, so these property reads trigger a TS18047 compile failure. Store errorReason in a local variable, check it against null, and assert on the guarded reference instead.
Also applies to: 340-342
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Bundle size report
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Lint
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Spec coverage report
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test (react hooks)
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test browser
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test NodeJS
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test NPM package
[error] 113-113: TS18047: 'client.connection.errorReason' is possibly 'null'.
[error] 114-114: TS18047: 'client.connection.errorReason' is possibly 'null'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/proxy/connection_open_failures.test.ts` around
lines 112 - 114, The assertions read client.connection.errorReason properties
without narrowing a nullable type; extract const err =
client.connection.errorReason, assert expect(err).to.not.be.null (or use
expect(err).to.exist), then access guarded properties as
expect(err!.code).to.equal(40005) and expect(err!.statusCode).to.equal(400);
apply the same pattern for the other occurrence that checks errorReason later in
the file.
| // Error reason is set indicating why resume failed | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| expect(client.connection.errorReason.code).to.equal(80008); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file
find . -name "connection_resume.test.ts" -type fRepository: ably/ably-js
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json for strictNullChecks setting
find . -name "tsconfig.json" -type f | head -5 | xargs grep -l "strictNullChecks" 2>/dev/null || echo "No tsconfig found with strictNullChecks"Repository: ably/ably-js
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the specific file and the lines mentioned in the review
if [ -f "test/uts/realtime/integration/proxy/connection_resume.test.ts" ]; then
echo "=== Lines 309-311 ==="
sed -n '305,315p' test/uts/realtime/integration/proxy/connection_resume.test.ts
echo ""
echo "=== Lines 390-391 ==="
sed -n '385,395p' test/uts/realtime/integration/proxy/connection_resume.test.ts
echo ""
echo "=== Lines 564-566 ==="
sed -n '560,570p' test/uts/realtime/integration/proxy/connection_resume.test.ts
echo ""
echo "=== Lines 964-965 ==="
sed -n '960,970p' test/uts/realtime/integration/proxy/connection_resume.test.ts
else
echo "File not found"
fiRepository: ably/ably-js
Length of output: 2141
🏁 Script executed:
#!/bin/bash
# Search for all instances where errorReason is checked with expect to.not.be.null in this file
rg -n "errorReason.*to\.not\.be\.null|to\.not\.be\.null.*errorReason" test/uts/realtime/integration/proxy/connection_resume.test.ts --context 2Repository: ably/ably-js
Length of output: 1348
🏁 Script executed:
#!/bin/bash
# Search for tsconfig files in the repository
find . -name "tsconfig*.json" -type f | head -20Repository: ably/ably-js
Length of output: 702
🏁 Script executed:
#!/bin/bash
# Read the main tsconfig.json if it exists
if [ -f "tsconfig.json" ]; then
echo "=== tsconfig.json ==="
cat tsconfig.json
fiRepository: ably/ably-js
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Look for type definitions of errorReason
rg -n "errorReason" --type ts | grep -E "(type|interface|property)" | head -20Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search in type definition files for errorReason
find . -path "*/node_modules" -prune -o -name "*.d.ts" -type f -print | xargs rg -l "errorReason" 2>/dev/null | head -10Repository: ably/ably-js
Length of output: 68
🏁 Script executed:
#!/bin/bash
# Get the line count of the connection_resume.test.ts file to understand file size
wc -l test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Read the ably.d.ts file to see errorReason type definition
head -200 ably.d.tsRepository: ably/ably-js
Length of output: 12875
🏁 Script executed:
#!/bin/bash
# Search specifically for errorReason in ably.d.ts
rg -n "errorReason" ably.d.ts -A 2 -B 2Repository: ably/ably-js
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Look for ConnectionState or Connection interface definition
rg -n "interface Connection|class Connection|errorReason.*:" ably.d.ts | head -20Repository: ably/ably-js
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Search for ErrorInfo interface definition
rg -n "interface ErrorInfo|type ErrorInfo" ably.d.ts -A 10 | head -50Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look for the full Connection interface to see if errorReason is optional
rg -n "export declare interface Connection" ably.d.ts -A 30Repository: ably/ably-js
Length of output: 2081
🏁 Script executed:
#!/bin/bash
# Find ErrorInfo interface definition
rg -n "interface ErrorInfo" ably.d.ts -B 2 -A 15Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Also check if ErrorInfo can be null in the type system
rg -n "ErrorInfo" ably.d.ts | grep -E "(null|undefined|\?)" | head -20Repository: ably/ably-js
Length of output: 1947
🏁 Script executed:
#!/bin/bash
# Look for complete ErrorInfo definition
sed -n '1,4000p' ably.d.ts | grep -A 20 "interface ErrorInfo"Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for class ErrorInfo definition
rg -n "class ErrorInfo|declare class ErrorInfo" ably.d.ts -A 20Repository: ably/ably-js
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Look at lines around 3263 to get complete context of Connection interface
sed -n '3258,3280p' ably.d.tsRepository: ably/ably-js
Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Check if there are any test errors or compilation issues
# Look for package.json to understand test setup
cat package.json | head -50Repository: ably/ably-js
Length of output: 1327
🏁 Script executed:
#!/bin/bash
# Look for the actual implementation of Connection to see if errorReason is nullable there
# Search in src directory for Connection class definition
rg -n "errorReason" src/ --type ts --type js -B 2 -A 2 | head -50Repository: ably/ably-js
Length of output: 3980
🏁 Script executed:
#!/bin/bash
# Check if there's a .ts implementation file for Connection
find src -name "*.ts" -type f | xargs rg -l "class.*Connection" | head -10Repository: ably/ably-js
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Look for where errorReason is initialized or declared in the source
rg -n "this\.errorReason|errorReason\s*=" src/ --type ts --type js | head -30Repository: ably/ably-js
Length of output: 2067
🏁 Script executed:
#!/bin/bash
# Read the Connection class definition in src/common/lib/client/connection.ts
cat -n src/common/lib/client/connection.tsRepository: ably/ably-js
Length of output: 3048
🏁 Script executed:
#!/bin/bash
# Check the type definition for IPartialErrorInfo and how it relates to ErrorInfo
rg -n "IPartialErrorInfo|interface.*ErrorInfo" src/common/lib/ --type ts | head -30Repository: ably/ably-js
Length of output: 2089
🏁 Script executed:
#!/bin/bash
# Let's check the Channel interface in ably.d.ts for errorReason
sed -n '2440,2460p' ably.d.tsRepository: ably/ably-js
Length of output: 839
🏁 Script executed:
#!/bin/bash
# Let's verify if there are actual type issues by checking for TypeScript compilation
# Look for any build or test scripts that compile TypeScript
grep -A 5 "typescript\|tsc\|compile" package.jsonRepository: ably/ably-js
Length of output: 1541
🏁 Script executed:
#!/bin/bash
# Check if there's a .ts version of the type definitions or if errorReason is actually nullable somewhere
# Look for Channel class in the source
rg -n "class.*Channel|errorReason.*null" src/common/lib/client/ --type ts -B 2 -A 2 | head -50Repository: ably/ably-js
Length of output: 2579
Extract errorReason to a local variable and guard before dereferencing.
The pattern expect(...).to.not.be.null does not perform TypeScript type narrowing, so accessing properties on errorReason after this assertion fails under strictNullChecks (TS18047). Instead, read client.connection.errorReason into a local variable, guard it with a null check, and assert against the checked local.
This pattern appears at:
- Lines 310–311
- Lines 390–391
- Lines 564–566
- Lines 964–965
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Bundle size report
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Lint
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Spec coverage report
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test (react hooks)
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test browser
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test NodeJS
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🪛 GitHub Actions: Test NPM package
[error] 311-311: TS18047: 'client.connection.errorReason' is possibly 'null'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/proxy/connection_resume.test.ts` around lines
309 - 311, Extract client.connection.errorReason into a local variable and check
it for null before accessing properties: replace direct assertions like
expect(client.connection.errorReason).to.not.be.null;
expect(client.connection.errorReason.code).to.equal(80008); with a guarded
pattern that assigns const err = client.connection.errorReason; assert err is
not null (or fail the test) and then assert err.code === 80008. Apply this
change for all occurrences referencing client.connection.errorReason (the blocks
around the assertions at the noted locations).
| // RTN16k: Verify the recover query parameter was sent via proxy log | ||
| const log = await session.getLog(); | ||
| const wsConnects = log.filter((e) => e.type === 'ws_connect'); | ||
| expect(wsConnects.length).to.be.at.least(1); | ||
| expect(wsConnects[0].queryParams).to.exist; | ||
| expect(wsConnects[0].queryParams!['recover']).to.equal(originalConnectionKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file structure and find the lines mentioned
wc -l test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 122
🏁 Script executed:
# Read the code around lines 871-876
sed -n '850,890p' test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 1435
🏁 Script executed:
# Read the code around lines 970-975
sed -n '950,990p' test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 1214
🏁 Script executed:
# Find where originalConnectionKey is defined in the file
rg -n 'originalConnectionKey\s*=' test/uts/realtime/integration/proxy/connection_resume.test.ts | head -20Repository: ably/ably-js
Length of output: 179
🏁 Script executed:
# Find where recoveryKey is defined
rg -n 'recoveryKey\s*=' test/uts/realtime/integration/proxy/connection_resume.test.ts | head -20Repository: ably/ably-js
Length of output: 124
🏁 Script executed:
# Get more context around where recoveryKey is created - let's look further back in the test
sed -n '820,875p' test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 2059
🏁 Script executed:
# Search for recovery key format or how it's constructed in the SDK
fd -e ts -e js | xargs rg -l 'recovery.*key|recoveryKey' | grep -v test | grep -v node_modules | head -10Repository: ably/ably-js
Length of output: 148
🏁 Script executed:
# Look at createRecoveryKey implementation
rg -A 15 'createRecoveryKey\s*\(' src/common/lib/client/connection.ts | head -40Repository: ably/ably-js
Length of output: 482
🏁 Script executed:
# Check lines 970-975 for the second assertion mentioned
sed -n '955,1000p' test/uts/realtime/integration/proxy/connection_resume.test.tsRepository: ably/ably-js
Length of output: 1109
🏁 Script executed:
# Search for how recovery key is used in connection manager
rg -A 5 -B 5 'recover.*param|recovery.*param' src/common/lib/transport/connectionmanager.ts | head -50Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
# Let's also verify what createRecoveryKey returns to fully understand the difference
sed -n '1,100p' src/common/lib/transport/connectionmanager.ts | grep -A 20 'createRecoveryKey'Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
# Better search for the implementation
rg -B 5 -A 20 'createRecoveryKey\(\)' src/common/lib/transport/connectionmanager.ts | head -50Repository: ably/ably-js
Length of output: 1525
Assert recover against recoveryKey, not originalConnectionKey.
The test creates a recovery key via client1.connection.createRecoveryKey() (line 829) which returns a JSON string containing the connection key, message serial, and channel serials. This recoveryKey is passed to client2 as the recover parameter, but the assertion at line 876 compares it against the bare originalConnectionKey instead. Update the assertion to check against the actual recoveryKey variable:
expect(wsConnects[0].queryParams!['recover']).to.equal(recoveryKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/proxy/connection_resume.test.ts` around lines
871 - 876, The assertion is comparing the proxy 'recover' query param to the
wrong value; update the test so the ws_connect log entry's queryParams 'recover'
is asserted against the recoveryKey created by
client1.connection.createRecoveryKey(), not originalConnectionKey. Locate the
check using wsConnects[0].queryParams and replace the expected value reference
from originalConnectionKey to recoveryKey (the variable produced earlier in the
test) so the test verifies the actual recovery payload passed to client2.
| const httpResponses = log.filter((e) => e.type === 'http_response'); | ||
| expect(httpResponses[0].status).to.equal(401); | ||
| expect(httpResponses[1].status).to.be.oneOf([200, 201]); |
There was a problem hiding this comment.
Positional httpResponses[0] assertion is fragile
If any HTTP traffic through the proxy precedes the publish (e.g. an SDK-internal probe or auth warmup request), httpResponses[0] will not be the injected 401 and the assertion silently validates the wrong response. Filtering by path the same way httpRequests is filtered makes this resilient to unrelated traffic.
♻️ Proposed fix
- const httpResponses = log.filter((e) => e.type === 'http_response');
- expect(httpResponses[0].status).to.equal(401);
- expect(httpResponses[1].status).to.be.oneOf([200, 201]);
+ const channelResponses = log.filter(
+ (e) => e.type === 'http_response' && e.path && e.path.includes('/channels/'),
+ );
+ expect(channelResponses[0].status).to.equal(401);
+ expect(channelResponses[1].status).to.be.oneOf([200, 201]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/proxy/rest_faults.test.ts` around lines 101 -
103, The assertion is using positional indexing on httpResponses which is
brittle; instead filter responses by the same request path used for httpRequests
so you target the proxy's publish response only. Update the httpResponses
assignment (the const httpResponses = log.filter(...)) to include a path check
(matching the same path string or variable used when filtering httpRequests) and
then assert on the first element of that filtered array; reference the
httpResponses variable and the existing log.filter call to locate where to add
the path predicate.
| const publishedMsg = history.items.find((m: any) => m.name === 'test-msg'); | ||
| expect(publishedMsg).to.not.be.undefined; | ||
| expect(publishedMsg.data).to.equal('hello world'); |
There was a problem hiding this comment.
publishedMsg possibly undefined after .find() — TypeScript pipeline failure (TS18048)
The Chai assertion at line 230 does not narrow the TypeScript type; publishedMsg.data on line 231 therefore produces TS18048: 'publishedMsg' is possibly 'undefined' across all CI jobs.
🐛 Proposed fix
const publishedMsg = history.items.find((m: any) => m.name === 'test-msg');
expect(publishedMsg).to.not.be.undefined;
- expect(publishedMsg.data).to.equal('hello world');
+ expect(publishedMsg!.data).to.equal('hello world');📝 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.
| const publishedMsg = history.items.find((m: any) => m.name === 'test-msg'); | |
| expect(publishedMsg).to.not.be.undefined; | |
| expect(publishedMsg.data).to.equal('hello world'); | |
| const publishedMsg = history.items.find((m: any) => m.name === 'test-msg'); | |
| expect(publishedMsg).to.not.be.undefined; | |
| expect(publishedMsg!.data).to.equal('hello world'); |
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Bundle size report
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Lint
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Spec coverage report
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Test (react hooks)
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Test browser
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Test NodeJS
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🪛 GitHub Actions: Test NPM package
[error] 231-231: TS18048: 'publishedMsg' is possibly 'undefined'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/proxy/rest_faults.test.ts` around lines 229 -
231, The test accesses publishedMsg.data after using history.items.find which
can return undefined, causing TS18048; update the test around the publishedMsg
variable (the result of history.items.find) to narrow the type before accessing
.data — for example, add an explicit runtime/type guard such as
expect(publishedMsg).to.not.be.undefined or expect(publishedMsg).to.exist and
then either use a non-null assertion (publishedMsg!.data) or reassign to a typed
variable (const msg = publishedMsg as { data: string }) before calling .data to
satisfy TypeScript; ensure the assertion and subsequent access both reference
the publishedMsg identifier.
| const client = new Ably.Realtime({ | ||
| key: 'appId.keyId:keySecret', | ||
| channelRetryTimeout: channelRetryTimeout, | ||
| autoConnect: false, | ||
| useBinaryProtocol: false, | ||
| }); | ||
| trackClient(client); |
There was a problem hiding this comment.
channelRetryTimeout is not a known ClientOptions property — all CI pipelines fail with TS2345
The object literal lacks the as any cast needed to pass an internal option that isn't in the public ClientOptions type definition. Every CI variant in the pipeline fails with:
TS2345: Object literal may only specify known properties; 'channelRetryTimeout' does not exist in type 'ClientOptions'.
Note that disconnectedRetryTimeout on line 143 is in the public type and works without a cast, but channelRetryTimeout is not.
🐛 Proposed fix
const client = new Ably.Realtime({
key: 'appId.keyId:keySecret',
channelRetryTimeout: channelRetryTimeout,
autoConnect: false,
useBinaryProtocol: false,
-});
+} as any);📝 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.
| const client = new Ably.Realtime({ | |
| key: 'appId.keyId:keySecret', | |
| channelRetryTimeout: channelRetryTimeout, | |
| autoConnect: false, | |
| useBinaryProtocol: false, | |
| }); | |
| trackClient(client); | |
| const client = new Ably.Realtime({ | |
| key: 'appId.keyId:keySecret', | |
| channelRetryTimeout: channelRetryTimeout, | |
| autoConnect: false, | |
| useBinaryProtocol: false, | |
| } as any); | |
| trackClient(client); |
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 273-273: TS2345: Argument object literal may only specify known properties; 'channelRetryTimeout' does not exist in type 'ClientOptions'.
🪛 GitHub Actions: Bundle size report
[error] 273-273: TS2345: Argument object includes unknown property 'channelRetryTimeout' (not in ClientOptions).
🪛 GitHub Actions: Lint
[error] 273-273: TS2345: Argument object includes 'channelRetryTimeout', which is not a property of ClientOptions.
🪛 GitHub Actions: Spec coverage report
[error] 273-273: TS2345: Argument object includes unknown property 'channelRetryTimeout' for parameter type 'string | ClientOptions | undefined'.
🪛 GitHub Actions: Test (react hooks)
[error] 273-273: TS2345: Argument object includes unknown property 'channelRetryTimeout' for parameter type 'string | ClientOptions | undefined'.
🪛 GitHub Actions: Test browser
[error] 273-273: TS2345: Argument object not assignable to 'string | ClientOptions | undefined'; 'channelRetryTimeout' does not exist in type 'ClientOptions'.
🪛 GitHub Actions: Test NodeJS
[error] 273-273: TS2345: Object literal includes unknown property 'channelRetryTimeout' (not in ClientOptions).
🪛 GitHub Actions: Test NPM package
[error] 273-273: TS2345: Argument of type '{ key: string; channelRetryTimeout: number; autoConnect: false; useBinaryProtocol: false; }' is not assignable to parameter of type 'string | ClientOptions | undefined'. Object literal may only specify known properties; 'channelRetryTimeout' does not exist in type 'ClientOptions'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/unit/connection/backoff_jitter.test.ts` around lines 271 -
277, The test is passing an internal-only option channelRetryTimeout into the
Ably.Realtime constructor which is not part of the public ClientOptions type,
causing TS2345; update the Ably.Realtime instantiation (the object passed to new
Ably.Realtime in this test where channelRetryTimeout and
disconnectedRetryTimeout are set) to cast the options object to any (e.g. via an
as any cast) so the internal option is allowed, leaving trackClient unchanged.
| import Logger from '../../../../../src/common/lib/util/logger'; | ||
|
|
||
| function createMockPresence(): any { | ||
| const logger = new Logger(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Retrieve the Logger class constructor signature to determine the correct fix.
# Expected: constructor with 0 params, or a factory/static method to use instead.
ast-grep --pattern 'class Logger {
$$$
constructor($$$) {
$$$
}
$$$
}'Repository: ably/ably-js
Length of output: 6984
Remove the numeric argument from Logger constructor call
The Logger constructor in src/common/lib/util/logger.ts takes no arguments (line 97: constructor()), but line 23 passes a numeric value. This breaks all CI pipelines with TS2554: Expected 0 arguments, but got 1.
Fix
function createMockPresence(): any {
- const logger = new Logger(0);
+ const logger = new Logger();
return {📝 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.
| import Logger from '../../../../../src/common/lib/util/logger'; | |
| function createMockPresence(): any { | |
| const logger = new Logger(0); | |
| import Logger from '../../../../../src/common/lib/util/logger'; | |
| function createMockPresence(): any { | |
| const logger = new Logger(); |
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Bundle size report
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Lint
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Spec coverage report
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Test (react hooks)
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Test browser
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Test NodeJS
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🪛 GitHub Actions: Test NPM package
[error] 23-23: TS2554: Expected 0 arguments, but got 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/unit/presence/presence_sync.test.ts` around lines 20 - 23,
In createMockPresence(), remove the incorrect numeric argument passed to the
Logger constructor: change the instantiation in the createMockPresence function
from new Logger(0) to new Logger() so it matches the Logger class signature
(constructor()) and resolves the TS2554 error; update any other test helpers in
this file that call new Logger(...) to use no-argument construction as well.
| // Small delay to ensure server timestamps differ between batches | ||
| await new Promise((r) => setTimeout(r, 2)); |
There was a problem hiding this comment.
2 ms sleep is too short to guarantee distinct server timestamps, risking intermittent failures.
When maxEarlyTs === minLateTs (server batches both publishes in the same millisecond), timeBoundary = maxEarlyTs and the lateHistory query becomes { start: maxEarlyTs + 1, ... }, which returns zero items — failing the expect(lateHistory.items.length).to.be.at.least(1) assertion. A longer sleep (e.g., 50–100 ms) makes distinct timestamps reliably likely under sandbox latency.
🛡️ Proposed fix
- await new Promise((r) => setTimeout(r, 2));
+ await new Promise((r) => setTimeout(r, 100));📝 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.
| // Small delay to ensure server timestamps differ between batches | |
| await new Promise((r) => setTimeout(r, 2)); | |
| // Small delay to ensure server timestamps differ between batches | |
| await new Promise((r) => setTimeout(r, 100)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/integration/history.test.ts` around lines 151 - 152, The 2 ms
delay is too short to guarantee distinct server timestamps causing intermittent
test failures (when maxEarlyTs === minLateTs), so increase the sleep in the test
where the Promise/setTimeout is used to a much larger value (e.g., 50–100 ms) so
maxEarlyTs and minLateTs will almost always differ; update the await new
Promise((r) => setTimeout(r, 2)) call to use ~100 ms and keep the existing
assertions that check timeBoundary, lateHistory, and
expect(lateHistory.items.length).to.be.at.least(1).
| // Poll until all messages appear and retrieve with timestamps | ||
| const allMessages: any[] = await pollUntil(async () => { | ||
| const result = await channel.history(); | ||
| return result.items.length === 4 ? result : null; | ||
| return result.items.length === 4 ? result.items : null; | ||
| }, { interval: 500, timeout: 10000 }); |
There was a problem hiding this comment.
TypeScript errors TS2322 and TS2554 in changed code are blocking all CI pipelines.
Two errors in the changed RSL2b3 block:
- Line 159 (
TS2322): Explicitly typingallMessagesasany[]conflicts with theMessage[] | nullreturn type ofpollUntil. Remove the explicit annotation and add a non-null assertion. - Line 160 (
TS2554):channel.history()requires at least one argument per the TypeScript type signature. Pass{}. The sameTS2554error occurs in unchanged calls at lines 49, 91, 122, and 213 — all need the same fix.
🐛 Proposed fix for lines 158–162
- const allMessages: any[] = await pollUntil(async () => {
- const result = await channel.history();
- return result.items.length === 4 ? result.items : null;
+ const allMessages = (await pollUntil(async () => {
+ const result = await channel.history({});
+ return result.items.length === 4 ? result.items : null;
- }, { interval: 500, timeout: 10000 });
+ }, { interval: 500, timeout: 10000 }))!;And apply the same channel.history({}) fix to the unchanged calls at lines 49, 91, 122, and 213.
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Bundle size report
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Lint
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Spec coverage report
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Test (react hooks)
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Test browser
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]'; 'null' is not assignable.
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Test NodeJS
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: Test NPM package
[error] 159-159: TS2322: Type 'Message[] | null' is not assignable to type 'any[]' (null not assignable).
[error] 160-160: TS2554: Expected 1 arguments, but got 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/integration/history.test.ts` around lines 158 - 162, Remove the
explicit any[] annotation and use a non-null assertion on the pollUntil result,
and call channel.history with an empty options object; specifically, change the
polling block so you assign the result without typing (let allMessages = await
pollUntil(... )!) and inside the callback call channel.history({}) instead of
channel.history(), and apply the same channel.history({}) change to the other
calls to channel.history in this file (the calls referenced around lines where
history is used).
Verifies that when the proxy closes the underlying TCP connection without sending a WebSocket close frame, ably-js detects the TCP FIN and transitions to disconnected with the same minimal delay as the close-frame case. Corresponds to specification issue #464. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…om source Removes the Go toolchain dependency for running proxy integration tests. The binary is fetched from github.com/ably/uts-proxy releases, verified against SHA-256 checksums, and cached in node_modules/.cache/uts-proxy/. Also removes the unused run-proxy-tests.sh script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/uts/realtime/integration/helpers/proxy.ts`:
- Around line 31-35: The current allocatePort() uses a process-local counter
(nextPort++) which can collide across processes; replace it with an OS-backed
ephemeral allocation by creating a temporary socket bound to port 0 to let the
OS pick a free port, read the assigned port from the socket (e.g., via
server.address().port), close the socket and return that port, or change
createProxySession() to ask the proxy to bind an ephemeral port and return the
chosen port; update the allocatePort() caller(s) and any logic in
createProxySession() to use this OS-assigned port rather than relying on
nextPort++.
- Around line 179-190: The assetName() helper currently treats any non-darwin
platform as linux which returns wrong artifacts on Windows; update assetName()
to explicitly handle process.platform === 'linux' and process.platform ===
'darwin' and throw a descriptive Error for unsupported platforms (e.g., 'win32')
so callers fail fast; reference the assetName() function and the CHECKSUMS map
when adding the error message so it’s clear which platforms are supported.
- Around line 246-279: The ensureProxy function risks racing: two callers can
pass the _proxyEnsured check and both call spawnProxy, overwriting _proxyProcess
and orphaning the real process; fix by adding a module-scoped in-flight Promise
(e.g., _ensureProxyPromise) that is set at the start of ensureProxy and returned
if present so concurrent callers wait on the same startup flow, ensure only the
first caller runs downloadProxy() and spawnProxy(), and clear/reset
_ensureProxyPromise and set/clear _proxyProcess/_proxyEnsured appropriately on
success or failure (killing the process and rejecting) so stopProxy() can
reliably reference the correct _proxyProcess.
- Around line 97-127: All control-plane fetch calls are unbounded and must be
given per-request timeouts: for each function named addRules, triggerAction,
getLog, close, createProxySession, downloadProxy and the health poll inside
ensureProxy, create an AbortSignal with AbortSignal.timeout(ms) and pass it as
the signal option to fetch; choose short sensible timeouts (e.g. 5–10s for
startup/health probes, similar or slightly longer for session
operations/downloads) so each fetch can abort quickly and let the outer
retry/timeout logic (ensureProxy timeoutMs) handle retries. Ensure you construct
the signal per-request and cleanly let fetch throw on abort so existing error
handling (resp.ok checks and throws) remains effective.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3ffb853-6ac9-4daf-be1b-1c0b2d113c0f
📒 Files selected for processing (1)
test/uts/realtime/integration/helpers/proxy.ts
| let nextPort = 19000 + Math.floor(Math.random() * 1000); | ||
|
|
||
| function allocatePort(): number { | ||
| return nextPort++; | ||
| } |
There was a problem hiding this comment.
Stop hand-allocating session ports.
nextPort++ never checks or reserves the socket, so parallel workers or anything already bound in the 19000-19999 range can make createProxySession() fail nondeterministically. This needs OS-backed allocation, or proxy-side ephemeral port assignment, instead of a process-local counter.
Also applies to: 146-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/helpers/proxy.ts` around lines 31 - 35, The
current allocatePort() uses a process-local counter (nextPort++) which can
collide across processes; replace it with an OS-backed ephemeral allocation by
creating a temporary socket bound to port 0 to let the OS pick a free port, read
the assigned port from the socket (e.g., via server.address().port), close the
socket and return that port, or change createProxySession() to ask the proxy to
bind an ephemeral port and return the chosen port; update the allocatePort()
caller(s) and any logic in createProxySession() to use this OS-assigned port
rather than relying on nextPort++.
| const resp = await fetch(`${this.controlUrl}/sessions/${this.sessionId}/rules`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ rules, position }), | ||
| }); | ||
| if (!resp.ok) { | ||
| const body = await resp.text(); | ||
| throw new Error(`addRules failed (${resp.status}): ${body}`); | ||
| } | ||
| } | ||
|
|
||
| async triggerAction(action: ImperativeAction): Promise<void> { | ||
| const resp = await fetch(`${this.controlUrl}/sessions/${this.sessionId}/actions`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(action), | ||
| }); | ||
| if (!resp.ok) { | ||
| const body = await resp.text(); | ||
| throw new Error(`triggerAction failed (${resp.status}): ${body}`); | ||
| } | ||
| } | ||
|
|
||
| async getLog(): Promise<ProxyEvent[]> { | ||
| const resp = await fetch(`${this.controlUrl}/sessions/${this.sessionId}/log`); | ||
| if (!resp.ok) { | ||
| const body = await resp.text(); | ||
| throw new Error(`getLog failed (${resp.status}): ${body}`); | ||
| } | ||
| const data = await resp.json(); | ||
| return data.events || []; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Node.js global fetchhave a default request timeout, and how isAbortSignal.timeout() used to cap a request?
💡 Result:
No, the Node.js global fetch does not have a single default request timeout parameter in its API. Instead, the underlying Undici implementation enforces specific timeouts: headersTimeout (300 seconds / 5 minutes, time to receive headers), bodyTimeout (300 seconds, time between body chunks), and connectTimeout (around 10 seconds in some cases). These can cause requests to fail with errors like HeadersTimeoutError or ConnectTimeoutError if exceeded, even without an explicit user-set timeout. AbortSignal.timeout(ms) creates an AbortSignal that automatically aborts after the specified milliseconds with a TimeoutError. Pass it via the signal option to cap the entire request duration, overriding or complementing Undici's internal timeouts: try { const response = await fetch('https://example.com', { signal: AbortSignal.timeout(5000) // 5 second cap }); // handle response } catch (err) { if (err.name === 'TimeoutError') { console.log('Request timed out after 5s'); } } This is supported in Node.js since v17.3.0 (AbortSignal.timeout added v17.3.0/v16.14.0; stable fetch since v18). For finer control over Undici timeouts (e.g., increasing headersTimeout), use a custom Agent dispatcher: import { Agent } from 'undici'; await fetch(url, { dispatcher: new Agent({ headersTimeout: 600e3 }) });
Citations:
- 1: https://nodejs.org/api/globals.html
- 2: https://github.com/nodejs/undici/blob/main/docs/docs/api/Client.md
- 3: https://stackoverflow.com/questions/76755913/increase-timeout-in-nodejs-built-in-fetch-api
- 4: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 5: https://medium.com/deno-the-complete-reference/timeout-fetch-request-in-node-js-4231f33a9b95
- 6: https://nodejs.dev/en/learn/getting-started/fetch
🏁 Script executed:
# Find the proxy.ts file and examine the ensureProxy method around line 263
find . -type f -name "proxy.ts" | head -5Repository: ably/ably-js
Length of output: 105
🏁 Script executed:
# Check the file to understand the ensureProxy method and timeoutMs usage
cat -n test/uts/realtime/integration/helpers/proxy.ts | sed -n '250,270p'Repository: ably/ably-js
Length of output: 671
🏁 Script executed:
# Also check the full context of ensureProxy method
cat -n test/uts/realtime/integration/helpers/proxy.ts | sed -n '250,300p'Repository: ably/ably-js
Length of output: 1462
🏁 Script executed:
# Check other fetch calls mentioned (lines 132, 164-168, 206-208, 251-267)
cat -n test/uts/realtime/integration/helpers/proxy.ts | sed -n '1,50p'Repository: ably/ably-js
Length of output: 1910
🏁 Script executed:
# Check the other fetch calls mentioned in the review
cat -n test/uts/realtime/integration/helpers/proxy.ts | sed -n '95,135p'Repository: ably/ably-js
Length of output: 1731
🏁 Script executed:
# Check lines around 164-168 and 206-208
cat -n test/uts/realtime/integration/helpers/proxy.ts | sed -n '160,210p'Repository: ably/ably-js
Length of output: 2223
Add per-request timeouts to all control-plane fetch calls.
These fetch() calls are unbounded. Each can hang for up to 5 minutes (Undici's headersTimeout), which defeats the startup timeout budget at line 282 (ensureProxy(timeoutMs)). While the loop measures wall-clock time, individual health probes ignore it—if localhost accepts a connection and stops responding, the suite hangs beyond the intended timeout.
Use AbortSignal.timeout(ms) to cap each request. Allocate a per-request timeout (e.g., 5–10 seconds for startup probes; similar for session operations) and let the startup loop handle retries within its budget.
Affects: addRules() (line 97), triggerAction() (line 109), getLog() (line 121), close() (line 132), createProxySession() (lines 164–168), downloadProxy() (line 206), and ensureProxy() health polling (line 266).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/helpers/proxy.ts` around lines 97 - 127, All
control-plane fetch calls are unbounded and must be given per-request timeouts:
for each function named addRules, triggerAction, getLog, close,
createProxySession, downloadProxy and the health poll inside ensureProxy, create
an AbortSignal with AbortSignal.timeout(ms) and pass it as the signal option to
fetch; choose short sensible timeouts (e.g. 5–10s for startup/health probes,
similar or slightly longer for session operations/downloads) so each fetch can
abort quickly and let the outer retry/timeout logic (ensureProxy timeoutMs)
handle retries. Ensure you construct the signal per-request and cleanly let
fetch throw on abort so existing error handling (resp.ok checks and throws)
remains effective.
| const CHECKSUMS: Record<string, string> = { | ||
| 'uts-proxy_darwin_amd64.tar.gz': 'eb8abf5eec7f7137cf9e7cb6ab6f45fd162303c242b4567ab9e354c4b9a4a4ff', | ||
| 'uts-proxy_darwin_arm64.tar.gz': '845da80af7d5b1daacbdf30b34aff6ca1b2bb88c708065bdc5d9a636baf32a1f', | ||
| 'uts-proxy_linux_amd64.tar.gz': '79f444c23362cc277d163deb243dc16063c74665ff63b8bd3e56789b9d9610c7', | ||
| 'uts-proxy_linux_arm64.tar.gz': '7357e4605f19451d83bb419ee959537d6e95ca74b766721eae006d4171371030', | ||
| }; | ||
|
|
||
| function assetName(): string { | ||
| const platform = process.platform === 'darwin' ? 'darwin' : 'linux'; | ||
| const arch = process.arch === 'arm64' ? 'arm64' : 'amd64'; | ||
| return `uts-proxy_${platform}_${arch}.tar.gz`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What values can Node.js process.platform return, and how is Windows identified?
💡 Result:
Node.js process.platform returns one of the following string values: 'aix', 'darwin', 'freebsd', 'linux', 'openbsd', 'sunos', or 'win32'. The value 'android' may also be returned if Node.js is built on Android (experimental support). Windows is identified by the value 'win32' (regardless of 32-bit or 64-bit architecture).
Citations:
- 1: https://nodejs.org/api/os.html
- 2: https://stackoverflow.com/questions/8683895/how-do-i-determine-the-current-operating-system-with-node-js
- 3: https://nodejs.org/api/process.html
- 4: https://nodejs.org/download/release/v6.15.1/docs/api/process.html
Handle unsupported platforms explicitly.
The assetName() function at line 187 maps every non-darwin platform to linux. Since Windows is identified as win32 in Node.js, this will return a Linux artifact name on Windows and fail during spawn() execution. Add explicit handling for the linux platform and throw a clear error for unsupported systems (including win32) until matching artifacts are available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/helpers/proxy.ts` around lines 179 - 190, The
assetName() helper currently treats any non-darwin platform as linux which
returns wrong artifacts on Windows; update assetName() to explicitly handle
process.platform === 'linux' and process.platform === 'darwin' and throw a
descriptive Error for unsupported platforms (e.g., 'win32') so callers fail
fast; reference the assetName() function and the CHECKSUMS map when adding the
error message so it’s clear which platforms are supported.
| async function ensureProxy(timeoutMs = 15000): Promise<void> { | ||
| if (_proxyEnsured) return; | ||
|
|
||
| // Check if proxy is already running (e.g. started externally) | ||
| try { | ||
| const resp = await fetch(`${PROXY_CONTROL_HOST}/health`); | ||
| if (resp.ok) { | ||
| _proxyEnsured = true; | ||
| return; | ||
| } | ||
| } catch { | ||
| // Not running — we'll start it | ||
| } | ||
|
|
||
| await downloadProxy(); | ||
| _proxyProcess = spawnProxy(); | ||
|
|
||
| const start = Date.now(); | ||
| while (Date.now() - start < timeoutMs) { | ||
| try { | ||
| const resp = await fetch(`${PROXY_CONTROL_HOST}/health`); | ||
| if (resp.ok) { | ||
| _proxyEnsured = true; | ||
| return; | ||
| } | ||
| } catch { | ||
| // Not ready yet | ||
| } | ||
| await new Promise((r) => setTimeout(r, 200)); | ||
| } | ||
|
|
||
| _proxyProcess.kill(); | ||
| _proxyProcess = null; | ||
| throw new Error(`Proxy failed to start within ${timeoutMs}ms`); |
There was a problem hiding this comment.
Memoize proxy startup instead of racing it.
Two callers can pass Line 247 before _proxyEnsured flips, then both try to spawn on the same control port. The later call also overwrites _proxyProcess, which can leave the real proxy orphaned from stopProxy(). Please serialize this with a module-scoped in-flight promise.
Possible shape of the fix
let _proxyProcess: ChildProcess | null = null;
let _proxyEnsured = false;
+let _proxyEnsurePromise: Promise<void> | null = null;
async function ensureProxy(timeoutMs = 15000): Promise<void> {
if (_proxyEnsured) return;
+ if (_proxyEnsurePromise) return _proxyEnsurePromise;
- // existing startup logic
+ _proxyEnsurePromise = (async () => {
+ // existing startup logic
+ })();
+
+ try {
+ await _proxyEnsurePromise;
+ } finally {
+ _proxyEnsurePromise = null;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/integration/helpers/proxy.ts` around lines 246 - 279, The
ensureProxy function risks racing: two callers can pass the _proxyEnsured check
and both call spawnProxy, overwriting _proxyProcess and orphaning the real
process; fix by adding a module-scoped in-flight Promise (e.g.,
_ensureProxyPromise) that is set at the start of ensureProxy and returned if
present so concurrent callers wait on the same startup flow, ensure only the
first caller runs downloadProxy() and spawnProxy(), and clear/reset
_ensureProxyPromise and set/clear _proxyProcess/_proxyEnsured appropriately on
success or failure (killing the process and rejecting) so stopProxy() can
reliably reference the correct _proxyProcess.
There was a problem hiding this comment.
I like the implementation.There are a few minor points that I think are worth addressing:
- I agree with CodeRabbit’s point about port allocation. Manual allocation seems a bit risky; I’d prefer the
uts-proxyto allocate a new port itself and return it to the caller. - I also agree about the race condition in the
ensureProxyfunction.
Personally, instead of using a port and a separate proxy per session, which requires allocating new ports (a limited resource and a relatively heavy operation). I’d prefer generating a new hostname per session and managing everything within a single proxy. I think that’s a more lightweight solution.
That said, it would be useful to dynamically get the endpoint from the proxy session object instead of manually setting it to 'localhost':
endpoint: session.proxyEndpoint,
port: session.proxyPort,This way, if we later decide to modify uts-proxy to support host-based sessions, we won’t need to change anything here.
That said, I don’t object to merging this as is and refactoring later
|
Generating a new host for host-based session management can be done by using |
…ason ably-js behavior is correct — when auth renewal fails while CONNECTED, errorReason should NOT be set because the connection is healthy and the existing token is still valid. Remove deviation skip and update assertions. Refs: ably/specification#466 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/uts/deviations.md (2)
265-274: ⚡ Quick winRSH1b2 documents a server issue, not an SDK deviation.
The RSH1b2 entry describes a backend service limitation (missing
Linkheaders from the/push/deviceRegistrationsendpoint). Consider moving this to a separate "Server/Backend Limitations" section to distinguish server-side issues from SDK spec deviations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/deviations.md` around lines 265 - 274, The RSH1b2 entry under the "integration/push_admin: RSH1b2 - push device list pagination missing Link headers" heading in test/uts/deviations.md documents a backend/server limitation rather than an SDK deviation; move this entire entry into a new or existing "Server/Backend Limitations" section (create one if missing), update any local TOC or index in deviations.md to include the new section, and keep the content and the issue link ([ably/realtime#8380]) intact with a short note that this is server-side behavior rather than an SDK deviation.
11-250: ⚡ Quick winConsider adding issue tracking links for deviations without them.
The following deviations document spec non-compliance but lack issue tracking links: RTC1a, RTL5k, RTN24, RTP17e, TM4, RTL4c, and RTP18a. Adding links (or noting if issues are not yet filed) would improve traceability and help developers track resolution status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/deviations.md` around lines 11 - 250, The deviations file documents several spec non-compliances (RTC1a, RTL5k, RTN24, RTP17e, TM4, RTL4c, RTP18a) but lacks issue-tracking links; update each section header (e.g. the "RTC1a - echoMessages default does not send echo=true", "RTL5k - ATTACHED while detached does not send DETACH", "RTN24 - connection.id/key not updated on UPDATE", "RTP17e - re-entry error message missing clientId", "TM4 - toJSON not a method on Message", "RTL4c - errorReason not cleared on successful re-attach", "RTP18a - new sync does not discard in-flight sync") to include a link to the corresponding GH issue or, if none exists, add a short note like "no issue filed" and optionally a tracking TODO/issue number placeholder; ensure link format matches the existing linked entries and keep the change limited to the deviation headings and a one-line annotation under each affected block for traceability.test/uts/realtime/unit/auth/connection_auth.test.ts (1)
278-347: 💤 Low valueDuplicate of the RSA4c3 case in
auth_callback_errors.test.ts.This test is essentially identical to the
RSA4c3 - authCallback error while CONNECTED does not set errorReasoncase at lines 173-236 of the newtest/uts/realtime/unit/auth/auth_callback_errors.test.ts(same mock setup, same auth flow, same assertions). Since the new file is the dedicated home for RSA4 callback-error coverage, consider keeping the case in only one place to avoid drift between the two suites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/realtime/unit/auth/connection_auth.test.ts` around lines 278 - 347, This test "RSA4c3 - authCallback error while CONNECTED does not set errorReason" is a duplicate of the same case in auth_callback_errors.test.ts; remove the duplicate test from connection_auth.test.ts (delete the it(...) block that sets up MockWebSocket, the authCallback increments, the auth request and assertions) and leave the canonical test in auth_callback_errors.test.ts so coverage remains unchanged; if any shared helpers or test setup were added only for this duplicate, move them into the shared setup or into auth_callback_errors.test.ts instead and run the test suite to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/uts/realtime/unit/auth/auth_callback_errors.test.ts`:
- Around line 222-236: Ensure the test explicitly verifies the auth callback was
invoked twice before making no-op assertions: after the waiting loop using
flushAsync, add an assertion that authCallbackCount is at least 2 (e.g.,
expect(authCallbackCount).to.be.at.least(2)) and only then assert
client.connection.state === 'connected', stateChanges.length === 0, and
client.connection.errorReason === null; reference authCallbackCount, flushAsync,
client.connection.state, stateChanges, and client.connection.errorReason when
locating where to add the assertion.
- Around line 404-424: The test registers two once-handlers that both call
done() (client.connection.once('disconnected', ...) and
client.connection.once('failed', ...)), risking a double done() call if both
events occur; fix by introducing a single-shot guard variable (e.g., let
finished = false) or a helper function (e.g., function finish(err?) { if
(finished) return; finished = true; done(err); }) and replace direct done()
calls in the 'disconnected' and 'failed' handlers (and the similar RSA4f
oversized-token test handlers) with a call to that guard to ensure done() is
invoked only once.
In `@test/uts/realtime/unit/auth/connection_auth.test.ts`:
- Around line 333-347: The polling loop can silently pass if the second auth
callback never ran; add an explicit precondition assertion after the loop (using
authCallbackCount and flushAsync) to fail the test if authCallbackCount < 2
before performing the RSA4c3 checks. Concretely, after the for-loop ensure you
assert something like "expect(authCallbackCount).to.be.at.least(2)" (or
throw/assert with a clear message) so the subsequent assertions on
client.connection.state, stateChanges, and client.connection.errorReason only
run when the reauth callback actually fired.
---
Nitpick comments:
In `@test/uts/deviations.md`:
- Around line 265-274: The RSH1b2 entry under the "integration/push_admin:
RSH1b2 - push device list pagination missing Link headers" heading in
test/uts/deviations.md documents a backend/server limitation rather than an SDK
deviation; move this entire entry into a new or existing "Server/Backend
Limitations" section (create one if missing), update any local TOC or index in
deviations.md to include the new section, and keep the content and the issue
link ([ably/realtime#8380]) intact with a short note that this is server-side
behavior rather than an SDK deviation.
- Around line 11-250: The deviations file documents several spec non-compliances
(RTC1a, RTL5k, RTN24, RTP17e, TM4, RTL4c, RTP18a) but lacks issue-tracking
links; update each section header (e.g. the "RTC1a - echoMessages default does
not send echo=true", "RTL5k - ATTACHED while detached does not send DETACH",
"RTN24 - connection.id/key not updated on UPDATE", "RTP17e - re-entry error
message missing clientId", "TM4 - toJSON not a method on Message", "RTL4c -
errorReason not cleared on successful re-attach", "RTP18a - new sync does not
discard in-flight sync") to include a link to the corresponding GH issue or, if
none exists, add a short note like "no issue filed" and optionally a tracking
TODO/issue number placeholder; ensure link format matches the existing linked
entries and keep the change limited to the deviation headings and a one-line
annotation under each affected block for traceability.
In `@test/uts/realtime/unit/auth/connection_auth.test.ts`:
- Around line 278-347: This test "RSA4c3 - authCallback error while CONNECTED
does not set errorReason" is a duplicate of the same case in
auth_callback_errors.test.ts; remove the duplicate test from
connection_auth.test.ts (delete the it(...) block that sets up MockWebSocket,
the authCallback increments, the auth request and assertions) and leave the
canonical test in auth_callback_errors.test.ts so coverage remains unchanged; if
any shared helpers or test setup were added only for this duplicate, move them
into the shared setup or into auth_callback_errors.test.ts instead and run the
test suite to confirm no regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4ddd97d-1693-4a9f-b5ba-44e99bf29423
📒 Files selected for processing (3)
test/uts/deviations.mdtest/uts/realtime/unit/auth/auth_callback_errors.test.tstest/uts/realtime/unit/auth/connection_auth.test.ts
| // Wait for the auth callback to be called a second time (the failure) | ||
| for (let i = 0; i < 10; i++) { | ||
| await flushAsync(); | ||
| if (authCallbackCount >= 2) break; | ||
| } | ||
|
|
||
| // RSA4c3: Connection remains CONNECTED | ||
| expect(client.connection.state).to.equal('connected'); | ||
|
|
||
| // No state changes at all — the auth failure is silently swallowed | ||
| expect(stateChanges).to.have.length(0); | ||
|
|
||
| // errorReason is NOT set (see specification#466) | ||
| expect(client.connection.errorReason).to.be.null; | ||
| }); |
There was a problem hiding this comment.
Same false-positive risk as the RSA4c3 case in connection_auth.test.ts.
If authCallbackCount never reaches 2 (e.g., AUTH handling regresses, fake timers/microtasks aren't being drained, etc.), the loop exits silently and the three subsequent assertions all hold trivially: state stays connected, stateChanges stays empty, and errorReason stays null. The test would then pass without actually exercising RSA4c3.
Assert the second invocation actually happened before checking the "no-op" behavior.
🛡️ Suggested fix
// Wait for the auth callback to be called a second time (the failure)
for (let i = 0; i < 10; i++) {
await flushAsync();
if (authCallbackCount >= 2) break;
}
+
+ // Sanity check: the reauth path actually invoked authCallback a second time
+ expect(authCallbackCount, 'reauth authCallback was not invoked').to.equal(2);
// RSA4c3: Connection remains CONNECTED
expect(client.connection.state).to.equal('connected');📝 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.
| // Wait for the auth callback to be called a second time (the failure) | |
| for (let i = 0; i < 10; i++) { | |
| await flushAsync(); | |
| if (authCallbackCount >= 2) break; | |
| } | |
| // RSA4c3: Connection remains CONNECTED | |
| expect(client.connection.state).to.equal('connected'); | |
| // No state changes at all — the auth failure is silently swallowed | |
| expect(stateChanges).to.have.length(0); | |
| // errorReason is NOT set (see specification#466) | |
| expect(client.connection.errorReason).to.be.null; | |
| }); | |
| // Wait for the auth callback to be called a second time (the failure) | |
| for (let i = 0; i < 10; i++) { | |
| await flushAsync(); | |
| if (authCallbackCount >= 2) break; | |
| } | |
| // Sanity check: the reauth path actually invoked authCallback a second time | |
| expect(authCallbackCount, 'reauth authCallback was not invoked').to.equal(2); | |
| // RSA4c3: Connection remains CONNECTED | |
| expect(client.connection.state).to.equal('connected'); | |
| // No state changes at all — the auth failure is silently swallowed | |
| expect(stateChanges).to.have.length(0); | |
| // errorReason is NOT set (see specification#466) | |
| expect(client.connection.errorReason).to.be.null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/uts/realtime/unit/auth/auth_callback_errors.test.ts` around lines 222 -
236, Ensure the test explicitly verifies the auth callback was invoked twice
before making no-op assertions: after the waiting loop using flushAsync, add an
assertion that authCallbackCount is at least 2 (e.g.,
expect(authCallbackCount).to.be.at.least(2)) and only then assert
client.connection.state === 'connected', stateChanges.length === 0, and
client.connection.errorReason === null; reference authCallbackCount, flushAsync,
client.connection.state, stateChanges, and client.connection.errorReason when
locating where to add the assertion.
| client.connection.once('disconnected', () => { | ||
| // RSA4c2: Connection transitioned to DISCONNECTED | ||
| expect(client.connection.state).to.equal('disconnected'); | ||
|
|
||
| // RSA4c1: errorReason has code 80019 | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| expect(client.connection.errorReason!.code).to.equal(80019); | ||
| expect(client.connection.errorReason!.statusCode).to.equal(401); | ||
|
|
||
| done(); | ||
| }); | ||
|
|
||
| // Also listen for FAILED in case ably-js treats this as fatal | ||
| client.connection.once('failed', () => { | ||
| // Some implementations may treat invalid format as fatal | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| done(); | ||
| }); | ||
|
|
||
| client.connect(); | ||
| }); |
There was a problem hiding this comment.
Both disconnected and failed once-handlers call done() — risk of double-call.
Mocha treats a second done() invocation as a test error ("done() called multiple times"). With .once, each handler still fires up to once, but the SDK can plausibly emit disconnected followed by failed (or vice versa) in some code paths, producing two done() calls and a confusing failure rather than a clean assertion. Same pattern repeats in the RSA4f oversized-token test at lines 461-477.
Guard with a single-shot flag (or pick the terminal state actually expected by spec).
🛡️ Suggested fix
it('RSA4f - authCallback returns invalid type transitions to DISCONNECTED', function (done) {
+ let finished = false;
+ const finish = (fn: () => void) => {
+ if (finished) return;
+ finished = true;
+ fn();
+ done();
+ };
...
- client.connection.once('disconnected', () => {
+ client.connection.once('disconnected', () => finish(() => {
// RSA4c2: Connection transitioned to DISCONNECTED
expect(client.connection.state).to.equal('disconnected');
...
- done();
- });
+ }));
- // Also listen for FAILED in case ably-js treats this as fatal
- client.connection.once('failed', () => {
- // Some implementations may treat invalid format as fatal
+ client.connection.once('failed', () => finish(() => {
expect(client.connection.errorReason).to.not.be.null;
- done();
- });
+ }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/uts/realtime/unit/auth/auth_callback_errors.test.ts` around lines 404 -
424, The test registers two once-handlers that both call done()
(client.connection.once('disconnected', ...) and
client.connection.once('failed', ...)), risking a double done() call if both
events occur; fix by introducing a single-shot guard variable (e.g., let
finished = false) or a helper function (e.g., function finish(err?) { if
(finished) return; finished = true; done(err); }) and replace direct done()
calls in the 'disconnected' and 'failed' handlers (and the similar RSA4f
oversized-token test handlers) with a call to that guard to ensure done() is
invoked only once.
| // Wait for the auth callback to be called a second time (the failure) | ||
| for (let i = 0; i < 10; i++) { | ||
| await flushAsync(); | ||
| if (client.connection.errorReason != null || stateChanges.length > 0) break; | ||
| if (authCallbackCount >= 2) break; | ||
| } | ||
|
|
||
| // RSA4c3: connection should remain CONNECTED | ||
| expect(client.connection.state).to.equal('connected'); | ||
|
|
||
| // No transitions away from connected | ||
| const nonConnected = stateChanges.filter((c: any) => c.current !== 'connected'); | ||
| expect(nonConnected).to.have.length(0); | ||
| // No state changes at all — the auth failure is silently swallowed | ||
| expect(stateChanges).to.have.length(0); | ||
|
|
||
| // RSA4c1: errorReason has code 80019 | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| expect(client.connection.errorReason!.code).to.equal(80019); | ||
| expect(client.connection.errorReason!.statusCode).to.equal(401); | ||
| expect(client.connection.errorReason!.cause).to.not.be.null; | ||
| expect((client.connection.errorReason!.cause as any).code).to.equal(50000); | ||
| // errorReason is NOT set (see specification#466) | ||
| expect(client.connection.errorReason).to.be.null; | ||
| }); |
There was a problem hiding this comment.
Polling loop silently passes the test if the second authCallback never runs.
The loop exits without failure if authCallbackCount never reaches 2 (e.g., if the AUTH message handling regresses or flushAsync doesn't drain the relevant timers). All three subsequent assertions (state === 'connected', empty stateChanges, errorReason === null) then pass trivially, giving a false positive for RSA4c3.
Add a precondition assertion that the failing reauth callback actually fired before validating "no state change".
🛡️ Suggested fix
// Wait for the auth callback to be called a second time (the failure)
for (let i = 0; i < 10; i++) {
await flushAsync();
if (authCallbackCount >= 2) break;
}
+
+ // Sanity check: the reauth flow actually triggered a second authCallback
+ expect(authCallbackCount, 'reauth authCallback was not invoked').to.equal(2);
// RSA4c3: connection should remain CONNECTED
expect(client.connection.state).to.equal('connected');📝 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.
| // Wait for the auth callback to be called a second time (the failure) | |
| for (let i = 0; i < 10; i++) { | |
| await flushAsync(); | |
| if (client.connection.errorReason != null || stateChanges.length > 0) break; | |
| if (authCallbackCount >= 2) break; | |
| } | |
| // RSA4c3: connection should remain CONNECTED | |
| expect(client.connection.state).to.equal('connected'); | |
| // No transitions away from connected | |
| const nonConnected = stateChanges.filter((c: any) => c.current !== 'connected'); | |
| expect(nonConnected).to.have.length(0); | |
| // No state changes at all — the auth failure is silently swallowed | |
| expect(stateChanges).to.have.length(0); | |
| // RSA4c1: errorReason has code 80019 | |
| expect(client.connection.errorReason).to.not.be.null; | |
| expect(client.connection.errorReason!.code).to.equal(80019); | |
| expect(client.connection.errorReason!.statusCode).to.equal(401); | |
| expect(client.connection.errorReason!.cause).to.not.be.null; | |
| expect((client.connection.errorReason!.cause as any).code).to.equal(50000); | |
| // errorReason is NOT set (see specification#466) | |
| expect(client.connection.errorReason).to.be.null; | |
| }); | |
| // Wait for the auth callback to be called a second time (the failure) | |
| for (let i = 0; i < 10; i++) { | |
| await flushAsync(); | |
| if (authCallbackCount >= 2) break; | |
| } | |
| // Sanity check: the reauth flow actually triggered a second authCallback | |
| expect(authCallbackCount, 'reauth authCallback was not invoked').to.equal(2); | |
| // RSA4c3: connection should remain CONNECTED | |
| expect(client.connection.state).to.equal('connected'); | |
| // No state changes at all — the auth failure is silently swallowed | |
| expect(stateChanges).to.have.length(0); | |
| // errorReason is NOT set (see specification#466) | |
| expect(client.connection.errorReason).to.be.null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/uts/realtime/unit/auth/connection_auth.test.ts` around lines 333 - 347,
The polling loop can silently pass if the second auth callback never ran; add an
explicit precondition assertion after the loop (using authCallbackCount and
flushAsync) to fail the test if authCallbackCount < 2 before performing the
RSA4c3 checks. Concretely, after the for-loop ensure you assert something like
"expect(authCallbackCount).to.be.at.least(2)" (or throw/assert with a clear
message) so the subsequent assertions on client.connection.state, stateChanges,
and client.connection.errorReason only run when the reauth callback actually
fired.
connectionId is a top-level ProtocolMessage field, not inside connectionDetails. RTN24's "connectionDetails must override stored details" does not apply to it — connection.id never changes for an in-progress connection. ably-js behavior was already correct. Remove deviation entry and deviation skip from test. Refs: #2208 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TM4 is about Message constructors (name, data) and (name, data, clientId), not serialization. Replace toJSON deviation test with constructor tests. Remove TM4 deviation entry — ably-js was correct. Refs: #2210 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, RSA17c) With X-Ably-Version >= 3 (ably-js sends version 6), the server returns a BatchResult envelope with HTTP 200 for all batch responses. Updated all mock responses from legacy format (plain arrays / batchResponse with HTTP 400) to the new format. Removed corresponding deviation entries — these were not real deviations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…l, RSC15l4) - Fix mock shouldFallback to handle statusCode 408 (request timeout) - Fix mock respond_with_timeout to use code: 'ETIMEDOUT' (matching Node) - Remove RSC15l deviation — test now passes (mock was wrong, not ably-js) - Add proxy integration tests for RSC15l2 (request timeout) and RSC15l4 (CloudFront header). RSC15l2 passes; RSC15l4 skipped as deviation. - Update deviations.md: narrow to RSC15l4 only Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/uts/rest/unit/types/message_types.test.ts (1)
200-204: ⚡ Quick win"name and data are nullable" test only verifies absence, not actual
nullacceptance.The spec point TM4 states that
nameanddataare nullable, but the test passes{}(omitted fields) and assertsundefined. This does not exercise the nullable contract — i.e., thatMessage.fromValues({ name: null, data: null })is valid. Consider adding a case that explicitly passesnullfor both fields.💡 Proposed addition
it('TM4 - name and data are nullable', function () { const msg = Message.fromValues({}); expect(msg.name).to.be.undefined; expect(msg.data).to.be.undefined; + + const msgWithNulls = Message.fromValues({ name: null, data: null }); + expect(msgWithNulls.name).to.be.null; + expect(msgWithNulls.data).to.be.null; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/rest/unit/types/message_types.test.ts` around lines 200 - 204, Update the TM4 test to explicitly verify null acceptance: add a case calling Message.fromValues({ name: null, data: null }) (referencing the Message.fromValues factory used in the test) and assert that the resulting msg.name and msg.data are null (not just undefined); keep the original omitted-fields check but augment it with this explicit null-input assertion so the nullable contract is exercised.test/uts/mock_http.ts (1)
15-21: ⚡ Quick winAlign
RequestResult.error.codetyping with the new symbolic codes.Line 130 sets
code: 'ETIMEDOUT'and is forced throughas anybecauseRequestResult.error.codeis declared asnumber(line 16), whileConnectionResult.error.codeis alreadystring(line 12). Widening the request-side type avoids the cast and prevents future drift between the two shapes.♻️ Proposed fix
interface RequestResult { - error: { message: string; code: number; statusCode: number } | null; + error: { message: string; code: number | string; statusCode: number } | null; body: string | null; headers: Record<string, string>; unpacked: boolean; statusCode: number; } @@ respond_with_timeout(): void { this._resolve!({ - error: { code: 'ETIMEDOUT', statusCode: 408, message: 'Request timed out' } as any, + error: { code: 'ETIMEDOUT', statusCode: 408, message: 'Request timed out' }, body: null, headers: {}, unpacked: false, statusCode: 408, }); }Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/mock_http.ts` around lines 15 - 21, The RequestResult interface currently declares error.code as number; change it to string | number (or at least string) to match ConnectionResult.error.code and remove the need for casts like `as any`; update the RequestResult declaration (the error field in RequestResult) so its code property is string | number and verify call sites that set code (e.g., where `code: 'ETIMEDOUT'` is used) no longer require casts.test/uts/rest/integration/proxy/rest_fallback.test.ts (1)
61-75: 💤 Low valueOptional: extract the duplicated Rest/authCallback setup.
Both tests build the same nested-Rest authCallback and
Ably.Restconfig, differing only inhttpRequestTimeout. A small helper (e.g.,makeProxiedRest(session, overrides)) would reduce duplication and make future RSC15l* additions easier.Also applies to: 114-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/uts/rest/integration/proxy/rest_fallback.test.ts` around lines 61 - 75, Tests duplicate the same nested Ably.Rest authCallback and configuration (only httpRequestTimeout varies); extract a helper (e.g., makeProxiedRest(session, overrides)) that builds the innerRest authCallback and constructs the outer new Ably.Rest with defaults (endpoint:'localhost', fallbackHosts:['localhost'], port: session.proxyPort, tls:false, useBinaryProtocol:false, and any overrides such as httpRequestTimeout) and replace both test usages (the blocks creating restClient) to call makeProxiedRest(session, { httpRequestTimeout: ... }) so future tests can change only overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/uts/mock_http.ts`:
- Around line 15-21: The RequestResult interface currently declares error.code
as number; change it to string | number (or at least string) to match
ConnectionResult.error.code and remove the need for casts like `as any`; update
the RequestResult declaration (the error field in RequestResult) so its code
property is string | number and verify call sites that set code (e.g., where
`code: 'ETIMEDOUT'` is used) no longer require casts.
In `@test/uts/rest/integration/proxy/rest_fallback.test.ts`:
- Around line 61-75: Tests duplicate the same nested Ably.Rest authCallback and
configuration (only httpRequestTimeout varies); extract a helper (e.g.,
makeProxiedRest(session, overrides)) that builds the innerRest authCallback and
constructs the outer new Ably.Rest with defaults (endpoint:'localhost',
fallbackHosts:['localhost'], port: session.proxyPort, tls:false,
useBinaryProtocol:false, and any overrides such as httpRequestTimeout) and
replace both test usages (the blocks creating restClient) to call
makeProxiedRest(session, { httpRequestTimeout: ... }) so future tests can change
only overrides.
In `@test/uts/rest/unit/types/message_types.test.ts`:
- Around line 200-204: Update the TM4 test to explicitly verify null acceptance:
add a case calling Message.fromValues({ name: null, data: null }) (referencing
the Message.fromValues factory used in the test) and assert that the resulting
msg.name and msg.data are null (not just undefined); keep the original
omitted-fields check but augment it with this explicit null-input assertion so
the nullable contract is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd08788f-7206-4c54-bb40-9bb166fd0f0c
📒 Files selected for processing (8)
test/uts/deviations.mdtest/uts/mock_http.tstest/uts/realtime/unit/connection/update_events.test.tstest/uts/rest/integration/proxy/rest_fallback.test.tstest/uts/rest/unit/auth/revoke_tokens.test.tstest/uts/rest/unit/batch_presence.test.tstest/uts/rest/unit/fallback.test.tstest/uts/rest/unit/types/message_types.test.ts
Adds tests for unreachable endpoint (ECONNREFUSED), connection drop (http_drop), 5xx with/without error body, 4xx not retried, and RSL1k4 idempotent publish (skipped pending proxy response modification support). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Every ably-js UTS test now has a `// UTS: <test-id>` comment mapping it to its corresponding UTS spec. All 1120 UTS Test IDs are covered (1226 total JS tests, with some specs covered by multiple tests via .N suffixes). New tests added across 13 files: - Fallback/endpoint config (REC): 10 tests in fallback.test.ts - REST request handling (RSC): 8 tests across request/rest_client/channel - Batch publish (RSC22): 6 tests in batch_publish.test.ts - Auth token lifecycle (RSA16): 4 tests in token_details.test.ts - Presence get/history (RSP): 14 tests in rest_presence.test.ts - Pagination (TG): 3 tests in paginated_result.test.ts - Presence message types (TP3): 2 tests in presence_message_types.test.ts - Realtime reconnect (RTN23b): 1 test in heartbeat.test.ts - Msgpack deviations: 4 skip stubs across encoding/presence/channel/auth - Integration (RSA7): 1 test in auth.test.ts 18 tests are it.skip() documenting deviations (msgpack not supported, fallbackHostsUseDefault not implemented, connectivity check not accessible, addRequestIds not implemented, switch-to-basic-auth not supported). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Unit tests: endpoint: 'sandbox' → 'test' (not a real environment) - Update host/fallback assertions to match (test.realtime.ably.net) - Proxy helper: use nonprod sandbox hostnames Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add describeEachProtocol helper and update all 10 data-path integration test files to run with both protocol variants. Each test receives the protocol and sets useBinaryProtocol accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify decode and round-trip of all 8 ably-common msgpack fixtures (strings, binary, JSON array/object at various sizes). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
ensureProxy()in the test setup — no external startup needed.Proxy approach
The Go test proxy (source in the
specificationrepo atuts/proxy/) sits between the SDK and the Ably sandbox. Each test creates a proxy session with declarative rules that match on message type/action/channel/HTTP method and fire actions (drop connection, inject error, delay, replace message). After the test, the proxy's event log is queried to verify what happened on the wire.Infrastructure (
test/uts/realtime/integration/helpers/proxy.ts)ensureProxy()— idempotent: checks if proxy is already running via/health, builds the Go binary if needed (go build), spawns it viachild_process.spawn, polls until ready, kills onprocess.exitcreateProxySession(opts)— creates a session targeting sandbox-realtime/sandbox-rest on an allocated port, returns aProxySessionwithaddRules(),triggerAction(),getLog(),close()localhost:{proxyPort}with TLS disabled, so all traffic flows through the proxyProxy integration tests added
connection_resume.test.tschannel_faults.test.tsheartbeat.test.tsrest_faults.test.tsauth_reauth.test.tspresence_reentry.test.tsconnection_open_failures.test.tspackage.jsonchangestest:utsnow runs all tests including proxy (proxy auto-starts)test:uts:unitexcludes bothproxy/andintegration/directoriestest:uts:proxyscriptMissing unit/integration tests added
backoff_jitter.test.tstoken_expiry_non_renewable.test.tsauth_callback_errors.test.tsconnection_recovery.test.tsforwards_compatibility.test.tsnetwork_change.test.tspush_channels.test.ts(unit)push_channels.test.ts(integration)channel_subscribe.test.ts(extended)channel_publish.test.ts(extended)rest_channel_attributes.test.ts(extended)connection_resume.test.ts(extended)Test plan
npm run test:uts:unit -- --timeout 30000— all unit tests passnpm run test:uts -- --timeout 30000— unit + integration + proxy tests (requires Go installed for proxy build)🤖 Generated with Claude Code
Summary by CodeRabbit