Skip to content

V1.5: emit insert_workflow ops from the CLI - #863

Draft
christian-byrne wants to merge 10 commits into
mainfrom
christian-byrne/insert-workflow-op
Draft

christian-byrne wants to merge 10 commits into
mainfrom
christian-byrne/insert-workflow-op

Conversation

@christian-byrne

@christian-byrne christian-byrne commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Draft. Adds comfy workflow insert-workflow <file> <template>, which emits one stamped insert_workflow op without validating, remapping, applying, or writing the workflow locally. Python gates green. Blocked on cmp publish: comfy-multi-player draft PR adding the insert_workflow op.

Design: Notion TDD — agent subgraph lifecycle, V1.5 row. Linear: agent editing subgraph — human tech review needed, frontend agent subgraph follower.

Full context for agent readers

Why

The CLI emits the cmp insert_workflow operation while cmp remains the sole owner of validation, ID remapping, application, replay, and projection.

What changed

  • comfy_cli/command/workflow.py: new standalone insert-workflow command.
  • comfy_cli/command/workflow_edit.py, workflow_ops.py: preserve the input payload verbatim and emit it without mutating or writing the local workflow.
  • comfy_cli/command/error_codes.py: adds cmp rejection codes.
  • docs/op-vocabulary-v1.md: documents the emit-only ownership boundary.
  • tests/comfy_cli/test_insert_workflow_op.py: covers exact payload preservation, semantic pass-through, no local mutation, envelope shape, and standalone-only behavior.

Verification

  • ruff check . and ruff format --check .: pass.
  • Focused tests: 12 passed.
  • Full pytest: 7,407 passed, 32 skipped, 47 unrelated baseline failures.
  • git diff --check: clean.
  • Not run: round trip against a live dochost. Needs the cmp publish and pin bump tracked on the cloud PR.

Decisions to veto

  1. New command name insert-workflow rather than a flag on the existing load command.
  2. The command refuses to emit more than one op.

Ship order

cmp → cloud → comfy-cli (this PR) → frontend follower. Sibling PRs: cloud agent tool draft PR.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Five correctness gaps block safe emission.

Full context for agent readers

Gates: ruff check . passed; ruff format --check passed for all five changed Python/test files; focused insert-workflow tests passed (3 passed); tests/comfy_cli completed with 7,281 passed, 46 failed, 8 skipped. The 46 failures are environmental/pre-existing classes described in the PR scope, not chased. git diff --check passed.

Reviewer probes: op shape matches cmp PR 183 and cloud PR 8981; live-id collisions remap; nested definitions and widget values survive remap; exact replay of the same op is byte-identical. Failures: a dangling endpoint is emitted; empty node/link collections crash after mutation; malformed node/link members escape as AttributeError/IndexError; colliding definitions leave the local file inconsistent with cmp; extra definitions on add_node are accepted.

Could not verify a live doc-host round trip or real applier error rendering because cmp PR 183 is unpublished.

Glossary: cmp is @comfyorg/comfy-multi-player, the authoritative shared-document applier; exact replay means applying the same op_id again.

Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread docs/op-vocabulary-v1.md Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three ownership violations remain: the CLI remaps, validates shared semantics, and applies locally.

Full context for agent readers

Findings

  1. Major, comfy_cli/workflow_ops.py:1392: remap_workflow_ids rewrites caller-owned node/link IDs. The required boundary is emit-only; cmp owns ID handling. Minimal fix: preserve the fragment verbatim in the op payload and remove the remapper from this path.
  2. Major, comfy_cli/workflow_ops.py:1387: the CLI validates dangling endpoints and subgraph-definition semantics. Local malformed JSON/shape checks are appropriate, but cmp owns semantic validation. Minimal fix: limit pre-emission checks to readable JSON, object shape, and required top-level collection types; leave graph and definition semantics to cmp.
  3. Major, comfy_cli/workflow_ops.py:1394: emission calls apply_op, mutates the target workflow, and the command writes it through _finish. This is an applier, not an emitter. Minimal fix: construct and emit exactly one cmp 0.2.1 insert_workflow op without applying or writing a workflow document; update the command arguments/output contract and tests accordingly.

