Skip to content

feat(cli): --ack summary|full on workflow apply (default full; pinned summary shape) - #706

Merged
skishore23 merged 1 commit into
fix/validate-lowers-ui-to-apifrom
kishore/v1-012-ack
Aug 14, 2026
Merged

feat(cli): --ack summary|full on workflow apply (default full; pinned summary shape)#706
skishore23 merged 1 commit into
fix/validate-lowers-ui-to-apifrom
kishore/v1-012-ack

Conversation

@skishore23

Copy link
Copy Markdown
Contributor

Implements Linear BE-7149 (ticket V1-012).

comfy workflow apply echoes the full ops array back in the success envelope. The sender already has the batch it sent, so for a large batch the echo is pure transcript weight. This adds --ack summary|full to apply.

Pinned summary shape

--ack summary returns exactly (field names feed the cloud field-contract manifest — add/rename only with a contract bump on that side):

{
  "count": 4,
  "ops_by_kind": {"add_node": 2, "connect": 1, "set_widget": 1},
  "nodes_added": [3504687186153845, 1916471524889913],
  "nodes_deleted": [],
  "aliases": {"ckpt": 3504687186153845, "pos": 1916471524889913},
  "base_version": 0,
  "version": 4,
  "changed": true
}

No ops echo. changed and version bookkeeping (base_version, version = base_version + len(ops)) are computed identically in both modes; the envelope-level changed flag is unchanged.

Live smoke (temp project, offline catalog):

$ comfy --json workflow apply wf.json --ops ops.json --ack summary --input sd15_object_info.json
{"schema": "envelope/1", "type": "envelope", "ok": true, "command": "workflow apply", "version": "0.0.0", "where": null, "data": {"count": 4, "ops_by_kind": {"add_node": 2, "connect": 1, "set_widget": 1}, "nodes_added": [3504687186153845, 1916471524889913], "nodes_deleted": [], "aliases": {"ckpt": 3504687186153845, "pos": 1916471524889913}, "base_version": 0, "version": 4, "changed": true}, "error": null, "changed": true}

Pretty mode renders the summary as per-kind counts + alias lines under the existing ✓ applied N edit(s) line.

Default unchanged (proof)

The default (no flag) payload is byte-identical to today's, and --ack full is an explicit synonym: test_apply_ack_default_byte_identical pins the only nondeterminism in the apply path (node ids via mint_id/random.getrandbits, op ids via uuid.uuid4; layout is documented "no randomness, no clock", the CRDT stamp is [base_version, actor]) to counters, runs the same batch on the same file path with and without --ack full (resetting file content + counters in between), and byte-compares the envelope lines. It additionally pins the default payload to today's exact key set ({workflow, count, ops, aliases, base_version, version, wrote}) with the full per-op echo.

Partial-failure semantics

The batch stays atomic (op k of n fails → abort, nothing written). --ack never changes the outcome: both modes keep error.code = workflow_edit_invalid and exit 1. Summary mode only ADDS a structured receipt to error.details:

{"failed": {"index": 1, "op": "add_node", "code": "workflow_edit_invalid"}, "applied_count": 1}

failed.index is 0-based (matching the existing spec #N message convention); applied_count counts specs applied before the abort — all discarded, per the atomic batch. Default mode's failure envelope is unchanged (details: null, pinned by test_full_mode_failure_envelope_unchanged). To carry the failing spec's position out of workflow_ops.apply_specs, the batch-level wrap now stamps spec_index/spec_op/applied_count attributes on the wrapped ValueError (message and type unchanged).

foreach deliberately takes no --ack: its payload never echoed ops ({recipe, count, out_dir, written}), so there is nothing to summarize.

Sibling-PR rebase notes

Two sibling drafts sit on this same base branch:

Test evidence

New tests/comfy_cli/command/test_ack_flag.py, written red-first (6 failed / 3 baseline-passed before the implementation; 9 passed after):

  • test_apply_ops_ack_summary_returns_counts_and_aliases — 3-op batch with as: aliases → exact pinned key set, "ops" not in data
  • test_ack_summary_reports_deleted_nodesnodes_deleted carries real ids
  • test_ack_rejects_unknown_value--ack bogusworkflow_edit_invalid
  • test_apply_ack_default_byte_identical — see proof above
  • test_ack_summary_partial_failure_reports_index_and_code — op 2 of 3 invalid → failed == {index: 1, op: "add_node", code: "workflow_edit_invalid"}, applied_count == 1, file untouched
  • test_full_mode_failure_envelope_unchanged
  • test_single_edit_verbs_unaffected + test_single_edit_verbs_do_not_take_ack
  • test_pretty_summary_renders_counts_and_aliases

Verification run:

  • uv run pytest tests/comfy_cli/command/test_workflow_edit.py tests/comfy_cli/command/test_ack_flag.py tests/comfy_cli/command/test_workflow_apply_rollback.py tests/comfy_cli/output/test_error_code_registry.py -q115 passed (includes the error-code registry gate)
  • Full suite: 4584 passed, 29 failed, 38 skipped — all 29 failures (test_logs/test_jobs/test_onboarding/test_host_port/restore_snapshot) reproduce identically at the base commit with this change stashed; pre-existing on fix/validate-lowers-ui-to-api, unrelated.
  • ruff check + ruff format --check clean on the three changed files.

🤖 Generated with Claude Code

… summary shape)

`comfy workflow apply` echoes the FULL ops array back in the success
envelope — for a big batch the sender gets its own batch mirrored back and
learns nothing new. Add `--ack summary|full`:

* default stays `full` and is byte-identical to today's payload (proved by
  a determinism-pinned byte-compare test); `--ack full` is an explicit
  synonym for the default.
* `--ack summary` returns a compact receipt with a PINNED shape (field
  names feed the cloud field-contract manifest):
  {count, ops_by_kind, nodes_added, nodes_deleted, aliases, base_version,
   version, changed} — no ops echo.
* on batch failure, summary mode keeps the exact full-mode outcome
  (code=workflow_edit_invalid, exit 1, atomic no-write) and ADDS
  error.details = {failed: {index, op, code}, applied_count}; apply_specs
  now stamps the wrapped batch error with the failing spec's position.
* pretty mode renders the summary as per-kind counts + alias lines.

`foreach` takes no --ack: its payload never echoed ops.

Implements Linear BE-7149 (ticket V1-012).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@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: fef3449c-62c1-414d-8fa5-cbf4d8226a0e

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.

@skishore23
skishore23 merged commit 6aa2846 into fix/validate-lowers-ui-to-api Aug 14, 2026
5 checks passed
@skishore23
skishore23 deleted the kishore/v1-012-ack branch August 14, 2026 19:32
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.
@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