DX-2077: fix failure function response - json parsing issue #143
Conversation
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
…etryable error is thrown
…ter error handling
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a JSON parsing issue that occurred when failure functions returned custom responses in Next.js Pages Router and Express handlers. The changes ensure all responses are JSON-parsable by wrapping failure function results in a { result: <value> } format.
Key changes:
- Replaced
instanceofchecks with a customisInstanceOfhelper function across the codebase - Modified failure callback response handling to always return JSON-parsable objects
- Added test coverage for failure function responses (empty and string returns)
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/error.ts | Added isInstanceOf helper function to handle instance checks across different contexts |
| src/serve/options.ts | Wrapped failure callback results in JSON object format |
| src/serve/index.ts | Wrapped error messages in JSON format and replaced instanceof check |
| src/workflow-requests.ts | Replaced instanceof checks with isInstanceOf helper |
| src/context/auto-executor.ts | Replaced instanceof checks with isInstanceOf helper |
| src/client/utils.ts | Replaced instanceof checks with isInstanceOf helper |
| src/agents/agent.ts | Replaced instanceof checks with isInstanceOf helper |
| src/agents/adapters.ts | Replaced instanceof checks with isInstanceOf helper |
| platforms/nextjs.ts | Extracted response.json() to separate variable before passing to res.json() |
| platforms/express.ts | Extracted response.json() to separate variable before passing to res.json() |
| examples/nextjs-pages/src/pages/api/ci.ts | Added failure function test implementation with headers and return scenarios |
| examples/nextjs-pages/ci.test.ts | Added tests for empty and string failure function responses |
| examples/express/api/index.ts | Added failure function test implementation with headers and return scenarios |
| examples/express/ci.test.ts | Added tests for empty and string failure function responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
There was a type error in Pinning the versions to |
alitariksahin
approved these changes
Oct 28, 2025
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.
after we made it possible to return a custom response in failure function, serve methods we wrote for nextjs-pages/express started to get errors if failure function was executed
This is because these handlers try to parse the response into a JSON before passing this body to their own respective response objects.
Our fix will be always passing a json parsable object to the response if failure function runs in the
{ result: <failureFunctionResonse>}format, where<failureFunctionResonse>is a string.Error Handling
While checking this issue in express/nextjs-pages examples, I noticed that we weren't returning the correct response status (489) when non retryable error was thrown. 500 was being thrown instead.
I noticed that this was happening because
instanceofdidn't work as expected in some frameworks. So replaced theerror instanceof SomeErrorusages with a helper I borrowed from here