fix(github): keep a webhook delivery alive when link metadata will not parse

apps/api/src/plugins/github/utils/parse-link-metadata.ts

@@ -0,0 +1,49 @@
+// An external link's metadata is a JSON string in the database, so a row
+// written by an older version, or truncated, is not the caller's fault and
+// should not take the webhook delivery down with it. The Gitea handlers already
+// warn and carry on with an empty object; this is the same behaviour in one
+// place, since the GitHub side needs it in four.
+// The shape is whatever an earlier write left behind, so callers that read
+// named fields say what they expect. The default keeps the untyped reading for
+// the handlers that only spread the value forward.
+export function parseLinkMetadata<T extends object = Record<string, unknown>>(
+  raw: string | null | undefined,
+  context: { externalLinkId: string; source: string },
+): Partial<T> {
+  if (!raw) {
+    return {};
+  }
+
+  let parsed: unknown;
+
+  try {
+    parsed = JSON.parse(raw);
+  } catch (error) {
+    console.warn("Failed to parse GitHub external link metadata", {
+      ...context,
+      error,
+    });
+
+    return {};
+  }
+
+  // Parsing succeeding is not the same as the row holding metadata. The
+  // literal `null` parses to `null`, `"kaneo"` to a string, and either would
+  // pass the return type on to a caller that reads a named field off it, which
+  // is the crash this helper exists to prevent. An array is not what any writer
+  // here produces either, and spreading one forward would turn its indices into
+  // keys. Both take the same exit as a row that will not parse.
+  if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
+    console.warn(
+      "Ignoring GitHub external link metadata that is not an object",
+      {
+        ...context,
+        metadataType: parsed === null ? "null" : typeof parsed,
+      },
+    );
+
+    return {};
+  }
+
+  return parsed as Partial<T>;
+}

apps/api/src/plugins/github/webhooks/issue-closed.ts

@@ -7,6 +7,7 @@ import {
   findAllIntegrationsByRepo,
   updateTaskStatus,
 } from "../services/task-service";
+import { parseLinkMetadata } from "../utils/parse-link-metadata";
 import { resolveTargetStatus } from "../utils/resolve-column";
 
 type IssueClosedPayload = {
@@ -53,9 +54,10 @@ export async function handleIssueClosed(payload: IssueClosedPayload) {
       continue;
     }
 
-    const existingMetadata = externalLink.metadata
-      ? JSON.parse(externalLink.metadata)
-      : {};
+    const existingMetadata = parseLinkMetadata(externalLink.metadata, {
+      externalLinkId: externalLink.id,
+      source: "issue_closed",
+    });
 
     if (existingMetadata.createdFrom === "kaneo") {
       continue;
@@ -90,7 +92,5 @@ export async function handleIssueClosed(payload: IssueClosedPayload) {
         state: "closed",
       },
     });
-
-    return;
   }
 }

apps/api/src/plugins/github/webhooks/issue-edited.ts

@@ -4,6 +4,22 @@ import { taskTable } from "../../../database/schema";
 import { findExternalLink, updateExternalLink } from "../services/link-manager";
 import { findAllIntegrationsByRepo } from "../services/task-service";
 import { formatTaskDescriptionFromIssue } from "../utils/format";
