diff --git a/comfy_cli/tracking.py b/comfy_cli/tracking.py index 1516cfbb..bed5a2b4 100644 --- a/comfy_cli/tracking.py +++ b/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 + ``__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: diff --git a/tests/comfy_cli/test_tracking_completion.py b/tests/comfy_cli/test_tracking_completion.py new file mode 100644 index 00000000..5efd6718 --- /dev/null +++ b/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 ``__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 `` + # 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)