Test gap: no coverage for the new CantDo(DisputeCreationError) path in dispute_action

[grunch]

Summary

#848 changes dispute_action so that a setup_dispute failure is propagated to the client instead of being silently swallowed. That is the right call, but the new error arm has no test: grep -rn "DisputeCreationError" src/ returns nothing.

This is a behavior change on a user-facing protocol response, so it should be pinned by a test before it can regress.

Context

Before #848, src/app/dispute.rs did:

if order.setup_dispute(is_buyer_dispute).is_ok() {
    order.clone().update(pool).await.map_err(...)?;
}
// execution continued, and the dispute row was created anyway

On failure this skipped order.update() but kept going, creating a disputes row while leaving the order flags unset. After #848:

order.setup_dispute(is_buyer_dispute).map_err(MostroCantDo)?;
order.clone().update(pool).await.map_err(...)?;

The caller now receives CantDo(DisputeCreationError) and no dispute row is created.

setup_dispute (mostro-core 0.14.5, order.rs:522) returns Err(CantDoReason::DisputeCreationError) only when the initiating side's dispute flag was already set. dispute_action guards at the top with find_dispute_by_order_id(...).is_ok() → DisputeAlreadyExists, so the ordinary double-dispute flow never reaches setup_dispute twice. The new arm is reachable only from an inconsistent database state: an order with buyer_dispute/seller_dispute set but no matching row in disputes.

That state should not arise, and failing loudly on a violated precondition is the correct behavior — this was discussed by @ToRyVand in the #848 review thread. But "should not arise" is exactly the kind of assumption that deserves a test, both to lock in the semantics and to document the trade-off in executable form.

Current coverage in src/app/dispute.rs

The dispute_action_* tests already cover: missing order id (NotFound), unknown order id (NotFound), order that already has a dispute (DisputeAlreadyExists), non-disputable status, missing seller pubkey, missing buyer pubkey, sender that is not a party, and both the buyer-initiated and seller-initiated happy paths.

The setup_dispute error arm is the one gap.

What to do

Add one test to the mod tests block in src/app/dispute.rs, next to dispute_action_rejects_order_that_already_has_a_dispute.

How to build the state

Follow the existing helpers in that module (create_test_pool, and the setup used by dispute_action_buyer_initiated_flow_persists_dispute_and_notifies):

1. Create a test pool and insert an order in a disputable status (fiat-sent or active) with both buyer_pubkey and seller_pubkey set.
2. Set buyer_dispute = 1 on that order row — directly via sqlx::query("UPDATE orders SET buyer_dispute = 1 WHERE id = ?1") — and insert no row into disputes. This is the inconsistent state described above; going through the normal flow would trip the DisputeAlreadyExists guard instead and never reach setup_dispute.
3. Build the Message / UnwrappedMessage with the buyer as sender, so get_counterpart_info yields is_buyer_dispute == true and setup_dispute hits the already-set flag.

What to assert

let err = dispute_action(&ctx, msg, &event, &my_keys)
    .await
    .expect_err("inconsistent dispute state must be rejected");
assert!(matches!(
    err,
    MostroError::MostroCantDo(CantDoReason::DisputeCreationError)
));

Plus the side-effect assertion that distinguishes new behavior from old — this is the part that actually catches a regression:

// The old code created the dispute row anyway; the new code must not.
assert!(find_dispute_by_order_id(&pool, order_id).await.is_err());

Optionally also assert the order row is untouched (status unchanged, seller_dispute still 0), confirming nothing was persisted before the early return.

Related nit worth folding in

setup_dispute sets self.status = Status::Dispute.to_string() before its error return (mostro-core 0.14.5, order.rs:537), so the local order is left dirty on Err. The current code returns before any persist, so this is harmless today. While adding the test, add the one-line comment that pins the invariant:

// setup_dispute leaves order.status dirty on Err; we return before any persist.
order.setup_dispute(is_buyer_dispute).map_err(MostroCantDo)?;

Acceptance criteria

• A test in src/app/dispute.rs asserts dispute_action returns CantDo(DisputeCreationError) for an order whose dispute flag is set with no matching disputes row.
• The same test asserts no dispute row is created — the behavior that changed in refactor: removed dead code identified in issue  #848.
• cargo test --all, cargo clippy --all-targets -- -D warnings and cargo fmt --check are clean.

Severity: Low (test gap, no bug). Can be resolved directly in #848 if it has not merged yet; otherwise as a follow-up.

[ToRyVand]

Verified the premise against mostro-core 0.14.5 (what Cargo.lock pins, not the 0.14.3 sitting in my registry cache): setup_dispute is at order.rs:522, self.status = Status::Dispute at :538, and the Err return at :542 — so the dirty-status note is right. DisputeCreationError appears nowhere in src/, so the gap is real.

Two notes for whoever picks this up: #848 merged at 18:12, ~2 min after this was filed, so the "resolve inside #848" option is closed — it's a follow-up now. And the suggested find_dispute_by_order_id(..).is_err() assertion is the right shape: it uses fetch_one, so a missing row really is Err, not Ok(None).