Repository navigation
fix(agent): load history entries that predate interacted_element - #5845
Open
Lesereingrape wants to merge 9 commits into
Open
Lesereingrape wants to merge 9 commits into
Lesereingrape wants to merge 9 commits into
Conversation
BrowserStateHistory.interacted_element is a required list, so filling it with None in the legacy-compat branches made every entry that lacks the key fail validation instead of loading. Fill an empty list, which is what the downstream `or [None] * len(...)` fallbacks already treat as "no captured elements".
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Replay reads state.interacted_element[i] for each action in _execute_step_from_history, so an empty list loads but raises IndexError on the first action. Fill one None per action instead, which is also what service.py:1751 and service.py:3318 already write for "no element info".
…interacted-element
…interacted-element
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
…interacted-element
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
BrowserStateHistory.interacted_elementis a requiredlist[DOMInteractedElement | None] | list[None](browser_use/browser/views.py:122), but the two branches that exist to tolerate history entries written before that field was introduced fill the missing key withNone:browser_use/agent/views.py:683inAgentHistoryList.load_from_dictbrowser_use/beta/service.py:4062in_load_rust_historyNoneis the one value that field cannot hold, so the compat branch converts "legacy entry" intoValidationError: 2 validation errors for AgentHistoryList— it fails for exactly the input it was written for.Both now insert a list.
load_from_dictinserts oneNoneper action in the entry'smodel_output;_load_rust_historyinserts[], because that loader setsitem['model_output'] = Noneunconditionally (beta/service.py:4059), so an entry arriving through it has no actions to align with.Fixes #5844
Why this shape
Why a list at all.
Nonecannot be made valid instead: widening the field type would let a genuinely missing element be confused with an un-padded action list everywhere the length is meaningful —service.py:3430and:3481both index into it positionally.Why one slot per action rather than
[]. Raised by @cubic-dev-ai's review on this PR, and correct: the replay worker enumerates the actions and indexes element state by that same position —service.py:3453for i, action in enumerate(history_item.model_output.action):service.py:3481historical_elem = history_item.state.interacted_element[i]← unguarded, while the sibling read at:3430in the same function does guard (if i < len(...) else None).So an entry with actions and an empty element list loads and then fails later with a bare
IndexErrorduring replay. That is a worse failure than the one this PR removes, so the empty list was the wrong fill even though it validates.[None] * len(actions)fixes it without inventing a convention:Noneis a legal item type,_update_action_indicesstarts withif not historical_element ... : return action(service.py:3546, i.e. "no element info → run the action as-is"), and first-party code already writes exactly this shape for "actions with no captured elements" atservice.py:1751(interacted_elements = [None]) andservice.py:3318(interacted_element=[None] * len(self.initial_actions)).tests/ci/test_ax_name_matching.py:94,205,319pin the same representation.What is unchanged. Readers that only consume the field stay correct either way:
views.py:827and:842writeh.state.interacted_element or [None] * len(h.model_output.action), and for an entry padded to lengthnthat expression now returns the list it already wanted; the other readers (service.py:3238,:3797,:3800,variable_detector.py:39-41) are length- or truthiness-guarded.Testing
tests/ci/test_history_legacy_load.py(new, 4 tests) — the two loaders, a control that pins the behaviour that already worked, and the replay-alignment case from the review.main@d8110c5+ the first three testspytest tests/ci/test_history_legacy_load.py2 failed, 1 passed4a48943([]fill) + all four testspytest tests/ci/test_history_legacy_load.py1 failed, 3 passed593fdc7a(per-action fill)pytest tests/ci/test_history_legacy_load.py4 passed4a48943pytest tests/ci/test_history_legacy_load.py tests/ci/test_history_wait_time.py11 passedThe two
mainfailures are verbatimpydantic_core._pydantic_core.ValidationError: ... history.0.state.interacted_element.list[nullable[DOMInteractedElement]] Input should be a valid list [type=list_type, input_value=None, input_type=NoneType], one per loader. The middle row is the reviewer's case measured as a test:assert [] == [None]→Right contains one more item: None, i.e. the entry loads with zero slots for one action.test_load_from_dict_keeps_a_stored_interacted_elementpasses on every row, which is the point: a payload that already carries the key is untouched by this change.Regression sweep over the nearest neighbours:
pytest tests/ci/test_ax_name_matching.py tests/ci/test_beta_agent.py→4 failed, 207 passed, 1 skipped(162 s). All four failures are intest_beta_agent.py(test_beta_agent_run_env_sets_packaged_agent_tools_dir,..._preserves_explicit_agent_tools_dir,test_rust_terminal_binary_finds_default_terminal_install,test_beta_agent_adds_available_files_to_task_context) and reproduce identically withviews.pyreverted to4a48943on this machine (4 failed, 198 deselected, 3.9 s) — they concern the packaged/rust terminal and tools-dir plumbing on Windows, not history loading.test_ax_name_matching.py, which exercises the element-matching path that consumes this field, is fully green.Static checks, run against the working tree with the repo's own config:
ruff check(0.12.10, the rev pinned in.pre-commit-config.yaml) on the changed files →All checks passed!ruff format --check→ clean. Note on how that was measured: this clone hascore.autocrlf=true, so files on disk are CRLF while committed blobs are LF, and a direct check then reports a whole-file rewrite for reasons unrelated to this diff. Checking an LF-normalised copy reports1 file already formatted.browser_use/beta/service.pyis in the[tool.pyright] excludelist, and neither commit widens a type — the fill islist[None]where the annotation already accepts it.Not run: the full
tests/cisuite and the browser/CDP integration tests (this reproduction needs neither),./bin/lint.shas a script, and anything requiring a live model provider or API key. TheIndexErroratservice.py:3481is cited from reading that function, not from an executed replay — driving it would need a browser session, so the test pins the invariant that makes it unreachable instead. Reported from a local source checkout, not from CI.Overlap with existing PRs
305 open PRs (the count when this PR was first pushed) were enumerated and their patches scanned for these paths. Sixteen touch
browser_use/agent/views.py,browser_use/agent/service.pyhistory code, orbrowser_use/beta/service.py— including several that only constructBrowserStateHistory(..., interacted_element=[])in tests (#5626, #5656, #5680, #5709, #5741, #5786). None of them modifies theload_from_dictfill or_load_rust_history: #5644 mentionsload_from_dictin its diff but its changes are confined tobrowser_use/agent/service.pyandtests/ci/test_agent_checkpoint.py. #5786 and #5777 touch the same file in other regions.Additional notes
Only the missing-key branch is fixed here. A
statedict that is absent entirely still fails on the requiredurl/title/tabsfields, which is a different (and defensible) contract —load_from_dictdeliberately keepsh['state'] = stateinside the guard.AI assistance: Qoder (agentic CLI) wrote the reproduction, the tests, both fills and this description, including the correction pushed in
593fdc7a. This PR was opened during an unattended contribution run, so the account owner has not read the diff yet; the human-only review attestation is therefore left unticked deliberately.