Enhance SeqSearch with date range, signals and pagination#3
Enhance SeqSearch with date range, signals and pagination#3matt-richardson wants to merge 9 commits into
Conversation
Add enhancements to SeqSearch tool: New parameters: - signalId: Filter by saved signal/search - fromDateUtc/toDateUtc: Date range filtering (more efficient than @timestamp in filter) - afterId: Pagination support for fetching more than 1000 events - timeoutSeconds: Configurable timeout (1-300 seconds) Improvements: - Filter normalization: Accepts "*" as alias for "all events" (empty string) - Catches SeqApiException for filter syntax errors and provides guidance - Date validation - Signal validation Includes tests: - Date range filtering - Timeout handling - Invalid input validation (dates, signals, filter syntax) - Pagination support - Filter normalization
|
@willibrandon - I've also merged |
willibrandon
left a comment
willibrandon
left a comment
There was a problem hiding this comment.
Caught up on main, the feature additions look solid. A few notes on the test side, pick what you want to address.
| }); | ||
|
|
||
| Assert.NotNull(result); | ||
| Assert.NotNull(result.Content); |
There was a problem hiding this comment.
This passes as long as the call returns something non-null, even if the date range was ignored entirely. Most of the other new tests have the same shape (WithTimeout_ReturnsBeforeTimeout, WithVeryShortTimeout_HandlesGracefully, WithAfterId_ReturnsPaginatedResults). To actually exercise the feature, the tests need to seed events with known timestamps or IDs and then assert what came back. The pagination test in particular passes a hardcoded event-test-id that doesn't exist, so it isn't testing pagination, it's testing that an unknown afterId doesn't blow up.
There was a problem hiding this comment.
Agreed. Rewrote 3 of them to seed events and assert properly. Deleted both timeout tests - making them meaningful would be pretty invasive
|
|
||
| Assert.NotNull(result); | ||
| Assert.NotNull(result.Content); | ||
| if (result.IsError) |
There was a problem hiding this comment.
This test passes either way. If the asterisk normalizes to empty and returns events, success. If it doesn't normalize and Seq returns a syntax error, also success (the inner Assert just checks the error message matches one of two strings). The point of normalization is that * should return events, so the test should seed an event first and then assert IsError is false with the seeded event in the content.
There was a problem hiding this comment.
Fixed - seeds an event, asserts IsError=false and marker in content.
| [McpServerTool, Description("Search Seq events with filters, date ranges, signals, pagination, and optional timeout. For date filtering, use fromDateUtc/toDateUtc parameters instead of @Timestamp in the filter expression. For pagination, use afterId with the last event ID from previous results.")] | ||
| public static async Task<List<EventEntity>> SeqSearch( | ||
| SeqConnectionFactory fac, | ||
| [Required] string filter, |
There was a problem hiding this comment.
Quick question on [Required] combined with the README pointing users at "" for all events. Whether [Required] rejects empty string depends on how MCP wires DataAnnotations validation. If it rejects, the documented all-events path is unreachable. If it accepts, the attribute is misleading. Either confirming what actually happens or relaxing the attribute makes the rule explicit.
There was a problem hiding this comment.
Fixed - dropped the [Required], defaulted filter to "", updated docs.
The README documents passing "" to return all events, but the [Required] attribute on a non-nullable string is ambiguous: depending on whether the MCP runtime runs DataAnnotations validation, the documented all-events path is either unreachable or the attribute is misleading. Removing it and defaulting filter to "" makes the rule explicit and keeps the documented behavior intact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous test only asserted Content was non-null; it would have passed even if fromDateUtc/toDateUtc were ignored entirely. Now it seeds a uniquely-tagged event, queries a bracketing range and asserts the event comes back, then queries a future range and asserts nothing matches the marker. Adds shared JSON helpers (GetInnerText, GetEventCount, GetEventIds) on the test base for inspecting tool call results. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous test passed whether or not "*" was normalized: the if (IsError) branch accepted both "Syntax error" and "Invalid filter" substrings, so a Seq syntax error from a non-normalized "*" would have counted as success. Now the test seeds a uniquely-tagged event, calls SeqSearch with filter="*", and asserts no error plus the seeded marker appearing in the result content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous test passed a hardcoded "event-test-id" that did not exist in Seq, so it only verified that an unknown afterId did not throw. Now the test seeds 10 uniquely-tagged events, fetches the first page, takes the last (oldest) event ID as the cursor, fetches the next page, and asserts the second page is non-empty and shares no IDs with the first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The strict pageSize equality on firstIds.Count would flake if Seq's indexing ever lagged behind the seed loop; the inline note tells a future debugger what to soften it to. The cursor-direction note warns that switching to .First() would silently re-include page 1 events, since afterId is exclusive and Seq returns newest-first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both tests only asserted that result and result.Content were non-null when timeoutSeconds was set, which passes whether or not the timeout logic actually works. Meaningfully testing the CTS + cancellation plumbing requires a behaviour-injection seam (TimeProvider plus a mockable event source), which is more invasive than the five-line feature warrants. The non-timeout SeqSearch tests already exercise the same code path with timeoutSeconds = null, so deletion doesn't open a real coverage gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Add enhancements to SeqSearch tool:
New parameters:
Improvements:
Includes tests: