Skip to content

feat: TOML config file support#202

Merged
inureyes merged 3 commits into
mainfrom
feat/192-toml-config
Apr 20, 2026
Merged

feat: TOML config file support#202
inureyes merged 3 commits into
mainfrom
feat/192-toml-config

Conversation

@inureyes

@inureyes inureyes commented Apr 20, 2026

Copy link
Copy Markdown
Member

Summary

  • Introduces a user-level TOML configuration file so operators can persist common settings (hostfile path, update interval, alert thresholds, $/kWh, etc.) without retyping CLI flags on every invocation.
  • Adds an all-smi config init | print | validate subcommand family for initialising, inspecting, and sanity-checking the file.
  • Implements the full precedence chain documented in feat: TOML config file support ('~/.config/all-smi/config.toml') #192: CLI flag > env var > config file > compiled default.

Implementation

  • src/common/config_file.rs owns the merged runtime Settings struct and the [load] entry point. Parsing and schema definitions live in config_schema.rs; file-to-settings projection in config_apply.rs; env-var overlay in config_env.rs. Keeping each file under ~400 lines keeps the merge logic readable.
  • src/common/paths.rs resolves the platform-appropriate config path (XDG on Linux, ~/Library/Application Support on macOS with XDG parity fallback, %APPDATA% on Windows) plus ~/~/ expansion.
  • src/common/secure_write.rs centralises the O_NOFOLLOW + 0o600 pattern from snapshot::write_output_atomic and record::writer::open_secure so config init reuses the same hardening.
  • src/config_cmd/ implements the init/print/validate runtime glue, a canonical commented example TOML, and both TOML and JSON renderers for config print. webhook_url is redacted unless --show-secrets is passed.
  • src/cli.rs + src/cli_config.rs add the global --config <PATH> option and the Config subcommand with Init/Print/Validate variants. ApiArgs/LocalArgs/ViewArgs now use Option<T> for every field that the config can provide so the precedence chain merges cleanly in main.rs.
  • src/main.rs calls config_file::load(cli.config.as_deref()) after parsing the CLI, then merges the resulting Settings into the mode entry points (run_api_mode, view::run_local_mode, view::run_view_mode, view::run_replay_mode). A malformed or explicitly-missing config file exits 2; implicit discovery silently falls back to defaults.
  • src/api/server.rs and src/view/runner.rs now accept a &Settings; their existing CLI args are resolved in main.rs before they are called.
  • Existing env vars from feat(tui): interactive filter query ('/') and threshold alerts for local/remote modes #186 (ALL_SMI_ALERT_TEMP, ALL_SMI_ALERT_UTIL_LOW_MINS) and feat: energy accumulation (kWh) and cost estimation with $/kWh config #191 (ALL_SMI_ENERGY_PRICE, ALL_SMI_ENERGY_NO_COST, ALL_SMI_ENERGY_WAL_PATH, ALL_SMI_ENERGY_NO_WAL, ALL_SMI_ENERGY_GAP_SECONDS, ALL_SMI_ENERGY_CURRENCY) are preserved as legacy aliases for the new canonical ALL_SMI_<SECTION>_<KEY> names.
  • Schema version 1 is enforced with a clean error for unsupported future versions.
  • Unknown keys are preserved and surfaced: config print warns about them, config validate accepts them for forward compat, and config validate --strict rejects them.

Testing

  • cargo test --lib — 899 passed
  • cargo test --bin all-smi — 1029 passed
  • cargo test --test config_file_integration_test — 12 passed (covers precedence chain, malformed TOML, unknown keys, schema version, legacy env aliases, webhook secrets, tilde paths)
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • Manual smoke: all-smi config init/print/validate round-trip, including --force overwrite, redacted/--show-secrets webhook, and explicit vs implicit --config discovery.

Documentation

  • README gains a new Configuration section with file locations, precedence table, helper-subcommand reference, and reload semantics.
  • DEVELOPERS.md gains a Configuration Architecture section describing the merge layers, file layout, and the recipe for adding a new config-file-driven field.

Closes #192

@inureyes
feat: TOML config file support
66c924a 
Adds a user-level TOML configuration file for `all-smi` so operators
can persist common settings without retyping CLI flags. Implements the
full precedence chain (CLI flag > env var > config file > compiled
default), a new `all-smi config init|print|validate` subcommand
family, and backward-compatible env var aliases.

Closes #192
@inureyes inureyes added type:enhancement New feature or request priority:medium Medium priority issue status:review Under review labels Apr 20, 2026
@inureyes

inureyes commented Apr 20, 2026

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

PR #202 implements issue #192 — TOML config file support (~/.config/all-smi/config.toml) with precedence chain (CLI > env > file > default), plus config init/print/validate helpers.

Overall Assessment

The core loader, schema, env overlay, secure-write hardening, and integration into api::server::run_api_mode and view::runner are well-executed. Tests pass (899 lib + 1029 bin + 12 integration), clippy clean, fmt clean. However, several acceptance-criteria items have gaps that would ship as orphaned or broken config knobs. Codex review (at gpt-5.4, high reasoning) independently surfaced overlapping findings.

Findings

HIGH

  • Orphaned sections — [general], [display], [record], [snapshot] — loaded, validated, and rendered but never consumed by any mode entry point. general.default_mode = "view" in config does not change the no-subcommand startup mode (src/main.rs:431-487). [snapshot] / [record] have no overlay at src/main.rs:307-362 before SnapshotOptions::from_args / RecorderOptions::from_args. Five sections worth of keys effectively no-op.
  • view.hostfile tilde expansion missing — loader preserves ~/hosts.csv verbatim (tests/config_file_integration_test.rs:294-311); consumer remote_collector::load_hosts_from_file (src/view/data_collection/remote_collector.rs:317) calls path.canonicalize() without tilde expansion, so a config value like hostfile = "~/.config/all-smi/hosts.csv" will fail at runtime despite being advertised in the commented example (src/config_cmd/example.rs:46) and documented in the README. Acceptance criterion: "~ expansion … resolve correctly."
  • energy.gap_interpolate_seconds config value does not reach the integrator — AppState::new() constructs EnergyAccountant with the default gap-interpolation window (src/app_state.rs:420-423). runner.rs and api/server.rs then overwrite initial_state.energy_config with settings.energy.clone() (src/view/runner.rs:63,167,256; src/api/server.rs:112), but the already-constructed PowerIntegrator inside EnergyAccountant still holds the default window. Config-supplied gap_interpolate_seconds = 30 silently has no effect.
  • Legacy ALL_SMI_ALERT_TEMP overrides canonical ALL_SMI_ALERTS_TEMP_WARN_C — in apply_env_alerts (src/common/config_env.rs:137-193) the canonical name is applied first, then the legacy alias runs and overwrites it. For energy, canonical_energy_env_beats_legacy asserts the opposite ordering. Inconsistent precedence between the two section aliases.

MEDIUM

  • Global --config <PATH> is only honored by config print — run_init (src/config_cmd/mod.rs:48-95) ignores the explicit path and always writes to paths::default_config_path(); run_validate (src/config_cmd/mod.rs:134-165) ignores it too. Contradicts the documented behavior that --config <PATH> applies "to any subcommand" (README.md:171).
  • Main runtime swallows unknown-key warnings — main.rs:83-97 emits outcome.warnings but ignores outcome.settings.unknown_keys. config print warns (src/config_cmd/mod.rs:111-113), so a typo like [alerts] temp_warn-c = 80 is silently discarded during all-smi local and only surfaces if the user explicitly runs config print or config validate --strict.
  • Duplicate expand_tilde helpers — common::paths::expand_tilde (src/common/paths.rs:56-72) is #[allow(dead_code)] and unused; the helper that's actually wired in is metrics::energy_wal::expand_tilde (src/metrics/energy_wal.rs:87-102). Consolidate into paths and have energy_wal re-export/use it — addresses "Module and Function Reuse" criterion.
  • Duplicate dirs crate versions — PR adds dirs = "5" as a direct dep (Cargo.toml:21); main already transitively pulls dirs = 6.0.0. Both end up compiled. Align on dirs = 6.
  • --processes cannot be overridden from CLI when config sets true — main.rs:225-227 uses if !args.processes { args.processes = settings.api.processes; }. Because args.processes is a raw bool, there is no CLI way to disable the flag when the config enables it. Either switch to Option<bool> with num_args = 0..=1 or add a --no-processes negation flag.

