feat(telemetry): emit partner_nodes_detected + stamp caller_kind on every event (BE-5633)

comfy_cli/caller.py

@@ -9,7 +9,7 @@
     1. ``COMFY_USER_AGENT=<label>`` → explicit override, agentic, label preserved.
     2. ``AI_AGENT`` truthy → agentic, kind="agent".
     3. ``CLAUDECODE`` truthy → Claude Code session, kind="claude-code".
-    4. stdout is not a TTY → agentic, kind="pipe".
+    4. stdout is not a TTY (or is missing/closed) → agentic, kind="pipe".
     5. otherwise → kind="user".
 
 Claude Code is checked after AI_AGENT because AI_AGENT is the generic
@@ -42,13 +42,57 @@ def _truthy(value: str | None) -> bool:
     return value.strip().lower() not in {"", "0", "false", "no", "off"}
 
 
+def stream_is_tty(stream: object) -> bool:
+    """True only when *stream* is a live TTY. Never raises — that is the point.
+
+    ``sys.stdout.isatty()`` assumes stdout is a live stream, but a process's
+    standard streams are not guaranteed to be one. Under ``pythonw``, a Windows
+    service, or a detached/daemonised parent they can be ``None``; after a
+    wrapper closes or replaces them they can be an already-closed file, an
+    object with no ``isatty`` at all, or one backed by a revoked file
+    descriptor. Those raise ``AttributeError``, ``ValueError``, ``OSError``
+    (``EBADF`` / ``WinError 6``) and ``TypeError`` respectively — and a
+    non-conforming replacement stream can raise anything at all, since
+    ``isatty`` is just an arbitrary attribute on an arbitrary object.
+
+    So the handler is deliberately broad rather than a list of the failures we
+    happened to think of. A process with no usable stream is by definition not
+    a human at a terminal, so every failure means the same thing: not a TTY.
+
+    This is the shared, fail-safe probe for every standard-stream TTY check on
+    the startup path — ``detect_caller`` below, ``Renderer.resolve``, and
+    ``tracking.prompt_tracking_consent``. All three run before argument
+    parsing, so an escaping exception would take down every command, including
+    ``--help`` and runs with tracking disabled.
+    """
+    if stream is None:
+        return False
+    try:
+        # The attribute LOOKUP is inside the try, not just the call: on a proxy
+        # or lazy wrapper stream, `isatty` can be a property or come from a
+        # `__getattr__`, either of which can raise. `getattr(..., None)` only
+        # swallows AttributeError, so a lookup that raised ValueError/OSError
+        # would escape a function whose whole contract is "never raises".
+        isatty = getattr(stream, "isatty", None)
+        if isatty is None:
+            return False
+        return bool(isatty())
+    except Exception:
+        return False
+
+
+def _stdout_is_tty() -> bool:
+    """``stream_is_tty`` against the live ``sys.stdout``, re-read on each call
+    so a test or wrapper that swaps the stream is honoured."""
+    return stream_is_tty(getattr(sys, "stdout", None))
+
+
 def detect_caller(
     env: Mapping[str, str] | None = None,
     *,
     is_tty: bool | None = None,
 ) -> Caller:
     e = env if env is not None else os.environ
-    tty = is_tty if is_tty is not None else sys.stdout.isatty()
 
     # 1. Explicit override — custom agent frameworks self-attribute here.
     explicit = e.get("COMFY_USER_AGENT")
