feat: add /clear endpoint for V1 conversations#12786
Conversation
|
@enyst Just opened the PR for v1 |
all-hands-bot
left a comment
There was a problem hiding this comment.
This PR adds a useful clear endpoint, but there are some important security and error handling concerns that should be addressed before merging.
Fixed all! |
all-hands-bot
left a comment
There was a problem hiding this comment.
Overall solid implementation of the /clear command. Found several security and design concerns that should be addressed, particularly around CORS configuration and the contradictory clear_events implementation.
|
@enyst hope you had a good weekend! could you please review the changes and let me know your feedback? |
enyst
left a comment
There was a problem hiding this comment.
Thank you for the work on this, @MkDev11 ! I started a discussion on it on slack.
There was a problem hiding this comment.
Hello @MkDev11,
Thank you for creating this pull request — much appreciated. 🙏
I’ve left a few comments on the PR for your reference. Overall, I believe we should avoid creating a new conversation and instead clear the events of the current conversation.
@tofarr, I would greatly appreciate your feedback and guidance on this approach.
Additionally, could you please add some front-end tests to help cover the changes introduced here?
Thank you very much, and I appreciate your work on this! 🙏
The current approach (creating a new conversation with |
|
@hieptl could you please have a look at the changes again? |
|
@tofarr thanks for your feedback. could you please review again? |
tofarr
left a comment
There was a problem hiding this comment.
This is some great work - thank you for taking this on. I've left a few comments, and will review again if you have any questions or changes.
|
@tofarr fixed all the comments. |
094642a to
84f2393
Compare
|
Hello @MkDev11, Could you please add tests to cover your front-end changes when you have a chance? In addition, I ran the pull request locally and noticed a few issues. Issue 1: issue1.movIssue 2: issue2.movIssue 3:
Please refer to the video below for more information. issue3.movIssue 4: issue5.movThank you very much! 🙏 |
|
@hieptl fixed the issues you mentioned. please review again |
|
Hello @MkDev11, I noticed the issue below while running the code locally. Please feel free to refer to the video below for additional details and context. issue.movThank you very much! 🙏 |
d0c9c53 to
b5f9831
Compare
|
I think maybe it makes a lot of sense, and it would be expected, that the "application control commands" executed on slash /command would be in the menu. By control commands, I mean those that are not skills. This one is not a skill, it's pure python implementation of a control command. (Of course, we include skills too, those look like slash commands and will be interpreted by the agent as commands for itself; I'm just saying this one is not an agent thing, it's an application thing) Though I'd add a thought, they make sense to me at the start of the line... like a CLI command. Could be in a future PR, I suppose? I'm not sure if other control commands are in the slash menu currently.
You're right, that's clearly the case... We could do as you suggest:
Yes. I believe we should, it would be inline with what it does, and easier to understand for users coming from other agentic applications. |
|
Hello @MkDev11, As Engel mentioned:
Additionally, based on the two issues I noted in my previous comment, both will need to be addressed. Thank you very much! 🙏 |
…-allowed, fix cross-conversation change sync - Rename /clear command to /new in chat-interface.tsx - Update i18n translations to reflect /new command wording - Update toast success message to clarify shared runtime - Add cursor-not-allowed and opacity-50 to disabled chat input - Add refetchOnMount: 'always' to useUnifiedGetGitChanges so navigating between shared-sandbox conversations refetches git changes
|
Hello @MkDev11, While testing your code locally, I came across a few issues that might be worth reviewing. Issue 1:
For additional context, please refer to the video below. issue1.movIssue 2: In one of my earlier comments, I suggested disabling the chat input only during the brief moment when a new conversation is being created after the user enters For additional context, please refer to the video below. issue2.movThank you very much! 🙏 |
…rsations When deleting a conversation that shares a sandbox with other conversations (created via /new), skip the DELETE /api/conversations call to the agent server. This prevents destabilizing the shared runtime, which would cause remaining conversations to appear as ARCHIVED. - Add skip_agent_server_delete param to delete_app_conversation - Check count_conversations_by_sandbox_id before deletion - Pass skip flag when sandbox is shared (count > 1) - Re-check remaining count after deletion before sandbox cleanup
Thread a separate inputLocked prop through the component chain so that ChatInputField is only locked (contentEditable=false, cursor-not-allowed) during /new command processing, not during AgentState.LOADING. - Add inputLocked prop to CustomChatInput, ChatInputContainer, ChatInputRow - ChatInputRow passes inputLocked (not disabled) to ChatInputField - disabled still controls send button, file button, and Enter submission - ChatInputField only gets disabled=true during /new (isClearingConversation) This fixes the regression where typing was blocked during conversation startup, while preserving cursor-not-allowed feedback during /new.
…mmand # Conflicts: # frontend/src/components/features/chat/interactive-chat-box.tsx # frontend/src/i18n/declaration.ts # frontend/src/i18n/translation.json # openhands/server/routes/manage_conversations.py
|
@hieptl I fixed the issues you mentioend. can you review the change? |
…by_sandbox_id and skip_agent_server_delete
…ther skipped tests)
|
@hieptl do you have any feedbacK? |
|
@hieptl just fixed the lint error |
hieptl
left a comment
There was a problem hiding this comment.
Hello @MkDev11,
For the front-end changes, it looks like the current updates focus on adding tests for use-clear-conversation. It would be helpful to also include tests that cover the changes made in the component files. There’s no need to add tests specifically for styling.
Thank you! 🙏
- Fix flaky WebSocket onClose test (reset spy after connection instead of skipping) - Rename inputLocked → isNewConversationPending for clarity - Use cn() instead of template string in chat-input-field.tsx - Rename use-clear-conversation → use-new-conversation-command (file, hook, tests) - Move get_global_config import to top of middleware.py - Add component tests for disabled input field behavior
|
all of your comments are addressed, could you please review again? |
The changes look good to me. Thank you very much!
|
thanks for your help! |
Summary
This PR refactors the
/clearcommand for V1 conversations to use Option 4: New conversation ID, same runtime. Instead of deleting event files (which violated the append-only invariant),/clearnow creates a brand new conversation that inherits the parent's sandbox and configuration. The old conversation is preserved and linked viaparent_conversation_id.What changed
Backend (
app_conversation_router.py):clear_conversationendpoint to callstart_app_conversationwith the current conversation as the parentFrontend (
chat-interface.tsx,v1-conversation-service.api.ts):clearConversationstatic method toV1ConversationService/clearin a V1 conversation, the frontend calls the API, shows a success toast, and navigates to the new conversation using React RouterInfrastructure (deployment-related, not core feature):
docker_sandbox_service.py: CORS origins are now passed as indexed environment variables (OH_ALLOW_CORS_ORIGINS_0,_1, etc.) to agent server containers, supporting multiple allowed originswebsocket-url.ts: When accessing the frontend from an external IP, WebSocket URLs that point tolocalhostare rewritten to use the browser's hostnamemiddleware.py: Falls back to allowing all origins when no specific CORS origins are configuredTests:
app_conversation_id)How it works
/clearin a V1 conversationPOST /api/v1/app-conversations/{id}/clearparent_conversation_idset to the old one — same sandbox, same agent config, same LLM model/conversations/{new_id}— fresh chat history, runtime state preservedDemo Screenshots/Videos
Loom recording showing /clear working end-to-end
Change Type
Checklist
Fixes
Resolves #12564
Release Notes
Added
/clearcommand for V1 conversations. Typing/clearin the chat creates a new conversation that inherits the current sandbox and configuration, giving you a fresh chat history while keeping your runtime state (files, packages, environment) intact.