From 2da3a1399c9855d96943e186e9cf1801b10357a4 Mon Sep 17 00:00:00 2001
From: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
Date: Wed, 4 Mar 2026 21:40:57 -0600
Subject: [PATCH] fix(cron): resolve #35185 bot review findings

---
 .../subagent-announce.format.e2e.test.ts      | 34 +++++++++++++++++++
 src/agents/subagent-announce.ts               | 12 ++++---
 ...p-recipient-besteffortdeliver-true.test.ts | 32 +++++++++++++++++
 src/cron/isolated-agent/delivery-dispatch.ts  | 17 +++++-----
 4 files changed, 81 insertions(+), 14 deletions(-)

diff --git a/src/agents/subagent-announce.format.e2e.test.ts b/src/agents/subagent-announce.format.e2e.test.ts
index 1f1698c4722..56e8206364e 100644
--- a/src/agents/subagent-announce.format.e2e.test.ts
+++ b/src/agents/subagent-announce.format.e2e.test.ts
@@ -516,6 +516,40 @@ describe("subagent announce formatting", () => {
     expect(msg).not.toContain("There are still 1 active subagent run for this session.");
   });
 
+  it("keeps cron child session when descendants are still pending", async () => {
+    sessionStore = {
+      "agent:main:subagent:test": {
+        sessionId: "child-session-cron-pending-descendants",
+      },
+      "agent:main:main": {
+        sessionId: "requester-session-cron-pending-descendants",
+      },
+    };
+    readLatestAssistantReplyMock.mockResolvedValue("");
+    chatHistoryMock.mockResolvedValueOnce({
+      messages: [{ role: "assistant", content: [{ type: "text", text: "final answer: cron" }] }],
+    });
+    subagentRegistryMock.countPendingDescendantRuns.mockImplementation((sessionKey: string) =>
+      sessionKey === "agent:main:subagent:test" ? 1 : 0,
+    );
+
+    const didAnnounce = await runSubagentAnnounceFlow({
+      childSessionKey: "agent:main:subagent:test",
+      childRunId: "run-direct-cron-pending-descendants",
+      requesterSessionKey: "agent:main:main",
+      requesterDisplayKey: "main",
+      requesterOrigin: { channel: "discord", to: "channel:12345", accountId: "acct-1" },
+      announceType: "cron job",
+      ...defaultOutcomeAnnounce,
+      cleanup: "delete",
+      expectsCompletionMessage: true,
+    });
+
+    expect(didAnnounce).toBe(true);
+    expect(sendSpy).toHaveBeenCalledTimes(1);
+    expect(sessionsDeleteSpy).not.toHaveBeenCalled();
+  });
+
   it("suppresses completion delivery when subagent reply is ANNOUNCE_SKIP", async () => {
     const didAnnounce = await runSubagentAnnounceFlow({
       childSessionKey: "agent:main:subagent:test",
diff --git a/src/agents/subagent-announce.ts b/src/agents/subagent-announce.ts
index 8b0c432db3b..03d30a0934c 100644
--- a/src/agents/subagent-announce.ts
+++ b/src/agents/subagent-announce.ts
@@ -1238,12 +1238,14 @@ export async function runSubagentAnnounceFlow(params: {
       // Best-effort only; fall back to direct announce behavior when unavailable.
     }
     const isCronAnnounce = params.announceType === "cron job";
-    if (pendingChildDescendantRuns > 0 && !isCronAnnounce) {
-      // The finished run still has pending descendant subagents (either active,
-      // or ended but still finishing their own announce and cleanup flow). Defer
-      // announcing this run until descendants fully settle.
+    if (pendingChildDescendantRuns > 0) {
+      // Descendants are still pending cleanup/announce work. Keep the child
+      // session alive so descendant routing and retry paths are not orphaned.
       shouldDeleteChildSession = false;
-      return false;
+      if (!isCronAnnounce) {
+        // Non-cron announce flows still defer while descendants settle.
+        return false;
+      }
     }
 
     if (requesterDepth >= 1 && reply?.trim()) {
diff --git a/src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts b/src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts
index a4522279c63..f0958902752 100644
--- a/src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts
+++ b/src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts
@@ -421,6 +421,38 @@ describe("runCronIsolatedAgentTurn", () => {
     });
   });
 
+  it("keeps cron run status ok when announce and direct fallback both fail", async () => {
+    await withTelegramAnnounceFixture(
+      async ({ home, storePath, deps }) => {
+        mockAgentPayloads([{ text: "hello from cron" }]);
+        vi.mocked(runSubagentAnnounceFlow).mockResolvedValueOnce(false);
+
+        const res = await runTelegramAnnounceTurn({
+          home,
+          storePath,
+          deps,
+          delivery: {
+            mode: "announce",
+            channel: "telegram",
+            to: "123",
+            bestEffort: false,
+          },
+        });
+
+        expect(res.status).toBe("ok");
+        expect(res.delivered).toBe(false);
+        expect(res.deliveryAttempted).toBe(true);
+        expect(res.error).toContain("cron announce delivery failed");
+        expect(deps.sendMessageTelegram).toHaveBeenCalledTimes(1);
+      },
+      {
+        deps: {
+          sendMessageTelegram: vi.fn().mockRejectedValue(new Error("direct fallback failed")),
+        },
+      },
+    );
+  });
+
   it("marks attempted when announce delivery reports false and best-effort is enabled", async () => {
     const { res, deps } = await runAnnounceFlowResult(true);
     expect(res.status).toBe("ok");
diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts
index 0fc301cc2b7..a130c2cc575 100644
--- a/src/cron/isolated-agent/delivery-dispatch.ts
+++ b/src/cron/isolated-agent/delivery-dispatch.ts
@@ -475,15 +475,14 @@ export async function dispatchCronDelivery(
         if (announceDeliveryWasAttempted && !delivered && !params.isAborted()) {
           const directFallback = await deliverViaDirect(params.resolvedDelivery);
           if (directFallback) {
-            return {
-              result: directFallback,
-              delivered,
-              deliveryAttempted,
-              summary,
-              outputText,
-              synthesizedText,
-              deliveryPayloads,
-            };
+            // Preserve announce-mode semantics: announce failure should not
+            // downgrade an otherwise successful cron execution to hard error.
+            // Keep announceResult and only use direct fallback on success.
+            logWarn(
+              `[cron:${params.job.id}] direct fallback after announce failure also failed: ${
+                directFallback.error ?? "unknown error"
+              }`,
+            );
           }
           // If direct delivery succeeded (returned null without error),
           // `delivered` has been set to true by deliverViaDirect.