feat: select agent profiles on automation definitions - #453
Conversation
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review Summary
ð�¡ Acceptable â�� Core design is sound, but one critical bug will break Cloud and profile-less local dispatch.
Critical Issue
backend.api_prefix raises AttributeError on non-conversation backends (openhands/automation/dispatcher.py, line 515)
The old code checked get_config().service.run_agent_profile � a config string that was truthy when a profile was configured. The replacement backend.api_prefix is a property defined only on ConversationAgentServerBackend. Neither CloudSandboxBackend nor LocalAgentServerBackend defines it, so accessing it raises AttributeError.
This line is reached after a successful execution (if result.success:). The exception propagates to _execute_run_safe's broad except Exception, which marks the run FAILED � despite the actual execution having succeeded. The conversation_id link is also skipped.
Impact: Every successful Cloud dispatch and every successful local-mode-without-profile dispatch will be incorrectly marked FAILED.
Fix: Use isinstance(backend, ConversationAgentServerBackend) instead, or add an api_prefix property to the ExecutionBackend base class returning "" by default.
Risk Assessment
- Overall PR: ð�¡ MEDIUM â�� The
AttributeErrorbug is a regression that breaks the primary dispatch path for Cloud and non-profile local runs. Once fixed, the rest of the change is well-structured: migration is correct, run snapshot logic is sound, validation properly rejects conflicting model+profile selections, git sync round-trip is tested, and the Docker compatibility backend removal is clean.
Verdict
� Needs rework � The backend.api_prefix AttributeError must be fixed before merge. All other changes look good.
Key Insight: The api_prefix property is conversation-backend-specific, but the guard that uses it runs for every backend type.
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.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR persists agent_profile_id on automation definitions and snapshots it on queued runs, replacing the previous host-controlled deployment-level profile override map with per-automation profile selection. The CRUD API, preset endpoints, git sync, run history, and capability discovery all carry the selection. The unreleased Docker-only compatibility backend and profile override settings are removed.
Analysis
Data model & migration: The migration (023) adds a nullable agent_profile_id Uuid column to both automations and automation_runs, using generic SQLAlchemy types for cross-database compatibility. The snapshot-on-queue pattern in create_pending_run correctly ensures that editing a definition affects only future runs, not already-queued ones. The fallback to the deployment default (AUTOMATION_AGENT_PROFILE) when the automation has no explicit profile is handled correctly.
Validation: validate_agent_profile_selection consistently rejects (a) profiles in non-local mode and (b) a conflicting model field. This is applied uniformly across the CRUD create, CRUD update, prompt preset, plugin preset, and validate-draft endpoints. The update path correctly uses the pending update_data value (or the existing value when only model is being changed) for validation.
Dispatcher concurrency: Changing the gate from run_agent_profile to is_local_mode means all local-mode deployments now enforce conversation_max_concurrent_runs, not just those with a profile configured. This is correct — LocalAgentServerBackend also dispatches to the agent server, which has resource constraints. Since the removed settings were unreleased, no existing deployment loses behavior it was relying on.
Preset runner changes: The load_provisioned_agent helper fetches the conversation from the agent server, validates the launched profile matches AUTOMATION_AGENT_PROFILE_ID, and returns the already-configured agent. When a provisioned agent is present, the runner skips get_llm, get_secrets, get_mcp_config, and get_default_agent — meaning the host secret store is never forwarded to the sandbox. This is a genuine security improvement.
Profile-change tarball refresh: When an existing preset automation's profile is changed, the tarball is rebuilt with the current runner files (refresh_runner=True) while preserving prompt and plugin/repo configuration. The _replace_prompt_in_tarball function handles this correctly by tracking seen members and appending any replacement files that weren't already present.
Watchdog: The cleanup and staleness checks correctly shifted from deployment-level (run_agent_profile) to per-run (run.agent_profile_id or settings.agent_profile) and per-mode (is_local_mode) checks, which is more precise.
Risk Assessment
🟡 MEDIUM — The PR removes unreleased configuration options (AUTOMATION_DOCKER_AGENT_PROFILE, AUTOMATION_AGENT_PROFILE_OVERRIDES, docker_max_concurrent_runs) and the DockerAgentServerBackend compatibility class. Since these were unreleased, this is safe. The dispatcher concurrency behavior change for local-mode deployments without profiles is a behavioral shift but is correct. The SDK dependency bump (79021c6 → 85b8bc7) is a first-party package from the same organization and is excluded from the 7-day waiting rule, but the PR description notes a live factory rollout is in progress, which serves as real-world validation.
Verdict
✅ Worth merging — The design is sound, the data model is clean, validation is consistent across all entry points, and the preset runner changes improve security by avoiding host secret forwarding. Test coverage is thorough for the new behavior. No material issues found.
Improve this review? If any feedback above seems incorrect or irrelevant for 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.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR moves agent_profile_id from a global config setting (AUTOMATION_AGENT_PROFILE) to a per-automation and per-run field, snapshots it on queued runs, and carries it through CRUD, presets, git sync, history, and capabilities. The Agent Server resolves the profile at dispatch time. Profile-scoped workers use runtime polling for completion instead of receiving the service-admin callback credential.
Analysis
Taste Rating: 🟢 Good taste
The core data structure change — moving profile selection from a deployment-wide config to per-automation/per-run columns — is the right abstraction. Snapshotting on queued runs so edits only affect future runs is correct. The security boundary is sound: ConversationAgentServerBackend.build_env_vars() deliberately omits AUTOMATION_CALLBACK_API_KEY, and the dispatcher's callback suppression condition correctly prevents the service-admin key from reaching profile-scoped workers.
Key improvements verified:
- Fixes a provisioning bug:
_resolve_agent_serverandfetch_latest_finish_tool_response_for_runnow useisinstance(backend, LocalAgentServerBackend)and readagent_server_url/get_api_key()directly instead of callingget_execution_context(), which would have provisioned a new conversation just to send a turn or read an outcome to an existing one. - Watchdog broadened: Stale-run detection now covers all local-mode runs (
settings.is_local_mode) rather than only profile-scoped ones, fixing a gap where non-profile local runs weren't being watchdogged. - Callback credential boundary: The condition
env_vars.get("AUTOMATION_CALLBACK_API_KEY") or env_vars.get("OPENHANDS_API_KEY") or not settings.local_api_keycorrectly suppresses callbacks for profile-scoped workers (no callback key, no OPENHANDS_API_KEY, local_api_key exists) while preserving the old unauthenticated-callback behavior when no local_api_key is configured. - Conversation ID linking:
ctx.runtime_conversation_id is not Noneis semantically equivalent to the oldget_config().service.agent_profilecheck, since onlyConversationAgentServerBackendsets it. - Preset runner refresh: Changing an automation's profile correctly triggers tarball regeneration with the updated runner, preserving prompt and plugin/repo config.
No material findings. The acknowledged prerequisites (SDK #5010 release, migration number reconciliation, FinishTool hook limitation tracked in #457) are appropriately documented in the PR description and are not code-level issues.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
The change is well-scoped, backward-compatible (nullable columns,nulluses deployment default), and the security boundary is deliberate and tested. The main external dependency is the unreleased SDK #5010, which is explicitly gated in the PR description.
VERDICT: ✅ Worth merging
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Posted an APPROVED review on PR #453 (OpenHands/automation) at commit a9cc2ad. Verdict: ✅ Worth merging — 🟢 LOW risk The PR moves
Review URL: #453 (review) This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Overview
This PR moves agent profile selection from a global deployment setting (AUTOMATION_AGENT_PROFILE) to a per-automation agent_profile_id field, snapshotted on each queued run. The design is clean: the definition owns the selection, the run retains it, and the Agent Server resolves the profile at dispatch. The migration from raw HTTP calls to SDK-typed RemoteConversation.attach() for outcome reads and turn delivery is a solid simplification.
Analysis
Data structure and flow — Good taste. The per-run snapshot on AutomationRun correctly isolates queued runs from definition edits. The validate_agent_profile_selection helper is consistently applied across all four creation/update paths (raw CRUD, prompt preset, plugin preset, validate endpoint). The mutual exclusion of model and agent_profile_id is enforced at the right layer.
Security posture — Sound. ConversationAgentServerBackend.build_env_vars() deliberately omits AUTOMATION_CALLBACK_API_KEY, so the service-admin key is never exposed to a profile-scoped worker. The dispatcher's callback-URL gating condition correctly withholds callback/phase URLs when no callback credential is available and a service key exists, falling back to watchdog polling. The isinstance(backend, LocalAgentServerBackend) checks in conversation_turn.py and conversation_outcome.py ensure that reading outcomes and sending turns never provision a new conversation — a subtle but important correctness property.
Dispatcher polling — The change from settings.agent_profile to settings.is_local_mode for active-run tracking is correct. Previously, non-profile local runs weren't counted toward conversation_max_concurrent_runs; now all local-mode runs are. This is a behavior change but the right one.
Watchdog — Both the cleanup condition (run.agent_profile_id instead of settings.agent_profile) and the staleness query (settings.is_local_mode instead of settings.agent_profile) correctly move from global to per-run/local-mode checks.
Known limitation — Profile-created preset conversations don't receive the FinishTool enforcement hook because RemoteConversation.attach() doesn't accept hook_config. This is explicitly acknowledged in the PR description and tracked in #457. No live factory failure has been attributed to this gap.
Testing
Comprehensive test coverage: profile round-trip and queued-run snapshot, preset runner refresh on profile change, restricted backend credential withholding, SDK-based outcome/turn delivery with proper 404 retry semantics, and git sync round-trip. The test migration from httpx.MockTransport to sdk_http_transport for conversation operations is appropriate given the SDK migration.
Risk Assessment
🟢 LOW — The change is well-scoped, backward-compatible (null agent_profile_id preserves existing behavior), and properly gated to local mode. The migration is cross-database compatible. The SDK dependency is noted as release-gated (SDK #5010), and the migration number reconciliation with main is flagged as a pre-merge task.
Verdict: ✅ Worth merging
Key insight: Moving profile selection from a global deployment setting to a per-automation field with run-level snapshotting is the correct data model — it gives each automation independent control over its agent configuration while preserving run isolation.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
GITHUB_REVIEW_POSTED This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: feat: select agent profiles on automation definitions
Taste Rating: 🟢 Good taste
This PR cleanly migrates from a global AUTOMATION_AGENT_PROFILE config setting to per-automation agent_profile_id selection, with run-level snapshotting so queued runs retain their profile even after definition edits. The design is sound across all touched layers: CRUD, presets, git sync, dispatcher, watchdog, and capabilities.
Key strengths
-
Snapshot-on-queue pattern (
utils/run.py): Copyingagent_profile_idfrom the automation to the run at creation time is the right data-structure choice. It eliminates the race between editing a definition and dispatching a queued run, with no special-case handling needed. -
Preset runner refresh (
router.py+preset_router.py): When a profile is added to or removed from an existing preset automation, the tarball is rebuilt with the current runner via_replace_prompt_in_tarballwithrefresh_runner=True. The runner detects provisioned vs. non-provisioned mode at runtime viaAUTOMATION_AGENT_PROFILE_ID, so the same code handles both paths — no separate profile vs. non-profile runner variants. -
Validation consistency (
model_profiles.py):validate_agent_profile_selectionis called at every creation/update path (CRUD, prompt preset, plugin preset, capabilities preflight). Rejectingmodelalongside a profile and rejecting profiles without a configured Agent Server are both enforced uniformly. -
Dispatcher concurrency (
dispatcher.py): Gating the active-run concurrency limit onis_local_modeinstead of the old globalagent_profileis correct — since profiles are now per-automation, the dispatcher can'''t know at poll time which runs have profiles without loading them, so applying the limit to all local-mode runs is the safe choice. -
Watchdog (
watchdog.py): Per-runagent_profile_idcheck for sandbox cleanup andis_local_modefor staleness scanning correctly replace the old global setting. -
Git sync round-trip: Serializer and loop both handle
agent_profile_idwith proper None guards. The new test verifies UUID round-trip fidelity. -
Tests: New tests exercise real code paths — profile creation/round-trip, queued-run snapshot retention, preset runner refresh with tarball content assertions, backend selection, and credential handoff failure recovery. Tests that could run without Docker passed (3/5); the 2 router tests require testcontainers Docker which is unavailable in this review environment.
Known limitations (documented in PR)
- Profile-created preset conversations don'''t yet receive the FinishTool enforcement hook (#457). The
conversation_kwargsdict (includinghook_configandtags) is built but unused in theRemoteConversation.attach()path. This is acknowledged and tracked separately. - Migration revision
023needs reconciliation with main if #456 lands first. - SDK dependency on software-agent-sdk#5010 must be released before merge (temporary source pin to be replaced).
None of these are blocking for this PR'''s code quality — they are upstream/merge-sequencing concerns explicitly called out in the PR description.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- The change adds a nullable column to two tables (no data migration needed).
- Validation is enforced at all entry points.
- The per-run snapshot eliminates edit/dispatch races.
- Cloud dispatch is explicitly rejected for profiles (no silent fallback).
- No secrets, tokens, or credential values are introduced or exposed by this PR — profile resolution is delegated to the Agent Server.
VERDICT: ✅ Worth merging
KEY INSIGHT: Snapshotting agent_profile_id on queued runs rather than reading it from the parent automation at dispatch time is the single design decision that makes the entire feature race-free — every other layer is a straightforward propagation of that 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.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Code review for PR #453 (feat: select agent profiles on automation definitions) has been posted to GitHub as APPROVED. Verdict: ✅ Worth merging — 🟢 LOW risk The PR cleanly migrates from a global
Review submitted as APPROVED per the repo's custom code review guidelines, which require approval when the verdict is "Worth merging" with LOW risk and no critical issues. This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR moves agent profile selection from a global deployment setting (AUTOMATION_AGENT_PROFILE) to a per-automation agent_profile_id field, snapshotted on each queued run. The profile is carried through CRUD, presets, git sync, history, and capabilities. The Agent Server resolves the profile at dispatch. The design is sound, the migration is clean and cross-database compatible, and validation is consistently applied across all creation paths (API, presets, git sync, draft validation).
Analysis
Taste Rating: 🟢 Good taste
The core data model change — snapshotting agent_profile_id on AutomationRun at queue time so definition edits affect only future runs — is the right approach. Moving from a global setting to per-automation selection eliminates the implicit coupling between deployment config and individual automation behavior.
Key design decisions verified:
- Migration (023): Uses generic
sa.Uuid(), correct upgrade/downgrade ordering (downgrade reverses table order). Cross-database compatible. - Backend selection (
backends/__init__.py): Per-runagent_profile_iddrives backend type. Non-local mode with a profile raisesValueError— correctly rejected at validation time (create/update/preset), with the backend check as a defense-in-depth fallback. - Dispatcher concurrency: Changed from
settings.agent_profiletosettings.is_local_mode. This correctly broadens throttling to all local-mode runs (includingLocalAgentServerBackendwithout a profile), since they all share the same agent server. - Watchdog: Staleness check now uses
settings.is_local_modeand cleanup usesrun.agent_profile_id. Correct — thebash_command_idcheck should apply to all local runs, and cleanup should be per-run. - Preset runner refresh: When a profile is added/changed on a preset automation, the runner tarball is regenerated with
refresh_runner=True, upgradingmain.pyto the version that usesRemoteConversation.attach. Good design — prevents stale runners from executing with a profile they were not built for. - Git sync validation: Profile/model conflicts are rejected on import using the same
validate_agent_profile_selectionused by the API. Consistent enforcement. - Profile path in sdk_main.py:
agent = None,secrets = {}— the server already resolved everything. Repos are passed as context text rather than cloned host-side, which is correct since the provisioned conversation runs under the profile credentials, not the host. - Security: Profile IDs are UUIDs with no injection surface. The profile path correctly avoids forwarding host secrets. Non-local deployments are rejected.
Known limitation (acknowledged in PR description):
The FinishTool enforcement hook is not passed to RemoteConversation.attach in the profile path. This is tracked in #457 and acknowledged in the PR description. Not a blocker for this PR.
Testing
42 focused tests pass (2 Docker-dependent tests errored on missing Docker daemon, unrelated to PR changes). New tests cover:
- Profile round-trip and queued-run snapshot isolation
- Preset runner refresh on profile change
- Backend selection with/without explicit profile
- Git sync import validation (accepted and rejected)
- Serializer round-trip for profile references
Risk Assessment
🟢 LOW — The change is additive (nullable column, opt-in field). Existing automations without agent_profile_id behave exactly as before. The global agent_profile setting is removed, but it was only used in local mode and the per-run field supersedes it. The preset runner refresh is the most complex new path, but it is well-tested and the prompt.txt preservation logic is sound.
Verdict
✅ Worth merging — Clean data model, consistent validation across all entry points, good test coverage, no material issues found.
Key insight: Snapshotting the profile ID on the run at queue time is the correct invariant — it ensures definition edits never silently change the behavior of an already-queued run.
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 comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds agent_profile_id to automation definitions and queued-run snapshots, carrying it through CRUD, presets, git sync, capabilities, and the watchdog. The design is clean: validation is centralized in validate_agent_profile_selection, the snapshot-on-queue pattern in create_pending_run is correct, and the preset runner scripts have a well-structured dual path (provisioned-conversation attach vs. legacy create).
Findings
No material issues found. A few observations:
-
Dispatcher/watchdog scope change is correct: The switch from
settings.agent_profiletosettings.is_local_modeindispatcher.pyandwatchdog.pybroadens the concurrency-limiting and staleness-scan logic to cover all local-mode runs, not just profile-based ones. This is the right call — local mode always uses the conversation backend pattern, so the concurrency limit and broader scan condition apply regardless of whether an individual run has a profile. -
PATCH clearing without model restoration: In
update_automation, whenagent_profile_idis explicitly set tonullwithout also providing amodel, the existingmodel=None(set when the profile was attached) stays as-is rather than being resolved to the user's active default. This is a minor design choice, not a bug —model=Nonemeans "use deployment default," which is functionally equivalent in most cases. The test suite confirms the profile-clearing path works. -
Git sync import consistency: The git import path in
_validate_and_resolve_fieldsreusesvalidate_agent_profile_selectionbefore resolving the tarball, which correctly rejects conflictingmodel+agent_profile_idcombinations without side effects. Themodelvalue from YAML is stored directly withoutresolve_model_profile_for_user— this is pre-existing behavior, not a regression. -
Security posture: The validation rejects profiles in cloud mode (preventing privilege escalation through a more privileged agent), and the README documents that missing profiles fail rather than falling back. The preset runner scripts correctly skip loading secrets/LLM/MCP when attaching to a provisioned conversation, avoiding secret-scope leakage.
-
Migration: Simple nullable
sa.Uuid()column addition to bothautomationsandautomation_runstables. Cross-database compatible.
Risk Assessment
🟢 LOW — The change is well-contained. Profile selection is validated consistently across all entry points. The snapshot pattern ensures queued runs are unaffected by definition edits. Tests cover round-trip creation, queued-run snapshots, preset runner refresh, git import validation, and credential handoff failure.
Verdict
✅ Worth merging — Core logic is sound, validation is consistent across all paths, and the security model is correct.
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 with the/codereviewtrigger and context the reviewer is missing.- Re-request a review — the reviewer reads guidelines from the PR branch, so changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review.
Was this review helpful? React with 👍 or 👎 to give feedback.
|
@all-hands-bot please review the current head after restacking on the SDK pin refresh and explicitly approve it if there are no material findings. |
Code Review � PR #453 (current head
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Reviewed head e17c670 (post SDK pin restack). No material findings. See the detailed review comment for the full assessment. Verdict: APPROVED, risk LOW.
Generated by OpenHands AI on behalf of the user.
Co-authored-by: openhands <openhands@all-hands.dev>
e17c670 to
4c6ef57
Compare
3 participants
Why
Automations need to select a saved agent profile so its model, tools, and allowed secrets apply consistently to each run.
Summary
Persist
agent_profile_idon definitions and queued-run snapshots, carrying it through CRUD, presets, git sync, history, and capabilities. The Agent Server resolves the profile. Reject an explicit model alongside a profile and reject unsupported Cloud dispatch.Ordinary and ACP presets attach with the explicit
RemoteConversation.attach(workspace, conversation_id)method; there is no separate profile deserializer. Restricted workers use existing runtime polling for completion without the service-admin credential. Runtime identity, attachment, threaded follow-ups and outcome retrieval are inherited from #449; this PR adds profile selection, persistence and preset integration.Issue Number
Implements #273. Canvas selector: OpenHands/OpenHands#17396.
How to Test
The current migration preserves the focused runtime, execution, local-mode, follow-up, and raw FinishTool-result checks. SDK HTTP tests verify buffered messages do not start a run, active follow-ups do, and missing conversations retain bounded retry behavior. All commit hooks pass. Earlier live validation completed the same canonical SDK bundle in local and Docker workspaces, with the selected saved profile creating each conversation and the worker attaching through the then-current constructor. The explicit create/attach update passes 31 backend, follow-up, and outcome tests. Refreshed live validation passed with the explicit API in local and Docker workspaces, including canonical profile creation and worker attachment. The older queue/credential recording remains separate historical evidence. The preset PyPI installer remains release-gated until SDK #5010 is published.
Review order and dependencies
Native stack #454: #449 → #453. #451 was consolidated into #449, which owns the shared SDK backend. Depends on SDK #5010 including explicit typed-request creation and existing-conversation attachment through
RemoteConversation.create/.attach; replace the temporary source pin with a released SDK before merging. Reconcile the profile migration number with main before merge; if independent legacy-database repair #456 lands first, this migration must follow its revision.Live Agent Canvas evidence
After moving subject-ID support into #449, the assembled #453 production tree remains
9de630f76614a6a26ce8a9088e7907bab1865f54, identical to the published live recording. Tests additionally verify persisted subject-derived conversation metadata. The two subject-keyed backend cases fail before the parent fix and pass afterward.Current explicit API: live local/Docker demonstration · Exact revisions, bundle, results and limits. The identical uploaded bundle completed a real agent task with
RemoteConversation.create/.attachin both runtimes, exited 0, and produced the expected file in Canvas. The report retains the initial request-validation failure and successful rerun after the SDK serialization fix. All private workers and services were released afterward. This validates the explicit API migration; earlier admission, queued-profile and credential-scope recordings remain separate.Earlier SDK migration: 24-second local/Docker demonstration · Exact revisions, executed bundle, results and limits. Both runs completed a real DeepSeek task, wrote and independently read back the expected file, and appeared as Successful in Canvas. The identical bundle and SDK source hashes were verified in both workers; all private runtimes were released afterward.
This earlier recording validates selected-profile creation and the preceding constructor attachment API; it does not repeat the earlier queued-profile snapshot or credential-scope matrix. Earlier evidence follows:
Animated recording · Scenario, exact revisions and limits · Allowlisted observations. Direct integrated enhancement demonstration with a real agent and synthetic secrets; the report distinguishes the owning PR from companion SDK/Automation/Canvas changes.
Merge and release prerequisites
Native predecessor: automation#449; inherit its SDK release/pin gates, including software-agent-sdk#5010. Factory secret enforcement additionally requires software-agent-sdk#4931 / software-agent-sdk#5017: profile selection alone is not the secret boundary. The profile migration is revision
024, following main’s org-scoped Git-sync revision023. The separate legacy-database repair #456 is not a fresh-install factory prerequisite; reconcile later migration additions again at merge time.Known pre-existing limitation: profile-created preset conversations do not yet receive the preset FinishTool enforcement hook. Automation #457 tracks the correction; no live factory failure has been attributed to this gap. This migration does not claim to install hooks when attaching to an existing conversation.
Final split audit
Git imports now reuse API profile validation before resolving the bundle: conflicting
modelandagent_profile_idvalues are rejected without updating the definition or uploads. A real Git/SQLite regression covers accepted and rejected imports. Git-ownedtarball/runner files are preserved; README documents upgrading an older preset runner before enabling a profile through Git, or using the API to refresh generated runners.Validation: 156 focused Git-sync, serialization, backend, turn-delivery, and outcome tests pass against SDK #5010 head
e9392e7cf35fcb011acf2eca825a80f9a22774ea; formatting, lint, style, and Pyright pass. The new Git validation is covered by tests, not a new Canvas recording. Runtime code remains byte-identical to the published explicit-API live proof; onlygit_sync/loop.pychanges within the assembled production tree.Current-base refresh
Restacked on current main through #449 (
9b4ede3), preserving organization-scoped Git import ownership and existing profile/model validation. The profile migration now follows revision 023 as revision 024. Validation: 156 focused backend/conversation/Git-sync tests, the SQLite migration regression (including both profile columns), focused Pyright, and formatting/lint pass. This refresh does not change runtime behavior; the existing live evidence retains its recorded revisions.