Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion openhands/automation/capabilities_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,12 @@ async def _custom_sources(org_id: uuid.UUID, session: AsyncSession) -> list[str]

def _cron_interval_floor() -> int:
"""Shortest interval between fires the scheduler can actually honour."""
return max(POLL_INTERVAL_SECONDS, get_config().service.scheduler_interval_seconds)
settings = get_config().service
return max(
POLL_INTERVAL_SECONDS,
settings.scheduler_interval_seconds,
settings.min_cron_interval_seconds,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including min_cron_interval_seconds in the advertised max(...) only matches the create/update enforcement when this knob is the largest term. At the default 0 the schema validator performs no floor check at all, yet capabilities still advertises max(POLL_INTERVAL_SECONDS, scheduler_interval_seconds). Confirmed on this head: with AUTOMATION_SCHEDULER_INTERVAL_SECONDS=300 and the knob unset, */2 * * * * is accepted at create time while the advertised floor is 300. Routing enforcement through this same helper would close the gap this PR is meant to close.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding min_cron_interval_seconds to the advertised max(...) does not line the advertisement up with enforcement, because the schema validator still only checks that knob when it is > 0. Confirmed on this head with AUTOMATION_SCHEDULER_INTERVAL_SECONDS=300 and the knob unset: */2 * * * * is accepted at create time while _cron_interval_floor() returns 300. In the default configuration the authoritative path enforces nothing while clients are told a 300s floor applies. Routing enforcement through this same helper would close the gap this PR targets.

)


def _cron_errors(trigger: CronTrigger) -> list[DraftValidationError]:
Expand Down
3 changes: 3 additions & 0 deletions openhands/automation/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,8 @@ class ServiceSettings(BaseSettings):
AUTOMATION_WORKSPACE_BASE: Base workspace directory (local mode default)

# Background workers
AUTOMATION_MIN_CRON_INTERVAL_SECONDS: Minimum allowed gap between cron
fire times (default: 0, disabled)
AUTOMATION_SCHEDULER_INTERVAL_SECONDS: Scheduler poll interval (default: 60)
AUTOMATION_SCHEDULER_BATCH_SIZE: Scheduler batch size (default: 50)
AUTOMATION_DISPATCHER_INTERVAL_SECONDS: Dispatcher poll interval (default: 10)
Expand Down Expand Up @@ -604,6 +606,7 @@ class ServiceSettings(BaseSettings):
# Background workers
scheduler_interval_seconds: int = 60
scheduler_batch_size: int = 50
min_cron_interval_seconds: int = Field(default=0, ge=0)
dispatcher_interval_seconds: int = 10
dispatcher_batch_size: int = 10
watchdog_interval_seconds: int = 60
Expand Down
16 changes: 15 additions & 1 deletion openhands/automation/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
model_validator,
)
from pydantic.alias_generators import to_camel
from pydantic_core import PydanticCustomError

from openhands.automation.constants import MODEL_PROFILE_PATTERN
from openhands.automation.models import AutomationState
Expand All @@ -28,6 +29,7 @@
verifier_schemes,
)
from openhands.automation.utils.cron import (
min_interval_seconds,
validate_cron_schedule as validate_cron_schedule_value,
validate_timezone_name,
)
Expand Down Expand Up @@ -68,7 +70,19 @@ class CronTrigger(BaseModel):
@field_validator("schedule")
@classmethod
def validate_cron_schedule(cls, v: str) -> str:
return validate_cron_schedule_value(v)
schedule = validate_cron_schedule_value(v)

# Import lazily to keep the schema/config dependency one-way at import time.
from openhands.automation.config import get_config

floor = get_config().service.min_cron_interval_seconds

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enforcement floor here is only min_cron_interval_seconds, but the effective deployment floor advertised by /capabilities and used by /v1/validate is max(POLL_INTERVAL_SECONDS, scheduler_interval_seconds, min_cron_interval_seconds) (capabilities_router.py:249). With AUTOMATION_SCHEDULER_INTERVAL_SECONDS=300 and AUTOMATION_MIN_CRON_INTERVAL_SECONDS=120, CreatePromptAutomationRequest/UpdateAutomationRequest accept */2 * * * * while capabilities reports a 300s floor and _cron_errors rejects the same schedule. Please enforce the same composed value here (or share one helper) so the authoritative path cannot accept a schedule the deployment advertises as un-honourable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads only min_cron_interval_seconds, but the effective deployment floor used by /capabilities and _cron_errors is max(POLL_INTERVAL_SECONDS, scheduler_interval_seconds, min_cron_interval_seconds) (capabilities_router.py:249). Confirmed on this head with AUTOMATION_SCHEDULER_INTERVAL_SECONDS=300 and AUTOMATION_MIN_CRON_INTERVAL_SECONDS=120: CreatePromptAutomationRequest accepts */2 * * * * while the advertised floor is 300 and _cron_errors rejects the same schedule. Please enforce the composed floor here (or share a helper with _cron_interval_floor) so create/update cannot accept a schedule the deployment advertises as un-honourable.

if floor > 0 and min_interval_seconds(schedule) < floor:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising ValueError inside the field validator turns the floor violation into a Pydantic value_error, so validate_draft returns _schema_errors(e) before _cron_errors can run. On this head, with the knob at 300, a */2 * * * * draft yields ("trigger.schedule", "value_error") rather than the documented interval_too_short (asserted in tests/test_capabilities_router.py:300 and :425), making the structured-code branch at capabilities_router.py:264 unreachable whenever the knob is enabled. Enforcing only in _cron_errors would keep the existing error contract for clients keyed on the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 40fbfac. Fixed with PydanticCustomError("interval_too_short", ...), so _schema_errors preserves the preflight code for both prompt and bundle drafts. The validator remains in place because #217 requires create/update requests to enforce the configured minimum as well. The focused regression fails on the previous head and passes after this change; real HTTP preflight checks confirm the code with the knob set to 300, while floor 0 and the five-minute boundary still pass.

