fix(delivery): add deliverySucceeded compat field alongside deliveryStatus

Kaspre · claude · Kaspre · commit 8be2194c4b4f · 2026-05-08T19:57:51.000-04:00
ClawSweeper re-review (2026-05-08) caught that the slimmed return object
dropped the top-level `deliverySucceeded` field. `src/agents/agent-command.ts`
reads `deliveryResult?.deliverySucceeded === true` to decide whether to
clear the durable `pendingFinalDelivery` retry marker — without it, a
successful delivery never clears the marker and a restart can replay the
already-delivered reply.

Restore `deliverySucceeded: boolean` as a top-level field on the return
object alongside `deliveryStatus`. Backward-compatible — agent-command.ts
needs no changes. Test asserts the field mirrors `deliveryStatus.succeeded`
on the success and missing-target paths.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/src/agents/command/delivery.test.ts b/src/agents/command/delivery.test.ts
@@ -447,6 +447,15 @@ function logMessages(runtime: ReturnType<typeof createRuntime>): string[] {
   return runtime.log.mock.calls.map((c: unknown[]) => String(c[0]));
 }
 
+// The `[delivery] delivery requested but not completed` warning is routed to
+// runtime.error (with runtime.log fallback when error is unavailable). Tests
+// that look for that warning should use this helper rather than logMessages.
+function diagnosticMessages(runtime: ReturnType<typeof createRuntime>): string[] {
+  return [...runtime.error.mock.calls, ...runtime.log.mock.calls].map((c: unknown[]) =>
+    String(c[0]),
+  );
+}
+
 // ---------------------------------------------------------------------------
 // Tests
 // ---------------------------------------------------------------------------
@@ -477,6 +486,9 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
       attempted: true,
       succeeded: true,
     });
+    // Backward-compat field consumed by agent-command.ts to clear
+    // pendingFinalDelivery — must mirror deliveryStatus.succeeded.
+    expect(result.deliverySucceeded).toBe(true);
     // No warning log on success
     expect(logMessages(runtime).some((msg) => msg.includes("[delivery]"))).toBe(false);
   });
@@ -510,8 +522,9 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
       attempted: false,
       succeeded: false,
     });
+    expect(result.deliverySucceeded).toBe(false);
     expect(
-      logMessages(runtime).some((msg) =>
+      diagnosticMessages(runtime).some((msg) =>
         msg.includes("[delivery] delivery requested but not completed"),
       ),
     ).toBe(true);
@@ -533,9 +546,9 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
       attempted: true,
       succeeded: false,
     });
-    expect(logMessages(runtime).some((msg) => msg.includes("delivery returned zero results"))).toBe(
-      true,
-    );
+    expect(
+      diagnosticMessages(runtime).some((msg) => msg.includes("delivery returned zero results")),
+    ).toBe(true);
   });
 
   it("catches thrown error in bestEffort mode without re-throwing", async () => {
@@ -597,7 +610,7 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
     expect(result.deliveryStatus).toBeUndefined();
   });
 
