DX-1209: inline fix-it hints on SDK ErrorInfo throw sites#2233
Open
umair-ably wants to merge 19 commits into
Open
DX-1209: inline fix-it hints on SDK ErrorInfo throw sites#2233umair-ably wants to merge 19 commits into
umair-ably wants to merge 19 commits into
Conversation
|
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:
WalkthroughThis PR introduces a comprehensive error-hint infrastructure: a static TypeScript AST scanner validates that error sites include hints matching a hardcoded rubric, integrates into CI via npm script, extends ErrorInfo/PartialErrorInfo constructors to accept object-form initialization with optional ChangesError Hint Coverage Infrastructure and Rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
umair-ably
commented
May 27, 2026
| 'use strict'; | ||
|
|
||
| /* | ||
| * DX-1209 — hint pinning tests for inline SDK throw sites. |
Contributor
Author
There was a problem hiding this comment.
I'm not bedded to the hint-coverage script and/or this test file but we need something to test the presence of hints... I'm leaning more towards the script as opposed to exhaustively testing all the hints here (although these few as a smokescreen don't hurt). Happy to hear your thoughts here @AndyTWF
umair-ably
force-pushed
the
DX-1205/legacy-import-shims
branch
from
0e65931 to
1ff768b
Compare
umair-ably
force-pushed
the
DX-1209/error-hints
branch
from
e326daa to
89ec75a
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
scripts/hint-coverage.ts (3)
111-113: 💤 Low valueBlock comment stripping may mishandle strings containing
/*or*/.The regex-based block comment removal can incorrectly strip content from string literals that contain
/*or*/sequences (e.g.,const url = "https://example.com/*path*/"). This could cause false negatives if hint assignments are within such strings or false positives if code is incorrectly stripped.For a static lint tool, this edge case is unlikely to affect actual hint validation in this codebase, but worth noting.
🤖 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 `@scripts/hint-coverage.ts` around lines 111 - 113, The stripBlockComments function uses a regex that will remove /*...*/ sequences even when they occur inside string or template literals; replace this with a safe scanner or tokenizer that walks the source and only removes block comments when not inside single-quoted, double-quoted, or template literal strings (and should also respect escaped quotes and backticks), or use an existing JS/TS tokenizer (e.g., Acorn/TypeScript scanner) to detect comment tokens; update stripBlockComments to implement this stateful scan/token-based approach so it ignores /*...*/ inside strings and only strips real block comments.
233-236: 💤 Low valueConsider using
flatMapfor cleaner collection.Minor style suggestion: the repeated
concatin a loop can be replaced withflatMapfor a more idiomatic approach.♻️ Optional refactor
- let allEntries: HintEntry[] = []; - for (const f of files) { - allEntries = allEntries.concat(extractHints(f)); - } + const allEntries: HintEntry[] = files.flatMap(extractHints);🤖 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 `@scripts/hint-coverage.ts` around lines 233 - 236, Replace the manual accumulation loop that builds allEntries by repeatedly concat-ing extractHints results with a single idiomatic flatMap: call files.flatMap(f => extractHints(f)) and assign that to allEntries, keeping the variable name allEntries and using the existing extractHints function so behavior remains identical but the code is clearer and more concise.
148-155: 💤 Low valueMultiline hint assembly may fail if semicolon appears inside the string literal.
The loop termination condition checks if the line ends with
;, but if a hint string contains a semicolon near a line break (e.g.,"Fix; see docs"split across lines), the loop could terminate prematurely. Additionally, the trailing semicolon removal on line 155 happens afterjoin(' ')but beforetrim(), which works correctly.Given hint strings are developer-controlled and this is an internal lint tool, the impact is low—but malformed hints would silently pass validation.
🤖 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 `@scripts/hint-coverage.ts` around lines 148 - 155, The current multiline assembly for the RHS (rhsParts, rhs, hintMatch, lines) stops when a line ends with ';' which can be inside a string literal; change the loop to scan until you find a statement-terminating semicolon that is not inside a string/template literal or escaped sequence: maintain a small state machine (inSingleQuote, inDoubleQuote, inBacktick, escaped) while walking characters across lines (or join progressively) and only break when you see an unescaped semicolon outside any quote state; then strip the trailing semicolon only if it was that terminating semicolon (i.e., not part of a string) before trim().src/plugins/push/pushactivation.ts (1)
65-70: ⚡ Quick winRemove unnecessary block braces.
The braces wrapping the error construction and hint assignment serve no functional purpose. This pattern appears throughout the file.
♻️ Proposed fix
- { - const err = new this.rest.ErrorInfo('Push activation is not available on this platform', 40000, 400); - err.hint = - 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; - throw err; - } + const err = new this.rest.ErrorInfo('Push activation is not available on this platform', 40000, 400); + err.hint = + 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; + throw err;🤖 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 `@src/plugins/push/pushactivation.ts` around lines 65 - 70, Remove the unnecessary standalone block braces around the error construction and hint assignment in the push activation path; locate the push.activate implementation in pushactivation.ts where new this.rest.ErrorInfo(...) is created and the err.hint is set, and replace the braced block with the same statements directly (create err, set err.hint, then throw err) — apply the same removal for other occurrences in this file where a braced-only block wraps an ErrorInfo creation/throw.src/common/lib/client/push.ts (2)
40-45: ⚡ Quick winRemove unnecessary block braces.
The braces wrapping the error construction and hint assignment serve no functional purpose and add unnecessary nesting.
♻️ Proposed fix
- { - const err = new ErrorInfo('This platform is not supported as a target of push notifications', 40000, 400); - err.hint = - 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; - reject(err); - } + const err = new ErrorInfo('This platform is not supported as a target of push notifications', 40000, 400); + err.hint = + 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; + reject(err);🤖 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 `@src/common/lib/client/push.ts` around lines 40 - 45, The extra block braces around the ErrorInfo construction and reject call are unnecessary; remove the surrounding { } so the code directly constructs the ErrorInfo, sets err.hint, and calls reject(err). Locate the snippet that creates new ErrorInfo('This platform is not supported as a target of push notifications', 40000, 400) and the subsequent err.hint assignment inside the push.activate() / push registration failure branch and simplify it by eliminating the enclosing braces so the flow is not nested needlessly.
76-81: ⚡ Quick winRemove unnecessary block braces.
The braces wrapping the error construction and hint assignment serve no functional purpose. Same issue as lines 40-45.
♻️ Proposed fix
- { - const err = new ErrorInfo('This platform is not supported as a target of push notifications', 40000, 400); - err.hint = - 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; - reject(err); - } + const err = new ErrorInfo('This platform is not supported as a target of push notifications', 40000, 400); + err.hint = + 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; + reject(err);🤖 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 `@src/common/lib/client/push.ts` around lines 76 - 81, Remove the unnecessary block braces around the error creation in the push activation failure path: inline the ErrorInfo construction, set its hint, and call reject(err) directly instead of wrapping these statements in an extra { ... } block; locate the code in src/common/lib/client/push.ts inside the push activation flow (the branch that constructs new ErrorInfo(...), assigns err.hint and calls reject(err)) and mirror the same fix already applied for the earlier occurrence around the other error block (lines ~40-45).test/unit/error-hints.test.js (2)
19-28: ⚡ Quick winConsider asserting
err.hintexistence before substring checks.All five test cases check
err.hint.to.contain(...)without first asserting thaterr.hintis defined. Iferr.hintis unexpectedlyundefined, the test will fail with a TypeError rather than a clear assertion failure.🛡️ Proposed fix to add hint existence checks
For each test, add an assertion before the substring checks:
} catch (err) { expect(err.code).to.equal(40400); + expect(err.hint).to.be.a('string'); expect(err.hint).to.contain('appId'); expect(err.hint).to.contain('keyId'); }Apply the same pattern to all five test cases (lines 24-27, 35-37, 45-48, 56-59, 70-73).
Also applies to: 30-38, 40-49, 51-60, 64-75
🤖 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/unit/error-hints.test.js` around lines 19 - 28, The tests call expect(err.hint).to.contain(...) without ensuring err.hint exists; update each test case (e.g., the "invalid key format (40400) carries a key-format hint" block where err is caught after new Ably.Rest(...)) to first assert that err.hint is defined (e.g., expect(err).to.have.property('hint') or expect(err.hint).to.exist) before the subsequent substring checks; apply the same change to the other four test blocks that catch err so the substring assertions never run when hint is undefined.
67-67: 💤 Low valuePrivate member access may be fragile across refactors.
The test accesses
rest._FilteredSubscriptions, which is a private implementation detail. While acceptable for validating the missing-plugin error path, this coupling may break if the SDK renames or restructures the getter.Consider adding a comment documenting that
_FilteredSubscriptionsis the stable trigger forcreateMissingPluginError('MessageInteractions'), or verify whether there's a public API path that triggers the same error.🤖 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/unit/error-hints.test.js` at line 67, The test is fragile because it accesses the private member rest._FilteredSubscriptions to trigger createMissingPluginError('MessageInteractions'); either replace this with a public API call that provokes the same missing-plugin error (if one exists) or, if no public trigger is available, add a short inline comment immediately above the rest._FilteredSubscriptions access documenting that this specific private getter is the intentional, stable trigger for createMissingPluginError('MessageInteractions') and why the test relies on it (to prevent future refactors from removing the usage unintentionally).scripts/moduleReport.ts (1)
9-9: ⚡ Quick winDocument the rationale for threshold increase.
The threshold increased by ~9 KiB raw and ~3 KiB gzipped to accommodate the hint string additions, but there's no inline comment explaining this. Future maintainers may not understand why these specific values were chosen.
📝 Proposed addition
-// The maximum size we allow for a minimal useful Realtime bundle (i.e. one that can subscribe to a channel) +// The maximum size we allow for a minimal useful Realtime bundle (i.e. one that can subscribe to a channel). +// Thresholds updated in DX-1209 to accommodate ~80 inline err.hint strings across SDK throw sites. const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 116, gzip: 36 };🤖 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 `@scripts/moduleReport.ts` at line 9, Add an inline comment above the minimalUsefulRealtimeBundleSizeThresholdsKiB constant documenting why the thresholds were raised: note that raw increased by ~9 KiB and gzip by ~3 KiB to accommodate recent hint string additions, reference the related PR/commit or feature that introduced the hint strings, and include guidance on how to recalibrate these numbers if the hint size changes (e.g., measuring bundle sizes before/after removing hints). This comment should live next to minimalUsefulRealtimeBundleSizeThresholdsKiB so future maintainers understand the specific rationale and how to update the values.
🤖 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 `@src/common/lib/client/realtimeannotations.ts`:
- Line 72: Fix the typo in the inline comment referencing the attach state:
change "caes" to "case" in the comment near the attachOnSubscribe handling (the
comment that starts "// explicit check for attach state in caes
attachOnSubscribe=false") in realtimeannotations.ts so it reads "// explicit
check for attach state in case attachOnSubscribe=false".
In `@src/plugins/push/getW3CDeviceDetails.ts`:
- Around line 31-34: The ErrorInfo constructor arguments are swapped in the push
device flow: when creating the ErrorInfo for denied Notification permission in
getW3CDeviceDetails.ts you passed (message, 400, 40000) but ErrorInfo expects
(message, code, statusCode); update the call to pass the error code first and
the HTTP status second (i.e., use (message, 40000, 400)) so the
GettingPushDeviceDetailsFailed event (created and sent via machine.handleEvent)
contains the correct code and statusCode.
- Around line 40-43: The ErrorInfo constructor arguments are swapped in the new
ErrorInfo call inside getW3CDeviceDetails; replace the current arguments so they
match the expected (message, code, statusCode) order—i.e., construct ErrorInfo
with the numeric error code 40000 as the second argument and the HTTP status
code 400 as the third—then continue to set err.hint and dispatch via
machine.handleEvent(new GettingPushDeviceDetailsFailed(err)).
---
Nitpick comments:
In `@scripts/hint-coverage.ts`:
- Around line 111-113: The stripBlockComments function uses a regex that will
remove /*...*/ sequences even when they occur inside string or template
literals; replace this with a safe scanner or tokenizer that walks the source
and only removes block comments when not inside single-quoted, double-quoted, or
template literal strings (and should also respect escaped quotes and backticks),
or use an existing JS/TS tokenizer (e.g., Acorn/TypeScript scanner) to detect
comment tokens; update stripBlockComments to implement this stateful
scan/token-based approach so it ignores /*...*/ inside strings and only strips
real block comments.
- Around line 233-236: Replace the manual accumulation loop that builds
allEntries by repeatedly concat-ing extractHints results with a single idiomatic
flatMap: call files.flatMap(f => extractHints(f)) and assign that to allEntries,
keeping the variable name allEntries and using the existing extractHints
function so behavior remains identical but the code is clearer and more concise.
- Around line 148-155: The current multiline assembly for the RHS (rhsParts,
rhs, hintMatch, lines) stops when a line ends with ';' which can be inside a
string literal; change the loop to scan until you find a statement-terminating
semicolon that is not inside a string/template literal or escaped sequence:
maintain a small state machine (inSingleQuote, inDoubleQuote, inBacktick,
escaped) while walking characters across lines (or join progressively) and only
break when you see an unescaped semicolon outside any quote state; then strip
the trailing semicolon only if it was that terminating semicolon (i.e., not part
of a string) before trim().
In `@scripts/moduleReport.ts`:
- Line 9: Add an inline comment above the
minimalUsefulRealtimeBundleSizeThresholdsKiB constant documenting why the
thresholds were raised: note that raw increased by ~9 KiB and gzip by ~3 KiB to
accommodate recent hint string additions, reference the related PR/commit or
feature that introduced the hint strings, and include guidance on how to
recalibrate these numbers if the hint size changes (e.g., measuring bundle sizes
before/after removing hints). This comment should live next to
minimalUsefulRealtimeBundleSizeThresholdsKiB so future maintainers understand
the specific rationale and how to update the values.
In `@src/common/lib/client/push.ts`:
- Around line 40-45: The extra block braces around the ErrorInfo construction
and reject call are unnecessary; remove the surrounding { } so the code directly
constructs the ErrorInfo, sets err.hint, and calls reject(err). Locate the
snippet that creates new ErrorInfo('This platform is not supported as a target
of push notifications', 40000, 400) and the subsequent err.hint assignment
inside the push.activate() / push registration failure branch and simplify it by
eliminating the enclosing braces so the flow is not nested needlessly.
- Around line 76-81: Remove the unnecessary block braces around the error
creation in the push activation failure path: inline the ErrorInfo construction,
set its hint, and call reject(err) directly instead of wrapping these statements
in an extra { ... } block; locate the code in src/common/lib/client/push.ts
inside the push activation flow (the branch that constructs new ErrorInfo(...),
assigns err.hint and calls reject(err)) and mirror the same fix already applied
for the earlier occurrence around the other error block (lines ~40-45).
In `@src/plugins/push/pushactivation.ts`:
- Around line 65-70: Remove the unnecessary standalone block braces around the
error construction and hint assignment in the push activation path; locate the
push.activate implementation in pushactivation.ts where new
this.rest.ErrorInfo(...) is created and the err.hint is set, and replace the
braced block with the same statements directly (create err, set err.hint, then
throw err) — apply the same removal for other occurrences in this file where a
braced-only block wraps an ErrorInfo creation/throw.
In `@test/unit/error-hints.test.js`:
- Around line 19-28: The tests call expect(err.hint).to.contain(...) without
ensuring err.hint exists; update each test case (e.g., the "invalid key format
(40400) carries a key-format hint" block where err is caught after new
Ably.Rest(...)) to first assert that err.hint is defined (e.g.,
expect(err).to.have.property('hint') or expect(err.hint).to.exist) before the
subsequent substring checks; apply the same change to the other four test blocks
that catch err so the substring assertions never run when hint is undefined.
- Line 67: The test is fragile because it accesses the private member
rest._FilteredSubscriptions to trigger
createMissingPluginError('MessageInteractions'); either replace this with a
public API call that provokes the same missing-plugin error (if one exists) or,
if no public trigger is available, add a short inline comment immediately above
the rest._FilteredSubscriptions access documenting that this specific private
getter is the intentional, stable trigger for
createMissingPluginError('MessageInteractions') and why the test relies on it
(to prevent future refactors from removing the usage unintentionally).
🪄 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: b1d70444-e652-4027-8bca-25c66c751fa5
📒 Files selected for processing (25)
.github/workflows/check.ymlpackage.jsonscripts/hint-coverage.tsscripts/moduleReport.tssrc/common/lib/client/auth.tssrc/common/lib/client/baseclient.tssrc/common/lib/client/baserealtime.tssrc/common/lib/client/paginatedresource.tssrc/common/lib/client/push.tssrc/common/lib/client/realtimeannotations.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/client/rest.tssrc/common/lib/client/restannotations.tssrc/common/lib/client/restchannel.tssrc/common/lib/client/restchannelmixin.tssrc/common/lib/transport/connectionmanager.tssrc/common/lib/types/basemessage.tssrc/common/lib/util/defaults.tssrc/common/lib/util/utils.tssrc/plugins/liveobjects/realtimeobject.tssrc/plugins/push/getW3CDeviceDetails.tssrc/plugins/push/pushactivation.tssrc/plugins/push/pushchannel.tstest/unit/error-hints.test.js
2 tasks
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
umair-ably
force-pushed
the
DX-1209/error-hints
branch
from
4886d8d to
89ec75a
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/push/pushactivation.ts (1)
256-268:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing return statement causes undefined deviceRegistration to propagate.
Lines 257-260 create an error and call
handleEvent(new GettingDeviceRegistrationFailed(err))whendeviceRegistrationis missing, but execution continues to line 263 wheredeviceRegistrationis cast toanyand passed toGotDeviceRegistration. This will corrupt the activation state machine with an undefined registration.🐛 Required fix: add return statement
if (!deviceRegistration) { const err = new this.client.ErrorInfo('registerCallback did not return deviceRegistration', 40000, 400); err.hint = 'Your registerCallback must invoke its callback with (null, deviceRegistration). Returning undefined or null in the second argument fails activation.'; this.handleEvent(new GettingDeviceRegistrationFailed(err)); + return; } if (isNew) {🤖 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 `@src/plugins/push/pushactivation.ts` around lines 256 - 268, The code checks for a missing deviceRegistration and constructs an ErrorInfo then calls this.handleEvent(new GettingDeviceRegistrationFailed(err)) but does not stop execution, allowing deviceRegistration to be passed into this.handleEvent(new GotDeviceRegistration(deviceRegistration as any)); add an early return immediately after emitting GettingDeviceRegistrationFailed(err) to prevent further execution (so the missing deviceRegistration won't be used to emit GotDeviceRegistration or RegistrationSynced); update the block around the deviceRegistration check in pushactivation.ts (the if (!deviceRegistration) branch) to return after calling handleEvent.
🧹 Nitpick comments (2)
src/plugins/push/pushactivation.ts (1)
65-70: 💤 Low valueConsider extracting duplicated hint string to a constant.
The same hint text for "push activation is not available on this platform" is repeated at four throw sites. Extracting it to a module-level constant would improve maintainability and ensure consistency if the wording needs to evolve.
♻️ Suggested refactor
At the top of the file, after imports:
+const HINT_PUSH_ACTIVATION_UNAVAILABLE = + 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.';Then replace each occurrence:
const err = new this.rest.ErrorInfo('Push activation is not available on this platform', 40000, 400); - err.hint = - 'push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts (there is no device to register). Use client.push.admin to manage other devices from a server: client.push.admin.publish(recipient, payload) to send to a device or clientId, client.push.admin.deviceRegistrations.save(device) to register a device record.'; + err.hint = HINT_PUSH_ACTIVATION_UNAVAILABLE;Also applies to: 107-112, 133-138, 214-221
🤖 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 `@src/plugins/push/pushactivation.ts` around lines 65 - 70, The hint string explaining that "push activation is not available on this platform" is duplicated across multiple throw sites; extract it into a module-level constant (e.g., PUSH_ACTIVATION_NOT_AVAILABLE_HINT) near the top of src/plugins/push/pushactivation.ts and replace the repeated inline assignments to err.hint with that constant in the places where new this.rest.ErrorInfo(...) is created (references: the throw blocks that construct this.rest.ErrorInfo and set err.hint). Ensure the constant name is descriptive and used in all four throw locations so the message is centralized for maintainability.LLMEval/DX-1209/README.md (1)
18-20: 💤 Low valueAdd language identifier to fenced code block.
The code block should specify
bashas the language for proper syntax highlighting and to satisfy the markdownlint rule.📝 Suggested fix
-``` +```bash ./run-flip-tests.sh <path-to-built-ably-js></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@LLMEval/DX-1209/README.mdaround lines 18 - 20, Update the fenced code block
that currently shows "./run-flip-tests.sh " to include
the bash language identifier (i.e., change the opening backticks to ```bash) so
the snippet is treated as bash and satisfies markdownlint; locate the code block
in README.md containing the ./run-flip-tests.sh invocation and update its
opening fence accordingly.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@LLMEval/DX-1209/flip-tests.test.js:
- Around line 17-18: This test file intentionally uses CommonJS require() (see
usage around the tests validating legacy imports throwing in flip-tests.test.js)
so suppress the false-positive rule by adding an ESLint disable for
@typescript-eslint/no-var-requiresat the top of the file (or alternatively
exclude LLMEval/**/*.js from TypeScript ESLint in your ESLint config); ensure
the disable affects the whole file so the require() calls in the tests (the
blocks that import legacy modules and the main SDK export) are not flagged.
Outside diff comments:
In@src/plugins/push/pushactivation.ts:
- Around line 256-268: The code checks for a missing deviceRegistration and
constructs an ErrorInfo then calls this.handleEvent(new
GettingDeviceRegistrationFailed(err)) but does not stop execution, allowing
deviceRegistration to be passed into this.handleEvent(new
GotDeviceRegistration(deviceRegistration as any)); add an early return
immediately after emitting GettingDeviceRegistrationFailed(err) to prevent
further execution (so the missing deviceRegistration won't be used to emit
GotDeviceRegistration or RegistrationSynced); update the block around the
deviceRegistration check in pushactivation.ts (the if (!deviceRegistration)
branch) to return after calling handleEvent.
Nitpick comments:
In@LLMEval/DX-1209/README.md:
- Around line 18-20: Update the fenced code block that currently shows
"./run-flip-tests.sh " to include the bash language
identifier (i.e., change the opening backticks to ```bash) so the snippet is
treated as bash and satisfies markdownlint; locate the code block in README.md
containing the ./run-flip-tests.sh invocation and update its opening fence
accordingly.In
@src/plugins/push/pushactivation.ts:
- Around line 65-70: The hint string explaining that "push activation is not
available on this platform" is duplicated across multiple throw sites; extract
it into a module-level constant (e.g., PUSH_ACTIVATION_NOT_AVAILABLE_HINT) near
the top of src/plugins/push/pushactivation.ts and replace the repeated inline
assignments to err.hint with that constant in the places where new
this.rest.ErrorInfo(...) is created (references: the throw blocks that construct
this.rest.ErrorInfo and set err.hint). Ensure the constant name is descriptive
and used in all four throw locations so the message is centralized for
maintainability.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `ec652704-121e-478c-b7c3-4b0864be458d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e326daae0e5716c7fe04f95d8be75d2074281d26 and 4886d8d5e3a7c6ec3501104067f01b06e95d52dd. </details> <details> <summary>📒 Files selected for processing (28)</summary> * `.github/workflows/check.yml` * `LLMEval/DX-1209/README.md` * `LLMEval/DX-1209/flip-tests.test.js` * `LLMEval/DX-1209/run-flip-tests.sh` * `package.json` * `scripts/hint-coverage.ts` * `scripts/moduleReport.ts` * `src/common/lib/client/auth.ts` * `src/common/lib/client/baseclient.ts` * `src/common/lib/client/baserealtime.ts` * `src/common/lib/client/paginatedresource.ts` * `src/common/lib/client/push.ts` * `src/common/lib/client/realtimeannotations.ts` * `src/common/lib/client/realtimechannel.ts` * `src/common/lib/client/realtimepresence.ts` * `src/common/lib/client/rest.ts` * `src/common/lib/client/restannotations.ts` * `src/common/lib/client/restchannel.ts` * `src/common/lib/client/restchannelmixin.ts` * `src/common/lib/transport/connectionmanager.ts` * `src/common/lib/types/basemessage.ts` * `src/common/lib/util/defaults.ts` * `src/common/lib/util/utils.ts` * `src/plugins/liveobjects/realtimeobject.ts` * `src/plugins/push/getW3CDeviceDetails.ts` * `src/plugins/push/pushactivation.ts` * `src/plugins/push/pushchannel.ts` * `test/unit/error-hints.test.js` </details> <details> <summary>✅ Files skipped from review due to trivial changes (4)</summary> * src/common/lib/client/paginatedresource.ts * src/common/lib/client/baserealtime.ts * package.json * src/common/lib/client/realtimepresence.ts </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (20)</summary> * .github/workflows/check.yml * src/common/lib/client/restchannel.ts * src/common/lib/client/realtimeannotations.ts * src/common/lib/client/restchannelmixin.ts * src/common/lib/util/utils.ts * src/plugins/liveobjects/realtimeobject.ts * src/common/lib/types/basemessage.ts * scripts/moduleReport.ts * src/common/lib/client/rest.ts * src/plugins/push/pushchannel.ts * src/common/lib/client/baseclient.ts * test/unit/error-hints.test.js * src/common/lib/client/push.ts * src/common/lib/client/restannotations.ts * src/common/lib/util/defaults.ts * src/common/lib/transport/connectionmanager.ts * src/plugins/push/getW3CDeviceDetails.ts * scripts/hint-coverage.ts * src/common/lib/client/realtimechannel.ts * src/common/lib/client/auth.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+17
to
+18
| const test = require('node:test'); | ||
| const assert = require('node:assert'); |
There was a problem hiding this comment.
Suppress or exclude TypeScript ESLint rule for this CommonJS test file.
The @typescript-eslint/no-var-requires warnings are false positives. This test file deliberately uses CommonJS require() to validate import-time throw behavior for DX-1205 (lines 24-46 test that legacy imports throw) and the main SDK export. Using ES6 import would break the test design since imports are hoisted and evaluated before test code executes.
🔧 Recommended fix
Add an ESLint disable comment at the top of the file:
// Deterministic flip tests for DX-1205 + DX-1209.
//
// Each test asserts the contractual shape of the new ErrorInfo.hint field
// (and, for DX-1205, the existence of the import-time throw at all). Run
// against three SDK builds:
//
// pre-all (4d23cdeb^): no DX-1205, no DX-1209 → every test FAILS
// pre-dx1209 (0e65931c): DX-1205 only → DX-1205 tests PASS,
// DX-1209 tests FAIL
// local (89ec75a7): both PRs → every test PASSES
//
// Driven from outside; this file just expects `node_modules/ably` to point
// at whichever SDK build is under test.
+/* eslint-disable `@typescript-eslint/no-var-requires` */
+
'use strict';Alternatively, exclude LLMEval/**/*.js from TypeScript ESLint rules in your ESLint configuration.
Also applies to: 50-50
🧰 Tools
🪛 ESLint
[error] 17-17: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 18-18: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🪛 GitHub Actions: Lint / 0_lint.txt
[error] 17-14: ESLint failed (@typescript-eslint/no-var-requires): Require statement not part of import statement.
🪛 GitHub Actions: Lint / lint
[error] 17-17: ESLint (rule: @typescript-eslint/no-var-requires): Require statement not part of import statement.
🪛 GitHub Check: lint
[failure] 18-18:
Require statement not part of import statement
[failure] 17-17:
Require statement not part of import statement
🤖 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 `@LLMEval/DX-1209/flip-tests.test.js` around lines 17 - 18, This test file
intentionally uses CommonJS require() (see usage around the tests validating
legacy imports throwing in flip-tests.test.js) so suppress the false-positive
rule by adding an ESLint disable for `@typescript-eslint/no-var-requires` at the
top of the file (or alternatively exclude LLMEval/**/*.js from TypeScript ESLint
in your ESLint config); ensure the disable affects the whole file so the
require() calls in the tests (the blocks that import legacy modules and the main
SDK export) are not flagged.
1ff768b to
924f85a
Compare
AndyTWF
reviewed
May 27, 2026
AndyTWF
left a comment
AndyTWF
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this. Generally - we have a fair number of hints that are pretty much re-wordings of the actual error messages. Are hints necessary in this case if they don't add additional information/substance, or are they just noise? Otherwise we could trim down the message and instead lean more heavily on the hint.
| { | ||
| const err = new ErrorInfo('Unable to update auth options with incompatible key', 40102, 401); | ||
| err.hint = | ||
| 'auth.authorize() cannot change the API key — the new authOptions.key differs from the one the client was constructed with. To use a different key, construct a new Ably client.'; |
Contributor
There was a problem hiding this comment.
Regular hyphens over em-dashes
| cb(new ErrorInfo('authUrl response is missing a content-type header', 40170, 401), null); | ||
| const err = new ErrorInfo('authUrl response is missing a content-type header', 40170, 401); | ||
| err.hint = | ||
| 'Have your auth endpoint return a Content-Type of application/json (TokenDetails/TokenRequest), text/plain (token string) or application/jwt.'; |
Contributor
There was a problem hiding this comment.
Imo this sounds too imperative and more akin with docs than actual error, perhaps a better form would be (and ditto styling elsewhere):
Auth endpoints may return a Content-Type of application/json (TokenDetails/TokenRequest), text/plain (token string) or application/jwt.
| const err = new ErrorInfo( | ||
| 'authUrl responded with unacceptable content-type ' + | ||
| contentType + | ||
| ', should be either text/plain, application/jwt or application/json', |
Contributor
There was a problem hiding this comment.
should? or must?
"either" usually implies a binary choice, so the more natural message would be authUrl responded with unacceptable Content-Type. Expected one of: text/plain, application/jwt or application/json
| 401, | ||
| ); | ||
| err.hint = | ||
| 'Update your auth endpoint to return Content-Type application/json, text/plain or application/jwt — the SDK cannot parse other content types.'; |
Contributor
There was a problem hiding this comment.
Ditto my previous comment on imperative and em-dashes
| cb(new ErrorInfo('authUrl response exceeded max permitted length', 40170, 401), null); | ||
| const err = new ErrorInfo('authUrl response exceeded max permitted length', 40170, 401); | ||
| err.hint = | ||
| 'authUrl payloads must be under 128 KB. Your endpoint is returning more than a TokenDetails/TokenRequest object — return only the token shape, not wrapping JSON or extra fields.'; |
Contributor
There was a problem hiding this comment.
Is it true that they are returning something other than TokenDetails? Could the TokenDetails just be large (e.g. a big capabilities block)?
| if (typeof host !== 'string') { | ||
| throw new ErrorInfo('host must be a string; was a ' + typeof host, 40000, 400); | ||
| const err = new ErrorInfo('host must be a string; was a ' + typeof host, 40000, 400); | ||
| err.hint = 'Pass `endpoint` as a single hostname string (e.g. "main.realtime.ably.net"), not an array or object.'; |
Contributor
There was a problem hiding this comment.
The endpoint for main would just be main
| const match = name.match(regex); | ||
| if (!match || !match.length || match.length < 5) { | ||
| throw new ErrorInfo('regex match failed', 400, 40010); | ||
| const err = new ErrorInfo('regex match failed', 400, 40010); |
Contributor
There was a problem hiding this comment.
Is this an opportunity to also improve the message, not just the hint?
| machine.handleEvent( | ||
| new GettingPushDeviceDetailsFailed(new ErrorInfo('User denied permission to send notifications', 400, 40000)), | ||
| ); | ||
| const err = new ErrorInfo('User denied permission to send notifications', 400, 40000); |
Contributor
There was a problem hiding this comment.
Actual error to fix here from coderabbit
| * to evolve. | ||
| * - Use `matches` (regex) only when the token genuinely varies. | ||
| */ | ||
| const RUBRIC: Record<number, RubricEntry> = { |
Contributor
There was a problem hiding this comment.
Is this likely to cause us development friction, feels brittle?
| return src.replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, ' ')); | ||
| } | ||
|
|
||
| function extractHints(filePath: string): HintEntry[] { |
Contributor
There was a problem hiding this comment.
Subsequent to my comment about expanding ErrorInfo.fromValues to take a hint directly and not having an extra err.hint = ... line each time, would this script being an AST rather than a regex be more sturdy here?
umair-ably
added a commit
that referenced
this pull request
May 28, 2026
getW3CDeviceDetails.ts: swap ErrorInfo argument order at two throw sites where (statusCode, code) had been transposed. The ErrorInfo constructor expects (message, code, statusCode); the two calls were passing (message, 400, 40000) where 400 is HTTP status and 40000 is the SDK error code. Pre-existing bug pre-DX-1209, flagged by CodeRabbit. pushactivation.ts: add missing `return;` after the GettingDeviceRegistrationFailed handleEvent call so an undefined deviceRegistration cannot fall through into GotDeviceRegistration on the next line. Without the return, the activation state machine would emit a registration event with `deviceRegistration as any === undefined`. Flagged by CodeRabbit as a critical correctness issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sharpens ~15 of the inline error hints DX-1209 introduced based on ~120 LLM-pilot runs (see PR description for the full evaluation). Language: remove soft hedges (`probably`, `likely`, `potentially`, `may trigger`) in favour of active, definitive guidance. Second-wall forecasts: where a client-side fix opens up a server-side rejection, the hint now names the downstream cause inline. Applied to annotation_subscribe (realtimeannotations.ts), object_subscribe (liveobjects/realtimeobject.ts), wildcard clientId (auth.ts, baseclient.ts), presence.enter/update/leave (realtimepresence.ts), invalid channel modes (realtimechannel.ts), publish wrong-shape (realtimechannel.ts, restchannel.ts), and message annotations dashboard config (restchannelmixin.ts, realtimechannel.ts). The pattern brings LiveObjects 92xxx into scope — pilots showed weaker-model abandon rates of 40% on this surface without the forecast, 0% with it. Anti-hack notes: annotation_subscribe and object_subscribe hints now say "appending to channel.modes after attach() does not enable the mode server-side" so agents stop mutating the local modes array. Ably CLI pointers: where a CLI command would close the diagnosis loop, the hint adds a conditional `If you have the Ably CLI installed, ...` sentence (e.g. `ably auth keys list` for capability errors, `ably apps rules list` for channel-namespace settings). The conditional phrasing avoids regressing agents on machines without the CLI — an imperative phrasing caused 3/5 Opus runs to time out trying to invoke a missing binary in a confirmation pilot. Push platform-not-supported hints (push.ts ×2, pushactivation.ts ×4) now lead with the structural impossibility — "push.activate() cannot succeed in Node.js/server contexts (there is no device to register)" — and name the specific admin APIs to use instead. This turned 22-30-tool-call monkey-patch attempts into clean 7-tool-call acknowledgements that the SDK is right and the call is misused. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Static drift-guard for the ~90 inline err.hint sites DX-1209 added. For every err.hint = assignment under src/, asserts the hint string is non-empty and — if a per-code rubric exists — contains required tokens (e.g. 93001 must contain "annotation_subscribe" and "modes"; 40024 must contain the dynamic "expectedMode" identifier and "modes"; 40162 must contain "revoke" and "key"; etc). The rubric is deliberately scoped: codes shared across semantically unrelated throw sites (40000/40012/40013/40400/40500) are left out since a code-level rule would over-constrain. New entries are a 4-line block per error code. Wired into the existing Lint workflow (.github/workflows/check.yml) alongside npm run lint / format:check so drift fails PRs cheaply. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prettier --write across the 9 hint-affected files (8 SDK source + the new scripts/hint-coverage.ts) and bump the minimal-Realtime bundle threshold to keep the size guard green. - Lint: prettier formatting on the trailing CLI-line additions and the multi-line hint strings. No semantic change. - Bundle: minimal-Realtime raw 107 → 116 KiB and gzip 33 → 36 KiB in scripts/moduleReport.ts. The hint extensions across the audit (capability forecasts, CLI mentions, anti-hack notes, Push admin pivot) added ~9 KiB raw / ~2 KiB gzipped to the minimal bundle. Same threshold-bump pattern fff1c1b used for the initial DX-1209 hint additions (106→107 / 32→33). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
getW3CDeviceDetails.ts: swap ErrorInfo argument order at two throw sites where (statusCode, code) had been transposed. The ErrorInfo constructor expects (message, code, statusCode); the two calls were passing (message, 400, 40000) where 400 is HTTP status and 40000 is the SDK error code. Pre-existing bug pre-DX-1209, flagged by CodeRabbit. pushactivation.ts: add missing `return;` after the GettingDeviceRegistrationFailed handleEvent call so an undefined deviceRegistration cannot fall through into GotDeviceRegistration on the next line. Without the return, the activation state machine would emit a registration event with `deviceRegistration as any === undefined`. Flagged by CodeRabbit as a critical correctness issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hint-bearing throw sites in the SDK currently follow a 3-step pattern:
const err = new ErrorInfo('msg', 40000, 400);
err.hint = 'how to fix...';
throw err;
Add an options-object overload to ErrorInfo and PartialErrorInfo so
the same throw collapses to one call:
throw new ErrorInfo({ message, code, statusCode, hint });
The overload mirrors `fromValues` semantics: validate the shape, set
the fields, Object.assign for hint/cause/href/extras, auto-populate
href from the error code if not already set. Server-decoded errors
still flow through ErrorInfo.fromValues unchanged and continue to
carry their server-provided href and extra fields (requestId,
serverId) via the same Object.assign mechanism.
Also extend IConvertibleToErrorInfo and IConvertibleToPartialErrorInfo
to declare the hint, cause, and href fields that Object.assign has
been carrying through at runtime since the interface was introduced.
This is a type-only widening — no runtime behaviour change for any
existing caller.
Addresses AndyTWF's review-feedback question on whether fromValues
should accept hint directly so the SDK throw pattern is a single call.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address AndyTWF's review-feedback question on whether the SDK should
expose hint as a constructor parameter so the throw pattern is one call
instead of three. Now that ErrorInfo (and PartialErrorInfo) accept a
values-object overload, all ~85 inline hint sites move from
const err = new ErrorInfo('msg', code, statusCode);
err.hint = 'how to fix...';
throw err;
to
throw new ErrorInfo({ message, code, statusCode, hint: '...' });
(or `const err = new ErrorInfo({...}); reject(err);` where the variable
is used by a callback). One call site that already used
ErrorInfo.fromValues({...}) for its construction (realtimepresence.ts
get-when-suspended) folds the hint into the same object literal.
Also address CodeRabbit nits in the push plugin:
- Drop the unnecessary block braces wrapping single throws/rejects in
src/common/lib/client/push.ts (2 sites) and
src/plugins/push/pushactivation.ts (3 sites) and
src/common/lib/client/auth.ts (1 site) and
src/common/lib/types/basemessage.ts (1 site).
- Extract the four-times-repeated "push.activate() registers this
process..." hint into module-level PUSH_NOT_AVAILABLE_HINT constants
in pushactivation.ts and push.ts.
No wording changes in this commit; subsequent commits will refine hint
content per Andy's substantive feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address AndyTWF's substantive feedback on hint wording, factual
accuracy, and over-prescriptive server-rejection forecasts.
Wording (declarative tone, drop docs-style imperative):
- auth.ts authUrl Content-Type missing/unacceptable hints: switch
from "Have your auth endpoint return..." to "Auth endpoints may
return..."
- auth.ts unacceptable Content-Type message: "should be either" -> "Expected one of"
- "mint" word replaced with "construct" / removed in auth.ts hints
Factual corrections:
- auth.ts authUrl/token-too-large hints: acknowledge that a TokenDetails
can legitimately be large (big capability block); soften the accusation
that the endpoint is wrapping the token in extra JSON.
- auth.ts authCallback timeout hint: authUrl does not invoke a callback
and authCallback cannot return a promise; reword to differentiate the
two paths.
- auth.ts double-encoded token: trim the docs-style suffix off the
message, expand the hint with the JSON-vs-JWT diagnosis. (D1)
- connectionmanager.ts authorize 403 hint: per RSA4d the Ably server
itself can return 403 refusing to mint a TokenDetails from a
TokenRequest; broaden the hint beyond just authUrl/authCallback.
- baserealtime.ts channels.get options hint: per RTS3c, channels.get
with options is deprecated; rewrite to point at channel.setOptions()
only.
- realtimechannel.ts invalidStateError suspended branch: "suspended"
recovers automatically once the connection is re-established; only
"failed" needs an explicit attach() call.
- realtimechannel.ts/restchannel.ts publish: drop the
capability-forecast tail ("capability must include 'publish'");
there are many reasons a publish can be rejected, not just caps.
- realtimechannel.ts sync(): drop the hint entirely — sync() is an
internal SDK method, no user/LLM action is applicable.
- realtimepresence.ts enter/update/leave: drop the generic
presence-capability forecast; surface the wildcard-clientId
requirement when using enterClient/updateClient/leaveClient
on behalf of a different identity.
- realtimeannotations.ts and realtimeobject.ts (×2): "namespace must
have X enabled" reads as both an obligation and a cause; reword to
"check that the namespace has X enabled" so it's unambiguously a
hypothesis the user verifies.
- defaults.ts host-not-string hint: change example from
"main.realtime.ably.net" (a host) to "main" (an endpoint name).
- push.ts / pushactivation.ts PUSH_NOT_AVAILABLE_HINT: lead with the
supported platforms (browsers with service-worker support) before
naming the unsupported context (Node.js/server).
- utils.ts derived-channel regex: replace the placeholder "regex match
failed" message with a useful description, and swap the
transposed code/statusCode args (40010, 400 not 400, 40010) so
href auto-populates correctly under the new ErrorInfo overload.
No new throw sites added; no semantic behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace em-dashes (—) with hyphens (-) inside every err.hint string per AndyTWF's convention preference. Code-comment em-dashes elsewhere in the codebase are left alone. - Fix typo: "caes" → "case" in realtimeannotations.ts attach-state comment (CodeRabbit). No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the regex-based scanner with a ts.createSourceFile + AST walk.
Addresses AndyTWF's concern that the script is brittle, and CodeRabbit's
edge-case findings (block-comment stripping breaks inside string
literals; semicolon termination breaks inside string literals).
The new implementation walks `NewExpression` and `CallExpression` nodes
to recognise:
- new ErrorInfo({ ..., hint, code, ... })
- new PartialErrorInfo({ ... })
- <chain>.ErrorInfo({ ... }) (e.g. this.client.ErrorInfo)
- ErrorInfo.fromValues({ ... }) / PartialErrorInfo.fromValues({ ... })
For each match it extracts the `code:` numeric value and the `hint:`
string. Hint values can be plain string literals, template literals
(interpolations collapsed to their identifier text for substring
checks), string concatenations, or identifier references resolved
against same-file top-level const declarations (e.g.
PUSH_NOT_AVAILABLE_HINT).
The RUBRIC and per-code rules are unchanged from the regex version;
this commit only swaps the extractor.
After C3 (which migrated hint sites to the options-object form) the
old regex extractor was finding zero hints. The new walker finds 92
sites; all pass the rubric.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test/unit/error-hints.test.js:
- Add `expect(err.hint).to.be.a('string')` before each substring
assertion so a missing/undefined hint fails with a clear "expected
a string" rather than a TypeError on `.contain()` (CodeRabbit nit).
- Document why `rest._FilteredSubscriptions` is the trigger for the
missing-plugin throw: no public API exposes the throw without a
Realtime connection, so we rely on the stable internal getter
(CodeRabbit fragility flag).
scripts/moduleReport.ts:
- Annotate the minimalUsefulRealtimeBundleSizeThresholdsKiB
constant: previous {raw: 107, gzip: 33}; bumped in DX-1209 to
absorb ~80 inline err.hint strings (~9 KiB raw, ~3 KiB gzip), with
recalibration guidance for future hint changes (CodeRabbit nit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sweep every (message, hint) pair in the SDK; six sites had a hint that
just rephrased the message (or vice versa). For each, keep the
"what failed" in the message and lean on the hint for the "how to fix".
utils.ts:
- derived-channel regex hint dropped the rephrased "Channel names with
derived options must look like..." (now in the message); keeps only
the docs link.
- concurrent next() hint dropped its trailing "Calling next() twice
concurrently is not supported" sentence, which mirrored the message.
restchannelmixin.ts (×3 sites):
- message trimmed to "This message lacks a serial" /
"...and cannot be updated"; the "Make sure you have enabled X in
channel settings on your dashboard" guidance already lives in the
hint and is dropped from the message.
realtimechannel.ts:
- message-lacks-serial site: same trim as restchannelmixin.
- detach-from-failed hint dropped the "A failed channel cannot be
detached" sentence (a rephrasing of "Unable to detach; channel
state = failed").
- attach/detach-timeout hints dropped the "server did not acknowledge
within realtimeRequestTimeout" sentence (a rephrasing of "Channel
attach/detach timed out").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The decoding loop in basemessage.ts catches per-encoding errors and rewraps them with an outer ErrorInfo so the caller gets one error per message rather than one per encoding stage. The wrapper used the positional ErrorInfo constructor, which has no hint parameter, so the three hint-bearing throws in this loop - Vcdiff plugin missing (40019), no typed-array support (40020), Vcdiff decode failure (40018) - had their hints silently swallowed before reaching the caller. Pre-DX-1209 this was harmless because no inner hints existed; flagged by CodeRabbit review. Switch the wrapper to the options-object constructor and forward err.hint. Cause is intentionally not forwarded: three of the inner throws are plain Error (cipher and 'Unknown encoding' branches) which do not fit cause: ErrorInfo | PartialErrorInfo without an instanceof branch, and CodeRabbit flagged cause as optional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The _getDeviceIdentityToken() throw site in pushchannel.ts is reached from both subscribeDevice() and unsubscribeDevice() (via the shared _getPushAuthHeaders() helper), but its message and hint only mentioned subscribing. CodeRabbit flagged that this misdirects users hitting the error from an unsubscribe call. Rewrite both message and hint to be action-neutral. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pinning exact hint substrings (and the static AST coverage check that guarded token presence) treats hints like a tested API surface. We don't test exact error-message wording elsewhere, and the same reasoning applies to hints — they're guidance, not a contract. Remove the apparatus rather than maintain drift assertions for it. Removes: - scripts/hint-coverage.ts (+ npm run hintcoverage, CI step in check.yml) - test/unit/error-hints.test.js Guidance on writing/using hints will live in a best-practices doc instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per PR review, message and hint must not overlap: message states only "what" went wrong; hint states only "how" to fix it. A workflow assessed all 93 message/hint pairs and tightened the 19 remaining offenders: - Stripped restated-"what" preambles from hints (realtimepresence, connectionmanager, basemessage, auth, push, pushchannel, rest, paginatedresource, defaults, getW3CDeviceDetails). - Reduced messages that carried remediation already present in the hint (wildcard-clientId in auth/baseclient; setOptions guidance in baserealtime). No actionable guidance lost - removed text was duplicated in its counterpart. tsc --noEmit clean for all edited files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
umair-ably
force-pushed
the
DX-1209/error-hints
branch
from
22392bd to
b2537c4
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
Contributor
Author
|
@lmars I've made some more tweaks in the latest commit. Re: guidelines - these were created using a dynamic workflow following some basic rules e.g. error message is what went wrong, hint is guidance on what can or should be done next - followed by some generic "follow the code to verify what's happening". The recent commit has tried to codify some standards e.g. imperative voice. I'll commit this to the repo in a separate repo once this lands. Re: when to add a hint. I think we've touched on this before - it's easier to mandate a hint everywhere than trying to draw some arbitrary line and miss useful cases. Likewise, I've shown some evidence that even a hint we don't think is useful has proved some use to LLMs. I think the risk of adding a hint when it is not needed is so small that it shouldn't be blocking this PR - we can certainly revisit it if it is something that is happening more often than not, but I don't think that is the case. |
umair-ably
force-pushed
the
DX-1209/error-hints
branch
from
b2537c4 to
35f3492
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2233/bundle-report
lmars
reviewed
Jun 29, 2026
lmars
left a comment
lmars
left a comment
Member
There was a problem hiding this comment.
I'm down to src/common/lib/transport/connectionmanager.ts in the diff but leaving my review so far (I'll come back to it).
| message: msg, | ||
| code: 40160, | ||
| statusCode: 401, | ||
| hint: 'Pass one of ClientOptions.{ key, authUrl, authCallback, token, tokenDetails }. For production, prefer authUrl or authCallback so the API key stays on your server.', |
Member
There was a problem hiding this comment.
I think "For production, prefer authUrl or authCallback so the API key stays on your server." is confusing, what if this code is running on a server? How about the following which matches Recommended authentication from docs:
Suggested change
| hint: 'Pass one of ClientOptions.{ key, authUrl, authCallback, token, tokenDetails }. For production, prefer authUrl or authCallback so the API key stays on your server.', | |
| hint: 'Set one of the following in ClientOptions: key, authUrl, authCallback, token, or tokenDetails. For production use, prefer authUrl or authCallback for client-side (browser, mobile apps), or key for server-side.', |
Comment on lines
+277
to
+280
| message: 'Unable to update auth options with incompatible key', | ||
| code: 40102, | ||
| statusCode: 401, | ||
| hint: 'auth.authorize() cannot change the API key - the new authOptions.key differs from the one the client was constructed with. To use a different key, construct a new Ably client.', |
Member
There was a problem hiding this comment.
The general guidance is that "err.message says what failed; err.hint says how to fix it", and here "the new authOptions.key differs from the one the client was constructed with" is more "what failed" than it is "how to fix it".
Saying this here because I'm wondering if the prompts we're using to generate these hints needs to be a bit more explicit that hint should be to-the-point about specifically what needs to be done, rather than expanding on what the error means (which is what err.message is for).
If I were writing this based on the guidance:
Suggested change
| message: 'Unable to update auth options with incompatible key', | |
| code: 40102, | |
| statusCode: 401, | |
| hint: 'auth.authorize() cannot change the API key - the new authOptions.key differs from the one the client was constructed with. To use a different key, construct a new Ably client.', | |
| message: 'authorize called with a key that does not match the existing key being used by the client', | |
| code: 40102, | |
| statusCode: 401, | |
| hint: 'To use a different key, construct a new Ably client with the key as a client option.', |
This is more about updating the guidelines than it is about this specific case. I won't keep suggesting this for other cases, I'll just focus on anything that is inaccurate, and leave you to decide if any futher message/hint splitting is useful.
| message: 'authUrl JSON response exceeded the maximum permitted length of 128 KB', | ||
| code: 40170, | ||
| statusCode: 401, | ||
| hint: 'Make your authUrl endpoint return only the token payload (a TokenDetails or TokenRequest object, or a token string), not an envelope wrapping it in extra fields. If a TokenDetails genuinely needs a large capability, narrow the capability to the channels and operations the client actually needs.', |
Member
There was a problem hiding this comment.
Make your authUrl endpoint return only the token payload
How do we know this is the reason? Can an encoded TokenDetails object not itself exceed this limit?
If a TokenDetails genuinely needs a large capability
Why does this jump into capabilities?
I think here, behind the scenes we've thought of two reasons the token might be too large: either it's wrapped which makes it bigger, or its got a very large capability, and then we're hinting at a fix for both without stating those might be the reasons?
This is what I'd probably write:
If the server behind authUrl is returning Ably tokens wrapped with extra fields, consider removing the fields that are not required. If the client is being granted a long list of capabilities that make the token itself very large, consider using wildcards rather than long lists of channels.
| message: 'Token string is empty', | ||
| code: 40170, | ||
| statusCode: 401, | ||
| hint: 'Return a non-empty value from your authCallback/authUrl: a token string, a TokenRequest, or a TokenDetails object. The callback returned an empty string.', |
Member
There was a problem hiding this comment.
Suggested change
| hint: 'Return a non-empty value from your authCallback/authUrl: a token string, a TokenRequest, or a TokenDetails object. The callback returned an empty string.', | |
| hint: 'Return a non-empty value from your authCallback/authUrl: a token string, a TokenRequest, or a TokenDetails object.', |
| message: msg, | ||
| code: 40170, | ||
| statusCode: 401, | ||
| hint: 'authCallback must invoke its callback with (err, tokenStringOrTokenDetailsOrTokenRequest). authUrl must respond with a token string or TokenDetails/TokenRequest JSON.', |
Member
There was a problem hiding this comment.
Suggested change
| hint: 'authCallback must invoke its callback with (err, tokenStringOrTokenDetailsOrTokenRequest). authUrl must respond with a token string or TokenDetails/TokenRequest JSON.', | |
| hint: 'If authenticating with an authCallback, update it to callback with (err, tokenStringOrTokenDetailsOrTokenRequest). If authenticating with an authUrl, update the server to respond with a token string or TokenDetails/TokenRequest JSON.', |
| message: 'No link to the current page of results', | ||
| code: 40400, | ||
| statusCode: 404, | ||
| hint: 'Call current() only on a PaginatedResult returned by a paginated REST query (such as channel history, presence, or stats) whose response carried pagination Link headers. This page has no rel="current" navigation link, so there is nothing to reload; if you only need to walk pages, use next() together with isLast() instead, since those are the public navigation methods this result type exposes.', |
Member
There was a problem hiding this comment.
Call current() only on a PaginatedResult returned by a paginated REST query (such as channel history, presence, or stats) whose response carried pagination Link headers
How exactly should the developer do that though?
| "You are trying to add an annotation listener, but you haven't requested the annotation_subscribe channel mode in ChannelOptions, so this won't do anything (we only deliver annotations to clients who have explicitly requested them)", | ||
| code: 93001, | ||
| statusCode: 400, | ||
| hint: 'Re-create the channel with annotation_subscribe in modes, e.g. realtime.channels.get(name, { modes: ["subscribe", "annotation_subscribe"] }), since appending to channel.modes after attach() does not enable the mode server-side. If the subsequent attach is rejected by the server, confirm the channel namespace has "Message annotations, updates, appends, and deletes" enabled in the Ably dashboard and that your API key has annotation-subscribe capability on this channel. If you have the Ably CLI installed, `ably apps rules list` shows which namespaces have it enabled and `ably auth keys list` shows the capabilities of your key.', |
Member
There was a problem hiding this comment.
Should this be suggesting setOptions to update the modes, because won't channels.get(name) with different options throw an error?
| message: 'Unable to detach; channel state = failed', | ||
| code: 90001, | ||
| statusCode: 400, | ||
| hint: 'Release it via channels.release(name) and call channels.get(name) again to start a fresh channel.', |
Member
There was a problem hiding this comment.
If the code is trying to detach, why call channels.get(name) to start a fresh channel, which will attach again?
| message: 'Channel attach timed out', | ||
| code: 90007, | ||
| statusCode: 408, | ||
| hint: 'The SDK will retry automatically once the connection is healthy; check connection.state and connection.errorReason.', |
Member
There was a problem hiding this comment.
How do we know the timeout is because the connection is unhealthy?
| throw new PartialErrorInfo({ | ||
| message: 'Unable to leave presence channel while in ' + channel.state + ' state', | ||
| code: 90001, | ||
| hint: 'Inspect channel.errorReason for the cause, then await channel.attach() and retry presence.leave() if the channel state is "failed". If the state is "initialized" no member was ever entered, so there is nothing to leave and no action is needed.', |
Member
There was a problem hiding this comment.
If the state is "failed", isn't channel.attach() going to throw?
Contributor
Author
|
Thanks @lmars - I've applied most of these changes, just checking through them myself once more before pushing up. A few I'd like to sanity-check before I finalise the wording:
|
Address lmars' 2026-06-29 review comments plus a PR-wide accuracy sweep,
each verified against the SDK code paths, state machines, and ably.com docs:
- auth.ts: clarify 40160 client-side vs server-side auth guidance (docs-aligned);
split the 40102 message vs hint; reframe the 40170 "too large" family
(envelope + capability/wildcards) consistently; tighten the callback
return-shape and createTokenRequest hints; clarify clientId "*" (a fixed
identity cannot be "*"; a wildcard identity comes from a wildcard token)
- baseclient.ts: drop the token/tokenDetails suggestion on an invalid key;
remove the irrelevant wildcard tangent on a non-string clientId; clarify
clientId "*"
- paginatedresource.ts: de-jargon the first()/current() hints
- realtimeannotations.ts, realtimeobject.ts: recommend channel.setOptions to
change modes (channels.get(name, { modes }) throws on an attached channel)
- realtimechannel.ts: align the detach/attach-timeout recovery hints with the
channel state machine (attach() recovers from failed; suspended auto-retries)
- realtimepresence.ts: correct leave() guidance per state (presence members
are cleared on failed/detached, so there is nothing to leave)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ll-PR docs review Verified findings from the 2026-07-02 docstrings-pr-review run (each traced against code paths, state machines, CLI output, and a live sandbox trace): - utils.ts: derived-channel 40010 message was false (branch fires only on empty/unparseable names, and the hint's own example triggered a different error); createMissingPluginError now names the real entry point per plugin (Push -> ably/push, LiveObjects -> ably/liveobjects, others -> ably/modular); v1-callback message/hint reworded (tests re-pinned) - defaults.ts: drop invented v1/v2 version framing (endpoint arrived in 2.10.0 per CHANGELOG; environment/restHost/realtimeHost are deprecated v2 options); fallbackHosts hints are now single prescriptions - realtimeobject.ts (LiveObjects): 40024 hints no longer claim the attach is rejected on a capability shortfall (live-traced: the server resolves the attach and silently drops the mode) and no longer cite a nonexistent "LiveObjects namespace" dashboard setting; recipe aligned with the sibling missing-mode hints - realtimechannel.ts: invalid-mode hint no longer claims the attach is rejected (silently-dropped story, matching channel.modes); 40003 sendUpdate hint unified to the canonical missing-serial construction; untilAttach message now carries the observed state like the presence twin - realtimepresence.ts: enter/update 40012 hints share one skeleton; leave 40012 no longer prescribes a no-op set-clientId-and-retry errand - connectionmanager.ts: 403 auth hint no longer narrates a server-refusal branch that never fires on that path; ping hint covers the closed/failed/initialized states that never auto-connect - basemessage.ts: vcdiff 40018 recovery hint matches the traced behaviour (re-attach from last processed message; delta stays enabled) - push cluster: platform-unavailable messages now diagnose (browser/service worker requirement), hints are pure prescriptions; subscribeClient/ unsubscribeClient hints state the real clientId sources per client type; getW3CDeviceDetails branches denied vs dismissed permission - rest.ts: revokeTokens hint names the real mechanism (revocable tokens enabled on the issuing key), not a nonexistent "revoke capability" - restchannel/restchannelmixin/restannotations/paginatedresource/baserealtime: canonical missing-serial constructions, single-prescription hints, message mechanics; 40009 max-size message parenthetical unified across the three publish sites - auth.ts/baseclient.ts: clientId type-check hints byte-identical; residual diagnosis clauses moved from hints into messages Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
This PR extends the message-vs-hint split DX-1204 introduced (
err.messagesays what failed;err.hintsays how to fix it) to ~80 client-sideErrorInfothrow sites in the SDK. Where applicable, hints also forecast the next likely failure — typically a server-side rejection driven by channel-namespace / capability config the agent can't see from inside the SDK alone.Why
LLMs (and humans) recover faster from SDK errors when the actionable next step is named in the error itself, rather than left to be inferred or searched. We saw two recurring failure modes when agents hit SDK errors:
DX-1209's hints are designed to convert both behaviours into "investigate, then commit to the idiomatic fix" — naming the second wall up front (so the agent doesn't spiral into it), and pointing at concrete diagnostics or replacement APIs (so the agent doesn't quit).
Outcome and Evidence
Same emulated-constraint LLM testing as DX-1204: subagents fix a failing Node test, no
WebFetch/WebSearch, SDK as a black box visible only through runtime errors. Four experiments below summarise ~120 piloted runs across Opus 4.7, Sonnet 4.6, Haiku 4.5 × pre/post × n=5.How we scored fixes
The interesting question wasn't just "did the test go green?" but "how did the agent make it green?" Every passing run was bucketed by reading the final
test.js:channels.get(name, { modes: ['subscribe', 'annotation_subscribe'] })plus a channel name in a namespace where Mutable Messages is enabled. The code looks like documentation.modes) but wrapped the result in atry/catchthey don't appear to understand, or paired the fix with an unrelated workaround. Test passes; intent is partial..catch(() => {})on a real failure,channel.modes.push('annotation_subscribe')after attach to satisfy a snapshot assertion, swappingpush.activate()for a private monkey-patch ofPlatform.Config.push. Test passes; the code that lands in the repo doesn't reflect how the SDK is meant to be used.Experiment 1: annotation_subscribe hint with second-wall forecast
channel.annotations.subscribe()withoutannotation_subscribeinChannelOptions.modesthrows 93001. Pre-PR the message named the symptom only; post-PR the hint names the modes API call shape and forecasts that the channel namespace must have Mutable Messages enabled in the Ably dashboard.The idiomatic fix is two steps: add
annotation_subscribetoChannelOptions.modes, and use a channel name in a namespace that has Mutable Messages enabled. Pre-PR agents tended to get the first step and miss the second, then either swallow the server-side rejection (HACKY) or fence-sit between the right modes call and a defensive try/catch (HALF). Post-PR, the hint primes them to expect the second wall, so they go looking for the right namespace immediately.channel.modesafter attach containsannotation_subscribe), IDIOMATIC rate among passing runs: pre 67% → post 83%. The HALF bucket collapses (3 → 0): no more "I added the modes and threw a catch around it just in case." Sonnet specifically shifts from HACKY (no-await + empty.catch) to IDIOMATIC (probes namespaces, findstest:, commits to the fix).The annotation hint also includes a short anti-hack line — "Note: appending to channel.modes after attach() does not enable the mode server-side." That sentence exists because one Haiku run literally did
channel.modes.push('annotation_subscribe')to satisfy a snapshot assertion. The hint now names that gaming pattern explicitly so future agents (and humans) don't repeat it. Same line was added to the LiveObjects hint for the same reason. This provides some evidence going forwards that in a similar vain to prompting LLMs, our hints can and should also include advice on what not to do.Experiment 2: pointing at the Ably CLI from inside the hint
The dashboard line in the annotations hint surfaces a real wall ("this is a config issue, not a code issue") but doesn't tell an agent how to see the config. From inside a Node process the agent has no view of which channel namespaces actually have Mutable Messages enabled; without that visibility, weaker models quit, capable models burn tool calls probing namespace prefixes by trial and error.
Adding a final sentence — "If you have the Ably CLI installed,
ably apps rules listwill show which channel namespaces have Mutable Messages enabled" — bridges that gap. The CLI is the natural diagnostic surface for this class of question (it speaks to the same backend the SDK does, with the same credentials), and agents are willing to invoke it when the hint tells them it exists. The conditional phrasing matters:ably apps rules list, found a working namespace, and moved on. No more namespace-probing spirals.ably apps rules list"): 3/5 Opus runs timed out trying to invoke the missing CLI. The imperative phrasing turned "the CLI helps if you have it" into "the CLI is required" — and capable agents committed hardest to that wrong path.The conditional form is therefore shipped: it's strictly better when the CLI is absent, equivalent when present, and tells production users that the CLI is the right diagnostic for any error rooted in dashboard or capability config.
NOTE: Just this experiment was a strong signal that the CLI paired with these hints was a powerful combination in reducing the number of tool calls whilst reaching an idiomatic solution quicker. I will investigate how we can surface installing the CLI at a sooner, and more appropriate stage e.g. perhaps we log CLI install instructions when the SDK is first initialised.
Experiment 3: LiveObjects portability
I originally didn't apply a hint to
object_subscribemode missing (40024) because the error message alone seemed descriptive enough, however, there's evidence that shows we can still help weaker models here.Idiomatic fix mirrors annotations:
channels.get(name, { modes: [..., 'object_subscribe'] }). The test channel name happens to work in the default namespace, so there's no second-wall to navigate here; the hint's job is just to surface the modes API.Experiment 4: telling agents to stop fighting the SDK when the API is structurally wrong for the context
Some errors signal "you're using the wrong API entirely, not a fixable mistake" — e.g.
push.activate()on a Node server, where there is no device to register. The idiomatic fix is to recognise thatpush.activate()is the wrong API in this context and pivot toclient.push.admin.publish(...)/deviceRegistrations.save(...), which are designed for server-side push management. The HACKY alternative the eval kept seeing was monkey-patchingPlatform.Config.pushwith a hand-rolled storage object so the SDK's platform check would pass — an entirely different code path that simulates being a browser inside Node.The original hint named the alternative ("Use Push admin") but left the door open to retry; Opus runs spent 22–30 tool calls building those monkey-patches. The hint was true but too soft, so agents read it as advice instead of an instruction to stop.
Rephrasing to lead with the impossibility — "push.activate() registers this process as a push target — it cannot succeed in Node.js/server contexts. Use client.push.admin.publish(...) / deviceRegistrations.save(...) instead." — dropped Opus tool counts to mean 7.3 (range 6–9) across 3 confirmation runs, with zero monkey-patches. The transcripts shift from "puzzle to solve" to "cite the hint, accept the SDK is right, wrap the call narrowly and move on." Same pattern applied to all 6 push platform-not-supported throw sites (2 in
push.ts, 4 inpushactivation.ts).Where the hints do not help
Two scenarios where the eval showed no measurable delta — included for honest scoping:
40160 / "Channel access denied"(constrained-key presence test): the server's own error already names the failure clearly enough that capable agents decode it without needing the client-side hint. Adding the capability forecast didn't move tool counts.These are useful as the negative space around DX-1209's value: hints help most where the message is symptomatic-only, the fix is non-obvious, or the next failure is hidden behind a wall the agent can't see from inside the process. Where the message already encodes the fix, the hint is a no-op.
The cross-cutting story across all four experiments: DX-1209's hints shorten the loop between hitting an SDK error and committing to the idiomatic fix. Where agents previously spiralled (mutating code against a wall that needs a dashboard change) or quit (no path forward visible from inside the process), the hints now name the next wall up front, point at the diagnostic tool that can see it, and — when the call is structurally wrong for the context — tell the agent to pivot rather than keep fighting. Tool-count averages move modestly; abandon rates at the weakest tier collapse; and the code that lands in your repo is materially more likely to follow the SDK's intended design.
Summary
message(what failed) /hint(how to fix) split that DX-1204 introduced fordetectV1Callbackand applies it to everyErrorInfothrow in the SDK where a fix-it hint adds value. The split lets agents (and humans) read the what inerr.messageand act on the how inerr.hintwithout parsing prose.Themes / where this sits
This is the foundation slice of
LLMEvalUpdatedPlan.mdA1 (TKT-6) and aligns with the team's "Theme 3 — errors guide self-healing" target for the quarter. It's defence-in-depth alongside DX-1204 (v1-callback runtime throws with a hint) and DX-1205 (legacy-import shims with a hint).What's in scope vs deliberately not
Hint added (~80 sites):
realtimechannel.ts— channel options validation, publish shape (40013), max size (40009), detach-while-failed,invalidStateError(90001), release-state, attach/detach timeout (90007), untilAttach,sendUpdatemissing serial (40003),sync()not-attached.realtimepresence.ts— clientId missing for enter/update/leave (40012), leave-in-incompatible-state (90001),get()on suspended (91005), untilAttach, auto-re-enter failure (91004).auth.ts— no auth options (40160), incompatible key (40102 ×2), authUrl/authCallback errors (40170 ×9), no-renewable-token (40171), no/invalid key (40101 ×2), empty/wildcard/non-string clientId (40012 ×3), token clientId mismatch (RSA15c), token-shape rejection.baseclient.ts,defaults.ts,rest.ts,baserealtime.ts,paginatedresource.ts,utils.ts,connectionmanager.ts,basemessage.ts— see commit message for the per-file rundown.push.ts,pushactivation.ts,pushchannel.ts,getW3CDeviceDetails.ts— push platform/state, registration callback, device-identity preconditions.restchannel.ts,restchannelmixin.ts,restannotations.ts,realtimeannotations.ts— message-serial requirements, annotation shape / mode.Skipped (per A1 scope):
80xxxconnection-lifecycle codes — auto-recovered by the SDK; a hint would encourage user retry code that fights the SDK.50000internal errors — only actionable advice is file an issue, not worth a per-site hint.40140family,40160,40300,42910, etc.) — the SDK does not construct these, the server does. Propagating server-supplied hints is separate ongoing work.What's next
As mentioned in the above section; server-supplied hints is ongoing work. Once this is ready, we can revisit these current hints to see if we still need to forecast the next error, or if the combination of client error + server error works just as well.
I'd also like to surface the CLI install steps in the SDK initialisation journey.
Base branch
DX-1205/legacy-import-shims, notmain. We're stacking — merge DX-1205 (#2227) first, then this branch auto-retargets tomainor can be rebased.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores