fix(admin-disputes): confirm Mostro reply before reporting settle/cancel success#171
fix(admin-disputes): confirm Mostro reply before reporting settle/cancel success#171AndreaDiazCorreia wants to merge 2 commits into
Conversation
…min cancel/settle
|
Warning Review limit reached
More reviews will be available in 36 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. WalkthroughThis PR refactors admin dispute settlement and cancellation commands to await and validate Mostro's DM responses before reporting success, replacing fire-and-forget behavior that masked operation failures (resolving issue ChangesAdmin Dispute Response Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/take_dispute.rs (1)
92-136: ⚡ Quick winExtract the shared admin dispute round-trip helper.
Cancel and settle now duplicate the same send/wait/parse/validate/error-mapping flow almost verbatim. A small helper parameterized by request action, expected reply action, and success text would keep these paths from drifting on the next change.
Also applies to: 184-228
🤖 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/cli/take_dispute.rs` around lines 92 - 136, Cancel and settle duplicate the same send/wait/parse/validate/error-mapping flow; extract a single async helper (e.g. admin_dispute_roundtrip) that accepts the request message, the expected reply Action, and success text, then internally calls send_dm, awaits wait_for_dm, calls parse_dm_events, validates sender against ctx.mostro_pubkey, maps Action::CantDo to print_commands_results, and returns Ok on the expected action (e.g. Action::AdminCanceled) or Err with the same error strings used now; update the cancel and settle code paths to call this helper instead of duplicating the logic.
🤖 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 `@src/cli/take_dispute.rs`:
- Around line 106-113: The current map_err collapses send (sent_message.await?)
failures into a "receive response" error from wait_for_dm; separately await and
handle sent_message.await and map its error to a publish/network/auth-specific
message that mentions the dispute_id and the operation (e.g., AdminCancel), then
call wait_for_dm(...).map_err for genuine receive-timeouts with the existing "no
reply" hint (also referencing dispute_id, not order id); apply the same change
to the settle path (the analogous sent_message.await and wait_for_dm sequence
around lines 198-205).
---
Nitpick comments:
In `@src/cli/take_dispute.rs`:
- Around line 92-136: Cancel and settle duplicate the same
send/wait/parse/validate/error-mapping flow; extract a single async helper (e.g.
admin_dispute_roundtrip) that accepts the request message, the expected reply
Action, and success text, then internally calls send_dm, awaits wait_for_dm,
calls parse_dm_events, validates sender against ctx.mostro_pubkey, maps
Action::CantDo to print_commands_results, and returns Ok on the expected action
(e.g. Action::AdminCanceled) or Err with the same error strings used now; update
the cancel and settle code paths to call this helper instead of duplicating the
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
Run ID: 0f6fce81-6011-4337-a525-47bd9cf29648
📒 Files selected for processing (1)
src/cli/take_dispute.rs
| let recv_event = wait_for_dm(ctx, Some(admin_keys), sent_message) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to receive response from Mostro for AdminCancel: {e}. \ | ||
| The operation may not have completed; verify the order id exists and check backend logs." | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Don't collapse send failures into a "receive response" error.
wait_for_dm(...) also returns errors from sent_message.await?, so a publish/network/auth failure will be reported here as if Mostro just never replied. The added hint also says "order id", but these commands take a dispute_id, which makes the failure path harder to diagnose.
Possible adjustment
- let recv_event = wait_for_dm(ctx, Some(admin_keys), sent_message)
- .await
- .map_err(|e| {
- anyhow::anyhow!(
- "Failed to receive response from Mostro for AdminCancel: {e}. \
- The operation may not have completed; verify the order id exists and check backend logs."
- )
- })?;
+ let recv_event = wait_for_dm(ctx, Some(admin_keys), sent_message)
+ .await
+ .map_err(|e| {
+ anyhow::anyhow!(
+ "AdminCancel failed while sending the request or waiting for Mostro's response: {e}. \
+ Verify the dispute id and check client/backend logs."
+ )
+ })?;Apply the same change to the settle path.
Also applies to: 198-205
🤖 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/cli/take_dispute.rs` around lines 106 - 113, The current map_err
collapses send (sent_message.await?) failures into a "receive response" error
from wait_for_dm; separately await and handle sent_message.await and map its
error to a publish/network/auth-specific message that mentions the dispute_id
and the operation (e.g., AdminCancel), then call wait_for_dm(...).map_err for
genuine receive-timeouts with the existing "no reply" hint (also referencing
dispute_id, not order id); apply the same change to the settle path (the
analogous sent_message.await and wait_for_dm sequence around lines 198-205).
1 participant
Closes #170
Summary
execute_admin_settle_disputeandexecute_admin_cancel_disputeusedadmin_send_dm(fire-and-forget) and then printedDispute settled/canceled successfully!unconditionally, regardless of whether the daemon accepted the request. This caused the CLI to report success for operations that never happened — including the case reported in #170, where a non-existentorder_idproduced a backend log error but a green tick on the CLI.Both handlers now follow the same pattern as
execute_take_dispute: send the message, wait for Mostro's reply, validate the sender, and only print the success message when the daemon replies with the matching action (AdminSettled/AdminCanceled).Changes
execute_admin_settle_dispute: replaceadmin_send_dmwith thesend_dm+wait_for_dm+parse_dm_eventsflow. Accept onlyAction::AdminSettledas success; routeAction::CantDothroughprint_commands_resultsso the backend reason is surfaced to the operator; bail with an explicit error on mismatched actions.execute_admin_cancel_dispute: same treatment, acceptingAction::AdminCanceledas success.wait_for_dmerrors (notablyWaitForDmTimeout) with context guiding the operator to verify the order id and check backend logs.Known limitation
When the
order_iddoes not exist, the daemon currently logs theInvalidOrderIderror locally but does not enqueue aCantDoDM back to the sender. With this PR the CLI will surface that case as a timeout error rather than false success — a correct, actionable result. A complete fix requires the daemon to reply withCantDo(NotFound); that is being tracked in a separate issue against themostrorepo.Summary by CodeRabbit