Skip to content

feat: add one-shot CLI mode#8

Merged
nonnil merged 13 commits into
mainfrom
feat/issue-5-one-shot-cli
Apr 22, 2026
Merged

feat: add one-shot CLI mode#8
nonnil merged 13 commits into
mainfrom
feat/issue-5-one-shot-cli

Conversation

@nonnil
Copy link
Copy Markdown
Member

@nonnil nonnil commented Apr 22, 2026

Summary

  • Adds one-shot CLI mode with --model/--timeout target selection.
  • Adds config.toml device memory with default, labelled devices, and device save/list/remove.
  • Adds interactive device list/save/remove support using the current connected device metadata.
  • Preserves the existing interactive REPL flow and reuses the existing command registry for device actions.

Verification

  • cargo fmt --check
  • cargo check
  • cargo test
  • cargo clippy -- -D warnings
  • cargo clippy --all-targets --all-features -- -D warnings

Related issue: #5

Summary by CodeRabbit

  • New Features

    • Full CLI mode with global options and one-shot command routing; interactive mode now reuses saved device context
    • Device management: save, list, remove device labels with persistent, file-backed config and remembered default device
    • Model-label parsing and reserved-label checks; updated shell completions for device subcommands
  • Documentation

    • Help text updated to document the device management command
  • Chores

    • Added CLI/config tooling; removed local history file and updated .gitignore

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

Warning

Rate limit exceeded

@nonnil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 25 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 25 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a8939a03-5d4a-4ea7-8574-7e78ac0929a0

📥 Commits

Reviewing files that changed from the base of the PR and between ac56870 and 2bc016f.

📒 Files selected for processing (17)
  • README.md
  • src/cli.rs
  • src/config.rs
  • src/loader/cmds/autostart.rs
  • src/loader/cmds/brightness.rs
  • src/loader/cmds/device.rs
  • src/loader/cmds/flexbattery.rs
  • src/loader/cmds/flexpuff.rs
  • src/loader/cmds/help.rs
  • src/loader/cmds/info.rs
  • src/loader/cmds/mod.rs
  • src/loader/cmds/smartgesture.rs
  • src/loader/cmds/version.rs
  • src/loader/cmds/vibration.rs
  • src/loader/iqoshelper.rs
  • src/loader/parser.rs
  • src/main.rs
📝 Walkthrough

Walkthrough

Adds a non-interactive one-shot CLI (clap) with global options, persistent TOML-backed device config and device-memory commands, model/label-based target resolution and timed BLE auto-connect, console entrypoints accepting a connected device, and refactors main/loader to route one-shot vs interactive flows.

Changes

Cohort / File(s) Summary
Cargo / deps
Cargo.toml
Added clap (derive), serde (derive), and toml = "0.8".
CLI parsing
src/cli.rs
New Clap-based parser and enums (Cli, CliCommand, DeviceCommand, OneShotCommand), global-option normalization, timeout resolution, CLI-mode detection, and unit tests.
Configuration (TOML)
src/config.rs
New AppConfig, SavedDevice/DefaultDevice/ConnectedDevice, load/save (TOML) APIs, config path resolution (config_file()), device CRUD/update helpers, label validation, permission handling on Unix, and printing helpers with tests.
Model selector
src/model_selector.rs
New parsing utilities: parse_device_model and is_reserved_model_label for model-name normalization and reserved-label detection with tests.
Loader public API
src/loader/mod.rs
Expanded re-exports to include run_console_with_device and run_registered_command (added allow-unused-imports).
Console parser / runner
src/loader/parser.rs
IQOSConsole now holds connected_device, added with_connected_device ctor; execute_command returns Result<bool> with "device" routing; added run_console_with_device and run_registered_command; interactive loop updated to honor boolean handling and new error classification.
Device command module
src/loader/cmds/device.rs
New device subcommand dispatcher (list, save <label>, remove <label>) using AppConfig for persistence, enforcing argument arity and printing status.
Commands / completion / help
src/loader/iqoshelper.rs, src/loader/cmds/help.rs, src/loader/cmds/mod.rs
Registered new device command, added DEVICE_ARGS completion (list, save, remove), updated help text, and exported pub mod device.
Main entrypoint / control flow
src/main.rs
Refactored main to choose CLI vs interactive, added structured exit codes and ExitError, one-shot run flow, target resolution (ScanTarget/ResolvedTarget), timed BLE scan/connect helpers, connection-memory persistence, serial-mismatch warning, and integration with run_console_with_device / run_registered_command.
Misc files
.gitignore, history.txt
Added history.txt to .gitignore and removed history.txt file.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as "CLI Parser"
    participant Config as "Config Manager"
    participant BLE as "BLE Scanner"
    participant Device as "IQOS Device"
    participant Console as "Command Console"

    User->>CLI: iqos --model <target> <command>
    CLI->>Config: load config / resolve target
    CLI->>BLE: start scan (timeout)
    BLE->>CLI: discovered peripheral
    CLI->>BLE: match peripheral (by model or address)
    CLI->>Device: connect_and_discover()
    Device->>CLI: read DeviceInfo (serial, model)
    CLI->>Config: update default / save metadata
    CLI->>Console: run one-shot command (connected device)
    Console->>Device: send command
    Device-->>Console: command response
    Console-->>CLI: result
    CLI->>User: print result, exit(code)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰
