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/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/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/code/code.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/code/code.tsx index 8cfef78abef..163f80f9abc 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/code/code.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/code/code.tsx @@ -67,9 +67,32 @@ IMPORTANT FORMATTING RULES: 1. Reference Environment Variables: Use the exact syntax {{VARIABLE_NAME}}. Do NOT wrap it in quotes. 2. Reference Input Parameters/Workflow Variables: Use the exact syntax . Do NOT wrap it in quotes. 3. Function Body ONLY: Do NOT include the function signature (e.g., 'def my_func(...)') or surrounding braces. Return the final value with 'return'. -4. Imports: You may add imports as needed (standard library or pip-installed packages) without comments. +4. Imports: The Python standard library is always available. Third-party packages are available ONLY when the block has a sandbox selected — the sandbox's package list is appended below when one is. Never import a package that is not on that list. 5. No Markdown: Do NOT include backticks, code fences, or any markdown. -6. Clarity: Write clean, readable Python code.` +6. Clarity: Write clean, readable Python code. +7. No Explanations: Output the raw Python code only — no prose before or after it. + +Example Scenario: +User Prompt: "Fetch user data from an API. Use the User ID passed in as 'userId' and an API Key stored as the 'SERVICE_API_KEY' environment variable." + +Generated Code: +import json +import urllib.error +import urllib.request + +user_id = # Correct: accessing an input parameter without quotes +api_key = {{SERVICE_API_KEY}} # Correct: accessing an environment variable without quotes +url = f"https://api.example.com/users/{user_id}" + +request = urllib.request.Request(url, headers={"Authorization": f"Bearer {api_key}"}) + +try: + with urllib.request.urlopen(request) as response: + # Return the fetched data, which becomes the block's output + return json.loads(response.read().decode()) +except urllib.error.HTTPError as error: + # Raising marks the block execution as failed + raise Exception(f"API request failed with status {error.code}: {error.read().decode()}")` /** * Line height constant for consistent rendering. @@ -330,6 +353,9 @@ export const Code = memo(function Code({ tableId: typeof tableIdValue === 'string' ? tableIdValue : null, sandboxId: typeof sandboxIdValue === 'string' ? sandboxIdValue : null, }, + // Keyed off the same value that swaps the prompt below, so history from the + // previous language cannot steer the next generation back to it. + historyResetKey: typeof languageValue === 'string' ? languageValue : undefined, onStreamStart: () => handleStreamStartRef.current?.(), onStreamChunk: (chunk: string) => handleStreamChunkRef.current?.(chunk), onGeneratedContent: (content: string) => handleGeneratedContentRef.current?.(content), diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-wand.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-wand.ts index 34c0b712ee2..058e714757e 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-wand.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-wand.ts @@ -1,4 +1,4 @@ -import { useCallback, useRef, useState } from 'react' +import { useCallback, useLayoutEffect, useRef, useState } from 'react' import { toast } from '@sim/emcn' import { createLogger } from '@sim/logger' import { filterUndefined } from '@sim/utils/object' @@ -8,6 +8,7 @@ import { requestRaw } from '@/lib/api/client' import { isApiClientError } from '@/lib/api/client/errors' import { wandGenerateStreamContract } from '@/lib/api/contracts' import { readSSEStream } from '@/lib/core/utils/sse' +import { shouldStripCodeFences, stripCodeFences } from '@/lib/wand/strip-code-fences' import type { GenerationType } from '@/blocks/types' import { subscriptionKeys } from '@/hooks/queries/subscription' import { useSettingsNavigation } from '@/hooks/use-settings-navigation' @@ -100,20 +101,26 @@ interface UseWandProps { wandConfig?: WandConfig currentValue?: string contextParams?: WandContextParams + /** + * Clears the conversation history whenever this value changes. Pass anything + * that invalidates prior turns — a Function block switching language rewrites + * `wandConfig.prompt`, but replayed history would keep steering the model back + * to the previous language. + */ + historyResetKey?: string onGeneratedContent: (content: string) => void onStreamChunk?: (chunk: string) => void onStreamStart?: () => void - onGenerationComplete?: (prompt: string, generatedContent: string) => void } export function useWand({ wandConfig, currentValue, contextParams, + historyResetKey, onGeneratedContent, onStreamChunk, onStreamStart, - onGenerationComplete, }: UseWandProps) { const queryClient = useQueryClient() const { navigateToSettings } = useSettingsNavigation() @@ -127,6 +134,35 @@ export function useWand({ const [conversationHistory, setConversationHistory] = useState([]) + /** + * Adjusted during render rather than in an effect so a generation started in + * the same commit as the change can never send the stale history. History is + * already empty on mount, so seeding the tracker with the current key + * correctly makes the first render a no-op. + */ + const [prevHistoryResetKey, setPrevHistoryResetKey] = useState(historyResetKey) + const [historyEpoch, setHistoryEpoch] = useState(0) + if (prevHistoryResetKey !== historyResetKey) { + setPrevHistoryResetKey(historyResetKey) + setConversationHistory([]) + setHistoryEpoch((epoch) => epoch + 1) + } + + /** + * Mirrors {@link historyEpoch} for the in-flight request to read on completion. + * A request that started before a reset must not append its turn to the fresh + * history — its prompt and reply belong to the superseded context. + * + * Synced in a layout effect, not a passive one: passive effects flush in a later + * task, so a request settling between the reset's commit and that flush would + * still read the old epoch and append anyway. Layout effects run synchronously + * during commit, before any promise continuation can observe the ref. + */ + const historyEpochRef = useRef(historyEpoch) + useLayoutEffect(() => { + historyEpochRef.current = historyEpoch + }, [historyEpoch]) + const abortControllerRef = useRef(null) const showPromptInline = useCallback(() => { @@ -171,6 +207,9 @@ export function useWand({ setError(null) setPromptInputValue('') + /** The context this request belongs to; a reset while it streams retires it. */ + const startedHistoryEpoch = historyEpochRef.current + abortControllerRef.current = new AbortController() if (onStreamStart) { @@ -224,25 +263,37 @@ export function useWand({ signal: abortControllerRef.current?.signal, }) - if (accumulatedContent) { - onGeneratedContent(accumulatedContent) - - if (wandConfig?.maintainHistory) { + /** + * Sanitized once the full response is known, then written back over the + * streamed text. Doing it per-chunk would mean guessing whether a + * trailing backtick run opens a fence or is part of the code, so the + * editor may briefly show a fence that the final value does not. + */ + const generatedContent = shouldStripCodeFences(wandConfig?.generationType) + ? stripCodeFences(accumulatedContent) + : accumulatedContent + + if (generatedContent) { + onGeneratedContent(generatedContent) + + /** + * The sanitized form goes into history so a single fenced reply cannot + * become the in-context example for every later turn. Skipped entirely + * when a reset retired this request's context mid-flight. + */ + if (wandConfig?.maintainHistory && historyEpochRef.current === startedHistoryEpoch) { setConversationHistory((prev) => [ ...prev, { role: 'user', content: currentPrompt }, - { role: 'assistant', content: accumulatedContent }, + { role: 'assistant', content: generatedContent }, ]) } - - if (onGenerationComplete) { - onGenerationComplete(currentPrompt, accumulatedContent) - } } logger.debug('Wand generation completed', { prompt, - contentLength: accumulatedContent.length, + contentLength: generatedContent.length, + strippedFences: generatedContent !== accumulatedContent, }) setTimeout(() => { @@ -282,7 +333,6 @@ export function useWand({ onGeneratedContent, onStreamChunk, onStreamStart, - onGenerationComplete, queryClient, contextParams?.tableId, contextParams?.sandboxId, 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} 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..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 @@ -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,94 @@ 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 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 ( +

+
+ {/* 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 ? ( + { + const owner = pathOwners.get(path) + const base = + mapping.adoptablePaths.length === 1 + ? 'Keep existing URL' + : `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} + 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 +682,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 +805,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 +847,20 @@ export function ForkSyncView({ controller, onDirectionChange }: ForkSyncViewProp ) : null} + {controller.triggerMappings.length > 0 ? ( + +
+ {controller.triggerMappings.map((mapping) => ( + + ))} +
+
+ ) : null} + {controller.hasVisibleCopyables ? (
@@ -743,18 +882,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 +934,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/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 15f70c53e4d..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 @@ -9,6 +9,8 @@ import type { ForkDependentReconfig, ForkMappingEntry, ForkResourceUsage, + ForkTriggerMapping, + ForkTriggerUrlChange, ForkWorkflowChange, } from '@/lib/api/contracts/workspace-fork' import { @@ -33,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, @@ -40,6 +47,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 +100,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 +154,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 +185,26 @@ export interface ForkSyncController { workflowChanges: ForkWorkflowChange[] /** Names of target workflows this sync archives, for the confirm modal. */ archivedWorkflowNames: string[] + /** + * 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[] + /** + * 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 + /** 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. */ @@ -239,6 +292,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 +311,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 +331,14 @@ 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] + ) + 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 @@ -298,6 +371,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 +531,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 +577,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 +605,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 +760,69 @@ 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 })) + } + + /** 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({}) + setTriggerAdoptions({}) } const sync = async () => { @@ -685,6 +840,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 +892,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 +931,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 +964,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 +989,12 @@ export function useForkSync(params: { copyingKeys, copySelected, toggleCopyKeys, + droppedRefs, + toggleDroppedRef, + dropAllDeletedRefs, + droppableBlockerCount: droppableBlockerKeys.length, + blockingUsesByResource, + firstBlockingRowForResource, referencedByKind, unreferencedByKind, hasVisibleCopyables: visibleCopyables.length > 0, @@ -800,6 +1002,12 @@ export function useForkSync(params: { dependentClears, workflowChanges, archivedWorkflowNames, + 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/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/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/lib/api/contracts/workspace-fork.test.ts b/apps/sim/lib/api/contracts/workspace-fork.test.ts index 9a72c30fc0f..b8defe69939 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.retiringTriggerUrls).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', + }, + ], + 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.retiringTriggerUrls[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..c861a0995d6 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,18 @@ export const getForkDiffContract = defineRouteContract({ * always clear (informational). */ clearedRefs: z.array(forkClearedRefSchema), + /** + * 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. + */ + retiringTriggerUrls: z.array(forkTriggerUrlChangeSchema).default([]), + /** Arriving trigger blocks whose URL this sync decides, with their adoptable alternatives. */ + triggerMappings: z.array(forkTriggerMappingSchema).default([]), }), }, }) @@ -598,6 +665,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 +712,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 +785,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/copilot/request/handlers/handlers.test.ts b/apps/sim/lib/copilot/request/handlers/handlers.test.ts index c3f92fe3c06..7c69cbf96c4 100644 --- a/apps/sim/lib/copilot/request/handlers/handlers.test.ts +++ b/apps/sim/lib/copilot/request/handlers/handlers.test.ts @@ -408,6 +408,7 @@ describe('sse-handlers tool lifecycle', () => { const updated = context.toolCalls.get('tool-1') expect(updated?.status).toBe(MothershipStreamV1ToolOutcome.success) + expect(updated?.agentId).toBe('main') // Display titles are derived client-side from the tool name (+args), not the // stream; read with no path resolves to the static "Reading file". expect(updated?.displayTitle).toBe('Reading file') @@ -889,9 +890,11 @@ describe('sse-handlers tool lifecycle', () => { expect.any(Object) ) expect(context.toolCalls.get('sub-tool-1')?.params).toEqual({ name: 'Example Workflow' }) + expect(context.toolCalls.get('sub-tool-1')?.agentId).toBe('workflow') expect(context.subAgentToolCalls['parent-1']?.[0]?.params).toEqual({ name: 'Example Workflow', }) + expect(context.subAgentToolCalls['parent-1']?.[0]?.agentId).toBe('workflow') }) it('routes subagent text using the event scope parent tool call id', async () => { @@ -973,6 +976,40 @@ describe('sse-handlers tool lifecycle', () => { await sleep(0) expect(context.subAgentToolCalls['parent-1']?.[0]?.id).toBe('sub-tool-scope-1') + expect(context.toolCalls.get('sub-tool-scope-1')?.agentId).toBe('deploy') + }) + + it('retains the first agent attribution on replayed partial tool calls', async () => { + context.toolCalls.set('replayed-read', { + id: 'replayed-read', + name: 'read', + status: 'executing', + }) + + const replayPartial = (agentId: string) => + subAgentHandlers.tool( + { + type: MothershipStreamV1EventType.tool, + scope: { lane: 'subagent', parentToolCallId: 'parent-1', agentId }, + payload: { + toolCallId: 'replayed-read', + toolName: 'read', + executor: MothershipStreamV1ToolExecutor.go, + mode: MothershipStreamV1ToolMode.sync, + phase: MothershipStreamV1ToolPhase.call, + status: 'generating', + partial: true, + }, + } satisfies StreamEvent, + context, + execContext, + { interactive: false, timeout: 1000 } + ) + + await replayPartial('workflow') + await replayPartial('deploy') + + expect(context.toolCalls.get('replayed-read')?.agentId).toBe('workflow') }) it('pairs compaction lifecycle events within each scoped subagent lane', async () => { diff --git a/apps/sim/lib/copilot/request/handlers/tool.ts b/apps/sim/lib/copilot/request/handlers/tool.ts index 6634a625308..aeabc123d71 100644 --- a/apps/sim/lib/copilot/request/handlers/tool.ts +++ b/apps/sim/lib/copilot/request/handlers/tool.ts @@ -285,6 +285,7 @@ export async function handleToolEvent( ): Promise { const isSubagent = scope === 'subagent' const parentToolCallId = isSubagent ? getScopedParentToolCallId(event, context) : undefined + const agentId = event.scope?.agentId ?? 'main' if (isSubagent && !parentToolCallId) return @@ -332,6 +333,7 @@ export async function handleToolEvent( options, parentToolCallId, scope, + agentId, getScopedSpanIdentity(event) ) } @@ -409,6 +411,7 @@ async function handleCallPhase( options: OrchestratorOptions, parentToolCallId: string | undefined, scope: ToolScope, + agentId: string, spanIdentity: { spanId?: string; parentSpanId?: string } ): Promise { const { toolCallId, toolName } = data @@ -416,6 +419,7 @@ async function handleCallPhase( const isGenerating = data.status === TOOL_CALL_STATUS.generating const isPartial = data.partial === true || isGenerating const existing = context.toolCalls.get(toolCallId) + if (existing) existing.agentId ??= agentId const isSubagent = scope === 'subagent' const ui = getToolCallUI(data) @@ -459,12 +463,13 @@ async function handleCallPhase( toolName, args, parentToolCallId!, + agentId, ui, spanIdentity, !isPartial ) } else { - registerMainToolCall(context, toolCallId, toolName, args, existing, ui, !isPartial) + registerMainToolCall(context, toolCallId, toolName, args, existing, agentId, ui, !isPartial) } if (isPartial) return @@ -554,6 +559,7 @@ function registerSubagentToolCall( toolName: string, args: Record | undefined, parentToolCallId: string, + agentId: string, ui: { title?: string; phaseLabel?: string; hidden?: boolean }, spanIdentity: { spanId?: string; parentSpanId?: string }, finalized: boolean @@ -574,6 +580,7 @@ function registerSubagentToolCall( id: toolCallId, name: toolName, status: 'pending', + agentId, params: args, startTime: Date.now(), } @@ -594,6 +601,7 @@ function registerSubagentToolCall( const subagentToolCalls = context.subAgentToolCalls[parentToolCallId] const existingSubagentToolCall = subagentToolCalls.find((tc) => tc.id === toolCallId) if (existingSubagentToolCall) { + existingSubagentToolCall.agentId ??= agentId if (!rebindResolvedIntegrationCall(existingSubagentToolCall, toolName, args)) { updateToolCallFromFrame(existingSubagentToolCall, toolName, args, finalized) } @@ -609,6 +617,7 @@ function registerMainToolCall( toolName: string, args: Record | undefined, existing: ToolCallState | undefined, + agentId: string, ui: { title?: string; phaseLabel?: string; hidden?: boolean }, finalized: boolean ): void { @@ -633,6 +642,7 @@ function registerMainToolCall( id: toolCallId, name: toolName, status: 'pending', + agentId, params: args, startTime: Date.now(), } diff --git a/apps/sim/lib/copilot/request/metrics.test.ts b/apps/sim/lib/copilot/request/metrics.test.ts new file mode 100644 index 00000000000..77e00075061 --- /dev/null +++ b/apps/sim/lib/copilot/request/metrics.test.ts @@ -0,0 +1,78 @@ +/** + * @vitest-environment node + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { toolCallsAdd, toolDurationRecord } = vi.hoisted(() => ({ + toolCallsAdd: vi.fn(), + toolDurationRecord: vi.fn(), +})) + +vi.mock('@opentelemetry/api', () => ({ + metrics: { + getMeter: vi.fn(() => ({ + createCounter: vi.fn(() => ({ add: toolCallsAdd })), + createHistogram: vi.fn((name: string) => ({ + record: name === 'copilot.tool.duration' ? toolDurationRecord : vi.fn(), + })), + })), + }, +})) + +import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1' +import { normalizeToolAgentId, recordSimToolMetric } from '@/lib/copilot/request/metrics' + +describe('recordSimToolMetric', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it.each(['main', 'workflow'])( + 'attributes call counts and duration to the registered %s agent', + (agentId) => { + recordSimToolMetric('read', agentId, 'success', 125) + + const baseAttributes = { + [TraceAttr.ToolName]: 'read', + [TraceAttr.ToolExecutor]: 'sim', + [TraceAttr.ToolOutcome]: 'success', + } + expect(toolCallsAdd).toHaveBeenCalledWith(1, { + ...baseAttributes, + [TraceAttr.GenAiAgentName]: agentId, + }) + expect(toolDurationRecord).toHaveBeenCalledWith(125, { + ...baseAttributes, + [TraceAttr.GenAiAgentName]: agentId, + }) + } + ) + + it('collapses unknown agent IDs to the bounded fallback', () => { + recordSimToolMetric('read', 'tenant-defined-agent', 'success', 125) + + const baseAttributes = { + [TraceAttr.ToolName]: 'read', + [TraceAttr.ToolExecutor]: 'sim', + [TraceAttr.ToolOutcome]: 'success', + } + expect(toolCallsAdd).toHaveBeenCalledWith(1, { + ...baseAttributes, + [TraceAttr.GenAiAgentName]: 'other', + }) + expect(toolDurationRecord).toHaveBeenCalledWith(125, { + ...baseAttributes, + [TraceAttr.GenAiAgentName]: 'other', + }) + }) + + it.each([ + { agentId: 'main', expected: 'main' }, + { agentId: 'workflow', expected: 'workflow' }, + { agentId: 'tenant-defined-agent', expected: 'other' }, + { agentId: '', expected: 'other' }, + ])('normalizes $agentId to $expected for every telemetry signal', ({ agentId, expected }) => { + expect(normalizeToolAgentId(agentId)).toBe(expected) + }) +}) diff --git a/apps/sim/lib/copilot/request/metrics.ts b/apps/sim/lib/copilot/request/metrics.ts index 31f0e7996f6..d3bfb804382 100644 --- a/apps/sim/lib/copilot/request/metrics.ts +++ b/apps/sim/lib/copilot/request/metrics.ts @@ -5,9 +5,9 @@ // contracts/metrics_v1.go) so the Go∪Sim union is queryable as one series set // — e.g. `copilot.tool.duration` split by `tool.executor` (go|client|sim). // -// Bounded cardinality only: tool.name is capped to the shared tool catalog -// (else "other"); vfs phase / file-read outcome are bounded sets. NEVER a -// user/chat/request id (those explode Prometheus series). +// Bounded cardinality only: tool.name and gen_ai.agent.name are capped to the +// shared catalogs (else "other"); vfs phase / file-read outcome are bounded +// sets. NEVER a user/chat/request id (those explode Prometheus series). import { type Counter, type Histogram, metrics } from '@opentelemetry/api' import { Metric } from '@/lib/copilot/generated/metrics-v1' import { TOOL_CATALOG } from '@/lib/copilot/generated/tool-catalog-v1' @@ -68,15 +68,30 @@ function cappedToolName(name: string): string { return TOOL_CATALOG[name] ? name : 'other' } +const REGISTERED_AGENT_IDS = new Set([ + 'main', + ...Object.values(TOOL_CATALOG).flatMap(({ subagentId }) => (subagentId ? [subagentId] : [])), +]) + +export function normalizeToolAgentId(agentId: string): string { + return REGISTERED_AGENT_IDS.has(agentId) ? agentId : 'other' +} + // recordSimToolMetric emits copilot.tool.calls (+1) and copilot.tool.duration // for one server-side Sim tool dispatch (executor=sim). outcome is the bounded // tool outcome (success/error/…). Pure telemetry. -export function recordSimToolMetric(name: string, outcome: string, durationMs: number): void { +export function recordSimToolMetric( + name: string, + agentId: string, + outcome: string, + durationMs: number +): void { const { toolDuration, toolCalls } = instruments() const attrs = { [TraceAttr.ToolName]: cappedToolName(name), [TraceAttr.ToolExecutor]: 'sim', [TraceAttr.ToolOutcome]: outcome, + [TraceAttr.GenAiAgentName]: normalizeToolAgentId(agentId), } toolCalls.add(1, attrs) if (durationMs >= 0) toolDuration.record(durationMs, attrs) diff --git a/apps/sim/lib/copilot/request/otel.ts b/apps/sim/lib/copilot/request/otel.ts index e93f9f53d5a..3804eb5ebe3 100644 --- a/apps/sim/lib/copilot/request/otel.ts +++ b/apps/sim/lib/copilot/request/otel.ts @@ -22,6 +22,7 @@ import { import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1' import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1' import { contextFromRequestHeaders } from '@/lib/copilot/request/go/propagation' +import { normalizeToolAgentId } from '@/lib/copilot/request/metrics' import { isExplicitStopReason } from '@/lib/copilot/request/session/abort-reason' // OTel GenAI content-capture env var (spec: @@ -284,6 +285,7 @@ export async function withCopilotToolSpan( input: { toolName: string toolCallId: string + agentName: string runId?: string chatId?: string argsBytes?: number @@ -299,6 +301,7 @@ export async function withCopilotToolSpan( [TraceAttr.ToolName]: input.toolName, [TraceAttr.ToolCallId]: input.toolCallId, [TraceAttr.ToolExecutor]: 'sim', + [TraceAttr.GenAiAgentName]: normalizeToolAgentId(input.agentName), ...(input.runId ? { [TraceAttr.RunId]: input.runId } : {}), ...(input.chatId ? { [TraceAttr.ChatId]: input.chatId } : {}), ...(typeof input.argsBytes === 'number' diff --git a/apps/sim/lib/copilot/request/tools/executor.test.ts b/apps/sim/lib/copilot/request/tools/executor.test.ts index 8d60e889a05..b2ca84938ee 100644 --- a/apps/sim/lib/copilot/request/tools/executor.test.ts +++ b/apps/sim/lib/copilot/request/tools/executor.test.ts @@ -1,13 +1,37 @@ import '@sim/testing/mocks/executor' -import { describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { recordSimToolMetric, setAttribute, withCopilotToolSpan } = vi.hoisted(() => { + const setAttribute = vi.fn() + return { + recordSimToolMetric: vi.fn(), + setAttribute, + withCopilotToolSpan: vi.fn( + (_input: unknown, fn: (span: { setAttribute: typeof setAttribute }) => Promise) => + fn({ setAttribute }) + ), + } +}) + +vi.mock('@/lib/copilot/request/metrics', () => ({ + recordSimToolMetric, +})) + +vi.mock('@/lib/copilot/request/otel', () => ({ + withCopilotToolSpan, +})) + import { TOOL_WATCHDOG_DEFAULT_MS, TOOL_WATCHDOG_LONG_RUNNING_MS } from '@/lib/copilot/constants' +import { MothershipStreamV1ToolOutcome } from '@/lib/copilot/generated/mothership-stream-v1' +import { createStreamingContext } from '@/lib/copilot/request/context/request-context' import { buildToolExecutionContext, + executeToolAndReport, pendingToolWaitBudgetMs, toolWatchdogTimeoutMs, } from '@/lib/copilot/request/tools/executor' -import type { ExecutionContext } from '@/lib/copilot/request/types' +import type { ExecutionContext, ToolCallState } from '@/lib/copilot/request/types' describe('toolWatchdogTimeoutMs', () => { it('gives request-scoped MCP tools the long-running watchdog', () => { @@ -58,3 +82,74 @@ describe('buildToolExecutionContext', () => { }) }) }) + +describe('executeToolAndReport metrics', () => { + const executionContext: ExecutionContext = { + userId: 'user-1', + workflowId: 'workflow-1', + } + + beforeEach(() => { + vi.clearAllMocks() + }) + + it('forwards the stored agent on normal completion', async () => { + const toolCall: ToolCallState = { + id: 'call-1', + name: 'read', + status: MothershipStreamV1ToolOutcome.success, + result: { success: true, output: 'done' }, + agentId: 'workflow', + endTime: Date.now(), + } + const context = createStreamingContext({ + toolCalls: new Map([[toolCall.id, toolCall]]), + }) + + await executeToolAndReport(toolCall.id, context, executionContext) + + expect(recordSimToolMetric).toHaveBeenCalledWith( + 'read', + 'workflow', + MothershipStreamV1ToolOutcome.success, + expect.any(Number) + ) + expect(withCopilotToolSpan).toHaveBeenCalledWith( + expect.objectContaining({ agentName: 'workflow' }), + expect.any(Function) + ) + }) + + it.each([ + { agentId: 'workflow', expectedAgentId: 'workflow' }, + { agentId: undefined, expectedAgentId: 'main' }, + ])( + 'forwards $expectedAgentId when an unexpected error occurs', + async ({ agentId, expectedAgentId }) => { + const toolCall: ToolCallState = { + id: 'call-2', + name: 'read', + status: MothershipStreamV1ToolOutcome.error, + agentId, + endTime: Date.now(), + } + const context = createStreamingContext({ + toolCalls: new Map([[toolCall.id, toolCall]]), + }) + + await expect(executeToolAndReport(toolCall.id, context, executionContext)).rejects.toThrow( + 'missing a canonical error' + ) + expect(recordSimToolMetric).toHaveBeenCalledWith( + 'read', + expectedAgentId, + MothershipStreamV1ToolOutcome.error, + expect.any(Number) + ) + expect(withCopilotToolSpan).toHaveBeenCalledWith( + expect.objectContaining({ agentName: expectedAgentId }), + expect.any(Function) + ) + } + ) +}) diff --git a/apps/sim/lib/copilot/request/tools/executor.ts b/apps/sim/lib/copilot/request/tools/executor.ts index 10f0f84402c..388f8356663 100644 --- a/apps/sim/lib/copilot/request/tools/executor.ts +++ b/apps/sim/lib/copilot/request/tools/executor.ts @@ -408,6 +408,7 @@ export async function executeToolAndReport( { toolName: toolCall.name, toolCallId: toolCall.id, + agentName: toolCall.agentId ?? 'main', runId: context.runId, chatId: execContext.chatId, argsBytes: argsPayload?.length, @@ -428,7 +429,12 @@ export async function executeToolAndReport( } // Durable Grafana signal for "which Sim tool is slowest" (executor=sim); // pairs with the Go executor-boundary metric (U15) as one series set. - recordSimToolMetric(toolCall.name, completion.status, durationMs) + recordSimToolMetric( + toolCall.name, + toolCall.agentId ?? 'main', + completion.status, + durationMs + ) return completion } catch (err) { // executeToolAndReportInner threw (infra/unexpected error, not a normal @@ -437,7 +443,12 @@ export async function executeToolAndReport( const durationMs = Date.now() - startedAt otelSpan.setAttribute(TraceAttr.ToolOutcome, 'error') otelSpan.setAttribute(TraceAttr.ToolDurationMs, durationMs) - recordSimToolMetric(toolCall.name, 'error', durationMs) + recordSimToolMetric( + toolCall.name, + toolCall.agentId ?? 'main', + MothershipStreamV1ToolOutcome.error, + durationMs + ) throw err } } diff --git a/apps/sim/lib/copilot/request/types.ts b/apps/sim/lib/copilot/request/types.ts index bf4908896db..ed76e5cd505 100644 --- a/apps/sim/lib/copilot/request/types.ts +++ b/apps/sim/lib/copilot/request/types.ts @@ -25,6 +25,8 @@ export interface ToolCallState { id: string name: string status: ToolCallStatus + /** Bounded registry ID of the agent that invoked this tool. */ + agentId?: string displayTitle?: string /** Model-authored activity text for a gateway-resolved integration call. */ integrationDescription?: string diff --git a/apps/sim/lib/wand/strip-code-fences.test.ts b/apps/sim/lib/wand/strip-code-fences.test.ts new file mode 100644 index 00000000000..0dbf8af603a --- /dev/null +++ b/apps/sim/lib/wand/strip-code-fences.test.ts @@ -0,0 +1,110 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { shouldStripCodeFences, stripCodeFences } from '@/lib/wand/strip-code-fences' + +describe('stripCodeFences', () => { + it('leaves unfenced code untouched', () => { + const code = 'const total = + ;\nreturn total;' + expect(stripCodeFences(code)).toBe(code) + }) + + it('unwraps a fully wrapped response', () => { + expect(stripCodeFences('```python\nresult = + \nreturn result\n```')).toBe( + 'result = + \nreturn result' + ) + }) + + it('unwraps a response with no closing fence', () => { + expect(stripCodeFences('```javascript\nconst x = 1;\nreturn x;')).toBe( + 'const x = 1;\nreturn x;' + ) + }) + + it('unwraps an untagged fence', () => { + expect(stripCodeFences('```\nreturn 1;\n```')).toBe('return 1;') + }) + + it('tolerates leading whitespace before the opening fence', () => { + expect(stripCodeFences('\n ```python\nreturn 1\n```')).toBe('return 1') + }) + + it('preserves indentation inside the fence', () => { + const fenced = '```python\nif :\n return "yes"\nreturn "no"\n```' + expect(stripCodeFences(fenced)).toBe('if :\n return "yes"\nreturn "no"') + }) + + it('preserves fence lines embedded inside the fenced body', () => { + const fenced = '```javascript\nconst md = `\n```\nhello\n```\n`;\nreturn md;\n```' + expect(stripCodeFences(fenced)).toBe('const md = `\n```\nhello\n```\n`;\nreturn md;') + }) + + it('keeps every line when a body with nested fences is truncated mid-response', () => { + const truncated = '```javascript\nconst md = `\n```\nhello\n`;\nreturn md;' + expect(stripCodeFences(truncated)).toBe('const md = `\n```\nhello\n`;\nreturn md;') + }) + + it('treats a trailing bare fence as the closer even when the body was truncated at one', () => { + // Irreducibly ambiguous: a trailing bare fence closes the wrapper in every + // well-formed response, and is content only when generation stopped exactly + // at an embedded delimiter. Declining to strip it would leave a stray fence + // in the common case, which is the bug this util exists to fix. + expect(stripCodeFences('```javascript\nconst md = `\n```')).toBe('const md = `') + }) + + it('preserves a fenced docstring inside a Python body', () => { + const fenced = '```python\ntemplate = """\n```sql\nSELECT 1\n```\n"""\nreturn template\n```' + expect(stripCodeFences(fenced)).toBe( + 'template = """\n```sql\nSELECT 1\n```\n"""\nreturn template' + ) + }) + + it('keeps everything between the outer delimiters for a multi-block answer', () => { + // Prose survives rather than risk dropping code between two delimiters that + // may be a nested literal instead of a block boundary. + const fenced = '```js\nconst a = 1;\n```\nThen send it:\n```js\nreturn a;\n```' + expect(stripCodeFences(fenced)).toBe('const a = 1;\n```\nThen send it:\n```js\nreturn a;') + }) + + it('does not touch code that merely contains a fence later', () => { + const code = 'const doc = `\n```json\n{"a":1}\n```\n`;\nreturn doc;' + expect(stripCodeFences(code)).toBe(code) + }) + + it('returns the original when stripping would leave nothing', () => { + const empty = '```python\n```' + expect(stripCodeFences(empty)).toBe(empty) + }) + + it('is idempotent', () => { + const once = stripCodeFences('```python\nreturn \n```') + expect(stripCodeFences(once)).toBe(once) + }) + + it('handles an empty string', () => { + expect(stripCodeFences('')).toBe('') + }) +}) + +describe('shouldStripCodeFences', () => { + it('strips for code and structured value types', () => { + expect(shouldStripCodeFences('javascript-function-body')).toBe(true) + expect(shouldStripCodeFences('custom-tool-schema')).toBe(true) + expect(shouldStripCodeFences('json-object')).toBe(true) + expect(shouldStripCodeFences('cron-expression')).toBe(true) + }) + + it('does not strip free-form prose', () => { + expect(shouldStripCodeFences('system-prompt')).toBe(false) + }) + + it('does not strip when no generation type is declared', () => { + expect(shouldStripCodeFences(undefined)).toBe(false) + expect(shouldStripCodeFences('')).toBe(false) + }) + + it('does not strip an unrecognized type', () => { + expect(shouldStripCodeFences('something-new')).toBe(false) + }) +}) diff --git a/apps/sim/lib/wand/strip-code-fences.ts b/apps/sim/lib/wand/strip-code-fences.ts new file mode 100644 index 00000000000..7a888d6bfff --- /dev/null +++ b/apps/sim/lib/wand/strip-code-fences.ts @@ -0,0 +1,101 @@ +import type { GenerationType } from '@/blocks/types' + +/** A markdown fence delimiter at the start of a line, ignoring indentation. */ +const FENCE_LINE = /^\s*```/ + +/** + * Whether a wand generation's output is a raw machine value, where a leading + * markdown fence is always wrong and must be removed. + * + * Declared as a total `Record` so adding a `GenerationType` fails the build + * until the new type opts in or out deliberately — a silent default would let a + * prose type start stripping fences (or a code type stop) without review. + * + * `system-prompt` is the sole exclusion: it is free-form prose for a model, so a + * fenced example inside it is legitimate authored content, not a formatting slip. + */ +const STRIPS_CODE_FENCES: Record = { + 'javascript-function-body': true, + 'typescript-function-body': true, + 'json-schema': true, + 'json-object': true, + 'table-schema': true, + 'system-prompt': false, + 'custom-tool-schema': true, + 'sql-query': true, + postgrest: true, + 'mongodb-filter': true, + 'mongodb-pipeline': true, + 'mongodb-sort': true, + 'mongodb-documents': true, + 'mongodb-update': true, + 'neo4j-cypher': true, + 'neo4j-parameters': true, + timestamp: true, + timezone: true, + 'cron-expression': true, + 'odata-expression': true, +} + +/** + * Whether generated content for this type should have markdown fences stripped. + * + * An absent type means the field's `wandConfig` never declared one, which is the + * case for free-form prose fields — those are left untouched. + */ +export function shouldStripCodeFences(generationType?: string): boolean { + if (!generationType) return false + return STRIPS_CODE_FENCES[generationType as GenerationType] === true +} + +/** + * Removes the markdown code fences a model wrapped around a raw value. + * + * Applies only when the response *opens* with a fence. Content that merely + * contains a fence later is left untouched, because a backtick run inside a + * template literal or a docstring is valid code that must survive verbatim — + * a false positive here would corrupt working code, which is far worse than + * leaving a rare unwrapped response for the user to fix. + * + * Only two lines can ever be removed: the opening fence, and the final line when + * it is also a fence. An interior fence line is always treated as content, because + * a generated body may legitimately contain line-leading backticks (code that + * builds a markdown string) and there is no way to tell that apart from a + * delimiter. Scanning for the *last* fence anywhere would truncate such a body + * whenever the response is cut off before its closing fence. + * + * The cost is that a model which answers with several fenced blocks and prose + * between them keeps that prose — visibly wrong output the user can re-roll, + * rather than code quietly missing a chunk. + * + * Falls back to the original text if stripping would leave nothing. + */ +export function stripCodeFences(text: string): string { + const lines = text.split('\n') + const openingFence = lines.findIndex((line) => FENCE_LINE.test(line)) + if (openingFence === -1) return text + + // Anything non-blank ahead of the first fence means the response does not open + // with one, so the backticks belong to the content. + for (let index = 0; index < openingFence; index++) { + if (lines[index].trim() !== '') return text + } + + const inner = lines.slice(openingFence + 1) + + // Trim blank lines only — leading whitespace on a kept line is indentation, + // which is load-bearing in Python. + while (inner.length > 0 && inner[inner.length - 1].trim() === '') inner.pop() + + // Only the very last line may close the wrapper. A truncated response simply + // has no closer, and every line after the opener survives. The one case this + // cannot get right is generation stopping exactly on an embedded delimiter, + // where that final line is content — indistinguishable from a real closer, and + // rarer than the wrap it would otherwise fail to strip. + if (inner.length > 0 && FENCE_LINE.test(inner[inner.length - 1])) inner.pop() + + while (inner.length > 0 && inner[0].trim() === '') inner.shift() + while (inner.length > 0 && inner[inner.length - 1].trim() === '') inner.pop() + + return inner.length > 0 ? inner.join('\n') : text +} 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/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..d0a27422a99 --- /dev/null +++ b/apps/sim/providers/openai/core.transport-phase.test.ts @@ -0,0 +1,226 @@ +/** + * @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) + }) + + /** + * 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, + 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..77848c08937 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}` } /** @@ -270,22 +410,41 @@ 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, abortSignal = request.abortSignal ): Promise => { 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) + const message = await parseErrorResponse(response, startedAt) const strippedBody = isReasoningSummaryVerificationError(response.status, message) ? stripReasoningSummary(body) : null @@ -298,14 +457,9 @@ 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) + const retryMessage = await parseErrorResponse(retryResponse, startedAt) throw new Error( `${config.providerLabel} API error (${retryResponse.status}): ${retryMessage}` ) @@ -316,8 +470,25 @@ export async function executeResponsesProviderRequest( const postResponses = async ( body: Record ): Promise => { - const response = await fetchResponsesWithSummaryFallback(body) - return response.json() + const startedAt = Date.now() + + const response = await fetchResponsesWithSummaryFallback(body, 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 +526,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 +554,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 +898,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 } 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) + ) +} 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() && (