refactor: hm-core (credentials + workspace), typed paths, unified root resolution#168
Open
markovejnovic wants to merge 5 commits into
Open
refactor: hm-core (credentials + workspace), typed paths, unified root resolution#168markovejnovic wants to merge 5 commits into
markovejnovic wants to merge 5 commits into
Conversation
Introduce hm-core as the home for process-level state, and move the
credential store into it as Sys::creds.
The store collapses from a two-level (service, account) map to a single
token. The cloud issues one user-scoped bearer -- claim_token / redeem_code
take no org, and that one token lists every organization the user belongs to
-- so keying it by anything was storing one secret under a key that could not
discriminate it. A second API base (localhost, staging) is served by
HM_API_TOKEN, which is how the integration tests already inject one, so the
stored file only ever held a single entry in practice.
This is a breaking change to ~/.config/hm/credentials.toml. The old nested
shape no longer parses and there is no migration; existing logins must re-run
`hm cloud login`.
Behaviour changes beyond the move:
- The file must be owned by the invoking user and chmod 0600, else load
fails. Previously a malformed or insecure file was silently ignored.
- Tokens are held as secrecy::SecretString rather than String, so they no
longer leak through Debug. They are exposed only where a str is required:
HarmontClient::with_base_url.
- HM_API_TOKEN="" is treated as unset. Honouring it sent an empty bearer
(a 401) instead of falling back to the stored credential, which bites when
CI exports an unset secret as the empty string.
Also carries a distinct fix that lives in the same rewritten resolver, called
out here because it is not implied by the title. The cloud verbs called
Config::load(None) -- user and env layers only -- so `[cloud] org` set in a
project's .hm/config.toml was silently ignored, while the error message users
hit named the very file being ignored:
no organization — set `[cloud] org = "…"` in ~/.config/hm/config.toml
(or .hm/config.toml), or run `hm cloud org switch <slug>`
settings::config() now walks up from the cwd and passes the project config
path to Config::load, which already takes an Option<&Path> for exactly this.
`hm cloud org switch` deliberately keeps Config::load(None): it is a
load-mutate-save on the user file, and merging the project layer in first
would persist project-scoped values into ~/.config/hm/config.toml.
Supporting work: hm-util gains AbsPath/AbsPathBuf and RelPath/RelPathBuf,
typed wrappers carrying the absolute/relative invariant, used by hm-core's
paths; hm-config drops its now-unused anyhow dependency and replaces two
hand-written Default impls with SmartDefault.
RunContext held a bare Config, and three verbs each hand-rolled the same root
resolution with no validation and no shared rule:
let root = match args.dir { Some(d) => d, None => current_dir()? };
in cli/render.rs, cli/pipelines.rs and commands/run/mod.rs.
Introduce hm_core::Workspace -- a validated project root (the directory
containing `.hm/`) plus its layered config -- and give it the single
resolution rule via Workspace::resolve:
- `--dir` names the project root verbatim, with no walk-up. The backend runs
discovery against cloned repo roots, and walking up out of a clone could
bind to an unrelated `.hm/` above it. A relative `--dir` is resolved against
the cwd, since a workspace root must be absolute.
- No `--dir`: walk up from the cwd, so a verb works from any subdirectory.
RunContext now carries the Workspace and is built only for `hm run`. It was
previously constructed unconditionally in main.rs, which would have made every
other verb (`hm version`, `hm cloud login`) require a project once a workspace
became mandatory.
Fixes a divergence in `hm run`: render_pipeline took a &RunContext, ignored it
(the parameter was named `_ctx`), and recomputed the root from `--dir`/cwd.
That PathBuf became req.repo_root and propagated into the scheduler, the
archive builder and the cloud backend -- so with `--dir`, config came from one
tree and code from another. It now uses ctx.workspace.path().
As a side effect `hm run --dir X` works from a cwd outside any workspace,
where it previously failed with "no harmont workspace found".
Behaviour change: `hm render` and `hm pipelines` now walk up when given no
`--dir`, where they were previously cwd-exact and failed in subdirectories.
Discovery is unaffected -- the backend always passes `--dir`.
`hm pipelines` keeps its contract that a repo with no `.hm/` yields the empty
envelope rather than an error. Only NotFound/InvalidPath/InvalidWorkspace map
to it; a malformed `.hm/config.toml` still fails loudly instead of
masquerading as a repo with no pipelines.
The platform roots resolve from $HOME/%APPDATA% and are absolute by
construction, so `Option<PathBuf>` understated them and pushed the
absolute-ness proof onto every caller. `Sys::load` was re-validating
what was already guaranteed:
hm_util::dirs::hm_config_dir()
.and_then(AbsPathBuf::new)
.ok_or(LoadingError::ConfigDirUnavailable)?
which conflated "no config dir" with "not absolute". Now the accessors
return `Option<AbsPathBuf>` and that collapses to a plain `.ok_or(...)`
with one honest failure mode.
`find_project_root` takes an `AbsPath` because the walk only terminates
meaningfully from an absolute start: a relative start would walk to the
empty path rather than `/`, and would yield a root that means something
different once the cwd changes.
Adds `AbsPathBuf::current_dir()` so callers that need a cwd-rooted
`AbsPath` don't each re-prove the invariant std already documents.
Callers reading these paths through Deref (`cache/clean.rs`,
`local/backend.rs`) needed no changes.
`hm-util::dirs` was three unrelated policies wearing one hat, and none of
them were platform utilities:
- `hm_config_dir` / `hm_cache_dir` / `hm_workspace_cache_dir` — where
*this user's* hm state lives. That is `Sys`, whose own doc already
claimed the ground: "the scope of the current user. This includes the
user's configuration directory". `Sys::load` was already resolving the
config dir and storing it as `hm_dir`.
- `find_project_root` — the project-scope walk-up rule, i.e. the
discovery half of `Workspace::resolve`, which was the only real caller.
What the platform actually tells us — `~/.config`, `~/.cache`, `$HOME` —
stays in hm-util as `os::dirs`, now `pub`. Joining `hm/` onto those is our
decision, not the platform's, so it moves to `Sys`.
The new `Sys` path accessors are associated fns: resolution only, no I/O,
no `Sys::load`. `Workspace::load` needs the user config path, not a set of
credentials — `hm render` should not create `~/.config/hm` and read
`credentials.toml` just to layer config.
Two forced consequences:
- `hm-config` loses `Config::load` / `save_user` / `user_config_path`.
They need the config dir, the config dir now lives in hm-core, and
hm-core already depends on hm-config — the reverse edge is a cycle.
`load_from_paths` / `save_to` already existed; callers now name their
layers explicitly, which makes `org switch`'s user-file-only rule
visible at the call site instead of a comment warning about it.
- `LocalBackend::new` takes the cache dir. hm-exec reaching into
`~/.cache/hm` broke the crate's own doctrine ("Auth is injected: this
crate takes a pre-built HarmontClient; it never reads credentials from
disk"). Injecting also stops the contract test from writing a snapshot
registry into the developer's real cache.
Verified end-to-end against a fake $HOME: the user layer alone selects
`backend = cloud`, and a project `.hm/config.toml` overrides it back.
`resolve` already called `find_root` — there was one walk-up, not two. But
`find_root` was public for exactly one caller, `settings::config()`, which
then hand-assembled the layering `Workspace::load` already does:
// workspace.rs load()
Config::load_from_paths(Sys::config_path()…, Some(&hm_dir.join("config.toml")))
// settings.rs config() — same layering, second copy
Config::load_from_paths(Sys::config_path()…, project.as_deref())
Two places knew "the user layer is Sys::config_path()", and settings
re-derived the project config path Workspace already knows. That is what
handing out a bare root invites.
`find_root` was public only because `resolve` couldn't say "maybe there is
no workspace" — the `hm cloud` verbs legitimately run outside a project. So
make absence expressible instead of exposing the primitive:
find(dir) -> Result<Option<Workspace>> no workspace = Ok(None)
resolve(dir) -> Result<Workspace> no workspace = Err(NotFound)
`resolve` is now `find(dir)?.ok_or(NotFound)`, keeping the three CLI call
sites unchanged rather than making each repeat the NotFound message.
`Ok(None)` means only "the cwd walk-up reached / without finding .hm/"; an
explicit `--dir` that is not a workspace stays a user error.
Adds `Sys::config()` (user + env, no project layer) as the user-scope
counterpart to `Workspace::config()`, so the user-layer wiring lives once.
`org switch` now names it, which states its user-file-only intent instead
of open-coding the same call the comment had to explain.
Verified against a fake $HOME through `hm cloud`: outside a project the
api_url comes from the user layer, inside it the project layer overrides,
the walk-up works from a subdirectory, and HM_API_URL still beats both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebuilt on
main, independent of the secrets feature (#152). Contains no secrets code — verified: nothing underdomain/,local/secrets.rs,cloud/secrets.rs,secret_resolver.rs, orverbs/secret.rsis touched.Two commits, each builds standalone (verified in detached worktrees). 184 passed, 0 failed.
202a8ac— move credential storage from hm-config to hm-coreNew
hm-corecrate owns process-level state; the credential store moves in asSys::creds.The store collapses from a two-level
(service, account)map to a single token. The cloud issues one user-scoped bearer —claim_token/redeem_codetake no org, and that one token lists every org the user belongs to — so keying it by anything was storing one secret under a key that couldn't discriminate it. A second API base (localhost, staging) is served byHM_API_TOKEN, which is how the integration tests already inject one, so the file only ever held a single entry in practice.~/.config/hm/credentials.toml. The old nested shape no longer parses and there is no migration — existing logins must re-runhm cloud login.Behaviour changes beyond the move:
0600, else load fails. Previously a malformed or insecure file was silently ignored.secrecy::SecretString, so they no longer leak viaDebug. Exposed only where astris required.HM_API_TOKEN=""is treated as unset. Honouring it sent an empty bearer (401) instead of falling back to the stored credential — bites when CI exports an unset secret as the empty string.Also carries a distinct fix, called out in the commit body since the title doesn't imply it: the cloud verbs called
Config::load(None), so[cloud] orgin a project's.hm/config.tomlwas silently ignored — while the error message named the very file being ignored.hm runread it correctly, so the two halves of the CLI disagreed about the active org in the same directory.hm cloud org switchdeliberately keepsConfig::load(None): it's a load-mutate-save on the user file, and the project layer would leak into~/.config/hm/config.toml.Supporting:
hm-utilgainsAbsPath/RelPath(typed absolute/relative invariant);hm-configdrops now-unusedanyhowand swaps two hand-writtenDefaultimpls forSmartDefault.c832178— add Workspace, unify root resolutionRunContextheld a bareConfig, and three verbs each hand-rolled the same unvalidated resolution:hm_core::Workspaceis a validated project root + its layered config, with one rule inWorkspace::resolve:--dirnames the root verbatim, no walk-up (the backend runs discovery against cloned repo roots — walking up could bind to an unrelated.hm/above the clone); no--dirwalks up from cwd.RunContextnow carries theWorkspaceand is built only forhm run. It was previously constructed unconditionally inmain.rs, which would have madehm version/hm cloud loginrequire a project once a workspace became mandatory. (Verified:hm versionstill works outside a project.)Fixes a divergence in
hm run:render_pipelinetook a&RunContext, ignored it (param named_ctx), and recomputed the root from--dir/cwd. ThatPathBufbecamereq.repo_rootand flowed into the scheduler, archive builder, and cloud backend — so with--dir, config came from one tree and code from another. Side effect:hm run --dir Xnow works from a cwd outside any workspace.hm render/hm pipelinesnow walk up when given no--dir, where they were cwd-exact and failed in subdirectories. Discovery is unaffected — the backend always passes--dir.hm pipelineskeeps its empty-envelope contract; onlyNotFound/InvalidPath/InvalidWorkspacemap to it, so a malformed.hm/config.tomlstill fails loudly rather than masquerading as a repo with no pipelines.Verification
Beyond
184 passed, 0 failed, driven against the built binary: no-creds andHM_API_TOKEN=""both fall back to "not logged in"; a0644credentials file is refused; project-config layering resolves in-project and falls back outside;pipelines --dirat a non-workspace subdir returns the empty envelope rather than escaping upward;hm versionworks outside a project.Follow-on
#152 (secrets) should rebase onto this once it lands. Two fixes stay with #152 because they touch files that don't exist on
main— theraw_org_ctx→SecretStringchange, and a percent-encoding bug whereNON_ALPHANUMERICescapes RFC 3986 unreserved chars, sohm cloud secret rm DEPLOY_TOKENrequests/secrets/DEPLOY%5FTOKEN. That one currently leaves #152's suite red.