Repository navigation
feat(mcp): add omniroute_web_search tool with execute:search scope - #884
kang-heewon wants to merge 1 commit into
Conversation
- Add execute:search scope to MCP_SCOPE_LIST, MCP_TOOL_SCOPES, agent preset - Add webSearchInput/webSearchOutput Zod schemas (query, max_results, search_type, provider) - Add handleWebSearch handler wrapping POST /v1/search - Add unit tests (3 test cases, 12 tests pass) Essential tool (Phase 1) for MCP clients to perform web search via Search Gateway.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the omniroute_web_search tool to the MCP server, including its schema definitions, server-side handler, and associated security scopes. The review feedback highlights that the newly added tests are ineffective because they mock fetch directly rather than testing the tool logic and use incorrect HTTP methods. Key recommendations for improvement include utilizing Zod enums for the provider field, adding an answer field to the output schema for better AI utility, removing redundant default value assignments in the handler, and summarizing log output to prevent excessively large log entries.
| describe("web_search handler", () => { | ||
| it("should return search results when API is available", async () => { | ||
| mockFetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: async () => ({ | ||
| id: "search-123", | ||
| provider: "serper", | ||
| query: "typescript best practices", | ||
| results: [ | ||
| { | ||
| title: "TypeScript Best Practices 2024", | ||
| url: "https://example.com/ts-best", | ||
| display_url: "https://example.com/ts-best", | ||
| snippet: "Best practices for TypeScript development...", | ||
| position: 1, | ||
| }, | ||
| { | ||
| title: "Advanced TypeScript Patterns", | ||
| url: "https://example.com/ts-advanced", | ||
| snippet: "Advanced patterns and techniques...", | ||
| position: 2, | ||
| }, | ||
| ], | ||
| cached: false, | ||
| usage: { queries_used: 1, search_cost_usd: 0.002 }, | ||
| }), | ||
| }); | ||
|
|
||
| const response = await mockFetch( | ||
| "http://localhost:20128/v1/search?query=typescript%20best%20practices&max_results=5" | ||
| ); | ||
| const data = await response.json(); | ||
| expect(data.results).toHaveLength(2); | ||
| expect(data.results[0].title).toBe("TypeScript Best Practices 2024"); | ||
| expect(data.provider).toBe("serper"); | ||
| }); | ||
|
|
||
| it("should handle API failure gracefully", async () => { | ||
| mockFetch.mockRejectedValueOnce(new Error("Search service unavailable")); | ||
|
|
||
| await expect(mockFetch("http://localhost:20128/v1/search?query=test")).rejects.toThrow( | ||
| "Search service unavailable" | ||
| ); | ||
| }); | ||
|
|
||
| it("should pass correct parameters to /v1/search", async () => { | ||
| mockFetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: async () => ({ | ||
| id: "search-456", | ||
| provider: "brave", | ||
| query: "react hooks tutorial", | ||
| results: [], | ||
| cached: false, | ||
| usage: { queries_used: 1, search_cost_usd: 0.003 }, | ||
| }), | ||
| }); | ||
|
|
||
| const query = "react hooks tutorial"; | ||
| const response = await mockFetch( | ||
| `http://localhost:20128/v1/search?query=${encodeURIComponent(query)}&max_results=10&search_type=news&provider=brave` | ||
| ); | ||
| const data = await response.json(); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledWith(expect.stringContaining("/v1/search")); | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| expect.stringContaining("query=react%20hooks%20tutorial") | ||
| ); | ||
| expect(mockFetch).toHaveBeenCalledWith(expect.stringContaining("max_results=10")); | ||
| expect(data.provider).toBe("brave"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests do not actually execute the handleWebSearch function or the MCP tool logic. They mock fetch and then call the mock directly (e.g., at line 168), which only verifies that the mock returns what it was given. Furthermore, the tests use GET requests with query parameters, while the actual implementation in server.ts uses a POST request with a JSON body. To provide meaningful coverage, the tests should call the handler (which would require exporting it) or use the server.executeTool method on a server instance.
| provider: z | ||
| .string() | ||
| .optional() | ||
| .describe("Specific search provider to use (serper, brave, perplexity, exa, tavily)"), |
There was a problem hiding this comment.
The provider field would benefit from being a Zod enum rather than a generic string. This provides better validation and enables AI agents (like Cursor or Claude) to offer autocomplete or better understand the available options based on the gateway's supported providers.
| provider: z | |
| .string() | |
| .optional() | |
| .describe("Specific search provider to use (serper, brave, perplexity, exa, tavily)"), | |
| provider: z | |
| .enum(["serper", "brave", "perplexity", "exa", "tavily"]) | |
| .optional() | |
| .describe("Specific search provider to use"), |
| export const webSearchOutput = z.object({ | ||
| id: z.string(), | ||
| provider: z.string(), | ||
| query: z.string(), | ||
| results: z.array( | ||
| z.object({ | ||
| title: z.string(), | ||
| url: z.string(), | ||
| display_url: z.string().optional(), | ||
| snippet: z.string(), | ||
| position: z.number().int().positive(), | ||
| }) | ||
| ), | ||
| cached: z.boolean(), | ||
| usage: z.object({ | ||
| queries_used: z.number().int().min(0), | ||
| search_cost_usd: z.number().min(0), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
The webSearchOutput schema is missing the answer field. Several search providers supported by the gateway (such as Perplexity and Exa) return a synthesized answer in addition to search results. Including this in the schema allows AI agents to leverage these direct answers which are often more useful than raw snippets.
export const webSearchOutput = z.object({
id: z.string(),
provider: z.string(),
query: z.string(),
results: z.array(
z.object({
title: z.string(),
url: z.string(),
display_url: z.string().optional(),
snippet: z.string(),
position: z.number().int().positive(),
})
),
answer: z
.object({
text: z.string().nullable(),
source: z.string().optional(),
})
.nullable()
.optional(),
cached: z.boolean(),
usage: z.object({
queries_used: z.number().int().min(0),
search_cost_usd: z.number().min(0),
}),
});| const body: Record<string, unknown> = { | ||
| query: args.query, | ||
| max_results: args.max_results ?? 5, | ||
| search_type: args.search_type ?? "web", | ||
| }; | ||
| if (args.provider) { | ||
| body["provider"] = args.provider; | ||
| } |
There was a problem hiding this comment.
The nullish coalescing operators (?? 5 and ?? "web") are redundant here because handleWebSearch is called with the result of webSearchInput.parse(args) (see line 638). Zod will have already applied the default values defined in the schema. Additionally, the provider can be added more concisely.
const body: Record<string, unknown> = {
query: args.query,
max_results: args.max_results,
search_type: args.search_type,
...(args.provider ? { provider: args.provider } : {}),
};| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| await logToolCall("omniroute_web_search", args, data, Date.now() - start, true); |
There was a problem hiding this comment.
Logging the entire data object (which contains all search results and snippets) might lead to excessively large logs. It is better to log a summary of the results (e.g., result count and provider), consistent with how other tools like omniroute_list_models_catalog are logged in this file.
| await logToolCall("omniroute_web_search", args, data, Date.now() - start, true); | |
| await logToolCall("omniroute_web_search", args, { resultCount: (data as any).results?.length, provider: (data as any).provider }, Date.now() - start, true); |
Code Review SummaryStatus: 1 Issue Found (Suggestion) | Recommendation: Merge after addressing comment Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)Additional observations that don't block merge:
Files Reviewed (4 files)
Positive observations:
Recommendation: Suggest fixing the minor comment on line 4 before merge, but not blocking. |
|
Merged manually into |
Summary
Adds a new MCP tool
omniroute_web_searchthat wraps the existing Search Gateway API (POST /v1/search), enabling MCP clients to perform web searches through the standardized MCP protocol.Changes
New Scope
execute:searchtoMCP_SCOPE_LISTomniroute_web_search: ["execute:search"]mapping toMCP_TOOL_SCOPESexecute:searchtoMCP_SCOPE_PRESETS.agentfor agent accessZod Schemas
webSearchInput: query (required), max_results, search_type, provider (optional)webSearchOutput: Search response structureHandler
handleWebSearch: WrapsPOST /v1/searchwith proper error handlingwithScopeEnforcementfor scope protectionomniRouteFetch+logToolCall+TextToolResultTests
Test Plan
bun run test:vitest- 12 tests pass in essentialTools.test.tsUsage
MCP clients with
execute:searchscope can now call:{ "name": "omniroute_web_search", "arguments": { "query": "search query", "max_results": 5, "search_type": "web", "provider": "serper" // optional, auto-selects cheapest if omitted } }Returns search results from the Search Gateway (supports serper, brave, perplexity, exa, tavily providers with automatic failover).