Skip to content

Expose active LLM profile in settings API#3552

Open
enyst wants to merge 6 commits into
mainfrom
feat/settings-active-profile
Open

Expose active LLM profile in settings API#3552
enyst wants to merge 6 commits into
mainfrom
feat/settings-active-profile

Conversation

@enyst

@enyst enyst commented Jun 6, 2026

Copy link
Copy Markdown
Member

HUMAN:
Small PR to start the refactoring of LLM Profiles to be the only, or almost the only, way to use LLMs.

  • A human has tested these changes.

AGENT:


Why

This is the first small step toward making LLM profiles the persistent/user-facing LLM source of truth. The settings API already persists active_profile, but clients could not read or update it through the shared settings API contract.

Summary

  • Adds active_profile to the shared settings API response and update request models.
  • Returns the persisted active LLM profile from GET /api/settings and PATCH /api/settings.
  • Allows PATCH /api/settings to set or clear active_profile without changing raw agent_settings.llm behavior.

Issue Number

Related to #3511.

How to Test

I validated the focused settings API behavior and changed-file checks locally:

uv run pytest tests/agent_server/test_settings_router.py -q
# 55 passed

uv run pre-commit run --files \
  openhands-sdk/openhands/sdk/settings/api_models.py \
  openhands-agent-server/openhands/agent_server/settings_router.py \
  tests/agent_server/test_settings_router.py
# all hooks passed

The PR CI also passed the broader agent-server, SDK, workspace, cross, stress, OpenAPI, and pre-commit checks before the PR-description-only failure.

Video/Screenshots

Not applicable; this is a settings API/model change with automated test coverage.

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

This PR is intentionally additive and minimal. It does not remove or change raw agent_settings.llm behavior; follow-up PRs can use this field to move profile activation and conversation start toward profile references.

This PR description AGENT section was updated by an AI agent (OpenHands) on behalf of the user.

@enyst can click here to continue refining the PR


Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.13-nodejs22-slim Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:6fff19a-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-6fff19a-python \
  ghcr.io/openhands/agent-server:6fff19a-python

All tags pushed for this build

ghcr.io/openhands/agent-server:6fff19a-golang-amd64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-golang-amd64
ghcr.io/openhands/agent-server:feat-settings-active-profile-golang-amd64
ghcr.io/openhands/agent-server:6fff19a-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:6fff19a-golang-arm64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-golang-arm64
ghcr.io/openhands/agent-server:feat-settings-active-profile-golang-arm64
ghcr.io/openhands/agent-server:6fff19a-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:6fff19a-java-amd64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-java-amd64
ghcr.io/openhands/agent-server:feat-settings-active-profile-java-amd64
ghcr.io/openhands/agent-server:6fff19a-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:6fff19a-java-arm64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-java-arm64
ghcr.io/openhands/agent-server:feat-settings-active-profile-java-arm64
ghcr.io/openhands/agent-server:6fff19a-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:6fff19a-python-amd64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-python-amd64
ghcr.io/openhands/agent-server:feat-settings-active-profile-python-amd64
ghcr.io/openhands/agent-server:6fff19a-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-amd64
ghcr.io/openhands/agent-server:6fff19a-python-arm64
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-python-arm64
ghcr.io/openhands/agent-server:feat-settings-active-profile-python-arm64
ghcr.io/openhands/agent-server:6fff19a-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-arm64
ghcr.io/openhands/agent-server:6fff19a-golang
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-golang
ghcr.io/openhands/agent-server:feat-settings-active-profile-golang
ghcr.io/openhands/agent-server:6fff19a-golang_tag_1.21-bookworm
ghcr.io/openhands/agent-server:6fff19a-java
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-java
ghcr.io/openhands/agent-server:feat-settings-active-profile-java
ghcr.io/openhands/agent-server:6fff19a-eclipse-temurin_tag_17-jdk
ghcr.io/openhands/agent-server:6fff19a-python
ghcr.io/openhands/agent-server:6fff19a3a6ecb2eca8a95a300af0ac4851589fab-python
ghcr.io/openhands/agent-server:feat-settings-active-profile-python
ghcr.io/openhands/agent-server:6fff19a-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim

About Multi-Architecture Support

  • Each variant tag (e.g., 6fff19a-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 6fff19a-python-amd64) are also available if needed

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Python API breakage checks — ✅ PASSED

Result:PASSED

Action log

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

