feat(tui): improve reasoning and agent activity rendering#231
Conversation
Route single `Agent` tool calls through the same payload-free semantic activity tree previously used only for `RunAgents`, so a lone subagent renders per-agent status rows (waiting/running/thinking + reading…/ searching…/running command…) instead of raw nested tool calls, args, or streamed output. Derive the running description from the tool `description` argument, suppress stale pre-result activity once the parent returns, and render the completed result via the dedicated result renderer. Update the block, nested-lifecycle, integration, and card tests to the new semantic contract (raw payloads never leak; first-result-wins dedup and ancestry roll-up preserved), and fix the stale `RunAgents(`->`Agents(` label assertion in the action-spacer test.
Address CodeRabbit review findings on the branch: - core: a non-streaming OpenAI Responses reasoning item can return encrypted_content with an empty summary; the conversion loop dropped it entirely, diverging from the streaming output_item.done path and making the reasoning boundary non-replayable. Emit an encrypted ThinkPart in that case, mirroring the streaming behavior, with focused coverage. - tui: render "1 agent" (singular) instead of "1 agents" in the grouped agents summary, and make the uniform-status check explicitly boolean. The two other flagged reasoning findings (summary-index preservation and cross-output merge collisions) were verified against the code and tests and are non-issues: real deltas always carry valid indices, and encrypted boundaries already block adjacent cross-output merges.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds indexed OpenAI Responses reasoning boundaries, semantic payload-free Agent and RunAgents activity rendering, subagent description and ownership propagation, and platform-specific terminal resize recovery with continuous PTY coverage. ChangesReasoning stream boundaries
Subagent activity tree
Resize recovery behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
elkaix
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/visualize/_blocks.py (1)
1490-1524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Nonereturn annotations to these public methods.
append_sub_tool_call(Line 1490),append_sub_tool_call_part(Line 1508), andfinish_sub_tool_call(Line 1524) returnNonebut omit the annotation (Ruff ANN202 flags the same).Proposed fix
- def append_sub_tool_call(self, tool_call: ToolCall, *, agent_id: str | None = None): + def append_sub_tool_call(self, tool_call: ToolCall, *, agent_id: str | None = None) -> None:def append_sub_tool_call_part( self, tool_call_part: ToolCallPart, *, agent_id: str | None = None - ): + ) -> None:- def finish_sub_tool_call(self, tool_result: ToolResult, *, agent_id: str | None = None): + def finish_sub_tool_call(self, tool_result: ToolResult, *, agent_id: str | None = None) -> None:As per path instructions: "Flag missing type annotations on public functions and methods."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/visualize/_blocks.py` around lines 1490 - 1524, Add explicit -> None return annotations to the public methods append_sub_tool_call, append_sub_tool_call_part, and finish_sub_tool_call, preserving their existing behavior and parameters.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 1490-1524: Add explicit -> None return annotations to the public
methods append_sub_tool_call, append_sub_tool_call_part, and
finish_sub_tool_call, preserving their existing behavior and parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70866f53-9b78-42a4-a448-f993b8cd6ed0
📒 Files selected for processing (24)
CHANGELOG.mdpackages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_responses.pypackages/pythinker-core/src/pythinker_core/message.pypackages/pythinker-core/tests/test_message.pypackages/pythinker-core/tests/test_stream_message_assembler.pypackages/pythinker-core/tests/test_stream_tool_call_metadata.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/wire/types.pytests/core/test_wire_message.pytests/e2e/test_shell_pty_prompt_layout_e2e.pytests/ui_and_conv/test_empty_think_part_indicator.pytests/ui_and_conv/test_live_content_parts.pytests/ui_and_conv/test_nested_subagent_events.pytests/ui_and_conv/test_render_matrix.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_subagent_live_stream.pytests/ui_and_conv/test_tool_call_block.pytests/ui_and_conv/test_tui_blocks_integration.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_visualize_running_prompt.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/visualize/_blocks.py (1)
1830-1836: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep provisional background agents in the live activity path.
finish()sets_is_background_pendingfor launch results, but this branch treats every non-Noneresult as terminal and removes the activity rows. The legacy_compose()then renders the successful launch asCOMPLETED, so the spinner and agent progress disappear until the terminal update.Proposed lifecycle fix
- if self._result is not None: + if self._result is not None and not self._is_background_pending: return [] - if self._result is None: + if self._result is None or self._is_background_pending: streamed_output = self._streamed_output_text()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/visualize/_blocks.py` around lines 1830 - 1836, Update the ownership branch in the activity-rendering method to distinguish provisional background launches from terminal results: when _is_background_pending is set, continue returning _agent_activity_children so live progress remains visible. Only suppress activity rows for non-pending results, and ensure the legacy _compose() path does not render pending launches as COMPLETED.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 1830-1836: Update the ownership branch in the activity-rendering
method to distinguish provisional background launches from terminal results:
when _is_background_pending is set, continue returning _agent_activity_children
so live progress remains visible. Only suppress activity rows for non-pending
results, and ensure the legacy _compose() path does not render pending launches
as COMPLETED.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e57fc246-5a82-4e50-b01b-715a04466e12
📒 Files selected for processing (1)
src/pythinker_code/ui/shell/visualize/_blocks.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 2069-2077: Update the card activity-building logic around
_subagent_activity_children to remove the outer condition requiring _result to
be unset, allowing the helper to run for background-pending cards. Preserve the
existing arguments and rely on _subagent_activity_children to distinguish
terminal results from background-pending activity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a01c85ac-3758-4d71-9f5d-b152aad18971
📒 Files selected for processing (3)
src/pythinker_code/ui/shell/visualize/_blocks.pytests/e2e/test_shell_pty_e2e.pytests/ui_and_conv/test_tool_call_block.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/visualize/_blocks.py (1)
765-769: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve encrypted thinking boundaries in the UI accumulator.
ThinkPart.merge_in_place()treatsencryptedas a merge boundary, but_append_thinking_segment()only comparessummary_index. An encrypted-only part can still be merged into a later segment with the same index, losing that boundary in the UI. Thread the encrypted/boundary state throughappend()and_append_thinking_segment(), and add a regression test for the encrypted-only case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/visualize/_blocks.py` around lines 765 - 769, Update the UI accumulator’s append and _append_thinking_segment flow to accept and propagate the encrypted boundary state, and make segment merging respect encrypted boundaries in addition to summary_index. Ensure encrypted-only parts remain separate from later segments with the same index, and add a regression test covering that case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test_shell_pty_e2e.py`:
- Around line 930-931: Update the cancellation test around the command
constructing cancel_output.txt and the verification near line 976 so it cannot
pass before the 30-second sleep completes. Either assert that the entire process
group terminates directly, or delay the sentinel-file check until after the
command could have reached the write while preserving the expected cancellation
behavior.
- Around line 956-963: Replace the fixed stabilization deadline after the
submitted prompt with a bounded wait for an observable running-turn/Escape-ready
signal. Use the existing shell or test state exposed by the surrounding PTY flow
to confirm prompt_toolkit has installed the running-turn delegate before sending
Escape, while retaining a timeout and diagnostic failure if readiness is not
observed.
In `@tests/ui_and_conv/test_subagent_live_stream.py`:
- Around line 680-681: Add an assertion in the live-stream test alongside the
existing output checks to verify that both background task IDs, including
agent-beta-raw-id without a nested event, are suppressed from output. Keep the
existing assertions and cover the absence of each raw ID.
In `@tests/ui_and_conv/test_tool_call_block.py`:
- Around line 526-553: Strengthen
test_run_agents_background_launch_keeps_live_agent_activity_in_card_style by
asserting that the composed card-mode output does not contain the terminal “Run
Agents completed” row, while retaining the existing assertion for live waiting
activity. Ensure the test covers suppression of completion output for background
launches rather than only verifying that activity is present.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 765-769: Update the UI accumulator’s append and
_append_thinking_segment flow to accept and propagate the encrypted boundary
state, and make segment merging respect encrypted boundaries in addition to
summary_index. Ensure encrypted-only parts remain separate from later segments
with the same index, and add a regression test covering that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc355538-2103-40b8-be2a-719ff51cbb8f
📒 Files selected for processing (4)
src/pythinker_code/ui/shell/visualize/_blocks.pytests/e2e/test_shell_pty_e2e.pytests/ui_and_conv/test_subagent_live_stream.pytests/ui_and_conv/test_tool_call_block.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Related Issue
None. This branch was developed without a linked issue.
Description
Summary
User impact
Reasoning summaries no longer expose internal Markdown delimiters or lose replay metadata, while agent progress remains readable at narrow and normal terminal widths without leaking task payloads.
Risk
Verification
make check-pythinker-codemake test-pythinker-code: 8,428 passed, 7 skipped, 1 xfailed;tests_e2e: 65 passed, 4 skipped.make check-pythinker-coremake test-pythinker-core: 446 passed.Rollback and review
Checklist
make gen-changelogto update the changelog. Not applicable:CHANGELOG.mdwas updated directly under## Unreleasedper repository instructions.make gen-docsto update the user documentation. Not applicable: no generated documentation source changed.Summary by CodeRabbit
New Features
Bug Fixes