I nibbled flags and TOML roots,
hid labels in cozy config boots,
one-shot hop — connect and run,
a carrot click, then homeward fun,
nibble, save, then off I scoot!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add one-shot CLI mode' directly and clearly summarizes the main feature addition in the PR.
Linked Issues check ✅ Passed The PR comprehensively implements all coding requirements from issue #5: one-shot CLI mode with clap parsing, device memory with config.toml, auto-connect flow with scan targeting, device save/list/remove commands, exit code handling, and interactive mode preservation.
Out of Scope Changes check ✅ Passed All code changes align with PR objectives: CLI infrastructure (clap/serde/toml), device configuration, auto-connect logic, device commands, and git ignore updates. The history.txt deletion is a minor cleanup consistent with the changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-5-one-shot-cli

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli.rs`:
- Around line 29-31: The Autostart enum variant (and the other five enum
variants that define an args: Vec<String> passthrough) are using
#[arg(value_name = "arg")] so clap treats hyphen-prefixed tokens as flags;
update each of those #[arg(...)] attributes to include allow_hyphen_values =
true and trailing_var_arg = true (e.g., #[arg(value_name = "arg",
allow_hyphen_values = true, trailing_var_arg = true)]) for Autostart and the
other variants that pass args to the registered command handler so
hyphen-prefixed values are accepted and all remaining tokens are collected.

In `@src/config.rs`:
- Around line 108-117: In config_file(), stop using XDG_CONFIG_HOME when it's
empty or a relative path: when std::env::var_os("XDG_CONFIG_HOME") returns a
value, convert it to a PathBuf and only use it if it's non-empty and
path.is_absolute() (otherwise ignore it and fall through to the home_dir or
temp_dir branches); update the logic in the config_file function to perform this
check before joining "iqos_cli/config.toml" so relative/empty XDG_CONFIG_HOME
values don't make you read/write in the CWD.

In `@src/main.rs`:
- Around line 353-356: The config persistence currently calls AppConfig::load()
and config.save() directly which can return errors and abort the REPL; change
this to non-fatal handling: call connected_device(...) as before, then attempt
AppConfig::load() inside a match/if let, and if loading fails log a warning and
create a default AppConfig, call update_default(&device) on the config, then
attempt config.save() but catch and log any save errors instead of propagating
them; ensure no Err from AppConfig::load or save escapes so the REPL always
continues after a successful BLE connection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ea45a2dd-7a01-4b7e-a75b-5c506aeaf354

📥 Commits

Reviewing files that changed from the base of the PR and between 8ceed20 and f744658.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • Cargo.toml
  • src/cli.rs
  • src/config.rs
  • src/loader/mod.rs
  • src/loader/parser.rs
  • src/main.rs

Comment thread src/cli.rs
Comment thread src/config.rs
Comment thread src/main.rs Outdated
@nonnil
Copy link
Copy Markdown
Member Author

nonnil commented Apr 22, 2026

review @coderabbitai @copilot

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

@nonnil Sure, I'll kick off a full review of PR #8 right away!


🧠 Learnings used
Learnt from: nonnil
Repo: V-VX/iqos_cli PR: 7
File: src/loader/compat.rs:1-3
Timestamp: 2026-04-21T15:43:05.993Z
Learning: In `src/loader/compat.rs` of the `V-VX/iqos_cli` repository (Rust), as of `iqos = "1.1.0"`, all capability helpers (`supports_brightness`, `supports_flexbattery`, `supports_flexpuff`, `supports_smartgesture`) correctly delegate to `model.supports(DeviceCapability::...)` from the `iqos` crate. `SmartGesture` covers all holder models (Iluma, IlumaPrime, IlumaI, IlumaIPrime); `FlexPuff` is restricted to ILUMA i holder models only (IlumaI, IlumaIPrime). Do not flag the delegation to the crate's capability matrix as a bug.
✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/config.rs`:
- Around line 77-87: save_device currently persists arbitrary labels that may
collide with model selector names used by resolve_target (e.g., "iluma" or
"iluma-i"), causing saved labels to be unreachable via --model; add validation
in save_device to reject any label that matches reserved model-selector names by
reusing a shared helper (e.g., introduce/use is_reserved_model_label or
validate_label) and check before inserting into self.devices, returning an error
or refusing to save; update SavedDevice creation path to only run after
validation and ensure the same helper is used by resolve_target to keep the set
of reserved names consistent.