Verification

  • Exact head: fd09a3f9629172ba7b5596823af9098889953436.
  • Full PR diff read: 456 lines; surrounding command, emitter, replay, documentation, tests, and cmp contract in Comfy-Org/comfy-multi-player#183 inspected.
  • ruff check .: passed.
  • ruff format --check .: failed on pre-existing tests/comfy_cli/command/github/test_pr.py (1 file; none of the PR's changed files).
  • pytest: could not collect because the current Python environment lacks project dependencies (typer, semver, blake3, and others): 206 collection errors.
  • git diff origin/main...HEAD --check: passed.
  • Targeted probes covered ID rewriting, local mutation/apply, op key shape, malformed JSON handling, and rejection of definitions on add_node. Runtime import probes were blocked by the same missing dependencies; static inspection confirms the three findings.
  • All five previously resolved threads were inspected. Their narrow fixes are present: empty collections, dangling links, malformed members, definition collisions, and definitions on edit ops. Those fixes do not satisfy the clarified emit-only ownership boundary.

Could not verify

  • Full/focused pytest execution or CLI envelope runtime output because dependencies are absent.
  • Live cmp apply/replay/project integration; cmp change Comfy-Org/comfy-multi-player#183 is not published as 0.2.1 in this checkout.

Glossary: cmp means @comfyorg/comfy-multi-player, the authoritative validator/applier; emit-only means producing an op without changing IDs or workflow state.

@christian-byrne christian-byrne changed the title feat(workflow): add insert-workflow command emitting a stamped insert_workflow op V1.5: emit insert_workflow ops from the CLI Sep 11, 2026

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: the command never sends the op.
P2: malformed fragment envelopes still emit.

Full context for agent readers

The V1.5 contract makes comfy-cli emit-only with cmp owning validate/apply/replay/projection, but emit-only still requires sending the shaped envelope and surfacing cmp/server failures verbatim. This implementation only renders a local CLI result, so no cmp response can occur and the required error-passthrough path is absent. Separately, structural checks are client-owned; a probe showed a fragment containing only definitions is accepted despite missing the required nodes, links, and groups envelope fields. Nested definitions passed through unchanged in the companion probe.

Focused tests: tests/comfy_cli/test_insert_workflow_op.py passed 5/5. Full suite: 7,408 passed, 46 failed, 32 skipped; failures are outside the touched test module and align with the stated baseline class. Ruff lint and format checks passed. Pre-commit's ruff/uv/YAML/TOML gates passed; repository-wide hygiene/pyproject formatting hooks modified unrelated baseline files, which were restored.

Comment thread comfy_cli/command/workflow_edit.py
Comment thread comfy_cli/command/workflow_edit.py

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three contract blockers: no doc-host dispatch, producer IDs stay unremapped, and valid optional groups are rejected.

Full context for agent readers

Reviewed exact head f20eae5b16b07aff26c73346df0af78c4425ade7. Reviewer probes reproduced the missing transport and rejection pass-through path. The cmp contract in PR 183 makes the producer responsible for top-level ID remapping and treats groups as optional. Cloud PR 8981 agrees on the op envelope but currently contradicts cmp about who remaps IDs. Ruff and 127 focused tests pass. Full suite: 46 failed, 7,417 passed, 32 skipped; base: 46 failed, 7,412 passed, 32 skipped, with no PR-attributable increase.

Comment thread comfy_cli/command/workflow_edit.py
Comment thread comfy_cli/workflow_ops.py
Comment thread comfy_cli/workflow_ops.py
Comment thread comfy_cli/command/workflow_edit.py Outdated
Comment thread comfy_cli/command/workflow_edit.py Outdated
Comment thread comfy_cli/command/workflow.py

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One transport blocker. Help and bundled docs drift.

Full context for agent readers

Findings:

  1. Major: the new path renders a local output envelope but never invokes an outbound cmp/server transport. Consequently, the server-error catch cannot surface a real rejection verbatim.
  2. Minor: --stdout/--in-place is accepted but ignored, and its inherited help describes behavior this emit-only command does not provide.
  3. Minor: the bundled agent skill and command-schema discovery map omit insert-workflow and retain an outdated exhaustive list of op emitters.

Verification:

  • Exact head: df7bf3fcc00855a57751bc6b3e995627aa8f0761 from both GitHub and the worktree.
  • uv sync --all-extras: passed.
  • uv run ruff check .: passed.
  • uv run ruff format --check .: passed, 449 files formatted.
  • uv run pytest: 46 failed, 7,417 passed, 32 skipped. The 46 failures match the documented base failure count and are outside the insert-workflow tests.
  • Focused insert/edit tests: 127 passed.
  • uv run pre-commit run --all-files: ruff, uv, YAML, and TOML validation passed; repository-wide EOF, whitespace, and pyproject formatting hooks changed unrelated baseline files. Those changes were restored.
  • git diff origin/main...HEAD --check: passed.

Reviewer probes:

  • Payload fidelity: node IDs, definitions, unknown fields, and null values remain unchanged; the source workflow remains unchanged.
  • Optional fields: omitted links/groups/definitions stay omitted; explicit invalid nodes: null fails with workflow_edit_invalid.
  • Malformed input: outer-shape errors produce a stable error envelope.
  • Help: command is listed, but exposes the ineffective inherited --stdout/--in-place option.
  • JSON output: retains envelope/1, data.op, base_version, and wrote: null with changed: false.

Not verified: live cmp/doc-host round trip, because this implementation has no cmp/server dispatch and the companion cmp change is unpublished.

Glossary: cmp is @comfyorg/comfy-multi-player, the authoritative operation validator and applier; emitter means the CLI produces the operation payload without applying it locally.

@christian-byrne christian-byrne left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two gaps: stdin is rejected; bundled guidance contradicts emit-only validation.

Full context for agent readers

Reviewed exact head ef08532cb45097452285fbb31cff71910c4740b0 against main. The op preserves unusual nested payloads and __* keys exactly, leaves the local workflow unchanged, requires nodes, permits omitted optional collections, and propagates renderer/server errors unchanged with exit 1. --stdout and --in-place are absent from command help and repository references are limited to sibling commands/tests.

Verification: uv sync --all-extras passed; focused insert/edit suite passed (129); Ruff lint and format checks passed; git diff --check passed. Full pytest completed with 7419 passed, 32 skipped, and the expected 46 baseline failures in unrelated run/file-mode/cache/format tests. No typecheck is configured in pyproject.toml. No live dochost round trip was possible. Reviewer probes covered exact unusual-payload passthrough, missing-nodes exit 1, stdin behavior, local-file immutability, and simulated HTTP 422 propagation. Probe artifacts were external/temporary and removed.

Comment thread comfy_cli/command/workflow_edit.py Outdated
Comment thread comfy_cli/skills/comfy/SKILL.md
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