From 202a8acab548f385e7a049a884a154cf7e491c97 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:07:55 -0700 Subject: [PATCH 1/5] refactor(creds): move credential storage from hm-config to hm-core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce hm-core as the home for process-level state, and move the credential store into it as Sys::creds. The store collapses from a two-level (service, account) map to a single token. The cloud issues one user-scoped bearer -- claim_token / redeem_code take no org, and that one token lists every organization the user belongs to -- so keying it by anything was storing one secret under a key that could not discriminate it. A second API base (localhost, staging) is served by HM_API_TOKEN, which is how the integration tests already inject one, so the stored file only ever held a single entry in practice. This is a breaking change to ~/.config/hm/credentials.toml. The old nested shape no longer parses and there is no migration; existing logins must re-run `hm cloud login`. Behaviour changes beyond the move: - The file must be owned by the invoking user and chmod 0600, else load fails. Previously a malformed or insecure file was silently ignored. - Tokens are held as secrecy::SecretString rather than String, so they no longer leak through Debug. They are exposed only where a str is required: HarmontClient::with_base_url. - HM_API_TOKEN="" is treated as unset. Honouring it sent an empty bearer (a 401) instead of falling back to the stored credential, which bites when CI exports an unset secret as the empty string. Also carries a distinct fix that lives in the same rewritten resolver, called out here because it is not implied by the title. The cloud verbs called Config::load(None) -- user and env layers only -- so `[cloud] org` set in a project's .hm/config.toml was silently ignored, while the error message users hit named the very file being ignored: no organization — set `[cloud] org = "…"` in ~/.config/hm/config.toml (or .hm/config.toml), or run `hm cloud org switch ` settings::config() now walks up from the cwd and passes the project config path to Config::load, which already takes an Option<&Path> for exactly this. `hm cloud org switch` deliberately keeps Config::load(None): it is a load-mutate-save on the user file, and merging the project layer in first would persist project-scoped values into ~/.config/hm/config.toml. Supporting work: hm-util gains AbsPath/AbsPathBuf and RelPath/RelPathBuf, typed wrappers carrying the absolute/relative invariant, used by hm-core's paths; hm-config drops its now-unused anyhow dependency and replaces two hand-written Default impls with SmartDefault. --- Cargo.lock | 49 ++- Cargo.toml | 3 + crates/hm-config/Cargo.toml | 3 +- crates/hm-config/src/creds.rs | 143 --------- crates/hm-config/src/lib.rs | 369 +++++----------------- crates/hm-core/Cargo.toml | 26 ++ crates/hm-core/lib.rs | 7 + crates/hm-core/sys.rs | 113 +++++++ crates/hm-core/sys/creds.rs | 187 +++++++++++ crates/hm-core/sys/env.rs | 22 ++ crates/hm-core/workspace.rs | 141 +++++++++ crates/hm-plugin-cloud/Cargo.toml | 9 + crates/hm-plugin-cloud/src/auth/login.rs | 8 +- crates/hm-plugin-cloud/src/auth/logout.rs | 7 +- crates/hm-plugin-cloud/src/settings.rs | 51 ++- crates/hm-plugin-cloud/src/verbs/org.rs | 3 + crates/hm-util/Cargo.toml | 1 + crates/hm-util/src/lib.rs | 1 + crates/hm-util/src/path/abs_path.rs | 150 +++++++++ crates/hm-util/src/path/mod.rs | 5 + crates/hm-util/src/path/rel_path.rs | 111 +++++++ crates/hm/Cargo.toml | 2 + crates/hm/src/commands/init.rs | 7 +- crates/hm/src/commands/run/mod.rs | 10 +- crates/hm/src/lib.rs | 3 - 25 files changed, 966 insertions(+), 465 deletions(-) delete mode 100644 crates/hm-config/src/creds.rs create mode 100644 crates/hm-core/Cargo.toml create mode 100644 crates/hm-core/lib.rs create mode 100644 crates/hm-core/sys.rs create mode 100644 crates/hm-core/sys/creds.rs create mode 100644 crates/hm-core/sys/env.rs create mode 100644 crates/hm-core/workspace.rs create mode 100644 crates/hm-util/src/path/abs_path.rs create mode 100644 crates/hm-util/src/path/mod.rs create mode 100644 crates/hm-util/src/path/rel_path.rs diff --git a/Cargo.lock b/Cargo.lock index 217d31c1..2f447144 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1097,6 +1097,7 @@ dependencies = [ "harmont-cloud", "hex", "hm-config", + "hm-core", "hm-dsl-engine", "hm-exec", "hm-pipeline-ir", @@ -1112,6 +1113,7 @@ dependencies = [ "rand 0.8.6", "reqwest", "schemars 0.8.22", + "secrecy", "semver", "serde", "serde_json", @@ -1225,12 +1227,28 @@ checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" name = "hm-config" version = "0.0.0-dev" dependencies = [ - "anyhow", "derive_more", "figment", "hm-util", "serde", + "smart-default", + "tempfile", + "thiserror 2.0.18", + "tokio", + "toml", +] + +[[package]] +name = "hm-core" +version = "0.0.0-dev" +dependencies = [ + "hm-config", + "hm-util", + "nix", + "secrecy", + "serde", "tempfile", + "thiserror 2.0.18", "tokio", "toml", ] @@ -1303,10 +1321,17 @@ dependencies = [ "harmont-cloud", "harmont-cloud-raw", "hm-config", + "hm-core", "hm-exec", "hm-plugin-protocol", "hm-render", + "hm-util", + "percent-encoding", + "reqwest", + "secrecy", + "serde", "serde_json", + "tempfile", "tokio", "tracing", "uuid", @@ -1348,6 +1373,7 @@ name = "hm-util" version = "0.0.0-dev" dependencies = [ "anyhow", + "derive_more", "dirs", "tempfile", "tokio", @@ -1994,6 +2020,18 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "27b02d87554356db9e9a873add8782d4ea6e3e58ea071a9adb9a2e8ddb884a8b" +[[package]] +name = "nix" +version = "0.30.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "74523f3a35e05aba87a1d978330aef40f67b0304ac79c1c00b294c9830543db6" +dependencies = [ + "bitflags", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "nom" version = "7.1.3" @@ -2763,6 +2801,15 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "secrecy" +version = "0.10.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e891af845473308773346dc847b2c23ee78fe442e0472ac50e22a18a93d3ae5a" +dependencies = [ + "zeroize", +] + [[package]] name = "security-framework" version = "3.7.0" diff --git a/Cargo.toml b/Cargo.toml index b3397710..b8250e5c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,6 +3,7 @@ resolver = "2" members = [ "crates/hm", "crates/hm-config", + "crates/hm-core", "crates/hm-plugin-protocol", "crates/hm-pipeline-ir", "crates/hm-util", @@ -15,6 +16,7 @@ members = [ default-members = [ "crates/hm", "crates/hm-config", + "crates/hm-core", "crates/hm-plugin-protocol", "crates/hm-pipeline-ir", "crates/hm-util", @@ -37,6 +39,7 @@ hm-plugin-cloud = { path = "crates/hm-plugin-cloud", version = "0.0.0-dev" hm-pipeline-ir = { path = "crates/hm-pipeline-ir", version = "0.0.0-dev" } hm-util = { path = "crates/hm-util", version = "0.0.0-dev" } hm-config = { path = "crates/hm-config", version = "0.0.0-dev" } +hm-core = { path = "crates/hm-core", version = "0.0.0-dev" } hm-dsl-engine = { path = "crates/hm-dsl-engine", version = "0.0.0-dev" } hm-render = { path = "crates/hm-render", version = "0.0.0-dev" } hm-vm = { path = "crates/hm-vm", version = "0.0.0-dev" } diff --git a/crates/hm-config/Cargo.toml b/crates/hm-config/Cargo.toml index 557fb149..cfe18d2c 100644 --- a/crates/hm-config/Cargo.toml +++ b/crates/hm-config/Cargo.toml @@ -11,8 +11,9 @@ hm-util = { workspace = true } derive_more = { workspace = true } figment = { workspace = true } serde = { workspace = true } +smart-default = { workspace = true } toml = "0.8" -anyhow = "1" +thiserror = { workspace = true } [dev-dependencies] tempfile = "3" diff --git a/crates/hm-config/src/creds.rs b/crates/hm-config/src/creds.rs deleted file mode 100644 index 7008256d..00000000 --- a/crates/hm-config/src/creds.rs +++ /dev/null @@ -1,143 +0,0 @@ -//! File-backed credential store at `~/.config/hm/credentials.toml`. -//! -//! Replaces the OS keyring as the sole backend. The file is written with -//! mode 0o600 (parent dir 0o700) via [`hm_util::os::fs::blocking::write_atomic_restricted`]. -//! Keyed by `(service, account)` to match the host-fn ABI plugins use. - -use anyhow::{Context, Result}; -use serde::{Deserialize, Serialize}; -use std::collections::BTreeMap; -use std::path::PathBuf; - -#[derive(Debug, Default, Serialize, Deserialize)] -struct CredentialFile { - #[serde(default)] - entries: BTreeMap>, -} - -fn path() -> Result { - let dir = hm_util::dirs::hm_config_dir().context("could not determine config directory")?; - Ok(dir.join("credentials.toml")) -} - -fn load() -> CredentialFile { - let Ok(p) = path() else { - return CredentialFile::default(); - }; - let Ok(contents) = std::fs::read_to_string(&p) else { - return CredentialFile::default(); - }; - toml::from_str(&contents).unwrap_or_default() -} - -fn save(file: &CredentialFile) -> Result<()> { - let p = path()?; - let serialized = toml::to_string_pretty(file).context("serializing credentials")?; - hm_util::os::fs::blocking::write_atomic_restricted( - &p, - serialized.as_bytes(), - hm_util::os::fs::FileMode(0o600), - hm_util::os::fs::DirMode(0o700), - ) - .with_context(|| format!("writing {}", p.display()))?; - Ok(()) -} - -/// Read a credential for `(service, account)`. Returns `None` when the -/// file is missing, unreadable, or the entry is absent. -#[must_use] -pub fn get(service: &str, account: &str) -> Option { - load().entries.get(service)?.get(account).cloned() -} - -/// Write a credential. Silently no-ops on I/O failure so plugin callers -/// match the prior keyring-backed best-effort semantics. -pub fn set(service: &str, account: &str, secret: &str) { - let mut f = load(); - f.entries - .entry(service.to_string()) - .or_default() - .insert(account.to_string(), secret.to_string()); - let _ = save(&f); -} - -/// Credential `service` name for the cloud bearer token (account = API base URL). -pub const CLOUD_SERVICE: &str = "harmont-cloud"; - -/// Resolve the cloud bearer token for `api_base`. -/// -/// Priority: `HM_API_TOKEN` env (non-empty) first, then the stored -/// credential keyed by `(CLOUD_SERVICE, api_base)`. Returns `None` when -/// neither is present, so the caller can produce a clear "not logged in" error. -#[must_use] -pub fn cloud_token(api_base: &str) -> Option { - if let Ok(t) = std::env::var("HM_API_TOKEN") - && !t.is_empty() - { - return Some(t); - } - get(CLOUD_SERVICE, api_base) -} - -/// Persist the cloud bearer token for `api_base`. -/// -/// Silently no-ops on I/O failure (matches the best-effort semantics of -/// the underlying [`set`] call). -pub fn set_cloud_token(api_base: &str, token: &str) { - set(CLOUD_SERVICE, api_base, token); -} - -/// Remove any stored cloud bearer token for `api_base`. -/// -/// Silently no-ops if the entry is absent or the write fails. -pub fn forget_cloud_token(api_base: &str) { - delete(CLOUD_SERVICE, api_base); -} - -/// Remove a credential. Silently no-ops if the entry is absent or the -/// underlying write fails. -pub fn delete(service: &str, account: &str) { - let mut f = load(); - let now_empty = f.entries.get_mut(service).is_some_and(|svc| { - svc.remove(account); - svc.is_empty() - }); - if now_empty { - f.entries.remove(service); - } - let _ = save(&f); -} - -#[cfg(test)] -#[allow(clippy::unwrap_used, unsafe_code)] -mod tests { - use super::*; - - fn with_home(f: F) { - let tmp = tempfile::tempdir().unwrap(); - let prev = std::env::var_os("HOME"); - // SAFETY: tests are single-threaded for env mutation by Cargo. - unsafe { - std::env::set_var("HOME", tmp.path()); - } - f(); - unsafe { - if let Some(v) = prev { - std::env::set_var("HOME", v); - } else { - std::env::remove_var("HOME"); - } - } - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn round_trip() { - with_home(|| { - assert_eq!(get("svc", "acct"), None); - set("svc", "acct", "shh"); - assert_eq!(get("svc", "acct").as_deref(), Some("shh")); - delete("svc", "acct"); - assert_eq!(get("svc", "acct"), None); - }); - } -} diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index 7bd5fd2f..22d162f6 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -1,20 +1,50 @@ -//! Layered (project/user/env) configuration and credential storage for the -//! `hm` CLI. Shared between the `hm` binary and `hm-plugin-cloud` so both sides -//! resolve config and credentials through one source of truth. +//! Layered (project/user/env) configuration for the `hm` CLI. Shared between +//! the `hm` binary and `hm-plugin-cloud` so both sides resolve config through +//! one source of truth. +//! +//! Credentials live in `hm-core` (`hm_core::Sys::creds`), not here. use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; use figment::{ Figment, providers::{Env, Format, Serialized, Toml}, }; use serde::{Deserialize, Serialize}; - -pub mod creds; +use smart_default::SmartDefault; pub const DEFAULT_API_URL: &str = "https://api.harmont.dev"; +/// Errors produced while loading configuration. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum LoadError { + /// Figment failed to merge/extract a config layer (malformed TOML, type mismatch, …). + #[error(transparent)] + Figment(#[from] figment::Error), +} + +/// Errors produced while persisting configuration. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum SaveError { + /// Platform config directory (`~/.config/hm` or equivalent) could not be resolved. + #[error("could not determine config directory")] + ConfigDirUnavailable, + + /// TOML serialization of a [`Config`] failed. + #[error("serializing config")] + Serialize(#[from] toml::ser::Error), + + /// Atomic write of a config file failed. + #[error("writing {}", .path.display())] + Write { + path: PathBuf, + #[source] + source: std::io::Error, + }, +} + /// Execution backend for `hm run`. /// /// Closed set parsed at the config boundary so invalid values are rejected at @@ -64,10 +94,11 @@ pub fn app_url(api: &str, override_url: Option<&str>) -> String { api.to_string() } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, SmartDefault)] #[non_exhaustive] pub struct CloudConfig { pub org: Option, + #[default(DEFAULT_API_URL.to_owned())] pub api_url: String, /// Org-global pipeline slug to submit builds to directly (set by `hm run` /// after registering a remoteless directory). When present, cloud runs @@ -75,32 +106,14 @@ pub struct CloudConfig { pub pipeline: Option, } -impl Default for CloudConfig { - fn default() -> Self { - Self { - org: None, - api_url: DEFAULT_API_URL.to_owned(), - pipeline: None, - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, SmartDefault)] #[non_exhaustive] pub struct Preferences { + #[default("human".to_owned())] pub format: String, pub auto_watch: bool, } -impl Default for Preferences { - fn default() -> Self { - Self { - format: "human".to_owned(), - auto_watch: false, - } - } -} - #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] #[non_exhaustive] pub struct Config { @@ -115,12 +128,10 @@ pub struct Config { impl Config { /// XDG-aware user config path (`~/.config/hm/config.toml`). /// - /// # Errors - /// - /// Returns an error if the platform config directory cannot be determined. - pub fn user_config_path() -> Result { - let dir = hm_util::dirs::hm_config_dir().context("could not determine config directory")?; - Ok(dir.join("config.toml")) + /// Returns `None` if the platform config directory cannot be determined. + #[must_use] + pub fn user_config_path() -> Option { + hm_util::dirs::hm_config_dir().map(|d| d.join("config.toml")) } /// Project-level config path: `/.hm/config.toml`. @@ -131,15 +142,17 @@ impl Config { /// Load configuration with full layering: defaults -> user file -> project file -> env. /// + /// `project_config_path` is the path to the project-level config file + /// (typically `.hm/config.toml`), not the project root. Pass `None` to + /// skip the project layer (user + env only). When the user config path + /// cannot be resolved, the user layer is skipped as well. + /// /// # Errors /// - /// Returns an error if the user config path cannot be determined or - /// figment extraction fails (malformed TOML, type mismatches). - pub fn load(project_root: Option<&Path>) -> Result { - let user_path = Self::user_config_path()?; - let project_path = project_root.map(Self::project_config_path); - Self::load_from_paths(Some(&user_path), project_path.as_deref()) - .context("loading configuration") + /// Returns [`LoadError`] if figment extraction fails (malformed TOML, type + /// mismatches). + pub fn load(project_config_path: Option<&Path>) -> Result { + Self::load_from_paths(Self::user_config_path().as_deref(), project_config_path) } /// Testable core: build a `Config` from explicit file paths. @@ -154,8 +167,12 @@ impl Config { /// /// # Errors /// - /// Returns an error if figment extraction fails (malformed TOML, type mismatches). - pub fn load_from_paths(user_path: Option<&Path>, project_path: Option<&Path>) -> Result { + /// Returns [`LoadError`] if figment extraction fails (malformed TOML, type + /// mismatches). + pub fn load_from_paths( + user_path: Option<&Path>, + project_path: Option<&Path>, + ) -> Result { let mut figment = Figment::new().merge(Serialized::defaults(Self::default())); if let Some(p) = user_path { @@ -176,25 +193,31 @@ impl Config { /// /// # Errors /// - /// Returns an error if TOML serialization fails or the atomic write fails. - pub fn save_to(&self, path: &Path) -> Result<()> { - let serialized = toml::to_string_pretty(self).context("serializing config")?; + /// Returns [`SaveError`] if TOML serialization fails or the atomic write + /// fails. + pub fn save_to(&self, path: &Path) -> Result<(), SaveError> { + let serialized = toml::to_string_pretty(self)?; hm_util::os::fs::blocking::write_atomic_restricted( path, serialized.as_bytes(), hm_util::os::fs::FileMode(0o644), hm_util::os::fs::DirMode(0o700), ) - .with_context(|| format!("writing {}", path.display())) + .map_err(|source| SaveError::Write { + path: path.to_path_buf(), + source, + }) } /// Save to user-level config path (`~/.config/hm/config.toml`). /// /// # Errors /// - /// Returns an error if the path cannot be determined or the write fails. - pub fn save_user(&self) -> Result<()> { - self.save_to(&Self::user_config_path()?) + /// Returns [`SaveError::ConfigDirUnavailable`] if the path cannot be + /// determined, or any other [`SaveError`] if the write fails. + pub fn save_user(&self) -> Result<(), SaveError> { + let path = Self::user_config_path().ok_or(SaveError::ConfigDirUnavailable)?; + self.save_to(&path) } } @@ -215,251 +238,3 @@ fn hm_alias_env() -> Env { }) .split(".") } - -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - use std::io::Write as _; - use std::sync::{Mutex, MutexGuard}; - - /// Serializes every test that resolves config through `load_*`. - /// - /// All `load_*` paths merge the process environment as their top layer, so - /// a test that sets `HM_*` (via `figment::Jail`, which mutates the - /// real process env for the duration of its closure) would otherwise leak - /// into a concurrently-running file-layering test. Holding this lock for - /// the whole body of any env-or-load test makes them mutually exclusive. - static ENV_LOCK: Mutex<()> = Mutex::new(()); - - fn env_guard() -> MutexGuard<'static, ()> { - ENV_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - } - - #[test] - fn app_url_maps_prod_api_to_app() { - assert_eq!(app_url(DEFAULT_API_URL, None), "https://app.harmont.dev"); - } - - #[test] - fn app_url_override_wins_and_trims_trailing_slash() { - assert_eq!( - app_url(DEFAULT_API_URL, Some("http://localhost:5173/")), - "http://localhost:5173" - ); - } - - #[test] - fn app_url_empty_override_is_ignored() { - assert_eq!( - app_url(DEFAULT_API_URL, Some(" ")), - "https://app.harmont.dev" - ); - } - - #[test] - fn app_url_falls_back_to_api_for_unmapped_host() { - assert_eq!( - app_url("http://localhost:4000", None), - "http://localhost:4000" - ); - // http api. → http app. - assert_eq!(app_url("http://api.dev.test/", None), "http://app.dev.test"); - } - - #[test] - fn default_config_values() { - let cfg = Config::default(); - assert_eq!(cfg.backend, Backend::Docker); - assert_eq!(cfg.cloud.api_url, DEFAULT_API_URL); - assert!(cfg.cloud.org.is_none()); - assert!(cfg.cloud.pipeline.is_none()); - assert_eq!(cfg.preferences.format, "human"); - assert!(!cfg.preferences.auto_watch); - } - - #[test] - fn deserialize_full_toml() { - let toml_str = r#" -[cloud] -org = "acme" -api_url = "https://custom.api" -pipeline = "acme/web" - -[preferences] -format = "json" -auto_watch = true -"#; - let cfg: Config = toml::from_str(toml_str).unwrap(); - assert_eq!(cfg.cloud.org.as_deref(), Some("acme")); - assert_eq!(cfg.cloud.api_url, "https://custom.api"); - assert_eq!(cfg.cloud.pipeline.as_deref(), Some("acme/web")); - assert_eq!(cfg.preferences.format, "json"); - assert!(cfg.preferences.auto_watch); - } - - #[test] - fn deserialize_sparse_toml() { - let _g = env_guard(); - let toml_str = r#" -[cloud] -org = "sparse-co" -"#; - let mut f = tempfile::NamedTempFile::new().unwrap(); - f.write_all(toml_str.as_bytes()).unwrap(); - - let cfg = Config::load_from_paths(Some(f.path()), None).unwrap(); - assert_eq!(cfg.cloud.org.as_deref(), Some("sparse-co")); - assert_eq!(cfg.cloud.api_url, DEFAULT_API_URL); - assert_eq!(cfg.preferences.format, "human"); - assert!(!cfg.preferences.auto_watch); - } - - #[test] - fn deserialize_empty_toml() { - let _g = env_guard(); - let mut f = tempfile::NamedTempFile::new().unwrap(); - f.write_all(b"").unwrap(); - - let cfg = Config::load_from_paths(Some(f.path()), None).unwrap(); - assert_eq!(cfg.cloud.api_url, DEFAULT_API_URL); - assert!(cfg.cloud.org.is_none()); - assert_eq!(cfg.preferences.format, "human"); - assert!(!cfg.preferences.auto_watch); - } - - #[test] - fn figment_project_overrides_user() { - let _g = env_guard(); - let user_toml = r#" -[cloud] -org = "user-org" -api_url = "https://user.api" - -[preferences] -format = "json" -"#; - let project_toml = r#" -[cloud] -org = "project-org" -"#; - - let mut user_file = tempfile::NamedTempFile::new().unwrap(); - user_file.write_all(user_toml.as_bytes()).unwrap(); - - let mut project_file = tempfile::NamedTempFile::new().unwrap(); - project_file.write_all(project_toml.as_bytes()).unwrap(); - - let cfg = - Config::load_from_paths(Some(user_file.path()), Some(project_file.path())).unwrap(); - - assert_eq!(cfg.cloud.org.as_deref(), Some("project-org")); - assert_eq!(cfg.cloud.api_url, "https://user.api"); - assert_eq!(cfg.preferences.format, "json"); - } - - #[test] - fn backend_display_matches_wire_strings() { - assert_eq!(Backend::Docker.to_string(), "docker"); - assert_eq!(Backend::Cloud.to_string(), "cloud"); - } - - #[test] - fn backend_defaults_docker_and_parses_and_layers() { - let _g = env_guard(); - // default - assert_eq!(Config::default().backend, Backend::Docker); - - // user file sets cloud; project file sets docker -> project wins. - let mut user_file = tempfile::NamedTempFile::new().unwrap(); - user_file.write_all(br#"backend = "cloud""#).unwrap(); - - let mut project_file = tempfile::NamedTempFile::new().unwrap(); - project_file.write_all(br#"backend = "docker""#).unwrap(); - - let cfg = - Config::load_from_paths(Some(user_file.path()), Some(project_file.path())).unwrap(); - assert_eq!(cfg.backend, Backend::Docker); - - // user file alone parses "cloud". - let cfg_user = Config::load_from_paths(Some(user_file.path()), None).unwrap(); - assert_eq!(cfg_user.backend, Backend::Cloud); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn save_and_reload_roundtrip() { - let _g = env_guard(); - let tmp = tempfile::tempdir().unwrap(); - let path = tmp.path().join("config.toml"); - let cfg = Config { - cloud: CloudConfig { - org: Some("saved-org".into()), - pipeline: Some("saved-org/web".into()), - ..CloudConfig::default() - }, - ..Config::default() - }; - cfg.save_to(&path).unwrap(); - - let loaded = Config::load_from_paths(Some(&path), None).unwrap(); - assert_eq!(loaded.cloud.org.as_deref(), Some("saved-org")); - assert_eq!(loaded.cloud.pipeline.as_deref(), Some("saved-org/web")); - assert_eq!(loaded.cloud.api_url, DEFAULT_API_URL); - assert_eq!(loaded.preferences.format, "human"); - } - - #[test] - #[allow(clippy::result_large_err)] // figment::Error is the Jail closure's error type. - fn hm_env_overrides_cloud_keys() { - let _g = env_guard(); - // `Jail` isolates env mutation from concurrently-running tests. - figment::Jail::expect_with(|jail| { - jail.set_env("HM_ORG", "env-org"); - jail.set_env("HM_API_URL", "https://env.api"); - - let cfg = Config::load_from_paths(None, None).unwrap(); - assert_eq!(cfg.cloud.org.as_deref(), Some("env-org")); - assert_eq!(cfg.cloud.api_url, "https://env.api"); - Ok(()) - }); - } - - #[test] - #[allow(clippy::result_large_err)] // figment::Error is the Jail closure's error type. - fn hm_env_overrides_user_file() { - let _g = env_guard(); - // Env is the highest-precedence layer: it wins over a user file. - figment::Jail::expect_with(|jail| { - jail.set_env("HM_ORG", "env-org"); - - jail.create_file( - "config.toml", - "[cloud]\norg = \"file-org\"\napi_url = \"https://file.api\"\n", - )?; - let user = jail.directory().join("config.toml"); - - let cfg = Config::load_from_paths(Some(&user), None).unwrap(); - assert_eq!(cfg.cloud.org.as_deref(), Some("env-org")); - // Unset env keys still come from the file. - assert_eq!(cfg.cloud.api_url, "https://file.api"); - Ok(()) - }); - } - - #[test] - fn figment_missing_files_still_resolve() { - let _g = env_guard(); - let nonexistent_user = Path::new("/tmp/harmont-test-nonexistent-user/config.toml"); - let nonexistent_project = Path::new("/tmp/harmont-test-nonexistent-project/config.toml"); - - let cfg = - Config::load_from_paths(Some(nonexistent_user), Some(nonexistent_project)).unwrap(); - - assert_eq!(cfg.cloud.api_url, DEFAULT_API_URL); - assert!(cfg.cloud.org.is_none()); - assert_eq!(cfg.preferences.format, "human"); - assert!(!cfg.preferences.auto_watch); - } -} diff --git a/crates/hm-core/Cargo.toml b/crates/hm-core/Cargo.toml new file mode 100644 index 00000000..9beba055 --- /dev/null +++ b/crates/hm-core/Cargo.toml @@ -0,0 +1,26 @@ +[package] +name = "hm-core" +version = "0.0.0-dev" +edition.workspace = true +license.workspace = true +repository.workspace = true +description = "Core system types (workspace, credentials, process identity) for the hm CLI." + +[lib] +path = "lib.rs" + +[dependencies] +hm-config = { workspace = true } +hm-util = { workspace = true } +serde = { workspace = true } +thiserror = { workspace = true } +toml = "0.8" +secrecy = "0.10.3" +nix = { version = "0.30", default-features = false, features = ["user"] } + +[dev-dependencies] +tempfile = "3" +tokio = { workspace = true } + +[lints] +workspace = true diff --git a/crates/hm-core/lib.rs b/crates/hm-core/lib.rs new file mode 100644 index 00000000..c1a4cf21 --- /dev/null +++ b/crates/hm-core/lib.rs @@ -0,0 +1,7 @@ +//! Core system types for the hm CLI (process identity, credentials, …). + +pub mod sys; +pub mod workspace; + +pub use sys::{LoadingError as SysLoadingError, Sys}; +pub use workspace::{LoadError as WorkspaceLoadError, Workspace}; diff --git a/crates/hm-core/sys.rs b/crates/hm-core/sys.rs new file mode 100644 index 00000000..80b0f35c --- /dev/null +++ b/crates/hm-core/sys.rs @@ -0,0 +1,113 @@ +//! Global utility for fetching system-relevant information on `hm`. +//! +//! [`Sys`] is the process-level handle: credentials (and later system config) +//! for the invoking user. Project-scoped state lives elsewhere (workspace). +//! +//! We define the system to mean the scope of the current user. This includes the user's +//! configuration directory, etc. + +pub mod creds; +pub mod env; + +use std::io; +use std::path::PathBuf; + +use hm_util::path::{AbsPath, AbsPathBuf}; +use thiserror::Error; + +use creds::Creds; + +/// Failure loading process-level [`Sys`] state. +#[derive(Debug, Error)] +pub enum LoadingError { + /// Platform config directory could not be resolved. + #[error("could not determine config directory")] + ConfigDirUnavailable, + + /// Failed to create the config directory. + #[error("failed to create {}: {source}", .path.display())] + CreateDir { + path: PathBuf, + #[source] + source: io::Error, + }, + + /// Credentials file failed security checks or parse. + /// + /// Transparent: `#[from]` already makes this a `#[source]`, so wrapping the + /// text here too would print the inner message twice. + #[error(transparent)] + Creds(#[from] creds::LoadingError), +} + +/// Process-level hm state (credentials, …). +#[derive(Debug)] +pub struct Sys { + /// Absolute path to `~/.config/hm` (user config root). + hm_dir: AbsPathBuf, + + creds: Creds, +} + +impl Sys { + /// Return the current user name (best-effort). + /// + /// Prefers `$USER`, then `$LOGNAME`, then a `uid N` fallback so ownership + /// error messages always have something printable. + #[must_use] + pub fn whoami() -> String { + std::env::var("USER") + .or_else(|_| std::env::var("LOGNAME")) + .unwrap_or_else(|_| format!("uid {}", Self::whoami_id())) + } + + /// Return the current real user id. + #[must_use] + pub fn whoami_id() -> u32 { + nix::unistd::Uid::current().as_raw() + } + + /// Absolute path to the `~/.config/hm` directory. + #[must_use] + pub fn hm_dir(&self) -> AbsPath<'_> { + self.hm_dir.as_abs_path() + } + + /// Load process-level system state (credentials, …). + /// + /// Ensures `~/.config/hm` exists, then loads `credentials.toml` from it. + /// + /// # Errors + /// + /// Returns [`LoadingError`] when the config directory cannot be resolved or + /// created, or credentials fail to load. + pub fn load() -> Result { + let hm_dir = hm_util::dirs::hm_config_dir() + .and_then(AbsPathBuf::new) + .ok_or(LoadingError::ConfigDirUnavailable)?; + + if !hm_dir.exists() { + std::fs::create_dir_all(hm_dir.as_abs_path().as_path()).map_err(|source| { + LoadingError::CreateDir { + path: hm_dir.clone().into_path_buf(), + source, + } + })?; + } + + let creds_path = hm_dir.join("credentials.toml"); + let creds = Creds::load(creds_path.as_abs_path().as_path())?; + Ok(Self { hm_dir, creds }) + } + + /// Loaded credentials. + #[must_use] + pub const fn creds(&self) -> &Creds { + &self.creds + } + + /// Mutable access to credentials (for `set` / `remove`). + pub const fn creds_mut(&mut self) -> &mut Creds { + &mut self.creds + } +} diff --git a/crates/hm-core/sys/creds.rs b/crates/hm-core/sys/creds.rs new file mode 100644 index 00000000..0a66237d --- /dev/null +++ b/crates/hm-core/sys/creds.rs @@ -0,0 +1,187 @@ +//! Credentials to access the Harmont cloud. +//! +//! The cloud issues a single **user-scoped** bearer: `claim_token` / +//! `redeem_code` return one token, and it lists every organization the user +//! belongs to. So there is exactly one credential to store, not a map. +//! +//! A second API base (localhost, staging) is served by the `HM_API_TOKEN` +//! environment variable, which overrides the stored value on resolve. + +use std::fs; +use std::io; +use std::path::{Path, PathBuf}; + +use secrecy::{ExposeSecret, SecretString}; +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +use crate::sys; + +use super::Sys; + +/// Failure loading a credentials file. +#[derive(Debug, Error)] +pub enum LoadingError { + /// File exists but is owned by someone other than the current user. + #[error( + "{user} does not own {path}. In order for this to be safe, {user} must own {path}", + user = .0, + path = .1.display() + )] + FileNotOwned(String, PathBuf), + + /// File exists but is not mode `0o600`. + #[error( + "{path} is not chmod 600. In order for this to be secure, {path} must be chmod 0o600", + path = .0.display() + )] + FileNotExclusivelyOwned(PathBuf), + + /// Credentials file could not be read. + #[error("failed to read {}: {source}", .path.display())] + Read { + path: PathBuf, + #[source] + source: io::Error, + }, + + /// Credentials file was not valid TOML / shape. + #[error("failed to parse {}: {source}", .path.display())] + Parse { + path: PathBuf, + #[source] + source: toml::de::Error, + }, +} + +/// On-disk TOML shape: a plain string (serde). +#[derive(Debug, Default, Serialize, Deserialize)] +struct CredsDto { + #[serde(default, skip_serializing_if = "Option::is_none")] + token: Option, +} + +impl From<&Creds> for CredsDto { + fn from(creds: &Creds) -> Self { + Self { + token: creds + .token + .as_ref() + .map(|s| s.expose_secret().to_owned()), + } + } +} + +/// The stored Harmont Cloud bearer token. +/// +/// Remembers the path it was loaded from so [`Self::set`] / [`Self::clear`] +/// can flush. +#[derive(Debug, Clone)] +pub struct Creds { + /// Path used for flush (the credentials file, which may not exist yet). + path: PathBuf, + /// The stored bearer, if the user has logged in. + token: Option, +} + +impl Creds { + /// Load credentials from `path`. + /// + /// If the file does not exist, returns empty credentials bound to that + /// path (so a later [`Self::set`] can create it). If the file exists, it + /// must pass ownership and mode checks before being parsed. + /// + /// # Errors + /// + /// Returns [`LoadingError`] when the file is insecure, unreadable, or + /// malformed. + pub fn load(path: &Path) -> Result { + if !path.exists() { + return Ok(Self { + path: path.to_path_buf(), + token: None, + }); + } + + Self::validate_secure(path)?; + + let contents = fs::read_to_string(path).map_err(|source| LoadingError::Read { + path: path.to_path_buf(), + source, + })?; + let dto: CredsDto = toml::from_str(&contents).map_err(|source| LoadingError::Parse { + path: path.to_path_buf(), + source, + })?; + + Ok(Self { + path: path.to_path_buf(), + token: dto.token.map(SecretString::from), + }) + } + + /// Resolve the bearer token. + /// + /// Priority: a non-empty `HM_API_TOKEN` environment variable, then the + /// stored value. Returns an owned [`SecretString`] so the env override + /// does not need a place in the store. + #[must_use] + pub fn token(&self) -> Option { + sys::env::hm_api_token() + .clone() + .or_else(|| self.token.clone()) + } + + /// Store the bearer and best-effort flush to disk. + /// + /// Flush failures are discarded (best-effort persist). + pub fn set(&mut self, secret: SecretString) { + self.token = Some(secret); + let _ = self.flush(); + } + + /// Drop the stored bearer and best-effort flush. + /// + /// Note this only clears the *stored* value; a `HM_API_TOKEN` in the + /// environment still wins on the next [`Self::token`]. + pub fn clear(&mut self) { + self.token = None; + let _ = self.flush(); + } + + /// Write the current state to the credentials path (mode `0o600`). + /// + /// Uses [`hm_util::os::fs::blocking::write_atomic_restricted`]; requires a + /// current tokio runtime. + fn flush(&self) -> io::Result<()> { + let dto = CredsDto::from(self); + let serialized = toml::to_string_pretty(&dto) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; + hm_util::os::fs::blocking::write_atomic_restricted( + &self.path, + serialized.as_bytes(), + hm_util::os::fs::FileMode(0o600), + hm_util::os::fs::DirMode(0o700), + ) + } + + /// Ensure `path` is owned by the current user and mode `0o600`. + fn validate_secure(path: &Path) -> Result<(), LoadingError> { + use std::os::unix::fs::MetadataExt; + + let meta = fs::metadata(path).map_err(|source| LoadingError::Read { + path: path.to_path_buf(), + source, + })?; + + if meta.uid() != Sys::whoami_id() { + return Err(LoadingError::FileNotOwned(Sys::whoami(), path.to_path_buf())); + } + + if meta.mode() & 0o777 != 0o600 { + return Err(LoadingError::FileNotExclusivelyOwned(path.to_path_buf())); + } + + Ok(()) + } +} diff --git a/crates/hm-core/sys/env.rs b/crates/hm-core/sys/env.rs new file mode 100644 index 00000000..db1cd49f --- /dev/null +++ b/crates/hm-core/sys/env.rs @@ -0,0 +1,22 @@ +use std::sync::OnceLock; + +use secrecy::SecretString; + +static HM_API_TOKEN: OnceLock> = OnceLock::new(); + +/// The `HM_API_TOKEN` override, if set to a non-empty value. +/// +/// An empty `HM_API_TOKEN` is treated as unset: CI commonly exports an unset +/// secret as the empty string, and honoring it would send an empty bearer +/// (a 401) instead of falling back to the stored credential. +/// +/// Read once per process — `hm` is a CLI, so the environment does not change +/// under us. +pub fn hm_api_token() -> &'static Option { + HM_API_TOKEN.get_or_init(|| { + std::env::var("HM_API_TOKEN") + .ok() + .filter(|t| !t.is_empty()) + .map(SecretString::from) + }) +} diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs new file mode 100644 index 00000000..23603b90 --- /dev/null +++ b/crates/hm-core/workspace.rs @@ -0,0 +1,141 @@ +//! Global utility for managing the workspace. + +use std::io; +use std::path::{Path, PathBuf}; + +use hm_config::Config; +use hm_util::path::{AbsPath, AbsPathBuf}; +use thiserror::Error; + +/// Failure resolving or loading a directory as a [`Workspace`]. +#[derive(Debug, Error)] +pub enum LoadError { + #[error("{} does not appear to be a valid path", .0.display())] + InvalidPath(PathBuf), + #[error("{} is not a harmont workspace. harmont workspaces must have a `.hm/` directory", .0.display())] + InvalidWorkspace(PathBuf), + + /// The process working directory could not be read while resolving a root. + #[error("cannot determine current directory")] + CurrentDir(#[source] io::Error), + + /// Walked to the filesystem root without finding a `.hm/` directory. + #[error( + "no harmont workspace found\n → run from a directory that contains `.hm/`, or initialize one with `hm init`" + )] + NotFound, + + #[error(transparent)] + Config(#[from] hm_config::LoadError), +} + +/// Utility for accessing well-known paths inside a workspace. +/// +/// Two constructors: +/// +/// - [`Workspace::resolve`] — the one CLI verbs want. Honors a `--dir`-style +/// override, else walks up from the cwd. +/// - [`Workspace::load`] — validates one **specific** project root (the +/// directory that contains `.hm/`), with no discovery. +#[derive(Debug)] +pub struct Workspace { + /// Absolute path to the `.hm` directory. + hm_dir: AbsPathBuf, + + /// The loaded configuration for this workspace. + /// + /// Note that this will also pull in the user config as required. In other words, this will + /// include the `~/.config/hm/config.toml` configuration as well as the overlayed workspace + /// `.hm/config.toml`. + config: Config, +} + +impl Workspace { + /// Resolve the workspace a command should operate on. + /// + /// The single root-resolution rule for every verb that takes a `--dir`: + /// + /// - **`dir` given** — that directory is the project root, verbatim. No + /// walk-up: the backend runs discovery against cloned repo roots, and + /// walking up out of a clone could bind to an unrelated `.hm/` above it. + /// A relative path is resolved against the cwd, since a workspace root + /// must be absolute. + /// - **`dir` absent** — walk up from the cwd, so a verb works from any + /// subdirectory of the project. + /// + /// # Errors + /// + /// Returns [`LoadError`] if the cwd cannot be determined, no project root + /// is found while walking up, or the resolved root fails [`Self::load`]. + pub fn resolve(dir: Option<&Path>) -> Result { + let root = match dir { + Some(d) if d.is_absolute() => d.to_path_buf(), + Some(d) => std::env::current_dir() + .map_err(LoadError::CurrentDir)? + .join(d), + None => { + let start = std::env::current_dir().map_err(LoadError::CurrentDir)?; + hm_util::dirs::find_project_root(&start).ok_or(LoadError::NotFound)? + } + }; + Self::load(&root) + } + + /// Attempt to load the given directory as a workspace, if it appears to be one. + /// + /// We label a workspace any directory which has a `.hm` directory within it. + /// This does **not** walk parent directories; pass the project root itself. + /// + /// # Errors + /// + /// Returns [`LoadError`] when the path is missing/not absolute, has no + /// `.hm/` directory, or the layered config cannot be loaded. + pub fn load(workspace_path: &Path) -> Result { + if !workspace_path.exists() { + return Err(LoadError::InvalidPath(workspace_path.to_path_buf())); + } + + let hm_dir = workspace_path.join(".hm"); + if !hm_dir.is_dir() { + return Err(LoadError::InvalidWorkspace(workspace_path.to_path_buf())); + } + + let hm_dir = AbsPathBuf::new(hm_dir) + .ok_or_else(|| LoadError::InvalidPath(workspace_path.to_path_buf()))?; + let config = Config::load(Some(&hm_dir.join("config.toml")))?; + Ok(Self { hm_dir, config }) + } + + /// Absolute path to the workspace root (parent of `.hm/`). + #[must_use] + pub fn path(&self) -> AbsPath<'_> { + self.hm_dir + .as_abs_path() + .parent() + .expect("`.hm` always has a parent") + } + + /// Path to the `.hm` directory. + #[must_use] + pub fn hm_dir(&self) -> AbsPath<'_> { + self.hm_dir.as_abs_path() + } + + /// Layered configuration for this workspace (user + project + env). + #[must_use] + pub const fn config(&self) -> &Config { + &self.config + } + + /// Returns the path to the `.env` file at the top level of this workspace. + #[must_use] + pub fn env_file_path(&self) -> AbsPathBuf { + self.path().join(".env") + } + + /// Path to the `.hm/secrets` secrets file. + #[must_use] + pub fn secrets_path(&self) -> AbsPathBuf { + self.hm_dir().join("secrets") + } +} diff --git a/crates/hm-plugin-cloud/Cargo.toml b/crates/hm-plugin-cloud/Cargo.toml index 97d20a07..9c696ceb 100644 --- a/crates/hm-plugin-cloud/Cargo.toml +++ b/crates/hm-plugin-cloud/Cargo.toml @@ -14,9 +14,12 @@ path = "src/lib.rs" [dependencies] harmont-cloud = { workspace = true } harmont-cloud-raw = { workspace = true } +serde = { workspace = true } hm-exec = { workspace = true } hm-render = { workspace = true } hm-config = { workspace = true } +hm-core = { workspace = true } +hm-util = { workspace = true } hm-plugin-protocol = { workspace = true } serde_json = { workspace = true } chrono = { workspace = true } @@ -24,10 +27,16 @@ uuid = { workspace = true } clap = { version = "4", features = ["derive"] } anyhow = "1" base64 = "0.22" +secrecy = "0.10.3" +percent-encoding = "2.3" tokio = { version = "1", features = ["net", "rt", "time", "sync", "macros"] } webbrowser = "1" dialoguer = "0.11" tracing = { workspace = true } +reqwest = { version = "0.13", default-features = false, features = ["rustls", "http2", "json"] } + +[dev-dependencies] +tempfile = "3" [lints] workspace = true diff --git a/crates/hm-plugin-cloud/src/auth/login.rs b/crates/hm-plugin-cloud/src/auth/login.rs index e148e720..4082fd06 100644 --- a/crates/hm-plugin-cloud/src/auth/login.rs +++ b/crates/hm-plugin-cloud/src/auth/login.rs @@ -13,8 +13,9 @@ use std::collections::BTreeMap; use std::time::Duration; -use anyhow::{Result, bail}; +use anyhow::{Context, Result, bail}; use harmont_cloud::{HarmontClient, HarmontError}; +use secrecy::SecretString; use crate::settings; @@ -28,7 +29,10 @@ pub(crate) async fn run(env: &BTreeMap, paste: bool) -> Result<( login_loopback(&client, &app).await? }; - hm_config::creds::set_cloud_token(&api, &token); + hm_core::Sys::load() + .context("loading credentials")? + .creds_mut() + .set(SecretString::from(token.clone())); // Confirm by reading back the authenticated user. let authed = HarmontClient::with_base_url(token, &api); diff --git a/crates/hm-plugin-cloud/src/auth/logout.rs b/crates/hm-plugin-cloud/src/auth/logout.rs index 9cd5d788..7999c83f 100644 --- a/crates/hm-plugin-cloud/src/auth/logout.rs +++ b/crates/hm-plugin-cloud/src/auth/logout.rs @@ -2,13 +2,16 @@ use std::collections::BTreeMap; -use anyhow::Result; +use anyhow::{Context, Result}; use crate::settings; pub(crate) async fn run(_env: &BTreeMap) -> Result<()> { let (_client, api) = settings::anon_client()?; - hm_config::creds::forget_cloud_token(&api); + hm_core::Sys::load() + .context("loading credentials")? + .creds_mut() + .clear(); tracing::info!("logged out of {api}"); Ok(()) } diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index d3a2a2bc..d955e839 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -1,18 +1,49 @@ //! Cloud client builders for the `hm cloud` verbs. //! -//! Config and credentials are owned by the shared [`hm_config`] crate: +//! Config and credentials come from two shared crates: //! -//! - layered config (user `~/.config/hm/config.toml` + project -//! `.hm/config.toml` + `HM_*` env) supplies the API base -//! (`cloud.api_url`) and the active org (`cloud.org`); -//! - bearer tokens live in `hm_config::creds`, keyed by API base, with -//! `HM_API_TOKEN` taking precedence. +//! - [`hm_config`] supplies layered config (user `~/.config/hm/config.toml` + +//! project `.hm/config.toml` + `HM_*` env): the API base (`cloud.api_url`) +//! and the active org (`cloud.org`); +//! - [`hm_core::Sys`] owns the single bearer token, with `HM_API_TOKEN` taking +//! precedence. //! //! This module only assembles an SDK client from that config; it does not own //! any config or credential storage of its own. use anyhow::{Context, Result}; use harmont_cloud::HarmontClient; +use secrecy::{ExposeSecret, SecretString}; + +/// The layered config for the current directory. +/// +/// Includes the project `.hm/config.toml` layer whenever the cwd is inside a +/// harmont project — [`ResolvedCtx::org`]'s own error text tells users to set +/// `[cloud] org` there, so it has to be read. Outside a project there is simply +/// no project layer, and the cloud verbs still work from anywhere. +/// +/// **Read-only callers only.** `hm cloud org switch` deliberately loads the user +/// layer alone: it saves back to the user file, and merging the project layer in +/// first would persist project-scoped values into `~/.config/hm/config.toml`. +fn config() -> Result { + let project = std::env::current_dir() + .ok() + .and_then(|cwd| hm_util::dirs::find_project_root(&cwd)) + .map(|root| hm_config::Config::project_config_path(&root)); + hm_config::Config::load(project.as_deref()).context("loading config") +} + +/// Resolve the bearer token, or the shared "not logged in" error. +/// +/// Every authenticated path routes through here so the not-logged-in text +/// lives in one place. +fn token() -> Result { + hm_core::Sys::load() + .context("loading credentials")? + .creds() + .token() + .context("not logged in — run `hm cloud login` or set HM_API_TOKEN") +} /// Resolved cloud context for the `hm cloud` verbs. #[derive(Debug, Clone)] @@ -43,11 +74,9 @@ impl ResolvedCtx { /// /// Returns an error if config can't be loaded or no token is available. pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { - let cfg = hm_config::Config::load(None).context("loading config")?; // user + env layering + let cfg = config()?; // user + project + env layering let api = cfg.cloud.api_url.clone(); - let token = hm_config::creds::cloud_token(&api) - .context("not logged in — run `hm cloud login` or set HM_API_TOKEN")?; - let client = HarmontClient::with_base_url(token, &api); + let client = HarmontClient::with_base_url(token()?.expose_secret(), &api); Ok(( client, ResolvedCtx { @@ -63,7 +92,7 @@ pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { /// /// Returns an error if config can't be loaded. pub fn anon_client() -> Result<(HarmontClient, String)> { - let cfg = hm_config::Config::load(None).context("loading config")?; + let cfg = config()?; let api = cfg.cloud.api_url.clone(); Ok((HarmontClient::anonymous(&api), api)) } diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index 6d9f76fe..cdf3708d 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -27,6 +27,9 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .iter() .find(|o| o.slug == slug) .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; + // Load-mutate-save on the *user* config, so this deliberately does NOT use + // `settings::config()`: merging the project `.hm/config.toml` layer in first + // would persist project-scoped values into ~/.config/hm/config.toml. let mut cfg = hm_config::Config::load(None)?; cfg.cloud.org = Some(found.slug.clone()); cfg.save_user().context("saving config")?; diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml index 55ed84a6..da244b38 100644 --- a/crates/hm-util/Cargo.toml +++ b/crates/hm-util/Cargo.toml @@ -8,6 +8,7 @@ description = "Shared OS and filesystem utilities for Harmont crates." [dependencies] anyhow = { workspace = true } +derive_more = { workspace = true } dirs = "6" tempfile = "3" tokio = { version = "1", features = ["rt", "rt-multi-thread", "fs", "io-util"] } diff --git a/crates/hm-util/src/lib.rs b/crates/hm-util/src/lib.rs index c5284c52..a950e2c7 100644 --- a/crates/hm-util/src/lib.rs +++ b/crates/hm-util/src/lib.rs @@ -1,2 +1,3 @@ pub mod dirs; pub mod os; +pub mod path; diff --git a/crates/hm-util/src/path/abs_path.rs b/crates/hm-util/src/path/abs_path.rs new file mode 100644 index 00000000..4acc1aac --- /dev/null +++ b/crates/hm-util/src/path/abs_path.rs @@ -0,0 +1,150 @@ +//! A path that is guaranteed to be absolute. +//! +//! [`AbsPath`] / [`AbsPathBuf`] mirror the [`Path`] / [`PathBuf`] split from +//! `std`, adding the invariant that the path is absolute. + +use std::borrow::Borrow; +use std::ffi::OsStr; +use std::fmt; +use std::path::{Components, Path, PathBuf}; + +/// A borrowed path that is guaranteed to be absolute. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(derive_more::AsRef)] +#[as_ref(forward)] +pub struct AbsPath<'a>(&'a Path); + +impl<'a> AbsPath<'a> { + /// Wrap `path` if it is absolute, otherwise return `None`. + pub fn new + ?Sized>(path: &'a P) -> Option { + let p = path.as_ref(); + if p.is_absolute() { + Some(Self(p)) + } else { + None + } + } + + /// Join a path component onto this absolute path. + /// + /// The result is always absolute: if `tail` is absolute it replaces `self` + /// (same as [`Path::join`]); otherwise it is appended. + #[must_use] + pub fn join>(&self, tail: P) -> AbsPathBuf { + AbsPathBuf(self.0.join(tail)) + } + + /// Return the parent directory, if any, as an [`AbsPath`]. + #[must_use] + pub fn parent(&self) -> Option> { + self.0.parent().map(|p| Self(p)) + } + + /// The final component of this path, if any. + #[must_use] + pub fn file_name(&self) -> Option<&OsStr> { + self.0.file_name() + } + + /// An iterator over the components of this path. + #[must_use] + pub fn components(&self) -> Components<'_> { + self.0.components() + } + + /// Tests whether this path points to an existing entity on disk. + #[must_use] + pub fn exists(&self) -> bool { + self.0.exists() + } + + /// Tests whether this path points to a directory. + #[must_use] + pub fn is_dir(&self) -> bool { + self.0.is_dir() + } + + /// Tests whether this path points to a regular file. + #[must_use] + pub fn is_file(&self) -> bool { + self.0.is_file() + } + + /// Yield the underlying [`Path`]. + #[must_use] + pub fn as_path(&self) -> &Path { + self.0 + } + + /// Convert to an owned [`AbsPathBuf`]. + #[must_use] + pub fn to_abs_path_buf(&self) -> AbsPathBuf { + AbsPathBuf(self.0.to_path_buf()) + } +} + +impl fmt::Display for AbsPath<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0.display(), f) + } +} + +impl<'a> From<&'a AbsPathBuf> for AbsPath<'a> { + fn from(buf: &'a AbsPathBuf) -> Self { + buf.as_abs_path() + } +} + +/// An owned path that is guaranteed to be absolute. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(derive_more::Deref, derive_more::AsRef, derive_more::Into)] +#[deref(forward)] +#[as_ref(forward)] +pub struct AbsPathBuf(PathBuf); + +impl AbsPathBuf { + /// Wrap `path` if it is absolute, otherwise return `None`. + pub fn new(path: PathBuf) -> Option { + if path.is_absolute() { + Some(Self(path)) + } else { + None + } + } + + /// Join a path component onto this absolute path. + /// + /// The result is always absolute: if `tail` is absolute it replaces `self` + /// (same as [`Path::join`]); otherwise it is appended. + /// + /// This inherent method shadows [`Path::join`] from the [`Deref`] target so + /// the absolute invariant is preserved without re-wrapping. + #[must_use] + pub fn join>(&self, tail: P) -> AbsPathBuf { + AbsPathBuf(self.0.join(tail)) + } + + /// Borrow as an [`AbsPath`]. + #[must_use] + pub fn as_abs_path(&self) -> AbsPath<'_> { + AbsPath(self.0.as_path()) + } + + /// Consume and return the inner [`PathBuf`]. + #[must_use] + pub fn into_path_buf(self) -> PathBuf { + self.0 + } +} + +impl Borrow for AbsPathBuf { + fn borrow(&self) -> &Path { + self.0.as_path() + } +} + +impl fmt::Display for AbsPathBuf { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0.display(), f) + } +} diff --git a/crates/hm-util/src/path/mod.rs b/crates/hm-util/src/path/mod.rs new file mode 100644 index 00000000..e9ab8da0 --- /dev/null +++ b/crates/hm-util/src/path/mod.rs @@ -0,0 +1,5 @@ +pub mod abs_path; +pub mod rel_path; + +pub use abs_path::{AbsPath, AbsPathBuf}; +pub use rel_path::{RelPath, RelPathBuf}; diff --git a/crates/hm-util/src/path/rel_path.rs b/crates/hm-util/src/path/rel_path.rs new file mode 100644 index 00000000..dfd770d1 --- /dev/null +++ b/crates/hm-util/src/path/rel_path.rs @@ -0,0 +1,111 @@ +//! A path that is guaranteed to be relative. +//! +//! [`RelPath`] / [`RelPathBuf`] mirror the [`Path`] / [`PathBuf`] split from +//! `std`, adding the invariant that the path is relative. + +use std::borrow::Borrow; +use std::ffi::OsStr; +use std::fmt; +use std::path::{Components, Path, PathBuf}; + +/// A borrowed path that is guaranteed to be relative. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(derive_more::AsRef)] +#[as_ref(forward)] +pub struct RelPath<'a>(&'a Path); + +impl<'a> RelPath<'a> { + /// Wrap `path` if it is relative, otherwise return `None`. + pub fn new + ?Sized>(path: &'a P) -> Option { + let p = path.as_ref(); + if p.is_relative() { + Some(Self(p)) + } else { + None + } + } + + /// Append another path segment. + #[must_use] + pub fn join>(&self, tail: P) -> PathBuf { + self.0.join(tail) + } + + /// The final component of this path, if any. + #[must_use] + pub fn file_name(&self) -> Option<&OsStr> { + self.0.file_name() + } + + /// An iterator over the components of this path. + pub fn components(&self) -> Components<'_> { + self.0.components() + } + + /// Yield the underlying [`Path`]. + #[must_use] + pub const fn as_path(&self) -> &Path { + self.0 + } + + /// Convert to an owned [`RelPathBuf`]. + #[must_use] + pub fn to_rel_path_buf(&self) -> RelPathBuf { + RelPathBuf(self.0.to_path_buf()) + } +} + +impl fmt::Display for RelPath<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0.display(), f) + } +} + +impl<'a> From<&'a RelPathBuf> for RelPath<'a> { + fn from(buf: &'a RelPathBuf) -> Self { + buf.as_rel_path() + } +} + +/// An owned path that is guaranteed to be relative. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(derive_more::Deref, derive_more::AsRef, derive_more::Into)] +#[deref(forward)] +#[as_ref(forward)] +pub struct RelPathBuf(PathBuf); + +impl RelPathBuf { + /// Wrap `path` if it is relative, otherwise return `None`. + #[must_use] + pub fn new(path: PathBuf) -> Option { + if path.is_relative() { + Some(Self(path)) + } else { + None + } + } + + /// Borrow as a [`RelPath`]. + #[must_use] + pub fn as_rel_path(&self) -> RelPath<'_> { + RelPath(self.0.as_path()) + } + + /// Consume and return the inner [`PathBuf`]. + #[must_use] + pub fn into_path_buf(self) -> PathBuf { + self.0 + } +} + +impl Borrow for RelPathBuf { + fn borrow(&self) -> &Path { + self.0.as_path() + } +} + +impl fmt::Display for RelPathBuf { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0.display(), f) + } +} diff --git a/crates/hm/Cargo.toml b/crates/hm/Cargo.toml index 370de3a4..73df36d6 100644 --- a/crates/hm/Cargo.toml +++ b/crates/hm/Cargo.toml @@ -60,6 +60,8 @@ hm-pipeline-ir = { workspace = true } hm-plugin-protocol = { workspace = true } hm-util = { workspace = true } hm-config = { workspace = true } +hm-core = { workspace = true } +secrecy = "0.10.3" hm-plugin-cloud = { workspace = true } harmont-cloud = { workspace = true } hm-vm = { workspace = true, features = ["docker-backend"] } diff --git a/crates/hm/src/commands/init.rs b/crates/hm/src/commands/init.rs index 2987f400..dbf9ee88 100644 --- a/crates/hm/src/commands/init.rs +++ b/crates/hm/src/commands/init.rs @@ -97,9 +97,10 @@ fn prompt_skills() -> Result { /// /// Silently returns `Ok(())` on any user-cancellation (Esc, Ctrl-C on a prompt). async fn prompt_cloud_registration(dir: &std::path::Path) -> Result<()> { - let cfg = hm_config::Config::load(None).unwrap_or_default(); - let api_url = &cfg.cloud.api_url; - let is_logged_in = hm_config::creds::cloud_token(api_url).is_some(); + let is_logged_in = hm_core::Sys::load() + .ok() + .and_then(|sys| sys.creds().token()) + .is_some(); if !is_logged_in { let want_login = dialoguer::Confirm::new() diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index c3c4e1ae..2d0665ed 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use anyhow::{Context, Result}; +use secrecy::ExposeSecret as _; use hm_dsl_engine::{DslEngine, detect}; @@ -54,7 +55,11 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { // instead of a daemon-connection error. let cloud_creds = if backend_name == "cloud" { let api_url = ctx.config.cloud.api_url.clone(); - let token = hm_config::creds::cloud_token(&api_url).context( + let token = hm_core::Sys::load() + .context("loading credentials")? + .creds() + .token() + .context( "`hm run --backend cloud` requires authentication — run `hm cloud login` or set HM_API_TOKEN", )?; let org = args @@ -88,7 +93,8 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { let mut autocreate_client: Option<(harmont_cloud::HarmontClient, String)> = None; let backend: Box = if let Some((api_url, token, org)) = cloud_creds { - let client = harmont_cloud::HarmontClient::with_base_url(token, &api_url); + let client = + harmont_cloud::HarmontClient::with_base_url(token.expose_secret(), &api_url); autocreate_client = Some((client.clone(), org.clone())); // The watch link must point at the dashboard (app.) host, not the // API host — a link built from `api_url` lands on raw JSON. diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 846b85da..40e2b342 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -20,9 +20,6 @@ pub mod commands; /// keep resolving. The layered config + credential store now live in /// `hm-config` so `hm-plugin-cloud` can share them. pub use hm_config as config; -/// Re-export the credential store under the historical -/// `harmont_cli::creds_store` path. -pub use hm_config::creds as creds_store; pub mod context; pub mod error; pub(crate) mod signal; From c83217884f4c15279804c5654ad0a4c10da03169 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:09:07 -0700 Subject: [PATCH 2/5] refactor(workspace): add Workspace and unify root resolution RunContext held a bare Config, and three verbs each hand-rolled the same root resolution with no validation and no shared rule: let root = match args.dir { Some(d) => d, None => current_dir()? }; in cli/render.rs, cli/pipelines.rs and commands/run/mod.rs. Introduce hm_core::Workspace -- a validated project root (the directory containing `.hm/`) plus its layered config -- and give it the single resolution rule via Workspace::resolve: - `--dir` names the project root verbatim, with no walk-up. The backend runs discovery against cloned repo roots, and walking up out of a clone could bind to an unrelated `.hm/` above it. A relative `--dir` is resolved against the cwd, since a workspace root must be absolute. - No `--dir`: walk up from the cwd, so a verb works from any subdirectory. RunContext now carries the Workspace and is built only for `hm run`. It was previously constructed unconditionally in main.rs, which would have made every other verb (`hm version`, `hm cloud login`) require a project once a workspace became mandatory. Fixes a divergence in `hm run`: render_pipeline took a &RunContext, ignored it (the parameter was named `_ctx`), and recomputed the root from `--dir`/cwd. That PathBuf became req.repo_root and propagated into the scheduler, the archive builder and the cloud backend -- so with `--dir`, config came from one tree and code from another. It now uses ctx.workspace.path(). As a side effect `hm run --dir X` works from a cwd outside any workspace, where it previously failed with "no harmont workspace found". Behaviour change: `hm render` and `hm pipelines` now walk up when given no `--dir`, where they were previously cwd-exact and failed in subdirectories. Discovery is unaffected -- the backend always passes `--dir`. `hm pipelines` keeps its contract that a repo with no `.hm/` yields the empty envelope rather than an error. Only NotFound/InvalidPath/InvalidWorkspace map to it; a malformed `.hm/config.toml` still fails loudly instead of masquerading as a repo with no pipelines. --- crates/hm/src/cli/mod.rs | 10 ++++++++-- crates/hm/src/cli/pipelines.rs | 15 +++++++++++--- crates/hm/src/cli/render.rs | 6 ++---- crates/hm/src/commands/run/mod.rs | 20 +++++++++---------- crates/hm/src/context.rs | 33 ++++++++++++++++++------------- crates/hm/src/main.rs | 4 +--- 6 files changed, 52 insertions(+), 36 deletions(-) diff --git a/crates/hm/src/cli/mod.rs b/crates/hm/src/cli/mod.rs index 7ea088ff..a13efa62 100644 --- a/crates/hm/src/cli/mod.rs +++ b/crates/hm/src/cli/mod.rs @@ -97,13 +97,19 @@ pub struct CacheRestoreArgs { /// Dispatch a parsed CLI command to the appropriate handler. Returns an exit code. /// +/// [`RunContext`] is loaded only for [`Command::Run`] — other verbs do not +/// require a project workspace (e.g. `hm cloud login`, `hm version`). +/// /// # Errors /// /// Returns an error if the dispatched handler fails. -pub async fn dispatch(command: Command, ctx: RunContext) -> Result { +pub async fn dispatch(command: Command, cli: &Cli) -> Result { match command { Command::Init(args) => crate::commands::init::handle(args).await.map(|()| 0), - Command::Run(args) => crate::commands::run::handle(args, ctx).await, + Command::Run(args) => { + let ctx = RunContext::from_cli(cli, args.dir.as_deref())?; + crate::commands::run::handle(args, ctx).await + } Command::Pipelines(args) => crate::cli::pipelines::run(args).await.map(|()| 0), Command::Render(args) => crate::cli::render::run(args).await.map(|()| 0), Command::Cache(cmd) => match cmd { diff --git a/crates/hm/src/cli/pipelines.rs b/crates/hm/src/cli/pipelines.rs index 5ee250ee..223ace30 100644 --- a/crates/hm/src/cli/pipelines.rs +++ b/crates/hm/src/cli/pipelines.rs @@ -28,10 +28,19 @@ const EMPTY_ENVELOPE: &str = r#"{"schema_version":"1","pipelines":[]}"#; /// Returns an error if the engine can't start or the DSL runtime fails to /// evaluate the pipelines. pub async fn run(args: PipelinesArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, + use hm_core::WorkspaceLoadError as WsErr; + let workspace = match hm_core::Workspace::resolve(args.dir.as_deref()) { + Ok(w) => w, + // "Not a harmont project" is not an error here — it means this repo + // declares no pipelines. A malformed `.hm/config.toml` still fails + // loudly rather than masquerading as an empty repo. + Err(WsErr::NotFound | WsErr::InvalidPath(_) | WsErr::InvalidWorkspace(_)) => { + print!("{EMPTY_ENVELOPE}"); + return Ok(()); + } + Err(e) => return Err(e.into()), }; + let repo_root = workspace.path().as_path().to_path_buf(); if !detect::has_pipeline_files(&repo_root) { print!("{EMPTY_ENVELOPE}"); diff --git a/crates/hm/src/cli/render.rs b/crates/hm/src/cli/render.rs index cc10ca99..6b45f15b 100644 --- a/crates/hm/src/cli/render.rs +++ b/crates/hm/src/cli/render.rs @@ -26,10 +26,8 @@ pub struct RenderArgs { /// or the slug is unknown / fails to render (the available slugs are written to /// stderr by the DSL runtime). pub async fn run(args: RenderArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + let workspace = hm_core::Workspace::resolve(args.dir.as_deref())?; + let repo_root = workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).context("detecting pipeline language")?; let engine = python_engine().context("initializing DSL engine")?; diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index 2d0665ed..a83265a1 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -18,7 +18,7 @@ use crate::error::{ErrorCategory, HmError}; /// Backend resolution (flag wins over config): /// - `--backend ` → that backend (`cloud`, `docker`, …) /// - `--cloud` → `cloud` (deprecated alias) -/// - neither → `ctx.config.backend` (figment-layered, default `docker`) +/// - neither → `ctx.workspace.config().backend` (figment-layered, default `docker`) /// /// This is a THIN driver over the `hm-exec` backends: it builds an /// [`hm_exec::ExecutionBackend`], renders the pipeline to v0 IR once, starts @@ -45,7 +45,7 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { None } }) - .unwrap_or_else(|| ctx.config.backend.to_string()); + .unwrap_or_else(|| ctx.workspace.config().backend.to_string()); // 2. Cloud needs auth + org resolution up front — fail fast on a missing // token before any render work. We resolve the credentials here but @@ -54,7 +54,7 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { // missing/ambiguous pipeline argument fails with a helpful message // instead of a daemon-connection error. let cloud_creds = if backend_name == "cloud" { - let api_url = ctx.config.cloud.api_url.clone(); + let api_url = ctx.workspace.config().cloud.api_url.clone(); let token = hm_core::Sys::load() .context("loading credentials")? .creds() @@ -65,7 +65,7 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { let org = args .org .clone() - .or_else(|| ctx.config.cloud.org.clone()) + .or_else(|| ctx.workspace.config().cloud.org.clone()) .context("no organization — pass --org or set `[cloud] org = \"…\"` in .hm/config.toml or ~/.config/hm/config.toml")?; Some((api_url, token, org)) } else if backend_name != "docker" { @@ -160,7 +160,7 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { // interactively now and its slug persisted. A worktree WITH a remote falls // through to the repo-identity submit + get-or-create fallback below. if let Some((client, org)) = autocreate_client.as_ref() { - if let Some(slug) = ctx.config.cloud.pipeline.clone() { + if let Some(slug) = ctx.workspace.config().cloud.pipeline.clone() { req.cloud_pipeline_slug = Some(slug); } else if req.source.repo_name.is_none() { let default_branch = @@ -344,12 +344,12 @@ fn git_remote_repo_name(root: &std::path::Path) -> Option { /// the DSL detection / pipeline-render step fails. async fn render_pipeline( args: &RunArgs, - _ctx: &RunContext, + ctx: &RunContext, ) -> Result<(std::path::PathBuf, String, String)> { - let repo_root = match args.dir.clone() { - Some(p) => p, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + // The root the run executes against is the workspace's, not a second + // resolution from `--dir`/cwd — otherwise config could come from one tree + // and code from another. `RunContext` already resolved `--dir`. + let repo_root = ctx.workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).map_err(|e| HmError::DslEngine(format!("{e:#}")))?; let engine = diff --git a/crates/hm/src/context.rs b/crates/hm/src/context.rs index 82194497..71c6bcb4 100644 --- a/crates/hm/src/context.rs +++ b/crates/hm/src/context.rs @@ -1,20 +1,20 @@ use std::io::IsTerminal; - -use anyhow::{Context, Result}; +use std::path::Path; use crate::cli::Cli; -use crate::config::Config; +use hm_core::{Workspace, WorkspaceLoadError}; use hm_render::OutputMode; -/// Runtime context that bundles resolved config and output preferences. +/// Runtime context for commands that operate on a harmont project workspace. /// /// After the plan-4 cloud-plugin cutover this is intentionally thin: /// API client, credential store, and active-org resolution moved into -/// `hm-plugin-cloud`. The host context retains the config file (for -/// future use) and the output mode. +/// `hm-plugin-cloud`. Project config and well-known paths live on +/// [`Self::workspace`]; output mode is host-owned. #[derive(Debug)] pub struct RunContext { - pub config: Config, + /// Loaded project workspace (paths + layered config). + pub workspace: Workspace, /// Output mode for the residual built-in verbs (the legacy global /// `--format` flag was retired in plan 3; per-subcommand `--format` /// is the only currently-wired source, so this defaults to human). @@ -22,15 +22,20 @@ pub struct RunContext { } impl RunContext { - /// Build a [`RunContext`] from parsed CLI args. + /// Build a [`RunContext`] from parsed CLI args and the verb's `--dir`. + /// + /// Root resolution is [`Workspace::resolve`]'s, shared with every other + /// `--dir` verb. Passing `dir` through matters: the workspace this loads is + /// the one the run executes against, so resolving it from the cwd while the + /// run used `--dir` would let config come from one tree and code from + /// another. /// /// # Errors /// - /// Returns an error if the config file is unreadable or malformed. - pub fn from_cli(cli: &Cli) -> Result { - let start_dir = std::env::current_dir().context("cannot determine current directory")?; - let project_root = hm_util::dirs::find_project_root(&start_dir); - let config = Config::load(project_root.as_deref())?; + /// Returns [`WorkspaceLoadError`] if the current directory cannot be + /// determined, no project root is found, or workspace load fails. + pub fn from_cli(cli: &Cli, dir: Option<&Path>) -> Result { + let workspace = Workspace::resolve(dir)?; let output = OutputMode::Human { // Single source of truth for the color/TTY rule (still honors --no-color). @@ -39,6 +44,6 @@ impl RunContext { interactive: std::io::stdout().is_terminal(), }; - Ok(Self { config, output }) + Ok(Self { workspace, output }) } } diff --git a/crates/hm/src/main.rs b/crates/hm/src/main.rs index 84e9c93c..25b2a88f 100644 --- a/crates/hm/src/main.rs +++ b/crates/hm/src/main.rs @@ -11,7 +11,6 @@ use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; use harmont_cli::cli::{self, Cli}; -use harmont_cli::context::RunContext; use harmont_cli::error::{self, HmError}; #[tokio::main] @@ -90,8 +89,7 @@ async fn main() { async fn run(args: Cli) -> Result { let command = args.command.clone(); - let ctx = RunContext::from_cli(&args)?; - cli::dispatch(command, ctx).await + cli::dispatch(command, &args).await } fn build_chrome_layer( From e08f3e26692dc5d501e08fa371adc28b1a377de1 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:27:21 -0700 Subject: [PATCH 3/5] refactor(dirs): return typed absolute paths from directory accessors The platform roots resolve from $HOME/%APPDATA% and are absolute by construction, so `Option` understated them and pushed the absolute-ness proof onto every caller. `Sys::load` was re-validating what was already guaranteed: hm_util::dirs::hm_config_dir() .and_then(AbsPathBuf::new) .ok_or(LoadingError::ConfigDirUnavailable)? which conflated "no config dir" with "not absolute". Now the accessors return `Option` and that collapses to a plain `.ok_or(...)` with one honest failure mode. `find_project_root` takes an `AbsPath` because the walk only terminates meaningfully from an absolute start: a relative start would walk to the empty path rather than `/`, and would yield a root that means something different once the cwd changes. Adds `AbsPathBuf::current_dir()` so callers that need a cwd-rooted `AbsPath` don't each re-prove the invariant std already documents. Callers reading these paths through Deref (`cache/clean.rs`, `local/backend.rs`) needed no changes. --- crates/hm-config/src/lib.rs | 4 ++- crates/hm-core/sys.rs | 4 +-- crates/hm-core/workspace.rs | 18 ++++++----- crates/hm-plugin-cloud/src/settings.rs | 7 ++-- crates/hm-util/src/dirs.rs | 45 ++++++++++++++++++-------- crates/hm-util/src/path/abs_path.rs | 34 +++++++++++++++---- crates/hm/tests/config_layered.rs | 8 +++-- 7 files changed, 83 insertions(+), 37 deletions(-) diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index 22d162f6..03b78c8d 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -6,6 +6,8 @@ use std::path::{Path, PathBuf}; +use hm_util::path::AbsPathBuf; + use figment::{ Figment, providers::{Env, Format, Serialized, Toml}, @@ -130,7 +132,7 @@ impl Config { /// /// Returns `None` if the platform config directory cannot be determined. #[must_use] - pub fn user_config_path() -> Option { + pub fn user_config_path() -> Option { hm_util::dirs::hm_config_dir().map(|d| d.join("config.toml")) } diff --git a/crates/hm-core/sys.rs b/crates/hm-core/sys.rs index 80b0f35c..348eb71d 100644 --- a/crates/hm-core/sys.rs +++ b/crates/hm-core/sys.rs @@ -82,9 +82,7 @@ impl Sys { /// Returns [`LoadingError`] when the config directory cannot be resolved or /// created, or credentials fail to load. pub fn load() -> Result { - let hm_dir = hm_util::dirs::hm_config_dir() - .and_then(AbsPathBuf::new) - .ok_or(LoadingError::ConfigDirUnavailable)?; + let hm_dir = hm_util::dirs::hm_config_dir().ok_or(LoadingError::ConfigDirUnavailable)?; if !hm_dir.exists() { std::fs::create_dir_all(hm_dir.as_abs_path().as_path()).map_err(|source| { diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs index 23603b90..88ddfbda 100644 --- a/crates/hm-core/workspace.rs +++ b/crates/hm-core/workspace.rs @@ -68,15 +68,17 @@ impl Workspace { /// Returns [`LoadError`] if the cwd cannot be determined, no project root /// is found while walking up, or the resolved root fails [`Self::load`]. pub fn resolve(dir: Option<&Path>) -> Result { - let root = match dir { - Some(d) if d.is_absolute() => d.to_path_buf(), - Some(d) => std::env::current_dir() - .map_err(LoadError::CurrentDir)? - .join(d), - None => { - let start = std::env::current_dir().map_err(LoadError::CurrentDir)?; - hm_util::dirs::find_project_root(&start).ok_or(LoadError::NotFound)? + let root = if let Some(d) = dir { + if let Some(abs) = AbsPath::new(d) { + abs.to_abs_path_buf() + } else { + AbsPathBuf::current_dir() + .map_err(LoadError::CurrentDir)? + .join(d) } + } else { + let start = AbsPathBuf::current_dir().map_err(LoadError::CurrentDir)?; + hm_util::dirs::find_project_root(start.as_abs_path()).ok_or(LoadError::NotFound)? }; Self::load(&root) } diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index d955e839..a45491cb 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -26,9 +26,10 @@ use secrecy::{ExposeSecret, SecretString}; /// layer alone: it saves back to the user file, and merging the project layer in /// first would persist project-scoped values into `~/.config/hm/config.toml`. fn config() -> Result { - let project = std::env::current_dir() - .ok() - .and_then(|cwd| hm_util::dirs::find_project_root(&cwd)) + let cwd = hm_util::path::AbsPathBuf::current_dir().ok(); + let project = cwd + .as_ref() + .and_then(|cwd| hm_util::dirs::find_project_root(cwd.as_abs_path())) .map(|root| hm_config::Config::project_config_path(&root)); hm_config::Config::load(project.as_deref()).context("loading config") } diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs index ccdcebc7..1da17213 100644 --- a/crates/hm-util/src/dirs.rs +++ b/crates/hm-util/src/dirs.rs @@ -8,33 +8,44 @@ #![allow(clippy::must_use_candidate)] -use std::path::PathBuf; - use crate::os::dirs as platform; +use crate::path::{AbsPath, AbsPathBuf}; /// `~/.config/hm/` — user config root (`config.toml`, `credentials.toml`). -pub fn hm_config_dir() -> Option { - platform::config_dir().map(|c| c.join("hm")) +/// +/// `None` means the platform has no config directory. The platform roots +/// resolve from `$HOME`/`%APPDATA%` and so are absolute by construction. +pub fn hm_config_dir() -> Option { + platform::config_dir() + .map(|c| c.join("hm")) + .and_then(AbsPathBuf::new) } /// `~/.cache/hm/` — local build cache root (regenerable). -pub fn hm_cache_dir() -> Option { - platform::cache_dir().map(|c| c.join("hm")) +pub fn hm_cache_dir() -> Option { + platform::cache_dir() + .map(|c| c.join("hm")) + .and_then(AbsPathBuf::new) } /// `~/.cache/hm/workspaces/` — COW workspace cache root. -pub fn hm_workspace_cache_dir() -> Option { +pub fn hm_workspace_cache_dir() -> Option { hm_cache_dir().map(|c| c.join("workspaces")) } /// Walk up from `start` looking for a directory containing `.hm/`. /// Returns the project root (the directory *containing* `.hm/`), /// or `None` if the filesystem root is reached without finding one. -pub fn find_project_root(start: &std::path::Path) -> Option { +/// +/// Takes an [`AbsPath`] because the walk only terminates meaningfully from an +/// absolute start: a relative `start` would walk to the empty path rather than +/// `/`, and would yield a relative root that means something different once the +/// cwd changes. Absolute in, absolute out. +pub fn find_project_root(start: AbsPath<'_>) -> Option { let mut current = start; loop { if current.join(".hm").is_dir() { - return Some(current.to_path_buf()); + return Some(current.to_abs_path_buf()); } current = current.parent()?; } @@ -43,6 +54,8 @@ pub fn find_project_root(start: &std::path::Path) -> Option { #[cfg(test)] #[allow(clippy::unwrap_used)] mod tests { + use std::path::Path; + use super::*; #[test] @@ -68,12 +81,16 @@ mod tests { assert!(p.ends_with("hm/workspaces"), "got {p:?}"); } + fn abs(p: &Path) -> AbsPath<'_> { + AbsPath::new(p).unwrap() + } + #[test] fn find_project_root_at_current_dir() { let tmp = tempfile::tempdir().unwrap(); std::fs::create_dir(tmp.path().join(".hm")).unwrap(); - let found = find_project_root(tmp.path()); - assert_eq!(found, Some(tmp.path().to_path_buf())); + let found = find_project_root(abs(tmp.path())); + assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); } #[test] @@ -82,14 +99,14 @@ mod tests { std::fs::create_dir(tmp.path().join(".hm")).unwrap(); let nested = tmp.path().join("src").join("deep"); std::fs::create_dir_all(&nested).unwrap(); - let found = find_project_root(&nested); - assert_eq!(found, Some(tmp.path().to_path_buf())); + let found = find_project_root(abs(&nested)); + assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); } #[test] fn find_project_root_returns_none_when_missing() { let tmp = tempfile::tempdir().unwrap(); - let found = find_project_root(tmp.path()); + let found = find_project_root(abs(tmp.path())); assert_eq!(found, None); } } diff --git a/crates/hm-util/src/path/abs_path.rs b/crates/hm-util/src/path/abs_path.rs index 4acc1aac..a09a4235 100644 --- a/crates/hm-util/src/path/abs_path.rs +++ b/crates/hm-util/src/path/abs_path.rs @@ -16,6 +16,7 @@ pub struct AbsPath<'a>(&'a Path); impl<'a> AbsPath<'a> { /// Wrap `path` if it is absolute, otherwise return `None`. + #[must_use] pub fn new + ?Sized>(path: &'a P) -> Option { let p = path.as_ref(); if p.is_absolute() { @@ -36,8 +37,8 @@ impl<'a> AbsPath<'a> { /// Return the parent directory, if any, as an [`AbsPath`]. #[must_use] - pub fn parent(&self) -> Option> { - self.0.parent().map(|p| Self(p)) + pub fn parent(&self) -> Option { + self.0.parent().map(Self) } /// The final component of this path, if any. @@ -47,7 +48,6 @@ impl<'a> AbsPath<'a> { } /// An iterator over the components of this path. - #[must_use] pub fn components(&self) -> Components<'_> { self.0.components() } @@ -72,7 +72,7 @@ impl<'a> AbsPath<'a> { /// Yield the underlying [`Path`]. #[must_use] - pub fn as_path(&self) -> &Path { + pub const fn as_path(&self) -> &Path { self.0 } @@ -104,6 +104,7 @@ pub struct AbsPathBuf(PathBuf); impl AbsPathBuf { /// Wrap `path` if it is absolute, otherwise return `None`. + #[must_use] pub fn new(path: PathBuf) -> Option { if path.is_absolute() { Some(Self(path)) @@ -112,6 +113,27 @@ impl AbsPathBuf { } } + /// The process working directory. + /// + /// [`std::env::current_dir`] is documented to return an absolute path, so + /// the invariant holds by construction. This exists so callers that need a + /// cwd-rooted [`AbsPath`] don't each have to re-prove that. + /// + /// # Errors + /// + /// Returns the underlying [`std::env::current_dir`] error, or + /// [`std::io::ErrorKind::InvalidData`] in the should-not-happen case where + /// the platform hands back a relative path. + pub fn current_dir() -> std::io::Result { + let cwd = std::env::current_dir()?; + Self::new(cwd).ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "current directory is not an absolute path", + ) + }) + } + /// Join a path component onto this absolute path. /// /// The result is always absolute: if `tail` is absolute it replaces `self` @@ -120,8 +142,8 @@ impl AbsPathBuf { /// This inherent method shadows [`Path::join`] from the [`Deref`] target so /// the absolute invariant is preserved without re-wrapping. #[must_use] - pub fn join>(&self, tail: P) -> AbsPathBuf { - AbsPathBuf(self.0.join(tail)) + pub fn join>(&self, tail: P) -> Self { + Self(self.0.join(tail)) } /// Borrow as an [`AbsPath`]. diff --git a/crates/hm/tests/config_layered.rs b/crates/hm/tests/config_layered.rs index 19bb03b0..9871a676 100644 --- a/crates/hm/tests/config_layered.rs +++ b/crates/hm/tests/config_layered.rs @@ -104,8 +104,12 @@ fn load_resolves_project_root() { ) .unwrap(); - let found = hm_util::dirs::find_project_root(project_dir.path()); - assert_eq!(found, Some(project_dir.path().to_path_buf())); + let start = hm_util::path::AbsPath::new(project_dir.path()).unwrap(); + let found = hm_util::dirs::find_project_root(start); + assert_eq!( + found, + hm_util::path::AbsPathBuf::new(project_dir.path().to_path_buf()) + ); let config_path = harmont_cli::config::Config::project_config_path(project_dir.path()); assert_eq!(config_path, harmont_dir.join("config.toml")); From dca12ff212283fbc48fc862113153ba03ddc628f Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 23:47:13 -0700 Subject: [PATCH 4/5] refactor(dirs): delete hm-util::dirs, split its policy by scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hm-util::dirs` was three unrelated policies wearing one hat, and none of them were platform utilities: - `hm_config_dir` / `hm_cache_dir` / `hm_workspace_cache_dir` — where *this user's* hm state lives. That is `Sys`, whose own doc already claimed the ground: "the scope of the current user. This includes the user's configuration directory". `Sys::load` was already resolving the config dir and storing it as `hm_dir`. - `find_project_root` — the project-scope walk-up rule, i.e. the discovery half of `Workspace::resolve`, which was the only real caller. What the platform actually tells us — `~/.config`, `~/.cache`, `$HOME` — stays in hm-util as `os::dirs`, now `pub`. Joining `hm/` onto those is our decision, not the platform's, so it moves to `Sys`. The new `Sys` path accessors are associated fns: resolution only, no I/O, no `Sys::load`. `Workspace::load` needs the user config path, not a set of credentials — `hm render` should not create `~/.config/hm` and read `credentials.toml` just to layer config. Two forced consequences: - `hm-config` loses `Config::load` / `save_user` / `user_config_path`. They need the config dir, the config dir now lives in hm-core, and hm-core already depends on hm-config — the reverse edge is a cycle. `load_from_paths` / `save_to` already existed; callers now name their layers explicitly, which makes `org switch`'s user-file-only rule visible at the call site instead of a comment warning about it. - `LocalBackend::new` takes the cache dir. hm-exec reaching into `~/.cache/hm` broke the crate's own doctrine ("Auth is injected: this crate takes a pre-built HarmontClient; it never reads credentials from disk"). Injecting also stops the contract test from writing a snapshot registry into the developer's real cache. Verified end-to-end against a fake $HOME: the user layer alone selects `backend = cloud`, and a project `.hm/config.toml` overrides it back. --- crates/hm-config/src/lib.rs | 42 +-------- crates/hm-core/sys.rs | 74 ++++++++++++++- crates/hm-core/workspace.rs | 67 +++++++++++++- crates/hm-exec/src/local/backend.rs | 19 +++- crates/hm-exec/tests/backend_contract.rs | 2 + crates/hm-plugin-cloud/src/settings.rs | 5 +- crates/hm-plugin-cloud/src/verbs/org.rs | 11 ++- crates/hm-util/src/dirs.rs | 112 ----------------------- crates/hm-util/src/lib.rs | 1 - crates/hm-util/src/os/dirs.rs | 28 ++++-- crates/hm-util/src/os/mod.rs | 2 +- crates/hm/src/commands/cache/clean.rs | 4 +- crates/hm/src/commands/run/mod.rs | 3 + crates/hm/src/lib.rs | 9 +- crates/hm/tests/config_layered.rs | 2 +- 15 files changed, 195 insertions(+), 186 deletions(-) delete mode 100644 crates/hm-util/src/dirs.rs diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index 03b78c8d..513f0166 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -6,8 +6,6 @@ use std::path::{Path, PathBuf}; -use hm_util::path::AbsPathBuf; - use figment::{ Figment, providers::{Env, Format, Serialized, Toml}, @@ -30,10 +28,6 @@ pub enum LoadError { #[derive(Debug, thiserror::Error)] #[non_exhaustive] pub enum SaveError { - /// Platform config directory (`~/.config/hm` or equivalent) could not be resolved. - #[error("could not determine config directory")] - ConfigDirUnavailable, - /// TOML serialization of a [`Config`] failed. #[error("serializing config")] Serialize(#[from] toml::ser::Error), @@ -128,36 +122,13 @@ pub struct Config { } impl Config { - /// XDG-aware user config path (`~/.config/hm/config.toml`). - /// - /// Returns `None` if the platform config directory cannot be determined. - #[must_use] - pub fn user_config_path() -> Option { - hm_util::dirs::hm_config_dir().map(|d| d.join("config.toml")) - } - /// Project-level config path: `/.hm/config.toml`. #[must_use] pub fn project_config_path(project_root: &Path) -> PathBuf { project_root.join(".hm").join("config.toml") } - /// Load configuration with full layering: defaults -> user file -> project file -> env. - /// - /// `project_config_path` is the path to the project-level config file - /// (typically `.hm/config.toml`), not the project root. Pass `None` to - /// skip the project layer (user + env only). When the user config path - /// cannot be resolved, the user layer is skipped as well. - /// - /// # Errors - /// - /// Returns [`LoadError`] if figment extraction fails (malformed TOML, type - /// mismatches). - pub fn load(project_config_path: Option<&Path>) -> Result { - Self::load_from_paths(Self::user_config_path().as_deref(), project_config_path) - } - - /// Testable core: build a `Config` from explicit file paths. + /// Build a `Config` from explicit file paths. /// /// Layering, lowest to highest precedence: defaults -> user file -> /// project file -> env. @@ -210,17 +181,6 @@ impl Config { source, }) } - - /// Save to user-level config path (`~/.config/hm/config.toml`). - /// - /// # Errors - /// - /// Returns [`SaveError::ConfigDirUnavailable`] if the path cannot be - /// determined, or any other [`SaveError`] if the write fails. - pub fn save_user(&self) -> Result<(), SaveError> { - let path = Self::user_config_path().ok_or(SaveError::ConfigDirUnavailable)?; - self.save_to(&path) - } } /// Figment env provider mapping the friendly `HM_ORG` / `HM_API_URL` diff --git a/crates/hm-core/sys.rs b/crates/hm-core/sys.rs index 348eb71d..016d96b2 100644 --- a/crates/hm-core/sys.rs +++ b/crates/hm-core/sys.rs @@ -73,6 +73,41 @@ impl Sys { self.hm_dir.as_abs_path() } + /// `~/.config/hm/` — this user's config root. + /// + /// This and its siblings are the `hm`-namespacing policy: the platform only + /// tells us where *configuration* goes ([`hm_util::os::dirs`]); that we put + /// ours in an `hm/` subdirectory is our decision, and a user-scoped one, so + /// it lives here. + /// + /// Resolution only — no I/O, no [`Self::load`] required. Callers that just + /// need a path (layering config, clearing the cache) should not have to + /// create directories and read credentials to get one. + /// + /// `None` means the platform has no config directory. + #[must_use] + pub fn config_dir() -> Option { + hm_util::os::dirs::config_dir().map(|c| c.join("hm")) + } + + /// `~/.config/hm/config.toml` — this user's config file. + #[must_use] + pub fn config_path() -> Option { + Self::config_dir().map(|d| d.join("config.toml")) + } + + /// `~/.cache/hm/` — this user's cache root (regenerable). + #[must_use] + pub fn cache_dir() -> Option { + hm_util::os::dirs::cache_dir().map(|c| c.join("hm")) + } + + /// `~/.cache/hm/workspaces/` — COW workspace cache root. + #[must_use] + pub fn workspace_cache_dir() -> Option { + Self::cache_dir().map(|c| c.join("workspaces")) + } + /// Load process-level system state (credentials, …). /// /// Ensures `~/.config/hm` exists, then loads `credentials.toml` from it. @@ -82,7 +117,7 @@ impl Sys { /// Returns [`LoadingError`] when the config directory cannot be resolved or /// created, or credentials fail to load. pub fn load() -> Result { - let hm_dir = hm_util::dirs::hm_config_dir().ok_or(LoadingError::ConfigDirUnavailable)?; + let hm_dir = Self::config_dir().ok_or(LoadingError::ConfigDirUnavailable)?; if !hm_dir.exists() { std::fs::create_dir_all(hm_dir.as_abs_path().as_path()).map_err(|source| { @@ -109,3 +144,40 @@ impl Sys { &mut self.creds } } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[test] + fn config_dir_is_hm_under_platform_config() { + let p = Sys::config_dir().unwrap(); + assert!(p.ends_with("hm"), "expected path ending in 'hm', got {p}"); + let parent = p.as_abs_path().parent().unwrap(); + assert!( + parent.as_path().ends_with(".config") || parent.as_path().ends_with("AppData/Roaming"), + "unexpected parent: {parent}" + ); + } + + #[test] + fn config_path_is_config_toml_in_config_dir() { + let path = Sys::config_path().unwrap(); + let dir = Sys::config_dir().unwrap(); + assert_eq!(path.as_abs_path().parent().unwrap(), dir.as_abs_path()); + assert!(path.ends_with("config.toml"), "got {path}"); + } + + #[test] + fn cache_dir_is_hm_under_platform_cache() { + let p = Sys::cache_dir().unwrap(); + assert!(p.ends_with("hm"), "expected path ending in 'hm', got {p}"); + } + + #[test] + fn workspace_cache_dir_is_under_cache_dir() { + let p = Sys::workspace_cache_dir().unwrap(); + assert!(p.ends_with("hm/workspaces"), "got {p}"); + } +} diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs index 88ddfbda..6dbee05a 100644 --- a/crates/hm-core/workspace.rs +++ b/crates/hm-core/workspace.rs @@ -7,6 +7,8 @@ use hm_config::Config; use hm_util::path::{AbsPath, AbsPathBuf}; use thiserror::Error; +use crate::sys::Sys; + /// Failure resolving or loading a directory as a [`Workspace`]. #[derive(Debug, Error)] pub enum LoadError { @@ -78,11 +80,34 @@ impl Workspace { } } else { let start = AbsPathBuf::current_dir().map_err(LoadError::CurrentDir)?; - hm_util::dirs::find_project_root(start.as_abs_path()).ok_or(LoadError::NotFound)? + Self::find_root(start.as_abs_path()).ok_or(LoadError::NotFound)? }; Self::load(&root) } + /// Walk up from `start` looking for a directory containing `.hm/`, and + /// return that directory (the project root), or `None` if the filesystem + /// root is reached without finding one. + /// + /// This is the discovery half of [`Self::resolve`], exposed for callers that + /// want to know *whether* they are inside a project without paying to load + /// one — e.g. deciding whether a project config layer exists. + /// + /// Takes an [`AbsPath`] because the walk only terminates meaningfully from + /// an absolute start: a relative `start` would walk to the empty path rather + /// than `/`, and would yield a root that means something different once the + /// cwd changes. Absolute in, absolute out. + #[must_use] + pub fn find_root(start: AbsPath<'_>) -> Option { + let mut current = start; + loop { + if current.join(".hm").is_dir() { + return Some(current.to_abs_path_buf()); + } + current = current.parent()?; + } + } + /// Attempt to load the given directory as a workspace, if it appears to be one. /// /// We label a workspace any directory which has a `.hm` directory within it. @@ -104,7 +129,10 @@ impl Workspace { let hm_dir = AbsPathBuf::new(hm_dir) .ok_or_else(|| LoadError::InvalidPath(workspace_path.to_path_buf()))?; - let config = Config::load(Some(&hm_dir.join("config.toml")))?; + let config = Config::load_from_paths( + Sys::config_path().as_deref(), + Some(&hm_dir.join("config.toml")), + )?; Ok(Self { hm_dir, config }) } @@ -141,3 +169,38 @@ impl Workspace { self.hm_dir().join("secrets") } } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + fn abs(p: &Path) -> AbsPath<'_> { + AbsPath::new(p).unwrap() + } + + #[test] + fn find_root_at_start_dir() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir(tmp.path().join(".hm")).unwrap(); + let found = Workspace::find_root(abs(tmp.path())); + assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); + } + + #[test] + fn find_root_walks_up() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir(tmp.path().join(".hm")).unwrap(); + let nested = tmp.path().join("src").join("deep"); + std::fs::create_dir_all(&nested).unwrap(); + let found = Workspace::find_root(abs(&nested)); + assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); + } + + #[test] + fn find_root_returns_none_when_missing() { + let tmp = tempfile::tempdir().unwrap(); + let found = Workspace::find_root(abs(tmp.path())); + assert_eq!(found, None); + } +} diff --git a/crates/hm-exec/src/local/backend.rs b/crates/hm-exec/src/local/backend.rs index 0713100c..b5c1b902 100644 --- a/crates/hm-exec/src/local/backend.rs +++ b/crates/hm-exec/src/local/backend.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; +use hm_util::path::AbsPathBuf; use hm_vm::{HmVm, ImageRegistry, VmBackend, VmConfig}; use crate::local::{RunnerRegistry, VmRunner}; @@ -30,6 +31,7 @@ const REGISTRY_CAPACITY: NonZeroU64 = NonZeroU64::new(64).expect("64 is non-zero pub struct LocalBackend { parallelism: NonZeroUsize, vm_backend: Arc, + cache_dir: AbsPathBuf, } impl LocalBackend { @@ -38,11 +40,21 @@ impl LocalBackend { /// `parallelism` = max concurrent step chains. The [`NonZeroUsize`] type /// makes the scheduler's semaphore construction deadlock-free by /// construction (a zero-permit semaphore would stall every step). + /// + /// `cache_dir` is where the snapshot registry lives. It is injected for the + /// same reason the [`harmont_cloud::HarmontClient`] is: this crate executes + /// builds, it does not decide where a user's state lives. That also lets + /// tests point it at a tempdir instead of the real `~/.cache/hm`. #[must_use] - pub fn new(parallelism: NonZeroUsize, vm_backend: Arc) -> Self { + pub fn new( + parallelism: NonZeroUsize, + vm_backend: Arc, + cache_dir: AbsPathBuf, + ) -> Self { Self { parallelism, vm_backend, + cache_dir, } } @@ -50,10 +62,7 @@ impl LocalBackend { /// (VM backend + snapshot registry) and registering the [`VmRunner`] as /// the default runner. fn build_registry(&self) -> Result { - let cache_dir = hm_util::dirs::hm_cache_dir().ok_or_else(|| { - BackendError::Local("cannot resolve the Harmont cache directory".into()) - })?; - let registry = ImageRegistry::open(&cache_dir.join("registry.db"), REGISTRY_CAPACITY) + let registry = ImageRegistry::open(&self.cache_dir.join("registry.db"), REGISTRY_CAPACITY) .map_err(|e| BackendError::Local(format!("opening snapshot registry: {e:#}")))?; let config = VmConfig { diff --git a/crates/hm-exec/tests/backend_contract.rs b/crates/hm-exec/tests/backend_contract.rs index 797eccdf..21326911 100644 --- a/crates/hm-exec/tests/backend_contract.rs +++ b/crates/hm-exec/tests/backend_contract.rs @@ -94,9 +94,11 @@ impl hm_vm::VmBackend for NoopVmBackend { #[tokio::test] async fn local_backend_reports_capabilities() { + let cache = tempfile::tempdir().unwrap(); let b = hm_exec::LocalBackend::new( std::num::NonZeroUsize::new(4).unwrap(), std::sync::Arc::new(NoopVmBackend), + hm_util::path::AbsPathBuf::new(cache.path().to_path_buf()).unwrap(), ); assert_eq!(b.name(), "local"); assert!(b.capabilities().honors_parallelism); diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index a45491cb..5900a8b9 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -29,9 +29,10 @@ fn config() -> Result { let cwd = hm_util::path::AbsPathBuf::current_dir().ok(); let project = cwd .as_ref() - .and_then(|cwd| hm_util::dirs::find_project_root(cwd.as_abs_path())) + .and_then(|cwd| hm_core::Workspace::find_root(cwd.as_abs_path())) .map(|root| hm_config::Config::project_config_path(&root)); - hm_config::Config::load(project.as_deref()).context("loading config") + hm_config::Config::load_from_paths(hm_core::Sys::config_path().as_deref(), project.as_deref()) + .context("loading config") } /// Resolve the bearer token, or the shared "not logged in" error. diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index cdf3708d..a6eb5423 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -27,12 +27,13 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .iter() .find(|o| o.slug == slug) .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; - // Load-mutate-save on the *user* config, so this deliberately does NOT use - // `settings::config()`: merging the project `.hm/config.toml` layer in first - // would persist project-scoped values into ~/.config/hm/config.toml. - let mut cfg = hm_config::Config::load(None)?; + // Load-mutate-save on the *user* config, so this deliberately passes no + // project layer: merging `.hm/config.toml` in first would persist + // project-scoped values into ~/.config/hm/config.toml. + let user_path = hm_core::Sys::config_path().context("could not determine config directory")?; + let mut cfg = hm_config::Config::load_from_paths(Some(&user_path), None)?; cfg.cloud.org = Some(found.slug.clone()); - cfg.save_user().context("saving config")?; + cfg.save_to(&user_path).context("saving config")?; tracing::info!("active organization: {} ({})", found.name, found.slug); Ok(()) } diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs deleted file mode 100644 index 1da17213..00000000 --- a/crates/hm-util/src/dirs.rs +++ /dev/null @@ -1,112 +0,0 @@ -//! Harmont-specific directory resolution. -//! -//! Every directory accessor in this module returns an `hm`-namespaced path -//! under an XDG-correct root: configuration in `~/.config/hm/`, regenerable -//! cache in `~/.cache/hm/`. Raw platform primitives (`home_dir`, `config_dir`, -//! `cache_dir`) live in `os::dirs` and are **not** re-exported — callers -//! outside `hm-util` should never need them. - -#![allow(clippy::must_use_candidate)] - -use crate::os::dirs as platform; -use crate::path::{AbsPath, AbsPathBuf}; - -/// `~/.config/hm/` — user config root (`config.toml`, `credentials.toml`). -/// -/// `None` means the platform has no config directory. The platform roots -/// resolve from `$HOME`/`%APPDATA%` and so are absolute by construction. -pub fn hm_config_dir() -> Option { - platform::config_dir() - .map(|c| c.join("hm")) - .and_then(AbsPathBuf::new) -} - -/// `~/.cache/hm/` — local build cache root (regenerable). -pub fn hm_cache_dir() -> Option { - platform::cache_dir() - .map(|c| c.join("hm")) - .and_then(AbsPathBuf::new) -} - -/// `~/.cache/hm/workspaces/` — COW workspace cache root. -pub fn hm_workspace_cache_dir() -> Option { - hm_cache_dir().map(|c| c.join("workspaces")) -} - -/// Walk up from `start` looking for a directory containing `.hm/`. -/// Returns the project root (the directory *containing* `.hm/`), -/// or `None` if the filesystem root is reached without finding one. -/// -/// Takes an [`AbsPath`] because the walk only terminates meaningfully from an -/// absolute start: a relative `start` would walk to the empty path rather than -/// `/`, and would yield a relative root that means something different once the -/// cwd changes. Absolute in, absolute out. -pub fn find_project_root(start: AbsPath<'_>) -> Option { - let mut current = start; - loop { - if current.join(".hm").is_dir() { - return Some(current.to_abs_path_buf()); - } - current = current.parent()?; - } -} - -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use std::path::Path; - - use super::*; - - #[test] - fn hm_config_dir_under_config() { - let p = hm_config_dir().unwrap(); - assert!(p.ends_with("hm"), "expected path ending in 'hm', got {p:?}"); - let parent = p.parent().unwrap(); - assert!( - parent.ends_with(".config") || parent.ends_with("AppData/Roaming"), - "unexpected parent: {parent:?}" - ); - } - - #[test] - fn hm_cache_dir_under_cache() { - let p = hm_cache_dir().unwrap(); - assert!(p.ends_with("hm"), "expected path ending in 'hm', got {p:?}"); - } - - #[test] - fn hm_workspace_cache_dir_resolves() { - let p = hm_workspace_cache_dir().unwrap(); - assert!(p.ends_with("hm/workspaces"), "got {p:?}"); - } - - fn abs(p: &Path) -> AbsPath<'_> { - AbsPath::new(p).unwrap() - } - - #[test] - fn find_project_root_at_current_dir() { - let tmp = tempfile::tempdir().unwrap(); - std::fs::create_dir(tmp.path().join(".hm")).unwrap(); - let found = find_project_root(abs(tmp.path())); - assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); - } - - #[test] - fn find_project_root_walks_up() { - let tmp = tempfile::tempdir().unwrap(); - std::fs::create_dir(tmp.path().join(".hm")).unwrap(); - let nested = tmp.path().join("src").join("deep"); - std::fs::create_dir_all(&nested).unwrap(); - let found = find_project_root(abs(&nested)); - assert_eq!(found, AbsPathBuf::new(tmp.path().to_path_buf())); - } - - #[test] - fn find_project_root_returns_none_when_missing() { - let tmp = tempfile::tempdir().unwrap(); - let found = find_project_root(abs(tmp.path())); - assert_eq!(found, None); - } -} diff --git a/crates/hm-util/src/lib.rs b/crates/hm-util/src/lib.rs index a950e2c7..4ad380e2 100644 --- a/crates/hm-util/src/lib.rs +++ b/crates/hm-util/src/lib.rs @@ -1,3 +1,2 @@ -pub mod dirs; pub mod os; pub mod path; diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs index 9027ad3a..f010088f 100644 --- a/crates/hm-util/src/os/dirs.rs +++ b/crates/hm-util/src/os/dirs.rs @@ -1,30 +1,40 @@ //! Raw platform directory primitives. //! -//! This module is `pub(crate)` — external callers must use -//! [`crate::dirs`] which provides Harmont-specific accessors. +//! These are the unopinionated platform roots — no `hm` namespacing. Where +//! *this* tool keeps *this user's* state (`~/.config/hm`, `~/.cache/hm`, …) is +//! policy, not a platform fact, and lives on `hm_core::Sys`. +//! +//! Each root is absolute by construction: they resolve from `$HOME` / +//! `%APPDATA%`, so `None` means only "the platform has no such directory". //! //! On non-Windows we intentionally hardcode `~/.config` and `~/.cache` rather //! than reading `$XDG_CONFIG_HOME` / `$XDG_CACHE_HOME`. This keeps both //! primitives consistent and our paths predictable; it is deliberate, not an //! oversight. Revisit only if honoring the XDG env vars becomes a real need. -use std::path::PathBuf; +use crate::path::AbsPathBuf; -pub(crate) fn home_dir() -> Option { - dirs::home_dir() +/// The invoking user's home directory. +#[must_use] +pub fn home_dir() -> Option { + dirs::home_dir().and_then(AbsPathBuf::new) } -pub(crate) fn config_dir() -> Option { +/// The platform configuration root (`~/.config`, `%APPDATA%`). +#[must_use] +pub fn config_dir() -> Option { if cfg!(windows) { - dirs::config_dir() + dirs::config_dir().and_then(AbsPathBuf::new) } else { home_dir().map(|h| h.join(".config")) } } -pub(crate) fn cache_dir() -> Option { +/// The platform cache root (`~/.cache`, `%LOCALAPPDATA%`). +#[must_use] +pub fn cache_dir() -> Option { if cfg!(windows) { - dirs::cache_dir() + dirs::cache_dir().and_then(AbsPathBuf::new) } else { home_dir().map(|h| h.join(".cache")) } diff --git a/crates/hm-util/src/os/mod.rs b/crates/hm-util/src/os/mod.rs index 0bc7829b..ac4b93c8 100644 --- a/crates/hm-util/src/os/mod.rs +++ b/crates/hm-util/src/os/mod.rs @@ -1,2 +1,2 @@ -pub(crate) mod dirs; +pub mod dirs; pub mod fs; diff --git a/crates/hm/src/commands/cache/clean.rs b/crates/hm/src/commands/cache/clean.rs index aa71cb02..f21c1e5b 100644 --- a/crates/hm/src/commands/cache/clean.rs +++ b/crates/hm/src/commands/cache/clean.rs @@ -4,7 +4,7 @@ use hm_vm::VmBackend as _; /// # Errors /// Returns an error if workspace cache removal fails. pub async fn handle_clean() -> Result { - let ws_cleaned = if let Some(ws_cache) = hm_util::dirs::hm_workspace_cache_dir() + let ws_cleaned = if let Some(ws_cache) = hm_core::Sys::workspace_cache_dir() && ws_cache.exists() { let size = dir_size(&ws_cache); @@ -19,7 +19,7 @@ pub async fn handle_clean() -> Result { false }; - let db_cleaned = if let Some(cache_dir) = hm_util::dirs::hm_cache_dir() { + let db_cleaned = if let Some(cache_dir) = hm_core::Sys::cache_dir() { let db_path = cache_dir.join("registry.db"); if db_path.exists() { // Remove the backing Docker images BEFORE deleting registry.db. diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index a83265a1..77baf21f 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -105,9 +105,12 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { let vm_backend: std::sync::Arc = std::sync::Arc::new( hm_vm::docker::DockerBackend::connect().map_err(|e| anyhow::anyhow!("{e:#}"))?, ); + let cache_dir = hm_core::Sys::cache_dir() + .context("cannot resolve the Harmont cache directory")?; Box::new(hm_exec::LocalBackend::new( resolve_parallelism(&args), vm_backend, + cache_dir, )) }; diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 40e2b342..33faf50f 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -3,10 +3,11 @@ reason = "transitive dependency version conflicts in rand/windows-sys/thiserror chains; not fixable without upstream updates" )] // The `dirs` crate must NOT be added as a direct dependency of this -// crate. All directory resolution goes through `hm_util::dirs`, which -// owns the `dirs` dependency and provides both platform primitives and -// Harmont-specific discovery. Adding `dirs` here would bypass that -// single source of truth. +// crate. Directory resolution is split by scope and both halves are +// single sources of truth: `hm_util::os::dirs` owns the `dirs` +// dependency and exposes the raw platform roots, and `hm_core::Sys` +// owns where *this user's* hm state lives under them +// (`~/.config/hm`, `~/.cache/hm`). Adding `dirs` here would bypass both. #[allow( clippy::print_stdout, diff --git a/crates/hm/tests/config_layered.rs b/crates/hm/tests/config_layered.rs index 9871a676..081ddc91 100644 --- a/crates/hm/tests/config_layered.rs +++ b/crates/hm/tests/config_layered.rs @@ -105,7 +105,7 @@ fn load_resolves_project_root() { .unwrap(); let start = hm_util::path::AbsPath::new(project_dir.path()).unwrap(); - let found = hm_util::dirs::find_project_root(start); + let found = hm_core::Workspace::find_root(start); assert_eq!( found, hm_util::path::AbsPathBuf::new(project_dir.path().to_path_buf()) From eeec22a87109c6bdd1bb26f3517e679c777f6842 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 23:53:01 -0700 Subject: [PATCH 5/5] refactor(workspace): make find_root private behind Workspace::find MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve` already called `find_root` — there was one walk-up, not two. But `find_root` was public for exactly one caller, `settings::config()`, which then hand-assembled the layering `Workspace::load` already does: // workspace.rs load() Config::load_from_paths(Sys::config_path()…, Some(&hm_dir.join("config.toml"))) // settings.rs config() — same layering, second copy Config::load_from_paths(Sys::config_path()…, project.as_deref()) Two places knew "the user layer is Sys::config_path()", and settings re-derived the project config path Workspace already knows. That is what handing out a bare root invites. `find_root` was public only because `resolve` couldn't say "maybe there is no workspace" — the `hm cloud` verbs legitimately run outside a project. So make absence expressible instead of exposing the primitive: find(dir) -> Result> no workspace = Ok(None) resolve(dir) -> Result no workspace = Err(NotFound) `resolve` is now `find(dir)?.ok_or(NotFound)`, keeping the three CLI call sites unchanged rather than making each repeat the NotFound message. `Ok(None)` means only "the cwd walk-up reached / without finding .hm/"; an explicit `--dir` that is not a workspace stays a user error. Adds `Sys::config()` (user + env, no project layer) as the user-scope counterpart to `Workspace::config()`, so the user-layer wiring lives once. `org switch` now names it, which states its user-file-only intent instead of open-coding the same call the comment had to explain. Verified against a fake $HOME through `hm cloud`: outside a project the api_url comes from the user layer, inside it the project layer overrides, the walk-up works from a subdirectory, and HM_API_URL still beats both. --- crates/hm-core/sys.rs | 20 ++++++++++ crates/hm-core/workspace.rs | 52 ++++++++++++++++++------- crates/hm-plugin-cloud/src/settings.rs | 27 +++++++------ crates/hm-plugin-cloud/src/verbs/org.rs | 11 +++--- crates/hm/tests/config_layered.rs | 27 +++++++++---- 5 files changed, 98 insertions(+), 39 deletions(-) diff --git a/crates/hm-core/sys.rs b/crates/hm-core/sys.rs index 016d96b2..60d9c5f0 100644 --- a/crates/hm-core/sys.rs +++ b/crates/hm-core/sys.rs @@ -12,6 +12,7 @@ pub mod env; use std::io; use std::path::PathBuf; +use hm_config::Config; use hm_util::path::{AbsPath, AbsPathBuf}; use thiserror::Error; @@ -96,6 +97,25 @@ impl Sys { Self::config_dir().map(|d| d.join("config.toml")) } + /// This user's configuration: `~/.config/hm/config.toml` + `HM_*` env, with + /// no project layer. + /// + /// The user-scope counterpart to [`crate::Workspace::config`], which layers + /// a project's `.hm/config.toml` on top of this. Use this one when there is + /// no workspace, or when a project layer would be wrong — `hm cloud org + /// switch` writes back to the user file, so merging the project layer in + /// first would persist project-scoped values into `~/.config/hm/config.toml`. + /// + /// When the config directory cannot be resolved the file layer is skipped; + /// defaults and env still apply. + /// + /// # Errors + /// + /// Returns [`hm_config::LoadError`] if the config file is malformed. + pub fn config() -> Result { + Config::load_from_paths(Self::config_path().as_deref(), None) + } + /// `~/.cache/hm/` — this user's cache root (regenerable). #[must_use] pub fn cache_dir() -> Option { diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs index 6dbee05a..7e1303bc 100644 --- a/crates/hm-core/workspace.rs +++ b/crates/hm-core/workspace.rs @@ -33,10 +33,12 @@ pub enum LoadError { /// Utility for accessing well-known paths inside a workspace. /// -/// Two constructors: +/// Three constructors, one discovery rule: /// /// - [`Workspace::resolve`] — the one CLI verbs want. Honors a `--dir`-style -/// override, else walks up from the cwd. +/// override, else walks up from the cwd; no workspace is an error. +/// - [`Workspace::find`] — same rule, but no workspace is `Ok(None)`, for +/// callers that also work outside a project (the `hm cloud` verbs). /// - [`Workspace::load`] — validates one **specific** project root (the /// directory that contains `.hm/`), with no discovery. #[derive(Debug)] @@ -53,7 +55,7 @@ pub struct Workspace { } impl Workspace { - /// Resolve the workspace a command should operate on. + /// Find the workspace a command should operate on, if there is one. /// /// The single root-resolution rule for every verb that takes a `--dir`: /// @@ -65,11 +67,18 @@ impl Workspace { /// - **`dir` absent** — walk up from the cwd, so a verb works from any /// subdirectory of the project. /// + /// `Ok(None)` means only "the walk-up reached `/` without finding `.hm/`" — + /// for callers that legitimately run outside a project, like the `hm cloud` + /// verbs. An explicit `dir` that is not a workspace is a user error, not an + /// absence, so it still fails with [`LoadError::InvalidWorkspace`]. + /// + /// Verbs that require a workspace want [`Self::resolve`] instead. + /// /// # Errors /// - /// Returns [`LoadError`] if the cwd cannot be determined, no project root - /// is found while walking up, or the resolved root fails [`Self::load`]. - pub fn resolve(dir: Option<&Path>) -> Result { + /// Returns [`LoadError`] if the cwd cannot be determined, or the resolved + /// root fails [`Self::load`]. + pub fn find(dir: Option<&Path>) -> Result, LoadError> { let root = if let Some(d) = dir { if let Some(abs) = AbsPath::new(d) { abs.to_abs_path_buf() @@ -80,25 +89,42 @@ impl Workspace { } } else { let start = AbsPathBuf::current_dir().map_err(LoadError::CurrentDir)?; - Self::find_root(start.as_abs_path()).ok_or(LoadError::NotFound)? + match Self::find_root(start.as_abs_path()) { + Some(root) => root, + None => return Ok(None), + } }; - Self::load(&root) + Self::load(&root).map(Some) + } + + /// Resolve the workspace a command should operate on, requiring one. + /// + /// [`Self::find`]'s rule, with the absence policy every CLI verb wants: no + /// workspace is [`LoadError::NotFound`], whose message tells the user how to + /// get one. + /// + /// # Errors + /// + /// As [`Self::find`], plus [`LoadError::NotFound`] when the walk-up finds no + /// `.hm/` directory. + pub fn resolve(dir: Option<&Path>) -> Result { + Self::find(dir)?.ok_or(LoadError::NotFound) } /// Walk up from `start` looking for a directory containing `.hm/`, and /// return that directory (the project root), or `None` if the filesystem /// root is reached without finding one. /// - /// This is the discovery half of [`Self::resolve`], exposed for callers that - /// want to know *whether* they are inside a project without paying to load - /// one — e.g. deciding whether a project config layer exists. + /// Private: this is [`Self::find`]'s discovery half, and callers that want + /// to know whether they are in a project should ask for the workspace + /// itself. Handing out a bare root invites re-deriving the paths and config + /// layering that [`Self::load`] already owns. /// /// Takes an [`AbsPath`] because the walk only terminates meaningfully from /// an absolute start: a relative `start` would walk to the empty path rather /// than `/`, and would yield a root that means something different once the /// cwd changes. Absolute in, absolute out. - #[must_use] - pub fn find_root(start: AbsPath<'_>) -> Option { + fn find_root(start: AbsPath<'_>) -> Option { let mut current = start; loop { if current.join(".hm").is_dir() { diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index 5900a8b9..648d603c 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -17,22 +17,21 @@ use secrecy::{ExposeSecret, SecretString}; /// The layered config for the current directory. /// -/// Includes the project `.hm/config.toml` layer whenever the cwd is inside a -/// harmont project — [`ResolvedCtx::org`]'s own error text tells users to set -/// `[cloud] org` there, so it has to be read. Outside a project there is simply -/// no project layer, and the cloud verbs still work from anywhere. +/// Inside a harmont project this is the workspace's config, project layer +/// included — [`ResolvedCtx::org`]'s own error text tells users to set +/// `[cloud] org` in `.hm/config.toml`, so it has to be read. Outside a project +/// there is simply no project layer, and the cloud verbs still work from +/// anywhere. /// -/// **Read-only callers only.** `hm cloud org switch` deliberately loads the user -/// layer alone: it saves back to the user file, and merging the project layer in -/// first would persist project-scoped values into `~/.config/hm/config.toml`. +/// **Read-only callers only.** `hm cloud org switch` deliberately uses +/// [`hm_core::Sys::config`] instead: it saves back to the user file, and merging +/// the project layer in first would persist project-scoped values into +/// `~/.config/hm/config.toml`. fn config() -> Result { - let cwd = hm_util::path::AbsPathBuf::current_dir().ok(); - let project = cwd - .as_ref() - .and_then(|cwd| hm_core::Workspace::find_root(cwd.as_abs_path())) - .map(|root| hm_config::Config::project_config_path(&root)); - hm_config::Config::load_from_paths(hm_core::Sys::config_path().as_deref(), project.as_deref()) - .context("loading config") + match hm_core::Workspace::find(None).context("loading config")? { + Some(ws) => Ok(ws.config().clone()), + None => hm_core::Sys::config().context("loading config"), + } } /// Resolve the bearer token, or the shared "not logged in" error. diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index a6eb5423..954fbede 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -27,12 +27,13 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .iter() .find(|o| o.slug == slug) .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; - // Load-mutate-save on the *user* config, so this deliberately passes no - // project layer: merging `.hm/config.toml` in first would persist - // project-scoped values into ~/.config/hm/config.toml. - let user_path = hm_core::Sys::config_path().context("could not determine config directory")?; - let mut cfg = hm_config::Config::load_from_paths(Some(&user_path), None)?; + // Load-mutate-save on the *user* config, so this deliberately uses + // `Sys::config` rather than `settings::config`: merging the project + // `.hm/config.toml` layer in first would persist project-scoped values into + // ~/.config/hm/config.toml. + let mut cfg = hm_core::Sys::config()?; cfg.cloud.org = Some(found.slug.clone()); + let user_path = hm_core::Sys::config_path().context("could not determine config directory")?; cfg.save_to(&user_path).context("saving config")?; tracing::info!("active organization: {} ({})", found.name, found.slug); Ok(()) diff --git a/crates/hm/tests/config_layered.rs b/crates/hm/tests/config_layered.rs index 081ddc91..ae2bf79b 100644 --- a/crates/hm/tests/config_layered.rs +++ b/crates/hm/tests/config_layered.rs @@ -104,13 +104,26 @@ fn load_resolves_project_root() { ) .unwrap(); - let start = hm_util::path::AbsPath::new(project_dir.path()).unwrap(); - let found = hm_core::Workspace::find_root(start); - assert_eq!( - found, - hm_util::path::AbsPathBuf::new(project_dir.path().to_path_buf()) - ); - let config_path = harmont_cli::config::Config::project_config_path(project_dir.path()); assert_eq!(config_path, harmont_dir.join("config.toml")); + + // Discovery and layering together: the workspace found at this root must + // actually read the project layer, not just point at it. + let ws = hm_core::Workspace::find(Some(project_dir.path())) + .unwrap() + .expect("project_dir is a workspace"); + assert_eq!(ws.path().as_path(), project_dir.path()); + assert_eq!(ws.config().cloud.org.as_deref(), Some("proj-root")); +} + +/// An explicit `--dir` that is not a workspace is a user error, not an absence: +/// only the cwd walk-up reaching `/` yields `Ok(None)`. +#[test] +fn find_rejects_an_explicit_dir_that_is_not_a_workspace() { + let empty = tempdir().unwrap(); + let found = hm_core::Workspace::find(Some(empty.path())); + assert!(matches!( + found, + Err(hm_core::WorkspaceLoadError::InvalidWorkspace(_)) + )); }