Skip to content

Commit 1a5b9ee

Browse files
mcp: activate the per-session venv for child processes (#315)
## What Sessions now export venv activation to the worker and any child it spawns: `VIRTUAL_ENV` points at the per-session venv and its `bin` is prepended to `PATH`. ## Why Fixes #314. The worker was launched with the venv's `bin/python`, so `import` resolved from the venv, but the child inherited the server's `PATH` with no `VIRTUAL_ENV`. An in-session `pip install` (or any `subprocess` that resolves a tool by name) hit the host instead of the session venv, so the writable-venv promise only held through `python -m pip`. This is the promise that justified removing the bring-your-own interpreter in #312, so it should be fully true. ## Changes - `create_venv` returns the venv directory; `venv_env` builds the activation env (`VIRTUAL_ENV` + prepended `PATH`). - `PythonSession::start` and the `repl` CLI apply it; the `#[cfg(test)] spawn_command` seam passes an empty env, so unit tests stay venv-free. ## Tests - New passthru test `mcp.tests.sessionVenv`: runs `ix-mcp eval '__import__("shutil").which("pip")'` and asserts the resolved `pip` lives under the session `.venv/bin`. Without activation this returns the host pip or `None`, so the test actually distinguishes fixed from broken. - `cargo test -p ix-mcp` (4 pass), `cargo clippy` clean, `nix run .#lint` clean, `nix build .#mcp.tests.sessionVenv` passes.
1 parent 28d466e commit 1a5b9ee

2 files changed

Lines changed: 71 additions & 16 deletions

File tree

packages/mcp/default.nix

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,29 @@ let
4141
4242
mkdir -p "$out"
4343
'';
44+
sessionVenv =
45+
pkgs.runCommand "ix-mcp-session-venv"
46+
{
47+
nativeBuildInputs = [ package ];
48+
strictDeps = true;
49+
}
50+
''
51+
export HOME=$TMPDIR/home
52+
mkdir -p "$HOME"
53+
54+
# A session must activate its venv for child processes so an in-session
55+
# `pip` resolves the per-session venv rather than the host. Without
56+
# activation, `shutil.which("pip")` misses the venv bin on PATH and
57+
# returns the host pip or None.
58+
ix-mcp eval '__import__("shutil").which("pip")' >stdout 2>stderr
59+
if ! grep -q '/[.]venv/bin/pip' stdout; then
60+
echo "in-session pip did not resolve to the venv:" >&2
61+
cat stdout >&2
62+
exit 1
63+
fi
64+
65+
mkdir -p "$out"
66+
'';
4467
in
4568
package.overrideAttrs (old: {
4669
passthru =
@@ -49,7 +72,7 @@ package.overrideAttrs (old: {
4972
// {
5073
inherit unwrapped;
5174
tests = (unwrapped.passthru.tests or { }) // {
52-
inherit replDefault;
75+
inherit replDefault sessionVenv;
5376
};
5477
};
5578
})

packages/mcp/src/main.rs

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::{
22
collections::HashMap,
3+
ffi::OsString,
34
fs,
45
io::{BufRead, BufReader, Write},
56
path::{Path, PathBuf},
@@ -238,28 +239,32 @@ struct PythonSession {
238239

239240
impl PythonSession {
240241
/// Start a session on the pinned interpreter. Each session gets its own
241-
/// writable venv so an agent can `pip install` into it without mutating the
242-
/// read-only store interpreter.
242+
/// writable venv, activated for the worker and the children it spawns, so
243+
/// an agent can `pip install` into it without mutating the read-only store
244+
/// interpreter.
243245
fn start(id: String, cwd: Option<PathBuf>) -> Result<Self> {
244246
let temp_dir = session_temp_dir(&id)?;
245-
let venv_python = create_venv(temp_dir.path())?;
246-
Self::spawn(id, temp_dir, vec![venv_python], cwd)
247+
let venv_dir = create_venv(temp_dir.path())?;
248+
let python = venv_dir.join("bin/python").display().to_string();
249+
let env = venv_env(&venv_dir)?;
250+
Self::spawn(id, temp_dir, vec![python], cwd, env)
247251
}
248252

249-
/// Spawn a worker against an explicit command, skipping the venv build.
250-
/// Tests use this to inject a bare interpreter or a fake worker; production
251-
/// always pins the interpreter through `start`.
253+
/// Spawn a worker against an explicit command, skipping the venv build and
254+
/// activation. Tests use this to inject a bare interpreter or a fake worker;
255+
/// production always pins the interpreter through `start`.
252256
#[cfg(test)]
253257
fn spawn_command(id: String, command: Vec<String>, cwd: Option<PathBuf>) -> Result<Self> {
254258
let temp_dir = session_temp_dir(&id)?;
255-
Self::spawn(id, temp_dir, command, cwd)
259+
Self::spawn(id, temp_dir, command, cwd, Vec::new())
256260
}
257261

258262
fn spawn(
259263
id: String,
260264
temp_dir: TempDir,
261265
command: Vec<String>,
262266
cwd: Option<PathBuf>,
267+
env: Vec<(OsString, OsString)>,
263268
) -> Result<Self> {
264269
let worker_path = temp_dir.path().join("worker.py");
265270
fs::write(&worker_path, WORKER_SOURCE).context("failed to write Python worker")?;
@@ -271,6 +276,7 @@ impl PythonSession {
271276
.args(&command[1..])
272277
.arg(&worker_path)
273278
.current_dir(cwd.as_deref().unwrap_or_else(|| Path::new(".")))
279+
.envs(env)
274280
.stdin(Stdio::piped())
275281
.stdout(Stdio::piped())
276282
// Worker stderr is not part of the JSON protocol. Normal Python
@@ -367,13 +373,13 @@ fn session_temp_dir(id: &str) -> Result<TempDir> {
367373
.context("failed to create Python session directory")
368374
}
369375

370-
/// Build a writable venv from the pinned interpreter and return its `python`.
376+
/// Build a writable venv from the pinned interpreter and return its directory.
371377
/// The interpreter is fixed at `IX_MCP_PYTHON`, which the Nix wrapper sets. A
372378
/// missing pin is a hard error rather than an ambient-`python3` fallback, so a
373379
/// wrapper regression fails loudly instead of silently running whatever is on
374380
/// `PATH`. The venv gives each session a writable site-packages with no
375381
/// per-call interpreter choice.
376-
fn create_venv(temp_dir: &Path) -> Result<String> {
382+
fn create_venv(temp_dir: &Path) -> Result<PathBuf> {
377383
let python = std::env::var("IX_MCP_PYTHON")
378384
.context("IX_MCP_PYTHON is unset; run ix-mcp via its Nix wrapper, which pins the interpreter")?;
379385
let venv_dir = temp_dir.join(".venv");
@@ -385,7 +391,29 @@ fn create_venv(temp_dir: &Path) -> Result<String> {
385391
if !status.success() {
386392
bail!("Python environment command exited with {status}");
387393
}
388-
Ok(venv_dir.join("bin/python").display().to_string())
394+
Ok(venv_dir)
395+
}
396+
397+
/// Activation environment for a child running in `venv_dir`: point
398+
/// `VIRTUAL_ENV` at the venv and prepend its `bin` to `PATH`. Running the
399+
/// venv's own `python` already resolves the venv site-packages, but a child the
400+
/// session spawns (an in-session `pip`, or any `subprocess` that finds a tool
401+
/// by name) would otherwise hit the host `PATH`. This makes the writable-venv
402+
/// promise hold for those too.
403+
fn venv_env(venv_dir: &Path) -> Result<Vec<(OsString, OsString)>> {
404+
let bin = venv_dir.join("bin");
405+
let path = match std::env::var_os("PATH") {
406+
Some(existing) => {
407+
let mut entries = vec![bin];
408+
entries.extend(std::env::split_paths(&existing));
409+
std::env::join_paths(entries).context("failed to compose venv PATH")?
410+
}
411+
None => bin.into_os_string(),
412+
};
413+
Ok(vec![
414+
("VIRTUAL_ENV".into(), venv_dir.as_os_str().to_owned()),
415+
("PATH".into(), path),
416+
])
389417
}
390418

391419
fn format_worker_response(response: &Value) -> String {
@@ -442,14 +470,18 @@ async fn main() -> Result<ExitCode> {
442470
.prefix("ix-mcp-python-repl-")
443471
.tempdir()
444472
.context("failed to create Python REPL directory")?;
445-
// The venv python lives inside temp_dir, so keep temp_dir bound
446-
// until the interactive process exits.
447-
let venv_python = create_venv(temp_dir.path())?;
473+
// The venv lives inside temp_dir, so keep temp_dir bound until the
474+
// interactive process exits.
475+
let venv_dir = create_venv(temp_dir.path())?;
476+
let venv_python = venv_dir.join("bin/python");
448477
let status = Command::new(&venv_python)
449478
.arg("-i")
450479
.current_dir(cli.cwd.as_deref().unwrap_or_else(|| Path::new(".")))
480+
.envs(venv_env(&venv_dir)?)
451481
.status()
452-
.with_context(|| format!("failed to start Python REPL command {venv_python}"))?;
482+
.with_context(|| {
483+
format!("failed to start Python REPL command {}", venv_python.display())
484+
})?;
453485
let code = status
454486
.code()
455487
.and_then(|code| u8::try_from(code).ok())

0 commit comments

Comments
 (0)