tests(#487): 24 unit tests for cli/execute-handler.js#332
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a new Jest test file Changesexecute-handler test coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/execute-handler.test.js`:
- Around line 305-374: Add a test in handlePlan for the non-OK fetch response
path, since fetch only rejects on network errors and 4xx/5xx responses can
currently slip through as success. In __tests__/execute-handler.test.js, mock
global.fetch to resolve with ok false and a JSON body/error payload, then assert
handlePlan calls io.outputError with the integration_error payload (similar to
the existing fetch-throws case) rather than io.output. Use the existing
handlePlan, makeIoPlan, and makeCmd helpers to keep the new case aligned with
the current server tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1a4c66-7ed2-4c9a-917d-e9c4cafc8c9e
📒 Files selected for processing (1)
__tests__/execute-handler.test.js
| test("with server: fetches /api/plans and outputs annotated plan", async () => { | ||
| const serverPlan = { plan_id: "sp1", steps: [] }; | ||
| const annotated = { ...serverPlan, persisted: true, execution_mode: "server" }; | ||
| annotateServerPlan.mockReturnValue(annotated); | ||
| global.fetch = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValue(serverPlan), | ||
| }); | ||
| const io = makeIoPlan({ hasServer: true, humanMode: false }); | ||
| const cmd = makeCmd(); | ||
|
|
||
| await handlePlan(cmd, { q: "b" }, io); | ||
|
|
||
| expect(global.fetch).toHaveBeenCalledWith( | ||
| "https://srv.io/api/plans", | ||
| expect.objectContaining({ method: "POST" }) | ||
| ); | ||
| expect(annotateServerPlan).toHaveBeenCalledWith(serverPlan); | ||
| expect(io.output).toHaveBeenCalledWith(annotated); | ||
| }); | ||
|
|
||
| test("with server + human mode: calls outputHumanPlan with annotated plan", async () => { | ||
| const annotated = { plan_id: "sp2", persisted: true, steps: [] }; | ||
| annotateServerPlan.mockReturnValue(annotated); | ||
| global.fetch = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValue({}), | ||
| }); | ||
| const io = makeIoPlan({ hasServer: true, humanMode: true }); | ||
|
|
||
| await handlePlan(makeCmd(), {}, io); | ||
|
|
||
| expect(outputHumanPlan).toHaveBeenCalledWith(annotated); | ||
| expect(io.output).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("with server: POST body includes command and args", async () => { | ||
| annotateServerPlan.mockReturnValue({}); | ||
| global.fetch = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValue({}), | ||
| }); | ||
| const io = makeIoPlan({ hasServer: true }); | ||
| const cmd = makeCmd(); | ||
|
|
||
| await handlePlan(cmd, { limit: 10 }, io); | ||
|
|
||
| const [, opts] = global.fetch.mock.calls[0]; | ||
| const body = JSON.parse(opts.body); | ||
| expect(body.command).toBe("ai.text.summarize"); | ||
| expect(body.args).toEqual({ limit: 10 }); | ||
| expect(body.cmd).toEqual(cmd); | ||
| }); | ||
|
|
||
| test("with server + fetch throws: calls outputError with code 105", async () => { | ||
| global.fetch = jest.fn().mockRejectedValue(new Error("network failure")); | ||
| const io = makeIoPlan({ hasServer: true }); | ||
|
|
||
| await handlePlan(makeCmd(), {}, io); | ||
|
|
||
| expect(io.outputError).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| code: 105, | ||
| type: "integration_error", | ||
| recoverable: true, | ||
| }) | ||
| ); | ||
| const err = io.outputError.mock.calls[0][0]; | ||
| expect(err.message).toContain("network failure"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Node 18 / WHATWG fetch, does fetch reject on HTTP 4xx/5xx responses or only on network failures?
💡 Result:
In Node.js 18 and later (which implements the WHATWG Fetch standard), the fetch promise does not reject on HTTP 4xx or 5xx error responses [1][2]. The promise only rejects on network failures, such as DNS issues, connection refusal, TLS handshake errors, or when the request is aborted [1][2][3]. Because HTTP 4xx and 5xx responses are considered successful HTTP transactions, the fetch promise will resolve normally with a Response object [2][3]. To handle these HTTP error statuses, you must manually check the response.ok property (which is false for statuses outside the 200–299 range) or the response.status property [1][4]. Common practice involves creating a wrapper or a helper function to check the status code and throw an error manually if needed [5][2][6]. Example of manual error handling: async function safeFetch(url) { const response = await fetch(url); if (!response.ok) { throw new Error(HTTP error! Status: ${response.status}); } return response.json; }
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch
- 2: https://devcraftly.com/nodejs/native-fetch/
- 3: https://kettanaito.com/blog/why-fetch-promise-doesnt-reject-on-error-responses
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch?undefined=
- 5: https://stackoverflow.com/questions/38235715/fetch-reject-promise-and-catch-the-error-if-status-is-not-ok
- 6: https://github.com/node-fetch/node-fetch/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the handler and the test file, then inspect the relevant ranges.
git ls-files | rg '^(cli/execute-handler\.js|__tests__/execute-handler\.test\.js)$'
echo '--- execute-handler.js outline ---'
ast-grep outline cli/execute-handler.js --view expanded || true
echo '--- execute-handler.test.js outline ---'
ast-grep outline __tests__/execute-handler.test.js --view expanded || true
echo '--- execute-handler.js relevant lines ---'
sed -n '1,140p' cli/execute-handler.js | cat -n
echo '--- execute-handler.test.js relevant lines ---'
sed -n '250,420p' __tests__/execute-handler.test.js | cat -nRepository: javimosch/supercli
Length of output: 10526
Add a non-OK response case for handlePlan.
fetch() only rejects on network failures, so a 4xx/5xx JSON response currently bypasses outputError and is treated as a successful plan. Cover that path here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/execute-handler.test.js` around lines 305 - 374, Add a test in
handlePlan for the non-OK fetch response path, since fetch only rejects on
network errors and 4xx/5xx responses can currently slip through as success. In
__tests__/execute-handler.test.js, mock global.fetch to resolve with ok false
and a JSON body/error payload, then assert handlePlan calls io.outputError with
the integration_error payload (similar to the existing fetch-throws case) rather
than io.output. Use the existing handlePlan, makeIoPlan, and makeCmd helpers to
keep the new case aligned with the current server tests.
|
Review — Head of Org Engineering: The PR adds a new test file (approved — review-only mode; merge when ready.) |
1 participant
What
24 unit tests for
cli/execute-handler.jscovering all three exported functions.Coverage
handleExecutecmd,uFlags,config, andSERVERtoexecute()SERVERis falsyonStreamEventisnullwhenadapterConfigis absent orstreamis not"jsonl"makeStreamEmitterand forwards the result asonStreamEventwhenstream === "jsonl"writeLogreceives the correct command string, args,status: "success",duration_ms,timestamp, andclient_id(fromgetClientId())outputcalled with theversion: "1.0"envelopeoutputHumanTablecalled with full array and column mapconsole.logJSON.stringifyappliedexecute()propagates —outputandwriteLogare not calledhandlePlanbuildLocalPlan(cmd, args)called; result passed tooutputoutputHumanPlancalled instead ofoutputfetchPOST to/api/plans; result passed throughannotateServerPlan;outputcalledoutputHumanPlancalled with annotated plancommand,args, andcmdfetchthrows →outputError({ code: 105, type: "integration_error", recoverable: true, ... })handleExecutePlanfetchPOST to/api/plans/{planId}/executeoutputfetchthrows →outputError({ code: 105, ... });outputnot calledHow verified
Closes #487 (mago task #487)
Summary by CodeRabbit