raise PydanticCustomError(
"interval_too_short",
"Cron expression must have at least {floor} seconds between fires",
{"floor": floor},
)
return schedule

@field_validator("timezone")
@classmethod
Expand Down
51 changes: 51 additions & 0 deletions tests/test_capabilities_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@
import uuid

import pytest
from sqlalchemy.ext.asyncio import AsyncSession

from openhands.automation.app import app
from openhands.automation.auth import authenticate_request
from openhands.automation.capabilities_router import validate_draft
from openhands.automation.config import clear_config_cache
from openhands.automation.models import CustomWebhook
from openhands.automation.schemas import ValidateDraftRequest


# Test UUID matching mock_authenticated_user fixture
Expand Down Expand Up @@ -225,6 +228,17 @@ async def test_advertised_cron_floor_follows_the_scheduler_interval(

assert response.json()["triggers"]["cron"]["minIntervalSeconds"] == 300

async def test_advertises_a_higher_configured_cron_floor(
self, async_client, ready_deployment, monkeypatch
):
"""Clients see the same deployment floor enforced by trigger validation."""
monkeypatch.setenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", "900")
clear_config_cache()

response = await async_client.get(CAPABILITIES_URL)

assert response.json()["triggers"]["cron"]["minIntervalSeconds"] == 900


class TestValidateDraft:
"""Tests for POST /v1/validate endpoint."""
Expand Down Expand Up @@ -288,6 +302,43 @@ async def test_schedule_faster_than_the_deployment_floor_is_rejected(
assert body["valid"] is False
assert addressed_errors(body) == [("trigger.schedule", "interval_too_short")]

@pytest.mark.parametrize(
("endpoint", "draft"),
[("/v1/preset/prompt", CRON_DRAFT), ("/v1", BUNDLE_DRAFT)],
)
@pytest.mark.parametrize(
("floor", "schedule", "expected_errors"),
[
(0, "*/2 * * * *", []),
(300, "*/2 * * * *", [("trigger.schedule", "interval_too_short")]),
(300, "*/5 * * * *", []),
],
)
async def test_configured_cron_floor_preserves_preflight_error_codes(
self,
mock_authenticated_user,
monkeypatch,
endpoint,
draft,
floor,
schedule,
expected_errors,
):
monkeypatch.setenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", str(floor))
clear_config_cache()

request = ValidateDraftRequest.model_validate(
preflight(with_trigger(draft, schedule=schedule), endpoint=endpoint)
)
async with AsyncSession() as session:
response = await validate_draft(
request, user=mock_authenticated_user, session=session
)

body = response.model_dump()
assert body["valid"] == (expected_errors == [])
assert addressed_errors(body) == expected_errors

@pytest.mark.parametrize(
("event_pattern", "expected_errors"),
[
Expand Down
16 changes: 16 additions & 0 deletions tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,22 @@ def test_failure_disable_threshold_uses_documented_env_var(self, monkeypatch):
assert settings.failure_disable_threshold == 0


class TestCronIntervalSettings:
def test_default_disables_minimum_cron_interval(self, monkeypatch):
monkeypatch.delenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", raising=False)

assert ServiceSettings().min_cron_interval_seconds == 0

def test_loads_minimum_cron_interval_from_environment(self, monkeypatch):
monkeypatch.setenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", "300")

assert ServiceSettings().min_cron_interval_seconds == 300

def test_rejects_negative_minimum_cron_interval(self):
with pytest.raises(ValueError, match="greater than or equal to 0"):
ServiceSettings(min_cron_interval_seconds=-1)


class TestBasePath:
"""Verify base_path is derived from base_url path + /api/automation."""

Expand Down
26 changes: 26 additions & 0 deletions tests/test_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import pytest
from pydantic import ValidationError

from openhands.automation.config import clear_config_cache
from openhands.automation.schemas import (
AutomationResponse,
AutomationRunResponse,
Expand Down Expand Up @@ -54,6 +55,9 @@ def test_non_utc_aware_datetime_is_unchanged(self):


class TestCronTriggerValidation:
def teardown_method(self):
clear_config_cache()

def test_accepts_valid_cron_and_timezone(self):
trigger = CronTrigger(schedule="0 9 * * *", timezone="America/New_York")

Expand All @@ -70,6 +74,28 @@ def test_rejects_invalid_timezone(self):
with pytest.raises(ValidationError, match="Invalid timezone"):
CronTrigger(schedule="0 9 * * *", timezone="Not/A_Timezone")

def test_default_allows_any_valid_interval(self):
trigger = CronTrigger(schedule="* * * * *")

assert trigger.schedule == "* * * * *"

def test_rejects_schedule_below_configured_interval(self, monkeypatch):
monkeypatch.setenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", "300")
clear_config_cache()

with pytest.raises(ValidationError, match="at least 300 seconds") as exc_info:
CronTrigger(schedule="* * * * *")

assert exc_info.value.errors()[0]["type"] == "interval_too_short"

def test_accepts_schedule_at_configured_interval(self, monkeypatch):
monkeypatch.setenv("AUTOMATION_MIN_CRON_INTERVAL_SECONDS", "300")
clear_config_cache()

trigger = CronTrigger(schedule="*/5 * * * *")

assert trigger.schedule == "*/5 * * * *"


class TestAutomationStateEnabledValidation:
_CREATE_PAYLOAD: ClassVar[dict[str, Any]] = {
Expand Down
Loading