Repository navigation
Conversation
Closes #5035 Co-authored-by: openhands <openhands@all-hands.dev>
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Co-authored-by: openhands <openhands@all-hands.dev>
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Reviewed head 7b69c3a776d8897a6fbe39896e88701ca55b9f6f (fetched and checked out detached). Scope: clients/typescript/ is owned by this repository (it mirrors the Agent Server API), so the change belongs here.
Verdict: no material findings. The fix is targeted and well tested.
What I verified against the workspace:
- Traced the error boundary in
HttpClient.request(src/client/http-client.ts): lost sockets and aborts surface as plainError(Request failed: .../Request timeout after ...), while non-2xx responses raiseHttpError. The recovery predicate!(error instanceof HttpError)matches exactly that boundary, so explicit HTTP failures (400/401/422/...) still propagate and the testdoes not conceal server validation failuresis correct. - Confirmed the create POST is never replayed: recovery uses bounded
GET /api/conversations/{id}keyed on the caller's stablepayload.conversation_id(the same fieldConversationConfig.conversation_idaccepts server-side), with a per-request timeout clamped to the remaining deadline and a404-only continue condition. Any non-404HttpError(e.g. 401) breaks immediately and the original transport error is rethrown, matching the acceptance criteria. - Ran the focused suite (
conversation-create-recovery.test.ts): 6/6 pass; full suite: 20 suites / 324 tests pass;npm run buildandnpm run lintpass (only pre-existing warnings elsewhere). - All GitHub Actions check-runs for this head are
successor intentionallyskipped(build, coverage-report, agent-server-tests, integration-test, windows-tests, REST API/OpenAPI, public-type-budget, etc.).
Non-blocking observation (not a blocker): the recovery window is only configurable by constructing ConversationClient directly — creationRecoveryTimeout is not plumbed through OpenHandsClientOptions/AgentServerClient, so agentServer.conversations callers can only use the 120s default. That default also means a caller who supplied conversation_id waits the full window before the original transport error surfaces when the server is genuinely unreachable. The trade-off is documented in the PR notes and is intentional, so this is a follow-up consideration, not a merge blocker.
✅ APPROVED
The recovery tests asserted the client rethrows `Unknown request error`, which only held under Jest: there, the failed `fetch` surfaced as a non-Error. After the client moved to Vitest (#5075), Node's `fetch` throws a real `TypeError` (`fetch failed`), so `HttpClient` takes its `Request failed: <message>` branch and all three assertions failed. Assert the stable contract instead -- a generic (non-`HttpError`) `Error` that preserves the cause. All 336 client tests pass. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Re-reviewed head 566dbaf084bda7363ae8c6c9130b20cda6575234 (fetched and checked out detached). Treating this as a fresh review of the current head; the PR is now out of draft.
Scope: clients/typescript/ is owned by this repository (it mirrors the Agent Server API), so the change belongs here. The production change is the same recovery logic as before; the test file was adjusted to stop asserting a runner-specific transport-error string, and the .pr/ artifacts were removed after the main merge.
Implementation assessment: no code defect found.
- Traced
HttpClient.request: lost sockets/aborts surface as plainError(Request failed: .../Request timeout after ..., carryingcause), while non-2xx responses raiseHttpError. So!(error instanceof HttpError)targets exactly transport failures, and a real 400/401/422 still propagates (does not conceal server validation failuresis correct). - The create POST is never replayed: reconciliation GETs
/api/conversations/{id}keyed on the caller'spayload.conversation_id, with a per-request timeout clamped to the remaining deadline, continues only on 404, and rethrows the original transport error otherwise (including on a 401 stop). - Ran the focused suite: 6/6 pass. Full suite at this head: 21 files / 336 tests pass.
npm run build,npm run lint,npm run format:checkall clean. Every other check-run for this head issuccessor intentionallyskipped.
Material finding — the required Validate PR description check fails on this head (run 36459801107, job 109055224690), with two independent causes:
- Empty
HUMAN:section..github/scripts/check_pr_description.pyrequires a human-written note of at least 20 visible characters betweenHUMAN:andAGENT:. AGENTS.md makes this section human-only and forbids AI agents from editing it, so the author must add it in their own words. - Linked-issue readiness. The validator also reports: "Linked issue(s) (#5035, #4966) carry neither
ready-for-devnor a pre-rollout creation date." #5035 was created2026-09-14, after the2026-08-13rollout cutoff, and currently carries onlyenhancement,sdk,javascript(itsready-for-devlabel was removed on 2026-09-14). Because #5035 is linked both throughCloses #5035and the## Issue Numbersection, this repository gate blocks merge until a maintainer confirms the issue is implementation-ready or re-appliesready-for-dev. (#4966, created 2026-09-12, is grandfathered.)
Also note, for the maintainer rather than as a code blocker: the PR body's "Live before/after Canvas recordings" link targets .pr/sdk5036-create-response-recovery/README.md at 7b69c3a776, which is no longer the head, and .pr/ no longer exists on this head (the old commit still resolves, HTTP 200). The current review guide's live-evidence checkpoint asks for before/after evidence tied to the current revision; the production file is byte-identical to what was recorded at e27c6d24, so the recordings likely still apply, but confirming that is a maintainer call.
Deferring the readiness label and the human-authored note to a maintainer.
🔄 CHANGES REQUESTED
|
Closing this in favor of #5362 (server) and #5363 (client). The diagnosis here is correct and the test harness is a good one — driving a real HTTP server to drop a successful POST response is the right way to exercise this, and I kept that approach in the replacement. The difference is the remedy. This PR reconciles a lost response with bounded Notably, the server already advertised exactly the contract this PR works around — #5362 makes the re-check happen inside the lifecycle lock, so a re-sent create is deduplicated: one applies, the other gets the existing conversation back, and the initial message runs once. #5363 then lets the client simply re-send — once, only where the server advertises Why I prefer that: it recovers the value from the authority that owns it instead of reconstructing it by polling, it needs no new The work here is not wasted — the concurrency tests in #5362 cover the window this PR was certifying around, and #5035's acceptance criteria have been updated since they previously mandated never replaying the POST. This comment was written by an AI agent (OpenHands) on behalf of @neubig. |
3 participants
HUMAN:
AGENT:
Why
A conversation can be created and its initial message started even if its POST response is lost. The TypeScript client currently reports failure, leaving Canvas unable to open that conversation.
Summary
Reconcile a transport failure with bounded GET requests using the caller's stable conversation ID. Never replay the create POST. Preserve ordinary HTTP errors and the original transport error if reconciliation fails. This is independent of runtime routing and Docker provisioning.
Issue Number
Closes #5035. Extracted from #4966 to keep the runtime API foundation focused.
How to Test
cd clients/typescript && npm run test:coverage && npm run lint && npm run build && npm run format:check324 tests passed across 20 suites. Six targeted tests use a real HTTP server to drop a successful POST response, delay visibility, and check rejection handling. Against main, four recovery scenarios fail; all six pass with this fix. Live before/after Canvas recordings inject the same lost successful response. Before the fix, Canvas shows disconnected errors; after it, the real agent completes without an error. Both cases send exactly one create POST.
Type
Notes
Targets main and is not a prerequisite for the runtime PR stack. The default additional recovery window is 120 seconds; callers can set
creationRecoveryTimeout.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:566dbaf-pythonRun
All tags pushed for this build
About Multi-Architecture Support
566dbaf-python) is a multi-arch manifest supporting both amd64 and arm64566dbaf-python-amd64) are also available if needed