Skip to content

Conversation

@enyst
Copy link
Collaborator

@enyst enyst commented Jan 23, 2026

HUMAN: This PR proposes a follow-up to SystemPromptEvent immutability issue, previous removal of persisted enforcement, and conversation restore:

  • cross-package tests for the behaviors we want on conversation restore
  • to lock in invariants.

Summary

Adds an integration-like test module that documents the expected behavior of LocalConversation restore.

Covered behaviors

  • Happy path lifecycle: start -> send/run -> send/run -> close -> restore -> send/run
  • Restore fails when the runtime agent toolset changes (removed/added tools)
  • Restore succeeds when non-breaking agent config changes (LLM model, condenser max_size, AgentContext/skills)
  • and others.

Notes

  • Uses patched litellm_completion and create_mock_litellm_response for deterministic, offline runs.
  • Includes a small RestoreLifecycle helper to keep the behavior spec readable.

@enyst 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

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:20bdc38-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-20bdc38-python \
  ghcr.io/openhands/agent-server:20bdc38-python

All tags pushed for this build

ghcr.io/openhands/agent-server:20bdc38-golang-amd64
ghcr.io/openhands/agent-server:20bdc38-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:20bdc38-golang-arm64
ghcr.io/openhands/agent-server:20bdc38-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:20bdc38-java-amd64
ghcr.io/openhands/agent-server:20bdc38-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:20bdc38-java-arm64
ghcr.io/openhands/agent-server:20bdc38-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:20bdc38-python-amd64
ghcr.io/openhands/agent-server:20bdc38-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:20bdc38-python-arm64
ghcr.io/openhands/agent-server:20bdc38-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:20bdc38-golang
ghcr.io/openhands/agent-server:20bdc38-java
ghcr.io/openhands/agent-server:20bdc38-python

About Multi-Architecture Support

  • Each variant tag (e.g., 20bdc38-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 20bdc38-python-amd64) are also available if needed

@enyst enyst marked this pull request as ready for review January 23, 2026 10:59
@OpenHands OpenHands deleted a comment from openhands-ai bot Jan 23, 2026
Copy link
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test coverage for conversation restore behavior! The tests clearly document the expected semantics. Found a few minor issues to address.

@enyst enyst marked this pull request as draft January 24, 2026 06:24
@enyst
Copy link
Collaborator Author

enyst commented Jan 24, 2026

@OpenHands Install yourself gh if you don't have it, then check this PR, understand it deeply, and pull the AI reviewer comments, also inline review threads comments.

Keep in mind it's AI, so it can be wrong and we need to verify each comment. If valid, let's fix them (and mark it solved), otherwise just reply with your opinion to the comment.

@openhands-ai
Copy link

openhands-ai bot commented Jan 24, 2026

I'm on it! enyst can track my progress at all-hands.dev

Copy link
Collaborator Author

enyst commented Jan 24, 2026

Resolved AI review comment r2720748938 by hoisting create_mock_litellm_response import to module scope (commit 7ed20a1). For r2720748971: keeping getattr() is intentional because event type/shape may vary; we only assert the two relevant fields when present.

@openhands-ai
Copy link

openhands-ai bot commented Jan 24, 2026

Final summary (new work since the last summary)

Verification

  • Fetched AI reviewer inline threads via GitHub API/GraphQL and re-validated each comment.
  • Applied valid comments as minimal, focused code changes (no behavior changes beyond test cleanup/clarity).
  • Kept changes concise: only edited tests/cross/test_conversation_restore_behavior.py.
  • Ran the test module locally: pytest tests/cross/test_conversation_restore_behavior.py8 passed.
  • Pushed updates to the existing PR branch test/conversation-restore-behavior.
  • Marked review threads as resolved (including the remaining one after replying).

Code changes made (since last summary)

All included in commit d76eba8f:

  • Removed dead code in multiple pytest.raises(...) blocks (restored.close() after an exception) by calling lifecycle.restore(...) directly.
  • Added a short comment explicitly documenting tool-order independence in the happy-path test (persisted vs runtime tool list order).
  • Simplified a minor assertion: replaced pytest.approx(0.42) with == 0.42.

Notes on PR interactions

Replied on the remaining “getattr vs explicit event type” thread to explain why keeping getattr is intentional (resilience to event model refactors while still asserting the relevant fields), then resolved it.

No further diffs remain on the branch (working tree clean, branch already pushed).

View full conversation

@enyst enyst marked this pull request as ready for review January 24, 2026 07:48
Copy link
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a well-structured test suite that clearly documents conversation restore behavior. Found a few issues that should be addressed.

@raymyers
Copy link
Contributor

Neat approach!

@enyst enyst merged commit edfea67 into main Jan 27, 2026
25 checks passed
@enyst enyst deleted the test/conversation-restore-behavior branch January 27, 2026 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants