diff --git a/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx b/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx index 421eea9354e..c4f9f04103e 100644 --- a/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx +++ b/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx @@ -100,7 +100,11 @@ export function Person({ organizationId, memberId }: PersonProps) { const [editing, setEditing] = useState(false) const routes = organizationRoutes(organizationId) const originQuery = serializePeopleView({ ...origin, view: origin.view ?? 'directory' }) - const data = query.isError ? undefined : query.data + const notFound = isApiClientError(query.error) && query.error.status === 404 + const refused = isApiClientError(query.error) && query.error.status < 500 + // A transient refresh failure keeps the loaded profile and any edit open on it; a refusal does not. + const loadError = query.data && !refused ? null : query.error + const data = loadError ? undefined : query.data const chartHref = serializePeopleView(routes.people, { ...origin, @@ -129,14 +133,14 @@ export function Person({ organizationId, memberId }: PersonProps) { ) : undefined } > - {query.error ? ( - isApiClientError(query.error) && query.error.status === 404 ? ( + {loadError ? ( + notFound ? ( This person isn't in your organization. ) : ( void query.refetch()} diff --git a/apps/sim/app/o/[organizationId]/people/directory-sync-form.tsx b/apps/sim/app/o/[organizationId]/people/directory-sync-form.tsx index bb8af032fc0..9d68645cd42 100644 --- a/apps/sim/app/o/[organizationId]/people/directory-sync-form.tsx +++ b/apps/sim/app/o/[organizationId]/people/directory-sync-form.tsx @@ -16,6 +16,7 @@ import { type PeopleSyncStatus, peopleSyncConfigSchema, } from '@/lib/api/contracts/people-sync' +import { flattenFieldErrors } from '@/lib/api/contracts/primitives' import { organizationRoutes } from '@/lib/navigation/paths' import { useOrganizationContext } from '@/app/o/[organizationId]/providers/organization-provider' import { useScimConnection } from '@/ee/scim/hooks/scim' @@ -45,7 +46,9 @@ export function DirectorySyncForm({ const [tenantId, setTenantId] = useState(current?.tenantId ?? '') const [clientId, setClientId] = useState(current?.clientId ?? '') const [secret, setSecret] = useState('') - const [fieldErrors, setFieldErrors] = useState>({}) + const [fieldErrors, setFieldErrors] = useState< + Partial> + >({}) const { settingsFeatures } = useOrganizationContext() const provisioning = useScimConnection(organizationId, source === 'scim') const scimReady = provisioning.isSuccess && provisioning.data.connection?.status === 'active' @@ -66,11 +69,7 @@ export function DirectorySyncForm({ : { source } ) if (!config.success) { - setFieldErrors( - Object.fromEntries( - config.error.issues.map((issue) => [String(issue.path[0]), issue.message]) - ) - ) + setFieldErrors(flattenFieldErrors(config.error)) return } setFieldErrors({}) diff --git a/apps/sim/app/o/[organizationId]/people/people-chart.tsx b/apps/sim/app/o/[organizationId]/people/people-chart.tsx index dc42af40249..d8a07255ebf 100644 --- a/apps/sim/app/o/[organizationId]/people/people-chart.tsx +++ b/apps/sim/app/o/[organizationId]/people/people-chart.tsx @@ -102,7 +102,10 @@ export function PeopleChart({ organizationId, viewSwitch, onImport, onSync }: Pe const { focus, page } = origin const size = useIsMobile() ? 1 : 4 const chart = usePeopleChart(organizationId, focus, Math.max(1, Math.min(page, 1_000_000)), size) - const branch = chart.isError ? undefined : chart.data + const notFound = isApiClientError(chart.error) && chart.error.status === 404 + const refused = isApiClientError(chart.error) && chart.error.status < 500 + // A transient refresh failure keeps the chart on screen; a refusal replaces it. + const branch = refused ? undefined : chart.data const profileQuery = serializePeopleView({ ...origin, view: 'chart', @@ -264,7 +267,7 @@ export function PeopleChart({ organizationId, viewSwitch, onImport, onSync }: Pe ) : chart.error ? ( - isApiClientError(chart.error) && chart.error.status === 404 ? ( + notFound ? ( router.push(profileHref(memberId))} onLoadMore={() => void people.fetchNextPage()} - hasMore={people.hasNextPage} + hasMore={people.hasNextPage && !changingFilter} isLoadingMore={people.isFetchingNextPage} isFetching={people.isFetching} loadMoreFailed={people.isFetchNextPageError} diff --git a/apps/sim/app/o/[organizationId]/search/search.tsx b/apps/sim/app/o/[organizationId]/search/search.tsx index 5dce1e9991e..1f36e12bf8f 100644 --- a/apps/sim/app/o/[organizationId]/search/search.tsx +++ b/apps/sim/app/o/[organizationId]/search/search.tsx @@ -167,7 +167,7 @@ function OrganizationSearchContent({ userId, chatId }: OrganizationSearchProps) const { recordQuery } = useSearchHistoryActions() const router = useRouter() const pathname = usePathname() - const hasSearchParams = useSearchParams().size > 0 + const hasSearchParams = useSearchParams().toString() !== '' const routes = organizationRoutes(organization.id) const hasConversation = Boolean(chatId) || pathname.startsWith(`${routes.search}/chat/`) const queryClient = useQueryClient() diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/resolve-citations.test.ts b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/resolve-citations.test.ts index 05bf07b7752..8adbaf567e5 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/message-content/resolve-citations.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/components/message-content/resolve-citations.test.ts @@ -160,6 +160,13 @@ describe('evidence-linked citations', () => { true ) expect(persisted.sources).toEqual(live.sources) + const disguised = { + ...webOutput, + results: [{ ...webOutput.results[0], url: 'https://docs.example.test@attacker.test/cli' }], + } + expect( + resolveMessageCitations(webBlocks('success', true, disguised), '', true).sources + ).toEqual([]) expect( resolveMessageCitations(webBlocks('error', false, webOutput), '', true).blocks[1].content ).toBe('Install it ') diff --git a/apps/sim/lib/mcp/utils.ts b/apps/sim/lib/mcp/utils.ts index a7dcbab1ffd..6f484ff3dd0 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -189,8 +189,8 @@ export function categorizeMcpServerError( } /** - * Enhanced error categorization for more specific HTTP status codes. - * Returns safe, generic messages to prevent leaking internal details. + * HTTP status and safe, generic message for any error: Sim's own MCP error classes + * first, then message matching for SDK and third-party errors. */ export function categorizeError(error: unknown): { message: string; status: number } { if (!(error instanceof Error)) { diff --git a/apps/sim/lib/mothership/chat/citation-evidence.ts b/apps/sim/lib/mothership/chat/citation-evidence.ts index e3e5d2593c2..ce7a48399cf 100644 --- a/apps/sim/lib/mothership/chat/citation-evidence.ts +++ b/apps/sim/lib/mothership/chat/citation-evidence.ts @@ -14,8 +14,8 @@ type RetrievalCitationSource = { snippet?: string } -/** Tools whose successful results supply sources: connected-source retrieval and public web research. */ const WEB_TOOLS = new Set(['web_search', 'web_fetch']) +/** Tools whose successful results supply sources: connected-source retrieval and public web research. */ const CITATION_TOOLS = new Set(['search_workspace', 'read_document', ...WEB_TOOLS]) /** Whether a successful result of this tool may supply source destinations. */ @@ -80,7 +80,9 @@ export function collectRetrievalCitationEvidence(blocks: readonly RetrievalCitat continue try { const url = new URL(result.citationUrl) + // A web page can steer a fetched URL, so `https://trusted.example@other.example` never cites. if (url.protocol !== 'https:' && url.protocol !== 'http:') continue + if (url.username || url.password) continue } catch { continue } diff --git a/apps/sim/lib/people/sync/entra.ts b/apps/sim/lib/people/sync/entra.ts index b9f76171d00..12ca3af7038 100644 --- a/apps/sim/lib/people/sync/entra.ts +++ b/apps/sim/lib/people/sync/entra.ts @@ -44,6 +44,15 @@ const reportsResponse = z.object({ '@odata.nextLink': z.string().min(1).max(MAX_NEXT_LINK_LENGTH).optional(), }) +const deletedUserResponse = z.object({ + error: z.object({ code: z.literal('Request_ResourceNotFound') }), +}) + +/** A user deleted after the users phase has no reports left to read; any other 404 still fails. */ +function isDeletedUser(status: number, body: unknown) { + return status === 404 && deletedUserResponse.safeParse(body).success +} + /** App-only Graph tokens cannot read /manager; each user's /directReports supports User.Read.All. */ async function readReportPages( items: readonly DirectoryReportRequest[], @@ -101,6 +110,8 @@ async function readReportPages( const timing = { get: (name: string) => headers.get(name) ?? null } const retryAfterMs = directoryRetryDelay(response.status, timing) if (retryAfterMs !== null) return { managerId: page.managerId, retryAfterMs } + if (isDeletedUser(response.status, response.body)) + return { managerId: page.managerId, reportIds: [], next: null } requireDirectorySuccess(response.status, timing) const parsedReports = reportsResponse.safeParse(response.body) if (!parsedReports.success) diff --git a/apps/sim/lib/people/sync/large-directory.integration.ts b/apps/sim/lib/people/sync/large-directory.integration.ts index a2474e327e2..9ef708f4269 100644 --- a/apps/sim/lib/people/sync/large-directory.integration.ts +++ b/apps/sim/lib/people/sync/large-directory.integration.ts @@ -312,7 +312,7 @@ it('rotates credentials without bypassing a provider cooldown or publishing the expect(rows).toEqual([{ id: director, title: 'President' }]) }) -it('resumes a large Entra hierarchy without replaying successful batch siblings or creating accounts', async () => { +it('resumes a large Entra hierarchy past a user deleted mid-read without replaying successful batch siblings or creating accounts', async () => { const total = 10007 const tenantId = generateId() const graphConfig = { @@ -378,6 +378,12 @@ it('resumes a large Entra hierarchy without replaying successful batch siblings body: { error: graphConfig.clientSecret }, } } + if (managerId === employee(2)) + return { + id: request.id, + status: 404, + body: { error: { code: 'Request_ResourceNotFound' } }, + } if (managerId !== lead) return { id: request.id, status: 200, body: { value: [] } } const offset = Number(target.searchParams.get('$skiptoken') || 0) const length = Math.min(1000, total - 1 - offset) diff --git a/apps/sim/lib/people/sync/runner.ts b/apps/sim/lib/people/sync/runner.ts index 855f734b68d..85a94157189 100644 --- a/apps/sim/lib/people/sync/runner.ts +++ b/apps/sim/lib/people/sync/runner.ts @@ -2,6 +2,7 @@ import { db } from '@sim/db' import { peopleDirectoryScimChange as change, peopleDirectorySync as sync } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getPostgresErrorCode } from '@sim/utils/errors' +import { chunkArray } from '@sim/utils/helpers' import { generateId } from '@sim/utils/id' import { backoffWithJitter } from '@sim/utils/retry' import { and, eq, isNotNull, isNull, lte, or, sql } from 'drizzle-orm' @@ -40,7 +41,7 @@ export const CLEARED_RUN = { cursor: null, usersRead: 0, } as const -const RELEASED = { leaseToken: null, leaseExpiresAt: null } as const +export const RELEASED = { leaseToken: null, leaseExpiresAt: null } as const const available = (at: Date) => or(isNull(sync.leaseToken), lte(sync.leaseExpiresAt, at)) const due = (at: Date) => or(lte(sync.nextSyncAt, at), isNotNull(pendingScimChange)) type Connection = typeof sync.$inferSelect @@ -78,11 +79,11 @@ const ownsLease = (worker: Worker) => /** Every page and its next cursor commit together, after checking that the same run still owns the source. */ async function checkpoint( worker: Worker, - step: 'page' | 'publish', - write: (tx: DbTransaction, row: Checkpoint) => Promise + write: (tx: DbTransaction, row: Checkpoint) => Promise, + options: { lockProfiles?: boolean } = {} ) { return db.transaction(async (tx) => { - if (step === 'publish') await lockProfiles(tx, worker.organizationId) + if (options.lockProfiles) await lockProfiles(tx, worker.organizationId) const [row] = await tx .select(checkpointColumns) .from(sync) @@ -127,7 +128,7 @@ function retryUpdate(row: Checkpoint, providerDelay: number, reason: string) { async function checkpointUsers(worker: Worker, reader: DirectoryReader, cursor: string | null) { const page = await reader.users(cursor) - return checkpoint(worker, 'page', async (tx) => { + return checkpoint(worker, async (tx) => { await stageUsers(tx, worker, cursor, page.people) await tx .update(sync) @@ -147,7 +148,7 @@ async function checkpointReports(worker: Worker, reader: DirectoryReader) { throw new OrchestrationError('validation', 'The directory cannot read reporting lines.') const items = await pendingManagers(db, worker) if (!items.length) { - return checkpoint(worker, 'page', async (tx) => { + return checkpoint(worker, async (tx) => { await tx.update(sync).set({ phase: 'publish' }).where(ownsLease(worker)) }) } @@ -168,7 +169,7 @@ async function checkpointReports(worker: Worker, reader: DirectoryReader) { const delays = results.flatMap((result) => 'retryAfterMs' in result ? [result.retryAfterMs] : [] ) - const applied = await checkpoint(worker, 'page', async (tx, row) => { + const applied = await checkpoint(worker, async (tx, row) => { await stageRelationships(tx, worker, pages) const update = delays.length ? retryUpdate( @@ -184,12 +185,16 @@ async function checkpointReports(worker: Worker, reader: DirectoryReader) { /** A local SCIM generation captures, validates, and publishes atomically. New provisioning changes have their own marker. */ async function runScim(worker: Worker) { - await checkpoint(worker, 'publish', async (tx, row) => { - await discardStage(tx, row.organizationId) - const count = await stageScimDirectory(tx, row.organizationId, worker.runId, row.sourceTarget) - await tx.update(sync).set({ phase: 'publish', usersRead: count }).where(ownsLease(worker)) - await publish(tx, worker, 'scim') - }) + await checkpoint( + worker, + async (tx, row) => { + await discardStage(tx, row.organizationId) + const count = await stageScimDirectory(tx, row.organizationId, worker.runId, row.sourceTarget) + await tx.update(sync).set({ phase: 'publish', usersRead: count }).where(ownsLease(worker)) + await publish(tx, worker, 'scim') + }, + { lockProfiles: true } + ) } async function handleFailure(worker: Worker, error: unknown) { @@ -203,7 +208,7 @@ async function handleFailure(worker: Worker, error: unknown) { retry || (error instanceof OrchestrationError && error.code !== 'internal') ? error.message : 'Directory sync was interrupted. Sim will retry automatically.' - const applied = await checkpoint(worker, 'page', async (tx, row) => { + const applied = await checkpoint(worker, async (tx, row) => { if (temporary) { await tx .update(sync) @@ -297,7 +302,7 @@ export async function runDirectorySync(organizationId: string, trigger: keyof ty if (!current) return true await requirePeopleEnabled(organizationId) if (current.phase === 'publish') { - await checkpoint(worker, 'publish', (tx) => publish(tx, worker, row.source)) + await checkpoint(worker, (tx) => publish(tx, worker, row.source), { lockProfiles: true }) return true } reader ??= await connectReader(row) @@ -340,8 +345,8 @@ export async function syncDueDirectories() { SELECT organization_id AS "organizationId" FROM candidates GROUP BY organization_id ORDER BY min(due_at), organization_id LIMIT 50 `) - for (let i = 0; i < connections.length && Date.now() < deadline; i += 2) { - const slice = connections.slice(i, i + 2) + for (const slice of chunkArray(connections, 2)) { + if (Date.now() >= deadline) break const results = await Promise.allSettled( slice.map(({ organizationId }) => runDirectorySync(organizationId, 'cron')) ) diff --git a/apps/sim/lib/people/sync/service.ts b/apps/sim/lib/people/sync/service.ts index a89edfe9c68..de4e4ee8fcf 100644 --- a/apps/sim/lib/people/sync/service.ts +++ b/apps/sim/lib/people/sync/service.ts @@ -10,7 +10,7 @@ import { OrchestrationError } from '@/lib/core/orchestration/types' import { encryptSecret } from '@/lib/core/security/encryption' import { oktaDirectoryHost } from '@/lib/people/provider-import' import { lockProfiles } from '@/lib/people/repository' -import { CLEARED_RUN, runDirectorySync } from '@/lib/people/sync/runner' +import { CLEARED_RUN, RELEASED, runDirectorySync } from '@/lib/people/sync/runner' import { pendingScimChange } from '@/lib/people/sync/scim-changes' import { discardStage } from '@/lib/people/sync/stage' import { isScimEntitledForOrganization } from '@/ee/scim/lib/entitlement' @@ -132,10 +132,9 @@ export async function configureDirectorySync( existing && existing.retryCount > 0 && existing.nextSyncAt.getTime() > Date.now() const values = { ...CLEARED_RUN, + ...RELEASED, ...stored, configuredBy: actorUserId, - leaseToken: null, - leaseExpiresAt: null, nextSyncAt: coolingDown ? existing.nextSyncAt : new Date(), retryCount: coolingDown ? existing.retryCount : 0, lastError: coolingDown ? existing.lastError : null, diff --git a/apps/sim/lib/people/sync/stage.integration.ts b/apps/sim/lib/people/sync/stage.integration.ts index 3547ff7e16f..e6b7504a4c3 100644 --- a/apps/sim/lib/people/sync/stage.integration.ts +++ b/apps/sim/lib/people/sync/stage.integration.ts @@ -99,12 +99,12 @@ it('refuses cyclic components, including cycles outside otherwise publishable tr await expect(db.transaction(publish)).rejects.toThrow(/loop|cycle/i) expect((await published()).map((p) => p.externalId)).toEqual(['root']) }) -it('resumes relationship pages from stored continuations and refuses incomplete or contradictory Graph data', async () => { +it('resumes relationship pages from stored continuations, refuses incomplete Graph data, and keeps the latest manager read', async () => { await db .update(peopleDirectorySync) .set({ source: 'entra' }) .where(eq(peopleDirectorySync.organizationId, run.organizationId)) - await writeUsers(null, [person('lead'), person('worker'), person('other')]) + await writeUsers(null, [person('lead'), person('worker'), person('other'), person('peer')]) await expect(db.transaction((tx) => publishStagedSnapshot(tx, run, 'entra'))).rejects.toThrow( /relationship|report/i ) @@ -123,12 +123,18 @@ it('resumes relationship pages from stored continuations and refuses incomplete cursor: 'continuation', }) await record('lead', 'continuation', ['worker'], null) - await expect(record('other', null, ['worker'], null)).rejects.toThrow(/manager/i) - await record('other', null, [], null) - await record('worker', null, [], null) + await record('other', null, ['worker'], null) + await db.transaction((tx) => + stageRelationships(tx, run, [ + { managerId: 'worker', cursor: null, reportIds: ['other'], nextCursor: null }, + { managerId: 'peer', cursor: null, reportIds: ['other'], nextCursor: null }, + ]) + ) expect(await pendingManagers(db, run)).toEqual([]) await db.transaction((tx) => publishStagedSnapshot(tx, run, 'entra')) - expect((await published()).find((r) => r.externalId === 'worker')?.managerExternalId).toBe('lead') + const managers = new Map((await published()).map((r) => [r.externalId, r.managerExternalId])) + expect(managers.get('worker')).toBe('other') + expect(managers.get('other')).toBe('peer') }) it('refuses to publish a replaced generation', async () => { await writeUsers(null, [person('live')]) diff --git a/apps/sim/lib/people/sync/stage.ts b/apps/sim/lib/people/sync/stage.ts index 545f4fd2c4a..9aa18dc1782 100644 --- a/apps/sim/lib/people/sync/stage.ts +++ b/apps/sim/lib/people/sync/stage.ts @@ -101,7 +101,13 @@ export async function pendingManagers(executor: DbOrTx, run: StageRun) { .limit(GRAPH_BATCH_SIZE) } -/** Commits only completed subrequests, rejecting a cursor replay or a second parent for an active person. */ +/** + * Commits only completed subrequests and rejects a cursor replay. Graph gives a person one + * manager, so a person claimed again was moved during a read that spans many slices: a later + * batch overwrites an earlier one, and publication's cycle check still guards the finished + * graph. Reads inside one batch are concurrent, so a claim repeated there resolves to the + * lowest manager ID, independent of response order. + */ export async function stageRelationships( tx: DbTransaction, run: StageRun, @@ -135,27 +141,17 @@ export async function stageRelationships( 'conflict', 'The directory relationship page no longer matches its checkpoint.' ) - const [result] = await tx.execute<{ conflict: boolean }>(sql` + await tx.execute(sql` WITH claims AS ( SELECT p."managerId" AS manager, jsonb_array_elements_text(p."reportIds") AS report FROM jsonb_to_recordset(${JSON.stringify(pages.map(({ managerId, reportIds }) => ({ managerId, reportIds })))}::text::jsonb) AS p("managerId" text,"reportIds" jsonb) ), assignments AS ( - SELECT s.external_id, min(c.manager) AS manager, count(*) > 1 OR bool_or(s.manager_external_id IS NOT NULL) AS conflict - FROM claims c JOIN ${stage} s ON s.organization_id = ${run.organizationId} AND s.run_id = ${run.runId} AND s.external_id = c.report - GROUP BY s.external_id - ), written AS ( - UPDATE ${stage} s SET manager_external_id = a.manager FROM assignments a - WHERE s.organization_id = ${run.organizationId} AND s.run_id = ${run.runId} AND s.external_id = a.external_id - AND NOT EXISTS (SELECT 1 FROM assignments WHERE conflict) + SELECT c.report AS external_id, min(c.manager) AS manager FROM claims c GROUP BY c.report ) - SELECT EXISTS (SELECT 1 FROM assignments WHERE conflict) AS conflict + UPDATE ${stage} s SET manager_external_id = a.manager FROM assignments a + WHERE s.organization_id = ${run.organizationId} AND s.run_id = ${run.runId} AND s.external_id = a.external_id `) - if (result.conflict) - throw new OrchestrationError( - 'validation', - 'The directory returned multiple manager assignments for the same person. Correct the reporting lines and sync again.' - ) } /** Clears private pages and rows on reset, replacement, or completion with the organization's sync row locked. */