From b922508a03c2cd4c1e25654b4ed7169f54389dea Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Sat, 15 Aug 2026 19:06:31 -0700 Subject: [PATCH 1/4] fix(forks): stop sync demanding config the source never had Fork sync manufactured required re-pick rows the source never asked for, disabling Sync and suppressing the "Fully mapped" badge. Two distinct causes landed on the same line in `collectForkDependentReconfigs`. Cause A - a block-level `required` applied to a nested tool param. The collector runs one helper over both a block's own subblocks and the params of each tool in a `tool-input`, reading the raw block config for the nested pass. A Jira Get Issue tool inside an Agent block therefore inherited Jira's `issueKey` required-condition even though the tool param is `visibility: 'user-or-llm'` - the agent fills it at runtime, and `createLLMToolSchema` keeps an empty one in the model's schema precisely so it can. The tool-row editor already strips `required` for anything not `user-only`; the fork collector was the only surface that did not. Cause B - demanding a value the source never had. `projectId` hangs off the credential anchor, so a Jira block on `write` with a blank project emitted a required row. The proof it is a collector bug: only members carrying a `selectorKey` are emitted, so the basic selector gated while its advanced twin (identical `required`, no `selectorKey`) did not - the same empty config blocked or not purely on a display preference. That is also why toggling to manual mode "fixed" it. Neither fix subsumes the other: an emptiness guard alone leaves a *populated* `user-or-llm` param gating after a parent swap (`effectiveDependentValue` blanks it when the parent changed), and the visibility rule alone never touches top-level subblocks. Both invariants now meet in one predicate, and the tool-row rule is extracted so the editor and the sync gate cannot drift. Rows are still emitted either way - only the gating flag changes - so no stored dependent value is orphaned. Also fixes the placeholder stutter in the re-pick selector ("Select select issue"), which composed a verb onto titles that already read as instructions. Verified end-to-end against a forked workspace: before, 3 of 7 rows were required (including one with a populated source value); after, only a top-level populated required field gates, Sync enables, and the badge reads "Fully mapped". --- .../components/tools/sub-block-renderer.tsx | 7 +- .../fork-sync/dependent-field-selector.tsx | 11 +- .../lib/mapping/dependent-reconfigs.test.ts | 225 ++++++++++++++++++ .../lib/mapping/dependent-reconfigs.ts | 61 ++++- .../lib/remap/remap-references.ts | 15 +- .../tool-input/param-visibility.test.ts | 63 +++++ .../workflows/tool-input/param-visibility.ts | 60 +++++ 7 files changed, 421 insertions(+), 21 deletions(-) create mode 100644 apps/sim/lib/workflows/tool-input/param-visibility.test.ts create mode 100644 apps/sim/lib/workflows/tool-input/param-visibility.ts 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-selector.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-selector.tsx index a962841025d..35a1cfc3890 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 @@ -46,6 +46,11 @@ export function DependentFieldSelector({ [options] ) + // A field title is a label, and some already read as an instruction ("Select Issue", + // "Select Project"). Composing those directly produced "Select select issue", so strip a + // leading verb to get the bare noun the surrounding copy supplies its own verb for. + const noun = title.replace(/^select\s+/i, '').toLowerCase() + if (isLoading && enabled) { return (
@@ -62,10 +67,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 50accd71868..a849cf6cdd5 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 @@ -2,6 +2,17 @@ * @vitest-environment node */ import { 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 { @@ -689,6 +700,220 @@ 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('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 (dormant member); basic keeps it but non-blocking. + // Neither may produce a required row from the same blank pair. + expect(basicResult.every((f) => !f.required)).toBe(true) + expect(advancedResult.every((f) => !f.required)).toBe(true) + }) +}) + +/** + * 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 }) + }) +}) + 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 f5684cd9b26..bcbc1d58975 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -1,6 +1,7 @@ import type { ForkDependentReconfig, ForkResourceUsage } from '@/lib/api/contracts/workspace-fork' import { coerceObjectArray, isRecord } 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, @@ -13,6 +14,10 @@ import { isNonEmptyValue, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' +import { + isSubBlockRequired, + isToolParamUserRequired, +} from '@/lib/workflows/tool-input/param-visibility' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' import { getDependsOnFields } from '@/blocks/utils' @@ -20,10 +25,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 @@ -83,6 +88,15 @@ 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 isToolParamUserRequired}). A param missing from + * the map has no authoritative visibility (custom-tool / MCP generic fallback, or an + * unresolvable tool id) and falls back to the block-level `required`, failing closed. + */ + paramVisibilityById?: Map out: ForkDependentReconfig[] } @@ -106,6 +120,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { makeTitle, toolName, chaining, + paramVisibilityById, out, } = params const fullContext = buildSelectorContextFromBlock(contextBlockType, contextSubBlocks) @@ -195,6 +210,30 @@ 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 paramVisibility = paramVisibilityById + ? (paramVisibilityById.get(dependent.id) ?? + (dependent.canonicalParamId + ? paramVisibilityById.get(dependent.canonicalParamId) + : undefined)) + : undefined + const configuredRequired = + paramVisibilityById && paramVisibility !== undefined + ? isToolParamUserRequired({ required: dependent.required, paramVisibility }, values) + : isSubBlockRequired(dependent.required, values) out.push({ parentKind: anchor.parentKind, parentSourceId, @@ -210,7 +249,7 @@ 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), + required: configuredRequired && isNonEmptyValue(rawSourceValue), providesContextKey, consumesContextKeys, context: dependentContext, @@ -312,6 +351,23 @@ 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) + if (resolved.config.canonicalParamId) { + paramVisibilityById.set(resolved.config.canonicalParamId, visibility) + } + } emitAnchoredDependents({ config: toolConfig, values: toolValues, @@ -329,6 +385,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.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts index 9dfb419f3ac..e6488ce0409 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -30,6 +30,7 @@ import { resolveCanonicalMode, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' +import { isSubBlockRequired } 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' @@ -1188,20 +1189,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, 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..dc7f379b954 --- /dev/null +++ b/apps/sim/lib/workflows/tool-input/param-visibility.ts @@ -0,0 +1,60 @@ +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' +} + +/** + * 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 + ) +} From 3ca8d290335990bad586da8951ebcd375a7e0ae7 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Sun, 16 Aug 2026 00:20:47 -0700 Subject: [PATCH 2/4] fix(forks): ask emptiness of the raw dependent value, not the coerced one Pre-landing review caught a silent un-gating in the new predicate: it asked `isNonEmptyValue` about `rawSourceValue`, which flattens every non-string to `''` for the wire contract. A multi-select dependent selector stores an array, so a populated one reported blank and stopped gating the sync. `isNonEmptyValue` handles arrays and non-strings deliberately - give it the raw value. Reachable today via zoho-desk `departmentIds`, the one multi-select dependent selector with a `selectorKey` + `dependsOn`. Also from review: - the canonical id is an alias, so it no longer clobbers a param that owns that key as its own `paramId` (first write wins) - drop a redundant conjunct that implied a third state the code cannot reach - document the present-but-undefined visibility case, which the resolver's `buildToolInputSearchConfig` branch produces routinely - extract the placeholder-noun transform so a bare "Select" title falls back to the whole title instead of rendering "Select " / "No found", and test it Tests: non-string and empty-array source values, both verified to fail without the fix; the basic/advanced parity case now pins both shapes rather than calling `.every()` on an empty array; the indexer mock is reset per test so the new cases are not order-dependent. --- .../fork-sync/dependent-field-noun.test.ts | 34 ++++ .../fork-sync/dependent-field-noun.ts | 19 ++ .../fork-sync/dependent-field-selector.tsx | 6 +- .../lib/mapping/dependent-reconfigs.test.ts | 171 +++++++++++++++++- .../lib/mapping/dependent-reconfigs.ts | 24 ++- 5 files changed, 239 insertions(+), 15 deletions(-) create mode 100644 apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.test.ts create mode 100644 apps/sim/ee/workspace-forking/components/fork-sync/dependent-field-noun.ts 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 35a1cfc3890..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,10 +47,7 @@ export function DependentFieldSelector({ [options] ) - // A field title is a label, and some already read as an instruction ("Select Issue", - // "Select Project"). Composing those directly produced "Select select issue", so strip a - // leading verb to get the bare noun the surrounding copy supplies its own verb for. - const noun = title.replace(/^select\s+/i, '').toLowerCase() + const noun = dependentFieldNoun(title) if (isLoading && enabled) { return ( 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 8dbd9582662..8f88955f75b 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,7 @@ /** * @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[]), @@ -60,6 +60,14 @@ const replaceItem = { // `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( @@ -837,6 +845,41 @@ describe('collectForkDependentReconfigs — blank source values never gate', () 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 = { @@ -861,10 +904,13 @@ describe('collectForkDependentReconfigs — blank source values never gate', () new Map([['wf-src', advanced as unknown as WorkflowState]]), resolve ) - // Advanced drops the row entirely (dormant member); basic keeps it but non-blocking. - // Neither may produce a required row from the same blank pair. - expect(basicResult.every((f) => !f.required)).toBe(true) - expect(advancedResult.every((f) => !f.required)).toBe(true) + // 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) }) }) @@ -982,6 +1028,121 @@ describe('collectForkDependentReconfigs — nested tool params follow ParameterV 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', () => { 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 29e47ce50fd..49d795c47e6 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -93,9 +93,12 @@ interface EmitAnchoredParams { * 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 isToolParamUserRequired}). A param missing from - * the map has no authoritative visibility (custom-tool / MCP generic fallback, or an - * unresolvable tool id) and falls back to the block-level `required`, failing closed. + * can apply the tool-row rule (see {@link isToolParamUserRequired}). + * + * 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[] @@ -234,7 +237,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { : undefined)) : undefined const configuredRequired = - paramVisibilityById && paramVisibility !== undefined + paramVisibility !== undefined ? isToolParamUserRequired({ required: dependent.required, paramVisibility }, values) : isSubBlockRequired(dependent.required, values) out.push({ @@ -252,7 +255,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: configuredRequired && isNonEmptyValue(rawSourceValue), + // 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, @@ -367,7 +374,12 @@ export function collectForkDependentReconfigs( if (!resolved.authoritative) continue const visibility = resolved.config.paramVisibility paramVisibilityById.set(resolved.paramId, visibility) - if (resolved.config.canonicalParamId) { + // 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) } } From 68c1b10527292a045ef26298f6dfcd2b49146d19 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Sun, 16 Aug 2026 00:30:11 -0700 Subject: [PATCH 3/4] fix(forks): make the post-sync collector honor tool param visibility too Greptile caught a real asymmetry, and corrected a wrong assumption in the first commit: `needsConfiguration` is NOT warning-only. `promote.ts:1035` skips the target's redeploy for any workflow in that list, and `:792` also withholds its chat-deployment carry-over. So with only the pre-sync collector fixed, an Agent block whose Jira `issueKey` was populated in the target and cleared by a credential remap would let the sync through (the modal correctly treats a model-supplied param as non-blocking) and then silently decline to redeploy that workflow - leaving the fork running its previous deployed version with no gate and no error. Both collectors now resolve `required` through one shared `resolveToolParamRequired`, so the pre-sync gate and the promote path cannot disagree about what a nested tool param means. The lookup (sub-block id, then canonical param id, then fail closed to the block-level rule) lives in one place instead of being duplicated. The `@/tools/params` mock in remap-references.test.ts is now overridable so a test can opt into an authoritative resolution; its defaults are unchanged and the other 74 tests pass untouched. The new case is verified to fail without the fix. --- .../lib/mapping/dependent-reconfigs.ts | 18 +---- .../lib/remap/remap-references.test.ts | 71 ++++++++++++++++--- .../lib/remap/remap-references.ts | 29 +++++++- .../workflows/tool-input/param-visibility.ts | 27 +++++++ 4 files changed, 120 insertions(+), 25 deletions(-) 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 49d795c47e6..8982e9b0ef1 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -15,10 +15,7 @@ import { isNonEmptyValue, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' -import { - isSubBlockRequired, - isToolParamUserRequired, -} from '@/lib/workflows/tool-input/param-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' @@ -93,7 +90,7 @@ interface EmitAnchoredParams { * 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 isToolParamUserRequired}). + * 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 @@ -230,16 +227,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { // 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 paramVisibility = paramVisibilityById - ? (paramVisibilityById.get(dependent.id) ?? - (dependent.canonicalParamId - ? paramVisibilityById.get(dependent.canonicalParamId) - : undefined)) - : undefined - const configuredRequired = - paramVisibility !== undefined - ? isToolParamUserRequired({ required: dependent.required, paramVisibility }, values) - : isSubBlockRequired(dependent.required, values) + const configuredRequired = resolveToolParamRequired(dependent, values, paramVisibilityById) out.push({ parentKind: anchor.parentKind, parentSourceId, 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 a9863893eac..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,7 +29,10 @@ import { resolveCanonicalMode, scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' -import { isSubBlockRequired } from '@/lib/workflows/tool-input/param-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' @@ -39,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 @@ -1232,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). @@ -1247,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.ts b/apps/sim/lib/workflows/tool-input/param-visibility.ts index dc7f379b954..22272e692ec 100644 --- a/apps/sim/lib/workflows/tool-input/param-visibility.ts +++ b/apps/sim/lib/workflows/tool-input/param-visibility.ts @@ -42,6 +42,33 @@ export function isUserSuppliedToolParam(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 From dbeba09ebb77b571d08212385353baeada2cd2e0 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Sun, 16 Aug 2026 00:31:38 -0700 Subject: [PATCH 4/4] style(forks): use TSDoc for the new test declaration comments CLAUDE.md requires TSDoc for documentation and no non-TSDoc comments. The vi.mock boundary note, the resolve helper, and the beforeEach mock-reset rationale all document declarations, so doc tooling could not associate them. In-body step comments are left as-is - they explain a flow, not a declaration. --- .../lib/mapping/dependent-reconfigs.test.ts | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) 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 8f88955f75b..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 @@ -7,8 +7,10 @@ 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. +/** + * 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, })) @@ -56,13 +58,17 @@ 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. +/** + * 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([])