Check error codes against the ably-common registry at compile time - #2281
Check error codes against the ably-common registry at compile time#2281lmars wants to merge 1 commit into
Conversation
`new ErrorInfo(message, code, statusCode)` takes two adjacent numbers the
compiler accepted in either order, and codes were written as bare literals
with nothing tying them to the registry in ably-common. Generating the set
of registered codes from that registry and narrowing `code` to it closes
both gaps: an unregistered code and a transposed `code`/`statusCode` pair
now fail to compile, because registry codes are 5-6 digits and an HTTP
status is 3, so a status can never be a member of the union.
src/common/lib/types/errorcodes.ts is generated by the ably-common script
`errors/scripts/generate-ts.js` via `npm run generate:errorcodes-ts`, and
committed. The type erases at compile time, so this costs nothing in the
bundle: `errorcodes` appears zero times in build/ably-node.js and
build/ably.min.js.
Errors this SDK raises are checked; errors decoded from the server are not,
because the server chooses those codes and may use ones a given client
version does not know. Rather than leave one signature to serve both, the
two are separated by name:
fromValues - checked, for errors this repository raises
fromWireValues - unchecked, for a response body, a response header, or
the `error` field of a ProtocolMessage
`fromValues` was previously reached by both kinds of caller, so it could
not be both checked and honest. Splitting it makes the safe path the
default and leaves every unchecked site greppable; there are ten, all
genuine server decodes apart from the application's authCallback in auth.ts,
whose code originates in user code and so cannot be validated here.
`ErrorInfoValues` and `PartialErrorInfoValues` are the checked shapes for
the options-object constructor. `IConvertibleToErrorInfo` keeps
`code: number` and is now reached only by the wire path.
This exposed three defects:
- Five sites had `code` and `statusCode` the wrong way round:
src/common/types/http.ts (x2), nodejs and web crypto.ts, and web
http.ts. Their `err.code` and `err.statusCode` were transposed and are
now correct. The web crypto site had no valid pairing in either order
(400/50000); it now matches its corrected Node sibling.
- connectionerrors.ts `unknownChannelErr` used UNKNOWN_CONNECTION_ERR
(50002), which is why UNKNOWN_CHANNEL_ERR (50001) was unused. The
registry gives 50001 as `Internal channel error` and protocol.ts
already uses it that way. Channel errors now report 50001.
- normaliseAuthcallbackError in auth.ts took its code off an `any`, so
neither the callback's value nor the SDK's own fallbacks were checked.
The fallbacks are now annotated constants; the callback's value is an
explicit cast.
Note the first two change observable error codes.
ConnectionErrorCodes uses `as const satisfies Record<string, ErrorCode>`
so that keys stay literal and values are checked. A plain
`Record<string, ErrorCode>` annotation would have kept accepting a
misspelled key, which is the shape of the bug fixed above.
Two CI steps in check.yml. The first typechecks src/, which nothing did
before: the build is esbuild and the UTS suite runs under tsx, both of
which strip types without checking them, so `tsc --noEmit ably.d.ts
modular.d.ts` was only covering the two declaration files. Without it none
of this is enforced anywhere but an editor. It passes with no other change.
The second regenerates at the pinned submodule commit and fails on a diff,
so a reference to a code that is not registered in ably-common cannot merge
here first.
The submodule pin is provisional: it points at the unmerged ably-common
branch `error-code-typescript-gen`, needed for the generate and drift-check
steps to run. Repoint it to main once that is merged. Only protocol/
differs between the old pin and this one; test-resources/, which is all
ably-js reads from the submodule, is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe change adds a generated ChangesError code typing and decoding
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/auth.ts`:
- Around line 40-46: Update the raw error branch in authCallback handling to use
the 403-specific fallback error code when err.statusCode is 403 and no err.code
is present, while retaining AUTH_CALLBACK_ERROR_CODE for other statuses and
existing codes.
In `@src/platform/web/lib/util/crypto.ts`:
- Line 146: Update the error handling in Crypto.generateRandomKey to append an
actionable remediation note when crypto.getRandomValues fails, specifically
directing callers to use random key material of 65,536 bytes or less and
securely expand it for larger keys. Preserve the existing error details,
ErrorInfo status values, and public generateRandomKey behavior.
🪄 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 Plus
Run ID: 129eb633-1ce6-45e4-bb09-215bbffe916a
📒 Files selected for processing (19)
.github/workflows/check.ymlpackage.jsonsrc/common/lib/client/auth.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/transport/connectionerrors.tssrc/common/lib/types/devicedetails.tssrc/common/lib/types/errorcodes.tssrc/common/lib/types/errorinfo.tssrc/common/lib/types/protocolmessage.tssrc/common/types/http.tssrc/platform/nodejs/lib/transport/nodecomettransport.jssrc/platform/nodejs/lib/util/crypto.tssrc/platform/nodejs/lib/util/http.tssrc/platform/web/lib/http/http.tssrc/platform/web/lib/http/request/fetchrequest.tssrc/platform/web/lib/http/request/xhrrequest.tssrc/platform/web/lib/util/crypto.tstest/common/ably-commontest/uts/rest/unit/types/error_types.test.ts
| if (!Utils.isErrorInfoOrPartialErrorInfo(err)) { | ||
| return new ErrorInfo(Utils.inspectError(err), err.code || 40170, err.statusCode || 401); | ||
| return new ErrorInfo( | ||
| Utils.inspectError(err), | ||
| (err.code as ErrorCode) || AUTH_CALLBACK_ERROR_CODE, | ||
| err.statusCode || 401, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'auth\.ts$' . | sed 's#^\./##'
echo "== target outline =="
ast-grep outline src/common/lib/client/auth.ts --view expanded || true
echo "== target lines =="
cat -n src/common/lib/client/auth.ts | sed -n '1,120p'
echo "== search auth callback codes and error info helpers =="
rg -n "AUTH_CALLBACK|isErrorInfoOrPartialErrorInfo|40300|40170|statusCode === 403|authCallback" src test -S || true
echo "== deterministic behavior probe from source text =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path('src/common/lib/client/auth.ts')
text=p.read_text()
for name in ['AUTH_CALLBACK_ERROR_CODE','AUTH_CALLBACK_FORBIDDEN_CODE']:
m=re.search(r'\b'+re.escape(name)+r'\s*=\s*(\d+)',text)
print(name, '=' , m.group(1) if m else 'NOT_FOUND')
# approximate source branches
print("non_errorinfo_has_status_code_403_check=", bool(re.search(r'!Utils\.isErrorInfoOrPartialErrorInfo\(err\)\s*\{[^}]*statusCode\s*===\s*403', text, re.S)))
print("errorinfo_branch_has_status_code_403_check=", bool(re.search(r'Utils\.isErrorInfoOrPartialErrorInfo\(err\)[^}]*statusCode\s*===\s*403', text, re.S)))
PYRepository: ably/ably-js
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== normalize helper and call sites =="
sed -n '38,56p' src/common/lib/client/auth.ts | cat -n
rg -n "normaliseAuthcallbackError" src test -S --max-count 50
echo "== targeted test cases for auth callback 403 and code behavior =="
sed -n '239,290p' test/uts/realtime/unit/auth/auth_callback_errors.test.ts | cat -n
sed -n '700,760p' test/realtime/auth.test.js | cat -n
echo "== error code constants =="
sed -n '76,90p' src/common/lib/types/errorcodes.ts | cat -n
sed -n '100,170p' src/common/lib/types/errorcodes.ts | cat -n
echo "== source-derived deterministic behavior models =="
python3 - <<'PY'
from pathlib import Path
for raw, shape in [
("raw_403_no_code", {"type_not_errorinfo": True, "statusCode": 403}),
("raw_403_with_40170", {"type_not_errorinfo": True, "statusCode": 403, "code": 40170}),
("shape_403_no_code", {"type_not_errorinfo": False, "statusCode": 403}),
("shape_403_existing_code", {"type_not_errorinfo": False, "statusCode": 403, "code": 80022}),
("shape_401_no_code", {"type_not_errorinfo": False, "statusCode": 401}),
]:
err = dict(shape)
if not err.get("type_not_errorinfo"):
err["code"] = 40300 if err["statusCode"] == 403 else 40170
if err["statusCode"] != 403:
err["statusCode"] = 401
else:
err["code"] = err.get("code") or 40170
print(raw, "-> code", err["code"], "status", err.get("statusCode"))
PYRepository: ably/ably-js
Length of output: 8120
Apply the same 403 fallback in the raw authCallback error branch.
A raw authCallback error with statusCode === 403 and no code currently becomes 40170, while an ErrorInfo with the same status gets 40300 as required by the RSA4d tests. Use the 403 fallback here too.
🔧 Possible fix
if (!Utils.isErrorInfoOrPartialErrorInfo(err)) {
return new ErrorInfo(
Utils.inspectError(err),
- (err.code as ErrorCode) || AUTH_CALLBACK_ERROR_CODE,
+ (err.code as ErrorCode) || (err.statusCode === 403 ? AUTH_CALLBACK_FORBIDDEN_CODE : AUTH_CALLBACK_ERROR_CODE),
err.statusCode || 401,
);
}📝 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.
| if (!Utils.isErrorInfoOrPartialErrorInfo(err)) { | |
| return new ErrorInfo(Utils.inspectError(err), err.code || 40170, err.statusCode || 401); | |
| return new ErrorInfo( | |
| Utils.inspectError(err), | |
| (err.code as ErrorCode) || AUTH_CALLBACK_ERROR_CODE, | |
| err.statusCode || 401, | |
| ); | |
| } | |
| if (!Utils.isErrorInfoOrPartialErrorInfo(err)) { | |
| return new ErrorInfo( | |
| Utils.inspectError(err), | |
| (err.code as ErrorCode) || (err.statusCode === 403 ? AUTH_CALLBACK_FORBIDDEN_CODE : AUTH_CALLBACK_ERROR_CODE), | |
| err.statusCode || 401, | |
| ); | |
| } |
🤖 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/auth.ts` around lines 40 - 46, Update the raw error
branch in authCallback handling to use the 403-specific fallback error code when
err.statusCode is 403 and no err.code is present, while retaining
AUTH_CALLBACK_ERROR_CODE for other statuses and existing codes.
| return config.getRandomArrayBuffer((keyLength || DEFAULT_KEYLENGTH) / 8); | ||
| } catch (err) { | ||
| throw new ErrorInfo('Failed to generate random key: ' + (err as Error).message, 400, 50000); | ||
| throw new ErrorInfo('Failed to generate random key: ' + (err as Error).message, 50000, 500); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'crypto\.ts$' . || true
echo "== file outline =="
ast-grep outline src/platform/web/lib/util/crypto.ts --view expanded || true
echo "== relevant lines =="
cat -n src/platform/web/lib/util/crypto.ts | sed -n '1,220p'
echo "== search getRandomArrayBuffer usages/definitions =="
rg -n "getRandomArrayBuffer|generateRandomKey|ErrorInfo\\(" src package.json README.md 2>/dev/null || trueRepository: ably/ably-js
Length of output: 36409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web platform config getRandomArrayBuffer =="
cat -n src/platform/web/config.ts | sed -n '60,95p'
echo "== node/react-native/nativescript config implementations =="
for f in src/platform/nodejs/config.ts src/platform/react-native/config.ts src/platform/nativescript/config.js; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -A 12 -B 3 "getRandomArrayBuffer" "$f"
fi
done
echo "== exported Crypto/generateRandomKey API references =="
rg -n "generateRandomKey|Crypto" ably src/common src/platform/web/modular src/platform/web/lib 2>/dev/null | head -n 120
echo "== ErrorInfo constructor and help behavior =="
cat -n src/common/lib/types/errorinfo.ts | sed -n '1,240p'Repository: ably/ably-js
Length of output: 12857
🌐 Web query:
W3C Web Crypto API getRandomValues exception DOMException INVALID_ACCESS_ERR zero length
💡 Result:
The W3C Web Cryptography API specification for the getRandomValues method does not define an exception specifically for zero-length arrays [1][2]. According to the official Web Crypto API documentation, the getRandomValues method is governed by two primary constraints that result in thrown exceptions: 1. Type Check: If the provided array is not one of the allowed integer-based TypedArrays (Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, BigInt64Array, or BigUint64Array), the method throws a TypeError (previously referred to as TypeMismatchError in older specifications) [1][2][3]. 2. Size Limit: If the byteLength of the array exceeds 65,536 bytes, the method throws a QuotaExceededError [1][2][3]. A zero-length array (e.g., new Uint8Array(0)) has a byteLength of 0. Since 0 is not greater than 65,536, it does not trigger the QuotaExceededError [1]. Provided the array is of an allowed integer type, the method will execute successfully—it will simply perform no operations and return the empty array [1]. If you are encountering an InvalidAccessError, it is not a standard behavior defined by the W3C Web Crypto specification for zero-length arrays [1][2].
Citations:
- 1: https://w3c.github.io/webcrypto/
- 2: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/
- 3: https://github.com/mdn/content/blob/main/files/en-us/web/api/crypto/getrandomvalues/index.md
🌐 Web query:
W3C Web Crypto getRandomValues exception type SizeMismatchError QuotaExceededError
💡 Result:
The W3C Web Cryptography API specifies that the crypto.getRandomValues() method throws specific exceptions under defined error conditions [1][2]. QuotaExceededError The crypto.getRandomValues() method throws a QuotaExceededError if the byteLength of the provided ArrayBufferView is greater than 65,536 bytes [1][2]. This is a hard limit established by the specification to manage the amount of entropy requested in a single operation [1][3][4]. TypeMismatchError The specification historically defines a TypeMismatchError that is thrown if the provided argument is not an integer-type TypedArray (such as Int8Array, Uint8Array, Int16Array, Uint16Array, Int32Array, or Uint32Array) [2][5]. Note on Exception Types While earlier versions of the W3C specification and various browser implementations explicitly referenced TypeMismatchError and QuotaExceededError [2][6][3], modern Web IDL and evolving standards have sometimes favored more generalized exceptions like TypeError (for type mismatches) and RangeError (for length constraints) to align with broader ECMAScript conventions [5]. Consequently, developers may encounter implementation-specific differences in error naming across different browsers and environments [5][7]. Always ensure code is robust by catching DOMException or checking the error type dynamically if cross-browser compatibility is required [1][8].
Citations:
- 1: https://w3c.github.io/webcrypto/
- 2: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/
- 3: https://codereview.chromium.org/87963002/diff/100001/LayoutTests/crypto/worker-random-values-limits-expected.txt
- 4: crypto/rand: panic in rand.Read() when called from wasm in a browser golang/go#46256
- 5: Errors thrown from getRandomValues in WebCrypto incompatible with current proposal tc39/proposal-csprng#5
- 6: https://bugzilla.mozilla.org/show_bug.cgi?id=713433
- 7:
crypto.getRandomValues()throws on subclasses ofUint8Arrayjsdom/jsdom#3395 - 8: https://www.typeerror.org/docs/dom/crypto/getrandomvalues
🌐 Web query:
MDN Crypto.getRandomValues DOMException QuotaExceededError MaxUint32 MAX_SAFE_INTEGER
💡 Result:
The method crypto.getRandomValues is defined by the Web Cryptography API to generate cryptographically strong random values [1][2][3]. A QuotaExceededError (which is a type of DOMException) is thrown if the byteLength of the provided TypedArray exceeds 65,536 bytes [1][4][2][3]. This limit of 65,536 (2^16) bytes is a strict specification-defined constraint [2][5][3]. Regarding the other terms mentioned: - MaxUint32 (4,294,967,295) is not the limit for this method; the limit is significantly smaller at 65,536 bytes [2][3]. - MAX_SAFE_INTEGER (2^53 - 1) is a JavaScript property representing the maximum integer that can be safely represented as a number in JavaScript [6]. It is not directly related to the entropy or byte-length limits of crypto.getRandomValues [2][3]. To handle data larger than 65,536 bytes, developers must invoke crypto.getRandomValues in a loop or process the data in smaller chunks that do not exceed the 65,536-byte limit [5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues
- 2: https://w3c.github.io/webcrypto/
- 3: https://www.w3.org/TR/webcrypto/
- 4: https://github.com/mdn/content/blob/main/files/en-us/web/api/crypto/getrandomvalues/index.md
- 5: crypto/rand: panic in rand.Read() when called from wasm in a browser golang/go#46256
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
Add a concrete remediation for generateRandomKey failures.
Crypto.generateRandomKey is exposed through the public modular generateRandomKey export, and the web implementation delegates to crypto.getRandomValues, which can fail in supported browsers for requests over 65,536 bytes or unsupported random-source failures. Wrap the error with an actionable note, e.g. if the key length exceeds 65,536 bytes, use smaller random key material and expand it securely.
🤖 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/platform/web/lib/util/crypto.ts` at line 146, Update the error handling
in Crypto.generateRandomKey to append an actionable remediation note when
crypto.getRandomValues fails, specifically directing callers to use random key
material of 65,536 bytes or less and securely expand it for larger keys.
Preserve the existing error details, ErrorInfo status values, and public
generateRandomKey behavior.
Source: Coding guidelines
1 participant
Generates the set of registered Ably error codes from the registry in
ably-commonand narrowsErrorInfo.codeto it, so that an unregistered code — or a transposedcode/statusCodepair — fails to compile.Depends on ably/ably-common#357, which adds the generator. Don't merge this before that one; see Submodule pin below.
Why
new ErrorInfo(message, code, statusCode)takes two adjacent numbers that the compiler accepted in either order, and codes were written as bare numeric literals with nothing tying them to the registry. Registry codes are 5–6 digits and an HTTP status is 3, so a status can never be a member of the generated union — a transposition is caught exactly, not heuristically.src/common/lib/types/errorcodes.tsis generated bynpm run generate:errorcodes-tsand committed. The type erases at compile time, so this costs nothing in the bundle:errorcodesappears zero times inbuild/ably-node.jsandbuild/ably.min.js.Checked vs unchecked, split by name
Errors this SDK raises are checked. Errors decoded from the server are not, because the server chooses those codes and may use ones a given client version doesn't know about.
fromValuesfromWireValueserrorfield of aProtocolMessagefromValueswas previously reached by both kinds of caller, so a single signature couldn't be both checked and honest. Splitting it makes the safe path the default and leaves every unchecked site greppable. There are ten: nine genuine server decodes, plus the application'sauthCallbackinauth.ts, whose code originates in user code and so can't be validated here.Several of those wire sites previously compiled only because they were cast to
ErrorInfo— asserting that untrusted JSON carries a registry-valid code.fromWireValuestakesIConvertibleToErrorInfo(code: number), so the typing now matches the data.ErrorInfoValues/PartialErrorInfoValuesare the checked shapes for the options-object constructor, which is the form new code tends to prefer since it's the one that carriesremediation. Closing it cost exactly one cast, because every existing site already passed a valid code.Three defects this exposed
codeandstatusCodethe wrong way round —src/common/types/http.ts(×2), the Node and webcrypto.ts, and webhttp.ts. Theirerr.code/err.statusCodewere transposed and are now correct. The web crypto site had no valid pairing in either order (400/50000); it now matches its corrected Node sibling.connectionerrors.tsunknownChannelErrusedUNKNOWN_CONNECTION_ERR(50002), which is whyUNKNOWN_CHANNEL_ERR(50001) was unused. The registry gives 50001 as Internal channel error andprotocol.tsalready uses it that way. Channel errors now report 50001.normaliseAuthcallbackErrortook its code off anany, so neither the callback's value nor the SDK's own fallbacks were checked. The fallbacks are now annotated constants; the callback's value is an explicit cast.ConnectionErrorCodesusesas const satisfies Record<string, ErrorCode>so keys stay literal and values get checked. A plainRecord<string, ErrorCode>annotation would have kept accepting a misspelled key — which is the shape of the bug fixed above.CI
Two steps in
check.yml:tsc --noEmit -p tsconfig.json— nothing typecheckedsrc/before this. The build is esbuild and the UTS suite runs undertsx; both strip types without checking them, andtsc --noEmit ably.d.ts modular.d.tsonly covered the two standalone declaration files. Without this step none of the above is enforced anywhere but an editor. It passes with no other change.git diff --exit-code— at the pinned submodule commit, so a reference to a code that isn't registered inably-commoncan't merge here first.Submodule pin
The pin is provisional. It points at the unmerged
ably-commonbrancherror-code-typescript-gen, which the two steps above need in order to run at all — the current pin496da5epredateserrors/codes/existing. Once ably/ably-common#357 merges, repoint tomain.Only
protocol/differs between the old pin and this one.test-resources/— all ably-js actually reads from the submodule — is byte-identical, so the bump is safe for the fixtures.Note that because the drift check runs against the pinned commit, CI here will fail if that branch is force-pushed while this PR is open. That's expected, not a real failure.
Verification
tsc --noEmit -p tsconfig.json: 0 errorserrorcodesabsent from both bundlesfromValues; valid codes and the explicitfromWireValuesescape hatch accepted🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Type Safety