diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/sub-block-renderer.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/sub-block-renderer.tsx index 20492c4127d..e19bac41ca3 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/sub-block-renderer.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/sub-block-renderer.tsx @@ -1,6 +1,7 @@ 'use client' import { useCallback, useEffect, useRef } from 'react' +import { isUserSuppliedToolParam } from '@/lib/workflows/tool-input/param-visibility' import { buildToolSubBlockId, resolveToolParamSync, @@ -122,8 +123,10 @@ export function ToolSubBlockRenderer({ pushParamValueToStore(toolParamValue) }, [toolParamValue, pushParamValueToStore]) - const visibility = subBlock.paramVisibility ?? 'user-or-llm' - const isOptionalForUser = visibility !== 'user-only' + // Shared with the fork-sync gate so "is this the user's to fill?" is answered the same way + // in the editor and when a sync decides whether a blank value blocks. `required` itself + // stays for the field below to resolve in its own value context. + const isOptionalForUser = !isUserSuppliedToolParam(subBlock) const config = { ...subBlock, diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.test.ts b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.test.ts new file mode 100644 index 00000000000..3304c6dbd9d --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.test.ts @@ -0,0 +1,34 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { dependentFieldNoun } from '@/ee/workspace-forking/components/fork-sync/dependent-field-noun' + +describe('dependentFieldNoun', () => { + it('strips a leading imperative verb so copy does not stutter', () => { + // The defect this exists to prevent: `Select ${title.toLowerCase()}` on a title that + // already reads as an instruction rendered "Select select issue". + expect(dependentFieldNoun('Select Issue')).toBe('issue') + expect(dependentFieldNoun('Select Project')).toBe('project') + expect(dependentFieldNoun('Choose Document')).toBe('document') + expect(dependentFieldNoun('Pick a Table')).toBe('a table') + }) + + it('leaves a title that merely starts with those letters alone', () => { + // The trailing `\s+` is what separates the verb from a word that begins with it. + expect(dependentFieldNoun('Selected Files')).toBe('selected files') + expect(dependentFieldNoun('Selection')).toBe('selection') + }) + + it('falls back to the whole title when stripping would leave nothing', () => { + // A bare verb has no noun to extract; an empty result would render "Select " and + // "No found". + expect(dependentFieldNoun('Select')).toBe('select') + expect(dependentFieldNoun('Select ')).toBe('select ') + }) + + it('passes a plain noun through lowercased', () => { + expect(dependentFieldNoun('Label')).toBe('label') + expect(dependentFieldNoun('Conflict Column')).toBe('conflict column') + }) +}) diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.ts b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.ts new file mode 100644 index 00000000000..566bd7166cc --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.ts @@ -0,0 +1,19 @@ +/** + * Leading imperative verb on a field title. Titles are labels, and some already read as an + * instruction ("Select Issue", "Choose Project"), so composing surrounding copy onto them + * verbatim produces "Select select issue". The trailing `\s+` is load-bearing: it stops + * "Selected Files" and "Selection" from being mangled into "ed Files" / "ion". + */ +const LEADING_IMPERATIVE_VERB = /^(?:select|choose|pick)\s+/i + +/** + * The bare noun of a dependent field's title, for copy that supplies its own verb + * ("Select {noun}", "Search {noun}...", "No {noun} found"). + * + * Falls back to the whole title when stripping would leave nothing — a title that is only a + * verb has no noun to extract, and an empty noun would render "Select " and "No found". + */ +export function dependentFieldNoun(title: string): string { + const stripped = title.replace(LEADING_IMPERATIVE_VERB, '').trim() + return (stripped || title).toLowerCase() +} diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-selector.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-selector.tsx index a962841025d..a10e098e491 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-selector.tsx +++ b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-selector.tsx @@ -2,6 +2,7 @@ import { useMemo } from 'react' import { ChipCombobox, type ComboboxOption, Loader } from '@sim/emcn' +import { dependentFieldNoun } from '@/ee/workspace-forking/components/fork-sync/dependent-field-noun' import type { SelectorContext, SelectorKey } from '@/hooks/selectors/types' import { useSelectorOptions } from '@/hooks/selectors/use-selector-query' @@ -46,6 +47,8 @@ export function DependentFieldSelector({ [options] ) + const noun = dependentFieldNoun(title) + if (isLoading && enabled) { return (
@@ -62,10 +65,10 @@ export function DependentFieldSelector({ value={value || undefined} onChange={(next) => onChange(next)} searchable - searchPlaceholder={`Search ${title.toLowerCase()}...`} - placeholder={`Select ${title.toLowerCase()}`} + searchPlaceholder={`Search ${noun}...`} + placeholder={`Select ${noun}`} disabled={!enabled} - emptyMessage={`No ${title.toLowerCase()} found`} + emptyMessage={`No ${noun} found`} /> ) } diff --git a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts index 542deab06e8..9895edff498 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts @@ -1,7 +1,20 @@ /** * @vitest-environment node */ -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetToolInputParamConfigs } = vi.hoisted(() => ({ + mockGetToolInputParamConfigs: vi.fn(() => [] as unknown[]), +})) + +/** + * Mocked at the module boundary so these tests stay about the collector's own logic rather + * than the tool/block registries the real resolver reaches into. + */ +vi.mock('@/lib/workflows/search-replace/indexer', () => ({ + getToolInputParamConfigs: mockGetToolInputParamConfigs, +})) + import { getBlock } from '@/blocks/registry' import type { BlockConfig, SubBlockConfig } from '@/blocks/types' import { @@ -45,10 +58,22 @@ const replaceItem = { mode: 'replace' as const, } -// No persisted block map in these unit tests, so the resolver derives - matching the -// `deriveForkBlockId(...)` ids the expectations assert. +/** + * No persisted block map in these unit tests, so the resolver derives - matching the + * `deriveForkBlockId(...)` ids the expectations assert. + */ const resolve = buildForkBlockIdResolver(true, EMPTY_FORK_BLOCK_MAP) +/** + * The indexer mock is module-scoped, so a `mockReturnValue` from one test would otherwise + * leak into the next and make these order-dependent. Reset to the empty (no authoritative + * visibility) default before each. + */ +beforeEach(() => { + mockGetToolInputParamConfigs.mockReset() + mockGetToolInputParamConfigs.mockReturnValue([]) +}) + describe('collectForkDependentReconfigs', () => { it("emits the active operation's credential-dependent selector (condition-gated)", () => { vi.mocked(getBlock).mockReturnValue( @@ -759,6 +784,373 @@ describe('collectForkDependentReconfigs', () => { }) }) +/** + * A sync carries the source's configuration across; it never invents configuration the + * source never had. A blank selector is still OFFERED (so it can be set in place during the + * swap) but must not gate the sync. + */ +describe('collectForkDependentReconfigs — blank source values never gate', () => { + const jiraProjectBlock = () => + blockWith([ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { id: 'operation', title: 'Operation', type: 'dropdown' }, + { + id: 'projectId', + title: 'Select Project', + type: 'project-selector', + canonicalParamId: 'projectId', + selectorKey: 'jira.projects', + dependsOn: ['credential'], + mode: 'basic', + required: { field: 'operation', value: ['write'] }, + }, + // The advanced twin carries the SAME `required` but no `selectorKey`, so it is never + // emitted. That asymmetry is what made an identical blank config gate or not gate + // purely on a display preference. + { + id: 'manualProjectId', + title: 'Project ID', + type: 'short-input', + canonicalParamId: 'projectId', + dependsOn: ['credential'], + mode: 'advanced', + required: { field: 'operation', value: ['write'] }, + }, + ]) + + it('offers a blank required selector but does not mark it required', () => { + vi.mocked(getBlock).mockReturnValue(jiraProjectBlock()) + const states = new Map([ + [ + 'wf-src', + sourceState('jira', { + credential: { value: 'cred-src' }, + operation: { value: 'write' }, + projectId: { value: '' }, + }), + ], + ]) + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + expect(result).toHaveLength(1) + expect(result[0]).toMatchObject({ subBlockKey: 'projectId', required: false, sourceValue: '' }) + }) + + it('still marks a populated required selector as required', () => { + vi.mocked(getBlock).mockReturnValue(jiraProjectBlock()) + const states = new Map([ + [ + 'wf-src', + sourceState('jira', { + credential: { value: 'cred-src' }, + operation: { value: 'write' }, + projectId: { value: 'PROJ-1' }, + }), + ], + ]) + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + expect(result[0]).toMatchObject({ subBlockKey: 'projectId', required: true }) + }) + + it('still gates a dependent whose source value is a non-string', () => { + // A multi-select selector (e.g. zoho-desk `departmentIds`) stores an array. The wire + // `sourceValue` coerces non-strings to '' - if the emptiness check read that coerced + // value, a populated multi-select would report blank and silently stop gating. + vi.mocked(getBlock).mockReturnValue(jiraProjectBlock()) + const states = new Map([ + [ + 'wf-src', + sourceState('jira', { + credential: { value: 'cred-src' }, + operation: { value: 'write' }, + projectId: { value: ['PROJ-1'] as unknown as string }, + }), + ], + ]) + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + expect(result[0]).toMatchObject({ subBlockKey: 'projectId', required: true }) + }) + + it('does not gate a dependent whose source value is an empty array', () => { + vi.mocked(getBlock).mockReturnValue(jiraProjectBlock()) + const states = new Map([ + [ + 'wf-src', + sourceState('jira', { + credential: { value: 'cred-src' }, + operation: { value: 'write' }, + projectId: { value: [] as unknown as string }, + }), + ], + ]) + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + expect(result[0]).toMatchObject({ subBlockKey: 'projectId', required: false }) + }) + + it('reaches the same verdict in basic and advanced canonical mode', () => { + vi.mocked(getBlock).mockReturnValue(jiraProjectBlock()) + const blankSubBlocks = { + credential: { value: 'cred-src' }, + operation: { value: 'write' }, + projectId: { value: '' }, + manualProjectId: { value: '' }, + } + const basic = sourceState('jira', blankSubBlocks) + const advanced = sourceState('jira', blankSubBlocks) as unknown as WorkflowState & { + blocks: Record }> + } + advanced.blocks['block-1'].data = { canonicalModes: { projectId: 'advanced' } } + + const basicResult = collectForkDependentReconfigs( + [replaceItem], + new Map([['wf-src', basic]]), + resolve + ) + const advancedResult = collectForkDependentReconfigs( + [replaceItem], + new Map([['wf-src', advanced as unknown as WorkflowState]]), + resolve + ) + // Advanced drops the row entirely (the dormant-member guard), basic keeps it but + // non-blocking. Pin BOTH shapes, not just `.every(...)`: over an empty array `.every` + // is vacuously true, so an advanced path that regressed to emitting a required row + // would still pass. + expect(basicResult).toHaveLength(1) + expect(basicResult[0]).toMatchObject({ subBlockKey: 'projectId', required: false }) + expect(advancedResult).toHaveLength(0) + }) +}) + +/** + * Inside a `tool-input`, only a `user-only` param is the user's to supply. A `user-or-llm` + * or `llm-only` param is filled by the model at runtime (`createLLMToolSchema` keeps an empty + * one in the schema handed to the model), so a blank value there is a deliberate + * configuration state and must never gate a sync. + */ +describe('collectForkDependentReconfigs — nested tool params follow ParameterVisibility', () => { + const agentWithJiraTool = () => + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'jira') + return blockWith([ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { + id: 'issueKey', + title: 'Select Issue', + type: 'file-selector', + canonicalParamId: 'issueKey', + selectorKey: 'jira.issues', + dependsOn: ['credential'], + required: true, + }, + { + id: 'domain', + title: 'Domain', + type: 'short-input', + selectorKey: 'jira.domains', + dependsOn: ['credential'], + required: true, + }, + ]) + return undefined as unknown as BlockConfig + }) + + const stateWithIssueKey = (issueKey: string) => + new Map([ + [ + 'wf-src', + sourceState('agent', { + tools: { + value: [ + { + type: 'jira', + title: 'Jira', + operation: 'read', + params: { credential: 'cred-src', domain: 'acme.atlassian.net', issueKey }, + }, + ], + }, + }), + ], + ]) + + const resolvedParams = (visibilityByParam: Record) => + Object.entries(visibilityByParam).map(([paramId, paramVisibility]) => ({ + paramId, + authoritative: true, + config: { id: paramId, type: 'short-input', paramVisibility }, + value: undefined, + })) + + it('does not require a blank user-or-llm param the agent fills at runtime', () => { + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue( + resolvedParams({ issueKey: 'user-or-llm', domain: 'user-only' }) + ) + const result = collectForkDependentReconfigs([replaceItem], stateWithIssueKey(''), resolve) + const issue = result.find((f) => f.subBlockKey === 'tools[0].issueKey') + expect(issue).toMatchObject({ required: false, toolName: 'Jira' }) + }) + + it('does not require a POPULATED user-or-llm param either', () => { + // Visibility, not emptiness, is the rule here. A parent swap blanks this field in the + // modal (`effectiveDependentValue`), so an emptiness-only guard would still gate it. + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue( + resolvedParams({ issueKey: 'user-or-llm', domain: 'user-only' }) + ) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + const issue = result.find((f) => f.subBlockKey === 'tools[0].issueKey') + expect(issue).toMatchObject({ required: false, sourceValue: 'ACME-999' }) + }) + + it('still requires a populated user-only param', () => { + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue( + resolvedParams({ issueKey: 'user-or-llm', domain: 'user-only' }) + ) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + const domain = result.find((f) => f.subBlockKey === 'tools[0].domain') + expect(domain).toMatchObject({ required: true }) + }) + + it('falls back to the block-level required when no authoritative visibility exists', () => { + // custom-tool / MCP / unresolvable tool id -> the resolver has no authoritative entry. + // Fail closed: keep the pre-existing gate rather than silently un-gating an unknown schema. + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue([]) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + const issue = result.find((f) => f.subBlockKey === 'tools[0].issueKey') + expect(issue).toMatchObject({ required: true }) + }) + + it('ignores a non-authoritative visibility rather than trusting it to un-gate', () => { + // A generic/inferred entry carries no reliable annotation, so it must not be able to + // turn a gating field into a non-gating one. + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue([ + { + paramId: 'issueKey', + authoritative: false, + config: { id: 'issueKey', type: 'short-input', paramVisibility: 'user-or-llm' }, + value: undefined, + }, + ]) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + expect(result.find((f) => f.subBlockKey === 'tools[0].issueKey')).toMatchObject({ + required: true, + }) + }) + + it('fails closed when an authoritative entry carries no visibility', () => { + // The real resolver's `uncoveredParams` branch builds its config via + // `buildToolInputSearchConfig`, which does NOT copy `paramVisibility` - so the map holds + // the key with an `undefined` value. That must fall back to the block-level `required`, + // not be read as "not user-only". + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue([ + { + paramId: 'issueKey', + authoritative: true, + config: { id: 'issueKey', type: 'short-input' }, + value: undefined, + }, + ]) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + expect(result.find((f) => f.subBlockKey === 'tools[0].issueKey')).toMatchObject({ + required: true, + }) + }) + + it('resolves visibility through the canonical param id when the sub-block id differs', () => { + // The resolver keys by its own paramId; a canonical pair's sub-block id can differ, so the + // map is double-keyed and the lookup falls back to `canonicalParamId`. + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'jira') + return blockWith([ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { + id: 'issueKeySelector', + title: 'Select Issue', + type: 'file-selector', + canonicalParamId: 'issueKey', + selectorKey: 'jira.issues', + dependsOn: ['credential'], + required: true, + }, + ]) + return undefined as unknown as BlockConfig + }) + mockGetToolInputParamConfigs.mockReturnValue([ + { + paramId: 'issueKey', + authoritative: true, + config: { id: 'issueKey', type: 'file-selector', paramVisibility: 'user-or-llm' }, + value: undefined, + }, + ]) + const result = collectForkDependentReconfigs( + [replaceItem], + stateWithIssueKey('ACME-999'), + resolve + ) + // Found via canonicalParamId -> user-or-llm -> not the user's to fill. + expect(result.find((f) => f.subBlockKey === 'tools[0].issueKeySelector')).toMatchObject({ + required: false, + }) + }) + + it('does not gate a blank user-only nested param', () => { + // Both invariants fire at once: user-only (so visibility would gate) but blank in the + // source (so there is nothing to carry across). + agentWithJiraTool() + mockGetToolInputParamConfigs.mockReturnValue( + resolvedParams({ issueKey: 'user-or-llm', domain: 'user-only' }) + ) + const states = new Map([ + [ + 'wf-src', + sourceState('agent', { + tools: { + value: [ + { + type: 'jira', + title: 'Jira', + operation: 'read', + params: { credential: 'cred-src', domain: '', issueKey: '' }, + }, + ], + }, + }), + ], + ]) + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + expect(result.find((f) => f.subBlockKey === 'tools[0].domain')).toMatchObject({ + required: false, + }) + }) +}) + describe('collectForkResourceUsages', () => { const usageItem = ( sourceWorkflowId: string, diff --git a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts index e064216e588..8982e9b0ef1 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -2,6 +2,7 @@ import { isRecordLike } from '@sim/utils/object' import type { ForkDependentReconfig, ForkResourceUsage } from '@/lib/api/contracts/workspace-fork' import { coerceObjectArray } from '@/lib/workflows/persistence/remap-internal-ids' import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' +import { getToolInputParamConfigs } from '@/lib/workflows/search-replace/indexer' import { buildSelectorContextFromBlock, SELECTOR_CONTEXT_FIELDS, @@ -14,6 +15,7 @@ import { isNonEmptyValue, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' +import { resolveToolParamRequired } from '@/lib/workflows/tool-input/param-visibility' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' import { getDependsOnFields } from '@/blocks/utils' @@ -21,10 +23,10 @@ import type { ForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block import { toScannerBlocks } from '@/ee/workspace-forking/lib/remap/reference-scan' import { createCanonicalModeGates, - isSubBlockRequired, scanWorkflowReferences, } from '@/ee/workspace-forking/lib/remap/remap-references' import type { WorkflowState } from '@/stores/workflows/workflow/types' +import type { ParameterVisibility } from '@/tools/types' const isSelectorContextKey = ( key: string @@ -84,6 +86,18 @@ interface EmitAnchoredParams { * credential-anchored field). */ chaining: boolean + /** + * Present ONLY for the nested `tool-input` pass: each param's resolved + * {@link ParameterVisibility}, keyed by sub-block id and by canonical param id. Its presence + * is what marks a dependent as a tool param rather than a block sub-block, so `required` + * can apply the tool-row rule (see {@link resolveToolParamRequired}). + * + * Two cases fall back to the block-level `required`, failing closed: a param absent from + * the map (custom-tool / MCP generic fallback, or an unresolvable tool id), and a param + * present with an `undefined` value — the resolver's `buildToolInputSearchConfig` branch + * does not copy `paramVisibility`, so an authoritative entry can still carry none. + */ + paramVisibilityById?: Map out: ForkDependentReconfig[] } @@ -107,6 +121,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { makeTitle, toolName, chaining, + paramVisibilityById, out, } = params const fullContext = buildSelectorContextFromBlock(contextBlockType, contextSubBlocks, { @@ -198,6 +213,21 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { ? values[dependent.canonicalParamId] : undefined) const rawSourceValue = typeof rawDependentValue === 'string' ? rawDependentValue : '' + // Two independent invariants decide whether this row GATES the sync (it is always + // offered either way - see the comment above the `condition` skip): + // + // 1. A sync carries the source's configuration across; it never invents configuration + // the source never had. A field the source left blank has nothing to carry, so it + // cannot block. A genuinely missing value is still caught by the block's own + // required-field validation at run/deploy time. + // 2. Inside a `tool-input`, only a `user-only` param is the user's to supply; a + // `user-or-llm` / `llm-only` param is filled by the model at runtime, so a blank + // one is intentional. Applies to the nested pass only, where visibility is known. + // + // Testing `rawSourceValue` directly is sound: the dormant guard above has already + // returned for any pair in advanced mode, so the pair is basic-active here and + // `rawSourceValue` IS the group's active canonical value. + const configuredRequired = resolveToolParamRequired(dependent, values, paramVisibilityById) out.push({ parentKind: anchor.parentKind, parentSourceId, @@ -213,7 +243,11 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { // The diff route overlays the stored/target-draft value onto `currentValue`; // `sourceValue` stays the raw source reference (the copy-resolved parent's seed). currentValue: rawSourceValue, - required: isSubBlockRequired(dependent.required, values), + // Ask the emptiness question of the RAW value, not the string-coerced one: + // `rawSourceValue` flattens every non-string (a multi-select selector stores an + // array) to `''`, which would report a populated field as blank and silently + // un-gate it. `isNonEmptyValue` handles arrays and non-strings on purpose. + required: configuredRequired && isNonEmptyValue(rawDependentValue), providesContextKey, consumesContextKeys, context: dependentContext, @@ -315,6 +349,28 @@ export function collectForkDependentReconfigs( typeof tool.title === 'string' && tool.title ? tool.title : toolConfig.name const toolInputKey = cfg.id const toolIndex = index + // Resolved `ParameterVisibility` per param, from the same resolver the tool-row UI + // and the rest of fork remapping use - so "is this the user's to fill?" is answered + // identically in the editor and in the sync gate. Keyed by both the sub-block id and + // its canonical param id, since a nested tool stores picks under either. + const paramVisibilityById = new Map() + for (const resolved of getToolInputParamConfigs({ + tool: { ...tool, type: tool.type, params: toolParams }, + toolIndex, + parentCanonicalModes: block.data?.canonicalModes, + })) { + if (!resolved.authoritative) continue + const visibility = resolved.config.paramVisibility + paramVisibilityById.set(resolved.paramId, visibility) + // The canonical id is an ALIAS, so it must never clobber a param that owns that + // key as its own `paramId` - first (own-id) write wins. + if ( + resolved.config.canonicalParamId && + !paramVisibilityById.has(resolved.config.canonicalParamId) + ) { + paramVisibilityById.set(resolved.config.canonicalParamId, visibility) + } + } emitAnchoredDependents({ config: toolConfig, values: toolValues, @@ -332,6 +388,7 @@ export function collectForkDependentReconfigs( makeTitle: (dependent) => dependent.title ?? dependent.id ?? '', toolName: toolLabel, chaining: false, + paramVisibilityById, out, }) } 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 26bd1bc0d55..4f050b71c5d 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 @@ -6,16 +6,25 @@ import type { BlockConfig, SubBlockConfig } from '@/blocks/types' // The indexer resolves a tool's params via the tool registry; stub it so the // injected blockConfigs subBlocks drive resolution deterministically in tests. +// Exposed as vi.fn()s (with the historical defaults) so a test that needs an +// AUTHORITATIVE resolution - i.e. one carrying `paramVisibility` - can opt in. +const { mockGetToolIdForOperation, mockGetSubBlocksForToolInput } = vi.hoisted(() => ({ + mockGetToolIdForOperation: vi.fn((): string | undefined => undefined), + mockGetSubBlocksForToolInput: vi.fn( + ( + _toolId: string, + _type: string, + _values: unknown, + _modes: unknown, + provided?: { subBlocks?: SubBlockConfig[] } + ) => ({ subBlocks: provided?.subBlocks ?? [] }) + ), +})) + vi.mock('@/tools/params', () => ({ - getToolIdForOperation: () => undefined, + getToolIdForOperation: mockGetToolIdForOperation, getToolParametersConfig: () => null, - getSubBlocksForToolInput: ( - _toolId: string, - _type: string, - _values: unknown, - _modes: unknown, - provided?: { subBlocks?: SubBlockConfig[] } - ) => ({ subBlocks: provided?.subBlocks ?? [] }), + getSubBlocksForToolInput: mockGetSubBlocksForToolInput, formatParameterLabel: (label: string) => label, })) @@ -1005,6 +1014,52 @@ describe('collectClearedDependents', () => { }, ]) }) + + it('does not mark a cleared model-supplied tool param as required', () => { + // The pre-sync modal treats a `user-or-llm` param as non-blocking (the agent fills it at + // runtime). This collector must agree: a `required` entry here makes promote SKIP the + // target's redeploy, so disagreeing would let a sync through and then silently withhold + // the deployment. + mockGetToolIdForOperation.mockReturnValueOnce('gmail_read') + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'gmail') + return blockWith([ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { + id: 'folder', + title: 'Label', + type: 'folder-selector', + dependsOn: ['credential'], + required: true, + paramVisibility: 'user-or-llm', + }, + ]) + return undefined as unknown as BlockConfig + }) + const targetDraft: SubBlockRecord = { + tools: entry('tools', 'tool-input', [ + { type: 'gmail', title: 'Gmail', params: { credential: 'c-target', folder: 'INBOX' } }, + ]), + } + const merged: SubBlockRecord = { + tools: entry('tools', 'tool-input', [ + { type: 'gmail', title: 'Gmail', params: { credential: 'c-new', folder: '' } }, + ]), + } + const result = collectClearedDependents('agent', 'b1', 'Agent', targetDraft, merged) + // Still surfaced (the value really was cleared), just not gating the redeploy. + expect(result).toEqual([ + { + blockId: 'b1', + blockName: 'Agent', + subBlockKey: 'tools[0].folder', + title: 'Label', + toolName: 'Gmail', + required: false, + }, + ]) + }) }) describe('applyDependentOverrides', () => { 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 f422496a4be..1a71e90e60b 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -29,6 +29,10 @@ import { resolveCanonicalMode, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' +import { + isSubBlockRequired, + resolveToolParamRequired, +} from '@/lib/workflows/tool-input/param-visibility' import type { ParsedStoredTool } from '@/lib/workflows/tool-input/types' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' @@ -38,6 +42,7 @@ import { remapForkFileUploadValue, } from '@/ee/workspace-forking/lib/remap/remap-files' import { isEnvVarReference, isReference } from '@/executor/constants' +import type { ParameterVisibility } from '@/tools/types' /** * Resource kinds the fork remapper rewrites across workspaces, derived from the @@ -1191,20 +1196,6 @@ export interface NeedsConfigurationField { required: boolean } -/** Evaluate a subblock's `required` (boolean | condition | fn) against a value map. */ -export function isSubBlockRequired( - required: SubBlockConfig['required'], - values: Record -): boolean { - if (required === true) return true - if (!required) return false - // The object/function forms are structurally a SubBlockCondition. - return evaluateSubBlockCondition( - required as Parameters[0], - values - ) -} - /** Nested `tool-input` dependents (Agent/tool blocks) the TARGET configured that a remap cleared. */ function collectClearedToolParamDependents( toolInputKey: string, @@ -1245,6 +1236,27 @@ function collectClearedToolParamDependents( scopeCanonicalModesForTool(parentCanonicalModes, index, tool.type) ) const toolLabel = typeof tool.title === 'string' && tool.title ? tool.title : toolConfig.name + // Resolved visibility per param, so `required` here means the same thing it means in the + // pre-sync modal. Without this the two paths disagree: the modal would let a sync through + // (a model-supplied param is not the user's to fill) and then this collector would mark it + // required, which SKIPS the target's redeploy in `promote.ts` - leaving the fork silently + // running its previous deployed version. + const paramVisibilityById = new Map() + for (const resolved of getToolInputParamConfigs({ + tool: { ...tool, type: tool.type, params: mergedParams }, + toolIndex: index, + parentCanonicalModes, + })) { + if (!resolved.authoritative) continue + const visibility = resolved.config.paramVisibility + paramVisibilityById.set(resolved.paramId, visibility) + if ( + resolved.config.canonicalParamId && + !paramVisibilityById.has(resolved.config.canonicalParamId) + ) { + paramVisibilityById.set(resolved.config.canonicalParamId, visibility) + } + } for (const cfg of toolConfig.subBlocks) { if (!cfg.dependsOn || !cfg.id) continue // Only flag a param the TARGET tool had configured (not one the source carried in). @@ -1260,7 +1272,7 @@ function collectClearedToolParamDependents( subBlockKey: `${toolInputKey}[${index}].${cfg.id}`, title: cfg.title ?? cfg.id, toolName: toolLabel, - required: isSubBlockRequired(cfg.required, mergedValues), + required: resolveToolParamRequired(cfg, mergedValues, paramVisibilityById), }) } } diff --git a/apps/sim/lib/workflows/tool-input/param-visibility.test.ts b/apps/sim/lib/workflows/tool-input/param-visibility.test.ts new file mode 100644 index 00000000000..3b1365928c2 --- /dev/null +++ b/apps/sim/lib/workflows/tool-input/param-visibility.test.ts @@ -0,0 +1,63 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + isSubBlockRequired, + isToolParamUserRequired, + isUserSuppliedToolParam, +} from '@/lib/workflows/tool-input/param-visibility' + +describe('isUserSuppliedToolParam', () => { + it('is true only for user-only', () => { + expect(isUserSuppliedToolParam({ paramVisibility: 'user-only' })).toBe(true) + expect(isUserSuppliedToolParam({ paramVisibility: 'user-or-llm' })).toBe(false) + expect(isUserSuppliedToolParam({ paramVisibility: 'llm-only' })).toBe(false) + expect(isUserSuppliedToolParam({ paramVisibility: 'hidden' })).toBe(false) + }) + + it('treats an undeclared visibility as user-or-llm', () => { + // Matches the tool-row renderer's fallback: an unannotated param is never user-required. + expect(isUserSuppliedToolParam({})).toBe(false) + }) +}) + +describe('isToolParamUserRequired', () => { + it('requires both user-only visibility and a satisfied required condition', () => { + expect(isToolParamUserRequired({ paramVisibility: 'user-only', required: true }, {})).toBe(true) + expect(isToolParamUserRequired({ paramVisibility: 'user-only', required: false }, {})).toBe( + false + ) + }) + + it('never requires a param the model can supply, even when required is true', () => { + // `required: true` still drives the model-facing schema; it just is not the USER's to fill. + expect(isToolParamUserRequired({ paramVisibility: 'user-or-llm', required: true }, {})).toBe( + false + ) + expect(isToolParamUserRequired({ paramVisibility: 'llm-only', required: true }, {})).toBe(false) + }) + + it('evaluates the condition form against the surrounding values', () => { + const config = { + paramVisibility: 'user-only' as const, + required: { field: 'operation', value: ['write'] }, + } + expect(isToolParamUserRequired(config, { operation: 'write' })).toBe(true) + expect(isToolParamUserRequired(config, { operation: 'read' })).toBe(false) + }) +}) + +describe('isSubBlockRequired', () => { + it('handles the boolean and absent forms', () => { + expect(isSubBlockRequired(true, {})).toBe(true) + expect(isSubBlockRequired(false, {})).toBe(false) + expect(isSubBlockRequired(undefined, {})).toBe(false) + }) + + it('evaluates the condition form', () => { + const required = { field: 'operation', value: ['write', 'read-bulk'] } + expect(isSubBlockRequired(required, { operation: 'write' })).toBe(true) + expect(isSubBlockRequired(required, { operation: 'search' })).toBe(false) + }) +}) diff --git a/apps/sim/lib/workflows/tool-input/param-visibility.ts b/apps/sim/lib/workflows/tool-input/param-visibility.ts new file mode 100644 index 00000000000..22272e692ec --- /dev/null +++ b/apps/sim/lib/workflows/tool-input/param-visibility.ts @@ -0,0 +1,87 @@ +import { evaluateSubBlockCondition } from '@/lib/workflows/subblocks/visibility' +import type { SubBlockConfig } from '@/blocks/types' +import type { ParameterVisibility } from '@/tools/types' + +/** + * Visibility assumed for a `tool-input` param that declares none. Matches the tool-row + * renderer's own fallback, so an unannotated param is never treated as user-required. + */ +const DEFAULT_TOOL_PARAM_VISIBILITY: ParameterVisibility = 'user-or-llm' + +/** + * Whether a param nested in a `tool-input` must be supplied by the USER. + * + * Only `user-only` qualifies — it is the one visibility where no other source for the value + * exists. A `user-or-llm` or `llm-only` param is still mandatory at runtime, but the model + * supplies it: `createLLMToolSchema` keeps an empty param in the schema handed to the model + * (and in that schema's `required` list), so a blank value is a deliberate configuration + * state rather than a missing one. + * + * The predicate is deliberately the positive `=== 'user-only'` rather than a + * `user-or-llm || llm-only` denylist: the two non-user visibilities reach the same verdict + * for different reasons, and a denylist would silently mis-classify any visibility added + * later. + */ +export function isToolParamUserRequired( + config: Pick, + values: Record +): boolean { + if (!isUserSuppliedToolParam(config)) return false + return isSubBlockRequired(config.required, values) +} + +/** + * The visibility half of {@link isToolParamUserRequired}, without evaluating `required`. + * + * Callers that let a downstream component resolve `required` in its own value context (the + * tool-row renderer) need exactly this question and must not collapse the condition here — + * doing so would evaluate it against a different value map than the one the field renders + * with. + */ +export function isUserSuppliedToolParam(config: Pick): boolean { + return (config.paramVisibility ?? DEFAULT_TOOL_PARAM_VISIBILITY) === 'user-only' +} + +/** + * Whether a `tool-input` param must be supplied by the user, resolving its visibility from a + * map keyed by sub-block id and canonical param id. + * + * Shared by fork sync's PRE-sync collector (which decides whether a row blocks the Sync + * button) and its POST-sync collector (whose `required` entries make promote SKIP the + * target's redeploy). Those two must agree: if the modal lets a sync through because a param + * is the model's to fill, the promote path must not then withhold the deployment for the + * same param. + * + * `paramVisibilityById` omitted means the caller is not in a tool-input context (a block's + * own sub-blocks), so the plain block-level rule applies. A param absent from the map, or + * present with an `undefined` value, falls back the same way — failing closed. + */ +export function resolveToolParamRequired( + config: Pick, + values: Record, + paramVisibilityById?: ReadonlyMap +): boolean { + if (!paramVisibilityById) return isSubBlockRequired(config.required, values) + const visibility = + paramVisibilityById.get(config.id) ?? + (config.canonicalParamId ? paramVisibilityById.get(config.canonicalParamId) : undefined) + if (visibility === undefined) return isSubBlockRequired(config.required, values) + return isToolParamUserRequired({ required: config.required, paramVisibility: visibility }, values) +} + +/** + * Resolve a sub-block's `required` declaration against the surrounding values. `true` is + * unconditional; the object form is structurally a `SubBlockCondition` evaluated against the + * same value map the editor uses (so an operation-scoped requirement resolves per operation). + */ +export function isSubBlockRequired( + required: SubBlockConfig['required'], + values: Record +): boolean { + if (required === true) return true + if (!required) return false + return evaluateSubBlockCondition( + required as Parameters[0], + values + ) +}