From 896736a37b6e9d3ab9ccd95d17b0b6e14066fa4c Mon Sep 17 00:00:00 2001 From: Candido Sales Gomes Date: Sun, 19 Apr 2026 23:17:42 -0400 Subject: [PATCH 1/3] feat(ruby): add Fuzzy Ruby Server language server support --- extension.toml | 4 ++ src/language_servers/fuzzy_ruby_server.rs | 68 +++++++++++++++++++++++ src/language_servers/mod.rs | 2 + src/ruby.rs | 7 ++- 4 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 src/language_servers/fuzzy_ruby_server.rs diff --git a/extension.toml b/extension.toml index 978ade5..b64782d 100644 --- a/extension.toml +++ b/extension.toml @@ -42,6 +42,10 @@ languages = ["HTML+ERB"] name = "Kanayago" languages = ["Ruby"] +[language_servers.fuzzy-ruby-server] +name = "Fuzzy Ruby Server" +languages = ["Ruby"] + [grammars.ruby] repository = "https://github.com/tree-sitter/tree-sitter-ruby" commit = "71bd32fb7607035768799732addba884a37a6210" diff --git a/src/language_servers/fuzzy_ruby_server.rs b/src/language_servers/fuzzy_ruby_server.rs new file mode 100644 index 0000000..8d6f2c7 --- /dev/null +++ b/src/language_servers/fuzzy_ruby_server.rs @@ -0,0 +1,68 @@ +use super::language_server::LanguageServerBinary; +use super::LanguageServer; +use zed_extension_api::{self as zed}; + +pub struct FuzzyRubyServer {} + +impl LanguageServer for FuzzyRubyServer { + const SERVER_ID: &str = "fuzzy-ruby-server"; + const EXECUTABLE_NAME: &str = "fuzzy"; + const GEM_NAME: &str = ""; + + fn language_server_binary( + &self, + language_server_id: &zed::LanguageServerId, + worktree: &zed::Worktree, + ) -> zed::Result { + let lsp_settings = + zed::settings::LspSettings::for_worktree(language_server_id.as_ref(), worktree)?; + + if let Some(binary) = &lsp_settings.binary { + if let Some(path) = &binary.path { + return Ok(LanguageServerBinary { + path: path.clone(), + args: binary.arguments.clone(), + env: Some(worktree.shell_env()), + }); + } + } + + if let Some(path) = worktree.which(Self::EXECUTABLE_NAME) { + return Ok(LanguageServerBinary { + path, + args: Some(self.get_executable_args(worktree)), + env: Some(worktree.shell_env()), + }); + } + + Err("fuzzy not found. Install with: cargo install --git https://github.com/doompling/fuzzy_ruby_server".into()) + } +} + +impl FuzzyRubyServer { + pub fn new() -> Self { + Self {} + } +} + +#[cfg(test)] +mod tests { + use crate::language_servers::{language_server::FakeWorktree, FuzzyRubyServer, LanguageServer}; + + #[test] + fn test_server_id() { + assert_eq!(FuzzyRubyServer::SERVER_ID, "fuzzy-ruby-server"); + } + + #[test] + fn test_executable_name() { + assert_eq!(FuzzyRubyServer::EXECUTABLE_NAME, "fuzzy"); + } + + #[test] + fn test_executable_args() { + let server = FuzzyRubyServer::new(); + let mock_worktree = FakeWorktree::new("/path/to/project".to_string()); + assert_eq!(server.get_executable_args(&mock_worktree), Vec::::new()); + } +} diff --git a/src/language_servers/mod.rs b/src/language_servers/mod.rs index 8360f25..3641508 100644 --- a/src/language_servers/mod.rs +++ b/src/language_servers/mod.rs @@ -1,3 +1,4 @@ +mod fuzzy_ruby_server; mod herb; mod kanayago; mod language_server; @@ -7,6 +8,7 @@ mod solargraph; mod sorbet; mod steep; +pub use fuzzy_ruby_server::FuzzyRubyServer; pub use herb::Herb; pub use kanayago::Kanayago; pub use language_server::LanguageServer; diff --git a/src/ruby.rs b/src/ruby.rs index 2ee387a..5a775a1 100644 --- a/src/ruby.rs +++ b/src/ruby.rs @@ -9,7 +9,7 @@ use bundler::Bundler; use command_executor::RealCommandExecutor; use gemset::{versioned_gem_home, Gemset}; use language_servers::{ - Herb, Kanayago, LanguageServer, Rubocop, RubyLsp, Solargraph, Sorbet, Steep, + FuzzyRubyServer, Herb, Kanayago, LanguageServer, Rubocop, RubyLsp, Solargraph, Sorbet, Steep, }; use serde::{Deserialize, Serialize}; use zed_extension_api::{ @@ -27,6 +27,7 @@ struct RubyExtension { steep: Option, herb: Option, kanayago: Option, + fuzzy_ruby_server: Option, } #[derive(Serialize, Deserialize)] @@ -80,6 +81,10 @@ impl zed::Extension for RubyExtension { let kanayago = self.kanayago.get_or_insert_with(Kanayago::new); kanayago.language_server_command(language_server_id, worktree) } + FuzzyRubyServer::SERVER_ID => { + let server = self.fuzzy_ruby_server.get_or_insert_with(FuzzyRubyServer::new); + server.language_server_command(language_server_id, worktree) + } language_server_id => Err(format!("unknown language server: {language_server_id}")), } } From 8adac638fee7526f7f66ffe93f503f7161b80551 Mon Sep 17 00:00:00 2001 From: Candido Sales Gomes Date: Sun, 19 Apr 2026 23:34:06 -0400 Subject: [PATCH 2/3] Refactor FuzzyRubyServer to improve binary resolution and testing coverage - Updated GEM_NAME to a non-empty sentinel value to avoid silent failures in gem operations. - Refactored language_server_binary to separate binary resolution logic for custom paths and PATH lookups. - Added checks for the existence and executability of custom binary paths. - Enhanced testing coverage for language_server_binary, including tests for custom paths, PATH discovery, and error handling. - Introduced new methods in the WorktreeLike trait to facilitate testing. - Documented findings and suggestions for maintainability and reliability improvements. --- .../api-contract.json | 51 ++++++++ .../20260419-231842-d1c7183d/correctness.json | 89 ++++++++++++++ .../maintainability.json | 61 ++++++++++ .../20260419-231842-d1c7183d/metadata.json | 7 ++ .../project-standards.json | 49 ++++++++ .../20260419-231842-d1c7183d/reliability.json | 37 ++++++ .../20260419-231842-d1c7183d/testing.json | 53 +++++++++ src/language_servers/fuzzy_ruby_server.rs | 110 +++++++++++++++--- src/language_servers/language_server.rs | 15 +++ 9 files changed, 458 insertions(+), 14 deletions(-) create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json create mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json new file mode 100644 index 0000000..f3054c0 --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json @@ -0,0 +1,51 @@ +{ + "reviewer": "api-contract", + "findings": [ + { + "title": "GEM_NAME = \"\" creates a latent foot-gun: removing the language_server_binary override silently passes \"\" to all gem operations", + "severity": "medium", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 10, + "confidence": 0.82, + "autofix_class": "defensive_constant", + "owner": "extension-maintainer", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Introduce a sentinel that is structurally distinct from a valid gem name. Either (1) add a trait-level mechanism such as `const IS_GEM: bool = false` checked before any gem operation, (2) define a module-internal const like `const GEM_NAME: &str = \"fuzzy-ruby-server--not-a-gem\"` that would produce a visible, diagnosable failure rather than a silent empty-string install, or (3) add a compile-time assert or doc-comment on the trait stating that GEM_NAME = \"\" is explicitly unsupported. The current empty string is a valid `&str` that passes through `gemset.installed_gem_version(\"\")`, `gemset.install_gem(\"\")`, `gemset.update_gem(\"\")`, etc. without a type error, making the missing-override scenario impossible to catch until runtime." + }, + { + "title": "extension.toml version not bumped when a new public language server ID is added", + "severity": "low", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/extension.toml", + "line": 4, + "confidence": 0.72, + "autofix_class": "version_bump", + "owner": "extension-maintainer", + "requires_verification": true, + "pre_existing": false, + "suggested_fix": "Zed's extension registry uses the `version` field in extension.toml to decide when to push an updated extension to users. Adding a new `[language_servers.fuzzy-ruby-server]` section is an additive contract change (non-breaking for existing consumers) but it does constitute a public surface change that should be reflected by a version increment (currently 0.16.11). Verify whether the registry requires a bump on any manifest change; if so, increment to 0.16.12. This is not a breaking change to existing consumers, but omitting the bump means users who already have the extension cached will not receive the new server entry." + }, + { + "title": "custom-path branch passes binary.arguments.clone() but PATH-found branch passes get_executable_args — inconsistent args source for the same server ID", + "severity": "low", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 23, + "confidence": 0.68, + "autofix_class": "interface_alignment", + "owner": "extension-maintainer", + "requires_verification": true, + "pre_existing": false, + "suggested_fix": "In the custom-path branch (lines 20-28), args are set to `binary.arguments.clone()` — which may be `None` — and the trait's `language_server_command` then resolves `None` via `binary.args.unwrap_or(self.get_executable_args(worktree))`. This means a user who sets a custom path but omits `arguments` will fall back to `get_executable_args` (currently `[]`), which is the same as the PATH branch. Behaviour is currently consistent because `get_executable_args` returns `[]` for this server, but this is coincidental. The base trait in language_server.rs line 139 (`binary.args.unwrap_or(self.get_executable_args(worktree))`) already handles the fallback; the override should either (a) always return `args: binary.arguments.clone()` and rely on the trait fallback, or (b) explicitly fill in `get_executable_args` in both branches so the semantics are explicit and won't break if `get_executable_args` is ever overridden." + } + ], + "residual_risks": [ + "The server ID `fuzzy-ruby-server` is now a permanent public contract registered in extension.toml. If the upstream project (https://github.com/doompling/fuzzy_ruby_server) renames its binary from `fuzzy` to something else, updating EXECUTABLE_NAME is a silent behavioral change — users with a cached PATH binary named `fuzzy` will continue to work while new installs fail with an opaque 'fuzzy not found' error.", + "fuzzy_ruby_server is installed via `cargo install`, not via `gem install`. The extension's process:exec capabilities in extension.toml only declare gem/bundle/ruby commands. If a future auto-install code path ever attempts to install this server through the gemset machinery (e.g., if the language_server_binary override is accidentally removed), the capability declarations do not cover `cargo` and the operation will be denied by the sandbox at runtime.", + "There is no test covering the fallback behaviour when `worktree.which(\"fuzzy\")` returns None and no custom binary path is set. The error message string `'fuzzy not found. Install with: cargo install ...'` is the only signal a user gets; it is not tested and can silently drift from the actual install command." + ], + "testing_gaps": [ + "No test exercises the custom-path branch of language_server_binary (lsp_settings.binary.path set). The two existing tests only cover SERVER_ID, EXECUTABLE_NAME, and default get_executable_args.", + "No test covers the error path (both worktree.which and lsp_settings.binary return nothing), so the error message string and Err propagation are untested.", + "No test verifies that GEM_NAME = \"\" does NOT trigger gem operations — i.e., that the override of language_server_binary is the only path taken and the trait's default gem logic is unreachable for this server." + ] +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json new file mode 100644 index 0000000..5b0d436 --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json @@ -0,0 +1,89 @@ +{ + "reviewer": "correctness", + "findings": [ + { + "title": "GEM_NAME empty string causes silent gem logic fallthrough", + "severity": "P2", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 10, + "why_it_matters": "The trait's default `language_server_binary` uses `GEM_NAME` in two places: `bundler.installed_gem_version(Self::GEM_NAME, ...)` and `gemset.installed_gem_version(Self::GEM_NAME)`. Passing an empty string `\"\"` to those calls will either query for a gem named `\"\"` (likely returning not-found), execute `gemset.install_gem(\"\")` (attempting to install a gem with an empty name), or behave in an implementation-defined way. Because `FuzzyRubyServer` overrides `language_server_binary` entirely and never calls the default, this is not an active runtime hazard today. However, if the override is ever accidentally removed or a refactor falls back to the default, the empty `GEM_NAME` will silently drive the gem-install codepath with an invalid gem name. A clearly invalid sentinel (e.g., a doc comment or a compile-time assertion) would be safer than a value that looks valid but is not.", + "autofix_class": "advisory", + "owner": "human", + "requires_verification": false, + "suggested_fix": "Use a value that makes the intent explicit, for example a compile-time assertion `const _: () = assert!(!Self::GEM_NAME.is_empty(), \"...\");` guarded inside the override, or document GEM_NAME as intentionally unused. Alternatively rename the const to something that cannot be accidentally passed to gem APIs, but that requires a trait change.", + "confidence": 0.72, + "evidence": [ + "language_server.rs line 179: `bundler.installed_gem_version(Self::GEM_NAME, &env_vars)` — empty string is forwarded to bundler", + "language_server.rs line 244: `gemset.installed_gem_version(Self::GEM_NAME)` — empty string is forwarded to gemset", + "fuzzy_ruby_server.rs line 10: `const GEM_NAME: &str = \"\";`", + "The override on lines 12-39 fully replaces the default, so there is no current runtime path that reaches those gem calls via FuzzyRubyServer. The risk is latent." + ], + "pre_existing": false + }, + { + "title": "language_server_command sets installation status None before PATH found", + "severity": "P2", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 30, + "why_it_matters": "The trait's `language_server_command` (language_server.rs line 132-135) calls `zed::set_language_server_installation_status(..., None)` after `language_server_binary` returns `Ok`. For gem-based servers this is appropriate because they show `Downloading` status during install. For FuzzyRubyServer the binary is found via PATH only — there is no prior status transition to `CheckingForUpdate` or `Downloading`. Setting status to `None` immediately is correct in the success case. However, if `worktree.which` returns `None` and the error is returned, no installation status is ever set at all (no `CheckingForUpdate`, no failure status), leaving Zed's UI with a stale or empty status. Other extension servers call `set_language_server_installation_status` before attempting discovery. Absence of any status update on failure may cause the UI to appear stuck rather than showing a useful message.", + "autofix_class": "manual", + "owner": "human", + "requires_verification": true, + "suggested_fix": "At the start of `language_server_binary`, call `zed::set_language_server_installation_status(language_server_id, &zed::LanguageServerInstallationStatus::CheckingForUpdate)`. On the error path (binary not found), set an appropriate failure status before returning `Err(...)` so Zed's UI reflects the missing binary.", + "confidence": 0.65, + "evidence": [ + "language_server.rs lines 132-135: `set_language_server_installation_status(..., None)` is called only on success", + "language_server.rs lines 239-242: the gem path calls `CheckingForUpdate` before discovery", + "fuzzy_ruby_server.rs line 38: `Err(\"fuzzy not found...\".into())` returns without setting any status", + "Behavior depends on how Zed handles missing status updates — requires verification against Zed's extension runtime." + ], + "pre_existing": false + }, + { + "title": "Custom binary path ignores user-supplied arguments when path is set", + "severity": "P3", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 20, + "why_it_matters": "When a user configures a custom binary `path` in LSP settings, the code at lines 20-28 returns early with `args: binary.arguments.clone()`. If the user sets a `path` but omits `arguments`, `binary.arguments` is `None`, and the returned `LanguageServerBinary` has `args: None`. In `language_server_command` (language_server.rs line 139), `binary.args.unwrap_or(self.get_executable_args(worktree))` falls back to `get_executable_args`, which returns `Vec::new()` for `FuzzyRubyServer`. This means a user who sets only a custom path gets an empty argument list. This mirrors the pattern of all other servers in the codebase (same logic in the trait default), so it is consistent — but for a binary that might require arguments to start in LSP mode, this could cause a silent failure where the process starts but does not speak LSP.", + "autofix_class": "advisory", + "owner": "human", + "requires_verification": true, + "suggested_fix": "Document whether `fuzzy_ruby_server` requires any arguments when launched in LSP mode. If default arguments are needed, override `get_executable_args` to return them, which will then serve as the fallback in `language_server_command` whenever `args` is `None`.", + "confidence": 0.61, + "evidence": [ + "fuzzy_ruby_server.rs lines 22-26: returns `args: binary.arguments.clone()` which is `None` when user omits arguments", + "language_server.rs line 139: `binary.args.unwrap_or(self.get_executable_args(worktree))`", + "fuzzy_ruby_server.rs: no override of `get_executable_args`, so the fallback is `Vec::new()`" + ], + "pre_existing": false + }, + { + "title": "Test for language_server_binary with PATH hit is missing", + "severity": "P3", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 48, + "why_it_matters": "The three tests only verify constants and that `get_executable_args` returns an empty vec. No test exercises `language_server_binary` itself: not the custom-path branch, not the PATH-found branch, and not the error branch. The FakeWorktree type is available and supports `lsp_binary_settings`, so these paths are testable. Without coverage, a future refactor that accidentally removes the PATH-only override and falls back to the gem-based default would not be caught by tests.", + "autofix_class": "manual", + "owner": "human", + "requires_verification": false, + "suggested_fix": "Add unit tests that: (1) configure a custom binary path via `FakeWorktree::add_lsp_binary_setting` and assert the path is returned; (2) simulate `worktree.which` returning `Some(path)` and assert the PATH binary is returned; (3) simulate neither configured path nor PATH binary and assert the error message contains the cargo install hint.", + "confidence": 0.90, + "evidence": [ + "fuzzy_ruby_server.rs lines 48-68: only three tests, none call `language_server_binary`", + "language_server.rs lines 59-113: `FakeWorktree` supports `add_lsp_binary_setting` and file stubs, making these tests feasible", + "The critical override logic on lines 12-39 has zero test coverage" + ], + "pre_existing": false + } + ], + "residual_risks": [ + "If `worktree.which` is non-deterministic across environments (e.g., Cargo bin dir not in Zed's PATH), users will hit the error message even with `fuzzy_ruby_server` installed. The error text is correct (cargo install hint) but offers no guidance on PATH configuration for Zed specifically.", + "The executable name `fuzzy` (EXECUTABLE_NAME = \"fuzzy\") is very short and could shadow an unrelated binary named `fuzzy` on the user's PATH, causing the wrong binary to be launched silently. The trait dispatches on `SERVER_ID` but executes whatever `which(\"fuzzy\")` returns." + ], + "testing_gaps": [ + "No test for the `language_server_binary` custom-path branch (lsp_settings.binary.path set).", + "No test for the `language_server_binary` PATH-found branch (worktree.which returns Some).", + "No test for the `language_server_binary` error branch (binary not on PATH, no custom path configured).", + "No integration/smoke test verifying that the SERVER_ID `\"fuzzy-ruby-server\"` registered in extension.toml matches the match arm in ruby.rs and the constant in fuzzy_ruby_server.rs (currently verified only by manual string comparison)." + ] +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json new file mode 100644 index 0000000..5b518b2 --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json @@ -0,0 +1,61 @@ +{ + "reviewer": "maintainability", + "findings": [ + { + "title": "GEM_NAME forced to empty string to satisfy a trait contract that does not apply", + "severity": "high", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 10, + "confidence": 0.92, + "autofix_class": "refactor", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Introduce a second trait — e.g. `NonGemLanguageServer` or a trait without `GEM_NAME` — for binary-only servers, or make `GEM_NAME` an `Option<&str>` defaulting to `None`. The current empty-string sentinel is a lie the compiler cannot catch: future callers of `Self::GEM_NAME` will silently receive `\"\"` and may pass it to `gem install`, `bundler.installed_gem_version`, or similar gem tooling, producing silent failures. At minimum, rename the constant to make the intent explicit and add a compile-time assertion (`const _: () = assert!(Self::GEM_NAME.is_empty() ...)`), but the real fix is removing the constraint from the trait boundary." + }, + { + "title": "language_server_binary override in FuzzyRubyServer duplicates the custom-path + PATH-lookup logic already in the trait default", + "severity": "medium", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 12, + "confidence": 0.85, + "autofix_class": "refactor", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "The first 28 lines of `FuzzyRubyServer::language_server_binary` (custom path from settings, then `worktree.which`) are structurally identical to the top of the trait default and to `try_find_on_path_or_extension_gemset`. Extract a shared helper — e.g. `trait LanguageServer::binary_from_settings_or_path` — that does the settings-path check and PATH lookup, then let both the gem path and the Cargo-binary path call it. This eliminates the need to override the entire method just to skip the gem tail." + }, + { + "title": "FuzzyRubyServer::new() is a trivial constructor on a zero-field struct with no derived Default", + "severity": "low", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 43, + "confidence": 0.82, + "autofix_class": "trivial_fix", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Derive `Default` on `FuzzyRubyServer` and remove the handwritten `new()`. Every other zero-field server in the codebase (Rubocop, Kanayago) follows the same pattern and also omits `Default`, so this is a systemic pattern — but it becomes a maintenance liability here because `ruby.rs` calls `FuzzyRubyServer::new` in `get_or_insert_with`, meaning any future refactor that adds state must touch both the struct and the call site. `#[derive(Default)]` removes the boilerplate without adding cost." + }, + { + "title": "Herb implements language_server_command on the concrete type, shadowing the trait method without overriding it through the trait", + "severity": "medium", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/herb.rs", + "line": 30, + "confidence": 0.80, + "autofix_class": "refactor", + "owner": "author", + "requires_verification": false, + "pre_existing": true, + "suggested_fix": "Herb defines `language_server_command` as an inherent method (not a trait override), so the trait dispatch in ruby.rs calls the inherent version only because the inherent method takes priority. This is fragile: if a caller holds a `&dyn LanguageServer`, the trait version would run instead. The method should be moved into the `impl LanguageServer for Herb` block. This is pre-existing but surfaced because FuzzyRubyServer correctly uses the trait override pattern, making the inconsistency visible." + } + ], + "residual_risks": [ + "The `GEM_NAME: &str = \"\"` sentinel is not enforced by the type system. If `extension_gemset_language_server_binary` is ever called on FuzzyRubyServer (e.g. through a future refactor that adds a fallback chain), it will pass the empty string to `gem install` and `gem list`, producing confusing runtime errors rather than a compile-time or clear panic.", + "The error message in `FuzzyRubyServer::language_server_binary` hardcodes the GitHub URL of the upstream project. If the project moves or is renamed, the error message becomes misleading. There is no test covering the not-found path." + ], + "testing_gaps": [ + "There are no tests for `FuzzyRubyServer::language_server_binary` covering (a) the custom-path-from-settings branch, (b) the `worktree.which` found branch, or (c) the not-found error branch. The existing tests only assert constant values and the default empty args, which do not exercise any resolution logic.", + "There is no test asserting that GEM_NAME being empty does not cause a panic or silent no-op if the gem code path is accidentally reached. A regression could be introduced silently." + ] +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json new file mode 100644 index 0000000..7487caf --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json @@ -0,0 +1,7 @@ +{ + "run_id": "20260419-231842-d1c7183d", + "branch": "fuzzy-ruby-server", + "head_sha": "896736a37b6e9d3ab9ccd95d17b0b6e14066fa4c", + "verdict": "Ready with fixes", + "completed_at": "2026-04-19T23:21:00Z" +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json new file mode 100644 index 0000000..f696496 --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json @@ -0,0 +1,49 @@ +{ + "reviewer": "project-standards", + "findings": [ + { + "title": "GEM_NAME is empty string, breaking gem-based fallback resolution", + "severity": "high", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 10, + "confidence": 0.92, + "autofix_class": "manual", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "fuzzy_ruby_server is not a RubyGem (it is installed via `cargo install`), so the shared trait's gem-based resolution path in `language_server_binary` / `extension_gemset_language_server_binary` will silently pass an empty string to every gem lookup call (e.g. `bundler.installed_gem_version(\"\", ...)` and `gemset.installed_gem_version(\"\", ...)`). The established pattern for all gem-backed servers (kanayago, rubocop, solargraph, sorbet, steep) sets GEM_NAME to the actual gem name. Since fuzzy_ruby_server is Cargo-installed rather than gem-installed, GEM_NAME cannot meaningfully point to a gem. The correct fix is to give GEM_NAME a sentinel value that makes the intent explicit (e.g. `const GEM_NAME: &str = \"fuzzy_ruby_server\"` paired with a note, or an empty string only if the override in `language_server_binary` fully guards against the gem path being reached). As implemented, the override in `language_server_binary` does skip the trait default, so the empty GEM_NAME is never passed through — but the value is still misleading and fragile: any future refactor that removes the override would silently break gem resolution." + }, + { + "title": "language_server_binary override duplicates trait default logic and bypasses bundler/gemset resolution", + "severity": "medium", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 12, + "confidence": 0.85, + "autofix_class": "manual", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Every peer language server that uses gem-based resolution (kanayago, rubocop, solargraph, sorbet, steep) relies entirely on the trait default `language_server_binary`, which handles the LspSettings binary override, bundler lookup, PATH lookup, and extension gemset install in that order. FuzzyRubyServer re-implements only the first two steps (LspSettings binary override + `worktree.which()`) and omits the bundler and gemset steps, which is correct for a Cargo-installed binary but the duplication is fragile: if the trait default's LspSettings handling changes, this override will diverge silently. The established pattern for servers that need non-gem resolution (see Herb, which uses npm) is to override `language_server_command` instead and include a clear comment explaining why. Consider either (a) overriding `language_server_command` instead of `language_server_binary` to match how Herb handles its non-gem installation, or (b) documenting in a comment on the override why the gem/bundler paths are intentionally skipped." + }, + { + "title": "Import style inconsistent with simple gem-backed peers", + "severity": "low", + "file": "src/language_servers/fuzzy_ruby_server.rs", + "line": 1, + "confidence": 0.72, + "autofix_class": "manual", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Kanayago and Rubocop (the closest structural peers) import only `use super::{language_server::WorktreeLike, LanguageServer}`. FuzzyRubyServer imports `use super::language_server::LanguageServerBinary` and `use super::LanguageServer` as separate lines, and does not import `WorktreeLike` (because `get_executable_args` is not overridden with a generic bound). This is a consequence of the `language_server_binary` override (finding #2). Resolving finding #2 would bring the imports back into line with peers." + } + ], + "residual_risks": [ + "If the `language_server_binary` override is removed in a future refactor (e.g. to consolidate resolution logic), the empty GEM_NAME will cause silent failures in the gem-based fallback path — `installed_gem_version(\"\", ...)` will not error loudly and the extension will fall through to an unhelpful error message.", + "The error message on line 38 (`'fuzzy not found. Install with: cargo install ...'`) is only reached when `worktree.which(\"fuzzy\")` fails. Users who have the binary installed under a different name or in a PATH location not visible to the worktree will receive this message with no PATH diagnostic information, unlike the trait default which provides more context via the gemset path." + ], + "testing_gaps": [ + "There is no test covering the fallback error path (when `worktree.which(\"fuzzy\")` returns None and no LspSettings binary is configured). The existing `test_executable_args` test only exercises the happy-path `get_executable_args` result. A test using a FakeWorktree that returns None for `which()` would validate the error message string, consistent with how other servers test their error cases.", + "There is no test asserting that GEM_NAME has the expected value (unlike SERVER_ID and EXECUTABLE_NAME which are both tested). Given that GEM_NAME is empty and this is unusual, an explicit test with a comment explaining the empty value would prevent silent breakage if the constant is changed." + ] +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json new file mode 100644 index 0000000..9195b0e --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json @@ -0,0 +1,37 @@ +{ + "reviewer": "reliability", + "findings": [ + { + "title": "Custom binary path returned without existence or executability check", + "severity": "medium", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 22, + "confidence": 0.82, + "autofix_class": "logic_fix", + "owner": "author", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "Before returning Ok with the user-configured path, verify the path exists and is executable using std::fs::metadata or std::path::Path::is_file. Return a descriptive Err if the configured path is not found, e.g.: `if !std::path::Path::new(path).is_file() { return Err(format!(\"fuzzy-ruby-server: configured binary path '{}' does not exist or is not a file\", path).into()); }`. This converts a later opaque OS process-spawn error into a clear actionable message at configuration time." + }, + { + "title": "PATH-based discovery silently fails when cargo bin dir is not in Zed shell env", + "severity": "low", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 30, + "confidence": 0.68, + "autofix_class": "documentation", + "owner": "author", + "requires_verification": true, + "pre_existing": false, + "suggested_fix": "Zed's shell env may not include ~/.cargo/bin because Zed launches with a login shell that doesn't always source .profile or .cargo/env. The error message on line 38 is accurate but users may not understand why `which` fails even though `fuzzy` works in their terminal. Consider augmenting the error message to note that Zed may not inherit the full PATH and suggest setting the binary path explicitly in LSP settings as a fallback: append ' If fuzzy is installed but not found, set the path explicitly in your Zed LSP settings.' to the error string." + } + ], + "residual_risks": [ + "If Zed's worktree.shell_env() does not include the user's cargo bin directory (a common issue on macOS with GUI-launched apps), fuzzy_ruby_server will always fall through to the Err branch even when the binary is installed. This is not a code defect but it will cause repeated failed activations on every file open until the user manually configures the binary path.", + "The base trait language_server_command calls set_language_server_installation_status with None only on success. On the Err path (binary not found), no status is set, so the Zed UI may display a stale installation status from a previous successful run. This is inherent to the base trait design and not fixable in the override alone." + ], + "testing_gaps": [ + "There are no tests exercising the custom binary path code path (lsp_settings.binary.path set). The existing tests only cover SERVER_ID, EXECUTABLE_NAME, and get_executable_args. A test that injects a non-existent custom path should assert that an appropriate error is returned rather than Ok.", + "There is no test for the which() fallback path returning a binary. Since FakeWorktree does not implement worktree.which(), the PATH lookup branch (lines 30-36) cannot be unit-tested with the current test infrastructure. The FakeWorktree would need a which() mock method to cover this branch." + ] +} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json new file mode 100644 index 0000000..b37561e --- /dev/null +++ b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json @@ -0,0 +1,53 @@ +{ + "reviewer": "testing", + "findings": [ + { + "title": "All three branches of language_server_binary are untested: custom path, PATH lookup, and error", + "severity": "P1", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 12, + "confidence": 0.95, + "autofix_class": "manual", + "owner": "candidosg@gmail.com", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "The language_server_binary method has three distinct branches that need separate tests: (1) custom binary path from LspSettings — use FakeWorktree::add_lsp_binary_setting() to inject a path and assert the returned LanguageServerBinary.path matches; (2) PATH-based discovery via worktree.which() — this requires either refactoring language_server_binary to accept a WorktreeLike trait (consistent with how get_executable_args works) so FakeWorktree can stub which(), or adding a which() method to WorktreeLike and FakeWorktree; (3) the error path when worktree.which() returns None — assert that Err containing 'fuzzy not found' is returned. Without these tests, a regression that silently drops the custom-path branch or always errors would not be caught." + }, + { + "title": "test_executable_args asserts the trait default, not FuzzyRubyServer-specific behavior, providing false confidence", + "severity": "P2", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 62, + "confidence": 0.90, + "autofix_class": "advisory", + "owner": "candidosg@gmail.com", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "FuzzyRubyServer does not override get_executable_args, so the test merely confirms the trait default returns an empty Vec. This is already covered by the test_default_executable_args test in language_server.rs. The test adds no signal specific to FuzzyRubyServer. Either remove it to reduce noise, or—if the intent is to lock FuzzyRubyServer to zero args permanently—add a comment documenting why, and include a test that the binary receives no extra args end-to-end (which would require the language_server_binary branch tests above)." + }, + { + "title": "language_server_binary takes &zed::Worktree directly rather than a WorktreeLike trait, structurally preventing unit testing of all three branches", + "severity": "P2", + "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", + "line": 12, + "confidence": 0.92, + "autofix_class": "manual", + "owner": "candidosg@gmail.com", + "requires_verification": false, + "pre_existing": false, + "suggested_fix": "The trait method signature uses &zed::Worktree (the concrete Zed API type) so FakeWorktree cannot substitute. Add a which(name: &str) -> Option method to the WorktreeLike trait and implement it on both FakeWorktree and the zed::Worktree adapter. Then refactor language_server_binary (in the trait and in FuzzyRubyServer's override) to accept &dyn WorktreeLike or a generic T: WorktreeLike, matching the pattern already used by get_executable_args. This unblocks writing the three unit tests described in the first finding." + } + ], + "residual_risks": [ + "The error message string 'fuzzy not found. Install with: cargo install --git https://github.com/doompling/fuzzy_ruby_server' is user-facing and has no test asserting its exact content. If the URL or phrasing changes silently, users get broken install instructions with no test failure.", + "GEM_NAME is intentionally empty string. No test documents or guards this intentional deviation from every other LanguageServer implementation. If the trait ever adds logic gated on GEM_NAME being non-empty, FuzzyRubyServer silently misbehaves.", + "The custom-path branch copies binary.arguments as-is from LspSettings without sanitization. No test verifies behavior when arguments is None vs Some(vec![]) vs Some(vec![\"--flag\"])." + ], + "testing_gaps": [ + "Branch: lsp_settings.binary.path is Some(path) — custom binary path is returned with correct path, args from settings, and shell_env. Zero tests cover this.", + "Branch: lsp_settings.binary.path is None and worktree.which('fuzzy') returns Some(path) — PATH-discovered binary is returned with empty args and shell_env. Zero tests cover this.", + "Branch: both lsp_settings.binary.path is None and worktree.which('fuzzy') returns None — Err containing install instructions is returned. Zero tests cover this.", + "Branch: lsp_settings.binary is Some but binary.path is None (binary settings exist but no path override) — code falls through to the which() check. Zero tests cover this.", + "Integration: FuzzyRubyServer::SERVER_ID arm in ruby.rs language_server_command dispatch is not tested. If the match arm were deleted or the SERVER_ID changed without updating ruby.rs, no test would catch it." + ] +} diff --git a/src/language_servers/fuzzy_ruby_server.rs b/src/language_servers/fuzzy_ruby_server.rs index 8d6f2c7..1f5a9ac 100644 --- a/src/language_servers/fuzzy_ruby_server.rs +++ b/src/language_servers/fuzzy_ruby_server.rs @@ -7,21 +7,41 @@ pub struct FuzzyRubyServer {} impl LanguageServer for FuzzyRubyServer { const SERVER_ID: &str = "fuzzy-ruby-server"; const EXECUTABLE_NAME: &str = "fuzzy"; - const GEM_NAME: &str = ""; + const GEM_NAME: &str = "fuzzy-ruby-server--not-a-gem"; fn language_server_binary( &self, language_server_id: &zed::LanguageServerId, worktree: &zed::Worktree, ) -> zed::Result { - let lsp_settings = - zed::settings::LspSettings::for_worktree(language_server_id.as_ref(), worktree)?; + self.resolve_binary(language_server_id.as_ref(), worktree) + } +} + +impl FuzzyRubyServer { + pub fn new() -> Self { + Self {} + } + + fn resolve_binary( + &self, + server_id: &str, + worktree: &T, + ) -> zed::Result { + let lsp_settings = worktree.lsp_binary_settings(server_id)?; - if let Some(binary) = &lsp_settings.binary { - if let Some(path) = &binary.path { + if let Some(binary_settings) = lsp_settings { + if let Some(path) = binary_settings.path { + if !std::path::Path::new(&path).is_file() { + return Err(format!( + "fuzzy-ruby-server: configured binary path '{}' does not exist or is not a file. Update lsp.fuzzy-ruby-server.binary.path in your Zed settings.", + path + ) + .into()); + } return Ok(LanguageServerBinary { - path: path.clone(), - args: binary.arguments.clone(), + path, + args: binary_settings.arguments, env: Some(worktree.shell_env()), }); } @@ -39,15 +59,12 @@ impl LanguageServer for FuzzyRubyServer { } } -impl FuzzyRubyServer { - pub fn new() -> Self { - Self {} - } -} - #[cfg(test)] mod tests { - use crate::language_servers::{language_server::FakeWorktree, FuzzyRubyServer, LanguageServer}; + use crate::language_servers::{ + language_server::{FakeWorktree, LspBinarySettings}, + FuzzyRubyServer, LanguageServer, + }; #[test] fn test_server_id() { @@ -65,4 +82,69 @@ mod tests { let mock_worktree = FakeWorktree::new("/path/to/project".to_string()); assert_eq!(server.get_executable_args(&mock_worktree), Vec::::new()); } + + #[test] + fn test_language_server_binary_custom_path() { + let server = FuzzyRubyServer::new(); + let mut mock_worktree = FakeWorktree::new("/path/to/project".to_string()); + // Use a path that actually exists on the system for the custom-path test + let real_path = std::env::current_exe() + .expect("could not get test binary path") + .to_string_lossy() + .to_string(); + mock_worktree.add_lsp_binary_setting( + FuzzyRubyServer::SERVER_ID.to_string(), + Ok(Some(LspBinarySettings { + path: Some(real_path.clone()), + arguments: None, + })), + ); + let result = server.resolve_binary(FuzzyRubyServer::SERVER_ID, &mock_worktree); + assert!(result.is_ok()); + assert_eq!(result.unwrap().path, real_path); + } + + #[test] + fn test_language_server_binary_custom_path_missing() { + let server = FuzzyRubyServer::new(); + let mut mock_worktree = FakeWorktree::new("/path/to/project".to_string()); + mock_worktree.add_lsp_binary_setting( + FuzzyRubyServer::SERVER_ID.to_string(), + Ok(Some(LspBinarySettings { + path: Some("/nonexistent/fuzzy".to_string()), + arguments: None, + })), + ); + let result = server.resolve_binary(FuzzyRubyServer::SERVER_ID, &mock_worktree); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + err.contains("does not exist or is not a file"), + "Error was: {err}" + ); + } + + #[test] + fn test_language_server_binary_path_lookup() { + let server = FuzzyRubyServer::new(); + let mut mock_worktree = FakeWorktree::new("/path/to/project".to_string()); + mock_worktree.add_lsp_binary_setting(FuzzyRubyServer::SERVER_ID.to_string(), Ok(None)); + mock_worktree.set_which("fuzzy".to_string(), Some("/usr/local/bin/fuzzy".to_string())); + let result = server.resolve_binary(FuzzyRubyServer::SERVER_ID, &mock_worktree); + assert!(result.is_ok()); + assert_eq!(result.unwrap().path, "/usr/local/bin/fuzzy"); + } + + #[test] + fn test_language_server_binary_not_found() { + let server = FuzzyRubyServer::new(); + let mut mock_worktree = FakeWorktree::new("/path/to/project".to_string()); + mock_worktree.add_lsp_binary_setting(FuzzyRubyServer::SERVER_ID.to_string(), Ok(None)); + mock_worktree.set_which("fuzzy".to_string(), None); + let result = server.resolve_binary(FuzzyRubyServer::SERVER_ID, &mock_worktree); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.contains("fuzzy not found"), "Error was: {err}"); + assert!(err.contains("cargo install"), "Error was: {err}"); + } } diff --git a/src/language_servers/language_server.rs b/src/language_servers/language_server.rs index 816d8e6..8542ebc 100644 --- a/src/language_servers/language_server.rs +++ b/src/language_servers/language_server.rs @@ -30,6 +30,7 @@ pub trait WorktreeLike { fn shell_env(&self) -> Vec<(String, String)>; fn read_text_file(&self, path: &str) -> Result; fn lsp_binary_settings(&self, server_id: &str) -> Result, String>; + fn which(&self, name: &str) -> Option; } impl WorktreeLike for zed::Worktree { @@ -54,6 +55,10 @@ impl WorktreeLike for zed::Worktree { Err(e) => Err(e), } } + + fn which(&self, name: &str) -> Option { + zed::Worktree::which(self, name) + } } #[cfg(test)] @@ -62,6 +67,7 @@ pub struct FakeWorktree { shell_env: Vec<(String, String)>, files: HashMap>, lsp_binary_settings_map: HashMap, String>>, + which_map: HashMap>, } #[cfg(test)] @@ -72,6 +78,7 @@ impl FakeWorktree { shell_env: Vec::new(), files: HashMap::new(), lsp_binary_settings_map: HashMap::new(), + which_map: HashMap::new(), } } @@ -86,6 +93,10 @@ impl FakeWorktree { ) { self.lsp_binary_settings_map.insert(server_id, settings); } + + pub fn set_which(&mut self, name: String, result: Option) { + self.which_map.insert(name, result); + } } #[cfg(test)] @@ -111,6 +122,10 @@ impl WorktreeLike for FakeWorktree { .cloned() .unwrap_or(Ok(None)) } + + fn which(&self, name: &str) -> Option { + self.which_map.get(name).cloned().flatten() + } } pub trait LanguageServer { From 2d74d0c6a5706456ce85c7195a8b2e6d6c469ca3 Mon Sep 17 00:00:00 2001 From: Candido Sales Gomes Date: Sun, 19 Apr 2026 23:34:22 -0400 Subject: [PATCH 3/3] delete --- .../api-contract.json | 51 ----------- .../20260419-231842-d1c7183d/correctness.json | 89 ------------------- .../maintainability.json | 61 ------------- .../20260419-231842-d1c7183d/metadata.json | 7 -- .../project-standards.json | 49 ---------- .../20260419-231842-d1c7183d/reliability.json | 37 -------- .../20260419-231842-d1c7183d/testing.json | 53 ----------- 7 files changed, 347 deletions(-) delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json delete mode 100644 .context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json deleted file mode 100644 index f3054c0..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/api-contract.json +++ /dev/null @@ -1,51 +0,0 @@ -{ - "reviewer": "api-contract", - "findings": [ - { - "title": "GEM_NAME = \"\" creates a latent foot-gun: removing the language_server_binary override silently passes \"\" to all gem operations", - "severity": "medium", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 10, - "confidence": 0.82, - "autofix_class": "defensive_constant", - "owner": "extension-maintainer", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Introduce a sentinel that is structurally distinct from a valid gem name. Either (1) add a trait-level mechanism such as `const IS_GEM: bool = false` checked before any gem operation, (2) define a module-internal const like `const GEM_NAME: &str = \"fuzzy-ruby-server--not-a-gem\"` that would produce a visible, diagnosable failure rather than a silent empty-string install, or (3) add a compile-time assert or doc-comment on the trait stating that GEM_NAME = \"\" is explicitly unsupported. The current empty string is a valid `&str` that passes through `gemset.installed_gem_version(\"\")`, `gemset.install_gem(\"\")`, `gemset.update_gem(\"\")`, etc. without a type error, making the missing-override scenario impossible to catch until runtime." - }, - { - "title": "extension.toml version not bumped when a new public language server ID is added", - "severity": "low", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/extension.toml", - "line": 4, - "confidence": 0.72, - "autofix_class": "version_bump", - "owner": "extension-maintainer", - "requires_verification": true, - "pre_existing": false, - "suggested_fix": "Zed's extension registry uses the `version` field in extension.toml to decide when to push an updated extension to users. Adding a new `[language_servers.fuzzy-ruby-server]` section is an additive contract change (non-breaking for existing consumers) but it does constitute a public surface change that should be reflected by a version increment (currently 0.16.11). Verify whether the registry requires a bump on any manifest change; if so, increment to 0.16.12. This is not a breaking change to existing consumers, but omitting the bump means users who already have the extension cached will not receive the new server entry." - }, - { - "title": "custom-path branch passes binary.arguments.clone() but PATH-found branch passes get_executable_args — inconsistent args source for the same server ID", - "severity": "low", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 23, - "confidence": 0.68, - "autofix_class": "interface_alignment", - "owner": "extension-maintainer", - "requires_verification": true, - "pre_existing": false, - "suggested_fix": "In the custom-path branch (lines 20-28), args are set to `binary.arguments.clone()` — which may be `None` — and the trait's `language_server_command` then resolves `None` via `binary.args.unwrap_or(self.get_executable_args(worktree))`. This means a user who sets a custom path but omits `arguments` will fall back to `get_executable_args` (currently `[]`), which is the same as the PATH branch. Behaviour is currently consistent because `get_executable_args` returns `[]` for this server, but this is coincidental. The base trait in language_server.rs line 139 (`binary.args.unwrap_or(self.get_executable_args(worktree))`) already handles the fallback; the override should either (a) always return `args: binary.arguments.clone()` and rely on the trait fallback, or (b) explicitly fill in `get_executable_args` in both branches so the semantics are explicit and won't break if `get_executable_args` is ever overridden." - } - ], - "residual_risks": [ - "The server ID `fuzzy-ruby-server` is now a permanent public contract registered in extension.toml. If the upstream project (https://github.com/doompling/fuzzy_ruby_server) renames its binary from `fuzzy` to something else, updating EXECUTABLE_NAME is a silent behavioral change — users with a cached PATH binary named `fuzzy` will continue to work while new installs fail with an opaque 'fuzzy not found' error.", - "fuzzy_ruby_server is installed via `cargo install`, not via `gem install`. The extension's process:exec capabilities in extension.toml only declare gem/bundle/ruby commands. If a future auto-install code path ever attempts to install this server through the gemset machinery (e.g., if the language_server_binary override is accidentally removed), the capability declarations do not cover `cargo` and the operation will be denied by the sandbox at runtime.", - "There is no test covering the fallback behaviour when `worktree.which(\"fuzzy\")` returns None and no custom binary path is set. The error message string `'fuzzy not found. Install with: cargo install ...'` is the only signal a user gets; it is not tested and can silently drift from the actual install command." - ], - "testing_gaps": [ - "No test exercises the custom-path branch of language_server_binary (lsp_settings.binary.path set). The two existing tests only cover SERVER_ID, EXECUTABLE_NAME, and default get_executable_args.", - "No test covers the error path (both worktree.which and lsp_settings.binary return nothing), so the error message string and Err propagation are untested.", - "No test verifies that GEM_NAME = \"\" does NOT trigger gem operations — i.e., that the override of language_server_binary is the only path taken and the trait's default gem logic is unreachable for this server." - ] -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json deleted file mode 100644 index 5b0d436..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/correctness.json +++ /dev/null @@ -1,89 +0,0 @@ -{ - "reviewer": "correctness", - "findings": [ - { - "title": "GEM_NAME empty string causes silent gem logic fallthrough", - "severity": "P2", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 10, - "why_it_matters": "The trait's default `language_server_binary` uses `GEM_NAME` in two places: `bundler.installed_gem_version(Self::GEM_NAME, ...)` and `gemset.installed_gem_version(Self::GEM_NAME)`. Passing an empty string `\"\"` to those calls will either query for a gem named `\"\"` (likely returning not-found), execute `gemset.install_gem(\"\")` (attempting to install a gem with an empty name), or behave in an implementation-defined way. Because `FuzzyRubyServer` overrides `language_server_binary` entirely and never calls the default, this is not an active runtime hazard today. However, if the override is ever accidentally removed or a refactor falls back to the default, the empty `GEM_NAME` will silently drive the gem-install codepath with an invalid gem name. A clearly invalid sentinel (e.g., a doc comment or a compile-time assertion) would be safer than a value that looks valid but is not.", - "autofix_class": "advisory", - "owner": "human", - "requires_verification": false, - "suggested_fix": "Use a value that makes the intent explicit, for example a compile-time assertion `const _: () = assert!(!Self::GEM_NAME.is_empty(), \"...\");` guarded inside the override, or document GEM_NAME as intentionally unused. Alternatively rename the const to something that cannot be accidentally passed to gem APIs, but that requires a trait change.", - "confidence": 0.72, - "evidence": [ - "language_server.rs line 179: `bundler.installed_gem_version(Self::GEM_NAME, &env_vars)` — empty string is forwarded to bundler", - "language_server.rs line 244: `gemset.installed_gem_version(Self::GEM_NAME)` — empty string is forwarded to gemset", - "fuzzy_ruby_server.rs line 10: `const GEM_NAME: &str = \"\";`", - "The override on lines 12-39 fully replaces the default, so there is no current runtime path that reaches those gem calls via FuzzyRubyServer. The risk is latent." - ], - "pre_existing": false - }, - { - "title": "language_server_command sets installation status None before PATH found", - "severity": "P2", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 30, - "why_it_matters": "The trait's `language_server_command` (language_server.rs line 132-135) calls `zed::set_language_server_installation_status(..., None)` after `language_server_binary` returns `Ok`. For gem-based servers this is appropriate because they show `Downloading` status during install. For FuzzyRubyServer the binary is found via PATH only — there is no prior status transition to `CheckingForUpdate` or `Downloading`. Setting status to `None` immediately is correct in the success case. However, if `worktree.which` returns `None` and the error is returned, no installation status is ever set at all (no `CheckingForUpdate`, no failure status), leaving Zed's UI with a stale or empty status. Other extension servers call `set_language_server_installation_status` before attempting discovery. Absence of any status update on failure may cause the UI to appear stuck rather than showing a useful message.", - "autofix_class": "manual", - "owner": "human", - "requires_verification": true, - "suggested_fix": "At the start of `language_server_binary`, call `zed::set_language_server_installation_status(language_server_id, &zed::LanguageServerInstallationStatus::CheckingForUpdate)`. On the error path (binary not found), set an appropriate failure status before returning `Err(...)` so Zed's UI reflects the missing binary.", - "confidence": 0.65, - "evidence": [ - "language_server.rs lines 132-135: `set_language_server_installation_status(..., None)` is called only on success", - "language_server.rs lines 239-242: the gem path calls `CheckingForUpdate` before discovery", - "fuzzy_ruby_server.rs line 38: `Err(\"fuzzy not found...\".into())` returns without setting any status", - "Behavior depends on how Zed handles missing status updates — requires verification against Zed's extension runtime." - ], - "pre_existing": false - }, - { - "title": "Custom binary path ignores user-supplied arguments when path is set", - "severity": "P3", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 20, - "why_it_matters": "When a user configures a custom binary `path` in LSP settings, the code at lines 20-28 returns early with `args: binary.arguments.clone()`. If the user sets a `path` but omits `arguments`, `binary.arguments` is `None`, and the returned `LanguageServerBinary` has `args: None`. In `language_server_command` (language_server.rs line 139), `binary.args.unwrap_or(self.get_executable_args(worktree))` falls back to `get_executable_args`, which returns `Vec::new()` for `FuzzyRubyServer`. This means a user who sets only a custom path gets an empty argument list. This mirrors the pattern of all other servers in the codebase (same logic in the trait default), so it is consistent — but for a binary that might require arguments to start in LSP mode, this could cause a silent failure where the process starts but does not speak LSP.", - "autofix_class": "advisory", - "owner": "human", - "requires_verification": true, - "suggested_fix": "Document whether `fuzzy_ruby_server` requires any arguments when launched in LSP mode. If default arguments are needed, override `get_executable_args` to return them, which will then serve as the fallback in `language_server_command` whenever `args` is `None`.", - "confidence": 0.61, - "evidence": [ - "fuzzy_ruby_server.rs lines 22-26: returns `args: binary.arguments.clone()` which is `None` when user omits arguments", - "language_server.rs line 139: `binary.args.unwrap_or(self.get_executable_args(worktree))`", - "fuzzy_ruby_server.rs: no override of `get_executable_args`, so the fallback is `Vec::new()`" - ], - "pre_existing": false - }, - { - "title": "Test for language_server_binary with PATH hit is missing", - "severity": "P3", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 48, - "why_it_matters": "The three tests only verify constants and that `get_executable_args` returns an empty vec. No test exercises `language_server_binary` itself: not the custom-path branch, not the PATH-found branch, and not the error branch. The FakeWorktree type is available and supports `lsp_binary_settings`, so these paths are testable. Without coverage, a future refactor that accidentally removes the PATH-only override and falls back to the gem-based default would not be caught by tests.", - "autofix_class": "manual", - "owner": "human", - "requires_verification": false, - "suggested_fix": "Add unit tests that: (1) configure a custom binary path via `FakeWorktree::add_lsp_binary_setting` and assert the path is returned; (2) simulate `worktree.which` returning `Some(path)` and assert the PATH binary is returned; (3) simulate neither configured path nor PATH binary and assert the error message contains the cargo install hint.", - "confidence": 0.90, - "evidence": [ - "fuzzy_ruby_server.rs lines 48-68: only three tests, none call `language_server_binary`", - "language_server.rs lines 59-113: `FakeWorktree` supports `add_lsp_binary_setting` and file stubs, making these tests feasible", - "The critical override logic on lines 12-39 has zero test coverage" - ], - "pre_existing": false - } - ], - "residual_risks": [ - "If `worktree.which` is non-deterministic across environments (e.g., Cargo bin dir not in Zed's PATH), users will hit the error message even with `fuzzy_ruby_server` installed. The error text is correct (cargo install hint) but offers no guidance on PATH configuration for Zed specifically.", - "The executable name `fuzzy` (EXECUTABLE_NAME = \"fuzzy\") is very short and could shadow an unrelated binary named `fuzzy` on the user's PATH, causing the wrong binary to be launched silently. The trait dispatches on `SERVER_ID` but executes whatever `which(\"fuzzy\")` returns." - ], - "testing_gaps": [ - "No test for the `language_server_binary` custom-path branch (lsp_settings.binary.path set).", - "No test for the `language_server_binary` PATH-found branch (worktree.which returns Some).", - "No test for the `language_server_binary` error branch (binary not on PATH, no custom path configured).", - "No integration/smoke test verifying that the SERVER_ID `\"fuzzy-ruby-server\"` registered in extension.toml matches the match arm in ruby.rs and the constant in fuzzy_ruby_server.rs (currently verified only by manual string comparison)." - ] -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json deleted file mode 100644 index 5b518b2..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/maintainability.json +++ /dev/null @@ -1,61 +0,0 @@ -{ - "reviewer": "maintainability", - "findings": [ - { - "title": "GEM_NAME forced to empty string to satisfy a trait contract that does not apply", - "severity": "high", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 10, - "confidence": 0.92, - "autofix_class": "refactor", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Introduce a second trait — e.g. `NonGemLanguageServer` or a trait without `GEM_NAME` — for binary-only servers, or make `GEM_NAME` an `Option<&str>` defaulting to `None`. The current empty-string sentinel is a lie the compiler cannot catch: future callers of `Self::GEM_NAME` will silently receive `\"\"` and may pass it to `gem install`, `bundler.installed_gem_version`, or similar gem tooling, producing silent failures. At minimum, rename the constant to make the intent explicit and add a compile-time assertion (`const _: () = assert!(Self::GEM_NAME.is_empty() ...)`), but the real fix is removing the constraint from the trait boundary." - }, - { - "title": "language_server_binary override in FuzzyRubyServer duplicates the custom-path + PATH-lookup logic already in the trait default", - "severity": "medium", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 12, - "confidence": 0.85, - "autofix_class": "refactor", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "The first 28 lines of `FuzzyRubyServer::language_server_binary` (custom path from settings, then `worktree.which`) are structurally identical to the top of the trait default and to `try_find_on_path_or_extension_gemset`. Extract a shared helper — e.g. `trait LanguageServer::binary_from_settings_or_path` — that does the settings-path check and PATH lookup, then let both the gem path and the Cargo-binary path call it. This eliminates the need to override the entire method just to skip the gem tail." - }, - { - "title": "FuzzyRubyServer::new() is a trivial constructor on a zero-field struct with no derived Default", - "severity": "low", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 43, - "confidence": 0.82, - "autofix_class": "trivial_fix", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Derive `Default` on `FuzzyRubyServer` and remove the handwritten `new()`. Every other zero-field server in the codebase (Rubocop, Kanayago) follows the same pattern and also omits `Default`, so this is a systemic pattern — but it becomes a maintenance liability here because `ruby.rs` calls `FuzzyRubyServer::new` in `get_or_insert_with`, meaning any future refactor that adds state must touch both the struct and the call site. `#[derive(Default)]` removes the boilerplate without adding cost." - }, - { - "title": "Herb implements language_server_command on the concrete type, shadowing the trait method without overriding it through the trait", - "severity": "medium", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/herb.rs", - "line": 30, - "confidence": 0.80, - "autofix_class": "refactor", - "owner": "author", - "requires_verification": false, - "pre_existing": true, - "suggested_fix": "Herb defines `language_server_command` as an inherent method (not a trait override), so the trait dispatch in ruby.rs calls the inherent version only because the inherent method takes priority. This is fragile: if a caller holds a `&dyn LanguageServer`, the trait version would run instead. The method should be moved into the `impl LanguageServer for Herb` block. This is pre-existing but surfaced because FuzzyRubyServer correctly uses the trait override pattern, making the inconsistency visible." - } - ], - "residual_risks": [ - "The `GEM_NAME: &str = \"\"` sentinel is not enforced by the type system. If `extension_gemset_language_server_binary` is ever called on FuzzyRubyServer (e.g. through a future refactor that adds a fallback chain), it will pass the empty string to `gem install` and `gem list`, producing confusing runtime errors rather than a compile-time or clear panic.", - "The error message in `FuzzyRubyServer::language_server_binary` hardcodes the GitHub URL of the upstream project. If the project moves or is renamed, the error message becomes misleading. There is no test covering the not-found path." - ], - "testing_gaps": [ - "There are no tests for `FuzzyRubyServer::language_server_binary` covering (a) the custom-path-from-settings branch, (b) the `worktree.which` found branch, or (c) the not-found error branch. The existing tests only assert constant values and the default empty args, which do not exercise any resolution logic.", - "There is no test asserting that GEM_NAME being empty does not cause a panic or silent no-op if the gem code path is accidentally reached. A regression could be introduced silently." - ] -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json deleted file mode 100644 index 7487caf..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/metadata.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "run_id": "20260419-231842-d1c7183d", - "branch": "fuzzy-ruby-server", - "head_sha": "896736a37b6e9d3ab9ccd95d17b0b6e14066fa4c", - "verdict": "Ready with fixes", - "completed_at": "2026-04-19T23:21:00Z" -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json deleted file mode 100644 index f696496..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/project-standards.json +++ /dev/null @@ -1,49 +0,0 @@ -{ - "reviewer": "project-standards", - "findings": [ - { - "title": "GEM_NAME is empty string, breaking gem-based fallback resolution", - "severity": "high", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 10, - "confidence": 0.92, - "autofix_class": "manual", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "fuzzy_ruby_server is not a RubyGem (it is installed via `cargo install`), so the shared trait's gem-based resolution path in `language_server_binary` / `extension_gemset_language_server_binary` will silently pass an empty string to every gem lookup call (e.g. `bundler.installed_gem_version(\"\", ...)` and `gemset.installed_gem_version(\"\", ...)`). The established pattern for all gem-backed servers (kanayago, rubocop, solargraph, sorbet, steep) sets GEM_NAME to the actual gem name. Since fuzzy_ruby_server is Cargo-installed rather than gem-installed, GEM_NAME cannot meaningfully point to a gem. The correct fix is to give GEM_NAME a sentinel value that makes the intent explicit (e.g. `const GEM_NAME: &str = \"fuzzy_ruby_server\"` paired with a note, or an empty string only if the override in `language_server_binary` fully guards against the gem path being reached). As implemented, the override in `language_server_binary` does skip the trait default, so the empty GEM_NAME is never passed through — but the value is still misleading and fragile: any future refactor that removes the override would silently break gem resolution." - }, - { - "title": "language_server_binary override duplicates trait default logic and bypasses bundler/gemset resolution", - "severity": "medium", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 12, - "confidence": 0.85, - "autofix_class": "manual", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Every peer language server that uses gem-based resolution (kanayago, rubocop, solargraph, sorbet, steep) relies entirely on the trait default `language_server_binary`, which handles the LspSettings binary override, bundler lookup, PATH lookup, and extension gemset install in that order. FuzzyRubyServer re-implements only the first two steps (LspSettings binary override + `worktree.which()`) and omits the bundler and gemset steps, which is correct for a Cargo-installed binary but the duplication is fragile: if the trait default's LspSettings handling changes, this override will diverge silently. The established pattern for servers that need non-gem resolution (see Herb, which uses npm) is to override `language_server_command` instead and include a clear comment explaining why. Consider either (a) overriding `language_server_command` instead of `language_server_binary` to match how Herb handles its non-gem installation, or (b) documenting in a comment on the override why the gem/bundler paths are intentionally skipped." - }, - { - "title": "Import style inconsistent with simple gem-backed peers", - "severity": "low", - "file": "src/language_servers/fuzzy_ruby_server.rs", - "line": 1, - "confidence": 0.72, - "autofix_class": "manual", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Kanayago and Rubocop (the closest structural peers) import only `use super::{language_server::WorktreeLike, LanguageServer}`. FuzzyRubyServer imports `use super::language_server::LanguageServerBinary` and `use super::LanguageServer` as separate lines, and does not import `WorktreeLike` (because `get_executable_args` is not overridden with a generic bound). This is a consequence of the `language_server_binary` override (finding #2). Resolving finding #2 would bring the imports back into line with peers." - } - ], - "residual_risks": [ - "If the `language_server_binary` override is removed in a future refactor (e.g. to consolidate resolution logic), the empty GEM_NAME will cause silent failures in the gem-based fallback path — `installed_gem_version(\"\", ...)` will not error loudly and the extension will fall through to an unhelpful error message.", - "The error message on line 38 (`'fuzzy not found. Install with: cargo install ...'`) is only reached when `worktree.which(\"fuzzy\")` fails. Users who have the binary installed under a different name or in a PATH location not visible to the worktree will receive this message with no PATH diagnostic information, unlike the trait default which provides more context via the gemset path." - ], - "testing_gaps": [ - "There is no test covering the fallback error path (when `worktree.which(\"fuzzy\")` returns None and no LspSettings binary is configured). The existing `test_executable_args` test only exercises the happy-path `get_executable_args` result. A test using a FakeWorktree that returns None for `which()` would validate the error message string, consistent with how other servers test their error cases.", - "There is no test asserting that GEM_NAME has the expected value (unlike SERVER_ID and EXECUTABLE_NAME which are both tested). Given that GEM_NAME is empty and this is unusual, an explicit test with a comment explaining the empty value would prevent silent breakage if the constant is changed." - ] -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json deleted file mode 100644 index 9195b0e..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/reliability.json +++ /dev/null @@ -1,37 +0,0 @@ -{ - "reviewer": "reliability", - "findings": [ - { - "title": "Custom binary path returned without existence or executability check", - "severity": "medium", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 22, - "confidence": 0.82, - "autofix_class": "logic_fix", - "owner": "author", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "Before returning Ok with the user-configured path, verify the path exists and is executable using std::fs::metadata or std::path::Path::is_file. Return a descriptive Err if the configured path is not found, e.g.: `if !std::path::Path::new(path).is_file() { return Err(format!(\"fuzzy-ruby-server: configured binary path '{}' does not exist or is not a file\", path).into()); }`. This converts a later opaque OS process-spawn error into a clear actionable message at configuration time." - }, - { - "title": "PATH-based discovery silently fails when cargo bin dir is not in Zed shell env", - "severity": "low", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 30, - "confidence": 0.68, - "autofix_class": "documentation", - "owner": "author", - "requires_verification": true, - "pre_existing": false, - "suggested_fix": "Zed's shell env may not include ~/.cargo/bin because Zed launches with a login shell that doesn't always source .profile or .cargo/env. The error message on line 38 is accurate but users may not understand why `which` fails even though `fuzzy` works in their terminal. Consider augmenting the error message to note that Zed may not inherit the full PATH and suggest setting the binary path explicitly in LSP settings as a fallback: append ' If fuzzy is installed but not found, set the path explicitly in your Zed LSP settings.' to the error string." - } - ], - "residual_risks": [ - "If Zed's worktree.shell_env() does not include the user's cargo bin directory (a common issue on macOS with GUI-launched apps), fuzzy_ruby_server will always fall through to the Err branch even when the binary is installed. This is not a code defect but it will cause repeated failed activations on every file open until the user manually configures the binary path.", - "The base trait language_server_command calls set_language_server_installation_status with None only on success. On the Err path (binary not found), no status is set, so the Zed UI may display a stale installation status from a previous successful run. This is inherent to the base trait design and not fixable in the override alone." - ], - "testing_gaps": [ - "There are no tests exercising the custom binary path code path (lsp_settings.binary.path set). The existing tests only cover SERVER_ID, EXECUTABLE_NAME, and get_executable_args. A test that injects a non-existent custom path should assert that an appropriate error is returned rather than Ok.", - "There is no test for the which() fallback path returning a binary. Since FakeWorktree does not implement worktree.which(), the PATH lookup branch (lines 30-36) cannot be unit-tested with the current test infrastructure. The FakeWorktree would need a which() mock method to cover this branch." - ] -} diff --git a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json b/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json deleted file mode 100644 index b37561e..0000000 --- a/.context/compound-engineering/ce-code-review/20260419-231842-d1c7183d/testing.json +++ /dev/null @@ -1,53 +0,0 @@ -{ - "reviewer": "testing", - "findings": [ - { - "title": "All three branches of language_server_binary are untested: custom path, PATH lookup, and error", - "severity": "P1", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 12, - "confidence": 0.95, - "autofix_class": "manual", - "owner": "candidosg@gmail.com", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "The language_server_binary method has three distinct branches that need separate tests: (1) custom binary path from LspSettings — use FakeWorktree::add_lsp_binary_setting() to inject a path and assert the returned LanguageServerBinary.path matches; (2) PATH-based discovery via worktree.which() — this requires either refactoring language_server_binary to accept a WorktreeLike trait (consistent with how get_executable_args works) so FakeWorktree can stub which(), or adding a which() method to WorktreeLike and FakeWorktree; (3) the error path when worktree.which() returns None — assert that Err containing 'fuzzy not found' is returned. Without these tests, a regression that silently drops the custom-path branch or always errors would not be caught." - }, - { - "title": "test_executable_args asserts the trait default, not FuzzyRubyServer-specific behavior, providing false confidence", - "severity": "P2", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 62, - "confidence": 0.90, - "autofix_class": "advisory", - "owner": "candidosg@gmail.com", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "FuzzyRubyServer does not override get_executable_args, so the test merely confirms the trait default returns an empty Vec. This is already covered by the test_default_executable_args test in language_server.rs. The test adds no signal specific to FuzzyRubyServer. Either remove it to reduce noise, or—if the intent is to lock FuzzyRubyServer to zero args permanently—add a comment documenting why, and include a test that the binary receives no extra args end-to-end (which would require the language_server_binary branch tests above)." - }, - { - "title": "language_server_binary takes &zed::Worktree directly rather than a WorktreeLike trait, structurally preventing unit testing of all three branches", - "severity": "P2", - "file": "/Users/candidogomes/Documents/Projects/zed-extensions-ruby/src/language_servers/fuzzy_ruby_server.rs", - "line": 12, - "confidence": 0.92, - "autofix_class": "manual", - "owner": "candidosg@gmail.com", - "requires_verification": false, - "pre_existing": false, - "suggested_fix": "The trait method signature uses &zed::Worktree (the concrete Zed API type) so FakeWorktree cannot substitute. Add a which(name: &str) -> Option method to the WorktreeLike trait and implement it on both FakeWorktree and the zed::Worktree adapter. Then refactor language_server_binary (in the trait and in FuzzyRubyServer's override) to accept &dyn WorktreeLike or a generic T: WorktreeLike, matching the pattern already used by get_executable_args. This unblocks writing the three unit tests described in the first finding." - } - ], - "residual_risks": [ - "The error message string 'fuzzy not found. Install with: cargo install --git https://github.com/doompling/fuzzy_ruby_server' is user-facing and has no test asserting its exact content. If the URL or phrasing changes silently, users get broken install instructions with no test failure.", - "GEM_NAME is intentionally empty string. No test documents or guards this intentional deviation from every other LanguageServer implementation. If the trait ever adds logic gated on GEM_NAME being non-empty, FuzzyRubyServer silently misbehaves.", - "The custom-path branch copies binary.arguments as-is from LspSettings without sanitization. No test verifies behavior when arguments is None vs Some(vec![]) vs Some(vec![\"--flag\"])." - ], - "testing_gaps": [ - "Branch: lsp_settings.binary.path is Some(path) — custom binary path is returned with correct path, args from settings, and shell_env. Zero tests cover this.", - "Branch: lsp_settings.binary.path is None and worktree.which('fuzzy') returns Some(path) — PATH-discovered binary is returned with empty args and shell_env. Zero tests cover this.", - "Branch: both lsp_settings.binary.path is None and worktree.which('fuzzy') returns None — Err containing install instructions is returned. Zero tests cover this.", - "Branch: lsp_settings.binary is Some but binary.path is None (binary settings exist but no path override) — code falls through to the which() check. Zero tests cover this.", - "Integration: FuzzyRubyServer::SERVER_ID arm in ruby.rs language_server_command dispatch is not tested. If the match arm were deleted or the SERVER_ID changed without updating ruby.rs, no test would catch it." - ] -}