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. |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
29284cf to
435b315
Compare
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
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 introduces an explicit execution_mode field (script | agent) alongside agent_profile_id, cleanly separating credential scoping from conversation runtime creation. Profile-backed scripts pass the profile ID through the typed SDK command API so the Agent Server exposes only that profile's selected secrets, without creating a conversation or Docker runtime. Prompt/plugin presets default to agent; raw bundles default to script. Existing profile-backed definitions migrate to agent, preserving current behavior.
Analysis
Data structure: Good taste. Adding execution_mode as an independent dimension eliminates the implicit coupling between profile selection and conversation creation — previously, setting a profile ID implicitly forced conversation-backed execution. The AutomationExecutionMode enum provides a single source of truth for the valid values and defaults.
Migration: Cross-database compatible — generic sa.Column with String(20), server_default="script", and raw op.execute UPDATE statements that work on both PostgreSQL and SQLite. The backfill (SET execution_mode = 'agent' WHERE agent_profile_id IS NOT NULL) correctly preserves the previous behavior for existing automations. Downgrade is clean.
Backend selection: get_backend now gates on run.execution_mode == "agent" and bool(profile_id), which correctly handles all four combinations (script/agent x profile/no-profile). Script runs with a profile fall through to LocalAgentServerBackend but get the profile ID passed to workspace.start_command for secret scoping. Agent runs without a profile also fall through — reasonable since there's nothing to configure the agent with.
Dispatcher: agent_profile_id is only forwarded to execute_in_context for script runs. For agent runs it's None, which is correct — the SDK conversation resolves the profile itself.
Git sync: Export includes execution_mode; import defaults to "script" when absent (backward compat with older YAML). Validation rejects invalid values. Consistent with the existing agent_profile_id handling pattern.
Tests: test_conversation_backend.py parametrizes all three meaningful combinations (script-no-profile, script-with-profile, agent-with-profile). test_execution.py verifies agent_profile_id is forwarded through the full call chain. test_db.py checks the migration column exists on both tables. These exercise real code paths, not just mock assertions.
Non-blocking observation
UpdateAutomationRequest.execution_mode is typed as Literal["script", "agent"] | None = None. If a client explicitly sends "execution_mode": null in a PATCH body, model_dump(exclude_unset=True) includes it, and the setattr loop sets the NOT NULL column to None, causing a 500 at flush time. This is low impact (requires explicit action, result is just an error) but could be avoided by either omitting None from the union or skipping None values in the setattr loop for NOT NULL fields.
Risk Assessment
🟢 LOW — The change is additive: a new nullable-with-default column, a new field on existing schemas, and a new branch in backend selection. The migration preserves existing behavior. No breaking changes to public APIs (the new field defaults to script, and existing automations are backfilled to agent). The SDK pin update is first-party (OpenHands/software-agent-sdk) and acknowledged as temporary in the PR description.
Verdict: ✅ Worth merging
Key insight: Decoupling execution_mode from agent_profile_id is the right abstraction — it lets deterministic scripts use profile-scoped secrets without the overhead of a conversation runtime, and it makes the execution path explicit rather than implied by the presence of a profile ID.
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.
|
Posted an APPROVED review to PR #466 on GitHub (review ID 5204534183, commit 435b315). Verdict: ✅ Worth merging — 🟢 LOW risk The PR cleanly separates
Per the custom codereview guide, submitted as APPROVED (not COMMENTED) since the verdict is "Worth merging" with LOW risk and no blocking issues. This comment was posted by an AI agent (OpenHands). |
Why
A saved agent profile chooses credentials and agent configuration. It should not also force every deterministic automation script to create an agent conversation and, in Docker mode, a container. Scheduled scanners need their selected API token, while only the agent work they select needs a conversation runtime.
Summary
Add an explicit
execution_modesnapshot alongsideagent_profile_id. Raw automation bundles default toscript; prompt and plugin presets useagent. Profile-backed scripts remain ordinary automation commands and pass the profile ID through the typed software-agent-sdk command API so the Agent Server exposes only that profile's selected secrets. Existing profile-backed definitions migrate toagent, preserving their current behavior.Webhook-created runs now snapshot the same profile and execution fields as scheduled runs. CRUD responses and Git sync preserve both fields.
Issue Number
Closes #464.
How to Test
Focused backend, execution, CRUD, preset, Git-sync, queued-run, and SQLite migration tests pass. Formatting, Ruff, pycodestyle, and Pyright pass.
Review order and dependencies
Stack #454: #449 → #453 → #466 → #467 → #468. Review and merge in that order. This PR also depends on software-agent-sdk #5046; replace the temporary immutable source pin with its release before merging.