fix(A1.3): Add comprehensive validation to submit_config MCP tool
Issue: #11 (A1.3 - Add MCP tool to submit custom configs) ## Summary Fixed submit_config MCP tool to use ConfigValidator for comprehensive validation instead of basic 3-field checks. Now supports both legacy and unified config formats with detailed error messages and validation warnings. ## Critical Gaps Fixed (6 total) 1. ✅ Missing comprehensive validation (HIGH) - Only checked 3 fields 2. ✅ No unified config support (HIGH) - Couldn't handle multi-source configs 3. ✅ No test coverage (MEDIUM) - Zero tests for submit_config_tool 4. ✅ No URL format validation (MEDIUM) - Accepted malformed URLs 5. ✅ No warnings for unlimited scraping (LOW) - Silent config issues 6. ✅ No url_patterns validation (MEDIUM) - No selector structure checks ## Changes Made ### Phase 1: Validation Logic (server.py lines 1224-1380) - Added ConfigValidator import with graceful degradation - Replaced basic validation (3 fields) with comprehensive ConfigValidator.validate() - Enhanced category detection for unified multi-source configs - Added validation warnings collection (unlimited scraping, missing max_pages) - Updated GitHub issue template with: * Config format type (Unified vs Legacy) * Validation warnings section * Updated documentation URL handling for unified configs * Checklist showing "Config validated with ConfigValidator" ### Phase 2: Test Coverage (test_mcp_server.py lines 617-769) Added 8 comprehensive test cases: 1. test_submit_config_requires_token - GitHub token requirement 2. test_submit_config_validates_required_fields - Required field validation 3. test_submit_config_validates_name_format - Name format validation 4. test_submit_config_validates_url_format - URL format validation 5. test_submit_config_accepts_legacy_format - Legacy config acceptance 6. test_submit_config_accepts_unified_format - Unified config acceptance 7. test_submit_config_from_file_path - File path input support 8. test_submit_config_detects_category - Category auto-detection ### Phase 3: Documentation Updates - Updated Issue #11 with completion notes - Updated tool description to mention format support - Updated CHANGELOG.md with fix details - Added EVOLUTION_ANALYSIS.md for deep architecture analysis ## Validation Improvements ### Before: ```python required_fields = ["name", "description", "base_url"] missing_fields = [field for field in required_fields if field not in config_data] if missing_fields: return error ``` ### After: ```python validator = ConfigValidator(config_data) validator.validate() # Comprehensive validation: # - Name format (alphanumeric, hyphens, underscores only) # - URL formats (must start with http:// or https://) # - Selectors structure (dict with proper keys) # - Rate limits (non-negative numbers) # - Max pages (positive integer or -1) # - Supports both legacy AND unified formats # - Provides detailed error messages with examples ``` ## Test Results ✅ All 427 tests passing (no regressions) ✅ 8 new tests for submit_config_tool ✅ No breaking changes ## Files Modified - src/skill_seekers/mcp/server.py (157 lines changed) - tests/test_mcp_server.py (157 lines added) - CHANGELOG.md (12 lines added) - EVOLUTION_ANALYSIS.md (500+ lines, new file) ## Issue Resolution Closes #11 - A1.3 now fully implemented with comprehensive validation, test coverage, and support for both config formats. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
yusyus
@@ -614,5 +614,160 @@ class TestMCPServerIntegration(unittest.IsolatedAsyncioTestCase):
|
||||
shutil.rmtree(temp_dir, ignore_errors=True)
|
||||
|
||||
|
||||
@unittest.skipUnless(MCP_AVAILABLE, "MCP package not installed")
|
||||
class TestSubmitConfigTool(unittest.IsolatedAsyncioTestCase):
|
||||
"""Test submit_config MCP tool"""
|
||||
|
||||
async def test_submit_config_requires_token(self):
|
||||
"""Should error without GitHub token"""
|
||||
args = {
|
||||
"config_json": '{"name": "test", "description": "Test", "base_url": "https://example.com"}'
|
||||
}
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("GitHub token required", result[0].text)
|
||||
|
||||
async def test_submit_config_validates_required_fields(self):
|
||||
"""Should reject config missing required fields"""
|
||||
args = {
|
||||
"config_json": '{"name": "test"}', # Missing description, base_url
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("validation failed", result[0].text.lower())
|
||||
self.assertIn("description", result[0].text)
|
||||
|
||||
async def test_submit_config_validates_name_format(self):
|
||||
"""Should reject invalid name characters"""
|
||||
args = {
|
||||
"config_json": '{"name": "React@2024!", "description": "Test", "base_url": "https://example.com"}',
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("validation failed", result[0].text.lower())
|
||||
|
||||
async def test_submit_config_validates_url_format(self):
|
||||
"""Should reject invalid URL format"""
|
||||
args = {
|
||||
"config_json": '{"name": "test", "description": "Test", "base_url": "not-a-url"}',
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("validation failed", result[0].text.lower())
|
||||
|
||||
async def test_submit_config_accepts_legacy_format(self):
|
||||
"""Should accept valid legacy config"""
|
||||
valid_config = {
|
||||
"name": "testframework",
|
||||
"description": "Test framework docs",
|
||||
"base_url": "https://docs.test.com/",
|
||||
"selectors": {
|
||||
"main_content": "article",
|
||||
"title": "h1",
|
||||
"code_blocks": "pre code"
|
||||
},
|
||||
"max_pages": 100
|
||||
}
|
||||
args = {
|
||||
"config_json": json.dumps(valid_config),
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
|
||||
# Mock GitHub API call
|
||||
with patch('github.Github') as mock_gh:
|
||||
mock_repo = MagicMock()
|
||||
mock_issue = MagicMock()
|
||||
mock_issue.html_url = "https://github.com/test/issue/1"
|
||||
mock_issue.number = 1
|
||||
mock_repo.create_issue.return_value = mock_issue
|
||||
mock_gh.return_value.get_repo.return_value = mock_repo
|
||||
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("Config submitted successfully", result[0].text)
|
||||
self.assertIn("https://github.com", result[0].text)
|
||||
|
||||
async def test_submit_config_accepts_unified_format(self):
|
||||
"""Should accept valid unified config"""
|
||||
unified_config = {
|
||||
"name": "testunified",
|
||||
"description": "Test unified config",
|
||||
"merge_mode": "rule-based",
|
||||
"sources": [
|
||||
{
|
||||
"type": "documentation",
|
||||
"base_url": "https://docs.test.com/",
|
||||
"max_pages": 100
|
||||
},
|
||||
{
|
||||
"type": "github",
|
||||
"repo": "testorg/testrepo"
|
||||
}
|
||||
]
|
||||
}
|
||||
args = {
|
||||
"config_json": json.dumps(unified_config),
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
|
||||
with patch('github.Github') as mock_gh:
|
||||
mock_repo = MagicMock()
|
||||
mock_issue = MagicMock()
|
||||
mock_issue.html_url = "https://github.com/test/issue/2"
|
||||
mock_issue.number = 2
|
||||
mock_repo.create_issue.return_value = mock_issue
|
||||
mock_gh.return_value.get_repo.return_value = mock_repo
|
||||
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("Config submitted successfully", result[0].text)
|
||||
self.assertTrue("Unified" in result[0].text or "multi-source" in result[0].text)
|
||||
|
||||
async def test_submit_config_from_file_path(self):
|
||||
"""Should accept config_path parameter"""
|
||||
with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f:
|
||||
json.dump({
|
||||
"name": "testfile",
|
||||
"description": "From file",
|
||||
"base_url": "https://test.com/"
|
||||
}, f)
|
||||
temp_path = f.name
|
||||
|
||||
try:
|
||||
args = {
|
||||
"config_path": temp_path,
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
|
||||
with patch('github.Github') as mock_gh:
|
||||
mock_repo = MagicMock()
|
||||
mock_issue = MagicMock()
|
||||
mock_issue.html_url = "https://github.com/test/issue/3"
|
||||
mock_issue.number = 3
|
||||
mock_repo.create_issue.return_value = mock_issue
|
||||
mock_gh.return_value.get_repo.return_value = mock_repo
|
||||
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
self.assertIn("Config submitted successfully", result[0].text)
|
||||
finally:
|
||||
os.unlink(temp_path)
|
||||
|
||||
async def test_submit_config_detects_category(self):
|
||||
"""Should auto-detect category from config name"""
|
||||
args = {
|
||||
"config_json": '{"name": "react-test", "description": "React", "base_url": "https://react.dev/"}',
|
||||
"github_token": "fake_token"
|
||||
}
|
||||
|
||||
with patch('github.Github') as mock_gh:
|
||||
mock_repo = MagicMock()
|
||||
mock_issue = MagicMock()
|
||||
mock_issue.html_url = "https://github.com/test/issue/4"
|
||||
mock_issue.number = 4
|
||||
mock_repo.create_issue.return_value = mock_issue
|
||||
mock_gh.return_value.get_repo.return_value = mock_repo
|
||||
|
||||
result = await skill_seeker_server.submit_config_tool(args)
|
||||
# Verify category appears in result
|
||||
self.assertTrue("web-frameworks" in result[0].text or "Category" in result[0].text)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user