test(restore-session): raise mutation score with pure-fn extraction#826
Open
ToRyVand wants to merge 5 commits into
Open
test(restore-session): raise mutation score with pure-fn extraction#826ToRyVand wants to merge 5 commits into
ToRyVand wants to merge 5 commits into
Conversation
Follow-up of MostroP2P#618. cargo mutants on src/app/restore_session.rs went from 33.3% (4/12) to 83.3% (10/12): - Extract pubkey-hex validation into util::is_valid_hex_pubkey, shared with the identical check already duplicated 5x in db.rs, instead of adding a 6th private copy. - Extract RESTORE_SESSION_TIMEOUT_SECS as a named constant so the 1h timeout computation is directly testable. - Add focused tests for send_restore_session_response/_timeout's early error path on an invalid trade key (no DB/network needed). The 2 remaining survivors replace restore_session_action and handle_restore_session_results wholesale; killing them needs a full AppContext (real pool + nostr client + settings), which is out of proportion for this issue and consistent with admin_add_solver_action being untested at that level too. Closes MostroP2P#637
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes add shared hexadecimal pubkey validation, apply it to restore-session keys, centralize the restore timeout, and route mutation testing through a shared Make target with bounded concurrency. ChangesRestore session validation
Mutation testing infrastructure
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Resolves conflict with MostroP2P#803 (merged upstream while this branch was in progress), which added a full TestContextBuilder-based test suite for restore_session_action/handle_restore_session_results — the exact gap this branch had documented as out of scope. Kept MostroP2P#803's tests as-is and folded in the one non-redundant test this branch added (timeout constant value).
Matches the existing .idea/.vscode/.cursor pattern — this holds per-machine tool permissions, not project config.
Uncapped parallel jobs + per-test thread fan-out exhausted RAM and crashed the machine during a local run. Cap via CARGO_MUTANTS_JOBS=2 (Makefile, verified with strace since .cargo/config.toml's [env] does not propagate to third-party subcommands) and --test-threads=4 (.cargo/mutants.toml). Both CI mutation jobs now go through the same `make mutation-test` target.
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/restore_session.rs (3)
1-1: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScrub Nostr keys from logs.
Logging
trade_keyexposes a Nostr key in the application logs. As per coding guidelines, logs that could expose invoices or Nostr keys must be scrubbed to protect user privacy.
src/app/restore_session.rs#L90-112: Removetrade_keyfrom thetracing::info!log insend_restore_session_response.src/app/restore_session.rs#L115-125: Removetrade_keyfrom thetracing::warn!log insend_restore_session_timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/restore_session.rs` at line 1, Remove the trade_key field from the tracing::info! call in send_restore_session_response and the tracing::warn! call in send_restore_session_timeout, while preserving the remaining contextual log fields and messages.Source: Coding guidelines
115-125: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScrub Nostr keys from logs.
Logging
trade_keyexposes a Nostr key in the logs. As per coding guidelines, logs that could expose invoices or Nostr keys must be scrubbed to protect user privacy.🛡️ Proposed fix to remove the key from the log
- tracing::warn!("Restore session timed out for user: {}", trade_key); + tracing::warn!("Restore session timed out for user");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/restore_session.rs` around lines 115 - 125, Remove the trade_key value from the tracing::warn! call in send_restore_session_timeout, while preserving the timeout warning message and existing restore-session behavior. Do not log the parsed trade_pubkey or any other Nostr key.Source: Coding guidelines
90-112: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScrub Nostr keys from logs.
Logging
trade_keyexposes a Nostr key in the logs. As per coding guidelines, logs that could expose invoices or Nostr keys must be scrubbed to protect user privacy.🛡️ Proposed fix to remove the key from the log
- tracing::info!("Restore session response sent to user {}", trade_key,); + tracing::info!("Restore session response sent to user");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/restore_session.rs` around lines 90 - 112, Remove the raw trade_key value from the tracing::info call in send_restore_session_response, while retaining a generic message that the restore session response was sent. Do not alter the key parsing or response-enqueue behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 71: Prevent shell injection in the mutation-test invocation: in Makefile
lines 71-71, construct argv-safe quoted arguments instead of expanding ARGS
directly; in .github/workflows/mutation.yml lines 71-71, pass changed filenames
via a safe file-list or argument mechanism. Preserve mutation testing for all
changed files without allowing filename contents to execute as shell syntax.
---
Outside diff comments:
In `@src/app/restore_session.rs`:
- Line 1: Remove the trade_key field from the tracing::info! call in
send_restore_session_response and the tracing::warn! call in
send_restore_session_timeout, while preserving the remaining contextual log
fields and messages.
- Around line 115-125: Remove the trade_key value from the tracing::warn! call
in send_restore_session_timeout, while preserving the timeout warning message
and existing restore-session behavior. Do not log the parsed trade_pubkey or any
other Nostr key.
- Around line 90-112: Remove the raw trade_key value from the tracing::info call
in send_restore_session_response, while retaining a generic message that the
restore session response was sent. Do not alter the key parsing or
response-enqueue behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea3577ba-9f63-4f28-8a6f-c1aaaf74d417
📒 Files selected for processing (6)
.cargo/mutants.toml.github/workflows/mutation.yml.gitignoreMakefilesrc/app/restore_session.rssrc/util.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util.rs
Filenames from `git diff --name-only` on a PR are attacker-controlled.
The old `file_args="$file_args --file $f"` + `make mutation-test
ARGS="$file_args"` path round-tripped that string through Make's macro
substitution and a second shell parse, so a crafted "filename" could
word-split into standalone argv tokens — argument injection into
cargo-mutants/cargo/rustc's own flag surface, not classic shell
command substitution (unquoted variable expansion doesn't re-parse
$()/backticks, but it does still word-split).
Fixed by building a bash array (`file_args+=(--file "$f")`) and
expanding it with "${file_args[@]}", so each filename — however it's
spelled — can only ever land as the single value of one --file flag.
This bypasses `make mutation-test` for this call site specifically;
its $(ARGS) stays a plain string splice, fine for human-typed input
(the label-triggered baseline job, unaffected, still uses it), not
for diff-derived filenames. Comment added to the Makefile target
so that distinction doesn't get lost later.
Found by CodeRabbit on PR MostroP2P#826's post-rebase re-review.
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.
Follow-up of #618 / closes #637.
cargo mutants --file src/app/restore_session.rswent from 33.3% (4/12) to 83.3% (10/12).Changes
util::is_valid_hex_pubkey. It replaces two inline checks inrestore_session_action, and is shared with the identical check already duplicated 5x indb.rs(lines 928, 973, 1014, 1197, 1229) instead of adding a 6th private copy.RESTORE_SESSION_TIMEOUT_SECSas a named constant so the 1h timeout computation (60 * 60) is directly testable without instantiating the session manager.send_restore_session_response/send_restore_session_timeout's early error path on an invalid trade key — no DB/network dependency needed, since both functions returnErrbefore reachingenqueue_restore_session_msg.Remaining survivors
The 2 mutants still surviving replace
restore_session_actionandhandle_restore_session_resultswholesale (-> Ok(())/-> ()). Killing them needs a fullAppContext(real Sqlite pool +nostr_sdk::Client+Settings+OrderMsgQueue) constructed in a test, which is out of proportion for this issue and consistent withadmin_add_solver_actionbeing untested at that same level elsewhere in the codebase — only its inner pureparse_solver_payloadhas unit tests.db.rs's 5 existing copies of the hex-pubkey check were left as-is (not migrated to the new shared helper) — they're unrelated functions outside this issue's scope; migrating them is its own small refactor with its own blast radius.Test plan
cargo fmt --all -- --checkcargo clippy --all-targets --all-features— cleancargo test— 537 passed, 1 failed (test_lnurl_validation_with_test_server, pre-existing/unrelated — fails on port 8080 already in use in this environment, reproduces onmainwithout this diff too)cargo mutants --file src/app/restore_session.rs: 83.3% (10/12), up from 33.3%cargo mutantson the newutil::is_valid_hex_pubkey: 100% (4/4)Summary by CodeRabbit