fix: correct JS entry point, classify well-known JSONC filenames, switch to filesystem PATH probe

lonexreb · lonexreb · commit e90d0a0927f8 · 2026-04-30T03:28:35.000-05:00
Addresses follow-up review on PR #9564 (oz-for-oss bot, second pass) with three important findings: > Custom installs will not be detected or launched because the hard-coded > JS entrypoint does not match the published `vscode-json-languageserver` > package layout. Fixed: the package's `package.json` declares `"main": "./out/node/jsonServerMain"`, not `dist/...`. Updated `LANGSERVER_JS_PATH` accordingly. > JSON-with-comments files such as `tsconfig.json` and > `.vscode/settings.json` are still classified as strict `json`, so valid > comments will be reported as syntax errors. Fixed: `LanguageId::from_path` now first checks the filename. The following well-known config files use `.json` by convention but contain JSONC and now route to `LanguageId::Jsonc`: - `tsconfig.json`, `jsconfig.json` - `tsconfig.<variant>.json`, `jsconfig.<variant>.json` (build, app, etc.) - `.vscode/{settings,launch,tasks,keybindings,extensions}.json` - `devcontainer.json`, `typedoc.json` Strict-JSON filenames (`package.json`, `package-lock.json`, `manifest.json`, etc.) keep going through the extension-only path and remain `LanguageId::Json`. > The PATH installation probe treats any successful spawn as installed > even when the executable exits non-zero. Fixed: `is_installed_on_path` no longer spawns the binary. Spawn-based probes can't reliably distinguish a healthy install from a broken one for this server (it has no documented `--version` or `--help`). Replaced with a pure filesystem PATH search (`binary_in_path` helper) that walks the `path_env_var` directories looking for an executable file by name. On Windows it also tries `.exe`, `.cmd`, `.bat` since npm ships a `.cmd` shim there. Tests added/updated: - `known_dotjson_jsonc_filenames_route_to_jsonc` — exhaustively asserts the 10 well-known JSONC filenames. - `unrelated_dotjson_stays_strict_json` — protects regressions where `package.json` / `manifest.json` would accidentally be reclassified. - `binary_in_path` test module: found in PATH entry and absent. Adds `tempfile` as a workspace dev-dependency (already present in the root `Cargo.toml`) for the test setup. Also adds `.claude/scheduled_tasks.lock` to `.gitignore` so the Claude Code session lock can't sneak into a future commit. Refs #9556, #9564 second review.
diff --git a/.gitignore b/.gitignore
@@ -48,6 +48,7 @@ app/src/persistence/schema.rs.orig
 
 # Don't include personal Claude Code settings
 .claude/settings.local.json
+.claude/scheduled_tasks.lock
 
 # Tab drag development notes
 pr_cleanup.md
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/lsp/Cargo.toml b/crates/lsp/Cargo.toml
@@ -47,3 +47,6 @@ nix = { workspace = true }
 repo_metadata.workspace = true
 sha2 = { workspace = true }
 tokio = { workspace = true, features = ["process"] }
