fix: address code review feedback for after_timestamp feature

- Add _make_timestamp_filter() helper to eliminate code duplication
- Convert datetime filters to ISO strings once (O(1) vs O(n) parsing)
- Add unit tests for normalize_datetime_to_server_timezone()
- Add unit tests for _make_timestamp_filter() helper
- Add behavioral tests that verify actual filtering (not just mock calls)
- Add warning log when after_timestamp provided without resend_all
- Add Query description for after_timestamp in OpenAPI docs

Co-authored-by: openhands <openhands@all-hands.dev>

Author: jpshackelford

openhands-agent-server/openhands/agent_server/event_service.py

@@ -1,4 +1,5 @@
 import asyncio
+from collections.abc import Callable
 from dataclasses import dataclass, field
 from datetime import datetime
 from pathlib import Path
@@ -32,6 +33,41 @@
 logger = get_logger(__name__)
 
 
+def _make_timestamp_filter(
+    timestamp__gte: datetime | None,
+    timestamp__lt: datetime | None,
+) -> Callable[[str], bool] | None:
+    """Create a filter function for timestamp comparisons.
+
+    Converts datetime filters to ISO strings once, then returns a function
+    that performs string comparisons. This avoids parsing event.timestamp
+    on every iteration.
+
+    Args:
+        timestamp__gte: Filter for events with timestamp >= this value
+        timestamp__lt: Filter for events with timestamp < this value
+
+    Returns:
+        A function that takes an event timestamp (ISO string) and returns
+        True if it passes the filter, or None if no filtering is needed.
+    """
+    if timestamp__gte is None and timestamp__lt is None:
+        return None
+
+    # Convert datetime filters to ISO strings once (outside the loop)
+    gte_str = timestamp__gte.isoformat() if timestamp__gte else None
+    lt_str = timestamp__lt.isoformat() if timestamp__lt else None
+
+    def passes_filter(event_timestamp: str) -> bool:
+        if gte_str is not None and event_timestamp < gte_str:
+            return False
+        if lt_str is not None and event_timestamp >= lt_str:
+            return False
+        return True
+
+    return passes_filter
+
+
 @dataclass
 class EventService:
     """
@@ -107,6 +143,9 @@ def _search_events_sync(
         if not self._conversation:
             raise ValueError("inactive_service")
 
+        # Create timestamp filter once (converts datetimes to ISO strings)
+        timestamp_filter = _make_timestamp_filter(timestamp__gte, timestamp__lt)
+
         # Collect all events
         all_events = []
         with self._conversation._state as state:
@@ -128,15 +167,11 @@ def _search_events_sync(
                     if not self._event_matches_body(event, body):
                         continue
 
-                # Apply timestamp filters using proper datetime comparison.
-                # Event timestamps are naive (server local time), so filter
-                # datetimes should also be naive or normalized to server timezone.
-                if timestamp__gte is not None or timestamp__lt is not None:
-                    event_dt = datetime.fromisoformat(event.timestamp)
-                    if timestamp__gte is not None and event_dt < timestamp__gte:
-                        continue
-                    if timestamp__lt is not None and event_dt >= timestamp__lt:
-                        continue
+                # Apply timestamp filter (uses string comparison for efficiency)
+                if timestamp_filter is not None and not timestamp_filter(
+                    event.timestamp
+                ):
+                    continue
 
                 all_events.append(event)
 
@@ -208,6 +243,9 @@ def _count_events_sync(
         if not self._conversation:
             raise ValueError("inactive_service")
 
+        # Create timestamp filter once (converts datetimes to ISO strings)
+        timestamp_filter = _make_timestamp_filter(timestamp__gte, timestamp__lt)
+
         count = 0
         with self._conversation._state as state:
             for event in state.events:
@@ -228,15 +266,11 @@ def _count_events_sync(
                     if not self._event_matches_body(event, body):
                         continue
 
-                # Apply timestamp filters using proper datetime comparison.
-                # Event timestamps are naive (server local time), so filter
-                # datetimes should also be naive or normalized to server timezone.
-                if timestamp__gte is not None or timestamp__lt is not None:
-                    event_dt = datetime.fromisoformat(event.timestamp)
-                    if timestamp__gte is not None and event_dt < timestamp__gte:
-                        continue
-                    if timestamp__lt is not None and event_dt >= timestamp__lt:
-                        continue
+                # Apply timestamp filter (uses string comparison for efficiency)
+                if timestamp_filter is not None and not timestamp_filter(
+                    event.timestamp
+                ):
+                    continue
 
                 count += 1
 

openhands-agent-server/openhands/agent_server/sockets.py

@@ -43,7 +43,16 @@ async def events_socket(
     websocket: WebSocket,
     session_api_key: Annotated[str | None, Query(alias="session_api_key")] = None,
     resend_all: Annotated[bool, Query()] = False,
-    after_timestamp: Annotated[datetime | None, Query()] = None,
+    after_timestamp: Annotated[
+        datetime | None,
+        Query(
+            description=(
+                "Filter events to timestamps >= this value when resend_all=True. "
+                "Accepts ISO 8601 format. Timezone-aware datetimes are converted "
+                "to server local time; naive datetimes assumed in server timezone."
+            )
+        ),
+    ] = None,
 ):
     """WebSocket endpoint for conversation events.
 
