feat: add automation draft lifecycle, endpoints, and synthetic event payloads - #439
feat: add automation draft lifecycle, endpoints, and synthetic event payloads#439malhotra5 wants to merge 12 commits into
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. |
a12a761 to
e23e34e
Compare
|
📁 PR Artifacts Notice This PR contains a |
|
QA pass for the requested draft event lifecycle flow. I exercised the flow through the FastAPI app with authenticated API requests, a SQLite test DB, and an in-memory file store:
Notes:
This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
Correction/update on QA: the earlier QA evidence used an in-process ASGI client plus a dispatcher completion stub, not a standalone uvicorn backend with external REST calls. I re-ran the requested flow the stricter way:
Pushed evidence in commit
This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
🤖 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
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 a server-backed automation draft lifecycle (CRUD + materialization + dispatch), synthetic event payloads for test dispatching, and restricts the public creation endpoints from setting state=DRAFT directly.
Taste Rating: Good taste -- the design cleanly separates draft state from automation state, reuses existing validation/materialization paths, and correctly prevents DRAFT automations from being triggered by the scheduler or webhook ingest (both filter on enabled=True AND state=ACTIVE).
Key findings
No material bugs, security problems, or significant design flaws found.
Security -- synthetic event_payload bypass (verified safe): The POST /v1/drafts/{id}/dispatch endpoint accepts an optional event_payload that bypasses webhook signature verification. This is explicitly documented and gated behind _require_manage_automations authentication -- the caller is trusted, so the payload is test input, not a real delivery. The synthetic payload flows correctly through create_pending_run -> stored on run.event_payload -> the dispatcher's _build_event_payload merges it into AUTOMATION_EVENT_PAYLOAD. Sound design.
DRAFT state isolation (verified safe): Materialized draft automations are created with enabled=False, state=DRAFT. The scheduler (_fetch_enabled_automations) and webhook dispatch (get_event_automations) both filter on enabled=True AND state=ACTIVE, so draft automations cannot be triggered automatically. The reject_public_draft_state validator prevents direct creation of DRAFT automations through /v1, /v1/preset/prompt, and /v1/preset/plugin. The normalize_automation_state_enabled function correctly handles the DRAFT case (skips the enabled-state conflict check, sets enabled=False).
Materialization reuse (verified safe): _get_live_materialized_draft_automation correctly returns None if the linked automation was activated (state != DRAFT), deleted, or is in a different org -- causing a new DRAFT automation to be created rather than overwriting an active one. This is well-tested in test_dispatch_draft_does_not_overwrite_linked_non_draft_automation.
Migration (verified compatible): Migration 024 uses generic SQLAlchemy types (sa.Uuid, sa.JSON, sa.Boolean, sa.String, sa.DateTime) and has the _is_sqlite() guard for the PostgreSQL-only COMMENT. Cross-database compatible per the AGENTS.md guidance.
Note (non-blocking): The .pr/ QA artifacts should be removed before merge. The PR Artifacts workflow will handle this for same-repo PRs, but it's worth confirming.
Test coverage
7 new tests in test_draft_router.py exercise the real code paths (no mock-only tests): incomplete draft validation, endpoint schema enforcement, tarball upload validation, dispatch with synthetic payload, re-dispatch with materialization reuse, incomplete-edit edge case, non-draft automation protection, and soft-delete cleanup. The synthetic payload test (test_dispatch_event_draft_with_synthetic_payload) verifies the payload is stored on the run.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: LOW
The change adds a new API surface (drafts) without modifying existing automation dispatch behavior. DRAFT-state automations are correctly isolated from automatic triggers. The synthetic payload bypass is authenticated and documented. The migration is cross-database compatible. Existing tests (1714 from PR 1) still pass.
VERDICT: Worth merging
KEY INSIGHT: The draft lifecycle cleanly separates "editing state" (drafts) from "runtime state" (automations) by materializing drafts into disabled DRAFT-state rows only on explicit dispatch, with proper guards preventing both automatic triggering and direct DRAFT creation through public endpoints.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a 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. |
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
🟢 Good taste — The draft lifecycle is well-designed: DRAFT state is properly fenced off from the public creation endpoints, materialization correctly reuses/overwrites draft automations, and the synthetic event payload path is a sensible authenticated-user test mechanism.
Key design validations
-
DRAFT state isolation:
reject_public_draft_stateon all public create/update endpoints prevents direct DRAFT creation, while thenormalize_automation_state_enabledspecial case allows draft bodies to carrystate=DRAFTwithout spurious errors. The Pydantic validation ordering (model_validator before → field_validator before) ensures rejection works correctly. -
Materialization lifecycle:
_get_live_materialized_draft_automationcorrectly returnsNonewhen the linked automation has been activated (state ≠ DRAFT), causing a fresh automation to be created rather than overwriting a live one. Tested intest_dispatch_draft_does_not_overwrite_linked_non_draft_automation. -
Dispatcher integration: Manual runs (
trigger_source=manual) are picked up by the dispatcher regardless of automationenabled/state, so DRAFT-state materialized automations dispatch correctly. The syntheticevent_payloadflows through the same_build_event_payloadpath as real webhook events. -
Delete cascade:
delete_draftsoft-deletes the materialized automation, skips pending runs (including manual), and marks git sync dirty. Does not touch linked non-DRAFT automations. -
Migration: Cross-database compatible — uses generic SQLAlchemy types, guards
COMMENT ON TABLEwith_is_sqlite(). Chain 023 → 024 is correct. -
Org-scoped access: All draft queries filter by
org_idanddeleted_at IS NULL.manage_automationspermission for mutations,view_automationsfor reads.
Non-blocking observations
-
Private function coupling (
draft_router.pylines 44-52, 37-40): The draft router imports 6 underscore-prefixed functions frompreset_router.pyand 3 fromcapabilities_router.py. This creates tight coupling between modules, but is pragmatic given the shared tarball-generation and validation logic. Consider promoting the most reused helpers to a shared utility module in a future refactor. -
No trigger-type guard on event_payload (
draft_router.pyline 610):DraftDispatchRequest.event_payloadis accepted for any draft type, including cron-triggered ones. For a cron draft, the synthetic payload would still be injected intoAUTOMATION_EVENT_PAYLOADby the dispatcher's_build_event_payload. This is harmless but could surprise a user testing a cron automation. Optionally, consider rejectingevent_payloadwhen the materialized automation's trigger type is notevent.
Security assessment
The synthetic event payload bypasses webhook signature verification by design — the caller is authenticated with manage_automations permission, so the payload is trusted as test input. This is the correct approach for a test-dispatch feature. The payload is stored on the run and injected into the sandbox environment the same way real webhook payloads are, so the execution path is identical.
.pr/ directory
The .pr/ QA artifacts (JSON evidence + markdown reports) will be auto-removed by the PR Artifacts workflow on approval. No action needed.
[RISK ASSESSMENT]
- Overall PR Risk: 🟢 LOW
- New table with proper migration, no changes to existing table schemas
- DRAFT state is additive — existing endpoints reject it, so no regression risk
- All new endpoints are org-scoped with permission checks
- Comprehensive test coverage (12 tests covering CRUD, dispatch, materialization, delete, synthetic payloads, edge cases)
- No dependency changes
VERDICT: ✅ Worth merging
KEY INSIGHT: The draft lifecycle cleanly separates incomplete setup state from dispatchable automations via DRAFT state + materialization, with the synthetic event payload being a well-scoped authenticated-user test mechanism.
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.
| automation = await _materialize_draft( | ||
| draft, parsed, user, request, session, file_store | ||
| ) | ||
| event_payload = body.event_payload if body is not None else None |
There was a problem hiding this comment.
Non-blocking: event_payload is accepted for any draft type, including cron-triggered ones. For a cron draft, the synthetic payload would still be stored on the run and injected into AUTOMATION_EVENT_PAYLOAD by the dispatcher's _build_event_payload. This is harmless but could surprise a user testing a non-event automation. Consider optionally rejecting event_payload when the materialized automation's trigger type is not event, to give clearer feedback.
| ) | ||
| from openhands.automation.preset_router import ( | ||
| CreatePluginAutomationRequest, | ||
| CreatePromptAutomationRequest, |
There was a problem hiding this comment.
Non-blocking: Importing 6 private (_-prefixed) functions from preset_router creates tight coupling between these modules. The shared tarball-generation and validation helpers (_generate_tarball, _generate_plugin_tarball, _bytes_to_async_iter, _safe_truncate, etc.) are good candidates for promotion to a shared utility module in a future refactor, so both routers depend on a public interface rather than each other's internals.
…payloads Add the draft lifecycle layer on top of the lifecycle_status foundation from PR #438: - draft_schemas.py: partial draft body models for all three creation endpoints (/v1, /v1/preset/prompt, /v1/preset/plugin), draft body normalization, and FINAL_DRAFT_MODELS registry - draft_router.py: full CRUD for server-backed drafts (POST/GET/PATCH/DELETE /v1/drafts), draft validation, materialization into DRAFT-state automations, and POST /v1/drafts/{id}/dispatch for test runs. Dispatch accepts an optional event_payload so event-triggered draft automations can be test-run with a user-supplied synthetic payload, bypassing webhook signature verification (caller is authenticated). - models.py: AutomationDraft model with source/materialized/last_test_run foreign keys - schemas.py: CreateAutomationDraftRequest, UpdateAutomationDraftRequest, AutomationDraftResponse, AutomationDraftListResponse, DraftDispatchRequest - capabilities_router.py: use draft_schemas registry instead of inline _DRAFT_MODELS; add automationDrafts to static features - app.py: register draft_router - utils/run.py: create_pending_run accepts optional event_payload - migration 024: automation_drafts table Stacked on PR #438 (lifecycle_status + trigger_source). Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
f31ae3a to
517a200
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Summary
This is PR 2 of a stacked series splitting PR #417 (automation draft lifecycle dispatch) into smaller, reviewable PRs.
Stack:
lifecycle_statusenum +trigger_sourceon runs →mainopenhands/lifecycle-stateWhat This PR Adds
Builds on the
lifecycle_status/trigger_sourcefoundation from PR #438:Draft lifecycle (
draft_router.py,draft_schemas.py)POST/GET/PATCH/DELETE /v1/drafts)/v1,/v1/preset/prompt,/v1/preset/plugin)DRAFT-state (disabled) AutomationPOST /v1/drafts/{id}/dispatch— test-run a draft as a manual runSynthetic event payloads (user requirement: option b)
POST /v1/drafts/{id}/dispatchaccepts an optionalDraftDispatchRequestbody withevent_payloadcreate_pending_run()now accepts an optionalevent_payloadparameterOther changes
models.py:AutomationDraftmodel withsource_automation_id,materialized_automation_id,last_test_run_idforeign keysschemas.py:CreateAutomationDraftRequest,UpdateAutomationDraftRequest,AutomationDraftResponse,AutomationDraftListResponse,DraftDispatchRequestcapabilities_router.py: draft schemas registry,automationDraftsin static featuresapp.py: registerdraft_routerautomation_draftstableTest plan
test_draft_router.py, includingtest_dispatch_event_draft_with_synthetic_payload)This PR was created by an AI agent (OpenHands) on behalf of the user.
@malhotra5 can click here to continue refining the PR