LOW

  • redact_secrets and socket_display in src/config_cmd/mod.rs:183-199 are #[allow(dead_code)] forward helpers. Either wire in or remove.
  • parse_toml deserializes the document twice (src/common/config_file.rs:326-328), once as TomlValue for unknown-key scanning and once as RawConfig. Minor cosmetic concern only.

What's correct

  • Schema version gate (schema_version = 2 → clean error).
  • Malformed TOML → stderr + exit 2.
  • config init uses the shared secure_write::create_new_secure / write_atomic_secure (O_NOFOLLOW + 0o600 on Unix, exclusive share-mode on Windows).
  • webhook_url redaction in both TOML and JSON renderers; --show-secrets opt-out works.
  • Toml-crate parse errors include line/column for config validate.
  • Unknown keys: warn in print, accept in validate, reject in validate --strict.
  • Settings flows through api::server::run_api_mode, view::run_local_mode, view::run_view_mode, view::run_replay_mode.
  • Legacy env vars (ALL_SMI_ENERGY_PRICE, ALL_SMI_ENERGY_NO_COST, ALL_SMI_ENERGY_WAL_PATH, ALL_SMI_ENERGY_NO_WAL, ALL_SMI_ENERGY_GAP_SECONDS, ALL_SMI_ENERGY_CURRENCY) still work as aliases for energy.
  • Precedence tests exist (env_overrides_file_value, file_overrides_default_when_no_env, defaults_apply_when_no_file_no_env, canonical_energy_env_beats_legacy).

Verification Checklist

  • All stated requirements implemented (with caveats above)
  • No placeholder/mock code
  • [~] Integrated into project code flow — api, local, view, replay modes consume Settings; snapshot/record/general/display sections do not
  • Project conventions followed (thiserror, no unwrap in library code, inline format args, 500-line soft cap respected)
  • Existing modules reused (secure_write consolidates three callers)
  • No unintended structural changes
  • Tests pass (cargo test --lib, cargo test --bin all-smi, cargo test --test config_file_integration_test, cargo clippy --all-targets -- -D warnings, cargo fmt --all -- --check)

Recommendation

Address the four HIGH findings before merge. The orphaned sections and missing hostfile tilde expansion are the highest-impact gaps because they change documented behavior; gap_interpolate_seconds is a silent energy-accounting correctness bug; the alert env-alias ordering asymmetry is a footgun for operators migrating scripts across subsystems. The four MEDIUM findings are quality-of-life improvements that can follow as a small companion PR.

inureyes added 2 commits April 21, 2026 04:35
@inureyes
fix(config): wire orphaned sections, harden loader, add missing aliases
297afa8 
Addresses review findings on PR #202. The [general], [display], [record],
and [snapshot] sections were parsed but had no runtime effect. The loader
was vulnerable to DoS via unbounded reads, parsed the TOML document twice,
and did not escape control characters before printing unknown keys.

HIGH-R1: wire orphaned config sections
  * general.default_mode redispatches to the matching subcommand when
    no CLI subcommand is given.
  * snapshot.default_format / default_pretty merge into SnapshotOptions
    via from_args_with_settings when the CLI flag is unset.
  * record.output_dir / record.compress merge into RecorderOptions via
    from_args_with_settings. output_dir is treated as the directory for
    the default basename with tilde expansion applied.
  * display.* propagates into AppState.display_config for downstream
    renderer consumption (TUI renderers do not yet switch on these
    values; future integration work can consume the field).

HIGH-R2: expand tilde on view.hostfile before canonicalize so a value
  like '~/.config/all-smi/hosts' resolves to the user's home instead of
  failing with NotFound.

HIGH-R3: add AppState::with_energy_config so the PowerIntegrator's
  gap_interpolate window is built from the merged EnergyConfig instead
  of the compiled default. Callers in view::runner and api::server
  updated. Previously, overwriting AppState.energy_config after
  AppState::new() had no effect on the already-constructed integrator.

HIGH-R4: apply legacy alerts env aliases first, canonical second. The
  energy pattern already worked this way; alerts was reversed. Added
  tests/config_file_integration_test.rs::canonical_alerts_env_beats_legacy
  to cover the precedence direction.

HIGH-S1: cap config file reads at 1 MiB with a defensive double-check
  (metadata + Read::take) and refuse to follow symlinks. Added
  oversized_config_file_rejected_cleanly integration test.

