components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py (1)

41-46: Validate environment variables before use.

Line 42 will raise TypeError if CLP_LOGS_DIR is not set (Path(None) is invalid). Line 46 should provide a default for CLP_LOGGING_LEVEL.

Apply defensive validation:

     # Setup logging to file
+    clp_logs_dir = os.getenv("CLP_LOGS_DIR")
+    if clp_logs_dir is None:
+        logger.error("CLP_LOGS_DIR environment variable is not set.")
+        return 1
+    log_file_path = Path(clp_logs_dir) / "mcp_server.log"
-    log_file_path = Path(os.getenv("CLP_LOGS_DIR")) / "mcp_server.log"
+    log_file_path.parent.mkdir(parents=True, exist_ok=True)
     logging_file_handler = logging.FileHandler(filename=log_file_path, encoding="utf-8")
     logging_file_handler.setFormatter(get_logging_formatter())
     logger.addHandler(logging_file_handler)
-    set_logging_level(logger, os.getenv("CLP_LOGGING_LEVEL"))
+    set_logging_level(logger, os.getenv("CLP_LOGGING_LEVEL", "INFO"))

tools/deployment/package/docker-compose.yaml (1)

146-165: Move healthcheck to the bottom for consistency.

For consistency with other services like query-scheduler (lines 61-68), move the healthcheck block below the command block.

Apply this reordering:

     volumes:
       - *volume_clp_config_readonly
       - *volume_clp_logs
-    healthcheck:
-      <<: *healthcheck_defaults
-      test: [
-        "CMD",
-        "curl",
-        "-f",
-        "http://mcp_server:8000/health"
-      ]
     depends_on:
       db-table-creator:
         condition: "service_completed_successfully"
       results-cache-indices-creator:
         condition: "service_completed_successfully"
     command: [
       "python3", "-u",
       "-m", "clp_mcp_server.clp_mcp_server",
       "--host", "mcp_server",
       "--port", "8000",
       "--config-path", "/etc/clp-config.yml",
     ]
+    healthcheck:
+      <<: *healthcheck_defaults
+      test: [
+        "CMD",
+        "curl",
+        "-f",
+        "http://mcp_server:8000/health"
+      ]

components/clp-package-utils/clp_package_utils/controller.py (3)

533-538: Update docstring for consistency.

The docstring should match the pattern used in other component setup methods (e.g., _set_up_env_for_reducer at line 385).

Apply this change:

     def _set_up_env_for_mcp_server(self) -> EnvVarsDict:
         """
-        Sets up environment variables and directories for the MCP server component.
+        Sets up environment variables for the MCP server component.
 
         :return: Dictionary of environment variables necessary to launch the component.
         """

---

540-560: Fix logging call, reorder setup steps, add chown, and alphabetize env groups.

Multiple issues need addressing:

• Line 540: f-string in logging violates Ruff G004
• Lines 542-545: validate_mcp_server_config should be called before mkdir (per junhaoliao's comment and the pattern that validation should verify the path could be created, not that it already exists)
• Missing _chown_paths_if_root(logs_dir) call after mkdir
• Lines 549-558: Env var groups should be alphabetized (Connection before Logging)

Based on learnings

Apply these fixes:

         component_name = MCP_SERVER_COMPONENT_NAME
-        logger.info(f"Setting up environment for {component_name}...")
+        logger.info("Setting up environment for %s...", component_name)
 
         logs_dir = self._clp_config.logs_directory / component_name
+        validate_mcp_server_config(self._clp_config, logs_dir)
         logs_dir.mkdir(parents=True, exist_ok=True)
-
-        validate_mcp_server_config(self._clp_config, logs_dir)
+        _chown_paths_if_root(logs_dir)
 
         env_vars = EnvVarsDict()
 
         # Connection config
         env_vars |= {
             "CLP_MCP_HOST": _get_ip_from_hostname(self._clp_config.mcp_server.host),
             "CLP_MCP_PORT": str(self._clp_config.mcp_server.port),
         }
 
         # Logging config
         env_vars |= {
             "CLP_MCP_LOGGING_LEVEL": self._clp_config.mcp_server.logging_level,
         }

Note: This assumes validate_mcp_server_config in general.py has been updated to check if the path could be a directory rather than checking if it already exists (per junhaoliao's comment on lines 542-545).

---

661-662: Consider adding explicit enabled flag for MCP server.

The current check len(self._clp_config.mcp_server.model_fields_set) > 0 works but is implicit. An explicit enabled: bool field in the McpServer config model would make the intent clearer and align with junhaoliao's suggestion.

Example config structure:

mcp_server:
  enabled: false
  host: "localhost"
  port: 8000

This would allow:

if self._clp_config.mcp_server.enabled:
    cmd += ["--profile", "mcp"]

This is a design decision for the maintainers to consider.

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: 20001020ycx
PR: y-scope/clp#1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.

Learnt from: LinZhihao-723
PR: y-scope/clp#1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.

Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.

components/clp-py-utils/clp_py_utils/clp_config.py (1)

579-585: Add transform_for_container() method to McpServer.

The McpServer class is missing a transform_for_container() method that all similar components implement. Without it, the MCP server will attempt to connect to localhost:8000 in the container instead of using the Docker service hostname mcp_server:8000.

Apply this diff to add the method:

 class McpServer(BaseModel):
     DEFAULT_PORT: ClassVar[int] = 8000
 
     host: DomainStr = "localhost"
     port: Port = DEFAULT_PORT
     logging_level: LoggingLevel = "INFO"
+
+    def transform_for_container(self):
+        self.host = MCP_SERVER_COMPONENT_NAME
+        self.port = self.DEFAULT_PORT

Additionally, ensure CLPConfig.transform_for_container() calls this method. After line 807, add:

         self.query_scheduler.transform_for_container()
         self.reducer.transform_for_container()
+        if self.mcp_server is not None:
+            self.mcp_server.transform_for_container()

components/clp-package-utils/clp_package_utils/controller.py (2)

540-543: Fix f-string logging violations.

Lines 540 and 543 use f-strings in logging statements, violating Ruff rule G004. Use parameterized logging instead.

Apply this diff:

         if self._clp_config.mcp_server is None:
-            logger.info(f"The MCP Server is not configured, skipping {component_name} creation...")
+            logger.info("The MCP Server is not configured, skipping %s creation...", component_name)
             return EnvVarsDict()
-        logger.info(f"Setting up environment for {component_name}...")
+        logger.info("Setting up environment for %s...", component_name)

---

545-547: Add _chown_paths_if_root() call after creating logs directory.

The logs directory ownership should be corrected for root execution, consistent with other component setup methods.

Apply this diff:

         logs_dir = self._clp_config.logs_directory / component_name
         validate_mcp_server_config(self._clp_config, logs_dir)
         logs_dir.mkdir(parents=True, exist_ok=True)
+        _chown_paths_if_root(logs_dir)

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: 20001020ycx
PR: y-scope/clp#1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.