Repository navigation
feat: select an agent profile for delegated automation work - #479
Conversation
|
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 please review this PR and approve it if no blocking findings remain. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
PR Review: feat: select an agent profile for delegated automation workI've reviewed the full diff (17 files, +250/�8) and run the tests that don't require Docker in this environment. Here's my assessment. SummaryThe PR adds an optional Detailed ReviewMigration ( Models ( Validation (
The lazy Router ( Preset router ( Git sync ( Dispatcher ( Run creation ( Capabilities router ( Schemas ( Tests � Good coverage across all new paths:
Test ResultsI ran VerdictApproved. No blocking findings. The implementation is clean, follows existing patterns consistently across all entry points, enforces mutual exclusivity correctly, and has thorough test coverage. The snapshot-on-queue semantics are sound, and the migration is safe (nullable column, reversible). Generated by OpenHands AI on behalf of the user. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Approved � no blocking findings. See the detailed review comment above.
|
@all-hands-bot please review the current head after the source-scoped secret lookup refactor. If there are no blocking findings, please explicitly approve this PR rather than leaving only a comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Re-review: source-scoped secret lookup refactor (head c969997)
I reviewed the current head, focusing on the latest commit feat(profiles): scope automation secrets at lookup, and re-verified the full 3-commit diff against main.
What the refactor changes
Both preset entrypoints (presets/prompt/sdk_main.py and presets/plugin/sdk_main.py) now pass the selected profile ID to workspace.get_secrets():
secrets = workspace.get_secrets(
agent_profile_id=os.environ.get("AUTOMATION_AGENT_PROFILE_ID")
)When no profile is selected, os.environ.get() returns None, so the call is get_secrets(agent_profile_id=None) — fully backward compatible. The existing try/except wrapper means a mismatch with an older SDK that doesn't yet accept the argument degrades to a soft "no secrets" warning rather than a hard crash. os is already imported in both files.
Full PR assessment (unchanged from prior review, re-verified)
- Migration
025: Nullable UUID column onautomationsandautomation_runs; downgrade drops in reverse order. ✓ - Models:
agent_profile_idon both the definition and the run snapshot, with clear comments. ✓ - Validation (
validate_agent_profile_selection): No-op whenNone, requires local mode, rejects profile + model combination. Lazyget_configimport avoids circular dependency. ✓ - Router: Create nulls model when a profile is selected; update path correctly handles replace, clear, and model-only patches using
update_data.get("agent_profile_id", auto.agent_profile_id). ✓ - Preset router: Both prompt and plugin paths validate-then-null-model; plugin path additionally rejects profiles + experiment variants (422). ✓
- Git sync: Parses UUID from YAML, validates via the shared helper, converts
HTTPExceptiontoValueError(consistent with all other field validation). Serializer round-trips as a string. ✓ - Dispatcher: Exports
AUTOMATION_AGENT_PROFILE_IDonly when the run has a profile. ✓ - Run creation: Both
create_pending_runandcreate_automation_runsnapshot the profile onto the run. ✓ - Capabilities router:
agentProfilesfeature flag in local mode; profile validated in draft preflight. ✓ - Schemas:
agent_profile_idon all relevant request/response models. ✓
Test results
uv run pytest tests/test_preset_router.py tests/test_git_sync_serializer.py tests/test_db.py -q — 173 passed, 41 skipped. The test_router errors are all docker.errors.DockerException (no Docker socket in this environment), a pre-existing limitation unrelated to this PR. The PR description confirms 217 tests pass in CI with Docker available.
Verdict
Approved — no blocking findings. The source-scoped secret lookup refactor is a clean, minimal change that correctly threads the profile ID through to get_secrets() while preserving backward compatibility. The full PR is well-structured, consistently validated across all entry points, and thoroughly tested.
Generated by OpenHands AI on behalf of the user.
c969997 to
5f93288
Compare
| agent_profile_id: uuid.UUID | None = Field( | ||
| default=None, | ||
| description="Agent profile selected for this automation.", | ||
| ) | ||
|
|
| if not get_config().service.is_local_mode: | ||
| raise HTTPException(422, "Agent profiles require a configured Agent Server") |
There was a problem hiding this comment.
Is this correct? Profiles won't work in remote deployments?
| _build_event_payload(automation, run) | ||
| ) | ||
| if automation.agent_profile_id: | ||
| env_vars["AUTOMATION_AGENT_PROFILE_ID"] = str(automation.agent_profile_id) |
There was a problem hiding this comment.
NIT: followup would be to modify the sdk workspace to support workspace.get_agent_profile helper that will return the profile based off AUTOMATION_AGENT_PROFILE_ID
|
🚀 Released in 1.13.0. |
Why
An automation that delegates work to an agent needs to select the Agent Server profile for that agent. The profile is the existing policy boundary for its model, tools, MCP servers, and secrets. Automation should only carry the profile identifier; profile resolution remains in Agent Server and the SDK.
The automation command itself remains an ordinary host-side scanner. When it delegates work, the merged Extensions workers pass the selected identifier to
StartConversationRequest.agent_profile_id.What changed
agent_profile_idto automation definitions and their create, update, and response schemas.agentProfilesin capabilities when Automation is connected to an Agent Server.modeland an agent profile, because the profile owns model selection.AUTOMATION_AGENT_PROFILE_ID.The dispatcher reads the current automation definition, as it already does for
model; this PR adds no run snapshot, preset-specific profile handling, profile resolver, secret filtering, Git synchronization, or conversation lifecycle machinery.Closes #273.
Validation
uv run python -m pytest tests/test_db.py tests/test_dispatcher.py tests/test_router.py tests/test_capabilities_router.py -q— 233 passed.uv run pre-commit run --all-files --show-diff-on-failure— passed.Live evidence
A fresh Canvas selected profiles in the automation forms and retained them across reload. Scheduled developer and reviewer runs received the selected IDs, while an automation with no selected profile retained a null value. Six Docker developers ran concurrently, and Airbnb PR #87 completed development, review, and merge.