-  it("returns succeeded=true with hadPartialFailure when onError fires but results exist", async () => {
+  it("returns succeeded=false with hadPartialFailure when onError fires under best-effort", async () => {
     deliverSpy.mockImplementation(async (opts) => {
       // Simulate partial failure: onError fires for one payload, but results still returned
       opts.onError?.(new Error("Payload 2 failed"), { text: "hello" } as never);
@@ -609,16 +622,25 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
       deliver: true,
       channel: "discord",
       to: "channel:123456",
+      bestEffortDeliver: true,
     });
 
+    // Partial failures count as not-succeeded so `agent-command.ts` keeps the
+    // durable `pendingFinalDelivery` retry marker; matches upstream's pre-PR
+    // `!deliveryHadError` semantics.
     expect(result.deliveryStatus).toEqual({
       requested: true,
       attempted: true,
-      succeeded: true,
+      succeeded: false,
       hadPartialFailure: true,
     });
-    // No [delivery] warning — succeeded is true
-    expect(logMessages(runtime).some((msg) => msg.includes("[delivery]"))).toBe(false);
+    expect(result.deliverySucceeded).toBe(false);
+    // [delivery] warning fires because succeeded is false
+    expect(
+      diagnosticMessages(runtime).some((msg) =>
+        msg.includes("[delivery] delivery requested but not completed"),
+      ),
+    ).toBe(true);
   });
 
   it("logs warning when channel resolves to internal", async () => {
@@ -642,8 +664,68 @@ describe("deliverAgentCommandResult — delivery status tracking", () => {
       succeeded: false,
       error: true,
     });
-    expect(logMessages(runtime).some((msg) => msg.includes("channel resolved to internal"))).toBe(
+    expect(
+      diagnosticMessages(runtime).some((msg) => msg.includes("channel resolved to internal")),
+    ).toBe(true);
+  });
+
+  it("reports preflight rejection (not zero results) when explicit target fails resolution", async () => {
+    // Simulate best-effort preflight rejection: deliveryPlan still resolves a
+    // target string, but resolveAgentOutboundTarget returns ok:false. The
+    // delivery code marks hadPreflightError and skips the deliver call; the
+    // diagnostic must report the preflight reason, not "zero results".
+    outboundTargetSpy.mockReturnValue({
+      resolvedTarget: {
+        ok: false as const,
+        error: new Error("target rejected by plugin"),
+      } as ReturnType<typeof agentDeliveryModule.resolveAgentOutboundTarget>["resolvedTarget"],
+      resolvedTo: "channel:123456",
+      targetMode: "explicit" as const,
+    });
+
+    const { result, runtime } = await runDelivery({
+      message: "hello",
+      deliver: true,
+      bestEffortDeliver: true,
+      channel: "discord",
+      to: "channel:123456",
+    });
+
+    expect(deliverSpy).not.toHaveBeenCalled();
+    expect(result.deliveryStatus).toEqual({
+      requested: true,
+      attempted: false,
+      succeeded: false,
+      error: true,
+    });
+    const diagnostics = diagnosticMessages(runtime);
+    expect(diagnostics.some((msg) => msg.includes("preflight rejected delivery target"))).toBe(
       true,
     );
+    expect(diagnostics.some((msg) => msg.includes("delivery returned zero results"))).toBe(false);
+  });
+
+  it("emits the [delivery] warning on stderr (runtime.error), not stdout", async () => {
+    // Diagnostics belong on stderr so scripts that pipe stdout for payload
+    // text don't conflate it with delivery status. Tested by verifying
+    // runtime.error received the message and runtime.log did not.
+    deliverSpy.mockResolvedValue([]);
+
+    const { runtime } = await runDelivery({
+      message: "hello",
+      deliver: true,
+      channel: "discord",
+      to: "channel:123456",
+    });
+
+    const errorMessages = runtime.error.mock.calls.map((c: unknown[]) => String(c[0]));
+    expect(
+      errorMessages.some((msg) => msg.includes("[delivery] delivery requested but not completed")),
+    ).toBe(true);
+    expect(
+      logMessages(runtime).some((msg) =>
+        msg.includes("[delivery] delivery requested but not completed"),
+      ),
+    ).toBe(false);
   });
 });
diff --git a/src/agents/command/delivery.ts b/src/agents/command/delivery.ts
@@ -385,7 +385,8 @@ export async function deliverAgentCommandResult(params: {
     deliver &&
     deliveryChannel &&
     !isInternalMessageChannel(deliveryChannel) &&
-    deliveryPayloads.length > 0
+    deliveryPayloads.length > 0 &&
+    !hadPreflightError
   ) {
     if (deliveryTarget) {
       deliveryAttempted = true;
@@ -413,7 +414,13 @@ export async function deliverAgentCommandResult(params: {
         // since catching silent failures is this patch's primary goal. A future
         // change to deliverOutboundPayloads could expose cancellation metadata to
         // distinguish the two cases.
-        deliverySucceeded = results.length > 0;
+        //
+        // Partial failures also count as not-succeeded: matches upstream's
+        // pre-PR `!deliveryHadError` semantics so `agent-command.ts` keeps the
+        // durable `pendingFinalDelivery` retry marker when any payload errored
+        // under best-effort. Without this, a partial-failure final-delivery
+        // would never be retried after restart.
+        deliverySucceeded = results.length > 0 && !hadPartialFailure;
       } catch (err) {
         if (!bestEffortDeliver) {
           throw err;
@@ -435,14 +442,21 @@ export async function deliverAgentCommandResult(params: {
         ? "channel resolved to internal"
         : !deliveryTarget
           ? "no delivery target resolved"
-          : deliveryThrewError
-            ? "delivery threw an error"
-            : "delivery returned zero results";
-    runtime.log(
+          : hadPreflightError
+            ? "preflight rejected delivery target"
+            : deliveryThrewError
+              ? "delivery threw an error"
+              : hadPartialFailure
+                ? "partial failure during best-effort delivery"
+                : "delivery returned zero results";
+    const message =
       `[delivery] delivery requested but not completed: ${reason} ` +
-        `(session=${effectiveSessionKey ?? "unknown"} channel=${deliveryChannel ?? "none"} ` +
-        `target=${deliveryTarget ?? "none"} payloads=${deliveryPayloads.length})`,
-    );
+      `(session=${effectiveSessionKey ?? "unknown"} channel=${deliveryChannel ?? "none"} ` +
+      `target=${deliveryTarget ?? "none"} payloads=${deliveryPayloads.length})`;
+    runtime.error?.(message);
+    if (!runtime.error) {
+      runtime.log(message);
+    }
   }
 
   const deliveryStatusResult = deliver
@@ -458,6 +472,11 @@ export async function deliverAgentCommandResult(params: {
   return {
     payloads: normalizedPayloads,
     meta: resultMeta,
+    // Backward-compat field for `src/agents/agent-command.ts` which reads
+    // `deliveryResult?.deliverySucceeded === true` to decide whether to
+    // clear the durable `pendingFinalDelivery` retry marker. Keeping this
+    // top-level alongside `deliveryStatus` avoids touching agent-command.ts.
+    deliverySucceeded,
     deliveryStatus: deliveryStatusResult,
   };
 }