HIGH-S2: parse TOML once via toml::from_str -> Value, then reuse the
  parsed Value via try_into for the typed RawConfig. Avoids the second
  full-document parse that the previous implementation performed.

MEDIUM-S3: escape_printable() escapes U+0000-U+001F and U+007F as
  \\u{XXXX} for every unknown-key print site. Prevents terminal
  cursor/color injection via a hostile quoted TOML key.

MEDIUM-S4: render_toml escape_toml now covers the full TOML 1.0 basic
  string escape set (\\n, \\r, \\t, \\b, \\f, \\uXXXX for other
  control chars). Added a roundtrip test through parse_toml exercising
  every branch.

MEDIUM-R5: --config is now threaded into config init (write target) and
  config validate (default path when positional omitted), not just
  config print. Without this the clap-declared "global" flag was a
  no-op for two of the three sub-subcommands.

MEDIUM-R6: main.rs surfaces unknown-key warnings at startup so
  operators notice typos without running 'config print' separately.

MEDIUM-R9: ApiArgs::processes becomes Option<bool> with num_args=0..=1
  and default_missing_value="true" so the operator can force-disable
  process collection via --processes=false even when the config file
  sets api.processes = true. Previously bool could not express the
  third "unset" state.

LOW-R7: consolidate expand_tilde into common::paths. The duplicate in
  metrics::energy_wal becomes a re-export so every settings-consuming
  site shares the same implementation.

LOW-R8: bump direct dirs dep to version 6 to match the transitive
  version in the dependency graph.

Deferred (follow-up): webhook URL validation, string length bounds,
secure_write::create_new enforcement on every consumer, and any renderer
integration that consumes AppState.display_config.
@inureyes
docs: expand Configuration schema section with per-section field tabl…
1738590 
…es and legacy env var aliases

README.md's Schema section previously named the nine config sections
without documenting individual fields. Expand it with per-section
field tables (key, type, default, canonical env var, description)
for all sections: [general], [local], [view], [api], [alerts],
[energy], [display], [record], and [snapshot]. Add a dedicated
Legacy environment variable aliases table covering ALL_SMI_ALERT_TEMP,
ALL_SMI_ENERGY_PRICE, ALL_SMI_ENERGY_NO_COST, and the other six
backward-compat names introduced by issues #186 and #191.
@inureyes

inureyes commented Apr 20, 2026

Copy link
Copy Markdown
Member Author

PR Finalization Report

Tests

All tests pass at HEAD 1738590:

  • lib (unit): 901 passed, 0 failed
  • bin (unit): 1032 passed, 0 failed
  • integration (config_file_integration_test.rs): 14 passed, 0 failed

Clippy (-D warnings) and cargo fmt --check are both clean.

Documentation

README.md: The existing Configuration section already covered file paths, precedence chain, helpers (config init/print/validate), and the Reload note. The Schema subsection named all nine sections but had no field-level detail. Expanded it with:

  • A per-section field table for each of [general], [local], [view], [api], [alerts], [energy], [display], [record], and [snapshot] — key, type, compiled default, canonical env var, and description.
  • A dedicated "Legacy environment variable aliases" table documenting all eight backward-compat names (ALL_SMI_ALERT_TEMP, ALL_SMI_ENERGY_PRICE, ALL_SMI_ENERGY_NO_COST, etc.) with their canonical equivalents.

DEVELOPERS.md: The "Configuration Architecture" section (added in this PR) already documents the Settings refactor, the three-layer precedence model, key file responsibilities, and the step-by-step guide for wiring new config-driven fields. No changes needed.

--help / CLI: The --config global flag and the config init/print/validate subcommands with their flags are all reflected correctly in the binary's help output.

Lint/Format

No changes required — Clippy and fmt were already clean.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Apr 20, 2026
@inureyes inureyes merged commit 12bf2b2 into main Apr 20, 2026
4 checks passed
@inureyes inureyes deleted the feat/192-toml-config branch April 20, 2026 19:51
This was referenced May 24, 2026
Closed
Closed
@inureyes inureyes self-assigned this May 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@inureyes inureyes

Labels

priority:medium Medium priority issue status:done Completed type:enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: TOML config file support ('~/.config/all-smi/config.toml')

1 participant

@inureyes