In `@src/main.rs`:
- Around line 163-171: The current flow aborts on AppConfig::load and
config.save even for known-model targets; change it so AppConfig::load errors
are non-fatal when resolve_target indicates a known-model target (i.e., when
target identifies a built-in/registered model from model_arg) by falling back to
a default/empty AppConfig and logging a warning instead of returning ExitError;
likewise make apply_connection_memory + config.save non-fatal for the console
and registered-command one-shot paths (the code paths around connect_target,
apply_connection_memory, and the save call): on save failure, log a warning and
continue only when the target is a known-model and the current mode is console
or registered-command, but keep load/save errors fatal for default/label
resolution and for device save/remove commands. Use the symbols AppConfig::load,
resolve_target, connect_target, apply_connection_memory, and config.save to
locate and implement the conditional fallback and warnings.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9e2f1459-ed7c-4fba-b012-1e09a1db381e

📥 Commits

Reviewing files that changed from the base of the PR and between f744658 and 9d798a8.

📒 Files selected for processing (11)
  • .gitignore
  • history.txt
  • src/cli.rs
  • src/config.rs
  • src/loader/cmds/device.rs
  • src/loader/cmds/help.rs
  • src/loader/cmds/mod.rs
  • src/loader/iqoshelper.rs
  • src/loader/mod.rs
  • src/loader/parser.rs
  • src/main.rs
💤 Files with no reviewable changes (1)
  • history.txt
✅ Files skipped from review due to trivial changes (3)
  • src/loader/cmds/help.rs
  • .gitignore
  • src/loader/cmds/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/loader/mod.rs
  • src/loader/parser.rs
  • src/cli.rs

Comment thread src/config.rs Outdated
Comment thread src/main.rs Outdated
@nonnil
Copy link
Copy Markdown
Member Author

nonnil commented Apr 22, 2026

review @coderabbitai @copilot

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

@nonnil Sure! I'll kick off a full review of PR #8 now.


🧠 Learnings used
Learnt from: nonnil
Repo: V-VX/iqos_cli PR: 7
File: src/loader/compat.rs:1-3
Timestamp: 2026-04-21T15:43:05.993Z
Learning: In `src/loader/compat.rs` of the `V-VX/iqos_cli` repository (Rust), as of `iqos = "1.1.0"`, all capability helpers (`supports_brightness`, `supports_flexbattery`, `supports_flexpuff`, `supports_smartgesture`) correctly delegate to `model.supports(DeviceCapability::...)` from the `iqos` crate. `SmartGesture` covers all holder models (Iluma, IlumaPrime, IlumaI, IlumaIPrime); `FlexPuff` is restricted to ILUMA i holder models only (IlumaI, IlumaIPrime). Do not flag the delegation to the crate's capability matrix as a bug.

Learnt from: nonnil
Repo: V-VX/iqos_cli PR: 8
File: src/config.rs:0-0
Timestamp: 2026-04-22T02:50:27.576Z
Learning: In `V-VX/iqos_cli` (Rust), as of PR `#8` (commit 2708385), known IQOS device model selectors (e.g., "iluma", "iluma-i", "iluma-prime", etc.) are centralized in a shared `model_selector` module. `AppConfig::save_device` (in `src/config.rs`) rejects any label that parses as a built-in model selector using this shared helper. The CLI `device save` subcommand also validates the label before connecting, so labels colliding with model selectors are rejected early. Do not flag label-collision with model selectors as unguarded.
✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Member Author

