Skip to content

fix: resolve record default output path consistently (#223)#227

Merged
inureyes merged 2 commits into
mainfrom
fix/issue-223-record-default-output-path
May 24, 2026
Merged

fix: resolve record default output path consistently (#223)#227
inureyes merged 2 commits into
mainfrom
fix/issue-223-record-default-output-path

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

Fixes the inconsistency between what all-smi record --help, the source doc comment on RecorderOptions::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.zst under the cache directory.

Behaviour change

Before this PR, the resolver detected "operator did not pass -o" by string-comparing args.output against the magic basename all-smi-record.ndjson.zst. As a side effect, an operator who passed -o all-smi-record.ndjson.zst explicitly was silently redirected under record.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 -o is omitted.

What changed

  • src/cli.rsRecordArgs::output is now Option<PathBuf> with no clap default_value. The doc comment is rewritten as a clap long_help so -h stays terse and --help describes the actual resolution rules (config record.output_dir default ~/.cache/all-smi/records/, basename all-smi-record.ndjson.zst, or just the basename in CWD when no config is loaded).
  • src/record/mod.rs — adds a shared pub const DEFAULT_BASENAME: &str = "all-smi-record.ndjson.zst" next to INDEX_EVERY_N_FRAMES. Rewrites the output-resolution block in from_args_with_settings to match &args.output: Some(p) is honoured verbatim; None joins DEFAULT_BASENAME under the (tilde-expanded) config output_dir, or falls back to DEFAULT_BASENAME in CWD. The old args.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 on from_args_with_settings is rewritten to match.
  • README.md — the record options-summary table now shows the actual default for --output as ~/.cache/all-smi/records/all-smi-record.ndjson.zst with 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::tests cover all four acceptance-criteria branches of the resolver:

  • resolve_output_no_cli_no_config — no -o, no config → just DEFAULT_BASENAME in CWD.
  • resolve_output_no_cli_with_config_dir — no -o, config output_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.zst with config output_dir set is honoured verbatim, not redirected under the cache dir.
  • resolve_output_explicit_absolute_path_is_honored-o /var/log/cluster.ndjson.zst is 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 touch src/common/config_file.rs, src/common/paths.rs, src/metrics/energy_wal.rs, or src/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 --tests passes.
  • cargo test --bin all-smi record:: — 33 passed, including the 4 new resolver tests (the record module lives under the all-smi binary target, not the library).
  • cargo clippy --bin all-smi --tests -- -D warnings passes.
  • cargo fmt --check passes.

Credit to @victorhooi for the original README correction in #210.

Closes #223

`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.
@inureyes inureyes added status:review Under review type:bug Something isn't working priority:medium Medium priority issue labels May 24, 2026
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.
@inureyes

Copy link
Copy Markdown
Member Author

PR Finalization Complete

Test coverage

Added one new resolver test () in . The four existing resolver tests used absolute paths, leaving the call in the None-CLI/Some-config branch untested. The new test passes output_dir = "~/my-records" through the resolver and derives the expected path using the same expand_tilde helper, making it portable across environments where $HOME may or may not be set. Total: 8 record tests, all passing.

Documentation

  • src/cli.rs long_help on RecordArgs::output: clearly states both cases (config default ~/.cache/all-smi/records/ + no-config fallback to CWD). Accurate.
  • src/record/mod.rs doc comment on from_args_with_settings: lists all three precedence levels and names DEFAULT_BASENAME. Accurate.
  • README.md record options table row for --output (line 1105): shows ~/.cache/all-smi/records/all-smi-record.ndjson.zst as the effective default. Accurate (matches the config-layer default).
  • No localized READMEs exist in this repository.
  • docs/man/all-smi.1 does not cover the record subcommand; no update needed (no --output entry to correct).
  • No CHANGELOG file exists in this repository.

Lint / format

  • cargo fmt --check: clean (no diffs).
  • cargo clippy --bin all-smi --tests -- -D warnings: clean (no warnings).

Build sanity

  • cargo check --bin all-smi --tests: finished successfully.
  • cargo test --bin all-smi record::tests::: 8 passed, 0 failed.

All checks passing. Ready for merge.

@inureyes inureyes added status:done Completed and removed status:review Under review labels May 24, 2026
@inureyes inureyes self-assigned this May 24, 2026
@inureyes inureyes merged commit 0d4cded into main May 24, 2026
2 checks passed
@inureyes inureyes deleted the fix/issue-223-record-default-output-path branch May 24, 2026 06:56
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:medium Medium priority issue status:done Completed type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

record: default output path is inconsistently documented and resolved (and not platform-aware)

1 participant