refactor: Make prefix_sep configurable instead of class constant

sitbon · sitbon · commit 79e97e5c824f · 2025-07-16T00:07:13.000-07:00
- Change PREFIX_SEP from ClassVar to prefix_sep field on MaggConfig
- Add MAGG_PREFIX_SEP env var support (default: "_")
- Update ServerManager to use config.prefix_sep
- Remove unused generate_prefix_from_name() method
- Update docs to explain configurable separator
- Add tests for prefix_sep configuration

Allows users to customize the separator between prefix and tool name.
diff --git a/docs/index.md b/docs/index.md
@@ -842,6 +842,86 @@ Check logs for:
 - Tool delegation errors
 - Configuration problems
 
+### Tool Naming and Prefix System
+
+#### Understanding Tool Names
+
+Magg organizes tools from multiple servers using a prefix system. Every tool name follows this pattern:
+
+```
+{prefix}{separator}{tool_name}
+```
+
+Where:
+- `prefix` is the namespace for a server (configurable per server)
+- `separator` is configurable via `prefix_sep` field or `MAGG_PREFIX_SEP` env var (default: `_`)
+- `tool_name` is the original tool name from the server
+
+Examples:
+- `calc_add` - "add" tool from server with "calc" prefix
+- `pw_screenshot` - "screenshot" tool from server with "pw" prefix  
+- `magg_list_servers` - "list_servers" tool from Magg itself
+- `read_file` - "read_file" tool from server with no prefix
+
+#### Configuring Server Prefixes
+
+When adding a server, specify the `prefix` field:
+
+```json
+{
+  "name": "calculator",
+  "source": "...",
+  "prefix": "calc"      // All tools will be calc_*
+}
+```
+
+Special cases:
+- `"prefix": ""` or `"prefix": null` - No prefix, tools keep original names
+- Omitting prefix field - Server name is used as default prefix
+- Multiple servers can share the same prefix (tools are merged)
+
+#### Configuring Magg's Prefix
+
+Magg's own tools use the prefix from `MAGG_SELF_PREFIX`:
+
+```bash
+# Default: magg_list_servers, magg_add_server, etc.
+export MAGG_SELF_PREFIX=magg
+
+# Custom: myapp_list_servers, myapp_add_server, etc.
+export MAGG_SELF_PREFIX=myapp
+
+# No prefix: list_servers, add_server, etc.
+export MAGG_SELF_PREFIX=""
+```
+
+#### Configuring the Separator
+
+You can change the separator between prefix and tool name:
+
+```bash
+# Default underscore separator: calc_add, magg_list_servers
+export MAGG_PREFIX_SEP="_"
+
+# Dash separator: calc-add, magg-list-servers  
+export MAGG_PREFIX_SEP="-"
+
+# Dot separator: calc.add, magg.list.servers
+export MAGG_PREFIX_SEP="."
+
+# No separator: calcadd, magglistservers
+export MAGG_PREFIX_SEP=""
+```
+
+#### Prefix Validation Rules
+
+- Must be valid Python identifiers (alphanumeric, not starting with digit)
+- Cannot contain the separator character (default: underscore, but configurable)
+- Case-sensitive
+- Empty string allowed (results in no prefix)
+
+Note: While the separator is configurable, server prefixes currently still cannot contain underscores due to Python identifier constraints.
+
 ### Performance Optimization
 
 1. **Disable unused servers** to reduce memory usage
diff --git a/magg/server/manager.py b/magg/server/manager.py
@@ -60,7 +60,7 @@ def save_config(self, config: MaggConfig):
     @cached_property
     def prefix_separator(self) -> str:
         """Get the prefix separator for this Magg server - cannot be changed during process lifetime."""
-        return ServerConfig.PREFIX_SEP
+        return self.config.prefix_sep
 
     @cached_property
     def self_prefix(self) -> str:
diff --git a/magg/settings.py b/magg/settings.py
@@ -4,10 +4,10 @@
 import os
 from functools import cached_property
 from pathlib import Path
-from typing import Any, ClassVar, Callable, Coroutine, TYPE_CHECKING
+from typing import Any, Callable, Coroutine, TYPE_CHECKING
 
 if TYPE_CHECKING:
-    from .reload import ConfigReloader, ConfigChange
+    from .reload import ConfigChange
 
 from pydantic import field_validator, Field, model_validator, AnyUrl
 from pydantic_settings import BaseSettings, SettingsConfigDict
@@ -114,13 +114,11 @@ class ServerConfig(BaseSettings):
         arbitrary_types_allowed=True,
     )
 
