From 8f96782c81b453b196ecb1ba14f1f08c05eeb00c Mon Sep 17 00:00:00 2001 From: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com> Date: Sat, 8 Aug 2026 14:35:58 -0300 Subject: [PATCH 1/3] 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. --- .../github/utils/parse-link-metadata.ts | 25 ++++++++++++ .../plugins/github/webhooks/issue-closed.ts | 10 ++--- .../plugins/github/webhooks/issue-edited.ts | 8 ++-- .../github/webhooks/pull-request-closed.ts | 13 +++++-- .../github/utils/parse-link-metadata.test.ts | 38 +++++++++++++++++++ 5 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 apps/api/src/plugins/github/utils/parse-link-metadata.ts create mode 100644 tests/api/plugins/github/utils/parse-link-metadata.test.ts diff --git a/apps/api/src/plugins/github/utils/parse-link-metadata.ts b/apps/api/src/plugins/github/utils/parse-link-metadata.ts new file mode 100644 index 000000000..ebe14e01b --- /dev/null +++ b/apps/api/src/plugins/github/utils/parse-link-metadata.ts @@ -0,0 +1,25 @@ +// 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. +export function parseLinkMetadata( + raw: string | null | undefined, + context: { externalLinkId: string; source: string }, +): Record { + if (!raw) { + return {}; + } + + try { + return JSON.parse(raw) as Record; + } catch (error) { + console.warn("Failed to parse GitHub external link metadata", { + ...context, + metadata: raw, + error, + }); + + return {}; + } +} diff --git a/apps/api/src/plugins/github/webhooks/issue-closed.ts b/apps/api/src/plugins/github/webhooks/issue-closed.ts index 10e622595..9f490825b 100644 --- a/apps/api/src/plugins/github/webhooks/issue-closed.ts +++ b/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; } } diff --git a/apps/api/src/plugins/github/webhooks/issue-edited.ts b/apps/api/src/plugins/github/webhooks/issue-edited.ts index 9b4e48dd6..ab570bd8a 100644 --- a/apps/api/src/plugins/github/webhooks/issue-edited.ts +++ b/apps/api/src/plugins/github/webhooks/issue-edited.ts @@ -4,6 +4,7 @@ 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"; type IssueEditedPayload = { action: string; @@ -63,9 +64,10 @@ export async function handleIssueEdited(payload: IssueEditedPayload) { continue; } - const metadata = externalLink.metadata - ? JSON.parse(externalLink.metadata) - : {}; + const metadata = parseLinkMetadata(externalLink.metadata, { + externalLinkId: externalLink.id, + source: "issue_edited", + }); const updateData: Record = {}; const updatedMetadata = { ...metadata }; diff --git a/apps/api/src/plugins/github/webhooks/pull-request-closed.ts b/apps/api/src/plugins/github/webhooks/pull-request-closed.ts index fc674e573..a4a7bbbfd 100644 --- a/apps/api/src/plugins/github/webhooks/pull-request-closed.ts +++ b/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"; }); diff --git a/tests/api/plugins/github/utils/parse-link-metadata.test.ts b/tests/api/plugins/github/utils/parse-link-metadata.test.ts new file mode 100644 index 000000000..8f329520f --- /dev/null +++ b/tests/api/plugins/github/utils/parse-link-metadata.test.ts @@ -0,0 +1,38 @@ +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", + }); + }); +}); From 32a44478b10fc7396878a57b80994785cf13c11b Mon Sep 17 00:00:00 2001 From: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com> Date: Sat, 8 Aug 2026 14:41:11 -0300 Subject: [PATCH 2/3] 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. --- .../github/utils/parse-link-metadata.ts | 9 ++++-- .../plugins/github/webhooks/issue-edited.ts | 32 +++++++++++++++---- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/apps/api/src/plugins/github/utils/parse-link-metadata.ts b/apps/api/src/plugins/github/utils/parse-link-metadata.ts index ebe14e01b..0ff9bb3a2 100644 --- a/apps/api/src/plugins/github/utils/parse-link-metadata.ts +++ b/apps/api/src/plugins/github/utils/parse-link-metadata.ts @@ -3,16 +3,19 @@ // 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. -export function parseLinkMetadata( +// 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>( raw: string | null | undefined, context: { externalLinkId: string; source: string }, -): Record { +): Partial { if (!raw) { return {}; } try { - return JSON.parse(raw) as Record; + return JSON.parse(raw) as Partial; } catch (error) { console.warn("Failed to parse GitHub external link metadata", { ...context, diff --git a/apps/api/src/plugins/github/webhooks/issue-edited.ts b/apps/api/src/plugins/github/webhooks/issue-edited.ts index ab570bd8a..eaafa9e32 100644 --- a/apps/api/src/plugins/github/webhooks/issue-edited.ts +++ b/apps/api/src/plugins/github/webhooks/issue-edited.ts @@ -6,6 +6,21 @@ 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; issue: { @@ -64,13 +79,16 @@ export async function handleIssueEdited(payload: IssueEditedPayload) { continue; } - const metadata = parseLinkMetadata(externalLink.metadata, { - externalLinkId: externalLink.id, - source: "issue_edited", - }); + const metadata = parseLinkMetadata( + externalLink.metadata, + { + externalLinkId: externalLink.id, + source: "issue_edited", + }, + ); const updateData: Record = {}; - const updatedMetadata = { ...metadata }; + const updatedMetadata: IssueEditedMetadata = { ...metadata }; if (!updatedMetadata.lastSync) { updatedMetadata.lastSync = {}; @@ -91,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)`, @@ -131,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)`, From cdf3ce2a4e022968a4f98383681346f4c5e514b8 Mon Sep 17 00:00:00 2001 From: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com> Date: Sat, 8 Aug 2026 16:49:34 -0300 Subject: [PATCH 3/3] 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. --- .../github/utils/parse-link-metadata.ts | 25 +++++++++++++++-- .../github/utils/parse-link-metadata.test.ts | 28 +++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/apps/api/src/plugins/github/utils/parse-link-metadata.ts b/apps/api/src/plugins/github/utils/parse-link-metadata.ts index 0ff9bb3a2..98f9982df 100644 --- a/apps/api/src/plugins/github/utils/parse-link-metadata.ts +++ b/apps/api/src/plugins/github/utils/parse-link-metadata.ts @@ -14,15 +14,36 @@ export function parseLinkMetadata>( return {}; } + let parsed: unknown; + try { - return JSON.parse(raw) as Partial; + parsed = JSON.parse(raw); } catch (error) { console.warn("Failed to parse GitHub external link metadata", { ...context, - metadata: raw, 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; } diff --git a/tests/api/plugins/github/utils/parse-link-metadata.test.ts b/tests/api/plugins/github/utils/parse-link-metadata.test.ts index 8f329520f..4c4b5b78b 100644 --- a/tests/api/plugins/github/utils/parse-link-metadata.test.ts +++ b/tests/api/plugins/github/utils/parse-link-metadata.test.ts @@ -35,4 +35,32 @@ describe("parseLinkMetadata", () => { 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); + }); });