OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

refactor err details

Open rbren opened this issue 8 months ago • 5 comments

  • [ ] This change is worth documenting at https://docs.all-hands.dev/
  • [x] Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below CHANGELOG: Fix for unprocessed error IDs showing up in the chat window

End-user friendly description of the problem this fixes or functionality that this introduces.

We changed the way error details get plumbed through in https://github.com/All-Hands-AI/OpenHands/pull/7584/files

This was a breaking change for the frontend.


Give a summary of what the PR does, explaining any non-trivial design decisions.


Link of any specific issues this addresses.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:4b0299b-nikolaik   --name openhands-app-4b0299b   docker.all-hands.dev/all-hands-ai/openhands:4b0299b

rbren avatar Apr 07 '25 21:04 rbren

@openhands please fix the python unit tests

rbren avatar Apr 16 '25 19:04 rbren

I'm on it! @rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 16 '25 19:04 openhands-ai[bot]

I've successfully fixed the Python unit tests for PR #7753 on the rb/err-details branch. Here's a summary of what I did:

Issue Identified

The PR had refactored the error handling in agent_controller.py, changing how error messages are stored and displayed. This caused several unit tests to fail because they were expecting specific error message formats that no longer matched the new implementation.

Changes Made

  1. Updated test assertions in test_agent_controller.py to match the new error message format:
    • Modified test_react_to_content_policy_violation to expect the detailed error message instead of just the error ID
    • Updated test_run_controller_with_fatal_error and other tests to check for just the error type name instead of the full error message with description
    • Fixed test_run_controller_with_context_window_exceeded_without_truncation to match the new error format
    • Fixed test_run_controller_max_iterations_has_metrics to check for the simplified error message

Results

  • All unit tests in test_agent_controller.py now pass successfully
  • The full test suite also passes without any failures related to the changes
  • Changes were committed and pushed to the rb/err-details branch, updating PR #7753

The PR can now be merged as the tests are passing with the updated assertions that match the refactored error handling implementation.

openhands-ai[bot] avatar Apr 16 '25 19:04 openhands-ai[bot]

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Docker

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #7753

Feel free to include any additional details that might help me get this PR into a better state.

openhands-ai[bot] avatar Apr 16 '25 20:04 openhands-ai[bot]

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Docker

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #7753

Feel free to include any additional details that might help me get this PR into a better state.

openhands-ai[bot] avatar Apr 16 '25 20:04 openhands-ai[bot]

@openhands fix the merge conflicts

rbren avatar Apr 29 '25 12:04 rbren

I'm on it! @rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 29 '25 12:04 openhands-ai[bot]

I successfully fixed the merge conflicts in PR #7753 on branch rb/err-details.

Here's a summary of what I did:

  1. Identified the merge conflicts in tests/unit/test_agent_controller.py which were related to error message formatting.

  2. Resolved the conflicts by accepting the changes from the main branch, which included more detailed error messages:

    • Updated AgentStuckInLoopError assertions to include the full error message: AgentStuckInLoopError: Agent got stuck in a loop
    • Updated LLMContextWindowExceedError assertion to include the full error message
  3. Committed the resolved conflicts with the message "Merge main into rb/err-details and fix merge conflicts"

  4. Successfully pushed the changes to the remote branch, which will update PR #7753

The merge conflicts have been successfully resolved, and the PR should now be able to be merged without conflicts.

The changes align with the refactored error handling in the main branch, which now includes more detailed error messages in the format ErrorType: Error message rather than just the error type.

openhands-ai[bot] avatar Apr 29 '25 12:04 openhands-ai[bot]

@openhands fix the python unit tests

rbren avatar Apr 29 '25 22:04 rbren

I'm on it! rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 29 '25 22:04 openhands-ai[bot]

@OpenHands fix the python unit tests

rbren avatar Apr 29 '25 22:04 rbren

I'm on it! rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 29 '25 22:04 openhands-ai[bot]

@OpenHands fix the python unit tests

rbren avatar Apr 29 '25 22:04 rbren

I'm on it! rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 29 '25 22:04 openhands-ai[bot]

Summary of Work

I fixed the Python unit tests in the PR #7753 (branch rb/err-details) by addressing issues with error message handling in the agent controller.

Changes Made

  1. Fixed Error Message Handling:

    • Modified the _react_to_exception method in agent_controller.py to properly include the full error message in the state.last_error field
    • For known error types (like ContentPolicyViolationError), the full error details are now stored
    • For other error types, the error type and message are properly preserved
  2. Updated Test Expectations:

    • Updated test assertions to match the actual error messages returned by the code
    • Fixed tests for context window exceeded errors, stuck in loop errors, and content policy violations

Results

  • All unit tests in tests/unit/test_agent_controller.py now pass successfully
  • The changes maintain the existing behavior while ensuring error messages are properly handled
  • The PR has been updated with these changes and is ready for review

The changes were minimal and focused on fixing the specific issue with error message handling without modifying the core functionality of the agent controller.

openhands-ai[bot] avatar Apr 29 '25 22:04 openhands-ai[bot]

Linting might be better in this release. It's not updated yet, I assume?

enyst avatar Apr 29 '25 23:04 enyst

@openhands fix the python lint

rbren avatar Apr 30 '25 16:04 rbren

I'm on it! rbren can track my progress at all-hands.dev

openhands-ai[bot] avatar Apr 30 '25 16:04 openhands-ai[bot]

I successfully fixed the Python lint issues in the PR #7753 on branch rb/err-details as requested in the comment.

Here's a summary of what I did:

  1. First, I examined the repository and identified the Python lint issues by running the pre-commit hooks.
  2. The pre-commit hooks automatically fixed several issues:
    • Trailing whitespace in multiple files
    • Missing newlines at end of files
    • Import ordering issues (imports were not properly sorted)
    • Code formatting issues (indentation, line breaks, etc.)
  3. I committed these changes with the message "Fix Python lint issues"
  4. I verified that all lint issues were fixed by running the pre-commit hooks again, which passed successfully
  5. I pushed the changes to the rb/err-details branch, updating PR #7753

The main types of fixes included:

  • Proper import ordering (e.g., moving standard library imports before third-party imports)
  • Consistent formatting of multi-line statements
  • Adding missing newlines at end of files
  • Removing trailing whitespace
  • Fixing indentation and line breaks

All Python lint issues have been successfully addressed, and the PR should now pass the lint checks.

openhands-ai[bot] avatar Apr 30 '25 16:04 openhands-ai[bot]