- 
                Notifications
    You must be signed in to change notification settings 
- Fork 30
          refactor: rename AgentExecutionStatus -> ConversationExecutionStatus & state's agent_status -> executiong_status
          #839
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: openhands <[email protected]>
| Coverage Report •
 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's... accurate that Agents themselves don't 'hold' this status, but I always think of it as a descriptive term, not a term about composition. 🤔
It describes the situation in which the agent is...
On the other hand, I'm looking at the code in the whole PR, and it seems like 'attaching' it to conversation makes sense too.
I guess the question is, which one is more clear to client developers / UIs / users? Do we want them to understand and speak about Conversations rather than about Agents?
Is this rename related to the delegation discussion?
| 
 No. It's just that copilot guessed wrong the path to import the "agent execution status" because it's not anywhere near the agent, and it got me thinking. It is descriptive, but it's deceitful concerning our implementation. | 
| 
 We should also probably rename  Perhaps we should also rename  | 
Co-authored-by: openhands <[email protected]>
Updated test calls in test_conversation_service.py to use the new parameter name 'execution_status' instead of the old 'agent_status' parameter name. Co-authored-by: openhands <[email protected]>
- Added execution_status property to RemoteState class to satisfy ConversationStateProtocol - Made agent_status a backward compatibility property that delegates to execution_status - Added backward compatibility for reading both execution_status and agent_status from server response - Fixed type checking errors in remote conversation implementation Co-authored-by: openhands <[email protected]>
- Removed AgentExecutionStatus alias for ConversationExecutionStatus - Removed agent_status backward compatibility properties from ConversationState and RemoteState - Removed agent_status property from ConversationStateProtocol - Simplified RemoteState execution_status property to only read execution_status field - Cleaned up codebase by removing all backward compatibility layers Co-authored-by: openhands <[email protected]>
AgentExecutionStatus -> ConversationExecutionStatus & state.agent_status -> executiong_status
      AgentExecutionStatus -> ConversationExecutionStatus & state.agent_status -> executiong_statusAgentExecutionStatus -> ConversationExecutionStatus & state's agent_status -> executiong_status
      AgentExecutionStatus -> ConversationExecutionStatus & state's agent_status -> executiong_statusAgentExecutionStatus -> ConversationExecutionStatus & state's agent_status -> executiong_status
      | Aha, I'm not sure though! Tiny fun detail about this: 
 I was just mulling these days about... what's in a name. 😅 It turns out that we believe Agent Skills sound more cool than mundane things like "writing a md file", even though md files is what reality is under the hood. Whether "Conversation status" is better than "Agent status", I don't know. I do agree with you that what LLMs do actually has the right to matter! In fact, I made myself a theory on LLM-driven development... about this time last year, when I first saw o1 unable to fix tests because we were breaking the assumptions of some Pydantic annotation in our code. It was fascinating to see it try over and over, and every time the Pydantic world knowledge took over and made it stumble. 😅 ❤️ In this case, though, Copilot is bad, lol, and I haven't seen GPT-5 get confused, on the contrary it will find things. I'd love to get more opinions on this one! What LLM was Copilot using? | 
| 
 Poor o1 stuck in a logic loop :p 
 It was the free-tier autocompletion model GPT-4o Copilot. Probably not the brightest model out there :p | 
| 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 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for @tofarr's confirmation!
| @tofarr bump | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if maybe we table this until we finish Delegation and its follow-up? Over there, it seems we might want to rethink the Agent doing too much, like executing things. That will likely change what Conversation does, and it might affect how state / status feels like.
I'd personally incline to keep it Agent status. Conversation is an orchestrator.
Not a strong opinion, though, and FWIW I'm usually too conservative on this kind of stuff. Happy to go with what you like.
To note, the important part 😊 !
it's not a reason to make things slightly more tricky than it should be in our repo... I think following the principle of least surprise her makes sense.
I hear you! I wonder what others do.
I do think that LLM Driven Development should be a thing - and I've been nitpicking any PRs for long, to design and name things in an easier way for LLMs. Still, Copilot with 4o is really bad, sorry, I just don't think it's a good indicator yet.
| [Automatic Post]: It has been a while since there was any activity on this PR. @simonrosenberg, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. | 
Summary
This PR renames
AgentExecutionStatustoConversationExecutionStatusto better reflect the actual semantics of the enum.Changes Made
AgentExecutionStatus→ConversationExecutionStatusinopenhands/sdk/conversation/state.pytest_agent_status_enum.py→test_conversation_execution_status_enum.pyAgentExecutionStatus = ConversationExecutionStatusto maintain backward compatibilityRationale
Backward Compatibility
A backward compatibility alias has been added:
This ensures existing code will continue to work without modification.
Testing
Fixes #838
@simonrosenberg can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
golang:1.21-bookwormeclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22Pull (multi-arch manifest)
Run
All tags pushed for this build
The
14a6481tag is a multi-arch manifest (amd64/arm64); your client pulls the right arch automatically.