[AgentProfile][agent-server] active_agent_profile_id + /api/agent-profiles router + migration seed#3781
Conversation
…files router + migration seed Expose the AgentProfile store over HTTP and add a separate active pointer, mirroring the LLM profiles router. AgentProfile activation is pointer-only: unlike the LLM /activate it must not write agent_settings (creation-time-only contract). Runs parallel to #3717; POST /{id}/materialize is a fast-follow once the resolver lands. - PersistedSettings.active_agent_profile_id (additive, defaults None; no schema bump) threaded through update(), SettingsResponse/Update, and the settings_router GET/PATCH passthrough. - /api/agent-profiles router: CRUD + POST /{id}/activate (pointer only) with FK error mapping (ProfileReferenced/FileExists -> 409, FileNotFound -> 404, Timeout -> 503, ValueError -> 400). - Lazy migration seed: first GET on an empty store seeds one default profile from agent_settings and sets the active pointer. - LLM profiles_router delete/rename now enforce the agent-profile FK (delete referenced LLM profile -> 409 naming referrers; rename cascades). Closes #3719 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
simonrosenberg
added
the
review-this
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable — the router shape is straightforward and the pointer-only activation contract is clear, but there are two correctness gaps around migration fidelity and encrypted secret round-trips that should be addressed before approval.
[CRITICAL ISSUES]
- [openhands-agent-server/openhands/agent_server/agent_profiles_router.py, Line 249] Secret Handling: Saving a profile body that came from
GET /api/agent-profiles/{name}withX-Expose-Secrets: encryptedwill re-encrypt the already encryptedskills[].mcp_toolsenv/header values. A resolver that decrypts once then receives a Fernet token instead of the original secret, so ordinary edit/save round-trips can break MCP credentials. - [openhands-agent-server/openhands/agent_server/agent_profiles_router.py, Line 141] Migration Fidelity: The lazy seed does not preserve several overlapping fields from the current
agent_settings(agent_context.skills,system_message_suffix,condenser,verification, and ACPacp_command/acp_args). When the resolver starts honoringactive_agent_profile_id, existing single-config users can silently fall back to defaults rather than their configured launch behavior.
[TESTING GAPS]
- Add a cipher-backed router test that exercises
GET /api/agent-profiles/{name}with encrypted exposure followed byPOST /api/agent-profiles/{name}and verifies the stored MCP secret decrypts exactly once to the original value. - Add lazy-seed tests with non-default OpenHands and ACP settings so the migration proves it preserves the profile fields the resolver will later use.
I ran the focused router tests locally: uv run pytest tests/agent_server/test_agent_profiles_router.py tests/agent_server/test_settings_router.py::test_patch_settings_active_agent_profile_id_independent -q → 29 passed.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a new persisted HTTP surface and a migration path for existing users. The endpoint structure is simple, but secret round-tripping and migration fidelity directly affect whether future profile materialization launches agents with the expected credentials and settings.
VERDICT:
❌ Needs rework: The API shape is reasonable, but the encrypted edit/save path and the lazy seed need to preserve user data before this should be approved.
KEY INSIGHT:
The active pointer is deliberately lightweight, so the stored profile it points at must be a faithful and stable representation of the user's launch configuration.
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
Review bot flagged two correctness gaps: - Migration fidelity: the lazy seed now carries every cleanly-overlapping launch field from agent_settings (OpenHands: skills, system_message_suffix, condenser, verification subset; ACP: acp_command via shlex.join + acp_args), not just the LLM ref. Existing single-config users migrate faithfully once the resolver honors active_agent_profile_id. - Secret round-trip: skills[].mcp_tools env/headers are decrypted (Fernet -> plaintext) on GET, save, and seed via a shared helper. The store masks/ encrypts on save but has no symmetric load-time validator, so without this a GET(encrypted) -> POST edit double-encrypted the value. Plaintext now decrypts exactly once. Tests: cipher-backed GET(encrypted) -> POST round-trip + encrypted-at-rest, and seed-fidelity tests for non-default OpenHands and ACP settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the two findings in 97a4fd1: Secret round-trip (save path): added Migration fidelity (seed):
|
simonrosenberg
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.
🟡 Taste Rating: Acceptable - The API shape is straightforward and the test coverage is broad, but two identity/persistence paths can corrupt the new stable active-profile pointer.
[CRITICAL ISSUES]
save_agent_profilecan replace an existing profile with a fresh UUID/revision when the request body omits those fields, leavingactive_agent_profile_idpointing at a profile id that no longer exists.- The lazy migration seed is not idempotent under concurrent first requests, so it can also leave the active pointer referring to an overwritten default profile id.
[TESTING GAPS]
- Add regression coverage for overwriting an active profile with a create-style body that omits
id/revision, and for concurrent firstGET /api/agent-profilescalls (or an equivalent locked/idempotent seed test).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds new agent-server endpoints and persistence migration behavior. The main risk is not broad security exposure, but durable settings/store inconsistency: once the pointer references a non-existent profile id, the UI/server can lose the active selection until manually repaired.
VERDICT:
❌ Needs rework: Stable AgentProfile identity is the core invariant for pointer-only activation, so updates and lazy seeding must preserve the UUID that settings point at.
KEY INSIGHT:
The design is sound only if every write path treats the AgentProfile UUID as stable state, not as another defaultable request field.
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
Review bot flagged two pointer-integrity gaps (active_agent_profile_id is keyed on the stable id): - save_agent_profile preserved nothing, so a create-style body (no id/revision) overwriting a namesake minted a fresh UUID and reset revision, dangling the active pointer. Now reuses the existing profile's id and bumps revision. - The lazy seed wasn't idempotent under concurrent first requests. It now holds the store lock across the empty-check + save + pointer write (double-checked), so concurrent first GETs seed exactly once and the pointer always matches the persisted profile id. Tests: overwrite-preserves-id-and-pointer + concurrent-first-list-seeds-once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed both id-stability findings in a48296e: Overwrite mints a fresh id (Line 382): Seed not idempotent under concurrency (Line 325): |
simonrosenberg
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.
🟡 Acceptable — well-structured, correctly locked where it matters, and has excellent test coverage including a real concurrency test for the seed path. A few minor corrections before this merges.
[CRITICAL ISSUES]
- [agent_profiles_router.py, Line 517] Wrong HTTP Status: (corrupted/mis-keyed settings file) is mapped to HTTP 409 Conflict. A server-side data integrity failure should be HTTP 500. The parallel handler in correctly returns 500 for the same scenario.
[IMPROVEMENT OPPORTUNITIES]
-
[api_models.py, Line 120] Missing Format Validation: on has no format constraint, while the sibling field has . A UUID pattern would reject clearly malformed inputs at the HTTP layer.
-
[models.py, Line 261] Comment Noise: is a diff-narrative comment that restates what the code already shows. It will drift as the surrounding code changes.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
New router, FK guard on existing delete/rename (could 409 callers that previously succeeded), and writes to the shared file. All changes are well-tested with real code paths. The pointer-only activation contract is correctly enforced. The lazy migration seed is guarded by the store lock with a double-check. Breaking risk is low — defaults to so existing settings files deserialize unchanged.
VERDICT:
✅ Worth merging: one correctness fix (HTTP 500 vs 409), plus two optional polish items.
KEY INSIGHT:
The seed holding the store lock across empty-check + save + pointer write is the right design — that window is exactly where a concurrent first request would otherwise race.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a file to your branch (or edit it if one already exists) with the trigger and the context the reviewer is missing. 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
…t trim - activate: a corrupted/mis-keyed settings file (RuntimeError) is a server-side integrity failure → 500, not 409. Test added. - SettingsUpdateRequest.active_agent_profile_id gains a UUID format pattern so malformed pointers are rejected at the HTTP layer (mirrors active_profile). - Drop a diff-narrative comment in PersistedSettings.update(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All three addressed in eda4828:
|
simonrosenberg
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.
🟡 Acceptable — Solid implementation of a complex feature with one sharp edge.
Overview
This PR cleanly delivers Phase 2 of the AgentProfile epic: a full CRUD router for named agent launch specs, a pointer-only active_agent_profile_id threaded through the settings surface, a one-time lazy seed migration, and a FK guard on LLM profile delete/rename. The design is intentional and well-documented.
Standouts:
- The concurrent seeding double-check (
store._acquire_lock()+ innerstore.list()guard) is the right approach for a lazy singleton migration. - Activation is correctly pointer-only — no
agent_settingsmutation. TheActivateAgentProfileResponse.agent_settings_applied: bool = Falsefield makes the contract self-documenting. - The FK guard on
delete_llm_profile/rename_llm_profilenames the blocking referrers in the 409, which is exactly what a caller needs to unblock. - Test coverage is comprehensive: 35 test functions including a concurrent seeding test with 8 threads and a full MCP-secrets round-trip test.
Issues
[IMPROVEMENT OPPORTUNITIES]
-
[
agent_profiles_router.py, Line 386] Error detail strips all diagnostic information:detail=f"Invalid agent profile: {type(e).__name__}"tells the client only the exception class name ("ValidationError","ValueError","TypeError"), losing the actual field-level error messages from Pydantic and the value messages from the others. A client posting a bad profile body gets back zero actionable information.detail=str(e)(or at minimumf"Invalid agent profile: {e}") would give a structured, readable breakdown of which fields are wrong. The PydanticValidationError.__str__is already client-safe. -
[
agent_profiles_router.py, Line 253] Cross-module private method call:store._acquire_lock()reaches intoAgentProfileStore's internals from outside the module. This works today, but it's fragile — any refactor of how the store serializes its locking API would silently break this call site. If the locking-across-the-emptiness-check semantics are genuinely needed by callers, the store should expose them through a public API (store.acquire_lock()or astore.seed_once(profile, ...)helper). For now this is a style concern, but it will bite on the first internal refactor ofAgentProfileStore.
[TESTING GAPS]
- Missing coverage for stale-pointer + empty store: The seeding condition is
not existing and settings.active_agent_profile_id is None. If the store is empty but the pointer is non-null (e.g. after a failed delete or direct-FS manipulation),GET /api/agent-profilesreturns{"profiles": [], "active_agent_profile_id": "stale-uuid"}— no seed, no error. This is probably intentional (stale pointer = user state), but there is no test asserting this behavior, making it invisible to future readers and easy to accidentally change.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
All changes are additive: new router, new nullable field with backward-compatible default, FK guard that only adds a new 409 path to previously-silent success. Older settings files load cleanly withactive_agent_profile_id=None(no schema-version bump needed). The only behavioral change on existing endpoints is the 409 guard on LLM profile delete/rename, which is tested. No breaking changes to existing API surface.
VERDICT:
✅ Worth merging: The pointer-only activation contract, lazy seeding with lock, and FK guard are all correct. The error-message issue (line 386) is the only thing I'd fix before shipping — it will make production debugging significantly harder when clients post malformed profiles.
KEY INSIGHT:
The creation-time-only pointer design is the right call — keeping active_agent_profile_id strictly separate from active_profile prevents the resolver from having to unwind entangled live settings.
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. 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
- save_agent_profile: surface field locations + error types on a ValidationError (actionable), but never the input/msg (a nested mcp_tools MCPConfig error embeds the input, which may carry secrets). Any other validation failure — notably SkillValidationError from a malformed mcp_tools, which is NOT a ValueError and previously escaped to a 500 leaking the secret in the traceback — is now caught and returned as a generic 422. - Tests: 422 surfaces the offending field location; a malformed mcp_tools body never echoes its secret; empty store + non-null (stale) pointer is left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in 94c6c5c: Error detail (line 386):
|
simonrosenberg
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.
🟡 Taste Rating: Acceptable — the router is much tighter after the previous fixes, and the focused tests pass, but the new id-keyed activation path still allows one durable identity invariant to be broken.
[CRITICAL ISSUES]
- [openhands-agent-server/openhands/agent_server/agent_profiles_router.py, Line 414] Stable ID Uniqueness: Creating a new profile can reuse an existing profile's
idfrom the request body. Because activation and active-pointer cleanup are keyed only byid, duplicate ids make the pointer ambiguous and let deleting one profile clear the active selection even while another profile with the same id still exists.
[TESTING GAPS]
- Add a regression test that creates profile
a, creates profilebwitha'sid, activates that id, then deletesa. The API should either reject the duplicate id or assignba fresh id so the active pointer remains unambiguous.
I ran the focused router/settings coverage locally: uv run pytest tests/agent_server/test_agent_profiles_router.py tests/agent_server/test_profiles_router.py::test_delete_referenced_llm_profile_returns_409 tests/agent_server/test_profiles_router.py::test_rename_llm_profile_cascades_to_agent_refs tests/agent_server/test_settings_router.py::test_patch_settings_active_agent_profile_id_independent -q → 40 passed.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a new persisted HTTP surface and a new active pointer whose correctness depends onAgentProfile.idbeing a unique stable handle. Most paths preserve that invariant, but accepting duplicate ids on new names can leave the UI/server with ambiguous active-profile state.
VERDICT:
❌ Needs rework: Do not approve until the save path enforces unique AgentProfile ids (or ignores client-supplied ids for new profile names) and covers the duplicate-id case.
KEY INSIGHT:
A pointer-only design is only safe if the pointed-to id is globally unique across the profile store, not just stable within a single profile 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.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
The active pointer is keyed on AgentProfile.id, so ids must be globally unique, not just stable per file. save now mints a server id on create (ignoring any client-supplied id) and preserves it only on overwrite. Prevents a client from creating two names with the same id, which made the pointer ambiguous (deleting one could clear the active selection while a namesake id lives on). Test: creating 'b' with 'a's id yields a distinct id; activating b then deleting a leaves the pointer intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed in 56b6023: |
simonrosenberg
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.
🟢 Good taste — the earlier blocking concerns are fully addressed in this commit.
Key fixes verified:
- ID uniqueness:
save_agent_profilenow always mints a freshuuid4()for new names, ignoring any client-suppliedid. Overwriting a namesake keeps the existingidstable so active pointers survive edits. ✅ - Regression test:
test_create_mints_fresh_id_ignoring_client_idexercises the exact scenario from the prior review — creates profilea, tries to reusea's id for profileb, deletesa, and confirms the active pointer tobis unambiguous. ✅ - Seed race: Double-checked locking under
store._acquire_lock()prevents concurrent first requests from seeding twice. ✅ - MCP secret double-encrypt:
_decrypt_profile_mcp_toolsis called before both_seed_default_profileandsave_agent_profilesaves, preventing Fernet-token double-encryption on round-trips. ✅
All 10 prior review threads are resolved. Test coverage: 647-line test file covering CRUD, activation, seeding, FK guards, cipher round-trips, and error mapping. Broader suite (1477 tests) passes with one pre-existing unrelated failure on main that is untouched by this PR.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
New persisted HTTP surface with an active pointer keyed on UUID. The critical invariant (id-uniqueness across the profile store) is now enforced on all write paths and covered by a regression test. FK guards on LLM profile delete/rename add new behavior to an existing router but are well-exercised. The one-time lazy migration seeds exactly once under a lock. Suitable for merge once out of draft.
VERDICT:
✅ Worth merging: The blocking ID-uniqueness issue is fixed and regressed; no new concerns found.
KEY INSIGHT:
Server-managed UUIDs (always uuid4() on first save, never trusted from the client) make the active pointer safe — the correctness argument is now fully encoded in both the implementation and the test suite.
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
The new agent-profile HTTP surface works end-to-end in the running FastAPI app, including lazy seeding, independent pointer updates, pointer-only activation, rename/delete pointer behavior, and LLM-profile FK protection.
Does this PR achieve its stated goal?
Yes. I exercised the actual agent-server API with an isolated HOME/persistence directory using FastAPI TestClient: on main, /api/agent-profiles was absent and active_agent_profile_id could not be cleared; on this PR, first list seeded one default profile and set the active pointer, CRUD worked, activation set only active_agent_profile_id while leaving agent_settings unchanged, renames preserved the stable id/pointer, deletes cleared the active pointer, and deleting a referenced LLM profile returned 409 naming the referencing agent profile.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and installed the uv workspace environment. |
| CI Status | 🟡 Queried GitHub checks: test/pre-commit/API compatibility checks were green; Agent Server build/coverage jobs were still in progress at query time. |
| Functional Verification | ✅ Real API calls against the running FastAPI app matched the PR’s stated behavior. |
Functional Verification
Test 1: Baseline on origin/main
Step 1 — Establish baseline without the PR:
Ran git checkout origin/main && uv run python /tmp/qa_agent_profiles_flow.py.
Relevant output:
{
"GET /api/agent-profiles first": {"status": 404, "body": {"detail": "Not Found"}},
"PATCH /api/settings clear agent pointer": {
"status": 400,
"body": {"detail": "At least one of agent_settings_diff, conversation_settings_diff, misc_settings_diff, or active_profile must be provided"}
},
"POST /api/agent-profiles/my-agent": {"status": 404, "body": {"detail": "Not Found"}},
"POST /api/profiles/base-llm": {"status": 201},
"DELETE /api/profiles/base-llm": {"status": 200}
}This shows the new agent-profile HTTP router and settings pointer behavior did not exist on the base branch; the LLM profile delete path also allowed deletion in this baseline flow.
Step 2 — Apply the PR’s changes:
Checked out agent-profile-router at d8978d27161cce20a4990876008898b05527feb2.
Step 3 — Re-run with the PR in place:
Ran uv run python /tmp/qa_agent_profiles_flow.py.
Relevant output:
{
"GET /api/agent-profiles first": {
"status": 200,
"body": {
"active_agent_profile_id": "ff130889-316a-4c75-906d-aebeaa98251e",
"profiles": [{"name": "default", "agent_kind": "openhands", "llm_profile_ref": "default", "mcp_server_refs": null}]
}
},
"PATCH /api/settings set both pointers": {"status": 200, "body": {"active_profile": "keep-me", "active_agent_profile_id": "12345678-1234-1234-1234-1234567890ab"}},
"PATCH /api/settings clear agent pointer": {"status": 200, "body": {"active_profile": "keep-me", "active_agent_profile_id": null}}
}This confirms the lazy migration seed creates exactly one default profile and persists an active agent-profile pointer, and that active_agent_profile_id can be set/cleared independently from the existing active LLM profile pointer.
Test 2: Agent-profile CRUD and pointer-only activation
Step 1 — Baseline:
On origin/main, POST /api/agent-profiles/my-agent returned 404, so this user-facing API did not exist.
Step 2 — Apply the PR’s changes:
Used the PR branch/commit above.
Step 3 — Exercise the new behavior:
The same API flow created, fetched, activated, renamed, and deleted an agent profile:
{
"POST /api/agent-profiles/my-agent": {"status": 201},
"GET /api/agent-profiles/my-agent": {
"status": 200,
"body": {"profile": {"id": "c969c2c9-0b06-4e49-b5a5-6b0ecc9ef27b", "llm_profile_ref": "base-llm", "system_message_suffix": "qa-user-flow"}}
},
"POST /api/agent-profiles/{id}/activate": {
"status": 200,
"active_agent_profile_id_after": "c969c2c9-0b06-4e49-b5a5-6b0ecc9ef27b",
"agent_settings_unchanged": true,
"body": {"agent_settings_applied": false}
},
"POST /api/agent-profiles/my-agent/rename": {
"status": 200,
"renamed_id": "c969c2c9-0b06-4e49-b5a5-6b0ecc9ef27b",
"active_pointer_after_rename": "c969c2c9-0b06-4e49-b5a5-6b0ecc9ef27b"
},
"DELETE /api/agent-profiles/renamed-agent": {"status": 200, "active_pointer_after_delete": null}
}This confirms the active selection is id-based, activation is pointer-only (agent_settings_unchanged: true and agent_settings_applied: false), renames preserve the id/pointer, and deleting the active profile clears the pointer.
Test 3: LLM profile FK protection
Step 1 — Baseline:
On origin/main, the new agent-profile HTTP route was unavailable; creating and deleting an LLM profile through /api/profiles completed with 201 then 200.
Step 2 — Apply the PR’s changes:
Used the PR branch/commit above.
Step 3 — Exercise referenced LLM delete:
Created an LLM profile, created an agent profile referencing it, then attempted to delete the LLM profile:
{
"POST /api/profiles/base-llm": {"status": 201},
"POST /api/agent-profiles/refs-base": {"status": 201},
"DELETE /api/profiles/base-llm": {
"status": 409,
"body": {"detail": "LLM profile is referenced by 1 agent profile(s): refs-base"}
}
}This confirms the FK guard works through the public API and names the referrer as claimed.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
Cosmetic consistency, no behavior change to status codes: - _store_errors now maps the same set as profiles_router (TimeoutError -> 503, ValueError -> 400); FileNotFoundError/FileExistsError are handled inline per endpoint with clean, resource-specific messages (matching get/rename there). - save's 422 uses FastAPI's request-validation shape (detail = list of error objects), trimmed to loc/type to stay secret-safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
simonrosenberg
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.
🟡 Acceptable — solid design, a few cleanup items worth addressing before merge.
Summary
Phase 2 of the AgentProfile epic is well-structured. The pointer-only activation contract is correctly enforced, concurrent seeding is properly guarded, the id-keyed pointer is a smart choice (survives renames), UUID validation happens at the HTTP boundary, and secret leakage is explicitly blocked from 422 responses. The FK guard on LLM profile delete/rename is a thoughtful addition. Tests are comprehensive and cover the interesting concurrency and round-trip cases.
Found a few items below worth addressing.
[IMPROVEMENT OPPORTUNITIES]
-
[agent_profiles_router.py, ~line 95]
agent_settings_applied: bool = False— This field is alwaysFalsein the response; the endpoint unconditionally constructsActivateAgentProfileResponsewithout ever setting it toTrue. Per the PR description,POST /{id}/materializeis a deliberate fast-follow that isn't implemented here. Shipping a permanently-Falsefield is misleading — API consumers could reasonably infer it has semantic meaning. Either add a comment like# Always False until materialize (#3717) landsto make the pre-wiring explicit, or defer this field until the endpoint that can actually set it arrives. -
[agent_profiles_router.py, ~line 540]
except (OSError, PermissionError)—PermissionErroris a subclass ofOSErrorin Python's exception hierarchy, so the union is redundant.except OSErroris sufficient. -
[agent_profiles_router.py,
save_agent_profile] There is a TOCTOU window between_existing_identity(store, name)(reads without the write lock) and the subsequentstore.save(...). Two concurrent POSTs for the same new profile name both observeexisting_id=None, both mint fresh UUIDs, and the second write overwrites the first — dangling anyactive_agent_profile_idpointer set to the first write's UUID. The seed path avoids this correctly (it holdsstore._acquire_lock()across check + save + pointer write). The save path should do the same, or id-minting should move inside the store. This is an unlikely race in practice, but the seed path already demonstrates the pattern. -
[agent_profiles_router.py,
list_agent_profiles] The emptiness check callsstore.list()(deserialises full profiles) thenstore.list_summaries()separately for the response. Usinglist_summaries()for the emptiness check too avoids the double I/O and the unnecessary full-profile deserialisation.
[STYLE NOTES]
- Excessive inline comments — many docstrings paraphrase the PR description or restate what the code structure already shows. The
_seed_default_profile8-line docstring and the_decrypt_profile_mcp_tools18-line docstring are examples: they narrate the what instead of the why. Reserve comments for genuinely non-obvious invariants — the# settings store the command as a token list; the profile holds a single (re-parseable) stringcomment is a good example that earns its place.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Changes are additive: new field with a safe default (None), new router that doesn't affect existing routes, FK guard that fails safe (rejects rather than silently deletes). No breaking changes to existing APIs. All net-new behavior is covered by tests.
VERDICT:
✅ Worth merging — Core logic is sound and well-tested. Address the TOCTOU in save_agent_profile and the two minor nits before merge.
KEY INSIGHT:
The id-keyed active pointer is the right abstraction — it's the only reason rename and concurrent-overwrite semantics can be proven safe — but that guarantee is only complete when the save path holds the lock during the full identity-read + id-mint + write sequence, just as the seed path already does.
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. 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
- save_agent_profile now holds the store lock across the identity read + id mint + write, so two concurrent creates of the same new name converge on one id instead of clobbering each other (mirrors the seed path). Regression test added. - Comment agent_settings_applied as intentionally always-False (pointer-only; materialize #3717 is the path that resolves into settings). - Trim the _seed_default_profile / _decrypt_profile_mcp_tools docstrings to the non-obvious why. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed review 8 in bfbe52a:
Two intentionally kept, for consistency with the existing codebase (per the request to align with current conventions):
|
simonrosenberg
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.
🟢 Good taste — the main blocking concern from the previous review (TOCTOU race in save_agent_profile) is fully resolved. The id-read + id-mint + store.save are now all executed under store._acquire_lock(), matching the pattern the seed path already used correctly. All prior critical/important threads are resolved.
Two minor items left from the last review remain unaddressed (noted inline). Neither is blocking — the core design is correct, the pointer-only activation contract is enforced, concurrency is properly guarded, secret handling is secure, and tests are thorough.
[IMPROVEMENT OPPORTUNITIES]
list_agent_profilescallsstore.list()(full profile deserialisation) for the emptiness check and thenstore.list_summaries()separately for the response — double I/O, see inline.except (OSError, PermissionError)inactivate_agent_profile—PermissionErroris a subclass ofOSError; the union is redundant, see inline.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — fully additive: new nullable field with a safe default, new router that doesn't interfere with existing routes, FK guard that fails safe (rejects rather than silently deletes). No breaking changes to existing APIs.
VERDICT:
✅ Worth merging — all blocking concerns addressed, comprehensive test coverage, solid evidence in the PR description.
KEY INSIGHT:
The id-keyed active pointer is the right abstraction; the TOCTOU fix in save_agent_profile closes the last gap where two concurrent creates could produce a dangling pointer.
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. 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
|
On the two remaining non-blocking suggestions — keeping both, for consistency with the existing codebase (and one is a factual miss):
Resolving these two threads; the review is already approved. |
2 participants
HUMAN:
Phase 2 of the AgentProfile epic (#3713): serve the new store over HTTP and add the separate, pointer-only active-profile selector. Reviewed the seed mapping and the FK-on-delete wiring locally; behaviour matches the issue's creation-time-only contract.
AGENT:
Why
Epic #3713 (AgentProfiles) needs the merged
AgentProfileStore(#3716) exposed over HTTP so the frontend/cloud can list, edit, and select named agent launch specs. Selecting anAgentProfileis creation-time-only: unlike the existing LLM/activate(which rewritesagent_settings.llm), activating anAgentProfilemust only move a pointer and never mutate liveagent_settings. This PR adds that surface and a one-time lazy migration so existing single-config users land with one profile already present.Runs in parallel with #3717 (the resolver); it depends only on the merged store, not the resolver.
Summary
PersistedSettings.active_agent_profile_id: str | None = None— distinct fromactive_profile(the active LLM profile name). Additive with a default, so older settings files load with itNone(no schema-version bump). Threaded throughSettingsUpdatePayload+update(), the SDKSettingsResponse/SettingsUpdateRequest, and thesettings_routerGET/PATCH passthrough (PATCH can set or clear it, independently ofactive_profile)./api/agent-profilesrouter (mirrorsprofiles_router.py): list / get / save / delete / rename +POST /{id}/activatethat sets onlyactive_agent_profile_idviasettings_store.update— noagent_settingswrite. Activation is keyed on the stableid, so it survives renames. FK/store error mapping:ProfileReferenced/FileExistsError→ 409,FileNotFoundError→ 404,TimeoutError→ 503,ValueError→ 400,ProfileLimitExceeded→ 409.GET /api/agent-profileson an empty store with a null pointer seeds exactly one default profile from the currentagent_settings(OpenHands → references the active LLM profile, falling back to"default"; ACP → an ACP profile;mcp_server_refs=null= all) and sets the active pointer. Conservative — one profile, never one-per-LLM-profile.profiles_routerdelete/rename now go throughdelete_llm_profile/rename_llm_profile, so deleting an LLM profile anAgentProfilereferences returns 409 naming the referrers, and renaming cascades thellm_profile_ref.api.pynext toprofiles_router.Deferred:
POST /{id}/materializeis a fast-follow after #3717 — it will load + decrypt the globalmcp_configviadecrypt_mcp_config_secrets, call the resolver's dry-run, and mapDanglingMcpServerRef→ 422. Leaving it out keeps this PR green and mergeable independently.Issue Number
Closes #3719 (epic #3713). Parallel to #3717.
How to Test
Automated (run from repo root):
Broader sweep (
tests/agent_server tests/cross/test_check_persisted_settings_compat.py tests/sdk/profiles tests/sdk/llm/test_llm_profile_store.py tests/sdk/test_settings.py): 1477 passed. The single unrelated failure (test_conversation_service.py::TestAutoTitle::test_autotitle_sets_title_on_first_user_message) is pre-existing onmain— those files are byte-identical toorigin/mainand the test fails there too; it is untouched by this PR.Live end-to-end against the real app (
create_app+ FastAPITestClient, isolated temp HOME/persistence dirs):Type
Notes
id(not the renameable name), so an active selection survives profile renames; the pointer is cleared automatically when the active profile is deleted.materializefast-follows once [AgentProfile][sdk] resolve_agent_profile(): collision-checked composition + resource-specific secret channels #3717 merges.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:bfbe52a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
bfbe52a-python) is a multi-arch manifest supporting both amd64 and arm64bfbe52a-python-amd64) are also available if needed