From d5384fd01febf72e9c68f2d41c232174ab5f9c10 Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Wed, 12 Aug 2026 18:35:19 -0700 Subject: [PATCH 1/2] fix(tests): redirect the saved session under every test runner conftest.py redirects XDG_CONFIG_HOME so a test that reaches `sp auth login` cannot overwrite the developer's real session. That guard only works under pytest: conftest is a pytest mechanism, and the suite is also run with unittest and nose2, where the fixture is never loaded and the protection silently does nothing. It is not theoretical. The docstring already records one live token lost this way, and it has now happened a second time, with `python -m unittest discover -s tests`. The plaintext token is returned only at creation, so an overwritten session cannot be recovered. Move the redirection into the tests package __init__, which pytest, nose2 and `python -m unittest discover` from the repository root all import before collecting anything. `discover -s tests` imports test modules top-level and never imports the package, so each test module also imports SESSION_SANDBOX explicitly, which pulls the package in whichever way the suite was started. conftest.py is left in place: under pytest it still narrows this to a per-test directory, and overriding an already-redirected variable is harmless. Verified by planting a sentinel in ~/.config/sp/config.json and running the suite three ways -- `discover -s tests`, `discover` from the root, and pytest. 210 tests pass and the sentinel is untouched in each; before this change the first of those overwrote it. --- tests/__init__.py | 40 +++++++++++++++++++++++++++++++++++++++- tests/test_classifier.py | 2 ++ tests/test_cli.py | 2 ++ tests/test_client.py | 2 ++ tests/test_compare.py | 2 ++ tests/test_history.py | 2 ++ tests/test_triage.py | 2 ++ tests/test_ux.py | 2 ++ 8 files changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/__init__.py b/tests/__init__.py index ccccefe..a393c18 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1 +1,39 @@ -"""Tests for the sp CLI (sp_cli).""" +"""Tests for the sp CLI (sp_cli). + +Redirect the saved-session location before any test touches it. + +``sp auth login`` writes a bearer token to ``$XDG_CONFIG_HOME/sp/config.json``, +so a test reaching that code path writes to the developer's *real* config. The +plaintext token is returned only at creation, so an overwritten session is +unrecoverable -- and this has now cost a live token twice. + +``tests/conftest.py`` already guards this, but only under pytest: conftest is a +pytest mechanism, and the suite is also run with ``unittest`` and ``nose2``, +where the fixture is never loaded and the guard silently does nothing. + +Putting the redirect in the package ``__init__`` covers pytest, ``nose2``, and +``python -m unittest discover`` from the repository root. It does *not* cover +``python -m unittest discover -s tests``, which imports test modules top-level +and never imports this package -- which is why every test module imports +``SESSION_SANDBOX`` below explicitly. ``conftest.py`` still narrows this to a +per-test directory under pytest; overriding an already-redirected variable is +harmless. +""" + +import atexit +import os +import shutil +import tempfile + +#: Throwaway root standing in for the developer's home during tests. Test +#: modules import this name so that importing them runs the redirect below, +#: whichever runner collected them. +SESSION_SANDBOX = tempfile.mkdtemp(prefix='sp-cli-tests-') + +# HOME as well as XDG_CONFIG_HOME: config_path() falls back to ~/.config when +# XDG_CONFIG_HOME is unset, so redirecting only the latter leaves a gap. +os.environ['XDG_CONFIG_HOME'] = os.path.join(SESSION_SANDBOX, 'config') +os.environ['HOME'] = os.path.join(SESSION_SANDBOX, 'home') +os.makedirs(os.environ['HOME'], exist_ok=True) + +atexit.register(shutil.rmtree, SESSION_SANDBOX, True) diff --git a/tests/test_classifier.py b/tests/test_classifier.py index 34df46c..8113968 100644 --- a/tests/test_classifier.py +++ b/tests/test_classifier.py @@ -1,5 +1,7 @@ """Tests for the rule-based failure classifier, using real examples from run #9299.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import unittest from sp_cli import classifier diff --git a/tests/test_cli.py b/tests/test_cli.py index e299358..44fd75f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,5 +1,7 @@ """Tests for the sp CLI command surface, mocking the API client.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import json import unittest from unittest import mock diff --git a/tests/test_client.py b/tests/test_client.py index 90acb7f..aeac3a8 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,5 +1,7 @@ """Tests for the CLI's HTTP client, mocking the requests session.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import unittest from unittest import mock diff --git a/tests/test_compare.py b/tests/test_compare.py index ce8ff1c..efbf33b 100644 --- a/tests/test_compare.py +++ b/tests/test_compare.py @@ -1,5 +1,7 @@ """Tests for the run-to-run failure diff behind ``sp run compare``.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import unittest from sp_cli import compare diff --git a/tests/test_history.py b/tests/test_history.py index 5f63204..6b84b3a 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -1,5 +1,7 @@ """Tests for the cross-run history verdicts behind ``sp investigate --with-history``.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import unittest from sp_cli.history import (FLAKY, NEVER_PASSED, NEW_REGRESSION, NO_HISTORY, diff --git a/tests/test_triage.py b/tests/test_triage.py index 5a570c9..4d54b7f 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -1,5 +1,7 @@ """Tests for the triage helpers that adapt RunSample results into failure rows.""" +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import unittest from sp_cli import triage diff --git a/tests/test_ux.py b/tests/test_ux.py index 1f493cd..a9005bf 100644 --- a/tests/test_ux.py +++ b/tests/test_ux.py @@ -3,6 +3,8 @@ The rule these all share: a human-only effect must never reach machine output. """ +from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first + import json import os import stat From d4f40e49a46e1d4ebacadfd32e63f902c96f7eae Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Wed, 12 Aug 2026 19:42:32 -0700 Subject: [PATCH 2/2] fix: satisfy isort, and stop claiming the sandbox import must come first The sandbox import carried a "keep first" comment and a trailing explanation that pushed the line past the wrap limit, so isort reformatted it into a backslash continuation and the check job failed on the resulting diff. The comment was also wrong. `config.config_path()` reads XDG_CONFIG_HOME when it is called, not when it is imported, so nothing depends on this import preceding the others -- and `tests/__init__.py` runs before any submodule body regardless, because Python imports the parent package first. Shortening the comment lets isort sort the line wherever it belongs. --- tests/test_classifier.py | 3 +-- tests/test_cli.py | 3 +-- tests/test_client.py | 3 +-- tests/test_compare.py | 3 +-- tests/test_history.py | 3 +-- tests/test_triage.py | 3 +-- tests/test_ux.py | 3 +-- 7 files changed, 7 insertions(+), 14 deletions(-) diff --git a/tests/test_classifier.py b/tests/test_classifier.py index 8113968..d9d3f05 100644 --- a/tests/test_classifier.py +++ b/tests/test_classifier.py @@ -1,10 +1,9 @@ """Tests for the rule-based failure classifier, using real examples from run #9299.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import unittest from sp_cli import classifier +from tests import SESSION_SANDBOX # noqa: F401 class ClassifierTests(unittest.TestCase): diff --git a/tests/test_cli.py b/tests/test_cli.py index 44fd75f..55b6915 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,7 +1,5 @@ """Tests for the sp CLI command surface, mocking the API client.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import json import unittest from unittest import mock @@ -10,6 +8,7 @@ from sp_cli.client import ApiError from sp_cli.main import cli +from tests import SESSION_SANDBOX # noqa: F401 RUNS_PAGE = { 'data': [{'run_id': 9299, 'status': 'fail', 'platform': 'windows', 'commit_sha': 'e6cd34e'}], diff --git a/tests/test_client.py b/tests/test_client.py index aeac3a8..67ffce4 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,13 +1,12 @@ """Tests for the CLI's HTTP client, mocking the requests session.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import unittest from unittest import mock import requests # type: ignore[import-untyped] from sp_cli.client import ApiClient, ApiError +from tests import SESSION_SANDBOX # noqa: F401 class FakeResponse: diff --git a/tests/test_compare.py b/tests/test_compare.py index efbf33b..c14ae8d 100644 --- a/tests/test_compare.py +++ b/tests/test_compare.py @@ -1,10 +1,9 @@ """Tests for the run-to-run failure diff behind ``sp run compare``.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import unittest from sp_cli import compare +from tests import SESSION_SANDBOX # noqa: F401 def sample(test_id, status, exit_code=0, expected_rc=0, outputs=None): diff --git a/tests/test_history.py b/tests/test_history.py index 6b84b3a..347cc48 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -1,12 +1,11 @@ """Tests for the cross-run history verdicts behind ``sp investigate --with-history``.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import unittest from sp_cli.history import (FLAKY, NEVER_PASSED, NEW_REGRESSION, NO_HISTORY, STILL_FAILING, UNKNOWN, classify_history, group_by_verdict, split_history, unknown_history) +from tests import SESSION_SANDBOX # noqa: F401 def entry(run_id, status, regression_test_id=137, signature=None): diff --git a/tests/test_triage.py b/tests/test_triage.py index 4d54b7f..0f28da9 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -1,10 +1,9 @@ """Tests for the triage helpers that adapt RunSample results into failure rows.""" -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import unittest from sp_cli import triage +from tests import SESSION_SANDBOX # noqa: F401 class IsFailureTests(unittest.TestCase): diff --git a/tests/test_ux.py b/tests/test_ux.py index a9005bf..c3c1f35 100644 --- a/tests/test_ux.py +++ b/tests/test_ux.py @@ -3,8 +3,6 @@ The rule these all share: a human-only effect must never reach machine output. """ -from tests import SESSION_SANDBOX # noqa: F401 # redirects the saved session; keep first - import json import os import stat @@ -18,6 +16,7 @@ from sp_cli import config from sp_cli.main import cli from sp_cli.output import render +from tests import SESSION_SANDBOX # noqa: F401 CLASSIFIED_ROWS = { 'data': [