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)

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 `<connector-dir>/.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<String, String>,
     secret: &HashMap<String, String>,
-) {
-    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<String, String>,
-    secret: &HashMap<String, String>,
-) {
-    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]

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;