From f6999839c14479209dc47fc854712a1ed28d6ffd Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 23 Jun 2026 00:45:56 +0000 Subject: [PATCH] Address external object review findings --- .../external-objects-service.test.ts | 51 ++++++++++++++++--- server/src/services/external-objects.ts | 10 +++- .../github-external-object-provider.ts | 2 +- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/external-objects-service.test.ts b/server/src/__tests__/external-objects-service.test.ts index 03a81f09ab6..3bfe5225dc1 100644 --- a/server/src/__tests__/external-objects-service.test.ts +++ b/server/src/__tests__/external-objects-service.test.ts @@ -182,7 +182,7 @@ describe("GitHub external object provider", () => { providerKey: "github", objectType: "pull_request", externalId: "acme/app#pull/42", - displayKey: "Github Pull Request", + displayKey: "GitHub Pull Request", iconKey: "github", displayTitle: "Acme/App#42", }), @@ -190,7 +190,7 @@ describe("GitHub external object provider", () => { providerKey: "github", objectType: "issue", externalId: "acme/app#issues/7", - displayKey: "Github Issue", + displayKey: "GitHub Issue", iconKey: "github", displayTitle: "Acme/App#7", }), @@ -239,7 +239,7 @@ describe("GitHub external object provider", () => { ok: true, snapshot: expect.objectContaining({ ...expected, - displayKey: "Github Pull Request", + displayKey: "GitHub Pull Request", iconKey: "github", displayTitle: expect.stringContaining(String(body.title)), remoteVersion: "2026-04-24T01:02:03Z", @@ -281,7 +281,7 @@ describe("GitHub external object provider", () => { ok: true, snapshot: expect.objectContaining({ ...expected, - displayKey: "Github Issue", + displayKey: "GitHub Issue", iconKey: "github", data: expect.objectContaining({ provider: "github", @@ -330,7 +330,7 @@ describe("GitHub external object provider", () => { [ "not-found", new Response("", { status: 404, headers: { etag: '"missing"' } }), - { ok: true, snapshot: expect.objectContaining({ displayKey: "Github Pull Request", iconKey: "github", statusKey: "not_found", statusIconKey: "archive", statusCategory: "archived", statusTone: "muted" }) }, + { ok: true, snapshot: expect.objectContaining({ displayKey: "GitHub Pull Request", iconKey: "github", statusKey: "not_found", statusIconKey: "archive", statusCategory: "archived", statusTone: "muted" }) }, ], ])("maps %s responses to provider-safe results", async (_name, githubResponse, expected) => { const provider = createGitHubExternalObjectProvider({} as any, { @@ -466,7 +466,7 @@ describeEmbeddedPostgres("externalObjectService", () => { ok: false, liveness: "auth_required", errorCode: "auth_required", - errorMessage: "token failed for https://github.com/acme/app/pull/42?token=secret", + errorMessage: "token=secret failed for https://github.com/acme/app/pull/42?token=secret", retryAfterSeconds: 60, }) .mockResolvedValueOnce({ @@ -483,6 +483,11 @@ describeEmbeddedPostgres("externalObjectService", () => { await svc.refreshObject(object.id, { companyId, force: true }); await svc.refreshObject(object.id, { companyId, force: true }); + + const authFailure = await db.select().from(externalObjects).then((rows) => rows[0]!); + expect(authFailure.lastErrorMessage).toContain("token=[redacted]"); + expect(authFailure.lastErrorMessage).not.toContain("secret"); + await svc.refreshObject(object.id, { companyId, force: true }); const updated = await db.select().from(externalObjects).then((rows) => rows[0]!); @@ -493,6 +498,40 @@ describeEmbeddedPostgres("externalObjectService", () => { expect(updated.lastErrorMessage).not.toContain("secret"); }); + it("skips terminal objects when refreshing due objects", async () => { + const { companyId, issueId } = await createIssue(); + const resolve = vi.fn(async () => ({ + ok: true as const, + snapshot: { + statusCategory: "closed" as const, + statusTone: "muted" as const, + statusKey: "closed", + statusLabel: "Closed", + isTerminal: true, + ttlSeconds: 1, + }, + })); + const resolver: ExternalObjectResolver = { + providerKey: "url", + objectType: "link", + resolve, + }; + const svc = externalObjectService(db, { resolvers: [resolver], github: false }); + await svc.syncIssue(issueId); + const object = await db.select().from(externalObjects).then((rows) => rows[0]!); + + await svc.refreshObject(object.id, { companyId, force: true }); + await db + .update(externalObjects) + .set({ nextRefreshAt: new Date(0) }) + .where(eq(externalObjects.id, object.id)); + + const refreshed = await svc.refreshDueObjects(companyId); + + expect(refreshed).toEqual([]); + expect(resolve).toHaveBeenCalledTimes(1); + }); + it("keeps external object identities company-scoped for duplicate urls", async () => { const first = await createIssue(); const second = await createIssue(); diff --git a/server/src/services/external-objects.ts b/server/src/services/external-objects.ts index ba0a292b49c..6cf85f72924 100644 --- a/server/src/services/external-objects.ts +++ b/server/src/services/external-objects.ts @@ -142,7 +142,7 @@ function sanitizeErrorMessage(message: string | null | undefined) { if (!message) return null; return message .replace(/https?:\/\/[^\s<>()]+/gi, "[redacted-url]") - .replace(/\b(?:token|key|secret|authorization|bearer)=\S+/gi, "$1=[redacted]"); + .replace(/\b(token|key|secret|authorization|bearer)=\S+/gi, "$1=[redacted]"); } function genericUrlDetector(): ExternalObjectDetector { @@ -895,7 +895,13 @@ export function externalObjectService( const due = await db .select({ id: externalObjects.id }) .from(externalObjects) - .where(and(eq(externalObjects.companyId, companyId), lte(externalObjects.nextRefreshAt, now))) + .where( + and( + eq(externalObjects.companyId, companyId), + eq(externalObjects.isTerminal, false), + lte(externalObjects.nextRefreshAt, now), + ), + ) .limit(limit); const results = []; for (const row of due) { diff --git a/server/src/services/github-external-object-provider.ts b/server/src/services/github-external-object-provider.ts index 9cfa4d6ddeb..f434b24b411 100644 --- a/server/src/services/github-external-object-provider.ts +++ b/server/src/services/github-external-object-provider.ts @@ -106,7 +106,7 @@ function displayTitleFor(identity: GitHubObjectIdentity) { } function displayKeyFor(identity: Pick) { - return identity.objectType === "pull_request" ? "Github Pull Request" : "Github Issue"; + return identity.objectType === "pull_request" ? "GitHub Pull Request" : "GitHub Issue"; } function retryAfterSeconds(response: Response) {