From 020ec68cfeb2cd292f0d4b0406abc7fe793cf251 Mon Sep 17 00:00:00 2001 From: Waleed Date: Tue, 4 Aug 2026 21:55:13 -0700 Subject: [PATCH 01/10] fix(sidebar): keep the collapsed profile chip inside the rail (#6282) --- .../sidebar-footer/sidebar-footer.tsx | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/sidebar-footer/sidebar-footer.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/sidebar-footer/sidebar-footer.tsx index ff0adf795e6..9aa8366f5e1 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/sidebar-footer/sidebar-footer.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/sidebar-footer/sidebar-footer.tsx @@ -151,10 +151,18 @@ export function SidebarFooter({ * so hover highlights only the avatar and name. Collapsed, `fullWidth` fills the * narrow rail instead. Both mirror the workspace header's chip exactly. * - * No `min-w-0`: the label already truncates on its own, and letting the chip - * shrink past its avatar is what let the help button ride onto the photo while - * the rail was still narrow (see {@link SidebarFooter}). Its automatic minimum - * is exactly the icon-only chip, so the avatar holds the same spot at any width. + * No `min-w-0` expanded: the label already truncates on its own, and letting the + * chip shrink past its avatar is what let the help button ride onto the photo + * while the rail was still narrow (see {@link SidebarFooter}). + * + * Collapsed it takes `min-w-0`, because the label stays in the layout there — the + * rail hides it with `opacity`, not `display`, so the fade survives a toggle. Its + * empty box still contributes the content row's gap, putting the chip's automatic + * minimum at 38px against a 35px rail: the chip overflowed, the aside clipped its + * right edge, and the hover fill read as a full-width row bleeding off the rail + * instead of the padded pill every other collapsed chip draws. Floored at zero it + * fills exactly the rail, and the avatar keeps the same 8px offset as the help + * glyph above it. * * The name is the button's accessible name — no `aria-label`, which would * override the visible text. Radix contributes the menu role and expanded state. @@ -167,7 +175,9 @@ export function SidebarFooter({ type='button' data-item-id='profile' className={ - isCollapsed ? chipVariants({ fullWidth: true }) : cn(chipVariants(), 'max-w-full') + isCollapsed + ? cn(chipVariants({ fullWidth: true }), 'min-w-0') + : cn(chipVariants(), 'max-w-full') } > {avatar} From 5718def4fdccfb67e4bf9bc3350cc06c13405538 Mon Sep 17 00:00:00 2001 From: mzxchandra <129460234+mzxchandra@users.noreply.github.com> Date: Tue, 4 Aug 2026 22:27:50 -0700 Subject: [PATCH 02/10] feat(logs): open the workflow from the log details panel (#6275) * refactor(logs): centralize workflow-id resolution and editor path The logs list, its context menu, and the details panel each resolved a log's workflow id with their own copy of `workflow?.id || workflowId`, and the list disagreed with the details panel on what counts as a deleted workflow. Extract `resolveLogWorkflowId` and `workflowEditorPath` so the three surfaces cannot drift. `resolveLogWorkflowId` also returns null for Sim agent jobs, which have no workflow of their own. Only the context menu's "Open Workflow" item adopts that stricter predicate; cancel and retry keep using the previous `hasWorkflow` check so their gating is unchanged. * feat(logs): open the workflow from the log details panel The workflow name in a log's details panel was static text, so the only way to reach the workflow was the row's right-click context menu. Make the label a link to the workflow editor, opening in a new tab so the log list keeps its filters, scroll position, and open panel. On hover or keyboard focus the leading workflow icon morphs into SquareArrowUpRight, reusing the grid-stacked cross-fade already used by the resource header breadcrumb. Sim agent jobs and deleted workflows have no reachable workflow and stay static text. Adds a `group-hover-hover` variant so the morph is gated on a real hover-capable pointer, matching the existing `hover-hover` variant and keeping touch devices out of a half-applied hover state. * refactor(logs): use the existing group-hover variant for the workflow link Drops the group-hover-hover Tailwind variant this branch added and moves the details-panel workflow link to plain group-hover:, matching the variant already used throughout the app. tailwind.config.ts is untouched by the branch again; all colors, radii, and the focus ring come from existing design tokens. --- .../components/log-details/log-details.tsx | 51 ++++++++++++++--- .../log-row-context-menu.tsx | 9 ++- .../app/workspace/[workspaceId]/logs/logs.tsx | 6 +- .../[workspaceId]/logs/utils.test.ts | 55 +++++++++++++++++++ .../app/workspace/[workspaceId]/logs/utils.ts | 23 ++++++++ 5 files changed, 132 insertions(+), 12 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/logs/utils.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/logs/components/log-details/log-details.tsx b/apps/sim/app/workspace/[workspaceId]/logs/components/log-details/log-details.tsx index bde89f51df8..4990fd6609e 100644 --- a/apps/sim/app/workspace/[workspaceId]/logs/components/log-details/log-details.tsx +++ b/apps/sim/app/workspace/[workspaceId]/logs/components/log-details/log-details.tsx @@ -29,11 +29,13 @@ import { ChevronUp, Clipboard, Search, + SquareArrowUpRight, Workflow, Wrench, X, } from '@sim/emcn/icons' import { formatDuration } from '@sim/utils/formatting' +import Link from 'next/link' import { useParams, useRouter } from 'next/navigation' import { useQueryState } from 'nuqs' import { createPortal } from 'react-dom' @@ -59,8 +61,10 @@ import { DELETED_WORKFLOW_LABEL, formatDate, getDisplayStatus, + resolveLogWorkflowId, StatusBadge, TriggerBadge, + workflowEditorPath, } from '@/app/workspace/[workspaceId]/logs/utils' import { useCodeViewerFeatures } from '@/hooks/use-code-viewer' import { usePermissionConfig } from '@/hooks/use-permission-config' @@ -317,6 +321,19 @@ export function LogDetailsContent({ log, onActiveTabChange }: LogDetailsContentP const isWorkflowExecutionLog = (log.trigger === 'manual' && !!log.duration) || !!log.executionData?.traceSpans + /** + * The workflow this run belongs to, when it is still reachable. Null for Sim + * agent jobs and deleted workflows, which render their label as static text. + */ + const openableWorkflowId = resolveLogWorkflowId(log) + + const workflowLabel = + log.trigger === 'mothership' + ? log.jobTitle || 'Untitled Job' + : openableWorkflowId + ? log.workflow?.name || 'Unknown' + : DELETED_WORKFLOW_LABEL + const hasCostInfo = !!(isWorkflowExecutionLog && log.cost) const showWorkflowState = isWorkflowExecutionLog && @@ -465,15 +482,31 @@ export function LogDetailsContent({ log, onActiveTabChange }: LogDetailsContentP {log.trigger === 'mothership' ? 'Job' : 'Workflow'} -
- - - {log.trigger === 'mothership' - ? log.jobTitle || 'Untitled Job' - : log.workflow?.name || - (!log.workflowId ? DELETED_WORKFLOW_LABEL : 'Unknown')} - -
+ {openableWorkflowId ? ( + + + + + + + {workflowLabel} + + (opens in a new tab) + + ) : ( +
+ + + {workflowLabel} + +
+ )} diff --git a/apps/sim/app/workspace/[workspaceId]/logs/components/log-row-context-menu/log-row-context-menu.tsx b/apps/sim/app/workspace/[workspaceId]/logs/components/log-row-context-menu/log-row-context-menu.tsx index 0b2f7c11a2b..d0c8940bd8d 100644 --- a/apps/sim/app/workspace/[workspaceId]/logs/components/log-row-context-menu/log-row-context-menu.tsx +++ b/apps/sim/app/workspace/[workspaceId]/logs/components/log-row-context-menu/log-row-context-menu.tsx @@ -16,6 +16,7 @@ import { X, } from '@sim/emcn' import type { WorkflowLogSummary } from '@/lib/api/contracts/logs' +import { resolveLogWorkflowId } from '@/app/workspace/[workspaceId]/logs/utils' interface LogRowContextMenuProps { isOpen: boolean @@ -58,6 +59,12 @@ export const LogRowContextMenu = memo(function LogRowContextMenu({ }: LogRowContextMenuProps) { const hasExecutionId = Boolean(log?.executionId) const hasWorkflow = Boolean(log?.workflow?.id || log?.workflowId) + /** + * "Open Workflow" needs a navigable target, which is stricter than + * `hasWorkflow`: Sim agent jobs have no workflow of their own. Cancel/retry + * keep using `hasWorkflow` so their gating is unchanged. + */ + const hasOpenableWorkflow = Boolean(log && resolveLogWorkflowId(log)) const isCancellable = (log?.status === 'running' || log?.status === 'pending') && hasExecutionId && hasWorkflow const isRetryable = log?.status === 'failed' && hasWorkflow && log?.trigger !== 'mothership' @@ -112,7 +119,7 @@ export const LogRowContextMenu = memo(function LogRowContextMenu({ - + Open Workflow diff --git a/apps/sim/app/workspace/[workspaceId]/logs/logs.tsx b/apps/sim/app/workspace/[workspaceId]/logs/logs.tsx index 7bf6142e139..5d812699e7c 100644 --- a/apps/sim/app/workspace/[workspaceId]/logs/logs.tsx +++ b/apps/sim/app/workspace/[workspaceId]/logs/logs.tsx @@ -92,9 +92,11 @@ import { getDisplayStatus, type LogStatus, parseDuration, + resolveLogWorkflowId, STATUS_CONFIG, StatusBadge, TriggerBadge, + workflowEditorPath, } from './utils' const LOGS_PER_PAGE = 50 as const @@ -524,9 +526,9 @@ export default function Logs() { }, [contextMenuLog, workspaceId]) const handleOpenWorkflow = useCallback(() => { - const wfId = contextMenuLog?.workflow?.id || contextMenuLog?.workflowId + const wfId = contextMenuLog ? resolveLogWorkflowId(contextMenuLog) : null if (wfId) { - window.open(`/workspace/${workspaceId}/w/${wfId}`, '_blank') + window.open(workflowEditorPath(workspaceId, wfId), '_blank') } }, [contextMenuLog, workspaceId]) diff --git a/apps/sim/app/workspace/[workspaceId]/logs/utils.test.ts b/apps/sim/app/workspace/[workspaceId]/logs/utils.test.ts new file mode 100644 index 00000000000..4d4cff37ec7 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/logs/utils.test.ts @@ -0,0 +1,55 @@ +/** + * @vitest-environment jsdom + */ +import { describe, expect, it } from 'vitest' +import { resolveLogWorkflowId, workflowEditorPath } from './utils' + +describe('resolveLogWorkflowId', () => { + it('returns the nested workflow id when present', () => { + expect( + resolveLogWorkflowId({ trigger: 'manual', workflowId: 'wf-1', workflow: { id: 'wf-1' } }) + ).toBe('wf-1') + }) + + it('falls back to workflowId when the workflow object is absent', () => { + expect(resolveLogWorkflowId({ trigger: 'api', workflowId: 'wf-2', workflow: null })).toBe( + 'wf-2' + ) + }) + + it('prefers the nested workflow id over workflowId when both are set', () => { + expect( + resolveLogWorkflowId({ trigger: 'manual', workflowId: 'stale', workflow: { id: 'fresh' } }) + ).toBe('fresh') + }) + + it('returns null for Sim agent jobs even when a workflow id exists', () => { + expect( + resolveLogWorkflowId({ + trigger: 'mothership', + workflowId: 'wf-3', + workflow: { id: 'wf-3' }, + }) + ).toBeNull() + }) + + it('returns null for a deleted workflow (both id fields empty)', () => { + expect(resolveLogWorkflowId({ trigger: 'manual', workflowId: null, workflow: null })).toBeNull() + }) + + it('returns null when ids are present but empty strings', () => { + expect( + resolveLogWorkflowId({ trigger: 'manual', workflowId: '', workflow: { id: '' } }) + ).toBeNull() + }) + + it('treats a missing trigger as a normal workflow run', () => { + expect(resolveLogWorkflowId({ workflowId: 'wf-4' })).toBe('wf-4') + }) +}) + +describe('workflowEditorPath', () => { + it('builds the workspace-scoped editor path', () => { + expect(workflowEditorPath('ws-1', 'wf-1')).toBe('/workspace/ws-1/w/wf-1') + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/logs/utils.ts b/apps/sim/app/workspace/[workspaceId]/logs/utils.ts index fbffba81702..05820985af3 100644 --- a/apps/sim/app/workspace/[workspaceId]/logs/utils.ts +++ b/apps/sim/app/workspace/[workspaceId]/logs/utils.ts @@ -17,6 +17,29 @@ export const LOG_COLUMNS = { export const DELETED_WORKFLOW_LABEL = 'Deleted Workflow' +/** + * Resolves the workflow a log row points at, or null when there is nowhere to + * navigate. Sim agent jobs have no workflow of their own, and a deleted + * workflow leaves both id fields empty. + * + * Single source of truth for "is this log's workflow reachable" — the list row, + * its context menu, and the details panel must agree, or a row can render as + * "Deleted Workflow" while still linking somewhere. + */ +export function resolveLogWorkflowId(log: { + trigger?: string | null + workflowId?: string | null + workflow?: { id?: string } | null +}): string | null { + if (log.trigger === 'mothership') return null + return log.workflow?.id || log.workflowId || null +} + +/** Path to a workflow in the editor. */ +export function workflowEditorPath(workspaceId: string, workflowId: string): string { + return `/workspace/${workspaceId}/w/${workflowId}` +} + export type LogStatus = | 'error' | 'pending' From 623003e43bd1270699551610519ddc23a8139897 Mon Sep 17 00:00:00 2001 From: Waleed Date: Tue, 4 Aug 2026 23:17:37 -0700 Subject: [PATCH 03/10] fix(providers): name the failing phase of a stalled OpenAI call, and reject a failed generation (#6283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(providers): name the failing phase of a stalled OpenAI call, and reject a failed generation An agent block hung ~4.5 minutes with an empty trace and surfaced only the runtime's own `TimeoutError: The operation timed out.` The cause was a runaway generation: the model repeated one tool call until it consumed the whole 128,000-token output budget, which takes minutes, and `/v1/responses` withholds its 200 until generation finishes — so the client waited, bounded only by an undocumented runtime socket deadline, and gave up before the response existed. Nothing in the trace could distinguish that from a request the provider never answered, or from one whose body never arrived. - Name the phase a transport failure died in — `awaiting-response-headers` vs `reading-response-body` — with status, ttfb, content-length and `x-request-id`. undici draws the same line as two error types (UND_ERR_HEADERS_TIMEOUT / UND_ERR_BODY_TIMEOUT); the OpenAI SDK captures `x-request-id` for the same reason. It rides the error message because that reaches the trace span, which survives when a task stops shipping logs. - Carry the cause through `ProviderError` so a transport timeout still classifies after wrapping overwrites `name`. - Reject a 200 that reports a failed or unusable generation instead of returning empty content with billed tokens, and stop truncated tool calls from executing. Matches `streamResponsesTurn`, which already did this, and `@ai-sdk/openai`, which throws on the same condition. - Bound non-JSON error bodies so a gateway error page cannot become the user-facing block error. Deliberately not included: a response-body deadline (the observed failure is in the headers phase, and the body transfers in ~1ms) and status-based retries (worth doing, unrelated to this, and separable). * test(providers): pin that a structured provider error survives the error-body bound * chore(providers): trim comments to the non-obvious why * fix(providers): let a deadline while reading an error body propagate * fix(providers): name the body phase when an error body read fails --- .claude/rules/sim-ui-copy.md | 46 ++++ .cursor/rules/sim-ui-copy.mdc | 44 ++++ .../handlers/agent/agent-handler.test.ts | 43 ++++ .../executor/handlers/agent/agent-handler.ts | 27 +- .../openai/core.response-status.test.ts | 241 ++++++++++++++++++ .../openai/core.transport-phase.test.ts | 212 +++++++++++++++ apps/sim/providers/openai/core.ts | 206 +++++++++++++-- apps/sim/providers/types.ts | 13 +- 8 files changed, 811 insertions(+), 21 deletions(-) create mode 100644 .claude/rules/sim-ui-copy.md create mode 100644 .cursor/rules/sim-ui-copy.mdc create mode 100644 apps/sim/providers/openai/core.response-status.test.ts create mode 100644 apps/sim/providers/openai/core.transport-phase.test.ts diff --git a/.claude/rules/sim-ui-copy.md b/.claude/rules/sim-ui-copy.md new file mode 100644 index 00000000000..951e4a15367 --- /dev/null +++ b/.claude/rules/sim-ui-copy.md @@ -0,0 +1,46 @@ +--- +paths: + - "apps/sim/**/*.tsx" + - "apps/sim/components/emcn/**" +--- + +# UI Copy + +**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading. + +This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.md` instead. + +**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections, so it earns its place by the "prevents misunderstanding" test. Keep those descriptions verb-first and one line, per `sim-settings-pages.md`. Everything else on a settings page — inline `

` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above. + +## The default is no description + +```tsx +// ✗ Bad — the subtitle restates the heading +

API Keys

+

Manage your API keys.

+ +// ✗ Bad — decorative filler under a field label + + +// ✓ Good — the label carries the whole meaning +

API Keys

+ +``` + +If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line. + +## When supporting copy earns its place + +Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake: + +- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone." +- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens." +- **A security or access implication** — "This key is shown once and grants full workspace access." +- **A state the user cannot otherwise see** — "Inherited from your organization's policy." +- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com." + +Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted. + +## Component APIs + +Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above. diff --git a/.cursor/rules/sim-ui-copy.mdc b/.cursor/rules/sim-ui-copy.mdc new file mode 100644 index 00000000000..4648eb21e32 --- /dev/null +++ b/.cursor/rules/sim-ui-copy.mdc @@ -0,0 +1,44 @@ +--- +description: UI copy conventions — no default subtitles or helper text under headings, labels, cards, or settings +globs: ["apps/sim/**/*.tsx"] +--- +# UI Copy + +**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading. + +This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.mdc` instead. + +**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections. Everything else on a settings page — inline `

` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above. + +## The default is no description + +```tsx +// ✗ Bad — the subtitle restates the heading +

API Keys

+

Manage your API keys.

+ +// ✗ Bad — decorative filler under a field label + + +// ✓ Good — the label carries the whole meaning +

API Keys

+ +``` + +If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line. + +## When supporting copy earns its place + +Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake: + +- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone." +- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens." +- **A security or access implication** — "This key is shown once and grants full workspace access." +- **A state the user cannot otherwise see** — "Inherited from your organization's policy." +- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com." + +Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted. + +## Component APIs + +Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above. diff --git a/apps/sim/executor/handlers/agent/agent-handler.test.ts b/apps/sim/executor/handlers/agent/agent-handler.test.ts index 0da21a6a431..35dbf52d7f3 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.test.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.test.ts @@ -979,6 +979,49 @@ describe('AgentBlockHandler', () => { ) }) + /** + * A stalled model call reaches here as the runtime's own `TimeoutError`, whose bare + * message ("The operation timed out.") names nothing. It must become a Sim-level + * message WITHOUT discarding the phase detail the provider attached — that detail is + * the only thing distinguishing "never answered" from "body never completed". + */ + it('maps a provider TimeoutError to a Sim message while keeping the phase detail', async () => { + const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' } + mockGetProviderFromModel.mockReturnValue('openai') + + // Faithful to production: providers rewrap the transport failure in a + // ProviderError, which overwrites `name` — so only the cause still classifies it. + const transport = new Error( + 'The operation timed out. [phase=reading-response-body elapsedMs=60001 status=200 contentLength=32116]' + ) + transport.name = 'TimeoutError' + const wrapped = new Error(transport.message, { cause: transport }) + wrapped.name = 'ProviderError' + mockExecuteProviderRequest.mockRejectedValueOnce(wrapped) + + const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e) + + expect(error.message).toContain('Provider request timed out') + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=200') + }) + + it('maps a provider AbortError the same way', async () => { + const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' } + mockGetProviderFromModel.mockReturnValue('openai') + + const aborted = new Error('aborted [phase=awaiting-response-headers elapsedMs=12]') + aborted.name = 'AbortError' + const wrapped = new Error(aborted.message, { cause: aborted }) + wrapped.name = 'ProviderError' + mockExecuteProviderRequest.mockRejectedValueOnce(wrapped) + + const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e) + + expect(error.message).toContain('Provider request timed out') + expect(error.message).toContain('phase=awaiting-response-headers') + }) + it('should handle streaming responses with text/event-stream content type', async () => { const mockStreamBody = new ReadableStream({ start(controller) { diff --git a/apps/sim/executor/handlers/agent/agent-handler.ts b/apps/sim/executor/handlers/agent/agent-handler.ts index 8d510d1696e..209443bb7c2 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.ts @@ -71,6 +71,22 @@ import { getToolAsync } from '@/tools/utils.server' const logger = createLogger('AgentBlockHandler') +/** + * True when a failure originated from a transport deadline or abort, at any depth of the + * cause chain. + * + * Providers rewrap transport failures (`ProviderError` overwrites `name`), so a check on + * the top-level `name` alone misses every wrapped case. Bounded to a short walk so a + * self-referential cause cannot loop. + */ +function isTransportTimeout(error: unknown): boolean { + for (let current = error, depth = 0; current instanceof Error && depth < 5; depth++) { + if (current.name === 'AbortError' || current.name === 'TimeoutError') return true + current = current.cause + } + return false +} + /** * Handler for Agent blocks that process LLM requests with optional tools. */ @@ -1299,8 +1315,15 @@ export class AgentBlockHandler implements BlockHandler { timestamp: new Date().toISOString(), }) - if (error.name === 'AbortError') { - throw new Error('Provider request timed out - the API took too long to respond') + /** + * The original message is appended rather than replaced: providers annotate it with + * the request phase they died in, which is the only thing separating a request that + * was never answered from one whose body stalled. + */ + if (isTransportTimeout(error)) { + throw new Error( + `Provider request timed out - the API took too long to respond (${error.message})` + ) } if (error.name === 'TypeError' && error.message.includes('fetch')) { throw new Error( diff --git a/apps/sim/providers/openai/core.response-status.test.ts b/apps/sim/providers/openai/core.response-status.test.ts new file mode 100644 index 00000000000..1ff16b1b0a0 --- /dev/null +++ b/apps/sim/providers/openai/core.response-status.test.ts @@ -0,0 +1,241 @@ +/** + * @vitest-environment node + * + * Pins the non-streaming status/error gate, and pins its `incomplete` policy to the one + * `streamResponsesTurn` applies so the two paths cannot silently diverge. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { executeResponsesProviderRequest } from '@/providers/openai/core' +import type { ProviderRequest, ProviderResponse } from '@/providers/types' + +vi.mock('@/providers', () => ({ MAX_TOOL_ITERATIONS: 5 })) + +vi.mock('@/providers/utils', () => ({ + isFunctionToolCall: () => false, + calculateCost: () => ({ input: 0, output: 0, total: 0 }), + sumToolCosts: () => 0, + enforceStrictSchema: (schema: unknown) => schema, + prepareToolExecution: () => ({ toolParams: {}, executionParams: {} }), + prepareToolsWithUsageControl: (tools: unknown[]) => ({ + tools, + toolChoice: undefined, + forcedTools: [], + hasFilteredTools: false, + }), + trackForcedToolUsage: () => ({ hasUsedForcedTool: false, usedForcedTools: [] }), + supportsReasoningEffort: () => false, +})) + +const { mockExecuteProviderTool } = vi.hoisted(() => ({ + mockExecuteProviderTool: vi.fn(), +})) + +vi.mock('@/providers/runtime-context', () => ({ + executeProviderTool: mockExecuteProviderTool, +})) + +function jsonResponse(body: unknown) { + return { + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(body), + } +} + +const USAGE = { input_tokens: 1, output_tokens: 1, total_tokens: 2 } + +function message(text: string) { + return { + type: 'message', + role: 'assistant', + content: [{ type: 'output_text', text }], + } +} + +function functionCall(args: string) { + return { type: 'function_call', call_id: 'call_1', name: 'exa_search', arguments: args } +} + +const COMPLETED_RESPONSE = { + id: 'resp_1', + status: 'completed', + error: null, + incomplete_details: null, + output: [message('hello')], + usage: USAGE, +} + +describe('OpenAI non-streaming response status handling', () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as any + + beforeEach(() => { + vi.clearAllMocks() + mockExecuteProviderTool.mockResolvedValue({ success: true, output: { results: [] } }) + }) + + function run(fetchMock: unknown, request: Partial = {}) { + return executeResponsesProviderRequest( + { apiKey: 'k', model: 'gpt-5.5', messages: [{ role: 'user', content: 'hi' }], ...request }, + { + providerId: 'openai', + providerLabel: 'OpenAI', + modelName: 'gpt-5.5', + endpoint: 'https://api.openai.com/v1/responses', + headers: { Authorization: 'Bearer k' }, + logger, + fetch: fetchMock as typeof fetch, + } + ) + } + + const TOOL_REQUEST: Partial = { + tools: [{ id: 'exa_search', name: 'exa_search', description: 'search', params: {} }], + } + + it('fails the block on a 200 carrying status "failed", surfacing the API error message', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'failed', + error: { code: 'server_error', message: 'The model produced an invalid response.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow('The model produced an invalid response.') + }) + + it('fails the block when error is populated but status is absent', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + error: { code: null, message: 'Upstream provider rejected the request.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow('Upstream provider rejected the request.') + }) + + /** Policy is shared with `streamResponsesTurn` — keep both in step. */ + it('returns the partial content of a max_output_tokens incomplete response instead of failing', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'max_output_tokens' }, + output: [message('a truncated but usable answer')], + usage: USAGE, + }) + ) + + const result = (await run(fetchMock)) as ProviderResponse + expect(result.content).toBe('a truncated but usable answer') + }) + + it('fails the block on an incomplete response whose reason is not max_output_tokens', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'content_filter' }, + output: [message('partial')], + usage: USAGE, + }) + ) + + await expect(run(fetchMock)).rejects.toThrow(/content_filter/) + }) + + /** + * The confusing-failure case: a truncated `function_call` holds half-written JSON. + * Executing it made `parseToolArguments` throw, reporting a tool bug rather than the + * truncation that actually happened. + */ + it('does not execute a tool call from a non-completed response', async () => { + const fetchMock = vi.fn().mockResolvedValue( + jsonResponse({ + id: 'resp_1', + status: 'incomplete', + error: null, + incomplete_details: { reason: 'max_output_tokens' }, + output: [functionCall('{"query": "half writ')], + usage: USAGE, + }) + ) + + await expect(run(fetchMock, TOOL_REQUEST)).rejects.toThrow(/max_output_tokens/) + expect(mockExecuteProviderTool).not.toHaveBeenCalled() + }) + + it('leaves a healthy completed response entirely unaffected', async () => { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse(COMPLETED_RESPONSE)) + + const result = (await run(fetchMock)) as ProviderResponse + expect(result.content).toBe('hello') + expect(result.toolCalls).toBeUndefined() + expect(result.tokens?.total).toBe(2) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('still runs the multi-turn tool loop end to end', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_tool', + status: 'completed', + error: null, + incomplete_details: null, + output: [functionCall('{"query":"sim"}')], + usage: USAGE, + }) + ) + .mockResolvedValueOnce(jsonResponse(COMPLETED_RESPONSE)) + + const result = (await run(fetchMock, TOOL_REQUEST)) as ProviderResponse + + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(mockExecuteProviderTool).toHaveBeenCalledTimes(1) + expect(result.toolCalls).toHaveLength(1) + expect(result.toolCalls?.[0].success).toBe(true) + expect(result.content).toBe('hello') + expect(result.tokens?.total).toBe(4) + }) + + /** The gate lives in `postResponses`, so continuation turns are covered too. */ + it('fails the block when a later tool-loop turn comes back failed', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_tool', + status: 'completed', + error: null, + incomplete_details: null, + output: [functionCall('{"query":"sim"}')], + usage: USAGE, + }) + ) + .mockResolvedValueOnce( + jsonResponse({ + id: 'resp_2', + status: 'failed', + error: { code: 'server_error', message: 'Second turn blew up.' }, + incomplete_details: null, + output: [], + usage: USAGE, + }) + ) + + await expect(run(fetchMock, TOOL_REQUEST)).rejects.toThrow('Second turn blew up.') + expect(mockExecuteProviderTool).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/providers/openai/core.transport-phase.test.ts b/apps/sim/providers/openai/core.transport-phase.test.ts new file mode 100644 index 00000000000..74459fc40e5 --- /dev/null +++ b/apps/sim/providers/openai/core.transport-phase.test.ts @@ -0,0 +1,212 @@ +/** + * @vitest-environment node + * + * Covers the phase annotation that separates "never answered" from "answered, but the + * body never arrived" — the runtime reports both as a bare `TimeoutError`. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { executeResponsesProviderRequest } from '@/providers/openai/core' +import type { ProviderRequest } from '@/providers/types' + +const { mockSupportsReasoningEffort } = vi.hoisted(() => ({ + mockSupportsReasoningEffort: vi.fn(() => false), +})) + +vi.mock('@/providers', () => ({ MAX_TOOL_ITERATIONS: 5 })) + +vi.mock('@/providers/utils', () => ({ + isFunctionToolCall: () => false, + calculateCost: () => ({ input: 0, output: 0, total: 0 }), + sumToolCosts: () => 0, + enforceStrictSchema: (schema: unknown) => schema, + prepareToolExecution: () => ({ toolParams: {}, executionParams: {} }), + prepareToolsWithUsageControl: (tools: unknown[]) => ({ + tools, + toolChoice: undefined, + forcedTools: [], + hasFilteredTools: false, + }), + trackForcedToolUsage: () => ({ hasUsedForcedTool: false, usedForcedTools: [] }), + supportsReasoningEffort: mockSupportsReasoningEffort, +})) + +vi.mock('@/tools', () => ({ executeTool: vi.fn() })) + +/** + * Exactly what the runtime raises when a fetch deadline fires: a `DOMException`, NOT a + * plain `Error`. The distinction is load-bearing — `DOMException.message` is a readonly + * getter, so annotating by assignment throws a `TypeError` and replaces the real + * failure. Building a plain `Error` here would let that regression pass. + */ +function timeoutError() { + return new DOMException('The operation timed out.', 'TimeoutError') +} + +const COMPLETED = { + id: 'resp_1', + status: 'completed', + output: [{ type: 'message', role: 'assistant', content: [{ type: 'output_text', text: 'ok' }] }], + usage: { input_tokens: 1, output_tokens: 1, total_tokens: 2 }, +} + +describe('OpenAI transport phase annotation', () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as never + + beforeEach(() => { + vi.clearAllMocks() + mockSupportsReasoningEffort.mockReturnValue(false) + }) + + function run(fetchMock: unknown, request: Partial = {}) { + return executeResponsesProviderRequest( + { apiKey: 'k', model: 'gpt-5.5', messages: [{ role: 'user', content: 'hi' }], ...request }, + { + providerId: 'openai', + providerLabel: 'OpenAI', + modelName: 'gpt-5.5', + endpoint: 'https://api.openai.com/v1/responses', + headers: { Authorization: 'Bearer k' }, + logger, + fetch: fetchMock as typeof fetch, + } + ) + } + + /** + * The production case: `/v1/responses` withholds its 200 until generation finishes, so + * a runaway generation is still in the headers phase when the client gives up. + */ + it('names the header phase when the request was never answered', async () => { + const error = await run(vi.fn().mockRejectedValue(timeoutError())).catch((e) => e) + + expect(error.message).toContain('phase=awaiting-response-headers') + expect(error.message).toMatch(/elapsedMs=\d+/) + // No response existed, so no response metadata may be claimed. + expect(error.message).not.toContain('status=') + }) + + it('names the body phase when headers arrived but the body did not', async () => { + const stalled = { + ok: true, + status: 200, + headers: new Headers({ 'content-length': '32116', 'content-encoding': 'br' }), + json: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(stalled)).catch((e) => e) + + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=200') + expect(error.message).toContain('contentLength=32116') + expect(error.message).toContain('contentEncoding=br') + expect(error.message).toMatch(/ttfbMs=\d+/) + }) + + /** The only identifier the provider can trace a failed call by. */ + it('carries the x-request-id of a failed response', async () => { + const stalled = { + ok: true, + status: 200, + headers: new Headers({ 'x-request-id': 'req_abc123' }), + json: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(stalled)).catch((e) => e) + expect(error.message).toContain('requestId=req_abc123') + }) + + it('leaves a self-describing API error untouched', async () => { + const apiError = { + ok: false, + status: 429, + headers: new Headers(), + text: () => Promise.resolve(JSON.stringify({ error: { message: 'Rate limit reached' } })), + } + + const error = await run(vi.fn().mockResolvedValue(apiError)).catch((e) => e) + expect(error.message).toContain('Rate limit reached') + expect(error.message).not.toContain('phase=') + }) + + it('bounds a non-JSON error body instead of pasting a gateway page into the error', async () => { + const htmlError = { + ok: false, + status: 502, + headers: new Headers(), + text: () => Promise.resolve(`${'x'.repeat(5000)}`), + } + + const error = await run(vi.fn().mockResolvedValue(htmlError)).catch((e) => e) + expect(error.message.length).toBeLessThan(700) + }) + + /** + * The bound applies only to non-JSON bodies. A structured provider error must survive + * intact, because the reasoning-summary strip-and-retry fallback matches on its text + * (`message.includes('reasoning.summary')`) — truncating it would silently disable + * that recovery path for any provider whose error message runs long. + */ + it('does not truncate a structured provider error, so the summary fallback still matches', async () => { + // Marker sits past the 500-char bound, so truncation would break the fallback match. + const longMessage = `${'context detail. '.repeat(40)}Invalid value for reasoning.summary: your organization must be verified to use this feature.` + expect(longMessage.indexOf('reasoning.summary')).toBeGreaterThan(500) + + const completed = { + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(COMPLETED), + } + const verificationError = { + ok: false, + status: 400, + headers: new Headers(), + text: () => Promise.resolve(JSON.stringify({ error: { message: longMessage } })), + } + const fetchMock = vi + .fn() + .mockResolvedValueOnce(verificationError) + .mockResolvedValueOnce(completed) + // The fallback only applies when the payload actually carried reasoning.summary. + mockSupportsReasoningEffort.mockReturnValue(true) + + await expect(run(fetchMock, { agentEvents: true })).resolves.toMatchObject({ content: 'ok' }) + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + /** + * Reading the error body of a non-OK response can itself hit the deadline or be + * cancelled. Swallowing that would report the HTTP status as the failure and lose both + * the transport detail and the fact that the user aborted. + */ + it('propagates a deadline hit while reading a non-OK error body', async () => { + const unreadable = { + ok: false, + status: 502, + headers: new Headers(), + text: () => Promise.reject(timeoutError()), + } + + const error = await run(vi.fn().mockResolvedValue(unreadable)).catch((e) => e) + + expect(error.message).toContain('The operation timed out.') + expect(error.message).not.toContain('API error') + // The headers already arrived, so this is the body phase despite the 4xx/5xx status. + expect(error.message).toContain('phase=reading-response-body') + expect(error.message).toContain('status=502') + // Annotated exactly once: the outer catch must not append a second, wrong phase. + expect(error.message.match(/phase=/g)).toHaveLength(1) + }) + + it('leaves a healthy response entirely unaffected', async () => { + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: () => Promise.resolve(COMPLETED), + }) + + await expect(run(fetchMock)).resolves.toMatchObject({ content: 'ok' }) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/providers/openai/core.ts b/apps/sim/providers/openai/core.ts index 47dd9e18b7c..739d6d21e87 100644 --- a/apps/sim/providers/openai/core.ts +++ b/apps/sim/providers/openai/core.ts @@ -2,6 +2,7 @@ import { createHash } from 'node:crypto' import type { Logger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' +import { truncate } from '@sim/utils/string' import type OpenAI from 'openai' import type { NormalizedBlockOutput, StreamingExecution } from '@/executor/types' import { MAX_TOOL_ITERATIONS } from '@/providers' @@ -33,12 +34,65 @@ import { createReadableStreamFromResponses, extractResponseText, extractResponseToolCalls, + isMaxOutputTokensIncompleteResponse, parseResponsesUsage, type ResponsesInputItem, type ResponsesToolCall, + responseContainsFunctionCall, toResponsesToolChoice, } from './utils' +/** + * Rejects a `/v1/responses` body reporting a generation that did not succeed — the + * endpoint answers HTTP 200 for both `status: 'failed'` and `status: 'incomplete'`. + * + * The tolerated case must stay matched to `streamResponsesTurn`: `incomplete` is accepted + * only when truncated by `max_output_tokens` AND carrying no function call. Truncated + * prose is a usable partial answer, but a truncated `function_call` holds half-written + * JSON that makes `parseToolArguments` throw a confusing tool failure. + * + * An absent `status` is deliberately not treated as a failure: this path is shared with + * Azure OpenAI and OpenAI-compatible gateways. + */ +function assertUsableResponse(response: OpenAI.Responses.Response, providerLabel: string): void { + if (response.error) { + const code = response.error.code ? ` (${response.error.code})` : '' + throw new Error(`${providerLabel} generation failed${code}: ${response.error.message}`) + } + + if (response.status === 'failed') { + throw new Error( + `${providerLabel} generation failed, and the API returned no error detail explaining why.` + ) + } + + if (response.status === 'incomplete') { + const reason = response.incomplete_details?.reason ?? 'unknown' + if (responseContainsFunctionCall(response)) { + throw new Error( + `${providerLabel} generation stopped before completion (${reason}), truncating a tool call mid-argument. Raise the max output tokens or reduce the tool schema size.` + ) + } + if (!isMaxOutputTokensIncompleteResponse(response)) { + throw new Error(`${providerLabel} generation stopped before completion: ${reason}.`) + } + return + } + + if (response.status && response.status !== 'completed') { + throw new Error( + `${providerLabel} returned a response with status "${response.status}", which carries no finished generation.` + ) + } +} + +/** + * Transport failures annotated once already. The error-body read is annotated where the + * phase is known, then rethrown through an outer catch that would otherwise append a + * second, wrong phase to the same message. + */ +const annotatedTransportFailures = new WeakSet() + type PreparedTools = ReturnType type ToolChoice = PreparedTools['toolChoice'] @@ -85,6 +139,9 @@ export async function executeResponsesProviderRequest( logger.info(`Preparing ${config.providerLabel} request`, { model: request.model, + workflowId: request.workflowId, + blockId: request.blockId, + executionId: request.executionId, hasSystemPrompt: !!request.systemPrompt, hasMessages: !!request.messages?.length, hasTools: !!request.tools?.length, @@ -237,14 +294,97 @@ export async function executeResponsesProviderRequest( ...overrides, }) - const parseErrorResponse = async (response: Response): Promise => { - const text = await response.text() + /** + * Names the request phase an opaque transport failure died in. + * + * Bun raises only `TimeoutError: The operation timed out.`, which cannot distinguish + * "never answered" from "answered, but the body never arrived" — opposite owners, + * opposite fixes. undici splits these as `UND_ERR_HEADERS_TIMEOUT` vs + * `UND_ERR_BODY_TIMEOUT`; this records the equivalent for a runtime that reports + * neither. + * + * The phase rides the error message because that reaches the block's trace span, which + * survives when a task has stopped shipping logs; `x-request-id` is the only handle the + * provider can trace the call by. Self-describing API errors are left untouched. + */ + const annotateTransportFailure = ( + error: unknown, + phase: 'awaiting-response-headers' | 'reading-response-body', + startedAt: number, + detail?: Record + ): unknown => { + if (!(error instanceof Error)) return error + if (error.name !== 'TimeoutError' && error.name !== 'AbortError') return error + if (annotatedTransportFailures.has(error)) return error + + const elapsedMs = Date.now() - startedAt + const fields = Object.entries(detail ?? {}) + .filter(([, value]) => value !== null && value !== undefined) + .map(([key, value]) => `${key}=${value}`) + const context = [`phase=${phase}`, `elapsedMs=${elapsedMs}`, ...fields].join(' ') + + logger.error(`${config.providerLabel} request failed in transport`, { + phase, + elapsedMs, + errorName: error.name, + model: config.modelName, + workflowId: request.workflowId, + blockId: request.blockId, + executionId: request.executionId, + ...detail, + }) + + /** + * A new Error rather than a mutation: the runtime raises these as `DOMException`, + * whose `message` is a readonly getter, so assigning to it throws a `TypeError` and + * destroys the very failure being reported. `name` is copied and the original hangs + * off `cause` so the classification survives the `ProviderError` wrapping below, + * which overwrites `name`. + */ + const annotated = new Error(`${error.message} [${context}]`, { cause: error }) + annotated.name = error.name + annotatedTransportFailures.add(annotated) + return annotated + } + + /** + * The response-side facts worth carrying on a transport failure. `x-request-id` is the + * only handle the provider can trace a failed call by. + */ + const describeResponse = (response: Response): Record => ({ + status: response.status, + requestId: response.headers.get('x-request-id'), + contentLength: response.headers.get('content-length'), + contentEncoding: response.headers.get('content-encoding'), + }) + + /** + * A non-JSON body is usually a gateway or CDN error page and reaches the user-facing + * block error, so it is bounded and falls back to `statusText`. A structured provider + * message is returned untruncated on purpose: the reasoning-summary strip-and-retry + * fallback matches on its text. + * + * A failed body read is annotated rather than swallowed: a deadline or a cancellation + * here must stay distinguishable from an error response that simply carried no body. + * The headers already arrived, so this is the body phase even though the status is 4xx. + */ + const parseErrorResponse = async (response: Response, startedAt: number): Promise => { + let text: string try { - const payload = JSON.parse(text) - return payload?.error?.message || text - } catch { - return text + text = await response.text() + } catch (error) { + throw annotateTransportFailure( + error, + 'reading-response-body', + startedAt, + describeResponse(response) + ) } + try { + const payload = JSON.parse(text) + if (payload?.error?.message) return payload.error.message + } catch {} + return truncate(text.trim(), 500) || response.statusText || `HTTP ${response.status}` } /** @@ -272,6 +412,7 @@ export async function executeResponsesProviderRequest( const fetchResponsesWithSummaryFallback = async ( requestedBody: Record, + startedAt: number, abortSignal = request.abortSignal ): Promise => { const body = reasoningSummariesUnavailable @@ -285,7 +426,7 @@ export async function executeResponsesProviderRequest( }) if (response.ok) return response - const message = await parseErrorResponse(response) + const message = await parseErrorResponse(response, startedAt) const strippedBody = isReasoningSummaryVerificationError(response.status, message) ? stripReasoningSummary(body) : null @@ -305,7 +446,7 @@ export async function executeResponsesProviderRequest( signal: abortSignal, }) if (!retryResponse.ok) { - const retryMessage = await parseErrorResponse(retryResponse) + const retryMessage = await parseErrorResponse(retryResponse, startedAt) throw new Error( `${config.providerLabel} API error (${retryResponse.status}): ${retryMessage}` ) @@ -316,8 +457,30 @@ export async function executeResponsesProviderRequest( const postResponses = async ( body: Record ): Promise => { - const response = await fetchResponsesWithSummaryFallback(body) - return response.json() + const startedAt = Date.now() + + let response: Response + try { + response = await fetchResponsesWithSummaryFallback(body, startedAt) + } catch (error) { + throw annotateTransportFailure(error, 'awaiting-response-headers', startedAt) + } + + const responseMeta = { ...describeResponse(response), ttfbMs: Date.now() - startedAt } + + let parsed: OpenAI.Responses.Response + try { + parsed = await response.json() + } catch (error) { + throw annotateTransportFailure(error, 'reading-response-body', startedAt, responseMeta) + } + + /** + * Placed here so every tool-loop turn is covered, and outside the transport `try` so + * a rejected generation is not misreported as a transport failure. + */ + assertUsableResponse(parsed, config.providerLabel) + return parsed } const providerStartTime = Date.now() @@ -355,7 +518,11 @@ export async function executeResponsesProviderRequest( initialToolChoice: responsesToolChoice, forcedTools: preparedTools?.forcedTools, createStream: (input, overrides, abortSignal) => - fetchResponsesWithSummaryFallback(createRequestBody(input, overrides), abortSignal), + fetchResponsesWithSummaryFallback( + createRequestBody(input, overrides), + Date.now(), + abortSignal + ), logger, timeSegments, onComplete: (result) => { @@ -379,7 +546,8 @@ export async function executeResponsesProviderRequest( logger.info(`Using streaming response for ${config.providerLabel} request`) const streamResponse = await fetchResponsesWithSummaryFallback( - createRequestBody(initialInput, { stream: true }) + createRequestBody(initialInput, { stream: true }), + Date.now() ) const streamingResult = createStreamingExecution({ @@ -722,10 +890,14 @@ export async function executeResponsesProviderRequest( throw error } - throw new ProviderError(toError(error).message, { - startTime: providerStartTimeISO, - endTime: providerEndTimeISO, - duration: totalDuration, - }) + throw new ProviderError( + toError(error).message, + { + startTime: providerStartTimeISO, + endTime: providerEndTimeISO, + duration: totalDuration, + }, + { cause: error } + ) } } diff --git a/apps/sim/providers/types.ts b/apps/sim/providers/types.ts index 7c402d66677..e029f830d2c 100644 --- a/apps/sim/providers/types.ts +++ b/apps/sim/providers/types.ts @@ -241,8 +241,17 @@ export class ProviderError extends Error { duration: number } - constructor(message: string, timing: { startTime: string; endTime: string; duration: number }) { - super(message) + /** + * `options.cause` should carry the error being wrapped. `name` is deliberately + * overwritten with `'ProviderError'`, so without a cause every classification the + * original carried — notably a transport `TimeoutError` — is lost to callers. + */ + constructor( + message: string, + timing: { startTime: string; endTime: string; duration: number }, + options?: ErrorOptions + ) { + super(message, options) this.name = 'ProviderError' this.timing = timing } From 0fce5358b66624775c71861aa3735e1478b214a2 Mon Sep 17 00:00:00 2001 From: Waleed Date: Tue, 4 Aug 2026 23:24:56 -0700 Subject: [PATCH 04/10] fix(files): stop markdown reflow on open by matching placeholder wrapping to the editor (#6285) --- .../rich-markdown-editor/rich-markdown-editor.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx index ffb118973d5..83730f5465e 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx @@ -1198,10 +1198,12 @@ export function LoadedRichMarkdownEditor({ }} /> {showPlaceholder && placeholderHtml && ( - // Instant read-only content while the collaborative doc seeds; the editor stays mounted-but- - // hidden below so it renders the seeded doc before the swap. Same layout box → no reflow. + // Instant read-only content while the collaborative doc seeds, swapped for the live editor + // once ready. The `ProseMirror` class is load-bearing: it gives the placeholder the same base + // text layout as the live editable (prosemirror-view sets `white-space: break-spaces` and + // disables ligatures), so a line wraps identically and never re-wraps on the swap.
)} From 6cc25f6d9bb8e867a94bc72c52f602b2e4ca22b6 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 5 Aug 2026 01:10:33 -0700 Subject: [PATCH 05/10] improvement(forking): make webhook url mapping clear (#6272) * improvement(forking): make webhook url mapping clear * fix(forking): honour drops before the unmapped gate, match provider on URL adoption Review round 1 on #6272. - Drop was inert for required references: `postCopyUnmappedRequired` gates before the cleared-ref gate that honours acknowledgments, so a source-deleted reference on a required field still failed with "map all required ... first". Verified drops are now resolved once (`verifyForkDropAcknowledgments`) and subtracted from both gates. Verification is not optional: an unmapped reference of a non-blocking kind (credential, env-var) never re-blocks downstream, so subtracting raw acknowledgments would let a crafted payload skip the required gate. - URL adoption now requires provider equality. A count-only 1:1 pairing could hand a GitHub URL to an arriving Slack trigger, keeping the endpoint alive while every request failed signature verification - and reporting the URL as preserved. - `resolveTriggerId` moved from `lib/webhooks/deploy.ts` to `@/triggers/webhook-url` so the deploy path and the fork's provider check share one resolution. - Trigger URL warnings render the full public URL in the heads-up section and name the URL in the overwrite confirm, where identical workflow names were ambiguous. - The Drop control renders once per resource and states how many fields it covers; the remapper clears by reference, so a per-row control implied a choice the write path cannot honour. - Export clears `workflow-selector`: nothing on the import path remaps workflow ids (`import-export.ts` re-creates each workflow under a fresh id), so a preserved reference dangled - bundle or not. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../api/workspaces/[id]/fork/diff/route.ts | 37 +++ .../api/workspaces/[id]/fork/promote/route.ts | 19 +- .../components/fork-sync/cleared-refs-list.ts | 12 +- .../fork-sync/copy-reconciliation.ts | 21 +- .../components/fork-sync/fork-sync-view.tsx | 222 +++++++++++++-- .../components/fork-sync/use-fork-sync.ts | 179 ++++++++++++- .../ee/workspace-forking/components/forks.tsx | 33 ++- .../lib/copy/copy-workflows.test.ts | 78 ++++++ .../lib/copy/copy-workflows.ts | 21 ++ .../lib/copy/deploy-bridge.ts | 85 +++++- .../lib/mapping/mapping-service.test.ts | 173 +++++++++++- .../lib/mapping/mapping-service.ts | 87 ++++-- .../lib/mapping/resources.ts | 108 ++++++-- .../lib/promote/cleared-refs.test.ts | 84 ++++-- .../lib/promote/cleared-refs.ts | 85 +++++- .../lib/promote/promote-plan.ts | 39 ++- .../lib/promote/promote.test.ts | 165 +++++++++++- .../workspace-forking/lib/promote/promote.ts | 80 +++++- .../lib/promote/trigger-urls.test.ts | 253 ++++++++++++++++++ .../lib/promote/trigger-urls.ts | 195 ++++++++++++++ .../lib/remap/remap-references.test.ts | 58 ++++ .../lib/remap/remap-references.ts | 14 +- .../lib/api/contracts/workspace-fork.test.ts | 49 ++++ apps/sim/lib/api/contracts/workspace-fork.ts | 101 +++++++ apps/sim/lib/webhooks/deploy.test.ts | 7 +- apps/sim/lib/webhooks/deploy.ts | 43 +-- .../credentials/credential-extractor.test.ts | 96 +++++++ .../credentials/credential-extractor.ts | 45 +++- .../workflows/persistence/duplicate.test.ts | 9 + .../workflows/sanitization/json-sanitizer.ts | 21 +- .../search-replace/resources/registry.test.ts | 72 +++++ .../search-replace/resources/registry.ts | 15 +- apps/sim/triggers/webhook-url.test.ts | 161 +++++++++++ apps/sim/triggers/webhook-url.ts | 110 ++++++++ 34 files changed, 2569 insertions(+), 208 deletions(-) create mode 100644 apps/sim/ee/workspace-forking/lib/promote/trigger-urls.test.ts create mode 100644 apps/sim/ee/workspace-forking/lib/promote/trigger-urls.ts create mode 100644 apps/sim/lib/workflows/credentials/credential-extractor.test.ts create mode 100644 apps/sim/lib/workflows/search-replace/resources/registry.test.ts create mode 100644 apps/sim/triggers/webhook-url.test.ts create mode 100644 apps/sim/triggers/webhook-url.ts diff --git a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts index caf75870d29..72b0d7dd435 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts @@ -10,6 +10,7 @@ import { loadTargetDraftSubBlocks } from '@/ee/workspace-forking/lib/copy/copy-w import { listForkExcludedDeployedWorkflows, loadSourceDeployedStates, + loadTargetWebhookPathsByBlock, } from '@/ee/workspace-forking/lib/copy/deploy-bridge' import { assertCanPromote } from '@/ee/workspace-forking/lib/lineage/authz' import { loadForkBlockMap } from '@/ee/workspace-forking/lib/mapping/block-map-store' @@ -27,6 +28,10 @@ import { collectForkClearedRefCandidates, } from '@/ee/workspace-forking/lib/promote/cleared-refs' import { computeForkPromotePlan } from '@/ee/workspace-forking/lib/promote/promote-plan' +import { + buildForkTriggerPlan, + resolveForkTriggerPaths, +} from '@/ee/workspace-forking/lib/promote/trigger-urls' import { buildForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' import { readTargetDraftDependentValue } from '@/ee/workspace-forking/lib/remap/remap-references' @@ -173,6 +178,36 @@ export const GET = withRouteHandler( }) ) + // Trigger URLs this sync decides in the target - the "we had to re-paste the Slack Request + // URL again" case, surfaced as an editable pairing before the overwrite instead of discovered + // after it. The preview reports the plan's DEFAULT resolution; the user's picks ride the + // promote call, where the same plan is rebuilt and validated against them. + const triggerPlan = buildForkTriggerPlan({ + items: plan.items, + sourceStates, + resolveBlockId, + targetWebhooks: await loadTargetWebhookPathsByBlock(db, allTargetIds), + }) + const { changes: triggerUrlChanges } = resolveForkTriggerPaths(triggerPlan) + // Every trigger that HAS a public URL, plus every one whose URL is up for decision - not just + // the decisions, so the section reads as a standing statement of each URL rather than an alert. + // + // A trigger with neither is deliberately absent: whether a block will serve a URL at all is + // only knowable from its webhook row, and a schedule / chat / manual / poller trigger never + // gets one. Claiming "gets a new URL" for those would be a straight lie, and no declarative + // flag separates them - `polling` is set on 10 of the trigger defs, while `webhook` is set on + // 345 including `slack_oauth`, which routes by `routingKey` with a NULL path. + const triggerMappings = triggerPlan.slots + .filter((slot) => slot.ownPath !== null || slot.adoptablePaths.length > 0) + .map((slot) => ({ + sourceBlockId: slot.sourceBlockId, + blockName: slot.blockName, + workflowName: slot.workflowName, + ownPath: slot.ownPath, + adoptablePaths: slot.adoptablePaths, + defaultAdoptPath: slot.defaultAdoptPath, + })) + const toRef = (reference: (typeof plan.unmappedRequired)[number]) => ({ kind: reference.kind, sourceId: reference.sourceId, @@ -224,6 +259,8 @@ export const GET = withRouteHandler( resourceUsages: collectForkResourceUsages(plan.items, sourceStates), copyableUnmapped: plan.copyableUnmapped, clearedRefs, + triggerUrlChanges, + triggerMappings, }) } ) diff --git a/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts b/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts index cbf6c0fb23b..a06dd6300f9 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/promote/route.ts @@ -25,7 +25,14 @@ export const POST = withRouteHandler( const parsed = await parseRequest(promoteForkContract, req, context) if (!parsed.success) return parsed.response const { id } = parsed.data.params - const { otherWorkspaceId, direction, dependentValues, copyResources } = parsed.data.body + const { + otherWorkspaceId, + direction, + dependentValues, + copyResources, + dropReferences, + triggerMappings, + } = parsed.data.body const auth = await assertCanPromote(id, otherWorkspaceId, direction, session.user.id) @@ -38,6 +45,8 @@ export const POST = withRouteHandler( actorName: session.user.name ?? undefined, dependentValues, copyResources, + dropReferences, + triggerMappings, requestId, }) @@ -52,6 +61,8 @@ export const POST = withRouteHandler( blockers: result.blockers, needsConfiguration: result.needsConfiguration, clearedOptional: result.clearedOptional, + droppedReferences: result.droppedReferences, + triggerUrlChanges: result.triggerUrlChanges, } if (result.blocked) { @@ -91,7 +102,9 @@ export const POST = withRouteHandler( status: result.deployFailed > 0 || result.needsConfiguration.length > 0 || - result.clearedOptional.length > 0 + result.clearedOptional.length > 0 || + result.droppedReferences.length > 0 || + result.triggerUrlChanges.length > 0 ? 'completed_with_warnings' : 'completed', message: direction === 'pull' ? `Pulled from "${otherName}"` : `Pushed to "${otherName}"`, @@ -110,6 +123,8 @@ export const POST = withRouteHandler( archivedNames: result.archivedNames, needsConfiguration: result.needsConfiguration, clearedOptional: result.clearedOptional, + droppedReferences: result.droppedReferences.length, + triggerUrlChanges: result.triggerUrlChanges.length, }, }).catch((error) => logger.error(`[${requestId}] Failed to record sync activity`, { diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/cleared-refs-list.ts b/apps/sim/ee/workspace-forking/components/fork-sync/cleared-refs-list.ts index d80da2d03f0..0e928112356 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/cleared-refs-list.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/cleared-refs-list.ts @@ -58,14 +58,20 @@ export function splitForkClearedRefs(visibleRefs: ForkClearedRef[]): { return { blockers, informational } } -/** Human label per blocker kind for the resolution copy (singular, lowercase mid-sentence). */ -const BLOCKER_KIND_LABEL: Record = { +/** + * Human label per remap kind for the resolution copy (singular, lowercase mid-sentence). Shared + * with the Mappings section's source-deleted note so both phrase the same resolution identically. + * `credential` is reachable only from a mapping entry - credentials gate through the required + * check, never through the cleared-ref blockers. + */ +export const FORK_RESOURCE_KIND_LABEL: Record = { table: 'table', 'knowledge-base': 'knowledge base', file: 'file', 'custom-tool': 'custom tool', skill: 'skill', 'mcp-server': 'MCP server', + credential: 'credential', } /** @@ -79,7 +85,7 @@ export function forkBlockerResolution(ref: ForkClearedRef): string | null { case 'unmapped-copyable': return 'map it to a target or select it for copy' case 'source-deleted': - return `deleted in the source — map it to an existing ${BLOCKER_KIND_LABEL[ref.kind] ?? 'resource'} in the target` + return `deleted in the source — map it to an existing ${FORK_RESOURCE_KIND_LABEL[ref.kind] ?? 'resource'} in the target` case 'workflow-missing': return `deploy "${ref.sourceLabel}" in the source or remove the reference` } diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/copy-reconciliation.ts b/apps/sim/ee/workspace-forking/components/fork-sync/copy-reconciliation.ts index 1cb90b2bc8a..f1896d9d19c 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/copy-reconciliation.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/copy-reconciliation.ts @@ -83,38 +83,39 @@ export function forkParentResolution( } /** - * Whether every required reference is satisfied - it has a mapping target OR is selected for copy. - * The server accepts a copy as resolving a required ref (promote.ts `willResolve`), so the client - * gate must too. No double-count: a mapped copyable is excluded from the copy candidates, so the two - * branches are mutually exclusive. + * Whether every required reference is satisfied - it has a mapping target, or its key is in + * `satisfiedKeys` (selected for copy, or acknowledged as a dropped source-deleted reference). + * The server accepts both as resolving a required ref, so the client gate must too. No + * double-count: a mapped copyable is excluded from the copy candidates, and a droppable reference + * is source-deleted, so it has no copy candidate either. */ export function isForkRequiredComplete( entries: ForkMappingEntry[], targets: Record, - copyingKeys: ReadonlySet + satisfiedKeys: ReadonlySet ): boolean { return entries.every( (entry) => !entry.required || effectiveForkTarget(entry, targets) !== '' || - copyingKeys.has(forkRefKey(entry)) + satisfiedKeys.has(forkRefKey(entry)) ) } /** - * Whether any reference in a kind is required AND still unmapped AND not selected for copy - drives - * the mapping summary's amber "pending" badge. Mirrors {@link isForkRequiredComplete}'s satisfied rule. + * Whether any reference in a kind is required AND still unmapped AND not satisfied another way - + * drives the mapping summary's amber "pending" badge. Mirrors {@link isForkRequiredComplete}. */ export function forkRequiredPending( items: ForkMappingEntry[], targets: Record, - copyingKeys: ReadonlySet + satisfiedKeys: ReadonlySet ): boolean { return items.some( (entry) => entry.required && effectiveForkTarget(entry, targets) === '' && - !copyingKeys.has(forkRefKey(entry)) + !satisfiedKeys.has(forkRefKey(entry)) ) } diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx index 0423b05b3f8..a3ab75a55d2 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx +++ b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx @@ -4,6 +4,7 @@ import { type Dispatch, Fragment, type SetStateAction, useMemo, useState } from import { Badge, ChevronDown, + Chip, ChipCombobox, ChipSwitch, CollapsibleCard, @@ -18,6 +19,7 @@ import type { ForkDependentReconfig, ForkMappingEntry, ForkResourceUsage, + ForkTriggerMapping, } from '@/lib/api/contracts/workspace-fork' import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state' import { SettingsSection } from '@/app/workspace/[workspaceId]/settings/components/settings-section/settings-section' @@ -25,7 +27,10 @@ import { FileKindRow, ResourceKindRow, } from '@/ee/workspace-forking/components/fork-resource-picker/fork-resource-picker' -import { forkBlockerResolution } from '@/ee/workspace-forking/components/fork-sync/cleared-refs-list' +import { + FORK_RESOURCE_KIND_LABEL, + forkBlockerResolution, +} from '@/ee/workspace-forking/components/fork-sync/cleared-refs-list' import { forkRefKey } from '@/ee/workspace-forking/components/fork-sync/copy-reconciliation' import { DependentFieldSelector } from '@/ee/workspace-forking/components/fork-sync/dependent-field-selector' import { @@ -39,7 +44,9 @@ import type { ForkSyncController, } from '@/ee/workspace-forking/components/fork-sync/use-fork-sync' import type { ForkDirection } from '@/ee/workspace-forking/hooks/workspace-fork' +import { forkSyncBlockerReasonFor } from '@/ee/workspace-forking/lib/promote/sync-blockers' import type { SelectorKey } from '@/hooks/selectors/types' +import { buildWebhookTriggerUrl } from '@/triggers/webhook-url' /** * Copyable kinds as expandable rows in the "Copy resources" section, ordered + labeled to match @@ -65,6 +72,12 @@ const COPYABLE_KIND_SECTIONS: ReadonlyArray<{ */ const NEW_COPY_VALUE = '__new_copy__' +/** + * Sentinel option value for "New URL" - the trigger mints a fresh public URL instead of taking + * over a retiring one. Sent as `adoptPath: null`. + */ +const NEW_TRIGGER_URL_VALUE = '__new_trigger_url__' + /** Fixed target-picker width so every mapping row's control lines up as one column (mirrors General). */ const MAPPING_TARGET_TRIGGER_CLASS = 'w-[240px] flex-shrink-0' @@ -390,6 +403,13 @@ function MappingEntry({ controller, group, entry }: MappingEntryProps) { />
+ {entry.sourceDeleted ? ( +

+ Deleted in the source — its name can't be shown. Map it to an existing{' '} + {FORK_RESOURCE_KIND_LABEL[entry.kind] ?? 'resource'} in the target, or fix the reference + in the source and redeploy. +

+ ) : null} {entry.candidatesTruncated ? (

More options than shown — search by name. @@ -561,6 +581,84 @@ function CopyKindSections({ controller, byKind }: CopyKindSectionsProps) { ) } +interface TriggerMappingRowProps { + controller: ForkSyncController + mapping: ForkTriggerMapping +} + +/** + * One arriving trigger's URL decision: take over a URL that is retiring in the same target + * workflow, or mint a new one. + * + * Keyed and labelled by BLOCK NAME rather than the raw path - it is one block to one webhook URL, + * and the name is what the user recognises. Adopting keeps the external caller (a Slack Request + * URL, a provider subscription) working with no re-registration at all. + */ +function TriggerMappingRow({ controller, mapping }: TriggerMappingRowProps) { + // A trigger that already serves a URL keeps it, so the row states the URL and offers no + // control. Only a trigger the sync would give a NEW URL has something to decide. + const decidable = mapping.ownPath === null && mapping.adoptablePaths.length > 0 + const chosen = + mapping.sourceBlockId in controller.triggerAdoptions + ? controller.triggerAdoptions[mapping.sourceBlockId] + : (mapping.defaultAdoptPath ?? '') + const resultingPath = mapping.ownPath ?? (chosen === '' ? null : chosen) + + return ( +

+
+ {/* One inner span, so the name and its "in " suffix share a normal inline flow: + `Label` is inline-flex, and a flex container DISCARDS whitespace-only children, which + eats the separating space (and leaves `truncate` with no text run to clip). */} + +
+ {decidable ? ( + ({ + label: + mapping.adoptablePaths.length === 1 + ? 'Keep existing URL' + : `Keep …${path.slice(-12)}`, + value: path, + })), + { label: 'Generate new URL', value: NEW_TRIGGER_URL_VALUE }, + ]} + value={chosen === '' ? NEW_TRIGGER_URL_VALUE : chosen} + onChange={(value) => + controller.setTriggerAdoption( + mapping.sourceBlockId, + value === NEW_TRIGGER_URL_VALUE ? '' : value + ) + } + placeholder='Generate new URL' + /> + ) : ( +

Unchanged

+ )} +
+
+

+ {resultingPath ? ( + {buildWebhookTriggerUrl(resultingPath)} + ) : ( + 'Gets a new URL on sync — register it with the calling service afterwards.' + )} +

+
+ ) +} + interface ForkSyncViewProps { controller: ForkSyncController onDirectionChange: (direction: ForkDirection) => void @@ -574,7 +672,10 @@ interface ForkSyncViewProps { */ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProps) { const detailsError = controller.errorMessage ?? controller.diffErrorMessage - const headsUp = controller.mcpReauthCount > 0 || controller.inlineSecretCount > 0 + const headsUp = + controller.mcpReauthCount > 0 || + controller.inlineSecretCount > 0 || + controller.triggerUrlChanges.length > 0 // Excluded workflows render greyed in the change list. Orient each name's tooltip // to WHERE it is excluded (that's the only place it can be re-included): the sync's @@ -694,6 +795,20 @@ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProp target workspace. ) : null} + {controller.triggerUrlChanges.map((change) => ( +
+ + A webhook URL in {change.workflowName} + {' '} + stops being served — anything calling it will stop working. + + {buildWebhookTriggerUrl(change.path)} + +
+ ))} ) : null} @@ -722,6 +837,20 @@ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProp ) : null} + {controller.triggerMappings.length > 0 ? ( + +
+ {controller.triggerMappings.map((mapping) => ( + + ))} +
+ + ) : null} + {controller.hasVisibleCopyables ? (
@@ -743,18 +872,51 @@ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProp ) : null} {controller.blockingRefs.length > 0 ? ( - + 1 ? ( + Drop all deleted + ) : undefined + } + >
- {controller.blockingRefs.map((ref, index) => ( -
- {ref.blockLabel} would lose{' '} - {ref.fieldLabel} in{' '} - {ref.workflowName} — {forkBlockerResolution(ref)} -
- ))} + {controller.blockingRefs.map((ref, index) => { + const dropKey = `${ref.kind}:${ref.sourceId}` + const uses = controller.blockingUsesByResource.get(dropKey) ?? 1 + return ( +
+ + {ref.blockLabel} would lose{' '} + {ref.fieldLabel} in{' '} + {ref.workflowName} — {forkBlockerResolution(ref)} + + {/* Only a source-deleted reference can be dropped: an unmapped copyable can still + be copied and a missing workflow can still be deployed, so neither is a dead + end the user should be able to accept away. + + One control per RESOURCE, not per row: the resource is gone, so the sync + clears every field naming it (the remapper's clear resolves by reference, not + by field). Rendering a Drop on each row would imply a per-field choice the + write path cannot honour, so later rows for the same id state the scope + instead. */} + {forkSyncBlockerReasonFor(ref) !== + 'source-deleted' ? null : controller.firstBlockingRowForResource.get(dropKey) === + index ? ( + controller.toggleDroppedRef(ref.kind, ref.sourceId, true)}> + {uses > 1 ? `Drop from ${uses} fields` : 'Drop'} + + ) : ( + + same reference + + )} +
+ ) + })}
) : null} @@ -762,16 +924,30 @@ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProp {controller.dependentClears.length > 0 ? (
- {controller.dependentClears.map((ref, index) => ( -
- {ref.blockLabel} will lose{' '} - {ref.fieldLabel} in{' '} - {ref.workflowName} -
- ))} + {controller.dependentClears.map((ref, index) => { + const droppedKey = `${ref.kind}:${ref.sourceId}` + const dropped = controller.droppedRefs.has(droppedKey) + return ( +
+ + {ref.blockLabel} will lose{' '} + {ref.fieldLabel} in{' '} + {ref.workflowName} + {dropped ? ' — dropped' : ''} + + {dropped ? ( + controller.toggleDroppedRef(ref.kind, ref.sourceId, false)} + > + Undo + + ) : null} +
+ ) + })}

Re-pick these in the target after the sync. diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts index 15f70c53e4d..54b3669a249 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts @@ -9,6 +9,8 @@ import type { ForkDependentReconfig, ForkMappingEntry, ForkResourceUsage, + ForkTriggerMapping, + ForkTriggerUrlChange, ForkWorkflowChange, } from '@/lib/api/contracts/workspace-fork' import { @@ -40,6 +42,7 @@ import { usePromoteFork, useUpdateForkMapping, } from '@/ee/workspace-forking/hooks/workspace-fork' +import { forkSyncBlockerReasonFor } from '@/ee/workspace-forking/lib/promote/sync-blockers' /** * The mapping kinds that can be a standalone mapping entry. `knowledge-document` is excluded: @@ -92,6 +95,12 @@ export interface ForkKindSummary { export interface ForkSyncController { direction: ForkDirection otherWorkspaceName: string + /** + * The workspace this sync WRITES, named for user-facing copy: the other workspace on push, + * "this workspace" on pull. Derived once here so every surface that names the target - the + * overwrite confirm, the Trigger URLs heading - says the same thing. + */ + targetWorkspaceName: string isLoading: boolean isError: boolean errorMessage: string | null @@ -140,6 +149,25 @@ export interface ForkSyncController { /** The raw copy selection (visible-ness not applied), for per-kind selected-id derivation. */ copySelected: ReadonlySet toggleCopyKeys: (keys: string[], checked: boolean) => void + /** + * Source-deleted references the user accepted losing in the target, keyed `${kind}:${sourceId}`. + * In-session only - an acknowledgment is a decision about this sync, never a stored mapping. + */ + droppedRefs: ReadonlySet + /** Toggle one acknowledgment; the row leaves "Blocking sync" for "Will be cleared". */ + toggleDroppedRef: (kind: string, sourceId: string, dropped: boolean) => void + /** Accept losing every source-deleted blocker at once - the volume is the point. */ + dropAllDeletedRefs: () => void + /** Source-deleted blockers still awaiting a decision, for the bulk affordance. */ + droppableBlockerCount: number + /** + * How many blocking rows name each resource, keyed `${kind}:${sourceId}`. A drop is inherently + * resource-scoped - the remapper clears by reference, not by field - so the row that offers the + * control states how many fields it covers rather than implying a per-field choice. + */ + blockingUsesByResource: ReadonlyMap + /** Index of the row that owns each resource's Drop control, so it renders exactly once. */ + firstBlockingRowForResource: ReadonlyMap /** Visible copy candidates split by referenced-ness, grouped per kind for the section rows. */ referencedByKind: ReadonlyMap unreferencedByKind: ReadonlyMap @@ -152,6 +180,16 @@ export interface ForkSyncController { workflowChanges: ForkWorkflowChange[] /** Names of target workflows this sync archives, for the confirm modal. */ archivedWorkflowNames: string[] + /** Public trigger URLs this sync would stop serving in the target (warn before overwriting). */ + triggerUrlChanges: ForkTriggerUrlChange[] + /** Arriving triggers whose URL is a choice: keep a retiring one, or mint a new one. */ + triggerMappings: ForkTriggerMapping[] + /** + * The chosen adoption per source trigger block. A key present with a path adopts it; present + * with `''` mints a new URL; absent takes the server's `defaultAdoptPath`. + */ + triggerAdoptions: Readonly> + setTriggerAdoption: (sourceBlockId: string, path: string) => void /** Names of deployed SOURCE workflows marked "Exclude from sync" - never sent. */ excludedSourceWorkflows: string[] /** Names of mapped TARGET workflows marked "Exclude from sync" - never replaced or archived. */ @@ -239,6 +277,16 @@ export function useForkSync(params: { // sync so their references resolve to the copy instead of being cleared. const [copySelected, setCopySelected] = useState>(new Set()) const [copyDefaulted, setCopyDefaulted] = useState(false) + // Source-deleted references the user explicitly accepted losing in the target (keyed by + // `${kind}:${sourceId}`). In-session only, like `copySelected` - an acknowledgment is a decision + // about THIS sync, never a stored mapping. The server re-checks that each source really is gone + // before honouring one. + const [droppedRefs, setDroppedRefs] = useState>(new Set()) + // Which retiring public URL each arriving trigger takes over, keyed by SOURCE block id. Session + // state like the two above: the choice is about THIS sync, and once it lands the adopted path is + // stored in the target block's `triggerPath`, so later syncs preserve it with no input at all. + // `''` is the explicit "mint a new URL" choice, distinct from an absent key (take the default). + const [triggerAdoptions, setTriggerAdoptions] = useState>({}) const [submitting, setSubmitting] = useState(false) // Drop every in-session choice when the direction (or edge) changes - the mapping set, @@ -248,6 +296,8 @@ export function useForkSync(params: { setReconfig({}) setCopySelected(new Set()) setCopyDefaulted(false) + setDroppedRefs(new Set()) + setTriggerAdoptions({}) }, [direction, otherWorkspaceId]) const mapping = useForkMapping({ workspaceId, otherWorkspaceId, direction, enabled }) @@ -266,6 +316,10 @@ export function useForkSync(params: { [diff.data?.copyableUnmapped] ) const clearedRefs = useMemo(() => diff.data?.clearedRefs ?? [], [diff.data?.clearedRefs]) + const triggerMappings = useMemo( + () => diff.data?.triggerMappings ?? [], + [diff.data?.triggerMappings] + ) // Keys the backend offers as copy candidates, so the entry rows show a "Copy instead" // affordance only for those - clearing a name-match suggestion returns the ref to the copy @@ -298,6 +352,16 @@ export function useForkSync(params: { [visibleCopyables, copySelected] ) + /** + * Keys that no longer need a mapping target: selected for copy, or an acknowledged drop. Kept + * separate from `copyingKeys` so a dropped reference is never counted as "copied" in the + * per-kind badge. + */ + const satisfiedKeys = useMemo(() => { + if (droppedRefs.size === 0) return copyingKeys + return new Set([...copyingKeys, ...droppedRefs]) + }, [copyingKeys, droppedRefs]) + // Group the visible copy candidates by kind so each renders as its own expandable section // (chevron + tri-state select-all + count), matching the fork picker. Referenced and // unreferenced candidates group separately: unreferenced ones (used by no synced workflow) @@ -448,7 +512,7 @@ export function useForkSync(params: { // A required reference is satisfied when it has a mapping target OR the user selected it for // copy (the server accepts a copy as resolving a required ref). See `isForkRequiredComplete`. - const requiredComplete = isForkRequiredComplete(entries, targets, copyingKeys) + const requiredComplete = isForkRequiredComplete(entries, targets, satisfiedKeys) // Every required dependent whose parent is RESOLVED must have a value before sync. Under a // mapped parent the user re-picks against the target; under a copy-resolved parent the field @@ -494,8 +558,20 @@ export function useForkSync(params: { const mapped = entry ? (targets[key] ?? entry.targetId ?? '') !== '' : false return mapped || copyingKeys.has(key) } - return splitForkClearedRefs(selectVisibleClearedRefs(clearedRefs, isResolved)) - }, [clearedRefs, entriesByParent, targets, copyingKeys]) + const { blockers, informational } = splitForkClearedRefs( + selectVisibleClearedRefs(clearedRefs, isResolved) + ) + if (droppedRefs.size === 0) return { blockers, informational } + // An acknowledged drop stops blocking and moves into the informational "Will be cleared" + // list, mirroring the server: it filters the same entries out of its own gate, but only + // after re-checking that each source really is gone. + const dropped = blockers.filter((ref) => droppedRefs.has(`${ref.kind}:${ref.sourceId}`)) + if (dropped.length === 0) return { blockers, informational } + return { + blockers: blockers.filter((ref) => !droppedRefs.has(`${ref.kind}:${ref.sourceId}`)), + informational: [...informational, ...dropped], + } + }, [clearedRefs, entriesByParent, targets, copyingKeys, droppedRefs]) // Per-kind status for the Mappings summary: "Fully mapped" or "n/total mapped", flagged when // a REQUIRED target is still missing (which blocks Sync). Reads the effective @@ -510,7 +586,7 @@ export function useForkSync(params: { const copied = group.items.filter((entry) => copyingKeys.has(entryKey(entry))).length // Mirror the Sync gate: a required ref selected for copy is satisfied, so it is not // "pending". - const requiredPending = forkRequiredPending(group.items, targets, copyingKeys) + const requiredPending = forkRequiredPending(group.items, targets, satisfiedKeys) const reconfigPending = reconfigPendingByKind.has(group.kind) return { kind: group.kind, total, mapped, copied, requiredPending, reconfigPending } }) @@ -665,9 +741,51 @@ export function useForkSync(params: { ) } + const toggleDroppedRef = (kind: string, sourceId: string, dropped: boolean) => { + const key = `${kind}:${sourceId}` + setDroppedRefs((prev) => { + const next = new Set(prev) + if (dropped) next.add(key) + else next.delete(key) + return next + }) + } + + // Only `source-deleted` blockers are droppable: an unmapped-copyable can be copied and a + // missing workflow can be deployed, so neither is a dead end the user should be able to accept. + const droppableBlockerKeys = useMemo( + () => + blockingRefs + .filter((ref) => forkSyncBlockerReasonFor(ref) === 'source-deleted') + .map((ref) => `${ref.kind}:${ref.sourceId}`), + [blockingRefs] + ) + + const dropAllDeletedRefs = () => { + setDroppedRefs((prev) => new Set([...prev, ...droppableBlockerKeys])) + } + + // Blocking rows indexed by the resource they name, so the Drop control renders once per resource + // and can state how many fields it covers - matching what the sync actually does. + const { blockingUsesByResource, firstBlockingRowForResource } = useMemo(() => { + const uses = new Map() + const firstRow = new Map() + blockingRefs.forEach((ref, index) => { + const key = `${ref.kind}:${ref.sourceId}` + uses.set(key, (uses.get(key) ?? 0) + 1) + if (!firstRow.has(key)) firstRow.set(key, index) + }) + return { blockingUsesByResource: uses, firstBlockingRowForResource: firstRow } + }, [blockingRefs]) + + const setTriggerAdoption = (sourceBlockId: string, path: string) => { + setTriggerAdoptions((prev) => ({ ...prev, [sourceBlockId]: path })) + } + const discard = () => { setTargets({}) setReconfig({}) + setTriggerAdoptions({}) } const sync = async () => { @@ -685,6 +803,27 @@ export function useForkSync(params: { const selectedCopyables = visibleCopyables.filter((candidate) => copySelected.has(forkRefKey(candidate)) ) + // Acknowledged drops, captured at confirm time like every other payload. The server honours + // one only after re-checking that the source resource is genuinely gone. + const dropReferences = Array.from(droppedRefs).map((key) => { + const separator = key.indexOf(':') + return { + kind: key.slice(0, separator) as ForkMappingEntry['kind'], + sourceId: key.slice(separator + 1), + } + }) + // Only the choices that DIFFER from the server's default need sending - an untouched row is + // already what the server would pick, so an empty list means "the preview, as shown". + const triggerMappingOverrides = triggerMappings + .filter( + (mapping) => + mapping.sourceBlockId in triggerAdoptions && + (triggerAdoptions[mapping.sourceBlockId] || null) !== mapping.defaultAdoptPath + ) + .map((mapping) => ({ + sourceBlockId: mapping.sourceBlockId, + adoptPath: triggerAdoptions[mapping.sourceBlockId] || null, + })) try { await updateMapping.mutateAsync({ workspaceId, @@ -716,6 +855,10 @@ export function useForkSync(params: { // existing store is left untouched. ...(dependentValues !== null ? { dependentValues } : {}), ...(selectedCopyables.length > 0 ? { copyResources } : {}), + ...(dropReferences.length > 0 ? { dropReferences } : {}), + ...(triggerMappingOverrides.length > 0 + ? { triggerMappings: triggerMappingOverrides } + : {}), }, }) @@ -751,11 +894,26 @@ export function useForkSync(params: { // Activity entry (needsConfiguration/clearedOptional are recorded there) and a // needs-config workflow visibly stays undeployed. Deploy FAILURES remain a real, // actionable outcome, so they keep a warning. + const dropped = result.droppedReferences.length + // Naming the dropped count is the point of making the drop explicit: the fields really are + // blank in the target now, and the server reports only the acknowledgments it honoured. + const droppedSuffix = + dropped > 0 ? ` ${dropped} deleted reference${dropped === 1 ? '' : 's'} dropped.` : '' + // A dead webhook URL fails silently and externally - nothing in the app breaks - so the one + // moment the user can act on it is right after the sync that killed it. + const deadUrls = result.triggerUrlChanges.length + const urlSuffix = + deadUrls > 0 + ? ` ${deadUrls} webhook URL${deadUrls === 1 ? '' : 's'} stopped being served — re-register ${deadUrls === 1 ? 'it' : 'them'}.` + : '' + const suffix = `${droppedSuffix}${urlSuffix}` if (result.deployFailed > 0) { const n = result.deployFailed toast.warning( - `${label}, but ${n} workflow${n === 1 ? '' : 's'} failed to deploy — open and redeploy ${n === 1 ? 'it' : 'them'}.` + `${label}, but ${n} workflow${n === 1 ? '' : 's'} failed to deploy — open and redeploy ${n === 1 ? 'it' : 'them'}.${suffix}` ) + } else if (suffix !== '') { + toast.warning(`${label}.${suffix}`) } else { toast.success(label) } @@ -769,6 +927,7 @@ export function useForkSync(params: { return { direction, otherWorkspaceName, + targetWorkspaceName: direction === 'push' ? otherWorkspaceName : 'this workspace', isLoading: enabled && mapping.isLoading, isError: mapping.isError, errorMessage: mapping.isError ? getErrorMessage(mapping.error, 'Failed to load mapping') : null, @@ -793,6 +952,12 @@ export function useForkSync(params: { copyingKeys, copySelected, toggleCopyKeys, + droppedRefs, + toggleDroppedRef, + dropAllDeletedRefs, + droppableBlockerCount: droppableBlockerKeys.length, + blockingUsesByResource, + firstBlockingRowForResource, referencedByKind, unreferencedByKind, hasVisibleCopyables: visibleCopyables.length > 0, @@ -800,6 +965,10 @@ export function useForkSync(params: { dependentClears, workflowChanges, archivedWorkflowNames, + triggerUrlChanges: diff.data?.triggerUrlChanges ?? [], + triggerMappings, + triggerAdoptions, + setTriggerAdoption, excludedSourceWorkflows: diff.data?.excludedSourceWorkflows ?? [], excludedTargetWorkflows: diff.data?.excludedTargetWorkflows ?? [], mcpReauthCount: diff.data?.mcpReauthServerIds.length ?? 0, diff --git a/apps/sim/ee/workspace-forking/components/forks.tsx b/apps/sim/ee/workspace-forking/components/forks.tsx index 6cc9789acd1..c8582141d6b 100644 --- a/apps/sim/ee/workspace-forking/components/forks.tsx +++ b/apps/sim/ee/workspace-forking/components/forks.tsx @@ -46,6 +46,7 @@ import { } from '@/ee/workspace-forking/hooks/workspace-fork' import { useWorkspaceCreationPolicy, useWorkspacesQuery } from '@/hooks/queries/workspace' import { useSettingsNavigation } from '@/hooks/use-settings-navigation' +import { buildWebhookTriggerUrl } from '@/triggers/webhook-url' /** Explains a disabled lineage action whose target workspace the viewer cannot open. */ const NO_ACCESS_TOOLTIP = "You don't have access to this workspace" @@ -152,7 +153,7 @@ function ForkSyncDetailView({ }, ] - const targetWorkspaceName = direction === 'push' ? otherWorkspaceName : 'this workspace' + const targetWorkspaceName = controller.targetWorkspaceName return ( <> @@ -224,6 +225,36 @@ function ForkSyncDetailView({ ) : null}

) : null} + {/* A dead trigger URL is only discoverable after the fact, when the external caller goes + quiet - so it belongs in the confirm, next to the other irreversible consequences. */} + {controller.triggerUrlChanges.length > 0 ? ( +
+

+ {controller.triggerUrlChanges.length === 1 ? 'A webhook URL' : 'Webhook URLs'} in{' '} + {targetWorkspaceName} will stop being served — + anything calling {controller.triggerUrlChanges.length === 1 ? 'it' : 'them'} breaks + until you re-register: +

+ {controller.triggerUrlChanges.slice(0, ARCHIVED_PREVIEW_LIMIT).map((change) => ( + // Naming the URL, not just its workflow: several URLs in one workflow would render + // as identical lines, and this confirm is the last point before they stop serving. +
+ {change.workflowName} + + {buildWebhookTriggerUrl(change.path)} + +
+ ))} + {controller.triggerUrlChanges.length > ARCHIVED_PREVIEW_LIMIT ? ( +
+ and {controller.triggerUrlChanges.length - ARCHIVED_PREVIEW_LIMIT} more +
+ ) : null} +
+ ) : null} ) diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts index 330fdb2a025..44913d6381c 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts @@ -354,3 +354,81 @@ describe('copyWorkflowStateIntoTarget canonicalModes reindex propagation', () => } ) }) + +describe('copyWorkflowStateIntoTarget webhook path pinning', () => { + const sourceState = { + blocks: { + 'blk-src': { + id: 'blk-src', + type: 'slack', + name: 'Slack', + // The SOURCE's own path, written back into its draft after its deploy. Copying it would + // point the target at the source's URL, so the sanitizer strips it. + subBlocks: { triggerPath: { id: 'triggerPath', type: 'short-input', value: 'src-path' } }, + outputs: {}, + enabled: true, + }, + }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + } as never + + const baseParams = { + targetWorkflowId: 'wf-tgt', + targetWorkspaceId: 'ws-target', + userId: 'target-user', + mode: 'replace' as const, + now: new Date('2026-07-01'), + sourceState, + sourceMeta: { name: 'Prod', description: null, folderId: null, sortOrder: 0 }, + workflowIdMap: new Map(), + folderIdMap: new Map(), + nameRegistry: buildWorkflowNameRegistry([]), + resolveBlockId: (_targetWorkflowId: string, sourceBlockId: string) => `tgt-${sourceBlockId}`, + } + + /** `replace` mode updates the existing target workflow row; stub just that chain. */ + const stubTx = () => + ({ + update: () => ({ set: () => ({ where: () => Promise.resolve() }) }), + }) as unknown as DbOrTx + + function writtenSubBlocks() { + const state = mockSaveWorkflowToNormalizedTables.mock.calls.at(-1)?.[1] as { + blocks: Record }> + } + return state.blocks['tgt-blk-src'].subBlocks ?? {} + } + + it("pins the TARGET's live webhook path so a sync never moves a URL already in the wild", async () => { + mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true }) + await copyWorkflowStateIntoTarget({ + ...baseParams, + tx: stubTx(), + triggerPathByBlockId: new Map([['tgt-blk-src', 'parent-live-path']]), + }) + expect(writtenSubBlocks().triggerPath?.value).toBe('parent-live-path') + }) + + /** + * The adoption case: the arriving trigger has a different target block id (re-created in the + * source), and the resolver handed it the URL retiring in the same target workflow. + */ + it('writes an ADOPTED path onto a trigger block that serves no webhook of its own', async () => { + mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true }) + await copyWorkflowStateIntoTarget({ + ...baseParams, + tx: stubTx(), + triggerPathByBlockId: new Map([['tgt-blk-src', 'retiring-slack-path']]), + }) + expect(writtenSubBlocks().triggerPath?.value).toBe('retiring-slack-path') + }) + + it('leaves the path unset when the target block serves no webhook yet (derives as before)', async () => { + mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true }) + await copyWorkflowStateIntoTarget({ ...baseParams, tx: stubTx() }) + expect(writtenSubBlocks().triggerPath).toBeUndefined() + }) +}) diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts index 0e6ac294fb2..2d6fec8dbb2 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts @@ -362,6 +362,13 @@ export interface CopyWorkflowStateParams { * creation, where every id is derived fresh. */ resolveBlockId?: ForkBlockIdResolver + /** + * The resolved public webhook path per TARGET block id - the block's own live path, or a + * retiring one it adopts (see `resolveForkTriggerPaths`). Pinned into the target block's + * `triggerPath` so the URL stops being a derivation of the block id this sync assigns. + * Omitted on fork creation, where the child has no webhooks yet. + */ + triggerPathByBlockId?: ReadonlyMap requestId?: string } @@ -394,6 +401,7 @@ export async function copyWorkflowStateIntoTarget( dependentOverrides, nameRegistry, resolveBlockId, + triggerPathByBlockId, requestId = 'unknown', } = params @@ -438,6 +446,19 @@ export async function copyWorkflowStateIntoTarget( const sourceSubBlocks = (block.subBlocks ?? {}) as unknown as SubBlockRecord const sanitizedSource = sanitizeSubBlocksForDuplicate(sourceSubBlocks) let subBlocks: SubBlockRecord = sanitizedSource + // The sanitizer strips `triggerPath` (the SOURCE's URL must never be copied). Pin the + // TARGET's resolved path back in - the one this block already serves, or a retiring one it + // adopts - so the URL stops being a derivation of the block id this sync assigns. Otherwise + // any later change to that id silently re-points a URL external systems already call (a + // Slack Request URL, a provider subscription). With no resolved path the field stays empty + // and derives as before, so a first-time sync is unchanged. + const resolvedTriggerPath = triggerPathByBlockId?.get(newBlockId) + if (resolvedTriggerPath) { + subBlocks = { + ...subBlocks, + triggerPath: { id: 'triggerPath', type: 'short-input', value: resolvedTriggerPath }, + } + } // Tracks the block's live `canonicalModes` through this pass, so a `tool-input` reindex // (a dropped custom-tool/MCP entry shifts later tools' array positions) is visible to every // later step below that resolves a nested tool's basic/advanced mode - not just the final diff --git a/apps/sim/ee/workspace-forking/lib/copy/deploy-bridge.ts b/apps/sim/ee/workspace-forking/lib/copy/deploy-bridge.ts index 8bfc338bf20..759c48347d1 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/deploy-bridge.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/deploy-bridge.ts @@ -1,11 +1,12 @@ import { db, runOutsideTransactionContext } from '@sim/db' -import { workflow, workflowDeploymentVersion } from '@sim/db/schema' +import { webhook, workflow, workflowDeploymentVersion } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { and, eq, exists, inArray, isNull, sql } from 'drizzle-orm' +import { and, eq, exists, inArray, isNotNull, isNull, sql } from 'drizzle-orm' import type { DbOrTx } from '@/lib/db/types' import { loadDeployedWorkflowState } from '@/lib/workflows/persistence/utils' import { ForkError } from '@/ee/workspace-forking/lib/lineage/authz' import type { Variable, WorkflowState } from '@/stores/workflows/workflow/types' +import { isInternalTriggerProvider, isPollingWebhookProvider } from '@/triggers/constants' const logger = createLogger('WorkspaceForkDeployBridge') @@ -227,3 +228,83 @@ export async function readDeployedState( } }) } + +/** A live, path-based webhook on a target block: the URL it serves and the workflow it belongs to. */ +export interface ForkTargetWebhook { + path: string + workflowId: string + /** + * The provider the path is served under. An inbound request is authenticated and parsed as this + * provider, so a URL is only meaningfully transferable to a trigger of the SAME provider. + */ + provider: string | null +} + +/** + * The public webhook path each target trigger block currently serves on, keyed by block id. + * + * A webhook's path defaults to its block id (`triggerPath || block.id`, see + * `lib/webhooks/deploy.ts`), so the target's URL has always been a *derivation* of an id the + * sync itself assigns. Reading the live path lets the copy pin it back into the target block's + * own `triggerPath`, turning the URL into stored data - the sync then cannot move a URL that + * external systems (a Slack Request URL, a provider subscription) are already pointing at. + * + * Only rows serving a PUBLIC URL are returned. Three families are excluded, because preserving + * their path would be meaningless and offering it for adoption actively wrong: + * - shared-app providers (the native Slack trigger) route by `routingKey` with a NULL path; + * - polling providers ({@link isPollingWebhookProvider}) keep a webhook row as state, but Sim + * pulls from the provider - nothing external calls the path; + * - internal providers ({@link isInternalTriggerProvider}) register a path that the public + * trigger route deliberately rejects, so it is not an endpoint either. + * + * Scoped to each workflow's ACTIVE deployment version, exactly as inbound delivery resolves a + * path (`lib/webhooks/processor.ts`). A workflow keeps non-archived webhook rows from previous + * versions too (`lib/webhooks/deploy.ts` reads "ALL webhooks for this workflow (all versions)" + * before narrowing to the current one), so an unscoped read would return several rows per block + * and pick a stale path arbitrarily - pinning a URL nothing is actually serving, which is the + * precise failure this function exists to prevent. + */ +export async function loadTargetWebhookPathsByBlock( + executor: DbOrTx, + targetWorkflowIds: string[] +): Promise> { + if (targetWorkflowIds.length === 0) return new Map() + const rows = await executor + .select({ + blockId: webhook.blockId, + path: webhook.path, + workflowId: webhook.workflowId, + provider: webhook.provider, + }) + .from(webhook) + .innerJoin( + workflowDeploymentVersion, + and( + eq(workflowDeploymentVersion.workflowId, webhook.workflowId), + eq(workflowDeploymentVersion.isActive, true), + eq(workflowDeploymentVersion.id, webhook.deploymentVersionId) + ) + ) + .where( + and( + inArray(webhook.workflowId, targetWorkflowIds), + isNull(webhook.archivedAt), + isNotNull(webhook.blockId), + isNotNull(webhook.path) + ) + ) + const byBlock = new Map() + for (const row of rows) { + if (!row.blockId || !row.path) continue + if (isPollingWebhookProvider(row.provider ?? '') || isInternalTriggerProvider(row.provider)) { + continue + } + // One live path-based row per block within a version - `path_deployment_unique` enforces it. + byBlock.set(row.blockId, { + path: row.path, + workflowId: row.workflowId, + provider: row.provider, + }) + } + return byBlock +} diff --git a/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.test.ts b/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.test.ts index 4efb1d0778a..2a7fda4ead2 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.test.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.test.ts @@ -4,24 +4,63 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { ForkRemapKind } from '@/ee/workspace-forking/lib/remap/remap-references' -const { mockFilterExisting, mockGetCredentialProviders, mockGetEnvKeys } = vi.hoisted(() => ({ +const { + mockFilterExisting, + mockGetCredentialProviders, + mockGetEnvKeys, + mockLoadLabels, + mockListCandidates, + mockClassifyCredential, + mockListDeployedWorkflows, + mockReadDeployedState, + mockScanWorkflowReferences, + mockDetectCascade, +} = vi.hoisted(() => ({ mockFilterExisting: vi.fn(), mockGetCredentialProviders: vi.fn(), mockGetEnvKeys: vi.fn(), + mockLoadLabels: vi.fn(), + mockListCandidates: vi.fn(), + mockClassifyCredential: vi.fn(), + mockListDeployedWorkflows: vi.fn(), + mockReadDeployedState: vi.fn(), + mockScanWorkflowReferences: vi.fn(), + mockDetectCascade: vi.fn(), })) vi.mock('@/ee/workspace-forking/lib/mapping/resources', () => ({ - listForkResourceCandidates: vi.fn(), - classifyCredentialResourceType: vi.fn(), + listForkResourceCandidates: mockListCandidates, + classifyCredentialResourceType: mockClassifyCredential, getWorkspaceEnvKeys: mockGetEnvKeys, filterExistingForkTargets: mockFilterExisting, getCredentialProvidersByIds: mockGetCredentialProviders, + loadForkResourceLabels: mockLoadLabels, CANDIDATE_LIMIT: 1000, })) +vi.mock('@/ee/workspace-forking/lib/copy/deploy-bridge', () => ({ + listDeployedWorkflows: mockListDeployedWorkflows, + readDeployedState: mockReadDeployedState, +})) + +vi.mock('@/ee/workspace-forking/lib/mapping/cascade', () => ({ + detectForkCascadeReferences: mockDetectCascade, +})) + +vi.mock('@/ee/workspace-forking/lib/remap/remap-references', () => ({ + scanWorkflowReferences: mockScanWorkflowReferences, +})) + +vi.mock('@/ee/workspace-forking/lib/remap/reference-scan', () => ({ + toScannerBlocks: vi.fn((state: unknown) => state), +})) + +import { workflow, workspaceForkResourceMap } from '@sim/db/schema' +import { queueTableRows, resetDbChainMock } from '@sim/testing' import { ForkError } from '@/ee/workspace-forking/lib/lineage/authz' import { findDuplicateTargetEntry, + getForkMappingView, suggestTarget, validateForkMappingTargets, } from '@/ee/workspace-forking/lib/mapping/mapping-service' @@ -149,16 +188,32 @@ describe('validateForkMappingTargets', () => { ).resolves.toBeUndefined() }) - it('rejects a credential whose source is not a credential in the source workspace', async () => { + /** + * A source credential that no longer exists is exactly what the mapping editor asks the user + * to resolve (`sourceDeleted`), so the save must accept it - rejecting made it the one kind of + * reference the UI told you to map and the server refused. Access propagation is not driven + * from here: `propagateCredentialAccess` re-validates both sides inside the promote tx. + */ + it('accepts a credential whose source no longer exists in the source workspace', async () => { mockFilterExisting.mockResolvedValue({ credential: new Set(['cred-tgt']) }) mockGetCredentialProviders.mockImplementation(async (_db: unknown, workspaceId: string) => workspaceId === 'ws-source' - ? new Map() // cred-foreign is not in the source + ? new Map() // cred-deleted is gone from the source : new Map([['cred-tgt', 'google-email']]) ) await expect( validateForkMappingTargets('ws-source', 'ws-target', [ - { resourceType: 'oauth_credential', sourceId: 'cred-foreign', targetId: 'cred-tgt' }, + { resourceType: 'oauth_credential', sourceId: 'cred-deleted', targetId: 'cred-tgt' }, + ]) + ).resolves.toBeUndefined() + }) + + it('still rejects a target that does not exist, even when the source is gone', async () => { + mockFilterExisting.mockResolvedValue({ credential: new Set() }) + mockGetCredentialProviders.mockImplementation(async () => new Map()) + await expect( + validateForkMappingTargets('ws-source', 'ws-target', [ + { resourceType: 'oauth_credential', sourceId: 'cred-deleted', targetId: 'cred-foreign' }, ]) ).rejects.toBeInstanceOf(ForkError) }) @@ -246,3 +301,109 @@ describe('suggestTarget', () => { expect(suggestTarget('table', ' Orders ', undefined, [cand('t1', 'orders')])).toBe('t1') }) }) + +describe('getForkMappingView', () => { + const edge = { parentWorkspaceId: 'ws-parent', childWorkspaceId: 'ws-child' } as never + const emptyCandidates = { + credential: [], + 'env-var': [], + table: [], + 'knowledge-base': [], + 'mcp-server': [], + 'custom-tool': [], + skill: [], + 'knowledge-document': [], + file: [], + } + + /** Pull: parent is the source, child the target — the direction the raw-id rows showed up in. */ + function pullView(overrides: { workflowRows?: unknown[] } = {}) { + // The real `getEdgeMappingRows` runs; this row is the workflow identity pair it returns. + queueTableRows(workspaceForkResourceMap, [ + { + id: 'map-1', + childWorkspaceId: 'ws-child', + resourceType: 'workflow', + parentResourceId: 'wf-parent', + childResourceId: 'wf-child', + }, + ]) + queueTableRows( + workflow, + overrides.workflowRows ?? [{ id: 'wf-child', forkSyncExcluded: false }] + ) + return getForkMappingView({ + edge, + sourceWorkspaceId: 'ws-parent', + targetWorkspaceId: 'ws-child', + }) + } + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockGetEnvKeys.mockResolvedValue(new Set()) + mockListCandidates.mockResolvedValue(emptyCandidates) + mockListDeployedWorkflows.mockResolvedValue([{ id: 'wf-parent', name: 'Prod' }]) + mockReadDeployedState.mockResolvedValue({ blocks: {} }) + mockScanWorkflowReferences.mockReturnValue({ + references: [ + { kind: 'table', sourceId: 'tbl_live', subBlockKey: 'tableSelector', required: false }, + { kind: 'table', sourceId: 'tbl_gone', subBlockKey: 'tableSelector', required: false }, + ], + }) + mockDetectCascade.mockResolvedValue({ references: [] }) + mockFilterExisting.mockResolvedValue({}) + mockGetCredentialProviders.mockResolvedValue(new Map()) + mockClassifyCredential.mockResolvedValue('oauth_credential') + mockLoadLabels.mockResolvedValue({ table: new Map([['tbl_live', 'Orders']]) }) + }) + + it('labels a live source resource by name and flags a deleted one', async () => { + const { entries } = await pullView() + expect(entries).toEqual([ + expect.objectContaining({ + sourceId: 'tbl_live', + sourceLabel: 'Orders', + sourceDeleted: false, + }), + expect.objectContaining({ + sourceId: 'tbl_gone', + sourceLabel: 'tbl_gone', + sourceDeleted: true, + }), + ]) + }) + + /** + * The label lookup must be by exact id, never the display-capped candidate list — otherwise a + * workspace past CANDIDATE_LIMIT renders live resources as raw ids, indistinguishable from + * deleted ones. Pinned by asserting the exact ids are what gets looked up. + */ + it('looks source labels up by exact id, not through the capped candidate list', async () => { + await pullView() + expect(mockLoadLabels).toHaveBeenCalledWith(expect.anything(), 'ws-parent', { + table: new Set(['tbl_live', 'tbl_gone']), + }) + expect(mockListCandidates).toHaveBeenCalledTimes(1) + expect(mockListCandidates).toHaveBeenCalledWith(expect.anything(), 'ws-child') + }) + + it('skips a source workflow whose target is excluded from sync', async () => { + const { entries } = await pullView({ + workflowRows: [{ id: 'wf-child', forkSyncExcluded: true }], + }) + expect(entries).toEqual([]) + expect(mockReadDeployedState).not.toHaveBeenCalled() + }) + + it('still scans when the excluded flag is on an unrelated target workflow', async () => { + const { entries } = await pullView({ + workflowRows: [ + { id: 'wf-child', forkSyncExcluded: false }, + { id: 'wf-other', forkSyncExcluded: true }, + ], + }) + expect(entries).toHaveLength(2) + }) +}) diff --git a/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.ts b/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.ts index 64f3ca2d91a..8ad23f68d0f 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/mapping-service.ts @@ -1,4 +1,6 @@ import { db } from '@sim/db' +import { workflow } from '@sim/db/schema' +import { and, eq, isNull } from 'drizzle-orm' import type { ForkMappableResourceType, ForkMappingEntry } from '@/lib/api/contracts/workspace-fork' import type { DbOrTx } from '@/lib/db/types' import { @@ -25,7 +27,9 @@ import { getCredentialProvidersByIds, getWorkspaceEnvKeys, listForkResourceCandidates, + loadForkResourceLabels, } from '@/ee/workspace-forking/lib/mapping/resources' +import { resolveForkExcludedTargetId } from '@/ee/workspace-forking/lib/promote/promote-plan' import { toScannerBlocks } from '@/ee/workspace-forking/lib/remap/reference-scan' import { type ForkReference, @@ -66,13 +70,16 @@ export async function getForkMappingView( const { edge, sourceWorkspaceId, targetWorkspaceId } = params const sourceIsParent = sourceWorkspaceId === edge.parentWorkspaceId - const [mappingRows, targetEnvKeys, sourceEnvKeys, sourceCandidates, targetCandidates] = + const [mappingRows, targetEnvKeys, sourceEnvKeys, targetCandidates, targetWorkflows] = await Promise.all([ getEdgeMappingRows(db, edge.childWorkspaceId), getWorkspaceEnvKeys(db, targetWorkspaceId), getWorkspaceEnvKeys(db, sourceWorkspaceId), - listForkResourceCandidates(db, sourceWorkspaceId), listForkResourceCandidates(db, targetWorkspaceId), + db + .select({ id: workflow.id, forkSyncExcluded: workflow.forkSyncExcluded }) + .from(workflow) + .where(and(eq(workflow.workspaceId, targetWorkspaceId), isNull(workflow.archivedAt))), ]) const resolver = buildForkResolver(mappingRows, { sourceIsParent, targetEnvKeys, sourceEnvKeys }) @@ -93,11 +100,30 @@ export async function getForkMappingView( if (key) resourceTypeBySourceId.set(key, row.resourceType) } + // The workflow identity map + the target's live/excluded sets, so this view scans exactly the + // workflows a sync would write. Without the exclusion filter a source whose target is marked + // "Exclude from sync" still contributed blocking mapping entries the sync could never act on. + const identityMap = new Map() + for (const row of mappingRows) { + if (row.resourceType !== 'workflow' || row.childResourceId == null) continue + if (sourceIsParent) identityMap.set(row.parentResourceId, row.childResourceId) + else identityMap.set(row.childResourceId, row.parentResourceId) + } + const targetActiveIds = new Set(targetWorkflows.map((w) => w.id)) + const excludedTargetIds = new Set( + targetWorkflows.filter((w) => w.forkSyncExcluded).map((w) => w.id) + ) + // Scan one deployed workflow state at a time and merge deduped references, so // peak memory stays at a single workflow state rather than all of them at once. const deployedWorkflows = await listDeployedWorkflows(db, sourceWorkspaceId) const referenceByKey = new Map() for (const wf of deployedWorkflows) { + if ( + resolveForkExcludedTargetId(wf.id, identityMap, targetActiveIds, excludedTargetIds) !== null + ) { + continue + } const state = await readDeployedState(wf.id, sourceWorkspaceId) if (!state) continue for (const reference of scanWorkflowReferences(toScannerBlocks(state), () => null).references) { @@ -116,6 +142,25 @@ export async function getForkMappingView( } const references: ForkReference[] = Array.from(referenceByKey.values()) + // Source-side labels and credential providers, both looked up by EXACT ID (never the capped + // candidate list). A capped lookup made a live resource past `CANDIDATE_LIMIT` render as a raw + // id, indistinguishable from a deleted one - and, for a credential, silently dropped the + // provider filter so the picker offered every provider's credentials. Resolved here, an id + // missing from `sourceLabels` means exactly one thing: it no longer exists in the source. + const sourceIdsByKind: Partial>> = {} + for (const reference of references) { + if (reference.kind === 'env-var' || reference.kind === 'knowledge-document') continue + ;(sourceIdsByKind[reference.kind] ??= new Set()).add(reference.sourceId) + } + const [sourceLabels, sourceProviders] = await Promise.all([ + loadForkResourceLabels(db, sourceWorkspaceId, sourceIdsByKind), + getCredentialProvidersByIds( + db, + sourceWorkspaceId, + Array.from(sourceIdsByKind.credential ?? []) + ), + ]) + // First pass: resolve each reference's stored target + the data to build its entry, // collecting stored target ids so existence is checked by exact id (cap-free) - a // valid mapping to a target past the display cap must be RETAINED, not shown unmapped. @@ -123,6 +168,7 @@ export async function getForkMappingView( reference: ForkReference resourceType: ForkMappableResourceType sourceLabel: string + sourceDeleted: boolean sourceProviderId: string | undefined candidates: ForkResourceCandidate[] storedTargetId: string | null @@ -147,11 +193,16 @@ export async function getForkMappingView( : nonCredentialForkKindToResourceType(reference.kind) } - const sourceCandidate = sourceCandidates[reference.kind].find( - (c) => c.id === reference.sourceId - ) - const sourceLabel = sourceCandidate?.label ?? reference.sourceId - const sourceProviderId = sourceCandidate?.providerId + // An env var IS its own name, so it can never be "deleted but referenced" here - a `{{KEY}}` + // absent from the source workspace was already skipped above as a personal secret. + const sourceLabel = + reference.kind === 'env-var' + ? reference.sourceId + : (sourceLabels[reference.kind]?.get(reference.sourceId) ?? reference.sourceId) + const sourceDeleted = + reference.kind !== 'env-var' && + !(sourceLabels[reference.kind]?.has(reference.sourceId) ?? false) + const sourceProviderId = sourceProviders.get(reference.sourceId) ?? undefined // A credential reference only maps to a target credential of the SAME OAuth // provider - a Gmail (google-email) reference must never offer a Google Calendar // credential. Non-credential kinds carry no provider, so their full list stands. @@ -169,6 +220,7 @@ export async function getForkMappingView( reference, resourceType, sourceLabel, + sourceDeleted, sourceProviderId, candidates, storedTargetId, @@ -212,6 +264,7 @@ export async function getForkMappingView( resourceType: p.resourceType, sourceId: p.reference.sourceId, sourceLabel: p.sourceLabel, + sourceDeleted: p.sourceDeleted, targetId, suggested, // Every entry here is a reference a synced workflow actually carries, and a sync is @@ -414,16 +467,18 @@ export async function validateForkMappingTargets( } if (kind === 'credential') { - // The source must be a real credential in the source workspace. A foreign id - // (not present) would skip the provider check and let a crafted mapping drive - // cross-workspace credential-access propagation on promote. - if (!sourceProviders.has(entry.sourceId)) { - throw new ForkError( - `Source credential "${entry.sourceId}" is not a credential in the source workspace`, - 400 - ) - } + // A source credential that no longer exists in the source workspace is EXPECTED here: the + // mapping editor deliberately lists such references (`sourceDeleted`) because mapping the + // dead id to a live target is the documented way to unblock the sync. Rejecting the save + // made that the one kind you could not resolve - the UI told you to map it and the server + // refused. Accepting it is safe: the target is still proven to belong to the target + // workspace above, and credential-ACCESS propagation is not driven from here - promote's + // `propagateCredentialAccess` re-validates BOTH sides inside its transaction and skips any + // pair whose source is not a live credential of the source workspace. const sourceProviderId = sourceProviders.get(entry.sourceId) + if (sourceProviderId === undefined) continue + // With a live source, the target must share its OAuth provider - a Gmail reference can + // never be pointed at a Google Calendar credential. const targetProviderId = targetProviders.get(targetId) ?? null if (sourceProviderId && targetProviderId !== sourceProviderId) { throw new ForkError( diff --git a/apps/sim/ee/workspace-forking/lib/mapping/resources.ts b/apps/sim/ee/workspace-forking/lib/mapping/resources.ts index 362849b5e10..8216d5c4a0a 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/resources.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/resources.ts @@ -240,21 +240,27 @@ export async function listForkResourceCandidates( } } +/** One live resource, by exact id. `label` is absent for kinds looked up by id only. */ +interface ForkResourceRow { + id: string + label?: string +} + /** - * Given mapped target ids grouped by kind, return the subset that still EXISTS in the - * target workspace (same archived/deleted filters as `listForkResourceCandidates`). - * Used at promote time so a mapping whose target was deleted after it was saved - * resolves as unmapped (surfaced/cleared) instead of writing a dead id into the - * promoted workflow. Queries the exact ids (not the capped candidate list) so a valid - * target is never wrongly dropped, and only the DB-backed kinds are checked - env-var - * existence is handled by the resolver's `targetEnvKeys`, and `file`/`workflow` are - * resolved by other paths. + * Look up the given ids, grouped by kind, in one workspace and return the rows that still EXIST + * (same archived/deleted filters as `listForkResourceCandidates`). Queries the exact ids - NOT + * the capped candidate list - so a resource sitting past `CANDIDATE_LIMIT` is never mistaken for + * a missing one. Only the DB-backed kinds are checked: env-var existence is handled by the + * resolver's `targetEnvKeys`, and `file`/`workflow` are resolved by other paths. + * + * Backs both {@link filterExistingForkTargets} (existence) and {@link loadForkResourceLabels} + * (display names), so the two can never disagree about what "exists" means. */ -export async function filterExistingForkTargets( +async function loadForkResourceRows( executor: DbOrTx, workspaceId: string, idsByKind: Partial>> -): Promise>>> { +): Promise>> { const ids = (kind: ForkRemapKind): string[] => { const set = idsByKind[kind] return set && set.size > 0 ? Array.from(set) : [] @@ -272,9 +278,9 @@ export async function filterExistingForkTargets( const [creds, tables, kbs, docs, servers, tools, skills, files] = await Promise.all([ credIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : executor - .select({ id: credential.id }) + .select({ id: credential.id, label: credential.displayName }) .from(credential) .where( and( @@ -284,15 +290,15 @@ export async function filterExistingForkTargets( ) ), tableIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : tableCandidatesQuery(executor, workspaceId, tableIds), kbIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : knowledgeBaseCandidatesQuery(executor, workspaceId, kbIds), // Documents are validated through a KB join (they are not a standalone candidate kind), so // this existence check stays inline rather than sharing a per-kind candidate query. docIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : executor .select({ id: document.id }) .from(document) @@ -307,29 +313,75 @@ export async function filterExistingForkTargets( ) ), mcpIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : mcpServerCandidatesQuery(executor, workspaceId, mcpIds), toolIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : customToolCandidatesQuery(executor, workspaceId, toolIds), skillIds.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : skillCandidatesQuery(executor, workspaceId, skillIds), fileKeys.length === 0 - ? Promise.resolve([] as Array<{ id: string }>) + ? Promise.resolve([] as ForkResourceRow[]) : fileCandidatesQuery(executor, workspaceId, fileKeys), ]) + const result: Partial> = {} + if (credIds.length > 0) result.credential = creds + if (tableIds.length > 0) result.table = tables + if (kbIds.length > 0) result['knowledge-base'] = kbs + if (docIds.length > 0) result['knowledge-document'] = docs + if (mcpIds.length > 0) result['mcp-server'] = servers + if (toolIds.length > 0) result['custom-tool'] = tools + if (skillIds.length > 0) result.skill = skills + // `fileCandidatesQuery` exposes the storage key under `id`, so file rows key by `r.id`. + if (fileKeys.length > 0) result.file = files + return result +} + +/** + * Given mapped target ids grouped by kind, return the subset that still EXISTS in the target + * workspace. Used at promote time so a mapping whose target was deleted after it was saved + * resolves as unmapped (surfaced/cleared) instead of writing a dead id into the promoted + * workflow, and by the cleared-ref collector pointed at the SOURCE workspace to flag a + * reference whose resource is gone. + */ +export async function filterExistingForkTargets( + executor: DbOrTx, + workspaceId: string, + idsByKind: Partial>> +): Promise>>> { + const rows = await loadForkResourceRows(executor, workspaceId, idsByKind) const result: Partial>> = {} - if (credIds.length > 0) result.credential = new Set(creds.map((r) => r.id)) - if (tableIds.length > 0) result.table = new Set(tables.map((r) => r.id)) - if (kbIds.length > 0) result['knowledge-base'] = new Set(kbs.map((r) => r.id)) - if (docIds.length > 0) result['knowledge-document'] = new Set(docs.map((r) => r.id)) - if (mcpIds.length > 0) result['mcp-server'] = new Set(servers.map((r) => r.id)) - if (toolIds.length > 0) result['custom-tool'] = new Set(tools.map((r) => r.id)) - if (skillIds.length > 0) result.skill = new Set(skills.map((r) => r.id)) - // `fileCandidatesQuery` exposes the storage key under `id`, so file existence keys by `r.id`. - if (fileKeys.length > 0) result.file = new Set(files.map((r) => r.id)) + for (const [kind, kindRows] of Object.entries(rows) as Array< + [ForkRemapKind, ForkResourceRow[]] + >) { + result[kind] = new Set(kindRows.map((row) => row.id)) + } + return result +} + +/** + * Display names for the given ids, grouped by kind, looked up by exact id in one workspace. + * + * The mapping view labels each scanned reference with this rather than with the capped + * `listForkResourceCandidates` output: a workspace past `CANDIDATE_LIMIT` would otherwise render + * a perfectly live resource as a raw id, indistinguishable from one that was actually deleted. + * With this, an id absent from the returned map means exactly one thing - the resource no longer + * exists in that workspace - which is what `ForkMappingEntry.sourceDeleted` reports. + */ +export async function loadForkResourceLabels( + executor: DbOrTx, + workspaceId: string, + idsByKind: Partial>> +): Promise>>> { + const rows = await loadForkResourceRows(executor, workspaceId, idsByKind) + const result: Partial>> = {} + for (const [kind, kindRows] of Object.entries(rows) as Array< + [ForkRemapKind, ForkResourceRow[]] + >) { + result[kind] = new Map(kindRows.map((row) => [row.id, row.label ?? row.id])) + } return result } diff --git a/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.test.ts b/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.test.ts index 0487a6082c9..e988d239cec 100644 --- a/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.test.ts +++ b/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.test.ts @@ -957,7 +957,7 @@ describe('collectForkSyncBlockers', () => { mockLoadCopyableLabels.mockResolvedValue( new Map([['table:tbl-src', { label: 'Orders', parentId: null, parentLabel: null }]]) ) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ sourceStates: new Map([ [ @@ -987,7 +987,7 @@ describe('collectForkSyncBlockers', () => { blockWith([{ id: 'tbl', title: 'Table', type: 'table-selector' }]) ) const { executor, select } = makeExecutor() - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1014,7 +1014,7 @@ describe('collectForkSyncBlockers', () => { ) mockFilterExisting.mockResolvedValue({ 'mcp-server': new Set(['srv-1']) }) const { executor } = makeExecutor([[{ id: 'srv-1', name: 'Internal Tools' }]]) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1037,6 +1037,58 @@ describe('collectForkSyncBlockers', () => { ]) }) + /** + * The drop hatch. `sourceDeleted` is re-derived here from the source workspace inside the + * promote transaction, so the acknowledgment is only ever honoured against a reference that is + * genuinely gone - a crafted payload can never drop a working one. + */ + it('honours a drop acknowledgment for a source-deleted reference', async () => { + vi.mocked(getBlock).mockReturnValue( + blockWith([{ id: 'kb', title: 'Knowledge Base', type: 'knowledge-base-selector' }]) + ) + mockFilterExisting.mockResolvedValue({ 'knowledge-base': new Set() }) + const { blockers, appliedDrops } = await collectForkSyncBlockers( + baseParams({ + sourceStates: new Map([ + [ + 'wf-src', + stateWith('knowledge', 'KB Block', { + kb: { type: 'knowledge-base-selector', value: 'kb-gone' }, + }), + ], + ]), + droppedReferences: [{ kind: 'knowledge-base', sourceId: 'kb-gone' }], + }) + ) + expect(blockers).toEqual([]) + expect(appliedDrops).toEqual([{ kind: 'knowledge-base', sourceId: 'kb-gone' }]) + }) + + it('ignores a drop acknowledgment for a reference whose source is still live', async () => { + vi.mocked(getBlock).mockReturnValue( + blockWith([{ id: 'kb', title: 'Knowledge Base', type: 'knowledge-base-selector' }]) + ) + // The source row still exists, so the reference is an unmapped-copyable, not source-deleted. + mockFilterExisting.mockResolvedValue({ 'knowledge-base': new Set(['kb-live']) }) + const { blockers, appliedDrops } = await collectForkSyncBlockers( + baseParams({ + sourceStates: new Map([ + [ + 'wf-src', + stateWith('knowledge', 'KB Block', { + kb: { type: 'knowledge-base-selector', value: 'kb-live' }, + }), + ], + ]), + droppedReferences: [{ kind: 'knowledge-base', sourceId: 'kb-live' }], + }) + ) + expect(blockers).toEqual([ + expect.objectContaining({ sourceId: 'kb-live', reason: 'unmapped-copyable' }), + ]) + expect(appliedDrops).toEqual([]) + }) + it('blocks a source-deleted reference (source-deleted) - no exemption, resolvable by mapping', async () => { vi.mocked(getBlock).mockReturnValue( blockWith([{ id: 'kb', title: 'Knowledge Base', type: 'knowledge-base-selector' }]) @@ -1044,7 +1096,7 @@ describe('collectForkSyncBlockers', () => { // The liveness check reports the source row gone; the copy loader (live rows only) misses, // so the label falls back to the id. mockFilterExisting.mockResolvedValue({ 'knowledge-base': new Set() }) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ sourceStates: new Map([ [ @@ -1066,7 +1118,7 @@ describe('collectForkSyncBlockers', () => { ]) // Mapping the dead id to a live target resolves it (the resolver never checks source // liveness - a mapping row whose source row is gone still resolves). - const resolved = await collectForkSyncBlockers( + const { blockers: resolved } = await collectForkSyncBlockers( baseParams({ sourceStates: new Map([ [ @@ -1095,7 +1147,7 @@ describe('collectForkSyncBlockers', () => { targetActiveIds: new Set(['wf-child-tgt']), items: [{ sourceWorkflowId: 'wf-src', targetWorkflowId: 'wf-tgt' }], }) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1132,7 +1184,7 @@ describe('collectForkSyncBlockers', () => { targetActiveIds: new Set(['wf-child-tgt']), items: [{ sourceWorkflowId: 'wf-src', targetWorkflowId: 'wf-tgt' }], }) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1167,8 +1219,8 @@ describe('collectForkSyncBlockers', () => { ], ]) - const freshScan = await collectForkSyncBlockers(baseParams({ sourceStates })) - const reusedPlan = await collectForkSyncBlockers( + const { blockers: freshScan } = await collectForkSyncBlockers(baseParams({ sourceStates })) + const { blockers: reusedPlan } = await collectForkSyncBlockers( baseParams({ sourceStates, planUnmapped: [{ kind: 'table', sourceId: 'tbl-src' }], @@ -1179,7 +1231,7 @@ describe('collectForkSyncBlockers', () => { // unchanged either way. const overlayResolver: ForkReferenceResolver = (kind, id) => kind === 'custom-tool' && id === 'ct-unreferenced' ? 'ct-copy' : null - const withIrrelevantCopy = await collectForkSyncBlockers( + const { blockers: withIrrelevantCopy } = await collectForkSyncBlockers( baseParams({ sourceStates, resolver: overlayResolver, @@ -1206,7 +1258,7 @@ describe('collectForkSyncBlockers', () => { blockWith([{ id: 'tbl', title: 'Table', type: 'table-selector' }]) ) const { executor, select } = makeExecutor() - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1230,7 +1282,7 @@ describe('collectForkSyncBlockers', () => { blockWith([{ id: 'tbl', title: 'Table', type: 'table-selector' }]) ) const { executor, select } = makeExecutor() - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1259,7 +1311,7 @@ describe('collectForkSyncBlockers', () => { blockWith([{ id: 'target', title: 'Workflow', type: 'workflow-selector' }]) ) const { executor } = makeExecutor([[{ id: 'wf-child', name: 'Child Flow' }]]) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1289,7 +1341,7 @@ describe('collectForkSyncBlockers', () => { blockWith([{ id: 'workflowIds', title: 'Workflows', type: 'dropdown', multiSelect: true }]) ) const { executor } = makeExecutor([[{ id: 'wf-watched', name: 'Watched Workflow' }]]) - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([ @@ -1351,7 +1403,7 @@ describe('collectForkSyncBlockers', () => { parallels: {}, variables: {}, } as unknown as WorkflowState - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, sourceStates: new Map([['wf-src', state]]), @@ -1376,7 +1428,7 @@ describe('collectForkSyncBlockers', () => { ]) ) const { executor, select } = makeExecutor() - const blockers = await collectForkSyncBlockers( + const { blockers } = await collectForkSyncBlockers( baseParams({ executor, items: [{ ...replaceItem, mode: 'create' as const }], diff --git a/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.ts b/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.ts index 521111dfde4..540c2e10e84 100644 --- a/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.ts +++ b/apps/sim/ee/workspace-forking/lib/promote/cleared-refs.ts @@ -300,6 +300,34 @@ export async function annotateForkClearedRefSourceLiveness( ) } +/** + * Narrow a caller's drop acknowledgments to the ones the server will actually honour: a reference + * whose kind can block at all, and whose resource is genuinely gone from the SOURCE workspace. + * + * Both promote gates consult this, so "which drops count" is decided once. The unmapped gate runs + * FIRST and would otherwise reject a dropped required reference before the cleared-ref gate ever + * got to honour it - making Drop unusable for exactly the required references it exists for. It + * cannot simply subtract the raw acknowledgments there either: an unmapped reference of a + * non-blocking kind (credential, env-var) never re-blocks downstream, so an unverified subtraction + * would let a crafted payload skip the required gate entirely. + */ +export async function verifyForkDropAcknowledgments( + executor: DbOrTx, + sourceWorkspaceId: string, + acknowledged: ReadonlyArray<{ kind: ForkRemapKind; sourceId: string }> | undefined +): Promise> { + const droppable = (acknowledged ?? []).filter( + (entry) => !CLEARED_REF_EXCLUDED_KINDS.has(entry.kind) + ) + if (droppable.length === 0) return [] + const idsByKind: Partial>> = {} + for (const entry of droppable) { + ;(idsByKind[entry.kind] ??= new Set()).add(entry.sourceId) + } + const liveByKind = await filterExistingForkTargets(executor, sourceWorkspaceId, idsByKind) + return droppable.filter((entry) => !(liveByKind[entry.kind]?.has(entry.sourceId) ?? false)) +} + /** Upper bound on the blockers a gate failure reports, so the error body stays sane. */ const FORK_SYNC_BLOCKER_LIMIT = 100 @@ -376,24 +404,60 @@ export async function collectForkSyncBlockers( * rows) when one does. Omit to always collect from scratch. */ planUnmapped?: ReadonlyArray> + /** + * References the user explicitly acknowledged dropping. Applied ONLY where the source + * resource is actually gone, judged by the liveness annotation below - which reads the + * source workspace inside this same transaction - so an acknowledgment for a still-live + * reference is ignored and keeps blocking. + */ + droppedReferences?: ReadonlyArray<{ kind: ForkRemapKind; sourceId: string }> } -): Promise { - const { executor, sourceWorkspaceId, planUnmapped, ...collectParams } = params - if (planUnmapped && !hasForkSyncBlockerCandidates(planUnmapped, collectParams)) return [] +): Promise<{ + blockers: ForkSyncBlocker[] + /** The acknowledgments that were actually honoured, for post-sync reporting. */ + appliedDrops: Array<{ kind: ForkRemapKind; sourceId: string }> +}> { + const { executor, sourceWorkspaceId, planUnmapped, droppedReferences, ...collectParams } = params + const empty = { blockers: [] as ForkSyncBlocker[], appliedDrops: [] } + if (planUnmapped && !hasForkSyncBlockerCandidates(planUnmapped, collectParams)) return empty const candidates = collectForkClearedRefCandidates({ ...collectParams, sourceLabels: new Map(), sourceWorkflowNames: new Map(), }) - if (!candidates.some((ref) => ref.cause === 'reference' || ref.cause === 'workflow')) return [] + if (!candidates.some((ref) => ref.cause === 'reference' || ref.cause === 'workflow')) return empty const annotated = await annotateForkClearedRefSourceLiveness( executor, sourceWorkspaceId, candidates ) - const blocking = selectForkSyncBlockingRefs(annotated).slice(0, FORK_SYNC_BLOCKER_LIMIT) - if (blocking.length === 0) return [] + + const acknowledged = new Set( + (droppedReferences ?? []).map((entry) => `${entry.kind}:${entry.sourceId}`) + ) + const appliedDropKeys = new Set() + const afterDrops = + acknowledged.size === 0 + ? annotated + : annotated.filter((ref) => { + const key = `${ref.kind}:${ref.sourceId}` + // `sourceDeleted` is set only on `reference`-cause entries, so this can never drop a + // dependent- or workflow-cause blocker, nor a reference whose source is still live. + if (ref.cause !== 'reference' || !ref.sourceDeleted || !acknowledged.has(key)) return true + appliedDropKeys.add(key) + return false + }) + const appliedDrops = Array.from(appliedDropKeys).map((key) => { + const separator = key.indexOf(':') + return { + kind: key.slice(0, separator) as ForkRemapKind, + sourceId: key.slice(separator + 1), + } + }) + + const blocking = selectForkSyncBlockingRefs(afterDrops).slice(0, FORK_SYNC_BLOCKER_LIMIT) + if (blocking.length === 0) return { blockers: [], appliedDrops } // Best-effort display labels (failure path only). Copyable kinds go through the shared label // loader (live rows only - a deleted source keeps its id label); MCP servers are read without @@ -434,7 +498,10 @@ export async function collectForkSyncBlockers( return copyableLabels.get(`${ref.kind}:${ref.sourceId}`)?.label ?? ref.sourceLabel } - return toForkSyncBlockers( - blocking.map(({ ref, reason }) => ({ ref: { ...ref, sourceLabel: labelFor(ref) }, reason })) - ) + return { + blockers: toForkSyncBlockers( + blocking.map(({ ref, reason }) => ({ ref: { ...ref, sourceLabel: labelFor(ref) }, reason })) + ), + appliedDrops, + } } diff --git a/apps/sim/ee/workspace-forking/lib/promote/promote-plan.ts b/apps/sim/ee/workspace-forking/lib/promote/promote-plan.ts index 79932823041..c666c345c30 100644 --- a/apps/sim/ee/workspace-forking/lib/promote/promote-plan.ts +++ b/apps/sim/ee/workspace-forking/lib/promote/promote-plan.ts @@ -140,6 +140,27 @@ export function buildPromoteWorkflowIdMap(params: { * reported in `excludedTargets` instead of written - the target side of the * "Exclude from sync" contract. Pure - split from the DB reads so it is unit-testable. */ +/** + * The target this source would write, when that target is live AND marked "Exclude from sync" - + * the target side of the exclusion contract, which makes the sync skip the source entirely. + * Returns null when the source is not excluded. + * + * Shared by the plan builder and by `getForkMappingView`, so the Mappings section can never list + * references carried only by a workflow the sync provably never touches. Such an entry would be + * unresolvable-looking (its "Used in" list is plan-scoped, so it renders empty) yet still block + * Sync, because every mapping entry is `required`. + */ +export function resolveForkExcludedTargetId( + sourceWorkflowId: string, + identityMap: ReadonlyMap, + targetActiveIds: ReadonlySet, + excludedTargetIds: ReadonlySet +): string | null { + const mappedTargetId = identityMap.get(sourceWorkflowId) + if (!mappedTargetId || !targetActiveIds.has(mappedTargetId)) return null + return excludedTargetIds.has(mappedTargetId) ? mappedTargetId : null +} + export function buildForkPromotePlanItems(params: { deployedSourceWorkflows: DeployedWorkflowSummary[] sourceStateIds: ReadonlySet @@ -164,16 +185,22 @@ export function buildForkPromotePlanItems(params: { for (const source of deployedSourceWorkflows) { if (!sourceStateIds.has(source.id)) continue - const mappedTargetId = identityMap.get(source.id) - const activeTargetId = - mappedTargetId && targetActiveIds.has(mappedTargetId) ? mappedTargetId : null - if (activeTargetId && excludedTargetIds.has(activeTargetId)) { + const excludedTargetId = resolveForkExcludedTargetId( + source.id, + identityMap, + targetActiveIds, + excludedTargetIds + ) + if (excludedTargetId !== null) { excludedTargets.push({ - id: activeTargetId, - name: targetNameById.get(activeTargetId) ?? source.name, + id: excludedTargetId, + name: targetNameById.get(excludedTargetId) ?? source.name, }) continue } + const mappedTargetId = identityMap.get(source.id) + const activeTargetId = + mappedTargetId && targetActiveIds.has(mappedTargetId) ? mappedTargetId : null items.push({ sourceWorkflowId: source.id, targetWorkflowId: activeTargetId ?? generateId(), diff --git a/apps/sim/ee/workspace-forking/lib/promote/promote.test.ts b/apps/sim/ee/workspace-forking/lib/promote/promote.test.ts index fd5f5035892..c44dd9c9e2a 100644 --- a/apps/sim/ee/workspace-forking/lib/promote/promote.test.ts +++ b/apps/sim/ee/workspace-forking/lib/promote/promote.test.ts @@ -20,6 +20,8 @@ const { mockCreateTransform, mockSumForkCopyBytes, mockAssertForkStorageHeadroom, + mockLoadTargetWebhookPaths, + mockVerifyDrops, } = vi.hoisted(() => ({ mockComputePlan: vi.fn(), mockBuildCopySelection: vi.fn(), @@ -36,6 +38,8 @@ const { mockCreateTransform: vi.fn(), mockSumForkCopyBytes: vi.fn(), mockAssertForkStorageHeadroom: vi.fn(), + mockLoadTargetWebhookPaths: vi.fn(), + mockVerifyDrops: vi.fn(), })) vi.mock('@/lib/workflows/deployment-outbox', () => ({ @@ -68,6 +72,7 @@ vi.mock('@/ee/workspace-forking/lib/copy/storage-quota', () => ({ vi.mock('@/ee/workspace-forking/lib/copy/deploy-bridge', () => ({ getActiveDeploymentVersionNumbers: vi.fn(async () => new Map()), loadSourceDeployedStates: mockLoadSourceDeployedStates, + loadTargetWebhookPathsByBlock: mockLoadTargetWebhookPaths, })) vi.mock('@/ee/workspace-forking/lib/lineage/lineage', () => ({ acquireForkEdgeLock: vi.fn(), @@ -104,6 +109,7 @@ vi.mock('@/ee/workspace-forking/lib/mapping/mapping-store', () => ({ })) vi.mock('@/ee/workspace-forking/lib/promote/cleared-refs', () => ({ collectForkSyncBlockers: mockCollectBlockers, + verifyForkDropAcknowledgments: mockVerifyDrops, })) vi.mock('@/ee/workspace-forking/lib/promote/copy-unmapped', () => ({ // Faithful mirror of the real overlay so a copy's id maps resolve through the augmented @@ -152,6 +158,7 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({ })) import { db } from '@sim/db' +import { getBlock } from '@/blocks/registry' import { copyWorkflowStateIntoTarget } from '@/ee/workspace-forking/lib/copy/copy-workflows' import { reconcileForkDependentValues } from '@/ee/workspace-forking/lib/mapping/dependent-value-store' import { promoteFork } from '@/ee/workspace-forking/lib/promote/promote' @@ -246,7 +253,7 @@ beforeEach(() => { willResolve: new Set(), }) mockHasCopySelection.mockReturnValue(false) - mockCollectBlockers.mockResolvedValue([]) + mockCollectBlockers.mockResolvedValue({ blockers: [], appliedDrops: [] }) mockLoadBlockMap.mockResolvedValue(new Map()) mockBuildBlockIdResolver.mockReturnValue((_wf: string, blockId: string) => blockId) mockResolveFolderMapping.mockResolvedValue(new Map()) @@ -255,6 +262,9 @@ beforeEach(() => { mockCreateTransform.mockReturnValue((subBlocks: unknown) => subBlocks) mockSumForkCopyBytes.mockResolvedValue(0) mockAssertForkStorageHeadroom.mockResolvedValue(undefined) + mockLoadTargetWebhookPaths.mockResolvedValue(new Map()) + // Default: no acknowledgments, so the unmapped gate behaves exactly as before. + mockVerifyDrops.mockResolvedValue([]) }) describe('promoteFork gates', () => { @@ -321,8 +331,56 @@ describe('promoteFork gates', () => { expect(mockUpsertPromoteRun).not.toHaveBeenCalled() }) + /** + * A source-deleted reference on a REQUIRED field sits in `unmappedRequired`, and that gate runs + * before the cleared-ref gate that honours drops. Without subtracting verified drops here, Drop + * was inert for exactly the references it exists to unblock - the sync still failed with + * "map all required ... first". + */ + it('lets a VERIFIED drop clear the unmapped gate for a required reference', async () => { + mockComputePlan.mockResolvedValue( + makePlan({ + unmappedRequired: [ + { kind: 'table', sourceId: 'tbl-gone', subBlockKey: 'tableSelector', required: true }, + ], + }) + ) + mockVerifyDrops.mockResolvedValue([{ kind: 'table', sourceId: 'tbl-gone' }]) + + const result = await promoteFork({ + ...promoteParams(), + dropReferences: [{ kind: 'table', sourceId: 'tbl-gone' }], + }) + + expect(result.blocked).toBeNull() + // The SAME verified set reaches the cleared-ref gate, so one liveness check governs both. + expect(mockCollectBlockers).toHaveBeenCalledWith( + expect.objectContaining({ droppedReferences: [{ kind: 'table', sourceId: 'tbl-gone' }] }) + ) + }) + + /** An acknowledgment the server refuses (source still live) must not weaken the required gate. */ + it('keeps blocking when the acknowledgment fails verification', async () => { + mockComputePlan.mockResolvedValue( + makePlan({ + unmappedRequired: [ + { kind: 'table', sourceId: 'tbl-live', subBlockKey: 'tableSelector', required: true }, + ], + }) + ) + mockVerifyDrops.mockResolvedValue([]) + + const result = await promoteFork({ + ...promoteParams(), + dropReferences: [{ kind: 'table', sourceId: 'tbl-live' }], + }) + + expect(result.blocked).toBe('unmapped') + expect(mockCollectBlockers).not.toHaveBeenCalled() + }) + it('blocks with the structured blocker list when references would clear, writing NOTHING', async () => { - mockCollectBlockers.mockResolvedValue([BLOCKER]) + mockCollectBlockers.mockResolvedValue({ blockers: [BLOCKER], appliedDrops: [] }) const result = await promoteFork(promoteParams()) @@ -624,3 +682,106 @@ describe('promoteFork dependent values', () => { ) }) }) + +describe('promoteFork trigger URLs', () => { + beforeEach(() => { + // A block only holds a public URL when its config declares a `useWebhookUrl` field, so the + // fixture has to look like a webhook trigger to the shared predicate. + vi.mocked(getBlock).mockReturnValue({ + category: 'triggers', + subBlocks: [{ id: 'triggerWebhookUrl', useWebhookUrl: true }], + } as never) + }) + + const triggerState = { + blocks: { + 'blk-new': { + id: 'blk-new', + // The REAL slack_webhook trigger id, so the provider check resolves against the actual + // registry - adoption only pairs a URL with a trigger of the SAME provider. + type: 'slack_webhook', + name: 'Slack messages', + triggerMode: true, + subBlocks: {}, + outputs: {}, + enabled: true, + }, + }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + } + + function arrangeReCreatedTrigger() { + const item = { + sourceWorkflowId: 'wf-src', + targetWorkflowId: 'wf-tgt', + targetName: 'Flow', + mode: 'replace' as const, + sourceMeta: { name: 'Flow', description: null, folderId: null, sortOrder: 0 }, + } + mockComputePlan.mockResolvedValue(makePlan({ items: [item] })) + mockLoadSourceDeployedStates.mockResolvedValue({ + deployedWorkflows: [], + sourceStates: new Map([['wf-src', triggerState]]), + }) + // The old trigger block ('blk-old') serves the live URL and is NOT in the source any more: + // the user deleted and re-added the trigger, so the sync writes 'blk-new' instead. + mockLoadTargetWebhookPaths.mockResolvedValue( + new Map([['blk-old', { path: 'live-slack-path', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + vi.mocked(copyWorkflowStateIntoTarget).mockResolvedValue({ + targetWorkflowId: 'wf-tgt', + mode: 'replace', + name: 'Flow', + blocksCount: 1, + edgesCount: 0, + subflowsCount: 0, + clearedDependents: [], + blockIdMapping: new Map(), + }) + } + + /** + * The reported bug, at the promote level: pushing a workflow whose Slack trigger was re-created + * used to hand the parent a brand-new webhook URL, forcing a re-paste into Slack every sync. + */ + it('hands the retiring URL to the arriving trigger instead of minting a new one', async () => { + arrangeReCreatedTrigger() + + const result = await promoteFork(promoteParams()) + + expect(result.blocked).toBeNull() + const writeParams = vi.mocked(copyWorkflowStateIntoTarget).mock.calls[0][0] + expect(writeParams.triggerPathByBlockId?.get('blk-new')).toBe('live-slack-path') + // Adopted, so nothing needs re-registering externally. + expect(result.triggerUrlChanges).toEqual([]) + }) + + it('reports the URL as lost when the caller explicitly opts into a new one', async () => { + arrangeReCreatedTrigger() + + const result = await promoteFork({ + ...promoteParams(), + triggerMappings: [{ sourceBlockId: 'blk-new', adoptPath: null }], + }) + + const writeParams = vi.mocked(copyWorkflowStateIntoTarget).mock.calls[0][0] + expect(writeParams.triggerPathByBlockId?.size).toBe(0) + expect(result.triggerUrlChanges).toEqual([{ workflowName: 'Flow', path: 'live-slack-path' }]) + }) + + /** The server re-derives the adoptable set, so a stale or crafted path is never honoured. */ + it('ignores a mapping naming a path the plan does not offer', async () => { + arrangeReCreatedTrigger() + + await promoteFork({ + ...promoteParams(), + triggerMappings: [{ sourceBlockId: 'blk-new', adoptPath: 'someone-elses-path' }], + }) + + const writeParams = vi.mocked(copyWorkflowStateIntoTarget).mock.calls[0][0] + expect(writeParams.triggerPathByBlockId?.size).toBe(0) + }) +}) diff --git a/apps/sim/ee/workspace-forking/lib/promote/promote.ts b/apps/sim/ee/workspace-forking/lib/promote/promote.ts index 5de3872f0f4..22a25a9e42c 100644 --- a/apps/sim/ee/workspace-forking/lib/promote/promote.ts +++ b/apps/sim/ee/workspace-forking/lib/promote/promote.ts @@ -33,6 +33,7 @@ import { import { getActiveDeploymentVersionNumbers, loadSourceDeployedStates, + loadTargetWebhookPathsByBlock, } from '@/ee/workspace-forking/lib/copy/deploy-bridge' import { assertForkStorageHeadroom, @@ -63,7 +64,10 @@ import { upsertEdgeMappings, } from '@/ee/workspace-forking/lib/mapping/mapping-store' import { getMcpServerMetaByIds } from '@/ee/workspace-forking/lib/mapping/resources' -import { collectForkSyncBlockers } from '@/ee/workspace-forking/lib/promote/cleared-refs' +import { + collectForkSyncBlockers, + verifyForkDropAcknowledgments, +} from '@/ee/workspace-forking/lib/promote/cleared-refs' import { augmentForkResolver, buildPromoteCopySelection, @@ -78,6 +82,12 @@ import { type PromoteRunWorkflowSnapshot, upsertPromoteRun, } from '@/ee/workspace-forking/lib/promote/promote-run-store' +import { + buildForkTriggerPlan, + type ForkTriggerMappingInput, + type ForkTriggerUrlChange, + resolveForkTriggerPaths, +} from '@/ee/workspace-forking/lib/promote/trigger-urls' import { buildForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' import { createForkSubBlockTransform, @@ -119,6 +129,17 @@ export interface PromoteForkParams { * plan's copyable candidates, so an arbitrary id is ignored. */ copyResources?: PromoteCopyResources + /** + * References the caller explicitly acknowledged dropping, so the sync clears them in the target + * instead of blocking. Honoured only where the source resource is genuinely gone (re-derived + * in-transaction), so a live reference can never be dropped by a crafted payload. + */ + dropReferences?: Array<{ kind: ForkRemapKind; sourceId: string }> + /** + * Which retiring public URL each arriving trigger takes over. Re-validated in-transaction + * against the adoptable set the plan derives, so an entry the plan does not offer is ignored. + */ + triggerMappings?: ForkTriggerMappingInput[] requestId?: string } @@ -159,6 +180,16 @@ export interface PromoteForkResult { * behavior is never silent. */ clearedOptional: Array<{ workflowName: string; blocks: string[] }> + /** + * Source-deleted references the user acknowledged dropping that this sync actually cleared in + * the target. Only entries whose source was verified gone in-transaction appear here. + */ + droppedReferences: Array<{ kind: ForkRemapKind; sourceId: string }> + /** + * Public trigger URLs this sync stopped serving in the target - a URL that retired with no + * arriving trigger adopting it. Whatever calls it externally has to be repointed. + */ + triggerUrlChanges: ForkTriggerUrlChange[] } function collectCredentialPairs(plan: ForkPromotePlan): Array<[string, string]> { @@ -307,6 +338,10 @@ interface PromoteTxApplied { needsConfiguration: Array<{ workflowId: string; workflowName: string; blocks: string[] }> /** Per-workflow optional dependents a parent change cleared (surfaced, not gated). */ clearedOptional: Array<{ workflowName: string; blocks: string[] }> + /** Acknowledged source-deleted references this sync cleared instead of blocking on. */ + droppedReferences: Array<{ kind: ForkRemapKind; sourceId: string }> + /** Public trigger URLs this sync stopped serving (nothing adopted them). */ + triggerUrlChanges: ForkTriggerUrlChange[] /** Heavy content for resources copied into the target this sync, filled best-effort post-commit. */ copyContentPlan: ForkContentPlan | null /** Serialized in-content maps for the post-commit skill-body rewrite (paired with the plan). */ @@ -442,10 +477,23 @@ export async function promoteFork(params: PromoteForkParams): Promise `${entry.kind}:${entry.sourceId}`)) // plan.unmappedRequired is already references.filter(resolver == null).filter(required), so // subtracting the refs the copy will resolve is equivalent to re-scanning the predicate. const postCopyUnmappedRequired = plan.unmappedRequired.filter( - (reference) => !willResolve.has(`${reference.kind}:${reference.sourceId}`) + (reference) => + !willResolve.has(`${reference.kind}:${reference.sourceId}`) && + !droppedKeys.has(`${reference.kind}:${reference.sourceId}`) ) if (postCopyUnmappedRequired.length > 0) { return { @@ -477,9 +525,10 @@ export async function promoteFork(params: PromoteForkParams): Promise = [] const gateResolver: ForkReferenceResolver = (kind, sourceId) => willResolve.has(`${kind}:${sourceId}`) ? sourceId : plan.resolver(kind, sourceId) - const blockers = await collectForkSyncBlockers({ + const { blockers, appliedDrops } = await collectForkSyncBlockers({ executor: tx, sourceWorkspaceId, items: plan.items, @@ -488,10 +537,12 @@ export async function promoteFork(params: PromoteForkParams): Promise 0) { return { blocked: 'cleared-refs', blockers } } + droppedReferences = appliedDrops // Resolve the source->target folder map BEFORE the copy so the folders already exist in the // target and the copy can rewrite `sim:folder/` references inside copied skill / markdown @@ -638,6 +689,22 @@ export async function promoteFork(params: PromoteForkParams): Promise item.targetWorkflowId) + ), + }) + const { pathByTargetBlockId: triggerPathByBlockId, changes: triggerUrlChanges } = + resolveForkTriggerPaths(triggerPlan, params.triggerMappings) + const updatedSnapshots: PromoteRunWorkflowSnapshot[] = [] const createdTargetIds: string[] = [] const writtenItems: typeof plan.items = [] @@ -672,6 +739,7 @@ export async function promoteFork(params: PromoteForkParams): Promise): WorkflowState { + return { + blocks: Object.fromEntries( + Object.entries(blocks).map(([id, block]) => [ + id, + { id, type: block.type, name: block.name, subBlocks: {}, outputs: {}, enabled: true }, + ]) + ), + edges: [], + loops: {}, + parallels: {}, + } as unknown as WorkflowState +} + +const item = { + sourceWorkflowId: 'wf-src', + targetWorkflowId: 'wf-tgt', + targetName: 'Prod', + mode: 'replace' as const, + sourceMeta: { + name: 'Prod', + description: null, + folderId: null, + sortOrder: 0, + isPublicApi: false, + }, +} + +/** Identity resolver: the source block keeps its id in the target (the stable-pairing case). */ +const identityResolver = (_targetWorkflowId: string, sourceBlockId: string) => sourceBlockId + +function webhooks(entries: Array<[string, ForkTargetWebhook]>) { + return new Map(entries) +} + +function run( + blocks: Record, + targetWebhooks: Map, + overrides?: ForkTriggerMappingInput[] +) { + const plan = buildForkTriggerPlan({ + items: [item], + sourceStates: new Map([['wf-src', stateWith(blocks)]]), + resolveBlockId: identityResolver, + targetWebhooks, + }) + return { plan, ...resolveForkTriggerPaths(plan, overrides) } +} + +/** + * Blocks use the REAL `slack_webhook` trigger id, so provider resolution runs against the actual + * trigger registry (provider `slack`) rather than a mock that could drift from it. + */ +describe('fork trigger URLs', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(getBlock).mockReturnValue(TRIGGER_BLOCK as never) + }) + + it('pins a trigger that keeps its target identity to its own path, reporting no change', () => { + const { pathByTargetBlockId, changes, plan } = run( + { blk: { type: 'slack_webhook', name: 'Slack' } }, + webhooks([['blk', { path: 'custom-path', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + expect(changes).toEqual([]) + expect(pathByTargetBlockId.get('blk')).toBe('custom-path') + // Nothing to decide: the block already serves a URL, so it offers no alternatives. + expect(plan.slots[0].adoptablePaths).toEqual([]) + }) + + /** + * The reported bug: a trigger deleted and re-added in the source re-keys the target block, which + * used to mint a new URL. The single arriving trigger now adopts the retiring URL instead. + */ + it('adopts a retiring URL onto the single arriving trigger that replaces it', () => { + const { pathByTargetBlockId, changes, plan } = run( + { blk2: { type: 'slack_webhook', name: 'Slack v2' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + expect(plan.slots[0].defaultAdoptPath).toBe('blk1') + expect(pathByTargetBlockId.get('blk2')).toBe('blk1') + // Adopted means still served — there is nothing for the user to re-register. + expect(changes).toEqual([]) + }) + + it('reports a removal when the trigger is gone from the source entirely', () => { + vi.mocked(getBlock).mockReturnValue({ ...TRIGGER_BLOCK, category: 'blocks' } as never) + const { pathByTargetBlockId, changes } = run( + { fn: { type: 'function', name: 'Fn' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + expect(changes).toEqual([{ workflowName: 'Prod', path: 'blk1' }]) + expect(pathByTargetBlockId.size).toBe(0) + }) + + it('does not guess a pairing when several URLs retire at once', () => { + const { pathByTargetBlockId, changes, plan } = run( + { blk3: { type: 'slack_webhook', name: 'Slack' } }, + webhooks([ + ['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }], + ['blk2', { path: 'blk2', workflowId: 'wf-tgt', provider: 'slack' }], + ]) + ) + expect(plan.slots[0].defaultAdoptPath).toBeNull() + // Both are offered, so the user can resolve the ambiguity; neither is taken by default. + expect(plan.slots[0].adoptablePaths).toEqual(['blk1', 'blk2']) + expect(pathByTargetBlockId.size).toBe(0) + expect(changes.map((change) => change.path)).toEqual(['blk1', 'blk2']) + }) + + it('honours an explicit pick when the pairing is ambiguous', () => { + const { pathByTargetBlockId, changes } = run( + { blk3: { type: 'slack_webhook', name: 'Slack' } }, + webhooks([ + ['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }], + ['blk2', { path: 'blk2', workflowId: 'wf-tgt', provider: 'slack' }], + ]), + [{ sourceBlockId: 'blk3', adoptPath: 'blk2' }] + ) + expect(pathByTargetBlockId.get('blk3')).toBe('blk2') + expect(changes.map((change) => change.path)).toEqual(['blk1']) + }) + + it('lets an explicit null override the default and mint a new URL', () => { + const { pathByTargetBlockId, changes } = run( + { blk2: { type: 'slack_webhook', name: 'Slack v2' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]), + [{ sourceBlockId: 'blk2', adoptPath: null }] + ) + expect(pathByTargetBlockId.size).toBe(0) + expect(changes).toEqual([{ workflowName: 'Prod', path: 'blk1' }]) + }) + + /** A crafted payload must not be able to move a URL the plan never offered. */ + it('ignores an override naming a path this slot does not offer', () => { + const { pathByTargetBlockId } = run( + { blk2: { type: 'slack_webhook', name: 'Slack v2' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]), + [{ sourceBlockId: 'blk2', adoptPath: 'a-path-from-another-workspace' }] + ) + expect(pathByTargetBlockId.size).toBe(0) + }) + + it('never lets two triggers adopt the same path', () => { + const { pathByTargetBlockId } = run( + { + blk2: { type: 'slack_webhook', name: 'Slack A' }, + blk3: { type: 'slack_webhook', name: 'Slack B' }, + }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]), + [ + { sourceBlockId: 'blk2', adoptPath: 'blk1' }, + { sourceBlockId: 'blk3', adoptPath: 'blk1' }, + ] + ) + expect(Array.from(pathByTargetBlockId.values())).toEqual(['blk1']) + expect(pathByTargetBlockId.get('blk2')).toBe('blk1') + }) + + /** + * A path is authenticated and parsed as its provider. Handing a GitHub URL to an arriving Slack + * trigger would keep the endpoint alive while every request failed signature verification — and + * the sync would have reported the URL as preserved, so nobody would go looking. + */ + it('never offers a retiring URL from a DIFFERENT provider', () => { + const { plan, pathByTargetBlockId, changes } = run( + { blk2: { type: 'slack_webhook', name: 'Slack v2' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'github' }]]) + ) + expect(plan.slots[0].adoptablePaths).toEqual([]) + expect(plan.slots[0].defaultAdoptPath).toBeNull() + expect(pathByTargetBlockId.size).toBe(0) + // Still reported as lost, so the GitHub subscription's owner is told it stopped serving. + expect(changes).toEqual([{ workflowName: 'Prod', path: 'blk1' }]) + }) + + it('pairs only within the matching provider when several URLs retire', () => { + const { plan, pathByTargetBlockId } = run( + { blk3: { type: 'slack_webhook', name: 'Slack' } }, + webhooks([ + ['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'github' }], + ['blk2', { path: 'blk2', workflowId: 'wf-tgt', provider: 'slack' }], + ]) + ) + // Only the same-provider URL is a candidate, which makes the pairing unambiguous again. + expect(plan.slots[0].adoptablePaths).toEqual(['blk2']) + expect(pathByTargetBlockId.get('blk3')).toBe('blk2') + }) + + /** + * Adoption is scoped to one target workflow because `webhook_path_claim` ownership is + * per-workflow: taking a path from another workflow would be an ownership transfer the claim + * layer refuses, so it must never be offered. + */ + it('never offers a path owned by a different workflow', () => { + const { plan, pathByTargetBlockId, changes } = run( + { blk: { type: 'slack_webhook', name: 'Slack' } }, + webhooks([['other', { path: 'other', workflowId: 'wf-elsewhere', provider: 'slack' }]]) + ) + expect(plan.slots[0].adoptablePaths).toEqual([]) + expect(pathByTargetBlockId.size).toBe(0) + expect(changes).toEqual([]) + }) + + it('skips a non-trigger block arriving on a target block with no webhook', () => { + vi.mocked(getBlock).mockReturnValue({ ...TRIGGER_BLOCK, category: 'blocks' } as never) + const { plan } = run( + { fn: { type: 'function', name: 'Fn' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + expect(plan.slots).toEqual([]) + }) + + /** + * A poller or schedule trigger has no public URL, so handing it a retiring one would point an + * external caller at a path its provider never serves. + */ + it('never offers a retiring URL to a trigger that serves no public URL', () => { + vi.mocked(getBlock).mockReturnValue(URL_LESS_TRIGGER_BLOCK as never) + const { plan, pathByTargetBlockId, changes } = run( + { poller: { type: 'gmail', name: 'Gmail poller' } }, + webhooks([['blk1', { path: 'blk1', workflowId: 'wf-tgt', provider: 'slack' }]]) + ) + expect(plan.slots).toEqual([]) + expect(pathByTargetBlockId.size).toBe(0) + // The URL still retires and is still reported - it just has no eligible adopter. + expect(changes).toEqual([{ workflowName: 'Prod', path: 'blk1' }]) + }) +}) diff --git a/apps/sim/ee/workspace-forking/lib/promote/trigger-urls.ts b/apps/sim/ee/workspace-forking/lib/promote/trigger-urls.ts new file mode 100644 index 00000000000..e02b46198ce --- /dev/null +++ b/apps/sim/ee/workspace-forking/lib/promote/trigger-urls.ts @@ -0,0 +1,195 @@ +import type { ForkTargetWebhook } from '@/ee/workspace-forking/lib/copy/deploy-bridge' +import type { ForkPromotePlanItem } from '@/ee/workspace-forking/lib/promote/promote-plan' +import type { ForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' +import type { BlockState, WorkflowState } from '@/stores/workflows/workflow/types' +import { blockAdvertisesWebhookUrl, resolveBlockTriggerProvider } from '@/triggers/webhook-url' + +/** + * A public trigger URL a sync stops serving in the target. + * + * Only URLs that genuinely go away are reported: one an arriving trigger adopts keeps serving the + * same path, so it is not a change. Whatever calls this path externally - a Slack Request URL, a + * provider subscription - stops being called and has to be repointed by hand. + */ +export interface ForkTriggerUrlChange { + workflowName: string + path: string +} + +/** + * One arriving trigger block whose public URL this sync decides. + * + * A target webhook's path is `triggerPath || block.id`, and a sync assigns target block ids from + * the SOURCE's block identity - so a trigger re-created in the source re-keys its target block and + * moves the URL. `ownPath` is the stable case (the block already serves a URL, which is pinned + * back verbatim); `adoptablePaths` is the decision, listing URLs retiring in the SAME target + * workflow that this block can take over instead of minting a new one. + */ +export interface ForkTriggerSlot { + sourceBlockId: string + targetBlockId: string + blockName: string + workflowName: string + /** The path this block already serves. Pinned as-is; there is no decision to make. */ + ownPath: string | null + /** Retiring paths in the same target workflow this block could take over instead. */ + adoptablePaths: string[] + /** The unambiguous pairing (exactly one retiring URL, exactly one arriving trigger). */ + defaultAdoptPath: string | null +} + +/** Every trigger decision a sync makes, plus the URLs it would retire. */ +export interface ForkTriggerPlan { + slots: ForkTriggerSlot[] + /** Live target webhooks on blocks this sync will not write - their URLs stop being served. */ + retiring: Array<{ path: string; workflowName: string }> +} + +/** A caller's explicit choice of which retiring URL an arriving trigger takes over. */ +export interface ForkTriggerMappingInput { + sourceBlockId: string + /** A path from that slot's `adoptablePaths`, or null to mint a new URL. */ + adoptPath: string | null +} + +/** + * Work out, per target workflow, which trigger URLs retire and which arriving triggers could + * take them over. + * + * Adoption is deliberately scoped to a SINGLE target workflow. `webhook_path_claim` ownership is + * per-workflow (`claimWebhookPath` conflicts only against a *different* workflow), so moving a + * path between blocks of the same workflow re-uses a claim that workflow already holds and can + * never conflict. Offering a path from another workflow would be a genuine ownership transfer, + * which the claim layer refuses by design - so it is never a candidate. + * + * Pure over the pre-read source states, so the preview and the write agree by construction. + */ +export function buildForkTriggerPlan(params: { + items: ForkPromotePlanItem[] + sourceStates: Map + resolveBlockId: ForkBlockIdResolver + targetWebhooks: ReadonlyMap +}): ForkTriggerPlan { + const { items, sourceStates, resolveBlockId, targetWebhooks } = params + + const liveByWorkflow = new Map< + string, + Array<{ blockId: string; path: string; provider: string | null }> + >() + for (const [blockId, row] of targetWebhooks) { + const entry = { blockId, path: row.path, provider: row.provider } + const list = liveByWorkflow.get(row.workflowId) + if (list) list.push(entry) + else liveByWorkflow.set(row.workflowId, [entry]) + } + + const slots: ForkTriggerSlot[] = [] + const retiring: ForkTriggerPlan['retiring'] = [] + + for (const item of items) { + const sourceState = sourceStates.get(item.sourceWorkflowId) + if (!sourceState) continue + + const sourceByTargetBlockId = new Map() + for (const [sourceBlockId, block] of Object.entries(sourceState.blocks)) { + sourceByTargetBlockId.set(resolveBlockId(item.targetWorkflowId, sourceBlockId), { + sourceBlockId, + block, + }) + } + + // A live webhook on a block this sync will not write: its URL stops being served. + const live = liveByWorkflow.get(item.targetWorkflowId) ?? [] + const retired = live.filter((row) => !sourceByTargetBlockId.has(row.blockId)) + for (const row of retired) { + retiring.push({ path: row.path, workflowName: item.sourceMeta.name }) + } + + const arriving: ForkTriggerSlot[] = [] + for (const [targetBlockId, { sourceBlockId, block }] of sourceByTargetBlockId) { + // Only a block that advertises a public URL can hold one. Handing a retiring URL to a + // poller or a shared-app trigger would point an external caller at a path its provider + // never serves - so those are not candidates, and never appear as rows. + if (!blockAdvertisesWebhookUrl(block)) continue + const ownPath = targetWebhooks.get(targetBlockId)?.path ?? null + // Only a retiring URL of the SAME provider is adoptable. A path is authenticated and parsed + // as its provider, so handing a GitHub URL to a Slack trigger would keep the endpoint alive + // while every request failed signature verification - and the sync would have reported the + // URL as preserved, so nobody would go looking. + const provider = resolveBlockTriggerProvider(block) + arriving.push({ + sourceBlockId, + targetBlockId, + blockName: block.name, + workflowName: item.sourceMeta.name, + ownPath, + // A block already serving a URL keeps it; only a block without one is a candidate to + // adopt, so offering it a second URL would just be a way to break the first. + adoptablePaths: + ownPath === null && provider !== null + ? retired.filter((row) => row.provider === provider).map((row) => row.path) + : [], + defaultAdoptPath: null, + }) + } + + // Default only the unambiguous pairing, and only within one provider: with several retiring or + // several arriving, guessing which new trigger replaces which old URL would silently point an + // external caller at the wrong workflow branch - the user picks instead. + const adopters = arriving.filter((slot) => slot.adoptablePaths.length > 0) + if (adopters.length === 1 && adopters[0].adoptablePaths.length === 1) { + adopters[0].defaultAdoptPath = adopters[0].adoptablePaths[0] + } + slots.push(...arriving) + } + + return { slots, retiring } +} + +/** + * Resolve every trigger block's final path, applying the caller's explicit choices over the + * plan's defaults, and report the URLs that still retire. + * + * An override is honoured only for a path the slot actually offered (same target workflow, still + * retiring), and each path can be adopted once - so a crafted payload can neither move a URL + * across workflows nor point two triggers at one path (which the unique webhook path index would + * reject at deploy time anyway, failing the whole sync). + */ +export function resolveForkTriggerPaths( + plan: ForkTriggerPlan, + overrides: readonly ForkTriggerMappingInput[] = [] +): { + /** Target block id -> the path to pin into its `triggerPath`. */ + pathByTargetBlockId: Map + changes: ForkTriggerUrlChange[] +} { + const overrideBySourceBlockId = new Map( + overrides.map((entry) => [entry.sourceBlockId, entry.adoptPath]) + ) + + const pathByTargetBlockId = new Map() + const adopted = new Set() + + for (const slot of plan.slots) { + if (slot.ownPath !== null) { + pathByTargetBlockId.set(slot.targetBlockId, slot.ownPath) + continue + } + const requested = overrideBySourceBlockId.has(slot.sourceBlockId) + ? overrideBySourceBlockId.get(slot.sourceBlockId)! + : slot.defaultAdoptPath + if (requested === null || requested === undefined) continue + if (!slot.adoptablePaths.includes(requested)) continue + if (adopted.has(requested)) continue + adopted.add(requested) + pathByTargetBlockId.set(slot.targetBlockId, requested) + } + + const changes: ForkTriggerUrlChange[] = [] + for (const row of plan.retiring) { + // An adopted path keeps serving the same URL, so it is not a change to warn about. + if (adopted.has(row.path)) continue + changes.push({ workflowName: row.workflowName, path: row.path }) + } + return { pathByTargetBlockId, changes } +} diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts index dbc1a95ac2a..10d16ab6841 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts @@ -1300,6 +1300,64 @@ describe('canonical mode policy (fork/promote)', () => { expect(scan.references).toEqual([]) }) + /** + * Every shipped canonical pair's advanced member is a plain `short-input`, which carries no + * resource definition — so the "advanced is user-owned, verbatim" policy has never been + * exercised against an advanced member that IS a resource selector. Pin it here so the + * policy holds by enforcement rather than by the accident of the current block configs. + */ + const selectorPairBlock = () => + blockWith([ + { + id: 'tableSelector', + title: 'Table', + type: 'table-selector', + canonicalParamId: 'tableId', + mode: 'basic', + }, + { + id: 'advancedTableSelector', + title: 'Table (advanced)', + type: 'table-selector', + canonicalParamId: 'tableId', + mode: 'advanced', + }, + ]) + + it('advanced mode: a selector-typed manual member is neither remapped nor detected', () => { + vi.mocked(getBlock).mockReturnValue(selectorPairBlock()) + const resolveTable = (kind: string, id: string) => + kind === 'table' && id === 'tbl-manual' ? 'tbl-copy' : null + const transform = createForkBootstrapTransform(resolveTable as never) + const result = transform( + { + tableSelector: entry('tableSelector', 'table-selector', 'tbl-basic'), + advancedTableSelector: entry('advancedTableSelector', 'table-selector', 'tbl-manual'), + }, + 'table', + { tableId: 'advanced' } + ) + expect(result.advancedTableSelector.value).toBe('tbl-manual') + expect(result.tableSelector.value).toBe('') + + const scan = scanWorkflowReferences( + [ + { + id: 'b1', + name: 'Table', + type: 'table', + subBlocks: { + tableSelector: entry('tableSelector', 'table-selector', 'tbl-basic'), + advancedTableSelector: entry('advancedTableSelector', 'table-selector', 'tbl-manual'), + }, + canonicalModes: { tableId: 'advanced' }, + }, + ], + () => null + ) + expect(scan.references).toEqual([]) + }) + it('does not detect a condition-hidden subblock (its value never executes)', () => { vi.mocked(getBlock).mockReturnValue( blockWith([ diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts index 60b2663f57a..d419669f296 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -859,14 +859,20 @@ export function remapForkSubBlocks( // under a MANUAL (advanced-active) parent passes through verbatim; a condition-hidden // subblock is rewritten but never detected. const dormant = gates.isDormantMember(subBlockKey) - const verbatimManualDependent = !dormant && gates.isManualParentDependent(subBlockKey) - const detectionSkipped = - dormant || verbatimManualDependent || gates.isConditionHidden(subBlockKey) + // Verbatim (user-owned: never remapped, never a mapping requirement) covers the ACTIVE + // advanced member itself as well as every dependent scoped to it. `clearDependentsOnRemap` + // already spares an active manual member from a parent remap; naming it here applies the + // same policy on the detect/rewrite side, which until now held only because every shipped + // pair's advanced member is a plain `short-input` carrying no resource definition. + const verbatimManual = + !dormant && + (gates.isActiveManualMember(subBlockKey) || gates.isManualParentDependent(subBlockKey)) + const detectionSkipped = dormant || verbatimManual || gates.isConditionHidden(subBlockKey) if (dormant && isNonEmptyValue(value)) { value = '' } - if (definition && forkKind && subBlockType && !verbatimManualDependent) { + if (definition && forkKind && subBlockType && !verbatimManual) { const parsed = parseWorkflowSearchSubBlockResources(value, { type: subBlockType as SubBlockType, }) diff --git a/apps/sim/lib/api/contracts/workspace-fork.test.ts b/apps/sim/lib/api/contracts/workspace-fork.test.ts index 9a72c30fc0f..03ebc5d2326 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.test.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.test.ts @@ -8,6 +8,7 @@ import { forkMappableResourceTypeSchema, getForkDiffContract, getWorkspaceBackgroundWorkQuerySchema, + promoteForkBodySchema, updateForkExcludedWorkflowsBodySchema, updateForkMappingBodySchema, } from '@/lib/api/contracts/workspace-fork' @@ -187,6 +188,54 @@ describe('getForkDiffContract response excluded-workflow lists', () => { const parsed = getForkDiffContract.response.schema.parse(baseDiffResponse) expect(parsed.excludedSourceWorkflows).toEqual([]) expect(parsed.excludedTargetWorkflows).toEqual([]) + expect(parsed.triggerUrlChanges).toEqual([]) + expect(parsed.triggerMappings).toEqual([]) + }) + + it('carries every trigger, whether or not its URL is up for decision', () => { + const parsed = getForkDiffContract.response.schema.parse({ + ...baseDiffResponse, + triggerMappings: [ + // Already serving a URL: informational, no choice offered. + { + sourceBlockId: 'blk-stable', + blockName: 'Prod intake', + workflowName: 'ITSM intake', + ownPath: 'prod-live-path', + adoptablePaths: [], + defaultAdoptPath: null, + }, + // Arriving without one, with a retiring URL it can take over. + { + sourceBlockId: 'blk-new', + blockName: 'Slack messages', + workflowName: 'ITSM intake', + ownPath: null, + adoptablePaths: ['live-slack-path'], + defaultAdoptPath: 'live-slack-path', + }, + ], + triggerUrlChanges: [{ workflowName: 'ITSM intake', path: 'dead-path' }], + }) + expect(parsed.triggerMappings[0].ownPath).toBe('prod-live-path') + expect(parsed.triggerMappings[0].adoptablePaths).toEqual([]) + expect(parsed.triggerMappings[1].defaultAdoptPath).toBe('live-slack-path') + expect(parsed.triggerUrlChanges[0].path).toBe('dead-path') + }) + + it('accepts a trigger mapping choice on the promote body, including "new URL"', () => { + const parsed = promoteForkBodySchema.parse({ + otherWorkspaceId: 'ws-other', + direction: 'push', + triggerMappings: [ + { sourceBlockId: 'blk-a', adoptPath: 'keep-this-path' }, + { sourceBlockId: 'blk-b', adoptPath: null }, + ], + }) + expect(parsed.triggerMappings).toEqual([ + { sourceBlockId: 'blk-a', adoptPath: 'keep-this-path' }, + { sourceBlockId: 'blk-b', adoptPath: null }, + ]) }) it('carries the lists when present', () => { diff --git a/apps/sim/lib/api/contracts/workspace-fork.ts b/apps/sim/lib/api/contracts/workspace-fork.ts index 6a62bb4c15d..5546100fdcf 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.ts @@ -212,6 +212,14 @@ export const forkMappingEntrySchema = z.object({ /** True when `targetId` is an unconfirmed auto-suggestion (no persisted mapping yet). */ suggested: z.boolean(), required: z.boolean(), + /** + * True when the referenced resource no longer exists in the SOURCE workspace, so `sourceLabel` + * falls back to the raw id. Checked by exact id (never the capped candidate list), so this is + * unambiguous: a live resource always resolves its name, however many the workspace has. Such a + * reference cannot be offered for copy - there is nothing to copy - so the resolutions are + * mapping it to a live target, fixing the block in the source, or dropping it. + */ + sourceDeleted: z.boolean(), candidates: z.array(forkMappingCandidateSchema), /** * True when the target workspace has more candidates of this kind than the picker @@ -486,6 +494,53 @@ export const getForkDiffQuerySchema = z.object({ otherWorkspaceId: workspaceIdSchema, direction: forkDirectionSchema, }) +/** + * A public trigger URL a sync would stop serving in the target. Surfaced before the overwrite is + * confirmed, because the external system calling it - a Slack Request URL, a provider webhook + * subscription - has to be repointed by hand afterwards. + */ +export const forkTriggerUrlChangeSchema = z.object({ + workflowName: z.string(), + /** The path that stops being served. A URL an arriving trigger adopts is not reported here. */ + path: z.string(), +}) +export type ForkTriggerUrlChange = z.output + +/** + * One trigger block in this sync that has a public webhook URL, or whose URL is up for decision. + * + * Both cases get an entry, not just the decisions, so the Trigger URLs section reads as a + * standing statement of each URL rather than an alert that appears only when something is wrong. + * A trigger already serving one reports it as `ownPath` and keeps it - `adoptablePaths` is empty + * and the row is informational. A trigger arriving without one lists the URLs retiring in the + * SAME target workflow; picking one hands that live URL to the new block, so the external caller + * - a Slack Request URL, a provider webhook subscription - keeps working untouched. + * + * Triggers with neither are absent, because whether a block serves a URL at all is only knowable + * from its webhook row: a schedule, chat, manual or poller trigger never gets one, and no + * declarative flag on the trigger definition separates them cleanly. + */ +export const forkTriggerMappingSchema = z.object({ + /** The SOURCE block id - stable across the sync, and what a chosen mapping is keyed by. */ + sourceBlockId: z.string(), + blockName: z.string(), + workflowName: z.string(), + /** + * The URL path this trigger already serves in the target, which the sync preserves verbatim. + * Null when the target block has no webhook yet, i.e. the sync decides its URL. + */ + ownPath: z.string().nullable(), + /** + * Retiring URLs in the same target workflow this block may take over instead of minting a new + * one. Always empty when `ownPath` is set: a trigger that already serves a URL keeps it, and + * offering it a second one would only be a way to abandon the first. + */ + adoptablePaths: z.array(z.string()), + /** The pre-selected pairing: unambiguous only when one URL retires and one trigger arrives. */ + defaultAdoptPath: z.string().nullable(), +}) +export type ForkTriggerMapping = z.output + export const getForkDiffContract = defineRouteContract({ method: 'GET', path: '/api/workspaces/[id]/fork/diff', @@ -548,6 +603,13 @@ export const getForkDiffContract = defineRouteContract({ * always clear (informational). */ clearedRefs: z.array(forkClearedRefSchema), + /** + * Public trigger URLs this sync would stop serving in the target. Defaulted so a new client + * tolerates an old server's response during rollout. + */ + triggerUrlChanges: z.array(forkTriggerUrlChangeSchema).default([]), + /** Arriving trigger blocks whose URL this sync decides, with their adoptable alternatives. */ + triggerMappings: z.array(forkTriggerMappingSchema).default([]), }), }, }) @@ -598,6 +660,27 @@ export const promoteForkBodySchema = z.object({ dependentValues: z.array(forkDependentValueEntrySchema).max(2000).optional(), /** Referenced-but-unmapped resources to copy into the target before the sync gate (U17). */ copyResources: promoteCopyResourcesSchema.optional(), + /** + * References the user explicitly acknowledged dropping, so the sync may clear them in the + * target instead of blocking. Honoured ONLY for a reference whose resource no longer exists in + * the source workspace - the server re-derives that liveness inside the promote transaction and + * ignores an acknowledgment for anything still live, so a working reference can never be + * dropped and the zero-cleared-refs invariant relaxes only where the source is already broken. + */ + dropReferences: z + .array(z.object({ kind: forkRemapKindSchema, sourceId: z.string().min(1) })) + .max(2000) + .optional(), + /** + * Which retiring public URL each arriving trigger takes over, overriding the unambiguous + * default. `adoptPath: null` means "mint a new URL for this trigger". The server re-derives the + * adoptable set inside the promote transaction and ignores a path that slot did not offer, so a + * URL can never be moved between workflows (which the per-workflow path claim forbids anyway). + */ + triggerMappings: z + .array(z.object({ sourceBlockId: z.string().min(1), adoptPath: z.string().min(1).nullable() })) + .max(500) + .optional(), }) export const promoteForkContract = defineRouteContract({ method: 'POST', @@ -624,6 +707,20 @@ export const promoteForkContract = defineRouteContract({ needsConfiguration: z.array(forkNeedsConfigurationSchema), /** Workflows whose optional dependent fields a swap cleared (surfaced, not gated). */ clearedOptional: z.array(forkNeedsConfigurationSchema), + /** + * Acknowledged source-deleted references this sync cleared in the target instead of + * blocking on. Only entries whose source was verified gone in-transaction appear here, so + * an acknowledgment the server refused is visibly absent. + */ + droppedReferences: z + .array(z.object({ kind: forkRemapKindSchema, sourceId: z.string() })) + .default([]), + /** + * Public trigger URLs this sync stopped serving, because no arriving trigger adopted them. + * Reported after the fact so the post-sync toast can name what needs re-registering. + * Defaulted alongside the rest, so an old server's response still parses. + */ + triggerUrlChanges: z.array(forkTriggerUrlChangeSchema).default([]), }), }, }) @@ -683,6 +780,10 @@ export const backgroundWorkMetadataSchema = z needsConfiguration: z.array(forkNeedsConfigurationSchema).optional(), /** Workflows whose optional dependent fields a sync cleared (FYI, non-blocking). */ clearedOptional: z.array(forkNeedsConfigurationSchema).optional(), + /** How many source-deleted references the operator explicitly dropped in this sync. */ + droppedReferences: z.number().int().optional(), + /** How many public trigger URLs this sync stopped serving. */ + triggerUrlChanges: z.number().int().optional(), }) .nullable() export const backgroundWorkItemSchema = z.object({ diff --git a/apps/sim/lib/webhooks/deploy.test.ts b/apps/sim/lib/webhooks/deploy.test.ts index da0f6f4a4f0..4aa780ceeac 100644 --- a/apps/sim/lib/webhooks/deploy.test.ts +++ b/apps/sim/lib/webhooks/deploy.test.ts @@ -10,7 +10,12 @@ import type { BlockState } from '@/stores/workflows/workflow/types' // deploy.ts pulls in the trigger/block/provider registries at module load; none are exercised by // buildProviderConfig (a pure function), so stub them to keep this unit test fast and isolated. -vi.mock('@/blocks', () => ({ getBlock: vi.fn() })) +const { mockGetBlock } = vi.hoisted(() => ({ mockGetBlock: vi.fn() })) +// `deploy.ts` reads the registry through `@/blocks`, while the trigger-id resolution it now +// shares (`@/triggers/webhook-url`) reads `@/blocks/registry`. Point both specifiers at ONE spy +// so a test configuring the block config governs the whole path, not half of it. +vi.mock('@/blocks', () => ({ getBlock: mockGetBlock })) +vi.mock('@/blocks/registry', () => ({ getBlock: mockGetBlock })) vi.mock('@/triggers', () => ({ getTrigger: vi.fn(), isTriggerValid: vi.fn(() => true) })) vi.mock('@/lib/webhooks/providers', () => ({ getProviderHandler: vi.fn() })) vi.mock('@/lib/webhooks/provider-subscriptions', () => ({ diff --git a/apps/sim/lib/webhooks/deploy.ts b/apps/sim/lib/webhooks/deploy.ts index b5e04b99df2..52e3e5244d2 100644 --- a/apps/sim/lib/webhooks/deploy.ts +++ b/apps/sim/lib/webhooks/deploy.ts @@ -32,12 +32,12 @@ import { refreshAccessTokenIfNeeded, resolveOAuthAccountId, } from '@/app/api/auth/oauth/utils' -import { getBlock } from '@/blocks' import type { SubBlockConfig } from '@/blocks/types' import type { BlockState } from '@/stores/workflows/workflow/types' import { getTrigger, isTriggerValid } from '@/triggers' import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' import { SIM_SUBSCRIBED_EVENTS } from '@/triggers/slack/shared' +import { resolveBlockTriggerId } from '@/triggers/webhook-url' const logger = createLogger('DeployWebhookSync') @@ -75,7 +75,7 @@ export async function validateTriggerWebhookConfigForDeploy( const triggerBlocks = Object.values(blocks || {}).filter((b) => b && b.enabled !== false) for (const block of triggerBlocks) { - const triggerId = resolveTriggerId(block) + const triggerId = resolveBlockTriggerId(block) if (!triggerId || !isTriggerValid(triggerId)) continue const triggerDef = getTrigger(triggerId) @@ -172,43 +172,6 @@ function isFieldRequired( return evalCond(condition, subBlockValues) } -function resolveTriggerId(block: BlockState): string | undefined { - const blockConfig = getBlock(block.type) - - if (blockConfig?.category === 'triggers' && isTriggerValid(block.type)) { - return block.type - } - - if (!block.triggerMode) { - return undefined - } - - const selectedTriggerId = getSubBlockValue(block, 'selectedTriggerId') - if (typeof selectedTriggerId === 'string' && isTriggerValid(selectedTriggerId)) { - return selectedTriggerId - } - - const storedTriggerId = getSubBlockValue(block, 'triggerId') - if (typeof storedTriggerId === 'string' && isTriggerValid(storedTriggerId)) { - return storedTriggerId - } - - if (blockConfig?.triggers?.enabled) { - const configuredTriggerId = - typeof selectedTriggerId === 'string' ? selectedTriggerId : undefined - if (configuredTriggerId && isTriggerValid(configuredTriggerId)) { - return configuredTriggerId - } - - const available = blockConfig.triggers?.available?.[0] - if (available && isTriggerValid(available)) { - return available - } - } - - return undefined -} - function getConfigValue(block: BlockState, subBlock: SubBlockConfig): unknown { const fieldValue = getSubBlockValue(block, subBlock.id) @@ -372,7 +335,7 @@ export async function resolveWebhookConfigForBlock(input: { userId: string requestId: string }): Promise { - const triggerId = resolveTriggerId(input.block) + const triggerId = resolveBlockTriggerId(input.block) if (!triggerId || !isTriggerValid(triggerId)) return null const triggerDef = getTrigger(triggerId) diff --git a/apps/sim/lib/workflows/credentials/credential-extractor.test.ts b/apps/sim/lib/workflows/credentials/credential-extractor.test.ts new file mode 100644 index 00000000000..7f609589d0e --- /dev/null +++ b/apps/sim/lib/workflows/credentials/credential-extractor.test.ts @@ -0,0 +1,96 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + EXPORT_PRESERVED_RESOURCE_TYPES, + sanitizeForExport, +} from '@/lib/workflows/credentials/credential-extractor' +import { WORKFLOW_SEARCH_SUBBLOCK_RESOURCE_TYPES } from '@/lib/workflows/search-replace/resources/registry' +import { getBlock } from '@/blocks/registry' +import type { WorkflowState } from '@/stores/workflows/workflow/types' + +function stateWithSubBlock(type: string, value: unknown): Partial { + return { + blocks: { + b1: { + id: 'b1', + type: 'test-block', + name: 'Test', + position: { x: 0, y: 0 }, + subBlocks: { field: { id: 'field', type, value } }, + outputs: {}, + enabled: true, + }, + }, + } as unknown as Partial +} + +function sanitizedValue(type: string, value: unknown): unknown { + vi.mocked(getBlock).mockReturnValue({ + name: 'Test', + description: '', + subBlocks: [{ id: 'field', title: 'Field', type }], + outputs: {}, + } as never) + const sanitized = sanitizeForExport(stateWithSubBlock(type, value)) + return sanitized.blocks?.b1?.subBlocks?.field?.value +} + +describe('export sanitizer resource coverage', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + /** + * The drift guard. Adding a selector to the resource registry without deciding how export + * should treat it fails here rather than silently shipping a workspace-scoped id to another + * workspace — which is exactly how raw `tbl_…` table ids used to escape. + */ + it.each( + WORKFLOW_SEARCH_SUBBLOCK_RESOURCE_TYPES.filter( + (type) => !EXPORT_PRESERVED_RESOURCE_TYPES.has(type) + ) + )('clears %s on export', (type) => { + expect(sanitizedValue(type, 'res-id-123')).toBeNull() + }) + + it('clears a table-selector id, the omission that leaked ids across workspaces', () => { + expect(sanitizedValue('table-selector', 'tbl_239e870374c14d4a89923175a7b10648')).toBeNull() + }) + + /** + * Nothing on the import path remaps workflow references — `import-export.ts` extracts each + * workflow independently under a fresh id — so a preserved reference names a workflow that does + * not exist in the target, bundle or not. + */ + it('clears workflow-selector, since import never remaps the id it names', () => { + expect(sanitizedValue('workflow-selector', 'wf-123')).toBeNull() + }) + + it('still clears oauth-input, via the credential rule rather than the workspace rule', () => { + expect(sanitizedValue('oauth-input', 'cred-123')).toBeNull() + }) + + it('leaves an ordinary field untouched', () => { + expect(sanitizedValue('short-input', 'plain text')).toBe('plain text') + }) + + it('clears tableId by key on a block with no registry config', () => { + vi.mocked(getBlock).mockReturnValue(undefined as never) + const sanitized = sanitizeForExport({ + blocks: { + b1: { + id: 'b1', + type: 'unknown-block', + name: 'Test', + position: { x: 0, y: 0 }, + subBlocks: { tableId: { id: 'tableId', type: 'short-input', value: 'tbl_abc' } }, + outputs: {}, + enabled: true, + }, + }, + } as unknown as Partial) + expect(sanitized.blocks?.b1?.subBlocks?.tableId?.value).toBeNull() + }) +}) diff --git a/apps/sim/lib/workflows/credentials/credential-extractor.ts b/apps/sim/lib/workflows/credentials/credential-extractor.ts index 9540c5891d7..8e9e99bd9c1 100644 --- a/apps/sim/lib/workflows/credentials/credential-extractor.ts +++ b/apps/sim/lib/workflows/credentials/credential-extractor.ts @@ -1,3 +1,4 @@ +import { WORKFLOW_SEARCH_SUBBLOCK_RESOURCE_TYPES } from '@/lib/workflows/search-replace/resources/registry' import { buildCanonicalIndex, buildSubBlockValues, @@ -28,27 +29,49 @@ export interface CredentialRequirement { required: boolean } -// Workspace-specific subblock types that should be cleared -const WORKSPACE_SPECIFIC_TYPES = new Set([ - 'knowledge-base-selector', +/** + * Resource-selector types NOT cleared by the workspace rule below. Everything else the resource + * registry knows about IS cleared, so the two lists can never drift apart again — the previous + * hand-written copy had silently omitted `table-selector`, `mcp-tool-selector`, `user-selector` + * and `sheet-selector`, which is how raw `tbl_…` ids reached other workspaces through an export. + * + * Every id in an export is workspace-scoped, and nothing on the import path remaps them: + * `import-export.ts` extracts each workflow independently and assigns it a fresh id, so a + * preserved reference points at a workflow that does not exist in the target — including inside a + * multi-workflow bundle, where the sibling it named was itself re-created under a new id. Clearing + * is therefore the only correct treatment for every id-bearing selector. + */ +export const EXPORT_PRESERVED_RESOURCE_TYPES: ReadonlySet = new Set([ + // Cleared by the dedicated `oauth-input` branch in `sanitizeWorkflowForSharing`, so excluding it + // here only avoids clearing it twice - it never survives an export. + 'oauth-input', +]) + +/** + * Sub-block types holding a reference scoped to this workspace, or to a credential that is itself + * cleared on export. Derived from the canonical resource registry plus the name/slot-based + * knowledge fields, which carry no resource id and therefore no registry entry. + */ +const WORKSPACE_SPECIFIC_TYPES: ReadonlySet = new Set([ + ...WORKFLOW_SEARCH_SUBBLOCK_RESOURCE_TYPES.filter( + (type) => !EXPORT_PRESERVED_RESOURCE_TYPES.has(type) + ), 'knowledge-tag-filters', - 'document-selector', 'document-tag-entry', - 'file-selector', // Workspace files - 'file-upload', // Uploaded files in workspace - 'project-selector', // Workspace-specific projects - 'channel-selector', // Workspace-specific channels - 'folder-selector', // User-specific folders - 'mcp-server-selector', // User-specific MCP servers ]) -// Field IDs that are workspace-specific +/** + * Field IDs that are workspace-specific, for the fallback pass over blocks with no registry + * config (and over legacy `block.data`). Keyed by sub-block / canonical param id, which the + * type-keyed registry above cannot supply, so this list stays explicit. + */ const WORKSPACE_SPECIFIC_FIELDS = new Set([ 'knowledgeBaseId', 'tagFilters', 'documentTags', 'documentId', 'fileId', + 'tableId', 'projectId', 'channelId', 'folderId', diff --git a/apps/sim/lib/workflows/persistence/duplicate.test.ts b/apps/sim/lib/workflows/persistence/duplicate.test.ts index c2a09c242ae..1952c164dec 100644 --- a/apps/sim/lib/workflows/persistence/duplicate.test.ts +++ b/apps/sim/lib/workflows/persistence/duplicate.test.ts @@ -166,6 +166,11 @@ describe('duplicateWorkflow ordering', () => { subBlocks: { triggerPath: { id: 'triggerPath', type: 'short-input', value: 'old-webhook-path' }, webhookId: { id: 'webhookId', type: 'short-input', value: 'old-webhook-id' }, + triggerConfig: { + id: 'triggerConfig', + type: 'trigger-config', + value: { tableSelector: 'tbl_stale' }, + }, webhookUrlDisplay: { id: 'webhookUrlDisplay', type: 'short-input', @@ -217,6 +222,10 @@ describe('duplicateWorkflow ordering', () => { expect(copiedSubBlocks.triggerPath).toBeUndefined() expect(copiedSubBlocks.webhookId).toBeUndefined() expect(copiedSubBlocks.webhookUrlDisplay).toBeUndefined() + // The aggregate must not ride along: `populateTriggerFieldsFromConfig` re-seeds any empty + // trigger field from it on load, so carrying it would resurrect the source's resource ids in + // the copy right after the remapper cleared or remapped them. + expect(copiedSubBlocks.triggerConfig).toBeUndefined() expect(copiedSubBlocks.variables.value[0].variableId).not.toBe('old-var-id') expect(copiedSubBlocks.variables.value[0].variableName).toBe('customerName') expect(insertedBlocks?.[0].locked).toBe(false) diff --git a/apps/sim/lib/workflows/sanitization/json-sanitizer.ts b/apps/sim/lib/workflows/sanitization/json-sanitizer.ts index 812dbb62f6f..1f05840615e 100644 --- a/apps/sim/lib/workflows/sanitization/json-sanitizer.ts +++ b/apps/sim/lib/workflows/sanitization/json-sanitizer.ts @@ -2,14 +2,11 @@ import { isRecordLike, sortObjectKeysDeep } from '@sim/utils/object' import type { Edge } from 'reactflow' import { getBaseUrl } from '@/lib/core/utils/urls' import { sanitizeWorkflowForSharing } from '@/lib/workflows/credentials/credential-extractor' -import { - buildSubBlockValues, - evaluateSubBlockCondition, -} from '@/lib/workflows/subblocks/visibility' import { getBlock } from '@/blocks/registry' import type { BlockState, Loop, Parallel, WorkflowState } from '@/stores/workflows/workflow/types' import { generateLoopBlocks, generateParallelBlocks } from '@/stores/workflows/workflow/utils' import { TRIGGER_WEBHOOK_URL_FIELD } from '@/triggers/constants' +import { blockAdvertisesWebhookUrl } from '@/triggers/webhook-url' /** * Sanitized workflow state for copilot (removes all UI-specific data) @@ -345,21 +342,7 @@ function sanitizeSubBlocks( * read time, never stored, and rejected on write by `edit_workflow` validation. */ function resolveTriggerWebhookUrl(blockId: string, block: BlockState): string | null { - const blockConfig = getBlock(block.type) - if (!blockConfig) return null - - const actsAsTrigger = blockConfig.category === 'triggers' || block.triggerMode === true - if (!actsAsTrigger) return null - - // A webhook-URL display subblock (`useWebhookUrl`) marks a webhook-based trigger. - // Multi-trigger blocks namespace one per trigger id, each gated by a condition on - // selectedTriggerId — only count a field active for the current values, so a block - // configured with a polling trigger doesn't advertise a webhook URL. - const values = buildSubBlockValues(block.subBlocks || {}) - const hasActiveWebhookUrlField = blockConfig.subBlocks.some( - (sb) => sb.useWebhookUrl === true && evaluateSubBlockCondition(sb.condition, values) - ) - if (!hasActiveWebhookUrlField) return null + if (!blockAdvertisesWebhookUrl(block)) return null const triggerPath = block.subBlocks?.triggerPath?.value const path = typeof triggerPath === 'string' && triggerPath.length > 0 ? triggerPath : blockId diff --git a/apps/sim/lib/workflows/search-replace/resources/registry.test.ts b/apps/sim/lib/workflows/search-replace/resources/registry.test.ts new file mode 100644 index 00000000000..db92f519c40 --- /dev/null +++ b/apps/sim/lib/workflows/search-replace/resources/registry.test.ts @@ -0,0 +1,72 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + getWorkflowSearchSubBlockResourceDefinition, + parseWorkflowSearchSubBlockResources, + workflowSearchResourceValueContains, +} from '@/lib/workflows/search-replace/resources/registry' + +const TABLE_SUB_BLOCK = { type: 'table-selector' } as const + +function replaceTableValue(value: unknown, rawValue: string, replacement: string) { + const definition = getWorkflowSearchSubBlockResourceDefinition(TABLE_SUB_BLOCK) + if (!definition) throw new Error('table-selector is not a registered resource selector') + return definition.codec.replace(value, rawValue, replacement) +} + +/** + * `parse` and `contains` split on commas and trim, so a value carrying stray whitespace is + * detected as a reference. `replace` must agree, or the reference becomes permanently stuck: + * it is reported as needing a mapping, yet neither remapping nor clearing can ever touch it. + */ +describe('scalar resource codec whitespace handling', () => { + const padded = ' tbl_239e870374c14d4a89923175a7b10648 ' + const rawValue = 'tbl_239e870374c14d4a89923175a7b10648' + + it('detects a padded single value as a reference', () => { + const parsed = parseWorkflowSearchSubBlockResources(padded, TABLE_SUB_BLOCK) + expect(parsed.map((reference) => reference.rawValue)).toEqual([rawValue]) + expect( + workflowSearchResourceValueContains( + { subBlockType: 'table-selector', rawValue } as Parameters< + typeof workflowSearchResourceValueContains + >[0], + padded + ) + ).toBe(true) + }) + + it('remaps a padded single value to its target', () => { + expect(replaceTableValue(padded, rawValue, 'tbl_target')).toEqual({ + success: true, + nextValue: 'tbl_target', + }) + }) + + it('clears a padded single value when the reference is unresolved', () => { + expect(replaceTableValue(padded, rawValue, '')).toEqual({ success: true, nextValue: '' }) + }) + + it('leaves a non-matching single value untouched', () => { + expect(replaceTableValue(' tbl_other ', rawValue, 'tbl_target')).toEqual({ + success: true, + nextValue: ' tbl_other ', + }) + }) + + it('still remaps an unpadded single value', () => { + expect(replaceTableValue(rawValue, rawValue, 'tbl_target')).toEqual({ + success: true, + nextValue: 'tbl_target', + }) + }) + + it('still remaps one entry of a padded multi-value list', () => { + expect(replaceTableValue(` ${rawValue} , tbl_other `, rawValue, 'tbl_target')).toEqual({ + success: true, + nextValue: 'tbl_target,tbl_other', + }) + }) +}) diff --git a/apps/sim/lib/workflows/search-replace/resources/registry.ts b/apps/sim/lib/workflows/search-replace/resources/registry.ts index 8215636f339..37f19619ea4 100644 --- a/apps/sim/lib/workflows/search-replace/resources/registry.ts +++ b/apps/sim/lib/workflows/search-replace/resources/registry.ts @@ -135,7 +135,10 @@ function replaceCommaResourceValue( } return { success: true, nextValue } } - const nextValue = shouldReplace(value) ? replacement : value + // Compare the TRIMMED token, matching what `parse` and `contains` produced. Comparing the + // raw string made a padded single value (`" tbl_abc"`) unmatchable here even though it was + // detected as a reference, so it could never be remapped nor cleared and stuck forever. + const nextValue = shouldReplace(parts[0]) ? replacement : value if (targetOccurrenceIndex !== undefined && !replaced) { return { success: false, reason: 'Target resource changed since search' } } @@ -353,6 +356,16 @@ const WORKFLOW_SEARCH_SUBBLOCK_RESOURCES: Partial< 'project-selector': { kind: 'selector-resource', codec: scalarResourceCodec }, } +/** + * Every sub-block type that carries a resource reference. This registry is the single source of + * truth for "does this field hold an id scoped to a workspace or a credential", so consumers that + * need that answer derive it from here rather than keeping a parallel hand-written list — see + * `sanitizeWorkflowForSharing`, whose hand-maintained copy had silently omitted `table-selector`. + */ +export const WORKFLOW_SEARCH_SUBBLOCK_RESOURCE_TYPES = Object.keys( + WORKFLOW_SEARCH_SUBBLOCK_RESOURCES +) as SubBlockType[] + export function getWorkflowSearchResourceKindDefinition( kind: WorkflowSearchMatchKind ): WorkflowSearchResourceKindDefinition | null { diff --git a/apps/sim/triggers/webhook-url.test.ts b/apps/sim/triggers/webhook-url.test.ts new file mode 100644 index 00000000000..099fdb68cfb --- /dev/null +++ b/apps/sim/triggers/webhook-url.test.ts @@ -0,0 +1,161 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { getBlock } from '@/blocks/registry' +import type { BlockState } from '@/stores/workflows/workflow/types' +import { + INTERNAL_TRIGGER_PROVIDERS, + isInternalTriggerProvider, + isPollingWebhookProvider, + POLLING_PROVIDERS, +} from '@/triggers/constants' +import { TRIGGER_REGISTRY } from '@/triggers/registry' +import { blockAdvertisesWebhookUrl } from '@/triggers/webhook-url' + +function block(overrides: Partial = {}): BlockState { + return { + id: 'blk', + type: 'slack', + name: 'Slack', + subBlocks: {}, + outputs: {}, + enabled: true, + ...overrides, + } as unknown as BlockState +} + +describe('blockAdvertisesWebhookUrl', () => { + beforeEach(() => vi.clearAllMocks()) + + it('is true for a trigger block with an unconditional webhook-URL field', () => { + vi.mocked(getBlock).mockReturnValue({ + category: 'triggers', + subBlocks: [{ id: 'triggerWebhookUrl', useWebhookUrl: true }], + } as never) + expect(blockAdvertisesWebhookUrl(block())).toBe(true) + }) + + it('is false for a trigger block that declares no webhook-URL field (poller, schedule, chat)', () => { + vi.mocked(getBlock).mockReturnValue({ + category: 'triggers', + subBlocks: [{ id: 'cron' }], + } as never) + expect(blockAdvertisesWebhookUrl(block())).toBe(false) + }) + + it('is false for a non-trigger block, even one whose config declares a URL field', () => { + vi.mocked(getBlock).mockReturnValue({ + category: 'blocks', + subBlocks: [{ id: 'triggerWebhookUrl', useWebhookUrl: true }], + } as never) + expect(blockAdvertisesWebhookUrl(block())).toBe(false) + }) + + it('is true for a tool block flipped into trigger mode', () => { + vi.mocked(getBlock).mockReturnValue({ + category: 'blocks', + subBlocks: [{ id: 'triggerWebhookUrl', useWebhookUrl: true }], + } as never) + expect(blockAdvertisesWebhookUrl(block({ triggerMode: true } as never))).toBe(true) + }) + + /** + * The case a trigger-definition flag cannot express: one block hosts several triggers, and only + * some of them serve a URL. Reading the ACTIVE condition is what keeps a block currently set to + * the polling trigger from claiming a URL its webhook sibling would have. + */ + it('honours the selectedTriggerId condition on a multi-trigger block', () => { + vi.mocked(getBlock).mockReturnValue({ + category: 'triggers', + subBlocks: [ + { + id: 'webhookUrl', + useWebhookUrl: true, + condition: { field: 'selectedTriggerId', value: 'service_webhook' }, + }, + ], + } as never) + const pollingBlock = block({ + subBlocks: { selectedTriggerId: { id: 'selectedTriggerId', value: 'service_poller' } }, + } as never) + const webhookBlock = block({ + subBlocks: { selectedTriggerId: { id: 'selectedTriggerId', value: 'service_webhook' } }, + } as never) + expect(blockAdvertisesWebhookUrl(pollingBlock)).toBe(false) + expect(blockAdvertisesWebhookUrl(webhookBlock)).toBe(true) + }) + + it('is false when the block type is not in the registry', () => { + vi.mocked(getBlock).mockReturnValue(undefined as never) + expect(blockAdvertisesWebhookUrl(block())).toBe(false) + }) +}) + +/** + * The two provider registries and the per-subblock `useWebhookUrl` marker must agree on which + * triggers serve a public URL. `POLLING_PROVIDERS` is already pinned against `polling: true` in + * `constants.test.ts`; this closes the remaining gap - a trigger whose events Sim pulls, or whose + * path the public route rejects, must never also advertise a URL to paste into a provider console. + */ +describe('provider registries agree with the webhook-URL marker', () => { + it('no polling or internal trigger declares a webhook-URL sub-block', () => { + const offenders = Object.values(TRIGGER_REGISTRY) + .filter( + (trigger) => + isPollingWebhookProvider(trigger.provider) || isInternalTriggerProvider(trigger.provider) + ) + .filter((trigger) => trigger.subBlocks.some((subBlock) => subBlock.useWebhookUrl === true)) + .map((trigger) => `${trigger.id} (provider: ${trigger.provider})`) + + expect( + offenders, + 'A polling/internal trigger advertising a webhook URL would offer a path nothing external can call' + ).toEqual([]) + }) + + it('keeps both registries non-empty, so neither guard can pass vacuously', () => { + expect(POLLING_PROVIDERS.size).toBeGreaterThan(0) + expect(INTERNAL_TRIGGER_PROVIDERS.size).toBeGreaterThan(0) + }) +}) + +/** + * Slack ships BOTH delivery families, so it is the sharpest test of the marker - and the trigger + * the fork sync's URL preservation exists for. `slack_webhook` is path-based and its URL is what + * a user pastes into a Slack app's Request URL; `slack_oauth` arrives on a shared endpoint routed + * by `routingKey`, so `lib/webhooks/deploy.ts` nulls its path and there is no URL to preserve. + * + * The block configs spread these exact arrays (`blocks/blocks/slack.ts` `...getTrigger(...) + * .subBlocks`), so asserting on the trigger definitions is asserting on what the predicate reads. + */ +describe('Slack: both delivery families classify correctly', () => { + function slackBlock(triggerId: 'slack_webhook' | 'slack_oauth'): BlockState { + vi.mocked(getBlock).mockReturnValue({ + category: 'triggers', + subBlocks: TRIGGER_REGISTRY[triggerId].subBlocks, + } as never) + return block({ type: triggerId === 'slack_webhook' ? 'slack' : 'slack_v2' }) + } + + it('slack_webhook advertises a URL, so the fork sync can preserve it', () => { + expect(blockAdvertisesWebhookUrl(slackBlock('slack_webhook'))).toBe(true) + }) + + it('slack_oauth does NOT, so it is never offered a URL it cannot serve', () => { + expect(blockAdvertisesWebhookUrl(slackBlock('slack_oauth'))).toBe(false) + }) + + /** + * The URL field must stay UNCONDITIONAL on the single-trigger Slack block. A `selectedTriggerId` + * condition would evaluate false there (no dropdown ⇒ no value), silently dropping Slack from + * the Trigger URLs section - the one trigger this feature was built for. + */ + it('slack_webhook gates its URL field on nothing', () => { + const urlField = TRIGGER_REGISTRY.slack_webhook.subBlocks.find( + (subBlock) => subBlock.useWebhookUrl === true + ) + expect(urlField).toBeDefined() + expect(urlField?.condition).toBeUndefined() + }) +}) diff --git a/apps/sim/triggers/webhook-url.ts b/apps/sim/triggers/webhook-url.ts new file mode 100644 index 00000000000..289e928a9cf --- /dev/null +++ b/apps/sim/triggers/webhook-url.ts @@ -0,0 +1,110 @@ +import { getBaseUrl } from '@/lib/core/utils/urls' +import { + buildSubBlockValues, + evaluateSubBlockCondition, +} from '@/lib/workflows/subblocks/visibility' +import { getBlock } from '@/blocks/registry' +import type { BlockState } from '@/stores/workflows/workflow/types' +import { getTrigger, isTriggerValid } from '@/triggers' + +/** The public URL an external system POSTs to for a given webhook path. */ +export function buildWebhookTriggerUrl(path: string): string { + return `${getBaseUrl()}/api/webhooks/trigger/${path}` +} + +function subBlockValue(block: BlockState, subBlockId: string): unknown { + return block.subBlocks?.[subBlockId]?.value +} + +/** + * The trigger a block deploys as, or undefined when it is not acting as one. + * + * A dedicated trigger block IS its trigger; a tool block flipped into trigger mode names one via + * `selectedTriggerId` / `triggerId`, falling back to the first trigger its config declares + * available. Single-sourced here because the webhook deploy path and anything reasoning about a + * block's delivery must agree on the answer - two resolutions would let a block deploy as one + * trigger while another layer classified it as a different one. + */ +export function resolveBlockTriggerId(block: BlockState): string | undefined { + const blockConfig = getBlock(block.type) + + if (blockConfig?.category === 'triggers' && isTriggerValid(block.type)) { + return block.type + } + + if (!block.triggerMode) { + return undefined + } + + const selectedTriggerId = subBlockValue(block, 'selectedTriggerId') + if (typeof selectedTriggerId === 'string' && isTriggerValid(selectedTriggerId)) { + return selectedTriggerId + } + + const storedTriggerId = subBlockValue(block, 'triggerId') + if (typeof storedTriggerId === 'string' && isTriggerValid(storedTriggerId)) { + return storedTriggerId + } + + if (blockConfig?.triggers?.enabled) { + const configuredTriggerId = + typeof selectedTriggerId === 'string' ? selectedTriggerId : undefined + if (configuredTriggerId && isTriggerValid(configuredTriggerId)) { + return configuredTriggerId + } + + const available = blockConfig.triggers?.available?.[0] + if (available && isTriggerValid(available)) { + return available + } + } + + return undefined +} + +/** + * The webhook provider a block's events arrive under, or null when it is not a trigger. + * + * This is the identity an inbound request is verified against: a path served by a `slack` webhook + * authenticates Slack's signature and parses Slack's event shape. Two triggers of DIFFERENT + * providers can therefore never share a URL meaningfully, however similar they look. + */ +export function resolveBlockTriggerProvider(block: BlockState): string | null { + const triggerId = resolveBlockTriggerId(block) + if (!triggerId || !isTriggerValid(triggerId)) return null + return getTrigger(triggerId).provider ?? null +} + +/** + * Whether this block advertises a public webhook URL - one an external system POSTs to at + * `/api/webhooks/trigger/`. + * + * The marker is the `useWebhookUrl` sub-block: the field that renders the copyable URL in the + * block's own config. If the UI shows a URL for a block, that is a URL someone could have pasted + * into Slack or a provider console; if it does not, there is nothing external pointing at it. + * That makes this the right question for anything reasoning about "would changing this break a + * caller" - which is why the copilot's read view and the fork sync both ask it here rather than + * re-deriving it. + * + * Deliberately NOT derived from the trigger definition. Neither declarative flag separates the + * families cleanly: `polling` is set on 8 of the trigger defs while several pollers omit it, and + * `webhook` is set on ~345 including `slack_oauth`, which routes by `routingKey` on a shared + * endpoint and has no per-workflow URL at all. + * + * Condition-aware on purpose: a multi-trigger block namespaces one URL field per trigger id, each + * gated on `selectedTriggerId`, so a block currently configured with a POLLING trigger correctly + * reports false even though its config declares a URL field for a sibling trigger. + */ +export function blockAdvertisesWebhookUrl(block: BlockState): boolean { + const blockConfig = getBlock(block.type) + if (!blockConfig) return false + + const actsAsTrigger = blockConfig.category === 'triggers' || block.triggerMode === true + if (!actsAsTrigger) return false + + const values = buildSubBlockValues(block.subBlocks || {}) + return blockConfig.subBlocks.some( + (subBlock) => + subBlock.useWebhookUrl === true && evaluateSubBlockCondition(subBlock.condition, values) + ) +} From 53c3beace18753291581163aa131d3907c03be17 Mon Sep 17 00:00:00 2001 From: Waleed Date: Wed, 5 Aug 2026 10:22:55 -0700 Subject: [PATCH 06/10] fix(providers): name the header phase on streaming OpenAI requests (#6288) --- .../openai/core.transport-phase.test.ts | 14 ++++++ apps/sim/providers/openai/core.ts | 44 +++++++++++-------- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/apps/sim/providers/openai/core.transport-phase.test.ts b/apps/sim/providers/openai/core.transport-phase.test.ts index 74459fc40e5..d0a27422a99 100644 --- a/apps/sim/providers/openai/core.transport-phase.test.ts +++ b/apps/sim/providers/openai/core.transport-phase.test.ts @@ -198,6 +198,20 @@ describe('OpenAI transport phase annotation', () => { expect(error.message.match(/phase=/g)).toHaveLength(1) }) + /** + * Streaming calls the request helper directly and never reaches `postResponses`, so a + * header stall on a chat or SSE run used to surface as the bare runtime string with no + * phase, elapsed time, or request id. + */ + it('names the header phase on a streaming request too', async () => { + const error = await run(vi.fn().mockRejectedValue(timeoutError()), { + stream: true, + }).catch((e) => e) + + expect(error.message).toContain('phase=awaiting-response-headers') + expect(error.message).toMatch(/elapsedMs=\d+/) + }) + it('leaves a healthy response entirely unaffected', async () => { const fetchMock = vi.fn().mockResolvedValue({ ok: true, diff --git a/apps/sim/providers/openai/core.ts b/apps/sim/providers/openai/core.ts index 739d6d21e87..77848c08937 100644 --- a/apps/sim/providers/openai/core.ts +++ b/apps/sim/providers/openai/core.ts @@ -410,6 +410,29 @@ export async function executeResponsesProviderRequest( let reasoningSummariesUnavailable = false + /** + * The single point every Responses request leaves through, so a stall waiting for + * headers is named on the streaming paths too — they call + * {@link fetchResponsesWithSummaryFallback} directly and never reach `postResponses`, + * which is where the annotation used to live. + */ + const postOnce = async ( + payload: Record, + abortSignal: AbortSignal | undefined, + startedAt: number + ): Promise => { + try { + return await fetchImpl(config.endpoint, { + method: 'POST', + headers: config.headers, + body: JSON.stringify(payload), + signal: abortSignal, + }) + } catch (error) { + throw annotateTransportFailure(error, 'awaiting-response-headers', startedAt) + } + } + const fetchResponsesWithSummaryFallback = async ( requestedBody: Record, startedAt: number, @@ -418,12 +441,7 @@ export async function executeResponsesProviderRequest( const body = reasoningSummariesUnavailable ? (stripReasoningSummary(requestedBody) ?? requestedBody) : requestedBody - const response = await fetchImpl(config.endpoint, { - method: 'POST', - headers: config.headers, - body: JSON.stringify(body), - signal: abortSignal, - }) + const response = await postOnce(body, abortSignal, startedAt) if (response.ok) return response const message = await parseErrorResponse(response, startedAt) @@ -439,12 +457,7 @@ export async function executeResponsesProviderRequest( `${config.providerLabel} rejected reasoning summaries (organization not verified); retrying without summary`, { model: config.modelName } ) - const retryResponse = await fetchImpl(config.endpoint, { - method: 'POST', - headers: config.headers, - body: JSON.stringify(strippedBody), - signal: abortSignal, - }) + const retryResponse = await postOnce(strippedBody, abortSignal, startedAt) if (!retryResponse.ok) { const retryMessage = await parseErrorResponse(retryResponse, startedAt) throw new Error( @@ -459,12 +472,7 @@ export async function executeResponsesProviderRequest( ): Promise => { const startedAt = Date.now() - let response: Response - try { - response = await fetchResponsesWithSummaryFallback(body, startedAt) - } catch (error) { - throw annotateTransportFailure(error, 'awaiting-response-headers', startedAt) - } + const response = await fetchResponsesWithSummaryFallback(body, startedAt) const responseMeta = { ...describeResponse(response), ttfbMs: Date.now() - startedAt } From 402f8622103b2cbacad67cd5a95621d0161bb641 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 5 Aug 2026 10:44:44 -0700 Subject: [PATCH 07/10] fix(forking): make the trigger URL preview reflect the user's actual picks (#6290) Two Cursor findings on #6272, both in the preview layer - the sync's write path was correct in each case, but the UI stated an outcome that did not match it. - The heads-up and overwrite confirm read `triggerUrlChanges` straight off the diff, which the server computes with its DEFAULT resolution before the user chooses anything. Selecting "Generate new URL" for a trigger that would have adopted a URL therefore killed that URL with no warning, in the one modal whose job is to state irreversible consequences (it also over-warned in the reverse case). The diff now returns the RAW retiring set and the client subtracts the live choices, so the rows, the heads-up and the confirm cannot disagree. - The picker let two triggers select the same retiring URL and showed both as keeping it. Two blocks cannot serve one path (`path_deployment_unique`) and the resolver awards it to the first slot, so the loser silently got a new URL. A path another row claimed is now disabled and named, and each row displays its RESOLVED outcome rather than its raw pick. The choice resolution is a pure module mirroring `resolveForkTriggerPaths` (offered-paths guard, first-claim-wins), so the preview and the server agree by construction rather than by two hand-kept implementations. Co-authored-by: Claude Opus 5 (1M context) --- .../api/workspaces/[id]/fork/diff/route.ts | 15 ++- .../components/fork-sync/fork-sync-view.tsx | 28 +++-- .../fork-sync/trigger-choices.test.ts | 114 ++++++++++++++++++ .../components/fork-sync/trigger-choices.ts | 63 ++++++++++ .../components/fork-sync/use-fork-sync.ts | 43 ++++++- .../lib/api/contracts/workspace-fork.test.ts | 6 +- apps/sim/lib/api/contracts/workspace-fork.ts | 11 +- 7 files changed, 257 insertions(+), 23 deletions(-) create mode 100644 apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts create mode 100644 apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts diff --git a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts index 72b0d7dd435..4bb41317799 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts @@ -28,10 +28,7 @@ import { collectForkClearedRefCandidates, } from '@/ee/workspace-forking/lib/promote/cleared-refs' import { computeForkPromotePlan } from '@/ee/workspace-forking/lib/promote/promote-plan' -import { - buildForkTriggerPlan, - resolveForkTriggerPaths, -} from '@/ee/workspace-forking/lib/promote/trigger-urls' +import { buildForkTriggerPlan } from '@/ee/workspace-forking/lib/promote/trigger-urls' import { buildForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' import { readTargetDraftDependentValue } from '@/ee/workspace-forking/lib/remap/remap-references' @@ -188,7 +185,13 @@ export const GET = withRouteHandler( resolveBlockId, targetWebhooks: await loadTargetWebhookPathsByBlock(db, allTargetIds), }) - const { changes: triggerUrlChanges } = resolveForkTriggerPaths(triggerPlan) + // The RAW retiring set, not the default resolution: the client derives which of these actually + // stop being served from the picks the user is making right now, so the heads-up and the + // overwrite confirm can never disagree with the Trigger URLs rows. + const retiringTriggerUrls = triggerPlan.retiring.map((row) => ({ + workflowName: row.workflowName, + path: row.path, + })) // Every trigger that HAS a public URL, plus every one whose URL is up for decision - not just // the decisions, so the section reads as a standing statement of each URL rather than an alert. // @@ -259,7 +262,7 @@ export const GET = withRouteHandler( resourceUsages: collectForkResourceUsages(plan.items, sourceStates), copyableUnmapped: plan.copyableUnmapped, clearedRefs, - triggerUrlChanges, + retiringTriggerUrls, triggerMappings, }) } diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx index a3ab75a55d2..266a1a993be 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx +++ b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx @@ -598,10 +598,10 @@ function TriggerMappingRow({ controller, mapping }: TriggerMappingRowProps) { // A trigger that already serves a URL keeps it, so the row states the URL and offers no // control. Only a trigger the sync would give a NEW URL has something to decide. const decidable = mapping.ownPath === null && mapping.adoptablePaths.length > 0 - const chosen = - mapping.sourceBlockId in controller.triggerAdoptions - ? controller.triggerAdoptions[mapping.sourceBlockId] - : (mapping.defaultAdoptPath ?? '') + const pathOwners = controller.triggerPathOwnersFor(mapping.sourceBlockId) + // The RESOLVED choice, not the raw pick: a path another row claimed first is awarded once, so + // displaying the raw pick would promise a URL this row is not going to get. + const chosen = controller.triggerChoiceFor(mapping.sourceBlockId) const resultingPath = mapping.ownPath ?? (chosen === '' ? null : chosen) return ( @@ -625,13 +625,23 @@ function TriggerMappingRow({ controller, mapping }: TriggerMappingRowProps) { // The full URL lives under the row and follows the selection, so an option only // has to name the CHOICE. Several retiring URLs is the one case that needs a // disambiguator, and the path tail is what distinguishes them. - ...mapping.adoptablePaths.map((path) => ({ - label: + // + // A URL another trigger already took is disabled and says who took it: two blocks + // cannot serve one path, and the resolver awards it to the first slot - so + // allowing the pick would leave this row reading "Keeps this URL" while the sync + // silently minted it a new one. + ...mapping.adoptablePaths.map((path) => { + const owner = pathOwners.get(path) + const base = mapping.adoptablePaths.length === 1 ? 'Keep existing URL' - : `Keep …${path.slice(-12)}`, - value: path, - })), + : `Keep …${path.slice(-12)}` + return { + label: owner ? `${base} · taken by ${owner}` : base, + value: path, + disabled: owner !== undefined, + } + }), { label: 'Generate new URL', value: NEW_TRIGGER_URL_VALUE }, ]} value={chosen === '' ? NEW_TRIGGER_URL_VALUE : chosen} diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts new file mode 100644 index 00000000000..1fdd3717ede --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts @@ -0,0 +1,114 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import type { ForkTriggerMapping } from '@/lib/api/contracts/workspace-fork' +import { + forkDyingTriggerUrls, + forkTriggerChoices, + forkTriggerPathOwners, +} from '@/ee/workspace-forking/components/fork-sync/trigger-choices' + +function mapping(overrides: Partial = {}): ForkTriggerMapping { + return { + sourceBlockId: 'blk', + blockName: 'Slack messages', + workflowName: 'ITSM intake', + ownPath: null, + adoptablePaths: ['p1'], + defaultAdoptPath: 'p1', + ...overrides, + } +} + +describe('forkTriggerChoices', () => { + it('takes the default when the user has not chosen', () => { + expect(forkTriggerChoices([mapping()], {}).get('blk')).toBe('p1') + }) + + it('honours an explicit pick over the default', () => { + const mappings = [mapping({ adoptablePaths: ['p1', 'p2'], defaultAdoptPath: null })] + expect(forkTriggerChoices(mappings, { blk: 'p2' }).get('blk')).toBe('p2') + }) + + it("treats an explicit '' as minting a new URL, overriding the default", () => { + expect(forkTriggerChoices([mapping()], { blk: '' }).get('blk')).toBe('') + }) + + it('ignores a pick the slot never offered', () => { + expect(forkTriggerChoices([mapping()], { blk: 'not-offered' }).get('blk')).toBe('') + }) + + /** + * Two blocks cannot serve one path (`path_deployment_unique`) and the server awards it to the + * first slot, so the second row's real outcome is a NEW URL - not the path it asked for. + */ + it('awards a contested path to the first row only', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', blockName: 'Slack A', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', blockName: 'Slack B', defaultAdoptPath: null }), + ] + const chosen = forkTriggerChoices(mappings, { a: 'p1', b: 'p1' }) + expect(chosen.get('a')).toBe('p1') + expect(chosen.get('b')).toBe('') + }) +}) + +describe('forkDyingTriggerUrls', () => { + const retiring = [ + { workflowName: 'ITSM intake', path: 'p1' }, + { workflowName: 'ITSM intake', path: 'p2' }, + ] + + it('excludes a URL some row adopts', () => { + const chosen = forkTriggerChoices([mapping()], {}) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p2']) + }) + + /** + * The bug this exists for: the server computes its warning from the DEFAULT resolution, so + * choosing "Generate new URL" used to kill a URL the confirm never mentioned. + */ + it('re-lists a URL once the user opts into a new one instead', () => { + const chosen = forkTriggerChoices([mapping()], { blk: '' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p1', 'p2']) + }) + + it('drops a URL the user adopts where the default adopted nothing', () => { + const mappings = [mapping({ adoptablePaths: ['p1', 'p2'], defaultAdoptPath: null })] + const chosen = forkTriggerChoices(mappings, { blk: 'p2' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p1']) + }) + + /** A contested path is still served by its winner, so it is not dying. */ + it('counts a contested path as adopted exactly once', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', defaultAdoptPath: null }), + ] + const chosen = forkTriggerChoices(mappings, { a: 'p1', b: 'p1' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p2']) + }) +}) + +describe('forkTriggerPathOwners', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', blockName: 'Slack A', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', blockName: 'Slack B', defaultAdoptPath: null }), + ] + + it('names the row that claimed a path, from another row’s perspective', () => { + const chosen = forkTriggerChoices(mappings, { a: 'p1' }) + expect(forkTriggerPathOwners(mappings, chosen, 'b').get('p1')).toBe('Slack A') + }) + + it('never reports a row as the owner of its own claim', () => { + const chosen = forkTriggerChoices(mappings, { a: 'p1' }) + expect(forkTriggerPathOwners(mappings, chosen, 'a').has('p1')).toBe(false) + }) + + it('reports nothing while no row has claimed anything', () => { + const chosen = forkTriggerChoices(mappings, {}) + expect(forkTriggerPathOwners(mappings, chosen, 'b').size).toBe(0) + }) +}) diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts new file mode 100644 index 00000000000..914f12a533a --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts @@ -0,0 +1,63 @@ +import type { ForkTriggerMapping, ForkTriggerUrlChange } from '@/lib/api/contracts/workspace-fork' + +/** + * Which retiring URL each arriving trigger currently takes, keyed by source block id. `''` means + * "mint a new URL". + * + * Mirrors `resolveForkTriggerPaths` on the server, which is what makes the preview trustworthy: + * an override counts only for a path the slot actually offered, and a path is awarded to the + * FIRST row that claims it - two blocks cannot serve one path (`path_deployment_unique`), so a + * later row claiming the same URL silently receives a new one instead. + */ +export function forkTriggerChoices( + mappings: readonly ForkTriggerMapping[], + adoptions: Readonly> +): Map { + const chosen = new Map() + const claimed = new Set() + for (const mapping of mappings) { + const picked = + mapping.sourceBlockId in adoptions + ? adoptions[mapping.sourceBlockId] + : (mapping.defaultAdoptPath ?? '') + const honoured = + picked !== '' && mapping.adoptablePaths.includes(picked) && !claimed.has(picked) ? picked : '' + if (honoured !== '') claimed.add(honoured) + chosen.set(mapping.sourceBlockId, honoured) + } + return chosen +} + +/** + * The retiring URLs the CURRENT choices leave unserved. + * + * Derived from the raw retiring set rather than read off the diff: the server computes its own + * default before the user picks anything, so a preview built from it would omit a URL the user + * has just chosen to abandon - in the one modal that exists to state irreversible consequences. + */ +export function forkDyingTriggerUrls( + retiring: readonly ForkTriggerUrlChange[], + chosen: ReadonlyMap +): ForkTriggerUrlChange[] { + const adopted = new Set(Array.from(chosen.values()).filter((path) => path !== '')) + return retiring.filter((row) => !adopted.has(row.path)) +} + +/** + * The block name already claiming each path, from the perspective of one row - so its picker can + * disable a URL another trigger took rather than letting the user select a choice the sync will + * silently overrule. + */ +export function forkTriggerPathOwners( + mappings: readonly ForkTriggerMapping[], + chosen: ReadonlyMap, + forSourceBlockId: string +): Map { + const owners = new Map() + for (const mapping of mappings) { + if (mapping.sourceBlockId === forSourceBlockId) continue + const pick = chosen.get(mapping.sourceBlockId) + if (pick) owners.set(pick, mapping.blockName) + } + return owners +} diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts index 54b3669a249..c28d29c371b 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts @@ -35,6 +35,11 @@ import { effectiveCopyDependentValue, effectiveDependentValue, } from '@/ee/workspace-forking/components/fork-sync/dependent-value' +import { + forkDyingTriggerUrls, + forkTriggerChoices, + forkTriggerPathOwners, +} from '@/ee/workspace-forking/components/fork-sync/trigger-choices' import { type ForkDirection, useForkDiff, @@ -180,7 +185,10 @@ export interface ForkSyncController { workflowChanges: ForkWorkflowChange[] /** Names of target workflows this sync archives, for the confirm modal. */ archivedWorkflowNames: string[] - /** Public trigger URLs this sync would stop serving in the target (warn before overwriting). */ + /** + * Public trigger URLs the CURRENT picks leave unserved. Derived from the retiring set and the + * live adoption choices, so the heads-up, the overwrite confirm and the rows always agree. + */ triggerUrlChanges: ForkTriggerUrlChange[] /** Arriving triggers whose URL is a choice: keep a retiring one, or mint a new one. */ triggerMappings: ForkTriggerMapping[] @@ -190,6 +198,13 @@ export interface ForkSyncController { */ triggerAdoptions: Readonly> setTriggerAdoption: (sourceBlockId: string, path: string) => void + /** Paths another trigger row has already claimed, so this row can disable them. */ + triggerPathOwnersFor: (sourceBlockId: string) => ReadonlyMap + /** + * The path a row will actually serve, resolved the same way the server resolves it. Never + * reports a path another row claimed first, so the row's displayed URL is its real outcome. + */ + triggerChoiceFor: (sourceBlockId: string) => string /** Names of deployed SOURCE workflows marked "Exclude from sync" - never sent. */ excludedSourceWorkflows: string[] /** Names of mapped TARGET workflows marked "Exclude from sync" - never replaced or archived. */ @@ -320,6 +335,10 @@ export function useForkSync(params: { () => diff.data?.triggerMappings ?? [], [diff.data?.triggerMappings] ) + const retiringTriggerUrls = useMemo( + () => diff.data?.retiringTriggerUrls ?? [], + [diff.data?.retiringTriggerUrls] + ) // Keys the backend offers as copy candidates, so the entry rows show a "Copy instead" // affordance only for those - clearing a name-match suggestion returns the ref to the copy @@ -782,6 +801,24 @@ export function useForkSync(params: { setTriggerAdoptions((prev) => ({ ...prev, [sourceBlockId]: path })) } + /** Live choices, resolved exactly as the server will resolve them (first claim wins a path). */ + const chosenTriggerPaths = useMemo( + () => forkTriggerChoices(triggerMappings, triggerAdoptions), + [triggerMappings, triggerAdoptions] + ) + + const triggerUrlChanges = useMemo( + () => forkDyingTriggerUrls(retiringTriggerUrls, chosenTriggerPaths), + [retiringTriggerUrls, chosenTriggerPaths] + ) + + const triggerPathOwnersFor = (sourceBlockId: string): ReadonlyMap => + forkTriggerPathOwners(triggerMappings, chosenTriggerPaths, sourceBlockId) + + /** The path a row will actually serve, or '' for a new URL - never a claim another row won. */ + const triggerChoiceFor = (sourceBlockId: string): string => + chosenTriggerPaths.get(sourceBlockId) ?? '' + const discard = () => { setTargets({}) setReconfig({}) @@ -965,10 +1002,12 @@ export function useForkSync(params: { dependentClears, workflowChanges, archivedWorkflowNames, - triggerUrlChanges: diff.data?.triggerUrlChanges ?? [], + triggerUrlChanges, triggerMappings, triggerAdoptions, setTriggerAdoption, + triggerPathOwnersFor, + triggerChoiceFor, excludedSourceWorkflows: diff.data?.excludedSourceWorkflows ?? [], excludedTargetWorkflows: diff.data?.excludedTargetWorkflows ?? [], mcpReauthCount: diff.data?.mcpReauthServerIds.length ?? 0, diff --git a/apps/sim/lib/api/contracts/workspace-fork.test.ts b/apps/sim/lib/api/contracts/workspace-fork.test.ts index 03ebc5d2326..b8defe69939 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.test.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.test.ts @@ -188,7 +188,7 @@ describe('getForkDiffContract response excluded-workflow lists', () => { const parsed = getForkDiffContract.response.schema.parse(baseDiffResponse) expect(parsed.excludedSourceWorkflows).toEqual([]) expect(parsed.excludedTargetWorkflows).toEqual([]) - expect(parsed.triggerUrlChanges).toEqual([]) + expect(parsed.retiringTriggerUrls).toEqual([]) expect(parsed.triggerMappings).toEqual([]) }) @@ -215,12 +215,12 @@ describe('getForkDiffContract response excluded-workflow lists', () => { defaultAdoptPath: 'live-slack-path', }, ], - triggerUrlChanges: [{ workflowName: 'ITSM intake', path: 'dead-path' }], + retiringTriggerUrls: [{ workflowName: 'ITSM intake', path: 'dead-path' }], }) expect(parsed.triggerMappings[0].ownPath).toBe('prod-live-path') expect(parsed.triggerMappings[0].adoptablePaths).toEqual([]) expect(parsed.triggerMappings[1].defaultAdoptPath).toBe('live-slack-path') - expect(parsed.triggerUrlChanges[0].path).toBe('dead-path') + expect(parsed.retiringTriggerUrls[0].path).toBe('dead-path') }) it('accepts a trigger mapping choice on the promote body, including "new URL"', () => { diff --git a/apps/sim/lib/api/contracts/workspace-fork.ts b/apps/sim/lib/api/contracts/workspace-fork.ts index 5546100fdcf..c861a0995d6 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.ts @@ -604,10 +604,15 @@ export const getForkDiffContract = defineRouteContract({ */ clearedRefs: z.array(forkClearedRefSchema), /** - * Public trigger URLs this sync would stop serving in the target. Defaulted so a new client - * tolerates an old server's response during rollout. + * Every public trigger URL this sync retires in the target, BEFORE any adoption is applied. + * + * Deliberately pre-adoption: which of these actually stop being served depends on the + * caller's live picks in `triggerMappings`, which only exist client-side until the promote + * call. Returning the post-default set instead would freeze the preview at the server's + * guess, so choosing "Generate new URL" would kill a URL the confirm never warned about. + * Defaulted so a new client tolerates an old server's response during rollout. */ - triggerUrlChanges: z.array(forkTriggerUrlChangeSchema).default([]), + retiringTriggerUrls: z.array(forkTriggerUrlChangeSchema).default([]), /** Arriving trigger blocks whose URL this sync decides, with their adoptable alternatives. */ triggerMappings: z.array(forkTriggerMappingSchema).default([]), }), From 0583e2085f310c075bfc446f2999f57e8546fea8 Mon Sep 17 00:00:00 2001 From: Waleed Date: Wed, 5 Aug 2026 11:21:46 -0700 Subject: [PATCH 08/10] improvement(emcn): let every primitive inherit the document font weight (#6291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * improvement(emcn): let every primitive inherit the document font weight #6241 flattened the type scale by deleting the tailwind `fontWeight` override that remapped `font-medium` to 440/480. Nothing was restyled, but the utility's meaning changed: every one of ~505 call sites written when `font-medium` sat ~10 units above body snapped to a stock 500 against a body that also dropped 430 -> 400. The tab strip and table header Emir reported are two symptoms. The same PR added the "Font Weight" section to sim-styling.md declaring the end state (400 default, weight class only to step up) without migrating the codebase to it, so the rule and its violations shipped together and no gate could flag it. Removes the hardcoded weight from the ~20 pre-chip emcn primitives so they inherit 400, matching the chip family that was already correct. Deletes the three `font-normal` overrides that existed only to undo those defaults (`TabStrip`, `ChipCombobox`, `ChipModalField`) — their TSDoc said as much. `` needed care in the other direction: Preflight resets h1-h6 but not `th`, so a header keeps the UA bold 700 and `font-medium` was holding it *down*. Deleting it made headers heavier. Rather than neutralize per call site — the codebase had already accumulated 12 such workarounds — globals.css completes Preflight with `th { font-weight: inherit }`, and the 8 now-dead `font-normal` workarounds come out. The rule keeps no element-level exceptions. Two width-measurement mirrors had to move with the text they measure: table-grid's auto-fit canvas and tag-input's hidden sizing span. Left stale, both would mis-measure. Also: drops an inline `font-weight:500` from the header drag ghost, normalizes `text-[13px]`/`text-[11px]` to `text-small`/`text-xs`, aligns the landing table previews that clone the product header, documents AvatarFallback's deliberate step-up, and corrects the stale AGENTS.md line claiming Button owns a weight. Verified: typecheck 0, lint clean, full vitest run identical to origin/staging across three runs (158 failed suites / 1 failed test / 16350 passing — all pre-existing: a PostCSS env error and a missing `rg` binary). * fix(tables): align the workflow-group drag ghost with the label it previews Cursor Bugbot caught the sibling of the ghost fixed in column-header-menu: this one kept an inline `font-weight:500` after the band label dropped to the inherited 400, so the drag preview no longer matched the text it represents — the same mirror drift this PR fixes for the width-measurement spans. It was drifted on size too: hardcoded `font-size:13px` against a label that is `text-xs` (11px), so it never matched. Both now come from the token the label uses, which also clears the inline fontWeight the styling rule bans. * fix(toolbar): stop the block drag preview hardcoding a weight its label does not use Third instance of the same mirror drift, found by sweeping rather than waiting for it to be reported. The toolbar item label renders at the inherited 400 (its container is `chipVariants`, which is weight-free), but the drag preview hardcoded `font-weight: 500` — so the preview never matched the item it previews, before or after this PR. Drops the weight only. The 16px is left alone: a drag preview reading larger than its source is a deliberate affordance, not a mismatch. Not fixed here, deliberately: the `font-weight: 500` in code-editor.tsx's highlight layer. That HTML is rendered in registration with a transparent textarea, so weight affects glyph advance widths and the caret alignment — it needs live in-editor verification, unlike a detached drag ghost. Its two placeholder branches already disagree on weight, so it wants its own change. * revert(canvas): drop the workflow-editor panel changes from this PR The canvas panel is under active modification elsewhere, so this PR stays out of it. Reverts the toolbar search input, the sub-block table cell and its overlay mirror, the messages-input textarea and its mirror, and the block drag preview — all back to staging verbatim. Cursor Bugbot was right about that last one and I was wrong: the preview mirrors the drag DESTINATION (its TSDoc says "looks like a workflow block", and 250px/16px are block-card dimensions), not the toolbar chip I had compared it against. workflow-block-view renders the title `font-medium text-md`, so its 500 was correct. Moot now that the file is reverted, but worth recording so the next sweep does not repeat the mistake. The globals `th` rule still covers the sub-block table header without a call-site class; the explicit font-medium there simply wins over it, exactly as staging renders today. --- .../landing-preview-logs.tsx | 5 +- .../landing-preview-resource.tsx | 5 +- .../landing-preview-tables.tsx | 2 +- .../components/knowledge-hero-loop.tsx | 2 +- .../logs/components/logs-hero-loop.tsx | 2 +- .../tables/components/tables-hero-loop.tsx | 2 +- apps/sim/app/_styles/globals.css | 9 +++ .../new-column-dropdown.tsx | 2 +- .../table-grid/headers/column-header-menu.tsx | 9 +-- .../headers/workflow-group-meta-cell.tsx | 7 +-- .../components/table-grid/table-grid.tsx | 3 +- packages/emcn/src/AGENTS.md | 2 +- .../emcn/src/components/avatar/avatar.tsx | 5 ++ packages/emcn/src/components/badge/badge.tsx | 63 +++++++++---------- .../components/button-group/button-group.tsx | 2 +- .../emcn/src/components/button/button.tsx | 2 +- .../chip-combobox/chip-combobox.tsx | 7 ++- .../src/components/chip-modal/chip-modal.tsx | 5 +- packages/emcn/src/components/code/code.tsx | 6 +- .../collapsible-card/collapsible-card.tsx | 4 +- .../emcn/src/components/combobox/combobox.tsx | 12 ++-- .../dropdown-menu/dropdown-menu.tsx | 6 +- .../src/components/input-otp/input-otp.tsx | 2 +- packages/emcn/src/components/input/input.tsx | 2 +- packages/emcn/src/components/label/label.tsx | 2 +- packages/emcn/src/components/modal/modal.tsx | 4 +- .../emcn/src/components/popover/popover.tsx | 12 +--- .../progress-item/progress-item.tsx | 4 +- .../src/components/tab-strip/tab-strip.tsx | 2 +- packages/emcn/src/components/table/table.tsx | 4 +- .../src/components/tag-input/tag-input.tsx | 8 +-- .../emcn/src/components/textarea/textarea.tsx | 2 +- .../components/time-picker/time-picker.tsx | 10 +-- 33 files changed, 101 insertions(+), 113 deletions(-) diff --git a/apps/sim/app/(landing)/components/landing-preview/components/landing-preview-logs/landing-preview-logs.tsx b/apps/sim/app/(landing)/components/landing-preview/components/landing-preview-logs/landing-preview-logs.tsx index 06d0d287411..9b35012f1c4 100644 --- a/apps/sim/app/(landing)/components/landing-preview/components/landing-preview-logs/landing-preview-logs.tsx +++ b/apps/sim/app/(landing)/components/landing-preview/components/landing-preview-logs/landing-preview-logs.tsx @@ -236,10 +236,7 @@ export function LandingPreviewLogs() { {COL_HEADERS.map(({ key, label }) => ( - + ) diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/column-header-menu.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/column-header-menu.tsx index 8d775e8b8ac..e2834ab3c7d 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/column-header-menu.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/column-header-menu.tsx @@ -162,8 +162,9 @@ export const ColumnHeaderMenu = React.memo(function ColumnHeaderMenu({ const ghost = document.createElement('div') ghost.textContent = ghostLabel + ghost.className = 'text-small' ghost.style.cssText = - 'position:absolute;top:-9999px;padding:4px 8px;background:var(--bg);border:1px solid var(--border);border-radius:4px;font-size:13px;font-weight:500;white-space:nowrap;color:var(--text-primary)' + 'position:absolute;top:-9999px;padding:4px 8px;background:var(--bg);border:1px solid var(--border);border-radius:4px;white-space:nowrap;color:var(--text-primary)' document.body.appendChild(ghost) e.dataTransfer.setDragImage(ghost, ghost.offsetWidth / 2, ghost.offsetHeight / 2) requestAnimationFrame(() => ghost.parentNode?.removeChild(ghost)) @@ -284,7 +285,7 @@ export const ColumnHeaderMenu = React.memo(function ColumnHeaderMenu({ if (e.key === 'Escape') onRenameCancel() }} onBlur={onRenameSubmit} - className='ml-1.5 min-w-0 flex-1 border-0 bg-transparent p-0 font-medium text-[var(--text-primary)] text-small outline-none focus:outline-none focus:ring-0' + className='ml-1.5 min-w-0 flex-1 border-0 bg-transparent p-0 text-[var(--text-primary)] text-small outline-none focus:outline-none focus:ring-0' /> ) : readOnly ? ( @@ -295,7 +296,7 @@ export const ColumnHeaderMenu = React.memo(function ColumnHeaderMenu({ blockIconInfo={sourceInfo?.blockIconInfo} blockMissing={blockMissing} /> - + {column.workflowGroupId ? column.headerLabel : column.name} @@ -313,7 +314,7 @@ export const ColumnHeaderMenu = React.memo(function ColumnHeaderMenu({ blockIconInfo={sourceInfo?.blockIconInfo} blockMissing={blockMissing} /> - + {column.workflowGroupId ? column.headerLabel : column.name} diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/workflow-group-meta-cell.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/workflow-group-meta-cell.tsx index 031a9c8a6f2..bf09f73c3eb 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/workflow-group-meta-cell.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/workflow-group-meta-cell.tsx @@ -368,8 +368,9 @@ export function WorkflowGroupMetaCell({ const ghost = document.createElement('div') ghost.textContent = name + ghost.className = 'text-xs' ghost.style.cssText = - 'position:absolute;top:-9999px;padding:4px 8px;background:var(--bg);border:1px solid var(--border);border-radius:4px;font-size:13px;font-weight:500;white-space:nowrap;color:var(--text-primary)' + 'position:absolute;top:-9999px;padding:4px 8px;background:var(--bg);border:1px solid var(--border);border-radius:4px;white-space:nowrap;color:var(--text-primary)' document.body.appendChild(ghost) e.dataTransfer.setDragImage(ghost, ghost.offsetWidth / 2, ghost.offsetHeight / 2) requestAnimationFrame(() => ghost.parentNode?.removeChild(ghost)) @@ -438,9 +439,7 @@ export function WorkflowGroupMetaCell({ ) : ( )} - - {name} - + {name} {onRunColumn && ( diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx index bcca45899c4..c37bf812c6e 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx @@ -1687,11 +1687,10 @@ export function TableGrid({ host.appendChild(measure) try { - measure.className = 'font-medium text-small' + measure.className = 'text-small' measure.textContent = column.headerLabel maxWidth = Math.max(maxWidth, measure.getBoundingClientRect().width + 57) - measure.className = 'text-small' for (const row of currentRows) { const val = row.data[column.key] if (val == null) continue diff --git a/packages/emcn/src/AGENTS.md b/packages/emcn/src/AGENTS.md index 8f8497c5913..c7bfda44b81 100644 --- a/packages/emcn/src/AGENTS.md +++ b/packages/emcn/src/AGENTS.md @@ -6,6 +6,6 @@ These rules apply to `packages/emcn/**`. - Use Radix UI primitives for accessibility where applicable. - Use CVA when a component has 2+ variants; use direct `className` composition for single-style components. - Export both the component and its variants helper when using CVA. -- Keep tokens consistent with the chip-pill canonical look: normal font-weight, `--text-body` value text, `--text-icon` icons at `size-[14px]`, `rounded-lg`. Components own their exact tokens (e.g. `Button` uses `rounded-[5px]`+`font-medium`). See `.claude/rules/emcn-components.md` for the full chip-chrome reference. +- Keep tokens consistent with the chip-pill canonical look: normal font-weight, `--text-body` value text, `--text-icon` icons at `size-[14px]`, `rounded-lg`. Components own their exact geometry tokens (e.g. `Button` uses `rounded-[5px]`), but never their own font-weight — every primitive inherits the document 400, and a weight class is reached for only to step deliberately up. See `.claude/rules/emcn-components.md` for the full chip-chrome reference. - Prefer `transition-colors` for interactive hover and active states. - Use TSDoc when documenting public components or APIs. diff --git a/packages/emcn/src/components/avatar/avatar.tsx b/packages/emcn/src/components/avatar/avatar.tsx index a511b7cb089..85ebaa522c8 100644 --- a/packages/emcn/src/components/avatar/avatar.tsx +++ b/packages/emcn/src/components/avatar/avatar.tsx @@ -133,6 +133,11 @@ AvatarImage.displayName = 'AvatarImage' /** * Fallback component for Avatar. Displays initials or icon when image is unavailable. + * + * Carries the package's only hardcoded `font-medium`, and deliberately: one or + * two capitals at `text-xs` on a filled disc are a glyph, not running text, and + * need the extra mass to read at avatar sizes. This is the sanctioned "step up + * from body" — every other primitive inherits the document 400. */ const AvatarFallback = React.forwardRef< React.ElementRef, diff --git a/packages/emcn/src/components/badge/badge.tsx b/packages/emcn/src/components/badge/badge.tsx index 7f7cea2c7d4..a7dffc6821b 100644 --- a/packages/emcn/src/components/badge/badge.tsx +++ b/packages/emcn/src/components/badge/badge.tsx @@ -5,41 +5,38 @@ import { cn } from '../../lib/cn' /** Shared base styles for status color badge variants */ const STATUS_BASE = 'gap-1.5 rounded-md' -const badgeVariants = cva( - 'inline-flex items-center font-medium focus:outline-none transition-colors', - { - variants: { - variant: { - default: - 'gap-1 rounded-[40px] border border-[var(--border)] text-[var(--text-secondary)] bg-[var(--surface-4)] hover-hover:text-[var(--text-primary)] hover-hover:border-[var(--border-1)] hover-hover:bg-[var(--surface-6)] dark:hover-hover:bg-[var(--surface-5)]', - outline: - 'gap-1 rounded-[40px] border border-[var(--border-1)] bg-transparent text-[var(--text-secondary)] hover-hover:text-[var(--text-primary)] hover-hover:bg-[var(--surface-5)] dark:hover-hover:bg-transparent dark:hover-hover:border-[var(--surface-6)]', - type: 'gap-1 rounded-[40px] border border-[var(--border)] text-[var(--text-secondary)] bg-[var(--surface-4)] dark:bg-[var(--surface-6)]', - green: `${STATUS_BASE} bg-[var(--badge-success-bg)] text-[var(--badge-success-text)]`, - red: `${STATUS_BASE} bg-[var(--badge-error-bg)] text-[var(--badge-error-text)]`, - gray: `${STATUS_BASE} bg-[var(--badge-gray-bg)] text-[var(--badge-gray-text)]`, - blue: `${STATUS_BASE} bg-[var(--badge-blue-bg)] text-[var(--badge-blue-text)]`, - 'blue-secondary': `${STATUS_BASE} bg-[var(--badge-blue-secondary-bg)] text-[var(--badge-blue-secondary-text)]`, - purple: `${STATUS_BASE} bg-[var(--badge-purple-bg)] text-[var(--badge-purple-text)]`, - orange: `${STATUS_BASE} bg-[var(--badge-orange-bg)] text-[var(--badge-orange-text)]`, - amber: `${STATUS_BASE} bg-[var(--badge-amber-bg)] text-[var(--badge-amber-text)]`, - teal: `${STATUS_BASE} bg-[var(--badge-teal-bg)] text-[var(--badge-teal-text)]`, - cyan: `${STATUS_BASE} bg-[var(--badge-cyan-bg)] text-[var(--badge-cyan-text)]`, - pink: `${STATUS_BASE} bg-[var(--badge-pink-bg)] text-[var(--badge-pink-text)]`, - 'gray-secondary': `${STATUS_BASE} bg-[var(--surface-4)] text-[var(--text-secondary)]`, - }, - size: { - sm: 'px-[7px] py-[1px] text-xs', - md: 'px-[9px] py-0.5 text-caption', - lg: 'px-[9px] py-[2.25px] text-caption', - }, +const badgeVariants = cva('inline-flex items-center focus:outline-none transition-colors', { + variants: { + variant: { + default: + 'gap-1 rounded-[40px] border border-[var(--border)] text-[var(--text-secondary)] bg-[var(--surface-4)] hover-hover:text-[var(--text-primary)] hover-hover:border-[var(--border-1)] hover-hover:bg-[var(--surface-6)] dark:hover-hover:bg-[var(--surface-5)]', + outline: + 'gap-1 rounded-[40px] border border-[var(--border-1)] bg-transparent text-[var(--text-secondary)] hover-hover:text-[var(--text-primary)] hover-hover:bg-[var(--surface-5)] dark:hover-hover:bg-transparent dark:hover-hover:border-[var(--surface-6)]', + type: 'gap-1 rounded-[40px] border border-[var(--border)] text-[var(--text-secondary)] bg-[var(--surface-4)] dark:bg-[var(--surface-6)]', + green: `${STATUS_BASE} bg-[var(--badge-success-bg)] text-[var(--badge-success-text)]`, + red: `${STATUS_BASE} bg-[var(--badge-error-bg)] text-[var(--badge-error-text)]`, + gray: `${STATUS_BASE} bg-[var(--badge-gray-bg)] text-[var(--badge-gray-text)]`, + blue: `${STATUS_BASE} bg-[var(--badge-blue-bg)] text-[var(--badge-blue-text)]`, + 'blue-secondary': `${STATUS_BASE} bg-[var(--badge-blue-secondary-bg)] text-[var(--badge-blue-secondary-text)]`, + purple: `${STATUS_BASE} bg-[var(--badge-purple-bg)] text-[var(--badge-purple-text)]`, + orange: `${STATUS_BASE} bg-[var(--badge-orange-bg)] text-[var(--badge-orange-text)]`, + amber: `${STATUS_BASE} bg-[var(--badge-amber-bg)] text-[var(--badge-amber-text)]`, + teal: `${STATUS_BASE} bg-[var(--badge-teal-bg)] text-[var(--badge-teal-text)]`, + cyan: `${STATUS_BASE} bg-[var(--badge-cyan-bg)] text-[var(--badge-cyan-text)]`, + pink: `${STATUS_BASE} bg-[var(--badge-pink-bg)] text-[var(--badge-pink-text)]`, + 'gray-secondary': `${STATUS_BASE} bg-[var(--surface-4)] text-[var(--text-secondary)]`, }, - defaultVariants: { - variant: 'default', - size: 'md', + size: { + sm: 'px-[7px] py-[1px] text-xs', + md: 'px-[9px] py-0.5 text-caption', + lg: 'px-[9px] py-[2.25px] text-caption', }, - } -) + }, + defaultVariants: { + variant: 'default', + size: 'md', + }, +}) /** Color variants that support dot indicators */ const STATUS_VARIANTS = [ diff --git a/packages/emcn/src/components/button-group/button-group.tsx b/packages/emcn/src/components/button-group/button-group.tsx index ca7056669d6..d1ead7aaef9 100644 --- a/packages/emcn/src/components/button-group/button-group.tsx +++ b/packages/emcn/src/components/button-group/button-group.tsx @@ -100,7 +100,7 @@ function ButtonGroup({ } const buttonGroupItemVariants = cva( - 'inline-flex items-center justify-center font-medium transition-colors outline-none focus:outline-none focus-visible:outline-none disabled:pointer-events-none disabled:opacity-70 px-2 py-1 text-caption border', + 'inline-flex items-center justify-center transition-colors outline-none focus:outline-none focus-visible:outline-none disabled:pointer-events-none disabled:opacity-70 px-2 py-1 text-caption border', { variants: { active: { diff --git a/packages/emcn/src/components/button/button.tsx b/packages/emcn/src/components/button/button.tsx index e1916bda1e4..1893a682403 100644 --- a/packages/emcn/src/components/button/button.tsx +++ b/packages/emcn/src/components/button/button.tsx @@ -21,7 +21,7 @@ import { cn } from '../../lib/cn' * @example */ const buttonVariants = cva( - 'inline-flex items-center justify-center font-medium transition-colors disabled:pointer-events-none disabled:opacity-70 outline-none focus:outline-none focus-visible:outline-none rounded-[5px]', + 'inline-flex items-center justify-center transition-colors disabled:pointer-events-none disabled:opacity-70 outline-none focus:outline-none focus-visible:outline-none rounded-[5px]', { variants: { variant: { diff --git a/packages/emcn/src/components/chip-combobox/chip-combobox.tsx b/packages/emcn/src/components/chip-combobox/chip-combobox.tsx index b766225e8ed..9650a9892f7 100644 --- a/packages/emcn/src/components/chip-combobox/chip-combobox.tsx +++ b/packages/emcn/src/components/chip-combobox/chip-combobox.tsx @@ -11,8 +11,9 @@ import { Combobox, type ComboboxProps } from '../combobox/combobox' * Reuses 100% of `Combobox` — search, editable entry, multi-select, groups, * async loading, per-option icons, and `overlayContent` all work unchanged. * Only the trigger chrome is overridden (the `className` merges last in - * `Combobox`, so `rounded-lg` / height / dark surface and the chip typography - * — normal weight, `--text-body` — win over the heavier combobox defaults). + * `Combobox`, so `rounded-lg` / height / dark surface and the chip `--text-body` + * color win over the combobox defaults). Weight is no longer overridden here — + * `Combobox` inherits the document's 400, which is already the chip weight. * The muted placeholder still applies because the combobox tints the inner * label span with `--text-muted` independently of the trigger className. * @@ -27,7 +28,7 @@ export function ChipCombobox({ className, ...props }: ComboboxProps) { diff --git a/packages/emcn/src/components/chip-modal/chip-modal.tsx b/packages/emcn/src/components/chip-modal/chip-modal.tsx index 85418066f65..4dbb0ab3e10 100644 --- a/packages/emcn/src/components/chip-modal/chip-modal.tsx +++ b/packages/emcn/src/components/chip-modal/chip-modal.tsx @@ -623,10 +623,7 @@ function ChipModalField(props: ChipModalFieldProps) { return (
-
diff --git a/packages/emcn/src/components/combobox/combobox.tsx b/packages/emcn/src/components/combobox/combobox.tsx index 16c361c5bf3..7cf58e0187c 100644 --- a/packages/emcn/src/components/combobox/combobox.tsx +++ b/packages/emcn/src/components/combobox/combobox.tsx @@ -21,7 +21,7 @@ import { Input } from '../input/input' import { Popover, PopoverAnchor, PopoverContent, PopoverScrollArea } from '../popover/popover' const comboboxVariants = cva( - 'flex w-full rounded-sm border border-[var(--border-1)] bg-[var(--surface-5)] px-2 font-sans font-medium text-[var(--text-primary)] placeholder:text-[var(--text-muted)] outline-none disabled:cursor-not-allowed disabled:opacity-50', + 'flex w-full rounded-sm border border-[var(--border-1)] bg-[var(--surface-5)] px-2 font-sans text-[var(--text-primary)] placeholder:text-[var(--text-muted)] outline-none disabled:cursor-not-allowed disabled:opacity-50', { variants: { variant: { @@ -572,7 +572,7 @@ const Combobox = memo( @@ -797,7 +797,7 @@ const Combobox = memo( !option.disabled && setHighlightedIndex(globalIndex) } className={cn( - 'relative flex cursor-pointer select-none items-center gap-2 rounded-sm px-1.5 font-medium font-sans', + 'relative flex cursor-pointer select-none items-center gap-2 rounded-sm px-1.5 font-sans', size === 'sm' ? 'py-[5px] text-caption' : 'py-1.5 text-sm', 'hover-hover:bg-[var(--surface-active)]', (isHighlighted || isSelected) && 'bg-[var(--surface-active)]', @@ -837,7 +837,7 @@ const Combobox = memo( }} onMouseEnter={() => setHighlightedIndex(-1)} className={cn( - 'relative flex cursor-pointer select-none items-center rounded-sm px-1.5 font-medium font-sans', + 'relative flex cursor-pointer select-none items-center rounded-sm px-1.5 font-sans', size === 'sm' ? 'py-[5px] text-caption' : 'py-1.5 text-sm', 'hover-hover:bg-[var(--surface-active)]', !multiSelectValues?.length && 'bg-[var(--surface-active)]' @@ -871,7 +871,7 @@ const Combobox = memo( }} onMouseEnter={() => !option.disabled && setHighlightedIndex(index)} className={cn( - 'relative flex cursor-pointer select-none items-center gap-2 rounded-sm px-1.5 font-medium font-sans', + 'relative flex cursor-pointer select-none items-center gap-2 rounded-sm px-1.5 font-sans', size === 'sm' ? 'py-[5px] text-caption' : 'py-1.5 text-sm', 'hover-hover:bg-[var(--surface-active)]', (isHighlighted || isSelected) && 'bg-[var(--surface-active)]', diff --git a/packages/emcn/src/components/dropdown-menu/dropdown-menu.tsx b/packages/emcn/src/components/dropdown-menu/dropdown-menu.tsx index 220c61fb684..c7af01b215b 100644 --- a/packages/emcn/src/components/dropdown-menu/dropdown-menu.tsx +++ b/packages/emcn/src/components/dropdown-menu/dropdown-menu.tsx @@ -298,11 +298,7 @@ const DropdownMenuLabel = React.forwardRef< >(({ className, inset, ...props }, ref) => ( )) diff --git a/packages/emcn/src/components/input-otp/input-otp.tsx b/packages/emcn/src/components/input-otp/input-otp.tsx index af5642e0747..a6c5ea82fff 100644 --- a/packages/emcn/src/components/input-otp/input-otp.tsx +++ b/packages/emcn/src/components/input-otp/input-otp.tsx @@ -77,7 +77,7 @@ const InputOTPSlot = React.forwardRef<
diff --git a/packages/emcn/src/components/label/label.tsx b/packages/emcn/src/components/label/label.tsx index a4623bca65a..412f12e8dc7 100644 --- a/packages/emcn/src/components/label/label.tsx +++ b/packages/emcn/src/components/label/label.tsx @@ -25,7 +25,7 @@ function Label({ className, ...props }: LabelProps) { return ( - + {children} @@ -825,7 +825,7 @@ const ModalTabsTrigger = React.forwardRef< ( return (
)} {folderTitle && !onFolderSelect && ( -
+
{folderTitle}
)} @@ -1173,7 +1167,7 @@ const PopoverSearch = React.forwardRef( (function Prog
- - {title} - + {title} {meta != null && ( {meta} )} diff --git a/packages/emcn/src/components/tab-strip/tab-strip.tsx b/packages/emcn/src/components/tab-strip/tab-strip.tsx index d1fb18c7d6e..f6f6e4e90e5 100644 --- a/packages/emcn/src/components/tab-strip/tab-strip.tsx +++ b/packages/emcn/src/components/tab-strip/tab-strip.tsx @@ -180,7 +180,7 @@ function Tab({ aria-current={tab.active ? 'page' : undefined} aria-label={tab.pinned ? tab.title : undefined} className={cn( - 'h-[30px] w-full select-none rounded-b-none border border-transparent border-b-0 bg-transparent py-0 font-normal text-caption', + 'h-[30px] w-full select-none rounded-b-none border border-transparent border-b-0 bg-transparent py-0 text-caption', tab.pinned ? 'justify-center px-0' : 'justify-start gap-1.5 px-2', closeable && !tab.pinned && 'pr-7', tab.active && diff --git a/packages/emcn/src/components/table/table.tsx b/packages/emcn/src/components/table/table.tsx index 498c0067c4f..0ba67ba490f 100644 --- a/packages/emcn/src/components/table/table.tsx +++ b/packages/emcn/src/components/table/table.tsx @@ -54,7 +54,7 @@ const TableFooter = React.forwardRef< tr]:last:border-b-0', + 'border-t bg-[color-mix(in_srgb,var(--surface-3)_50%,transparent)] [&>tr]:last:border-b-0', className )} {...props} @@ -80,7 +80,7 @@ const TableHead = React.forwardRef< [role=checkbox]]:translate-y-[2px]', + 'h-10 px-3 py-2 text-left align-middle text-[var(--text-secondary)] [&:has([role=checkbox])]:pr-0 [&>[role=checkbox]]:translate-y-[2px]', className )} {...props} diff --git a/packages/emcn/src/components/tag-input/tag-input.tsx b/packages/emcn/src/components/tag-input/tag-input.tsx index 6cee52f3be3..3e5a99eddc9 100644 --- a/packages/emcn/src/components/tag-input/tag-input.tsx +++ b/packages/emcn/src/components/tag-input/tag-input.tsx @@ -180,7 +180,7 @@ const TagInputTag = React.memo(function TagInputTag({ onRightIconClick={disabled ? undefined : handleRemove} rightIconLabel={`Remove ${item.value}`} > - + {item.value} {showError && {item.error}} @@ -423,7 +423,7 @@ const TagInput = React.forwardRef(
{inputValue.trim() && (