Bridge/LSP hardening: shared timeout constant, RFC 3986 encoding test, retry-loop test coverage

CHANGELOG.md

@@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **Shared `DEFAULT_LSP_TIMEOUT` constant** — the 16 handler methods in `bridge::translator` that each duplicated `Duration::from_secs(30)` now share one module-level constant; `get_completions`'s intentionally shorter timeout is now the named `COMPLETIONS_LSP_TIMEOUT` constant. No behavior change. (#231)
+- Regression test pinning RFC 3986 §2.2 percent-encoding of `[`, `]`, `^`, `|`, `{`, `}`, and backtick in the `try_path_to_uri`/`encode_rfc3986_path_chars` `file://` URI conversion; confirms the `url` crate already encodes `{`, `}`, and backtick, so no code change was needed for those three. Scope: covers `file://` URI conversion only — `bridge::resources`'s separate `lsp-diagnostics://` URI construction does not call `encode_rfc3986_path_chars` and is not covered here. (#168)
+- Unit tests for `LspClient::should_retrigger` and its wiring into the `ServerCancelled` (-32802) retry loop: full retry exhaustion returns the original error, `retriggerRequest: false` returns immediately without retrying, and a cancelled-then-successful retry resolves normally. (#161)
 - **`HttpConfig` gains request body and session limits** — Breaking change: HTTP transport now caps POST request body size (`413 Payload Too Large` on overflow, wired into `rmcp`'s built-in body-size enforcement) and concurrent HTTP sessions. The session cap is a hard bound enforced atomically at session creation by a semaphore-backed `SessionManager` wrapper (not inferred from request headers, which cannot reliably distinguish session-creating requests across `rmcp`'s legacy and stateless protocol paths); requests rejected once the cap is reached receive `429 Too Many Requests` with a `Retry-After` header. `HttpConfig` gains a `new(bind, path)` constructor plus `with_max_request_body_bytes`/`with_max_concurrent_sessions` builders and is now `#[non_exhaustive]`; existing `HttpConfig { .. }` struct-literal construction must switch to `HttpConfig::new(..)`. (#243)
 - **Spawned LSP servers no longer inherit mcpls's full environment** — Breaking change: previously `LspServer::spawn` called `tokio::process::Command::new` with no `.env_clear()`/`.env()`/`.envs()`, so every LSP server process (and every tool *it* invokes, e.g. rust-analyzer's `cargo`/`rustc`/`build.rs` children) inherited the entire parent environment by default; separately, the `env` field on `[[lsp_servers]]` config entries was parsed but never applied (dead code). `spawn` now clears the child's environment and passes through only a minimal allowlist — `PATH`, `HOME`, `USERPROFILE`, `TMPDIR`/`TEMP`/`TMP` on every platform, plus `SystemRoot`, `SystemDrive`, `windir`, `APPDATA`, `LOCALAPPDATA`, `ProgramData`, `ProgramFiles`, `COMSPEC`, `PATHEXT`, `NUMBER_OF_PROCESSORS`, `USERNAME` on Windows — for variables actually present in the parent process, then applies `[lsp_servers.env]` on top so configured entries can override the passthrough. If your server relies on an inherited variable outside this allowlist (proxy settings, `SSH_AUTH_SOCK`, toolchain env like `DATABASE_URL`/`LIBCLANG_PATH` read by a `build.rs`, custom `PATH` entries, etc.), add it explicitly under that server's `[lsp_servers.env]` in `mcpls.toml` — see `docs/user-guide/configuration.md#env`. This closes a real information-disclosure risk: any secret or token present in mcpls's own environment was previously leaked to every third-party LSP binary, whether or not it had a legitimate need for it. (#236, #246, #247)
 - **Capability-gated tool dispatch** — every tool with a corresponding optional `ServerCapabilities` field now checks it before dispatching: `get_hover` (`hoverProvider`), `get_definition` (`definitionProvider`), `get_references` (`referencesProvider`), `rename_symbol` (`renameProvider`), `get_completions` (`completionProvider`), `get_document_symbols` (`documentSymbolProvider`), `format_document` (`documentFormattingProvider`), `workspace_symbol_search` (`workspaceSymbolProvider`), `get_code_actions` (`codeActionProvider`), `prepare_call_hierarchy`/`get_incoming_calls`/`get_outgoing_calls` (`callHierarchyProvider`), `get_signature_help` (`signatureHelpProvider`), `go_to_implementation` (`implementationProvider`), `go_to_type_definition` (`typeDefinitionProvider`), and `get_inlay_hints` (`inlayHintProvider`) — returning `Error::CapabilityNotSupported` instead of sending a request the server never claimed to support. `get_diagnostics` is deliberately left ungated, since it already falls back to the push-notification cache on error. For the six handlers that open a document, the check now runs before `textDocument/didOpen` is sent, so a rejected server never observes the open notification. The check is based solely on the `ServerCapabilities` snapshot taken during `initialize` — a server that only advertises a capability later via dynamic `client/registerCapability` will still be rejected as unsupported. (#240)

crates/mcpls-core/src/bridge/state.rs

@@ -833,6 +833,12 @@ fn windows_rooted_path_to_file_url(path: &Path) -> Option<Url> {
     Some(file_url)
 }
 
+/// Percent-encodes the RFC 3986 §2.2 "other reserved" characters that the
+/// `url` crate's default WHATWG path percent-encode set leaves untouched:
+/// `[`, `]`, `^`, `|`. The remaining three characters in that set -- `{`,
+/// `}`, and backtick -- are already encoded by `url` on serialization, so
+/// they need no handling here; see
+/// `test_path_to_uri_percent_encodes_all_rfc3986_other_reserved_chars`.
 fn encode_rfc3986_path_chars(url: &Url) -> String {
     let prefix = url[..url::Position::BeforePath].to_owned();
     let encoded = url[url::Position::BeforePath..]
@@ -1408,6 +1414,39 @@ mod tests {
         assert_eq!(uri_to_path(&uri).as_deref(), Some(path));
     }
 
+    #[test]
+    fn test_path_to_uri_percent_encodes_all_rfc3986_other_reserved_chars() {
+        // RFC 3986 §2.2 "other reserved" characters. The `url` crate already
+        // percent-encodes `{`, `}`, and backtick when serializing; `[`, `]`,
+        // `^`, `|` are handled explicitly by `encode_rfc3986_path_chars`.
+        #[cfg(windows)]
+        let path = Path::new(r"C:\home\user\test[]^|{}`.ts");
+        #[cfg(not(windows))]
+        let path = Path::new("/home/user/test[]^|{}`.ts");
+
+        let uri = try_path_to_uri(path).unwrap();
+        let uri_str = uri.as_str();
+
+        for (raw, encoded) in [
+            ('[', "%5B"),
+            (']', "%5D"),
+            ('^', "%5E"),
+            ('|', "%7C"),
+            ('{', "%7B"),
+            ('}', "%7D"),
+            ('`', "%60"),
+        ] {
+            assert!(
+                uri_str.contains(encoded),
+                "expected {raw:?} to be percent-encoded as {encoded} in {uri_str}"
+            );
+        }
+        assert!(
+            !uri_str.contains(['[', ']', '^', '|', '{', '}', '`']),
+            "no raw reserved characters should remain in {uri_str}"
+        );
+    }
+
     #[test]
     fn test_document_tracker_concurrent_operations() {
         let mut map = HashMap::new();

crates/mcpls-core/src/bridge/translator.rs

@@ -679,6 +679,16 @@ const MAX_POSITION_VALUE: u32 = 1_000_000;
 /// Maximum allowed range size in lines.
 const MAX_RANGE_LINES: u32 = 10_000;
 
+/// Default timeout for most LSP request/response round trips.
+///
+/// Independent of `LspServerConfig::timeout_seconds`, which only bounds the
+/// `initialize` handshake (see `lsp::lifecycle`) and has no effect on
+/// per-request timeouts. There is currently no way to configure this value.
+const DEFAULT_LSP_TIMEOUT: Duration = Duration::from_secs(30);
+/// Timeout for `textDocument/completion`, kept shorter than
+/// [`DEFAULT_LSP_TIMEOUT`] since completions are latency-sensitive.
+const COMPLETIONS_LSP_TIMEOUT: Duration = Duration::from_secs(10);
+
 /// Validate that `path` is within one of `workspace_roots`.
 ///
 /// Free function (rather than a `Translator` method) so callers that only need
@@ -1270,9 +1280,8 @@ impl Translator {
             work_done_progress_params: WorkDoneProgressParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Hover> = client
-            .request("textDocument/hover", params, timeout_duration)
+            .request("textDocument/hover", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let result = match response {
@@ -1326,9 +1335,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::GotoDefinitionResponse> = client
-            .request("textDocument/definition", params, timeout_duration)
+            .request("textDocument/definition", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let locations = match response {
@@ -1397,9 +1405,8 @@ impl Translator {
             },
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<lsp_types::Location>> = client
-            .request("textDocument/references", params, timeout_duration)
+            .request("textDocument/references", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let locations = response.unwrap_or_default();
@@ -1454,9 +1461,8 @@ impl Translator {
 
         let params = diagnostic_request_params(TextDocumentIdentifier { uri: uri.clone() });
 
-        let timeout_duration = Duration::from_secs(30);
         let pull_response: Result<lsp_types::DocumentDiagnosticReportResult> = client
-            .request("textDocument/diagnostic", params, timeout_duration)
+            .request("textDocument/diagnostic", params, DEFAULT_LSP_TIMEOUT)
             .await;
 
         let diag_info = {
@@ -1523,9 +1529,8 @@ impl Translator {
             work_done_progress_params: WorkDoneProgressParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<WorkspaceEdit> = client
-            .request("textDocument/rename", params, timeout_duration)
+            .request("textDocument/rename", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let changes = if let Some(edit) = response {
@@ -1627,9 +1632,8 @@ impl Translator {
             context,
         };
 
-        let timeout_duration = Duration::from_secs(10);
         let response: Option<lsp_types::CompletionResponse> = client
-            .request("textDocument/completion", params, timeout_duration)
+            .request("textDocument/completion", params, COMPLETIONS_LSP_TIMEOUT)
             .await?;
 
         let items = match response {
@@ -1686,9 +1690,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::DocumentSymbolResponse> = client
-            .request("textDocument/documentSymbol", params, timeout_duration)
+            .request("textDocument/documentSymbol", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let symbols = match response {
@@ -1747,9 +1750,8 @@ impl Translator {
             work_done_progress_params: WorkDoneProgressParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<lsp_types::TextEdit>> = client
-            .request("textDocument/formatting", params, timeout_duration)
+            .request("textDocument/formatting", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let edits = response.unwrap_or_default();
@@ -1860,9 +1862,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<lsp_types::SymbolInformation>> = client
-            .request("workspace/symbol", params, timeout_duration)
+            .request("workspace/symbol", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let mut symbols: Vec<WorkspaceSymbol> = response
@@ -1956,9 +1957,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::CodeActionResponse> = client
-            .request("textDocument/codeAction", params, timeout_duration)
+            .request("textDocument/codeAction", params, DEFAULT_LSP_TIMEOUT)
             .await?;
         let response_vec = response.unwrap_or_default();
         let mut actions = Vec::with_capacity(response_vec.len());
@@ -2031,12 +2031,11 @@ impl Translator {
             work_done_progress_params: WorkDoneProgressParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<CallHierarchyItem>> = client
             .request(
                 "textDocument/prepareCallHierarchy",
                 params,
-                timeout_duration,
+                DEFAULT_LSP_TIMEOUT,
             )
             .await?;
 
@@ -2084,9 +2083,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<CallHierarchyIncomingCall>> = client
-            .request("callHierarchy/incomingCalls", params, timeout_duration)
+            .request("callHierarchy/incomingCalls", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         // Pre-allocate and build result
@@ -2142,9 +2140,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<CallHierarchyOutgoingCall>> = client
-            .request("callHierarchy/outgoingCalls", params, timeout_duration)
+            .request("callHierarchy/outgoingCalls", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         // Pre-allocate and build result
@@ -2383,9 +2380,8 @@ impl Translator {
             context: None,
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::SignatureHelp> = client
-            .request("textDocument/signatureHelp", params, timeout_duration)
+            .request("textDocument/signatureHelp", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let result = match response {
@@ -2466,9 +2462,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::GotoDefinitionResponse> = client
-            .request("textDocument/implementation", params, timeout_duration)
+            .request("textDocument/implementation", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         Ok(LocationsResult {
@@ -2518,9 +2513,8 @@ impl Translator {
             partial_result_params: PartialResultParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<lsp_types::GotoDefinitionResponse> = client
-            .request("textDocument/typeDefinition", params, timeout_duration)
+            .request("textDocument/typeDefinition", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         Ok(LocationsResult {
@@ -2573,9 +2567,8 @@ impl Translator {
             work_done_progress_params: WorkDoneProgressParams::default(),
         };
 
-        let timeout_duration = Duration::from_secs(30);
         let response: Option<Vec<lsp_types::InlayHint>> = client
-            .request("textDocument/inlayHint", params, timeout_duration)
+            .request("textDocument/inlayHint", params, DEFAULT_LSP_TIMEOUT)
             .await?;
 
         let hints = response