From 59549d2b3b4f414f42406e42c4130ebe63adc088 Mon Sep 17 00:00:00 2001 From: Daniele Date: Mon, 10 Aug 2026 13:11:02 +0100 Subject: [PATCH] fix(mcp): install and expose a global connector's deps where they are needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves of the same failure, found debugging a marketplace connector that logged "connected — 6 tool(s)" while every call died on a missing module. The verify ran as a bare `sh -c` and inherited nothing, so a python connector was rejected by its own verify for a dependency installed one directory away — `global_enable` installs before it verifies, so the deps were provably there at the moment the check denied them, and the row ended up disabled. Only connectors that bother to declare a verify could hit it. `verify_env` now builds the verify's environment in one place and derives PYTHONPATH from the workdir, which is already the connector dir in both targets; `or_insert`, so a value the form declares still wins. The global branch of the reinstall refresh restarted the server without ever installing its deps: `ensure_installed_host` was reachable from `global_enable` alone, so a marketplace Update that adds a requirements.txt landed the file and brought the connector back exactly as broken. It now runs once per connector folder before the restart loop, best-effort. The per-user branch had always reinstalled, which is why nothing with scope=user ever showed the bug. Known gap, deliberate: POST /api/mcp/test shares run_verify but not the install, so testing a python connector never enabled on the box still fails on missing deps. Making a "try it" button write to disk for minutes is the worse trade. --- CLAUDE.md | 4 + crates/skald-core/src/mcp/verify.rs | 113 ++++++++++++++++++++--- crates/skald-core/src/skald/accessors.rs | 40 +++++++- 3 files changed, 138 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 815d612..dc91036 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,6 +217,10 @@ MCP servers are surfaced to users as **"Connectors"** (UI naming; `mcp`/schema s **Dependency reconciler (`mcp::install::ensure_installed`).** Copying a local-script connector's files into a container never installed its deps. `ensure_installed` closes that: a **content-hash reconciler** keyed on the connector's *source* files (not a version string) that, when the hash changed, re-copies the files and installs deps inside the container — `npm ci --omit=dev` (node, from `package.json`) and/or `pip install --target .pydeps` (python, from `requirements.txt`, put on the server's `PYTHONPATH` by `user_row_spec`). Runs at activation **and** on every per-user startup path (`UserContext` build, remount) via `mcp::prepare_local_connector`, so a fresh container installs from scratch, an updated connector re-installs, and an unchanged one is a hash-match no-op. Deps are therefore **never vendored** — connectors ship `package.json`/`requirements.txt`, not `node_modules/`. Authoring contract for connectors lives in `CONNECTOR_MANIFEST_GUIDE.md` (repo root). +**The host half has no reconciler, so its call sites are the contract.** A `global` connector runs in the Skald process, not a container, and `ensure_installed_host` is not hash-guarded — it leans on `pip`/`npm` being idempotent, which is only safe as long as *every* path that lands new files also calls it. There are two: `global_enable` (the admin saving a connector's config) and, since it was missing, the global branch of `Skald::refresh_connector_after_reinstall`. Without the second, a marketplace **Update** that *adds* a `requirements.txt` copied the file and restarted the server without installing anything — the connector came back exactly as broken, and the only cure was re-saving its config. Note what that asymmetry cost: the per-user branch of the same function had always reinstalled (`prepare_local_connector`), so the bug was invisible on anything `scope: user`. + +**The verify runs with `.pydeps` on `PYTHONPATH`, and must** (`mcp::verify::verify_env`). Only the *server* launch used to get that path (`global_row_spec` / `user_row_spec`); the verify is a bare `sh -c` inheriting nothing, so a python connector was rejected **by its own verify** for a dependency sitting installed one directory away — and `global_enable` installs *before* it verifies, so the deps were provably there at the moment the check denied them. The failure selected for well-written connectors: declaring no `verify` meant never meeting it. The workdir *is* the connector dir in both targets, so the path is derived, not plumbed, and set with `or_insert` — an explicit `PYTHONPATH` from the form is the author's. One gap left deliberately: `POST /api/mcp/test` (the Test button) shares `run_verify` but **not** `ensure_installed_host`, so testing a python connector that was never enabled on this box still fails on the missing deps. Making a "try it" button write to disk for minutes is the worse trade; enable first. + **Connector versioning.** `mcp_catalog` carries `version` (INTEGER — the update-comparison key), `version_string` (semver, display) and `version_release_date` (ISO, display), snapshotted from the feed on install. The marketplace list computes `update_available` = feed `version` > installed `version` (strict) and surfaces it as an "Update" button (`marketplace.js`). The integer is the UI signal; the actual re-install trigger is the reconciler's content-hash. ### OAuth per-user connectors (blueprint §15 — copy-paste flow) diff --git a/crates/skald-core/src/mcp/verify.rs b/crates/skald-core/src/mcp/verify.rs index 94d15f2..0bc98b2 100644 --- a/crates/skald-core/src/mcp/verify.rs +++ b/crates/skald-core/src/mcp/verify.rs @@ -281,40 +281,77 @@ fn build_command( ) -> tokio::process::Command { match target { VerifyTarget::Container { container, workdir } => { + let vars = verify_env(workdir, env_values, secret_values); let mut c = tokio::process::Command::new("docker"); c.arg("exec").arg("-w").arg(workdir); - inject_env_flags(&mut c, env_values, secret_values); + inject_env_flags(&mut c, &vars); c.arg(container); c.arg("sh").arg("-c").arg(resolved); c } VerifyTarget::Host { workdir } => { + let vars = verify_env(workdir, env_values, secret_values); let mut c = tokio::process::Command::new("sh"); c.arg("-c").arg(resolved).current_dir(workdir); - inject_env_vars(&mut c, env_values, secret_values); + inject_env_vars(&mut c, &vars); c } } } -/// Adds `-e KEY=VALUE` flags for `docker exec`, for both env and secret values. -fn inject_env_flags( - cmd: &mut tokio::process::Command, +/// The full environment for a verify run: the form's env + secret values, plus a +/// derived `PYTHONPATH` pointing at the connector's own `.pydeps`. +/// +/// Without that last part a well-written python connector is **rejected by its own +/// verify**. Its dependencies are installed under `/.pydeps` +/// ([`install::ensure_installed`] / [`install::ensure_installed_host`]) and only the +/// *server* launch ever put them on `PYTHONPATH` (`mcp::global_row_spec` / +/// `user_row_spec`); the verify runs as a bare `sh -c` and inherits nothing. In +/// `global_enable` the install runs *before* the verify, so the deps are sitting +/// installed in the very directory the verify then declares them missing from — and +/// the row ends up `enabled = 0`. Only connectors that bother to declare a `verify` +/// hit it. +/// +/// The workdir *is* the connector dir in both targets (`global_verify_workdir` and +/// `prepare_user_verify_workdir`), so the path needs no new parameter. Node needs no +/// equivalent: `node_modules/` beside the entry file resolves from the cwd, which is +/// that same workdir. +/// +/// Set only when the form did not declare one — `or_insert`, not `insert`, mirroring +/// `global_row_spec`: an explicit `PYTHONPATH` is the connector author's call. Adding +/// it unconditionally is harmless for a node or remote connector, since nothing there +/// reads it. +fn verify_env( + workdir: &Path, env: &HashMap, secret: &HashMap, -) { - for (k, v) in env.iter().chain(secret.iter()) { +) -> Vec<(String, String)> { + let mut vars: Vec<(String, String)> = env + .iter() + .chain(secret.iter()) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + if !vars.iter().any(|(k, _)| k == PYTHONPATH_VAR) { + let pydeps = workdir.join(super::install::PYDEPS_DIR); + vars.push((PYTHONPATH_VAR.to_string(), pydeps.to_string_lossy().into_owned())); + } + vars +} + +/// The variable [`verify_env`] derives. Named so the "don't override the form's own +/// value" check and the value it would set cannot drift apart. +const PYTHONPATH_VAR: &str = "PYTHONPATH"; + +/// Adds `-e KEY=VALUE` flags for `docker exec`. +fn inject_env_flags(cmd: &mut tokio::process::Command, vars: &[(String, String)]) { + for (k, v) in vars { cmd.arg("-e").arg(format!("{k}={v}")); } } /// Sets environment variables for a host `sh -c` process. -fn inject_env_vars( - cmd: &mut tokio::process::Command, - env: &HashMap, - secret: &HashMap, -) { - for (k, v) in env.iter().chain(secret.iter()) { +fn inject_env_vars(cmd: &mut tokio::process::Command, vars: &[(String, String)]) { + for (k, v) in vars { cmd.env(k, v); } } @@ -393,7 +430,7 @@ mod tests { } #[test] - fn container_command_without_env_has_no_flags() { + fn container_command_without_env_still_carries_pythonpath() { let target = VerifyTarget::Container { container: "skald-user1", workdir: Path::new("/root/.skald/mcp/x"), @@ -404,7 +441,53 @@ mod tests { .get_args() .map(|a| a.to_string_lossy().into_owned()) .collect(); - assert_eq!(args, ["exec", "-w", "/root/.skald/mcp/x", "skald-user1", "sh", "-c", "true"]); + assert_eq!( + args, + [ + "exec", "-w", "/root/.skald/mcp/x", + "-e", "PYTHONPATH=/root/.skald/mcp/x/.pydeps", + "skald-user1", "sh", "-c", "true", + ] + ); + } + + #[test] + fn verify_env_derives_pythonpath_from_the_workdir() { + let vars = verify_env( + Path::new("/srv/skald/connectors/gmaps"), + &m(&[("REGION", "eu")]), + &m(&[("KEY", "abc")]), + ); + let pp = vars.iter().find(|(k, _)| k == "PYTHONPATH").expect("PYTHONPATH derived"); + assert_eq!(pp.1, "/srv/skald/connectors/gmaps/.pydeps"); + // The form's own values are untouched. + assert!(vars.iter().any(|(k, v)| k == "REGION" && v == "eu")); + assert!(vars.iter().any(|(k, v)| k == "KEY" && v == "abc")); + } + + #[test] + fn verify_env_does_not_override_a_declared_pythonpath() { + let vars = verify_env( + Path::new("/srv/skald/connectors/gmaps"), + &m(&[("PYTHONPATH", "/opt/vendored")]), + &HashMap::new(), + ); + let pps: Vec<&String> = vars.iter().filter(|(k, _)| k == "PYTHONPATH").map(|(_, v)| v).collect(); + assert_eq!(pps, ["/opt/vendored"], "the connector's own value must win, and only once"); + } + + #[test] + fn host_command_runs_in_the_workdir_with_pythonpath() { + let target = VerifyTarget::Host { workdir: Path::new("/srv/skald/connectors/gmaps") }; + let cmd = build_command(&target, "python3 verify.py", &HashMap::new(), &HashMap::new()); + let std = cmd.as_std(); + assert_eq!(std.get_current_dir(), Some(Path::new("/srv/skald/connectors/gmaps"))); + let pp = std + .get_envs() + .find(|(k, _)| *k == std::ffi::OsStr::new("PYTHONPATH")) + .and_then(|(_, v)| v) + .expect("PYTHONPATH set"); + assert_eq!(pp, std::ffi::OsStr::new("/srv/skald/connectors/gmaps/.pydeps")); } #[test] diff --git a/crates/skald-core/src/skald/accessors.rs b/crates/skald-core/src/skald/accessors.rs index 40ba785..0f311ba 100644 --- a/crates/skald-core/src/skald/accessors.rs +++ b/crates/skald-core/src/skald/accessors.rs @@ -243,9 +243,11 @@ impl Skald { /// without a re-login — the reinstall counterpart of the §6/§7 remount helpers. /// The reinstall has already rewritten `mcp_catalog`; this reconnects what runs: /// - /// - **Global runtime**: for each *enabled* `mcp_global_servers` row snapshotting - /// this catalog entry, re-snapshot its `description` from the catalog and restart - /// it, so the running server's in-RAM description (and code) catches up. + /// - **Global runtime**: install the connector's declared dependencies on the host + /// (`ensure_installed_host`, once per folder), then for each *enabled* + /// `mcp_global_servers` row snapshotting this catalog entry, re-snapshot its + /// `description` from the catalog and restart it, so the running server's in-RAM + /// description (and code) catches up. /// - **Per-user runtimes**: for each live user who has this connector *startable*, /// re-copy its files/deps into the container (`prepare_local_connector` — a hash /// no-op when the source is unchanged) and restart that one server. The rebuilt @@ -267,7 +269,37 @@ impl Skald { // 1. Global runtime. if let Ok(globals) = crate::db::mcp_global_servers::all_enabled(self.db()).await { - for g in globals.iter().filter(|g| g.catalog_name.as_deref() == Some(catalog_name)) { + let live: Vec<_> = globals + .iter() + .filter(|g| g.catalog_name.as_deref() == Some(catalog_name)) + .collect(); + + // Dependencies before code. A global connector runs on the host, where + // nothing reconciles it the way the container reconciler does below, and + // `ensure_installed_host` was otherwise reachable from `global_enable` + // alone — so an Update that *adds* a `requirements.txt` landed the file, + // restarted the server, and never installed what it declared: the + // connector came back exactly as broken as before, curable only by + // re-saving its config from the UI. + // + // Once per connector folder rather than per row: the deps live beside the + // files, so two runtime names snapshotting one catalog entry share them. + // Not hash-guarded, unlike the per-user `ensure_installed` — it leans on + // `pip`/`npm` being idempotent, so a no-change reinstall pays one fast + // satisfied-requirements pass. Best-effort like the rest of this function. + if !live.is_empty() && entry.source == "local_script" { + match entry.script_path.as_deref().map(crate::mcp::split_script_path) { + Some(Ok((folder, _))) => { + if let Err(e) = crate::mcp::ensure_installed_host(folder).await { + tracing::warn!(connector = %catalog_name, error = %e, "reinstall refresh: global dependency install failed"); + } + } + Some(Err(e)) => tracing::warn!(connector = %catalog_name, error = %e, "reinstall refresh: unusable script_path, skipping dependency install"), + None => tracing::warn!(connector = %catalog_name, "reinstall refresh: local_script entry has no script_path, skipping dependency install"), + } + } + + for g in live { if let Err(e) = crate::db::mcp_global_servers::set_description(self.db(), g.id, entry.description.as_deref()).await { tracing::warn!(connector = %catalog_name, error = %e, "reinstall refresh: failed to update global description"); continue;