Visualizer: hide “Reasoning:” when Responses reasoning is absent #1163
Conversation
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
… ActionEvent/MessageEvent now only render the Reasoning section when\n summary or content is present.\n- Add unit test to assert no Reasoning label when none provided.\n\nCo-authored-by: openhands <[email protected]>
Contributor
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Collaborator
Author
|
OpenHands-GPT-5 here. Ran the reasoning example with openai/gpt-5.1-codex-mini, direct OpenAI (no base_url), reasoning_effort=None. As expected, no 'Reasoning:' section was rendered. Below are consecutive console lines: the metrics line before, the panel with the agent's output, and the metrics line after. No 'Reasoning:' label present in the visualization, which is the expected behavior. Token counters updated between panels (input/output increased). |
Collaborator
Author
|
OpenHands-GPT-5 update (ANSI-clean). Reposting the snippet without terminal color codes. Ran openai/gpt-5.1-codex-mini via direct OpenAI, reasoning_effort=None. No 'Reasoning:' section appears. Confirmed: no 'Reasoning:' label present; token counters incremented between panels. |
Collaborator
Author
|
OpenHands-GPT-5 follow-up (ANSI-clean): reasoning_effort=high, no summaries. Showing the first moment reasoning tokens appear: the metrics line before, the event in between (no Reasoning: section), and the metrics line after. Concise comparison:
|
tofarr
reviewed
Nov 14, 2025
| content.append(f"- {s}\n") | ||
| if has_content: | ||
| for b in list(reasoning_item.content or []): | ||
| content.append(f"{b}\n") |
Collaborator
There was a problem hiding this comment.
It feels that this should be in a utility method to be DRYer
There was a problem hiding this comment.
I'm on it! enyst can track my progress at all-hands.dev
There was a problem hiding this comment.
Summary of work completed
Goal addressed
- Reviewer asked to DRY up the duplicated “Reasoning:” rendering logic.
Changes made
- Added utility
- File: openhands-sdk/openhands/sdk/event/utils.py
- Function: render_responses_reasoning_block(reasoning_item, *, leading_newlines=False) -> Text | None
- Renders a “Reasoning:” block only when Responses reasoning has summary or content; returns None otherwise. Supports optional leading newlines to preserve spacing where needed.
- Refactored visualizers to use the utility
- openhands-sdk/openhands/sdk/event/llm_convertible/action.py
- Use render_responses_reasoning_block(self.responses_reasoning_item)
- openhands-sdk/openhands/sdk/event/llm_convertible/message.py
- Use render_responses_reasoning_block(self.llm_message.responses_reasoning_item, leading_newlines=True) to match prior spacing
- openhands-sdk/openhands/sdk/event/llm_convertible/action.py
- No behavior changes beyond preserving the prior fix to hide “Reasoning:” when absent; code is now simpler and centralized.
Validation performed
- Ran pre-commit on edited files: ruff format/lint, pycodestyle, pyright all passed.
- Ran tests for the visualizer: tests/sdk/conversation/test_visualizer.py — 17 passed.
- Pushed commit to the existing branch fix/hide-empty-reasoning-visualizer:
- Commit: 1efd2ec
- Files changed: action.py, message.py, and new utils.py
Checklist
- Addressed the reviewer’s DRY request: done
- Kept changes minimal and focused (only 3 files touched): done
- Preserved existing behavior and presentation semantics: done
- Pre-commit hooks and relevant tests pass: done
Next steps
- Ready for review. If desired, we can extend the utility in the future to support additional styling hooks or use it anywhere else that may eventually render Responses reasoning.
Collaborator
Author
There was a problem hiding this comment.
It feels that this should be in a utility method to be DRYer
I did this, but TBH I think maybe it's not worth it? It appears to repeat itself, but it's a few lines, and they're tricky to split in a very clear way since it's an attribute there. I feel the original is more clear than other options. 🤷
It's also a bit quirky in message.py, because it's not helping understanding and maintainability to hide from sight (aka utility method you need to navigate to) details of what exactly is happening. We had a number of troubles figuring out every detail in this code and it's the kind of code I should know by heart. That may be just me though. Cc: @tofarr
tofarr
requested changes
Nov 14, 2025
Collaborator
tofarr
left a comment
tofarr
left a comment
There was a problem hiding this comment.
Maybe introduce a utility method as suggested to dry up the code
…ctionEvent/MessageEvent - Introduce event.utils.render_responses_reasoning_block - Replace duplicated logic in action/message visualizers Co-authored-by: openhands <[email protected]>
Contributor
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
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.
Summary
Changes
Motivation
Some providers (e.g., OpenAI GPT-5.1) may not return reasoning summaries by default. Previously, the UI would still display a "Reasoning:" header and then show other content (like a tool command), which was confusing. This change keeps the UI clean and only shows a Reasoning block when we actually have textual reasoning summary/content to render.
Notes
Co-authored-by: openhands [email protected]
@enyst can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:860b8c1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
860b8c1-python) is a multi-arch manifest supporting both amd64 and arm64860b8c1-python-amd64) are also available if needed