From 66f0aec43b0db82a30c1dcc26945b45689af12cd Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 20:36:43 -0700 Subject: [PATCH 1/6] refactor: merge hm-util into hm-common Fold the hm-util crate (dirs resolution + os fs helpers) into hm-common and delete it. Its modules land as hm_common::dirs, hm_common::fs, and a pub(crate) hm_common::os for platform primitives. Replace the FileMode(u32)/DirMode(u32) newtypes on the atomic writer with a semantic Privacy enum (Private = owner-only, Public = world-readable). The parent dir is now always created owner-only, so the dir-mode argument drops out entirely. Raw mode bits no longer leak into call sites, and the long-standing Windows no-op (modes silently ignored) is now documented on Privacy rather than hidden in u32s. write_atomic_restricted is renamed to write_atomic. Call sites in hm-config/hm-exec/hm move from hm_util::* to hm_common::*. --- Cargo.lock | 19 +- Cargo.toml | 3 - crates/hm-common/Cargo.toml | 10 +- crates/{hm-util => hm-common}/src/dirs.rs | 2 +- crates/hm-common/src/fs.rs | 337 +++++++++++++++++++ crates/hm-common/src/lib.rs | 5 + crates/{hm-util => hm-common}/src/os/dirs.rs | 0 crates/{hm-util => hm-common}/src/os/mod.rs | 1 - crates/hm-config/Cargo.toml | 2 +- crates/hm-config/src/creds.rs | 12 +- crates/hm-config/src/lib.rs | 7 +- crates/hm-exec/Cargo.toml | 1 - crates/hm-exec/src/local/backend.rs | 2 +- crates/hm-util/Cargo.toml | 29 -- crates/hm-util/src/lib.rs | 2 - crates/hm-util/src/os/fs.rs | 287 ---------------- crates/hm/Cargo.toml | 1 - crates/hm/src/commands/cache/clean.rs | 4 +- crates/hm/src/context.rs | 2 +- crates/hm/src/lib.rs | 2 +- crates/hm/tests/config_layered.rs | 2 +- 21 files changed, 370 insertions(+), 360 deletions(-) rename crates/{hm-util => hm-common}/src/dirs.rs (98%) rename crates/{hm-util => hm-common}/src/os/dirs.rs (100%) rename crates/{hm-util => hm-common}/src/os/mod.rs (63%) delete mode 100644 crates/hm-util/Cargo.toml delete mode 100644 crates/hm-util/src/lib.rs delete mode 100644 crates/hm-util/src/os/fs.rs diff --git a/Cargo.lock b/Cargo.lock index 4f8972bb..70d816c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1132,7 +1132,6 @@ dependencies = [ "hm-plugin-cloud", "hm-plugin-protocol", "hm-render", - "hm-util", "hm-vm", "human-units", "indicatif", @@ -1259,6 +1258,7 @@ dependencies = [ "bstr", "chrono", "derive_more", + "dirs", "include_dir", "num-traits", "proptest", @@ -1272,6 +1272,7 @@ dependencies = [ "unicode-segmentation", "unicode-width", "which", + "windows", ] [[package]] @@ -1281,7 +1282,7 @@ dependencies = [ "anyhow", "derive_more", "figment", - "hm-util", + "hm-common", "rstest", "serde", "tempfile", @@ -1321,7 +1322,6 @@ dependencies = [ "hm-common", "hm-pipeline-ir", "hm-plugin-protocol", - "hm-util", "hm-vm", "human-units", "ignore", @@ -1407,19 +1407,6 @@ dependencies = [ "uuid", ] -[[package]] -name = "hm-util" -version = "0.0.0-dev" -dependencies = [ - "anyhow", - "dirs", - "rstest", - "tempfile", - "tokio", - "tracing", - "windows", -] - [[package]] name = "hm-vm" version = "0.0.0-dev" diff --git a/Cargo.toml b/Cargo.toml index 307070a5..c57a8a2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,6 @@ members = [ "crates/hm-config", "crates/hm-plugin-protocol", "crates/hm-pipeline-ir", - "crates/hm-util", "crates/hm-plugin-cloud", "crates/hm-dsl-engine", "crates/hm-render", @@ -19,7 +18,6 @@ default-members = [ "crates/hm-config", "crates/hm-plugin-protocol", "crates/hm-pipeline-ir", - "crates/hm-util", "crates/hm-plugin-cloud", "crates/hm-dsl-engine", "crates/hm-render", @@ -37,7 +35,6 @@ hm-exec = { path = "crates/hm-exec", version = "0.0.0-dev" hm-plugin-protocol = { path = "crates/hm-plugin-protocol", version = "0.0.0-dev" } 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-dsl-engine = { path = "crates/hm-dsl-engine", version = "0.0.0-dev" } hm-render = { path = "crates/hm-render", version = "0.0.0-dev" } diff --git a/crates/hm-common/Cargo.toml b/crates/hm-common/Cargo.toml index 618b1f57..44a636cd 100644 --- a/crates/hm-common/Cargo.toml +++ b/crates/hm-common/Cargo.toml @@ -19,6 +19,7 @@ anyhow = { workspace = true } async-trait = { workspace = true } chrono = { workspace = true } derive_more = { workspace = true } +dirs = "6" serde = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } @@ -26,12 +27,19 @@ bstr = "1" include_dir = "0.7" num-traits = { workspace = true } tempfile = "3" -tokio = { version = "1", features = ["process", "fs"] } +tokio = { version = "1", features = ["process", "fs", "rt", "rt-multi-thread", "io-util"] } tracing = "0.1" unicode-width = { workspace = true } unicode-segmentation = { workspace = true } which = "7" +[target.'cfg(windows)'.dependencies.windows] +version = "0.62" +features = [ + "Win32_Foundation", + "Win32_Storage_FileSystem", +] + [dev-dependencies] tempfile = "3" tokio = { version = "1", features = ["macros", "rt-multi-thread"] } diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-common/src/dirs.rs similarity index 98% rename from crates/hm-util/src/dirs.rs rename to crates/hm-common/src/dirs.rs index d9e9b172..32553826 100644 --- a/crates/hm-util/src/dirs.rs +++ b/crates/hm-common/src/dirs.rs @@ -4,7 +4,7 @@ //! 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. +//! outside `hm-common` should never need them. #![allow(clippy::must_use_candidate)] diff --git a/crates/hm-common/src/fs.rs b/crates/hm-common/src/fs.rs index 319bac16..bcaae4d7 100644 --- a/crates/hm-common/src/fs.rs +++ b/crates/hm-common/src/fs.rs @@ -1,14 +1,69 @@ //! Filesystem helpers. +//! +//! Two families live here: +//! +//! * [`write_create_all`] — a convenience wrapper that scaffolds parent +//! directories before a plain write, with no permission guarantees. +//! * [`write_atomic`] (and its [`blocking`] counterpart) — an atomic writer +//! that also controls who may read the result via [`Privacy`]. Readers +//! observe either the full old contents or the full new contents, never a +//! truncated file. +//! +//! ## Privacy on Windows +//! +//! [`Privacy`] maps to Unix permission bits. On Windows it is currently a +//! **no-op** — Windows ACLs are not yet enforced, so a `Private` file is no +//! more restricted than a `Public` one. The intent is still recorded in the +//! type, so a real ACL implementation can drop in behind `#[cfg(windows)]` +//! without touching any call site. use std::io; use std::path::Path; +/// Who may read a file (or directory) that hm writes. +/// +/// Carries intent rather than raw mode bits: the file- and directory-mode +/// mapping is applied internally, so a secrets file can never accidentally +/// land with a directory's permissions. See the [module docs](self) for the +/// Windows caveat. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Privacy { + /// Owner-only. Unix file `0o600`, directory `0o700`. Use for secrets + /// (e.g. credentials). + Private, + /// World-readable, owner-writable. Unix file `0o644`, directory `0o755`. + Public, +} + +impl Privacy { + /// Unix permission bits for a *file* at this privacy level. + #[cfg(unix)] + const fn file_mode(self) -> u32 { + match self { + Self::Private => 0o600, + Self::Public => 0o644, + } + } + + /// Unix permission bits for a *directory* at this privacy level. + #[cfg(unix)] + const fn dir_mode(self) -> u32 { + match self { + Self::Private => 0o700, + Self::Public => 0o755, + } + } +} + /// Write `contents` to `path`, creating any missing parent directories first. /// /// Joins [`std::fs::create_dir_all`] on the parent and [`std::fs::write`] into /// one call, so callers scaffolding a file into a not-yet-existing directory /// don't repeat the parent-creation dance. An existing file is overwritten. /// +/// This makes no permission guarantees — reach for [`write_atomic`] when the +/// result must be owner-only or written atomically. +/// /// # Errors /// Returns the underlying [`io::Error`] if a parent directory cannot be created /// or the file cannot be written. @@ -20,6 +75,236 @@ pub fn write_create_all(path: impl AsRef, contents: impl AsRef<[u8]>) -> i std::fs::write(path, contents) } +/// Atomically write `contents` to `path` at the given [`Privacy`], ensuring the +/// parent directory exists. +/// +/// The parent directory is always created owner-only (`0o700` on Unix); only +/// the file's readability is controlled by `file`. The write goes to a +/// tempfile in the same directory and is `rename`d over `path`, so a reader +/// never sees a partial file. +/// +/// # Errors +/// +/// Returns an error if `path` has no parent or no file-name component, the +/// parent directory cannot be created or chmod'd, the tempfile cannot be +/// opened or written, or the final `rename` over `path` fails. +pub async fn write_atomic( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file: Privacy, +) -> io::Result<()> { + let path = path.as_ref().to_owned(); + let contents = contents.as_ref().to_vec(); + + let parent = path + .parent() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("{} has no parent directory", path.display()), + ) + })? + .to_owned(); + + create_dir_private(&parent).await?; + + let file_name = path + .file_name() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("{} has no file name", path.display()), + ) + })? + .to_os_string(); + let mut tmp_name = file_name; + tmp_name.push(format!(".tmp.{}", std::process::id())); + let tmp_path = parent.join(&tmp_name); + + write_file_private(&tmp_path, &contents, file).await?; + + let rename_result = atomic_rename_over(&tmp_path, &path).await; + if rename_result.is_err() { + let _ = tokio::fs::remove_file(&tmp_path).await; + } + rename_result +} + +/// Atomically replace `to` with `from`. +/// +/// On Unix this delegates to [`tokio::fs::rename`] (`rename(2)` — atomic +/// by POSIX guarantee). On Windows this uses `ReplaceFileW` (preserves +/// ACLs and alternate data streams) when the target exists, falling back +/// to `MoveFileExW` with `MOVEFILE_REPLACE_EXISTING` for first-write. +/// +/// # Errors +/// +/// Returns an error if the rename fails (permission denied, cross-device, +/// source missing, etc.). +pub async fn atomic_rename_over(from: impl AsRef, to: impl AsRef) -> io::Result<()> { + #[cfg(unix)] + { + tokio::fs::rename(from.as_ref(), to.as_ref()).await + } + #[cfg(windows)] + { + fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + use windows::Win32::Storage::FileSystem::{ + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, MoveFileExW, + REPLACEFILE_IGNORE_MERGE_ERRORS, ReplaceFileW, + }; + use windows::core::HSTRING; + + let from_w = HSTRING::from(from.as_os_str()); + let to_w = HSTRING::from(to.as_os_str()); + + if to.exists() { + let result = unsafe { + ReplaceFileW( + &to_w, + &from_w, + windows::core::PCWSTR::null(), + REPLACEFILE_IGNORE_MERGE_ERRORS, + None, + None, + ) + }; + return result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)); + } + + let result = unsafe { + MoveFileExW( + &from_w, + &to_w, + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, + ) + }; + result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)) + } + + let from = from.as_ref().to_owned(); + let to = to.as_ref().to_owned(); + tokio::task::spawn_blocking(move || atomic_rename_over_impl(&from, &to)) + .await + .map_err(io::Error::other)? + } +} + +/// Remove a file if it exists; silently return `Ok(())` if it does not. +/// +/// # Errors +/// +/// Returns an error if `remove_file` fails for any reason other than +/// `NotFound`. +pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { + match tokio::fs::remove_file(path).await { + Ok(()) => Ok(()), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e), + } +} + +/// Create `dir` (and any missing parents) owner-only. +/// +/// On Unix the directory is created at, or chmod'd to, `0o700`. On Windows +/// this is a plain recursive create (see the [module docs](self) on Windows +/// privacy). +async fn create_dir_private(dir: &Path) -> io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = Privacy::Private.dir_mode(); + match tokio::fs::metadata(dir).await { + Ok(meta) => { + let current = meta.permissions().mode() & 0o777; + if current != mode { + tokio::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode)).await?; + } + } + Err(e) if e.kind() == io::ErrorKind::NotFound => { + let mut builder = tokio::fs::DirBuilder::new(); + builder.recursive(true).mode(mode); + builder.create(dir).await?; + } + Err(e) => return Err(e), + } + Ok(()) + } + + #[cfg(windows)] + { + tokio::fs::create_dir_all(dir).await + } +} + +/// Write `contents` to `path` at the file mode implied by `privacy`. +/// +/// On Unix the file is opened with the exact mode so the bytes never exist at +/// a laxer permission. On Windows the privacy is a no-op (see [module +/// docs](self)). +async fn write_file_private(path: &Path, contents: &[u8], privacy: Privacy) -> io::Result<()> { + #[cfg(unix)] + { + use tokio::io::AsyncWriteExt; + let mut opts = tokio::fs::OpenOptions::new(); + opts.write(true) + .create(true) + .truncate(true) + .mode(privacy.file_mode()); + let mut f = opts.open(path).await?; + f.write_all(contents).await?; + f.sync_all().await?; + } + + #[cfg(windows)] + { + let _ = privacy; + tokio::fs::write(path, contents).await?; + } + + Ok(()) +} + +/// Synchronous wrappers that shell out to the async API via +/// `tokio::task::block_in_place`. Safe to call from sync contexts +/// that run inside a tokio runtime. +pub mod blocking { + use super::Privacy; + use std::io; + use std::path::Path; + + fn block_on>>(f: F) -> io::Result<()> { + tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(f)) + } + + /// Blocking counterpart of [`super::write_atomic`]. + /// + /// See the [module-level documentation](super) for semantics. + /// + /// # Errors + /// + /// Returns an error if `path` has no parent or no file-name component, + /// the parent directory cannot be created or chmod'd, the tempfile cannot + /// be opened or written, or the final `rename` over `path` fails. + pub fn write_atomic( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file: Privacy, + ) -> io::Result<()> { + block_on(super::write_atomic(path, contents, file)) + } + + /// Blocking counterpart of [`super::remove_file_if_exists`]. + /// + /// # Errors + /// + /// Returns an error if `remove_file` fails for any reason other than + /// `NotFound`. + pub fn remove_if_exists(path: impl AsRef) -> io::Result<()> { + block_on(super::remove_file_if_exists(path)) + } +} + #[cfg(test)] #[allow(clippy::unwrap_used, reason = "test setup and assertions")] mod tests { @@ -62,3 +347,55 @@ mod tests { assert!(write_create_all(&target, b"data").is_err()); } } + +#[cfg(all(test, unix))] +#[allow(clippy::unwrap_used, reason = "test setup and assertions")] +mod unix_privacy_tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + /// A `Private` file must land at exactly 0o600, in a 0o700 dir. + #[tokio::test] + async fn private_file_is_0600_in_dir_0700() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("hm"); + let file = dir.join("credentials.toml"); + + write_atomic(&file, b"token = \"hunter2\"\n", Privacy::Private) + .await + .unwrap(); + + assert_eq!(mode_of(&file), 0o600, "file mode must be 0o600"); + assert_eq!(mode_of(&dir), 0o700, "dir mode must be 0o700"); + } + + /// A `Public` file lands at 0o644, still in a 0o700 dir. + #[tokio::test] + async fn public_file_is_0644_in_dir_0700() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("hm"); + let file = dir.join("config.toml"); + + write_atomic(&file, b"key = 1\n", Privacy::Public) + .await + .unwrap(); + + assert_eq!(mode_of(&file), 0o644, "file mode must be 0o644"); + assert_eq!(mode_of(&dir), 0o700, "dir mode must be 0o700"); + } + + /// Overwriting a secret must preserve 0o600 (guards the tempfile + + /// atomic-rename path against perm drift). + #[tokio::test] + async fn rewrite_preserves_private_mode() { + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("credentials.toml"); + write_atomic(&file, b"a", Privacy::Private).await.unwrap(); + write_atomic(&file, b"bb", Privacy::Private).await.unwrap(); + assert_eq!(mode_of(&file), 0o600, "file mode must stay 0o600"); + } +} diff --git a/crates/hm-common/src/lib.rs b/crates/hm-common/src/lib.rs index b0c5fb15..fc0d86ba 100644 --- a/crates/hm-common/src/lib.rs +++ b/crates/hm-common/src/lib.rs @@ -2,6 +2,7 @@ #[cfg(feature = "app-runtime")] pub mod app_runtime; +pub mod dirs; pub mod format; pub mod fs; pub mod git; @@ -9,3 +10,7 @@ pub mod process; pub mod python; pub mod string; pub mod time; + +/// Raw platform primitives. Not part of the public surface — callers use +/// [`dirs`], which wraps these with Harmont-specific paths. +pub(crate) mod os; diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-common/src/os/dirs.rs similarity index 100% rename from crates/hm-util/src/os/dirs.rs rename to crates/hm-common/src/os/dirs.rs diff --git a/crates/hm-util/src/os/mod.rs b/crates/hm-common/src/os/mod.rs similarity index 63% rename from crates/hm-util/src/os/mod.rs rename to crates/hm-common/src/os/mod.rs index 0bc7829b..1b1480fa 100644 --- a/crates/hm-util/src/os/mod.rs +++ b/crates/hm-common/src/os/mod.rs @@ -1,2 +1 @@ pub(crate) mod dirs; -pub mod fs; diff --git a/crates/hm-config/Cargo.toml b/crates/hm-config/Cargo.toml index 2dd182c1..5f635577 100644 --- a/crates/hm-config/Cargo.toml +++ b/crates/hm-config/Cargo.toml @@ -7,7 +7,7 @@ repository.workspace = true description = "Layered (project/user/env) configuration + credential storage for the hm CLI." [dependencies] -hm-util = { workspace = true } +hm-common = { workspace = true } derive_more = { workspace = true } figment = { workspace = true } serde = { workspace = true } diff --git a/crates/hm-config/src/creds.rs b/crates/hm-config/src/creds.rs index b8efcfa8..ff469cd5 100644 --- a/crates/hm-config/src/creds.rs +++ b/crates/hm-config/src/creds.rs @@ -1,8 +1,7 @@ //! File-backed credential store at `~/.config/hm/credentials.toml`. //! -//! The file is written with mode 0o600 (parent dir 0o700) via -//! [`hm_util::os::fs::blocking::write_atomic_restricted`], keyed by -//! `(service, account)`. +//! The file is written with [`Privacy::Private`] (0o600, parent dir 0o700) +//! via [`hm_common::fs::blocking::write_atomic`], keyed by `(service, account)`. use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; @@ -16,7 +15,7 @@ struct CredentialFile { } fn path() -> Result { - let dir = hm_util::dirs::hm_config_dir().context("could not determine config directory")?; + let dir = hm_common::dirs::hm_config_dir().context("could not determine config directory")?; Ok(dir.join("credentials.toml")) } @@ -33,11 +32,10 @@ fn load() -> CredentialFile { 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( + hm_common::fs::blocking::write_atomic( &p, serialized.as_bytes(), - hm_util::os::fs::FileMode(0o600), - hm_util::os::fs::DirMode(0o700), + hm_common::fs::Privacy::Private, ) .with_context(|| format!("writing {}", p.display()))?; Ok(()) diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index a916f92e..00481ed1 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -119,7 +119,7 @@ impl Config { /// /// 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")?; + let dir = hm_common::dirs::hm_config_dir().context("could not determine config directory")?; Ok(dir.join("config.toml")) } @@ -179,11 +179,10 @@ impl Config { /// 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")?; - hm_util::os::fs::blocking::write_atomic_restricted( + hm_common::fs::blocking::write_atomic( path, serialized.as_bytes(), - hm_util::os::fs::FileMode(0o644), - hm_util::os::fs::DirMode(0o700), + hm_common::fs::Privacy::Public, ) .with_context(|| format!("writing {}", path.display())) } diff --git a/crates/hm-exec/Cargo.toml b/crates/hm-exec/Cargo.toml index 6afc9d28..90056d24 100644 --- a/crates/hm-exec/Cargo.toml +++ b/crates/hm-exec/Cargo.toml @@ -10,7 +10,6 @@ description = "Pluggable CI execution backends (local VM + cloud) for the hm CLI hm-common = { workspace = true } hm-plugin-protocol = { workspace = true } hm-pipeline-ir = { workspace = true } -hm-util = { workspace = true } hm-vm = { workspace = true, features = ["docker-backend"] } harmont-cloud = { workspace = true } async-trait = { workspace = true } diff --git a/crates/hm-exec/src/local/backend.rs b/crates/hm-exec/src/local/backend.rs index 0713100c..8da851b8 100644 --- a/crates/hm-exec/src/local/backend.rs +++ b/crates/hm-exec/src/local/backend.rs @@ -50,7 +50,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(|| { + let cache_dir = hm_common::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) diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml deleted file mode 100644 index e5738a6a..00000000 --- a/crates/hm-util/Cargo.toml +++ /dev/null @@ -1,29 +0,0 @@ -[package] -name = "hm-util" -version = "0.0.0-dev" -edition.workspace = true -license.workspace = true -repository.workspace = true -description = "Shared OS and filesystem utilities for Harmont crates." - -[dependencies] -anyhow = { workspace = true } -dirs = "6" -tempfile = "3" -tokio = { version = "1", features = ["rt", "rt-multi-thread", "fs", "io-util"] } -tracing = { workspace = true } - -[target.'cfg(windows)'.dependencies.windows] -version = "0.62" -features = [ - "Win32_Foundation", - "Win32_Storage_FileSystem", -] - -[dev-dependencies] -rstest = { workspace = true } -tempfile = "3" -tokio = { version = "1", features = ["full", "test-util"] } - -[lints] -workspace = true diff --git a/crates/hm-util/src/lib.rs b/crates/hm-util/src/lib.rs deleted file mode 100644 index c5284c52..00000000 --- a/crates/hm-util/src/lib.rs +++ /dev/null @@ -1,2 +0,0 @@ -pub mod dirs; -pub mod os; diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs deleted file mode 100644 index 19441ca4..00000000 --- a/crates/hm-util/src/os/fs.rs +++ /dev/null @@ -1,287 +0,0 @@ -//! Filesystem helpers. -//! -//! The main entry point is [`write_atomic_restricted`]. A synchronous -//! wrapper is available at [`blocking::write_atomic_restricted`] for -//! callers that run inside a tokio runtime but cannot use async. -//! -//! Both guarantee that readers observe either the full old contents or -//! the full new contents — never a truncated file — and that Unix -//! file/directory modes are set atomically with creation. - -use std::io; -use std::path::Path; - -/// Unix mode bits for a file (e.g. `0o600`). -/// -/// A distinct newtype from [`DirMode`] so the file- and directory-mode -/// arguments of [`write_atomic_restricted`] cannot be transposed: passing -/// them in the wrong order is a compile error rather than a silent -/// security regression (a secrets file landing at `0o700`, say). -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct FileMode(pub u32); - -/// Unix mode bits for a directory (e.g. `0o700`). -/// -/// See [`FileMode`] for why this is a distinct newtype. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct DirMode(pub u32); - -/// Write `contents` to `path` atomically with `file`, ensuring the -/// parent directory exists and is set to `dir`. -/// -/// # Errors -/// -/// Returns an error if `path` has no parent or no file-name component, -/// the parent directory cannot be created or chmod'd to `dir`, the -/// tempfile cannot be opened with `file` or written, or the final -/// `rename` over `path` fails. -pub async fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file: FileMode, - dir: DirMode, -) -> io::Result<()> { - let path = path.as_ref().to_owned(); - let contents = contents.as_ref().to_vec(); - - let parent = path - .parent() - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - format!("{} has no parent directory", path.display()), - ) - })? - .to_owned(); - - create_dir_with_mode(&parent, dir.0).await?; - - let file_name = path - .file_name() - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - format!("{} has no file name", path.display()), - ) - })? - .to_os_string(); - let mut tmp_name = file_name; - tmp_name.push(format!(".tmp.{}", std::process::id())); - let tmp_path = parent.join(&tmp_name); - - write_file_with_mode(&tmp_path, &contents, file.0).await?; - - let rename_result = atomic_rename_over(&tmp_path, &path).await; - if rename_result.is_err() { - let _ = tokio::fs::remove_file(&tmp_path).await; - } - rename_result -} - -/// Atomically replace `to` with `from`. -/// -/// On Unix this delegates to [`tokio::fs::rename`] (`rename(2)` — atomic -/// by POSIX guarantee). On Windows this uses `ReplaceFileW` (preserves -/// ACLs and alternate data streams) when the target exists, falling back -/// to `MoveFileExW` with `MOVEFILE_REPLACE_EXISTING` for first-write. -/// -/// # Errors -/// -/// Returns an error if the rename fails (permission denied, cross-device, -/// source missing, etc.). -pub async fn atomic_rename_over(from: impl AsRef, to: impl AsRef) -> io::Result<()> { - #[cfg(unix)] - { - tokio::fs::rename(from.as_ref(), to.as_ref()).await - } - #[cfg(windows)] - { - fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { - use windows::Win32::Storage::FileSystem::{ - MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, MoveFileExW, - REPLACEFILE_IGNORE_MERGE_ERRORS, ReplaceFileW, - }; - use windows::core::HSTRING; - - let from_w = HSTRING::from(from.as_os_str()); - let to_w = HSTRING::from(to.as_os_str()); - - if to.exists() { - let result = unsafe { - ReplaceFileW( - &to_w, - &from_w, - windows::core::PCWSTR::null(), - REPLACEFILE_IGNORE_MERGE_ERRORS, - None, - None, - ) - }; - return result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)); - } - - let result = unsafe { - MoveFileExW( - &from_w, - &to_w, - MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, - ) - }; - result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)) - } - - let from = from.as_ref().to_owned(); - let to = to.as_ref().to_owned(); - tokio::task::spawn_blocking(move || atomic_rename_over_impl(&from, &to)) - .await - .map_err(io::Error::other)? - } -} - -/// Remove a file if it exists; silently return `Ok(())` if it does not. -/// -/// # Errors -/// -/// Returns an error if `remove_file` fails for any reason other than -/// `NotFound`. -pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { - match tokio::fs::remove_file(path).await { - Ok(()) => Ok(()), - Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e), - } -} - -#[cfg(unix)] -async fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - match tokio::fs::metadata(dir).await { - Ok(meta) => { - let current = meta.permissions().mode() & 0o777; - if current != mode { - tokio::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode)).await?; - } - } - Err(e) if e.kind() == io::ErrorKind::NotFound => { - let mut builder = tokio::fs::DirBuilder::new(); - builder.recursive(true).mode(mode); - builder.create(dir).await?; - } - Err(e) => return Err(e), - } - } - - #[cfg(windows)] - { - tokio::fs::create_dir_all(dir).await - } - Ok(()) -} - -async fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { - #[cfg(unix)] - { - use tokio::io::AsyncWriteExt; - let mut opts = tokio::fs::OpenOptions::new(); - opts.write(true).create(true).truncate(true).mode(mode); - let mut f = opts.open(path).await?; - f.write_all(contents).await?; - f.sync_all().await?; - } - - #[cfg(windows)] - { - tokio::fs::write(path, contents).await - } - - Ok(()) -} - -/// Synchronous wrappers that shell out to the async API via -/// `tokio::task::block_in_place`. Safe to call from sync contexts -/// that run inside a tokio runtime. -pub mod blocking { - use super::{DirMode, FileMode}; - use std::io; - use std::path::Path; - - fn block_on>>(f: F) -> io::Result<()> { - tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(f)) - } - - /// Blocking counterpart of [`super::write_atomic_restricted`]. - /// - /// See the [module-level documentation](super) for semantics. - /// - /// # Errors - /// - /// Returns an error if `path` has no parent or no file-name component, - /// the parent directory cannot be created or chmod'd to `dir`, the - /// tempfile cannot be opened with `file` or written, or the final - /// `rename` over `path` fails. - pub fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file: FileMode, - dir: DirMode, - ) -> io::Result<()> { - block_on(super::write_atomic_restricted(path, contents, file, dir)) - } - - /// Blocking counterpart of [`super::remove_file_if_exists`]. - /// - /// # Errors - /// - /// Returns an error if `remove_file` fails for any reason other than - /// `NotFound`. - pub fn remove_if_exists(path: impl AsRef) -> io::Result<()> { - block_on(super::remove_file_if_exists(path)) - } -} - -#[cfg(all(test, unix))] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - use std::os::unix::fs::PermissionsExt; - - /// Credentials (and any secret) must land at exactly 0o600, in a 0o700 dir. - #[tokio::test] - async fn writes_file_0600_in_dir_0700() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("hm"); - let file = dir.join("credentials.toml"); - - write_atomic_restricted( - &file, - b"token = \"hunter2\"\n", - FileMode(0o600), - DirMode(0o700), - ) - .await - .unwrap(); - - let fmode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; - assert_eq!(fmode, 0o600, "file mode must be 0o600, got {fmode:o}"); - let dmode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dmode, 0o700, "dir mode must be 0o700, got {dmode:o}"); - } - - /// Overwriting an existing secret must preserve 0o600 (guards the - /// temp-file + atomic-rename path against perm drift). - #[tokio::test] - async fn rewrite_preserves_0600() { - let tmp = tempfile::tempdir().unwrap(); - let file = tmp.path().join("credentials.toml"); - write_atomic_restricted(&file, b"a", FileMode(0o600), DirMode(0o700)) - .await - .unwrap(); - write_atomic_restricted(&file, b"bb", FileMode(0o600), DirMode(0o700)) - .await - .unwrap(); - let fmode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; - assert_eq!(fmode, 0o600, "file mode must stay 0o600, got {fmode:o}"); - } -} diff --git a/crates/hm/Cargo.toml b/crates/hm/Cargo.toml index a54b868d..ec26dd95 100644 --- a/crates/hm/Cargo.toml +++ b/crates/hm/Cargo.toml @@ -60,7 +60,6 @@ futures-util = "0.3" hm-pipeline-ir = { workspace = true } hm-plugin-protocol = { workspace = true } hm-common = { workspace = true, features = ["app-runtime"] } -hm-util = { workspace = true } hm-config = { workspace = true } hm-plugin-cloud = { workspace = true } harmont-cloud = { workspace = true } diff --git a/crates/hm/src/commands/cache/clean.rs b/crates/hm/src/commands/cache/clean.rs index 0b7fe184..a014abbd 100644 --- a/crates/hm/src/commands/cache/clean.rs +++ b/crates/hm/src/commands/cache/clean.rs @@ -5,7 +5,7 @@ use human_units::FormatSize 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_common::dirs::hm_workspace_cache_dir() && ws_cache.exists() { let size = dir_size(&ws_cache); @@ -20,7 +20,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_common::dirs::hm_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/context.rs b/crates/hm/src/context.rs index eaf4c1e7..99a2b5f6 100644 --- a/crates/hm/src/context.rs +++ b/crates/hm/src/context.rs @@ -28,7 +28,7 @@ impl RunContext { /// 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 project_root = hm_common::dirs::find_project_root(&start_dir); let config = Config::load(project_root.as_deref())?; let output = OutputMode::Human { diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 846b85da..95472547 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -3,7 +3,7 @@ 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 +// crate. All directory resolution goes through `hm_common::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. diff --git a/crates/hm/tests/config_layered.rs b/crates/hm/tests/config_layered.rs index 074e27ae..c0f69bec 100644 --- a/crates/hm/tests/config_layered.rs +++ b/crates/hm/tests/config_layered.rs @@ -85,7 +85,7 @@ fn load_resolves_project_root() { ) .unwrap(); - let found = hm_util::dirs::find_project_root(project_dir.path()); + let found = hm_common::dirs::find_project_root(project_dir.path()); assert_eq!(found, Some(project_dir.path().to_path_buf())); let config_path = harmont_cli::config::Config::project_config_path(project_dir.path()); From 501198b23f89926f4ad23200a0bcc96339afc664 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 22:48:46 -0700 Subject: [PATCH 2/6] refactor(hm-common): make fs writers async, drop blocking module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fs::blocking wrappers existed for extism host_fn sync callbacks, which the plugin-era cleanup already removed. Delete the module and make every public fs writer async: write_atomic, atomic_rename_over, remove_file_if_exists, and write_create_all (now tokio::fs, was std::fs). Propagate async through the only sync callers — hm-config's credential and config writers (save/set/delete/save_to/save_user/set_cloud_token/ forget_cloud_token) and hm's init/run helpers (write_template, write_skills, persist_project_pipeline) — plus the cloud auth verbs that drive them. Tests touching these move to #[tokio::test]. --- crates/hm-common/src/fs.rs | 76 +++++++---------------- crates/hm-config/src/creds.rs | 51 +++++++-------- crates/hm-config/src/lib.rs | 17 +++-- crates/hm-plugin-cloud/src/auth/login.rs | 2 +- crates/hm-plugin-cloud/src/auth/logout.rs | 6 +- crates/hm-plugin-cloud/src/verbs/org.rs | 2 +- crates/hm/src/commands/init.rs | 20 +++--- crates/hm/src/commands/run/mod.rs | 12 ++-- 8 files changed, 74 insertions(+), 112 deletions(-) diff --git a/crates/hm-common/src/fs.rs b/crates/hm-common/src/fs.rs index bcaae4d7..19bbbe93 100644 --- a/crates/hm-common/src/fs.rs +++ b/crates/hm-common/src/fs.rs @@ -1,13 +1,13 @@ //! Filesystem helpers. //! -//! Two families live here: +//! Every writer here is `async` — there is no blocking variant. Two families +//! live here: //! //! * [`write_create_all`] — a convenience wrapper that scaffolds parent //! directories before a plain write, with no permission guarantees. -//! * [`write_atomic`] (and its [`blocking`] counterpart) — an atomic writer -//! that also controls who may read the result via [`Privacy`]. Readers -//! observe either the full old contents or the full new contents, never a -//! truncated file. +//! * [`write_atomic`] — an atomic writer that also controls who may read the +//! result via [`Privacy`]. Readers observe either the full old contents or +//! the full new contents, never a truncated file. //! //! ## Privacy on Windows //! @@ -67,12 +67,15 @@ impl Privacy { /// # Errors /// Returns the underlying [`io::Error`] if a parent directory cannot be created /// or the file cannot be written. -pub fn write_create_all(path: impl AsRef, contents: impl AsRef<[u8]>) -> io::Result<()> { +pub async fn write_create_all( + path: impl AsRef, + contents: impl AsRef<[u8]>, +) -> io::Result<()> { let path = path.as_ref(); if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { - std::fs::create_dir_all(parent)?; + tokio::fs::create_dir_all(parent).await?; } - std::fs::write(path, contents) + tokio::fs::write(path, contents).await } /// Atomically write `contents` to `path` at the given [`Privacy`], ensuring the @@ -265,46 +268,6 @@ async fn write_file_private(path: &Path, contents: &[u8], privacy: Privacy) -> i Ok(()) } -/// Synchronous wrappers that shell out to the async API via -/// `tokio::task::block_in_place`. Safe to call from sync contexts -/// that run inside a tokio runtime. -pub mod blocking { - use super::Privacy; - use std::io; - use std::path::Path; - - fn block_on>>(f: F) -> io::Result<()> { - tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(f)) - } - - /// Blocking counterpart of [`super::write_atomic`]. - /// - /// See the [module-level documentation](super) for semantics. - /// - /// # Errors - /// - /// Returns an error if `path` has no parent or no file-name component, - /// the parent directory cannot be created or chmod'd, the tempfile cannot - /// be opened or written, or the final `rename` over `path` fails. - pub fn write_atomic( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file: Privacy, - ) -> io::Result<()> { - block_on(super::write_atomic(path, contents, file)) - } - - /// Blocking counterpart of [`super::remove_file_if_exists`]. - /// - /// # Errors - /// - /// Returns an error if `remove_file` fails for any reason other than - /// `NotFound`. - pub fn remove_if_exists(path: impl AsRef) -> io::Result<()> { - block_on(super::remove_file_if_exists(path)) - } -} - #[cfg(test)] #[allow(clippy::unwrap_used, reason = "test setup and assertions")] mod tests { @@ -315,28 +278,31 @@ mod tests { #[case::flat("file.txt")] #[case::one_level("sub/file.txt")] #[case::deeply_nested("a/b/c/file.txt")] - fn writes_file_creating_missing_parents(#[case] rel: &str) { + #[tokio::test] + async fn writes_file_creating_missing_parents(#[case] rel: &str) { let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join(rel); - write_create_all(&path, b"hello").unwrap(); + write_create_all(&path, b"hello").await.unwrap(); assert_eq!(std::fs::read_to_string(&path).unwrap(), "hello"); } #[rstest] - fn overwrites_an_existing_file() { + #[tokio::test] + async fn overwrites_an_existing_file() { let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join("file.txt"); - write_create_all(&path, b"first").unwrap(); - write_create_all(&path, b"second").unwrap(); + write_create_all(&path, b"first").await.unwrap(); + write_create_all(&path, b"second").await.unwrap(); assert_eq!(std::fs::read_to_string(&path).unwrap(), "second"); } #[rstest] - fn propagates_io_error_when_parent_is_a_file() { + #[tokio::test] + async fn propagates_io_error_when_parent_is_a_file() { let tmp = tempfile::tempdir().unwrap(); // A *file* sits where the target's parent directory would go, so // creating the parent must fail with an OS error. @@ -344,7 +310,7 @@ mod tests { std::fs::write(&blocker, b"x").unwrap(); let target = blocker.join("child.txt"); - assert!(write_create_all(&target, b"data").is_err()); + assert!(write_create_all(&target, b"data").await.is_err()); } } diff --git a/crates/hm-config/src/creds.rs b/crates/hm-config/src/creds.rs index ff469cd5..77be1050 100644 --- a/crates/hm-config/src/creds.rs +++ b/crates/hm-config/src/creds.rs @@ -29,15 +29,12 @@ fn load() -> CredentialFile { toml::from_str(&contents).unwrap_or_default() } -fn save(file: &CredentialFile) -> Result<()> { +async fn save(file: &CredentialFile) -> Result<()> { let p = path()?; let serialized = toml::to_string_pretty(file).context("serializing credentials")?; - hm_common::fs::blocking::write_atomic( - &p, - serialized.as_bytes(), - hm_common::fs::Privacy::Private, - ) - .with_context(|| format!("writing {}", p.display()))?; + hm_common::fs::write_atomic(&p, serialized.as_bytes(), hm_common::fs::Privacy::Private) + .await + .with_context(|| format!("writing {}", p.display()))?; Ok(()) } @@ -49,13 +46,13 @@ pub fn get(service: &str, account: &str) -> Option { } /// Write a credential. Silently no-ops on I/O failure (best-effort). -pub fn set(service: &str, account: &str, secret: &str) { +pub async 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); + let _ = save(&f).await; } /// Credential `service` name for the cloud bearer token (account = API base URL). @@ -80,20 +77,20 @@ pub fn cloud_token(api_base: &str) -> Option { /// /// 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); +pub async fn set_cloud_token(api_base: &str, token: &str) { + set(CLOUD_SERVICE, api_base, token).await; } /// 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); +pub async fn forget_cloud_token(api_base: &str) { + delete(CLOUD_SERVICE, api_base).await; } /// Remove a credential. Silently no-ops if the entry is absent or the /// underlying write fails. -pub fn delete(service: &str, account: &str) { +pub async 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); @@ -102,22 +99,31 @@ pub fn delete(service: &str, account: &str) { if now_empty { f.entries.remove(service); } - let _ = save(&f); + let _ = save(&f).await; } #[cfg(test)] #[allow(clippy::unwrap_used, unsafe_code)] mod tests { use super::*; + use rstest::rstest; - fn with_home(f: F) { + #[rstest] + #[tokio::test] + async fn round_trip() { 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(); + + assert_eq!(get("svc", "acct"), None); + set("svc", "acct", "shh").await; + assert_eq!(get("svc", "acct").as_deref(), Some("shh")); + delete("svc", "acct").await; + assert_eq!(get("svc", "acct"), None); + unsafe { if let Some(v) = prev { std::env::set_var("HOME", v); @@ -126,15 +132,4 @@ mod tests { } } } - - #[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 00481ed1..99b780f5 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -177,14 +177,11 @@ impl Config { /// # Errors /// /// Returns an error if TOML serialization fails or the atomic write fails. - pub fn save_to(&self, path: &Path) -> Result<()> { + pub async fn save_to(&self, path: &Path) -> Result<()> { let serialized = toml::to_string_pretty(self).context("serializing config")?; - hm_common::fs::blocking::write_atomic( - path, - serialized.as_bytes(), - hm_common::fs::Privacy::Public, - ) - .with_context(|| format!("writing {}", path.display())) + hm_common::fs::write_atomic(path, serialized.as_bytes(), hm_common::fs::Privacy::Public) + .await + .with_context(|| format!("writing {}", path.display())) } /// Save to user-level config path (`~/.config/hm/config.toml`). @@ -192,8 +189,8 @@ impl Config { /// # 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()?) + pub async fn save_user(&self) -> Result<()> { + self.save_to(&Self::user_config_path()?).await } } @@ -389,7 +386,7 @@ org = "project-org" }, ..Config::default() }; - cfg.save_to(&path).unwrap(); + cfg.save_to(&path).await.unwrap(); let loaded = Config::load_from_paths(Some(&path), None).unwrap(); assert_eq!(loaded.cloud.org.as_deref(), Some("saved-org")); diff --git a/crates/hm-plugin-cloud/src/auth/login.rs b/crates/hm-plugin-cloud/src/auth/login.rs index 97b18423..d5bc7b85 100644 --- a/crates/hm-plugin-cloud/src/auth/login.rs +++ b/crates/hm-plugin-cloud/src/auth/login.rs @@ -28,7 +28,7 @@ pub(crate) async fn run(env: &BTreeMap, paste: bool) -> Result<( login_loopback(&client, &app).await? }; - hm_config::creds::set_cloud_token(&api, &token); + hm_config::creds::set_cloud_token(&api, &token).await; // 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 57478847..9461dc6b 100644 --- a/crates/hm-plugin-cloud/src/auth/logout.rs +++ b/crates/hm-plugin-cloud/src/auth/logout.rs @@ -6,13 +6,9 @@ use anyhow::Result; use crate::settings; -#[allow( - clippy::unused_async, - reason = "async to match the uniform await arm in cli::dispatch_command" -)] pub(crate) async fn run(_env: &BTreeMap) -> Result<()> { let (_client, api) = settings::anon_client()?; - hm_config::creds::forget_cloud_token(&api); + hm_config::creds::forget_cloud_token(&api).await; tracing::info!("logged out of {api}"); Ok(()) } diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index 6d9f76fe..00aefbbf 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -29,7 +29,7 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; let mut cfg = hm_config::Config::load(None)?; cfg.cloud.org = Some(found.slug.clone()); - cfg.save_user().context("saving config")?; + cfg.save_user().await.context("saving config")?; tracing::info!("active organization: {} ({})", found.name, found.slug); Ok(()) } diff --git a/crates/hm/src/commands/init.rs b/crates/hm/src/commands/init.rs index 846e7a38..40e32b87 100644 --- a/crates/hm/src/commands/init.rs +++ b/crates/hm/src/commands/init.rs @@ -168,7 +168,7 @@ fn write_cloud_project_config(dir: &std::path::Path, org_slug: &str) -> Result<( Ok(()) } -fn write_template(dir: &Path, tmpl: &Template, force: bool) -> Result { +async fn write_template(dir: &Path, tmpl: &Template, force: bool) -> Result { let harmont_dir = dir.join(".hm"); let already_has_pipeline = detect::has_pipeline_files(dir); @@ -187,13 +187,14 @@ fn write_template(dir: &Path, tmpl: &Template, force: bool) -> Result { // pipeline.py and deploy.py). `std::fs::write` clobbers just the target. let dest = harmont_dir.join(tmpl.filename); hm_common::fs::write_create_all(&dest, tmpl.content) + .await .with_context(|| format!("writing {}", dest.display()))?; ensure_gitignore_entry(&harmont_dir, "node_modules/")?; ensure_gitignore_entry(&harmont_dir, "__pycache__/")?; Ok(true) } -fn write_skills(dir: &Path, force: bool) -> Result<()> { +async fn write_skills(dir: &Path, force: bool) -> Result<()> { let skills: &[(&str, &str)] = &[ ("validate-ci", SKILL_VALIDATE_CI), ("write-pipeline", SKILL_WRITE_PIPELINE), @@ -220,6 +221,7 @@ fn write_skills(dir: &Path, force: bool) -> Result<()> { let updated = dest.exists(); hm_common::fs::write_create_all(&dest, content) + .await .with_context(|| format!("writing {}", dest.display()))?; if updated { tracing::info!("overwrote Claude Code skill: .claude/skills/{slug}/SKILL.md"); @@ -287,7 +289,7 @@ pub async fn handle(args: InitArgs) -> Result<()> { pick_interactive()? }; let tmpl = kind.meta(); - let wrote_pipeline = write_template(&args.dir, &tmpl, args.force)?; + let wrote_pipeline = write_template(&args.dir, &tmpl, args.force).await?; if wrote_pipeline { let dsl = match kind { TemplateKind::Nextjs | TemplateKind::Js | TemplateKind::Zig => "TypeScript", @@ -314,7 +316,7 @@ pub async fn handle(args: InitArgs) -> Result<()> { // Skills are offered whenever a terminal is present, independent of // whether a template flag was passed. if tty && prompt_skills()? { - write_skills(&args.dir, args.force)?; + write_skills(&args.dir, args.force).await?; } let project_config = hm_config::Config::project_config_path(&args.dir); @@ -352,7 +354,8 @@ mod tests { #[case::customized_no_force(Some("# my local edits"), false, "# my local edits")] #[case::customized_force(Some("# my local edits"), true, SKILL_VALIDATE_CI)] #[case::unchanged_idempotent(Some(SKILL_VALIDATE_CI), false, SKILL_VALIDATE_CI)] - fn write_skills_behaves( + #[tokio::test] + async fn write_skills_behaves( #[case] preexisting: Option<&str>, #[case] force: bool, #[case] expected: &str, @@ -364,15 +367,16 @@ mod tests { std::fs::write(&dest, content).unwrap(); } - write_skills(dir.path(), force).unwrap(); + write_skills(dir.path(), force).await.unwrap(); assert_eq!(std::fs::read_to_string(&dest).unwrap(), expected); } #[rstest] - fn write_skills_installs_sibling_skills_when_absent() { + #[tokio::test] + async fn write_skills_installs_sibling_skills_when_absent() { let dir = tempfile::tempdir().unwrap(); - write_skills(dir.path(), false).unwrap(); + write_skills(dir.path(), false).await.unwrap(); // Skills other than the one under test are installed too. assert!(skill_path(dir.path(), "write-pipeline").exists()); diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index ce2ef283..23a20c47 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -382,7 +382,7 @@ fn build_create_pipeline_request( /// `.hm/config.toml`, preserving any other keys already in the file. Creates /// the file (and `.hm/`) when absent. Used after registering a remoteless /// directory so later runs submit by the persisted slug without prompting. -fn persist_project_pipeline(dir: &std::path::Path, org: &str, slug: &str) -> Result<()> { +async fn persist_project_pipeline(dir: &std::path::Path, org: &str, slug: &str) -> Result<()> { let path = dir.join(".hm/config.toml"); let mut doc: toml::Table = std::fs::read_to_string(&path) .ok() @@ -398,6 +398,7 @@ fn persist_project_pipeline(dir: &std::path::Path, org: &str, slug: &str) -> Res } let serialized = toml::to_string_pretty(&doc).context("serializing .hm/config.toml")?; hm_common::fs::write_create_all(&path, serialized) + .await .with_context(|| format!("writing {}", path.display()))?; Ok(()) } @@ -467,7 +468,9 @@ async fn register_remoteless_pipeline( } }; - persist_project_pipeline(dir, org, &slug).context("saving .hm/config.toml")?; + persist_project_pipeline(dir, org, &slug) + .await + .context("saving .hm/config.toml")?; tracing::info!("registered pipeline '{slug}' — submitting build"); Ok(slug) } @@ -798,7 +801,8 @@ mod tests { "web", Some("https://example.test") )] - fn persist_project_pipeline_writes_config( + #[tokio::test] + async fn persist_project_pipeline_writes_config( #[case] preexisting: Option<&str>, #[case] slug: &str, #[case] expected_api_url: Option<&str>, @@ -809,7 +813,7 @@ mod tests { std::fs::write(dir.path().join(".hm/config.toml"), content).unwrap(); } - persist_project_pipeline(dir.path(), "acme", slug).unwrap(); + persist_project_pipeline(dir.path(), "acme", slug).await.unwrap(); let raw = std::fs::read_to_string(dir.path().join(".hm/config.toml")).unwrap(); let doc: toml::Table = toml::from_str(&raw).unwrap(); From 3bb847044abe2d91217bc4c86a48317578ced749 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 23:04:56 -0700 Subject: [PATCH 3/6] feat(hm-common): add DirProvider for platform user directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce DirProvider (dir_provider.rs): it resolves the platform per-user directory roots (config, cache) a single time at construction and hands them out as &Path, so consumers stop re-resolving them per call. It is deliberately application-agnostic — it knows nothing of Harmont's own `hm/` subdirectory; callers join that (and any file name) onto these roots. Replaces the pub(crate) os/dirs.rs platform primitives, which are deleted, and the hm_config_dir/hm_cache_dir/ hm_workspace_cache_dir free functions in dirs.rs. dirs.rs now holds only find_project_root (project discovery, orthogonal to the platform roots). AppRuntime gains a resolved-once DirProvider and an AppRuntime::dirs() accessor, alongside an InitError::Dirs variant. Consumers migrate: hm/cache-clean reads AppRuntime::dirs(); hm-config and hm-exec (no app-runtime feature) construct DirProvider::new() directly. Each joins `hm/` onto the returned root as before, so resolved paths are unchanged. Also make write_atomic create the parent directory at the file's own Privacy (0o755 for Public, 0o700 for Private) rather than always owner-only. --- crates/hm-common/src/app_runtime.rs | 16 +++++ crates/hm-common/src/dir_provider.rs | 80 +++++++++++++++++++++++ crates/hm-common/src/dirs.rs | 50 ++------------- crates/hm-common/src/fs.rs | 91 ++++++--------------------- crates/hm-common/src/lib.rs | 5 +- crates/hm-common/src/os/dirs.rs | 31 --------- crates/hm-common/src/os/mod.rs | 1 - crates/hm-config/src/creds.rs | 4 +- crates/hm-config/src/lib.rs | 4 +- crates/hm-exec/src/local/backend.rs | 5 +- crates/hm/src/commands/cache/clean.rs | 38 ++++++----- 11 files changed, 145 insertions(+), 180 deletions(-) create mode 100644 crates/hm-common/src/dir_provider.rs delete mode 100644 crates/hm-common/src/os/dirs.rs delete mode 100644 crates/hm-common/src/os/mod.rs diff --git a/crates/hm-common/src/app_runtime.rs b/crates/hm-common/src/app_runtime.rs index 8844ff4f..7532fdf0 100644 --- a/crates/hm-common/src/app_runtime.rs +++ b/crates/hm-common/src/app_runtime.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use std::sync::OnceLock; +use crate::dir_provider::DirProvider; use crate::git::Git; use crate::process::{ExecutableNotFound, pathbin}; use crate::python::Python; @@ -16,6 +17,9 @@ pub enum InitError { /// The current directory could not be read. #[error("reading the current directory")] Cwd(#[source] std::io::Error), + /// The Harmont directory layout could not be resolved (no home directory?). + #[error("could not resolve the Harmont directory layout")] + Dirs, } // TODO: This is a process-wide singleton (a global `OnceLock`), which is @@ -31,6 +35,7 @@ pub struct AppRuntime { git: PathBuf, python3: PathBuf, cwd: PathBuf, + dirs: DirProvider, } static RUNTIME: OnceLock = OnceLock::new(); @@ -54,6 +59,7 @@ impl AppRuntime { git: pathbin("git")?, python3: pathbin("python3")?, cwd: std::env::current_dir().map_err(InitError::Cwd)?, + dirs: DirProvider::new().ok_or(InitError::Dirs)?, }) } @@ -76,6 +82,16 @@ impl AppRuntime { &Self::get().cwd } + /// The `hm`-namespaced directory layout (config, cache, …), resolved once + /// at initialization. + /// + /// # Panics + /// If [`init`](Self::init) has not been called. + #[must_use] + pub fn dirs() -> &'static DirProvider { + &Self::get().dirs + } + /// The system `git`, bound to a [`Git`] handle. /// /// # Panics diff --git a/crates/hm-common/src/dir_provider.rs b/crates/hm-common/src/dir_provider.rs new file mode 100644 index 00000000..4c7a63c8 --- /dev/null +++ b/crates/hm-common/src/dir_provider.rs @@ -0,0 +1,80 @@ +//! Platform user directories, resolved once and read as borrowed paths. +//! +//! [`DirProvider`] resolves the operating system's per-user directory roots a +//! single time at construction and hands them out as `&Path`, so consumers +//! neither re-resolve the platform roots per call nor allocate to read one. It +//! is deliberately application-agnostic: it knows nothing of Harmont's own +//! `hm/` subdirectory — callers join that (and any file name) onto these roots. +//! +//! On non-Windows we intentionally hardcode `~/.config` and `~/.cache` rather +//! than honoring `$XDG_CONFIG_HOME` / `$XDG_CACHE_HOME`: it keeps our paths +//! predictable and the two roots consistent. Revisit only if honoring the XDG +//! env vars becomes a real need. + +use std::path::{Path, PathBuf}; + +/// The platform per-user directory roots, resolved once at construction. +/// +/// Build with [`DirProvider::new`], then read the `&Path` accessors. Attach a +/// process-wide instance to the app runtime and reach it via its `dirs()` +/// accessor instead of reconstructing one per call. +#[derive(Debug, Clone)] +pub struct DirProvider { + config: PathBuf, + cache: PathBuf, +} + +impl DirProvider { + /// Resolve the platform directory roots, once. + /// + /// On non-Windows the roots are `~/.config` and `~/.cache` (the `$XDG_*` + /// env vars are intentionally ignored). Returns `None` if a root cannot be + /// determined — e.g. there is no home directory. + #[must_use] + pub fn new() -> Option { + #[cfg(windows)] + let (config, cache) = (dirs::config_dir()?, dirs::cache_dir()?); + #[cfg(not(windows))] + let (config, cache) = { + let home = dirs::home_dir()?; + (home.join(".config"), home.join(".cache")) + }; + + Some(Self { config, cache }) + } + + /// The user configuration root (`~/.config` on non-Windows). + #[must_use] + pub fn config(&self) -> &Path { + &self.config + } + + /// The user cache root (`~/.cache` on non-Windows). + #[must_use] + pub fn cache(&self) -> &Path { + &self.cache + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, reason = "test setup and assertions")] +mod tests { + use super::*; + use rstest::rstest; + + #[rstest] + fn config_is_the_platform_config_root() { + let dirs = DirProvider::new().unwrap(); + assert!(dirs.config().is_absolute(), "got {:?}", dirs.config()); + #[cfg(not(windows))] + assert!(dirs.config().ends_with(".config"), "got {:?}", dirs.config()); + } + + #[rstest] + fn cache_is_the_platform_cache_root() { + let dirs = DirProvider::new().unwrap(); + assert!(dirs.cache().is_absolute(), "got {:?}", dirs.cache()); + #[cfg(not(windows))] + assert!(dirs.cache().ends_with(".cache"), "got {:?}", dirs.cache()); + } +} diff --git a/crates/hm-common/src/dirs.rs b/crates/hm-common/src/dirs.rs index 32553826..72732c9e 100644 --- a/crates/hm-common/src/dirs.rs +++ b/crates/hm-common/src/dirs.rs @@ -1,32 +1,13 @@ -//! Harmont-specific directory resolution. +//! Project-directory discovery. //! -//! 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-common` should never need them. +//! The `hm`-namespaced directory *roots* (config, cache, …) live in +//! [`crate::dir_provider`]. This module handles the orthogonal problem of +//! locating the current project by walking up from a starting path. #![allow(clippy::must_use_candidate)] use std::path::PathBuf; -use crate::os::dirs as platform; - -/// `~/.config/hm/` — user config root (`config.toml`, `credentials.toml`). -pub fn hm_config_dir() -> Option { - platform::config_dir().map(|c| c.join("hm")) -} - -/// `~/.cache/hm/` — local build cache root (regenerable). -pub fn hm_cache_dir() -> Option { - platform::cache_dir().map(|c| c.join("hm")) -} - -/// `~/.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. @@ -47,29 +28,6 @@ mod tests { use super::*; - #[rstest] - 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:?}" - ); - } - - #[rstest] - fn hm_cache_dir_under_cache() { - let p = hm_cache_dir().unwrap(); - assert!(p.ends_with("hm"), "expected path ending in 'hm', got {p:?}"); - } - - #[rstest] - fn hm_workspace_cache_dir_resolves() { - let p = hm_workspace_cache_dir().unwrap(); - assert!(p.ends_with("hm/workspaces"), "got {p:?}"); - } - #[rstest] fn find_project_root_at_current_dir() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/hm-common/src/fs.rs b/crates/hm-common/src/fs.rs index 19bbbe93..bcc6a643 100644 --- a/crates/hm-common/src/fs.rs +++ b/crates/hm-common/src/fs.rs @@ -1,35 +1,14 @@ //! Filesystem helpers. -//! -//! Every writer here is `async` — there is no blocking variant. Two families -//! live here: -//! -//! * [`write_create_all`] — a convenience wrapper that scaffolds parent -//! directories before a plain write, with no permission guarantees. -//! * [`write_atomic`] — an atomic writer that also controls who may read the -//! result via [`Privacy`]. Readers observe either the full old contents or -//! the full new contents, never a truncated file. -//! -//! ## Privacy on Windows -//! -//! [`Privacy`] maps to Unix permission bits. On Windows it is currently a -//! **no-op** — Windows ACLs are not yet enforced, so a `Private` file is no -//! more restricted than a `Public` one. The intent is still recorded in the -//! type, so a real ACL implementation can drop in behind `#[cfg(windows)]` -//! without touching any call site. use std::io; use std::path::Path; /// Who may read a file (or directory) that hm writes. /// -/// Carries intent rather than raw mode bits: the file- and directory-mode -/// mapping is applied internally, so a secrets file can never accidentally -/// land with a directory's permissions. See the [module docs](self) for the -/// Windows caveat. +/// On Windows this is not enforced nor respected. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Privacy { - /// Owner-only. Unix file `0o600`, directory `0o700`. Use for secrets - /// (e.g. credentials). + /// Owner-only. Unix file `0o600`, directory `0o700`. Use for secrets (e.g. credentials). Private, /// World-readable, owner-writable. Unix file `0o644`, directory `0o755`. Public, @@ -57,16 +36,12 @@ impl Privacy { /// Write `contents` to `path`, creating any missing parent directories first. /// -/// Joins [`std::fs::create_dir_all`] on the parent and [`std::fs::write`] into -/// one call, so callers scaffolding a file into a not-yet-existing directory -/// don't repeat the parent-creation dance. An existing file is overwritten. -/// -/// This makes no permission guarantees — reach for [`write_atomic`] when the -/// result must be owner-only or written atomically. +/// This makes no permission guarantees — reach for [`write_atomic`] when the result must be +/// owner-only or written atomically. /// /// # Errors -/// Returns the underlying [`io::Error`] if a parent directory cannot be created -/// or the file cannot be written. +/// Returns the underlying [`io::Error`] if a parent directory cannot be created or the file cannot +/// be written. pub async fn write_create_all( path: impl AsRef, contents: impl AsRef<[u8]>, @@ -78,19 +53,11 @@ pub async fn write_create_all( tokio::fs::write(path, contents).await } -/// Atomically write `contents` to `path` at the given [`Privacy`], ensuring the -/// parent directory exists. -/// -/// The parent directory is always created owner-only (`0o700` on Unix); only -/// the file's readability is controlled by `file`. The write goes to a -/// tempfile in the same directory and is `rename`d over `path`, so a reader -/// never sees a partial file. -/// -/// # Errors +/// Atomically write `contents` to `path` at the given [`Privacy`], ensuring the parent directory +/// exists at that same privacy. /// -/// Returns an error if `path` has no parent or no file-name component, the -/// parent directory cannot be created or chmod'd, the tempfile cannot be -/// opened or written, or the final `rename` over `path` fails. +/// The parent directory is created (and, if it already exists, chmod'd) to match `file`: `0o700` +/// for [`Privacy::Private`], `0o755` for [`Privacy::Public`] on Unix. pub async fn write_atomic( path: impl AsRef, contents: impl AsRef<[u8]>, @@ -109,7 +76,7 @@ pub async fn write_atomic( })? .to_owned(); - create_dir_private(&parent).await?; + create_dir_at(&parent, file).await?; let file_name = path .file_name() @@ -134,16 +101,6 @@ pub async fn write_atomic( } /// Atomically replace `to` with `from`. -/// -/// On Unix this delegates to [`tokio::fs::rename`] (`rename(2)` — atomic -/// by POSIX guarantee). On Windows this uses `ReplaceFileW` (preserves -/// ACLs and alternate data streams) when the target exists, falling back -/// to `MoveFileExW` with `MOVEFILE_REPLACE_EXISTING` for first-write. -/// -/// # Errors -/// -/// Returns an error if the rename fails (permission denied, cross-device, -/// source missing, etc.). pub async fn atomic_rename_over(from: impl AsRef, to: impl AsRef) -> io::Result<()> { #[cfg(unix)] { @@ -194,11 +151,6 @@ pub async fn atomic_rename_over(from: impl AsRef, to: impl AsRef) -> } /// Remove a file if it exists; silently return `Ok(())` if it does not. -/// -/// # Errors -/// -/// Returns an error if `remove_file` fails for any reason other than -/// `NotFound`. pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { match tokio::fs::remove_file(path).await { Ok(()) => Ok(()), @@ -207,16 +159,12 @@ pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { } } -/// Create `dir` (and any missing parents) owner-only. -/// -/// On Unix the directory is created at, or chmod'd to, `0o700`. On Windows -/// this is a plain recursive create (see the [module docs](self) on Windows -/// privacy). -async fn create_dir_private(dir: &Path) -> io::Result<()> { +/// Create `dir` (and any missing parents) at `privacy`'s directory mode. +async fn create_dir_at(dir: &Path, privacy: Privacy) -> io::Result<()> { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - let mode = Privacy::Private.dir_mode(); + let mode = privacy.dir_mode(); match tokio::fs::metadata(dir).await { Ok(meta) => { let current = meta.permissions().mode() & 0o777; @@ -236,15 +184,12 @@ async fn create_dir_private(dir: &Path) -> io::Result<()> { #[cfg(windows)] { + let _ = privacy; tokio::fs::create_dir_all(dir).await } } /// Write `contents` to `path` at the file mode implied by `privacy`. -/// -/// On Unix the file is opened with the exact mode so the bytes never exist at -/// a laxer permission. On Windows the privacy is a no-op (see [module -/// docs](self)). async fn write_file_private(path: &Path, contents: &[u8], privacy: Privacy) -> io::Result<()> { #[cfg(unix)] { @@ -339,9 +284,9 @@ mod unix_privacy_tests { assert_eq!(mode_of(&dir), 0o700, "dir mode must be 0o700"); } - /// A `Public` file lands at 0o644, still in a 0o700 dir. + /// A `Public` file lands at 0o644, in a matching 0o755 dir. #[tokio::test] - async fn public_file_is_0644_in_dir_0700() { + async fn public_file_is_0644_in_dir_0755() { let tmp = tempfile::tempdir().unwrap(); let dir = tmp.path().join("hm"); let file = dir.join("config.toml"); @@ -351,7 +296,7 @@ mod unix_privacy_tests { .unwrap(); assert_eq!(mode_of(&file), 0o644, "file mode must be 0o644"); - assert_eq!(mode_of(&dir), 0o700, "dir mode must be 0o700"); + assert_eq!(mode_of(&dir), 0o755, "dir mode must be 0o755"); } /// Overwriting a secret must preserve 0o600 (guards the tempfile + diff --git a/crates/hm-common/src/lib.rs b/crates/hm-common/src/lib.rs index fc0d86ba..42d11b73 100644 --- a/crates/hm-common/src/lib.rs +++ b/crates/hm-common/src/lib.rs @@ -2,6 +2,7 @@ #[cfg(feature = "app-runtime")] pub mod app_runtime; +pub mod dir_provider; pub mod dirs; pub mod format; pub mod fs; @@ -11,6 +12,4 @@ pub mod python; pub mod string; pub mod time; -/// Raw platform primitives. Not part of the public surface — callers use -/// [`dirs`], which wraps these with Harmont-specific paths. -pub(crate) mod os; +pub use dir_provider::DirProvider; diff --git a/crates/hm-common/src/os/dirs.rs b/crates/hm-common/src/os/dirs.rs deleted file mode 100644 index 9027ad3a..00000000 --- a/crates/hm-common/src/os/dirs.rs +++ /dev/null @@ -1,31 +0,0 @@ -//! Raw platform directory primitives. -//! -//! This module is `pub(crate)` — external callers must use -//! [`crate::dirs`] which provides Harmont-specific accessors. -//! -//! 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; - -pub(crate) fn home_dir() -> Option { - dirs::home_dir() -} - -pub(crate) fn config_dir() -> Option { - if cfg!(windows) { - dirs::config_dir() - } else { - home_dir().map(|h| h.join(".config")) - } -} - -pub(crate) fn cache_dir() -> Option { - if cfg!(windows) { - dirs::cache_dir() - } else { - home_dir().map(|h| h.join(".cache")) - } -} diff --git a/crates/hm-common/src/os/mod.rs b/crates/hm-common/src/os/mod.rs deleted file mode 100644 index 1b1480fa..00000000 --- a/crates/hm-common/src/os/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub(crate) mod dirs; diff --git a/crates/hm-config/src/creds.rs b/crates/hm-config/src/creds.rs index 77be1050..896f1151 100644 --- a/crates/hm-config/src/creds.rs +++ b/crates/hm-config/src/creds.rs @@ -15,8 +15,8 @@ struct CredentialFile { } fn path() -> Result { - let dir = hm_common::dirs::hm_config_dir().context("could not determine config directory")?; - Ok(dir.join("credentials.toml")) + let dirs = hm_common::DirProvider::new().context("could not determine config directory")?; + Ok(dirs.config().join("hm").join("credentials.toml")) } fn load() -> CredentialFile { diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index 99b780f5..9165f4f4 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -119,8 +119,8 @@ impl Config { /// /// Returns an error if the platform config directory cannot be determined. pub fn user_config_path() -> Result { - let dir = hm_common::dirs::hm_config_dir().context("could not determine config directory")?; - Ok(dir.join("config.toml")) + let dirs = hm_common::DirProvider::new().context("could not determine config directory")?; + Ok(dirs.config().join("hm").join("config.toml")) } /// Project-level config path: `/.hm/config.toml`. diff --git a/crates/hm-exec/src/local/backend.rs b/crates/hm-exec/src/local/backend.rs index 8da851b8..c4c41a43 100644 --- a/crates/hm-exec/src/local/backend.rs +++ b/crates/hm-exec/src/local/backend.rs @@ -50,10 +50,11 @@ impl LocalBackend { /// (VM backend + snapshot registry) and registering the [`VmRunner`] as /// the default runner. fn build_registry(&self) -> Result { - let cache_dir = hm_common::dirs::hm_cache_dir().ok_or_else(|| { + let dirs = hm_common::DirProvider::new().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(&dirs.cache().join("hm").join("registry.db"), REGISTRY_CAPACITY) .map_err(|e| BackendError::Local(format!("opening snapshot registry: {e:#}")))?; let config = VmConfig { diff --git a/crates/hm/src/commands/cache/clean.rs b/crates/hm/src/commands/cache/clean.rs index a014abbd..61469eea 100644 --- a/crates/hm/src/commands/cache/clean.rs +++ b/crates/hm/src/commands/cache/clean.rs @@ -1,13 +1,15 @@ use anyhow::Result; +use hm_common::app_runtime::AppRuntime; use hm_vm::VmBackend as _; use human_units::FormatSize 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_common::dirs::hm_workspace_cache_dir() - && ws_cache.exists() - { + let hm_cache = AppRuntime::dirs().cache().join("hm"); + + let ws_cache = hm_cache.join("workspaces"); + let ws_cleaned = if ws_cache.exists() { let size = dir_size(&ws_cache); std::fs::remove_dir_all(&ws_cache)?; tracing::info!( @@ -20,24 +22,20 @@ pub async fn handle_clean() -> Result { false }; - let db_cleaned = if let Some(cache_dir) = hm_common::dirs::hm_cache_dir() { - let db_path = cache_dir.join("registry.db"); - if db_path.exists() { - // Remove the backing Docker images BEFORE deleting registry.db. - // The registry is the only index from a cache key to its tagged - // image (`forever-*`, etc.); once the DB is gone the images can't - // be located by key, and `docker image prune` only reclaims - // *dangling* images, so a tagged snapshot survives it. So we - // enumerate the registry, remove each image via the Docker - // backend (best-effort), then drop the DB. - remove_registered_images(&db_path).await; + let db_path = hm_cache.join("registry.db"); + let db_cleaned = if db_path.exists() { + // Remove the backing Docker images BEFORE deleting registry.db. + // The registry is the only index from a cache key to its tagged + // image (`forever-*`, etc.); once the DB is gone the images can't + // be located by key, and `docker image prune` only reclaims + // *dangling* images, so a tagged snapshot survives it. So we + // enumerate the registry, remove each image via the Docker + // backend (best-effort), then drop the DB. + remove_registered_images(&db_path).await; - std::fs::remove_file(&db_path)?; - tracing::info!(path = %db_path.display(), "removed VM image registry"); - true - } else { - false - } + std::fs::remove_file(&db_path)?; + tracing::info!(path = %db_path.display(), "removed VM image registry"); + true } else { false }; From cb6f1367197e17d4aad1167a760c3248cdd0ed83 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 23:13:55 -0700 Subject: [PATCH 4/6] refactor(hm-common): rename AppRuntime to SysRuntime Rename app_runtime.rs -> sys_runtime.rs, the AppRuntime type -> SysRuntime, and the app-runtime feature -> sys-runtime, across hm-common, hm, and hm-dsl-engine. The type resolves host toolchain and platform directories from the OS environment, so the "system runtime" name fits better than "app runtime". Also correct the SysRuntime::dirs() and DirProvider docblocks, which described the roots as hm-namespaced; DirProvider now exposes the raw platform roots. --- crates/hm-common/Cargo.toml | 2 +- crates/hm-common/src/dir_provider.rs | 26 +++++++--------- crates/hm-common/src/git.rs | 2 +- crates/hm-common/src/lib.rs | 4 +-- crates/hm-common/src/python.rs | 2 +- .../src/{app_runtime.rs => sys_runtime.rs} | 30 +++++++++---------- crates/hm-dsl-engine/Cargo.toml | 2 +- crates/hm-dsl-engine/src/python_engine.rs | 8 ++--- .../hm-dsl-engine/tests/python_engine_test.rs | 4 +-- crates/hm/Cargo.toml | 2 +- crates/hm/src/commands/cache/clean.rs | 4 +-- crates/hm/src/commands/run/mod.rs | 6 ++-- crates/hm/src/main.rs | 4 +-- 13 files changed, 45 insertions(+), 51 deletions(-) rename crates/hm-common/src/{app_runtime.rs => sys_runtime.rs} (84%) diff --git a/crates/hm-common/Cargo.toml b/crates/hm-common/Cargo.toml index 44a636cd..e1900042 100644 --- a/crates/hm-common/Cargo.toml +++ b/crates/hm-common/Cargo.toml @@ -12,7 +12,7 @@ categories = ["command-line-utilities"] path = "src/lib.rs" [features] -app-runtime = [] +sys-runtime = [] [dependencies] anyhow = { workspace = true } diff --git a/crates/hm-common/src/dir_provider.rs b/crates/hm-common/src/dir_provider.rs index 4c7a63c8..477bf113 100644 --- a/crates/hm-common/src/dir_provider.rs +++ b/crates/hm-common/src/dir_provider.rs @@ -1,22 +1,19 @@ -//! Platform user directories, resolved once and read as borrowed paths. +//! Platform user directories. //! -//! [`DirProvider`] resolves the operating system's per-user directory roots a -//! single time at construction and hands them out as `&Path`, so consumers -//! neither re-resolve the platform roots per call nor allocate to read one. It -//! is deliberately application-agnostic: it knows nothing of Harmont's own -//! `hm/` subdirectory — callers join that (and any file name) onto these roots. +//! Exposes the operating system's per-user directory roots. Application- +//! agnostic: it knows nothing of Harmont's own `hm/` subdirectory — callers +//! join that (and any file name) onto these roots. //! -//! On non-Windows we intentionally hardcode `~/.config` and `~/.cache` rather -//! than honoring `$XDG_CONFIG_HOME` / `$XDG_CACHE_HOME`: it keeps our paths -//! predictable and the two roots consistent. Revisit only if honoring the XDG -//! env vars becomes a real need. +//! On non-Windows the roots are `~/.config` and `~/.cache`; the `$XDG_*` env +//! vars are intentionally not honored, keeping paths predictable. use std::path::{Path, PathBuf}; -/// The platform per-user directory roots, resolved once at construction. +/// The platform per-user directory roots, resolved once at construction and +/// read as borrowed paths. /// /// Build with [`DirProvider::new`], then read the `&Path` accessors. Attach a -/// process-wide instance to the app runtime and reach it via its `dirs()` +/// process-wide instance to the system runtime and reach it via its `dirs()` /// accessor instead of reconstructing one per call. #[derive(Debug, Clone)] pub struct DirProvider { @@ -27,9 +24,8 @@ pub struct DirProvider { impl DirProvider { /// Resolve the platform directory roots, once. /// - /// On non-Windows the roots are `~/.config` and `~/.cache` (the `$XDG_*` - /// env vars are intentionally ignored). Returns `None` if a root cannot be - /// determined — e.g. there is no home directory. + /// Returns `None` if a root cannot be determined — e.g. there is no home + /// directory. #[must_use] pub fn new() -> Option { #[cfg(windows)] diff --git a/crates/hm-common/src/git.rs b/crates/hm-common/src/git.rs index d4f021e4..7149b765 100644 --- a/crates/hm-common/src/git.rs +++ b/crates/hm-common/src/git.rs @@ -21,7 +21,7 @@ pub struct Git<'bin> { } impl<'bin> Git<'bin> { - /// Wrap a `git` executable, typically via `AppRuntime::git()`. + /// Wrap a `git` executable, typically via `SysRuntime::git()`. #[must_use] pub const fn new(bin: &'bin Path) -> Self { Self { bin } diff --git a/crates/hm-common/src/lib.rs b/crates/hm-common/src/lib.rs index 42d11b73..17f9c1ea 100644 --- a/crates/hm-common/src/lib.rs +++ b/crates/hm-common/src/lib.rs @@ -1,7 +1,7 @@ //! Harmont common utilities shared across the `hm` workspace. -#[cfg(feature = "app-runtime")] -pub mod app_runtime; +#[cfg(feature = "sys-runtime")] +pub mod sys_runtime; pub mod dir_provider; pub mod dirs; pub mod format; diff --git a/crates/hm-common/src/python.rs b/crates/hm-common/src/python.rs index f0b6b9a8..a79897ba 100644 --- a/crates/hm-common/src/python.rs +++ b/crates/hm-common/src/python.rs @@ -15,7 +15,7 @@ pub struct Python<'bin> { } impl<'bin> Python<'bin> { - /// Wrap a `python3` executable, typically via `AppRuntime::python()`. + /// Wrap a `python3` executable, typically via `SysRuntime::python()`. #[must_use] pub const fn new(bin: &'bin Path) -> Self { Self { bin } diff --git a/crates/hm-common/src/app_runtime.rs b/crates/hm-common/src/sys_runtime.rs similarity index 84% rename from crates/hm-common/src/app_runtime.rs rename to crates/hm-common/src/sys_runtime.rs index 7532fdf0..617f4261 100644 --- a/crates/hm-common/src/app_runtime.rs +++ b/crates/hm-common/src/sys_runtime.rs @@ -1,4 +1,4 @@ -//! Application runtime context. +//! System runtime context. use std::path::{Path, PathBuf}; use std::sync::OnceLock; @@ -8,7 +8,7 @@ use crate::git::Git; use crate::process::{ExecutableNotFound, pathbin}; use crate::python::Python; -/// Failure to initialize the [`AppRuntime`]. +/// Failure to initialize the [`SysRuntime`]. #[derive(Debug, thiserror::Error)] pub enum InitError { /// A required executable was missing from `PATH`. @@ -24,23 +24,22 @@ pub enum InitError { // TODO: This is a process-wide singleton (a global `OnceLock`), which is // convenient but couples every caller to hidden global state. Consider -// threading an explicit `AppRuntime` handle through the application instead, +// threading an explicit `SysRuntime` handle through the application instead, // so dependencies are visible in signatures and tests can inject their own. -/// Process-wide runtime context, resolved once at startup. /// /// Install it with [`init`](Self::init), then read it from anywhere through the /// associated accessors — no threading required. #[derive(Debug)] -pub struct AppRuntime { +pub struct SysRuntime { git: PathBuf, python3: PathBuf, cwd: PathBuf, dirs: DirProvider, } -static RUNTIME: OnceLock = OnceLock::new(); +static RUNTIME: OnceLock = OnceLock::new(); -impl AppRuntime { +impl SysRuntime { /// Resolve the runtime and install it as the process-wide singleton. /// /// Call once, early in `main`, before any accessor. A later call is ignored. @@ -70,7 +69,7 @@ impl AppRuntime { fn get() -> &'static Self { RUNTIME .get() - .expect("AppRuntime::init must be called before the runtime is accessed") + .expect("SysRuntime::init must be called before the runtime is accessed") } /// The absolute working directory captured at initialization. @@ -82,8 +81,7 @@ impl AppRuntime { &Self::get().cwd } - /// The `hm`-namespaced directory layout (config, cache, …), resolved once - /// at initialization. + /// The platform user directory roots, resolved once at initialization. /// /// # Panics /// If [`init`](Self::init) has not been called. @@ -124,14 +122,14 @@ mod tests { #[rstest] fn resolve_reports_toolchain_availability() { assert_eq!( - AppRuntime::resolve().is_ok(), + SysRuntime::resolve().is_ok(), pathbin("python3").is_ok() && pathbin("git").is_ok() ); } #[rstest] fn resolve_captures_an_absolute_cwd() { - let Ok(runtime) = AppRuntime::resolve() else { + let Ok(runtime) = SysRuntime::resolve() else { eprintln!("skipping: toolchain unavailable"); return; }; @@ -140,15 +138,15 @@ mod tests { #[rstest] fn init_installs_a_globally_accessible_runtime() { - if AppRuntime::resolve().is_err() { + if SysRuntime::resolve().is_err() { eprintln!("skipping: toolchain unavailable"); return; } - AppRuntime::init().unwrap(); - assert!(AppRuntime::cwd().is_absolute()); + SysRuntime::init().unwrap(); + assert!(SysRuntime::cwd().is_absolute()); // git() binds the resolved git; a fresh temp dir is not a repo. let dir = tempfile::tempdir().unwrap(); - assert!(AppRuntime::git().repo(dir.path()).is_err()); + assert!(SysRuntime::git().repo(dir.path()).is_err()); } #[rstest] diff --git a/crates/hm-dsl-engine/Cargo.toml b/crates/hm-dsl-engine/Cargo.toml index db718a23..261b7651 100644 --- a/crates/hm-dsl-engine/Cargo.toml +++ b/crates/hm-dsl-engine/Cargo.toml @@ -12,7 +12,7 @@ categories = ["command-line-utilities"] path = "src/lib.rs" [dependencies] -hm-common = { workspace = true, features = ["app-runtime"] } +hm-common = { workspace = true, features = ["sys-runtime"] } anyhow = { workspace = true } async-trait = { workspace = true } derive_more = { workspace = true } diff --git a/crates/hm-dsl-engine/src/python_engine.rs b/crates/hm-dsl-engine/src/python_engine.rs index 9980898a..b8e2dea9 100644 --- a/crates/hm-dsl-engine/src/python_engine.rs +++ b/crates/hm-dsl-engine/src/python_engine.rs @@ -2,7 +2,7 @@ use std::path::Path; use anyhow::{Context, Result}; use async_trait::async_trait; -use hm_common::app_runtime::AppRuntime; +use hm_common::sys_runtime::SysRuntime; use hm_common::process::CapturedStreams as _; use tracing::debug; @@ -63,8 +63,8 @@ print(json.dumps(match['definition'])) pub struct SubprocessPythonEngine; impl SubprocessPythonEngine { - /// Create the engine. It runs `python3` through [`AppRuntime::python`], so - /// [`AppRuntime::init`] must have been called first. + /// Create the engine. It runs `python3` through [`SysRuntime::python`], so + /// [`SysRuntime::init`] must have been called first. #[must_use] pub const fn new() -> Self { Self @@ -80,7 +80,7 @@ impl SubprocessPythonEngine { let harmont_pkg = tmp.path().join("harmont"); bundled_sources::extract_to(&bundled_sources::HARMONT_PY, &harmont_pkg)?; - let mut py = AppRuntime::python().program(script); + let mut py = SysRuntime::python().program(script); py.args(extra_args).current_dir(project_dir); py.pythonpath(tmp.path()); diff --git a/crates/hm-dsl-engine/tests/python_engine_test.rs b/crates/hm-dsl-engine/tests/python_engine_test.rs index 76b37cc5..ca8ba081 100644 --- a/crates/hm-dsl-engine/tests/python_engine_test.rs +++ b/crates/hm-dsl-engine/tests/python_engine_test.rs @@ -10,7 +10,7 @@ use hm_dsl_engine::DslEngine; #[tokio::test] async fn python_roundtrip() { // Skip if the toolchain (python3 + git) is unavailable. - if hm_common::app_runtime::AppRuntime::init().is_err() { + if hm_common::sys_runtime::SysRuntime::init().is_err() { eprintln!("skipping: build toolchain unavailable"); return; } @@ -43,7 +43,7 @@ def ci() -> hm.Step: #[tokio::test] async fn python_registry_json_carries_triggers_and_allow_manual() { - if hm_common::app_runtime::AppRuntime::init().is_err() { + if hm_common::sys_runtime::SysRuntime::init().is_err() { eprintln!("skipping: build toolchain unavailable"); return; } diff --git a/crates/hm/Cargo.toml b/crates/hm/Cargo.toml index ec26dd95..4187392a 100644 --- a/crates/hm/Cargo.toml +++ b/crates/hm/Cargo.toml @@ -59,7 +59,7 @@ futures = "0.3" futures-util = "0.3" hm-pipeline-ir = { workspace = true } hm-plugin-protocol = { workspace = true } -hm-common = { workspace = true, features = ["app-runtime"] } +hm-common = { workspace = true, features = ["sys-runtime"] } hm-config = { workspace = true } hm-plugin-cloud = { workspace = true } harmont-cloud = { workspace = true } diff --git a/crates/hm/src/commands/cache/clean.rs b/crates/hm/src/commands/cache/clean.rs index 61469eea..1fe68280 100644 --- a/crates/hm/src/commands/cache/clean.rs +++ b/crates/hm/src/commands/cache/clean.rs @@ -1,12 +1,12 @@ use anyhow::Result; -use hm_common::app_runtime::AppRuntime; +use hm_common::sys_runtime::SysRuntime; use hm_vm::VmBackend as _; use human_units::FormatSize as _; /// # Errors /// Returns an error if workspace cache removal fails. pub async fn handle_clean() -> Result { - let hm_cache = AppRuntime::dirs().cache().join("hm"); + let hm_cache = SysRuntime::dirs().cache().join("hm"); let ws_cache = hm_cache.join("workspaces"); let ws_cleaned = if ws_cache.exists() { diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index 23a20c47..82510567 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use anyhow::{Context, Result}; use bstr::ByteSlice as _; -use hm_common::app_runtime::AppRuntime; +use hm_common::sys_runtime::SysRuntime; use hm_common::git::{GitBranch, GitRemote, GitRepo}; use hm_dsl_engine::{DslEngine, detect}; use human_units::FormatSize as _; @@ -137,7 +137,7 @@ pub async fn handle(args: RunArgs, ctx: RunContext) -> Result { // strings so `repo_root` can move into the request, and the cloud paths // below reuse `remote_url` / `default_branch` instead of re-shelling to git. let (branch, commit, repo_name, remote_url, default_branch) = { - let git = AppRuntime::git(); + let git = SysRuntime::git(); let repo = git.repo(&repo_root).ok(); let head = repo.as_ref().and_then(GitRepo::current_branch); let remote = repo.as_ref().and_then(|r| r.remote("origin")); @@ -296,7 +296,7 @@ async fn render_pipeline( ) -> Result<(std::path::PathBuf, String, String)> { let repo_root = match args.dir.clone() { Some(p) => p, - None => AppRuntime::cwd().to_path_buf(), + None => SysRuntime::cwd().to_path_buf(), }; detect::check_python(&repo_root).map_err(|e| HmError::DslEngine(format!("{e:#}")))?; diff --git a/crates/hm/src/main.rs b/crates/hm/src/main.rs index dae1a46e..c33675f7 100644 --- a/crates/hm/src/main.rs +++ b/crates/hm/src/main.rs @@ -5,7 +5,7 @@ use anyhow::Context as _; use clap::Parser; -use hm_common::app_runtime::AppRuntime; +use hm_common::sys_runtime::SysRuntime; use tracing_subscriber::EnvFilter; use tracing_subscriber::Layer; use tracing_subscriber::filter::Targets; @@ -93,7 +93,7 @@ async fn main() { async fn run(args: Cli) -> Result { // Every command runs against the build toolchain (git + python3); resolve it // once up front and fail fast with a clear error if anything is missing. - AppRuntime::init().context("resolving the build toolchain")?; + SysRuntime::init().context("resolving the build toolchain")?; let command = args.command.clone(); let ctx = RunContext::from_cli(&args)?; From c9c6cf9c79e40164298e0d81695596a2e4854a74 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 23:18:20 -0700 Subject: [PATCH 5/6] yell more at claude --- CLAUDE.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index e89e138e..b495036c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,6 +18,16 @@ When writing or running any Rust test, follow the functions and hand-rolled loops, `proptest` for domain-wide properties. Run with plain `cargo test -p ` (no nextest/just wrapper). +## Documentation + +When writing or editing any docblock, doc comment, or module header (`///`, +`//!`) — including on code you just changed — follow the +[`writing-interface-docblocks`](.claude/skills/writing-interface-docblocks/SKILL.md) +skill: a docblock is a contract, not a changelog. Terse, present-tense, no +prompt or diff leakage (`rather than`, `now returns`, `as requested`); document +the *when* of errors/panics, not the *why*; module docs name the domain, not the +one item currently inside them. + ## DSL The `harmont` Python package (pipeline DSL) lives inside `crates/hm-dsl-engine/harmont-py/` so it ships with the crate. From c9df362158e8d05676f94880f613495e19b995b7 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 25 Jul 2026 23:24:49 -0700 Subject: [PATCH 6/6] refactor: move find_project_root to the hm crate, drop DirProvider re-export Delete crates/hm-common/src/dirs.rs. Its sole remaining function, find_project_root, discovers the `.hm/` project root and is used only by the hm crate (context.rs and the config_layered test), so it moves to crates/hm/src/project.rs where it belongs. Also drop the `pub use dir_provider::DirProvider` crate-root re-export: every other hm-common type is module-qualified (hm_common::git::Git, hm_common::fs::write_atomic), so callers now use hm_common::dir_provider::DirProvider for consistency. --- crates/hm-common/src/lib.rs | 3 --- crates/hm-config/src/creds.rs | 4 ++-- crates/hm-config/src/lib.rs | 2 +- crates/hm-exec/src/local/backend.rs | 2 +- crates/hm/src/context.rs | 2 +- crates/hm/src/lib.rs | 8 ++++---- crates/{hm-common/src/dirs.rs => hm/src/project.rs} | 7 +++---- crates/hm/tests/config_layered.rs | 2 +- 8 files changed, 13 insertions(+), 17 deletions(-) rename crates/{hm-common/src/dirs.rs => hm/src/project.rs} (86%) diff --git a/crates/hm-common/src/lib.rs b/crates/hm-common/src/lib.rs index 17f9c1ea..556bfa89 100644 --- a/crates/hm-common/src/lib.rs +++ b/crates/hm-common/src/lib.rs @@ -3,7 +3,6 @@ #[cfg(feature = "sys-runtime")] pub mod sys_runtime; pub mod dir_provider; -pub mod dirs; pub mod format; pub mod fs; pub mod git; @@ -11,5 +10,3 @@ pub mod process; pub mod python; pub mod string; pub mod time; - -pub use dir_provider::DirProvider; diff --git a/crates/hm-config/src/creds.rs b/crates/hm-config/src/creds.rs index 896f1151..5a1ab705 100644 --- a/crates/hm-config/src/creds.rs +++ b/crates/hm-config/src/creds.rs @@ -1,7 +1,7 @@ //! File-backed credential store at `~/.config/hm/credentials.toml`. //! //! The file is written with [`Privacy::Private`] (0o600, parent dir 0o700) -//! via [`hm_common::fs::blocking::write_atomic`], keyed by `(service, account)`. +//! via [`hm_common::fs::write_atomic`], keyed by `(service, account)`. use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; @@ -15,7 +15,7 @@ struct CredentialFile { } fn path() -> Result { - let dirs = hm_common::DirProvider::new().context("could not determine config directory")?; + let dirs = hm_common::dir_provider::DirProvider::new().context("could not determine config directory")?; Ok(dirs.config().join("hm").join("credentials.toml")) } diff --git a/crates/hm-config/src/lib.rs b/crates/hm-config/src/lib.rs index 9165f4f4..d9bd57e8 100644 --- a/crates/hm-config/src/lib.rs +++ b/crates/hm-config/src/lib.rs @@ -119,7 +119,7 @@ impl Config { /// /// Returns an error if the platform config directory cannot be determined. pub fn user_config_path() -> Result { - let dirs = hm_common::DirProvider::new().context("could not determine config directory")?; + let dirs = hm_common::dir_provider::DirProvider::new().context("could not determine config directory")?; Ok(dirs.config().join("hm").join("config.toml")) } diff --git a/crates/hm-exec/src/local/backend.rs b/crates/hm-exec/src/local/backend.rs index c4c41a43..0f3b3b9d 100644 --- a/crates/hm-exec/src/local/backend.rs +++ b/crates/hm-exec/src/local/backend.rs @@ -50,7 +50,7 @@ impl LocalBackend { /// (VM backend + snapshot registry) and registering the [`VmRunner`] as /// the default runner. fn build_registry(&self) -> Result { - let dirs = hm_common::DirProvider::new().ok_or_else(|| { + let dirs = hm_common::dir_provider::DirProvider::new().ok_or_else(|| { BackendError::Local("cannot resolve the Harmont cache directory".into()) })?; let registry = diff --git a/crates/hm/src/context.rs b/crates/hm/src/context.rs index 99a2b5f6..d64c7b27 100644 --- a/crates/hm/src/context.rs +++ b/crates/hm/src/context.rs @@ -28,7 +28,7 @@ impl RunContext { /// 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_common::dirs::find_project_root(&start_dir); + let project_root = crate::project::find_project_root(&start_dir); let config = Config::load(project_root.as_deref())?; let output = OutputMode::Human { diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 95472547..2219d437 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -3,10 +3,9 @@ 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_common::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. Platform-directory resolution goes through +// `hm_common::DirProvider`, which owns the `dirs` dependency. Adding +// `dirs` here would bypass that single source of truth. #[allow( clippy::print_stdout, @@ -24,5 +23,6 @@ pub use hm_config as config; /// `harmont_cli::creds_store` path. pub use hm_config::creds as creds_store; pub mod context; +pub mod project; pub mod error; pub(crate) mod signal; diff --git a/crates/hm-common/src/dirs.rs b/crates/hm/src/project.rs similarity index 86% rename from crates/hm-common/src/dirs.rs rename to crates/hm/src/project.rs index 72732c9e..8a580bf9 100644 --- a/crates/hm-common/src/dirs.rs +++ b/crates/hm/src/project.rs @@ -1,8 +1,7 @@ -//! Project-directory discovery. +//! The current Harmont project on disk. //! -//! The `hm`-namespaced directory *roots* (config, cache, …) live in -//! [`crate::dir_provider`]. This module handles the orthogonal problem of -//! locating the current project by walking up from a starting path. +//! A project is any directory tree rooted at a directory that contains `.hm/`; +//! this module locates that root. #![allow(clippy::must_use_candidate)] diff --git a/crates/hm/tests/config_layered.rs b/crates/hm/tests/config_layered.rs index c0f69bec..c4d7076a 100644 --- a/crates/hm/tests/config_layered.rs +++ b/crates/hm/tests/config_layered.rs @@ -85,7 +85,7 @@ fn load_resolves_project_root() { ) .unwrap(); - let found = hm_common::dirs::find_project_root(project_dir.path()); + let found = harmont_cli::project::find_project_root(project_dir.path()); assert_eq!(found, Some(project_dir.path().to_path_buf())); let config_path = harmont_cli::config::Config::project_config_path(project_dir.path());