fix(forks): stop sync demanding config the source never had (#6766)

* 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". * 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. * 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. * 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.
2026-09-24 15:45:35 +08:00 · 2026-08-16 04:51:27 -07:00
parent aeb5624cc8
commit ad83796b69
10 changed files with 758 additions and 33 deletions
@@ -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,
@@ -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')
+  })
+})
@@ -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()
+}
@@ -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 (
      <div className='flex h-[30px] items-center gap-2 rounded-lg border border-[var(--border-1)] bg-[var(--surface-5)] px-2 text-[var(--text-muted)] text-small dark:bg-[var(--surface-4)]'>
@@ -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`}
    />
  )
 }
@@ -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<string, WorkflowState>([
+      [
+        '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<string, WorkflowState>([
+      [
+        '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<string, WorkflowState>([
+      [
+        '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<string, WorkflowState>([
+      [
+        '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<string, { data?: Record<string, unknown> }>
+    }
+    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<string, WorkflowState>([
+      [
+        '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<string, string>) =>
+    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<string, WorkflowState>([
+      [
+        '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,
@@ -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<string, ParameterVisibility | undefined>
  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<string, ParameterVisibility | undefined>()
+          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,
          })
        }
@@ -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', () => {
@@ -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<string, unknown>
-): boolean {
-  if (required === true) return true
-  if (!required) return false
-  // The object/function forms are structurally a SubBlockCondition.
-  return evaluateSubBlockCondition(
-    required as Parameters<typeof evaluateSubBlockCondition>[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<string, ParameterVisibility | undefined>()
+    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),
      })
    }
  }
@@ -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)
+  })
+})
@@ -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<SubBlockConfig, 'required' | 'paramVisibility'>,
+  values: Record<string, unknown>
+): 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<SubBlockConfig, 'paramVisibility'>): 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<SubBlockConfig, 'id' | 'required' | 'canonicalParamId'>,
+  values: Record<string, unknown>,
+  paramVisibilityById?: ReadonlyMap<string, ParameterVisibility | undefined>
+): 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<string, unknown>
+): boolean {
+  if (required === true) return true
+  if (!required) return false
+  return evaluateSubBlockCondition(
+    required as Parameters<typeof evaluateSubBlockCondition>[0],
+    values
+  )
+}