Skip to content

Let a system daemon manage Pi for one configured user #196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebbycorp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Let a system daemon manage Pi for one configured user #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 30 additions & 18 deletions crates/agent/src/daemon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Comment on lines +237 to +239
},
_ => bail!(
"daemon.pi.models and daemon.pi.settings must both be set to manage Pi from a system daemon"
),
}
},
reconcile_interval: startup.reconcile_interval,
Expand Down Expand Up @@ -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(
Expand All @@ -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"))
Expand Down Expand Up @@ -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();
Expand Down
93 changes: 84 additions & 9 deletions crates/agent/src/provider/pi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf>,
pub settings_path: Option<PathBuf>,
/// 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 {
Expand All @@ -36,11 +40,15 @@ impl Provider for Pi {
}

fn plan(&self, ctx: &ReconcileContext, config: &DaemonConfig) -> anyhow::Result<ReconcilePlan> {
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();
Expand All @@ -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,
Expand All @@ -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<crate::reconcile::plan::FileOwner> {
#[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())
Expand All @@ -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,
Expand All @@ -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]
Expand All @@ -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,
Expand All @@ -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();
Expand All @@ -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()));
}
}
}
}
5 changes: 5 additions & 0 deletions crates/agent/src/reconcile/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
Expand Down
Loading
Loading