Skip to content

Commit c4a46b0

Browse files
committed
fix(shell): reload MCP and model sessions cleanly
1 parent 158ee96 commit c4a46b0

6 files changed

Lines changed: 186 additions & 37 deletions

File tree

src/pythinker_code/app.py

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,22 @@ def session(self) -> Session:
467467
"""Get the Session instance."""
468468
return self._runtime.session
469469

470+
async def cleanup_runtime_resources(self) -> None:
471+
"""Cleanup per-CLI-instance resources without stopping persisted background tasks."""
472+
# Cancel the startup managed-model refresh task if it is still running
473+
# so it does not outlive this CLI instance across reloads.
474+
if self._bg_refresh_task is not None and not self._bg_refresh_task.done():
475+
self._bg_refresh_task.cancel()
476+
477+
# Cleanup MCP connections held by this instance's toolset. Background
478+
# task workers are persisted elsewhere and intentionally left alone.
479+
from pythinker_code.soul.toolset import PythinkerToolset
480+
481+
toolset = self._soul.agent.toolset
482+
if isinstance(toolset, PythinkerToolset):
483+
with contextlib.suppress(Exception):
484+
await toolset.cleanup()
485+
470486
async def shutdown_background_tasks(self) -> None:
471487
"""Kill active background tasks on exit, unless keep_alive_on_exit is configured.
472488
@@ -480,18 +496,7 @@ async def shutdown_background_tasks(self) -> None:
480496
store corruption must not propagate and replace the real exit code
481497
with a traceback.
482498
"""
483-
# Cancel the startup managed-model refresh task if it is still running
484-
# so it does not outlive the CLI process.
485-
if self._bg_refresh_task is not None and not self._bg_refresh_task.done():
486-
self._bg_refresh_task.cancel()
487-
488-
# Cleanup MCP connections held by the toolset
489-
from pythinker_code.soul.toolset import PythinkerToolset
490-
491-
toolset = self._soul.agent.toolset
492-
if isinstance(toolset, PythinkerToolset):
493-
with contextlib.suppress(Exception):
494-
await toolset.cleanup()
499+
await self.cleanup_runtime_resources()
495500

496501
bg_config = self._runtime.config.background
497502
if bg_config.keep_alive_on_exit:

src/pythinker_code/cli/__init__.py

Lines changed: 52 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import json
34
import os
45
from importlib import import_module
56
from pathlib import Path
@@ -173,6 +174,51 @@ class ExitCode:
173174
RETRYABLE = 75 # EX_TEMPFAIL from sysexits.h
174175

175176

177+
def _load_mcp_configs_from_cli_inputs(
178+
mcp_config_file: list[Path] | None,
179+
mcp_config: list[str] | None,
180+
) -> list[Any]:
181+
"""Load MCP config JSON from the current CLI inputs.
182+
183+
This intentionally re-resolves the default global MCP file on every call so
184+
`/reload` observes servers added after the process started.
185+
"""
186+
from .mcp import get_global_mcp_config_file
187+
188+
file_configs = list(mcp_config_file or [])
189+
raw_mcp_config = list(mcp_config or [])
190+
191+
# Use default MCP config file if no MCP config file is provided. Keep this
192+
# lookup live for reloads: the file may be created after process startup.
193+
if not file_configs:
194+
default_mcp_file = get_global_mcp_config_file()
195+
if default_mcp_file.exists():
196+
file_configs.append(default_mcp_file)
197+
198+
configs: list[Any] = []
199+
for conf in file_configs:
200+
try:
201+
configs.append(json.loads(conf.read_text(encoding="utf-8")))
202+
except json.JSONDecodeError as e:
203+
raise typer.BadParameter(
204+
f"Invalid JSON in MCP config file {conf}: {e}",
205+
param_hint="--mcp-config-file",
206+
) from e
207+
except OSError as e:
208+
raise typer.BadParameter(
209+
f"Cannot read MCP config file {conf}: {e}",
210+
param_hint="--mcp-config-file",
211+
) from e
212+
213+
for conf in raw_mcp_config:
214+
try:
215+
configs.append(json.loads(conf))
216+
except json.JSONDecodeError as e:
217+
raise typer.BadParameter(f"Invalid JSON: {e}", param_hint="--mcp-config") from e
218+
219+
return configs
220+
221+
176222
InputFormat = Literal["text", "stream-json"]
177223
OutputFormat = Literal["text", "stream-json"]
178224

