fix(provider-microsoft): return structured failure when reply createReplyAll fails (#50) (#54)

* fix(provider-microsoft): return structured failure when reply createReplyAll fails

When createReplyAll threw inside replyToMessage (e.g., the original message
was deleted and Graph returned 404), the catch fell through to a manual
sendMessage that used opts?.cc as `to:` and a literal `'Re: '` as subject.
The result was success: true with the email going to the wrong recipients
(or no one) and no thread context — silent mis-send.

Replace the silent fallback with a structured failure that mirrors
createReplyDraft:

  { success: false, error: { code: 'REPLY_FAILED', message, recoverable: false } }

A sendMail-based "fallback reply" is not actually a reply: it lacks
In-Reply-To / References headers so recipients' clients won't thread it,
and no amount of hydrating to/subject fixes that. Loud failure is correct;
the caller decides whether to send a fresh email.

Tests:
- Replace the broken-fallback scenario that asserted success: true.
- Add four failure-path tests covering createReplyAll throw, opts.cc
  regression guard, /send failure after a successful PATCH, and PATCH
  failure inside prepareReplyDraft.

Closes #50

* docs(spec): align provider-microsoft reply spec with new failure shape

Update the Draft-Then-Send via createReplyAll requirement to reflect that
replyToMessage now returns a structured REPLY_FAILED instead of silently
falling back to sendMail. Replace the stale "Fallback to sendMail on 404"
scenario with one that captures the new contract.

Spotted by Gemini and Codex during peer review of #54.

Author: stevenobiajulu

openspec/specs/provider-microsoft/spec.md

@@ -29,7 +29,7 @@ The system SHALL support client credentials (app-only) authentication for daemon
 
 ### Requirement: Draft-Then-Send via createReplyAll
 
-The system SHALL use `createReplyAll` (not `sendMail`) for replies. `createReplyAll` preserves embedded images, CID references, and thread metadata. The system merges Graph's auto-quoted body with caller content rather than overwriting it. `sendMail` is fallback only when the original message is deleted (404).
+The system SHALL use `createReplyAll` for replies. `createReplyAll` preserves embedded images, CID references, and thread metadata. The system merges Graph's auto-quoted body with caller content rather than overwriting it. When `createReplyAll`, the body merge PATCH, or the final `/send` POST fails, `replyToMessage` SHALL return a structured `{ success: false, error: { code: 'REPLY_FAILED', recoverable: false } }` rather than silently falling back to `sendMail` — a `sendMail`-based message would lack `In-Reply-To` / `References` headers and so would not thread on the recipient side.
 
 #### Scenario: Reply preserves Graph auto-quoted thread (plain text)
 - **WHEN** the original email has prior thread history
@@ -41,9 +41,10 @@ The system SHALL use `createReplyAll` (not `sendMail`) for replies. `createReply
 - **AND** the system replies via `createReplyAll`
 - **THEN** the merged draft body retains every `cid:` reference intact
 
-#### Scenario: Fallback to sendMail on 404
-- **WHEN** `createReplyAll` returns 404 (original message deleted)
-- **THEN** the system falls back to `sendMail` with manually constructed quoted content
+#### Scenario: createReplyAll failure returns structured REPLY_FAILED
+- **WHEN** `createReplyAll`, the body-merge PATCH, or the final `/send` POST throws
+- **THEN** `replyToMessage` returns `{ success: false, error: { code: 'REPLY_FAILED', recoverable: false } }`
+- **AND** does not call `sendMail`
 
 ### Requirement: Validation Token Handling
 

packages/provider-microsoft/src/email-graph-provider.test.ts

@@ -475,25 +475,75 @@ describe('provider-microsoft/Draft-Then-Send via createReplyAll', () => {
     expect(content.indexOf('<p>rendered</p>')).toBeLessThan(content.indexOf('just a fragment'));
   });
 
-  it('Scenario: Fallback to sendMail on 404', async () => {
+  it('Scenario: createReplyAll failure returns structured REPLY_FAILED', async () => {
     const client = createMockClient({
-      post: vi.fn()
-        .mockRejectedValueOnce(new Error('404 Not Found')) // createReplyAll fails
-        .mockResolvedValueOnce({ id: 'sent-msg' }), // sendMail fallback
-      get: vi.fn().mockResolvedValue({
-        id: 'deleted-msg',
-        subject: 'Deleted',
-        from: { emailAddress: { address: 'alice@corp.com' } },
-        receivedDateTime: '2024-03-15T10:00:00Z',
-      }),
+      post: vi.fn().mockRejectedValueOnce(new GraphApiError(404, 'Not Found')),
     });
     const provider = new GraphEmailProvider(client);
 
     const result = await provider.replyToMessage('deleted-msg', 'Response');
 
-    expect(result.success).toBe(true);
-    // Falls back to sendMail
-    expect(client.post).toHaveBeenCalledWith(
+    expect(result.success).toBe(false);
+    expect(result.error?.code).toBe('REPLY_FAILED');
+    expect(result.error?.recoverable).toBe(false);
+    expect(result.error?.message).toBeTruthy();
+    // Critical: does NOT silently send via sendMail
+    expect(client.post).not.toHaveBeenCalledWith(
+      expect.stringContaining('sendMail'),
+      expect.anything(),
+    );
+  });
+
+  it('Scenario: replyToMessage failure does not use opts.cc as to:', async () => {
+    // Regression guard for the silent-fallback bug — even when cc is supplied,
+    // a createReplyAll failure must not turn into a fresh email to the cc list.
+    const client = createMockClient({
+      post: vi.fn().mockRejectedValueOnce(new GraphApiError(404, 'Not Found')),
+    });
+    const provider = new GraphEmailProvider(client);
+
+    const result = await provider.replyToMessage('deleted-msg', 'Response', {
+      cc: [{ email: 'bystander@corp.com' }],
+    });
+
+    expect(result.success).toBe(false);
+    expect(client.post).toHaveBeenCalledTimes(1); // only the failed createReplyAll
+  });
+
+  it('Scenario: send failure (createReplyAll succeeds, /send fails) returns REPLY_FAILED', async () => {
+    const client = createMockClient({
+      post: vi.fn()
+        .mockResolvedValueOnce(quotedReplyResponse())
+        .mockRejectedValueOnce(new GraphApiError(500, 'Server Error')),
+    });
+    const provider = new GraphEmailProvider(client);
+
+    const result = await provider.replyToMessage('msg-1', 'Response');
+
+    expect(result.success).toBe(false);
+    expect(result.error?.code).toBe('REPLY_FAILED');
+    expect(result.error?.message).toBeTruthy();
+  });
+
+  it('Scenario: PATCH failure inside prepareReplyDraft returns REPLY_FAILED', async () => {
+    // Helper-stage failure: createReplyAll succeeds, PATCH rejects. Previously
+    // this also fell into the broken sendMail fallback.
+    const client = createMockClient({
+      post: vi.fn().mockResolvedValueOnce(quotedReplyResponse()),
+      patch: vi.fn().mockRejectedValueOnce(new GraphApiError(500, 'Server Error')),
+    });
+    const provider = new GraphEmailProvider(client);
+
+    const result = await provider.replyToMessage('msg-1', 'Response');
+
+    expect(result.success).toBe(false);
+    expect(result.error?.code).toBe('REPLY_FAILED');
+    // Critical: no /send and no /sendMail call
+    expect(client.post).not.toHaveBeenCalledWith(
+      expect.stringContaining('/send'),
+      expect.anything(),
+    );
+    expect(client.post).not.toHaveBeenCalledWith(
       expect.stringContaining('sendMail'),
       expect.anything(),
     );

packages/provider-microsoft/src/email-graph-provider.ts

@@ -320,17 +320,10 @@ export class GraphEmailProvider implements EmailReader, EmailSender, EmailCatego
       const draftId = await this.prepareReplyDraft(messageId, body, opts);
       await this.client.post(`${this.basePath}/messages/${draftId}/send`, {});
       return { success: true, messageId: draftId };
-    } catch {
-      // Fallback to sendMail on 404 (original deleted) or other failures
+    } catch (err) {
+      const message = err instanceof Error ? err.message : 'Failed to send reply';
+      return { success: false, error: { code: 'REPLY_FAILED', message, recoverable: false } };
     }
-
-    // Fallback: construct reply manually via sendMail
-    return this.sendMessage({
-      to: opts?.cc ?? [],
-      subject: `Re: `,
-      body,
-      bodyHtml: opts?.bodyHtml,
-    });
   }
 
   async createDraft(msg: ComposeMessage): Promise<DraftResult> {