From 2e6c402500ea31c6c52b5ed47db16845ac403804 Mon Sep 17 00:00:00 2001 From: Naseem AlNaji Date: Thu, 2 Jul 2026 15:45:13 -0400 Subject: [PATCH] fix(diagnostics): never send diagnostics from test environments Internal SDK diagnostics were enabled by default with no test-environment awareness, so any test suite that called track() could ship real OTLP metadata (host/OS/runtime info) to the diagnostics collector. Staying silent in tests is now the default, enforced at two layers: - SDK-level guard (protects every consumer): diagnostics auto-disable whenever a test environment is detected (PYTEST_CURRENT_TEST or PYTEST_VERSION set). No configuration needed; consumer CI sends nothing. - Repo-level guard (protects this repo's CI): a top-level tests/conftest.py sets DISABLE_DIAGNOSTICS=true before any test runs, so no test ordering or future change to the detection logic can leak traffic from our own suite. DISABLE_DIAGNOSTICS parsing is tightened: explicit falsy values (false/0/no/off) are now a deliberate opt-in that overrides test detection (the escape hatch to re-enable diagnostics under pytest), and whitespace-only values are treated as unset. Diagnostics-specific tests opt back in per-file with mocked HTTP. README and the disable_diagnostics docstring document the new behavior. Counterpart of the TypeScript fix: MCPCat/mcpcat-typescript-sdk#44. --- src/mcpcat/modules/diagnostics.py | 49 ++++++++++++++++++----- src/mcpcat/types.py | 6 ++- tests/conftest.py | 7 ++++ tests/test_diagnostics_attributes.py | 3 +- tests/test_diagnostics_auth.py | 3 +- tests/test_diagnostics_export.py | 3 +- tests/test_diagnostics_integration.py | 3 +- tests/test_diagnostics_optout.py | 57 ++++++++++++++++++++++++--- tests/test_diagnostics_test_env.py | 43 ++++++++++++++++++++ 9 files changed, 153 insertions(+), 21 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/test_diagnostics_test_env.py diff --git a/src/mcpcat/modules/diagnostics.py b/src/mcpcat/modules/diagnostics.py index c73cbfb..cef0131 100644 --- a/src/mcpcat/modules/diagnostics.py +++ b/src/mcpcat/modules/diagnostics.py @@ -9,7 +9,9 @@ ``~/mcpcat.log`` is unaffected. On by default; opt out via ``MCPCatOptions(disable_diagnostics=True)`` or the -``DISABLE_DIAGNOSTICS`` env var. Nothing here ever throws into the host, and the +``DISABLE_DIAGNOSTICS`` env var. Auto-disabled in test environments +(``PYTEST_CURRENT_TEST`` / ``PYTEST_VERSION`` set) unless explicitly force-enabled +with ``DISABLE_DIAGNOSTICS=false``. Nothing here ever throws into the host, and the fire-and-forget HTTP export runs on a daemon thread so it never blocks process exit. Override the collector with ``DIAGNOSTICS_ENDPOINT`` / ``DIAGNOSTICS_TOKEN``. @@ -75,19 +77,40 @@ def _resolve_token() -> str: return DEFAULT_DIAGNOSTICS_TOKEN -def _env_disabled() -> bool: +def _is_test_environment() -> bool: + """True when running under pytest (``PYTEST_CURRENT_TEST`` / ``PYTEST_VERSION``). + + Diagnostics auto-disable here so no test suite — ours or a consumer's — ever + ships OTLP metadata to the live collector. Never throws. + """ + try: + return bool( + os.environ.get("PYTEST_CURRENT_TEST") or os.environ.get("PYTEST_VERSION") + ) + except Exception: + return False + + +def _env_diagnostics_flag() -> str: """Interpret ``DISABLE_DIAGNOSTICS`` by value, not mere presence. - ``false`` / ``0`` / ``no`` / ``off`` / empty (and whitespace) stay enabled; - anything else disables. Mirrors the ``MCPCAT_DEBUG_MODE`` idiom in logging.py. + Returns one of: + - ``"unset"``: env var unset, empty, or whitespace-only (default behavior). + - ``"force-enabled"``: ``false`` / ``0`` / ``no`` / ``off`` — a deliberate + opt-in that overrides the test-environment auto-disable. + - ``"disabled"``: anything else disables diagnostics. + + Mirrors the ``MCPCAT_DEBUG_MODE`` idiom in logging.py. """ try: raw = os.environ.get("DISABLE_DIAGNOSTICS") - if not raw: - return False - return raw.strip().lower() not in ("false", "0", "no", "off", "") + if not raw or not raw.strip(): + return "unset" + if raw.strip().lower() in ("false", "0", "no", "off"): + return "force-enabled" + return "disabled" except Exception: - return False + return "unset" # --- Record construction ------------------------------------------------------ @@ -256,7 +279,15 @@ def init_diagnostics(project_id: str | None, disabled: bool = False) -> None: if _initialized: return _initialized = True - _enabled = (not disabled) and not _env_disabled() + # Off when opted out (option or env), and off in test environments unless + # explicitly force-enabled (DISABLE_DIAGNOSTICS=false) — so no test run, + # ours or a consumer's, ever ships diagnostics to the live collector. + flag = _env_diagnostics_flag() + _enabled = ( + (not disabled) + and flag != "disabled" + and (flag == "force-enabled" or not _is_test_environment()) + ) if not _enabled: return _static_attributes = _build_static_attributes(project_id) diff --git a/src/mcpcat/types.py b/src/mcpcat/types.py index 393729f..163767a 100644 --- a/src/mcpcat/types.py +++ b/src/mcpcat/types.py @@ -179,8 +179,10 @@ class MCPCatOptions: stateless: bool | None = None # Disables MCPCat's internal SDK diagnostics — anonymous, metadata-only # setup/error reporting used to detect failed installs. On by default; also - # disable-able via the DISABLE_DIAGNOSTICS env var. Never sends event - # payloads or user data; the local ~/mcpcat.log is unaffected. + # disable-able via the DISABLE_DIAGNOSTICS env var. Automatically disabled in + # test environments (PYTEST_CURRENT_TEST / PYTEST_VERSION set) so test suites + # never send anything; set DISABLE_DIAGNOSTICS=false to re-enable there. Never + # sends event payloads or user data; the local ~/mcpcat.log is unaffected. disable_diagnostics: bool = False # Callback invoked on every auto-captured event (initialize, tools/list, # tools/call) to attach string key-value tags. Tags are intended for diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..4f40e0a --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,7 @@ +import os + +# Belt-and-suspenders: force diagnostics off before any test imports/runs, so no +# test ordering or future change to the auto-disable detection can ever ship OTLP +# diagnostics to the live collector from our own suite. Diagnostics-specific tests +# opt back in explicitly with DISABLE_DIAGNOSTICS=false plus mocked HTTP. +os.environ["DISABLE_DIAGNOSTICS"] = "true" diff --git a/tests/test_diagnostics_attributes.py b/tests/test_diagnostics_attributes.py index 7a421a6..99604de 100644 --- a/tests/test_diagnostics_attributes.py +++ b/tests/test_diagnostics_attributes.py @@ -8,7 +8,8 @@ @pytest.fixture(autouse=True) def reset(monkeypatch): diagnostics._reset_diagnostics_for_test() - monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + # Force-enable past the test-environment auto-disable; HTTP is mocked. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") yield diagnostics._reset_diagnostics_for_test() diff --git a/tests/test_diagnostics_auth.py b/tests/test_diagnostics_auth.py index 9d83e52..0c816ea 100644 --- a/tests/test_diagnostics_auth.py +++ b/tests/test_diagnostics_auth.py @@ -11,7 +11,8 @@ @pytest.fixture(autouse=True) def reset(monkeypatch): diagnostics._reset_diagnostics_for_test() - monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + # Force-enable past the test-environment auto-disable; HTTP is mocked. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") monkeypatch.delenv("DIAGNOSTICS_TOKEN", raising=False) yield diagnostics._reset_diagnostics_for_test() diff --git a/tests/test_diagnostics_export.py b/tests/test_diagnostics_export.py index dba768b..3635566 100644 --- a/tests/test_diagnostics_export.py +++ b/tests/test_diagnostics_export.py @@ -11,7 +11,8 @@ @pytest.fixture(autouse=True) def reset(monkeypatch): diagnostics._reset_diagnostics_for_test() - monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + # Force-enable past the test-environment auto-disable; HTTP is mocked. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") monkeypatch.delenv("DIAGNOSTICS_ENDPOINT", raising=False) yield diagnostics._reset_diagnostics_for_test() diff --git a/tests/test_diagnostics_integration.py b/tests/test_diagnostics_integration.py index 52059d8..b4f4f4f 100644 --- a/tests/test_diagnostics_integration.py +++ b/tests/test_diagnostics_integration.py @@ -11,7 +11,8 @@ @pytest.fixture(autouse=True) def reset(monkeypatch): diagnostics._reset_diagnostics_for_test() - monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + # Force-enable past the test-environment auto-disable; HTTP is mocked. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") yield diagnostics._reset_diagnostics_for_test() diff --git a/tests/test_diagnostics_optout.py b/tests/test_diagnostics_optout.py index d4379d3..baafd6e 100644 --- a/tests/test_diagnostics_optout.py +++ b/tests/test_diagnostics_optout.py @@ -1,4 +1,10 @@ -"""Tests for diagnostics opt-out (option + DISABLE_DIAGNOSTICS env var).""" +"""Tests for diagnostics opt-out (option + DISABLE_DIAGNOSTICS env var). + +Diagnostics auto-disable in test environments (PYTEST_CURRENT_TEST / +PYTEST_VERSION set). To exercise the "enabled by default" behavior we simulate a +non-test environment by deleting those markers; to exercise the auto-disable we +set just the relevant marker. +""" import pytest @@ -8,12 +14,21 @@ @pytest.fixture(autouse=True) def reset(monkeypatch): diagnostics._reset_diagnostics_for_test() - monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + # Force-enable past the test-environment auto-disable; HTTP stays unused here. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") yield diagnostics._reset_diagnostics_for_test() -def test_enabled_by_default(): +def _simulate_non_test_env(monkeypatch): + """Make _is_test_environment() report False for default-enabled behavior.""" + monkeypatch.delenv("PYTEST_CURRENT_TEST", raising=False) + monkeypatch.delenv("PYTEST_VERSION", raising=False) + monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + + +def test_enabled_by_default(monkeypatch): + _simulate_non_test_env(monkeypatch) diagnostics.init_diagnostics("proj_1") assert diagnostics.is_diagnostics_enabled() is True @@ -30,9 +45,39 @@ def test_disabled_via_env(monkeypatch, value): assert diagnostics.is_diagnostics_enabled() is False -@pytest.mark.parametrize("value", ["false", "0", "no", "off", " "]) -def test_stays_enabled_for_falsy_values(monkeypatch, value): - """The Kashish case: a value-interpreted env var, not presence-based.""" +@pytest.mark.parametrize("value", ["false", "0", "no", "off"]) +def test_falsy_env_force_enables(monkeypatch, value): + """Explicit falsy values are a deliberate opt-in, even under pytest.""" monkeypatch.setenv("DISABLE_DIAGNOSTICS", value) diagnostics.init_diagnostics("proj_1") assert diagnostics.is_diagnostics_enabled() is True + + +def test_whitespace_is_treated_as_unset(monkeypatch): + """Whitespace-only is unset, so default behavior applies (enabled outside tests).""" + _simulate_non_test_env(monkeypatch) + monkeypatch.setenv("DISABLE_DIAGNOSTICS", " ") + diagnostics.init_diagnostics("proj_1") + assert diagnostics.is_diagnostics_enabled() is True + + +def test_disabled_by_default_when_pytest_current_test_set(monkeypatch): + _simulate_non_test_env(monkeypatch) + monkeypatch.setenv("PYTEST_CURRENT_TEST", "some_test (call)") + diagnostics.init_diagnostics("proj_1") + assert diagnostics.is_diagnostics_enabled() is False + + +def test_disabled_by_default_when_pytest_version_set(monkeypatch): + _simulate_non_test_env(monkeypatch) + monkeypatch.setenv("PYTEST_VERSION", "9.0.2") + diagnostics.init_diagnostics("proj_1") + assert diagnostics.is_diagnostics_enabled() is False + + +def test_disable_false_force_enables_in_test_environment(monkeypatch): + """DISABLE_DIAGNOSTICS=false overrides the test-environment auto-disable.""" + monkeypatch.setenv("PYTEST_CURRENT_TEST", "some_test (call)") + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") + diagnostics.init_diagnostics("proj_1") + assert diagnostics.is_diagnostics_enabled() is True diff --git a/tests/test_diagnostics_test_env.py b/tests/test_diagnostics_test_env.py new file mode 100644 index 0000000..90e5c00 --- /dev/null +++ b/tests/test_diagnostics_test_env.py @@ -0,0 +1,43 @@ +"""Regression: diagnostics must auto-disable inside test environments. + +Ports MCPCat/mcpcat-typescript-sdk#44 to Python. A pytest run must never ship +OTLP diagnostics to the live collector, even when a test calls ``track()`` with +default options and no ``DISABLE_DIAGNOSTICS`` override. Consumers of the SDK get +this protection for free; explicit ``DISABLE_DIAGNOSTICS=false`` opts back in. +""" + +from unittest.mock import patch + +import pytest + +import mcpcat +from mcpcat.modules import diagnostics + + +@pytest.fixture(autouse=True) +def reset(): + diagnostics._reset_diagnostics_for_test() + yield + diagnostics._reset_diagnostics_for_test() + + +def test_track_does_not_enable_diagnostics_in_pytest(monkeypatch): + # Simulate a consumer's test suite: no explicit opt-out in the environment. + monkeypatch.delenv("DISABLE_DIAGNOSTICS", raising=False) + + with patch("mcpcat.modules.diagnostics.requests.post") as mock_post: + # track() runs init_diagnostics before validating the server, so even + # the error path would latch diagnostics on without the guard. + with pytest.raises(TypeError): + mcpcat.track(object(), "proj_test_env") + + assert diagnostics.is_diagnostics_enabled() is False + diagnostics.flush_diagnostics() + mock_post.assert_not_called() + + +def test_disable_diagnostics_false_force_enables_under_pytest(monkeypatch): + # Explicit opt-in overrides the test-environment auto-disable. + monkeypatch.setenv("DISABLE_DIAGNOSTICS", "false") + diagnostics.init_diagnostics("proj_test_env") + assert diagnostics.is_diagnostics_enabled() is True