diff --git a/.pr/state-regression-tests.md b/.pr/state-regression-tests.md new file mode 100644 index 00000000..30f7337b --- /dev/null +++ b/.pr/state-regression-tests.md @@ -0,0 +1,32 @@ +# Reproductions for #438 + +Tests only; the author should fix production code to make the regressions pass. + +Run: + +```bash +uv run pytest tests/test_git_sync_state.py tests/test_migration_history.py -q +``` + +Expected on `aa6a0dea23ab411c2df910bebc7825e371c50fcc`: **4 failed, 7 passed**. + +| Regression | Expected behavior | Current failure | +| --- | --- | --- | +| Git `state: INACTIVE`, no legacy `enabled` | Persist INACTIVE and exclude it from scheduling | Persists ACTIVE/true and is selected | +| Explicit conflicting state/enabled, two cases | Reject the contradiction, as the API does | Silently accepts it | +| Integration with published migrations | Unique revisions and one upgrade head | Reuses revision 023 already published on main | + +Seven passing controls preserve legacy enabled-only inputs (including empty +YAML values) and consistent ACTIVE/INACTIVE/DRAFT pairs. The Git tests use the +real YAML decoder, importer, SQLite database, and scheduler query. They do not +download or execute the placeholder tarball. + +The migration fixtures pin published history at main commit +`baae0a8032470ceba014814210bd4b5c2e616d04`. The test is offline and also detects +#439's reuse of 024 when run on that stacked branch. It does not merge source +changes from main or test a production database. + +The #438-only ACTIVE -> DRAFT -> ACTIVE history case from the review is not +included: #439 intentionally rejects that public transition. The separate +question of draft-test failures counting toward production automatic disabling +needs a product decision before a test prescribes its desired behavior. diff --git a/migrations/versions/025_add_state_and_trigger_source.py b/migrations/versions/025_add_state_and_trigger_source.py new file mode 100644 index 00000000..922fabeb --- /dev/null +++ b/migrations/versions/025_add_state_and_trigger_source.py @@ -0,0 +1,60 @@ +"""Add automation state and run trigger_source. + +Revision ID: 025 +Revises: 024 +Create Date: 2026-09-10 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + + +revision: str = "025" +down_revision: str = "024" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.add_column( + "automations", + sa.Column( + "state", + sa.String(length=20), + nullable=False, + server_default="ACTIVE", + ), + ) + op.create_index("ix_automations_state", "automations", ["state"]) + # Backfill from the legacy enabled flag so existing rows match the new + # state model on day one. + op.execute( + "UPDATE automations SET state = CASE " + "WHEN enabled THEN 'ACTIVE' ELSE 'INACTIVE' END" + ) + + op.add_column( + "automation_runs", + sa.Column("trigger_source", sa.String(length=32), nullable=True), + ) + op.create_index( + "ix_automation_runs_trigger_source", "automation_runs", ["trigger_source"] + ) + op.create_index( + "ix_automation_runs_status_trigger_source", + "automation_runs", + ["status", "trigger_source"], + ) + + +def downgrade() -> None: + op.drop_index( + "ix_automation_runs_status_trigger_source", table_name="automation_runs" + ) + op.drop_index("ix_automation_runs_trigger_source", table_name="automation_runs") + op.drop_column("automation_runs", "trigger_source") + + op.drop_index("ix_automations_state", table_name="automations") + op.drop_column("automations", "state") diff --git a/openhands/automation/dispatcher.py b/openhands/automation/dispatcher.py index e5b6cef6..328e4f6a 100644 --- a/openhands/automation/dispatcher.py +++ b/openhands/automation/dispatcher.py @@ -22,7 +22,7 @@ from typing import Any import httpx -from sqlalchemy import select +from sqlalchemy import and_, or_, select from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from sqlalchemy.orm import selectinload @@ -40,6 +40,7 @@ Automation, AutomationRun, AutomationRunStatus, + AutomationState, TarballUpload, ) from openhands.automation.subjects import conversation_id_for @@ -137,8 +138,14 @@ async def _poll_pending_runs( .options(selectinload(AutomationRun.automation)) .where( AutomationRun.status == AutomationRunStatus.PENDING, - Automation.enabled.is_(True), Automation.deleted_at.is_(None), + or_( + AutomationRun.trigger_source == "manual", + and_( + Automation.enabled.is_(True), + Automation.state == AutomationState.ACTIVE, + ), + ), ) .order_by(AutomationRun.created_at.asc()) .limit(batch_size) diff --git a/openhands/automation/git_sync/loop.py b/openhands/automation/git_sync/loop.py index 5c3e363d..daf99f51 100644 --- a/openhands/automation/git_sync/loop.py +++ b/openhands/automation/git_sync/loop.py @@ -58,6 +58,7 @@ Automation, AutomationGitSyncOrgConfig, AutomationGitSyncState, + AutomationState, TarballUpload, UploadStatus, ) @@ -65,6 +66,10 @@ from openhands.automation.storage import ObjectNotFoundError, get_file_store from openhands.automation.utils import utcnow from openhands.automation.utils.periodic_loop import run_periodic_loop +from openhands.automation.utils.state import ( + automation_state_enabled, + model_automation_state, +) from openhands.automation.utils.tarball_validation import ( build_internal_url, build_upload_storage_path, @@ -505,6 +510,14 @@ async def _validate_and_resolve_fields( session, fields, deserialized, slug, existing, pending_storage_deletes, owner ) + enabled = True if fields.get("enabled") is None else bool(fields["enabled"]) + automation_state = model_automation_state(fields.get("state"), enabled) + expected_enabled = automation_state_enabled(automation_state) + if fields.get("state") is not None and fields.get("enabled") is not None: + if enabled != expected_enabled: + raise ValueError("enabled must be true only when state is ACTIVE") + enabled = expected_enabled + return { "name": name, "model": fields.get("model"), @@ -513,10 +526,8 @@ async def _validate_and_resolve_fields( "setup_script_path": setup_script_path, "timeout": timeout, "keep_alive": fields.get("keep_alive"), - # `dict.get`'s default only applies when the key is absent. A hand edit - # leaving "enabled:" empty is valid YAML parsing to None, and - # bool(None) would silently disable a live automation on import. - "enabled": True if fields.get("enabled") is None else bool(fields["enabled"]), + "enabled": enabled, + "state": automation_state, "prompt": fields.get("prompt"), "preset_metadata": fields.get("preset_metadata"), "tarball_path": tarball_path, @@ -769,6 +780,7 @@ async def _import_from_git( automation = await session.get(Automation, state.automation_id) if automation is not None and automation.deleted_at is None: automation.enabled = False + automation.state = AutomationState.INACTIVE automation.deleted_at = utcnow() result.deleted_in_db += 1 logger.info("Soft-deleted automation %s (removed from git)", automation.id) diff --git a/openhands/automation/git_sync/serializer.py b/openhands/automation/git_sync/serializer.py index 6e035202..56fff7b7 100644 --- a/openhands/automation/git_sync/serializer.py +++ b/openhands/automation/git_sync/serializer.py @@ -140,6 +140,7 @@ def _automation_yaml_fields( "timeout": automation.timeout, "keep_alive": automation.keep_alive, "enabled": automation.enabled, + "state": getattr(automation.state, "value", automation.state), "prompt": automation.prompt, "preset_metadata": automation.preset_metadata, "tarball_source": { diff --git a/openhands/automation/models.py b/openhands/automation/models.py index 1d250491..6f88d59b 100644 --- a/openhands/automation/models.py +++ b/openhands/automation/models.py @@ -48,6 +48,14 @@ class AutomationRunStatus(enum.Enum): SKIPPED = "SKIPPED" +class AutomationState(enum.Enum): + """State of an automation definition.""" + + ACTIVE = "ACTIVE" + INACTIVE = "INACTIVE" + DRAFT = "DRAFT" + + class Automation(Base): """An automation definition: what to run and when to trigger it.""" @@ -94,9 +102,19 @@ class Automation(Base): # means the automation service owns explicit cleanup. keep_alive: Mapped[bool | None] = mapped_column(default=None, nullable=True) - # Whether the automation is enabled (can be triggered) + # Deprecated: use state instead. Kept for backwards + # compatibility; only ACTIVE rows have enabled=True. Will be removed in a + # future release. enabled: Mapped[bool] = mapped_column(default=True, nullable=False, index=True) + state: Mapped[AutomationState] = mapped_column( + Enum(AutomationState, native_enum=False, length=20), + nullable=False, + default=AutomationState.ACTIVE, + server_default=AutomationState.ACTIVE.value, + index=True, + ) + # Current disabled-state metadata. AutomationDisableEvent keeps history. disabled_reason: Mapped[str | None] = mapped_column(Text, nullable=True) disabled_detail: Mapped[dict[str, Any] | None] = mapped_column(JSON, nullable=True) @@ -226,6 +244,11 @@ class AutomationRun(Base): # local mode). Set immediately after `_start_bash` returns. bash_command_id: Mapped[str | None] = mapped_column(String(64), nullable=True) + # How this run was created: manual, cron, event, or null for legacy rows. + trigger_source: Mapped[str | None] = mapped_column( + String(32), nullable=True, index=True + ) + # Event payload for event-triggered runs (JSON) # Contains the webhook payload that triggered this run. # For GitHub events: model_dump() of the parsed Pydantic event @@ -266,6 +289,7 @@ class AutomationRun(Base): Index("ix_automation_runs_status", "status"), Index("ix_automation_runs_status_created_at", "status", "created_at"), Index("ix_automation_runs_status_timeout_at", "status", "timeout_at"), + Index("ix_automation_runs_status_trigger_source", "status", "trigger_source"), # Partial: only live subjects are ever looked up, and only # `continue_conversation` runs set one. Index( diff --git a/openhands/automation/preset_router.py b/openhands/automation/preset_router.py index bb66d049..3385a70c 100644 --- a/openhands/automation/preset_router.py +++ b/openhands/automation/preset_router.py @@ -39,11 +39,17 @@ from openhands.automation.constants import MODEL_PROFILE_PATTERN from openhands.automation.db import get_session from openhands.automation.git_sync import mark_git_sync_dirty -from openhands.automation.models import Automation, TarballUpload, UploadStatus +from openhands.automation.models import ( + Automation, + TarballUpload, + UploadStatus, +) from openhands.automation.schemas import ( AutomationResponse, + AutomationState, TemplateProvenance, Trigger, + normalize_automation_state_enabled, ) from openhands.automation.storage import FileStore, ObjectNotFoundError, get_file_store from openhands.automation.telemetry import ( @@ -52,6 +58,10 @@ ) from openhands.automation.utils import utcnow from openhands.automation.utils.model_profiles import resolve_model_profile_for_user +from openhands.automation.utils.state import ( + automation_state_enabled, + model_automation_state, +) from openhands.automation.utils.tarball_validation import ( build_internal_url, build_upload_storage_path, @@ -74,6 +84,7 @@ router = APIRouter(prefix="/v1/preset", tags=["Presets"]) + _require_manage_automations = require_permission("manage_automations") # Preset files directories @@ -202,6 +213,13 @@ class CreatePromptAutomationRequest(BaseModel): default=True, description="Whether the automation starts enabled.", ) + state: AutomationState | None = Field( + default=None, + description=( + "First-class automation state. DRAFT/INACTIVE rows are not " + "triggered automatically." + ), + ) @field_validator("timeout") @classmethod @@ -212,6 +230,7 @@ def validate_timeout(cls, v: int | None) -> int | None: @classmethod def normalize_repos(cls, data: Any) -> Any: """Normalize repos to always be a list if provided.""" + data = normalize_automation_state_enabled(data) if isinstance(data, dict) and "repos" in data and data["repos"] is not None: repos = data["repos"] if isinstance(repos, (str, dict)): @@ -482,6 +501,7 @@ async def create_automation_from_prompt( return AutomationResponse.model_validate(existing) model = resolve_model_profile_for_user(body.model, user) + state = model_automation_state(body.state, body.enabled) # 1. Generate tarball with SDK code, prompt, and optional repos config tarball_content = _generate_tarball(body.prompt, repos=body.repos) @@ -551,7 +571,8 @@ async def create_automation_from_prompt( entrypoint=_get_preset_entrypoint(), timeout=default_automation_timeout(body.timeout), keep_alive=body.keep_alive, - enabled=body.enabled, + enabled=automation_state_enabled(state), + state=state, telemetry_distinct_id=get_request_telemetry_context( request ).frontend_distinct_id, @@ -706,6 +727,13 @@ class CreatePluginAutomationRequest(BaseModel): default=True, description="Whether the automation starts enabled.", ) + state: AutomationState | None = Field( + default=None, + description=( + "First-class automation state. DRAFT/INACTIVE rows are not " + "triggered automatically." + ), + ) @field_validator("timeout") @classmethod @@ -716,6 +744,7 @@ def validate_timeout(cls, v: int | None) -> int | None: @classmethod def normalize_plugins_and_repos(cls, data: dict) -> dict: # type: ignore[type-arg] """Normalize plugins and repos to always be lists.""" + data = normalize_automation_state_enabled(data) if isinstance(data, dict): # Normalize plugins if "plugins" in data and data["plugins"] is not None: @@ -892,6 +921,7 @@ async def create_automation_from_plugin( return AutomationResponse.model_validate(existing) model = resolve_model_profile_for_user(body.model, user) + state = model_automation_state(body.state, body.enabled) variants = _resolve_experiment_variant_models( body.variants, user, default_model=model ) @@ -984,7 +1014,8 @@ async def create_automation_from_plugin( entrypoint=_get_preset_entrypoint(), timeout=default_automation_timeout(body.timeout), keep_alive=body.keep_alive, - enabled=body.enabled, + enabled=automation_state_enabled(state), + state=state, telemetry_distinct_id=get_request_telemetry_context( request ).frontend_distinct_id, diff --git a/openhands/automation/router.py b/openhands/automation/router.py index 2cce3e7a..8a59dfc8 100644 --- a/openhands/automation/router.py +++ b/openhands/automation/router.py @@ -5,7 +5,7 @@ import re import uuid from datetime import timedelta -from typing import Any +from typing import Any, Final from fastapi import ( APIRouter, @@ -31,6 +31,7 @@ AutomationDisableEvent, AutomationRun, AutomationRunStatus, + AutomationState as ModelAutomationState, TarballUpload, ) from openhands.automation.preset_router import regenerate_preset_prompt_tarball @@ -68,6 +69,10 @@ run_status_detail_from_callback_error, ) from openhands.automation.utils.sandbox import cleanup_sandbox, pause_sandbox +from openhands.automation.utils.state import ( + automation_state_enabled, + model_automation_state, +) from openhands.automation.utils.tarball_validation import ( is_http_url, parse_internal_upload_id, @@ -85,6 +90,10 @@ router = APIRouter(prefix="/v1", tags=["Automations"]) + +_STATE_ONLY_UPDATE_FIELDS: Final[frozenset[str]] = frozenset({"state", "enabled"}) + + _require_view_automations = require_permission("view_automations") _require_manage_automations = require_permission("manage_automations") @@ -101,9 +110,6 @@ async def _assert_can_manage(automation: Automation, user: AuthenticatedUser) -> Callers must have already passed a ``view_automations`` dependency so the user is at least a member of the org. - - ``update_automation`` narrows this further: only the creator may change - an automation's definition; everyone else may only turn it off. """ if "manage_automations" in user.permissions: return @@ -115,6 +121,19 @@ async def _assert_can_manage(automation: Automation, user: AuthenticatedUser) -> ) +def _assert_can_update_fields( + automation: Automation, user: AuthenticatedUser, requested_fields: set[str] +) -> None: + if automation.user_id == user.user_id: + return + if requested_fields <= _STATE_ONLY_UPDATE_FIELDS: + return + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only the automation creator can change its definition", + ) + + # --- CRUD --- @@ -161,6 +180,8 @@ async def create_automation( if body.template is not None: preset_metadata = {"template": body.template.model_dump(exclude_none=True)} + state = model_automation_state(body.state, body.enabled) + auto = Automation( user_id=user.user_id, org_id=user.org_id, @@ -173,6 +194,8 @@ async def create_automation( entrypoint=body.entrypoint, timeout=default_automation_timeout(body.timeout), keep_alive=body.keep_alive, + enabled=automation_state_enabled(state), + state=state, telemetry_distinct_id=get_request_telemetry_context( request ).frontend_distinct_id, @@ -249,30 +272,26 @@ async def update_automation( # already-deleted object. session: AsyncSession = Depends(get_session, scope="function"), ) -> AutomationResponse: - """Partially update an automation. - - Only the creator may edit the definition. Admins and owners may set - ``enabled`` to ``False`` (turn it off) but nothing else. - """ + """Partially update an automation.""" auto = await _get_org_automation(session, automation_id, user.org_id) await _assert_can_manage(auto, user) update_data = body.model_dump(exclude_unset=True) - # Automations run under their creator's identity (git tokens, secrets, - # MCP servers), so only the creator may change what they do. Anyone else - # who passed _assert_can_manage (admins/owners) may only turn it off. - if auto.user_id != user.user_id and update_data != {"enabled": False}: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail=( - "Only the automation creator can edit it; admins and owners " - "can only turn it off or delete it" - ), - ) + _assert_can_update_fields(auto, user, set(update_data)) # Handle trigger field mapping (only if trigger has a real value) if body.trigger is not None: update_data["trigger"] = body.trigger.model_dump() + requested_state = update_data.pop("state", None) + if requested_state is not None: + state = model_automation_state( + requested_state, update_data.get("enabled", auto.enabled) + ) + update_data["state"] = state + update_data["enabled"] = automation_state_enabled(state) + elif "enabled" in update_data: + update_data["state"] = model_automation_state(None, update_data["enabled"]) + # Same rule CreateAutomationRequest enforces, applied to the merged view: # either half of the pair can arrive alone in a partial update. trigger = update_data.get("trigger") or auto.trigger or {} @@ -297,8 +316,16 @@ async def update_automation( update_data["disabled_detail"] = None update_data["disabled_at"] = None elif update_data.get("enabled") is False: - if auto.enabled: - skip_pending_reason = "Automation disabled by user" + state = update_data.get("state") + is_manual_inactive = state == ModelAutomationState.INACTIVE or ( + state is None and auto.state != ModelAutomationState.DRAFT + ) + skip_pending_reason = ( + "Automation moved to draft by user" + if state == ModelAutomationState.DRAFT + else "Automation disabled by user" + ) + if auto.enabled and is_manual_inactive: disabled_at = utcnow() disabled_detail = {"reason": "manual", "source": "user"} update_data["disabled_reason"] = "manual" @@ -375,6 +402,7 @@ async def delete_automation( await _assert_can_manage(auto, user) was_enabled = auto.enabled auto.enabled = False + auto.state = ModelAutomationState.INACTIVE deleted_at = utcnow() auto.deleted_at = deleted_at if was_enabled: @@ -396,6 +424,7 @@ async def delete_automation( reason="Automation deleted by user", disabled_detail=auto.disabled_detail, completed_at=deleted_at, + include_manual=True, ) await session.flush() await mark_git_sync_dirty(session, auto) @@ -491,15 +520,6 @@ async def dispatch_automation( """ auto = await _get_org_automation(session, automation_id, user.org_id) await _assert_can_manage(auto, user) - if not auto.enabled: - raise HTTPException( - status.HTTP_409_CONFLICT, - detail={ - "message": "Automation is disabled", - "disabled_reason": auto.disabled_reason, - "disabled_detail": auto.disabled_detail, - }, - ) run = await create_pending_run( session, @@ -507,6 +527,7 @@ async def dispatch_automation( telemetry_distinct_id=get_request_telemetry_context( request ).frontend_distinct_id, + trigger_source="manual", ) await session.flush() await session.refresh(run) diff --git a/openhands/automation/scheduler.py b/openhands/automation/scheduler.py index 2a916436..50ba3c25 100644 --- a/openhands/automation/scheduler.py +++ b/openhands/automation/scheduler.py @@ -19,7 +19,11 @@ from openhands.automation.db import using_sqlite from openhands.automation.git_sync import mark_git_sync_dirty -from openhands.automation.models import Automation, AutomationRun +from openhands.automation.models import ( + Automation, + AutomationRun, + AutomationState, +) from openhands.automation.telemetry import capture_automation_event from openhands.automation.utils import get_next_fire_time, is_automation_due, utcnow from openhands.automation.utils.run import create_pending_run @@ -58,6 +62,7 @@ def _disable_invalid_cron_automation( error: BaseException, ) -> None: automation.enabled = False + automation.state = AutomationState.INACTIVE logger.error( "Disabling automation with invalid cron trigger: %s", reason, @@ -127,6 +132,7 @@ async def _fetch_enabled_automations( select(Automation) .where( Automation.enabled.is_(True), + Automation.state == AutomationState.ACTIVE, Automation.deleted_at.is_(None), (Automation.last_polled_at.is_(None)) | (Automation.last_polled_at < poll_threshold), @@ -201,7 +207,9 @@ async def poll_and_schedule( for automation in due_automations: try: - run = await create_pending_run(session, automation) + run = await create_pending_run( + session, automation, trigger_source="cron" + ) created_runs.append(run) schedule_properties = { "trigger_source": "cron", diff --git a/openhands/automation/schemas.py b/openhands/automation/schemas.py index efb9996e..982ef46a 100644 --- a/openhands/automation/schemas.py +++ b/openhands/automation/schemas.py @@ -3,6 +3,7 @@ import json import re import uuid +import warnings from enum import StrEnum from typing import Annotated, Any, Final, Literal @@ -29,6 +30,7 @@ validate_cron_schedule as validate_cron_schedule_value, validate_timezone_name, ) +from openhands.automation.utils.state import automation_state_enabled from openhands.automation.utils.time import UtcDatetime from openhands.automation.utils.timeout import ( build_automation_timeout_description, @@ -335,6 +337,42 @@ class RunStatus(StrEnum): SKIPPED = "SKIPPED" +class AutomationState(StrEnum): + """State of an automation definition.""" + + ACTIVE = "ACTIVE" + INACTIVE = "INACTIVE" + DRAFT = "DRAFT" + + +def normalize_automation_state_enabled(data: Any) -> Any: + """Keep automation state and enabled compatible in request bodies. + + Emits a DeprecationWarning when ``enabled`` is explicitly provided — + callers should migrate to ``state``. + """ + if not isinstance(data, dict): + return data + if "enabled" in data: + warnings.warn( + "The 'enabled' field is deprecated; use 'state' instead.", + DeprecationWarning, + stacklevel=2, + ) + state_value = data.get("state") + if state_value is None: + return data + try: + expected_enabled = automation_state_enabled(state_value) + except ValueError: + return data + if "enabled" in data and bool(data["enabled"]) != expected_enabled: + raise ValueError("enabled must be true only when state is ACTIVE") + data = dict(data) + data["enabled"] = expected_enabled + return data + + def validate_command_string( v: str | None, field_name: str, *, allow_none: bool = True ) -> str | None: @@ -460,6 +498,22 @@ class CreateAutomationRequest(BaseModel): "completion (or after post-run callbacks, when configured)." ), ) + enabled: bool = Field( + default=True, + deprecated=True, + description=( + "Deprecated: use state instead. Backward-compatible " + "active flag; false creates INACTIVE unless state is " + "DRAFT. Will be removed in a future release." + ), + ) + state: AutomationState | None = Field( + default=None, + description=( + "First-class automation state. DRAFT/INACTIVE rows are not " + "triggered automatically." + ), + ) template: TemplateProvenance | None = Field( default=None, description=( @@ -469,6 +523,11 @@ class CreateAutomationRequest(BaseModel): ), ) + @model_validator(mode="before") + @classmethod + def validate_automation_state_enabled(cls, data: Any) -> Any: + return normalize_automation_state_enabled(data) + @field_validator("tarball_path") @classmethod def validate_tarball_path(cls, v: str) -> str: @@ -547,7 +606,19 @@ class UpdateAutomationRequest(BaseModel): description=build_automation_timeout_description(include_default=False), ) keep_alive: bool | None = Field(default=None) - enabled: bool | None = None + enabled: bool | None = Field( + default=None, + deprecated=True, + description=( + "Deprecated: use state instead. Will be removed in a future release." + ), + ) + state: AutomationState | None = None + + @model_validator(mode="before") + @classmethod + def validate_automation_state_enabled(cls, data: Any) -> Any: + return normalize_automation_state_enabled(data) @field_validator("tarball_path") @classmethod @@ -876,7 +947,14 @@ class AutomationResponse(BaseModel): entrypoint: str timeout: int | None keep_alive: bool | None - enabled: bool + enabled: bool = Field( + deprecated=True, + description=( + "Deprecated: use state instead. Included for backward " + "compatibility; will be removed in a future release." + ), + ) + state: AutomationState = AutomationState.ACTIVE disabled_reason: str | None = None disabled_detail: dict[str, Any] | None = None disabled_at: UtcDatetime | None = None @@ -942,6 +1020,7 @@ class AutomationRunResponse(BaseModel): id: uuid.UUID automation_id: uuid.UUID status: RunStatus + trigger_source: str | None = None error_detail: str | None status_detail: dict[str, Any] | None = None current_phase: str | None = None diff --git a/openhands/automation/utils/run.py b/openhands/automation/utils/run.py index 7fe41731..802f96ca 100644 --- a/openhands/automation/utils/run.py +++ b/openhands/automation/utils/run.py @@ -14,6 +14,7 @@ AutomationDisableEvent, AutomationRun, AutomationRunStatus, + AutomationState, ) from openhands.automation.telemetry import capture_automation_event from openhands.automation.utils.time import utcnow @@ -70,6 +71,7 @@ async def disable_automation( ) .values( enabled=False, + state=AutomationState.INACTIVE, disabled_reason=reason, disabled_detail=disabled_detail, disabled_at=disabled_at, @@ -129,6 +131,7 @@ async def skip_pending_runs_for_disabled_automation( reason: str, disabled_detail: dict | None = None, completed_at: datetime | None = None, + include_manual: bool = False, ) -> int: """Mark accepted-but-not-dispatched runs terminal when automation is disabled.""" completed_at = completed_at or utcnow() @@ -144,12 +147,19 @@ async def skip_pending_runs_for_disabled_automation( if disabled_detail is not None: status_detail["disabled_detail"] = disabled_detail + filters = [ + AutomationRun.automation_id == automation_id, + AutomationRun.status == AutomationRunStatus.PENDING, + ] + if not include_manual: + filters.append( + (AutomationRun.trigger_source.is_(None)) + | (AutomationRun.trigger_source != "manual") + ) + result: CursorResult = await session.execute( # type: ignore[assignment] update(AutomationRun) - .where( - AutomationRun.automation_id == automation_id, - AutomationRun.status == AutomationRunStatus.PENDING, - ) + .where(*filters) .values( status=AutomationRunStatus.SKIPPED, completed_at=completed_at, @@ -165,6 +175,7 @@ async def create_pending_run( automation: Automation, *, telemetry_distinct_id: str | None = None, + trigger_source: str | None = None, ) -> AutomationRun: """Create a PENDING automation run for dispatch. @@ -184,6 +195,7 @@ async def create_pending_run( id=uuid.uuid4(), automation_id=automation.id, status=AutomationRunStatus.PENDING, + trigger_source=trigger_source, telemetry_distinct_id=( telemetry_distinct_id or automation.telemetry_distinct_id ), diff --git a/openhands/automation/utils/state.py b/openhands/automation/utils/state.py new file mode 100644 index 00000000..86e64327 --- /dev/null +++ b/openhands/automation/utils/state.py @@ -0,0 +1,24 @@ +"""Helpers for automation state compatibility.""" + +from enum import Enum +from typing import Any + +from openhands.automation.models import AutomationState + + +def _state_value(state: Any) -> Any: + return state.value if isinstance(state, Enum) else state + + +def model_automation_state( + state: AutomationState | str | Enum | None, enabled: bool +) -> AutomationState: + if state is not None: + return AutomationState(_state_value(state)) + return AutomationState.ACTIVE if enabled else AutomationState.INACTIVE + + +def automation_state_enabled(state: AutomationState | str | Enum | None) -> bool: + if state is None: + return True + return AutomationState(_state_value(state)) == AutomationState.ACTIVE diff --git a/openhands/automation/utils/unhealthy.py b/openhands/automation/utils/unhealthy.py index a39a5084..b9cc33db 100644 --- a/openhands/automation/utils/unhealthy.py +++ b/openhands/automation/utils/unhealthy.py @@ -31,6 +31,7 @@ AutomationDisableEvent, AutomationRun, AutomationRunStatus, + AutomationState, ) from openhands.automation.utils.run import skip_pending_runs_for_disabled_automation from openhands.automation.utils.time import ensure_utc, utcnow @@ -240,6 +241,7 @@ async def _apply_disable( ) .values( enabled=False, + state=AutomationState.INACTIVE, disabled_reason=cause.reason, disabled_detail=disabled_detail, disabled_at=disabled_at, diff --git a/openhands/automation/utils/webhook.py b/openhands/automation/utils/webhook.py index 8826945d..999a55a4 100644 --- a/openhands/automation/utils/webhook.py +++ b/openhands/automation/utils/webhook.py @@ -19,6 +19,7 @@ Automation, AutomationRun, AutomationRunStatus, + AutomationState, CustomWebhook, ) from openhands.automation.providers import ( @@ -134,6 +135,7 @@ async def get_event_automations( base_filters = [ Automation.org_id == org_id, Automation.enabled == True, # noqa: E712 + Automation.state == AutomationState.ACTIVE, Automation.deleted_at.is_(None), ] @@ -197,6 +199,7 @@ async def get_requested_event_types( base_filters = [ Automation.enabled == True, # noqa: E712 + Automation.state == AutomationState.ACTIVE, Automation.deleted_at.is_(None), ] @@ -263,6 +266,7 @@ async def create_automation_run( id=uuid.uuid4(), automation_id=automation.id, status=AutomationRunStatus.PENDING, + trigger_source="event", event_payload=event_payload, telemetry_distinct_id=automation.telemetry_distinct_id, subject_key=subject_key, diff --git a/tests/fixtures/published_migrations/023_org_scoped_git_sync.py.txt b/tests/fixtures/published_migrations/023_org_scoped_git_sync.py.txt new file mode 100644 index 00000000..2665f7c4 --- /dev/null +++ b/tests/fixtures/published_migrations/023_org_scoped_git_sync.py.txt @@ -0,0 +1,222 @@ +"""Scope git sync to organizations. + +Revision ID: 023 +Revises: 022 +Create Date: 2026-09-05 + +Git sync used to be service-wide: one runtime-config blob and one set of +status keys in `automation_service_metadata`, and a globally unique directory +slug per synced automation. Each organization now syncs its own automations +to its own repo, so config and bookkeeping move to a per-org table and slugs +are unique per org. +""" + +import uuid +from collections.abc import Sequence +from datetime import datetime + +import sqlalchemy as sa +from alembic import op + + +revision: str = "023" +down_revision: str = "022" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +# The deterministic local-mode org, as derived by `auth.py`'s +# `_get_local_user()`. Duplicated rather than imported: migrations must not +# load the application. +_LOCAL_ORG_ID = uuid.uuid5(uuid.NAMESPACE_DNS, "openhands-local-org") + +_LEGACY_CONFIG_KEY = "git_sync_config_override" +# Legacy service-metadata key -> column on the per-org table. +_LEGACY_STATUS_COLUMNS = { + "git_sync_last_commit": "last_synced_commit", + "git_sync_last_path": "last_synced_path", + "git_sync_last_run_at": "last_run_at", + "git_sync_last_error": "last_error", + "git_sync_last_error_at": "last_error_at", +} +_TIMESTAMP_COLUMNS = {"last_run_at", "last_error_at"} + +_service_metadata = sa.table( + "automation_service_metadata", + sa.column("key", sa.String(255)), + sa.column("value", sa.Text()), +) +_org_config = sa.table( + "automation_git_sync_org_config", + sa.column("org_id", sa.Uuid()), + sa.column("overrides", sa.Text()), + sa.column("last_synced_commit", sa.String(64)), + sa.column("last_synced_path", sa.String(255)), + sa.column("last_run_at", sa.DateTime(timezone=True)), + sa.column("last_error", sa.Text()), + sa.column("last_error_at", sa.DateTime(timezone=True)), +) + + +def _is_sqlite() -> bool: + return op.get_bind().dialect.name == "sqlite" + + +def _parse_timestamp(value: str | None) -> datetime | None: + # The loop wrote "" to clear a value, so blank means unset. + if not value: + return None + try: + return datetime.fromisoformat(value) + except ValueError: + return None + + +def upgrade() -> None: + op.create_table( + "automation_git_sync_org_config", + sa.Column("org_id", sa.Uuid, primary_key=True), + sa.Column("overrides", sa.Text, nullable=False, server_default="{}"), + sa.Column("configured_by_user_id", sa.Uuid, nullable=True), + sa.Column("last_synced_commit", sa.String(64), nullable=True), + sa.Column("last_synced_path", sa.String(255), nullable=True), + sa.Column("last_run_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("last_error", sa.Text, nullable=True), + sa.Column("last_error_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("sync_started_at", sa.DateTime(timezone=True), nullable=True), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("CURRENT_TIMESTAMP"), + nullable=False, + ), + sa.Column( + "updated_at", + sa.DateTime(timezone=True), + server_default=sa.text("CURRENT_TIMESTAMP"), + nullable=False, + ), + ) + + # Nullable first so existing rows can be backfilled from their automation + # (the CASCADE foreign key guarantees every row still has one), then made + # required. Batch mode is what lets SQLite alter the column; on PostgreSQL + # it is a plain ALTER TABLE. + op.add_column( + "automation_git_sync_state", sa.Column("org_id", sa.Uuid, nullable=True) + ) + op.execute( + "UPDATE automation_git_sync_state SET org_id = (" + "SELECT org_id FROM automations " + "WHERE automations.id = automation_git_sync_state.automation_id)" + ) + # Before the batch step, so SQLite's table recreate doesn't carry over an + # index that is about to be replaced. + op.drop_index( + "ix_automation_git_sync_state_slug", table_name="automation_git_sync_state" + ) + with op.batch_alter_table("automation_git_sync_state") as batch: + batch.alter_column("org_id", existing_type=sa.Uuid(), nullable=False) + op.create_index( + "ix_automation_git_sync_state_org_id", + "automation_git_sync_state", + ["org_id"], + ) + op.create_index( + "ix_automation_git_sync_state_org_slug", + "automation_git_sync_state", + ["org_id", "slug"], + unique=True, + ) + + _move_legacy_service_metadata() + + if _is_sqlite(): + return + + op.execute( + "COMMENT ON TABLE automation_git_sync_org_config IS " + "'Per-organization git sync: runtime config overrides (JSON, secrets " + "wrapped at rest), last synced commit/run/error, and the sync lease.'" + ) + + +def _move_legacy_service_metadata() -> None: + """Carry the service-wide config and status over to the local org's row. + + Only a local-mode loop ever ran a cycle, and only a cycle writes the + status keys, so their presence (or a SQLite database, which is what local + deployments use) means this deployment is the local org. A PostgreSQL + database holding only the config blob is a cloud org that saved settings + while sync was unavailable there: there is no org to attribute it to, so + it is dropped rather than handed to the local org id. + """ + bind = op.get_bind() + keys = [_LEGACY_CONFIG_KEY, *_LEGACY_STATUS_COLUMNS] + legacy: dict[str, str] = { + row.key: row.value + for row in bind.execute( + sa.select(_service_metadata.c.key, _service_metadata.c.value).where( + _service_metadata.c.key.in_(keys) + ) + ) + } + if not legacy: + return + + ran_here = _is_sqlite() or any(key in legacy for key in _LEGACY_STATUS_COLUMNS) + if ran_here: + values: dict[str, object] = { + "org_id": _LOCAL_ORG_ID, + "overrides": legacy.get(_LEGACY_CONFIG_KEY) or "{}", + } + for key, column in _LEGACY_STATUS_COLUMNS.items(): + raw = legacy.get(key) + values[column] = ( + _parse_timestamp(raw) if column in _TIMESTAMP_COLUMNS else raw or None + ) + bind.execute(_org_config.insert().values(**values)) + + bind.execute(_service_metadata.delete().where(_service_metadata.c.key.in_(keys))) + + +def downgrade() -> None: + """Best-effort reverse. + + Only the local org's row can go back to service metadata; other orgs' + config is dropped with the table. Re-creating the global slug index fails + if two orgs share a slug. + """ + bind = op.get_bind() + row = bind.execute( + sa.select(_org_config).where(_org_config.c.org_id == _LOCAL_ORG_ID) + ).first() + if row is not None: + keys = [_LEGACY_CONFIG_KEY, *_LEGACY_STATUS_COLUMNS] + bind.execute( + _service_metadata.delete().where(_service_metadata.c.key.in_(keys)) + ) + legacy: dict[str, str] = {_LEGACY_CONFIG_KEY: row.overrides or "{}"} + for key, column in _LEGACY_STATUS_COLUMNS.items(): + value = getattr(row, column) + if value is None: + continue + legacy[key] = value.isoformat() if isinstance(value, datetime) else value + for key, value in legacy.items(): + bind.execute(_service_metadata.insert().values(key=key, value=value)) + + op.drop_index( + "ix_automation_git_sync_state_org_slug", + table_name="automation_git_sync_state", + ) + op.drop_index( + "ix_automation_git_sync_state_org_id", table_name="automation_git_sync_state" + ) + with op.batch_alter_table("automation_git_sync_state") as batch: + batch.drop_column("org_id") + op.create_index( + "ix_automation_git_sync_state_slug", + "automation_git_sync_state", + ["slug"], + unique=True, + ) + op.drop_table("automation_git_sync_org_config") diff --git a/tests/fixtures/published_migrations/024_add_run_sandbox_cleanup_due_at.py.txt b/tests/fixtures/published_migrations/024_add_run_sandbox_cleanup_due_at.py.txt new file mode 100644 index 00000000..0705070a --- /dev/null +++ b/tests/fixtures/published_migrations/024_add_run_sandbox_cleanup_due_at.py.txt @@ -0,0 +1,42 @@ +"""Add when a run's sandbox is due for deferred deletion. + +Revision ID: 024 +Revises: 023 +Create Date: 2026-09-14 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + + +revision: str = "024" +down_revision: str = "023" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.add_column( + "automation_runs", + sa.Column("sandbox_cleanup_due_at", sa.DateTime(timezone=True), nullable=True), + ) + # Partial: NULL on nearly every row; only stamped when the service defers + # sandbox deletion, and cleared once the sandbox is gone. + where = "sandbox_cleanup_due_at IS NOT NULL" + op.create_index( + "ix_automation_runs_sandbox_cleanup_due", + "automation_runs", + ["sandbox_cleanup_due_at"], + unique=False, + postgresql_where=sa.text(where), + sqlite_where=sa.text(where), + ) + + +def downgrade() -> None: + op.drop_index( + "ix_automation_runs_sandbox_cleanup_due", table_name="automation_runs" + ) + op.drop_column("automation_runs", "sandbox_cleanup_due_at") diff --git a/tests/fixtures/published_migrations/README.md b/tests/fixtures/published_migrations/README.md new file mode 100644 index 00000000..52ffddfc --- /dev/null +++ b/tests/fixtures/published_migrations/README.md @@ -0,0 +1,10 @@ +These are byte-for-byte copies of migrations already published on `main` at +`baae0a8032470ceba014814210bd4b5c2e616d04` (September 14, 2026): + +- [023_org_scoped_git_sync.py](https://github.com/OpenHands/automation/blob/baae0a8032470ceba014814210bd4b5c2e616d04/migrations/versions/023_org_scoped_git_sync.py) +- [024_add_run_sandbox_cleanup_due_at.py](https://github.com/OpenHands/automation/blob/baae0a8032470ceba014814210bd4b5c2e616d04/migrations/versions/024_add_run_sandbox_cleanup_due_at.py) + +`test_migration_history.py` combines these files with the branch's migrations in +a temporary directory and checks the Alembic graph. This catches reused IDs +before rebasing, without Git remotes, network access, or a deployed database. +The `.txt` suffix keeps historical snapshots out of automatic Python rewrites. diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index ec84ad84..e976a5c6 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -23,7 +23,12 @@ dispatcher_loop, ) from openhands.automation.exceptions import ConcurrencyLimitReachedError -from openhands.automation.models import Automation, AutomationRun, AutomationRunStatus +from openhands.automation.models import ( + Automation, + AutomationRun, + AutomationRunStatus, + AutomationState, +) from openhands.automation.subjects import conversation_id_for from openhands.automation.utils import utcnow from openhands.automation.utils.run import ( @@ -550,6 +555,41 @@ async def test_ignores_pending_runs_for_disabled_automations( assert dispatched == [] mock_execute.assert_not_awaited() + @patch("openhands.automation.dispatcher._execute_run_safe", new_callable=AsyncMock) + async def test_dispatches_manual_run_for_disabled_automation( + self, mock_execute, async_session_factory, mock_settings, mock_client + ): + """Manual pending runs are dispatched even when automation is inactive.""" + async with async_session_factory() as session: + automation = Automation( + user_id=TEST_USER_ID, + org_id=TEST_ORG_ID, + name="Test", + trigger={"type": "cron", "schedule": "* * * * *", "timezone": "UTC"}, + tarball_path="s3://bucket/code.tar.gz", + entrypoint="uv run main.py", + enabled=False, + state=AutomationState.INACTIVE, + ) + session.add(automation) + await session.commit() + + run = AutomationRun( + automation_id=automation.id, + status=AutomationRunStatus.PENDING, + trigger_source="manual", + ) + session.add(run) + await session.commit() + run_id = run.id + + dispatched = await dispatch_pending_runs( + async_session_factory, mock_settings, mock_client + ) + + assert [run.id for run in dispatched] == [run_id] + mock_execute.assert_awaited_once() + @patch("openhands.automation.dispatcher._execute_run_safe", new_callable=AsyncMock) async def test_respects_batch_size( self, mock_execute, async_session_factory, mock_settings, mock_client diff --git a/tests/test_git_sync.py b/tests/test_git_sync.py index 4b835121..a4c7162e 100644 --- a/tests/test_git_sync.py +++ b/tests/test_git_sync.py @@ -1821,7 +1821,34 @@ async def test_yaml_only_edit_does_not_create_a_new_upload( ) await self._push_yaml_edit( - origin, "editor-yaml", "enabled: true", "enabled: false" + origin, + "editor-yaml", + "\n".join( + [ + "enabled: true", + "entrypoint: python main.py", + "keep_alive: null", + "model: null", + "name: My First Automation", + "preset_metadata: null", + "prompt: null", + "setup_script_path: null", + "state: ACTIVE", + ] + ), + "\n".join( + [ + "enabled: false", + "entrypoint: python main.py", + "keep_alive: null", + "model: null", + "name: My First Automation", + "preset_metadata: null", + "prompt: null", + "setup_script_path: null", + "state: INACTIVE", + ] + ), ) await run_sync_cycle( sqlite_session_factory, LOCAL_ORG_ID, git_settings, service_settings diff --git a/tests/test_git_sync_state.py b/tests/test_git_sync_state.py new file mode 100644 index 00000000..93a90d95 --- /dev/null +++ b/tests/test_git_sync_state.py @@ -0,0 +1,141 @@ +"""Lifecycle state at the Git import boundary. + +The new state field must work without the deprecated enabled field. When both +are explicit, imports must reject contradictions just as API requests do. +These tests use the real YAML decoder, importer, database, and scheduler query. +""" + +import uuid +from collections.abc import AsyncIterator +from typing import Any + +import pytest +import yaml +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine + +from openhands.automation.db import set_sqlite_mode, using_sqlite +from openhands.automation.git_sync.loop import _create_automation_from_git, _Owner +from openhands.automation.git_sync.serializer import deserialize_automation +from openhands.automation.models import Automation, AutomationState, Base +from openhands.automation.scheduler import _fetch_enabled_automations +from openhands.automation.utils.time import utcnow + + +TEST_USER_ID = uuid.UUID("12345678-1234-5678-1234-567812345678") +TEST_ORG_ID = uuid.UUID("87654321-4321-8765-4321-876543218765") + + +@pytest.fixture +async def state_session() -> AsyncIterator[AsyncSession]: + previous_sqlite_mode = using_sqlite() + set_sqlite_mode(True) + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + async with engine.begin() as connection: + await connection.run_sync(Base.metadata.create_all) + async with AsyncSession(engine, expire_on_commit=False) as session: + yield session + finally: + await engine.dispose() + set_sqlite_mode(previous_sqlite_mode) + + +async def _import_automation( + session: AsyncSession, lifecycle_fields: dict[str, Any] +) -> Automation: + fields = { + "name": "Git state regression", + "entrypoint": "python main.py", + "trigger": {"type": "cron", "schedule": "* * * * *", "timezone": "UTC"}, + "tarball_source": { + "type": "external", + "url": "https://example.com/automation.tar.gz", + }, + **lifecycle_fields, + } + files = {"automation.yaml": yaml.safe_dump(fields).encode()} + deserialized = deserialize_automation(files) + assert deserialized is not None + # Match the import loop's savepoint: rejected input cannot leave partial rows. + async with session.begin_nested(): + await _create_automation_from_git( + session, + _Owner(TEST_USER_ID, TEST_ORG_ID), + "git-state-regression", + deserialized, + files, + "test-head", + [], + ) + await session.flush() + automation = await session.scalar(select(Automation)) + assert automation is not None + return automation + + +async def test_inactive_state_without_enabled_is_not_scheduled(state_session): + automation = await _import_automation(state_session, {"state": "INACTIVE"}) + scheduled = await _fetch_enabled_automations( + state_session, batch_size=100, poll_threshold=utcnow() + ) + + assert (automation.state, automation.enabled, len(scheduled)) == ( + AutomationState.INACTIVE, + False, + 0, + ), "An explicitly INACTIVE Git definition must not become a scheduled automation" + + +@pytest.mark.parametrize( + "lifecycle_fields", + [ + pytest.param({"state": "INACTIVE", "enabled": True}, id="inactive-enabled"), + pytest.param({"state": "ACTIVE", "enabled": False}, id="active-disabled"), + ], +) +async def test_import_rejects_conflicting_state_and_enabled( + state_session, lifecycle_fields +): + with pytest.raises(ValueError, match="enabled.*state|state.*enabled"): + await _import_automation(state_session, lifecycle_fields) + assert await state_session.scalar(select(Automation.id)) is None + + +@pytest.mark.parametrize( + ("lifecycle_fields", "expected_state", "expected_enabled"), + [ + pytest.param({}, AutomationState.ACTIVE, True, id="legacy-default"), + pytest.param({"enabled": True}, AutomationState.ACTIVE, True, id="legacy-on"), + pytest.param( + {"enabled": False}, AutomationState.INACTIVE, False, id="legacy-off" + ), + pytest.param( + {"enabled": None}, AutomationState.ACTIVE, True, id="legacy-empty" + ), + pytest.param( + {"state": "ACTIVE", "enabled": True}, + AutomationState.ACTIVE, + True, + id="consistent-active", + ), + pytest.param( + {"state": "INACTIVE", "enabled": False}, + AutomationState.INACTIVE, + False, + id="consistent-inactive", + ), + pytest.param( + {"state": "DRAFT", "enabled": False}, + AutomationState.DRAFT, + False, + id="consistent-draft", + ), + ], +) +async def test_import_preserves_legacy_and_consistent_state_inputs( + state_session, lifecycle_fields, expected_state, expected_enabled +): + automation = await _import_automation(state_session, lifecycle_fields) + assert automation.state == expected_state + assert automation.enabled is expected_enabled diff --git a/tests/test_migration_history.py b/tests/test_migration_history.py new file mode 100644 index 00000000..6c1491c6 --- /dev/null +++ b/tests/test_migration_history.py @@ -0,0 +1,39 @@ +"""New migrations must coexist with migration IDs already published on main. + +The fixture snapshot makes the integration regression reproducible offline, +including before a stale feature branch has been rebased onto that history. +""" + +import shutil +import warnings +from pathlib import Path + +from alembic.script import ScriptDirectory + + +def test_migrations_do_not_reuse_published_revision_ids(tmp_path: Path): + repository = Path(__file__).resolve().parents[1] + combined = tmp_path / "migrations" + shutil.copytree(repository / "migrations", combined) + published = Path(__file__).parent / "fixtures" / "published_migrations" + for snapshot in published.glob("*.py.txt"): + destination = combined / "versions" / snapshot.name.removesuffix(".txt") + # After a rebase these migrations are already present in the branch. + if not destination.exists(): + shutil.copyfile(snapshot, destination) + + with warnings.catch_warnings(record=True) as captured: + warnings.simplefilter("always") + heads = ScriptDirectory(str(combined)).get_heads() + + duplicate_revisions = [ + str(warning.message) + for warning in captured + if "is present more than once" in str(warning.message) + ] + assert not duplicate_revisions, ( + "New migrations collide with published main history: " + + "; ".join(duplicate_revisions) + + ". Assign unused revisions and update the down_revision chain." + ) + assert len(heads) == 1, f"Expected one upgrade head after integration, got {heads}" diff --git a/tests/test_router.py b/tests/test_router.py index c42f3fb4..cd90ce69 100644 --- a/tests/test_router.py +++ b/tests/test_router.py @@ -12,6 +12,7 @@ Automation, AutomationDisableEvent, AutomationRun, + AutomationState, TarballUpload, UploadStatus, ) @@ -227,86 +228,55 @@ async def test_delete_as_creator_succeeds(self, readonly_client, async_session): assert response.status_code == 204 - async def _other_users_automation( - self, async_session, *, enabled: bool = True - ) -> Automation: - """Persist an automation created by someone other than the caller.""" + async def test_admin_non_creator_cannot_update_definition( + self, async_client, async_session + ): + """Admins cannot edit code/config that runs as another user.""" automation = Automation( user_id=self._OTHER_USER_ID, org_id=TEST_ORG_ID, - name="Teammate Automation", + name="Owned by someone else", trigger={"type": "cron", "schedule": "0 9 * * *", "timezone": "UTC"}, tarball_path="s3://bucket/code.tar.gz", entrypoint="uv run script.py", - enabled=enabled, ) async_session.add(automation) await async_session.commit() - return automation - - async def test_update_as_non_creator_manager_returns_403( - self, async_client, async_session - ): - """A manager cannot edit another user's automation definition.""" - # Arrange - automation = await self._other_users_automation(async_session) - # Act response = await async_client.patch( f"/api/automation/v1/{automation.id}", - json={"prompt": "Do something else"}, + json={"name": "Hijacked definition"}, ) - # Assert assert response.status_code == 403 assert "creator" in response.json()["detail"] - async def test_disable_as_non_creator_manager_succeeds( + async def test_admin_non_creator_can_update_state( self, async_client, async_session ): - """A manager can turn off another user's automation.""" - # Arrange - automation = await self._other_users_automation(async_session, enabled=True) - - # Act - response = await async_client.patch( - f"/api/automation/v1/{automation.id}", json={"enabled": False} - ) - - # Assert - assert response.status_code == 200 - assert response.json()["enabled"] is False - - async def test_enable_as_non_creator_manager_returns_403( - self, async_client, async_session - ): - """A manager cannot turn another user's automation back on.""" - # Arrange - automation = await self._other_users_automation(async_session, enabled=False) - - # Act - response = await async_client.patch( - f"/api/automation/v1/{automation.id}", json={"enabled": True} + """Admins can activate or deactivate automations they do not own.""" + automation = Automation( + user_id=self._OTHER_USER_ID, + org_id=TEST_ORG_ID, + name="Owned by someone else", + trigger={"type": "cron", "schedule": "0 9 * * *", "timezone": "UTC"}, + tarball_path="s3://bucket/code.tar.gz", + entrypoint="uv run script.py", + enabled=True, + state=AutomationState.ACTIVE, ) + async_session.add(automation) + await async_session.commit() - # Assert - assert response.status_code == 403 - - async def test_disable_with_edits_as_non_creator_manager_returns_403( - self, async_client, async_session - ): - """Turning off cannot carry other edits along with it.""" - # Arrange - automation = await self._other_users_automation(async_session) - - # Act response = await async_client.patch( f"/api/automation/v1/{automation.id}", - json={"enabled": False, "name": "Renamed"}, + json={"state": "INACTIVE"}, ) - # Assert - assert response.status_code == 403 + assert response.status_code == 200 + data = response.json() + assert data["state"] == "INACTIVE" + assert data["enabled"] is False class TestCreateAutomation: @@ -1776,10 +1746,10 @@ async def test_dispatch_automation_not_found(self, async_client): assert response.status_code == 404 assert "Automation not found" in response.json()["detail"] - async def test_dispatch_disabled_automation_returns_reason( + async def test_dispatch_disabled_automation_creates_manual_run( self, async_client, async_session ): - """Dispatching a disabled automation returns its blocking reason.""" + """Manual dispatch is allowed for inactive automations.""" automation = Automation( user_id=TEST_USER_ID, org_id=TEST_ORG_ID, @@ -1798,11 +1768,11 @@ async def test_dispatch_disabled_automation_returns_reason( f"/api/automation/v1/{automation.id}/dispatch" ) - assert response.status_code == 409 - detail = response.json()["detail"] - assert detail["message"] == "Automation is disabled" - assert detail["disabled_reason"] == "auth: Invalid API key" - assert detail["disabled_detail"] == {"kind": "auth", "threshold": 3} + assert response.status_code == 201 + data = response.json() + assert data["automation_id"] == str(automation.id) + assert data["status"] == "PENDING" + assert data["trigger_source"] == "manual" async def test_dispatch_automation_deleted(self, async_client, async_session): """Dispatching a soft-deleted automation returns 404.""" diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 4ee69244..5a38229f 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -7,7 +7,12 @@ import pytest from sqlalchemy import func, select -from openhands.automation.models import Automation, AutomationRun, AutomationRunStatus +from openhands.automation.models import ( + Automation, + AutomationRun, + AutomationRunStatus, + AutomationState, +) from openhands.automation.scheduler import ( POLL_INTERVAL_SECONDS, poll_and_schedule, @@ -473,6 +478,28 @@ async def test_poll_excludes_deleted(self, async_session_factory): assert len(runs) == 0 + async def test_poll_excludes_draft_even_if_enabled_flag_is_true( + self, async_session_factory + ): + """Draft state rows are never scheduled automatically.""" + async with async_session_factory() as session: + automation = Automation( + user_id=TEST_USER_ID, + org_id=TEST_ORG_ID, + name="Draft Automation", + trigger={"type": "cron", "schedule": "* * * * *", "timezone": "UTC"}, + tarball_path="s3://bucket/code.tar.gz", + entrypoint="uv run main.py", + enabled=True, + state=AutomationState.DRAFT, + ) + session.add(automation) + await session.commit() + + runs = await poll_and_schedule(async_session_factory) + + assert runs == [] + async def test_poll_excludes_recently_triggered(self, async_session_factory): """Recently triggered automations are not returned as due.""" now = utcnow() diff --git a/tests/test_webhook_utils.py b/tests/test_webhook_utils.py index eb0f1aed..1e3ec6fc 100644 --- a/tests/test_webhook_utils.py +++ b/tests/test_webhook_utils.py @@ -9,7 +9,7 @@ from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine from openhands.automation.db import set_sqlite_mode -from openhands.automation.models import Automation, Base +from openhands.automation.models import Automation, AutomationState, Base from openhands.automation.utils.webhook import get_event_automations, verify_signature @@ -275,6 +275,47 @@ async def test_excludes_disabled_automations(self, sqlite_session): finally: set_sqlite_mode(False) + @pytest.mark.asyncio + async def test_excludes_draft_automations(self, sqlite_session): + """Draft automations are not returned even if enabled is inconsistent.""" + org_id = uuid.uuid4() + user_id = uuid.uuid4() + + set_sqlite_mode(True) + + try: + active = Automation( + id=uuid.uuid4(), + name="Active Automation", + org_id=org_id, + user_id=user_id, + trigger={"type": "event", "source": "github", "on": "push"}, + tarball_path="test.tar.gz", + entrypoint="main.py", + enabled=True, + state=AutomationState.ACTIVE, + ) + draft = Automation( + id=uuid.uuid4(), + name="Draft Automation", + org_id=org_id, + user_id=user_id, + trigger={"type": "event", "source": "github", "on": "push"}, + tarball_path="test.tar.gz", + entrypoint="main.py", + enabled=True, + state=AutomationState.DRAFT, + ) + + sqlite_session.add_all([active, draft]) + await sqlite_session.commit() + + result = await get_event_automations(org_id, "github", sqlite_session) + + assert [automation.id for automation, _ in result] == [active.id] + finally: + set_sqlite_mode(False) + @pytest.mark.asyncio async def test_excludes_deleted_automations(self, sqlite_session): """Deleted automations are not returned."""