[AgentProfile][sdk] resolve_agent_profile()#3780
Conversation
Resolve an AgentProfile's references into a validated AgentSettingsConfig: the single join point between the authoring layer (profiles + refs) and the execution layer, so create_agent / apply_agent_settings_diff / validate_agent_settings stay unchanged (epic #3713, closes #3717). - MCP composition shared by both variants: null refs pass mcp_config through; a non-null list filters mcpServers to the named keys; a missing name raises DanglingMcpServerRef. - OpenHands path injects the LLM loaded from LLMProfileStore (missing -> ProfileNotFound), decrypts skills[].mcp_tools ciphertext with the cipher (deferred from AgentProfileStore.load), and copies the configurable surface. - ACP path sets acp_* + filtered mcp_config and injects no credential; provider creds ride state.secret_registry at conversation-start (#3720). - resolve_agent_profile_dry_run() returns AgentProfileDiagnostics (resolved / dangling refs, required ACP provider secret names, valid/invalid verdict, redacted resolved settings) for /materialize (#3719) and the canvas editor. 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.
🟡 Acceptable - The resolver shape is clean, but the dry-run path can still raise on malformed profile input instead of returning diagnostics.
[CRITICAL ISSUES]
- None.
[IMPROVEMENT OPPORTUNITIES]
- [openhands-sdk/openhands/sdk/profiles/resolver.py, Line 331] Dry-run correctness:
resolve_agent_profile_dry_run()promises a non-raising diagnostics path, but valid profile models can still fail while building settings. The inline comment has a concrete ACP command example.
[TESTING GAPS]
- Add a regression test where settings construction fails after the initial dangling-ref checks (for example an ACP command with unmatched shell quotes) and assert the dry-run returns
valid=Falsewith an error instead of raising.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a new SDK profile materialization path that composes secrets, MCP configuration, and agent settings. The normal-path coverage is good and the targeted resolver tests pass locally, but the dry-run failure contract is user-facing and currently has an exception path.
VERDICT:
❌ Needs rework: Normalize settings-build/validation failures into diagnostics before this is ready.
KEY INSIGHT:
The data flow is straightforward; the remaining issue is making the dry-run path as total as its API contract says it is.
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
Address PR review: resolve_agent_profile_dry_run() promised a non-raising diagnostics contract but settings construction could still raise on input that passes profile validation (e.g. an acp_command with unbalanced shell quotes, which shlex.split rejects). Wrap the build step so such failures surface as valid=False + an error instead of propagating. Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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.
✅ QA Report: PASS
The new AgentProfile resolver API works end-to-end for the claimed SDK use cases: OpenHands resolution, ACP resolution, MCP reference composition, secret-channel handling, and dry-run diagnostics.
Does this PR achieve its stated goal?
Yes. The PR set out to add the phase-1 SDK join point that converts profile references into validated AgentSettingsConfig without changing create_agent; I exercised that public SDK path directly and confirmed resolved OpenHands settings create an Agent, resolved ACP settings create an ACPAgent, MCP refs filter/pass-through/disable as described, dangling refs surface the expected exceptions/diagnostics, and dry-run output redacts secrets. I also verified the API is absent on main and available on the PR branch, so this is the new behavior introduced by the PR.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the uv environment. |
| CI Status | ⏳ GitHub checks observed with 0 failing, 18 successful, 11 pending, and 3 skipped; no local tests were run. |
| Functional Verification | ✅ Real SDK scripts exercised resolver calls with profile stores, MCP configs, cipher-backed skill MCP tools, dry-run diagnostics, and create_agent(). |
Functional Verification
Test 1: Baseline API availability before the PR
Step 1 — Establish baseline without the PR:
Ran git switch main && uv run python /tmp/qa_agent_profile_baseline.py:
ab886d39
has_resolve_agent_profile= False
import_error= ImportError
import_error_message= cannot import name 'resolve_agent_profile' from 'openhands.sdk.profiles' (...)
This shows the resolver entry point does not exist on main, establishing the before state for this new SDK feature.
Step 2 — Apply the PR's changes:
Checked out agent-profile-resolver at 110749a56fe3ba095369b79d3af40803178988d7.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_agent_profile_resolver_functional.py:
OpenHands resolve: settings built, LLM injected, MCP filtered, Agent created
MCP and LLM reference handling: passthrough, none, dangling, and missing cases OK
ACP resolve: command tokenized, MCP filtered, no credentials injected, ACPAgent created
Skill MCP secret handling: ciphertext at rest, plaintext restored for execution
Dry-run diagnostics: valid/invalid verdicts, redaction, and ACP secret names OK
SUMMARY {"acp_agent_type": "ACPAgent", "acp_required_secrets": ["OPENAI_API_KEY", "OPENAI_BASE_URL", "CODEX_AUTH_JSON"], "dry_run_secret_leak": false, "filtered_mcp_servers": ["fetch"], "openhands_agent_type": "Agent"}
This confirms the PR's public resolver APIs compose validated settings that feed the unchanged create_agent() path, handle the claimed MCP and LLM reference cases, preserve ACP credential separation, decrypt cipher-backed skill MCP tools for execution, and keep dry-run settings redacted.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - The resolver is clean and the previous dry-run exception path is fixed, but the ACP dry-run diagnostics currently overstate optional/alternative credentials as mandatory.
[CRITICAL ISSUES]
- None.
[IMPROVEMENT OPPORTUNITIES]
- The ACP secret diagnostics should distinguish required credentials from optional proxy settings and alternative auth mechanisms. Details are in the inline comment.
[TESTING GAPS]
- Add or adjust ACP diagnostic tests so a provider-specific optional base URL and alternative file/API auth paths are not treated as jointly required secrets.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a new SDK profile materialization path that composes stored profiles, LLM profiles, MCP configuration, and ACP settings. The core resolver flow is straightforward, the previous dry-run exception path is addressed, anduv run pytest tests/sdk/profiles/test_resolver.py -qpasses locally (17 passed). The remaining risk is in the diagnostic contract: downstream editor/materialize code may reject valid ACP configurations if it treats optional or alternative credential names as mandatory.
VERDICT:
❌ Needs rework: Fix the ACP credential diagnostic shape before relying on this in materialize/editor flows.
KEY INSIGHT:
The data flow is solid; the problematic part is collapsing optional, alternative, and required ACP secret channels into one required list.
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 is 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
Address PR review: the dry-run lumped api_key_env_var, base_url_env_var, and file_secrets[].secret_name into one "required" list, but they are not jointly required — the API key and file-content creds are alternative auth paths (one suffices) and the base URL is optional proxy routing. Replace required_acp_secret_names with acp_api_key_secret_name / acp_base_url_secret_name / acp_file_secret_names so the editor/materialize can report set/missing honestly instead of flagging a working api-key-only setup as incomplete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - The resolver is mostly clean and the focused resolver tests pass, but one ACP validity gap still lets materialization report a profile as valid even though it cannot create an agent.
[CRITICAL ISSUES]
- [openhands-sdk/openhands/sdk/profiles/resolver.py, Line 251] ACP validation: A
customACP profile with noacp_commandresolves successfully andresolve_agent_profile_dry_run()returnsvalid=True, but the resolved settings fail later increate_agent()becauseACPAgentSettings.resolve_acp_command()has no default forcustom. Details are in the inline comment.
[IMPROVEMENT OPPORTUNITIES]
- None beyond the blocking validation gap above.
[TESTING GAPS]
- Add a regression test for
ACPAgentProfile(name=..., acp_server="custom")withoutacp_commandso dry-run/materialize cannot claim invalid launch settings are valid.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This adds a new SDK profile-resolution path that feeds conversation startup. The scope is contained anduv run pytest tests/sdk/profiles/test_resolver.py -qpasses locally, but the current ACP custom-command gap can surface as a late runtime failure in profile materialization / conversation start.
VERDICT:
❌ Needs rework: Fix the ACP custom-command invariant before this is merge-ready.
KEY INSIGHT:
The resolver needs to validate executability invariants for the settings it materializes, not just Pydantic shape.
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
Address PR review (critical): a custom ACP profile with no acp_command built a valid ACPAgentSettings, so dry-run reported valid=True even though create_agent() would later raise in resolve_acp_command() (custom has no default command). _build_acp_settings now enforces that invariant up front, so the strict resolve raises and the dry-run reports valid=False instead of deferring the failure to conversation start. Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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.
✅ QA Report: PASS
Verified the new SDK profile resolver end-to-end with real profile/LLM/MCP stores and settings.create_agent() for both OpenHands and ACP profiles.
Does this PR achieve its stated goal?
Yes. The PR sets out to add the phase-1 SDK substrate that resolves secret-free AgentProfile references into validated AgentSettingsConfig objects while leaving execution paths like create_agent() unchanged. On main, the resolver API is absent; on the PR commit, real SDK scripts created stores/profiles, resolved OpenHands and ACP settings, filtered MCP refs, produced redacted dry-run diagnostics, decrypted cipher-backed skill MCP secrets, and instantiated Agent/ACPAgent successfully.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the uv dev environment. |
| CI Status | 🟡 At review time: 19 successful, 9 pending, 3 skipped, 0 failing checks. |
| Functional Verification | ✅ Resolver behavior worked for OpenHands, ACP, MCP filtering/errors, dry-run diagnostics, and encrypted skill MCP secrets. |
Functional Verification
Test 1: Baseline vs PR profile-to-settings resolver
Step 1 — Establish baseline without the feature:
Ran git checkout --detach origin/main && uv run python - <<'PY' ... import resolve_agent_profile ... PY:
BASELINE_RESULT=resolver_api_absent
IMPORT_ERROR=cannot import name 'resolve_agent_profile' from 'openhands.sdk.profiles' (.../profiles/__init__.py)
This confirms the base branch has no user-facing SDK join point for resolving agent profiles into executable settings.
Step 2 — Apply the PR's changes:
Checked out agent-profile-resolver at 0a906ad3ae3970782bf9c3b97dc2f52200f3ca97.
Step 3 — Re-run with the PR in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY' ... create LLMProfileStore, MCPConfig, OpenHandsAgentProfile, ACPAgentProfile; call resolve_agent_profile()/resolve_agent_profile_dry_run(); call settings.create_agent() ... PY:
OPENHANDS_SETTINGS_TYPE=OpenHandsAgentSettings
OPENHANDS_CREATE_AGENT_TYPE=Agent
OPENHANDS_LLM_MODEL=gpt-4o
OPENHANDS_API_KEY_SET=True
OPENHANDS_MCP_SERVERS=fetch
OPENHANDS_SUFFIX=QA suffix
MCP_NULL_REFS=fetch,shell
MCP_EMPTY_REFS_NONE=True
DANGLING_MCP_RAISED=missing
DRY_RUN_VALID=True
DRY_RUN_RESOLVED_MCP=fetch
DRY_RUN_SETTINGS_PRESENT=True
DRY_RUN_REDACTED_LLM_SECRET=True
DRY_RUN_REDACTED_MCP_SECRET=True
BAD_DRY_RUN_VALID=False
BAD_DRY_RUN_ERRORS=2
ACP_SETTINGS_TYPE=ACPAgentSettings
ACP_CREATE_AGENT_TYPE=ACPAgent
ACP_COMMAND=codex-acp --foo
ACP_ARGS=--bar
ACP_MCP_SERVERS=shell
ACP_ENV_EMPTY=True
ACP_LLM_API_KEY_NONE=True
ACP_DRY_RUN_VALID=True
ACP_API_KEY_SECRET=OPENAI_API_KEY
ACP_BASE_URL_SECRET=OPENAI_BASE_URL
ACP_FILE_SECRETS=CODEX_AUTH_JSON
This shows the resolver produces validated executable settings, preserves the unchanged create_agent() path, applies the documented MCP None/empty/filter semantics, reports dangling refs, redacts dry-run secrets, and keeps ACP credentials out of deprecated acp_env/llm.api_key channels.
Test 2: Cipher-backed skills[].mcp_tools secret flow
Step 1 — Establish baseline:
The base branch lacks resolve_agent_profile, so there is no resolver path to decrypt skills[].mcp_tools ciphertext for execution.
Step 2 — Apply the PR's changes:
Used the PR checkout and saved an encrypted LLM profile plus an encrypted AgentProfileStore profile containing a skill with MCP env and headers secrets.
Step 3 — Exercise the PR behavior:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY' ... save encrypted stores; load profile; resolve with cipher; inspect execution settings and dry-run dump ... PY:
LOADED_SKILL_ENV_IS_CIPHERTEXT=True
LOADED_SKILL_HEADER_IS_CIPHERTEXT=True
LOADED_SKILL_NO_PLAINTEXT=True
RESOLVED_LLM_API_KEY_SET=True
RESOLVED_SKILL_ENV_PLAINTEXT=True
RESOLVED_SKILL_HEADER_PLAINTEXT=True
RESOLVED_CREATE_AGENT_TYPE=Agent
DRY_RUN_VALID=True
DRY_RUN_SKILL_SECRET_REDACTED=True
This confirms the profile remains secret-free at rest after load, while the resolver decrypts the skill MCP secrets for execution and dry-run output remains redacted.
Test 3: Resolver error behavior
Step 1 — Establish baseline:
On main, the strict/dry-run resolver APIs are absent, so callers cannot receive resolver-specific ProfileNotFound/diagnostic behavior.
Step 2 — Apply the PR's changes:
Used the PR checkout and created real temporary stores/profiles with missing references and an invalid custom ACP profile.
Step 3 — Exercise error paths:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY' ... resolve missing LLM and custom ACP without command ... PY:
MISSING_LLM_RAISED=ProfileNotFound
CUSTOM_ACP_NO_COMMAND_DRY_RUN_VALID=False
CUSTOM_ACP_NO_COMMAND_SETTINGS_PRESENT=False
CUSTOM_ACP_NO_COMMAND_STRICT_RAISED=ValueError
This confirms strict resolution raises useful failures and dry-run converts invalid materialization into diagnostics instead of producing unusable settings.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Taste Rating: Good taste - the resolver keeps the profile/reference model separate from the executable settings model, with straightforward MCP filtering and secret redaction boundaries.
I reviewed the three changed files and did not find blocking code issues. There are no unresolved review threads to avoid duplicating. I also ran uv run pytest tests/sdk/profiles/test_resolver.py -q locally and got 18 passed.
Note: I am submitting this as a COMMENT rather than APPROVE because the PR is still marked draft and several PR checks are currently pending (for example QA/coverage and agent-server build jobs). Once the author marks it ready and CI is green, the code itself looks ready from this review.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This introduces a new resolver for profile → settings materialization, including LLM/MCP secret handling and ACP launch settings. The change is well-contained and has focused tests covering valid, dangling, dry-run, redaction, and cipher-backed paths, but it sits on a configuration/credential boundary so regressions would be user-visible when the follow-up server routes start consuming it.
VERDICT:
✅ Worth merging once draft status is cleared and CI completes: core logic is sound; no blocking code changes requested.
KEY INSIGHT:
The strongest part of this PR is the clean data boundary: profiles remain reference-only and secret-free, while the resolver is the single join point that materializes executable AgentSettingsConfig.
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
Verified the new SDK profile resolver end-to-end through real uv run python usage against temporary LLM/profile stores; the PR branch resolves OpenHands and ACP profiles into executable settings where main has no resolver API.
Does this PR achieve its stated goal?
Yes. The stated goal is to add the Phase-1 SDK join point that turns AgentProfile references into validated AgentSettingsConfig without changing create_agent; the exercised script imported the exported resolver API, resolved an OpenHands profile with LLM + filtered MCP config into OpenHandsAgentSettings, resolved an ACP profile into ACPAgentSettings without injected credentials, and successfully called create_agent() for both. It also verified dangling LLM/MCP errors, cipher-backed skills[].mcp_tools decryption, and dry-run redaction/diagnostics.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully (MAKE_BUILD_RC=0). |
| CI Status | ⏳ 22 pass, 7 pending Agent Server image/QA jobs, 1 skipped at final check. |
| Functional Verification | ✅ Direct SDK exercise passed on PR branch and failed on main because the API does not exist there. |
Functional Verification
Test 1: Resolver API exists only after the PR and supports the claimed OpenHands/ACP flows
Step 1 — Establish baseline without the feature:
Checked out origin/main at cceb86fa and ran uv run python /tmp/qa_agent_profile_resolver.py.
BASELINE_SCRIPT_RC=1
Traceback (most recent call last):
File "/tmp/qa_agent_profile_resolver.py", line 11, in <module>
from openhands.sdk.profiles import (
ImportError: cannot import name 'DanglingMcpServerRef' from 'openhands.sdk.profiles'
This confirms the base branch does not expose the resolver symbols, so the feature entry point is new in this PR.
Step 2 — Apply the PR's changes:
Checked out agent-profile-resolver at dd58f47c2973cfaf69f0e09bd4ad41d37dbe320e.
Step 3 — Re-run with the PR in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_agent_profile_resolver.py.
{
"acp_resolve": {
"acp_env_empty": true,
"agent_type": "ACPAgent",
"args": ["--verbose"],
"codex_api_key_secret": "OPENAI_API_KEY",
"codex_base_url_secret": "OPENAI_BASE_URL",
"codex_file_secrets": ["CODEX_AUTH_JSON"],
"command": ["codex-acp", "--project", "/tmp/work"],
"custom_without_command_valid": false,
"llm_api_key_is_none": true,
"mcp_servers": ["shell"],
"settings_type": "ACPAgentSettings"
},
"dangling_mcp": {"error_type": "DanglingMcpServerRef", "missing": ["missing"]},
"dry_run": {
"invalid_errors": 2,
"invalid_has_settings": false,
"llm_api_key_set": true,
"llm_profile_resolved": true,
"redacted_llm_secret": true,
"redacted_mcp_secret": true,
"resolved_mcp_servers": ["fetch"],
"valid": true
},
"missing_llm": {"error_type": "ProfileNotFound", "message_contains_ref": true},
"openhands_resolve": {
"agent_type": "Agent",
"api_key_loaded": true,
"mcp_servers": ["fetch"],
"settings_type": "OpenHandsAgentSettings"
},
"skill_mcp_secret_roundtrip": {
"decrypted_env_matches": true,
"decrypted_header_matches": true,
"stored_ciphertext_prefix": "gAAAAA"
}
}This shows the new resolver API works for realistic SDK usage: stores are created on disk, an LLM profile is saved/loaded, MCP refs are filtered, missing refs raise the new exceptions, dry-run returns redacted diagnostics, encrypted skill MCP secrets are decrypted for execution, and the resolved settings feed the existing create_agent() path for both OpenHands and ACP variants.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟡 Acceptable — Mostly clean and well-scoped, but the dry-run's "without raising" contract is not fully enforced (one real gap remains from the earlier "make dry-run total" iteration).
What I like
- The strict / dry-run split is exactly the right shape for a router:
resolve_agent_profile()raises for the imperative path,resolve_agent_profile_dry_run()returnsAgentProfileDiagnosticsfor the editor preview (#3719). Clean separation. - Helpers are small, named, and documented:
_compute_mcp_filter,_decrypt_skill_mcp_tools,_build_openhands_settings,_build_acp_settings,_acp_credential_channels. The docstrings explain why (e.g., the cipher dance aroundSkill.mcp_tools) rather than narrating the diff. - Secret hygiene in the dry-run is correct:
settings.model_dump(mode="json")without expose context routes throughserialize_mcp_config→sanitize_dict, which redactsenv/headersand the LLMapi_key. The test fixture confirms this end-to-end. - The ACP path injects no credential (relies on
state.secret_registry←request.secretsfrom #3720) — the deprecatedacp_env/llm.api_keychannels stay empty. Good. - Tests exercise real code paths against real profile/cipher/MCP fixtures, not mocks. Coverage spans the union variants, null/empty/filter/dangling MCP, dangling-LLM hard error, decryption round-trip, dry-run redaction, ACP custom-command invariant, and dry-run settings-build failure.
- Does not break
create_agent/apply_agent_settings_diff/validate_agent_settings— the resolver produces anAgentSettingsConfig, the existing consumers stay unchanged. Matches the epic's "minimal join point" goal.
What's left
I posted inline comments for the two items below; both are small. The Important one is a real contract gap in the dry-run that the earlier "make dry-run total" iteration missed.
Risk Assessment
- Overall PR: 🟢 LOW. New module, additive exports, no changes to existing public API, well-tested, CI green, the prior review threads were all addressed.
Verdict
❌ Needs a small fix: one Important (dry-run LLM load is not total) + one Suggestion (the assert will be stripped under python -O). Both are 1–2 line changes plus optional regression tests.
Key Insight
The dry-run's "total" contract is the whole point of having a separate diagnostics-returning function — the editor preview path in #3719 must not crash on a transient lock contention, because that's exactly when the user most needs to see "your profile is still valid, we just can't verify it right now."
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 complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Address PR review:
- dry-run LLM load now catches any store exception (filelock.TimeoutError from
lock contention, OSError, validation errors) and records it as a diagnostic
("Could not load ...", distinct from "not found") instead of propagating and
crashing the editor preview (#3719). Adds a regression test with a load that
raises TimeoutError.
- replace `assert llm is not None` (stripped under python -O) with an explicit
guard so a None LLM can never reach _build_openhands_settings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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
🟢 Good taste — The resolver is elegantly designed and all issues from the prior CHANGES_REQUESTED review have been properly addressed.
What changed in this iteration
The two blocking items from the previous review are both resolved:
-
Dry-run LLM load is now total. The
except FileNotFoundErrorblock inresolve_agent_profile_dry_runis followed by a secondexcept Exception as ethat catchesfilelock.TimeoutError,OSError, and any other transient load failures, recording them as a distinctCould not load LLM profile …diagnostic. This fulfills the "without raising or side effects" contract in the docstring and ensures the editor preview (#3719) can never crash on lock contention. -
assertreplaced with an explicit guard.assert llm is not Noneis nowif llm is None: raise RuntimeError(...), wrapped inside the outertry/exceptso it records the failure as a diagnostic instead of propagating. This is correct underpython -Oand produces a meaningful error message rather than a crypticTypeErrordownstream.
Overall assessment
The architecture is exactly right:
resolve_agent_profile()raises on bad refs (router maps to 404/422)resolve_agent_profile_dry_run()returnsAgentProfileDiagnostics— total, no side effects, secrets redacted- The ACP path enforces the launch invariant for
customservers up front rather than deferring tocreate_agent() - Secret hygiene is sound: LLM key and MCP env/headers redacted via the standard
model_dump(mode="json")path; no plaintext at rest - Tests are real code paths with real stores, not mock-only assertions; the one mock (
llm_store.load = _boom) is the minimum viable way to simulate lock contention - Evidence in the PR description includes CI output (ruff, pyright, pytest), and an end-to-end walkthrough against real stores
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — New module, purely additive exports, no changes to existing public API surface (create_agent/apply_agent_settings_diff/validate_agent_settingsunchanged), CI green, comprehensive test coverage.
VERDICT:
✅ Worth merging: Both prior blocking issues are fixed. The code is clean, well-tested, and ready.
KEY INSIGHT:
The strict/dry-run split — raise on the imperative path, return diagnostics on the preview path — is the right shape for the router, and the dry-run's totality guarantee is now fully enforced.
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 AgentProfile resolver SDK entry points worked end-to-end with real profile stores, MCP configs, secret round-tripping, dry-run diagnostics, and unchanged create_agent() paths.
Does this PR achieve its stated goal?
Yes. The PR sets out to add the phase-1 SDK substrate that resolves AgentProfile references into executable AgentSettingsConfig; I verified that the symbols are absent on main, then on the PR branch resolve_agent_profile() successfully produced OpenHandsAgentSettings and ACPAgentSettings, loaded the LLM profile secret, filtered MCP servers, decrypted stored skill MCP secrets, surfaced dangling references, and returned real Agent / ACPAgent instances via create_agent().
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed; no tests/linters were run locally. |
| CI Status | ⏳ At check time: 20 successful, 9 pending, 3 skipped, 0 failing. |
| Functional Verification | ✅ SDK behavior exercised through real imports, stores, settings, and agent creation. |
Functional Verification
Test 1: Base branch lacks the resolver entry point
Step 1 — Establish baseline on main:
Ran git fetch origin main && git checkout main && uv run python /tmp/qa_agent_profile_resolver.py:
{"status": "IMPORT_ERROR", "error_type": "ImportError", "error": "cannot import name 'DanglingMcpServerRef' from 'openhands.sdk.profiles' (...)"}This confirms the base branch does not expose the new resolver API, so the PR is adding a new user-facing SDK entry point rather than preserving existing behavior.
Step 2 — Apply the PR's changes:
Checked out agent-profile-resolver at 0653f44bc6647f4f54951cd668137f32766846f5.
Step 3 — Re-run with the PR in place:
Ran uv run python /tmp/qa_agent_profile_resolver.py:
{
"status": "OK",
"openhands": {
"settings_type": "OpenHandsAgentSettings",
"agent_type": "Agent",
"llm_key_loaded": true,
"mcp_servers": ["fetch"],
"sub_agents": true,
"concurrency": 2
},
"mcp_composition": {
"null_refs_servers": ["fetch", "shell"],
"empty_refs_is_none": true
},
"dangling_mcp_error": {"type": "DanglingMcpServerRef", "missing": ["missing"]},
"missing_llm_error": {"type": "ProfileNotFound", "message_contains_ref": true},
"skill_mcp_secret_roundtrip": {
"stored_as_ciphertext": true,
"plaintext_absent_at_load": true,
"resolver_decrypted_env": true,
"resolver_decrypted_header": true
},
"acp": {
"settings_type": "ACPAgentSettings",
"agent_type": "ACPAgent",
"command": ["codex-acp", "--foo"],
"mcp_servers": ["shell"],
"acp_env_empty": true,
"llm_key_is_none": true
},
"dry_run": {
"valid_openhands": true,
"redacted_has_settings": true,
"llm_secret_redacted": true,
"mcp_secret_redacted": true,
"invalid_reports_errors": true,
"invalid_no_settings": true,
"acp_api_key_channel": "OPENAI_API_KEY",
"acp_base_url_channel": "OPENAI_BASE_URL",
"acp_file_channels": ["CODEX_AUTH_JSON"]
}
}This shows the OpenHands path resolves a stored LLM profile into executable settings and a real Agent; MCP null/empty/filter semantics behave as described; dangling LLM/MCP refs surface the new exceptions; cipher-backed skills[].mcp_tools are ciphertext at rest and plaintext after resolve; ACP settings are built without deprecated credential injection and produce a real ACPAgent; dry-run returns redacted settings and faithful invalid diagnostics.
Test 2: Custom ACP without command is rejected before launch
Ran uv run python - <<'PY' ... PY with an ACPAgentProfile(acp_server="custom") and calls to dry-run plus strict resolve. Observed:
{
"dry_run_has_error": true,
"dry_run_resolved_settings": null,
"dry_run_valid": false,
"strict_resolve_error": "ValueError"
}This confirms the resolver/dry-run verdict is honest for a custom ACP server with no launch command, instead of producing settings that would only fail later at conversation start.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
2 participants
HUMAN:
Phase-1 SDK substrate for the AgentProfile epic (#3713): wires the
profile→settings resolver so the agent-server router (#3719) and conversation
start (#3720) can compose profiles. Reviewed the secret-channel seams locally.
AGENT:
Why
resolve_agent_profile()is the only join point between the authoring layer(profiles carry references —
llm_profile_ref/mcp_server_refs— and aresecret-free at rest) and the execution layer (
AgentSettingsConfig, whichembeds the resolved
llm/mcp_config). Resolving references into a validatedAgentSettingsConfigkeepscreate_agent/apply_agent_settings_diff/validate_agent_settingscompletely unchanged (epic #3713). Both dependencies —the
AgentProfilemodel (#3715) andAgentProfileStore(#3716) — are already onmain; this is the next phase-1 building block.Summary
openhands-sdk/openhands/sdk/profiles/resolver.py:resolve_agent_profile(profile, *, llm_store, mcp_config, cipher)→ validatedAgentSettingsConfig.mcp_server_refs=Nonepasses the caller's already-decryptedmcp_configthrough unchanged; a non-null list filtersmcpServersto the named keys ([]→ none); a referenced name absent frommcp_configraisesDanglingMcpServerRefnaming the missing key.LLMProfileStore.load(name, cipher=)(missing →ProfileNotFound), decryptsskills[].mcp_toolsenv/headers ciphertext with the cipher (the decryptionAgentProfileStore.loaddefers to the resolver), and copies throughagent/skills/system_message_suffix/condenser/verification/enable_sub_agents/tool_concurrency_limit.acp_*+ filteredmcp_configand injects no credential — provider creds ridestate.secret_registry←request.secretswired at conversation-start ([AgentProfile][agent-server] Accept agent_profile_id at conversation start + stamp LaunchedProfile provenance #3720), never the deprecatedacp_envorllm.api_key.resolve_agent_profile_dry_run(...)→AgentProfileDiagnostics(pure, no side effects): resolved/dangling LLM + MCP refs, ACP provider credential channels split by role (acp_api_key_secret_name,acp_base_url_secret_name,acp_file_secret_names) fromACP_PROVIDERS— kept separate because the API key and file-content creds are alternative auth paths and the base URL is optional proxy routing, not jointly required, a valid/invalid verdict matching a real resolve, and redacted resolved settings (api_key_set+ the redact-mode dump). Consumed byPOST /{id}/materialize([AgentProfile][agent-server] active_agent_profile_id + /api/agent-profiles router + migration seed #3719) and the canvas editor.ProfileNotFound(→ 404) /DanglingMcpServerRef(→ 422) defined here for the router to import + map.openhands.sdk.profiles.tests/sdk/profiles/test_resolver.pycover both variants, the null/empty/filter/dangling MCP cases, the dangling-LLM hard error, the cipher-backedskills[].mcp_toolsdecryption, and the dry-run redaction + verdict-matches-real-resolve.Issue Number
#3717
How to Test
Ran the SDK's required checks against the new source + tests:
End-to-end (beyond unit tests), I exercised the full resolve path against real
stores to confirm the resolved settings feed the unchanged
create_agent:OpenHandsAgentSettingswith theconcrete decrypted
llm, MCP filtered to the referenced keys, andsettings.create_agent()returning a realAgent.ACPAgentSettingswithacp_*set,acp_command(a singlestring) tokenized into the settings'
list[str], no credential injected(
acp_env == {},llm.api_key is None), andcreate_agent()returning anACPAgent.skills[].mcp_toolsenv/headers as Fernet ciphertext (no plaintext at rest); after
AgentProfileStore.load(..., cipher=)the values are still ciphertext, andthe resolver decrypts them back to plaintext for execution.
resolved_settingswith the LLM key andMCP header masked (no secret in the JSON dump); dangling LLM + MCP →
valid=False, both errors recorded,resolved_settings is None; ACP codex →acp_api_key_secret_name="OPENAI_API_KEY",acp_file_secret_names=["CODEX_AUTH_JSON"](alternative auth),acp_base_url_secret_name="OPENAI_BASE_URL"(optional).Type
Notes
decrypt_mcp_config_secretsand passes an already-decryptedmcp_config.POST /{id}/materialize([AgentProfile][agent-server] active_agent_profile_id + /api/agent-profiles router + migration seed #3719,maps
ProfileNotFound→404 /DanglingMcpServerRef→422) and conversation-startcred wiring ([AgentProfile][agent-server] Accept agent_profile_id at conversation start + stamp LaunchedProfile provenance #3720).
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:0653f44-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0653f44-python) is a multi-arch manifest supporting both amd64 and arm640653f44-python-amd64) are also available if needed