From 211be464a7ae7895d003c280fb72ad506819ad64 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Wed, 30 Sep 2026 18:42:45 +0200 Subject: [PATCH 1/5] feat: report per-program configuration status to the controller Each apply reports every managed program's outcome with a detail: applied, unchanged, removed, conflict, inactive (the program uses the gateway but the local proxy is not running), blocked (not applied because another program conflicted) or failed. The daemon logs one line per program that was not applied and sends the list with its ConfigStatus (additive proto fields `programs`, `programs_reported`). The controller stores the latest list per device (migration 0003, `device_program_status`), returns it in `GET /api/v1/devices/{id}`, and the device page shows a "Managed programs" card. Upgrade the controller first (migration 0003 is one-way). Co-Authored-By: Claude Opus 5.5 --- README.md | 37 + crates/agent/src/daemon.rs | 15 +- crates/agent/src/llm_proxy.rs | 7 +- crates/agent/src/provider/claude_code/mod.rs | 4 + .../agent/src/provider/claude_desktop/mod.rs | 4 + crates/agent/src/provider/codex/mod.rs | 4 + crates/agent/src/provider/copilot/mod.rs | 4 + .../agent/src/provider/copilot/reconcile.rs | 1 + crates/agent/src/provider/cursor/mod.rs | 4 + crates/agent/src/provider/grok/mod.rs | 4 + crates/agent/src/provider/json_merge.rs | 12 +- crates/agent/src/provider/mod.rs | 4 + crates/agent/src/provider/ollama/mod.rs | 4 + crates/agent/src/provider/opencode/mod.rs | 4 + .../src/provider/vscode/integration_tests.rs | 58 + crates/agent/src/provider/vscode/mod.rs | 4 + crates/agent/src/provider/vscode/reconcile.rs | 1 + crates/agent/src/provider/vscode/settings.rs | 1 + crates/agent/src/reconcile/mod.rs | 1046 ++++++++++++++++- crates/agent/src/reconcile/plan.rs | 186 ++- crates/agent/src/remote.rs | 107 +- .../migrations/0003_program_status.sql | 14 + crates/controller/src/database.rs | 432 ++++++- crates/controller/src/service.rs | 89 +- crates/proto/proto/fleet.proto | 29 + frontend/controller/src/stories/fixtures.ts | 71 ++ frontend/controller/src/styles.css | 22 + frontend/controller/src/types.ts | 18 + .../src/views/DeviceView.stories.tsx | 20 + frontend/controller/src/views/DeviceView.tsx | 65 +- 30 files changed, 2205 insertions(+), 66 deletions(-) create mode 100644 crates/controller/migrations/0003_program_status.sql diff --git a/README.md b/README.md index 4a6ea14e..e596aac9 100644 --- a/README.md +++ b/README.md @@ -587,6 +587,43 @@ stage. ![Agentdesktop controller device inventory](images/controller-ui.png) +### Configuration status + +Each apply reports the outcome for every managed program, next to the +device-wide state. The device page in the controller lists them under +"Managed programs" (`GET /api/v1/devices/{id}` returns them as `programs`): + +| State | Meaning | +| --- | --- | +| `applied` | The program is configured and its files were changed. | +| `unchanged` | The program is configured and nothing needed to change. | +| `removed` | The program is no longer configured and its managed content was removed. | +| `conflict` | A managed file holds configuration the daemon will not overwrite; the detail names the file. | +| `inactive` | The program uses the LLM gateway but the local LLM proxy is not running (see `llmProxy.error` in daemon-info), so its files point nowhere and were removed. | +| `blocked` | The program had changes, but none were written because another program conflicted or failed; the detail names that program. | +| `failed` | Planning or writing this program failed; the detail carries the error. | + +When several apply, the first in the list `failed`, `conflict`, `blocked`, +`inactive`, `applied`/`removed`, `unchanged` is shown. An apply is all or +nothing across programs: one conflict or failure means no file is written for +any program, which is what `blocked` makes visible. Programs that are not +configured and have nothing to clean up, and discovery-only tools, are not +listed. After each apply the daemon logs one `program configuration outcome` +line (`program`, `state`, `detail`) per program that is not `applied` or +`unchanged`. A daemon restart reports again, because the controller re-sends +the configuration on every connection; the files are already written by the +startup apply of the saved configuration at that point, so the report after a +restart usually says `unchanged` (the startup apply's outcome lines for programs +that are not `applied` or `unchanged` are in the daemon log). If the controller +rejects a report (an agent outside the reporting limits), it keeps the device +status and the last accepted program rows; rows from an earlier revision show +that revision on the device page. Agents and controllers can be upgraded +in either order: an older agent shows as "Per-program status is not reported +by this agent version", and an older controller ignores the new fields. The +controller's database migration is one-way: once this controller has run, an +older controller refuses the same database, so roll back from a database +backup taken before the upgrade. + ## Core capabilities - **AI tool discovery:** detect supported developer tools and their versions diff --git a/crates/agent/src/daemon.rs b/crates/agent/src/daemon.rs index 64b38d31..a5d2f049 100644 --- a/crates/agent/src/daemon.rs +++ b/crates/agent/src/daemon.rs @@ -430,9 +430,9 @@ where .context("preview daemon configuration")?; } else { validate_one_shot(&config)?; - reconciler - .apply(&config) - .context("apply daemon configuration")?; + let (report, applied) = reconciler.apply_with_report(&config); + report.log(); + applied.context("apply daemon configuration")?; println!("Reconciliation complete."); } return Ok(()); @@ -480,9 +480,10 @@ where Some(local_config) }; if let Some(initial_config) = initial_config { - reconciler - .apply(&initial_config) - .context("apply initial daemon configuration")?; + // The outcomes are logged before a failure stops the daemon. + let (report, applied) = reconciler.apply_with_report(&initial_config); + report.log(); + applied.context("apply initial daemon configuration")?; // With a controller, the cached controller configuration is what is in effect // from here, and start_gateway_authentication above only saw the local file. // Without one the two are the same configuration and it already ran. @@ -607,7 +608,7 @@ fn attach_llm_proxy_pairing( /// failure is logged and reported as `bound: false` rather than propagated: /// the proxy is optional, the rest of the daemon is not. /// -/// Invariant for callers: this runs before the initial `reconciler.apply`, so a +/// Invariant for callers: this runs before the initial apply, so a /// reconciler that writes the proxy address into a client file must take the /// address from this result and must not write it when `bound` is false. The /// accept loop starts later, after discovery; connections in between queue in diff --git a/crates/agent/src/llm_proxy.rs b/crates/agent/src/llm_proxy.rs index 04beecbe..b9ea6522 100644 --- a/crates/agent/src/llm_proxy.rs +++ b/crates/agent/src/llm_proxy.rs @@ -3411,13 +3411,16 @@ mod tests { let key = format!("vscode-copilot\u{0}http://{address}/\u{0}agentgateway"); // Inserted directly (bypassing `insert`'s expiry-margin floor, // which would otherwise refuse to cache anything this close to - // expiry) so the tunnel has time to open before the deadline. + // expiry). The cache reuses the entry for a minute, so the + // request always gets it however slowly it arrives; the + // credential itself expires in 300 ms, and that is the deadline + // the tunnel must close at. cache.entries.lock().unwrap().insert( key, CachedCredential { credential: "short-lived".to_owned(), device_id: "device-test".to_owned(), - valid_until: std::time::Instant::now() + Duration::from_millis(300), + valid_until: std::time::Instant::now() + Duration::from_secs(60), valid_until_unix: now_unix() + 60, expires: std::time::Instant::now() + Duration::from_millis(300), expires_unix: now_unix() + 60, diff --git a/crates/agent/src/provider/claude_code/mod.rs b/crates/agent/src/provider/claude_code/mod.rs index 1acb4f8c..0c016487 100644 --- a/crates/agent/src/provider/claude_code/mod.rs +++ b/crates/agent/src/provider/claude_code/mod.rs @@ -70,6 +70,10 @@ impl ClaudeCode { #[async_trait::async_trait] impl Provider for ClaudeCode { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover(&self.settings_path) diff --git a/crates/agent/src/provider/claude_desktop/mod.rs b/crates/agent/src/provider/claude_desktop/mod.rs index cbac676e..e6a09e58 100644 --- a/crates/agent/src/provider/claude_desktop/mod.rs +++ b/crates/agent/src/provider/claude_desktop/mod.rs @@ -31,6 +31,10 @@ impl ClaudeDesktop { #[async_trait::async_trait] impl Provider for ClaudeDesktop { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover(&self.managed_settings_path) diff --git a/crates/agent/src/provider/codex/mod.rs b/crates/agent/src/provider/codex/mod.rs index afbfe16b..0fceb0c6 100644 --- a/crates/agent/src/provider/codex/mod.rs +++ b/crates/agent/src/provider/codex/mod.rs @@ -29,6 +29,10 @@ impl Codex { #[async_trait::async_trait] impl Provider for Codex { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/copilot/mod.rs b/crates/agent/src/provider/copilot/mod.rs index 1b31f2a7..c461b9a3 100644 --- a/crates/agent/src/provider/copilot/mod.rs +++ b/crates/agent/src/provider/copilot/mod.rs @@ -40,6 +40,10 @@ pub fn default_copilot_providers_path() -> anyhow::Result { #[async_trait::async_trait] impl Provider for Copilot { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/copilot/reconcile.rs b/crates/agent/src/provider/copilot/reconcile.rs index c1aaeb48..ffb2442c 100644 --- a/crates/agent/src/provider/copilot/reconcile.rs +++ b/crates/agent/src/provider/copilot/reconcile.rs @@ -112,6 +112,7 @@ pub(super) fn plan( path = %path.display(), "programs.copilot is configured but the local LLM proxy is not available, so the Copilot CLI is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed Copilot CLI providers" ); + plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); remove(path, &state_path, None, plan)?; return Ok(()); }; diff --git a/crates/agent/src/provider/cursor/mod.rs b/crates/agent/src/provider/cursor/mod.rs index d549ead7..0877f61f 100644 --- a/crates/agent/src/provider/cursor/mod.rs +++ b/crates/agent/src/provider/cursor/mod.rs @@ -13,6 +13,10 @@ impl Cursor { #[async_trait::async_trait] impl Provider for Cursor { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/grok/mod.rs b/crates/agent/src/provider/grok/mod.rs index 80c463f8..b91e24e5 100644 --- a/crates/agent/src/provider/grok/mod.rs +++ b/crates/agent/src/provider/grok/mod.rs @@ -29,6 +29,10 @@ impl Grok { #[async_trait::async_trait] impl Provider for Grok { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/json_merge.rs b/crates/agent/src/provider/json_merge.rs index bcbe6eb1..9c8e7faa 100644 --- a/crates/agent/src/provider/json_merge.rs +++ b/crates/agent/src/provider/json_merge.rs @@ -336,8 +336,18 @@ fn read_state( plan: &ReconcilePlan, ) -> anyhow::Result> { match plan.read(path) { + // The parse error names the position only: serde's message can quote + // a value from the file, and the sidecar holds managed content (a + // pairing value, a credential helper command). Ok(contents) => serde_json::from_slice(&contents) - .with_context(|| format!("parse {display_name} merge state from {}", path.display())) + .map_err(|error| { + anyhow::anyhow!( + "parse {display_name} merge state from {}: not a valid merge state (line {}, column {})", + path.display(), + error.line(), + error.column() + ) + }) .map(Some), Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), Err(error) => Err(error) diff --git a/crates/agent/src/provider/mod.rs b/crates/agent/src/provider/mod.rs index 465f8cf3..b22310d3 100644 --- a/crates/agent/src/provider/mod.rs +++ b/crates/agent/src/provider/mod.rs @@ -24,6 +24,10 @@ pub mod vscode; /// An integration with a developer tool or local model runtime. #[async_trait::async_trait] pub trait Provider: Send + Sync { + /// The provider's ID (the inventory's agent kind and the program key in + /// configuration status reports). + fn id(&self) -> &'static str; + async fn discover(&self) -> Discovery; /// Propose configuration changes, including cleanup when disabled. diff --git a/crates/agent/src/provider/ollama/mod.rs b/crates/agent/src/provider/ollama/mod.rs index f69a9579..e05a75b8 100644 --- a/crates/agent/src/provider/ollama/mod.rs +++ b/crates/agent/src/provider/ollama/mod.rs @@ -13,6 +13,10 @@ impl Ollama { #[async_trait::async_trait] impl Provider for Ollama { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: Vec::new(), diff --git a/crates/agent/src/provider/opencode/mod.rs b/crates/agent/src/provider/opencode/mod.rs index de739e3b..9c86ebe9 100644 --- a/crates/agent/src/provider/opencode/mod.rs +++ b/crates/agent/src/provider/opencode/mod.rs @@ -31,6 +31,10 @@ impl OpenCode { #[async_trait::async_trait] impl Provider for OpenCode { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/vscode/integration_tests.rs b/crates/agent/src/provider/vscode/integration_tests.rs index f3694e20..7eaada09 100644 --- a/crates/agent/src/provider/vscode/integration_tests.rs +++ b/crates/agent/src/provider/vscode/integration_tests.rs @@ -395,3 +395,61 @@ fn jsonc(text: &str) -> anyhow::Result { .and_then(|value| value.to_serde_value()) .context("settings.json holds no JSON value") } + +#[tokio::test] +async fn program_status_reports_inactive_without_the_proxy() -> anyhow::Result<()> { + Container::run( + "vscode", + "crates/agent/src/provider/vscode/testdata/Dockerfile", + async |container| { + let gateway = Gateway::start().await?; + // A gateway and the VS Code program, but no daemon.llmProxy: the + // program uses the gateway without a listener, so it is inactive. + let document = serde_json::json!({ + "daemon": { "user": true }, + "llmGateway": { "url": gateway.url }, + "programs": { "vscode": { "models": { "gpt-4.1-mini": {} } } }, + }); + container + .write(CONFIG, &serde_json::to_string_pretty(&document)?) + .await?; + container.exec(&["chown", "tester:tester", CONFIG]).await?; + container + .exec(&["chown", "-R", "tester:tester", "/home/tester"]) + .await?; + let daemon = [ + "runuser", + "-u", + "tester", + "--", + "agentdesktop", + "daemon", + "--config", + CONFIG, + ]; + container.start_process("daemon", &daemon).await?; + container + .wait_ready(&["agentdesktop", "--socket", SOCKET, "status"]) + .await?; + + info!("Checking the program outcome line in the daemon log"); + let log = container.exec(&["cat", "/tmp/daemon.log"]).await?; + let line = log + .lines() + .find(|line| { + line.contains("program configuration outcome") && line.contains("vscode") + }) + .with_context(|| format!("no outcome line for vscode in the daemon log:\n{log}"))?; + ensure!( + line.contains("inactive"), + "vscode must be inactive without the proxy: {line}" + ); + ensure!( + line.contains("local LLM proxy not available"), + "the reason is in the detail: {line}" + ); + Ok(()) + }, + ) + .await +} diff --git a/crates/agent/src/provider/vscode/mod.rs b/crates/agent/src/provider/vscode/mod.rs index f9c08557..8e5c7d97 100644 --- a/crates/agent/src/provider/vscode/mod.rs +++ b/crates/agent/src/provider/vscode/mod.rs @@ -44,6 +44,10 @@ pub fn default_vscode_settings_path(home: &std::path::Path) -> PathBuf { #[async_trait::async_trait] impl Provider for VsCode { + fn id(&self) -> &'static str { + Self::ID + } + async fn discover(&self) -> Discovery { Discovery { agents: discovery::discover().into_iter().collect(), diff --git a/crates/agent/src/provider/vscode/reconcile.rs b/crates/agent/src/provider/vscode/reconcile.rs index bbb1f317..63193332 100644 --- a/crates/agent/src/provider/vscode/reconcile.rs +++ b/crates/agent/src/provider/vscode/reconcile.rs @@ -107,6 +107,7 @@ pub(super) fn plan( path = %path.display(), "programs.vscode is configured but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed chat language models" ); + plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); remove(path, &state_path, None, plan)?; return Ok(()); }; diff --git a/crates/agent/src/provider/vscode/settings.rs b/crates/agent/src/provider/vscode/settings.rs index 7973cbf8..da1c1361 100644 --- a/crates/agent/src/provider/vscode/settings.rs +++ b/crates/agent/src/provider/vscode/settings.rs @@ -511,6 +511,7 @@ pub(super) fn plan( path = %path.display(), "programs.vscode uses copilotChat: githubModels but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed settings" ); + plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); return remove(path, &state_path, None, plan); }; if let Some(parent) = path diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index b766326a..f307ce6f 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -7,7 +7,10 @@ use crate::provider::{ codex::Codex, copilot::Copilot, cursor::Cursor, grok::Grok, ollama::Ollama, opencode::OpenCode, vscode::VsCode, }; -use agentdesktop_core::{config::DaemonConfig, model::Discovery}; +use agentdesktop_core::{ + config::{DaemonConfig, ProgramsConfig}, + model::Discovery, +}; use serde_json::Value; use similar::TextDiff; use std::{ @@ -121,13 +124,359 @@ impl Reconciler { } pub fn apply(&self, config: &DaemonConfig) -> anyhow::Result<()> { - self.plan(config)?.apply() + self.apply_with_report(config).1 } pub fn dry_run(&self, config: &DaemonConfig) -> anyhow::Result<()> { print!("{}", self.plan(config)?.render()); Ok(()) } + + /// Plans every provider like [`Reconciler::plan`], but a provider whose + /// `plan` fails is recorded as failed and the others are still planned, + /// so each program's outcome is known. Nothing is written here. + pub fn plan_with_report(&self, config: &DaemonConfig) -> AttributedPlan { + let configured = configured_programs(&config.programs); + let mut plan = ReconcilePlan::default(); + let mut entries: Vec = Vec::new(); + let mut first_error = None; + for provider in self.providers.iter() { + let program = provider.id(); + let mut entry = ProgramEntry { + program, + configured: configured.contains(&program), + failed: None, + changes: false, + conflicts: Vec::new(), + inactive: None, + paths: Vec::new(), + operations: 0..0, + }; + match provider.plan(&self.context, config) { + Err(error) => { + entry.failed = Some(format!("{error:#}")); + first_error.get_or_insert(error); + } + Ok(own) => { + let summary = own.summary(); + entry.changes = summary.changes; + entry.conflicts = summary.conflicts; + entry.inactive = summary.inactive; + entry.paths = summary.paths; + let start = plan.operation_count(); + if let Err((error, path)) = plan.append_attributed(own) { + // Both programs of the disagreement fail. + for other in entries + .iter_mut() + .filter(|other| other.paths.contains(&path)) + { + other.failed.get_or_insert_with(|| format!("{error:#}")); + } + entry.failed = Some(format!("{error:#}")); + first_error.get_or_insert(error); + } + entry.operations = start..plan.operation_count(); + } + } + entries.push(entry); + } + AttributedPlan { + plan, + entries, + first_error, + } + } + + /// [`Reconciler::plan_with_report`] followed by [`AttributedPlan::apply`]. + pub fn apply_with_report(&self, config: &DaemonConfig) -> (ApplyReport, anyhow::Result<()>) { + self.plan_with_report(config).apply() + } +} + +/// The reason recorded for a program that uses the gateway while the local +/// LLM proxy is absent. +pub(crate) const PROXY_ABSENT_REASON: &str = + "local LLM proxy not available; see llmProxy.error in daemon-info"; + +/// A program's outcome, by precedence (highest first): Failed > Conflict > +/// Blocked > Inactive > Applied/Removed > Unchanged. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ProgramState { + /// The program is configured and its files were changed by this apply. + Applied, + /// The program is configured; nothing needed to change. + Unchanged, + /// The program is not configured; its managed content was removed by + /// this apply. + Removed, + /// A managed file held conflicting or invalid existing configuration. + Conflict, + /// The program uses the gateway, a gateway is configured, and the + /// loopback proxy is absent. + Inactive, + /// The program had changes, but none of them was written because the + /// apply was refused or stopped. + Blocked, + /// The program's plan failed, it disagreed with another program's plan, + /// one of its writes failed, or one of its files changed before the + /// apply. + Failed, +} + +impl ProgramState { + /// Lowercase name, as logged and stored. + pub fn as_str(self) -> &'static str { + match self { + Self::Applied => "applied", + Self::Unchanged => "unchanged", + Self::Removed => "removed", + Self::Conflict => "conflict", + Self::Inactive => "inactive", + Self::Blocked => "blocked", + Self::Failed => "failed", + } + } +} + +/// One program's outcome from an apply. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ProgramOutcome { + /// The provider ID, for example `"claude-code"` or `"vscode"`. + pub program: &'static str, + pub state: ProgramState, + pub detail: String, +} + +/// Every reported program's outcome from one apply, in provider +/// registration order. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct ApplyReport { + pub programs: Vec, +} + +impl ApplyReport { + /// One log line per program whose state is not applied or unchanged; a + /// program that failed, conflicted or was blocked is a warning. + pub fn log(&self) { + for outcome in &self.programs { + let warning = matches!( + outcome.state, + ProgramState::Failed | ProgramState::Conflict | ProgramState::Blocked + ); + if matches!( + outcome.state, + ProgramState::Applied | ProgramState::Unchanged + ) { + continue; + } + if warning { + tracing::warn!( + program = outcome.program, + state = outcome.state.as_str(), + detail = %outcome.detail, + "program configuration outcome" + ); + } else { + tracing::info!( + program = outcome.program, + state = outcome.state.as_str(), + detail = %outcome.detail, + "program configuration outcome" + ); + } + } + } +} + +/// The longest detail an outcome carries, in bytes. +const DETAIL_LIMIT: usize = 1024; + +fn truncate_detail(mut detail: String) -> String { + if detail.len() > DETAIL_LIMIT { + let mut end = DETAIL_LIMIT; + while !detail.is_char_boundary(end) { + end -= 1; + } + detail.truncate(end); + } + detail +} + +struct ProgramEntry { + program: &'static str, + configured: bool, + failed: Option, + changes: bool, + conflicts: Vec, + inactive: Option, + paths: Vec, + operations: std::ops::Range, +} + +impl ProgramEntry { + /// The outcome when this program's own part did not fail or conflict: + /// `written` says whether its changes reached the disk. + fn settled(&self, written: bool, blocked_by: &str) -> Option<(ProgramState, String)> { + if self.changes && !written { + return Some((ProgramState::Blocked, format!("not applied: {blocked_by}"))); + } + if let Some(reason) = &self.inactive { + return Some((ProgramState::Inactive, reason.clone())); + } + match (self.changes, self.configured) { + (true, true) => Some((ProgramState::Applied, String::new())), + (true, false) => Some((ProgramState::Removed, String::new())), + (false, true) => Some((ProgramState::Unchanged, String::new())), + (false, false) => None, + } + } +} + +/// A plan with each program's part identified, ready to apply with +/// per-program reporting. +pub struct AttributedPlan { + plan: ReconcilePlan, + entries: Vec, + first_error: Option, +} + +impl AttributedPlan { + /// Applies with the all-or-nothing rules of [`ReconcilePlan::apply`] (a + /// failed plan or any conflict means nothing is written) and returns the + /// same first error, alongside each program's outcome. + pub fn apply(self) -> (ApplyReport, anyhow::Result<()>) { + let Self { + plan, + entries, + first_error, + } = self; + let (result, stop) = if let Some(error) = first_error { + (Err(error), Stop::BeforeWrites) + } else if entries.iter().any(|entry| !entry.conflicts.is_empty()) { + // The plan refuses without writing and returns the conflict error. + (plan.apply(), Stop::BeforeWrites) + } else { + match plan.apply_tracked() { + Ok(()) => (Ok(()), Stop::Completed), + Err((error, at)) => { + let stop = Stop::Failed { + owners: owners_of(&entries, &at), + detail: format!("{error:#}"), + written: match at { + plan::ApplyStop::Operation(index) => index, + _ => 0, + }, + }; + (Err(error), stop) + } + } + }; + let failed_during = |program: &str| match &stop { + Stop::Failed { owners, .. } => owners.contains(&program), + _ => false, + }; + // The first program, in registration order, that stopped the apply. + let blocked_by = entries + .iter() + .find_map(|entry| { + if entry.failed.is_some() || failed_during(entry.program) { + Some(format!("{} failed", entry.program)) + } else if !entry.conflicts.is_empty() { + Some(format!("{} conflicted", entry.program)) + } else { + None + } + }) + .unwrap_or_default(); + let programs = entries + .iter() + .filter_map(|entry| { + let (state, detail) = if let Some(detail) = &entry.failed { + (ProgramState::Failed, detail.clone()) + } else if let Stop::Failed { detail, .. } = &stop + && failed_during(entry.program) + { + (ProgramState::Failed, detail.clone()) + } else if !entry.conflicts.is_empty() { + (ProgramState::Conflict, entry.conflicts.join("; ")) + } else { + let written = match &stop { + Stop::Completed => true, + Stop::BeforeWrites => false, + Stop::Failed { written, .. } => entry.operations.end <= *written, + }; + entry.settled(written, &blocked_by)? + }; + Some(outcome(entry.program, state, detail)) + }) + .collect(); + (ApplyReport { programs }, result) + } +} + +/// How an attributed apply ended. +enum Stop { + /// Every operation was written. + Completed, + /// Refused before any write (a failed plan or a conflict). + BeforeWrites, + /// Stopped while applying: `owners` failed, the first `written` + /// operations reached the disk. + Failed { + owners: Vec<&'static str>, + detail: String, + written: usize, + }, +} + +/// The programs responsible for where an apply stopped. +fn owners_of(entries: &[ProgramEntry], at: &plan::ApplyStop) -> Vec<&'static str> { + entries + .iter() + .filter(|entry| match at { + plan::ApplyStop::Conflict => false, + plan::ApplyStop::Observed(path) => entry.paths.contains(path), + plan::ApplyStop::PrivateDir(dir) => { + entry.paths.iter().any(|path| path.starts_with(dir)) + } + plan::ApplyStop::Operation(index) => entry.operations.contains(index), + }) + .map(|entry| entry.program) + .collect() +} + +fn outcome(program: &'static str, state: ProgramState, detail: String) -> ProgramOutcome { + ProgramOutcome { + program, + state, + detail: truncate_detail(detail), + } +} + +/// The provider IDs of the configured programs. The destructure is +/// exhaustive, so a new program key does not compile until it is mapped. +pub fn configured_programs(programs: &ProgramsConfig) -> Vec<&'static str> { + let ProgramsConfig { + claude_code, + claude_desktop, + codex, + open_code, + grok, + copilot, + vscode, + } = programs; + [ + (claude_code.is_some(), ClaudeCode::ID), + (claude_desktop.is_some(), ClaudeDesktop::ID), + (codex.is_some(), Codex::ID), + (open_code.is_some(), OpenCode::ID), + (grok.is_some(), Grok::ID), + (copilot.is_some(), Copilot::ID), + (vscode.is_some(), VsCode::ID), + ] + .into_iter() + .filter_map(|(configured, id)| configured.then_some(id)) + .collect() } #[derive(Default)] @@ -240,7 +589,7 @@ mod tests { use agentdesktop_core::config::parse_daemon; - use super::{DryRunReport, Reconciler}; + use super::{ApplyReport, DryRunReport, ProgramOutcome, ProgramState, Reconciler}; #[test] fn dry_run_report_shows_changes_and_hides_unchanged_files() { @@ -741,4 +1090,695 @@ programs: ); assert!(!fixture.root.exists()); } + + // --- Per-program configuration status ----------------------------------- + + fn new_root(name: &str) -> PathBuf { + std::env::temp_dir().join(format!( + "agentdesktop-reconcile-status-{name}-{}-{}", + std::process::id(), + rand::random::() + )) + } + + /// A user-mode reconciler with every provider's paths set (including + /// Copilot CLI and VS Code), rooted at `root`. + fn full_reconciler(root: &Path) -> Reconciler { + Reconciler::new( + true, + root.join("claude/settings.json"), + root.join("claude-desktop/settings.json"), + root.join("claude-desktop/helper"), + root.join("codex/config.toml"), + root.join("opencode/config.json"), + root.join("opencode/plugin.js"), + root.join("grok/managed_config.toml"), + Some(root.join("copilot/providers.json")), + Some(root.join("vscode/User/chatLanguageModels.json")), + Some(root.join("vscode/User/settings.json")), + root.join("bin/agentdesktop"), + root.join("agentdesktop.sock"), + ) + } + + fn proxy_context(pairing: &str) -> crate::llm_proxy::LlmProxyContext { + crate::llm_proxy::LlmProxyContext { + address: "127.0.0.1:18095".parse().unwrap(), + pairing: std::sync::Arc::from(pairing), + } + } + + fn program_outcome<'a>(report: &'a ApplyReport, program: &str) -> Option<&'a ProgramOutcome> { + report + .programs + .iter() + .find(|outcome| outcome.program == program) + } + + /// A provider whose `plan` always fails with a caller-supplied message, + /// for scenarios only the message shape matters for (detail truncation). + struct FailWithMessage { + message: String, + } + + #[async_trait::async_trait] + impl crate::provider::Provider for FailWithMessage { + fn id(&self) -> &'static str { + "fail-with-message" + } + + async fn discover(&self) -> agentdesktop_core::model::Discovery { + agentdesktop_core::model::Discovery { + agents: Vec::new(), + model_runtimes: Vec::new(), + } + } + + fn plan( + &self, + _ctx: &crate::provider::ReconcileContext, + _config: &agentdesktop_core::config::DaemonConfig, + ) -> anyhow::Result { + anyhow::bail!("{}", self.message) + } + } + + #[test] + fn applied_then_unchanged_with_report() { + let root = new_root("applied-unchanged"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + claudeCode: {} + copilot: + models: + gpt-4.1: {} +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-1"))); + + let (report, result) = reconciler.apply_with_report(&config); + result.expect("first apply succeeds"); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + + let (report, result) = reconciler.apply_with_report(&config); + result.expect("second apply succeeds"); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Unchanged) + ); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Unchanged) + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn removed_then_absent_on_the_next_apply() { + let root = new_root("removed-absent"); + let configured = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + models: + gpt-4.1: {} +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-2"))); + reconciler + .apply_with_report(&configured) + .1 + .expect("apply configured"); + + let disabled = parse_daemon("programs: {}").unwrap(); + let (report, result) = reconciler.apply_with_report(&disabled); + result.expect("apply disabled"); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Removed) + ); + + let (report, result) = reconciler.apply_with_report(&disabled); + result.expect("apply disabled again"); + assert!( + program_outcome(&report, "copilot").is_none(), + "an unconfigured, idle provider gets no row" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn use_llm_gateway_false_reports_applied_or_unchanged() { + let root = new_root("use-llm-gateway-false"); + let with_gateway = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + models: + gpt-4.1: {} +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-3"))); + reconciler + .apply_with_report(&with_gateway) + .1 + .expect("apply with the gateway on"); + + let gateway_off = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + useLlmGateway: false +"#, + ) + .unwrap(); + let (report, result) = reconciler.apply_with_report(&gateway_off); + result.expect("apply with the gateway off"); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Applied), + "the managed file is removed, so a configured program is Applied, not Removed" + ); + + let (report, result) = reconciler.apply_with_report(&gateway_off); + result.expect("apply with the gateway off again"); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Unchanged) + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn conflict_blocks_other_programs_and_reports_blocked() { + let root = new_root("conflict-blocked"); + fs::create_dir_all(root.join("codex")).unwrap(); + fs::write(root.join("codex/config.toml"), b"model = \"personal\"\n").unwrap(); + let config = parse_daemon("programs:\n claudeCode: {}\n codex: {}").unwrap(); + let reconciler = full_reconciler(&root); + + let (report, result) = reconciler.apply_with_report(&config); + assert!(result.is_err(), "a conflict means nothing is written"); + assert_eq!( + program_outcome(&report, "codex").map(|outcome| outcome.state), + Some(ProgramState::Conflict) + ); + let claude_code = program_outcome(&report, "claude-code").expect("claude-code is reported"); + assert_eq!(claude_code.state, ProgramState::Blocked); + assert!( + claude_code.detail.contains("codex"), + "a blocked program's detail names the first conflicted program: {}", + claude_code.detail + ); + assert!(!root.join("claude").exists()); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn failed_provider_plan_does_not_stop_the_others() { + let root = new_root("failed-plan-continues"); + // Grok Build's managed config lives in a system location, so a + // user-mode daemon rejects it in `plan()` (see + // `user_mode_rejects_grok_before_writing_other_settings` above) while + // Claude Code plans normally: a real, deterministic plan failure. + let reconciler = full_reconciler(&root); + let config = parse_daemon("programs:\n claudeCode: {}\n grok: {}").unwrap(); + + let (report, result) = reconciler.apply_with_report(&config); + assert!(result.is_err()); + let grok = program_outcome(&report, "grok").expect("grok is reported"); + assert_eq!(grok.state, ProgramState::Failed); + assert!(grok.detail.contains("Grok Build"), "{}", grok.detail); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Blocked), + "claude-code was still planned, but nothing is written because the apply fails" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn observed_file_mismatch_fails_its_owners_and_blocks_the_rest() { + let root = new_root("observed-mismatch"); + let reconciler = full_reconciler(&root); + let original = parse_daemon("programs:\n claudeCode: {}\n codex: {}").unwrap(); + reconciler + .apply_with_report(&original) + .1 + .expect("initial apply"); + + let update = + parse_daemon("programs:\n claudeCode:\n env:\n COMPANY: updated\n codex: {}") + .unwrap(); + let attributed = reconciler.plan_with_report(&update); + // Codex's file changes after planning but before apply. + fs::write(root.join("codex/config.toml"), b"externally changed").unwrap(); + let (report, result) = attributed.apply(); + assert!(result.is_err()); + assert_eq!( + program_outcome(&report, "codex").map(|outcome| outcome.state), + Some(ProgramState::Failed) + ); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Blocked) + ); + let _ = fs::remove_dir_all(&root); + } + + #[cfg(unix)] + #[test] + fn write_failure_orders_applied_failed_blocked() { + // SAFETY: geteuid has no preconditions and does not dereference pointers. + if unsafe { libc::geteuid() } == 0 { + return; // root ignores directory permissions. + } + let root = new_root("write-failure-order"); + fs::create_dir_all(root.join("codex")).unwrap(); + let mut reconciler = full_reconciler(&root); + reconciler.providers = std::sync::Arc::new(vec![ + Box::new(super::ClaudeCode { + settings_path: root.join("claude/settings.json"), + }), + Box::new(super::Codex { + managed_config_path: root.join("codex/config.toml"), + }), + Box::new(super::OpenCode { + managed_config_path: root.join("opencode/config.json"), + plugin_path: root.join("opencode/plugin.js"), + }), + ]); + let config = parse_daemon( + r#" +programs: + claudeCode: {} + codex: {} + openCode: + model: m + models: + m: {} +"#, + ) + .unwrap(); + let attributed = reconciler.plan_with_report(&config); + + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(root.join("codex"), fs::Permissions::from_mode(0o500)).unwrap(); + let (report, result) = attributed.apply(); + // Restore permissions so the temp directory can be removed. + fs::set_permissions(root.join("codex"), fs::Permissions::from_mode(0o700)).unwrap(); + + assert!(result.is_err()); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + assert_eq!( + program_outcome(&report, "codex").map(|outcome| outcome.state), + Some(ProgramState::Failed) + ); + assert_eq!( + program_outcome(&report, "opencode").map(|outcome| outcome.state), + Some(ProgramState::Blocked) + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn copilot_and_vscode_own_models_go_inactive_when_the_proxy_is_absent() { + let root = new_root("inactive-own-models"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + models: + gpt-4.1: {} + vscode: + models: + gpt-4.1-mini: {} +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-5"))); + let (applied, result) = reconciler.apply_with_report(&config); + result.expect("apply with the proxy available"); + assert_eq!( + program_outcome(&applied, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + assert_eq!( + program_outcome(&applied, "vscode").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + + // A restart (or a hot reload) without the proxy: from an applied + // state, both programs go Inactive rather than being torn down as an + // ordinary removal. + let reconciler_without_proxy = full_reconciler(&root); + let (report, result) = reconciler_without_proxy.apply_with_report(&config); + result.expect("apply without the proxy still succeeds: files are removed, not written"); + for program in ["copilot", "vscode"] { + let outcome = program_outcome(&report, program) + .unwrap_or_else(|| panic!("{program} is reported")); + assert_eq!(outcome.state, ProgramState::Inactive, "{program}"); + assert!( + outcome.detail.contains("local LLM proxy not available"), + "{program}: {}", + outcome.detail + ); + } + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn vscode_github_models_goes_inactive_when_the_proxy_is_absent() { + let root = new_root("inactive-github-models"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com + proxyUrl: https://gateway.example.com/copilot-proxy +programs: + vscode: + copilotChat: githubModels +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root); + let (report, result) = reconciler.apply_with_report(&config); + result.expect("apply without the proxy still succeeds"); + let outcome = program_outcome(&report, "vscode").expect("vscode is reported"); + assert_eq!(outcome.state, ProgramState::Inactive); + assert!( + outcome.detail.contains("local LLM proxy not available"), + "{}", + outcome.detail + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn vscode_conflict_on_one_file_outranks_inactive_on_the_other() { + let root = new_root("vscode-conflict-outranks-inactive"); + let github_models = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com + proxyUrl: https://gateway.example.com/copilot-proxy +programs: + vscode: + copilotChat: githubModels +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-7"))); + reconciler + .apply_with_report(&github_models) + .1 + .expect("apply githubModels with the proxy available"); + let settings_path = root.join("vscode/User/settings.json"); + assert!(settings_path.is_file(), "the override was written"); + + // Break the shape the merge left in settings.json (an object) so its + // removal below is a conflict, and switch to ownModels with no proxy + // so the other managed file (chatLanguageModels.json) is Inactive in + // the same apply: Conflict must outrank Inactive for the program. + fs::write(&settings_path, b"[]\n").unwrap(); + let own_models = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + vscode: + models: + gpt-4.1-mini: {} +"#, + ) + .unwrap(); + let reconciler = full_reconciler(&root); + let (report, result) = reconciler.apply_with_report(&own_models); + assert!(result.is_err(), "the conflict means nothing is written"); + let vscode = program_outcome(&report, "vscode").expect("vscode is reported"); + assert_eq!( + vscode.state, + ProgramState::Conflict, + "conflict outranks inactive" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn discovery_only_and_unconfigured_idle_providers_get_no_row() { + let root = new_root("no-row-for-idle"); + let reconciler = full_reconciler(&root); + let config = parse_daemon("programs:\n claudeCode: {}\n").unwrap(); + let (report, result) = reconciler.apply_with_report(&config); + result.expect("apply succeeds"); + assert_eq!( + program_outcome(&report, "claude-code").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + for absent in [ + "cursor", + "ollama", + "codex", + "claude-desktop", + "grok", + "copilot", + "vscode", + "opencode", + ] { + assert!( + program_outcome(&report, absent).is_none(), + "{absent} must get no row" + ); + } + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn program_detail_is_truncated_to_1024_bytes() { + let mut fixture = Fixture::new(); + // A multi-byte character straddles the 1024-byte cutoff so + // truncation must land on a char boundary, not merely a byte count. + let long_run = "a".repeat(1020); + let message = format!("{long_run}\u{1F600}{long_run}"); + fixture.reconciler.providers = std::sync::Arc::new(vec![Box::new(FailWithMessage { + message: message.clone(), + })]); + let config = parse_daemon("programs:\n claudeCode: {}\n").unwrap(); + + let (report, result) = fixture.reconciler.apply_with_report(&config); + assert!(result.is_err()); + assert_eq!( + report.programs.len(), + 1, + "the one failing provider is reported" + ); + let detail = &report.programs[0].detail; + assert!( + detail.len() <= 1024, + "detail must be truncated to 1024 bytes: {} bytes", + detail.len() + ); + assert!( + message.len() > 1024, + "the untruncated message must exceed the limit" + ); + } + + #[test] + fn conflict_and_sidecar_failure_details_never_include_the_pairing() { + const PAIRING: &str = "PAIRING-SECRET-13"; + + // (a) A foreign "agentdesktop" vendor entry is a conflict. + { + let root = new_root("no-pairing-conflict"); + let chat_models = root.join("vscode/User/chatLanguageModels.json"); + fs::create_dir_all(chat_models.parent().unwrap()).unwrap(); + fs::write( + &chat_models, + serde_json::to_vec_pretty(&serde_json::json!([{ + "name": "agentdesktop", + "vendor": "customendpoint", + "apiKey": "sk-user", + "apiType": "chat-completions", + "models": [{ + "id": "not-ours", + "url": "https://not-ours.example.com/v1/chat/completions", + }], + }])) + .unwrap(), + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context(PAIRING))); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + vscode: + models: + gpt-4.1-mini: {} +"#, + ) + .unwrap(); + let (report, result) = reconciler.apply_with_report(&config); + assert!(result.is_err()); + let vscode = program_outcome(&report, "vscode").expect("vscode is reported"); + assert_eq!(vscode.state, ProgramState::Conflict); + assert!( + !vscode.detail.contains(PAIRING), + "conflict detail must not leak the pairing: {}", + vscode.detail + ); + let _ = fs::remove_dir_all(&root); + } + + // (b) A corrupted sidecar file is a plan error (Failed), not a conflict. + { + let root = new_root("no-pairing-sidecar"); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context(PAIRING))); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + vscode: + models: + gpt-4.1-mini: {} +"#, + ) + .unwrap(); + reconciler + .apply_with_report(&config) + .1 + .expect("initial apply"); + let sidecar = root.join("vscode/User/.chatLanguageModels.json.agentdesktop"); + assert!(sidecar.is_file(), "the sidecar was written"); + fs::write(&sidecar, b"{ not json").unwrap(); + + let (report, result) = reconciler.apply_with_report(&config); + assert!(result.is_err()); + let vscode = program_outcome(&report, "vscode").expect("vscode is reported"); + assert_eq!(vscode.state, ProgramState::Failed); + assert!( + !vscode.detail.contains(PAIRING), + "sidecar parse failure detail must not leak the pairing: {}", + vscode.detail + ); + let _ = fs::remove_dir_all(&root); + } + } + + #[test] + fn two_programs_planning_the_same_path_both_fail_and_the_rest_is_blocked() { + let root = new_root("same-path"); + let shared = root.join("shared/settings.json"); + let mut reconciler = full_reconciler(&root); + reconciler.providers = std::sync::Arc::new(vec![ + Box::new(super::ClaudeCode { + settings_path: shared.clone(), + }), + Box::new(super::OpenCode { + managed_config_path: root.join("opencode/config.json"), + plugin_path: root.join("opencode/plugin.js"), + }), + Box::new(super::Codex { + managed_config_path: shared, + }), + ]); + let config = parse_daemon( + "programs:\n claudeCode: {}\n codex: {}\n openCode:\n model: m\n models:\n m: {}\n", + ) + .unwrap(); + let (report, result) = reconciler.apply_with_report(&config); + assert!( + result + .unwrap_err() + .to_string() + .contains("multiple providers plan to modify") + ); + let state = |program| program_outcome(&report, program).map(|outcome| outcome.state); + assert_eq!(state("claude-code"), Some(ProgramState::Failed)); + assert_eq!(state("codex"), Some(ProgramState::Failed)); + assert_eq!(state("opencode"), Some(ProgramState::Blocked)); + assert!(!root.exists(), "nothing is written"); + } + + #[test] + fn a_wrongly_typed_sidecar_field_does_not_put_its_value_in_the_detail() { + const PAIRING: &str = "PAIRING-IN-SIDECAR"; + let root = new_root("typed-sidecar"); + let providers = root.join("copilot/providers.json"); + fs::create_dir_all(providers.parent().unwrap()).unwrap(); + fs::write(&providers, b"{}\n").unwrap(); + // `created` must be a boolean; serde would quote the string. + fs::write( + root.join("copilot/.providers.json.agentdesktop"), + format!("{{\"created\": \"{PAIRING}\", \"before\": {{}}, \"after\": {{}}}}"), + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context(PAIRING))); + let config = parse_daemon( + "llmGateway:\n url: https://gateway.example.com\nprograms:\n copilot:\n models:\n gpt-4.1: {}\n", + ) + .unwrap(); + let (report, result) = reconciler.apply_with_report(&config); + assert!(result.is_err()); + let copilot = program_outcome(&report, "copilot").expect("copilot is reported"); + assert_eq!(copilot.state, ProgramState::Failed); + assert!(!copilot.detail.contains(PAIRING), "{}", copilot.detail); + let _ = fs::remove_dir_all(&root); + } + + #[cfg(unix)] + #[test] + fn a_private_directory_that_cannot_be_created_fails_its_owner_and_blocks_the_rest() { + // SAFETY: geteuid has no preconditions and does not dereference pointers. + if unsafe { libc::geteuid() } == 0 { + return; // root ignores directory permissions. + } + use std::os::unix::fs::PermissionsExt; + let root = new_root("private-dir"); + let config = parse_daemon( + "llmGateway:\n url: https://gateway.example.com\nprograms:\n claudeCode: {}\n vscode:\n models:\n gpt-4.1-mini: {}\n", + ) + .unwrap(); + let reconciler = full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-PD"))); + let attributed = reconciler.plan_with_report(&config); + // The VS Code profile directory the plan wants to create owner-only + // cannot be created any more. + fs::create_dir_all(root.join("vscode")).unwrap(); + fs::set_permissions(root.join("vscode"), fs::Permissions::from_mode(0o500)).unwrap(); + let (report, result) = attributed.apply(); + fs::set_permissions(root.join("vscode"), fs::Permissions::from_mode(0o700)).unwrap(); + assert!(result.is_err()); + let state = |program| program_outcome(&report, program).map(|outcome| outcome.state); + assert_eq!(state("vscode"), Some(ProgramState::Failed)); + assert_eq!(state("claude-code"), Some(ProgramState::Blocked)); + assert!(!root.join("claude").exists(), "nothing is written"); + let _ = fs::remove_dir_all(&root); + } } diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index fbef996a..e577dcb8 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -16,6 +16,34 @@ pub struct ReconcilePlan { operations: RefCell>, private_dirs: PrivateDirs, report: DryRunReport, + /// Why the program that owns this plan is configured but not pointed at + /// the gateway (the loopback proxy is absent); set by the provider. + inactive: RefCell>, +} + +/// What one provider's plan contains, taken before it is appended to the +/// device-wide plan, for per-program reporting. +pub(super) struct PlanSummary { + /// The plan writes or removes something that is not already so on disk, + /// or records a create/update/remove (a mode-only rewrite). + pub changes: bool, + /// One line per conflicting file. + pub conflicts: Vec, + pub inactive: Option, + /// Paths the plan read or writes. + pub paths: Vec, +} + +/// Where an apply stopped. +pub(super) enum ApplyStop { + /// A recorded conflict refused the whole plan before anything ran. + Conflict, + /// An observed file changed after planning; nothing was written. + Observed(PathBuf), + /// A private directory could not be created; nothing was written. + PrivateDir(PathBuf), + /// The operation at this index failed; the ones before it were written. + Operation(usize), } struct FileChange { @@ -45,6 +73,56 @@ impl ReconcilePlan { /// Consume a plan so it cannot accidentally be applied twice. pub fn apply(self) -> anyhow::Result<()> { + self.apply_tracked().map_err(|(error, _)| error) + } + + /// Records that the program owning this plan uses the gateway, a gateway + /// is configured, and the loopback proxy is absent. + pub fn inactive(&self, reason: &str) { + *self.inactive.borrow_mut() = Some(reason.to_owned()); + } + + pub(super) fn operation_count(&self) -> usize { + self.operations.borrow().len() + } + + pub(super) fn summary(&self) -> PlanSummary { + let observed = self.observed.borrow(); + let operations = self.operations.borrow(); + let changes = self.report.changes.borrow(); + let differs = operations.iter().any(|change| { + observed.get(&change.path).map(Option::as_deref) != Some(change.contents.as_deref()) + }); + let recorded = changes + .iter() + .any(|change| matches!(change.action.as_str(), "create" | "update" | "remove")); + let conflicts = changes + .iter() + .filter(|change| change.action == "conflict") + .map(|change| { + format!( + "{} at {}: conflicting or invalid existing configuration", + change.description, + change.path.display() + ) + }) + .collect(); + let mut paths: Vec = observed.keys().cloned().collect(); + for change in operations.iter() { + if !paths.contains(&change.path) { + paths.push(change.path.clone()); + } + } + PlanSummary { + changes: differs || recorded, + conflicts, + inactive: self.inactive.borrow().clone(), + paths, + } + } + + /// Applies like [`ReconcilePlan::apply`] and says where it stopped. + pub(super) fn apply_tracked(self) -> Result<(), (anyhow::Error, ApplyStop)> { if let Some(conflict) = self .report .changes @@ -52,56 +130,79 @@ impl ReconcilePlan { .iter() .find(|change| change.action == "conflict") { - anyhow::bail!( - "refusing to change {} {} at {}: conflicting or invalid existing configuration", - conflict.display_name, - conflict.description, - conflict.path.display() - ); + return Err(( + anyhow::anyhow!( + "refusing to change {} {} at {}: conflicting or invalid existing configuration", + conflict.display_name, + conflict.description, + conflict.path.display() + ), + ApplyStop::Conflict, + )); } for (path, expected) in self.observed.borrow().iter() { + let stop = || ApplyStop::Observed(path.clone()); let current = read_optional(path) - .with_context(|| format!("validate {} before applying plan", path.display()))?; - anyhow::ensure!( - ¤t == expected, - "{} changed since reconciliation was planned; retry reconciliation", - path.display() - ); + .with_context(|| format!("validate {} before applying plan", path.display())) + .map_err(|error| (error, stop()))?; + if ¤t != expected { + return Err(( + anyhow::anyhow!( + "{} changed since reconciliation was planned; retry reconciliation", + path.display() + ), + stop(), + )); + } } for dir in self.private_dirs.0.into_inner() { if !dir.exists() { - crate::secure_fs::ensure_private_dir(&dir)?; + crate::secure_fs::ensure_private_dir(&dir) + .map_err(|error| (error, ApplyStop::PrivateDir(dir.clone())))?; } } - for change in self.operations.into_inner() { - match change.contents { - Some(contents) => { - let parent = change - .path - .parent() - .filter(|p| !p.as_os_str().is_empty()) - .unwrap_or_else(|| Path::new(".")); - fs::create_dir_all(parent) - .with_context(|| format!("create directory {}", parent.display()))?; - crate::secure_fs::atomic_write(&change.path, &contents, change.permissions)?; + for (index, change) in self.operations.into_inner().into_iter().enumerate() { + let result = (|| -> anyhow::Result<()> { + match change.contents { + Some(contents) => { + let parent = change + .path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + fs::create_dir_all(parent) + .with_context(|| format!("create directory {}", parent.display()))?; + crate::secure_fs::atomic_write(&change.path, &contents, change.permissions) + } + None => fs::remove_file(&change.path) + .with_context(|| format!("remove {}", change.path.display())), } - None => fs::remove_file(&change.path) - .with_context(|| format!("remove {}", change.path.display()))?, - } + })(); + result.map_err(|error| (error, ApplyStop::Operation(index)))?; tracing::info!(path = %change.path.display(), "applied reconciliation change"); } Ok(()) } pub(super) fn append(&mut self, other: Self) -> anyhow::Result<()> { + self.append_attributed(other).map_err(|(error, _)| error) + } + + /// Appends like [`ReconcilePlan::append`]; on failure also returns the + /// path the two plans disagree about. + pub(super) fn append_attributed( + &mut self, + other: Self, + ) -> Result<(), (anyhow::Error, PathBuf)> { let observations = self.observed.get_mut(); for (path, expected) in other.observed.into_inner() { - if let Some(previous) = observations.get(&path) { - anyhow::ensure!( - previous == &expected, - "{} changed while planning reconciliation", - path.display() - ); + if let Some(previous) = observations.get(&path) + && previous != &expected + { + return Err(( + anyhow::anyhow!("{} changed while planning reconciliation", path.display()), + path, + )); } observations.insert(path, expected); } @@ -111,13 +212,18 @@ impl ReconcilePlan { .extend(other.private_dirs.0.into_inner()); let operations = self.operations.get_mut(); for change in other.operations.into_inner() { - anyhow::ensure!( - !operations - .iter() - .any(|existing| existing.path == change.path), - "multiple providers plan to modify {}", - change.path.display() - ); + if operations + .iter() + .any(|existing| existing.path == change.path) + { + return Err(( + anyhow::anyhow!( + "multiple providers plan to modify {}", + change.path.display() + ), + change.path, + )); + } operations.push(change); } self.report diff --git a/crates/agent/src/remote.rs b/crates/agent/src/remote.rs index d528d11a..994853f3 100644 --- a/crates/agent/src/remote.rs +++ b/crates/agent/src/remote.rs @@ -30,9 +30,9 @@ use agentdesktop_core::{ }; use agentdesktop_proto::fleet::{ AgentMessage, ConfigState, ConfigStatus, Discovery, Heartbeat, Hello, Inventory, - LlmGatewayCredentialRequest, RenewDeviceCertificateRequest, SessionNewEvent, TelemetryEvent, - ToolUseEvent, agent_message, controller_message, fleet_agent_client::FleetAgentClient, - telemetry_event, + LlmGatewayCredentialRequest, ProgramState as ProtoProgramState, ProgramStatus, + RenewDeviceCertificateRequest, SessionNewEvent, TelemetryEvent, ToolUseEvent, agent_message, + controller_message, fleet_agent_client::FleetAgentClient, telemetry_event, }; use crate::{ @@ -721,6 +721,7 @@ fn apply_daemon_config( config: agentdesktop_proto::fleet::DaemonConfig, reconciler: &Reconciler, ) -> ConfigStatus { + let mut programs = Vec::new(); let result = (|| -> anyhow::Result<()> { let actual_hash = Sha256::digest(&config.yaml); if actual_hash.as_slice() != config.sha256 { @@ -734,7 +735,18 @@ fn apply_daemon_config( let yaml = std::str::from_utf8(&config.yaml).context("configuration is not UTF-8")?; let daemon_config = config::parse_daemon(yaml)?; debug!(revision = config.revision, "parsed daemon configuration"); - reconciler.apply(&daemon_config)?; + let (report, applied) = reconciler.apply_with_report(&daemon_config); + report.log(); + programs = report + .programs + .iter() + .map(|outcome| ProgramStatus { + program: outcome.program.to_owned(), + state: program_state_proto(outcome.state).into(), + detail: outcome.detail.clone(), + }) + .collect(); + applied?; secure_fs::ensure_private_dir(state_dir)?; let path = state_dir.join("remote-config.yaml"); secure_fs::atomic_write(&path, &config.yaml, 0o600)?; @@ -750,20 +762,42 @@ fn apply_daemon_config( Ok(()) })(); + // `programs_reported` says this agent reports per-program status; the + // list is empty when the configuration was not applied at all (hash or + // parse error), and holds the apply's outcomes otherwise, also when a + // later step (persisting the configuration) failed. match result { Ok(()) => ConfigStatus { revision: config.revision, state: ConfigState::Applied.into(), error: String::new(), + programs, + programs_reported: true, }, Err(error) => ConfigStatus { revision: config.revision, state: ConfigState::Failed.into(), error: format!("{error:#}"), + programs, + programs_reported: true, }, } } +/// The proto value of a program's outcome. +fn program_state_proto(state: crate::reconcile::ProgramState) -> ProtoProgramState { + use crate::reconcile::ProgramState; + match state { + ProgramState::Applied => ProtoProgramState::Applied, + ProgramState::Unchanged => ProtoProgramState::Unchanged, + ProgramState::Removed => ProtoProgramState::Removed, + ProgramState::Conflict => ProtoProgramState::Conflict, + ProgramState::Inactive => ProtoProgramState::Inactive, + ProgramState::Blocked => ProtoProgramState::Blocked, + ProgramState::Failed => ProtoProgramState::Failed, + } +} + /// Sends the current inventory snapshot to the controller. /// /// The controller replaces a device's stored inventory on each message, so @@ -1301,4 +1335,69 @@ mod tests { ControllerConnectionState::new().get().await ); } + + // --- Per-program configuration status ----------------------------------- + + fn test_reconciler(root: &std::path::Path) -> crate::reconcile::Reconciler { + crate::reconcile::Reconciler::new( + true, + root.join("claude/settings.json"), + root.join("claude-desktop/settings.json"), + root.join("claude-desktop/helper"), + root.join("codex/config.toml"), + root.join("opencode/config.json"), + root.join("opencode/plugin.js"), + root.join("grok/managed_config.toml"), + Some(root.join("copilot/providers.json")), + None, + None, + root.join("bin/agentdesktop"), + root.join("agentdesktop.sock"), + ) + } + + #[test] + fn hash_error_reports_empty_programs_and_programs_reported_true() { + let state_dir = std::env::temp_dir().join(format!( + "agentdesktop-remote-hash-error-{}-{}", + std::process::id(), + rand::random::() + )); + let reconciler = test_reconciler(&state_dir); + let config = agentdesktop_proto::fleet::DaemonConfig { + revision: 7, + yaml: b"programs: {}\n".to_vec(), + sha256: vec![0u8; 32], + }; + + let status = super::apply_daemon_config(&state_dir, config, &reconciler); + + assert_eq!( + status.state, + agentdesktop_proto::fleet::ConfigState::Failed as i32 + ); + assert!(status.error.contains("hash")); + assert!(status.programs.is_empty()); + assert!(status.programs_reported); + let _ = std::fs::remove_dir_all(&state_dir); + } + + #[test] + fn mapping_every_program_state_to_the_proto_enum() { + use crate::reconcile::ProgramState; + use agentdesktop_proto::fleet::ProgramState as ProtoProgramState; + + let cases = [ + (ProgramState::Applied, ProtoProgramState::Applied), + (ProgramState::Unchanged, ProtoProgramState::Unchanged), + (ProgramState::Removed, ProtoProgramState::Removed), + (ProgramState::Conflict, ProtoProgramState::Conflict), + (ProgramState::Inactive, ProtoProgramState::Inactive), + (ProgramState::Blocked, ProtoProgramState::Blocked), + (ProgramState::Failed, ProtoProgramState::Failed), + ]; + for (state, expected) in cases { + assert_eq!(super::program_state_proto(state), expected, "{state:?}"); + } + } } diff --git a/crates/controller/migrations/0003_program_status.sql b/crates/controller/migrations/0003_program_status.sql new file mode 100644 index 00000000..1fd4c00a --- /dev/null +++ b/crates/controller/migrations/0003_program_status.sql @@ -0,0 +1,14 @@ +CREATE TABLE device_program_status ( + device_id TEXT NOT NULL REFERENCES devices(id) ON DELETE CASCADE, + program TEXT NOT NULL, + position BIGINT NOT NULL, + revision BIGINT NOT NULL, + state BIGINT NOT NULL, + detail TEXT NOT NULL, + updated_at BIGINT NOT NULL, + PRIMARY KEY (device_id, program) +); + +CREATE INDEX device_program_status_device_id_idx ON device_program_status(device_id); + +ALTER TABLE device_config_status ADD COLUMN programs_reported BIGINT NOT NULL DEFAULT 0; diff --git a/crates/controller/src/database.rs b/crates/controller/src/database.rs index a1deea6e..83aa63fa 100644 --- a/crates/controller/src/database.rs +++ b/crates/controller/src/database.rs @@ -3,7 +3,9 @@ use sqlx::{AnyPool, any::AnyPoolOptions}; use std::{collections::BTreeMap, path::PathBuf}; use agentdesktop_core::model::{LocalModel, McpServer, ModelRuntime, Skill}; -use agentdesktop_proto::fleet::{ConfigStatus, Hello, Inventory, TelemetryEvent, telemetry_event}; +use agentdesktop_proto::fleet::{ + ConfigStatus, Hello, Inventory, ProgramState, TelemetryEvent, telemetry_event, +}; use serde::Serialize; #[derive(Clone)] @@ -116,6 +118,82 @@ pub struct DeviceDetail { pub discoveries: Vec, pub model_runtimes: Vec, pub recent_events: Vec, + /// Per-program configuration status, in the order the agent reported it. + pub programs: Vec, + /// `None` when this device has no configuration status at all; `Some(_)` + /// once one has been reported, reflecting the reporting agent's + /// `ConfigStatus.programs_reported`. + pub programs_reported: Option, +} + +/// Bounds on what an agent reports per program, as for telemetry fields. +pub(crate) fn validate_program_status(status: &ConfigStatus) -> anyhow::Result<()> { + anyhow::ensure!( + status.programs.len() <= 32, + "too many programs in configuration status" + ); + let mut seen = std::collections::BTreeSet::new(); + for program in &status.programs { + anyhow::ensure!( + !program.program.is_empty() + && program.program.len() <= 64 + && program + .program + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-'), + "invalid program in configuration status" + ); + anyhow::ensure!( + seen.insert(program.program.as_str()), + "duplicate program in configuration status" + ); + anyhow::ensure!( + program.detail.len() <= 2048, + "program detail too long in configuration status" + ); + } + Ok(()) +} + +/// The stored name of a program state; values this controller does not know +/// are `unspecified`. +fn program_state_name(state: i64) -> &'static str { + match i32::try_from(state) + .ok() + .and_then(|state| ProgramState::try_from(state).ok()) + { + Some(ProgramState::Applied) => "applied", + Some(ProgramState::Unchanged) => "unchanged", + Some(ProgramState::Removed) => "removed", + Some(ProgramState::Conflict) => "conflict", + Some(ProgramState::Inactive) => "inactive", + Some(ProgramState::Blocked) => "blocked", + Some(ProgramState::Failed) => "failed", + Some(ProgramState::Unspecified) | None => "unspecified", + } +} + +#[derive(sqlx::FromRow)] +struct ProgramStatusRow { + program: String, + state: i64, + detail: String, + revision: i64, + updated_at: i64, +} + +/// One managed program's stored configuration status. `revision` is the +/// controller revision the report answered; it does not change across hot +/// reloads of the same revision. +#[derive(Clone, Debug, Serialize)] +pub struct ProgramStatusRecord { + pub program: String, + /// `applied`, `unchanged`, `removed`, `conflict`, `inactive`, `blocked`, + /// `failed`, or `unspecified` for a state this controller does not know. + pub state: String, + pub detail: String, + pub revision: i64, + pub updated_at: i64, } #[derive(sqlx::FromRow)] @@ -316,11 +394,38 @@ impl Database { .map(|discovery: &DeviceDiscovery| discovery.kind.clone()) .collect(); let recent_events = self.recent_telemetry(device_id, 50).await?; + let programs_reported: Option = sqlx::query_scalar( + "SELECT programs_reported FROM device_config_status WHERE device_id = $1", + ) + .bind(device_id) + .fetch_optional(&self.pool) + .await + .context("load configuration status reporting")?; + let rows: Vec = sqlx::query_as( + "SELECT program, state, detail, revision, updated_at FROM device_program_status + WHERE device_id = $1 ORDER BY position ASC", + ) + .bind(device_id) + .fetch_all(&self.pool) + .await + .context("load program configuration status")?; + let programs = rows + .into_iter() + .map(|row| ProgramStatusRecord { + program: row.program, + state: program_state_name(row.state).to_owned(), + detail: row.detail, + revision: row.revision, + updated_at: row.updated_at, + }) + .collect(); Ok(Some(DeviceDetail { device, discoveries, model_runtimes, recent_events, + programs, + programs_reported: programs_reported.map(|reported| reported != 0), })) } @@ -440,7 +545,9 @@ impl Database { Ok(()) } - pub async fn update_config_status( + /// Stores only the device-wide status (revision, state, error) and keeps + /// the program rows and `programs_reported` of the last accepted report. + pub async fn update_device_config_state( &self, device_id: &str, status: &ConfigStatus, @@ -465,6 +572,60 @@ impl Database { Ok(()) } + pub async fn update_config_status( + &self, + device_id: &str, + status: &ConfigStatus, + ) -> anyhow::Result<()> { + validate_program_status(status)?; + let now = unix_time_seconds(); + let revision = i64::try_from(status.revision).unwrap_or(i64::MAX); + let mut transaction = self.pool.begin().await?; + sqlx::query( + "INSERT INTO device_config_status + (device_id, revision, state, error, updated_at, programs_reported) + VALUES ($1, $2, $3, $4, $5, $6) + ON CONFLICT (device_id) DO UPDATE SET + revision = excluded.revision, + state = excluded.state, + error = excluded.error, + updated_at = excluded.updated_at, + programs_reported = excluded.programs_reported", + ) + .bind(device_id) + .bind(revision) + .bind(i64::from(status.state)) + .bind(&status.error) + .bind(now) + .bind(i64::from(status.programs_reported)) + .execute(&mut *transaction) + .await?; + // The report replaces the previous one: a program missing from it is + // gone from the table. + sqlx::query("DELETE FROM device_program_status WHERE device_id = $1") + .bind(device_id) + .execute(&mut *transaction) + .await?; + for (position, program) in status.programs.iter().enumerate() { + sqlx::query( + "INSERT INTO device_program_status + (device_id, program, position, revision, state, detail, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7)", + ) + .bind(device_id) + .bind(&program.program) + .bind(i64::try_from(position).unwrap_or(i64::MAX)) + .bind(revision) + .bind(i64::from(program.state)) + .bind(&program.detail) + .bind(now) + .execute(&mut *transaction) + .await?; + } + transaction.commit().await?; + Ok(()) + } + pub async fn insert_telemetry( &self, device_id: &str, @@ -570,8 +731,8 @@ mod tests { use std::{collections::BTreeMap, path::PathBuf}; use agentdesktop_proto::fleet::{ - Discovery, Inventory, LocalModel, McpServer, ModelRuntime, Skill, TelemetryEvent, - ToolUseEvent, telemetry_event, + ConfigState, ConfigStatus, Discovery, Inventory, LocalModel, McpServer, ModelRuntime, + ProgramState, ProgramStatus, Skill, TelemetryEvent, ToolUseEvent, telemetry_event, }; use super::Database; @@ -675,4 +836,267 @@ mod tests { drop(database); let _ = std::fs::remove_file(path); } + + // --- Per-program configuration status ----------------------------------- + + async fn test_database(name: &str) -> (Database, PathBuf) { + let path = std::env::temp_dir().join(format!( + "agentdesktop-program-status-{name}-{}-{}.db", + std::process::id(), + rand::random::() + )); + let database = Database::connect(&format!("sqlite://{}?mode=rwc", path.display())) + .await + .expect("connect test database"); + database + .enroll_device("device", "host", "issuer", "subject", None) + .await + .expect("enroll device"); + (database, path) + } + + fn program_status(program: &str, state: ProgramState, detail: &str) -> ProgramStatus { + ProgramStatus { + program: program.to_owned(), + state: state.into(), + detail: detail.to_owned(), + } + } + + fn config_status(programs: Vec, programs_reported: bool) -> ConfigStatus { + ConfigStatus { + revision: 1, + state: ConfigState::Applied.into(), + error: String::new(), + programs, + programs_reported, + } + } + + #[tokio::test] + async fn program_status_rows_are_stored_replaced_and_ordered_by_position() { + let (database, path) = test_database("stores-and-replaces").await; + + database + .update_config_status( + "device", + &config_status( + vec![ + program_status( + "vscode", + ProgramState::Conflict, + "conflict at settings.json", + ), + program_status("claude-code", ProgramState::Applied, ""), + ], + true, + ), + ) + .await + .expect("store the first report"); + let device = database + .get_device("device") + .await + .expect("load device") + .expect("device exists"); + assert_eq!( + device + .programs + .iter() + .map(|program| program.program.as_str()) + .collect::>(), + vec!["vscode", "claude-code"], + "rows come back ordered by position, the order they were reported in" + ); + + // A program missing from the next report is deleted, not left stale. + database + .update_config_status( + "device", + &config_status( + vec![program_status("claude-code", ProgramState::Unchanged, "")], + true, + ), + ) + .await + .expect("store the second report"); + let device = database + .get_device("device") + .await + .expect("load device") + .expect("device exists"); + assert_eq!( + device + .programs + .iter() + .map(|program| program.program.as_str()) + .collect::>(), + vec!["claude-code"], + "vscode was replaced away, not appended to" + ); + + drop(database); + let _ = std::fs::remove_file(path); + } + + #[tokio::test] + async fn programs_reported_is_none_without_a_status_and_the_stored_flag_once_one_exists() { + let (database, path) = test_database("programs-reported").await; + + let device = database + .get_device("device") + .await + .expect("load device") + .expect("device exists"); + assert_eq!( + device.programs_reported, None, + "no configuration status has been reported yet" + ); + + database + .update_config_status("device", &config_status(Vec::new(), false)) + .await + .expect("store a status without per-program reporting"); + let device = database + .get_device("device") + .await + .expect("load device") + .expect("device exists"); + assert_eq!( + device.programs_reported, + Some(false), + "an old agent's status carries no programs" + ); + + drop(database); + let _ = std::fs::remove_file(path); + } + + #[tokio::test] + async fn update_config_status_rejects_out_of_bounds_program_status() { + let (database, path) = test_database("bounds-rejected").await; + + let too_many = (0..33) + .map(|index| program_status(&format!("program-{index}"), ProgramState::Applied, "")) + .collect(); + assert!( + database + .update_config_status("device", &config_status(too_many, true)) + .await + .is_err(), + "more than 32 programs must be rejected" + ); + + assert!( + database + .update_config_status( + "device", + &config_status( + vec![program_status("Not-Valid", ProgramState::Applied, "")], + true + ), + ) + .await + .is_err(), + "a program outside [a-z0-9-] must be rejected" + ); + + assert!( + database + .update_config_status( + "device", + &config_status( + vec![program_status(&"a".repeat(65), ProgramState::Applied, "")], + true + ), + ) + .await + .is_err(), + "a program longer than 64 bytes must be rejected" + ); + + assert!( + database + .update_config_status( + "device", + &config_status( + vec![program_status( + "claude-code", + ProgramState::Applied, + &"x".repeat(2049) + )], + true + ), + ) + .await + .is_err(), + "a detail longer than 2048 bytes must be rejected" + ); + + drop(database); + let _ = std::fs::remove_file(path); + } + + #[tokio::test] + async fn unknown_program_state_round_trips_as_unspecified() { + let (database, path) = test_database("unknown-state").await; + + let mut status = program_status("claude-code", ProgramState::Applied, ""); + status.state = 99; // Not a defined ProgramState value. + database + .update_config_status("device", &config_status(vec![status], true)) + .await + .expect("store a status with a future/unknown state"); + + let device = database + .get_device("device") + .await + .expect("load device") + .expect("device exists"); + assert_eq!(device.programs[0].state, "unspecified"); + + drop(database); + let _ = std::fs::remove_file(path); + } + + #[tokio::test] + async fn program_status_rows_cascade_on_device_delete() { + let (database, path) = test_database("cascade-delete").await; + database + .update_config_status( + "device", + &config_status( + vec![program_status("claude-code", ProgramState::Applied, "")], + true, + ), + ) + .await + .expect("store a report"); + + let count_rows = || async { + sqlx::query_scalar::<_, i64>( + "SELECT COUNT(*) FROM device_program_status WHERE device_id = $1", + ) + .bind("device") + .fetch_one(&database.pool) + .await + .expect("count device_program_status rows") + }; + assert_eq!(count_rows().await, 1); + + assert!( + database + .delete_device("device") + .await + .expect("delete device") + ); + assert_eq!( + count_rows().await, + 0, + "device_program_status cascades on device delete" + ); + + drop(database); + let _ = std::fs::remove_file(path); + } } diff --git a/crates/controller/src/service.rs b/crates/controller/src/service.rs index db64a6cd..ed582093 100644 --- a/crates/controller/src/service.rs +++ b/crates/controller/src/service.rs @@ -2,9 +2,9 @@ use std::pin::Pin; use agentdesktop_proto::fleet::{ AgentMessage, BeginEnrollmentRequest, BeginEnrollmentResponse, CompleteEnrollmentRequest, - ControllerMessage, DeviceCertificateResponse, EnrollResponse, LlmGatewayCredentialRequest, - LlmGatewayCredentialResponse, RenewDeviceCertificateRequest, agent_message, controller_message, - fleet_agent_server::FleetAgent, + ConfigStatus, ControllerMessage, DeviceCertificateResponse, EnrollResponse, + LlmGatewayCredentialRequest, LlmGatewayCredentialResponse, RenewDeviceCertificateRequest, + agent_message, controller_message, fleet_agent_server::FleetAgent, }; use futures_core::Stream; use tokio::{sync::mpsc, time}; @@ -484,7 +484,7 @@ async fn handle_agent_message( ); } Some(agent_message::Message::ConfigStatus(status)) => { - database.update_config_status(device_id, &status).await?; + store_config_status(database, device_id, &status).await?; if status.error.is_empty() { info!( device_id, @@ -509,6 +509,27 @@ async fn handle_agent_message( Ok(()) } +/// Stores a device's configuration status. A per-program report the +/// controller rejects must not break the stream (the agent would reconnect, +/// receive the configuration again and send the same report), so the +/// device-wide status is stored and the last accepted program rows stay. +/// Database errors are returned as they are. +async fn store_config_status( + database: &Database, + device_id: &str, + status: &ConfigStatus, +) -> anyhow::Result<()> { + if let Err(error) = crate::database::validate_program_status(status) { + warn!( + device_id, + error = %format!("{error:#}"), + "rejected per-program configuration status; storing the device status and keeping the last accepted program status" + ); + return database.update_device_config_state(device_id, status).await; + } + database.update_config_status(device_id, status).await +} + fn bearer_credential(metadata: &tonic::metadata::MetadataMap) -> Result<&str, Status> { let value = metadata .get("authorization") @@ -658,4 +679,64 @@ mod tests { device_id ); } + + #[tokio::test] + async fn a_rejected_program_report_keeps_the_device_status() { + use agentdesktop_proto::fleet::{ConfigState, ConfigStatus, ProgramState, ProgramStatus}; + let path = std::env::temp_dir().join(format!( + "agentdesktop-rejected-report-{}-{}.db", + std::process::id(), + rand::random::() + )); + let database = Database::connect(&format!("sqlite://{}?mode=rwc", path.display())) + .await + .unwrap(); + database + .enroll_device("device", "host", "issuer", "subject", None) + .await + .unwrap(); + let accepted = ConfigStatus { + revision: 2, + state: ConfigState::Applied.into(), + error: String::new(), + programs: vec![ProgramStatus { + program: "claude-code".to_owned(), + state: ProgramState::Applied.into(), + detail: String::new(), + }], + programs_reported: true, + }; + super::store_config_status(&database, "device", &accepted) + .await + .unwrap(); + let status = ConfigStatus { + revision: 3, + state: ConfigState::Failed.into(), + error: "refusing to change VS Code settings".to_owned(), + programs: vec![ProgramStatus { + program: "Not Valid".to_owned(), + state: ProgramState::Conflict.into(), + detail: String::new(), + }], + programs_reported: true, + }; + super::store_config_status(&database, "device", &status) + .await + .expect("a rejected program report does not fail the stream"); + let device = database.get_device("device").await.unwrap().unwrap(); + assert_eq!(device.device.config_revision, Some(3)); + assert_eq!(device.device.config_state, Some(2)); + assert_eq!( + device + .programs + .iter() + .map(|program| program.program.as_str()) + .collect::>(), + vec!["claude-code"], + "the last accepted program rows stay" + ); + assert_eq!(device.programs_reported, Some(true)); + drop(database); + let _ = std::fs::remove_file(path); + } } diff --git a/crates/proto/proto/fleet.proto b/crates/proto/proto/fleet.proto index 36909325..d9b9f0f7 100644 --- a/crates/proto/proto/fleet.proto +++ b/crates/proto/proto/fleet.proto @@ -168,6 +168,14 @@ message ConfigStatus { uint64 revision = 1; ConfigState state = 2; string error = 3; + // Per-program outcomes of this apply, in the agent's provider order. Empty + // when the configuration was not applied at all (hash or parse error), and + // from agents that do not report per-program status: `programs_reported` + // distinguishes the two. + repeated ProgramStatus programs = 4; + // Whether this agent version reports per-program status at all. An older + // agent never sets this field, so it reads back as `false`. + bool programs_reported = 5; } enum ConfigState { @@ -175,3 +183,24 @@ enum ConfigState { CONFIG_STATE_APPLIED = 1; CONFIG_STATE_FAILED = 2; } + +// One managed program's outcome from an apply. `program` is the provider ID +// (for example `claude-code`, `vscode`); `detail` is at most 1024 bytes and +// carries paths, fixed reasons and error chains of the kind sent in +// `ConfigStatus.error`, which do not quote managed file content. +message ProgramStatus { + string program = 1; + ProgramState state = 2; + string detail = 3; +} + +enum ProgramState { + PROGRAM_STATE_UNSPECIFIED = 0; + PROGRAM_STATE_APPLIED = 1; + PROGRAM_STATE_UNCHANGED = 2; + PROGRAM_STATE_REMOVED = 3; + PROGRAM_STATE_CONFLICT = 4; + PROGRAM_STATE_INACTIVE = 5; + PROGRAM_STATE_BLOCKED = 6; + PROGRAM_STATE_FAILED = 7; +} diff --git a/frontend/controller/src/stories/fixtures.ts b/frontend/controller/src/stories/fixtures.ts index 6cc1dce6..bcac53d6 100644 --- a/frontend/controller/src/stories/fixtures.ts +++ b/frontend/controller/src/stories/fixtures.ts @@ -208,6 +208,30 @@ const deviceSkills = [ export const deviceDetail: DeviceDetail = { ...macDevice, + programs: [ + { + program: "claude-code", + state: "applied", + detail: "", + revision: 12, + updated_at: now - 60, + }, + { + program: "vscode", + state: "unchanged", + detail: "", + revision: 12, + updated_at: now - 60, + }, + { + program: "copilot", + state: "removed", + detail: "", + revision: 12, + updated_at: now - 60, + }, + ], + programs_reported: true, discoveries: [ { kind: "vscode", @@ -254,6 +278,51 @@ export const deviceDetail: DeviceDetail = { export const failedDeviceDetail: DeviceDetail = { ...deviceDetail, ...failedDevice, + programs: [ + { + program: "claude-code", + state: "blocked", + detail: "not applied: vscode conflicted", + revision: 12, + updated_at: now - 60, + }, + { + program: "copilot", + state: "inactive", + detail: + "local LLM proxy not available; see llmProxy.error in daemon-info", + revision: 12, + updated_at: now - 60, + }, + { + program: "vscode", + state: "conflict", + detail: + "settings at /Users/developer/Library/Application Support/Code/User/settings.json: conflicting or invalid existing configuration", + revision: 12, + updated_at: now - 60, + }, + { + program: "codex", + state: "failed", + detail: "multiple providers plan to modify /etc/codex/config.toml", + revision: 12, + updated_at: now - 60, + }, + { + program: "grok", + state: "unspecified", + detail: "", + revision: 12, + updated_at: now - 60, + }, + ], +}; + +export const oldAgentDeviceDetail: DeviceDetail = { + ...deviceDetail, + programs: [], + programs_reported: false, }; export const emptyDeviceDetail: DeviceDetail = { @@ -261,6 +330,8 @@ export const emptyDeviceDetail: DeviceDetail = { discoveries: [], model_runtimes: [], recent_events: [], + programs: [], + programs_reported: true, }; export const controllerSettings: ControllerSettings = { diff --git a/frontend/controller/src/styles.css b/frontend/controller/src/styles.css index 50b5824b..abd2d664 100644 --- a/frontend/controller/src/styles.css +++ b/frontend/controller/src/styles.css @@ -487,6 +487,23 @@ code { color: var(--muted); } +.badge.warning { + color: var(--warning-strong); + border-color: var(--warning-border); +} + +.program-state { + min-width: 0; + display: flex; + align-items: center; + gap: 8px; +} + +.program-state code { + min-width: 0; + overflow-wrap: anywhere; +} + .config-summary { padding-bottom: 14px; } @@ -1230,6 +1247,11 @@ code { text-align: center; } +.empty-state.compact { + min-height: 0; + padding: 16px; +} + .empty-state h3, .empty-state h2 { margin: 10px 0 5px; diff --git a/frontend/controller/src/types.ts b/frontend/controller/src/types.ts index e2ee8906..60e103a1 100644 --- a/frontend/controller/src/types.ts +++ b/frontend/controller/src/types.ts @@ -16,7 +16,25 @@ export type Device = { installed_tools: string[]; }; +export type ProgramStatus = { + program: string; + state: + | "applied" + | "unchanged" + | "removed" + | "conflict" + | "inactive" + | "blocked" + | "failed" + | "unspecified"; + detail: string; + revision: number; + updated_at: number; +}; + export type DeviceDetail = Device & { + programs: ProgramStatus[]; + programs_reported: boolean | null; discoveries: Array<{ kind: string; version: string; diff --git a/frontend/controller/src/views/DeviceView.stories.tsx b/frontend/controller/src/views/DeviceView.stories.tsx index 404a0063..899e75f6 100644 --- a/frontend/controller/src/views/DeviceView.stories.tsx +++ b/frontend/controller/src/views/DeviceView.stories.tsx @@ -6,6 +6,7 @@ import { deviceDetail, emptyDeviceDetail, failedDeviceDetail, + oldAgentDeviceDetail, } from "../stories/fixtures"; import { DeviceView } from "./DeviceView"; @@ -64,6 +65,25 @@ export const BrowsesDiscoveredCapabilities: Story = { export const ConfigurationFailed: Story = { args: { device: failedDeviceDetail }, + play: async ({ canvas }) => { + await expect(canvas.getByText("Managed programs")).toBeVisible(); + await expect(canvas.getByText("Conflict")).toBeVisible(); + await expect(canvas.getByText("Blocked")).toBeVisible(); + await expect( + canvas.getByText("not applied: vscode conflicted"), + ).toBeVisible(); + }, +}; + +export const OlderAgentWithoutProgramStatus: Story = { + args: { device: oldAgentDeviceDetail }, + play: async ({ canvas }) => { + await expect( + canvas.getByText( + "Per-program status is not reported by this agent version", + ), + ).toBeVisible(); + }, }; export const NoActivityOrTools: Story = { diff --git a/frontend/controller/src/views/DeviceView.tsx b/frontend/controller/src/views/DeviceView.tsx index eccbc1f0..f8b1e0a3 100644 --- a/frontend/controller/src/views/DeviceView.tsx +++ b/frontend/controller/src/views/DeviceView.tsx @@ -20,7 +20,7 @@ import { friendlyOs, } from "../format"; import { Link } from "../router"; -import type { DeviceDetail } from "../types"; +import type { DeviceDetail, ProgramStatus } from "../types"; export interface DeviceViewProps { deleteError: string | null; @@ -105,6 +105,44 @@ export function DeviceView({ )} + {device.programs_reported !== null && ( +
+ + {!device.programs_reported ? ( +

+ Per-program status is not reported by this agent version +

+ ) : device.programs.length ? ( +
+ {device.programs.map((program) => ( +
+ + + {friendlyTool(program.program)} + + + + {program.detail && ( + {program.detail} + )} + + +
+ ))} +
+ ) : ( +

No managed programs

+ )} +
+ )}
180 ? `${encoded.slice(0, 177)}…` : encoded; } + +const programStateLabels: Record = { + applied: "Applied", + unchanged: "Unchanged", + removed: "Removed", + conflict: "Conflict", + inactive: "Inactive", + blocked: "Blocked", + failed: "Failed", + unspecified: "Unknown", +}; + +function ProgramBadge({ state }: { state: ProgramStatus["state"] }) { + const tone = + state === "conflict" || state === "failed" || state === "blocked" + ? "danger" + : state === "inactive" + ? "warning" + : "neutral"; + return ( + + {programStateLabels[state] ?? state} + + ); +} From ee0b2b05c71da4b85318c124f33e33383d6421e5 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Wed, 30 Sep 2026 18:42:45 +0200 Subject: [PATCH 2/5] feat(agent): opt-in reconcile tick; skip identical writes `daemon.reconcileInterval` (local startup setting, off when unset; zero or more than 30 days is rejected) re-applies the current configuration, which repairs drift between pushes: a deleted or hand-edited managed file, a fixed conflict, a loosened file mode. - The current configuration is the startup configuration, replaced by each pushed configuration that parses, and cleared on logout (also between controller sessions and while waiting to retry). - Applies are serialized: a push, the startup apply and a tick never overlap. - A tick reports to the controller only when its outcome differs from what was last sent on the stream; a pushed configuration whose apply failed is saved once a tick applies it. Behaviour change for every program: an apply no longer rewrites a file that already holds the planned bytes with no mode bit beyond the planned mode, so unchanged files and sidecars are no longer rewritten on each apply. Co-Authored-By: Claude Opus 5.5 --- README.md | 45 +- crates/agent/src/daemon.rs | 58 ++- crates/agent/src/lib.rs | 1 + .../agent/src/provider/copilot/reconcile.rs | 2 +- crates/agent/src/provider/json_merge.rs | 59 ++- .../src/provider/vscode/integration_tests.rs | 44 ++ crates/agent/src/provider/vscode/reconcile.rs | 2 +- crates/agent/src/provider/vscode/settings.rs | 2 +- crates/agent/src/reconcile/mod.rs | 439 +++++++++++++++- crates/agent/src/reconcile/plan.rs | 151 ++++++ crates/agent/src/remote.rs | 470 ++++++++++++++++-- crates/agent/src/tick.rs | 258 ++++++++++ crates/core/src/config.rs | 68 +++ schema/daemon-config.json | 8 + schema/daemon-config.md | 1 + 15 files changed, 1541 insertions(+), 67 deletions(-) create mode 100644 crates/agent/src/tick.rs diff --git a/README.md b/README.md index e596aac9..dea1dcc2 100644 --- a/README.md +++ b/README.md @@ -468,7 +468,8 @@ carries the pairing) and removal keeps the mode it finds; the sidecar is itself saves the file with its own default mode (664 with a new inode when a setting is changed in the UI), so the URL is readable to the group and others until the daemon's next apply -tightens it again (a config push or a daemon restart); an unchanged file +tightens it again (a config push, a daemon restart, a reconnect to the +controller, or the reconcile tick when `daemon.reconcileInterval` is set); an unchanged file looser than 0600 is tightened on the next apply without being rewritten otherwise. The override key is the daemon's while the program is active: a hand edit of the URL is replaced on the next apply and is not kept as the @@ -499,7 +500,7 @@ single-quoted string; the top level is not an object; `settingsSync.ignoredSettings` is not an array; or a managed key appears twice) leaves the file untouched and blocks every managed file on the device until it is resolved (and stops the daemon at startup); fix the file in VS Code and -re-apply. Removal (program +re-apply (with `daemon.reconcileInterval` set, the next tick applies it). Removal (program absent, `ownModels`, `useLlmGateway: false`, no gateway, no `proxyUrl`, or no proxy, with a warning) takes the key and the two entries out and deletes the file only if the daemon created it. Restart VS Code after an apply that changes @@ -575,6 +576,39 @@ routes: - x-llm-token ``` +## Periodic re-apply (opt-in) + +The daemon applies its configuration at startup and whenever the controller +pushes one; a controller-managed device also re-applies the controller's +configuration on every reconnect, which happens at least once per OIDC +access-token lifetime. To repair drift between those points, set an interval +in the local configuration: + +```yaml +daemon: + reconcileInterval: 5m +``` + +Every interval the daemon re-applies the current configuration (the local +file, or the last configuration the controller pushed). A managed file that +was deleted or edited by hand comes back, a file whose mode was loosened is +tightened, and a conflict that was fixed is applied. Nothing is written when +nothing changed, and a tick reports to the controller and logs its outcome lines only when the outcome changed; a provider warning that explains a conflict (for example the file it refuses to change) repeats with every apply until the conflict is fixed. Deleting a managed file is then no longer a way to opt out: +switch the program off in the configuration or stop the daemon instead. The +tick cannot start a local LLM proxy that was not running when the daemon +started (restart the daemon for that). Unset means no periodic re-apply; zero +and more than 30 days are rejected. The interval is read at startup only. After a +logout from the controller the tick stops re-applying (the managed files stay +until the next configuration). A controller configuration whose apply failed +is saved as the one to restore once a tick applies it; if the device loses the +connection before that and restarts offline, it restores the previous saved +configuration until the controller pushes again. + +Any apply (tick or not) leaves a managed file alone when it already holds the +planned bytes, and rewrites it with the same bytes when its mode grants more +than the daemon writes: for example a Claude Code user `settings.json` at +0664 becomes 0644, a Copilot CLI `providers.json` at 0644 becomes 0600. + ## Start locally, grow into a fleet Agentdesktop uses the same daemon and tool-native configuration model at every @@ -608,9 +642,10 @@ When several apply, the first in the list `failed`, `conflict`, `blocked`, nothing across programs: one conflict or failure means no file is written for any program, which is what `blocked` makes visible. Programs that are not configured and have nothing to clean up, and discovery-only tools, are not -listed. After each apply the daemon logs one `program configuration outcome` -line (`program`, `state`, `detail`) per program that is not `applied` or -`unchanged`. A daemon restart reports again, because the controller re-sends +listed. After each push and startup apply the daemon logs one `program +configuration outcome` line (`program`, `state`, `detail`) per program that is +not `applied` or `unchanged`; a reconcile tick logs and reports only when the +outcome changed. A daemon restart reports again, because the controller re-sends the configuration on every connection; the files are already written by the startup apply of the saved configuration at that point, so the report after a restart usually says `unchanged` (the startup apply's outcome lines for programs diff --git a/crates/agent/src/daemon.rs b/crates/agent/src/daemon.rs index a5d2f049..bd01c190 100644 --- a/crates/agent/src/daemon.rs +++ b/crates/agent/src/daemon.rs @@ -87,6 +87,8 @@ struct ResolvedDaemonArgs { vscode_chat_models: Option, /// VS Code's user `settings.json`; `None` in system mode (user-only program). vscode_settings: Option, + /// `daemon.reconcileInterval`; `None` means no periodic re-apply. + reconcile_interval: Option, once: bool, dry_run: bool, } @@ -208,6 +210,7 @@ impl DaemonArgs { }, vscode_chat_models: None, vscode_settings: None, + reconcile_interval: startup.reconcile_interval, once: self.once || self.dry_run, dry_run: self.dry_run, }); @@ -294,6 +297,7 @@ impl DaemonArgs { .settings .unwrap_or_else(|| reconcile::default_vscode_settings_path(&home)), ), + reconcile_interval: startup.reconcile_interval, once: self.once || self.dry_run, dry_run: self.dry_run, }) @@ -479,22 +483,43 @@ where } else { Some(local_config) }; - if let Some(initial_config) = initial_config { + if let Some(initial_config) = &initial_config { // The outcomes are logged before a failure stops the daemon. - let (report, applied) = reconciler.apply_with_report(&initial_config); + let (report, applied) = reconciler.apply_with_report(initial_config); report.log(); applied.context("apply initial daemon configuration")?; // With a controller, the cached controller configuration is what is in effect // from here, and start_gateway_authentication above only saw the local file. // Without one the two are the same configuration and it already ran. if config.controller.is_some() { - gateway_oidc::start_device_login_if_configured(&initial_config, &args.state_dir); + gateway_oidc::start_device_login_if_configured(initial_config, &args.state_dir); } } else { tracing::info!( "preserving managed files until the controller provides daemon configuration" ); } + // The current configuration, re-applied by the reconcile tick; held here + // for the daemon's lifetime and replaced by each pushed configuration. + let (current_config, current_config_receiver) = + watch::channel(initial_config.map(|config| crate::tick::CurrentConfig { + revision: None, + config: Arc::new(config), + })); + let (tick_status_sender, tick_status_receiver) = watch::channel(None); + let tick_enabled = args.reconcile_interval.is_some(); + if let Some(interval) = args.reconcile_interval { + tracing::info!( + interval_seconds = interval.as_secs_f64(), + "reconcile tick enabled" + ); + tokio::spawn(crate::tick::run_tick( + reconciler.clone(), + current_config_receiver, + interval, + config.controller.is_some().then_some(tick_status_sender), + )); + } let discovery = reconciler.discover().await; log_discovery(&discovery); let (inventory_sender, inventory) = watch::channel(Arc::new(discovery)); @@ -508,6 +533,7 @@ where let (logout_sender, logout_receiver) = mpsc::channel(1); let logout = config.controller.as_ref().map(|_| logout_sender); if let Some(controller) = config.controller.clone() { + let remote_current_config = current_config.clone(); let remote_discovery = inventory.clone(); let state_dir = args.state_dir.clone(); let oidc_callback_listen = args.oidc_callback_listen; @@ -523,6 +549,8 @@ where remote::Requests { telemetry: telemetry_receiver, logout: logout_receiver, + current: remote_current_config, + tick_statuses: tick_enabled.then_some(tick_status_receiver), }, )); } @@ -1783,6 +1811,30 @@ programs: ); } + #[test] + fn the_reconcile_tick_is_off_unless_an_interval_is_set() { + let resolved = daemon_args(true) + .resolve( + config::DaemonStartupConfig::default(), + PathBuf::from("config.yaml"), + PathBuf::from("agentdesktop.sock"), + ) + .unwrap(); + assert_eq!(resolved.reconcile_interval, None); + let startup = config::DaemonStartupConfig { + reconcile_interval: Some(Duration::from_secs(120)), + ..Default::default() + }; + let resolved = daemon_args(true) + .resolve( + startup, + PathBuf::from("config.yaml"), + PathBuf::from("agentdesktop.sock"), + ) + .unwrap(); + assert_eq!(resolved.reconcile_interval, Some(Duration::from_secs(120))); + } + #[test] fn system_mode_rejects_a_vscode_settings_path() { let mut startup = config::DaemonStartupConfig::default(); diff --git a/crates/agent/src/lib.rs b/crates/agent/src/lib.rs index 9b16f3dd..b961d56a 100644 --- a/crates/agent/src/lib.rs +++ b/crates/agent/src/lib.rs @@ -14,6 +14,7 @@ pub mod remote; pub mod secret_store; pub mod secure_fs; pub mod subscription; +mod tick; #[cfg(windows)] mod windows_security; diff --git a/crates/agent/src/provider/copilot/reconcile.rs b/crates/agent/src/provider/copilot/reconcile.rs index ffb2442c..e42ed01e 100644 --- a/crates/agent/src/provider/copilot/reconcile.rs +++ b/crates/agent/src/provider/copilot/reconcile.rs @@ -108,7 +108,7 @@ pub(super) fn plan( return Ok(()); }; let Some((listen, pairing)) = proxy else { - warn!( + tracing::debug!( path = %path.display(), "programs.copilot is configured but the local LLM proxy is not available, so the Copilot CLI is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed Copilot CLI providers" ); diff --git a/crates/agent/src/provider/json_merge.rs b/crates/agent/src/provider/json_merge.rs index 9c8e7faa..60f2ea6e 100644 --- a/crates/agent/src/provider/json_merge.rs +++ b/crates/agent/src/provider/json_merge.rs @@ -170,8 +170,12 @@ pub(super) fn plan_merge_with( let mut contents = serde_json::to_vec_pretty(&combined) .with_context(|| format!("serialize merged {display_name}"))?; contents.push(b'\n'); + // An unchanged file that grants more than `options.mode` is rewritten + // with the same bytes at that mode (the plan skips identical writes + // otherwise). + let looser = current_mode(path).is_some_and(|mode| mode & !options.mode & 0o777 != 0); let action = match existing.as_deref() { - Some(existing) if existing == contents => "unchanged", + Some(existing) if existing == contents && !looser => "unchanged", Some(_) => "update", None => "create", }; @@ -884,4 +888,57 @@ mod tests { assert!(plan.apply().is_err()); assert_eq!(std::fs::read(&path).unwrap(), b"42\n"); } + + // --- No-op writes skipped -------------------------------------------------- + + #[test] + #[cfg(unix)] + fn a_looser_mode_with_identical_bytes_is_recorded_as_update_and_tightened() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("chatLanguageModels.json"); + let state = state_path(&path); + let options = root_array_options(); // mode 0600 + + let managed = json!([{"name": "agentdesktop"}]); + let plan = ReconcilePlan::default(); + plan_merge_with( + &path, + &state, + managed.clone(), + false, + "chat models", + "Test", + options, + &plan, + ) + .unwrap(); + plan.apply().unwrap(); + + // VS Code (or another program) saves the file at a looser mode. + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o664)).unwrap(); + + let plan = ReconcilePlan::default(); + plan_merge_with( + &path, + &state, + managed, + false, + "chat models", + "Test", + options, + &plan, + ) + .unwrap(); + assert!( + plan.render().contains("UPDATE"), + "a file looser than the planned mode must be treated as an update, \ + not unchanged: {}", + plan.render() + ); + plan.apply().unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "the loosened file must be tightened"); + } } diff --git a/crates/agent/src/provider/vscode/integration_tests.rs b/crates/agent/src/provider/vscode/integration_tests.rs index 7eaada09..86fad1e7 100644 --- a/crates/agent/src/provider/vscode/integration_tests.rs +++ b/crates/agent/src/provider/vscode/integration_tests.rs @@ -453,3 +453,47 @@ async fn program_status_reports_inactive_without_the_proxy() -> anyhow::Result<( ) .await } + +/// Its own port: the Docker tests share the host network and run in parallel. +const TICK_LISTEN: &str = "127.0.0.1:18098"; + +#[tokio::test] +async fn reconcile_tick_recreates_a_deleted_managed_file() -> anyhow::Result<()> { + Container::run("vscode", "crates/agent/src/provider/vscode/testdata/Dockerfile", async |container| { + let gateway = Gateway::start().await?; + let document = serde_json::json!({ + "daemon": { "user": true, "llmProxy": { "listen": TICK_LISTEN }, "reconcileInterval": "2s" }, + "llmGateway": { "url": gateway.url }, + "programs": { "vscode": { "models": { "gpt-4.1-mini": {} } } }, + }); + container.write(CONFIG, &serde_json::to_string_pretty(&document)?).await?; + container.exec(&["chown", "tester:tester", CONFIG]).await?; + container.exec(&["chown", "-R", "tester:tester", "/home/tester"]).await?; + let daemon = ["runuser", "-u", "tester", "--", "agentdesktop", "daemon", "--config", CONFIG]; + container.start_process("daemon", &daemon).await?; + container.wait_ready(&["agentdesktop", "--socket", SOCKET, "status"]).await?; + container.exec(&["test", "-e", CHAT_MODELS]).await?; + + info!("Deleting the managed file and waiting for the tick"); + container.exec(&["rm", CHAT_MODELS]).await?; + let mut back = false; + for _ in 0..40 { + if container.exec(&["test", "-e", CHAT_MODELS]).await.is_ok() { + back = true; + break; + } + tokio::time::sleep(Duration::from_millis(250)).await; + } + ensure!(back, "the reconcile tick did not recreate {CHAT_MODELS} within 10 s"); + let mode = container.exec(&["stat", "-c", "%a", CHAT_MODELS]).await?; + ensure!(mode.trim() == "600", "recreated file must be owner-only, got {mode}"); + + info!("Checking that later ticks write nothing"); + let before = container.exec(&["stat", "-c", "%i %Y", CHAT_MODELS, CHAT_SIDECAR]).await?; + tokio::time::sleep(Duration::from_secs(5)).await; + let after = container.exec(&["stat", "-c", "%i %Y", CHAT_MODELS, CHAT_SIDECAR]).await?; + ensure!(before == after, "an unchanged tick rewrote a file:\n{before}\n{after}"); + Ok(()) + }) + .await +} diff --git a/crates/agent/src/provider/vscode/reconcile.rs b/crates/agent/src/provider/vscode/reconcile.rs index 63193332..4272daf5 100644 --- a/crates/agent/src/provider/vscode/reconcile.rs +++ b/crates/agent/src/provider/vscode/reconcile.rs @@ -103,7 +103,7 @@ pub(super) fn plan( return Ok(()); }; let Some((listen, pairing)) = proxy else { - warn!( + tracing::debug!( path = %path.display(), "programs.vscode is configured but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed chat language models" ); diff --git a/crates/agent/src/provider/vscode/settings.rs b/crates/agent/src/provider/vscode/settings.rs index da1c1361..a2ca7c5e 100644 --- a/crates/agent/src/provider/vscode/settings.rs +++ b/crates/agent/src/provider/vscode/settings.rs @@ -507,7 +507,7 @@ pub(super) fn plan( return remove(path, &state_path, proxy, plan); } let Some((listen, pairing)) = proxy else { - warn!( + tracing::debug!( path = %path.display(), "programs.vscode uses copilotChat: githubModels but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed settings" ); diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index f307ce6f..101f39d7 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -37,6 +37,12 @@ pub use crate::provider::{ pub struct Reconciler { context: ReconcileContext, providers: Arc>>, + /// The apply lock and the report of the last apply. Held for planning + /// and applying, so a push, the startup apply and a reconcile tick never + /// overlap across clones; never held across an `.await`. `plan`, + /// `plan_with_report` and `dry_run` take no lock. The report feeds the + /// tick's log rule. + last_apply: Arc>>, } impl Reconciler { @@ -91,6 +97,7 @@ impl Reconciler { }), Box::new(Ollama), ]), + last_apply: Arc::new(std::sync::Mutex::new(None)), } } @@ -187,16 +194,48 @@ impl Reconciler { } } - /// [`Reconciler::plan_with_report`] followed by [`AttributedPlan::apply`]. + /// [`Reconciler::plan_with_report`] followed by [`AttributedPlan::apply`], + /// under the apply lock. pub fn apply_with_report(&self, config: &DaemonConfig) -> (ApplyReport, anyhow::Result<()>) { - self.plan_with_report(config).apply() + let (_, report, result) = self.apply_after_previous(config); + (report, result) + } + + /// Like [`Reconciler::apply_with_report`], also returning the report of + /// the apply before this one (from any source), read under the same lock. + pub(crate) fn apply_after_previous( + &self, + config: &DaemonConfig, + ) -> (Option, ApplyReport, anyhow::Result<()>) { + let (previous, report, result) = self + .apply_read_under_lock(|| Some(Arc::new(config.clone()))) + .expect("a configuration was given"); + (previous, report, result) + } + + /// Takes the apply lock, then reads the configuration to apply, so a + /// configuration pushed while this call waited for the lock is the one + /// applied (a tick never re-applies an older revision after a push). + /// `None` from `read` applies nothing. + #[allow(clippy::type_complexity)] + pub(crate) fn apply_read_under_lock( + &self, + read: impl FnOnce() -> Option>, + ) -> Option<(Option, ApplyReport, anyhow::Result<()>)> { + let mut last = self + .last_apply + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let config = read()?; + let (report, result) = self.plan_with_report(&config).apply(); + let previous = last.replace(report.clone()); + Some((previous, report, result)) } } /// The reason recorded for a program that uses the gateway while the local /// LLM proxy is absent. -pub(crate) const PROXY_ABSENT_REASON: &str = - "local LLM proxy not available; see llmProxy.error in daemon-info"; +pub(crate) const PROXY_ABSENT_REASON: &str = "local LLM proxy not available; see llmProxy.error in daemon-info, or set daemon.llmProxy.listen"; /// A program's outcome, by precedence (highest first): Failed > Conflict > /// Blocked > Inactive > Applied/Removed > Unchanged. @@ -256,12 +295,19 @@ pub struct ApplyReport { impl ApplyReport { /// One log line per program whose state is not applied or unchanged; a - /// program that failed, conflicted or was blocked is a warning. + /// program that failed, conflicted, was blocked or is inactive is a + /// warning. pub fn log(&self) { for outcome in &self.programs { + // Inactive is a warning too: the program is configured for the + // gateway but its files are removed. The providers log the cause + // at debug level, since they plan on every apply. let warning = matches!( outcome.state, - ProgramState::Failed | ProgramState::Conflict | ProgramState::Blocked + ProgramState::Failed + | ProgramState::Conflict + | ProgramState::Blocked + | ProgramState::Inactive ); if matches!( outcome.state, @@ -288,6 +334,53 @@ impl ApplyReport { } } +/// Whether `current` (a tick's apply outcome) differs enough from +/// `previous` (the report of the last apply from any source: startup, a +/// push, or an earlier tick) to log, using the same same-kind-of-difference +/// rule as `remote::tick_config_status`: a program moving from `Applied` or +/// `Removed` to `Unchanged` is not itself a difference. `previous: None` +/// (nothing has been applied yet) always logs. +pub(crate) fn should_log(previous: Option<&ApplyReport>, current: &ApplyReport) -> bool { + let Some(previous) = previous else { + return true; + }; + outcomes_differ( + &outcome_keys(&previous.programs), + &outcome_keys(¤t.programs), + ) +} + +/// `(program, state, detail)` of each outcome, for [`outcomes_differ`]. +pub(crate) fn outcome_keys(programs: &[ProgramOutcome]) -> Vec<(&str, ProgramState, &str)> { + programs + .iter() + .map(|outcome| (outcome.program, outcome.state, outcome.detail.as_str())) + .collect() +} + +/// Whether two outcome lists differ by program, state or detail. A program +/// that settles from applied or removed to unchanged, or a removed program +/// that is no longer listed, is not a difference: the files are as reported. +pub(crate) fn outcomes_differ( + previous: &[(&str, ProgramState, &str)], + current: &[(&str, ProgramState, &str)], +) -> bool { + let changed = current.iter().any(|&(program, state, detail)| { + match previous.iter().find(|(before, _, _)| *before == program) { + None => true, + Some(&(_, before_state, before_detail)) => { + let settled = state == ProgramState::Unchanged + && matches!(before_state, ProgramState::Applied | ProgramState::Removed); + !settled && (state != before_state || detail != before_detail) + } + } + }); + let gone = previous.iter().any(|&(program, state, _)| { + state != ProgramState::Removed && !current.iter().any(|(now, _, _)| *now == program) + }); + changed || gone +} + /// The longest detail an outcome carries, in bytes. const DETAIL_LIMIT: usize = 1024; @@ -1781,4 +1874,338 @@ programs: assert!(!root.join("claude").exists(), "nothing is written"); let _ = fs::remove_dir_all(&root); } + + // --- No-op writes skipped, full reconciler --------------------------------- + + fn sidecar_path(path: &Path) -> PathBuf { + let name = path.file_name().unwrap().to_str().unwrap(); + path.with_file_name(format!(".{name}.agentdesktop")) + } + + #[test] + fn second_apply_of_an_unchanged_multi_program_config_writes_nothing() { + let root = new_root("second-apply-no-op"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + claudeCode: {} + copilot: + models: + gpt-4.1: {} + vscode: + models: + gpt-4.1-mini: {} +"#, + ) + .unwrap(); + let reconciler = + full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-TICK-1"))); + reconciler + .apply_with_report(&config) + .1 + .expect("first apply succeeds"); + + let paths = [ + root.join("claude/settings.json"), + sidecar_path(&root.join("claude/settings.json")), + root.join("copilot/providers.json"), + sidecar_path(&root.join("copilot/providers.json")), + root.join("vscode/User/chatLanguageModels.json"), + sidecar_path(&root.join("vscode/User/chatLanguageModels.json")), + ]; + let before: Vec<_> = paths + .iter() + .map(|path| { + fs::metadata(path).unwrap_or_else(|error| { + panic!( + "{} must exist after the first apply: {error}", + path.display() + ) + }) + }) + .collect(); + + reconciler + .apply_with_report(&config) + .1 + .expect("second apply succeeds"); + + for (path, before) in paths.iter().zip(before) { + let after = fs::metadata(path).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!( + after.ino(), + before.ino(), + "{} must not be rewritten by an unchanged apply", + path.display() + ); + } + assert_eq!( + after.modified().unwrap(), + before.modified().unwrap(), + "{} mtime must not change on an unchanged apply", + path.display() + ); + } + let _ = fs::remove_dir_all(&root); + } + + #[test] + #[cfg(unix)] + fn providers_json_at_a_looser_mode_is_tightened_on_the_next_apply() { + use std::os::unix::fs::PermissionsExt; + let root = new_root("copilot-tighten"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + models: + gpt-4.1: {} +"#, + ) + .unwrap(); + let reconciler = + full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-TICK-2"))); + reconciler + .apply_with_report(&config) + .1 + .expect("first apply succeeds"); + + let providers_path = root.join("copilot/providers.json"); + fs::set_permissions(&providers_path, fs::Permissions::from_mode(0o664)).unwrap(); + let before_contents = fs::read(&providers_path).unwrap(); + + reconciler + .apply_with_report(&config) + .1 + .expect("second apply succeeds"); + + assert_eq!( + fs::read(&providers_path).unwrap(), + before_contents, + "bytes must not change" + ); + let mode = fs::metadata(&providers_path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "a looser providers.json must be tightened"); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_deleted_managed_file_is_recreated_on_the_next_apply() { + let root = new_root("recreate-deleted"); + let config = parse_daemon( + r#" +llmGateway: + url: https://gateway.example.com +programs: + copilot: + models: + gpt-4.1: {} +"#, + ) + .unwrap(); + let reconciler = + full_reconciler(&root).with_llm_proxy(Some(proxy_context("PAIRING-TICK-3"))); + reconciler + .apply_with_report(&config) + .1 + .expect("first apply succeeds"); + + let providers_path = root.join("copilot/providers.json"); + let original = fs::read(&providers_path).unwrap(); + fs::remove_file(&providers_path).unwrap(); + + let (report, result) = reconciler.apply_with_report(&config); + result.expect("second apply recreates the deleted file"); + assert_eq!( + program_outcome(&report, "copilot").map(|outcome| outcome.state), + Some(ProgramState::Applied) + ); + assert_eq!(fs::read(&providers_path).unwrap(), original); + let _ = fs::remove_dir_all(&root); + } + + // --- Serialized applies ---------------------------------------------------- + + /// A provider whose `plan` records that it was entered (in order) and, + /// only the first time, blocks until `release` is signalled. Later calls + /// return immediately, so the test can tell whether a second + /// `apply_with_report` was let into `plan` before the first released the + /// lock. + struct BlockingProbe { + order: std::sync::Arc>>, + release: std::sync::Arc<(std::sync::Mutex, std::sync::Condvar)>, + } + + #[async_trait::async_trait] + impl crate::provider::Provider for BlockingProbe { + fn id(&self) -> &'static str { + "blocking-probe" + } + + async fn discover(&self) -> agentdesktop_core::model::Discovery { + agentdesktop_core::model::Discovery { + agents: Vec::new(), + model_runtimes: Vec::new(), + } + } + + fn plan( + &self, + _ctx: &crate::provider::ReconcileContext, + _config: &agentdesktop_core::config::DaemonConfig, + ) -> anyhow::Result { + let is_first = { + let mut order = self.order.lock().unwrap(); + order.push("enter"); + order.len() == 1 + }; + if is_first { + let (lock, condvar) = &*self.release; + let mut released = lock.lock().unwrap(); + while !*released { + released = condvar.wait(released).unwrap(); + } + } + Ok(super::ReconcilePlan::default()) + } + } + + #[test] + fn a_second_apply_on_a_clone_does_not_enter_plan_until_the_first_releases_the_lock() { + let order = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let release = + std::sync::Arc::new((std::sync::Mutex::new(false), std::sync::Condvar::new())); + let mut reconciler = full_reconciler(&new_root("apply-lock")); + reconciler.providers = std::sync::Arc::new(vec![Box::new(BlockingProbe { + order: order.clone(), + release: release.clone(), + })]); + let config = parse_daemon("programs: {}").unwrap(); + + let first = reconciler.clone(); + let first_config = config.clone(); + let first_handle = std::thread::spawn(move || first.apply_with_report(&first_config)); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while order.lock().unwrap().is_empty() { + assert!( + std::time::Instant::now() < deadline, + "the first apply never entered plan" + ); + std::thread::sleep(std::time::Duration::from_millis(5)); + } + + let second = reconciler.clone(); + let second_config = config.clone(); + let second_handle = std::thread::spawn(move || second.apply_with_report(&second_config)); + + // Give a wrongly-unserialized second call time to enter `plan`. + std::thread::sleep(std::time::Duration::from_millis(200)); + assert_eq!( + order.lock().unwrap().len(), + 1, + "a second apply_with_report on a clone must not enter plan while \ + the first holds the lock" + ); + + { + let (lock, condvar) = &*release; + *lock.lock().unwrap() = true; + condvar.notify_all(); + } + + let (_, first_result) = first_handle.join().unwrap(); + let (_, second_result) = second_handle.join().unwrap(); + assert!(first_result.is_ok(), "{first_result:?}"); + assert!(second_result.is_ok(), "{second_result:?}"); + assert_eq!( + order.lock().unwrap().len(), + 2, + "the second apply must eventually run once the lock is released" + ); + } + + // --- Tick logging dedup ---------------------------------------------------- + + fn outcome_report(state: ProgramState, detail: &str) -> ApplyReport { + ApplyReport { + programs: vec![ProgramOutcome { + program: "claude-code", + state, + detail: detail.to_owned(), + }], + } + } + + #[test] + fn should_log_with_nothing_applied_yet_always_logs() { + let current = outcome_report(ProgramState::Applied, ""); + assert!(super::should_log(None, ¤t)); + } + + #[test] + fn should_log_is_false_when_the_report_is_unchanged() { + let previous = outcome_report(ProgramState::Unchanged, ""); + let current = outcome_report(ProgramState::Unchanged, ""); + assert!(!super::should_log(Some(&previous), ¤t)); + } + + #[test] + fn should_log_is_false_when_applied_or_removed_settles_to_unchanged() { + for state in [ProgramState::Applied, ProgramState::Removed] { + let previous = outcome_report(state, ""); + let current = outcome_report(ProgramState::Unchanged, ""); + assert!( + !super::should_log(Some(&previous), ¤t), + "{state:?} -> Unchanged must not be logged" + ); + } + } + + #[test] + fn should_log_is_true_when_a_programs_detail_changes() { + let previous = outcome_report(ProgramState::Conflict, "conflict at a"); + let current = outcome_report(ProgramState::Conflict, "conflict at b"); + assert!(super::should_log(Some(&previous), ¤t)); + } + + #[test] + fn should_log_is_true_when_a_programs_state_changes_to_something_other_than_unchanged() { + let previous = outcome_report(ProgramState::Unchanged, ""); + let current = outcome_report(ProgramState::Failed, "boom"); + assert!(super::should_log(Some(&previous), ¤t)); + } + + #[test] + fn a_program_that_disappears_is_a_difference_unless_it_was_removed() { + let both = ApplyReport { + programs: vec![ + ProgramOutcome { + program: "claude-code", + state: ProgramState::Unchanged, + detail: String::new(), + }, + ProgramOutcome { + program: "copilot", + state: ProgramState::Unchanged, + detail: String::new(), + }, + ], + }; + let one = outcome_report(ProgramState::Unchanged, ""); + assert!(super::should_log(Some(&both), &one), "copilot vanished"); + let mut removed = both.clone(); + removed.programs[1].state = ProgramState::Removed; + assert!( + !super::should_log(Some(&removed), &one), + "a removed program that is no longer listed is not news" + ); + } } diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index e577dcb8..cf968fbd 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -161,7 +161,13 @@ impl ReconcilePlan { .map_err(|error| (error, ApplyStop::PrivateDir(dir.clone())))?; } } + let observed = self.observed.into_inner(); 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) + { + continue; + } let result = (|| -> anyhow::Result<()> { match change.contents { Some(contents) => { @@ -314,3 +320,148 @@ fn read_optional(path: &Path) -> io::Result>> { Err(error) => Err(error), } } + +// --- No-op writes skipped ---------------------------------------------------- + +/// Whether a planned write would change nothing: the file already holds the +/// planned bytes (as observed when planning, which the apply has just +/// verified) and, on Unix, grants no permission bit outside the planned +/// mode. A file looser than planned is rewritten; a stricter one is left. +fn already_in_place( + path: &Path, + contents: &[u8], + permissions: u32, + observed: &BTreeMap>>, +) -> bool { + if observed.get(path).and_then(Option::as_deref) != Some(contents) { + return false; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + match fs::metadata(path) { + Ok(metadata) => metadata.permissions().mode() & !permissions & 0o777 == 0, + Err(_) => false, + } + } + #[cfg(not(unix))] + { + let _ = permissions; + true + } +} + +#[cfg(test)] +mod tests { + use super::ReconcilePlan; + use std::fs; + + #[test] + fn identical_write_is_skipped_same_inode_and_mtime() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file.txt"); + + let plan = ReconcilePlan::default(); + plan.write_file(&path, b"hello\n", 0o644).unwrap(); + plan.apply().unwrap(); + let before = fs::metadata(&path).unwrap(); + let before_mtime = before.modified().unwrap(); + #[cfg(unix)] + let before_ino = { + use std::os::unix::fs::MetadataExt; + before.ino() + }; + + // A second, identical plan must not touch the file at all. + let plan = ReconcilePlan::default(); + plan.write_file(&path, b"hello\n", 0o644).unwrap(); + plan.apply().unwrap(); + + let after = fs::metadata(&path).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!( + after.ino(), + before_ino, + "an identical write must not replace the file" + ); + } + assert_eq!( + after.modified().unwrap(), + before_mtime, + "an identical write must not touch the file's mtime" + ); + assert_eq!(fs::read(&path).unwrap(), b"hello\n"); + } + + #[test] + #[cfg(unix)] + fn same_bytes_at_a_looser_mode_are_rewritten_at_the_planned_mode() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file.txt"); + fs::write(&path, b"hello\n").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o664)).unwrap(); + + let plan = ReconcilePlan::default(); + plan.write_file(&path, b"hello\n", 0o600).unwrap(); + plan.apply().unwrap(); + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "a file looser than planned must be tightened even with identical bytes" + ); + assert_eq!(fs::read(&path).unwrap(), b"hello\n"); + } + + #[test] + #[cfg(unix)] + fn same_bytes_at_a_stricter_mode_are_left() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file.txt"); + fs::write(&path, b"hello\n").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o600)).unwrap(); + let before_ino = fs::metadata(&path).unwrap().ino(); + + let plan = ReconcilePlan::default(); + plan.write_file(&path, b"hello\n", 0o644).unwrap(); + plan.apply().unwrap(); + + let after = fs::metadata(&path).unwrap(); + assert_eq!( + after.ino(), + before_ino, + "a file stricter than planned must not be rewritten" + ); + assert_eq!( + after.permissions().mode() & 0o777, + 0o600, + "a stricter mode must not be loosened to match the plan" + ); + } + + #[test] + fn removal_whose_file_vanished_before_apply_fails_the_observed_check_naming_the_path() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file.txt"); + fs::write(&path, b"hello\n").unwrap(); + + let plan = ReconcilePlan::default(); + plan.remove_file(&path).unwrap(); + fs::remove_file(&path).unwrap(); + + let error = plan.apply().unwrap_err(); + let message = error.to_string(); + assert!( + message.contains(&path.display().to_string()), + "error must name the path: {message}" + ); + assert!( + message.contains("changed since reconciliation was planned"), + "unexpected error: {message}" + ); + } +} diff --git a/crates/agent/src/remote.rs b/crates/agent/src/remote.rs index 994853f3..4b41a456 100644 --- a/crates/agent/src/remote.rs +++ b/crates/agent/src/remote.rs @@ -42,6 +42,7 @@ use crate::{ oidc, reconcile::Reconciler, secure_fs, + tick::{CurrentConfig, TickStatus}, }; static OAUTH_REFRESH_MUTEX: Mutex<()> = Mutex::const_new(()); @@ -59,6 +60,17 @@ pub struct LogoutRequest { pub struct Requests { pub telemetry: mpsc::Receiver, pub logout: mpsc::Receiver, + /// The daemon's current configuration, replaced by each pushed + /// configuration that parses (read by the reconcile tick). + pub(crate) current: watch::Sender>, + /// Reconcile tick outcomes, when the tick is enabled. + pub(crate) tick_statuses: Option>>, +} + +/// The configuration channels shared with the reconcile tick. +struct ConfigChannels { + current: watch::Sender>, + tick_statuses: Option>>, } /// Tracks whether the daemon's connection to the controller is currently @@ -152,7 +164,13 @@ pub async fn run( let Requests { mut telemetry, mut logout, + current, + tick_statuses, } = requests; + let mut channels = ConfigChannels { + current, + tick_statuses, + }; let mut delay = INITIAL_RETRY_DELAY; loop { let started = time::Instant::now(); @@ -166,6 +184,7 @@ pub async fn run( &controller_status, &mut telemetry, &mut logout, + &mut channels, ) .await; // A session that stayed up for a while failed for a new reason; do not @@ -191,6 +210,7 @@ pub async fn run( &state_dir.join("identity.json"), &enrollment, &controller_status, + &channels.current, ) .await; delay = INITIAL_RETRY_DELAY; @@ -204,6 +224,7 @@ async fn logout_between_sessions( identity_path: &Path, enrollment: &EnrollmentState, controller_status: &ControllerConnectionState, + current: &watch::Sender>, ) { match identity::load(identity_path) { Ok(Some(identity)) => { @@ -213,6 +234,7 @@ async fn logout_between_sessions( &identity, enrollment, controller_status, + current, ) .await; } @@ -259,6 +281,7 @@ async fn run_session( controller_status: &ControllerConnectionState, telemetry: &mut mpsc::Receiver, logout: &mut mpsc::Receiver, + channels: &mut ConfigChannels, ) -> anyhow::Result { let identity_path = state_dir.join("identity.json"); loop { @@ -302,7 +325,7 @@ async fn run_session( let refresh_result = tokio::select! { result = refresh_oauth_if_needed(&mut identity, &identity_path) => result, Some(request) = logout.recv() => { - if complete_logout(request, &identity_path, &identity, enrollment, controller_status).await { + if complete_logout(request, &identity_path, &identity, enrollment, controller_status, &channels.current).await { break; } continue; @@ -342,6 +365,7 @@ async fn run_session( &identity, enrollment, controller_status, + &channels.current, ) .await { @@ -376,9 +400,10 @@ async fn run_session( reconciler, telemetry, controller_status, + channels, ) => Some(result), Some(request) = logout.recv() => { - if complete_logout(request, &identity_path, &identity, enrollment, controller_status).await { + if complete_logout(request, &identity_path, &identity, enrollment, controller_status, &channels.current).await { None } else { continue; @@ -429,6 +454,7 @@ async fn run_session( &identity, enrollment, controller_status, + &channels.current, ) .await { @@ -448,11 +474,12 @@ async fn wait_before_retry( identity: &Identity, enrollment: &EnrollmentState, controller_status: &ControllerConnectionState, + current: &watch::Sender>, ) -> bool { tokio::select! { _ = time::sleep(delay) => false, Some(request) = logout.recv() => { - complete_logout(request, identity_path, identity, enrollment, controller_status).await + complete_logout(request, identity_path, identity, enrollment, controller_status, current).await } } } @@ -514,10 +541,14 @@ async fn complete_logout( identity: &Identity, enrollment: &EnrollmentState, controller_status: &ControllerConnectionState, + current: &watch::Sender>, ) -> bool { let result = identity::delete(identity_path, &identity.device_id) .map_err(|error| format!("remove local organization identity: {error:#}")); if result.is_ok() { + // The organization's configuration is no longer enforced by the + // reconcile tick; the managed files stay until the next configuration. + current.send_replace(None); enrollment.set("starting").await; controller_status.reset().await; info!(device_id = %identity.device_id, "logged out local organization session"); @@ -594,6 +625,7 @@ pub async fn llm_gateway_credential( }) } +#[allow(clippy::too_many_arguments)] async fn connect( controller: &ControllerConnectionConfig, identity: &Identity, @@ -602,8 +634,19 @@ async fn connect( reconciler: &Reconciler, telemetry: &mut mpsc::Receiver, controller_status: &ControllerConnectionState, + channels: &mut ConfigChannels, ) -> anyhow::Result<()> { let mut client = client(controller, Some(identity)).await?; + // Tick outcomes produced before this stream are not sent on it; the + // controller's configuration push on connect reports anyway. + if let Some(statuses) = channels.tick_statuses.as_mut() { + statuses.borrow_and_update(); + } + // The revision of the last configuration received on this stream and the + // last status sent on it, for deciding whether a tick outcome is news. + let mut stream_revision = None; + let mut last_sent: Option = None; + let mut pending_persist: Option<(u64, Vec)> = None; let (sender, receiver) = mpsc::channel(16); let mut request = Request::new(ReceiverStream::new(receiver)); authenticate_request(identity, &mut request)?; @@ -658,6 +701,27 @@ async fn connect( snapshot = next_inventory(discovered) => { send_inventory(&sender, &snapshot).await?; } + tick = next_tick_status(&mut channels.tick_statuses) => { + let mut tick = tick; + if tick.error.is_none() + && tick.revision.is_some() + && tick.revision == stream_revision + && let Some((revision, yaml)) = pending_persist.take() + { + // A failed save stays visible: the tick is reported as + // failed and the save is retried on the next tick. + if let Err(error) = persist_config(state_dir, &yaml, revision) { + warn!(revision, error = %format!("{error:#}"), "saving the configuration after a reconcile tick failed; retrying on the next tick"); + tick.error = Some(format!("{error:#}")); + pending_persist = Some((revision, yaml)); + } + } + if let Some(status) = tick_config_status(last_sent.as_ref(), stream_revision, &tick) { + info!(revision = status.revision, "reporting a changed reconcile outcome"); + last_sent = Some(status.clone()); + send(&sender, agent_message::Message::ConfigStatus(status)).await?; + } + } message = inbound.message() => { let Some(message) = message.context("read controller stream")? else { return Ok(()); @@ -669,7 +733,16 @@ async fn connect( bytes = config.yaml.len(), "received daemon configuration" ); - let status = apply_daemon_config(state_dir, config, reconciler); + stream_revision = Some(config.revision); + let parsed = replace_current_config(&channels.current, &config); + let parsed_ok = parsed.is_ok(); + let status = apply_parsed(state_dir, &config, parsed, reconciler); + // A parsed configuration whose apply failed is the current + // one but not yet saved; a later tick that applies it + // saves it, so a restart does not go back to the previous one. + pending_persist = (parsed_ok && !status.error.is_empty()) + .then(|| (config.revision, config.yaml.clone())); + last_sent = Some(status.clone()); if status.error.is_empty() { info!(revision = status.revision, "applied daemon configuration"); } else { @@ -716,71 +789,88 @@ fn telemetry_to_proto(event: ModelTelemetryEvent) -> TelemetryEvent { } } +/// Parses and applies a pushed configuration in one step (the connection +/// loop parses first to update the current configuration). +#[cfg(test)] fn apply_daemon_config( state_dir: &Path, config: agentdesktop_proto::fleet::DaemonConfig, reconciler: &Reconciler, +) -> ConfigStatus { + let parsed = parse_pushed_config(&config); + apply_parsed(state_dir, &config, parsed, reconciler) +} + +/// Applies a pushed configuration already checked by [`parse_pushed_config`] +/// and persists it once its apply succeeded. +fn apply_parsed( + state_dir: &Path, + config: &agentdesktop_proto::fleet::DaemonConfig, + parsed: anyhow::Result, + reconciler: &Reconciler, ) -> ConfigStatus { let mut programs = Vec::new(); let result = (|| -> anyhow::Result<()> { - let actual_hash = Sha256::digest(&config.yaml); - if actual_hash.as_slice() != config.sha256 { - bail!("configuration hash does not match payload"); - } - debug!( - revision = config.revision, - "verified daemon configuration hash" - ); - - let yaml = std::str::from_utf8(&config.yaml).context("configuration is not UTF-8")?; - let daemon_config = config::parse_daemon(yaml)?; + let daemon_config = parsed?; debug!(revision = config.revision, "parsed daemon configuration"); let (report, applied) = reconciler.apply_with_report(&daemon_config); report.log(); - programs = report - .programs - .iter() - .map(|outcome| ProgramStatus { - program: outcome.program.to_owned(), - state: program_state_proto(outcome.state).into(), - detail: outcome.detail.clone(), - }) - .collect(); + programs = program_statuses(&report); applied?; - secure_fs::ensure_private_dir(state_dir)?; - let path = state_dir.join("remote-config.yaml"); - secure_fs::atomic_write(&path, &config.yaml, 0o600)?; + persist_config(state_dir, &config.yaml, config.revision)?; // Managed gateway policy that asks for device authorization has to start its // sign-in here: the startup path only sees the local file. Idempotent, so a // repeated or unchanged update returns the pending grant instead of a new one. gateway_oidc::start_device_login_if_configured(&daemon_config, state_dir); - info!( - revision = config.revision, - path = %path.display(), - "persisted daemon configuration" - ); Ok(()) })(); - // `programs_reported` says this agent reports per-program status; the // list is empty when the configuration was not applied at all (hash or // parse error), and holds the apply's outcomes otherwise, also when a // later step (persisting the configuration) failed. - match result { - Ok(()) => ConfigStatus { - revision: config.revision, - state: ConfigState::Applied.into(), - error: String::new(), - programs, - programs_reported: true, - }, - Err(error) => ConfigStatus { - revision: config.revision, - state: ConfigState::Failed.into(), - error: format!("{error:#}"), - programs, - programs_reported: true, - }, + config_status( + config.revision, + programs, + result.map_err(|error| format!("{error:#}")), + ) +} + +/// Saves the controller's configuration as the one to restore at startup. +fn persist_config(state_dir: &Path, yaml: &[u8], revision: u64) -> anyhow::Result<()> { + secure_fs::ensure_private_dir(state_dir)?; + let path = state_dir.join("remote-config.yaml"); + secure_fs::atomic_write(&path, yaml, 0o600)?; + info!(revision, path = %path.display(), "persisted daemon configuration"); + Ok(()) +} + +fn program_statuses(report: &crate::reconcile::ApplyReport) -> Vec { + report + .programs + .iter() + .map(|outcome| ProgramStatus { + program: outcome.program.to_owned(), + state: program_state_proto(outcome.state).into(), + detail: outcome.detail.clone(), + }) + .collect() +} + +fn config_status( + revision: u64, + programs: Vec, + result: Result<(), String>, +) -> ConfigStatus { + let (state, error) = match result { + Ok(()) => (ConfigState::Applied, String::new()), + Err(error) => (ConfigState::Failed, error), + }; + ConfigStatus { + revision, + state: state.into(), + error, + programs, + programs_reported: true, } } @@ -798,6 +888,63 @@ fn program_state_proto(state: crate::reconcile::ProgramState) -> ProtoProgramSta } } +/// The `ConfigStatus` a tick's outcome should send on this stream, if any. +/// A status is sent only when the tick's revision is `Some` and +/// equals the revision of the last `DaemonConfig` received on this stream, +/// and the device state, the error, or any program's `(program, state, +/// detail)` differs from `last_sent` (the last `ConfigStatus` sent on this +/// stream, whether by a push or an earlier tick), where a program going from +/// `applied` or `removed` to `unchanged` is not a difference. A failed tick +/// is always reported (when its revision matches) as `FAILED` with the +/// error and `programs_reported: true`. +pub(crate) fn tick_config_status( + last_sent: Option<&ConfigStatus>, + stream_revision: Option, + tick: &TickStatus, +) -> Option { + if tick.revision.is_none() || tick.revision != stream_revision { + return None; + } + let status = config_status( + tick.revision.unwrap_or_default(), + program_statuses(&tick.report), + tick.error.clone().map_or(Ok(()), Err), + ); + let Some(last_sent) = last_sent else { + return Some(status); + }; + let differs = last_sent.state != status.state + || last_sent.error != status.error + || crate::reconcile::outcomes_differ( + &status_keys(&last_sent.programs), + &status_keys(&status.programs), + ); + differs.then_some(status) +} + +/// `(program, state, detail)` of each reported program, for +/// [`crate::reconcile::outcomes_differ`]. +fn status_keys(programs: &[ProgramStatus]) -> Vec<(&str, crate::reconcile::ProgramState, &str)> { + use crate::reconcile::ProgramState; + programs + .iter() + .map(|program| { + let state = match ProtoProgramState::try_from(program.state) { + Ok(ProtoProgramState::Applied) => ProgramState::Applied, + Ok(ProtoProgramState::Unchanged) => ProgramState::Unchanged, + Ok(ProtoProgramState::Removed) => ProgramState::Removed, + Ok(ProtoProgramState::Conflict) => ProgramState::Conflict, + Ok(ProtoProgramState::Inactive) => ProgramState::Inactive, + Ok(ProtoProgramState::Blocked) => ProgramState::Blocked, + Ok(ProtoProgramState::Failed | ProtoProgramState::Unspecified) | Err(_) => { + ProgramState::Failed + } + }; + (program.program.as_str(), state, program.detail.as_str()) + }) + .collect() +} + /// Sends the current inventory snapshot to the controller. /// /// The controller replaces a device's stored inventory on each message, so @@ -865,6 +1012,52 @@ async fn send_inventory( Ok(()) } +/// The next reconcile tick outcome; never resolves when the tick is off or +/// has stopped. +async fn next_tick_status( + statuses: &mut Option>>, +) -> TickStatus { + if let Some(statuses) = statuses.as_mut() { + while statuses.changed().await.is_ok() { + if let Some(tick) = statuses.borrow_and_update().clone() { + return tick; + } + } + } + std::future::pending().await +} + +/// Makes a pushed configuration the current one for the reconcile tick if it +/// parses, whether or not its apply then succeeds; a push that does not +/// parse leaves the current configuration as it is. +fn replace_current_config( + current: &watch::Sender>, + config: &agentdesktop_proto::fleet::DaemonConfig, +) -> anyhow::Result { + let parsed = parse_pushed_config(config)?; + current.send_replace(Some(CurrentConfig { + revision: Some(config.revision), + config: Arc::new(parsed.clone()), + })); + Ok(parsed) +} + +/// Hash, UTF-8 and parse checks of a pushed configuration. +fn parse_pushed_config( + config: &agentdesktop_proto::fleet::DaemonConfig, +) -> anyhow::Result { + let actual_hash = Sha256::digest(&config.yaml); + if actual_hash.as_slice() != config.sha256 { + bail!("configuration hash does not match payload"); + } + debug!( + revision = config.revision, + "verified daemon configuration hash" + ); + let yaml = std::str::from_utf8(&config.yaml).context("configuration is not UTF-8")?; + config::parse_daemon(yaml) +} + /// Resolves with the next inventory snapshot, and never resolves once the /// refresher has stopped, so the controller stream keeps running without it. async fn next_inventory( @@ -1335,7 +1528,6 @@ mod tests { ControllerConnectionState::new().get().await ); } - // --- Per-program configuration status ----------------------------------- fn test_reconciler(root: &std::path::Path) -> crate::reconcile::Reconciler { @@ -1400,4 +1592,184 @@ mod tests { assert_eq!(super::program_state_proto(state), expected, "{state:?}"); } } + + // --- Tick reporting dedup -------------------------------------------------- + + fn program_report( + state: crate::reconcile::ProgramState, + detail: &str, + ) -> crate::reconcile::ApplyReport { + crate::reconcile::ApplyReport { + programs: vec![crate::reconcile::ProgramOutcome { + program: "claude-code", + state, + detail: detail.to_owned(), + }], + } + } + + fn sent_status(report: &crate::reconcile::ApplyReport, revision: u64) -> super::ConfigStatus { + use super::{ConfigState, ConfigStatus, ProgramStatus}; + ConfigStatus { + revision, + state: ConfigState::Applied.into(), + error: String::new(), + programs: report + .programs + .iter() + .map(|outcome| ProgramStatus { + program: outcome.program.to_owned(), + state: super::program_state_proto(outcome.state).into(), + detail: outcome.detail.clone(), + }) + .collect(), + programs_reported: true, + } + } + + fn tick_status( + revision: Option, + report: crate::reconcile::ApplyReport, + error: Option<&str>, + ) -> crate::tick::TickStatus { + use crate::tick::TickStatus; + TickStatus { + revision, + report, + error: error.map(str::to_owned), + } + } + + #[test] + fn tick_config_status_is_none_when_the_revision_is_none_or_mismatched() { + use crate::reconcile::ProgramState; + let previous = program_report(ProgramState::Unchanged, ""); + let last_sent = sent_status(&previous, 3); + let current = program_report(ProgramState::Failed, "boom"); + + assert!( + super::tick_config_status( + Some(&last_sent), + Some(3), + &tick_status(None, current.clone(), None) + ) + .is_none(), + "a tick with no revision is never reported" + ); + assert!( + super::tick_config_status( + Some(&last_sent), + Some(3), + &tick_status(Some(2), current, None) + ) + .is_none(), + "a tick whose revision does not match the stream's last push is never reported" + ); + } + + #[test] + fn tick_config_status_is_none_when_nothing_differs() { + use crate::reconcile::ProgramState; + let report = program_report(ProgramState::Unchanged, ""); + let last_sent = sent_status(&report, 4); + assert!( + super::tick_config_status( + Some(&last_sent), + Some(4), + &tick_status(Some(4), report, None) + ) + .is_none() + ); + } + + #[test] + fn tick_config_status_is_none_when_applied_or_removed_settles_to_unchanged() { + use crate::reconcile::ProgramState; + for state in [ProgramState::Applied, ProgramState::Removed] { + let previous = program_report(state, ""); + let last_sent = sent_status(&previous, 5); + let current = program_report(ProgramState::Unchanged, ""); + assert!( + super::tick_config_status( + Some(&last_sent), + Some(5), + &tick_status(Some(5), current, None) + ) + .is_none(), + "{state:?} -> Unchanged must not be reported" + ); + } + } + + #[test] + fn tick_config_status_is_some_when_a_programs_detail_changes() { + use crate::reconcile::ProgramState; + let previous = program_report(ProgramState::Conflict, "conflict at a"); + let last_sent = sent_status(&previous, 6); + let current = program_report(ProgramState::Conflict, "conflict at b"); + + let status = super::tick_config_status( + Some(&last_sent), + Some(6), + &tick_status(Some(6), current, None), + ) + .expect("a changed detail must be reported"); + assert_eq!(status.revision, 6); + assert_eq!(status.state, super::ConfigState::Applied as i32); + assert!(status.programs_reported); + } + + #[test] + fn tick_config_status_is_some_with_no_baseline_on_this_stream() { + use crate::reconcile::ProgramState; + let current = program_report(ProgramState::Applied, ""); + let status = super::tick_config_status(None, Some(1), &tick_status(Some(1), current, None)) + .expect( + "nothing sent yet on this stream must still be reported once the revision matches", + ); + assert_eq!(status.revision, 1); + } + + #[test] + fn tick_config_status_reports_a_failed_tick_as_failed_with_programs_reported() { + use crate::reconcile::ProgramState; + let previous = program_report(ProgramState::Unchanged, ""); + let last_sent = sent_status(&previous, 7); + let current = program_report(ProgramState::Unchanged, ""); + + let status = super::tick_config_status( + Some(&last_sent), + Some(7), + &tick_status(Some(7), current, Some("disk full")), + ) + .expect("a failed tick must be reported when its revision matches"); + assert_eq!(status.revision, 7); + assert_eq!(status.state, super::ConfigState::Failed as i32); + assert_eq!(status.error, "disk full"); + assert!(status.programs_reported); + } + + #[test] + fn only_a_push_that_parses_replaces_the_current_configuration() { + use sha2::{Digest, Sha256}; + let pushed = |revision: u64, yaml: &str| agentdesktop_proto::fleet::DaemonConfig { + revision, + yaml: yaml.as_bytes().to_vec(), + sha256: Sha256::digest(yaml.as_bytes()).to_vec(), + }; + let (current, receiver) = tokio::sync::watch::channel(None); + let _ = super::replace_current_config(¤t, &pushed(1, "programs: {}\n")); + assert_eq!(receiver.borrow().as_ref().and_then(|c| c.revision), Some(1)); + // Does not parse (unknown key): the current configuration stays. + let _ = super::replace_current_config(¤t, &pushed(2, "notAKey: 1\n")); + assert_eq!(receiver.borrow().as_ref().and_then(|c| c.revision), Some(1)); + // Wrong hash: stays. + let mut tampered = pushed(3, "programs: {}\n"); + tampered.sha256 = vec![0; 32]; + let _ = super::replace_current_config(¤t, &tampered); + assert_eq!(receiver.borrow().as_ref().and_then(|c| c.revision), Some(1)); + // Parses (its apply may still fail later): replaced. + let _ = super::replace_current_config(¤t, &pushed(4, "programs:\n grok: {}\n")); + assert_eq!(receiver.borrow().as_ref().and_then(|c| c.revision), Some(4)); + } } diff --git a/crates/agent/src/tick.rs b/crates/agent/src/tick.rs new file mode 100644 index 00000000..ef0c0d97 --- /dev/null +++ b/crates/agent/src/tick.rs @@ -0,0 +1,258 @@ +//! Periodic re-apply of the daemon's current configuration (opt-in, +//! `daemon.reconcileInterval`). +//! +//! A managed file deleted or edited by hand, a conflict the user has fixed, +//! or a managed file whose mode was loosened stay as they are until the next +//! config push or restart. [`run_tick`] repairs that by re-applying the +//! current configuration every `reconcileInterval`, without rewriting +//! anything unchanged (see [`crate::reconcile::plan`]). + +use std::{sync::Arc, time::Duration}; + +use agentdesktop_core::config::DaemonConfig; +use tokio::sync::watch; + +use crate::reconcile::{ApplyReport, Reconciler}; + +/// The daemon's current configuration: the startup configuration +/// (`revision: None`), or the latest pushed configuration that passed hash, +/// UTF-8 and parse (`revision: Some(rev)`), whether its apply then succeeded +/// or failed. A push that does not parse leaves this unchanged. +#[derive(Clone)] +pub(crate) struct CurrentConfig { + pub(crate) revision: Option, + pub(crate) config: Arc, +} + +/// One tick's outcome, forwarded to the connection loop for reporting. A +/// tick never persists anything or changes the current configuration. +#[derive(Clone)] +pub(crate) struct TickStatus { + pub(crate) revision: Option, + pub(crate) report: ApplyReport, + pub(crate) error: Option, +} + +/// Applies `current`'s configuration every `interval`, first one interval +/// after start (`interval_at`, `MissedTickBehavior::Delay`), skipping a tick +/// while `current` holds `None`. Stops only when `current`'s sender is +/// dropped (daemon shutdown). `interval` must be greater than zero; a zero +/// interval is refused (logged) rather than scheduled. +pub(crate) async fn run_tick_with( + current: watch::Receiver>, + interval: Duration, + statuses: Option>>, + apply: F, +) where + F: FnMut(&DaemonConfig) -> (ApplyReport, anyhow::Result<()>), +{ + if interval.is_zero() { + tracing::warn!("reconcile interval is zero; not scheduling periodic re-applies"); + return; + } + let mut current = current; + let mut apply = apply; + let mut ticks = tokio::time::interval_at(tokio::time::Instant::now() + interval, interval); + ticks.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + loop { + tokio::select! { + _ = ticks.tick() => {} + changed = current.changed() => { + // A new configuration is picked up at the next tick; a + // dropped sender (daemon shutdown) ends the loop. + if changed.is_err() { + return; + } + continue; + } + } + let Some(snapshot) = current.borrow_and_update().clone() else { + continue; + }; + let (report, result) = apply(&snapshot.config); + if let Some(statuses) = &statuses { + statuses.send_replace(Some(TickStatus { + revision: snapshot.revision, + report, + error: result.err().map(|error| format!("{error:#}")), + })); + } + } +} + +/// [`run_tick_with`] applying through a real [`Reconciler`]. +pub(crate) async fn run_tick( + reconciler: Reconciler, + current: watch::Receiver>, + interval: Duration, + statuses: Option>>, +) { + let latest = current.clone(); + run_tick_with(current, interval, statuses, move |_| { + // Read again under the apply lock: a push that took the lock first + // may have replaced the configuration this tick woke up with. + let Some((previous, report, result)) = reconciler.apply_read_under_lock(|| { + latest + .borrow() + .as_ref() + .map(|current| Arc::clone(¤t.config)) + }) else { + return (ApplyReport::default(), Ok(())); + }; + // Outcome lines and the failure warning only when something changed, + // so a conflict that persists does not repeat every interval. + if crate::reconcile::should_log(previous.as_ref(), &report) { + report.log(); + if let Err(error) = &result { + tracing::warn!(error = %format!("{error:#}"), "reconcile tick failed"); + } + } + (report, result) + }) + .await +} + +#[cfg(test)] +mod tests { + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + + use agentdesktop_core::config::DaemonConfig; + use tokio::{sync::watch, time::Duration}; + + use super::{CurrentConfig, run_tick_with}; + use crate::reconcile::ApplyReport; + + fn current_config(revision: Option) -> CurrentConfig { + CurrentConfig { + revision, + config: Arc::new(DaemonConfig::default()), + } + } + + fn counting( + count: Arc, + ) -> impl FnMut(&DaemonConfig) -> (ApplyReport, anyhow::Result<()>) { + move |_config| { + count.fetch_add(1, Ordering::SeqCst); + (ApplyReport::default(), Ok(())) + } + } + + #[tokio::test(start_paused = true)] + async fn refuses_to_schedule_a_zero_interval() { + let (_current_tx, current_rx) = watch::channel(Some(current_config(None))); + run_tick_with(current_rx, Duration::ZERO, None, |_: &DaemonConfig| { + panic!("a zero interval must not apply") + }) + .await; + } + + #[tokio::test(start_paused = true)] + async fn nothing_before_one_interval_then_one_apply_per_interval() { + let (_current_tx, current_rx) = watch::channel(Some(current_config(None))); + let (status_tx, mut status_rx) = watch::channel(None); + let count = Arc::new(AtomicUsize::new(0)); + let _ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(60), + Some(status_tx), + counting(Arc::clone(&count)), + )); + + assert!( + tokio::time::timeout(Duration::from_secs(59), status_rx.changed()) + .await + .is_err(), + "must not apply before one interval elapses" + ); + assert_eq!(count.load(Ordering::SeqCst), 0); + + status_rx.changed().await.unwrap(); + assert_eq!( + count.load(Ordering::SeqCst), + 1, + "one apply after one interval" + ); + + status_rx.changed().await.unwrap(); + assert_eq!(count.load(Ordering::SeqCst), 2, "one apply per interval"); + } + + #[tokio::test(start_paused = true)] + async fn skips_ticks_while_the_current_configuration_is_none() { + let (current_tx, current_rx) = watch::channel(None); + let count = Arc::new(AtomicUsize::new(0)); + let ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(60), + None, + counting(Arc::clone(&count)), + )); + + tokio::time::sleep(Duration::from_secs(180)).await; + assert_eq!( + count.load(Ordering::SeqCst), + 0, + "no tick applies while the current configuration is None" + ); + + current_tx.send(Some(current_config(None))).unwrap(); + tokio::time::sleep(Duration::from_secs(70)).await; + assert!( + count.load(Ordering::SeqCst) >= 1, + "a tick applies once a configuration is set" + ); + ticker.abort(); + } + + #[tokio::test(start_paused = true)] + async fn stops_when_the_current_configuration_sender_is_dropped() { + let (current_tx, current_rx) = watch::channel(Some(current_config(None))); + let count = Arc::new(AtomicUsize::new(0)); + let ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(60), + None, + counting(Arc::clone(&count)), + )); + drop(current_tx); + + assert!( + tokio::time::timeout(Duration::from_secs(600), ticker) + .await + .expect("run_tick_with must stop once the sender is dropped") + .is_ok() + ); + } + + #[tokio::test(start_paused = true)] + async fn a_failed_apply_forwards_the_error_and_revision() { + let (_current_tx, current_rx) = watch::channel(Some(current_config(Some(9)))); + let (status_tx, mut status_rx) = watch::channel(None); + let _ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(30), + Some(status_tx), + |_: &DaemonConfig| (ApplyReport::default(), Err(anyhow::anyhow!("disk full"))), + )); + + status_rx.changed().await.unwrap(); + let status = status_rx + .borrow_and_update() + .clone() + .expect("a tick status must be published"); + assert_eq!(status.revision, Some(9)); + assert!( + status + .error + .as_deref() + .unwrap_or_default() + .contains("disk full"), + "{:?}", + status.error + ); + } +} diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index c1fbc58d..9aa8a0ac 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -91,6 +91,16 @@ pub struct DaemonStartupConfig { /// Local loopback LLM proxy. #[serde(default)] pub llm_proxy: LlmProxyStartupConfig, + /// 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; read at startup only (restart the + /// daemon after changing it). + #[serde(default, with = "humantime_serde::option")] + #[cfg_attr(feature = "schema", schemars(with = "Option"))] + pub reconcile_interval: Option, } /// Local loopback LLM proxy settings. User mode only: the proxy hands out the @@ -1153,6 +1163,9 @@ pub fn parse_daemon(contents: &str) -> anyhow::Result { Ok(config) } +/// The longest accepted `daemon.reconcileInterval`. +const MAX_RECONCILE_INTERVAL: Duration = Duration::from_secs(30 * 24 * 60 * 60); + fn parse_local_daemon(contents: &str) -> anyhow::Result { let config: DaemonConfig = crate::serdes::yamlviajson::from_str(contents).context("parse daemon configuration")?; @@ -1164,6 +1177,16 @@ fn parse_local_daemon(contents: &str) -> anyhow::Result { if config.inventory_interval.is_zero() { anyhow::bail!("inventoryInterval must be greater than zero"); } + if config + .daemon + .as_ref() + .and_then(|daemon| daemon.reconcile_interval) + .is_some_and(|interval| interval.is_zero() || interval > MAX_RECONCILE_INTERVAL) + { + anyhow::bail!( + "daemon.reconcileInterval must be greater than zero and at most 30 days (leave it unset for no periodic re-apply)" + ); + } validate_daemon( config.llm_gateway.as_ref(), config.sandbox.as_ref(), @@ -1523,6 +1546,51 @@ programs: { claudeCode: { useLlmGateway: false } } ); } + #[test] + fn daemon_reconcile_interval_is_unset_by_default_and_parses_humantime() { + let default = + super::parse_local_daemon("programs: {}").expect("valid daemon configuration"); + assert!(default.daemon.is_none()); + + let configured = super::parse_local_daemon("daemon:\n reconcileInterval: 90s\n") + .expect("valid daemon configuration"); + assert_eq!( + configured.daemon.unwrap().reconcile_interval, + Some(std::time::Duration::from_secs(90)) + ); + } + + #[test] + fn daemon_reconcile_interval_rejects_zero() { + let error = super::parse_local_daemon("daemon:\n reconcileInterval: 0s\n") + .expect_err("a zero reconcile interval is not schedulable"); + assert!( + error.to_string().contains("daemon.reconcileInterval"), + "unexpected error: {error}" + ); + } + + #[test] + fn daemon_reconcile_interval_rejects_more_than_thirty_days() { + assert!(super::parse_local_daemon("daemon:\n reconcileInterval: 30days\n").is_ok()); + let error = super::parse_local_daemon("daemon:\n reconcileInterval: 31days\n") + .expect_err("an interval beyond 30 days is refused"); + assert!( + error.to_string().contains("daemon.reconcileInterval"), + "{error}" + ); + } + + #[test] + fn daemon_reconcile_interval_is_rejected_in_pushed_configuration() { + let error = parse_daemon("daemon:\n reconcileInterval: 5m\nprograms: {}") + .expect_err("daemon.* is local-only, like every other startup setting"); + assert!( + error.to_string().contains("only allowed in the local"), + "unexpected error: {error}" + ); + } + #[test] fn daemon_controller_requires_https() { let plaintext = r#" diff --git a/schema/daemon-config.json b/schema/daemon-config.json index 846c6daa..9c509a06 100644 --- a/schema/daemon-config.json +++ b/schema/daemon-config.json @@ -274,6 +274,14 @@ "$ref": "#/$defs/OpenCodeStartupConfig", "default": {} }, + "reconcileInterval": { + "description": "Opt-in interval between periodic re-applies of the current\nconfiguration, which repair drift (a managed file deleted or edited by\nhand, a fixed conflict, a loosened mode) without rewriting anything\nunchanged. Unset means no periodic re-apply; a controller-managed\ndevice still re-applies the controller's configuration on every\nreconnect. Must be greater than zero; read at startup only (restart the\ndaemon after changing it).", + "type": [ + "string", + "null" + ], + "default": null + }, "socket": { "description": "Local API Unix socket or Windows named pipe.", "type": [ diff --git a/schema/daemon-config.md b/schema/daemon-config.md index 8a3f075e..8d2027f5 100644 --- a/schema/daemon-config.md +++ b/schema/daemon-config.md @@ -26,6 +26,7 @@ |`daemon.openCode`|object|OpenCode paths.| |`daemon.openCode.config`|string|Configuration file.| |`daemon.openCode.plugin`|string|Credential plugin path.| +|`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; read at startup only (restart the
daemon after changing it).| |`daemon.socket`|string|Local API Unix socket or Windows named pipe.| |`daemon.stateDir`|string|Persistent daemon state directory.| |`daemon.user`|boolean|Manage the current user’s tool settings instead of system settings.| From 4721425c08f2568861156e2945d7954c470223e6 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Wed, 7 Oct 2026 02:35:12 +0200 Subject: [PATCH 3/5] fix(agent): tick reports the revision it applied; loosened modes repaired for every managed file Review on #137. - Reconcile tick: the revision in a tick's status is the one read together with the configuration under the apply lock, not the one the tick woke up with. Before, a push between the two published the new revision's outcome under the old revision; the connection loop dropped it and delayed saving the pushed configuration by one interval. A tick that applies nothing (after a logout) publishes nothing. - Mode repair: Codex, Grok, OpenCode, Claude Code and Claude Desktop now treat a managed file with identical bytes but a mode looser than planned as an update and rewrite it at the planned mode (before, they returned `unchanged` before planning a write, so the plan's mode check never saw them). For Claude Code and Claude Desktop such a file is rewritten even without the owner marker, as the unchanged path already claimed it. One predicate, `reconcile::grants_beyond`, decides "looser than planned" for the plan's skip check, json_merge and the VS Code settings file and sidecar; Codex, Grok, OpenCode and Claude Code name their file mode once. Only the permission bits are compared (setuid/setgid/sticky are not); a managed path that is a symlink is replaced by a regular file on such a repair, as on any rewrite. - Schema: `daemon.reconcileInterval` states the 30-day maximum. Tests: 16 new (2 tick, 4 plan, 10 provider; the 10 provider tests and 2 of the plan tests are unix-only); the five existing tick tests take the closure's new return type. The file-mode tests assume umask 022, as the existing ones do. Co-Authored-By: Claude Opus 5.5 --- .../src/provider/claude_code/reconcile.rs | 74 +++++++- .../src/provider/claude_desktop/reconcile.rs | 71 +++++++- crates/agent/src/provider/codex/reconcile.rs | 66 ++++++- crates/agent/src/provider/grok/reconcile.rs | 65 ++++++- crates/agent/src/provider/json_merge.rs | 2 +- .../agent/src/provider/opencode/reconcile.rs | 68 ++++++- crates/agent/src/provider/vscode/settings.rs | 5 +- crates/agent/src/reconcile/mod.rs | 20 ++- crates/agent/src/reconcile/plan.rs | 168 ++++++++++++++++-- crates/agent/src/tick.rs | 89 ++++++++-- crates/core/src/config.rs | 4 +- schema/daemon-config.json | 2 +- schema/daemon-config.md | 2 +- 13 files changed, 585 insertions(+), 51 deletions(-) diff --git a/crates/agent/src/provider/claude_code/reconcile.rs b/crates/agent/src/provider/claude_code/reconcile.rs index 54727276..a981b7b9 100644 --- a/crates/agent/src/provider/claude_code/reconcile.rs +++ b/crates/agent/src/provider/claude_code/reconcile.rs @@ -17,6 +17,9 @@ use crate::provider::{ shared::{CommandSpec, deep_merge}, }; +/// Mode of the managed file; a looser mode is repaired on the next apply. +const MANAGED_FILE_MODE: u32 = 0o644; + const OWNER_MARKER: &[u8] = b"Agentdesktop\n"; pub(super) struct Hooks<'a> { @@ -91,7 +94,10 @@ pub(super) fn plan( } }; let action = match existing.as_deref() { - Some(existing) if existing == contents => { + Some(existing) + if existing == contents + && !crate::reconcile::grants_beyond(path, MANAGED_FILE_MODE) => + { if !owned { plan.write_file(&owner_path, OWNER_MARKER, 0o644)?; } @@ -104,7 +110,9 @@ pub(super) fn plan( plan.record(ClaudeCode::DISPLAY_NAME, "settings", "unchanged", path); return Ok(()); } - Some(_) if owned => "update", + // Identical bytes at a mode looser than planned are rewritten, like + // an owned file (the arm above claims identical bytes as ours). + Some(existing) if owned || existing == contents => "update", Some(existing) => { plan.record_diff( ClaudeCode::DISPLAY_NAME, @@ -119,7 +127,7 @@ pub(super) fn plan( None => "create", }; - plan.write_file(path, &contents, 0o644)?; + plan.write_file(path, &contents, MANAGED_FILE_MODE)?; plan.write_file(&owner_path, OWNER_MARKER, 0o644)?; debug!( program = ClaudeCode::ID, @@ -503,4 +511,64 @@ programs: json!(["~/.ssh"]) ); } + + #[cfg(unix)] + fn mode_repair_fixture() -> (tempfile::TempDir, agentdesktop_core::config::DaemonConfig) { + let config = parse_daemon( + r#" +programs: + claudeCode: {} +"#, + ) + .expect("valid daemon configuration"); + (tempfile::tempdir().unwrap(), config) + } + + #[test] + #[cfg(unix)] + fn identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("settings.json"); + let marker = super::owner_path(&path); + // System mode: `merge_existing = false` writes the file and its + // owner marker, the path that returned `unchanged` before writing. + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| plan_with(&path, &config, plan); + crate::reconcile::plan::mode_repair::assert_identical_bytes_with_looser_mode_is_an_update( + &[&path, &marker], + 0o644, + &plan_fn, + ); + } + + #[test] + #[cfg(unix)] + fn identical_bytes_within_mode_is_unchanged() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("settings.json"); + let marker = super::owner_path(&path); + // System mode: `merge_existing = false` writes the file and its + // owner marker, the path that returned `unchanged` before writing. + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| plan_with(&path, &config, plan); + crate::reconcile::plan::mode_repair::assert_identical_bytes_within_mode_is_unchanged( + &[&path, &marker], + &plan_fn, + ); + } + + #[cfg(unix)] + fn plan_with( + path: &Path, + config: &agentdesktop_core::config::DaemonConfig, + reconcile_plan: &crate::reconcile::ReconcilePlan, + ) -> anyhow::Result<()> { + plan( + path, + false, + "agentdesktop", + Hooks::new(None, None), + None, + Some((config.programs.claude_code.as_ref().unwrap(), None)), + reconcile_plan, + ) + } } diff --git a/crates/agent/src/provider/claude_desktop/reconcile.rs b/crates/agent/src/provider/claude_desktop/reconcile.rs index 29477c66..7b2982c2 100644 --- a/crates/agent/src/provider/claude_desktop/reconcile.rs +++ b/crates/agent/src/provider/claude_desktop/reconcile.rs @@ -194,13 +194,17 @@ fn write_owned( Err(error) => return Err(error).with_context(|| format!("read {}", path.display())), }; let action = match existing.as_deref() { - Some(existing) if existing == contents => { + Some(existing) + if existing == contents && !crate::reconcile::grants_beyond(path, permissions) => + { if !owned { plan.write_file(owner_path, OWNER_MARKER, 0o644)?; } "unchanged" } - Some(_) if owned => "update", + // Identical bytes at a mode looser than planned are rewritten, like + // an owned file (the arm above claims identical bytes as ours). + Some(existing) if owned || existing == contents => "update", Some(existing) => { plan.record_diff( ClaudeDesktop::DISPLAY_NAME, @@ -385,4 +389,67 @@ programs: Some("https://gateway.example.com/") ); } + + #[cfg(unix)] + fn mode_repair_fixture() -> (tempfile::TempDir, agentdesktop_core::config::DaemonConfig) { + let config = parse_daemon( + r#" +programs: + claudeDesktop: {} +"#, + ) + .expect("valid daemon configuration"); + (tempfile::tempdir().unwrap(), config) + } + + #[test] + #[cfg(unix)] + fn identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode() { + 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"); + // System mode: `merge_existing = false`. + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + false, + &helper, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.claude_desktop.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_with_looser_mode_is_an_update( + &[&path, &marker], + 0o644, + &plan_fn, + ); + } + + #[test] + #[cfg(unix)] + fn identical_bytes_within_mode_is_unchanged() { + 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"); + // System mode: `merge_existing = false`. + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + false, + &helper, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.claude_desktop.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_within_mode_is_unchanged( + &[&path, &marker], + &plan_fn, + ); + } } diff --git a/crates/agent/src/provider/codex/reconcile.rs b/crates/agent/src/provider/codex/reconcile.rs index ea67ce06..5f2c6956 100644 --- a/crates/agent/src/provider/codex/reconcile.rs +++ b/crates/agent/src/provider/codex/reconcile.rs @@ -13,6 +13,9 @@ use crate::reconcile::ReconcilePlan; use crate::provider::shared::{deep_merge, responses_base_url}; +/// Mode of the managed file; a looser mode is repaired on the next apply. +const MANAGED_FILE_MODE: u32 = 0o644; + const MANAGED_HEADER: &str = "# Managed by Agentdesktop. Manual changes will be replaced.\n"; pub(super) fn plan( @@ -48,7 +51,11 @@ pub(super) fn plan( } }; let action = match existing.as_deref() { - Some(existing) if existing == contents => { + // Identical bytes at a mode looser than planned: rewritten (update). + Some(existing) + if existing == contents + && !crate::reconcile::grants_beyond(path, MANAGED_FILE_MODE) => + { debug!( program = Codex::ID, action = "unchanged", @@ -73,7 +80,7 @@ pub(super) fn plan( None => "create", }; - plan.write_file(path, &contents, 0o644)?; + plan.write_file(path, &contents, MANAGED_FILE_MODE)?; debug!( program = Codex::ID, action, @@ -334,4 +341,59 @@ programs: "allow" ); } + + #[cfg(unix)] + fn mode_repair_fixture() -> (tempfile::TempDir, agentdesktop_core::config::DaemonConfig) { + let config = parse_daemon( + r#" +programs: + codex: {} +"#, + ) + .expect("valid daemon configuration"); + (tempfile::tempdir().unwrap(), config) + } + + #[test] + #[cfg(unix)] + fn identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("config.toml"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + None, + Some((config.programs.codex.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_with_looser_mode_is_an_update( + &[&path], + 0o644, + &plan_fn, + ); + } + + #[test] + #[cfg(unix)] + fn identical_bytes_within_mode_is_unchanged() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("config.toml"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + None, + Some((config.programs.codex.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_within_mode_is_unchanged( + &[&path], + &plan_fn, + ); + } } diff --git a/crates/agent/src/provider/grok/reconcile.rs b/crates/agent/src/provider/grok/reconcile.rs index abcc3241..eb25049e 100644 --- a/crates/agent/src/provider/grok/reconcile.rs +++ b/crates/agent/src/provider/grok/reconcile.rs @@ -10,6 +10,9 @@ use tracing::debug; use crate::provider::shared::{deep_merge, responses_base_url}; use crate::reconcile::ReconcilePlan; +/// Mode of the managed file; a looser mode is repaired on the next apply. +const MANAGED_FILE_MODE: u32 = 0o644; + const MANAGED_HEADER: &str = "# Managed by agentdesktop. Manual changes will be replaced.\n"; const PROVIDER_NAME: &str = "agentdesktop"; @@ -45,7 +48,11 @@ pub(super) fn plan( } }; let action = match existing.as_deref() { - Some(existing) if existing == contents => { + // Identical bytes at a mode looser than planned: rewritten (update). + Some(existing) + if existing == contents + && !crate::reconcile::grants_beyond(path, MANAGED_FILE_MODE) => + { debug!( program = Grok::ID, action = "unchanged", @@ -70,7 +77,7 @@ pub(super) fn plan( None => "create", }; - plan.write_file(path, &contents, 0o644)?; + plan.write_file(path, &contents, MANAGED_FILE_MODE)?; debug!( program = Grok::ID, action, @@ -470,4 +477,58 @@ programs: 600 ); } + + #[cfg(unix)] + fn mode_repair_fixture() -> (tempfile::TempDir, agentdesktop_core::config::DaemonConfig) { + let config = parse_daemon( + r#" +programs: + grok: + model: grok-4.6 +"#, + ) + .expect("valid daemon configuration"); + (tempfile::tempdir().unwrap(), config) + } + + #[test] + #[cfg(unix)] + fn identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("config.toml"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.grok.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_with_looser_mode_is_an_update( + &[&path], + 0o644, + &plan_fn, + ); + } + + #[test] + #[cfg(unix)] + fn identical_bytes_within_mode_is_unchanged() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("config.toml"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.grok.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_within_mode_is_unchanged( + &[&path], + &plan_fn, + ); + } } diff --git a/crates/agent/src/provider/json_merge.rs b/crates/agent/src/provider/json_merge.rs index 60f2ea6e..eecf7d27 100644 --- a/crates/agent/src/provider/json_merge.rs +++ b/crates/agent/src/provider/json_merge.rs @@ -173,7 +173,7 @@ pub(super) fn plan_merge_with( // An unchanged file that grants more than `options.mode` is rewritten // with the same bytes at that mode (the plan skips identical writes // otherwise). - let looser = current_mode(path).is_some_and(|mode| mode & !options.mode & 0o777 != 0); + let looser = crate::reconcile::grants_beyond(path, options.mode); let action = match existing.as_deref() { Some(existing) if existing == contents && !looser => "unchanged", Some(_) => "update", diff --git a/crates/agent/src/provider/opencode/reconcile.rs b/crates/agent/src/provider/opencode/reconcile.rs index 31793ee4..1b4f9219 100644 --- a/crates/agent/src/provider/opencode/reconcile.rs +++ b/crates/agent/src/provider/opencode/reconcile.rs @@ -12,6 +12,9 @@ use crate::reconcile::ReconcilePlan; use crate::provider::shared::{deep_merge, responses_base_url}; +/// Mode of the managed file; a looser mode is repaired on the next apply. +const MANAGED_FILE_MODE: u32 = 0o644; + const MANAGED_HEADER: &str = "// Managed by Agentdesktop. Manual changes will be replaced.\n"; const CONFIG_PROGRAM: &str = OpenCode::ID; @@ -190,7 +193,11 @@ fn reconcile_file( } }; let action = match existing.as_deref() { - Some(existing) if existing == contents => { + // Identical bytes at a mode looser than planned: rewritten (update). + Some(existing) + if existing == contents + && !crate::reconcile::grants_beyond(path, MANAGED_FILE_MODE) => + { debug!( program = CONFIG_PROGRAM, kind = description, @@ -215,7 +222,7 @@ fn reconcile_file( } None => "create", }; - plan.write_file(path, contents, 0o644)?; + plan.write_file(path, contents, MANAGED_FILE_MODE)?; debug!( program = CONFIG_PROGRAM, kind = description, @@ -352,4 +359,61 @@ programs: assert!(plugin.contains("input.model.providerID !== provider")); assert!(plugin.contains("output.headers.Authorization")); } + + #[cfg(unix)] + fn mode_repair_fixture() -> (tempfile::TempDir, agentdesktop_core::config::DaemonConfig) { + let config = parse_daemon( + r#" +programs: + openCode: {} +"#, + ) + .expect("valid daemon configuration"); + (tempfile::tempdir().unwrap(), config) + } + + #[test] + #[cfg(unix)] + fn identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("opencode.jsonc"); + let plugin = dir.path().join("plugin.js"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + &plugin, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.open_code.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_with_looser_mode_is_an_update( + &[&path], + 0o644, + &plan_fn, + ); + } + + #[test] + #[cfg(unix)] + fn identical_bytes_within_mode_is_unchanged() { + let (dir, config) = mode_repair_fixture(); + let path = dir.path().join("opencode.jsonc"); + let plugin = dir.path().join("plugin.js"); + let plan_fn = |plan: &crate::reconcile::ReconcilePlan| { + super::plan( + &path, + &plugin, + Path::new("agentdesktop"), + Path::new("agentdesktop.sock"), + Some((config.programs.open_code.as_ref().unwrap(), None)), + plan, + ) + }; + crate::reconcile::plan::mode_repair::assert_identical_bytes_within_mode_is_unchanged( + &[&path], + &plan_fn, + ); + } } diff --git a/crates/agent/src/provider/vscode/settings.rs b/crates/agent/src/provider/vscode/settings.rs index a2ca7c5e..5e0cfc21 100644 --- a/crates/agent/src/provider/vscode/settings.rs +++ b/crates/agent/src/provider/vscode/settings.rs @@ -537,7 +537,7 @@ pub(super) fn plan( // An unchanged file is not rewritten, unless its mode lets others read // it (VS Code saves it 664): then the same bytes are written 0600. - let looser = json_merge::current_mode(path).is_some_and(|mode| mode & !FILE_MODE & 0o777 != 0); + let looser = crate::reconcile::grants_beyond(path, FILE_MODE); let action = match text { None => "create", Some(text) if text == edited && !looser => "unchanged", @@ -549,8 +549,7 @@ pub(super) fn plan( .with_context(|| format!("serialize {} {DESCRIPTION} state", VsCode::DISPLAY_NAME))?; state_bytes.push(b'\n'); // An identical sidecar is rewritten only when its mode lets others read it. - let looser_sidecar = - json_merge::current_mode(&state_path).is_some_and(|mode| mode & !FILE_MODE & 0o777 != 0); + let looser_sidecar = crate::reconcile::grants_beyond(&state_path, FILE_MODE); if sidecar.as_deref() != Some(state_bytes.as_slice()) || looser_sidecar { plan.write_file(&state_path, &state_bytes, FILE_MODE)?; } diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index 101f39d7..3ca87d01 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -1,6 +1,7 @@ //! Shared reconciliation orchestration, plan application, and dry-run reporting. -mod plan; +pub(crate) mod plan; pub use plan::ReconcilePlan; +pub(crate) use plan::grants_beyond; use crate::provider::{ Provider, ReconcileContext, claude_code::ClaudeCode, claude_desktop::ClaudeDesktop, @@ -207,29 +208,30 @@ impl Reconciler { &self, config: &DaemonConfig, ) -> (Option, ApplyReport, anyhow::Result<()>) { - let (previous, report, result) = self - .apply_read_under_lock(|| Some(Arc::new(config.clone()))) + let ((), previous, report, result) = self + .apply_read_under_lock(|| Some(((), Arc::new(config.clone())))) .expect("a configuration was given"); (previous, report, result) } - /// Takes the apply lock, then reads the configuration to apply, so a + /// Takes the apply lock, then reads the configuration to apply (with + /// whatever `read` returns alongside it, such as its revision), so a /// configuration pushed while this call waited for the lock is the one /// applied (a tick never re-applies an older revision after a push). /// `None` from `read` applies nothing. #[allow(clippy::type_complexity)] - pub(crate) fn apply_read_under_lock( + pub(crate) fn apply_read_under_lock( &self, - read: impl FnOnce() -> Option>, - ) -> Option<(Option, ApplyReport, anyhow::Result<()>)> { + read: impl FnOnce() -> Option<(T, Arc)>, + ) -> Option<(T, Option, ApplyReport, anyhow::Result<()>)> { let mut last = self .last_apply .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - let config = read()?; + let (value, config) = read()?; let (report, result) = self.plan_with_report(&config).apply(); let previous = last.replace(report.clone()); - Some((previous, report, result)) + Some((value, previous, report, result)) } } diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index cf968fbd..f8092388 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -333,28 +333,174 @@ fn already_in_place( permissions: u32, observed: &BTreeMap>>, ) -> bool { - if observed.get(path).and_then(Option::as_deref) != Some(contents) { - return false; - } + // A file gone since it was observed is written again. + observed.get(path).and_then(Option::as_deref) == Some(contents) + && fs::metadata(path).is_ok() + && !grants_beyond(path, permissions) +} + +/// 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. +pub(crate) fn grants_beyond(path: &Path, mode: u32) -> bool { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - match fs::metadata(path) { - Ok(metadata) => metadata.permissions().mode() & !permissions & 0o777 == 0, - Err(_) => false, - } + fs::metadata(path).is_ok_and(|metadata| metadata.permissions().mode() & !mode & 0o777 != 0) } #[cfg(not(unix))] { - let _ = permissions; - true + let _ = (path, mode); + false + } +} + +/// Shared checks for the planners that write a managed file (C5.1). +#[cfg(all(test, unix))] +pub(crate) mod mode_repair { + use super::ReconcilePlan; + use std::{ + fs, + os::unix::fs::{MetadataExt, PermissionsExt}, + path::Path, + }; + + fn mode(path: &Path) -> u32 { + fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + fn applied(plan_fn: &dyn Fn(&ReconcilePlan) -> anyhow::Result<()>) { + let plan = ReconcilePlan::default(); + plan_fn(&plan).unwrap(); + plan.apply().unwrap(); + } + + /// `plan_fn` plans the program's managed `files`, each planned at + /// `planned_mode`. After a first apply every file is loosened to 0666 with + /// its bytes untouched; the next plan must record an `update` and the + /// apply must restore `planned_mode` on every file. + pub(crate) fn assert_identical_bytes_with_looser_mode_is_an_update( + files: &[&Path], + planned_mode: u32, + plan_fn: &dyn Fn(&ReconcilePlan) -> anyhow::Result<()>, + ) { + applied(plan_fn); + let bytes: Vec<_> = files.iter().map(|file| fs::read(file).unwrap()).collect(); + for file in files { + fs::set_permissions(file, fs::Permissions::from_mode(0o666)).unwrap(); + } + + let plan = ReconcilePlan::default(); + plan_fn(&plan).unwrap(); + let rendered = plan.render(); + assert!( + rendered.contains("UPDATE"), + "a loosened mode with identical bytes must be an update: {rendered}" + ); + plan.apply().unwrap(); + + for (file, bytes) in files.iter().zip(bytes) { + assert_eq!( + mode(file), + planned_mode, + "{} must be rewritten at the planned mode", + file.display() + ); + assert_eq!(fs::read(file).unwrap(), bytes); + } + } + + /// After a first apply every file is tightened to 0600 (within any planned + /// mode); the next plan must record no change and must not write. + pub(crate) fn assert_identical_bytes_within_mode_is_unchanged( + files: &[&Path], + plan_fn: &dyn Fn(&ReconcilePlan) -> anyhow::Result<()>, + ) { + applied(plan_fn); + for file in files { + fs::set_permissions(file, fs::Permissions::from_mode(0o600)).unwrap(); + } + let before: Vec<_> = files + .iter() + .map(|file| { + let metadata = fs::metadata(file).unwrap(); + (metadata.ino(), metadata.modified().unwrap()) + }) + .collect(); + + let plan = ReconcilePlan::default(); + plan_fn(&plan).unwrap(); + let rendered = plan.render(); + assert!( + !rendered.contains("UPDATE") && !rendered.contains("CREATE"), + "an identical file within the planned mode must stay unchanged: {rendered}" + ); + plan.apply().unwrap(); + + for (file, (ino, modified)) in files.iter().zip(before) { + let metadata = fs::metadata(file).unwrap(); + assert_eq!( + metadata.ino(), + ino, + "{} must not be replaced", + file.display() + ); + assert_eq!(metadata.modified().unwrap(), modified); + assert_eq!(mode(file), 0o600, "a stricter mode must be left"); + } } } #[cfg(test)] mod tests { - use super::ReconcilePlan; - use std::fs; + use super::{ReconcilePlan, already_in_place, grants_beyond}; + use std::{collections::BTreeMap, fs}; + + #[test] + fn grants_beyond_is_false_for_a_missing_file() { + let dir = tempfile::tempdir().unwrap(); + assert!(!grants_beyond(&dir.path().join("absent"), 0o600)); + } + + #[test] + #[cfg(unix)] + fn grants_beyond_is_false_within_the_mode() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + fs::write(&path, b"x").unwrap(); + for current in [0o600, 0o640, 0o644] { + fs::set_permissions(&path, fs::Permissions::from_mode(current)).unwrap(); + assert!(!grants_beyond(&path, 0o644), "{current:o} within 644"); + } + } + + #[test] + #[cfg(unix)] + fn grants_beyond_is_true_with_a_bit_outside_the_mode() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + fs::write(&path, b"x").unwrap(); + for (current, planned) in [(0o664, 0o644), (0o644, 0o600), (0o755, 0o644)] { + fs::set_permissions(&path, fs::Permissions::from_mode(current)).unwrap(); + assert!( + grants_beyond(&path, planned), + "{current:o} beyond {planned:o}" + ); + } + } + + #[test] + fn already_in_place_still_writes_a_file_removed_after_the_observed_check() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("gone"); + let observed = BTreeMap::from([(path.clone(), Some(b"hello\n".to_vec()))]); + assert!( + !already_in_place(&path, b"hello\n", 0o644, &observed), + "a file that no longer exists must be written" + ); + } #[test] fn identical_write_is_skipped_same_inode_and_mtime() { diff --git a/crates/agent/src/tick.rs b/crates/agent/src/tick.rs index ef0c0d97..ca41f821 100644 --- a/crates/agent/src/tick.rs +++ b/crates/agent/src/tick.rs @@ -28,6 +28,9 @@ pub(crate) struct CurrentConfig { /// tick never persists anything or changes the current configuration. #[derive(Clone)] pub(crate) struct TickStatus { + /// The revision of the configuration this tick applied, read with it + /// under the apply lock; the connection loop reports and saves the + /// outcome only when it matches the current stream revision. pub(crate) revision: Option, pub(crate) report: ApplyReport, pub(crate) error: Option, @@ -44,7 +47,7 @@ pub(crate) async fn run_tick_with( statuses: Option>>, apply: F, ) where - F: FnMut(&DaemonConfig) -> (ApplyReport, anyhow::Result<()>), + F: FnMut(&DaemonConfig) -> Option<(Option, ApplyReport, anyhow::Result<()>)>, { if interval.is_zero() { tracing::warn!("reconcile interval is zero; not scheduling periodic re-applies"); @@ -69,10 +72,13 @@ pub(crate) async fn run_tick_with( let Some(snapshot) = current.borrow_and_update().clone() else { continue; }; - let (report, result) = apply(&snapshot.config); + // `None`: the closure applied nothing, so nothing is published. + let Some((revision, report, result)) = apply(&snapshot.config) else { + continue; + }; if let Some(statuses) = &statuses { statuses.send_replace(Some(TickStatus { - revision: snapshot.revision, + revision, report, error: result.err().map(|error| format!("{error:#}")), })); @@ -91,14 +97,12 @@ pub(crate) async fn run_tick( run_tick_with(current, interval, statuses, move |_| { // Read again under the apply lock: a push that took the lock first // may have replaced the configuration this tick woke up with. - let Some((previous, report, result)) = reconciler.apply_read_under_lock(|| { + let (revision, previous, report, result) = reconciler.apply_read_under_lock(|| { latest .borrow() .as_ref() - .map(|current| Arc::clone(¤t.config)) - }) else { - return (ApplyReport::default(), Ok(())); - }; + .map(|current| (current.revision, Arc::clone(¤t.config))) + })?; // Outcome lines and the failure warning only when something changed, // so a conflict that persists does not repeat every interval. if crate::reconcile::should_log(previous.as_ref(), &report) { @@ -107,7 +111,7 @@ pub(crate) async fn run_tick( tracing::warn!(error = %format!("{error:#}"), "reconcile tick failed"); } } - (report, result) + Some((revision, report, result)) }) .await } @@ -134,10 +138,10 @@ mod tests { fn counting( count: Arc, - ) -> impl FnMut(&DaemonConfig) -> (ApplyReport, anyhow::Result<()>) { + ) -> impl FnMut(&DaemonConfig) -> Option<(Option, ApplyReport, anyhow::Result<()>)> { move |_config| { count.fetch_add(1, Ordering::SeqCst); - (ApplyReport::default(), Ok(())) + Some((None, ApplyReport::default(), Ok(()))) } } @@ -236,7 +240,13 @@ mod tests { current_rx, Duration::from_secs(30), Some(status_tx), - |_: &DaemonConfig| (ApplyReport::default(), Err(anyhow::anyhow!("disk full"))), + |_: &DaemonConfig| { + Some(( + Some(9), + ApplyReport::default(), + Err(anyhow::anyhow!("disk full")), + )) + }, )); status_rx.changed().await.unwrap(); @@ -255,4 +265,59 @@ mod tests { status.error ); } + + #[tokio::test(start_paused = true)] + async fn a_tick_reports_the_revision_the_apply_returns() { + // The snapshot the tick woke up with is revision 3; the apply (which + // re-reads under the lock) applied revision 5. + let (_current_tx, current_rx) = watch::channel(Some(current_config(Some(3)))); + let (status_tx, mut status_rx) = watch::channel(None); + let _ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(30), + Some(status_tx), + |_: &DaemonConfig| Some((Some(5), ApplyReport::default(), Ok(()))), + )); + + status_rx.changed().await.unwrap(); + let status = status_rx + .borrow_and_update() + .clone() + .expect("a tick status must be published"); + assert_eq!( + status.revision, + Some(5), + "the published revision is the one the apply returned, not the snapshot's" + ); + assert!(status.error.is_none()); + } + + #[tokio::test(start_paused = true)] + async fn no_status_when_the_apply_returns_none() { + let (_current_tx, current_rx) = watch::channel(Some(current_config(Some(3)))); + let (status_tx, mut status_rx) = watch::channel(None); + let count = Arc::new(AtomicUsize::new(0)); + let applied = Arc::clone(&count); + let _ticker = tokio::spawn(run_tick_with( + current_rx, + Duration::from_secs(30), + Some(status_tx), + move |_: &DaemonConfig| { + applied.fetch_add(1, Ordering::SeqCst); + None + }, + )); + + assert!( + tokio::time::timeout(Duration::from_secs(100), status_rx.changed()) + .await + .is_err(), + "a closure returning None must publish nothing" + ); + assert!( + count.load(Ordering::SeqCst) >= 3, + "the closure must have run on each tick" + ); + assert!(status_rx.borrow().is_none()); + } } diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index 9aa8a0ac..ffd5c6b0 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -96,8 +96,8 @@ pub struct DaemonStartupConfig { /// 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; read at startup only (restart the - /// daemon after changing it). + /// reconnect. Must be greater than zero and at most 30 days; read at + /// startup only (restart the daemon after changing it). #[serde(default, with = "humantime_serde::option")] #[cfg_attr(feature = "schema", schemars(with = "Option"))] pub reconcile_interval: Option, diff --git a/schema/daemon-config.json b/schema/daemon-config.json index 9c509a06..79fd886d 100644 --- a/schema/daemon-config.json +++ b/schema/daemon-config.json @@ -275,7 +275,7 @@ "default": {} }, "reconcileInterval": { - "description": "Opt-in interval between periodic re-applies of the current\nconfiguration, which repair drift (a managed file deleted or edited by\nhand, a fixed conflict, a loosened mode) without rewriting anything\nunchanged. Unset means no periodic re-apply; a controller-managed\ndevice still re-applies the controller's configuration on every\nreconnect. Must be greater than zero; read at startup only (restart the\ndaemon after changing it).", + "description": "Opt-in interval between periodic re-applies of the current\nconfiguration, which repair drift (a managed file deleted or edited by\nhand, a fixed conflict, a loosened mode) without rewriting anything\nunchanged. Unset means no periodic re-apply; a controller-managed\ndevice still re-applies the controller's configuration on every\nreconnect. Must be greater than zero and at most 30 days; read at\nstartup only (restart the daemon after changing it).", "type": [ "string", "null" diff --git a/schema/daemon-config.md b/schema/daemon-config.md index 8d2027f5..bc5a629d 100644 --- a/schema/daemon-config.md +++ b/schema/daemon-config.md @@ -26,7 +26,7 @@ |`daemon.openCode`|object|OpenCode paths.| |`daemon.openCode.config`|string|Configuration file.| |`daemon.openCode.plugin`|string|Credential plugin path.| -|`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; read at startup only (restart the
daemon after changing it).| +|`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.| |`daemon.stateDir`|string|Persistent daemon state directory.| |`daemon.user`|boolean|Manage the current user’s tool settings instead of system settings.| From 04d2c3fc36d935f163b235b6146e05d7c15f3ae4 Mon Sep 17 00:00:00 2001 From: Lutz Lange Date: Wed, 30 Sep 2026 18:42:45 +0200 Subject: [PATCH 4/5] feat: llmGateway.whenProxyUnavailable and a GitHub Copilot example Proxy-unavailable policy - While the daemon runs without its local LLM proxy, the programs behind it used to remove their managed entries; for VS Code on GitHub's models that removes the CAPI override, so Copilot Chat talks to GitHub directly, past the gateway. - `llmGateway.whenProxyUnavailable` now decides for `programs.copilot` and `programs.vscode`: `failClosed` (default) leaves the managed files as they are, so the tools stay pointed at the loopback port and fail; `failOpen` removes them until the proxy is back. The program is `inactive` with a reason that names the policy. - Program removal, a mode switch, `useLlmGateway: false` and a missing `proxyUrl` remove as before. The controller's configuration view offers the choice ("Fail open without the local proxy") when a Copilot program is selected. Older daemons and controllers reject the key (`deny_unknown_fields`); the default needs no key. Example - `examples/copilot/`: Dex, agentgateway, a daemon config and a step-by-step guide for the Copilot CLI, VS Code on own models and VS Code on GitHub's models, with what to check at each step (drift, conflicts, removal, the proxy gone), plus the settings the GitHub organisation owner configures (`github-side.md`). Co-Authored-By: Claude Opus 5.5 --- README.md | 53 ++++- .../agent/src/provider/copilot/reconcile.rs | 11 +- crates/agent/src/provider/copilot/tests.rs | 54 ++++- crates/agent/src/provider/vscode/reconcile.rs | 11 +- crates/agent/src/provider/vscode/settings.rs | 18 +- .../src/provider/vscode/settings_tests.rs | 87 +++++++- crates/agent/src/provider/vscode/tests.rs | 29 ++- crates/agent/src/reconcile/mod.rs | 20 ++ crates/agent/src/reconcile/plan.rs | 6 + crates/core/src/config.rs | 29 +++ examples/copilot/README.md | 199 ++++++++++++++++++ examples/copilot/agentgateway.yaml | 81 +++++++ examples/copilot/compose.yaml | 23 ++ examples/copilot/config.yaml | 39 ++++ examples/copilot/dex.yaml | 24 +++ examples/copilot/github-side.md | 37 ++++ frontend/controller/src/stories/fixtures.ts | 13 ++ frontend/controller/src/styles.css | 1 + frontend/controller/src/types.ts | 1 + .../src/views/ConfigurationView.stories.tsx | 31 ++- .../src/views/ConfigurationView.tsx | 31 +++ schema/daemon-config.json | 19 ++ schema/daemon-config.md | 1 + 23 files changed, 794 insertions(+), 24 deletions(-) create mode 100644 examples/copilot/README.md create mode 100644 examples/copilot/agentgateway.yaml create mode 100644 examples/copilot/compose.yaml create mode 100644 examples/copilot/config.yaml create mode 100644 examples/copilot/dex.yaml create mode 100644 examples/copilot/github-side.md diff --git a/README.md b/README.md index dea1dcc2..fd54fe90 100644 --- a/README.md +++ b/README.md @@ -133,6 +133,33 @@ The incoming path and query are appended to the gateway base URL: with `llmGateway.proxyUrl: https://gateway.example/prefix` (or `url`, when `proxyUrl` is unset), `/v1/messages` goes to `https://gateway.example/prefix/v1/messages`. Avoid repeating `/v1` in both URLs. + +**Without the proxy.** When the daemon runs without its loopback proxy +(`daemon.llmProxy.listen` unset, or the address could not be bound), +`llmGateway.whenProxyUnavailable` decides what happens to the files of the +programs that use the proxy (`programs.copilot`, `programs.vscode`). With +`failClosed`, the default, the daemon leaves their managed entries as they +are: the Copilot CLI and VS Code stay pointed at the loopback port, and those +requests fail instead of reaching GitHub past the gateway. If the address could +not be bound because another process holds it, that process receives those +requests, with VS Code's Copilot tokens and the pairing value: use the default +on devices with a single user, as for the proxy in general. With +`failOpen`, the daemon removes the entries until the proxy is back: VS Code on +GitHub's models then talks to GitHub directly, and the Copilot CLI and VS Code +on own models lose the gateway's models. Either way the programs report +`inactive` with the reason, and the next apply with the proxy writes the +current entries. A stopped daemon changes no file, so the tools fail until it +runs again, whatever the policy. The policy covers only the files the daemon +writes: whether developers can reach GitHub's models without the gateway is +set by the organisation's Copilot policy and egress rules. Set the key only +after the controller and the daemons run a version that knows it; older ones +reject a configuration that carries it. + +```yaml +llmGateway: + url: https://gateway.example.com + whenProxyUnavailable: failOpen # default failClosed +``` The proxy does not discover models or rewrite model IDs; configure those in the client. VS Code's built-in **Custom Endpoint** provider can use this endpoint without the experimental extension in `vscode/`. The provider reads @@ -271,6 +298,14 @@ device authorization again. Non-expiring App user tokens are also supported. Request bodies remain untouched. The existing Claude credential-helper flow is unchanged; the local proxy always uses the gateway identity in `Authorization`. +### GitHub Copilot: a runnable example + +[`examples/copilot`](examples/copilot/README.md) runs Dex and Agentgateway +locally and walks through the Copilot CLI, VS Code on the gateway's models and +VS Code on GitHub's models, with what to check at each step (drift, +conflicts, removal) and the settings the GitHub organisation owner controls +([`examples/copilot/github-side.md`](examples/copilot/github-side.md)). + ### Managed program: GitHub Copilot CLI `programs.copilot` makes the daemon write the Copilot CLI's BYOK provider @@ -317,8 +352,9 @@ away; entries with another pairing value are left alone. The file is written own daemon's sidecar next to it (`.providers.json.agentdesktop`, owner-only) keeps a copy of the file as last written, including the user's own providers and their `apiKey` values, until the next apply. Removing -`programs.copilot`, setting `useLlmGateway: false`, or running without the -proxy takes the managed entries out again and restores the user's file with +`programs.copilot` or setting `useLlmGateway: false` takes the managed +entries out again (running without the proxy depends on +`llmGateway.whenProxyUnavailable`, see **Without the proxy**) and restores the user's file with its previous mode (deleted only if the daemon created it and nothing else is left). @@ -501,11 +537,14 @@ single-quoted string; the top level is not an object; leaves the file untouched and blocks every managed file on the device until it is resolved (and stops the daemon at startup); fix the file in VS Code and re-apply (with `daemon.reconcileInterval` set, the next tick applies it). Removal (program -absent, `ownModels`, `useLlmGateway: false`, no gateway, no `proxyUrl`, or no -proxy, with a warning) takes the key and the two entries out and deletes the -file only if the daemon created it. Restart VS Code after an apply that changes +absent, `ownModels`, `useLlmGateway: false`, no gateway, or no `proxyUrl`) +takes the key and the two entries out and deletes the file only if the daemon +created it. Without the proxy, `llmGateway.whenProxyUnavailable` decides (see +**Without the proxy** above): the default keeps the override, so Copilot Chat +fails instead of reaching GitHub directly. Restart VS Code after an apply that changes the file (first apply, re-pairing after a re-enrollment). There is no -`--dry-run` preview: a dry run has no proxy and plans a removal; `--once` +`--dry-run` preview: a dry run has no proxy, so it lists nothing for this file +under `failClosed` and the removal under `failOpen`; `--once` refuses the program. Caveats. `github.copilot.advanced.debug.overrideCapiUrl` is an undocumented @@ -633,7 +672,7 @@ device-wide state. The device page in the controller lists them under | `unchanged` | The program is configured and nothing needed to change. | | `removed` | The program is no longer configured and its managed content was removed. | | `conflict` | A managed file holds configuration the daemon will not overwrite; the detail names the file. | -| `inactive` | The program uses the LLM gateway but the local LLM proxy is not running (see `llmProxy.error` in daemon-info), so its files point nowhere and were removed. | +| `inactive` | The program uses the LLM gateway but the local LLM proxy is not running (see `llmProxy.error` in daemon-info): its entries were left pointing at the loopback port (`whenProxyUnavailable: failClosed`, the default) or removed (`failOpen`). | | `blocked` | The program had changes, but none were written because another program conflicted or failed; the detail names that program. | | `failed` | Planning or writing this program failed; the detail carries the error. | diff --git a/crates/agent/src/provider/copilot/reconcile.rs b/crates/agent/src/provider/copilot/reconcile.rs index e42ed01e..997fec20 100644 --- a/crates/agent/src/provider/copilot/reconcile.rs +++ b/crates/agent/src/provider/copilot/reconcile.rs @@ -103,14 +103,21 @@ pub(super) fn plan( ) -> anyhow::Result<()> { let state_path = json_merge::state_path(path); let pairing = proxy.map(|(_, pairing)| pairing); - let Some((config, Some(_gateway))) = configured else { + let Some((config, Some(gateway))) = configured else { remove(path, &state_path, pairing, plan)?; return Ok(()); }; let Some((listen, pairing)) = proxy else { + if crate::reconcile::fail_closed_without_proxy(gateway, plan) { + tracing::debug!( + path = %path.display(), + "programs.copilot is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the managed Copilot CLI providers are left as they are" + ); + return Ok(()); + } tracing::debug!( path = %path.display(), - "programs.copilot is configured but the local LLM proxy is not available, so the Copilot CLI is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed Copilot CLI providers" + "programs.copilot is configured but the local LLM proxy is not available, so the Copilot CLI is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); whenProxyUnavailable: failOpen, removing the managed Copilot CLI providers" ); plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); remove(path, &state_path, None, plan)?; diff --git a/crates/agent/src/provider/copilot/tests.rs b/crates/agent/src/provider/copilot/tests.rs index 2975bab1..9ef723b3 100644 --- a/crates/agent/src/provider/copilot/tests.rs +++ b/crates/agent/src/provider/copilot/tests.rs @@ -7,7 +7,9 @@ use std::{ net::SocketAddr, }; -use agentdesktop_core::config::{CopilotConfig, CopilotModel, CopilotProvider, LlmGatewayConfig}; +use agentdesktop_core::config::{ + CopilotConfig, CopilotModel, CopilotProvider, LlmGatewayConfig, ProxyUnavailable, +}; use serde_json::{Value, json}; use super::discovery; @@ -49,6 +51,7 @@ fn sample_gateway() -> LlmGatewayConfig { authentication: None, proxy_url: None, github_oauth: None, + when_proxy_unavailable: Default::default(), } } @@ -575,11 +578,12 @@ fn removal_when_use_llm_gateway_is_effectively_false() { } #[test] -fn removal_when_the_loopback_proxy_is_unavailable() { +fn fail_open_removes_the_entries_when_the_loopback_proxy_is_unavailable() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("providers.json"); let config = sample_config(); - let gateway = sample_gateway(); + let mut gateway = sample_gateway(); + gateway.when_proxy_unavailable = ProxyUnavailable::FailOpen; let changes = ReconcilePlan::default(); plan( @@ -603,6 +607,50 @@ fn removal_when_the_loopback_proxy_is_unavailable() { ); } +#[test] +fn fail_closed_keeps_the_entries_when_the_loopback_proxy_is_unavailable() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("providers.json"); + let config = sample_config(); + let gateway = sample_gateway(); + assert_eq!( + gateway.when_proxy_unavailable, + ProxyUnavailable::FailClosed, + "the default" + ); + let changes = ReconcilePlan::default(); + plan( + &path, + Some((listen_addr(), PAIRING)), + Some((&config, Some(&gateway))), + &changes, + ) + .unwrap(); + changes.apply().unwrap(); + let state = super::super::json_merge::state_path(&path); + let (before, state_before) = (fs::read(&path).unwrap(), fs::read(&state).unwrap()); + + let changes = ReconcilePlan::default(); + plan(&path, None, Some((&config, Some(&gateway))), &changes).unwrap(); + assert_eq!( + changes.inactive_reason().as_deref(), + Some(crate::reconcile::FAIL_CLOSED_REASON) + ); + changes.apply().unwrap(); + assert_eq!( + fs::read(&path).unwrap(), + before, + "the providers stay, pointed at the loopback port" + ); + assert_eq!(fs::read(&state).unwrap(), state_before); + + // Removing the program still removes, whatever the policy. + let changes = ReconcilePlan::default(); + plan(&path, None, None, &changes).unwrap(); + changes.apply().unwrap(); + assert!(!path.exists()); +} + // --- plan(): conflicts ------------------------------------------------------ #[test] diff --git a/crates/agent/src/provider/vscode/reconcile.rs b/crates/agent/src/provider/vscode/reconcile.rs index 4272daf5..1d801953 100644 --- a/crates/agent/src/provider/vscode/reconcile.rs +++ b/crates/agent/src/provider/vscode/reconcile.rs @@ -98,14 +98,21 @@ pub(super) fn plan( ) -> anyhow::Result<()> { let state_path = json_merge::state_path(path); let pairing = proxy.map(|(_, pairing)| pairing); - let Some((config, Some(_gateway))) = configured else { + let Some((config, Some(gateway))) = configured else { remove(path, &state_path, pairing, plan)?; return Ok(()); }; let Some((listen, pairing)) = proxy else { + if crate::reconcile::fail_closed_without_proxy(gateway, plan) { + tracing::debug!( + path = %path.display(), + "programs.vscode is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the managed chat language models are left as they are" + ); + return Ok(()); + } tracing::debug!( path = %path.display(), - "programs.vscode is configured but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed chat language models" + "programs.vscode is configured but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); whenProxyUnavailable: failOpen, removing the managed chat language models" ); plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); remove(path, &state_path, None, plan)?; diff --git a/crates/agent/src/provider/vscode/settings.rs b/crates/agent/src/provider/vscode/settings.rs index 5e0cfc21..982ff40f 100644 --- a/crates/agent/src/provider/vscode/settings.rs +++ b/crates/agent/src/provider/vscode/settings.rs @@ -489,8 +489,11 @@ pub(super) fn read_state(bytes: &[u8]) -> Option { /// /// Edits only when the program is set with `copilotChat: githubModels`, /// uses the gateway, the gateway has `proxyUrl` and the loopback proxy is -/// available; removes the managed keys otherwise (with a warning when the -/// proxy is the missing part), the same decision shape as `reconcile::plan`. +/// available; removes the managed keys otherwise, the same decision shape as +/// `reconcile::plan`. The one exception is the proxy being the missing part: +/// then `llmGateway.whenProxyUnavailable` decides, and `failClosed` (the default) leaves +/// the file as it is, so VS Code stays pointed at the loopback port +/// instead of reaching GitHub past the gateway. pub(super) fn plan( path: &Path, proxy: Option<(SocketAddr, &str)>, @@ -507,9 +510,18 @@ pub(super) fn plan( return remove(path, &state_path, proxy, plan); } let Some((listen, pairing)) = proxy else { + if let Some((_, Some(gateway))) = configured + && crate::reconcile::fail_closed_without_proxy(gateway, plan) + { + debug!( + path = %path.display(), + "programs.vscode uses copilotChat: githubModels but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the settings are left as they are" + ); + return Ok(()); + } tracing::debug!( path = %path.display(), - "programs.vscode uses copilotChat: githubModels but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); removing the managed settings" + "programs.vscode uses copilotChat: githubModels but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); whenProxyUnavailable: failOpen, removing the managed settings" ); plan.inactive(crate::reconcile::PROXY_ABSENT_REASON); return remove(path, &state_path, None, plan); diff --git a/crates/agent/src/provider/vscode/settings_tests.rs b/crates/agent/src/provider/vscode/settings_tests.rs index 61bed628..4ac245ed 100644 --- a/crates/agent/src/provider/vscode/settings_tests.rs +++ b/crates/agent/src/provider/vscode/settings_tests.rs @@ -7,7 +7,9 @@ use std::{collections::BTreeMap, fs, net::SocketAddr, path::PathBuf}; -use agentdesktop_core::config::{LlmGatewayConfig, VsCodeConfig, VsCodeCopilotChat, VsCodeModel}; +use agentdesktop_core::config::{ + LlmGatewayConfig, ProxyUnavailable, VsCodeConfig, VsCodeCopilotChat, VsCodeModel, +}; use jsonc_parser::{ParseOptions, cst::CstRootNode}; use serde_json::{Value, json}; @@ -15,6 +17,7 @@ use super::settings::{ Removal, SETTINGS_STATE_VERSION, SettingsConflict, SettingsState, edit_settings, plan, read_state, remove_settings, settings_path, }; +use crate::provider::json_merge; use crate::reconcile::ReconcilePlan; /// The two keys the daemon owns in `settings.json`. @@ -57,6 +60,7 @@ fn gateway_with_proxy() -> LlmGatewayConfig { authentication: None, proxy_url: Some("https://gateway.example.com/copilot-proxy".parse().unwrap()), github_oauth: None, + when_proxy_unavailable: Default::default(), } } @@ -365,12 +369,13 @@ fn removal_no_proxy_url_keeps_user_keys() { } #[test] -fn removal_no_proxy_available_keeps_user_keys_with_a_warning() { +fn fail_open_removal_no_proxy_available_keeps_user_keys() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("settings.json"); write_user_settings(&path, &json!({"editor.fontSize": 14})); let config = github_models_config(); - let gateway = gateway_with_proxy(); + let mut gateway = gateway_with_proxy(); + gateway.when_proxy_unavailable = ProxyUnavailable::FailOpen; apply_managed(&path, listen_addr(), PAIRING, &config, &gateway); // `ctx.llm_proxy` is `None` (bind failed, proxy off) even though the @@ -384,6 +389,82 @@ fn removal_no_proxy_available_keeps_user_keys_with_a_warning() { assert!(after.get(OVERRIDE_KEY).is_none(), "{after}"); } +#[test] +fn fail_closed_leaves_the_file_and_sidecar_until_the_proxy_is_back() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("settings.json"); + write_user_settings(&path, &json!({"editor.fontSize": 14})); + let config = github_models_config(); + let gateway = gateway_with_proxy(); + apply_managed(&path, listen_addr(), PAIRING, &config, &gateway); + let state = json_merge::state_path(&path); + let (before, state_before) = (fs::read(&path).unwrap(), fs::read(&state).unwrap()); + + let changes = apply_removal(&path, None, Some((&config, Some(&gateway)))); + assert_eq!( + changes.inactive_reason().as_deref(), + Some(crate::reconcile::FAIL_CLOSED_REASON) + ); + changes.apply().unwrap(); + assert_eq!( + fs::read(&path).unwrap(), + before, + "the override stays, pointed at the loopback port" + ); + assert_eq!(fs::read(&state).unwrap(), state_before); + + // The proxy back, on another port: the current URL is written. + let other: SocketAddr = "127.0.0.1:18199".parse().unwrap(); + apply_managed(&path, other, PAIRING, &config, &gateway); + assert_eq!(read(&path)[OVERRIDE_KEY], override_url(other, PAIRING)); + + // Removing the program still removes, whatever the policy. + apply_removal(&path, None, None).apply().unwrap(); + let after = read(&path); + assert!(after.get(OVERRIDE_KEY).is_none(), "{after}"); + assert_eq!(after["editor.fontSize"], 14); +} + +#[test] +fn fail_closed_does_not_create_a_file_without_the_proxy() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("settings.json"); + let changes = apply_removal( + &path, + None, + Some((&github_models_config(), Some(&gateway_with_proxy()))), + ); + changes.apply().unwrap(); + assert!(!path.exists()); +} + +#[test] +fn when_proxy_unavailable_defaults_to_fail_closed_and_parses_fail_open() { + let parse = |extra: Value| { + let mut document = json!({"url": "https://gateway.example.com"}); + document + .as_object_mut() + .unwrap() + .extend(extra.as_object().unwrap().clone()); + serde_json::from_value::(document) + }; + let default = parse(json!({})).unwrap(); + assert_eq!(default.when_proxy_unavailable, ProxyUnavailable::FailClosed); + assert!( + serde_json::to_value(&default) + .unwrap() + .get("whenProxyUnavailable") + .is_none() + ); + assert_eq!( + parse(json!({"whenProxyUnavailable": "failOpen"})) + .unwrap() + .when_proxy_unavailable, + ProxyUnavailable::FailOpen + ); + assert!(parse(json!({"whenProxyUnavailable": "failSoft"})).is_err()); +} + #[test] fn removal_of_a_created_and_emptied_file_deletes_it() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/agent/src/provider/vscode/tests.rs b/crates/agent/src/provider/vscode/tests.rs index f8439319..3ba8da70 100644 --- a/crates/agent/src/provider/vscode/tests.rs +++ b/crates/agent/src/provider/vscode/tests.rs @@ -3,7 +3,7 @@ use std::{collections::BTreeMap, fs, net::SocketAddr, path::PathBuf}; -use agentdesktop_core::config::{LlmGatewayConfig, VsCodeConfig, VsCodeModel}; +use agentdesktop_core::config::{LlmGatewayConfig, ProxyUnavailable, VsCodeConfig, VsCodeModel}; use serde_json::{Value, json}; use super::reconcile::{chat_models_path, managed_document, plan}; @@ -40,6 +40,7 @@ fn sample_gateway() -> LlmGatewayConfig { authentication: None, proxy_url: None, github_oauth: None, + when_proxy_unavailable: Default::default(), } } @@ -503,11 +504,12 @@ fn removal_when_use_llm_gateway_is_effectively_false() { } #[test] -fn removal_when_the_loopback_proxy_is_unavailable() { +fn fail_open_removes_the_entry_when_the_loopback_proxy_is_unavailable() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("chatLanguageModels.json"); let config = sample_config(); - let gateway = sample_gateway(); + let mut gateway = sample_gateway(); + gateway.when_proxy_unavailable = ProxyUnavailable::FailOpen; apply_managed(&path, listen_addr(), PAIRING, &config, &gateway); // `ctx.llm_proxy` is `None` (bind failed, proxy off, no pairing) even @@ -522,6 +524,27 @@ fn removal_when_the_loopback_proxy_is_unavailable() { ); } +#[test] +fn fail_closed_keeps_the_entry_when_the_loopback_proxy_is_unavailable() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("chatLanguageModels.json"); + let config = sample_config(); + let gateway = sample_gateway(); + apply_managed(&path, listen_addr(), PAIRING, &config, &gateway); + let state = super::super::json_merge::state_path(&path); + let (before, state_before) = (fs::read(&path).unwrap(), fs::read(&state).unwrap()); + + let changes = ReconcilePlan::default(); + plan(&path, None, Some((&config, Some(&gateway))), &changes).unwrap(); + assert_eq!( + changes.inactive_reason().as_deref(), + Some(crate::reconcile::FAIL_CLOSED_REASON) + ); + changes.apply().unwrap(); + assert_eq!(fs::read(&path).unwrap(), before); + assert_eq!(fs::read(&state).unwrap(), state_before); +} + #[test] fn removal_without_a_sidecar_strips_entries_that_carry_a_pairing_header() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/agent/src/reconcile/mod.rs b/crates/agent/src/reconcile/mod.rs index 3ca87d01..c0d1ae01 100644 --- a/crates/agent/src/reconcile/mod.rs +++ b/crates/agent/src/reconcile/mod.rs @@ -239,6 +239,26 @@ impl Reconciler { /// LLM proxy is absent. pub(crate) const PROXY_ABSENT_REASON: &str = "local LLM proxy not available; see llmProxy.error in daemon-info, or set daemon.llmProxy.listen"; +/// The `inactive` detail under `llmGateway.whenProxyUnavailable: failClosed`, +/// when a program behind the local LLM proxy keeps its managed entries. +pub(crate) const FAIL_CLOSED_REASON: &str = concat!( + "local LLM proxy not available; see llmProxy.error in daemon-info, or set daemon.llmProxy.listen", + "; the managed entries stay and the tool fails until the proxy is back (whenProxyUnavailable: failClosed)" +); + +/// Under `failClosed`, marks the program `inactive` and returns `true`: the +/// caller leaves its managed files as they are. +pub(crate) fn fail_closed_without_proxy( + gateway: &agentdesktop_core::config::LlmGatewayConfig, + plan: &ReconcilePlan, +) -> bool { + if gateway.when_proxy_unavailable != agentdesktop_core::config::ProxyUnavailable::FailClosed { + return false; + } + plan.inactive(FAIL_CLOSED_REASON); + true +} + /// A program's outcome, by precedence (highest first): Failed > Conflict > /// Blocked > Inactive > Applied/Removed > Unchanged. #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crates/agent/src/reconcile/plan.rs b/crates/agent/src/reconcile/plan.rs index f8092388..333a5b4f 100644 --- a/crates/agent/src/reconcile/plan.rs +++ b/crates/agent/src/reconcile/plan.rs @@ -82,6 +82,12 @@ impl ReconcilePlan { *self.inactive.borrow_mut() = Some(reason.to_owned()); } + /// The reason recorded by [`ReconcilePlan::inactive`], for tests. + #[cfg(test)] + pub(crate) fn inactive_reason(&self) -> Option { + self.inactive.borrow().clone() + } + pub(super) fn operation_count(&self) -> usize { self.operations.borrow().len() } diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index ffd5c6b0..36224983 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -252,6 +252,35 @@ pub struct LlmGatewayConfig { skip_serializing_if = "Option::is_none" )] pub github_oauth: Option, + /// What the programs behind the local LLM proxy (`programs.copilot`, + /// `programs.vscode`) do while the daemon runs without that proxy: + /// `failClosed` (the default) leaves their managed entries in place, so + /// the tools stay pointed at the loopback port and fail instead + /// of falling back to GitHub past the gateway; `failOpen` removes them + /// until the proxy is back (VS Code on `githubModels` then talks to GitHub + /// directly, the Copilot CLI and VS Code on `ownModels` lose the gateway's + /// models). + #[serde(default, skip_serializing_if = "ProxyUnavailable::is_default")] + pub when_proxy_unavailable: ProxyUnavailable, +} + +/// The `llmGateway.whenProxyUnavailable` policy. +#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize, PartialEq, Eq)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +#[serde(rename_all = "camelCase")] +pub enum ProxyUnavailable { + /// Keep the managed entries: the tools fail instead of bypassing the + /// gateway. + #[default] + FailClosed, + /// Remove the managed entries until the proxy is back. + FailOpen, +} + +impl ProxyUnavailable { + fn is_default(&self) -> bool { + *self == Self::default() + } } /// GitHub App user authorization for Copilot requests through the local proxy. diff --git a/examples/copilot/README.md b/examples/copilot/README.md new file mode 100644 index 00000000..6d11ba4c --- /dev/null +++ b/examples/copilot/README.md @@ -0,0 +1,199 @@ +# GitHub Copilot through Agentgateway + +This example points the GitHub Copilot CLI and VS Code Copilot Chat at a local +Agentgateway through Agentdesktop, without a controller. It is also the test +guide for the Copilot programs: each step says what to check. + +Agentdesktop runs a loopback LLM proxy (`daemon.llmProxy.listen`) and writes the +client files that point the tools at it. The proxy adds the developer's +identity (here a Dex OIDC token) to every request, and the gateway checks it. +Two paths go through the same gateway: + +| Path | Clients | Models | Billed by | +| --- | --- | --- | --- | +| The gateway's own models (`llm` in `agentgateway.yaml`) | Copilot CLI (`programs.copilot`), VS Code on own models (`programs.vscode`) | The models the gateway serves | Your provider key | +| GitHub's models (`copilot-proxy` route) | VS Code on GitHub's models (`copilotChat: githubModels`) | GitHub's models, as your Copilot plan and policy allow | The developer's Copilot seat | + +## Prerequisites + +- Linux with Docker Compose (the example was run on Ubuntu 24.04; macOS and + Windows paths are covered by unit tests only) +- `agentdesktop` on your `PATH` (see the repository README) +- An OpenAI API key for the gateway's own models +- For step 3: the Copilot CLI (`copilot`); for steps 4 and 5: VS Code with + GitHub Copilot Chat. The example was run with both signed in to GitHub with a + Copilot Business seat; without a sign-in it was not tried. Steps 3 and 4 + need the organisation's BYOK policy enabled (see + [github-side.md](github-side.md)); step 5 needs a Copilot Business or + Enterprise seat. +- A user account where no other Agentdesktop daemon runs or ran. A daemon + that ran before left sidecar files next to the files it managed; this + example's daemon removes what that earlier setup added (for example Claude + Code or OpenCode entries). Stop the other daemon first, and expect to + re-apply its configuration afterwards. +- Nothing else listening on 127.0.0.1:4001 (gateway), :4010 (the loopback + proxy), :5557 (Dex) and :51327 (the login callback) + +## 1. Start Dex and the gateway + +From the repository root: + +```sh +export OPENAI_API_KEY=sk-... +docker compose -f examples/copilot/compose.yaml up -d +curl -sI http://127.0.0.1:4001/ | head -1 # HTTP/1.1 200 OK +``` + +The gateway fetches Dex's keys at start and restarts until Dex answers: if the +check prints nothing, wait a few seconds and run it again +(`docker compose -f examples/copilot/compose.yaml ps` shows both services +`Up`). Dex has one user, `admin@example.com` with password `password`. + +## 2. Run the daemon + +```sh +agentdesktop daemon --config examples/copilot/config.yaml +``` + +`config.yaml` sets `daemon.user: true`, so `--user` is not needed. The daemon +stays in the foreground and logs there; run the checks below in a second +terminal, and stop it with Ctrl+C (later steps restart it this way). There is +no `--dry-run` preview for this example: the loopback proxy cannot run in a +one-shot run. + +The first start prints `Open this URL to connect Agentdesktop:` with +`http://127.0.0.1:51327/` and opens a browser; sign in to Dex there. Without a +browser on this machine, open that URL in a browser on the same machine; over +ssh, forward ports 51327 and 5557 first. Later restarts reuse the login. Then: + +```sh +agentdesktop --socket "$XDG_RUNTIME_DIR/agentdesktop.sock" status # ok +curl -s --unix-socket "$XDG_RUNTIME_DIR/agentdesktop.sock" http://localhost/v1/daemon-info +``` + +`llmProxy.bound` is `true` and `listen` is `127.0.0.1:4010`. The daemon log has +`applied reconciliation change` lines for `~/.copilot/providers.json` and the VS +Code `chatLanguageModels.json` (`~/.config/Code/User/`). Both files are +owner-only (600) and carry a pairing value that the proxy requires on every +request; other local users and browser pages cannot use the proxy without it. +The daemon keeps a small sidecar next to each file +(`.providers.json.agentdesktop`, `.chatLanguageModels.json.agentdesktop`) that +records what it added, so it can take out exactly that later. + +## 3. Copilot CLI on the gateway's models + +```sh +copilot -p "Reply with exactly one word: PONG" --model agentdesktop/gpt-4.1 +``` + +The answer comes from the gateway: its log (`docker compose -f +examples/copilot/compose.yaml logs agentgateway`) shows the request with +`user="admin@example.com"`. `~/.copilot/providers.json` holds the providers +`agentdesktop` (OpenAI-shaped) and `agentdesktop-anthropic`; your own providers +in that file are kept. The CLI reads the file at start: restart a running +session after the first apply. + +## 4. VS Code on the gateway's models + +Restart VS Code, open Copilot Chat and pick `gpt-4.1-mini (agentdesktop)` in the +model picker. A prompt answers through the gateway (same log line as above). +Your own entries in `chatLanguageModels.json` are kept. + +## 5. VS Code on GitHub's models (Copilot Business or Enterprise) + +Replace the `vscode` block in `config.yaml` with `copilotChat: githubModels` +(the comment there shows it) and restart the daemon (Ctrl+C, then the command +of step 2). Then: + +- The user `settings.json` gains `github.copilot.advanced.debug.overrideCapiUrl` + pointing at `http://127.0.0.1:4010/vscode-copilot-capi/` and two + entries in `settingsSync.ignoredSettings`, so Settings Sync does not carry the + override to other machines. Nothing else in the file changes: comments, key + order and formatting stay. The `agentdesktop` entry in + `chatLanguageModels.json` is removed. +- Restart VS Code. The picker shows GitHub's models (Auto, and the models your + organisation's policy enables). A chat turn answers; the gateway log shows + `POST /copilot-proxy/chat/completions` with status 200 and the Dex user, and + an agent-mode turn opens `GET /copilot-proxy/responses` with status 101 (the + gateway logs it when the tunnel closes). + +This step was run against Solo Enterprise for AgentGateway with the same route; +with the OSS gateway image of this example the route configuration is accepted +(`--validate-only`) but the step has not been run end to end yet. The route +targets `api.business.githubcopilot.com`. Other plans use another host, which +VS Code's Copilot Chat log shows (the `_ping` requests): change both +occurrences in `agentgateway.yaml` (`urlRewrite.authority.full` and the backend +`host`) and restart the gateway (`docker compose -f +examples/copilot/compose.yaml restart agentgateway`). + +While the daemon is not running, or runs without the proxy under the default +`llmGateway.whenProxyUnavailable: failClosed`, the override stays and Copilot +Chat fails instead of reaching GitHub past the gateway; with `failOpen` a +daemon without the proxy removes it and VS Code uses GitHub directly. The +failing requests still carry VS Code's Copilot tokens to the loopback port: +use this on single-user machines, and switch back to own models (or stop VS +Code) before removing the daemon. + +## 6. Drift, conflicts and removal + +Each item starts from the `config.yaml` of step 4 (own models); undo each +change before the next. + +- **Drift.** With `daemon.reconcileInterval` set (one minute in this example; + the tick is off when it is left out), delete `~/.copilot/providers.json`: + within one interval it is back with the same managed entries (your own + entries were in the deleted file and do not come back). `chmod 644` on a + managed file: back to 600. When nothing changed, the daemon writes nothing. +- **Conflict.** Put `// x` on the first line of `chatLanguageModels.json`. The + daemon refuses to rewrite a file it cannot parse and logs `program + configuration outcome` with `state="conflict"` and the path for `vscode`, and + `state="blocked"` (`not applied: vscode conflicted`) for the other programs + that had changes; it writes no managed file on the device until the comment + is gone (VS Code's `settings.json` is the exception that accepts comments and + trailing commas). Remove the comment: within one interval everything applies + again (without the tick: at the next restart). +- **Proxy gone.** Remove `daemon.llmProxy` and restart: the log says + `state="inactive"` with the reason, and the Copilot and VS Code entries stay + (`llmGateway.whenProxyUnavailable: failClosed`, the default), so the CLI's + `agentdesktop/gpt-4.1` fails instead of answering. Add + `whenProxyUnavailable: failOpen` under `llmGateway` in `config.yaml` and + restart: the entries are removed. +- **Removal.** Remove a program from `programs` and restart: its entries are + taken out, your own entries stay, and a file the daemon created is deleted + when nothing else is left. + +## 7. With a controller + +The same programs can be delivered by the controller instead of the local +file; see the managed quickstart in the repository README. The controller's +device page then lists each managed program with its outcome (applied, +unchanged, removed, conflict, inactive, blocked, failed), and `GET +/api/v1/devices/{id}` returns the same list. With a controller, the gateway +checks the controller's short-lived JWT (issuer `agentdesktop-controller`) +instead of Dex, and the proxy uses the client IDs `copilot-cli` and +`vscode-copilot`, which must be in `llmGateway.authentication.allowedClientIds`. + +## 8. GitHub-side settings + +The Copilot plan, the BYOK policy, model policies, default models and egress +rules are set by the GitHub organisation or enterprise owner; see +[github-side.md](github-side.md). + +## 9. Clean up + +Stop the daemon (Ctrl+C). To take the managed entries out, remove `programs` +from `config.yaml` and start the daemon once more (or delete the files if the +daemon created them). Then `docker compose -f examples/copilot/compose.yaml +down`, and remove `~/.local/state/agentdesktop` for a fresh login next time. +Dex keeps its data in memory, so after `down` and `up` the daemon needs a new +login. + +## Known limits + +- Inline completions and next edit suggestions go directly to GitHub; this + example routes chat and agent mode only. +- `github.copilot.advanced.debug.overrideCapiUrl` is an undocumented VS Code + setting; the fallback is VS Code's HTTP proxy setting pointed at the gateway. +- Tested on Linux with VS Code 1.139, Copilot CLI 1.0.88 and a Copilot Business + seat: steps 1 to 4 and 6 with this example's OSS gateway; step 5 with Solo + Enterprise for AgentGateway. diff --git a/examples/copilot/agentgateway.yaml b/examples/copilot/agentgateway.yaml new file mode 100644 index 00000000..90470e87 --- /dev/null +++ b/examples/copilot/agentgateway.yaml @@ -0,0 +1,81 @@ +# yaml-language-server: $schema=https://agentgateway.dev/schema/config +# Two paths for GitHub Copilot through one gateway: +# - llm: the gateway's own models (Copilot CLI, VS Code on own models), billed by your provider key +# - routes/copilot-proxy: GitHub's models (VS Code on GitHub's models), the developer's Copilot +# token passed through, billed by the Copilot seat +# Both require the developer's Dex token, which Agentdesktop adds on the loopback proxy. +frontendPolicies: + logging: + add: + user: jwt.email + +gateways: + gateway: + port: 4001 + +routes: +- name: reachability + gateways: gateway + matches: + - method: HEAD + path: + exact: / + policies: + directResponse: + status: 200 + +# llmGateway.proxyUrl points here. Agentdesktop sends the developer's identity in +# Authorization and VS Code's own Copilot token in x-llm-token; the route checks the +# identity, restores the Copilot token as the bearer and forwards to GitHub. +- name: copilot-proxy + gateways: gateway + matches: + - path: + pathPrefix: /copilot-proxy + policies: + jwtAuth: + mode: strict + issuer: http://127.0.0.1:5557/dex + audiences: [agentdesktop-local] + jwks: + url: http://dex:5557/dex/keys + urlRewrite: + authority: + # Copilot Business; other plans use another host (VS Code's Copilot Chat log shows it) + full: api.business.githubcopilot.com + path: + prefix: / + timeout: + requestTimeout: 120s + backends: + - host: api.business.githubcopilot.com:443 + policies: + backendTLS: + alpn: [http/1.1] # the WebSocket upgrade VS Code uses cannot travel over HTTP/2 + backendAuth: + passthrough: {} + transformations: + request: + set: + authorization: '"Bearer " + request.headers["x-llm-token"]' + remove: + - x-llm-token + +llm: + gateways: gateway + policies: + jwtAuth: + mode: strict + issuer: http://127.0.0.1:5557/dex + audiences: [agentdesktop-local] + jwks: + url: http://dex:5557/dex/keys + models: + - name: "gpt-4.1-mini" + provider: openai + params: + apiKey: $OPENAI_API_KEY + - name: "gpt-4.1" + provider: openai + params: + apiKey: $OPENAI_API_KEY diff --git a/examples/copilot/compose.yaml b/examples/copilot/compose.yaml new file mode 100644 index 00000000..b82fc609 --- /dev/null +++ b/examples/copilot/compose.yaml @@ -0,0 +1,23 @@ +# Dex (OIDC for the developer) and Agentgateway (the LLM gateway) for the Copilot example. +services: + dex: + image: ghcr.io/dexidp/dex:v2.45.1 + command: dex serve /etc/dex/config.yaml + ports: + - 127.0.0.1:5557:5557 + volumes: + - ./dex.yaml:/etc/dex/config.yaml:ro + + agentgateway: + image: cr.agentgateway.dev/agentgateway:v1.4.1 + command: -f /etc/agentgateway/config.yaml + depends_on: + - dex + # The gateway fetches Dex's keys at start; retry until Dex is up. + restart: on-failure + ports: + - 127.0.0.1:4001:4001 + environment: + OPENAI_API_KEY: + volumes: + - ./agentgateway.yaml:/etc/agentgateway/config.yaml:ro diff --git a/examples/copilot/config.yaml b/examples/copilot/config.yaml new file mode 100644 index 00000000..6ef39341 --- /dev/null +++ b/examples/copilot/config.yaml @@ -0,0 +1,39 @@ +# yaml-language-server: $schema=../../schema/daemon-config.json +# Agentdesktop daemon configuration for the Copilot example (user mode, no controller). +llmGateway: + url: http://127.0.0.1:4001 + # The pass-through route for GitHub's models (VS Code copilotChat: githubModels). + proxyUrl: http://127.0.0.1:4001/copilot-proxy + authentication: + type: oidc + issuer: http://127.0.0.1:5557/dex + clientId: agentdesktop-local + scopes: [openid, email, offline_access] + allowInsecure: true + +daemon: + user: true + llmProxy: + # The loopback proxy the managed client files point at. + listen: 127.0.0.1:4010 + # Opt-in: re-apply every minute, so deleted or edited managed files come back and fixed + # conflicts apply. Leave it out for no periodic re-apply. + reconcileInterval: 1m + +programs: + # GitHub Copilot CLI: ~/.copilot/providers.json gains the providers "agentdesktop" and + # "agentdesktop-anthropic"; run `copilot --model agentdesktop/gpt-4.1`. + copilot: + models: + gpt-4.1: {} + # VS Code Copilot Chat on the gateway's models: chatLanguageModels.json gains the vendor + # entry "agentdesktop"; the picker shows "gpt-4.1-mini (agentdesktop)". + vscode: + models: + gpt-4.1-mini: + maxInputTokens: 128000 + maxOutputTokens: 16000 + # VS Code Copilot Chat on GitHub's models through the gateway instead (Copilot Business or + # Enterprise seat): replace the vscode block above with + # vscode: + # copilotChat: githubModels diff --git a/examples/copilot/dex.yaml b/examples/copilot/dex.yaml new file mode 100644 index 00000000..76f4d1ff --- /dev/null +++ b/examples/copilot/dex.yaml @@ -0,0 +1,24 @@ +issuer: http://127.0.0.1:5557/dex + +storage: + type: memory + +web: + http: 0.0.0.0:5557 + +oauth2: + skipApprovalScreen: true + +staticClients: +- id: agentdesktop-local + name: Agentdesktop Copilot example + public: true + redirectURIs: + - http://127.0.0.1:51327/callback + +enablePasswordDB: true +staticPasswords: +- email: admin@example.com + username: admin + userID: local-admin + hash: $2y$10$cA5xff5AaaAoxB8VZjwXUeXZIlH8V0sLf/JkIBXHip6gnQsyKJiHW diff --git a/examples/copilot/github-side.md b/examples/copilot/github-side.md new file mode 100644 index 00000000..09ae3493 --- /dev/null +++ b/examples/copilot/github-side.md @@ -0,0 +1,37 @@ +# GitHub Copilot: what the GitHub organisation or enterprise owner configures + +Agentdesktop points the Copilot CLI and VS Code Copilot Chat at the local LLM proxy, and the proxy sends their traffic through the gateway with the developer's identity attached. Some settings that decide whether that traffic flows, and whether developers can go around it, live on the GitHub side and belong to your GitHub organisation or enterprise owner and your network team. This page lists them per client mode. + +## The two VS Code modes and the Copilot CLI + +| | Copilot CLI (`programs.copilot`) | VS Code, own models (`copilotChat: ownModels`) | VS Code, GitHub's models (`copilotChat: githubModels`) | +|---|---|---|---| +| Models | The gateway's models | The gateway's models | GitHub's models, as the Copilot plan and model policy allow | +| Billed by | Your model provider, per token | Your model provider, per token | The Copilot seat (included AI credits, then metered) | +| GitHub BYOK policy | Not verified whether it applies | Must stay enabled | Not needed | +| Gateway sees | The user; with a controller also device and client; full request | The user; with a controller also device and client; full request | The user; with a controller also device and client; the request as VS Code sends it to GitHub | + +## Settings on the GitHub side + +| Item | What to set | Applies to | Verified | +|---|---|---|---| +| Copilot plan | Business or Enterprise seats. BYOK is admin-controllable only on these plans. | all | Business seat, tested | +| BYOK policy | "Bring Your Own Language Model Key in select IDEs" must stay enabled, otherwise VS Code's custom endpoint is switched off for every seat. Whether the Copilot CLI's BYOK follows the same policy is not verified. | own models, CLI | GitHub docs | +| Model availability policy (optional, enforcement) | Hosted chat models "Disabled for every user and agent app" leaves only the gateway models in the VS Code picker, where GitHub's models are greyed out with "Contact your admin". The CLI still offered gpt-4.1 in the lab with the enterprise's default availability disabled; the base model's own status is undocumented. Do not use with `githubModels`, which needs GitHub's models enabled. | own models, CLI | tested | +| CLI default model | Managed settings `{"model": "agentdesktop/"}`: Linux `/etc/github-copilot/managed-settings.json`, macOS `/Library/Application Support/GitHubCopilot/`, Windows `%ProgramFiles%\GitHubCopilot\`. Precedence: native MDM, then server-managed (`.github-private` repository), then the file. A default only: the user's own `config.json` overrides it. Managed settings have no provider, URL or credential keys. | CLI | GitHub docs, tested | +| VS Code default model | `chat.defaultModel` (also the `ChatDefaultModel` enterprise policy), for example `customendpoint/agentdesktop/gpt-4.1-mini`. A default only. Agentdesktop does not write it. | own models | GitHub docs, tested | +| Egress deny (optional, enforcement) | Deny `api.githubcopilot.com`, `api.{individual,business,enterprise}.githubcopilot.com` and the model providers' hosts from developer machines; allow the gateway, `api.github.com`, and the completion hosts `proxy..githubcopilot.com` and `copilot-proxy.githubusercontent.com`. With `githubModels`, the gateway (not the device) reaches `api..githubcopilot.com`, so the deny on devices still holds. Roll out only after the fleet view shows every device configured. | all | GitHub docs, tested | +| Seat-side observability | The Copilot usage metrics API stays the source for completions and hosted-model use; the gateway sees the traffic routed through it. | all | GitHub docs | +| Not needed | Enterprise custom models ("Editor preview features" policy). | | | + +## What each mode leaves outside the gateway + +- **Inline completions and next edit suggestions** go straight to `proxy..githubcopilot.com` in every mode; Agentdesktop does not route them. +- **GitHub's models mode** relies on `github.copilot.advanced.debug.overrideCapiUrl`, an undocumented Copilot Chat setting. If a VS Code release drops it, the fallback is VS Code's HTTP proxy setting with the gateway as the proxy. +- **Defaults are not enforcement.** A developer can change the default model or remove the managed file entries; only the model policy and egress rules above make the gateway path non-optional. + +## Not verified + +- Whether the Copilot CLI's BYOK is gated by the VS Code BYOK policy. +- The Enterprise Cloud host (`api.enterprise.githubcopilot.com`) for `githubModels`; tested with a Business seat only. +- macOS and Windows (firewall prompts for the loopback listener, paths beyond unit tests); corporate proxies (`NO_PROXY` for 127.0.0.1). diff --git a/frontend/controller/src/stories/fixtures.ts b/frontend/controller/src/stories/fixtures.ts index bcac53d6..d69e7f2f 100644 --- a/frontend/controller/src/stories/fixtures.ts +++ b/frontend/controller/src/stories/fixtures.ts @@ -354,6 +354,19 @@ export const activeDaemonConfig: DaemonConfigDocument = { }, }; +export const copilotFailOpenDaemonConfig: DaemonConfigDocument = { + llmGateway: { + url: "https://gateway.example.internal", + proxyUrl: "https://gateway.example.internal/copilot-proxy", + whenProxyUnavailable: "failOpen", + authentication: { type: "controllerJwt", audience: "agentgateway" }, + }, + programs: { + copilot: { models: { "gpt-4.1": {} } }, + vscode: { copilotChat: "githubModels" }, + }, +}; + export const sandboxDaemonConfig: DaemonConfigDocument = { sandbox: { network: { diff --git a/frontend/controller/src/styles.css b/frontend/controller/src/styles.css index abd2d664..f0f22543 100644 --- a/frontend/controller/src/styles.css +++ b/frontend/controller/src/styles.css @@ -886,6 +886,7 @@ code { .toggle-row input { width: 16px; height: 16px; + flex-shrink: 0; accent-color: var(--accent-button); } diff --git a/frontend/controller/src/types.ts b/frontend/controller/src/types.ts index 60e103a1..6a152abe 100644 --- a/frontend/controller/src/types.ts +++ b/frontend/controller/src/types.ts @@ -116,6 +116,7 @@ export type DaemonConfigDocument = { llmGateway?: { url: string; proxyUrl?: string; + whenProxyUnavailable?: "failClosed" | "failOpen"; authentication?: { type: string; audience?: string; diff --git a/frontend/controller/src/views/ConfigurationView.stories.tsx b/frontend/controller/src/views/ConfigurationView.stories.tsx index 5c56bbe2..8009e356 100644 --- a/frontend/controller/src/views/ConfigurationView.stories.tsx +++ b/frontend/controller/src/views/ConfigurationView.stories.tsx @@ -2,7 +2,11 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; import { expect, fn, userEvent } from "storybook/test"; import { ControllerStoryFrame } from "../stories/ControllerStoryFrame"; -import { activeDaemonConfig, sandboxDaemonConfig } from "../stories/fixtures"; +import { + activeDaemonConfig, + copilotFailOpenDaemonConfig, + sandboxDaemonConfig, +} from "../stories/fixtures"; import { ConfigurationView } from "./ConfigurationView"; const meta = { @@ -52,6 +56,31 @@ export const ActiveSandbox: Story = { }, }; +export const CopilotFailOpen: Story = { + args: { initialConfig: copilotFailOpenDaemonConfig }, + play: async ({ canvas, canvasElement }) => { + const toggle = canvas.getByRole("checkbox", { + name: /Fail open without the local proxy/, + }); + await expect(toggle).toBeChecked(); + const output = canvasElement.querySelector(".output-card code"); + await expect(output).toHaveTextContent("whenProxyUnavailable: failOpen"); + await userEvent.click(toggle); + await expect(output).not.toHaveTextContent("whenProxyUnavailable"); + }, +}; + +export const NoProxyPolicyWithoutCopilot: Story = { + args: { initialConfig: activeDaemonConfig }, + play: async ({ canvas }) => { + await expect( + canvas.queryByRole("checkbox", { + name: /Fail open without the local proxy/, + }), + ).toBeNull(); + }, +}; + export const GeneratedYamlScrolls: Story = { args: { initialConfig: activeDaemonConfig }, play: async ({ canvasElement }) => { diff --git a/frontend/controller/src/views/ConfigurationView.tsx b/frontend/controller/src/views/ConfigurationView.tsx index 351fa0a3..18708bb6 100644 --- a/frontend/controller/src/views/ConfigurationView.tsx +++ b/frontend/controller/src/views/ConfigurationView.tsx @@ -25,6 +25,7 @@ export function ConfigurationView({ const [gateway, setGateway] = useState(true); const [gatewayUrl, setGatewayUrl] = useState("https://gateway.example.com"); const [gatewayProxyUrl, setGatewayProxyUrl] = useState(""); + const [proxyFailOpen, setProxyFailOpen] = useState(false); const [controllerJwt, setControllerJwt] = useState(true); const [audience, setAudience] = useState("agentgateway"); const [sessionNewTelemetry, setSessionNewTelemetry] = useState(false); @@ -46,10 +47,12 @@ export function ConfigurationView({ return [definition?.label ?? agent.kind]; }); const sandboxUnavailable = incompatibleSandboxAgentNames.length > 0; + const usesLocalProxy = agents.some((agent) => proxyAgents.has(agent.kind)); const yaml = daemonConfigYaml({ gateway, gatewayUrl, gatewayProxyUrl, + proxyFailOpen: proxyFailOpen && usesLocalProxy, controllerJwt, audience, sandboxEnabled, @@ -78,6 +81,7 @@ export function ConfigurationView({ if (llmGateway) { setGatewayUrl(llmGateway.url); setGatewayProxyUrl(llmGateway.proxyUrl ?? ""); + setProxyFailOpen(llmGateway.whenProxyUnavailable === "failOpen"); setControllerJwt(llmGateway.authentication?.type === "controllerJwt"); setAudience(llmGateway.authentication?.audience ?? "agentgateway"); } @@ -195,6 +199,25 @@ export function ConfigurationView({ GitHub's models (copilotChat: githubModels). + {usesLocalProxy && ( + + )}