Skip to content

Commit ed42375

Browse files
jeremymanningclaude
jeremymanning
and
claude
authored
Update notes for handoff to a fresh session (#136)
Adds notes/HANDOFF.md as the single entry point: current state, the environment traps that cost time (no usable venv, pre_push_check's PATH requirement, the flake8 gate that can never pass), the billable-test guard invariant and why it must not be "simplified", prioritised next steps, and the working agreements established in these sessions. Also reconciles the two existing notes, which had gone stale or self-contradictory now that the work is merged: * handoff_130_pytest_config.md gets a banner marking it EXECUTED. A fresh session finding it in notes/ would otherwise re-run a completed plan -- and two of its instructions were wrong. Its Step 1 recommendation (switch the #109 guard to config.invocation_params.args) is unsafe: it opens PYTEST_ADDOPTS and -o testpaths= bypasses to billable tests. * session_130_pytest_config_execution.md gains the red-team section, which the merged version predated, and three corrections to its own earlier text: the "224 tests in tests/real_world/" figure was a count of decorator lines (it is 388); the claim that a bare `pytest` was "harmless while no config applied" was wrong (it aborted on master and ran zero tests); and the table suggesting the two fixes were interchangeable is now marked superseded, since only one of them is safe. Verified on master @ 760432c: tests/unit/ 79 passed, test_billable_safety 60 passed, flake8 91, #130 closed, #133 and #135 open. Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 760432c commit ed42375

3 files changed

Lines changed: 303 additions & 6 deletions

File tree

‎notes/HANDOFF.md‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
# HANDOFF — start here
2+
3+
**Last updated:** 2026-08-17 · **`master` @ `760432c`** · Read this before anything else in `notes/`.
4+
5+
---
6+
7+
## 1. Where things stand
8+
9+
The package is mid-way through the production-readiness effort tracked by the
10+
master plan **#108**. The audit behind it is in
11+
`audit_and_master_plan_2026-08-17.md`.
12+
13+
Two things landed today:
14+
15+
| Issue | State | Result |
16+
|-|-|-|
17+
| **#109** billable tests cannot run by accident | closed earlier | guard + opt-in, now hardened further |
18+
| **#130** `pytest.ini` was dead config shadowing `pyproject.toml` | **closed** | PR #134 → `760432c` |
19+
20+
Three issues were **filed** today and are open: **#133**, **#135**, and the
21+
correction comments on **#110** and **#114**.
22+
23+
### What #130 actually changed
24+
25+
`pytest.ini` used `[tool:pytest]`, a header valid only in `setup.cfg`. pytest
26+
selected the file anyway and stopped searching, so `pyproject.toml` was never
27+
read either — two config files, **zero** effective configuration. `addopts`,
28+
`testpaths`, `filterwarnings`, every marker and `--strict-markers` were all
29+
declared and inert.
30+
31+
Now: `pyproject.toml` is the single source, `testpaths = ["tests"]`,
32+
`--strict-markers` is live, 7 markers registered, `filterwarnings` restored.
33+
`pytest.ini` is deleted and `tests/unit/test_pytest_config.py` fails if any
34+
config file ever shadows `pyproject.toml` again.
35+
36+
---
37+
38+
## 2. Read these in this order
39+
40+
1. **This file.**
41+
2. `session_130_pytest_config_execution.md` — what was done, what was measured,
42+
and a red-team section listing **two bugs that the fix itself introduced**.
43+
Its "Final verified state" block is the current truth.
44+
3. `handoff_130_pytest_config.md` — **already executed, do not run again.** It
45+
carries a banner saying so. Kept for its analysis; two of its instructions
46+
were wrong and were reversed.
47+
4. `audit_and_master_plan_2026-08-17.md` — the #108 background.
48+
49+
---
50+
51+
## 3. Environment — read before running anything
52+
53+
**The repo has no usable venv and `import clustrix` fails everywhere by
54+
default** (`paramiko` missing). This is #110 and it is still open. Build a
55+
throwaway one:
56+
57+
```bash
58+
python3 -m venv /tmp/v && /tmp/v/bin/pip install -e ".[dev]"
59+
/tmp/v/bin/pytest tests/unit/ -q # expect: 79 passed
60+
```
61+
62+
`[dev]` is now sufficient to collect the whole tree (`numpy`/`pandas` were
63+
added). Add `pytest-xdist pytest-timeout` only if you need them.
64+
65+
Gotchas that will cost you time:
66+
67+
- **`scripts/pre_push_check.py` shells out to bare `pytest`/`mypy`/`flake8`
68+
from `PATH`.** Run it as
69+
`PATH="/tmp/v/bin:$PATH" /tmp/v/bin/python scripts/pre_push_check.py`.
70+
Using the venv interpreter alone fails with `ModuleNotFoundError: paramiko`.
71+
- **That script cannot exit 0** — flake8 reports 91 findings on `master`. This
72+
is #133, not something you broke. CI runs the same flake8 command with
73+
`--exit-zero`, so it has never been green anywhere.
74+
- **`tomllib` needs Python ≥3.11.** Default interpreter here is 3.9; use
75+
`/opt/homebrew/bin/python3.12`.
76+
- **Pass `-o addopts=`** when comparing collection counts across changes.
77+
- **`tests/real_world/conftest.py:223`** calls `is_dartmouth_network()` at
78+
collection time — live DNS plus a `ping`. If collection hangs off-network,
79+
that is why. Tracked as #117.
80+
- **CI's `black` is pinned to 25.1.0.** Do not "upgrade" it (see #110).
81+
- GitHub's API returned 503s repeatedly today; `gh pr edit` also fails with a
82+
Projects-classic GraphQL deprecation error. Use
83+
`gh api -X PATCH repos/ContextLab/clustrix/pulls/<n> -F body=@file.md`.
84+
85+
---
86+
87+
## 4. ⚠️ The one thing not to break
88+
89+
`tests/integration/` provisions **real, billable** AWS EKS/EC2 resources. Two
90+
layers stop it:
91+
92+
1. `tests/conftest.py::pytest_configure` refuses any run whose targets resolve
93+
under `tests/integration`.
94+
2. `tests/integration/conftest.py` sets `collect_ignore_glob` so pytest never
95+
imports anything there.
96+
97+
Opt in deliberately with `CLUSTRIX_ALLOW_BILLABLE=1`.
98+
99+
**The guard reads `config.args`, not `config.invocation_params.args`. Do not
100+
"simplify" this.** `invocation_params.args` is only what the operator typed;
101+
`testpaths`, `-o testpaths=` and `PYTEST_ADDOPTS` all feed `config.args`
102+
without appearing in it. Narrowing the guard opened two working money-safety
103+
bypasses during this PR, caught only by red-teaming.
104+
105+
Reading `config.args` is safe **only because `testpaths = ["tests"]`**. If you
106+
ever put `tests/integration` back into `testpaths`, a bare `pytest` will be
107+
refused and the whole suite becomes unrunnable. The two settings are coupled;
108+
`tests/unit/test_billable_safety.py` (60 tests) enforces the coupling.
109+
110+
---
111+
112+
## 5. Suggested next steps, in priority order
113+
114+
1. **#121 — P0 security.** Unauthenticated pickle of remote results (RCE) plus
115+
disabled SSH host-key verification. The only P0 that is a live exploit
116+
rather than hygiene. Start here unless told otherwise.
117+
2. **#114 — re-baseline the failures.** Now genuinely actionable for the first
118+
time: collection used to abort on a SyntaxError, so its "127 failures /
119+
8 collection errors" was measured while 6 modules could not be imported.
120+
Collection errors are now **0**. A partial run showed 13 failures in the
121+
first 59 tests; the real number is unknown. Re-derive, then triage.
122+
3. **#135 + #133 together.** Both are "a gate that silently does nothing" —
123+
the same failure mode as #130. `fast_ci.yml` is invalid YAML and has never
124+
run a single job (on `master` too); `pre_push_check.py` can never pass.
125+
Sequence #135 against #120, since its integration job runs real `@cluster`
126+
execution and may fail legitimately.
127+
4. **#110 — finish it.** Two of its acceptance criteria are met (f-string
128+
SyntaxError fixed; `numpy`/`pandas` declared). Still open: `pytest-xdist`
129+
placement, the `requires-python` floor, a documented bootstrap, and a CI job
130+
that installs only `[dev]` and asserts collection succeeds.
131+
132+
---
133+
134+
## 6. Working agreements established in these sessions
135+
136+
- **Do not hand PRs back for review.** The instruction on record is: *"this is
137+
too early for a serious review; you should review both PRs and merge or edit
138+
as you see fit. use subagents to red-team and do this carefully. then suggest
139+
next steps."* Red-team, fix, merge, then propose what is next.
140+
- **Red-teaming is not optional and it works.** Four agents found two bugs the
141+
fix itself had introduced, including a money-safety regression. Budget for it.
142+
- **Do not take subagent findings on trust.** Of four reports, one claim was
143+
wrong (it inspected the wrong venv) and one misattributed a cause. Verify
144+
before acting.
145+
- **Fix what you encounter, but do not smuggle it into an unrelated PR.** The
146+
f-string SyntaxError and the `numpy`/`pandas` gap were fixed because they
147+
blocked the #130 gate. The dead `fast_ci.yml` workflow and the 91 flake8
148+
findings were filed instead, because repairing them has unpredictable blast
149+
radius.
150+
151+
---
152+
153+
## 7. Loose ends in the working tree
154+
155+
- `.omc/` — tooling state, untracked, ignore it.
156+
- `coverage_detailed_report.txt` — stale, untracked, **nothing in the repo
157+
generates it**. Safe to delete; left in place rather than presume.
158+
- `/tmp/v130` and `/tmp/fresh130` — throwaway venvs from today. `fresh130` is
159+
`[dev]`-only and useful for checking what a clean contributor install does.
160+
Both are disposable.

‎notes/handoff_130_pytest_config.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,24 @@
11
# Handoff: fix #130 — pytest.ini is dead config shadowing pyproject.toml
22

3+
> ## ✅ EXECUTED AND MERGED — 2026-08-17. Do not run this plan again.
4+
>
5+
> Landed as PR #134, squashed to `760432c` on `master`. Issue #130 is closed.
6+
> Outcome, corrections, and what this plan got wrong: `session_130_pytest_config_execution.md`.
7+
>
8+
> **This document is kept for its analysis, not as a to-do list.** Two of its
9+
> instructions were wrong and were reversed during execution:
10+
>
11+
> 1. **Step 1's core recommendation is unsafe.** It says to switch the #109
12+
> guard to `config.invocation_params.args`. Doing so opens real
13+
> money-safety bypasses (`PYTEST_ADDOPTS=`, `-o testpaths=`), because what
14+
> the operator types is not what pytest collects. The guard reads
15+
> `config.args` on `master` today; that is safe only because Step 2's
16+
> `testpaths = ["tests"]` also landed. Do not "restore" Step 1 as written.
17+
> 2. **Step 0's baseline was not clean.** It had 6 collection errors that had
18+
> to be fixed before Step 4's gate could mean anything.
19+
>
20+
> Its §8 "Things that will trip you up" is still accurate and still useful.
21+
322
**Written:** 2026-08-17 · **For:** a fresh session picking this up cold
423
**Issue:** ContextLab/clustrix#130 · **Parent:** #108 · **Related:** #110, #113, #115, #117
524

‎notes/session_130_pytest_config_execution.md‎

Lines changed: 124 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
# Session log: executing the #130 plan
22

3-
**Date:** 2026-08-17 · **Branch:** `fix/130-pytest-config` · **Base:** `master` @ `a9393b7`
4-
**Plan followed:** `notes/handoff_130_pytest_config.md`
3+
**Date:** 2026-08-17 · **Merged:** PR #134 → `760432c` on `master` · **Issue #130: closed**
4+
**Base:** `master` @ `a9393b7` · **Plan followed:** `notes/handoff_130_pytest_config.md`
5+
6+
> Read `HANDOFF.md` first if you are a fresh session. This file is the detailed
7+
> record; `HANDOFF.md` is the entry point.
58
69
Every command below was run in a throwaway venv, since the repo has no usable
710
one by default (#110):
@@ -60,20 +63,34 @@ So the two fixes are each independently sufficient, and the test guards the
6063
*combination*. Both are kept as belt and braces, and the table is recorded in
6164
the test's docstring.
6265

66+
> **Superseded — see the red-team section below.** That table is about one
67+
> property only (does bare `pytest` run?). It is true as far as it goes and
68+
> badly misleading as a conclusion: `invocation_params.args` also opens real
69+
> money-safety bypasses, so the two options were never equivalent. The guard
70+
> reads `config.args` on `master` today.
71+
6372
**`--strict-markers` needs care with `-o addopts=`.** It reaches pytest through
6473
`addopts`, so `test_strict_markers_is_active` skips when
6574
`config.option.override_ini` contains an `addopts=` entry. Verified against
6675
pytest 8.4.2: that attribute holds `['addopts=']`.
6776

68-
**`pre_push_check.py` ran a bare `pytest`.** Harmless only while no config was
69-
effective. Once `testpaths` went live it resolved to all 1670 tests including
70-
the 224 network-bound ones in `tests/real_world/`, and stopped terminating. It
77+
**`pre_push_check.py` ran a bare `pytest`.** (Two figures in this paragraph
78+
were wrong when first written and are corrected here: it was never "harmless",
79+
and `tests/real_world/` holds 388 tests, not 224 — that was a count of
80+
decorator lines.) On `master` the bare `pytest` aborted in seconds on the
81+
f-string SyntaxError and ran **zero** tests, so the gate was permanently red
82+
and checked nothing. Fixing that error let collection succeed, at which point
83+
the bare `pytest` resolved to all 1670 tests including the 388 network-bound
84+
ones in `tests/real_world/`, and stopped terminating. It
7185
also left artifacts behind: it modified checked-in files under
7286
`tests/real_world/screenshots/` and created an untracked
7387
`performance_test_results/`. Now runs `pytest tests/unit/ -m "not real_world"`,
7488
matching `.github/workflows/tests.yml`; the generated paths are gitignored.
7589

76-
## Definition of done — measured
90+
## Definition of done — measured *at the time the PR was opened*
91+
92+
Superseded by the "Final verified state" block at the end of this file; the
93+
counts moved once the red-team fixes added tests. Kept for the audit trail.
7794

7895
```
7996
configfile pyproject.toml
@@ -110,3 +127,104 @@ https://github.com/ContextLab/clustrix/issues/110#issuecomment-5317453905
110127

111128
**#117** untouched, as instructed — `tests/real_world/conftest.py:223` still
112129
calls `is_dartmouth_network()` at collection time.
130+
131+
---
132+
133+
# Red-team pass (after the PR was opened)
134+
135+
Four parallel subagents were pointed at the branch: guard bypasses, config
136+
regressions, the new tests, and PR-claim honesty. **Two of the bugs they found
137+
were mine, introduced by this PR.** Everything below was fixed before merge.
138+
139+
## 1. Step 1 of the plan was actively unsafe (the important one)
140+
141+
Switching the #109 guard to `config.invocation_params.args` unblocked bare
142+
`pytest`, but `invocation_params.args` is only *what the operator typed*.
143+
`testpaths`, `-o testpaths=` and `PYTEST_ADDOPTS` all feed `config.args`
144+
without ever appearing there. Verified against `master`, which **refused** both:
145+
146+
```bash
147+
PYTEST_ADDOPTS=tests/integration/test_timeout_mechanism.py pytest --co # branch: 2 collected
148+
pytest --co -o testpaths=tests/integration/test_timeout_mechanism.py # branch: 2 collected
149+
```
150+
151+
**Resolution: the fix belonged in the config, not the guard.** The guard reads
152+
`config.args` again — safe *because* `testpaths = ["tests"]` landed in Step 2.
153+
154+
Also fixed while in there (both pre-existing #109 holes):
155+
156+
- relative paths were resolved only against `rootdir`, so from inside `tests/`
157+
the path `integration/test_x.py` was unrecognised. All plausible bases are
158+
tried now.
159+
- `--pyargs` / `-p` address modules by dotted name and never looked like paths;
160+
both are translated and checked.
161+
162+
Final state, measured:
163+
164+
| invocation | result |
165+
|-|-|
166+
| explicit dir / file / node id | refused |
167+
| `-o testpaths=` | refused |
168+
| `PYTEST_ADDOPTS=` | refused |
169+
| `--pyargs <dotted>` | refused |
170+
| cwd-relative from `tests/` | refused |
171+
| bare `pytest` | allowed, **0** integration nodes |
172+
| `CLUSTRIX_ALLOW_BILLABLE=1` | works |
173+
174+
`-p <dotted>` cannot be blocked *before* the import it triggers — no conftest
175+
hook runs that early — but the run is refused before any test executes, which
176+
is where the cost is. It also fails on its own here: `tests/` is not a package.
177+
178+
Covered by `test_indirect_targeting_of_integration_is_refused` (4 params).
179+
Three of the four fail against the old guard, so they are not decorative.
180+
181+
## 2. Claims of mine that were false
182+
183+
- **"the 224 tests in tests/real_world/"** — wrong, and I had committed it as a
184+
code comment. 224 counts `@pytest.mark.real_world` *decorator lines*; the
185+
directory collects **388** tests.
186+
- **The `pre_push_check.py` rationale** — I wrote that bare `pytest` was
187+
"harmless while no config applied." It was not: on `master` it aborted in
188+
seconds on the f-string SyntaxError and ran **zero** tests. Fixing that error
189+
is what made the runtime problem appear.
190+
- **`test_no_shadowing_config_file_exists` forbade `tox.ini`/`setup.cfg`** —
191+
pytest checks both *after* `pyproject.toml` (`_pytest/config/findpaths.py`),
192+
so neither can shadow it, and the test failed on an ordinary pytest-free
193+
`setup.cfg` holding flake8 config. Now limited to `pytest.ini`/`.pytest.ini`.
194+
- **`coverage_detailed_report.txt` in `.gitignore`** — nothing in the repo
195+
writes it. Entry removed.
196+
- **"18/18 CI checks passing"** — incomplete. `gh pr checks` omits the Fast CI
197+
workflow, which was running and failing.
198+
199+
One agent claim was **rejected**: it reported the fresh-venv result as
200+
unsubstantiated, having inspected `/tmp/v130` (which also has `[test]`).
201+
`/tmp/fresh130` is genuinely `[dev]`-only. Do not take agent findings on
202+
trust — two of the four needed verification before acting.
203+
204+
## 3. `fast_ci.yml` — my fix was reverted, diagnosis was wrong
205+
206+
I added `pytest-timeout` and claimed it fixed a job "failing on every run."
207+
It does not. The workflow is **invalid YAML and has never run a single job**
208+
— every run, on `master` too, is `conclusion: failure` with `jobs: 0`.
209+
210+
```
211+
yaml.scanner.ScannerError: while scanning a simple key
212+
in ".github/workflows/fast_ci.yml", line 89, column 1
213+
```
214+
215+
Three steps open `python -c "` inside a `run: |` block and write the Python
216+
body at column 0, ending the block scalar. Reverted to match `master` exactly
217+
and filed as **#135**; the missing `pytest-timeout` is real but latent.
218+
219+
## Final verified state on `master` @ `760432c`
220+
221+
```
222+
configfile pyproject.toml
223+
bare pytest 1674 collected, 0 integration nodes
224+
collection errors 0
225+
markers registered 7 / 7
226+
tests/unit/ 79 passed
227+
mypy clean
228+
flake8 91 (master before: 92) -- see #133
229+
CI 15 test jobs + docs + integration pass
230+
```

0 commit comments

Comments
 (0)