fix(sdk): mark iterative refinement as critical setting#3115
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
LGTM! Clean change that properly marks iterative refinement as a critical setting for UI presentation. The test coverage ensures the prominence is correctly set.
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified that verification.enable_iterative_refinement is correctly marked as CRITICAL in the exported SDK schema, enabling frontend clients to render it in Basic settings.
Does this PR achieve its stated goal?
Yes. The PR successfully marks verification.enable_iterative_refinement as a CRITICAL SDK setting. Before this change, the field had SettingProminence.MINOR prominence. After this change, it has SettingProminence.CRITICAL prominence, which will enable frontend clients to render it in the Basic settings section alongside "Enable Critic" once the SDK is bumped in OpenHands.
The change is minimal, surgical, and exactly targets the stated goal of replacing the frontend-only workaround from OpenHands/OpenHands#14133.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build succeeded, dependencies installed |
| CI Status | ✅ All checks passing (pre-commit, tests, API checks, etc.) |
| Functional Verification | ✅ Schema exports CRITICAL prominence; tests pass; pre-commit clean |
Functional Verification
Test 1: Schema Prominence Change (Before/After)
Step 1 — Establish baseline (main branch without the fix):
Checked out origin/main and queried the exported schema:
uv run python -c "
from openhands.sdk.settings.model import AgentSettings
schema = AgentSettings.export_schema()
sections = {s.key: s for s in schema.sections}
v_fields = {f.key: f for f in sections['verification'].fields}
iterative_field = v_fields['verification.enable_iterative_refinement']
print(f'Prominence: {iterative_field.prominence}')
"Output:
Prominence: SettingProminence.MINOR
Interpretation: On the main branch, enable_iterative_refinement has MINOR prominence, meaning it would be hidden in Advanced settings in the frontend.
Step 2 — Apply the PR's changes:
Checked out the PR branch codex/iterative-refinement-critical.
Step 3 — Re-run with the fix in place:
Re-ran the same query:
uv run python -c "
from openhands.sdk.settings.model import AgentSettings, SettingProminence
schema = AgentSettings.export_schema()
sections = {s.key: s for s in schema.sections}
v_fields = {f.key: f for f in sections['verification'].fields}
iterative_field = v_fields['verification.enable_iterative_refinement']
print(f'Prominence: {iterative_field.prominence}')
print(f'Is CRITICAL: {iterative_field.prominence is SettingProminence.CRITICAL}')
"Output:
Prominence: SettingProminence.CRITICAL
Is CRITICAL: True
Interpretation: The prominence is now CRITICAL, confirming the fix works. Frontend clients consuming the schema will see this field as a Basic setting.
Test 2: Verify CRITICAL Settings Grouping
Checked which settings in the verification section are CRITICAL:
uv run python -c "
from openhands.sdk.settings.model import AgentSettings, SettingProminence
schema = AgentSettings.export_schema()
sections = {s.key: s for s in schema.sections}
for field in sections['verification'].fields:
if field.prominence is SettingProminence.CRITICAL:
print(f'{field.key}: {field.label}')
if field.depends_on:
print(f' Depends on: {field.depends_on}')
"Output:
verification.critic_enabled: Enable critic
verification.enable_iterative_refinement: Enable iterative refinement
Depends on: ['verification.critic_enabled']
Interpretation: The two CRITICAL settings in the verification section are now "Enable critic" and "Enable iterative refinement", with the latter correctly depending on the former. This matches the intended UX where both settings appear in Basic settings, with iterative refinement only enabled when critic is enabled.
Test 3: Unit Test Coverage
Ran the tests specified in the PR validation section:
uv run pytest tests/sdk/test_settings.py -k "agent_settings_export_schema or iterative_refinement" -vOutput:
collected 55 items / 52 deselected / 3 selected
tests/sdk/test_settings.py::test_llm_agent_settings_export_schema_groups_sections PASSED [ 33%]
tests/sdk/test_settings.py::test_acp_agent_settings_export_schema_has_acp_section PASSED [ 66%]
tests/sdk/test_settings.py::test_llm_create_agent_critic_with_iterative_refinement PASSED [100%]
3 passed, 52 deselected, 7 warnings in 0.05s
Interpretation: All tests pass, including the new assertion on lines 113-116 that explicitly verifies prominence is SettingProminence.CRITICAL.
Test 4: Code Quality (Pre-commit Hooks)
Ran pre-commit on the changed files:
uv run pre-commit run --files openhands-sdk/openhands/sdk/settings/model.py tests/sdk/test_settings.pyOutput:
Format YAML files....................................(no files to check)Skipped
Ruff format..............................................................Passed
Ruff lint................................................................Passed
PEP8 style check (pycodestyle)...........................................Passed
Type check with pyright..................................................Passed
Check import dependency rules............................................Passed
Check Tool subclass registration.........................................Passed
Interpretation: All pre-commit hooks pass. The code change is clean, properly formatted, and type-safe.
Issues Found
None.
Summary: This PR delivers exactly what it promises: a one-line metadata change that elevates enable_iterative_refinement from MINOR to CRITICAL prominence, enabling frontend clients to surface it in Basic settings alongside its dependency, "Enable critic". The change is tested, clean, and ready to merge.
2 participants
Summary
verification.enable_iterative_refinementas a critical SDK setting so clients render it in Basic settings next toEnable Criticonce its dependency is enabled.Context
This replaces the frontend-only workaround that was reverted from OpenHands/OpenHands#14133. The frontend can now pick up the intended Basic visibility directly from the SDK schema after the SDK is bumped in OpenHands.
Validation
make builduv run pytest tests/sdk/test_settings.py -k "agent_settings_export_schema or iterative_refinement"uv run pre-commit run --files openhands-sdk/openhands/sdk/settings/model.py tests/sdk/test_settings.pyAgent 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.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:6560d50-pythonRun
All tags pushed for this build
About Multi-Architecture Support
6560d50-python) is a multi-arch manifest supporting both amd64 and arm646560d50-python-amd64) are also available if needed