fix(executor): harden JSON parsing and error visibility in block handlers#4788
fix(executor): harden JSON parsing and error visibility in block handlers#4788rpatricksmith wants to merge 2 commits into
Conversation
…lers Replace bare JSON.parse() calls across 5 executor handler files with the project's own parseJSON/parseJSONOrThrow utilities from executor/utils/json.ts. Add logger.warn to 2 empty catch blocks in the router handler that previously swallowed errors silently. The utilities already exist and are imported by some of these files (for parseObjectStrings). This makes handlers consistent with the project's established parsing patterns. Also adds test coverage for error paths: 2 new test files (response-handler, human-in-the-loop-handler) and error-path test cases for 3 existing test files (router, api, generic handlers).
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview API, generic, response, and human-in-the-loop handlers use Tests cover malformed JSON fallbacks, non-JSON provider errors, invalid route strings, and new suites for response and human-in-the-loop handlers. Reviewed by Cursor Bugbot for commit 7308b53. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR replaces bare
Confidence Score: 4/5Safe to merge; all changed handlers preserve correct fallback behaviour and the router's previously-silent error-body catches now log properly. The functional behaviour of every handler is preserved — invalid JSON still falls back to the original value or an empty array, and execution continues correctly. The two issues flagged are purely observability regressions: router-handler.ts deserves a second look on the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Block Handler receives input] --> B{Handler type?}
B -->|API / Generic / Response / HITL| C[parseJSON value, fallback]
C --> D{Valid JSON?}
D -->|Yes| E[Return parsed object]
D -->|No| F[Return fallback string — silent, no log]
B -->|Router V2 — route selection| G[parseJSONOrThrow result.content]
G --> H{Valid JSON?}
H -->|Yes| I[Extract route + reasoning]
H -->|No| J[Catch _parseError → log error → use raw content]
B -->|Router — error response| K[response.json try/catch]
K -->|Success| L[Use errorData.error message]
K -->|Failure| M[logger.warn — was empty catch before]
B -->|Router V2 — parseRoutes| N[parseJSON input, empty-array fallback]
N --> O{!Array.isArray parsed?}
O -->|true — valid but wrong type| P[logger.error + return empty array]
O -->|false — invalid JSON returns fallback| Q[Return empty array — silent, no log]
Reviews (1): Last reviewed commit: "fix(executor): harden JSON parsing and e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private parseRoutes(input: any): RouteDefinition[] { | ||
| try { | ||
| if (typeof input === 'string') { | ||
| return JSON.parse(input) | ||
| } | ||
| if (Array.isArray(input)) { | ||
| return input | ||
| } | ||
| return [] | ||
| } catch (error) { | ||
| logger.error('Failed to parse routes:', { input, error }) | ||
| if (Array.isArray(input)) { | ||
| return input | ||
| } | ||
| const parsed = parseJSON(input, []) | ||
| if (!Array.isArray(parsed)) { | ||
| logger.error('Failed to parse routes:', { input }) | ||
| return [] | ||
| } | ||
| return parsed | ||
| } |
There was a problem hiding this comment.
!Array.isArray(parsed) guard never fires for invalid JSON inputs. When input is a malformed JSON string, parseJSON(input, []) silently returns the [] fallback, which passes the Array.isArray check — so logger.error is dead code for the most common failure case (parse errors). The old implementation explicitly caught SyntaxError and logged both the input and the error. Consider restoring a try/catch for this method so that both invalid JSON and unexpected-type cases are logged.
| private parseRoutes(input: any): RouteDefinition[] { | |
| try { | |
| if (typeof input === 'string') { | |
| return JSON.parse(input) | |
| } | |
| if (Array.isArray(input)) { | |
| return input | |
| } | |
| return [] | |
| } catch (error) { | |
| logger.error('Failed to parse routes:', { input, error }) | |
| if (Array.isArray(input)) { | |
| return input | |
| } | |
| const parsed = parseJSON(input, []) | |
| if (!Array.isArray(parsed)) { | |
| logger.error('Failed to parse routes:', { input }) | |
| return [] | |
| } | |
| return parsed | |
| } | |
| private parseRoutes(input: any): RouteDefinition[] { | |
| if (Array.isArray(input)) { | |
| return input | |
| } | |
| if (typeof input === 'string') { | |
| try { | |
| const parsed = JSON.parse(input.trim()) | |
| if (!Array.isArray(parsed)) { | |
| logger.error('Failed to parse routes: expected array', { input }) | |
| return [] | |
| } | |
| return parsed | |
| } catch (error) { | |
| logger.error('Failed to parse routes:', { input, error }) | |
| return [] | |
| } | |
| } | |
| return [] | |
| } |
| if (inputType === 'json' || inputType === 'array') { | ||
| try { | ||
| finalInputs[key] = JSON.parse(value.trim()) | ||
| } catch (error) { | ||
| logger.warn(`Failed to parse ${inputType} field "${key}":`, { | ||
| error: toError(error).message, | ||
| }) | ||
| } | ||
| finalInputs[key] = parseJSON(value, value) | ||
| } |
There was a problem hiding this comment.
logger.warn(\Failed to parse ${inputType} field "${key}":`, { error })on parse failure. Replacing it withparseJSON(value, value)— which silently returns the fallback — drops that diagnostic. The same pattern applies inresponse-handler.tsandhuman-in-the-loop-handler.ts, both of which also had logger.warn('Failed to parse JSON data, returning as string:', error)` before. With these warnings gone, malformed-JSON inputs in production are now invisible in logs until a downstream symptom appears.
parseJSON(input, []) returns [] on parse failure, making the subsequent !Array.isArray check unreachable for the invalid-JSON case. Use null as the fallback to distinguish parse failure from valid-but- not-array, and log both cases separately.
1 participant
Summary
Replace bare
JSON.parse()calls in executor block handlers with the project's ownparseJSON/parseJSONOrThrowutilities fromexecutor/utils/json.ts, and add logging to empty catch blocks that silently swallow errors.The Problem
Five executor handler files use bare
JSON.parse()instead of the project's own parsing utilities —parseJSON(value, fallback)andparseJSONOrThrow(value)inexecutor/utils/json.ts. These utilities already handle trimming, fallback values, and enriched error messages. The handlers bypass them, creating inconsistency and missing out on the error enrichment.Additionally, the router handler has 2 empty
catch (_e) {}blocks (lines 121, 270) that silently swallow provider API error response parsing failures. When a provider returns a non-JSON error page (e.g., HTML 502), the error vanishes — making debugging production failures harder.The utilities are already imported by some of these files (for
parseObjectStrings). The import path exists — handlers just drifted from using it for JSON parsing.The Fix
Handler changes (5 files):
response-handler.ts—JSON.parse(inputs.data)→parseJSON(inputs.data, inputs.data)(fallback to original string)human-in-the-loop-handler.ts— identical change to response-handler (duplicatedparseResponseDatamethod)router-handler.ts— empty catches →logger.warn(...),JSON.parse(result.content)→parseJSONOrThrow(result.content),parseRoutes→parseJSON(input, [])api-handler.ts—JSON.parse(trimmedBody)+ empty catch →parseJSON(trimmedBody, processedInputs.body)generic-handler.ts—JSON.parse(value.trim())→parseJSON(value, value)What's NOT changed:
anytypes in handler signatures — separate concernTest additions (5 files, 2 new):
response-handler.test.ts— canHandle, structured data, JSON data, malformed JSON fallback, catch-all error pathhuman-in-the-loop-handler.test.ts— canHandle, human operation execution, malformed JSON, catch-all error pathrouter-handler.test.ts— legacy/V2 non-JSON error response handling, invalid JSON routesapi-handler.test.ts— malformed JSON body keeps original stringgeneric-handler.test.ts— malformed JSON field keeps original stringTesting
171 handler tests pass. Lint clean. No regressions. 3 pre-existing PostCSS config failures in unrelated handler suites (agent-handler, function-handler, workflow-handler).
Found while analyzing the codebase with Anatomia. Full analysis →