@nonnil nonnil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a non-interactive “one-shot” CLI mode to the existing IQOS interactive console, including persistent device memory (default device + labelled devices) stored in a TOML config, and reuses the existing command registry for executing device actions.

Changes:

  • Introduces a clap-based CLI with global --model/--timeout, one-shot command routing, and an auto-connected console mode.
  • Adds persistent config.toml device memory (default + labelled devices) with list/save/remove flows.
  • Extends the interactive REPL to support device subcommands and to remember the last connected device.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/main.rs Implements CLI entry path, target resolution, scan/connect flow, exit codes, and interactive flow integration with device memory.
src/cli.rs Defines clap CLI surface, one-shot command mapping, global option normalization, and scan timeout resolution.
src/config.rs Implements config load/save + device memory model and label validation.
src/model_selector.rs Adds parsing/normalization for model selectors and “reserved label” detection with tests.
src/loader/parser.rs Extends console execution to support device command and adds helpers for running with connected device / one-shot execution.
src/loader/mod.rs Re-exports new console entry points.
src/loader/iqoshelper.rs Adds completions for device subcommands and updates tests.
src/loader/cmds/device.rs New REPL device command (list/save/remove) backed by config.
src/loader/cmds/mod.rs Registers the new device command module.
src/loader/cmds/help.rs Updates help output to include the new device command.
Cargo.toml Adds clap/serde/toml dependencies for CLI parsing + config persistence.
Cargo.lock Locks new transitive dependencies from clap/toml/serde additions.
.gitignore Adds an explicit ignore for history.txt.
history.txt Removes the committed history file.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/cli.rs
Comment thread src/loader/mod.rs
Comment thread src/loader/parser.rs
Comment thread src/config.rs
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
.gitignore (1)

15-15: Redundant pattern; already covered by line 2.

The *.txt pattern on line 2 already ignores all .txt files, including history.txt. The explicit entry here is redundant and can be removed for cleaner maintainability.

🧹 Proposed fix to remove redundancy
-history.txt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.gitignore at line 15, Remove the redundant "history.txt" entry from
.gitignore because the existing "*.txt" pattern already covers it; locate the
explicit "history.txt" line and delete it so only the general "*.txt" rule
remains, keeping the file clean and maintainable.
src/main.rs (1)

564-576: Fragile exit-code classification via error-message prefix matching.

classify_command_error pattern-matches on anyhow::Error::to_string() prefixes. Any rewording in a command's bail!/anyhow! call (or a new user-input error prefix added later) will silently misclassify exit codes, which the spec ties to user-observable behavior (2 = invalid args vs. 3 = device error). This is a growing maintenance hazard as the command registry expands.

Consider introducing a typed error (e.g., a CommandError enum with InvalidArgs/DeviceFailure variants) returned by command handlers, or attach a sentinel via anyhow::Error::downcast_ref so classification is based on type rather than text. At minimum, centralize the usage/invalid-arg messages behind constants that both the bailing code and classify_command_error reference.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main.rs` around lines 564 - 576, classify_command_error currently
inspects the string from anyhow::Error to decide exit codes, which is fragile;
change command handlers to return a typed error (e.g., introduce a CommandError
enum with variants InvalidArgs and DeviceFailure) or attach a sentinel type that
can be downcast from anyhow::Error; update classify_command_error to first
attempt error.downcast_ref::<CommandError>() and map those variants to
EXIT_INVALID_ARGUMENTS or EXIT_DEVICE_COMMAND_FAILED, and only fall back to
string checks if no typed error is present; update existing bail!/anyhow! calls
in command handlers to produce/convert to CommandError so classification is
type-based rather than text-based.
src/loader/cmds/device.rs (1)

24-35: Optional: validate label before mutating config state.

config.save_device (via validate_device_label inside src/config.rs) may reject the label after update_default has already mutated the in-memory config. It's harmless today because config.save() only runs on the success path, but reordering makes the intent clearer and future-proofs against accidental early saves:

♻️ Proposed refactor
     let mut config = AppConfig::load()?;
-    config.update_default(device);
     config.save_device(label.to_string(), device)?;
+    config.update_default(device);
     config.save()?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/loader/cmds/device.rs` around lines 24 - 35, Load the AppConfig first,
