Repository navigation
feat(client): add conversation navigate endpoint - #254
Conversation
Support POST /api/conversations/{id}/navigate, which moves a conversation's HEAD to an existing event, re-rooting the active branch in place (no new conversation, unlike fork). Mirrors the Python SDK's RemoteConversation.navigate_to.
- ConversationClient.navigateConversation(id, request, { includeSkills }) returns the updated ConversationInfo carrying the new leaf_event_id
- RemoteConversation.navigateTo(eventId) posts then refreshes cached state (leaf_event_id is not broadcast over the WebSocket)
- Add NavigateConversationRequest and an explicit leaf_event_id on ConversationInfo
- Unit tests plus a deterministic integration test covering re-root semantics and the 404 contract guards
Endpoint audit❌ 6 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 6)⛔ (no known backend) — served by no backend we can see (5)
|
|
@OpenHands I don’t see any footguns here, do you? /codereview |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable
I found one small public-API footgun, not a logic problem in the endpoint implementation:
[IMPROVEMENT OPPORTUNITIES]
- [
src/index.ts] Public type export:NavigateConversationRequestis added insrc/models/conversation.tsbut is not exported from the package root alongsideForkConversationRequest. Becausepackage.jsononly exposes.and./clients(not./models/conversation), consumers usingConversationClient.navigateConversation(...)cannot name/import the new request type from the published package. AddNavigateConversationRequestto the conversation model export block insrc/index.ts. The rest of the route wiring looks consistent withforkConversation: path/body shape,include_skills,nullfor the empty tree, and theRemoteConversation.navigateTo()refresh are all covered by unit and deterministic integration tests.
I couldn’t run the tests locally because this checkout has no node_modules, but the PR checks are green, including unit, build, endpoint audit, and integration-test.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Small additive client API for a pinned server endpoint. No dependency changes, no behavior changes to existing methods, and regression coverage exercises both mocked client calls and the real deterministic server contract. Main risk is packaging/API discoverability of the new request type.
VERDICT:
✅ Worth merging once the type export is added.
KEY INSIGHT:
The implementation itself is simple and sound; the only footgun is making sure the new public request type is actually public in the packaged SDK.
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 created by an AI agent (OpenHands) on behalf of the user.
|
Posted a top-level PR review comment here: Summary:
|
Consumers of ConversationClient.navigateConversation() can now import and name the request type from the published package, matching ForkConversationRequest.
|
@enyst corrected ;-) |
|
🚀 Released in v1.32.0. |
Summary
Adds client support for
POST /api/conversations/{id}/navigate(agent-server / software-agent-sdk #3923, released in v1.31.0). Navigate moves a conversation's HEAD to an existing event, re-rooting the active branch the agent runs on next — all branches stay on disk and, unlikefork, no new conversation is created. Mirrors the Python SDK'sRemoteConversation.navigate_to.Changes
ConversationClient.navigateConversation(id, request?, { includeSkills? })—POST .../navigate, returns the updatedConversationInfocarrying the newleaf_event_id. Follows theforkConversationtemplate.RemoteConversation.navigateTo(eventId)— ergonomic wrapper that posts and then refreshes cached state (leaf_event_idis not broadcast over the WebSocket), matching the Python SDK.NavigateConversationRequest { event_id?: string | null }and an explicitleaf_event_id?: string | nullonConversationInfo.event_id: nullselects the empty tree (a deliberate new root).Tests
api-clients.test.ts): client-level navigate withinclude_skills, navigate-to-null, and thenavigateTowrapper (POST + state refresh).deterministic-api.integration.test.ts): re-roots HEAD across two real events and back to the empty tree, plus 404 contract guards for an unknown conversation and an unknownevent_id.Verification
tsc --noEmit,eslint,prettier --check: cleanghcr.io/openhands/agent-server:1.31.0-python(the pinned CI image) — deterministic suite green, including the two navigate tests.