From 291b80eee228a157eb86d5865704749ae8c7f175 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:00:02 -0700 Subject: [PATCH 1/3] refactor(workspace): unify root resolution behind Workspace::resolve Three verbs each hand-rolled the same root resolution: let root = match args.dir { Some(d) => d, None => current_dir()? }; in cli/render.rs, cli/pipelines.rs, and commands/run/mod.rs. None of them consulted a Workspace. Replace all three with Workspace::resolve, which owns the single rule: - `--dir` names the project root verbatim, with no walk-up. The backend runs discovery against cloned repo roots, and walking up out of a clone could bind to an unrelated `.hm/` above it. A relative `--dir` is resolved against the cwd, since a workspace root must be absolute. - No `--dir`: walk up from the cwd, so a verb works from any subdirectory. Fixes a divergence in `hm run`. render_pipeline took a &RunContext holding a fully-validated Workspace, ignored it (the parameter was named `_ctx`), and recomputed the root from `--dir`/cwd. That PathBuf became req.repo_root and propagated into the scheduler, the archive builder, the secret resolver and the cloud backend -- so with `--dir`, config came from one tree and code from another. RunContext::from_cli now takes the verb's `--dir` and resolves once; render_pipeline uses ctx.workspace.path(). As a side effect `hm run --dir X` now works from a cwd outside any workspace. It previously failed with "no harmont workspace found" because from_cli walked up from the cwd and ignored `--dir` entirely. Behaviour change: `hm render` and `hm pipelines` now walk up when given no `--dir`, where they were previously cwd-exact and failed in subdirectories. Discovery is unaffected -- the backend always passes `--dir`. `hm pipelines` keeps its contract that a repo with no `.hm/` yields the empty envelope rather than an error. Only NotFound/InvalidPath/InvalidWorkspace map to it; a malformed `.hm/config.toml` still fails loudly instead of masquerading as a repo with no pipelines. CurrentDir and NotFound move onto WorkspaceLoadError, next to the other ways resolving a root can fail. context::Error is dropped: it had become a single-variant passthrough. --- crates/hm-core/workspace.rs | 54 ++++++++++++++++++++++++++++--- crates/hm/src/cli/mod.rs | 2 +- crates/hm/src/cli/pipelines.rs | 15 +++++++-- crates/hm/src/cli/render.rs | 6 ++-- crates/hm/src/commands/run/mod.rs | 10 +++--- crates/hm/src/context.rs | 45 +++++++------------------- 6 files changed, 81 insertions(+), 51 deletions(-) diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs index 17c57e86..23603b90 100644 --- a/crates/hm-core/workspace.rs +++ b/crates/hm-core/workspace.rs @@ -1,28 +1,42 @@ //! Global utility for managing the workspace. +use std::io; use std::path::{Path, PathBuf}; use hm_config::Config; use hm_util::path::{AbsPath, AbsPathBuf}; use thiserror::Error; -/// Failure loading a directory as a [`Workspace`]. +/// Failure resolving or loading a directory as a [`Workspace`]. #[derive(Debug, Error)] pub enum LoadError { #[error("{} does not appear to be a valid path", .0.display())] InvalidPath(PathBuf), #[error("{} is not a harmont workspace. harmont workspaces must have a `.hm/` directory", .0.display())] InvalidWorkspace(PathBuf), + + /// The process working directory could not be read while resolving a root. + #[error("cannot determine current directory")] + CurrentDir(#[source] io::Error), + + /// Walked to the filesystem root without finding a `.hm/` directory. + #[error( + "no harmont workspace found\n → run from a directory that contains `.hm/`, or initialize one with `hm init`" + )] + NotFound, + #[error(transparent)] Config(#[from] hm_config::LoadError), } /// Utility for accessing well-known paths inside a workspace. /// -/// Construct with [`Workspace::load`] on a **specific** project root (the -/// directory that contains `.hm/`). Callers that need parent-directory -/// discovery should walk first (e.g. [`hm_util::dirs::find_project_root`]) -/// and only then call [`Workspace::load`]. +/// Two constructors: +/// +/// - [`Workspace::resolve`] — the one CLI verbs want. Honors a `--dir`-style +/// override, else walks up from the cwd. +/// - [`Workspace::load`] — validates one **specific** project root (the +/// directory that contains `.hm/`), with no discovery. #[derive(Debug)] pub struct Workspace { /// Absolute path to the `.hm` directory. @@ -37,6 +51,36 @@ pub struct Workspace { } impl Workspace { + /// Resolve the workspace a command should operate on. + /// + /// The single root-resolution rule for every verb that takes a `--dir`: + /// + /// - **`dir` given** — that directory is the project root, verbatim. No + /// walk-up: the backend runs discovery against cloned repo roots, and + /// walking up out of a clone could bind to an unrelated `.hm/` above it. + /// A relative path is resolved against the cwd, since a workspace root + /// must be absolute. + /// - **`dir` absent** — walk up from the cwd, so a verb works from any + /// subdirectory of the project. + /// + /// # Errors + /// + /// Returns [`LoadError`] if the cwd cannot be determined, no project root + /// is found while walking up, or the resolved root fails [`Self::load`]. + pub fn resolve(dir: Option<&Path>) -> Result { + let root = match dir { + Some(d) if d.is_absolute() => d.to_path_buf(), + Some(d) => std::env::current_dir() + .map_err(LoadError::CurrentDir)? + .join(d), + None => { + let start = std::env::current_dir().map_err(LoadError::CurrentDir)?; + hm_util::dirs::find_project_root(&start).ok_or(LoadError::NotFound)? + } + }; + Self::load(&root) + } + /// Attempt to load the given directory as a workspace, if it appears to be one. /// /// We label a workspace any directory which has a `.hm` directory within it. diff --git a/crates/hm/src/cli/mod.rs b/crates/hm/src/cli/mod.rs index b1cd2875..a13efa62 100644 --- a/crates/hm/src/cli/mod.rs +++ b/crates/hm/src/cli/mod.rs @@ -107,7 +107,7 @@ pub async fn dispatch(command: Command, cli: &Cli) -> Result { match command { Command::Init(args) => crate::commands::init::handle(args).await.map(|()| 0), Command::Run(args) => { - let ctx = RunContext::from_cli(cli)?; + let ctx = RunContext::from_cli(cli, args.dir.as_deref())?; crate::commands::run::handle(args, ctx).await } Command::Pipelines(args) => crate::cli::pipelines::run(args).await.map(|()| 0), diff --git a/crates/hm/src/cli/pipelines.rs b/crates/hm/src/cli/pipelines.rs index 5ee250ee..223ace30 100644 --- a/crates/hm/src/cli/pipelines.rs +++ b/crates/hm/src/cli/pipelines.rs @@ -28,10 +28,19 @@ const EMPTY_ENVELOPE: &str = r#"{"schema_version":"1","pipelines":[]}"#; /// Returns an error if the engine can't start or the DSL runtime fails to /// evaluate the pipelines. pub async fn run(args: PipelinesArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, + use hm_core::WorkspaceLoadError as WsErr; + let workspace = match hm_core::Workspace::resolve(args.dir.as_deref()) { + Ok(w) => w, + // "Not a harmont project" is not an error here — it means this repo + // declares no pipelines. A malformed `.hm/config.toml` still fails + // loudly rather than masquerading as an empty repo. + Err(WsErr::NotFound | WsErr::InvalidPath(_) | WsErr::InvalidWorkspace(_)) => { + print!("{EMPTY_ENVELOPE}"); + return Ok(()); + } + Err(e) => return Err(e.into()), }; + let repo_root = workspace.path().as_path().to_path_buf(); if !detect::has_pipeline_files(&repo_root) { print!("{EMPTY_ENVELOPE}"); diff --git a/crates/hm/src/cli/render.rs b/crates/hm/src/cli/render.rs index cc10ca99..6b45f15b 100644 --- a/crates/hm/src/cli/render.rs +++ b/crates/hm/src/cli/render.rs @@ -26,10 +26,8 @@ pub struct RenderArgs { /// or the slug is unknown / fails to render (the available slugs are written to /// stderr by the DSL runtime). pub async fn run(args: RenderArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + let workspace = hm_core::Workspace::resolve(args.dir.as_deref())?; + let repo_root = workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).context("detecting pipeline language")?; let engine = python_engine().context("initializing DSL engine")?; diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index d89427dc..a83265a1 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -344,12 +344,12 @@ fn git_remote_repo_name(root: &std::path::Path) -> Option { /// the DSL detection / pipeline-render step fails. async fn render_pipeline( args: &RunArgs, - _ctx: &RunContext, + ctx: &RunContext, ) -> Result<(std::path::PathBuf, String, String)> { - let repo_root = match args.dir.clone() { - Some(p) => p, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + // The root the run executes against is the workspace's, not a second + // resolution from `--dir`/cwd — otherwise config could come from one tree + // and code from another. `RunContext` already resolved `--dir`. + let repo_root = ctx.workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).map_err(|e| HmError::DslEngine(format!("{e:#}")))?; let engine = diff --git a/crates/hm/src/context.rs b/crates/hm/src/context.rs index 95fc6a47..71c6bcb4 100644 --- a/crates/hm/src/context.rs +++ b/crates/hm/src/context.rs @@ -1,27 +1,9 @@ use std::io::IsTerminal; +use std::path::Path; use crate::cli::Cli; -use hm_core::Workspace; +use hm_core::{Workspace, WorkspaceLoadError}; use hm_render::OutputMode; -use thiserror::Error; - -/// Failure building a [`RunContext`]. -#[derive(Debug, Error)] -pub enum Error { - /// Process current directory could not be resolved. - #[error("cannot determine current directory")] - CurrentDir(#[source] std::io::Error), - - /// No directory containing `.hm/` was found above the process cwd. - #[error( - "no harmont workspace found\n → run from a directory that contains `.hm/`, or initialize one with `hm init`" - )] - NotFound, - - /// A discovered project root failed [`Workspace::load`]. - #[error(transparent)] - Workspace(#[from] hm_core::WorkspaceLoadError), -} /// Runtime context for commands that operate on a harmont project workspace. /// @@ -40,23 +22,20 @@ pub struct RunContext { } impl RunContext { - /// Build a [`RunContext`] from parsed CLI args. + /// Build a [`RunContext`] from parsed CLI args and the verb's `--dir`. /// - /// Discovers the project root by walking up from the current directory - /// ([`hm_util::dirs::find_project_root`]), then loads a [`Workspace`] for - /// that exact path. Walk-up is intentionally separate from - /// [`Workspace::load`]. + /// Root resolution is [`Workspace::resolve`]'s, shared with every other + /// `--dir` verb. Passing `dir` through matters: the workspace this loads is + /// the one the run executes against, so resolving it from the cwd while the + /// run used `--dir` would let config come from one tree and code from + /// another. /// /// # Errors /// - /// Returns [`Error`] if the current directory cannot be determined, no - /// project root is found, or workspace load fails. - pub fn from_cli(cli: &Cli) -> Result { - let start_dir = std::env::current_dir().map_err(Error::CurrentDir)?; - // Walk-up is separate from Workspace::load (which only validates the - // exact path). - let root = hm_util::dirs::find_project_root(&start_dir).ok_or(Error::NotFound)?; - let workspace = Workspace::load(&root)?; + /// Returns [`WorkspaceLoadError`] if the current directory cannot be + /// determined, no project root is found, or workspace load fails. + pub fn from_cli(cli: &Cli, dir: Option<&Path>) -> Result { + let workspace = Workspace::resolve(dir)?; let output = OutputMode::Human { // Single source of truth for the color/TTY rule (still honors --no-color). From 14c2b2f32fccba2540997742ee7d99ee69b06740 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:00:14 -0700 Subject: [PATCH 2/3] fix(cloud): read the project .hm/config.toml layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cloud verbs called Config::load(None) -- user and env layers only -- so `[cloud] org` set in a project's .hm/config.toml was silently ignored. The error message users hit told them to set it in exactly the file being ignored: no organization — set `[cloud] org = "…"` in ~/.config/hm/config.toml (or .hm/config.toml), or run `hm cloud org switch ` `hm run` read the project layer correctly via ctx.workspace.config(), so the two halves of the CLI disagreed about the active org in the same directory. settings::config() now walks up from the cwd and passes the project config path to Config::load, which already takes an Option<&Path> for precisely this. Outside a project there is simply no project layer and the cloud verbs still work from anywhere. `hm cloud org switch` deliberately keeps Config::load(None): it is a load-mutate-save on the user file, and merging the project layer in first would persist project-scoped values into ~/.config/hm/config.toml. Commented in place so it isn't "fixed" later. Note a pre-existing wrinkle left alone here: Config::load also merges the env layer, so `HM_API_URL=x hm cloud org switch y` still writes api_url=x into the user config. --- Cargo.lock | 1 + crates/hm-plugin-cloud/Cargo.toml | 1 + crates/hm-plugin-cloud/src/settings.rs | 24 +++++++++++++++++++++--- crates/hm-plugin-cloud/src/verbs/org.rs | 3 +++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ff190e07..ad7170a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1335,6 +1335,7 @@ dependencies = [ "hm-exec", "hm-plugin-protocol", "hm-render", + "hm-util", "percent-encoding", "reqwest", "secrecy", diff --git a/crates/hm-plugin-cloud/Cargo.toml b/crates/hm-plugin-cloud/Cargo.toml index 65e35fa7..9c696ceb 100644 --- a/crates/hm-plugin-cloud/Cargo.toml +++ b/crates/hm-plugin-cloud/Cargo.toml @@ -19,6 +19,7 @@ hm-exec = { workspace = true } hm-render = { workspace = true } hm-config = { workspace = true } hm-core = { workspace = true } +hm-util = { workspace = true } hm-plugin-protocol = { workspace = true } serde_json = { workspace = true } chrono = { workspace = true } diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index 598a79da..8018f383 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -15,6 +15,24 @@ use anyhow::{Context, Result}; use harmont_cloud::HarmontClient; use secrecy::{ExposeSecret, SecretString}; +/// The layered config for the current directory. +/// +/// Includes the project `.hm/config.toml` layer whenever the cwd is inside a +/// harmont project — [`ResolvedCtx::org`]'s own error text tells users to set +/// `[cloud] org` there, so it has to be read. Outside a project there simply is +/// no project layer, and the cloud verbs still work from anywhere. +/// +/// **Read-only callers only.** `hm cloud org switch` deliberately loads the user +/// layer alone: it saves back to the user file, and merging the project layer in +/// first would persist project-scoped values into `~/.config/hm/config.toml`. +fn config() -> Result { + let project = std::env::current_dir() + .ok() + .and_then(|cwd| hm_util::dirs::find_project_root(&cwd)) + .map(|root| hm_config::Config::project_config_path(&root)); + hm_config::Config::load(project.as_deref()).context("loading config") +} + /// Resolve the bearer token, or the shared "not logged in" error. /// /// Every authenticated path routes through here so the not-logged-in text @@ -56,7 +74,7 @@ impl ResolvedCtx { /// /// Returns an error if config can't be loaded or no token is available. pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { - let cfg = hm_config::Config::load(None).context("loading config")?; // user + env layering + let cfg = config()?; // user + project + env layering let api = cfg.cloud.api_url.clone(); let client = HarmontClient::with_base_url(token()?.expose_secret(), &api); Ok(( @@ -81,7 +99,7 @@ pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { /// Returns an error if config can't be loaded, no token is available, or no /// organization is configured. pub fn raw_org_ctx() -> Result<(String, SecretString, String)> { - let cfg = hm_config::Config::load(None).context("loading config")?; + let cfg = config()?; let api = cfg.cloud.api_url.clone(); let token = token()?; let org = ResolvedCtx { @@ -98,7 +116,7 @@ pub fn raw_org_ctx() -> Result<(String, SecretString, String)> { /// /// Returns an error if config can't be loaded. pub fn anon_client() -> Result<(HarmontClient, String)> { - let cfg = hm_config::Config::load(None).context("loading config")?; + let cfg = config()?; let api = cfg.cloud.api_url.clone(); Ok((HarmontClient::anonymous(&api), api)) } diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index 6d9f76fe..cdf3708d 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -27,6 +27,9 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .iter() .find(|o| o.slug == slug) .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; + // Load-mutate-save on the *user* config, so this deliberately does NOT use + // `settings::config()`: merging the project `.hm/config.toml` layer in first + // would persist project-scoped values into ~/.config/hm/config.toml. let mut cfg = hm_config::Config::load(None)?; cfg.cloud.org = Some(found.slug.clone()); cfg.save_user().context("saving config")?; From 55201838ef494e773acea985353df7e619bc57d0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 15 Jul 2026 22:00:24 -0700 Subject: [PATCH 3/3] fix(cloud): stop escaping RFC 3986 unreserved chars in secret names 0d2256d replaced the hand-listed SEGMENT AsciiSet with NON_ALPHANUMERIC, which also escapes `-`, `.`, `_` and `~`. Those are RFC 3986 unreserved and must pass through: `hm cloud secret rm DEPLOY_TOKEN` was requesting /secrets/DEPLOY%5FTOKEN, a different resource than the user named. A name like DEPLOY-TOKEN_v2.0~rc was mangled four ways. Keep that commit's win -- 30 hand-listed bytes are gone -- by deriving the set instead of enumerating it: const SEGMENT: &AsciiSet = &NON_ALPHANUMERIC .remove(b'-').remove(b'.').remove(b'_').remove(b'~'); This is RFC 3986's unreserved set by construction rather than by a list that has to be audited. `/` and space still escape, so a name cannot break out of its path segment. This also unbreaks the test suite, which has been red since 0d2256d. That commit renamed unreserved_name_is_not_encoded to special_chars_are_encoded and flipped its expectation to the escaped output, dropping the comment "RFC 3986 unreserved chars (incl. `-` `_` `.` `~`) must pass through" -- the bug was written down as the spec. It left item_path_appends_encoded_name asserting the opposite, so the two tests contradicted each other and the suite could not pass either way. Restore the original test under its original name, looping over the characters rather than asserting one string -- a single case is what let `_` be noticed while `-.~` broke silently. Add reserved_chars_are_encoded to pin the other half of the contract. --- crates/hm-plugin-cloud/src/verbs/secret.rs | 51 +++++++++++++++++++--- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/crates/hm-plugin-cloud/src/verbs/secret.rs b/crates/hm-plugin-cloud/src/verbs/secret.rs index 9a388d14..5af4cd3f 100644 --- a/crates/hm-plugin-cloud/src/verbs/secret.rs +++ b/crates/hm-plugin-cloud/src/verbs/secret.rs @@ -17,11 +17,27 @@ use std::io::Read; use std::path::Path; use anyhow::{Context, Result, bail}; -use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; +use percent_encoding::{AsciiSet, NON_ALPHANUMERIC, utf8_percent_encode}; use secrecy::{ExposeSecret, SecretString}; use crate::cli::SecretCommand; +/// Characters to percent-encode in a single URL path segment. +/// +/// RFC 3986 leaves the *unreserved* set (`A-Z a-z 0-9 - . _ ~`) safe in a path +/// segment; everything else is escaped. `NON_ALPHANUMERIC` escapes all four of +/// `-._~` too, so they are removed here — secret names are conventionally +/// `[A-Za-z0-9_]`, and encoding `_` as `%5F` requests a different resource than +/// the one the user named. +/// +/// What must NOT be removed: `/` and space, which have to survive as `%2F` / +/// `%20` so a name like `a/b c` cannot escape its scope. +const SEGMENT: &AsciiSet = &NON_ALPHANUMERIC + .remove(b'-') + .remove(b'.') + .remove(b'_') + .remove(b'~'); + /// Where the secret value comes from on `set`. /// /// Exactly one source is allowed; the resolver rejects none and ambiguity. @@ -47,7 +63,7 @@ fn secrets_path(org: &str, pipeline: Option<&str>) -> String { /// Append a URL-path-escaped secret name to a collection path. fn secret_item_path(collection: &str, name: &str) -> String { - format!("{collection}/{}", utf8_percent_encode(name, NON_ALPHANUMERIC)) + format!("{collection}/{}", utf8_percent_encode(name, SEGMENT)) } /// Resolve the secret value from exactly one of: positional `VALUE`, @@ -318,6 +334,29 @@ mod tests { ); } + #[test] + fn unreserved_name_is_not_encoded() { + // RFC 3986 unreserved chars (incl. `-` `_` `.` `~`) must pass through. + // Escaping any of them requests a different resource than the user + // named. Pinned per-character: encoding these has regressed once + // already, via a blanket `NON_ALPHANUMERIC`. + let coll = secrets_path("acme", None); + for name in [ + "DEPLOY-TOKEN_v2.0~rc", + "DEPLOY_TOKEN", + "a-b", + "a.b", + "a~b", + "A-Za-z0-9-._~", + ] { + assert_eq!( + secret_item_path(&coll, name), + format!("/api/v0/organizations/acme/secrets/{name}"), + "unreserved char was escaped in {name:?}" + ); + } + } + #[test] fn item_path_escapes_funny_names() { let coll = secrets_path("acme", Some("ci")); @@ -432,11 +471,13 @@ mod tests { } #[test] - fn special_chars_are_encoded() { + fn reserved_chars_are_encoded() { + // The other half of the contract: anything outside the unreserved set + // must escape, so a name cannot break out of its path segment. let coll = secrets_path("acme", None); assert_eq!( - secret_item_path(&coll, "DEPLOY-TOKEN_v2.0~rc"), - "/api/v0/organizations/acme/secrets/DEPLOY%2DTOKEN%5Fv2%2E0%7Erc" + secret_item_path(&coll, "a/b c?d#e%f"), + "/api/v0/organizations/acme/secrets/a%2Fb%20c%3Fd%23e%25f" ); }