From 60e3bada467905c290e000d728d124b9dc22ecaa Mon Sep 17 00:00:00 2001 From: igerber Date: Tue, 7 Jul 2026 15:40:38 -0400 Subject: [PATCH] fix(tests): sandbox doc-snippet env mutations; order-robust dCDH baseline backend detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doc-snippet runner exec'd documentation code blocks without sandboxing os.environ, so the troubleshooting page's backend-override snippet (os.environ['DIFF_DIFF_BACKEND'] = 'python') leaked into every later test in a full-suite run (7,008 teardowns with perturbed state, traced with a teardown-hook leak detector). The only victim was the dCDH pinned bootstrap baseline test, whose call-time env read selected the pure-Python baseline arm while the fit still dispatched to the already-imported Rust backend (fails under full-suite order, passes standalone — the TODO row's exact signature). Root cause: snapshot/restore os.environ around each snippet exec. Defense in depth: the dCDH test now derives its baseline arm from the same dispatch globals the fit consumes (bootstrap_utils / linalg, bound at import), with a coherence assert between the two. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 10 ++++++++++ TODO.md | 1 - tests/test_doc_snippets.py | 17 ++++++++++++++++- tests/test_survey_dcdh.py | 27 ++++++++++++++++++++++----- 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60e5c940c..092c1cbdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`fixest` cluster-robust SE band**: the DiD/TWFE cluster-at-unit SE is pinned within the documented ~0.25% fixest-CR1 small-sample DOF-convention band (guards an unintended SE-formula change; the machine-precision hetero/cluster lock is deferred — needs an unbalanced-DGP golden). +- **Doc-snippet env leak fixed; dCDH pinned-baseline backend detection made order-robust.** The + doc-snippet runner (`tests/test_doc_snippets.py`) executed documentation code blocks without + sandboxing `os.environ`, so the troubleshooting page's backend-override snippet leaked + `DIFF_DIFF_BACKEND='python'` into every later test in a full-suite run. The only victim was + `test_survey_dcdh.py::test_bootstrap_se_matches_pre_pr4_baseline`, whose call-time env read then + selected the pure-Python baseline arm while the fit still dispatched to the already-imported Rust + backend (fails under full-suite order, passes standalone). The runner now snapshot/restores + `os.environ` around each snippet (root cause), and the dCDH test derives its baseline arm from + the same dispatch globals the fit consumes (`bootstrap_utils` / `linalg`), with a coherence + assert between the two (defense in depth). ### Added - **`ImputationDiD` leave-one-out conservative variance** (`leave_one_out`, default `False`) — the diff --git a/TODO.md b/TODO.md index c35b548fd..954177aa5 100644 --- a/TODO.md +++ b/TODO.md @@ -70,7 +70,6 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m | Render `docs/methodology/REPORTING.md` and `REGISTRY.md` as in-site Sphinx pages so cross-refs can use `:doc:` instead of off-site `blob/main` URLs (stable-docs readers can otherwise land on a different revision than their package version). Two paths: (a) add `myst-parser` to `conf.py` + docs extras and link with `:doc:`, or (b) convert both to `.rst`. **Note:** REGISTRY.md is ~4.5k lines of LaTeX-heavy markdown — high risk under the `-W` (warnings-as-errors) Sphinx build; budget multiple rounds. | `docs/conf.py`, `docs/api/business_report.rst`, `docs/api/diagnostic_report.rst`, tutorials 18 & 19 | follow-up | Mid | Low | | `ImputationDiD` covariate-path variance lacks a dedicated parity anchor — only the no-covariate staggered panel is R-parity'd, though the covariate path shares the same validated projection code. Add a small dense-design **hand-calc** for the covariate projection (no external tooling), or a covariate (time-varying X) R `didimputation` golden asserting overall/ES SE parity (the golden variant needs local R). | `tests/test_methodology_imputation.py`, `benchmarks/R/generate_didimputation_golden.R` | imputation-validation | Mid | Low | | Add true half-sample BRR replicate-weight regressions per estimator family (current tests use Fay-like 0.5/1.5 perturbations; `test_survey_phase6.py` covers true BRR at the helper level). | `tests/test_replicate_weight_expansion.py` | #253 | Mid | Low | -| `test_bootstrap_se_matches_pre_pr4_baseline` (dCDH bit-identity guard) fails under FULL-SUITE order but passes standalone/as-a-file: its `pure_python` backend detection reads a state some earlier test perturbs, so it compares against the wrong per-backend baseline arm (observed failure message says `backend=pure-python` while the run produces the other arm's value). Reproduced identically on pristine `origin/main` (2026-07-05, `b56931a6`), so it pre-dates the CS reg/ipw IF fix. Make the backend detection order-robust (e.g. re-resolve from `diff_diff._backend` at call time, or isolate via monkeypatch). | `tests/test_survey_dcdh.py::TestBootstrapCellPeriod` | CS-scaling | Quick | Low | | Port the CI `` extraction into the reviewer-eval harness so `docs/tutorials/*.ipynb` cases (currently guarded out of `verify-corpus`/`run`) can be reviewed with CI-equivalent context. | `tools/reviewer-eval/adapters/ci_prompt.py` | local-review | Mid | Low | --- diff --git a/tests/test_doc_snippets.py b/tests/test_doc_snippets.py index c98791408..a8e50a2fa 100644 --- a/tests/test_doc_snippets.py +++ b/tests/test_doc_snippets.py @@ -8,6 +8,7 @@ snippets and optional-dependency guards like matplotlib). """ +import os import re import textwrap from pathlib import Path @@ -375,11 +376,20 @@ def _restore_datasets_module(): [pytest.param(tid, c, s, id=tid) for tid, c, s in _CASES], ) def test_doc_snippet(test_id: str, code: str, skip_reason: Optional[str]): - """Execute a documentation code snippet and assert no API/runtime errors.""" + """Execute a documentation code snippet and assert no API/runtime errors. + + ``os.environ`` is snapshot/restored around the exec: snippets may + legitimately mutate the environment (e.g. the troubleshooting + backend-override block sets ``DIFF_DIFF_BACKEND='python'``), and an + unreverted mutation leaks process state into every later test in the + session (it flipped the backend-arm selection of the dCDH pinned + bootstrap baseline under full-suite order). + """ if skip_reason: pytest.skip(skip_reason) ns = _build_namespace() + env_snapshot = os.environ.copy() try: exec(compile(code, f"<{test_id}>", "exec"), ns) except NameError as exc: @@ -411,3 +421,8 @@ def test_doc_snippet(test_id: str, code: str, skip_reason: Optional[str]): f"Snippet {test_id} raised {type(exc).__name__}: {exc}\n\n" f"Code:\n{textwrap.indent(code, ' ')}" ) + finally: + # Revert any environment mutation the snippet made (pytest.fail + # raises, so this must be a finally, not a trailing statement). + os.environ.clear() + os.environ.update(env_snapshot) diff --git a/tests/test_survey_dcdh.py b/tests/test_survey_dcdh.py index 69b5a400d..a44b90da1 100644 --- a/tests/test_survey_dcdh.py +++ b/tests/test_survey_dcdh.py @@ -1,6 +1,5 @@ """Survey support tests for ChaisemartinDHaultfoeuille (dCDH).""" -import os from typing import Optional import numpy as np @@ -1865,12 +1864,30 @@ def test_bootstrap_se_matches_pre_pr4_baseline(self): to ULP precision. The baseline values were captured on `origin/main` at `ac181b7f` (the PR #329 merge) under each backend independently. + + The baseline arm is selected from the SAME dispatch globals + the fit consumes (bound at import into bootstrap_utils / + linalg). Reading ``os.environ`` or ``diff_diff._backend`` at + call time is order-fragile: an env mutation after import + (e.g. a leaked doc-snippet backend override) flips that + detection without changing the already-imported dispatch, + selecting the wrong pinned-baseline arm under full-suite + order. """ - from diff_diff._backend import HAS_RUST_BACKEND - pure_python = ( - os.environ.get("DIFF_DIFF_BACKEND", "auto").lower() == "python" - or not HAS_RUST_BACKEND + from diff_diff import bootstrap_utils as _bu + from diff_diff import linalg as _la + + bootstrap_rust = bool( + _bu.HAS_RUST_BACKEND and _bu._rust_bootstrap_weights is not None + ) + ols_rust = bool(_la.HAS_RUST_BACKEND and _la._rust_solve_ols is not None) + assert bootstrap_rust == ols_rust, ( + "Incoherent backend dispatch state between bootstrap_utils " + f"(rust={bootstrap_rust}) and linalg (rust={ols_rust}) — a " + "leaked monkeypatch? No pinned baseline exists for a mixed " + "backend; refusing to compare against either arm." ) + pure_python = not bootstrap_rust expected = ( self._BASELINE_OVERALL_SE_PYTHON if pure_python