chore: forbid getattr and setattr in SDK - #4906
Conversation
Add a focused pre-commit checker for getattr and setattr calls in SDK Python files.\n\nCo-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
The forbidden-attributes hook introduced in this PR flagged 127 existing getattr/setattr calls across the SDK, blocking pre-commit. Add a committed baseline (keyed on file + call name + source-line hash) so the hook only reports *new* violations. This lets the enforcement ship now while the cleanup work (#4903, #4904, #4905) removes the legacy calls. Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟡 Acceptable — the AST check is straightforward, but the baseline identity loses occurrence counts and therefore does not actually prevent all new calls.
The Validate PR description check is also failing because the human-only HUMAN: field still contains only its placeholder. A human author needs to fill that field before merge; I have not edited it.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This only affects development enforcement, not SDK runtime behavior, but the central guardrail can silently accept newly added forbidden calls.
VERDICT:
❌ Needs rework: Preserve occurrence counts when comparing current calls with the baseline.
KEY INSIGHT:
A baseline for repeated syntax must be a multiset, not a membership set.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟡 Acceptable — the AST approach is straightforward, but the baseline comparison currently permits new forbidden calls in a common case.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is tooling-only and does not alter SDK runtime behavior, but the multiplicity bug weakens the central enforcement guarantee and can let newgetattr/setattrcalls merge unnoticed.
VERDICT:
❌ Needs rework: Preserve occurrence counts when comparing current calls with the baseline.
KEY INSIGHT:
A baseline for occurrences must retain multiplicity; set membership only proves that an identical line existed somewhere in the file.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
The baseline was loaded as a set, which deduplicates entries with the same (file, name, hash) key. This means adding another copy of an already-baselined call would not be detected. Switch to Counter (multiset) so occurrence counts are preserved: a new violation is any occurrence that exceeds the baseline count for its key. Addresses all-hands-bot review feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
When a baselined call is removed from the codebase, the baseline entry becomes stale. Previously this was only a note; now it causes a failure, requiring --update-baseline to permanently shrink the allowance rather than leaving a reusable slot. Addresses all-hands-bot review feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟡 Acceptable — the multiset fix catches duplicate additions while the baseline is current, but stale baseline credit can still be reused after cleanup.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is development tooling only, so it does not change SDK runtime behavior. However, the enforcement can silently permit a forbidden call to be reintroduced after an existing occurrence is removed.
VERDICT:
❌ Needs rework: Make stale baseline entries fail the check so cleanup cannot leave reusable allowance behind.
KEY INSIGHT:
A suppression baseline remains sound only if unused allowances cannot survive successful checks.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Covers multiset comparison (duplicate baselined lines are rejected) and stale baseline entries (removed calls shrink the allowance). Addresses all-hands-bot review feedback requesting regression tests.
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
Covers the exact sequence the reviewer described: baseline a getattr, replace with x.y (stale failure forces refresh), reintroduce the identical line → now correctly rejected as a new violation.
|
Updates pushed addressing all review feedback:
@all-hands-bot please re-review the latest commit. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟡 Acceptable — stale entries now fail for modified files, but deleting a baselined file bypasses that validation entirely.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is development tooling only, but a common file-deletion path can leave reusable baseline allowances and weaken the intended guardrail.
VERDICT:
❌ Needs rework: Ensure stale baseline validation also runs when SDK files are deleted.
KEY INSIGHT:
A baseline can only shrink reliably if validation runs against repository state, not only the existing files pre-commit passes to the hook.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Top-level tests/test_*.py files are rejected by the test directory allowlist CI check. Move the test file to tests/cross/ (where other check-script tests live) and fix the script path (parents[1] → parents[2]). Also fix the baseline isolation bug: BASELINE_FILE must be overridden AFTER exec_module, not before, because the module-level assignment would clobber the pre-set temp path and write --update-baseline output to the real file. Co-authored-by: openhands <openhands@all-hands.dev>
|
Thanks for the updates. I re-reviewed latest commit Deleted files leave reusable baseline allowances ( I reproduced the underlying behavior by baselining one file, deleting it, and calling Verification performed:
Verdict: ❌ Needs rework before approval. The current fixes close the duplicate-line and in-place removal cases, but deletion still leaves reusable baseline credit. Generated by OpenHands AI on behalf of the user. |
The pre-commit hook previously used pass_filenames: true with always_run: false, which meant deletions of baselined SDK files were never checked (pre-commit does not pass deleted files to hooks). Changes: - Set pass_filenames: false and always_run: true so the hook always validates the entire SDK - Add _discover_sdk_files() to auto-enumerate SDK Python files when no paths are passed - Update stale detection to flag baseline entries for files that no longer exist (deleted), not just files that were explicitly checked - Add test_deleted_baselined_file_is_stale regression test Addresses all-hands-bot review comment on .pre-commit-config.yaml. Co-authored-by: openhands <openhands@all-hands.dev>
|
All review feedback has been addressed in 2571ad2. All 4 review threads are resolved. Please re-review and approve. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
- Collapse multi-line expressions to single-line per ruff format - Fix _write_py return type: None -> Path - Add assert for importlib spec None-check (pyright) - Add type: ignore for dynamic module attribute assignment Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
🟡 Acceptable — the deletion and stale-baseline fixes work, but multiline calls are still identified too weakly to enforce the stated invariant.
Separately, the PR description's human-only HUMAN: field still contains only the placeholder. A human author needs to fill it before the description check can pass; I have not edited it.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is development tooling only, so it does not affect SDK runtime behavior. However, the guardrail can silently accept genuine edits to existing multilinegetattr/setattrcalls, weakening its central enforcement guarantee.
VERDICT:
❌ Needs rework: Include the complete call expression in baseline identity, not only its first physical line.
KEY INSIGHT:
An AST-based baseline should identify the AST call's full source span; a first-line hash cannot distinguish multiline calls whose arguments change.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
Re-requesting review from @all-hands-bot after fixing pre-commit formatting and pyright type errors in commit aadcb7b. All pre-commit hooks and tests pass locally. |
|
Thanks for the update. I re-reviewed current head
value = getattr(
old_object, "old_name"
)and then changing it to: value = getattr(
new_object, "new_name"
)The checker still returned Verification performed:
Verdict: ❌ Needs rework before approval; the previously reported blocker remains reproducible on the current head. Generated by OpenHands AI on behalf of the user. |
Address bot review feedback: baseline identity now uses the complete AST call source segment via ast.get_source_segment, not just the first physical line. This ensures argument changes on subsequent lines of multiline getattr/setattr calls are detected as new violations. Also adds test_multiline_arg_change_is_caught regression test and regenerates the baseline with the new hash algorithm. Co-authored-by: openhands <openhands@all-hands.dev>
|
Re-requesting review from @all-hands-bot for commit 0453fe5. All four previously flagged issues have been addressed:
All 10 unit tests pass, and the pre-commit hook itself passes. |
|
@all-hands-bot Please re-review commit 0453fe5. All four previously flagged issues have been addressed in commits since your last review (2571ad2). All CI checks pass except 'Validate PR description' which requires a human-written note in the HUMAN section. All review threads have been resolved. |
|
Requesting re-review from @all-hands-bot on commit 0453fe5. All previously raised review threads have been addressed and resolved. Please review the latest changes. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
LGTM. The full-call source hashing addresses the multiline bypass, and the regression coverage exercises it. I also verified the targeted test suite (10 passed), the checker itself, and pre-commit on the changed files.
The remaining failing check is Validate PR description (the human-only PR metadata gate), not a code failure.
Generated by OpenHands AI on behalf of the user.
|
Re-review complete on Validation performed:
The remaining failing check is Generated by OpenHands AI on behalf of the user. |
|
The only failing check is Validate PR description, which requires at least 20 visible characters in the Summary of what was done:
The sole remaining blocker is the empty |
3 participants
HUMAN:
AGENT:
This PR was created by an AI agent (OpenHands) on behalf of the user.
Why
Prevent new dynamic
getattrandsetattrcalls from entering the SDK while the existing migration work is tracked separately.Summary
getattrandsetattrcalls.openhands-sdk/.Issue Number
Closes #4902
How to Test
uv run python scripts/check_forbidden_dynamic_attributes.py /tmp/sample.pyreports both forbidden calls and allows direct attribute access.uv run pre-commit run --files .pre-commit-config.yaml scripts/check_forbidden_dynamic_attributes.pyruns the configured hooks; the checker itself passes, while the repository's existing SDK calls are intentionally tracked by the child issues.uv run ruff check scripts/check_forbidden_dynamic_attributes.pyuv run pycodestyle --max-line-length=88 --ignore=E203,E501,W503,E704 scripts/check_forbidden_dynamic_attributes.pyVideo/Screenshots
Not applicable for this lint-only change.
Design Doc
Not applicable.
Type
Notes
The checker is intentionally introduced before cleanup so the remaining dynamic access is visible and can be removed in coherent follow-up PRs.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/python-nodejs:python3.13-nodejs22-slimgolang: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:0453fe5-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0453fe5-python) is a multi-arch manifest supporting both amd64 and arm640453fe5-python-amd64) are also available if needed