Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: adopt a rule that a compatibility accessor is fully inert or fully honoured, never half (12 half-honoured found, 7 branch-introduced)
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 702
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
**Proposed rule: a compatibility accessor over store-derived state must be fully inert or fully honoured, never half.**
Fully inert (every write ignored) is safe — a legacy caller's mutation becomes a no-op and nothing is lost. Fully honoured (every write commits) is correct. **Half-honoured — one direction commits, the other is silently discarded — is the only variant that destroys data**, because it turns a legacy *move* idiom into a *delete*.
This came out of auditing every mirror and compat accessor on `feature/ecs-migration` (`5002fae1b12d44831a21367afa7c0f798f7e7a2c`, merge base `6532665db947acb61ed044fe91a1d4fe1fb84c8b`). It is a class, not a bug.
## What the audit found
**12 half-honoured accessors, 7 of them branch-introduced.** Against 13 fully honoured, 5 fully inert, and the rest plain fields or read-only derivations.
| Accessor | file:line | Asymmetry | Issue |
| --- | --- | --- | --- |
| `NodeOutputSlot.links` | `node/NodeOutputSlot.ts:36,44` | removals disconnect, additions discarded | #15620 |
| `NodeInputSlot.link` | `node/NodeInputSlot.ts:28,35` | `null` disconnects, an id is discarded | #15620 (same family) |
| `LGraphNode.widgets` | `node/widgetsView.ts:35` | removals land, additions land only if store-registered | #15630 |
| `LLink.origin_id/origin_slot/target_id/target_slot` | `LLink.ts:160-193` | rejection is `console.error` only; two-statement retarget half-applies | #15629 |
| `LGraph.reroutes` | `LGraph.ts:463` | `.set` loses chain state, `.delete` poisons the id | #15633 |
| `BaseWidget.options` | `widgets/BaseWidget.ts:81` | in-place honoured, whole assignment discarded | #15632 |
| `options.values` via `bindDynamicValuesOption` | `composables/widgets/useComboWidget.ts:58` | discarded while `getValues()` is live | pre-existing |
| `BaseDOMWidgetImpl.value`, `DOMWidgetImpl.value` | `scripts/domWidget.ts:143,381` | bypasses `_state` | pre-existing |
Plus an adjacent hazard that is not an accessor but shares the failure mode: `EMPTY_MEMBERSHIP` is a shared mutable singleton whose corruption reaches the saved workflow (#15631).
## Why "inert" is not enough on its own, and why that is fine
The tempting response is "make them all inert". For `output.links` that is nearly right — but note the current removal behaviour is load-bearing for at least one real idiom (`output.links = output.links.filter(...)` to disconnect), and `src/lib/litegraph/src/node/legacySlotLinkMutations.test.ts:57-70` pins it deliberately. So the choice is per-accessor. What the rule forbids is *not choosing* — shipping a surface where the answer differs by direction.
## The documentation is already split on this
Eight documents describe the contract. Seven say writes are ignored:
- `docs/adr/0008-entity-component-system.md:197-198` — "read-only compatibility accessors derived from the store; **assignments are ignored**"
- `docs/architecture/ecs/ecs-migration-plan.md:45`
- `docs/architecture/ecs/ecs-documentation-audit.md:73`
- `docs/architecture/ecs/ecs-state-authority-audit.md:146`
- `docs/architecture/ecs/ecs-decision-traceability.md:64`
- `docs/architecture/node-data-store.md:214-217` (true mechanically, omits the precondition that is the failure mode)
- `docs/architecture/output-slot-connectivity.md:143-147` — additionally claims the getter returns a **fresh** array, which the branch's own test refutes: `expect(slot.links).toBe(links)` at `src/lib/litegraph/src/node/NodeOutputSlot.test.ts:97`
One is right:
- `docs/architecture/ecs/ecs-extension-compatibility-audit.md:50-51` — "Assigning `null` disconnects; ID assignment is ignored" / "Removing IDs disconnects; additions are discarded"
So this is not ignorance. The ADR and four audit docs describe a design the compatibility table contradicts on the same branch, and **they are wrong in the permissive direction** — a reader consulting ADR 0008 to decide whether `output.links = filtered` is safe concludes it is a no-op. It deletes links. That is how this class gets reintroduced by the next person. Tracked as #15618; this adds the eighth document.
## Proposed enforcement
1. **Decide and record.** Each compat accessor gets an explicit inert-or-honoured decision in ADR 0008. There is currently **no** decision record explaining why half was chosen, even though the branch carries honest `it.fails` coverage (`node/legacySlotLinkMutations.test.ts:110-160`) showing the behaviour is deliberate.
2. **Make the type say so.** `INodeInputSlot.link` and `INodeOutputSlot.links` are declared **mutable** (`src/lib/litegraph/src/interfaces.ts:367-373, 389-395`) despite `output-slot-connectivity.md:150` claiming they are `@deprecated readonly`. Adding `readonly` costs nothing and stops first-party and typed-extension misuse. It does not help plain-JS packs, which is where the measured hits are.
3. **Never swallow a rejection.** `applyEndpointPatch` (`LLink.ts:115-117`) and `replaceNodeInputs` (`node/slotLinks.ts:185-188`) both `console.error` and return as if successful. A rejected write should throw or return a result the caller must handle.
4. **Test both directions for every accessor.** The existing `it.fails` pattern is the right one — it asserts the wanted behaviour and goes green when someone fixes it, rather than fossilising the current wrong answer. Extend it to the accessors that have no such coverage.
5. **Prefer the `LinkMap` pattern.** `graph.links` (`LinkMap` + `MapProxyHandler`, `LGraph.ts:519-534`) makes every mutation form commit and is the cleanest fully-honoured surface on the branch. It is the model to copy.
## Reachability, for calibration
All measurements over 29 custom-node packs on one host, 157 frontend source files (`.js`/`.ts` under `*/web/*`, excluding minified bundles, `litegraph.d.ts` and `node_modules`), control `/registerExtension/` matching **84 files** on every run.
Four idioms measured a **real zero against that live control** — genuinely unused by the corpus, not unmeasured: `inputs[..].link =`, `_floatingLinks`, `badgePosition`, `.reroutes.set|delete|clear`. The rest are reachable, and the named paths are in the per-accessor issues.
Related: #15620, #15618, #15577, #15594, #15600, #15623.
Contributor guide
Assessment
This issue has not been assessed yet.