Repository navigation
feat(core): add extension() registrar for SEP-2133 capability-aware custom methods - #1868
Closed
felixweinberger wants to merge 7 commits into
Closed
felixweinberger wants to merge 7 commits into
felixweinberger wants to merge 7 commits into
Claude / Claude Code Review
completed
Apr 13, 2026 in 5m 8s
Code review found 2 potential issues
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/core/src/shared/extensionHandle.ts:158-168 |
_getPeerCapabilitiesPresent() returns stale true after close(), so post-close strict sends still misreport CapabilityNot |
| 🟡 Nit | packages/core/src/errors/sdkErrors.ts:13 |
ExtensionAlreadyRegistered enum member lacks JSDoc and is awkwardly placed |
Annotations
Check warning on line 168 in packages/core/src/shared/extensionHandle.ts
claude / Claude Code Review
_getPeerCapabilitiesPresent() returns stale true after close(), so post-close strict sends still misreport CapabilityNotSupported
nit: The 9a22a160 fix only covers the pre-first-connect path — `_getPeerCapabilitiesPresent()` keys off `this._serverCapabilities \!== undefined` (and `_clientCapabilities` on Server), but neither field is cleared by `close()`/`_onclose()`. So after `await client.close()`, in strict mode `handle.sendRequest()` still throws `CapabilityNotSupported` against the *previous* peer's caps instead of surfacing `NotConnected`. Consider keying the getter off live transport state (`() => this.transport \!=
Check warning on line 13 in packages/core/src/errors/sdkErrors.ts
claude / Claude Code Review
ExtensionAlreadyRegistered enum member lacks JSDoc and is awkwardly placed
The new `ExtensionAlreadyRegistered` member is missing a JSDoc comment and is wedged between `NotConnected` and the `/** Transport is already connected */` comment for `AlreadyConnected`, breaking up an obvious pair. Consider adding a JSDoc line (e.g. `/** Extension ID is already registered via Client.extension()/Server.extension() */`) and moving it after `AlreadyConnected`.
Loading