From 72f5fe578ffd6854227bb5b3888cbcef0111f256 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 21:07:24 -0700 Subject: [PATCH 1/4] fix(confluence): exclude archived pages from KB connector listings so reconciliation purges them --- .../connectors/confluence/confluence.test.ts | 21 +++++++++- apps/sim/connectors/confluence/confluence.ts | 40 ++++++++++++++----- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/apps/sim/connectors/confluence/confluence.test.ts b/apps/sim/connectors/confluence/confluence.test.ts index 629deae5f03..706023ccb93 100644 --- a/apps/sim/connectors/confluence/confluence.test.ts +++ b/apps/sim/connectors/confluence/confluence.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { escapeCql } from '@/connectors/confluence/confluence' +import { escapeCql, isCurrentContent } from '@/connectors/confluence/confluence' describe('escapeCql', () => { it.concurrent('returns plain strings unchanged', () => { @@ -29,3 +29,22 @@ describe('escapeCql', () => { expect(escapeCql("it's a test & ")).toBe("it's a test & ") }) }) + +describe('isCurrentContent', () => { + it.concurrent('keeps current content', () => { + expect(isCurrentContent({ id: '1', status: 'current' })).toBe(true) + }) + + it.concurrent('keeps content with no status field', () => { + expect(isCurrentContent({ id: '1' })).toBe(true) + }) + + it.concurrent('excludes archived content', () => { + expect(isCurrentContent({ id: '1', status: 'archived' })).toBe(false) + }) + + it.concurrent('excludes trashed and deleted content', () => { + expect(isCurrentContent({ id: '1', status: 'trashed' })).toBe(false) + expect(isCurrentContent({ id: '1', status: 'deleted' })).toBe(false) + }) +}) diff --git a/apps/sim/connectors/confluence/confluence.ts b/apps/sim/connectors/confluence/confluence.ts index 4cc5e81f705..47cf4f7a626 100644 --- a/apps/sim/connectors/confluence/confluence.ts +++ b/apps/sim/connectors/confluence/confluence.ts @@ -15,6 +15,18 @@ export function escapeCql(value: string): string { return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"') } +/** + * Keeps only content that is still current in Confluence. The v2 + * `/spaces/{id}/pages` endpoint includes `archived` pages by default and CQL has + * no status filter, so without this guard archived pages stay in every listing, + * keep getting upserted, and never fall out via deletion reconciliation (which + * removes only documents absent from the listing). Items with no status field + * are kept — only an explicit non-current status excludes a result. + */ +export function isCurrentContent(item: Record): boolean { + return item.status == null || item.status === 'current' +} + /** * Builds a CQL clause restricting content to the given space keys. * Single key uses `space = "X"`; multiple keys use `space in ("X","Y")`. @@ -272,7 +284,7 @@ export const confluenceConnector: ConnectorConfig = { } } - if (!page) return null + if (!page || !isCurrentContent(page)) return null const body = page.body as Record | undefined const view = body?.view as Record | undefined const rawContent = (view?.value as string) || '' @@ -381,6 +393,12 @@ async function listDocumentsV2( ): Promise { const queryParams = new URLSearchParams() queryParams.append('limit', '250') + /** + * Restrict to current content: the pages endpoint defaults to + * `current,archived`, so archived pages would otherwise stay in the listing + * forever and never be purged by deletion reconciliation. + */ + queryParams.append('status', 'current') if (cursor) { queryParams.append('cursor', cursor) } @@ -410,13 +428,15 @@ async function listDocumentsV2( const data = await response.json() const results = data.results || [] - const documents: ExternalDocument[] = results.map((page: Record) => { - const links = page._links as Record | undefined - return pageToStub(page, { - spaceId: page.spaceId, - sourceUrl: links?.webui ? `https://${domain}/wiki${links.webui}` : undefined, + const documents: ExternalDocument[] = (results as Record[]) + .filter(isCurrentContent) + .map((page) => { + const links = page._links as Record | undefined + return pageToStub(page, { + spaceId: page.spaceId, + sourceUrl: links?.webui ? `https://${domain}/wiki${links.webui}` : undefined, + }) }) - }) let nextCursor: string | undefined const nextLink = (data._links as Record)?.next @@ -594,9 +614,9 @@ async function listDocumentsViaCql( const data = await response.json() const results = data.results || [] - const documents: ExternalDocument[] = results.map((item: Record) => - cqlResultToStub(item, domain) - ) + const documents: ExternalDocument[] = (results as Record[]) + .filter(isCurrentContent) + .map((item) => cqlResultToStub(item, domain)) const totalFetched = ((syncContext?.totalDocsFetched as number) ?? 0) + documents.length if (syncContext) syncContext.totalDocsFetched = totalFetched From 2f909fb74a3d10249a698b416707c47a925457c6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 21:51:21 -0700 Subject: [PATCH 2/4] fix(connectors): purge archived/deleted source items across seven more KB connectors The sync engine only purges a knowledge-base document when its source item is absent from a full-sync listing, so any connector that keeps listing archived/trashed/canceled items never drops them. An audit of all 51 connectors found seven with this bug: - asana: list only non-archived projects (the API returns both when `archived` is omitted), so tasks under archived projects stop being re-listed - google-sheets: skip a spreadsheet Drive reports as trashed, which stays readable by id for 30 days before the Sheets call starts 404ing - incidentio: exclude canceled incidents by default (cancelling is incident.io's documented stand-in for deletion), with an explicit opt-in to sync them - outlook: exclude Deleted Items from the all-mail listing, which Graph otherwise includes - servicenow: drop retired knowledge articles, which the Table API returns with no implicit state filter - webflow: drop archived CMS items, which the staged items endpoint always returns and offers no way to filter - youtube: drop playlist entries whose video was deleted or made private, which the API keeps returning as placeholder items Every exclusion keys off an explicit non-current signal and fails open on a missing field or a failed metadata read, since wrongly excluding a live item would hard-delete it. Explicit user filter selections are still honoured verbatim; the new defaults apply only when nothing is configured. Also flag truncated listings as capped in asana, outlook, and servicenow. All three silently cut a listing short at their configured item cap without setting `syncContext.listingCapped`, so reconciliation read the untraversed tail as deleted at the source and hard-deleted it. --- apps/sim/connectors/asana/asana.test.ts | 442 +++++++++++++++++ apps/sim/connectors/asana/asana.ts | 161 ++++++- .../google-sheets/google-sheets.test.ts | 244 ++++++++++ .../connectors/google-sheets/google-sheets.ts | 132 +++++- .../connectors/incidentio/incidentio.test.ts | 306 ++++++++++++ apps/sim/connectors/incidentio/incidentio.ts | 57 ++- apps/sim/connectors/incidentio/meta.ts | 5 +- apps/sim/connectors/outlook/outlook.test.ts | 448 ++++++++++++++++++ apps/sim/connectors/outlook/outlook.ts | 299 +++++++++++- .../connectors/servicenow/servicenow.test.ts | 300 ++++++++++++ apps/sim/connectors/servicenow/servicenow.ts | 96 +++- apps/sim/connectors/webflow/webflow.test.ts | 49 ++ apps/sim/connectors/webflow/webflow.ts | 34 +- apps/sim/connectors/youtube/youtube.test.ts | 426 +++++++++++++++++ apps/sim/connectors/youtube/youtube.ts | 233 +++++++-- 15 files changed, 3153 insertions(+), 79 deletions(-) create mode 100644 apps/sim/connectors/asana/asana.test.ts create mode 100644 apps/sim/connectors/google-sheets/google-sheets.test.ts create mode 100644 apps/sim/connectors/incidentio/incidentio.test.ts create mode 100644 apps/sim/connectors/outlook/outlook.test.ts create mode 100644 apps/sim/connectors/servicenow/servicenow.test.ts create mode 100644 apps/sim/connectors/webflow/webflow.test.ts create mode 100644 apps/sim/connectors/youtube/youtube.test.ts diff --git a/apps/sim/connectors/asana/asana.test.ts b/apps/sim/connectors/asana/asana.test.ts new file mode 100644 index 00000000000..6e1ffefda48 --- /dev/null +++ b/apps/sim/connectors/asana/asana.test.ts @@ -0,0 +1,442 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + asanaConnector, + buildProjectsPath, + decideTaskCap, + isActiveProject, + isTaskUnderActiveProject, +} from '@/connectors/asana/asana' + +describe('buildProjectsPath', () => { + it.concurrent('always filters out archived projects', () => { + expect(buildProjectsPath('123')).toContain('archived=false') + }) + + it.concurrent('scopes the listing to the workspace with a page size', () => { + const path = buildProjectsPath('123') + expect(path.startsWith('/projects?')).toBe(true) + expect(path).toContain('workspace=123') + expect(path).toContain('limit=100') + }) + + it.concurrent('requests the archived field so it can be re-checked client side', () => { + expect(buildProjectsPath('123')).toContain('opt_fields=gid,name,archived') + }) + + it.concurrent('omits the offset param on the first page', () => { + expect(buildProjectsPath('123')).not.toContain('offset=') + }) + + it.concurrent('appends the offset token when paginating', () => { + expect(buildProjectsPath('123', 'abc:def')).toContain('offset=abc%3Adef') + }) + + it.concurrent('keeps the archived filter on paginated requests', () => { + expect(buildProjectsPath('123', 'abc')).toContain('archived=false') + }) +}) + +describe('isActiveProject', () => { + it.concurrent('keeps projects explicitly marked as not archived', () => { + expect(isActiveProject({ gid: '1', name: 'Roadmap', archived: false })).toBe(true) + }) + + it.concurrent('keeps projects with no archived field', () => { + expect(isActiveProject({ gid: '1', name: 'Roadmap' })).toBe(true) + }) + + it.concurrent('excludes explicitly archived projects', () => { + expect(isActiveProject({ gid: '1', name: 'Roadmap', archived: true })).toBe(false) + }) + + it.concurrent('keeps projects whose archived flag is a non-boolean truthy value', () => { + expect( + isActiveProject({ gid: '1', name: 'Roadmap', archived: 'true' } as unknown as { + gid: string + name: string + archived?: boolean + }) + ).toBe(true) + }) +}) + +const baseTask = { gid: 't1', name: 'Task', completed: false } + +describe('isTaskUnderActiveProject', () => { + it.concurrent('keeps a task with no projects field', () => { + expect(isTaskUnderActiveProject(baseTask)).toBe(true) + }) + + it.concurrent('keeps a task with an empty projects array', () => { + expect(isTaskUnderActiveProject({ ...baseTask, projects: [] })).toBe(true) + }) + + it.concurrent('keeps a task in at least one active project', () => { + expect( + isTaskUnderActiveProject({ + ...baseTask, + projects: [ + { gid: 'p1', name: 'Old', archived: true }, + { gid: 'p2', name: 'Live', archived: false }, + ], + }) + ).toBe(true) + }) + + it.concurrent('excludes a task whose every project is archived', () => { + expect( + isTaskUnderActiveProject({ + ...baseTask, + projects: [ + { gid: 'p1', name: 'Old', archived: true }, + { gid: 'p2', name: 'Older', archived: true }, + ], + }) + ).toBe(false) + }) + + it.concurrent('keeps a task when one project omits the archived field', () => { + expect( + isTaskUnderActiveProject({ + ...baseTask, + projects: [ + { gid: 'p1', name: 'Old', archived: true }, + { gid: 'p2', name: 'Unknown' }, + ], + }) + ).toBe(true) + }) + + it.concurrent('keeps a task when archived is a non-boolean truthy value', () => { + expect( + isTaskUnderActiveProject({ + ...baseTask, + projects: [{ gid: 'p1', name: 'Old', archived: 'true' }], + } as unknown as typeof baseTask) + ).toBe(true) + }) +}) + +describe('decideTaskCap', () => { + it.concurrent('keeps everything and reports no cap when maxTasks is unset', () => { + expect(decideTaskCap(0, 0, 50, true)).toEqual({ + keepCount: 50, + hitLimit: false, + truncated: false, + }) + }) + + it.concurrent('reports truncation when the page is sliced to the cap', () => { + expect(decideTaskCap(10, 4, 20, false)).toEqual({ + keepCount: 6, + hitLimit: true, + truncated: true, + }) + }) + + it.concurrent('reports truncation when the cap stops unread pages', () => { + expect(decideTaskCap(10, 0, 10, true)).toEqual({ + keepCount: 10, + hitLimit: true, + truncated: true, + }) + }) + + it.concurrent('does not report truncation when the cap coincides with the last page', () => { + expect(decideTaskCap(10, 0, 10, false)).toEqual({ + keepCount: 10, + hitLimit: true, + truncated: false, + }) + }) + + it.concurrent('does not report truncation below the cap', () => { + expect(decideTaskCap(10, 2, 3, false)).toEqual({ + keepCount: 3, + hitLimit: false, + truncated: false, + }) + }) + + it.concurrent('never returns a negative keep count when already past the cap', () => { + expect(decideTaskCap(10, 12, 5, true)).toEqual({ + keepCount: 0, + hitLimit: true, + truncated: true, + }) + }) +}) + +/** + * Minimal JSON response stub for the mocked global fetch. + */ +function jsonResponse(body: unknown): Response { + return { + ok: true, + status: 200, + statusText: 'OK', + headers: new Headers(), + json: async () => body, + text: async () => JSON.stringify(body), + } as unknown as Response +} + +function errorResponse(status: number): Response { + return { + ok: false, + status, + statusText: 'Error', + headers: new Headers(), + json: async () => ({}), + text: async () => 'boom', + } as unknown as Response +} + +const mockFetch = vi.fn<(input: string, init?: RequestInit) => Promise>() + +describe('asanaConnector.listDocuments', () => { + beforeEach(() => { + mockFetch.mockReset() + vi.stubGlobal('fetch', mockFetch) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + const requestedUrls = () => mockFetch.mock.calls.map(([url]) => url) + + it('requests the archived filter and drops archived projects from the listing', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + return jsonResponse({ + data: [ + { gid: 'p1', name: 'Live', archived: false }, + { gid: 'p2', name: 'Old', archived: true }, + ], + next_page: null, + }) + } + return jsonResponse({ data: [], next_page: null }) + }) + + const syncContext: Record = {} + await asanaConnector.listDocuments('token', { workspace: 'w1' }, undefined, syncContext) + + expect(requestedUrls()[0]).toContain('archived=false') + expect(syncContext.projectGids).toEqual(['p1']) + expect(requestedUrls().some((url) => url.includes('project=p2'))).toBe(false) + }) + + it('keeps paginating projects when an entire page filters to empty', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + if (url.includes('offset=page2')) { + return jsonResponse({ + data: [{ gid: 'p9', name: 'Live', archived: false }], + next_page: null, + }) + } + return jsonResponse({ + data: [ + { gid: 'p1', name: 'Old', archived: true }, + { gid: 'p2', name: 'Older', archived: true }, + ], + next_page: { offset: 'page2' }, + }) + } + return jsonResponse({ data: [], next_page: null }) + }) + + const syncContext: Record = {} + await asanaConnector.listDocuments('token', { workspace: 'w1' }, undefined, syncContext) + + expect(syncContext.projectGids).toEqual(['p9']) + expect(requestedUrls().filter((url) => url.includes('/projects')).length).toBe(2) + }) + + it('flags listingCapped when maxTasks truncates the listing', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + return jsonResponse({ data: [{ gid: 'p1', name: 'Live' }], next_page: null }) + } + return jsonResponse({ + data: [ + { gid: 't1', name: 'One', completed: false }, + { gid: 't2', name: 'Two', completed: false }, + { gid: 't3', name: 'Three', completed: false }, + ], + next_page: { offset: 'next', uri: 'x' }, + }) + }) + + const syncContext: Record = {} + const result = await asanaConnector.listDocuments( + 'token', + { workspace: 'w1', maxTasks: '2' }, + undefined, + syncContext + ) + + expect(result.documents).toHaveLength(2) + expect(result.hasMore).toBe(false) + expect(result.nextCursor).toBeUndefined() + expect(syncContext.listingCapped).toBe(true) + }) + + it('flags listingCapped when the cap lands exactly on a page with more pages left', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + return jsonResponse({ data: [{ gid: 'p1', name: 'Live' }], next_page: null }) + } + return jsonResponse({ + data: [ + { gid: 't1', name: 'One', completed: false }, + { gid: 't2', name: 'Two', completed: false }, + ], + next_page: { offset: 'next', uri: 'x' }, + }) + }) + + const syncContext: Record = {} + await asanaConnector.listDocuments( + 'token', + { workspace: 'w1', maxTasks: '2' }, + undefined, + syncContext + ) + + expect(syncContext.listingCapped).toBe(true) + }) + + it('leaves listingCapped unset when the source is exhausted within the cap', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + return jsonResponse({ data: [{ gid: 'p1', name: 'Live' }], next_page: null }) + } + return jsonResponse({ + data: [{ gid: 't1', name: 'One', completed: false }], + next_page: null, + }) + }) + + const syncContext: Record = {} + const result = await asanaConnector.listDocuments( + 'token', + { workspace: 'w1', maxTasks: '50' }, + undefined, + syncContext + ) + + expect(result.documents).toHaveLength(1) + expect(syncContext.listingCapped).toBeUndefined() + }) + + it('leaves listingCapped unset when no cap is configured', async () => { + mockFetch.mockImplementation(async (url) => { + if (url.includes('/projects')) { + return jsonResponse({ data: [{ gid: 'p1', name: 'Live' }], next_page: null }) + } + return jsonResponse({ + data: [{ gid: 't1', name: 'One', completed: false }], + next_page: null, + }) + }) + + const syncContext: Record = {} + await asanaConnector.listDocuments('token', { workspace: 'w1' }, undefined, syncContext) + + expect(syncContext.listingCapped).toBeUndefined() + }) + + it('keeps syncing an explicitly pinned project without listing workspace projects', async () => { + mockFetch.mockImplementation(async () => + jsonResponse({ data: [{ gid: 't1', name: 'One', completed: false }], next_page: null }) + ) + + await asanaConnector.listDocuments('token', { workspace: 'w1', project: 'p7' }, undefined, {}) + + expect(requestedUrls().some((url) => url.includes('/projects?'))).toBe(false) + expect(requestedUrls()[0]).toContain('project=p7') + }) +}) + +describe('asanaConnector.getDocument', () => { + beforeEach(() => { + mockFetch.mockReset() + vi.stubGlobal('fetch', mockFetch) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('requests the parent projects so the archived check can run', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ data: { gid: 't1', name: 'One', completed: false } }) + ) + + await asanaConnector.getDocument('token', {}, 't1') + + expect(mockFetch.mock.calls[0][0]).toContain('projects.archived') + }) + + it('returns null for a task whose every project is archived', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + data: { + gid: 't1', + name: 'One', + completed: false, + projects: [{ gid: 'p1', name: 'Old', archived: true }], + }, + }) + ) + + expect(await asanaConnector.getDocument('token', {}, 't1')).toBeNull() + }) + + it('returns the task when at least one parent project is still active', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + data: { + gid: 't1', + name: 'One', + completed: false, + projects: [ + { gid: 'p1', name: 'Old', archived: true }, + { gid: 'p2', name: 'Live', archived: false }, + ], + }, + }) + ) + + const doc = await asanaConnector.getDocument('token', {}, 't1') + expect(doc?.externalId).toBe('t1') + }) + + it('fails open and returns the task when the archived flag is missing', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + data: { + gid: 't1', + name: 'One', + completed: false, + projects: [{ gid: 'p1', name: 'Unknown' }], + }, + }) + ) + + const doc = await asanaConnector.getDocument('token', {}, 't1') + expect(doc?.externalId).toBe('t1') + }) + + it('returns null when the task fetch fails', async () => { + mockFetch.mockResolvedValue(errorResponse(404)) + + expect(await asanaConnector.getDocument('token', {}, 't1')).toBeNull() + }) +}) diff --git a/apps/sim/connectors/asana/asana.ts b/apps/sim/connectors/asana/asana.ts index 63b6f84c490..093cd2da8d8 100644 --- a/apps/sim/connectors/asana/asana.ts +++ b/apps/sim/connectors/asana/asana.ts @@ -23,7 +23,7 @@ interface AsanaPageResponse { /** * Minimal Asana task shape used by this connector. */ -interface AsanaTask { +export interface AsanaTask { gid: string name: string notes?: string @@ -33,6 +33,7 @@ interface AsanaTask { assignee?: { name: string } tags?: { name: string }[] permalink_url?: string + projects?: AsanaProject[] } /** @@ -46,9 +47,125 @@ interface AsanaWorkspace { /** * Asana project shape. */ -interface AsanaProject { +export interface AsanaProject { gid: string name: string + archived?: boolean +} + +/** + * Optional fields requested when enumerating workspace projects. `archived` is + * not returned by default and is needed to defensively re-check the server-side + * `archived=false` filter. + */ +const PROJECT_OPT_FIELDS = 'gid,name,archived' + +/** + * Optional fields requested when re-fetching a single task. Adds the task's + * parent projects (with their archived flag) on top of the listing fields so + * `isTaskUnderActiveProject` can run on the rehydrate path. `opt_fields` + * supports dot paths, so `projects.archived` expands the compact project stubs + * with the field the listing filter relies on. + */ +const TASK_DETAIL_OPT_FIELDS = `${TASK_OPT_FIELDS},projects.archived` + +/** + * Builds the workspace projects listing path. + * + * `GET /projects` only filters on `archived` when the parameter is supplied — + * omitting it returns archived AND active projects. Archived projects are + * hidden from view in Asana, yet their tasks keep appearing in the connector's + * full listing, so those tasks would never fall out via deletion reconciliation + * (which hard-deletes only stored documents absent from the full listing) and + * would linger in the knowledge base forever. + * + * `opt_fields` is appended raw rather than through `URLSearchParams` so its + * separators stay literal commas, matching every other task call in this + * connector and Asana's own documented examples. Only the caller-supplied + * values (workspace gid, pagination offset) go through `URLSearchParams`, which + * is where escaping actually matters. + */ +export function buildProjectsPath(workspaceGid: string, offset?: string): string { + const params = new URLSearchParams({ + workspace: workspaceGid, + archived: 'false', + limit: '100', + }) + if (offset) params.append('offset', offset) + return `/projects?${params.toString()}&opt_fields=${PROJECT_OPT_FIELDS}` +} + +/** + * Keeps only projects that are still active. Asana regressed the server-side + * `archived=false` filter once before (fixed 2024-11-06), so results are + * re-checked client side. Fails open: a project is dropped only on an explicit + * `archived === true`, never on a missing or non-boolean field. + */ +export function isActiveProject(project: AsanaProject): boolean { + return project.archived !== true +} + +/** + * Mirrors `isActiveProject` on the single-task rehydrate path so a task whose + * every parent project is archived cannot be resurrected after the listing + * dropped it. Fails open in every ambiguous case: a task with no `projects` + * field (the field is optional and only returned when requested), an empty + * array, or any entry whose `archived` is missing or non-boolean is kept. A + * task that still sits in at least one active project is kept, matching the + * listing, which reaches it through that active project. + */ +export function isTaskUnderActiveProject(task: AsanaTask): boolean { + const projects = task.projects + if (!Array.isArray(projects) || projects.length === 0) return true + return !projects.every((project) => project?.archived === true) +} + +/** + * Outcome of applying the `maxTasks` cap to one listing page. + */ +export interface TaskCapDecision { + /** How many of this page's documents to keep. */ + keepCount: number + /** True once the cap is reached, so pagination must stop. */ + hitLimit: boolean + /** + * True when the cap made the listing knowingly incomplete — either documents + * on this page were dropped, or pages beyond the cap were left unread. The + * caller flags `syncContext.listingCapped` so the sync engine refuses to + * reconcile deletions against a partial listing and hard-delete everything + * past the cap. + */ + truncated: boolean +} + +/** + * Decides how the `maxTasks` cap applies to a listing page. + * + * `pageDocumentCount` and `previouslyFetched` are pre-filter counts of returned + * tasks — the cap is never derived from post-filter array lengths. Reaching the + * cap exactly as the source runs out (`morePagesAvailable === false` and nothing + * dropped) leaves the listing complete, so it is not reported as truncated. + */ +export function decideTaskCap( + maxTasks: number, + previouslyFetched: number, + pageDocumentCount: number, + morePagesAvailable: boolean +): TaskCapDecision { + if (!(maxTasks > 0)) { + return { keepCount: pageDocumentCount, hitLimit: false, truncated: false } + } + + const remaining = Math.max(maxTasks - previouslyFetched, 0) + const keepCount = Math.min(pageDocumentCount, remaining) + const droppedFromPage = keepCount < pageDocumentCount + const hitLimit = previouslyFetched + keepCount >= maxTasks + + return { + keepCount, + hitLimit, + truncated: droppedFromPage || (hitLimit && morePagesAvailable), + } } /** @@ -106,7 +223,18 @@ function buildTaskContent(task: AsanaTask): string { } /** - * Fetches all project GIDs in a workspace, used when no specific project is configured. + * Fetches all active project GIDs in a workspace, used when no specific project + * is configured. Archived projects are excluded so their tasks drop out of the + * full listing and get purged by deletion reconciliation. A project the user + * pinned explicitly via the `project` config field keeps syncing even once + * archived — that is a deliberate user choice, not a stale listing. + * + * This is a one-time destructive change on rollout: the first full sync after + * deploy stops listing tasks that live only under archived projects, so + * deletion reconciliation removes their stored documents. That is the intended + * correction — those documents track content the workspace already archived — + * but it must be called out in the release notes, since re-indexing them + * requires unarchiving the project in Asana. */ async function listWorkspaceProjects( accessToken: string, @@ -117,12 +245,11 @@ async function listWorkspaceProjects( // eslint-disable-next-line no-constant-condition while (true) { - const offsetParam = offset ? `&offset=${offset}` : '' const result = await asanaGet<{ data: AsanaProject[]; next_page: { offset: string } | null }>( accessToken, - `/projects?workspace=${workspaceGid}&limit=100${offsetParam}` + buildProjectsPath(workspaceGid, offset) ) - projects.push(...result.data) + projects.push(...result.data.filter(isActiveProject)) if (!result.next_page) break offset = result.next_page.offset } @@ -235,18 +362,13 @@ export const asanaConnector: ConnectorConfig = { } const previouslyFetched = (syncContext?.totalDocsFetched as number) ?? 0 - if (maxTasks > 0) { - const remaining = maxTasks - previouslyFetched - if (documents.length > remaining) { - documents.splice(remaining) - } - } + const cap = decideTaskCap(maxTasks, previouslyFetched, documents.length, hasMore) + if (cap.keepCount < documents.length) documents.splice(cap.keepCount) - const totalFetched = previouslyFetched + documents.length - if (syncContext) syncContext.totalDocsFetched = totalFetched - const hitLimit = maxTasks > 0 && totalFetched >= maxTasks + if (syncContext) syncContext.totalDocsFetched = previouslyFetched + documents.length + if (cap.truncated && syncContext) syncContext.listingCapped = true - if (hitLimit) { + if (cap.hitLimit) { hasMore = false nextCursor = undefined } @@ -266,12 +388,17 @@ export const asanaConnector: ConnectorConfig = { try { const result = await asanaGet<{ data: AsanaTask }>( accessToken, - `/tasks/${externalId}?opt_fields=${TASK_OPT_FIELDS}` + `/tasks/${externalId}?opt_fields=${TASK_DETAIL_OPT_FIELDS}` ) const task = result.data if (!task) return null + if (!isTaskUnderActiveProject(task)) { + logger.info('Skipping Asana task whose projects are all archived', { externalId }) + return null + } + const content = buildTaskContent(task) const tagNames = task.tags?.map((t) => t.name).filter(Boolean) || [] diff --git a/apps/sim/connectors/google-sheets/google-sheets.test.ts b/apps/sim/connectors/google-sheets/google-sheets.test.ts new file mode 100644 index 00000000000..66eb84aa434 --- /dev/null +++ b/apps/sim/connectors/google-sheets/google-sheets.test.ts @@ -0,0 +1,244 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/components/icons', () => ({ + GoogleSheetsIcon: () => null, +})) + +import { + type DriveFileMetadata, + googleSheetsConnector, + isTrashedDriveFile, + parseDriveFileMetadata, +} from '@/connectors/google-sheets/google-sheets' + +describe('isTrashedDriveFile', () => { + it.concurrent('excludes an explicitly trashed file', () => { + expect(isTrashedDriveFile({ trashed: true })).toBe(true) + }) + + it.concurrent('keeps a file explicitly marked not trashed', () => { + expect(isTrashedDriveFile({ trashed: false })).toBe(false) + }) + + it.concurrent('keeps a file when the trashed field is absent', () => { + expect(isTrashedDriveFile({ modifiedTime: '2026-07-22T10:00:00.000Z' })).toBe(false) + }) + + it.concurrent('keeps a file when the Drive read failed and returned nothing', () => { + expect(isTrashedDriveFile({})).toBe(false) + }) +}) + +describe('parseDriveFileMetadata', () => { + it.concurrent('extracts modifiedTime and trashed', () => { + expect( + parseDriveFileMetadata({ modifiedTime: '2026-07-22T10:00:00.000Z', trashed: true }) + ).toEqual({ modifiedTime: '2026-07-22T10:00:00.000Z', trashed: true }) + }) + + it.concurrent('omits fields with the wrong type instead of coercing them', () => { + const parsed: DriveFileMetadata = parseDriveFileMetadata({ modifiedTime: 123, trashed: 'true' }) + expect(parsed).toEqual({}) + expect(isTrashedDriveFile(parsed)).toBe(false) + }) + + it.concurrent('ignores unrelated fields', () => { + expect(parseDriveFileMetadata({ id: 'abc', name: 'Sheet' })).toEqual({}) + }) + + it.concurrent('returns an empty object for non-object bodies', () => { + expect(parseDriveFileMetadata(null)).toEqual({}) + expect(parseDriveFileMetadata(undefined)).toEqual({}) + expect(parseDriveFileMetadata('trashed')).toEqual({}) + }) + + it.concurrent('preserves trashed: false', () => { + expect(parseDriveFileMetadata({ trashed: false })).toEqual({ trashed: false }) + }) +}) + +const SPREADSHEET_ID = 'sheet-abc' +const ACCESS_TOKEN = 'token-123' +const SOURCE_CONFIG = { spreadsheetId: SPREADSHEET_ID } + +const SPREADSHEET_METADATA = { + spreadsheetId: SPREADSHEET_ID, + properties: { title: 'Quarterly Plan' }, + sheets: [ + { properties: { sheetId: 0, title: 'Revenue', index: 0 } }, + { properties: { sheetId: 7, title: 'Costs', index: 1 } }, + ], +} + +/** Drive response bodies keyed by the scenario each test exercises. */ +interface FetchStubResponses { + drive: { status: number; body: unknown } + values?: unknown +} + +/** + * Routes Sheets metadata, Sheets values, and Drive `files.get` calls to canned + * responses. Non-2xx statuses are restricted to codes `fetchWithRetry` treats as + * non-retryable so no test ever waits on a backoff sleep. + */ +function stubFetch(responses: FetchStubResponses) { + const fetchMock = vi.fn(async (input: string | URL | Request) => { + const url = typeof input === 'string' ? input : input.toString() + + if (url.startsWith('https://www.googleapis.com/drive/v3/files/')) { + return new Response(JSON.stringify(responses.drive.body), { + status: responses.drive.status, + }) + } + if (url.includes('/values/')) { + return new Response(JSON.stringify(responses.values ?? {}), { status: 200 }) + } + if (url.startsWith('https://sheets.googleapis.com/v4/spreadsheets/')) { + return new Response(JSON.stringify(SPREADSHEET_METADATA), { status: 200 }) + } + throw new Error(`Unexpected fetch to ${url}`) + }) + + vi.stubGlobal('fetch', fetchMock) + return fetchMock +} + +describe('googleSheetsConnector trashed handling', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + describe('listDocuments', () => { + it('returns an empty listing when the spreadsheet is trashed', async () => { + stubFetch({ + drive: { status: 200, body: { trashed: true, modifiedTime: '2026-07-01T00:00:00.000Z' } }, + }) + + const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result).toEqual({ documents: [], hasMore: false }) + }) + + it('lists every tab when the trashed field is absent', async () => { + stubFetch({ drive: { status: 200, body: { modifiedTime: '2026-07-01T00:00:00.000Z' } } }) + + const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result.documents.map((d) => d.externalId)).toEqual([ + `${SPREADSHEET_ID}__sheet__0`, + `${SPREADSHEET_ID}__sheet__7`, + ]) + expect(result.hasMore).toBe(false) + }) + + it('lists every tab when trashed is explicitly false', async () => { + stubFetch({ drive: { status: 200, body: { trashed: false } } }) + + const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result.documents).toHaveLength(2) + }) + + it('fails open and lists every tab when the Drive read fails', async () => { + stubFetch({ drive: { status: 500, body: { error: 'backend error' } } }) + + const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result.documents.map((d) => d.externalId)).toEqual([ + `${SPREADSHEET_ID}__sheet__0`, + `${SPREADSHEET_ID}__sheet__7`, + ]) + }) + + it('fails open when the Drive body is not an object', async () => { + stubFetch({ drive: { status: 200, body: 'trashed' } }) + + const result = await googleSheetsConnector.listDocuments(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result.documents).toHaveLength(2) + }) + }) + + describe('getDocument', () => { + const VALUES = { + values: [ + ['Region', 'Total'], + ['West', '10'], + ], + } + + it('returns null when the spreadsheet is trashed', async () => { + stubFetch({ drive: { status: 200, body: { trashed: true } }, values: VALUES }) + + const doc = await googleSheetsConnector.getDocument( + ACCESS_TOKEN, + SOURCE_CONFIG, + `${SPREADSHEET_ID}__sheet__0` + ) + + expect(doc).toBeNull() + }) + + it('returns the document when the trashed field is absent', async () => { + stubFetch({ + drive: { status: 200, body: { modifiedTime: '2026-07-01T00:00:00.000Z' } }, + values: VALUES, + }) + + const doc = await googleSheetsConnector.getDocument( + ACCESS_TOKEN, + SOURCE_CONFIG, + `${SPREADSHEET_ID}__sheet__0` + ) + + expect(doc?.externalId).toBe(`${SPREADSHEET_ID}__sheet__0`) + expect(doc?.contentDeferred).toBe(false) + }) + + it('fails open and returns the document when the Drive read fails', async () => { + stubFetch({ drive: { status: 500, body: { error: 'backend error' } }, values: VALUES }) + + const doc = await googleSheetsConnector.getDocument( + ACCESS_TOKEN, + SOURCE_CONFIG, + `${SPREADSHEET_ID}__sheet__0` + ) + + expect(doc?.externalId).toBe(`${SPREADSHEET_ID}__sheet__0`) + }) + }) + + describe('validateConfig', () => { + it('rejects a spreadsheet that is already in the Drive trash', async () => { + stubFetch({ drive: { status: 200, body: { trashed: true } } }) + + const result = await googleSheetsConnector.validateConfig(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result.valid).toBe(false) + expect(result.error).toContain('trash') + }) + + it('accepts a spreadsheet that is not trashed', async () => { + stubFetch({ drive: { status: 200, body: { trashed: false } } }) + + const result = await googleSheetsConnector.validateConfig(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result).toEqual({ valid: true }) + }) + + it('fails open and accepts the config when the Drive read fails', async () => { + stubFetch({ drive: { status: 500, body: { error: 'backend error' } } }) + + const result = await googleSheetsConnector.validateConfig(ACCESS_TOKEN, SOURCE_CONFIG) + + expect(result).toEqual({ valid: true }) + }) + }) +}) diff --git a/apps/sim/connectors/google-sheets/google-sheets.ts b/apps/sim/connectors/google-sheets/google-sheets.ts index be14fa1f2bd..a565bed2361 100644 --- a/apps/sim/connectors/google-sheets/google-sheets.ts +++ b/apps/sim/connectors/google-sheets/google-sheets.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' +import type { RetryOptions } from '@/lib/knowledge/documents/utils' import { fetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import { googleSheetsConnectorMeta } from '@/connectors/google-sheets/meta' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' @@ -105,34 +106,83 @@ async function fetchSpreadsheetMetadata( } /** - * Fetches the spreadsheet's modifiedTime from the Drive API. + * Drive `files.get` metadata for the backing spreadsheet file. + * + * `trashed` is the Drive v3 File boolean meaning "whether the file has been + * trashed, either explicitly or from a trashed parent folder". Both fields are + * optional here because a failed Drive read yields an empty object. */ -async function fetchSpreadsheetModifiedTime( +export interface DriveFileMetadata { + modifiedTime?: string + trashed?: boolean +} + +/** + * Reports whether the spreadsheet's Drive file is in the trash. + * + * Trashing a Drive file does not make it unreadable: Drive keeps trashed files + * accessible by ID for 30 days before permanent deletion ("other users can + * still access the file in the owner's trash until it's permanently deleted"), + * so `spreadsheets.get` keeps succeeding and every tab keeps appearing in the + * listing. Since KB deletion reconciliation only purges stored documents that + * are absent from a full listing, a trashed spreadsheet's tabs would otherwise + * live in the knowledge base forever — and once the 30 days elapse the Sheets + * call 404s, the listing throws, and reconciliation never runs at all. + * + * Fails open: only an explicit `trashed === true` counts. A missing field or a + * failed Drive read (which returns `{}`) is treated as not trashed, because a + * wrongful exclusion would hard-delete still-current documents. + */ +export function isTrashedDriveFile(metadata: DriveFileMetadata): boolean { + return metadata.trashed === true +} + +/** + * Narrows an untyped Drive `files.get` response body to the fields we consume. + */ +export function parseDriveFileMetadata(data: unknown): DriveFileMetadata { + if (typeof data !== 'object' || data === null) return {} + const record = data as Record + return { + ...(typeof record.modifiedTime === 'string' ? { modifiedTime: record.modifiedTime } : {}), + ...(typeof record.trashed === 'boolean' ? { trashed: record.trashed } : {}), + } +} + +/** + * Fetches the spreadsheet's `modifiedTime` and `trashed` state from the Drive API. + * Returns an empty object when the Drive read fails so callers fail open. + */ +async function fetchDriveFileMetadata( accessToken: string, - spreadsheetId: string -): Promise { + spreadsheetId: string, + retryOptions?: RetryOptions +): Promise { try { - const url = `${DRIVE_API_BASE}/${encodeURIComponent(spreadsheetId)}?fields=modifiedTime&supportsAllDrives=true` - const response = await fetchWithRetry(url, { - method: 'GET', - headers: { - Authorization: `Bearer ${accessToken}`, - Accept: 'application/json', + const url = `${DRIVE_API_BASE}/${encodeURIComponent(spreadsheetId)}?fields=modifiedTime,trashed&supportsAllDrives=true` + const response = await fetchWithRetry( + url, + { + method: 'GET', + headers: { + Authorization: `Bearer ${accessToken}`, + Accept: 'application/json', + }, }, - }) + retryOptions + ) if (!response.ok) { - logger.warn('Failed to fetch modifiedTime from Drive API', { status: response.status }) - return undefined + logger.warn('Failed to fetch file metadata from Drive API', { status: response.status }) + return {} } - const data = (await response.json()) as { modifiedTime?: string } - return data.modifiedTime + return parseDriveFileMetadata(await response.json()) } catch (error) { - logger.warn('Error fetching modifiedTime from Drive API', { + logger.warn('Error fetching file metadata from Drive API', { error: toError(error).message, }) - return undefined + return {} } } @@ -212,10 +262,21 @@ export const googleSheetsConnector: ConnectorConfig = { logger.info('Fetching spreadsheet metadata', { spreadsheetId }) - const [metadata, modifiedTime] = await Promise.all([ + const [metadata, driveMetadata] = await Promise.all([ fetchSpreadsheetMetadata(accessToken, spreadsheetId), - fetchSpreadsheetModifiedTime(accessToken, spreadsheetId), + fetchDriveFileMetadata(accessToken, spreadsheetId), ]) + + /** + * A trashed spreadsheet is no longer current content, so it must drop out of + * the full listing for deletion reconciliation to purge its stored tabs. + */ + if (isTrashedDriveFile(driveMetadata)) { + logger.info('Spreadsheet is in the Drive trash; listing no documents', { spreadsheetId }) + return { documents: [], hasMore: false } + } + + const modifiedTime = driveMetadata.modifiedTime const sheetFilter = (sourceConfig.sheetFilter as string) || 'all' let sheets = metadata.sheets.map((s) => s.properties) @@ -273,11 +334,11 @@ export const googleSheetsConnector: ConnectorConfig = { } let metadata: SpreadsheetMetadata - let modifiedTime: string | undefined + let driveMetadata: DriveFileMetadata try { - ;[metadata, modifiedTime] = await Promise.all([ + ;[metadata, driveMetadata] = await Promise.all([ fetchSpreadsheetMetadata(accessToken, spreadsheetId), - fetchSpreadsheetModifiedTime(accessToken, spreadsheetId), + fetchDriveFileMetadata(accessToken, spreadsheetId), ]) } catch (error) { const message = toError(error).message @@ -288,6 +349,12 @@ export const googleSheetsConnector: ConnectorConfig = { throw error } + /** Mirrors the listing: a trashed spreadsheet is still readable but no longer current. */ + if (isTrashedDriveFile(driveMetadata)) { + logger.info('Spreadsheet is in the Drive trash', { spreadsheetId }) + return null + } + const sheetEntry = metadata.sheets.find((s) => s.properties.sheetId === sheetId) if (!sheetEntry) { @@ -300,7 +367,7 @@ export const googleSheetsConnector: ConnectorConfig = { spreadsheetId, metadata.properties.title, sheetEntry.properties, - modifiedTime + driveMetadata.modifiedTime ) if (!doc) return null return { ...doc, contentDeferred: false } @@ -347,6 +414,25 @@ export const googleSheetsConnector: ConnectorConfig = { return { valid: false, error: `Failed to access spreadsheet: ${response.status}` } } + /** + * A trashed spreadsheet still reads back fine from the Sheets API, so without + * this check the connector would validate and then sync zero documents. Fails + * open exactly like the sync paths: only an explicit `trashed === true` blocks + * validation, and a failed Drive read leaves the config valid. + */ + const driveMetadata = await fetchDriveFileMetadata( + accessToken, + spreadsheetId, + VALIDATE_RETRY_OPTIONS + ) + if (isTrashedDriveFile(driveMetadata)) { + return { + valid: false, + error: + 'This spreadsheet is in the Google Drive trash. Restore it in Drive, then try again.', + } + } + return { valid: true } } catch (error) { const message = getErrorMessage(error, 'Failed to validate configuration') diff --git a/apps/sim/connectors/incidentio/incidentio.test.ts b/apps/sim/connectors/incidentio/incidentio.test.ts new file mode 100644 index 00000000000..a264663450a --- /dev/null +++ b/apps/sim/connectors/incidentio/incidentio.test.ts @@ -0,0 +1,306 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + buildStatusCategoryParams, + DEFAULT_EXCLUDED_STATUS_CATEGORIES, + incidentioConnector, + SYNC_ALL_STATUS_CATEGORIES, +} from '@/connectors/incidentio/incidentio' + +/** Every status category incident.io documents for a still-current incident. */ +const CURRENT_STATUS_CATEGORIES = ['triage', 'live', 'paused', 'learning', 'closed'] as const + +/** + * Triage outcomes that are NOT deletions: the incident still exists and stays + * readable, and a declined incident can be moved back to triage. + */ +const NON_DELETION_TRIAGE_CATEGORIES = ['declined', 'merged'] as const + +const ACCESS_TOKEN = 'test-token' + +const mockFetch = vi.fn() + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }) +} + +/** Resolves the URL of the nth (0-indexed) fetch the connector performed. */ +function requestUrl(callIndex = 0): URL { + const call = mockFetch.mock.calls[callIndex] + if (!call) throw new Error(`No fetch call at index ${callIndex}`) + return new URL(String(call[0])) +} + +function incidentFixture(overrides: Record = {}) { + return { + id: 'inc-1', + reference: 'INC-1', + name: 'Checkout is down', + summary: 'Payments failing', + permalink: 'https://app.incident.io/incidents/1', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-02T00:00:00Z', + incident_status: { name: 'Closed', category: 'closed' }, + ...overrides, + } +} + +describe('buildStatusCategoryParams', () => { + it('excludes only canceled when no category is selected', () => { + expect(buildStatusCategoryParams('')).toEqual([['status_category[not_in]', 'canceled']]) + }) + + it('treats a whitespace-only selection as unset', () => { + expect(buildStatusCategoryParams(' ')).toEqual([['status_category[not_in]', 'canceled']]) + }) + + it('never excludes a still-current category', () => { + const excluded = buildStatusCategoryParams('').map(([, value]) => value) + for (const category of CURRENT_STATUS_CATEGORIES) { + expect(excluded).not.toContain(category) + } + }) + + it('never excludes declined or merged, which are not deletions', () => { + const excluded = buildStatusCategoryParams('').map(([, value]) => value) + for (const category of NON_DELETION_TRIAGE_CATEGORIES) { + expect(excluded).not.toContain(category) + } + }) + + it('emits no filter at all for the explicit sync-everything option', () => { + expect(buildStatusCategoryParams(SYNC_ALL_STATUS_CATEGORIES)).toEqual([]) + }) + + it('honours an explicit category selection verbatim', () => { + expect(buildStatusCategoryParams('closed')).toEqual([['status_category[one_of]', 'closed']]) + }) + + it('honours an explicitly selected canceled category', () => { + expect(buildStatusCategoryParams('canceled')).toEqual([['status_category[one_of]', 'canceled']]) + }) + + it('trims an explicit selection', () => { + expect(buildStatusCategoryParams(' live ')).toEqual([['status_category[one_of]', 'live']]) + }) +}) + +describe('incidentioConnector.listDocuments', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('fetch', mockFetch) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('appends every excluded category as a separate not_in param', async () => { + mockFetch.mockResolvedValue(jsonResponse({ incidents: [incidentFixture()] })) + + await incidentioConnector.listDocuments(ACCESS_TOKEN, {}) + + const url = requestUrl() + expect(url.searchParams.getAll('status_category[not_in]')).toEqual([ + ...DEFAULT_EXCLUDED_STATUS_CATEGORIES, + ]) + expect(url.searchParams.has('status_category[one_of]')).toBe(false) + }) + + it('keeps the other listing params intact alongside the exclusion', async () => { + mockFetch.mockResolvedValue(jsonResponse({ incidents: [] })) + + await incidentioConnector.listDocuments( + ACCESS_TOKEN, + { mode: 'standard' }, + 'cursor-1', + undefined, + new Date('2026-01-01T00:00:00.000Z') + ) + + const url = requestUrl() + expect(url.searchParams.get('page_size')).toBe('100') + expect(url.searchParams.get('sort_by')).toBe('created_at_oldest_first') + expect(url.searchParams.get('after')).toBe('cursor-1') + expect(url.searchParams.get('updated_at[gte]')).toBe('2026-01-01T00:00:00.000Z') + expect(url.searchParams.get('mode[one_of]')).toBe('standard') + expect(url.searchParams.getAll('status_category[not_in]')).toEqual([ + ...DEFAULT_EXCLUDED_STATUS_CATEGORIES, + ]) + }) + + it('sends no status_category filter when the user explicitly selects all', async () => { + mockFetch.mockResolvedValue(jsonResponse({ incidents: [] })) + + await incidentioConnector.listDocuments(ACCESS_TOKEN, { + statusCategory: SYNC_ALL_STATUS_CATEGORIES, + }) + + const url = requestUrl() + expect(url.searchParams.has('status_category[not_in]')).toBe(false) + expect(url.searchParams.has('status_category[one_of]')).toBe(false) + }) + + it('sends one_of for an explicit category and drops the default exclusion', async () => { + mockFetch.mockResolvedValue(jsonResponse({ incidents: [] })) + + await incidentioConnector.listDocuments(ACCESS_TOKEN, { statusCategory: 'live' }) + + const url = requestUrl() + expect(url.searchParams.get('status_category[one_of]')).toBe('live') + expect(url.searchParams.has('status_category[not_in]')).toBe(false) + }) + + it('falls back to the default exclusion when statusCategory is not a string', async () => { + mockFetch.mockResolvedValue(jsonResponse({ incidents: [] })) + + await incidentioConnector.listDocuments(ACCESS_TOKEN, { statusCategory: 42 }) + + const url = requestUrl() + expect(url.searchParams.getAll('status_category[not_in]')).toEqual([ + ...DEFAULT_EXCLUDED_STATUS_CATEGORIES, + ]) + expect(url.searchParams.has('status_category[one_of]')).toBe(false) + }) + + it('does not client-side filter the listing, so no still-listed incident is dropped', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + incidents: [ + incidentFixture({ + id: 'inc-1', + incident_status: { name: 'Declined', category: 'declined' }, + }), + incidentFixture({ id: 'inc-2', incident_status: { name: 'Merged', category: 'merged' } }), + incidentFixture({ + id: 'inc-3', + incident_status: { name: 'Canceled', category: 'canceled' }, + }), + ], + }) + ) + + const result = await incidentioConnector.listDocuments(ACCESS_TOKEN, {}) + + expect(result.documents.map((doc) => doc.externalId)).toEqual(['inc-1', 'inc-2', 'inc-3']) + }) + + it('derives the next cursor from pagination_meta, not from the document count', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + incidents: [incidentFixture({ id: 'inc-1' }), incidentFixture({ id: 'inc-2' })], + pagination_meta: { after: 'cursor-2', page_size: 100 }, + }) + ) + + const result = await incidentioConnector.listDocuments(ACCESS_TOKEN, {}) + + expect(result.nextCursor).toBe('cursor-2') + expect(result.hasMore).toBe(true) + }) + + it('stops paginating once the max-incident cap is reached', async () => { + mockFetch.mockResolvedValue( + jsonResponse({ + incidents: [incidentFixture({ id: 'inc-1' }), incidentFixture({ id: 'inc-2' })], + pagination_meta: { after: 'cursor-2' }, + }) + ) + + const syncContext: Record = {} + const result = await incidentioConnector.listDocuments( + ACCESS_TOKEN, + { maxIncidents: '1' }, + undefined, + syncContext + ) + + expect(result.documents).toHaveLength(1) + expect(result.hasMore).toBe(false) + expect(result.nextCursor).toBeUndefined() + expect(syncContext.listingCapped).toBe(true) + }) + + it('throws instead of reporting an empty listing when the API fails', async () => { + mockFetch.mockResolvedValue(jsonResponse({ error: 'nope' }, 500)) + + await expect(incidentioConnector.listDocuments(ACCESS_TOKEN, {})).rejects.toThrow( + 'Failed to list incident.io incidents: 500' + ) + }) +}) + +describe('incidentioConnector.getDocument', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('fetch', mockFetch) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('hydrates an incident with its status updates', async () => { + mockFetch + .mockResolvedValueOnce(jsonResponse({ incident: incidentFixture() })) + .mockResolvedValueOnce( + jsonResponse({ + incident_updates: [ + { + id: 'upd-1', + message: 'Mitigated', + created_at: '2026-01-01T01:00:00Z', + updater: { user: { name: 'Ada' } }, + }, + ], + }) + ) + + const doc = await incidentioConnector.getDocument(ACCESS_TOKEN, {}, 'inc-1') + + expect(doc?.externalId).toBe('inc-1') + expect(doc?.title).toBe('INC-1: Checkout is down') + expect(doc?.content).toContain('Mitigated') + expect(doc?.content).toContain('Ada') + expect(doc?.contentDeferred).toBe(false) + }) + + it('returns the incident even when the updates fetch fails', async () => { + mockFetch + .mockResolvedValueOnce(jsonResponse({ incident: incidentFixture() })) + .mockResolvedValueOnce(jsonResponse({ error: 'nope' }, 500)) + + const doc = await incidentioConnector.getDocument(ACCESS_TOKEN, {}, 'inc-1') + + expect(doc?.externalId).toBe('inc-1') + expect(doc?.content).toContain('Checkout is down') + }) + + it('returns null for a deleted incident', async () => { + mockFetch.mockResolvedValue(jsonResponse({}, 404)) + + await expect(incidentioConnector.getDocument(ACCESS_TOKEN, {}, 'inc-1')).resolves.toBeNull() + }) + + it('returns null instead of throwing when the API fails', async () => { + mockFetch.mockResolvedValue(jsonResponse({ error: 'nope' }, 500)) + + await expect(incidentioConnector.getDocument(ACCESS_TOKEN, {}, 'inc-1')).resolves.toBeNull() + }) + + it('returns null instead of throwing when fetch rejects', async () => { + mockFetch.mockRejectedValue(new Error('boom')) + + await expect(incidentioConnector.getDocument(ACCESS_TOKEN, {}, 'inc-1')).resolves.toBeNull() + }) + + it('returns null for an empty external id without calling the API', async () => { + await expect(incidentioConnector.getDocument(ACCESS_TOKEN, {}, '')).resolves.toBeNull() + expect(mockFetch).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/connectors/incidentio/incidentio.ts b/apps/sim/connectors/incidentio/incidentio.ts index b2b3c66f9a2..264343fb319 100644 --- a/apps/sim/connectors/incidentio/incidentio.ts +++ b/apps/sim/connectors/incidentio/incidentio.ts @@ -138,6 +138,59 @@ interface IncidentioUpdatesListResponse { pagination_meta?: IncidentioPaginationMeta } +/** + * Sentinel `statusCategory` value meaning "sync every category, excluding + * nothing". It is a UI-only value and is never sent to incident.io. + * + * This exists so the dropdown keeps an escape hatch: leaving the field untouched + * applies the implicit canceled exclusion, while explicitly choosing this option + * opts back into a truly unfiltered listing. + */ +export const SYNC_ALL_STATUS_CATEGORIES = 'all' + +/** + * Status categories excluded from the listing when the user has not chosen one. + * + * Only `canceled` qualifies. incident.io has no incident delete endpoint and its + * docs name cancelling as the sole removal mechanism: "we don't currently + * support deleting incidents ... if you want to 'hide' an incident, you can + * simply cancel it". + * + * `declined` and `merged` are deliberately NOT excluded. Both are ordinary + * triage outcomes for incidents that still exist and remain readable, and a + * declined incident can be moved back to triage later — excluding them would + * make the sync engine hard-delete live customer content. + */ +export const DEFAULT_EXCLUDED_STATUS_CATEGORIES = ['canceled'] as const + +/** + * Builds the `status_category` query params for a listing request. + * + * `GET /v2/incidents` applies no default `status_category` filter, so canceled + * incidents stay in the full listing forever. Because the sync engine purges + * stale KB documents only via deletion reconciliation — documents absent from a + * full listing are removed — a canceled incident would otherwise be re-upserted + * indefinitely with no way to ever drop it. + * + * Resolution order: + * - `all` → no `status_category` param at all, so every category syncs. + * - any other non-empty value → honoured verbatim as `status_category[one_of]`, + * including deliberately selecting `canceled`. + * - empty / whitespace / unset → the default exclusion above. + * + * Values are emitted as repeated `status_category[not_in]` params, the documented + * syntax for the list operand, so callers must `append` (never `set`) each pair. + */ +export function buildStatusCategoryParams(statusCategory: string): Array<[string, string]> { + const explicit = statusCategory.trim() + if (explicit === SYNC_ALL_STATUS_CATEGORIES) return [] + if (explicit) return [['status_category[one_of]', explicit]] + return DEFAULT_EXCLUDED_STATUS_CATEGORIES.map((category): [string, string] => [ + 'status_category[not_in]', + category, + ]) +} + /** * Builds the metadata-based content hash for an incident. * @@ -374,7 +427,9 @@ export const incidentioConnector: ConnectorConfig = { url.searchParams.set('sort_by', 'created_at_oldest_first') if (cursor) url.searchParams.set('after', cursor) if (lastSyncAt) url.searchParams.set('updated_at[gte]', lastSyncAt.toISOString()) - if (statusCategory) url.searchParams.set('status_category[one_of]', statusCategory) + for (const [key, value] of buildStatusCategoryParams(statusCategory)) { + url.searchParams.append(key, value) + } if (mode) url.searchParams.set('mode[one_of]', mode) logger.info('Listing incident.io incidents', { diff --git a/apps/sim/connectors/incidentio/meta.ts b/apps/sim/connectors/incidentio/meta.ts index 7a4eec0ac59..c1370f68044 100644 --- a/apps/sim/connectors/incidentio/meta.ts +++ b/apps/sim/connectors/incidentio/meta.ts @@ -24,7 +24,8 @@ export const incidentioConnectorMeta: ConnectorMeta = { required: false, mode: 'advanced', options: [ - { label: 'All', id: '' }, + { label: 'All except canceled (default)', id: '' }, + { label: 'All (including canceled)', id: 'all' }, { label: 'Live (active)', id: 'live' }, { label: 'Paused', id: 'paused' }, { label: 'Closed', id: 'closed' }, @@ -35,7 +36,7 @@ export const incidentioConnectorMeta: ConnectorMeta = { { label: 'Canceled', id: 'canceled' }, ], description: - 'Only sync incidents in this status category. Leave as All to sync every category.', + 'Only sync incidents in this status category. The default skips canceled incidents, which is how incident.io hides an incident since it has no delete. Choose "All (including canceled)" to sync every category.', }, { id: 'mode', diff --git a/apps/sim/connectors/outlook/outlook.test.ts b/apps/sim/connectors/outlook/outlook.test.ts new file mode 100644 index 00000000000..1c3f59e75a7 --- /dev/null +++ b/apps/sim/connectors/outlook/outlook.test.ts @@ -0,0 +1,448 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + DELETED_ITEMS_FOLDER, + isAllMailSync, + isCurrentMessage, + outlookConnector, + parseFolderCollection, + resolveFolder, +} from '@/connectors/outlook/outlook' + +const DELETED_ITEMS_ID = 'deleted-items-id' +const DELETED_SUBFOLDER_ID = 'deleted-subfolder-id' +const INBOX_ID = 'inbox-id' + +interface JsonResponseInit { + status?: number +} + +function jsonResponse(body: unknown, init: JsonResponseInit = {}): Response { + return new Response(JSON.stringify(body), { + status: init.status ?? 200, + headers: { 'Content-Type': 'application/json' }, + }) +} + +/** + * Minimal Graph message payload, defaulting to fields the connector requires + * to build a conversation stub. + */ +function message(overrides: Record): Record { + return { + id: 'msg-1', + conversationId: 'conv-1', + subject: 'Hello', + receivedDateTime: '2026-01-01T00:00:00Z', + inferenceClassification: 'focused', + webLink: 'https://outlook.office.com/mail/id/msg-1', + ...overrides, + } +} + +const fetchMock = vi.fn<(input: string | URL | Request, init?: RequestInit) => Promise>() + +beforeEach(() => { + fetchMock.mockReset() + vi.stubGlobal('fetch', fetchMock) +}) + +afterEach(() => { + vi.unstubAllGlobals() +}) + +/** + * Routes Graph requests by URL so tests only declare the responses they care + * about. Unmatched URLs fail loudly rather than silently returning empty data. + */ +function routeFetch(routes: Array<[RegExp, () => Response]>) { + fetchMock.mockImplementation(async (input) => { + const url = String(input) + for (const [pattern, respond] of routes) { + if (pattern.test(url)) return respond() + } + throw new Error(`Unexpected fetch: ${url}`) + }) +} + +const deletedItemsRoute: [RegExp, () => Response] = [ + /mailFolders\/deleteditems\?/, + () => jsonResponse({ id: DELETED_ITEMS_ID, childFolderCount: 1 }), +] + +const childFoldersRoute: [RegExp, () => Response] = [ + /childFolders/, + () => jsonResponse({ value: [{ id: DELETED_SUBFOLDER_ID, childFolderCount: 0 }] }), +] + +describe('resolveFolder', () => { + it('defaults to the inbox when unset', () => { + expect(resolveFolder({})).toBe('inbox') + }) + + it('defaults to the inbox for empty or non-string values', () => { + expect(resolveFolder({ folder: '' })).toBe('inbox') + expect(resolveFolder({ folder: ' ' })).toBe('inbox') + expect(resolveFolder({ folder: 42 })).toBe('inbox') + }) + + it('returns the trimmed folder', () => { + expect(resolveFolder({ folder: 'sentitems' })).toBe('sentitems') + expect(resolveFolder({ folder: ' archive ' })).toBe('archive') + expect(resolveFolder({ folder: 'all' })).toBe('all') + }) +}) + +describe('isAllMailSync', () => { + it('is true only for the mailbox-wide option', () => { + expect(isAllMailSync({ folder: 'all' })).toBe(true) + expect(isAllMailSync({ folder: ' all ' })).toBe(true) + }) + + it('is false for folder-scoped syncs', () => { + expect(isAllMailSync({})).toBe(false) + expect(isAllMailSync({ folder: 'inbox' })).toBe(false) + expect(isAllMailSync({ folder: 'deleteditems' })).toBe(false) + }) +}) + +describe('DELETED_ITEMS_FOLDER', () => { + it('excludes Deleted Items only, never Junk Email', () => { + expect(DELETED_ITEMS_FOLDER).toBe('deleteditems') + }) +}) + +describe('isCurrentMessage', () => { + const excluded: ReadonlySet = new Set([DELETED_ITEMS_ID, DELETED_SUBFOLDER_ID]) + + it('keeps messages in non-excluded folders', () => { + expect(isCurrentMessage({ parentFolderId: INBOX_ID }, excluded)).toBe(true) + }) + + it('excludes messages in Deleted Items and its subfolders', () => { + expect(isCurrentMessage({ parentFolderId: DELETED_ITEMS_ID }, excluded)).toBe(false) + expect(isCurrentMessage({ parentFolderId: DELETED_SUBFOLDER_ID }, excluded)).toBe(false) + }) + + it('fails open when parentFolderId is missing', () => { + expect(isCurrentMessage({}, excluded)).toBe(true) + expect(isCurrentMessage({ parentFolderId: undefined }, excluded)).toBe(true) + expect(isCurrentMessage({ parentFolderId: '' }, excluded)).toBe(true) + }) + + it('fails open when no exclusions could be resolved', () => { + const none: ReadonlySet = new Set() + expect(isCurrentMessage({ parentFolderId: DELETED_ITEMS_ID }, none)).toBe(true) + }) + + it('matches folder ids exactly, not by prefix', () => { + expect(isCurrentMessage({ parentFolderId: `${DELETED_ITEMS_ID}-2` }, excluded)).toBe(true) + }) +}) + +describe('parseFolderCollection', () => { + it('extracts folder ids and the continuation link', () => { + expect( + parseFolderCollection({ + value: [{ id: 'a', childFolderCount: 2 }, { id: 'b' }], + '@odata.nextLink': 'https://graph.microsoft.com/next', + }) + ).toEqual({ + folders: [ + { id: 'a', childFolderCount: 2 }, + { id: 'b', childFolderCount: 0 }, + ], + nextLink: 'https://graph.microsoft.com/next', + }) + }) + + it('yields no folders for malformed payloads', () => { + expect(parseFolderCollection(null).folders).toEqual([]) + expect(parseFolderCollection('nope').folders).toEqual([]) + expect(parseFolderCollection({ value: 'nope' }).folders).toEqual([]) + expect(parseFolderCollection({ value: [null, 42, { id: '' }, { id: 7 }] }).folders).toEqual([]) + expect(parseFolderCollection({ value: [], '@odata.nextLink': 5 }).nextLink).toBeUndefined() + }) +}) + +describe('listDocuments folder exclusion', () => { + it('drops conversations whose messages sit in Deleted Items or a subfolder', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm1', conversationId: 'live', parentFolderId: INBOX_ID }), + message({ id: 'm2', conversationId: 'trashed', parentFolderId: DELETED_ITEMS_ID }), + message({ id: 'm3', conversationId: 'nested', parentFolderId: DELETED_SUBFOLDER_ID }), + ], + }), + ], + ]) + + const syncContext: Record = {} + const result = await outlookConnector.listDocuments( + 'token', + { folder: 'all' }, + undefined, + syncContext + ) + + expect(result.documents.map((d) => d.externalId)).toEqual(['live']) + }) + + it('keeps every conversation when the Deleted Items lookup fails with a non-2xx', async () => { + routeFetch([ + [/mailFolders\/deleteditems\?/, () => jsonResponse({ error: 'forbidden' }, { status: 403 })], + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm1', conversationId: 'live', parentFolderId: INBOX_ID }), + message({ id: 'm2', conversationId: 'trashed', parentFolderId: DELETED_ITEMS_ID }), + ], + }), + ], + ]) + + const result = await outlookConnector.listDocuments('token', { folder: 'all' }, undefined, {}) + + expect(result.documents.map((d) => d.externalId).sort()).toEqual(['live', 'trashed']) + }) + + it('keeps every conversation when the Deleted Items lookup throws', async () => { + routeFetch([ + [ + /mailFolders\/deleteditems\?/, + () => { + throw new Error('graph exploded') + }, + ], + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm2', conversationId: 'trashed', parentFolderId: DELETED_ITEMS_ID }), + ], + }), + ], + ]) + + const result = await outlookConnector.listDocuments('token', { folder: 'all' }, undefined, {}) + + expect(result.documents.map((d) => d.externalId)).toEqual(['trashed']) + }) + + it('keeps Deleted Items messages when the subfolder walk fails partway', async () => { + routeFetch([ + deletedItemsRoute, + [/childFolders/, () => jsonResponse({ error: 'boom' }, { status: 403 })], + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm3', conversationId: 'nested', parentFolderId: DELETED_SUBFOLDER_ID }), + message({ id: 'm2', conversationId: 'trashed', parentFolderId: DELETED_ITEMS_ID }), + ], + }), + ], + ]) + + const result = await outlookConnector.listDocuments('token', { folder: 'all' }, undefined, {}) + + expect(result.documents.map((d) => d.externalId)).toEqual(['nested']) + }) + + it('never resolves folders for a folder-scoped sync', async () => { + routeFetch([ + [ + /mailFolders\/inbox\/messages\?/, + () => + jsonResponse({ + value: [message({ id: 'm1', conversationId: 'live', parentFolderId: INBOX_ID })], + }), + ], + ]) + + const result = await outlookConnector.listDocuments('token', { folder: 'inbox' }, undefined, {}) + + expect(result.documents.map((d) => d.externalId)).toEqual(['live']) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('reuses one folder walk across pages of the same sync run', async () => { + let page = 0 + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + [ + /\/me\/messages/, + () => { + page++ + return page === 1 + ? jsonResponse({ + value: [message({ id: 'm1', conversationId: 'live', parentFolderId: INBOX_ID })], + '@odata.nextLink': 'https://graph.microsoft.com/v1.0/me/messages?page=2', + }) + : jsonResponse({ + value: [message({ id: 'm4', conversationId: 'live2', parentFolderId: INBOX_ID })], + }) + }, + ], + ]) + + const syncContext: Record = {} + const first = await outlookConnector.listDocuments( + 'token', + { folder: 'all' }, + undefined, + syncContext + ) + expect(first.hasMore).toBe(true) + + const second = await outlookConnector.listDocuments( + 'token', + { folder: 'all' }, + first.nextCursor, + syncContext + ) + + expect(second.documents.map((d) => d.externalId).sort()).toEqual(['live', 'live2']) + const folderCalls = fetchMock.mock.calls.filter(([input]) => /mailFolders/.test(String(input))) + expect(folderCalls).toHaveLength(2) + }) + + it('paginates on the raw page length, not the post-exclusion count', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm2', conversationId: 'trashed', parentFolderId: DELETED_ITEMS_ID }), + ], + '@odata.nextLink': 'https://graph.microsoft.com/v1.0/me/messages?page=2', + }), + ], + ]) + + const syncContext: Record = {} + const result = await outlookConnector.listDocuments( + 'token', + { folder: 'all' }, + undefined, + syncContext + ) + + expect(result.hasMore).toBe(true) + expect(result.documents).toEqual([]) + expect(syncContext._totalMessagesFetched).toBe(1) + }) +}) + +describe('getDocument folder exclusion', () => { + const conversationRoute = (parentFolderIds: string[]): [RegExp, () => Response] => [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: parentFolderIds.map((parentFolderId, index) => + message({ + id: `msg-${index}`, + conversationId: 'conv-1', + parentFolderId, + receivedDateTime: `2026-01-0${index + 1}T00:00:00Z`, + body: { contentType: 'text', content: `body ${index}` }, + }) + ), + }), + ] + + it('excludes deleted messages from the content and contentHash', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + conversationRoute([INBOX_ID, DELETED_ITEMS_ID]), + ]) + + const doc = await outlookConnector.getDocument('token', { folder: 'all' }, 'conv-1', {}) + + expect(doc?.content).toContain('body 0') + expect(doc?.content).not.toContain('body 1') + expect(doc?.contentHash).toBe('outlook:conv-1:2026-01-01T00:00:00Z') + }) + + it('returns null when every message in the conversation is deleted', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + conversationRoute([DELETED_ITEMS_ID, DELETED_SUBFOLDER_ID]), + ]) + + expect(await outlookConnector.getDocument('token', { folder: 'all' }, 'conv-1', {})).toBeNull() + }) + + it('fails open and keeps every message when folder resolution fails', async () => { + routeFetch([ + [/mailFolders\/deleteditems\?/, () => jsonResponse({ error: 'nope' }, { status: 403 })], + conversationRoute([INBOX_ID, DELETED_ITEMS_ID]), + ]) + + const doc = await outlookConnector.getDocument('token', { folder: 'all' }, 'conv-1', {}) + + expect(doc?.content).toContain('body 0') + expect(doc?.content).toContain('body 1') + expect(doc?.contentHash).toBe('outlook:conv-1:2026-01-02T00:00:00Z') + }) + + it('does not resolve folders for a folder-scoped sync', async () => { + routeFetch([ + [ + /mailFolders\/inbox\/messages\?/, + () => + jsonResponse({ + value: [ + message({ + id: 'msg-0', + conversationId: 'conv-1', + parentFolderId: INBOX_ID, + body: { contentType: 'text', content: 'body 0' }, + }), + ], + }), + ], + ]) + + const doc = await outlookConnector.getDocument('token', { folder: 'inbox' }, 'conv-1', {}) + + expect(doc?.content).toContain('body 0') + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('shares the resolved folder set across concurrent getDocument calls', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + conversationRoute([INBOX_ID, DELETED_ITEMS_ID]), + ]) + + const syncContext: Record = {} + await Promise.all([ + outlookConnector.getDocument('token', { folder: 'all' }, 'conv-1', syncContext), + outlookConnector.getDocument('token', { folder: 'all' }, 'conv-2', syncContext), + outlookConnector.getDocument('token', { folder: 'all' }, 'conv-3', syncContext), + ]) + + const folderCalls = fetchMock.mock.calls.filter(([input]) => /mailFolders/.test(String(input))) + expect(folderCalls).toHaveLength(2) + }) +}) diff --git a/apps/sim/connectors/outlook/outlook.ts b/apps/sim/connectors/outlook/outlook.ts index ed9c29af4d4..c9123451511 100644 --- a/apps/sim/connectors/outlook/outlook.ts +++ b/apps/sim/connectors/outlook/outlook.ts @@ -65,7 +65,7 @@ interface OutlookRecipient { emailAddress?: OutlookEmailAddress } -interface OutlookMessage { +export interface OutlookMessage { id: string conversationId?: string subject?: string @@ -95,11 +95,260 @@ const WELL_KNOWN_FOLDERS: Record = { junkemail: 'junkemail', } +/** + * Graph well-known name of the folder that holds messages the mailbox owner + * has thrown away. Well-known names resolve regardless of mailbox locale. + * + * Outlook has no "deleted" flag on a message — deleting moves it into Deleted + * Items — so a folder-scoped listing drops deleted messages naturally and KB + * deletion reconciliation purges them. The mailbox-wide endpoint used for the + * "All Mail" option does not: Microsoft Graph documents `GET /me/messages` as + * returning "the messages in the signed-in user's mailbox (including the + * Deleted Items and Clutter folders)". Without this exclusion a deleted + * conversation stays in the full listing forever and is never purged from the + * knowledge base. + * + * Deliberately limited to Deleted Items. Junk Email is a Microsoft spam + * classifier decision on a message that still exists in the mailbox, not a + * user deletion — excluding it would drop already-indexed conversations out of + * the listing and make deletion reconciliation hard-delete live content. + */ +export const DELETED_ITEMS_FOLDER = 'deleteditems' + +/** + * Page size for the Deleted Items child-folder walk. + */ +const CHILD_FOLDER_PAGE_SIZE = 100 + +/** + * Upper bound on Graph requests spent walking the Deleted Items subtree, + * covering both the well-known folder lookup and every `childFolders` page. + * + * Residual gap: a mailbox whose Deleted Items subtree needs more requests than + * this budget leaves its deepest folders unresolved. Those folders are then not + * excluded, so messages deleted into them stay indexed until they are purged + * from the mailbox. That is the deliberate failure direction — under-excluding + * only leaves stale documents, while over-excluding would hard-delete live + * conversations from the knowledge base. + */ +const MAX_FOLDER_RESOLUTION_REQUESTS = 25 + +/** + * Key under which the resolved exclusion set is memoized on the per-sync-run + * `syncContext`. Scoping the cache to a sync run keeps it bound to one mailbox + * without ever holding an OAuth access token in a process-global structure. + */ +const EXCLUDED_FOLDER_IDS_CONTEXT_KEY = '_outlookExcludedFolderIds' + +const EMPTY_FOLDER_IDS: ReadonlySet = new Set() + +/** + * A mail folder as returned by the folder endpoints, narrowed to the fields + * needed to walk the Deleted Items subtree. + */ +export interface OutlookMailFolder { + id?: string + childFolderCount?: number +} + +/** + * Resolves the configured folder, defaulting to the inbox. + */ +export function resolveFolder(sourceConfig: Record): string { + const folder = sourceConfig.folder + if (typeof folder !== 'string') return 'inbox' + const trimmed = folder.trim() + return trimmed || 'inbox' +} + +/** + * Whether the sync targets the mailbox-wide `/me/messages` endpoint, which is + * the only path that can surface Deleted Items content. + */ +export function isAllMailSync(sourceConfig: Record): boolean { + return resolveFolder(sourceConfig) === 'all' +} + +/** + * Keeps a message unless its parent folder is an explicitly excluded one. + * + * Fails open: a message with no `parentFolderId` (unselected or omitted by + * Graph) or an unresolved exclusion set is kept, because a false exclusion + * would hard-delete a still-current conversation from the knowledge base. + */ +export function isCurrentMessage( + message: Pick, + excludedFolderIds: ReadonlySet +): boolean { + if (excludedFolderIds.size === 0) return true + if (!message.parentFolderId) return true + return !excludedFolderIds.has(message.parentFolderId) +} + +/** + * Narrows an untyped Graph folder-collection payload into the folders it + * contains plus its continuation link. Anything unrecognized yields no folders, + * which fails open into keeping messages. + */ +export function parseFolderCollection(payload: unknown): { + folders: OutlookMailFolder[] + nextLink?: string +} { + if (!payload || typeof payload !== 'object') return { folders: [] } + const record = payload as Record + const value = Array.isArray(record.value) ? record.value : [] + const folders: OutlookMailFolder[] = [] + + for (const entry of value) { + if (!entry || typeof entry !== 'object') continue + const folder = entry as Record + if (typeof folder.id !== 'string' || !folder.id) continue + folders.push({ + id: folder.id, + childFolderCount: typeof folder.childFolderCount === 'number' ? folder.childFolderCount : 0, + }) + } + + const nextLink = record['@odata.nextLink'] + return { folders, nextLink: typeof nextLink === 'string' ? nextLink : undefined } +} + +/** + * Performs a `GET` against Graph and returns the parsed JSON body, or `null` + * when the request failed for any reason. Callers treat `null` as "unresolved" + * and fail open. + */ +async function fetchFolderJson(url: string, accessToken: string): Promise { + try { + const response = await fetchWithRetry(url, { + method: 'GET', + headers: { + Authorization: `Bearer ${accessToken}`, + Accept: 'application/json', + }, + }) + + if (!response.ok) { + logger.warn('Failed to read Outlook mail folder', { url, status: response.status }) + return null + } + + return await response.json() + } catch (error) { + logger.warn('Failed to read Outlook mail folder', { + url, + error: getErrorMessage(error, 'Unknown error'), + }) + return null + } +} + +/** + * Resolves the id of Deleted Items and of every folder nested beneath it. + * + * Graph cannot filter messages by well-known folder name and `parentFolderId` + * on a message points at its immediate folder, so a message a user deleted + * while it sat in a user-created folder carries that subfolder's id rather than + * the Deleted Items id. The subtree is therefore walked breadth-first, spending + * requests only on folders that report `childFolderCount > 0` and stopping at + * {@link MAX_FOLDER_RESOLUTION_REQUESTS}. + * + * Every id returned is known to sit inside Deleted Items, so a partial result + * from a failed or truncated walk is still safe — it under-excludes rather than + * over-excludes. + */ +async function computeExcludedFolderIds(accessToken: string): Promise> { + const ids = new Set() + let requests = 0 + + const rootPayload = await fetchFolderJson( + `${GRAPH_API_BASE}/mailFolders/${DELETED_ITEMS_FOLDER}?$select=id,childFolderCount`, + accessToken + ) + requests++ + + if (!rootPayload || typeof rootPayload !== 'object') return EMPTY_FOLDER_IDS + const root = rootPayload as Record + if (typeof root.id !== 'string' || !root.id) return EMPTY_FOLDER_IDS + + ids.add(root.id) + + const pending: string[] = + typeof root.childFolderCount === 'number' && root.childFolderCount > 0 ? [root.id] : [] + + while (pending.length > 0) { + const parentId = pending.shift() + if (!parentId) continue + + const params = new URLSearchParams({ + $select: 'id,childFolderCount', + $top: String(CHILD_FOLDER_PAGE_SIZE), + includeHiddenFolders: 'true', + }) + let url: string | undefined = + `${GRAPH_API_BASE}/mailFolders/${encodeURIComponent(parentId)}/childFolders?${params.toString()}` + + while (url) { + if (requests >= MAX_FOLDER_RESOLUTION_REQUESTS) { + logger.warn( + 'Deleted Items folder walk hit its request budget; deeper folders not excluded', + { + resolvedFolders: ids.size, + } + ) + return ids + } + + const payload = await fetchFolderJson(url, accessToken) + requests++ + if (!payload) break + + const { folders, nextLink } = parseFolderCollection(payload) + for (const folder of folders) { + if (!folder.id || ids.has(folder.id)) continue + ids.add(folder.id) + if ((folder.childFolderCount ?? 0) > 0) pending.push(folder.id) + } + + url = nextLink + } + } + + return ids +} + +/** + * Resolves the excluded folder ids for the current sync run, memoizing the + * in-flight lookup on `syncContext` when one is available so the concurrent + * `getDocument` fan-out shares a single folder walk. Without a `syncContext` + * the lookup is simply repeated — there is deliberately no process-global + * cache, and in particular none keyed by an OAuth access token. + */ +async function resolveExcludedFolderIds( + accessToken: string, + syncContext?: Record +): Promise> { + if (!syncContext) return computeExcludedFolderIds(accessToken) + + const cached = syncContext[EXCLUDED_FOLDER_IDS_CONTEXT_KEY] + if (cached instanceof Promise) return cached as Promise> + + const pending = computeExcludedFolderIds(accessToken).catch((error) => { + delete syncContext[EXCLUDED_FOLDER_IDS_CONTEXT_KEY] + logger.warn('Failed to resolve Outlook Deleted Items folders', { + error: getErrorMessage(error, 'Unknown error'), + }) + return EMPTY_FOLDER_IDS + }) + syncContext[EXCLUDED_FOLDER_IDS_CONTEXT_KEY] = pending + return pending +} + /** * Builds the initial Graph API URL for listing messages. */ function buildInitialUrl(sourceConfig: Record): string { - const folder = (sourceConfig.folder as string) || 'inbox' + const folder = resolveFolder(sourceConfig) const basePath = folder === 'all' ? `${GRAPH_API_BASE}/messages` @@ -320,7 +569,16 @@ export const outlookConnector: ConnectorConfig = { const focusedOnly = sourceConfig.focusedOnly !== 'false' const hasSearch = Boolean((sourceConfig.query as string)?.trim()) + const excludedFolderIds = isAllMailSync(sourceConfig) + ? await resolveExcludedFolderIds(accessToken, syncContext) + : EMPTY_FOLDER_IDS + for (const msg of messages) { + /** Deleted mail must leave the listing so reconciliation can purge it */ + if (!isCurrentMessage(msg, excludedFolderIds)) { + continue + } + // Skip drafts (filtered server-side when no search, client-side otherwise) if (hasSearch && msg.isDraft) { continue @@ -350,6 +608,13 @@ export const outlookConnector: ConnectorConfig = { } if (syncContext) { + /** + * Stopping at `MAX_TOTAL_MESSAGES` while Graph still offers a + * `@odata.nextLink` means the mailbox was not fully traversed. Flag the + * listing as capped so deletion reconciliation does not read the + * unvisited tail as deleted mail and hard-delete those documents. + */ + if (nextLink) syncContext.listingCapped = true syncContext._fetchComplete = true } } @@ -375,8 +640,15 @@ export const outlookConnector: ConnectorConfig = { return maxDateB.localeCompare(maxDateA) }) - // Limit to maxConversations + /** + * Limit to `maxConversations`. Dropping the overflow makes the listing an + * incomplete view of the mailbox, so it is flagged as capped — otherwise + * reconciliation would hard-delete every conversation past the cap. + */ const limited = conversationEntries.slice(0, maxConversations) + if (conversationEntries.length > limited.length && syncContext) { + syncContext.listingCapped = true + } const documents: ExternalDocument[] = [] for (const [convId, msgs] of limited) { @@ -409,11 +681,12 @@ export const outlookConnector: ConnectorConfig = { getDocument: async ( accessToken: string, sourceConfig: Record, - externalId: string + externalId: string, + syncContext?: Record ): Promise => { try { // Scope to the same folder as listDocuments so contentHash stays consistent - const folder = (sourceConfig.folder as string) || 'inbox' + const folder = resolveFolder(sourceConfig) const basePath = folder === 'all' ? `${GRAPH_API_BASE}/messages` @@ -447,7 +720,19 @@ export const outlookConnector: ConnectorConfig = { } const data = await response.json() - const messages = (data.value || []) as OutlookMessage[] + const allMessages = (data.value || []) as OutlookMessage[] + + /** + * Mirrors the listing's exclusion so `contentHash` is computed over the + * same message set on both sides. Without it, deleting the newest message + * of a conversation would leave the listing hash (recomputed from the + * surviving messages) permanently disagreeing with the hash returned + * here, re-fetching the conversation on every sync forever. + */ + const excludedFolderIds = isAllMailSync(sourceConfig) + ? await resolveExcludedFolderIds(accessToken, syncContext) + : EMPTY_FOLDER_IDS + const messages = allMessages.filter((msg) => isCurrentMessage(msg, excludedFolderIds)) if (messages.length === 0) return null @@ -495,7 +780,7 @@ export const outlookConnector: ConnectorConfig = { try { // Verify Graph API access - const folder = (sourceConfig.folder as string) || 'inbox' + const folder = resolveFolder(sourceConfig) const testUrl = folder === 'all' ? `${GRAPH_API_BASE}/messages?$top=1&$select=id` diff --git a/apps/sim/connectors/servicenow/servicenow.test.ts b/apps/sim/connectors/servicenow/servicenow.test.ts new file mode 100644 index 00000000000..64517be929d --- /dev/null +++ b/apps/sim/connectors/servicenow/servicenow.test.ts @@ -0,0 +1,300 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/components/icons', () => ({ + ServiceNowIcon: () => null, +})) + +import { servicenowConnector, shouldIngestKBArticle } from '@/connectors/servicenow/servicenow' + +const INSTANCE_URL = 'https://acme.service-now.com' +const SYS_ID_A = 'a'.repeat(32) +const SYS_ID_B = 'b'.repeat(32) +const SYS_ID_C = 'c'.repeat(32) + +const KB_CONFIG = { + instanceUrl: INSTANCE_URL, + username: 'svc', + contentType: 'kb_knowledge', +} as const + +interface FakeRecord { + sys_id: string + workflow_state?: unknown + short_description?: string + text?: string +} + +function kbRecord(sysId: string, workflowState?: unknown): FakeRecord { + return { + sys_id: sysId, + ...(workflowState === undefined ? {} : { workflow_state: workflowState }), + short_description: `Article ${sysId.slice(0, 4)}`, + text: `Body of ${sysId.slice(0, 4)}`, + } +} + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }) +} + +const mockFetch = vi.fn() + +beforeEach(() => { + mockFetch.mockReset() + vi.stubGlobal('fetch', mockFetch) +}) + +afterEach(() => { + vi.unstubAllGlobals() +}) + +/** Reads the URL passed to the single fetch call as a parsed URL. */ +function lastRequestUrl(): URL { + const [url] = mockFetch.mock.calls[mockFetch.mock.calls.length - 1] + return new URL(url as string) +} + +describe('shouldIngestKBArticle', () => { + it.concurrent('keeps published, draft and review articles', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'published' })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'draft' })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'review' })).toBe(true) + }) + + it.concurrent('excludes retired articles', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' })).toBe(false) + }) + + it.concurrent('keeps outdated articles, which may be the latest version past valid_to', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'outdated' })).toBe(true) + }) + + it.concurrent('keeps pending-retirement articles, which are still visible', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'pending retirement' })).toBe(true) + }) + + it.concurrent('reads the sysparm_display_value=all object shape', () => { + expect( + shouldIngestKBArticle({ + sys_id: 'a', + workflow_state: { value: 'retired', display_value: 'Retired' }, + }) + ).toBe(false) + expect( + shouldIngestKBArticle({ + sys_id: 'a', + workflow_state: { value: 'published', display_value: 'Published' }, + }) + ).toBe(true) + }) + + it.concurrent('falls back to display_value when no raw value is present', () => { + expect( + shouldIngestKBArticle({ sys_id: 'a', workflow_state: { display_value: 'Retired' } }) + ).toBe(false) + }) + + it.concurrent('is case and whitespace insensitive', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: ' Retired ' })).toBe(false) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'RETIRED' })).toBe(false) + }) + + it.concurrent('fails open when workflow_state is missing, empty or not a string', () => { + expect(shouldIngestKBArticle({ sys_id: 'a' })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: '' })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: ' ' })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: null })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 42 })).toBe(true) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: {} })).toBe(true) + }) + + it.concurrent('fails open on unrecognised custom workflow states', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'pending_translation' })).toBe(true) + }) + + it.concurrent('applies no implicit filter once the user selects a state explicitly', () => { + for (const selection of ['all', 'retired', 'outdated', 'published', 'draft', 'review']) { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' }, selection)).toBe( + true + ) + } + }) + + it.concurrent('treats an absent or blank selection as unset and filters', () => { + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' })).toBe(false) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' }, undefined)).toBe(false) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' }, '')).toBe(false) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' }, ' ')).toBe(false) + expect(shouldIngestKBArticle({ sys_id: 'a', workflow_state: 'retired' }, null)).toBe(false) + }) +}) + +describe('servicenowConnector.listDocuments', () => { + it('drops retired KB records and keeps every other state', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ + result: [ + kbRecord(SYS_ID_A, 'published'), + kbRecord(SYS_ID_B, 'retired'), + kbRecord(SYS_ID_C, 'outdated'), + ], + }) + ) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A, SYS_ID_C]) + }) + + it('keeps records with a missing or unrecognised workflow_state (fail open)', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ + result: [ + kbRecord(SYS_ID_A), + kbRecord(SYS_ID_B, ''), + kbRecord(SYS_ID_C, 'pending retirement'), + ], + }) + ) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A, SYS_ID_B, SYS_ID_C]) + }) + + it('applies no implicit filter when the user selected a workflow state', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ result: [kbRecord(SYS_ID_A, 'retired'), kbRecord(SYS_ID_B, 'retired')] }) + ) + + const list = await servicenowConnector.listDocuments('key', { + ...KB_CONFIG, + workflowState: 'retired', + }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A, SYS_ID_B]) + expect(lastRequestUrl().searchParams.get('sysparm_query')).toContain('workflow_state=retired') + }) + + it('applies no implicit filter under the explicit "All States" selection', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ result: [kbRecord(SYS_ID_A, 'published'), kbRecord(SYS_ID_B, 'retired')] }) + ) + + const list = await servicenowConnector.listDocuments('key', { + ...KB_CONFIG, + workflowState: 'all', + }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A, SYS_ID_B]) + expect(lastRequestUrl().searchParams.get('sysparm_query')).not.toContain('workflow_state') + }) + + it('leaves incidents unfiltered even when workflowState is set', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ + result: [ + { sys_id: SYS_ID_A, number: 'INC001', short_description: 'Down', state: '7' }, + { + sys_id: SYS_ID_B, + number: 'INC002', + short_description: 'Up', + workflow_state: 'retired', + }, + ], + }) + ) + + const list = await servicenowConnector.listDocuments('key', { + instanceUrl: INSTANCE_URL, + username: 'svc', + contentType: 'incident', + workflowState: 'published', + }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A, SYS_ID_B]) + expect(lastRequestUrl().pathname).toBe('/api/now/table/incident') + }) + + it('keeps paging when a full page is filtered away entirely', async () => { + const fullPage = Array.from({ length: 100 }, (_, index) => + kbRecord(index.toString(16).padStart(32, '0'), 'retired') + ) + mockFetch.mockResolvedValueOnce(jsonResponse({ result: fullPage })) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG, maxItems: '500' }) + + expect(list.documents).toEqual([]) + expect(list.hasMore).toBe(true) + expect(list.nextCursor).toBe('100') + }) + + it('derives the cursor from the API result count, not the filtered document count', async () => { + const page = Array.from({ length: 100 }, (_, index) => + kbRecord(index.toString(16).padStart(32, '0'), index === 0 ? 'published' : 'retired') + ) + mockFetch.mockResolvedValueOnce(jsonResponse({ result: page })) + + const list = await servicenowConnector.listDocuments('key', { + ...KB_CONFIG, + maxItems: '500', + }) + + expect(list.documents).toHaveLength(1) + expect(list.nextCursor).toBe('100') + + mockFetch.mockResolvedValueOnce(jsonResponse({ result: [] })) + await servicenowConnector.listDocuments('key', { ...KB_CONFIG, maxItems: '500' }, '100') + expect(lastRequestUrl().searchParams.get('sysparm_offset')).toBe('100') + }) + + it('stops paging on a short page', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse({ result: [kbRecord(SYS_ID_A, 'published')] })) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(list.hasMore).toBe(false) + expect(list.nextCursor).toBeUndefined() + }) +}) + +describe('servicenowConnector.getDocument', () => { + it('hydrates a retired article without re-filtering it', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse({ result: kbRecord(SYS_ID_A, 'retired') })) + + const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, SYS_ID_A) + + expect(doc?.externalId).toBe(SYS_ID_A) + expect(doc?.metadata.workflowState).toBe('retired') + }) + + it('hydrates a published article', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse({ result: kbRecord(SYS_ID_A, 'published') })) + + const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, SYS_ID_A) + + expect(doc?.externalId).toBe(SYS_ID_A) + expect(lastRequestUrl().pathname).toBe(`/api/now/table/kb_knowledge/${SYS_ID_A}`) + }) + + it('rejects a malformed sys_id without issuing a request', async () => { + const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, '../../secrets') + + expect(doc).toBeNull() + expect(mockFetch).not.toHaveBeenCalled() + }) + + it('returns null on a 404', async () => { + mockFetch.mockResolvedValueOnce(new Response('', { status: 404 })) + + const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, SYS_ID_A) + + expect(doc).toBeNull() + }) +}) diff --git a/apps/sim/connectors/servicenow/servicenow.ts b/apps/sim/connectors/servicenow/servicenow.ts index 74250d7a797..e7235a15f72 100644 --- a/apps/sim/connectors/servicenow/servicenow.ts +++ b/apps/sim/connectors/servicenow/servicenow.ts @@ -30,6 +30,64 @@ const NUMERIC_ID_PATTERN = /^\d+$/ const KB_CATEGORY_DISALLOWED = /[\^"'`\u0000-\u001f\u007f]/ const VALID_WORKFLOW_STATES = new Set(['published', 'draft', 'review', 'retired', 'outdated']) +/** + * The single KB workflow state that means "removed from view". + * + * `retired` is the documented end of a knowledge article's lifecycle: the row is + * never deleted from `kb_knowledge`, it just stops appearing in knowledge search + * and portal views. It is therefore the only state safe to drop implicitly. + * + * `outdated` is deliberately NOT in this set. It marks a superseded version when + * a new version is published, but ServiceNow also moves an article to `outdated` + * once it passes its `valid_to` date — and that record is still the latest + * version of the article, not a historical snapshot. Excluding `outdated` would + * therefore make deletion reconciliation hard-delete live customer content. + */ +const REMOVED_FROM_VIEW_KB_WORKFLOW_STATE = 'retired' + +/** + * Decides whether a `kb_knowledge` record should be ingested. + * + * `GET /api/now/table/kb_knowledge` applies no implicit state filter, so retired + * articles keep appearing in every full listing, keep getting upserted, and + * never fall out via deletion reconciliation (which removes only documents + * absent from the listing). This guard drops them. + * + * The implicit exclusion applies ONLY when the connector config leaves + * `workflowState` unset. Any explicit selection — including `all` ("All States") + * and `retired` itself — is the user's stated intent and is honoured verbatim, + * with no client-side filtering layered on top of the server-side + * `sysparm_query` clause built by {@link buildKBQuery}. + * + * Fail-open by design: only an EXPLICIT `retired` state excludes a record. A + * missing, empty, non-string or unrecognised state (`pending retirement`, a + * customer-defined state) is kept, because a wrongful exclusion would make + * reconciliation hard-delete a still-current article. For the same reason the + * exclusion is applied client-side rather than as an encoded-query `!=` clause: + * ServiceNow's negative operators have inconsistently reported behaviour for + * empty field values, and a server-side filter that silently dropped rows with + * no `workflow_state` would purge them. + * + * Known and accepted consequence: no `latest=true` filter is applied, so when an + * instance runs the knowledge versioning plugin, superseded versions are still + * ingested as separate documents (each prior version is its own `kb_knowledge` + * row with its own sys_id). Filtering on `latest` is not safe generally — + * instances without versioning enabled do not populate the field reliably, and a + * missing/false value there would purge current articles. + */ +export function shouldIngestKBArticle( + article: Record, + configuredWorkflowState?: unknown +): boolean { + if (typeof configuredWorkflowState === 'string' && configuredWorkflowState.trim()) { + return true + } + + const state = rawValue(article.workflow_state)?.trim().toLowerCase() + if (!state) return true + return state !== REMOVED_FROM_VIEW_KB_WORKFLOW_STATE +} + interface ServiceNowRecord { sys_id: string sys_updated_on?: string @@ -438,7 +496,7 @@ export const servicenowConnector: ConnectorConfig = { accessToken: string, sourceConfig: Record, cursor?: string, - _syncContext?: Record + syncContext?: Record ): Promise => { const instanceUrl = resolveServiceNowInstanceUrl(sourceConfig.instanceUrl as string) const contentType = (sourceConfig.contentType as string) || 'kb_knowledge' @@ -448,6 +506,13 @@ export const servicenowConnector: ConnectorConfig = { const offset = cursor ? Number(cursor) : 0 const remaining = maxItems - offset if (remaining <= 0) { + /** + * The `maxItems` cap stopped this listing before the table was exhausted, + * so it does not represent the full source set. Flagging it keeps the sync + * engine's deletion reconciliation from reading the missing tail as + * "deleted at the source" and hard-deleting those documents. + */ + if (syncContext) syncContext.listingCapped = true return { documents: [], hasMore: false } } @@ -489,6 +554,17 @@ export const servicenowConnector: ConnectorConfig = { continue } + /** + * Retired articles are dropped here, not via the encoded query, so records + * with no `workflow_state` are never lost to ServiceNow's + * negative-operator semantics. Paging stays derived from the API result + * count (see `serviceNowApiGet`), never from the post-filter document + * count, so filtering never shifts the offset window. + */ + if (isKB && !shouldIngestKBArticle(record, sourceConfig.workflowState)) { + continue + } + const doc = isKB ? kbArticleToDocument(record, instanceUrl) : incidentToDocument(record, instanceUrl) @@ -501,6 +577,15 @@ export const servicenowConnector: ConnectorConfig = { const hasMore = nextOffset !== undefined && nextOffset < maxItems const nextCursor = hasMore ? String(nextOffset) : undefined + /** + * More records exist past the `maxItems` cap. See the `remaining <= 0` + * branch above — the listing is knowingly incomplete, so reconciliation + * must not treat the untraversed tail as removed at the source. + */ + if (nextOffset !== undefined && !hasMore && syncContext) { + syncContext.listingCapped = true + } + logger.info('Fetched ServiceNow documents', { count: documents.length, hasMore, @@ -514,6 +599,15 @@ export const servicenowConnector: ConnectorConfig = { } }, + /** + * Fetches one record by sys_id. + * + * No `workflow_state` guard is applied here on purpose. The sync engine calls + * this only to hydrate content for documents that `listDocuments` already + * admitted (and only for `contentDeferred` entries, which this connector never + * emits), so a second filter would be unreachable today and, if it ever became + * reachable, could only turn an already-selected document into a skip. + */ getDocument: async ( accessToken: string, sourceConfig: Record, diff --git a/apps/sim/connectors/webflow/webflow.test.ts b/apps/sim/connectors/webflow/webflow.test.ts new file mode 100644 index 00000000000..25efd507f67 --- /dev/null +++ b/apps/sim/connectors/webflow/webflow.test.ts @@ -0,0 +1,49 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { isCurrentItem } from '@/connectors/webflow/webflow' + +describe('isCurrentItem', () => { + it.concurrent('keeps items explicitly not archived', () => { + expect(isCurrentItem({ isArchived: false })).toBe(true) + }) + + it.concurrent('excludes items explicitly archived', () => { + expect(isCurrentItem({ isArchived: true })).toBe(false) + }) + + it.concurrent('keeps items with no archived flag', () => { + expect(isCurrentItem({})).toBe(true) + }) + + it.concurrent('keeps items whose archived flag is undefined', () => { + expect(isCurrentItem({ isArchived: undefined })).toBe(true) + }) + + it.concurrent('keeps drafts, which are unpublished but still present in the CMS', () => { + expect(isCurrentItem({ isArchived: false, isDraft: true } as { isArchived?: boolean })).toBe( + true + ) + }) + + it.concurrent('excludes archived drafts', () => { + expect(isCurrentItem({ isArchived: true, isDraft: true } as { isArchived?: boolean })).toBe( + false + ) + }) + + it.concurrent('keeps items when the flag is a non-boolean truthy value', () => { + expect(isCurrentItem({ isArchived: 'true' } as unknown as { isArchived?: boolean })).toBe(true) + }) + + it.concurrent('filters only archived items out of a page listing', () => { + const items = [ + { id: 'a', isArchived: false }, + { id: 'b', isArchived: true }, + { id: 'c' }, + { id: 'd', isDraft: true }, + ] + expect(items.filter(isCurrentItem).map((i) => i.id)).toEqual(['a', 'c', 'd']) + }) +}) diff --git a/apps/sim/connectors/webflow/webflow.ts b/apps/sim/connectors/webflow/webflow.ts index c2172f0afa7..31e3ffd3707 100644 --- a/apps/sim/connectors/webflow/webflow.ts +++ b/apps/sim/connectors/webflow/webflow.ts @@ -22,6 +22,26 @@ interface WebflowItem { lastPublished?: string lastUpdated?: string createdOn?: string + isArchived?: boolean + isDraft?: boolean +} + +/** + * Keeps only CMS items that are still current in Webflow. The staged + * `GET /collections/{id}/items` endpoint returns every item regardless of + * state — it exposes `isArchived`/`isDraft` on each item but offers no query + * parameter to filter by them — and archiving only unpublishes an item from the + * live site while keeping it in the CMS. Without this guard archived items stay + * in every full-sync listing forever and are never purged by deletion + * reconciliation (which removes only stored documents absent from the listing). + * + * Only an explicit `isArchived === true` excludes an item: a missing or + * malformed flag keeps the item, since wrongly excluding a live item would + * hard-delete it from the knowledge base. Drafts are intentionally kept — a + * draft is unpublished, not removed, and it still exists in the CMS. + */ +export function isCurrentItem(item: { isArchived?: boolean }): boolean { + return item.isArchived !== true } interface WebflowPagination { @@ -151,7 +171,13 @@ export const webflowConnector: ConnectorConfig = { pagination: WebflowPagination } - const items = data.items || [] + /** + * Archived items are filtered out before mapping so they leave the listing + * and get purged by deletion reconciliation. Pagination stays driven by the + * raw `pagination.limit`/`pagination.total` from the API, never by the + * filtered count, so cursor math is unaffected. + */ + const items = (data.items || []).filter(isCurrentItem) const pageDocuments: ExternalDocument[] = items.map((item) => itemToDocument(item, currentCollectionId, collectionName) ) @@ -244,6 +270,12 @@ export const webflowConnector: ConnectorConfig = { const item = (await response.json()) as WebflowItem + /** + * Mirrors the listing filter: an archived item must not be resurrected by an + * incremental refresh after reconciliation removed it. + */ + if (!isCurrentItem(item)) return null + const collectionName = await fetchCollectionNameDirect(accessToken, docCollectionId) return itemToDocument(item, docCollectionId, collectionName) }, diff --git a/apps/sim/connectors/youtube/youtube.test.ts b/apps/sim/connectors/youtube/youtube.test.ts new file mode 100644 index 00000000000..fc8d1981c49 --- /dev/null +++ b/apps/sim/connectors/youtube/youtube.test.ts @@ -0,0 +1,426 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + filterAvailableItems, + type PlaylistItem, + readTrustedVideoItems, + youtubeConnector, +} from '@/connectors/youtube/youtube' + +const API_KEY = 'test-key' +const PLAYLIST_ID = 'PL123' + +function item(videoId: string, title = 'A video'): PlaylistItem { + return { + contentDetails: { videoId, videoPublishedAt: '2024-01-01T00:00:00Z' }, + snippet: { title }, + } +} + +interface FakeResponseInit { + status?: number + body?: unknown + text?: string +} + +function fakeResponse({ status = 200, body, text = '' }: FakeResponseInit): Response { + return { + ok: status >= 200 && status < 300, + status, + statusText: '', + headers: { get: () => null }, + json: async () => body, + text: async () => text, + } as unknown as Response +} + +/** Registers a fetch mock that answers by URL prefix, recording every requested URL. */ +function mockFetch(handler: (url: string) => Response): string[] { + const urls: string[] = [] + const fetchMock = vi.fn(async (input: unknown) => { + const url = String(input) + urls.push(url) + return handler(url) + }) + vi.stubGlobal('fetch', fetchMock) + return urls +} + +function playlistPage(items: PlaylistItem[], nextPageToken?: string) { + return { items, ...(nextPageToken ? { nextPageToken } : {}) } +} + +function videoListBody(ids: string[], overrides: Record = {}) { + return { + kind: 'youtube#videoListResponse', + items: ids.map((id) => ({ id })), + ...overrides, + } +} + +function fullVideo(id: string, duration: string) { + return { + id, + snippet: { + title: `Video ${id}`, + description: 'desc', + publishedAt: '2024-01-01T00:00:00Z', + channelTitle: 'Chan', + }, + contentDetails: { duration }, + status: { privacyStatus: 'public' }, + } +} + +const listDocuments = youtubeConnector.listDocuments +const getDocument = youtubeConnector.getDocument + +describe('filterAvailableItems', () => { + it('keeps items whose video is present in the availability set', () => { + const items = [item('aaa'), item('bbb')] + expect(filterAvailableItems(items, new Set(['aaa', 'bbb']))).toEqual(items) + }) + + it('drops the "Deleted video" placeholder absent from videos.list', () => { + const live = item('aaa') + const deleted = item('bbb', 'Deleted video') + expect(filterAvailableItems([live, deleted], new Set(['aaa']))).toEqual([live]) + }) + + it('drops the "Private video" placeholder absent from videos.list', () => { + const live = item('aaa') + const priv: PlaylistItem = { + contentDetails: { videoId: 'bbb' }, + snippet: { title: 'Private video' }, + } + expect(filterAvailableItems([live, priv], new Set(['aaa']))).toEqual([live]) + }) + + it('falls back to snippet.resourceId.videoId when contentDetails is absent', () => { + const legacy: PlaylistItem = { snippet: { resourceId: { videoId: 'ccc' } } } + expect(filterAvailableItems([legacy], new Set(['ccc']))).toEqual([legacy]) + expect(filterAvailableItems([legacy], new Set(['aaa']))).toEqual([]) + }) + + it('drops items with no resolvable video id', () => { + expect(filterAvailableItems([{ snippet: { title: 'No id' } }], new Set(['aaa']))).toEqual([]) + }) + + it('drops everything when the availability set is empty', () => { + expect(filterAvailableItems([item('aaa'), item('bbb')], new Set())).toEqual([]) + }) + + it('keeps everything when availability is unknown (null)', () => { + const items = [item('aaa'), item('bbb')] + expect(filterAvailableItems(items, null)).toEqual(items) + }) + + it('returns an empty list for an empty input', () => { + expect(filterAvailableItems([], new Set(['aaa']))).toEqual([]) + }) + + it('preserves input order and does not mutate the source array', () => { + const items = [item('aaa'), item('bbb'), item('ccc')] + const result = filterAvailableItems(items, new Set(['ccc', 'aaa'])) + expect(result.map((i) => i.contentDetails?.videoId)).toEqual(['aaa', 'ccc']) + expect(items).toHaveLength(3) + }) +}) + +describe('readTrustedVideoItems', () => { + it('accepts a well-formed response and returns its items', () => { + const items = readTrustedVideoItems(videoListBody(['aaa']), ['aaa', 'bbb']) + expect(items?.map((i) => i.id)).toEqual(['aaa']) + }) + + it('accepts a response with no kind field', () => { + expect(readTrustedVideoItems({ items: [{ id: 'aaa' }] }, ['aaa'])).toHaveLength(1) + }) + + it('rejects a response whose kind is not a video listing', () => { + expect(readTrustedVideoItems(videoListBody(['aaa'], { kind: 'youtube#other' }), ['aaa'])).toBe( + null + ) + }) + + it('rejects a response with a missing or non-array items field', () => { + expect(readTrustedVideoItems({ kind: 'youtube#videoListResponse' }, ['aaa'])).toBe(null) + expect(readTrustedVideoItems({ items: 'nope' }, ['aaa'])).toBe(null) + }) + + it('rejects an empty items array when ids were requested', () => { + expect(readTrustedVideoItems(videoListBody([]), ['aaa'])).toBe(null) + }) + + it('accepts an empty items array when nothing was requested', () => { + expect(readTrustedVideoItems(videoListBody([]), [])).toEqual([]) + }) + + it('rejects entries that are not objects or lack a usable string id', () => { + expect(readTrustedVideoItems({ items: ['aaa'] }, ['aaa'])).toBe(null) + expect(readTrustedVideoItems({ items: [null] }, ['aaa'])).toBe(null) + expect(readTrustedVideoItems({ items: [{ id: 42 }] }, ['aaa'])).toBe(null) + expect(readTrustedVideoItems({ items: [{ id: '' }] }, ['aaa'])).toBe(null) + expect(readTrustedVideoItems({ items: [{ snippet: {} }] }, ['aaa'])).toBe(null) + }) + + it('rejects a response containing an id that was never requested', () => { + expect(readTrustedVideoItems(videoListBody(['zzz']), ['aaa'])).toBe(null) + }) +}) + +describe('youtubeConnector.listDocuments', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('resolves availability with a single id-only videos.list call and drops absent videos', async () => { + const urls = mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb', 'Deleted video')]) }) + : fakeResponse({ body: videoListBody(['aaa']) }) + ) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) + expect(result.hasMore).toBe(false) + expect(urls).toHaveLength(2) + expect(urls[1]).toContain('/videos?part=id&id=aaa%2Cbbb') + expect(urls[1]).toContain('key=test-key') + }) + + it('never zeroes out a page: an availability response omitting every id is untrusted', async () => { + const urls = mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) + : fakeResponse({ body: videoListBody([]) }) + ) + + const syncContext: Record = {} + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }, undefined, syncContext) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + expect(result.hasMore).toBe(true) + expect(result.nextCursor).toBe('TOKEN2') + expect(urls[1]).toContain('/videos?part=id') + expect(syncContext.listingTruncated).toBeUndefined() + }) + + it('advances pagination on the real cursor even when most of a page is dropped', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ + body: playlistPage([item('aaa'), item('bbb'), item('ccc')], 'TOKEN2'), + }) + : fakeResponse({ body: videoListBody(['aaa']) }) + ) + + const syncContext: Record = {} + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }, undefined, syncContext) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) + expect(result.hasMore).toBe(true) + expect(result.nextCursor).toBe('TOKEN2') + expect(syncContext.totalDocsFetched).toBe(1) + }) + + it('fails open and keeps every item when videos.list returns an empty items array', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) + : fakeResponse({ body: videoListBody([]) }) + ) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + }) + + it('fails open when videos.list omits the items field entirely', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) + : fakeResponse({ body: { kind: 'youtube#videoListResponse' } }) + ) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + }) + + it('fails open when videos.list returns an unexpected kind', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) + : fakeResponse({ body: videoListBody(['aaa'], { kind: 'youtube#searchListResponse' }) }) + ) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + }) + + it('fails open when videos.list returns an id that was not requested', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) + : fakeResponse({ body: videoListBody(['aaa', 'unrelated']) }) + ) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + }) + + it('throws (aborting the sync, which deletes nothing) when videos.list returns 403', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa')]) }) + : fakeResponse({ status: 403, text: 'quotaExceeded' }) + ) + + await expect(listDocuments(API_KEY, { playlistId: PLAYLIST_ID })).rejects.toThrow( + 'Failed to batch-fetch YouTube videos: 403' + ) + }) + + it('throws when the playlistItems listing itself fails', async () => { + mockFetch(() => fakeResponse({ status: 404, text: 'playlistNotFound' })) + + await expect(listDocuments(API_KEY, { playlistId: PLAYLIST_ID })).rejects.toThrow( + 'Failed to list YouTube playlist items: 404' + ) + }) + + it('makes no videos.list call when the page has no usable items', async () => { + const urls = mockFetch(() => fakeResponse({ body: playlistPage([]) })) + + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) + + expect(result.documents).toEqual([]) + expect(urls).toHaveLength(1) + }) + + it('hydrates full documents and drops Shorts when excludeShorts is on', async () => { + const urls = mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb'), item('ccc')]) }) + : fakeResponse({ + body: { + kind: 'youtube#videoListResponse', + items: [fullVideo('aaa', 'PT5M'), fullVideo('bbb', 'PT30S')], + }, + }) + ) + + const result = await listDocuments(API_KEY, { + playlistId: PLAYLIST_ID, + excludeShorts: 'true', + }) + + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) + expect(result.documents[0].contentDeferred).toBe(false) + expect(urls[1]).toContain('/videos?part=snippet%2CcontentDetails%2Cstatus') + }) + + it('advances pagination when every video on a page is an excluded Short', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) + : fakeResponse({ + body: { + kind: 'youtube#videoListResponse', + items: [fullVideo('aaa', 'PT20S'), fullVideo('bbb', 'PT10S')], + }, + }) + ) + + const result = await listDocuments(API_KEY, { + playlistId: PLAYLIST_ID, + excludeShorts: 'true', + }) + + expect(result.documents).toEqual([]) + expect(result.hasMore).toBe(true) + expect(result.nextCursor).toBe('TOKEN2') + }) + + it('blocks reconciliation instead of dropping a page when the shorts lookup is untrusted', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) + : fakeResponse({ body: videoListBody([]) }) + ) + + const syncContext: Record = {} + const result = await listDocuments( + API_KEY, + { playlistId: PLAYLIST_ID, excludeShorts: 'true' }, + undefined, + syncContext + ) + + expect(result.documents).toEqual([]) + expect(syncContext.listingCapped).toBe(true) + expect(syncContext.listingTruncated).toBe(true) + expect(result.hasMore).toBe(true) + expect(result.nextCursor).toBe('TOKEN2') + }) +}) + +describe('youtubeConnector.getDocument', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('hydrates a video into a full document', async () => { + const urls = mockFetch(() => + fakeResponse({ + body: { kind: 'youtube#videoListResponse', items: [fullVideo('aaa', 'PT5M')] }, + }) + ) + + const doc = await getDocument(API_KEY, {}, 'aaa') + + expect(doc?.externalId).toBe('aaa') + expect(doc?.content).toBe('Video aaa\n\ndesc') + expect(urls[0]).toContain('id=aaa') + }) + + it('returns null when the video is gone (empty items)', async () => { + mockFetch(() => fakeResponse({ body: { kind: 'youtube#videoListResponse', items: [] } })) + + expect(await getDocument(API_KEY, {}, 'aaa')).toBe(null) + }) + + it('returns null on 403 and 404 without throwing', async () => { + mockFetch(() => fakeResponse({ status: 403, text: 'forbidden' })) + expect(await getDocument(API_KEY, {}, 'aaa')).toBe(null) + + vi.unstubAllGlobals() + mockFetch(() => fakeResponse({ status: 404, text: 'notFound' })) + expect(await getDocument(API_KEY, {}, 'aaa')).toBe(null) + }) + + it('swallows transport failures and returns null', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + throw new Error('boom') + }) + ) + + expect(await getDocument(API_KEY, {}, 'aaa')).toBe(null) + }) +}) diff --git a/apps/sim/connectors/youtube/youtube.ts b/apps/sim/connectors/youtube/youtube.ts index 39190d12c94..201e321b9dc 100644 --- a/apps/sim/connectors/youtube/youtube.ts +++ b/apps/sim/connectors/youtube/youtube.ts @@ -25,7 +25,7 @@ const SHORTS_MAX_DURATION_SECONDS = 60 * These differ for hand-curated playlists, so only `videoPublishedAt` is used for the * change-detection hash (it matches `videos.list` `snippet.publishedAt`). */ -interface PlaylistItem { +export interface PlaylistItem { contentDetails?: { videoId?: string; videoPublishedAt?: string } snippet?: { title?: string @@ -217,21 +217,45 @@ function itemToStub(item: PlaylistItem): ExternalDocument | null { } /** - * Batch-fetches full video resources for the given IDs via `videos.list`. + * `videos.list` part sets used by this connector. The full set hydrates a document; the + * id-only set is the cheapest way to ask "do these videos still exist?". + */ +const VIDEO_PART_FULL = 'snippet,contentDetails,status' +const VIDEO_PART_ID_ONLY = 'id' + +/** Documented `kind` of a `videos.list` response body. */ +const VIDEO_LIST_KIND = 'youtube#videoListResponse' + +/** + * Untyped `videos.list` response envelope. The payload is validated by + * `readTrustedVideoItems` before any field is consumed. + */ +interface VideoListResponse { + kind?: unknown + items?: unknown +} + +/** + * Issues a single `videos.list` call for the given IDs and part set. * * `videos.list` accepts up to 50 comma-separated IDs and costs a flat 1 quota unit per - * call regardless of ID count, so a single call covers a full `playlistItems.list` page. - * Videos that are private, deleted, or region-blocked are simply absent from the response. + * call regardless of ID count or part set, so one call covers a full + * `playlistItems.list` page. `maxResults` is explicitly unsupported alongside `id`, so an + * ID-filtered response is never paginated. + * + * A non-OK response throws. That deliberately aborts the whole sync: `runConnectorSync` + * pushes listing pages inside its try block and reaches deletion reconciliation only + * after the pagination loop completes, so an aborted listing performs no reconciliation + * and deletes nothing (the failure path only records the error and backs the connector + * off). Failing loud is therefore strictly safer than returning a partial listing. */ -async function fetchVideosByIds( +async function fetchVideoList( apiKey: string, videoIds: string[], + part: string, retryOptions?: Parameters[2] -): Promise> { - const result = new Map() - if (videoIds.length === 0) return result - - const url = `${YOUTUBE_API_BASE}/videos?part=snippet,contentDetails,status&id=${encodeURIComponent( +): Promise { + const url = `${YOUTUBE_API_BASE}/videos?part=${encodeURIComponent(part)}&id=${encodeURIComponent( videoIds.join(',') )}&key=${encodeURIComponent(apiKey)}` @@ -245,20 +269,135 @@ async function fetchVideosByIds( const errorText = await response.text().catch(() => '') logger.error('Failed to batch-fetch YouTube videos', { count: videoIds.length, + part, status: response.status, error: errorText.slice(0, 500), }) throw new Error(`Failed to batch-fetch YouTube videos: ${response.status}`) } - const data = await response.json() - const items = (data.items ?? []) as VideoItem[] + return (await response.json()) as VideoListResponse +} + +/** + * Narrows a `videos.list` payload to its item array, or returns null when the response + * cannot be trusted as a complete answer to the request that was made. + * + * The YouTube reference documents neither that every requested existing ID is returned + * nor how a partially-degraded batch is signalled, so absence from the response is only + * ever read as "this video is gone" when the payload is unambiguous. A response is + * rejected as untrusted when: + * - `kind` is present and is not `youtube#videoListResponse` (not a video listing) + * - `items` is missing or is not an array (malformed envelope) + * - `items` is empty while IDs were requested — indistinguishable from a degraded 200, + * and reading it literally would drop an entire page at once + * - any entry is not an object, lacks a non-empty string `id`, or carries an `id` that + * was never requested (the body does not correspond to this request) + * + * Callers fail open on null. The residual exposure is a well-formed response that omits + * only *some* still-live IDs; that cannot be detected from the payload, and treating a + * partial response as untrusted is impossible without a completeness guarantee the API + * does not provide. + */ +export function readTrustedVideoItems( + data: VideoListResponse, + requestedIds: readonly string[] +): VideoItem[] | null { + if (typeof data.kind === 'string' && data.kind !== VIDEO_LIST_KIND) return null + if (!Array.isArray(data.items)) return null + + const entries = data.items as unknown[] + if (requestedIds.length > 0 && entries.length === 0) return null + + const requested = new Set(requestedIds) + const result: VideoItem[] = [] + for (const entry of entries) { + if (typeof entry !== 'object' || entry === null) return null + const id = (entry as { id?: unknown }).id + if (typeof id !== 'string' || !id) return null + if (!requested.has(id)) return null + result.push(entry as VideoItem) + } + return result +} + +/** + * Batch-fetches full video resources for the given IDs, keyed by video ID. + * + * Returns null when the response is untrusted (see `readTrustedVideoItems`) so the caller + * can fail open instead of treating every ID as gone. Videos that are private, deleted, + * or region-blocked are absent from a trusted response. + */ +async function fetchVideosByIds( + apiKey: string, + videoIds: string[], + retryOptions?: Parameters[2] +): Promise | null> { + const result = new Map() + if (videoIds.length === 0) return result + + const data = await fetchVideoList(apiKey, videoIds, VIDEO_PART_FULL, retryOptions) + const items = readTrustedVideoItems(data, videoIds) + if (!items) return null + for (const item of items) { if (item.id) result.set(item.id, item) } return result } +/** + * Batch-resolves which of the given video IDs still exist, via a lightweight + * `videos.list?part=id` call (flat 1 quota unit, up to 50 IDs per call — a full + * `playlistItems.list` page). + * + * Returns null when the response is untrusted, which callers read as "availability is + * unknown, keep everything". + */ +async function fetchAvailableVideoIds( + apiKey: string, + videoIds: string[], + retryOptions?: Parameters[2] +): Promise | null> { + if (videoIds.length === 0) return new Set() + + const data = await fetchVideoList(apiKey, videoIds, VIDEO_PART_ID_ONLY, retryOptions) + const items = readTrustedVideoItems(data, videoIds) + if (!items) return null + + const result = new Set() + for (const item of items) { + if (item.id) result.add(item.id) + } + return result +} + +/** + * Keeps only playlist items whose video is still available. + * + * A playlist item is a resource independent of the video it points at: when the video is + * deleted or made private, `playlistItems.list` keeps returning a placeholder item + * ("Deleted video" / "Private video") with the same `contentDetails.videoId`, and the API + * exposes no request parameter to exclude them. Left in the listing, those placeholders + * keep the stored document's externalId present on every full sync, so deletion + * reconciliation (which hard-deletes only documents ABSENT from the listing) never purges + * it — and hydration cannot clean it up either, since `videos.list` omits the video and + * `getDocument` returns null, which the sync engine treats as last-known-good. + * + * Availability is therefore established positively, from a `videos.list` membership check: + * an item is dropped only when its video ID is explicitly absent from a trusted response. + * A null set means the membership check could not be trusted, and every item is kept — + * keeping a stale placeholder is recoverable, hard-deleting a live document (and its + * `userExcluded` flag) is not. + */ +export function filterAvailableItems( + items: T[], + availableVideoIds: ReadonlySet | null +): T[] { + if (!availableVideoIds) return items + return items.filter((item) => availableVideoIds.has(getVideoId(item))) +} + /** * Builds the full document for a video, combining title and description as plain-text * content. Returns null for unlisted/private/deleted videos, and (when configured) for @@ -408,23 +547,57 @@ export const youtubeConnector: ConnectorConfig = { let documents: ExternalDocument[] = [] if (excludeShorts && keptItems.length > 0) { - // When excluding Shorts we must know each video's duration, which is not exposed on - // `playlistItems.list`. Resolve it here with a single batched `videos.list` call - // (1 quota unit per page) and emit FULLY-HYDRATED documents. This is deliberate: - // emitting deferred stubs for Shorts would make every excluded Short re-list as a - // brand-new doc on every sync (it is never persisted), re-hydrating to null forever. - // Filtering at listing time bounds the cost to one batched call per page per sync. + /** + * When excluding Shorts we must know each video's duration, which is not exposed on + * `playlistItems.list`. Resolve it here with a single batched `videos.list` call + * (1 quota unit per page) and emit FULLY-HYDRATED documents. This is deliberate: + * emitting deferred stubs for Shorts would make every excluded Short re-list as a + * brand-new doc on every sync (it is never persisted), re-hydrating to null forever. + * Filtering at listing time bounds the cost to one batched call per page per sync. + * + * An untrusted response cannot be read as "every video on this page is gone", and + * durations are unavailable without it, so the page emits nothing and the listing is + * marked truncated — which blocks deletion reconciliation for this sync absolutely. + * Pagination still advances on the real `playlistItems` cursor. + */ const videoMap = await fetchVideosByIds(apiKey, keptItems.map(getVideoId)) - for (const item of keptItems) { - const video = videoMap.get(getVideoId(item)) - // Absent from `videos.list` => private/deleted/region-blocked. Drop it instead of - // emitting a stub that would re-hydrate to null on every sync. - if (!video) continue - const doc = videoToDocument(video, true) - if (doc) documents.push(doc) + if (!videoMap) { + logger.warn('Untrusted videos.list response; skipping page and blocking reconciliation', { + playlistId, + count: keptItems.length, + }) + if (syncContext) { + syncContext.listingCapped = true + syncContext.listingTruncated = true + } + } else { + for (const item of keptItems) { + /** + * Absent from a trusted `videos.list` => private/deleted/region-blocked. Drop it + * instead of emitting a stub that would re-hydrate to null on every sync. + */ + const video = videoMap.get(getVideoId(item)) + if (!video) continue + const doc = videoToDocument(video, true) + if (doc) documents.push(doc) + } + } + } else if (keptItems.length > 0) { + /** + * Placeholder items for deleted/private videos stay in `playlistItems.list` forever + * and would pin the stored document against deletion reconciliation. Resolve which + * videos still exist with one batched `videos.list?part=id` call (1 quota unit per + * page) and emit stubs only for those — mirroring the drop the excludeShorts branch + * above already performs. An untrusted response keeps every item (fail open). + */ + const availableVideoIds = await fetchAvailableVideoIds(apiKey, keptItems.map(getVideoId)) + if (!availableVideoIds) { + logger.warn('Untrusted videos.list response; keeping all playlist items for this page', { + playlistId, + count: keptItems.length, + }) } - } else { - for (const item of keptItems) { + for (const item of filterAvailableItems(keptItems, availableVideoIds)) { const stub = itemToStub(item) if (stub) documents.push(stub) } @@ -440,6 +613,12 @@ export const youtubeConnector: ConnectorConfig = { if (syncContext) syncContext.totalDocsFetched = maxVideos } + /** + * Pagination is driven exclusively by the `playlistItems.list` cursor, never by how + * many documents survived availability filtering. A page whose items are ALL + * unavailable therefore emits zero documents while still advancing to the next page + * and reporting `hasMore` truthfully, so it can neither wedge nor truncate the sync. + */ const nextPageToken = data.nextPageToken as string | undefined // When the `maxVideos` cap stops the listing before the source is exhausted, mark the From d2ee39e0b85c338e2351093ebdf0ec277fdd1b11 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 21:58:11 -0700 Subject: [PATCH 3/4] fix(asana): honour the pinned-project exception on the task rehydrate path listDocuments deliberately keeps syncing a project the user pinned via the `project` config field even once it is archived, but getDocument ignored sourceConfig and applied the all-parents-archived exclusion unconditionally. For a pinned archived project the listing kept emitting its tasks while every hydration returned null, so new tasks were dropped as empty and already-indexed ones were frozen at their last content. isTaskUnderActiveProject now takes the pinned project gid and keeps any task reachable through it, matching the listing exactly. The unpinned path is unchanged and still fails open on missing/non-boolean archived values. --- apps/sim/connectors/asana/asana.test.ts | 24 ++++++++++++++++++++++++ apps/sim/connectors/asana/asana.ts | 14 +++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/apps/sim/connectors/asana/asana.test.ts b/apps/sim/connectors/asana/asana.test.ts index 6e1ffefda48..950e505ba25 100644 --- a/apps/sim/connectors/asana/asana.test.ts +++ b/apps/sim/connectors/asana/asana.test.ts @@ -118,6 +118,30 @@ describe('isTaskUnderActiveProject', () => { } as unknown as typeof baseTask) ).toBe(true) }) + + it.concurrent('keeps a task reachable through the pinned project even once archived', () => { + expect( + isTaskUnderActiveProject( + { + ...baseTask, + projects: [{ gid: 'p1', name: 'Pinned', archived: true }], + }, + 'p1' + ) + ).toBe(true) + }) + + it.concurrent('still excludes an all-archived task when the pinned project is elsewhere', () => { + expect( + isTaskUnderActiveProject( + { + ...baseTask, + projects: [{ gid: 'p1', name: 'Old', archived: true }], + }, + 'p9' + ) + ).toBe(false) + }) }) describe('decideTaskCap', () => { diff --git a/apps/sim/connectors/asana/asana.ts b/apps/sim/connectors/asana/asana.ts index 093cd2da8d8..9c32111beb4 100644 --- a/apps/sim/connectors/asana/asana.ts +++ b/apps/sim/connectors/asana/asana.ts @@ -113,10 +113,17 @@ export function isActiveProject(project: AsanaProject): boolean { * array, or any entry whose `archived` is missing or non-boolean is kept. A * task that still sits in at least one active project is kept, matching the * listing, which reaches it through that active project. + * + * `pinnedProjectGid` mirrors the listing's pinned-project exception: when the + * user configured a specific `project`, the listing keeps syncing it even once + * archived, so a task reachable through that project must survive rehydration + * too. Without this the listing would keep emitting the task while every + * hydration returned `null`, stranding it as permanently empty. */ -export function isTaskUnderActiveProject(task: AsanaTask): boolean { +export function isTaskUnderActiveProject(task: AsanaTask, pinnedProjectGid?: string): boolean { const projects = task.projects if (!Array.isArray(projects) || projects.length === 0) return true + if (pinnedProjectGid && projects.some((project) => project?.gid === pinnedProjectGid)) return true return !projects.every((project) => project?.archived === true) } @@ -382,9 +389,10 @@ export const asanaConnector: ConnectorConfig = { getDocument: async ( accessToken: string, - _sourceConfig: Record, + sourceConfig: Record, externalId: string ): Promise => { + const pinnedProjectGid = (sourceConfig.project as string) || undefined try { const result = await asanaGet<{ data: AsanaTask }>( accessToken, @@ -394,7 +402,7 @@ export const asanaConnector: ConnectorConfig = { if (!task) return null - if (!isTaskUnderActiveProject(task)) { + if (!isTaskUnderActiveProject(task, pinnedProjectGid)) { logger.info('Skipping Asana task whose projects are all archived', { externalId }) return null } From f54b1256ed56bf24814249c47bc9ad648ee923e5 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 22 Jul 2026 22:22:54 -0700 Subject: [PATCH 4/4] fix(connectors): key removal on explicit source signals, never on absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the connector purge fixes, from an independent audit. YouTube inferred deletion from absence: a playlist entry whose id was missing from a `videos.list` response was dropped, so a well-formed 200 that returned 49 of 50 requested ids hard-deleted the 50th. Playlist items instead carry a documented `status.privacyStatus`, available as a free part on a call the connector already makes, so the extra `videos.list` request is gone along with its quota-failure and pagination-wedge risks. An item is now excluded only on an explicit `private`; missing, empty, or unrecognized values keep it. ServiceNow read every record through a guard requiring a string `sys_id`, but the listing requests `sysparm_display_value=all`, under which every field — `sys_id` included — comes back as `{display_value, value}`. The guard rejected every record, so the retired-article filter was unreachable and the sys_id object would have leaked into `externalId` and `title` had it not been. Records are now read through the existing `rawValue` normalizer, which accepts both wire shapes, and the fixtures use the shape the API actually returns. Also: resolve the ServiceNow cap ambiguity with `X-Total-Count` so a table that ends exactly on a page boundary is not read as truncated; stop the Google Sheets comment claiming a purge the engine's zero-document guard prevents; and assert the Outlook junk-mail invariant instead of comparing a constant to itself. Document the behavior change: content archived, retired, or trashed at the source is now removed from the knowledge base, and restoring it re-ingests it. --- .../docs/en/knowledgebase/connectors.mdx | 8 +- .../connectors/google-sheets/google-sheets.ts | 12 +- apps/sim/connectors/outlook/outlook.test.ts | 26 +- .../connectors/servicenow/servicenow.test.ts | 91 +++++- apps/sim/connectors/servicenow/servicenow.ts | 124 +++++--- apps/sim/connectors/youtube/youtube.test.ts | 298 ++++++------------ apps/sim/connectors/youtube/youtube.ts | 145 ++++----- 7 files changed, 369 insertions(+), 335 deletions(-) diff --git a/apps/docs/content/docs/en/knowledgebase/connectors.mdx b/apps/docs/content/docs/en/knowledgebase/connectors.mdx index af47da0818f..4595a6ba11f 100644 --- a/apps/docs/content/docs/en/knowledgebase/connectors.mdx +++ b/apps/docs/content/docs/en/knowledgebase/connectors.mdx @@ -162,6 +162,12 @@ To reverse an exclusion, switch to the **Excluded** tab and click **Restore** On each run the connector fetches documents from the source and compares them against what's already stored. Only changed documents are reprocessed — new content is added, updated content is re-chunked and re-embedded, deleted content is removed. A connector syncing thousands of documents will only do real work when something actually changes. +### Content Removed From View + +Content that still exists at the source but is no longer current is treated the same as deleted, and is removed from your knowledge base. This covers an archived Confluence page, a task in an archived Asana project, an archived Webflow CMS item, a retired ServiceNow knowledge article, a spreadsheet in the Google Drive trash, a message moved to Outlook's Deleted Items, and a cancelled incident.io incident. + +This keeps agents from citing content your team has already retired. Restoring the item at the source brings it back on the next sync as a new document. + ### Connector Status | Status | Meaning | @@ -196,7 +202,7 @@ You can add as many connectors as you need to a single knowledge base. Each mana { }) describe('DELETED_ITEMS_FOLDER', () => { - it('excludes Deleted Items only, never Junk Email', () => { + it('pins the well-known folder name interpolated into the Graph URL', () => { expect(DELETED_ITEMS_FOLDER).toBe('deleteditems') }) }) @@ -196,6 +197,29 @@ describe('listDocuments folder exclusion', () => { expect(result.documents.map((d) => d.externalId)).toEqual(['live']) }) + it('keeps junk mail, which is a spam classification rather than a deletion', async () => { + routeFetch([ + deletedItemsRoute, + childFoldersRoute, + [ + /\/me\/messages\?/, + () => + jsonResponse({ + value: [ + message({ id: 'm1', conversationId: 'live', parentFolderId: INBOX_ID }), + message({ id: 'm2', conversationId: 'junked', parentFolderId: JUNK_EMAIL_ID }), + ], + }), + ], + ]) + + const result = await outlookConnector.listDocuments('token', { folder: 'all' }, undefined, {}) + + expect(result.documents.map((d) => d.externalId).sort()).toEqual(['junked', 'live']) + const requested = fetchMock.mock.calls.map((call) => String(call[0])) + expect(requested.some((url) => url.includes('junkemail'))).toBe(false) + }) + it('keeps every conversation when the Deleted Items lookup fails with a non-2xx', async () => { routeFetch([ [/mailFolders\/deleteditems\?/, () => jsonResponse({ error: 'forbidden' }, { status: 403 })], diff --git a/apps/sim/connectors/servicenow/servicenow.test.ts b/apps/sim/connectors/servicenow/servicenow.test.ts index 64517be929d..65a3c5fa85c 100644 --- a/apps/sim/connectors/servicenow/servicenow.test.ts +++ b/apps/sim/connectors/servicenow/servicenow.test.ts @@ -20,14 +20,27 @@ const KB_CONFIG = { contentType: 'kb_knowledge', } as const -interface FakeRecord { - sys_id: string - workflow_state?: unknown - short_description?: string - text?: string +/** + * Wraps a value the way `sysparm_display_value=all` does. Under `all` the Table + * API returns EVERY column — `sys_id` included — as `{ display_value, value }`, + * so fixtures must use this shape to exercise the real listing path. + */ +function field(value: string): { display_value: string; value: string } { + return { display_value: value, value } +} + +/** Builds a `kb_knowledge` row in the `sysparm_display_value=all` wire shape. */ +function kbRecord(sysId: string, workflowState?: string): Record { + return { + sys_id: field(sysId), + ...(workflowState === undefined ? {} : { workflow_state: field(workflowState) }), + short_description: field(`Article ${sysId.slice(0, 4)}`), + text: field(`Body of ${sysId.slice(0, 4)}`), + } } -function kbRecord(sysId: string, workflowState?: unknown): FakeRecord { +/** Builds the same row in the plain-string shape (`display_value` absent/true/false). */ +function kbRecordPlain(sysId: string, workflowState?: string): Record { return { sys_id: sysId, ...(workflowState === undefined ? {} : { workflow_state: workflowState }), @@ -136,6 +149,47 @@ describe('shouldIngestKBArticle', () => { }) describe('servicenowConnector.listDocuments', () => { + it('accepts the sysparm_display_value=all object shape, where sys_id is an object', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse({ result: [kbRecord(SYS_ID_A, 'published')] })) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(lastRequestUrl().searchParams.get('sysparm_display_value')).toBe('all') + expect(list.documents).toHaveLength(1) + expect(list.documents[0].externalId).toBe(SYS_ID_A) + expect(list.documents[0].title).toBe(`Article ${SYS_ID_A.slice(0, 4)}`) + expect(list.documents[0].sourceUrl).toBe(`${INSTANCE_URL}/kb_view.do?sys_kb_id=${SYS_ID_A}`) + expect(list.documents[0].metadata.workflowState).toBe('published') + }) + + it('still accepts the plain-string shape', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ + result: [kbRecordPlain(SYS_ID_A, 'published'), kbRecordPlain(SYS_ID_B, 'retired')], + }) + ) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_A]) + expect(list.documents[0].title).toBe(`Article ${SYS_ID_A.slice(0, 4)}`) + }) + + it('skips records whose sys_id object carries no usable value', async () => { + mockFetch.mockResolvedValueOnce( + jsonResponse({ + result: [ + { ...kbRecord(SYS_ID_A, 'published'), sys_id: { display_value: null, value: '' } }, + kbRecord(SYS_ID_B, 'published'), + ], + }) + ) + + const list = await servicenowConnector.listDocuments('key', { ...KB_CONFIG }) + + expect(list.documents.map((doc) => doc.externalId)).toEqual([SYS_ID_B]) + }) + it('drops retired KB records and keeps every other state', async () => { mockFetch.mockResolvedValueOnce( jsonResponse({ @@ -200,12 +254,17 @@ describe('servicenowConnector.listDocuments', () => { mockFetch.mockResolvedValueOnce( jsonResponse({ result: [ - { sys_id: SYS_ID_A, number: 'INC001', short_description: 'Down', state: '7' }, { - sys_id: SYS_ID_B, - number: 'INC002', - short_description: 'Up', - workflow_state: 'retired', + sys_id: field(SYS_ID_A), + number: field('INC001'), + short_description: field('Down'), + state: { display_value: 'Closed', value: '7' }, + }, + { + sys_id: field(SYS_ID_B), + number: field('INC002'), + short_description: field('Up'), + workflow_state: field('retired'), }, ], }) @@ -280,7 +339,17 @@ describe('servicenowConnector.getDocument', () => { const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, SYS_ID_A) expect(doc?.externalId).toBe(SYS_ID_A) + expect(doc?.sourceUrl).toBe(`${INSTANCE_URL}/kb_view.do?sys_kb_id=${SYS_ID_A}`) expect(lastRequestUrl().pathname).toBe(`/api/now/table/kb_knowledge/${SYS_ID_A}`) + expect(lastRequestUrl().searchParams.get('sysparm_display_value')).toBe('all') + }) + + it('hydrates an article returned in the plain-string shape', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse({ result: kbRecordPlain(SYS_ID_A, 'published') })) + + const doc = await servicenowConnector.getDocument('key', { ...KB_CONFIG }, SYS_ID_A) + + expect(doc?.externalId).toBe(SYS_ID_A) }) it('rejects a malformed sys_id without issuing a request', async () => { diff --git a/apps/sim/connectors/servicenow/servicenow.ts b/apps/sim/connectors/servicenow/servicenow.ts index e7235a15f72..aa81a1dea12 100644 --- a/apps/sim/connectors/servicenow/servicenow.ts +++ b/apps/sim/connectors/servicenow/servicenow.ts @@ -88,38 +88,63 @@ export function shouldIngestKBArticle( return state !== REMOVED_FROM_VIEW_KB_WORKFLOW_STATE } +/** + * The object form a field takes under `sysparm_display_value=all`. + * + * Under `all` the Table API wraps EVERY column — not just reference and choice + * fields, and explicitly including `sys_id` — in `{ display_value, value }`. + * Reference fields carry an additional `link`, and `display_value` is `null` + * rather than `""` when the field is empty. + */ +interface ServiceNowFieldObject { + value?: string + display_value?: string | null + link?: string +} + +/** + * A field as it may arrive from the Table API. + * + * Both shapes must be tolerated: this connector requests + * `sysparm_display_value=all` (object form), but plain strings are what the + * same endpoint returns under `true`/`false`, and are still what a record + * fetched without the parameter looks like. Every read therefore goes through + * {@link rawValue} or {@link displayValue}, never through a direct field access. + */ +type ServiceNowField = string | ServiceNowFieldObject | null | undefined + interface ServiceNowRecord { - sys_id: string - sys_updated_on?: string - sys_created_on?: string - sys_created_by?: string - sys_updated_by?: string + sys_id: ServiceNowField + sys_updated_on?: ServiceNowField + sys_created_on?: ServiceNowField + sys_created_by?: ServiceNowField + sys_updated_by?: ServiceNowField } interface KBArticle extends ServiceNowRecord { - short_description?: string - text?: string - wiki?: string - workflow_state?: string - kb_category?: string | { display_value?: string } - kb_knowledge_base?: string | { display_value?: string } - number?: string - author?: string | { display_value?: string } + short_description?: ServiceNowField + text?: ServiceNowField + wiki?: ServiceNowField + workflow_state?: ServiceNowField + kb_category?: ServiceNowField + kb_knowledge_base?: ServiceNowField + number?: ServiceNowField + author?: ServiceNowField } interface Incident extends ServiceNowRecord { - number?: string - short_description?: string - description?: string - state?: string - priority?: string - category?: string - assigned_to?: string | { display_value?: string } - opened_by?: string | { display_value?: string } - close_notes?: string - comments_and_work_notes?: string - work_notes?: string - resolution_notes?: string + number?: ServiceNowField + short_description?: ServiceNowField + description?: ServiceNowField + state?: ServiceNowField + priority?: ServiceNowField + category?: ServiceNowField + assigned_to?: ServiceNowField + opened_by?: ServiceNowField + close_notes?: ServiceNowField + comments_and_work_notes?: ServiceNowField + work_notes?: ServiceNowField + resolution_notes?: ServiceNowField } /** @@ -244,14 +269,22 @@ async function serviceNowApiGetById( return data.result ?? null } +/** + * Accepts a record that carries a usable sys_id in either wire shape. + * + * Both listing and single-record fetches send `sysparm_display_value=all`, under + * which `sys_id` arrives as `{ display_value, value }` rather than as a plain + * string, so a `typeof === 'string'` test would reject every record. Normalising + * through {@link rawValue} accepts the object form and still accepts the plain + * string form returned when the parameter is absent or set to `true`/`false`. + * `rawValue` returns `undefined` for an empty string, so a non-empty result is + * exactly the original intent: reject records with no usable sys_id. + */ function isServiceNowRecord(record: unknown): record is ServiceNowRecord & Record { - return ( - typeof record === 'object' && - record !== null && - !Array.isArray(record) && - typeof (record as Record).sys_id === 'string' && - ((record as Record).sys_id as string).length > 0 - ) + if (typeof record !== 'object' || record === null || Array.isArray(record)) { + return false + } + return Boolean(rawValue((record as Record).sys_id)) } /** @@ -326,7 +359,8 @@ function priorityLabel(priority: string | undefined): string { * Converts a KB article record to an ExternalDocument. */ function kbArticleToDocument(article: KBArticle, instanceUrl: string): ExternalDocument { - const title = rawValue(article.short_description) || rawValue(article.number) || article.sys_id + const sysId = rawValue(article.sys_id) ?? '' + const title = rawValue(article.short_description) || rawValue(article.number) || sysId /** * Wiki-template KB articles populate `wiki` with the body and leave * `text` empty; HTML-template articles do the opposite. Falling back @@ -334,7 +368,6 @@ function kbArticleToDocument(article: KBArticle, instanceUrl: string): ExternalD */ const articleText = rawValue(article.text) || rawValue(article.wiki) || '' const content = htmlToPlainText(articleText) - const sysId = rawValue(article.sys_id) || article.sys_id const updatedOn = rawValue(article.sys_updated_on) || '' const contentHash = `servicenow:${sysId}:${updatedOn}` const sourceUrl = `${instanceUrl}/kb_view.do?sys_kb_id=${sysId}` @@ -363,9 +396,10 @@ function kbArticleToDocument(article: KBArticle, instanceUrl: string): ExternalD * Converts an incident record to an ExternalDocument. */ function incidentToDocument(incident: Incident, instanceUrl: string): ExternalDocument { + const sysId = rawValue(incident.sys_id) ?? '' const number = rawValue(incident.number) const shortDesc = rawValue(incident.short_description) - const title = number ? `${number}: ${shortDesc || 'Untitled'}` : shortDesc || incident.sys_id + const title = number ? `${number}: ${shortDesc || 'Untitled'}` : shortDesc || sysId const parts: string[] = [] if (shortDesc) { @@ -399,7 +433,6 @@ function incidentToDocument(incident: Incident, instanceUrl: string): ExternalDo } const content = parts.join('\n') - const sysId = rawValue(incident.sys_id) || incident.sys_id const updatedOn = rawValue(incident.sys_updated_on) || '' const contentHash = `servicenow:${sysId}:${updatedOn}` const sourceUrl = `${instanceUrl}/incident.do?sys_id=${sysId}` @@ -540,7 +573,7 @@ export const servicenowConnector: ConnectorConfig = { query, }) - const { result, nextOffset } = await serviceNowApiGet( + const { result, nextOffset, totalCount } = await serviceNowApiGet( instanceUrl, tableName, authHeader, @@ -578,12 +611,21 @@ export const servicenowConnector: ConnectorConfig = { const nextCursor = hasMore ? String(nextOffset) : undefined /** - * More records exist past the `maxItems` cap. See the `remaining <= 0` - * branch above — the listing is knowingly incomplete, so reconciliation - * must not treat the untraversed tail as removed at the source. + * Records exist past the `maxItems` cap. See the `remaining <= 0` branch + * above — the listing is knowingly incomplete, so reconciliation must not + * treat the untraversed tail as removed at the source. + * + * A full page landing exactly on the cap is ambiguous: `nextOffset` is set + * whenever a page comes back full, so it cannot distinguish "more rows + * follow" from "the table ended on a page boundary". `X-Total-Count` + * resolves it when present; when the header is absent the ambiguity is + * resolved conservatively (assume truncated), since over-flagging only + * defers a purge whereas under-flagging deletes live documents. */ if (nextOffset !== undefined && !hasMore && syncContext) { - syncContext.listingCapped = true + if (totalCount === undefined || totalCount > maxItems) { + syncContext.listingCapped = true + } } logger.info('Fetched ServiceNow documents', { diff --git a/apps/sim/connectors/youtube/youtube.test.ts b/apps/sim/connectors/youtube/youtube.test.ts index fc8d1981c49..966156fc041 100644 --- a/apps/sim/connectors/youtube/youtube.test.ts +++ b/apps/sim/connectors/youtube/youtube.test.ts @@ -2,20 +2,23 @@ * @vitest-environment node */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { - filterAvailableItems, - type PlaylistItem, - readTrustedVideoItems, - youtubeConnector, -} from '@/connectors/youtube/youtube' +import { youtubeConnector } from '@/connectors/youtube/youtube' const API_KEY = 'test-key' const PLAYLIST_ID = 'PL123' -function item(videoId: string, title = 'A video'): PlaylistItem { +interface TestPlaylistItem { + contentDetails?: { videoId?: string; videoPublishedAt?: string } + snippet?: { title?: string; resourceId?: { videoId?: string } } + status?: { privacyStatus?: string } +} + +/** Builds a playlist item, optionally carrying an explicit `status.privacyStatus`. */ +function item(videoId: string, privacyStatus?: string, title = 'A video'): TestPlaylistItem { return { contentDetails: { videoId, videoPublishedAt: '2024-01-01T00:00:00Z' }, snippet: { title }, + ...(privacyStatus === undefined ? {} : { status: { privacyStatus } }), } } @@ -36,7 +39,7 @@ function fakeResponse({ status = 200, body, text = '' }: FakeResponseInit): Resp } as unknown as Response } -/** Registers a fetch mock that answers by URL prefix, recording every requested URL. */ +/** Registers a fetch mock that answers by URL, recording every requested URL. */ function mockFetch(handler: (url: string) => Response): string[] { const urls: string[] = [] const fetchMock = vi.fn(async (input: unknown) => { @@ -48,18 +51,10 @@ function mockFetch(handler: (url: string) => Response): string[] { return urls } -function playlistPage(items: PlaylistItem[], nextPageToken?: string) { +function playlistPage(items: TestPlaylistItem[], nextPageToken?: string) { return { items, ...(nextPageToken ? { nextPageToken } : {}) } } -function videoListBody(ids: string[], overrides: Record = {}) { - return { - kind: 'youtube#videoListResponse', - items: ids.map((id) => ({ id })), - ...overrides, - } -} - function fullVideo(id: string, duration: string) { return { id, @@ -74,102 +69,18 @@ function fullVideo(id: string, duration: string) { } } -const listDocuments = youtubeConnector.listDocuments -const getDocument = youtubeConnector.getDocument - -describe('filterAvailableItems', () => { - it('keeps items whose video is present in the availability set', () => { - const items = [item('aaa'), item('bbb')] - expect(filterAvailableItems(items, new Set(['aaa', 'bbb']))).toEqual(items) - }) - - it('drops the "Deleted video" placeholder absent from videos.list', () => { - const live = item('aaa') - const deleted = item('bbb', 'Deleted video') - expect(filterAvailableItems([live, deleted], new Set(['aaa']))).toEqual([live]) - }) - - it('drops the "Private video" placeholder absent from videos.list', () => { - const live = item('aaa') - const priv: PlaylistItem = { - contentDetails: { videoId: 'bbb' }, - snippet: { title: 'Private video' }, +/** Answers the playlist page and fails loudly on any unexpected second call. */ +function playlistOnly(items: TestPlaylistItem[], nextPageToken?: string) { + return (url: string): Response => { + if (url.includes('/playlistItems')) { + return fakeResponse({ body: playlistPage(items, nextPageToken) }) } - expect(filterAvailableItems([live, priv], new Set(['aaa']))).toEqual([live]) - }) - - it('falls back to snippet.resourceId.videoId when contentDetails is absent', () => { - const legacy: PlaylistItem = { snippet: { resourceId: { videoId: 'ccc' } } } - expect(filterAvailableItems([legacy], new Set(['ccc']))).toEqual([legacy]) - expect(filterAvailableItems([legacy], new Set(['aaa']))).toEqual([]) - }) - - it('drops items with no resolvable video id', () => { - expect(filterAvailableItems([{ snippet: { title: 'No id' } }], new Set(['aaa']))).toEqual([]) - }) - - it('drops everything when the availability set is empty', () => { - expect(filterAvailableItems([item('aaa'), item('bbb')], new Set())).toEqual([]) - }) - - it('keeps everything when availability is unknown (null)', () => { - const items = [item('aaa'), item('bbb')] - expect(filterAvailableItems(items, null)).toEqual(items) - }) - - it('returns an empty list for an empty input', () => { - expect(filterAvailableItems([], new Set(['aaa']))).toEqual([]) - }) - - it('preserves input order and does not mutate the source array', () => { - const items = [item('aaa'), item('bbb'), item('ccc')] - const result = filterAvailableItems(items, new Set(['ccc', 'aaa'])) - expect(result.map((i) => i.contentDetails?.videoId)).toEqual(['aaa', 'ccc']) - expect(items).toHaveLength(3) - }) -}) - -describe('readTrustedVideoItems', () => { - it('accepts a well-formed response and returns its items', () => { - const items = readTrustedVideoItems(videoListBody(['aaa']), ['aaa', 'bbb']) - expect(items?.map((i) => i.id)).toEqual(['aaa']) - }) - - it('accepts a response with no kind field', () => { - expect(readTrustedVideoItems({ items: [{ id: 'aaa' }] }, ['aaa'])).toHaveLength(1) - }) - - it('rejects a response whose kind is not a video listing', () => { - expect(readTrustedVideoItems(videoListBody(['aaa'], { kind: 'youtube#other' }), ['aaa'])).toBe( - null - ) - }) - - it('rejects a response with a missing or non-array items field', () => { - expect(readTrustedVideoItems({ kind: 'youtube#videoListResponse' }, ['aaa'])).toBe(null) - expect(readTrustedVideoItems({ items: 'nope' }, ['aaa'])).toBe(null) - }) - - it('rejects an empty items array when ids were requested', () => { - expect(readTrustedVideoItems(videoListBody([]), ['aaa'])).toBe(null) - }) - - it('accepts an empty items array when nothing was requested', () => { - expect(readTrustedVideoItems(videoListBody([]), [])).toEqual([]) - }) - - it('rejects entries that are not objects or lack a usable string id', () => { - expect(readTrustedVideoItems({ items: ['aaa'] }, ['aaa'])).toBe(null) - expect(readTrustedVideoItems({ items: [null] }, ['aaa'])).toBe(null) - expect(readTrustedVideoItems({ items: [{ id: 42 }] }, ['aaa'])).toBe(null) - expect(readTrustedVideoItems({ items: [{ id: '' }] }, ['aaa'])).toBe(null) - expect(readTrustedVideoItems({ items: [{ snippet: {} }] }, ['aaa'])).toBe(null) - }) + throw new Error(`unexpected request: ${url}`) + } +} - it('rejects a response containing an id that was never requested', () => { - expect(readTrustedVideoItems(videoListBody(['zzz']), ['aaa'])).toBe(null) - }) -}) +const listDocuments = youtubeConnector.listDocuments +const getDocument = youtubeConnector.getDocument describe('youtubeConnector.listDocuments', () => { beforeEach(() => { @@ -180,115 +91,85 @@ describe('youtubeConnector.listDocuments', () => { vi.unstubAllGlobals() }) - it('resolves availability with a single id-only videos.list call and drops absent videos', async () => { - const urls = mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb', 'Deleted video')]) }) - : fakeResponse({ body: videoListBody(['aaa']) }) - ) + it('requests the status part and makes no extra videos.list call', async () => { + const urls = mockFetch(playlistOnly([item('aaa', 'public'), item('bbb', 'unlisted')])) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) - expect(result.hasMore).toBe(false) - expect(urls).toHaveLength(2) - expect(urls[1]).toContain('/videos?part=id&id=aaa%2Cbbb') - expect(urls[1]).toContain('key=test-key') - }) - - it('never zeroes out a page: an availability response omitting every id is untrusted', async () => { - const urls = mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) - : fakeResponse({ body: videoListBody([]) }) - ) - - const syncContext: Record = {} - const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }, undefined, syncContext) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) - expect(result.hasMore).toBe(true) - expect(result.nextCursor).toBe('TOKEN2') - expect(urls[1]).toContain('/videos?part=id') - expect(syncContext.listingTruncated).toBeUndefined() + expect(urls).toHaveLength(1) + expect(urls[0]).toContain('part=snippet%2CcontentDetails%2Cstatus') + expect(urls[0]).toContain('/playlistItems') }) - it('advances pagination on the real cursor even when most of a page is dropped', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ - body: playlistPage([item('aaa'), item('bbb'), item('ccc')], 'TOKEN2'), - }) - : fakeResponse({ body: videoListBody(['aaa']) }) + it('excludes only items whose privacyStatus is explicitly private', async () => { + mockFetch( + playlistOnly([ + item('aaa', 'public'), + item('bbb', 'private', 'Deleted video'), + item('ccc', 'unlisted'), + ]) ) - const syncContext: Record = {} - const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }, undefined, syncContext) + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) - expect(result.hasMore).toBe(true) - expect(result.nextCursor).toBe('TOKEN2') - expect(syncContext.totalDocsFetched).toBe(1) + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'ccc']) }) - it('fails open and keeps every item when videos.list returns an empty items array', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) - : fakeResponse({ body: videoListBody([]) }) - ) + it('keeps an item when the status part is missing entirely', async () => { + mockFetch(playlistOnly([item('aaa'), item('bbb')])) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) }) - it('fails open when videos.list omits the items field entirely', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) - : fakeResponse({ body: { kind: 'youtube#videoListResponse' } }) + it('keeps an item when privacyStatus is empty or unrecognized', async () => { + mockFetch( + playlistOnly([ + item('aaa', ''), + item('bbb', 'privacyStatusUnspecified'), + item('ccc', 'PRIVATE'), + ]) ) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb', 'ccc']) }) - it('fails open when videos.list returns an unexpected kind', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) - : fakeResponse({ body: videoListBody(['aaa'], { kind: 'youtube#searchListResponse' }) }) - ) + it('keeps an item whose title looks like a placeholder but whose status is public', async () => { + mockFetch(playlistOnly([item('aaa', 'public', 'Deleted video')])) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) }) - it('fails open when videos.list returns an id that was not requested', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')]) }) - : fakeResponse({ body: videoListBody(['aaa', 'unrelated']) }) + it('falls back to snippet.resourceId.videoId and drops items with no video id', async () => { + mockFetch( + playlistOnly([ + { snippet: { resourceId: { videoId: 'ccc' } } }, + { snippet: { title: 'No id' } }, + ]) ) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) - expect(result.documents.map((d) => d.externalId)).toEqual(['aaa', 'bbb']) + expect(result.documents.map((d) => d.externalId)).toEqual(['ccc']) }) - it('throws (aborting the sync, which deletes nothing) when videos.list returns 403', async () => { - mockFetch((url) => - url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa')]) }) - : fakeResponse({ status: 403, text: 'quotaExceeded' }) - ) + it('advances pagination on the real cursor even when every item on a page is private', async () => { + mockFetch(playlistOnly([item('aaa', 'private'), item('bbb', 'private')], 'TOKEN2')) - await expect(listDocuments(API_KEY, { playlistId: PLAYLIST_ID })).rejects.toThrow( - 'Failed to batch-fetch YouTube videos: 403' - ) + const syncContext: Record = {} + const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }, undefined, syncContext) + + expect(result.documents).toEqual([]) + expect(result.hasMore).toBe(true) + expect(result.nextCursor).toBe('TOKEN2') + expect(syncContext.listingCapped).toBeUndefined() + expect(syncContext.listingTruncated).toBeUndefined() }) it('throws when the playlistItems listing itself fails', async () => { @@ -299,8 +180,8 @@ describe('youtubeConnector.listDocuments', () => { ) }) - it('makes no videos.list call when the page has no usable items', async () => { - const urls = mockFetch(() => fakeResponse({ body: playlistPage([]) })) + it('emits nothing and makes no videos.list call for an empty page', async () => { + const urls = mockFetch(playlistOnly([])) const result = await listDocuments(API_KEY, { playlistId: PLAYLIST_ID }) @@ -311,7 +192,13 @@ describe('youtubeConnector.listDocuments', () => { it('hydrates full documents and drops Shorts when excludeShorts is on', async () => { const urls = mockFetch((url) => url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb'), item('ccc')]) }) + ? fakeResponse({ + body: playlistPage([ + item('aaa', 'public'), + item('bbb', 'public'), + item('ccc', 'public'), + ]), + }) : fakeResponse({ body: { kind: 'youtube#videoListResponse', @@ -330,15 +217,12 @@ describe('youtubeConnector.listDocuments', () => { expect(urls[1]).toContain('/videos?part=snippet%2CcontentDetails%2Cstatus') }) - it('advances pagination when every video on a page is an excluded Short', async () => { - mockFetch((url) => + it('drops private items before the excludeShorts videos.list call is even made', async () => { + const urls = mockFetch((url) => url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) + ? fakeResponse({ body: playlistPage([item('aaa', 'public'), item('bbb', 'private')]) }) : fakeResponse({ - body: { - kind: 'youtube#videoListResponse', - items: [fullVideo('aaa', 'PT20S'), fullVideo('bbb', 'PT10S')], - }, + body: { kind: 'youtube#videoListResponse', items: [fullVideo('aaa', 'PT5M')] }, }) ) @@ -347,16 +231,18 @@ describe('youtubeConnector.listDocuments', () => { excludeShorts: 'true', }) - expect(result.documents).toEqual([]) - expect(result.hasMore).toBe(true) - expect(result.nextCursor).toBe('TOKEN2') + expect(result.documents.map((d) => d.externalId)).toEqual(['aaa']) + expect(urls[1]).toContain('id=aaa&') + expect(urls[1]).not.toContain('bbb') }) it('blocks reconciliation instead of dropping a page when the shorts lookup is untrusted', async () => { mockFetch((url) => url.includes('/playlistItems') - ? fakeResponse({ body: playlistPage([item('aaa'), item('bbb')], 'TOKEN2') }) - : fakeResponse({ body: videoListBody([]) }) + ? fakeResponse({ + body: playlistPage([item('aaa', 'public'), item('bbb', 'public')], 'TOKEN2'), + }) + : fakeResponse({ body: { kind: 'youtube#videoListResponse', items: [] } }) ) const syncContext: Record = {} @@ -373,6 +259,18 @@ describe('youtubeConnector.listDocuments', () => { expect(result.hasMore).toBe(true) expect(result.nextCursor).toBe('TOKEN2') }) + + it('throws (aborting the sync, which deletes nothing) when the shorts videos.list 403s', async () => { + mockFetch((url) => + url.includes('/playlistItems') + ? fakeResponse({ body: playlistPage([item('aaa', 'public')]) }) + : fakeResponse({ status: 403, text: 'quotaExceeded' }) + ) + + await expect( + listDocuments(API_KEY, { playlistId: PLAYLIST_ID, excludeShorts: 'true' }) + ).rejects.toThrow('Failed to batch-fetch YouTube videos: 403') + }) }) describe('youtubeConnector.getDocument', () => { diff --git a/apps/sim/connectors/youtube/youtube.ts b/apps/sim/connectors/youtube/youtube.ts index 201e321b9dc..b80ed9117a2 100644 --- a/apps/sim/connectors/youtube/youtube.ts +++ b/apps/sim/connectors/youtube/youtube.ts @@ -25,7 +25,7 @@ const SHORTS_MAX_DURATION_SECONDS = 60 * These differ for hand-curated playlists, so only `videoPublishedAt` is used for the * change-detection hash (it matches `videos.list` `snippet.publishedAt`). */ -export interface PlaylistItem { +interface PlaylistItem { contentDetails?: { videoId?: string; videoPublishedAt?: string } snippet?: { title?: string @@ -34,6 +34,7 @@ export interface PlaylistItem { videoOwnerChannelTitle?: string resourceId?: { videoId?: string } } + status?: { privacyStatus?: string } } /** @@ -217,11 +218,9 @@ function itemToStub(item: PlaylistItem): ExternalDocument | null { } /** - * `videos.list` part sets used by this connector. The full set hydrates a document; the - * id-only set is the cheapest way to ask "do these videos still exist?". + * `videos.list` part set used to hydrate a full document. */ const VIDEO_PART_FULL = 'snippet,contentDetails,status' -const VIDEO_PART_ID_ONLY = 'id' /** Documented `kind` of a `videos.list` response body. */ const VIDEO_LIST_KIND = 'youtube#videoListResponse' @@ -252,12 +251,11 @@ interface VideoListResponse { async function fetchVideoList( apiKey: string, videoIds: string[], - part: string, retryOptions?: Parameters[2] ): Promise { - const url = `${YOUTUBE_API_BASE}/videos?part=${encodeURIComponent(part)}&id=${encodeURIComponent( - videoIds.join(',') - )}&key=${encodeURIComponent(apiKey)}` + const url = `${YOUTUBE_API_BASE}/videos?part=${encodeURIComponent( + VIDEO_PART_FULL + )}&id=${encodeURIComponent(videoIds.join(','))}&key=${encodeURIComponent(apiKey)}` const response = await fetchWithRetry( url, @@ -269,7 +267,6 @@ async function fetchVideoList( const errorText = await response.text().catch(() => '') logger.error('Failed to batch-fetch YouTube videos', { count: videoIds.length, - part, status: response.status, error: errorText.slice(0, 500), }) @@ -299,7 +296,7 @@ async function fetchVideoList( * partial response as untrusted is impossible without a completeness guarantee the API * does not provide. */ -export function readTrustedVideoItems( +function readTrustedVideoItems( data: VideoListResponse, requestedIds: readonly string[] ): VideoItem[] | null { @@ -325,8 +322,10 @@ export function readTrustedVideoItems( * Batch-fetches full video resources for the given IDs, keyed by video ID. * * Returns null when the response is untrusted (see `readTrustedVideoItems`) so the caller - * can fail open instead of treating every ID as gone. Videos that are private, deleted, - * or region-blocked are absent from a trusted response. + * can fail open instead of treating every ID as gone. Videos that are private or deleted + * are absent from a trusted response; region-restricted videos are NOT — an ID-filtered + * `videos.list` returns them normally, carrying their + * `contentDetails.regionRestriction.allowed` / `.blocked` country lists. */ async function fetchVideosByIds( apiKey: string, @@ -336,7 +335,7 @@ async function fetchVideosByIds( const result = new Map() if (videoIds.length === 0) return result - const data = await fetchVideoList(apiKey, videoIds, VIDEO_PART_FULL, retryOptions) + const data = await fetchVideoList(apiKey, videoIds, retryOptions) const items = readTrustedVideoItems(data, videoIds) if (!items) return null @@ -347,60 +346,50 @@ async function fetchVideosByIds( } /** - * Batch-resolves which of the given video IDs still exist, via a lightweight - * `videos.list?part=id` call (flat 1 quota unit, up to 50 IDs per call — a full - * `playlistItems.list` page). - * - * Returns null when the response is untrusted, which callers read as "availability is - * unknown, keep everything". + * The `playlistItem.status.privacyStatus` value that marks the referenced video as no + * longer retrievable. The YouTube Data API v3 discovery document defines + * `PlaylistItemStatus.privacyStatus` as a closed enum of exactly `public`, `unlisted`, + * and `private` — there is no `privacyStatusUnspecified` member — and documents the field + * as the privacy the uploading channel set on the video via `videos.insert`/`videos.update`. + * `private` is therefore an explicit, positive, API-asserted statement about the video, + * not an inference. */ -async function fetchAvailableVideoIds( - apiKey: string, - videoIds: string[], - retryOptions?: Parameters[2] -): Promise | null> { - if (videoIds.length === 0) return new Set() - - const data = await fetchVideoList(apiKey, videoIds, VIDEO_PART_ID_ONLY, retryOptions) - const items = readTrustedVideoItems(data, videoIds) - if (!items) return null - - const result = new Set() - for (const item of items) { - if (item.id) result.add(item.id) - } - return result -} +const PLAYLIST_ITEM_PRIVATE_STATUS = 'private' /** - * Keeps only playlist items whose video is still available. + * Decides whether a playlist item's referenced video is explicitly gone. * * A playlist item is a resource independent of the video it points at: when the video is - * deleted or made private, `playlistItems.list` keeps returning a placeholder item - * ("Deleted video" / "Private video") with the same `contentDetails.videoId`, and the API - * exposes no request parameter to exclude them. Left in the listing, those placeholders - * keep the stored document's externalId present on every full sync, so deletion - * reconciliation (which hard-deletes only documents ABSENT from the listing) never purges - * it — and hydration cannot clean it up either, since `videos.list` omits the video and - * `getDocument` returns null, which the sync engine treats as last-known-good. + * deleted or made private, `playlistItems.list` keeps returning a placeholder item with + * the same `contentDetails.videoId`, and the API exposes no request parameter to exclude + * them. Left in the listing, those placeholders keep the stored document's externalId + * present on every full sync, so deletion reconciliation (which hard-deletes only + * documents ABSENT from the listing) never purges it — and hydration cannot clean it up + * either, since `videos.list` omits the video and `getDocument` returns null, which the + * sync engine treats as last-known-good. + * + * Exclusion is therefore driven ONLY by the explicit `status.privacyStatus` field on the + * `status` part of the same `playlistItems.list` call (a `list` call costs a flat 1 quota + * unit regardless of how many parts it requests, so this signal is free). An item is + * dropped only when that field is exactly the documented value `private`. A missing, + * empty, or unrecognized value keeps the item, as do `public` and `unlisted` — matching + * `videoToDocument`, which already refuses to hydrate anything outside those two. * - * Availability is therefore established positively, from a `videos.list` membership check: - * an item is dropped only when its video ID is explicitly absent from a trusted response. - * A null set means the membership check could not be trusted, and every item is kept — - * keeping a stale placeholder is recoverable, hard-deleting a live document (and its - * `userExcluded` flag) is not. + * Signals deliberately NOT used: the `snippet.title`/`snippet.description` placeholders + * ("Deleted video" / "This video is unavailable") are undocumented and localized, so + * matching them would silently stop purging for non-English callers and could match a + * real video so titled; `contentDetails.videoPublishedAt` absence is likewise + * undocumented; and absence from a `videos.list` response is an inference from silence, + * which would turn any partially-degraded 200 into a hard delete of live documents. */ -export function filterAvailableItems( - items: T[], - availableVideoIds: ReadonlySet | null -): T[] { - if (!availableVideoIds) return items - return items.filter((item) => availableVideoIds.has(getVideoId(item))) +function isPlaylistItemPrivate(item: PlaylistItem): boolean { + return item.status?.privacyStatus === PLAYLIST_ITEM_PRIVATE_STATUS } /** * Builds the full document for a video, combining title and description as plain-text - * content. Returns null for unlisted/private/deleted videos, and (when configured) for + * content. Returns null for videos whose `privacyStatus` is neither `public` nor + * `unlisted`, and (when configured) for * Shorts shorter than 60 seconds. * * Captions/transcripts are intentionally not fetched: `captions.download` requires OAuth @@ -489,8 +478,13 @@ export const youtubeConnector: ConnectorConfig = { const remaining = maxVideos > 0 ? maxVideos - previouslyFetched : 0 const effectivePageSize = maxVideos > 0 ? Math.min(PAGE_SIZE, remaining) : PAGE_SIZE + /** + * `status` is requested so `isPlaylistItemPrivate` has an explicit signal to read. A + * `list` call costs a flat 1 quota unit regardless of the number of parts, so this + * adds no quota and no extra request. + */ const queryParams = new URLSearchParams({ - part: 'snippet,contentDetails', + part: 'snippet,contentDetails,status', playlistId, maxResults: String(effectivePageSize), key: apiKey, @@ -526,6 +520,8 @@ export const youtubeConnector: ConnectorConfig = { for (const item of items) { if (!getVideoId(item)) continue + if (isPlaylistItemPrivate(item)) continue + if (publishedAfter != null) { const videoPublishedAt = item.contentDetails?.videoPublishedAt const ms = videoPublishedAt ? new Date(videoPublishedAt).getTime() : Number.NaN @@ -573,8 +569,12 @@ export const youtubeConnector: ConnectorConfig = { } else { for (const item of keptItems) { /** - * Absent from a trusted `videos.list` => private/deleted/region-blocked. Drop it - * instead of emitting a stub that would re-hydrate to null on every sync. + * This branch cannot emit a document without the hydrated video, since the + * Shorts decision needs `contentDetails.duration`. An item absent from a trusted + * `videos.list` is skipped because there is nothing to build from — emitting a + * stub instead would re-hydrate to null on every sync. Deletion is never + * inferred from that absence: items whose video is explicitly gone were already + * removed above by `isPlaylistItemPrivate`. */ const video = videoMap.get(getVideoId(item)) if (!video) continue @@ -582,22 +582,8 @@ export const youtubeConnector: ConnectorConfig = { if (doc) documents.push(doc) } } - } else if (keptItems.length > 0) { - /** - * Placeholder items for deleted/private videos stay in `playlistItems.list` forever - * and would pin the stored document against deletion reconciliation. Resolve which - * videos still exist with one batched `videos.list?part=id` call (1 quota unit per - * page) and emit stubs only for those — mirroring the drop the excludeShorts branch - * above already performs. An untrusted response keeps every item (fail open). - */ - const availableVideoIds = await fetchAvailableVideoIds(apiKey, keptItems.map(getVideoId)) - if (!availableVideoIds) { - logger.warn('Untrusted videos.list response; keeping all playlist items for this page', { - playlistId, - count: keptItems.length, - }) - } - for (const item of filterAvailableItems(keptItems, availableVideoIds)) { + } else { + for (const item of keptItems) { const stub = itemToStub(item) if (stub) documents.push(stub) } @@ -615,9 +601,9 @@ export const youtubeConnector: ConnectorConfig = { /** * Pagination is driven exclusively by the `playlistItems.list` cursor, never by how - * many documents survived availability filtering. A page whose items are ALL - * unavailable therefore emits zero documents while still advancing to the next page - * and reporting `hasMore` truthfully, so it can neither wedge nor truncate the sync. + * many documents survived filtering. A page whose items are ALL private therefore + * emits zero documents while still advancing to the next page and reporting `hasMore` + * truthfully, so it can neither wedge nor truncate the sync. */ const nextPageToken = data.nextPageToken as string | undefined @@ -664,7 +650,8 @@ export const youtubeConnector: ConnectorConfig = { const items = (data.items ?? []) as VideoItem[] const video = items[0] - // An empty items array means the video is deleted, private, or region-blocked. + // An empty items array means the video is deleted or private. Region-restricted + // videos are still returned here, with contentDetails.regionRestriction populated. if (!video) return null return videoToDocument(video, excludeShorts)