https://docs.all-hands.dev/ logo
Join Slack
Powered by
Fix for the code design fault with the two sources...
# proj-agent
c
Fix for the code design fault with the two sources of truth for the persisted Agent: fix(agent-server): base_state.json as single source of truth for the agent (end meta.json duplication) cc @modern-fireman-99755 as you may recall, we save duplicate Agent settings for the persisted conversation. This has caused multiple bugs, we have looked into small tweaks but they only make the mess worse. Over the weekend, we had another report of totally unexpected behavior when the LLM choice seemed to “revert” to an old profile - again, it was saved in one file and restored from the other, that kind of thing. Please see the links in the PR description, if you wish. I have installed and tested manually the more tricky aspects, with codex via ACP. I think we need to just solve this from the root cause, or it will continue to have bad consequences. cc @limited-student-10747
m
Interesting... I remember, i think this is something that we should do at some point. However, I'm a little bit scared. This can be delicate. I'm going to review this but will probably take some time. A couple of questions: I saw that you have multiple PRs. What is the plan for the migration? Should we wait that every PRs is merged to be sure not to breaking something? Is it backward compatible?
c
I shouldn’t have multiple PRs (smolpaws just did stuff, they’re irelevant). I think it’s safe enough already. The alternative I took doesn’t, actually, change interfaces at all, it just saves in the other spot - so canvas and other clients should not be affected
I can make - oh wait
smolpaws
made an architecture study into the 3 alternatives. I think maybe it’s easy to skim to the point? Please let me know if not; I need to improve the HTML skill :) Three alternatives
TL;DR: The agent-server had the 
StoredConversation
model class, which is a server thing, inherit from 
StartConversationRequest
class, which is a bit of a code design smell. The request/response objects are typically leaves. That's what made it have suddenly the entire Agent, and some conversation fields. And stored them in 
meta.json
, duplicating those of the Conversation/Agent in 
base_state.json
. Killing that relationship (since 
StoredConversation
is not public API, it’s just a server-internal class) actually simplifies things and gives us the ability to read/write only from 
base_state.json
; a server conversation behaves now like a normal SDK conversation. Its 
meta.json
needs only the server-specific fields, e.g. those that help list conversations in a side panel etc.
Bumpy. Please see a new user report that this behavior is unfriendly and unexpected: https://openhands-ai.slack.com/archives/C06P5NCGSFP/p1786717462279369 I would love to merge this PR, and take it from there. I call this a code design fault, and it was there in V0 too, but when we ported to V1 it became worse because now we’re saving the entire Agent twice. I do believe we need to fix it, and work in the future on a better, cleaner design.
l
Yeah, I want to fix this, I’ve encountered it as well
m
giving a second review pass 🙂
c
Thank you! @modern-fireman-99755 I fixed two things your agent reported, and I think the other three are theoretical or apparent, not issues in practice.
m
Yes, I was reading the comments before posting but it was so much complicate that I preferred to post false positive 🙂 LGTM. Perhaps @limited-student-10747 want also to give a look.
gratitude thank you 1
c
Graham, do you have concerns with this PR? I’d love to get it merged so we can move forward.
l
I don’t have any concerns because I haven’t looked at it yet 🙂
But go ahead
3 Views
Open in Slack
PreviousNext
Powered by