Conversation
|
🤖 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 selection to automations — creation, editing, details display, and import/export — reusing the existing useAgentProfiles hook and SettingsDropdownInput component. A selected profile takes precedence over the separate model field, and deleted profiles remain visible as unavailable rather than silently changing privileges. The implementation is clean: one shared selector component, consistent precedence logic across all code paths, and focused tests.
Acceptance Criteria Checklist (from #17395)
- Profile selection in automation creation (manifest-setup-dialog.tsx)
- Profile selection in automation editing (edit-automation-modal.tsx)
- Saved selection shown in details (configuration-section.tsx)
- Selection preserved through import/export (automation-export.ts)
- Capability discovery controls availability (
features.includes("agentProfiles")) - Selected profile suppresses the separate model field
- Deleted profiles remain visible as unavailable
Findings
CI-Blocking: Missing translations for new i18n keys
The two new translation keys AUTOMATIONS$DEFAULT_AGENT_PROFILE and AUTOMATIONS$UNAVAILABLE_AGENT_PROFILE only have English ("en") values. Every other key in translation.json has translations for all 15 supported languages. The check-translation-completeness script (run via lint-staged on every commit and as part of CI) explicitly fails on keys missing any supported language:
ERROR: Missing translations detected
- Key "AUTOMATIONS$DEFAULT_AGENT_PROFILE" is missing translations for: ja, zh-CN, zh-TW, ko-KR, no, ar, de, fr, it, pt, es, ca, tr, uk
- Key "AUTOMATIONS$UNAVAILABLE_AGENT_PROFILE" is missing translations for: ja, zh-CN, zh-TW, ko-KR, no, ar, de, fr, it, pt, es, ca, tr, uk
This will block both local commits (lint-staged) and CI. See the inline comment on translation.json for the exact location.
Risk Assessment
- Overall PR: 🟡 MEDIUM
- The translation completeness failure will block CI and should be fixed before merge.
- The core logic (profile precedence over model, deleted-profile retention, import/export round-trip) is sound and well-tested.
- No security concerns — profile IDs are UUIDs from the backend, not user-supplied free text.
- No raw API calls or contract redeclarations — the PR correctly reuses existing hooks, services, and typed clients.
Verdict: COMMENT
The change is architecturally sound and well-tested. The only blocking issue is the missing translations, which the author should add before marking the PR ready.
|
🤖 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 selection to automation creation, editing, details, and import/export. The implementation is clean and follows established patterns: it reuses useAgentProfiles, SettingsDropdownInput, and the deployment capabilities gate, and consistently enforces agent_profile_id / model mutual exclusivity across all code paths.
Acceptance Criteria Checklist (issue #17395)
- ✅ Creation and editing persist
agent_profile_idthrough the Automation Service API —buildCreateAutomationRequestandEditAutomationModal.handleSubmitboth send it. - ✅ Selection offered only when deployment advertises
agentProfiles— gated in bothedit-automation-modal.tsxandmanifest-setup-dialog.tsx. - ✅ Selecting a profile suppresses model selection; clearing restores default —
__default__sentinel maps tonull; model dropdown hidden via!form.agentProfileId. - ✅ Existing selections visible in details and survive import/export —
configuration-section.tsxrenders read-only selector;automation-export.tsround-trips the field. - ✅ Missing profiles visible as unavailable —
agent-profile-selector.tsxappends the unavailable ID with a localized label.
Findings
No material bugs, security issues, or design flaws found. A few observations:
Minor UX edge case (not blocking): In edit-automation-modal.tsx, if an automation was created with agent_profile_id set but the deployment no longer reports the agentProfiles capability (e.g. server downgrade), neither the profile selector nor the model dropdown renders. The form state preserves agentProfileId so saving won't silently change privileges, but the user has no visible control. This is an unlikely server-downgrade scenario and the behavior is safe (no privilege change), just not editable.
Model clearing is correct: Traced through the handleSubmit logic for all transitions (profile→model, model→profile, no-change save with both set). The selectedModel = form.agentProfileId ? "" : form.model.trim() line correctly forces body.model = null when a profile is selected, enforcing mutual exclusivity even for legacy automations that might have both fields.
Risk Assessment
- Overall PR: 🟢 LOW
- Canvas-only change; no runtime transport or profile-resolution logic added. The backend (OpenHands/automation#453) owns profile resolution. No raw API calls introduced — reuses existing typed client patterns. No new dependencies.
Note
The PR is marked draft and the description notes type checking and lint are in progress. This review covers the code as committed at the head SHA.
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.- 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.
|
PR Artifacts Cleaned Up The |
|
🤖 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 adds agent-profile selection to automation creation, editing, details display, and import/export. The implementation is clean, reuses existing infrastructure (useAgentProfiles hook, SettingsDropdownInput, AgentProfilesService), and follows the repository API access rules (no direct agent-server calls).
Acceptance Criteria Checklist (issue #17395)
- ✅ Creation and editing persist
agent_profile_id— Verified inautomation-service.api.ts(import/create path sendsagent_profile_idwith model suppression),edit-automation-modal.tsx(PATCH body includesagent_profile_idon change), andautomation-setup.ts(OPTIONAL_CREATE_PROPERTIESincludesagent_profile_id, model skipped when profile is set). - ✅ Offer selection only when deployment advertises
agentProfiles— Bothmanifest-setup-dialog.tsxandedit-automation-modal.tsxgate the selector oncapabilities.features.includes("agentProfiles"). - ✅ Selecting a profile suppresses model selection; clearing restores default — All three paths (setup dialog field filter, edit modal conditional render,
optionalCreatePropertiesmodel skip) enforce mutual exclusivity. Clearing sendsmodel: nullin the edit path. - ✅ Existing selections visible in details and survive import/export —
configuration-section.tsxrenders a read-onlyAutomationAgentProfileSelector.automation-export.tsserializes and parsesagent_profile_idthroughvalidateNullableSelection. - ✅ Missing profiles visible as unavailable —
agent-profile-selector.tsxshows "Unavailable profile ({{id}})" and retains the deleted profile as a dropdown item rather than silently selecting another.
Architecture
- No direct
fetch/axiosto agent-server endpoints —useAgentProfiles→AgentProfilesServiceis the proper service-layer path. agent_profile_idonAutomationandAutomationRuntypes mirrors the existingmodelfield pattern (Canvas-local mirror of server-owned fields).- No new dependencies, no telemetry changes, no wire-contract redeclarations.
- The
AutomationAgentProfileSelectoris a single reusable component shared across setup dialog, edit modal, and details view — good ownership.
Testing & Evidence
- Unit tests cover: profile selection/clearing (
agent-profile-selector.test.tsx), deleted-profile retention, bundle preflight/create profile preservation with model suppression (manifest-bundle.test.ts), custom-automation profile precedence (automation-setup.test.ts), and import/export round-trip for both null and UUID values (automation-export.test.ts). - Live evidence in
.pr/includes a GIF recording through real Canvas/Agent Server/Automation services showing before (null profile) and after (saved profile) behavior, plus supporting DB/payload JSON snapshots.
Risk Assessment
🟢 Low risk — The change is additive: a new optional field flows through existing UI patterns and service layers. When agentProfiles capability is absent, the feature is invisible and behavior is unchanged. The mutual-exclusivity between agent_profile_id and model is enforced consistently across all code paths. No agent runtime behavior, prompts, or tool selection is modified in Canvas.
Recommendation: A human maintainer should confirm the companion Automation Service API change (OpenHands/automation#453) is merged or will merge alongside this, since the frontend relies on the backend accepting agent_profile_id in create/update requests.
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
PR #17396 adds agent-profile selection to automations — creation, editing, details display, and import/export — reusing the existing useAgentProfiles hook and SettingsDropdownInput. A selected profile suppresses the separate model field; deleted profiles remain visible as unavailable. Capability discovery (agentProfiles feature) gates the UI.
Acceptance Criteria Checklist (Issue #17395)
- ✅ Creation and editing persist
agent_profile_id—buildCreateAutomationRequestsends it;edit-automation-modal.tsxsends it in PATCH; bundle creation reusesoptionalCreateProperties. - ✅ Offer selection only when deployment advertises
agentProfiles— both the edit modal and setup dialog gate oncapabilities.features.includes("agentProfiles"). - ✅ Selecting a profile suppresses model selection; clearing restores default — model dropdown hidden when profile set;
__default__sentinel clears tonull; save logic forcesmodel: nullwhen a profile is active. - ✅ Existing selections visible in details; survive import/export —
configuration-section.tsxrenders read-only selector;serializeAutomation/parseAutomationFileround-trip the field. - ✅ Missing profiles visible as unavailable — selector shows "Unavailable profile ({id})" and retains the stale ID in the dropdown.
Analysis
The mutual-exclusivity logic between agent_profile_id and model is consistently enforced across all paths:
- API layer (
automation-service.api.ts):spec.agent_profile_id ? { agent_profile_id } : spec.model && { model }— only one is sent. - Edit modal save:
selectedModel = form.agentProfileId ? "" : form.model.trim()— forces model to empty when a profile is selected, and sendsmodel: nullif the automation previously had a model. - Manifest setup:
optionalCreatePropertiesskipsmodelwhenvalues.agent_profile_idis truthy;manifest-local-validation.tsskipsllm-profilefield validation when a profile is selected. - Export/import:
validateNullableSelectionvalidates both fields symmetrically; profile takes precedence on creation if both are present in an imported file.
No direct agent-server calls, raw fetch/axios, or Canvas-local wire-contract redeclarations were introduced. The new AUTOMATIONS$UNAVAILABLE_AGENT_PROFILE i18n key has all 15 supported languages. Live evidence is provided under .pr/.
No material issues found.
Risk Assessment
🟢 LOW — The change is additive: a new optional field flows through existing patterns (profile query, settings dropdown, manifest builder, export validator). The capability gate ensures deployments without agentProfiles see no change. No agent behavior, prompts, or tool-selection paths are modified in Canvas — the profile is passed through to the automation service API, which owns resolution.
Verdict: APPROVE
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Audited all review feedback. Missing translations were fixed in2ffcb495c and current CI verifies the implementation. The nonblocking server-downgrade observation is intentionally kept safe: unsupported deployments do not expose profile edits, while the saved profile ID is preserved and remains visible in configuration details. I did not add a fallback that could change privileges. Companion Automation453 availability and the existing HUMAN-note validation gate remain separate. Please review the current head. |
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 selection to automation creation, editing, details, and import/export. It reuses the existing useAgentProfiles hook, SettingsDropdownInput, and deployment capability discovery (agentProfiles feature flag) — no new API access patterns or raw endpoint calls are introduced.
Acceptance Criteria Checklist (issue #17395)
- ✅ Creation and editing persist
agent_profile_idthrough the Automation Service API —buildCreateAutomationRequestsends it; the edit modal PATCH body includes it. - ✅ Offer selection only when the deployment advertises
agentProfiles— both the setup dialog and edit modal gate oncapabilities?.features.includes("agentProfiles"). - ✅ Selecting a profile suppresses the separate model field, and clearing restores it — setup dialog filters out
llm-profilefields; edit modal hides the model dropdown; save handler forcesmodel: nullwhen a profile is selected. - ✅ Existing selections remain visible in details and survive import/export —
configuration-section.tsxrenders the selector read-only;serializeAutomation/parseAutomationFileround-tripagent_profile_idviavalidateNullableSelection. - ✅ Missing profiles are visible as unavailable — the selector appends an "Unavailable profile" entry for deleted profile IDs rather than silently selecting another.
Findings
No material bugs, security issues, or design flaws found. The model/profile mutual exclusivity is consistently enforced across all three data-flow paths (create payload, edit PATCH body, preflight draft). The optionalCreateProperties reuse for bundle payloads (the bug fix called out in the PR description) is verified: buildBundlePayload now calls optionalCreateProperties, and buildPreflightBody delegates to buildCreatePayload, so both paths carry agent_profile_id.
Cross-Repo Prerequisite
The PR description correctly notes this requires OpenHands/automation#453 for the backend to persist and dispatch agent_profile_id. The Canvas-side code is complete; the backend prerequisite must merge and ship first.
Risk Assessment
🟡 Medium — This PR affects agent behavior (tool selection, available secrets via the selected profile). The Canvas implementation is sound, but the cross-repo backend dependency and the agent-behavior surface warrant human review before merge. Recommend verifying end-to-end with a deployment that advertises agentProfiles once automation#453 ships.
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.- 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.
|
🤖 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 selection to automation creation, editing, details, and import/export. The agent_profile_id field is mutually exclusive with model -- when a profile is selected, the model field is suppressed across all paths (create request builder, optional-properties builder, form validation, and UI rendering). Deleted profiles remain visible as unavailable rather than silently changing privileges. The bundle creation path is fixed to include optional properties (previously missing).
Acceptance Criteria Checklist (Issue #17395)
- Profile selection in automation creation (manifest-setup-dialog.tsx)
- Profile selection in automation editing (edit-automation-modal.tsx)
- Saved selection shown in details (configuration-section.tsx, read-only mode)
- Profile preserved through import/export (automation-export.ts)
- Selected profile suppresses the separate model field (consistent across all code paths)
- Capability discovery controls availability (
agentProfilesfeature gate) - Deleted profiles remain visible as unavailable (agent-profile-selector.tsx)
- Catalog automations requiring profiles enforce selection before submit
Analysis
Mutual exclusivity is consistently enforced. The agent_profile_id vs model precedence is applied in:
buildCreateAutomationRequest(automation-service.api.ts:199-201): ternary spread -- profile OR modeloptionalCreateProperties(automation-setup.ts:278): skips model when profile is setedit-automation-modal.tsx:250:selectedModel = form.agentProfileId ? "" : form.model.trim()manifest-local-validation.ts:271: skips llm-profile validation when profile is setmanifest-setup-dialog.tsx:558-561: filters out llm-profile field when profile is set
Bundle path fix is correct. buildBundlePayload now calls optionalCreateProperties(entry.setup, values) (line 543), matching the non-bundle path which already did so at line 417-420. This closes the gap where bundle-created automations lost profile/model/timeout.
Component design is clean. AutomationAgentProfileSelector handles both interactive (with onChange) and read-only (without onChange) modes through a single interface, used consistently across setup dialog, edit modal, details view, and review step.
Deleted profile handling is a good security/UX decision. Rather than silently selecting a different profile or clearing the field, the selector appends an unavailable entry with the original ID visible, preserving the user's awareness of what was configured.
Cross-repo dependency is correctly scoped. The PR adds no runtime transport or profile-resolution logic to Canvas -- that belongs in automation#453. The merge prerequisite is documented.
Risk Assessment
LOW -- The change is additive: a new optional field with proper capability gating, consistent mutual exclusivity logic, and proportional test coverage. No existing behavior is altered for automations without a selected profile. The bundle path fix is a correctness improvement (previously missing optional properties). The cross-repo prerequisite (automation#453) must merge and ship before this has runtime effect.
Verdict
No material findings. The change is well-structured with consistent enforcement of the profile/model mutual exclusivity across all code paths, graceful handling of deleted profiles, and adequate test coverage. Live before/after evidence is provided.
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 selection to automation creation, editing, details display, and import/export. The implementation is clean: one shared AutomationAgentProfileSelector component is reused across setup dialog, edit modal, and read-only details view. A selected profile suppresses the separate model field consistently across all code paths. Deleted profiles remain visible as unavailable rather than silently changing privileges. The bundle creation path is also fixed to include optional properties (previously missing).
Acceptance Criteria Checklist (Issue #17395)
- ✅ Profile selection in creation —
manifest-setup-dialog.tsxrenders the selector gated onagentProfilescapability;agent_profile_idflows throughbuildCreatePayload→optionalCreateProperties. - ✅ Profile selection in editing —
edit-automation-modal.tsxrenders the selector and sendsagent_profile_idin the PATCH body. - ✅ Saved selection shown in details —
configuration-section.tsxrenders a read-onlyAutomationAgentProfileSelector. - ✅ Selection preserved through import/export —
automation-export.tsserializes and parsesagent_profile_idviavalidateNullableSelection. - ✅ Capability discovery controls availability — both setup dialog and edit modal gate on
capabilities.features.includes("agentProfiles"). - ✅ Selected profile suppresses the separate model field — enforced in 5 places:
buildCreateAutomationRequest(ternary spread),optionalCreateProperties(skip model when profile set),handleSubmit(selectedModel = form.agentProfileId ? "" : form.model.trim()),validateFormValues(skip llm-profile validation), andmanifest-setup-dialog.tsx(filter llm-profile fields from render). - ✅ Deleted profiles remain visible as unavailable — selector appends the stale ID with a localized "Unavailable profile" label.
- ✅ Catalog automations requiring profiles enforce selection — both setup dialog and edit modal block submit when
requiresAgentProfile && !agent_profile_id.
Analysis
Mutual exclusivity is consistently enforced. The profile/model precedence is applied identically across all data-flow paths. The edit modal's PATCH body correctly handles all transitions (profile→model, model→profile, no-change save) — switching to a profile forces model: null in the body even for legacy automations that had both fields.
Bundle path fix is correct. buildBundlePayload now calls optionalCreateProperties(entry.setup, values), matching the non-bundle direct path. This closes the gap where bundle-created automations lost profile/model/timeout.
Architecture follows repository rules. No raw fetch/axios calls — useAgentProfiles → AgentProfilesService is the proper service-layer path. The agent_profile_id on Automation and AutomationRun types mirrors the existing model field pattern. The new i18n key AUTOMATIONS$UNAVAILABLE_AGENT_PROFILE has all 15 supported languages.
Cross-repo dependency is correctly scoped. The PR adds no runtime transport or profile-resolution logic to Canvas — that belongs in automation#453. The merge prerequisite is documented.
Risk Assessment
🟢 LOW — The change is additive: a new optional field flows through existing UI patterns and service layers. The capability gate ensures deployments without agentProfiles see no change. No agent behavior, prompts, or tool-selection paths are modified in Canvas. Profile IDs are UUIDs from the backend, not user-supplied free text. No new dependencies. The bundle path fix is a correctness improvement.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
08213f3 to
0f995ed
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
0f995ed to
cc4d3ce
Compare
3 participants
AGENT:
Why
Automations can select a model but cannot select the saved agent profile that owns their tools and available secrets. Add profile selection to creation and editing, show the saved selection in details, and preserve it through import/export.
Summary
Reuses the existing profile query/service and settings dropdown. Capability discovery controls availability; a selected profile suppresses the separate model field. Deleted profiles remain visible as unavailable rather than silently changing privileges.
Issue Number
Closes #17395. Requires the Automation Service API in OpenHands/automation#453. No runtime transport or profile-resolution logic is added to Canvas.
How to Test
Validation: the fresh-install UI walkthrough exposed a missing profile in the bundle creation path. Bundle creation now reuses the existing optional-property builder for both preflight and create. Sixteen setup/bundle tests pass, including saved-profile precedence over model selection; focused lint passes. Live UI recreation and saved-profile verification passed; the linked before/after recording reproduces and resolves the omission. Earlier selection, explicit clearing, missing-profile, and import/export checks passed.
Live Canvas evidence
Live before/after evidence now reproduces the bundle bug: the chosen profile disappears before the fix and persists after the same UI creation flow. Request and private database observations confirm the saved profile ID.
Scenario, exact revisions, and limitations. The isolated integration uses the PR change and its required unreleased dependencies.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/OpenHands/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.46.0-pythonopenhands-automation==1.11.1cc4d3ce25bc9d7055cf0be9e580ec90b748617adPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-cc4d3ceRun
All tags pushed for this build
About Multi-Architecture Support
sha-cc4d3ce) is a multi-arch manifest supporting both amd64 and arm64sha-cc4d3ce-amd64) are also available if neededMerge and release prerequisites
Must merge and be available first: automation#453 (and its native predecessor #449). The backend must persist and dispatch
agent_profile_idbefore shipping the UI selection. This PR targets main; that does not represent or remove its cross-repository prerequisite.Catalog automations requiring agentProfiles must select a saved profile before review or creation; the edit dialog uses existing template provenance to preserve that requirement. Ordinary automations retain the deployment-default option. Setup, edit, and selector regressions pass (38 tests); full TypeScript checking and focused lint pass.
Live setup regression evidence
Fresh isolated Canvas evidence shows the old form saving a null profile, then the fixed form blocking Continue before preflight until a profile is selected. The selected UUID survives reload, and Edit no longer offers Default for this profile-required automation. The definition remained inactive and never ran.
Before/after GIFs, exact revisions, request/persistence evidence, and scope.