diff --git a/crates/agent/src/daemon.rs b/crates/agent/src/daemon.rs index bbb8a35..5962094 100644 --- a/crates/agent/src/daemon.rs +++ b/crates/agent/src/daemon.rs @@ -227,12 +227,20 @@ impl DaemonArgs { vscode_chat_models: None, vscode_settings: None, pi: { - if startup.pi.models.is_some() || startup.pi.settings.is_some() { - bail!("daemon.pi paths require --user (or daemon.user: true)"); - } - ResolvedPiStartupConfig { - models: None, - settings: None, + // Pi has no system-wide configuration. A system daemon manages + // it only for the one user whose files are named here. + match (startup.pi.models, startup.pi.settings) { + (None, None) => ResolvedPiStartupConfig { + models: None, + settings: None, + }, + (Some(models), Some(settings)) => ResolvedPiStartupConfig { + models: Some(models), + settings: Some(settings), + }, + _ => bail!( + "daemon.pi.models and daemon.pi.settings must both be set to manage Pi from a system daemon" + ), } }, reconcile_interval: startup.reconcile_interval, @@ -1408,7 +1416,7 @@ mod tests { use tokio::sync::{mpsc, watch}; #[test] - fn pi_startup_paths_require_user_mode() { + fn pi_startup_paths_resolve_in_both_modes() { for user in [false, true] { let file = tempfile::NamedTempFile::new().unwrap(); std::fs::write( @@ -1430,17 +1438,8 @@ mod tests { file.path().to_path_buf(), agentdesktop_core::DEFAULT_SOCKET_PATH.into(), ); - if !user { - assert!( - result - .err() - .unwrap() - .to_string() - .contains("daemon.pi paths require --user") - ); - continue; - } - let resolved = result.unwrap(); + // A system daemon uses them to manage that one user's Pi. + let resolved = result.unwrap_or_else(|error| panic!("user={user}: {error:#}")); assert_eq!( resolved.pi.models.as_deref(), Some(Path::new("custom/pi-models.json")) @@ -2323,6 +2322,19 @@ programs: assert_eq!(resolved.reconcile_interval, Some(Duration::from_secs(120))); } + #[test] + fn system_mode_rejects_a_single_pi_path() { + let mut startup = config::DaemonStartupConfig::default(); + startup.pi.models = Some(PathBuf::from("/home/dev/.pi/agent/models.json")); + let error = resolve_error(daemon_args(false), startup); + assert!( + error + .to_string() + .contains("daemon.pi.models and daemon.pi.settings must both be set"), + "{error:#}" + ); + } + #[test] fn system_mode_rejects_a_vscode_settings_path() { let mut startup = config::DaemonStartupConfig::default(); diff --git a/crates/agent/src/provider/pi/mod.rs b/crates/agent/src/provider/pi/mod.rs index ff03146..542add6 100644 --- a/crates/agent/src/provider/pi/mod.rs +++ b/crates/agent/src/provider/pi/mod.rs @@ -11,9 +11,13 @@ mod reconcile; #[derive(Clone, Default)] pub struct Pi { - /// User paths only; system daemons have no Pi files to manage. + /// User daemons always have these. A system daemon has them only when + /// `daemon.pi.models` and `daemon.pi.settings` name one user's files. pub models_path: Option, pub settings_path: Option, + /// Set for a system daemon configured with `daemon.pi` paths: manage that + /// one user's Pi files instead of skipping Pi as user-only. + pub manage_for_user: bool, } impl Pi { @@ -36,11 +40,15 @@ impl Provider for Pi { } fn plan(&self, ctx: &ReconcileContext, config: &DaemonConfig) -> anyhow::Result { - if let Some(plan) = ctx.skip_unless( - DaemonScope::User, - config.programs.pi.is_some(), - Self::DISPLAY_NAME, - ) { + // A system daemon manages Pi only for a user whose files are configured. + let for_user = !ctx.merge_user_settings && self.manage_for_user; + if !for_user + && let Some(plan) = ctx.skip_unless( + DaemonScope::User, + config.programs.pi.is_some(), + Self::DISPLAY_NAME, + ) + { return Ok(plan); } let plan = ReconcilePlan::default(); @@ -55,6 +63,11 @@ impl Provider for Pi { .filter(|_| provider.use_llm_gateway); (provider, gateway) }); + if for_user { + // Pi reads these as the user, so files and any directories the plan + // creates belong to whoever owns the user's Pi directory. + plan.set_owner(owner_of_nearest(models_path)); + } reconcile::plan( models_path, settings_path, @@ -67,6 +80,28 @@ impl Provider for Pi { } } +/// Owner of the closest existing ancestor of `path`, so a system daemon can +/// hand Pi's files to the user they belong to. `None` off Unix or if nothing +/// along the path can be inspected. +fn owner_of_nearest(path: &std::path::Path) -> Option { + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + path.ancestors() + .skip(1) + .find_map(|dir| std::fs::metadata(dir).ok()) + .map(|metadata| crate::reconcile::plan::FileOwner { + uid: metadata.uid(), + gid: metadata.gid(), + }) + } + #[cfg(not(unix))] + { + let _ = path; + None + } +} + pub fn user_pi_agent_dir(home: &std::path::Path) -> PathBuf { std::env::var_os("PI_CODING_AGENT_DIR") .filter(|value| !value.is_empty()) @@ -91,9 +126,11 @@ mod tests { #[test] fn skips_configured_pi_in_system_mode() { let root = tempfile::tempdir().unwrap(); + // User paths alone do not opt a system daemon in (daemon.pi does). let pi = Pi { models_path: Some(root.path().join("models.json")), settings_path: Some(root.path().join("settings.json")), + manage_for_user: false, }; let ctx = ReconcileContext { llm_proxy: None, @@ -109,8 +146,8 @@ mod tests { .expect("Pi is skipped in system mode"); assert!(plan.inactive_reason().unwrap().contains("--user")); plan.apply().unwrap(); - assert!(!pi.models_path.as_ref().unwrap().exists()); - assert!(!pi.settings_path.as_ref().unwrap().exists()); + assert!(!root.path().join("models.json").exists()); + assert!(!root.path().join("settings.json").exists()); } #[test] @@ -119,6 +156,7 @@ mod tests { let pi = Pi { models_path: Some(root.path().join("models.json")), settings_path: Some(root.path().join("settings.json")), + manage_for_user: false, }; let mut ctx = ReconcileContext { llm_proxy: None, @@ -132,7 +170,8 @@ mod tests { assert!(pi.models_path.as_ref().unwrap().exists()); ctx.merge_user_settings = false; - pi.plan(&ctx, &DaemonConfig::default()) + Pi::default() + .plan(&ctx, &DaemonConfig::default()) .unwrap() .apply() .unwrap(); @@ -153,4 +192,40 @@ mod tests { .apply() .unwrap(); } + + #[test] + fn system_mode_manages_a_configured_users_pi_files() { + let root = tempfile::tempdir().unwrap(); + let agent_dir = root.path().join("home/dev/.pi/agent"); + let pi = Pi { + models_path: Some(agent_dir.join("models.json")), + settings_path: Some(agent_dir.join("settings.json")), + manage_for_user: true, + }; + let ctx = ReconcileContext { + llm_proxy: None, + merge_user_settings: false, + credential_helper: root.path().join("agentdesktop"), + socket: root.path().join("agentdesktop.sock"), + }; + let config = + parse_daemon("programs:\n pi:\n managedConfig:\n providers: {}\n").unwrap(); + + let plan = pi.plan(&ctx, &config).unwrap(); + assert!(plan.inactive_reason().is_none()); + plan.apply().unwrap(); + assert!(pi.models_path.as_ref().unwrap().exists()); + + // Files and the directories created for them belong to the owner of the + // nearest existing ancestor (here the test user). + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let owner = std::fs::metadata(root.path()).unwrap(); + for path in [pi.models_path.as_ref().unwrap(), &agent_dir] { + let metadata = std::fs::metadata(path).unwrap(); + assert_eq!((metadata.uid(), metadata.gid()), (owner.uid(), owner.gid())); + } + } + } } diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index e7be69b..94574e5 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -107,6 +107,11 @@ impl Reconciler { providers_path: copilot_providers_path, }), Box::new(Pi { + // A system daemon gets Pi paths only from daemon.pi, which + // opts it in to managing that one user's Pi. + manage_for_user: !merge_user_settings + && pi_models_path.is_some() + && pi_settings_path.is_some(), models_path: pi_models_path, settings_path: pi_settings_path, }), diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index 5f135eb..b9d4b86 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -3,7 +3,7 @@ use super::DryRunReport; use super::registry::RegistryKeyPlan; use anyhow::Context; use std::{ - cell::RefCell, + cell::{Cell, RefCell}, collections::BTreeMap, fs, io, path::{Path, PathBuf}, @@ -26,6 +26,17 @@ pub struct ReconcilePlan { /// Why the configured program cannot be managed in this daemon's mode or /// cannot use the gateway (the loopback proxy is absent). inactive: RefCell>, + /// Owner for files and directories this plan creates, when a system daemon + /// manages a file in one user's profile. `None` keeps the daemon's owner. + owner: Cell>, +} + +/// The user and group a planned file or directory is handed to after it is +/// written. Applied on Unix only. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct FileOwner { + pub uid: u32, + pub gid: u32, } /// What one provider's plan contains, taken before it is appended to the @@ -61,13 +72,14 @@ struct FileChange { path: PathBuf, contents: Option>, permissions: u32, + owner: Option, } /// A directory to create owner-only before the file changes are applied. /// Used for a directory that will hold a secret-bearing file and does not /// exist yet; an existing directory is left as it is. #[derive(Default)] -struct PrivateDirs(RefCell>); +struct PrivateDirs(RefCell)>>); impl ReconcilePlan { pub fn render(&self) -> String { @@ -221,9 +233,11 @@ impl ReconcilePlan { )); } } - for dir in self.private_dirs.0.into_inner() { + for (dir, owner) in self.private_dirs.0.into_inner() { if !dir.exists() { + let created = missing_dirs(&dir); crate::secure_fs::ensure_private_dir(&dir) + .and_then(|()| apply_owner(&created, owner)) .map_err(|error| (error, ApplyStop::PrivateDir(dir.clone())))?; } } @@ -231,6 +245,7 @@ impl ReconcilePlan { for (index, change) in self.operations.into_inner().into_iter().enumerate() { if let Some(contents) = &change.contents && already_in_place(&change.path, contents, change.permissions, &observed) + && owned_by(&change.path, change.owner) { continue; } @@ -242,9 +257,17 @@ impl ReconcilePlan { .parent() .filter(|p| !p.as_os_str().is_empty()) .unwrap_or_else(|| Path::new(".")); + let created = missing_dirs(parent); fs::create_dir_all(parent) .with_context(|| format!("create directory {}", parent.display()))?; - crate::secure_fs::atomic_write(&change.path, &contents, change.permissions) + crate::secure_fs::atomic_write( + &change.path, + &contents, + change.permissions, + )?; + let mut owned = created; + owned.push(change.path.clone()); + apply_owner(&owned, change.owner) } None => fs::remove_file(&change.path) .with_context(|| format!("remove {}", change.path.display())), @@ -359,13 +382,23 @@ impl ReconcilePlan { path: path.to_owned(), contents: Some(contents.to_vec()), permissions, + owner: self.owner.get(), }); Ok(()) } /// Create `dir` owner-only at apply time if it does not exist by then. pub(crate) fn ensure_private_dir(&self, dir: &Path) { - self.private_dirs.0.borrow_mut().push(dir.to_owned()); + self.private_dirs + .0 + .borrow_mut() + .push((dir.to_owned(), self.owner.get())); + } + + /// Hand every file and directory this plan creates to `owner` (Unix). + /// Used when a system daemon manages a file inside one user's profile. + pub(crate) fn set_owner(&self, owner: Option) { + self.owner.set(owner); } /// Plan one Windows registry key's changes, applied after the file @@ -387,6 +420,7 @@ impl ReconcilePlan { path: path.to_owned(), contents: None, permissions: 0, + owner: None, }); Ok(()) } @@ -446,6 +480,49 @@ fn already_in_place( /// Whether the file at `path` grants a permission bit outside `mode`. /// Unix: the current mode has a bit outside `mode`. A missing file, and any /// file on non-Unix, is false. +/// Directories from `dir` upward that do not exist yet, deepest first, so the +/// ones a write creates can be handed to the plan's owner afterwards. +fn missing_dirs(dir: &Path) -> Vec { + let mut missing = Vec::new(); + let mut current = Some(dir); + while let Some(path) = current { + if path.as_os_str().is_empty() || path.exists() { + break; + } + missing.push(path.to_path_buf()); + current = path.parent(); + } + missing +} + +/// Give `paths` to `owner`. A no-op without an owner and on non-Unix systems. +fn apply_owner(paths: &[PathBuf], owner: Option) -> anyhow::Result<()> { + #[cfg(unix)] + if let Some(owner) = owner { + for path in paths { + std::os::unix::fs::chown(path, Some(owner.uid), Some(owner.gid)) + .with_context(|| format!("set owner of {}", path.display()))?; + } + } + #[cfg(not(unix))] + let _ = (paths, owner); + Ok(()) +} + +/// Whether `path` already belongs to `owner` (always true without an owner). +fn owned_by(path: &Path, owner: Option) -> bool { + #[cfg(unix)] + if let Some(owner) = owner { + use std::os::unix::fs::MetadataExt; + return fs::metadata(path) + .map(|metadata| metadata.uid() == owner.uid && metadata.gid() == owner.gid) + .unwrap_or(false); + } + #[cfg(not(unix))] + let _ = (path, owner); + true +} + pub(crate) fn grants_beyond(path: &Path, mode: u32) -> bool { #[cfg(unix)] { diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 83c7c12..2149028 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -205,7 +205,9 @@ pub struct OpenCodeStartupConfig { #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct PiStartupConfig { - /// Model catalog file. + /// Model catalog file. A user daemon defaults to Pi's agent directory. A + /// system daemon manages Pi only when this and `settings` name one user's + /// files; it writes them as the owner of that user's Pi directory. #[serde(default, skip_serializing_if = "Option::is_none")] pub models: Option, /// Settings file. @@ -1189,7 +1191,8 @@ impl VsCodeConfig { /// /// Generated `providers.agentdesktop` values take precedence over overlapping /// `managedConfig` keys. Pi stores configuration under `~/.pi/agent` (or -/// `PI_CODING_AGENT_DIR`), so management requires `--user`. System daemons skip it. +/// `PI_CODING_AGENT_DIR`), so a user daemon manages it; a system daemon manages +/// it only for the user named by `daemon.pi.models` and `daemon.pi.settings`. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] #[serde(rename_all = "camelCase", deny_unknown_fields)] diff --git a/schema/daemon-config.json b/schema/daemon-config.json index 377fc81..2542b06 100644 --- a/schema/daemon-config.json +++ b/schema/daemon-config.json @@ -593,7 +593,7 @@ "additionalProperties": false }, "PiConfig": { - "description": "Settings reconciled into Pi's user `models.json` and `settings.json`.\n\nGenerated `providers.agentdesktop` values take precedence over overlapping\n`managedConfig` keys. Pi stores configuration under `~/.pi/agent` (or\n`PI_CODING_AGENT_DIR`), so management requires `--user`. System daemons skip it.", + "description": "Settings reconciled into Pi's user `models.json` and `settings.json`.\n\nGenerated `providers.agentdesktop` values take precedence over overlapping\n`managedConfig` keys. Pi stores configuration under `~/.pi/agent` (or\n`PI_CODING_AGENT_DIR`), so a user daemon manages it; a system daemon manages\nit only for the user named by `daemon.pi.models` and `daemon.pi.settings`.", "type": "object", "properties": { "api": { @@ -632,7 +632,7 @@ "type": "object", "properties": { "models": { - "description": "Model catalog file.", + "description": "Model catalog file. A user daemon defaults to Pi's agent directory. A\nsystem daemon manages Pi only when this and `settings` name one user's\nfiles; it writes them as the owner of that user's Pi directory.", "type": [ "string", "null" diff --git a/schema/daemon-config.md b/schema/daemon-config.md index 81e5fe7..c1806f5 100644 --- a/schema/daemon-config.md +++ b/schema/daemon-config.md @@ -27,7 +27,7 @@ |`daemon.openCode.config`|string|Configuration file.| |`daemon.openCode.plugin`|string|Credential plugin path.| |`daemon.pi`|object|Pi paths.| -|`daemon.pi.models`|string|Model catalog file.| +|`daemon.pi.models`|string|Model catalog file. A user daemon defaults to Pi's agent directory. A
system daemon manages Pi only when this and `settings` name one user's
files; it writes them as the owner of that user's Pi directory.| |`daemon.pi.settings`|string|Settings file.| |`daemon.reconcileInterval`|string|Opt-in interval between periodic re-applies of the current
configuration, which repair drift (a managed file deleted or edited by
hand, a fixed conflict, a loosened mode) without rewriting anything
unchanged. Unset means no periodic re-apply; a controller-managed
device still re-applies the controller's configuration on every
reconnect. Must be greater than zero and at most 30 days; read at
startup only (restart the daemon after changing it).| |`daemon.socket`|string|Local API Unix socket or Windows named pipe.|