diff --git a/.github/scripts/check_sdk_api_breakage.py b/.github/scripts/check_sdk_api_breakage.py index 77179e09a9..3b062d056a 100755 --- a/.github/scripts/check_sdk_api_breakage.py +++ b/.github/scripts/check_sdk_api_breakage.py @@ -31,17 +31,13 @@ class member must have been marked deprecated in the *previous* release using from __future__ import annotations import ast -import io import json import os import subprocess import sys -import tarfile -import tempfile import tomllib import urllib.request from collections.abc import Iterable -from contextlib import contextmanager from dataclasses import asdict, dataclass, field from pathlib import Path @@ -114,14 +110,6 @@ class FieldDefaultChange: ACP_BASE_REF_ENV = "ACP_VERSION_CHECK_BASE_REF" -def _get_base_ref() -> str | None: - base_ref = os.environ.get(ACP_BASE_REF_ENV) or os.environ.get("GITHUB_BASE_REF") - if not base_ref: - return None - base_ref = base_ref.strip() - return base_ref or None - - def read_version_from_pyproject(path: str) -> str: """Read the version string from a pyproject.toml file.""" with open(path, "rb") as f: @@ -178,12 +166,8 @@ def _min_version_from_requirement(req_str: str) -> pkg_version.Version | None: return max(lower_bounds) -def _git_ref_candidates(ref: str) -> tuple[str, ...]: - return tuple(dict.fromkeys((f"origin/{ref}", ref))) - - def _git_show_file(ref: str, rel_path: str) -> str | None: - for candidate in _git_ref_candidates(ref): + for candidate in (f"origin/{ref}", ref): result = subprocess.run( ["git", "show", f"{candidate}:{rel_path}"], check=False, @@ -195,29 +179,6 @@ def _git_show_file(ref: str, rel_path: str) -> str | None: return None -def _git_archive_directory( - repo_root: str, - ref: str, - rel_path: str, - dest_root: str, -) -> bool: - for candidate in _git_ref_candidates(ref): - result = subprocess.run( - ["git", "archive", "--format=tar", candidate, rel_path], - cwd=repo_root, - check=False, - capture_output=True, - ) - if result.returncode != 0: - continue - - with tarfile.open(fileobj=io.BytesIO(result.stdout)) as archive: - archive.extractall(dest_root, filter="data") - return True - - return False - - def _load_base_pyproject(base_ref: str) -> dict | None: rel_path = "openhands-sdk/pyproject.toml" content = _git_show_file(base_ref, rel_path) @@ -245,7 +206,7 @@ def _check_acp_version_bump(repo_root: str) -> int: ) return 0 - base_ref = _get_base_ref() + base_ref = os.environ.get(ACP_BASE_REF_ENV) or os.environ.get("GITHUB_BASE_REF") if not base_ref: print( "::warning title=ACP version::No base ref found; skipping ACP version check" @@ -607,23 +568,19 @@ def _object_path(obj: object | None) -> str: ) -def _write_field_default_change_report( - changes: list[FieldDefaultChange], - *, - field_default_changes_since_base: list[FieldDefaultChange] | None = None, -) -> None: +def _write_field_default_change_report(changes: list[FieldDefaultChange]) -> None: """Write detected public Field default changes to a JSON report file.""" report_path = os.environ.get(FIELD_DEFAULT_CHANGE_REPORT_ENV, "").strip() if not report_path: return - report = {"field_default_changes": [asdict(change) for change in changes]} - if field_default_changes_since_base is not None: - report["field_default_changes_since_base"] = [ - asdict(change) for change in field_default_changes_since_base - ] - - Path(report_path).write_text(json.dumps(report, indent=2) + "\n") + Path(report_path).write_text( + json.dumps( + {"field_default_changes": [asdict(change) for change in changes]}, + indent=2, + ) + + "\n" + ) def _member_deprecation_metadata( @@ -671,8 +628,6 @@ def _collect_breakages_pairs( title: str, package: str, field_default_changes: list[FieldDefaultChange] | None = None, - field_defaults_only: bool = False, - emit_diagnostics: bool = True, ) -> tuple[list[object], int]: """Find breaking changes between pairs of old/new API objects. @@ -699,23 +654,20 @@ def _collect_breakages_pairs( old_value = getattr(br, "old_value", None) new_value = getattr(br, "new_value", None) if _is_field_metadata_only_change(old_value, new_value): - if emit_diagnostics: - print( - f"::notice title={title}::Ignoring Field " - "metadata-only change (non-breaking): " - f"{obj.name if obj else 'unknown'}" - ) + print( + f"::notice title={title}::Ignoring Field metadata-only " + f"change (non-breaking): {obj.name if obj else 'unknown'}" + ) continue if _is_field_default_only_change(old_value, new_value): object_path = _object_path(obj) old_default = _field_default_repr(old_value) or "" new_default = _field_default_repr(new_value) or "" - if emit_diagnostics: - print( - f"::warning title={title}::Public Field default " - "changed (release-note-required): " - f"{object_path} {old_default} -> {new_default}" - ) + print( + f"::warning title={title}::Public Field default changed " + f"(release-note-required): {object_path} " + f"{old_default} -> {new_default}" + ) if field_default_changes is not None: field_default_changes.append( FieldDefaultChange( @@ -727,9 +679,6 @@ def _collect_breakages_pairs( ) continue - if field_defaults_only: - continue - print(br.explain(style=ExplanationStyle.GITHUB)) breakages.append(br) @@ -753,8 +702,6 @@ def _collect_breakages_pairs( print(f"::error title={title}::{error}") removal_policy_errors += len(errors) except AliasResolutionError as e: - if field_defaults_only: - continue if isinstance(old, Alias) or isinstance(new, Alias): old_target = old.target_path if isinstance(old, Alias) else None new_target = new.target_path if isinstance(new, Alias) else None @@ -780,8 +727,6 @@ def _collect_breakages_pairs( f"unresolved alias: {e}" ) except Exception as e: - if field_defaults_only: - raise RuntimeError("Failed to collect Field default changes") from e print(f"::warning title={title}::Failed to compute breakages: {e}") return breakages, removal_policy_errors @@ -912,36 +857,6 @@ def _load_current( return None -@contextmanager -def _load_from_git_ref( - griffe_module: object, - repo_root: str, - ref: str, - cfg: PackageConfig, -): - title = f"{cfg.distribution} API" - with tempfile.TemporaryDirectory() as tmpdir: - if not _git_archive_directory(repo_root, ref, cfg.source_dir, tmpdir): - print( - f"::warning title={title}::Failed to load {cfg.distribution} from " - f"git ref {ref}: unable to archive {cfg.source_dir}" - ) - yield None - return - - try: - yield griffe_module.load( - cfg.package, - search_paths=[os.path.join(tmpdir, cfg.source_dir)], - ) - except Exception as e: - print( - f"::warning title={title}::Failed to load {cfg.distribution} from " - f"git ref {ref}: {e}" - ) - yield None - - def _load_prev_from_pypi( griffe_module: object, prev: str, @@ -964,40 +879,6 @@ def _load_prev_from_pypi( return None -def _collect_field_default_changes_since_ref( - griffe_module: object, - repo_root: str, - ref: str, - cfg: PackageConfig, -) -> list[FieldDefaultChange] | None: - new_root = _load_current(griffe_module, repo_root, cfg) - if not new_root: - return None - - with _load_from_git_ref(griffe_module, repo_root, ref, cfg) as old_root: - if not old_root: - return None - - changes: list[FieldDefaultChange] = [] - try: - _compute_breakages( - old_root, - new_root, - cfg, - field_default_changes=changes, - field_defaults_only=True, - emit_diagnostics=False, - ) - except Exception as e: - print( - f"::warning title={cfg.distribution} API::Failed to compare " - f"Field defaults against base ref {ref}: {e}" - ) - return None - - return changes - - # Names of module-level data registries that declare deprecated public # re-exports as ``{name: {"deprecated_in": ..., "removed_in": ...}}`` and are # consumed by a module-level ``__getattr__``. The SDK uses this form for renamed @@ -1201,8 +1082,6 @@ def _compute_breakages( *, current_version: str = "9999.0.0", field_default_changes: list[FieldDefaultChange] | None = None, - field_defaults_only: bool = False, - emit_diagnostics: bool = True, ) -> tuple[int, int]: """Detect breaking changes between old and new package versions. @@ -1237,38 +1116,36 @@ def _compute_breakages( # evaluate) __all__, we can't compute meaningful breakages. # # In this situation, skip rather than failing the entire workflow. - if emit_diagnostics: - print( - f"::notice title={title}::Skipping breakage check; baseline release " - f"has no statically-evaluable {pkg}.__all__: {e}" - ) + print( + f"::notice title={title}::Skipping breakage check; baseline release " + f"has no statically-evaluable {pkg}.__all__: {e}" + ) return 0, 0 - if not field_defaults_only: - removed = sorted(old_exports - new_exports) - - # Check deprecation runway policy (exports) - for name in removed: - total_breaks += 1 # every removal is a structural break - errors = _deprecation_schedule_errors( - feature=name, - metadata=( - deprecated.metadata.get(name, DeprecationMetadata()) - if name in deprecated.top_level - else None - ), - current_version=current_version, + removed = sorted(old_exports - new_exports) + + # Check deprecation runway policy (exports) + for name in removed: + total_breaks += 1 # every removal is a structural break + errors = _deprecation_schedule_errors( + feature=name, + metadata=( + deprecated.metadata.get(name, DeprecationMetadata()) + if name in deprecated.top_level + else None + ), + current_version=current_version, + ) + if not errors: + print( + f"::notice title={title}::Removed previously-deprecated symbol " + f"'{name}' from {pkg}.__all__ after its scheduled removal version" ) - if not errors: - print( - f"::notice title={title}::Removed previously-deprecated symbol " - f"'{name}' from {pkg}.__all__ after its scheduled removal version" - ) - continue + continue - for error in errors: - print(f"::error title={title}::{error}") - removal_policy_errors += len(errors) + for error in errors: + print(f"::error title={title}::{error}") + removal_policy_errors += len(errors) common = sorted(old_exports & new_exports) pairs: list[tuple[object, object]] = [] @@ -1276,8 +1153,7 @@ def _compute_breakages( try: pairs.append((old_mod[name], new_mod[name])) except Exception as e: - if emit_diagnostics: - print(f"::warning title={title}::Unable to resolve symbol {name}: {e}") + print(f"::warning title={title}::Unable to resolve symbol {name}: {e}") breakages, member_policy_errors = _collect_breakages_pairs( pairs, @@ -1286,8 +1162,6 @@ def _compute_breakages( title=title, package=cfg.package, field_default_changes=field_default_changes, - field_defaults_only=field_defaults_only, - emit_diagnostics=emit_diagnostics, ) total_breaks += len(breakages) removal_policy_errors += member_policy_errors @@ -1357,8 +1231,6 @@ def main() -> int: import griffe field_default_changes: list[FieldDefaultChange] = [] - field_default_changes_since_base: list[FieldDefaultChange] | None = [] - base_ref = _get_base_ref() for cfg in PACKAGES: print(f"\n{'=' * 60}") print(f"Checking {cfg.distribution} ({cfg.package})") @@ -1369,24 +1241,8 @@ def main() -> int: cfg, field_default_changes=field_default_changes, ) - if base_ref and field_default_changes_since_base is not None: - changes_since_base = _collect_field_default_changes_since_ref( - griffe, - repo_root, - base_ref, - cfg, - ) - if changes_since_base is None: - field_default_changes_since_base = None - else: - field_default_changes_since_base.extend(changes_since_base) - _write_field_default_change_report( - field_default_changes, - field_default_changes_since_base=( - field_default_changes_since_base if base_ref else None - ), - ) + _write_field_default_change_report(field_default_changes) return rc diff --git a/.github/workflows/api-breakage.yml b/.github/workflows/api-breakage.yml index 4350342654..e19c004f64 100644 --- a/.github/workflows/api-breakage.yml +++ b/.github/workflows/api-breakage.yml @@ -63,10 +63,6 @@ jobs: except Exception: report = {} default_changes = report.get('field_default_changes', []) - default_changes_since_base = report.get( - 'field_default_changes_since_base', - default_changes, - ) print(f'## Python API breakage checks — {status}') print() @@ -76,30 +72,12 @@ jobs: print('> ⚠️ Breaking API changes or policy violations detected.') print() - if default_changes_since_base: + if default_changes: print('### Behavioral default changes detected') print() print( - 'These public `Field(default=...)` changes were introduced ' - 'by this changeset and were auto-marked with the ' - '`release-note-required` label:' - ) - print() - for change in default_changes_since_base: - print( - '- `{object_path}`: `{old_default}` → `{new_default}`'.format( - **change, - ) - ) - print() - elif default_changes: - print('### Behavioral default changes detected') - print() - print( - 'These public `Field(default=...)` changes differ from the ' - 'latest released baseline, but they were already present ' - 'before this changeset, so the workflow did not add the ' - '`release-note-required` label on this run:' + 'These public `Field(default=...)` changes were auto-marked ' + 'with the `release-note-required` label:' ) print() for change in default_changes: @@ -146,12 +124,11 @@ jobs: script: | const fs = require('fs'); - let defaultChangesSinceBase = []; + let defaultChanges = []; try { - const report = JSON.parse(fs.readFileSync(process.env.REPORT_PATH, 'utf8')); - defaultChangesSinceBase = report.field_default_changes_since_base || report.field_default_changes || []; + defaultChanges = JSON.parse(fs.readFileSync(process.env.REPORT_PATH, 'utf8')).field_default_changes || []; } catch (_error) { - defaultChangesSinceBase = []; + defaultChanges = []; } const { owner, repo } = context.repo; @@ -165,7 +142,7 @@ jobs: }); const hasLabel = currentLabels.some((item) => item.name === label); - if (defaultChangesSinceBase.length > 0 && !hasLabel) { + if (defaultChanges.length > 0 && !hasLabel) { await github.rest.issues.addLabels({ owner, repo, @@ -174,7 +151,7 @@ jobs: }); } - if (defaultChangesSinceBase.length === 0 && hasLabel) { + if (defaultChanges.length === 0 && hasLabel) { await github.rest.issues.removeLabel({ owner, repo, @@ -200,14 +177,10 @@ jobs: const status = exitCode === 0 ? '✅ **PASSED**' : '❌ **FAILED**'; let defaultChanges = []; - let defaultChangesSinceBase = []; try { - const report = JSON.parse(fs.readFileSync(process.env.REPORT_PATH, 'utf8')); - defaultChanges = report.field_default_changes || []; - defaultChangesSinceBase = report.field_default_changes_since_base || defaultChanges; + defaultChanges = JSON.parse(fs.readFileSync(process.env.REPORT_PATH, 'utf8')).field_default_changes || []; } catch (_error) { defaultChanges = []; - defaultChangesSinceBase = []; } let body = `${marker}\n## Python API breakage checks — ${status}\n\n**Result:** ${status}\n`; @@ -225,15 +198,9 @@ jobs: body += `\n
Log excerpt (first 1000 characters)\n\n\`\`\`text\n${excerpt}\n\`\`\`\n\n
\n`; } - if (defaultChangesSinceBase.length > 0) { - body += '\n### Behavioral default changes detected\n\n'; - body += 'These public `Field(default=...)` changes were introduced by this PR and were auto-marked with the `release-note-required` label:\n\n'; - for (const change of defaultChangesSinceBase) { - body += `- \`${change.object_path}\`: \`${change.old_default}\` → \`${change.new_default}\`\n`; - } - } else if (defaultChanges.length > 0) { + if (defaultChanges.length > 0) { body += '\n### Behavioral default changes detected\n\n'; - body += 'These public `Field(default=...)` changes differ from the latest released baseline, but they were already present on the base branch, so this PR was not auto-marked with the `release-note-required` label:\n\n'; + body += 'These public `Field(default=...)` changes were auto-marked with the `release-note-required` label:\n\n'; for (const change of defaultChanges) { body += `- \`${change.object_path}\`: \`${change.old_default}\` → \`${change.new_default}\`\n`; } diff --git a/AGENTS.md b/AGENTS.md index e0f0c8c4b2..a58e7742a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -167,8 +167,6 @@ consult each relevant package-level AGENTS.md. removing, or editing `description`, `title`, `examples`, `json_schema_extra`, and `deprecated` kwargs. - Public SDK `Field(default=...)` changes are treated separately from removals/structural API breakages: the API breakage workflow should surface them as behavioral compatibility changes, auto-apply the green `release-note-required` label on PRs, and the release workflow should prepend those labeled PRs to generated GitHub release notes. -- For public SDK `Field(default=...)` changes, keep two views in the API breakage workflow: compare against the latest released PyPI baseline for compatibility reporting, but compare against the PR base ref before syncing the `release-note-required` label or PR comment so unrelated follow-up PRs are not re-labeled for already-merged unreleased defaults. - - The SDK API breakage checker compares stringified `Field(...)` values by parsing them as Python expressions after escaping literal newlines inside diff --git a/tests/cross/test_check_sdk_api_breakage.py b/tests/cross/test_check_sdk_api_breakage.py index fa05967133..fc25b187cf 100644 --- a/tests/cross/test_check_sdk_api_breakage.py +++ b/tests/cross/test_check_sdk_api_breakage.py @@ -8,7 +8,6 @@ import importlib.util import json -import subprocess import sys from pathlib import Path from types import SimpleNamespace @@ -108,44 +107,6 @@ def test_get_pypi_baseline_version_falls_back_to_previous(monkeypatch): assert get_pypi_baseline_version("openhands-sdk", "1.2.0") == "1.1.0" -def _git(repo_root: Path, *args: str) -> str: - result = subprocess.run( - ["git", *args], - cwd=repo_root, - check=True, - capture_output=True, - text=True, - ) - return result.stdout - - -def _init_git_repo(tmp_path: Path) -> Path: - repo_root = tmp_path / "repo" - repo_root.mkdir() - _git(repo_root, "init", "-b", "main") - _git(repo_root, "config", "user.name", "Test User") - _git(repo_root, "config", "user.email", "test@example.com") - return repo_root - - -def _write_repo_sdk_model(repo_root: Path, default: str) -> None: - pkg = repo_root / "openhands-sdk" / "openhands" / "sdk" - pkg.mkdir(parents=True, exist_ok=True) - (pkg.parent / "__init__.py").write_text("") - (pkg / "__init__.py").write_text( - "__all__ = ['Config']\n" - "from pydantic import BaseModel, Field\n\n" - "class Config(BaseModel):\n" - f" model: str = Field(default={default!r})\n" - ) - - -def _commit_all(repo_root: Path, message: str) -> str: - _git(repo_root, "add", ".") - _git(repo_root, "commit", "-m", message) - return _git(repo_root, "rev-parse", "HEAD").strip() - - def test_griffe_breakage_removed_attribute_requires_minor_bump(tmp_path): old_pkg = _write_pkg_init(tmp_path, "old", ["TextContent"]) new_pkg = _write_pkg_init(tmp_path, "new", ["TextContent"]) @@ -1069,166 +1030,3 @@ def test_subclass_member_deprecated_on_base_is_not_undeprecated(tmp_path): # The removal should NOT be flagged as undeprecated because # Base.old_method carried a @deprecated marker assert undeprecated == 0 - - -def test_collect_field_default_changes_since_ref_reports_pr_introduced_change(tmp_path): - repo_root = _init_git_repo(tmp_path) - _write_repo_sdk_model(repo_root, "claude-sonnet-4-20250514") - base_ref = _commit_all(repo_root, "Base version") - - _write_repo_sdk_model(repo_root, "gpt-5.5") - _commit_all(repo_root, "Change default") - - changes = _prod._collect_field_default_changes_since_ref( - griffe, - str(repo_root), - base_ref, - _SDK_CFG, - ) - - assert changes == [ - FieldDefaultChange( - package="openhands.sdk", - object_path="openhands.sdk.Config.model", - old_default="'claude-sonnet-4-20250514'", - new_default="'gpt-5.5'", - ) - ] - - -def test_collect_field_default_changes_since_ref_ignores_preexisting_change(tmp_path): - repo_root = _init_git_repo(tmp_path) - _write_repo_sdk_model(repo_root, "claude-sonnet-4-20250514") - _commit_all(repo_root, "Base version") - - _write_repo_sdk_model(repo_root, "gpt-5.5") - _commit_all(repo_root, "Introduce default change on main") - - _git(repo_root, "checkout", "-b", "feature/unrelated") - (repo_root / "README.md").write_text("Unrelated change\n") - _commit_all(repo_root, "Unrelated change") - - changes = _prod._collect_field_default_changes_since_ref( - griffe, - str(repo_root), - "main", - _SDK_CFG, - ) - - assert changes == [] - - -def test_collect_field_default_changes_since_ref_returns_none_on_load_failure(tmp_path): - repo_root = _init_git_repo(tmp_path) - _write_repo_sdk_model(repo_root, "gpt-5.5") - _commit_all(repo_root, "Current version") - - changes = _prod._collect_field_default_changes_since_ref( - griffe, - str(repo_root), - "missing-ref", - _SDK_CFG, - ) - - assert changes is None - - -def test_collect_field_default_changes_since_ref_is_quiet_for_structural_changes( - tmp_path, capsys -): - repo_root = _init_git_repo(tmp_path) - pkg = repo_root / "openhands-sdk" / "openhands" / "sdk" - pkg.mkdir(parents=True, exist_ok=True) - (pkg.parent / "__init__.py").write_text("") - (pkg / "__init__.py").write_text( - "__all__ = ['Config']\n" - "from pydantic import BaseModel, Field\n\n" - "class Config(BaseModel):\n" - " model: str = Field(default='gpt-5.5')\n" - " enabled: bool = True\n" - ) - base_ref = _commit_all(repo_root, "Base version") - - (pkg / "__init__.py").write_text( - "__all__ = ['Config']\n" - "from pydantic import BaseModel, Field\n\n" - "class Config(BaseModel):\n" - " model: str = Field(default='gpt-5.5')\n" - ) - _commit_all(repo_root, "Remove non-default API") - - changes = _prod._collect_field_default_changes_since_ref( - griffe, - str(repo_root), - base_ref, - _SDK_CFG, - ) - - captured = capsys.readouterr() - assert changes == [] - assert "::error" not in captured.out - - -def test_write_field_default_change_report_includes_base_ref_changes( - tmp_path, monkeypatch -): - report_path = tmp_path / "report.json" - monkeypatch.setenv(_prod.FIELD_DEFAULT_CHANGE_REPORT_ENV, str(report_path)) - - changes = [ - FieldDefaultChange( - package="openhands.sdk", - object_path="openhands.sdk.Config.model", - old_default="'claude-sonnet-4-20250514'", - new_default="'gpt-5.5'", - ) - ] - - _prod._write_field_default_change_report( - changes, - field_default_changes_since_base=[], - ) - - assert json.loads(report_path.read_text()) == { - "field_default_changes": [ - { - "package": "openhands.sdk", - "object_path": "openhands.sdk.Config.model", - "old_default": "'claude-sonnet-4-20250514'", - "new_default": "'gpt-5.5'", - } - ], - "field_default_changes_since_base": [], - } - - -def test_write_field_default_change_report_omits_unavailable_base_ref( - tmp_path, monkeypatch -): - report_path = tmp_path / "report.json" - monkeypatch.setenv(_prod.FIELD_DEFAULT_CHANGE_REPORT_ENV, str(report_path)) - - changes = [ - FieldDefaultChange( - package="openhands.sdk", - object_path="openhands.sdk.Config.model", - old_default="'claude-sonnet-4-20250514'", - new_default="'gpt-5.5'", - ) - ] - - _prod._write_field_default_change_report( - changes, - field_default_changes_since_base=None, - ) - - assert json.loads(report_path.read_text()) == { - "field_default_changes": [ - { - "package": "openhands.sdk", - "object_path": "openhands.sdk.Config.model", - "old_default": "'claude-sonnet-4-20250514'", - "new_default": "'gpt-5.5'", - } - ], - }