fix(service): unescape plist XML, surface both launchctl streams, clear status fields when not installed

Three review findings on task 4:
- read_plist_fields now parses the plist properly via the plist crate
  (already a transitive dep of service-manager, promoted to direct) instead
  of slicing raw XML, so a PATH or program path containing & or < no longer
  round-trips as literal &amp;/&lt; through cli::service::render_status.
- launchctl's error path now includes both stdout and stderr, trimmed and
  joined only on non-empty parts, so a failure never surfaces as
  "launchctl load failed: " with nothing after the colon.
- status() now returns identical None fields (program, path_env,
  snapshotted, pid) whether the plist is absent or the plist exists but the
  crate reports NotInstalled (e.g. written but never loaded, or booted out
  of band) - extracted via a shared not_installed_status/build_status split
  so both paths run the same code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGntTHCW3sEPy1VBRopNNp

Cargo.lock

@@ -1348,6 +1348,7 @@ dependencies = [
  "etcetera",
  "humantime",
  "nix",
+ "plist",
  "serde",
  "serde_json",
  "service-manager",

Cargo.toml

@@ -34,3 +34,4 @@ async-trait = "0.1"
 tempfile = "3"
 tokio-test = "0.4"
 service-manager = "0.11"
+plist = "1"

crates/xy/Cargo.toml

@@ -31,6 +31,7 @@ etcetera.workspace = true
 nix.workspace = true
 humantime.workspace = true
 service-manager = { workspace = true }
+plist = { workspace = true }
 
 [dev-dependencies]
 tempfile.workspace = true

crates/xy/src/service.rs

@@ -139,33 +139,27 @@ pub(crate) fn read_pid(pidfile: &Path) -> Option<u32> {
         .ok()
 }
 
-pub(crate) fn status(label: &str, pidfile: &Path) -> Result<AgentStatus> {
-    let plist = plist_path_for(label)?;
+fn not_installed_status(label: &str, plist: PathBuf) -> AgentStatus {
+    AgentStatus {
+        label: label.to_string(),
+        plist,
+        state: AgentState::NotInstalled,
+        program: None,
+        path_env: None,
+        snapshotted: None,
+        pid: None,
+    }
+}
 
-    if !plist.exists() {
-        return Ok(AgentStatus {
-            label: label.to_string(),
-            plist,
-            state: AgentState::NotInstalled,
-            program: None,
-            path_env: None,
-            snapshotted: None,
-            pid: None,
-        });
+fn build_status(label: &str, plist: PathBuf, pidfile: &Path, state: AgentState) -> AgentStatus {
+    if state == AgentState::NotInstalled {
+        return not_installed_status(label, plist);
     }
 
     let snapshotted = std::fs::metadata(&plist)
         .and_then(|meta| meta.modified())
         .ok();
 
-    let parsed: ServiceLabel = label.parse().context("parse service label")?;
-
-    let state = match manager().status(ServiceStatusCtx { label: parsed })? {
-        ServiceStatus::Running => AgentState::Running,
-        ServiceStatus::Stopped(_) => AgentState::Stopped,
-        ServiceStatus::NotInstalled => AgentState::NotInstalled,
-    };
-
     let pid = if state == AgentState::Running {
         read_pid(pidfile)
     } else {
@@ -174,7 +168,7 @@ pub(crate) fn status(label: &str, pidfile: &Path) -> Result<AgentStatus> {
 
     let (program, path_env) = read_plist_fields(&plist);
 
-    Ok(AgentStatus {
+    AgentStatus {
         label: label.to_string(),
         plist,
         state,
@@ -182,26 +176,46 @@ pub(crate) fn status(label: &str, pidfile: &Path) -> Result<AgentStatus> {
         path_env,
         snapshotted,
         pid,
-    })
+    }
+}
+
+pub(crate) fn status(label: &str, pidfile: &Path) -> Result<AgentStatus> {
+    let plist = plist_path_for(label)?;
+
+    if !plist.exists() {
+        return Ok(not_installed_status(label, plist));
+    }
+
+    let parsed: ServiceLabel = label.parse().context("parse service label")?;
+
+    let state = match manager().status(ServiceStatusCtx { label: parsed })? {
+        ServiceStatus::Running => AgentState::Running,
+        ServiceStatus::Stopped(_) => AgentState::Stopped,
+        ServiceStatus::NotInstalled => AgentState::NotInstalled,
+    };
+
+    Ok(build_status(label, plist, pidfile, state))
 }
 
 fn read_plist_fields(plist: &Path) -> (Option<PathBuf>, Option<String>) {
-    let Ok(contents) = std::fs::read_to_string(plist) else {
+    let Ok(value) = plist::Value::from_file(plist) else {
         return (None, None);
     };
 
-    let program = contents
-        .split("<key>ProgramArguments</key>")
-        .nth(1)
-        .and_then(|rest| rest.split("<string>").nth(1))
-        .and_then(|rest| rest.split("</string>").next())
+    let dict = value.as_dictionary();
+
+    let program = dict
+        .and_then(|dict| dict.get("ProgramArguments"))
+        .and_then(plist::Value::as_array)
+        .and_then(|args| args.first())
+        .and_then(plist::Value::as_string)
         .map(PathBuf::from);
 
-    let path_env = contents
-        .split("<key>PATH</key>")
-        .nth(1)
-        .and_then(|rest| rest.split("<string>").nth(1))
-        .and_then(|rest| rest.split("</string>").next())
+    let path_env = dict
+        .and_then(|dict| dict.get("EnvironmentVariables"))
+        .and_then(plist::Value::as_dictionary)
+        .and_then(|env| env.get("PATH"))
+        .and_then(plist::Value::as_string)
         .map(str::to_string);
 
     (program, path_env)
@@ -215,15 +229,24 @@ fn launchctl(verb: &str, plist: &Path) -> Result<()> {
         .with_context(|| format!("run launchctl {verb}"))?;
 
     if !output.status.success() {
-        anyhow::bail!(
-            "launchctl {verb} failed: {}",
-            String::from_utf8_lossy(&output.stderr).trim()
-        );
+        return Err(launchctl_error(verb, &output.stdout, &output.stderr));
     }
 
     Ok(())
 }
 
+fn launchctl_error(verb: &str, stdout: &[u8], stderr: &[u8]) -> anyhow::Error {
+    let stdout = String::from_utf8_lossy(stdout);
+    let stderr = String::from_utf8_lossy(stderr);
+
+    let detail: Vec<&str> = [stdout.trim(), stderr.trim()]
+        .into_iter()
+        .filter(|part| !part.is_empty())
+        .collect();
+
+    anyhow::anyhow!("launchctl {verb} failed: {}", detail.join("; "))
+}
+
 pub(crate) fn start(label: &str) -> Result<()> {
     launchctl("load", &plist_path_for(label)?)
 }
@@ -350,4 +373,102 @@ mod tests {
         assert_eq!(read_pid(&missing), None);
         assert_eq!(read_pid(&garbage), None);
     }
+
+    fn write_agent_plist(path: &Path, program: &str, path_env: Option<&str>) {
+        let mut dict = plist::Dictionary::new();
+
+        dict.insert(
+            "ProgramArguments".to_string(),
+            plist::Value::Array(vec![plist::Value::String(program.to_string())]),
+        );
+
+        if let Some(path_env) = path_env {
+            let mut env = plist::Dictionary::new();
+            env.insert(
+                "PATH".to_string(),
+                plist::Value::String(path_env.to_string()),
+            );
+
+            dict.insert(
+                "EnvironmentVariables".to_string(),
+                plist::Value::Dictionary(env),
+            );
+        }
+
+        plist::Value::Dictionary(dict).to_file_xml(path).unwrap();
+    }
+
+    #[test]
+    fn read_plist_fields_unescapes_xml_entities() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("agent.plist");
+
+        write_agent_plist(
+            &plist_path,
+            "/usr/local/bin/xy",
+            Some("/usr/bin:/opt/a&b:/opt/<c>"),
+        );
+
+        let (program, path_env) = read_plist_fields(&plist_path);
+
+        assert_eq!(program, Some(PathBuf::from("/usr/local/bin/xy")));
+        assert_eq!(path_env, Some("/usr/bin:/opt/a&b:/opt/<c>".to_string()));
+    }
+
+    #[test]
+    fn read_plist_fields_on_malformed_plist_returns_none_without_panicking() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("broken.plist");
+
+        std::fs::write(&plist_path, "<plist><dict><key>ProgramArgum").unwrap();
+
+        let (program, path_env) = read_plist_fields(&plist_path);
+
+        assert!(program.is_none());
+        assert!(path_env.is_none());
+    }
+
+    #[test]
+    fn launchctl_error_includes_both_streams_when_populated() {
+        let err = launchctl_error(
+            "unload",
+            b"out text",
+            b"Unload failed: 5: Input/output error",
+        );
+
+        let message = err.to_string();
+
+        assert!(message.contains("out text"));
+        assert!(message.contains("Unload failed: 5: Input/output error"));
+    }
+
+    #[test]
+    fn launchctl_error_omits_dangling_separator_when_stdout_empty() {
+        let err = launchctl_error("load", b"", b"boom");
+
+        assert_eq!(err.to_string(), "launchctl load failed: boom");
+    }
+
+    #[test]
+    fn not_installed_state_clears_all_optional_fields_even_when_plist_exists() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("agent.plist");
+
+        write_agent_plist(&plist_path, "/usr/local/bin/xy", Some("/usr/bin"));
+
+        let pidfile = tmp.path().join("xy.pid");
+        std::fs::write(&pidfile, "4821").unwrap();
+
+        let status = build_status(
+            "se.aceofba.xy-ghost",
+            plist_path,
+            &pidfile,
+            AgentState::NotInstalled,
+        );
+
+        assert!(status.program.is_none());
+        assert!(status.path_env.is_none());
+        assert!(status.snapshotted.is_none());
+        assert!(status.pid.is_none());
+    }
 }