@@ -84,6 +93,13 @@ async def events_socket(
         else None
     )
 
+    # Warn if after_timestamp is provided without resend_all
+    if after_timestamp and not resend_all:
+        logger.warning(
+            f"after_timestamp provided without resend_all=True, "
+            f"will be ignored: {conversation_id}"
+        )
+
     try:
         # Resend existing events if requested
         if resend_all:

tests/agent_server/test_event_router.py

@@ -1,5 +1,6 @@
 """Tests for event_router.py endpoints."""
 
+from datetime import UTC, datetime, timezone
 from pathlib import Path
 from typing import cast
 from unittest.mock import AsyncMock, MagicMock
@@ -10,14 +11,76 @@
 from fastapi.testclient import TestClient
 
 from openhands.agent_server.dependencies import get_event_service
-from openhands.agent_server.event_router import event_router
+from openhands.agent_server.event_router import (
+    event_router,
+    normalize_datetime_to_server_timezone,
+)
 from openhands.agent_server.event_service import EventService
 from openhands.agent_server.models import SendMessageRequest
 from openhands.sdk import Message
 from openhands.sdk.event.llm_convertible.message import MessageEvent
 from openhands.sdk.llm.message import ImageContent, TextContent
 
 
+class TestNormalizeDatetimeToServerTimezone:
+    """Unit tests for normalize_datetime_to_server_timezone function."""
+
+    def test_naive_datetime_passthrough(self):
+        """Naive datetimes should be returned unchanged."""
+        naive_dt = datetime(2025, 1, 15, 10, 30, 0)
+        result = normalize_datetime_to_server_timezone(naive_dt)
+
+        assert result == naive_dt
+        assert result.tzinfo is None
+
+    def test_utc_datetime_converted_to_naive(self):
+        """UTC datetime should be converted to server local time and made naive."""
+        utc_dt = datetime(2025, 1, 15, 10, 30, 0, tzinfo=UTC)
+        result = normalize_datetime_to_server_timezone(utc_dt)
+
+        # Result should be naive
+        assert result.tzinfo is None
+        # Result should represent the same instant converted to local time
+        expected = utc_dt.astimezone(None).replace(tzinfo=None)
+        assert result == expected
+
+    def test_non_utc_timezone_converted_to_naive(self):
+        """Non-UTC timezone-aware datetime should be converted to server local time."""
+        # Create a timezone-aware datetime in UTC
+        aware_dt = datetime(2025, 6, 15, 18, 30, 0, tzinfo=UTC)
+
+        result = normalize_datetime_to_server_timezone(aware_dt)
+
+        # Result should be naive
+        assert result.tzinfo is None
+        # Result should represent the same instant in server local time
+        expected = aware_dt.astimezone(None).replace(tzinfo=None)
+        assert result == expected
+
+    def test_preserves_microseconds(self):
+        """Microseconds should be preserved through conversion."""
+        utc_dt = datetime(2025, 1, 15, 10, 30, 0, 123456, tzinfo=UTC)
+        result = normalize_datetime_to_server_timezone(utc_dt)
+
+        assert result.microsecond == 123456
+
+    def test_different_fixed_offset_timezone(self):
+        """Test with a specific fixed offset timezone."""
+        # UTC+5:30 (like India Standard Time)
+        from datetime import timedelta
+
+        ist = timezone(timedelta(hours=5, minutes=30))
+        ist_dt = datetime(2025, 1, 15, 16, 0, 0, tzinfo=ist)  # 4:00 PM IST
+
+        result = normalize_datetime_to_server_timezone(ist_dt)
+
+        # Should be naive
+        assert result.tzinfo is None
+        # Should be the same instant converted to server local time
+        expected = ist_dt.astimezone(None).replace(tzinfo=None)
+        assert result == expected
+
+
 @pytest.fixture
 def client():
     """Create a test client for the FastAPI app without authentication."""

tests/agent_server/test_event_router_websocket.py

