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.

Author: mzxchandra

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')
+  })
+})

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()
+}

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 (

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<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 = {
@@ -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<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', () => {

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<string, ParameterVisibility | undefined>
   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)
             }
           }