Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
…Docker run - Switch to astral-sh/setup-uv and uv sync like other workflows - Ensure local agent-server deps are installed before OpenAPI generation - Add .github/oasdiff.yaml for formatting/fail-on control and future ignores - Pass config via working dir mount and set -w /work Co-authored-by: openhands <openhands@all-hands.dev>
…gets - Allow dotted paths relative to openhands.sdk - Keep warnings for unresolved paths Co-authored-by: openhands <openhands@all-hands.dev>
…r for OpenAPI generation - Align with repo standard of using uv over pip/pipx - Pre-commit formatting applied Co-authored-by: openhands <openhands@all-hands.dev>
- Update SemVer gate in REST breakage script - Fix long line formatting Co-authored-by: openhands <openhands@all-hands.dev>
|
OpenHands-GPT-5 here — suggestion on scoping SDK API breakage checks. To avoid over-constraining internals and to align with a curated public surface, please scope the SDK API check to the symbols exported from the public module(s), rather than scanning deep subpackages: Recommended options:
Context:
Happy to help wire the all-based resolution in the check_sdk_api_breakage.py script if useful. |
|
@OpenHands split this PR in two:
|
|
I'm on it! enyst can track my progress at all-hands.dev |
… and scripts Co-authored-by: openhands <openhands@all-hands.dev>
|
Summary of work completed Goal
Actions taken
Checklist
Notes
|
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
@OpenHands Understand this PR and do two things:
|
|
I'm on it! enyst can track my progress at all-hands.dev |
|
Summary of work You asked for two things: a roasted code review of PR #1098 (SDK API breakage checks) and a deep comparison vs the repo’s deprecation mechanism, with pros/cons of adding this on top.
Overall
Critical issues (must fix)
Improvement opportunities (should fix)
Style notes (minor)
Verdict
Key insight
What’s in the codebase (openhands-sdk/openhands/sdk/utils/deprecation.py):
What this PR adds:
Advantages of adding this on top (assuming fixes above):
Caveats:
Bottom line
|
|
@OpenHands Read all comments here. We focus on the last comment, the review roasted feedback. I agree with top level exports. FIx all issues in the feedback, except for the Major / minor version issue. Sorry, enforce MINOR for breaks. Fix all the rest. |
|
I'm on it! enyst can track my progress at all-hands.dev |
…ll__; treat removal from exports as breaking; use packaging.version with fallback; harden PyPI fetch and griffe.load_pypi usage Co-authored-by: openhands <openhands@all-hands.dev>
|
Final summary of changes (pushed to ci/api-breakage) What I changed (addressing the roasted review feedback, keeping MINOR-for-breaks as requested):
Files changed
Quality checks
Commit
Checklist against your request
No further changes since the previous summary; the above reflects the final state now pushed to ci/api-breakage. |
Coverage Report •
|
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
1 similar comment
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
Hi — OpenHands agent here. I went through the remaining unsolved inline review threads and addressed them. What I changed (commit 70859ff):
I replied in each thread with details and marked them resolved where the change fully addressed the feedback. |
Final summary (PR #1098 / branch
|
…by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solves a real problem but core logic lacks test coverage
The approach is sound (using Griffe for API breakage detection), but the functions that will actually gate PRs have zero test coverage. Previous reviews addressed structural issues well, but the most critical gap remains.
Critical Issues
🔴 Untested Core Logic: _extract_exported_names() and _compute_breakages() are the heart of breakage detection (73 lines combined), yet have ZERO test coverage. These functions will gate PRs through CI.
Why this matters:
- False positives = blocked PRs, frustrated developers
- False negatives = missed breakage, defeats the entire purpose
What needs testing:
_extract_exported_names: Mock griffe module with__all__, verify extraction works_compute_breakages: Test removed exports detected, signature changes caught, no false positives on unchanged APIs
The version bump tests are good, but they test the easy part. Test what actually matters.
Improvement Opportunities
🟠 Over-Engineering in _extract_exported_names (lines 117-146): Per resolved thread, "our SDK always has __all__ defined". Yet this function has 30 lines handling edge cases that never occur.
The problem: Lines 138-142 check for both string elements AND elements with a .value attribute. If __all__ is always a list of strings, this is solving an imaginary problem.
Simpler approach:
def _extract_exported_names(module) -> set[str]:
try:
all_var = module["__all__"]
except Exception as e:
raise ValueError("__all__ must be defined") from e
names = {name.strip("\"'") for name in all_var.value.elements}
if not names:
raise ValueError("__all__ is empty")
return namesIf the structure varies, let it fail loudly.
🟡 Fragile Fallback in _resolve_griffe_object (lines 183-201): Three resolution strategies with broad except Exception that swallows errors. If griffe changes its API, this fails silently. Add logging so failures are debuggable, or remove unused fallback paths.
Verdict
✅ Worth merging after adding tests: The architecture is sound and solves a real problem (catching accidental API breakage). But deploying 312 lines of CI infrastructure that gates PRs without testing the core logic is risky. Add basic unit tests for _extract_exported_names and _compute_breakages to prove they work correctly.
Key Insight: You tested the trivial part (version comparison) but not the critical part (API comparison). Flip that priority.
…: openhands <openhands@all-hands.dev>
|
Replying to review (#1098 (review)):
|
…nds <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
enyst
force-pushed
the
ci/api-breakage
branch
from
a96f4b8 to
926002c
Compare
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🔴 Needs improvement - CI gate with insufficient test coverage will block PRs on false positives
The core problem: This script will gate other PRs, but the main feature (tracking __all__ exports) has zero test coverage. The only integration test exercises a different code path entirely.
[CRITICAL ISSUES]
See inline comments for details. The __all__ extraction logic (35 lines, the main selling point of this PR) is completely untested.
VERDICT:
❌ Needs rework: Add tests for the __all__ extraction code path before merging
KEY INSIGHT:
The only integration test exercises the "additional paths" fallback, not the main openhands.sdk.__all__ extraction that the PR description emphasizes.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Pragmatic solution for catching API breakages.
On the unresolved thread: The _compute_breakages() function (line 244) does one conceptual task with clear structure: (1) compare top-level exports, (2) compare include paths. At 43 lines with max 2 nesting levels, it doesn't violate complexity principles. Has test coverage. No need to split.
What's good:
✅ Tests use real Griffe (not mocks)
✅ Pragmatic error handling (warns on failures, doesn't block CI)
✅ Focuses on __all__ (curated public API)
✅ Runs only on release PRs
LGTM! Ready to merge.
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
enyst
changed the title
With PEP 517, setuptools.build_meta handles wheel creation natively. The wheel package is not needed as an explicit build requirement. Co-authored-by: openhands <openhands@all-hands.dev>
This reverts commit 19b33e2.
2 participants
Summary for Reviewers
This PR adds automated SDK API breakage detection using Griffe. It runs on PRs to
mainandrelease/*branches.What it does
openhands-sdkagainst the previous PyPI releaseopenhands.sdk(governed by__all__)How it complements the deprecation mechanism
check_deprecations.py)@deprecated)The deprecation mechanism handles intentional deprecations with graceful warnings. This check catches accidental breaks that weren't planned—like accidentally removing a symbol from
__all__or changing a function signature.Files added
.github/scripts/check_sdk_api_breakage.py- The detection script.github/workflows/api-breakage.yml- GitHub Actions workflowConfiguration
SDK_INCLUDE_PATHSenv var can scope checks to specific modules (default:openhands.sdk)This PR adds API breakage checks to run on release PRs.
What's included
SDK programmatic API check using Griffe
openhands.sdk; override withSDK_INCLUDE_PATHSREST OpenAPI diff using oasdiff(moved to separate PR)Workflow
Notes
Co-authored-by: openhands openhands@all-hands.dev
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:b1fddb4-pythonRun
All tags pushed for this build
About Multi-Architecture Support
b1fddb4-python) is a multi-arch manifest supporting both amd64 and arm64b1fddb4-python-amd64) are also available if needed