diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..24ee832 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,338 @@ +# GitHub Copilot Instructions for PingOne MCP Server + +## Project Overview + +This project implements a Model Context Protocol (MCP) server for PingOne identity and access management platform. The MCP specification defines standards for communication between AI assistants and external systems. + +**Key Resources:** +- [MCP Specification](https://spec.modelcontextprotocol.io/) +- [MCP Go SDK](https://github.com/modelcontextprotocol/go-sdk) + +## MCP Specification Compliance + +**CRITICAL:** All code MUST comply with the Model Context Protocol specification. This is not optional. + +### 1. Logging Standards (MCP Specification Requirement) + +The MCP specification requires that servers use **structured logging only** to stderr. Direct output to stdout/stderr via `fmt.Printf`, `fmt.Println`, `print`, or `println` will break MCP protocol communication. + +#### Required Logging Pattern: + +**✅ CORRECT - Use structured logging:** +```go +import "github.com/pingidentity/pingone-mcp-server/internal/logger" + +// Get logger from context +log := logger.FromContext(ctx) + +// Use structured logging with key-value pairs +log.Info("Operation completed", "resource_id", resourceID, "status", "success") +log.Debug("Processing request", "tool_name", toolName) +log.Error("Operation failed", "error", err) +log.Warn("Deprecated feature used", "feature", featureName) +``` + +**❌ INCORRECT - Never use in internal/ packages:** +```go +// NEVER use these in internal/server, internal/tools, internal/sdk, or internal/auth: +fmt.Printf("Operation completed\n") // Breaks MCP protocol +fmt.Println("Processing...") // Breaks MCP protocol +print("Debug info") // Breaks MCP protocol +println("Error occurred") // Breaks MCP protocol +``` + +**Note:** The only exception is `cmd/` packages which provide CLI commands for users (not MCP server code). + +#### Enforcement: + +This is enforced by: +1. **golangci-lint** with `forbidigo` linter - checks at lint time +2. **Code review** - reviewers will reject PRs that violate this +3. **MCP specification compliance** - violations break protocol communication + +### 2. Error Handling Standards + +Follow the project's error handling patterns for consistent error reporting: + +**For Tool Errors:** +```go +import "github.com/pingidentity/pingone-mcp-server/internal/errs" + +// When a tool encounters an error +return errs.NewToolError("tool_name", err, "Additional context about what failed") +``` + +**For Command Errors:** +```go +import "github.com/pingidentity/pingone-mcp-server/internal/errs" + +// When a CLI command encounters an error +return errs.NewCommandError("command_name", err) +``` + +**For API Errors:** +```go +import "github.com/pingidentity/pingone-mcp-server/internal/errs" + +// When processing API responses +apiErr := errs.HandlePingOneAPIError(httpResponse, responseBody, err) +if apiErr != nil { + return errs.NewToolError("tool_name", apiErr, "Failed to call PingOne API") +} +``` + +### 3. Context Management + +Always pass context through the call chain for: +- Logging (logger is attached to context) +- Cancellation signals +- Transaction tracking +- Request lifecycle management + +**Pattern:** +```go +func ProcessRequest(ctx context.Context, input string) error { + log := logger.FromContext(ctx) // Get logger from context + + // Pass context to downstream calls + result, err := someOperation(ctx, input) + if err != nil { + log.Error("Operation failed", "error", err) + return err + } + + return nil +} +``` + +### 4. Tool Implementation Standards + +When implementing MCP tools, follow these patterns: + +**Tool Structure:** +```go +type MyTool struct { + clientFactory sdk.ClientFactory + // ... other dependencies +} + +func (t *MyTool) Execute(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResponse, error) { + log := logger.FromContext(ctx) + log.Debug("Tool execution started") + + // 1. Parse and validate input arguments + var args MyToolArgs + if err := parseToolArguments(req, &args); err != nil { + return nil, errs.NewToolError("my_tool", err, "Failed to parse arguments") + } + + // 2. Perform operations + result, err := t.performOperation(ctx, args) + if err != nil { + return nil, errs.NewToolError("my_tool", err, "Operation failed") + } + + // 3. Return structured response + return newToolResponse(result) +} +``` + +**Tool Registration:** +```go +func RegisterMyTools(ctx context.Context, server *mcp.Server, factory sdk.ClientFactory) error { + log := logger.FromContext(ctx) + + tool := NewMyTool(factory) + + if err := server.AddTool(tool.Definition(), tool.Execute); err != nil { + log.Error("Failed to register tool", "tool", "my_tool", "error", err) + return err + } + + log.Debug("Tool registered", "tool", "my_tool") + return nil +} +``` + +### 5. Testing Standards + +**Unit Tests:** +- Test all public functions and methods +- Use table-driven tests for multiple scenarios +- Mock external dependencies (SDK clients, token stores, etc.) +- Use `internal/testutils` for common test assertions + +**Integration Tests:** +- Test tool execution end-to-end +- Use `testutils.NewTestServer()` for MCP server testing +- Verify proper error handling and logging +- Test both success and failure scenarios + +**Example:** +```go +func TestMyTool_Execute(t *testing.T) { + tests := []struct { + name string + input map[string]interface{} + want interface{} + wantErr bool + }{ + { + name: "valid input", + input: map[string]interface{}{"key": "value"}, + want: expectedResult, + wantErr: false, + }, + // ... more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test implementation + }) + } +} +``` + +## Code Organization + +- **cmd/**: CLI commands for user interaction (fmt.Printf allowed here) +- **internal/server**: MCP server implementation (structured logging only) +- **internal/tools**: MCP tool implementations (structured logging only) +- **internal/sdk**: PingOne SDK client wrappers (structured logging only) +- **internal/auth**: Authentication and session management (structured logging only) +- **internal/logger**: Logging infrastructure (can use fmt for implementation) +- **internal/errs**: Error handling utilities (can use fmt for formatting) +- **internal/testutils**: Test utilities and helpers + +## Git Commit Standards + +Follow these commit message guidelines: +- Use present tense ("Add feature" not "Added feature") +- Use imperative mood ("Move cursor to..." not "Moves cursor to...") +- Reference issue numbers when applicable +- Keep first line under 72 characters + +## Pull Request Checklist + +Before submitting a PR, ensure: +- [ ] Code follows MCP specification (especially logging requirements) +- [ ] `make test` passes +- [ ] `make lint` passes (includes forbidigo checks) +- [ ] New tools have proper documentation +- [ ] Error handling follows project patterns +- [ ] All code uses structured logging (no fmt.Printf in internal/) +- [ ] Tests cover new functionality +- [ ] PR template is filled out completely + +## Additional Notes + +- **Copyright Headers**: All Go files must include the copyright header: + ```go + // Copyright © 2025 Ping Identity Corporation + ``` + +- **Dependencies**: Prefer standard library when possible. New dependencies require approval. + +- **Documentation**: Update relevant docs when adding new tools or features: + - Update README.md for user-facing changes + - Update docs/troubleshooting.md for common issues + - Add inline godoc comments for exported functions + +## Legitimate Stdout Output - When and How + +### Where fmt.Printf IS Allowed + +The linting rules **automatically exempt** these locations: + +1. **`cmd/` directory** - CLI commands for user interaction: + ```go + // cmd/session/session.go - ✅ ALLOWED + fmt.Println("Current Session Information:") + fmt.Printf(" Session ID: %s\n", sessionID) + ``` + +2. **Test files** (`*_test.go`): + ```go + // any_test.go - ✅ ALLOWED + fmt.Printf("Debug test output: %v\n", testData) + ``` + +3. **`internal/errs/` directory** - Error formatting: + ```go + // internal/errs/error.go - ✅ ALLOWED + return fmt.Sprintf("error: %s", msg) + ``` + +4. **`internal/logger/` directory** - Logger implementation: + ```go + // internal/logger/logger.go - ✅ ALLOWED + fmt.Fprintf(os.Stderr, "[LOG] %s\n", msg) // Writing to stderr for bootstrap logging + ``` + +### Where fmt.Printf IS FORBIDDEN + +**MCP server runtime code** - These packages must use structured logging only: +- `internal/server` +- `internal/tools` +- `internal/sdk` +- `internal/auth/client` +- `internal/auth/login` +- `internal/auth/logout` + +### What If I Need Output in Server Code? + +You should **never need direct stdout** in MCP server code. Use these alternatives: + +| Need | ❌ Don't Use | ✅ Use Instead | +|------|-------------|----------------| +| Debug info | `fmt.Printf("Debug: %v", data)` | `log.Debug("Debug info", "data", data)` | +| User feedback | `fmt.Println("Processing...")` | Return through MCP response | +| Error messages | `fmt.Printf("Error: %v", err)` | `log.Error("Operation failed", "error", err)` | +| Progress updates | `fmt.Printf("50%% complete")` | Use MCP progress notifications | +| Tool results | `fmt.Printf("%v", result)` | Return via `mcp.CallToolResponse` | + +### Temporarily Disabling Checks (Development Only) + +If you need to temporarily disable checks during development: + +```go +// TEMPORARY: Remove before committing +// nolint:forbidigo +fmt.Printf("Debug output: %v\n", data) +``` + +**Important**: These should NEVER be committed. The PR checklist will catch them. + +### Adding New Exemptions + +If you need to exempt a new package, update `.golangci.yml`: + +```yaml +issues: + exclude-rules: + # Allow in new package + - path: ^internal/mynewpackage/ + linters: + - forbidigo +``` + +However, **be very careful** - most new packages should follow MCP logging patterns. + +## Common Mistakes to Avoid + +❌ **Using fmt.Printf in server/tool code** - Breaks MCP protocol +❌ **Not passing context through call chains** - Breaks logging and cancellation +❌ **Ignoring errors** - All errors must be handled or explicitly ignored with comment +❌ **Missing structured logging** - Always use logger.FromContext(ctx) +❌ **Hardcoding configuration** - Use environment variables or config files +❌ **Not testing error paths** - Error handling must be tested + +## Questions? + +If you're unsure about MCP specification requirements or project patterns: +1. Check existing code in the same package for patterns +2. Review the MCP specification documentation +3. Ask in PR comments for clarification +4. Check `internal/logger` for logging examples +5. Check `internal/tools` for tool implementation examples diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..50d041d --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,74 @@ +# golangci-lint configuration for PingOne MCP Server +# https://golangci-lint.run/usage/configuration/ + +run: + timeout: 2m + tests: true + modules-download-mode: readonly + +linters: + enable: + - errcheck # Check for unchecked errors + - gosimple # Simplify code + - govet # Vet examines Go source code + - ineffassign # Detect ineffectual assignments + - staticcheck # Staticcheck is a go vet on steroids + - typecheck # Type checks + - unused # Check for unused constants, variables, functions and types + - forbidigo # Forbid specific identifiers (used to prevent fmt.Printf in MCP server code) + - revive # Fast, configurable, extensible, flexible, and beautiful linter + +linters-settings: + forbidigo: + # Forbid the following patterns in specific directories + forbid: + # Prevent fmt.Printf/Println in MCP server code (internal/, excluding cmd/ for CLI output) + - p: ^fmt\.Printf$ + msg: "Use structured logging (logger.FromContext(ctx)) instead of fmt.Printf in server code. See MCP specification for proper logging patterns." + pkg: ^github\.com/pingidentity/pingone-mcp-server/internal/(server|tools|sdk|auth/client|auth/login|auth/logout).* + - p: ^fmt\.Println$ + msg: "Use structured logging (logger.FromContext(ctx)) instead of fmt.Println in server code. See MCP specification for proper logging patterns." + pkg: ^github\.com/pingidentity/pingone-mcp-server/internal/(server|tools|sdk|auth/client|auth/login|auth/logout).* + - p: ^print$ + msg: "Use structured logging (logger.FromContext(ctx)) instead of print() in server code. See MCP specification for proper logging patterns." + pkg: ^github\.com/pingidentity/pingone-mcp-server/internal/(server|tools|sdk|auth/client|auth/login|auth/logout).* + - p: ^println$ + msg: "Use structured logging (logger.FromContext(ctx)) instead of println() in server code. See MCP specification for proper logging patterns." + pkg: ^github\.com/pingidentity/pingone-mcp-server/internal/(server|tools|sdk|auth/client|auth/login|auth/logout).* + # Allow fmt.Printf/Println in cmd/ for CLI user output + # Allow in internal/errs for error formatting + # Allow in internal/logger for logger implementation + # Allow in test files + analyze-types: true + + revive: + rules: + - name: exported + severity: warning + disabled: false + +issues: + exclude-rules: + # Allow fmt.Printf/Println in test files + - path: _test\.go + linters: + - forbidigo + # Allow fmt.Printf/Println in cmd/ directory (CLI output) + - path: ^cmd/ + linters: + - forbidigo + # Allow in internal/errs (error formatting) + - path: ^internal/errs/ + linters: + - forbidigo + # Allow in internal/logger (logger implementation) + - path: ^internal/logger/ + linters: + - forbidigo + # Allow in internal/testutils (test utilities) + - path: ^internal/testutils/ + linters: + - forbidigo + + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3c6a2ed..e7064a4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,6 +26,7 @@ Know that: - ✅ **Use structured logging** in server code (`internal/server`, `internal/tools`, `internal/sdk`, `internal/auth`) - ❌ **Never use `fmt.Printf`** in server code (breaks MCP protocol communication) - ✅ **Use `fmt.Printf`** in CLI commands (`cmd/` directory) for user output +- 📝 See [.github/copilot-instructions.md](.github/copilot-instructions.md) for complete logging patterns **Example:** ```go @@ -106,14 +107,14 @@ make test 3. **Test your changes:** ```bash -# Run tests -make test - -# Run linting -make lint - -# Build to verify -make build +# Run all validation checks +make validate-all + +# Or run individually: +make test # Unit and integration tests +make lint # Code quality and linting +make mcp-spec-check # MCP specification compliance +make build # Verify build ``` 4. **Commit your changes:** diff --git a/Makefile b/Makefile index 0764606..f0f7578 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test lint +.PHONY: build test lint mcp-spec-check validate-all default: build @@ -11,4 +11,17 @@ test: go test -v -timeout 1m ./... lint: - go tool golangci-lint run --timeout 2m + go tool golangci-lint run --timeout 2m --config .golangci.yml + +# MCP Specification compliance check - ensures no fmt.Printf/Println in server code +mcp-spec-check: + @echo "Checking MCP specification compliance (no fmt.Printf/Println in internal/server, internal/tools, internal/sdk, internal/auth)..." + @! grep -rn --include="*.go" --exclude="*_test.go" -E 'fmt\.(Printf|Println|Print|Fprint|Fprintf|Fprintln)' \ + internal/server internal/tools internal/sdk internal/auth/client internal/auth/login internal/auth/logout 2>/dev/null || \ + (echo "ERROR: Found fmt.Printf/Println in MCP server code. Use structured logging instead (logger.FromContext(ctx))." && \ + echo "See .github/copilot-instructions.md for proper logging patterns." && exit 1) + @echo "✓ MCP specification compliance verified" + +# Run all validation checks +validate-all: test lint mcp-spec-check + @echo "✓ All validation checks passed" diff --git a/contributing/pr-checklist.md b/contributing/pr-checklist.md index 9f26ed7..39197e8 100644 --- a/contributing/pr-checklist.md +++ b/contributing/pr-checklist.md @@ -65,10 +65,40 @@ make lint This includes: - Go vet checks -- golangci-lint +- golangci-lint with forbidigo (prevents fmt.Printf in server code) - Import organization checks - Go code formatting +- [ ] **MCP Specification Compliance**. **CRITICAL:** Verify code follows MCP specification requirements: + +```shell +make mcp-spec-check +``` +*Verification*: Command must exit with code 0 + +**Important:** The MCP specification requires structured logging only. Direct output to stdout/stderr via `fmt.Printf`, `fmt.Println`, `print`, or `println` will break MCP protocol communication. + +- ✅ **CORRECT** - Use in MCP server code (`internal/server`, `internal/tools`, `internal/sdk`, `internal/auth`): + ```go + log := logger.FromContext(ctx) + log.Info("Operation completed", "resource_id", resourceID) + log.Error("Operation failed", "error", err) + ``` + +- ❌ **INCORRECT** - Never use in MCP server code: + ```go + fmt.Printf("Operation completed\n") // Breaks MCP protocol + fmt.Println("Processing...") // Breaks MCP protocol + ``` + +- ✅ **ALLOWED** - Can use in CLI commands (`cmd/` directory): + ```go + fmt.Println("Session Information:") // OK for CLI output + fmt.Printf(" Status: %s\n", status) // OK for CLI output + ``` + +See [.github/copilot-instructions.md](../.github/copilot-instructions.md) for complete logging patterns and MCP specification requirements. + ## Testing ### Unit Tests @@ -163,15 +193,18 @@ make test ## Final Checks -- [ ] **All Development Checks**. Run the comprehensive development check: +- [ ] **All Development Checks**. Run the comprehensive validation: ```shell -make test -make lint -make build +make validate-all ``` *Verification*: All commands exit with code 0 +This runs: +- `make test` - Unit and integration tests +- `make lint` - Code quality and linting +- `make mcp-spec-check` - MCP specification compliance + - [ ] **CI Compatibility**. Verify your changes will pass automated CI checks by ensuring all the above steps pass locally: - *Verification*: All previous verification steps completed successfully diff --git a/docs/mcp-specification-compliance.md b/docs/mcp-specification-compliance.md new file mode 100644 index 0000000..a2c3f14 --- /dev/null +++ b/docs/mcp-specification-compliance.md @@ -0,0 +1,211 @@ +# MCP Specification Compliance + +This document explains how the PingOne MCP Server enforces compliance with the [Model Context Protocol (MCP) Specification](https://spec.modelcontextprotocol.io/). + +## Overview + +The MCP specification requires that servers use **structured logging only** to stderr. Direct output to stdout/stderr via `fmt.Printf`, `fmt.Println`, `print`, or `println` will break MCP protocol communication. + +## Automated Enforcement + +The project enforces MCP specification compliance through multiple layers: + +### 1. golangci-lint with forbidigo + +The `.golangci.yml` configuration uses the `forbidigo` linter to prevent `fmt.Printf`, `fmt.Println`, `print`, and `println` in MCP server code. + +**Protected packages** (where fmt.Printf is forbidden): +- `internal/server` +- `internal/tools` +- `internal/sdk` +- `internal/auth/client` +- `internal/auth/login` +- `internal/auth/logout` + +**Allowed packages** (where fmt.Printf is permitted): +- `cmd/` - CLI commands for user output +- `internal/errs` - Error formatting +- `internal/logger` - Logger implementation +- `*_test.go` - Test files + +### 2. Makefile Validation + +The `Makefile` includes an `mcp-spec-check` target that performs grep-based validation: + +```makefile +make mcp-spec-check +``` + +This check: +- Scans protected packages for `fmt.Printf` and related functions +- Excludes test files and allowed packages +- Provides clear error messages with remediation guidance + +### 3. Comprehensive Validation + +Run all validation checks together: + +```makefile +make validate-all +``` + +This runs: +- `make test` - Unit and integration tests +- `make lint` - Code quality and linting (includes forbidigo checks) +- `make mcp-spec-check` - MCP specification compliance + +## Proper Logging Patterns + +### ✅ CORRECT - Structured Logging + +In MCP server code (`internal/server`, `internal/tools`, `internal/sdk`, `internal/auth`): + +```go +import "github.com/pingidentity/pingone-mcp-server/internal/logger" + +// Get logger from context +log := logger.FromContext(ctx) + +// Use structured logging with key-value pairs +log.Info("Operation completed", "resource_id", resourceID, "status", "success") +log.Debug("Processing request", "tool_name", toolName) +log.Error("Operation failed", "error", err) +log.Warn("Deprecated feature used", "feature", featureName) +``` + +### ❌ INCORRECT - Direct Output + +Never use these in MCP server code: + +```go +// These break MCP protocol communication +fmt.Printf("Operation completed\n") // ❌ Breaks MCP protocol +fmt.Println("Processing...") // ❌ Breaks MCP protocol +print("Debug info") // ❌ Breaks MCP protocol +println("Error occurred") // ❌ Breaks MCP protocol +``` + +### ✅ ALLOWED - CLI Commands + +In `cmd/` packages (CLI commands for users): + +```go +// These are fine for CLI output to users +fmt.Println("Session Information:") // ✅ OK for CLI output +fmt.Printf(" Status: %s\n", status) // ✅ OK for CLI output +``` + +## Why This Matters + +The MCP specification requires structured logging because: + +1. **Protocol Communication**: MCP uses stdin/stdout for protocol messages. Direct `fmt.Printf` output interferes with protocol communication. + +2. **Structured Data**: Structured logging provides consistent, parsable output that can be filtered, searched, and analyzed. + +3. **Contextual Information**: Logger from context includes request tracking, session IDs, and transaction IDs automatically. + +4. **Debug Control**: Structured logging can be controlled via log levels without code changes. + +## CI/CD Integration + +The compliance checks are integrated into the development workflow: + +1. **Pre-commit**: Developers should run `make lint` before committing +2. **Pull Request**: CI runs `make validate-all` on all PRs +3. **Code Review**: Reviewers verify compliance with MCP patterns + +## Frequently Asked Questions + +### Q: What if I legitimately need to output to stdout? + +**A:** The short answer is: **you don't need to in MCP server code**. + +The linting rules already exempt the appropriate locations: + +1. **CLI Commands (`cmd/` directory)**: Use `fmt.Printf` freely for user-facing output + ```go + // cmd/session/session.go - This is fine + fmt.Println("Session Information:") + fmt.Printf(" Status: %s\n", status) + ``` + +2. **Test Files**: Use `fmt.Printf` for test debugging + ```go + // any_test.go - This is fine + fmt.Printf("Test data: %+v\n", testData) + ``` + +3. **MCP Server Code**: Use structured logging instead + ```go + // internal/tools/mytool.go - Use structured logging + log := logger.FromContext(ctx) + log.Info("Processing request", "data", data) + ``` + +### Q: How do I output tool results to users? + +**A:** Tool results should be returned through MCP responses, not printed: + +```go +// ❌ Don't do this +fmt.Printf("Result: %v\n", result) + +// ✅ Do this instead +return &mcp.CallToolResponse{ + Content: []mcp.Content{ + mcp.TextContent(fmt.Sprintf("Result: %v", result)), + }, +}, nil +``` + +### Q: What about debugging during development? + +**A:** Use structured logging with Debug level: + +```go +// ❌ Don't do this +fmt.Printf("Debug: processing user %s\n", userID) + +// ✅ Do this instead +log.Debug("Processing user", "user_id", userID, "step", "validation") +``` + +You can control log verbosity through environment variables without changing code. + +### Q: Can I temporarily bypass the check for testing? + +**A:** During development only, you can use `nolint` directives: + +```go +// TEMPORARY: Remove before committing +// nolint:forbidigo +fmt.Printf("Debug output\n") +``` + +**Warning**: These should NEVER be committed. PR reviewers will reject them. + +### Q: What if I need to add a new exempt package? + +**A:** Update `.golangci.yml` to add exemptions, but be very careful: + +```yaml +issues: + exclude-rules: + - path: ^internal/mypackage/ + linters: + - forbidigo +``` + +Most packages should follow MCP logging patterns. Only exempt packages that: +- Provide CLI interfaces to users (`cmd/`) +- Implement logging infrastructure (`internal/logger`) +- Format errors for logging (`internal/errs`) +- Are test utilities + +## References + +- [MCP Specification](https://spec.modelcontextprotocol.io/) +- [GitHub Copilot Instructions](.github/copilot-instructions.md) +- [PR Checklist](../contributing/pr-checklist.md) +- [golangci-lint Configuration](../.golangci.yml)