Comfy-Org / Comfy-Org/ComfyUI_frontend

convertToSubgraph registers corrupt link (target_slot -1, type undefined) when boundary input type changed after connect

Open
#15,732 2 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
area:subgraph Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`convertToSubgraph` registers a corrupt link (`target_slot: -1`, `type: undefined`) into the enclosing subgraph when a boundary input's slot type changed after the link was created. Demonstrated by unit test on `feature/ecs-migration` @ f1bfb313d6; the same code path exists on `main` (`LGraph.ts:1876`), so this is pre-existing, not a #14246 regression.

## Chain

1. `LGraph._convertToSubgraphImpl` reconnects boundary input links whose origin is `SUBGRAPH_INPUT_ID` via `subgraphNode.findInputSlotByType(link.type, true, true)` (branch `LGraph.ts:2060`, main `LGraph.ts:1876`).
2. `findInputSlotByType` returns `-1` when nothing matches. The `` overload declares plain `INodeInputSlot`, hiding the `-1` from the type checker.
3. `SubgraphInput.connect(slot, node)` has no membership guard: `node.inputs.indexOf(-1) === -1` and `(-1).type === undefined` flow straight into `new LLink(id, undefined, ..., -1)`, which is then registered (`SubgraphInput.ts:59/85`, main `:57`).

## Why the lookup misses

`mapSubgraphInputsAndLinks` derives the new subgraph node's input type from the target input slot's CURRENT type (`subgraphUtils.ts:356`), while the reconnect searches by the boundary link's FROZEN type. Any post-connect slot-type mutation makes them diverge. `_findSlotByType` normalization makes it worse: a `'*'` slot becomes `destTypes ['0']`, so the `dest === '*'` match arm is dead code and a wildcard input never matches a concrete search type.

## Repro (vitest, passes on f1bfb313d6)

```ts
const subgraph = createTestSubgraph({ inputs: [{ name: 'value', type: 'number' }] })
onTestFinished(enableSubgraphNodeCreation(subgraph.rootGraph))
const target = createTestNode(subgraph, ['number'])
subgraph.inputNode.slots[0].connect(target.inputs[0], target)

target.inputs[0].type = '*' // runtime type swap after connect (see #15579 mechanism)

const { node: nested } = subgraph.convertToSubgraph(new Set([target]))

const links = [...subgraph.links.values()].filter(l => l.origin_id === SUBGRAPH_INPUT_ID)
const broken = links.find(l => l.target_slot === -1)
// all pass:
expect(broken).toBeDefined()
expect(broken?.type).toBeUndefined()
expect(broken?.target_id).toBe(nested.id)
expect(nested.inputs[0].link).toBeNull() // nested node: unconnected
expect(subgraph.inputs[0].linkIds).toContain(broken!.id) // enclosing input: connected
```

Control arm (no type swap) reconnects correctly with `target_slot: 0`, `type: 'number'`, so the harness exercises the same branch.

## Consequence

Asymmetric topology: the enclosing subgraph input holds a link id whose target slot does not exist, while the nested node's real input is silently disconnected. The link serializes with `type: undefined` and `target_slot: -1`.

## Same pattern, no demonstrated trigger yet

`NodeInputSlot.ts:123-129` / `NodeOutputSlot.ts:168-173` (link getters at index `-1` return null/empty) and `LinkConnector`/`FloatingRenderLink` movement methods (slot captured at drag start, no membership check at drop) share the unguarded `indexOf` pattern; a 36-site audit found no other first-party caller that passes a detached slot. Guarding `SubgraphInput.connect` (reject `inputIndex === -1`) covers the demonstrated trigger; `SubgraphOutput.ts:45-47` already does exactly this on the output side.

## Working notes

- Repro test file (control + probe): drop-in at `src/lib/litegraph/src/subgraph/SubgraphConversion.detachedSlotProbe.test.ts`, uses existing `subgraphHelpers` fixtures only.
- The fix has a design choice: guard in `SubgraphInput.connect` (mirror `SubgraphOutput`), or fix the lookup at the `convertToSubgraph` call site (search by current input type, or use the positional `i - 1` already computed two lines above), or both.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.