feat: fixed error on first launch of mostrix on windows, now user is guided#39
Open
arkanoider wants to merge 2 commits intomainfrom
Open
feat: fixed error on first launch of mostrix on windows, now user is guided#39arkanoider wants to merge 2 commits intomainfrom
arkanoider wants to merge 2 commits intomainfrom
Conversation
Contributor
WalkthroughRemoved a Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SettingsModule as Settings
participant FS as Filesystem
participant User
App->>Settings: call init_settings()
Settings->>FS: check config path exists
alt config missing
Settings->>FS: write embedded DEFAULT_SETTINGS_TOML (create file)
FS-->>Settings: write OK
Settings->>App: log "default settings written"
else config exists
Settings->>FS: read config
FS-->>Settings: send contents
end
Settings->>Settings: deserialize via cfg.try_deserialize()
Settings->>Settings: validate mostro_pubkey / nsec_privkey placeholders
alt placeholders detected
Settings->>App: return error with guidance (first-run vs existing)
else valid
Settings->>App: return &'static Settings
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 145-148: The current user-facing message always says a default
settings.toml "has been created" using path_display even when the file already
existed; adjust the control flow that emits that message to check the actual
file creation outcome (e.g., the Result or bool returned from the create/write
call or using path.exists()) and branch: if the write actually created the file
emit the existing "Default settings.toml has been created at {}..." message,
otherwise emit a different message (e.g., "Default settings.toml already exists
at {}. Please edit it to replace placeholder values...") while keeping the
existing fail-fast check logic intact; update the code that constructs/emits the
message that currently uses path_display to use this conditional wording.
- Around line 119-123: When creating the default settings.toml (the block using
hidden_file and DEFAULT_SETTINGS_TOML in src/settings.rs), replace fs::write()
on Unix with opening the file via std::fs::OpenOptions and setting the file mode
to 0o600 (use std::os::unix::fs::OpenOptionsExt::set_mode) before writing the
contents, preserving the existing error mapping (map_err) and the println; keep
the existing fs::write() fallback for non-Unix targets (cfg(unix) /
cfg(not(unix))) so Windows behavior is unchanged. Ensure you reference
hidden_file and DEFAULT_SETTINGS_TOML and write the full contents to the
OpenOptions-created file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 398212af-75c1-40f6-9447-a6b087c58ba2
📒 Files selected for processing (2)
.gitignoresrc/settings.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/settings.rs (1)
198-202: Consider using restrictive permissions insave_settingson Unix for consistency.While
fs::write()preserves existing file permissions when truncating, applying explicit0o600mode here would provide defense-in-depth if the file were ever deleted and recreated by this function.Optional hardening
let toml_string = toml::to_string_pretty(settings) .map_err(|e| anyhow::anyhow!("Failed to serialize settings: {}", e))?; - fs::write(&hidden_file, toml_string) - .map_err(|e| anyhow::anyhow!("Failed to write settings file: {}", e))?; + #[cfg(unix)] + { + 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(&hidden_file) + .map_err(|e| anyhow::anyhow!("Failed to write settings file: {}", e))?; + file.write_all(toml_string.as_bytes()) + .map_err(|e| anyhow::anyhow!("Failed to write settings file: {}", e))?; + } + #[cfg(not(unix))] + { + fs::write(&hidden_file, toml_string) + .map_err(|e| anyhow::anyhow!("Failed to write settings file: {}", e))?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings.rs` around lines 198 - 202, In save_settings replace the fs::write(&hidden_file, toml_string) call with opening the file via std::fs::OpenOptions (use OpenOptions::new().create(true).write(true).truncate(true)) and write the toml_string via write_all, and on Unix set explicit permissions by calling OpenOptionsExt::mode(0o600) (import std::os::unix::fs::OpenOptionsExt) so the created file gets restrictive 0o600 permissions; keep the same error mapping for write failures and use the same hidden_file and toml_string variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/settings.rs`:
- Around line 198-202: In save_settings replace the fs::write(&hidden_file,
toml_string) call with opening the file via std::fs::OpenOptions (use
OpenOptions::new().create(true).write(true).truncate(true)) and write the
toml_string via write_all, and on Unix set explicit permissions by calling
OpenOptionsExt::mode(0o600) (import std::os::unix::fs::OpenOptionsExt) so the
created file gets restrictive 0o600 permissions; keep the same error mapping for
write failures and use the same hidden_file and toml_string variables.
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
1 participant
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
Improve first-run settings handling so packaged binaries (especially on Windows) bootstrap configuration reliably and fail with a clear, user-friendly message when still using placeholders.
Details
settings.tomlinto the binary and write it to~/.mostrix/settings.tomlon first launch if the file is missing (no dependency on source-tree paths likeCARGO_MANIFEST_DIR).mostro_pubkey_hex_format,nsec1_privkey_format) and exit gracefully with an error that:settings.toml.Summary by CodeRabbit
New Features
Chores