Break-glass merge PR #2633

src/cli/cli.c

@@ -14088,13 +14088,29 @@ static void cli_add_typed(yyjson_mut_doc *out, yyjson_mut_val *obj, const char *
     yyjson_mut_obj_add(obj, yyjson_mut_strcpy(out, key), vv);
 }
 
-static bool cli_stdin_allowed_for_schema(const char *schema_str);
+static bool cli_stdin_allowed_for_schema(const char *schema_str, cbm_cli_stdin_kind_t stdin_kind);
 
-bool cbm_cli_args_from_stdin_allowed(const char *tool_name, bool stdin_is_tty) {
+cbm_cli_stdin_kind_t cbm_cli_stdin_kind(int fd) {
+#ifdef _WIN32
+    bool terminal = _isatty(fd) != 0;
+#else
+    bool terminal = isatty(fd) != 0;
+#endif
+    if (terminal) {
+        return CBM_CLI_STDIN_TERMINAL;
+    }
+    struct stat state;
+    if (fstat(fd, &state) == 0 && S_ISREG(state.st_mode)) {
+        return CBM_CLI_STDIN_FILE;
+    }
+    return CBM_CLI_STDIN_STREAM;
+}
+
+bool cbm_cli_args_from_stdin_allowed(const char *tool_name, cbm_cli_stdin_kind_t stdin_kind) {
     /* An interactive run has nothing piped to read: consulting the terminal
      * would just sit waiting for the user to type JSON. Unchanged by #1359 —
      * this is why the hang was only ever reachable from automation. */
-    if (stdin_is_tty) {
+    if (stdin_kind == CBM_CLI_STDIN_TERMINAL) {
         return false;
     }
 
@@ -14105,12 +14121,9 @@ bool cbm_cli_args_from_stdin_allowed(const char *tool_name, bool stdin_is_tty) {
      * parent must call child.stdin.end() explicitly, which almost nobody does
      * for a command they are not writing to. fd 0 then stays open with no
      * writer and the read never returns; the reporter's shell script was still
-     * parked in fread(0) fourteen minutes later. For a tool whose input_schema
-     * declares no properties (list_projects) stdin cannot carry anything the
-     * tool would accept, so that read is pure deadlock with nothing to gain:
-     * never start it, and let the caller fall through to `{}` exactly as an
-     * interactive run already did. Tools that DO declare properties keep the
-     * documented `echo '<json>' | cli <tool>` channel untouched. */
+     * parked in fread(0) fourteen minutes later. A call that is complete
+     * without arguments must therefore never wait on such a stream: see
+     * cli_stdin_allowed_for_schema for which schemas that covers. */
     const char *schema_str = cbm_mcp_tool_input_schema(tool_name);
     if (!schema_str) {
         /* Unknown tool: dispatch rejects it by name and no stdin content can
@@ -14118,35 +14131,50 @@ bool cbm_cli_args_from_stdin_allowed(const char *tool_name, bool stdin_is_tty) {
         return false;
     }
 
-    return cli_stdin_allowed_for_schema(schema_str);
+    return cli_stdin_allowed_for_schema(schema_str, stdin_kind);
 }
 
-/* Schema→decision core of the stdin gate, split out so the zero-argument
- * branch stays TESTED even when no shipped tool is zero-argument anymore:
- * #1181 gave list_projects pagination parameters, retiring the last
- * empty-properties schema, and the #1359 regression tests had leaned on it
- * as their live example. The product contract is unchanged — a schema that
- * declares no properties means stdin has nothing to carry and the read is
- * pure deadlock; the tests now exercise this path directly. */
-static bool cli_stdin_allowed_for_schema(const char *schema_str) {
+/* Schema→decision core of the stdin gate, split out so every schema shape
+ * stays TESTED whatever the shipped tool table holds.
+ *   no properties   -> never: stdin cannot carry anything the tool accepts.
+ *   optional only   -> a regular file only. The bare call is already complete
+ *                      with `{}`, so blocking on a stream for optional extras
+ *                      is the #1359 deadlock again — #1181 gave list_projects
+ *                      paging properties and re-opened exactly that hang for
+ *                      every caller that leaves an inherited pipe open (ssh,
+ *                      Node spawn). A `< args.json` file always reaches EOF.
+ *   any required    -> file or stream: the bare call cannot succeed anyway, so
+ *                      the documented `echo '<json>' | cli <tool>` channel is
+ *                      the only way this invocation can carry its arguments. */
+static bool cli_stdin_allowed_for_schema(const char *schema_str, cbm_cli_stdin_kind_t stdin_kind) {
+    if (stdin_kind == CBM_CLI_STDIN_TERMINAL) {
+        return false;
+    }
     yyjson_doc *schema_doc = yyjson_read(schema_str, strlen(schema_str), 0);
     if (!schema_doc) {
         /* The schemas are compile-time constants, so a parse failure means a
          * malformed literal rather than caller input. Keep the documented stdin
          * channel rather than silently narrowing it on a schema we could not
-         * read — the gate must only ever fire on positive evidence that the
-         * tool takes no arguments. */
+         * read — the gate must only ever fire on positive evidence about the
+         * tool's arguments. */
         return true;
     }
-    yyjson_val *props = yyjson_obj_get(yyjson_doc_get_root(schema_doc), "properties");
+    yyjson_val *root = yyjson_doc_get_root(schema_doc);
+    yyjson_val *props = yyjson_obj_get(root, "properties");
+    yyjson_val *required = yyjson_obj_get(root, "required");
     bool accepts_arguments = props && yyjson_is_obj(props) && yyjson_obj_size(props) > 0;
+    bool requires_arguments = required && yyjson_is_arr(required) && yyjson_arr_size(required) > 0;
     yyjson_doc_free(schema_doc);
-    return accepts_arguments;
+    if (!accepts_arguments) {
+        return false;
+    }
+    return requires_arguments || stdin_kind == CBM_CLI_STDIN_FILE;
 }
 
 #ifdef CBM_CLI_ENABLE_TEST_API
-bool cbm_cli_stdin_allowed_for_schema_for_test(const char *schema_str) {
-    return cli_stdin_allowed_for_schema(schema_str);
+bool cbm_cli_stdin_allowed_for_schema_for_test(const char *schema_str,
+                                               cbm_cli_stdin_kind_t stdin_kind) {
+    return cli_stdin_allowed_for_schema(schema_str, stdin_kind);
 }
 #endif
 

src/cli/cli.h

@@ -48,13 +48,24 @@ const char *cbm_cli_get_version(void);
  * must free. Caller frees the returned JSON string. */
 char *cbm_cli_build_args_json(const char *tool_name, int argc, char **argv, char **err_out);
 
+/* What a descriptor is, as far as the stdin argument gate cares. */
+typedef enum {
+    CBM_CLI_STDIN_TERMINAL = 0, /* interactive: nothing piped to read */
+    CBM_CLI_STDIN_FILE = 1,     /* regular file (`< args.json`): always reaches EOF */
+    CBM_CLI_STDIN_STREAM = 2,   /* pipe, socket, device: EOF only when the writer closes */
+} cbm_cli_stdin_kind_t;
+
+/* Classify an open descriptor: terminal (isatty), regular file (fstat), else stream. */
+cbm_cli_stdin_kind_t cbm_cli_stdin_kind(int fd);
+
 /* Decide whether `cli <tool>` may take its JSON arguments from stdin, given
  * that no --args-file, raw-JSON positional and no --flag form was supplied.
- * stdin_is_tty is the caller's isatty(0) result. True only for a non-terminal
- * stdin AND a tool whose input_schema declares at least one property; a
- * zero-argument tool such as list_projects must never read stdin, because an
- * inherited-but-never-closed pipe makes that read block forever (#1359). */
-bool cbm_cli_args_from_stdin_allowed(const char *tool_name, bool stdin_is_tty);
+ * Never from a terminal, never for a tool whose input_schema declares no
+ * properties. A regular file is read for any tool that takes arguments; a
+ * stream only for a tool with a REQUIRED argument. A tool callable without
+ * arguments (list_projects) is complete with `{}`, and an inherited stream
+ * nobody closes would block that read forever (#1359). */
+bool cbm_cli_args_from_stdin_allowed(const char *tool_name, cbm_cli_stdin_kind_t stdin_kind);
 
 /* Print per-tool help (usage + the tool's flags with type/description/required)
  * derived from its input_schema, to stdout. Returns 0 if the tool is known,

src/main.c

@@ -974,9 +974,9 @@ static int run_cli(int argc, char **argv, cbm_project_lock_manager_t *project_lo
             return SKIP_ONE;
         }
         args_json = heap_args;
-    } else if (cbm_cli_args_from_stdin_allowed(tool_name, cli_isatty(0) != 0)) {
+    } else if (cbm_cli_args_from_stdin_allowed(tool_name, cbm_cli_stdin_kind(0))) {
         /* piped stdin (UTF-8 clean, no shell quoting): cli <tool> < args.json.
-         * Gated (#1359): a tool that declares no arguments must not read a pipe
+         * Gated (#1359): a call complete without arguments must not read a pipe
          * nobody is going to write to or close — see the WHY on the predicate. */
         heap_args = cli_slurp_stream(stdin);
         if (heap_args && heap_args[0]) {

tests/test_cli.c

@@ -40,6 +40,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 #ifdef _WIN32
+#include <fcntl.h> // _O_BINARY
 #include <io.h>
 #endif
 #ifndef _WIN32
@@ -74,7 +75,8 @@ int cbm_cli_activation_abort_cleanup_probe_for_test(void);
 bool cbm_cli_activation_test_ops_installed(void);
 int cbm_cli_build_yaml_stdio_mcp_block_for_test(const char *binary_path, bool goose_schema,
                                                 char *block, size_t block_size);
-bool cbm_cli_stdin_allowed_for_schema_for_test(const char *schema_str);
+bool cbm_cli_stdin_allowed_for_schema_for_test(const char *schema_str,
+                                               cbm_cli_stdin_kind_t stdin_kind);
 
 /* CLI config lifecycle tests exercise the real activation guard on POSIX. Keep
  * that protocol coverage, but bind it to this suite's private endpoint so an
@@ -17338,27 +17340,71 @@ TEST(cli_zero_argument_tool_never_reads_stdin_issue1359) {
      * never read stdin (nothing it could carry; the read is pure deadlock).
      * Since #1181 no SHIPPED tool is zero-argument anymore — list_projects,
      * the original example, now declares pagination properties — so the
-     * zero-argument branch is exercised directly through the schema seam,
-     * and list_projects asserts its NEW truth. */
-    ASSERT_FALSE(
-        cbm_cli_stdin_allowed_for_schema_for_test("{\"type\":\"object\",\"properties\":{}}"));
-    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test("{\"type\":\"object\"}"));
-    ASSERT_TRUE(cbm_cli_stdin_allowed_for_schema_for_test(
-        "{\"type\":\"object\",\"properties\":{\"p\":{\"type\":\"string\"}}}"));
+     * zero-argument branch is exercised directly through the schema seam. */
+    const char *no_props = "{\"type\":\"object\",\"properties\":{}}";
+    const char *no_props_key = "{\"type\":\"object\"}";
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(no_props, CBM_CLI_STDIN_STREAM));
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(no_props, CBM_CLI_STDIN_FILE));
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(no_props_key, CBM_CLI_STDIN_STREAM));
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(no_props_key, CBM_CLI_STDIN_FILE));
 
-    /* list_projects now takes piped arguments (offset/limit/include_details);
-     * the TTY guard still refuses regardless of schema. */
-    ASSERT_TRUE(cbm_cli_args_from_stdin_allowed("list_projects", false));
-    ASSERT_FALSE(cbm_cli_args_from_stdin_allowed("list_projects", true));
+    /* A required argument: the bare call cannot succeed, so stdin is the
+     * documented channel from a pipe and from a file alike. */
+    const char *required =
+        "{\"type\":\"object\",\"properties\":{\"p\":{\"type\":\"string\"}},\"required\":[\"p\"]}";
+    ASSERT_TRUE(cbm_cli_stdin_allowed_for_schema_for_test(required, CBM_CLI_STDIN_STREAM));
+    ASSERT_TRUE(cbm_cli_stdin_allowed_for_schema_for_test(required, CBM_CLI_STDIN_FILE));
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(required, CBM_CLI_STDIN_TERMINAL));
+    PASS();
+}
+
+/* #1181 gave list_projects optional paging properties, which re-opened #1359:
+ * the schema gate only skipped EMPTY schemas, so `cli list_projects` parked in
+ * fread(0) again for every caller holding an inherited pipe open (an ssh
+ * session's stdin hung the Windows smoke's Phase 1b for 25 minutes). A call
+ * complete without arguments must never wait on a stream; a regular file
+ * (`< args.json`) always reaches EOF and keeps working. */
+TEST(cli_optional_argument_tool_never_reads_stream_stdin) {
+    const char *optional = "{\"type\":\"object\",\"properties\":{\"p\":{\"type\":\"string\"}}}";
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(optional, CBM_CLI_STDIN_STREAM));
+    ASSERT_TRUE(cbm_cli_stdin_allowed_for_schema_for_test(optional, CBM_CLI_STDIN_FILE));
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(optional, CBM_CLI_STDIN_TERMINAL));
+    const char *empty_required =
+        "{\"type\":\"object\",\"properties\":{\"p\":{\"type\":\"string\"}},\"required\":[]}";
+    ASSERT_FALSE(cbm_cli_stdin_allowed_for_schema_for_test(empty_required, CBM_CLI_STDIN_STREAM));
+
+    ASSERT_FALSE(cbm_cli_args_from_stdin_allowed("list_projects", CBM_CLI_STDIN_STREAM));
+    ASSERT_TRUE(cbm_cli_args_from_stdin_allowed("list_projects", CBM_CLI_STDIN_FILE));
+    ASSERT_FALSE(cbm_cli_args_from_stdin_allowed("list_projects", CBM_CLI_STDIN_TERMINAL));
+    PASS();
+}
+
+/* The classifier behind the gate, on real descriptors: a regular file is FILE
+ * (reads always end), a pipe is STREAM (ends only when its writer closes). */
+TEST(cli_stdin_kind_classifies_file_and_pipe) {
+    FILE *file = tmpfile();
+    ASSERT_NOT_NULL(file);
+    ASSERT_EQ(cbm_cli_stdin_kind(fileno(file)), CBM_CLI_STDIN_FILE);
+    (void)fclose(file);
+
+    int fds[2];
+#ifdef _WIN32
+    ASSERT_EQ(_pipe(fds, 4096, _O_BINARY), 0);
+#else
+    ASSERT_EQ(pipe(fds), 0);
+#endif
+    ASSERT_EQ(cbm_cli_stdin_kind(fds[0]), CBM_CLI_STDIN_STREAM);
+    (void)close(fds[0]);
+    (void)close(fds[1]);
     PASS();
 }
 
 /* The gate must track the advertised input_schema for the WHOLE tool table, not
- * carry a special case for list_projects: a future zero-argument tool would
- * otherwise reintroduce #1359 silently. Expectation is derived independently
- * from the schema the MCP tools/list publishes. */
+ * carry a special case for list_projects: a future zero-argument or
+ * optional-only tool would otherwise reintroduce #1359 silently. Expectation is
+ * derived independently from the schema the MCP tools/list publishes. */
 TEST(cli_stdin_args_gate_tracks_tool_schema_issue1359) {
-    int zero_argument_tools = 0;
+    int optional_only_tools = 0;
     for (int i = 0; i < cbm_mcp_tool_count(); i++) {
         const char *name = cbm_mcp_tool_name(i);
         ASSERT_NOT_NULL(name);
@@ -17367,22 +17413,25 @@ TEST(cli_stdin_args_gate_tracks_tool_schema_issue1359) {
 
         yyjson_doc *doc = yyjson_read(schema, strlen(schema), 0);
         ASSERT_NOT_NULL(doc);
-        yyjson_val *props = yyjson_obj_get(yyjson_doc_get_root(doc), "properties");
+        yyjson_val *root = yyjson_doc_get_root(doc);
+        yyjson_val *props = yyjson_obj_get(root, "properties");
+        yyjson_val *required = yyjson_obj_get(root, "required");
         bool declares_properties = props && yyjson_is_obj(props) && yyjson_obj_size(props) > 0;
+        bool declares_required =
+            required && yyjson_is_arr(required) && yyjson_arr_size(required) > 0;
         yyjson_doc_free(doc);
 
-        if (!declares_properties) {
-            zero_argument_tools++;
+        if (declares_properties && !declares_required) {
+            optional_only_tools++;
         }
-        ASSERT_EQ(cbm_cli_args_from_stdin_allowed(name, false), declares_properties);
-        ASSERT_FALSE(cbm_cli_args_from_stdin_allowed(name, true));
+        ASSERT_EQ(cbm_cli_args_from_stdin_allowed(name, CBM_CLI_STDIN_FILE), declares_properties);
+        ASSERT_EQ(cbm_cli_args_from_stdin_allowed(name, CBM_CLI_STDIN_STREAM),
+                  declares_properties && declares_required);
+        ASSERT_FALSE(cbm_cli_args_from_stdin_allowed(name, CBM_CLI_STDIN_TERMINAL));
     }
-    /* Since #1181 every shipped tool declares properties, so the sweep's
-     * zero-argument branch can be empty here — that branch is pinned directly
-     * against the schema seam in the test above, which survives the registry
-     * having no zero-argument example. The sweep still proves schema↔gate
-     * parity for every tool that exists. */
-    (void)zero_argument_tools;
+    /* list_projects is the live optional-only example today; the sweep must
+     * actually exercise that branch, not pass vacuously. */
+    ASSERT_GT(optional_only_tools, 0);
     PASS();
 }
 
@@ -18529,6 +18578,8 @@ SUITE(cli) {
 
     /* Stdin argument gate (#1359) */
     RUN_TEST(cli_zero_argument_tool_never_reads_stdin_issue1359);
+    RUN_TEST(cli_optional_argument_tool_never_reads_stream_stdin);
+    RUN_TEST(cli_stdin_kind_classifies_file_and_pipe);
     RUN_TEST(cli_stdin_args_gate_tracks_tool_schema_issue1359);
     cbm_cli_set_activation_runtime_parent_for_test(NULL);
     test_rmdir_r(g_cli_suite_runtime_parent);