+
+[target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
+tempfile = { workspace = true }
diff --git a/crates/lsp/src/config.rs b/crates/lsp/src/config.rs
@@ -42,6 +42,15 @@ pub enum LanguageId {
 
 impl LanguageId {
     pub fn from_path(path: &Path) -> Option<Self> {
+        // Some files use the `.json` extension by convention but actually
+        // contain JSON-with-comments — `tsconfig.json` is the canonical
+        // example, alongside the VS Code `.vscode/*.json` family. Match
+        // these by filename first so we send `jsonc` (not `json`) in
+        // `didOpen` and the JSON server applies its comments-tolerant
+        // grammar instead of flagging valid `// …` lines as syntax errors.
+        if filename_is_jsonc(path) {
+            return Some(Self::Jsonc);
+        }
         let extn = path.extension()?;
         match extn.to_str()? {
             "rs" => Some(Self::Rust),
@@ -353,6 +362,38 @@ pub fn default_init_params(workspace_uri: &Path, client_name: String) -> Result<
     })
 }
 
+/// Returns `true` for paths whose final filename is a well-known JSON-with-comments
+/// config file. The VS Code JSON language server distinguishes `json` from `jsonc`
+/// and only allows comments under `jsonc`, but several Microsoft tools ship with
+/// `.json` extensions and JSONC contents (TypeScript's `tsconfig.json`,
+/// VS Code's own `settings.json`, etc.). Routing these by name keeps users
+/// from getting spurious "comments not allowed" diagnostics.
+fn filename_is_jsonc(path: &Path) -> bool {
+    let Some(name) = path.file_name().and_then(|n| n.to_str()) else {
+        return false;
+    };
+    // Exact filename matches that are always JSONC by convention.
+    const EXACT: &[&str] = &[
+        "tsconfig.json",
+        "jsconfig.json",
+        // VS Code workspace/user/folder settings files.
+        "settings.json",
+        "launch.json",
+        "tasks.json",
+        "keybindings.json",
+        "extensions.json",
+        // Other editor configs that follow the same convention.
+        "devcontainer.json",
+        "typedoc.json",
+    ];
+    if EXACT.contains(&name) {
+        return true;
+    }
+    // Variants like `tsconfig.build.json`, `tsconfig.app.json`, etc.
+    name.starts_with("tsconfig.") && name.ends_with(".json")
+        || name.starts_with("jsconfig.") && name.ends_with(".json")
+}
+
 #[cfg(test)]
 #[path = "config_tests.rs"]
 mod tests;
diff --git a/crates/lsp/src/config_tests.rs b/crates/lsp/src/config_tests.rs
@@ -244,11 +244,53 @@ mod json_language_detection {
         // route .jsonc through its own LanguageId so the right languageId is
         // sent in `didOpen`.
         assert_eq!(
-            LanguageId::from_path(&PathBuf::from(".vscode/settings.jsonc")),
+            LanguageId::from_path(&PathBuf::from("docs/example.jsonc")),
             Some(LanguageId::Jsonc)
         );
     }
 
+    #[test]
+    fn known_dotjson_jsonc_filenames_route_to_jsonc() {
+        // Several well-known config files use `.json` by convention but
+        // contain JSON-with-comments. Sending `json` would surface valid
+        // `// …` lines as syntax errors.
+        for name in [
+            "tsconfig.json",
+            "jsconfig.json",
+            "tsconfig.build.json",
+            "jsconfig.app.json",
+            ".vscode/settings.json",
+            ".vscode/launch.json",
+            ".vscode/keybindings.json",
+            ".vscode/tasks.json",
+            ".vscode/extensions.json",
+            ".devcontainer/devcontainer.json",
+        ] {
+            assert_eq!(
+                LanguageId::from_path(&PathBuf::from(name)),
+                Some(LanguageId::Jsonc),
+                "{name} should be classified as JSONC",
+            );
+        }
+    }
+
+    #[test]
+    fn unrelated_dotjson_stays_strict_json() {
+        // package.json, manifest.json, etc. are strict JSON.
+        for name in [
+            "package.json",
+            "package-lock.json",
+            "manifest.json",
+            "data.json",
+        ] {
+            assert_eq!(
+                LanguageId::from_path(&PathBuf::from(name)),
+                Some(LanguageId::Json),
+                "{name} should remain strict JSON",
+            );
+        }
+    }
+
     #[test]
     fn both_json_and_jsonc_route_to_vscode_json_language_server() {
         assert_eq!(
diff --git a/crates/lsp/src/servers/vscode_json_language_server.rs b/crates/lsp/src/servers/vscode_json_language_server.rs
@@ -27,11 +27,12 @@ pub struct VsCodeJsonLanguageServerCandidate {
 }
 
 impl VsCodeJsonLanguageServerCandidate {
-    /// Path to the langserver JS entry point relative to the install directory.
-    /// Mirrors the layout produced by `npm install vscode-json-languageserver`.
+    /// Path to the langserver JS entry point relative to the install directory,
+    /// matching the layout published by the `vscode-json-languageserver` npm
+    /// package (whose `package.json` declares `"main": "./out/node/jsonServerMain"`).
     #[cfg(feature = "local_fs")]
     const LANGSERVER_JS_PATH: &str =
-        "node_modules/vscode-json-languageserver/dist/node/jsonServerMain.js";
+        "node_modules/vscode-json-languageserver/out/node/jsonServerMain.js";
 
     pub fn new(client: Arc<http_client::Client>) -> Self {
         Self { client }
@@ -103,19 +104,14 @@ impl LanguageServerCandidate for VsCodeJsonLanguageServerCandidate {
 
     async fn is_installed_on_path(&self, executor: &CommandBuilder) -> bool {
         // `vscode-json-languageserver` only documents `--stdio`, `--node-ipc`,
-        // and `--socket=` as transport flags; passing `--version` or `--help`
-        // makes it error during connection-transport setup. We just need to
-        // know that the binary spawned at all (i.e. is on PATH and
-        // executable), so we use `--stdio` and accept any clean spawn — the
-        // server will start reading LSP messages from stdin, but `output()`
-        // returns Ok as soon as the child process is reaped after EOF.
-        executor
-            .command("vscode-json-languageserver")
-            .arg("--stdio")
-            .stdin(std::process::Stdio::null())
-            .output()
-            .await
-            .is_ok()
+        // and `--socket=` as transport flags. `--version`/`--help` enter
+        // connection-transport setup and exit non-zero; spawning the server
+        // with `--stdio` and EOF on stdin works but treats every spawnable
+        // binary as healthy (including a corrupted install). Use a pure
+        // filesystem PATH search instead — the binary either exists and is
+        // executable or it doesn't, and we never inadvertently prefer a
+        // broken global install over our (working) data_dir copy.
+        binary_in_path("vscode-json-languageserver", executor.path_env_var())
     }
 
     async fn install(
@@ -224,3 +220,92 @@ impl LanguageServerCandidate for VsCodeJsonLanguageServerCandidate {
         todo!()
     }
 }
+
+/// Pure-filesystem search for an executable named `name` in any directory
+/// listed by `path_env_var` (or the process's `PATH` if `None`).
+///
+/// We use this instead of spawning the binary with a probe flag for LSP
+/// servers that have no documented version/help argument — running them
+/// with arbitrary flags either errors during connection-transport setup
+/// or hangs reading from stdin. A filesystem check has no such ambiguity:
+/// the file either exists and is executable, or it doesn't.
+///
+/// On Windows we additionally try the standard executable extensions
+/// (`.exe`, `.cmd`, `.bat`) since the `vscode-json-languageserver` npm
+/// package ships a `.cmd` shim there.
+#[cfg(feature = "local_fs")]
+fn binary_in_path(name: &str, path_env_var: Option<&str>) -> bool {
+    let owned;
+    let path_str = match path_env_var {
+        Some(p) => p,
+        None => match std::env::var("PATH") {
+            Ok(p) => {
+                owned = p;
+                owned.as_str()
+            }
+            Err(_) => return false,
+        },
+    };
+    let separator = if cfg!(windows) { ';' } else { ':' };
+    #[cfg(windows)]
+    let extensions: &[&str] = &["", ".exe", ".cmd", ".bat"];
+    #[cfg(not(windows))]
+    let extensions: &[&str] = &[""];
+
+    for dir in path_str.split(separator) {
+        if dir.is_empty() {
+            continue;
+        }
+        let dir_path = std::path::Path::new(dir);
+        for ext in extensions {
+            let candidate = dir_path.join(format!("{name}{ext}"));
+            if candidate.is_file() {
+                return true;
+            }
+        }
+    }
+    false
+}
+
+#[cfg(test)]
+#[cfg(feature = "local_fs")]
+mod tests {
+    use super::binary_in_path;
+    use std::fs::{self, File};
+
+    #[cfg(unix)]
+    use std::os::unix::fs::PermissionsExt as _;
+
+    fn touch_exe(dir: &std::path::Path, name: &str) -> std::path::PathBuf {
+        let path = dir.join(name);
+        File::create(&path).expect("create test binary");
+        #[cfg(unix)]
+        {
+            let mut perms = fs::metadata(&path).unwrap().permissions();
+            perms.set_mode(0o755);
+            fs::set_permissions(&path, perms).unwrap();
+        }
+        path
+    }
+
+    #[test]
+    fn finds_binary_in_first_path_entry() {
+        let tmp = tempfile::tempdir().expect("tempdir");
+        touch_exe(tmp.path(), "vscode-json-languageserver");
+        let path_var = format!("{}:{}", tmp.path().display(), "/nonexistent/dir");
+        assert!(binary_in_path(
+            "vscode-json-languageserver",
+            Some(&path_var)
+        ));
+    }
+
+    #[test]
+    fn rejects_when_binary_absent() {
+        let tmp = tempfile::tempdir().expect("tempdir");
+        let path_var = tmp.path().display().to_string();
+        assert!(!binary_in_path(
+            "vscode-json-languageserver",
+            Some(&path_var)
+        ));
+    }
+}