REST API breakage checks (OpenAPI) — ✅ PASSED

Result:PASSED

Action log

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-agent-server/openhands/agent_server
   profiles_router.py150696%336–341
   settings_router.py123893%261, 263–264, 364, 366–367, 405, 410
openhands-agent-server/openhands/agent_server/persistence
   models.py2042687%297, 302, 340, 388, 423–429, 431, 433, 436, 440, 466, 483–484, 529, 533, 535, 564–567, 570
openhands-sdk/openhands/sdk/settings
   api_models.py30293%91, 93
TOTAL30379673077% 

@enyst enyst marked this pull request as ready for review June 6, 2026 14:10

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ QA Report: PASS

The settings API change works end-to-end: active_profile is now returned, can be set, persists across refetch, can be cleared, and does not alter raw LLM settings.

Does this PR achieve its stated goal?

Yes. Against the base branch, the live agent server omitted active_profile from GET /api/settings and rejected PATCH /api/settings requests containing only active_profile with HTTP 400. Against PR commit 4f6ca8e, the same live API returned active_profile, accepted setting it to fast-profile, persisted that value on a subsequent GET, accepted clearing it with null, and kept the agent_settings.llm hash unchanged throughout.

Phase Result
Environment Setup make build completed; uv run agent-server launched successfully
CI Status ⚠️ Most checks are green; Validate PR description is failing and qa-changes was still in progress at review time
Functional Verification ✅ Live HTTP requests verified /api/settings behavior and /openapi.json schema exposure
Functional Verification

Test 1: /api/settings exposes, sets, persists, and clears active_profile

Step 1 — Reproduce / establish baseline without the fix:
Ran bash /tmp/qa_settings_api.sh, which checked out origin/main, started the actual agent server on 127.0.0.1:8123, and made real HTTP requests:

===== base branch 0216cae5 =====
server_ready pid=3767
GET /api/settings -> HTTP 200
has_active_profile= False active_profile= <missing> llm_hash= 33016d138703
PATCH active_profile=fast-profile -> HTTP 400
{"detail": "At least one of agent_settings_diff, conversation_settings_diff, or app_preferences_diff must be provided"}
GET after set -> HTTP 200
has_active_profile= False active_profile= <missing> llm_hash= 33016d138703
PATCH active_profile=null -> HTTP 400
{"detail": "At least one of agent_settings_diff, conversation_settings_diff, or app_preferences_diff must be provided"}
GET after clear -> HTTP 200
has_active_profile= False active_profile= <missing> llm_hash= 33016d138703
server_stopped pid=3767

This establishes the prior behavior: active_profile was not part of the settings response, and clients could not set or clear it as a standalone settings update.

Step 2 — Apply the PR's changes:
The same script checked out PR commit 4f6ca8ecad54ed6958569c04a720d39089b7a822 and started a fresh agent server on 127.0.0.1:8124.

Step 3 — Re-run with the fix in place:
The script re-ran the same HTTP flow against the PR server:

===== pr branch 4f6ca8ec =====
server_ready pid=3855
GET /api/settings -> HTTP 200
has_active_profile= True active_profile= None llm_hash= 33016d138703
PATCH active_profile=fast-profile -> HTTP 200
has_active_profile= True active_profile= fast-profile llm_hash= 33016d138703
GET after set -> HTTP 200
has_active_profile= True active_profile= fast-profile llm_hash= 33016d138703
PATCH active_profile=null -> HTTP 200
has_active_profile= True active_profile= None llm_hash= 33016d138703
GET after clear -> HTTP 200
has_active_profile= True active_profile= None llm_hash= 33016d138703
server_stopped pid=3855

This confirms the PR behavior works end-to-end: the field is returned, standalone set and clear operations succeed, persistence is visible via follow-up GETs, and agent_settings.llm remains unchanged (llm_hash=33016d138703 before and after).

Test 2: OpenAPI schema exposes active_profile to API clients

Ran the PR server and fetched the generated schema from /openapi.json:

SettingsResponse: has_active_profile=True schema={'anyOf': [{'type': 'string'}, {'type': 'null'}], 'title': 'Active Profile', 'description': 'Name of the currently active LLM profile, if one is selected.'}
SettingsUpdateRequest: has_active_profile=True schema={'anyOf': [{'type': 'string'}, {'type': 'null'}], 'title': 'Active Profile', 'description': 'Name of the active LLM profile to persist; null clears it.'}

