Skip to content

fix(sdk): reject unknown event parents on append#4089

Open
hxaxd wants to merge 1 commit into
OpenHands:mainfrom
hxaxd:hxaxd/validate-event-parent
Open

fix(sdk): reject unknown event parents on append#4089
hxaxd wants to merge 1 commit into
OpenHands:mainfrom
hxaxd:hxaxd/validate-event-parent

Conversation

@hxaxd

@hxaxd hxaxd commented Jul 11, 2026

Copy link
Copy Markdown

HUMAN:

This PR was directed by me and implemented with AI assistance. I control the overall architecture and functional logic, have fully reviewed all changes and relevant test results, and take full responsibility for all modifications in this PR.


AGENT:

Why

A cyclic event graph prevents a conversation from loading:

ConversationState.create()rebuild_view()path_to_root()ValueError

Normally, _stamp_parent_id() assigns the current leaf as the parent, so the referenced event already exists. Forks also copy events in parent-first order.

The invalid case can occur when patched code or a custom agent emits an event with an explicit parent_id. _stamp_parent_id() preserves non-null values, and EventLog.append() previously wrote them without checking whether the parent existed. This allowed a forward reference such as A → B, followed by B → A, creating a cycle that was only detected during the next load.

Summary

  • Validate explicit parents in EventLog.append() after synchronizing the on-disk index.
  • Allow None, ROOT_PARENT_ID, or an event ID already present in the log.
  • Reject unknown, forward, and self-referencing parents before writing.

Because the log is append-only, requiring an ordinary parent to already exist prevents new cycles without changing legacy events, explicit roots, forks, or navigation.

Issue Number

Fixes OpenHands/OpenHands#15247

How to Test

Ran the event-store and event-tree unit tests:

uv run --frozen pytest \
  tests/sdk/conversation/test_event_store.py \
  tests/sdk/conversation/test_event_tree.py -q

Result:

47 passed

Also tested the real EventLog write path without mocks:

  • A valid root and child were written successfully.
  • An event referencing a future parent was rejected before writing.
  • The log length remained unchanged after rejection.

Pre-commit checks passed, including Ruff, pycodestyle, Pyright, and import dependency checks.

Video/Screenshots

N/A — SDK event-log validation with no UI changes.

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

@hxaxd hxaxd marked this pull request as ready for review July 11, 2026 17:26
Copilot AI review requested due to automatic review settings July 11, 2026 17:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the SDK’s persistent EventLog against malformed/cyclic event graphs by validating explicit parent_id references at append-time, preventing forward/self references that could otherwise surface later as load-time failures.

Changes:

  • Add EventLog.append() validation to reject explicit parent_id values that are neither None, ROOT_PARENT_ID, nor an already-present event ID.
  • Update/extend event-tree tests to (a) simulate an already-corrupted persisted cycle and (b) assert append-time rejection/acceptance of explicit parent IDs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/sdk/conversation/test_event_tree.py Adds coverage for append-time parent validation and adjusts the cycle test to simulate corruption that bypasses append-time checks.
openhands-sdk/openhands/sdk/conversation/event_store.py Enforces that an explicit non-root parent_id must already exist in the log before writing the event, preventing forward/self references.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

[Bug]: Conversation fails to load when event graph contains a cycle

2 participants