Repository navigation
[ty] Add untrustedWorkspace option to the LSP - #27828
Conversation
MichaReiser
added
server
MichaReiser
changed the title
untrustedWorkspace option to the LSP
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Should these tests be part of https://github.com/astral-sh/ruff/blob/main/crates/ty_server/tests/e2e/initialize.rs instead?
There was a problem hiding this comment.
I can look into making them e2e tests on top of the scroll support. The issue is that we currently have no behavior that changes based on the untrusted setting. The only way of asserting that the workspace is untrusted is by reading the value from the setting.
The other issue might be that our server setup doesn't allow us to pass invalid settings. But there might be a way around this
| let (initialization_options, deserialization_error) = | ||
| InitializationOptions::from_value(initialization_options); | ||
| match InitializationOptions::from_value(initialization_options) { | ||
| Ok(options) => options, | ||
| Err(error) => { | ||
| connection.sender.send(Message::Response(Response::new_err( | ||
| id, | ||
| ErrorCode::InvalidParams as i32, | ||
| format!("Invalid initialization options: {error:#}"), | ||
| )))?; | ||
| return Err(error).context("Failed to deserialize initialization options"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
It seems like we're hard-stopping the server initialization if the untrustedWorkspace deserialization fails, is that intentional? Should we still start the server and limit the capabilities instead?
We should also add a test case where the untrustedWorkspace value is invalid to demonstrate the deserialization failure case.
There was a problem hiding this comment.
We could fall back to untrusted. But I'm not sure it gives you much. I think this is really a case where you should fix your settings.
There's already a test, it's malformed_trust_rejects_initialization
There was a problem hiding this comment.
Oh, is this actually intentional to not start the server for untrusted workspace?
Sounds good!
3 participants
Summary
This is a generic stop-gap solution for astral-sh/ty#4275.
This PR adds a new
untrustedWorkspaceinitialization option and changes theLSPSystemto always return an IOError when trying to run a command. This seems like a safe default and is all we need for now.If we have a need in the future to run some commands in untrusted workspace, we can then extend
run_commandand add an option to bypass the early return.I plan to add dedicated handling of untrusted workspaces for astral-sh/ty#4275 in the scripts LSP, to always disables uv support in an untrusted workspace. Because there's no point in showing an error that running
uvisn't allowed in untrusted workspace for every script. But I still think this is worth landing independently as a general stop-gap solution for other cases where we might forget to add a dedicated check.This PR takes extra care that we always respect
untrustedWorkspace, even if otherinitializationOptionsare invalid. I think it would be nice to have a more general system where ty tries to retain as many valid options as possible instead of falling back to the default when any option is invalid, but that felt like a separate change.Test Plan
Added tests