fix(shell): reload MCP and model sessions cleanly - #51
Merged
Conversation
web/static/ is a build output (scripts/build_web.py rmtree's + repopulates it from the vite build, whose brand files come from web/public/brand). The two brand assets are force-committed only so the test job — which doesn't run the web build — has the files the OAuth callback reads. Add a guard asserting those committed copies stay byte-identical to web/public/brand, so a brand-source change can't silently leave the branding tests validating a stale fixture.
The OAuth callback page embeds icon.svg/favicon.ico as data URIs. These are build outputs (web/static) and normally always present, but a broken build shouldn't crash login over a cosmetic asset. Catch OSError in the data-uri helper, log a warning, and embed an empty source so the callback still renders. Add a test for the degraded path.
Under auto_deliberate, an irreversible shell action (rm -rf, git push --force, git reset --hard, dd, truncate) is bounced once before running -- even under auto/yolo -- so the agent weighs alternatives first. One-shot: the identical re-issue runs, so deliberation never permanently whitelists the command. ApprovalResult gains a deliberation variant whose feedback is not framed as a user rejection.
Pure helpers for the Shift+Tab thinking feature: next_thinking_level() cycles off->minimal->low->medium->high->xhigh->off, and thinking_frame_color/ thinking_frame_style map each level to a prompt border color (grey->blue-> violet->purple) per theme. Keymap + prompt-handler wiring follows.
Restructure the active-work shimmer into a four-phase loop: a wave sweeps right-to-left, splashes outward from the middle, sweeps back left-to-right, splashes again, then repeats. Replaces the previous single-direction repeating sweep. Stays purely time-derived so the prompt, activity tree, and pinned-todo renderers animate in sync.
Recolor the active-work shimmer: highlight is now silver (#D8DCE2) instead of violet, and the verb-spinner/pinned-todo base is a muted orange-yellow (#D49E5A) instead of golden amber. Point palette tests at the motion constants so future shade tweaks don't churn test literals.
The per-turn '※ recap:' line quoted the assistant's first sentence,
which is always an intent preamble ('I'll start by gathering...') rather
than what the turn accomplished. Add an outcome-sentence heuristic that
skips intent/offer/question/path-noise lines and prefers the closing
summary, and append factual deltas (N files changed · M steps). Wire the
live view to count files changed per turn from diff display blocks.
Also improve /recap: session bullets lead with the session outcome
instead of the first user message, and a single short no-op session is
reported plainly as a light day rather than padded into a bullet list.
Make "minimal" a first-class ThinkingEffort: OpenAI round-trips it natively, the Pythinker provider preserves it instead of collapsing to low, Anthropic clamps it down to its floor (low) rather than up to high, and Gemini maps it to its lowest thinking level/budget.
… setting Add a persisted default_thinking_effort alongside the legacy default_thinking bool, with the effort string as the source of truth (falling back to the bool only for pre-existing configs). create_llm now takes an explicit effort, clamps it to model capabilities, preserves levels like xhigh/minimal instead of collapsing every enabled request to high, and threads the effort through the CLI, ACP, review, web API, subagents, login flows, setup, and the /model and /thinking selectors. Always-thinking models surface native reasoning instead of an effort dial.
…y effort Repurpose Shift+Tab from plan-mode toggle to a thinking-effort cycle over the model's available levels, with a toast and telemetry. The prompt separator and bottom-toolbar label now reflect the active effort (or "native reasoning"), and the effort color ramp moves from a grey/violet scale to a cool-to-warm slate→pink ramp. Tips and keybinding help updated to match.
…tion Add an "auto_deliberate" ask-user policy: in auto mode, instead of silently dismissing AskUserQuestion, run an independent tool-less advisor that blind-ranks the agent's own options and hand the verdict back so the agent self-decides. Destructive auto-approved actions are bounced once for deliberation via a tool-agnostic classifier, and the auto-mode prompt invites the tool at genuine forks under this policy.
Render report blocks as a rounded, padded Rich panel (a standalone reading surface) with hanging-indented wrapped locations, and keep a one-row seam when a report fence follows streamed prose. Ask-question cards gain blank-row separation between header and questions.
Add a prompt-scoped refresh loop so the pinned-status shimmer stays frame-based when wire events are sparse (e.g. a long-running subagent), honoring reduced-motion with a slower interval.
Map each login/logout entry to the managed provider keys that signal it is configured, so /login and /logout report real status and a bare /logout opens a selector over only the logged-in providers (with a single OpenAI entry covering both OAuth and API-key credentials).
Two RunAgents calls differing only in base_prompt produced the same fingerprint; include it so distinct launches are not deduplicated.
Update rendering tests to match the committed compaction/recap seam behavior: compaction commits a leading blank row plus block, and turn recaps are framed by blank rows.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds thinking-effort plumbing and UI changes, factors MCP config loading into a helper that re-resolves the default file, adds per-instance runtime cleanup integrated into shutdown flows, makes /model create a fresh session for managed:lm-studio switches, and adds tests plus a changelog entry. ChangesThinking effort and lifecycle management updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 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/app.py`:
- Around line 481-484: The current contextlib.suppress around awaiting
toolset.cleanup() hides all exceptions; change it to catch Exception as e and
log the failure before continuing so MCP transport cleanup errors are visible.
Locate the block using self._soul.agent.toolset and PythinkerToolset and replace
the contextlib.suppress(...) with an async-aware try/except that awaits
toolset.cleanup() and logs the exception (using the module logger or existing
logging facility, e.g., logger.exception or logger.error with exception info)
while allowing execution to continue.
In `@src/pythinker_code/ui/shell/slash.py`:
- Around line 360-363: The synchronous call to session.save_state() inside the
async model handler blocks the event loop; change the persistence call to run
off the loop (e.g., call await asyncio.to_thread(session.save_state) when you
create the new session in the model path) or convert Session.save_state() and
its internals (load_session_state/save_session_state/atomic_json_write) to async
and internally thread the blocking I/O. Specifically, locate the Session.create
usage and replace the direct session.save_state() invocation with an awaited
asyncio.to_thread wrapper (or update Session.save_state to an async method and
await it) so file write/flush/fsync happen off the event loop.
In `@tests/core/test_cli_reload.py`:
- Around line 11-21: The test relies on the real global MCP config path; patch
get_global_mcp_config_file() inside tests/core/test_cli_reload.py to return a
file under the pytest tmp_path (or tmp_path_factory) so the test writes and
reads only within the temporary directory. Update the test to monkeypatch or
inject a stub for get_global_mcp_config_file() before calling
_load_mcp_configs_from_cli_inputs(None, None), create the parent dir and write
the expected JSON into that stubbed path, and then assert the loader returns the
expected result; ensure you reference get_global_mcp_config_file and
_load_mcp_configs_from_cli_inputs in your change.
🪄 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: a3efeec4-68a0-482f-998b-2509a1f9d46e
📒 Files selected for processing (6)
src/pythinker_code/app.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/ui/shell/setup.pysrc/pythinker_code/ui/shell/slash.pytests/core/test_cli_reload.pytests/ui_and_conv/test_shell_slash_commands.py
5 tasks
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
/reloadsees servers added after the process started./modelchanges the active model, while preserving added workspace directories./reloadto raise aReload()instance explicitly.Root cause analysis
The shell reload loop parsed the default/global MCP config once during initial CLI argument handling and then reused that captured
mcp_configslist for every same-process reload. If a user ranpythinker mcp add ...while the shell was open,/reloadrebuilt the agent from stale in-memory MCP config, so new MCP tools were unavailable until a full terminal restart reparsed the global MCP file.Reload control flow also preserved background tasks by skipping the shutdown path entirely. That avoided killing user tasks, but it also skipped per-instance cleanup for MCP transports and the managed-model refresh task. The fix separates reusable runtime cleanup from background-task termination.
For model switching,
/modelsaved the new default model and reloaded the same session, preserving the prior context. That allowed old-context turns to carry into a different model. The fix creates a new session on model changes so the next agent starts with fresh context.Verification
uv run pytest tests/core/test_cli_reload.py tests/ui_and_conv/test_shell_slash_commands.py -quv run ruff check src/pythinker_code/cli/__init__.py src/pythinker_code/app.py src/pythinker_code/ui/shell/slash.py src/pythinker_code/ui/shell/setup.py tests/core/test_cli_reload.py tests/ui_and_conv/test_shell_slash_commands.pyuv run pyright src/pythinker_code/cli/__init__.py src/pythinker_code/app.py src/pythinker_code/ui/shell/slash.py src/pythinker_code/ui/shell/setup.pymake checkmake testSummary by CodeRabbit
New Features
Improvements
/modelnow starts a fresh session when switching models to avoid cross-model context carry-over.Tests