Conversation
…nd validation messages
…llm-validator integration
…n logic for reuse
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the MCP (Model Context Protocol) server implementation with significant architectural improvements:
- Migrates from numeric
nofield to stringidfield for policy rules, enabling more flexible rule identifiers - Integrates the official
modelcontextprotocol/go-sdkfor spec-compliant MCP communication - Adds comprehensive test coverage for MCP convention queries with filtering by category and language
Key Changes
- Rule identification system upgraded from sequential numbers to flexible string IDs (e.g., "SEC-001", "STYLE-001")
- MCP server now uses official SDK with proper stdio transport and tool registration
- Enhanced LLM validator to handle non-git-diff formatted code snippets
- Automatic policy conversion when user-policy.json is updated
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/schema/types.go | Replaced No field with ID field in UserRule struct, removing numeric constraint |
| internal/policy/manager.go | Updated validation logic to check for duplicate IDs instead of rule numbers |
| internal/server/static/policy-editor.js | Refactored DOM handling from data-rule-no to data-rule-id attributes with new ID generation |
| internal/policy/templates/*.json | Converted all template rules from numeric no to string id fields |
| internal/mcp/server.go | Major refactor: integrated go-sdk, added dual policy support, improved MCP response formatting |
| internal/mcp/server_test.go | Added comprehensive test suite for query_conventions functionality |
| internal/validator/llm_validator.go | Enhanced to handle non-git-diff code snippets by treating entire content as added lines |
| internal/converter/converter.go | Changed default engine from "custom" to "llm-validator", reformatted struct fields |
| internal/cmd/mcp.go | Simplified by moving conversion logic to mcp package, removed duplicate code |
| internal/cmd/dashboard.go | Added "dash" alias for dashboard command |
| internal/adapter/prettier/executor_test.go | Changed test to skip instead of fail when Prettier unavailable |
| go.mod | Added modelcontextprotocol/go-sdk v1.1.0 and related dependencies |
| go.sum | Updated checksums for new dependencies |
| tests/e2e/full_workflow_test.go | Updated test to use new PolicyRule structure with id and enhanced fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CodePolicy *schema.CodePolicy // Internal policy | ||
| LinterConfigs map[string]*linters.LinterConfig // Linter-specific configs | ||
| Results map[string]*linters.ConversionResult // Detailed results per linter | ||
| Warnings []string // Overall warnings |
There was a problem hiding this comment.
[nitpick] The field alignment changes appear to be purely cosmetic formatting adjustments. While this improves visual consistency, it creates unnecessary noise in the diff. Consider using a tool like gofmt or goimports to automatically handle alignment across the entire codebase consistently, rather than manual alignment in individual PRs.
| CodePolicy *schema.CodePolicy // Internal policy | |
| LinterConfigs map[string]*linters.LinterConfig // Linter-specific configs | |
| Results map[string]*linters.ConversionResult // Detailed results per linter | |
| Warnings []string // Overall warnings | |
| CodePolicy *schema.CodePolicy // Internal policy | |
| LinterConfigs map[string]*linters.LinterConfig // Linter-specific configs | |
| Results map[string]*linters.ConversionResult // Detailed results per linter | |
| Warnings []string // Overall warnings |
|
|
||
| // Check if conversion is needed | ||
| if s.userPolicy != nil { | ||
| needsConversion := s.needsConversion(codePolicyPath) |
There was a problem hiding this comment.
The needsConversion method is called with codePolicyPath as a parameter (line 154), but the method implementation doesn't use this parameter at all. The parameter should either be removed from the call, or if it's intended to check file modification times, it should be used in the implementation to compare timestamps between user-policy.json and code-policy.json.
| const maxId = appState.policy.rules.reduce((max, rule) => { | ||
| const ruleId = parseInt(rule.id, 10); | ||
| return isNaN(ruleId) ? max : Math.max(max, ruleId); | ||
| }, 0); | ||
| const newId = String(maxId + 1); |
There was a problem hiding this comment.
The ID generation logic has a potential collision issue when rules contain a mix of numeric and non-numeric IDs (e.g., "SEC-001", "STYLE-1", "5"). The parseInt on line 463 will return NaN for non-numeric IDs like "SEC-001", which are then excluded from the max calculation. This could result in duplicate IDs if existing rules have IDs like "SEC-001" and the new rule gets ID "1" when there's already a rule with ID "1".
Consider using a more robust ID generation strategy, such as:
- Using UUIDs
- Using a timestamp-based ID
- Prefixing numeric IDs with a consistent pattern (e.g., "RULE-1", "RULE-2")
- Scanning for all existing IDs and generating a truly unique one
| if !s.isRuleRelevant(rule, req) { | ||
| continue | ||
| severity := rule.Severity | ||
| if severity == "" && s.userPolicy.Defaults.Severity != "" { |
There was a problem hiding this comment.
Potential null pointer dereference: s.userPolicy.Defaults.Severity is accessed without checking if s.userPolicy.Defaults is nil. If Defaults is nil, this will cause a panic. Add a nil check before accessing Defaults.Severity:
if severity == "" && s.userPolicy.Defaults != nil && s.userPolicy.Defaults.Severity != "" {
severity = s.userPolicy.Defaults.Severity
}| if severity == "" && s.userPolicy.Defaults.Severity != "" { | |
| if severity == "" && s.userPolicy.Defaults != nil && s.userPolicy.Defaults.Severity != "" { |
MCP 서버 개선