fix(desktop): pin resolve idempotency seam and gate 4xx reset on a full body

Two follow-ups to the idempotency fix from the kalvin-agent security review.

The requestId-preservation tests exercised only admin_reopen_report, so the
resolve call-site — the enforcement path — could regress to the stale-intent
bug with no test going red (verified: unconditionally resetting the resolve
requestId left the 35-test admin jsdom suite green). Add resolve lost-response
and definitive-4xx tests, mutation-verified against the resolve catch block.

preserveRequestIdOnError decided from HTTP status alone, so a truncated 4xx
(status read, body lost mid-stream) was treated as a definitive pre-commit
rejection and cleared the key even though no authoritative body arrived. Add a
bodyComplete discriminator to AdminMutationError, set true only when the full
response body is read (authoritative) and false for a redirect, over-cap, or
mid-stream read failure (partial). The UI resets a non-409 4xx only when the
body was complete; a truncated 4xx now preserves the key. Message strings are
unchanged.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src-tauri/src/commands/admin/error.rs

@@ -10,12 +10,16 @@
 /// pre-send failure (auth build, body serialisation): the relay never
 /// committed anything, so a retry must reuse the same idempotency key.
 ///
-/// A body-read failure mid-stream keeps the status it was reading (the relay
-/// answered with headers/status but the body was lost) — the outcome is
-/// unknown, so the caller preserves idempotency and lets the retry dedupe.
+/// `bodyComplete` is `true` only when the relay's full response body was read —
+/// an authoritative verdict. A non-409 4xx with `bodyComplete: true` is a
+/// definitive pre-commit rejection, so the UI may mint a fresh idempotency key.
+/// A status that arrives but whose body is lost mid-stream (or rejected over
+/// the size cap) carries `bodyComplete: false`: the outcome is unknown, so the
+/// caller preserves idempotency and lets the retry dedupe even on a 4xx.
 ///
 /// Serialises `rename_all = "camelCase"`; the JS bridge surfaces it as the
-/// rejected `TauriInvokeError.payload`, from which the UI reads `relayStatus`.
+/// rejected `TauriInvokeError.payload`, from which the UI reads `relayStatus`
+/// and `bodyComplete`.
 #[derive(Debug, serde::Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct AdminMutationError {
@@ -25,14 +29,29 @@ pub struct AdminMutationError {
     /// The relay's HTTP status when a response was received; `None` for a
     /// transport/pre-send failure where no relay answer exists.
     pub relay_status: Option<u16>,
+    /// Whether the relay's full response body was read. `true` only for an
+    /// authoritative verdict; `false` when the body was lost or truncated.
+    pub body_complete: bool,
 }
 
 impl AdminMutationError {
-    /// The relay answered with an HTTP status.
-    pub(super) fn relay(status: reqwest::StatusCode, message: String) -> Self {
+    /// The relay answered with an HTTP status and its full body was read — an
+    /// authoritative verdict.
+    pub(super) fn authoritative(status: reqwest::StatusCode, message: String) -> Self {
         Self {
             message,
             relay_status: Some(status.as_u16()),
+            body_complete: true,
+        }
+    }
+
+    /// The relay answered with an HTTP status but the body was not fully read
+    /// (redirect, over-cap, or a mid-stream read failure) — outcome unknown.
+    pub(super) fn partial(status: reqwest::StatusCode, message: String) -> Self {
+        Self {
+            message,
+            relay_status: Some(status.as_u16()),
+            body_complete: false,
         }
     }
 }
@@ -44,6 +63,7 @@ impl From<String> for AdminMutationError {
         Self {
             message,
             relay_status: None,
+            body_complete: false,
         }
     }
 }

desktop/src-tauri/src/commands/admin/helpers.rs

