Apply review feedback on XDG paths

Ignore relative XDG_* values and reject a relative HOME, per the XDG
spec; add a hint to the missing-config error; tighten docs and comments.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

docs/SPEC.md

@@ -341,9 +341,16 @@ Open questions on the schema:
   | Last-published / pending versions | `~/.local/state/pkgwatch` | `XDG_STATE_HOME` | `PKGWATCH_STATE_DIR` |
   | Downloads and build trees (safe to delete) | `~/.cache/pkgwatch` | `XDG_CACHE_HOME` | `PKGWATCH_WORK_DIR` |
 
-  The overrides are used verbatim (no `pkgwatch/` suffix) and exist for
+  Precedence per directory: override, then the XDG variable, then the
-  dry runs against scratch directories, like `PKGWATCH_REPO_DIR` does for
+  default under `$HOME`; an empty variable counts as unset. The overrides
-  the pacman repo. Migrating from the old cwd-relative layout: move
+  are used verbatim (no `pkgwatch/` suffix) and exist for dry runs against
+  scratch directories, like `PKGWATCH_REPO_DIR` does for the pacman repo.
+  The XDG variables and `$HOME` must be absolute paths: a relative XDG
+  value is ignored, as the XDG spec requires, and a relative `$HOME` is an
+  error.
+
+  The checkout's `packages.d/` is no longer read on its own; it's just the
+  source to link from. Migrating from the old cwd-relative layout: move
   `state/` to the state dir and copy or symlink `packages.d/` into the
   config dir; `work/` is cache and can simply be dropped.
 - **Checker**: per source type, resolves "what's the latest version" —

src/paths.rs

@@ -1,6 +1,7 @@
 //! Decides where pkgwatch's config, state, and work directories live on
-//! disk. The only module that reads the environment to answer that — every
+//! disk — the only module that reads the environment to answer that; every
-//! other module takes the directories it needs as parameters.
+//! other module takes the directories it needs as parameters. (The pacman
+//! repo dir, `pipeline::custom_repo_dir`, is resolved separately.)
 //!
 //! These follow the XDG base-directory spec instead of the current working
 //! directory, so an installed `/usr/bin/pkgwatch` behaves the same
