settings: generate CLI settings from SDK schema#580
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solves a real problem (settings duplication) with a pragmatic schema-driven approach. Core logic is sound, but there are data ownership concerns and some unnecessary complexity around API key preservation.
Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
05e7cda to
acb7530
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solves a real problem (settings duplication) with a pragmatic schema-driven approach. Core logic is sound, but there are testing gaps and data ownership concerns that should be addressed before merging.
Key Observations:
- ✅ Previous review feedback addressed well (type safety, DRY violations, import patterns)
⚠️ Dual-persistence model adds complexity (manual sync between AgentStore and CliSettings)⚠️ Minimal testing for a significant architectural change- ✅ Schema-driven approach eliminates duplication elegantly
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
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solves a real problem (settings duplication) with clean schema-driven architecture. Previous review feedback was addressed thoroughly. Core design is sound, manual glue code between CLI and SDK settings is pragmatic.
Assessment:
✅ Data Structures: Excellent choice - schema export as single source of truth eliminates duplication and special cases
✅ Real Problem: Yes - removes settings definitions duplicated between CLI and SDK
✅ Testing: Tests verify real behavior (save/load cycles, value parsing, nested updates) rather than just mocking calls
✅ Evidence: PR description includes actual CLI runs showing schema-driven commands working in the real TUI
✅ Simplicity: Core idea is clean. The manual CliSettings ↔ AgentSettings mapping (programmatic_settings.py:97-122) is unavoidable glue code - it's pragmatic, not elegant, but necessary.
✅ Previous Reviews Addressed: Type safety improved with SettingsFieldSchema, DRY violation fixed by extracting format_setting_argument_hint(), implicit API key merge removed
Minor Notes:
See inline comments for two low-priority observations on error messages and schema validation assumptions.
VERDICT:
✅ Worth merging - Fundamentally sound architecture, tests verify real behavior, known trade-offs are documented and acceptable
KEY INSIGHT:
Schema-driven settings eliminate an entire class of synchronization bugs by making the SDK schema the single source of truth.
| field_key: str, | ||
| field: SettingsFieldSchema, | ||
| argument: str, | ||
| ) -> Any: |
There was a problem hiding this comment.
🟢 Acceptable - Error Handling: The boolean parsing accepts many variants (on, true, enabled, etc.), which is good UX. The error message could theoretically expose all valid values to an attacker probing the system, but this is a CLI tool where users control input, not a web API. Pragmatic choice.
| def compose(self) -> ComposeResult: | ||
| """Compose the CLI settings tab content.""" | ||
| cli_fields = [ | ||
| field |
There was a problem hiding this comment.
🟢 Acceptable - Schema Assumption: This assumes export_schema() returns well-formed sections with a cli key. If the SDK schema is malformed, this will fail at runtime. That's acceptable (fail-fast), but worth noting that there's no defensive validation here. Consider adding a schema validation test if SDK schema changes are frequent.
Summary
AgentSettings.export_schema()pyproject.tomldirectly at thesoftware-agent-sdkissue branch so the PR can consume the new schema before a release is cutDependencies
openhands-sdk,openhands-tools, andopenhands-workspacefromOpenHands/software-agent-sdk@openhands/issue-2228-sdk-settings-schemaTesting
uv run pytest tests/shared/test_settings_commands.py tests/stores/test_programmatic_settings.py tests/acp/test_slash_commands.py -qmake lintmake testmake test-binaryEvidence
I replaced the earlier helper-only evidence with real CLI runs on this PR branch.
1) Actual Textual CLI run exercised a schema-derived settings command
I ran the real TTY entrypoint (
uv run openhands --exit-without-confirmation) on this branch and sent a schema-generated slash command inside the live TUI. Relevant transcript from the running CLI:That is the real CLI/TUI, not a direct Python helper call, and it shows a schema-derived slash command being recognized and applied in the running app.
2) Actual CLI command created a live OpenHands Cloud conversation
The local sandbox
LLM_MODEL/LLM_API_KEYenv pair was not directly usable for a local BYO run here (openhands/...provider mismatch, then auth failure after forcing anopenai/prefix), so I used the authenticated cloud entrypoint to get a genuine live run from this PR branch:Conversation link: https://app.all-hands.dev/conversations/6d40b6a27ba2497194d532efa0a37925
Fixes
Fixes OpenHands/software-agent-sdk#2228
Checklist
🚀 Try this PR