From f9b7101c765890a00ec562d75abb067636c62fad Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Tue, 20 Oct 2026 17:24:50 +0200 Subject: [PATCH 1/2] fix: write Claude Desktop managed settings to the Windows policy registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows Claude Desktop reads its managed configuration from flat REG_SZ values directly under HKLM\SOFTWARE\Policies\Claude, so the managed-settings file under /etc was a silent no-op there. The provider now plans registry value changes instead: strings as they are, other values as their JSON text. A .agentdesktop.owner value lists the names Agentdesktop manages, so updates and removal touch nothing else, and the key is deleted only once nothing else remains in it. Any value under the key — the marker included — counts as machine policy to the app and shadows HKCU policy, so an empty managed set plans removal rather than leaving a marker-only key behind. Value names match case-insensitively, as the registry stores them. Dry runs show the pending registry changes like file diffs, and apply refuses when the key changed after planning. Also drops a redundant clone in agentdesktop-service that clippy flags (clone_on_copy) when building for Windows. Fixes #186. Co-Authored-By: Claude Fable 5 --- crates/agent/Cargo.toml | 1 + crates/agent/src/bin/agentdesktop-service.rs | 2 +- .../src/provider/claude_desktop/discovery.rs | 23 + .../agent/src/provider/claude_desktop/mod.rs | 24 +- .../src/provider/claude_desktop/reconcile.rs | 702 +++++++++++++++++- crates/agent/src/reconcile/mod.rs | 154 +++- crates/agent/src/reconcile/plan.rs | 73 +- crates/agent/src/reconcile/registry.rs | 358 +++++++++ crates/core/src/config.rs | 4 +- schema/daemon-config.json | 2 +- schema/daemon-config.md | 2 +- 11 files changed, 1315 insertions(+), 30 deletions(-) create mode 100644 crates/agent/src/reconcile/registry.rs diff --git a/crates/agent/Cargo.toml b/crates/agent/Cargo.toml index 695324a8..8437ef81 100644 --- a/crates/agent/Cargo.toml +++ b/crates/agent/Cargo.toml @@ -84,4 +84,5 @@ windows-sys = { version = "0.61", features = [ "Win32_Foundation", "Win32_Security", "Win32_Security_Authorization", + "Win32_System_Registry", ] } diff --git a/crates/agent/src/bin/agentdesktop-service.rs b/crates/agent/src/bin/agentdesktop-service.rs index fcc93c77..262f5212 100644 --- a/crates/agent/src/bin/agentdesktop-service.rs +++ b/crates/agent/src/bin/agentdesktop-service.rs @@ -80,7 +80,7 @@ mod windows { )) .context("report Windows service running")?; - let stopping_status = status_handle.clone(); + let stopping_status = status_handle; let shutdown = async move { if !*shutdown_receiver.borrow() { shutdown_receiver diff --git a/crates/agent/src/provider/claude_desktop/discovery.rs b/crates/agent/src/provider/claude_desktop/discovery.rs index de547934..dab3bcc5 100644 --- a/crates/agent/src/provider/claude_desktop/discovery.rs +++ b/crates/agent/src/provider/claude_desktop/discovery.rs @@ -101,6 +101,12 @@ fn discover_mcp_servers(managed: &Path) -> Vec { }) .unwrap_or_default(); + // On Windows the reconciler manages settings as machine policy registry + // values — the only place Claude Desktop reads them — so the managed + // servers are inventoried from there too. + #[cfg(windows)] + servers.extend(registry_managed_mcp_servers()); + let mut paths = BTreeSet::new(); for home in metadata::user_home_dirs() { paths.insert(home.join(".config/Claude/claude_desktop_config.json")); @@ -118,6 +124,23 @@ fn discover_mcp_servers(managed: &Path) -> Vec { servers } +/// The managed MCP servers under Claude Desktop's Windows policy key, where +/// the reconciler writes `managedMcpServers` as a `REG_SZ` JSON string. +#[cfg(windows)] +fn registry_managed_mcp_servers() -> Vec { + let key = super::claude_desktop_policy_key(); + crate::reconcile::registry::read_values(&key) + .ok() + .and_then(|values| values.get("managedMcpServers").cloned().flatten()) + .map(|encoded| { + claude_code::managed_mcp_servers_from_value( + &Value::String(encoded), + &key.display_path(), + ) + }) + .unwrap_or_default() +} + /// Reads Claude Desktop's system-managed settings in the platform's native format — see /// `default_claude_desktop_managed_settings_path`. fn managed_settings(path: &Path) -> Option { diff --git a/crates/agent/src/provider/claude_desktop/mod.rs b/crates/agent/src/provider/claude_desktop/mod.rs index e6a09e58..be540cb4 100644 --- a/crates/agent/src/provider/claude_desktop/mod.rs +++ b/crates/agent/src/provider/claude_desktop/mod.rs @@ -5,6 +5,8 @@ use agentdesktop_core::model::Discovery; use super::{Provider, ReconcileContext}; use crate::reconcile::ReconcilePlan; +#[cfg(windows)] +use crate::reconcile::registry::RegistryKey; pub(super) mod discovery; mod reconcile; @@ -13,6 +15,11 @@ mod reconcile; pub struct ClaudeDesktop { pub managed_settings_path: PathBuf, pub credential_helper_path: PathBuf, + /// The policy registry key the managed settings are reconciled into. + /// Production uses [`claude_desktop_policy_key`]; tests inject a + /// disposable `HKCU` key so they never touch live machine policy. + #[cfg(windows)] + pub(crate) policy_key: RegistryKey, } impl Default for ClaudeDesktop { @@ -20,10 +27,19 @@ impl Default for ClaudeDesktop { Self { managed_settings_path: default_claude_desktop_managed_settings_path(), credential_helper_path: default_claude_desktop_credential_helper_path(), + #[cfg(windows)] + policy_key: claude_desktop_policy_key(), } } } +/// The live machine policy key Claude Desktop reads its managed settings +/// from on Windows. +#[cfg(windows)] +pub(crate) fn claude_desktop_policy_key() -> RegistryKey { + RegistryKey::local_machine(reconcile::POLICY_KEY) +} + impl ClaudeDesktop { pub const ID: &'static str = "claude-desktop"; pub const DISPLAY_NAME: &'static str = "Claude Desktop"; @@ -59,9 +75,8 @@ impl Provider for ClaudeDesktop { }); let plan = ReconcilePlan::default(); reconcile::plan( - &self.managed_settings_path, + self, ctx.merge_user_settings, - &self.credential_helper_path, &ctx.credential_helper, &ctx.socket, configured, @@ -72,6 +87,11 @@ impl Provider for ClaudeDesktop { } /// Returns Claude Desktop's system-managed settings path. +/// +/// On Windows Claude Desktop reads its managed configuration from the +/// `HKLM\SOFTWARE\Policies\Claude` registry key, never from a file, so the +/// reconciler targets that key and only removes what an earlier Agentdesktop +/// wrote at this path. #[cfg(not(target_os = "macos"))] pub fn default_claude_desktop_managed_settings_path() -> PathBuf { PathBuf::from("/etc/claude-desktop/managed-settings.json") diff --git a/crates/agent/src/provider/claude_desktop/reconcile.rs b/crates/agent/src/provider/claude_desktop/reconcile.rs index 7b2982c2..a6d8823d 100644 --- a/crates/agent/src/provider/claude_desktop/reconcile.rs +++ b/crates/agent/src/provider/claude_desktop/reconcile.rs @@ -8,28 +8,50 @@ use serde_json::{Value, json}; use tracing::debug; use crate::reconcile::ReconcilePlan; +#[cfg(any(windows, test))] +use crate::reconcile::registry::{RegistryKey, RegistryKeyPlan, RegistryValues}; #[cfg(any(not(windows), test))] use crate::provider::shared::shell_quote; use crate::provider::{json_merge, shared::deep_merge}; +#[cfg(any(windows, test))] +use std::collections::{BTreeMap, BTreeSet}; const OWNER_MARKER: &[u8] = b"Agentdesktop\n"; +/// The machine policy key Claude Desktop reads on Windows: flat `REG_SZ` +/// values directly under it (the app never reads subkeys). Any value +/// directly under the key — not the key itself — counts as machine policy +/// being present and makes the app ignore `HKCU` policy entirely. +#[cfg(any(windows, test))] +pub(super) const POLICY_KEY: &str = r"SOFTWARE\Policies\Claude"; + +/// The `REG_SZ` value under the policy key that marks what Agentdesktop +/// manages, like the `.owner` sidecar next to a managed file: its data is a +/// JSON array of the value names this daemon wrote, so updates and removal +/// touch nothing else. To Claude Desktop the marker is a policy value like +/// any other — its presence alone switches the app to machine policy — so +/// it is never written on its own: an empty managed set plans removal +/// instead (see [`plan_registry_settings`]). +#[cfg(any(windows, test))] +const REGISTRY_OWNER_VALUE: &str = ".agentdesktop.owner"; + pub(super) fn plan( - settings_path: &Path, + desktop: &ClaudeDesktop, merge_existing: bool, - helper_path: &Path, credential_binary: &Path, socket: &Path, config: Option<(&ClaudeDesktopConfig, Option<&LlmGatewayConfig>)>, plan: &ReconcilePlan, ) -> anyhow::Result<()> { + let settings_path = desktop.managed_settings_path.as_path(); + let helper_path = desktop.credential_helper_path.as_path(); let settings_owner = owner_path(settings_path); let settings_state = json_merge::state_path(settings_path); let helper_owner = owner_path(helper_path); let Some((config, gateway)) = config else { - if !merge_existing - || !json_merge::plan_remove( + if merge_existing + && json_merge::plan_remove( settings_path, &settings_state, "managed settings", @@ -37,9 +59,21 @@ pub(super) fn plan( plan, )? { - remove_owned(settings_path, &settings_owner, "managed settings", plan)?; - } else { remove_owner_marker(&settings_owner, plan)?; + } else { + #[cfg(windows)] + remove_legacy_settings_file(settings_path, &settings_owner, plan)?; + #[cfg(not(windows))] + remove_owned(settings_path, &settings_owner, "managed settings", plan)?; + } + // On Windows the managed settings are machine policy registry + // values; the file above is only what an Agentdesktop without the + // registry target wrote. A user daemon never wrote the policy key. + #[cfg(windows)] + if !merge_existing { + let observed = crate::reconcile::registry::read_values(&desktop.policy_key) + .with_context(|| format!("read {}", desktop.policy_key.display_path().display()))?; + plan_registry_remove(&desktop.policy_key, observed, plan); } return remove_owned(helper_path, &helper_owner, "credential helper", plan); }; @@ -70,10 +104,42 @@ pub(super) fn plan( "Claude Desktop settings cannot be applied to its user preferences; use system-managed settings" ); } - let contents = serialize_managed_settings(&settings)?; + plan_managed_settings(desktop, &settings_owner, &settings, plan) +} + +/// Plans the managed-settings target Claude Desktop reads on this platform: +/// the machine policy registry key on Windows, a managed file everywhere +/// else. +#[cfg(windows)] +fn plan_managed_settings( + desktop: &ClaudeDesktop, + settings_owner: &Path, + settings: &Value, + plan: &ReconcilePlan, +) -> anyhow::Result<()> { + remove_legacy_settings_file(&desktop.managed_settings_path, settings_owner, plan)?; + let observed = crate::reconcile::registry::read_values(&desktop.policy_key) + .with_context(|| format!("read {}", desktop.policy_key.display_path().display()))?; + plan_registry_settings( + &desktop.policy_key, + observed, + ®istry_values(settings)?, + plan, + ); + Ok(()) +} + +#[cfg(not(windows))] +fn plan_managed_settings( + desktop: &ClaudeDesktop, + settings_owner: &Path, + settings: &Value, + plan: &ReconcilePlan, +) -> anyhow::Result<()> { + let contents = serialize_managed_settings(settings)?; write_owned( - settings_path, - &settings_owner, + &desktop.managed_settings_path, + settings_owner, &contents, 0o644, "managed settings", @@ -81,6 +147,22 @@ pub(super) fn plan( ) } +/// Removes the managed-settings file an Agentdesktop without the registry +/// target wrote at the path. Claude Desktop never read a file on Windows, +/// so when no owned file is there nothing is recorded — recording would +/// count the obsolete path as "unchanged" in every dry run. +#[cfg(windows)] +fn remove_legacy_settings_file( + path: &Path, + owner_path: &Path, + plan: &ReconcilePlan, +) -> anyhow::Result<()> { + if !is_owned(owner_path, plan)? { + return Ok(()); + } + remove_owned(path, owner_path, "managed settings", plan) +} + /// Serializes managed settings into the format Claude Desktop actually reads on this /// platform. On macOS that's a property list — `CFPreferencesCopyAppValue` never consults /// a JSON file — everywhere else it's pretty-printed JSON. @@ -92,7 +174,7 @@ fn serialize_managed_settings(settings: &Value) -> anyhow::Result> { Ok(contents) } -#[cfg(not(target_os = "macos"))] +#[cfg(not(any(target_os = "macos", windows)))] fn serialize_managed_settings(settings: &Value) -> anyhow::Result> { let mut contents = serde_json::to_vec_pretty(settings).context("serialize Claude Desktop managed settings")?; @@ -100,6 +182,229 @@ fn serialize_managed_settings(settings: &Value) -> anyhow::Result> { Ok(contents) } +/// Flattens managed settings into the `REG_SZ` data Claude Desktop reads on +/// Windows: a string value as it is, every other value as its JSON text — +/// the encoding the configuration reference specifies for MDM payloads. +#[cfg(any(windows, test))] +fn registry_values(settings: &Value) -> anyhow::Result> { + let Value::Object(settings) = settings else { + anyhow::bail!("Claude Desktop managed settings serialize to an object"); + }; + // Value names are case-insensitive to the registry, so every case + // variant of the marker's name is reserved. + if settings + .keys() + .any(|name| name.eq_ignore_ascii_case(REGISTRY_OWNER_VALUE)) + { + anyhow::bail!( + "Claude Desktop setting name {REGISTRY_OWNER_VALUE} is reserved for Agentdesktop's ownership marker" + ); + } + Ok(settings + .iter() + .map(|(name, value)| { + let data = match value { + Value::String(text) => text.clone(), + other => other.to_string(), + }; + (name.clone(), data) + }) + .collect()) +} + +/// Looks `name` up among the observed values the way the registry itself +/// matches value names: case-insensitively. ASCII covers every name +/// Agentdesktop writes; the registry's own folding is locale-invariant +/// Unicode, so a non-ASCII case variant is treated as a different name and +/// errs on the side of writing it. +#[cfg(any(windows, test))] +fn observed_value<'a>(observed: &'a RegistryValues, name: &str) -> Option<&'a Option> { + observed + .iter() + .find_map(|(existing, data)| existing.eq_ignore_ascii_case(name).then_some(data)) +} + +/// Whether a value with `name` exists under the key, case-insensitively. +#[cfg(any(windows, test))] +fn observes_name(observed: &RegistryValues, name: &str) -> bool { + observed + .keys() + .any(|existing| existing.eq_ignore_ascii_case(name)) +} + +/// The value names the ownership marker claims; empty when the marker is +/// missing or unreadable (nothing is owned, like a missing `.owner` sidecar). +#[cfg(any(windows, test))] +fn registry_owned_values(observed: &RegistryValues) -> BTreeSet { + observed_value(observed, REGISTRY_OWNER_VALUE) + .and_then(|data| data.as_deref()) + .and_then(|data| serde_json::from_str(data).ok()) + .unwrap_or_default() +} + +/// The named values that exist under the key, as the JSON object dry-run +/// diffs display; a value of a non-string registry type reads as `null`. +#[cfg(any(windows, test))] +fn registry_report<'a>( + names: impl IntoIterator, + observed: &RegistryValues, +) -> Vec { + let values: serde_json::Map = names + .into_iter() + .filter_map(|name| { + let data = observed_value(observed, name)?; + Some(( + name.clone(), + data.clone().map_or(Value::Null, Value::String), + )) + }) + .collect(); + Value::Object(values).to_string().into_bytes() +} + +/// `desired` as the JSON object dry-run diffs display. +#[cfg(any(windows, test))] +fn registry_report_desired(desired: &BTreeMap) -> Vec { + let values: serde_json::Map = desired + .iter() + .map(|(name, data)| (name.clone(), Value::String(data.clone()))) + .collect(); + Value::Object(values).to_string().into_bytes() +} + +/// Plans the registry writes that bring the Windows policy key to `desired`, +/// with the ownership semantics of `write_owned`: a value whose data already +/// matches is claimed as ours, a differing value the marker does not claim +/// is a conflict, and names the marker claims but `desired` dropped are +/// deleted. The marker never claims anything the plan does not write. Value +/// names match case-insensitively, as the registry stores them. +/// +/// An empty `desired` plans like removal: the ownership marker alone would +/// switch Claude Desktop to machine policy and away from the user's `HKCU` +/// policy, so with nothing to manage no key — not even a marker-only one — +/// is left behind. +#[cfg(any(windows, test))] +fn plan_registry_settings( + key: &RegistryKey, + observed: RegistryValues, + desired: &BTreeMap, + plan: &ReconcilePlan, +) { + if desired.is_empty() { + return plan_registry_remove(key, observed, plan); + } + let path = key.display_path(); + let owned = registry_owned_values(&observed); + let before = registry_report(desired.keys().chain(owned.iter()), &observed); + let after = registry_report_desired(desired); + let mut changes = Vec::new(); + for (name, data) in desired { + match observed_value(&observed, name) { + // Identical data is claimed as ours, like identical file bytes. + Some(Some(existing)) if existing == data => {} + Some(_) + if !owned + .iter() + .any(|claimed| claimed.eq_ignore_ascii_case(name)) => + { + plan.record_diff( + ClaudeDesktop::DISPLAY_NAME, + "managed settings", + "conflict", + &path, + Some(&before), + Some(&after), + ); + return; + } + _ => changes.push((name.clone(), Some(data.clone()))), + } + } + for name in &owned { + if !desired.keys().any(|kept| kept.eq_ignore_ascii_case(name)) + && observes_name(&observed, name) + { + changes.push((name.clone(), None)); + } + } + let marker = + serde_json::to_string(&desired.keys().collect::>()).expect("value names serialize"); + if observed_value(&observed, REGISTRY_OWNER_VALUE).and_then(|data| data.as_deref()) + != Some(marker.as_str()) + { + changes.push((REGISTRY_OWNER_VALUE.to_owned(), Some(marker))); + } + let action = if changes.iter().any(|(name, _)| name != REGISTRY_OWNER_VALUE) { + if observed.is_empty() { + "create" + } else { + "update" + } + } else { + "unchanged" + }; + if !changes.is_empty() { + plan.plan_registry_key(RegistryKeyPlan { + key: key.clone(), + observed, + changes, + delete_key_when_empty: false, + }); + } + debug!(program = ClaudeDesktop::ID, action, path = %path.display(), "planned managed settings"); + plan.record_diff( + ClaudeDesktop::DISPLAY_NAME, + "managed settings", + action, + &path, + Some(&before), + Some(&after), + ); +} + +/// Plans the removal of the registry values the ownership marker claims and +/// of the marker itself; the key is deleted at apply time only when nothing +/// else remains under it. Without a marker nothing is owned and nothing is +/// touched. +#[cfg(any(windows, test))] +fn plan_registry_remove(key: &RegistryKey, observed: RegistryValues, plan: &ReconcilePlan) { + let path = key.display_path(); + if !observes_name(&observed, REGISTRY_OWNER_VALUE) { + plan.record_diff( + ClaudeDesktop::DISPLAY_NAME, + "managed settings", + "unchanged", + &path, + None, + None, + ); + return; + } + let owned = registry_owned_values(&observed); + let before = registry_report(owned.iter(), &observed); + let mut changes: Vec<(String, Option)> = owned + .iter() + .filter(|name| observes_name(&observed, name)) + .map(|name| (name.clone(), None)) + .collect(); + changes.push((REGISTRY_OWNER_VALUE.to_owned(), None)); + debug!(program = ClaudeDesktop::ID, action = "remove", path = %path.display(), "planned managed settings"); + plan.record_diff( + ClaudeDesktop::DISPLAY_NAME, + "managed settings", + "remove", + &path, + Some(&before), + None, + ); + plan.plan_registry_key(RegistryKeyPlan { + key: key.clone(), + observed, + changes, + delete_key_when_empty: true, + }); +} + fn credential_helper_contents(credential_binary: &Path, socket: &Path) -> anyhow::Result> { #[cfg(windows)] return windows_credential_helper_contents( @@ -283,7 +588,7 @@ fn remove_owner_marker(path: &Path, plan: &ReconcilePlan) -> anyhow::Result<()> #[cfg(test)] mod tests { - use std::path::Path; + use std::{collections::BTreeMap, path::Path}; #[cfg(target_os = "macos")] use super::serialize_managed_settings; @@ -291,6 +596,8 @@ mod tests { batch_quote, managed_settings, posix_credential_helper_contents, windows_credential_helper_contents, }; + use crate::reconcile::ReconcilePlan; + use crate::reconcile::registry::{RegistryKey, RegistryValues}; use agentdesktop_core::config::parse_daemon; #[test] @@ -408,13 +715,15 @@ programs: let (dir, config) = mode_repair_fixture(); let path = dir.path().join("managed-settings.json"); let marker = super::owner_path(&path); - let helper = dir.path().join("helper.sh"); + let desktop = super::ClaudeDesktop { + managed_settings_path: path.clone(), + credential_helper_path: dir.path().join("helper.sh"), + }; // System mode: `merge_existing = false`. let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { super::plan( - &path, + &desktop, false, - &helper, Path::new("agentdesktop"), Path::new("agentdesktop.sock"), Some((config.programs.claude_desktop.as_ref().unwrap(), None)), @@ -434,13 +743,15 @@ programs: let (dir, config) = mode_repair_fixture(); let path = dir.path().join("managed-settings.json"); let marker = super::owner_path(&path); - let helper = dir.path().join("helper.sh"); + let desktop = super::ClaudeDesktop { + managed_settings_path: path.clone(), + credential_helper_path: dir.path().join("helper.sh"), + }; // System mode: `merge_existing = false`. let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { super::plan( - &path, + &desktop, false, - &helper, Path::new("agentdesktop"), Path::new("agentdesktop.sock"), Some((config.programs.claude_desktop.as_ref().unwrap(), None)), @@ -452,4 +763,361 @@ programs: &plan_fn, ); } + + // --- Windows policy registry target -------------------------------------- + + #[test] + fn registry_values_encode_strings_literally_and_the_rest_as_json() { + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com + authentication: + type: controllerJwt + audience: agentgateway + allowedClientIds: [claude-desktop] +programs: + claudeDesktop: + isLocalDevMcpEnabled: true + coworkEgressAllowedHosts: ["example.com"] +"#, + ) + .unwrap(); + let desktop = config.programs.claude_desktop.as_ref().unwrap(); + let settings = + managed_settings(desktop, config.llm_gateway.as_ref(), Path::new("/helper")).unwrap(); + + let values = super::registry_values(&settings).unwrap(); + assert_eq!(values["inferenceProvider"], "gateway"); + assert_eq!( + values["inferenceGatewayBaseUrl"], + "https://gateway.example.com/" + ); + assert_eq!(values["isLocalDevMcpEnabled"], "true"); + assert_eq!(values["inferenceCredentialHelperTtlSec"], "60"); + assert_eq!(values["coworkEgressAllowedHosts"], r#"["example.com"]"#); + } + + #[test] + fn registry_values_reject_the_reserved_marker_name() { + // Value names are case-insensitive to the registry, so a case + // variant would overwrite the marker all the same. + for name in [".agentdesktop.owner", ".AgentDesktop.Owner"] { + let error = super::registry_values(&serde_json::json!({ name: "x" })).unwrap_err(); + assert!(error.to_string().contains("reserved"), "{name}: {error}"); + } + } + + #[test] + fn registry_plan_creates_values_and_the_ownership_marker() { + let desired = BTreeMap::from([ + ("inferenceProvider".to_owned(), "gateway".to_owned()), + ("isLocalDevMcpEnabled".to_owned(), "true".to_owned()), + ]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, RegistryValues::new(), &desired, &plan); + + let plans = plan.registry_key_plans(); + assert_eq!(plans.len(), 1); + assert!(!plans[0].delete_key_when_empty); + assert_eq!( + plans[0].changes, + vec![ + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ("isLocalDevMcpEnabled".to_owned(), Some("true".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider","isLocalDevMcpEnabled"]"#.to_owned()) + ), + ] + ); + let rendered = plan.render(); + assert!(rendered.contains("CREATE Claude Desktop managed settings")); + assert!(rendered.contains(r"HKLM\SOFTWARE\Policies\Claude")); + } + + #[test] + fn registry_plan_updates_owned_and_deletes_stale_values() { + let observed = RegistryValues::from([ + ("inferenceProvider".to_owned(), Some("direct".to_owned())), + ("stale".to_owned(), Some("old".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider","stale"]"#.to_owned()), + ), + ]); + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed, &desired, &plan); + + assert!(!plan.has_conflicts()); + assert_eq!( + plan.registry_key_plans()[0].changes, + vec![ + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ("stale".to_owned(), None), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()) + ), + ] + ); + assert!( + plan.render() + .contains("UPDATE Claude Desktop managed settings") + ); + } + + #[test] + fn registry_plan_adopts_identical_values_with_a_marker_write() { + let observed = + RegistryValues::from([("inferenceProvider".to_owned(), Some("gateway".to_owned()))]); + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let key = RegistryKey::current_user(r"Software\AgentdesktopTest"); + assert!( + key.display_path() + .to_string_lossy() + .starts_with(r"HKCU\Software") + ); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed, &desired, &plan); + + assert_eq!( + plan.registry_key_plans()[0].changes, + vec![( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()) + )] + ); + assert!(plan.render().contains("Summary: 0 changes, 1 unchanged")); + } + + #[test] + fn registry_plan_conflicts_on_a_foreign_value() { + // A differing value the marker does not claim, and a value of a + // non-string registry type, both refuse the plan. The name matches + // case-insensitively, as the registry stores it: a case-variant + // foreign value is the same value, not a writable second one. + for name in ["inferenceProvider", "InferenceProvider"] { + for foreign in [Some("direct".to_owned()), None] { + let observed = RegistryValues::from([(name.to_owned(), foreign)]); + let desired = + BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed, &desired, &plan); + + assert!(plan.has_conflicts(), "{name}"); + assert!(plan.registry_key_plans().is_empty(), "{name}"); + } + } + } + + #[test] + fn registry_plan_matches_owned_names_case_insensitively() { + // The registry kept another writer's casing for a name the marker + // claims: still ours, so an update and its removal, not a conflict. + let observed = || { + RegistryValues::from([ + ("InferenceProvider".to_owned(), Some("direct".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()), + ), + ]) + }; + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed(), &desired, &plan); + assert!(!plan.has_conflicts()); + assert_eq!( + plan.registry_key_plans()[0].changes, + vec![("inferenceProvider".to_owned(), Some("gateway".to_owned()))] + ); + + let plan = ReconcilePlan::default(); + super::plan_registry_remove(&key, observed(), &plan); + assert_eq!( + plan.registry_key_plans()[0].changes, + vec![ + ("inferenceProvider".to_owned(), None), + (".agentdesktop.owner".to_owned(), None), + ] + ); + } + + #[test] + fn registry_plan_with_nothing_to_manage_leaves_no_key() { + // `claudeDesktop: {}` without a gateway: even a marker-only key + // would count as machine policy to Claude Desktop and switch it + // away from `HKCU` policy, so nothing is planned on a clean system. + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, RegistryValues::new(), &BTreeMap::new(), &plan); + assert!(plan.registry_key_plans().is_empty()); + assert!(plan.render().contains("Summary: 0 changes, 1 unchanged")); + + // A previously managed key loses the owned values, the marker and — + // once empty — itself, and the dry run reports the removal. + let observed = RegistryValues::from([ + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()), + ), + ]); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed, &BTreeMap::new(), &plan); + let plans = plan.registry_key_plans(); + assert!(plans[0].delete_key_when_empty); + assert_eq!( + plans[0].changes, + vec![ + ("inferenceProvider".to_owned(), None), + (".agentdesktop.owner".to_owned(), None), + ] + ); + assert!( + plan.render() + .contains("REMOVE Claude Desktop managed settings") + ); + } + + #[test] + fn registry_remove_deletes_only_what_the_marker_claims() { + let observed = RegistryValues::from([ + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ("foreign".to_owned(), Some("keep".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()), + ), + ]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_remove(&key, observed, &plan); + + let plans = plan.registry_key_plans(); + assert!(plans[0].delete_key_when_empty); + assert_eq!( + plans[0].changes, + vec![ + ("inferenceProvider".to_owned(), None), + (".agentdesktop.owner".to_owned(), None), + ] + ); + + // Without the marker nothing is owned and nothing is planned. + let plan = ReconcilePlan::default(); + super::plan_registry_remove( + &key, + RegistryValues::from([("foreign".to_owned(), Some("keep".to_owned()))]), + &plan, + ); + assert!(plan.registry_key_plans().is_empty()); + assert!(plan.render().contains("Summary: 0 changes, 1 unchanged")); + } + + #[cfg(windows)] + use crate::reconcile::registry::TestPolicyKey; + + #[test] + #[cfg(windows)] + fn registry_round_trip_applies_creates_updates_and_removal() { + use crate::reconcile::registry::{key_exists, read_values}; + let key = TestPolicyKey::new(); + let desired = BTreeMap::from([ + ("inferenceProvider".to_owned(), "gateway".to_owned()), + ("isLocalDevMcpEnabled".to_owned(), "true".to_owned()), + ]); + + // Create: the values and the marker land under the key. + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key.0, read_values(&key.0).unwrap(), &desired, &plan); + plan.apply().unwrap(); + let values = read_values(&key.0).unwrap(); + assert_eq!(values["inferenceProvider"].as_deref(), Some("gateway")); + assert_eq!(values["isLocalDevMcpEnabled"].as_deref(), Some("true")); + assert_eq!( + values[".agentdesktop.owner"].as_deref(), + Some(r#"["inferenceProvider","isLocalDevMcpEnabled"]"#) + ); + + // In place: the next plan has nothing to apply. + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key.0, read_values(&key.0).unwrap(), &desired, &plan); + assert!(plan.registry_key_plans().is_empty()); + + // Update: one value changes its data, the other is dropped. + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "direct".to_owned())]); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key.0, read_values(&key.0).unwrap(), &desired, &plan); + plan.apply().unwrap(); + let values = read_values(&key.0).unwrap(); + assert_eq!(values["inferenceProvider"].as_deref(), Some("direct")); + assert!(!values.contains_key("isLocalDevMcpEnabled")); + + // Remove: the owned values go, and with them the empty key. + let plan = ReconcilePlan::default(); + super::plan_registry_remove(&key.0, read_values(&key.0).unwrap(), &plan); + plan.apply().unwrap(); + assert!(!key_exists(&key.0).unwrap()); + } + + #[test] + #[cfg(windows)] + fn registry_removal_keeps_foreign_values_and_the_key() { + use crate::reconcile::registry::{RegistryKeyPlan, key_exists, read_values}; + let key = TestPolicyKey::new(); + crate::reconcile::registry::apply(&RegistryKeyPlan { + key: key.0.clone(), + observed: RegistryValues::new(), + changes: vec![("foreign".to_owned(), Some("keep".to_owned()))], + delete_key_when_empty: false, + }) + .unwrap(); + + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key.0, read_values(&key.0).unwrap(), &desired, &plan); + plan.apply().unwrap(); + + let plan = ReconcilePlan::default(); + super::plan_registry_remove(&key.0, read_values(&key.0).unwrap(), &plan); + plan.apply().unwrap(); + + let values = read_values(&key.0).unwrap(); + assert_eq!(values["foreign"].as_deref(), Some("keep")); + assert!(!values.contains_key("inferenceProvider")); + assert!(key_exists(&key.0).unwrap()); + } + + #[test] + #[cfg(windows)] + fn registry_apply_refuses_when_the_key_changed_after_planning() { + use crate::reconcile::registry::{RegistryKeyPlan, read_values}; + let key = TestPolicyKey::new(); + let desired = BTreeMap::from([("inferenceProvider".to_owned(), "gateway".to_owned())]); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key.0, read_values(&key.0).unwrap(), &desired, &plan); + + // Another writer touches the key between planning and applying. + crate::reconcile::registry::apply(&RegistryKeyPlan { + key: key.0.clone(), + observed: RegistryValues::new(), + changes: vec![("foreign".to_owned(), Some("raced".to_owned()))], + delete_key_when_empty: false, + }) + .unwrap(); + + let error = plan.apply().unwrap_err(); + assert!( + error + .to_string() + .contains("changed since reconciliation was planned") + ); + } } diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index f4449fbd..c54d7804 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -1,5 +1,7 @@ //! Shared reconciliation orchestration, plan application, and dry-run reporting. pub(crate) mod plan; +#[cfg(any(windows, test))] +pub(crate) mod registry; pub use plan::ReconcilePlan; pub(crate) use plan::grants_beyond; @@ -81,6 +83,8 @@ impl Reconciler { Box::new(ClaudeDesktop { managed_settings_path: claude_desktop_managed_settings_path, credential_helper_path: claude_desktop_credential_helper_path, + #[cfg(windows)] + policy_key: crate::provider::claude_desktop::claude_desktop_policy_key(), }), Box::new(Codex { managed_config_path: codex_managed_config_path, @@ -555,6 +559,9 @@ impl AttributedPlan { detail: format!("{error:#}"), written: match at { plan::ApplyStop::Operation(index) => index, + // Registry changes apply after every file operation. + #[cfg(windows)] + plan::ApplyStop::Registry(_) => usize::MAX, _ => 0, }, }; @@ -633,6 +640,8 @@ fn owners_of(entries: &[ProgramEntry], at: &plan::ApplyStop) -> Vec<&'static str .filter(|entry| match at { plan::ApplyStop::Conflict => false, plan::ApplyStop::Observed(path) => entry.paths.contains(path), + #[cfg(windows)] + plan::ApplyStop::Registry(path) => entry.paths.contains(path), plan::ApplyStop::PrivateDir(dir) => { entry.paths.iter().any(|path| path.starts_with(dir)) } @@ -868,6 +877,8 @@ fn normalized_diff(before: &str, after: &str) -> (String, String) { mod tests { use std::{fs, path::Path, path::PathBuf}; + #[cfg(windows)] + use agentdesktop_core::config::DaemonConfig; use agentdesktop_core::config::parse_daemon; use agentdesktop_core::model::ProgramChoice; @@ -1108,7 +1119,8 @@ programs: // The Copilot CLI program is the inverse of Grok: it manages a file in // the user's home, so a system daemon (no providers path) rejects it // before any provider writes. - let reconciler = Reconciler::new( + #[allow(unused_mut)] + let mut reconciler = Reconciler::new( false, root.join("claude/settings.json"), root.join("claude-desktop/settings.json"), @@ -1123,6 +1135,15 @@ programs: root.join("bin/agentdesktop"), root.join("agentdesktop.sock"), ); + #[cfg(windows)] + let policy_key = super::registry::TestPolicyKey::new(); + #[cfg(windows)] + use_test_policy_key( + &mut reconciler, + root.join("claude-desktop/settings.json"), + root.join("claude-desktop/helper"), + policy_key.0.clone(), + ); let error = reconciler .apply(&config) @@ -1169,7 +1190,8 @@ programs: fs::write(root.join("claude/settings.json"), user_claude).unwrap(); fs::write(root.join("codex/config.toml"), old_codex).unwrap(); fs::write(root.join("opencode/plugin.js"), old_plugin).unwrap(); - let reconciler = Reconciler::new( + #[allow(unused_mut)] + let mut reconciler = Reconciler::new( false, root.join("claude/settings.json"), root.join("claude-desktop/settings.json"), @@ -1184,6 +1206,15 @@ programs: root.join("bin/agentdesktop"), root.join("agentdesktop.sock"), ); + #[cfg(windows)] + let policy_key = super::registry::TestPolicyKey::new(); + #[cfg(windows)] + use_test_policy_key( + &mut reconciler, + root.join("claude-desktop/settings.json"), + root.join("claude-desktop/helper"), + policy_key.0.clone(), + ); reconciler.dry_run(&config).expect("dry run succeeds"); @@ -1201,9 +1232,35 @@ programs: fs::remove_dir_all(root).unwrap(); } + /// Repoints the reconciler's Claude Desktop provider at `key`, a + /// disposable `HKCU` key, so the test never reads or writes the live + /// machine policy key. + #[cfg(windows)] + fn use_test_policy_key( + reconciler: &mut Reconciler, + settings: PathBuf, + helper: PathBuf, + key: super::registry::RegistryKey, + ) { + let providers = std::sync::Arc::get_mut(&mut reconciler.providers) + .expect("the providers are not shared yet"); + let desktop = providers + .iter_mut() + .find(|provider| provider.id() == super::ClaudeDesktop::ID) + .expect("the Claude Desktop provider is registered"); + *desktop = Box::new(super::ClaudeDesktop { + managed_settings_path: settings, + credential_helper_path: helper, + policy_key: key, + }); + } + struct Fixture { root: PathBuf, reconciler: Reconciler, + /// Stands in for the live policy key, deleted with the fixture. + #[cfg(windows)] + policy_key: super::registry::TestPolicyKey, } impl Fixture { @@ -1213,7 +1270,8 @@ programs: std::process::id(), rand::random::() )); - let reconciler = Reconciler::new( + #[allow(unused_mut)] + let mut reconciler = Reconciler::new( false, root.join("claude/settings.json"), root.join("desktop/settings.json"), @@ -1228,7 +1286,21 @@ programs: root.join("bin/agentdesktop"), root.join("agentdesktop.sock"), ); - Self { root, reconciler } + #[cfg(windows)] + let policy_key = super::registry::TestPolicyKey::new(); + #[cfg(windows)] + use_test_policy_key( + &mut reconciler, + root.join("desktop/settings.json"), + root.join("desktop/helper"), + policy_key.0.clone(), + ); + Self { + root, + reconciler, + #[cfg(windows)] + policy_key, + } } } @@ -1270,7 +1342,10 @@ programs: assert!(!plan.has_conflicts()); plan.apply().unwrap(); - let paths = [ + // On Windows the Claude Desktop managed settings land under the + // (test) policy registry key, asserted below, not in a file; the + // credential helper is a file there too. + let paths: Vec<&str> = [ "claude/settings.json", "claude/.settings.json.owner", "desktop/settings.json", @@ -1281,7 +1356,26 @@ programs: "opencode/config.json", "opencode/plugin.js", "grok/managed_config.toml", - ]; + ] + .into_iter() + .filter(|path| { + !cfg!(windows) + || !matches!( + *path, + "desktop/settings.json" | "desktop/.settings.json.owner" + ) + }) + .collect(); + #[cfg(windows)] + { + let values = super::registry::read_values(&fixture.policy_key.0).unwrap(); + assert_eq!(values["inferenceProvider"].as_deref(), Some("gateway")); + assert_eq!( + values["inferenceGatewayBaseUrl"].as_deref(), + Some("https://gateway.example.com/") + ); + assert!(values.contains_key(".agentdesktop.owner")); + } let contents: Vec<_> = paths .iter() .map(|path| fs::read(fixture.root.join(path)).unwrap()) @@ -1310,6 +1404,54 @@ programs: assert!(paths.iter().all(|path| fixture.root.join(path).exists())); cleanup.apply().unwrap(); assert!(paths.iter().all(|path| !fixture.root.join(path).exists())); + #[cfg(windows)] + assert!( + !super::registry::key_exists(&fixture.policy_key.0).unwrap(), + "removal deletes the owned values, the marker and the empty key" + ); + } + + /// A registry write failure after the file operations: the failure is + /// attributed to Claude Desktop through `ApplyStop::Registry`, and the + /// file operations — all applied before the registry changes — stay. + #[cfg(windows)] + #[test] + fn registry_write_failure_is_attributed_to_claude_desktop() { + let fixture = Fixture::new(); + // A value name past the registry's 16,383-character limit makes + // `RegSetValueExW` refuse after every file operation was written. + // Measured on Windows: 16,383 chars ok / 16,384+ returns rc 87 ERROR_INVALID_PARAMETER. + let long_name = "a".repeat(16_384); + let mut claude_desktop_settings = serde_json::Map::new(); + claude_desktop_settings.insert(long_name, serde_json::json!("boom")); + let config_json = serde_json::json!({ + "programs": { + "claudeCode": {}, + "claudeDesktop": claude_desktop_settings, + } + }); + let config: DaemonConfig = serde_json::from_value(config_json).unwrap(); + + let (report, result) = fixture.reconciler.apply_with_report(&config); + + let error = result.expect_err("the registry write must fail"); + assert!(error.to_string().contains("HKCU"), "{error}"); + let outcome = |program: &str| { + report + .programs + .iter() + .find(|outcome| outcome.program == program) + .expect("program reported") + }; + assert_eq!(outcome("claude-desktop").state, ProgramState::Failed); + assert!( + outcome("claude-desktop").detail.contains("HKCU"), + "{}", + outcome("claude-desktop").detail + ); + // `written: usize::MAX`: Claude Code's files landed and stay. + assert_eq!(outcome("claude-code").state, ProgramState::Applied); + assert!(fixture.root.join("claude/settings.json").is_file()); } #[test] diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index 333a5b4f..309443f2 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -1,4 +1,6 @@ use super::DryRunReport; +#[cfg(any(windows, test))] +use super::registry::RegistryKeyPlan; use anyhow::Context; use std::{ cell::RefCell, @@ -14,6 +16,11 @@ use std::{ pub struct ReconcilePlan { observed: RefCell>>>, operations: RefCell>, + /// Planned Windows registry changes, applied after the file operations. + /// Only the Windows planners create one; the pure planning tests on + /// other platforms inspect them without applying. + #[cfg(any(windows, test))] + registry: RefCell>, private_dirs: PrivateDirs, report: DryRunReport, /// Why the program that owns this plan is configured but not pointed at @@ -44,6 +51,10 @@ pub(super) enum ApplyStop { PrivateDir(PathBuf), /// The operation at this index failed; the ones before it were written. Operation(usize), + /// The registry changes for this key failed; every file operation was + /// written before them. + #[cfg(windows)] + Registry(PathBuf), } struct FileChange { @@ -119,8 +130,17 @@ impl ReconcilePlan { paths.push(change.path.clone()); } } + let changes = differs || recorded; + #[cfg(any(windows, test))] + let changes = { + let registry = self.registry.borrow(); + for key_plan in registry.iter() { + paths.push(key_plan.key.display_path()); + } + changes || !registry.is_empty() + }; PlanSummary { - changes: differs || recorded, + changes, conflicts, inactive: self.inactive.borrow().clone(), paths, @@ -161,6 +181,23 @@ impl ReconcilePlan { )); } } + #[cfg(windows)] + for key_plan in self.registry.borrow().iter() { + let display = key_plan.key.display_path(); + let stop = || ApplyStop::Observed(display.clone()); + let current = super::registry::read_values(&key_plan.key) + .with_context(|| format!("validate {} before applying plan", display.display())) + .map_err(|error| (error, stop()))?; + if current != key_plan.observed { + return Err(( + anyhow::anyhow!( + "{} changed since reconciliation was planned; retry reconciliation", + display.display() + ), + stop(), + )); + } + } for dir in self.private_dirs.0.into_inner() { if !dir.exists() { crate::secure_fs::ensure_private_dir(&dir) @@ -193,6 +230,13 @@ impl ReconcilePlan { result.map_err(|error| (error, ApplyStop::Operation(index)))?; tracing::info!(path = %change.path.display(), "applied reconciliation change"); } + #[cfg(windows)] + for key_plan in self.registry.into_inner() { + let key_path = key_plan.key.display_path(); + super::registry::apply(&key_plan) + .map_err(|error| (error, ApplyStop::Registry(key_path.clone())))?; + tracing::info!(path = %key_path.display(), "applied reconciliation change"); + } Ok(()) } @@ -238,6 +282,20 @@ impl ReconcilePlan { } operations.push(change); } + #[cfg(any(windows, test))] + { + let registry = self.registry.get_mut(); + for key_plan in other.registry.into_inner() { + let path = key_plan.key.display_path(); + if registry.iter().any(|existing| existing.key == key_plan.key) { + return Err(( + anyhow::anyhow!("multiple providers plan to modify {}", path.display()), + path, + )); + } + registry.push(key_plan); + } + } self.report .changes .get_mut() @@ -283,6 +341,19 @@ impl ReconcilePlan { self.private_dirs.0.borrow_mut().push(dir.to_owned()); } + /// Plan one Windows registry key's changes, applied after the file + /// operations. + #[cfg(any(windows, test))] + pub(crate) fn plan_registry_key(&self, key_plan: RegistryKeyPlan) { + self.registry.borrow_mut().push(key_plan); + } + + /// The planned registry changes, for tests. + #[cfg(test)] + pub(crate) fn registry_key_plans(&self) -> Vec { + self.registry.borrow().clone() + } + pub(crate) fn remove_file(&self, path: &Path) -> io::Result<()> { self.read(path)?; self.operations.borrow_mut().push(FileChange { diff --git a/crates/agent/src/reconcile/registry.rs b/crates/agent/src/reconcile/registry.rs new file mode 100644 index 00000000..edb8c549 --- /dev/null +++ b/crates/agent/src/reconcile/registry.rs @@ -0,0 +1,358 @@ +//! Windows registry reconcile target. +//! +//! Claude Desktop on Windows reads its managed configuration from flat +//! `REG_SZ` values directly under `HKLM\SOFTWARE\Policies\Claude`, never from +//! a file, so the reconciler plans registry value changes there. The planning +//! types are plain data and compile everywhere (the pure planning tests run on +//! every platform); only the functions that touch the live registry are +//! Windows-only. + +use std::collections::BTreeMap; +use std::path::PathBuf; + +/// A registry key under a fixed root. The root is a type of its own so tests +/// can run the writer against a hive that needs no elevation. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct RegistryKey { + root: RegistryRoot, + path: String, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum RegistryRoot { + /// `HKEY_LOCAL_MACHINE`: machine policy, written only by the system daemon. + LocalMachine, + /// `HKEY_CURRENT_USER`: writable without elevation, for the Windows tests. + #[cfg(test)] + CurrentUser, +} + +impl RegistryKey { + pub(crate) fn local_machine(path: impl Into) -> Self { + Self { + root: RegistryRoot::LocalMachine, + path: path.into(), + } + } + + #[cfg(test)] + pub(crate) fn current_user(path: impl Into) -> Self { + Self { + root: RegistryRoot::CurrentUser, + path: path.into(), + } + } + + /// The key as plans and logs display it, e.g. `HKLM\SOFTWARE\Policies\Claude`. + pub(crate) fn display_path(&self) -> PathBuf { + let root = match self.root { + RegistryRoot::LocalMachine => "HKLM", + #[cfg(test)] + RegistryRoot::CurrentUser => "HKCU", + }; + PathBuf::from(format!(r"{root}\{}", self.path)) + } +} + +/// The values observed under a key: the `REG_SZ` data by value name, or +/// `None` for a value of any other type, which Agentdesktop never writes and +/// never matches. +pub(crate) type RegistryValues = BTreeMap>; + +/// One key's planned changes: each entry writes a `REG_SZ` value (`Some`) or +/// deletes it (`None`). `observed` is the whole key's state when the plan was +/// made, so the apply can refuse when the key changed in between, like a +/// planned file. +#[derive(Clone, Debug, PartialEq)] +pub(crate) struct RegistryKeyPlan { + pub(crate) key: RegistryKey, + pub(crate) observed: RegistryValues, + pub(crate) changes: Vec<(String, Option)>, + /// Delete the key itself when nothing remains in it after the changes. + pub(crate) delete_key_when_empty: bool, +} + +#[cfg(windows)] +pub(crate) use self::windows::{apply, read_values}; +#[cfg(all(windows, test))] +pub(crate) use self::windows::{delete_tree, key_exists}; + +/// A unique key under `HKCU`, writable without elevation and deleted on +/// drop: what every test uses in place of the live machine policy key, +/// which no test may read or write. +#[cfg(all(windows, test))] +pub(crate) struct TestPolicyKey(pub(crate) RegistryKey); + +#[cfg(all(windows, test))] +impl TestPolicyKey { + pub(crate) fn new() -> Self { + Self(RegistryKey::current_user(format!( + r"Software\agentdesktop-test-{}-{}", + std::process::id(), + rand::random::() + ))) + } +} + +#[cfg(all(windows, test))] +impl Drop for TestPolicyKey { + fn drop(&mut self) { + delete_tree(&self.0); + } +} + +#[cfg(windows)] +mod windows { + use super::{RegistryKey, RegistryKeyPlan, RegistryRoot, RegistryValues}; + use anyhow::Context; + use std::{ffi::OsStr, io, os::windows::ffi::OsStrExt}; + use windows_sys::Win32::Foundation::{ + ERROR_FILE_NOT_FOUND, ERROR_MORE_DATA, ERROR_NO_MORE_ITEMS, ERROR_SUCCESS, + }; + use windows_sys::Win32::System::Registry::{ + HKEY, HKEY_LOCAL_MACHINE, KEY_READ, KEY_SET_VALUE, REG_OPTION_NON_VOLATILE, REG_SZ, + RegCloseKey, RegCreateKeyExW, RegDeleteKeyW, RegDeleteValueW, RegEnumValueW, RegOpenKeyExW, + RegQueryInfoKeyW, RegSetValueExW, + }; + #[cfg(test)] + use windows_sys::Win32::System::Registry::{HKEY_CURRENT_USER, RegDeleteTreeW}; + + impl RegistryRoot { + fn handle(self) -> HKEY { + match self { + RegistryRoot::LocalMachine => HKEY_LOCAL_MACHINE, + #[cfg(test)] + RegistryRoot::CurrentUser => HKEY_CURRENT_USER, + } + } + } + + /// An open key handle, closed on drop. + struct Key(HKEY); + + impl Drop for Key { + fn drop(&mut self) { + // SAFETY: the handle was opened by RegOpenKeyExW or RegCreateKeyExW. + unsafe { + RegCloseKey(self.0); + } + } + } + + fn wide(text: &str) -> Vec { + OsStr::new(text) + .encode_wide() + .chain(std::iter::once(0)) + .collect() + } + + fn registry_error(status: u32) -> io::Error { + io::Error::from_raw_os_error(status as i32) + } + + /// Every value under `key`, or an empty map when the key does not exist. + pub(crate) fn read_values(key: &RegistryKey) -> io::Result { + let path = wide(&key.path); + let mut handle = std::ptr::null_mut(); + // SAFETY: path is null-terminated and handle receives the opened key. + let status = + unsafe { RegOpenKeyExW(key.root.handle(), path.as_ptr(), 0, KEY_READ, &mut handle) }; + if status == ERROR_FILE_NOT_FOUND { + return Ok(RegistryValues::new()); + } + if status != ERROR_SUCCESS { + return Err(registry_error(status)); + } + let key = Key(handle); + let mut values = RegistryValues::new(); + let mut data = vec![0u8; 256]; + for index in 0.. { + let Some((name, value)) = read_value(&key, index, &mut data)? else { + break; + }; + values.insert(name, value); + } + Ok(values) + } + + /// The value at `index` under `key`, or `None` past the last one. + /// `data` grows as needed and is reused across values. + fn read_value( + key: &Key, + index: u32, + data: &mut Vec, + ) -> io::Result)>> { + // One more than the registry's maximum value-name length, in UTF-16 + // units, so every name fits with its terminator. + let mut name = [0u16; 16384]; + loop { + let mut name_len = name.len() as u32; + let mut value_type = 0u32; + let mut data_len = data.len() as u32; + // SAFETY: all buffers live across the call and their sizes are passed. + let status = unsafe { + RegEnumValueW( + key.0, + index, + name.as_mut_ptr(), + &mut name_len, + std::ptr::null(), + &mut value_type, + data.as_mut_ptr(), + &mut data_len, + ) + }; + match status { + ERROR_SUCCESS => { + let name = String::from_utf16_lossy(&name[..name_len as usize]); + let value = + (value_type == REG_SZ).then(|| decode_sz(&data[..data_len as usize])); + return Ok(Some((name, value))); + } + // The data buffer was too small; data_len has the needed size. + ERROR_MORE_DATA => data.resize(data_len as usize, 0), + ERROR_NO_MORE_ITEMS => return Ok(None), + status => return Err(registry_error(status)), + } + } + } + + /// Decodes `REG_SZ` bytes: UTF-16, usually with a trailing terminator. + fn decode_sz(data: &[u8]) -> String { + let mut units: Vec = data + .as_chunks::<2>() + .0 + .iter() + .map(|pair| u16::from_le_bytes(*pair)) + .collect(); + while units.last() == Some(&0) { + units.pop(); + } + String::from_utf16_lossy(&units) + } + + /// Applies one key's planned changes. The caller has already verified + /// that the key still matches `plan.observed`. + pub(crate) fn apply(plan: &RegistryKeyPlan) -> anyhow::Result<()> { + let display = plan.key.display_path(); + let path = wide(&plan.key.path); + let mut handle = std::ptr::null_mut(); + let mut disposition = 0u32; + // SAFETY: path is null-terminated and handle receives the created key. + let status = unsafe { + RegCreateKeyExW( + plan.key.root.handle(), + path.as_ptr(), + 0, + std::ptr::null(), + REG_OPTION_NON_VOLATILE, + KEY_READ | KEY_SET_VALUE, + std::ptr::null(), + &mut handle, + &mut disposition, + ) + }; + if status != ERROR_SUCCESS { + return Err(registry_error(status)) + .with_context(|| format!("open {}", display.display())); + } + let key = Key(handle); + for (name, value) in &plan.changes { + let value_name = wide(name); + match value { + Some(data) => { + let data = wide(data); + // SAFETY: the value name and data are null-terminated and + // the data length is its byte count. + let status = unsafe { + RegSetValueExW( + key.0, + value_name.as_ptr(), + 0, + REG_SZ, + data.as_ptr().cast(), + (data.len() * 2) as u32, + ) + }; + if status != ERROR_SUCCESS { + return Err(registry_error(status)) + .with_context(|| format!("write {}\\{name}", display.display())); + } + } + None => { + // SAFETY: the value name is null-terminated. + let status = unsafe { RegDeleteValueW(key.0, value_name.as_ptr()) }; + if status != ERROR_SUCCESS && status != ERROR_FILE_NOT_FOUND { + return Err(registry_error(status)) + .with_context(|| format!("remove {}\\{name}", display.display())); + } + } + } + } + if plan.delete_key_when_empty && key_is_empty(&key)? { + drop(key); + // SAFETY: path is null-terminated; the handle is already closed. + let status = unsafe { RegDeleteKeyW(plan.key.root.handle(), path.as_ptr()) }; + if status != ERROR_SUCCESS && status != ERROR_FILE_NOT_FOUND { + return Err(registry_error(status)) + .with_context(|| format!("remove {}", display.display())); + } + } + Ok(()) + } + + /// Whether the key has no values and no subkeys left. + fn key_is_empty(key: &Key) -> io::Result { + let mut subkeys = 0u32; + let mut values = 0u32; + // SAFETY: only the subkey and value counters are requested. + let status = unsafe { + RegQueryInfoKeyW( + key.0, + std::ptr::null_mut(), + std::ptr::null_mut(), + std::ptr::null(), + &mut subkeys, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut values, + std::ptr::null_mut(), + std::ptr::null_mut(), + std::ptr::null_mut(), + std::ptr::null_mut(), + ) + }; + if status != ERROR_SUCCESS { + return Err(registry_error(status)); + } + Ok(subkeys == 0 && values == 0) + } + + /// Whether the key exists, for tests. + #[cfg(test)] + pub(crate) fn key_exists(key: &RegistryKey) -> io::Result { + let path = wide(&key.path); + let mut handle = std::ptr::null_mut(); + // SAFETY: path is null-terminated and handle receives the opened key. + let status = + unsafe { RegOpenKeyExW(key.root.handle(), path.as_ptr(), 0, KEY_READ, &mut handle) }; + match status { + ERROR_SUCCESS => { + drop(Key(handle)); + Ok(true) + } + ERROR_FILE_NOT_FOUND => Ok(false), + status => Err(registry_error(status)), + } + } + + /// Deletes the key and everything under it, for test cleanup. + #[cfg(test)] + pub(crate) fn delete_tree(key: &RegistryKey) { + let path = wide(&key.path); + // SAFETY: path is null-terminated. + unsafe { + RegDeleteTreeW(key.root.handle(), path.as_ptr()); + } + } +} diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 2ac17f4b..d80feb5f 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -145,7 +145,9 @@ pub struct UsageStartupConfig { #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct ClaudeDesktopStartupConfig { - /// Configuration file. + /// Configuration file. On Windows the managed settings live in the + /// policy registry key instead, and this file is only removed when an + /// earlier Agentdesktop wrote it. #[serde(default, skip_serializing_if = "Option::is_none")] pub config: Option, /// Credential helper path. diff --git a/schema/daemon-config.json b/schema/daemon-config.json index 91d8f061..0c00d4f0 100644 --- a/schema/daemon-config.json +++ b/schema/daemon-config.json @@ -117,7 +117,7 @@ "type": "object", "properties": { "config": { - "description": "Configuration file.", + "description": "Configuration file. On Windows the managed settings live in the\npolicy registry key instead, and this file is only removed when an\nearlier Agentdesktop wrote it.", "type": [ "string", "null" diff --git a/schema/daemon-config.md b/schema/daemon-config.md index 957ce2ed..701b3b1e 100644 --- a/schema/daemon-config.md +++ b/schema/daemon-config.md @@ -11,7 +11,7 @@ |`daemon.claudeCode`|object|Claude Code paths.| |`daemon.claudeCode.config`|string|Configuration file.| |`daemon.claudeDesktop`|object|Claude Desktop paths.| -|`daemon.claudeDesktop.config`|string|Configuration file.| +|`daemon.claudeDesktop.config`|string|Configuration file. On Windows the managed settings live in the
policy registry key instead, and this file is only removed when an
earlier Agentdesktop wrote it.| |`daemon.claudeDesktop.credentialHelper`|string|Credential helper path.| |`daemon.codex`|object|Codex paths.| |`daemon.codex.config`|string|Configuration file.| From 4120948eac7f3d866df7aba03d004f655135f138 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Fri, 23 Oct 2026 09:34:47 +0200 Subject: [PATCH 2/2] fix: validate registry value names and keep partial applies recoverable Review round on #193 (peterj + Copilot): - registry_values refuses names with an embedded NUL (RegSetValueExW would truncate to a different name than conflict checking and the ownership marker used), names past the registry's 16,383-character limit (the registry refuses them only after earlier values were written), names differing only in ASCII case (one registry value, two desired entries, no convergence), and data with an embedded NUL. - The ownership marker now brackets the writes: the first change claims every name the plan touches, so an apply that fails midway leaves everything already written claimed and recoverable; the claim shrinks to exactly the desired names last. - Per-program completion accounting now tracks registry key plans next to file operations: a program with pending registry work is Blocked, not Applied, when an earlier failure stopped the apply before the registry loop. - Lowercase "agentdesktop" in the new prose (schema regenerated). Co-Authored-By: Claude Fable 5 --- .../agent/src/provider/claude_desktop/mod.rs | 2 +- .../src/provider/claude_desktop/reconcile.rs | 247 +++++++++++++++--- crates/agent/src/reconcile/mod.rs | 223 ++++++++++++++-- crates/agent/src/reconcile/plan.rs | 25 +- crates/agent/src/reconcile/registry.rs | 2 +- crates/core/src/config.rs | 2 +- schema/daemon-config.json | 2 +- schema/daemon-config.md | 2 +- 8 files changed, 435 insertions(+), 70 deletions(-) diff --git a/crates/agent/src/provider/claude_desktop/mod.rs b/crates/agent/src/provider/claude_desktop/mod.rs index be540cb4..6096c232 100644 --- a/crates/agent/src/provider/claude_desktop/mod.rs +++ b/crates/agent/src/provider/claude_desktop/mod.rs @@ -90,7 +90,7 @@ impl Provider for ClaudeDesktop { /// /// On Windows Claude Desktop reads its managed configuration from the /// `HKLM\SOFTWARE\Policies\Claude` registry key, never from a file, so the -/// reconciler targets that key and only removes what an earlier Agentdesktop +/// reconciler targets that key and only removes what an earlier agentdesktop /// wrote at this path. #[cfg(not(target_os = "macos"))] pub fn default_claude_desktop_managed_settings_path() -> PathBuf { diff --git a/crates/agent/src/provider/claude_desktop/reconcile.rs b/crates/agent/src/provider/claude_desktop/reconcile.rs index a6d8823d..58f49a89 100644 --- a/crates/agent/src/provider/claude_desktop/reconcile.rs +++ b/crates/agent/src/provider/claude_desktop/reconcile.rs @@ -26,7 +26,7 @@ const OWNER_MARKER: &[u8] = b"Agentdesktop\n"; #[cfg(any(windows, test))] pub(super) const POLICY_KEY: &str = r"SOFTWARE\Policies\Claude"; -/// The `REG_SZ` value under the policy key that marks what Agentdesktop +/// The `REG_SZ` value under the policy key that marks what agentdesktop /// manages, like the `.owner` sidecar next to a managed file: its data is a /// JSON array of the value names this daemon wrote, so updates and removal /// touch nothing else. To Claude Desktop the marker is a policy value like @@ -67,7 +67,7 @@ pub(super) fn plan( remove_owned(settings_path, &settings_owner, "managed settings", plan)?; } // On Windows the managed settings are machine policy registry - // values; the file above is only what an Agentdesktop without the + // values; the file above is only what an agentdesktop without the // registry target wrote. A user daemon never wrote the policy key. #[cfg(windows)] if !merge_existing { @@ -147,7 +147,7 @@ fn plan_managed_settings( ) } -/// Removes the managed-settings file an Agentdesktop without the registry +/// Removes the managed-settings file an agentdesktop without the registry /// target wrote at the path. Claude Desktop never read a file on Windows, /// so when no owned file is there nothing is recorded — recording would /// count the obsolete path as "unchanged" in every dry run. @@ -190,31 +190,55 @@ fn registry_values(settings: &Value) -> anyhow::Result> let Value::Object(settings) = settings else { anyhow::bail!("Claude Desktop managed settings serialize to an object"); }; - // Value names are case-insensitive to the registry, so every case - // variant of the marker's name is reserved. - if settings - .keys() - .any(|name| name.eq_ignore_ascii_case(REGISTRY_OWNER_VALUE)) - { - anyhow::bail!( - "Claude Desktop setting name {REGISTRY_OWNER_VALUE} is reserved for Agentdesktop's ownership marker" - ); + let mut values = BTreeMap::new(); + let mut folded: BTreeMap = BTreeMap::new(); + for (name, value) in settings { + // Value names are case-insensitive to the registry, so every case + // variant of the marker's name is reserved. + if name.eq_ignore_ascii_case(REGISTRY_OWNER_VALUE) { + anyhow::bail!( + "Claude Desktop setting name {REGISTRY_OWNER_VALUE} is reserved for agentdesktop's ownership marker" + ); + } + // `RegSetValueExW` reads the name up to its first NUL: an embedded + // NUL would write a shorter name than conflict checking compared and + // the ownership marker records. + if name.contains('\0') { + anyhow::bail!("Claude Desktop setting name {name:?} contains an embedded NUL"); + } + // The registry's maximum value-name length (one less than the + // `read_values` name buffer). The registry refuses a longer name at + // write time; refused here instead, before anything is written. + if name.encode_utf16().count() > 16_383 { + anyhow::bail!( + "Claude Desktop setting name of {} characters is longer than a registry value name allows", + name.chars().count() + ); + } + // Two names differing only in case address one registry value, so + // every reconciliation would rewrite it with whichever came later. + if let Some(previous) = folded.insert(name.to_ascii_lowercase(), name) { + anyhow::bail!( + "Claude Desktop setting names {previous} and {name} differ only in case and address the same registry value" + ); + } + let data = match value { + Value::String(text) => text.clone(), + other => other.to_string(), + }; + // `REG_SZ` data is NUL-terminated too; JSON-encoded data escapes + // every NUL, only a pass-through string can carry one. + if data.contains('\0') { + anyhow::bail!("Claude Desktop setting {name} contains an embedded NUL"); + } + values.insert(name.clone(), data); } - Ok(settings - .iter() - .map(|(name, value)| { - let data = match value { - Value::String(text) => text.clone(), - other => other.to_string(), - }; - (name.clone(), data) - }) - .collect()) + Ok(values) } /// Looks `name` up among the observed values the way the registry itself /// matches value names: case-insensitively. ASCII covers every name -/// Agentdesktop writes; the registry's own folding is locale-invariant +/// agentdesktop writes; the registry's own folding is locale-invariant /// Unicode, so a non-ASCII case variant is treated as a different name and /// errs on the side of writing it. #[cfg(any(windows, test))] @@ -276,8 +300,10 @@ fn registry_report_desired(desired: &BTreeMap) -> Vec { /// with the ownership semantics of `write_owned`: a value whose data already /// matches is claimed as ours, a differing value the marker does not claim /// is a conflict, and names the marker claims but `desired` dropped are -/// deleted. The marker never claims anything the plan does not write. Value -/// names match case-insensitively, as the registry stores them. +/// deleted. The marker never claims anything the plan does not touch, and +/// it is written before the values it claims (see the comment at the marker +/// changes below). Value names match case-insensitively, as the registry +/// stores them. /// /// An empty `desired` plans like removal: the ownership marker alone would /// switch Claude Desktop to machine policy and away from the user's `HKCU` @@ -320,19 +346,39 @@ fn plan_registry_settings( _ => changes.push((name.clone(), Some(data.clone()))), } } - for name in &owned { - if !desired.keys().any(|kept| kept.eq_ignore_ascii_case(name)) - && observes_name(&observed, name) - { - changes.push((name.clone(), None)); + let stale: Vec<&String> = owned + .iter() + .filter(|name| !desired.keys().any(|kept| kept.eq_ignore_ascii_case(name))) + .collect(); + for name in &stale { + if observes_name(&observed, name) { + changes.push(((*name).clone(), None)); } } - let marker = + // The marker brackets the writes so a partial apply stays recoverable: + // the first change claims every name this plan writes or deletes, so a + // failure midway leaves everything already touched claimed — removal + // still finds it and the next plan retries it. Once the stale names are + // gone the claim shrinks to exactly `desired`'s names, written last. + let full_claim = serde_json::to_string( + &desired + .keys() + .chain(stale.iter().copied()) + .collect::>(), + ) + .expect("value names serialize"); + let final_claim = serde_json::to_string(&desired.keys().collect::>()).expect("value names serialize"); if observed_value(&observed, REGISTRY_OWNER_VALUE).and_then(|data| data.as_deref()) - != Some(marker.as_str()) + != Some(full_claim.as_str()) { - changes.push((REGISTRY_OWNER_VALUE.to_owned(), Some(marker))); + changes.insert( + 0, + (REGISTRY_OWNER_VALUE.to_owned(), Some(full_claim.clone())), + ); + } + if final_claim != full_claim { + changes.push((REGISTRY_OWNER_VALUE.to_owned(), Some(final_claim))); } let action = if changes.iter().any(|(name, _)| name != REGISTRY_OWNER_VALUE) { if observed.is_empty() { @@ -808,6 +854,96 @@ programs: } } + #[test] + fn registry_values_reject_an_embedded_nul() { + // `RegSetValueExW` would truncate the name at the NUL and overwrite + // the value called `foreign`, which conflict checking compared + // against the full name and removal could never find again. + let error = super::registry_values(&serde_json::json!({ "foreign\0suffix": "x" })) + .unwrap_err() + .to_string(); + assert!(error.contains("embedded NUL"), "{error}"); + + // `REG_SZ` data is NUL-terminated the same way. + let error = super::registry_values(&serde_json::json!({ "name": "trun\0cated" })) + .unwrap_err() + .to_string(); + assert!(error.contains("embedded NUL"), "{error}"); + } + + #[test] + fn registry_values_reject_a_name_past_the_registry_limit() { + // 16,383 UTF-16 units is the registry's maximum value-name length; + // one more would make `RegSetValueExW` refuse at apply time, after + // earlier values of the plan were already written. + let at_limit = "a".repeat(16_383); + assert!(super::registry_values(&serde_json::json!({ at_limit: "x" })).is_ok()); + let past = "a".repeat(16_384); + let error = super::registry_values(&serde_json::json!({ past: "x" })) + .unwrap_err() + .to_string(); + assert!( + error.contains("longer than a registry value name"), + "{error}" + ); + } + + #[test] + fn registry_values_reject_names_differing_only_in_case() { + // Both names address the same registry value, so the reconciler + // would rewrite it on every tick with whichever came later and + // never converge. The realistic pair: a pass-through + // `inferenceprovider` next to the generated `inferenceProvider`. + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com + authentication: + type: controllerJwt + audience: agentgateway + allowedClientIds: [claude-desktop] +programs: + claudeDesktop: + inferenceprovider: direct +"#, + ) + .unwrap(); + let desktop = config.programs.claude_desktop.as_ref().unwrap(); + let settings = + managed_settings(desktop, config.llm_gateway.as_ref(), Path::new("/helper")).unwrap(); + let error = super::registry_values(&settings).unwrap_err().to_string(); + assert!(error.contains("differ only in case"), "{error}"); + assert!(error.contains("inferenceprovider"), "{error}"); + assert!(error.contains("inferenceProvider"), "{error}"); + } + + #[test] + fn registry_plan_retries_a_write_the_marker_claims_but_the_key_lacks() { + // The state a partial apply leaves behind: the marker (written + // first) claims a name whose value never landed. The next plan + // writes the missing value without a conflict. + let observed = RegistryValues::from([ + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider","isLocalDevMcpEnabled"]"#.to_owned()), + ), + ]); + let desired = BTreeMap::from([ + ("inferenceProvider".to_owned(), "gateway".to_owned()), + ("isLocalDevMcpEnabled".to_owned(), "true".to_owned()), + ]); + let key = RegistryKey::local_machine(super::POLICY_KEY); + let plan = ReconcilePlan::default(); + super::plan_registry_settings(&key, observed, &desired, &plan); + + assert!(!plan.has_conflicts()); + assert_eq!( + plan.registry_key_plans()[0].changes, + vec![("isLocalDevMcpEnabled".to_owned(), Some("true".to_owned()))] + ); + } + #[test] fn registry_plan_creates_values_and_the_ownership_marker() { let desired = BTreeMap::from([ @@ -821,15 +957,17 @@ programs: let plans = plan.registry_key_plans(); assert_eq!(plans.len(), 1); assert!(!plans[0].delete_key_when_empty); + // The marker comes first: a create that fails midway leaves every + // value already written claimed, so removal still finds it. assert_eq!( plans[0].changes, vec![ - ("inferenceProvider".to_owned(), Some("gateway".to_owned())), - ("isLocalDevMcpEnabled".to_owned(), Some("true".to_owned())), ( ".agentdesktop.owner".to_owned(), Some(r#"["inferenceProvider","isLocalDevMcpEnabled"]"#.to_owned()) ), + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ("isLocalDevMcpEnabled".to_owned(), Some("true".to_owned())), ] ); let rendered = plan.render(); @@ -1120,4 +1258,43 @@ programs: .contains("changed since reconciliation was planned") ); } + + /// A registry apply that fails after some writes landed: the marker was + /// written first and claims them, so removal cleans the key back up. + /// Constructed directly — `registry_values` refuses such a name at + /// planning, and the registry itself refuses it at write time + /// (measured on Windows: 16,383-character names ok, 16,384+ returns + /// rc 87 ERROR_INVALID_PARAMETER). + #[test] + #[cfg(windows)] + fn registry_partial_apply_failure_stays_recoverable() { + use crate::reconcile::registry::{RegistryKeyPlan, key_exists, read_values}; + let key = TestPolicyKey::new(); + let error = crate::reconcile::registry::apply(&RegistryKeyPlan { + key: key.0.clone(), + observed: RegistryValues::new(), + changes: vec![ + ( + ".agentdesktop.owner".to_owned(), + Some(r#"["inferenceProvider"]"#.to_owned()), + ), + ("inferenceProvider".to_owned(), Some("gateway".to_owned())), + ("z".repeat(16_384), Some("boom".to_owned())), + ], + delete_key_when_empty: false, + }) + .expect_err("the overlong name must fail the apply"); + assert!(error.to_string().contains("write"), "{error}"); + + // The write before the failure landed, claimed by the marker. + let values = read_values(&key.0).unwrap(); + assert_eq!(values["inferenceProvider"].as_deref(), Some("gateway")); + assert!(values.contains_key(".agentdesktop.owner")); + + // Removal finds everything through the marker and leaves no key. + let plan = ReconcilePlan::default(); + super::plan_registry_remove(&key.0, read_values(&key.0).unwrap(), &plan); + plan.apply().unwrap(); + assert!(!key_exists(&key.0).unwrap()); + } } diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index c54d7804..be282480 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -209,6 +209,7 @@ impl Reconciler { inactive: None, paths: Vec::new(), operations: 0..0, + registry_operations: 0..0, }; match provider.plan(&self.context, &config) { Err(error) => { @@ -222,6 +223,7 @@ impl Reconciler { entry.inactive = summary.inactive; entry.paths = summary.paths; let start = plan.operation_count(); + let registry_start = plan.registry_operation_count(); if let Err((error, path)) = plan.append_attributed(own) { // Both programs of the disagreement fail. for other in entries @@ -234,6 +236,7 @@ impl Reconciler { first_error.get_or_insert(error); } entry.operations = start..plan.operation_count(); + entry.registry_operations = registry_start..plan.registry_operation_count(); } } entries.push(entry); @@ -503,6 +506,9 @@ struct ProgramEntry { inactive: Option, paths: Vec, operations: std::ops::Range, + /// This program's registry key plans within the appended plan, applied + /// after every program's file operations. + registry_operations: std::ops::Range, } impl ProgramEntry { @@ -554,16 +560,18 @@ impl AttributedPlan { match plan.apply_tracked() { Ok(()) => (Ok(()), Stop::Completed), Err((error, at)) => { + // Registry changes apply after every file operation. + let (written_files, written_registry) = match &at { + plan::ApplyStop::Operation(index) => (*index, 0), + #[cfg(windows)] + plan::ApplyStop::Registry(index, _) => (usize::MAX, *index), + _ => (0, 0), + }; let stop = Stop::Failed { owners: owners_of(&entries, &at), detail: format!("{error:#}"), - written: match at { - plan::ApplyStop::Operation(index) => index, - // Registry changes apply after every file operation. - #[cfg(windows)] - plan::ApplyStop::Registry(_) => usize::MAX, - _ => 0, - }, + written_files, + written_registry, }; (Err(error), stop) } @@ -601,7 +609,14 @@ impl AttributedPlan { let written = match &stop { Stop::Completed => true, Stop::BeforeWrites => false, - Stop::Failed { written, .. } => entry.operations.end <= *written, + Stop::Failed { + written_files, + written_registry, + .. + } => { + entry.operations.end <= *written_files + && entry.registry_operations.end <= *written_registry + } }; entry.settled(written, &blocked_by)? }; @@ -624,12 +639,14 @@ enum Stop { Completed, /// Refused before any write (a failed plan or a conflict). BeforeWrites, - /// Stopped while applying: `owners` failed, the first `written` - /// operations reached the disk. + /// Stopped while applying: `owners` failed, the first `written_files` + /// file operations and the first `written_registry` registry key plans + /// were applied. Failed { owners: Vec<&'static str>, detail: String, - written: usize, + written_files: usize, + written_registry: usize, }, } @@ -641,7 +658,7 @@ fn owners_of(entries: &[ProgramEntry], at: &plan::ApplyStop) -> Vec<&'static str plan::ApplyStop::Conflict => false, plan::ApplyStop::Observed(path) => entry.paths.contains(path), #[cfg(windows)] - plan::ApplyStop::Registry(path) => entry.paths.contains(path), + plan::ApplyStop::Registry(_, path) => entry.paths.contains(path), plan::ApplyStop::PrivateDir(dir) => { entry.paths.iter().any(|path| path.starts_with(dir)) } @@ -1411,16 +1428,14 @@ programs: ); } - /// A registry write failure after the file operations: the failure is - /// attributed to Claude Desktop through `ApplyStop::Registry`, and the - /// file operations — all applied before the registry changes — stay. + /// A registry value name the registry itself would refuse fails at + /// planning (`registry_values` validation), before anything anywhere is + /// written: Claude Desktop is Failed and the other programs Blocked. #[cfg(windows)] #[test] - fn registry_write_failure_is_attributed_to_claude_desktop() { + fn registry_name_past_the_limit_fails_the_plan_before_any_write() { let fixture = Fixture::new(); - // A value name past the registry's 16,383-character limit makes - // `RegSetValueExW` refuse after every file operation was written. - // Measured on Windows: 16,383 chars ok / 16,384+ returns rc 87 ERROR_INVALID_PARAMETER. + // One past the registry's 16,383-character value-name limit. let long_name = "a".repeat(16_384); let mut claude_desktop_settings = serde_json::Map::new(); claude_desktop_settings.insert(long_name, serde_json::json!("boom")); @@ -1434,8 +1449,13 @@ programs: let (report, result) = fixture.reconciler.apply_with_report(&config); - let error = result.expect_err("the registry write must fail"); - assert!(error.to_string().contains("HKCU"), "{error}"); + let error = result.expect_err("the plan must fail validation"); + assert!( + error + .to_string() + .contains("longer than a registry value name"), + "{error}" + ); let outcome = |program: &str| { report .programs @@ -1444,14 +1464,167 @@ programs: .expect("program reported") }; assert_eq!(outcome("claude-desktop").state, ProgramState::Failed); + assert_eq!(outcome("claude-code").state, ProgramState::Blocked); + assert!( + !fixture.root.join("claude/settings.json").exists(), + "a failed plan must write nothing" + ); assert!( - outcome("claude-desktop").detail.contains("HKCU"), + !super::registry::key_exists(&fixture.policy_key.0).unwrap(), + "a failed plan must write no registry value" + ); + } + + /// A registry apply failure after the file operations, at the + /// `AttributedPlan` accounting layer (the planners can no longer produce + /// an invalid name, so the failing key plan is constructed directly): + /// the failure is attributed to Claude Desktop through + /// `ApplyStop::Registry`, and the file operations — all applied before + /// the registry changes — stay and count as written. + #[cfg(windows)] + #[test] + fn registry_apply_failure_is_attributed_and_keeps_earlier_file_writes() { + use super::registry::{RegistryKeyPlan, RegistryValues, TestPolicyKey}; + let key = TestPolicyKey::new(); + let dir = tempfile::tempdir().unwrap(); + let settings = dir.path().join("settings.json"); + + let plan = super::ReconcilePlan::default(); + plan.write_file(&settings, b"{}", 0o644).unwrap(); + plan.plan_registry_key(RegistryKeyPlan { + key: key.0.clone(), + observed: RegistryValues::new(), + // Past the registry's value-name limit: rc 87 at write time. + changes: vec![("z".repeat(16_384), Some("boom".to_owned()))], + delete_key_when_empty: false, + }); + let entries = vec![ + super::ProgramEntry { + program: "claude-code", + configured: true, + opted_out: false, + failed: None, + changes: true, + conflicts: Vec::new(), + inactive: None, + paths: vec![settings.clone()], + operations: 0..1, + registry_operations: 0..0, + }, + super::ProgramEntry { + program: "claude-desktop", + configured: true, + opted_out: false, + failed: None, + changes: true, + conflicts: Vec::new(), + inactive: None, + paths: vec![key.0.display_path()], + operations: 1..1, + registry_operations: 0..1, + }, + ]; + let (report, result) = super::AttributedPlan { + plan, + entries, + first_error: None, + } + .apply(); + + result.expect_err("the registry write must fail"); + let outcome = |program: &str| { + report + .programs + .iter() + .find(|outcome| outcome.program == program) + .expect("program reported") + }; + assert_eq!(outcome("claude-desktop").state, ProgramState::Failed); + assert_eq!(outcome("claude-code").state, ProgramState::Applied); + assert!(settings.is_file(), "earlier file writes stay"); + } + + /// A later provider's file write fails while Claude Desktop has pending + /// registry changes: registry changes run after every file operation, so + /// Claude Desktop's part never ran and must report Blocked, not Applied, + /// although its (empty) file range completed. The accounting is + /// platform-independent; the write failure is provoked through a + /// read-only directory, hence Unix. + #[cfg(unix)] + #[test] + fn pending_registry_work_blocks_completion_when_a_later_file_write_fails() { + use super::registry::{RegistryKey, RegistryKeyPlan, RegistryValues}; + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let locked = dir.path().join("locked"); + fs::create_dir(&locked).unwrap(); + let target = locked.join("config.toml"); + + let plan = super::ReconcilePlan::default(); + plan.plan_registry_key(RegistryKeyPlan { + key: RegistryKey::local_machine(r"SOFTWARE\Policies\Claude"), + observed: RegistryValues::new(), + changes: vec![("inferenceProvider".to_owned(), Some("gateway".to_owned()))], + delete_key_when_empty: false, + }); + plan.write_file(&target, b"managed", 0o644).unwrap(); + // Planned fine; the write fails at apply time. + fs::set_permissions(&locked, fs::Permissions::from_mode(0o555)).unwrap(); + + let entries = vec![ + super::ProgramEntry { + program: "claude-desktop", + configured: true, + opted_out: false, + failed: None, + changes: true, + conflicts: Vec::new(), + inactive: None, + paths: Vec::new(), + operations: 0..0, + registry_operations: 0..1, + }, + super::ProgramEntry { + program: "codex", + configured: true, + opted_out: false, + failed: None, + changes: true, + conflicts: Vec::new(), + inactive: None, + paths: vec![target.clone()], + operations: 0..1, + registry_operations: 1..1, + }, + ]; + let (report, result) = super::AttributedPlan { + plan, + entries, + first_error: None, + } + .apply(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o755)).unwrap(); + + result.expect_err("the file write must fail"); + let outcome = |program: &str| { + report + .programs + .iter() + .find(|outcome| outcome.program == program) + .expect("program reported") + }; + assert_eq!(outcome("codex").state, ProgramState::Failed); + assert_eq!( + outcome("claude-desktop").state, + ProgramState::Blocked, + "pending registry work must not count as applied: {}", + outcome("claude-desktop").detail + ); + assert!( + outcome("claude-desktop").detail.contains("codex failed"), "{}", outcome("claude-desktop").detail ); - // `written: usize::MAX`: Claude Code's files landed and stay. - assert_eq!(outcome("claude-code").state, ProgramState::Applied); - assert!(fixture.root.join("claude/settings.json").is_file()); } #[test] diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index 309443f2..ab100d08 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -51,10 +51,10 @@ pub(super) enum ApplyStop { PrivateDir(PathBuf), /// The operation at this index failed; the ones before it were written. Operation(usize), - /// The registry changes for this key failed; every file operation was - /// written before them. + /// The registry changes for the key at this index failed; every file + /// operation and the keys before it were applied. #[cfg(windows)] - Registry(PathBuf), + Registry(usize, PathBuf), } struct FileChange { @@ -103,6 +103,21 @@ impl ReconcilePlan { self.operations.borrow().len() } + /// How many registry key plans are queued, for per-program completion + /// accounting: registry changes apply after every file operation, so a + /// program with pending registry work is not complete when its file + /// range is. + pub(super) fn registry_operation_count(&self) -> usize { + #[cfg(any(windows, test))] + { + self.registry.borrow().len() + } + #[cfg(not(any(windows, test)))] + { + 0 + } + } + pub(super) fn summary(&self) -> PlanSummary { let observed = self.observed.borrow(); let operations = self.operations.borrow(); @@ -231,10 +246,10 @@ impl ReconcilePlan { tracing::info!(path = %change.path.display(), "applied reconciliation change"); } #[cfg(windows)] - for key_plan in self.registry.into_inner() { + for (index, key_plan) in self.registry.into_inner().into_iter().enumerate() { let key_path = key_plan.key.display_path(); super::registry::apply(&key_plan) - .map_err(|error| (error, ApplyStop::Registry(key_path.clone())))?; + .map_err(|error| (error, ApplyStop::Registry(index, key_path.clone())))?; tracing::info!(path = %key_path.display(), "applied reconciliation change"); } Ok(()) diff --git a/crates/agent/src/reconcile/registry.rs b/crates/agent/src/reconcile/registry.rs index edb8c549..6e312990 100644 --- a/crates/agent/src/reconcile/registry.rs +++ b/crates/agent/src/reconcile/registry.rs @@ -55,7 +55,7 @@ impl RegistryKey { } /// The values observed under a key: the `REG_SZ` data by value name, or -/// `None` for a value of any other type, which Agentdesktop never writes and +/// `None` for a value of any other type, which agentdesktop never writes and /// never matches. pub(crate) type RegistryValues = BTreeMap>; diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index d80feb5f..bd709213 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -147,7 +147,7 @@ pub struct UsageStartupConfig { pub struct ClaudeDesktopStartupConfig { /// Configuration file. On Windows the managed settings live in the /// policy registry key instead, and this file is only removed when an - /// earlier Agentdesktop wrote it. + /// earlier agentdesktop wrote it. #[serde(default, skip_serializing_if = "Option::is_none")] pub config: Option, /// Credential helper path. diff --git a/schema/daemon-config.json b/schema/daemon-config.json index 0c00d4f0..10391614 100644 --- a/schema/daemon-config.json +++ b/schema/daemon-config.json @@ -117,7 +117,7 @@ "type": "object", "properties": { "config": { - "description": "Configuration file. On Windows the managed settings live in the\npolicy registry key instead, and this file is only removed when an\nearlier Agentdesktop wrote it.", + "description": "Configuration file. On Windows the managed settings live in the\npolicy registry key instead, and this file is only removed when an\nearlier agentdesktop wrote it.", "type": [ "string", "null" diff --git a/schema/daemon-config.md b/schema/daemon-config.md index 701b3b1e..cd57ca93 100644 --- a/schema/daemon-config.md +++ b/schema/daemon-config.md @@ -11,7 +11,7 @@ |`daemon.claudeCode`|object|Claude Code paths.| |`daemon.claudeCode.config`|string|Configuration file.| |`daemon.claudeDesktop`|object|Claude Desktop paths.| -|`daemon.claudeDesktop.config`|string|Configuration file. On Windows the managed settings live in the
policy registry key instead, and this file is only removed when an
earlier Agentdesktop wrote it.| +|`daemon.claudeDesktop.config`|string|Configuration file. On Windows the managed settings live in the
policy registry key instead, and this file is only removed when an
earlier agentdesktop wrote it.| |`daemon.claudeDesktop.credentialHelper`|string|Credential helper path.| |`daemon.codex`|object|Codex paths.| |`daemon.codex.config`|string|Configuration file.|