then validate the provided label before mutating in-memory config: after calling
AppConfig::load(), call the label validation routine (the existing
validate_device_label logic exposed on AppConfig) with the incoming label and
bail on failure, and only then call config.update_default(device),
config.save_device(label.to_string(), device) and finally config.save(); this
prevents update_default from mutating state when the label would later be
rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/config.rs`:
- Around line 63-70: The current save_to function uses fs::write which creates
the file with default permissions; change it to create and write the file via
std::fs::OpenOptions so you can set restrictive permissions (owner read/write
only) on Unix. In save_to, after serializing with toml::to_string_pretty(self),
open the path with OpenOptions::new().write(true).create(true).truncate(true)
and, using std::os::unix::fs::OpenOptionsExt::mode(0o600) on Unix targets, write
the contents and flush; keep the existing context error messages. Ensure this is
done for the same save_to function and that Windows builds still compile (guard
Unix-specific imports/usage behind cfg(unix) if needed).

In `@src/main.rs`:
- Around line 195-216: The DeviceList and DeviceRemove branches currently map
AppConfig::load() and config.save() errors to EXIT_CONNECTION_FAILED; change
these to return EXIT_DEVICE_COMMAND_FAILED instead so pure-config I/O failures
are not reported as BLE connection errors—specifically update the map_err
closures around AppConfig::load() in OneShotCommand::DeviceList and
OneShotCommand::DeviceRemove, and the config.save() error handling in
DeviceRemove (and likewise the post-connect config.save() in
OneShotCommand::DeviceSave) to use ExitError::new(EXIT_DEVICE_COMMAND_FAILED,
... ) instead of EXIT_CONNECTION_FAILED.

---

Nitpick comments:
In @.gitignore:
- Line 15: Remove the redundant "history.txt" entry from .gitignore because the
existing "*.txt" pattern already covers it; locate the explicit "history.txt"
line and delete it so only the general "*.txt" rule remains, keeping the file
clean and maintainable.

In `@src/loader/cmds/device.rs`:
- Around line 24-35: Load the AppConfig first, then validate the provided label
before mutating in-memory config: after calling AppConfig::load(), call the
label validation routine (the existing validate_device_label logic exposed on
AppConfig) with the incoming label and bail on failure, and only then call
config.update_default(device), config.save_device(label.to_string(), device) and
finally config.save(); this prevents update_default from mutating state when the
label would later be rejected.

In `@src/main.rs`:
- Around line 564-576: classify_command_error currently inspects the string from
anyhow::Error to decide exit codes, which is fragile; change command handlers to
return a typed error (e.g., introduce a CommandError enum with variants
InvalidArgs and DeviceFailure) or attach a sentinel type that can be downcast
from anyhow::Error; update classify_command_error to first attempt
error.downcast_ref::<CommandError>() and map those variants to
EXIT_INVALID_ARGUMENTS or EXIT_DEVICE_COMMAND_FAILED, and only fall back to
string checks if no typed error is present; update existing bail!/anyhow! calls
in command handlers to produce/convert to CommandError so classification is
type-based rather than text-based.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 93c53d40-d769-4b8f-b65e-7df270983259

📥 Commits

Reviewing files that changed from the base of the PR and between 8ceed20 and dd73361.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • .gitignore
  • Cargo.toml
  • history.txt
  • src/cli.rs
  • src/config.rs
  • src/loader/cmds/device.rs
  • src/loader/cmds/help.rs
  • src/loader/cmds/mod.rs
  • src/loader/iqoshelper.rs
  • src/loader/mod.rs
  • src/loader/parser.rs
  • src/main.rs
  • src/model_selector.rs
💤 Files with no reviewable changes (1)
  • history.txt

Comment thread src/config.rs Outdated
Comment thread src/main.rs
@nonnil
Copy link
Copy Markdown
Member Author

nonnil commented Apr 22, 2026

CodeRabbit review follow-up for commit ac56870:

  • src/loader/cmds/device.rs: addressed the nitpick by validating device save <label> before mutating update_default.
  • src/main.rs: addressed the fragile command exit-code classification by introducing a typed CommandError sentinel for one-shot registered command failures, with the old string fallback retained for compatibility.
  • .gitignore: not changed. The duplicate history.txt note is technically correct, but this repository has a hard rule not to modify .gitignore, and the explicit entry was previously requested for local history handling.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/loader/parser.rs (1)

37-53: Fragile prefix-string classification ties exit codes to error message text.

CommandError::classify and is_invalid_argument_message distinguish "argument" vs "device" failures by scanning the first few characters of the error's Display string. Per the issue spec, this mapping drives exit codes 2 vs 3, so:

  • Any future command-module error that happens to start with Usage:, Invalid option:, etc. will be reported as an argument error even if it was produced deep inside a BLE/device call.
  • Conversely, an argument-validation message that doesn't start with one of the five hardcoded prefixes silently degrades to DeviceFailure (exit 3).
  • The prefixes are also duplicated at the main.rs side (classify_command_error falls back to is_invalid_argument_message), so every command now has an implicit contract on its error wording.

Consider having individual command handlers return CommandError::invalid_arguments(...) directly (via anyhow::Error::from(CommandError::...)) so classify_command_error in main.rs can rely purely on downcast_ref::<CommandError>() and drop the string-matching fallback.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/loader/parser.rs` around lines 37 - 53, Replace fragile prefix-based
classification by making CommandError the canonical source: update classify (fn
classify(error: anyhow::Error) -> Self) to first try
error.downcast_ref::<CommandError>() (or error.downcast::<CommandError>()) and
return the concrete CommandError if present; only then (for compatibility) fall
back to is_invalid_argument_message(message) as a last resort. Add/ensure a
constructor like CommandError::invalid_arguments(...) (or
CommandError::InvalidArguments(...) conversion helper) that command handlers use
to return argument errors directly (convert to anyhow::Error where needed). Also
update command handlers to return CommandError::invalid_arguments(...) for
argument validation failures so classify_command_error in main.rs can rely on
downcast_ref::<CommandError>() and you can eventually remove the brittle
is_invalid_argument_message checks.
src/config.rs (1)

