Mutation testing for session restore module#664
Mutation testing for session restore module#664sergey3bv wants to merge 1 commit intoMostroP2P:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExtracts restore-session logic into a new public core function that accepts a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as UnwrappedGift
participant API as restore_session_action / restore_session_core
participant DB as Sqlite Pool
participant Manager as RestoreSessionManager
participant BG as Background task
Client->>API: call restore_session_action(event)
API->>API: validate master_key & trade_key
API->>DB: query orders/disputes by master_key
API->>Manager: create & start restore for master_key (with DB pool)
Manager->>BG: spawn background task to await results/timeouts
BG->>Manager: deliver session results or timeout
Manager->>DB: update state / mark terminal statuses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/restore_session.rs (1)
154-233: Consider replacing inline DDL setup with shared migration-based test bootstrap.The full schema copy in
setup_restore_dbis brittle; schema drift can make tests pass while production queries break after migration changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 154 - 233, The setup_restore_db function currently creates the schema inline using sqlx::query which is brittle; change it to initialize an in-memory SqlitePool (still via SqlitePoolOptions::new and connect) and then run your canonical migrations instead of the hard-coded DDL (e.g. use sqlx::migrate! or the project's existing migration runner to call .run(&pool) or run_migrations(&pool)); remove the inline CREATE TABLE queries in setup_restore_db and ensure it references the same migration path used in production so tests stay in sync with migrations.
🤖 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/app/restore_session.rs`:
- Around line 303-335: The tests under the "restore_session_action validation"
block are calling crate::db::find_user_orders_by_master_key directly instead of
exercising the restore_session_action entrypoint, so rename the test block
and/or the three test functions (rejects_invalid_master_key_short,
rejects_invalid_master_key_non_hex, rejects_invalid_master_key_too_long) to
reflect they validate DB key parsing (e.g., move them under a
"find_user_orders_by_master_key validation" module) or add new async tests that
call restore_session_action with the same short/non-hex/too-long master keys and
assert the same MostroCantDo(CantDoReason::InvalidPubkey) behavior; update test
names and module comments to clearly indicate which function is being exercised
(restore_session_action vs find_user_orders_by_master_key) to avoid misleading
coverage.
---
Nitpick comments:
In `@src/app/restore_session.rs`:
- Around line 154-233: The setup_restore_db function currently creates the
schema inline using sqlx::query which is brittle; change it to initialize an
in-memory SqlitePool (still via SqlitePoolOptions::new and connect) and then run
your canonical migrations instead of the hard-coded DDL (e.g. use sqlx::migrate!
or the project's existing migration runner to call .run(&pool) or
run_migrations(&pool)); remove the inline CREATE TABLE queries in
setup_restore_db and ensure it references the same migration path used in
production so tests stay in sync with migrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce0639aa-d9f6-41ba-bdfa-a2058462f2cf
📒 Files selected for processing (1)
src/app/restore_session.rs
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
from
dc7c9fa to
2bb0284
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/restore_session.rs (1)
154-233: Reduce schema drift risk insetup_restore_db.The full hand-written
ordersschema in Line 163 through Line 208 can drift from real migrations over time. Prefer a shared test DB initializer (or migration-backed setup) so restore-session tests track schema changes automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 154 - 233, The tests use a hand-written CREATE TABLE for the orders/disputes schema in setup_restore_db which can drift from real migrations; replace the manual SQL with your migration-backed initializer instead: create the in-memory pool in setup_restore_db as before, then run the project's SQLx migrations (e.g. sqlx::migrate!("migrations").run(&pool).await) or call the shared test DB initializer utility so the real migration files produce the orders and disputes tables rather than the hard-coded CREATE TABLE blocks; remove the inline CREATE TABLE queries for orders and disputes in setup_restore_db and ensure the migrations path or shared initializer is reachable in test runtime.
🤖 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/app/restore_session.rs`:
- Around line 154-233: The tests use a hand-written CREATE TABLE for the
orders/disputes schema in setup_restore_db which can drift from real migrations;
replace the manual SQL with your migration-backed initializer instead: create
the in-memory pool in setup_restore_db as before, then run the project's SQLx
migrations (e.g. sqlx::migrate!("migrations").run(&pool).await) or call the
shared test DB initializer utility so the real migration files produce the
orders and disputes tables rather than the hard-coded CREATE TABLE blocks;
remove the inline CREATE TABLE queries for orders and disputes in
setup_restore_db and ensure the migrations path or shared initializer is
reachable in test runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e643276-0af3-4fcc-b24f-f8c738fe2bf7
📒 Files selected for processing (1)
src/app/restore_session.rs
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
from
2bb0284 to
1801590
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/restore_session.rs (2)
316-324: This only checks the synchronousOk(())path.The assertion never waits for or inspects the spawned worker, so it cannot kill mutants in
handle_restore_session_results(), payload assembly, ortrade_keyrouting. For a mutation-testing PR, add a seam/assertion around the background path instead of only the initial return value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 316 - 324, The test restore_session_action_accepts_valid_keys only asserts the immediate Ok(()) return from restore_session_action and never verifies the spawned background worker, so mutations in handle_restore_session_results, payload assembly, or trade_key routing are not exercised; update the test to assert the background path by providing a test seam: either make restore_session_action return or expose a JoinHandle for the spawned task, or inject a mock channel/worker hook (e.g., via dependency injection) so the test can await the worker and assert expected side effects from handle_restore_session_results, then validate created payloads/routed trade_key actions before cleaning up the worker.
160-239: Run the real migrations instead of hand-rolling the test schema.This helper has already drifted from production (
orders.statusisvarchar(10)inmigrations/20221222153301_orders.sql, butvarchar(50)here). Bootstrapping the in-memory DB from the project migrations will keep restore-session tests aligned with future schema changes and constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 160 - 239, The setup_restore_db function is creating a hand-rolled test schema that has drifted from production; replace the manual CREATE TABLE queries with running the real project migrations after creating the in-memory Sqlite pool. Concretely, in setup_restore_db() (where SqlitePoolOptions::new().connect("sqlite::memory:") is used) remove the sqlx::query(...CREATE TABLE...) blocks and call the sqlx migration runner (e.g. sqlx::migrate!("migrations").run(&pool).await.unwrap() or use sqlx::migrate::Migrator::new(...) and .run(&pool).await) to apply the project's migrations to the pool; ensure the migrations path and sqlx features are enabled so tests can run migrations against the in-memory DB.
🤖 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/app/restore_session.rs`:
- Around line 339-342: In the test function rejects_non_hex_master_key, avoid
the hard-coded pseudo-secret by deriving the bad key at runtime from
VALID_MASTER_KEY: create a mutable copy of VALID_MASTER_KEY (used in this test
module), alter a character or append a non-hex char to ensure it is non-hex,
then pass that mutated string to crate::db::find_user_orders_by_master_key
(keeping setup_restore_db as-is); this prevents secret-scanner churn while
preserving the test intent.
---
Nitpick comments:
In `@src/app/restore_session.rs`:
- Around line 316-324: The test restore_session_action_accepts_valid_keys only
asserts the immediate Ok(()) return from restore_session_action and never
verifies the spawned background worker, so mutations in
handle_restore_session_results, payload assembly, or trade_key routing are not
exercised; update the test to assert the background path by providing a test
seam: either make restore_session_action return or expose a JoinHandle for the
spawned task, or inject a mock channel/worker hook (e.g., via dependency
injection) so the test can await the worker and assert expected side effects
from handle_restore_session_results, then validate created payloads/routed
trade_key actions before cleaning up the worker.
- Around line 160-239: The setup_restore_db function is creating a hand-rolled
test schema that has drifted from production; replace the manual CREATE TABLE
queries with running the real project migrations after creating the in-memory
Sqlite pool. Concretely, in setup_restore_db() (where
SqlitePoolOptions::new().connect("sqlite::memory:") is used) remove the
sqlx::query(...CREATE TABLE...) blocks and call the sqlx migration runner (e.g.
sqlx::migrate!("migrations").run(&pool).await.unwrap() or use
sqlx::migrate::Migrator::new(...) and .run(&pool).await) to apply the project's
migrations to the pool; ensure the migrations path and sqlx features are enabled
so tests can run migrations against the in-memory DB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 406a9441-d4ca-4884-9342-c7c5c130e290
📒 Files selected for processing (1)
src/app/restore_session.rs
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
from
1801590 to
ade376a
Compare
|
Will look asap! Thanks @sergey3bv ! Try to fix conflicts if you can! |
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
2 times, most recently
from
62ab200 to
0a65695
Compare
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 (2)
src/app/restore_session.rs (2)
40-43:⚠️ Potential issue | 🟠 MajorAvoid logging full master/trade pubkeys in runtime logs.
These are stable user identifiers; logging full values increases data exposure risk. Prefer redacted/fingerprinted output.
🔧 Minimal redaction patch
@@ - tracing::info!( - "Starting background restore session for master key: {}", - master_key - ); + tracing::info!( + "Starting background restore session for master key: {}", + redact_pubkey(&master_key) + ); @@ - tracing::info!("Restore session response sent to user {}", trade_key,); + tracing::info!( + "Restore session response sent to user {}", + redact_pubkey(trade_key) + ); @@ - tracing::warn!("Restore session timed out for user: {}", trade_key); + tracing::warn!( + "Restore session timed out for user: {}", + redact_pubkey(trade_key) + ); + +fn redact_pubkey(key: &str) -> String { + if key.len() <= 16 { + return "<redacted>".to_string(); + } + format!("{}…{}", &key[..8], &key[key.len() - 8..]) +}Also applies to: 113-113, 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 40 - 43, The tracing::info calls in restore_session.rs currently log full stable identifiers (master_key and trade_pubkey); replace these with a redacted/fingerprinted representation to avoid exposing full pubkeys. Implement or use a helper like fingerprint_pubkey that returns a short, non-reversible identifier (e.g., hash or first/last chars with ellipsis) and call it wherever master_key or trade_pubkey are logged (the tracing::info at the shown location and the other occurrences around lines noted) so logs show only the fingerprinted/redacted value instead of the full key.
50-57:⚠️ Potential issue | 🟠 MajorBackground failures cause guaranteed 1-hour waits before user feedback.
When
process_restore_session_workerrors, the failure is only logged (line 1120) with no message sent to the channel. This leaveswait_for_result()(line 1135) indefinitely blocked until the 1-hour timeout expires (line 65), creating unavoidable latency and task buildup under transient failures.
Remove logging of encryption keys.
Lines 40–43 log the full
master_key(a private encryption key), and lines 113 and 126 logtrade_key. Remove these from production logging to avoid leaking sensitive identifiers.
Clarify non-hex test condition.
The
rejects_non_hex_master_keytest (line 341) appends"g"toVALID_MASTER_KEY, mixing non-hex and invalid-length conditions. Use a fixed-length non-hex string to isolate what is actually being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 50 - 57, The background task process_restore_session_work currently only logs errors and never sends a failure result into the session channel, causing wait_for_result() to block until the 1-hour timeout; update handle_restore_session_results (the spawned async block that calls process_restore_session_work) to send an explicit error/result message on the same channel whenever process_restore_session_work returns Err so wait_for_result() can return promptly. Remove any production logging of sensitive keys by deleting or redacting uses that print master_key and trade_key (references: start_restore_session call sites that currently log master_key, and any logs in process_restore_session_work or handle_restore_session_results that print trade_key). Finally, adjust the rejects_non_hex_master_key test to use a fixed-length non-hex string (instead of appending "g" to VALID_MASTER_KEY) so the test only validates non-hex input rather than also changing length.
🤖 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/app/restore_session.rs`:
- Around line 339-342: The test rejects_non_hex_master_key currently appends "g"
to VALID_MASTER_KEY which also alters length; instead keep the key length 64 and
make exactly one character non-hex by mutating a single character in place. In
the rejects_non_hex_master_key test, build bad_key by copying VALID_MASTER_KEY,
change one character at a chosen index (e.g., last or any index) to a non-hex
char like 'g' (without inserting or removing characters), then call
crate::db::find_user_orders_by_master_key(&pool, &bad_key).await so the failure
is due to non-hex content only.
---
Outside diff comments:
In `@src/app/restore_session.rs`:
- Around line 40-43: The tracing::info calls in restore_session.rs currently log
full stable identifiers (master_key and trade_pubkey); replace these with a
redacted/fingerprinted representation to avoid exposing full pubkeys. Implement
or use a helper like fingerprint_pubkey that returns a short, non-reversible
identifier (e.g., hash or first/last chars with ellipsis) and call it wherever
master_key or trade_pubkey are logged (the tracing::info at the shown location
and the other occurrences around lines noted) so logs show only the
fingerprinted/redacted value instead of the full key.
- Around line 50-57: The background task process_restore_session_work currently
only logs errors and never sends a failure result into the session channel,
causing wait_for_result() to block until the 1-hour timeout; update
handle_restore_session_results (the spawned async block that calls
process_restore_session_work) to send an explicit error/result message on the
same channel whenever process_restore_session_work returns Err so
wait_for_result() can return promptly. Remove any production logging of
sensitive keys by deleting or redacting uses that print master_key and trade_key
(references: start_restore_session call sites that currently log master_key, and
any logs in process_restore_session_work or handle_restore_session_results that
print trade_key). Finally, adjust the rejects_non_hex_master_key test to use a
fixed-length non-hex string (instead of appending "g" to VALID_MASTER_KEY) so
the test only validates non-hex input rather than also changing length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 439355b7-7194-458b-9007-49e026195462
📒 Files selected for processing (1)
src/app/restore_session.rs
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
from
0a65695 to
5d9b2bf
Compare
|
Thanks for persisting through the rebases @sergey3bv the test structure looks solid. One coverage gap worth flagging for the mutation score target in #637: The key validation tests (short key, non-hex, too long), terminal-status |
sergey3bv
force-pushed
the
test/mutation-session-restore
branch
from
5d9b2bf to
a2f7662
Compare
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Good initiative on mutation testing for the session restore module. The tests cover important scenarios. However, there are several issues that need to be addressed before merging.
Issues
1. 🔴 rejects_non_hex_master_key test is flawed
Line 346: The test does bad_key.replace_range(63..64, "g") but replace_range on a 64-char string with range 63..64 replaces the last character. However, looking at the comment "keep length 64, make last char non-hex" this should work. Let me verify the actual behavior...
Actually, replace_range(63..64, "g") is correct since it replaces characters at index 63 (the 64th character, 0-indexed). The comment is accurate. ✅
2. 🟡 Schema drift risk in setup_restore_db
Lines 166-231: The test creates its own schema inline. If the actual migration changes, these tests will silently pass with outdated schema. Consider:
- Using
sqlx::migrate!()like other tests in the codebase - Or at minimum, adding a comment noting the schema must match migrations
3. 🟡 Tests named "restore_session_action" actually test DB functions
Lines 316-350: The section header says // --- restore_session_action --- but tests rejects_short_master_key, rejects_non_hex_master_key, rejects_too_long_master_key call crate::db::find_user_orders_by_master_key directly, not restore_session_action.
This is misleading. Either:
- Rename the section to
// --- find_user_orders_by_master_key validation --- - Or add actual
restore_session_coretests for key validation
4. 🟡 Timeout hardcoded at 1 hour
Line 65: tokio::time::Duration::from_secs(60 * 60) is a 1-hour timeout. This seems excessive for a restore operation. Consider:
- Making it configurable
- Or at least extracting to a named constant
5. 🟢 Minor: Test helper SQL duplicates order schema
The INSERT INTO orders statements in stranger_sender_cannot_see_another_users_disputes, finds_active_dispute_for_buyer, finds_in_progress_dispute_for_seller, and excludes_resolved_disputes duplicate the order insertion logic. Consider extracting to a helper like insert_order_with_dispute_flags.
Questions
-
The new
restore_session_corefunction extracts the core logic fromrestore_session_actionfor testability. Good pattern. But why wasn'tAppContextalso abstracted to allow testing the full path? -
Is there a reason the tests don't verify the actual
RestoreSessionManagerbehavior? The tests focus on DB queries but not the background task spawning/timeout handling.
Required Changes
- Fix test section naming to accurately reflect what's being tested
- Consider using
sqlx::migrate!()for schema setup or document the drift risk
Suggestions (non-blocking)
- Extract 1-hour timeout to a constant
- Add tests for
restore_session_corekey validation path - Consider helper for dispute-related order insertions
3 participants
Addressing #637
Summary by CodeRabbit
New Features
Bug Fixes
Tests