Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .github/workflows/test-python.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ on:
pull_request:
paths:
- "sdk-python/**"
- "scripts/ga/cleanroom_python_assert.py"
- ".github/workflows/test-python.yml"
push:
branches: [main]
paths:
- "sdk-python/**"
- "scripts/ga/cleanroom_python_assert.py"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The push filter adds the probe path but not this workflow file, so workflow-only changes merged to main will not run the smoke-install gate. [logic error]

Assessment: 🟠 Major · 🔁 Occurrence: Rarely

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** .github/workflows/test-python.yml
**Line:** 16:16
**Comment:**
	*Logic Error: The push filter adds the probe path but not this workflow file, so workflow-only changes merged to `main` will not run the smoke-install gate.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
Expand All @@ -37,3 +39,48 @@ jobs:
run: python -m pip install --upgrade pip && python -m pip install -e '.[dev]'
- name: Test (offline)
run: python -m pytest -q

smoke-install:
# Regression guard for the fresh-install class of bug: builds the actual wheel from
# this checkout, installs it (no `-e`, no repo on sys.path) into a throwaway venv,
# and runs the SAME probe `scripts/ga/registry-cleanroom.mjs` runs against the live
# PyPI artifact after a publish. `python -m pytest` from the repo checkout does NOT
# catch this class of bug (the checkout dir is first on sys.path, which is exactly
# how wave-av-sdk 2.0.0's `wave` -> stdlib collision hid through review and CI) — this
# job is the one gate that runs it the way a real `pip install wave-av-sdk` user does,
# and it runs on every PR, before any registry publish is even possible.
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.10", "3.12"]
steps:
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
- uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5
with:
python-version: ${{ matrix.python-version }}
- name: Build wheel
working-directory: sdk-python
run: python -m pip install --upgrade pip && pip install build && python -m build --wheel
- name: Create fresh venv (no repo on sys.path)
run: python -m venv "$RUNNER_TEMP/smoke"
- name: Install the built wheel
run: |
WHEEL=$(ls sdk-python/dist/*.whl)
"$RUNNER_TEMP/smoke/bin/pip" install --upgrade pip
"$RUNNER_TEMP/smoke/bin/pip" install "$WHEEL"
- name: Copy the clean-room probe into the smoke venv
run: cp scripts/ga/cleanroom_python_assert.py "$RUNNER_TEMP/smoke/cleanroom_python_assert.py"
- name: Assert the installed wheel imports and does not shadow the stdlib
working-directory: ${{ runner.temp }}/smoke
run: |
set -euo pipefail
out=$(bin/python cleanroom_python_assert.py --dist wave-av-sdk --module wave_sdk --symbol Wave)
echo "$out"
echo "$out" | bin/python -c '
import json, sys
report = json.load(sys.stdin)
failed = [c["name"] for c in report["checks"] if not c["ok"]]
if failed:
print("FAILED:", failed)
sys.exit(1)
'
13 changes: 11 additions & 2 deletions scripts/ga/cleanroom-targets.mjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Per-ecosystem target runners: stand up the clean room, install the published artifact, then
// hand a context to the checks. Nothing here reads the repository checkout.

import { mkdtempSync } from 'node:fs';
import { copyFileSync, mkdtempSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
Expand Down Expand Up @@ -244,7 +244,16 @@ export async function runPypiTarget(target, args) {
// cwd is the throwaway room, never the repo: a checkout on sys.path could satisfy an import the
// published wheel is supposed to satisfy — exactly the illusion this suite exists to destroy.
// cleanroom_python_assert.py re-verifies that independently and reports it as its own check.
const argv = [join(HERE, 'cleanroom_python_assert.py'), '--dist', name, '--module', target.import_module];
//
// The probe is COPIED into the room before it runs. Python unconditionally prepends the
// executed script's own directory to sys.path, so running it in place put `<repo>/scripts/ga`
// — a repository directory — first on the path of every clean-room probe, which is the one
// thing this whole suite promises never happens. (`-P`/PYTHONSAFEPATH would also fix it but
// is 3.11+; the venv's interpreter version is not ours to assume.) `.github/workflows/
// test-python.yml`'s smoke-install job already copies the probe for the same reason.
const probePath = join(room, 'cleanroom_python_assert.py');
copyFileSync(join(HERE, 'cleanroom_python_assert.py'), probePath);
const argv = [probePath, '--dist', name, '--module', target.import_module];
if (target.import_symbol) argv.push('--symbol', target.import_symbol);
const probe = run(venv.py, argv, { cwd: room, timeout: 180000 });
let parsed;
Expand Down
74 changes: 70 additions & 4 deletions scripts/ga/cleanroom_python_assert.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,69 @@ def check(name: str, ok: bool, detail: str) -> dict:
return {"name": name, "ok": bool(ok), "detail": detail}


# Source-root layouts a checkout can present for the module under test. The clean room must
# not be able to satisfy `import <module>` from ANY of them, and the guard must not be keyed
# on one repository's directory names — that is precisely how it went blind before:
# "" wave-av/sdk-python -> `wave_sdk/` at the repository root
# "sdk-python" wave-av/sdks -> `sdk-python/wave_sdk/`
# "src"/"python" the two other conventional source roots, so a future re-layout of either
# repo does not silently disarm this check again
CHECKOUT_SUBROOTS: tuple[str, ...] = ("", "sdk-python", "src", "python")


def interpreter_owned(path: str) -> bool:
"""True when a sys.path entry belongs to the interpreter/venv under test.

site-packages (where the published wheel legitimately installs), the stdlib, and
everything else under the venv's own prefix are not leaks. They must be excluded
explicitly, because the leak test below asks "can this entry supply the module?" and
the correct answer for site-packages is yes. The install roots are read from sysconfig
and `site` rather than assumed to live under `sys.prefix`, so a `--user` install (which
does not) cannot be mistaken for a checkout.
"""
rp = realpath(path)
if not rp:
return False
roots = [realpath(sys.prefix), realpath(getattr(sys, "base_prefix", sys.prefix))]
paths = sysconfig.get_paths()
roots += [realpath(paths[k]) for k in ("purelib", "platlib", "stdlib", "platstdlib") if k in paths]
try:
import site

roots += [realpath(p) for p in (getattr(site, "USER_SITE", None), getattr(site, "USER_BASE", None)) if p]
except ImportError: # pragma: no cover - `site` is unimportable only under -S
pass
return any(r and (rp == r or rp.startswith(r + os.sep)) for r in roots)


def checkout_paths_providing(paths: list[str], module: str) -> list[str]:
"""sys.path entries from which a SOURCE CHECKOUT could satisfy `import <module>`.

Layout-independent on purpose. The previous implementation looked for one hardcoded
path (`<entry>/sdk-python/wave`); when `wave` was renamed to `wave_sdk` it matched
nothing anywhere and reported "no repo checkout on sys.path" unconditionally — a guard
that cannot fail, which is worse than no guard, because the whole clean-room result
rests on it. Keying on the module actually under test, across the layouts a checkout
can have, is what makes it self-maintaining.
"""
hits: list[str] = []
for p in paths:
entry = p or os.getcwd() # '' means cwd, which can absolutely be a checkout
if interpreter_owned(entry):
continue
for sub in CHECKOUT_SUBROOTS:
base = os.path.join(entry, sub) if sub else entry
pkg_dir = os.path.join(base, module)
mod_file = os.path.join(base, module + ".py")
if os.path.isdir(pkg_dir):
hits.append(pkg_dir)
break
if os.path.isfile(mod_file):
hits.append(mod_file)
break
return hits


def dist_top_level(dist_name: str) -> list[str]:
"""Top-level import names the installed distribution claims.

Expand Down Expand Up @@ -84,10 +147,13 @@ def main() -> int:
checks: list[dict] = []

# Guard: a repo checkout on sys.path would make this whole run meaningless.
repo_marker_on_path = [
p for p in sys.path
if p and os.path.isdir(os.path.join(p, "sdk-python", "wave"))
]
# Keyed on `args.module` (the same name the import check below uses) and on every
# source-root layout a checkout can present, not on a literal `sdk-python/wave`:
# that literal was the pre-rename package directory of ONE of the two repositories
# this probe runs against, so after the `wave` -> `wave_sdk` rename (ART-001) it
# matched nothing at all and this guard reported a pass unconditionally — blind to
# the exact repo-on-sys.path leak it exists to catch. See checkout_paths_providing().
repo_marker_on_path = checkout_paths_providing(list(sys.path), args.module)
checks.append(check(
"cleanroom-isolation",
not repo_marker_on_path,
Expand Down
3 changes: 3 additions & 0 deletions sdk-python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ dev = [
"mypy>=1.0.0",
"ruff>=0.1.0",
"black>=23.0.0",
# tests/test_packaging.py reads this file back to assert the shipped metadata
# matches the repo (name/version). tomllib is stdlib from 3.11 only.
"tomli>=2.0.0; python_version < '3.11'",
]

[project.urls]
Expand Down
144 changes: 144 additions & 0 deletions sdk-python/tests/test_cleanroom_probe.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
"""Unit guards for the clean-room isolation check in `scripts/ga/cleanroom_python_assert.py`.

WHY THESE EXIST
---------------
`cleanroom-isolation` is the load-bearing assertion of the entire registry clean-room gate:
every other check in that probe (`py-import-module`, `py-no-stdlib-shadow`) is only
meaningful if no source checkout can satisfy the import that the PUBLISHED WHEEL is
supposed to satisfy. If the isolation check is wrong, the gate can report a confident PASS
on a package that is completely broken for real users.

It WAS wrong. The check was keyed on one hardcoded path — `<sys.path entry>/sdk-python/wave`
— which was the pre-rename layout of exactly one of the two repositories the probe runs
against. After `wave` -> `wave_sdk` (ART-001) it matched nothing on any machine, so it
reported "no repo checkout on sys.path" unconditionally: a check that could not fail. That
is strictly worse than no check, because the rest of the gate's verdict rests on it.

A hardcoded-path guard has no test that would have caught this, so the fix is not just a
better path list — it is `checkout_paths_providing()`, a pure function over an explicit
sys.path, exercised below against real directory trees for BOTH repository layouts plus the
false-positive case (site-packages, where finding the module is the correct outcome). These
run offline in the normal `pytest` pass, so the guard can never again silently disarm
itself between releases.
"""

from __future__ import annotations

import importlib.util
import os
import sys
from pathlib import Path

import pytest

PROBE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ga" / "cleanroom_python_assert.py"


def _load_probe():
"""Import the GA probe by path — it is a script, not an installed module."""
assert PROBE_PATH.is_file(), (
f"clean-room probe not found at {PROBE_PATH}. If it moved, this test file and "
f".github/workflows/test-python.yml's path filter must move with it."
)
spec = importlib.util.spec_from_file_location("_ga_cleanroom_probe", PROBE_PATH)
assert spec and spec.loader
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module


probe = _load_probe()


def _make_checkout(root: Path, subroot: str, module: str) -> Path:
"""Create a source-checkout-shaped tree: <root>/<subroot>/<module>/__init__.py."""
pkg = (root / subroot / module) if subroot else (root / module)
pkg.mkdir(parents=True)
(pkg / "__init__.py").write_text("")
return pkg


# --- control: the helper can see anything at all ------------------------------------------


def test_flags_root_level_checkout_layout(tmp_path: Path):
"""`wave-av/sdk-python` layout: `wave_sdk/` sits at the repository root.

The pre-fix guard looked only under `<entry>/sdk-python/`, so this — a checkout of the
OTHER repository the probe runs against — was invisible to it.
"""
_make_checkout(tmp_path, "", "wave_sdk")
hits = probe.checkout_paths_providing([str(tmp_path)], "wave_sdk")
assert hits, "a root-level `wave_sdk/` on sys.path must be reported as a clean-room leak"


def test_flags_nested_sdk_python_checkout_layout(tmp_path: Path):
"""`wave-av/sdks` layout: `sdk-python/wave_sdk/`. This is this repository."""
_make_checkout(tmp_path, "sdk-python", "wave_sdk")
hits = probe.checkout_paths_providing([str(tmp_path)], "wave_sdk")
assert hits, "a `sdk-python/wave_sdk/` checkout on sys.path must be reported as a leak"


def test_flags_single_file_module(tmp_path: Path):
"""A leak does not have to be a package — `wave_sdk.py` shadows just as effectively."""
(tmp_path / "wave_sdk.py").write_text("")
hits = probe.checkout_paths_providing([str(tmp_path)], "wave_sdk")
assert hits, "a top-level `wave_sdk.py` on sys.path must be reported as a leak"


def test_flags_the_empty_sys_path_entry_meaning_cwd(tmp_path: Path, monkeypatch):
"""`''` on sys.path means the current directory, which can absolutely be a checkout.

The pre-fix guard skipped falsy entries outright, so `cd` into a checkout and run the
probe and it saw nothing.
"""
_make_checkout(tmp_path, "", "wave_sdk")
monkeypatch.chdir(tmp_path)
hits = probe.checkout_paths_providing([""], "wave_sdk")
assert hits, "the '' sys.path entry (cwd) must be resolved and checked, not skipped"


# --- false-positive controls: the guard must stay usable ----------------------------------


def test_does_not_flag_the_interpreters_own_site_packages():
"""Finding the module in site-packages is the CORRECT outcome, not a leak.

Without this exclusion the guard would fire on every legitimate run, and a guard that
fails when everything is fine gets deleted — which is how gates die.
"""
site_dirs = [p for p in sys.path if "site-packages" in p]
if not site_dirs:
pytest.skip("no site-packages on sys.path in this environment")
for entry in site_dirs:
assert probe.interpreter_owned(entry), (
f"{entry} is inside the interpreter prefix and must never be treated as a "
f"repository checkout"
)
assert probe.checkout_paths_providing(site_dirs, "wave_sdk") == []


def test_does_not_flag_an_unrelated_directory(tmp_path: Path):
"""A sys.path entry that cannot supply the module under test is not a leak."""
(tmp_path / "docs").mkdir()
assert probe.checkout_paths_providing([str(tmp_path)], "wave_sdk") == []


def test_guard_is_keyed_on_the_module_under_test(tmp_path: Path):
"""The self-maintenance property: rename the package, the guard follows it.

A checkout of the PRE-rename layout (`wave/`) is not a leak for `import wave_sdk`, and
a checkout of whatever the module is called today always is. This is the assertion the
hardcoded `"wave"` literal could not make, and its absence is why the rename disarmed
the check without a single test going red.
"""
_make_checkout(tmp_path, "sdk-python", "wave")
assert probe.checkout_paths_providing([str(tmp_path)], "wave_sdk") == []
assert probe.checkout_paths_providing([str(tmp_path)], "wave") != []


def test_reported_hit_points_at_the_offending_path(tmp_path: Path):
"""The failure message has to name the leaking path, not just say something is wrong."""
pkg = _make_checkout(tmp_path, "sdk-python", "wave_sdk")
hits = probe.checkout_paths_providing([str(tmp_path)], "wave_sdk")
assert [os.path.realpath(h) for h in hits] == [os.path.realpath(str(pkg))]
Loading
Loading