From 6d3f7bffc0569d279e5d9db487a27c3842e5cac8 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:40:18 -0700 Subject: [PATCH 01/25] feat: add hm-util crate skeleton with os module structure --- Cargo.lock | 10 ++++++++++ Cargo.toml | 3 +++ crates/hm-util/Cargo.toml | 19 +++++++++++++++++++ crates/hm-util/src/lib.rs | 1 + crates/hm-util/src/os/dirs.rs | 0 crates/hm-util/src/os/fs.rs | 0 crates/hm-util/src/os/mod.rs | 2 ++ 7 files changed, 35 insertions(+) create mode 100644 crates/hm-util/Cargo.toml create mode 100644 crates/hm-util/src/lib.rs create mode 100644 crates/hm-util/src/os/dirs.rs create mode 100644 crates/hm-util/src/os/fs.rs create mode 100644 crates/hm-util/src/os/mod.rs diff --git a/Cargo.lock b/Cargo.lock index 43ee1aa7..db03e5e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1711,6 +1711,16 @@ dependencies = [ "serde_json", ] +[[package]] +name = "hm-util" +version = "0.0.0-dev" +dependencies = [ + "anyhow", + "dirs", + "tempfile", + "tokio", +] + [[package]] name = "home" version = "0.5.12" diff --git a/Cargo.toml b/Cargo.toml index 53a63ce4..ce31cd7d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,11 +9,13 @@ members = [ "crates/hm-plugin-output-json", "crates/hm-plugin-cloud", "crates/hm-fixtures", + "crates/hm-util", ] default-members = [ "crates/hm", "crates/hm-plugin-protocol", "crates/hm-plugin-sdk", + "crates/hm-util", ] [workspace.package] @@ -24,6 +26,7 @@ repository = "https://github.com/harmont-dev/harmont-cli" [workspace.dependencies] hm-plugin-protocol = { path = "crates/hm-plugin-protocol", version = "0.0.0-dev" } hm-plugin-sdk = { path = "crates/hm-plugin-sdk", version = "0.0.0-dev" } +hm-util = { path = "crates/hm-util", version = "0.0.0-dev" } serde = { version = "1", features = ["derive"] } serde_json = "1" schemars = { version = "0.8", features = ["preserve_order", "semver", "uuid1", "chrono"] } diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml new file mode 100644 index 00000000..1201a305 --- /dev/null +++ b/crates/hm-util/Cargo.toml @@ -0,0 +1,19 @@ +[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" +tokio = { version = "1", features = ["rt"] } + +[dev-dependencies] +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 new file mode 100644 index 00000000..406ea475 --- /dev/null +++ b/crates/hm-util/src/lib.rs @@ -0,0 +1 @@ +pub mod os; diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs new file mode 100644 index 00000000..e69de29b diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs new file mode 100644 index 00000000..e69de29b diff --git a/crates/hm-util/src/os/mod.rs b/crates/hm-util/src/os/mod.rs new file mode 100644 index 00000000..ac4b93c8 --- /dev/null +++ b/crates/hm-util/src/os/mod.rs @@ -0,0 +1,2 @@ +pub mod dirs; +pub mod fs; From d62619876038d2efd44184d78c60dba88fd2ccbf Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:42:03 -0700 Subject: [PATCH 02/25] feat(hm-util): add os::dirs for platform directory resolution --- crates/hm-util/src/os/dirs.rs | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs index e69de29b..4d0c6ea7 100644 --- a/crates/hm-util/src/os/dirs.rs +++ b/crates/hm-util/src/os/dirs.rs @@ -0,0 +1,43 @@ +use std::path::PathBuf; + +use anyhow::{Context, Result}; + +/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). +/// +/// # Errors +/// +/// Returns an error if the home directory cannot be determined. +pub fn home_dir() -> Result { + dirs::home_dir().context("could not determine home directory") +} + +/// Platform config directory (`~/.config` on Linux, +/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). +/// +/// # Errors +/// +/// Returns an error if the config directory cannot be determined. +pub fn config_dir() -> Result { + dirs::config_dir().context("could not determine config directory") +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[test] + fn home_dir_resolves() { + let p = home_dir().unwrap(); + assert!(p.exists(), "home dir should exist: {}", p.display()); + } + + #[test] + fn config_dir_resolves() { + let p = config_dir().unwrap(); + assert!( + p.to_string_lossy().len() > 1, + "config dir should be a real path" + ); + } +} From fe3f48d51fc22588799c0c276f42f7f065aac5cf Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:42:19 -0700 Subject: [PATCH 03/25] feat(hm-util): implement os::fs with async + blocking atomic file I/O --- crates/hm-util/src/os/fs.rs | 279 ++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index e69de29b..e3c3e05b 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -0,0 +1,279 @@ +//! Atomic, permission-restricted filesystem helpers. +//! +//! The main entry point is [`write_atomic_restricted`] (async) or +//! [`blocking::write_atomic_restricted`] (sync). 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::path::Path; + +use anyhow::{Context, Result}; + +// --------------------------------------------------------------------------- +// Private sync core +// --------------------------------------------------------------------------- + +fn write_atomic_restricted_sync( + path: &Path, + contents: &[u8], + file_mode: u32, + dir_mode: u32, +) -> Result<()> { + let parent = path + .parent() + .with_context(|| format!("{} has no parent directory", path.display()))?; + + create_dir_with_mode_sync(parent, dir_mode) + .with_context(|| format!("creating {}", parent.display()))?; + + let file_name = path + .file_name() + .with_context(|| 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_sync(&tmp_path, contents, file_mode) + .with_context(|| format!("writing {}", tmp_path.display()))?; + + let persist_result = std::fs::rename(&tmp_path, path) + .with_context(|| format!("renaming {} -> {}", tmp_path.display(), path.display())); + + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result?; + + Ok(()) +} + +fn remove_if_exists_sync(path: &Path) -> Result<()> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e).with_context(|| format!("removing {}", path.display())), + } +} + +#[cfg(unix)] +fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> std::io::Result<()> { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + if dir.exists() { + let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; + if current != mode { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; + } + } else { + std::fs::DirBuilder::new() + .recursive(true) + .mode(mode) + .create(dir)?; + } + Ok(()) +} + +#[cfg(not(unix))] +fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> std::io::Result<()> { + std::fs::create_dir_all(dir) +} + +#[cfg(unix)] +fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(mode) + .open(path)?; + f.write_all(contents)?; + f.sync_all()?; + Ok(()) +} + +#[cfg(not(unix))] +fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> std::io::Result<()> { + std::fs::write(path, contents) +} + +// --------------------------------------------------------------------------- +// Public async API +// --------------------------------------------------------------------------- + +/// Write `contents` to `path` atomically with `file_mode`, ensuring the +/// parent directory exists and is set to `dir_mode`. +/// +/// This is the async counterpart of [`blocking::write_atomic_restricted`]; +/// the blocking I/O is offloaded to [`tokio::task::spawn_blocking`]. +/// +/// # 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_mode`, the +/// tempfile cannot be opened with `file_mode` or written, or the final +/// `rename` over `path` fails. +pub async fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, +) -> Result<()> { + let path = path.as_ref().to_owned(); + let contents = contents.as_ref().to_vec(); + tokio::task::spawn_blocking(move || { + write_atomic_restricted_sync(&path, &contents, file_mode, dir_mode) + }) + .await + .context("write_atomic_restricted task panicked")? +} + +/// Remove a file if it exists; silently return `Ok(())` if it does not. +/// +/// This is the async counterpart of [`blocking::remove_if_exists`]; +/// the blocking I/O is offloaded to [`tokio::task::spawn_blocking`]. +/// +/// # Errors +/// +/// Returns an error if `remove_file` fails for any reason other than +/// `NotFound`. +pub async fn remove_if_exists(path: impl AsRef) -> Result<()> { + let path = path.as_ref().to_owned(); + tokio::task::spawn_blocking(move || remove_if_exists_sync(&path)) + .await + .context("remove_if_exists task panicked")? +} + +// --------------------------------------------------------------------------- +// Public blocking module +// --------------------------------------------------------------------------- + +/// Synchronous (blocking) wrappers for callers that cannot use async, +/// such as extism `host_fn` callbacks. +pub mod blocking { + use std::path::Path; + + use anyhow::Result; + + /// Write `contents` to `path` atomically with `file_mode`, ensuring the + /// parent directory exists and is set to `dir_mode`. + /// + /// 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_mode`, the + /// tempfile cannot be opened with `file_mode` or written, or the final + /// `rename` over `path` fails. + pub fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, + ) -> Result<()> { + super::write_atomic_restricted_sync(path.as_ref(), contents.as_ref(), file_mode, dir_mode) + } + + /// 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 fn remove_if_exists(path: impl AsRef) -> Result<()> { + super::remove_if_exists_sync(path.as_ref()) + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(all(test, unix))] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::blocking; + use std::os::unix::fs::PermissionsExt; + + #[test] + fn writes_file_and_dir_with_requested_modes() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("creds"); + blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + let dir_mode = std::fs::metadata(target.parent().unwrap()) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!( + file_mode, 0o600, + "file mode must be 0o600, got {file_mode:o}" + ); + assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); + } + + #[test] + fn overwrites_existing_file_preserving_mode() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("creds"); + blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); + blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"v2"); + let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[test] + fn tightens_existing_dir_with_looser_mode() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("loose"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let target = dir.join("creds"); + blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); + + let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700); + } + + #[test] + fn remove_if_exists_is_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + blocking::remove_if_exists(&target).unwrap(); + std::fs::write(&target, "x").unwrap(); + blocking::remove_if_exists(&target).unwrap(); + assert!(!target.exists()); + } + + #[tokio::test] + async fn async_write_atomic_restricted() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("async_creds"); + super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) + .await + .unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600); + } + + #[tokio::test] + async fn async_remove_if_exists() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + super::remove_if_exists(&target).await.unwrap(); + std::fs::write(&target, "x").unwrap(); + super::remove_if_exists(&target).await.unwrap(); + assert!(!target.exists()); + } +} From 4a6c316c25ff167bcbbb2e57d1022d88a0cc424a Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:46:06 -0700 Subject: [PATCH 04/25] refactor: migrate fs_util callers to hm-util::os::fs::blocking --- crates/hm/Cargo.toml | 1 + crates/hm/src/config.rs | 2 +- crates/hm/src/creds_store.rs | 4 +- crates/hm/src/fs_util.rs | 192 ----------------------------------- crates/hm/src/lib.rs | 1 - 5 files changed, 4 insertions(+), 196 deletions(-) delete mode 100644 crates/hm/src/fs_util.rs diff --git a/crates/hm/Cargo.toml b/crates/hm/Cargo.toml index 97243c1f..36b48b2d 100644 --- a/crates/hm/Cargo.toml +++ b/crates/hm/Cargo.toml @@ -69,6 +69,7 @@ bollard = "0.18" which = "6" extism = { workspace = true } hm-plugin-protocol = { workspace = true } +hm-util = { workspace = true } schemars = { workspace = true } semver = { workspace = true } once_cell = "1" diff --git a/crates/hm/src/config.rs b/crates/hm/src/config.rs index 36da7439..da194327 100644 --- a/crates/hm/src/config.rs +++ b/crates/hm/src/config.rs @@ -81,7 +81,7 @@ impl Config { pub fn save(&self) -> Result<()> { let path = Self::path()?; let serialized = toml::to_string_pretty(self).context("serializing config")?; - crate::fs_util::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) + hm_util::os::fs::blocking::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) .with_context(|| format!("writing {}", path.display()))?; Ok(()) } diff --git a/crates/hm/src/creds_store.rs b/crates/hm/src/creds_store.rs index d994f729..730307fa 100644 --- a/crates/hm/src/creds_store.rs +++ b/crates/hm/src/creds_store.rs @@ -1,7 +1,7 @@ //! File-backed credential store at `~/.harmont/credentials.toml`. //! //! Replaces the OS keyring as the sole backend. The file is written with -//! mode 0o600 (parent dir 0o700) via [`crate::fs_util::write_atomic_restricted`]. +//! mode 0o600 (parent dir 0o700) via [`hm_util::os::fs::blocking::write_atomic_restricted`]. //! Keyed by `(service, account)` to match the host-fn ABI plugins use. use anyhow::{Context, Result}; @@ -32,7 +32,7 @@ fn load() -> CredentialFile { fn save(file: &CredentialFile) -> Result<()> { let p = path()?; let serialized = toml::to_string_pretty(file).context("serializing credentials")?; - crate::fs_util::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) + hm_util::os::fs::blocking::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) .with_context(|| format!("writing {}", p.display()))?; Ok(()) } diff --git a/crates/hm/src/fs_util.rs b/crates/hm/src/fs_util.rs deleted file mode 100644 index 40523afa..00000000 --- a/crates/hm/src/fs_util.rs +++ /dev/null @@ -1,192 +0,0 @@ -//! Small filesystem helpers for atomic, permission-restricted writes. -//! -//! The main entry point is [`write_atomic_restricted`]. It is used by -//! [`crate::creds_store`] (file-backed credential store) and by -//! `Config::save`, both of which write into `~/.harmont/` -//! (see `config::user_config_dir`). - -use anyhow::{Context, Result}; -use std::path::Path; - -/// Write `contents` to `path` atomically with `file_mode`, ensuring the -/// parent directory exists and is set to `dir_mode`. -/// -/// On Unix the target file is created with `OpenOptions::mode(file_mode)` -/// before any bytes are written, closing the TOCTOU window that -/// `fs::write(…)` + `set_permissions(…)` opens. The parent directory is -/// created with `DirBuilder::mode(dir_mode)`; if the directory already -/// exists with a looser mode, it is tightened. -/// -/// On non-Unix platforms the mode arguments are ignored and the function -/// falls back to `std::fs::create_dir_all` + tempfile + rename. -/// -/// Atomicity: contents are written to a sibling tempfile and then -/// `rename`d over `path`, so readers always observe either the full old -/// contents or the full new contents — never a truncated file. -/// -/// # 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_mode`, the -/// tempfile cannot be opened with `file_mode` or written, or the final -/// `rename` over `path` fails. The tempfile is cleaned up on rename -/// failure so secret material doesn't linger. -pub fn write_atomic_restricted( - path: &Path, - contents: &[u8], - file_mode: u32, - dir_mode: u32, -) -> Result<()> { - let parent = path - .parent() - .with_context(|| format!("{} has no parent directory", path.display()))?; - - create_dir_with_mode(parent, dir_mode) - .with_context(|| format!("creating {}", parent.display()))?; - - // Tempfile name is unique per process (pid) and per target filename, - // which is sufficient because write_atomic_restricted is never called - // concurrently on the same target from within a single process. - let file_name = path - .file_name() - .with_context(|| 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_mode) - .with_context(|| format!("writing {}", tmp_path.display()))?; - - let persist_result = std::fs::rename(&tmp_path, path) - .with_context(|| format!("renaming {} -> {}", tmp_path.display(), path.display())); - - if persist_result.is_err() { - // Clean up the orphaned tempfile — it may contain secret material - // and we don't want it sitting at an unexpected path. - let _ = std::fs::remove_file(&tmp_path); - } - persist_result?; - - Ok(()) -} - -/// 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` (typically permission denied or the path being a -/// non-empty directory). -pub fn remove_if_exists(path: &Path) -> Result<()> { - match std::fs::remove_file(path) { - Ok(()) => Ok(()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e).with_context(|| format!("removing {}", path.display())), - } -} - -#[cfg(unix)] -fn create_dir_with_mode(dir: &Path, mode: u32) -> std::io::Result<()> { - use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; - if dir.exists() { - let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; - if current != mode { - std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; - } - } else { - std::fs::DirBuilder::new() - .recursive(true) - .mode(mode) - .create(dir)?; - } - Ok(()) -} - -#[cfg(not(unix))] -fn create_dir_with_mode(dir: &Path, _mode: u32) -> std::io::Result<()> { - std::fs::create_dir_all(dir) -} - -#[cfg(unix)] -fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> std::io::Result<()> { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut f = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(mode) - .open(path)?; - f.write_all(contents)?; - f.sync_all()?; - Ok(()) -} - -#[cfg(not(unix))] -fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> std::io::Result<()> { - std::fs::write(path, contents) -} - -#[cfg(all(test, unix))] -#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] -mod tests { - use super::*; - use std::os::unix::fs::PermissionsExt; - - #[test] - fn writes_file_and_dir_with_requested_modes() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("creds"); - write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - let dir_mode = std::fs::metadata(target.parent().unwrap()) - .unwrap() - .permissions() - .mode() - & 0o777; - assert_eq!( - file_mode, 0o600, - "file mode must be 0o600, got {file_mode:o}" - ); - assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); - } - - #[test] - fn overwrites_existing_file_preserving_mode() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("creds"); - write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); - write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"v2"); - let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600); - } - - #[test] - fn tightens_existing_dir_with_looser_mode() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("loose"); - std::fs::create_dir(&dir).unwrap(); - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); - - let target = dir.join("creds"); - write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); - - let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dir_mode, 0o700); - } - - #[test] - fn remove_if_exists_is_idempotent() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - remove_if_exists(&target).unwrap(); - std::fs::write(&target, "x").unwrap(); - remove_if_exists(&target).unwrap(); - assert!(!target.exists()); - } -} diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 7549f7a8..6795ecdb 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -11,7 +11,6 @@ pub mod context; pub mod creds_store; pub mod dispatcher; pub mod error; -pub mod fs_util; pub mod orchestrator; pub mod output; pub mod plugin; From 7274073777116979d250345646596cec1546848d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:48:07 -0700 Subject: [PATCH 05/25] refactor: delegate directory resolution to hm-util::os::dirs --- crates/hm/Cargo.toml | 1 - crates/hm/src/config.rs | 3 +-- crates/hm/src/plugin/host_fns.rs | 2 +- crates/hm/src/plugin/paths.rs | 4 +++- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/hm/Cargo.toml b/crates/hm/Cargo.toml index 36b48b2d..6a421587 100644 --- a/crates/hm/Cargo.toml +++ b/crates/hm/Cargo.toml @@ -46,7 +46,6 @@ tar = "0.4" flate2 = "1" fs2 = { workspace = true } ignore = "0.4" -dirs = "6" tempfile = "3" anyhow = "1" thiserror = "2" diff --git a/crates/hm/src/config.rs b/crates/hm/src/config.rs index da194327..c89d50f7 100644 --- a/crates/hm/src/config.rs +++ b/crates/hm/src/config.rs @@ -12,8 +12,7 @@ const DEFAULT_API_URL: &str = "https://api.harmont.dev"; /// (the `dirs` crate's platform-specific lookup fails — typically only /// happens in restrictive sandboxes with no `HOME` / passwd entry). pub fn user_config_dir() -> Result { - let home = dirs::home_dir().context("could not determine home directory")?; - Ok(home.join(".harmont")) + Ok(hm_util::os::dirs::home_dir()?.join(".harmont")) } /// User preferences stored alongside the config. diff --git a/crates/hm/src/plugin/host_fns.rs b/crates/hm/src/plugin/host_fns.rs index 50ec8705..7091326c 100644 --- a/crates/hm/src/plugin/host_fns.rs +++ b/crates/hm/src/plugin/host_fns.rs @@ -652,7 +652,7 @@ pub fn kv_set_impl(scope: KvScope, key: &str, val: Vec) { // is not a practical concern. fn plugin_state_path() -> Option { - let dir = dirs::config_dir()?.join("harmont").join("state"); + let dir = hm_util::os::dirs::config_dir().ok()?.join("harmont").join("state"); let plugin = current_plugin_name()?; Some(dir.join(format!("{plugin}.kv"))) } diff --git a/crates/hm/src/plugin/paths.rs b/crates/hm/src/plugin/paths.rs index ebc82b90..d098e444 100644 --- a/crates/hm/src/plugin/paths.rs +++ b/crates/hm/src/plugin/paths.rs @@ -13,7 +13,9 @@ use std::path::PathBuf; /// `~/.config/harmont/plugins/` (or the platform's XDG equivalent). /// User-global plugins live here. pub fn user_plugins_dir() -> Option { - dirs::config_dir().map(|p| p.join("harmont").join("plugins")) + hm_util::os::dirs::config_dir() + .ok() + .map(|p| p.join("harmont").join("plugins")) } /// `/.harmont/plugins/`. Project-local plugins live here. From c32ae6b76c2e08d572164a4906b27608b42b6d9d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:48:22 -0700 Subject: [PATCH 06/25] refactor: replace custom CancellationToken with tokio_util::sync::CancellationToken Eliminates 55-line Arc wrapper. The 50ms polling loop in wait_cancel is replaced by the zero-cost .cancelled() future. --- crates/hm/src/orchestrator/cancel.rs | 54 ------------------- crates/hm/src/orchestrator/docker_host_fns.rs | 10 +--- crates/hm/src/orchestrator/mod.rs | 1 - crates/hm/src/orchestrator/scheduler.rs | 2 +- crates/hm/src/orchestrator/state.rs | 2 +- crates/hm/src/plugin/signal.rs | 2 +- 6 files changed, 5 insertions(+), 66 deletions(-) delete mode 100644 crates/hm/src/orchestrator/cancel.rs diff --git a/crates/hm/src/orchestrator/cancel.rs b/crates/hm/src/orchestrator/cancel.rs deleted file mode 100644 index 7a34ec13..00000000 --- a/crates/hm/src/orchestrator/cancel.rs +++ /dev/null @@ -1,54 +0,0 @@ -//! Cancellation atomic. -//! -//! Ctrl-C handlers and orchestrator failure paths flip the atomic; -//! the `hm_should_cancel` host fn reports its state to plugins; -//! plugins poll between long-running operations and unwind quickly. - -use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; - -#[derive(Debug, Clone, Default)] -pub struct CancellationToken { - inner: Arc, -} - -impl CancellationToken { - #[must_use] - pub fn new() -> Self { - Self::default() - } - - pub fn cancel(&self) { - self.inner.store(true, Ordering::SeqCst); - } - - #[must_use] - pub fn is_cancelled(&self) -> bool { - self.inner.load(Ordering::SeqCst) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn default_is_not_cancelled() { - assert!(!CancellationToken::new().is_cancelled()); - } - - #[test] - fn cancel_persists() { - let t = CancellationToken::new(); - t.cancel(); - assert!(t.is_cancelled()); - } - - #[test] - fn cancel_is_clone_shared() { - let t = CancellationToken::new(); - let u = t.clone(); - t.cancel(); - assert!(u.is_cancelled()); - } -} diff --git a/crates/hm/src/orchestrator/docker_host_fns.rs b/crates/hm/src/orchestrator/docker_host_fns.rs index 1375fc4f..7e7106f4 100644 --- a/crates/hm/src/orchestrator/docker_host_fns.rs +++ b/crates/hm/src/orchestrator/docker_host_fns.rs @@ -160,14 +160,8 @@ pub(crate) async fn exec_impl(args: DockerExecArgs) -> Result { i32::try_from(rc).context("docker exit code out of i32 range") } -async fn wait_cancel(cancel: &crate::orchestrator::cancel::CancellationToken) { - // Poll the atomic every 50ms. Cheap; never wakes a thread early. - loop { - if cancel.is_cancelled() { - return; - } - tokio::time::sleep(std::time::Duration::from_millis(50)).await; - } +async fn wait_cancel(cancel: &tokio_util::sync::CancellationToken) { + cancel.cancelled().await; } pub(crate) async fn commit_impl(args: DockerCommitArgs) -> Result { diff --git a/crates/hm/src/orchestrator/mod.rs b/crates/hm/src/orchestrator/mod.rs index a72d7129..a7e856c6 100644 --- a/crates/hm/src/orchestrator/mod.rs +++ b/crates/hm/src/orchestrator/mod.rs @@ -8,7 +8,6 @@ pub mod archive; pub mod cache; -pub mod cancel; pub mod docker_client; pub mod docker_host_fns; pub mod events; diff --git a/crates/hm/src/orchestrator/scheduler.rs b/crates/hm/src/orchestrator/scheduler.rs index 6a38f4b4..d65d916f 100644 --- a/crates/hm/src/orchestrator/scheduler.rs +++ b/crates/hm/src/orchestrator/scheduler.rs @@ -44,7 +44,7 @@ use crate::plugin::{PluginRegistry, RegistryConfig}; use super::archive::ArchiveStore; use super::cache; -use super::cancel::CancellationToken; +use tokio_util::sync::CancellationToken; use super::events::EventBus; use super::state::{self, OrchestratorState}; diff --git a/crates/hm/src/orchestrator/state.rs b/crates/hm/src/orchestrator/state.rs index ecfaa5a2..e5776a06 100644 --- a/crates/hm/src/orchestrator/state.rs +++ b/crates/hm/src/orchestrator/state.rs @@ -24,7 +24,7 @@ use uuid::Uuid; use crate::orchestrator::docker_client::DockerClient; use super::archive::ArchiveStore; -use super::cancel::CancellationToken; +use tokio_util::sync::CancellationToken; use super::events::EventBus; /// Live state visible to every host fn while an orchestrator run is diff --git a/crates/hm/src/plugin/signal.rs b/crates/hm/src/plugin/signal.rs index 83c13160..ebe5c1b7 100644 --- a/crates/hm/src/plugin/signal.rs +++ b/crates/hm/src/plugin/signal.rs @@ -15,7 +15,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; -use crate::orchestrator::cancel::CancellationToken; +use tokio_util::sync::CancellationToken; /// Spawn a tokio task that listens for SIGINT (Ctrl-C) and flips /// the token. Returns a handle; aborting the handle is sufficient From 63a14a3fe8f81378ff5ee0ac91f620757a9f0988 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:54:01 -0700 Subject: [PATCH 07/25] docs: update CLAUDE.md files for hm-util extraction Add hm-util to workspace crate listing. Update orchestrator docs to reflect CancellationToken is now tokio_util::sync, not a custom module. --- CLAUDE.md | 1 + crates/hm/CLAUDE.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0ab594bf..c90dc54e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,7 @@ The `cli/` directory is a Cargo workspace. - `crates/hm/` — the `hm` binary (today's CLI body). +- `crates/hm-util/` — shared OS and filesystem utilities. - `crates/hm-plugin-protocol/` — wire types (serde structs only). - `crates/hm-plugin-sdk/` — authoring SDK for plugin writers. - `crates/hm-fixtures/` — test-only WASM plugins; compiled to diff --git a/crates/hm/CLAUDE.md b/crates/hm/CLAUDE.md index e9b1a880..51f28a50 100644 --- a/crates/hm/CLAUDE.md +++ b/crates/hm/CLAUDE.md @@ -18,7 +18,7 @@ archive once into memory (`archive.rs` + `source.rs`), and drives the Docker daemon via the Bollard wrapper (`docker_client.rs`, exposed to step plugins through `docker_host_fns.rs`). -- Owns run-wide cancellation (`cancel.rs`) and shared mutable state +- Owns run-wide cancellation (`tokio_util::sync::CancellationToken`) and shared mutable state (`state.rs`) so step plugins can coordinate without reaching across module boundaries. From 428f6849fe51d6471c379ba24316998114145330 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 10:59:51 -0700 Subject: [PATCH 08/25] fix: use direct anyhow dep in hm-util after main merge Main removed anyhow from workspace.dependencies; hm-util still referenced it via workspace = true. --- crates/hm-util/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml index 1201a305..8256d43e 100644 --- a/crates/hm-util/Cargo.toml +++ b/crates/hm-util/Cargo.toml @@ -7,7 +7,7 @@ repository.workspace = true description = "Shared OS and filesystem utilities for Harmont crates." [dependencies] -anyhow = { workspace = true } +anyhow = "1" dirs = "6" tokio = { version = "1", features = ["rt"] } From db57676674c3dc71343e0409e689629864ff6cab Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 11:22:02 -0700 Subject: [PATCH 09/25] better dirs --- crates/hm-util/src/dirs.rs | 21 ++++++++++++ crates/hm-util/src/lib.rs | 1 + crates/hm-util/src/os/dirs.rs | 63 ++++++++++++++++++----------------- 3 files changed, 55 insertions(+), 30 deletions(-) create mode 100644 crates/hm-util/src/dirs.rs diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs new file mode 100644 index 00000000..7da2dae0 --- /dev/null +++ b/crates/hm-util/src/dirs.rs @@ -0,0 +1,21 @@ +use futures::stream::FuturesOrdered; +use std::io; + +/// Find the best harmont config directory. +/// +/// The harmont config directory is found by searching for +/// ```txt +/// ~/.hm +/// /etc/hm +/// ``` +/// +/// in that order. If any of these directories are found, then the first one, +/// in that precedence, will be returned. +/// +/// Note that the directory does not need to be well-formed to be considered. +/// +/// Note Windows uses `C:\ProgramData`. Note we do not respect `Application Support` on mac because +/// it confuses everyone. +pub async fn config_dir() -> io::Result { + // TODO(markovejnovic): Send out multtiple parallel tokio tasks which return stuff in order. +} diff --git a/crates/hm-util/src/lib.rs b/crates/hm-util/src/lib.rs index 406ea475..56ea3f30 100644 --- a/crates/hm-util/src/lib.rs +++ b/crates/hm-util/src/lib.rs @@ -1 +1,2 @@ pub mod os; +pub mod dirs; diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs index 4d0c6ea7..42e8e985 100644 --- a/crates/hm-util/src/os/dirs.rs +++ b/crates/hm-util/src/os/dirs.rs @@ -1,43 +1,46 @@ use std::path::PathBuf; +use tokio; +use io; -use anyhow::{Context, Result}; /// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). -/// -/// # Errors -/// -/// Returns an error if the home directory cannot be determined. -pub fn home_dir() -> Result { - dirs::home_dir().context("could not determine home directory") +pub fn home_dir() -> Option { + dirs::home_dir() } -/// Platform config directory (`~/.config` on Linux, -/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). +/// Platform config directory (`~/.config` on Posix, `%APPDATA%` on Windows). /// -/// # Errors -/// -/// Returns an error if the config directory cannot be determined. -pub fn config_dir() -> Result { - dirs::config_dir().context("could not determine config directory") -} +/// Note this doesn't respect XDG. +pub async fn user_config_dir() -> io::Result> { + #[cfg(unix)] + { + home_dir().and_then(async move |d: PathBuf| { + let d: PathBuf = d.join(".config"); + if tokio::fs::try_exists(d).await? { + Some(d) + } else { + None + } + }) + } -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; + #[cfg(windows)] + { + Ok(dirs::config_dir()) + } +} - #[test] - fn home_dir_resolves() { - let p = home_dir().unwrap(); - assert!(p.exists(), "home dir should exist: {}", p.display()); +/// Platform-equivalent of /etc/. +pub async fn sys_config_dir() -> PathBuf { + #[cfg(unix)] + { + PathBuf::from("/etc") } - #[test] - fn config_dir_resolves() { - let p = config_dir().unwrap(); - assert!( - p.to_string_lossy().len() > 1, - "config dir should be a real path" - ); + #[cfg(windows)] + { + std::env::var_os("ProgramData") + .map(PathBuf::from) + .unwrap_or("C:\\ProgramData") } } From 189da398fc2c010283710e9fd77e04fff7026382 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 11:27:09 -0700 Subject: [PATCH 10/25] refactor(hm-util): remove anyhow dep, implement dirs modules - os::fs returns std::io::Result instead of anyhow::Result - os::dirs returns Option instead of anyhow::Result - os::dirs adds sys_config_dir() for /etc (Unix) / C:\ProgramData (Win) - New top-level dirs module for harmont config dir discovery (searches ~/.hm then /etc/hm) - Update hm callers for new return types --- crates/hm-util/Cargo.toml | 3 +- crates/hm-util/src/dirs.rs | 76 ++++++++++++++++++++++++++------ crates/hm-util/src/os/dirs.rs | 71 ++++++++++++++++++----------- crates/hm-util/src/os/fs.rs | 64 +++++++++++++-------------- crates/hm/src/config.rs | 4 +- crates/hm/src/plugin/host_fns.rs | 2 +- crates/hm/src/plugin/paths.rs | 1 - 7 files changed, 146 insertions(+), 75 deletions(-) diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml index 8256d43e..758c0f65 100644 --- a/crates/hm-util/Cargo.toml +++ b/crates/hm-util/Cargo.toml @@ -7,9 +7,8 @@ repository.workspace = true description = "Shared OS and filesystem utilities for Harmont crates." [dependencies] -anyhow = "1" dirs = "6" -tokio = { version = "1", features = ["rt"] } +tokio = { version = "1", features = ["rt", "fs"] } [dev-dependencies] tempfile = "3" diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs index 7da2dae0..34ac816c 100644 --- a/crates/hm-util/src/dirs.rs +++ b/crates/hm-util/src/dirs.rs @@ -1,21 +1,71 @@ -use futures::stream::FuturesOrdered; +//! Harmont config directory discovery. +//! +//! Searches for the harmont config directory by checking, in order: +//! 1. `~/.hm` +//! 2. `/etc/hm` (or `C:\ProgramData\hm` on Windows) +//! +//! The first directory that exists on disk wins. The directory does not +//! need to be well-formed to be selected — existence is sufficient. +//! +//! Windows uses `C:\ProgramData`. macOS uses `~/.hm` rather than +//! `~/Library/Application Support` because that confuses everyone. + use std::io; +use std::path::PathBuf; /// Find the best harmont config directory. /// -/// The harmont config directory is found by searching for -/// ```txt -/// ~/.hm -/// /etc/hm -/// ``` +/// Returns the first existing directory from the search order: +/// `~/.hm`, then the system config dir (`/etc/hm`). +/// +/// Returns `None` if no candidate directory exists. +pub async fn config_dir() -> Option { + let candidates = [ + crate::os::dirs::home_dir().map(|h| h.join(".hm")), + Some(crate::os::dirs::sys_config_dir().join("hm")), + ]; + + for candidate in candidates.into_iter().flatten() { + if tokio::fs::try_exists(&candidate).await.unwrap_or(false) { + return Some(candidate); + } + } + None +} + +/// Find the best harmont config directory, or return an error. /// -/// in that order. If any of these directories are found, then the first one, -/// in that precedence, will be returned. +/// Same search as [`config_dir`], but returns an [`io::Error`] if no +/// candidate directory is found. /// -/// Note that the directory does not need to be well-formed to be considered. +/// # Errors /// -/// Note Windows uses `C:\ProgramData`. Note we do not respect `Application Support` on mac because -/// it confuses everyone. -pub async fn config_dir() -> io::Result { - // TODO(markovejnovic): Send out multtiple parallel tokio tasks which return stuff in order. +/// Returns [`io::ErrorKind::NotFound`] if neither `~/.hm` nor the +/// system config directory exists. +pub async fn config_dir_required() -> io::Result { + config_dir().await.ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "no harmont config directory found (searched ~/.hm, /etc/hm)", + ) + }) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[tokio::test] + async fn config_dir_does_not_panic() { + let _ = config_dir().await; + } + + #[tokio::test] + async fn config_dir_required_gives_not_found_when_missing() { + let result = config_dir_required().await; + if let Err(e) = result { + assert_eq!(e.kind(), io::ErrorKind::NotFound); + } + } } diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs index 42e8e985..1b664de5 100644 --- a/crates/hm-util/src/os/dirs.rs +++ b/crates/hm-util/src/os/dirs.rs @@ -1,37 +1,30 @@ -use std::path::PathBuf; -use tokio; -use io; +//! Platform directory resolution. +//! +//! Thin wrappers around the [`dirs`] crate that provide consistent +//! return types. Application-specific paths (e.g. `~/.harmont/`) +//! belong in the consuming crate, not here. +use std::path::PathBuf; /// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). +#[must_use] pub fn home_dir() -> Option { dirs::home_dir() } -/// Platform config directory (`~/.config` on Posix, `%APPDATA%` on Windows). +/// Platform user config directory (`~/.config` on Linux, +/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). /// -/// Note this doesn't respect XDG. -pub async fn user_config_dir() -> io::Result> { - #[cfg(unix)] - { - home_dir().and_then(async move |d: PathBuf| { - let d: PathBuf = d.join(".config"); - if tokio::fs::try_exists(d).await? { - Some(d) - } else { - None - } - }) - } - - #[cfg(windows)] - { - Ok(dirs::config_dir()) - } +/// Respects `$XDG_CONFIG_HOME` on Linux. +#[must_use] +pub fn config_dir() -> Option { + dirs::config_dir() } -/// Platform-equivalent of /etc/. -pub async fn sys_config_dir() -> PathBuf { +/// System-wide config directory (`/etc` on Unix, `C:\ProgramData` on +/// Windows). +#[must_use] +pub fn sys_config_dir() -> PathBuf { #[cfg(unix)] { PathBuf::from("/etc") @@ -41,6 +34,34 @@ pub async fn sys_config_dir() -> PathBuf { { std::env::var_os("ProgramData") .map(PathBuf::from) - .unwrap_or("C:\\ProgramData") + .unwrap_or_else(|| PathBuf::from("C:\\ProgramData")) + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[test] + fn home_dir_resolves() { + let p = home_dir().unwrap(); + assert!(p.exists(), "home dir should exist: {}", p.display()); + } + + #[test] + fn config_dir_resolves() { + let p = config_dir().unwrap(); + assert!( + p.to_string_lossy().len() > 1, + "config dir should be a real path" + ); + } + + #[test] + fn sys_config_dir_is_etc() { + let p = sys_config_dir(); + #[cfg(unix)] + assert_eq!(p, PathBuf::from("/etc")); } } diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index e3c3e05b..06b0ac92 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -6,10 +6,9 @@ //! contents — never a truncated file — and that Unix file/directory //! modes are set atomically with creation. +use std::io; use std::path::Path; -use anyhow::{Context, Result}; - // --------------------------------------------------------------------------- // Private sync core // --------------------------------------------------------------------------- @@ -19,46 +18,48 @@ fn write_atomic_restricted_sync( contents: &[u8], file_mode: u32, dir_mode: u32, -) -> Result<()> { - let parent = path - .parent() - .with_context(|| format!("{} has no parent directory", path.display()))?; +) -> io::Result<()> { + let parent = path.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("{} has no parent directory", path.display()), + ) + })?; - create_dir_with_mode_sync(parent, dir_mode) - .with_context(|| format!("creating {}", parent.display()))?; + create_dir_with_mode_sync(parent, dir_mode)?; let file_name = path .file_name() - .with_context(|| format!("{} has no file name", path.display()))? + .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_sync(&tmp_path, contents, file_mode) - .with_context(|| format!("writing {}", tmp_path.display()))?; - - let persist_result = std::fs::rename(&tmp_path, path) - .with_context(|| format!("renaming {} -> {}", tmp_path.display(), path.display())); + write_file_with_mode_sync(&tmp_path, contents, file_mode)?; + let persist_result = std::fs::rename(&tmp_path, path); if persist_result.is_err() { let _ = std::fs::remove_file(&tmp_path); } - persist_result?; - - Ok(()) + persist_result } -fn remove_if_exists_sync(path: &Path) -> Result<()> { +fn remove_if_exists_sync(path: &Path) -> io::Result<()> { match std::fs::remove_file(path) { Ok(()) => Ok(()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e).with_context(|| format!("removing {}", path.display())), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e), } } #[cfg(unix)] -fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> std::io::Result<()> { +fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> io::Result<()> { use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; if dir.exists() { let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; @@ -75,12 +76,12 @@ fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> std::io::Result<()> { } #[cfg(not(unix))] -fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> std::io::Result<()> { +fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> io::Result<()> { std::fs::create_dir_all(dir) } #[cfg(unix)] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> std::io::Result<()> { +fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { use std::io::Write; use std::os::unix::fs::OpenOptionsExt; let mut f = std::fs::OpenOptions::new() @@ -95,7 +96,7 @@ fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> std::io } #[cfg(not(unix))] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> std::io::Result<()> { +fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { std::fs::write(path, contents) } @@ -120,14 +121,14 @@ pub async fn write_atomic_restricted( contents: impl AsRef<[u8]>, file_mode: u32, dir_mode: u32, -) -> Result<()> { +) -> io::Result<()> { let path = path.as_ref().to_owned(); let contents = contents.as_ref().to_vec(); tokio::task::spawn_blocking(move || { write_atomic_restricted_sync(&path, &contents, file_mode, dir_mode) }) .await - .context("write_atomic_restricted task panicked")? + .map_err(io::Error::other)? } /// Remove a file if it exists; silently return `Ok(())` if it does not. @@ -139,11 +140,11 @@ pub async fn write_atomic_restricted( /// /// Returns an error if `remove_file` fails for any reason other than /// `NotFound`. -pub async fn remove_if_exists(path: impl AsRef) -> Result<()> { +pub async fn remove_if_exists(path: impl AsRef) -> io::Result<()> { let path = path.as_ref().to_owned(); tokio::task::spawn_blocking(move || remove_if_exists_sync(&path)) .await - .context("remove_if_exists task panicked")? + .map_err(io::Error::other)? } // --------------------------------------------------------------------------- @@ -153,10 +154,9 @@ pub async fn remove_if_exists(path: impl AsRef) -> Result<()> { /// Synchronous (blocking) wrappers for callers that cannot use async, /// such as extism `host_fn` callbacks. pub mod blocking { + use std::io; use std::path::Path; - use anyhow::Result; - /// Write `contents` to `path` atomically with `file_mode`, ensuring the /// parent directory exists and is set to `dir_mode`. /// @@ -173,7 +173,7 @@ pub mod blocking { contents: impl AsRef<[u8]>, file_mode: u32, dir_mode: u32, - ) -> Result<()> { + ) -> io::Result<()> { super::write_atomic_restricted_sync(path.as_ref(), contents.as_ref(), file_mode, dir_mode) } @@ -183,7 +183,7 @@ pub mod blocking { /// /// Returns an error if `remove_file` fails for any reason other than /// `NotFound`. - pub fn remove_if_exists(path: impl AsRef) -> Result<()> { + pub fn remove_if_exists(path: impl AsRef) -> io::Result<()> { super::remove_if_exists_sync(path.as_ref()) } } diff --git a/crates/hm/src/config.rs b/crates/hm/src/config.rs index c89d50f7..d61a7baf 100644 --- a/crates/hm/src/config.rs +++ b/crates/hm/src/config.rs @@ -12,7 +12,9 @@ const DEFAULT_API_URL: &str = "https://api.harmont.dev"; /// (the `dirs` crate's platform-specific lookup fails — typically only /// happens in restrictive sandboxes with no `HOME` / passwd entry). pub fn user_config_dir() -> Result { - Ok(hm_util::os::dirs::home_dir()?.join(".harmont")) + let home = hm_util::os::dirs::home_dir() + .context("could not determine home directory")?; + Ok(home.join(".harmont")) } /// User preferences stored alongside the config. diff --git a/crates/hm/src/plugin/host_fns.rs b/crates/hm/src/plugin/host_fns.rs index 99ae1bba..9d36e8ff 100644 --- a/crates/hm/src/plugin/host_fns.rs +++ b/crates/hm/src/plugin/host_fns.rs @@ -652,7 +652,7 @@ pub fn kv_set_impl(scope: KvScope, key: &str, val: Vec) { // is not a practical concern. fn plugin_state_path() -> Option { - let dir = hm_util::os::dirs::config_dir().ok()?.join("harmont").join("state"); + let dir = hm_util::os::dirs::config_dir()?.join("harmont").join("state"); let plugin = current_plugin_name()?; Some(dir.join(format!("{plugin}.kv"))) } diff --git a/crates/hm/src/plugin/paths.rs b/crates/hm/src/plugin/paths.rs index d098e444..6c160363 100644 --- a/crates/hm/src/plugin/paths.rs +++ b/crates/hm/src/plugin/paths.rs @@ -14,7 +14,6 @@ use std::path::PathBuf; /// User-global plugins live here. pub fn user_plugins_dir() -> Option { hm_util::os::dirs::config_dir() - .ok() .map(|p| p.join("harmont").join("plugins")) } From 7c52724b9b43b6b2e994b9e2d3025edd29348cae Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 11:38:13 -0700 Subject: [PATCH 11/25] refactor: consolidate dirs into hm_util::dirs, remove os::dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Merge os::dirs primitives (home_dir, config_dir, sys_config_dir) into hm_util::dirs alongside harmont-specific discovery - Delete os/dirs.rs — hm_util::dirs is the sole public API - Update all hm callers from hm_util::os::dirs to hm_util::dirs - Add prohibition comment in hm/src/lib.rs: dirs crate must not be added as a direct dependency of hm --- Cargo.lock | 3 +- crates/hm-util/src/dirs.rs | 109 ++- crates/hm-util/src/os/dirs.rs | 67 -- crates/hm-util/src/os/mod.rs | 1 - crates/hm/src/config.rs | 2 +- crates/hm/src/lib.rs | 5 + crates/hm/src/plugin/host_fns.rs | 4 +- crates/hm/src/plugin/paths.rs | 2 +- docs/plans/2026-05-23-extract-hm-util.md | 957 +++++++++++++++++++++++ 9 files changed, 1054 insertions(+), 96 deletions(-) delete mode 100644 crates/hm-util/src/os/dirs.rs create mode 100644 docs/plans/2026-05-23-extract-hm-util.md diff --git a/Cargo.lock b/Cargo.lock index db03e5e4..c50cfdff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1542,7 +1542,6 @@ dependencies = [ "comfy-table", "console 0.15.11", "dialoguer", - "dirs", "extism", "flate2", "fs2", @@ -1550,6 +1549,7 @@ dependencies = [ "futures-util", "hex", "hm-plugin-protocol", + "hm-util", "ignore", "indicatif", "nix", @@ -1715,7 +1715,6 @@ dependencies = [ name = "hm-util" version = "0.0.0-dev" dependencies = [ - "anyhow", "dirs", "tempfile", "tokio", diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs index 34ac816c..0a674d49 100644 --- a/crates/hm-util/src/dirs.rs +++ b/crates/hm-util/src/dirs.rs @@ -1,28 +1,71 @@ -//! Harmont config directory discovery. +//! Directory resolution for Harmont. //! -//! Searches for the harmont config directory by checking, in order: -//! 1. `~/.hm` -//! 2. `/etc/hm` (or `C:\ProgramData\hm` on Windows) +//! Provides both platform-level primitives (`home_dir`, `config_dir`, +//! `sys_config_dir`) and Harmont-specific config directory discovery +//! (`harmont_config_dir`). //! -//! The first directory that exists on disk wins. The directory does not -//! need to be well-formed to be selected — existence is sufficient. -//! -//! Windows uses `C:\ProgramData`. macOS uses `~/.hm` rather than -//! `~/Library/Application Support` because that confuses everyone. +//! This is the **only public directory API** in `hm-util`. The +//! low-level `os::dirs` module is `pub(crate)` and must not be used +//! outside this crate — consumers should use this module instead. use std::io; use std::path::PathBuf; +// --------------------------------------------------------------------------- +// Platform primitives +// --------------------------------------------------------------------------- + +/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). +#[must_use] +pub fn home_dir() -> Option { + dirs::home_dir() +} + +/// Platform user config directory (`~/.config` on Linux, +/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). +/// +/// Respects `$XDG_CONFIG_HOME` on Linux. +#[must_use] +pub fn config_dir() -> Option { + dirs::config_dir() +} + +/// System-wide config directory (`/etc` on Unix, `C:\ProgramData` on +/// Windows). +#[must_use] +pub fn sys_config_dir() -> PathBuf { + #[cfg(unix)] + { + PathBuf::from("/etc") + } + + #[cfg(windows)] + { + std::env::var_os("ProgramData") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("C:\\ProgramData")) + } +} + +// --------------------------------------------------------------------------- +// Harmont-specific discovery +// --------------------------------------------------------------------------- + /// Find the best harmont config directory. /// -/// Returns the first existing directory from the search order: -/// `~/.hm`, then the system config dir (`/etc/hm`). +/// Searches for the first existing directory in order: +/// 1. `~/.hm` +/// 2. `/etc/hm` (or `C:\ProgramData\hm` on Windows) +/// +/// The directory does not need to be well-formed — existence is +/// sufficient. macOS uses `~/.hm` rather than +/// `~/Library/Application Support` because that confuses everyone. /// /// Returns `None` if no candidate directory exists. -pub async fn config_dir() -> Option { +pub async fn harmont_config_dir() -> Option { let candidates = [ - crate::os::dirs::home_dir().map(|h| h.join(".hm")), - Some(crate::os::dirs::sys_config_dir().join("hm")), + home_dir().map(|h| h.join(".hm")), + Some(sys_config_dir().join("hm")), ]; for candidate in candidates.into_iter().flatten() { @@ -35,15 +78,15 @@ pub async fn config_dir() -> Option { /// Find the best harmont config directory, or return an error. /// -/// Same search as [`config_dir`], but returns an [`io::Error`] if no -/// candidate directory is found. +/// Same search as [`harmont_config_dir`], but returns an [`io::Error`] +/// if no candidate directory is found. /// /// # Errors /// /// Returns [`io::ErrorKind::NotFound`] if neither `~/.hm` nor the /// system config directory exists. -pub async fn config_dir_required() -> io::Result { - config_dir().await.ok_or_else(|| { +pub async fn harmont_config_dir_required() -> io::Result { + harmont_config_dir().await.ok_or_else(|| { io::Error::new( io::ErrorKind::NotFound, "no harmont config directory found (searched ~/.hm, /etc/hm)", @@ -56,14 +99,36 @@ pub async fn config_dir_required() -> io::Result { mod tests { use super::*; + #[test] + fn home_dir_resolves() { + let p = home_dir().unwrap(); + assert!(p.exists(), "home dir should exist: {}", p.display()); + } + + #[test] + fn config_dir_resolves() { + let p = config_dir().unwrap(); + assert!( + p.to_string_lossy().len() > 1, + "config dir should be a real path" + ); + } + + #[test] + fn sys_config_dir_is_etc() { + let p = sys_config_dir(); + #[cfg(unix)] + assert_eq!(p, PathBuf::from("/etc")); + } + #[tokio::test] - async fn config_dir_does_not_panic() { - let _ = config_dir().await; + async fn harmont_config_dir_does_not_panic() { + let _ = harmont_config_dir().await; } #[tokio::test] - async fn config_dir_required_gives_not_found_when_missing() { - let result = config_dir_required().await; + async fn harmont_config_dir_required_gives_not_found_when_missing() { + let result = harmont_config_dir_required().await; if let Err(e) = result { assert_eq!(e.kind(), io::ErrorKind::NotFound); } diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs deleted file mode 100644 index 1b664de5..00000000 --- a/crates/hm-util/src/os/dirs.rs +++ /dev/null @@ -1,67 +0,0 @@ -//! Platform directory resolution. -//! -//! Thin wrappers around the [`dirs`] crate that provide consistent -//! return types. Application-specific paths (e.g. `~/.harmont/`) -//! belong in the consuming crate, not here. - -use std::path::PathBuf; - -/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). -#[must_use] -pub fn home_dir() -> Option { - dirs::home_dir() -} - -/// Platform user config directory (`~/.config` on Linux, -/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). -/// -/// Respects `$XDG_CONFIG_HOME` on Linux. -#[must_use] -pub fn config_dir() -> Option { - dirs::config_dir() -} - -/// System-wide config directory (`/etc` on Unix, `C:\ProgramData` on -/// Windows). -#[must_use] -pub fn sys_config_dir() -> PathBuf { - #[cfg(unix)] - { - PathBuf::from("/etc") - } - - #[cfg(windows)] - { - std::env::var_os("ProgramData") - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from("C:\\ProgramData")) - } -} - -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - - #[test] - fn home_dir_resolves() { - let p = home_dir().unwrap(); - assert!(p.exists(), "home dir should exist: {}", p.display()); - } - - #[test] - fn config_dir_resolves() { - let p = config_dir().unwrap(); - assert!( - p.to_string_lossy().len() > 1, - "config dir should be a real path" - ); - } - - #[test] - fn sys_config_dir_is_etc() { - let p = sys_config_dir(); - #[cfg(unix)] - assert_eq!(p, PathBuf::from("/etc")); - } -} diff --git a/crates/hm-util/src/os/mod.rs b/crates/hm-util/src/os/mod.rs index ac4b93c8..d521fbd7 100644 --- a/crates/hm-util/src/os/mod.rs +++ b/crates/hm-util/src/os/mod.rs @@ -1,2 +1 @@ -pub mod dirs; pub mod fs; diff --git a/crates/hm/src/config.rs b/crates/hm/src/config.rs index d61a7baf..220fdf72 100644 --- a/crates/hm/src/config.rs +++ b/crates/hm/src/config.rs @@ -12,7 +12,7 @@ const DEFAULT_API_URL: &str = "https://api.harmont.dev"; /// (the `dirs` crate's platform-specific lookup fails — typically only /// happens in restrictive sandboxes with no `HOME` / passwd entry). pub fn user_config_dir() -> Result { - let home = hm_util::os::dirs::home_dir() + let home = hm_util::dirs::home_dir() .context("could not determine home directory")?; Ok(home.join(".harmont")) } diff --git a/crates/hm/src/lib.rs b/crates/hm/src/lib.rs index 1276eb69..9ae6fcaa 100644 --- a/crates/hm/src/lib.rs +++ b/crates/hm/src/lib.rs @@ -2,6 +2,11 @@ clippy::multiple_crate_versions, reason = "transitive dependency version conflicts in rand/windows-sys/thiserror chains; not fixable without upstream updates" )] +// The `dirs` crate must NOT be added as a direct dependency of this +// crate. All directory resolution goes through `hm_util::dirs`, which +// owns the `dirs` dependency and provides both platform primitives and +// Harmont-specific discovery. Adding `dirs` here would bypass that +// single source of truth. #[allow( clippy::print_stdout, diff --git a/crates/hm/src/plugin/host_fns.rs b/crates/hm/src/plugin/host_fns.rs index 9d36e8ff..3ac22368 100644 --- a/crates/hm/src/plugin/host_fns.rs +++ b/crates/hm/src/plugin/host_fns.rs @@ -652,7 +652,7 @@ pub fn kv_set_impl(scope: KvScope, key: &str, val: Vec) { // is not a practical concern. fn plugin_state_path() -> Option { - let dir = hm_util::os::dirs::config_dir()?.join("harmont").join("state"); + let dir = hm_util::dirs::config_dir()?.join("harmont").join("state"); let plugin = current_plugin_name()?; Some(dir.join(format!("{plugin}.kv"))) } @@ -991,7 +991,7 @@ mod plugin_kv_tests { use super::*; // Both tests mutate the process-wide `XDG_CONFIG_HOME` env var, - // which `dirs::config_dir()` reads. Serialize them so parallel + // which `hm_util::dirs::config_dir()` reads. Serialize them so parallel // test threads don't race on that global. static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); diff --git a/crates/hm/src/plugin/paths.rs b/crates/hm/src/plugin/paths.rs index 6c160363..1cc7aab7 100644 --- a/crates/hm/src/plugin/paths.rs +++ b/crates/hm/src/plugin/paths.rs @@ -13,7 +13,7 @@ use std::path::PathBuf; /// `~/.config/harmont/plugins/` (or the platform's XDG equivalent). /// User-global plugins live here. pub fn user_plugins_dir() -> Option { - hm_util::os::dirs::config_dir() + hm_util::dirs::config_dir() .map(|p| p.join("harmont").join("plugins")) } diff --git a/docs/plans/2026-05-23-extract-hm-util.md b/docs/plans/2026-05-23-extract-hm-util.md new file mode 100644 index 00000000..02262fec --- /dev/null +++ b/docs/plans/2026-05-23-extract-hm-util.md @@ -0,0 +1,957 @@ +# Extract `hm-util` Crate — OS & FS Utilities + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Extract low-level OS utilities from `crates/hm` into a shared `crates/hm-util` crate with async-first filesystem operations, then modernize the cancellation infrastructure. + +**Architecture:** New `hm-util` crate exposes `os::fs` (async + blocking atomic file I/O with permission control), `os::dirs` (platform directory resolution with proper error handling). Async functions use `spawn_blocking` over proven sync cores — same strategy as `tokio::fs`. The custom `CancellationToken` in `orchestrator/cancel.rs` is replaced by `tokio_util::sync::CancellationToken` (already used elsewhere in the codebase at `plugin/host_fns.rs:850`). + +**Tech Stack:** Rust 2024, tokio (spawn_blocking), anyhow, dirs, tokio-util (sync feature) + +**Key Design Decisions:** +- `os::fs` has two APIs: `pub async fn` primary + `pub mod blocking` for sync callers (host_fns extism callbacks are sync) +- `os::dirs` wraps the `dirs` crate with `anyhow::Result` instead of `Option` — standardizes error handling +- Application-specific paths (`~/.harmont/`, `~/.config/harmont/plugins/`) stay in `hm` — only generic OS primitives move to `hm-util` +- `creds_store` and `Config` stay sync using `blocking` API — their only callers from host_fns are sync extism callbacks +- Signal handler stays in `hm` (application-specific two-stage Ctrl-C with exit code 130) + +--- + +## Opportunity Inventory + +Before implementation, here's what was identified and the disposition: + +| Module | Location | Disposition | Rationale | +|--------|----------|-------------|-----------| +| `fs_util.rs` | `hm/src/fs_util.rs` | **Extract → `hm-util::os::fs`** | Pure OS utility, no domain logic, reusable | +| `user_config_dir()` | `hm/src/config.rs:14` | **Thin wrapper stays in `hm`; generic `home_dir`/`config_dir` → `hm-util::os::dirs`** | Product path (`~/.harmont/`) is app-specific; underlying dir resolution is generic | +| `user_plugins_dir()` etc. | `hm/src/plugin/paths.rs` | **Stay in `hm`; use `hm-util::os::dirs` underneath** | Product paths; thin wrappers over generic helpers | +| `CancellationToken` | `hm/src/orchestrator/cancel.rs` | **Replace with `tokio_util::sync::CancellationToken`** | 55-line Arc\ wrapper; tokio_util has same API + `.cancelled()` future + tree cancellation | +| `wait_cancel` polling loop | `hm/src/orchestrator/docker_host_fns.rs:163` | **Delete; replace with `token.cancelled().await`** | 50ms polling loop → zero-cost wakeup | +| `install_ctrlc` | `hm/src/plugin/signal.rs` | **Stay in `hm`** | Application-specific (two-stage Ctrl-C, exit 130, specific messages) | +| `EventBus` | `hm/src/orchestrator/events.rs` | **Stay in `hm`** | Domain-specific (BuildEvent broadcast) | +| `ArchiveStore` | `hm/src/orchestrator/archive.rs` | **Stay in `hm`** | Domain-specific (source archives for build runs) | +| `output/` module | `hm/src/output/` | **Stay in `hm`** | Tightly coupled to CLI output preferences | + +--- + +## Task 1: Create `hm-util` crate skeleton + +**Files:** +- Create: `crates/hm-util/Cargo.toml` +- Create: `crates/hm-util/src/lib.rs` +- Create: `crates/hm-util/src/os/mod.rs` +- Modify: `Cargo.toml` (workspace root) + +**Step 1: Create directory structure** + +```bash +mkdir -p crates/hm-util/src/os +``` + +**Step 2: Write `Cargo.toml`** + +Create `crates/hm-util/Cargo.toml`: + +```toml +[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" +tokio = { version = "1", features = ["rt"] } + +[dev-dependencies] +tempfile = "3" +tokio = { version = "1", features = ["full", "test-util"] } + +[lints] +workspace = true +``` + +**Step 3: Write `src/lib.rs`** + +```rust +pub mod os; +``` + +**Step 4: Write `src/os/mod.rs`** + +```rust +pub mod dirs; +pub mod fs; +``` + +**Step 5: Create placeholder files so it compiles** + +Create `crates/hm-util/src/os/dirs.rs`: +```rust +// Populated in Task 4. +``` + +Create `crates/hm-util/src/os/fs.rs`: +```rust +// Populated in Task 2. +``` + +**Step 6: Add to workspace** + +In root `Cargo.toml`, add `"crates/hm-util"` to `[workspace.members]` and `[workspace.default-members]`: + +```toml +members = [ + "crates/hm", + "crates/hm-plugin-protocol", + "crates/hm-plugin-sdk", + "crates/hm-plugin-docker", + "crates/hm-plugin-output-human", + "crates/hm-plugin-output-json", + "crates/hm-plugin-cloud", + "crates/hm-fixtures", + "crates/hm-util", +] +default-members = [ + "crates/hm", + "crates/hm-plugin-protocol", + "crates/hm-plugin-sdk", + "crates/hm-util", +] +``` + +Also add to `[workspace.dependencies]`: +```toml +hm-util = { path = "crates/hm-util", version = "0.0.0-dev" } +``` + +**Step 7: Verify compilation** + +```bash +cargo check -p hm-util +``` + +Expected: success (empty modules compile fine). + +**Step 8: Commit** + +```bash +git add crates/hm-util/ Cargo.toml +git commit -m "feat: add hm-util crate skeleton with os module structure" +``` + +--- + +## Task 2: Implement `os::fs` — blocking core + async wrappers + +**Files:** +- Create: `crates/hm-util/src/os/fs.rs` + +The sync implementation is the proven code from `hm/src/fs_util.rs`. The async API wraps it in `spawn_blocking`. + +**Step 1: Write tests for blocking API** + +Write tests first in `crates/hm-util/src/os/fs.rs` — these mirror the existing tests from `hm/src/fs_util.rs:131-192`: + +```rust +#[cfg(all(test, unix))] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::blocking; + use std::os::unix::fs::PermissionsExt; + + #[test] + fn writes_file_and_dir_with_requested_modes() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("creds"); + blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + let dir_mode = std::fs::metadata(target.parent().unwrap()) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(file_mode, 0o600, "file mode must be 0o600, got {file_mode:o}"); + assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); + } + + #[test] + fn overwrites_existing_file_preserving_mode() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("creds"); + blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); + blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"v2"); + let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[test] + fn tightens_existing_dir_with_looser_mode() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("loose"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let target = dir.join("creds"); + blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); + + let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700); + } + + #[test] + fn remove_if_exists_is_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + blocking::remove_if_exists(&target).unwrap(); + std::fs::write(&target, "x").unwrap(); + blocking::remove_if_exists(&target).unwrap(); + assert!(!target.exists()); + } + + #[tokio::test] + async fn async_write_atomic_restricted() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("async_creds"); + super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) + .await + .unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600); + } + + #[tokio::test] + async fn async_remove_if_exists() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + super::remove_if_exists(&target).await.unwrap(); + std::fs::write(&target, "x").unwrap(); + super::remove_if_exists(&target).await.unwrap(); + assert!(!target.exists()); + } +} +``` + +**Step 2: Run tests to verify they fail** + +```bash +cargo test -p hm-util +``` + +Expected: FAIL — `blocking` module and functions don't exist yet. + +**Step 3: Implement the full `os::fs` module** + +Write `crates/hm-util/src/os/fs.rs`: + +```rust +use std::path::Path; + +use anyhow::{Context, Result}; + +// --------------------------------------------------------------------------- +// Private sync core — shared by async wrappers and blocking module +// --------------------------------------------------------------------------- + +fn write_atomic_restricted_sync( + path: &Path, + contents: &[u8], + file_mode: u32, + dir_mode: u32, +) -> Result<()> { + let parent = path + .parent() + .with_context(|| format!("{} has no parent directory", path.display()))?; + + create_dir_with_mode_sync(parent, dir_mode) + .with_context(|| format!("creating {}", parent.display()))?; + + let file_name = path + .file_name() + .with_context(|| 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_sync(&tmp_path, contents, file_mode) + .with_context(|| format!("writing {}", tmp_path.display()))?; + + let persist_result = std::fs::rename(&tmp_path, path) + .with_context(|| format!("renaming {} -> {}", tmp_path.display(), path.display())); + + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result?; + + Ok(()) +} + +fn remove_if_exists_sync(path: &Path) -> Result<()> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e).with_context(|| format!("removing {}", path.display())), + } +} + +#[cfg(unix)] +fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> std::io::Result<()> { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + if dir.exists() { + let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; + if current != mode { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; + } + } else { + std::fs::DirBuilder::new() + .recursive(true) + .mode(mode) + .create(dir)?; + } + Ok(()) +} + +#[cfg(not(unix))] +fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> std::io::Result<()> { + std::fs::create_dir_all(dir) +} + +#[cfg(unix)] +fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(mode) + .open(path)?; + f.write_all(contents)?; + f.sync_all()?; + Ok(()) +} + +#[cfg(not(unix))] +fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> std::io::Result<()> { + std::fs::write(path, contents) +} + +// --------------------------------------------------------------------------- +// Public async API +// --------------------------------------------------------------------------- + +/// Write `contents` to `path` atomically with `file_mode`, ensuring the +/// parent directory exists and is set to `dir_mode`. +/// +/// On Unix the target file is created with the requested mode before any +/// bytes are written, closing the TOCTOU window. Contents are written to +/// a sibling tempfile and renamed over `path`. +/// +/// Offloads to the blocking thread pool via `spawn_blocking`. +pub async fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, +) -> Result<()> { + let path = path.as_ref().to_owned(); + let contents = contents.as_ref().to_vec(); + tokio::task::spawn_blocking(move || { + write_atomic_restricted_sync(&path, &contents, file_mode, dir_mode) + }) + .await + .context("write_atomic_restricted task panicked")? +} + +/// Remove a file if it exists; silently return `Ok(())` if not found. +/// +/// Offloads to the blocking thread pool via `spawn_blocking`. +pub async fn remove_if_exists(path: impl AsRef) -> Result<()> { + let path = path.as_ref().to_owned(); + tokio::task::spawn_blocking(move || remove_if_exists_sync(&path)) + .await + .context("remove_if_exists task panicked")? +} + +// --------------------------------------------------------------------------- +// Blocking (synchronous) API +// --------------------------------------------------------------------------- + +/// Synchronous variants for use in contexts that cannot await +/// (e.g. extism host function callbacks). +pub mod blocking { + use std::path::Path; + + use anyhow::Result; + + /// Synchronous version of [`super::write_atomic_restricted`]. + pub fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, + ) -> Result<()> { + super::write_atomic_restricted_sync( + path.as_ref(), + contents.as_ref(), + file_mode, + dir_mode, + ) + } + + /// Synchronous version of [`super::remove_if_exists`]. + pub fn remove_if_exists(path: impl AsRef) -> Result<()> { + super::remove_if_exists_sync(path.as_ref()) + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(all(test, unix))] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::blocking; + use std::os::unix::fs::PermissionsExt; + + #[test] + fn writes_file_and_dir_with_requested_modes() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("creds"); + blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + let dir_mode = std::fs::metadata(target.parent().unwrap()) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(file_mode, 0o600, "file mode must be 0o600, got {file_mode:o}"); + assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); + } + + #[test] + fn overwrites_existing_file_preserving_mode() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("creds"); + blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); + blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"v2"); + let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[test] + fn tightens_existing_dir_with_looser_mode() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("loose"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let target = dir.join("creds"); + blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); + + let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700); + } + + #[test] + fn remove_if_exists_is_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + blocking::remove_if_exists(&target).unwrap(); + std::fs::write(&target, "x").unwrap(); + blocking::remove_if_exists(&target).unwrap(); + assert!(!target.exists()); + } + + #[tokio::test] + async fn async_write_atomic_restricted() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("async_creds"); + super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) + .await + .unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600); + } + + #[tokio::test] + async fn async_remove_if_exists() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + super::remove_if_exists(&target).await.unwrap(); + std::fs::write(&target, "x").unwrap(); + super::remove_if_exists(&target).await.unwrap(); + assert!(!target.exists()); + } +} +``` + +**Step 4: Run tests to verify they pass** + +```bash +cargo test -p hm-util +``` + +Expected: all 6 tests pass. + +**Step 5: Commit** + +```bash +git add crates/hm-util/src/os/fs.rs +git commit -m "feat(hm-util): implement os::fs with async + blocking atomic file I/O" +``` + +--- + +## Task 3: Implement `os::dirs` — platform directory resolution + +**Files:** +- Modify: `crates/hm-util/src/os/dirs.rs` + +**Step 1: Write tests** + +```rust +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[test] + fn home_dir_resolves() { + let p = home_dir().unwrap(); + assert!(p.exists(), "home dir should exist: {}", p.display()); + } + + #[test] + fn config_dir_resolves() { + let p = config_dir().unwrap(); + assert!( + p.to_string_lossy().len() > 1, + "config dir should be a real path" + ); + } +} +``` + +**Step 2: Run tests to verify they fail** + +```bash +cargo test -p hm-util -- dirs +``` + +Expected: FAIL — functions don't exist. + +**Step 3: Implement `os::dirs`** + +Write `crates/hm-util/src/os/dirs.rs`: + +```rust +use std::path::PathBuf; + +use anyhow::{Context, Result}; + +/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). +pub fn home_dir() -> Result { + dirs::home_dir().context("could not determine home directory") +} + +/// Platform config directory (`~/.config` on Linux, +/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). +pub fn config_dir() -> Result { + dirs::config_dir().context("could not determine config directory") +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + + #[test] + fn home_dir_resolves() { + let p = home_dir().unwrap(); + assert!(p.exists(), "home dir should exist: {}", p.display()); + } + + #[test] + fn config_dir_resolves() { + let p = config_dir().unwrap(); + assert!( + p.to_string_lossy().len() > 1, + "config dir should be a real path" + ); + } +} +``` + +**Step 4: Run tests** + +```bash +cargo test -p hm-util +``` + +Expected: all tests pass (6 fs + 2 dirs). + +**Step 5: Commit** + +```bash +git add crates/hm-util/src/os/dirs.rs +git commit -m "feat(hm-util): add os::dirs for platform directory resolution" +``` + +--- + +## Task 4: Migrate `hm` to use `hm-util::os::fs` + +**Files:** +- Modify: `crates/hm/Cargo.toml` — add `hm-util` dependency +- Modify: `crates/hm/src/config.rs:84` — use `hm_util::os::fs::blocking` +- Modify: `crates/hm/src/creds_store.rs:35` — use `hm_util::os::fs::blocking` +- Modify: `crates/hm/src/lib.rs:14` — remove `pub mod fs_util;` +- Delete: `crates/hm/src/fs_util.rs` + +**Step 1: Add `hm-util` dependency to `hm`** + +In `crates/hm/Cargo.toml`, add to `[dependencies]`: + +```toml +hm-util = { workspace = true } +``` + +**Step 2: Update `config.rs` — replace `crate::fs_util` with `hm_util::os::fs::blocking`** + +In `crates/hm/src/config.rs`, line 84, change: + +```rust +// Before: +crate::fs_util::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) +// After: +hm_util::os::fs::blocking::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) +``` + +**Step 3: Update `creds_store.rs` — same replacement** + +In `crates/hm/src/creds_store.rs`, line 35, change: + +```rust +// Before: +crate::fs_util::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) +// After: +hm_util::os::fs::blocking::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) +``` + +Also update the module doc comment at line 4: + +```rust +// Before: +//! mode 0o600 (parent dir 0o700) via [`crate::fs_util::write_atomic_restricted`]. +// After: +//! mode 0o600 (parent dir 0o700) via [`hm_util::os::fs::blocking::write_atomic_restricted`]. +``` + +**Step 4: Remove `fs_util` module from `lib.rs`** + +In `crates/hm/src/lib.rs`, remove line 14: + +```rust +pub mod fs_util; +``` + +**Step 5: Delete `fs_util.rs`** + +```bash +rm crates/hm/src/fs_util.rs +``` + +**Step 6: Update doc comment in `fs_util.rs` references** + +The doc header in the now-deleted file referenced `crate::creds_store` and `config::user_config_dir` — these lived in `fs_util.rs` which is now gone. No action needed since the file is deleted. + +**Step 7: Verify compilation and tests** + +```bash +cargo check -p harmont-cli && cargo test -p harmont-cli +``` + +Expected: all existing tests pass. The `fs_util::tests` that were in the deleted file are now covered by identical tests in `hm-util`. + +**Step 8: Commit** + +```bash +git add crates/hm/Cargo.toml crates/hm/src/config.rs crates/hm/src/creds_store.rs crates/hm/src/lib.rs +git rm crates/hm/src/fs_util.rs +git commit -m "refactor: migrate fs_util callers to hm-util::os::fs::blocking" +``` + +--- + +## Task 5: Migrate `hm` directory functions to use `hm-util::os::dirs` + +**Files:** +- Modify: `crates/hm/src/config.rs:14-16` — use `hm_util::os::dirs::home_dir` +- Modify: `crates/hm/src/plugin/paths.rs:16` — use `hm_util::os::dirs::config_dir` + +**Step 1: Update `config.rs::user_config_dir()`** + +In `crates/hm/src/config.rs`, change the `user_config_dir` function (lines 14-17): + +```rust +// Before: +pub fn user_config_dir() -> Result { + let home = dirs::home_dir().context("could not determine home directory")?; + Ok(home.join(".harmont")) +} + +// After: +pub fn user_config_dir() -> Result { + Ok(hm_util::os::dirs::home_dir()?.join(".harmont")) +} +``` + +**Step 2: Update `plugin/paths.rs::user_plugins_dir()`** + +In `crates/hm/src/plugin/paths.rs`, change line 16: + +```rust +// Before: +pub fn user_plugins_dir() -> Option { + dirs::config_dir().map(|p| p.join("harmont").join("plugins")) +} + +// After: +pub fn user_plugins_dir() -> Option { + hm_util::os::dirs::config_dir() + .ok() + .map(|p| p.join("harmont").join("plugins")) +} +``` + +**Step 3: Remove `dirs` direct dependency from `hm`** + +In `crates/hm/Cargo.toml`, remove: + +```toml +dirs = "6" +``` + +**Step 4: Verify no other `dirs::` usage in `hm`** + +```bash +grep -rn 'dirs::' crates/hm/src/ --include='*.rs' +``` + +Expected: no matches (all usage now goes through `hm_util::os::dirs`). + +**Step 5: Run tests** + +```bash +cargo test -p harmont-cli +``` + +Expected: all tests pass. + +**Step 6: Commit** + +```bash +git add crates/hm/Cargo.toml crates/hm/src/config.rs crates/hm/src/plugin/paths.rs +git commit -m "refactor: delegate directory resolution to hm-util::os::dirs" +``` + +--- + +## Task 6: Replace custom `CancellationToken` with `tokio_util::sync::CancellationToken` + +**Context:** The custom `CancellationToken` in `orchestrator/cancel.rs` is a 55-line `Arc` wrapper. `tokio_util::sync::CancellationToken` provides the same API (`new()`, `cancel()`, `is_cancelled()`) plus a zero-cost `.cancelled()` future — eliminating the 50ms polling loop in `docker_host_fns.rs:163-171`. + +Note: `tokio_util::sync::CancellationToken` is already used in `plugin/host_fns.rs:850` for the OAuth loopback server, so the dependency and feature flag are already available. + +**Files:** +- Delete: `crates/hm/src/orchestrator/cancel.rs` +- Modify: `crates/hm/src/orchestrator/mod.rs` — remove `pub mod cancel;` +- Modify: `crates/hm/src/orchestrator/state.rs:27` — update import +- Modify: `crates/hm/src/orchestrator/scheduler.rs:47,74` — update import + construction +- Modify: `crates/hm/src/orchestrator/docker_host_fns.rs:163-171` — replace polling loop +- Modify: `crates/hm/src/plugin/signal.rs:18` — update import +- Modify: `crates/hm/src/plugin/host_fns.rs:925` — update path + +**Step 1: Update `orchestrator/mod.rs` — remove cancel module** + +In `crates/hm/src/orchestrator/mod.rs`, remove line 11: + +```rust +pub mod cancel; +``` + +**Step 2: Update `orchestrator/state.rs` — change import** + +In `crates/hm/src/orchestrator/state.rs`, replace line 27: + +```rust +// Before: +use super::cancel::CancellationToken; +// After: +use tokio_util::sync::CancellationToken; +``` + +**Step 3: Update `orchestrator/scheduler.rs` — change import** + +In `crates/hm/src/orchestrator/scheduler.rs`, replace line 47: + +```rust +// Before: +use super::cancel::CancellationToken; +// After: +use tokio_util::sync::CancellationToken; +``` + +**Step 4: Update `plugin/signal.rs` — change import** + +In `crates/hm/src/plugin/signal.rs`, replace line 18: + +```rust +// Before: +use crate::orchestrator::cancel::CancellationToken; +// After: +use tokio_util::sync::CancellationToken; +``` + +**Step 5: Update `orchestrator/docker_host_fns.rs` — replace polling loop with `.cancelled()`** + +Replace lines 163-171: + +```rust +// Before: +async fn wait_cancel(cancel: &crate::orchestrator::cancel::CancellationToken) { + // Poll the atomic every 50ms. Cheap; never wakes a thread early. + loop { + if cancel.is_cancelled() { + return; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } +} + +// After: +async fn wait_cancel(cancel: &tokio_util::sync::CancellationToken) { + cancel.cancelled().await; +} +``` + +**Step 6: Update `plugin/host_fns.rs:925` — fix `is_cancelled` path** + +In `crates/hm/src/plugin/host_fns.rs`, line 925 references `s.cancel.is_cancelled()`. The `CancellationToken` field type changes but the method name is the same — verify this line still compiles (it should, as `tokio_util::sync::CancellationToken` also has `is_cancelled()`). + +**Step 7: Delete `orchestrator/cancel.rs`** + +```bash +rm crates/hm/src/orchestrator/cancel.rs +``` + +**Step 8: Verify compilation and tests** + +```bash +cargo check -p harmont-cli && cargo test -p harmont-cli +``` + +Expected: compiles and all tests pass. The three tests that were in `cancel.rs` (default_is_not_cancelled, cancel_persists, cancel_is_clone_shared) are trivially true for `tokio_util::sync::CancellationToken` — the upstream crate tests them. + +**Step 9: Commit** + +```bash +git rm crates/hm/src/orchestrator/cancel.rs +git add crates/hm/src/orchestrator/mod.rs crates/hm/src/orchestrator/state.rs crates/hm/src/orchestrator/scheduler.rs crates/hm/src/orchestrator/docker_host_fns.rs crates/hm/src/plugin/signal.rs crates/hm/src/plugin/host_fns.rs +git commit -m "refactor: replace custom CancellationToken with tokio_util::sync::CancellationToken + +Eliminates 55-line Arc wrapper. The 50ms polling loop in +wait_cancel is replaced by the zero-cost .cancelled() future." +``` + +--- + +## Task 7: Final verification and cleanup + +**Step 1: Full workspace build** + +```bash +cargo build --workspace +``` + +Expected: clean build, no warnings. + +**Step 2: Full workspace test suite** + +```bash +cargo test --workspace +``` + +Expected: all tests pass. + +**Step 3: Clippy** + +```bash +cargo clippy --workspace -- -D warnings +``` + +Expected: no warnings. + +**Step 4: Verify module structure matches intent** + +```bash +find crates/hm-util/src -name '*.rs' | sort +``` + +Expected: +``` +crates/hm-util/src/lib.rs +crates/hm-util/src/os/dirs.rs +crates/hm-util/src/os/fs.rs +crates/hm-util/src/os/mod.rs +``` + +**Step 5: Verify deleted files are gone** + +```bash +test ! -f crates/hm/src/fs_util.rs && echo "fs_util.rs removed" +test ! -f crates/hm/src/orchestrator/cancel.rs && echo "cancel.rs removed" +``` + +**Step 6: Final commit if any cleanup was needed** + +```bash +git status +# If clean: done. If changes: commit cleanup. +``` + +--- + +## Future Opportunities (Not In Scope) + +These were identified during analysis but deferred: + +1. **Async propagation in `Config` and `RunContext`** — `Config::load()` and `Config::save()` could become async, using `hm_util::os::fs::write_atomic_restricted` (async variant) directly. `RunContext::from_cli()` would become `async fn from_cli()`. Benefit: avoids blocking tokio worker thread during config I/O. Cost: minor — `from_cli` is only called from `async fn run()` in `main.rs`. Deferred because config files are tiny and the perf impact is negligible. + +2. **`creds_store` async variant** — Blocked by extism host_fn callbacks being sync. Would require `block_in_place` bridge in host_fns. No benefit until extism supports async host functions. + +3. **Signal handler extraction** — `plugin/signal.rs::install_ctrlc` is application-specific (two-stage Ctrl-C, exit code 130, stderr messages). Not a reusable utility. Could move from `plugin/` to a top-level `signal.rs` module if the `plugin/` location feels wrong, but extraction to `hm-util` is over-engineering. + +4. **`output/format.rs` time utilities** — `rel_time()`, `duration_human()`, `elapsed_between()` are generic but small (< 30 lines total). Not worth extracting until a second consumer exists. + +5. **`os::process` module** — Future home for process-related utilities if patterns emerge (e.g., a generic `spawn_and_stream` helper for the Docker client). From 2dedf6349f1c862d56f972d16f703ca4c9769280 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 11:42:56 -0700 Subject: [PATCH 12/25] refactor: domain-specific dir accessors, hide raw platform calls Move raw `dirs::` calls into private `os::dirs` module within hm-util. Public `hm_util::dirs` now exposes only Harmont-specific accessors: harmont_config_dir, harmont_data_dir, harmont_plugins_dir, harmont_plugin_state_dir. No raw system directory calls leak into hm. --- crates/hm-util/src/dirs.rs | 140 ++++++++----------------------- crates/hm-util/src/os/dirs.rs | 16 ++++ crates/hm-util/src/os/mod.rs | 1 + crates/hm/src/config.rs | 5 +- crates/hm/src/plugin/host_fns.rs | 6 +- crates/hm/src/plugin/paths.rs | 3 +- 6 files changed, 56 insertions(+), 115 deletions(-) create mode 100644 crates/hm-util/src/os/dirs.rs diff --git a/crates/hm-util/src/dirs.rs b/crates/hm-util/src/dirs.rs index 0a674d49..4838e3f0 100644 --- a/crates/hm-util/src/dirs.rs +++ b/crates/hm-util/src/dirs.rs @@ -1,97 +1,34 @@ -//! Directory resolution for Harmont. +//! Harmont-specific directory resolution. //! -//! Provides both platform-level primitives (`home_dir`, `config_dir`, -//! `sys_config_dir`) and Harmont-specific config directory discovery -//! (`harmont_config_dir`). -//! -//! This is the **only public directory API** in `hm-util`. The -//! low-level `os::dirs` module is `pub(crate)` and must not be used -//! outside this crate — consumers should use this module instead. +//! Every directory accessor in this module returns a Harmont-namespaced +//! path. Raw platform primitives (`home_dir`, `config_dir`) live in +//! `os::dirs` and are **not** re-exported — callers outside `hm-util` +//! should never need them. + +#![allow(clippy::must_use_candidate)] -use std::io; use std::path::PathBuf; -// --------------------------------------------------------------------------- -// Platform primitives -// --------------------------------------------------------------------------- +use crate::os::dirs as platform; -/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). -#[must_use] -pub fn home_dir() -> Option { - dirs::home_dir() +/// `~/.harmont/` — CLI config home (config.toml, credentials.toml). +pub fn harmont_config_dir() -> Option { + platform::home_dir().map(|h| h.join(".harmont")) } -/// Platform user config directory (`~/.config` on Linux, -/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). -/// -/// Respects `$XDG_CONFIG_HOME` on Linux. -#[must_use] -pub fn config_dir() -> Option { - dirs::config_dir() +/// `/harmont/` — XDG-aware data root (plugins, state). +pub fn harmont_data_dir() -> Option { + platform::config_dir().map(|c| c.join("harmont")) } -/// System-wide config directory (`/etc` on Unix, `C:\ProgramData` on -/// Windows). -#[must_use] -pub fn sys_config_dir() -> PathBuf { - #[cfg(unix)] - { - PathBuf::from("/etc") - } - - #[cfg(windows)] - { - std::env::var_os("ProgramData") - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from("C:\\ProgramData")) - } +/// `/harmont/plugins/` — user-global plugin directory. +pub fn harmont_plugins_dir() -> Option { + harmont_data_dir().map(|d| d.join("plugins")) } -// --------------------------------------------------------------------------- -// Harmont-specific discovery -// --------------------------------------------------------------------------- - -/// Find the best harmont config directory. -/// -/// Searches for the first existing directory in order: -/// 1. `~/.hm` -/// 2. `/etc/hm` (or `C:\ProgramData\hm` on Windows) -/// -/// The directory does not need to be well-formed — existence is -/// sufficient. macOS uses `~/.hm` rather than -/// `~/Library/Application Support` because that confuses everyone. -/// -/// Returns `None` if no candidate directory exists. -pub async fn harmont_config_dir() -> Option { - let candidates = [ - home_dir().map(|h| h.join(".hm")), - Some(sys_config_dir().join("hm")), - ]; - - for candidate in candidates.into_iter().flatten() { - if tokio::fs::try_exists(&candidate).await.unwrap_or(false) { - return Some(candidate); - } - } - None -} - -/// Find the best harmont config directory, or return an error. -/// -/// Same search as [`harmont_config_dir`], but returns an [`io::Error`] -/// if no candidate directory is found. -/// -/// # Errors -/// -/// Returns [`io::ErrorKind::NotFound`] if neither `~/.hm` nor the -/// system config directory exists. -pub async fn harmont_config_dir_required() -> io::Result { - harmont_config_dir().await.ok_or_else(|| { - io::Error::new( - io::ErrorKind::NotFound, - "no harmont config directory found (searched ~/.hm, /etc/hm)", - ) - }) +/// `/harmont/state/` — per-plugin persistent KV state. +pub fn harmont_plugin_state_dir() -> Option { + harmont_data_dir().map(|d| d.join("state")) } #[cfg(test)] @@ -100,37 +37,26 @@ mod tests { use super::*; #[test] - fn home_dir_resolves() { - let p = home_dir().unwrap(); - assert!(p.exists(), "home dir should exist: {}", p.display()); + fn harmont_config_dir_under_home() { + let p = harmont_config_dir().unwrap(); + assert!(p.ends_with(".harmont")); } #[test] - fn config_dir_resolves() { - let p = config_dir().unwrap(); - assert!( - p.to_string_lossy().len() > 1, - "config dir should be a real path" - ); + fn harmont_data_dir_under_config() { + let p = harmont_data_dir().unwrap(); + assert!(p.ends_with("harmont")); } #[test] - fn sys_config_dir_is_etc() { - let p = sys_config_dir(); - #[cfg(unix)] - assert_eq!(p, PathBuf::from("/etc")); + fn harmont_plugins_dir_resolves() { + let p = harmont_plugins_dir().unwrap(); + assert!(p.ends_with("harmont/plugins")); } - #[tokio::test] - async fn harmont_config_dir_does_not_panic() { - let _ = harmont_config_dir().await; - } - - #[tokio::test] - async fn harmont_config_dir_required_gives_not_found_when_missing() { - let result = harmont_config_dir_required().await; - if let Err(e) = result { - assert_eq!(e.kind(), io::ErrorKind::NotFound); - } + #[test] + fn harmont_plugin_state_dir_resolves() { + let p = harmont_plugin_state_dir().unwrap(); + assert!(p.ends_with("harmont/state")); } } diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs new file mode 100644 index 00000000..76d22eff --- /dev/null +++ b/crates/hm-util/src/os/dirs.rs @@ -0,0 +1,16 @@ +//! Raw platform directory primitives. +//! +//! This module is `pub(crate)` — external callers must use +//! [`crate::dirs`] which provides Harmont-specific accessors. + +#![allow(unreachable_pub)] + +use std::path::PathBuf; + +pub fn home_dir() -> Option { + dirs::home_dir() +} + +pub fn config_dir() -> Option { + dirs::config_dir() +} diff --git a/crates/hm-util/src/os/mod.rs b/crates/hm-util/src/os/mod.rs index d521fbd7..0bc7829b 100644 --- a/crates/hm-util/src/os/mod.rs +++ b/crates/hm-util/src/os/mod.rs @@ -1 +1,2 @@ +pub(crate) mod dirs; pub mod fs; diff --git a/crates/hm/src/config.rs b/crates/hm/src/config.rs index 220fdf72..5a83422b 100644 --- a/crates/hm/src/config.rs +++ b/crates/hm/src/config.rs @@ -12,9 +12,8 @@ const DEFAULT_API_URL: &str = "https://api.harmont.dev"; /// (the `dirs` crate's platform-specific lookup fails — typically only /// happens in restrictive sandboxes with no `HOME` / passwd entry). pub fn user_config_dir() -> Result { - let home = hm_util::dirs::home_dir() - .context("could not determine home directory")?; - Ok(home.join(".harmont")) + hm_util::dirs::harmont_config_dir() + .context("could not determine home directory") } /// User preferences stored alongside the config. diff --git a/crates/hm/src/plugin/host_fns.rs b/crates/hm/src/plugin/host_fns.rs index 3ac22368..d7c1f7e9 100644 --- a/crates/hm/src/plugin/host_fns.rs +++ b/crates/hm/src/plugin/host_fns.rs @@ -652,7 +652,7 @@ pub fn kv_set_impl(scope: KvScope, key: &str, val: Vec) { // is not a practical concern. fn plugin_state_path() -> Option { - let dir = hm_util::dirs::config_dir()?.join("harmont").join("state"); + let dir = hm_util::dirs::harmont_plugin_state_dir()?; let plugin = current_plugin_name()?; Some(dir.join(format!("{plugin}.kv"))) } @@ -991,8 +991,8 @@ mod plugin_kv_tests { use super::*; // Both tests mutate the process-wide `XDG_CONFIG_HOME` env var, - // which `hm_util::dirs::config_dir()` reads. Serialize them so parallel - // test threads don't race on that global. + // which the platform config_dir lookup reads. Serialize them so + // parallel test threads don't race on that global. static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); #[test] diff --git a/crates/hm/src/plugin/paths.rs b/crates/hm/src/plugin/paths.rs index 1cc7aab7..b89895c6 100644 --- a/crates/hm/src/plugin/paths.rs +++ b/crates/hm/src/plugin/paths.rs @@ -13,8 +13,7 @@ use std::path::PathBuf; /// `~/.config/harmont/plugins/` (or the platform's XDG equivalent). /// User-global plugins live here. pub fn user_plugins_dir() -> Option { - hm_util::dirs::config_dir() - .map(|p| p.join("harmont").join("plugins")) + hm_util::dirs::harmont_plugins_dir() } /// `/.harmont/plugins/`. Project-local plugins live here. From ccbf7c8c014eadc420847799cbb324b21c541e2e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 11:47:25 -0700 Subject: [PATCH 13/25] refactor: suppress redundant_pub_crate at workspace level unreachable_pub (rustc) and redundant_pub_crate (clippy) contradict each other for pub(crate) items inside non-pub modules. Keep the rustc lint, suppress the clippy one globally. --- Cargo.toml | 3 +++ crates/hm-util/src/os/dirs.rs | 6 ++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 99f628df..5da02aa2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,6 +57,9 @@ cargo_common_metadata = "allow" # schemars 0.8 pulls older indexmap / wit-bindgen transitively; we # can't fix without bumping schemars. multiple_crate_versions = "allow" +# `pub(crate)` inside a non-pub module triggers this, but `unreachable_pub` +# already enforces the same invariant from the rustc side. +redundant_pub_crate = "allow" dbg_macro = "deny" todo = "deny" unimplemented = "deny" diff --git a/crates/hm-util/src/os/dirs.rs b/crates/hm-util/src/os/dirs.rs index 76d22eff..2d82f295 100644 --- a/crates/hm-util/src/os/dirs.rs +++ b/crates/hm-util/src/os/dirs.rs @@ -3,14 +3,12 @@ //! This module is `pub(crate)` — external callers must use //! [`crate::dirs`] which provides Harmont-specific accessors. -#![allow(unreachable_pub)] - use std::path::PathBuf; -pub fn home_dir() -> Option { +pub(crate) fn home_dir() -> Option { dirs::home_dir() } -pub fn config_dir() -> Option { +pub(crate) fn config_dir() -> Option { dirs::config_dir() } From 6cc77e9952f53e3e6413fefb5d51aaad48cf04cc Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 12:37:05 -0700 Subject: [PATCH 14/25] deslop --- docs/plans/2026-05-23-extract-hm-util.md | 957 ----------------------- 1 file changed, 957 deletions(-) delete mode 100644 docs/plans/2026-05-23-extract-hm-util.md diff --git a/docs/plans/2026-05-23-extract-hm-util.md b/docs/plans/2026-05-23-extract-hm-util.md deleted file mode 100644 index 02262fec..00000000 --- a/docs/plans/2026-05-23-extract-hm-util.md +++ /dev/null @@ -1,957 +0,0 @@ -# Extract `hm-util` Crate — OS & FS Utilities - -> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. - -**Goal:** Extract low-level OS utilities from `crates/hm` into a shared `crates/hm-util` crate with async-first filesystem operations, then modernize the cancellation infrastructure. - -**Architecture:** New `hm-util` crate exposes `os::fs` (async + blocking atomic file I/O with permission control), `os::dirs` (platform directory resolution with proper error handling). Async functions use `spawn_blocking` over proven sync cores — same strategy as `tokio::fs`. The custom `CancellationToken` in `orchestrator/cancel.rs` is replaced by `tokio_util::sync::CancellationToken` (already used elsewhere in the codebase at `plugin/host_fns.rs:850`). - -**Tech Stack:** Rust 2024, tokio (spawn_blocking), anyhow, dirs, tokio-util (sync feature) - -**Key Design Decisions:** -- `os::fs` has two APIs: `pub async fn` primary + `pub mod blocking` for sync callers (host_fns extism callbacks are sync) -- `os::dirs` wraps the `dirs` crate with `anyhow::Result` instead of `Option` — standardizes error handling -- Application-specific paths (`~/.harmont/`, `~/.config/harmont/plugins/`) stay in `hm` — only generic OS primitives move to `hm-util` -- `creds_store` and `Config` stay sync using `blocking` API — their only callers from host_fns are sync extism callbacks -- Signal handler stays in `hm` (application-specific two-stage Ctrl-C with exit code 130) - ---- - -## Opportunity Inventory - -Before implementation, here's what was identified and the disposition: - -| Module | Location | Disposition | Rationale | -|--------|----------|-------------|-----------| -| `fs_util.rs` | `hm/src/fs_util.rs` | **Extract → `hm-util::os::fs`** | Pure OS utility, no domain logic, reusable | -| `user_config_dir()` | `hm/src/config.rs:14` | **Thin wrapper stays in `hm`; generic `home_dir`/`config_dir` → `hm-util::os::dirs`** | Product path (`~/.harmont/`) is app-specific; underlying dir resolution is generic | -| `user_plugins_dir()` etc. | `hm/src/plugin/paths.rs` | **Stay in `hm`; use `hm-util::os::dirs` underneath** | Product paths; thin wrappers over generic helpers | -| `CancellationToken` | `hm/src/orchestrator/cancel.rs` | **Replace with `tokio_util::sync::CancellationToken`** | 55-line Arc\ wrapper; tokio_util has same API + `.cancelled()` future + tree cancellation | -| `wait_cancel` polling loop | `hm/src/orchestrator/docker_host_fns.rs:163` | **Delete; replace with `token.cancelled().await`** | 50ms polling loop → zero-cost wakeup | -| `install_ctrlc` | `hm/src/plugin/signal.rs` | **Stay in `hm`** | Application-specific (two-stage Ctrl-C, exit 130, specific messages) | -| `EventBus` | `hm/src/orchestrator/events.rs` | **Stay in `hm`** | Domain-specific (BuildEvent broadcast) | -| `ArchiveStore` | `hm/src/orchestrator/archive.rs` | **Stay in `hm`** | Domain-specific (source archives for build runs) | -| `output/` module | `hm/src/output/` | **Stay in `hm`** | Tightly coupled to CLI output preferences | - ---- - -## Task 1: Create `hm-util` crate skeleton - -**Files:** -- Create: `crates/hm-util/Cargo.toml` -- Create: `crates/hm-util/src/lib.rs` -- Create: `crates/hm-util/src/os/mod.rs` -- Modify: `Cargo.toml` (workspace root) - -**Step 1: Create directory structure** - -```bash -mkdir -p crates/hm-util/src/os -``` - -**Step 2: Write `Cargo.toml`** - -Create `crates/hm-util/Cargo.toml`: - -```toml -[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" -tokio = { version = "1", features = ["rt"] } - -[dev-dependencies] -tempfile = "3" -tokio = { version = "1", features = ["full", "test-util"] } - -[lints] -workspace = true -``` - -**Step 3: Write `src/lib.rs`** - -```rust -pub mod os; -``` - -**Step 4: Write `src/os/mod.rs`** - -```rust -pub mod dirs; -pub mod fs; -``` - -**Step 5: Create placeholder files so it compiles** - -Create `crates/hm-util/src/os/dirs.rs`: -```rust -// Populated in Task 4. -``` - -Create `crates/hm-util/src/os/fs.rs`: -```rust -// Populated in Task 2. -``` - -**Step 6: Add to workspace** - -In root `Cargo.toml`, add `"crates/hm-util"` to `[workspace.members]` and `[workspace.default-members]`: - -```toml -members = [ - "crates/hm", - "crates/hm-plugin-protocol", - "crates/hm-plugin-sdk", - "crates/hm-plugin-docker", - "crates/hm-plugin-output-human", - "crates/hm-plugin-output-json", - "crates/hm-plugin-cloud", - "crates/hm-fixtures", - "crates/hm-util", -] -default-members = [ - "crates/hm", - "crates/hm-plugin-protocol", - "crates/hm-plugin-sdk", - "crates/hm-util", -] -``` - -Also add to `[workspace.dependencies]`: -```toml -hm-util = { path = "crates/hm-util", version = "0.0.0-dev" } -``` - -**Step 7: Verify compilation** - -```bash -cargo check -p hm-util -``` - -Expected: success (empty modules compile fine). - -**Step 8: Commit** - -```bash -git add crates/hm-util/ Cargo.toml -git commit -m "feat: add hm-util crate skeleton with os module structure" -``` - ---- - -## Task 2: Implement `os::fs` — blocking core + async wrappers - -**Files:** -- Create: `crates/hm-util/src/os/fs.rs` - -The sync implementation is the proven code from `hm/src/fs_util.rs`. The async API wraps it in `spawn_blocking`. - -**Step 1: Write tests for blocking API** - -Write tests first in `crates/hm-util/src/os/fs.rs` — these mirror the existing tests from `hm/src/fs_util.rs:131-192`: - -```rust -#[cfg(all(test, unix))] -#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] -mod tests { - use super::blocking; - use std::os::unix::fs::PermissionsExt; - - #[test] - fn writes_file_and_dir_with_requested_modes() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("creds"); - blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - let dir_mode = std::fs::metadata(target.parent().unwrap()) - .unwrap() - .permissions() - .mode() - & 0o777; - assert_eq!(file_mode, 0o600, "file mode must be 0o600, got {file_mode:o}"); - assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); - } - - #[test] - fn overwrites_existing_file_preserving_mode() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("creds"); - blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); - blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"v2"); - let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600); - } - - #[test] - fn tightens_existing_dir_with_looser_mode() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("loose"); - std::fs::create_dir(&dir).unwrap(); - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); - - let target = dir.join("creds"); - blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); - - let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dir_mode, 0o700); - } - - #[test] - fn remove_if_exists_is_idempotent() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - blocking::remove_if_exists(&target).unwrap(); - std::fs::write(&target, "x").unwrap(); - blocking::remove_if_exists(&target).unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn async_write_atomic_restricted() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("async_creds"); - super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) - .await - .unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(file_mode, 0o600); - } - - #[tokio::test] - async fn async_remove_if_exists() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - super::remove_if_exists(&target).await.unwrap(); - std::fs::write(&target, "x").unwrap(); - super::remove_if_exists(&target).await.unwrap(); - assert!(!target.exists()); - } -} -``` - -**Step 2: Run tests to verify they fail** - -```bash -cargo test -p hm-util -``` - -Expected: FAIL — `blocking` module and functions don't exist yet. - -**Step 3: Implement the full `os::fs` module** - -Write `crates/hm-util/src/os/fs.rs`: - -```rust -use std::path::Path; - -use anyhow::{Context, Result}; - -// --------------------------------------------------------------------------- -// Private sync core — shared by async wrappers and blocking module -// --------------------------------------------------------------------------- - -fn write_atomic_restricted_sync( - path: &Path, - contents: &[u8], - file_mode: u32, - dir_mode: u32, -) -> Result<()> { - let parent = path - .parent() - .with_context(|| format!("{} has no parent directory", path.display()))?; - - create_dir_with_mode_sync(parent, dir_mode) - .with_context(|| format!("creating {}", parent.display()))?; - - let file_name = path - .file_name() - .with_context(|| 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_sync(&tmp_path, contents, file_mode) - .with_context(|| format!("writing {}", tmp_path.display()))?; - - let persist_result = std::fs::rename(&tmp_path, path) - .with_context(|| format!("renaming {} -> {}", tmp_path.display(), path.display())); - - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); - } - persist_result?; - - Ok(()) -} - -fn remove_if_exists_sync(path: &Path) -> Result<()> { - match std::fs::remove_file(path) { - Ok(()) => Ok(()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e).with_context(|| format!("removing {}", path.display())), - } -} - -#[cfg(unix)] -fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> std::io::Result<()> { - use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; - if dir.exists() { - let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; - if current != mode { - std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; - } - } else { - std::fs::DirBuilder::new() - .recursive(true) - .mode(mode) - .create(dir)?; - } - Ok(()) -} - -#[cfg(not(unix))] -fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> std::io::Result<()> { - std::fs::create_dir_all(dir) -} - -#[cfg(unix)] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> std::io::Result<()> { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut f = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(mode) - .open(path)?; - f.write_all(contents)?; - f.sync_all()?; - Ok(()) -} - -#[cfg(not(unix))] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> std::io::Result<()> { - std::fs::write(path, contents) -} - -// --------------------------------------------------------------------------- -// Public async API -// --------------------------------------------------------------------------- - -/// Write `contents` to `path` atomically with `file_mode`, ensuring the -/// parent directory exists and is set to `dir_mode`. -/// -/// On Unix the target file is created with the requested mode before any -/// bytes are written, closing the TOCTOU window. Contents are written to -/// a sibling tempfile and renamed over `path`. -/// -/// Offloads to the blocking thread pool via `spawn_blocking`. -pub async fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file_mode: u32, - dir_mode: u32, -) -> Result<()> { - let path = path.as_ref().to_owned(); - let contents = contents.as_ref().to_vec(); - tokio::task::spawn_blocking(move || { - write_atomic_restricted_sync(&path, &contents, file_mode, dir_mode) - }) - .await - .context("write_atomic_restricted task panicked")? -} - -/// Remove a file if it exists; silently return `Ok(())` if not found. -/// -/// Offloads to the blocking thread pool via `spawn_blocking`. -pub async fn remove_if_exists(path: impl AsRef) -> Result<()> { - let path = path.as_ref().to_owned(); - tokio::task::spawn_blocking(move || remove_if_exists_sync(&path)) - .await - .context("remove_if_exists task panicked")? -} - -// --------------------------------------------------------------------------- -// Blocking (synchronous) API -// --------------------------------------------------------------------------- - -/// Synchronous variants for use in contexts that cannot await -/// (e.g. extism host function callbacks). -pub mod blocking { - use std::path::Path; - - use anyhow::Result; - - /// Synchronous version of [`super::write_atomic_restricted`]. - pub fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file_mode: u32, - dir_mode: u32, - ) -> Result<()> { - super::write_atomic_restricted_sync( - path.as_ref(), - contents.as_ref(), - file_mode, - dir_mode, - ) - } - - /// Synchronous version of [`super::remove_if_exists`]. - pub fn remove_if_exists(path: impl AsRef) -> Result<()> { - super::remove_if_exists_sync(path.as_ref()) - } -} - -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - -#[cfg(all(test, unix))] -#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] -mod tests { - use super::blocking; - use std::os::unix::fs::PermissionsExt; - - #[test] - fn writes_file_and_dir_with_requested_modes() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("creds"); - blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - let dir_mode = std::fs::metadata(target.parent().unwrap()) - .unwrap() - .permissions() - .mode() - & 0o777; - assert_eq!(file_mode, 0o600, "file mode must be 0o600, got {file_mode:o}"); - assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); - } - - #[test] - fn overwrites_existing_file_preserving_mode() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("creds"); - blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); - blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"v2"); - let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600); - } - - #[test] - fn tightens_existing_dir_with_looser_mode() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("loose"); - std::fs::create_dir(&dir).unwrap(); - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); - - let target = dir.join("creds"); - blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); - - let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dir_mode, 0o700); - } - - #[test] - fn remove_if_exists_is_idempotent() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - blocking::remove_if_exists(&target).unwrap(); - std::fs::write(&target, "x").unwrap(); - blocking::remove_if_exists(&target).unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn async_write_atomic_restricted() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("async_creds"); - super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) - .await - .unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(file_mode, 0o600); - } - - #[tokio::test] - async fn async_remove_if_exists() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - super::remove_if_exists(&target).await.unwrap(); - std::fs::write(&target, "x").unwrap(); - super::remove_if_exists(&target).await.unwrap(); - assert!(!target.exists()); - } -} -``` - -**Step 4: Run tests to verify they pass** - -```bash -cargo test -p hm-util -``` - -Expected: all 6 tests pass. - -**Step 5: Commit** - -```bash -git add crates/hm-util/src/os/fs.rs -git commit -m "feat(hm-util): implement os::fs with async + blocking atomic file I/O" -``` - ---- - -## Task 3: Implement `os::dirs` — platform directory resolution - -**Files:** -- Modify: `crates/hm-util/src/os/dirs.rs` - -**Step 1: Write tests** - -```rust -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - - #[test] - fn home_dir_resolves() { - let p = home_dir().unwrap(); - assert!(p.exists(), "home dir should exist: {}", p.display()); - } - - #[test] - fn config_dir_resolves() { - let p = config_dir().unwrap(); - assert!( - p.to_string_lossy().len() > 1, - "config dir should be a real path" - ); - } -} -``` - -**Step 2: Run tests to verify they fail** - -```bash -cargo test -p hm-util -- dirs -``` - -Expected: FAIL — functions don't exist. - -**Step 3: Implement `os::dirs`** - -Write `crates/hm-util/src/os/dirs.rs`: - -```rust -use std::path::PathBuf; - -use anyhow::{Context, Result}; - -/// Platform home directory (`~/` on Unix, `C:\Users\` on Windows). -pub fn home_dir() -> Result { - dirs::home_dir().context("could not determine home directory") -} - -/// Platform config directory (`~/.config` on Linux, -/// `~/Library/Application Support` on macOS, `%APPDATA%` on Windows). -pub fn config_dir() -> Result { - dirs::config_dir().context("could not determine config directory") -} - -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - - #[test] - fn home_dir_resolves() { - let p = home_dir().unwrap(); - assert!(p.exists(), "home dir should exist: {}", p.display()); - } - - #[test] - fn config_dir_resolves() { - let p = config_dir().unwrap(); - assert!( - p.to_string_lossy().len() > 1, - "config dir should be a real path" - ); - } -} -``` - -**Step 4: Run tests** - -```bash -cargo test -p hm-util -``` - -Expected: all tests pass (6 fs + 2 dirs). - -**Step 5: Commit** - -```bash -git add crates/hm-util/src/os/dirs.rs -git commit -m "feat(hm-util): add os::dirs for platform directory resolution" -``` - ---- - -## Task 4: Migrate `hm` to use `hm-util::os::fs` - -**Files:** -- Modify: `crates/hm/Cargo.toml` — add `hm-util` dependency -- Modify: `crates/hm/src/config.rs:84` — use `hm_util::os::fs::blocking` -- Modify: `crates/hm/src/creds_store.rs:35` — use `hm_util::os::fs::blocking` -- Modify: `crates/hm/src/lib.rs:14` — remove `pub mod fs_util;` -- Delete: `crates/hm/src/fs_util.rs` - -**Step 1: Add `hm-util` dependency to `hm`** - -In `crates/hm/Cargo.toml`, add to `[dependencies]`: - -```toml -hm-util = { workspace = true } -``` - -**Step 2: Update `config.rs` — replace `crate::fs_util` with `hm_util::os::fs::blocking`** - -In `crates/hm/src/config.rs`, line 84, change: - -```rust -// Before: -crate::fs_util::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) -// After: -hm_util::os::fs::blocking::write_atomic_restricted(&path, serialized.as_bytes(), 0o644, 0o700) -``` - -**Step 3: Update `creds_store.rs` — same replacement** - -In `crates/hm/src/creds_store.rs`, line 35, change: - -```rust -// Before: -crate::fs_util::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) -// After: -hm_util::os::fs::blocking::write_atomic_restricted(&p, serialized.as_bytes(), 0o600, 0o700) -``` - -Also update the module doc comment at line 4: - -```rust -// Before: -//! mode 0o600 (parent dir 0o700) via [`crate::fs_util::write_atomic_restricted`]. -// After: -//! mode 0o600 (parent dir 0o700) via [`hm_util::os::fs::blocking::write_atomic_restricted`]. -``` - -**Step 4: Remove `fs_util` module from `lib.rs`** - -In `crates/hm/src/lib.rs`, remove line 14: - -```rust -pub mod fs_util; -``` - -**Step 5: Delete `fs_util.rs`** - -```bash -rm crates/hm/src/fs_util.rs -``` - -**Step 6: Update doc comment in `fs_util.rs` references** - -The doc header in the now-deleted file referenced `crate::creds_store` and `config::user_config_dir` — these lived in `fs_util.rs` which is now gone. No action needed since the file is deleted. - -**Step 7: Verify compilation and tests** - -```bash -cargo check -p harmont-cli && cargo test -p harmont-cli -``` - -Expected: all existing tests pass. The `fs_util::tests` that were in the deleted file are now covered by identical tests in `hm-util`. - -**Step 8: Commit** - -```bash -git add crates/hm/Cargo.toml crates/hm/src/config.rs crates/hm/src/creds_store.rs crates/hm/src/lib.rs -git rm crates/hm/src/fs_util.rs -git commit -m "refactor: migrate fs_util callers to hm-util::os::fs::blocking" -``` - ---- - -## Task 5: Migrate `hm` directory functions to use `hm-util::os::dirs` - -**Files:** -- Modify: `crates/hm/src/config.rs:14-16` — use `hm_util::os::dirs::home_dir` -- Modify: `crates/hm/src/plugin/paths.rs:16` — use `hm_util::os::dirs::config_dir` - -**Step 1: Update `config.rs::user_config_dir()`** - -In `crates/hm/src/config.rs`, change the `user_config_dir` function (lines 14-17): - -```rust -// Before: -pub fn user_config_dir() -> Result { - let home = dirs::home_dir().context("could not determine home directory")?; - Ok(home.join(".harmont")) -} - -// After: -pub fn user_config_dir() -> Result { - Ok(hm_util::os::dirs::home_dir()?.join(".harmont")) -} -``` - -**Step 2: Update `plugin/paths.rs::user_plugins_dir()`** - -In `crates/hm/src/plugin/paths.rs`, change line 16: - -```rust -// Before: -pub fn user_plugins_dir() -> Option { - dirs::config_dir().map(|p| p.join("harmont").join("plugins")) -} - -// After: -pub fn user_plugins_dir() -> Option { - hm_util::os::dirs::config_dir() - .ok() - .map(|p| p.join("harmont").join("plugins")) -} -``` - -**Step 3: Remove `dirs` direct dependency from `hm`** - -In `crates/hm/Cargo.toml`, remove: - -```toml -dirs = "6" -``` - -**Step 4: Verify no other `dirs::` usage in `hm`** - -```bash -grep -rn 'dirs::' crates/hm/src/ --include='*.rs' -``` - -Expected: no matches (all usage now goes through `hm_util::os::dirs`). - -**Step 5: Run tests** - -```bash -cargo test -p harmont-cli -``` - -Expected: all tests pass. - -**Step 6: Commit** - -```bash -git add crates/hm/Cargo.toml crates/hm/src/config.rs crates/hm/src/plugin/paths.rs -git commit -m "refactor: delegate directory resolution to hm-util::os::dirs" -``` - ---- - -## Task 6: Replace custom `CancellationToken` with `tokio_util::sync::CancellationToken` - -**Context:** The custom `CancellationToken` in `orchestrator/cancel.rs` is a 55-line `Arc` wrapper. `tokio_util::sync::CancellationToken` provides the same API (`new()`, `cancel()`, `is_cancelled()`) plus a zero-cost `.cancelled()` future — eliminating the 50ms polling loop in `docker_host_fns.rs:163-171`. - -Note: `tokio_util::sync::CancellationToken` is already used in `plugin/host_fns.rs:850` for the OAuth loopback server, so the dependency and feature flag are already available. - -**Files:** -- Delete: `crates/hm/src/orchestrator/cancel.rs` -- Modify: `crates/hm/src/orchestrator/mod.rs` — remove `pub mod cancel;` -- Modify: `crates/hm/src/orchestrator/state.rs:27` — update import -- Modify: `crates/hm/src/orchestrator/scheduler.rs:47,74` — update import + construction -- Modify: `crates/hm/src/orchestrator/docker_host_fns.rs:163-171` — replace polling loop -- Modify: `crates/hm/src/plugin/signal.rs:18` — update import -- Modify: `crates/hm/src/plugin/host_fns.rs:925` — update path - -**Step 1: Update `orchestrator/mod.rs` — remove cancel module** - -In `crates/hm/src/orchestrator/mod.rs`, remove line 11: - -```rust -pub mod cancel; -``` - -**Step 2: Update `orchestrator/state.rs` — change import** - -In `crates/hm/src/orchestrator/state.rs`, replace line 27: - -```rust -// Before: -use super::cancel::CancellationToken; -// After: -use tokio_util::sync::CancellationToken; -``` - -**Step 3: Update `orchestrator/scheduler.rs` — change import** - -In `crates/hm/src/orchestrator/scheduler.rs`, replace line 47: - -```rust -// Before: -use super::cancel::CancellationToken; -// After: -use tokio_util::sync::CancellationToken; -``` - -**Step 4: Update `plugin/signal.rs` — change import** - -In `crates/hm/src/plugin/signal.rs`, replace line 18: - -```rust -// Before: -use crate::orchestrator::cancel::CancellationToken; -// After: -use tokio_util::sync::CancellationToken; -``` - -**Step 5: Update `orchestrator/docker_host_fns.rs` — replace polling loop with `.cancelled()`** - -Replace lines 163-171: - -```rust -// Before: -async fn wait_cancel(cancel: &crate::orchestrator::cancel::CancellationToken) { - // Poll the atomic every 50ms. Cheap; never wakes a thread early. - loop { - if cancel.is_cancelled() { - return; - } - tokio::time::sleep(std::time::Duration::from_millis(50)).await; - } -} - -// After: -async fn wait_cancel(cancel: &tokio_util::sync::CancellationToken) { - cancel.cancelled().await; -} -``` - -**Step 6: Update `plugin/host_fns.rs:925` — fix `is_cancelled` path** - -In `crates/hm/src/plugin/host_fns.rs`, line 925 references `s.cancel.is_cancelled()`. The `CancellationToken` field type changes but the method name is the same — verify this line still compiles (it should, as `tokio_util::sync::CancellationToken` also has `is_cancelled()`). - -**Step 7: Delete `orchestrator/cancel.rs`** - -```bash -rm crates/hm/src/orchestrator/cancel.rs -``` - -**Step 8: Verify compilation and tests** - -```bash -cargo check -p harmont-cli && cargo test -p harmont-cli -``` - -Expected: compiles and all tests pass. The three tests that were in `cancel.rs` (default_is_not_cancelled, cancel_persists, cancel_is_clone_shared) are trivially true for `tokio_util::sync::CancellationToken` — the upstream crate tests them. - -**Step 9: Commit** - -```bash -git rm crates/hm/src/orchestrator/cancel.rs -git add crates/hm/src/orchestrator/mod.rs crates/hm/src/orchestrator/state.rs crates/hm/src/orchestrator/scheduler.rs crates/hm/src/orchestrator/docker_host_fns.rs crates/hm/src/plugin/signal.rs crates/hm/src/plugin/host_fns.rs -git commit -m "refactor: replace custom CancellationToken with tokio_util::sync::CancellationToken - -Eliminates 55-line Arc wrapper. The 50ms polling loop in -wait_cancel is replaced by the zero-cost .cancelled() future." -``` - ---- - -## Task 7: Final verification and cleanup - -**Step 1: Full workspace build** - -```bash -cargo build --workspace -``` - -Expected: clean build, no warnings. - -**Step 2: Full workspace test suite** - -```bash -cargo test --workspace -``` - -Expected: all tests pass. - -**Step 3: Clippy** - -```bash -cargo clippy --workspace -- -D warnings -``` - -Expected: no warnings. - -**Step 4: Verify module structure matches intent** - -```bash -find crates/hm-util/src -name '*.rs' | sort -``` - -Expected: -``` -crates/hm-util/src/lib.rs -crates/hm-util/src/os/dirs.rs -crates/hm-util/src/os/fs.rs -crates/hm-util/src/os/mod.rs -``` - -**Step 5: Verify deleted files are gone** - -```bash -test ! -f crates/hm/src/fs_util.rs && echo "fs_util.rs removed" -test ! -f crates/hm/src/orchestrator/cancel.rs && echo "cancel.rs removed" -``` - -**Step 6: Final commit if any cleanup was needed** - -```bash -git status -# If clean: done. If changes: commit cleanup. -``` - ---- - -## Future Opportunities (Not In Scope) - -These were identified during analysis but deferred: - -1. **Async propagation in `Config` and `RunContext`** — `Config::load()` and `Config::save()` could become async, using `hm_util::os::fs::write_atomic_restricted` (async variant) directly. `RunContext::from_cli()` would become `async fn from_cli()`. Benefit: avoids blocking tokio worker thread during config I/O. Cost: minor — `from_cli` is only called from `async fn run()` in `main.rs`. Deferred because config files are tiny and the perf impact is negligible. - -2. **`creds_store` async variant** — Blocked by extism host_fn callbacks being sync. Would require `block_in_place` bridge in host_fns. No benefit until extism supports async host functions. - -3. **Signal handler extraction** — `plugin/signal.rs::install_ctrlc` is application-specific (two-stage Ctrl-C, exit code 130, stderr messages). Not a reusable utility. Could move from `plugin/` to a top-level `signal.rs` module if the `plugin/` location feels wrong, but extraction to `hm-util` is over-engineering. - -4. **`output/format.rs` time utilities** — `rel_time()`, `duration_human()`, `elapsed_between()` are generic but small (< 30 lines total). Not worth extracting until a second consumer exists. - -5. **`os::process` module** — Future home for process-related utilities if patterns emerge (e.g., a generic `spawn_and_stream` helper for the Docker client). From 20f30f4144e7986a2f5e30147ad57750514021b8 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 12:58:00 -0700 Subject: [PATCH 15/25] feat(hm-util): add windows crate for atomic file replacement --- Cargo.lock | 52 +++++++++++++++++++++++++++++++++++++++ crates/hm-util/Cargo.toml | 7 ++++++ 2 files changed, 59 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index c50cfdff..67cd0e65 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1718,6 +1718,7 @@ dependencies = [ "dirs", "tempfile", "tokio", + "windows", ] [[package]] @@ -5019,6 +5020,27 @@ dependencies = [ "wasmtime-internal-math", ] +[[package]] +name = "windows" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "527fadee13e0c05939a6a05d5bd6eec6cd2e3dbd648b9f8e447c6518133d8580" +dependencies = [ + "windows-collections", + "windows-core", + "windows-future", + "windows-numerics", +] + +[[package]] +name = "windows-collections" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b2d95af1a8a14a3c7367e1ed4fc9c20e0a26e79551b1454d72583c97cc6610" +dependencies = [ + "windows-core", +] + [[package]] name = "windows-core" version = "0.62.2" @@ -5032,6 +5054,17 @@ dependencies = [ "windows-strings", ] +[[package]] +name = "windows-future" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1d6f90251fe18a279739e78025bd6ddc52a7e22f921070ccdc67dde84c605cb" +dependencies = [ + "windows-core", + "windows-link", + "windows-threading", +] + [[package]] name = "windows-implement" version = "0.60.2" @@ -5060,6 +5093,16 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-numerics" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2e40844ac143cdb44aead537bbf727de9b044e107a0f1220392177d15b0f26" +dependencies = [ + "windows-core", + "windows-link", +] + [[package]] name = "windows-result" version = "0.4.1" @@ -5147,6 +5190,15 @@ dependencies = [ "windows_x86_64_msvc 0.53.1", ] +[[package]] +name = "windows-threading" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3949bd5b99cafdf1c7ca86b43ca564028dfe27d66958f2470940f73d86d75b37" +dependencies = [ + "windows-link", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" diff --git a/crates/hm-util/Cargo.toml b/crates/hm-util/Cargo.toml index 758c0f65..01a9765c 100644 --- a/crates/hm-util/Cargo.toml +++ b/crates/hm-util/Cargo.toml @@ -10,6 +10,13 @@ description = "Shared OS and filesystem utilities for Harmont crates." dirs = "6" tokio = { version = "1", features = ["rt", "fs"] } +[target.'cfg(windows)'.dependencies.windows] +version = "0.62" +features = [ + "Win32_Foundation", + "Win32_Storage_FileSystem", +] + [dev-dependencies] tempfile = "3" tokio = { version = "1", features = ["full", "test-util"] } From 775d1d565be6ee0930bd8936ea95150a41bdb8af Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:00:11 -0700 Subject: [PATCH 16/25] feat(hm-util): add atomic_rename_over with ReplaceFileW on Windows --- crates/hm-util/src/os/fs.rs | 124 ++++++++++++++++++++++++++++++------ 1 file changed, 104 insertions(+), 20 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 06b0ac92..01c77121 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -9,10 +9,6 @@ use std::io; use std::path::Path; -// --------------------------------------------------------------------------- -// Private sync core -// --------------------------------------------------------------------------- - fn write_atomic_restricted_sync( path: &Path, contents: &[u8], @@ -101,9 +97,55 @@ fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> io::Re } // --------------------------------------------------------------------------- -// Public async API +// Cross-platform atomic rename // --------------------------------------------------------------------------- +#[cfg(unix)] +fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { + std::fs::rename(from, to) +} + +#[cfg(windows)] +fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { + use windows::core::HSTRING; + use windows::Win32::Storage::FileSystem::{ + MoveFileExW, ReplaceFileW, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + REPLACEFILE_IGNORE_MERGE_ERRORS, + }; + + let from_w = HSTRING::from(from.as_os_str()); + let to_w = HSTRING::from(to.as_os_str()); + + // ReplaceFileW preserves ACLs and alternate data streams on the + // target, but requires the target to already exist. + 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)); + } + + // Target doesn't exist yet — fall back to MoveFileExW which handles + // both cases but doesn't preserve target metadata (irrelevant here + // since there is no target). + 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)) +} + /// Write `contents` to `path` atomically with `file_mode`, ensuring the /// parent directory exists and is set to `dir_mode`. /// @@ -131,6 +173,28 @@ pub async fn write_atomic_restricted( .map_err(io::Error::other)? } +/// Atomically replace `to` with `from`. +/// +/// On Unix this is a single `rename(2)` call — 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<()> { + let from = from.as_ref().to_owned(); + let to = to.as_ref().to_owned(); + tokio::task::spawn_blocking(move || atomic_rename_over_sync(&from, &to)) + .await + .map_err(io::Error::other)? +} + /// Remove a file if it exists; silently return `Ok(())` if it does not. /// /// This is the async counterpart of [`blocking::remove_if_exists`]; @@ -140,17 +204,14 @@ pub async fn write_atomic_restricted( /// /// Returns an error if `remove_file` fails for any reason other than /// `NotFound`. -pub async fn remove_if_exists(path: impl AsRef) -> io::Result<()> { - let path = path.as_ref().to_owned(); - tokio::task::spawn_blocking(move || remove_if_exists_sync(&path)) - .await - .map_err(io::Error::other)? +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), + } } -// --------------------------------------------------------------------------- -// Public blocking module -// --------------------------------------------------------------------------- - /// Synchronous (blocking) wrappers for callers that cannot use async, /// such as extism `host_fn` callbacks. pub mod blocking { @@ -188,10 +249,6 @@ pub mod blocking { } } -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - #[cfg(all(test, unix))] #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { @@ -271,9 +328,36 @@ mod tests { async fn async_remove_if_exists() { let tmp = tempfile::tempdir().unwrap(); let target = tmp.path().join("nothing"); - super::remove_if_exists(&target).await.unwrap(); + super::remove_file_if_exists(&target).await.unwrap(); std::fs::write(&target, "x").unwrap(); - super::remove_if_exists(&target).await.unwrap(); + super::remove_file_if_exists(&target).await.unwrap(); assert!(!target.exists()); } + + #[tokio::test] + async fn atomic_rename_over_replaces_target() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&dst, b"old").unwrap(); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists(), "source should be gone after rename"); + } + + #[tokio::test] + async fn atomic_rename_over_works_when_target_missing() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists()); + } } From 716a4555a55b08588f6155c21dfa230eb300447a Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:05:35 -0700 Subject: [PATCH 17/25] refactor(hm-util): use atomic_rename_over_sync in write_atomic_restricted --- crates/hm-util/src/os/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 01c77121..97d79f6b 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -39,7 +39,7 @@ fn write_atomic_restricted_sync( write_file_with_mode_sync(&tmp_path, contents, file_mode)?; - let persist_result = std::fs::rename(&tmp_path, path); + let persist_result = atomic_rename_over_sync(&tmp_path, path); if persist_result.is_err() { let _ = std::fs::remove_file(&tmp_path); } From a45d21e066a1e73413a3a49c2a0d6d9a4ee48828 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:13:15 -0700 Subject: [PATCH 18/25] refactor(hm-util): unify blocking wrappers via block_in_place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove duplicate sync code paths — blocking:: now shells out to the async API through tokio::task::block_in_place. Platform-specific rename logic moves from standalone _sync functions to private _impl helpers positioned after the public async fn. --- crates/hm-util/src/os/fs.rs | 132 ++++++++++++++++------------------- crates/hm/src/creds_store.rs | 4 +- 2 files changed, 62 insertions(+), 74 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 97d79f6b..e8fda771 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -39,21 +39,13 @@ fn write_atomic_restricted_sync( write_file_with_mode_sync(&tmp_path, contents, file_mode)?; - let persist_result = atomic_rename_over_sync(&tmp_path, path); + let persist_result = atomic_rename_over_impl(&tmp_path, path); if persist_result.is_err() { let _ = std::fs::remove_file(&tmp_path); } persist_result } -fn remove_if_exists_sync(path: &Path) -> io::Result<()> { - match std::fs::remove_file(path) { - Ok(()) => Ok(()), - Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e), - } -} - #[cfg(unix)] fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> io::Result<()> { use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; @@ -96,56 +88,6 @@ fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> io::Re std::fs::write(path, contents) } -// --------------------------------------------------------------------------- -// Cross-platform atomic rename -// --------------------------------------------------------------------------- - -#[cfg(unix)] -fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { - std::fs::rename(from, to) -} - -#[cfg(windows)] -fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { - use windows::core::HSTRING; - use windows::Win32::Storage::FileSystem::{ - MoveFileExW, ReplaceFileW, - MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, - REPLACEFILE_IGNORE_MERGE_ERRORS, - }; - - let from_w = HSTRING::from(from.as_os_str()); - let to_w = HSTRING::from(to.as_os_str()); - - // ReplaceFileW preserves ACLs and alternate data streams on the - // target, but requires the target to already exist. - 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)); - } - - // Target doesn't exist yet — fall back to MoveFileExW which handles - // both cases but doesn't preserve target metadata (irrelevant here - // since there is no target). - 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)) -} - /// Write `contents` to `path` atomically with `file_mode`, ensuring the /// parent directory exists and is set to `dir_mode`. /// @@ -190,11 +132,52 @@ pub async fn atomic_rename_over( ) -> io::Result<()> { let from = from.as_ref().to_owned(); let to = to.as_ref().to_owned(); - tokio::task::spawn_blocking(move || atomic_rename_over_sync(&from, &to)) + tokio::task::spawn_blocking(move || atomic_rename_over_impl(&from, &to)) .await .map_err(io::Error::other)? } +#[cfg(unix)] +fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + std::fs::rename(from, to) +} + +#[cfg(windows)] +fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + use windows::core::HSTRING; + use windows::Win32::Storage::FileSystem::{ + MoveFileExW, ReplaceFileW, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + REPLACEFILE_IGNORE_MERGE_ERRORS, + }; + + 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)) +} + /// Remove a file if it exists; silently return `Ok(())` if it does not. /// /// This is the async counterpart of [`blocking::remove_if_exists`]; @@ -212,12 +195,17 @@ pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { } } -/// Synchronous (blocking) wrappers for callers that cannot use async, -/// such as extism `host_fn` callbacks. +/// Synchronous (blocking) 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 (e.g. extism `host_fn` callbacks). pub mod blocking { 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)) + } + /// Write `contents` to `path` atomically with `file_mode`, ensuring the /// parent directory exists and is set to `dir_mode`. /// @@ -235,7 +223,7 @@ pub mod blocking { file_mode: u32, dir_mode: u32, ) -> io::Result<()> { - super::write_atomic_restricted_sync(path.as_ref(), contents.as_ref(), file_mode, dir_mode) + block_on(super::write_atomic_restricted(path, contents, file_mode, dir_mode)) } /// Remove a file if it exists; silently return `Ok(())` if it does not. @@ -245,7 +233,7 @@ pub mod blocking { /// Returns an error if `remove_file` fails for any reason other than /// `NotFound`. pub fn remove_if_exists(path: impl AsRef) -> io::Result<()> { - super::remove_if_exists_sync(path.as_ref()) + block_on(super::remove_file_if_exists(path)) } } @@ -255,8 +243,8 @@ mod tests { use super::blocking; use std::os::unix::fs::PermissionsExt; - #[test] - fn writes_file_and_dir_with_requested_modes() { + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn writes_file_and_dir_with_requested_modes() { let tmp = tempfile::tempdir().unwrap(); let target = tmp.path().join("sub").join("creds"); blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); @@ -275,8 +263,8 @@ mod tests { assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); } - #[test] - fn overwrites_existing_file_preserving_mode() { + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn overwrites_existing_file_preserving_mode() { let tmp = tempfile::tempdir().unwrap(); let target = tmp.path().join("creds"); blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); @@ -287,8 +275,8 @@ mod tests { assert_eq!(mode, 0o600); } - #[test] - fn tightens_existing_dir_with_looser_mode() { + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn tightens_existing_dir_with_looser_mode() { let tmp = tempfile::tempdir().unwrap(); let dir = tmp.path().join("loose"); std::fs::create_dir(&dir).unwrap(); @@ -301,8 +289,8 @@ mod tests { assert_eq!(dir_mode, 0o700); } - #[test] - fn remove_if_exists_is_idempotent() { + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn remove_if_exists_is_idempotent() { let tmp = tempfile::tempdir().unwrap(); let target = tmp.path().join("nothing"); blocking::remove_if_exists(&target).unwrap(); diff --git a/crates/hm/src/creds_store.rs b/crates/hm/src/creds_store.rs index 730307fa..d6e45d03 100644 --- a/crates/hm/src/creds_store.rs +++ b/crates/hm/src/creds_store.rs @@ -92,8 +92,8 @@ mod tests { } } - #[test] - fn round_trip() { + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn round_trip() { with_home(|| { assert_eq!(get("svc", "acct"), None); set("svc", "acct", "shh"); From 3d8af12379020dc159ff850c18b822eecfe260a2 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:29:06 -0700 Subject: [PATCH 19/25] refactor(hm-util): async-first fs module, eliminate sync indirection --- crates/hm-util/src/os/fs.rs | 207 ++++++++++++++++++------------------ 1 file changed, 102 insertions(+), 105 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index e8fda771..fa451e70 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -1,53 +1,113 @@ //! Atomic, permission-restricted filesystem helpers. //! -//! The main entry point is [`write_atomic_restricted`] (async) or -//! [`blocking::write_atomic_restricted`] (sync). 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. +//! 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 +//! (e.g. extism `host_fn` callbacks). +//! +//! 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; -fn write_atomic_restricted_sync( - path: &Path, - contents: &[u8], +/// Write `contents` to `path` atomically with `file_mode`, ensuring the +/// parent directory exists and is set to `dir_mode`. +/// +/// Internally offloads blocking I/O to [`tokio::task::spawn_blocking`]. +/// +/// # 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_mode`, the +/// tempfile cannot be opened with `file_mode` or written, or the final +/// `rename` over `path` fails. +pub async fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, file_mode: u32, dir_mode: u32, ) -> io::Result<()> { - let parent = path.parent().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - format!("{} has no parent directory", path.display()), - ) - })?; - - create_dir_with_mode_sync(parent, dir_mode)?; - - let file_name = path - .file_name() - .ok_or_else(|| { + let path = path.as_ref().to_owned(); + let contents = contents.as_ref().to_vec(); + tokio::task::spawn_blocking(move || { + let parent = path.parent().ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidInput, - format!("{} has no file name", path.display()), + format!("{} has no parent directory", 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); + })?; + + create_dir_with_mode(parent, dir_mode)?; + + 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_mode)?; + + let persist_result = atomic_rename_over_impl(&tmp_path, &path); + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result + }) + .await + .map_err(io::Error::other)? +} - write_file_with_mode_sync(&tmp_path, contents, file_mode)?; +/// Atomically replace `to` with `from`. +/// +/// On Unix this is a single `rename(2)` call — 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<()> { + 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)? +} - let persist_result = atomic_rename_over_impl(&tmp_path, path); - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); +/// 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), } - persist_result } +// --------------------------------------------------------------------------- +// Platform helpers (private) +// --------------------------------------------------------------------------- + #[cfg(unix)] -fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> io::Result<()> { +fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; if dir.exists() { let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; @@ -64,12 +124,12 @@ fn create_dir_with_mode_sync(dir: &Path, mode: u32) -> io::Result<()> { } #[cfg(not(unix))] -fn create_dir_with_mode_sync(dir: &Path, _mode: u32) -> io::Result<()> { +fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { std::fs::create_dir_all(dir) } #[cfg(unix)] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { +fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { use std::io::Write; use std::os::unix::fs::OpenOptionsExt; let mut f = std::fs::OpenOptions::new() @@ -84,59 +144,10 @@ fn write_file_with_mode_sync(path: &Path, contents: &[u8], mode: u32) -> io::Res } #[cfg(not(unix))] -fn write_file_with_mode_sync(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { +fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { std::fs::write(path, contents) } -/// Write `contents` to `path` atomically with `file_mode`, ensuring the -/// parent directory exists and is set to `dir_mode`. -/// -/// This is the async counterpart of [`blocking::write_atomic_restricted`]; -/// the blocking I/O is offloaded to [`tokio::task::spawn_blocking`]. -/// -/// # 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_mode`, the -/// tempfile cannot be opened with `file_mode` or written, or the final -/// `rename` over `path` fails. -pub async fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file_mode: u32, - dir_mode: u32, -) -> io::Result<()> { - let path = path.as_ref().to_owned(); - let contents = contents.as_ref().to_vec(); - tokio::task::spawn_blocking(move || { - write_atomic_restricted_sync(&path, &contents, file_mode, dir_mode) - }) - .await - .map_err(io::Error::other)? -} - -/// Atomically replace `to` with `from`. -/// -/// On Unix this is a single `rename(2)` call — 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<()> { - 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)? -} - #[cfg(unix)] fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { std::fs::rename(from, to) @@ -178,25 +189,12 @@ fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)) } -/// Remove a file if it exists; silently return `Ok(())` if it does not. -/// -/// This is the async counterpart of [`blocking::remove_if_exists`]; -/// the blocking I/O is offloaded to [`tokio::task::spawn_blocking`]. -/// -/// # 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), - } -} +// --------------------------------------------------------------------------- +// Blocking wrappers +// --------------------------------------------------------------------------- -/// Synchronous (blocking) wrappers that shell out to the async API -/// via `tokio::task::block_in_place`. Safe to call from sync contexts +/// 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 (e.g. extism `host_fn` callbacks). pub mod blocking { use std::io; @@ -206,8 +204,7 @@ pub mod blocking { tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(f)) } - /// Write `contents` to `path` atomically with `file_mode`, ensuring the - /// parent directory exists and is set to `dir_mode`. + /// Blocking counterpart of [`super::write_atomic_restricted`]. /// /// See the [module-level documentation](super) for semantics. /// @@ -226,7 +223,7 @@ pub mod blocking { block_on(super::write_atomic_restricted(path, contents, file_mode, dir_mode)) } - /// Remove a file if it exists; silently return `Ok(())` if it does not. + /// Blocking counterpart of [`super::remove_file_if_exists`]. /// /// # Errors /// From 61aaf7411442ec09155a15d00fbb1f240dd70c90 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:49:15 -0700 Subject: [PATCH 20/25] refactor(hm-util): use tokio::fs::rename instead of std::fs::rename atomic_rename_over now delegates to tokio::fs::rename on Unix. write_atomic_restricted splits its spawn_blocking so the rename step goes through the async atomic_rename_over. The sync Unix atomic_rename_over_impl is removed (only Windows variant remains). --- crates/hm-util/src/os/fs.rs | 48 +++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index fa451e70..3110606a 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -29,9 +29,11 @@ pub async fn write_atomic_restricted( file_mode: u32, dir_mode: u32, ) -> io::Result<()> { - let path = path.as_ref().to_owned(); + let dest = path.as_ref().to_owned(); let contents = contents.as_ref().to_vec(); - tokio::task::spawn_blocking(move || { + let path = dest.clone(); + + let tmp_path = tokio::task::spawn_blocking(move || { let parent = path.parent().ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidInput, @@ -56,21 +58,23 @@ pub async fn write_atomic_restricted( write_file_with_mode(&tmp_path, &contents, file_mode)?; - let persist_result = atomic_rename_over_impl(&tmp_path, &path); - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); - } - persist_result + io::Result::Ok(tmp_path) }) .await - .map_err(io::Error::other)? + .map_err(io::Error::other)??; + + let rename_result = atomic_rename_over(&tmp_path, &dest).await; + if rename_result.is_err() { + let _ = tokio::fs::remove_file(&tmp_path).await; + } + rename_result } /// Atomically replace `to` with `from`. /// -/// On Unix this is a single `rename(2)` call — atomic by POSIX -/// guarantee. On Windows this uses `ReplaceFileW` (preserves ACLs -/// and alternate data streams) when the target exists, falling back +/// 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 @@ -81,11 +85,18 @@ pub async fn atomic_rename_over( from: impl AsRef, to: impl AsRef, ) -> io::Result<()> { - 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)? + #[cfg(unix)] + { + tokio::fs::rename(from.as_ref(), to.as_ref()).await + } + #[cfg(windows)] + { + 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. @@ -148,11 +159,6 @@ fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result< std::fs::write(path, contents) } -#[cfg(unix)] -fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { - std::fs::rename(from, to) -} - #[cfg(windows)] fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { use windows::core::HSTRING; From 2923fbbba4a2b11c1fb108ff6dca697960041a53 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 13:56:57 -0700 Subject: [PATCH 21/25] refactor(hm-util): convert fs helpers to async tokio operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_dir_with_mode and write_file_with_mode now use tokio::fs instead of std::fs. This eliminates the last spawn_blocking from write_atomic_restricted — all I/O goes through tokio's async API. --- crates/hm-util/src/os/fs.rs | 107 +++++++++++++++++------------------- 1 file changed, 50 insertions(+), 57 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 3110606a..277edb58 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -15,8 +15,6 @@ use std::path::Path; /// Write `contents` to `path` atomically with `file_mode`, ensuring the /// parent directory exists and is set to `dir_mode`. /// -/// Internally offloads blocking I/O to [`tokio::task::spawn_blocking`]. -/// /// # Errors /// /// Returns an error if `path` has no parent or no file-name component, @@ -29,41 +27,37 @@ pub async fn write_atomic_restricted( file_mode: u32, dir_mode: u32, ) -> io::Result<()> { - let dest = path.as_ref().to_owned(); + let path = path.as_ref().to_owned(); let contents = contents.as_ref().to_vec(); - let path = dest.clone(); - let tmp_path = tokio::task::spawn_blocking(move || { - let parent = path.parent().ok_or_else(|| { + let parent = path + .parent() + .ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidInput, format!("{} has no parent directory", path.display()), ) - })?; - - create_dir_with_mode(parent, dir_mode)?; - - 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_mode)?; - - io::Result::Ok(tmp_path) - }) - .await - .map_err(io::Error::other)??; - - let rename_result = atomic_rename_over(&tmp_path, &dest).await; + })? + .to_owned(); + + create_dir_with_mode(&parent, dir_mode).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_mode).await?; + + let rename_result = atomic_rename_over(&tmp_path, &path).await; if rename_result.is_err() { let _ = tokio::fs::remove_file(&tmp_path).await; } @@ -118,45 +112,44 @@ pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { // --------------------------------------------------------------------------- #[cfg(unix)] -fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { - use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; - if dir.exists() { - let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; - if current != mode { - std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; +async fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { + 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?; } - } else { - std::fs::DirBuilder::new() - .recursive(true) - .mode(mode) - .create(dir)?; + Err(e) => return Err(e), } Ok(()) } #[cfg(not(unix))] -fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { - std::fs::create_dir_all(dir) +async fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { + tokio::fs::create_dir_all(dir).await } #[cfg(unix)] -fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut f = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(mode) - .open(path)?; - f.write_all(contents)?; - f.sync_all()?; +async fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { + 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?; Ok(()) } #[cfg(not(unix))] -fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { - std::fs::write(path, contents) +async fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { + tokio::fs::write(path, contents).await } #[cfg(windows)] From 73c10e0996d1a9cfb373bed53b9ff772d80fc38f Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 14:55:47 -0700 Subject: [PATCH 22/25] manual cleanup --- crates/hm-util/src/os/fs.rs | 185 +++++++----------------------------- 1 file changed, 35 insertions(+), 150 deletions(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 277edb58..baa0eb43 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -107,49 +107,51 @@ pub async fn remove_file_if_exists(path: impl AsRef) -> io::Result<()> { } } -// --------------------------------------------------------------------------- -// Platform helpers (private) -// --------------------------------------------------------------------------- - #[cfg(unix)] async fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { - 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?; + #[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), } - 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(not(unix))] -async fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { - tokio::fs::create_dir_all(dir).await + #[cfg(windows)] + { + tokio::fs::create_dir_all(dir).await + } + Ok(()) } -#[cfg(unix)] async fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { - 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?; - Ok(()) -} + #[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(not(unix))] -async fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { - tokio::fs::write(path, contents).await + #[cfg(windows)] + { + tokio::fs::write(path, contents).await + } + + Ok(()) } #[cfg(windows)] @@ -188,10 +190,6 @@ fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { result.map_err(|e| io::Error::new(io::ErrorKind::Other, e)) } -// --------------------------------------------------------------------------- -// Blocking wrappers -// --------------------------------------------------------------------------- - /// 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 (e.g. extism `host_fn` callbacks). @@ -232,116 +230,3 @@ pub mod blocking { block_on(super::remove_file_if_exists(path)) } } - -#[cfg(all(test, unix))] -#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] -mod tests { - use super::blocking; - use std::os::unix::fs::PermissionsExt; - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn writes_file_and_dir_with_requested_modes() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("creds"); - blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - let dir_mode = std::fs::metadata(target.parent().unwrap()) - .unwrap() - .permissions() - .mode() - & 0o777; - assert_eq!( - file_mode, 0o600, - "file mode must be 0o600, got {file_mode:o}" - ); - assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn overwrites_existing_file_preserving_mode() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("creds"); - blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); - blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"v2"); - let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn tightens_existing_dir_with_looser_mode() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("loose"); - std::fs::create_dir(&dir).unwrap(); - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); - - let target = dir.join("creds"); - blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); - - let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dir_mode, 0o700); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn remove_if_exists_is_idempotent() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - blocking::remove_if_exists(&target).unwrap(); - std::fs::write(&target, "x").unwrap(); - blocking::remove_if_exists(&target).unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn async_write_atomic_restricted() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("async_creds"); - super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) - .await - .unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(file_mode, 0o600); - } - - #[tokio::test] - async fn async_remove_if_exists() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - super::remove_file_if_exists(&target).await.unwrap(); - std::fs::write(&target, "x").unwrap(); - super::remove_file_if_exists(&target).await.unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn atomic_rename_over_replaces_target() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&dst, b"old").unwrap(); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists(), "source should be gone after rename"); - } - - #[tokio::test] - async fn atomic_rename_over_works_when_target_missing() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists()); - } -} From b01d39730db0fd9b6f5bacaec5e89d4426596ae0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 14:57:09 -0700 Subject: [PATCH 23/25] deslop --- crates/hm-util/src/os/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index baa0eb43..4fc21a9b 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -1,4 +1,4 @@ -//! Atomic, permission-restricted filesystem helpers. +//! Filesystem helpers. //! //! The main entry point is [`write_atomic_restricted`]. A synchronous //! wrapper is available at [`blocking::write_atomic_restricted`] for From b16bc83dd6c6f7051b00707a2eee6a8e44c005a7 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 14:58:04 -0700 Subject: [PATCH 24/25] more cleanup --- crates/hm-util/src/os/fs.rs | 70 ++-- docs/plans/2026-05-23-async-first-fs.md | 409 ++++++++++++++++++++ docs/plans/2026-05-23-atomic-rename-over.md | 269 +++++++++++++ 3 files changed, 713 insertions(+), 35 deletions(-) create mode 100644 docs/plans/2026-05-23-async-first-fs.md create mode 100644 docs/plans/2026-05-23-atomic-rename-over.md diff --git a/crates/hm-util/src/os/fs.rs b/crates/hm-util/src/os/fs.rs index 4fc21a9b..fec7a308 100644 --- a/crates/hm-util/src/os/fs.rs +++ b/crates/hm-util/src/os/fs.rs @@ -85,6 +85,41 @@ pub async fn atomic_rename_over( } #[cfg(windows)] { + fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + use windows::core::HSTRING; + use windows::Win32::Storage::FileSystem::{ + MoveFileExW, ReplaceFileW, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + REPLACEFILE_IGNORE_MERGE_ERRORS, + }; + + 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)) @@ -154,41 +189,6 @@ async fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Re Ok(()) } -#[cfg(windows)] -fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { - use windows::core::HSTRING; - use windows::Win32::Storage::FileSystem::{ - MoveFileExW, ReplaceFileW, - MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, - REPLACEFILE_IGNORE_MERGE_ERRORS, - }; - - 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)) -} /// Synchronous wrappers that shell out to the async API via /// `tokio::task::block_in_place`. Safe to call from sync contexts diff --git a/docs/plans/2026-05-23-async-first-fs.md b/docs/plans/2026-05-23-async-first-fs.md new file mode 100644 index 00000000..ad82c175 --- /dev/null +++ b/docs/plans/2026-05-23-async-first-fs.md @@ -0,0 +1,409 @@ +# Async-First fs.rs Restructure + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Make async functions the primary implementations in `hm-util`'s `fs` module — eliminate the `write_atomic_restricted_sync` indirection, drop `_sync` suffixes from private helpers, fix doc comments that frame async as secondary. + +**Architecture:** The async public functions (`write_atomic_restricted`, `atomic_rename_over`, `remove_file_if_exists`) should contain all logic directly. Private platform helpers (`create_dir_with_mode`, `write_file_with_mode`, `atomic_rename_over_impl`) are leaf I/O operations — not "sync versions" of anything. The `blocking::` module already wraps async via `block_in_place` and is unchanged. Public async functions come first in the file; private helpers follow. + +**Tech Stack:** Rust, tokio (`spawn_blocking`), `#[cfg(unix)]`/`#[cfg(windows)]` platform gates. + +--- + +### Task 1: Restructure `write_atomic_restricted` as primary implementation + +**Files:** +- Modify: `crates/hm-util/src/os/fs.rs` + +**Step 1: Replace the entire file with the async-first structure** + +Replace all of `crates/hm-util/src/os/fs.rs` with: + +```rust +//! Atomic, permission-restricted 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 +//! (e.g. extism `host_fn` callbacks). +//! +//! 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; + +/// Write `contents` to `path` atomically with `file_mode`, ensuring the +/// parent directory exists and is set to `dir_mode`. +/// +/// Internally offloads blocking I/O to [`tokio::task::spawn_blocking`]. +/// +/// # 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_mode`, the +/// tempfile cannot be opened with `file_mode` or written, or the final +/// `rename` over `path` fails. +pub async fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, +) -> io::Result<()> { + let path = path.as_ref().to_owned(); + let contents = contents.as_ref().to_vec(); + tokio::task::spawn_blocking(move || { + let parent = path.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("{} has no parent directory", path.display()), + ) + })?; + + create_dir_with_mode(parent, dir_mode)?; + + 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_mode)?; + + let persist_result = atomic_rename_over_impl(&tmp_path, &path); + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result + }) + .await + .map_err(io::Error::other)? +} + +/// Atomically replace `to` with `from`. +/// +/// On Unix this is a single `rename(2)` call — 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<()> { + 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), + } +} + +// --------------------------------------------------------------------------- +// Platform helpers (private) +// --------------------------------------------------------------------------- + +#[cfg(unix)] +fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + if dir.exists() { + let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; + if current != mode { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; + } + } else { + std::fs::DirBuilder::new() + .recursive(true) + .mode(mode) + .create(dir)?; + } + Ok(()) +} + +#[cfg(not(unix))] +fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { + std::fs::create_dir_all(dir) +} + +#[cfg(unix)] +fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(mode) + .open(path)?; + f.write_all(contents)?; + f.sync_all()?; + Ok(()) +} + +#[cfg(not(unix))] +fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { + std::fs::write(path, contents) +} + +#[cfg(unix)] +fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + std::fs::rename(from, to) +} + +#[cfg(windows)] +fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { + use windows::core::HSTRING; + use windows::Win32::Storage::FileSystem::{ + MoveFileExW, ReplaceFileW, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + REPLACEFILE_IGNORE_MERGE_ERRORS, + }; + + 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)) +} + +// --------------------------------------------------------------------------- +// Blocking wrappers +// --------------------------------------------------------------------------- + +/// 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 (e.g. extism `host_fn` callbacks). +pub mod blocking { + 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_mode`, the + /// tempfile cannot be opened with `file_mode` or written, or the final + /// `rename` over `path` fails. + pub fn write_atomic_restricted( + path: impl AsRef, + contents: impl AsRef<[u8]>, + file_mode: u32, + dir_mode: u32, + ) -> io::Result<()> { + block_on(super::write_atomic_restricted(path, contents, file_mode, dir_mode)) + } + + /// 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, clippy::expect_used, clippy::panic)] +mod tests { + use super::blocking; + use std::os::unix::fs::PermissionsExt; + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn writes_file_and_dir_with_requested_modes() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("creds"); + blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + let dir_mode = std::fs::metadata(target.parent().unwrap()) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!( + file_mode, 0o600, + "file mode must be 0o600, got {file_mode:o}" + ); + assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn overwrites_existing_file_preserving_mode() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("creds"); + blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); + blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"v2"); + let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn tightens_existing_dir_with_looser_mode() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("loose"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let target = dir.join("creds"); + blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); + + let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn remove_if_exists_is_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + blocking::remove_if_exists(&target).unwrap(); + std::fs::write(&target, "x").unwrap(); + blocking::remove_if_exists(&target).unwrap(); + assert!(!target.exists()); + } + + #[tokio::test] + async fn async_write_atomic_restricted() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("sub").join("async_creds"); + super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) + .await + .unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); + let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600); + } + + #[tokio::test] + async fn async_remove_if_exists() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("nothing"); + super::remove_file_if_exists(&target).await.unwrap(); + std::fs::write(&target, "x").unwrap(); + super::remove_file_if_exists(&target).await.unwrap(); + assert!(!target.exists()); + } + + #[tokio::test] + async fn atomic_rename_over_replaces_target() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&dst, b"old").unwrap(); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists(), "source should be gone after rename"); + } + + #[tokio::test] + async fn atomic_rename_over_works_when_target_missing() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists()); + } +} +``` + +**Step 2: Run all tests** + +Run: `cargo test --lib -p hm-util -p harmont-cli` +Expected: All 8 `hm-util` tests + all `harmont-cli` tests pass. No behavior change — only code structure changed. + +**Step 3: Run clippy** + +Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` +Expected: PASS + +**Step 4: Commit** + +```bash +git add crates/hm-util/src/os/fs.rs +git commit -m "refactor(hm-util): async-first fs module, eliminate sync indirection" +``` + +--- + +### What changed and why + +| Before | After | Why | +|--------|-------|-----| +| `write_atomic_restricted_sync` holds all logic | Deleted — logic inlined into async fn's `spawn_blocking` | Async fn IS the implementation | +| `create_dir_with_mode_sync` | `create_dir_with_mode` | Not a "sync version" — just a platform primitive | +| `write_file_with_mode_sync` | `write_file_with_mode` | Same | +| Helpers above public fns | Public async API first, helpers below | Public interface is the main attraction | +| Doc: "async counterpart of blocking" | Doc: blocking is "counterpart of async" | Async is primary | +| `remove_file_if_exists` doc references blocking | Standalone doc, no blocking mention | Already truly async (`tokio::fs`) | + +### What did NOT change + +- `blocking::` module — already wraps async via `block_in_place`, untouched +- `atomic_rename_over` + `atomic_rename_over_impl` — already structured correctly +- `remove_file_if_exists` — already truly async +- All test bodies — identical, same assertions, same coverage +- All external callers (`creds_store.rs`, `config.rs`) — use `blocking::` or async, both unchanged diff --git a/docs/plans/2026-05-23-atomic-rename-over.md b/docs/plans/2026-05-23-atomic-rename-over.md new file mode 100644 index 00000000..c0b9f3d2 --- /dev/null +++ b/docs/plans/2026-05-23-atomic-rename-over.md @@ -0,0 +1,269 @@ +# Atomic Rename-Over Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Add a cross-platform `pub async fn atomic_rename_over` that atomically replaces a target file with a source file, using `ReplaceFileW` on Windows and `rename(2)` on Unix. + +**Architecture:** A private sync function (`atomic_rename_over_sync`) contains platform-specific logic behind `#[cfg]` gates. The public async wrapper offloads it to `spawn_blocking`. On Windows, `ReplaceFileW` is preferred (preserves ACLs/streams); falls back to `MoveFileExW` when the target doesn't exist yet. On Unix, `std::fs::rename` is already atomic. The existing `write_atomic_restricted_sync` is updated to call this instead of raw `std::fs::rename`. Dead code (`remove_file_if_exists` and its sync helper) is removed. + +**Tech Stack:** `windows` crate (0.62, `Win32_Storage_FileSystem` + `Win32_Foundation` features), conditional on `cfg(windows)`. Tokio `spawn_blocking` for async. + +--- + +### Task 1: Add `windows` crate conditional dependency + +**Files:** +- Modify: `crates/hm-util/Cargo.toml` + +**Step 1: Add the conditional dependency** + +Add to `crates/hm-util/Cargo.toml` after the existing `[dependencies]` entries: + +```toml +[target.'cfg(windows)'.dependencies.windows] +version = "0.62" +features = [ + "Win32_Foundation", + "Win32_Storage_FileSystem", +] +``` + +**Step 2: Verify it compiles** + +Run: `cargo check -p hm-util` +Expected: PASS (on macOS/Linux the `windows` dep is ignored; it only activates on Windows targets) + +**Step 3: Commit** + +```bash +git add crates/hm-util/Cargo.toml Cargo.lock +git commit -m "feat(hm-util): add windows crate for atomic file replacement" +``` + +--- + +### Task 2: Implement `atomic_rename_over` with platform backends + +**Files:** +- Modify: `crates/hm-util/src/os/fs.rs` + +**Step 1: Write the failing test** + +Add at the bottom of the existing `#[cfg(all(test, unix))]` test module in `crates/hm-util/src/os/fs.rs`: + +```rust +#[tokio::test] +async fn atomic_rename_over_replaces_target() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&dst, b"old").unwrap(); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists(), "source should be gone after rename"); +} + +#[tokio::test] +async fn atomic_rename_over_works_when_target_missing() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("source"); + let dst = tmp.path().join("target"); + std::fs::write(&src, b"new").unwrap(); + + super::atomic_rename_over(&src, &dst).await.unwrap(); + + assert_eq!(std::fs::read(&dst).unwrap(), b"new"); + assert!(!src.exists()); +} +``` + +**Step 2: Run the tests to verify they fail** + +Run: `cargo test --lib -p hm-util -- atomic_rename_over` +Expected: FAIL — `atomic_rename_over` does not exist yet. + +**Step 3: Implement the Unix sync backend** + +Add the following private sync function in `crates/hm-util/src/os/fs.rs`, after the existing `write_file_with_mode_sync` non-unix variant (around line 89), before the `// Public async API` section: + +```rust +// --------------------------------------------------------------------------- +// Cross-platform atomic rename +// --------------------------------------------------------------------------- + +#[cfg(unix)] +fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { + std::fs::rename(from, to) +} + +#[cfg(windows)] +fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { + use windows::core::HSTRING; + use windows::Win32::Storage::FileSystem::{ + MoveFileExW, ReplaceFileW, + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + REPLACEFILE_IGNORE_MERGE_ERRORS, + }; + + let from_w = HSTRING::from(from.as_os_str()); + let to_w = HSTRING::from(to.as_os_str()); + + // ReplaceFileW preserves ACLs and alternate data streams on the + // target, but requires the target to already exist. + 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)); + } + + // Target doesn't exist yet — fall back to MoveFileExW which handles + // both cases but doesn't preserve target metadata (irrelevant here + // since there is no target). + 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)) +} +``` + +**Step 4: Add the public async wrapper** + +Add this in the "Public async API" section of `crates/hm-util/src/os/fs.rs`, after the existing `write_atomic_restricted` async fn: + +```rust +/// Atomically replace `to` with `from`. +/// +/// On Unix this is a single `rename(2)` call — 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<()> { + let from = from.as_ref().to_owned(); + let to = to.as_ref().to_owned(); + tokio::task::spawn_blocking(move || atomic_rename_over_sync(&from, &to)) + .await + .map_err(io::Error::other)? +} +``` + +**Step 5: Run the tests to verify they pass** + +Run: `cargo test --lib -p hm-util -- atomic_rename_over` +Expected: PASS — both tests should succeed. + +**Step 6: Run clippy** + +Run: `cargo clippy -p hm-util -- -D warnings` +Expected: PASS + +**Step 7: Commit** + +```bash +git add crates/hm-util/src/os/fs.rs +git commit -m "feat(hm-util): add atomic_rename_over with ReplaceFileW on Windows" +``` + +--- + +### Task 3: Wire `atomic_rename_over_sync` into `write_atomic_restricted_sync` + +**Files:** +- Modify: `crates/hm-util/src/os/fs.rs` + +**Step 1: Replace `std::fs::rename` with `atomic_rename_over_sync`** + +In `write_atomic_restricted_sync` (around line 42-46), replace: + +```rust + let persist_result = std::fs::rename(&tmp_path, path); + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result +``` + +with: + +```rust + let persist_result = atomic_rename_over_sync(&tmp_path, path); + if persist_result.is_err() { + let _ = std::fs::remove_file(&tmp_path); + } + persist_result +``` + +**Step 2: Run all tests** + +Run: `cargo test --lib -p hm-util -p harmont-cli` +Expected: All tests pass (the existing `write_atomic_restricted` tests exercise this path). + +**Step 3: Run clippy** + +Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` +Expected: PASS + +**Step 4: Commit** + +```bash +git add crates/hm-util/src/os/fs.rs +git commit -m "refactor(hm-util): use atomic_rename_over_sync in write_atomic_restricted" +``` + +--- + +### Task 4: Remove dead code + +**Files:** +- Modify: `crates/hm-util/src/os/fs.rs` + +**Step 1: Delete `remove_file_if_exists` (async) and its sync helper** + +Remove the `remove_file_if_exists` async function (around lines 118-133 in the current file). There is no sync helper to remove — it uses `tokio::fs::remove_file` directly. But check: there may be a `remove_if_exists_sync` private fn — if it exists and has no callers, remove it too. + +**Step 2: Run tests** + +Run: `cargo test --lib -p hm-util -p harmont-cli` +Expected: All tests pass. If any test references `remove_file_if_exists`, delete that test too (it's testing dead code). + +**Step 3: Run clippy** + +Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` +Expected: PASS + +**Step 4: Commit** + +```bash +git add crates/hm-util/src/os/fs.rs +git commit -m "chore(hm-util): remove unused remove_file_if_exists" +``` + +--- + +### Notes + +- **Windows testing**: CI runs on Linux. The `#[cfg(windows)]` code path cannot be tested there. If Windows CI is added later, the existing `atomic_rename_over_replaces_target` and `atomic_rename_over_works_when_target_missing` tests will exercise the Windows path automatically — they are not gated behind `#[cfg(unix)]`. +- **`host_fns.rs:725`** also uses `std::fs::rename` for plugin KV state persistence. That's inside a sync `host_fn` callback, so it can't call the async version. A follow-up could add a `blocking::atomic_rename_over` wrapper, but that's out of scope for this plan (YAGNI — plugin state is low-stakes compared to credentials). +- **`ReplaceFileW` requires same volume**: both `from` and `to` must be on the same filesystem. This is guaranteed for our use case (temp file is created in the same parent directory as the target). From f1d51a8e487b019ab643510e34fe15ebf103f1f0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 23 May 2026 14:58:11 -0700 Subject: [PATCH 25/25] more cleanup --- docs/plans/2026-05-23-async-first-fs.md | 409 -------------------- docs/plans/2026-05-23-atomic-rename-over.md | 269 ------------- 2 files changed, 678 deletions(-) delete mode 100644 docs/plans/2026-05-23-async-first-fs.md delete mode 100644 docs/plans/2026-05-23-atomic-rename-over.md diff --git a/docs/plans/2026-05-23-async-first-fs.md b/docs/plans/2026-05-23-async-first-fs.md deleted file mode 100644 index ad82c175..00000000 --- a/docs/plans/2026-05-23-async-first-fs.md +++ /dev/null @@ -1,409 +0,0 @@ -# Async-First fs.rs Restructure - -> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. - -**Goal:** Make async functions the primary implementations in `hm-util`'s `fs` module — eliminate the `write_atomic_restricted_sync` indirection, drop `_sync` suffixes from private helpers, fix doc comments that frame async as secondary. - -**Architecture:** The async public functions (`write_atomic_restricted`, `atomic_rename_over`, `remove_file_if_exists`) should contain all logic directly. Private platform helpers (`create_dir_with_mode`, `write_file_with_mode`, `atomic_rename_over_impl`) are leaf I/O operations — not "sync versions" of anything. The `blocking::` module already wraps async via `block_in_place` and is unchanged. Public async functions come first in the file; private helpers follow. - -**Tech Stack:** Rust, tokio (`spawn_blocking`), `#[cfg(unix)]`/`#[cfg(windows)]` platform gates. - ---- - -### Task 1: Restructure `write_atomic_restricted` as primary implementation - -**Files:** -- Modify: `crates/hm-util/src/os/fs.rs` - -**Step 1: Replace the entire file with the async-first structure** - -Replace all of `crates/hm-util/src/os/fs.rs` with: - -```rust -//! Atomic, permission-restricted 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 -//! (e.g. extism `host_fn` callbacks). -//! -//! 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; - -/// Write `contents` to `path` atomically with `file_mode`, ensuring the -/// parent directory exists and is set to `dir_mode`. -/// -/// Internally offloads blocking I/O to [`tokio::task::spawn_blocking`]. -/// -/// # 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_mode`, the -/// tempfile cannot be opened with `file_mode` or written, or the final -/// `rename` over `path` fails. -pub async fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file_mode: u32, - dir_mode: u32, -) -> io::Result<()> { - let path = path.as_ref().to_owned(); - let contents = contents.as_ref().to_vec(); - tokio::task::spawn_blocking(move || { - let parent = path.parent().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - format!("{} has no parent directory", path.display()), - ) - })?; - - create_dir_with_mode(parent, dir_mode)?; - - 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_mode)?; - - let persist_result = atomic_rename_over_impl(&tmp_path, &path); - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); - } - persist_result - }) - .await - .map_err(io::Error::other)? -} - -/// Atomically replace `to` with `from`. -/// -/// On Unix this is a single `rename(2)` call — 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<()> { - 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), - } -} - -// --------------------------------------------------------------------------- -// Platform helpers (private) -// --------------------------------------------------------------------------- - -#[cfg(unix)] -fn create_dir_with_mode(dir: &Path, mode: u32) -> io::Result<()> { - use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; - if dir.exists() { - let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; - if current != mode { - std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; - } - } else { - std::fs::DirBuilder::new() - .recursive(true) - .mode(mode) - .create(dir)?; - } - Ok(()) -} - -#[cfg(not(unix))] -fn create_dir_with_mode(dir: &Path, _mode: u32) -> io::Result<()> { - std::fs::create_dir_all(dir) -} - -#[cfg(unix)] -fn write_file_with_mode(path: &Path, contents: &[u8], mode: u32) -> io::Result<()> { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut f = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(mode) - .open(path)?; - f.write_all(contents)?; - f.sync_all()?; - Ok(()) -} - -#[cfg(not(unix))] -fn write_file_with_mode(path: &Path, contents: &[u8], _mode: u32) -> io::Result<()> { - std::fs::write(path, contents) -} - -#[cfg(unix)] -fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { - std::fs::rename(from, to) -} - -#[cfg(windows)] -fn atomic_rename_over_impl(from: &Path, to: &Path) -> io::Result<()> { - use windows::core::HSTRING; - use windows::Win32::Storage::FileSystem::{ - MoveFileExW, ReplaceFileW, - MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, - REPLACEFILE_IGNORE_MERGE_ERRORS, - }; - - 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)) -} - -// --------------------------------------------------------------------------- -// Blocking wrappers -// --------------------------------------------------------------------------- - -/// 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 (e.g. extism `host_fn` callbacks). -pub mod blocking { - 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_mode`, the - /// tempfile cannot be opened with `file_mode` or written, or the final - /// `rename` over `path` fails. - pub fn write_atomic_restricted( - path: impl AsRef, - contents: impl AsRef<[u8]>, - file_mode: u32, - dir_mode: u32, - ) -> io::Result<()> { - block_on(super::write_atomic_restricted(path, contents, file_mode, dir_mode)) - } - - /// 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, clippy::expect_used, clippy::panic)] -mod tests { - use super::blocking; - use std::os::unix::fs::PermissionsExt; - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn writes_file_and_dir_with_requested_modes() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("creds"); - blocking::write_atomic_restricted(&target, b"hello", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - let dir_mode = std::fs::metadata(target.parent().unwrap()) - .unwrap() - .permissions() - .mode() - & 0o777; - assert_eq!( - file_mode, 0o600, - "file mode must be 0o600, got {file_mode:o}" - ); - assert_eq!(dir_mode, 0o700, "dir mode must be 0o700, got {dir_mode:o}"); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn overwrites_existing_file_preserving_mode() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("creds"); - blocking::write_atomic_restricted(&target, b"v1", 0o600, 0o700).unwrap(); - blocking::write_atomic_restricted(&target, b"v2", 0o600, 0o700).unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"v2"); - let mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn tightens_existing_dir_with_looser_mode() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("loose"); - std::fs::create_dir(&dir).unwrap(); - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); - - let target = dir.join("creds"); - blocking::write_atomic_restricted(&target, b"x", 0o600, 0o700).unwrap(); - - let dir_mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; - assert_eq!(dir_mode, 0o700); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn remove_if_exists_is_idempotent() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - blocking::remove_if_exists(&target).unwrap(); - std::fs::write(&target, "x").unwrap(); - blocking::remove_if_exists(&target).unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn async_write_atomic_restricted() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("sub").join("async_creds"); - super::write_atomic_restricted(&target, b"async hello", 0o600, 0o700) - .await - .unwrap(); - - assert_eq!(std::fs::read(&target).unwrap(), b"async hello"); - let file_mode = std::fs::metadata(&target).unwrap().permissions().mode() & 0o777; - assert_eq!(file_mode, 0o600); - } - - #[tokio::test] - async fn async_remove_if_exists() { - let tmp = tempfile::tempdir().unwrap(); - let target = tmp.path().join("nothing"); - super::remove_file_if_exists(&target).await.unwrap(); - std::fs::write(&target, "x").unwrap(); - super::remove_file_if_exists(&target).await.unwrap(); - assert!(!target.exists()); - } - - #[tokio::test] - async fn atomic_rename_over_replaces_target() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&dst, b"old").unwrap(); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists(), "source should be gone after rename"); - } - - #[tokio::test] - async fn atomic_rename_over_works_when_target_missing() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists()); - } -} -``` - -**Step 2: Run all tests** - -Run: `cargo test --lib -p hm-util -p harmont-cli` -Expected: All 8 `hm-util` tests + all `harmont-cli` tests pass. No behavior change — only code structure changed. - -**Step 3: Run clippy** - -Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` -Expected: PASS - -**Step 4: Commit** - -```bash -git add crates/hm-util/src/os/fs.rs -git commit -m "refactor(hm-util): async-first fs module, eliminate sync indirection" -``` - ---- - -### What changed and why - -| Before | After | Why | -|--------|-------|-----| -| `write_atomic_restricted_sync` holds all logic | Deleted — logic inlined into async fn's `spawn_blocking` | Async fn IS the implementation | -| `create_dir_with_mode_sync` | `create_dir_with_mode` | Not a "sync version" — just a platform primitive | -| `write_file_with_mode_sync` | `write_file_with_mode` | Same | -| Helpers above public fns | Public async API first, helpers below | Public interface is the main attraction | -| Doc: "async counterpart of blocking" | Doc: blocking is "counterpart of async" | Async is primary | -| `remove_file_if_exists` doc references blocking | Standalone doc, no blocking mention | Already truly async (`tokio::fs`) | - -### What did NOT change - -- `blocking::` module — already wraps async via `block_in_place`, untouched -- `atomic_rename_over` + `atomic_rename_over_impl` — already structured correctly -- `remove_file_if_exists` — already truly async -- All test bodies — identical, same assertions, same coverage -- All external callers (`creds_store.rs`, `config.rs`) — use `blocking::` or async, both unchanged diff --git a/docs/plans/2026-05-23-atomic-rename-over.md b/docs/plans/2026-05-23-atomic-rename-over.md deleted file mode 100644 index c0b9f3d2..00000000 --- a/docs/plans/2026-05-23-atomic-rename-over.md +++ /dev/null @@ -1,269 +0,0 @@ -# Atomic Rename-Over Implementation Plan - -> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. - -**Goal:** Add a cross-platform `pub async fn atomic_rename_over` that atomically replaces a target file with a source file, using `ReplaceFileW` on Windows and `rename(2)` on Unix. - -**Architecture:** A private sync function (`atomic_rename_over_sync`) contains platform-specific logic behind `#[cfg]` gates. The public async wrapper offloads it to `spawn_blocking`. On Windows, `ReplaceFileW` is preferred (preserves ACLs/streams); falls back to `MoveFileExW` when the target doesn't exist yet. On Unix, `std::fs::rename` is already atomic. The existing `write_atomic_restricted_sync` is updated to call this instead of raw `std::fs::rename`. Dead code (`remove_file_if_exists` and its sync helper) is removed. - -**Tech Stack:** `windows` crate (0.62, `Win32_Storage_FileSystem` + `Win32_Foundation` features), conditional on `cfg(windows)`. Tokio `spawn_blocking` for async. - ---- - -### Task 1: Add `windows` crate conditional dependency - -**Files:** -- Modify: `crates/hm-util/Cargo.toml` - -**Step 1: Add the conditional dependency** - -Add to `crates/hm-util/Cargo.toml` after the existing `[dependencies]` entries: - -```toml -[target.'cfg(windows)'.dependencies.windows] -version = "0.62" -features = [ - "Win32_Foundation", - "Win32_Storage_FileSystem", -] -``` - -**Step 2: Verify it compiles** - -Run: `cargo check -p hm-util` -Expected: PASS (on macOS/Linux the `windows` dep is ignored; it only activates on Windows targets) - -**Step 3: Commit** - -```bash -git add crates/hm-util/Cargo.toml Cargo.lock -git commit -m "feat(hm-util): add windows crate for atomic file replacement" -``` - ---- - -### Task 2: Implement `atomic_rename_over` with platform backends - -**Files:** -- Modify: `crates/hm-util/src/os/fs.rs` - -**Step 1: Write the failing test** - -Add at the bottom of the existing `#[cfg(all(test, unix))]` test module in `crates/hm-util/src/os/fs.rs`: - -```rust -#[tokio::test] -async fn atomic_rename_over_replaces_target() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&dst, b"old").unwrap(); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists(), "source should be gone after rename"); -} - -#[tokio::test] -async fn atomic_rename_over_works_when_target_missing() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("source"); - let dst = tmp.path().join("target"); - std::fs::write(&src, b"new").unwrap(); - - super::atomic_rename_over(&src, &dst).await.unwrap(); - - assert_eq!(std::fs::read(&dst).unwrap(), b"new"); - assert!(!src.exists()); -} -``` - -**Step 2: Run the tests to verify they fail** - -Run: `cargo test --lib -p hm-util -- atomic_rename_over` -Expected: FAIL — `atomic_rename_over` does not exist yet. - -**Step 3: Implement the Unix sync backend** - -Add the following private sync function in `crates/hm-util/src/os/fs.rs`, after the existing `write_file_with_mode_sync` non-unix variant (around line 89), before the `// Public async API` section: - -```rust -// --------------------------------------------------------------------------- -// Cross-platform atomic rename -// --------------------------------------------------------------------------- - -#[cfg(unix)] -fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { - std::fs::rename(from, to) -} - -#[cfg(windows)] -fn atomic_rename_over_sync(from: &Path, to: &Path) -> io::Result<()> { - use windows::core::HSTRING; - use windows::Win32::Storage::FileSystem::{ - MoveFileExW, ReplaceFileW, - MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, - REPLACEFILE_IGNORE_MERGE_ERRORS, - }; - - let from_w = HSTRING::from(from.as_os_str()); - let to_w = HSTRING::from(to.as_os_str()); - - // ReplaceFileW preserves ACLs and alternate data streams on the - // target, but requires the target to already exist. - 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)); - } - - // Target doesn't exist yet — fall back to MoveFileExW which handles - // both cases but doesn't preserve target metadata (irrelevant here - // since there is no target). - 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)) -} -``` - -**Step 4: Add the public async wrapper** - -Add this in the "Public async API" section of `crates/hm-util/src/os/fs.rs`, after the existing `write_atomic_restricted` async fn: - -```rust -/// Atomically replace `to` with `from`. -/// -/// On Unix this is a single `rename(2)` call — 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<()> { - let from = from.as_ref().to_owned(); - let to = to.as_ref().to_owned(); - tokio::task::spawn_blocking(move || atomic_rename_over_sync(&from, &to)) - .await - .map_err(io::Error::other)? -} -``` - -**Step 5: Run the tests to verify they pass** - -Run: `cargo test --lib -p hm-util -- atomic_rename_over` -Expected: PASS — both tests should succeed. - -**Step 6: Run clippy** - -Run: `cargo clippy -p hm-util -- -D warnings` -Expected: PASS - -**Step 7: Commit** - -```bash -git add crates/hm-util/src/os/fs.rs -git commit -m "feat(hm-util): add atomic_rename_over with ReplaceFileW on Windows" -``` - ---- - -### Task 3: Wire `atomic_rename_over_sync` into `write_atomic_restricted_sync` - -**Files:** -- Modify: `crates/hm-util/src/os/fs.rs` - -**Step 1: Replace `std::fs::rename` with `atomic_rename_over_sync`** - -In `write_atomic_restricted_sync` (around line 42-46), replace: - -```rust - let persist_result = std::fs::rename(&tmp_path, path); - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); - } - persist_result -``` - -with: - -```rust - let persist_result = atomic_rename_over_sync(&tmp_path, path); - if persist_result.is_err() { - let _ = std::fs::remove_file(&tmp_path); - } - persist_result -``` - -**Step 2: Run all tests** - -Run: `cargo test --lib -p hm-util -p harmont-cli` -Expected: All tests pass (the existing `write_atomic_restricted` tests exercise this path). - -**Step 3: Run clippy** - -Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` -Expected: PASS - -**Step 4: Commit** - -```bash -git add crates/hm-util/src/os/fs.rs -git commit -m "refactor(hm-util): use atomic_rename_over_sync in write_atomic_restricted" -``` - ---- - -### Task 4: Remove dead code - -**Files:** -- Modify: `crates/hm-util/src/os/fs.rs` - -**Step 1: Delete `remove_file_if_exists` (async) and its sync helper** - -Remove the `remove_file_if_exists` async function (around lines 118-133 in the current file). There is no sync helper to remove — it uses `tokio::fs::remove_file` directly. But check: there may be a `remove_if_exists_sync` private fn — if it exists and has no callers, remove it too. - -**Step 2: Run tests** - -Run: `cargo test --lib -p hm-util -p harmont-cli` -Expected: All tests pass. If any test references `remove_file_if_exists`, delete that test too (it's testing dead code). - -**Step 3: Run clippy** - -Run: `cargo clippy -p hm-util -p harmont-cli -- -D warnings` -Expected: PASS - -**Step 4: Commit** - -```bash -git add crates/hm-util/src/os/fs.rs -git commit -m "chore(hm-util): remove unused remove_file_if_exists" -``` - ---- - -### Notes - -- **Windows testing**: CI runs on Linux. The `#[cfg(windows)]` code path cannot be tested there. If Windows CI is added later, the existing `atomic_rename_over_replaces_target` and `atomic_rename_over_works_when_target_missing` tests will exercise the Windows path automatically — they are not gated behind `#[cfg(unix)]`. -- **`host_fns.rs:725`** also uses `std::fs::rename` for plugin KV state persistence. That's inside a sync `host_fn` callback, so it can't call the async version. A follow-up could add a `blocking::atomic_rename_over` wrapper, but that's out of scope for this plan (YAGNI — plugin state is low-stakes compared to credentials). -- **`ReplaceFileW` requires same volume**: both `from` and `to` must be on the same filesystem. This is guaranteed for our use case (temp file is created in the same parent directory as the target).