Use in-memory event history for condenser replay#252
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
There was a problem hiding this comment.
🟡 Acceptable — the in-memory replay approach is sound, but two things need attention before this merges.
This review was generated by an AI agent (OpenHands) on behalf of the repository maintainers.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/neulab/agent-data-protocol/actions/runs/26795767723
| conversation = Conversation(agent=agent, workspace=tmpdir, visualizer=None) | ||
| try: | ||
| conversation.send_message(first_event.content) | ||
| conversation._ensure_agent_ready() |
There was a problem hiding this comment.
🟠 Important: _ensure_agent_ready() is a private (underscore-prefixed) SDK method. Coupling to internal implementation details is fragile — the SDK can rename or remove it without a major version bump and this will break silently. If there is no public API for "initialize without sending a message", that gap should be raised with the SDK team. At minimum, add a comment explaining why this private method is called here and what it guards against.
| segment_index += 1 | ||
| records.append(prompt_record) | ||
| event_history.append(condensation) | ||
| conversation.state.events.append(condensation) |
There was a problem hiding this comment.
🟡 Suggestion: conversation.state.events is no longer read anywhere in this function after the refactor — event_history is the canonical source for all condensation logic. Yet TrackingSDKEventBuilder.append still calls super().append() (line 115), which writes every event to conversation.state.events; and here the condensation is also written there explicitly to keep the two lists in sync.
This dual-write pattern obscures what the true source of truth is. If conversation.state.events is being kept in sync intentionally (e.g. as a guard against unknown SDK side effects that read it internally), add a brief comment saying so. If it is not needed, remove the super().append() call in TrackingSDKEventBuilder and this explicit append, which would make the migration complete and the code self-documenting.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
No description provided.