From 9e2f1268f0fb813372dbecda42663567e96234be Mon Sep 17 00:00:00 2001 From: marcuswood-oai <323319819+marcuswood-oai@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:58:25 -0700 Subject: [PATCH 1/2] fix: compare consumer type diagnostics against the baseline --- scripts/check-consumer-compatibility.py | 110 ++++++++++++++++++++++++ scripts/detect-breaking-changes | 23 +---- tests/test_consumer_compatibility.py | 43 +++++++++ 3 files changed, 157 insertions(+), 19 deletions(-) create mode 100644 scripts/check-consumer-compatibility.py create mode 100644 tests/test_consumer_compatibility.py diff --git a/scripts/check-consumer-compatibility.py b/scripts/check-consumer-compatibility.py new file mode 100644 index 0000000000..88daf3ac84 --- /dev/null +++ b/scripts/check-consumer-compatibility.py @@ -0,0 +1,110 @@ +from __future__ import annotations + +import os +import sys +import json +import shutil +import tarfile +import tempfile +import subprocess +from typing import Any +from pathlib import Path +from collections import Counter + +ROOT = Path(__file__).resolve().parents[1] +CONFIG = ROOT / "scripts/pyrightconfig.breaking-changes.json" + + +def snapshot_baseline(ref: str, destination: Path) -> None: + commit = subprocess.check_output( + ["git", "rev-parse", "--verify", "--end-of-options", f"{ref}^{{commit}}"], cwd=ROOT, text=True + ).strip() + with tempfile.TemporaryFile() as archive: + subprocess.run(["git", "archive", commit, "src", "tests"], cwd=ROOT, stdout=archive, check=True) + archive.seek(0) + with tarfile.open(fileobj=archive) as contents: + for member in contents: + path = Path(member.name) + if path.is_absolute() or ".." in path.parts or not (member.isfile() or member.isdir()): + raise RuntimeError(f"Unsupported baseline archive entry: {member.name}") + target = destination / path + if member.isdir(): + target.mkdir(parents=True, exist_ok=True) + else: + target.parent.mkdir(parents=True, exist_ok=True) + source = contents.extractfile(member) + assert source is not None + with source, target.open("wb") as output: + shutil.copyfileobj(source, output) + + +def check(snapshot: Path) -> list[dict[str, Any]]: + # Keep the current checker configuration and dependencies fixed. Only the + # SDK source changes; all tests and their helpers come from the baseline. + # Preserve relative include and import paths by keeping the config in scripts/. + config_path = snapshot / "scripts" / CONFIG.name + config_path.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(CONFIG, config_path) + result = subprocess.run( + [str(ROOT / "scripts/run-pyright"), "--outputjson", "--pythonpath", sys.executable], + cwd=ROOT, + env={**os.environ, "PYRIGHT_PROJECT": str(config_path)}, + capture_output=True, + text=True, + ) + if result.returncode not in (0, 1): + raise RuntimeError(f"Pyright failed ({result.returncode}): {result.stderr}\n{result.stdout}") + report = json.loads(result.stdout) + errors = [diagnostic for diagnostic in report["generalDiagnostics"] if diagnostic["severity"] == "error"] + if ( + report["summary"]["filesAnalyzed"] == 0 + or report["summary"]["errorCount"] != len(errors) + or result.returncode != bool(errors) + ): + raise RuntimeError("Pyright returned an incomplete or inconsistent report") + return errors + + +def diagnostic_key(diagnostic: dict[str, Any], snapshot: Path) -> str: + # Compare full messages and locations, not just counts or rule names: a + # different error on an already failing line is still a regression. + key = {field: diagnostic.get(field) for field in ("file", "range", "rule", "message", "severity")} + return json.dumps(key, sort_keys=True).replace(str(snapshot), "") + + +def new_errors(baseline: list[dict[str, Any]], candidate: list[dict[str, Any]], snapshot: Path) -> list[dict[str, Any]]: + remaining = Counter(diagnostic_key(diagnostic, snapshot) for diagnostic in baseline) + added = [] + for diagnostic in candidate: + key = diagnostic_key(diagnostic, snapshot) + if remaining[key]: + remaining[key] -= 1 + else: + added.append(diagnostic) + return added + + +def main(ref: str) -> int: + with tempfile.TemporaryDirectory(prefix="openai-consumer-compatibility-") as directory: + snapshot = Path(directory).resolve() + snapshot_baseline(ref, snapshot) + print("==> Checking previous callers against the baseline SDK", flush=True) + baseline = check(snapshot) + shutil.rmtree(snapshot / "src") + shutil.copytree(ROOT / "src", snapshot / "src", ignore=shutil.ignore_patterns("__pycache__", "*.pyc")) + print("==> Checking the same callers against the candidate SDK", flush=True) + candidate = check(snapshot) + added = new_errors(baseline, candidate, snapshot) + print(f"Baseline: {len(baseline)} errors; candidate: {len(candidate)} errors; new: {len(added)}") + for diagnostic in added: + path = Path(diagnostic["file"]).relative_to(snapshot) + start = diagnostic["range"]["start"] + print(f"{path}:{start['line'] + 1}:{start['character'] + 1}: {diagnostic['message']}") + print(f" [{diagnostic.get('rule', 'unknown')}]") + return 1 if added else 0 + + +if __name__ == "__main__": + if len(sys.argv) != 2: + raise SystemExit("Usage: check-consumer-compatibility.py BASE_REF") + raise SystemExit(main(sys.argv[1])) diff --git a/scripts/detect-breaking-changes b/scripts/detect-breaking-changes index bfb8d52858..9cb360b686 100755 --- a/scripts/detect-breaking-changes +++ b/scripts/detect-breaking-changes @@ -1,24 +1,9 @@ #!/usr/bin/env bash -set -e +set -euo pipefail cd "$(dirname "$0")/.." -echo "==> Detecting breaking changes" - -TEST_PATHS=( - tests/api_resources - tests/test_client.py - tests/test_response.py - tests/test_legacy_response.py -) - -for PATHSPEC in "${TEST_PATHS[@]}"; do - # Try to check out previous versions of the test files - # with the current SDK. - git checkout "$1" -- "${PATHSPEC}" 2>/dev/null || true -done - -# Check unchanged previous-version callers with Pyright. Repository development -# uses ty; this isolated gate protects consumers who use another checker. -uv run --locked --all-extras scripts/run-pyright +# Repository development uses ty. Compare unchanged callers with Pyright to +# detect new consumer errors without failing on existing fixture diagnostics. +uv run --locked --all-extras python scripts/check-consumer-compatibility.py "$1" diff --git a/tests/test_consumer_compatibility.py b/tests/test_consumer_compatibility.py new file mode 100644 index 0000000000..1954f24074 --- /dev/null +++ b/tests/test_consumer_compatibility.py @@ -0,0 +1,43 @@ +from __future__ import annotations + +import json +import runpy +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts/check-consumer-compatibility.py" + + +@pytest.mark.parametrize("change", ["none", "message", "location", "duplicate"]) +def test_existing_diagnostics_do_not_hide_new_errors(tmp_path: Path, change: str) -> None: + detector = runpy.run_path(str(SCRIPT)) + original = { + "file": str(tmp_path / "consumer.py"), + "range": {"start": {"line": 3, "character": 0}, "end": {"line": 3, "character": 10}}, + "severity": "error", + "rule": "reportMissingTypeArgument", + "message": 'Expected type arguments for generic class "dict"', + } + changed = dict(original) + if change == "message": + changed["message"] = 'Expected type arguments for generic class "Model"' + elif change == "location": + changed["file"] = str(tmp_path / "another_consumer.py") + candidate = [original, changed] if change == "duplicate" else [changed] + assert detector["new_errors"]([original], candidate, tmp_path) == ([] if change == "none" else [changed]) + + +@pytest.mark.parametrize("failure", ["crash", "invalid_json", "no_files", "missing_errors"]) +def test_checker_failures_cannot_pass_the_gate(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure: str) -> None: + detector = runpy.run_path(str(SCRIPT)) + output = json.dumps({"generalDiagnostics": [], "summary": {"filesAnalyzed": 0, "errorCount": 0}}) + if failure == "invalid_json": + output = "not JSON" + elif failure == "missing_errors": + output = json.dumps({"generalDiagnostics": [], "summary": {"filesAnalyzed": 1, "errorCount": 1}}) + result = subprocess.CompletedProcess([], 2 if failure == "crash" else 0, stdout=output, stderr="") + monkeypatch.setattr(subprocess, "run", lambda *_args, **_kwargs: result) + with pytest.raises((RuntimeError, ValueError)): + detector["check"](tmp_path) From 5336870aec25db0a30ca075d6a3c53ca14799064 Mon Sep 17 00:00:00 2001 From: Marcus Wood Date: Wed, 28 Oct 2026 14:33:16 -0700 Subject: [PATCH 2/2] fix: keep current and previous Pyright callers strict --- scripts/check-consumer-compatibility.py | 110 ------------------------ scripts/detect-breaking-changes | 27 +++++- tests/test_client.py | 8 +- tests/test_consumer_compatibility.py | 80 ++++++++++------- 4 files changed, 75 insertions(+), 150 deletions(-) delete mode 100644 scripts/check-consumer-compatibility.py diff --git a/scripts/check-consumer-compatibility.py b/scripts/check-consumer-compatibility.py deleted file mode 100644 index 88daf3ac84..0000000000 --- a/scripts/check-consumer-compatibility.py +++ /dev/null @@ -1,110 +0,0 @@ -from __future__ import annotations - -import os -import sys -import json -import shutil -import tarfile -import tempfile -import subprocess -from typing import Any -from pathlib import Path -from collections import Counter - -ROOT = Path(__file__).resolve().parents[1] -CONFIG = ROOT / "scripts/pyrightconfig.breaking-changes.json" - - -def snapshot_baseline(ref: str, destination: Path) -> None: - commit = subprocess.check_output( - ["git", "rev-parse", "--verify", "--end-of-options", f"{ref}^{{commit}}"], cwd=ROOT, text=True - ).strip() - with tempfile.TemporaryFile() as archive: - subprocess.run(["git", "archive", commit, "src", "tests"], cwd=ROOT, stdout=archive, check=True) - archive.seek(0) - with tarfile.open(fileobj=archive) as contents: - for member in contents: - path = Path(member.name) - if path.is_absolute() or ".." in path.parts or not (member.isfile() or member.isdir()): - raise RuntimeError(f"Unsupported baseline archive entry: {member.name}") - target = destination / path - if member.isdir(): - target.mkdir(parents=True, exist_ok=True) - else: - target.parent.mkdir(parents=True, exist_ok=True) - source = contents.extractfile(member) - assert source is not None - with source, target.open("wb") as output: - shutil.copyfileobj(source, output) - - -def check(snapshot: Path) -> list[dict[str, Any]]: - # Keep the current checker configuration and dependencies fixed. Only the - # SDK source changes; all tests and their helpers come from the baseline. - # Preserve relative include and import paths by keeping the config in scripts/. - config_path = snapshot / "scripts" / CONFIG.name - config_path.parent.mkdir(parents=True, exist_ok=True) - shutil.copyfile(CONFIG, config_path) - result = subprocess.run( - [str(ROOT / "scripts/run-pyright"), "--outputjson", "--pythonpath", sys.executable], - cwd=ROOT, - env={**os.environ, "PYRIGHT_PROJECT": str(config_path)}, - capture_output=True, - text=True, - ) - if result.returncode not in (0, 1): - raise RuntimeError(f"Pyright failed ({result.returncode}): {result.stderr}\n{result.stdout}") - report = json.loads(result.stdout) - errors = [diagnostic for diagnostic in report["generalDiagnostics"] if diagnostic["severity"] == "error"] - if ( - report["summary"]["filesAnalyzed"] == 0 - or report["summary"]["errorCount"] != len(errors) - or result.returncode != bool(errors) - ): - raise RuntimeError("Pyright returned an incomplete or inconsistent report") - return errors - - -def diagnostic_key(diagnostic: dict[str, Any], snapshot: Path) -> str: - # Compare full messages and locations, not just counts or rule names: a - # different error on an already failing line is still a regression. - key = {field: diagnostic.get(field) for field in ("file", "range", "rule", "message", "severity")} - return json.dumps(key, sort_keys=True).replace(str(snapshot), "") - - -def new_errors(baseline: list[dict[str, Any]], candidate: list[dict[str, Any]], snapshot: Path) -> list[dict[str, Any]]: - remaining = Counter(diagnostic_key(diagnostic, snapshot) for diagnostic in baseline) - added = [] - for diagnostic in candidate: - key = diagnostic_key(diagnostic, snapshot) - if remaining[key]: - remaining[key] -= 1 - else: - added.append(diagnostic) - return added - - -def main(ref: str) -> int: - with tempfile.TemporaryDirectory(prefix="openai-consumer-compatibility-") as directory: - snapshot = Path(directory).resolve() - snapshot_baseline(ref, snapshot) - print("==> Checking previous callers against the baseline SDK", flush=True) - baseline = check(snapshot) - shutil.rmtree(snapshot / "src") - shutil.copytree(ROOT / "src", snapshot / "src", ignore=shutil.ignore_patterns("__pycache__", "*.pyc")) - print("==> Checking the same callers against the candidate SDK", flush=True) - candidate = check(snapshot) - added = new_errors(baseline, candidate, snapshot) - print(f"Baseline: {len(baseline)} errors; candidate: {len(candidate)} errors; new: {len(added)}") - for diagnostic in added: - path = Path(diagnostic["file"]).relative_to(snapshot) - start = diagnostic["range"]["start"] - print(f"{path}:{start['line'] + 1}:{start['character'] + 1}: {diagnostic['message']}") - print(f" [{diagnostic.get('rule', 'unknown')}]") - return 1 if added else 0 - - -if __name__ == "__main__": - if len(sys.argv) != 2: - raise SystemExit("Usage: check-consumer-compatibility.py BASE_REF") - raise SystemExit(main(sys.argv[1])) diff --git a/scripts/detect-breaking-changes b/scripts/detect-breaking-changes index 9cb360b686..cb80da6998 100755 --- a/scripts/detect-breaking-changes +++ b/scripts/detect-breaking-changes @@ -3,7 +3,28 @@ set -euo pipefail cd "$(dirname "$0")/.." +base=$(git rev-parse --verify --end-of-options "${1:?expected a base revision}^{commit}") -# Repository development uses ty. Compare unchanged callers with Pyright to -# detect new consumer errors without failing on existing fixture diagnostics. -uv run --locked --all-extras python scripts/check-consumer-compatibility.py "$1" +# Keep today's callers Pyright-clean before they become tomorrow's baseline. +echo '==> Checking current callers against the candidate SDK' +uv run --locked --all-extras scripts/run-pyright + +# If the tests are identical, the first run already checked the base callers. +# A change confined to tests and this script has no new SDK to compare. +test_changes=$(git diff --name-only "$base" -- tests) +other_changes=$(git diff --name-only "$base" -- . ':(exclude)tests' ':(exclude)scripts/detect-breaking-changes') +if [[ -z "$test_changes" || -z "$other_changes" ]]; then + exit 0 +fi + +# Check the complete base test tree, including its helpers, against today's +# src and checker configuration. Leave the working tree and Git index intact. +echo '==> Checking previous callers against the candidate SDK' +snapshot=$(mktemp -d) +trap 'rm -rf -- "$snapshot"' EXIT +git archive "$base" tests | tar -x -C "$snapshot" +ln -s "$PWD/src" "$snapshot/src" +mkdir "$snapshot/scripts" +config=scripts/pyrightconfig.breaking-changes.json +cp "$config" "$snapshot/$config" +PYRIGHT_PROJECT="$snapshot/$config" uv run --locked --all-extras scripts/run-pyright diff --git a/tests/test_client.py b/tests/test_client.py index b97d53574f..8626b7b54f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1120,8 +1120,8 @@ def test_binary_content_upload_with_body_is_deprecated(self, respx2_mock: MockRo @pytest.mark.parametrize("client", [False], indirect=True) def test_bare_container_response(self, respx2_mock: MockRouter, client: OpenAI) -> None: class Model(BaseModel): - metadata: dict - items: list + metadata: dict # pyright: ignore[reportMissingTypeArgument] + items: list # pyright: ignore[reportMissingTypeArgument] data = {"metadata": {"key": "value"}, "items": [1, "two"]} respx2_mock.get("/foo").mock(return_value=httpx2.Response(200, json=data)) @@ -2510,8 +2510,8 @@ async def test_binary_content_upload_with_body_is_deprecated( @pytest.mark.parametrize("async_client", [False], indirect=True) async def test_bare_container_response(self, respx2_mock: MockRouter, async_client: AsyncOpenAI) -> None: class Model(BaseModel): - metadata: dict - items: list + metadata: dict # pyright: ignore[reportMissingTypeArgument] + items: list # pyright: ignore[reportMissingTypeArgument] data = {"metadata": {"key": "value"}, "items": [1, "two"]} respx2_mock.get("/foo").mock(return_value=httpx2.Response(200, json=data)) diff --git a/tests/test_consumer_compatibility.py b/tests/test_consumer_compatibility.py index 1954f24074..444f9236cb 100644 --- a/tests/test_consumer_compatibility.py +++ b/tests/test_consumer_compatibility.py @@ -1,43 +1,57 @@ from __future__ import annotations -import json -import runpy +import os +import shutil import subprocess from pathlib import Path import pytest -SCRIPT = Path(__file__).resolve().parents[1] / "scripts/check-consumer-compatibility.py" +@pytest.mark.skipif(os.name != "posix", reason="The compatibility gate runs in Bash on CI") +@pytest.mark.parametrize("also_change", [None, "src/model.py", "pyproject.toml"]) +def test_compatibility_gate_selects_callers(tmp_path: Path, also_change: str | None) -> None: + scripts = tmp_path / "scripts" + scripts.mkdir() + gate = scripts / "detect-breaking-changes" + shutil.copy(Path(__file__).resolve().parents[1] / "scripts" / gate.name, gate) + (scripts / "pyrightconfig.breaking-changes.json").write_text("{}") + (tmp_path / "tests").mkdir() + (tmp_path / "src").mkdir() + (tmp_path / "src/model.py").write_text("original SDK\n") + caller = tmp_path / "tests/caller.py" + caller.write_text("previous caller\n") -@pytest.mark.parametrize("change", ["none", "message", "location", "duplicate"]) -def test_existing_diagnostics_do_not_hide_new_errors(tmp_path: Path, change: str) -> None: - detector = runpy.run_path(str(SCRIPT)) - original = { - "file": str(tmp_path / "consumer.py"), - "range": {"start": {"line": 3, "character": 0}, "end": {"line": 3, "character": 10}}, - "severity": "error", - "rule": "reportMissingTypeArgument", - "message": 'Expected type arguments for generic class "dict"', - } - changed = dict(original) - if change == "message": - changed["message"] = 'Expected type arguments for generic class "Model"' - elif change == "location": - changed["file"] = str(tmp_path / "another_consumer.py") - candidate = [original, changed] if change == "duplicate" else [changed] - assert detector["new_errors"]([original], candidate, tmp_path) == ([] if change == "none" else [changed]) + # Record the checker inputs to exercise the real Git and snapshot routing. + uv = tmp_path / "uv" + uv.write_text( + "#!/bin/sh\nset -eu\nconfig=${PYRIGHT_PROJECT:-scripts/pyrightconfig.breaking-changes.json}\n" + 'root=$(dirname "$config")/..\ncat "$root/tests/caller.py" "$root/src/model.py"\n' + ) + uv.chmod(0o755) + subprocess.run(["git", "init", "--quiet"], cwd=tmp_path, check=True) + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + subprocess.run( + ["git", "-c", "user.name=Test", "-c", "user.email=test@example.org", "commit", "--no-gpg-sign", "-qm", "base"], + cwd=tmp_path, + check=True, + ) - -@pytest.mark.parametrize("failure", ["crash", "invalid_json", "no_files", "missing_errors"]) -def test_checker_failures_cannot_pass_the_gate(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure: str) -> None: - detector = runpy.run_path(str(SCRIPT)) - output = json.dumps({"generalDiagnostics": [], "summary": {"filesAnalyzed": 0, "errorCount": 0}}) - if failure == "invalid_json": - output = "not JSON" - elif failure == "missing_errors": - output = json.dumps({"generalDiagnostics": [], "summary": {"filesAnalyzed": 1, "errorCount": 1}}) - result = subprocess.CompletedProcess([], 2 if failure == "crash" else 0, stdout=output, stderr="") - monkeypatch.setattr(subprocess, "run", lambda *_args, **_kwargs: result) - with pytest.raises((RuntimeError, ValueError)): - detector["check"](tmp_path) + caller.write_text("current caller\n") + if also_change is not None: + (tmp_path / also_change).write_text("changed input\n") + subprocess.run(["git", "add", also_change], cwd=tmp_path, check=True) + result = subprocess.run( + [str(gate), "HEAD"], + cwd=tmp_path, + env={**os.environ, "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}"}, + text=True, + capture_output=True, + check=True, + ) + sdk = "changed input" if also_change == "src/model.py" else "original SDK" + expected = ["current caller", sdk] + if also_change is not None: + expected.extend(["previous caller", sdk]) + assert [line for line in result.stdout.splitlines() if not line.startswith("==>")] == expected + assert caller.read_text() == "current caller\n"