@@ -502,7 +548,6 @@ def pythinker(
502548
"""Pythinker, your next CLI agent."""
503549
import asyncio
504550
import contextlib
505-
import json
506551

507552
from pythinker_code.utils.proctitle import init_process_name
508553

@@ -525,8 +570,6 @@ def pythinker(
525570
from pythinker_code.ui.shell.startup import ShellStartupProgress
526571
from pythinker_code.utils.logging import logger, open_original_stderr, redirect_stderr_to_logger
527572

528-
from .mcp import get_global_mcp_config_file
529-
530573
# Don't redirect stderr during argument parsing. Our stderr redirector
531574
# replaces fd=2 with a pipe, which would swallow Click/Typer startup errors.
532575
# Redirection is installed later, right before PythinkerCLI.create(), so that
@@ -648,25 +691,6 @@ def _emit_fatal_error(message: str) -> None:
648691
elif config_file is not None:
649692
config = config_file
650693

651-
file_configs = list(mcp_config_file or [])
652-
raw_mcp_config = list(mcp_config or [])
653-
654-
# Use default MCP config file if no MCP config is provided
655-
if not file_configs:
656-
default_mcp_file = get_global_mcp_config_file()
657-
if default_mcp_file.exists():
658-
file_configs.append(default_mcp_file)
659-
660-
try:
661-
mcp_configs = [json.loads(conf.read_text(encoding="utf-8")) for conf in file_configs]
662-
except json.JSONDecodeError as e:
663-
raise typer.BadParameter(f"Invalid JSON: {e}", param_hint="--mcp-config-file") from e
664-
665-
try:
666-
mcp_configs += [json.loads(conf) for conf in raw_mcp_config]
667-
except json.JSONDecodeError as e:
668-
raise typer.BadParameter(f"Invalid JSON: {e}", param_hint="--mcp-config") from e
669-
670694
# Honor --no-telemetry by exporting the env var before any subsystem (Sentry,
671695
# OTel, sink) reads it during PythinkerCLI.create.
672696
if no_telemetry:
@@ -748,6 +772,8 @@ async def _run(session_id: str | None, prefill_text: str | None = None) -> tuple
748772
if changed:
749773
session.save_state()
750774

775+
mcp_configs = _load_mcp_configs_from_cli_inputs(mcp_config_file, mcp_config)
776+
751777
# Redirect stderr *before* PythinkerCLI.create() so that MCP server
752778
# subprocesses (e.g. mcp-remote OAuth debug logs) write to the log
753779
# file instead of polluting the user's terminal. CLI argument
@@ -899,9 +925,11 @@ async def _run(session_id: str | None, prefill_text: str | None = None) -> tuple
899925
timeout=5,
900926
)
901927

902-
if not preserve_background_tasks:
928+
if preserve_background_tasks:
929+
await instance.cleanup_runtime_resources()
930+
else:
903931
await instance.shutdown_background_tasks()
904-
await instance.await_bg_tasks_shutdown()
932+
await instance.await_bg_tasks_shutdown()
905933

906934
return session, exit_code
907935
finally:

src/pythinker_code/ui/shell/setup.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,4 +223,4 @@ def reload(app: Shell, args: str):
223223
"""Reload configuration"""
224224
from pythinker_code.cli import Reload
225225

226-
raise Reload
226+
raise Reload()

src/pythinker_code/ui/shell/slash.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,15 @@ async def model(app: Shell, args: str):
355355
if model_changed and selected_model_cfg.provider == "managed:lm-studio":
356356
await _preload_lm_studio_model(selected_provider, selected_model_cfg.model)
357357

358+
if model_changed:
359+
current_session = soul.runtime.session
360+
session = await Session.create(current_session.work_dir)
361+
session.state.additional_dirs = list(current_session.state.additional_dirs)
362+
if session.state.additional_dirs:
363+
session.save_state()
364+
console.print(f"[{_t.success}]Starting fresh session for the new model...[/]")
365+
raise Reload(session_id=session.id)
366+
358367
raise Reload(session_id=soul.runtime.session.id)
359368

360369

tests/core/test_cli_reload.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
from __future__ import annotations
2+
3+
import json
4+
5+
from pythinker_code.cli import _load_mcp_configs_from_cli_inputs
6+
from pythinker_code.cli.mcp import get_global_mcp_config_file
7+
8+
9+
def test_load_mcp_configs_rechecks_default_file_between_reloads() -> None:
10+
"""Reload must see MCP servers added after process startup."""
11+
default_mcp_file = get_global_mcp_config_file()
12+
assert not default_mcp_file.exists()
13+
assert _load_mcp_configs_from_cli_inputs(None, None) == []
14+
15+
default_mcp_file.parent.mkdir(parents=True, exist_ok=True)
16+
expected = {
17+
"mcpServers": {"context7": {"url": "https://mcp.example.test", "transport": "http"}}
18+
}
19+
default_mcp_file.write_text(json.dumps(expected), encoding="utf-8")
20+
21+
assert _load_mcp_configs_from_cli_inputs(None, None) == [expected]

tests/ui_and_conv/test_shell_slash_commands.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,12 @@
99
from unittest.mock import Mock
1010

1111
import pytest
12+
from pydantic import SecretStr
1213
from pythinker_core.message import Message
1314
from pythinker_host.path import HostPath
1415

1516
from pythinker_code.cli import Reload
17+
from pythinker_code.config import Config, LLMModel, LLMProvider
1618
from pythinker_code.session import Session
1719
from pythinker_code.subagents.models import AgentTypeDefinition, ToolPolicy
1820
from pythinker_code.ui.shell.slash import ShellSlashCmdFunc, shell_mode_registry
@@ -89,6 +91,90 @@ def test_blackbox_style_slash_aliases_are_registered() -> None:
8991
assert command.name == canonical
9092

9193

94+
async def test_model_switch_starts_fresh_session(monkeypatch: pytest.MonkeyPatch) -> None:
95+
"""Changing models should reload into a new session so old context is not reused."""
96+
from pythinker_code.soul.pythinkersoul import PythinkerSoul
97+
from pythinker_code.ui.shell import model_picker
98+
from pythinker_code.ui.shell import slash as shell_slash
99+
100+
config = Config(
101+
is_from_default_location=True,
102+
default_model="model-a",
103+
providers={
104+
"test-provider": LLMProvider(
105+
type="pythinker",
106+
base_url="https://example.test",
107+
api_key=SecretStr("test-key"),
108+
)
109+
},
110+
models={
111+
"model-a": LLMModel(
112+
provider="test-provider",
113+
model="model-a",
114+
max_context_size=100_000,
115+
),
116+
"model-b": LLMModel(
117+
provider="test-provider",
118+
model="model-b",
119+
max_context_size=100_000,
120+
),
121+
},
122+
)
123+
current_session = SimpleNamespace(
124+
id="current-session",
125+
work_dir=HostPath("/tmp/work"),
126+
state=SimpleNamespace(additional_dirs=["/extra"]),
127+
)
128+
mock_soul = Mock(spec=PythinkerSoul)
129+
mock_soul.runtime = SimpleNamespace(
130+
config=config,
131+
llm=SimpleNamespace(model_config=config.models["model-a"]),
132+
session=current_session,
133+
)
134+
mock_soul.thinking_effort = "off"
135+
mock_soul.thinking = False
136+
shell = SimpleNamespace(soul=mock_soul)
137+
138+
class _ModelPicker:
139+
def __init__(self, *args: Any, **kwargs: Any) -> None:
140+
pass
141+
142+
async def run(self) -> str:
143+
return "model-b"
144+
145+
saved_configs: list[Config] = []
146+
147+
async def fake_refresh_managed_models(_config: Config) -> None:
148+
return None
149+
150+
fresh_session = SimpleNamespace(
151+
id="fresh-session",
152+
state=SimpleNamespace(additional_dirs=[]),
153+
save_state=Mock(),
154+
)
155+
156+
async def fake_create_session(work_dir: HostPath) -> SimpleNamespace:
157+
assert work_dir == current_session.work_dir
158+
return fresh_session
159+
160+
monkeypatch.setattr(shell_slash, "refresh_managed_models", fake_refresh_managed_models)
161+
monkeypatch.setattr(model_picker, "ModelPickerApp", _ModelPicker)
162+
monkeypatch.setattr(shell_slash, "load_config", lambda: config.model_copy(deep=True))
163+
monkeypatch.setattr(shell_slash, "save_config", saved_configs.append)
164+
monkeypatch.setattr(shell_slash.Session, "create", fake_create_session)
165+
monkeypatch.setattr(shell_slash.console, "print", Mock())
166+
167+
cmd = shell_slash_registry.find_command("model")
168+
assert cmd is not None
169+
with pytest.raises(Reload) as exc_info:
170+
await _invoke_slash_command(cmd, shell)
171+
172+
assert exc_info.value.session_id == "fresh-session"
173+
assert fresh_session.state.additional_dirs == ["/extra"]
174+
fresh_session.save_state.assert_called_once_with()
175+
assert saved_configs[-1].default_model == "model-b"
176+
177+
92178
async def test_mcp_slash_persists_only_final_snapshot(
93179
monkeypatch: pytest.MonkeyPatch, capsys: Any
94180
) -> None:

0 commit comments

Comments
 (0)