test: mutation testing for Nostr event handling - #849
Open
ToRyVand wants to merge 4 commits into
Open
Conversation
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.
cargo-mutants requires a fully green baseline before mutating anything, and the lightning-address test path was hardcoded to 127.0.0.1:8080 in both the test server and lnurl.rs's cfg!(test) URL builder. On a machine already using 8080 that baseline never passes. MOSTRO_TEST_LN_PORT (env, default 8080) lets `make mutation-test` point both sides at a free port without changing default `cargo test` behavior. Also drops --test-threads=4 from the mutation-test target: cargo-mutants 27.1.0 has no working way to forward libtest args (config key, CLI flag, and trailing -- args all insert before cargo test's own --), so the flag only broke the baseline. CARGO_MUTANTS_JOBS=2 remains the real OOM cap.
Issue MostroP2P#636 (mutation testing for Nostr event handling) flagged accept_event()'s post-unwrap validation as uncovered: the replay-window timestamp check and the identity/signature check had zero mutants caught. Extracted both into pure functions (is_stale, missing_inner_signature) so cargo-mutants' boundary mutants can be hit directly, and added three accept_event-level tests built on a real wrap_message_with-signed GiftWrap event (happy path, wrong kind, wrong receiver) to cover the surrounding POW/kind/verify gates. Mutation run on src/app.rs + src/nip33.rs (partial, 90/108 mutants tested before this checkpoint): every accept_event mutant now caught except the 3 in the is_v2 spam-gate branch, deliberately deferred since exercising it needs the SpamGate global singleton initialized. Overall score and the rest of app.rs's dispatcher/warning_msg mutants still outstanding — see [[project_july_contribution_plan]] memory for the resume point.
…mutants Targets the remaining surviving mutants on the Nostr event-handling path for issue MostroP2P#636: - accept_event's protocol-v2 spam-gate / PoW-first-contact branch (3 mutants around the `!gate.is_known(..) && !event.check_pow(..)` check) was never exercised by is_v2=true — add tests for the accepted and rejected first-contact cases. - nip33::create_event's NIP-40 expiration-tag dedup check (`||` between the canonical and custom expiration tag kinds) was only exercised via the "no existing tag" paths — add a test that pre-supplies a real expiration tag and asserts it isn't duplicated. spam_gate.rs: fix a latent test-order bug the new accept_event tests exposed — install_global_then_second_install_is_rejected assumed it would always be the first test in the binary to install the process-wide SpamGate OnceLock, which broke once another test raced it there. 10/10 targeted mutants confirmed killed via a `-F`-filtered cargo-mutants run scoped to accept_event/create_event.
Contributor
WalkthroughMutation testing now runs through a shared Makefile target with configurable test ports. Event acceptance checks are extracted into helpers with expanded async coverage, while NIP-40 expiration tags and global spam-gate installation receive regression-test updates. ChangesNostr testing improvements
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 69-71: Update the mutation-test target to avoid the Bash-only set
-o pipefail under Make’s default shell, and preserve caller configuration by not
unconditionally overwriting MOSTRO_TEST_LN_PORT. Also prevent parallel mutation
workers from sharing the same fixed port by guarding concurrency or assigning
distinct worker ports while retaining configurable overrides.
In `@src/nip33.rs`:
- Around line 1121-1143: Add a companion test alongside
create_event_does_not_duplicate_a_caller_supplied_expiration_tag that supplies a
custom "expiration" tag through new_order_event, then assert the resulting order
contains no auto-added standard TagKind::Expiration tag. Keep the existing
standard-tag test unchanged and verify the custom branch in create_event's
has_expiration_tag logic.
🪄 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 Plus
Run ID: b2a6afb7-fe1d-4339-8c3c-1635a3e9717d
📒 Files selected for processing (7)
.github/workflows/mutation.ymlMakefilesrc/app.rssrc/lightning/invoice.rssrc/lnurl.rssrc/nip33.rssrc/spam_gate.rs
Comment on lines
+69
to
+71
| mutation-test: | ||
| @set -o pipefail; \ | ||
| CARGO_MUTANTS_JOBS=2 MOSTRO_TEST_LN_PORT=18080 cargo mutants $(ARGS) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -eu
grep -nE '^[[:space:]]*(SHELL|\.SHELL)' Makefile || true
make -n mutation-test
/bin/sh -c 'set -o pipefail'
cargo mutants --help | grep -iE 'job|parallel' || trueRepository: MostroP2P/mostro
Length of output: 99
🏁 Script executed:
#!/usr/bin/env bash
set -eu
printf 'Makefile shell/config near mutation-test:\n'
grep -nE '^[[:space:]]*(SHELL|\.SHELL|mutation-test|CARGO_MUTANTS|MOSTRO_TEST_LN_PORT|Cargo\.toml|\.cargo/config)' Makefile Cargo.toml -r . 2>/dev/null || true
printf '\nMakefile lines 55-78 if present:\n'
sed -n '55,78p' Makefile 2>/dev/null || true
printf '\nCargo mutants config references:\n'
rg -n "mutants|CARGO_MUTANTS|MOSTRO_TEST_LN_PORT|18080" Makefile Cargo.toml src .cargo 2>/dev/null || true
printf '\nShell pipefail behavior:\n'
bash -c '/bin/sh -c "set -o pipefail; echo ok"' 2>/dev/null && echo sh_pipefail=works || echo sh_pipefail=failed_exit=$?
bash -c '/bin/bash -c "set -o pipefail; echo ok"' && echo bash_pipefail=works || echo bash_pipefail=failed_exit=$?Repository: MostroP2P/mostro
Length of output: 1921
Make the mutation target portable and actually configurable.
Make may run this recipe with /bin/sh unless SHELL := /bin/bash is set earlier; Ubuntu’s default shell rejects set -o pipefail, so remove it unless another pipeline is added. This target also overwrites MOSTRO_TEST_LN_PORT, preventing callers from choosing a different port. With parallel mutation workers, either guard 18080 concurrency or use per-worker ports/serialized port-bound tests.
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 69-69: Required target "all" is missing from the Makefile.
(minphony)
[warning] 69-69: Required target "clean" is missing from the Makefile.
(minphony)
[warning] 69-69: Required target "test" is missing from the Makefile.
(minphony)
🤖 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 `@Makefile` around lines 69 - 71, Update the mutation-test target to avoid the
Bash-only set -o pipefail under Make’s default shell, and preserve caller
configuration by not unconditionally overwriting MOSTRO_TEST_LN_PORT. Also
prevent parallel mutation workers from sharing the same fixed port by guarding
concurrency or assigning distinct worker ports while retaining configurable
overrides.
Comment on lines
+1121
to
+1143
| fn create_event_does_not_duplicate_a_caller_supplied_expiration_tag() { | ||
| // Order events (kind 38383) always get an auto expiration tag from | ||
| // config when one isn't already present. Pre-supplying a real NIP-40 | ||
| // `TagKind::Expiration` tag must suppress the auto-add, exercising | ||
| // the `||` in create_event's has_expiration_tag check on its | ||
| // `TagKind::Expiration` arm rather than only the `Custom("expiration")` arm. | ||
| init_test_settings(); | ||
| let keys = Keys::generate(); | ||
| let extra_tags = Tags::from_list(vec![Tag::expiration(Timestamp::from(123_456_u64))]); | ||
|
|
||
| let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) | ||
| .expect("order event"); | ||
|
|
||
| let expiration_tags = order | ||
| .tags | ||
| .iter() | ||
| .filter(|t| matches!(t.kind(), TagKind::Expiration)) | ||
| .count(); | ||
| assert_eq!( | ||
| expiration_tags, 1, | ||
| "caller-supplied expiration tag must not be duplicated" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the custom "expiration" tag branch too.
This test only exercises TagKind::Expiration; deleting the TagKind::Custom("expiration") predicate in create_event would still pass. Add a companion case using a custom "expiration" tag and assert no standard expiration tag is auto-added.
Proposed companion test
+ #[test]
+ fn create_event_does_not_duplicate_a_custom_expiration_tag() {
+ init_test_settings();
+ let keys = Keys::generate();
+ let extra_tags = Tags::from_list(vec![Tag::custom(
+ TagKind::Custom(Cow::Borrowed("expiration")),
+ vec!["123456".to_string()],
+ )]);
+
+ let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags)
+ .expect("order event");
+
+ assert!(order.tags.iter().any(
+ |tag| matches!(tag.kind(), TagKind::Custom(ref name) if name == "expiration")
+ ));
+ assert!(!order
+ .tags
+ .iter()
+ .any(|tag| matches!(tag.kind(), TagKind::Expiration)));
+ }As per PR objectives, this change is intended to kill NIP-33 expiration-tag mutants.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn create_event_does_not_duplicate_a_caller_supplied_expiration_tag() { | |
| // Order events (kind 38383) always get an auto expiration tag from | |
| // config when one isn't already present. Pre-supplying a real NIP-40 | |
| // `TagKind::Expiration` tag must suppress the auto-add, exercising | |
| // the `||` in create_event's has_expiration_tag check on its | |
| // `TagKind::Expiration` arm rather than only the `Custom("expiration")` arm. | |
| init_test_settings(); | |
| let keys = Keys::generate(); | |
| let extra_tags = Tags::from_list(vec![Tag::expiration(Timestamp::from(123_456_u64))]); | |
| let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) | |
| .expect("order event"); | |
| let expiration_tags = order | |
| .tags | |
| .iter() | |
| .filter(|t| matches!(t.kind(), TagKind::Expiration)) | |
| .count(); | |
| assert_eq!( | |
| expiration_tags, 1, | |
| "caller-supplied expiration tag must not be duplicated" | |
| ); | |
| } | |
| fn create_event_does_not_duplicate_a_caller_supplied_expiration_tag() { | |
| // Order events (kind 38383) always get an auto expiration tag from | |
| // config when one isn't already present. Pre-supplying a real NIP-40 | |
| // `TagKind::Expiration` tag must suppress the auto-add, exercising | |
| // the `||` in create_event's has_expiration_tag check on its | |
| // `TagKind::Expiration` arm rather than only the `Custom("expiration")` arm. | |
| init_test_settings(); | |
| let keys = Keys::generate(); | |
| let extra_tags = Tags::from_list(vec![Tag::expiration(Timestamp::from(123_456_u64))]); | |
| let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) | |
| .expect("order event"); | |
| let expiration_tags = order | |
| .tags | |
| .iter() | |
| .filter(|t| matches!(t.kind(), TagKind::Expiration)) | |
| .count(); | |
| assert_eq!( | |
| expiration_tags, 1, | |
| "caller-supplied expiration tag must not be duplicated" | |
| ); | |
| } | |
| #[test] | |
| fn create_event_does_not_duplicate_a_custom_expiration_tag() { | |
| init_test_settings(); | |
| let keys = Keys::generate(); | |
| let extra_tags = Tags::from_list(vec![Tag::custom( | |
| TagKind::Custom(Cow::Borrowed("expiration")), | |
| vec!["123456".to_string()], | |
| )]); | |
| let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) | |
| .expect("order event"); | |
| assert!(order.tags.iter().any( | |
| |tag| matches!(tag.kind(), TagKind::Custom(ref name) if name == "expiration") | |
| )); | |
| assert!(!order | |
| .tags | |
| .iter() | |
| .any(|tag| matches!(tag.kind(), TagKind::Expiration))); | |
| } |
🤖 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/nip33.rs` around lines 1121 - 1143, Add a companion test alongside
create_event_does_not_duplicate_a_caller_supplied_expiration_tag that supplies a
custom "expiration" tag through new_order_event, then assert the resulting order
contains no auto-added standard TagKind::Expiration tag. Keep the existing
standard-tag test unchanged and verify the custom branch in create_event's
has_expiration_tag logic.
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.
Context
Follow-up from #618. Implements mutation testing for Nostr event handling — the communication layer between Mostro and its clients. Closes #636.
What changed
Makefile/.github/workflows/mutation.yml: added amutation-testtarget (CARGO_MUTANTS_JOBS=2 MOSTRO_TEST_LN_PORT=18080 cargo mutants) so a single knob caps concurrency (avoids OOM on constrained machines) and lets the local LN test port be overridden when 8080 is already taken on the host.src/lnurl.rs,src/lightning/invoice.rs: threadedMOSTRO_TEST_LN_PORTthrough the local test HTTP server/URL builder socargo-mutantsruns don't collide with something else already bound to 8080.src/app.rs:is_staleandmissing_inner_signatureout ofaccept_eventwith direct boundary tests.accept_event_testscovering the full accept/reject paths (valid gift wrap, wrong kind, wrong receiver) plus the protocol-v2 spam-gate / PoW-first-contact branch (accepted when the bar is cleared, dropped when it isn't) — this branch had 3 surviving mutants with zero coverage.src/nip33.rs: added a test forcreate_event's NIP-40 expiration-tag dedup check — a caller-supplied expiration tag must not be duplicated by the auto-expiration logic, which was the source of a surviving||→&&mutant.src/spam_gate.rs: fixedinstall_global_then_second_install_is_rejected, which assumed it would always be the first test in the binary to install the process-wideSpamGateOnceLock— the newaccept_eventspam-gate tests expose that the assumption doesn't hold once another test races it there. Now robust to install order, still asserts a second install is always rejected.Verification
10/10targeted mutants confirmed killed via a-F-filteredcargo mutants --file src/app.rs --file src/nip33.rs -F 'in accept_event|in create_event'run.cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings: clean.cargo test: 1058 passed, 1 ignored.Acceptance Criteria (from #636)
accept_event's spam-gate/PoW branch)create_event)Summary by CodeRabbit
Bug Fixes
Reliability