Fix:incorrectly removes action event by Gai-shi · Pull Request #4616 · OpenHands/software-agent-sdk

Gai-shi · 2026-08-24T14:22:26Z

HUMAN:
I verified the persisted confirmation resume flow locally and this PR is ready for review.

AGENT:
This PR fixes and verifies the persisted confirmation approval flow end-to-end in the local SDK path. I reproduced the relevant lifecycle with a real Conversation: send a user message, pause on AlwaysConfirm, close the conversation, reopen it from the same conversation_id and persistence_dir, approve by calling run(), then inspect the second LLM request shape.

Why

When a conversation pauses for confirmation, then gets closed and resumed from persistence before approval, the follow-up LLM request could include a tool result without the preceding assistant tool_calls message. Providers reject that malformed history, even though the persisted event log itself contains the matching ActionEvent and ObservationEvent.

This breaks the normal web app flow where approval happens in a later request/process.

Summary

Preserve pending confirmation action events in the rebuilt conversation view while the conversation is waiting for confirmation.
Thread allowed unmatched action IDs through view property enforcement so temporary pending actions are not permanently dropped during resume.
Add a regression test for the close-and-resume confirmation approval flow, asserting the second provider request includes assistant(tool_calls) before the matching tool result.

Issue Number

Fixes #4532

How to Test

I ran the focused regression test:

uv run pytest tests/sdk/conversation/local/test_confirmation_mode.py -k resume_confirmation_preserves_tool_call_message

Result:

1 passed, 10 deselected

I also ran pre-commit on the changed test file:

uv run pre-commit run --files tests/sdk/conversation/local/test_confirmation_mode.py

Result:

Ruff format..............................................................Passed
Ruff lint................................................................Passed
PEP8 style check (pycodestyle)...........................................Passed
Type check with pyright..................................................Passed
Check import dependency rules............................................Passed
Check Tool subclass registration.........................................Passed

The new regression test exercises the original issue shape without provider credentials by capturing the actual messages passed to the LiteLLM boundary. The second LLM request now contains:

system, user, assistant(tool_calls=1), tool

instead of the broken persisted-resume shape:

system, user, tool

Video/Screenshots

N/A. This is a backend SDK persistence/context-view bug with no UI surface.

Design Doc

N/A. The fix is narrowly scoped to conversation view rebuilding for confirmation-paused state.

Type

Notes

No rollout concerns.

all-hands-bot · 2026-08-24T14:57:47Z

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

all-hands-bot · 2026-08-25T19:19:17Z

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

all-hands-bot

🟡 Acceptable — Correct, targeted bug fix for a real production issue. The core logic is sound.

What the fix does: On cold resume from persistence in WAITING_FOR_CONFIRMATION state, rebuild_view() triggers View.from_events() → enforce_properties() → ToolCallMatchingProperty.enforce(), which was stripping the pending ActionEvent because no matching ObservationEvent existed yet. The result was a malformed [system, user, tool] history (missing the assistant(tool_calls) entry) that providers reject. The fix introduces allowed_unmatched_action_ids to thread the set of legitimately-unmatched action IDs through the property enforcement pipeline so they are preserved.

The regression test is well-structured: it exercises a real code path end-to-end, uses mocking only at the correct I/O boundary (the LiteLLM call), and asserts on the actual message sequence shape rather than internal call counts. PR description evidence is solid.

One minor design note raised inline — not blocking.

[RISK ASSESSMENT]

[Overall PR] ⚠️ Risk Assessment: 🟢 LOW
Narrowly scoped bug fix in the persistence resume path. Fully backward-compatible API change (keyword-only parameter, None default). Covered by a purpose-built regression test. No dependency changes. No public API surface changes.

VERDICT:
✅ Worth merging: The fix is correct, the test proves it, and the risk is low.

KEY INSIGHT:
ToolCallMatchingProperty.enforce() correctly cleans up orphaned events in general, but must be told which action IDs are legitimately orphaned (awaiting confirmation) to avoid corrupting the resumed conversation history.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.

Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.

When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

all-hands-bot · 2026-08-25T19:22:59Z

+        allowed_unmatched_action_ids = allowed_unmatched_action_ids or set()
        for property in ALL_PROPERTIES:
            events_to_forget = property.enforce(self.events, all_events)
+            if isinstance(property, ToolCallMatchingProperty):


🟡 Suggestion: The isinstance(property, ToolCallMatchingProperty) check breaks the abstraction of the polymorphic property loop — all other properties are treated uniformly, but this one concrete type is singled out after-the-fact.

A cleaner design would pass allowed_unmatched_action_ids into enforce() itself and let ToolCallMatchingProperty handle it internally (other properties would simply ignore the extra argument). That said, changing ViewPropertyBase.enforce()'s signature is a larger refactor that goes beyond a focused bug fix. Acceptable as-is; worth tracking as a follow-up cleanup.

all-hands-bot · 2026-08-25T19:38:29Z

🔍 Review in progress…