Bridge/LSP hardening: shared timeout constant, RFC 3986 encoding test, retry-loop test coverage - #264
Merged
Merged
Conversation
This was
linked to
issues
Aug 4, 2026
bug-ops
force-pushed
the
bridge-lsp-hardening
branch
from
August 4, 2026 19:59
51bc4d5 to
65600ac
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens mcpls’s MCP↔LSP bridge by (1) centralizing the default per-request LSP timeouts, (2) adding a regression test for RFC 3986 reserved-character percent-encoding in file:// URI conversion, and (3) adding unit tests that pin ServerCancelled (-32802) retry-loop behavior in the LSP client.
Changes:
- Extract shared
DEFAULT_LSP_TIMEOUT/COMPLETIONS_LSP_TIMEOUTconstants inbridge::translatorand update docs to clarifytimeout_secondsonly applies to theinitializehandshake. - Add regression coverage ensuring all RFC 3986 §2.2 “other reserved” characters are percent-encoded in
try_path_to_uri/encode_rfc3986_path_chars. - Add tests for
LspClient::should_retrigger()and end-to-end retry-loop outcomes (exhaustion, short-circuit onretriggerRequest: false, and retry-success).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/user-guide/troubleshooting.md | Clarifies what timeout_seconds affects and gives better guidance for large-project initialization vs post-init timeouts. |
| docs/user-guide/configuration.md | Corrects timeout_seconds documentation to “initialize-only” and distinguishes it from fixed per-request timeouts. |
| crates/mcpls-core/src/lsp/client.rs | Adds should_retrigger() and retry-loop unit tests using a fake stdio transport. |
| crates/mcpls-core/src/bridge/translator.rs | Introduces shared timeout constants and replaces repeated Duration::from_secs(...) literals. |
| crates/mcpls-core/src/bridge/state.rs | Adds RFC 3986 percent-encoding regression test and expands/clarifies encoding documentation. |
| CHANGELOG.md | Documents the timeout constant extraction and the new regression/unit test coverage. |
Translator's LSP round-trip handlers each hardcoded Duration::from_secs(30) (16 occurrences) or, for get_completions, Duration::from_secs(10). Extract DEFAULT_LSP_TIMEOUT and COMPLETIONS_LSP_TIMEOUT module constants so the default timeout policy is discoverable and changeable in one place instead of requiring a repo-wide find-and-replace. Also correct the timeout_seconds documentation in configuration.md and troubleshooting.md, which implied it bounds per-request LSP round trips; it only bounds the initial initialize handshake (lsp/lifecycle.rs). Per-request timeouts are not currently configurable, which the new constants' doc comments now state explicitly. Closes #231
encode_rfc3986_path_chars manually percent-encodes [, ], ^, and |
after building the URL via Url::from_file_path/Url::parse. RFC
3986 section 2.2 also lists {, }, and backtick as reserved
characters that must not appear unencoded in a path segment.
Add a test constructing a path with all seven characters and
asserting each is percent-encoded in the resulting URI. The url
crate already percent-encodes backtick, {, and } via its WHATWG
path percent-encode set on serialization, so no additional
replace() calls are needed; the four existing calls already cover
the remaining [, ], ^, |. Document this split in a comment on the
function, since its name promises all seven characters while the
body only explicitly handles four.
Closes #168
… path PR #160 added should_retrigger() and a retry loop for ServerCancelled (-32802) responses, but neither the gate function nor its wiring into the retry loop had test coverage. Add unit tests for should_retrigger() covering the None, retriggerRequest: false, and retriggerRequest: true cases, plus a retry_behavior submodule with a fake dual-process LSP transport covering: full retry exhaustion after 3 attempts returns the original ServerCancelled error verbatim, retriggerRequest: false short-circuits the retry loop immediately with no second request sent, and a cancelled-then-successful retry resolves normally with a fresh request id. Closes #161
bug-ops
force-pushed
the
bridge-lsp-hardening
branch
from
August 4, 2026 20:07
65600ac to
36cf8d3
Compare
bug-ops
enabled auto-merge (squash)
August 4, 2026 20:08
6 tasks
bug-ops
added a commit
that referenced
this pull request
Aug 4, 2026
`timeout_seconds` only ever bounded the `initialize` handshake; every per-request LSP round trip (hover, definition, references, etc.) used fixed constants that users could not tune, despite pre-#264 docs implying otherwise. Adds `request_timeout_seconds` on `LspServerConfig` as a distinct, per-request knob, reached via new `LspClient` accessors with no `Translator` signature changes. Completion requests remain capped at 10s regardless of the configured value, since they are the most latency-sensitive tool and the MCP client's own timeout leaves no margin for a longer wait. BREAKING CHANGE: `LspServerConfig` gains a new `pub` field `request_timeout_seconds` (default 30, additive under deny_unknown_fields so existing TOML configs are unaffected). `ServerConfig::validate()` now rejects `timeout_seconds == 0` and `request_timeout_seconds == 0`. Closes #267
bug-ops
added a commit
that referenced
this pull request
Aug 4, 2026
* feat(lsp): add per-request LSP timeout configuration `timeout_seconds` only ever bounded the `initialize` handshake; every per-request LSP round trip (hover, definition, references, etc.) used fixed constants that users could not tune, despite pre-#264 docs implying otherwise. Adds `request_timeout_seconds` on `LspServerConfig` as a distinct, per-request knob, reached via new `LspClient` accessors with no `Translator` signature changes. Completion requests remain capped at 10s regardless of the configured value, since they are the most latency-sensitive tool and the MCP client's own timeout leaves no margin for a longer wait. BREAKING CHANGE: `LspServerConfig` gains a new `pub` field `request_timeout_seconds` (default 30, additive under deny_unknown_fields so existing TOML configs are unaffected). `ServerConfig::validate()` now rejects `timeout_seconds == 0` and `request_timeout_seconds == 0`. Closes #267 * fix(bridge): add request_timeout_seconds to rebase-introduced test fixture Rebasing onto main pulled in a new LspServerConfig struct literal (added by #268's workspace_symbol_search regression test) that predates request_timeout_seconds and didn't compile after the rebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Grouped triage batch of three low-priority (P3) issues, all touching LSP request-handling internals in
mcpls-core(bridge/lsp modules), no cross-dependencies:DEFAULT_LSP_TIMEOUT/COMPLETIONS_LSP_TIMEOUTconstants inbridge::translator, replacing 17 duplicatedDuration::from_secs(...)literals. Also correctstimeout_secondsdocumentation inconfiguration.md/troubleshooting.md, which incorrectly implied it bounds per-request LSP timeouts — it only bounds theinitializehandshake.encode_rfc3986_path_chars/try_path_to_uri. Confirms the underlying bug was already fixed as a side effect of fix(core): encode reserved path chars in file URIs #151 (theurlcrate already percent-encodes backtick,{,}on serialization); no production code change needed for those three characters.LspClient::should_retrigger()and its wiring into theServerCancelled(-32802) retry loop: gate-function cases, full retry exhaustion, immediate short-circuit onretriggerRequest: false, and the retry-success happy path.Out of scope (follow-up issues to be filed)
bridge::resources::make_uribuildslsp-diagnostics://URIs without going throughencode_rfc3986_path_chars, so[ ] ^ |can still leak unencoded on that path. Outside encode_rfc3986_path_chars: incomplete character set (missing { } and backtick) #168's literal scope (which names onlyencode_rfc3986_path_chars); CHANGELOG wording is scoped accordingly.state.rs,translator.rs,client.rs); a shared test-utils module would reduce duplication but is out of scope for this batch.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features --lib --bins(513 passed, 0 failed)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --workspace