@@ -63,7 +107,10 @@ def detect_caller(
     if _truthy(e.get("CLAUDECODE")):
         return Caller(kind="claude-code", agentic=True, source_env="CLAUDECODE")
 
-    # 4. Non-TTY — piped, backgrounded, or CI.
+    # 4. Non-TTY — piped, backgrounded, or CI. Probed lazily, only once the
+    #    env-var branches above have all declined: an explicitly-attributed
+    #    caller is answered without ever touching stdout.
+    tty = is_tty if is_tty is not None else _stdout_is_tty()
     if not tty:
         return Caller(kind="pipe", agentic=True, source_env=None)
 

comfy_cli/cmdline.py

@@ -12,6 +12,7 @@
 from comfy_cli import cancellation, constants, env_checker, logging, tracking, ui, utils
 from comfy_cli import where as where_module
 from comfy_cli.auth import command as auth_command
+from comfy_cli.caller import stream_is_tty
 from comfy_cli.cloud import command as cloud_command
 from comfy_cli.command import (
     code_search,
@@ -138,7 +139,11 @@ def _maybe_nudge_setup(ctx: typer.Context, renderer) -> None:
     install. Onboarding must never break a command — failures are swallowed.
     """
     sub = ctx.invoked_subcommand
-    if sub in (None, "setup") or not renderer.is_pretty() or not sys.stderr.isatty():
+    # Guarded stderr probe: this runs from the main Typer callback, and stderr
+    # can be closed independently of stdout (`comfy install 2>&-`, where CPython
+    # sets `sys.stderr = None`). A bare `.isatty()` there would kill the command
+    # from the onboarding nudge of all places. See `caller.stream_is_tty`.
+    if sub in (None, "setup") or not renderer.is_pretty() or not stream_is_tty(getattr(sys, "stderr", None)):
         return
     try:
         from comfy_cli.credentials import get_session

comfy_cli/command/run/__init__.py

@@ -22,7 +22,8 @@
     WebSocketTimeoutException,
 )
 
-from comfy_cli import cancellation, execution_errors, jobs_state
+from comfy_cli import cancellation, execution_errors, jobs_state, tracking
+from comfy_cli.caller import stream_is_tty
 
 # Re-exports — names patched by tests live at this namespace.
 from comfy_cli.command.run.credentials import _resolve_partner_credential as _resolve_partner_credential
@@ -52,23 +53,61 @@
 
 workspace_manager = WorkspaceManager()
 
+# Bounds on the `partner_nodes` telemetry property and on the partner-node names
+# echoed in the missing-credential error. class_type strings are attacker- (or
+# just accident-) controlled workflow JSON, so cap the list length and each name.
+_TELEMETRY_NODE_LIST_CAP = 20
+_TELEMETRY_NODE_NAME_MAX_LEN = 64
+
+
+def _bounded_node_names(names: list[str]) -> tuple[list[str], int]:
+    """Cap a partner-node name list in BOTH dimensions — element count and each
+    name's length. Returns ``(shown, omitted)``.
+
+    Applies to every payload built from these names: the telemetry property,
+    the prose message, and the structured ``details`` of both the credential
+    error and the spend gate. Capping only the prose would bound nothing —
+    ``error_panel`` renders ``details`` as ``key=value`` rows directly beneath
+    the message in pretty mode, and JSON mode serialises them. The exact total
+    travels alongside as ``partner_node_count``, so nothing is lost: a consumer
+    still learns how many nodes are involved, and the remedy is identical
+    whichever ones they are.
+
+    De-duplicates AFTER truncation, not before: ``_detect_partner_nodes``
+    returns distinct class_types, but two sharing a 64-char prefix collapse to
+    the same string once truncated, which would list one name twice.
+
+    Consumes input until the cap is filled with DISTINCT truncated names rather
+    than slicing the first 20 up front — otherwise a run of prefix-colliding
+    names burns output slots and silently drops later, genuinely distinct ones.
+
+    ``omitted`` counts only what the cap actually left behind, never what
+    de-duplication collapsed: a caller appending "and N more" must not claim
+    unlisted nodes that don't exist.
+    """
+    shown: list[str] = []
+    consumed = 0
+    for name in names:
+        if len(shown) >= _TELEMETRY_NODE_LIST_CAP:
+            break
+        consumed += 1
+        truncated = name[:_TELEMETRY_NODE_NAME_MAX_LEN]
+        if truncated not in shown:
+            shown.append(truncated)
+    return shown, len(names) - consumed
+
 
 def _stdin_is_interactive() -> bool:
     """True only when stdin is a live TTY.
 
     ``sys.stdin.isatty()`` assumes stdin is a live stream, but in detached /
-    ``pythonw`` contexts ``sys.stdin`` can be ``None`` (AttributeError on
-    ``.isatty``) or a closed file (ValueError). Treat both as non-interactive so
-    the spend gate falls through to the fail-closed machine-mode error instead
-    of raising an uncontrolled exception (BE-4326).
+    ``pythonw`` contexts ``sys.stdin`` can be ``None``, closed, or backed by a
+    revoked file descriptor. Treat every such case as non-interactive so the
+    spend gate falls through to the fail-closed machine-mode error instead of
+    raising an uncontrolled exception (BE-4326). Delegates to the shared
+    fail-safe probe so stdin and stdout are guarded identically.
     """
-    stdin = getattr(sys, "stdin", None)
-    if stdin is None:
-        return False
-    try:
-        return bool(stdin.isatty())
-    except (AttributeError, ValueError):
-        return False
+    return stream_is_tty(getattr(sys, "stdin", None))
 
 
 def _spend_gate(renderer, partner_nodes: list[str], allow_spend: bool, *, details: dict) -> None:
@@ -86,11 +125,18 @@ def _spend_gate(renderer, partner_nodes: list[str], allow_spend: bool, *, detail
     """
     if not partner_nodes or allow_spend:
         return
+    # Bound the names here rather than at each call site, so both `execute` and
+    # `execute_cloud` are covered: this gate runs on the same untrusted
+    # class_type strings and is the MORE commonly hit branch (it fires before
+    # any credential resolution), so leaving it unbounded would let a
+    # pathological graph flood the terminal and the JSON envelope anyway.
+    shown, omitted = _bounded_node_names(partner_nodes)
+    details = {**details, "partner_nodes": shown, "partner_node_count": len(partner_nodes)}
     if renderer.is_pretty() and _stdin_is_interactive():
         # Escape class_type names before interpolating into Rich markup: a name
         # containing markup like ``[bold]`` would otherwise be parsed as a tag
         # (MarkupError/StyleSyntaxError, or injected formatting).
-        names = ", ".join(_rich_escape(n) for n in partner_nodes)
+        names = ", ".join(_rich_escape(n) for n in shown) + (f", and {omitted} more" if omitted > 0 else "")
         pprint(f"[yellow]⚠ This workflow uses partner-API nodes that spend Comfy credits: {names}.[/yellow]")
         if not typer.confirm("Run anyway and spend credits?", default=False):
             renderer.error(
@@ -291,32 +337,68 @@ def execute(
     extra_data: dict | None = None
     if api_key:
         extra_data = {"api_key_comfy_org": api_key}
-    # Only resolve an injected credential when an explicit --api-key hasn't
-    # already satisfied the partner node: the resolver may perform a network
-    # OAuth refresh, so skipping it here keeps an explicit-key run network-free.
-    if partner_nodes and not extra_data:
-        cred = _resolve_partner_credential()
-        if cred is None:
-            msg = (
-                "Workflow uses partner-API node(s) that need an `api_key_comfy_org` "
-                "credential the local server doesn't have: " + ", ".join(partner_nodes) + "."
-            )
-            renderer.error(
-                code="partner_node_requires_credential",
-                message=msg,
-                hint=(
-                    "run: comfy cloud login   (or set COMFY_API_KEY in the environment, "
-                    "or persist a key with `comfy cloud set-key --key …`; "
-                    "cloud runs auto-inject via --where cloud)"
-                ),
-                details={
-                    "partner_nodes": partner_nodes,
-                    "host": host,
-                    "port": port,
-                },
-            )
-            raise typer.Exit(code=1)
-        extra_data = {cred[0]: cred[1]}
+    if partner_nodes:
+        # Only resolve an injected credential when an explicit --api-key hasn't
+        # already satisfied the partner node: the resolver may perform a network
+        # OAuth refresh, so skipping it here keeps an explicit-key run network-free.
+        # Resolved once — the result feeds both the telemetry prop below and the
+        # credential gate that follows.
+        cred = _resolve_partner_credential() if not extra_data else None
+        # Fired BEFORE the reject-for-missing-credential branch so runs that are
+        # turned away are still counted: that funnel is exactly what the metric
+        # is for, and `credential_present: False` marks them. class_types are
+        # node names, not PII — the same data `workflow_unknown_nodes` reports.
+        # It does sit AFTER the BE-4326 spend gate, so a run refused for lack of
+        # `--allow-spend` emits no event: the gate deliberately precedes any
+        # credential resolution (a refusal must not trigger a network OAuth
+        # refresh), and `credential_present` needs that resolution. The
+        # spend-declined funnel wants its own event rather than an early
+        # resolve here.
+        # class_type strings come verbatim from untrusted workflow JSON, so the
+        # names are bounded before they ship. The count stays exact, so the cap
+        # never distorts the metric.
+        bounded_nodes, omitted_nodes = _bounded_node_names(partner_nodes)
+        tracking.track_event(
+            "partner_nodes_detected",
+            {
+                "partner_nodes": bounded_nodes,
+                "partner_node_count": len(partner_nodes),
+                "where": "local",
+                "credential_present": bool(api_key) or cred is not None,
+            },
+        )
+        if not extra_data:
+            if cred is None:
+                # Same bounded list in the prose and in `details` — a graph with
+                # hundreds of partner nodes would otherwise render an unreadable
+                # wall of text, and `details` is rendered right below the message
+                # in pretty mode, so capping only one of them bounds nothing.
+                # `partner_node_count` carries the exact total for consumers.
+                # The suffix counts only what the CAP omitted — names collapsed
+                # by de-duplication are still listed, so counting them would
+                # promise unlisted nodes that don't exist.
+                listed = ", ".join(bounded_nodes) + (f", and {omitted_nodes} more" if omitted_nodes > 0 else "")
+                msg = (
+                    "Workflow uses partner-API node(s) that need an `api_key_comfy_org` "
+                    "credential the local server doesn't have: " + listed + "."
+                )
+                renderer.error(
+                    code="partner_node_requires_credential",
+                    message=msg,
+                    hint=(
+                        "run: comfy cloud login   (or set COMFY_API_KEY in the environment, "
+                        "or persist a key with `comfy cloud set-key --key …`; "
+                        "cloud runs auto-inject via --where cloud)"
+                    ),
+                    details={
+                        "partner_nodes": bounded_nodes,
+                        "partner_node_count": len(partner_nodes),
+                        "host": host,
+                        "port": port,
+                    },
+                )
+                raise typer.Exit(code=1)
+            extra_data = {cred[0]: cred[1]}
 
     # Pre-submit validation via pure-Python CQL engine (checks class_types + input shapes).
     _preflight_validate(renderer, workflow, object_info, target_label="server")

comfy_cli/output/renderer.py

@@ -22,7 +22,7 @@
 
 from rich.console import Console
 
-from comfy_cli.caller import Caller, detect_caller
+from comfy_cli.caller import Caller, detect_caller, stream_is_tty
 from comfy_cli.output.sanitize import sanitize_markup
 
 # Machine-output contract versions, surfaced in every envelope/event line and
@@ -111,7 +111,11 @@ def resolve(
         env_map = env if env is not None else os.environ
         caller = caller if caller is not None else detect_caller(env_map)
         if is_stdout_tty is None:
-            is_stdout_tty = sys.stdout.isatty()
+            # Guarded probe, not a bare `sys.stdout.isatty()`: this runs from
+            # the main Typer callback before any command dispatch, so under a
+            # detached / `pythonw` stdout a raising probe would kill every
+            # invocation — `comfy --help` included. See `caller.stream_is_tty`.
+            is_stdout_tty = stream_is_tty(getattr(sys, "stdout", None))
 
         mode: OutputMode
         if json_stream_flag:
@@ -376,8 +380,31 @@ def _envelope(
 
     def _write_json_line(self, payload: Mapping[str, Any]) -> None:
         line = json.dumps(payload, default=_json_default, ensure_ascii=False)
-        self.machine_stream.write(line + "\n")
-        self.machine_stream.flush()
+        stream = self.machine_stream
+        try:
+            stream.write(line + "\n")
+            stream.flush()
+        except (AttributeError, ValueError):
+            # Resolving to JSON mode against an unusable stdout must not merely
+            # DEFER the crash to the first emit — by then the command has
+            # already run and its side effects have landed, so dying here is
+            # strictly worse than dying at startup. `machine_stream` falls back
+            # to `sys.stdout`, which under pythonw / a detached parent is
+            # `None` (AttributeError on `.write`) or an already-closed file
+            # (ValueError). Neither can ever receive output, so the write is a
+            # no-op and the process still exits with the right code.
+            #
+            # `OSError` is deliberately NOT caught, even though it looks like it
+            # belongs. A `BrokenPipeError` here means the stream was real and the
+            # reader hung up, which is load-bearing: `comfy cloud login` relies
+            # on it propagating out of the `login_url` emit so the command fails
+            # fast instead of blocking the full 300s on a browser callback nobody
+            # will read (see test_json_login_fails_fast_when_login_url_write_breaks).
+            # Swallowing it would turn that into a silent hang.
+            #
+            # `TypeError` is likewise not caught: it would mean the payload is
+            # malformed, which is our bug and must stay visible.
+            return
 
     @property
     def exit_code(self) -> int: