feat(harness): add optional Parallel web search - #3242
georgeatparallel wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds an opt-in .parallelWebSearch() on HarnessAgent.Builder that swaps the built-in Tavily web_search for Parallel's anonymous Search MCP (registered through McpServerRegistrar, enableTools=["web_search"], project/version User-Agent), keeps web_fetch as-is, honours disableWebTools(), propagates the choice to both subagent factory paths, and ships 359 lines of focused tests (default path, ordering, connection-failure abort, native MCP result/User-Agent capture, subagent inheritance). Design is sound and the test coverage is genuinely good for a first contribution — no blocking defects found. The findings below are about failure modes of an opt-in external dependency and one pre-existing disableWebTools propagation gap that this option makes visible:
- [Warning]
HarnessAgent.java:2760—required=trueturns a Parallel outage/429 into an agent that cannot be built (and no init timeout is pinned); prefer fail-open with a Tavily fallback, or an explicit strictness option. - [Warning]
HarnessAgent.java:2759— if the remote no longer advertisesweb_search,McpServerRegistrarcloses the client and returns without throwing, so the agent silently ends up with no search tool; assert the tool is present after registration. - [Warning]
HarnessAgentBuilderSupport.java:343— withdisableWebTools() + parallelWebSearch()children still get the keyless-failing Tavily tool (your own test asserts this), and each subagent spawn opens its own blocking MCP handshake. - [Info]
HarnessAgent.java:2762—isReadOnlyis now inherited from the server'sreadOnlyHint, which changes plan-mode/permission behaviour vs. the built-in tool if the live endpoint omits it. - [Info]
HarnessAgentBuilderSupport.java:467— the guard is duplicated on the declared-subagent path; consider a helper, plus surfacing the "connection failure aborts build" andwebHttpClientcaveats indocs/v2harness docs.
Nits: @return this builder Javadoc matches the surrounding style, README wording is clear, and List.of("web_search") + Map.of(...) immutability is fine here.
Verified statically against main at c0d03ccea (diff review only — no local build or test run this cycle). Please note that a hosted smoke test hitting https://search.parallel.ai/mcp from CI would introduce an external dependency in the test suite; the local HttpServer fixture is the right shape and I'd keep it as the only automated path.
Overall: comment only, not an approval — the opt-in surface is good, and addressing the two fail-closed/silent-tool-absent paths above would make it robust. Happy to re-review after updates.
Automated review by github-manager-bot
| parallel.setTransport("http"); | ||
| parallel.setUrl("https://search.parallel.ai/mcp"); | ||
| parallel.setEnableTools(List.of("web_search")); | ||
| parallel.setRequired(true); |
There was a problem hiding this comment.
[Warning] setRequired(true) makes agent construction fail-closed on a third-party, keyless, rate-limited endpoint. McpServerRegistrar turns a handshake failure into McpConnectionException from build(), so a Parallel outage or a 429 during tools/list/initialize becomes "the agent cannot be built at all" rather than "this search call failed". In agentscope-distribution an agent is re-materialised per session, so an outage would surface as session-start failures.
Could this be setRequired(false) plus a connectionFailureHandler that logs and falls back to the existing Tavily web_search (or an explicit parallelWebSearch(Options) strictness flag)? Also note no timeout / initializationTimeout is set here, so McpClientBuilder defaults apply (30s init / 120s request) and build() can block for tens of seconds on an unresponsive endpoint — worth pinning a short init timeout explicitly.
| McpServerConfig parallel = new McpServerConfig(); | ||
| parallel.setTransport("http"); | ||
| parallel.setUrl("https://search.parallel.ai/mcp"); | ||
| parallel.setEnableTools(List.of("web_search")); |
There was a problem hiding this comment.
[Warning] The allowlist is matched against the remote tool name, and the empty-match case is silent. In McpServerRegistrar.registerClient, when listTools() returns nothing that matches enableTools, the wrapper is closed and the method returns without throwing — required is only consulted for thrown connection/registration failures. Since the Tavily web_search is not registered on this branch, a Parallel-side rename (or any schema change to the advertised name) yields an agent with no web_search at all, no warning, and tests here would not catch it because the fixture advertises exactly web_search.
Suggest asserting the capability landed after registration, e.g. if (agentToolkit.getTool("web_search") == null) { /* fail, or fall back to WebSearchTool */ }, so a provider-side change is loud instead of silently removing the tool.
| parallel.setEnableTools(List.of("web_search")); | ||
| parallel.setRequired(true); | ||
| // Identify the project for aggregate MCP usage; never add user identifiers. | ||
| parallel.setHeaders(Map.of("User-Agent", "agentscope-java/" + Version.VERSION)); |
There was a problem hiding this comment.
[Info] Read-only semantics now depend on the remote server. WebTools.WebSearchTool is readOnly = true, whereas an MCP tool's flag comes from the server's annotations.readOnlyHint (see McpClientManager), so if the live Parallel endpoint omits annotations, web_search becomes non-read-only and PlanModeMiddleware (which gates on isReadOnly()) will treat search as a mutating call in plan mode — plus permission-engine prompts may appear where they previously did not. The new test fixture sets readOnlyHint: true, so CI cannot detect the real server omitting it. Consider pinning the read-only flag for this built-in registration rather than inheriting it from a third party.
| final boolean capturedDisableMemoryTools = b.disableMemoryTools; | ||
| final boolean capturedDisableMemoryHooks = b.disableMemoryHooks; | ||
| final var capturedWebHttpClient = b.webHttpClient; | ||
| final boolean capturedParallelWebSearch = b.parallelWebSearch && !b.disableWebTools; |
There was a problem hiding this comment.
[Warning] Two subagent-side consequences of this expression:
- When a caller combines
disableWebTools()withparallelWebSearch(), children end up with the Tavilyweb_search(which requiresTAVILY_API_KEY) —HarnessAgentBuilderSupportnever propagatesdisableWebTools, anddisabledWebToolsSuppressParallelConnectionRegardlessOfSelectionOrderasserts exactly that. Someone who picks Parallel because it is keyless gets a child tool that is advertised to the model but fails at call time. Either propagatedisableWebToolsto sub-builders, or carry the selected provider over instead of falling back to the provider the user turned off. - Each child build performs its own synchronous
initialize()/tools/listround trip tosearch.parallel.ai. In a fan-out (several subagents per turn) that is N anonymous connections to a rate-limited endpoint, each with a 30s default init timeout, on the spawn path. Reusing the parent's client, or registering Parallel once on the shared toolkit, would avoid both the latency and the quota pressure.
| final boolean capturedDisableMemoryTools = b.disableMemoryTools; | ||
| final boolean capturedDisableMemoryHooks = b.disableMemoryHooks; | ||
| final var capturedWebHttpClient = b.webHttpClient; | ||
| final boolean capturedParallelWebSearch = b.parallelWebSearch && !b.disableWebTools; |
There was a problem hiding this comment.
[Info] The same guard is duplicated in buildDeclaredFactory (and its sub.parallelWebSearch() application at line ~572). A tiny helper — e.g. static boolean resolveParallelWebSearch(Builder b) — would keep the general-purpose and declared paths from drifting if the fallback behaviour from the comment above changes. Related docs nit: the webHttpClient non-applicability for the Parallel path is described in the Javadoc but not in the README block, and the README block is also the only place a user learns that a connection failure aborts build(); docs/v2/*/docs/harness/* (where web tools are documented) probably deserves the same paragraph.
AgentScope-Java Version
2.0.4-SNAPSHOT
Description
This adds free, keyless Parallel Search MCP as an explicit alternative for the harness's normal
web_searchtool. Add.parallelWebSearch()toHarnessAgent.builder()to select it. Tavily stays the default, and its existing credentials and behavior are preserved.The builder connects to
https://search.parallel.ai/mcpthrough the existing Java MCP client and registers only the discoveredweb_searchtool. Parallel's schema usesobjectiveandsearch_queries; results include source URLs and excerpts through the existing tool conversion.web_fetchstays unchanged,disableWebTools()prevents the connection, and the toolkit owns cleanup. Requests identify the project asagentscope-java/<version>for aggregate usage measurement. The README explains setup, rate limits, and what search inputs are sent to Parallel.Anonymous Search MCP runs in Fast mode without a Parallel account or API key. DeepSearchQA results and time per task provide independent search-provider evidence. Those benchmarks measure their own evaluation, rather than performance in AgentScope; this PR makes no unverified numeric comparison.
Btw - I'm a devrel engineer at Parallel.
Related proposal: #3241.
Validation:
mvn -pl agentscope-harness spotless:applyandmvn -pl agentscope-harness testpassed (1,047 tests, 14 skipped, no failures/errors). The normal test lifecycle also passed Spotless. Four new focused provider tests passed with no skips, including a real Java MCP fixture for results, errors, cleanup and User-Agent capture. One hosted smoke test through.parallelWebSearch()and the canonical tool call returned useful URLs and excerpts with no credentials. Full multi-module tests were not run.Checklist
mvn spotless:applymvn -pl agentscope-harness test; 14 skips)