Remote LLM switching via agent-server /llm #625
Conversation
- RemoteConversation posts /api/conversations/{id}/llm on profile changes\n- Add remote agent-server E2E coverage for switching + restore\n- Update docs to reflect new server API
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @enyst, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the VS Code client's ability to manage Large Language Model (LLM) configurations for remote conversations. It introduces a mechanism where changes to the user's selected LLM profile are communicated to the remote agent-server, ensuring that the active conversation utilizes the desired LLM settings. This improves flexibility and control over the LLM used during remote interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for runtime LLM switching for remote conversations, which is a great enhancement. The implementation is robust, with a well-designed queuing mechanism to handle rapid setting changes and ensure state is synchronized before agent actions. The addition of comprehensive unit and end-to-end tests is excellent and covers the new functionality and its edge cases thoroughly. The documentation has also been updated to reflect these new capabilities.
I've found one high-severity issue in the error handling of the LLM update logic that could lead to an infinite loop on network failure. I've provided suggestions to fix this. Besides that, the changes are of high quality.
| } | ||
| this.lastAppliedLlmSignature = signature; | ||
| }).catch((err) => { | ||
| this.emit('error', err instanceof Error ? err : new Error(String(err))); |
There was a problem hiding this comment.
The current implementation of the .catch block can lead to an infinite loop of failing requests. When fetchWithTimeout fails, the error is emitted, but the promise chain resolves. This causes await this.llmUpdateInFlight to complete successfully. The subsequent check this.desiredLlmSignature !== this.lastAppliedLlmSignature will still be true (since lastAppliedLlmSignature wasn't updated), triggering a recursive call to flushRemoteLlmUpdate, which will fail again, and so on.
To fix this, the promise chain should propagate the failure. You can achieve this by re-throwing the error in the .catch block. This will cause await this.llmUpdateInFlight to throw, breaking the loop.
| this.emit('error', err instanceof Error ? err : new Error(String(err))); | |
| this.emit('error', err instanceof Error ? err : new Error(String(err))); | |
| throw err; |
| if (!signature) return; | ||
| if (signature === this.lastAppliedLlmSignature) return; | ||
| if (this.llmUpdateInFlight) return; | ||
| void this.flushRemoteLlmUpdate(); |
There was a problem hiding this comment.
Following the recommended change to propagate errors from flushRemoteLlmUpdate, this fire-and-forget call will result in an unhandled promise rejection if the flush fails. To prevent this, you should add a .catch() to the promise chain to swallow the already-emitted error.
| void this.flushRemoteLlmUpdate(); | |
| this.flushRemoteLlmUpdate().catch(() => { /* error is emitted by flush, swallow to prevent unhandled rejection */ }); |
🔧 VSCode Extension Built Successfully• File: openhands-tab-0.0.4.vsix (2262 KB) To install:
Built with Node 22. Commit 81fac5b. |
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Updates the VS Code client (agent-sdk-ts
RemoteConversation) to support runtime LLM switching against the Python agent-server.POST /api/conversations/{id}/llmwith an inlinellmpayload (TS profiles are local-only).@openhands/agent-sdk-tsbefore compiling the extension to avoid stale dist artifacts.Pairs with OpenHands/software-agent-sdk#1544.