Conversation
…swer_type Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
📝 WalkthroughWalkthroughThe change renames the MCP tool from "tavily-search" to "web-search" and extends its interface with two new parameters: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
nemo_skills/mcp/servers/tavily_search_tool.py (1)
62-62: Replace assertion with explicit validation.Assertions can be disabled with Python's
-Oflag and are not suitable for input validation in production code. Use an explicit check and raise aValueErrorinstead.Apply this diff:
- assert answer_type in ["answer", "results"], "Invalid answer type. Choose 'answer' or 'results'." + if answer_type not in ["answer", "results"]: + raise ValueError("Invalid answer_type. Choose 'answer' or 'results'.")Note: If you adopt the
Literaltype suggestion from the previous comment, this validation becomes redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nemo_skills/mcp/servers/tavily_search_tool.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
nemo_skills/mcp/servers/tavily_search_tool.py
50-50: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: unit-tests
🔇 Additional comments (3)
nemo_skills/mcp/servers/tavily_search_tool.py (3)
74-74: LGTM!The
num_resultsparameter is correctly added to the API payload.
111-111: LGTM!The
hide_argsconfiguration correctly reflects the tool rename and includes the new parameters.
132-134: LGTM! The config propagation logic is correct.The loop correctly merges
num_resultsandanswer_typefrom the config into the extra arguments.
| @mcp.tool(name="web-search") | ||
| async def answer( | ||
| query: Annotated[str, Field(description="Search query.")], | ||
| exclude_domains: Annotated[list[str], Field(description="Domains to exclude from the search.")] = [], |
There was a problem hiding this comment.
Replace mutable default argument with None.
Using a mutable list as a default argument can lead to unexpected behavior if the list is modified. Replace with None and initialize within the function.
Apply this diff:
- exclude_domains: Annotated[list[str], Field(description="Domains to exclude from the search.")] = [],
+ exclude_domains: Annotated[list[str] | None, Field(description="Domains to exclude from the search.")] = None,Then update the function body to handle None:
+ if exclude_domains is None:
+ exclude_domains = []
+
api_url = "https://api.tavily.com/search"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
50-50: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In nemo_skills/mcp/servers/tavily_search_tool.py around line 50, replace the
mutable default argument "exclude_domains: Annotated[list[str], Field(...)] =
[]" with "exclude_domains: Annotated[list[str] | None, Field(...)] = None" (or
simply set to None while keeping typing), and then in the function body check if
exclude_domains is None and set it to an empty list (e.g., exclude_domains = []
if exclude_domains is None else exclude_domains) so the default list is created
per-call rather than shared across calls.
| num_results: Annotated[int, Field(description="Number of results to return.")] = 10, | ||
| answer_type: Annotated[ | ||
| str, | ||
| Field( | ||
| description='Type of results to return. Choose "answer" for a concise answer or "results" for a list of results.' | ||
| ), | ||
| ] = "answer", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n nemo_skills/mcp/servers/tavily_search_tool.pyRepository: NVIDIA-NeMo/Skills
Length of output: 6495
🌐 Web query:
Tavily API search endpoint num_results parameter constraints documentation
💡 Result:
The Search endpoint uses the parameter named max_results (default 5). Constraint: integer in the range 0 ≤ x ≤ 20. Also note related limits: chunks_per_source must be 1–3. Source: Tavily API docs. [1][2]
Sources
- Tavily Search API reference (parameters and max_results range). [1]
- Tavily SDK / docs (parameters; chunks_per_source limits). [2]
Fix critical API payload parameter name and add type safety for answer_type.
The code sends "num_results" to the Tavily API payload (line 74), but the API expects "max_results". Additionally, use Literal type for answer_type to enforce valid options at the type level, and add validation for num_results according to Tavily API constraints (0–20).
Update the imports to include Literal:
-from typing import Annotated, Any
+from typing import Annotated, Any, LiteralFix the payload parameter name:
- "num_results": num_results,
+ "max_results": num_results,Use Literal type for answer_type:
answer_type: Annotated[
- str,
+ Literal["answer", "results"],
Field(
description='Type of results to return. Choose "answer" for a concise answer or "results" for a list of results.'
),
] = "answer",Add validation for num_results (API constraint: 0–20):
"""Search the web for a query"""
+ if not (0 <= num_results <= 20):
+ return {"error": "num_results must be between 0 and 20"}
+
api_url = "https://api.tavily.com/search"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
nemo_skills/mcp/servers/tavily_search_tool.py lines 51-57: the API parameter
name and types are incorrect — change the payload key sent to Tavily from
"num_results" to "max_results" where the request is built (around line 74),
update the function parameter annotation to use typing.Literal for answer_type
(Literal["answer","results"]) and import Literal at top, and add explicit
validation for num_results to ensure it's an int within Tavily's allowed range
0–20 (raise/convert and clamp or raise ValueError) before adding it to the
payload.
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com> Signed-off-by: wasiahmad <wasiahmad@ucla.edu>
Signed-off-by: George Armstrong <georgea@nvidia.com> Signed-off-by: Cheng-Ping Hsieh <chsieh@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com> Signed-off-by: dgitman <dgitman@nvidia.com>
1 participant
Summary by CodeRabbit
New Features
num_resultsparameter (default: 10) to customize search result countanswer_typeparameter (default: "answer") to choose response format: "answer" or "results"Refactor
✏️ Tip: You can customize this high-level summary in your review settings.