Refactor

[T-rav]

Summary by CodeRabbit

• New Features
  • Unified search with automatic fallback, enriched Markdown content scraping, and a health-check endpoint returning structured JSON.
• Build
  • Docker build now installs from project metadata; container start simplified; new Make targets for development and faster CI.
• Documentation
  • Added CLAUDE.md with integration, workflows, commands, and deployment guidance.
• Tests
  • Extensive unit tests, test builders, fixtures, and mock utilities across clients, services, tools, and models.
• Chores
  • Pre-commit hooks switched to local/system-installed tools; dev tooling consolidated into project metadata; dependency lists pruned.
• Refactor
  • Centralized logging and streamlined server tool registrations.

[coderabbitai]

Warning

Rate limit exceeded

@T-rav has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 38 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 245192f and 5198967.

📒 Files selected for processing (1)

• .github/workflows/docker_mcp.yml (2 hunks)

Walkthrough

Replaces remote pre-commit hooks with local/system entry points; centralizes logging and constants; extracts MCP tools into dedicated modules with Pydantic models; adds search clients, services (search, processor, scraper), extensive tests and fixtures; updates Docker build, Makefile, CI workflow, and dependency declarations.

Changes

Cohort / File(s)	Summary
Pre-commit & dev tooling \.pre-commit-config.yaml, pyproject.toml, requirements-dev.txt, setup.cfg, Makefile	Swap pre-commit hooks from remote repos to local/system entry points; consolidate dev extras in pyproject; remove requirements-dev contents; adjust flake8 rules; add/rename Makefile targets (install-all, dev, dev-demo, dev-setup, ci-fast, etc.).
Documentation CLAUDE.md	Add Claude / WebCat MCP integration guide covering architecture, components, dev/test commands, CI/CD, deployment, debugging, and extension patterns.
Docker image & build docker/Dockerfile, docker/requirements.txt	Change build to copy project metadata and package source, install from package (pyproject) instead of requirements.txt, set workdir under /app/docker, and use a static CMD to run python cli.py demo; remove many dependencies from docker/requirements.txt.
Server wiring, logging & constants docker/mcp_server.py, docker/utils/logging_config.py, docker/constants.py	Centralize logging via setup_logging; add constants module; remove in-file search/scrape logic and register external tool callables (search, health_check) with MCP server; update main to run MCP with SSE transport.
MCP tools docker/tools/search_tool.py, docker/tools/health_check_tool.py	Add async search_tool and health_check_tool that build Pydantic responses and return dicts via model_dump() for MCP serialization; search_tool uses SERPER_API_KEY and fallback flow, processing results into SearchResponse.
API/tool response models docker/api_tools.py	Add Pydantic response models (APISearchToolResponse, APIHealthCheckResponse, APIServerInfoResponse, APIScrapeResponse) and standardize tool return serialization at MCP boundary.
Search clients docker/clients/serper_client.py, docker/clients/duckduckgo_client.py	Add Serper client (POST-based) and optional DuckDuckGo client (duckduckgo_search fallback), mapping external results to APISearchResult and handling missing deps/errors.
Models docker/models/*	Add Pydantic models: APISearchResult, SearchResult, SearchResponse, HealthCheckResponse, ErrorResponse (and related schemas) for typed boundaries.
Search & scraping services docker/services/search_service.py, docker/services/search_processor.py, docker/services/content_scraper.py	Add fetch_with_fallback (Serper → DuckDuckGo), process_search_results (map & enrich), and a content_scraper that fetches URLs, branches on content-type, uses readability/html→markdown conversion, truncation, and error handling.
Test scaffolding & factories docker/tests/builders/*, docker/tests/factories/*, docker/tests/conftest.py	Add test builders, MockHttpResponse and HttpResponseFactory, pytest fixtures for environment, HTTP mocking, and temp test dir.
Unit tests — clients docker/tests/unit/clients/*	Add tests for Serper and DuckDuckGo clients covering success, empty results, exceptions, and defaulting behavior.
Unit tests — services docker/tests/unit/services/*	Add tests for content_scraper, search_processor, and search_service fallback behavior and edge cases.
Unit tests — tools, models & utils docker/tests/unit/tools/*, docker/tests/unit/models/test_models.py, docker/tests/unit/utils/test_logging_config.py	Add async tests for health_check_tool and search_tool, model validation tests, and logging configuration tests (file creation, levels, handlers).
CI workflow .github/workflows/docker_mcp.yml	Modify CI to install project (editable) and pytest extras, add per-job and per-step timeouts, and prefix pytest runs with PYTHONPATH for unit/integration tests.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant MCPClient as MCP Client
  participant MCPServer as MCP Server
  participant SearchTool as search_tool
  participant FetchSvc as fetch_with_fallback
  participant Serper as Serper Client
  participant DDG as DuckDuckGo Client
  participant Proc as process_search_results
  participant Scraper as scrape_search_result

  User->>MCPClient: Search request (query)
  MCPClient->>MCPServer: Call tool "search"
  MCPServer->>SearchTool: invoke search_tool(query)
  SearchTool->>FetchSvc: fetch_with_fallback(query, SERPER_API_KEY)
  alt Serper key used & returns results
    FetchSvc->>Serper: POST /search (query)
    Serper-->>FetchSvc: [APISearchResult...]
    FetchSvc-->>SearchTool: (results, "Serper API")
  else Fallback to DuckDuckGo
    FetchSvc->>DDG: text search (query)
    DDG-->>FetchSvc: [APISearchResult...]
    FetchSvc-->>SearchTool: (results, "DuckDuckGo (free fallback)")
  end
  SearchTool->>Proc: process_search_results(results)
  loop per result
    Proc->>Scraper: scrape_search_result(result)
    Scraper-->>Proc: SearchResult (with Markdown content)
  end
  Proc-->>SearchTool: [SearchResult...]
  SearchTool-->>MCPServer: return SearchResponse.model_dump()
  MCPServer-->>MCPClient: JSON response

Loading

sequenceDiagram
  autonumber
  participant User
  participant MCPClient as MCP Client
  participant MCPServer as MCP Server
  participant HealthTool as health_check_tool

  User->>MCPClient: Health check request
  MCPClient->>MCPServer: Call tool "health_check"
  MCPServer->>HealthTool: invoke health_check_tool()
  HealthTool-->>MCPServer: {"status":"healthy","service":"webcat"}
  MCPServer-->>MCPClient: JSON response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

I nibble bytes beneath the moon,
I stitch the logs and hum a tune,
Serper whispers, DDG hops by,
HTML folds to Markdown sky.
A rabbit patch — tidy and spry. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title Check	❓ Inconclusive	The title “Refactor” is overly generic and does not convey the primary changes in this pull request, which includes a broad overhaul of tooling configurations, introduction of Pydantic models, restructuring of the Docker build, and addition of new services and tests. Such a non-descriptive title makes it difficult for reviewers or future readers to quickly understand the scope and intent of the changes. A title should succinctly highlight the main practical impact or feature of the refactoring.	Please update the title to clearly reflect the main changes—for example, “Refactor MCP server to use Pydantic models and local tooling with expanded services and tests”—so that the review scope is immediately apparent.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

---

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: 14

🧹 Nitpick comments (8)

docker/clients/serper_client.py (1)

40-50: Move empty list return to else block.

For better code structure and readability, the empty list return should be in an else block rather than after the if statement.

Apply this diff:

         # Process and return the search results
         if "organic" in data:
             # Convert to APISearchResult objects
             return [
                 APISearchResult(
                     title=result.get("title", "Untitled"),
                     link=result.get("link", ""),
                     snippet=result.get("snippet", ""),
                 )
                 for result in data["organic"]
             ]
-        return []
+        else:
+            return []

CLAUDE.md (1)

1-560: Excellent comprehensive documentation!

This documentation provides thorough guidance for Claude Code integration, covering architecture, workflows, testing strategies, and engineering principles. The content is well-organized and highly valuable for developers working with the WebCat MCP server.

The markdown linter has flagged several optional formatting improvements (bare URLs, fenced code blocks without language tags, emphasis used instead of headings). These are purely stylistic and do not impact the documentation's effectiveness, so they can be addressed at your discretion.

If you'd like to address the markdown formatting issues, the main patterns are:

1. Add language identifiers to fenced code blocks (e.g., ```bash instead of ```)
2. Use proper heading syntax (e.g., ### Section instead of **Section**)
3. Wrap bare URLs in angle brackets (e.g., <http://localhost:8000>)
4. Ensure blank lines around code blocks

docker/models/health_check_response.py (1)

11-15: Consider adding validation for the status field.

The status field could benefit from stricter typing to ensure only valid health states are used.

Apply this diff to add validation:

+from typing import Literal
+
 from pydantic import BaseModel


 class HealthCheckResponse(BaseModel):
     """Response from health check tool."""

-    status: str
+    status: Literal["healthy", "unhealthy", "degraded"]
     service: str

docker/services/search_processor.py (1)

15-39: Consider async processing for content scraping.

The function processes results sequentially, calling scrape_search_result for each item in a loop. Based on the content_scraper code snippet showing requests.get calls, this creates a blocking I/O bottleneck.

Consider refactoring to:

1. Make process_search_results async
2. Make scrape_search_result async using an async HTTP client (e.g., httpx)
3. Process results concurrently using asyncio.gather

Example structure:

async def process_search_results(results: List[APISearchResult]) -> List[SearchResult]:
    """Processes API search results with concurrent scraping."""
    
    async def process_single(api_result: APISearchResult) -> SearchResult:
        search_result = SearchResult(
            title=api_result.title,
            url=api_result.link,
            snippet=api_result.snippet,
        )
        return await scrape_search_result(search_result)
    
    return await asyncio.gather(*[process_single(r) for r in results])

This would significantly improve performance when processing multiple search results.

docker/clients/duckduckgo_client.py (1)

38-65: Tighten exception handling and logging

Catching bare Exception hides actionable failures, and logger.error(f"... {str(e)}") drops the traceback. Please narrow the exception (e.g., to duckduckgo_search.DuckDuckGoSearchException/requests.RequestException) or, if a broad guard is unavoidable, switch to logger.exception("...") so diagnostics remain.

docker/mcp_server.py (1)

6-78: Run Black to satisfy the CI formatter.

CI reports that Black would reformat this file. Please run black docker/mcp_server.py (or your usual formatter task) and commit the result so the pipeline passes.

docker/tests/unit/services/test_search_processor.py (2)

19-36: Expand assertions to verify all fields and mock calls.

The test validates title and content, but does not verify:

• The url field (ensure api_result.link → search_result.url mapping works)
• The snippet field
• That mock_scrape was called exactly once
• That mock_scrape received the correct SearchResult argument

Consider adding:

         # Assert
         assert len(results) == 1
         assert results[0].title == "Test"
+        assert results[0].url == "https://test.com"
+        assert results[0].snippet == "Snippet"
         assert results[0].content == "Content"
+        mock_scrape.assert_called_once()

---

38-60: Consider adding assertions for other fields and mock call count.

The test validates result count and titles but could be more comprehensive by also checking:

• url, snippet, and content fields for both results
• mock_scrape.call_count == 2

Optional enhancement:

         # Assert
         assert len(results) == 2
         assert results[0].title == "Test1"
+        assert results[0].url == "https://test1.com"
+        assert results[0].content == "C1"
         assert results[1].title == "Test2"
+        assert results[1].url == "https://test2.com"
+        assert results[1].content == "C2"
+        assert mock_scrape.call_count == 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f105b3e and 94bd718.

📒 Files selected for processing (39)

• .pre-commit-config.yaml (1 hunks)
• CLAUDE.md (1 hunks)
• Makefile (3 hunks)
• docker/Dockerfile (2 hunks)
• docker/api_tools.py (2 hunks)
• docker/clients/duckduckgo_client.py (1 hunks)
• docker/clients/serper_client.py (1 hunks)
• docker/constants.py (1 hunks)
• docker/mcp_server.py (1 hunks)
• docker/models/api_search_result.py (1 hunks)
• docker/models/error_response.py (1 hunks)
• docker/models/health_check_response.py (1 hunks)
• docker/models/search_response.py (1 hunks)
• docker/models/search_result.py (1 hunks)
• docker/requirements.txt (0 hunks)
• docker/services/content_scraper.py (1 hunks)
• docker/services/search_processor.py (1 hunks)
• docker/services/search_service.py (1 hunks)
• docker/tests/builders/__init__.py (1 hunks)
• docker/tests/builders/search_result_builder.py (1 hunks)
• docker/tests/conftest.py (1 hunks)
• docker/tests/factories/__init__.py (1 hunks)
• docker/tests/factories/http_response_factory.py (1 hunks)
• docker/tests/factories/mock_http_response.py (1 hunks)
• docker/tests/unit/clients/test_duckduckgo_client.py (1 hunks)
• docker/tests/unit/clients/test_serper_client.py (1 hunks)
• docker/tests/unit/models/test_models.py (1 hunks)
• docker/tests/unit/services/test_content_scraper.py (1 hunks)
• docker/tests/unit/services/test_search_processor.py (1 hunks)
• docker/tests/unit/services/test_search_service.py (1 hunks)
• docker/tests/unit/tools/test_health_check_tool.py (1 hunks)
• docker/tests/unit/tools/test_search_tool.py (1 hunks)
• docker/tests/unit/utils/test_logging_config.py (1 hunks)
• docker/tools/health_check_tool.py (1 hunks)
• docker/tools/search_tool.py (1 hunks)
• docker/utils/logging_config.py (1 hunks)
• pyproject.toml (3 hunks)
• requirements-dev.txt (0 hunks)
• setup.cfg (1 hunks)

💤 Files with no reviewable changes (2)

• docker/requirements.txt
• requirements-dev.txt

🧰 Additional context used

🧬 Code graph analysis (20)

docker/clients/serper_client.py (2)

docker/tests/factories/mock_http_response.py (2)

• json (60-64)
• raise_for_status (48-53)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tools/search_tool.py (3)

docker/models/search_response.py (1)

• SearchResponse (15-21)

docker/services/search_processor.py (1)

• process_search_results (15-39)

docker/services/search_service.py (1)

• fetch_with_fallback (18-46)

docker/tests/unit/models/test_models.py (4)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/models/error_response.py (1)

• ErrorResponse (13-18)

docker/models/health_check_response.py (1)

• HealthCheckResponse (11-15)

docker/models/search_response.py (1)

• SearchResponse (15-21)

docker/tests/unit/utils/test_logging_config.py (2)

docker/utils/logging_config.py (1)

• setup_logging (21-71)

docker/tests/conftest.py (1)

• temp_test_dir (75-79)

docker/services/search_processor.py (2)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/services/content_scraper.py (1)

• scrape_search_result (21-167)

docker/tests/conftest.py (2)

docker/tests/builders/search_result_builder.py (3)

• a_search_result (66-68)
• a_wikipedia_article (71-78)
• build (56-63)

docker/tests/factories/http_response_factory.py (2)

• HttpResponseFactory (13-96)
• success (22-34)

docker/clients/duckduckgo_client.py (1)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/api_tools.py (1)

docker/tests/factories/http_response_factory.py (1)

• success (22-34)

docker/tests/unit/services/test_content_scraper.py (3)

docker/services/content_scraper.py (1)

• scrape_search_result (21-167)

docker/tests/builders/search_result_builder.py (4)

• a_search_result (66-68)
• with_title (29-32)
• build (56-63)
• with_url (34-37)

docker/tests/factories/http_response_factory.py (8)

• HttpResponseFactory (13-96)
• html_with_title (37-40)
• plaintext (43-45)
• pdf (48-52)
• error_404 (55-64)
• timeout (79-86)
• success (22-34)
• connection_error (89-96)

docker/tests/unit/clients/test_serper_client.py (1)

docker/clients/serper_client.py (1)

• fetch_search_results (19-53)

docker/services/content_scraper.py (2)

docker/tests/factories/http_response_factory.py (1)

• timeout (79-86)

docker/tests/factories/mock_http_response.py (1)

• raise_for_status (48-53)

docker/tests/unit/tools/test_search_tool.py (1)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tests/unit/clients/test_duckduckgo_client.py (2)

docker/tests/unit/tools/test_search_tool.py (1)

• test_returns_search_results (22-45)

docker/tests/unit/clients/test_serper_client.py (1)

• test_handles_missing_fields_with_defaults (84-101)

docker/tests/unit/services/test_search_service.py (2)

docker/services/search_service.py (1)

• fetch_with_fallback (18-46)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tools/health_check_tool.py (1)

docker/models/health_check_response.py (1)

• HealthCheckResponse (11-15)

docker/services/search_service.py (2)

docker/clients/serper_client.py (1)

• fetch_search_results (19-53)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/mcp_server.py (3)

docker/utils/logging_config.py (1)

• setup_logging (21-71)

docker/tools/health_check_tool.py (1)

• health_check_tool (11-23)

docker/tools/search_tool.py (1)

• search_tool (23-63)

docker/models/search_response.py (1)

docker/models/search_result.py (1)

• SearchResult (13-19)

docker/tests/unit/services/test_search_processor.py (2)

docker/services/search_processor.py (1)

• process_search_results (15-39)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tests/factories/http_response_factory.py (1)

docker/tests/factories/mock_http_response.py (1)

• MockHttpResponse (13-64)

🪛 checkmake (0.2.2)

Makefile

[warning] 118-118: Target body for "dev" exceeds allowed length of 5 (6).

(maxbodylength)

---

[warning] 221-221: Target body for "ci" exceeds allowed length of 5 (16).

(maxbodylength)

🪛 GitHub Actions: CI Pipeline

docker/tests/unit/models/test_models.py

[error] 1-1: Black formatting check failed. The file would be reformatted by Black. Run 'black' to fix code style issues.

docker/tests/unit/services/test_content_scraper.py

[error] 1-1: TestContentScraperEdgeCases.test_truncates_content_exceeding_max_length failed: expected '[content truncated]' to be present in scraped.content, but got content including a MagicMock representation ("# Default Test Article\n\n*Source: https://example.com/test*\n\n").

docker/services/content_scraper.py

[error] 1-1: Black formatting check failed. The file would be reformatted by Black. Run 'black' to fix code style issues.

Makefile

[error] 1-1: format-check step failed (exit code 1) due to Black formatting issues. Run 'make format' or 'black' to fix.

docker/tests/unit/services/test_search_service.py

[error] 1-1: Black formatting check failed. The file would be reformatted by Black. Run 'black' to fix code style issues.

docker/mcp_server.py

[error] 1-1: Black formatting check failed. The file would be reformatted by Black. Run 'black' to fix code style issues.

🪛 markdownlint-cli2 (0.18.1)

CLAUDE.md

19-19: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

106-106: Bare URL used

(MD034, no-bare-urls)

---

107-107: Bare URL used

(MD034, no-bare-urls)

---

108-108: Bare URL used

(MD034, no-bare-urls)

---

109-109: Bare URL used

(MD034, no-bare-urls)

---

119-119: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

140-140: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

193-193: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

201-201: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

210-210: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

273-273: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

279-279: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

333-333: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

348-348: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

359-359: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/2/3

(MD029, ol-prefix)

---

360-360: Ordered list item prefix
Expected: 2; Actual: 3; Style: 1/2/3

(MD029, ol-prefix)

---

361-361: Ordered list item prefix
Expected: 3; Actual: 4; Style: 1/2/3

(MD029, ol-prefix)

---

362-362: Ordered list item prefix
Expected: 4; Actual: 5; Style: 1/2/3

(MD029, ol-prefix)

---

367-367: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

373-373: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/2/3

(MD029, ol-prefix)

---

374-374: Ordered list item prefix
Expected: 2; Actual: 3; Style: 1/2/3

(MD029, ol-prefix)

---

375-375: Ordered list item prefix
Expected: 3; Actual: 4; Style: 1/2/3

(MD029, ol-prefix)

---

376-376: Ordered list item prefix
Expected: 4; Actual: 5; Style: 1/2/3

(MD029, ol-prefix)

---

383-383: Bare URL used

(MD034, no-bare-urls)

---

403-403: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

410-410: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

418-418: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

425-425: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

435-435: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

443-443: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

451-451: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

459-459: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

469-469: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

477-477: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

485-485: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

493-493: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

501-501: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

511-511: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

521-521: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

529-529: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

537-537: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

545-545: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🪛 Ruff (0.13.2)

docker/clients/serper_client.py

35-35: Probable use of requests call without timeout

(S113)

---

50-50: Consider moving this statement to an else block

(TRY300)

---

51-51: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

52-52: Use explicit conversion flag

Replace with conversion flag

(RUF010)

docker/tools/search_tool.py

23-23: Unused function argument: ctx

(ARG001)

docker/tests/conftest.py

67-67: Unused function argument: args

(ARG001)

---

67-67: Unused function argument: kwargs

(ARG001)

docker/clients/duckduckgo_client.py

63-63: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

64-64: Use explicit conversion flag

Replace with conversion flag

(RUF010)

docker/api_tools.py

116-116: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

117-117: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

158-158: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

159-159: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

201-201: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

202-202: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

244-244: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

245-245: Use explicit conversion flag

Replace with conversion flag

(RUF010)

docker/tests/factories/mock_http_response.py

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

(TRY003)

docker/services/content_scraper.py

128-128: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

136-136: Do not catch blind exception: Exception

(BLE001)

---

139-139: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

161-161: Consider moving this statement to an else block

(TRY300)

---

163-163: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

165-165: Do not catch blind exception: Exception

(BLE001)

---

166-166: Use explicit conversion flag

Replace with conversion flag

(RUF010)

docker/tests/unit/services/test_search_processor.py

85-85: Local variable results is assigned to but never used

Remove assignment to unused variable results

(F841)

🔇 Additional comments (14)

docker/tests/builders/__init__.py (1)

1-7: LGTM!

The package initializer correctly establishes the test builders module with appropriate license header and descriptive docstring.

docker/tests/factories/__init__.py (1)

1-7: LGTM!

The package initializer correctly establishes the test factories module with appropriate license header and descriptive docstring.

setup.cfg (1)

3-4: LGTM!

The Flake8 configuration now enables complexity (C) and bugbear (B) checks while appropriately ignoring B008 (function calls in argument defaults), which is a common pattern in frameworks like Pydantic and FastAPI.

docker/models/error_response.py (1)

13-18: LGTM!

The ErrorResponse model provides a clean, standardized structure for error payloads at MCP boundaries with appropriate required and optional fields.

docker/models/api_search_result.py (1)

11-22: LGTM!

The APISearchResult model appropriately represents raw search results with flexible field mapping via extra = "allow" to handle variations in API response formats (link/url/href).

.pre-commit-config.yaml (1)

27-61: LGTM!

Migrating from remote tool repos to local system hooks ensures version consistency across pre-commit, CI, and local development. All tools now reference the versions specified in pyproject.toml, eliminating potential mismatches. The configuration correctly preserves all arguments and config paths.

This change aligns with the "Single Source of Truth" philosophy documented in CLAUDE.md and requires developers to have the tools installed locally (via pip install -e ".[dev]").

docker/tests/unit/tools/test_health_check_tool.py (1)

13-33: LGTM!

The test coverage is comprehensive for the health check tool. Both test methods appropriately validate the response structure and content.

docker/tests/unit/models/test_models.py (1)

17-104: LGTM!

The test coverage for all Pydantic models is comprehensive and well-structured. Tests appropriately validate required fields, optional fields, and default values.

docker/tests/unit/clients/test_duckduckgo_client.py (2)

14-77: LGTM!

The test suite for DuckDuckGo client is comprehensive and covers all critical scenarios: successful results, max_results parameter, exception handling, and missing fields with defaults. The mocking strategy properly handles the DDGS context manager.

---

79-88: LGTM!

The test appropriately validates the fallback behavior when the duckduckgo-search library is unavailable.

docker/tools/health_check_tool.py (1)

11-23: LGTM!

The health check tool implementation follows best practices by using a typed Pydantic model and converting to dict only at the MCP boundary for JSON serialization. The logic is correct and well-documented.

docker/models/search_response.py (1)

15-21: LGTM!

The SearchResponse model is well-structured with appropriate field types and defaults. The optional error field with None default provides good error handling support.

docker/tests/unit/clients/test_serper_client.py (1)

14-101: LGTM!

The test suite for Serper client is comprehensive and covers all critical scenarios: successful results parsing, empty results, exception handling, correct API endpoint usage, and missing fields with appropriate defaults. The mocking strategy is appropriate and tests are well-structured.

docker/tests/unit/services/test_search_processor.py (1)

62-69: LGTM!

The empty-list edge case is correctly tested, and the assertion that the scraper is not called is appropriate.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Propagate errors from the search backend

When webcat_functions["search"] returns an "error" (e.g., no results or upstream failure), we still build a success=True response. Clients never see the failure state.

-                result: dict = await webcat_functions["search"](query)
-
-                # Limit results if specified
-                note: str = ""
-                results: List[Dict[str, Any]] = result.get("results", [])
+                result: dict = await webcat_functions["search"](query)
+                error = result.get("error")
+                if error:
+                    response = APISearchToolResponse(
+                        success=False,
+                        query=query,
+                        max_results=max_results,
+                        search_source=result.get("search_source", "Unknown"),
+                        results=[],
+                        total_found=0,
+                        error=error,
+                    )
+                    return response.model_dump()
+
+                # Limit results if specified
+                note: str = ""
+                results: List[Dict[str, Any]] = result.get("results", [])

This ensures the tool surfaces backend failures correctly.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if "search" in webcat_functions:
	result = await webcat_functions["search"](query)
	result: dict = await webcat_functions["search"](query)
	# Limit results if specified
	if result.get("results") and len(result["results"]) > max_results:
	result["results"] = result["results"][:max_results]
	result["note"] = f"Results limited to {max_results} items"
	return {
	"success": True,
	"query": query,
	"max_results": max_results,
	"search_source": result.get("search_source", "Unknown"),
	"results": result.get("results", []),
	"total_found": len(result.get("results", [])),
	"note": result.get("note", ""),
	}
	note: str = ""
	results: List[Dict[str, Any]] = result.get("results", [])
	if results and len(results) > max_results:
	results = results[:max_results]
	note = f"Results limited to {max_results} items"
	# Build typed response
	response = APISearchToolResponse(
	success=True,
	query=query,
	max_results=max_results,
	search_source=result.get("search_source", "Unknown"),
	results=results,
	total_found=len(results),
	note=note,
	)
	# Only convert to dict at MCP boundary for JSON serialization
	return response.model_dump()
	else:
	return {"error": "Search function not available"}
	response = APISearchToolResponse(
	success=False,
	query=query,
	max_results=max_results,
	search_source="Unknown",
	results=[],
	total_found=0,
	error="Search function not available",
	)
	return response.model_dump()
	if "search" in webcat_functions:
	result: dict = await webcat_functions["search"](query)
	error = result.get("error")
	if error:
	response = APISearchToolResponse(
	success=False,
	query=query,
	max_results=max_results,
	search_source=result.get("search_source", "Unknown"),
	results=[],
	total_found=0,
	error=error,
	)
	return response.model_dump()
	# Limit results if specified
	note: str = ""
	results: List[Dict[str, Any]] = result.get("results", [])
	if results and len(results) > max_results:
	results = results[:max_results]
	note = f"Results limited to {max_results} items"
	# Build typed response
	response = APISearchToolResponse(
	success=True,
	query=query,
	max_results=max_results,
	search_source=result.get("search_source", "Unknown"),
	results=results,
	total_found=len(results),
	note=note,
	)
	# Only convert to dict at MCP boundary for JSON serialization
	return response.model_dump()
	else:
	response = APISearchToolResponse(
	success=False,
	query=query,
	max_results=max_results,
	search_source="Unknown",
	results=[],
	total_found=0,
	error="Search function not available",
	)
	return response.model_dump()

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Add timeout to prevent indefinite hangs.

The POST request lacks a timeout parameter, which can cause the application to hang indefinitely if the Serper API is unresponsive.

Apply this diff to add a timeout:

-        response = requests.post(url, headers=headers, data=payload)
+        response = requests.post(url, headers=headers, data=payload, timeout=30)

As per learnings: "Always set timeouts (per-request or global) to avoid hanging calls."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	response = requests.post(url, headers=headers, data=payload)
	response = requests.post(url, headers=headers, data=payload, timeout=30)

🧰 Tools

🪛 Ruff (0.13.2)

35-35: Probable use of requests call without timeout

(S113)

🤖 Prompt for AI Agents

In docker/clients/serper_client.py around line 35, the requests.post call
doesn't set a timeout which can cause the process to hang if Serper is
unresponsive; update the call to include a sensible timeout (for example
timeout=10 or a configurable value), and propagate/handle
requests.exceptions.Timeout and other requests exceptions as appropriate so
callers can fail fast or retry instead of blocking indefinitely.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Improve exception handling specificity and logging.

The current implementation has several issues:

1. Catching bare Exception is too broad and may mask unexpected errors
2. Using logging.error instead of logging.exception loses traceback information
3. The f-string could use explicit conversion for better formatting

Apply this diff to improve error handling:

-    except Exception as e:
-        logger.error(f"Error fetching search results: {str(e)}")
+    except requests.RequestException as e:
+        logger.exception("Error fetching search results from Serper API: %s", e)
         return []

This change:

• Catches specific requests.RequestException to avoid masking unexpected errors
• Uses logging.exception to automatically include traceback
• Uses string formatting instead of f-string for consistency with logging best practices

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	except Exception as e:
	logger.error(f"Error fetching search results: {str(e)}")
	return []
	except requests.RequestException as e:
	logger.exception("Error fetching search results from Serper API: %s", e)
	return []

🧰 Tools

🪛 Ruff (0.13.2)

51-51: Do not catch blind exception: Exception

(BLE001)

---

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

Replace with exception

(TRY400)

---

52-52: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🤖 Prompt for AI Agents

In docker/clients/serper_client.py around lines 51 to 53, replace the broad
except Exception block with a specific except requests.RequestException clause,
log the error using logger.exception to include the traceback, and use
logger.exception with a formatted message (e.g. logger.exception("Error fetching
search results: %s", e)) so the exception type is specific, traceback is
preserved, and logging follows formatting best practices; keep the return []
after the exception handler.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Copy the full package tree before installing

Only copying docker/*.py leaves out subpackages (e.g., docker/services, docker/tools, docker/tests) and the sibling customgpt package that setuptools is configured to include. Because pip install --no-cache-dir /app runs after this copy, the build will fail (or produce a broken wheel) due to missing source files. Please copy the entire project directories instead:

-# Copy docker module (source code)
-COPY docker/*.py /app/docker/
+# Copy project source
+COPY docker/ /app/docker/
+COPY customgpt/ /app/customgpt/

(Adjust the list if additional top-level packages exist.)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In docker/Dockerfile around lines 9 to 16, the Dockerfile currently copies only
docker/*.py (and examples) then runs pip install /app, which omits subpackages
and sibling packages required by setuptools; update the COPY steps to copy the
entire project tree (all top-level packages and subdirectories, e.g., copy the
whole docker/ directory, customgpt/, pyproject.toml, setup.cfg or setup.py, and
any other top-level source dirs) into /app before running pip install so the
installer has the full source tree available during build.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fix package-relative imports for tool registration.

docker.mcp_server is part of the docker package, but these lines attempt to import tools.* as a top-level module. That works only when running python docker/mcp_server.py with the docker/ directory on sys.path. Once the project is installed (or invoked via python -m docker.mcp_server), those imports fail with ModuleNotFoundError because tools actually lives under the docker package. Switch to package-relative imports to keep the entry point working in both contexts.

-from tools.health_check_tool import health_check_tool
-from tools.search_tool import search_tool
+from .tools.health_check_tool import health_check_tool
+from .tools.search_tool import search_tool

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	from tools.health_check_tool import health_check_tool
	from tools.search_tool import search_tool
	# Handle LaTeX/MathJax by preserving the markup
	for math in soup.find_all(["math", "script"]):
	if math.name == "script" and math.get("type") in [
	"math/tex",
	"math/tex; mode=display",
	"application/x-mathjax-config",
	]:
	# Preserve math content
	math.replace_with(f"$$${math.string}$$$")
	elif math.name == "math":
	# Preserve MathML
	math.replace_with(f"$$${str(math)}$$$")
	# Get the markdown content
	markdown_text = h.handle(str(soup))
	# Add title and metadata at the beginning
	full_content = f"# {title}\n\n*Source: {result.url}*\n\n{markdown_text}"
	except Exception as e:
	# Fallback to direct HTML to Markdown conversion if readability fails
	logging.warning(
	f"Readability parsing failed: {str(e)}. Falling back to direct HTML parsing."
	)
	h = html2text.HTML2Text()
	h.ignore_links = False
	h.ignore_images = False
	h.body_width = 0
	soup = BeautifulSoup(response.content, "html.parser")
	title_tag = soup.find("title")
	title = title_tag.text if title_tag else result.title
	full_content = (
	f"# {title}\n\n*Source: {result.url}*\n\n{h.handle(str(soup))}"
	)
	# Limit content length to prevent huge responses
	if len(full_content) > 8000:
	full_content = full_content[:8000] + "... [content truncated]"
	result.content = full_content
	return result
	except requests.RequestException as e:
	result.content = f"Error: Failed to retrieve the webpage. {str(e)}"
	return result
	except Exception as e:
	result.content = f"Error: Failed to scrape content. {str(e)}"
	return result
	def process_search_results(results: List[Dict[str, Any]]) -> List[SearchResult]:
	"""
	Processes raw search results into SearchResult objects with content.
	Args:
	results: List of raw search result dictionaries
	Returns:
	List of SearchResult objects with scraped content
	"""
	processed_results = []
	for result in results:
	# Create a SearchResult object
	search_result = SearchResult(
	title=result.get("title", "Untitled"),
	url=result.get("link", ""),
	snippet=result.get("snippet", ""),
	)
	# Scrape content for the result
	search_result = scrape_search_result(search_result)
	processed_results.append(search_result)
	return processed_results
	def fetch_duckduckgo_search_results(
	query: str, max_results: int = 3
	) -> List[Dict[str, Any]]:
	"""
	Fetches search results from DuckDuckGo as a free fallback.
	Args:
	query: The search query
	max_results: Maximum number of results to return
	Returns:
	A list of search result dictionaries
	"""
	if not DDGS:
	logging.error("DuckDuckGo search not available (library not installed)")
	return []
	try:
	logging.info(f"Using DuckDuckGo fallback search for: {query}")
	with DDGS() as ddgs:
	# Get search results from DuckDuckGo
	results = []
	search_results = ddgs.text(query, max_results=max_results)
	for result in search_results:
	# Convert DuckDuckGo result format to match Serper format
	results.append(
	{
	"title": result.get("title", "Untitled"),
	"link": result.get("href", ""),
	"snippet": result.get("body", ""),
	}
	)
	logging.info(f"DuckDuckGo returned {len(results)} results")
	return results
	except Exception as e:
	logging.error(f"Error fetching DuckDuckGo search results: {str(e)}")
	return []
	# Create a search tool
	@mcp_server.tool(
	# Register tools with MCP server
	mcp_server.tool(
	name="search",
	description="Search the web for information using Serper API or DuckDuckGo fallback",
	)
	async def search_tool(query: str, ctx=None):
	"""Search the web for information on a given query."""
	logging.info(f"Processing search request: {query}")
	results = []
	search_source = "Unknown"
	# Try Serper API first if key is available
	if SERPER_API_KEY:
	logging.info("Using Serper API for search")
	search_source = "Serper API"
	results = fetch_search_results(query, SERPER_API_KEY)
	# Fall back to DuckDuckGo if no API key or no results from Serper
	if not results:
	if not SERPER_API_KEY:
	logging.info("No Serper API key configured, using DuckDuckGo fallback")
	else:
	logging.warning("No results from Serper API, trying DuckDuckGo fallback")
	search_source = "DuckDuckGo (free fallback)"
	results = fetch_duckduckgo_search_results(query)
	# Check if we got any results
	if not results:
	logging.warning(f"No search results found for query: {query}")
	return {
	"error": "No search results found from any source.",
	"query": query,
	"search_source": search_source,
	}
	# Process the results
	processed_results = process_search_results(results)
	# Return formatted results
	return {
	"query": query,
	"search_source": search_source,
	"results": [result.model_dump() for result in processed_results],
	}
	)(search_tool)
	# Create a simple health check tool
	@mcp_server.tool(name="health_check", description="Check the health of the server")
	async def health_check():
	"""Check the health of the server."""
	return {"status": "healthy", "service": "webcat"}
	mcp_server.tool(
	name="health_check", description="Check the health of the server"
	)(health_check_tool)
	from .tools.health_check_tool import health_check_tool
	from .tools.search_tool import search_tool
	# Register tools with MCP server
	mcp_server.tool(
	name="search",
	description="Search the web for information using Serper API or DuckDuckGo fallback",
	)(search_tool)
	mcp_server.tool(
	name="health_check", description="Check the health of the server"
	)(health_check_tool)

🤖 Prompt for AI Agents

In docker/mcp_server.py around lines 67 to 78 the imports use top-level names
("tools.health_check_tool", "tools.search_tool") which will break when the
module is run as part of the docker package; change them to package-relative
imports (e.g. import from .tools.health_check_tool and .tools.search_tool) so
the module can be imported/run both as a script and via python -m, then keep the
existing mcp_server.tool registration calls unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Isolate environment variables per test

Each test mutates LOG_DIR/LOG_LEVEL in os.environ but never restores them. Test outcomes now depend on execution order (e.g., test_sets_log_level_from_environment leaves LOG_LEVEL="DEBUG", so test_uses_default_log_level will fail whenever it runs first). Please use monkeypatch.setenv/monkeypatch.delenv (or a fixture that saves and restores) so every test observes a clean environment state.

🤖 Prompt for AI Agents

In docker/tests/unit/utils/test_logging_config.py around lines 28 to 64, the
tests mutate LOG_DIR and LOG_LEVEL on os.environ but do not restore them,
causing order-dependent failures; change each test to use pytest's monkeypatch
fixture (monkeypatch.setenv / monkeypatch.delenv) to set or remove LOG_DIR and
LOG_LEVEL within the test scope (or a helper fixture that sets and restores
them) so environment changes are isolated per test, ensuring each test sets
required env vars via monkeypatch and does not rely on global os.environ state.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Load the API key at call time

SERPER_API_KEY is resolved once at module import, so any runtime configuration (tests using monkeypatch.setenv, key rotation, CLI export done after import) is ignored. Move the os.environ.get inside search_tool so each invocation sees the current key.

🧰 Tools

🪛 Ruff (0.13.2)

23-23: Unused function argument: ctx

(ARG001)

🤖 Prompt for AI Agents

In docker/tools/search_tool.py around lines 19 to 40, the SERPER_API_KEY is read
at module import which prevents runtime changes (tests or key rotation) from
being picked up; move the os.environ.get("SERPER_API_KEY", "") call inside the
search_tool function so it is evaluated on each invocation, store it in a local
variable (e.g., serper_api_key) and pass that local variable to
fetch_with_fallback instead of the module-level constant.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid blocking the event loop in search_tool

fetch_with_fallback (and the downstream Serper/DuckDuckGo clients) perform synchronous HTTP calls. Calling them directly from this async def blocks the event loop for the entire request, defeating concurrency for every MCP caller. Please offload the blocking work (e.g., await asyncio.to_thread(...) or run the sync helper outside the coroutine) so the coroutine stays non-blocking.

🧰 Tools

🪛 Ruff (0.13.2)

23-23: Unused function argument: ctx

(ARG001)

🤖 Prompt for AI Agents

In docker/tools/search_tool.py around lines 23 to 63, the function calls
blocking synchronous HTTP helpers directly from an async coroutine; wrap the
blocking work in threads so the event loop isn't blocked — import asyncio and
replace the direct call to fetch_with_fallback with an awaited
asyncio.to_thread(fetch_with_fallback, query, SERPER_API_KEY) (and likewise
offload process_search_results via await
asyncio.to_thread(process_search_results, api_results) if it performs
blocking/CPU work), then use the returned values as before and return
response.model_dump(); ensure proper exception propagation and keep the rest of
the function logic unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Normalize LOG_LEVEL to avoid startup crashes

getattr(logging, log_level) explodes if someone sets LOG_LEVEL=debug (or any non-exact-case value), so the server dies during init. Please normalize and fall back cleanly.

-    logger.setLevel(getattr(logging, log_level))
+    resolved_level = logging.getLevelName(log_level.upper())
+    if isinstance(resolved_level, str):
+        logger.warning(
+            "Invalid LOG_LEVEL '%s', defaulting to INFO",
+            log_level,
+        )
+        resolved_level = logging.INFO
+    logger.setLevel(resolved_level)

This keeps the config resilient to typical environment usage.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# Get configuration from environment
	log_level: str = os.environ.get("LOG_LEVEL", "INFO")
	log_dir: str = os.environ.get("LOG_DIR", tempfile.gettempdir())
	log_file: str = os.path.join(log_dir, log_file_name)
	# Create log directory if it doesn't exist
	os.makedirs(log_dir, exist_ok=True)
	# Configure logger
	logger: logging.Logger = logging.getLogger(logger_name)
	logger.setLevel(getattr(logging, log_level))
	# Get configuration from environment
	log_level: str = os.environ.get("LOG_LEVEL", "INFO")
	log_dir: str = os.environ.get("LOG_DIR", tempfile.gettempdir())
	log_file: str = os.path.join(log_dir, log_file_name)
	# Create log directory if it doesn't exist
	os.makedirs(log_dir, exist_ok=True)
	# Configure logger
	logger: logging.Logger = logging.getLogger(logger_name)
	resolved_level = logging.getLevelName(log_level.upper())
	if isinstance(resolved_level, str):
	logger.warning(
	"Invalid LOG_LEVEL '%s', defaulting to INFO",
	log_level,
	)
	resolved_level = logging.INFO
	logger.setLevel(resolved_level)

🤖 Prompt for AI Agents

In docker/utils/logging_config.py around lines 34 to 45, the code uses
getattr(logging, log_level) directly which will raise if LOG_LEVEL is lowercase
or invalid; normalize the env var to upper() and resolve a safe logging level
with a fallback (e.g., level = getattr(logging, log_level.upper(), logging.INFO)
or use logging.getLevelName/logging._nameToLevel to validate) then call
logger.setLevel(level) so invalid or mixed-case LOG_LEVEL values do not crash
startup.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix self-referential all extra

all = ["webcat-mcp[dev,test,docs]"] makes the wheel metadata declare a dependency on itself, and pip install -e ".[all]" (see Makefile Line 30) fails with “A project cannot depend on itself.” Please expand all into the union of the dev/test/docs requirement lists instead of pointing back to the package. For example:

 all = [
-    # Install all optional dependencies
-    "webcat-mcp[dev,test,docs]",
+    # Dev toolchain
+    "black>=23.0.0",
+    "isort>=5.12.0",
+    "autoflake>=2.0.0",
+    ...,
+    # Test extras
+    "pytest>=7.4.0",
+    ...,
+    # Docs extras
+    "sphinx>=7.1.0",
+    "sphinx-rtd-theme>=1.3.0",
+    "myst-parser>=2.0.0",
 ]

(keep the full lists in sync).

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In pyproject.toml around lines 102 to 105, the "all" extra currently references
the package itself ("webcat-mcp[dev,test,docs]") which makes the wheel depend on
itself; replace that self-reference by expanding "all" into the union of the
individual dependency lists used for dev, test, and docs (i.e., copy the package
names/versions from the dev, test and docs extras into the all list), ensuring
formatting matches existing extras and keep the full lists in sync when updating
dependencies.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (2)

docker/mcp_server.py (1)

67-78: Relative imports required inside the package.

This repeats the earlier feedback: using from tools.* only works when docker/ happens to be on sys.path. Executing python -m docker.mcp_server (or installing the package) still raises ModuleNotFoundError. Please make the imports package-relative.

Apply this diff:

-from tools.health_check_tool import health_check_tool
-from tools.search_tool import search_tool
+from .tools.health_check_tool import health_check_tool
+from .tools.search_tool import search_tool

docker/services/content_scraper.py (1)

118-128: Address the unresolved math fence issue.

This is the same issue flagged in the previous review: wrapping math content with $$$...$$$ prevents Markdown/MathJax from recognizing math blocks. Standard double-dollar fences $$...$$ should be used instead.

Apply this diff:

-                    math.replace_with(f"$$${math.string}$$$")
+                    math.replace_with(f"$${math.string}$$")

And:

-                    math.replace_with(f"$$${str(math)}$$$")
+                    math.replace_with(f"$${str(math)}$$")

Additionally, line 128 can use an explicit conversion flag as suggested by Ruff:

-                    math.replace_with(f"$$${str(math)}$$$")
+                    math.replace_with(f"$${math!s}$$")

🧹 Nitpick comments (3)

docker/tests/unit/services/test_search_service.py (1)

52-79: LGTM! No-key scenarios properly covered.

The test class correctly verifies DuckDuckGo fallback behavior when no Serper API key is provided, including the empty results case.

Consider adding a test case for when the Serper client raises an exception (e.g., network error, API error). Currently, if fetch_search_results throws, the exception would propagate. Decide if this is the desired behavior or if you want graceful fallback to DuckDuckGo.

Example test:

@patch("services.search_service.fetch_duckduckgo_search_results")
@patch("services.search_service.fetch_search_results")
def test_falls_back_to_ddg_when_serper_raises(self, mock_serper, mock_ddg):
    # Arrange
    mock_serper.side_effect = Exception("API error")
    mock_ddg.return_value = [
        APISearchResult(title="DDG", link="https://ddg.com", snippet="Fallback")
    ]
    
    # Act
    results, source = fetch_with_fallback("test query", serper_api_key="fake_key")
    
    # Assert
    assert source == "DuckDuckGo (free fallback)"
    assert len(results) == 1

Note: This would require wrapping the Serper call in a try/except in search_service.py.

docker/services/content_scraper.py (2)

136-152: Consider catching more specific exceptions.

The broad Exception catch serves as a fallback mechanism and is logged appropriately. However, catching more specific exceptions from the readability library would improve clarity.

Optional improvement:

-        except Exception as e:
+        except (ValueError, AttributeError, TypeError) as e:

This would catch common parsing errors while letting truly unexpected exceptions propagate. You can also check the readability library documentation for its specific exception types.

Note: Line 139 can use an explicit conversion flag (!s) instead of str() as suggested by Ruff.

---

158-165: Consider using else block for success path.

The current exception handling is correct but could be slightly refactored to follow the else-after-try pattern suggested by TRY300.

Optional refactor:

         result.content = full_content
-        return result
     except requests.RequestException as e:
         result.content = f"Error: Failed to retrieve the webpage. {str(e)}"
-        return result
     except Exception as e:
         result.content = f"Error: Failed to scrape content. {str(e)}"
-        return result
+     else:
+         return result
+     
+     return result

Or more simply, just remove the intermediate returns:

         result.content = full_content
-        return result
     except requests.RequestException as e:
         result.content = f"Error: Failed to retrieve the webpage. {str(e)}"
-        return result
     except Exception as e:
         result.content = f"Error: Failed to scrape content. {str(e)}"
-        return result
+     
+     return result

Also, lines 161 and 164 can use explicit conversion flags ({e!s}) instead of {str(e)} as suggested by Ruff.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94bd718 and fc3db41.

📒 Files selected for processing (4)

• docker/mcp_server.py (1 hunks)
• docker/services/content_scraper.py (1 hunks)
• docker/tests/unit/models/test_models.py (1 hunks)
• docker/tests/unit/services/test_search_service.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• docker/tests/unit/models/test_models.py

🧰 Additional context used

🧬 Code graph analysis (3)

docker/mcp_server.py (3)

docker/utils/logging_config.py (1)

• setup_logging (21-71)

docker/tools/health_check_tool.py (1)

• health_check_tool (11-23)

docker/tools/search_tool.py (1)

• search_tool (23-63)

docker/tests/unit/services/test_search_service.py (2)

docker/services/search_service.py (1)

• fetch_with_fallback (18-46)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/services/content_scraper.py (2)

docker/tests/factories/http_response_factory.py (1)

• timeout (79-86)

docker/tests/factories/mock_http_response.py (1)

• raise_for_status (48-53)

🪛 GitHub Actions: CI Pipeline

docker/tests/unit/services/test_search_service.py

[error] 1-1: isort: Imports are incorrectly sorted and/or formatted.

🪛 Ruff (0.13.2)

docker/services/content_scraper.py

128-128: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

136-136: Do not catch blind exception: Exception

(BLE001)

---

139-139: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

159-159: Consider moving this statement to an else block

(TRY300)

---

161-161: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

163-163: Do not catch blind exception: Exception

(BLE001)

---

164-164: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🔇 Additional comments (6)

docker/tests/unit/services/test_search_service.py (1)

15-50: LGTM! Serper key scenarios well-tested.

The test class correctly verifies that when a Serper API key is provided:

1. The service uses Serper first and reports the correct source
2. The service falls back to DuckDuckGo when Serper returns empty results

Mock usage and assertions are appropriate.

docker/services/content_scraper.py (5)

1-19: LGTM!

Module structure, imports, and logging setup are correct.

---

21-33: LGTM!

Function signature is clean, and early validation for missing URL prevents unnecessary HTTP requests.

---

51-65: LGTM!

Content-type detection and special handling for plain text and binary formats are appropriate.

---

154-158: LGTM!

Content length limiting prevents excessive response sizes and includes a clear truncation notice.

---

46-49: Verify URL scheme validation to prevent SSRF. No URL parsing or scheme checks were found upstream; ensure result.url is restricted to only http/https before calling requests.get.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Use package-relative import to avoid ModuleNotFoundError.

When docker.mcp_server runs as part of the docker package (e.g., python -m docker.mcp_server), from utils.logging_config looks for a top-level utils module and crashes. Switch to a relative import so it resolves within the package.

Apply this diff:

-from utils.logging_config import setup_logging
+from .utils.logging_config import setup_logging

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	from utils.logging_config import setup_logging
	# Setup console handler
	console_handler = logging.StreamHandler()
	console_handler.setFormatter(console_formatter)
	logger.addHandler(console_handler)
	# Setup rotating file handler (10MB per file, keep 5 backup files)
	file_handler = logging.handlers.RotatingFileHandler(
	LOG_FILE, maxBytes=10 * 1024 * 1024, backupCount=5, encoding="utf-8" # 10MB
	)
	file_handler.setFormatter(file_formatter)
	logger.addHandler(file_handler)
	logging.info("Logging initialized with file rotation at %s", LOG_FILE)
	# Set up logging
	logger = setup_logging("webcat.log")
	from .utils.logging_config import setup_logging
	# Set up logging
	logger = setup_logging("webcat.log")

🤖 Prompt for AI Agents

In docker/mcp_server.py around lines 47–51, the current absolute import "from
utils.logging_config import setup_logging" will raise ModuleNotFoundError when
run as a module; change it to a package-relative import (e.g., "from
.utils.logging_config import setup_logging") so Python resolves the utils module
inside the docker package when invoking "python -m docker.mcp_server"; update
the import line accordingly and ensure other module references use relative
imports if they are package-local.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

docker/tests/unit/services/test_content_scraper.py (1)

103-115: This test requires the fix from previous review comments.

The truncation branch is not reliably exercised because the mocked requests.get response is processed by the real HTML2Text instance, which produces much shorter output than the input HTML. The previous review comment provides the necessary patch to stabilize this test by forcing an oversized markdown string.

docker/tests/unit/services/test_search_processor.py (1)

86-86: Remove unused variable or verify the output.

Line 86 assigns results but never uses it. As noted in the previous review, either verify the function's output end-to-end or remove the assignment.

Apply one of these solutions:

Option 1: Verify complete flow

         # Act
         results = process_search_results([api_result])
 
         # Assert
         call_args = mock_scrape.call_args[0][0]
         assert call_args.title == "Original"
         assert call_args.url == "https://orig.com"
         assert call_args.snippet == "Original snippet"
+        assert len(results) == 1
+        assert results[0].content == "Scraped"

Option 2: Remove unused variable

         # Act
-        results = process_search_results([api_result])
+        process_search_results([api_result])
 
         # Assert
         call_args = mock_scrape.call_args[0][0]

🧹 Nitpick comments (3)

docker/tests/unit/tools/test_search_tool.py (3)

20-46: Strengthen assertions to validate the complete result structure.

The test only asserts the first result's title. Consider also validating url, snippet, and content to ensure the full SearchResult is properly mapped.

Apply this diff to add more comprehensive assertions:

         assert result["results"][0]["title"] == "Test"
+        assert result["results"][0]["url"] == "https://test.com"
+        assert result["results"][0]["snippet"] == "Snippet"
+        assert result["results"][0]["content"] == "Content"

---

62-75: Consider adding result assertions alongside the interaction check.

The test correctly verifies that process_search_results is called, but doesn't validate the structure of the returned result when processing yields an empty list. This could catch regressions in result formatting.

Apply this diff to add result validation:

         # Assert
         mock_process.assert_called_once_with(api_results)
+        assert result["query"] == "query"
+        assert result["search_source"] == "Source"
+        assert len(result["results"]) == 0

---

17-75: Consider adding tests for exception scenarios.

The current tests cover the happy path and no-results case well. Consider adding tests for:

• Exception handling when fetch_with_fallback raises an error
• Exception handling when process_search_results raises an error
• Validation of malformed results

These additions would improve resilience testing and catch error-path regressions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc3db41 and f30b6d8.

📒 Files selected for processing (7)

• Makefile (4 hunks)
• docker/tests/unit/clients/test_duckduckgo_client.py (1 hunks)
• docker/tests/unit/clients/test_serper_client.py (1 hunks)
• docker/tests/unit/services/test_content_scraper.py (1 hunks)
• docker/tests/unit/services/test_search_processor.py (1 hunks)
• docker/tests/unit/services/test_search_service.py (1 hunks)
• docker/tests/unit/tools/test_search_tool.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• docker/tests/unit/clients/test_serper_client.py
• docker/tests/unit/services/test_search_service.py
• docker/tests/unit/clients/test_duckduckgo_client.py

🧰 Additional context used

🧬 Code graph analysis (3)

docker/tests/unit/services/test_search_processor.py (2)

docker/services/search_processor.py (1)

• process_search_results (15-39)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tests/unit/services/test_content_scraper.py (4)

docker/services/content_scraper.py (1)

• scrape_search_result (21-165)

docker/tests/conftest.py (2)

• search_result_builder (42-44)
• mock_get (67-68)

docker/tests/builders/search_result_builder.py (4)

• a_search_result (66-68)
• with_title (29-32)
• build (56-63)
• with_url (34-37)

docker/tests/factories/http_response_factory.py (8)

• HttpResponseFactory (13-96)
• html_with_title (37-40)
• plaintext (43-45)
• pdf (48-52)
• error_404 (55-64)
• timeout (79-86)
• success (22-34)
• connection_error (89-96)

docker/tests/unit/tools/test_search_tool.py (2)

docker/models/api_search_result.py (1)

• APISearchResult (11-22)

docker/tests/unit/clients/test_duckduckgo_client.py (1)

• test_returns_search_results (18-34)

🪛 checkmake (0.2.2)

Makefile

[warning] 118-118: Target body for "dev" exceeds allowed length of 5 (6).

(maxbodylength)

---

[warning] 221-221: Target body for "ci" exceeds allowed length of 5 (16).

(maxbodylength)

🪛 GitHub Actions: CI Pipeline

docker/tests/unit/services/test_search_processor.py

[error] 10-10: F401 'pytest' imported but unused

---

[error] 86-86: F841 local variable 'results' is assigned to but never used

docker/tests/unit/services/test_content_scraper.py

[error] 10-10: F401 'pytest' imported but unused

Makefile

[error] 47-47: lint target failed. Command 'make format-check lint' exited with non-zero status.

🪛 Ruff (0.13.2)

docker/tests/unit/services/test_search_processor.py

85-85: Local variable results is assigned to but never used

Remove assignment to unused variable results

(F841)

🔇 Additional comments (13)

docker/tests/unit/tools/test_search_tool.py (2)

1-14: LGTM!

Imports are clean and appropriate for the async test scenarios.

---

48-60: LGTM!

The error-handling path is well-tested with appropriate assertions for the no-results scenario.

docker/tests/unit/services/test_content_scraper.py (8)

21-32: LGTM!

The test correctly patches requests.get, uses the factory to create a typed mock response, and verifies both the title header and source citation in the markdown output.

---

34-45: LGTM!

The test correctly verifies that plaintext content is wrapped in markdown code blocks using the plaintext factory.

---

47-58: LGTM!

The test correctly verifies that PDF files are handled with an appropriate message rather than attempting conversion.

---

64-74: LGTM!

The test correctly verifies error handling for 404 responses using the factory's typed error response.

---

76-87: LGTM!

The test correctly verifies timeout handling by using side_effect with the factory's timeout exception and checking for the timeout indicator in the error message.

---

89-97: LGTM!

The test correctly verifies the early-exit path when the URL is missing, without requiring HTTP mocking.

---

117-127: LGTM!

The test correctly verifies connection error handling by using side_effect with the factory's connection error exception.

---

129-141: LGTM!

The test correctly verifies the fallback path when readability parsing fails by using intentionally malformed HTML and checking that content is still produced.

docker/tests/unit/services/test_search_processor.py (3)

20-37: LGTM!

The test correctly validates single result processing with proper mock setup and comprehensive assertions.

---

39-61: LGTM!

The test properly validates multiple result processing using side_effect to simulate sequential scraping operations.

---

63-70: LGTM!

The test correctly validates the empty list edge case.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fix dev-setup so it actually uses the venv it just created.

Right now dev-setup runs format, lint, and test after setup-dev, but those targets call black, flake8, and pytest without activating the freshly created venv. On a clean machine (no global installs) make dev-setup fails immediately with “command not found”. That defeats the purpose of the one-shot setup target.

Have dev-setup run the checks inside the activated venv instead of listing them as prerequisites:

-dev-setup: setup-dev format lint test ## Complete development setup
-	@echo "🎉 Development environment ready!"
+dev-setup: setup-dev ## Complete development setup
+	@echo "🎉 Running sanity checks inside venv..."
+	@. venv/bin/activate && $(MAKE) format lint test
+	@echo "🎉 Development environment ready!"

This keeps the workflow self-contained and works on a clean environment.

🤖 Prompt for AI Agents

Makefile lines 217-219: the dev-setup target currently lists setup-dev, format,
lint, test as prerequisites so format/lint/test run in a different shell without
the newly created virtualenv; change dev-setup to first run setup-dev and then
invoke format, lint and test commands inside the created venv in the same shell
(either by sourcing the venv activate script or by calling the venv's bin
executables directly, e.g. <venv>/bin/black <...>, <venv>/bin/flake8 <...>,
<venv>/bin/pytest <...>), and remove format/lint/test as prerequisites so they
aren’t executed outside the venv.