-
Notifications
You must be signed in to change notification settings - Fork 184
feat(core): carry structured exception detail on ExecutionResult #907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| --- | ||
| title: "SerializedError" | ||
| description: "Lossless wire representation of an execution result's exception" | ||
| --- | ||
|
|
||
|
|
||
|
|
||
| The legacy `exception` field is serialized as `str(e)`, which collapses the error | ||
| to whichever single message the server's `ErrorConfig` mode baked in and drops the | ||
| concrete type and the retry/notify flags. This model carries all of it so clients | ||
| can rehydrate the exception the server actually raised. The field is additive for | ||
| wire compatibility: old servers never send it, old clients ignore it | ||
|
|
||
| ## Fields | ||
|
|
||
| <ParamField path="error_type" type="str" required> | ||
| </ParamField> | ||
|
|
||
| <ParamField path="dev_message" type="str" required> | ||
| </ParamField> | ||
|
|
||
| <ParamField path="user_message" type="str" required> | ||
| </ParamField> | ||
|
|
||
| <ParamField path="agent_message" type="str" required> | ||
| </ParamField> | ||
|
|
||
| <ParamField path="should_retry_later" type="bool" default="False"> | ||
| </ParamField> | ||
|
|
||
| <ParamField path="should_notify_team" type="bool" default="False"> | ||
| </ParamField> | ||
|
|
||
|
|
||
| ## Module | ||
|
|
||
| `notte_core.browser.observation` |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| """ExecutionResult exception round-trips. | ||
|
|
||
| The legacy wire format serialized `exception` as `str(e)`, so the concrete error | ||
| type, the per-audience messages and the retry/notify flags were destroyed in | ||
| transit and every remote failure rehydrated as a bare `NotteBaseError`. The | ||
| additive `exception_detail` field carries the full error; these tests pin the | ||
| round-trip, the fallbacks, and compatibility with payloads from older servers. | ||
| """ | ||
|
|
||
| import json | ||
|
|
||
| import pytest | ||
| from notte_core.actions import ClickAction | ||
| from notte_core.browser.observation import ExecutionResult, SerializedError, TimedSpan | ||
| from notte_core.errors.actions import ActionExecutionError | ||
| from notte_core.errors.base import NotteBaseError | ||
|
|
||
|
|
||
| def _failed_result(exception: Exception) -> ExecutionResult: | ||
| span = TimedSpan.start().close() | ||
| return ExecutionResult( | ||
| action=ClickAction(id="B1"), | ||
| success=False, | ||
| message="click failed", | ||
| started_at=span.started_at, | ||
| ended_at=span.ended_at, | ||
| exception=exception, | ||
| ) | ||
|
|
||
|
|
||
| def test_notte_error_round_trips_with_type_messages_and_flags() -> None: | ||
| original = ActionExecutionError(action_id="click", url="https://example.com", reason="element is disabled") | ||
| dumped = _failed_result(original).model_dump_json() | ||
|
|
||
| restored = ExecutionResult.model_validate_json(dumped) | ||
|
|
||
| assert isinstance(restored.exception, ActionExecutionError) | ||
| assert restored.exception.dev_message == original.dev_message | ||
| assert restored.exception.user_message == original.user_message | ||
| assert restored.exception.agent_message == original.agent_message | ||
| assert restored.exception.should_retry_later is True | ||
| assert restored.exception.should_notify_team is True | ||
| assert "element is disabled" in restored.exception.dev_message | ||
|
|
||
|
|
||
| def test_legacy_payload_without_detail_keeps_old_behavior() -> None: | ||
| original = ActionExecutionError(action_id="click", url="https://example.com", reason="element is disabled") | ||
| payload = json.loads(_failed_result(original).model_dump_json()) | ||
| # An older server serializes only the stringified exception. | ||
| del payload["exception_detail"] | ||
|
|
||
| restored = ExecutionResult.model_validate(payload) | ||
|
|
||
| assert type(restored.exception) is NotteBaseError | ||
| assert restored.exception.dev_message == str(original) | ||
|
|
||
|
|
||
| def test_unknown_error_type_falls_back_to_base_class() -> None: | ||
| detail = SerializedError( | ||
| error_type="ServerOnlyError", | ||
| dev_message="dev", | ||
| user_message="user", | ||
| agent_message="agent", | ||
| should_retry_later=True, | ||
| ) | ||
| payload = json.loads(_failed_result(ValueError("boom")).model_dump_json()) | ||
| payload["exception"] = "dev" | ||
| payload["exception_detail"] = detail.model_dump() | ||
|
|
||
| restored = ExecutionResult.model_validate(payload) | ||
|
|
||
| assert type(restored.exception) is NotteBaseError | ||
| assert restored.exception.dev_message == "dev" | ||
| assert restored.exception.user_message == "user" | ||
| assert restored.exception.should_retry_later is True | ||
|
|
||
|
|
||
| def test_first_party_error_outside_core_rehydrates_concrete_type() -> None: | ||
| """Errors defined in notte-browser/notte-agent resolve without the caller importing them.""" | ||
| for error_type in ("InvalidLocatorRuntimeError", "MaxStepsReachedError", "PageLoadingError"): | ||
| detail = SerializedError( | ||
| error_type=error_type, | ||
| dev_message="dev", | ||
| user_message="user", | ||
| agent_message="agent", | ||
| ) | ||
|
|
||
| error = detail.to_exception() | ||
|
|
||
| assert type(error).__name__ == error_type | ||
|
|
||
| from notte_browser.errors import BrowserError | ||
|
|
||
| # hierarchy matters: `except BrowserError` on the client must catch a | ||
| # rehydrated PageLoadingError | ||
| assert isinstance( | ||
| SerializedError( | ||
| error_type="PageLoadingError", dev_message="dev", user_message="user", agent_message="agent" | ||
| ).to_exception(), | ||
| BrowserError, | ||
| ) | ||
|
|
||
|
|
||
| def test_plain_exception_round_trips_messages() -> None: | ||
| restored = ExecutionResult.model_validate_json(_failed_result(TimeoutError("boom")).model_dump_json()) | ||
|
|
||
| assert isinstance(restored.exception, NotteBaseError) | ||
| assert restored.exception.dev_message == "boom" | ||
| assert restored.exception.user_message == "boom" | ||
|
|
||
|
|
||
| def test_detail_only_payload_rehydrates_exception() -> None: | ||
| payload = json.loads( | ||
| _failed_result(ActionExecutionError(action_id="click", url="https://example.com")).model_dump_json() | ||
| ) | ||
| # A future server may stop sending the lossy legacy field altogether. | ||
| payload["exception"] = None | ||
|
|
||
| restored = ExecutionResult.model_validate(payload) | ||
|
|
||
| assert isinstance(restored.exception, ActionExecutionError) | ||
|
|
||
|
|
||
| def test_local_construction_populates_detail() -> None: | ||
| result = _failed_result(ActionExecutionError(action_id="click", url="https://example.com", reason="nope")) | ||
|
|
||
| assert result.exception_detail is not None | ||
| assert result.exception_detail.error_type == "ActionExecutionError" | ||
| assert "nope" in result.exception_detail.dev_message | ||
|
|
||
|
|
||
| def test_success_keeps_exception_invariant() -> None: | ||
| span = TimedSpan.start().close() | ||
| result = ExecutionResult( | ||
| action=ClickAction(id="B1"), | ||
| success=True, | ||
| message="clicked", | ||
| started_at=span.started_at, | ||
| ended_at=span.ended_at, | ||
| ) | ||
| assert result.exception is None | ||
| assert result.exception_detail is None | ||
|
|
||
| with pytest.raises(ValueError, match="Exception should be None"): | ||
| ExecutionResult( | ||
| action=ClickAction(id="B1"), | ||
| success=True, | ||
| message="clicked", | ||
| started_at=span.started_at, | ||
| ended_at=span.ended_at, | ||
| exception=ValueError("boom"), | ||
| ) |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject
exception_detailfor successful results.When
success=True, this validator returns before checkingexception_detail.model_post_initrejects onlyexception, so a payload withsuccess=True,exception=None, and populatedexception_detailis accepted and can serialize contradictory success and failure state.Extend the success invariant to reject a non-
Noneexception_detail. Add a matching test.Proposed fix
def model_post_init(self, context: Any, /) -> None: if self.success: - if self.exception is not None: + if self.exception is not None or self.exception_detail is not None: raise ValueError("Exception should be None if success is True")🤖 Prompt for AI Agents