Skip to content

Commit 5fda9be

Browse files
jeremymanningclaude
andcommitted
Issue #109: Stop the test suite from provisioning billable AWS resources
tests/integration/ carried no pytest markers, so the documented command `pytest tests/ -m "not real_world"` selected it -- including test_aws_eks_real_provision.py, whose docstring reads "this WILL create resources and incur costs!". A run was confirmed via lsof to hold ESTABLISHED connections to ec2-*.compute-1.amazonaws.com:443. Markers alone cannot fix this. pytest imports a module in order to collect it, and two files in that directory are standalone scripts rather than test modules: test_eks_permissions.py and test_aws_eks_debug.py fetch credentials, call boto3, and invoke exit() at module scope. Importing them is itself the harm -- and the module-level sys.exit(1) crashes collection outright with INTERNALERROR / "mainloop: caught unexpected SystemExit!". So the gate stops *collection*, via collect_ignore_glob in a new tests/integration/conftest.py. The gate is directory-wide (default-deny) rather than a per-file allowlist: misclassifying 1 file out of 48 costs real money, and a newly added file must not run for free just because nobody remembered to mark it. Opt in with CLUSTRIX_ALLOW_BILLABLE=1. Verification (full suite, network blocked via a socket-blocking sitecustomize that logs every attempt with a stack trace): before: pytest collection crashed with SystemExit inside AWS provisioning after: 2067 tests collected, 0 connection attempts to AWS/EC2/EKS, 0 boto3 frames in any captured stack Two non-billable network sources remain and are tracked elsewhere, not fixed here: 3072 pip-install-to-PyPI attempts from ClusterConfig.__init__ auto-installing packages (#123), and 6 live Azure Retail Prices API calls from tests/test_cost_monitoring.py:259,267 (#117). Tests: tests/unit/test_billable_safety.py, added first and watched to fail (50 failed, 1 passed) before the guard existed. The one passing test asserts the directory is still reachable under opt-in, so the guard cannot regress into silently deleting the tests. Now 52 passed. Placed in tests/unit/ deliberately -- that is the only directory CI currently executes, so this guard actually runs. Also ignores .ccpm_backup/, which carried a live PyPI token (see #111). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
1 parent 6df494a commit 5fda9be

4 files changed

Lines changed: 352 additions & 0 deletions

File tree

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,6 @@ docs/build/
6666

6767
# ClaudPoint checkpoint system
6868
.checkpoints/
69+
70+
# CCPM settings backups - contain local credentials, must never be committed (see #111)
71+
.ccpm_backup/
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
# Clustrix full audit + master plan — 2026-08-17
2+
3+
Session goal: explore the package thoroughly, triage all open GitHub issues, open a master plan
4+
for production readiness plus sub-issues. Five parallel read-only audits were run (source tree,
5+
test suite actually executed, all open issues, docs, security).
6+
7+
## Outcome
8+
9+
- **Master plan: #108** with **19 sub-issues (#109#127)**, all linked via GitHub sub-issue API.
10+
- **9 issues closed**, **11 issues updated** with evidence-cited corrections.
11+
- Open issues went 29 → 40 (net +11: 9 closed, 20 opened).
12+
- Nothing in the working tree was modified. No code changes made.
13+
14+
## Headline audit findings (all evidence-cited in #108)
15+
16+
| Area | Finding |
17+
|-|-|
18+
| Test run | `127 failed, 1738 passed, 26 skipped, 8 errors in 134.78s` |
19+
| CI | Runs **15 of 2,280 tests** (~0.7%). `tests/unit/` only. |
20+
| Coverage | 4 conflicting numbers: 74% (planning, unreproducible) / 5.69% (stale artifact) / 8.21% (CI scope) / 56.14% (full suite) |
21+
| Mocks | 60/240 test files, **2,513 occurrences**; 3 inside `tests/real_world/` |
22+
| Prod mock-awareness | `executor_scheduler_status.py:89` branches on `isinstance(ssh_client, Mock)`; `notebook_magic_mocks.py` imported by 6 prod modules |
23+
| Dead code | ~5,100 lines (13%) with zero importers |
24+
| Backends | ssh COMPLETE; slurm/sge/k8s/local PARTIAL; **pbs BROKEN**; **all cloud BROKEN/STUB** |
25+
| Security | #107 was a FALSE POSITIVE. Real issue: unauthenticated pickle of remote results = remote→local RCE |
26+
| Git | master was **30 commits ahead of origin** (unpushed epic #98 work) |
27+
28+
### Landmine (highest urgency)
29+
`tests/integration/` has **zero pytest markers**, so `pytest -m "not real_world"` — the documented
30+
unit-test command — selects `test_aws_eks_real_provision.py` ("this WILL create resources and incur
31+
costs!"). Confirmed via `lsof`: ESTABLISHED connections to `ec2-*.compute-1.amazonaws.com:443`.
32+
**#109**
33+
34+
### The structural pattern
35+
Nearly every "done" item is half-done the same way: **scaffolding landed, seam did not.**
36+
- closure vars detected + injected (`function_flattening.py:623,654-695`) but never passed (`:749-751`)
37+
- REPL limit documented (`README.md:554`) but code still fails opaquely (`utils.py:118-119`)
38+
- k8s provisioners exist but GCP `_assign_iam_role()` assigns nothing (`gcp_provisioner.py:356`)
39+
- cloud path: `utils.py:151` writes `{"function":...}`, `executor_cloud.py:390` reads `['func']`
40+
→ unconditional KeyError. Proof the path never ran.
41+
42+
## HuggingFace Jobs — VERIFIED WORKING (new capability)
43+
44+
Verified end-to-end today: real container ran, returned `CLUSTRIX_HF_OK 3.12.14 x86_64`.
45+
46+
- Namespace: **`contextlab`** (org, plan `academia`). Personal namespace gives `402 Payment Required`.
47+
- Token must be **fine-grained with `job.write` scoped to the ORG**, not just the user.
48+
User-only scope → `403 missing permissions: job.write` on the org namespace.
49+
- Diagnostic: `403` = token can't act here; `402` = token fine, nobody's paying.
50+
- Flavors: `cpu-basic … h100x8`.
51+
- `hf jobs run --namespace contextlab --flavor cpu-basic python:3.12-slim python -c "..."`
52+
53+
Strategic point: clustrix targets HF **Spaces** (wrong primitive, long-lived web apps) and it's a
54+
stub. HF **Jobs** matches clustrix's model exactly. → **#118**
55+
56+
## Issue triage performed
57+
58+
| Action | Issues |
59+
|-|-|
60+
| Closed — false positive | #107 |
61+
| Closed — completed | #85 (SGE fully implemented), #82, #72 |
62+
| Closed — superseded/dup | #61 (dup of #98), #86 (dup of #99+#102) |
63+
| Closed — archival | #92, #93, #94 |
64+
| Updated | #66, #68, #88, #89, #90, #91, #95, #98, #99, #100, #101, #102, #103, #104, #105, #106 |
65+
66+
## Recommended order of attack (from #108 §9)
67+
68+
1. **#109 alone first** — only issue with ongoing cost if ignored.
69+
2. #110#114#113 (runnable → green → CI on). Turning CI on before #114 makes master
70+
permanently red and invites re-adding `continue-on-error`.
71+
3. #116 before #117 (remove prod mock-awareness before rewriting tests).
72+
4. #122 before #115 (delete dead code before baselining coverage).
73+
5. #118 early — unblocks honest testing for #119, #120, #126.
74+
75+
## Open decisions for the user
76+
77+
- **#112**: what to do with the 30 unpushed commits. Recommended (b): push to
78+
`epic/test-coverage-90-percent` branch + PR, rather than merging ~2,500 mock occurrences into
79+
master right before #117 removes them.
80+
- **#111**: rotate HF tokens `hf_Fbf…` / `hf_hSV…` (local-only, never on GitHub — verified 3 ways),
81+
and enable GitHub secret scanning + push protection (currently disabled on this public repo).
82+
83+
## Verification commands used
84+
85+
```bash
86+
git rev-list --left-right --count origin/master...master # 0 30
87+
grep -rc "pytest.mark" tests/integration/ # 0 for every file
88+
gh api repos/ContextLab/clustrix/secret-scanning/alerts # 404 = disabled
89+
gh api repos/ContextLab/clustrix/issues/108/sub_issues --jq 'length' # 19
90+
```

tests/integration/conftest.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
"""Opt-in gate for the integration test suite.
2+
3+
See issue #109.
4+
5+
Everything in this directory talks to external infrastructure, and a large
6+
subset provisions **billable** cloud resources (AWS EKS clusters, EC2
7+
instances, GPU nodes). Before this gate existed, the documented command
8+
``pytest tests/ -m "not real_world"`` collected this directory, because none of
9+
its files carried a pytest marker.
10+
11+
A marker-based skip would not be sufficient. pytest must *import* a module in
12+
order to collect it, and several modules here are standalone scripts rather
13+
than test modules -- ``test_eks_permissions.py`` and ``test_aws_eks_debug.py``
14+
fetch credentials, call boto3, and invoke ``exit()`` at module scope. Importing
15+
them is itself the harm:
16+
17+
* the AWS calls happen before any marker or skip is consulted, and
18+
* the module-level ``sys.exit(1)`` raises ``SystemExit`` during collection,
19+
which crashes the whole pytest run with ``INTERNALERROR``.
20+
21+
So the gate has to stop *collection*, which is what ``collect_ignore_glob``
22+
does -- pytest never imports an ignored file.
23+
24+
The gate is deliberately directory-wide (default-deny) rather than a
25+
per-file allowlist. Misclassifying one file out of ~48 costs real money, and a
26+
newly added file must not be able to run for free simply because nobody
27+
remembered to mark it.
28+
29+
To run these tests deliberately::
30+
31+
CLUSTRIX_ALLOW_BILLABLE=1 pytest tests/integration/
32+
33+
Be aware that doing so may create real, chargeable cloud resources.
34+
"""
35+
36+
import os
37+
38+
import pytest
39+
40+
OPT_IN_VAR = "CLUSTRIX_ALLOW_BILLABLE"
41+
_TRUTHY = {"1", "true", "yes", "on"}
42+
43+
44+
def billable_tests_enabled() -> bool:
45+
"""Return True only if the operator explicitly opted in to paying."""
46+
return os.environ.get(OPT_IN_VAR, "").strip().lower() in _TRUTHY
47+
48+
49+
# Prevent pytest from importing anything in this directory unless opted in.
50+
# This must stay at module scope: conftest collect_ignore hooks are read when
51+
# the directory is first visited, before any test module is imported.
52+
if not billable_tests_enabled():
53+
collect_ignore_glob = ["*.py"]
54+
55+
56+
def pytest_collection_modifyitems(config, items):
57+
"""Mark everything here as integration + expensive when it does run.
58+
59+
Only reachable under opt-in (otherwise nothing is collected). This keeps
60+
``-m`` selection meaningful for anyone who has opted in, e.g.
61+
``-m "not expensive"`` to run the cheaper integration tests.
62+
"""
63+
for item in items:
64+
item.add_marker(pytest.mark.integration)
65+
item.add_marker(pytest.mark.expensive)

tests/unit/test_billable_safety.py

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
#!/usr/bin/env python3
2+
"""Guards that the default test suite cannot spend money.
3+
4+
Regression tests for issue #109.
5+
6+
Background: ``tests/integration/`` originally carried no pytest markers, so the
7+
documented command ``pytest tests/ -m "not real_world"`` selected
8+
``test_aws_eks_real_provision.py`` ("this WILL create resources and incur
9+
costs!") and opened live connections to AWS.
10+
11+
Two files in that directory (``test_eks_permissions.py`` and
12+
``test_aws_eks_debug.py``) are standalone scripts with no ``__main__`` guard:
13+
they fetch credentials, call boto3, and invoke ``exit()`` at *module scope*.
14+
That means a marker-based skip is not sufficient -- pytest imports a module in
15+
order to collect it, so the AWS calls happen before any marker is consulted.
16+
The guard must therefore prevent *collection*, not merely execution.
17+
18+
These tests run pytest in a subprocess so they exercise the real collection
19+
machinery rather than asserting against a stubbed pytest.
20+
"""
21+
22+
import os
23+
import pathlib
24+
import subprocess
25+
import sys
26+
27+
import pytest
28+
29+
REPO_ROOT = pathlib.Path(__file__).resolve().parents[2]
30+
INTEGRATION_DIR = REPO_ROOT / "tests" / "integration"
31+
OPT_IN_VAR = "CLUSTRIX_ALLOW_BILLABLE"
32+
33+
# Phrases pytest uses when a run selected nothing. Kept in one place so the
34+
# "gate" and "not a deletion" tests cannot drift apart.
35+
_EMPTY_COLLECTION_PHRASES = (
36+
"no tests collected",
37+
"no tests ran",
38+
"collected 0 items",
39+
)
40+
41+
42+
def _collected_nothing(output: str) -> bool:
43+
return any(phrase in output for phrase in _EMPTY_COLLECTION_PHRASES)
44+
45+
46+
def _collect_integration(opt_in: bool, tmp_home):
47+
"""Run `pytest --collect-only` against tests/integration in a subprocess.
48+
49+
`-o addopts=` strips the project's default addopts so this does not depend
50+
on xdist being installed.
51+
52+
The subprocess gets a throwaway HOME and a scrubbed environment. This is
53+
deliberate: a test whose job is to prove the suite cannot spend money must
54+
not itself be able to spend money if the guard it is testing is absent or
55+
broken. `FlexibleCredentialManager` reads `~/.clustrix/.env`
56+
(credential_manager.py:304-305), so redirecting HOME removes the only
57+
on-disk credential source these modules would otherwise find.
58+
"""
59+
env = dict(os.environ)
60+
env.pop(OPT_IN_VAR, None)
61+
if opt_in:
62+
env[OPT_IN_VAR] = "1"
63+
env["HOME"] = str(tmp_home)
64+
65+
# Hard network block for the subprocess.
66+
#
67+
# Scrubbing HOME is not sufficient on its own: FlexibleCredentialManager
68+
# also resolves credentials through the 1Password CLI, so a developer with
69+
# an unlocked `op` session still gets live AWS keys. If the guard under
70+
# test is broken, this subprocess would then make real API calls -- the
71+
# test would cause the very harm it exists to detect. Blocking socket
72+
# connections makes the failure mode "loud error" instead of "AWS bill".
73+
sitecustomize = tmp_home / "sitecustomize.py"
74+
sitecustomize.write_text(
75+
"import socket\n"
76+
"class _Blocked(OSError):\n"
77+
" pass\n"
78+
"def _deny(*a, **k):\n"
79+
" raise _Blocked('network disabled by test_billable_safety')\n"
80+
"socket.socket.connect = _deny\n"
81+
"socket.socket.connect_ex = _deny\n"
82+
"socket.create_connection = _deny\n"
83+
)
84+
env["PYTHONPATH"] = os.pathsep.join(
85+
[str(tmp_home), env.get("PYTHONPATH", "")]
86+
).rstrip(os.pathsep)
87+
for leaked in (
88+
"AWS_ACCESS_KEY_ID",
89+
"AWS_SECRET_ACCESS_KEY",
90+
"AWS_SESSION_TOKEN",
91+
"AWS_PROFILE",
92+
"AZURE_CLIENT_SECRET",
93+
"GOOGLE_APPLICATION_CREDENTIALS",
94+
"LAMBDA_CLOUD_API_KEY",
95+
):
96+
env.pop(leaked, None)
97+
return subprocess.run(
98+
[
99+
sys.executable,
100+
"-m",
101+
"pytest",
102+
str(INTEGRATION_DIR),
103+
"--collect-only",
104+
"-q",
105+
"-o",
106+
"addopts=",
107+
"-p",
108+
"no:cacheprovider",
109+
],
110+
cwd=str(REPO_ROOT),
111+
env=env,
112+
capture_output=True,
113+
text=True,
114+
timeout=300,
115+
)
116+
117+
118+
def test_integration_tests_are_not_collected_by_default(tmp_path):
119+
"""Without explicit opt-in, tests/integration must collect zero tests.
120+
121+
This is the core guarantee: the default suite is free to run.
122+
"""
123+
result = _collect_integration(opt_in=False, tmp_home=tmp_path)
124+
combined = result.stdout + result.stderr
125+
126+
assert _collected_nothing(combined), (
127+
"tests/integration was collected without opt-in.\n"
128+
f"exit={result.returncode}\n{combined[-3000:]}"
129+
)
130+
131+
132+
def test_default_collection_does_not_import_billable_modules(tmp_path):
133+
"""Collection must not *import* the unguarded AWS scripts.
134+
135+
`test_eks_permissions.py` and `test_aws_eks_debug.py` make real boto3 calls
136+
and call exit() at module scope, so importing them is itself the harm.
137+
"""
138+
result = _collect_integration(opt_in=False, tmp_home=tmp_path)
139+
combined = result.stdout + result.stderr
140+
141+
for marker in (
142+
"Testing EKS permissions", # printed at import by test_eks_permissions
143+
"Getting AWS credentials", # printed at import by test_aws_eks_debug
144+
"botocore",
145+
"NoCredentialsError",
146+
):
147+
assert marker not in combined, (
148+
f"Billable module was imported during default collection "
149+
f"(saw {marker!r}).\n{combined[-3000:]}"
150+
)
151+
152+
153+
def test_integration_tests_are_still_reachable_with_opt_in(tmp_path):
154+
"""The guard is a gate, not a deletion.
155+
156+
With CLUSTRIX_ALLOW_BILLABLE=1 the directory must become collectable again,
157+
otherwise we have silently dropped the tests instead of protecting them.
158+
"""
159+
result = _collect_integration(opt_in=True, tmp_home=tmp_path)
160+
combined = result.stdout + result.stderr
161+
162+
assert not _collected_nothing(combined), (
163+
"tests/integration collected nothing even with opt-in; the guard is "
164+
f"deleting tests rather than gating them.\n{combined[-3000:]}"
165+
)
166+
167+
168+
@pytest.mark.parametrize(
169+
"path",
170+
sorted(INTEGRATION_DIR.glob("*.py")),
171+
ids=lambda p: p.name,
172+
)
173+
def test_every_integration_file_is_declared_billable(path):
174+
"""Every file under tests/integration must be covered by the opt-in gate.
175+
176+
A new file dropped into this directory must not be able to run for free.
177+
The gate is directory-wide, so this asserts the directory conftest exists
178+
and names the file's suffix -- i.e. that nothing escapes by being added
179+
later.
180+
"""
181+
conftest = INTEGRATION_DIR / "conftest.py"
182+
assert conftest.exists(), (
183+
"tests/integration/conftest.py is missing; without it the directory "
184+
"collects for free."
185+
)
186+
source = conftest.read_text()
187+
assert (
188+
OPT_IN_VAR in source
189+
), f"tests/integration/conftest.py does not reference {OPT_IN_VAR}"
190+
assert "collect_ignore" in source, (
191+
"tests/integration/conftest.py must prevent collection (collect_ignore), "
192+
"not merely skip at runtime -- several modules make live AWS calls at "
193+
"import time."
194+
)

0 commit comments

Comments
 (0)