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

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

An external link's metadata is a JSON string in the database, so a row written
by an older version, or a truncated one, is not something the delivery can do
anything about. On the GitHub side four handlers called `JSON.parse` on it bare,
so the throw escaped the handler and the whole delivery failed, leaving the task
unsynced with no record of why.

The Gitea handlers already warn and carry on with an empty object. This is the
same behaviour, in one helper, since the GitHub side needs it in four places.

`issue-closed` also stopped at the first matching integration, so a repository
connected to two projects only moved one task. Its Gitea twin and
`issue-opened` on this side both walk all of them. `pull-request-closed` returns
early on both sides, so that one is left as it is.

* fix(github): type the metadata the edit handler reads back

`JSON.parse` returned `any`, so reading `metadata.lastSync.title` off it went
unchecked. The helper hands back a typed value, which made `tsc` point at four
reads that were never verified.

`parseLinkMetadata` takes the shape from the caller now, defaulting to the
untyped record for the handlers that only spread it forward, and the edit
handler declares what it looks for. Every field is optional, since a row may
predate any of them.

A stamp with no timestamp used to become an Invalid Date, whose NaN failed the
recency comparison; the epoch fails it the same way, so the branch taken does
not change.

* fix(github): ignore link metadata that parses to something other than an object

`JSON.parse` answers `null` for the row `null`, and a string for `"kaneo"`. The
helper handed either one back as metadata, so a caller reading
`metadata.createdFrom` off it threw inside the webhook, which is the crash this
helper exists to prevent.

A parsed value that is not a plain object now takes the same exit as a row that
will not parse. Arrays go with them: nothing here writes one, and spreading it
forward would turn its indices into keys.

The warning no longer carries the row itself. A row can hold a task description
synced from Kaneo, and the link id is enough to find it by.

---------

Co-authored-by: (justin)randoneering <justin@randoneering.tech>

Author: luantaraschi

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);
+  });
+});