feat: align Comfy CLI/MCP surface and harden subprocess lifecycle

Author: loong10k

.github/workflows/ci.yml

@@ -1,17 +1,9 @@
-# CI workflow for the feature/3.0.x line (JDK 21)
-#
-# Triggers:
-#   - push / pull_request on the feature/3.0.x branch
-#   - manual workflow_dispatch
-#
-# Runs `./mvnw -B clean verify` (Maven 4 via the checked-in wrapper — the
-# runner's bundled Maven 3 cannot parse the POM 4.1.0 model) which includes
-# the JaCoCo coverage gate (90% line coverage, haltOnFailure=false).
+# CI for the Java 21 / Jackson 3 line.
 name: CI
 
 on:
   push:
-    branches: [feature/3.0.x]
+    branches: [feature/3.0.x, hardening/3.0.x]
   pull_request:
     branches: [feature/3.0.x]
   workflow_dispatch:
@@ -35,7 +27,7 @@ jobs:
           java-version: '21'
           cache: maven
 
-      - name: Build and verify with JaCoCo coverage gate
+      - name: Build and verify
         run: ./mvnw -B --no-transfer-progress clean verify
 
       - name: Upload JaCoCo coverage report

src/main/java/io/github/easy4j/comfy/ComfyClient.java

@@ -2,186 +2,99 @@
  * Copyright (c) 2018-present, easy-4-java (https://github.com/easy-4-java).
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
  */
 package io.github.easy4j.comfy;
 
 import java.util.Objects;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import io.github.easy4j.comfy.cli.ComfyCli;
 import io.github.easy4j.comfy.cli.ComfyCliExecutor;
 import io.github.easy4j.comfy.cli.ComfyCliResult;
+import io.github.easy4j.comfy.model.ComfyCliEnvelope;
+import tools.jackson.databind.DeserializationFeature;
 import tools.jackson.databind.JsonNode;
+import tools.jackson.databind.ObjectMapper;
 import tools.jackson.databind.json.JsonMapper;
 
 /**
- * High-level Java facade that wraps every local {@code comfy} CLI invocation
- * behind ergonomic, strongly-typed methods.
+ * High-level Java facade for the local {@code comfy} CLI route.
  *
- * <p>This class is the recommended entry point for the CLI route. It owns a
- * single {@link ComfyClientConfig} and a single {@link ComfyCli}, forwarding
- * the configured defaults to every call. For the MCP route (spawn
- * {@code comfy-mcp} and speak JSON-RPC over stdio) use
- * {@code io.github.easy4j.comfy.mcp.ComfyMcpClient}.</p>
- *
- * @author <a href="https://github.com/loong10k">Loong Wan</a>
- * @since 1.0.0
- * @see ComfyClientConfig
- * @see ComfyCli
+ * <p>The lower-level {@link ComfyCli} mirrors the CLI command tree; this class
+ * adds parsed JSON helpers while retaining access to the raw mapper.</p>
  */
 public class ComfyClient implements AutoCloseable {
 
-    private static final Logger log = LoggerFactory.getLogger(ComfyClient.class);
-    private static final JsonMapper MAPPER = new JsonMapper();
+    private static final ObjectMapper MAPPER =
+            JsonMapper.builder().disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES).build();
 
     private final ComfyClientConfig config;
     private final ComfyCli cli;
 
-    /**
-     * Creates a new client backed by the given configuration. A default
-     * {@link ComfyCli} and {@link ComfyCliExecutor} are constructed
-     * automatically.
-     *
-     * @param config runtime configuration; must not be {@code null}.
-     * @throws NullPointerException if {@code config} is {@code null}.
-     */
     public ComfyClient(ComfyClientConfig config) {
         this.config = Objects.requireNonNull(config, "config");
         this.config.validate();
         this.cli = new ComfyCli(this.config, new ComfyCliExecutor(this.config));
     }
 
-    /**
-     * Creates a new client that delegates to the supplied {@link ComfyCli}.
-     *
-     * <p>This constructor exists primarily for testing &mdash; it lets a
-     * caller substitute a {@link ComfyCli} backed by a mocked executor while
-     * still using the default behaviour of the surrounding facade.</p>
-     *
-     * @param config runtime configuration; must not be {@code null}.
-     * @param cli    the CLI facade to delegate to; must not be {@code null}.
-     * @throws NullPointerException if either argument is {@code null}.
-     */
     public ComfyClient(ComfyClientConfig config, ComfyCli cli) {
         this.config = Objects.requireNonNull(config, "config");
+        this.config.validate();
         this.cli = Objects.requireNonNull(cli, "cli");
     }
 
-    /**
-     * Runs {@code comfy --version}.
-     *
-     * @return the raw CLI invocation result; never {@code null}.
-     */
-    public ComfyCliResult version() {
-        return cli.version();
-    }
+    public ComfyCliResult version() { return cli.version(); }
+    public ComfyCliResult help() { return cli.help(); }
+    public boolean isAvailable() { return cli.executor().probe(); }
+    public ComfyCliResult cloudLogin() { return cli.cloudLogin(); }
+    public ComfyCliResult setup() { return cli.setupYes(); }
+    public ComfyCliResult skillsInstall() { return cli.skillsInstall(); }
 
     /**
-     * Runs {@code comfy --help}.
-     *
-     * @return the raw CLI invocation result; never {@code null}.
-     */
-    public ComfyCliResult help() {
-        return cli.help();
-    }
-
-    /**
-     * Probes CLI availability with {@code comfy --version} and the configured
-     * probe timeout.
-     *
-     * @return {@code true} when the local CLI is reachable.
-     */
-    public boolean isAvailable() {
-        return cli.executor().probe();
-    }
-
-    /**
-     * Sends a generation request ({@code comfy generate <model>}) with
-     * {@code --json} so the standard output can be parsed as JSON.
-     *
-     * @param model   the generation model alias.
-     * @param options the generation options; must not be {@code null}.
-     * @return the parsed JSON root of the {@code --json} output; never
-     *         {@code null}.
-     * @throws ComfyException when the invocation fails or prints non-JSON.
+     * Runs partner generation with command-level JSON output without mutating
+     * the caller's reusable options object.
      */
     public JsonNode generateJson(String model, ComfyCli.GenerateOptions options) {
-        ComfyCli.GenerateOptions jsonOptions = options.json(true);
-        ComfyCliResult result = cli.generate(model, jsonOptions);
-        if (!result.isSuccess()) {
-            throw new ComfyException("comfy generate failed: exit=" + result.getExitCode()
-                    + " stderr=" + result.getStderr());
-        }
+        Objects.requireNonNull(options, "options");
+        ComfyCliResult result = cli.generate(model, options.copy().json(true));
+        requireSuccess(result, "comfy generate");
         try {
             return MAPPER.readTree(result.getStdout());
         } catch (Exception e) {
             throw new ComfyException("comfy generate --json printed non-JSON output", e);
         }
     }
 
-    /**
-     * Runs {@code comfy cloud login} (browser OAuth).
-     *
-     * @return the raw CLI invocation result; never {@code null}.
-     */
-    public ComfyCliResult cloudLogin() {
-        return cli.cloudLogin();
+    /** Executes any CLI command using the global uniform {@code --json} envelope. */
+    public ComfyCliEnvelope executeJson(String... args) {
+        ComfyCliResult result = cli.executeJson(args);
+        requireSuccess(result, "comfy --json");
+        if (result.isTruncated()) {
+            throw new ComfyException("comfy --json output exceeded maxOutputBytes="
+                    + config.getMaxOutputBytes());
+        }
+        try {
+            return MAPPER.readValue(result.getStdout(), ComfyCliEnvelope.class);
+        } catch (Exception e) {
+            throw new ComfyException("comfy --json printed an invalid envelope", e);
+        }
     }
 
-    /**
-     * Runs {@code comfy setup -y} (non-interactive setup).
-     *
-     * @return the raw CLI invocation result; never {@code null}.
-     */
-    public ComfyCliResult setup() {
-        return cli.setupYes();
-    }
+    public ComfyCliEnvelope environment() { return executeJson("env"); }
+    public ComfyCliEnvelope whichJson() { return executeJson("which"); }
+    public ComfyCliEnvelope discover() { return executeJson("discover"); }
 
-    /**
-     * Runs {@code comfy skills install}.
-     *
-     * @return the raw CLI invocation result; never {@code null}.
-     */
-    public ComfyCliResult skillsInstall() {
-        return cli.skillsInstall();
-    }
+    public ComfyCli cli() { return cli; }
+    public ComfyClientConfig getConfig() { return config; }
 
-    /**
-     * Returns the underlying {@link ComfyCli} for advanced callers.
-     *
-     * @return the CLI facade backing this client; never {@code null}.
-     */
-    public ComfyCli cli() {
-        return cli;
-    }
-
-    /**
-     * Returns the runtime configuration used by this client.
-     *
-     * @return the configuration; never {@code null}.
-     */
-    public ComfyClientConfig getConfig() {
-        return config;
+    private static void requireSuccess(ComfyCliResult result, String operation) {
+        if (!result.isSuccess()) {
+            throw new ComfyException(operation + " failed: exit=" + result.getExitCode()
+                    + " stderr=" + result.getStderr());
+        }
     }
 
-    /**
-     * Closes this client. The default implementation is a no-op because the
-     * underlying {@link ComfyCliExecutor} does not hold any long-lived
-     * resources.
-     */
     @Override
     public void close() {
+        // CLI route owns no persistent subprocess or executor.
     }
 }

src/main/java/io/github/easy4j/comfy/ComfyClientConfig.java

@@ -2,16 +2,6 @@
  * Copyright (c) 2018-present, easy-4-java (https://github.com/easy-4-java).
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
  */
 package io.github.easy4j.comfy;
 
@@ -21,51 +11,43 @@
 import lombok.Data;
 
 /**
- * Configuration for the comfy CLI subprocess route.
+ * Runtime configuration for the local {@code comfy} CLI subprocess route.
  *
- * <p>Plain POJO (Spring {@code @ConfigurationProperties}-bindable).</p>
- *
- * @author <a href="https://github.com/loong10k">Loong Wan</a>
- * @since 1.0.0
- * @see ComfyClient
+ * <p>Credentials belong in {@link #environment}, never in argv, because
+ * command-line arguments can be visible to other local processes.</p>
  */
 @Data
 public class ComfyClientConfig {
 
-    /** Name or absolute path of the local {@code comfy} CLI executable. */
     private String localExecutable = "comfy";
-
-    /**
-     * Extra environment variables for the child process (e.g.
-     * {@code COMFY_API_KEY}, {@code COMFY_WHERE}); merged over the parent
-     * environment. Credentials must travel here — never as command line
-     * arguments, which are visible in {@code ps} output.
-     */
     private Map<String, String> environment;
-
-    /** Command execution timeout in seconds (generation runs can be long). */
     private int localTimeoutSeconds = 600;
-
-    /** Timeout in seconds used by {@link ComfyCliExecutor#probe()} when verifying CLI availability. */
     private int localProbeTimeoutSeconds = 5;
-
-    /**
-     * Default routing forwarded as {@code --where <where>} to commands that
-     * accept it: {@code local} or {@code cloud}. The CLI also honours the
-     * {@code COMFY_WHERE} environment variable via {@link #environment}.
-     */
+    private int maxOutputBytes = 16 * 1024 * 1024;
     private String defaultWhere;
 
-    /**
-     * Validates the configuration.
-     *
-     * @throws IllegalStateException when {@code defaultWhere} is neither
-     *                               {@code local} nor {@code cloud}.
-     */
     public void validate() {
         Objects.requireNonNull(localExecutable, "localExecutable");
-        if (defaultWhere != null && !"local".equals(defaultWhere) && !"cloud".equals(defaultWhere)) {
-            throw new IllegalStateException("defaultWhere must be 'local' or 'cloud': " + defaultWhere);
+        if (localExecutable.trim().isEmpty()) {
+            throw new IllegalStateException("localExecutable must not be blank");
+        }
+        if (localTimeoutSeconds <= 0) {
+            throw new IllegalStateException("localTimeoutSeconds must be > 0");
+        }
+        if (localProbeTimeoutSeconds <= 0) {
+            throw new IllegalStateException("localProbeTimeoutSeconds must be > 0");
+        }
+        if (maxOutputBytes == 0 || maxOutputBytes < -1) {
+            throw new IllegalStateException("maxOutputBytes must be -1 (unbounded) or > 0");
+        }
+        if (defaultWhere != null) {
+            requireWhere(defaultWhere);
+        }
+    }
+
+    public static void requireWhere(String where) {
+        if (!"local".equals(where) && !"cloud".equals(where)) {
+            throw new IllegalArgumentException("where must be 'local' or 'cloud': " + where);
         }
     }
 }