66-93: Non-atomic save can corrupt config.toml on crash/interrupt.

save_to opens the target path with truncate(true) and then streams contents + flush(). If the process is killed (SIGINT, panic, power loss) between the truncate and the final flush, the previous contents — every [devices.<label>] entry plus [default] — are already gone and the file is left empty or partial. Given this file is the single source of truth for user-labelled devices, the blast radius is larger than usual.

Consider the standard write-temp + rename pattern so the old file survives until the new one is fully written:

♻️ Proposed refactor
-        let contents = toml::to_string_pretty(self)?;
-        let mut options = OpenOptions::new();
-        options.write(true).create(true).truncate(true);
-
-        #[cfg(unix)]
-        {
-            options.mode(0o600);
-        }
-
-        let mut file = options
-            .open(&path)
-            .with_context(|| format!("failed to write {}", path.display()))?;
-
-        #[cfg(unix)]
-        file.set_permissions(fs::Permissions::from_mode(0o600))
-            .with_context(|| format!("failed to set permissions on {}", path.display()))?;
-
-        file.write_all(contents.as_bytes())
-            .with_context(|| format!("failed to write {}", path.display()))?;
-        file.flush()
-            .with_context(|| format!("failed to write {}", path.display()))
+        let contents = toml::to_string_pretty(self)?;
+        let tmp_path = path.with_extension("toml.tmp");
+
+        let mut options = OpenOptions::new();
+        options.write(true).create(true).truncate(true);
+        #[cfg(unix)]
+        {
+            options.mode(0o600);
+        }
+
+        let mut file = options
+            .open(&tmp_path)
+            .with_context(|| format!("failed to write {}", tmp_path.display()))?;
+
+        #[cfg(unix)]
+        file.set_permissions(fs::Permissions::from_mode(0o600))
+            .with_context(|| format!("failed to set permissions on {}", tmp_path.display()))?;
+
+        file.write_all(contents.as_bytes())
+            .with_context(|| format!("failed to write {}", tmp_path.display()))?;
+        file.sync_all()
+            .with_context(|| format!("failed to sync {}", tmp_path.display()))?;
+        drop(file);
+
+        fs::rename(&tmp_path, &path)
+            .with_context(|| format!("failed to replace {}", path.display()))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/config.rs` around lines 66 - 93, The save_to function currently truncates
the target (OpenOptions::truncate(true)) and streams contents directly which can
leave config.toml empty/partial on crash; change save_to to write to a temporary
file in the same directory (e.g. create a temp PathBuf next to path), write the
serialized contents to that temp file (use OpenOptions without truncating the
real file), set the file mode/permissions on unix (the same 0o600 logic
currently applied to options/file), flush and sync data (fsync) on the temp
file, then atomically rename (fs::rename) the temp file to path so the original
file remains intact until the new file is fully persisted; ensure error contexts
remain (with_context) and remove the temp file on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/config.rs`:
- Around line 101-146: validate_device_label currently only trims for emptiness
but returns the original label, causing padded labels to be stored; change
validate_device_label(&str) to return a Result<String> with the trimmed label
and update callers to use that trimmed String (not the original) when calling
save_device(label, ...), update_saved_device_metadata(label, ...),
remove_device(label), and any confirmation prints (e.g., the DeviceSave flow in
run_one_shot). Ensure is_reserved_model_label/parse_device_model behavior
remains unchanged and that save_device, update_saved_device_metadata, and
remove_device continue to accept the label types they currently use (convert to
&str or clone as needed).

