Skip to content

feat(cli): bulk writers emit op batches + guarded reset_doc - #715

Merged
skishore23 merged 1 commit into
fix/validate-lowers-ui-to-apifrom
kishore/v1-038-bulk-op-emission
Aug 14, 2026
Merged

skishore23 merged 1 commit into
fix/validate-lowers-ui-to-apifrom
kishore/v1-038-bulk-op-emission

Conversation

@skishore23

Copy link
Copy Markdown
Contributor

Linear: BE-7171 (V1-038) — the comfy-cli half. Cloud half: Comfy-Org/cloud (linked below once open).

Why

comfy templates fetch -o <file> is a bulk writer: it replaces the working file wholesale. Downstream — in the cloud agent's shared document — that replacement could only be expressed one way: re-mint the whole document from the new file. That is the gap this closes.

Re-minting is bad for two reasons:

  1. It throws away the attributed, incremental op history the document exists to keep. A template fetch appears as "the document is now something else", with no author, no ops, nothing to replay.
  2. op-vocabulary-v1 §8.6 is explicit that independently re-seeding a base is the one thing a replica must never do — the duplicate identities are invisible until the first merge.

What

templates fetch --emit-ops

Emits data.ops: the stamped op batch that turns the file being replaced INTO the template.

delete_node × (nodes currently in the target file)
add_node    × (template nodes)
connect     × (template links)

No set_widget ops — widget values ride inside the add_node payload, which §8.5 makes authoritative at replay. New flags: --emit-ops, --actor, --base-version.

Two contracts, both tested.

consumer guarantee
op contract apply_op replay (what the cloud forwards to the doc host) reproduces the template exactly — classes, wiring, positions, widget values
spec contract apply_specs (comfy workflow apply --ops) accepts the array verbatim, rebuilds the structure — the same bar nodes path --emit-ops already meets

Each entry is dual-shape to satisfy both: a fully minted op (envelope + minted fields) that also carries that kind's spec keys (class_type/at/as, from/to, node). The two are not equivalent and the docstring says so: apply_specs re-mints each node from the live catalog, so it reproduces structure; the op path reproduces the graph.

Identity is re-minted, never inherited. Template graphs are numbered from small frontend counters (1, 2, 3…). Replaying those ids into a live document reuses identities a concurrent replica may still hold — §1.5's "a merge could resurrect a deleted node" hazard. Every node and link gets a fresh mint_id; every interior reference (inputs[].link, outputs[].links, the links tuples) is remapped onto it.

All or nothing. A graph the frozen vocabulary cannot express — a subgraph definition, a canvas group, a reroute point, a malformed node/link — emits no ops at all and reports ops_skipped: "<reason>". A partial batch is the dangerous answer: it applies cleanly and leaves a document that is not the graph the caller asked for. The consumer keeps its whole-document fallback for exactly these.

Without the flag the envelope is byte-identical to before (asserted).

reset_doc — un-deferred and guarded

reset_doc was frozen in §1.6 but left deferred (apply_op rejected it; DEFERRED_OPS pinned that). It is now implemented:

  • comfy workflow reset-doc <file> --confirm. Without --confirm the command fails closed with workflow_reset_doc_unconfirmed and writes nothing. The check runs before the file is read, so an unconfirmed call cannot fail halfway. It is the only guarded edit command — because it is the only one no later op can undo.
  • A history barrier, not a clear. clear empties the graph but preserves the id high-water marks and _applied_ops, so it merges like any other edit. reset_doc drops those too, plus _widget_stamps; only the document id survives. Safe because ids come from mint_id, never from the high-water marks (§8.3).
  • Idempotent. The reset's own op_id is written into the freshly-emptied _applied_ops, so a re-delivered reset_doc is a no-op rather than a second wipe. (apply_op now re-reads workflow["_applied_ops"] instead of appending to the pre-dispatch binding — that binding is stale precisely for this kind.)
  • Standalone-only, with its own registered code workflow_reset_doc_not_batchable. NotBatchableError is now per-kind; the class attributes keep the clear values so existing callers reading them off the class still resolve.
  • Never emitted implicitly — no --emit-ops surface and no bulk writer mints one. Replacing a canvas is deletes+adds, which merge; a barrier does not.

DEFERRED_OPS is now empty.

Doc

Per §9's amendment rule, docs/op-vocabulary-v1.md gains amendment v1.1 (§10) and a new normative §8.8 (bulk writers emit ops, they do not re-seed). §1.6 is rewritten from "semantics when implemented" to the implemented contract. No frozen kind was added, removed, or re-scoped — reset_doc was already in FROZEN_OPS and already Batchable = no.

Test evidence

Red first, then green. New files:

  • tests/comfy_cli/command/test_templates_fetch_emit_ops.py — test_template_fetch_emits_op_batch (stamping, ordering, exact op-replay round-trip, minted-id assertion, the written file is unchanged), test_emitted_batch_round_trips_through_apply_specs, test_without_the_flag_the_envelope_is_unchanged, test_emit_ops_on_a_fresh_canvas_has_no_deletes, test_inexpressible_template_emits_no_ops_and_says_why, test_emit_ops_without_out_still_emits.
  • tests/comfy_cli/test_reset_doc_op.py — test_reset_doc_requires_confirm (envelope code + the file is byte-for-byte untouched), test_reset_doc_with_confirm_empties_the_document, test_reset_doc_rejected_in_batch, test_reset_doc_rejected_through_the_apply_command, test_apply_op_replays_reset_doc_and_records_it, test_reset_doc_is_no_longer_deferred.
$ uv run pytest tests/comfy_cli/test_reset_doc_op.py \
    tests/comfy_cli/command/test_templates_fetch_emit_ops.py
11 failed, 1 passed          # before implementation

$ uv run pytest tests/comfy_cli -k "workflow or template or op_vocab or reset_doc or error_code"
770 passed, 2 skipped, 3830 deselected

The doc↔code contract test tests/comfy_cli/test_op_vocabulary_contract.py is green unchanged — it discovers apply_op / apply_specs dispatch behaviorally, so un-deferring reset_doc had to move the doc table, the constants and both dispatch tables together or it would have failed.

ruff check / ruff format --check clean on every touched file (the 17 repo-wide ruff check findings are pre-existing in untouched files). Full suite: 31 failures, of which 29 reproduce on the base branch with this diff stashed (test_logs.py, test_jobs.py, test_host_port.py, test_onboarding.py, test_run_json.py — environment-dependent) and the remaining 2 were introduced-and-fixed here (test_every_registered_code_is_raised needed the new code to be discoverable by the AST scanner — _NOT_BATCHABLE values are dicts with a "code" key for that reason).

Base caveat

Targets kishore/v1-p2-integration, not master — it builds on the frozen-vocabulary work that lands there. Rebase onto master once the P2 integration branch merges; nothing here depends on P2 beyond the vocabulary doc and FROZEN_OPS/DEFERRED_OPS constants.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7e4cb2bc-0845-49c4-ac1c-9bb2acbabb32

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.

`templates fetch -o <file>` is a BULK WRITER: it replaces the working file
wholesale. Downstream, that replacement could only be expressed as a new
document — the consumer re-minted a snapshot from the new file, throwing away
the attributed op history and doing precisely what op-vocabulary-v1 §8.6 says a
replica must never do (independently re-seeding a base duplicates identities,
silently, on the first merge).

`--emit-ops` closes that. The fetch also emits `data.ops`: the stamped op batch
that turns the file being replaced INTO the template — delete_node for what was
there, then add_node + connect for the template — in the frozen vocabulary.

Two contracts, both tested:
  * the OP contract: replaying the batch with apply_op reproduces the template
    exactly, widget values included (they ride inside the add_node payload,
    which §8.5 makes authoritative);
  * the SPEC contract: the same array is accepted by apply_specs verbatim, so
    it is a legal `comfy workflow apply --ops` batch — the same bar
    `nodes path --emit-ops` already meets.

Each entry is dual-shape to satisfy both: a fully minted op AND that kind's
spec keys. Identity is re-minted, never inherited — a template's small counter
ids would resurrect identities a concurrent replica may still hold.

A graph the vocabulary cannot express (subgraph definition, canvas group,
reroute) emits NO ops and says why in `ops_skipped`. A partial batch is the
dangerous answer: it applies cleanly and leaves a document that is not the
graph the caller asked for.

Also un-defers `reset_doc` (§1.6), which was frozen-but-rejected:
  * `comfy workflow reset-doc <file> --confirm` — fails closed without
    --confirm, before the file is read, so an unconfirmed call cannot fail
    halfway. The only guarded edit command, because it is the only one no
    later op can undo;
  * a history barrier, not a clear: it drops the id high-water marks,
    `_applied_ops` and `_widget_stamps`. The reset's own op_id is written into
    the freshly-emptied list, so a re-delivery is a no-op, not a second wipe;
  * standalone-only, with its own registered code;
  * never emitted by --emit-ops or by a bulk writer.

DEFERRED_OPS is now empty. docs/op-vocabulary-v1.md carries amendment v1.1
(§10) plus a new normative §8.8 for bulk writers, per the §9 amendment rule.

Tests: tests/comfy_cli/test_reset_doc_op.py,
tests/comfy_cli/command/test_templates_fetch_emit_ops.py. The doc↔code
contract test (test_op_vocabulary_contract.py) stays green unchanged.