@@ -12,7 +13,7 @@ use std::path::{Path, PathBuf};
 
 const APP_DIR: &str = "pkgwatch";
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct Paths {
     /// `*.toml` package declarations. Config: hand-edited, worth backing up.
     pub packages_dir: PathBuf,
@@ -32,18 +33,21 @@ impl Paths {
     /// environment, which would race with `cargo test`'s parallel threads.
     ///
     /// Each directory has a pkgwatch-specific override, then the matching
-    /// XDG variable, then the XDG default under `$HOME`. The overrides exist
+    /// XDG variable, then the XDG default under `$HOME`; empty counts as
-    /// for dry runs against scratch directories, like `PKGWATCH_REPO_DIR`
+    /// unset. The overrides exist for dry runs against scratch
-    /// does for the pacman repo.
+    /// directories, like `PKGWATCH_REPO_DIR` does for the pacman repo, so
+    /// they're used verbatim. The XDG variables and `$HOME` must be
+    /// absolute: the spec says to ignore a relative XDG value, and honoring
+    /// one would bring back the cwd dependence this module exists to remove.
     fn resolve(getenv: impl Fn(&str) -> Option<String>) -> Result<Self> {
         let base = |override_var: &str, xdg_var: &str, home_subpath: &str| -> Result<PathBuf> {
             if let Some(dir) = non_empty(&getenv, override_var) {
                 return Ok(PathBuf::from(dir));
             }
-            if let Some(dir) = non_empty(&getenv, xdg_var) {
+            if let Some(dir) = absolute(&getenv, xdg_var) {
                 return Ok(Path::new(&dir).join(APP_DIR));
             }
-            let home = non_empty(&getenv, "HOME").context("HOME is not set")?;
+            let home = absolute(&getenv, "HOME").context("HOME is not set to an absolute path")?;
             Ok(Path::new(&home).join(home_subpath).join(APP_DIR))
         };
 
@@ -61,6 +65,12 @@ fn non_empty(getenv: &impl Fn(&str) -> Option<String>, key: &str) -> Option<Stri
     getenv(key).filter(|v| !v.is_empty())
 }
 
+/// Like `non_empty`, but also drops relative values (an empty string isn't
+/// absolute either, so this subsumes the empty check).
+fn absolute(getenv: &impl Fn(&str) -> Option<String>, key: &str) -> Option<String> {
+    getenv(key).filter(|v| Path::new(v).is_absolute())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -132,6 +142,29 @@ mod tests {
         assert_eq!(paths.work_dir, Path::new("/home/u/.cache/pkgwatch"));
     }
 
+    #[test]
+    fn relative_xdg_variables_are_ignored() {
+        let paths = Paths::resolve(env(&[
+            ("HOME", "/home/u"),
+            ("XDG_CONFIG_HOME", "rel/config"),
+            ("XDG_STATE_HOME", "./state"),
+            ("XDG_CACHE_HOME", "cache"),
+        ]))
+        .unwrap();
+        assert_eq!(
+            paths.packages_dir,
+            Path::new("/home/u/.config/pkgwatch/packages.d")
+        );
+        assert_eq!(paths.state_dir, Path::new("/home/u/.local/state/pkgwatch"));
+        assert_eq!(paths.work_dir, Path::new("/home/u/.cache/pkgwatch"));
+    }
+
+    #[test]
+    fn relative_home_is_an_error() {
+        let err = Paths::resolve(env(&[("HOME", "relative/home")])).unwrap_err();
+        assert!(err.to_string().contains("HOME"));
+    }
+
     #[test]
     fn errors_when_nothing_locates_home() {
         let err = Paths::resolve(env(&[])).unwrap_err();

src/pipeline.rs

@@ -42,6 +42,19 @@ fn custom_repo_dir() -> Result<PathBuf> {
     Ok(Path::new(&home).join(CUSTOM_REPO_SUBPATH))
 }
 
+/// Adds a hint to the bare "No such file" a missing config dir would give:
+/// the dir is no longer relative to the cwd, so a checkout's `packages.d/`
+/// isn't picked up on its own (see docs/SPEC.md > Paths).
+fn load_packages(packages_dir: &Path) -> Result<Vec<(String, Package)>> {
+    config::load_packages_dir(packages_dir).with_context(|| {
+        format!(
+            "no package config at {} (set PKGWATCH_CONFIG_DIR to the directory containing \
+             packages.d, or see docs/SPEC.md > Paths for moving a checkout's packages.d/ there)",
+            packages_dir.display()
+        )
+    })
+}
+
 pub fn run_check() -> Result<()> {
     let client = build_client()?;
     let endpoints = GithubEndpoints::default();
@@ -51,7 +64,7 @@ pub fn run_check() -> Result<()> {
         work_dir,
     } = Paths::from_env()?;
 
-    let packages = config::load_packages_dir(&packages_dir)?;
+    let packages = load_packages(&packages_dir)?;
     if packages.is_empty() {
         println!("no packages configured under {}/", packages_dir.display());
         return Ok(());
@@ -272,7 +285,7 @@ pub fn run_review(args: &[String]) -> Result<()> {
         state_dir,
         work_dir,
     } = Paths::from_env()?;
-    let packages = config::load_packages_dir(&packages_dir)?;
+    let packages = load_packages(&packages_dir)?;
 
     match args {
         [] => {

src/state.rs

@@ -1,6 +1,6 @@
 //! Persists two independent per-package facts as plain files: the last
 //! published version, and any version currently pending human review.
-//! The only module that touches `state/` on disk.
+//! The only module that touches the state directory (see `paths.rs`) on disk.
 
 use anyhow::Result;
 use std::path::Path;