fix: decision lifecycle + stale callback hardening

assistant/src/__tests__/channel-approval-routes.test.ts

@@ -838,3 +838,145 @@ describe('terminal state check before markProcessed', () => {
     deliverSpy.mockRestore();
   });
 });
+
+// ═══════════════════════════════════════════════════════════════════════════
+// 10. No immediate reply after approval decision (WS-A)
+// ═══════════════════════════════════════════════════════════════════════════
+
+describe('no immediate reply after approval decision', () => {
+  beforeEach(() => {
+    process.env.CHANNEL_APPROVALS_ENABLED = 'true';
+  });
+
+  test('deliverChannelReply is NOT called from interception after decision is applied', async () => {
+    const orchestrator = makeMockOrchestrator();
+    const deliverSpy = spyOn(gatewayClient, 'deliverChannelReply').mockResolvedValue(undefined);
+
+    // Establish the conversation
+    const initReq = makeInboundRequest({ content: 'init' });
+    await handleChannelInbound(initReq, noopProcessMessage, 'token', orchestrator);
+
+    const db = getDb();
+    const events = db.$client.prepare('SELECT conversation_id FROM channel_inbound_events').all() as Array<{ conversation_id: string }>;
+    const conversationId = events[0]?.conversation_id;
+    ensureConversation(conversationId!);
+
+    // Create a pending run
+    const run = createRun(conversationId!);
+    setRunConfirmation(run.id, sampleConfirmation);
+
+    // Clear the spy to only track calls from the decision path
+    deliverSpy.mockClear();
+
+    // Send a callback decision
+    const req = makeInboundRequest({
+      content: '',
+      callbackData: `apr:${run.id}:approve_once`,
+    });
+
+    const res = await handleChannelInbound(req, noopProcessMessage, 'token', orchestrator);
+    const body = await res.json() as Record<string, unknown>;
+
+    expect(body.approval).toBe('decision_applied');
+
+    // The interception handler should NOT have called deliverChannelReply.
+    // The reply should only come from the terminal run completion path.
+    expect(deliverSpy).not.toHaveBeenCalled();
+
+    deliverSpy.mockRestore();
+  });
+
+  test('plain-text decision also does not trigger immediate reply', async () => {
+    const orchestrator = makeMockOrchestrator();
+    const deliverSpy = spyOn(gatewayClient, 'deliverChannelReply').mockResolvedValue(undefined);
+
+    // Establish the conversation
+    const initReq = makeInboundRequest({ content: 'init' });
+    await handleChannelInbound(initReq, noopProcessMessage, 'token', orchestrator);
+
+    const db = getDb();
+    const events = db.$client.prepare('SELECT conversation_id FROM channel_inbound_events').all() as Array<{ conversation_id: string }>;
+    const conversationId = events[0]?.conversation_id;
+    ensureConversation(conversationId!);
+
+    const run = createRun(conversationId!);
+    setRunConfirmation(run.id, sampleConfirmation);
+
+    deliverSpy.mockClear();
+
+    // Send a plain-text approval
+    const req = makeInboundRequest({ content: 'approve' });
+
+    const res = await handleChannelInbound(req, noopProcessMessage, 'token', orchestrator);
+    const body = await res.json() as Record<string, unknown>;
+
+    expect(body.approval).toBe('decision_applied');
+    expect(deliverSpy).not.toHaveBeenCalled();
+
+    deliverSpy.mockRestore();
+  });
+});
+
+// ═══════════════════════════════════════════════════════════════════════════
+// 11. Stale callback with no pending approval returns stale_ignored (WS-B)
+// ═══════════════════════════════════════════════════════════════════════════
+
+describe('stale callback handling', () => {
+  beforeEach(() => {
+    process.env.CHANNEL_APPROVALS_ENABLED = 'true';
+  });
+
+  test('callback with no pending approval returns stale_ignored and does not start a run', async () => {
+    const orchestrator = makeMockOrchestrator();
+
+    // No pending run/approval — send a stale callback
+    const req = makeInboundRequest({
+      content: '',
+      callbackData: 'apr:stale-run:approve_once',
+    });
+
+    const res = await handleChannelInbound(req, noopProcessMessage, 'token', orchestrator);
+    const body = await res.json() as Record<string, unknown>;
+
+    expect(body.accepted).toBe(true);
+    expect(body.approval).toBe('stale_ignored');
+
+    // startRun should NOT have been called — the stale callback must not
+    // enter processChannelMessageWithApprovals or processChannelMessageInBackground
+    expect(orchestrator.startRun).not.toHaveBeenCalled();
+  });
+
+  test('callback with non-empty content but no pending approval returns stale_ignored', async () => {
+    const orchestrator = makeMockOrchestrator();
+
+    // Simulate what normalize.ts does: callbackData present AND content is
+    // set to the callback data value (non-empty). Without the fix, this
+    // would fall through to normal processing because the old guard only
+    // checked for empty content.
+    const req = makeInboundRequest({
+      content: 'apr:stale-run:approve_once',
+      callbackData: 'apr:stale-run:approve_once',
+    });
+
+    const res = await handleChannelInbound(req, noopProcessMessage, 'token', orchestrator);
+    const body = await res.json() as Record<string, unknown>;
+
+    expect(body.accepted).toBe(true);
+    expect(body.approval).toBe('stale_ignored');
+    expect(orchestrator.startRun).not.toHaveBeenCalled();
+  });
+
+  test('non-callback message without pending approval proceeds to normal processing', async () => {
+    const orchestrator = makeMockOrchestrator();
+
+    // Regular text message (no callbackData) should proceed normally
+    const req = makeInboundRequest({ content: 'hello world' });
+
+    const res = await handleChannelInbound(req, noopProcessMessage, 'token', orchestrator);
+    const body = await res.json() as Record<string, unknown>;
+
+    expect(body.accepted).toBe(true);
+    // No approval field — normal processing
+    expect(body.approval).toBeUndefined();
+  });
+});

assistant/src/runtime/routes/channel-routes.ts

@@ -373,11 +373,12 @@ export async function handleChannelInbound(
       });
     }
 
-    // When a callback-only payload (no text content, no attachments) was not
-    // handled by approval interception, it's a stale button press with no
-    // pending approval. Return early instead of falling through to normal
-    // message processing, which would start a run with empty user content.
-    if (hasCallbackData && trimmedContent.length === 0 && !hasAttachments) {
+    // When a callback payload was not handled by approval interception, it's
+    // a stale button press with no pending approval. Return early regardless
+    // of whether content/attachments are present — callback payloads always
+    // have non-empty content (normalize.ts sets message.content to cbq.data),
+    // so checking for empty content alone would miss stale callbacks.
+    if (hasCallbackData) {
       return Response.json({
         accepted: true,
         duplicate: false,
@@ -954,16 +955,9 @@ async function handleApprovalInterception(
 
     const result = handleChannelDecision(conversationId, decision, orchestrator);
 
-    if (result.applied) {
-      // Deliver the run's result back to the channel once the decision is applied.
-      // The run will resume in the background; deliver whatever assistant reply
-      // is available now (there may not be one yet if the run is still processing).
-      try {
-        await deliverReplyViaCallback(conversationId, externalChatId, replyCallbackUrl, bearerToken);
-      } catch (err) {
-        log.error({ err, conversationId }, 'Failed to deliver post-decision reply');
-      }
-    }
+    // The decision is applied; the final reply will be delivered by the
+    // terminal run completion path in processChannelMessageWithApprovals.
+    // No immediate reply is sent here to avoid duplicate messages.
 
     return { handled: true, type: 'decision_applied' };
   }