Linear: BE-7171 (V1-038)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@skishore23
skishore23 force-pushed the kishore/v1-038-bulk-op-emission branch from 28a204a to a297c6d Compare August 13, 2026 21:28
@skishore23
skishore23 changed the base branch from kishore/v1-p2-integration to kishore/v1-001-op-vocab-freeze August 13, 2026 21:28
Base automatically changed from kishore/v1-001-op-vocab-freeze to fix/validate-lowers-ui-to-api August 14, 2026 19:32
@skishore23
skishore23 merged commit a297c6d into fix/validate-lowers-ui-to-api Aug 14, 2026
5 checks passed
skishore23 added a commit that referenced this pull request Aug 14, 2026
Brings four independent PR chains, all rooted on this branch, plus 32
commits of main drift.

Chains (tip merges bring the whole chain):
  #704 op-vocab-freeze -> #715 bulk-op-emission -> #718 connect-LWW
  #705 --select -> #714 widget-catalog
  #706 --ack
  #707 expand-top -> #708 templates-get -> #709 path --emit-ops -> #710 delete-nodes

All four merged with zero textual conflicts and zero test regressions.
Op vocabulary doc is coherent at v1.2 (DEFERRED_OPS empty, reset_doc
promoted in v1.1, concrete-input LWW in v1.2).

origin/main conflicted in 8 files / 21 hunks. Five were not either/or --
both sides had added different things at the same spot, so keeping one
side would have silently deleted the other's work:

  error_codes.py   both new codes (expand_miss + path_bounds_invalid)
  nodes.py         both dict keys (ops + support)
  loader.py        main: kept `import time`, dropped urllib (dead after _net.py)
  registry/api.py  both: 511's lazy `import requests` + main's timeout=
                   DEFAULT_HTTP_TIMEOUT (main had already timed out the
                   other three call sites in this file)
  preview.py       main's CWD binary-planting guard + ffprobe/render
                   timeouts, with 511's ffprobe-optional and
                   imageio-ffmpeg fallbacks restored inside it. Both
                   fallbacks fire only on a genuinely ABSENT binary; an
                   untrusted PATH match is still refused, so the planting
                   guard is intact.
  test_preview.py  both test bodies
  cmdline.py       main's deprecation of `comfy validate` into the shared
                   validate_api_workflow, plus 511-only api_node_id
                   (57:3 -> 57/3 subgraph remap) ported into that helper,
                   which main lacked
  cql/engine.py    hunk-by-hunk. None of 511's ten engine commits are in
                   main, so taking main's file would have deleted the
                   upload-backed enum fix, the MATCHTYPE wildcard, the
                   reachability check and the SHA-256 subgraph fork id.
                   Took main's dynamic-combo machinery (#574 / BE-3371,
                   a superset) and its 6-tuple _parse_input_spec arity;
                   restored Graph.widget_defaults and 511's structural
                   {key,inputs} detection at the new arity.

widgets_values guard: main's new code reads `node.get("widgets_values")
or []` and then indexes positionally. A dict is truthy, so VHS_* nodes
(which serialize widgets_values as a named dict) would raise KeyError: 0
again -- the defect behind 38 failures / 24% of in-scope in one 24h
window, surfacing as "Could not extract slots: 0". Every read site now
goes through _widgets_as_list, including two on main's NON-conflicting
lines that the merge never flagged.

Test tuning: two @patch targets in test_utils/test_standalone pointed at
comfy_cli.utils.requests, which does not exist under the lazy import
(verified: importing comfy_cli.utils does not load requests). Repointed
to "requests.get" -- the pattern the same file already used elsewhere.
Behaviour is unchanged; the timeout main added is still asserted.

KNOWN FAILING (10) -- two clusters, both design decisions, not splices:

 1. Dynamic-combo widget order (8). Two implementations of one feature.
    511 expands sub-widgets in the static widget_order; main moved
    expansion into the value-aware _expand_widget_entries/_WidgetEntry.
    Tests from 511, main AND #714 each assert their own model, so no
    single side passes all three. Needs a call on which model wins.
 2. run cloud lifecycle (2, plus test_run_prompt). The tests invoke a
    nonexistent wf.json expecting the mocked execute_cloud to
    short-circuit; a workflow_not_found check now fires first.

Baselines measured, not assumed: 511 = 29 failed / 4603 passed;
origin/main = 29 failed / 4617 passed. Lint is 19 errors, all UP038
style, zero F-class -- unchanged in kind from both baselines.
@skishore23
skishore23 deleted the kishore/v1-038-bulk-op-emission branch August 14, 2026 19:32
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant