FIX tavily search results return type

[gwarmstrong]

Summary by CodeRabbit

• Bug Fixes

  • Enhanced tool result extraction with improved error handling for failed executions.
  • Added support for processing multiple items from tool results.
  • Improved JSON parsing with fallback to raw text when needed.

• Tests

  • Added test coverage for handling multiple content items from tool results.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

These changes enhance tool result extraction in the MCP client by introducing a helper function to handle multiple content items from tool calls. The _extract_tool_result method now robustly processes multi-item content arrays, structured content, error states, and missing content, replacing the previous single-item fallback logic.

Changes

Cohort / File(s)	Change Summary
Result extraction logic nemo_skills/mcp/clients.py	Adds _extract_item() helper to parse JSON or extract text from individual content items; enhances _extract_tool_result() to handle error results, structured content, missing content, and arrays of multiple content items; improves error logging and centralized item extraction.
Multi-item content test tests/test_mcp_clients.py	Introduces async test test_stdio_client_returns_list_for_multiple_content_items validating that MCPStdioClient correctly returns a list when a tool produces multiple content items without an explicit return type hint.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• _extract_item() logic: Verify JSON parsing fallback and error handling for items without text attributes
• Error handling paths: Confirm all edge cases (isError, missing structuredContent, no content, multi-item arrays) are covered and return expected error formats
• Test coverage: Validate that the new test correctly exercises the multi-item extraction path and captures expected output structure
• Backward compatibility: Ensure existing single-item and error-only results still behave as before

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The pull request title mentions fixing a return type for tavily search results, but the actual changes focus on enhancing MCP client content extraction logic to handle multiple content items and errors in a generic way. The changes do not appear to be specifically about tavily search results.	Update the title to reflect the actual changes, such as 'Enhance MCP tool result extraction to handle multiple content items and errors' or similar, to accurately represent the modifications made to the codebase.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch georgea/fix-num-results-tavily

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

nemo_skills/mcp/clients.py (2)

109-121: Consider using TypeError for invalid content item structure.

The static analysis hints suggest using TypeError when the item doesn't have the expected text attribute, as this is fundamentally a type/structure mismatch rather than an invalid value. This is a minor style consideration.

 def _extract_item(item) -> Any:
     """Extract a JSON-serializable value from a single content item.
 
     Returns the parsed JSON if text is valid JSON, otherwise the raw text.
-    Raises ValueError if the item doesn't have a text attribute.
+    Raises TypeError if the item doesn't have a text attribute.
     """
     text = getattr(item, "text", None)
     if not isinstance(text, str):
-        raise ValueError(f"Content item has no text attribute: {item}")
+        raise TypeError(f"Content item has no text attribute: {item}")
     try:
         return json.loads(text)
     except json.JSONDecodeError:
         return text

Note: If you change to TypeError, you'd also need to update the except ValueError in _extract_tool_result to catch TypeError as well.

---

147-153: Use LOG.exception to include traceback in error logs.

When logging inside an except block, LOG.exception automatically includes the exception traceback, which aids debugging. As flagged by static analysis.

     try:
         if len(content) == 1:
             return _extract_item(content[0])
         return [_extract_item(item) for item in content]
     except ValueError as e:
-        LOG.error("Unsupported content type in tool result: %s", e)
+        LOG.exception("Unsupported content type in tool result: %s", e)
         return {"error": "Unsupported content type returned from tool"}

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f98d6ed and c51164c.

📒 Files selected for processing (2)

• nemo_skills/mcp/clients.py (1 hunks)
• tests/test_mcp_clients.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_mcp_clients.py (1)

nemo_skills/mcp/clients.py (4)

• MCPStdioClient (413-470)
• call_tool (342-343)
• call_tool (404-410)
• call_tool (464-470)

🪛 Ruff (0.14.8)

nemo_skills/mcp/clients.py

117-117: Prefer TypeError exception for invalid type

(TRY004)

---

117-117: Avoid specifying long messages outside the exception class

(TRY003)

---

152-152: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

⏰ 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: unit-tests
• GitHub Check: pre-commit

🔇 Additional comments (2)

tests/test_mcp_clients.py (1)

610-636: LGTM! Good test coverage for multi-item content extraction.

This test effectively validates the new behavior where tools returning lists without explicit return type hints produce multiple content items. The test structure is clean:

• Creates a minimal reproducible server script
• Validates both the return type and content
• Assertion message helps debugging if it fails

One minor suggestion: consider adding an edge case test for a tool returning an empty list to ensure consistent behavior.

nemo_skills/mcp/clients.py (1)

124-153: LGTM! Robust multi-item content extraction.

The refactored _extract_tool_result function properly handles:

• Error results via isError flag
• Structured content passthrough
• Single vs multiple content items (key fix for tavily search results)
• Graceful fallback with consistent error dict format

The distinction between returning a single item vs a list based on len(content) is the correct approach for preserving backward compatibility while supporting multi-item responses.