Repository navigation
feat: add rerun_actions method to replay conversation actions - #2351
Conversation
Add ability to re-execute all actions from a conversation's event history. This is useful for reproducing environment state from a saved conversation. Changes: - Add rerun_actions() method to LocalConversation that iterates through ActionEvents and re-executes them using their original action parameters - Add rerun parameter to LocalConversation.__init__ to trigger rerun during initialization - Add rerun parameter to Conversation factory class - Add validation that rerun is not supported for RemoteConversation The original observations in the event log are preserved; new observations from the rerun are returned but not persisted. WARNING: Many tool operations are not idempotent (file creation, API calls, etc). This feature is intended for advanced use cases where the user understands results may differ from the original run. Co-authored-by: openhands <openhands@all-hands.dev>
API breakage checks (Griffe)Result: Passed |
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
Coverage Report •
|
||||||||||||||||||||
|
@OpenHands I think we only want to expose the |
|
I'm on it! csmith49 can track my progress at all-hands.dev |
Remove the rerun parameter from Conversation factory and LocalConversation constructors. Users should call rerun_actions() method directly instead. Changes: - Remove rerun parameter from Conversation.__new__ overloads - Remove rerun parameter from LocalConversation.__init__ - Remove automatic rerun execution in LocalConversation constructor - Remove validation for rerun with RemoteConversation - Remove related tests for init flag functionality Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've addressed @csmith49's review comment by removing the Changes MadeChecklist:
Net change: 3 files modified, 1 insertion, 55 deletions New usage pattern: conversation = Conversation(agent=agent, workspace=".")
observations = conversation.rerun_actions() # Call method directly |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🔴 Needs improvement - The core design needs reconsideration. The stated goal is "reproducing environment state" but observations aren't persisted to the conversation, defeating this purpose. Additionally, the rerun parameter mentioned in the PR description isn't in the diff, suggesting incomplete implementation.
…run_actions Address PR review comments on testing: 1. Test isolation: Replace manual registry snapshot/restore with monkeypatch fixture for cleaner per-test isolation that automatically cleans up 2. Real file operations: Add new test tools (FileWriteTool, FileCreateTool, FailingTool) that perform actual file I/O operations 3. State verification: test_rerun_reproduces_file_state verifies that files created during rerun are actually persisted to disk, then verifies recreation after deletion 4. Non-idempotency: test_rerun_non_idempotent_failure demonstrates the documented warning about non-idempotent operations (file creation fails if file already exists) 5. Partial failure: test_rerun_partial_failure verifies rerun continues processing after failures and returns only successful observations 6. Multiple files: test_rerun_multiple_files verifies sequential file operations including subdirectory creation Co-authored-by: openhands <openhands@all-hands.dev>
…tional log Simplify the rerun_actions API based on PR feedback: 1. Return boolean: True if all actions succeed, False if any raises exception - Tool returning error observation (is_error=True) still counts as success - Only exceptions cause failure return 2. Add rerun_log_path parameter: Optional path to save JSONL event log - Each entry contains action_index, tool_name, action, observation - Partial log saved on failure for debugging 3. Early exit on failure: Stop immediately when a tool raises an exception - Prevents cascading failures from corrupted state - Partial log available for diagnosing what succeeded 4. Keep KeyError for missing tools: Configuration error (can't proceed) - Different from execution failure (returns False) 5. No workspace reset: User responsibility to ensure clean state - Too many execution modes to handle automatically Updated tests to verify: - Boolean return values - Rerun log contents via JSONL parsing - Early exit behavior (subsequent actions not executed) - Partial log saved on failure Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Core logic is sound, tests are excellent. Minor improvements suggested below.
Address PR review feedback: 1. Move imports to top of file (no inline imports): - Add EventLog, LocalFileStore, ObservationEvent imports at module level 2. Use EventLog for memory-efficient logging: - Replace in-memory list with EventLog that writes incrementally to disk - Avoids memory buildup for conversations with thousands of actions - Events are written immediately, even if process crashes 3. Store actual Event objects: - Append original ActionEvent from conversation - Create and append ObservationEvent with new observation - Log structure: events/ directory with individual event JSON files 4. Update tests to use EventLog for verification: - Read back events using EventLog API - Verify ObservationEvent.observation properties directly Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean implementation with excellent test coverage. All previous review concerns addressed (memory efficiency via EventLog, early exit error handling, real I/O tests). Ready to merge.
|
Interesting, thank you for this. This sounds like the old @OpenHands Do a /codereview-roasted on this PR. Fully understand the goal and investigate implementation and code design. Publish your review using gh api (not a comment, you are allowed to review) Then take a step back, and look at your review section by section with fresh eyes for a reassessment: is it as minor as it seems, is it as major as it seemed, do suggestions respect the design patterns in the codebase or does the PR code design does; respond to the previous review with your reassessment. |
|
I'm on it! enyst can track my progress at all-hands.dev |
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable
[CRITICAL ISSUES]
- local_conversation.py lines 1005-1010 Correctness: Actions tied to tools without executors are silently skipped, yet rerun_actions still returns True. That violates the contract of “re-execute all actions” and can mask incomplete reruns. Treat this as a configuration error like execute_tool (raise NotImplementedError or return False) or explicitly fail when an action is skipped.
[IMPROVEMENT OPPORTUNITIES]
- local_conversation.py lines 976-1029 Logging robustness: Reusing rerun_log_path can fail because EventLog.append rejects duplicate event IDs, and the failure is reported as a tool-execution failure. Enforce an empty log directory or create a per-run subdir/new IDs to prevent false negatives.
[TESTING GAPS]
- None.
VERDICT:
❌ Needs rework: fix the silent skip so “success” means all actions actually ran.
KEY INSIGHT:
If rerun claims success while skipping actions, it’s lying to the caller.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Yes — it’s conceptually similar to the old “replay” idea from OpenHands#6049, but narrower: this PR re-executes only ✅ Review published via GitHub API (COMMENT): Reassessment (fresh eyes)Critical issue (skip tools without executor)Still stands as significant. Returning Logging robustness (rerun_log_path reuse)This is more of a minor usability pitfall. It won’t affect most runs if users use a fresh log dir, but the current behavior can surface as a confusing “tool failure” when it’s really a duplicate-ID write issue. Worth addressing, but not a blocker if documented/guarded. Design alignmentOverall design is consistent with existing patterns (EventLog reuse, explicit agent init, no hidden state mutations). The main mismatch is the silent skip behavior, which conflicts with the codebase’s preference for explicit failures over silent no-ops. If you want, I can help draft a follow-up patch to address the executor skip (and optionally the log reuse behavior). |
…nds#2351) Co-authored-by: openhands <openhands@all-hands.dev>
4 participants
Summary
Add ability to re-execute all actions from a conversation's event history. This is useful for reproducing environment state from a saved conversation without re-running the LLM.
Changes
rerun_actions()method toLocalConversationthat iterates throughActionEvents and re-executes themboolindicating success/failure (early exit on first failure)rerun_log_pathparameter to save events usingEventLog(memory-efficient incremental writes)Usage
API
EventLog Format
When
rerun_log_pathis provided, events are stored usingEventLog:Benefits:
Implementation Details
ActionEvents with non-null actionstools_mapand calls executoris_error=Truemeans tool executed correctlyMany tool operations are NOT idempotent:
The workspace should typically be reset before rerunning. This is the user's responsibility.
Testing
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:3fa3ce6-pythonRun
All tags pushed for this build
About Multi-Architecture Support
3fa3ce6-python) is a multi-arch manifest supporting both amd64 and arm643fa3ce6-python-amd64) are also available if needed