This confirms generated API clients can discover both the response and update-request fields.

Issues Found

None found in functional QA. CI note: the Validate PR description check is currently failing, and the qa-changes check was still running when this review was posted.

This review was created by an AI agent (OpenHands) on behalf of the user.

@enyst enyst added the review-this This label triggers a PR review by OpenHands label Jun 6, 2026 — with OpenHands AI

all-hands-bot commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

🟢 Good taste - Clean, minimal implementation that solves a real problem.

Overview

This PR exposes the active_profile field (already persisted in PersistedSettings) through the settings API endpoints. The implementation is straightforward and follows existing patterns.

Analysis

Taste Rating: 🟢 Good

The changes are focused and minimal:

  • Added active_profile to SettingsResponse and SettingsUpdateRequest
  • Plumbed the field through get_settings() and update_settings()
  • Added tests covering both set and clear operations

Key Observation

The update logic handles the None case correctly:

update_data = payload.model_dump(exclude_none=True)
if "active_profile" in payload.model_fields_set:
    update_data["active_profile"] = payload.active_profile

Using model_fields_set to detect explicit field presence is the right approach — it allows distinguishing between "not provided" (excluded by exclude_none) and "explicitly set to None" (clearing the profile). This is a subtle but important distinction.

Minor Suggestion

The error message in update_settings() now reads:

At least one of agent_settings_diff, conversation_settings_diff, app_preferences_diff, or active_profile must be provided

This is accurate but slightly inconsistent with the docstring which describes diff fields as the primary update mechanism. The active_profile is a top-level field, not a diff. Consider either:

  • "at least one of the update fields must be provided"
  • Or explicitly list all allowed fields

Not blocking — just a minor style nit.

Tests

The test test_patch_settings_updates_active_profile covers the happy path well:

  • Setting a profile
  • Verifying persistence (refetch)
  • Clearing the profile
  • Verifying cleared state (refetch)

This is sufficient coverage for the feature.


[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

The changes are additive only (new optional field), don't modify existing behavior, and are well-tested. No breaking changes or security implications.


VERDICT:
Worth merging - Clean implementation with appropriate test coverage.

KEY INSIGHT:
The model_fields_set check is the correct way to handle partial updates with nullable fields — it properly distinguishes between "not provided" and "explicitly nulled."


This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

Co-authored-by: openhands <openhands@all-hands.dev>
@enyst enyst removed the review-this This label triggers a PR review by OpenHands label Jun 6, 2026
@enyst enyst added the review-this This label triggers a PR review by OpenHands label Jun 6, 2026 — with OpenHands AI

all-hands-bot commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (Supplementary)

🟢 Good taste - Clean implementation that follows existing patterns.

Overview

This PR exposes the active_profile field in the settings API. The existing reviews have covered the core implementation and QA, so this review focuses on edge cases and validation gaps.

Observations

1. Validation Gap in Settings API

The SettingsUpdateRequest.active_profile accepts any string, but the profiles_router validates profile names against PROFILE_NAME_PATTERN (^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$). This means:

  • ✅ A valid profile name can be set via /api/settings
  • ⚠️ An invalid profile name (e.g., with spaces or special characters) can also be stored through /api/settings

This inconsistency isn't blocking since the profiles API will 404 when trying to use an invalid name, but it could confuse clients who assume validation consistency. Consider adding the same regex validation to SettingsUpdateRequest.active_profile:

active_profile: str | None = Field(
    default=None,
    pattern=r"^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$",
    description="Name of the active LLM profile...",
)

2. Profile Deletion Edge Case

The rename_profile endpoint correctly updates active_profile when a profile is renamed. However, if a profile is deleted while it is the active profile, active_profile will point to a non-existent profile.

This isn't introduced by this PR (it's pre-existing behavior), but worth noting for future consideration.

What's Working Well

  • The model_fields_set check correctly distinguishes between "not provided" and "explicitly null"
  • The implementation is minimal and focused
  • Test coverage for the happy path is comprehensive
  • The error message simplification in update_settings is an improvement

Minor Style Note

The updated error message "At least one settings update field must be provided" is cleaner than the previous verbose version. 👍


