Fix: Enable Recording for Proxy Mode (Static & Dynamic Servers) #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add MergeEnvironment() to merge parent env with overrides - Update Connect() to use inherited environment - Handle Windows case-insensitive env vars (PATH vs Path) - Add comprehensive unit tests with platform-specific cases - Maintain 100% backward compatibility (no schema changes) Fixes: Environment variables now inherit from parent process and apply per-server overrides, ensuring servers receive critical system variables (PATH, HOME, SSL certs, proxies) while allowing custom configuration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add tier-based inheritance modes (none, tier1, tier1+tier2, all) - Tier1: Baseline vars (PATH, HOME, USER, SHELL, LANG, LC_ALL, TZ, TMPDIR, TEMP, TMP) - Tier2: Network/TLS vars (SSL_CERT_FILE, SSL_CERT_DIR, REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE, NODE_EXTRA_CA_CERTS) - Support extras, prefixes, denylist, allow_denied_if_explicit - Block HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy/NO_PROXY/no_proxy by default (httpoxy mitigation) - Add comprehensive unit tests with 12 test cases - Update all 4 integration points (ProxyServer, Discoverer, DynamicProxyServer, DynamicWrapper) - Add validation for inheritance config - Default mode: tier1 (security-first) Security improvements: - Prevents accidental secret leakage to upstream MCP servers - Controlled inheritance with auditable configuration - Per-server and proxy-level defaults - Explicit override support Test coverage: - 12 new unit tests for envbuilder (all passing) - 9 existing tests maintained (zero regressions) - Manual testing verified tier1, tier1+tier2, and mode=all scenarios - HTTP_PROXY blocking and allow_denied_if_explicit override tested Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ance When DynamicWrapper creates ProxyServer, it pre-assigns the baseServer instance (which has management tools registered) to proxyServer.mcpServer before calling Initialize(). Previously, ProxyServer.Initialize() unconditionally created a new mcpServer instance, causing static server tools to be registered to an orphaned server that never handled requests. This resulted in only management tools being exposed (5 tools) while static config server tools (26 drmm_* tools) were invisible. Fix: Only create a new mcpServer if one doesn't already exist. This ensures that static tools are registered to the same server instance that handles incoming requests. Result: All 31 tools now properly exposed (5 management + 26 static server). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove trailing whitespace in main.go - Add clarifying comment about mark3labs/mcp-go stdio compatibility - Remove mcp-debug binary from root (builds to ./bin/mcp-debug instead) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add ENV_INHERITANCE.md with complete guide (31 KB) - Update README.md with environment inheritance section - Add 4 example configs (basic, tier2, advanced, none) - Add 4 test fixtures for inheritance testing - Document tier system, modes, security rationale, troubleshooting Documentation marked as DRAFT pending broader real-world testing. Successfully tested with Datto RMM MCP server: - Tier1 vars inherited (PATH, HOME, USER, SHELL, locale) - DATTO_* prefix matching working - Secrets properly blocked (AWS keys, etc.) - All 31 tools exposed and functional Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add CLAUDE.md with project context and development guidelines for AI assistants working on the mcp-debug codebase. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed json.MarshalIndent() to json.Marshal() in EnableRecording()
to fix critical playback incompatibility. The playback parser expects
line-delimited JSON (JSONL), but MarshalIndent creates multi-line
pretty-printed JSON that breaks parsing.
The header was being written as:
{
"start_time": "...",
"server_info": "..."
}
But the parser reads line-by-line, so each line failed JSON parsing
and the entire header was silently ignored.
Now writes single-line JSON as expected:
{"start_time":"...","server_info":"...","messages":[]}
Verified:
- Recording file creates valid JSONL format
- Playback parser successfully loads sessions
- Header fields are correctly parsed
Fixes: Recording → playback pipeline now functional
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Static servers from config.yaml were using proxy.CreateProxyHandler which had no recording capability. Only dynamically added servers (via server_add) used DynamicWrapper.createDynamicProxyHandler with recording. Changes: - Add RecorderFunc type and optional recorder parameter to proxy.CreateProxyHandler - Add recording calls for requests, responses, and errors in CreateProxyHandler - Add recorderFunc field to ProxyServer - Inject DynamicWrapper.recordMessage into ProxyServer when recording enabled - Update CreateHandlerForTool to accept optional recorder This fixes the critical bug where recording files were created but captured zero messages when using static servers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added detailed documentation for session recording feature in proxy mode: New File: - docs/RECORDING.md - Complete guide covering: - Recording format (JSONL structure) - What gets recorded (static/dynamic servers, management tools, errors) - Playback modes (client/server) - Common workflows (debugging, regression testing, documentation) - Tips & best practices (filtering, sensitive data, file management) - Troubleshooting guide - Current limitations and future enhancements Updated: - README.md - Added link to recording documentation - README.md - Clarified what gets recorded (all servers, management tools) The recording feature is now fully functional and documented for: - Static servers (from config.yaml) - Dynamic servers (via server_add) - All management operations - Request/response pairs with timestamps - Error responses Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes two critical bugs that prevented the recording feature from working in proxy mode, and adds comprehensive documentation for the recording functionality.
Status: ✅ Fully tested and validated with real-world MCP traffic
Problems Fixed
Bug #1: Recording Format Incompatibility
Issue: Recording header used json.MarshalIndent() creating multi-line pretty-printed JSON that broke the playback parser which expects line-delimited JSONL format.
Impact: Playback parser silently ignored the header, causing playback functionality to fail.
Fix: Changed to json.Marshal() for single-line JSON header (commit d0df465)
Bug #2: Zero Messages Recorded from Static Servers
Issue: Recording file was created but captured zero messages when using servers defined in config.yaml.
Root Cause: Static servers used proxy.CreateProxyHandler which had no recording capability. Only dynamically added servers (via server_add) used DynamicWrapper.createDynamicProxyHandler which had recording calls.
Impact: Recording feature was non-functional for the most common use case (static server configuration).
Fix: Added recorder function injection throughout the proxy handler stack (commit e77a4e0)
Changes Made
Code Changes
Files Modified:
Architecture:
Before: proxy.CreateProxyHandler → No recording
After: proxy.CreateProxyHandler(client, tool, recorderFunc) → Records all traffic
Documentation Added
New File: docs/RECORDING.md (400+ lines)
Updated: README.md
Testing & Validation
Real-World Test: Validated with datto_rmm_smart_mcp project using live Datto RMM API
Test Results:
Recording: /mnt/c/dev/projects/github/datto_rmm_smart_mcp/mcp-session.jsonl
Messages: 4 (2 tool calls with request/response pairs)
Tools: drmm_device_resolve, drmm_device_get_fields
Server: datto-rmm
Format: ✅ Valid JSONL, ✅ Single-line JSON header, ✅ JSON-RPC compliant
Validation Checks:
What Now Works
✅ Static Servers - Servers from config.yaml now record all tool calls
✅ Dynamic Servers - Servers added via server_add continue to record
✅ Management Tools - server_add operations are recorded (other management tools in future PR)
✅ Error Recording - Failed requests and error responses are captured
✅ JSONL Format - Valid line-delimited JSON compatible with playback parser
✅ JSON-RPC Messages - Proper JSON-RPC structure for playback compatibility
Breaking Changes
None - This is a bug fix that makes existing functionality work correctly.
Future Work
Not included in this PR (can be separate PRs):
Related Issues
Fixes the recording feature which was previously non-functional in proxy mode with static servers.
Checklist
Commits: