Code Review — AbortSignal cancellation (#3463)
Reviewed locally on zen/sdk-ts-request-options-3nloh3 vs next via a multi-agent pass (TypeScript quality, async/race, architecture, simplicity) followed by manual verification of every high-severity claim. Verdict: no merge-blockers. The implementation is careful, consistently applied across ~25 methods, and backed by a dedicated +451-line test file. Findings below are correctness-clarifications and cleanups.
What's good
withCancellation() as a single normalization choke point and ComposioRequestCancelledError (stable code, instanceof-friendly) is the right public shape.- Stripping
signal out of the modifier bag in tools.get/tools.execute (Tools.ts:759, :1058) so an expired AbortSignal.timeout() can't poison subsequent calls — sharp, well-commented.
- Post-execute return-not-throw semantics avoid encouraging retries of non-idempotent operations — good judgment.
- Pre-execute
signal.aborted short-circuits before invoking user code; cooperative signal forwarded to custom tools via ctx.
🟡 P2 — Should address
1. applyAfterExecuteModifiers silently skips the user's afterExecute on a late abort — Tools.ts:329-331, :342-344
Once the remote tool has executed, an abort firing before post-processing returns the raw result without running the caller's afterExecute modifier. If a caller uses afterExecute for bookkeeping/dedup/logging of a side-effecting call (e.g. an email that was already sent), that bookkeeping is silently dropped — a half-finished pipeline that looks like success-shaped data. The two early-returns are also inconsistent: with auto-download on you return a partially modified result; with it off you return the un-downloaded one. Recommendation: once the remote call has committed, run afterExecute to completion (the side effect already happened — honor the lost race), or document the skip explicitly. Pick one policy for both return points.
2. Type-cast smells that can drift silently
CustomTools.ts:335 — ... } as Parameters<typeof this.client.tools.proxy>[0] widens the whole literal, defeating field-level checking on every key just to widen the one deprecated custom_connection_data field. Type the local explicitly (Parameters<ComposioClient['tools']['proxy']>[0]) and cast only custom_connection_data.CustomTools.ts:365-372 — const exec = execute as (input, connectionConfig, fn, ctx?) => Promise<...> re-states a fresh function signature that will compile even if the real execute signature drifts. Prefer narrowing on the toolkitSlug discriminant, or derive the cast from ToolkitBasedExecute<T>['execute'].
3. ComposioRequestOptions duplicates the client's transport type with no type-level link — types/requestOptions.types.ts:30
{ signal } is a hand-rolled subset of @composio/client's RequestOptions (which already has signal, timeout, maxRetries, headers). The forward works only by structural coincidence. Define it as Pick<ClientRequestOptions, 'signal'> (or add an assignability assertion) so future additions stay provably compatible and don't silently diverge.
🔵 P3 — Nice-to-have
4. (this as SessionContext).signal is a load-bearing self-cast — SessionContext.ts:64, :108
SessionContextImpl never declares a signal field; the cast only works because executeCustomTool grafts signal onto an Object.create(ctx) child. It reads as undefined on the base instance. Declare readonly signal?: AbortSignal on the class (or drop the read) so the contract is explicit rather than implicit. (The "cross-request bleed" concern raised in review is not present today — each call gets a fresh Object.create child, never an assign onto the shared base — but the invariant is invisible and worth a comment + a concurrent-execution test to lock it in.)
5. withCancellation's separate signal argument is redundant boilerplate — utils/cancellation.ts + ~50 call sites
Every call is withCancellation(() => client.x(p, requestOptions), requestOptions?.signal) — the closure already closes over requestOptions. The catch only uses signal?.aborted as a belt-and-suspenders guard alongside isRequestAbortError(error). Dropping the 2nd arg removes ~50 trailing lines. (Behavior note: keeps semantics, since the client only emits APIUserAbortError/AbortError when it aborts on the passed signal.)
6. _isRequestAbortErrorAt is largely defensive against shapes the client never produces — errors/SDKErrors.ts:70-92
The client throws APIUserAbortError directly and un-nested on abort, so instanceof APIUserAbortError already covers the real path. The .cause recursion + depth>5 guard + triple name-check (one disjunct, error.name === 'APIUserAbortError', is only ever true in tests) can be trimmed to instanceof + a single name === 'AbortError' / constructor.name fallback. If you keep the depth cap, extract it as a named const.
7. Repeated cancellation re-throw guard across ~8 catch blocks — Tools.ts:656,:946; CustomTools.ts:206; ConnectedAccounts.ts; Toolkits.ts; etc.
if (error instanceof ComposioRequestCancelledError) throw error; before domain-error wrapping is copy-pasted everywhere. Factor a rethrowCancellationOr(error, wrap) helper so the policy lives in one place and a future catch-and-wrap can't forget it.
8. Minor
signal: requestOptions?.signal ?? undefined — the ?? undefined is a no-op on an AbortSignal | undefined; drop it (CustomTools.ts:372, ToolRouterSession.ts:525).ctx?: { signal?: AbortSignal } inline types duplicate the exported CustomToolExecuteContext — reference it (CustomTools.ts:369, customToolExecution.ts:67).connectedAccounts.create mock-fallback path (response = await apiCall) isn't wrapped in withCancellation — affects test mocks only.waitForConnection polls HTTP but takes only a timeout, no AbortSignal — minor coverage gap, mitigated by its existing timeout.- TOCTOU in
withCancellation classification is real but narrow (dual-predicate guards misclassification). Worth a regression test: signal supplied, unrelated failure, signal aborts late → asserts the raw error is not relabeled.
Detailed findings tracked as todos/043–todos/049. Generated via /compound-engineering:workflows:review.
@zen-agent