refactor(tui): prompt deep-module PR 5 — lossless wire replay + shell-exec hardening#229
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds configured-shell execution with bounded output and cancellation cleanup, typed prompt-event routing, payload-free live-view labels, bounded tool-argument scanning, nested subagent ancestry handling, and replay-turn filtering for tool-associated media messages. ChangesShell and prompt routing
Live-view rendering
Replay and release notes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
22b0f07 to
993deb7
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
1420-1431: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
finish_sub_tool_callclears_last_subagent_tool_callunconditionally, before confirming it's the call being finished.
self._last_subagent_tool_call = None(line 1423) runs before thesub_tool_call is Nonecheck and regardless of whethertool_result.tool_call_idmatches the call the pointer actually references. If two subagent tool calls are concurrently ongoing — A is_last_subagent_tool_call(still streaming args) and B finishes — finishing B wipes the pointer to A. SubsequentToolCallPartevents for A then silently no-op inappend_sub_tool_call_part(guarded byif self._last_subagent_tool_call is None: return), dropping A's streamed arguments.The existing nested-lifecycle test only exercises a linear ancestor chain, so this concurrent-siblings case isn't covered.
🐛 Proposed fix
def finish_sub_tool_call(self, tool_result: ToolResult): if tool_result.tool_call_id in self._finished_subagent_tool_call_ids: return - self._last_subagent_tool_call = None sub_tool_call = self._ongoing_subagent_tool_calls.pop(tool_result.tool_call_id, None) if sub_tool_call is None: return + if self._last_subagent_tool_call is sub_tool_call: + self._last_subagent_tool_call = None self._finished_subagent_tool_call_ids.add(tool_result.tool_call_id)🤖 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 1420 - 1431, Update finish_sub_tool_call so _last_subagent_tool_call is cleared only when the completed tool_result corresponds to the call currently referenced by that pointer, and only after confirming the ongoing call exists. Preserve the pointer when another concurrent subagent call finishes so append_sub_tool_call_part can continue processing its streamed arguments.
🤖 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/command_runner.py`:
- Around line 54-72: Move process.stdin.close() and the stdout/stderr reader and
wait task creation into the try block guarded by _terminate_and_reap, ensuring
any failure during stdin closure or task setup terminates and reaps the child
process. Preserve cancellation and exception propagation while avoiding
references to tasks that were never created.
In `@src/pythinker_code/ui/shell/visualize/_live_view.py`:
- Around line 154-155: Scope unknown content-part deduplication per _LiveView
instance rather than process-wide: remove the module-level
_SEEN_UNKNOWN_CONTENT_PART_TYPES and initialize an equivalent set in
_LiveView.__init__, then update append_content to use that instance attribute.
In tests/ui_and_conv/test_live_content_parts.py lines 107-127, no direct change
is required; the instance-scoped fix removes the test’s dependence on global
state.
- Line 265: Update _archive_completed_tool_card, which handles blocks popped
from _tool_call_blocks, to remove every _subagent_tool_call_ancestry entry whose
referenced _ToolCallBlock is the archived block. Keep cleanup() and
discard_retry_attempt() clearing their existing state, while ensuring all
archive/pop paths purge ancestry immediately so stale nested events follow the
missing-ancestry path.
In `@tests/ui_and_conv/test_shell_command_runner.py`:
- Around line 31-38: Add a test near the _runner helper that constructs
ShellCommandRunner with a negative output_limit_bytes value and asserts that
ValueError is raised, covering the constructor’s validation path.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 1420-1431: Update finish_sub_tool_call so _last_subagent_tool_call
is cleared only when the completed tool_result corresponds to the call currently
referenced by that pointer, and only after confirming the ongoing call exists.
Preserve the pointer when another concurrent subagent call finishes so
append_sub_tool_call_part can continue processing its streamed arguments.
🪄 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: 764c729b-d1fd-4631-9a77-859540d45dc6
📒 Files selected for processing (14)
CHANGELOG.mdsrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/command_runner.pysrc/pythinker_code/ui/shell/replay.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_live_view.pytests/ui_and_conv/test_background_completion_watcher.pytests/ui_and_conv/test_live_content_parts.pytests/ui_and_conv/test_nested_subagent_events.pytests/ui_and_conv/test_replay.pytests/ui_and_conv/test_shell_bang_spawn.pytests/ui_and_conv/test_shell_command_runner.pytests/ui_and_conv/test_subagent_live_stream.pytests/ui_and_conv/test_tool_call_block.py
Address review findings on the session-scoped shell/live-view work: - ShellCommandRunner.run: move stdin.close() and stream/wait task creation inside the guarded scope so a failure there terminates and reaps the child instead of leaking it; _terminate_and_reap now tolerates a short task tuple. - _LiveView: purge subagent tool-call ancestry entries when their root block is flushed, so nested-event ids don't accumulate for the rest of a turn and a late/duplicate nested event falls back to "missing ancestry" instead of mutating an archived block. - _LiveView: scope the unknown-content-part "log once" set to each view instead of a process-global, keeping the behavior deterministic and tests isolated. Add regression tests for the stdin-close cleanup path, the ancestry purge on root flush, and the negative output-limit guard.
1 participant
Part 5 of the TUI prompt deep-module refactor (plan: docs/superpowers/plans/2026-07-19-tui-prompt-deep-module-refactor.md; stacked on #228).
Task 10 — lossless wire rendering and replay
ImageURLPart/AudioURLPart/VideoURLPartas payload-free labels ([image],[audio:<id>],[video]) and unknown future content parts as a muted[<type>]with one debug log per type — no data URLs or payloads ever reach the terminal.SubagentEvents resolve ancestry through a tool-call-id index and roll up under the correct root tool card, bounded at depth 16 with a muted fallback on missing ancestry or overflow; duplicate/late terminal results are ignored (first result stays authoritative).tool_call_id(provider-remapped tool results) no longer start replay user turns; real user media messages still do (resolves the long-standing FIXME in_is_user_message).Task 11 — shell-exec hardening + typed idle events
ShellCommandRunner(ui/shell/command_runner.py):!commands execute viaHost.execthrough the detected configured shell (<shell> -c; PowerShell-command) with separate 1 MiB stdout/stderr caps, post-cap draining, and terminate/reap on cancellation. Deliberate behavior change (changelog'd): POSIX moves off implicit/bin/sh, Windows moves offcmd.exe. Windows console detachment is preserved —Host.execappliesCREATE_NO_WINDOW(covered in pythinker-host tests); the shell-level spawn test now asserts the!path can never fall back tocreate_subprocess_shell.(No output),exit <code>,Failed to run shell command: <reason>) and the standalone-cdrejection are unchanged.PromptEventKindreplaces string prompt-event kinds;_wait_for_input_or_activitynow distinguishesBACKGROUND_GRACE_EXPIREDfromINPUT_ACTIVITY(0.75 s grace and input-wins-ties preserved), with new race/cancellation tests.Verification
make check-pythinker-code: all checks passed.make test-pythinker-code: 8,338 unit + 65 e2e passed (7 skipped, 1 xfailed).Summary by CodeRabbit
New Features
Bug Fixes