fix: resolve record default output path consistently (#223)#227
Merged
Conversation
`all-smi record`'s default output path was advertised inconsistently across `all-smi record --help`, the `from_args_with_settings` doc comment, and the README; behind the scenes the default was resolved by string-matching the operator's `-o` value against the magic basename `all-smi-record.ndjson.zst`. As a side effect of that string-sentinel guard, passing `-o all-smi-record.ndjson.zst` explicitly was silently redirected under `record.output_dir` (the cache dir) instead of being written verbatim to the current directory — a quiet behaviour change no operator would expect from a flag they passed explicitly. Scope A of the fix (this commit): - Make `RecordArgs::output` an `Option<PathBuf>` and drop clap's `default_value`. Resolution now happens in `from_args_with_settings`: `Some(p)` is honored verbatim, `None` falls back to `<output_dir>/<DEFAULT_BASENAME>` (or just `<DEFAULT_BASENAME>` when no config is loaded). The brittle basename comparison is gone. - Introduce a single shared `pub const DEFAULT_BASENAME: &str = "all-smi-record.ndjson.zst"` in `record/mod.rs`, dedupe-ing the literal that was previously inlined in both `cli.rs` and the resolver. - Align all three doc surfaces (the clap `long_help` on `--output`, the `from_args_with_settings` doc block, and the README record-options table) on the same default-resolution story. - Add four unit tests for the resolver: no `-o` + no config, no `-o` + config `output_dir`, explicit `-o` equal to the default basename (regression guard for the silent-redirect bug), and explicit `-o` absolute path with and without config. Scope B from the issue (deriving the cache base from `dirs::cache_dir()` so Windows / macOS land in their platform-correct cache directories) is intentionally deferred per the issue's own recommendation — it changes where existing files live and warrants a separate decision and migration note. Folds in the README correction originally proposed in #210 by @victorhooi, with credit; that PR could not be merged directly due to an unsigned CLA.
When `record.output_dir` is a tilde-prefixed path (e.g. `~/my-records`), the resolver calls `expand_tilde` before joining `DEFAULT_BASENAME`. The existing `resolve_output_no_cli_with_config_dir` test used an absolute path, so that branch was untouched. The new `resolve_output_no_cli_with_tilde_config_dir` test exercises the expansion call directly and derives its expected value from the same `expand_tilde` helper the resolver uses, making it portable across environments where `$HOME` may or may not be set.
Member
Author
PR Finalization CompleteTest coverageAdded one new resolver test () in . The four existing resolver tests used absolute paths, leaving the call in the Documentation
Lint / format
Build sanity
All checks passing. Ready for merge. |
This was referenced May 24, 2026
inureyes
added a commit
that referenced
this pull request
May 24, 2026
…230) * feat: unify cache paths via platform-aware dirs::cache_dir() (#229) Route every `all-smi` cache writer — record output, energy WAL, and the users-tab CSV export — through a single platform-aware helper built on `dirs::cache_dir()`, retire the three hard-coded `~/.cache/all-smi/...` literals, and delete the hand-rolled `cache_dir_for_all_smi()` reimplementation in `src/view/event_handler.rs`. The new `crate::common::paths::cache_dir()` helper mirrors the existing `config_dir()` so the cache layer is internally consistent with the config layer. `record.output_dir` and `energy.wal_path` now store `Option<String>` — `None` means "use the platform helper", `Some(s)` is an explicit operator override resolved through `expand_tilde`. Consumers (`record::RecorderOptions::from_args_with_settings`, `api::server::run_api_mode`) call the helper when no override is present. Adds `crate::common::cache_migration::migrate_legacy_cache_paths`, called from `main()` before any cache file is touched. On Linux without `$XDG_CACHE_HOME` the old and new paths are identical and it is a no-op; on macOS, Windows, and Linux with `$XDG_CACHE_HOME` set, it best-effort-renames `~/.cache/all-smi/{records,energy-wal.bin}` into the new root only when the destination is empty (never overwrites) and refuses to follow a pre-planted symlink at the source (same threat model as the writer-side `O_NOFOLLOW` defences). The users-CSV exports are deliberately not migrated — they carry per-second timestamps and bulk-renaming hundreds of files would be more disruptive than helpful. Tests added: `paths::tests::cache_dir_ends_with_app_name`, `record::tests::resolve_output_no_cli_no_config_uses_platform_cache_dir`, and a five-test `cache_migration::tests` module (`try_move_skips_when_source_missing`, `try_move_renames_file_when_dst_missing`, `try_move_renames_directory_when_dst_missing`, `try_move_no_op_when_dst_exists`, `try_move_refuses_symlink_source`). Existing `record::tests` resolver tests updated for the `Option<String>` shape; `config::tests::energy_config_defaults_match_issue_spec` and the `ALL_SMI_ENERGY_WAL_PATH` env test updated to assert `None` / `Some(...)`. Docs: README updates the `[energy] wal_path`, `[record] output_dir`, `--output` and users-CSV export rows; manpage updates the FILES and ENERGY ACCOUNTING sections; the `record --output` `long_help` and the example `config.toml` text spell out the per-platform cache locations. The v0.21.0 changelog gets a closing sentence describing the unification and the migration. The `config print` / `config print --json` renderer now shows the resolved on-disk path when no override is set so operators can confirm at a glance where data will land. Closes #229. Builds on #227 (Scope A of #223). The writer-side hardening (`O_NOFOLLOW`, mode `0600`, symlink refusal at rotation) lives in `src/record/writer.rs`, `src/metrics/energy_wal.rs::open_secure_append`, and `src/view/event_handler.rs::open_export_secure` and applies unchanged to the new resolved paths. `ALL_SMI_ENERGY_WAL_PATH` and `ALL_SMI_RECORD_OUTPUT_DIR` env-var support and the TOML key names are preserved. * docs: align ENERGY_HELP wal-path text with per-platform defaults The ALL_SMI_ENERGY_WAL_PATH help line still showed the Linux-only ~/.cache/all-smi/energy-wal.bin default, while the man page, README footnote, and RecordArgs long_help already describe the per-platform resolution. Use <platform cache dir>/all-smi/energy-wal.bin instead so all four doc surfaces describe the same default for #229. * refactor: tighten whitespace handling in cache-path resolvers (#229) Apply the two LOW defense-in-depth consistency fixes flagged by the security checker: **L1 — record resolver (src/record/mod.rs):** Add .map(str::trim) before .filter(!is_empty()) on the configured output_dir chain. A TOML value of " " (spaces only) now falls through to the platform cache helper instead of being honored as a literal relative directory, matching how resolve_wal_path() already handles whitespace-only wal_path values via str::trim. **L2 — TOML loader (src/common/config_apply.rs):** apply_file_energy and apply_file_record now trim and reject whitespace-only strings before writing to settings.energy.wal_path and settings.record.output_dir respectively, matching the existing behavior of apply_env_record which already calls trim().is_empty(). The collapsed let-chain form (if let Some(x) = ... && !x.trim().is_empty()) satisfies clippy's collapsible_if lint. **Tests added:** - src/record/mod.rs: resolve_output_whitespace_only_config_dir_falls_through — asserts a whitespace-only output_dir does not leak into the resolved output path. - tests/config_file_integration_test.rs: whitespace_only_wal_path_and_output_dir_treated_as_unset — loads a TOML with wal_path = " " and output_dir = " " and asserts both fields resolve to None. **Docs:** Changelog entry for the Users tab updated to reference the platform-aware cache path (via [^cache] footnote) instead of the Linux-only literal ~/.cache/all-smi/users-<timestamp>.csv. All targeted tests pass (1172 unit, 15 integration), clippy -D warnings clean, cargo fmt --check clean.
This was referenced May 24, 2026
inureyes
added a commit
that referenced
this pull request
May 24, 2026
Refs #221, #222, #224, #225, #226, #227. Align config path active discovery with the implicit loader's first-existing-candidate semantics, correct record output help and resolver docs against the binary default, and make the swap-color regression test derive crossterm's current Red SGR sequence instead of hard-coding older encodings.
inureyes
added a commit
that referenced
this pull request
May 24, 2026
Refs #221, #222, #224, #225, #226, #227. Align config path active discovery with the implicit loader's first-existing-candidate semantics, correct record output help and resolver docs against the binary default, and make the swap-color regression test derive crossterm's current Red SGR sequence instead of hard-coding older encodings.
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
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.
Summary
Fixes the inconsistency between what
all-smi record --help, the source doc comment onRecorderOptions::from_args_with_settings, and the README each claimed as the default output path, and removes the brittle string-sentinel resolver that silently redirected an explicit-o all-smi-record.ndjson.zstunder the cache directory.Behaviour change
Before this PR, the resolver detected "operator did not pass
-o" by string-comparingargs.outputagainst the magic basenameall-smi-record.ndjson.zst. As a side effect, an operator who passed-o all-smi-record.ndjson.zstexplicitly was silently redirected underrecord.output_dir(the cache dir) instead of having their flag honoured verbatim. After this PR,-o <path>is always honoured exactly as passed — including when<path>happens to equal the default basename. There is no change to where files land when-ois omitted.What changed
src/cli.rs—RecordArgs::outputis nowOption<PathBuf>with no clapdefault_value. The doc comment is rewritten as a claplong_helpso-hstays terse and--helpdescribes the actual resolution rules (configrecord.output_dirdefault~/.cache/all-smi/records/, basenameall-smi-record.ndjson.zst, or just the basename in CWD when no config is loaded).src/record/mod.rs— adds a sharedpub const DEFAULT_BASENAME: &str = "all-smi-record.ndjson.zst"next toINDEX_EVERY_N_FRAMES. Rewrites the output-resolution block infrom_args_with_settingstomatch &args.output:Some(p)is honoured verbatim;NonejoinsDEFAULT_BASENAMEunder the (tilde-expanded) configoutput_dir, or falls back toDEFAULT_BASENAMEin CWD. The oldargs.output == default_basename && let Some(dir)...guard (and its self-deprecating "if that ever changes, update this guard" comment) is gone. The precedence doc block onfrom_args_with_settingsis rewritten to match.README.md— therecordoptions-summary table now shows the actual default for--outputas~/.cache/all-smi/records/all-smi-record.ndjson.zstwith the override hints inline. This folds in the README correction originally proposed in Update default output path in README.md #210 by @victorhooi; that PR could not be merged directly due to an unsigned CLA, so the fix is reapplied here with credit.Tests
Four unit tests added to
record::testscover all four acceptance-criteria branches of the resolver:resolve_output_no_cli_no_config— no-o, no config → justDEFAULT_BASENAMEin CWD.resolve_output_no_cli_with_config_dir— no-o, configoutput_dir = /tmp/all-smi-records→/tmp/all-smi-records/all-smi-record.ndjson.zst.resolve_output_explicit_matching_basename_is_honored— regression guard for the silent-redirect bug:-o all-smi-record.ndjson.zstwith configoutput_dirset is honoured verbatim, not redirected under the cache dir.resolve_output_explicit_absolute_path_is_honored—-o /var/log/cluster.ndjson.zstis honoured verbatim, with or without config.Scope B is intentionally deferred
The issue defines a Scope B (derive the cache base from
dirs::cache_dir()so Windows / macOS land in their platform-correct cache directories instead of~/.cache/...) and recommends "land Scope A first; treat Scope B as a separate decision/PR to keep changes focused." This PR follows that recommendation and does not touchsrc/common/config_file.rs,src/common/paths.rs,src/metrics/energy_wal.rs, orsrc/view/event_handler.rs. A follow-up issue could track the platform-aware cache work, which is a behaviour change that moves existing files on macOS, Windows, and Linux-with-$XDG_CACHE_HOME-set and warrants its own migration / changelog note.Test plan
cargo check --lib --testspasses.cargo test --bin all-smi record::— 33 passed, including the 4 new resolver tests (therecordmodule lives under theall-smibinary target, not the library).cargo clippy --bin all-smi --tests -- -D warningspasses.cargo fmt --checkpasses.Credit to @victorhooi for the original README correction in #210.
Closes #223