Conversation
|
Warning Rate limit exceeded@alexandretrotel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 52 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds file mismatch validation (v2.2.0): compares current config files against backups in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidateTask
participant ConfigValidator
participant FileMismatchValidator
participant BackupStore as "~/.mntn/backup"
participant EncryptedRegistry
participant Filesystem
participant Reporter
User->>ValidateTask: trigger validate/dry-run
ValidateTask->>ConfigValidator: build validators
ConfigValidator->>FileMismatchValidator: invoke validate()
FileMismatchValidator->>BackupStore: read backup file
alt backup is encrypted
FileMismatchValidator->>EncryptedRegistry: request decryption (prompt password)
EncryptedRegistry->>FileMismatchValidator: decrypted content
end
FileMismatchValidator->>Filesystem: read current file
FileMismatchValidator->>FileMismatchValidator: compare backup vs current
FileMismatchValidator->>Reporter: emit ValidationError or OK
Reporter->>User: display results / planned operation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
392-437: Clarify unconditional password prompting in encrypted file validation.The README text suggests users are prompted for a password "for encrypted registry entries," but the implementation prompts unconditionally whenever validation runs, even if no encrypted entries have backups to compare. Unlike
backupandrestorecommands (which have--skip-encryptedgating),validatecallsprompt_password()before checking whether any encrypted entries are actually checkable. The documentation should clarify this upfront behavior.Regarding storage structure: The documented path layout (
~/.mntn/backup/common/encrypted/ssh/and profile-specific variants) correctly matches the implementation'sresolve_encrypted_source()function and backup behavior.
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Around line 5-9: The changelog claim causes confusion: the `validate` command
currently triggers a password prompt as soon as encrypted registry loading
succeeds even if there are no enabled encrypted entries with applicable
backups/targets to compare; change the implementation so `validate` only
initiates a password prompt after discovering at least one enabled encrypted
registry entry that has an applicable backup/target (and will be compared),
otherwise skip prompting (or alternatively update the wording to state prompts
occur only when such entries exist). Locate the logic in `validate` that loads
the encrypted registry and where it calls the password prompt, and guard the
prompt behind a check for enabled encrypted entries with matching
backups/targets before asking for a password.
In @src/tasks/validate.rs:
- Around line 348-568: The validate() implementation currently calls
prompt_password() unconditionally and treats certain decrypt errors by returning
early; instead, first scan EncryptedConfigsRegistry::get_enabled_entries() and
use Profile::resolve_encrypted_source (same logic as later) to determine if at
least one encrypted entry has both a target_path.exists() and
resolved.path.exists() before calling prompt_password(), then prompt only when
needed; for decrypt errors, avoid fragile substring matching on decrypt_file()
errors—either update decrypt_file() to return a typed DecryptError (e.g.,
DecryptError::WrongPassword vs DecryptError::Other) and handle WrongPassword by
pushing a skip-warning and breaking/returning, or if you cannot change
decrypt_file() now, treat any decryption failure per-entry as a warning and
continue to the next entry (do not return early); update the code paths around
decrypt_file(), prompt_password(), and the loop over encrypted entries
accordingly.
- Around line 348-568: The encrypted files are being stored as
encrypted/<hash>.age because get_encrypted_path currently hashes the full source
path and drops parent directories when encrypt_filename=true; either update the
README or change get_encrypted_path to preserve parent directories: keep the
original parent-relative directory (e.g., "ssh/") and return
encrypted/<parent_dir>/<hash>.age (compute the hash from the original relative
path or filename as before), ensuring resolve_encrypted_source and any code that
expects the encrypted path use this new layout and that directories are created
as needed when writing encrypted backups.
🧹 Nitpick comments (1)
src/tasks/validate.rs (1)
376-385: Directory entries are silently skipped — consider surfacing an Info/Warning.
If registries can include directories, skipping them without any signal can confuse users (“why didn’t validate detect drift?”). Either document “files only” or add anInforesult when an enabled entry is a directory.Also applies to: 461-469
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mdCargo.tomlREADME.mdsrc/tasks/validate.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:01:32.900Z
Learnt from: alexandretrotel
Repo: alexandretrotel/mntn PR: 3
File: src/registries/configs_registry.rs:125-135
Timestamp: 2025-12-18T16:01:32.900Z
Learning: In src/registries/configs_registry.rs, the default registry entries are meant to be extensible and customizable by users. The predefined paths (like VSCode settings) are starting points, not platform-specific production defaults, so cross-platform path differences are acceptable.
Applied to files:
README.mdsrc/tasks/validate.rs
🧬 Code graph analysis (1)
src/tasks/validate.rs (2)
src/encryption.rs (3)
decrypt_file(52-82)get_encrypted_path(96-104)prompt_password(10-26)src/utils/paths.rs (1)
get_encrypted_registry_path(61-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Test (windows-latest, stable)
🔇 Additional comments (4)
Cargo.toml (1)
3-3: Version bump looks consistent with the feature addition.src/tasks/validate.rs (2)
1-15: New imports are fine; ensure encrypted validation stays non-intrusive.
Main concern isn’t the imports themselves, but that the newly-added encrypted validation path can force interactivity invalidate(see below).
655-656: Wiring + dry-run + test expectation updates look correct.
Validator is added toConfigValidator::new(),dry_run()includes the planned op, and tests reflect the new counts.Also applies to: 715-716, 954-960, 966-968
README.md (1)
53-54: Feature list update is clear.
… password only when necessary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/tasks/validate.rs (2)
550-569: Consider using typed error handling if available.The string-based error detection (
error_msg.contains("password")) is fragile and could break if the encryption library changes its error messages. If the underlying encryption library provides typed errors or error codes, consider matching on those instead.That said, the current implementation degrades gracefully—the user would just see the raw error message in the worst case.
740-1016: Consider adding unit tests forFileMismatchValidator.The test file covers all other validators (
JsonConfigValidator,LegacySymlinkValidator,LayerValidator,RegistryValidator) with at least aname()test, butFileMismatchValidatorlacks dedicated unit tests. Consider adding:
- A test for
FileMismatchValidator::name()returning "File Mismatch Check"- Tests for mismatch detection with mock files (where feasible without encryption complexity)
Example test for validator name
#[test] fn test_file_mismatch_validator_name() { let profile = ActiveProfile::common_only(); let validator = FileMismatchValidator::new(profile); assert_eq!(validator.name(), "File Mismatch Check"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tasks/validate.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:01:32.900Z
Learnt from: alexandretrotel
Repo: alexandretrotel/mntn PR: 3
File: src/registries/configs_registry.rs:125-135
Timestamp: 2025-12-18T16:01:32.900Z
Learning: In src/registries/configs_registry.rs, the default registry entries are meant to be extensible and customizable by users. The predefined paths (like VSCode settings) are starting points, not platform-specific production defaults, so cross-platform path differences are acceptable.
Applied to files:
src/tasks/validate.rs
🧬 Code graph analysis (1)
src/tasks/validate.rs (2)
src/encryption.rs (3)
decrypt_file(52-82)get_encrypted_path(96-104)prompt_password(10-26)src/utils/paths.rs (2)
get_encrypted_registry_path(61-63)get_registry_path(46-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (windows-latest, stable)
🔇 Additional comments (5)
src/tasks/validate.rs (5)
1-14: LGTM!The new imports are appropriately added for the file mismatch validation feature, including encryption utilities, registry handling, and temporary file management.
348-357: LGTM!The struct follows the same pattern as the existing validators (
JsonConfigValidator,LayerValidator), maintaining consistency in the codebase.
376-432: LGTM!The regular config validation logic handles edge cases well (skipping non-existent files and directories), uses appropriate byte-level comparison, and provides helpful fix suggestions.
448-571: LGTM!Good design choices:
- Collecting entries first before prompting for password avoids unnecessary prompts when no encrypted backups exist
- Password is prompted only once and reused across all entries
- Temporary file cleanup is handled automatically via
NamedTempFile's Drop- Early return on password error prevents repeated failures for all entries while preserving already-collected errors
666-666: LGTM!The new validator is correctly integrated into the validation pipeline following the established pattern.
…ncrypted path generation
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.