@@ -281,10 +281,12 @@ pub(super) async fn read_admin_response(
 ///
 /// Mirrors [`read_admin_response`]'s size discipline and message wording so the
 /// UI's message parsing is unchanged, but on a non-2xx it returns an
-/// [`AdminMutationError`] tagged with the received status. A redirect or a
-/// body-read failure also carries the status: the relay answered (headers/
-/// status arrived), the body outcome is unknown, so the caller preserves the
-/// idempotency key and lets the retry dedupe against any commit that landed.
+/// [`AdminMutationError`] tagged with the received status and whether the full
+/// body was read. Only a status with a complete body (`authoritative`) is a
+/// verdict the UI treats as definitive; a redirect, an over-cap body, or a
+/// mid-stream read failure carries the status as `partial` — the relay answered
+/// but the outcome is unknown, so the caller preserves the idempotency key and
+/// lets the retry dedupe against any commit that landed.
 async fn read_admin_mutation_response(
     resp: reqwest::Response,
     success_cap: u64,
@@ -295,7 +297,7 @@ async fn read_admin_mutation_response(
     let status = resp.status();
 
     if status.is_redirection() {
-        return Err(AdminMutationError::relay(
+        return Err(AdminMutationError::partial(
             status,
             format!("admin API returned a {status} redirect (not followed)"),
         ));
@@ -309,7 +311,7 @@ async fn read_admin_mutation_response(
 
     if let Some(cl) = resp.content_length() {
         if cl > cap {
-            return Err(AdminMutationError::relay(
+            return Err(AdminMutationError::partial(
                 status,
                 format!("admin response too large ({cl} bytes, cap {cap} bytes)"),
             ));
@@ -320,10 +322,10 @@ async fn read_admin_mutation_response(
     let mut stream = resp.bytes_stream();
     while let Some(chunk) = stream.next().await {
         let chunk = chunk.map_err(|e| {
-            AdminMutationError::relay(status, format!("admin response stream error: {e}"))
+            AdminMutationError::partial(status, format!("admin response stream error: {e}"))
         })?;
         if bytes.len() as u64 + chunk.len() as u64 > cap {
-            return Err(AdminMutationError::relay(
+            return Err(AdminMutationError::partial(
                 status,
                 format!("admin response too large (cap {cap} bytes)"),
             ));
@@ -333,7 +335,7 @@ async fn read_admin_mutation_response(
 
     if !is_success {
         let body = String::from_utf8_lossy(&bytes);
-        return Err(AdminMutationError::relay(
+        return Err(AdminMutationError::authoritative(
             status,
             format!("admin API error: {body}"),
         ));

desktop/src/features/admin-console/AdminConsolePanelHelpers.tsx

@@ -92,7 +92,7 @@ export function adminErrorMessage(e: unknown): string {
  * Extract the relay's HTTP status from a rejected admin mutation, or `null`.
  *
  * Native mutation commands reject with a typed `AdminMutationError`
- * (`{message, relayStatus}`); the Tauri bridge surfaces it as
+ * (`{message, relayStatus, bodyComplete}`); the Tauri bridge surfaces it as
  * `TauriInvokeError` whose `payload` is that object. `relayStatus` is a number
  * only when the relay actually answered — `null`/absent for a transport or
  * pre-send failure where no relay verdict exists.
@@ -108,31 +108,56 @@ export function adminMutationRelayStatus(e: unknown): number | null {
   return null;
 }
 
+/**
+ * Whether the relay's full response body was read — an authoritative verdict.
+ *
+ * `AdminMutationError.bodyComplete` is `true` only when the relay answered AND
+ * its whole body was received. A status that arrives but whose body is lost
+ * mid-stream (or rejected over the size cap) is `false`: the outcome is
+ * unknown. Absent/non-boolean payloads (bare-string errors, non-typed
+ * rejections) read `false`, which is fail-safe — an unknown outcome preserves
+ * the idempotency key.
+ */
+export function adminMutationBodyComplete(e: unknown): boolean {
+  if (e && typeof e === "object" && "payload" in e) {
+    const payload = (e as { payload: unknown }).payload;
+    if (payload && typeof payload === "object" && "bodyComplete" in payload) {
+      const complete = (payload as { bodyComplete: unknown }).bodyComplete;
+      if (typeof complete === "boolean") return complete;
+    }
+  }
+  return false;
+}
+
 /**
  * Whether a failed mutation must reuse its idempotency `requestId` on retry.
  *
  * The id is preserved UNLESS the relay definitively rejected the request before
- * committing — a 4xx other than 409. Those (bad action, unauthorized, not
- * found) refuse the input pre-commit, so a corrected resubmission is a
- * genuinely new command and a fresh id is safe.
+ * committing — a non-409 4xx whose full body was read. Those (bad action,
+ * unauthorized, not found) refuse the input pre-commit, so a corrected
+ * resubmission is a genuinely new command and a fresh id is safe.
  *
  * Everything else preserves the id so the relay can dedupe against a commit
  * that may have landed:
  *   - 409 — an idempotency claim or in-progress action already exists;
  *   - 5xx — the relay may have committed before failing;
- *   - a lost response body (status arrived, outcome unknown);
+ *   - a lost or truncated response body (status arrived, `bodyComplete` false —
+ *     outcome unknown), including a truncated 4xx;
  *   - a transport or pre-send failure with no relay answer (`relayStatus` null).
  *
- * This replaces string-matching `"409"`/`"processing"` on the message — which
- * missed the native layer's transport errors (`relay unreachable: …`, `admin
- * response stream error`) and cleared the id on exactly the ambiguous
- * lost-response failures where reuse is required.
+ * Status alone is insufficient: a truncated 4xx carries a definitive-looking
+ * status without an authoritative body, so the `bodyComplete` bit gates the
+ * reset. This replaces string-matching `"409"`/`"processing"` on the message,
+ * which missed the native layer's transport errors and cleared the id on
+ * exactly the ambiguous lost-response failures where reuse is required.
  */
 export function preserveRequestIdOnError(e: unknown): boolean {
   const status = adminMutationRelayStatus(e);
   if (status === null) return true;
   if (status === 409) return true;
-  return status < 400 || status >= 500;
+  if (status < 400 || status >= 500) return true;
+  // A non-409 4xx resets only when the relay's full body confirmed the verdict.
+  return !adminMutationBodyComplete(e);
 }
 
 // ── Shared UI helpers ─────────────────────────────────────────────────────

desktop/src/features/admin-console/adminConsolePanelEvents.jsdom-test.mjs

@@ -103,16 +103,24 @@ toast.error = (msg) => {
 // ── Typed native mutation error ──────────────────────────────────────────────
 //
 // Admin mutation commands reject with a serialized Rust `AdminMutationError`
-// (`{message, relayStatus}`, camelCase). The real tauri bridge rejects with
-// that plain object and `toTauriError` wraps it into a `TauriInvokeError` whose
-// `.message` is the message and `.payload` is the whole object — from which the
-// UI reads `relayStatus` to decide idempotency-retry policy. Rejecting with a
-// plain object here (NOT an Error) reproduces that wire shape exactly.
+// (`{message, relayStatus, bodyComplete}`, camelCase). The real tauri bridge
+// rejects with that plain object and `toTauriError` wraps it into a
+// `TauriInvokeError` whose `.message` is the message and `.payload` is the
+// whole object — from which the UI reads `relayStatus`/`bodyComplete` to decide
+// idempotency-retry policy. Rejecting with a plain object here (NOT an Error)
+// reproduces that wire shape exactly.
 //
 // `relayStatus` is a number when the relay authoritatively answered, and `null`
-// for a transport/pre-send failure where no relay verdict exists.
-function mutationReject(message, relayStatus) {
-  return Promise.reject({ message, relayStatus });
+// for a transport/pre-send failure where no relay verdict exists. `bodyComplete`
+// is true only when the relay's full body was read; it defaults to `relayStatus
+// !== null` (a status with a fully-read body — the common authoritative case),
+// and callers pass `false` explicitly to model a truncated/lost-body response.
+function mutationReject(
+  message,
+  relayStatus,
+  bodyComplete = relayStatus !== null,
+) {
+  return Promise.reject({ message, relayStatus, bodyComplete });
 }
 
 // ── Deferred promise helper ──────────────────────────────────────────────────
@@ -2558,6 +2566,266 @@ test("reopen-4xx-resets-requestId: a definitive pre-commit rejection uses a fres
   await unmount();
 });
 
+test("resolve-lost-response-preserves-requestId: an ambiguous transport failure reuses the requestId on retry", async () => {
+  // The resolve path is the enforcement seam and carries the same stale-intent
+  // risk as reopen: a lost-response failure (`relayStatus: null`, no relay
+  // verdict) must reuse the idempotency requestId so a retry dedupes against a
+  // commit that may have landed — otherwise a retry with a fresh id re-applies
+  // an enforcement action over another operator's intervening state.
+  //
+  // Mutation evidence: replace the resolve catch's preservation branch with an
+  // unconditional `requestIdRef.current = null` → the two attempts carry
+  // different ids and this goes red (the helper and reopen path stay intact).
+
+  const origin = "https://admin.example.com";
+  const pubkey = "c7".repeat(32);
+
+  const openItem = {
+    id: "00000000-0000-0000-0000-0000000000c7",
+    communityId: "comm-1",
+    communityHost: "alpha.example.com",
+    reportEventId: "aa",
+    reporterPubkey: "bb",
+    targetKind: "event",
+    target: "cc",
+    reportType: "spam",
+    status: "open",
+    createdAt: "2024-06-01T12:00:00Z",
+  };
+  const openDetail = {
+    ...openItem,
+    channelId: null,
+    note: null,
+    resolvedBy: null,
+    resolvedAt: null,
+    actionId: null,
+    message: null,
+  };
+
+  setIpcHandler("admin_list_reports", () => Promise.resolve([openItem]));
+  setIpcHandler("admin_get_report", () => Promise.resolve(openDetail));
+  setIpcHandler("admin_list_feedback", () => Promise.resolve([]));
+
+  const requestIds = [];
+  setIpcHandler("admin_resolve_report", (args) => {
+    requestIds.push(args?.body?.requestId);
+    // Transport failure: no relay answer, so no HTTP status.
+    return mutationReject("relay unreachable: network error", null);
+  });
+
+  const { container, doRender, unmount } = mountPanel({ origin, pubkey });
+  await doRender();
+  await settle(30);
+  await openFirstReportDetail(container);
+  await settle(20);
+
+  // Select the dismiss action so the resolve submit button appears.
+  const dismissBtn = container.querySelector(
+    "[data-testid='action-btn-dismiss']",
+  );
+  assert.ok(dismissBtn, "dismiss action must be present");
+  await act(async () => {
+    fireEvent.click(dismissBtn);
+    await new Promise((r) => setTimeout(r, 10));
+  });
+  const submit = container.querySelector("[data-testid='resolve-submit-btn']");
+  assert.ok(
+    submit,
+    "resolve submit button must appear after selecting dismiss",
+  );
+
+  // First attempt → lost response.
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+  // Second attempt → same ambiguous failure; requestId must be identical.
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+
+  assert.equal(
+    requestIds.length,
+    2,
+    "two resolve attempts must have been made",
+  );
+  assert.equal(
+    requestIds[0],
+    requestIds[1],
+    `requestId must be preserved across a resolve lost-response retry; got: ${JSON.stringify(requestIds)}`,
+  );
+
+  await unmount();
+});
+
+test("resolve-4xx-resets-requestId: a definitive pre-commit rejection uses a fresh requestId", async () => {
+  // The resolve counterpart to reopen-4xx-resets: a non-409 4xx whose full body
+  // was read is a definitive pre-commit rejection, so a corrected resubmission
+  // is a genuinely new command and a fresh requestId is correct. Pins the
+  // resolve call-site's reset branch specifically.
+  //
+  // Mutation evidence: make the resolve catch preserve unconditionally → the
+  // two attempts share an id and this goes red.
+
+  const origin = "https://admin.example.com";
+  const pubkey = "c8".repeat(32);
+
+  const openItem = {
+    id: "00000000-0000-0000-0000-0000000000c8",
+    communityId: "comm-1",
+    communityHost: "alpha.example.com",
+    reportEventId: "aa",
+    reporterPubkey: "bb",
+    targetKind: "event",
+    target: "cc",
+    reportType: "spam",
+    status: "open",
+    createdAt: "2024-06-01T12:00:00Z",
+  };
+  const openDetail = {
+    ...openItem,
+    channelId: null,
+    note: null,
+    resolvedBy: null,
+    resolvedAt: null,
+    actionId: null,
+    message: null,
+  };
+
+  setIpcHandler("admin_list_reports", () => Promise.resolve([openItem]));
+  setIpcHandler("admin_get_report", () => Promise.resolve(openDetail));
+  setIpcHandler("admin_list_feedback", () => Promise.resolve([]));
+
+  const requestIds = [];
+  setIpcHandler("admin_resolve_report", (args) => {
+    requestIds.push(args?.body?.requestId);
+    // Full body read → authoritative pre-commit rejection.
+    return mutationReject("admin API error: bad request", 400);
+  });
+
+  const { container, doRender, unmount } = mountPanel({ origin, pubkey });
+  await doRender();
+  await settle(30);
+  await openFirstReportDetail(container);
+  await settle(20);
+
+  const dismissBtn = container.querySelector(
+    "[data-testid='action-btn-dismiss']",
+  );
+  assert.ok(dismissBtn, "dismiss action must be present");
+  await act(async () => {
+    fireEvent.click(dismissBtn);
+    await new Promise((r) => setTimeout(r, 10));
+  });
+  const submit = container.querySelector("[data-testid='resolve-submit-btn']");
+  assert.ok(
+    submit,
+    "resolve submit button must appear after selecting dismiss",
+  );
+
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+
+  assert.equal(
+    requestIds.length,
+    2,
+    "two resolve attempts must have been made",
+  );
+  assert.notEqual(
+    requestIds[0],
+    requestIds[1],
+    `a definitive non-409 4xx must reset the resolve requestId; got: ${JSON.stringify(requestIds)}`,
+  );
+
+  await unmount();
+});
+
+test("reopen-truncated-4xx-preserves-requestId: a 4xx with a lost body reuses the requestId on retry", async () => {
+  // Status alone is not a verdict: a 4xx whose body was lost mid-stream
+  // (`bodyComplete: false`) is NOT a definitive pre-commit rejection — the
+  // relay answered with a status but the outcome is unknown, so the requestId
+  // must be preserved and the retry left to dedupe. Only a 4xx with a fully
+  // read body resets. This pins the `bodyComplete` discriminator: reset-on-
+  // status-alone would clear the key here and re-issue a fresh command.
+  //
+  // Mutation evidence: drop the `bodyComplete` gate (reset every non-409 4xx) →
+  // the two attempts carry different ids and this goes red.
+
+  const origin = "https://admin.example.com";
+  const pubkey = "c9".repeat(32);
+
+  const resolvedItem = {
+    id: "00000000-0000-0000-0000-0000000000c9",
+    communityId: "comm-1",
+    communityHost: "alpha.example.com",
+    reportEventId: "aa",
+    reporterPubkey: "bb",
+    targetKind: "event",
+    target: "cc",
+    reportType: "spam",
+    status: "resolved",
+    createdAt: "2024-06-01T12:00:00Z",
+  };
+  const resolvedDetail = {
+    ...resolvedItem,
+    channelId: null,
+    note: null,
+    resolvedBy: "mod_pubkey",
+    resolvedAt: "2024-06-02T08:00:00Z",
+    actionId: null,
+    message: null,
+  };
+
+  setIpcHandler("admin_list_reports", () => Promise.resolve([resolvedItem]));
+  setIpcHandler("admin_get_report", () => Promise.resolve(resolvedDetail));
+  setIpcHandler("admin_list_feedback", () => Promise.resolve([]));
+
+  const requestIds = [];
+  setIpcHandler("admin_reopen_report", (args) => {
+    requestIds.push(args?.body?.requestId);
+    // Status arrived but the body was lost mid-stream: outcome unknown.
+    return mutationReject(
+      "admin response stream error: connection reset",
+      400,
+      false,
+    );
+  });
+
+  const { container, doRender, unmount } = mountPanel({ origin, pubkey });
+  await doRender();
+  await settle(30);
+  await openFirstReportDetail(container);
+  await settle(20);
+
+  const submit = container.querySelector("[data-testid='reopen-submit-btn']");
+  assert.ok(submit, "reopen submit button must be present");
+
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+  await act(async () => {
+    fireEvent.click(submit);
+    await new Promise((r) => setTimeout(r, 20));
+  });
+
+  assert.equal(requestIds.length, 2, "two reopen attempts must have been made");
+  assert.equal(
+    requestIds[0],
+    requestIds[1],
+    `a truncated 4xx (bodyComplete false) must preserve the requestId; got: ${JSON.stringify(requestIds)}`,
+  );
+
+  await unmount();
+});
+
 test("cancel-on-failed: a failed action offers Cancel, POSTs {actionId} to admin_cancel_report, and reloads to open", async () => {
   // Cancel-then-resolve is the only recovery from a failed enforcement. The
   // block offers Cancel on `status: "failed"`, fences it on the action id, and