diff --git a/Cargo.lock b/Cargo.lock index f6ec97e..3a7c861 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -945,7 +945,7 @@ dependencies = [ [[package]] name = "iris-agentic-dev" -version = "0.16.0" +version = "0.17.0" dependencies = [ "anyhow", "chrono", @@ -963,7 +963,7 @@ dependencies = [ [[package]] name = "iris-agentic-dev-core" -version = "0.16.0" +version = "0.17.0" dependencies = [ "anyhow", "bollard", diff --git a/Cargo.toml b/Cargo.toml index afe4839..128e75b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ members = [ resolver = "2" [workspace.package] -version = "0.16.0" +version = "0.17.0" edition = "2021" authors = ["Thomas Dyar "] license = "MIT" diff --git a/README.md b/README.md index 02640c9..d4e0566 100644 --- a/README.md +++ b/README.md @@ -71,8 +71,11 @@ Restart Claude and **verify with the `check_config` tool** that it connects and > Claude Code and reads the same user-scope `~/.claude.json` registration shown above. > - **Connection settings from VS Code are read natively by this binary.** `iris-interop-dev` parses > `.vscode/settings.json` — `objectscript.conn`, including named servers resolved through -> `intersystems.servers` — as the last step of its discovery cascade. The order, and the known -> limits of that step, are documented at the top of +> `intersystems.servers` — ahead of the blind localhost and Docker scans, so a workspace that +> names its server wins over whatever happens to answer on port 52773. The password is the one +> field it will not invent: Server Manager keeps it in the OS keychain, which this binary cannot +> read, so supply it with `IRIS_PASSWORD`. The full order, and the remaining limits, are at the +> top of > [`discovery.rs`](crates/iris-agentic-dev-core/src/iris/discovery.rs) (issue #187). > - **What this fork does not carry is the Marketplace extension** that auto-registers the server, > and `skill install --agent copilot`. That path belongs to the upstream community tool — see diff --git a/crates/iris-agentic-dev-core/src/iris/discovery.rs b/crates/iris-agentic-dev-core/src/iris/discovery.rs index 0ef737f..25e77bd 100644 --- a/crates/iris-agentic-dev-core/src/iris/discovery.rs +++ b/crates/iris-agentic-dev-core/src/iris/discovery.rs @@ -4,27 +4,27 @@ //! 1. Explicit IrisConnection passed directly //! 2. Env vars (IRIS_HOST + IRIS_WEB_PORT) //! 3. IRIS_CONTAINER — named Docker container, web port resolved via bollard -//! 4. Localhost port scan (100ms timeout, parallel), env-var credentials -//! 5. Docker container scan via bollard -//! 6. VS Code settings.json — `objectscript.conn`, resolving named servers through +//! 4. VS Code settings.json — `objectscript.conn`, resolving named servers through //! `intersystems.servers` (see `vscode_config.rs`) +//! 5. Localhost port scan (100ms timeout, parallel), env-var credentials +//! 6. Docker container scan via bollard //! //! Each step fails silently and falls through to the next. //! -//! KNOWN LIMITS of step 6, tracked by issue #187. Recorded here because a reader -//! concluded from a README sentence that no VS Code config path existed at all — -//! the code is the only place that can answer that: -//! - It runs LAST. Wherever IRIS answers on localhost or in Docker, steps 4-5 win -//! and settings.json is never opened. Step 3 already carries the equivalent -//! precedence fix for the named-container case; step 6 has no counterpart. -//! - `discover_via_vscode_settings` searches exactly one path, -//! `current_dir()/.vscode/settings.json` — not user-scope settings, which is where -//! Server Manager normally keeps servers. -//! - There is no OS-keychain reader. A server entry whose password lives in the -//! keychain arrives here with no password and defaults to _SYSTEM/SYS instead of -//! failing, producing a 401 that names no cause. +//! Steps 3 and 4 are both EXPLICIT configuration and both sit ahead of the blind scans +//! deliberately: they say which instance the user means, and a scan cannot tell one IRIS +//! on 52773 from another. #187 moved step 4 up from last, where it could not run on any +//! machine that had IRIS answering locally. +//! +//! REMAINING LIMIT of step 4, still tracked by #187: `discover_via_vscode_settings` +//! searches exactly one path, `current_dir()/.vscode/settings.json` — not user-scope VS +//! Code settings, which is where Server Manager normally keeps servers. A workspace that +//! defines its own server is found; a server added through the Server Manager UI is not. +//! There is still no OS-keychain reader either, but an absent password is now reported +//! rather than replaced with `SYS`. use crate::iris::connection::{DiscoverySource, IrisConnection}; +use crate::iris::vscode_config::VsCodeResolution; use std::time::Duration; /// The ports we scan on localhost for IRIS web servers. @@ -229,7 +229,17 @@ pub async fn discover_iris(explicit: Option) -> IrisDiscovery { } } - // 4. Localhost scan (parallel, 100ms each). Uses env var credentials. + // 4. VS Code settings.json — EXPLICIT configuration, so it runs before the blind + // scans for exactly the reason step 3 does: a settings.json naming a server states + // WHICH instance the user means, and a port scan cannot tell one IRIS on 52773 from + // another. #187: this used to be step 6, last, which made it unreachable on any + // machine where IRIS answered on localhost or in Docker — the code was present, + // tested, and never executed. + if let Some(conn) = discover_via_vscode_settings().await { + return IrisDiscovery::Found(conn); + } + + // 5. Localhost scan (parallel, 100ms each). Uses env var credentials. let username = std::env::var("IRIS_USERNAME").unwrap_or_else(|_| "_SYSTEM".to_string()); let password = std::env::var("IRIS_PASSWORD").unwrap_or_else(|_| "SYS".to_string()); let namespace = std::env::var("IRIS_NAMESPACE").unwrap_or_else(|_| "USER".to_string()); @@ -262,16 +272,11 @@ pub async fn discover_iris(explicit: Option) -> IrisDiscovery { } } - // 5. Docker scan via bollard + // 6. Docker scan via bollard if let Some(conn) = discover_via_docker().await { return IrisDiscovery::Found(conn); } - // 6. VS Code settings.json - if let Some(conn) = discover_via_vscode_settings().await { - return IrisDiscovery::Found(conn); - } - IrisDiscovery::NotFound } @@ -608,7 +613,12 @@ async fn discover_via_docker() -> Option { None } -/// Attempt to find IRIS connection from VS Code settings.json in common locations. +/// Resolve a connection from VS Code settings.json, if one is configured here. +/// +/// #187: a configured-but-passwordless entry returns `None` like an unconfigured one, +/// but emits a warning naming the file and the server first. The two used to be +/// indistinguishable because the missing password was filled in with `SYS`, so the +/// cascade stopped on a connection that could only ever 401. async fn discover_via_vscode_settings() -> Option { let candidates = [std::env::current_dir().ok()?.join(".vscode/settings.json")]; @@ -616,9 +626,29 @@ async fn discover_via_vscode_settings() -> Option { if !path.exists() { continue; } - if let Ok(settings) = crate::iris::vscode_config::parse_vscode_settings(path) { - if let Some(conn) = settings.to_iris_connection().await { - return Some(conn); + let settings = match crate::iris::vscode_config::parse_vscode_settings(path) { + Ok(s) => s, + Err(e) => { + tracing::warn!("Could not parse {}: {}", path.display(), e); + continue; + } + }; + match settings.resolve() { + VsCodeResolution::Resolved(conn) => return Some(conn), + VsCodeResolution::NotConfigured => continue, + VsCodeResolution::MissingPassword { server } => { + let what = match server.as_deref() { + Some(name) => format!("server '{name}'"), + None => "a connection".to_string(), + }; + tracing::warn!( + "{} configures {} but carries no password. VS Code Server Manager keeps it in \ + the OS keychain, which this binary cannot read — set IRIS_PASSWORD to supply \ + it. Continuing discovery without this entry. (issue #187)", + path.display(), + what + ); + continue; } } } diff --git a/crates/iris-agentic-dev-core/src/iris/vscode_config.rs b/crates/iris-agentic-dev-core/src/iris/vscode_config.rs index 190ac98..4f35502 100644 --- a/crates/iris-agentic-dev-core/src/iris/vscode_config.rs +++ b/crates/iris-agentic-dev-core/src/iris/vscode_config.rs @@ -147,18 +147,58 @@ pub fn parse_vscode_settings(path: impl AsRef) -> anyhow::Result`, which collapsed "nothing is +/// configured here" and "a connection IS configured but its password is missing" into +/// the same `None` — and then avoided the second case entirely by defaulting the +/// password to `SYS`. Those are different answers and the caller needs them apart. +#[derive(Debug)] +pub enum VsCodeResolution { + /// A complete connection was resolved. + Resolved(IrisConnection), + /// Nothing is configured here: no `objectscript.conn`, `active: false`, or a + /// `server:` name with no matching `intersystems.servers` entry. + NotConfigured, + /// A connection is configured but no password is available for it. + /// + /// This is the Server Manager case. The VS Code extension keeps the password in + /// the OS keychain, so it is absent from settings.json, and this binary has no + /// keychain reader. Fabricating `SYS` here produced a connection that looked + /// configured and was wrong — a 401 naming no cause. + MissingPassword { server: Option }, +} + impl VsCodeSettings { - /// Convert parsed settings to an IrisConnection, resolving named servers. - pub async fn to_iris_connection(&self) -> Option { - let conn = self.objectscript_conn.as_ref()?; + /// Resolve to a connection, taking the fallback password from the caller. + /// + /// Pure — `resolve()` supplies `$IRIS_PASSWORD`. Split so tests can cover the + /// credential rules without mutating process environment. + pub fn resolve_with(&self, env_password: Option<&str>) -> VsCodeResolution { + // An empty string is an absent password, not a password of length zero. + let env_password = env_password.filter(|p| !p.is_empty()); + let present = |v: Option<&str>| v.filter(|p| !p.is_empty()).map(str::to_owned); + + let conn = match self.objectscript_conn.as_ref() { + Some(c) => c, + None => return VsCodeResolution::NotConfigured, + }; if conn.active == Some(false) { - return None; + return VsCodeResolution::NotConfigured; } + let ns = conn.ns.as_deref().unwrap_or("USER"); - // Named server path + // Named server path — resolve through `intersystems.servers`. if let Some(server_name) = &conn.server { - let servers = self.intersystems_servers.as_ref()?; - let server = servers.get(server_name)?; + let server = match self + .intersystems_servers + .as_ref() + .and_then(|servers| servers.get(server_name)) + { + Some(s) => s, + // A name with no entry is a typo, not a licence to guess localhost. + None => return VsCodeResolution::NotConfigured, + }; let host = server.web_server.host.as_deref().unwrap_or("localhost"); let web_port = server.web_server.port.unwrap_or(52773); let scheme = server.web_server.scheme.as_deref().unwrap_or("http"); @@ -173,35 +213,48 @@ impl VsCodeSettings { } else { format!("{}://{}:{}/{}", scheme, host, web_port, path_prefix) }; + // Username is not a secret and `_SYSTEM` is this codebase's documented + // default everywhere else; the password is the field Server Manager + // deliberately does not write here, so it is the one we refuse to invent. let username = server.username.as_deref().unwrap_or("_SYSTEM"); - let password = server.password.as_deref().unwrap_or("SYS"); - let ns = conn.ns.as_deref().unwrap_or("USER"); - - let iris_conn = IrisConnection::new( + let password = + match present(server.password.as_deref()).or_else(|| present(env_password)) { + Some(p) => p, + None => { + return VsCodeResolution::MissingPassword { + server: Some(server_name.clone()), + } + } + }; + return VsCodeResolution::Resolved(IrisConnection::new( base_url, ns, username, password, DiscoverySource::VsCodeSettings, - ); - // Note: super_server_port is available if needed for native connections - return Some(iris_conn); + )); } - // Direct host/port path + // Direct host/port path. let host = conn.host.as_deref().unwrap_or("localhost"); let port = conn.port.unwrap_or(52773); let username = conn.username.as_deref().unwrap_or("_SYSTEM"); - let password = conn.password.as_deref().unwrap_or("SYS"); - let ns = conn.ns.as_deref().unwrap_or("USER"); - let base_url = format!("http://{}:{}", host, port); - - Some(IrisConnection::new( - base_url, + let password = match present(conn.password.as_deref()).or_else(|| present(env_password)) { + Some(p) => p, + None => return VsCodeResolution::MissingPassword { server: None }, + }; + VsCodeResolution::Resolved(IrisConnection::new( + format!("http://{}:{}", host, port), ns, username, password, DiscoverySource::VsCodeSettings, )) } + + /// Resolve to a connection, falling back to `$IRIS_PASSWORD` for the secret that + /// Server Manager keeps in the OS keychain. + pub fn resolve(&self) -> VsCodeResolution { + self.resolve_with(std::env::var("IRIS_PASSWORD").ok().as_deref()) + } } diff --git a/crates/iris-agentic-dev-core/tests/vscode_config_tests.rs b/crates/iris-agentic-dev-core/tests/vscode_config_tests.rs index f49b017..31fe397 100644 --- a/crates/iris-agentic-dev-core/tests/vscode_config_tests.rs +++ b/crates/iris-agentic-dev-core/tests/vscode_config_tests.rs @@ -136,3 +136,145 @@ fn settings_without_objectscript_conn_is_ok() { let settings = parse_vscode_settings(&path).unwrap(); assert!(settings.objectscript_conn.is_none()); } + +// ── #187: resolution, not just parsing ─────────────────────────────────────── +// +// Everything above tests that the FIELDS parse. These test what the parsed +// settings resolve TO, which is where the defect lived: a Server Manager entry +// keeps its password in the OS keychain, so it is absent from settings.json, and +// `unwrap_or("SYS")` turned that absence into a confident wrong connection — +// a 401 naming no cause. `resolve_with` takes the environment fallback as an +// argument so these stay pure and cannot race on process env. + +use iris_agentic_dev_core::iris::vscode_config::VsCodeResolution; + +const KEYCHAIN_SERVER: &str = r#"{ + "objectscript.conn": {"active": true, "server": "workshop-iris", "ns": "IRISAPP"}, + "intersystems.servers": { + "workshop-iris": { + "webServer": {"scheme": "http", "host": "localhost", "port": 52773}, + "username": "_SYSTEM" + } + } +}"#; + +fn resolve(content: &str, env_password: Option<&str>) -> VsCodeResolution { + let dir = tempfile::tempdir().unwrap(); + let path = write_settings(dir.path(), content); + parse_vscode_settings(&path) + .unwrap() + .resolve_with(env_password) +} + +/// The #187 defect itself: a named server whose password lives in the keychain +/// must NOT come back as a connection carrying "SYS". +#[test] +fn named_server_without_a_password_is_not_handed_sys() { + match resolve(KEYCHAIN_SERVER, None) { + VsCodeResolution::MissingPassword { server } => { + assert_eq!(server.as_deref(), Some("workshop-iris")); + } + VsCodeResolution::Resolved(conn) => panic!( + "resolved with a fabricated password {:?} — this is the 401 that names no cause", + conn.password + ), + VsCodeResolution::NotConfigured => panic!("a configured server read as NotConfigured"), + } +} + +/// Same for the direct host/port form, which had its own copy of the default. +#[test] +fn direct_connection_without_a_password_is_not_handed_sys() { + let settings = r#"{"objectscript.conn": {"active": true, "host": "localhost", "port": 52773}}"#; + match resolve(settings, None) { + VsCodeResolution::MissingPassword { server } => assert!(server.is_none()), + VsCodeResolution::Resolved(conn) => { + panic!("resolved with a fabricated password {:?}", conn.password) + } + VsCodeResolution::NotConfigured => panic!("a configured connection read as NotConfigured"), + } +} + +/// An empty string is an absent password, not a password of length zero. +#[test] +fn an_empty_password_counts_as_missing() { + let settings = r#"{"objectscript.conn": {"active": true, "host": "h", "password": ""}}"#; + assert!(matches!( + resolve(settings, None), + VsCodeResolution::MissingPassword { .. } + )); +} + +/// The intended composition: VS Code supplies host/port/namespace, the +/// environment supplies the secret that lives in the keychain. +#[test] +fn iris_password_from_the_environment_completes_a_keychain_server() { + match resolve(KEYCHAIN_SERVER, Some("from-the-env")) { + VsCodeResolution::Resolved(conn) => { + assert_eq!(conn.password, "from-the-env"); + assert_eq!(conn.username, "_SYSTEM"); + assert_eq!(conn.namespace, "IRISAPP"); + assert_eq!(conn.base_url, "http://localhost:52773"); + } + other => panic!("expected Resolved, got {other:?}"), + } +} + +/// A password written in settings.json wins over the environment fallback. +#[test] +fn a_password_in_settings_beats_the_environment() { + let settings = r#"{ + "objectscript.conn": {"active": true, "server": "s"}, + "intersystems.servers": { + "s": {"webServer": {"host": "localhost", "port": 52773}, "password": "inline"} + } + }"#; + match resolve(settings, Some("from-the-env")) { + VsCodeResolution::Resolved(conn) => assert_eq!(conn.password, "inline"), + other => panic!("expected Resolved, got {other:?}"), + } +} + +/// active:false means "do not use this", not "use it with defaults". +#[test] +fn inactive_connection_resolves_to_not_configured() { + let settings = r#"{"objectscript.conn": {"active": false, "host": "h", "password": "p"}}"#; + assert!(matches!( + resolve(settings, None), + VsCodeResolution::NotConfigured + )); +} + +/// A server: name with no matching entry is a typo, not a connection to localhost. +#[test] +fn a_named_server_with_no_matching_entry_is_not_configured() { + let settings = r#"{ + "objectscript.conn": {"active": true, "server": "ghost"}, + "intersystems.servers": {"other": {"webServer": {"host": "h"}, "password": "p"}} + }"#; + assert!(matches!( + resolve(settings, None), + VsCodeResolution::NotConfigured + )); +} + +/// pathPrefix still lands in the base URL (regression guard for the rewrite). +#[test] +fn path_prefix_survives_resolution() { + let settings = r#"{ + "objectscript.conn": {"active": true, "server": "s"}, + "intersystems.servers": { + "s": { + "webServer": {"scheme": "https", "host": "iris.example.com", "port": 443, + "pathPrefix": "/gateway/"}, + "password": "p" + } + } + }"#; + match resolve(settings, None) { + VsCodeResolution::Resolved(conn) => { + assert_eq!(conn.base_url, "https://iris.example.com:443/gateway"); + } + other => panic!("expected Resolved, got {other:?}"), + } +}