+import { parseLinkMetadata } from "../utils/parse-link-metadata";
+
+// What this handler reads back out of the row. Every field is optional,
+// because the row may predate any of them.
+type SyncStamp = {
+  timestamp?: string;
+  source?: string;
+  value?: string;
+};
+
+type IssueEditedMetadata = {
+  lastSync?: {
+    title?: SyncStamp;
+    description?: SyncStamp;
+  };
+};
 
 type IssueEditedPayload = {
   action: string;
@@ -63,12 +79,16 @@ export async function handleIssueEdited(payload: IssueEditedPayload) {
       continue;
     }
 
-    const metadata = externalLink.metadata
-      ? JSON.parse(externalLink.metadata)
-      : {};
+    const metadata = parseLinkMetadata<IssueEditedMetadata>(
+      externalLink.metadata,
+      {
+        externalLinkId: externalLink.id,
+        source: "issue_edited",
+      },
+    );
 
     const updateData: Record<string, unknown> = {};
-    const updatedMetadata = { ...metadata };
+    const updatedMetadata: IssueEditedMetadata = { ...metadata };
 
     if (!updatedMetadata.lastSync) {
       updatedMetadata.lastSync = {};
@@ -89,7 +109,7 @@ export async function handleIssueEdited(payload: IssueEditedPayload) {
         }
 
         const timeSinceLastSync =
-          Date.now() - new Date(lastTitleSync.timestamp).getTime();
+          Date.now() - new Date(lastTitleSync.timestamp ?? 0).getTime();
         if (timeSinceLastSync < 2000 && shouldUpdateTitle) {
           console.log(
             `Skipping title update - recent sync detected (${timeSinceLastSync}ms ago)`,
@@ -129,7 +149,7 @@ export async function handleIssueEdited(payload: IssueEditedPayload) {
         }
 
         const timeSinceLastSync =
-          Date.now() - new Date(lastDescSync.timestamp).getTime();
+          Date.now() - new Date(lastDescSync.timestamp ?? 0).getTime();
         if (timeSinceLastSync < 2000 && shouldUpdateDescription) {
           console.log(
             `Skipping description update - recent sync detected (${timeSinceLastSync}ms ago)`,

apps/api/src/plugins/github/webhooks/pull-request-closed.ts

@@ -9,6 +9,7 @@ import {
   findTaskById,
   updateTaskStatus,
 } from "../services/task-service";
+import { parseLinkMetadata } from "../utils/parse-link-metadata";
 import { resolveTargetStatus } from "../utils/resolve-column";
 
 type PRClosedPayload = {
@@ -59,9 +60,10 @@ export async function handlePullRequestClosed(payload: PRClosedPayload) {
       continue;
     }
 
-    const existingMetadata = externalLink.metadata
-      ? JSON.parse(externalLink.metadata)
-      : {};
+    const existingMetadata = parseLinkMetadata(externalLink.metadata, {
+      externalLinkId: externalLink.id,
+      source: "pull_request_closed",
+    });
 
     await updateExternalLink(externalLink.id, {
       metadata: {
@@ -82,7 +84,10 @@ export async function handlePullRequestClosed(payload: PRClosedPayload) {
 
       const hasOpenPRs = allTaskPRs.some((pr) => {
         if (pr.id === externalLink.id) return false;
-        const metadata = pr.metadata ? JSON.parse(pr.metadata) : {};
+        const metadata = parseLinkMetadata(pr.metadata, {
+          externalLinkId: pr.id,
+          source: "pull_request_closed",
+        });
         return metadata.state === "open";
       });
 

tests/api/plugins/github/utils/parse-link-metadata.test.ts

@@ -0,0 +1,66 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { parseLinkMetadata } from "../../../../../apps/api/src/plugins/github/utils/parse-link-metadata";
+
+const context = { externalLinkId: "link-1", source: "issue_closed" };
+
+afterEach(() => {
+  vi.restoreAllMocks();
+});
+
+describe("parseLinkMetadata", () => {
+  it("reads a well formed object", () => {
+    expect(
+      parseLinkMetadata('{"state":"open","createdFrom":"kaneo"}', context),
+    ).toEqual({
+      state: "open",
+      createdFrom: "kaneo",
+    });
+  });
+
+  it("treats an absent value as no metadata", () => {
+    expect(parseLinkMetadata(null, context)).toEqual({});
+    expect(parseLinkMetadata(undefined, context)).toEqual({});
+    expect(parseLinkMetadata("", context)).toEqual({});
+  });
+
+  it("warns and carries on when the row cannot be parsed", () => {
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
+
+    // A row truncated by an older write, which used to throw out of the handler
+    // and fail the whole webhook delivery.
+    expect(parseLinkMetadata('{"state":"op', context)).toEqual({});
+    expect(warn).toHaveBeenCalledOnce();
+    expect(warn.mock.calls[0][1]).toMatchObject({
+      externalLinkId: "link-1",
+      source: "issue_closed",
+    });
+  });
+
+  it("keeps the row out of the warning", () => {
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
+
+    // The row can hold a task description synced from Kaneo, so the log gets
+    // the link id to find it by and not the content itself.
+    parseLinkMetadata(
+      '{"lastSync":{"description":"segredo do cliente"',
+      context,
+    );
+    expect(JSON.stringify(warn.mock.calls[0])).not.toContain(
+      "segredo do cliente",
+    );
+  });
+
+  it("ignores a row that parses to something other than an object", () => {
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
+
+    // `JSON.parse` answers these without throwing, so they used to reach the
+    // handlers typed as metadata. `null` is the one that bites: reading
+    // `metadata.createdFrom` off it throws inside the webhook.
+    expect(parseLinkMetadata("null", context)).toEqual({});
+    expect(parseLinkMetadata('"kaneo"', context)).toEqual({});
+    expect(parseLinkMetadata("42", context)).toEqual({});
+    expect(parseLinkMetadata("true", context)).toEqual({});
+    expect(parseLinkMetadata('["kaneo"]', context)).toEqual({});
+    expect(warn).toHaveBeenCalledTimes(5);
+  });
+});