Avoid running `uv format` in untrusted workspaces by ntBre · Pull Request #28873 · astral-sh/ruff

ntBre · 2026-09-24T16:03:00Z

Summary

This PR adds the --untrusted-workspace flag to the server CLI, which informs the server that it's running in an untrusted workspace. When this is the case, we fall back on the internal formatting backend instead of using uv format to avoid reading uv configuration and potentially installing packages based on it.

Test Plan

New server tests

Summary -- This PR adds the `--untrusted-workspace` flag to the server CLI, which informs the server that it's running in an untrusted workspace. When this is the case, we fall back on the internal formatting backend instead of using `uv format` to avoid reading uv configuration and potentially installing packages based on it. Test Plan -- New server tests

astral-sh-bot · 2026-09-24T16:05:05Z

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 98.24%. The percentage of expected errors that received a diagnostic held steady at 98.24%. The number of fully passing files held steady at 134/146.

astral-sh-bot · 2026-09-24T16:06:53Z

Memory usage report

Memory usage unchanged ✅

astral-sh-bot · 2026-09-24T16:08:18Z

`ecosystem-analyzer` results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

astral-sh-bot · 2026-09-24T16:14:30Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

MichaReiser · 2026-09-24T16:36:24Z

+            match Server::new(
+                worker_threads,
+                server_connection,
+                None,
+                WorkspaceTrust::Trusted,
+                true,
+            ) {


Could we add a TestServer::untrusted factory method with an internal, shared new_impl? Or are there other reasons why it can't be a e2e test?

My understanding is that the main issue is determining what to assert at the end of the test, not setting up the test server initially. There's no user-visible output that we're using one backend or the other (except the new logging call). So it makes it hard to assert something useful in the e2e tests. But you'd know better about our e2e test capabilities than me, I could certainly be missing something.

Codex started producing something like this when I asked for an e2e test, which is what I thought looked too janky to include:

fn without_uv(body: impl FnOnce() -> Result<()>) -> Result<()> { const CHILD: &str = "RUFF_TEST_ISOLATED_CHILD"; let thread = std::thread::current(); let name = thread.name().context("missing test name")?; if std::env::var(CHILD).as_deref() == Ok(name) { return body(); } let empty_path = tempfile::tempdir()?; let output = Command::new(std::env::current_exe()?) .args(["--exact", name, "--nocapture"]) .env(CHILD, name) .env("PATH", empty_path.path()) .current_dir(empty_path.path()) .output()?; ensure!(output.status.success(), "{}\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); println!("{}", String::from_utf8_lossy(&output.stdout)); Ok(()) }

plus another ~50 lines of helpers.

We can of course assert that the code gets formatted somehow, which Codex also tried initially, but that didn't seem very useful, as both backends format code.

Can you override path and have uv format fail?

Yes, that's what Codex's snippet above was trying to do. I'll just push its patch so you can see what you think. Maybe it's not as bad as I thought.

Ah I see, ty's server tests have more indirection here, through TestServerBuilder::with_env_var and the system stuff. Here, we end up calling straight into Command::new("uv"), which makes it much harder to inject environment variables at the right point, or without modifying other tests, which is what Codex is trying to avoid.

Anyway, I've included Codex's e2e test scheme. We could also do something more like ty, although it feels like that probably belongs in a separate PR.

Ahh, I forgot that we're in Ruff's server where we don't have the ability to easily override commands. Hmm, I don't see a great solution other than plumbing through the argument somehow (e.g. by storing a CommandExecutor on Server). I'm not sure this is worth it for this change, or, at least, might be better as a follow up. Thanks for giving it a try

No problem! I'll revert this last commit and merge then :)

MichaReiser

Sorry. It looks like I forgot to submit my feedback on Friday.

MichaReiser · 2026-09-25T06:23:16Z

+/// Isolate PATH from other tests, which run their servers in the same process.
+/// Without uv, formatting only succeeds if the internal backend is used.
+fn without_uv(body: impl FnOnce() -> Result<()>) -> Result<()> {
+    const CHILD: &str = "RUFF_TEST_ISOLATED_CHILD";
+    let thread = std::thread::current();
+    let name = thread.name().context("missing test name")?;
+    if std::env::var(CHILD).as_deref() == Ok(name) {
+        return body();
+    }
+
+    let empty_path = tempfile::tempdir()?;
+    let output = Command::new(std::env::current_exe()?)
+        .args(["--exact", name, "--nocapture"])
+        .env(CHILD, name)
+        .env("PATH", empty_path.path())
+        .current_dir(empty_path.path())
+        .output()?;
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    // An unmatched `--exact` filter exits successfully without running any tests.
+    ensure!(
+        output.status.success() && stdout.contains("test result: ok. 1 passed;"),
+        "{stdout}\n{}",
+        String::from_utf8_lossy(&output.stderr),
+    );
+    Ok(())
+}


This feels a bit crazy to me. Seems a bit annoying to debug and keep compatibility.

MichaReiser · 2026-09-25T06:23:55Z

+        match workspace_trust {
+            WorkspaceTrust::Trusted => {
+                assert_matches!(
+                    document,
+                    Err(AwaitResponseError::RequestFailed(error))
+                        if error.message.contains("uv was not found"),
+                );
+                assert_matches!(
+                    range,
+                    Err(AwaitResponseError::RequestFailed(error))
+                        if error.message.contains("uv was not found"),
+                );
+                server.await_notification::<ShowMessageNotification>();
+                server.await_notification::<ShowMessageNotification>();
+            }
+            WorkspaceTrust::Untrusted => {
+                let expected = Some(vec![TextEdit {
+                    range: Range::new(Position::new(0, 0), Position::new(1, 0)),
+                    new_text: "x = 1\n".to_string(),
+                }]);
+                assert_eq!(document?, expected);
+                assert_eq!(range?, expected);
+            }
+        }


I think I'd prefer a helper function that sets up the test case and then have two separate tests over test_case + a match here

MichaReiser · 2026-09-25T06:41:31Z

+            match Server::new(
+                worker_threads,
+                server_connection,
+                None,
+                WorkspaceTrust::Trusted,
+                true,
+            ) {


Ahh, I forgot that we're in Ruff's server where we don't have the ability to easily override commands. Hmm, I don't see a great solution other than plumbing through the argument somehow (e.g. by storing a CommandExecutor on Server). I'm not sure this is worth it for this change, or, at least, might be better as a follow up. Thanks for giving it a try

This reverts commit f18ceda.

codspeed · 2026-09-28T20:26:58Z

Merging this PR will not alter performance

✅ 222 untouched benchmarks

_{Comparing brent/ruff-untrusted-workspace (bf78f4d) with main (33e920c)}

…#1181) Summary -- Related to astral-sh/ruff#28873. Test Plan -- https://github.com/user-attachments/assets/e4451bd3-befe-46ef-84b4-36c2e6dc4406

ntBre added the server Related to the Ruff or ty LSP server label Sep 24, 2026

MichaReiser approved these changes Sep 24, 2026

View reviewed changes

e2e tests

f18ceda

ntBre mentioned this pull request Sep 24, 2026

Pass --untrusted-workspace to ruff server in untrusted workspaces astral-sh/ruff-vscode#1181

Merged

MichaReiser approved these changes Sep 28, 2026

View reviewed changes

Revert "e2e tests"

e5f9bf3

This reverts commit f18ceda.

Merge branch 'main' into brent/ruff-untrusted-workspace

bf78f4d

ntBre merged commit 6cc0678 into main Sep 28, 2026
73 checks passed

ntBre deleted the brent/ruff-untrusted-workspace branch September 28, 2026 20:47

BrewTestBot mentioned this pull request Oct 1, 2026

ruff 0.16.10 Homebrew/homebrew-core#314834

Merged

kambarakun mentioned this pull request Oct 3, 2026

deps(deps): bump ruff from 0.16.8 to 0.16.9 in the build-tools group kambarakun/fetch-tokyo-idsc-github-actions#772

Merged

github-actions Bot mentioned this pull request Oct 3, 2026

Update pre-commit hook astral-sh/ruff-pre-commit to v0.16.8 claytono/infra#3078

Merged

1 task

github-actions Bot mentioned this pull request Oct 19, 2026

Update pre-commit hook astral-sh/ruff-pre-commit to v0.16.10 claytono/infra#3205

Merged

1 task

Conversation

ntBre commented Sep 24, 2026

Summary

Test Plan

Uh oh!

astral-sh-bot Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No changes detected ✅

Uh oh!

astral-sh-bot Bot commented Sep 24, 2026

Memory usage report

Uh oh!

astral-sh-bot Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ecosystem-analyzer results

Uh oh!

astral-sh-bot Bot commented Sep 24, 2026

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

astral-sh-bot Bot commented Sep 24, 2026 •

edited

Loading

astral-sh-bot Bot commented Sep 24, 2026 •

edited

Loading

`ecosystem-analyzer` results

`ruff-ecosystem` results

codspeed Bot commented Sep 28, 2026 •

edited

Loading