@@ -647,3 +647,162 @@ async def test_resend_all_without_after_timestamp(
         mock_event_service.search_events.assert_called_once_with(
             page_id=None, timestamp__gte=None
         )
+
+
+class TestAfterTimestampFilteringBehavioral:
+    """Behavioral tests that verify actual filtering works with real events.
+
+    These tests use real EventService instances with mock conversations
+    to verify that timestamp filtering actually produces correct results,
+    not just that the right methods were called.
+    """
+
+    @pytest.fixture
+    def event_service_with_timestamped_events(self):
+        """Create a real EventService with timestamped events for testing."""
+        from pathlib import Path
+
+        from openhands.agent_server.event_service import EventService
+        from openhands.agent_server.models import StoredConversation
+        from openhands.sdk import LLM, Agent
+        from openhands.sdk.security.confirmation_policy import NeverConfirm
+        from openhands.sdk.workspace import LocalWorkspace
+
+        stored = StoredConversation(
+            id=uuid4(),
+            agent=Agent(llm=LLM(model="gpt-4", usage_id="test-llm"), tools=[]),
+            workspace=LocalWorkspace(working_dir="workspace/project"),
+            confirmation_policy=NeverConfirm(),
+            initial_message=None,
+            metrics=None,
+        )
+        service = EventService(
+            stored=stored, conversations_dir=Path("test_conversation_dir")
+        )
+
+        # Create mock conversation with timestamped events
+        from unittest.mock import MagicMock
+
+        from openhands.sdk import Conversation
+        from openhands.sdk.conversation.state import ConversationState
+
+        conversation = MagicMock(spec=Conversation)
+        state = MagicMock(spec=ConversationState)
+
+        # Events with specific timestamps spanning 10:00 to 14:00
+        timestamps = [
+            "2025-01-01T10:00:00.000000",
+            "2025-01-01T11:00:00.000000",
+            "2025-01-01T12:00:00.000000",
+            "2025-01-01T13:00:00.000000",
+            "2025-01-01T14:00:00.000000",
+        ]
+
+        events = []
+        for index, timestamp in enumerate(timestamps, 1):
+            event = MessageEvent(
+                id=f"event{index}",
+                source="user",
+                llm_message=Message(
+                    role="user", content=[TextContent(text=f"Message {index}")]
+                ),
+                timestamp=timestamp,
+            )
+            events.append(event)
+
+        state.events = events
+        state.__enter__ = MagicMock(return_value=state)
+        state.__exit__ = MagicMock(return_value=None)
+        conversation._state = state
+
+        service._conversation = conversation
+        return service
+
+    @pytest.mark.asyncio
+    async def test_timestamp_filter_returns_correct_events(
+        self, event_service_with_timestamped_events
+    ):
+        """Test that timestamp filtering returns only events >= the filter time."""
+        from datetime import datetime
+
+        service = event_service_with_timestamped_events
+
+        # Filter for events >= 12:00:00 (should return events 3, 4, 5)
+        filter_time = datetime(2025, 1, 1, 12, 0, 0)
+        result = await service.search_events(timestamp__gte=filter_time)
+
+        # Should return exactly 3 events
+        assert len(result.items) == 3
+
+        # Verify the correct events were returned
+        returned_ids = [event.id for event in result.items]
+        assert "event3" in returned_ids
+        assert "event4" in returned_ids
+        assert "event5" in returned_ids
+
+        # Events 1 and 2 should NOT be returned
+        assert "event1" not in returned_ids
+        assert "event2" not in returned_ids
+
+    @pytest.mark.asyncio
+    async def test_timestamp_filter_boundary_condition(
+        self, event_service_with_timestamped_events
+    ):
+        """Test that filter boundary is inclusive (>=)."""
+        from datetime import datetime
+
+        service = event_service_with_timestamped_events
+
+        # Filter for events >= exactly 12:00:00 (event3's timestamp)
+        filter_time = datetime(2025, 1, 1, 12, 0, 0)
+        result = await service.search_events(timestamp__gte=filter_time)
+
+        # Event3 at exactly 12:00:00 should be included
+        returned_ids = [event.id for event in result.items]
+        assert "event3" in returned_ids
+
+    @pytest.mark.asyncio
+    async def test_timestamp_filter_no_matches(
+        self, event_service_with_timestamped_events
+    ):
+        """Test that filter returns empty when no events match."""
+        from datetime import datetime
+
+        service = event_service_with_timestamped_events
+
+        # Filter for events >= 15:00:00 (no events exist after 14:00)
+        filter_time = datetime(2025, 1, 1, 15, 0, 0)
+        result = await service.search_events(timestamp__gte=filter_time)
+
+        assert len(result.items) == 0
+
+    @pytest.mark.asyncio
+    async def test_timestamp_filter_all_events_match(
+        self, event_service_with_timestamped_events
+    ):
+        """Test that all events are returned when filter is before all events."""
+        from datetime import datetime
+
+        service = event_service_with_timestamped_events
+
+        # Filter for events >= 09:00:00 (before all events)
+        filter_time = datetime(2025, 1, 1, 9, 0, 0)
+        result = await service.search_events(timestamp__gte=filter_time)
+
+        # Should return all 5 events
+        assert len(result.items) == 5
+
+    @pytest.mark.asyncio
+    async def test_count_events_with_timestamp_filter(
+        self, event_service_with_timestamped_events
+    ):
+        """Test that count_events also respects timestamp filtering."""
+        from datetime import datetime
+
+        service = event_service_with_timestamped_events
+
+        # Count events >= 12:00:00 (should be 3)
+        filter_time = datetime(2025, 1, 1, 12, 0, 0)
+        count = await service.count_events(timestamp__gte=filter_time)
+
+        assert count == 3