feat(client): add ConversationClient.batchGetEvents server-side batch endpoint#246
feat(client): add ConversationClient.batchGetEvents server-side batch endpoint#246VascoSch92 wants to merge 1 commit into
Conversation
… endpoint
Adds coverage for the server-side conversation events batch endpoint:
- GET /api/conversations/{id}/events -> ConversationClient.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, preserving
input order.
Includes a unit test (mocked node:http, asserting method/URL/body and
the response mapping) and an integration test that round-trips a real
event id through the batch endpoint.
Endpoint audit❌ 8 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 8)⛔ (no known backend) — served by no backend we can see (7)
|
|
✅ 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.
🔴 Taste Rating: Needs improvement — the endpoint wrapper is small, but the transport change cuts across the shared HTTP client and violates the repository's browser-compatibility contract.
[CRITICAL ISSUES]
- The shared
HttpClientnow routes GET-with-body through Nodehttp/httpsandBuffer.AGENTS.mdexplicitly says allsrc/code must be browser-compatible and must not depend on Node built-ins, so this would break browser consumers of the SDK.
[IMPROVEMENT OPPORTUNITIES]
batchGetEvents()promises the same null-slot behavior asgetEvents(), but the integration test documents that the real server endpoint raises on unknown IDs. Align the public type/docs/tests with the actual endpoint, or implement the promised fallback semantics.
[TESTING GAPS]
- CI covers Node and the deterministic server route, but there is no browser/bundler guard that would catch
node:http,node:https, orBufferentering the exported SDK surface.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This modifies the central HTTP transport used by public clients and introduces Node-only APIs into a package whose repository contract requires browser-compatiblesrc/code. The blast radius is broad becauseHttpClientis shared across the SDK and exported directly. Recommendation: Do not auto-merge. Request human maintainer/architect review to decide how GET-with-body endpoints should be exposed without breaking browser consumers.
VERDICT:
❌ Needs rework: The new endpoint should not land by adding Node-only transport into the shared browser-compatible client.
KEY INSIGHT:
The hard problem is not adding batchGetEvents(); it is deciding how this SDK represents a server API shape that browsers fundamentally cannot send with fetch.
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 (e.g., "Security concerns about X do not apply here because Y"). 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.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| } | ||
|
|
||
| const transport = | ||
| url.protocol === 'https:' ? await import('node:https') : await import('node:http'); |
There was a problem hiding this comment.
🔴 Critical: This introduces Node-only transport into src/, which violates the repository's hard browser-compatibility requirement in AGENTS.md (src/ must not depend on Node built-ins). Browser consumers and bundlers can fail on node:http/node:https, and this path also uses Buffer above. Please do not put a Node transport behind the shared HttpClient; either keep the exported client browser-safe and surface this server-contract limitation clearly, or isolate Node-only support behind a separate Node-specific entrypoint/adapter that browser builds never import.
| * | ||
| * Unlike {@link getEvents} (which fans out one request per id), this calls the | ||
| * server-side batch endpoint and returns one entry per requested id, with | ||
| * `null` in the slots of any missing event — preserving input order. |
There was a problem hiding this comment.
🟠 Important: This promised null-slot contract does not match the real endpoint behavior documented by the new integration test, which says unknown conversation event IDs make the endpoint raise instead of returning null. The public type and unit test now teach consumers to expect getEvents() semantics, but a missing ID will reject the whole batch. Either change the docs/types/tests to state that this batch endpoint only succeeds for existing IDs, or add client logic that actually preserves the Array<Event | null> behavior.
2 participants
Summary
Split from #231 (one feature per PR). Adds client coverage for the server-side conversation events batch endpoint that the audit reported as missing.
GET /api/conversations/{id}/events(batch)ConversationClient.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, 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 response mapping (real id resolved, missing slotnull).deterministic-api.integration.test.ts): round-trips a real event id through the batch endpoint and asserts the route resolves and preserves order. (The conversation endpoint raises on unknown ids rather than returningnull, so this exercises the happy path with an id that exists.)npm run build,lint(0 errors), andformat:checkpass; full unit suite green (270).