fix(nodejs): validate sendAndWait timeout to fix malformed error message#1700
Open
gimenete wants to merge 3 commits into
Open
fix(nodejs): validate sendAndWait timeout to fix malformed error message#1700gimenete wants to merge 3 commits into
gimenete wants to merge 3 commits into
Conversation
When sendAndWait() received a non-numeric timeout (e.g. an object or NaN), the value was passed straight to setTimeout(), which coerced it to a 0ms delay. The session then rejected almost immediately with a misleading "Timeout after [object Object]ms waiting for session.idle" message rather than surfacing the real problem. Validate the timeout argument up front and throw a clear TypeError when it is not a non-negative finite number of milliseconds, so callers get an actionable error instead of a spurious immediate timeout. Note: this addresses the malformed-message bug from github#915. The separate report that resumeSession() with customAgents never reaches idle is a CLI runtime behavior and is not addressable in the SDK. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds validation for the CopilotSession.sendAndWait() timeout argument to prevent non-numeric values from being coerced by setTimeout(), and adds regression tests to ensure a clear TypeError is thrown for invalid timeouts.
Changes:
- Add runtime guard in
sendAndWait()to reject non-finite, non-number, or negative timeouts with aTypeError - Document the new
TypeErrorbehavior in the JSDoc - Add Jest tests covering object,
NaN, and negative timeout inputs (regression for #915)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nodejs/test/client.test.ts | Adds regression tests ensuring invalid timeout inputs fail with a clear error. |
| nodejs/src/session.ts | Implements and documents invalid timeout validation to prevent setTimeout() coercion. |
- Report a null timeout as "null" instead of "object" in the error message - Capture the rejection once per test instead of invoking sendAndWait twice - Assert the full error message (including the sendAndWait prefix) so the regression tests don't match unrelated errors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
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.
Summary
Fixes the malformed timeout error reported in #915.
When
session.sendAndWait()received a non-numerictimeout(e.g. an object orNaN), the value was passed straight tosetTimeout(), which coerces an invalid delay to 0ms. The session then rejected almost immediately with a misleading message:instead of surfacing the real problem (an invalid argument).
This PR validates the
timeoutargument up front and throws a clearTypeErrorwhen it is not a non-negative finite number of milliseconds:This also fixes the secondary symptom of a spurious immediate timeout caused by the
NaN/object →0mscoercion.Changes
nodejs/src/session.ts: validatetimeoutinsendAndWait()before use; document the new@throwsin the JSDoc.nodejs/test/client.test.ts: add regression tests covering object,NaN, and negative timeouts, asserting the error message no longer contains[object Object].Scope note
#915 also reports that
resumeSession()withcustomAgentsnever reachesidle. The SDK already forwardscustomAgentsin thesession.resumepayload, so that behavior is on the CLI runtime side and is not addressable in this SDK. This PR therefore targets only the malformed-message bug.Testing
From
nodejs/:npx vitest run test/client.test.ts -t "sendAndWait timeout validation"(fails before the fix, passes after)npx vitest run test/client.test.ts— all 111 passnpm run lint— 0 errorsnpm run format:check— cleannpm run typecheck— clean