|
1 | | -# Design: comfy-java-sdk CLI and MCP parity |
| 1 | +# Design: Comfy Java SDK Full Parity |
2 | 2 |
|
3 | | -## Architecture |
| 3 | +## 1. Architecture |
4 | 4 |
|
| 5 | +~~~text |
5 | 6 | Application |
6 | 7 | -> ComfyClient |
7 | | - -> ComfyCli -> ComfyCliExecutor -> comfy |
8 | | - -> ComfyMcpClient -> MCP stdio JSON-RPC -> comfy-mcp -> comfy |
| 8 | + -> CLI route: ComfyCli -> executor/stream session -> comfy |
| 9 | + -> MCP route: ComfyMcpClient -> stdio JSON-RPC -> comfy-mcp -> comfy |
| 10 | +~~~ |
9 | 11 |
|
10 | | -The Java SDK remains an orchestration/integration SDK. The Python CLI and MCP server stay authoritative for Comfy behavior. |
| 12 | +The Python CLI and MCP server remain authoritative. Java adds type-safe requests, process ownership, async/streaming abstractions, compatibility discovery, structured errors/results, bounded memory, deterministic cleanup and production diagnostics. |
11 | 13 |
|
12 | | -## CLI design |
13 | | -ComfyCli exposes typed methods for documented stable command families and keeps execute(String...) for newly-added or beta options. Option objects use Java 8-compatible mutable builders but support copy construction so high-level helpers never mutate caller-owned options. |
| 14 | +## 2. Canonical implementation |
14 | 15 |
|
15 | | -Global JSON parsing uses ComfyJsonEnvelope, preserving JsonNode data/error fields to remain forward compatible with the CLI self-describing contract. |
| 16 | +Use feature/2.0.x as the canonical behavior-development line because it currently contains the newest subprocess and MCP hardening. |
16 | 17 |
|
17 | | -GenerateOptions models common stable flags and supports param(name,value) for model-specific beta parameters. |
| 18 | +After canonical behavior is green: |
18 | 19 |
|
19 | | -## MCP design |
20 | | -ComfyMcpClient owns exactly one comfy-mcp child process. Lifecycle is NEW -> CONNECTING -> CONNECTED -> CLOSED. A failed connect cleans resources and returns to NEW so callers may retry. |
| 20 | +1. back-port identical behavior to feature/1.0.x, |
| 21 | +2. forward-port identical behavior to feature/3.0.x, |
| 22 | +3. normalize allowed Jackson/JDK differences and compare, |
| 23 | +4. run the same fixtures on all three. |
21 | 24 |
|
22 | | -Every JSON-RPC request owns: |
23 | | -- a monotonically increasing id |
24 | | -- one CompletableFuture |
25 | | -- one timeout guard |
26 | | -- one pending-map entry |
| 25 | +No feature is complete until all three lines pass. |
27 | 26 |
|
28 | | -Completion, timeout, serialization/write failure and close all remove the exact pending entry. |
| 27 | +## 3. CLI API |
29 | 28 |
|
30 | | -stdout is the MCP protocol channel. stderr is drained concurrently into a capped diagnostic tail so the child cannot deadlock on pipe backpressure. |
| 29 | +### Three layers |
31 | 30 |
|
32 | | -close() is idempotent, closes stdin, destroys the process, waits a bounded grace interval, escalates to destroyForcibly, fails all pending RPCs and shuts down the scheduler. |
| 31 | +A. Typed stable API for high-frequency stable commands. |
33 | 32 |
|
34 | | -## Resource and memory boundaries |
35 | | -- CLI stdout/stderr capture limits are configurable; <=0 means unbounded. |
36 | | -- MCP frame, accumulated content and stderr diagnostic caps are configurable. |
37 | | -- Capped buffers retain a prefix and record truncation instead of growing without bound. |
38 | | -- No long-lived executor exists on the CLI path. |
39 | | -- MCP scheduler is daemon-backed and always shutdown by close(). |
| 33 | +B. Command-family API so each current first-party family is reachable without a shell. |
40 | 34 |
|
41 | | -## Security |
42 | | -- Credentials are expected through environment variables (COMFY_API_KEY, COMFY_BIN); no SDK logging of environment values. |
43 | | -- Arguments are passed as argv without a shell. |
44 | | -- Executable paths are treated as a literal executable, not parsed shell text. |
45 | | -- Network-exposure and spend-consent policy remains enforced by upstream comfy/comfy-mcp. |
| 35 | +C. Generic execute(String...) for future/unknown commands. |
46 | 36 |
|
47 | | -## Compatibility strategy |
48 | | -The source is kept Java 8 syntax-compatible wherever possible. Branch 3 changes only Jackson imports/API details and Maven/JDK baseline. Tests assert identical command argv and MCP behavior. |
| 37 | +Typed option objects remain Java 8-compatible and copyable. |
| 38 | + |
| 39 | +### Capability discovery |
| 40 | + |
| 41 | +Add a runtime surface model populated from: |
| 42 | + |
| 43 | +~~~text |
| 44 | +comfy --help-json |
| 45 | +comfy --json discover |
| 46 | +comfy --json discover --schemas-only |
| 47 | +~~~ |
| 48 | + |
| 49 | +It records current CLI version, command/subcommand/schema availability, error-code contract, typed-wrapper availability and generic fallback requirements. |
| 50 | + |
| 51 | +### Machine envelope |
| 52 | + |
| 53 | +ComfyJsonEnvelope retains stable fields, data, error and raw JSON. Truncated machine output must never be accepted as a valid envelope. |
| 54 | + |
| 55 | +## 4. CLI streaming |
| 56 | + |
| 57 | +The synchronous executor remains correct for short commands. Long-running run/jobs/download/build/deploy operations need incremental output. |
| 58 | + |
| 59 | +~~~text |
| 60 | +ComfyCliStreamExecutor |
| 61 | + -> Process |
| 62 | + -> stdout reader -> NDJSON decoder -> ComfyCliEvent -> listener |
| 63 | + -> stderr reader -> listener/bounded diagnostic tail |
| 64 | + -> completion future |
| 65 | +~~~ |
| 66 | + |
| 67 | +Required invariants: |
| 68 | + |
| 69 | +- events arrive before process exit, |
| 70 | +- listener failures do not leak the child, |
| 71 | +- cancellation kills/reaps the child, |
| 72 | +- close is idempotent, |
| 73 | +- stdout/stderr stay drained, |
| 74 | +- retained memory is bounded, |
| 75 | +- final envelope/error remains queryable. |
| 76 | + |
| 77 | +Public behavior must remain Java 8-compatible through ordinary threads and CompletableFuture. |
| 78 | + |
| 79 | +## 5. MCP lifecycle |
| 80 | + |
| 81 | +~~~text |
| 82 | +NEW |
| 83 | + | |
| 84 | + v |
| 85 | +CONNECTING ----failure----> NEW |
| 86 | + | |
| 87 | + v |
| 88 | +CONNECTED ----transport failure----> NEW |
| 89 | + | |
| 90 | + +-------------------------------> CLOSED |
| 91 | +NEW -----------------------------> CLOSED |
| 92 | +~~~ |
| 93 | + |
| 94 | +CLOSED is terminal. |
| 95 | + |
| 96 | +Unexpected child death must transition away from CONNECTED. |
| 97 | + |
| 98 | +Introduce one internal fatal-transport cleanup path responsible for: |
| 99 | + |
| 100 | +1. transition state, |
| 101 | +2. fail pending RPCs, |
| 102 | +3. close writer, |
| 103 | +4. destroy/reap child, |
| 104 | +5. close child streams, |
| 105 | +6. stop/join readers, |
| 106 | +7. clear references. |
| 107 | + |
| 108 | +Use it for stdout EOF, frame overflow, write failure, failed initialize and close. |
| 109 | + |
| 110 | +## 6. MCP RPC registry |
| 111 | + |
| 112 | +pendingRpcs contains active calls only. |
| 113 | + |
| 114 | +~~~text |
| 115 | +allocate id |
| 116 | +put(id, future) |
| 117 | +schedule timeout |
| 118 | +write |
| 119 | + | |
| 120 | + +-- response -> remove -> complete |
| 121 | + +-- rpc error -> remove -> exceptional complete |
| 122 | + +-- timeout -> remove -> exceptional complete |
| 123 | + +-- write error -> remove -> exceptional complete |
| 124 | + +-- transport close -> failAllPending |
| 125 | +~~~ |
| 126 | + |
| 127 | +Every request has one id, Future, pending entry and timeout guard. |
| 128 | + |
| 129 | +## 7. MCP complete tool facade |
| 130 | + |
| 131 | +The reviewed server exposes 40 first-party tools. Each receives a direct convenience method while generic callTool remains. |
| 132 | + |
| 133 | +Use stable Java types for stable inputs and forward-compatible Map/DTO structures where upstream parameters are intentionally dynamic. |
| 134 | + |
| 135 | +## 8. MCP notifications and elicitation |
| 136 | + |
| 137 | +Frames must be classified as: |
| 138 | + |
| 139 | +- response: id plus result/error, |
| 140 | +- server request: id plus method, |
| 141 | +- notification: method without id. |
| 142 | + |
| 143 | +Add ComfyMcpNotification and ComfyMcpListener for generic/progress notifications. |
| 144 | + |
| 145 | +If elicitation support is configured, add ComfyMcpElicitationHandler and advertise the capability. Otherwise do not advertise it. |
| 146 | + |
| 147 | +## 9. MCP content |
| 148 | + |
| 149 | +ComfyMcpContent keeps type, text, MIME, URI, encoded data accessor and raw JSON. Avoid eagerly duplicating large encoded payloads. |
| 150 | + |
| 151 | +## 10. Skills |
| 152 | + |
| 153 | +Treat comfy-cli as the source of truth for local skills. |
| 154 | + |
| 155 | +Typed operations: |
| 156 | + |
| 157 | +- install, |
| 158 | +- uninstall, |
| 159 | +- list, |
| 160 | +- show, |
| 161 | +- status, |
| 162 | +- validate. |
| 163 | + |
| 164 | +Options cover user/project scope, repeated target, repeated skill and dry-run. |
| 165 | + |
| 166 | +Do not vendor skill contents. |
| 167 | + |
| 168 | +## 11. Memory boundaries |
| 169 | + |
| 170 | +CLI configuration covers stdout cap, stderr cap, command timeout, probe timeout, process shutdown grace and one shared stream-drain deadline. |
| 171 | + |
| 172 | +MCP configuration covers frame chars, concatenated text chars, stderr tail chars, request timeout, initialize timeout and shutdown grace. |
| 173 | + |
| 174 | +All limits are validated and all unbounded semantics are explicit. |
| 175 | + |
| 176 | +## 12. Security |
| 177 | + |
| 178 | +Use argv-safe ProcessBuilder execution and never concatenate shell command strings. |
| 179 | + |
| 180 | +Prefer environment variables for secrets, never log environment values, and redact/avoid secret-bearing argv in diagnostics. |
| 181 | + |
| 182 | +Do not silently bypass upstream confirmation gates for spend, network exposure, node installation, version switching/update-all or killing untracked listeners. |
| 183 | + |
| 184 | +## 13. Doctor / compatibility |
| 185 | + |
| 186 | +Expand doctor into: |
| 187 | + |
| 188 | +- CLI binary/version check, |
| 189 | +- workspace/env/discover check, |
| 190 | +- minimum-supported version check, |
| 191 | +- optional comfy-mcp initialize/tools-list check, |
| 192 | +- optional local ComfyUI server-info check, |
| 193 | +- runtime CLI/MCP typed-parity report. |
| 194 | + |
| 195 | +Diagnostics contain no secret values. |
| 196 | + |
| 197 | +## 14. CI / branch parity |
| 198 | + |
| 199 | +JaCoCo prepare-agent must reach the forked test JVM. CI fails if jacoco.exec/report is missing or coverage is below threshold. haltOnFailure is true for production validation. |
| 200 | + |
| 201 | +Commit one identical parity manifest on all three branches. |
| 202 | + |
| 203 | +Each branch validates CLI families, MCP tools, Skills and lifecycle invariants against that manifest. |
| 204 | + |
| 205 | +A normalized comparison may fetch other branches and ignore only Jackson imports, JDK adapters, POM/dependency/build differences. |
| 206 | + |
| 207 | +## 15. Dependencies |
| 208 | + |
| 209 | +Baseline targets at review time: |
| 210 | + |
| 211 | +- 1.x: Jackson at least 2.18.10 on Java 8, |
| 212 | +- 2.x: current patched Jackson 2 release compatible with Java 17, |
| 213 | +- 3.x: Jackson at least 3.2.2. |
| 214 | + |
| 215 | +Revalidate exact patch versions when implementing. |
| 216 | + |
| 217 | +## 16. Tests |
| 218 | + |
| 219 | +Unit tests cover option builders, argv, parsing and validation. |
| 220 | + |
| 221 | +Hermetic process fixtures cover stdout/stderr, huge output, timeout, inherited pipes, stdin, UTF-8 and early exit. |
| 222 | + |
| 223 | +Fake MCP E2E covers initialize, all typed wrapper request shapes, generic calls, concurrency, timeout, stderr flood, child crash, malformed/oversized frames, mixed content, notifications and elicitation if enabled. |
| 224 | + |
| 225 | +Soak tests assert zero pending RPCs, no live child, no accumulating reader/timer threads and bounded diagnostics. |
0 commit comments