-    PREFIX_SEP: ClassVar[str] = "_"
-
     name: str = Field(..., description="Unique server name - can contain any characters")
     source: str = Field(..., description="URL/URI/path of the server package, repository, or listing")
     prefix: str | None = Field(
         default=None,
-        description=f"Tool prefix for this server - must be a valid Python identifier without {PREFIX_SEP!r}."
+        description="Tool prefix for this server - must be a valid Python identifier without underscores."
     )
     notes: str | None = Field(None, description="Setup notes for LLM and humans")
 
@@ -147,9 +145,9 @@ def validate_prefix(cls, v: str | None) -> str | None:
                 raise ValueError(
                     f"Server prefix {v!r} must be a valid Python identifier (letters and numbers only, not starting with a number)"
                 )
-            if cls.PREFIX_SEP in v:
+            if "_" in v:
                 raise ValueError(
-                    f"Server prefix {v!r} must be a valid Python identifier and cannot contain {cls.PREFIX_SEP!r}"
+                    f"Server prefix {v!r} must be a valid Python identifier and cannot contain underscores"
                 )
         return v
 
@@ -165,31 +163,6 @@ def validate_uri(cls, v: str | None) -> str | None:
             AnyUrl(v)  # Validate as URL
         return v
 
-    @classmethod
-    def generate_prefix_from_name(cls, name: str) -> str:
-        """Generate a valid prefix from a server name.
-
-        Removes special characters and ensures it's a valid Python identifier
-        without underscores.
-        """
-        prefix = (
-            name.replace('-', '')
-                .replace('_', '')
-                .replace('.', '')
-                .replace(' ', '')
-                .replace(cls.PREFIX_SEP, '')
-        )
-
-        prefix = ''.join(c for c in prefix if c.isalnum())
-
-        if prefix and prefix[0].isdigit():
-            prefix = 'srv' + prefix
-
-        if not prefix or not prefix.isidentifier():
-            prefix = 'server'
-
-        return prefix.lower()[:30]
-
 
 class MaggConfig(BaseSettings):
     """Main Magg configuration."""
@@ -214,6 +187,10 @@ class MaggConfig(BaseSettings):
         default="magg",
         description="Prefix for Magg tools and commands - must be a valid Python identifier without underscores (env: MAGG_SELF_PREFIX)"
     )
+    prefix_sep: str = Field(
+        default="_",
+        description="Separator between prefix and tool name (env: MAGG_PREFIX_SEP)"
+    )
     auto_reload: bool = Field(default=True, description="Enable automatic config reloading on file changes (env: MAGG_AUTO_RELOAD)")
     reload_poll_interval: float = Field(default=1.0, description="Config file poll interval in seconds (env: MAGG_RELOAD_POLL_INTERVAL)")
     reload_use_watchdog: bool | None = Field(default=None, description="Use file system notifications if available, None=auto-detect (env: MAGG_RELOAD_USE_WATCHDOG)")
diff --git a/magg/test/test_config.py b/magg/test/test_config.py
@@ -66,25 +66,13 @@ def test_server_prefix_validation(self):
         ServerConfig(name="test", source="test", prefix="validprefix")
 
         # Invalid prefixes - no underscores allowed
-        with pytest.raises(ValueError, match="cannot contain '_'"):
+        with pytest.raises(ValueError, match="cannot contain underscores"):
             ServerConfig(name="test", source="test", prefix="invalid_prefix")
 
         # Invalid prefixes - must be identifier
         with pytest.raises(ValueError, match="must be a valid Python identifier"):
             ServerConfig(name="test", source="test", prefix="123invalid")
 
-    def test_prefix_generation(self):
-        """Test the generate_prefix_from_name helper method."""
-        # Test various name patterns
-        assert ServerConfig.generate_prefix_from_name("simple") == "simple"
-        assert ServerConfig.generate_prefix_from_name("weather-mcp") == "weathermcp"
-        assert ServerConfig.generate_prefix_from_name("test_server") == "testserver"
-        assert ServerConfig.generate_prefix_from_name("my.package.name") == "mypackagename"
-        assert ServerConfig.generate_prefix_from_name("123-start") == "srv123start"
-        assert ServerConfig.generate_prefix_from_name("@namespace/package") == "namespacepackage"
-        assert ServerConfig.generate_prefix_from_name("UPPERCASE") == "uppercase"
-        assert ServerConfig.generate_prefix_from_name("") == "server"
-        assert ServerConfig.generate_prefix_from_name("!!!") == "server"
 
 
 class TestMaggConfig:
diff --git a/magg/test/test_prefix_handling.py b/magg/test/test_prefix_handling.py
@@ -0,0 +1,81 @@
+"""Test prefix and separator handling."""
+
+import pytest
+import os
+from magg.settings import MaggConfig, ServerConfig
+from magg.server.server import MaggServer
+from fastmcp import Client
+
+
+class TestPrefixHandling:
+    """Test custom prefix and separator handling."""
+
+    def test_prefix_separator_on_magg_config(self):
+        """Test that prefix_sep is a configurable field on MaggConfig."""
+        config = MaggConfig()
+        assert hasattr(config, 'prefix_sep')
+        assert config.prefix_sep == "_"  # Default value
+        
+        # Test it can be configured
+        config2 = MaggConfig(prefix_sep="-")
+        assert config2.prefix_sep == "-"
+
+    def test_server_config_uses_separator(self):
+        """Test that ServerConfig validation uses the separator."""
+        # Valid prefix
+        server = ServerConfig(name="test", source="test", prefix="myprefix")
+        assert server.prefix == "myprefix"
+        
+        # Invalid prefix with underscore
+        with pytest.raises(ValueError, match="cannot contain underscores"):
+            ServerConfig(name="test", source="test", prefix="my_prefix")
+
+    @pytest.mark.asyncio
+    async def test_custom_self_prefix(self, tmp_path, monkeypatch):
+        """Test that custom self_prefix is used correctly."""
+        # Set custom prefix via environment
+        monkeypatch.setenv("MAGG_SELF_PREFIX", "myapp")
+        
+        config_path = tmp_path / "config.json"
+        server = MaggServer(config_path, enable_config_reload=False)
+        
+        # Check configuration
+        assert server.self_prefix == "myapp"
+        assert server.self_prefix_ == "myapp_"
+        
+        await server.setup()
+        
+        # Verify tools have the correct prefix
+        async with Client(server.mcp) as client:
+            tools = await client.list_tools()
+            tool_names = [tool.name for tool in tools]
+            
+            # All Magg tools should have myapp_ prefix
+            myapp_tools = [t for t in tool_names if t.startswith("myapp_")]
+            assert len(myapp_tools) > 0
+            assert "myapp_list_servers" in tool_names
+            assert "myapp_add_server" in tool_names
+            assert "myapp_status" in tool_names
+            
+            # Should not have any magg_ tools
+            magg_tools = [t for t in tool_names if t.startswith("magg_")]
+            assert len(magg_tools) == 0
+
+    def test_prefix_separator_consistency(self):
+        """Test that prefix separator is used consistently."""
+        # Create a server with default Magg config
+        config = MaggConfig()
+        assert config.self_prefix == "magg"
+        assert config.prefix_sep == "_"
+        
+        # Server config validation should use the separator
+        server1 = ServerConfig(name="test1", source="test", prefix="myprefix")
+        assert server1.prefix == "myprefix"
+        
+        # Empty prefix is allowed
+        server2 = ServerConfig(name="test2", source="test", prefix="")
+        assert server2.prefix == ""
+        
+        # None prefix is allowed  
+        server3 = ServerConfig(name="test3", source="test", prefix=None)
+        assert server3.prefix is None
diff --git a/magg/test/test_tool_delegation.py b/magg/test/test_tool_delegation.py
@@ -145,7 +145,8 @@ def test_tool2(value: int) -> int:
             assert "test_test_tool2" in tool_names
 
             # Verify we have tools from both servers
-            magg_tools = [t for t in tool_names if t.startswith("magg_")]
+            server_prefix = server.self_prefix_
+            magg_tools = [t for t in tool_names if t.startswith(server_prefix)]
             test_tools = [t for t in tool_names if t.startswith("test_")]
 
             assert len(magg_tools) >= 2  # At least magg_list_servers and magg_add_server
diff --git a/readme.md b/readme.md
@@ -281,7 +281,8 @@ Magg supports several environment variables for configuration:
 - `MAGG_RELOAD_POLL_INTERVAL` - Config polling interval in seconds (default: 1.0)
 - `MAGG_READ_ONLY=true` - Run in read-only mode
 - `MAGG_QUIET=true` - Suppress output unless errors occur
-- `MAGG_SELF_PREFIX` - Prefix for Magg tools (default: "magg")
+- `MAGG_SELF_PREFIX` - Prefix for Magg tools (default: "magg"). Tools will be named as `{prefix}{sep}{tool}` (e.g., `magg_list_servers`)
+- `MAGG_PREFIX_SEP` - Separator between prefix and tool name (default: "_")
 
 Example configuration:
 ```json
diff --git a/uv.lock b/uv.lock