In `@src/main.rs`:
- Around line 387-405: In find_matching_peripheral, per-peripheral backend
errors from central.peripheral(&addr).await and peripheral.properties().await
are currently propagated via ? which aborts the whole scan; change those awaits
to handle errors locally (use match/if let Err(...) { continue } or
map_err/log_and_continue) so a transient failure on one addr skips that
peripheral and the loop keeps draining events until timeout or a match; keep the
existing return Ok(Some((peripheral, discovered))) path and ensure the loop
still returns Ok(None) on timeout and preserves the outer anyhow::Error result
type.

---

Nitpick comments:
In `@src/config.rs`:
- Around line 66-93: The save_to function currently truncates the target
(OpenOptions::truncate(true)) and streams contents directly which can leave
config.toml empty/partial on crash; change save_to to write to a temporary file
in the same directory (e.g. create a temp PathBuf next to path), write the
serialized contents to that temp file (use OpenOptions without truncating the
real file), set the file mode/permissions on unix (the same 0o600 logic
currently applied to options/file), flush and sync data (fsync) on the temp
file, then atomically rename (fs::rename) the temp file to path so the original
file remains intact until the new file is fully persisted; ensure error contexts
remain (with_context) and remove the temp file on failure.

In `@src/loader/parser.rs`:
- Around line 37-53: Replace fragile prefix-based classification by making
CommandError the canonical source: update classify (fn classify(error:
anyhow::Error) -> Self) to first try error.downcast_ref::<CommandError>() (or
error.downcast::<CommandError>()) and return the concrete CommandError if
present; only then (for compatibility) fall back to
is_invalid_argument_message(message) as a last resort. Add/ensure a constructor
like CommandError::invalid_arguments(...) (or
CommandError::InvalidArguments(...) conversion helper) that command handlers use
to return argument errors directly (convert to anyhow::Error where needed). Also
update command handlers to return CommandError::invalid_arguments(...) for
argument validation failures so classify_command_error in main.rs can rely on
downcast_ref::<CommandError>() and you can eventually remove the brittle
is_invalid_argument_message checks.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0e970f2-9635-4896-9fc0-3abaa84875a7

📥 Commits

Reviewing files that changed from the base of the PR and between dd73361 and ac56870.

📒 Files selected for processing (4)
  • src/config.rs
  • src/loader/cmds/device.rs
  • src/loader/parser.rs
  • src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/loader/cmds/device.rs

Comment thread src/config.rs
Comment thread src/main.rs
@nonnil
Copy link
Copy Markdown
Member Author

nonnil commented Apr 22, 2026

Handled the CodeRabbit review-body nitpicks in 6865ad5 as well:

  • Config persistence now writes to a temporary file next to config.toml, applies the restrictive Unix permissions to that temp file, calls sync_all(), then replaces the target with rename; failed writes clean up the temp file.
  • CommandError::classify now downcasts existing CommandError values before falling back to legacy message-prefix classification, and command argument-validation paths now return CommandError::InvalidArguments through a shared helper.

Verified with:

  • cargo fmt --check
  • cargo check
  • cargo test
  • cargo clippy -- -D warnings
  • cargo test --all-targets --all-features
  • cargo clippy --all-targets --all-features -- -D warnings
  • git diff --check

Copy link
Copy Markdown
Member Author

@nonnil nonnil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member Author

@nonnil nonnil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nonnil nonnil merged commit c96dae5 into main Apr 22, 2026
1 check passed
@nonnil nonnil deleted the feat/issue-5-one-shot-cli branch April 29, 2026 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: non-interactive (one-shot) CLI mode with auto-connect and device memory

2 participants