fix(telemetry): keep telemetry inert during shell completion (BE-4274)

comfy_cli/tracking.py

@@ -121,6 +121,41 @@ def _telemetry_disabled_by_env() -> bool:
     return False
 
 
+# Click/Typer completion instruction tokens. The ``_*_COMPLETE`` var carries an
+# ``instruction_shell`` pair whose instruction is ``complete`` (resolve args) or
+# ``source`` (emit the completion script) — neither runs a command.
+_COMPLETION_INSTRUCTIONS = frozenset({"complete", "source"})
+
+
+def _in_shell_completion() -> bool:
+    """Return True when the process is resolving shell tab-completion rather
+    than running a real command.
+
+    Click/Typer trigger completion by re-invoking the CLI with a
+    ``_<PROG_NAME>_COMPLETE`` environment variable set (e.g. ``_COMFY_COMPLETE``
+    under fish, bash, and zsh). No command actually runs on that path, so there
+    is no telemetry to send — detecting it lets us skip standing up the PostHog
+    client (a background thread + network setup), which is wasted work on an
+    inert path (GitHub #506). The prog name varies with the invoking entrypoint
+    (``comfy`` / ``comfy-cli`` / ``comfycli``) and any user alias, so match the
+    ``_..._COMPLETE`` pattern rather than a fixed name.
+
+    The var's value is Click/Typer's completion *instruction* — ``complete_bash``
+    / ``source_zsh`` (Typer 8.x style) or ``bash_complete`` (Click 7.x style),
+    i.e. an ``instruction_shell`` / ``shell_instruction`` pair. Require a
+    recognized instruction token so a stray or empty user-exported
+    ``_FOO_COMPLETE`` can't silently suppress telemetry on a real command run.
+    Snapshot the keys with ``list(...)`` so a concurrent env mutation on another
+    thread can't raise ``RuntimeError: dictionary changed size during iteration``.
+    """
+    for name in list(os.environ):
+        if not (name.startswith("_") and name.endswith("_COMPLETE")):
+            continue
+        if _COMPLETION_INSTRUCTIONS & set(os.environ.get(name, "").split("_")):
+            return True
+    return False
+
+
 def _consent_enabled() -> bool:
     """Whether passive telemetry may be sent right now: no env opt-out AND the
     user has consented (persisted flag) or a session-only opt-in is active.
@@ -236,6 +271,14 @@ def flush(self) -> None:
 
 def _get_providers() -> list[TelemetryProvider]:
     global PROVIDERS
+    # Shell tab-completion (fish/bash/zsh) resolves the command tree without ever
+    # running a command, so nothing is sent — don't stand up (or even import) the
+    # telemetry SDKs on that inert path (GitHub #506). Today's lazy construction
+    # already keeps completion inert because _dispatch never runs there; this makes
+    # that guarantee explicit and independent of *when* providers get built.
+    # Returned uncached so a real invocation is never affected.
+    if _in_shell_completion():
+        return []
     if PROVIDERS is None:
         with _PROVIDERS_LOCK:
             if PROVIDERS is None:

tests/comfy_cli/test_tracking_completion.py

@@ -0,0 +1,153 @@
+"""Regression tests for GitHub #506 — telemetry must stay inert during shell
+tab-completion.
+
+Under fish/bash/zsh, every tab-completion of a ``comfy ...`` command re-invokes
+the CLI with a ``_<PROG_NAME>_COMPLETE`` environment variable set. No command
+runs on that path, so no telemetry is ever sent — standing up the PostHog client
+(a background thread + network setup) there is pure waste. These tests lock in
+that the telemetry providers are never constructed while a completion env var is
+present, at both the construction gateway (:func:`_get_providers`) and end to end
+through the real Typer completion resolution.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+import comfy_cli.tracking as tracking_mod
+
+
+class TestInShellCompletionDetection:
+    @pytest.mark.parametrize(
+        "env_var",
+        [
+            # Typer's own completion instruction var, per entrypoint / shell.
+            "_COMFY_COMPLETE",  # `comfy`
+            "_COMFY_CLI_COMPLETE",  # `comfy-cli`
+            "_COMFYCLI_COMPLETE",  # `comfycli`
+            "_SOMEOTHERTOOL_COMPLETE",  # a user alias
+        ],
+    )
+    def test_completion_env_var_is_detected(self, monkeypatch, env_var):
+        monkeypatch.setenv(env_var, "complete_fish")
+        assert tracking_mod._in_shell_completion() is True
+
+    @pytest.mark.parametrize("shell_instruction", ["complete_fish", "complete_bash", "complete_zsh"])
+    def test_detected_across_shells(self, monkeypatch, shell_instruction):
+        # The value differs per shell; detection keys off the var name, so any of
+        # them trips the guard.
+        monkeypatch.setenv("_COMFY_COMPLETE", shell_instruction)
+        assert tracking_mod._in_shell_completion() is True
+
+    @pytest.mark.parametrize(
+        "name",
+        [
+            "PATH",
+            "AUTO_COMPLETE",  # no leading underscore
+            "_COMFY_HOME",  # underscore-prefixed but not a completion var
+            "COMPLETE",
+        ],
+    )
+    def test_non_completion_env_is_not_detected(self, monkeypatch, name):
+        _clear_completion_env(monkeypatch)
+        monkeypatch.setenv(name, "1")
+        assert tracking_mod._in_shell_completion() is False
+
+    @pytest.mark.parametrize("value", ["", "0", "1", "garbage", "banana"])
+    def test_completion_var_with_non_instruction_value_is_not_detected(self, monkeypatch, value):
+        # A stray or empty user-exported `_FOO_COMPLETE` (no real completion
+        # instruction in its value) must NOT be mistaken for completion mode,
+        # else it would silently suppress telemetry on a genuine command run.
+        _clear_completion_env(monkeypatch)
+        monkeypatch.setenv("_SOMETOOL_COMPLETE", value)
+        assert tracking_mod._in_shell_completion() is False
+
+    @pytest.mark.parametrize("value", ["complete_bash", "source_zsh", "bash_complete", "complete_powershell"])
+    def test_recognized_instruction_values_are_detected(self, monkeypatch, value):
+        # Both the Typer 8.x (`instruction_shell`) and Click 7.x (`shell_instruction`)
+        # value styles carry a recognized instruction token and must trip the guard.
+        _clear_completion_env(monkeypatch)
+        monkeypatch.setenv("_COMFY_COMPLETE", value)
+        assert tracking_mod._in_shell_completion() is True
+
+
+class TestGetProvidersUnderCompletion:
+    def test_no_providers_constructed_during_completion(self, monkeypatch):
+        """The core acceptance criterion: with a completion env var set,
+        :func:`_get_providers` must return no providers and never invoke a
+        provider factory (which is what imports the SDKs and builds the PostHog
+        client)."""
+        monkeypatch.setenv("_COMFY_COMPLETE", "complete_fish")
+        # Start from a clean cache so we exercise the construction path.
+        monkeypatch.setattr(tracking_mod, "PROVIDERS", None)
+
+        def _boom(*_a, **_k):
+            raise AssertionError("telemetry provider constructed during shell completion")
+
+        with (
+            patch.object(tracking_mod, "MixpanelProvider", _boom),
+            patch.object(tracking_mod, "PostHogProvider", _boom),
+        ):
+            providers = tracking_mod._get_providers()
+
+        assert providers == []
+        # The guard is intentionally uncached so a real invocation is unaffected.
+        assert tracking_mod.PROVIDERS is None
+
+    def test_providers_constructed_when_not_completing(self, monkeypatch):
+        """Control: without a completion env var, providers build as before."""
+        _clear_completion_env(monkeypatch)
+        monkeypatch.setattr(tracking_mod, "PROVIDERS", None)
+
+        mp, ph = MagicMock(name="mixpanel"), MagicMock(name="posthog")
+        with (
+            patch.object(tracking_mod, "MixpanelProvider", MagicMock(return_value=mp)),
+            patch.object(tracking_mod, "PostHogProvider", MagicMock(return_value=ph)),
+        ):
+            providers = tracking_mod._get_providers()
+
+        assert providers == [mp, ph]
+
+
+class TestCompletionEndToEnd:
+    @pytest.mark.parametrize("shell_instruction", ["complete_fish", "complete_bash", "complete_zsh"])
+    def test_real_completion_never_constructs_posthog(self, monkeypatch, tmp_path, shell_instruction):
+        """Drive the actual Typer completion resolution the way a shell does and
+        assert the PostHog client is never constructed. This is the end-to-end
+        proof of GitHub #506's fix across fish/bash/zsh."""
+        # Isolate config so completion can't touch the real user config.
+        monkeypatch.setenv("COMFY_HOME", str(tmp_path))
+        monkeypatch.setenv("HOME", str(tmp_path))
+        # Typer's completion protocol: instruction + the words resolved so far.
+        # The var each shell reads to recover those words differs — zsh/fish use
+        # _TYPER_COMPLETE_ARGS, while bash reads COMP_WORDS/COMP_CWORD (a hard
+        # KeyError if absent) — so set all of them to emulate ``comfy <TAB>``
+        # regardless of which shell instruction is under test.
+        monkeypatch.setenv("_COMFY_COMPLETE", shell_instruction)
+        monkeypatch.setenv("_TYPER_COMPLETE_ARGS", "comfy ")
+        monkeypatch.setenv("_TYPER_COMPLETE_FISH_ACTION", "get-args")
+        monkeypatch.setenv("COMP_WORDS", "comfy ")
+        monkeypatch.setenv("COMP_CWORD", "1")
+        monkeypatch.setattr("sys.argv", ["comfy"])
+        monkeypatch.setattr(tracking_mod, "PROVIDERS", None)
+
+        constructed = MagicMock(side_effect=AssertionError("PostHog client constructed during completion"))
+        from comfy_cli.cmdline import main
+
+        with patch.object(tracking_mod.PostHogProvider, "__init__", constructed):
+            with pytest.raises(SystemExit) as exc:
+                main()
+
+        # Completion resolves successfully (exit 0) without ever touching PostHog.
+        assert exc.value.code == 0
+        constructed.assert_not_called()
+
+
+def _clear_completion_env(monkeypatch):
+    """Remove any ``_*_COMPLETE`` var the ambient environment might carry so the
+    negative/control cases start from a known non-completion state."""
+    import os
+
+    for key in list(os.environ):
+        if key.startswith("_") and key.endswith("_COMPLETE"):
+            monkeypatch.delenv(key, raising=False)