Skip to content

feat: add the set_node_field op and its command - #911

Draft
claude[bot] wants to merge 2 commits into
mainfrom
prototype/set-node-field-command
Draft

claude[bot] wants to merge 2 commits into
mainfrom
prototype/set-node-field-command

Conversation

@claude

@claude claude Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Requested by Christian Byrne · Slack thread

Prototype / spike — not intended to merge as-is. One of four PRs in a cross-repo prototype (comfy-multi-player, ComfyUI_frontend, this, cloud).

Before

There is no way to rename a node, mute one, collapse one or pin one through the op vocabulary. The closest thing is an add_node upsert, which is a legitimate LWW replace — but of the whole node: it rewrites the node's widget values and resets its widget stamps, so a field change concurrent with a set_widget on the same node discards that write.

That is the gap ComfyUI_frontend hit trying to sync flags.collapsed / flags.pinned out of the canvas, and it is why the in-app agent can build a graph but cannot label one.

After

comfy workflow set-node-field <file> <node_id> <field> <value>
Writable fields title, mode, flags.collapsed, flags.pinned — exported as WRITABLE_NODE_FIELDS
Write target ("node_field", node_id, field) — one LWW register per (node, field)
value of null clears the field, so a flag returns to absent the way workflow JSON round-trips it
Batchable yes — it rides inside workflow apply / foreach

Two collaborators can now edit two fields of one node, or a field and a widget, without contending — and detect_conflict agrees, so V0's ask-to-merge does not raise on a pair that cannot actually clobber.

Why the field list is closed

Everything outside it either has an op of its own (widgets_values is set_widget's; inputs/outputs belong to connect) or is node identity (id, type) that only add_node/delete_node may move. flags as a whole is excluded too — a whole-object write would reintroduce exactly the clobber this op exists to avoid.

Node ids compare as strings throughout (amendment v1.2), since the CLI takes this one straight off the command line.

Red → green

Commit What it proves
f9102ca test: 10 cases: the command, replay through apply_op, per-field LWW independence, either-order convergence, idempotency, the field allowlist and batchability. All 10 fail — No such command 'set-node-field' and unknown op 'set_node_field'.
75fd565 feat: Same 10, green.

test_op_vocabulary_contract.py passes, which is the point of it: the doc's frozen-kinds table, FROZEN_OPS, BATCHABLE_OPS, and both dispatch tables are pinned to agree, so the docs/op-vocabulary-v1.md row in this PR is not decoration — the contract test would fail without it. ruff check and ruff format --check clean.

Cross-repo notes

  • The vocabulary doc is the upstream source of truth. comfy-multi-player pins docs/op-vocabulary-v1.md by SHA in its upstream-pins.json, so its companion PR's set_node_field needs the pin re-established against this change once it lands. Its check:pins passes today only because it cites no new section.
  • FROZEN_OPS here and in cmp had already drifted before this PR — cmp carries disconnect and define_subgraph, this does not. Worth a separate look; not touched here.
  • The cloud agent tool shells out to comfy workflow set-node-field, so it pins against this branch rather than waiting for a merge.

Not done here

  • No amendment section written into docs/op-vocabulary-v1.md beyond the frozen-kinds row. A real landing wants the versioned amendment prose the file's own header asks for.
  • Subgraph-interior nodes are out of scope: the write is top-level only.

🤖 Generated with Claude Code

https://claude.ai/code/session_016jorVkGmycSkD6j7A6sdJV


Generated by Claude Code

The vocabulary can only express a node scalar-field change as an add_node
upsert, which replaces the whole node and clears its widget stamps. These
cases pin what a field-addressed register must do: a standalone command,
replay through apply_op, per-field LWW, a closed field allowlist, and
batchability.
comfy workflow set-node-field <file> <node> <field> <value> writes one
durable node field — title, mode, flags.collapsed or flags.pinned — and
emits a replayable op. One LWW register per (node, field), so a title
write and a flag write on one node never contend, and neither disturbs
that node's widget registers.

Writable fields are a closed set: everything outside it either has an op
of its own or is node identity that only add_node and delete_node move.
null clears a field, so a flag returns to absent the way workflow JSON
round-trips it.
@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 3ee90c94-3ddd-41c1-9e2f-efa246ad8afa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Run Tests on GPU Runners (linux) is red here, but it is pre-existing and unrelated to this PR — it is red on main too.

Evidence. Of the last 20 runs of .github/workflows/run-on-gpu.yml across all branches, 18 failed and 0 passed, including three consecutive main runs — one of them aec5220c, which is the exact commit this branch is based on:

failure | main | 2026-09-21T05:04:21Z | aec5220c   <- this branch's merge base
failure | main | 2026-09-21T04:51:03Z | 6e76507a
failure | main | 2026-09-21T04:19:45Z | 797af2da

What's actually failing — two E2E cases in tests/e2e/test_e2e_uv_compile.py, both asserting 'Conflicting packages (by node pack):' in <output>:

  • test_progressive_conflict
  • test_node_uv_sync_standalone_conflict

The resolver never reaches the conflict report, because it aborts earlier on a VCS URL pulled from a live third-party node pack:

[UnifiedDepResolver] rejected path separator:
  'git+https://github.com/facebookresearch/sam2'
  from .../custom_nodes/comfyui-impact-pack
Resolution failed: compile failed: error: No solution found when resolving dependencies

So these tests clone comfyui-impact-pack and nodepack-test2-do-not-install from GitHub at run time and assert on the resolver's output. An upstream change to one of those packs' requirements changed the failure mode out from under the assertion. That is an external-dependency breakage in the uv-compile path, not a regression.

Why it can't be this PR. The diff is five files — workflow_ops.py, command/workflow_edit.py, command/workflow.py, docs/op-vocabulary-v1.md, and one new test file. None of them is in the node-pack dependency-resolution path these tests exercise.

No re-run. The drive-to-green convention is to retry an unrelated failure once, but this is not an intermittent flake — it fails deterministically on main and on every other branch, so a retry would just spend a GPU runner reproducing a known break. Worth its own ticket against the uv-compile E2E suite (pinning those node packs, or teaching the resolver to skip a VCS requirement rather than abort the whole compile) rather than being absorbed into this PR.

Everything else on this PR is green: ruff_check, CodeQL, both test jobs, hygiene, AGENTS.md integrity, cla-assistant, Socket Security.

Created by Claude Tag

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant