Add end-to-end CLI integration tests #127
Conversation
WalkthroughAdds dev-only crypto deps and a comprehensive CLI integration test suite; introduces env-driven nsec/hidden-password input; parameterizes Argon2 KDF through Keep/HiddenStorage creation APIs; and updates FROST signing to accept either an Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as keep-cli (user)
participant Keep as Keep library
participant Storage as Local Storage (shares)
participant Warden as Warden (optional)
participant Signer as Local partial signer
CLI->>Keep: cmd_frost_sign(group_id, message_hex)
alt group_id starts with "npub1"
Keep->>Signer: parse npub → group_pubkey
else group_id is name
Keep->>Storage: lookup group by name
Storage-->>Keep: group_pubkey (or not found)
end
alt warden_url provided
Keep->>Warden: policy-check(group_pubkey)
Warden-->>Keep: allow / deny
end
Keep->>Signer: perform FROST signing with resolved group_pubkey
Signer-->>CLI: signature or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ 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)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-cli/src/commands/frost.rs (1)
357-362: Move group_id resolution before warden policy check to pass the resolved npub.The warden policy check receives
group_iddirectly, which can be a group name rather than an npub. The function parameter is namedgroup_npub, indicating it expects an npub format. However, group name-to-pubkey resolution happens later (lines 381–390), after the policy check. If the user provides a group name instead of an npub, the warden service receives the name string instead of the resolved public key, likely causing the policy check to fail.Resolve the group identifier before the warden check to ensure the service receives the correct npub format. This requires moving the group lookup logic before the policy check and passing the resolved npub to
check_warden_policy.
🧹 Nitpick comments (2)
keep-cli/tests/cli_integration.rs (1)
10-32: Consider usingenv!("CARGO_BIN_EXE_keep")for binary discovery.The current binary discovery logic traverses multiple paths. Cargo provides
env!("CARGO_BIN_EXE_<name>")at compile time for integration tests, which is more reliable.However, the current approach works and handles edge cases where the macro might not be available. This is acceptable as-is.
Alternative approach using Cargo's built-in macro
fn keep_binary() -> Option<PathBuf> { // Try Cargo's compile-time path first let cargo_path = option_env!("CARGO_BIN_EXE_keep"); if let Some(p) = cargo_path { let path = PathBuf::from(p); if path.exists() { return Some(path); } } // Fall back to current discovery logic // ... existing code ... }keep-cli/src/commands/vault.rs (1)
253-333: Consider extracting common patterns to reduce duplication.The import functions (
cmd_import,cmd_import_outer,cmd_import_hidden) share significant structural similarity—open storage, unlock, get nsec, create keypair, encrypt and store. The same pattern appears in generate, export, and delete operations.A helper or trait-based abstraction could reduce this duplication while maintaining the distinct unlock semantics for each volume type. This is not urgent but could improve long-term maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
keep-cli/src/commands/frost.rskeep-cli/src/commands/mod.rskeep-cli/src/commands/vault.rskeep-cli/tests/cli_integration.rskeep-core/src/hidden/volume.rskeep-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
keep-core/src/lib.rs (1)
keep-core/src/storage.rs (2)
path(334-336)create(122-165)
keep-core/src/hidden/volume.rs (3)
keep-core/src/hidden/header.rs (2)
new(35-52)new(182-198)keep-core/src/crypto.rs (5)
new(30-46)new(101-115)new(159-166)new(218-225)derive_key(264-285)keep-core/src/storage.rs (3)
new(38-51)create(122-165)path(334-336)
keep-cli/src/commands/frost.rs (2)
keep-frost-net/src/node.rs (1)
group_pubkey(236-238)keep-core/src/keys.rs (1)
npub_to_bytes(148-162)
⏰ 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: build
🔇 Additional comments (14)
keep-cli/src/commands/mod.rs (1)
52-63: LGTM!The
get_nsecfunction follows the established pattern ofget_passwordand correctly handles both environment variable and interactive input paths. UsingPasswordprompt ensures the nsec is not displayed during input.keep-core/src/lib.rs (1)
39-51: LGTM!Good refactoring that introduces
create_with_paramswhile preserving backward compatibility. The delegation pattern keepscreate()unchanged for existing callers while enabling parameterized creation for tests and special use cases.keep-cli/tests/cli_integration.rs (4)
84-99: Polling loop is functional but could usewait_timeoutif available.The busy-wait polling with 100ms sleep works, but
std::process::Childdoesn't have a nativewait_timeout. The current implementation is acceptable for integration tests.
124-153: LGTM!Comprehensive workflow test covering the full lifecycle of init → generate → list → export. Good use of assertion helpers.
252-306: LGTM!Good FROST signing test. The message hash using SHA-256 is appropriate, and the signature validation correctly checks for a 64-byte hex-encoded output (128 characters).
308-350: LGTM!Excellent test for hidden volume isolation. Verifies that outer and hidden volumes maintain separate key spaces and that keys in one volume are not visible in the other.
keep-cli/src/commands/frost.rs (1)
379-392: LGTM - Good UX improvement for group identification.The dual-mode resolution (npub or name) improves usability. The
npub1prefix check correctly identifies bech32-encoded public keys.keep-core/src/hidden/volume.rs (3)
68-75: LGTM - Good API extension for parameterized creation.Adding
Argon2Paramsto thecreatesignature enables testing with lightweight parameters while maintaining production-strength defaults through the caller.
301-305: Design note: Hidden volume inherits Argon2 params from outer header.The hidden unlock path uses
self.outer_header.argon2_params(). This is consistent with the creation flow where both volumes are created with the sameparamsargument. The design ensures both volumes have matching security parameters.
651-677: LGTM!Test correctly updated to use
Argon2Params::TESTINGfor faster execution while maintaining the same test coverage.keep-cli/src/commands/vault.rs (4)
8-8: LGTM!Import additions are appropriate for the new parameterized Argon2 support and centralized nsec input handling.
Also applies to: 15-15
76-81: Appropriate test configuration pattern.Using environment-based Argon2 parameter selection is a reasonable approach for integration tests. The debug log provides useful visibility when lightweight parameters are active.
Minor note: Consider documenting in the README or contributing guide that
KEEP_TESTING_MODEmust never be set in production environments, asArgon2Params::TESTINGsignificantly weakens key derivation security.
83-94: LGTM!The
paramsargument is correctly propagated to bothHiddenStorage::createandKeep::create_with_params, ensuring consistent Argon2 parameter usage across hidden and regular vault creation paths.
236-236: LGTM!Consistent use of
get_nsecacross all import flows (cmd_import,cmd_import_outer,cmd_import_hidden) improves maintainability and likely enables environment-based nsec input for integration testing.Also applies to: 267-267, 308-308
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
Test plan
cargo test -p keep-cliSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.