V1.5: emit insert_workflow ops from the CLI - #863
christian-byrne wants to merge 10 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
christian-byrne
left a comment
There was a problem hiding this comment.
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.
Addresses review feedback: #863 (comment) #863 (comment) #863 (comment) #863 (comment) #863 (comment)
christian-byrne
left a comment
There was a problem hiding this comment.
Three ownership violations remain: the CLI remaps, validates shared semantics, and applies locally.
Full context for agent readers
Findings
- Major,
comfy_cli/workflow_ops.py:1392:remap_workflow_idsrewrites 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. - 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. - Major,
comfy_cli/workflow_ops.py:1394: emission callsapply_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.1insert_workflowop 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-existingtests/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
definitionsonadd_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
left a comment
There was a problem hiding this comment.
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.
Addresses review feedback: #863 (comment)
Addresses review feedback: #863 (comment)
There was a problem hiding this comment.
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.
Addresses review feedback: #863 (comment)
Addresses review feedback with the vetoable contract decision: #863 (comment)
Addresses review feedback: #863 (comment)
christian-byrne
left a comment
There was a problem hiding this comment.
One transport blocker. Help and bundled docs drift.
Full context for agent readers
Findings:
- 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.
- Minor:
--stdout/--in-placeis accepted but ignored, and its inherited help describes behavior this emit-only command does not provide. - Minor: the bundled agent skill and command-schema discovery map omit
insert-workflowand retain an outdated exhaustive list of op emitters.
Verification:
- Exact head:
df7bf3fcc00855a57751bc6b3e995627aa8f0761from 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: nullfails withworkflow_edit_invalid. - Malformed input: outer-shape errors produce a stable error envelope.
- Help: command is listed, but exposes the ineffective inherited
--stdout/--in-placeoption. - JSON output: retains
envelope/1,data.op,base_version, andwrote: nullwithchanged: 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.
Addresses review feedback: #863 (comment) #863 (comment) #863 (comment)
christian-byrne
left a comment
There was a problem hiding this comment.
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.
…it-only validation Addresses #863 (comment) Addresses #863 (comment)
Draft. Adds
comfy workflow insert-workflow <file> <template>, which emits one stampedinsert_workflowop 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_workflowoperation while cmp remains the sole owner of validation, ID remapping, application, replay, and projection.What changed
comfy_cli/command/workflow.py: new standaloneinsert-workflowcommand.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 .andruff format --check .: pass.pytest: 7,407 passed, 32 skipped, 47 unrelated baseline failures.git diff --check: clean.Decisions to veto
insert-workflowrather than a flag on the existingloadcommand.Ship order
cmp → cloud → comfy-cli (this PR) → frontend follower. Sibling PRs: cloud agent tool draft PR.