feat(client): add BashClient.batchGetEvents server-side batch endpoint#245
Conversation
Adds coverage for the server-side bash batch endpoint:
- GET /api/bash/bash_events/ -> BashClient.batchGetEvents()
This endpoint is declared GET-with-required-body, which `fetch` rejects
("Request with GET/HEAD method cannot have body"). HttpClient now routes
GET-with-body requests through Node's http(s) module (requestWithBodyOnGet
+ parseNodeResponse), mirroring the fetch path's URL/header/error/parse
behavior; every other request is untouched. Unlike getEvents (one request
per id), batchGetEvents returns one entry per requested id with null in
the slots of missing events, preserving input order.
Includes a unit test (mocked node:http, asserting method/URL/body and
the null-for-missing mapping) and an integration assertion in
bash.integration.test.ts (real ids round-trip, a bogus id comes back as
null in place).
Endpoint audit❌ 7 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 7)⛔ (no known backend) — served by no backend we can see (6)
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — The approach is pragmatic and correct: fetch genuinely rejects GET bodies, routing those calls through Node's http/https module is the right workaround, and the early-return guard in request() is tight enough that no existing call is affected. Both a unit test (mocked transport, asserts method/URL/body/null-mapping) and an integration test (real round-trip with a bogus-id slot check) are present.
One structural issue is worth cleaning up — particularly because the PR description notes an identical pattern is coming in the conversations batch-event PR: the URL-construction and params-handling block is copy-pasted verbatim between request() and requestWithBodyOnGet(). See inline comment.
[IMPROVEMENT OPPORTUNITIES]
- [src/client/http-client.ts] Duplication: URL-building block copy-pasted from
request()— extract abuildUrl()helper (see inline). - [src/tests/api-clients.test.ts, Line 2150] Noise comment: Repeats the module-mock preamble already present at the top of the file.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
HttpClient.request()is a widely-imported shared utility and this PR adds an early-return branch to it. The guard condition (method === 'GET' && data !== undefined && data !== null) is narrow and correct — no existing call passes a non-null body on a GET, so the fetch path is untouched — but any shared core utility that gains conditional routing deserves attention. Mitigated by the tight guard, full unit coverage, and an integration test.
VERDICT:
✅ Worth merging: Core logic is correct and well-tested. The URL-duplication is worth fixing before the conversations batch-event companion PR lands (to avoid a third copy).
KEY INSIGHT:
The URL-building block shared between request() and requestWithBodyOnGet() will be copied a third time when the conversations batch-endpoint PR merges — extract buildUrl() now rather than after three copies exist.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
@OpenHands adress the suggestions |
|
I'm on it! VascoSch92 can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Done.
PR CI is currently pending on the latest commit. |
|
🚀 Released in v1.32.0. |
3 participants
Summary
Split from #231 (one feature per PR). Adds client coverage for the server-side bash batch endpoint that the audit reported as missing.
GET /api/bash/bash_events/(batch)BashClient.batchGetEvents()Details
fetchrejects (TypeError: Request with GET/HEAD method cannot have body).HttpClientnow routes GET-with-body requests through Node'shttp/httpsmodule (requestWithBodyOnGet+parseNodeResponse), mirroring the fetch path's URL/header/error/parse behavior; every other request is untouched.getEvents(which fans out one request per id),batchGetEventscalls the server-side batch endpoint and returns one entry per requested id, withnullin the slots of any missing event — preserving input order.Testing
src/__tests__/api-clients.test.ts): mocksnode:http(the GET-body transport) and asserts the request method/URL/body and the null-for-missing mapping.bash.integration.test.ts): real event ids round-trip through the batch endpoint and a bogus id comes back asnullin place.npm run build,lint(0 errors), andformat:checkpass; full unit suite green (270).