[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Changes are additive, non-breaking, and well-tested. The validation gap is a minor inconsistency rather than a security or correctness issue.


VERDICT:
Worth merging - Implementation is sound. The validation gap is a pre-existing pattern in the API design and not a regression.

KEY INSIGHT:
The settings API and profiles API have different validation strictness for profile names. For new fields, this is acceptable, but consider aligning validation patterns across the codebase in a future pass.


This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

@enyst

enyst commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

@OpenHands address these points if you can. KISS. #3552 (review)

@openhands-ai

This comment was marked as outdated.

Co-authored-by: openhands <openhands@all-hands.dev>
@openhands-ai

This comment was marked as duplicate.

@enyst

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@OpenHands This PR has conflicts with main branch. Understand the PR very well, specially the versioning of settings; then merge main into it. Give me as a comment on github a report on what you did.

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

Resolve settings API conflicts by preserving main's misc_settings persisted-settings contract while keeping active_profile exposed through GET/PATCH /api/settings.

Confirmed no settings schema version bump is needed: AgentSettings remains v4, ConversationSettings remains v1, and PersistedSettings remains v2 because active_profile already exists in the v1 persisted baseline; v2 is reserved for misc_settings.

Co-authored-by: openhands <openhands@all-hands.dev>

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

Report after merging main into this PR branch:

  • Merged origin/main (50d73244) into feat/settings-active-profile and pushed merge commit 62595c12.
  • Resolved conflicts in:
    • openhands-sdk/openhands/sdk/settings/api_models.py
    • openhands-agent-server/openhands/agent_server/settings_router.py
    • tests/agent_server/test_settings_router.py
  • Kept the new misc_settings settings API/persistence contract from main and preserved this PR active profile exposure in GET /api/settings and PATCH /api/settings.
  • Clarified the PersistedSettings version-history docstring to say v1 already includes active_profile; v2 is the misc_settings addition.

Versioning conclusion:

  • No AgentSettings schema version bump is needed for this PR.
  • After the merge, AgentSettings is already at v4 from main, ConversationSettings is v1, and PersistedSettings is v2.
  • This PR does not change the AgentSettings or ConversationSettings persisted payload shape.
  • active_profile is a top-level PersistedSettings field and already exists in the v1 persisted-settings compatibility baseline, so exposing/updating it through the REST settings API is an additive API change rather than a persisted schema change.
  • PersistedSettings v2 remains correct because that version is for adding misc_settings.

Verification run locally:

uv run pytest tests/agent_server/test_settings_router.py tests/agent_server/test_profiles_router.py tests/cross/test_check_persisted_settings_compat.py -q
# 141 passed

uv run pre-commit run --files \
  openhands-sdk/openhands/sdk/settings/api_models.py \
  openhands-agent-server/openhands/agent_server/settings_router.py \
  openhands-agent-server/openhands/agent_server/persistence/models.py \
  openhands-agent-server/openhands/agent_server/profiles_router.py \
  tests/agent_server/test_settings_router.py \
  tests/agent_server/test_profiles_router.py
# all hooks passed

This PR comment was created by an AI agent (OpenHands) on behalf of @enyst.

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Since my last summary, there have been no additional code changes or tool actions.

Final status:

  • ✅ The PR conflict request was completely addressed.
  • main was merged into feat/settings-active-profile.
  • ✅ Conflicts were resolved while preserving both misc_settings from main and this PR’s active_profile settings API exposure.
  • ✅ Settings versioning was investigated:
    • AgentSettings remains v4.
    • ConversationSettings remains v1.
    • PersistedSettings remains v2.
    • No version bump is needed for this PR.
  • ✅ Focused tests and pre-commit passed.
  • ✅ The merge commit was pushed.
  • ✅ A GitHub PR comment was posted with the requested report and versioning conclusion.

The changes are concise relative to the request: the only manual merge-resolution addition beyond conflict resolution was a small clarification to the PersistedSettings version-history docstring.

@enyst enyst added review-this This label triggers a PR review by OpenHands and removed review-this This label triggers a PR review by OpenHands labels Jun 9, 2026

all-hands-bot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Expose active LLM profile in settings API

Taste Rating: 🟢 Good taste — clean, focused change

The PR exposes the active_profile field through the settings API, allowing clients to read and update the active LLM profile name. The changes are minimal and well-scoped.


Summary

  • openhands-sdk/openhands/sdk/settings/api_models.py: Added active_profile to both SettingsResponse and SettingsUpdateRequest with appropriate pattern validation.
  • openhands-agent-server/openhands/agent_server/settings_router.py: Included active_profile in GET/PATCH responses; updated error message to mention the new field.
  • openhands-agent-server/openhands/agent_server/profiles_router.py: Extracted duplicate active-profile update logic into _set_active_profile_if_matches() helper, used in delete_profile and rename_profile.
  • tests/agent_server/test_settings_router.py: Good coverage for set/clear/validation of active_profile.

Observations

  1. [Good] The PATCH handler correctly handles explicit null to clear the profile by checking payload.model_fields_set rather than relying on model_dump(exclude_none=True) alone. This is the right pattern.

  2. [Minor] test_list_profiles_no_auto_create_after_deleting_active_profile now also asserts active_profile is None. Good test improvement, but there's no corresponding test for delete_profile updating active_profile when the deleted profile was active. Consider adding one:

def test_delete_profile_clears_active_profile(client, ...):
    # Set active profile
    client.patch("/api/settings", json={"active_profile": "my-profile"})
    # Delete it
    client.delete("/api/profiles/my-profile")
    # Verify cleared
    assert client.get("/api/settings").json()["active_profile"] is None
  1. [Minor] rename_profile now logs the active_profile update but the log message for non-matching case is gone. This is fine — it was noisy before.

Risk Assessment

  • ⚠️ Risk Assessment: 🟢 LOW

No breaking changes. All new fields have defaults or are optional. Existing behavior is preserved. Tests cover the new functionality.


This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

@enyst

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@OpenHands address the nits #3552 (review) then use gh api to re-review from all-hands-bot

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

Co-authored-by: openhands <openhands@all-hands.dev>
@enyst enyst requested a review from all-hands-bot June 9, 2026 22:29

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

Addressed the review nit from all-hands-bot by adding test_delete_active_profile_clears_active_profile, which verifies that deleting the currently active profile clears active_profile in /api/settings and /api/profiles while preserving an unrelated profile.

Validation run locally:

  • uv run pytest tests/agent_server/test_profiles_router.py::test_delete_active_profile_clears_active_profile -q
  • uv run pytest tests/agent_server/test_profiles_router.py -q
  • uv run pre-commit run --files tests/agent_server/test_profiles_router.py

Pushed commit 6fff19a3 and re-requested review from all-hands-bot via the GitHub API.

This comment was created by an AI agent (OpenHands) on behalf of the user.

all-hands-bot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Since my last summary, there have been no additional code changes or actions.

Final status:

  • ✅ The PR review nit was addressed completely.
  • ✅ The change was concise: one focused regression test was added, with no unrelated edits.
  • ✅ Validation passed for the targeted test, the full profile-router test file, and pre-commit on the changed file.
  • ✅ The fix was committed and pushed to the existing PR branch.
  • ✅ A PR comment was posted summarizing the fix and validation.
  • ✅ Review was re-requested from all-hands-bot via the GitHub API.

No extraneous changes remain.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Expose active LLM profile in settings API

Taste Rating: 🟢 Good taste — clean, focused change

This PR exposes the active_profile field through the settings API, allowing clients to read and write the currently active LLM profile name. The implementation is minimal, follows existing patterns, and solves a real problem.

Overview

The change adds active_profile to:

  • SettingsResponse — now returns the active profile name (nullable)
  • SettingsUpdateRequest — now accepts active_profile as a settable field
  • GET /api/settings — returns the persisted active profile
  • PATCH /api/settings — accepts active_profile for updates
  • DELETE /api/profiles/{name} — clears active_profile if the deleted profile was active

What's Good

  1. Minimal surface area — Only 3 files touched in the server, 1 in SDK. No unnecessary abstractions.
  2. Refactoring in rename_profile — Extracting _set_active_profile_if_matches reduces duplication and makes the logic reusable for delete operations.
  3. Pattern consistency — Uses the same PROFILE_NAME_PATTERN validation as other profile operations.
  4. Comprehensive tests — Covers activate, delete, update, clear, and invalid name scenarios.

Minor Observations

  • Schema version docstring (models.py:9-12): The docstring update is correct — v1 did include active_profile after the migration, not just agent_settings + conversation_settings.
  • Empty payload error message (settings_router.py:132-133): The updated error message now includes active_profile as a valid field. This is appropriate since it can be set directly.

Testing

Tests cover the core functionality:

  • test_delete_active_profile_clears_active_profile — verifies deletion clears active
  • test_patch_settings_updates_active_profile — covers set, persist, and clear
  • test_patch_settings_rejects_invalid_active_profile — validates pattern matching

All assertions check real behavior against the API response, not just mocked calls.

Risk Assessment: 🟢 LOW

No breaking changes. The active_profile field is additive (nullable with None default). Existing clients are unaffected.


Worth merging: Clean implementation that follows existing patterns and solves the stated problem without introducing complexity.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Expose active LLM profile in settings API

Taste Rating: 🟢 Good taste — clean, focused change

This PR exposes the active_profile field through the settings API, allowing clients to read and write the currently active LLM profile name. The implementation is minimal, follows existing patterns, and solves a real problem.

Overview

The change adds active_profile to:

  • SettingsResponse — now returns the active profile name (nullable)
  • SettingsUpdateRequest — now accepts active_profile as a settable field
  • GET /api/settings — returns the persisted active profile
  • PATCH /api/settings — accepts active_profile for updates
  • DELETE /api/profiles/{name} — clears active_profile if the deleted profile was active

What's Good

  1. Minimal surface area — Only 3 files touched in the server, 1 in SDK. No unnecessary abstractions.
  2. Refactoring in rename_profile — Extracting _set_active_profile_if_matches reduces duplication and makes the logic reusable for delete operations.
  3. Pattern consistency — Uses the same PROFILE_NAME_PATTERN validation as other profile operations.
  4. Comprehensive tests — Covers activate, delete, update, clear, and invalid name scenarios.

Minor Observations

  • Schema version docstring (models.py:9-12): The docstring update is correct — v1 did include active_profile after the migration, not just agent_settings + conversation_settings.
  • Empty payload error message (settings_router.py:132-133): The updated error message now includes active_profile as a valid field. This is appropriate since it can be set directly.

Testing

Tests cover the core functionality:

  • test_delete_active_profile_clears_active_profile — verifies deletion clears active
  • test_patch_settings_updates_active_profile — covers set, persist, and clear
  • test_patch_settings_rejects_invalid_active_profile — validates pattern matching

All assertions check real behavior against the API response, not just mocked calls.

Risk Assessment: 🟢 LOW

No breaking changes. The active_profile field is additive (nullable with None default). Existing clients are unaffected.


Worth merging: Clean implementation that follows existing patterns and solves the stated problem without introducing complexity.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

@enyst

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@OpenHands why does the REST API check with oasdiff pass?

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

enyst commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

It passes because the REST API workflow is a breaking-change check, not an “any OpenAPI diff” check.

The workflow runs oasdiff breaking --fail-on ERR against the previous released agent-server OpenAPI schema. This PR only widens the /api/settings contract:

  • SettingsResponse.active_profile is a new nullable field with a default, so it is not added to the response schema's required list.
  • SettingsUpdateRequest.active_profile is a new optional nullable request field. Existing PATCH payloads remain valid; the new pattern validation only applies when a client sends this new field.
  • No endpoint, method, existing property, enum value, or required field was removed or tightened.

I also checked the generated schema locally: SettingsResponse.required is still only agent_settings, conversation_settings, and llm_api_key_is_set; active_profile appears as string | null but optional. SettingsUpdateRequest still has no required fields.

So from oasdiff's compatibility model, this is additive/backward-compatible. A strict client that rejects unknown JSON response fields could still be surprised at runtime, but that is outside what the OpenAPI breaking checker treats as a REST contract break.

This comment was created by an AI agent (OpenHands) on behalf of the user.

@openhands-ai

openhands-ai Bot commented Jun 9, 2026

Copy link
Copy Markdown

OpenHands encountered an error: **Failed to send message to agent server: HTTP 503 error: no available server
**

See the conversation for more information.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ QA Report: PASS

Verified the settings/profile APIs against live agent-server instances: active_profile is exposed, updateable, clearable, persisted, validated, and kept in sync on profile rename/delete.

Does this PR achieve its stated goal?

Yes. On main, GET /api/settings returned 200 but omitted active_profile, and PATCH /api/settings with active_profile returned 400. On PR commit 6fff19a3a6ecb2eca8a95a300af0ac4851589fab, the same API accepted active_profile, returned it on PATCH/GET, preserved raw agent_settings.llm.model, allowed clearing with null, rejected an invalid profile name with 422, and persisted the selected profile across a server restart.

Phase Result
Environment Setup make build completed and installed the runnable dev environment.
CI Status ⚠️ PR rollup showed the core checks green; qa-changes and multi-arch manifest merge checks were still in progress when checked.
Functional Verification ✅ Started real local agent-server processes and exercised affected APIs over HTTP.
Functional Verification

Test 1: Settings API exposes and updates active_profile

Step 1 — Establish baseline without the fix:
Checked out origin/main, started uv run agent-server --host 127.0.0.1 --port 18101, and made real HTTP requests to /api/settings:

baseline GET contains "active_profile": False
GET /api/settings HTTP_STATUS:200
PATCH /api/settings {"active_profile":"fast-profile"}
{"detail":"At least one of agent_settings_diff, conversation_settings_diff, or misc_settings_diff must be provided"}
HTTP_STATUS:400

This confirms the prior behavior described by the PR: clients could not read active_profile from the shared settings response, and could not update it through the settings PATCH contract.

Step 2 — Apply the PR's changes:
Checked out PR commit 6fff19a3a6ecb2eca8a95a300af0ac4851589fab and started uv run agent-server --host 127.0.0.1 --port 18102 with a fresh temp home.

Step 3 — Re-run with the fix in place:
Made the same style of HTTP requests, plus clear/validation/persistence checks:

### initial GET
HTTP_STATUS:200
{"active_profile": null, "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### PATCH active_profile=fast-profile
HTTP_STATUS:200
{"active_profile": "fast-profile", "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### GET after set
HTTP_STATUS:200
{"active_profile": "fast-profile", "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### PATCH active_profile=null
HTTP_STATUS:200
{"active_profile": null, "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### GET after clear
HTTP_STATUS:200
{"active_profile": null, "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### PATCH invalid active_profile
HTTP_STATUS:422
{"detail": [{"ctx": {"pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$"}, "input": "not a valid profile", "loc": ["body", "active_profile"], "msg": "String should match pattern '^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$'", "type": "string_pattern_mismatch"}]}
### PATCH active_profile=persistent-profile
HTTP_STATUS:200
{"active_profile": "persistent-profile", "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}
### GET after server restart
HTTP_STATUS:200
{"active_profile": "persistent-profile", "agent_settings.llm.model": "gpt-5.5", "llm_api_key_is_set": false, "misc_settings": {}}

This shows the PR delivers the stated settings API behavior and does not mutate raw agent_settings.llm when only active_profile is patched.

Test 2: Profile lifecycle keeps active_profile consistent

Step 1 — Establish baseline context:
The baseline settings API could not expose/update active_profile, so profile lifecycle consistency could not be verified through that shared contract on main.

Step 2 — Apply the PR's changes:
Using the same PR commit, started another fresh agent-server and exercised the profile APIs as a user/client would.

Step 3 — Run profile operations with the fix in place:
Created two profiles, activated one, renamed the active profile, then deleted it:

### save active-profile
HTTP_STATUS:201
{"message": "Profile 'active-profile' saved", "name": "active-profile"}
### save other-profile
HTTP_STATUS:201
{"message": "Profile 'other-profile' saved", "name": "other-profile"}
### activate active-profile
HTTP_STATUS:200
{"llm_applied": true, "message": "Profile 'active-profile' activated and applied to current settings", "name": "active-profile"}
### settings after activate
HTTP_STATUS:200
{"active_profile": "active-profile", "agent_settings.llm.model": "gpt-4o"}
### rename active-profile to renamed-profile
HTTP_STATUS:200
{"message": "Profile 'active-profile' renamed to 'renamed-profile'", "name": "renamed-profile"}
### settings after rename
HTTP_STATUS:200
{"active_profile": "renamed-profile", "agent_settings.llm.model": "gpt-4o"}
### profiles after rename
HTTP_STATUS:200
{"active_profile": "renamed-profile", "profiles": ["other-profile", "renamed-profile"]}
### delete renamed active profile
HTTP_STATUS:200
{"message": "Profile 'renamed-profile' deleted", "name": "renamed-profile"}
### settings after delete
HTTP_STATUS:200
{"active_profile": null, "agent_settings.llm.model": "gpt-4o"}
### profiles after delete
HTTP_STATUS:200
{"active_profile": null, "profiles": ["other-profile"]}

This confirms active profile state is readable through /api/settings and remains coherent across profile rename/delete operations.

Issues Found

None.

This QA review was created by an AI agent (OpenHands) on behalf of the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-this This label triggers a PR review by OpenHands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants