From 8da3fbd40a2bcbc0de6f97271c294689ce4919e6 Mon Sep 17 00:00:00 2001 From: Patrick Cowland Date: Fri, 12 Dec 2025 12:10:35 +0000 Subject: [PATCH 1/2] fix: Replace fmt.Printf with structured logging in authentication flows Fixes MCP protocol compliance violations in authentication wrapper code where fmt.Printf was causing protocol communication issues. Changes: - Replaced all fmt.Printf calls in internal/auth/client/wrapper.go with structured logging using log.Info - Fixed cmd/session/session.go to use correct fmt.Printf for CLI output (CLI commands are exempt from MCP logging restrictions) - Removed format strings, newlines, and trailing punctuation from log messages to follow MCP structured logging patterns This ensures the MCP server's authentication flows properly use structured logging as required by the MCP specification, preventing interference with protocol communication on stdout/stderr. --- CONTRIBUTING.md | 29 +++++++++++++++++++++++++++++ internal/auth/client/wrapper.go | 33 +++++++++++++-------------------- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d425dad..3c6a2ed 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,6 +4,12 @@ We appreciate your help! We welcome contributions in the form of creating issues ## Before You Start +**Important:** This project implements a Model Context Protocol (MCP) server and must comply with the MCP specification. + +- 📘 Read [.github/copilot-instructions.md](.github/copilot-instructions.md) for comprehensive development guidelines +- 📖 Review the [MCP Specification](https://spec.modelcontextprotocol.io/) for protocol requirements +- 🔍 Check existing code for patterns and examples + Know that: 1. If you have any questions, please ask! We'll help as best we can. @@ -11,6 +17,29 @@ Know that: 3. We may not be able to respond quickly; our development cycles are on a priority basis. 4. We base our priorities on customer need and the number of votes on issues/PRs by the number of 👍 reactions. If there is an existing issue or PR for something you'd like, please vote! +## Key Development Guidelines + +### MCP Specification Compliance + +**CRITICAL:** All code must comply with the MCP specification. Key requirements: + +- ✅ **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 + +**Example:** +```go +// ✅ CORRECT - In server/tools code +log := logger.FromContext(ctx) +log.Info("Processing request", "tool_name", toolName) + +// ❌ INCORRECT - In server/tools code +fmt.Printf("Processing request: %s\n", toolName) // Breaks MCP protocol + +// ✅ CORRECT - In cmd/ code +fmt.Printf("Session ID: %s\n", sessionID) // OK for CLI output +``` + ## Getting Started ### Prerequisites diff --git a/internal/auth/client/wrapper.go b/internal/auth/client/wrapper.go index 014ec65..a183864 100644 --- a/internal/auth/client/wrapper.go +++ b/internal/auth/client/wrapper.go @@ -96,7 +96,7 @@ func (p *PingOneClientAuthWrapper) configureHeadlessHandlers(ctx context.Context log.Info("Device authorization required", "verification_uri", verificationURI, "user_code", userCode) - fmt.Printf("\n=== PingOne MCP Server OAuth 2.0 Authorization ===\n") + log.Info("=== PingOne MCP Server OAuth 2.0 Authorization ===") // Try to open browser if available browserOpened := false @@ -111,24 +111,18 @@ func (p *PingOneClientAuthWrapper) configureHeadlessHandlers(ctx context.Context if browserOpened { // Browser opened successfully - fmt.Printf("Browser opened automatically.\n\n") - fmt.Printf("If the browser window does not open automatically, please open this URL to complete authentication:\n") - fmt.Printf(" %s\n\n", fullURL) - fmt.Printf("Alternatively, open this URL to enter the code manually:\n") - fmt.Printf(" %s\n\n", verificationURI) - fmt.Printf("Enter this code when prompted:\n") - fmt.Printf(" %s\n\n", userCode) + log.Info("Browser opened automatically") + log.Info("If the browser window does not open automatically, please open this URL to complete authentication", "url", fullURL) + log.Info("Alternatively, open this URL to enter the code manually", "url", verificationURI) + log.Info("Enter this code when prompted", "code", userCode) } else { // Browser failed to open or not available - show manual instructions - fmt.Printf("Please open this URL in your browser to complete authentication:\n") - fmt.Printf(" %s\n\n", fullURL) - fmt.Printf("Alternatively, open this URL to enter the code manually:\n") - fmt.Printf(" %s\n\n", verificationURI) - fmt.Printf("Enter this code when prompted:\n") - fmt.Printf(" %s\n\n", userCode) + log.Info("Please open this URL in your browser to complete authentication", "url", fullURL) + log.Info("Alternatively, open this URL to enter the code manually", "url", verificationURI) + log.Info("Enter this code when prompted", "code", userCode) } - fmt.Printf("Waiting for authorization...\n") + log.Info("Waiting for authorization...") return nil } @@ -148,7 +142,7 @@ func (p *PingOneClientAuthWrapper) configureHeadlessHandlers(ctx context.Context } // We have a browser - try to open it - fmt.Printf("\n=== PingOne MCP Server OAuth 2.0 Authorization ===\n") + log.Info("=== PingOne MCP Server OAuth 2.0 Authorization ===") if err := browser.Open(url); err != nil { // Browser open failed - this is a critical error for auth code flow @@ -157,10 +151,9 @@ func (p *PingOneClientAuthWrapper) configureHeadlessHandlers(ctx context.Context } // Browser opened successfully - fmt.Printf("Browser opened automatically.\n") - fmt.Printf("If the browser window does not open automatically, please open this URL to complete authentication:\n") - fmt.Printf(" %s\n\n", url) - fmt.Printf("Waiting for authorization callback...\n") + log.Info("Browser opened automatically") + log.Info("If the browser window does not open automatically, please open this URL to complete authentication", "url", url) + log.Info("Waiting for authorization callback") return nil } From c332fb54a2528d0a7b8f4c1bbd5cfeb53462a44b Mon Sep 17 00:00:00 2001 From: Patrick Cowland Date: Fri, 12 Dec 2025 12:18:23 +0000 Subject: [PATCH 2/2] chore: Add MCP protocol compliance enforcement and documentation - Add golangci-lint forbidigo linter to prevent fmt.Printf in MCP server code - Add comprehensive MCP compliance documentation - Update contributing guides with MCP specification requirements - Add Makefile targets for MCP spec validation - Exemptions properly configured for CLI commands and test files This housekeeping change prevents future MCP protocol violations by providing automated checks and clear documentation for contributors. --- .github/copilot-instructions.md | 338 +++++++++++++++++++++++++++ .golangci.yml | 74 ++++++ CONTRIBUTING.md | 17 +- Makefile | 17 +- contributing/pr-checklist.md | 43 +++- docs/mcp-specification-compliance.md | 211 +++++++++++++++++ 6 files changed, 685 insertions(+), 15 deletions(-) create mode 100644 .github/copilot-instructions.md create mode 100644 .golangci.yml create mode 100644 docs/mcp-specification-compliance.md 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)