mostrix first run with automatic settings.toml#41
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. WalkthroughAdds first-run settings auto-generation using an embedded defaults template (generates a Nostr keypair, writes settings.toml, validates configs) and refactors settings load paths; also reorganizes Changes
Sequence DiagramsequenceDiagram
actor User
participant App as Application Startup
participant Config as Settings Module
participant FS as File System
participant Nostr as Nostr SDK
User->>App: run binary
App->>Config: load_settings_from_disk()
Config->>FS: check executable-adjacent `settings.toml`
alt found
FS-->>Config: return path
Config->>Config: deserialize & validate
Config-->>App: settings loaded
else not found
Config->>FS: check legacy `~/.mostrix/settings.toml`
alt legacy found
FS-->>Config: return path
Config->>Config: deserialize & validate
Config-->>App: settings loaded
else first run
Config->>Nostr: generate keypair (nsec + npub)
Nostr-->>Config: new keys
Config->>Config: populate embedded defaults + generated values
Config->>FS: create settings.toml (write, set mode 600)
FS-->>Config: file created
Config-->>App: settings generated (prints npub)
end
end
App-->>User: startup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Contributor
There was a problem hiding this comment.
Review Summary
✅ Checks Passed
- CI Status: Passing on latest commit (930b4c8)
- Implements #40: Auto-generates
settings.tomlon first run - Security: Uses
0o600permissions on Unix for the generated file - UX: Prints generated npub so user knows their identity
- Backwards compatible: Existing configs still work, placeholder guard kept
👍 Good Implementation Details
- Uses
include_str!to embed default template at compile time create_dir_allinstead ofcreate_dir(handles nested paths)create_new(true)prevents overwriting existing files (race condition safe)- Cross-platform: Unix gets
0o600, others usefs::write - Generates fresh Nostr keypair with
Keys::generate() - Clear error message if legacy file has placeholder values
⚠️ Minor Observations (non-blocking)
-
Issue #40 asked for binary-relative lookup first:
"1. look for the settings.toml in the same directory where the binary file"
This PR only checks
~/.mostrix/. Not a blocker since~/.mostrix/is a reasonable canonical location, but worth noting the deviation from the original spec. -
No test for the new generation logic:
The auto-generation path isn't unit tested. Consider adding a test that:- Creates a temp dir
- Calls the init logic
- Verifies file is created with correct defaults
This can be a follow-up.
📋 Checklist
- CI passes
- Implements the issue requirements
- Secure file permissions
- Backwards compatible
- User-friendly (prints npub)
- Binary-relative lookup (spec deviation, non-blocking)
- Unit test for generation path (can be follow-up)
Verdict
LGTM — Clean implementation that solves the first-run UX problem. The spec deviations are minor and the implementation is solid.
🦀
Contributor
There was a problem hiding this comment.
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/settings.rs`:
- Around line 106-111: The code only checks hidden_file
(~/.{package}/settings.toml) and never probes the executable directory for a
bundled settings.toml; update the path discovery to also build an executable_dir
candidate (resolve the current executable via std::env::current_exe(), get its
parent, join "settings.toml") and pass that path through the same placeholder
guard/validation used for hidden_file (the same load/placeholder-check logic
around hidden_dir/hidden_file and wherever the 129-151 block repeats) so a
portable install with settings.toml next to the binary is recognized before
falling back to "no config anywhere".
- Around line 129-205: There is a TOCTOU when writing hidden_file: replace the
non-atomic write path with an atomic create_new attempt and, on AlreadyExists,
fall back to loading the just-created file and reusing the same
placeholder-guarded validation you have earlier; specifically, when persisting
the generated settings (the block that currently writes toml_string to
hidden_file), use OpenOptions::new().create_new(true).write(true) (or create a
temp file and fs::rename for non-unix) so the write fails atomically if another
process won the race, and on Err(e) where e.kind() == AlreadyExists call
load_settings_from_path(&hidden_file) and perform the same placeholder checks
(mostro_pubkey, nsec_privkey) and return those settings instead of the in-memory
generated Settings/NKeys (Keys::generate(), nsec, npub).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 938606b7-f651-445f-9c30-bb7e921ca54c
📒 Files selected for processing (2)
.gitignoresrc/settings.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/settings.rs (1)
301-317:⚠️ Potential issue | 🟡 Minor
save_settingsmay not preserve 0600 permissions on Unix.The initial file creation uses
.mode(0o600)for security, butsave_settingsusesfs::write(), which on some systems may reset permissions to the default umask (often 0644). This could expose thensec_privkeyto other users on multi-user systems.Suggested fix to preserve permissions
+#[cfg(unix)] +fn write_settings_file(path: &std::path::Path, content: &str) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let mut file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path)?; + file.write_all(content.as_bytes()) +} + +#[cfg(not(unix))] +fn write_settings_file(path: &std::path::Path, content: &str) -> std::io::Result<()> { + fs::write(path, content) +} + pub fn save_settings(settings: &Settings) -> Result<(), anyhow::Error> { // ... - fs::write(&hidden_file, toml_string) + write_settings_file(&hidden_file, &toml_string) .map_err(|e| anyhow::anyhow!("Failed to write settings file: {}", e))?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings.rs` around lines 301 - 317, save_settings currently uses fs::write which can create the file with default umask (e.g., 0644) and expose secrets; change it to open the hidden_file with std::fs::OpenOptions and on Unix use std::os::unix::fs::OpenOptionsExt::mode(0o600) when creating/truncating, write the toml bytes via the opened file (and flush), and if the file already exists check its metadata().permissions().mode() and call fs::set_permissions to enforce 0o600 when it differs; update references in save_settings (hidden_file, toml_string) and add the necessary use of OpenOptions/Unix extension.
🧹 Nitpick comments (1)
src/settings.rs (1)
209-285: Consider consolidating the Unix/non-Unix file write logic.The two
#[cfg]blocks are nearly identical (~35 lines each), differing only in the.mode(0o600)call. This duplication increases maintenance burden.Suggested refactor direction
use std::io::{ErrorKind, Write}; let write_result: std::io::Result<()> = (|| { #[cfg(unix)] let file = { use std::os::unix::fs::OpenOptionsExt; fs::OpenOptions::new() .write(true) .create_new(true) .mode(0o600) .open(&hidden_file) }; #[cfg(not(unix))] let file = fs::OpenOptions::new() .write(true) .create_new(true) .open(&hidden_file); file?.write_all(toml_string.as_bytes()) })(); match write_result { Ok(()) => {} Err(e) if e.kind() == ErrorKind::AlreadyExists => { let settings = load_settings_from_path(&hidden_file)?; // placeholder guard... return Ok(settings); } Err(e) => { anyhow::bail!("Could not write generated settings.toml: {}", e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings.rs` around lines 209 - 285, The two cfg-specific blocks duplicate almost identical logic writing toml_string to hidden_file; consolidate by creating a single write attempt that uses a cfg-only OpenOptions variant for the mode (e.g. assign a cfg-defined let file = ... using OpenOptionsExt::mode(0o600) under #[cfg(unix)] and a plain OpenOptions under #[cfg(not(unix)]; then perform file?.write_all(toml_string.as_bytes()) inside a small Result-returning closure or block, capture the Result as write_result, and reuse the existing match handling (Err(e) if e.kind() == ErrorKind::AlreadyExists -> load_settings_from_path(&hidden_file) and placeholder checks on settings.mostro_pubkey/settings.nsec_privkey, other Err -> anyhow::bail!). Ensure you still import ErrorKind and Write where needed and preserve the exact error messages and placeholder guard logic.
🤖 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/settings.rs`:
- Around line 250-285: The non-Unix branch uses file.write_all(...) but doesn't
import the std::io::Write trait into that #[cfg(not(unix))] block, causing a
compile error; add use std::io::Write; inside the same #[cfg(not(unix))] scope
(near where hidden_file is opened and file is matched) so file.write_all(...)
resolves, leaving the rest of the logic (load_settings_from_path, checking
settings.mostro_pubkey/nsec_privkey and returning settings) unchanged.
---
Outside diff comments:
In `@src/settings.rs`:
- Around line 301-317: save_settings currently uses fs::write which can create
the file with default umask (e.g., 0644) and expose secrets; change it to open
the hidden_file with std::fs::OpenOptions and on Unix use
std::os::unix::fs::OpenOptionsExt::mode(0o600) when creating/truncating, write
the toml bytes via the opened file (and flush), and if the file already exists
check its metadata().permissions().mode() and call fs::set_permissions to
enforce 0o600 when it differs; update references in save_settings (hidden_file,
toml_string) and add the necessary use of OpenOptions/Unix extension.
---
Nitpick comments:
In `@src/settings.rs`:
- Around line 209-285: The two cfg-specific blocks duplicate almost identical
logic writing toml_string to hidden_file; consolidate by creating a single write
attempt that uses a cfg-only OpenOptions variant for the mode (e.g. assign a
cfg-defined let file = ... using OpenOptionsExt::mode(0o600) under #[cfg(unix)]
and a plain OpenOptions under #[cfg(not(unix)]; then perform
file?.write_all(toml_string.as_bytes()) inside a small Result-returning closure
or block, capture the Result as write_result, and reuse the existing match
handling (Err(e) if e.kind() == ErrorKind::AlreadyExists ->
load_settings_from_path(&hidden_file) and placeholder checks on
settings.mostro_pubkey/settings.nsec_privkey, other Err -> anyhow::bail!).
Ensure you still import ErrorKind and Write where needed and preserve the exact
error messages and placeholder guard logic.
mostronatorcoder Bot
added a commit
that referenced
this pull request
Mar 19, 2026
Updates documentation to reflect the new first-run behavior implemented in PR #41: ## Changes ### Settings section rewritten - Documented zero-config first run experience - Listed auto-generated defaults (keypair, relay, user_mode, etc.) - Explained configuration priority (user dir > portable > auto-generate) - Added note about 0600 file permissions on Unix ### Example settings.toml updated - Removed placeholder values that suggested manual editing - Shows actual defaults used by auto-generation - Added note explaining values are auto-generated ### TODO list - Marked auto-generate feature as complete with issue link ## Related - Closes #40 (via PR #41) - Documents behavior from PR #41
6 tasks
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
3 participants
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 #40: Mostrix no longer fails on first run when
settings.tomlis missing. On first launch, it auto-generates a working~/.mostrix/settings.tomlwith sensible defaults.What changed
~/.mostrix/settings.toml.nsec_privkeyrelays = ["wss://relay.mostro.network"]user_mode = "user"pow = 0currencies_filter = []mostro_pubkey = "82fa8cb978b43c79b2156585bac2c022276a21d2aead6d9f7c575c005be88390"~/.mostrix/settings.toml(with0600permissions on Unix)npubto stdout so the user can record itCompatibility
~/.mostrix/settings.tomlalready exists, it is loaded normally.Testing
cargo test~/.mostrix/settings.toml, runmostrix, confirm the file is created and the app starts.Summary by CodeRabbit
New Features
Bug Fixes
Chores