From 84fa7e2baa0d6f180b0033dfdd094f41869fd00a Mon Sep 17 00:00:00 2001
From: Jonathan Jing <achillesjing@gmail.com>
Date: Tue, 10 Mar 2026 12:58:47 -0700
Subject: [PATCH] fix(mattermost): parse status code before matching generic
 '429' text

- Fix isRetryableError: check 'mattermost api 429' status code BEFORE
  generic '429' text match to avoid false positives
- Error messages containing '429' in detail (e.g., 'Invalid ID: 4294967295')
  are no longer incorrectly treated as rate limiting
- Add test for 400 error containing '429' text to verify no false retry
---
 .../src/mattermost/client.retry.test.ts       | 24 +++++++++++++++++++
 .../mattermost/src/mattermost/client.ts       | 13 +++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/extensions/mattermost/src/mattermost/client.retry.test.ts b/extensions/mattermost/src/mattermost/client.retry.test.ts
index 8fedc16dfd4..1296b8448d0 100644
--- a/extensions/mattermost/src/mattermost/client.retry.test.ts
+++ b/extensions/mattermost/src/mattermost/client.retry.test.ts
@@ -98,6 +98,30 @@ describe("createMattermostDirectChannelWithRetry", () => {
     expect(result.id).toBe("dm-channel-port");
   });
 
+  it("does not retry on 400 even if error message contains '429' text", async () => {
+    // This tests that "429" in error detail doesn't trigger false rate-limit retry
+    // e.g., "Invalid user ID: 4294967295" should NOT be retried
+    mockFetch.mockResolvedValueOnce({
+      ok: false,
+      status: 400,
+      headers: new Headers({ "content-type": "application/json" }),
+      json: async () => ({ message: "Invalid user ID: 4294967295" }),
+      text: async () => "Invalid user ID: 4294967295",
+    } as Response);
+
+    const client = createMockClient();
+
+    await expect(
+      createMattermostDirectChannelWithRetry(client, ["user-1", "user-2"], {
+        maxRetries: 3,
+        initialDelayMs: 10,
+      }),
+    ).rejects.toThrow();
+
+    // Should not retry - only called once (400 is a client error, even though message contains "429")
+    expect(mockFetch).toHaveBeenCalledTimes(1);
+  });
+
   it("retries on 5xx server errors", async () => {
     mockFetch
       .mockResolvedValueOnce({
diff --git a/extensions/mattermost/src/mattermost/client.ts b/extensions/mattermost/src/mattermost/client.ts
index a71950117a4..dfb8fdde726 100644
--- a/extensions/mattermost/src/mattermost/client.ts
+++ b/extensions/mattermost/src/mattermost/client.ts
@@ -257,11 +257,6 @@ export async function createMattermostDirectChannelWithRetry(
 function isRetryableError(error: Error): boolean {
   const message = error.message.toLowerCase();
 
-  // Retry on rate limiting (429)
-  if (message.includes("429") || message.includes("too many requests")) {
-    return true;
-  }
-
   // Retry on 5xx server errors FIRST (before checking 4xx)
   // Use "mattermost api" prefix to avoid matching port numbers (e.g., :443) or IP octets
   // This prevents misclassification when a 5xx error detail contains a 4xx substring
@@ -270,13 +265,19 @@ function isRetryableError(error: Error): boolean {
     return true;
   }
 
+  // Check for explicit 429 rate limiting FIRST (before generic "429" text match)
+  // This avoids retrying when error detail contains "429" but it's not the status code
+  if (/mattermost api 429\b/.test(message) || message.includes("too many requests")) {
+    return true;
+  }
+
   // Check for explicit 4xx status codes - these are client errors and should NOT be retried
   // (except 429 which is handled above)
   // Use "mattermost api" prefix to avoid matching port numbers like :443
   const clientErrorMatch = message.match(/mattermost api (4\d{2})\b/);
   if (clientErrorMatch) {
     const statusCode = parseInt(clientErrorMatch[1], 10);
-    if (statusCode >= 400 && statusCode < 500 && statusCode !== 429) {
+    if (statusCode >= 400 && statusCode < 500) {
       return false;
     }
   }