Issue #130: Close the guard bypasses red-teaming found, and fix my own claims

Red-teaming the branch found that my Step 1 change traded a false refusal
for two real money-safety holes. Verified against master, which refused
both:

    PYTEST_ADDOPTS=tests/integration/test_x.py pytest    -> collected 2
    pytest -o testpaths=tests/integration/test_x.py      -> collected 2

Cause: `config.invocation_params.args` is only what the operator typed.
`testpaths`, `-o testpaths=` and PYTEST_ADDOPTS all feed `config.args`
without appearing there. Reading the typed argv makes the guard blind to
every indirect route.

The right fix was in the config, not the guard. Reading `config.args` is
safe now precisely because Step 2 set `testpaths = ["tests"]`, so the
guard is back on the effective target list. Also fixed, and pre-existing
(#109): relative paths were resolved only against rootdir, so from within
tests/ the path `integration/test_x.py` went unrecognised. All plausible
bases are now tried. `--pyargs` and `-p` address modules by dotted name and
never looked like paths; both are now translated and checked.

Measured after the change -- refused: explicit dir, explicit file, node id,
-o testpaths, PYTEST_ADDOPTS, --pyargs, cwd-relative. Still allowed and
must be: bare `pytest` (0 integration nodes collected) and the
CLUSTRIX_ALLOW_BILLABLE=1 opt-in. `-p` cannot be blocked before the import
it triggers -- no conftest hook runs that early -- but the run is now
refused before any test executes, which is where the cost is; it also fails
on its own here, since tests/ is not a package.

Four parametrized regression tests cover the indirect routes. Three fail
against the old guard, confirming they are not decorative.

Corrections to my own earlier work in this PR:

* "the 224 in tests/real_world/" was wrong and had been committed as a
  code comment in scripts/pre_push_check.py. 224 counts decorator lines;
  tests/real_world/ collects 388 tests. That comment's rationale was also
  wrong: the old bare `pytest` was not "harmless while no config applied",
  it aborted on master in seconds with a collection error and gated
  nothing. Fixing that SyntaxError is what let collection succeed and made
  the runtime problem visible.
* test_no_shadowing_config_file_exists forbade tox.ini and setup.cfg.
  pytest checks those *after* pyproject.toml, so neither can shadow it --
  verified. The test also failed on an ordinary pytest-free setup.cfg
  holding flake8 config. Now limited to pytest.ini/.pytest.ini, and it
  checks the repo tree rather than the resolved rootdir.
* test_bare_pytest_is_not_refused_by_the_guard described the guard source
  that no longer exists; rewritten to state the actual invariant.
* coverage_detailed_report.txt was removed from .gitignore: nothing in the
  repo writes it, so the justification given was unsupported.

Unrelated CI fix found while verifying: .github/workflows/fast_ci.yml
installs pytest without pytest-timeout, then runs pytest with
--timeout=60. It exits 4 with "unrecognized arguments" and has been
failing on every run, including on master. Added the plugin.

Restored the `filterwarnings` block from the deleted pytest.ini, which was
never migrated. It only suppresses third-party paramiko/cryptography noise.
The five markers pytest.ini also declared are deliberately not migrated:
tests/real_world/conftest.py registers them with addinivalue_line where
they are used.

tests/unit/ 79 passed. 1674 collected, 0 errors. flake8 91 (master: 92).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU

Author: jeremymanning

.github/workflows/fast_ci.yml

@@ -39,7 +39,11 @@ jobs:
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip
-          pip install black flake8 mypy pytest
+          # pytest-timeout is required: the test step below passes
+          # --timeout=60, and without the plugin pytest exits 4 with
+          # "unrecognized arguments". This job had been failing on every run
+          # for that reason (see #130).
+          pip install black flake8 mypy pytest pytest-timeout
           pip install -e .
       
       - name: Format check with Black

.gitignore

@@ -26,7 +26,6 @@ test-reports/
 .coverage
 .coverage.*
 coverage.json
-coverage_detailed_report.txt
 htmlcov/
 .tox/
 # Written by the benchmark tests in tests/real_world/ at run time.

pyproject.toml

@@ -219,6 +219,19 @@ markers = [
     "dartmouth_network: marks tests that require access to the Dartmouth campus network",
     "performance: marks performance benchmark tests",
 ]
+# Carried over from the deleted pytest.ini, where it never took effect. These
+# suppress third-party noise only; nothing here hides a warning raised by
+# clustrix itself.
+# NB: markers visual/ssh_required/aws_required/azure_required/gcp_required are
+# deliberately NOT listed. tests/real_world/conftest.py registers them with
+# addinivalue_line at the point they are used, so --strict-markers is satisfied
+# there and they stay scoped to the suite that actually defines them.
+filterwarnings = [
+    "ignore::DeprecationWarning",
+    "ignore::PendingDeprecationWarning",
+    "ignore::UserWarning:paramiko.*",
+    "ignore::UserWarning:cryptography.*",
+]
 
 [tool.coverage.run]
 source = ["clustrix"]

scripts/pre_push_check.py

@@ -43,20 +43,23 @@ def main():
 
         checks = [
             ("black clustrix/ tests/", "Black formatting"),  # Format, don't just check
-            ("flake8 clustrix/ tests/ --max-line-length=88 --extend-ignore=E203,W503,F401,E722,F541,F841,F811,E731,E501,W291,W293,F824", "Flake8 linting"),
+            (
+                "flake8 clustrix/ tests/ --max-line-length=88 --extend-ignore=E203,W503,F401,E722,F541,F841,F811,E731,E501,W291,W293,F824",
+                "Flake8 linting",
+            ),
             ("mypy clustrix/", "MyPy type checking"),
             # Must mirror what GitHub Actions actually runs (see
             # .github/workflows/tests.yml), or this script cannot deliver on
             # the promise in its own docstring.
             #
-            # This was a bare `pytest`, which was harmless only for as long as
-            # the project had no effective pytest config: pytest.ini shadowed
-            # pyproject.toml and neither applied, so `testpaths` did nothing
-            # (see #130). With the config live, a bare `pytest` resolves to
-            # testpaths and pulls in all 1670 tests -- including the 224 in
-            # tests/real_world/, which make live network and cloud calls and do
-            # not terminate in a reasonable time. Real-world tests are run
-            # deliberately via scripts/run_real_world_tests.py, not here.
+            # This was a bare `pytest`, which gated nothing: on master it
+            # aborted in seconds with "Interrupted: 1 error during collection"
+            # (an f-string SyntaxError under Python < 3.12) and ran zero tests.
+            # Fixing that error is what made the problem visible -- collection
+            # then succeeded and a bare `pytest` ran all 1670 tests, including
+            # the 388 in tests/real_world/, which open live SSH and cloud
+            # connections. Real-world tests are run deliberately through
+            # scripts/run_real_world_tests.py, not from this gate.
             ('pytest tests/unit/ -m "not real_world"', "Tests"),
         ]
 

tests/conftest.py

@@ -15,6 +15,66 @@ def _billable_tests_enabled():
     return os.environ.get(_OPT_IN_VAR, "").strip().lower() in _TRUTHY
 
 
+def _iter_candidate_targets(config):
+    """Yield every path-ish string this run could end up collecting from.
+
+    Reads `config.args` -- the *effective* target list -- rather than
+    `config.invocation_params.args`, which is only what the operator literally
+    typed. The difference is the whole ballgame, because there are three ways
+    to aim pytest at a directory without typing its path:
+
+        PYTEST_ADDOPTS=tests/integration/test_x.py pytest
+        pytest -o testpaths=tests/integration/test_x.py
+        (testpaths in pyproject.toml)
+
+    All three land in `config.args` and none appear in `invocation_params.args`.
+    Red-teaming confirmed the first two collected billable modules when the
+    guard read the typed argv.
+
+    Reading `config.args` is only safe because `testpaths` is `["tests"]`. If
+    it ever names tests/integration again, a bare `pytest` will be refused --
+    loudly and correctly, since testpaths would then be pointing every default
+    run at billable tests.
+    """
+    for arg in config.args:
+        yield str(arg).split("::")[0]
+    # --pyargs addresses modules by dotted name, which never looks like a path.
+    if getattr(config.option, "pyargs", False):
+        for arg in config.args:
+            yield str(arg).split("::")[0].replace(".", os.sep)
+    # -p imports a plugin by dotted name, before collection begins.
+    for plugin in getattr(config.option, "plugins", None) or []:
+        yield str(plugin).replace(".", os.sep)
+
+
+def _targets_integration_dir(candidate, config):
+    """True if `candidate` names tests/integration under any sane resolution.
+
+    Paths on the command line resolve against the invocation directory, while
+    `testpaths` resolves against rootdir. Those differ whenever pytest is run
+    from a subdirectory, and resolving against only one of them was a real
+    hole: from `tests/`, `pytest integration/test_x.py` was not recognised.
+
+    Both bases are tried. A guard protecting real money should over-match
+    rather than under-match.
+    """
+    raw = pathlib.Path(candidate)
+    if raw.is_absolute():
+        attempts = [raw]
+    else:
+        invocation_dir = getattr(config.invocation_params, "dir", None)
+        bases = [invocation_dir, pathlib.Path.cwd(), pathlib.Path(str(config.rootpath))]
+        attempts = [pathlib.Path(str(base)) / raw for base in bases if base]
+    for attempt in attempts:
+        try:
+            resolved = attempt.resolve()
+        except OSError:  # pragma: no cover - defensive, unresolvable path
+            continue
+        if resolved == _INTEGRATION_DIR or _INTEGRATION_DIR in resolved.parents:
+            return True
+    return False
+
+
 def pytest_configure(config):
     """Refuse to start when a run explicitly targets tests/integration.
 
@@ -35,31 +95,20 @@ def pytest_configure(config):
     these tests by name gets told why they got nothing, rather than an
     inscrutable empty run.
 
-    Reads `config.invocation_params.args` -- what the operator actually typed --
-    and NOT `config.args`. The two differ in exactly the case that matters: when
-    no path is given on the command line, pytest populates `config.args` from
-    `testpaths` in pyproject.toml, which lists `tests`. Keying off `config.args`
-    therefore made a bare `pytest` abort with this very error the moment the
-    project's config became effective (see #130). `invocation_params.args`
-    contains flags as well as paths, so entries starting with "-" are skipped.
+    Target discovery is in `_iter_candidate_targets` and path matching is in
+    `_targets_integration_dir`; both carry the reasoning for why they look
+    where they do. In short: read the *effective* target list, and resolve
+    relative paths against every plausible base.
     """
     if _billable_tests_enabled():
         return
-    for arg in config.invocation_params.args:
-        if str(arg).startswith("-"):
-            continue
-        # strip pytest's "::TestClass::test_name" node-id suffix
-        candidate = pathlib.Path(str(arg).split("::")[0])
-        if not candidate.is_absolute():
-            candidate = (pathlib.Path(str(config.rootpath)) / candidate).resolve()
-        else:
-            candidate = candidate.resolve()
-        if candidate == _INTEGRATION_DIR or _INTEGRATION_DIR in candidate.parents:
+    for candidate in _iter_candidate_targets(config):
+        if _targets_integration_dir(candidate, config):
             raise pytest.UsageError(
-                f"Refusing to run {arg!r}: tests/integration provisions real, "
-                f"billable cloud resources (AWS EKS/EC2). Set {_OPT_IN_VAR}=1 to "
-                f"run them deliberately, e.g.\n"
-                f"    {_OPT_IN_VAR}=1 pytest {arg}"
+                f"Refusing to run {candidate!r}: tests/integration provisions "
+                f"real, billable cloud resources (AWS EKS/EC2). Set "
+                f"{_OPT_IN_VAR}=1 to run them deliberately, e.g.\n"
+                f"    {_OPT_IN_VAR}=1 pytest {candidate}"
             )
 
 

tests/unit/test_billable_safety.py

@@ -160,37 +160,31 @@ def test_bare_pytest_is_not_refused_by_the_guard(tmp_path):
     """A bare `pytest` must still run. See #130.
 
     This is the counterpart to the refusal tests, and it guards a trap that
-    already sprang once. The guard originally inspected `config.args`, which
-    reads like "what the user asked for" but is not: when no path is given on
-    the command line, pytest populates `config.args` from `testpaths` in
-    pyproject.toml. `testpaths` used to list `tests/integration`, so the moment
-    the project's config actually took effect, a bare `pytest` matched the
-    guard and aborted the entire suite with the billable-resources refusal.
-
-    Two independent changes fixed it: the guard now reads
-    `config.invocation_params.args` (the real argv), and `testpaths` no longer
-    names `tests/integration`. Either one alone is sufficient, which is why
-    both are kept -- belt and braces.
-
-    Measured on this branch, undoing one is survivable and undoing both is not:
-
-        guard source            testpaths                       bare pytest
-        invocation_params.args  ["tests"]                       ok
-        invocation_params.args  ["tests/unit","tests/integration"] ok
-        config.args             ["tests"]                       ok
-        config.args             ["tests/unit","tests/integration"] REFUSED
-
-    So this test is a guard on the *combination*, not on either edit alone.
-    That is the honest scope: it is the last line of defence rather than the
-    first, and it fails loudly the moment the suite becomes unrunnable.
+    already sprang once. The guard reads `config.args` -- the *effective*
+    target list -- which is what makes it resistant to indirect targeting
+    (see `test_indirect_targeting_of_integration_is_refused`). The cost of
+    reading it is that when no path is given, pytest fills `config.args` from
+    `testpaths`. While `testpaths` named `tests/integration`, that made a bare
+    `pytest` match the guard and abort the whole suite.
+
+    The fix is therefore in the config, not the guard: `testpaths` is `["tests"]`.
+    That keeps `config.args` readable (so PYTEST_ADDOPTS and `-o testpaths=`
+    cannot sneak past) while leaving a default run collectible.
+
+    Note this is the opposite resolution from the one first attempted here.
+    Narrowing the guard to `invocation_params.args` also unblocked bare
+    `pytest`, but it opened three real bypasses, because what the operator
+    types is not what pytest collects. Reverting `testpaths` to name
+    `tests/integration` will make this test fail -- correctly, since every
+    default run would then be aimed at billable tests.
     """
     result = _collect_integration(opt_in=False, tmp_home=tmp_path, target=NO_TARGET)
     combined = (result.stdout or "") + (result.stderr or "")
 
     assert "Refusing to run" not in combined, (
-        "A bare `pytest` was refused by the billable-resources guard. The guard "
-        "is matching testpaths-derived arguments instead of what the operator "
-        "typed (see #130).\n" + combined[-2000:]
+        "A bare `pytest` was refused by the billable-resources guard, so the "
+        "default suite cannot run at all. `testpaths` is probably naming "
+        "tests/integration again (see #130).\n" + combined[-2000:]
     )
     assert result.returncode == 0, (
         f"A bare `pytest --collect-only` failed (exit {result.returncode}).\n"
@@ -201,6 +195,123 @@ def test_bare_pytest_is_not_refused_by_the_guard(tmp_path):
     )
 
 
+def _run_pytest(tmp_home, argv_extra, env_extra=None, cwd=None):
+    """Run pytest in a scrubbed subprocess with arbitrary argv and env.
+
+    Same isolation contract as `_collect_integration` (throwaway HOME, blocked
+    sockets, credentials stripped) but without assuming the shape of the
+    command, so indirect targeting routes can be exercised.
+    """
+    env = dict(os.environ)
+    env.pop(OPT_IN_VAR, None)
+    env["HOME"] = str(tmp_home)
+    env["USERPROFILE"] = str(tmp_home)
+    sitecustomize = tmp_home / "sitecustomize.py"
+    sitecustomize.write_text(
+        "import socket\n"
+        "def _deny(*a, **k):\n"
+        "    raise OSError('network disabled by test_billable_safety')\n"
+        "socket.socket.connect = _deny\n"
+        "socket.socket.connect_ex = _deny\n"
+        "socket.create_connection = _deny\n",
+        encoding="utf-8",
+    )
+    env["PYTHONPATH"] = os.pathsep.join(
+        [str(tmp_home), env.get("PYTHONPATH", "")]
+    ).rstrip(os.pathsep)
+    for leaked in (
+        "AWS_ACCESS_KEY_ID",
+        "AWS_SECRET_ACCESS_KEY",
+        "AWS_SESSION_TOKEN",
+        "AWS_PROFILE",
+        "AZURE_CLIENT_SECRET",
+        "GOOGLE_APPLICATION_CREDENTIALS",
+        "LAMBDA_CLOUD_API_KEY",
+    ):
+        env.pop(leaked, None)
+    env.update(env_extra or {})
+    return subprocess.run(
+        [sys.executable, "-m", "pytest", "--collect-only", "-q", "-o", "addopts="]
+        + list(argv_extra),
+        cwd=str(cwd or REPO_ROOT),
+        env=env,
+        capture_output=True,
+        encoding="utf-8",
+        errors="replace",
+        timeout=300,
+    )
+
+
+# Each entry is an *indirect* way to aim pytest at tests/integration -- one
+# that does not put the path anywhere in the typed argv, or that spells it
+# relative to something other than the repo root. Every one of these was a
+# working bypass at some point during #130 and is now refused. See the module
+# docstring of tests/conftest.py for why config.args is the right thing to read.
+_TARGET = "tests/integration/test_timeout_mechanism.py"
+INDIRECT_TARGETING = [
+    pytest.param(
+        {"argv_extra": ["-o", f"testpaths={_TARGET}"]},
+        id="testpaths-overridden-on-command-line",
+    ),
+    pytest.param(
+        {"env_extra": {"PYTEST_ADDOPTS": _TARGET}},
+        id="path-injected-through-PYTEST_ADDOPTS",
+    ),
+    pytest.param(
+        {"argv_extra": ["--pyargs", "tests.integration.test_timeout_mechanism"]},
+        id="dotted-module-name-via-pyargs",
+    ),
+    pytest.param(
+        {
+            "argv_extra": ["integration/test_timeout_mechanism.py"],
+            "from_tests_dir": True,
+        },
+        id="path-relative-to-a-subdirectory-cwd",
+    ),
+]
+
+
+@pytest.mark.parametrize("case", INDIRECT_TARGETING)
+def test_indirect_targeting_of_integration_is_refused(case, tmp_path):
+    """Aiming pytest at tests/integration without typing the path is refused.
+
+    The guard originally read `config.invocation_params.args` -- literally what
+    the operator typed. That is not the same as what pytest will collect:
+    `testpaths`, `-o testpaths=` and `PYTEST_ADDOPTS` all feed `config.args`
+    without appearing in the typed argv, and a path spelled relative to a
+    subdirectory did not resolve against the repo root. Each of these collected
+    billable modules while the guard believed nothing had been targeted.
+
+    Money is the reason this is parametrized rather than merged into one test:
+    a single assertion that stops at the first failure would hide the others.
+    """
+    case = dict(case)
+    from_tests_dir = case.pop("from_tests_dir", False)
+    result = _run_pytest(
+        tmp_path,
+        argv_extra=case.get("argv_extra", []),
+        env_extra=case.get("env_extra"),
+        cwd=(REPO_ROOT / "tests") if from_tests_dir else None,
+    )
+    combined = (result.stdout or "") + (result.stderr or "")
+
+    collected = [
+        line
+        for line in combined.splitlines()
+        if "integration/test_timeout_mechanism" in line.replace("\\", "/")
+        and "::" in line
+    ]
+    assert not collected, (
+        "Billable integration tests were collected through an indirect route "
+        "the guard did not see:\n" + "\n".join(collected[:10])
+    )
+    assert "Refusing to run" in combined, (
+        f"The guard did not refuse this run (exit {result.returncode}). It is "
+        f"reading something narrower than the effective target list.\n"
+        + combined[-2000:]
+    )
+
+
 def test_explicitly_targeting_integration_is_refused(tmp_path):
     """Naming the directory or a file in it must fail loudly, not silently.