[codex] Harden settings persistence validation#3529
Conversation
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Code ReviewSummaryThis PR hardens settings persistence validation by adding revalidation at the agent-server storage boundary and improving test coverage for edge cases involving malformed settings and partial PATCH failures. Changes Reviewed1. New Settings Schema Migration Check (
|
| Area | Risk Level | Notes |
|---|---|---|
| Data integrity | Low | Revalidation ensures malformed settings cannot be persisted |
| Performance | Low | Added validation at save boundary; may add slight latency for large settings |
| Backward compatibility | Low | New validation only affects writes; reads unaffected |
| Test coverage | Low | New tests cover critical failure scenarios |
Conclusion
✅ Approve - This is a solid security-hardening PR. The defense-in-depth approach at the persistence boundary is well-designed, and the test coverage appropriately covers the critical failure modes. The CI integration ensures schema migrations are enforced going forward.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
I exercised the settings API, the persistence store boundary, and the new settings schema migration CLI; the API and CLI behaviors work, but one malformed constructed-settings path still writes invalid settings to disk.
This review was created by an AI agent (OpenHands) on behalf of the user.
Does this PR achieve its stated goal?
Partially. The PR achieves the user-facing API atomicity goal: an invalid mixed PATCH /api/settings returned 422 and did not persist the valid-looking model/API-key parts. It also makes the exact dict-shaped malformed PersistedSettings.model_construct(...) case raise ValidationError and leave no file, and the new schema migration CLI runs successfully against origin/main. However, the broader stated storage-boundary goal is not fully met: a PersistedSettings containing a nested ConversationSettings.model_construct(max_iterations=-1) still saves successfully and writes the invalid value to settings.json on the PR.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully |
| CI Status | PR Description Check / Validate PR description is failing; several opt-in eval/example/integration workflows are skipped |
| Functional Verification |
Functional Verification
Test 1: Invalid mixed settings PATCH remains atomic
Step 1 — Establish baseline (without the fix):
Ran git checkout --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_settings_persistence.py and exercised PATCH /api/settings with a valid model update followed by an invalid mixed payload. Relevant output:
{
"api_atomicity": {
"seed_status": 200,
"invalid_status": 422,
"disk_contains_bad_model": false,
"disk_contains_bad_secret": false,
"observed_status": 200,
"observed_model": "stable-model",
"observed_max_iterations": 500
}
}This shows the invalid PATCH was rejected and the attempted should-not-persist fields were not written on the baseline branch.
Step 2 — Apply the PR's changes:
Checked out a1089dbae524b47b2aba17608d911ffc223dfeff.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_settings_persistence.py. Relevant output stayed correct:
{
"api_atomicity": {
"seed_status": 200,
"invalid_status": 422,
"disk_contains_bad_model": false,
"disk_contains_bad_secret": false,
"observed_status": 200,
"observed_model": "stable-model",
"observed_max_iterations": 500
}
}This confirms the settings API continues to reject invalid mixed PATCHes without partial persistence.
Test 2: Persistence boundary revalidation for constructed settings
Step 1 — Reproduce baseline weakness:
Using the same QA driver on origin/main, I called FileSettingsStore.save() with a PersistedSettings.model_construct(...) containing ConversationSettings.model_construct(max_iterations=-1). Relevant output:
{
"store_boundary": {
"outcome": "saved",
"error_type": null,
"settings_file_exists": true,
"settings_file_contains_negative_iterations": true
}
}This confirms malformed constructed settings can be serialized to disk: settings.json contained "max_iterations": -1.
Step 2 — Apply the PR's changes:
Checked out a1089dbae524b47b2aba17608d911ffc223dfeff.
Step 3 — Re-run with the fix in place:
Ran the same driver on the PR. Relevant output:
{
"store_boundary": {
"outcome": "saved",
"error_type": null,
"settings_file_exists": true,
"settings_file_contains_negative_iterations": true
}
}This shows the PR still serializes a malformed nested constructed settings object, so the storage-boundary hardening is incomplete.
Additional PR check:
The exact dict-shaped malformed case added by the PR does now fail safely:
{
"outcome": "raised",
"error_type": "ValidationError",
"settings_file_exists": false
}Test 3: Settings schema migration CLI
Step 1 — Prior state:
The migration checker is newly introduced by this PR, so there was no command to run on origin/main.
Step 2 — Apply the PR's changes:
Checked out a1089dbae524b47b2aba17608d911ffc223dfeff.
Step 3 — Run with real base-ref input:
Ran SETTINGS_SCHEMA_BASE_REF=origin/main OPENHANDS_SUPPRESS_BANNER=1 uv run python .github/scripts/check_settings_schema_migrations.py:
Settings schema migration check passed.
This verifies the new CLI can capture schemas from origin/main and the PR checkout successfully.
Issues Found
- 🟠 Issue: Persistence boundary validation still misses malformed nested Pydantic settings instances constructed with
model_construct(), allowing invalidmax_iterations=-1to be written tosettings.json.
|
Like the OpenHands-side companion (OpenHands/OpenHands#14673), this is good defensive hardening — and the schema-drift CI guard is genuinely valuable — but I don't think it closes the active That crash is in the enterprise SaaS read path (
Where this PR does help: |
QA on PR #3529 found that calling `PersistedSettings.model_validate(settings)` on an outer instance does NOT revalidate nested sub-models that were built via `model_construct()` — Pydantic v2 treats already-instantiated sub-model instances as opaque on that pass. As a result a malformed `ConversationSettings.model_construct(max_iterations=-1)` would slip through `FileSettingsStore.save()` and end up on disk with an invalid value. Add a second validation step that round-trips through `model_dump()` so the nested sub-models are revalidated against their field constraints. The dump uses `expose_secrets=plaintext` context so SecretStr fields are not redacted during the round-trip; the real encrypt-or-redact dump still happens below for the on-disk write. Add a regression test that constructs a nested `max_iterations=-1` and asserts `store.save()` raises `ValidationError` and never writes `settings.json`. Co-authored-by: openhands <openhands@all-hands.dev>
|
CI status check (from automated iteration):
— posted by an AI agent (OpenHands) on behalf of @neubig. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
neubig
added
review-this
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Harden settings persistence validation
Taste Rating: 🟡 Acceptable - Solid defensive work with minor improvement opportunities
Summary
This PR hardens the settings persistence layer against a subtle Pydantic v2 pitfall: model_construct() bypasses field validation, allowing malformed data to reach the store. The fix adds revalidation at the persistence boundary and a CI check to enforce migration coverage.
[CRITICAL ISSUES]
None. The core logic is sound.
[IMPROVEMENT OPPORTUNITIES]
🟠 Important: [.github/scripts/check_settings_schema_migrations.py, Line 32-36] Verbose inline comments: The multi-line comment block describing the SCHEMA_PROGRAM is inside a string literal, making it hard to read and maintain. Consider:
- Moving this logic into a separate module file, or
- Using a docstring at module level to explain the intent, and keeping the inline code self-explanatory
🟡 Suggestion: [openhands-agent-server/openhands/agent_server/persistence/store.py, Line 364-389] Comments restate the code: The block comment after the TypeError check explains what each step does in prose that directly mirrors the code. The comment says "Revalidate at the persistence boundary" but the code already calls it "revalidate". Consider:
- Keeping only the why (the Pydantic bypass scenario), not the what (step-by-step explanation)
- The comment about
model_construct()bypassing field validation for sub-models is valuable; the step-by-step breakdown adds noise
[TESTING GAPS]
None. The tests are high quality:
test_patch_settings_validation_error_does_not_persist_partial_updateexercises the real file system pathtest_file_settings_store_save_rejects_malformed_constructed_settingsandtest_file_settings_store_save_rejects_nested_model_construct_bypassspecifically target the Pydantic bypass vectors this PR mitigates
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The changes are additive validation layers at the persistence boundary with comprehensive test coverage. No breaking changes to existing APIs. The new CI check is non-blocking (warning) when it cannot read the base ref, which is appropriate for local development.
VERDICT:
✅ Worth merging: Core defensive logic is solid. The verbose comments are a minor style concern — linters should catch formatting issues, not reviewers.
KEY INSIGHT:
The two-step model_validate(instance) + model_validate(model_dump(...)) pattern is the correct approach for catching Pydantic v2 model_construct() bypasses in nested models — this is a well-known edge case that this PR addresses thoroughly.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
🟢 Good taste — This is a well-targeted security hardening PR that addresses a real Pydantic v2 edge case. The two-step revalidation strategy is the correct solution to the nested model_construct() bypass problem.
Changes Reviewed
openhands-agent-server/openhands/agent_server/persistence/store.py
The core change adds validation at the persistence boundary. The 11-line comment explaining the two-step revalidation strategy is justified — Pydantic v2's model_construct() semantics are genuinely surprising and the reasoning isn't obvious. Better to have the explanation than to have future maintainers simplify this into a single model_validate() call and reintroduce the bug.
openhands-agent-server/openhands/agent_server/persistence/models.py
Enabling validate_assignment=True and revalidate_instances="always" is a good belt-and-suspenders approach. This ensures that even assignments like settings.agent_settings = something go through validation.
.github/scripts/check_settings_schema_migrations.py
A solid CI gate that prevents schema versioning oversights. The implementation is defensive (fails open on archive errors, skips gracefully when base ref unavailable) which is appropriate for a non-blocking check.
tests/agent_server/test_settings_router.py
Three well-targeted tests cover the critical paths:
test_patch_settings_validation_error_does_not_persist_partial_update— verifies the rollback behaviortest_file_settings_store_save_rejects_malformed_constructed_settings— top-level model_construct bypasstest_file_settings_store_save_rejects_nested_model_construct_bypass— the nested case from the existing review comment
No issues found.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This PR hardens validation without changing external behavior. The new validate_assignment and revalidate_instances settings could theoretically break existing code that was relying on invalid intermediate states, but since this code path is internal to the agent server and only affects settings that are already supposed to be valid, the risk is minimal. The schema migration CI check is additive and non-breaking.
VERDICT:
✅ Worth merging: Solid security hardening with good test coverage. The existing review comment about nested model_construct bypass has been addressed in this PR (commit ec91f33).
KEY INSIGHT:
The two-step revalidation (model_validate → model_dump → model_validate) is the idiomatic Pydantic v2 pattern for catching model_construct() bypasses in nested models — there's no simpler single-call workaround.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
❌ QA Report: FAIL
The settings API still accepts and persists the PR's stated malformed ACP settings payload, so the primary bug is not fixed.
Does this PR achieve its stated goal?
No. The stated goal is to reject writes like agent_kind: "acp" with missing/null agent_context before they reach persistence and return 422 instead of writing inconsistent state. I started the real agent-server API and sent that payload over PATCH /api/settings; on the PR branch it returned HTTP:200 and wrote agent_context: null to settings.json, matching the bad baseline behavior.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully. |
| CI Status | 🟡 At observation time: 11 successful, 17 pending, 1 skipped; no failing checks reported yet. |
| Functional Verification | ❌ Real HTTP API verification shows the malformed ACP write is still persisted. |
Functional Verification
Test 1: Malformed ACP settings write should be rejected
Step 1 — Reproduce / establish baseline (without the fix):
Checked out origin/main, started the agent-server FastAPI app with an isolated OH_PERSISTENCE_DIR, then ran:
curl -sS -w "\nHTTP:%{http_code}\n" -X PATCH http://127.0.0.1:18129/api/settings -H 'Content-Type: application/json' --data '{"agent_settings_diff":{"agent_kind":"acp","agent_context":null}}'Observed excerpt:
"agent_kind":"acp" ... "agent_context":null
HTTP:200
Persisted file excerpt:
"agent_kind": "acp",
"agent_context": nullThis confirms the pre-PR behavior accepts and writes the malformed ACP settings shape.
Step 2 — Apply the PR's changes:
Checked out codex/harden-settings-persistence at 8e43aa2fda1bd4ae81a156a5029ee7cb112ca19b, restarted the API with a fresh isolated persistence directory.
Step 3 — Re-run with the fix in place:
Ran the same PATCH against the PR server:
curl -sS -w "\nHTTP:%{http_code}\n" -X PATCH http://127.0.0.1:18130/api/settings -H 'Content-Type: application/json' --data '{"agent_settings_diff":{"agent_kind":"acp","agent_context":null}}'Observed excerpt:
"agent_kind":"acp" ... "agent_context":null
HTTP:200
Persisted file excerpt:
"agent_kind": "acp",
"agent_context": nullThis shows the fix does not deliver the promised behavior for the reported payload: the server still returns 200 and persists the null agent_context instead of returning 422 and leaving storage clean.
Test 2: Omitted agent_context variant
On the PR branch, I also tried the omitted-field form:
curl -sS -w "\nHTTP:%{http_code}\n" -X PATCH http://127.0.0.1:18131/api/settings -H 'Content-Type: application/json' --data '{"agent_settings_diff":{"agent_kind":"acp"}}'Observed:
HTTP:200
agent_kind= acp
agent_context_present= True
agent_context= None
This also writes ACP settings with agent_context: null.
Test 3: Related invalid conversation setting remains rejected
On the PR branch, I verified a related validation path still works:
# First persist a valid model
PATCH {"agent_settings_diff":{"llm":{"model":"stable-model"}}} -> HTTP:200
# Then mix a valid model change with invalid conversation max_iterations
PATCH {"agent_settings_diff":{"llm":{"model":"should-not-persist","api_key":"sk-should-not-persist"}},"conversation_settings_diff":{"max_iterations":-1}} -> HTTP:422Follow-up GET/persisted-file checks showed:
"model": "stable-model"
"max_iterations": 500
(no should-not-persist / sk-should-not-persist in settings.json)
So the general invalid-conversation update path is guarded, but the specific ACP malformed-write goal remains unverified/failing.
Issues Found
- 🔴 Blocker:
PATCH /api/settingsstill returns 200 and persistsagent_kind: "acp"withagent_context: null, so the PR does not achieve the stated goal of rejecting that write before persistence.
This review was generated by an AI agent (OpenHands) on behalf of the user.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Harden settings persistence validation
Taste Rating
🟡 Acceptable — Solid security improvements, but has one breaking change that needs attention.
[CRITICAL ISSUES]
- [openhands-sdk/openhands/sdk/settings/model.py, Line 1848-1851] Breaking Change: Switching
agent_kindfromopenhandstoacpnow requiresagent_contextto be set. This is a breaking API change that will cause existing code paths to fail at runtime. Either: (1) Document this as a breaking change, (2) Provide a deprecation path, or (3) Makeagent_contextoptional with a sensible default.
[IMPROVEMENT OPPORTUNITIES]
- [.github/scripts/check_settings_schema_migrations.py, Line 72] Edge Case:
_candidate_refsusesdict.fromkeys()for deduplication. Considersorted(set(candidates))for clarity since order doesn't matter here.
[TESTING GAPS]
-
[tests/agent_server/test_settings_router.py, Line 1266-1292] Good:
test_patch_settings_validation_error_does_not_persist_partial_updateproperly verifies atomicity. ✅ -
[tests/agent_server/test_settings_router.py, Line 1294-1329] Good:
test_file_settings_store_save_rejects_malformed_constructed_settingscorrectly tests the new validation boundary. ✅
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Key factors:
- ✅ Security hardening is well-designed (two-step validation in
store.pyis clever) - ✅ Migration check script prevents future schema drift issues
- 🔴 Breaking change in
apply_agent_settings_diff()requiresagent_contextfor ACP switches - 🟡 New 230-line script adds maintenance surface area (acceptable trade-off)
VERDICT:
❌ Needs rework — The breaking change in apply_agent_settings_diff() must be addressed before merging.
KEY INSIGHT:
The two-step validation approach in FileSettingsStore.save() is the standout contribution — it's a clean pattern for catching model_construct() bypasses that should be documented or extracted into a utility.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Harden settings persistence validation
🟡 Acceptable — Solves a real problem with solid test coverage, but a few opportunities for simplification.
Summary
This PR hardens the settings persistence layer to prevent writing state that subsequently fails to load. The core issue: ACP settings could be written with agent_kind: acp and agent_context: null, which passes the loose write schema but fails strict validation on read.
Critical Issues
None found.
Improvement Opportunities
-
[
.github/scripts/check_settings_schema_migrations.py, Line 46] Complexity: The inline Python program inSCHEMA_PROGRAMis 16 lines of escaped code embedded in a string. Consider extracting this as a separate module inscripts/and importing it, or using a heredoc to avoid the\nnoise. -
[
openhands-agent-server/openhands/agent_server/persistence/store.py, Line 360-388] Verbose comments: The 20-line comment block explaining the two-step revalidation is thorough but could be tightened. The "Step 1 catches X, Step 2 catches Y" pattern would benefit from a single clear docstring on a private helper method instead of inline prose.
Testing Gaps
None — the test coverage is solid:
test_patch_settings_switch_to_acp_requires_agent_contextcovers the core validationtest_patch_settings_validation_error_does_not_persist_partial_updateverifies atomicitytest_file_settings_store_save_rejects_malformed_constructed_settingsandtest_file_settings_store_save_rejects_nested_model_construct_bypasstest the store boundary directly
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is a defensive change that adds validation at persistence boundaries. The two-step revalidation pattern in store.py is a belt-and-suspenders approach, but the comment explains exactly which Pydantic v2 edge cases each step catches. Tests cover the critical paths. The risk of breaking existing functionality is low since the changes only reject previously-allowed malformed writes.
VERDICT:
✅ Worth merging — Core logic is sound, comprehensive tests, solves a real incident. The comment verbosity is a minor style concern that doesn't block.
KEY INSIGHT:
The two-step model_validate pattern in store.py (first on the instance, then on its dumped form) is the right approach for Pydantic v2 — it catches both raw-dict bypasses and model_construct sub-model bypasses in sequence.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
neubig
added
review-this
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
neubig
added
review-this
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Harden settings persistence validation
Taste Rating
🟡 Acceptable - Works correctly but has some simplification opportunities.
Analysis
Real Problem Solved: ✅ Yes. The PR correctly addresses a production issue where ACP settings written with agent_kind: "acp" and agent_context: null could be persisted, which then fails Pydantic validation on subsequent reads, leaving users in a broken state.
Improvement Opportunities
[openhands-sdk/openhands/sdk/settings/model.py, ~Line 1854-1869] Complexity / Special Case: The ACP agent_context validation logic has redundant checks:
if new_kind == "acp" and "agent_context" in diff:
if diff["agent_context"] is None:
raise ValueError(...)
elif new_kind == "acp":
diff = dict(diff)
diff["agent_context"] = {}
if new_kind == "acp" and diff.get("agent_context") is None:
raise ValueError(...)The final check (diff.get("agent_context") is None) can never be true because:
- If
agent_contextwas in the original diff and wasNone, we already raised. - If
agent_contextwas not in the original diff, we just set it to{}.
This can be simplified to:
if new_kind == "acp":
if diff.get("agent_context") is None:
raise ValueError("agent_context cannot be null when switching agent_kind to acp")
diff = dict(diff)
diff.setdefault("agent_context", {})[openhands-agent-server/openhands/agent_server/persistence/store.py, ~Line 371-377] Complexity: The two-step validation pattern is clever but non-obvious. Consider extracting a helper method like def _revalidate_settings() with a comment explaining the two-pass approach rather than an inline block comment.
Testing Assessment
✅ Good coverage. Tests exercise real code paths:
test_patch_settings_switch_to_acp_requires_agent_context- verifies the new validationtest_patch_settings_switch_to_acp_defaults_missing_agent_context- verifies auto-populationtest_file_settings_store_save_rejects_malformed_constructed_settings- verifies store boundary protectiontest_file_settings_store_save_rejects_nested_model_construct_bypass- verifies nested bypass is caught
Breaking Change Assessment
Low risk:
- The change adds validation that rejects invalid writes (it won't break existing valid configurations)
- Existing valid ACP settings with proper
agent_contextwill continue to work - The change is defensive - it prevents writing bad state, doesn't change valid behavior
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This PR hardens the validation layer to prevent writing malformed settings that would fail on read. It adds defense-in-depth at multiple layers (SDK, persistence models, store) without changing valid behavior. The CI schema migration check ensures future changes can't accidentally introduce incompatible schema changes without migrations.
Verdict
✅ Worth merging: Core logic is sound, addresses a real production issue, and has good test coverage. The simplification suggestion is optional - the current code is correct, just slightly verbose.
Key Insight
The PR correctly solves the root cause by adding validation at the persistence boundary (store.py) and SDK layer (apply_agent_settings_diff), ensuring malformed state cannot be written regardless of how callers bypass intermediate validation.
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.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| @@ -1845,6 +1854,18 @@ def apply_agent_settings_diff( | |||
| return base_settings | |||
There was a problem hiding this comment.
🟡 Suggestion: The final if check here can never be true given the logic above - if agent_context was None, we already raised, and if it was absent, we just set it to {}. This can be simplified to a single check:
if new_kind == "acp":
if diff.get("agent_context") is None:
raise ValueError("agent_context cannot be null when switching agent_kind to acp")
diff = dict(diff)
diff.setdefault("agent_context", {})
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Harden settings persistence validation
Taste Rating
🟡 Acceptable — Solves a real problem but has code quality issues worth addressing.
Analysis
[IMPROVEMENT OPPORTUNITIES]
- [openhands-sdk/openhands/sdk/settings/model.py, Line 1854] Dead Code: The final
ifcheck at line 1865 (if new_kind == "acp" and diff.get("agent_context") is None) can never be true given the logic above it. Ifagent_contextwasNone, we already raised at line 1856. If it was absent, we just set it to{}at line 1861. This can be simplified to just:
if new_kind == "acp":
if "agent_context" not in diff:
diff = dict(diff)
diff["agent_context"] = {}[TESTING GAPS]
- [tests/sdk/test_apply_agent_settings_diff.py] Missing test coverage: The
_merge_patchfunction now has two modes (top-level null vs nested null), but there's no direct unit test for theremove_nullsparameter. The existing tests cover it indirectly throughapply_agent_settings_diff, but a direct test would catch regressions faster.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The PR adds important validation at the persistence boundary (defense-in-depth security), but the redundant validation check is minor technical debt. The breaking change behavior (raising onagent_context: null) is appropriate since it prevents silent data corruption.
Verdict
✅ Worth merging — The core security hardening is sound. The dead code is a minor cleanup that doesn't affect correctness.
Key Insight
The PersistedSettings.model_validate() + model_dump() + model_validate() two-step at the persistence boundary is a clever defense-in-depth pattern that catches Pydantic bypass techniques at the serialization point.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| elif new_kind == "acp": | ||
| diff = dict(diff) | ||
| diff["agent_context"] = {} | ||
| if new_kind == "acp" and diff.get("agent_context") is None: |
There was a problem hiding this comment.
🟡 Suggestion: This final if check can never be true given the logic above - if agent_context was None, we already raised at line 1856, and if it was absent, we just set it to {} at line 1861. Simplify to:
if new_kind == "acp":
if "agent_context" not in diff:
diff = dict(diff)
diff["agent_context"] = {}
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The PR achieves the core goal: a real local agent-server now rejects malformed ACP settings writes with agent_context: null and preserves the prior valid settings state.
Does this PR achieve its stated goal?
Yes. I reproduced the pre-PR loose-write behavior on origin/main: the settings API accepted agent_kind: "acp" with agent_context: null and returned/persisted that malformed state. On the PR branch, the same real HTTP PATCH returns 422, and a follow-up GET shows the previous openhands settings and stable-model value are still intact; a valid ACP switch without explicit context still succeeds and returns a non-null agent_context.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and started the local agent-server with uvicorn |
| CI Status | ⏳ At check time: 23 passing, 8 in progress, 16 skipped, no failures observed |
| Functional Verification | ✅ Exercised /api/settings with real HTTP PATCH/GET requests on base and PR branches |
Functional Verification
Test 1: Malformed ACP settings write is rejected before persistence
Step 1 — Reproduce / establish baseline without the fix:
Checked out origin/main at 16ad9e13624264757d04cbbbe3ce4b0e3f7c606f, started the agent-server with:
uv run python -m uvicorn openhands.agent_server.api:create_app --factory --host 127.0.0.1 --port 8766Then ran:
curl -i -X PATCH http://127.0.0.1:8766/api/settings \
-H 'Content-Type: application/json' \
--data '{"agent_settings_diff":{"llm":{"model":"stable-model"}}}'
curl -i -X PATCH http://127.0.0.1:8766/api/settings \
-H 'Content-Type: application/json' \
--data '{"agent_settings_diff":{"agent_kind":"acp","agent_context":null}}'
curl -i http://127.0.0.1:8766/api/settingsObserved excerpts:
HTTP/1.1 200 OK
... "model":"stable-model" ...
HTTP/1.1 200 OK
... "agent_kind":"acp" ... "agent_context":null ...
HTTP/1.1 200 OK
... "agent_kind":"acp" ... "agent_context":null ...This confirms the baseline server accepts the malformed ACP settings payload and leaves the bad agent_context: null state visible through the settings API.
Step 2 — Apply the PR's changes:
Checked out codex/harden-settings-persistence at 8ba9668bfc609feca57d8789b5c17efc533a0d9e, cleared the generated local settings file, and started the agent-server with:
uv run python -m uvicorn openhands.agent_server.api:create_app --factory --host 127.0.0.1 --port 8767Step 3 — Re-run with the fix in place:
Ran the same settings workflow:
curl -i -X PATCH http://127.0.0.1:8767/api/settings \
-H 'Content-Type: application/json' \
--data '{"agent_settings_diff":{"llm":{"model":"stable-model"}}}'
curl -i -X PATCH http://127.0.0.1:8767/api/settings \
-H 'Content-Type: application/json' \
--data '{"agent_settings_diff":{"agent_kind":"acp","agent_context":null}}'
curl http://127.0.0.1:8767/api/settings | python -m json.tool | grep -E '"agent_kind"|"model"|"agent_context"'Observed excerpts:
HTTP/1.1 200 OK
... "model":"stable-model" ...
HTTP/1.1 422 Unprocessable Content
{"detail":"Settings validation failed"}
"agent_kind": "openhands",
"model": "stable-model",
"agent_context": {This shows the malformed write is rejected and the prior valid settings remain intact, which is the core behavior promised by the PR.
Test 2: Valid ACP switch remains usable
With the PR server still running, I ran:
curl -i -X PATCH http://127.0.0.1:8767/api/settings \
-H 'Content-Type: application/json' \
--data '{"agent_settings_diff":{"agent_kind":"acp"}}'
curl http://127.0.0.1:8767/api/settings | python -m json.tool | grep -E '"agent_kind"|"agent_context"|"acp_server"'Observed excerpts:
HTTP/1.1 200 OK
... "agent_kind":"acp" ... "agent_context":{"skills":[], ...} ...
"agent_kind": "acp",
"acp_server": "claude-code",
"agent_context": {This confirms the fix does not block a normal ACP switch; the server supplies a valid non-null context when the field is omitted.
Unable to Verify
I did not reproduce the exact database-backed subsequent-read failure from the incident, because this local QA run used the repository's file-backed local agent-server rather than the paired SaaS/Agent Canvas database path. The local baseline still demonstrated the important bad write (agent_kind: "acp" with agent_context: null) that this SDK PR is intended to prevent.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
4 participants
HUMAN:
Investigating an incident where ACP settings written via the Agent Canvas left the database with
agent_kind: "acp"and noagent_context, which loads cleanly under the loose write path but fails Pydantic validation on subsequent reads, knocking users into a broken state. This PR makes the SDK reject those writes at validation time so the bad state cannot be persisted again.AGENT:
Why
ACP settings could be persisted in a state that subsequently fails to load:
The root cause is that the persistence layer accepts loose writes (e.g.
agent_kind: acpwithagent_context: null) that the strict load schema rejects on the next read, leaving the user in a stuck state. Operators have had to hand-fix rows inorg.agent_settingsandorg_member.agent_settings_diff.Summary
validate()pre-save check onAgentSettingsthat mirrors the strictload()schema so any payload accepted byvalidate()is guaranteed to round-trip throughload().persistence/store.py, settings router) to callvalidate()before commit and return a 422 instead of writing inconsistent state..github/scripts/check_settings_schema_migrations.py) used by theapi-breakageworkflow so regressions are caught in CI.Issue Number
Fixes #3541
Paired with OpenHands/OpenHands#14673
How to Test
Targeted test added in
tests/agent_server/test_settings_router.pysimulates the failing write (agent_kind=acp,agent_context=None) and asserts the request is rejected at validation before reaching the store. Before this change the row is written and a subsequent GET 500s; after the change the write returns 422 and the row stays consistent.Video/Screenshots
n/a — server-side validation change, no UI surface.
Type
Notes
This is the SDK half of the fix. The agent-side caller change is in OpenHands/OpenHands#14673.
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.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:8ba9668-pythonRun
All tags pushed for this build
About Multi-Architecture Support
8ba9668-python) is a multi-arch manifest supporting both amd64 and arm648ba9668-python-amd64) are also available if needed