agent: Fix NTFS warning breaking terminal tool when WSL not available (#62049)
There is a bug on main and preview causes all terminal tool calls to fail when: - on windows - sandboxing is not available (i.e. no WSL) - sandboxing is enabled in settings (the default) - the "warn windows-drive grants" setting is enabled (the default) Example error: <img width="352" height="79" alt="image" src="https://github.com/user-attachments/assets/24affbec-f086-40eb-8827-ad74aabc6167" /> This PR fixes it by making sure we only show the check at the right time --- Release Notes: - N/A or Added/Fixed/Improved ...
This commit is contained in:
1 parent
3b56897a51
commit
e5c9663b4f
8 files changed
+123
-12
No files matched your search
Generated
+1
@@ -16200,6 +16200,7 @@ dependencies = [
|
||||
"serde_json",
|
||||
"smol",
|
||||
"tempfile",
|
||||
"windows-registry 0.6.1",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
||||
@@ -897,6 +897,7 @@ web-time = "1.1.0"
|
||||
webrtc-sys = "0.3.23"
|
||||
wgpu = "29.0.4"
|
||||
windows-core = "0.61"
|
||||
windows-registry = "0.6.0"
|
||||
yaml-rust2 = "0.8"
|
||||
yawc = { git = "https://github.com/zed-industries/yawc", rev = "71a452f551cac178367eaac5d7418a09afa1f3a2", version = "0.3.3" }
|
||||
zeroize = "1.8"
|
||||
|
||||
@@ -5498,9 +5498,12 @@ impl ToolCallEventStream {
|
||||
let (events_tx, events_rx) = mpsc::unbounded::<Result<ThreadEvent>>();
|
||||
let (_cancellation_tx, cancellation_rx) = watch::channel(false);
|
||||
|
||||
// The raw and scoped ids deliberately differ (mirroring
|
||||
// `scoped_tool_call_id`) so that conflating them fails tests instead
|
||||
// of silently passing.
|
||||
let stream = ToolCallEventStream::new(
|
||||
"test_id".into(),
|
||||
acp::ToolCallId::new("test_id"),
|
||||
acp::ToolCallId::new("0:test_id"),
|
||||
ThreadEventStream(events_tx),
|
||||
None,
|
||||
cancellation_rx,
|
||||
@@ -5516,9 +5519,12 @@ impl ToolCallEventStream {
|
||||
let (events_tx, events_rx) = mpsc::unbounded::<Result<ThreadEvent>>();
|
||||
let (cancellation_tx, cancellation_rx) = watch::channel(false);
|
||||
|
||||
// The raw and scoped ids deliberately differ (mirroring
|
||||
// `scoped_tool_call_id`) so that conflating them fails tests instead
|
||||
// of silently passing.
|
||||
let stream = ToolCallEventStream::new(
|
||||
"test_id".into(),
|
||||
acp::ToolCallId::new("test_id"),
|
||||
acp::ToolCallId::new("0:test_id"),
|
||||
ThreadEventStream(events_tx),
|
||||
None,
|
||||
cancellation_rx,
|
||||
@@ -5606,6 +5612,11 @@ impl ToolCallEventStream {
|
||||
&self.tool_use_id
|
||||
}
|
||||
|
||||
/// The ACP-facing id for this tool call (see [`scoped_tool_call_id`]).
|
||||
pub fn tool_call_id(&self) -> &acp::ToolCallId {
|
||||
&self.tool_call_id
|
||||
}
|
||||
|
||||
pub fn update_fields(&self, fields: acp::ToolCallUpdateFields) {
|
||||
self.stream
|
||||
.update_tool_call_fields(&self.tool_call_id, fields, None);
|
||||
@@ -5947,7 +5958,12 @@ impl ToolCallEventStream {
|
||||
]);
|
||||
|
||||
let stream = self.stream.clone();
|
||||
let tool_use_id = self.tool_use_id.clone();
|
||||
// The update must target the ACP-facing (scoped) id: an update keyed
|
||||
// by the raw provider id matches no existing tool call, and creating
|
||||
// one from empty fields is rejected downstream ("title is required"),
|
||||
// which tears down the stream and turns the prompt into a phantom
|
||||
// decline.
|
||||
let tool_call_id = self.tool_call_id.clone();
|
||||
cx.spawn(async move |_cx| {
|
||||
let (response_tx, response_rx) = oneshot::channel();
|
||||
if let Err(error) = stream
|
||||
@@ -5955,7 +5971,10 @@ impl ToolCallEventStream {
|
||||
.unbounded_send(Ok(ThreadEvent::ToolCallAuthorization(
|
||||
ToolCallAuthorization {
|
||||
tool_call: acp::ToolCallUpdate::new(
|
||||
tool_use_id.to_string(),
|
||||
tool_call_id,
|
||||
// Leave the title untouched so the card keeps
|
||||
// showing the command (matching the escalation
|
||||
// flow).
|
||||
acp::ToolCallUpdateFields::new(),
|
||||
)
|
||||
.meta(acp_thread::meta_with_sandbox_authorization(details)),
|
||||
@@ -5975,6 +5994,7 @@ impl ToolCallEventStream {
|
||||
let outcome = response_rx
|
||||
.await
|
||||
.map_err(|_| anyhow!("authorization channel closed"))?;
|
||||
ensure_tool_call_authorization_not_interrupted(&outcome)?;
|
||||
match acp_thread::SandboxPermission::from_id(outcome.option_id.0.as_ref()) {
|
||||
Some(acp_thread::SandboxPermission::AllowOnce) => Ok(()),
|
||||
_ => Err(anyhow!("Windows-drive write aborted by user")),
|
||||
@@ -8271,6 +8291,41 @@ mod tests {
|
||||
assert!(!event_stream.sandbox_fallback_granted_for_thread());
|
||||
}
|
||||
|
||||
/// Regression test: the Windows-drive (DrvFs) warning prompt must target
|
||||
/// the ACP-facing *scoped* tool-call id. When it used the raw provider id,
|
||||
/// the update matched no existing tool call, the ACP layer rejected the
|
||||
/// resulting title-less insert ("title is required for a tool call"), the
|
||||
/// event stream was torn down, and every terminal command was auto-declined
|
||||
/// without the user ever seeing a prompt.
|
||||
#[gpui::test]
|
||||
async fn test_windows_fs_warning_targets_scoped_tool_call_id(cx: &mut TestAppContext) {
|
||||
crate::tests::init_test(cx);
|
||||
|
||||
let (event_stream, mut receiver) = ToolCallEventStream::test();
|
||||
let authorize = cx.update(|cx| event_stream.authorize_windows_fs_warning(cx));
|
||||
|
||||
let authorization = receiver.expect_authorization().await;
|
||||
assert_eq!(
|
||||
&authorization.tool_call.tool_call_id,
|
||||
event_stream.tool_call_id(),
|
||||
"the warning prompt must reference the scoped ACP tool-call id, \
|
||||
not the raw provider id"
|
||||
);
|
||||
let details =
|
||||
acp_thread::sandbox_authorization_details_from_meta(&authorization.tool_call.meta)
|
||||
.expect("warning authorization should include sandbox details");
|
||||
assert!(details.warn_windows_fs);
|
||||
|
||||
authorization
|
||||
.response
|
||||
.send(acp_thread::SelectedPermissionOutcome::new(
|
||||
acp::PermissionOptionId::new(acp_thread::SandboxPermission::AllowOnce.as_id()),
|
||||
acp::PermissionOptionKind::AllowOnce,
|
||||
))
|
||||
.unwrap();
|
||||
authorize.await.unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_auto_resolve_permission_outcome_uses_once_only_options() {
|
||||
let options = acp_thread::PermissionOptions::Dropdown(vec![
|
||||
|
||||
@@ -634,7 +634,19 @@ async fn run_terminal_tool(
|
||||
// gate is transient (never persisted): on "Continue" the normal flow
|
||||
// (including any escalation prompt) proceeds; on "Abort" the command is
|
||||
// cancelled. It recurs until the warning is disabled in settings.
|
||||
if sandboxing && !want_unsandboxed && persistent.warn_ntfs_grants {
|
||||
//
|
||||
// The warning only makes sense when a WSL sandbox will actually wrap the
|
||||
// command, so it is skipped when the command is guaranteed to run without
|
||||
// one (`unsandboxed_floor`: the user already turned the sandbox off for
|
||||
// this thread) and when WSL is structurally absent (no registered distro —
|
||||
// sandbox creation is guaranteed to fail, and the creation-failure
|
||||
// fallback prompt handles that conversation instead).
|
||||
if sandboxing
|
||||
&& !want_unsandboxed
|
||||
&& !unsandboxed_floor
|
||||
&& persistent.warn_ntfs_grants
|
||||
&& sandbox::wsl_distro_registered()
|
||||
{
|
||||
let effective = event_stream.effective_sandbox_request(&request, &persistent);
|
||||
let contains_windows_fs = effective
|
||||
.write_paths
|
||||
@@ -650,15 +662,15 @@ async fn run_terminal_tool(
|
||||
.any(|path| sandbox::path_is_on_windows_drive(path))
|
||||
});
|
||||
if contains_windows_fs
|
||||
&& cx
|
||||
&& let Err(error) = cx
|
||||
.update(|cx| event_stream.authorize_windows_fs_warning(cx))
|
||||
.await
|
||||
.is_err()
|
||||
{
|
||||
return Ok(
|
||||
"Command cancelled: the user declined to run a command whose sandbox writes to a Windows drive."
|
||||
.to_string(),
|
||||
);
|
||||
// Carry the underlying error so a prompt-delivery failure is
|
||||
// distinguishable from a genuine user abort.
|
||||
return Ok(format!(
|
||||
"Command cancelled: the user declined to run a command whose sandbox writes to a Windows drive ({error})."
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -54,7 +54,7 @@ zed_actions.workspace = true
|
||||
indoc.workspace = true
|
||||
|
||||
[target.'cfg(target_os = "windows")'.dependencies]
|
||||
windows-registry = "0.6.0"
|
||||
windows-registry.workspace = true
|
||||
|
||||
[dev-dependencies]
|
||||
editor = { workspace = true, features = ["test-support"] }
|
||||
|
||||
@@ -74,3 +74,6 @@ tempfile.workspace = true
|
||||
|
||||
[target.'cfg(target_os = "windows")'.dependencies]
|
||||
smol.workspace = true
|
||||
# Structural WSL-installed check (registered distros under HKCU\...\Lxss)
|
||||
# without spawning `wsl.exe`.
|
||||
windows-registry.workspace = true
|
||||
@@ -141,6 +141,23 @@ pub fn path_is_on_windows_drive(path: &Path) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether WSL is structurally present (at least one registered distro), via a
|
||||
/// cheap registry read with no `wsl.exe` round-trip. Always `false` off
|
||||
/// Windows. Suitable only for UI gating (e.g. suppressing DrvFs
|
||||
/// sandbox-integrity warnings on machines where no WSL sandbox could ever be
|
||||
/// constructed); enforcement paths must keep relying on the real in-WSL probe,
|
||||
/// which is authoritative about whether a sandbox can actually be built.
|
||||
pub fn wsl_distro_registered() -> bool {
|
||||
#[cfg(target_os = "windows")]
|
||||
{
|
||||
windows_wsl::wsl_distro_registered()
|
||||
}
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
{
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
impl SandboxPolicy {
|
||||
/// Combine two policy layers. Filesystem/network grants are unioned into the
|
||||
/// least-restrictive policy that satisfies both layers, while protected paths
|
||||
|
||||
@@ -261,6 +261,28 @@ pub fn path_is_on_windows_drive(path: &Path) -> bool {
|
||||
)
|
||||
}
|
||||
|
||||
/// Whether WSL is structurally present: at least one distro is registered
|
||||
/// under `HKCU\Software\Microsoft\Windows\CurrentVersion\Lxss`. A cheap
|
||||
/// registry read (no `wsl.exe` spawn), suitable for UI gating decisions like
|
||||
/// suppressing sandbox-integrity warnings on machines where no WSL sandbox
|
||||
/// could ever be constructed. It is *not* an availability guarantee: a
|
||||
/// registered distro can still fail the real environment probe (no `bwrap`,
|
||||
/// unhealthy WSL service, …), so enforcement paths must keep relying on
|
||||
/// [`probe_environment`]'s in-WSL round-trip.
|
||||
pub fn wsl_distro_registered() -> bool {
|
||||
windows_registry::CURRENT_USER
|
||||
.open("Software\\Microsoft\\Windows\\CurrentVersion\\Lxss")
|
||||
.and_then(|lxss_key| {
|
||||
Ok(lxss_key.keys()?.any(|key| {
|
||||
lxss_key
|
||||
.open(&key)
|
||||
.and_then(|distro| distro.get_string("DistributionName"))
|
||||
.is_ok()
|
||||
}))
|
||||
})
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
/// Resolve a requested write grant to the path we persist for it, in
|
||||
/// **platform-native** form: a Windows drive path stays `C:\...` (NTFS) and a
|
||||
/// WSL path stays a Linux-absolute `/...`. In both cases the path is resolved to
|
||||
|
||||
Reference in new issue
Block a user