feat(shell): polish prompt rendering and recaps - #49
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds session-recap generation and a ChangesSession Recaps and Hook Outputs with Interactive UI
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
elkaix
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pythinker_code/ui/shell/components/tool_execution.py (1)
295-303: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDocument (or clear)
__has_expandable_payload__so its lifetime matches frame expectations.
tool_execution.pypops__suppress_generic_expand_hint__each frame, but there’s no corresponding clear for__has_expandable_payload__—it’s only set in_file_diff.pyand then only read in_has_expandable_payload(). If persistence across frames is intentional, add a one-line comment explaining that; otherwise clear it alongside the suppress flag to avoid stalecan_expand.🤖 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/components/tool_execution.py` around lines 295 - 303, The stored flag __has_expandable_payload__ is persisting across frames but only set in _file_diff.py and read by _has_expandable_payload(), which can cause stale can_expand state; either add a one-line comment near the flag's use in _file_diff.py (or above _has_expandable_payload) stating that cross-frame persistence is intentional, or clear/pop __has_expandable_payload__ in the same place where __suppress_generic_expand_hint__ is popped each frame so both flags share the same lifetime and avoid stale values.src/pythinker_code/ui/shell/visualize/__init__.py (1)
120-135: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd an explicit return type to
visualize.
visualizeis a public function undersrc/**; please declare-> None.As per coding guidelines,
src/**/*.py: 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/__init__.py` around lines 120 - 135, The public async function visualize should declare an explicit return type; update the function signature of visualize to include "-> None" (i.e., async def visualize(... ) -> None:) so the public API in src/pythinker_code/ui/shell/visualize/__init__.py is properly annotated; keep all existing parameters (initial_status, cancel_event, prompt_session, steer, btw_runner, bind_running_input, unbind_running_input, on_view_ready, on_view_closed, show_thinking_stream, show_turn_recaps) unchanged.src/pythinker_code/ui/shell/visualize/_interactive.py (1)
242-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTurn recaps can be dropped in interactive mode.
TurnEndis consumed here and never reaches_LiveView’sTurnEndbranch, so_pending_turn_recapis not set. That prevents recap emission duringcleanup()for interactive runs.Proposed fix
if isinstance(msg, TurnEnd): self._active_turn_depth = max(0, self._active_turn_depth - 1) self._turn_ended = self._active_turn_depth == 0 if self._turn_ended: self._turn_start_time = None + self._pending_turn_recap = True self._flush_prompt_refresh() continue🤖 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/_interactive.py` around lines 242 - 248, The TurnEnd message is being consumed in the interactive loop and never reaches _LiveView, so set the pending recap flag or forward the message instead: in the TurnEnd handling inside _interactive.py (the block that updates _active_turn_depth, _turn_ended, clears _turn_start_time and calls _flush_prompt_refresh), after those steps either set self._pending_turn_recap = True (so cleanup() will emit the recap) or explicitly forward the TurnEnd to the _LiveView handler (e.g., call the same dispatch used for other messages) so the _LiveView TurnEnd branch still runs; ensure the chosen approach preserves the existing depth/timing updates and does not duplicate recap emission.
🤖 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/soul/slash.py`:
- Line 60: The public async function recap in src/pythinker_code/soul/slash.py
is missing an explicit return type; update its signature from "async def
recap(soul: PythinkerSoul, args: str):" to include "-> None" (i.e., "async def
recap(soul: PythinkerSoul, args: str) -> None:") so it satisfies the project's
public typing rule for functions in src/**.
In `@src/pythinker_code/ui/shell/visualize/_blocks.py`:
- Around line 1038-1043: The timeout warning is only shown when output.timed_out
and not body.plain, hiding timeout status if partial output exists; update the
logic in the block that checks output.timed_out/output.truncated so that when
output.timed_out is true you always append a "hook timed out" warning (using
body.append and tui_rich_style("warning")), regardless of body.plain, mirroring
how output.truncated is handled, and ensure you preserve spacing/newline
behavior consistent with the existing truncation branch.
In `@tests/ui_and_conv/test_streaming_content_block.py`:
- Around line 239-243: The helper _assert_blank_line_after_activity should first
assert that the provided label appears in output and provide a clear failure
message if not; locate the activity_index by searching lines for label (as
currently done), but wrap the search in a check (or try/except around next(...))
to raise an AssertionError like f"Label '{label}' not found in output" instead
of letting StopIteration propagate, and also assert that there is a following
line before accessing lines[activity_index + 1], failing with a clear message if
the activity is the last line.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/components/tool_execution.py`:
- Around line 295-303: The stored flag __has_expandable_payload__ is persisting
across frames but only set in _file_diff.py and read by
_has_expandable_payload(), which can cause stale can_expand state; either add a
one-line comment near the flag's use in _file_diff.py (or above
_has_expandable_payload) stating that cross-frame persistence is intentional, or
clear/pop __has_expandable_payload__ in the same place where
__suppress_generic_expand_hint__ is popped each frame so both flags share the
same lifetime and avoid stale values.
In `@src/pythinker_code/ui/shell/visualize/__init__.py`:
- Around line 120-135: The public async function visualize should declare an
explicit return type; update the function signature of visualize to include "->
None" (i.e., async def visualize(... ) -> None:) so the public API in
src/pythinker_code/ui/shell/visualize/__init__.py is properly annotated; keep
all existing parameters (initial_status, cancel_event, prompt_session, steer,
btw_runner, bind_running_input, unbind_running_input, on_view_ready,
on_view_closed, show_thinking_stream, show_turn_recaps) unchanged.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 242-248: The TurnEnd message is being consumed in the interactive
loop and never reaches _LiveView, so set the pending recap flag or forward the
message instead: in the TurnEnd handling inside _interactive.py (the block that
updates _active_turn_depth, _turn_ended, clears _turn_start_time and calls
_flush_prompt_refresh), after those steps either set self._pending_turn_recap =
True (so cleanup() will emit the recap) or explicitly forward the TurnEnd to the
_LiveView handler (e.g., call the same dispatch used for other messages) so the
_LiveView TurnEnd branch still runs; ensure the chosen approach preserves the
existing depth/timing updates and does not duplicate recap emission.
🪄 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 Plus
Run ID: 738240ed-65c6-44a6-9aa7-08ec547f73e0
📒 Files selected for processing (37)
CHANGELOG.mdsrc/pythinker_code/auth/browser_login_page.pysrc/pythinker_code/auth/openai.pysrc/pythinker_code/config.pysrc/pythinker_code/hooks/engine.pysrc/pythinker_code/session_recap.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/tool_execution.pysrc/pythinker_code/ui/shell/echo.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/selectors/settings.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/tool_renderers/_file_diff.pysrc/pythinker_code/ui/shell/tool_renderers/edit.pysrc/pythinker_code/ui/shell/tool_renderers/write.pysrc/pythinker_code/ui/shell/visualize/__init__.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/ui/theme.pysrc/pythinker_code/wire/server.pysrc/pythinker_code/wire/types.pytests/auth/test_openai_auth.pytests/core/test_config.pytests/core/test_wire_message.pytests/hooks/test_integration.pytests/test_session_recap.pytests/ui_and_conv/test_live_view_notifications.pytests/ui_and_conv/test_md_table_contract.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_shell_prompt_echo.pytests/ui_and_conv/test_slash_completer.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_tui_card_tool_renderers.pytests/ui_and_conv/test_visualize_running_prompt.pytests_e2e/test_wire_protocol.py
Apply verified fixes from the branch code-diff review:
- write/_file_diff: a brand-new file >10k lines produced a one-line diff
summary ("removed 1 / added 1") instead of "Wrote N lines". The summary
block's "- (0 lines)" line made preview.removed==1, so the diff path was
taken regardless of _is_existing_file_diff. Add DiffPreview.is_new_file
(every block's old side empty) and gate the diff path on it, fixing both
routing clauses. Pre-existing bug; validated across new/existing × huge/small.
- render_utils.sanitize_ansi: strip 8-bit C1 controls (0x80-0x9F), incl. the
single-byte CSI/OSC/PM/APC introducers terminals still interpret.
- _live_view: route the per-turn recap banner through sanitize_ansi.
- wire/types.HookOutput: bound stdout/stderr with max_length (12_032, headroom
for the engine's truncation marker) so oversized payloads fail at the wire
boundary instead of being silently accepted.
- auth/browser_login_page: cap the asset data-uri cache (lru_cache maxsize=16).
- hooks/engine: document why OnResolved stays Callable[..., None] (the runtime
intentionally supports both 5- and 6-arg callbacks; a strict Protocol/overload
rejects one valid arity — confirmed via the type gate).
- tests: backfill session_recap (3 -> 24 tests, incl. summarize_session via a
fake session), huge-new-file write render, and C1 ANSI stripping.
- soul/permission: add shell_destructive_reason() to flag irreversible commands (recursive force-delete, force-push, hard reset, raw disk writes) so auto mode routes them into a deliberation turn instead of auto-approving. Reuses the shell_mutation_reason tokenizer (shlex split, wrapper unwrap, git-subcommand extraction) for wrapper/quote/chain hardening. - CodeRabbit review fixes: annotate /recap slash command with -> None; show hook "timed out" status even when the hook produced output; make the streaming-block test helper fail loudly when the label is missing. - tests: cover destructive classification, the recap/visualize paths, and the hardened test helper.
Contributor
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 `@tests/ui_and_conv/test_live_view_notifications.py`:
- Around line 125-143: In
test_live_view_prints_hook_timeout_status_with_partial_output, the
monkeypatch.setattr for shell_console.print uses a lambda that accepts **kwargs
but doesn't use it; update the replacement callable used in monkeypatch.setattr
(in the test function
test_live_view_prints_hook_timeout_status_with_partial_output) to either remove
the unused kwargs (use lambda *args: printed.extend(args)) or rename it to
**_kwargs to silence the unused-kwargs warning so the printed list still
collects args from shell_console.print.
🪄 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 Plus
Run ID: 3b58d645-3f9d-4d83-9421-29dcf08616bb
📒 Files selected for processing (11)
src/pythinker_code/soul/permission.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/ui/shell/components/tool_execution.pysrc/pythinker_code/ui/shell/visualize/__init__.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pytests/core/test_permission_profiles.pytests/ui_and_conv/test_live_view_notifications.pytests/ui_and_conv/test_streaming_content_block.pytests/ui_and_conv/test_tui_render_snapshots.pytests/ui_and_conv/test_visualize_running_prompt.py
Contributor
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)
tests/ui_and_conv/test_live_view_notifications.py (1)
322-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
**_kwargsfor consistency with line 129.Line 129 was updated to silence the unused-kwargs warning by renaming
**kwargsto**_kwargs. This new test should follow the same pattern.♻️ Proposed fix
- live_view_module.console, "print", lambda *args, **kwargs: printed.extend(args) + live_view_module.console, "print", lambda *args, **_kwargs: printed.extend(args)🤖 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 `@tests/ui_and_conv/test_live_view_notifications.py` at line 322, The lambda used in the monkeypatch calling live_view_module.console.print should rename its kwargs parameter to _kwargs for consistency with the change on line 129; update the lambda signature from "lambda *args, **kwargs: printed.extend(args)" to use "lambda *args, **_kwargs: printed.extend(args)" so the unused-kwargs warning is silenced while keeping behavior identical.
🤖 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 `@tests/ui_and_conv/test_live_view_notifications.py`:
- Line 322: The lambda used in the monkeypatch calling
live_view_module.console.print should rename its kwargs parameter to _kwargs for
consistency with the change on line 129; update the lambda signature from
"lambda *args, **kwargs: printed.extend(args)" to use "lambda *args, **_kwargs:
printed.extend(args)" so the unused-kwargs warning is silenced while keeping
behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d4992c14-590e-4840-a215-712df277e0bb
📒 Files selected for processing (1)
tests/ui_and_conv/test_live_view_notifications.py
icon.svg and favicon.ico under web/static/brand are read at runtime by auth/browser_login_page to embed branding in the OAuth callback HTML, but the whole web/static tree is gitignored — so CI's clean checkout lacked them and test_openai_callback_html_* failed with FileNotFoundError (passed locally only because the files exist on disk). Force-add the two required assets so the feature and its tests work from a fresh clone.
Committing the brand assets makes web/static/ exist in CI for the first time, which previously un-skipped test_index_html_has_no_cache_header — but the built frontend (index.html, assets/) still isn't present, so GET / would 404. Guard on index.html existence instead, matching the 'web static assets not built' intent: runs where the app is built (local/prod), skips where only brand assets exist (CI).
This was referenced Jun 2, 2026
This was referenced Jun 10, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
/recap, turn recap display controls, and tests.Verification
uv run pytest tests/core/test_wire_message.py::test_wire_message_type_alias tests/hooks/test_integration.py::test_wire_resolved_callback_receives_hook_output tests/ui_and_conv/test_live_view_notifications.py::test_live_view_prints_resolved_hook_stdout -quv run pytest tests/core/test_config.py::test_default_config_dump tests/ui_and_conv/test_visualize_running_prompt.py::test_visualize_uses_prompt_live_view_when_prompt_session_and_steer_are_provided -quv run pytest tests_e2e/test_wire_protocol.py::test_initialize_handshake tests_e2e/test_wire_protocol.py::test_initialize_external_tool_conflict -qmake check && make testSummary by CodeRabbit
New Features
Improvements
Tests